fix: no sizeRootFs unless queried #1710
Conversation
WalkthroughAdds a GraphQL field-inspection helper and tests; DockerResolver now reads GraphQL info to request container size. DockerService gained separate caches for size/no-size and cache-key logic; size field moved to BigInt across schema, models, and codegen; frontend typings and codegen updated; lint-staged rule removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as GraphQL Client
participant R as DockerResolver
participant H as GraphQLFieldHelper
participant S as DockerService
participant Cache as Cache Store
participant D as Docker Engine
C->>R: query containers(skipCache, selectionSet)
R->>H: isFieldRequested(info, "sizeRootFs")
H-->>R: true/false
R->>S: getContainers({ skipCache, size: requestsSize })
alt size == true
S->>Cache: get(CONTAINER_WITH_SIZE_CACHE_KEY)
else size == false
S->>Cache: get(CONTAINER_CACHE_KEY)
end
alt cache hit
Cache-->>S: containers
else cache miss
S->>D: listContainers(size)
D-->>S: containers
S->>Cache: set(appropriate key, containers, TTL)
end
S-->>R: containers
R-->>C: containers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
Claude finished @elibosley's task —— View job Code Review - Analyzing Diff OnlyTodo List:
Critical Issue Found
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1710 +/- ##
==========================================
+ Coverage 52.59% 52.65% +0.05%
==========================================
Files 854 855 +1
Lines 47869 47926 +57
Branches 4746 4762 +16
==========================================
+ Hits 25177 25234 +57
Misses 22623 22623
Partials 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…thout size - Added a new cache key for containers that include the sizeRootFs field. - Updated the DockerService to use the appropriate cache key based on the size parameter. - Enhanced logging to indicate whether the cache is being used with or without size. - Added unit tests to verify the functionality of separate cache keys.
- Changed the type of sizeRootFs from Int to BigInt to accommodate larger file sizes. - Updated related GraphQL schema and resolver definitions to reflect this change. - Incremented version in api.json to 4.22.2 for compatibility with the new schema.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (2)
112-169: Also assert cache invalidation clears both keys in start/stop testsService now deletes both container caches; tests currently only assert one key. Add expects for the with-size key as well.
Apply this diff outside this hunk to the start/stop tests:
expect(mockCacheManager.del).toHaveBeenCalledWith(DockerService.CONTAINER_CACHE_KEY); +expect(mockCacheManager.del).toHaveBeenCalledWith(DockerService.CONTAINER_WITH_SIZE_CACHE_KEY);
112-169: Avoid brittle exact error message assertionsFor error tests in this spec, prefer
.rejects.toThrow()without a string to avoid brittleness per repo guidelines.Apply this diff outside this hunk:
-await expect(service.start('not-found')).rejects.toThrow( - 'Container not-found not found after starting' -); +await expect(service.start('not-found')).rejects.toThrow(); -await expect(service.stop('not-found')).rejects.toThrow( - 'Container not-found not found after stopping' -); +await expect(service.stop('not-found')).rejects.toThrow();api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
192-198: Invalidate both caches togetherParallel
delis good. Consider debouncing if external events can burst.
200-213: Start: invalidate caches then refetch and publishFlow is sound. Minor micro-opt: reuse the fetched
containersto compute appInfo to avoid an extra list later.Apply outside-hunk adjustment:
-const appInfo = await this.getAppInfo(); +const appInfo = { + info: { + apps: { installed: containers.length, running: containers.filter(c => c.state === ContainerState.RUNNING).length }, + }, +};
215-244: Stop: retry loop with sleep is reasonableGood visibility logs. Same micro-opt as in start regarding
getAppInfo.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.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/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.ts
🧠 Learnings (5)
📚 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.resolver.ts
📚 Learning: 2024-11-05T14:48:56.949Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:41-47
Timestamp: 2024-11-05T14:48:56.949Z
Learning: Input types for GraphQL mutations can be pulled from the GraphQL codegen types, as they are generated into the API files. Therefore, explicit typing in the resolver files may be unnecessary.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.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.resolver.ts
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: All GraphQL under web must follow the .query.ts/.mutation.ts naming convention
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
📚 Learning: 2025-07-21T14:00:25.140Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under `web/` must follow this naming convention
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
🧬 Code graph analysis (3)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (1)
api/src/unraid-api/cli/generated/graphql.ts (1)
DockerContainer(698-718)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (1)
Info(811-834)api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1)
info(37-41)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
containers(44-51)
🔇 Additional comments (9)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (2)
112-169: Good addition: verifies dual cache keys and TTL in msTest correctly asserts distinct keys and 60000 ms TTL. Matches cache-manager v7 guidance.
221-222: Default size=false path assertedAsserting
size: falseonlistContainersaligns with the new default.api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
1-1: Imports OK for ESMUsing
.jsextensions andGraphQLResolveInfoimport conforms to project conventions.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
34-35: Separate cache key for size-aware fetchesAdding
CONTAINER_WITH_SIZE_CACHE_KEYis the right split to avoid cross-contamination.
90-90: Populate sizeRootFs only when provided by DockerCasting to include optional
SizeRootFsis fine; value will beundefinedwhen size not requested.If Dockerode ever returns
0rather than omitting the field, confirm that0is a meaningful value vs. “unknown”.
109-144: Size-aware cache selection and TTL in ms look correct
- Default
size=falseis aligned with resolver logic.- Cache key switches by
size.- TTL multiplies seconds by 1000, matching cache-manager v7.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (3)
84-97: Default: no size requestedTest asserts
size: falsewhen selection set is empty. Solid.
99-135: Direct size field triggers size=trueGood coverage for explicit
sizeRootFsselection.
181-210: Non-size fields pathGood negative case. Keep.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
Outdated
Show resolved
Hide resolved
| it('should request size when fragment spread is present', async () => { | ||
| const mockContainers: DockerContainer[] = []; | ||
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | ||
|
|
||
| const mockInfoWithFragmentSpread = { | ||
| fieldNodes: [ | ||
| { | ||
| selectionSet: { | ||
| selections: [ | ||
| { | ||
| kind: 'FragmentSpread', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ], | ||
| } as any; | ||
|
|
||
| await resolver.containers(false, mockInfoWithFragmentSpread); | ||
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | ||
| }); |
There was a problem hiding this comment.
Fragment spread should resolve via info.fragments
Provide a fragment definition containing sizeRootFs; otherwise expect size: false.
Apply this diff:
-const mockInfoWithFragmentSpread = {
+const mockInfoWithFragmentSpread = {
fieldNodes: [
{
selectionSet: {
selections: [
{
kind: 'FragmentSpread',
- },
+ name: { kind: 'Name', value: 'ContainerFields' },
+ },
],
},
},
],
+ fragments: {
+ ContainerFields: {
+ kind: 'FragmentDefinition',
+ name: { kind: 'Name', value: 'ContainerFields' },
+ typeCondition: { kind: 'NamedType', name: { kind: 'Name', value: 'DockerContainer' } },
+ selectionSet: {
+ selections: [{ kind: 'Field', name: { kind: 'Name', value: 'sizeRootFs' } }],
+ },
+ },
+ },
} as any;
-await resolver.containers(false, mockInfoWithFragmentSpread);
-expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true });
+await resolver.containers(false, mockInfoWithFragmentSpread);
+expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true });Also add a negative test where the fragment lacks sizeRootFs and expect size: false.
📝 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.
| it('should request size when fragment spread is present', async () => { | |
| const mockContainers: DockerContainer[] = []; | |
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | |
| const mockInfoWithFragmentSpread = { | |
| fieldNodes: [ | |
| { | |
| selectionSet: { | |
| selections: [ | |
| { | |
| kind: 'FragmentSpread', | |
| }, | |
| ], | |
| }, | |
| }, | |
| ], | |
| } as any; | |
| await resolver.containers(false, mockInfoWithFragmentSpread); | |
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | |
| }); | |
| it('should request size when fragment spread is present', async () => { | |
| const mockContainers: DockerContainer[] = []; | |
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | |
| const mockInfoWithFragmentSpread = { | |
| fieldNodes: [ | |
| { | |
| selectionSet: { | |
| selections: [ | |
| { | |
| kind: 'FragmentSpread', | |
| name: { kind: 'Name', value: 'ContainerFields' }, | |
| }, | |
| ], | |
| }, | |
| }, | |
| ], | |
| fragments: { | |
| ContainerFields: { | |
| kind: 'FragmentDefinition', | |
| name: { kind: 'Name', value: 'ContainerFields' }, | |
| typeCondition: { kind: 'NamedType', name: { kind: 'Name', value: 'DockerContainer' } }, | |
| selectionSet: { | |
| selections: [{ kind: 'Field', name: { kind: 'Name', value: 'sizeRootFs' } }], | |
| }, | |
| }, | |
| }, | |
| } as any; | |
| await resolver.containers(false, mockInfoWithFragmentSpread); | |
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | |
| }); |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts around
lines 159 to 179, the test that asserts size:true when a FragmentSpread is
present is incomplete because it does not supply a fragment definition via
info.fragments; update mockInfoWithFragmentSpread to include an appropriate
fragments map where the referenced fragment contains a selection with a Field
named "sizeRootFs" so the resolver will detect it and call getContainers with
size: true, and add a new negative test that supplies a fragment definition
without "sizeRootFs" (or a FragmentSpread whose fragment selections lack that
field) and assert dockerService.getContainers is called with size: false.
…DockerResolver - Replaced the custom isFieldRequested method with GraphQLFieldHelper to streamline field request checks. - Updated DockerResolver to utilize the new helper for determining if the sizeRootFs field is requested. - Added unit tests to ensure correct behavior of field request handling in various scenarios. - Improved clarity and maintainability of the resolver code.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
144-145: TTL units are correct (ms), but the name is misleading.Consider renaming the constant to reflect milliseconds to prevent future confusion.
Proposed tweak (no behavior change):
public static readonly CACHE_TTL_MS = 60_000; // ... await this.cacheManager.set(cacheKey, containers, DockerService.CACHE_TTL_MS);api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (2)
273-275: Also assert both caches are invalidated.Strengthen expectations to reflect clearContainerCache() deleting both keys.
expect(mockCacheManager.del).toHaveBeenCalledWith(DockerService.CONTAINER_CACHE_KEY); +expect(mockCacheManager.del).toHaveBeenCalledWith(DockerService.CONTAINER_WITH_SIZE_CACHE_KEY);Also applies to: 331-333, 350-351, 361-362
347-350: Follow test guideline: don’t assert exact error messages.Use
.rejects.toThrow()without arguments unless the message format is under test.- await expect(service.start('not-found')).rejects.toThrow( - 'Container not-found not found after starting' - ); + await expect(service.start('not-found')).rejects.toThrow(); - await expect(service.stop('not-found')).rejects.toThrow( - 'Container not-found not found after stopping' - ); + await expect(service.stop('not-found')).rejects.toThrow();Also applies to: 358-362
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
3-3: Import Kind or rely on literal kinds consistently.If you prefer enum constants, import
Kindto avoid stringly-typed AST checks. Otherwise, keep literals but ensure tests build nodes with matching literals.Apply:
-import type { GraphQLResolveInfo } from 'graphql'; +import type { GraphQLResolveInfo } from 'graphql'; +// Alternatively: import { Kind, type GraphQLResolveInfo } from 'graphql';
43-47: Optional: add an explicit return type.Annotating improves readability and catches accidental changes.
Apply:
- public async containers( + public async containers( @Args('skipCache', { defaultValue: false, type: () => Boolean }) skipCache: boolean, @Info() info: GraphQLResolveInfo - ) { + ): Promise<DockerContainer[]> {api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (1)
84-97: Avoidas any; build minimally-typed GraphQL AST for info.Use GraphQL AST shapes (or a tiny helper) and cast once to
GraphQLResolveInfoif needed. This keeps us within the “no any” guideline.Example helper:
+import { Kind, type GraphQLResolveInfo } from 'graphql'; + +const makeInfo = (selections: any[] = [], fragments: Record<string, any> = {}): GraphQLResolveInfo => + ({ + // minimal shape consumed by resolver + fieldNodes: [ + { + kind: Kind.FIELD, + name: { kind: Kind.NAME, value: 'containers' }, + selectionSet: { kind: Kind.SELECTION_SET, selections }, + }, + ], + fragments, + } as unknown as GraphQLResolveInfo);Then replace inline literals:
-const mockInfo = { fieldNodes: [{ selectionSet: { selections: [] } }] } as any; +const mockInfo = makeInfo([]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
api/dev/configs/api.json(1 hunks)api/generated-schema.graphql(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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
Files:
api/src/unraid-api/graph/resolvers/docker/docker.model.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/docker/docker.model.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/docker/docker.model.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.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/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
🧠 Learnings (6)
📚 Learning: 2024-11-05T14:48:56.949Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:41-47
Timestamp: 2024-11-05T14:48:56.949Z
Learning: Input types for GraphQL mutations can be pulled from the GraphQL codegen types, as they are generated into the API files. Therefore, explicit typing in the resolver files may be unnecessary.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
📚 Learning: 2025-07-21T14:00:25.140Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under `web/` must follow this naming convention
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.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.resolver.ts
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: All GraphQL under web must follow the .query.ts/.mutation.ts naming convention
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.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.resolver.ts
📚 Learning: 2025-07-21T14:00:25.140Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.mutation.ts : GraphQL mutations must be placed in `.mutation.ts` files
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
🧬 Code graph analysis (2)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (1)
api/src/unraid-api/cli/generated/graphql.ts (1)
DockerContainer(698-718)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (2)
Docker(679-686)DockerContainer(698-718)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
containers(44-51)
🔇 Additional comments (15)
api/dev/configs/api.json (1)
2-2: Version bump looks good.Please confirm changelog and any client‑visible schema changes are tagged against 4.22.2.
api/src/unraid-api/graph/resolvers/docker/docker.model.ts (1)
4-4: Importing GraphQLBigInt is correct for large sizes.Nice move to a BigInt scalar to avoid 32‑bit limits.
api/generated-schema.graphql (1)
1096-1097: Schema change to BigInt is appropriate.Docstring and type now reflect bytes and large ranges. Ensure resolvers return JS BigInt values to match the scalar. (the-guild.dev)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (7)
34-35: Separate cache key for sized listings is a solid choice.Prevents cache pollution between expensive size queries and default listings.
115-116: Defaulting size=false is the right performance trade‑off.This keeps the common path fast.
119-127: Cache key selection by size is correct.Logs clearly indicate which cache is used; helpful for debugging.
131-131: Good: explicit log when refreshing cache.No action needed.
195-201: Cache invalidation covers both keys.Looks good. Please extend tests to assert invalidation of both keys after start/stop.
Suggested test addendum shown below in the spec comments.
205-207: Start flow invalidates caches before refetch.Good sequencing and pubsub update.
220-221: Stop flow invalidates caches and retries status—LGTM.Retry with sleep is pragmatic; logs help when EXITED isn’t reached.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (2)
112-169: Nice: verifies distinct caches and TTL in ms.This test covers the new behavior well.
219-223: Assertion updated to size: false matches new default.Good catch.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
1-1: Nice: info-aware resolver to avoid unnecessary size computation.Good direction aligning with size-aware service and caching.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (2)
99-135: LGTM: direct field selection should trigger size.Test reads well and matches intended behavior.
181-210: LGTM: non-size fields should not request size.Matches desired behavior.
| @Field(() => GraphQLBigInt, { | ||
| nullable: true, | ||
| description: 'Total size of all files in the container (in bytes)', | ||
| }) | ||
| sizeRootFs?: number; |
There was a problem hiding this comment.
Align TS type with GraphQLBigInt (use bigint, not number).
GraphQLBigInt uses JS BigInt at runtime. Keeping the property typed as number risks precision loss and serialization mismatches for large values.
Apply this minimal change:
- })
- sizeRootFs?: number;
+ })
+ sizeRootFs?: bigint;Follow‑up: ensure the resolver populates this field with a BigInt, not a number (see service comment).
Reference: GraphQLBigInt uses native JS BigInt. (the-guild.dev)
📝 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.
| @Field(() => GraphQLBigInt, { | |
| nullable: true, | |
| description: 'Total size of all files in the container (in bytes)', | |
| }) | |
| sizeRootFs?: number; | |
| @Field(() => GraphQLBigInt, { | |
| nullable: true, | |
| description: 'Total size of all files in the container (in bytes)', | |
| }) | |
| sizeRootFs?: bigint; |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.model.ts around lines 92-96,
the GraphQLBigInt-decorated field sizeRootFs is currently typed as number which
can cause precision loss and mismatch; change the TypeScript type to bigint
(sizeRootFs?: bigint) and update any callers/resolvers to produce/return a
native BigInt value (e.g., BigInt(...)) so the resolver serializes correctly
with GraphQLBigInt.
| it('should request size when inline fragment is present', async () => { | ||
| const mockContainers: DockerContainer[] = []; | ||
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | ||
|
|
||
| const mockInfoWithFragment = { | ||
| fieldNodes: [ | ||
| { | ||
| selectionSet: { | ||
| selections: [ | ||
| { | ||
| kind: 'InlineFragment', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ], | ||
| } as any; | ||
|
|
||
| await resolver.containers(false, mockInfoWithFragment); | ||
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | ||
| }); |
There was a problem hiding this comment.
Incorrect expectation: inline fragment presence alone shouldn’t imply size.
This encodes a false positive: size will be requested even when the fragment does not include sizeRootFs. That contradicts “no sizeRootFs unless queried.”
Update the test to only expect size: true when the inline fragment actually selects sizeRootFs; add a negative case for fragments without it.
-it('should request size when inline fragment is present', async () => {
+it('should request size when inline fragment selects sizeRootFs', async () => {
const mockContainers: DockerContainer[] = [];
vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers);
- const mockInfoWithFragment = {
- fieldNodes: [{ selectionSet: { selections: [{ kind: 'InlineFragment' }] } }],
- } as any;
+ const mockInfoWithFragment = makeInfo([
+ {
+ kind: Kind.INLINE_FRAGMENT,
+ typeCondition: { kind: Kind.NAMED_TYPE, name: { kind: Kind.NAME, value: 'DockerContainer' } },
+ selectionSet: {
+ kind: Kind.SELECTION_SET,
+ selections: [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value: 'sizeRootFs' } }],
+ },
+ },
+ ]);
await resolver.containers(false, mockInfoWithFragment);
expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true });
});
+it('should NOT request size when inline fragment omits sizeRootFs', async () => {
+ vi.mocked(dockerService.getContainers).mockResolvedValue([]);
+ const info = makeInfo([
+ {
+ kind: Kind.INLINE_FRAGMENT,
+ selectionSet: {
+ kind: Kind.SELECTION_SET,
+ selections: [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value: 'id' } }],
+ },
+ },
+ ]);
+ await resolver.containers(false, info);
+ expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: false });
+});📝 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.
| it('should request size when inline fragment is present', async () => { | |
| const mockContainers: DockerContainer[] = []; | |
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | |
| const mockInfoWithFragment = { | |
| fieldNodes: [ | |
| { | |
| selectionSet: { | |
| selections: [ | |
| { | |
| kind: 'InlineFragment', | |
| }, | |
| ], | |
| }, | |
| }, | |
| ], | |
| } as any; | |
| await resolver.containers(false, mockInfoWithFragment); | |
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | |
| }); | |
| it('should request size when inline fragment selects sizeRootFs', async () => { | |
| const mockContainers: DockerContainer[] = []; | |
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | |
| const mockInfoWithFragment = makeInfo([ | |
| { | |
| kind: Kind.INLINE_FRAGMENT, | |
| typeCondition: { kind: Kind.NAMED_TYPE, name: { kind: Kind.NAME, value: 'DockerContainer' } }, | |
| selectionSet: { | |
| kind: Kind.SELECTION_SET, | |
| selections: [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value: 'sizeRootFs' } }], | |
| }, | |
| }, | |
| ]); | |
| await resolver.containers(false, mockInfoWithFragment); | |
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | |
| }); | |
| it('should NOT request size when inline fragment omits sizeRootFs', async () => { | |
| vi.mocked(dockerService.getContainers).mockResolvedValue([]); | |
| const info = makeInfo([ | |
| { | |
| kind: Kind.INLINE_FRAGMENT, | |
| selectionSet: { | |
| kind: Kind.SELECTION_SET, | |
| selections: [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value: 'id' } }], | |
| }, | |
| }, | |
| ]); | |
| await resolver.containers(false, info); | |
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: false }); | |
| }); |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts around
lines 137-157, the test currently asserts size: true when any InlineFragment is
present which is incorrect; change this test so the InlineFragment actually
contains a selection for sizeRootFs (e.g. InlineFragment -> selectionSet ->
selections includes a Field with name.value === 'sizeRootFs') and then assert
dockerService.getContainers was called with size: true, and add a new negative
test where an InlineFragment without a sizeRootFs field (or with unrelated
fields) is passed and you assert dockerService.getContainers was called with
size: false (or omitted), ensuring both positive and negative cases are covered.
| it('should request size when fragment spread is present', async () => { | ||
| const mockContainers: DockerContainer[] = []; | ||
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | ||
|
|
||
| const mockInfoWithFragmentSpread = { | ||
| fieldNodes: [ | ||
| { | ||
| selectionSet: { | ||
| selections: [ | ||
| { | ||
| kind: 'FragmentSpread', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ], | ||
| } as any; | ||
|
|
||
| await resolver.containers(false, mockInfoWithFragmentSpread); | ||
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | ||
| }); |
There was a problem hiding this comment.
Same issue for fragment spreads: resolve and check contents, not mere presence.
Expect size: true only when the referenced fragment contains sizeRootFs.
Apply:
-it('should request size when fragment spread is present', async () => {
+it('should request size when fragment spread selects sizeRootFs', async () => {
vi.mocked(dockerService.getContainers).mockResolvedValue([]);
- const mockInfoWithFragmentSpread = {
- fieldNodes: [{ selectionSet: { selections: [{ kind: 'FragmentSpread' }] } }],
- } as any;
+ const fragments = {
+ WithSize: {
+ kind: Kind.FRAGMENT_DEFINITION,
+ name: { kind: Kind.NAME, value: 'WithSize' },
+ typeCondition: { kind: Kind.NAMED_TYPE, name: { kind: Kind.NAME, value: 'DockerContainer' } },
+ selectionSet: {
+ kind: Kind.SELECTION_SET,
+ selections: [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value: 'sizeRootFs' } }],
+ },
+ },
+ };
+ const mockInfoWithFragmentSpread = makeInfo(
+ [{ kind: Kind.FRAGMENT_SPREAD, name: { kind: Kind.NAME, value: 'WithSize' } }],
+ fragments
+ );
await resolver.containers(false, mockInfoWithFragmentSpread);
expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true });
});Optionally add a negative case where the spread fragment omits sizeRootFs and expect size: false.
📝 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.
| it('should request size when fragment spread is present', async () => { | |
| const mockContainers: DockerContainer[] = []; | |
| vi.mocked(dockerService.getContainers).mockResolvedValue(mockContainers); | |
| const mockInfoWithFragmentSpread = { | |
| fieldNodes: [ | |
| { | |
| selectionSet: { | |
| selections: [ | |
| { | |
| kind: 'FragmentSpread', | |
| }, | |
| ], | |
| }, | |
| }, | |
| ], | |
| } as any; | |
| await resolver.containers(false, mockInfoWithFragmentSpread); | |
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | |
| }); | |
| it('should request size when fragment spread selects sizeRootFs', async () => { | |
| vi.mocked(dockerService.getContainers).mockResolvedValue([]); | |
| const fragments = { | |
| WithSize: { | |
| kind: Kind.FRAGMENT_DEFINITION, | |
| name: { kind: Kind.NAME, value: 'WithSize' }, | |
| typeCondition: { kind: Kind.NAMED_TYPE, name: { kind: Kind.NAME, value: 'DockerContainer' } }, | |
| selectionSet: { | |
| kind: Kind.SELECTION_SET, | |
| selections: [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value: 'sizeRootFs' } }], | |
| }, | |
| }, | |
| }; | |
| const mockInfoWithFragmentSpread = makeInfo( | |
| [{ kind: Kind.FRAGMENT_SPREAD, name: { kind: Kind.NAME, value: 'WithSize' } }], | |
| fragments | |
| ); | |
| await resolver.containers(false, mockInfoWithFragmentSpread); | |
| expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: false, size: true }); | |
| }); |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts around
lines 159 to 179, the test currently treats any FragmentSpread presence as
sufficient to expect size: true; change the mock GraphQLResolveInfo to resolve
the fragment spread by providing a fragments map whose referenced fragment
contains a selection for sizeRootFs (so the resolver will detect the actual
field and call getContainers with size: true), and update the assertion
accordingly; optionally add a second test where the fragment's selection does
NOT include sizeRootFs and assert getContainers was called with size: false.
| // Check if sizeRootFs field is requested in the query | ||
| const requestsSize = this.isFieldRequested(info, 'sizeRootFs'); | ||
| return this.dockerService.getContainers({ skipCache, size: requestsSize }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t treat “any fragment” as requesting size; traverse fragments to check for sizeRootFs explicitly.
Current logic sets size: true whenever any fragment is present, which can trigger expensive size calculations even when sizeRootFs isn’t actually requested. This weakens the PR goal (“no sizeRootFs unless queried”).
Refactor to recursively walk selections and resolve FragmentSpread via info.fragments to detect sizeRootFs precisely. Also iterate all fieldNodes (there can be more than one).
Apply:
- // Check if sizeRootFs field is requested in the query
- const requestsSize = this.isFieldRequested(info, 'sizeRootFs');
+ // Check if sizeRootFs field is explicitly requested in the query (including via fragments)
+ const requestsSize = this.isFieldRequested(info, 'sizeRootFs');
return this.dockerService.getContainers({ skipCache, size: requestsSize });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts around lines 48
to 51, the current logic treats any fragment presence as a request for
sizeRootFs; change this to iterate all info.fieldNodes and recursively walk
their selection sets, resolving FragmentSpread nodes via info.fragments and
handling InlineFragment and Field nodes, checking selection.name.value ===
'sizeRootFs' and returning true only if explicitly found; update the
isFieldRequested helper to perform this recursive traversal and return a boolean
so you only pass size: true when sizeRootFs is actually requested.
| private isFieldRequested(info: GraphQLResolveInfo, fieldName: string): boolean { | ||
| const selections = info.fieldNodes[0]?.selectionSet?.selections; | ||
| if (!selections) return false; | ||
|
|
||
| for (const selection of selections) { | ||
| if (selection.kind === 'Field' && selection.name.value === fieldName) { | ||
| return true; | ||
| } | ||
| // Check nested selections for fragments | ||
| if (selection.kind === 'InlineFragment' || selection.kind === 'FragmentSpread') { | ||
| // For simplicity, if we see fragments, assume the field might be requested | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make field detection robust: recurse and resolve fragments.
Accurately detect sizeRootFs in inline fragments and fragment spreads instead of assuming any fragment implies size. Also scan all fieldNodes.
Apply:
- private isFieldRequested(info: GraphQLResolveInfo, fieldName: string): boolean {
- const selections = info.fieldNodes[0]?.selectionSet?.selections;
- if (!selections) return false;
-
- for (const selection of selections) {
- if (selection.kind === 'Field' && selection.name.value === fieldName) {
- return true;
- }
- // Check nested selections for fragments
- if (selection.kind === 'InlineFragment' || selection.kind === 'FragmentSpread') {
- // For simplicity, if we see fragments, assume the field might be requested
- return true;
- }
- }
- return false;
- }
+ private isFieldRequested(info: GraphQLResolveInfo, fieldName: string): boolean {
+ const visit = (selections?: readonly import('graphql').SelectionNode[]): boolean => {
+ if (!selections) return false;
+ for (const sel of selections) {
+ switch (sel.kind) {
+ case 'Field':
+ if (sel.name.value === fieldName) return true;
+ break;
+ case 'InlineFragment':
+ if (visit(sel.selectionSet?.selections)) return true;
+ break;
+ case 'FragmentSpread': {
+ const frag = info.fragments?.[sel.name.value];
+ if (frag && visit(frag.selectionSet?.selections)) return true;
+ break;
+ }
+ }
+ }
+ return false;
+ };
+ for (const node of info.fieldNodes ?? []) {
+ if (visit(node.selectionSet?.selections)) return true;
+ }
+ return false;
+ }📝 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.
| private isFieldRequested(info: GraphQLResolveInfo, fieldName: string): boolean { | |
| const selections = info.fieldNodes[0]?.selectionSet?.selections; | |
| if (!selections) return false; | |
| for (const selection of selections) { | |
| if (selection.kind === 'Field' && selection.name.value === fieldName) { | |
| return true; | |
| } | |
| // Check nested selections for fragments | |
| if (selection.kind === 'InlineFragment' || selection.kind === 'FragmentSpread') { | |
| // For simplicity, if we see fragments, assume the field might be requested | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| private isFieldRequested(info: GraphQLResolveInfo, fieldName: string): boolean { | |
| const visit = (selections?: readonly import('graphql').SelectionNode[]): boolean => { | |
| if (!selections) return false; | |
| for (const sel of selections) { | |
| switch (sel.kind) { | |
| case 'Field': | |
| if (sel.name.value === fieldName) return true; | |
| break; | |
| case 'InlineFragment': | |
| if (visit(sel.selectionSet?.selections)) return true; | |
| break; | |
| case 'FragmentSpread': { | |
| const frag = info.fragments?.[sel.name.value]; | |
| if (frag && visit(frag.selectionSet?.selections)) return true; | |
| break; | |
| } | |
| } | |
| } | |
| return false; | |
| }; | |
| for (const node of info.fieldNodes ?? []) { | |
| if (visit(node.selectionSet?.selections)) return true; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts around lines
149-165, the isFieldRequested helper currently only inspects the first fieldNode
and treats any fragment presence as a match; change it to scan all
info.fieldNodes and recursively traverse selectionSets: when you encounter a
Field check its name, when you encounter an InlineFragment recurse into its
selectionSet, and when you encounter a FragmentSpread resolve the fragment
definition from info.fragments[fragmentName] and recurse into that fragment's
selectionSet; return true as soon as sizeRootFs (or the requested fieldName) is
found, otherwise return false.
| const sizeValue = (container as Docker.ContainerInfo & { SizeRootFs?: number }).SizeRootFs; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid cast and prepare value for BigInt.
Current cast to add SizeRootFs is brittle. Prefer a small helper type or a type guard, and coerce to BigInt at the source.
Option A (local helper type):
+type ContainerInfoMaybeSized = Docker.ContainerInfo & { SizeRootFs?: number };
-const sizeValue = (container as Docker.ContainerInfo & { SizeRootFs?: number }).SizeRootFs;
+const sizeValue = (container as ContainerInfoMaybeSized).SizeRootFs;Then set the field as BigInt (see next comment).
📝 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 sizeValue = (container as Docker.ContainerInfo & { SizeRootFs?: number }).SizeRootFs; | |
| type ContainerInfoMaybeSized = Docker.ContainerInfo & { SizeRootFs?: number }; | |
| const sizeValue = (container as ContainerInfoMaybeSized).SizeRootFs; |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.service.ts around lines
75-76, avoid the brittle inline cast for SizeRootFs; introduce a small local
helper type or a type guard that safely narrows Docker.ContainerInfo to a shape
that includes SizeRootFs, then read and coerce the value to a BigInt at the
source (e.g., check for undefined/null and numeric/string input, convert via
BigInt) and assign the field as a BigInt so downstream code receives a
strongly-typed BigInt instead of relying on a fragile cast.
| sizeRootFs: sizeValue, | ||
| labels: container.Labels ?? {}, |
There was a problem hiding this comment.
Populate GraphQLBigInt with a JS BigInt.
Return a BigInt to match the scalar and avoid precision issues.
Apply this change:
- sizeRootFs: sizeValue,
+ sizeRootFs: sizeValue !== undefined ? BigInt(sizeValue) : undefined,Reference: GraphQLBigInt expects native BigInt. (the-guild.dev)
📝 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.
| sizeRootFs: sizeValue, | |
| labels: container.Labels ?? {}, | |
| sizeRootFs: sizeValue !== undefined ? BigInt(sizeValue) : undefined, | |
| labels: container.Labels ?? {}, |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.service.ts around lines
92-93, sizeRootFs is being set to sizeValue (a JS number) but the GraphQLBigInt
scalar expects a native BigInt; change the assignment to return a BigInt
instead, e.g. set sizeRootFs to sizeValue != null ? BigInt(sizeValue) : null so
you return a BigInt when a value exists and preserve null/undefined otherwise.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
api/src/unraid-api/graph/resolvers/docker/docker.model.ts (1)
92-96: GraphQL BigInt vs TS number: avoid precision risk.
sizeRootFsis exposed as GraphQL BigInt but typed asnumberin TS. For large containers this can exceed Number.MAX_SAFE_INTEGER. Preferbigintin the model and cast in the service.Apply this diff here:
- }) - sizeRootFs?: number; + }) + sizeRootFs?: bigint;Outside this file, convert in transform:
-const sizeValue = (container as Docker.ContainerInfo & { SizeRootFs?: number }).SizeRootFs; -... -sizeRootFs: sizeValue, +const sizeValue = (container as Docker.ContainerInfo & { SizeRootFs?: number }).SizeRootFs; +... +sizeRootFs: sizeValue !== undefined ? BigInt(sizeValue) : undefined,api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
36-36: TTL unit naming: prefer milliseconds to avoid misuse.cache‑manager v7 expects TTL in ms; you’re multiplying seconds by 1000 at call sites. Rename the constant to reflect ms and remove per‑call math to prevent future bugs.
Apply:
- public static readonly CACHE_TTL_SECONDS = 60; // Cache for 60 seconds + public static readonly CACHE_TTL_MS = 60_000; // Cache for 60 seconds (cache-manager v7 expects ms)- await this.cacheManager.set(cacheKey, containers, DockerService.CACHE_TTL_SECONDS * 1000); + await this.cacheManager.set(cacheKey, containers, DockerService.CACHE_TTL_MS);- await this.cacheManager.set( - DockerService.NETWORK_CACHE_KEY, - networks, - DockerService.CACHE_TTL_SECONDS * 1000 - ); + await this.cacheManager.set( + DockerService.NETWORK_CACHE_KEY, + networks, + DockerService.CACHE_TTL_MS + );Also applies to: 144-145, 186-190
75-76: Optional: cast SizeRootFs to bigint to match GraphQL BigInt.If you switch the model to
bigint, cast here to preserve precision.-const sizeValue = (container as Docker.ContainerInfo & { SizeRootFs?: number }).SizeRootFs; +const sizeValue = (container as Docker.ContainerInfo & { SizeRootFs?: number }).SizeRootFs; ... - sizeRootFs: sizeValue, + sizeRootFs: sizeValue !== undefined ? BigInt(sizeValue) : undefined,Also applies to: 92-92
119-127: Optional: populate both caches when size=true.Listing with size is expensive; when fetched, you can also warm the non‑size cache to reduce a subsequent miss.
- this.logger.debug(`Updating docker container cache (${size ? 'with' : 'without'} size)`); + this.logger.debug(`Updating docker container cache (${size ? 'with' : 'without'} size)`); ... - await this.cacheManager.set(cacheKey, containers, DockerService.CACHE_TTL_SECONDS * 1000); + await this.cacheManager.set(cacheKey, containers, DockerService.CACHE_TTL_MS); + // Warm the other cache when we have the heavier result + if (size) { + await this.cacheManager.set( + DockerService.CONTAINER_CACHE_KEY, + containers, + DockerService.CACHE_TTL_MS + ); + }Also applies to: 131-132, 144-145
api/src/unraid-api/utils/graphql-field-helper.spec.ts (2)
175-178: Avoid order‑sensitive assertion.Object key order isn’t a contract; make the list comparison order‑insensitive.
- expect(fieldsList).toEqual(['id', 'name', 'email', 'profile']); + expect(fieldsList.sort()).toEqual(['email', 'id', 'name', 'profile'].sort());
57-109: Consider adding alias and fragment coverage.Add tests for aliased fields and fragment spreads to lock in desired behavior.
Would you like me to add tests asserting:
isFieldRequested(info, 'sizeRootFs')is true forsize: sizeRootFs- positive/negative cases for inline fragments and fragment spreads including/excluding
sizeRootFs?api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (1)
154-163: Add one more case: skipCache=true with size=true.Covers the matrix of flags to prevent regressions.
+ it('should pass skipCache=true alongside size=true', async () => { + vi.mocked(GraphQLFieldHelper.isFieldRequested).mockReturnValue(true); + vi.mocked(dockerService.getContainers).mockResolvedValue([]); + const mockInfo = {} as any; + await resolver.containers(true, mockInfo); + expect(dockerService.getContainers).toHaveBeenCalledWith({ skipCache: true, size: true }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
api/dev/configs/api.json(1 hunks)api/generated-schema.graphql(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(7 hunks)api/src/unraid-api/utils/graphql-field-helper.spec.ts(1 hunks)api/src/unraid-api/utils/graphql-field-helper.ts(1 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 (6)
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
Files:
api/src/unraid-api/utils/graphql-field-helper.spec.tsapi/src/unraid-api/utils/graphql-field-helper.tsapi/src/unraid-api/graph/resolvers/docker/docker.model.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.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/utils/graphql-field-helper.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/utils/graphql-field-helper.spec.tsapi/src/unraid-api/utils/graphql-field-helper.tsapi/src/unraid-api/graph/resolvers/docker/docker.model.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/utils/graphql-field-helper.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/utils/graphql-field-helper.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/utils/graphql-field-helper.spec.tsapi/src/unraid-api/utils/graphql-field-helper.tsapi/src/unraid-api/graph/resolvers/docker/docker.model.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.ts
🧠 Learnings (6)
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: Applies to {api,web}/**/*.{test,spec}.{ts,tsx} : Focus tests on behavior, not implementation details
Applied to files:
api/src/unraid-api/utils/graphql-field-helper.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/utils/graphql-field-helper.spec.ts
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: Applies to api/**/*.{test,spec}.{ts,tsx} : API test suite is Vitest; do not use Jest
Applied to files:
api/src/unraid-api/utils/graphql-field-helper.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/utils/graphql-field-helper.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.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: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
🧬 Code graph analysis (3)
api/src/unraid-api/utils/graphql-field-helper.spec.ts (1)
api/src/unraid-api/utils/graphql-field-helper.ts (1)
GraphQLFieldHelper(13-63)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (2)
api/src/unraid-api/utils/graphql-field-helper.ts (1)
GraphQLFieldHelper(13-63)api/src/unraid-api/cli/generated/graphql.ts (1)
DockerContainer(698-718)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (2)
Docker(679-686)DockerContainer(698-718)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
containers(45-51)
🔇 Additional comments (5)
api/dev/configs/api.json (1)
2-2: Version bump looks good.No issues found with the 4.22.2 update.
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
195-200: Good: invalidating both container caches.The Promise.all deletion and clear log help avoid stale size/non‑size results.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (2)
52-54: Reset mocks withbeforeEachis good.Ensures isolation across tests.
93-101: Good behavioral test for size=false path.Mocks
isFieldRequestedand asserts service call params precisely.api/src/unraid-api/utils/graphql-field-helper.ts (1)
18-31: Alias handling: add AST fallback to isFieldRequestedgraphql-fields maps requested keys to aliases, so a query like
size: sizeRootFswon't match'sizeRootFs'— add an AST-based fallback that scans info.fieldNodes for the original field name.File: api/src/unraid-api/utils/graphql-field-helper.ts (lines 18-31)
- static isFieldRequested(info: GraphQLResolveInfo, fieldPath: string): boolean { + static isFieldRequested(info: GraphQLResolveInfo, fieldPath: string): boolean { const fields = this.getRequestedFields(info); const pathParts = fieldPath.split('.'); let current: RequestedFields | {} = fields; for (const part of pathParts) { if (!(part in current)) { - return false; + // Fallback: scan AST for original field name (handles aliases) + const hasByAst = info.fieldNodes?.some((node) => { + const stack = [node.selectionSet?.selections ?? []]; + while (stack.length) { + for (const sel of stack.pop() as any[]) { + if (sel.kind === 'Field') { + if (sel.name?.value === part) return true; + if (sel.selectionSet) stack.push(sel.selectionSet.selections); + } else if (sel.selectionSet) { + stack.push(sel.selectionSet.selections); + } + } + } + return false; + }); + if (!hasByAst) return false; + continue; } current = current[part as keyof typeof current] as RequestedFields | {}; } return true; }Add unit test: query requests
size: sizeRootFsand assert isFieldRequested(..., 'sizeRootFs') === true.
| """Total size of all files in the container (in bytes)""" | ||
| sizeRootFs: BigInt |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Schema change to BigInt matches the model.
Alignment with the model is correct. Ensure client codegen maps BigInt consistently (string or bigint) to prevent runtime mismatches.
Run this to sanity-check mappings and usages:
🏁 Script executed:
#!/bin/bash
# Locate codegen config and BigInt scalar mappings/usages
fd -a -H 'codegen*' .
rg -nP --glob '!**/node_modules/**' 'BigInt' -C2
rg -nP --glob '!**/node_modules/**' 'sizeRootFs' -C2Length of output: 430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Listing codegen files (relative paths):"
fd -a -H 'codegen.ts' || true
echo
echo "----- Show head of api/codegen.ts -----"
sed -n '1,300p' api/codegen.ts || true
echo
echo "----- Show head of packages/unraid-api-plugin-connect/codegen.ts -----"
sed -n '1,300p' packages/unraid-api-plugin-connect/codegen.ts || true
echo
echo "----- Show head of web/codegen.ts -----"
sed -n '1,300p' web/codegen.ts || true
echo
echo "----- Repo-wide search for 'BigInt' (including ignored files) -----"
rg -nP -uu 'BigInt' -C3 || true
echo
echo "----- Repo-wide search for 'sizeRootFs' (including ignored files) -----"
rg -nP -uu 'sizeRootFs' -C3 || true
echo
echo "----- Search for GraphQL scalar mappings (scalars:) -----"
rg -nP -uu "scalars\s*:" -C3 || true
echo
echo "----- Search for explicit 'scalar BigInt' GraphQL definition -----"
rg -nP -uu "scalar\s+BigInt" -C3 || true
echo
echo "----- Search for BigInt mapping patterns (e.g. BigInt: 'string' or BigInt: 'bigint') -----"
rg -nP -uu "BigInt\s*:\s*(['\"][a-zA-Z_]+['\"]|[a-zA-Z_]+)" -C3 || trueLength of output: 61970
Add explicit BigInt scalar mapping and regenerate client types
- Schema change to BigInt matches server use of GraphQLBigInt.
- Problem: codegen configs do not map BigInt — update needed in api/codegen.ts, packages/unraid-api-plugin-connect/codegen.ts, web/codegen.ts. Current generated outputs show BigInt defaulting to any or (in some client files) remaining as Int (see api/src/unraid-api/cli/generated/graphql.ts, web/composables/gql/graphql.ts, web/src/composables/gql/graphql.ts).
- Action: add a BigInt scalar mapping (choose "string" or "bigint") and scalarSchemas where applicable, run graphql-codegen to regenerate client types, and verify/adjust server model TS property types to match the chosen runtime representation (e.g., api/src/unraid-api/graph/resolvers/*/— memory.model.ts, docker.model.ts, array.model.ts).
🤖 Prompt for AI Agents
In api/generated-schema.graphql around lines 1096-1097, BigInt scalar is added
but codegen configs lack an explicit mapping which causes generated clients to
default to any/Int; update api/codegen.ts,
packages/unraid-api-plugin-connect/codegen.ts, and web/codegen.ts to add a
BigInt scalar mapping (pick either "string" or "bigint") and include
scalarSchemas where required, then run graphql-codegen to regenerate client
types (verify api/src/unraid-api/cli/generated/graphql.ts,
web/composables/gql/graphql.ts, web/src/composables/gql/graphql.ts reflect the
mapping), and finally adjust server model TypeScript property types to match the
chosen runtime representation in files like
api/src/unraid-api/graph/resolvers/*/— memory.model.ts, docker.model.ts,
array.model.ts before committing.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/unraid-api-plugin-connect/codegen.ts (1)
28-29: Harden Zod schema for BigInt inputsIf any BigInt is ever accepted as input, add a safe‑integer guard to avoid silently truncating large values on the way in.
- BigInt: 'z.number()', + BigInt: 'z.number().int().refine(Number.isSafeInteger, "BigInt exceeds JS safe integer range")',api/codegen.ts (1)
28-29: Strengthen input validation for BigIntAdd a safe‑integer check in the Zod schema to prevent unsafe numbers from passing validation.
- BigInt: 'z.number()', + BigInt: 'z.number().int().refine(Number.isSafeInteger, "BigInt exceeds JS safe integer range")',web/src/composables/gql/graphql.ts (2)
244-246: ArrayDisk.isSpinning is nullable while Disk.isSpinning is non‑nullInconsistent nullability may complicate UI logic. Consider aligning both (either both nullable or both non‑null with a defined fallback), or document why array view can be null while disk view cannot.
707-709: Booleans overlap with UpdateStatusisUpdateAvailable/isRebuildReady duplicate info represented by UpdateStatus. Prefer deriving UI booleans from the enum or mark these as convenience fields and document precedence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**packages/unraid-api-plugin-connect/src/graphql/generated/client/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
api/codegen.ts(1 hunks)package.json(0 hunks)packages/unraid-api-plugin-connect/codegen.ts(1 hunks)web/codegen.ts(1 hunks)web/src/composables/gql/graphql.ts(8 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/codegen.tspackages/unraid-api-plugin-connect/codegen.tsweb/codegen.tsweb/src/composables/gql/graphql.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/codegen.ts
🧠 Learnings (1)
📚 Learning: 2025-03-12T13:35:43.900Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/codegen.ts:14-14
Timestamp: 2025-03-12T13:35:43.900Z
Learning: The JSON scalar type in web/codegen.ts was temporarily changed from 'string' to 'any' for compatibility with JsonForms integration. This change facilitates the implementation of the Connect settings web component.
Applied to files:
api/codegen.tspackages/unraid-api-plugin-connect/codegen.tsweb/codegen.tsweb/src/composables/gql/graphql.ts
🧬 Code graph analysis (1)
web/src/composables/gql/graphql.ts (2)
packages/unraid-api-plugin-connect/src/graphql/generated/client/graphql.ts (1)
Scalars(11-31)api/src/unraid-api/cli/generated/graphql.ts (2)
Scalars(11-75)ExplicitStatusItem(780-784)
🔇 Additional comments (8)
web/src/composables/gql/graphql.ts (5)
780-785: ExplicitStatusItem type addition—LGTMType looks consistent and minimal. No issues spotted.
2277-2283: UpdateStatus enum—LGTMEnum values cover known states and match new fields. Ensure unknown states map cleanly in UI.
612-614: Disk.isSpinning non‑null — no change requiredArrayDisk.isSpinning is nullable, but disks.service defaults null to false (api/src/unraid-api/graph/resolvers/disks/disks.service.ts:132) and the Disk model/resolver declare a non-null boolean (api/src/unraid-api/graph/resolvers/disks/disks.model.ts:150; api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts:38), so keeping Disk.isSpinning as Boolean! is correct.
1241-1241: Confirm auth and rate‑limiting for refreshDockerDigests
- Server mutation exists: api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts — returns this.dockerManifestService.refreshDigests().
- Schema/client entries: api/generated-schema.graphql (refreshDockerDigests: Boolean!) and api/src/unraid-api/cli/generated/graphql.ts (refreshDockerDigests: Scalars['Boolean']['output']).
- Action: ensure the resolver enforces authentication/authorization, verify refreshDigests() is idempotent, and add throttling/rate‑limiting if it makes external network/registry calls.
18-18: BigInt scalar generated as number — verify precision-sensitive fieldsweb/src/composables/gql/graphql.ts line 18:
BigInt: { input: number; output: number; }Several fields use Scalars['BigInt']['output'] (e.g., fsSize, size, numReads/numWrites, total). If any can exceed Number.MAX_SAFE_INTEGER (9,007,199,254,740,992) switch the codegen scalar to bigint or string (or serialize as string from the server).
api/codegen.ts (1)
20-21: Server codegen maps BigInt → number — sanity‑check rangesEvidence: api/codegen.ts maps BigInt → 'number' (scalarSchemas: BigInt: 'z.number()'); client types at api/src/unraid-api/cli/generated/graphql.ts also use number; generated-schema.graphql exposes many BigInt fields (e.g., size, fsSize/fsUsed, sizeRootFs, numReads/numWrites/numErrors, memory totals, swap*, etc.). Action: confirm none of these can exceed Number.MAX_SAFE_INTEGER (9,007,199,254,740,991) in realistic deployments; if they can, change the codegen mapping to string|bigint and update validators/codegen accordingly.
web/codegen.ts (1)
20-20: BigInt → number is unsafe — confirm bounds or change mappingMapping GraphQL BigInt to TS number loses precision above Number.MAX_SAFE_INTEGER (~9.007e15).
- Location to fix: web/codegen.ts:20 — currently
BigInt: 'number',.- Generated usages (representative): web/src/composables/gql/graphql.ts — fsFree/fsSize/fsUsed/size, numErrors/numReads/numWrites, sizeRootFs, memory (active/available/buffcache/free/total/used), swapFree/swapTotal/swapUsed, etc. (occurrences around lines 17, 234, 248, 256, 715, 1148, 1162–1183, 2021, 2032).
- Action (pick one): confirm all these fields will always be <= 9.007e15, or change the codegen scalar mapping to
string(recommended) orbigintand add proper (de)serialization and consumer updates.packages/unraid-api-plugin-connect/codegen.ts (1)
20-21: Plugin client: BigInt → number mapping — confirm safe-integer envelopeBigInt is mapped to number in packages/unraid-api-plugin-connect/codegen.ts (scalars.BigInt = 'number', scalarSchemas.BigInt = 'z.number()'); same mapping appears in api/codegen.ts and web/codegen.ts. Verify all produced BigInt values are ≤ Number.MAX_SAFE_INTEGER (9,007,199,254,740,991). If not, change the mapping to 'string' or 'bigint' and use an appropriate Zod schema (e.g., z.bigint() or z.string().transform(s => BigInt(s))).
|
|
||
| export type Docker = Node & { | ||
| __typename?: 'Docker'; | ||
| containerUpdateStatuses: Array<ExplicitStatusItem>; |
There was a problem hiding this comment.
🧩 Analysis chain
New Docker.containerUpdateStatuses: ensure resolver coverage and caching
Confirm the resolver exists and is efficiently cached/derived to avoid N+1 queries on large container sets.
🏁 Script executed:
#!/bin/bash
rg -n -C3 "containerUpdateStatuses" apiLength of output: 1701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo root =="
pwd
echo
echo "== Show resolver file (first 240 lines) =="
sed -n '1,240p' api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts || true
echo
echo "== Find occurrences of getContainerUpdateStatuses =="
rg -n "getContainerUpdateStatuses" || true
echo
echo "== Show files that define/mention getContainerUpdateStatuses (full file up to 400 lines) =="
for f in $(rg -n --files "getContainerUpdateStatuses" || true); do
echo "---- $f ----"
sed -n '1,400p' "$f" || true
echo
done
echo "== Find dockerPhpService usages/definition =="
rg -n "dockerPhpService" || true
rg -n "class .*DockerPhpService|interface .*DockerPhpService" -S || true
echo
echo "== Search for dataloader/cache usage or batching patterns related to Docker resolvers =="
rg -n "DataLoader|dataloader|batch|batchLoad|cache|caching|memoize" -S || true
echo
echo "== Show generated graphql types around Docker (to confirm schema) =="
sed -n '640,720p' api/src/unraid-api/cli/generated/graphql.ts || trueLength of output: 82683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-php.service.ts (1-400) =="
sed -n '1,400p' api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts || true
echo
echo "== docker-container.resolver.ts (1-400) =="
sed -n '1,400p' api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts || true
echo
echo "== docker-update-status.model.ts (1-240) =="
sed -n '1,240p' api/src/unraid-api/graph/resolvers/docker/docker-update-status.model.ts || true
echo
echo "== grep isUpdateAvailable usages =="
rg -n "isUpdateAvailable" -S || true
echo
echo "== grep containerUpdateStatuses usages =="
rg -n "containerUpdateStatuses" -S || trueLength of output: 9047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-manifest.service.ts (1-400) =="
sed -n '1,400p' api/src/unraid-api/graph/resolvers/docker/docker-manifest.service.ts || true
echoLength of output: 2763
Docker.containerUpdateStatuses — resolver present; N+1 risk needs fixing
- Verified: containerUpdateStatuses() is implemented at api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts and returns DockerPhpService.getContainerUpdateStatuses() (single phpLoader call) — OK.
- Problem: per-container ResolveField isUpdateAvailable (api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts) calls DockerManifestService.isUpdateAvailableCached(image) which calls DockerPhpService.readCachedUpdateStatus() on each container (api/src/unraid-api/graph/resolvers/docker/docker-php.service.ts) → causes N+1 file reads/parses/php calls for large container lists.
- Action required: batch or cache the cached-status read once per request (e.g., memoize readCachedUpdateStatus in DockerManifestService with short TTL or request-scoped cache, use DataLoader/batching, or prefetch containerUpdateStatuses and derive per-container isUpdateAvailable from that map). Change points: docker-container.resolver.ts (isUpdateAvailable), docker-manifest.service.ts (isUpdateAvailableCached), docker-php.service.ts (readCachedUpdateStatus).
🤖 Prompt for AI Agents
web/src/composables/gql/graphql.ts around line 681: the resolver chain causes
N+1 reads because docker-container.resolver.ts calls
DockerManifestService.isUpdateAvailableCached(image) which invokes
DockerPhpService.readCachedUpdateStatus() per container; fix by making the
cached-status read happen once per request and serving per-container checks from
that batch/cache — implement one of: (a) add a request-scoped cache or short-TTL
memoization in DockerManifestService.isUpdateAvailableCached so repeated calls
return the same in-memory result for the request; (b) add a bulk
readCachedUpdateStatuses() in DockerPhpService that reads/parses the file once
and returns a map, and change DockerManifestService to call it once and store
the map; or (c) use a DataLoader or prefetch containerUpdateStatuses in the
parent resolver and have docker-container.resolver.ts derive isUpdateAvailable
from the precomputed map; apply corresponding changes to
docker-container.resolver.ts to read from the map/cache instead of calling
readCachedUpdateStatus per container, and ensure thread-safety and short TTL or
request scoping to keep data fresh.
| /** Total size of all files in the container (in bytes) */ | ||
| sizeRootFs?: Maybe<Scalars['BigInt']['output']>; | ||
| state: ContainerState; |
There was a problem hiding this comment.
🧩 Analysis chain
sizeRootFs now BigInt and optional—verify it's not requested by default
This aligns with the PR goal (“no sizeRootFs unless queried”). Please verify no default queries still select sizeRootFs.
🏁 Script executed:
#!/bin/bash
rg -n -C2 "sizeRootFs" web packagesLength of output: 776
sizeRootFs inconsistent in generated typings — remove stale file & re-run codegen
rg found sizeRootFs only in generated GraphQL typings:
- web/src/composables/gql/graphql.ts:715 — sizeRootFs?: Maybe<Scalars['BigInt']['output']>;
- web/composables/gql/graphql.ts:708 — sizeRootFs?: Maybe<Scalars['Int']['output'].
No other matches. Delete or update the stale file (web/composables/gql/graphql.ts) and re-run the codegen so only the BigInt optional variant remains; verify no default queries request sizeRootFs.
🤖 Prompt for AI Agents
In web/src/composables/gql/graphql.ts around lines 714-716 there is a
BigInt-typed sizeRootFs but a stale duplicate file exists at
web/composables/gql/graphql.ts with an Int-typed sizeRootFs; remove the stale
file (web/composables/gql/graphql.ts), re-run your GraphQL codegen to regenerate
typings so only the BigInt optional sizeRootFs remains in the generated output,
and then search the codebase for any GraphQL queries or fragments that still
request sizeRootFs and update them to match the schema (or remove those fields)
before committing.
🤖 I have created a release *beep* *boop* --- ## [4.23.0](v4.22.2...v4.23.0) (2025-09-16) ### Features * add unraid api status manager ([#1708](#1708)) ([1d9ce0a](1d9ce0a)) ### Bug Fixes * **logging:** remove colorized logs ([#1705](#1705)) ([1d2c670](1d2c670)) * no sizeRootFs unless queried ([#1710](#1710)) ([9714b21](9714b21)) * use virtual-modal-container ([#1709](#1709)) ([44b4d77](44b4d77)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit