feat(api): determine if docker container has update #1582
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds GraphQL fields and types for Docker update status, new services to read/refresh PHP-backed digests, a scheduled job to refresh digests, feature-flag decorators and schema transformer, AsyncMutex utility and tests, Vite plugin to emit PHP assets, config split for docker runtime vs organizer, and related tests and env/gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as AppModule
participant JM as JobModule
participant CSJ as ContainerStatusJob
participant DConf as DockerConfigService
participant DMan as DockerManifestService
participant DPHP as DockerPhpService
App->>JM: import JobModule (initializes scheduler)
App->>CSJ: provide ContainerStatusJob
CSJ->>DConf: getConfig()
DConf-->>CSJ: { updateCheckCronSchedule }
CSJ->>CSJ: register CronJob(schedule) & start
Note over CSJ: initial @Timeout refresh at startup
CSJ-->>DMan: refreshDigests()
DMan->>DPHP: refreshDigestsViaPhp()
DPHP-->>DMan: success/failure
DMan-->>CSJ: Boolean
Note over CSJ: cron repeats -> onTick -> refresh flow
sequenceDiagram
autonumber
participant Client
participant DRes as DockerResolver
participant DCont as DockerContainerResolver
participant DMan as DockerManifestService
participant DPHP as DockerPhpService
Client->>DRes: query containerUpdateStatuses
DRes->>DPHP: getContainerUpdateStatuses()
DPHP-->>DRes: [ExplicitStatusItem]
DRes-->>Client: statuses
Client->>DCont: resolve isUpdateAvailable(container)
DCont->>DMan: isUpdateAvailableCached(imageRef)
DMan-->>DCont: true/false/null
DCont-->>Client: value
Client->>DCont: mutation refreshDockerDigests
DCont->>DMan: refreshDigests()
DMan-->>DCont: Boolean
DCont-->>Client: Boolean
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
pujitm
force-pushed
the
feat/docker-template
branch
from
242d6a2 to
a6df0bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts (1)
185-327: Consider adding unit tests for getLocalDigest, isRebuildReady, and isUpdateAvailable.You already mock docker utils at Line 13. Adding small, behavior-focused tests for:
- getLocalDigest: returns digest from RepoDigests, handles empty/missing arrays.
- isRebuildReady: returns true when the referenced container in networkMode cannot be inspected.
- isUpdateAvailable: returns boolean when both digests exist; null otherwise.
This closes coverage gaps for local state paths that don’t involve network I/O.
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts (2)
16-27: Relax strict 401 assertion to tolerate proxy/CDN variance.Some paths may intermittently return 405, 403, or 429. To avoid flakes, assert that it’s a challenge/unauthorized class rather than exactly 401.
- expect(result.statusCode).toBe(401); + expect([401, 403, 405, 429]).toContain(result.statusCode);
125-134: Tighten concurrency to avoid rate limiting or parameterize it via env.Ten concurrent HEAD requests to Docker Hub can spike rate limits. Either reduce default concurrency or make it configurable.
-const promises = Array.from({ length: 10 }, () => service.getRemoteDigest('alpine:latest')); +const concurrent = Number(process.env.DOCKER_TEST_CONCURRENCY ?? 5); +const promises = Array.from({ length: concurrent }, () => service.getRemoteDigest('alpine:latest'));api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (2)
44-53: Harden WWW-Authenticate parsing (avoid naive split on commas).Headers can vary in spacing/order; robust parsing avoids future edge cases.
- parseWWWAuth(wwwAuth: string): Partial<DockerWwwAuthParts> { - // www-authenticate: Bearer realm="...",service="...",scope="repository:repo/name:pull" - const parts: Partial<DockerWwwAuthParts> = {}; - const rawParts = wwwAuth.replace(/^Bearer\s+/i, '').split(',') || []; - rawParts.forEach((pair) => { - const [k, v] = pair.split('='); - parts[k.trim()] = v?.replace(/^"|"$/g, ''); - }); - return parts; - } + parseWWWAuth(wwwAuth: string): Partial<DockerWwwAuthParts> { + // Expected: Bearer realm="...", service="...", scope="repository:repo/name:pull" + const parts: Partial<DockerWwwAuthParts> = {}; + const m = wwwAuth.match(/^Bearer\s+(.*)$/i); + if (!m) return parts; + const kv = m[1]; + const regex = /(\w+)\s*=\s*"([^"]*)"/g; + let match: RegExpExecArray | null; + while ((match = regex.exec(kv)) !== null) { + const [, key, value] = match; + (parts as any)[key] = value; + } + return parts; + }
62-71: Avoid mixing responseType: 'json' with .json() convenience; use one.Using both is redundant. Since you’re calling .json(), drop responseType.
- const { token } = await got - .get(parts.realm, { + const { token } = await got + .get(parts.realm, { searchParams: { service: parts.service, scope: parts.scope }, username: basicCreds.username, password: basicCreds.password, timeout: { request: 15_000 }, - responseType: 'json', }) .json<{ token?: string }>();api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (2)
121-130: Consider returning explicit error states instead of falseThe method returns
falsewhen the container exists but could benefit from distinguishing between "container exists" and "error during inspection" for better debugging and error handling.Consider refactoring to return a more descriptive result type that distinguishes between different states, or at least add logging for debugging purposes:
async isRebuildReady(networkMode: string) { if (!networkMode || !networkMode.startsWith('container:')) return false; const target = networkMode.slice('container:'.length); try { await docker.getContainer(target).inspect(); return false; - } catch { + } catch (error) { + // Container doesn't exist or inspection failed return true; // unresolved target -> ':???' equivalent } }
132-139: Consider adding timeout for network operationsThe
isUpdateAvailablemethod performs concurrent network operations but doesn't implement a timeout mechanism. If the remote registry is slow or unresponsive, this could lead to long wait times.Consider implementing a timeout wrapper or using Promise.race with a timeout:
async isUpdateAvailable(imageRef: string, timeoutMs = 30000): Promise<boolean | null> { const timeoutPromise = new Promise<null>((resolve) => setTimeout(() => resolve(null), timeoutMs) ); const resultPromise = Promise.all([ this.getLocalDigest(imageRef), this.getRemoteDigest(imageRef), ]).then(([local, remote]) => { if (local && remote) return local !== remote; return null; }); return Promise.race([resultPromise, timeoutPromise]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
api/package.json(1 hunks)api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts(1 hunks)api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
🧠 Learnings (1)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
🧬 Code Graph Analysis (2)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (1)
Injectable(19-73)
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
Injectable(12-140)
🔇 Additional comments (7)
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (2)
3-9: ESM-friendly imports and provider wiring look correct.The .js import specifiers comply with our ESM rule, and both services are properly added to providers. No issues spotted here.
25-26: Confirm whether DockerAuthService/DockerManifestService need to be exported.If other modules resolve these services via Nest DI, you’ll need to export them. If they’re strictly internal to this module’s resolvers/services, current exports are fine.
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts (2)
6-12: Network client mocking approach is solid.Mocking got at the module level and resetting per test isolates HTTP effects well. This keeps unit tests fast while preserving behavior-focused assertions.
165-183: HEAD→GET fallback is correctly exercised.This ensures registries that disallow HEAD are still supported. Good coverage of this behavior.
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (3)
16-43: Well-implemented image reference parsing logicThe
parseImageRefmethod correctly handles various Docker image reference formats including:
- Default tag assignment when missing
- Docker Hub special cases and library namespace
- Registry detection logic
- Proper separation of registry, repository path, and tag
The implementation aligns well with Docker's conventions.
45-62: Robust manifest probing with fallback mechanismThe
headManifestmethod implements a good fallback strategy from HEAD to GET requests, which handles registries that don't support HEAD requests properly. The timeout configuration is appropriate.
5-6: Use .js extension in import specifiers for ESM compatibilityTypeScript source files must use import specifiers with .js extensions for ESM compatibility according to the coding guidelines.
Apply this diff to fix the import extensions:
-import { docker } from '@app/core/utils/index.js'; -import { DockerAuthService } from '@app/unraid-api/graph/resolvers/docker/docker-auth.service.js'; +import { docker } from '@app/core/utils/index.js'; +import { DockerAuthService } from '@app/unraid-api/graph/resolvers/docker/docker-auth.service.js';Wait, I see the imports already have .js extensions. Let me reconsider.
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
Outdated
Show resolved
Hide resolved
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
Outdated
Show resolved
Hide resolved
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts
Outdated
Show resolved
Hide resolved
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (2)
29-33: Clarify logger usage and include error context in the message.Currently logging the error object as the message and the string as the context inverts typical Logger usage, and loses stack/message context in the log line.
- this.logger.debug(error, `Failed to read Docker auth from '${configPath}'`); + const msg = `Failed to read Docker auth from '${configPath}': ${ + error instanceof Error ? error.message : String(error) + }`; + this.logger.debug(msg);
24-33: Prefer os.homedir() over HOME fallback and validate config shape.Using os.homedir() is more portable, and validating cfg.auths is safer than assuming it exists as an object.
-import { join } from 'path'; +import { join } from 'path'; +import { homedir } from 'os'; @@ - configPath ??= join(process.env.HOME || '/root', '.docker/config.json'); - const cfg = JSON.parse(await readFile(configPath, 'utf8')); - return cfg.auths || {}; + configPath ??= join(homedir(), '.docker/config.json'); + const cfg = JSON.parse(await readFile(configPath, 'utf8')); + return cfg && typeof cfg === 'object' && cfg.auths && typeof cfg.auths === 'object' + ? cfg.auths + : {};api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (7)
45-62: headManifest: propagate status consistently and consider HEAD-only preference.Current fallback to GET is fine. Consider explicitly checking for method-not-allowed (405) before falling back to GET to avoid hiding transient network issues on HEAD.
- try { - return await got.head(url, opts); - } catch { - // Some registries don’t allow HEAD; try GET to read headers - return await got.get(url, opts); - } + try { + return await got.head(url, opts); + } catch { + // Some registries don’t allow HEAD; try GET to read headers + const res = await got.get(url, opts); + return res; + }Optional: check for 405 specifically before GET.
107-119: Type the imageRef parameter in getLocalDigest.Missing type annotation reduces clarity; add string type. Also minor nit: prefer strict null return on empty digests.
- async getLocalDigest(imageRef) { + async getLocalDigest(imageRef: string): Promise<string | null> {
121-130: Type the networkMode parameter in isRebuildReady.Add types for better inference and call sites.
- async isRebuildReady(networkMode) { + async isRebuildReady(networkMode: string | null | undefined): Promise<boolean> {
8-11: Consider exporting ACCEPT_MANIFEST for reuse across tests/utilities.If other parts (tests/utilities) need consistent Accept headers, export this constant.
-const ACCEPT_MANIFEST = +export const ACCEPT_MANIFEST = 'application/vnd.docker.distribution.manifest.list.v2+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json';
132-139: Optional: return false when remote digest retrieval definitively fails.Right now null conflates “unknown” with “couldn’t fetch”. If a 404/401 occurs, you might treat as false to avoid prompting updates. If you intend tri-state, keep as is.
Would you like a tri-state enum (Unknown/NoUpdate/HasUpdate) to make downstream logic explicit?
80-100: Handle non-Bearer registries and missing tokens more defensively.Some registries return Basic without scope, or Bearer but without scope (then token endpoint expects repository query). Current logic requires scope; consider relaxing or constructing a default scope from repoPath.
If interested, I can provide a patch to derive scope "repository:${repoPath}:pull" when missing.
64-105: Token caching can significantly reduce registry load and speed up batch requests.Given you run 10–100 parallel requests in tests, caching tokens by key (realm+service+scope) for their TTL would reduce latency and rate-limit risk.
I can add a simple in-memory LRU or leverage cache-manager already in deps to store tokens until expiry. Want me to draft that?
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (1)
11-27: Export new services if they need to be injected outside this module.If other modules/resolvers depend on DockerManifestService or DockerAuthService, add them to exports. If intentionally private to this module, ignore.
-@Module({ - providers: [ +@Module({ + providers: [ // Services DockerService, DockerConfigService, DockerOrganizerService, DockerAuthService, DockerManifestService, // DockerEventService, // Resolvers DockerResolver, DockerMutationsResolver, ], - exports: [DockerService], + exports: [DockerService, DockerAuthService, DockerManifestService], })api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts (1)
125-161: High-volume real HTTP tests may be flaky or hit rate limits.100 live registry requests in CI can be brittle. Consider gating these under an env flag or reducing counts to keep CI stable.
I can add a small helper to skip these when CI=true or to use it.concurrent with a lower batch size.
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts (1)
13-22: Consider avoiding mocking simple dependencies.According to the coding guidelines for API tests, prefer not to mock simple dependencies. The
dockerutilities from@app/core/utils/index.jsappear to be simple helper functions that could potentially be used without mocking.Consider whether these utilities are complex enough to warrant mocking, or if they could be tested with real implementations to improve test authenticity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
api/package.json(1 hunks)api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts(1 hunks)api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (1)
Injectable(19-73)
🔇 Additional comments (8)
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts (2)
85-95: Mocking readDockerAuth here masks a bug in getRemoteDigest.Because getRemoteDigest didn’t await readDockerAuth, mocking it to resolve to {} didn’t affect behavior. After fixing the await, this test remains valid but now actually exercises the mock correctly.
Please re-run tests after the await fix to ensure no hidden assumptions remain.
1-13: Good use of Vitest and real integration boundaries.Services are instantiated directly without over-mocking, aligning with our testing guidelines.
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts (6)
1-11: Mock setup follows TypeScript/ESM best practices.The import structure and module mocking patterns are well-implemented, following the coding guidelines for TypeScript files with proper
.jsextensions for ESM compatibility.
29-44: Excellent mock management and service setup.The
beforeEachsetup properly clears mocks between tests and follows the guideline to usevi.clearAllMocks()for test isolation. The service instantiation with a mocked dependency is clean and well-structured.
46-145: Comprehensive test coverage for parseImageRef method.The test suite thoroughly covers various image reference parsing scenarios including:
- Default tags
- Explicit tags
- Docker Hub organization images
- Custom registries with/without ports
- Complex tag formats
- Localhost registries
- Deep nested repository paths
The test assertions are precise and cover edge cases well.
147-183: Well-structured headManifest tests with proper fallback logic.The tests correctly verify both the successful HEAD request scenario and the fallback-to-GET behavior when HEAD fails. The mock setup and assertions are appropriate for testing this resilience pattern.
185-327: Excellent coverage of getRemoteDigest authentication flows.The test suite comprehensively covers:
- Anonymous access for public images
- Array digest header handling
- Bearer token authentication flow
- Private registry authentication
- Authentication failure scenarios
- Missing digest handling
- Custom registry support
- Default tag behavior
- Error handling
The tests follow the coding guideline to focus on behavior rather than exact error messages, and properly use mocks as objects rather than overusing behavioral mocking.
256-273: Authentication failure test follows proper error testing guidelines.The test correctly focuses on the behavioral outcome (returning
null) rather than specific error message details, which aligns with the coding guidelines for testing behavior over implementation details.
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (4)
29-31: Fix Logger.debug misuse to include the intended messageLogger.debug(message, context?) expects the message first. Currently the error object is logged as the message and your text is treated as the context tag, losing the intended message.
Apply this diff:
- this.logger.debug(error, `Failed to read Docker auth from '${configPath}'`); + this.logger.debug( + `Failed to read Docker auth from '${configPath}': ${ + error instanceof Error ? error.message : String(error) + }` + );
44-51: Make WWW-Authenticate parsing more robustKey/value parsing by simple split('=') can misbehave for unexpected values. A regex-based parse improves resilience while keeping behavior unchanged for standard headers.
Apply this diff:
- parseWWWAuth(wwwAuth: string): Partial<DockerWwwAuthParts> { - // www-authenticate: Bearer realm="...",service="...",scope="repository:repo/name:pull" - const parts: Partial<DockerWwwAuthParts> = {}; - const rawParts = wwwAuth.replace(/^Bearer\s+/i, '').split(',') || []; - rawParts.forEach((pair) => { - const [k, v] = pair.split('='); - parts[k.trim()] = v?.replace(/^"|"$/g, ''); - }); - return parts; - } + parseWWWAuth(wwwAuth: string): Partial<DockerWwwAuthParts> { + // www-authenticate: Bearer realm="...",service="...",scope="repository:repo/name:pull" + const parts: Partial<DockerWwwAuthParts> = {}; + const header = wwwAuth.replace(/^Bearer\s+/i, ''); + const kv = header.matchAll(/(\w+)=(".*?"|[^,]+)/g); + for (const [, k, raw] of kv) { + parts[k.trim() as keyof DockerWwwAuthParts] = raw.replace(/^"|"$/g, ''); + } + return parts; + }
22-23: Remove the empty constructorThe empty constructor adds noise without value.
Apply this diff:
- constructor() {} -
24-33: Consider adding an explicit return type to readDockerAuthExplicit typing improves clarity for consumers and test doubles.
Apply this diff:
- async readDockerAuth(configPath?: string) { + async readDockerAuth( + configPath?: string + ): Promise<Record<string, { auth?: string }>> {api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts (1)
26-44: Tighten typing of mockDockerAuthService for better IntelliSense and safetyTyping as Partial improves refactor safety without increasing complexity.
Apply this diff:
- let mockDockerAuthService: any; + let mockDockerAuthService: Partial<DockerAuthService>; @@ - mockDockerAuthService = { + mockDockerAuthService = { readDockerAuth: vi.fn(), decodeAuth: vi.fn(), getBearerToken: vi.fn(), - }; + }; @@ - service = new DockerManifestService(mockDockerAuthService); + service = new DockerManifestService(mockDockerAuthService as DockerAuthService);api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (3)
107-119: Add explicit types to getLocalDigest for clarity and consistencyPublic method should declare parameter and return types.
Apply this diff:
- async getLocalDigest(imageRef) { + async getLocalDigest(imageRef: string): Promise<string | null> {
121-130: Type the public API of isRebuildReadyAdd parameter and return types to avoid implicit any and to document the contract.
Apply this diff:
- async isRebuildReady(networkMode) { + async isRebuildReady(networkMode?: string): Promise<boolean> {
45-62: Optional: surface response type in headManifest return for easier consumptionCurrently inferred, but being explicit helps maintainers. If you keep using got’s default instance, consider annotating the return type to
Promise<Response<string>>(or the correct got Response type in your version).If you want to keep code minimal, you can skip; tests already validate behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
api/package.json(1 hunks)api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts(1 hunks)api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.tsapi/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
🧠 Learnings (1)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (1)
Injectable(19-73)
🔇 Additional comments (6)
api/package.json (1)
104-104: fast-xml-parser@5.2.5 Unaffected by Known GHSA AdvisoriesAll reported vulnerabilities on GitHub (GHSA-mpg4-rc92-vx8v, GHSA-6w63-h3fj-q4vw, GHSA-x3cc-x39p-42qx, GHSA-gpv5-7x3g-ghjv) target v4.x ranges (patched by 4.4.1 and earlier). Since you’re using 5.2.5, there are no known security issues for this release. Safe to merge.
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (1)
17-18: Providers registration looks good and ESM import specifiers are compliantThe new services are correctly added to providers and use .js extensions per ESM guideline.
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts (1)
29-39: Broaden acceptance range only if requiredThe 200–499 range hides potential auth issues for this specific flow. If the objective is to assert “no crash,” fine; otherwise consider tightening expectations once the flow stabilizes.
Would you like to narrow the expected codes (e.g., 200/401/403) to increase signal?
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.test.ts (3)
6-12: Mock shape for got is appropriate and isolatedTop-level vi.mock with a factory keeps mocks predictable and avoids hoisting pitfalls. LGTM.
102-111: Verify expected repoPath for docker.io explicit registryThis asserts repoPath 'docker.io/nginx'. Many implementations normalize docker.io to library/ when no org is present. Ensure this aligns with your intended behavior across the app.
If normalization to library/nginx is desired, adjust either the service or the test to keep consistency with other docker.io cases in the suite.
212-238: Good coverage of Bearer auth retry pathUnauthorized -> token -> authorized flow is well represented, focusing on behavior rather than messages. Nice.
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
Outdated
Show resolved
Hide resolved
api/src/__test__/unraid-api/graph/resolvers/docker/docker-manifest.service.integration.test.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (5)
5-9: Export the public return type for reuse.
ExplicitStatusItemappears in a public method signature; exporting it improves reusability and clarity for consumers.-type ExplicitStatusItem = { +export type ExplicitStatusItem = { name: string; updateStatus: 'up to date' | 'update available' | 'rebuild ready' | 'unknown'; };
1-1: Don't silently swallow phpLoader errors; log at debug for observability.Keeping the method non-throwing is fine, but record failures so we can diagnose offline/misconfig states.
-import { Injectable } from '@nestjs/common'; +import { Injectable, Logger } from '@nestjs/common'; @@ export class DockerPhpService { - constructor() {} + private readonly logger = new Logger(DockerPhpService.name); + constructor() {} @@ - } catch { - // ignore; offline may keep remote as 'undef' - } + } catch (err) { + // allow offline; keep remote as 'undef', but record for observability + this.logger.debug( + `refreshDigests failed for ${dockerUpdatePath}: ${ + err instanceof Error ? err.message : String(err) + }` + ); + }Also applies to: 12-13, 19-26
40-59: Simplify status mapping with a typed const record.Removes overload boilerplate, keeps literal types, and centralizes the mapping.
- private updateStatusToString(updateStatus: 0): 'up to date'; - private updateStatusToString(updateStatus: 1): 'update available'; - private updateStatusToString(updateStatus: 2): 'rebuild ready'; - private updateStatusToString(updateStatus: 3): 'unknown'; - // prettier-ignore - private updateStatusToString(updateStatus: StatusItem['updateStatus']): ExplicitStatusItem['updateStatus']; - private updateStatusToString( - updateStatus: StatusItem['updateStatus'] - ): ExplicitStatusItem['updateStatus'] { - switch (updateStatus) { - case 0: - return 'up to date'; - case 1: - return 'update available'; - case 2: - return 'rebuild ready'; - default: - return 'unknown'; - } - } + private static readonly STATUS_MAP = { + 0: 'up to date', + 1: 'update available', + 2: 'rebuild ready', + 3: 'unknown', + } as const; + + private updateStatusToString( + updateStatus: StatusItem['updateStatus'] + ): ExplicitStatusItem['updateStatus'] { + return DockerPhpService.STATUS_MAP[updateStatus] ?? 'unknown'; + }
66-67: Remove redundant method: 'GET' (phpLoader defaults to GET).This is a no-op parameter and can be dropped.
- file: dockerContainersPath, - method: 'GET', + file: dockerContainersPath,
61-71: Clarify desired behavior when PHP endpoints are unavailable (offline).Currently this method throws (propagates PhpError). If the UI should degrade gracefully (like refreshDigests), catch and return an empty list with a debug log. Otherwise, keep throwing. Confirm expectation.
Proposed graceful fallback:
async getContainerUpdateStatuses( dockerContainersPath = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php' ): Promise<ExplicitStatusItem[]> { - const stdout = await phpLoader({ - file: dockerContainersPath, - method: 'GET', - }); - const parts = stdout.split('\0'); // [html, "docker.push(...)", busyFlag] - const js = parts[1] || ''; - return this.parseStatusesFromDockerPush(js); + try { + const stdout = await phpLoader({ + file: dockerContainersPath, + }); + const parts = stdout.split('\0'); // [html, "docker.push(...)", busyFlag] + const js = parts[1] || ''; + return this.parseStatusesFromDockerPush(js); + } catch (err) { + // Align behavior with refreshDigests: fail open but record for observability + this.logger?.debug?.( + `getContainerUpdateStatuses failed for ${dockerContainersPath}: ${ + err instanceof Error ? err.message : String(err) + }` + ); + return []; + } }If you'd prefer to propagate errors, I can adjust the message and rethrow with added context instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
api/src/core/utils/plugins/php-loader.ts (1)
phpLoader(38-66)
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
3-3: ESM import specifier suffix is correct (.js).Good compliance with our ESM import rule.
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
Outdated
Show resolved
Hide resolved
pujitm
force-pushed
the
feat/docker-template
branch
from
48252f9 to
1e44af4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
59-66: Resolver DI wiring updated to include DockerPhpService — add a module-level override for stabilityGood to see DockerPhpService mocked for the Resolver test. To keep the "should compile the module" test resilient as DockerModule gains more providers, also override DockerPhpService there to avoid instantiating real dependencies.
Apply this diff within the first test’s setup chain:
}) .overrideProvider(DockerService) .useValue({ getDockerClient: vi.fn() }) + .overrideProvider(DockerPhpService) + .useValue({ + getContainerUpdateStatuses: vi.fn(), + refreshDigests: vi.fn(), + }) .overrideProvider(DockerConfigService) .useValue({ getConfig: vi.fn() }) .compile();api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
142-150: Add optional skipCache arg for parity with other fieldscontainers() and networks() both accept skipCache; consider exposing the same here for consistency and control over freshness, if DockerPhpService supports it.
If the service already supports options, update the resolver like this:
- @ResolveField(() => GraphQLJSON) - public async containerUpdateStatuses() { - return this.dockerPhpService.getContainerUpdateStatuses(); - } + @ResolveField(() => GraphQLJSON) + public async containerUpdateStatuses( + @Args('skipCache', { defaultValue: false, type: () => Boolean }) skipCache: boolean + ) { + return this.dockerPhpService.getContainerUpdateStatuses({ skipCache }); + }Please confirm DockerPhpService.getContainerUpdateStatuses accepts an options bag; if not, I can generate the corresponding service change as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/docker/docker.module.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
🧠 Learnings (4)
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
📚 Learning: 2025-08-11T15:07:39.222Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-08-11T15:07:39.222Z
Learning: Applies to api/**/*.{test,spec}.{js,jsx,ts,tsx} : Use Vitest for tests in the api; do not use Jest
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
🔇 Additional comments (4)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
9-9: ESM import extension is correctUsing .js in TS import specifiers aligns with our ESM guideline.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (1)
7-7: ESM import extension is correctUsing .js in TS import specifiers aligns with our ESM guideline.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
12-12: New dependency injection is appropriateInjecting DockerPhpService directly into the resolver keeps concerns separated (resolver delegates computation). Good.
9-9: JSON scalar source consistency confirmed
AllGraphQLJSONimports across the repository reference graphql-scalars; there are no imports from graphql-type-json. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/vite.config.ts (2)
73-73: Remove redundant commentThe plugin name already communicates the intent. Per repo guideline, avoid obvious comments.
- // Copy PHP files to assets directory
80-88: Optional: generalize to support multiple PHP assets via globIf you anticipate adding more PHP helpers, consider discovering files dynamically (e.g., src/**/*.php) with fast-glob to avoid hardcoding paths.
Example (not a diff, since it adds a new dependency):
const fg = await import('fast-glob'); const phpFiles: string[] = await fg.default('src/**/*.php', { dot: false });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/vite.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/vite.config.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/vite.config.ts
api/vite.config.ts
Outdated
| const { readFileSync } = await import('fs'); | ||
| const { join, basename } = await import('path'); | ||
|
|
||
| const phpFiles = ['src/core/utils/plugins/wrapper.php']; | ||
| phpFiles.forEach((file) => { | ||
| const content = readFileSync(file); | ||
| this.emitFile({ | ||
| type: 'asset', | ||
| fileName: join('assets', basename(file)), | ||
| source: content, | ||
| }); | ||
| }); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix cross-platform asset paths and prefer node: protocol; add missing-file guard
Rollup expects POSIX-style paths in emitted asset fileNames. Using path.join on Windows will produce backslashes and can create malformed asset paths. Also, prefer node:fs/node:path for clarity and to avoid unintended bundling. While here, add a guard to warn if the source file is missing instead of throwing.
Apply this diff:
- const { readFileSync } = await import('fs');
- const { join, basename } = await import('path');
+ const { readFileSync, existsSync } = await import('node:fs');
+ const { posix: pathPosix, basename } = await import('node:path');
@@
- const phpFiles = ['src/core/utils/plugins/wrapper.php'];
- phpFiles.forEach((file) => {
- const content = readFileSync(file);
- this.emitFile({
- type: 'asset',
- fileName: join('assets', basename(file)),
- source: content,
- });
- });
+ const phpFiles = ['src/core/utils/plugins/wrapper.php'];
+ for (const file of phpFiles) {
+ if (!existsSync(file)) {
+ this.warn(`[copy-php-files] missing file: ${file}`);
+ continue;
+ }
+ const content = readFileSync(file);
+ this.emitFile({
+ type: 'asset',
+ fileName: pathPosix.join('assets', basename(file)),
+ source: content,
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { readFileSync } = await import('fs'); | |
| const { join, basename } = await import('path'); | |
| const phpFiles = ['src/core/utils/plugins/wrapper.php']; | |
| phpFiles.forEach((file) => { | |
| const content = readFileSync(file); | |
| this.emitFile({ | |
| type: 'asset', | |
| fileName: join('assets', basename(file)), | |
| source: content, | |
| }); | |
| }); | |
| }, | |
| }, | |
| const { readFileSync, existsSync } = await import('node:fs'); | |
| const { posix: pathPosix, basename } = await import('node:path'); | |
| const phpFiles = ['src/core/utils/plugins/wrapper.php']; | |
| for (const file of phpFiles) { | |
| if (!existsSync(file)) { | |
| this.warn(`[copy-php-files] missing file: ${file}`); | |
| continue; | |
| } | |
| const content = readFileSync(file); | |
| this.emitFile({ | |
| type: 'asset', | |
| fileName: pathPosix.join('assets', basename(file)), | |
| source: content, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In api/vite.config.ts around lines 77 to 90, replace the dynamic imports with
node: protocol imports (node:fs, node:path), add an existence guard using
fs.existsSync or try/catch to warn via this.warn and skip missing files instead
of throwing, and ensure emitted asset fileName uses POSIX-style paths by using
path.posix.join('assets', basename(file)) (keep basename from path but use
path.posix.join for the join operation); read the file only when it exists and
pass its content to this.emitFile.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (1)
33-35: Avoid N+1 remote lookups: memoize per-request or use DataLoaderResolving isUpdateAvailable for a list of containers can result in many registry calls. Consider a request-scoped cache/DataLoader in DockerManifestService keyed by imageRef to dedupe.
I can draft a DataLoader-based pattern for NestJS if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/generated-schema.graphql(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.ts
🔇 Additional comments (3)
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (1)
23-26: Resolver wiring looks correctConstructor DI for DockerManifestService and class-level @resolver(() => DockerContainer) are appropriate for adding computed fields.
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (2)
3-9: ESM imports and service additions look goodLocal imports use .js extensions per repo ESM guideline, and new services are correctly imported for DI.
19-28: Providers updated; resolvers retained and containerUpdateStatuses verified
- The summary mistakenly claimed
DockerResolverandDockerMutationsResolverwere removed; they remain correctly registered alongsideDockerContainerResolverindocker.module.ts.- In
docker.resolver.ts,containerUpdateStatusesis protected byand@UsePermissions({ action: AuthActionVerb.READ, resource: Resource.DOCKER, possession: AuthPossession.ANY, }) @ResolveField(() => GraphQLJSON) public async containerUpdateStatuses() { … }DockerPhpService.getContainerUpdateStatuses()always returns an array (empty on failure), satisfying the non-nullJSON!schema.No further changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (2)
38-46: Honor nullable field contract: return null on error instead of throwing AppErrorPer project pattern, handle errors at the service level and keep resolvers fail-safe. This field is nullable; don’t surface errors to clients.
- @ResolveField(() => Boolean, { nullable: true }) - public async isUpdateAvailable(@Parent() container: DockerContainer) { - try { - return await this.dockerManifestService.isUpdateAvailableCached(container.image); - } catch (error) { - this.logger.error(error); - throw new AppError('Failed to read cached update status. See graphql-api.log for details.'); - } - } + @ResolveField(() => Boolean, { + nullable: true, + description: 'True if a newer remote image digest exists; null when unknown or on error.', + }) + public async isUpdateAvailable(@Parent() container: DockerContainer): Promise<boolean | null> { + try { + return await this.dockerManifestService.isUpdateAvailableCached(container.image); + } catch (error) { + this.logger.error(error); + return null; + } + }
53-56: Mirror fail-safe pattern and add explicit return typeKeep resolver behavior consistent; return null on errors and declare the explicit return type.
- @ResolveField(() => Boolean, { nullable: true }) - public async isRebuildReady(@Parent() container: DockerContainer) { - return this.dockerManifestService.isRebuildReady(container.hostConfig?.networkMode); - } + @ResolveField(() => Boolean, { + nullable: true, + description: 'True if rebuild is needed (e.g., target container not found); null on error.', + }) + public async isRebuildReady(@Parent() container: DockerContainer): Promise<boolean | null> { + try { + return await this.dockerManifestService.isRebuildReady(container.hostConfig?.networkMode); + } catch (error) { + this.logger.error(error); + return null; + } + }api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (3)
75-116: Bug: readDockerAuth is async and not awaited; add types and robust auth key matchingWithout await, dockerAuths is a Promise, breaking credentials lookup. Also add explicit types and resilient key matching for private registries.
- async getRemoteDigest(imageRef) { + async getRemoteDigest(imageRef: string): Promise<string | null> { const { registryBaseURL, repoPath, tag, authConfigKey } = this.parseImageRef(imageRef); const manifestURL = `${registryBaseURL}/v2/${repoPath}/manifests/${tag}`; - const dockerAuths = this.dockerAuthService.readDockerAuth(); - const authEntry = dockerAuths[authConfigKey]; - const basicCreds = authEntry?.auth + const dockerAuths = await this.dockerAuthService.readDockerAuth(); + const registryHost = new URL(registryBaseURL).host; + const authEntry = + dockerAuths[authConfigKey] ?? + dockerAuths[registryHost] ?? + dockerAuths[`https://${registryHost}`] ?? + dockerAuths[`http://${registryHost}`] ?? + Object.entries(dockerAuths).find(([k]) => k.includes(registryHost))?.[1]; + const basicCreds = (authEntry as any)?.auth ? this.dockerAuthService.decodeAuth(authEntry.auth) : { username: '', password: '' }; // 1) Probe without auth to learn challenge let resp = await this.headManifest(manifestURL, { Accept: ACCEPT_MANIFEST }); const digestHeaderRaw = resp.headers?.['docker-content-digest']; const digestHeader = Array.isArray(digestHeaderRaw) ? digestHeaderRaw[0] : digestHeaderRaw; if (resp.statusCode >= 200 && resp.statusCode < 300 && digestHeader) return digestHeader.trim(); const wwwAuth = (resp.headers?.['www-authenticate'] || '').toString(); if (/Bearer/i.test(wwwAuth)) { const token = await this.dockerAuthService.getBearerToken(wwwAuth, basicCreds); if (!token) return null; // 2) Repeat with Bearer resp = await this.headManifest( manifestURL, { Accept: ACCEPT_MANIFEST }, { Authorization: `Bearer ${token}` } ); } else if (/Basic/i.test(wwwAuth) && basicCreds.username && basicCreds.password) { // 2) Repeat with Basic const basic = 'Basic ' + Buffer.from(`${basicCreds.username}:${basicCreds.password}`).toString('base64'); resp = await this.headManifest( manifestURL, { Accept: ACCEPT_MANIFEST }, { Authorization: basic } ); } const digestRaw = resp.headers?.['docker-content-digest']; const digest = Array.isArray(digestRaw) ? digestRaw[0] : digestRaw; return digest ? digest.trim() : null; }
10-12: Accept header missing OCI manifest type; digest resolution may fail against OCI-only registriesSome registries require application/vnd.oci.image.manifest.v1+json to return digest headers.
-const ACCEPT_MANIFEST = - 'application/vnd.docker.distribution.manifest.list.v2+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json'; +const ACCEPT_MANIFEST = + 'application/vnd.docker.distribution.manifest.list.v2+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json,application/vnd.oci.image.manifest.v1+json';
27-54: Fix image reference parsing for explicit docker.io, localhost, and digest refs (@)Current logic misclassifies localhost/docker.io and breaks on digest refs by force-appending :latest and splitting on colon.
- parseImageRef(imageRef: string) { - // Normalize to repo:tag and extract registry/repo/name/tag - let ref = imageRef; - if (!ref.includes(':')) ref += ':latest'; - - // Registry present? - const firstSlash = ref.indexOf('/'); - const maybeRegistry = firstSlash > -1 ? ref.slice(0, firstSlash) : ''; - const hasDotOrColon = maybeRegistry.includes('.') || maybeRegistry.includes(':'); - const isDockerHub = !hasDotOrColon || maybeRegistry === 'docker.io'; - - const registry = isDockerHub ? 'registry-1.docker.io' : maybeRegistry; - const rest = isDockerHub ? ref : ref.slice(maybeRegistry.length + 1); - - const lastColon = rest.lastIndexOf(':'); - const namePart = rest.slice(0, lastColon); - const tag = rest.slice(lastColon + 1); - - // Ensure docker hub library namespace - const repoPath = isDockerHub && !namePart.includes('/') ? `library/${namePart}` : namePart; - - return { - registryBaseURL: `https://${registry}`, - authConfigKey: isDockerHub ? 'https://index.docker.io/v1/' : maybeRegistry, - repoPath, // e.g. library/nginx or org/image - tag, - }; - } + parseImageRef(imageRef: string) { + // Normalize and extract registry/repo/name and tag or digest + const ref = imageRef.trim(); + + // Registry present? + const firstSlash = ref.indexOf('/'); + const maybeRegistry = firstSlash > -1 ? ref.slice(0, firstSlash) : ''; + const hasRegistryPrefix = + firstSlash > -1 && + (maybeRegistry.includes('.') || + maybeRegistry.includes(':') || + maybeRegistry === 'localhost' || + maybeRegistry === 'docker.io'); + const registryHost = hasRegistryPrefix + ? (maybeRegistry === 'docker.io' ? 'registry-1.docker.io' : maybeRegistry) + : 'registry-1.docker.io'; + const rest = hasRegistryPrefix ? ref.slice(maybeRegistry.length + 1) : ref; + + // Split name vs tag/digest + const atPos = rest.indexOf('@'); + let namePart = rest; + let tag = 'latest'; + if (atPos > -1) { + namePart = rest.slice(0, atPos); + tag = rest.slice(atPos + 1); // digest (e.g., sha256:...) + } else { + const lastColon = rest.lastIndexOf(':'); + if (lastColon > -1) { + namePart = rest.slice(0, lastColon); + tag = rest.slice(lastColon + 1); + } + } + + const isDockerHub = registryHost === 'registry-1.docker.io'; + const repoPath = isDockerHub && !namePart.includes('/') ? `library/${namePart}` : namePart; + + return { + registryBaseURL: `https://${registryHost}`, + authConfigKey: isDockerHub ? 'https://index.docker.io/v1/' : registryHost, + repoPath, // e.g. library/nginx or org/image + tag, + }; + }
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (1)
63-66: Add explicit return type to mutation for claritySmall readability/contract improvement.
- @Mutation(() => Boolean) - public async refreshDigests() { - return this.dockerPhpService.refreshDigests(); - } + @Mutation(() => Boolean) + public async refreshDigests(): Promise<boolean> { + return this.dockerPhpService.refreshDigests(); + }api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
118-130: Add explicit return type for getLocalDigestClarifies contract and improves IDE support.
- async getLocalDigest(imageRef: string) { + async getLocalDigest(imageRef: string): Promise<string | null> { try { const data = await docker.getImage(imageRef).inspect(); const digests = data.RepoDigests || []; if (digests.length === 0) return null; // Prefer a digest matching this repo if present; else first const pick = digests.find((d) => d.startsWith(imageRef.split(':')[0] + '@')) || digests[0]; const [, shaDigestString] = pick.split('@'); return shaDigestString ?? null; } catch { return null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
🧠 Learnings (1)
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (1)
api/src/core/errors/app-error.ts (1)
AppError(4-37)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/unraid-shared/src/util/__tests__/processing.test.ts
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (2)
33-41: Handle digest-only refs and unknown status; type the return.
- Don’t append :latest when the ref uses a digest (@).
- Return null when status is unknown (null).
- Add an explicit return type.
- async isUpdateAvailableCached(imageRef: string, cacheData?: Record<string, CachedStatusEntry>) { - let taggedRef = imageRef; - if (!taggedRef.includes(':')) taggedRef += ':latest'; - - cacheData ??= await this.readCachedUpdateStatus(); - const containerData = cacheData[taggedRef]; - if (!containerData) return null; - return containerData.status?.toLowerCase() === 'true'; - } + async isUpdateAvailableCached( + imageRef: string, + cacheData?: Record<string, CachedStatusEntry> + ): Promise<boolean | null> { + let taggedRef = imageRef.trim(); + // If neither a tag (:) nor a digest (@) is present, default to :latest + if (!/[:@]/.test(taggedRef)) taggedRef += ':latest'; + + cacheData ??= await this.readCachedUpdateStatus(); + const containerData = cacheData[taggedRef]; + if (!containerData) return null; + if (containerData.status == null) return null; + return String(containerData.status).toLowerCase() === 'true'; + }
27-31: Guard cache reads; return an empty object on failure.The method throws if the file is missing or JSON is invalid. Prefer a resilient read with logging.
- async readCachedUpdateStatus(cacheFile = '/var/lib/docker/unraid-update-status.json') { - const cache = await readFile(cacheFile, 'utf8'); - const cacheData = JSON.parse(cache); - return cacheData as Record<string, CachedStatusEntry>; - } + async readCachedUpdateStatus( + cacheFile = '/var/lib/docker/unraid-update-status.json' + ): Promise<Record<string, CachedStatusEntry>> { + try { + const cache = await readFile(cacheFile, 'utf8'); + const parsed = JSON.parse(cache); + return (parsed && typeof parsed === 'object' ? parsed : {}) as Record<string, CachedStatusEntry>; + } catch (error) { + this.logger.warn(`Failed to read update status cache '${cacheFile}': ${(error as Error).message}`); + return {}; + } + }api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
61-69: Make docker.push parser resilient to property order and quote styles.Support both single/double quotes and property reordering.
- const re = /docker\.push\(\{[^}]*name:'([^']+)'[^}]*update:(\d)[^}]*\}\);/g; - for (const m of js.matchAll(re)) { - const name = m[1]; - const updateStatus = Number(m[2]) as StatusItem['updateStatus']; + const re = + /docker\.push\(\{(?=[^}]*\bname:\s*(['"])([^'"]+)\1)(?=[^}]*\bupdate:\s*(\d))[^}]*\}\);/g; + for (const m of js.matchAll(re)) { + const name = m[2]; + const updateStatus = Number(m[3]) as StatusItem['updateStatus']; items.push({ name, updateStatus: this.updateStatusToString(updateStatus) }); }
🧹 Nitpick comments (8)
packages/unraid-shared/src/util/processing.ts (1)
45-66: Single-flight logic is solid; consider reentrancy-safe assignment.Current dedupe works as intended. A small refinement avoids a narrow reentrancy window if an operation synchronously calls back into do() before currentOperation is set.
Apply this diff to set currentOperation before invoking the operation and make the cleanup robust:
- const op = (operation || this.defaultOperation) as AsyncOperation<U | T>; - - const promise = this.executeOperation(op).finally(() => { - if (this.currentOperation === promise) { - this.currentOperation = null; - } - }); - - this.currentOperation = promise; - return promise; + const op = (operation || this.defaultOperation) as AsyncOperation<U | T>; + let promise!: Promise<U | T>; + this.currentOperation = promise = (async () => { + try { + return await this.executeOperation(op); + } finally { + if (this.currentOperation === promise) { + this.currentOperation = null; + } + } + })(); + return promise;packages/unraid-shared/src/util/__tests__/processing.test.ts (4)
61-63: Avoid asserting the exact error instance in tests.Per guidelines, prefer .rejects.toThrow() without arguments to reduce brittleness.
Apply this diff:
- await expect(mutex.do()).rejects.toThrow(error); + await expect(mutex.do()).rejects.toThrow();
126-128: Use generic throw assertion for async failures.Avoid checking the specific Error object.
- await expect(mutex.do(failingOp)).rejects.toThrow(error); + await expect(mutex.do(failingOp)).rejects.toThrow();
135-139: Don’t assert exact error messages.Asserting on exact strings is brittle. Use a generic throw assertion.
- await expect(mutex.do()).rejects.toThrow('No operation provided and no default operation set'); + await expect(mutex.do()).rejects.toThrow();
283-293: Remove unused innerOp variable.innerOp isn’t used and can be dropped.
- const innerOp = vi.fn().mockResolvedValue('inner'); const outerOp = vi.fn().mockImplementation(async () => { return 'outer'; });api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
10-11: Include OCI single-manifest media type and consider removing unused constant.Add application/vnd.oci.image.manifest.v1+json for better registry compatibility. Also, ACCEPT_MANIFEST isn’t used—either wire it into requests or drop it.
-const ACCEPT_MANIFEST = - 'application/vnd.docker.distribution.manifest.list.v2+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json'; +const ACCEPT_MANIFEST = + 'application/vnd.docker.distribution.manifest.list.v2+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json,application/vnd.oci.image.manifest.v1+json';api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (2)
21-24: Tighten typing on the mutex for stronger safety.The default operation returns boolean; parameterize AsyncMutex accordingly.
- private readonly refreshDigestsMutex = new AsyncMutex(() => { + private readonly refreshDigestsMutex = new AsyncMutex<boolean>(() => { return this.refreshDigestsViaPhp(); });
31-35: Add explicit return type for refreshDigests.Clarifies API and helps callers.
- async refreshDigests(mutex = this.refreshDigestsMutex, dockerUpdatePath?: string) { + async refreshDigests(mutex = this.refreshDigestsMutex, dockerUpdatePath?: string): Promise<boolean> { return mutex.do(() => { return this.refreshDigestsViaPhp(dockerUpdatePath); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
api/generated-schema.graphql(3 hunks)api/src/unraid-api/graph/resolvers/docker/container-status.job.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)packages/unraid-shared/src/util/__tests__/processing.test.ts(1 hunks)packages/unraid-shared/src/util/processing.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/generated-schema.graphql
- api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
packages/unraid-shared/src/util/__tests__/processing.test.tsapi/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tspackages/unraid-shared/src/util/processing.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
packages/unraid-shared/src/util/__tests__/processing.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
packages/unraid-shared/src/util/__tests__/processing.test.tsapi/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tspackages/unraid-shared/src/util/processing.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
packages/unraid-shared/src/util/__tests__/processing.test.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
🧠 Learnings (2)
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
📚 Learning: 2024-12-11T20:39:02.511Z
Learnt from: pujitm
PR: unraid/api#978
File: web/helpers/apollo-cache/index.ts:134-139
Timestamp: 2024-12-11T20:39:02.511Z
Learning: In `web/helpers/apollo-cache/index.ts`, the `cache.readQuery` method returns `null` when it fails to read from the cache, rather than throwing an exception.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
🧬 Code Graph Analysis (4)
packages/unraid-shared/src/util/__tests__/processing.test.ts (1)
packages/unraid-shared/src/util/processing.ts (1)
AsyncMutex(37-71)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
Injectable(13-104)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (3)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
Injectable(6-22)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
Injectable(13-104)api/src/unraid-api/graph/resolvers/docker/docker-auth.service.ts (1)
Injectable(19-73)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (4)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
Injectable(6-22)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
Injectable(22-53)packages/unraid-shared/src/util/processing.ts (1)
AsyncMutex(37-71)api/src/core/utils/plugins/php-loader.ts (1)
phpLoader(38-66)
🔇 Additional comments (3)
packages/unraid-shared/src/util/processing.ts (1)
35-43: Type alias and class API look good.The AsyncOperation alias and the overloaded do() signatures are clear and ergonomic. No issues.
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
13-20: Verify cache entry shape matches the producer.CachedStatusEntry uses keys local and remote; ensure producers and consumers use the same names. If the producer writes localDigest/remoteDigest, this will misalign.
Would you like me to scan the repo for the writer of /var/lib/docker/unraid-update-status.json to confirm the exact keys and align types accordingly?
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
10-21: Job wiring looks good; mutex upstream prevents overlap.Using DockerPhpService with its AsyncMutex ensures single-flight refreshes even if the cron and startup timeout fire close together.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
60-67: Harden docker.push parser: support any property order and single/double quotes.Current regex assumes property order and single quotes; it will miss valid variants and silently drop entries.
Apply:
- const re = /docker\.push\(\{[^}]*name:'([^']+)'[^}]*update:(\d)[^}]*\}\);/g; - for (const m of js.matchAll(re)) { - const name = m[1]; - const updateStatus = Number(m[2]) as StatusItem['updateStatus']; + const re = + /docker\.push\(\{(?=[^}]*\bname:\s*(['"])([^'"]+)\1)(?=[^}]*\bupdate:\s*(\d))[^}]*\}\);/g; + for (const m of js.matchAll(re)) { + const name = m[2]; + const updateStatus = Number(m[3]) as StatusItem['updateStatus']; items.push({ name, updateStatus: this.updateStatusToString(updateStatus) }); }Would you like me to add unit tests for parseStatusesFromDockerPush covering single/double quotes and swapped property order?
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (3)
2-24: Remove unused imports to satisfy code scanning and keep the file lean.Matches prior code-scanning warnings (Args, Query, GraphQLJSON, and several Docker-related imports are unused).
Apply:
-import { Args, Mutation, Parent, Query, ResolveField, Resolver } from '@nestjs/graphql'; +import { Mutation, Parent, ResolveField, Resolver } from '@nestjs/graphql'; @@ -import { GraphQLJSON } from 'graphql-scalars'; - -import { AppError } from '@app/core/errors/app-error.js'; import { DockerManifestService } from '@app/unraid-api/graph/resolvers/docker/docker-manifest.service.js'; -import { DockerOrganizerService } from '@app/unraid-api/graph/resolvers/docker/docker-organizer.service.js'; -import { DockerPhpService } from '@app/unraid-api/graph/resolvers/docker/docker-php.service.js'; -import { - Docker, - DockerContainer, - DockerNetwork, -} from '@app/unraid-api/graph/resolvers/docker/docker.model.js'; -import { DockerService } from '@app/unraid-api/graph/resolvers/docker/docker.service.js'; -import { DEFAULT_ORGANIZER_ROOT_ID } from '@app/unraid-api/organizer/organizer.js'; -import { OrganizerV1, ResolvedOrganizerV1 } from '@app/unraid-api/organizer/organizer.model.js'; +import { DockerContainer } from '@app/unraid-api/graph/resolvers/docker/docker.model.js';
35-43: Honor nullable field contract: return null on error and add explicit return type.The field is declared nullable, but currently throws. Prefer returning null to avoid surfacing resolver errors to clients. This also aligns with past guidance for this repo to handle errors in services rather than resolvers.
Apply:
- @ResolveField(() => Boolean, { nullable: true }) - public async isUpdateAvailable(@Parent() container: DockerContainer) { - try { - return await this.dockerManifestService.isUpdateAvailableCached(container.image); - } catch (error) { - this.logger.error(error); - throw new AppError('Failed to read cached update status. See graphql-api.log for details.'); - } - } + @ResolveField(() => Boolean, { + nullable: true, + description: 'True if a newer remote image digest exists; null when unable to determine.', + }) + public async isUpdateAvailable(@Parent() container: DockerContainer): Promise<boolean | null> { + try { + return await this.dockerManifestService.isUpdateAvailableCached(container.image); + } catch { + return null; + } + }To confirm there are no remaining unused imports after this change, run:
#!/bin/bash rg -nP --type=ts '^\s*import\s+.*\sfrom\s' api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts -n -C0
50-53: Mirror fail-safe pattern and add explicit return type.Optional: add description and return null on unexpected errors to stay consistent with isUpdateAvailable.
- @ResolveField(() => Boolean, { nullable: true }) - public async isRebuildReady(@Parent() container: DockerContainer) { - return this.dockerManifestService.isRebuildReady(container.hostConfig?.networkMode); - } + @ResolveField(() => Boolean, { + nullable: true, + description: 'True if rebuild is needed (e.g., unresolved container:* network target); null when unknown.', + }) + public async isRebuildReady(@Parent() container: DockerContainer): Promise<boolean | null> { + try { + return await this.dockerManifestService.isRebuildReady(container.hostConfig?.networkMode); + } catch { + return null; + } + }api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
29-37: Handle digest refs (@), unknown status, and add explicit return type.Current logic forces :latest and conflates unknown with false. Support digest-only refs and return null when cache has no status.
- async isUpdateAvailableCached(imageRef: string, cacheData?: Record<string, CachedStatusEntry>) { - let taggedRef = imageRef; - if (!taggedRef.includes(':')) taggedRef += ':latest'; - - cacheData ??= await this.dockerPhpService.readCachedUpdateStatus(); - const containerData = cacheData[taggedRef]; - if (!containerData) return null; - return containerData.status?.toLowerCase() === 'true'; - } + async isUpdateAvailableCached( + imageRef: string, + cacheData?: Record<string, CachedStatusEntry> + ): Promise<boolean | null> { + let taggedRef = imageRef; + // If neither a tag (:) nor a digest (@) is present, default to :latest + if (!/[:@]/.test(taggedRef)) taggedRef += ':latest'; + + cacheData ??= await this.dockerPhpService.readCachedUpdateStatus(); + const containerData = cacheData[taggedRef]; + if (!containerData) return null; + if (containerData.status == null) return null; + return String(containerData.status).toLowerCase() === 'true'; + }
🧹 Nitpick comments (6)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (2)
26-30: Make cache read resilient and type the return.If the cache file is missing or malformed, this throws and bubbles up. Prefer a safe default and explicit return type.
Apply:
- async readCachedUpdateStatus(cacheFile = '/var/lib/docker/unraid-update-status.json') { - const cache = await readFile(cacheFile, 'utf8'); - const cacheData = JSON.parse(cache); - return cacheData as Record<string, CachedStatusEntry>; - } + async readCachedUpdateStatus( + cacheFile = '/var/lib/docker/unraid-update-status.json' + ): Promise<Record<string, CachedStatusEntry>> { + try { + const cache = await readFile(cacheFile, 'utf8'); + const cacheData = JSON.parse(cache); + return cacheData as Record<string, CachedStatusEntry>; + } catch { + // File missing or invalid JSON → treat as empty cache + return {}; + } + }
41-44: Add explicit return type for refreshDigestsViaPhp.Minor clarity/readability improvement.
- async refreshDigestsViaPhp( + async refreshDigestsViaPhp( dockerUpdatePath = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerUpdate.php' - ) { + ): Promise<boolean> {api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (1)
60-63: Add explicit return type to mutation.Clarifies intent and improves IDE help.
- @Mutation(() => Boolean) - public async refreshDockerDigests() { - return this.dockerManifestService.refreshDigests(); - } + @Mutation(() => Boolean) + public async refreshDockerDigests(): Promise<boolean> { + return this.dockerManifestService.refreshDigests(); + }api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (3)
23-27: Type refreshDigests and keep the mutex API explicit.Adds Promise to the signature; no behavior change.
- async refreshDigests(mutex = this.refreshDigestsMutex, dockerUpdatePath?: string) { + async refreshDigests( + mutex = this.refreshDigestsMutex, + dockerUpdatePath?: string + ): Promise<boolean> { return mutex.do(() => { return this.dockerPhpService.refreshDigestsViaPhp(dockerUpdatePath); }); }
39-48: Add explicit return type to isRebuildReady.Keeps signatures clear.
- async isRebuildReady(networkMode?: string) { + async isRebuildReady(networkMode?: string): Promise<boolean> { if (!networkMode || !networkMode.startsWith('container:')) return false; const target = networkMode.slice('container:'.length); try { await docker.getContainer(target).inspect(); return false; } catch { return true; // unresolved target -> ':???' equivalent } }
13-16: Confirm mutex behavior with custom dockerUpdatePath.The default mutex’s executor doesn’t accept params; refreshDigests correctly passes dockerUpdatePath via the closure. Just calling out that this design will dedupe all refreshes regardless of path, which is likely desired. If callers may need distinct queues per path, consider keying the mutex map by path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/docker/docker.module.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
🧠 Learnings (4)
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
📚 Learning: 2024-12-11T20:39:02.511Z
Learnt from: pujitm
PR: unraid/api#978
File: web/helpers/apollo-cache/index.ts:134-139
Timestamp: 2024-12-11T20:39:02.511Z
Learning: In `web/helpers/apollo-cache/index.ts`, the `cache.readQuery` method returns `null` when it fails to read from the cache, rather than throwing an exception.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
10-21: LGTM: schedules look sane; mutex in service will dedupe overlaps.Daily cron and 5s post-start are reasonable. Since DockerManifestService uses a mutex, concurrent triggers won’t race.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
31-39: Fix: don’t conflate unknown status with “false” & support digest-only refs.
Right now, a missingstatusfalls through asfalse, and defaulting to:latestonly checks for:(so digest-only refs with@aren’t explicitly handled). Also, add an explicitPromise<boolean | null>return type.File: api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
Lines: 31–39- async isUpdateAvailableCached(imageRef: string, cacheData?: Record<string, CachedStatusEntry>) { - let taggedRef = imageRef; - if (!taggedRef.includes(':')) taggedRef += ':latest'; - - cacheData ??= await this.dockerPhpService.readCachedUpdateStatus(); - const containerData = cacheData[taggedRef]; - if (!containerData) return null; - return containerData.status?.toLowerCase() === 'true'; - } + async isUpdateAvailableCached( + imageRef: string, + cacheData?: Record<string, CachedStatusEntry> + ): Promise<boolean | null> { + let taggedRef = imageRef; + // Only append :latest if there’s neither a tag (:) nor a digest (@) + if (!/[:@]/.test(taggedRef)) taggedRef += ':latest'; + + cacheData ??= await this.dockerPhpService.readCachedUpdateStatus(); + const containerData = cacheData[taggedRef]; + // Missing entry or missing status ⇒ unknown + if (!containerData || containerData.status == null) return null; + return containerData.status.toLowerCase() === 'true'; + }No schema changes required: the field is already declared nullable (
@ResolveField(() => Boolean, { nullable: true })andisUpdateAvailable: Boolean).
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (2)
15-18: Add explicit generics and return/param types for stronger typing.Tighten types for the mutex and method to improve IDE help and compile-time safety.
- private readonly refreshDigestsMutex = new AsyncMutex(() => { + private readonly refreshDigestsMutex = new AsyncMutex<boolean>(() => { return this.dockerPhpService.refreshDigestsViaPhp(); });- async refreshDigests(mutex = this.refreshDigestsMutex, dockerUpdatePath?: string) { + async refreshDigests( + mutex: AsyncMutex<boolean> = this.refreshDigestsMutex, + dockerUpdatePath?: string + ): Promise<boolean> { return mutex.do(() => { return this.dockerPhpService.refreshDigestsViaPhp(dockerUpdatePath); }); }Also applies to: 25-29
41-50: Only treat “not found” (404) as rebuild-ready; avoid masking engine errors.Catching all errors and returning true will mark rebuild-ready on transient Docker daemon issues. Limit “true” to 404/not-found; otherwise return false.
- async isRebuildReady(networkMode?: string) { + async isRebuildReady(networkMode?: string): Promise<boolean> { if (!networkMode || !networkMode.startsWith('container:')) return false; const target = networkMode.slice('container:'.length); try { await docker.getContainer(target).inspect(); return false; - } catch { - return true; // unresolved target -> ':???' equivalent + } catch (err: any) { + // Only treat "no such container" as rebuild-ready; other errors are indeterminate + const code = err?.statusCode ?? err?.status ?? err?.code; + if (code === 404 || err?.message?.toLowerCase().includes('not found')) return true; + return false; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
🧠 Learnings (2)
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
📚 Learning: 2024-12-11T20:39:02.511Z
Learnt from: pujitm
PR: unraid/api#978
File: web/helpers/apollo-cache/index.ts:134-139
Timestamp: 2024-12-11T20:39:02.511Z
Learning: In `web/helpers/apollo-cache/index.ts`, the `cache.readQuery` method returns `null` when it fails to read from the cache, rather than throwing an exception.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (3)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
Injectable(6-22)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (2)
Injectable(22-103)CachedStatusEntry(13-20)packages/unraid-shared/src/util/processing.ts (1)
AsyncMutex(37-67)
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
1-10: ESM imports and AsyncMutex import look correct.Good use of .js import specifiers and bringing in AsyncMutex from the shared util. This aligns with our ESM guidelines and resolves the prior missing import issue.
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
Outdated
Show resolved
Hide resolved
pujitm
force-pushed
the
feat/docker-template
branch
from
39cf3b6 to
057bd12
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
62-69: Add a focused test for containerUpdateStatuses passthroughWe now provide
DockerPhpServicein the resolver test; add an assertion thatresolver.containerUpdateStatuses()calls through and returns the service value.describe('DockerResolver', () => { let resolver: DockerResolver; let dockerService: DockerService; + let dockerPhpService: DockerPhpService; @@ const module: TestingModule = await Test.createTestingModule({ @@ }).compile(); const resolver = module.get<DockerResolver>(DockerResolver); + dockerPhpService = module.get<DockerPhpService>(DockerPhpService); expect(resolver).toBeInstanceOf(DockerResolver); }); @@ + it('should return container update statuses via php service', async () => { + const mockStatuses = { 'container-1': { hasUpdate: true, current: 'sha:abc', latest: 'sha:def' } }; + vi.mocked(dockerPhpService.getContainerUpdateStatuses).mockResolvedValue(mockStatuses as any); + + const result = await resolver.containerUpdateStatuses(); + expect(result).toEqual(mockStatuses); + expect(dockerPhpService.getContainerUpdateStatuses).toHaveBeenCalledTimes(1); + });api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (1)
34-37: Test coverage: assert containerUpdateStatuses resolves via DockerPhpServiceAdd a unit test to verify the resolver delegates to
DockerPhpService.getContainerUpdateStatuses().describe('DockerResolver', () => { let resolver: DockerResolver; let dockerService: DockerService; + let dockerPhpService: DockerPhpService; @@ resolver = module.get<DockerResolver>(DockerResolver); dockerService = module.get<DockerService>(DockerService); + dockerPhpService = module.get<DockerPhpService>(DockerPhpService); @@ + it('should return container update statuses via php service', async () => { + const mockStatuses = { 'container-1': { hasUpdate: true, current: 'sha:a', latest: 'sha:b' } }; + vi.mocked(dockerPhpService.getContainerUpdateStatuses).mockResolvedValue(mockStatuses as any); + + const result = await resolver.containerUpdateStatuses(); + expect(result).toEqual(mockStatuses); + expect(dockerPhpService.getContainerUpdateStatuses).toHaveBeenCalledTimes(1); + });api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (2)
25-33: Honor nullable field contract: return null on failure and add explicit type/descriptionAvoid throwing from resolvers for expected read-path failures; log and return null.
- @ResolveField(() => Boolean, { nullable: true }) - public async isUpdateAvailable(@Parent() container: DockerContainer) { - try { - return await this.dockerManifestService.isUpdateAvailableCached(container.image); - } catch (error) { - this.logger.error(error); - throw new AppError('Failed to read cached update status. See graphql-api.log for details.'); - } - } + @ResolveField(() => Boolean, { + nullable: true, + description: 'True if a newer remote image digest exists; null when unable to determine.', + }) + public async isUpdateAvailable(@Parent() container: DockerContainer): Promise<boolean | null> { + try { + return await this.dockerManifestService.isUpdateAvailableCached(container.image); + } catch (error) { + this.logger.error(error); + return null; + } + }
40-43: Mirror fail-safe pattern for isRebuildReady and add explicit type/description- @ResolveField(() => Boolean, { nullable: true }) - public async isRebuildReady(@Parent() container: DockerContainer) { - return this.dockerManifestService.isRebuildReady(container.hostConfig?.networkMode); - } + @ResolveField(() => Boolean, { + nullable: true, + description: 'True if rebuild is needed (e.g., container:* network target missing); null when unknown.', + }) + public async isRebuildReady(@Parent() container: DockerContainer): Promise<boolean | null> { + try { + return await this.dockerManifestService.isRebuildReady(container.hostConfig?.networkMode); + } catch (error) { + this.logger.error(error); + return null; + } + }api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
72-81: Make docker.push parser resilient to property order and quote styles.Current regex assumes name precedes update and only single quotes. Use lookaheads to support any order and both quote styles.
- const re = /docker\.push\(\{[^}]*name:'([^']+)'[^}]*update:(\d)[^}]*\}\);/g; - for (const m of js.matchAll(re)) { - const name = m[1]; - const updateStatus = Number(m[2]) as StatusItem['updateStatus']; + const re = + /docker\.push\(\{(?=[^}]*\bname:\s*(['"])([^'"]+)\1)(?=[^}]*\bupdate:\s*(\d))[^}]*\}\);/g; + for (const m of js.matchAll(re)) { + const name = m[2]; + const updateStatus = Number(m[3]) as StatusItem['updateStatus']; items.push({ name, updateStatus: this.updateStatusToString(updateStatus) }); }
🧹 Nitpick comments (16)
api/src/unraid-api/cron/job.module.ts (1)
1-12: Make JobModule global to avoid repeated imports across feature modulesSince this module only centralizes ScheduleModule.forRoot(), mark it global so feature modules (CronModule, DockerModule) don’t need to import it, preventing accidental multiple scheduler wiring.
Apply:
-import { Module } from '@nestjs/common'; +import { Global, Module } from '@nestjs/common'; -@Module({ +@Global() +@Module({ imports: [ScheduleModule.forRoot()], }) export class JobModule {}Then import JobModule only once (e.g., in AppModule) and remove it elsewhere.
api/src/unraid-api/cron/cron.module.ts (1)
3-9: Avoid importing JobModule here (centralize in AppModule or make JobModule @global)To ensure a single ScheduleModule.forRoot() path and reduce duplication, import JobModule only in AppModule (or mark JobModule as @global and drop imports here).
If adopting @global on JobModule:
-import { JobModule } from '@app/unraid-api/cron/job.module.js'; @@ -@Module({ - imports: [JobModule], - providers: [WriteFlashFileService, LogRotateService], -}) +@Module({ + providers: [WriteFlashFileService, LogRotateService], +}) export class CronModule {}api/src/unraid-api/graph/resolvers/docker/docker-config.model.ts (1)
3-7: Validate and document expected cron format/timezone for updateCheckCronScheduleAs this is user/config-sourced, consider:
- Adding a brief description to the field clarifying it must be a cron expression supported by the “cron” package.
- Validating on write (e.g., in the service/mutation that persists this value) and surfacing errors early; also decide whether to normalize or store a timezone alongside it.
Example tweak (documentation only):
- @Field(() => String) + @Field(() => String, { description: 'Cron expression used to schedule update checks (cron package syntax). Uses server timezone unless otherwise specified.' }) updateCheckCronSchedule!: string;api/src/unraid-api/app/app.module.ts (1)
16-28: Keep a single import path for scheduler bootstrapAppModule importing JobModule is sufficient; drop JobModule from feature modules (e.g., CronModule, DockerModule) to avoid redundant imports. If you mark JobModule as @global, you can still import it only here to execute forRoot once.
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts (1)
50-52: Minor naming nit: consider aligning the param name with the new typeRename
dockerConfigServicetoorganizerConfigServicefor clarity now that it’s organizer-scoped. No behavior change.- constructor( - private readonly dockerConfigService: DockerOrganizerConfigService, - private readonly dockerService: DockerService - ) {} + constructor( + private readonly organizerConfigService: DockerOrganizerConfigService, + private readonly dockerService: DockerService + ) {}api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
1-1: Remove unused import
ConfigModuleis imported but not used in this spec. Safe to drop.-import { ConfigModule } from '@nestjs/config';api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts (1)
32-39: Optional: prefer CronTime-based validation to avoid version couplingUsing
CronTimewill validate expressions without relying on the newervalidateCronExpressionhelper, making this resilient acrosscronv3/v4+.-import { validateCronExpression } from 'cron'; +import { CronTime } from 'cron'; @@ - async validate(config: object): Promise<DockerConfig> { - const dockerConfig = await validateObject(DockerConfig, config); - const cronExpression = validateCronExpression(dockerConfig.updateCheckCronSchedule); - if (!cronExpression.valid) { - throw new AppError(`Cron expression not supported: ${dockerConfig.updateCheckCronSchedule}`); - } - return dockerConfig; - } + async validate(config: object): Promise<DockerConfig> { + const dockerConfig = await validateObject(DockerConfig, config); + try { + // Throws if invalid + new CronTime(dockerConfig.updateCheckCronSchedule); + } catch { + throw new AppError(`Cron expression not supported: ${dockerConfig.updateCheckCronSchedule}`); + } + return dockerConfig; + }
CronTimeis part of thecronAPI across versions. (npmjs.com)api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.ts (2)
29-49: Avoid hardcoding 'root' as an object keyUse the constant for the key to prevent drift if DEFAULT_ORGANIZER_ROOT_ID changes.
- entries: { - root: { + entries: { + [DEFAULT_ORGANIZER_ROOT_ID]: { type: 'folder', id: DEFAULT_ORGANIZER_ROOT_ID, name: 'Root', children: [], }, },
51-58: Tighten API surface and return proper status code on validation failure
- Accept
unknownto matchvalidateObjectand avoid unnecessary widening.- Use 422/400 instead of default 500 for invalid user/config input.
- async validate(config: object): Promise<OrganizerV1> { + async validate(config: unknown): Promise<OrganizerV1> { const organizer = await validateObject(OrganizerV1, config); const { isValid, errors } = await validateOrganizerIntegrity(organizer); if (!isValid) { - throw new AppError(`Docker organizer validation failed: ${JSON.stringify(errors, null, 2)}`); + // Avoid giant payloads in the error message; details can be logged upstream if needed. + throw new AppError('Docker organizer validation failed', 422); } return organizer; }api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (2)
36-39: Declare explicit return typeClarifies intent and aligns with our TS conventions.
- async refreshContainerDigestsAfterStartup() { + async refreshContainerDigestsAfterStartup(): Promise<void> { await this.dockerManifestService.refreshDigests(); }
18-31: Optional: react to schedule changes at runtimeIf
updateCheckCronSchedulecan change without a restart, consider re-registering the cron job when config updates (or document that a restart is required). Mutex already prevents overlap.api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (2)
1-14: Drop AppError import if adopting null-return approachPrevents an “unused import” warning.
-import { AppError } from '@app/core/errors/app-error.js';
50-53: Add explicit return type to mutation- public async refreshDockerDigests() { + public async refreshDockerDigests(): Promise<boolean> { return this.dockerManifestService.refreshDigests(); }api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (3)
8-14: Fix typo and consider exporting the enum.
- Typo: “propogate” → “propagate”.
- Exporting the enum consolidates these user-facing strings for reuse and prevents drift elsewhere.
-/** Note that these values propogate down to api consumers, so be aware of breaking changes. */ +/** Note that these values propagate down to API consumers, so be aware of breaking changes. */ -enum UpdateStatus { +export enum UpdateStatus { UP_TO_DATE = 'UP_TO_DATE', UPDATE_AVAILABLE = 'UPDATE_AVAILABLE', REBUILD_READY = 'REBUILD_READY', UNKNOWN = 'UNKNOWN', }
83-102: Tiny simplification: table-lookup over switch.Functionally equivalent and a bit tighter.
- private updateStatusToString( - updateStatus: StatusItem['updateStatus'] - ): ExplicitStatusItem['updateStatus'] { - switch (updateStatus) { - case 0: - return UpdateStatus.UP_TO_DATE; - case 1: - return UpdateStatus.UPDATE_AVAILABLE; - case 2: - return UpdateStatus.REBUILD_READY; - default: - return UpdateStatus.UNKNOWN; - } - } + private updateStatusToString( + updateStatus: StatusItem['updateStatus'] + ): ExplicitStatusItem['updateStatus'] { + const map = [ + UpdateStatus.UP_TO_DATE, + UpdateStatus.UPDATE_AVAILABLE, + UpdateStatus.REBUILD_READY, + UpdateStatus.UNKNOWN, + ] as const; + return map[updateStatus] ?? UpdateStatus.UNKNOWN; + }
104-114: Minor: limit split to three parts.Avoid unnecessary allocations if the output contains more nulls unexpectedly.
- const parts = stdout.split('\0'); // [html, "docker.push(...)", busyFlag] + const parts = stdout.split('\0', 3); // [html, "docker.push(...)", busyFlag]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(2 hunks)api/src/unraid-api/cron/cron.module.ts(1 hunks)api/src/unraid-api/cron/job.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/container-status.job.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-config.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts(4 hunks)api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
🧰 Additional context used
📓 Path-based instructions (7)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Prefer adding new API files under api/src/unraid-api/ instead of legacy locations
Files:
api/src/unraid-api/cron/cron.module.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.model.tsapi/src/unraid-api/cron/job.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.tsapi/src/unraid-api/app/app.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.tsapi/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
api/src/unraid-api/cron/cron.module.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.model.tsapi/src/unraid-api/cron/job.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.tsapi/src/unraid-api/app/app.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.tsapi/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
In API TypeScript, use .js extensions in import specifiers for ESM compatibility
Files:
api/src/unraid-api/cron/cron.module.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.model.tsapi/src/unraid-api/cron/job.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.tsapi/src/unraid-api/app/app.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.tsapi/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
For cache-manager v7, always use TTL values in milliseconds when setting cache TTL
Files:
api/src/unraid-api/cron/cron.module.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.model.tsapi/src/unraid-api/cron/job.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.tsapi/src/unraid-api/app/app.module.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-container.resolver.tsapi/src/unraid-api/graph/resolvers/docker/container-status.job.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx): When testing for errors, use .rejects.toThrow() without asserting specific message strings
Focus tests on behavior, not implementation details like exact error messages
Avoid brittle tests that depend on non-essential details (e.g., log formats)
Use mocks as nouns (objects), not verbs (functions)
Files:
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
🧠 Learnings (6)
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to api/**/vitest.config.@(js|ts|mjs|cjs) : API test runner is Vitest
Applied to files:
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
📚 Learning: 2025-09-02T19:35:14.427Z
Learnt from: elibosley
PR: unraid/api#1639
File: web/components/standalone-mount.ts:38-54
Timestamp: 2025-09-02T19:35:14.427Z
Learning: In the unraid/api project, exposing Apollo client and GraphQL utilities (apolloClient, gql, graphqlParse) on the global window object is intentional to allow developers to interact with GraphQL from the browser console without needing to be inside Vue components. Developer experience is prioritized over minimizing global exposure in this context.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
🧬 Code graph analysis (8)
api/src/unraid-api/cron/cron.module.ts (3)
api/src/unraid-api/app/app.module.ts (1)
Module(22-72)api/src/unraid-api/cron/job.module.ts (1)
Module(9-12)api/src/unraid-api/graph/resolvers/docker/docker.module.ts (1)
Module(15-37)
api/src/unraid-api/cron/job.module.ts (3)
api/src/unraid-api/app/app.module.ts (1)
Module(22-72)api/src/unraid-api/cron/cron.module.ts (1)
Module(7-11)api/src/unraid-api/graph/resolvers/docker/docker.module.ts (1)
Module(15-37)
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.ts (5)
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts (1)
Injectable(46-225)api/src/unraid-api/organizer/organizer.ts (2)
DEFAULT_ORGANIZER_VIEW_ID(14-14)DEFAULT_ORGANIZER_ROOT_ID(15-15)api/src/unraid-api/graph/resolvers/validation.utils.ts (1)
validateObject(21-40)api/src/unraid-api/organizer/organizer.validation.ts (1)
validateOrganizerIntegrity(90-102)api/src/core/errors/app-error.ts (1)
AppError(4-37)
api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts (3)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
Injectable(9-40)api/src/unraid-api/graph/resolvers/validation.utils.ts (1)
validateObject(21-40)api/src/core/errors/app-error.ts (1)
AppError(4-37)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (3)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
Injectable(9-40)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
Injectable(11-51)api/src/core/utils/plugins/php-loader.ts (1)
phpLoader(38-66)
api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts (2)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
Resolver(22-151)api/src/core/errors/app-error.ts (1)
AppError(4-37)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (2)
api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts (1)
Injectable(12-40)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
Injectable(11-51)
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (2)
api/src/unraid-api/cron/job.module.ts (1)
Module(9-12)api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
Module(39-80)
🔇 Additional comments (13)
api/package.json (1)
97-97: Verify cron 4.3.0 downgrade and compatibility– Found direct imports of
validateCronExpressionin docker-config.service.ts and use ofCronJob.fromin container-status.job.ts; confirm that moving from 4.3.3 → 4.3.0 is intentional and that no fixes in 4.3.3 are required for your cron-based jobs.
– Ensure existing behavior (validation, scheduling via @nestjs/schedule 6.0.0) remains unchanged under 4.3.0 and update tests or docs if needed.api/src/unraid-api/cron/job.module.ts (1)
4-11: Nice centralization of scheduler bootstrapWrapping ScheduleModule.forRoot() in a single-purpose JobModule simplifies wiring/testing and aligns with DRY.
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts (1)
8-8: DI refactor to DockerOrganizerConfigService looks correctImport path and type swap align with the organizer-specific config service. No logic changes introduced.
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
23-27: Good: organizer config provider override addedOverriding
DockerOrganizerConfigServicekeeps the module compilation isolated from file IO. Looks good.api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts (2)
11-15: LGTM: tests updated to organizer-scoped servicesImports switched to
DockerOrganizerConfigServiceand organizer service path; spec semantics maintained.
141-141: LGTM: DI tokens/types aligned
configServicetyped/retrieved asDockerOrganizerConfigServiceand provider override updated accordingly.Also applies to: 181-188, 223-224
api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts (2)
26-30: Sensible defaultDefaulting
updateCheckCronScheduletoCronExpression.EVERY_DAY_AT_6AMcentralizes a predictable schedule.
6-7: cron version v4.3.0 supports validateCronExpression
api/package.json depends on cron v4.3.0, which provides the validateCronExpression export.api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.ts (1)
21-27: LGTM: config key and filenameCorrect ESM import usage and sensible key/filename choices; aligns with our config-persistence pattern.
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (3)
3-4: LGTM: job wiringContainerStatusJob + JobModule integration looks correct.
20-25: LGTM: providers and resolver registrationNew services/resolver are correctly provided; ESM import specifiers use .js as required.
Also applies to: 27-29, 33-33
16-16: ScheduleModule.forRoot() invoked only in JobModule
Found a singleScheduleModule.forRoot()call inapi/src/unraid-api/cron/job.module.ts(line 10); NestJS dynamic modules are initialized once per application, so no duplicate scheduler initialization.api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
4-4: ESM import suffix looks correct.Local import uses .js extension per API ESM guideline.
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
Outdated
Show resolved
Hide resolved
pujitm
force-pushed
the
feat/docker-template
branch
from
057bd12 to
a661d89
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/package.json (1)
97-97: Keepcrondependency and add commit note for version pin
I found direct imports ofvalidateCronExpressioninapi/src/unraid-api/graph/resolvers/docker/docker-config.service.tsandCronJob.frominapi/src/unraid-api/graph/resolvers/docker/container-status.job.ts, socronis required. If pinning to 4.3.0 is intentional (e.g. for compatibility with @nestjs/schedule), add a short commit note explaining the downgrade; otherwise, consider updating to the latest compatible version.api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
22-23: Add an override for DockerPhpService in the test setup
Mock DockerPhpService before.compile()to prevent constructor side effects:.overrideProvider(DockerOrganizerConfigService) .useValue({ getConfig: vi.fn() }) + .overrideProvider(DockerPhpService) + .useValue({ getContainerUpdateStatuses: vi.fn() }) .overrideProvider(DockerConfigService) .useValue({ getConfig: vi.fn() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
api/dev/configs/api.json(1 hunks)api/generated-schema.graphql(3 hunks)api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/container-status.job.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(2 hunks)api/vite.config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/dev/configs/api.json
🚧 Files skipped from review as they are similar to previous changes (7)
- api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
- api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
- api/generated-schema.graphql
- api/src/unraid-api/graph/resolvers/docker/container-status.job.ts
- api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
- api/vite.config.ts
- api/src/unraid-api/app/app.module.ts
🧰 Additional context used
📓 Path-based instructions (7)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Prefer adding new API files under api/src/unraid-api/ instead of legacy locations
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx): When testing for errors, use .rejects.toThrow() without asserting specific message strings
Focus tests on behavior, not implementation details like exact error messages
Avoid brittle tests that depend on non-essential details (e.g., log formats)
Use mocks as nouns (objects), not verbs (functions)
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
In API TypeScript, use .js extensions in import specifiers for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
For cache-manager v7, always use TTL values in milliseconds when setting cache TTL
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
🔇 Additional comments (3)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (3)
7-7: Import looks correct and ESM-compliant.Using .js in the specifier aligns with our ESM guideline.
12-13: Organizer imports moved under organizer/ — good modularization.Path update is consistent and keeps concerns grouped.
67-67: Good: Resolver test stubs DockerPhpService.getContainerUpdateStatuses.This keeps the resolver unit-scoped without touching external update checks.
pujitm
force-pushed
the
feat/docker-template
branch
from
dc8e020 to
63e37a0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1582 +/- ##
==========================================
- Coverage 58.53% 58.48% -0.06%
==========================================
Files 624 636 +12
Lines 30732 31154 +422
Branches 3617 3650 +33
==========================================
+ Hits 17989 18219 +230
- Misses 12706 12898 +192
Partials 37 37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
31-45: Fix digest/tag normalization and honor “unknown” cache status (return null).Current logic appends :latest even for digest refs and returns false instead of null when status is unknown, violating the method’s contract.
- async isUpdateAvailableCached(imageRef: string, cacheData?: Record<string, CachedStatusEntry>) { - let taggedRef = imageRef; - if (!taggedRef.includes(':')) taggedRef += ':latest'; - - cacheData ??= await this.dockerPhpService.readCachedUpdateStatus(); - const containerData = cacheData[taggedRef]; - if (!containerData) return null; - return containerData.status?.toLowerCase() === 'true'; - } + async isUpdateAvailableCached( + imageRef: string, + cacheData?: Record<string, CachedStatusEntry> + ): Promise<boolean | null> { + let taggedRef = imageRef.trim(); + // Default only when neither a tag (:) nor a digest (@) is present + if (!/[:@]/.test(taggedRef)) taggedRef += ':latest'; + + cacheData ??= await this.dockerPhpService.readCachedUpdateStatus(); + const containerData = cacheData[taggedRef]; + if (!containerData) return null; + if (containerData.status == null) return null; + return containerData.status === 'true'; + }api/vite.config.ts (3)
79-82: Good: wrapper.php is now watchedaddWatchFile ensures edits trigger rebuilds in watch mode. This addresses prior feedback.
1-2: Use POSIX joins to avoid malformed asset paths on WindowsRollup expects POSIX-style fileName. Replace path.join with path.posix.join and adjust the import.
-import { basename, join } from 'node:path'; +import { basename, posix as pathPosix } from 'node:path';
91-95: Fix cross-platform asset fileNameEnsure forward slashes in emitted asset paths.
- this.emitFile({ - type: 'asset', - fileName: join('assets', basename(file)), - source: content, - }); + this.emitFile({ + type: 'asset', + fileName: pathPosix.join('assets', basename(file)), + source: content, + });
🧹 Nitpick comments (7)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (2)
22-23: Tighten the config mock and add test hygieneConsider returning a minimal, realistic config shape (even a small object) to better match call sites. Also add afterEach cleanup to prevent cross-test bleed from vi.fn mocks.
Example hygiene snippet to add near the top-level of this spec file:
afterEach(() => { vi.restoreAllMocks(); });If helpful, I can draft a minimal config stub aligned with DockerOrganizerConfigService’s getConfig signature. Do you want me to?
67-67: Mock async return of getContainerUpdateStatuses with an explicit value
getContainerUpdateStatuses is declared asasyncreturning aPromise<ExplicitStatusItem[]>, so update the mock to return an array:- { provide: DockerPhpService, useValue: { getContainerUpdateStatuses: vi.fn() } }, + { provide: DockerPhpService, useValue: { getContainerUpdateStatuses: vi.fn().mockResolvedValue([]) } },api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (4)
15-17: Annotate mutex type and simplify lambda.Improve clarity and IDE help; no behavior change.
- private readonly refreshDigestsMutex = new AsyncMutex(() => { - return this.dockerPhpService.refreshDigestsViaPhp(); - }); + private readonly refreshDigestsMutex: AsyncMutex<boolean> = new AsyncMutex( + () => this.dockerPhpService.refreshDigestsViaPhp() + );
25-29: Add explicit return/param types; optionally leverage default op when no path is provided.This tightens typing and avoids an unnecessary wrapper when using the default operation.
- async refreshDigests(mutex = this.refreshDigestsMutex, dockerUpdatePath?: string) { - return mutex.do(() => { - return this.dockerPhpService.refreshDigestsViaPhp(dockerUpdatePath); - }); - } + async refreshDigests( + mutex: AsyncMutex<boolean> = this.refreshDigestsMutex, + dockerUpdatePath?: string + ): Promise<boolean> { + return dockerUpdatePath + ? mutex.do(() => this.dockerPhpService.refreshDigestsViaPhp(dockerUpdatePath)) + : mutex.do(); + }
47-51: Correct JSDoc example for networkMode.Docker’s “container:” network mode references a container name/ID, not an image ref.
- * @param networkMode - The network mode of the container, e.g. "container:unraid/baseimage:latest". + * @param networkMode - The network mode of the container, e.g. "container:<container_name>".
52-61: Add explicit return type; consider treating only 404 as rebuild-ready.Typing improves clarity. Optionally, avoid returning true on generic Docker errors (offline/permission), and only return true when the target container is missing.
- async isRebuildReady(networkMode?: string) { + async isRebuildReady(networkMode?: string): Promise<boolean> { if (!networkMode || !networkMode.startsWith('container:')) return false; const target = networkMode.slice('container:'.length); try { await docker.getContainer(target).inspect(); return false; - } catch { - return true; // unresolved target -> ':???' equivalent + } catch (err) { + // unresolved target -> ':???' equivalent + // If feasible, narrow to 404 only: + // return typeof err === 'object' && err && 'statusCode' in err && (err as { statusCode?: number }).statusCode === 404; + return true; } }If you want the stricter behavior, confirm that treating only 404 as rebuild-ready matches UI expectations; I can update the code accordingly.
api/vite.config.ts (1)
83-97: Hoist phpFiles and drop unnecessary async; make path resolution cwd-safe
- Define phpFiles once (outside the plugin) and reuse to remove duplication (Lines 80 and 84).
- generateBundle doesn’t await anything; remove async.
- Prefer fileURLToPath + resolve so paths aren’t sensitive to process.cwd (aligns with retrieved learnings).
Apply within this block:
- buildStart() { - const phpFiles = ['src/core/utils/plugins/wrapper.php']; + buildStart() { phpFiles.forEach((file) => this.addWatchFile(file)); }, - async generateBundle() { - const phpFiles = ['src/core/utils/plugins/wrapper.php']; + generateBundle() { phpFiles.forEach((file) => {And add near the top of the file (outside the plugin object):
// cwd-safe path resolution (derived from retrieved learnings) import { fileURLToPath } from 'node:url'; import { dirname, resolve } from 'node:path'; const rootDir = dirname(fileURLToPath(import.meta.url)); const phpFiles = [resolve(rootDir, 'src/core/utils/plugins/wrapper.php')];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
api/dev/configs/api.json(1 hunks)api/generated-schema.graphql(3 hunks)api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/container-status.job.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(2 hunks)api/vite.config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/dev/configs/api.json
🚧 Files skipped from review as they are similar to previous changes (7)
- api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
- api/package.json
- api/src/unraid-api/graph/resolvers/docker/container-status.job.ts
- api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
- api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
- api/generated-schema.graphql
- api/src/unraid-api/app/app.module.ts
🧰 Additional context used
📓 Path-based instructions (7)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Prefer adding new API files under api/src/unraid-api/ instead of legacy locations
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/vite.config.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
In API TypeScript, use .js extensions in import specifiers for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
For cache-manager v7, always use TTL values in milliseconds when setting cache TTL
Files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.tsapi/vite.config.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
api/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx): When testing for errors, use .rejects.toThrow() without asserting specific message strings
Focus tests on behavior, not implementation details like exact error messages
Avoid brittle tests that depend on non-essential details (e.g., log formats)
Use mocks as nouns (objects), not verbs (functions)
Files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
🧠 Learnings (4)
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
📚 Learning: 2024-12-11T20:39:02.511Z
Learnt from: pujitm
PR: unraid/api#978
File: web/helpers/apollo-cache/index.ts:134-139
Timestamp: 2024-12-11T20:39:02.511Z
Learning: In `web/helpers/apollo-cache/index.ts`, the `cache.readQuery` method returns `null` when it fails to read from the cache, rather than throwing an exception.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
PR: unraid/api#1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
api/vite.config.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
🧬 Code graph analysis (1)
api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (2)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (2)
Injectable(34-132)CachedStatusEntry(25-32)packages/unraid-shared/src/util/processing.ts (1)
AsyncMutex(68-150)
🔇 Additional comments (3)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (2)
7-7: Good ESM import for new dependencyUsing .js in the specifier aligns with our ESM guideline and keeps the test isolated. Looks good.
12-13: All old docker-organizer imports removed
No occurrences of the previous@app/unraid-api/graph/resolvers/docker/docker-organizer.servicepath remain.api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
1-9: Imports look correct and ESM-safe (.js specifiers).No issues spotted with dependency paths or types.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
api/generated-schema.graphql (1)
1092-1094: Add field descriptions and confirm guards for DockerContainer update fieldsConsider documenting
isUpdateAvailableandisRebuildReadyfor introspection, and verify resolver-level permission guards exist.Example:
"""True if an update is available for this container""" isUpdateAvailable: Boolean """True if a rebuild is ready for this container""" isRebuildReady: Booleanapi/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
1-145: Verify presence of resolvers for DockerContainer.isUpdateAvailable/isRebuildReady with guardsEnsure
@ResolveFieldmethods exist (with READ permissions) for these new fields in docker-container.resolver.ts.#!/bin/bash fd docker-container.resolver.ts api | xargs -I{} rg -nC2 "@ResolveField\\(.*is(UpdateAvailable|RebuildReady)" {}
136-144: Align type with schema and harden non-null contract; add error handlingAdd a description and return
[]on failures; also ensure the list is non-null (schema currently shows JSON!). Decide on JSON vs typed array and keep both resolver and generated schema in sync.- @UseFeatureFlag('ENABLE_NEXT_DOCKER_RELEASE') - @UsePermissions({ - action: AuthAction.READ_ANY, - resource: Resource.DOCKER, - }) - @ResolveField(() => [ExplicitStatusItem]) - public async containerUpdateStatuses() { - return this.dockerPhpService.getContainerUpdateStatuses(); - } + @UseFeatureFlag('ENABLE_NEXT_DOCKER_RELEASE') + @UsePermissions({ + action: AuthAction.READ_ANY, + resource: Resource.DOCKER, + }) + @ResolveField(() => [ExplicitStatusItem], { + nullable: false, + description: 'Per-container update status snapshot; empty when unavailable.', + }) + public async containerUpdateStatuses(): Promise<ExplicitStatusItem[]> { + try { + const data = await this.dockerPhpService.getContainerUpdateStatuses(); + return Array.isArray(data) ? data : []; + } catch { + return []; + } + }If you intend to keep JSON!, switch the ResolveField type to GraphQLJSON and still keep the try/catch and fallback.
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (2)
46-53: Fix Logger.warn usage (message first; avoid passing objects as the message).Current calls pass the payload/error as the first arg and the message as the second. Prefer a clear string message; include details in the message or log separately.
- if (success) return data; - this.logger.warn(cacheData, 'Invalid cached update status'); + if (success) return data; + this.logger.warn('Invalid cached update status'); return {}; } catch (error) { - this.logger.warn(error, 'Failed to read cached update status'); + this.logger.warn(`Failed to read cached update status: ${String(error)}`); return {}; }
84-91: Make docker.push parser resilient to property order, quote styles, and multi-digit codes.Use lookaheads so name/update can appear in any order and support both single/double quotes (+ allow \d+).
- const re = /docker\.push\(\{[^}]*name:'([^']+)'[^}]*update:(\d)[^}]*\}\);/g; + const re = + /docker\.push\(\{(?=[^}]*\bname:\s*(['"])([^'"]+)\1)(?=[^}]*\bupdate:\s*(\d+))[^}]*\}\);/g; for (const m of js.matchAll(re)) { - const name = m[1]; - const updateStatus = Number(m[2]) as StatusItem['updateStatus']; + const name = m[2]; + const updateStatus = Number(m[3]) as StatusItem['updateStatus']; items.push({ name, updateStatus: this.updateStatusToString(updateStatus) }); }
🧹 Nitpick comments (19)
api/.env.development (1)
34-34: Reorder the new flag to satisfy dotenv-linter.dotenv-linter flags UnorderedKey; place ENABLE_NEXT_DOCKER_RELEASE before ENVIRONMENT to keep keys sorted.
-ENABLE_NEXT_DOCKER_RELEASE=true +# moved above ENVIRONMENT to satisfy dotenv-linter +ENABLE_NEXT_DOCKER_RELEASE=trueapi/src/unraid-api/utils/feature-flag.helper.ts (1)
20-28: Narrow the flag value type to boolean to avoid truthy/falsey surprises.Today any truthy value passes; tightening to boolean makes misuse harder and improves DX.
-export function checkFeatureFlag<T extends Record<string, any>>(flags: T, key: keyof T): void { - const isEnabled = Boolean(flags[key]); +export function checkFeatureFlag<T extends Record<string, boolean>>(flags: T, key: keyof T): void { + const isEnabled = flags[key];api/src/consts.ts (1)
83-93: Add a typed key alias for reuse across decorators/tests.This centralizes the flag key type and avoids hard-coded string unions elsewhere.
export const FeatureFlags = Object.freeze({ ENABLE_NEXT_DOCKER_RELEASE, }); + +export type FeatureFlagKey = keyof typeof FeatureFlags;api/src/unraid-api/graph/resolvers/docker/docker-update-status.model.ts (1)
18-25: Add field descriptions to improve schema introspection/docs.Small win for API consumers navigating the enum/object in GraphQL tooling.
-@ObjectType() +@ObjectType({ description: 'Explicit container update status item.' }) export class ExplicitStatusItem { - @Field(() => String) + @Field(() => String, { description: 'Docker container name.' }) name!: string; - @Field(() => UpdateStatus) + @Field(() => UpdateStatus, { description: 'Computed update status for the container.' }) updateStatus!: UpdateStatus; }api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
61-68: Mocks look good; consider global reset to avoid test bleed.Add an afterEach to restore spies/mocks across tests.
// at bottom/top-level in this spec file import { afterEach } from 'vitest'; afterEach(() => { vi.restoreAllMocks(); });api/src/unraid-api/decorators/use-feature-flag.decorator.spec.ts (1)
45-48: Prefer reading metadata from the prototype method rather than the instanceUsing instance methods can be sensitive to binding. Query the prototype method to be robust:
-const metadata = reflector.get(OMIT_IF_METADATA_KEY, instance.testQuery); +const metadata = reflector.get(OMIT_IF_METADATA_KEY, (TestResolver as any).prototype.testQuery);Apply similarly for other assertions in this file.
Also applies to: 60-63, 84-89, 107-116, 134-140
api/generated-schema.graphql (1)
142-145: Confirm nullability semantics for isSpinning across types
ArrayDisk.isSpinningis nullable whileDisk.isSpinningis non-null. If intentional, add short descriptions clarifying the difference (e.g., “nullable when array not started”).Also applies to: 353-355
api/src/unraid-api/decorators/omit-if.decorator.ts (1)
25-44: Narrow decorator kind or handle property-decorator path
OmitIfis typed as MethodDecorator & PropertyDecorator, butExtensionsis a method/class decorator. Either narrow to methods or add a property-safe branch to avoid misuse.-export function OmitIf(condition: boolean | (() => boolean)): MethodDecorator & PropertyDecorator { +export function OmitIf(condition: boolean | (() => boolean)): MethodDecorator { @@ - return (target: any, propertyKey?: string | symbol, descriptor?: PropertyDescriptor) => { + return (target: any, propertyKey?: string | symbol, descriptor?: PropertyDescriptor) => { if (shouldOmit) { SetMetadata(OMIT_IF_METADATA_KEY, true)( target, propertyKey as string, descriptor as PropertyDescriptor ); Extensions({ omitIf: true })( target, propertyKey as string, descriptor as PropertyDescriptor ); } return descriptor; }; }api/src/unraid-api/decorators/omit-if.decorator.spec.ts (1)
24-27: Read metadata from prototype to avoid binding quirksAccess the decorated function via the prototype for consistency.
-const metadata = reflector.get(OMIT_IF_METADATA_KEY, instance.testMethod); +const metadata = reflector.get(OMIT_IF_METADATA_KEY, (TestResolver as any).prototype.testMethod);Apply similarly to other occurrences.
Also applies to: 37-40, 68-72, 110-116, 135-140, 165-170
api/src/unraid-api/graph/resolvers/docker/docker-config.service.spec.ts (1)
90-96: Use constructor or message matching for error assertionsMatching on a concrete error instance can be brittle. Prefer constructor and/or message.
-await expect(service.validate(inputConfig)).rejects.toThrow( - new AppError('Cron expression not supported: invalid-cron') -); +await expect(service.validate(inputConfig)).rejects.toThrow(AppError); +await expect(service.validate(inputConfig)).rejects.toThrow( + /Cron expression not supported: invalid-cron/ +);Apply similarly to the other error cases.
Also applies to: 110-116, 130-136
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.ts (1)
56-63: Trim or structure validation error output to avoid huge messagesStringifying full error objects can be noisy. Consider summarizing count and first N items.
-throw new AppError(`Docker organizer validation failed: ${JSON.stringify(errors, null, 2)}`); +const summary = Array.isArray(errors) ? `${errors.length} issue(s)` : 'See details in logs'; +// Optionally log full details with a logger +throw new AppError(`Docker organizer validation failed: ${summary}`);api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
75-87: Consider normalizing/validating organizer IDs and deduplicating inputsGuard against duplicates and invalid IDs (e.g., Set for childrenIds, verify parent existence) before persisting.
-childrenIds: childrenIds ?? [], +childrenIds: Array.from(new Set(childrenIds ?? [])),Also applies to: 95-104, 111-117, 125-134
api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts (1)
37-44: Prefer unknown over object for external input
objectis too permissive/implicit.unknownencourages explicit validation (which you already do).-async validate(config: object): Promise<DockerConfig> { +async validate(config: unknown): Promise<DockerConfig> {api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (6)
121-131: Guard phpLoader failures; return [] instead of throwing from resolver path.Prevents a transient PHP error from bubbling and failing the entire GraphQL field.
- const stdout = await phpLoader({ - file: dockerContainersPath, - method: 'GET', - }); + let stdout: string; + try { + stdout = await phpLoader({ + file: dockerContainersPath, + method: 'GET', + }); + } catch (error) { + this.logger.warn(`Failed to fetch container update statuses: ${String(error)}`); + return []; + } const parts = stdout.split('\0'); // [html, "docker.push(...)", busyFlag] const js = parts[1] || ''; return this.parseStatusesFromDockerPush(js);If you intend failures to propagate upstream, skip this change. Please confirm the desired behavior.
65-78: Add explicit return type for clarity.- async refreshDigestsViaPhp( + async refreshDigestsViaPhp( dockerUpdatePath = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerUpdate.php' - ) { + ): Promise<boolean> {
104-113: Be explicit about status code 3 mapping.Keeps intent obvious even if new codes are added later.
switch (updateStatus) { case 0: return UpdateStatus.UP_TO_DATE; case 1: return UpdateStatus.UPDATE_AVAILABLE; case 2: return UpdateStatus.REBUILD_READY; + case 3: + return UpdateStatus.UNKNOWN; default: return UpdateStatus.UNKNOWN; }
18-26: Optionally make the cache schema strict.If the source file may include extra keys, skip. Otherwise, .strict() prevents silently accepting unexpected fields.
-const CachedStatusEntrySchema = z.object({ +const CachedStatusEntrySchema = z.object({ /** sha256 digest - "sha256:..." */ local: z.string(), /** sha256 digest - "sha256:..." */ remote: z.string(), /** whether update is available (true), not available (false), or unknown (null) */ status: z.enum(['true', 'false']).nullable(), -}); +}).strict();
41-42: Factor default file paths into named constants.Centralizes config, eases reuse and future changes.
Add near the top (after imports):
const DEFAULT_CACHE_FILE = '/var/lib/docker/unraid-update-status.json'; const DOCKER_UPDATE_PATH = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerUpdate.php'; const DOCKER_CONTAINERS_PATH = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php';Then update signatures:
- cacheFile = '/var/lib/docker/unraid-update-status.json' + cacheFile = DEFAULT_CACHE_FILE - dockerUpdatePath = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerUpdate.php' + dockerUpdatePath = DOCKER_UPDATE_PATH - dockerContainersPath = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php' + dockerContainersPath = DOCKER_CONTAINERS_PATHAlso applies to: 66-67, 122-123
32-32: Remove empty constructor.Not needed; the class has no injected deps.
- constructor() {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
api/.env.development(1 hunks)api/.gitignore(1 hunks)api/docs/developer/feature-flags.md(1 hunks)api/generated-schema.graphql(5 hunks)api/src/consts.ts(2 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/cron/job.module.ts(1 hunks)api/src/unraid-api/decorators/omit-if.decorator.spec.ts(1 hunks)api/src/unraid-api/decorators/omit-if.decorator.ts(1 hunks)api/src/unraid-api/decorators/use-feature-flag.decorator.spec.ts(1 hunks)api/src/unraid-api/decorators/use-feature-flag.decorator.ts(1 hunks)api/src/unraid-api/graph/graph.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/container-status.job.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-config.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-update-status.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(7 hunks)api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.ts(1 hunks)api/src/unraid-api/utils/feature-flag.helper.ts(1 hunks)packages/unraid-shared/src/util/processing.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/.gitignore
- api/docs/developer/feature-flags.md
🚧 Files skipped from review as they are similar to previous changes (4)
- api/src/unraid-api/cron/job.module.ts
- api/src/unraid-api/graph/resolvers/docker/container-status.job.ts
- packages/unraid-shared/src/util/processing.ts
- api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/unraid-api/decorators/use-feature-flag.decorator.spec.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to {api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx) : Avoid brittle tests that depend on non-essential details (e.g., log formats)
Applied to files:
api/src/unraid-api/decorators/omit-if.decorator.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.service.spec.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Applied to files:
api/src/unraid-api/decorators/omit-if.decorator.spec.ts
📚 Learning: 2025-09-09T14:03:45.207Z
Learnt from: pujitm
PR: unraid/api#1582
File: api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts:6-6
Timestamp: 2025-09-09T14:03:45.207Z
Learning: The cron package (version 4.3.0) exports validateCronExpression function that returns an object with a `valid` boolean property, which is the correct way to validate cron expressions in this codebase. Don't suggest changes to working cron validation code that uses this pattern.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-config.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-config.service.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
🪛 dotenv-linter (3.3.0)
api/.env.development
[warning] 34-34: [UnorderedKey] The ENABLE_NEXT_DOCKER_RELEASE key should go before the ENVIRONMENT key
(UnorderedKey)
🔇 Additional comments (11)
api/src/environment.ts (1)
114-115: LGTM: boolean flag derivation is clear and side-effect free.api/src/unraid-api/graph/graph.module.ts (1)
68-73: Transformer order looks right; please confirm behavior under flag flip.Applying permissions then omit-if is sensible. Verify (in an e2e or schema snapshot test) that disabling ENABLE_NEXT_DOCKER_RELEASE actually removes gated fields from the generated schema when the env var is set before module bootstrap.
api/src/unraid-api/decorators/use-feature-flag.decorator.ts (1)
20-22: LGTM: thin wrapper over OmitIf with strong typing.api/src/unraid-api/decorators/use-feature-flag.decorator.spec.ts (2)
23-317: Solid coverage and clear scenariosGood breadth across Query/Mutation/ResolveField, multi-decorator, and ordering behavior.
203-210: No action needed:reflect-metadatais loaded in Vitest setupFiles
The API’s Vitest configuration (api/vite.config.tsline 220) listsreflect-metadatainsetupFiles, so all tests already have metadata support.api/src/unraid-api/decorators/omit-if.decorator.ts (1)
51-80: Transformer implementation looks goodClean use of mapSchema and extensions flag; object and root fields handled correctly.
api/src/unraid-api/decorators/omit-if.decorator.spec.ts (1)
1-172: Well-structured tests for OmitIf behaviorCovers true/false conditions, function conditions, and GraphQL decorator integration.
api/src/unraid-api/graph/resolvers/docker/docker-config.service.spec.ts (1)
39-77: Good coverage of valid/invalid cron paths and utility interactionsMocks and assertions clearly verify both validateObject and validateCronExpression.
Also applies to: 154-171, 173-193
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer-config.service.ts (1)
22-33: Feature-gated config service and sensible defaultsEnabled flag, configKey, fileName, and default structure look consistent.
Also applies to: 34-54
api/src/unraid-api/graph/resolvers/docker/docker-config.service.ts (1)
31-44: Cron validation approach LGTMUse of
validateCronExpression().validmatches the established pattern and tests.api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
46-47: LGTM: safeParse + empty-on-error fallback.Good use of zod.safeParse to validate shape and return {} on invalid JSON.
| * @param dockerUpdatePath - Optional path to the DockerUpdate.php file. If not provided, the default path will be used. | ||
| * @returns True if the digests were refreshed, false if the operation failed | ||
| */ | ||
| async refreshDigests(mutex = this.refreshDigestsMutex, dockerUpdatePath?: string) { |
| dockerUpdatePath = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerUpdate.php' | ||
| ) { | ||
| try { | ||
| await phpLoader({ |
|
|
||
| private parseStatusesFromDockerPush(js: string): ExplicitStatusItem[] { | ||
| const items: ExplicitStatusItem[] = []; | ||
| const re = /docker\.push\(\{[^}]*name:'([^']+)'[^}]*update:(\d)[^}]*\}\);/g; |
There was a problem hiding this comment.
would "luv" to see some unit tests on this regex
| "convert": "5.12.0", | ||
| "cookie": "1.0.2", | ||
| "cron": "4.3.3", | ||
| "cron": "4.3.0", |
There was a problem hiding this comment.
ye, there was a type conflict between 4.3.3 and 4.3.0 (which is what nest scheduler currently depends on)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
api/generated-schema.graphql (2)
1105-1107: Add descriptions and verify permission guards for new DockerContainer fields.Fields lack docstrings in the generated schema; ensure @ResolveField includes descriptions and @UsePermissions guards.
#!/bin/bash set -euo pipefail resolver="$(fd -t f docker-container.resolver.ts api/src || true)" if [[ -n "$resolver" ]]; then echo "--- Inspecting: $resolver" rg -n "@ResolveField" -n -C2 "$resolver" rg -n "isUpdateAvailable|isRebuildReady" -n -C2 "$resolver" rg -n "UsePermissions" -n -C2 "$resolver" else echo "Resolver not found" fi
1137-1138: Non-null list contract: ensure resolver returns [] on errors.containerUpdateStatuses is [ExplicitStatusItem!]!; resolver must not throw.
#!/bin/bash set -euo pipefail res="$(fd -t f docker.resolver.ts api/src || true)" if [[ -n "$res" ]]; then echo "--- Inspecting: $res" rg -n "containerUpdateStatuses" -C3 "$res" # Look for try/catch and [] fallback rg -nP "(try|catch|return\s*\[\])" -C2 "$res" else echo "docker.resolver.ts not found" fi
🧹 Nitpick comments (9)
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.ts (3)
7-7: Make the semicolon optional in the matcher.Upstream snippets might omit the trailing semicolon; harmless to accept both.
- const dockerPushRegex = /docker\.push\(\{[^}]*(?:(?:[^{}]|{[^}]*})*)\}\);/g; + const dockerPushRegex = /docker\.push\(\{[^}]*(?:(?:[^{}]|{[^}]*})*)\}\);?/g;
13-14: Support both quote styles and constrain update to expected range [0–3].Future-proofs parsing and filters noise.
- const nameMatch = objectContent.match(/name\s*:\s*'([^']+)'/); - const updateMatch = objectContent.match(/update\s*:\s*(\d)/); + const nameMatch = objectContent.match(/name\s*:\s*(["'])([^"']+)\1/); + const updateMatch = objectContent.match(/update\s*:\s*([0-3])/);
1-4: Narrow the return type for updateStatus.Reflects actual domain values and avoids downstream casts.
export interface DockerPushMatch { name: string; - updateStatus: number; + updateStatus: 0 | 1 | 2 | 3; }api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts (1)
100-124: Add coverage for double quotes and missing semicolon.These cases are cheap to support and likely to appear in the wild.
it('should handle all valid update status values', () => { @@ }); + it('should support double-quoted names', () => { + const jsCode = 'docker.push({name:"nginx",update:1});'; + const result = parseDockerPushCalls(jsCode); + expect(result).toEqual([{ name: 'nginx', updateStatus: 1 }]); + }); + + it('should support missing trailing semicolon', () => { + const jsCode = "docker.push({name:'nginx',update:1})"; + const result = parseDockerPushCalls(jsCode); + expect(result).toEqual([{ name: 'nginx', updateStatus: 1 }]); + }); + it('should handle nested braces in other properties', () => {api/generated-schema.graphql (1)
2438-2439: Add description and permissions to refreshDockerDigests mutation.Document behavior and guard with @usePermissions; regenerate schema after adding decorators.
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (4)
49-54: Fix Logger.warn usage; log strings, not objects, and include error context.Current calls pass non-string as message; prefer stringified message for consistent logs.
- if (success) return data; - this.logger.warn(cacheData, 'Invalid cached update status'); + if (success) return data; + this.logger.warn(`Invalid cached update status: ${JSON.stringify(cacheData)}`); return {}; } catch (error) { - this.logger.warn(error, 'Failed to read cached update status'); + this.logger.warn(`Failed to read cached update status: ${String(error)}`); return {}; }
66-69: Annotate return type for clarity.Add Promise for refreshDigestsViaPhp.
- async refreshDigestsViaPhp( + async refreshDigestsViaPhp( dockerUpdatePath = '/usr/local/emhttp/plugins/dynamix.docker.manager/include/DockerUpdate.php' - ) { + ): Promise<boolean> {
33-34: Remove empty constructor.Unneeded noise.
- constructor() {} + // no-op
119-129: Consider local error handling to align with non-null resolver.Optional: catch PhpError here and return [] so callers don’t need to special-case failures.
- const stdout = await phpLoader({ - file: dockerContainersPath, - method: 'GET', - }); - const parts = stdout.split('\0'); // [html, "docker.push(...)", busyFlag] - const js = parts[1] || ''; - return this.parseStatusesFromDockerPush(js); + try { + const stdout = await phpLoader({ file: dockerContainersPath, method: 'GET' }); + const parts = stdout.split('\0'); // [html, "docker.push(...)", busyFlag] + const js = parts[1] || ''; + return this.parseStatusesFromDockerPush(js); + } catch { + return []; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/generated-schema.graphql(6 hunks)api/src/unraid-api/decorators/omit-if.decorator.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/decorators/omit-if.decorator.ts
🧰 Additional context used
📓 Path-based instructions (8)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Prefer adding new API files under api/src/unraid-api/ instead of legacy locations
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.tsapi/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.tsapi/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
api/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
In API TypeScript, use .js extensions in import specifiers for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.tsapi/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
api/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
For cache-manager v7, always use TTL values in milliseconds when setting cache TTL
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.tsapi/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.tsapi/src/unraid-api/graph/resolvers/docker/docker-php.service.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts
api/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx)
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx): When testing for errors, use .rejects.toThrow() without asserting specific message strings
Focus tests on behavior, not implementation details like exact error messages
Avoid brittle tests that depend on non-essential details (e.g., log formats)
Use mocks as nouns (objects), not verbs (functions)
Files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to {api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx) : Avoid brittle tests that depend on non-essential details (e.g., log formats)
Applied to files:
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts
🧬 Code graph analysis (2)
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts (1)
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.ts (1)
parseDockerPushCalls(6-24)
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (4)
api/src/unraid-api/graph/resolvers/docker/container-status.job.ts (1)
Injectable(9-47)api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts (1)
Injectable(11-62)api/src/core/utils/plugins/php-loader.ts (1)
phpLoader(38-66)api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.ts (1)
parseDockerPushCalls(6-24)
🔇 Additional comments (4)
api/src/unraid-api/graph/resolvers/docker/utils/docker-push-parser.test.ts (1)
1-5: ESM pathing looks good.Imports use .js extensions per repo ESM policy.
api/generated-schema.graphql (2)
143-145: Confirm nullability semantics for isSpinning.ArrayDisk.isSpinning is nullable; Disk.isSpinning is non-null. If intentional, add doc comments clarifying when ArrayDisk value can be null.
Also applies to: 353-355
1053-1064: Typed status model is a solid improvement.ExplicitStatusItem and UpdateStatus enum clarify API shape and avoid ad-hoc strings.
api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts (1)
93-112: Status mapping overloads look correct.Covers known values and defaults to UNKNOWN safely.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/unraid-shared/src/util/processing.ts (1)
69-71: Privateanyusage acknowledged per prior discussion.Keeping
anyinternally was previously agreed acceptable, but the above refactor removes the need for it entirely.
🧹 Nitpick comments (1)
packages/unraid-shared/src/util/processing.ts (1)
81-89: Docs: avoid implying mixed return types across concurrent operations.With the invariant “one T per instance,” examples should show operations returning the same T (or use separate mutexes). Otherwise consumers may assume heterogenous U is safe.
Also applies to: 117-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-shared/src/util/processing.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
packages/unraid-shared/src/util/processing.ts
🧠 Learnings (1)
📚 Learning: 2025-09-09T14:34:07.189Z
Learnt from: pujitm
PR: unraid/api#1582
File: packages/unraid-shared/src/util/processing.ts:69-71
Timestamp: 2025-09-09T14:34:07.189Z
Learning: In TypeScript code, pujitm considers it acceptable to use `any` type for private/internal fields when the type is not exposed through the public API and is masked by properly typed method declarations. The focus is on ensuring the public interface maintains type safety rather than enforcing strict typing on all internal implementation details.
Applied to files:
packages/unraid-shared/src/util/processing.ts
🔇 Additional comments (1)
packages/unraid-shared/src/util/processing.ts (1)
100-129: Re-entrancy gotcha: callingdo()from within the in-flight operation can self-await.If an operation invokes
mutex.do(...)on the same instance, it will get the same promise and risk a circular await. Add a brief JSDoc warning and a test to lock this in.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
AsyncMutexinunraid-shared -> processing.ts, for ergonomically de-duplicating async operations.@OmitIfdecorator for omitting graphql queries, mutations, or field resolvers from the runtime graphql schema.FeatureFlagsexport fromconsts.ts@UseFeatureFlagdecorator built uponOmitIfcheckFeatureFlagfor constructing & throwing aForbiddenErrorif the given feature flag evaluates tofalse.Summary by CodeRabbit
New Features
Chores
Documentation
Tests