feat: replace docker overview table with web component #1764
Conversation
BREAKING CHANGE: ResolvedOrganizerView -> root is replaced by rootId + flatEntries. `root` resolved a tree representation of an organizer, but this required clients to defined and use their own tree operations, thus increasing client complexity. Instead, `flatEntries` is more suitable for client operations and only requires an initial mapping step on the client for efficiency.
|
@coderabbitai pause |
WalkthroughAdds Docker template scanning and icon extraction, enriches containers with templatePath, adds pause/unpause, introduces a flattened organizer model (rootId + flatEntries) with folder operations, implements notification deduplication/warnings stream, provides a JSON‑Forms container overview, and adds many frontend components/composables and GraphQL surface changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Frontend UI
participant Resolver as DockerResolver
participant DockerSvc as DockerService
participant Scanner as DockerTemplateScannerService
participant Config as DockerConfigService
participant Organizer as DockerOrganizerService
participant Notifications as NotificationsService
Note over Scanner,Config: Template scanning lifecycle
Scanner->>Config: getConfig()
Config-->>Scanner: templateMappings / skip paths
Scanner->>Scanner: scanTemplates() (read XML, parse, match)
Scanner->>Config: updateMappings(...)
Config-->>Scanner: ack
Note over User,UI: User opens containers view
User->>UI: request GetDockerContainers(skipCache)
UI->>Resolver: GraphQL query
Resolver->>DockerSvc: getContainers(skipCache)
DockerSvc->>Scanner: syncMissingContainers()
alt scanner triggers scan
Scanner->>Config: scanTemplates()
end
DockerSvc->>Config: getConfig()
Config-->>DockerSvc: templateMappings
DockerSvc-->>Resolver: containers (enriched with templatePath)
Resolver->>Organizer: resolveOrganizer(skipCache, iconMap)
Organizer-->>Resolver: ResolvedOrganizerV1 (rootId + flatEntries)
Resolver-->>UI: GraphQL response
UI-->>User: render tree table and overview
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { | ||
| forwardRef, | ||
| Inject, | ||
| Injectable, | ||
| Logger, | ||
| OnApplicationBootstrap, | ||
| OnModuleInit, | ||
| } from '@nestjs/common'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
The best way to fix the unused import is to simply remove OnModuleInit from the import { ... } statement at the top of the file (api/src/unraid-api/graph/resolvers/docker/docker.service.ts). Only OnApplicationBootstrap is actually used by the class, so removing the unused import will improve clarity and maintainability. No other changes are needed, as there is no use of OnModuleInit anywhere else in the snippet.
| @@ -5,7 +5,6 @@ | ||
| Injectable, | ||
| Logger, | ||
| OnApplicationBootstrap, | ||
| OnModuleInit, | ||
| } from '@nestjs/common'; | ||
| import { readFile } from 'fs/promises'; | ||
|
|
| await this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY); | ||
| this.logger.debug(`Invalidated container cache after pausing ${id}`); | ||
|
|
||
| let containers = await this.getContainers({ skipCache: true }); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To remedy the issue, we should remove the initial useless assignment to containers on line 289. The variable should still be declared (with let), but not assigned a value until inside the loop, to make clear that the assignment within the loop is the first usage. Thus, change let containers = await this.getContainers({ skipCache: true }); to let containers;, with the assignment happening on line 293 in the loop. No additional imports, methods, or other changes are needed—simply adjust the variable declaration as described.
| @@ -286,7 +286,7 @@ | ||
| await this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY); | ||
| this.logger.debug(`Invalidated container cache after pausing ${id}`); | ||
|
|
||
| let containers = await this.getContainers({ skipCache: true }); | ||
| let containers; | ||
| let updatedContainer: DockerContainer | undefined; | ||
| for (let i = 0; i < 5; i++) { | ||
| await sleep(500); |
| await this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY); | ||
| this.logger.debug(`Invalidated container cache after unpausing ${id}`); | ||
|
|
||
| let containers = await this.getContainers({ skipCache: true }); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix this issue, you should remove the unnecessary assignment to containers on line 317 in the unpause method. Since its value is immediately overwritten in every loop iteration, including the first, omitting the initial assignment does not change any functional behavior. You only need to declare the variable (without initialization), so the for loop can assign and use it. Specifically, change line 317 in docker.service.ts from let containers = await this.getContainers({ skipCache: true }); to just let containers;. No additional imports or method definitions are needed for this change.
| @@ -314,7 +314,7 @@ | ||
| await this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY); | ||
| this.logger.debug(`Invalidated container cache after unpausing ${id}`); | ||
|
|
||
| let containers = await this.getContainers({ skipCache: true }); | ||
| let containers; | ||
| let updatedContainer: DockerContainer | undefined; | ||
| for (let i = 0; i < 5; i++) { | ||
| await sleep(500); |
| * Combines moveEntriesToFolder with position-based insertion. | ||
| */ | ||
| export function moveItemsToPosition(params: MoveItemsToPositionParams): OrganizerView { | ||
| const { view, sourceEntryIds, destinationFolderId, position, resources } = params; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
The best way to fix this issue is to remove resources from the destructured assignment in the function signature of moveItemsToPosition at line 661. This change should be made only in the destructuring, so that the interface MoveItemsToPositionParams remains unchanged (in case it is useful elsewhere), and only the function itself is corrected. No import or additional code changes are needed.
| @@ -658,7 +658,7 @@ | ||
| * Combines moveEntriesToFolder with position-based insertion. | ||
| */ | ||
| export function moveItemsToPosition(params: MoveItemsToPositionParams): OrganizerView { | ||
| const { view, sourceEntryIds, destinationFolderId, position, resources } = params; | ||
| const { view, sourceEntryIds, destinationFolderId, position } = params; | ||
|
|
||
| const movedView = moveEntriesToFolder({ view, sourceEntryIds, destinationFolderId }); | ||
|
|
| * Combines createFolder + moveItems + positioning in a single atomic operation. | ||
| */ | ||
| export function createFolderWithItems(params: CreateFolderWithItemsParams): OrganizerView { | ||
| const { view, folderId, folderName, parentId, sourceEntryIds = [], position, resources } = params; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
The best way to fix the problem is to remove the unused resources local variable from the destructuring assignment on line 721. This means editing the destructuring pattern:
const { view, folderId, folderName, parentId, sourceEntryIds = [], position, resources } = params;to remove resources:
const { view, folderId, folderName, parentId, sourceEntryIds = [], position } = params;This change is confined to line 721 of api/src/unraid-api/organizer/organizer.ts. No other changes (such as modifying function signatures, interfaces, or logic) are required, since resources is not used anywhere else within this provided code region.
| @@ -718,7 +718,7 @@ | ||
| * Combines createFolder + moveItems + positioning in a single atomic operation. | ||
| */ | ||
| export function createFolderWithItems(params: CreateFolderWithItemsParams): OrganizerView { | ||
| const { view, folderId, folderName, parentId, sourceEntryIds = [], position, resources } = params; | ||
| const { view, folderId, folderName, parentId, sourceEntryIds = [], position } = params; | ||
|
|
||
| let newView = createFolderInView({ | ||
| view, |
✅ Actions performedReviews paused. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
845-851: Log the original timestamp string (not the null date object).Current log prints "null" instead of the bad input.
Apply this diff:
- if (!date) { - this.logger.warn(`[formatTimestamp] Could not parse date from timestamp: ${date}`); - return timestamp; - } + if (!date) { + this.logger.warn( + `[formatTimestamp] Could not parse date from timestamp: ${timestamp}` + ); + return timestamp; + }api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts (1)
29-37: Normalize resource IDs to match UI/organizer expectations.Docker container names include a leading "/" from the Docker API. The UI strips this for display (as seen in docker-form.service.ts line 19), but containerToResource doesn't normalize it, causing ID mismatches between organizer resources and UI rows.
export function containerToResource(container: DockerContainer): OrganizerContainerResource { - const stableRef = container.names[0] || container.image; + const stableRef = + (container.names?.[0]?.replace(/^\//, '') || + container.image || + container.id); return { id: stableRef, type: 'container', name: stableRef, meta: container, }; }This normalizes IDs to match the slash-less format already used elsewhere in the codebase (docker-form.service.ts, docker-template-scanner.service.ts) and adds container.id as a stable fallback. Verify DnD/move flows work correctly after applying this change.
🧹 Nitpick comments (50)
api/src/unraid-api/organizer/organizer.test.ts (1)
267-300: Consider adding assertions to verify resource entries and nested folder contents remain unchanged.The test correctly validates that resources already referenced in nested folders are not duplicated in the root. To make it more comprehensive, consider also asserting that:
- The ref entries for
resourceAandresourceBstill exist in the result- The
stufffolder's children remain['resourceA', 'resourceB']This would guard against accidental removal or modification of existing entries.
Example additional assertions:
const rootChildren = (result.entries['root1'] as OrganizerFolder).children; expect(rootChildren).toEqual(['stuff']); + +// Verify resource entries still exist +expect(result.entries['resourceA']).toBeDefined(); +expect(result.entries['resourceB']).toBeDefined(); + +// Verify nested folder contents unchanged +const stuffChildren = (result.entries['stuff'] as OrganizerFolder).children; +expect(stuffChildren).toEqual(['resourceA', 'resourceB']);api/src/unraid-api/organizer/organizer.model.ts (1)
260-310: Add boolean validation for hasChildrenStrengthen validation with IsBoolean on hasChildren.
Apply:
@@ -import { - Equals, - IsArray, - IsIn, - IsNumber, - IsObject, - IsOptional, - IsString, - ValidateNested, -} from 'class-validator'; +import { + Equals, + IsArray, + IsBoolean, + IsIn, + IsNumber, + IsObject, + IsOptional, + IsString, + ValidateNested, +} from 'class-validator'; @@ @Field() - hasChildren!: boolean; + @IsBoolean() + hasChildren!: boolean;web/src/components/Wrapper/mount-engine.ts (2)
39-61: Cache portal selector to avoid repeated DOM/style workMemoize ensurePortalRoot’s result; subsequent calls can return the cached selector.
+let cachedPortalSelector: string | undefined; function ensurePortalRoot(): string | undefined { - if (typeof document === 'undefined') { + if (cachedPortalSelector) { + return cachedPortalSelector; + } + if (typeof document === 'undefined') { return undefined; } @@ - return `#${PORTAL_ROOT_ID}`; + cachedPortalSelector = `#${PORTAL_ROOT_ID}`; + return cachedPortalSelector; }
14-15: ESM import extension for internal modulesPer web TypeScript ESM guideline, include .js on internal specifiers.
-import { ensureUnapiScope, ensureUnapiScopeForSelectors, observeUnapiScope } from '~/utils/unapiScope'; +import { ensureUnapiScope, ensureUnapiScopeForSelectors, observeUnapiScope } from '~/utils/unapiScope.js';web/src/composables/useTreeData.ts (3)
41-52: Eliminate unsafe casts by typingflatEntrieswith meta generics.Avoid
as Tby typing entries soentry.metais alreadyT. This aligns with “avoid type casting” guidance. Minimal change:Apply:
@@ -import type { FlatOrganizerEntry } from '@/composables/gql/graphql'; +import type { FlatOrganizerEntry } from '@/composables/gql/graphql'; + +type FlatEntryWithMeta<M> = Omit<FlatOrganizerEntry, 'meta'> & { meta?: M | null }; @@ -export interface TreeDataOptions<T> { - flatEntries?: MaybeRef<FlatOrganizerEntry[] | undefined>; +export interface TreeDataOptions<T> { + flatEntries?: MaybeRef<FlatEntryWithMeta<T>[] | undefined>; @@ - const builtFromMeta = - entry.meta && buildFlatRow ? buildFlatRow(entry.meta as T, entry.name) : null; + const builtFromMeta = + entry.meta && buildFlatRow ? buildFlatRow(entry.meta, entry.name) : null; @@ - meta: (entry.meta as T) ?? builtFromMeta?.meta, + meta: entry.meta ?? builtFromMeta?.meta,This removes the casts and preserves behavior. As per coding guidelines.
Also applies to: 53-64
66-71: Guard against accidental cycles inchildrenIds.If backend data ever contains a cycle or self-reference, recursion will blow the stack. Add a visited set to short‑circuit repeats.
@@ - function buildTreeFromFlat(entry: FlatOrganizerEntry): TreeRow<T> { + const visited = new Set<string>(); + function buildTreeFromFlat(entry: FlatEntryWithMeta<T>): TreeRow<T> { + if (visited.has(entry.id)) { + return { id: entry.id, type: entry.type, name: entry.name, children: [], icon: entry.icon }; + } + visited.add(entry.id); @@ - row.children = entry.childrenIds + row.children = entry.childrenIds .map((childId) => entriesById.get(childId)) .filter(Boolean) .map((child) => buildTreeFromFlat(child!));
132-141: Avoid duplicate “flatten rows” logic across composables.
flattenRowshere andflattenSelectableRowsinuseRowSelectionare near‑duplicates. Extract a shared utility to prevent drift.web/src/utils/unapiScope.ts (2)
111-119: Clarify multi‑root behavior; new roots aren’t observed after first call.Subsequent calls add selectors (good) but early‑return prevents observing a different
root. Either document that only the first root is observed, or support multiple roots.Option A (doc): add JSDoc noting only initial root is observed. Option B (code): maintain a Map<root, observer> and observe each unique root.
137-146: Harden against invalid selectors.
node.matches(observerState.selectorList)andquerySelectorAllcan throw on malformed selectors. Wrap in try/catch to avoid breaking the observer loop.web/src/composables/useRowSelection.ts (1)
24-33: DRY: Share flattening logic withuseTreeData.
flattenSelectableRowsmirrorsflattenRows. Consider extracting a shared utility (e.g.,flattenTreeRows(rows, filterType?)) to keep one source of truth.web/src/composables/gql/graphql.ts (1)
22-22: Avoidanyfor GraphQL JSON scalar (update codegen mapping).Generated types use
anyfor JSON. Map JSON tounknown(or a precise Record) in codegen to align with our TS guidelines.Add to your codegen config (e.g., web/codegen.ts):
export default defineConfig({ generates: { 'web/src/composables/gql/': { preset: 'client', presetConfig: { /* ... */ }, + config: { + scalars: { + JSON: 'unknown', + }, + }, }, }, });As per coding guidelines.
web/package.json (1)
107-107: Pin version for reproducible builds.Use an exact version for
@tanstack/vue-tableto avoid surprise upgrades in CI.- "@tanstack/vue-table": "^8.21.3", + "@tanstack/vue-table": "8.21.3",api/package.json (1)
107-107: Pinfast-xml-parserand verify safe parser options.
- Pin the version to reduce supply-chain drift.
- Ensure parser options disable dangerous entity expansion.
- "fast-xml-parser": "^5.3.0", + "fast-xml-parser": "5.3.0",Please confirm you initialize XMLParser with conservative options (e.g., ignoreAttributes as needed, no external entity expansion).
web/src/assets/main.css (1)
12-12: Import Nuxt UI into a cascade layer to minimize bleed.Anchor Nuxt UI under
components(keeps cascade predictable with Tailwind v4 layers).-@import "@nuxt/ui"; +@import "@nuxt/ui" layer(components);Please verify no unintended style overrides in the legacy WebGUI (spot-check buttons/links/modals).
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
68-77: Nice wiring of new providers in tests; consider DRY setup.Repeated provider stubs can live in a small factory to reduce duplication across specs.
api/src/environment.ts (1)
114-117: Harden PATHS_DOCKER_TEMPLATES parsing (trim/filter; safe fallback).split(',') can yield empty/whitespace paths, breaking scanners. Trim, filter falsy, and fall back to defaults if env resolves to empty.
-export const PATHS_DOCKER_TEMPLATES = process.env.PATHS_DOCKER_TEMPLATES?.split(',') ?? [ - '/boot/config/plugins/dockerMan/templates-user', - '/boot/config/plugins/dockerMan/templates', -]; +const DEFAULT_DOCKER_TEMPLATE_PATHS = [ + '/boot/config/plugins/dockerMan/templates-user', + '/boot/config/plugins/dockerMan/templates', +] as const; + +const envPathsRaw = process.env.PATHS_DOCKER_TEMPLATES; +export const PATHS_DOCKER_TEMPLATES: readonly string[] = + envPathsRaw + ? envPathsRaw + .split(',') + .map((p) => p.trim()) + .filter(Boolean) + : DEFAULT_DOCKER_TEMPLATE_PATHS; + +// If provided env resolves to empty, fall back to defaults +export const _PATHS_DOCKER_TEMPLATES = + PATHS_DOCKER_TEMPLATES.length > 0 ? PATHS_DOCKER_TEMPLATES : DEFAULT_DOCKER_TEMPLATE_PATHS;If consumers import PATHS_DOCKER_TEMPLATES directly, swap usages to _PATHS_DOCKER_TEMPLATES.
web/src/components/Docker/docker-create-folder.mutation.ts (1)
1-25: Mutation doc looks good; consider fragments and typed DocumentNode.Location and naming conform. To reduce duplication across Docker mutations, extract a fragment for flatEntries and export a typed DocumentNode for better TS inference.
As per coding guidelines.
api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.ts (2)
292-357: Solid coverage for dedupe and limit; add ordering assertion.Great tests. Consider asserting deterministic ordering (e.g., ALERTs before WARNINGs, or newest first) so UI expectations are locked and regressions are caught.
359-396: Good uniqueness tests; clarify scope across ARCHIVE vs UNREAD.Add a case: create UNREAD, archive it, then call notifyIfUnique with same payload and expect creation → this documents uniqueness being scoped to UNREAD only.
web/src/components/Docker/DockerContainerOverview.vue (1)
2-2: Consider removing explicitcomputedimport.Nuxt provides auto-imports for Vue composition API functions like
computed. While explicit imports aren't incorrect and may be required in test files, they're typically unnecessary in component files.Apply this diff if auto-imports are working in your setup:
-import { computed } from 'vue'; import { useQuery } from '@vue/apollo-composable';web/src/components/Docker/DockerSidebarTree.vue (2)
6-9: Narrow emit payload types to what you actually emit.You only emit for container rows; tighten typing to reflect that and avoid accidental folder handling later.
interface Emits { - (e: 'item:click', item: { id: string; type: string; name: string }): void; - (e: 'item:select', item: { id: string; type: string; name: string; selected: boolean }): void; + (e: 'item:click', item: { id: string; type: 'container'; name: string }): void; + (e: 'item:select', item: { id: string; type: 'container'; name: string; selected: boolean }): void; } function onRowClick(payload: { id: string; type: 'container' | 'folder'; name: string; containerId?: string; }) { if (payload.type === 'container') { - emit('item:click', { id: payload.id, type: payload.type, name: payload.name }); + emit('item:click', { id: payload.id, type: 'container', name: payload.name }); } } function onRowSelect(payload: { id: string; type: 'container' | 'folder'; name: string; containerId?: string; selected: boolean; }) { if (payload.type === 'container') { emit('item:select', { id: payload.id, - type: payload.type, + type: 'container', name: payload.name, selected: payload.selected, }); } }Also applies to: 31-40, 42-57
63-69: Use a consistent prop access style in the template.You mix props.* and bare prop keys. Pick one for readability; simplest is to use props.* everywhere here.
<DockerContainersTable - :containers="props.containers" - :flat-entries="props.flatEntries" - :root-folder-id="props.rootFolderId" + :containers="props.containers" + :flat-entries="props.flatEntries" + :root-folder-id="props.rootFolderId" compact - :active-id="activeId" - :selected-ids="selectedIds" + :active-id="props.activeId" + :selected-ids="props.selectedIds" :loading="props.disabled"web/src/components/Docker/docker-rename-folder.mutation.ts (1)
3-44: Right-size the selection and include __typename for better cache updates.Consider extracting a shared fragment for flatEntries and adding __typename fields. This reduces drift across mutations/queries and helps Apollo cache normalization.
export const RENAME_DOCKER_FOLDER = gql` mutation RenameDockerFolder($folderId: String!, $newName: String!) { renameDockerFolder(folderId: $folderId, newName: $newName) { + __typename version views { + __typename id name rootId flatEntries { + __typename id type name parentId depth position path hasChildren childrenIds meta { + __typename id names state status image ports { + __typename privatePort publicPort type } autoStart hostConfig { + __typename networkMode } created isUpdateAvailable isRebuildReady } } } } } `;If multiple documents repeat this shape, extract a FlatEntry fragment and spread it here to avoid duplication.
web/src/components/Notifications/CriticalNotifications.standalone.vue (1)
26-31: Avoid casting empty variables object when the query has no variables.If the query truly has no variables, prefer passing
undefinedand adjusting types/codegen so variables are optional to eliminate{} as ...casting.->(warningsAndAlerts, {} as WarningAndAlertNotificationsQueryVariables, { +>(warningsAndAlerts, undefined, { fetchPolicy: 'network-only', });As per coding guidelines.
api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.spec.ts (2)
12-15: Use a unique temp directory per test run to avoid collisions.Hard-coding
/tmp/test-templatesrisks cross-run interference. Use a unique path and feed it into the env mock.-vi.mock('@app/environment.js', () => ({ - PATHS_DOCKER_TEMPLATES: ['/tmp/test-templates'], +const TEST_TEMPLATES_DIR = `/tmp/test-templates-${process.pid}-${Date.now()}`; +vi.mock('@app/environment.js', () => ({ + PATHS_DOCKER_TEMPLATES: [TEST_TEMPLATES_DIR], ENABLE_NEXT_DOCKER_RELEASE: true, })); describe('DockerTemplateScannerService', () => { let service: DockerTemplateScannerService; let dockerConfigService: DockerConfigService; let dockerService: DockerService; - const testTemplateDir = '/tmp/test-templates'; + const testTemplateDir = TEST_TEMPLATES_DIR; beforeEach(async () => { await mkdir(testTemplateDir, { recursive: true });Also applies to: 21-25, 55-57
270-282: Avoid asserting on exact error wording unless that format is the subject.This assertion is brittle; prefer checking that an error exists or that an expected code/path was taken.
- expect(result.errors.length).toBeGreaterThan(0); - expect(result.errors[0]).toContain('Failed to get containers'); + expect(result.errors.length).toBeGreaterThan(0); + expect(result.errors[0]).toBeTruthy();As per coding guidelines.
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (1)
20-21: Wiring looks solid; consider exporting new services if needed externally.If other modules inject DockerTemplateScannerService or DockerTemplateIconService, re-export them from this module; otherwise this is fine as-is.
Also applies to: 24-24, 30-31
api/src/unraid-api/graph/resolvers/docker/docker-template-icon.service.ts (1)
38-56: Avoid unbounded parallel file reads; batch to cap concurrency.Large fleets can exhaust file descriptors. Batch reads to, e.g., 25 at a time.
Apply this refactor to getIconsForContainers:
- const results = await Promise.all(iconPromises); + const results: Array<{ id: string; icon: string } | null> = []; + const batchSize = 25; + for (let i = 0; i < iconPromises.length; i += batchSize) { + const batch = iconPromises.slice(i, i + batchSize); + results.push(...(await Promise.all(batch))); + }api/src/unraid-api/organizer/organizer.resolution.test.ts (1)
75-112: Reduce index-coupling in tests; prefer id-based selection.Use flatEntries.find(e => e.id === '...') over fixed indices to avoid brittleness if traversal order changes.
Also applies to: 232-251, 336-347, 390-410, 516-529
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (2)
115-124: Avoid enum re-indexing; pass the enum value directly.NotificationType[type] is redundant; type is already the enum value.
Apply this diff:
- const notification = await this.loadNotificationFile(path, NotificationType[type]); + const notification = await this.loadNotificationFile(path, type);
145-157: Nice: centralized snapshot publisher for warnings/alerts. Consider coalescing bursts.On heavy churn, debounce/throttle publishWarningsAndAlerts (e.g., 200–500 ms) to reduce redundant broadcasts.
api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts (1)
113-116: Improve error logging context/stack.Pass message + stack to Logger.error for clearer diagnostics.
Apply this diff:
- const errorMsg = `Failed to get containers: ${error instanceof Error ? error.message : 'Unknown error'}`; - this.logger.error(error, 'Failed to get containers'); + const errorMsg = `Failed to get containers: ${ + error instanceof Error ? error.message : 'Unknown error' + }`; + this.logger.error(errorMsg, (error as Error)?.stack); result.errors.push(errorMsg);web/src/composables/useContainerActions.ts (2)
53-55: Remove unnecessary casts; use TreeRow fields directly.TreeRow already includes containerId and state. Casting to inline shapes is avoidable and risks drift. Read row.containerId and row.state directly.
-const containerId = (row as { containerId?: string }).containerId || row.id; -const state = (row as { state?: string }).state; +const containerId = row.containerId ?? row.id; +const state = row.state;Also applies to: 68-70, 147-153, 167-173
77-109: Add error handling and user feedback for partial failures.Batches await each mutation but swallow errors. Provide onError callback (peer to onSuccess) or surface failed items; otherwise users get “Action completed” even when some ops fail.
-export interface ContainerActionOptions<T = unknown> { +export interface ContainerActionOptions<T = unknown> { ... onSuccess?: (message: string) => void; + onError?: (message: string, detail?: unknown) => void; } @@ try { await runStartStopBatch(toStart, toStop); onSuccess?.('Action completed'); } finally { + // optionally: catch and call onError }Also consider try/catch within each loop to collect failures and refetch once at end.
Also applies to: 111-143, 196-202, 229-235, 204-216, 237-249
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1)
185-192: Subscription shape looks good. Add brief description for SDL clarity.Optional: add { description } to @subscription decorator to document the stream.
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (2)
2-9: Remove unused OnModuleInit import.Imported but not used; trim to satisfy static analysis.
-import { - forwardRef, - Inject, - Injectable, - Logger, - OnApplicationBootstrap, - OnModuleInit, -} from '@nestjs/common'; +import { forwardRef, Inject, Injectable, Logger, OnApplicationBootstrap } from '@nestjs/common';
289-295: Drop redundant initial containers assignment before polling.The first assignment is overwritten inside the loop. Initialize updatedContainer directly.
- let containers = await this.getContainers({ skipCache: true }); - let updatedContainer: DockerContainer | undefined; + let updatedContainer: DockerContainer | undefined; for (let i = 0; i < 5; i++) { await sleep(500); - containers = await this.getContainers({ skipCache: true }); - updatedContainer = containers.find((c) => c.id === id); + const containers = await this.getContainers({ skipCache: true }); + updatedContainer = containers.find((c) => c.id === id);(Apply similarly to unpause.)
Also applies to: 317-323
web/src/components/Docker/DockerContainersTable.vue (1)
447-458: Avoid blocking window.prompt for folder creation UX.Inline prompt is jarring and inconsistent with rest of the UI modals. Prefer a small input modal within the existing "Move to folder" dialog or a dedicated modal.
I can provide a small UModal-based "New folder" prompt component if helpful.
web/src/composables/useDragDrop.ts (2)
51-57: Simplify selection logic at dragstart.The
isDraggingId(row.id)check is redundant at dragstart (draggingIds is set later). Clean up for clarity.- const ids = - selected.length && isDraggingId(row.id) - ? selected - : selected.length && rowSelection?.value?.[row.id] - ? selected - : [row.id]; + const ids = selected.length && rowSelection?.value?.[row.id] ? selected : [row.id];
73-82: Optional: guard RAF in non-DOM environments.In SSR unit tests without RAF, add a fallback to avoid ReferenceErrors.
- rafId = requestAnimationFrame(() => { + const raf = typeof requestAnimationFrame === 'function' ? requestAnimationFrame : (fn: FrameRequestCallback) => { + fn(performance?.now?.() ?? 0); + return 0 as unknown as number; + }; + rafId = raf(() => { const area = computeDropArea(e, element); dragOverState.value = { rowId: row.id, area }; rafId = null; });web/src/composables/useFolderOperations.ts (5)
3-3: ESM import specifier should end with .jsTypeScript files must import with .js extensions for ESM compatibility. Update the path.
-import type { FlatFolderRow } from '@/composables/useFolderTree'; +import type { FlatFolderRow } from '@/composables/useFolderTree.js';As per coding guidelines.
18-33: Replace explicit any in mutation options with a minimal typed shapeAvoid any. Model the subset you use (refetchQueries, awaitRefetchQueries).
deleteFolderMutation: ( args: { entryIds: string[] }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - options?: any + options?: { refetchQueries?: unknown[]; awaitRefetchQueries?: boolean } ) => Promise<unknown>; setFolderChildrenMutation: ( args: { folderId: string; childrenIds: string[] }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - options?: any + options?: { refetchQueries?: unknown[]; awaitRefetchQueries?: boolean } ) => Promise<unknown>; moveEntriesMutation: ( args: { destinationFolderId: string; sourceEntryIds: string[] }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - options?: any + options?: { refetchQueries?: unknown[]; awaitRefetchQueries?: boolean } ) => Promise<unknown>;As per coding guidelines.
147-163: Interactive rename should also call rename mutationMirror the non-interactive path; avoid create+delete.
- const parentId = parentById.value[id] || rootFolderId.value; - const children = folderChildrenIds.value[id] || []; - await createFolderMutation({ name: proposed, parentId, childrenIds: children }); - await setFolderChildrenMutation({ folderId: id, childrenIds: [] }); - await deleteFolderMutation( - { entryIds: [id] }, - { - refetchQueries: [refetchQuery], - awaitRefetchQueries: true, - } - ); + await options.renameFolderMutation?.( + { folderId: id, newName: proposed }, + { refetchQueries: [refetchQuery], awaitRefetchQueries: true } + ); onSuccess?.('Folder renamed');
74-87: Add basic error handling around move operationPrevents stuck UI when mutation fails; surfaces errors.
- await moveEntriesMutation( + try { + await moveEntriesMutation( { destinationFolderId: selectedFolderId.value, sourceEntryIds: ids }, { refetchQueries: [refetchQuery], awaitRefetchQueries: true, } ); onSuccess?.('Moved to folder'); close(); + } catch (err) { + console.error('confirmMove failed', err); + }
10-12: Remove unused visibleFolders from the public APIIt’s not used in this composable; trimming avoids confusion and churn for callers.
- visibleFolders: Ref<FlatFolderRow[]>;api/src/unraid-api/organizer/organizer.ts (3)
126-142: Avoid any for meta; use precise typemeta is a DockerContainer (via OrganizerContainerResource.meta). Type it explicitly.
- let meta: any = undefined; + let meta: OrganizerContainerResource['meta'] | undefined = undefined;As per coding guidelines.
648-654: Unused resources param in MoveItemsToPosition; either use or prefix to silenceIt’s not used and triggers CodeQL notices.
-export interface MoveItemsToPositionParams { +export interface MoveItemsToPositionParams { view: OrganizerView; sourceEntryIds: Set<string>; destinationFolderId: string; position: number; - resources?: OrganizerV1['resources']; + resources?: OrganizerV1['resources']; } @@ -export function moveItemsToPosition(params: MoveItemsToPositionParams): OrganizerView { - const { view, sourceEntryIds, destinationFolderId, position, resources } = params; +export function moveItemsToPosition(params: MoveItemsToPositionParams): OrganizerView { + const { view, sourceEntryIds, destinationFolderId, position, resources: _resources } = params;Alternatively, remove the field and update call sites.
Also applies to: 660-678
720-751: Materialize missing refs when creating a folder with itemsWhen sourceEntryIds include resources not yet in entries, ensure ref entries exist before move. Also resolves CodeQL “unused variable resources”.
export function createFolderWithItems(params: CreateFolderWithItemsParams): OrganizerView { const { view, folderId, folderName, parentId, sourceEntryIds = [], position, resources } = params; - let newView = createFolderInView({ + let newView = createFolderInView({ view, folderId, folderName, parentId, - childrenIds: sourceEntryIds, + childrenIds: [], }); - if (sourceEntryIds.length > 0) { - newView = moveEntriesToFolder({ - view: newView, - sourceEntryIds: new Set(sourceEntryIds), - destinationFolderId: folderId, - }); - } + if (sourceEntryIds.length > 0) { + // Ensure refs exist for resources (if provided) + if (resources) { + newView = setFolderChildrenInView({ + view: newView, + folderId, + childrenIds: sourceEntryIds, + resources, + }); + } + newView = moveEntriesToFolder({ + view: newView, + sourceEntryIds: new Set(sourceEntryIds), + destinationFolderId: folderId, + }); + }To keep API stable, callers already pass resources (see docker-organizer.service.ts). No other changes required.
web/src/components/Common/BaseTreeTable.vue (2)
458-472: Tighten row typing to avoid any in table callbacksLeverage TreeRow instead of any for get-row-id/sub-rows/can-select to preserve type safety.
- :get-row-id="(row: any) => row.id" - :get-sub-rows="(row: any) => row.children" - :get-row-can-select="(row: any) => row.original.type === selectableType" + :get-row-id="(row: TreeRow<T>) => row.id" + :get-sub-rows="(row: TreeRow<T>) => row.children" + :get-row-can-select="(row: { original: TreeRow<T> }) => row.original.type === selectableType"
224-240: Column-visibility on compact: also reset when expanding backCurrently only sets when compact=true; consider restoring defaults when compact=false to avoid stale hidden columns if props.columns change.
(isCompact) => { if (isCompact) { // existing code... columnVisibility.value = hideColumns; + } else { + columnVisibility.value = {}; } },Ensure this matches UTable’s column-visibility contract in @nuxt/ui.
| const dataSchema = this.createDataSchema(); | ||
| const uiSchema = this.createUiSchema(); | ||
|
|
||
| return { | ||
| id: 'docker-container-overview', | ||
| dataSchema: { | ||
| type: 'object', | ||
| properties: dataSchema, | ||
| }, | ||
| uiSchema: { | ||
| type: 'VerticalLayout', | ||
| elements: [uiSchema], | ||
| }, | ||
| data: tableData, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Data/schema mismatch and incorrect UI scope break JSON Forms rendering.
You return data as an array, but your schema defines an object with a containers array. Also, the UI control scope should target #/properties/containers. This will fail at runtime.
- const dataSchema = this.createDataSchema();
- const uiSchema = this.createUiSchema();
+ const dataSchema = this.createDataSchema();
+ const uiSchema = this.createUiSchema();
return {
id: 'docker-container-overview',
dataSchema: {
type: 'object',
properties: dataSchema,
},
uiSchema: {
type: 'VerticalLayout',
elements: [uiSchema],
},
- data: tableData,
+ data: {
+ containers: tableData,
+ },
};- private createUiSchema(): UISchemaElement {
+ private createUiSchema(): UISchemaElement {
return {
type: 'Control',
- scope: '#',
+ scope: '#/properties/containers',
options: {
variant: 'table',
},
};
}Additionally, the state enum is too narrow; Docker states can include PAUSED/RESTARTING/etc. Consider removing the enum or aligning with the full ContainerState union.
- state: {
- type: 'string',
- title: 'State',
- enum: ['RUNNING', 'EXITED'],
- },
+ state: {
+ type: 'string',
+ title: 'State',
+ },Also applies to: 55-99, 101-109
🏁 Script executed:
#!/bin/bash
# Verify no consumers assume `data` is an array.
rg -n -C2 "docker-container-overview|getContainerOverviewForm|dataSchema|uiSchema|containers" --type=ts --type=tsx --type=vueLength of output: 27
I'll fix the ripgrep command by removing the unrecognized file type and using glob patterns instead.
🏁 Script executed:
#!/bin/bash
# Verify no consumers assume `data` is an array
# Using glob patterns instead of unrecognized file type
rg -n -C3 "docker-container-overview|getContainerOverviewForm" -g '*.ts' -g '*.tsx' -g '*.vue'Length of output: 5681
🏁 Script executed:
#!/bin/bash
# Find the docker-form.service.ts file and examine createDataSchema/createUiSchema
fd "docker-form\.service\.ts" --type fLength of output: 64
🏁 Script executed:
#!/bin/bash
# Search for schema/data usage patterns in docker-related files
rg -n "tableData|containers.*array\|data.*containers" -g '*.ts' -g '*.vue' --type tsLength of output: 223
I need to examine the full implementation to verify the review comment's claims about the schema/data mismatch.
---
🏁 Script executed:
```shell
#!/bin/bash
# View the full docker-form.service.ts implementation
cat api/src/unraid-api/graph/resolvers/docker/docker-form.service.ts
Length of output: 3731
🏁 Script executed:
#!/bin/bash
# View DockerContainerOverview.vue to see how it consumes the form data
cat web/src/components/Docker/DockerContainerOverview.vueLength of output: 1960
🏁 Script executed:
#!/bin/bash
# Search for ContainerState type definition
rg -n "ContainerState|enum.*State|type.*State" -g '*.ts' api/src/unraid-api/graph/resolvers/docker/ -A2Length of output: 9768
🏁 Script executed:
#!/bin/bash
# Look for JSON Forms usage patterns in the codebase
rg -n "JSONForms|renderForm|uiSchema|dataSchema" -g '*.vue' web/src/components/ -B2 -A2Length of output: 4664
🏁 Script executed:
#!/bin/bash
# Check the exact type definition for DockerContainerOverviewForm
rg -n "DockerContainerOverviewForm|interface.*Form" -g '*.ts' api/src/unraid-api/graph/resolvers/docker/ -A10Length of output: 5602
Fix data/schema mismatch and incorrect UI scope in docker container overview form.
The data field returns an array, but the schema expects an object with a containers property—this will cause JSON Forms to fail. Additionally, the UI control scope must target the containers array.
- data: tableData,
+ data: {
+ containers: tableData,
+ },- private createUiSchema(): UISchemaElement {
+ private createUiSchema(): UISchemaElement {
return {
type: 'Control',
- scope: '#',
+ scope: '#/properties/containers',
options: {
variant: 'table',
},
};
}Additionally, the state enum is incomplete. The ContainerState enum includes PAUSED and RESTARTING, but the schema only allows RUNNING and EXITED. Update it to match:
- state: {
- type: 'string',
- title: 'State',
- enum: ['RUNNING', 'EXITED'],
- },
+ state: {
+ type: 'string',
+ title: 'State',
+ enum: ['RUNNING', 'PAUSED', 'RESTARTING', 'EXITED'],
+ },📝 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 dataSchema = this.createDataSchema(); | |
| const uiSchema = this.createUiSchema(); | |
| return { | |
| id: 'docker-container-overview', | |
| dataSchema: { | |
| type: 'object', | |
| properties: dataSchema, | |
| }, | |
| uiSchema: { | |
| type: 'VerticalLayout', | |
| elements: [uiSchema], | |
| }, | |
| data: tableData, | |
| }; | |
| } | |
| const dataSchema = this.createDataSchema(); | |
| const uiSchema = this.createUiSchema(); | |
| return { | |
| id: 'docker-container-overview', | |
| dataSchema: { | |
| type: 'object', | |
| properties: dataSchema, | |
| }, | |
| uiSchema: { | |
| type: 'VerticalLayout', | |
| elements: [uiSchema], | |
| }, | |
| data: { | |
| containers: tableData, | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker-form.service.ts around lines
38 to 53, the form returns an array in `data` while the schema expects an object
with a `containers` property, the UI control scope doesn't target the containers
array, and the state enum is missing PAUSED and RESTARTING; change the returned
data to an object like { containers: tableData }, update the dataSchema so
`properties.containers` is an array whose `items` are objects using the existing
`dataSchema` (i.e. containers: { type: 'array', items: { type: 'object',
properties: dataSchema } }), change the uiSchema control scope to point at the
containers array (target the containers array path used by JSON Forms), and
update the ContainerState enum/schema to include PAUSED and RESTARTING so the
schema enum matches the ContainerState values.
| private normalizeRepository(repository: string): string { | ||
| return repository.split(':')[0].toLowerCase(); | ||
| } |
There was a problem hiding this comment.
Repository normalization breaks images with ports/digests (false matches).
Split only on a tag colon that appears after the last slash, and strip digests. Current split(':')[0] truncates e.g. registry:5000/repo:tag -> 'registry'.
Apply this diff:
- private normalizeRepository(repository: string): string {
- return repository.split(':')[0].toLowerCase();
- }
+ private normalizeRepository(repository: string): string {
+ // strip digest if present
+ const [noDigest] = repository.split('@');
+ // remove tag only if the colon is after the last slash (i.e., it's a tag, not a port)
+ const lastColon = noDigest.lastIndexOf(':');
+ const lastSlash = noDigest.lastIndexOf('/');
+ const withoutTag = lastColon > lastSlash ? noDigest.slice(0, lastColon) : noDigest;
+ return withoutTag.toLowerCase();
+ }| @ResolveField(() => DockerContainer, { description: 'Pause (Suspend) a container' }) | ||
| @UsePermissions({ | ||
| action: AuthAction.UPDATE_ANY, | ||
| resource: Resource.DOCKER, | ||
| }) | ||
| public async pause(@Args('id', { type: () => PrefixedID }) id: string) { | ||
| return this.dockerService.pause(id); | ||
| } | ||
| @ResolveField(() => DockerContainer, { description: 'Unpause (Resume) a container' }) | ||
| @UsePermissions({ | ||
| action: AuthAction.UPDATE_ANY, | ||
| resource: Resource.DOCKER, | ||
| }) | ||
| public async unpause(@Args('id', { type: () => PrefixedID }) id: string) { | ||
| return this.dockerService.unpause(id); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Pause/unpause resolvers look consistent; please verify service wiring and idempotency.
Looks good with correct @ResolveField and permissions. Two asks:
- Confirm DockerService exposes pause/unpause and GraphQL schema includes these fields.
- Ensure pause/unpause are idempotent (calling on already paused/running containers returns 200/OK state vs throwing), and map engine errors to typed GraphQL errors.
Run to verify:
Optional: unify descriptions ("Pause (Suspend)" vs "Unpause (Resume)") for symmetry.
🏁 Script executed:
#!/bin/bash
# 1) Service methods exist
rg -nP --type=ts -C2 '\bclass\s+DockerService\b' api | sed -n '1,200p'
rg -nP --type=ts -n '\bpause\s*\(' api
rg -nP --type=ts -n '\bunpause\s*\(' api
# 2) Schema contains mutations
rg -nP -n '(type|extend\s+type)\s+DockerMutations' api | head -n 5
rg -nP -n '\bpause\s*\(' api | head -n 5
rg -nP -n '\bunpause\s*\(' api | head -n 5
# 3) ContainerState has PAUSED
rg -nP -n 'enum\s+ContainerState\b.*?PAUSED' -U apiLength of output: 3254
🏁 Script executed:
#!/bin/bash
# Get full pause/unpause method implementations
echo "=== pause method implementation ==="
sed -n '283,310p' api/src/unraid-api/graph/resolvers/docker/docker.service.ts
echo -e "\n=== unpause method implementation ==="
sed -n '311,338p' api/src/unraid-api/graph/resolvers/docker/docker.service.ts
# Check for error handling patterns in the file
echo -e "\n=== Error handling patterns in docker.service ==="
rg -nP --type=ts '(catch|throw|GraphQL|error)' api/src/unraid-api/graph/resolvers/docker/docker.service.ts | head -n 20
# Check ContainerState enum
echo -e "\n=== ContainerState enum ==="
rg -nP --type=ts 'enum\s+ContainerState' -A 15 api/src/unraid-api/graph/resolvers/docker/
# Check DockerContainer response type
echo -e "\n=== DockerContainer type/interface ==="
rg -nP --type=ts '(interface|type|class)\s+DockerContainer' -A 5 api/src/unraid-api/graph/Length of output: 6877
Critical: pause/unpause lack error handling and idempotency guarantees.
Service methods confirmed to exist with correct schema wiring. However, verification reveals two critical gaps:
-
Missing error handling:
container.pause()andcontainer.unpause()calls are unprotected. If already paused/running or on Docker errors, exceptions bubble up as generic errors instead of typed GraphQL errors (inconsistent with error handling patterns elsewhere in the service likehandleDockerListError). -
Not idempotent: Methods call
container.pause()/unpause()without checking current state first. Calling pause on already-paused containers will likely throw, violating idempotency expectations.
Required fixes at api/src/unraid-api/graph/resolvers/docker/docker.service.ts:
- Lines 283–310 (pause): Wrap
container.pause()in try/catch; map errors to GraphQL errors; optionally check current state first. - Lines 311–338 (unpause): Same pattern—wrap call, map errors, check state.
Optional: Unify resolver descriptions ("Pause (Suspend)" vs "Unpause (Resume)") for consistency.
| public async pause(id: string): Promise<DockerContainer> { | ||
| const container = this.client.getContainer(id); | ||
| await container.pause(); | ||
| await this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY); | ||
| this.logger.debug(`Invalidated container cache after pausing ${id}`); | ||
|
|
||
| let containers = await this.getContainers({ skipCache: true }); | ||
| let updatedContainer: DockerContainer | undefined; | ||
| for (let i = 0; i < 5; i++) { | ||
| await sleep(500); | ||
| containers = await this.getContainers({ skipCache: true }); | ||
| updatedContainer = containers.find((c) => c.id === id); | ||
| this.logger.debug( | ||
| `Container ${id} state after pause attempt ${i + 1}: ${updatedContainer?.state}` | ||
| ); | ||
| if (updatedContainer?.state === ContainerState.PAUSED) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!updatedContainer) { | ||
| throw new Error(`Container ${id} not found after pausing`); | ||
| } | ||
| const appInfo = await this.getAppInfo(); | ||
| await pubsub.publish(PUBSUB_CHANNEL.INFO, appInfo); | ||
| return updatedContainer; | ||
| } |
There was a problem hiding this comment.
Invalidate both container caches on pause/unpause.
Only deleting CONTAINER_CACHE_KEY leaves CONTAINER_WITH_SIZE_CACHE_KEY stale. Use clearContainerCache() for consistency.
- await this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY);
- this.logger.debug(`Invalidated container cache after pausing ${id}`);
+ await this.clearContainerCache();
+ this.logger.debug(`Invalidated container caches after pausing ${id}`);
@@
- await this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY);
- this.logger.debug(`Invalidated container cache after unpausing ${id}`);
+ await this.clearContainerCache();
+ this.logger.debug(`Invalidated container caches after unpausing ${id}`);Also applies to: 311-337
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 289-289: Useless assignment to local variable
The initial value of containers is unused, since it is always overwritten.
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/docker/docker.service.ts around lines 283
to 309 (and similarly for lines 311 to 337), the pause/unpause logic only
deletes CONTAINER_CACHE_KEY leaving CONTAINER_WITH_SIZE_CACHE_KEY stale; replace
the direct this.cacheManager.del(DockerService.CONTAINER_CACHE_KEY) calls with a
call to this.clearContainerCache() so both caches are invalidated consistently,
and ensure the subsequent logic remains unchanged (invalidate both caches, fetch
updated containers, wait for state change, publish app info).
| async getResources( | ||
| opts?: Partial<ContainerListOptions> & { skipCache?: boolean } | ||
| ): Promise<OrganizerV1['resources']> { | ||
| const { skipCache = false, ...listOptions } = opts ?? {}; | ||
| const containers = await this.dockerService.getContainers({ | ||
| skipCache, | ||
| ...(listOptions as any), | ||
| }); | ||
| return containerListToResourcesObject(containers); |
There was a problem hiding this comment.
Avoid any spread; keep typed options.
Minor typing issue: (listOptions as any) breaks type safety.
- const containers = await this.dockerService.getContainers({
- skipCache,
- ...(listOptions as any),
- });
+ const containers = await this.dockerService.getContainers({
+ skipCache,
+ ...(listOptions as Partial<ContainerListOptions>),
+ });If DockerService has a narrow options type, consider introducing a shared GetContainersOptions interface to avoid casting altogether.
🤖 Prompt for AI Agents
In
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts
around lines 59 to 67, the code spreads (listOptions as any) into getContainers
which breaks type safety; replace the any cast by using a properly typed options
interface: create or import a shared GetContainersOptions (e.g., a narrow
interface that matches DockerService.getContainers parameter), type the local
opts/listOptions to Partial<GetContainersOptions> & { skipCache?: boolean }, and
spread listOptions without casting so the call to
this.dockerService.getContainers remains fully typed; if necessary adjust
DockerService.getContainers signature to accept GetContainersOptions to keep
types aligned.
| async function handleDeleteFolder() { | ||
| const id = selectedFolderId.value; | ||
| if (!id || id === rootFolderId.value) return; | ||
| if (!confirm('Delete this folder? Contents will move to root.')) return; | ||
| await deleteFolderMutation( | ||
| { entryIds: [id] }, | ||
| { | ||
| refetchQueries: [refetchQuery], | ||
| awaitRefetchQueries: true, | ||
| } | ||
| ); | ||
| selectedFolderId.value = rootFolderId.value; | ||
| onSuccess?.('Folder deleted'); | ||
| } |
There was a problem hiding this comment.
Confirmation text is misleading; backend deletes contents, not “move to root”
deleteDockerEntries cascades delete; telling users “Contents will move to root” risks data loss.
- if (!confirm('Delete this folder? Contents will move to root.')) return;
+ if (!confirm('Delete this folder? This will also delete all its contents.')) return;If you actually intend “move to root”, implement a dedicated mutation to reparent children before deleting, and call that here.
Also applies to: 165-176
🤖 Prompt for AI Agents
In web/src/composables/useFolderOperations.ts around lines 132 to 145 (and apply
the same change to 165–176), the confirmation prompt falsely states “Contents
will move to root” even though the backend cascades deletes; update the
confirmation text to accurately warn that deleting the folder will also delete
its contents (e.g., “Delete this folder and all its contents?”) OR, if you
intend to move children to the root instead of deleting, implement and call a
dedicated reparenting mutation to move child entries to root before calling the
deleteFolderMutation and adjust the UI flow to await that mutation and only then
delete the now-empty folder.
| const visibleFolders = computed<FlatFolderRow[]>(() => { | ||
| const folders = allFolders.value; | ||
| const visible: FlatFolderRow[] = []; | ||
| const expanded = expandedFolders.value; | ||
|
|
||
| const visibleIds = new Set<string>(); | ||
|
|
||
| for (const folder of folders) { | ||
| if (!folder.parentId) { | ||
| visibleIds.add(folder.id); | ||
| } else if (visibleIds.has(folder.parentId) && expanded.has(folder.parentId)) { | ||
| visibleIds.add(folder.id); | ||
| } | ||
| } | ||
|
|
||
| for (const folder of folders) { | ||
| if (visibleIds.has(folder.id)) { | ||
| visible.push({ | ||
| id: folder.id, | ||
| name: folder.name, | ||
| depth: folder.depth, | ||
| hasChildren: folder.hasChildren, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return visible; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Derive visibility independent of input order.
Current single-pass parent-expanded check assumes folders are ordered. Sort by depth and/or walk from roots to ensure grandchildren appear only when all ancestors are expanded.
- const visibleFolders = computed<FlatFolderRow[]>(() => {
- const folders = allFolders.value;
- const visible: FlatFolderRow[] = [];
- const expanded = expandedFolders.value;
- const visibleIds = new Set<string>();
- for (const folder of folders) {
- if (!folder.parentId) {
- visibleIds.add(folder.id);
- } else if (visibleIds.has(folder.parentId) && expanded.has(folder.parentId)) {
- visibleIds.add(folder.id);
- }
- }
- for (const folder of folders) {
- if (visibleIds.has(folder.id)) {
- visible.push({ id: folder.id, name: folder.name, depth: folder.depth, hasChildren: folder.hasChildren });
- }
- }
- return visible;
- });
+ const visibleFolders = computed<FlatFolderRow[]>(() => {
+ const folders = [...allFolders.value].sort((a, b) => a.depth - b.depth);
+ const expanded = expandedFolders.value;
+ const visibleIds = new Set<string>();
+ const out: FlatFolderRow[] = [];
+ for (const f of folders) {
+ const parentVisible = !f.parentId || (visibleIds.has(f.parentId) && expanded.has(f.parentId));
+ if (parentVisible) {
+ visibleIds.add(f.id);
+ out.push({ id: f.id, name: f.name, depth: f.depth, hasChildren: f.hasChildren });
+ }
+ }
+ return out;
+ });Also applies to: 35-41, 43-52
| function expandFolder(id: string) { | ||
| expandedFolders.value.add(id); | ||
| } | ||
|
|
||
| function collapseFolder(id: string) { | ||
| expandedFolders.value.delete(id); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix non-reactive Set mutations in expand/collapse.
Mutating Set in place won’t trigger reactivity. Reassign a new Set after updates (like toggle does).
function expandFolder(id: string) {
- expandedFolders.value.add(id);
+ const set = new Set(expandedFolders.value);
+ set.add(id);
+ expandedFolders.value = set;
}
function collapseFolder(id: string) {
- expandedFolders.value.delete(id);
+ const set = new Set(expandedFolders.value);
+ set.delete(id);
+ expandedFolders.value = set;
}🤖 Prompt for AI Agents
In web/src/composables/useFolderTree.ts around lines 64 to 71, expandFolder and
collapseFolder mutate the Set in place which does not trigger Vue reactivity;
change them to create and assign a new Set instead (for expandFolder: build a
new Set from the existing values plus the id, then assign to
expandedFolders.value; for collapseFolder: create a copy of the current Set,
remove the id from the copy, then assign that new Set to expandedFolders.value)
so updates are reactive (match the toggle implementation pattern).
| import { createI18nInstance, ensureLocale, getWindowLocale } from '~/helpers/i18n-loader'; | ||
|
|
||
| import { globalPinia } from '~/store/globalPinia'; | ||
| import { ensureUnapiScope } from '~/utils/unapiScope'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use .js extension in TS ESM import.
For ESM compatibility, import internal TS modules with .js in specifiers.
-import { ensureUnapiScope } from '~/utils/unapiScope';
+import { ensureUnapiScope } from '~/utils/unapiScope.js';As per coding guidelines. Mount logic is fine; no other issues noted.
Also applies to: 31-37
🤖 Prompt for AI Agents
In web/src/main.ts around lines 15 and also for imports at lines 31-37, the
review notes that TypeScript ESM imports of internal modules must include the
.js extension in the import specifiers; update each import statement (including
the one for ~/utils/unapiScope and the imports on lines 31-37) to append .js to
the module path so the compiled ESM output resolves correctly at runtime.
| function getObserverRoot(root: ScopeRoot): Element | null { | ||
| if (root instanceof Document) { | ||
| return root.body ?? root.documentElement; | ||
| } | ||
|
|
||
| if (root instanceof DocumentFragment) { | ||
| return root.firstElementChild; | ||
| } | ||
|
|
||
| return root; | ||
| } |
There was a problem hiding this comment.
Bug: Observing DocumentFragment via firstElementChild misses later additions.
You can observe a DocumentFragment directly. Current approach returns null if empty, so mutations are never observed.
Apply:
@@
-function getObserverRoot(root: ScopeRoot): Element | null {
- if (root instanceof Document) {
- return root.body ?? root.documentElement;
- }
- if (root instanceof DocumentFragment) {
- return root.firstElementChild;
- }
- return root;
-}
+function getObserverRoot(root: ScopeRoot): Node | null {
+ if (root instanceof Document) return root;
+ if (root instanceof DocumentFragment) return root;
+ return root; // Element
+}
@@
- const observerRoot = getObserverRoot(getQueryRoot(root));
+ const observerRoot = getObserverRoot(getQueryRoot(root));
if (!observerRoot) {
return;
}This ensures future nodes appended to the fragment are captured.
Also applies to: 121-124
🤖 Prompt for AI Agents
In web/src/utils/unapiScope.ts around lines 44 to 54, the function returns
root.firstElementChild for DocumentFragment which misses mutations added later;
change the logic to return the DocumentFragment itself instead of its
firstElementChild and update the function return type from Element | null to
Node | null (and adjust any call sites to accept Node) so MutationObserver can
be attached directly to the fragment; apply the same fix to the similar code at
lines ~121-124.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/app/__test__/app.module.integration.spec.ts (1)
69-113: Add mock cleanup between tests.The guidelines require clearing mocks between tests to prevent state pollution. Add an
afterEachhook to reset mock state.As per coding guidelines.
Apply this diff to add mock cleanup:
afterAll(async () => { if (app) { await app.close(); } }); + + afterEach(() => { + vi.clearAllMocks(); + });
🧹 Nitpick comments (4)
api/src/core/utils/misc/catch-handlers.ts (1)
4-6: Type change looks good, but consider explicit undefined handling below.Making
addressoptional better reflects the actual error structure. However, the usage at line 16 would benefit from an explicit check.Consider applying this defensive check at line 16 for clarity:
- if (error.code === 'ENOENT' && error.address === socketPath) { + if (error.code === 'ENOENT' && error.address && error.address === socketPath) {This makes the undefined handling explicit and improves readability.
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1)
52-57: LGTM! Consider exposing an optional limit parameter.The implementation is clean and follows existing resolver patterns. The service method signature suggests it accepts a
limitparameter—consider exposing this as an optional argument to give clients control over the result size, or confirm the default behavior is intentional.api/src/unraid-api/app/__test__/app.module.integration.spec.ts (1)
183-189: Clarify test description to match actual behavior.The test description claims to verify "service-to-service communication," but it only checks that
AuthServicecan be resolved and has the expected method. No actual inter-service communication is tested.Apply this diff to align the description with the actual test:
- it('should have working service-to-service communication', () => { - // Test that the module can resolve its services without errors - // This validates that dependency injection is working correctly + it('should resolve services with correct method signatures', () => { const authService = moduleRef.get(AuthService); expect(authService).toBeDefined(); expect(typeof authService.validateCookiesWithCsrfToken).toBe('function');api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
125-131: Consider resetting all mock methods for consistency.Currently, only some mock methods are reset in
beforeEach. For better test isolation and to prevent potential state leakage in future tests, consider also resetting:
mockDockerConfigService.replaceConfigmockDockerConfigService.validatemockDockerTemplateScannerService.scanTemplatesmockNotificationsService.notifyIfUniqueExample:
mockDockerTemplateScannerService.bootstrapScan.mockResolvedValue(undefined); mockDockerTemplateScannerService.syncMissingContainers.mockResolvedValue(false); + mockDockerTemplateScannerService.scanTemplates.mockResolvedValue({ + scanned: 0, + matched: 0, + skipped: 0, + errors: [], + }); + mockDockerConfigService.replaceConfig.mockReset(); + mockDockerConfigService.validate.mockResolvedValue(undefined); + mockNotificationsService.notifyIfUnique.mockResolvedValue(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/src/core/utils/misc/catch-handlers.ts(1 hunks)api/src/unraid-api/app/__test__/app.module.integration.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(4 hunks)api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts(2 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/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/vms/vms.service.spec.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.resolver.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/app/__test__/app.module.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/vms/vms.service.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/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/core/utils/misc/catch-handlers.tsapi/src/unraid-api/graph/resolvers/vms/vms.service.spec.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.resolver.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/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/vms/vms.service.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/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/vms/vms.service.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/organizer/docker-organizer.service.spec.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/core/utils/misc/catch-handlers.tsapi/src/unraid-api/graph/resolvers/vms/vms.service.spec.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
🧠 Learnings (10)
📚 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__/store/**/*.ts : In store tests, mock external dependencies used by the store and verify interactions with those mocks
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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,web}/**/*.{test,spec}.{ts,tsx} : Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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 web/__test__/**/*.{test,spec}.{ts,tsx} : Mock external dependencies and services in component tests
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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 web/__test__/**/*.{test,spec}.{ts,tsx} : Let stores initialize with natural default state; don’t mock the store under test
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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__/store/**/*.ts : Do not mock the store under test; allow the store to initialize with its natural default state
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.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} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.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/app/__test__/app.module.integration.spec.ts
🧬 Code graph analysis (1)
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (3)
web/src/composables/gql/graphql.ts (4)
Notification(1459-1472)Mutation(1264-1311)NotificationData(1482-1488)Subscription(2113-2126)packages/unraid-shared/src/use-permissions.directive.ts (1)
UsePermissions(81-128)api/src/core/pubsub.ts (1)
createSubscription(19-23)
🔇 Additional comments (11)
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts (2)
5-5: LGTM!The import statement correctly includes the
.jsextension for ESM compatibility and is necessary for providing the mock dependency in the test setup.
220-225: LGTM!The mock provider for
DockerTemplateIconServiceis appropriately structured and follows the established pattern of other service mocks in this test suite. Mocking this dependency is suitable since icon extraction is a non-trivial operation, and the mock correctly provides a minimal implementation that returns an empty Map for isolated testing ofDockerOrganizerService.api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (2)
106-116: Formatting issue resolved.The Prettier formatting issue flagged in the previous review has been addressed—the description is now properly split across lines 108-109 with correct indentation. The mutation logic and types are correct.
186-193: LGTM!The subscription is properly implemented with correct permissions, follows the established pattern for other subscriptions in the file, and correctly delegates to the pub/sub infrastructure.
api/src/unraid-api/app/__test__/app.module.integration.spec.ts (2)
13-63: Mock setup is well-structured.The centralized store mock with factory functions aligns with testing guidelines. The mock provides comprehensive test data for CSRF tokens, Docker state, and filesystem paths.
115-126: LGTM!The tests correctly validate module compilation and dependency injection with overridden providers.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (5)
10-11: LGTM! Imports follow ESM conventions.The new service imports correctly use
.jsextensions for ESM compatibility.Also applies to: 14-14
85-94: LGTM! Mock structure follows existing patterns.The
mockDockerConfigServiceis properly structured with sensible default return values for testing.
96-106: LGTM! Mock implementation is well-structured.The
mockDockerTemplateScannerServiceprovides appropriate default return values for all methods.
108-111: LGTM! Minimal mock is appropriate.The
mockNotificationsServicefollows the established pattern.
140-151: LGTM! Provider injection follows NestJS testing patterns.The new service mocks are properly injected into the TestingModule, enabling isolated unit testing of the DockerService.
| beforeAll(() => { | ||
| if (!isQemuAvailable()) { | ||
| throw new Error( | ||
| 'QEMU not available - skipping VM integration tests. Please install QEMU to run these tests.' | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Clarify error message - throwing doesn't skip tests.
The error message states "skipping VM integration tests", but throwing an error in beforeAll fails the entire test suite rather than skipping it. The fail-fast approach is reasonable, but the message should reflect the actual behavior.
Apply this diff to clarify the message:
- throw new Error(
- 'QEMU not available - skipping VM integration tests. Please install QEMU to run these tests.'
- );
+ throw new Error(
+ 'QEMU not available. Please install QEMU to run VM integration tests.'
+ );📝 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.
| beforeAll(() => { | |
| if (!isQemuAvailable()) { | |
| throw new Error( | |
| 'QEMU not available - skipping VM integration tests. Please install QEMU to run these tests.' | |
| ); | |
| } | |
| }); | |
| beforeAll(() => { | |
| if (!isQemuAvailable()) { | |
| throw new Error( | |
| 'QEMU not available. Please install QEMU to run VM integration tests.' | |
| ); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts around lines 187
to 193, the beforeAll currently throws an Error saying "QEMU not available -
skipping VM integration tests..." which is inaccurate because throwing in
beforeAll fails the suite rather than skipping; update the thrown Error message
to accurately state the behavior (e.g., "QEMU not available - failing VM
integration tests. Install QEMU to run these tests.") so the message reflects
that the tests will fail when QEMU is missing.
pujitm
force-pushed
the
feat/docker-overview-table
branch
from
7b8a4ff to
79f874a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1764 +/- ##
==========================================
- Coverage 52.69% 50.10% -2.59%
==========================================
Files 865 901 +36
Lines 49343 53014 +3671
Branches 4952 5114 +162
==========================================
+ Hits 26000 26564 +564
- Misses 23270 26356 +3086
- Partials 73 94 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/auth/api-key.service.ts (1)
232-236: Avoid usinganytype.The explicit
anytype annotation violates the coding guidelines. Define a proper type for the parsed permission structure or useunknownwith appropriate type narrowing.As per coding guidelines.
Consider defining an interface for the parsed content:
+ interface ParsedPermission { + resource?: string; + actions?: string[]; + } + // Normalize permission actions to AuthAction enum values // Uses shared helper to handle all legacy formats if (parsedContent.permissions) { - parsedContent.permissions = parsedContent.permissions.map((permission: any) => ({ + parsedContent.permissions = parsedContent.permissions.map((permission: ParsedPermission) => ({ ...permission, actions: normalizeLegacyActions(permission.actions || []), }));
🧹 Nitpick comments (3)
api/src/core/utils/misc/catch-handlers.ts (1)
16-16: Consider adding explicit undefined check for defensive programming.While the current logic works correctly in typical scenarios, explicitly checking for
error.addressbefore comparison would guard against the edge case where bothaddressandsocketPathare undefined.Apply this diff for more explicit defensive handling:
- if (error.code === 'ENOENT' && error.address === socketPath) { + if (error.code === 'ENOENT' && error.address && error.address === socketPath) {api/src/unraid-api/auth/api-key.service.ts (2)
186-190: Use a lower log level for non-fatal ENOENT case.The code treats ENOENT as non-fatal (returns empty array), but logs it as an error. Since the constructor ensures the directory exists and this scenario represents a valid state (no API keys created yet), consider using
this.logger.debug()orthis.logger.warn()instead ofthis.logger.error()to avoid alarming users or operators with error-level logs for expected conditions.Apply this diff:
if (error.code === 'ENOENT') { // Directory doesn't exist, which means no API keys have been created yet - this.logger.error(`API key directory does not exist: ${this.basePath}`); + this.logger.debug(`API key directory does not exist: ${this.basePath}`); return []; }
66-70: Correct the chokidar callback signature.The
'all'event handler receives(event, path)parameters, but the callback only declares(path). While this doesn't cause a runtime error due to JavaScript's parameter handling, it's imprecise and prevents logging which event occurred (add, change, unlink).Apply this diff:
- watch(this.basePath, { ignoreInitial: false }).on('all', async (path) => { - this.logger.debug(`API key changed: ${path}`); + watch(this.basePath, { ignoreInitial: false }).on('all', async (event, path) => { + this.logger.debug(`API key ${event}: ${path}`); this.memoryApiKeys = []; this.memoryApiKeys = await this.loadAllFromDisk(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/src/core/utils/misc/catch-handlers.ts(1 hunks)api/src/unraid-api/app/__test__/app.module.integration.spec.ts(3 hunks)api/src/unraid-api/auth/api-key.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(4 hunks)api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.spec.ts
- api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
- api/src/unraid-api/app/test/app.module.integration.spec.ts
- api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
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/notifications/notifications.resolver.tsapi/src/unraid-api/auth/api-key.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/notifications/notifications.resolver.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/core/utils/misc/catch-handlers.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/notifications/notifications.resolver.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/core/utils/misc/catch-handlers.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/auth/api-key.service.ts
📚 Learning: 2024-11-04T20:41:22.303Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:41:22.303Z
Learning: In `api/src/unraid-api/auth/auth.service.ts`, the `addRoleToApiKey` function operates on API keys stored as JSON files in a directory, not a database, so concurrency is not a concern for modifying `apiKey.roles`.
Applied to files:
api/src/unraid-api/auth/api-key.service.ts
🧬 Code graph analysis (2)
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (3)
web/src/composables/gql/graphql.ts (4)
Notification(1459-1472)Mutation(1264-1311)NotificationData(1482-1488)Subscription(2113-2126)packages/unraid-shared/src/use-permissions.directive.ts (1)
UsePermissions(81-128)api/src/core/pubsub.ts (1)
createSubscription(19-23)
api/src/unraid-api/auth/api-key.service.ts (1)
api/src/unraid-api/cli/log.service.ts (1)
error(65-69)
🔇 Additional comments (3)
api/src/core/utils/misc/catch-handlers.ts (1)
4-6: LGTM! Correct defensive typing improvement.Making the
addressproperty optional accurately models the behavior ofNodeJS.ErrnoException, which doesn't always include an address. This prevents type errors when handling Docker errors that lack this property.api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (2)
52-57: LGTM!The
warningsAndAlertsResolveField is well-structured with a clear description and proper typing. The delegation to the service layer follows the pattern used throughout the resolver.
185-193: Code implementation verified; external package definition cannot be inspected in this environment.The subscription correctly imports
PUBSUB_CHANNELfrom@app/core/pubsub.jsand usesPUBSUB_CHANNEL.NOTIFICATION_WARNINGS_AND_ALERTSconsistently with the service layer (which publishes to the same channel at line 148 of notifications.service.ts). The resolver has proper@Subscriptionand@UsePermissionsdecorators with correct type annotations. However, the actualNOTIFICATION_WARNINGS_AND_ALERTSconstant definition resides in the external@unraid/sharedpackage, which cannot be inspected in the sandbox environment. The fact that the code compiles and both service and resolver use the same channel consistently indicates the constant is properly defined.
| @Mutation(() => Notification, { | ||
| nullable: true, | ||
| description: | ||
| 'Creates a notification if an equivalent unread notification does not already exist.', | ||
| }) | ||
| public notifyIfUnique( | ||
| @Args('input', { type: () => NotificationData }) | ||
| data: NotificationData | ||
| ): Promise<Notification | null> { | ||
| return this.notificationsService.notifyIfUnique(data); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify authorization requirements for the mutation.
The notifyIfUnique mutation lacks a @UsePermissions decorator, while queries and subscriptions in this resolver are protected. If this mutation is intended for programmatic/internal use only, this may be acceptable. However, if it's exposed to external GraphQL clients, consider whether authorization checks are needed.
Verify whether this mutation requires permissions by checking:
- The intended usage pattern (internal vs. external API calls)
- Whether permission checks are handled at a different layer
- Consistency with the authorization model for other mutations
🏁 Script executed:
#!/bin/bash
# Check if other notification mutations have permissions and look for any authorization middleware
rg -n --type=ts -C3 '@Mutation.*Notification' | head -80
# Check for any authorization guards or middleware applied at module or controller level
rg -n --type=ts -C3 'UseGuards|CanActivate|@UsePermissions' api/src/unraid-api/graph/resolvers/notifications/Length of output: 11075
[skip_cloning_not_applicable]
---
🏁 Script executed:
```shell
#!/bin/bash
# Check authorization pattern across other resolvers - do mutations generally lack permissions?
rg -n --type=ts '@Mutation' api/src/unraid-api/graph/resolvers/ -A5 | grep -E '(@Mutation|@UsePermissions)' | head -60
# Check if NotificationsService implements authorization checks
rg -n --type=ts -C5 'notifyIfUnique|notificationsService' api/src/unraid-api/graph/services/ | grep -i 'permission\|auth\|guard' | head -30
# Check module-level guards or guards applied at GraphQL module level
rg -n --type=ts '@Module|UseGuards|canActivate' api/src/unraid-api/graph/ | head -40
Length of output: 5941
🏁 Script executed:
#!/bin/bash
# Check where notifyIfUnique is called from - is it internal or external?
rg -n 'notifyIfUnique' api/src/unraid-api/ -B2 -A2
# Compare settings mutations with notifications to understand the difference
rg -n --type=ts '@Mutation' api/src/unraid-api/graph/resolvers/settings/ -B1 -A8 | head -50
# Check if there's a GraphQL interceptor or guard at the module level
rg -n --type=ts 'GraphQL.*Guard|Interceptor.*GraphQL|@UseInterceptors' api/src/unraid-api/graph/ | head -20Length of output: 6650
Add @UsePermissions decorator to the notifyIfUnique mutation for consistency.
The mutation lacks authorization protection while all queries, subscriptions, and other mutations in this resolver (including settings mutations) are protected with @UsePermissions. Although the method is primarily called internally from services like docker.service, exposing it via GraphQL without authentication creates an inconsistency with your authorization model. Add the same READ_ANY permission check applied to queries and subscriptions:
@UsePermissions({
action: AuthAction.READ_ANY,
resource: Resource.NOTIFICATIONS,
})🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
around lines 106 to 116, the notifyIfUnique GraphQL mutation is missing the
@UsePermissions decorator used elsewhere; add the same permission check as other
resolver methods by applying @UsePermissions with action AuthAction.READ_ANY and
resource Resource.NOTIFICATIONS immediately above the notifyIfUnique method so
the mutation is protected consistently with queries, subscriptions, and other
mutations.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/__tests__/scopeTailwindToUnapi.spec.ts (1)
70-77: LGTM! Test validates the core root element scoping behavior.The test correctly verifies that
htmlandbodyselectors are transformed to the scope element (.unapi), with suffixes properly preserved.Optional: Consider additional test cases for comprehensive coverage.
The current test covers the primary scenarios. For more thorough validation, you could add cases for:
- Combinators:
html > div→.unapi > div- Pseudo-selectors:
body:hover→.unapi:hover- Multiple suffixes:
body.dark.theme-custom→.unapi.dark.theme-custom- Negative cases: verify
htmldivis treated as a regular class (not specially scoped)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/postcss/scopeTailwindToUnapi.ts(2 hunks)web/src/__tests__/scopeTailwindToUnapi.spec.ts(1 hunks)web/src/assets/main.css(2 hunks)
🧰 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:
web/src/__tests__/scopeTailwindToUnapi.spec.tsweb/postcss/scopeTailwindToUnapi.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:
web/src/__tests__/scopeTailwindToUnapi.spec.ts
🧠 Learnings (2)
📚 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: Tailwind v4 is used; no tailwind config file is needed
Applied to files:
web/src/assets/main.css
📚 Learning: 2025-04-02T21:21:29.168Z
Learnt from: elibosley
PR: unraid/api#1308
File: unraid-ui/src/components/common/loading/Error.vue:2-2
Timestamp: 2025-04-02T21:21:29.168Z
Learning: Components in the unraid-ui folder require explicit imports and are not autoloaded, unlike other parts of the project that may use Nuxt.js autoloading features.
Applied to files:
web/src/assets/main.css
🧬 Code graph analysis (1)
web/src/__tests__/scopeTailwindToUnapi.spec.ts (1)
web/postcss/scopeTailwindToUnapi.ts (1)
scopeTailwindToUnapi(133-168)
🔇 Additional comments (4)
web/postcss/scopeTailwindToUnapi.ts (2)
39-39: LGTM! Pattern correctly identifies root element selectors.The regex properly matches
htmlorbodyonly when followed by valid selector boundaries, preventing false positives likehtmldivorbodybuilder.
108-112: LGTM! Root element transformation logic is sound.The implementation correctly:
- Strips
htmlorbodyfrom the selector- Preserves any suffix (classes, pseudo-selectors, combinators)
- Returns just the scope when no suffix exists
Examples:
html→.unapi,body.dark→.unapi.dark,html:hover→.unapi:hoverweb/src/assets/main.css (2)
12-12: ✓ Nuxt UI import re-enabled for Docker web component feature.Re-enabling the Nuxt UI import is appropriate for the Docker web component implementation. This aligns with the PR objectives.
38-40: Dialog button resets look good but verify styling consistency.The simplified button resets (removing
!importantflags and using standard properties) are cleaner. Ensure Nuxt UI modal/slideover buttons maintain proper styling across the dialog variants. This assumes Nuxt UI v3 buttons don't rely on inherited button styles that the preflight previously provided globally.Test that Nuxt UI modal close buttons and dialog controls render correctly with the scoped preflight approach.
pujitm
force-pushed
the
feat/docker-overview-table
branch
from
5d6bda0 to
234c2e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/__tests__/scopeTailwindToUnapi.spec.ts (1)
5-5: Missing .js extension in import specifier.The import path should include the
.jsextension for ESM compatibility.As per coding guidelines
Apply this diff:
-import scopeTailwindToUnapi from '../../postcss/scopeTailwindToUnapi'; +import scopeTailwindToUnapi from '../../postcss/scopeTailwindToUnapi.js';
♻️ Duplicate comments (1)
web/src/assets/main.css (1)
27-28: Preflight scoping approach remains problematic.As previously flagged, importing
tailwindcss/preflight.cssinside@layer unapi-basehas fundamental issues—preflight targets document root elements (html, body) which cannot be scoped to a container selector like.unapi.Refer to the earlier review comment for detailed solutions: either omit preflight and provide custom scoped resets, or use a PostCSS nesting plugin with
:where(.unapi)wrapper.
🧹 Nitpick comments (1)
web/src/__tests__/scopeTailwindToUnapi.spec.ts (1)
70-77: LGTM: Test correctly validates root element scoping.The test appropriately verifies both bare root elements (
html→.unapi) and root elements with suffixes (body.dark→.unapi.dark).Consider adding edge case coverage for selectors like
html.dark,body > div, orhtml bodyin future test iterations to ensure comprehensive validation of the ROOT_ELEMENT_PATTERN logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/postcss/scopeTailwindToUnapi.ts(2 hunks)web/src/__tests__/scopeTailwindToUnapi.spec.ts(1 hunks)web/src/assets/main.css(2 hunks)web/vite.config.ts(1 hunks)
🧰 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:
web/postcss/scopeTailwindToUnapi.tsweb/vite.config.tsweb/src/__tests__/scopeTailwindToUnapi.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:
web/src/__tests__/scopeTailwindToUnapi.spec.ts
🧠 Learnings (2)
📚 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: Tailwind v4 is used; no tailwind config file is needed
Applied to files:
web/src/assets/main.css
📚 Learning: 2025-04-02T21:21:29.168Z
Learnt from: elibosley
PR: unraid/api#1308
File: unraid-ui/src/components/common/loading/Error.vue:2-2
Timestamp: 2025-04-02T21:21:29.168Z
Learning: Components in the unraid-ui folder require explicit imports and are not autoloaded, unlike other parts of the project that may use Nuxt.js autoloading features.
Applied to files:
web/src/assets/main.css
🧬 Code graph analysis (1)
web/src/__tests__/scopeTailwindToUnapi.spec.ts (1)
web/postcss/scopeTailwindToUnapi.ts (1)
scopeTailwindToUnapi(133-168)
🪛 GitHub Actions: CI - Main (API)
web/src/assets/main.css
[error] 1-1: [@tailwindcss/vite:generate:build] Package path ./.nuxt/ui.css is not exported from package /home/runner/work/api/api/web/node_modules/@nuxt/ui (see exports field in /home/runner/work/api/api/web/node_modules/@nuxt/ui/package.json)
🔇 Additional comments (5)
web/src/assets/main.css (1)
39-42: LGTM! Improved button reset specificity.Removing
!importantfrom these button resets improves maintainability while still achieving the necessary style resets for dialog and Radix UI buttons.web/postcss/scopeTailwindToUnapi.ts (3)
38-38: LGTM: Light mode pattern addition is consistent.The addition of
/^\.light\b/mirrors the existing dark mode pattern and correctly uses a word boundary to ensure precise matching.
39-39: LGTM: Regex pattern correctly identifies root elements.The lookahead pattern ensures
htmlandbodyare matched only when followed by valid CSS syntax, and the character class appropriately covers common selectors and combinators.
108-112: LGTM: Root element transformation logic is correct.The implementation properly transforms root elements (
html/body) to the scope element with any suffix preserved (e.g.,body.dark→.unapi.dark,html→.unapi).web/src/__tests__/scopeTailwindToUnapi.spec.ts (1)
79-86: LGTM: Test correctly validates theme mode merging.The test properly verifies that both
.lightand.darkselectors are merged onto the unapi scope (.unapi.light,.unapi.dark), confirming the updated MERGE_WITH_SCOPE_PATTERNS behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/unraid-file-modifier/modifications/docker-containers-page.modification.ts (1)
9-9: Consider usingreadonlyfor theidproperty.The
idproperty could be simplified by adding thereadonlymodifier, and the explicit type annotation is optional when initialized with a string literal.- id: string = 'docker-containers-page'; + readonly id = 'docker-containers-page';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/unraid-api/unraid-file-modifier/modifications/docker-containers-page.modification.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/unraid-file-modifier/modifications/docker-containers-page.modification.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/unraid-file-modifier/modifications/docker-containers-page.modification.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/unraid-file-modifier/modifications/docker-containers-page.modification.ts
🧬 Code graph analysis (1)
api/src/unraid-api/unraid-file-modifier/modifications/docker-containers-page.modification.ts (1)
api/src/unraid-api/unraid-file-modifier/file-modification.ts (1)
ShouldApplyWithReason(14-23)
🔇 Additional comments (4)
api/src/unraid-api/unraid-file-modifier/modifications/docker-containers-page.modification.ts (4)
1-6: LGTM: Imports follow ESM guidelines.The imports correctly use
.jsextensions for local modules as required for ESM compatibility.
32-37: LGTM: Patch generation logic is correct.The method appropriately reads the current file content, generates new content, and creates a diff-based patch. The override path support is a good pattern for testing.
19-19: No issues found—method exists and is correctly used.The
isUnraidVersionGreaterThanOrEqualTomethod is properly defined as a protected async method on theFileModificationbase class (api/src/unraid-api/unraid-file-modifier/file-modification.ts, line 298) and is correctly called on line 19 of the docker-containers-page.modification.ts file.
39-52: No issues found—web component is properly registered and complete file replacement is intentional.The modification follows the FileModification pattern to replace the entire Unraid Docker Containers page template. The web component
unraid-docker-container-overviewis correctly registered incomponent-registry.tsand importsDockerContainerOverview.standalone.vue(confirmed to exist). The complete replacement is the intended behavior to upgrade the UI for Unraid versions < 7.3, with a version check that disables the modification for Unraid 7.3+ where Docker integration is native.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
web/src/components/Docker/DockerContainersTable.vue (2)
95-114: CRITICAL: Row ID must use stable organizer identifier (container name), not Docker runtime ID.This issue was flagged in a previous review but remains unresolved. Line 104 uses
meta?.id || name, which sets the row ID to the Docker runtime ID. However, theuseTreeDatacomposable keysentryParentByIdandpositionByIdmaps by organizer entry IDs (stable container names). This mismatch causes silent failures in drag-and-drop, folder moves, and position lookups because the row ID won't match the keys in these maps.Apply this diff to fix the issue:
function toContainerTreeRow( meta: DockerContainer | null | undefined, fallbackName?: string ): TreeRow<DockerContainer> { const name = meta?.names?.[0]?.replace(/^\//, '') || fallbackName || 'Unknown'; const updatesParts: string[] = []; if (meta?.isUpdateAvailable) updatesParts.push('Update'); if (meta?.isRebuildReady) updatesParts.push('Rebuild'); return { - id: meta?.id || name, + id: name, type: 'container', name, state: meta?.state ?? '', ports: formatPorts(meta || undefined), autoStart: meta?.autoStart ? 'On' : 'Off', updates: updatesParts.join(' / ') || '—', containerId: meta?.id, meta: meta || undefined, }; }
232-249: Normalize container state before color mapping.This issue was flagged in a previous review but remains unresolved. The
meta.statevalue may be a lowercase string (e.g., "running", "paused", "exited"), but thecolorMapis keyed byContainerStateenum values which are typically uppercase. Without normalization, the lookup at line 243 will silently fail and default to 'neutral', causing incorrect badge colors.Apply this diff to normalize the state:
cell: ({ row }) => { if (row.original.type === 'folder') return ''; - const state = row.original.state ?? ''; + const state = String(row.original.state ?? '').toUpperCase(); const isBusy = busyRowIds.value.has(row.original.id); const colorMap: Record<string, 'success' | 'warning' | 'neutral'> = { [ContainerState.RUNNING]: 'success', [ContainerState.PAUSED]: 'warning', [ContainerState.EXITED]: 'neutral', }; const color = colorMap[state] || 'neutral'; if (isBusy) { return h(USkeleton, { class: 'h-5 w-20' }); } return h(UBadge, { color }, () => state); },
🧹 Nitpick comments (10)
web/src/helpers/env.ts (1)
7-10: Feature flag default semantics: confirm intent.This enables navigation by default and only disables when env is exactly 'false'. If that’s intended, LGTM. Optionally mark as readonly for safer typing.
-export const featureFlags = { +export const featureFlags = { /** Navigate to legacy edit page when clicking docker containers instead of showing compacted view */ DOCKER_EDIT_PAGE_NAVIGATION: String(import.meta.env.VITE_DOCKER_EDIT_PAGE_NAVIGATION) !== 'false', -}; +} as const;web/src/components/Docker/DockerContainerManagement.vue (1)
235-249: Avoid 1970-01-01T... when created is missing.Show a placeholder instead of epoch when created is falsy.
- creationDate: new Date((c.created || 0) * 1000).toISOString(), + creationDate: c.created ? new Date(c.created * 1000).toISOString() : '—',web/src/composables/useDockerEditNavigation.ts (2)
1-1: ESM import extension for TS runtime imports.Per guidelines, use .js extension in TS import specifiers for ESM.
-import { featureFlags } from '@/helpers/env'; +import { featureFlags } from '@/helpers/env.js';As per coding guidelines.
18-23: Simplify basePath derivation.pathname never includes '?'. This can be simplified; also avoid trailing slash duplication.
- const currentPath = window.location.pathname; - const basePath = currentPath.substring( - 0, - currentPath.indexOf('?') === -1 ? currentPath.length : currentPath.indexOf('?') - ); - window.location.href = `${basePath}/UpdateContainer?xmlTemplate=edit:${encodeURIComponent(templatePath)}`; + const basePath = window.location.pathname.replace(/\/$/, ''); + window.location.href = `${basePath}/UpdateContainer?xmlTemplate=edit:${encodeURIComponent(templatePath)}`;web/src/components/Docker/DockerContainersTable.vue (3)
143-177: Consider simplifying the bidirectional columnVisibility sync.The current implementation uses a computed property with getter/setter plus two watchers to maintain bidirectional sync between
baseTableRef.columnVisibilityandcolumnVisibilityFallback. While functional, this pattern is complex and could lead to subtle timing issues or unnecessary re-renders.Consider using a single source of truth with a simpler fallback mechanism:
-const columnVisibilityFallback = ref<Record<string, boolean>>({}); -const columnVisibility = computed({ - get: (): Record<string, boolean> => { - if (baseTableRef.value?.columnVisibility) { - return baseTableRef.value.columnVisibility.value; - } - return columnVisibilityFallback.value; - }, - set: (value: Record<string, boolean>) => { - if (baseTableRef.value?.columnVisibility) { - baseTableRef.value.columnVisibility.value = value; - } - columnVisibilityFallback.value = value; - }, -}); - -watch( - () => baseTableRef.value?.columnVisibility?.value, - (value) => { - if (value) { - columnVisibilityFallback.value = value; - } - }, - { immediate: true, deep: true } -); - -watch( - baseTableRef, - (instance) => { - if (instance?.columnVisibility && Object.keys(columnVisibilityFallback.value).length) { - instance.columnVisibility.value = { ...columnVisibilityFallback.value }; - } - }, - { immediate: true } -); +const columnVisibility = computed(() => + baseTableRef.value?.columnVisibility?.value ?? {} +); + +function setColumnVisibility(value: Record<string, boolean>) { + if (baseTableRef.value?.columnVisibility) { + baseTableRef.value.columnVisibility.value = value; + } +}Then update line 366 from
columnVisibility.value = { ...current, [id]: next };tosetColumnVisibility({ ...current, [id]: next });
385-396: Consider using a proper toast service instead of global window.toast.The current implementation augments the global
Windowinterface and directly accesseswindow.toast. While functional, this approach is fragile and doesn't provide type safety or testability.Consider creating a composable like
useToast()that provides a strongly-typed toast API:// composables/useToast.ts export function useToast() { return { success: (message: string, options?: { description?: string }) => { // Implementation }, error: (message: string, options?: { description?: string }) => { // Implementation }, }; }Then use it in the component:
-declare global { - interface Window { - toast?: { - success: (title: string, options: { description?: string }) => void; - error?: (title: string, options: { description?: string }) => void; - }; - } -} - -function showToast(message: string) { - window.toast?.success(message); -} +const toast = useToast(); + +function showToast(message: string) { + toast.success(message); +}
445-465: Consider replacing window.prompt with a proper modal dialog.Line 448 uses
window.prompt()to collect the new folder name. While functional, this provides a poor user experience with limited styling and validation capabilities.Consider using the existing
UModalcomponent pattern (similar to the move/rename modals) to create a dedicated "Create Folder" modal with proper validation and styling. This would provide:
- Better visual consistency with the rest of the UI
- Input validation before submission
- Ability to cancel without dismissing via keyboard shortcuts
- Accessibility improvements
Example structure:
<UModal v-model:open="createFolderModalOpen" title="Create folder"> <template #body> <UInput v-model="newFolderName" placeholder="Folder name" @keydown.enter="confirmCreateFolder" /> </template> <template #footer="{ close }"> <UButton variant="outline" @click="close">Cancel</UButton> <UButton :disabled="!newFolderName.trim()" @click="confirmCreateFolder">Create</UButton> </template> </UModal>web/src/composables/gql/graphql.ts (3)
839-853: FlatOrganizerEntry structure looks solid; minor type nitSchema is coherent for a flattened model. If positions are always integers, prefer Int; if fractional indexing is intended, Float is fine.
17-23: Safer scalar mappings: avoid number for BigInt and any for JSON
- BigInt → number risks precision loss; prefer string (or bigint with custom parsing).
- JSON → any violates the TS guideline; prefer unknown (or a JsonValue type).
Update scalar mappings (ideally in codegen config) and re-generate.
export type Scalars = { ID: { input: string; output: string; } String: { input: string; output: string; } Boolean: { input: boolean; output: boolean; } Int: { input: number; output: number; } Float: { input: number; output: number; } - /** The `BigInt` scalar type represents non-fractional signed whole numeric values. */ - BigInt: { input: number; output: number; } + /** The `BigInt` scalar type represents non-fractional signed whole numeric values. */ + BigInt: { input: string; output: string; } /** A date-time string at UTC, such as 2019-12-03T09:54:33Z, compliant with the date-time format. */ DateTime: { input: string; output: string; } /** The `JSON` scalar type represents JSON values as specified by [ECMA-404](http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf). */ - JSON: { input: any; output: any; } + JSON: { input: JSONValue; output: JSONValue; }Add once near the top (outside this block):
type JSONValue = unknown;As per coding guidelines.
1-5: Generated file noteThis appears code-generated. Please apply fixes in the source GraphQL ops/codegen config and re-run generation; avoid manual edits here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/src/components/Docker/DockerContainerManagement.vue(1 hunks)web/src/components/Docker/DockerContainersTable.vue(1 hunks)web/src/components/Docker/docker-containers.query.ts(1 hunks)web/src/composables/gql/gql.ts(6 hunks)web/src/composables/gql/graphql.ts(20 hunks)web/src/composables/useDockerEditNavigation.ts(1 hunks)web/src/helpers/env.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
web/src/helpers/env.tsweb/src/composables/useDockerEditNavigation.tsweb/src/composables/gql/graphql.tsweb/src/components/Docker/docker-containers.query.tsweb/src/composables/gql/gql.ts
**/components/**/*.vue
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
Some Vue files may require explicit imports like ref or computed due to Nuxt auto-imports not applying in tests
Files:
web/src/components/Docker/DockerContainerManagement.vueweb/src/components/Docker/DockerContainersTable.vue
web/**/*.query.ts
📄 CodeRabbit inference engine (.cursor/rules/web-graphql.mdc)
GraphQL queries must be placed in
.query.tsfilesPlace GraphQL queries in .query.ts files under web/
Files:
web/src/components/Docker/docker-containers.query.ts
web/**/*.{query,mutation}.ts
📄 CodeRabbit inference engine (.cursor/rules/web-graphql.mdc)
All GraphQL under
web/must follow this naming convention
Files:
web/src/components/Docker/docker-containers.query.ts
🧠 Learnings (1)
📚 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:
web/src/composables/gql/gql.ts
🧬 Code graph analysis (2)
web/src/composables/useDockerEditNavigation.ts (1)
web/src/helpers/env.ts (1)
featureFlags(7-10)
web/src/composables/gql/graphql.ts (1)
api/src/unraid-api/cli/generated/graphql.ts (11)
Scalars(11-75)Maybe(3-3)DockerContainerOverviewForm(725-731)DockerMutations(733-739)DockerContainer(703-723)ResolvedOrganizerV1(1914-1918)Notification(1388-1401)NotificationOverview(1432-1436)InputMaybe(4-4)QueryDockerContainerOverviewFormArgs(1743-1745)Exact(5-5)
🔇 Additional comments (15)
web/src/composables/gql/gql.ts (1)
31-44: Generated GraphQL additions look good.The new Docker/Notifications documents and overloads are consistent with the codegen pattern; no runtime logic changes. Please ensure the codegen Babel/SWC plugin is enabled for production to avoid bundle bloat from the in-memory documents map. Based on learnings.
Also applies to: 51-61, 93-106, 113-123, 211-266, 291-294, 319-334
web/src/composables/useDockerEditNavigation.ts (1)
11-16: Remove unused variable.name is computed but unused; clean it up.
- const name = containerName || (container?.names?.[0] || '').replace(/^\//, ''); const templatePath = container?.templatePath; - if (!name || !templatePath) { + if (!templatePath) { return false; }Likely an incorrect or invalid review comment.
web/src/composables/gql/graphql.ts (13)
526-530: Adding ContainerState.PAUSED looks goodEnum extension is consistent with Docker semantics. Ensure UI state handling accounts for PAUSED distinctly from RUNNING/EXITED.
727-733: DockerContainerOverviewForm type: OKMatches the new overview-form query usage; no issues spotted.
1736-1737: dockerContainerOverviewForm query: OKType and args align with usage; looks good.
1800-1803: Query args for overview form: OKskipCache arg is consistent.
1968-1974: Organizer view migration (rootId + flatEntries): fully propagated downstreamVerification confirms all downstream consumers have been updated. No references to the old
.rootproperty remain, and all components, composables, and GraphQL operations consistently userootIdandflatEntries.
1515-1522: warningsAndAlerts field is fully implemented on the server — review comment is incorrectThe
warningsAndAlertsfield exists in the server GraphQL schema (api/generated-schema.graphql:1204), has a resolver implementation (notifications.resolver.ts:55), a model definition (notifications.model.ts:172), and is actively published in subscriptions. No coordination or removal is needed; the client type is correct.Likely an incorrect or invalid review comment.
786-793: DockerTemplateSyncResult type matches server implementationVerification confirms the mutation
syncDockerTemplatePathsin the server GraphQL schema (api/generated-schema.graphql:2466) returnsDockerTemplateSyncResult!with matching field definitions:scanned,matched,skipped, anderrors. All field types align between the server schema and the client TypeScript type definition.
3066-3066: I need to search the codebase directly for GraphQL schema definitions and the codegen configuration to determine iftemplatePathis actually defined server-side.Now let me search the codebase for GraphQL schema files and query definitions to verify if templatePath is actually supported by the server.
Verify that the server exposes
templatePathbefore shipping. This field is present in GetDockerContainers but may cause runtime errors if unsupported.The verification script confirms that
templatePathis indeed included in the GetDockerContainersDocument at line 3066. However, this only confirms the field is present in the generated client query document—it does not verify whether the server's GraphQL schema actually exposes this field. If the server does not support it, queries will fail at runtime with a GraphQL validation error.Before shipping:
- Confirm that the server schema defines
meta.templatePath- Test against the backend to ensure queries don't return errors
- If unsupported, remove
templatePathfrom the query and re-generate
699-703: Schema support for organizer(skipCache) argument is confirmedThe API schema at api/generated-schema.graphql (line 1144) explicitly defines the
organizerfield on theDockertype with theskipCacheargument:organizer(skipCache: Boolean! = false): ResolvedOrganizerV1!. TheDockerOrganizerArgstype is correctly defined and supported by the server. Queries will not fail.Likely an incorrect or invalid review comment.
3083-3083: Field is properly defined and non-nullable; review concern is incorrectThe field
warningsAndAlertsis defined as non-nullable in the GraphQL schema ([Notification!]!) and implemented in the resolver. Since the query is generated from schema, type consistency is guaranteed. The client code safely uses optional chaining as defensive programming. If the field were missing from the server, this would manifest as a schema definition error during code generation, not a runtime failure. This is standard API versioning, not a code defect.Likely an incorrect or invalid review comment.
3090-3090: No API validation needed—mutation is properly implemented and generatedThe
notifyIfUniquemutation is already implemented server-side with the@Mutationdecorator and full deduplication logic. The web-side generated types (NotifyIfUniqueMutation,NotifyIfUniqueDocument) confirm the schema was properly generated. No risk of a 400 error.
724-725: Based on the script execution results, I can confirm the client-side usage pattern but cannot fully verify the server-side schema status. The evidence shows:Confirmed findings:
templatePathis actively selected inweb/src/components/Docker/docker-containers.query.ts:42templatePathis used inweb/src/composables/useDockerEditNavigation.ts(lines 12, 14, 23)- Generated types include
templatePath?: Maybe<Scalars['String']['output']>in theDockerContainermetadata type- Multiple GraphQL documents in
web/src/composables/gql/gql.tsreferencetemplatePathfieldUnable to verify:
The server schema verification failed because the pathapi/src/**/generated/graphql.tsdoesn't exist. Without access to the actual server GraphQL schema definition, I cannot confirm whetherDockerContainertype currently exposes this field.Key observation:
The fact that generated types (graphql.ts) already includetemplatePathas an optional field suggests it was present in the schema during codegen. However, if the server schema has been modified since codegen was last run, there could be a mismatch.
Verify server schema before landing this change. Confirm that the server's
DockerContainertype currently exposes thetemplatePathfield. You can:
- Check
api/src/schema/directory for server GraphQL type definitions- Run GraphQL introspection against the running server endpoint
- Re-run client codegen and verify no validation errors occur
If
templatePathis not exposed by the server, either remove it from the client queries or land the server schema addition first.
737-745: Regenerate server GraphQL schema before deployment—resolver implementations exist but generated types are staleThe server resolvers for
pause()andunpause()are implemented ingit/api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts, but the generated GraphQL schema ingit/api/src/unraid-api/cli/generated/graphql.tsis outdated and does not include these fields in theDockerMutationstype.Server resolvers have:
pause()resolver with@ResolveFielddecoratorunpause()resolver with@ResolveFielddecoratorServer generated schema is missing:
pause: DockerContainerunpause: DockerContainerThe client code is correct and complete. Ensure the server's GraphQL schema is regenerated as part of the build process before deployment to prevent runtime failures when the client calls these mutations.
| export const GET_DOCKER_CONTAINERS = gql` | ||
| query GetDockerContainers($skipCache: Boolean = false) { | ||
| docker { | ||
| id | ||
| organizer(skipCache: $skipCache) { | ||
| version | ||
| views { | ||
| id | ||
| name | ||
| rootId | ||
| flatEntries { | ||
| id | ||
| type | ||
| name | ||
| parentId | ||
| depth | ||
| position | ||
| path | ||
| hasChildren | ||
| childrenIds | ||
| icon | ||
| meta { | ||
| id | ||
| names | ||
| state | ||
| status | ||
| image | ||
| ports { | ||
| privatePort | ||
| publicPort | ||
| type | ||
| } | ||
| autoStart | ||
| hostConfig { | ||
| networkMode | ||
| } | ||
| created | ||
| isUpdateAvailable | ||
| isRebuildReady | ||
| templatePath | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
Add containers to the query to match consumer usage.
Vue expects docker.containers; include it here to prevent empty data.
export const GET_DOCKER_CONTAINERS = gql`
query GetDockerContainers($skipCache: Boolean = false) {
docker {
id
organizer(skipCache: $skipCache) {
version
views {
id
name
rootId
flatEntries {
id
type
name
parentId
depth
position
path
hasChildren
childrenIds
icon
meta {
id
names
state
status
image
ports {
privatePort
publicPort
type
}
autoStart
hostConfig {
networkMode
}
created
isUpdateAvailable
isRebuildReady
templatePath
}
}
}
}
+ containers {
+ id
+ names
+ image
+ created
+ state
+ status
+ autoStart
+ ports {
+ privatePort
+ publicPort
+ type
+ }
+ hostConfig { networkMode }
+ labels
+ networkSettings
+ }
}
}
`;📝 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.
| export const GET_DOCKER_CONTAINERS = gql` | |
| query GetDockerContainers($skipCache: Boolean = false) { | |
| docker { | |
| id | |
| organizer(skipCache: $skipCache) { | |
| version | |
| views { | |
| id | |
| name | |
| rootId | |
| flatEntries { | |
| id | |
| type | |
| name | |
| parentId | |
| depth | |
| position | |
| path | |
| hasChildren | |
| childrenIds | |
| icon | |
| meta { | |
| id | |
| names | |
| state | |
| status | |
| image | |
| ports { | |
| privatePort | |
| publicPort | |
| type | |
| } | |
| autoStart | |
| hostConfig { | |
| networkMode | |
| } | |
| created | |
| isUpdateAvailable | |
| isRebuildReady | |
| templatePath | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| `; | |
| export const GET_DOCKER_CONTAINERS = gql` | |
| query GetDockerContainers($skipCache: Boolean = false) { | |
| docker { | |
| id | |
| organizer(skipCache: $skipCache) { | |
| version | |
| views { | |
| id | |
| name | |
| rootId | |
| flatEntries { | |
| id | |
| type | |
| name | |
| parentId | |
| depth | |
| position | |
| path | |
| hasChildren | |
| childrenIds | |
| icon | |
| meta { | |
| id | |
| names | |
| state | |
| status | |
| image | |
| ports { | |
| privatePort | |
| publicPort | |
| type | |
| } | |
| autoStart | |
| hostConfig { | |
| networkMode | |
| } | |
| created | |
| isUpdateAvailable | |
| isRebuildReady | |
| templatePath | |
| } | |
| } | |
| } | |
| } | |
| containers { | |
| id | |
| names | |
| image | |
| created | |
| state | |
| status | |
| autoStart | |
| ports { | |
| privatePort | |
| publicPort | |
| type | |
| } | |
| hostConfig { networkMode } | |
| labels | |
| networkSettings | |
| } | |
| } | |
| } | |
| `; |
🤖 Prompt for AI Agents
In web/src/components/Docker/docker-containers.query.ts around lines 3 to 49,
the GraphQL query omits the docker.containers field that the Vue consumer
expects, causing empty data; add a containers selection under docker (or under
docker.organizer if consumer expects that shape) that returns the fields the UI
uses (at minimum id and the same meta/container details used elsewhere such as
name, status, image, ports, autoStart, hostConfig, created, isUpdateAvailable,
isRebuildReady, templatePath) so the component receives docker.containers
populated; ensure the selection matches the consumer’s shape.
| const { result, loading, refetch } = useQuery<{ | ||
| docker: { | ||
| id: string; | ||
| organizer: { | ||
| views: Array<{ | ||
| id: string; | ||
| name: string; | ||
| rootId: string; | ||
| flatEntries: FlatOrganizerEntry[]; | ||
| }>; | ||
| }; | ||
| containers: DockerContainer[]; | ||
| }; | ||
| }>(GET_DOCKER_CONTAINERS, { | ||
| fetchPolicy: 'cache-and-network', | ||
| variables: { skipCache: true }, | ||
| }); | ||
There was a problem hiding this comment.
Query/data mismatch: docker.containers is not fetched but is used.
GET_DOCKER_CONTAINERS (as defined) returns organizer/views only, so result.value.docker.containers will be undefined and your containers computed becomes [] always. Fetch containers in the query or stop relying on it.
Apply this fix (paired with the change in docker-containers.query.ts below):
-const { result, loading, refetch } = useQuery<{
- docker: {
- id: string;
- organizer: {
- views: Array<{
- id: string;
- name: string;
- rootId: string;
- flatEntries: FlatOrganizerEntry[];
- }>;
- };
- containers: DockerContainer[];
- };
-}>(GET_DOCKER_CONTAINERS, {
+const { result, loading, refetch } = useQuery(GET_DOCKER_CONTAINERS, {
fetchPolicy: 'cache-and-network',
variables: { skipCache: true },
});Optional (strongly recommended): use generated types for safety.
-import { GET_DOCKER_CONTAINERS } from '@/components/Docker/docker-containers.query';
+import { GetDockerContainersDocument, type GetDockerContainersQuery, type GetDockerContainersQueryVariables } from '@/composables/gql/graphql';
-const { result, loading, refetch } = useQuery(GET_DOCKER_CONTAINERS, {
+const { result, loading, refetch } = useQuery<GetDockerContainersQuery, GetDockerContainersQueryVariables>(GetDockerContainersDocument, {
fetchPolicy: 'cache-and-network',
variables: { skipCache: true },
});Also applies to: 178-179
🤖 Prompt for AI Agents
In web/src/components/Docker/DockerContainerManagement.vue around lines 157-174
(also check lines 178-179), the GraphQL hook expects docker.containers but
GET_DOCKER_CONTAINERS currently only returns organizer/views so
result.value.docker.containers is undefined and your computed containers is
always empty; update the GET_DOCKER_CONTAINERS query to include the containers
selection (id, name, status, etc.) or remove usage of docker.containers and
fetch containers with a separate query, then adjust the useQuery type to the
generated TypeScript types (or at minimum include containers in the local type)
so the hook returns the containers field and the computed property works
correctly.
| id: Scalars['PrefixedID']['input']; | ||
| }>; | ||
|
|
There was a problem hiding this comment.
GetDockerActiveContainer: unused $id variable breaks GraphQL validation
The operation declares $id but never uses it. Many servers reject unused variables. Fix by removing the variable or using it (if a single‑container query exists).
- query GetDockerActiveContainer($id: PrefixedID!) {
- docker {
+ query GetDockerActiveContainer {
+ docker {
id
containers {
id
names
image
created
state
status
autoStart
ports { privatePort publicPort type }
hostConfig { networkMode }
networkSettings
labels
}
}
}Follow-up: update the source .graphql/.gql for this op and re-run codegen so the QueryVariables type drops id.
Also applies to: 3065-3065
🤖 Prompt for AI Agents
In web/src/composables/gql/graphql.ts around lines 2734-2736 (and also at line
3065), the generated GetDockerActiveContainer operation declares a $id variable
that is never used which breaks GraphQL validation; edit the original
.graphql/.gql operation to remove the unused $id parameter (or modify the
operation to actually use $id if the intent was to query a single container),
then re-run your GraphQL codegen so the generated QueryVariables type no longer
includes `id`.
Summary by CodeRabbit
New Features
Improvements