feat: docker overview #1855
feat: docker overview #1855
Conversation
WalkthroughUpdates API version from 4.28.2 to 4.29.2, removes unused imports across Docker service tests and models, eliminates hostname derivation logic and resources parameters from functions, refactors subscription handling and portal/scope management, synchronizes line keys in log viewers, and converts pending state to Vue refs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (7)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
api/**/*📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
api/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Files:
web/**/*📄 CodeRabbit inference engine (CLAUDE.md)
Files:
web/src/**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (11)📚 Learning: 2025-08-09T01:03:29.676ZApplied to files:
📚 Learning: 2025-11-24T17:52:26.907ZApplied to files:
📚 Learning: 2025-11-24T17:52:26.907ZApplied to files:
📚 Learning: 2025-11-24T17:52:26.908ZApplied to files:
📚 Learning: 2025-11-24T17:52:26.908ZApplied to files:
📚 Learning: 2025-11-24T17:52:26.907ZApplied to files:
📚 Learning: 2025-11-24T17:51:46.348ZApplied to files:
📚 Learning: 2025-11-24T17:51:46.348ZApplied to files:
📚 Learning: 2025-11-24T17:52:26.908ZApplied to files:
📚 Learning: 2025-11-24T17:52:26.907ZApplied to files:
📚 Learning: 2024-11-06T20:59:25.809ZApplied to files:
🔇 Additional comments (5)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
Fixed
Show fixed
Hide fixed
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
api/web/src/components/Logs/SingleLogViewer.vue
Lines 280 to 284 in deccf34
Start log subscription on initial render
startLogSubscription is now only called from refreshLogContent (lines 280–281) and the watch on logFilePath at line 284 uses the default non‑immediate behaviour, so nothing invokes the subscription on initial mount. In practice the log viewer no longer starts streaming updates after the first fetch; the live feed and subscription indicator stay inactive unless the user manually hits refresh or changes the file path, regressing the real‑time tailing behaviour.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1855 +/- ##
==========================================
+ Coverage 46.43% 46.48% +0.05%
==========================================
Files 954 954
Lines 59784 59788 +4
Branches 5544 5548 +4
==========================================
+ Hits 27763 27795 +32
+ Misses 31902 31874 -28
Partials 119 119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/app/__test__/app.module.integration.spec.ts (1)
69-107: Add mock cleanup between tests.The test suite doesn't reset mocks between test cases. While less critical in integration tests, this could still lead to test pollution if mock state is modified during test execution.
Recommended fix
afterAll(async () => { if (app) { await app.close(); } }); + + afterEach(() => { + vi.clearAllMocks(); + });Based on learnings from web-testing-rules.
web/src/components/Notifications/graphql/notification.query.ts (1)
1-1: Add.jsextension for ESM compatibility.The import should include
.jsextension for TypeScript ESM compatibility.🔎 Proposed fix
-import { graphql } from '~/composables/gql/gql'; +import { graphql } from '~/composables/gql/gql.js';As per coding guidelines: "Always use TypeScript imports with
.jsextensions for ESM compatibility"
🟡 Minor comments (29)
api/src/core/log.ts-21-26 (1)
21-26: Use the importedSUPPRESS_LOGSconstant for consistency.Line 22 duplicates the logic
process.env.SUPPRESS_LOGS === 'true', which is already encapsulated in the importedSUPPRESS_LOGSconstant (line 4). This creates inconsistency with line 28, which correctly uses the imported constant.🔎 Proposed fix
export const logDestination = - process.env.SUPPRESS_LOGS === 'true' + SUPPRESS_LOGS ? nullDestination : useConsole ? pino.destination(1) // stdout : pino.destination({ dest: PATHS_LOGS_FILE, mkdir: true });web/eslint.config.mjs-71-76 (1)
71-76: Update vue/no-unsupported-features rule to match actual project version.The Vue version change from
^3.3.0to^3.5.0correctly aligns the linting rule with the actual Vue 3.5.20 version in use. Ensurepnpm codegenis run to regenerate any GraphQL types if the updated ESLint rules affect code generation output.web/public/test-pages/all-components.html-378-380 (1)
378-380: Remove the unused event listener or implement the dispatcher.The
unraid:theme-changedevent listener on line 378 has no corresponding dispatcher in the codebase. Either remove this listener or implement the dispatcher that should trigger it when the theme changes (e.g., fromDevThemeSwitcher.standalone.vue'ssetThemeMutation).api/src/unraid-api/auth/api-key.service.ts-186-190 (1)
186-190: Use logger.debug() or logger.warn() instead of logger.error() for non-fatal case.The ENOENT handling treats a missing directory as non-fatal by returning an empty array, but logs it at ERROR level. Since this is an expected scenario (no API keys created yet) that's handled gracefully, consider downgrading to
logger.debug()orlogger.warn()to avoid alarming operators with errors that don't require intervention.🔎 Proposed fix
const files = await readdir(this.basePath).catch((error) => { 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 []; } this.logger.error(`Failed to read API key directory: ${error}`);web/src/composables/useDockerContainerStats.ts-4-6 (1)
4-6: Add.jsextensions to local imports for ESM compatibility.Local TypeScript file imports should include
.jsextensions for ESM compatibility.🔎 Proposed fix
-import { DOCKER_STATS_SUBSCRIPTION } from '@/components/Docker/docker-stats.subscription'; +import { DOCKER_STATS_SUBSCRIPTION } from '@/components/Docker/docker-stats.subscription.js'; -import type { DockerContainerStats } from '@/composables/gql/graphql'; +import type { DockerContainerStats } from '@/composables/gql/graphql.js';As per coding guidelines: "Always use TypeScript imports with
.jsextensions for ESM compatibility"web/src/composables/useDockerEditNavigation.ts-22-35 (1)
22-35: Unusednamevariable.The
namevariable is computed on line 27 but never used -buildLegacyEditUrlonly takestemplatePath. Either remove the unused computation or verify ifnameshould be part of the URL.🔎 Proposed fix if name is not needed
function getLegacyEditUrl(container: DockerContainer | undefined, containerName?: string) { if (!shouldUseLegacyEditPage) { return null; } - const name = containerName || (container?.names?.[0] || '').replace(/^\//, ''); const templatePath = container?.templatePath; - if (!name || !templatePath) { + if (!templatePath) { return null; } return buildLegacyEditUrl(templatePath); }api/src/unraid-api/graph/resolvers/docker/docker.model.ts-1-10 (1)
1-10: Remove unusedIDimport.The static analysis correctly identifies that
IDis imported but never used in this file. All ID fields usePrefixedIDinstead.🔎 Proposed fix
import { Field, Float, GraphQLISODateTime, - ID, InputType, Int, ObjectType, registerEnumType, } from '@nestjs/graphql';web/src/composables/useDockerConsoleSessions.ts-77-78 (1)
77-78: Unhandled fetch error and hardcoded delay.The fetch call doesn't handle errors - if the terminal fails to open, the code proceeds to create a non-functional session. The 300ms delay is also a race condition waiting to happen.
🔎 Suggested improvement
- await fetch(`/webGui/include/OpenTerminal.php?${params.toString()}`); - await new Promise((resolve) => setTimeout(resolve, 300)); + const response = await fetch(`/webGui/include/OpenTerminal.php?${params.toString()}`); + if (!response.ok) { + throw new Error(`Failed to open terminal: ${response.statusText}`); + } + await new Promise((resolve) => setTimeout(resolve, 300));api/src/environment.ts-114-117 (1)
114-117: Review:PATHS_DOCKER_TEMPLATESuses.split(',')which produces empty strings for edge cases.The
.split(',')on line 114 will create empty strings in the array for trailing commas (e.g.,"path1,path2,") or consecutive commas (e.g.,"path1,,path2"). Since this variable represents file paths, empty strings would be invalid entries. Consider filtering them:export const PATHS_DOCKER_TEMPLATES = process.env.PATHS_DOCKER_TEMPLATES?.split(',').filter(Boolean) ?? [ '/boot/config/plugins/dockerMan/templates-user', '/boot/config/plugins/dockerMan/templates', ];api/src/unraid-api/graph/resolvers/docker/docker-log.service.spec.ts-7-7 (1)
7-7: Remove unused import.The
DockerContainerLogsimport is never used in this test file.🔎 Proposed fix
import { AppError } from '@app/core/errors/app-error.js'; import { DockerLogService } from '@app/unraid-api/graph/resolvers/docker/docker-log.service.js'; -import { DockerContainerLogs } from '@app/unraid-api/graph/resolvers/docker/docker.model.js';api/src/unraid-api/graph/resolvers/docker/docker-stats.service.ts-85-112 (1)
85-112: Fix the linter disable comment for Biome.Line 91 uses an ESLint-style disable comment, but the project uses Biome which has a different rule name. The regex is legitimate (removing ANSI escape codes), but the disable comment won't suppress the Biome warning.
Proposed fix
// Remove ANSI escape codes if any (docker stats sometimes includes them) - // eslint-disable-next-line no-control-regex + // biome-ignore lint/suspicious/noControlCharactersInRegex: legitimate use for removing ANSI escape codes const cleanLine = line.replace(/\x1B\[[0-9;]*[mK]/g, '');api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts-34-57 (1)
34-57: Retry mechanism lacks cleanup on service destruction.The
setTimeoutat line 50 could leak if the service is destroyed before all retries complete. Consider using NestJS'sSchedulerRegistryto manage the timeout, or implementOnModuleDestroyto clear pending retries.🔎 Suggested improvement
+import { OnModuleDestroy } from '@nestjs/common'; +import { SchedulerRegistry } from '@nestjs/schedule'; @Injectable() -export class DockerTemplateScannerService { +export class DockerTemplateScannerService implements OnModuleDestroy { private readonly logger = new Logger(DockerTemplateScannerService.name); + private retryTimeout?: NodeJS.Timeout; // ... + constructor( + private readonly dockerConfigService: DockerConfigService, + private readonly dockerService: DockerService, + ) {} + onModuleDestroy() { + if (this.retryTimeout) { + clearTimeout(this.retryTimeout); + } + } @Timeout(5_000) async bootstrapScan(attempt = 1, maxAttempts = 5): Promise<void> { // ... if (attempt < maxAttempts) { this.logger.warn(...); - setTimeout(() => this.bootstrapScan(attempt + 1, maxAttempts), 60_000); + this.retryTimeout = setTimeout(() => this.bootstrapScan(attempt + 1, maxAttempts), 60_000); } }Committable suggestion skipped: line range outside the PR's diff.
api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts-114-117 (1)
114-117: Logger.error argument order may be incorrect.NestJS Logger's
errormethod typically expects the message first, then an optional stack trace. The current order(error, message)may not log correctly.🔎 Suggested fix
- this.logger.error(error, 'Failed to get containers'); + this.logger.error( + `Failed to get containers: ${error instanceof Error ? error.message : 'Unknown error'}`, + error instanceof Error ? error.stack : undefined + );web/src/components/Logs/SingleLogViewer.vue-144-153 (1)
144-153: Direct access to BaseLogViewer's internalscrollViewportRefbreaks encapsulation.Accessing
baseLogViewerRef.value?.scrollViewportRefdirectly couples this component to BaseLogViewer's internal structure. If BaseLogViewer changes its internal refs, this code will break.Consider exposing a method like
getScrollState()orpreserveScrollPosition(callback)from BaseLogViewer instead.🔎 Suggested approach
Add a method to BaseLogViewer:
// In BaseLogViewer function executeWithScrollPreservation(callback: () => Promise<void>) { const prevHeight = scrollViewportRef.value?.scrollHeight || 0; await callback(); nextTick(() => { if (scrollViewportRef.value) { scrollViewportRef.value.scrollTop += scrollViewportRef.value.scrollHeight - prevHeight; } }); } defineExpose({ forceScrollToBottom, executeWithScrollPreservation });Then in SingleLogViewer:
await baseLogViewerRef.value?.executeWithScrollPreservation(() => refetchLogContent());web/src/components/Docker/ContainerOverviewCard.vue-101-111 (1)
101-111: Async timing:tailscaleFetchedis set before data actually loads.
loadTailscaleStatus()returns a Promise, but it's not awaited. SettingtailscaleFetched.value = trueimmediately after the call means the flag is true before the query completes. This affects the refresh button visibility logic but doesn't break functionality.🔎 Proposed fix
watch( () => props.container?.id, - (newId, oldId) => { + async (newId, oldId) => { if (newId && newId !== oldId && isTailscaleEnabled.value && isContainerRunning.value) { tailscaleFetched.value = false; - loadTailscaleStatus(); + await loadTailscaleStatus(); tailscaleFetched.value = true; } }, { immediate: true } );api/docs/developer/docker.md-379-432 (1)
379-432: Data model type mismatch with GraphQL schema.The
createdfield is documented asFloat!(line 391), but the GraphQL schema ingraphql.tsdefines it asInt. This inconsistency could confuse developers.🔎 Suggested fix
- created: Float! + created: Int!api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts-5-8 (1)
5-8: Remove unused importAutoStartEntry.The
AutoStartEntrytype is imported but never used in this test file, as flagged by static analysis.🔎 Suggested fix
-import { - AutoStartEntry, - DockerAutostartService, -} from '@app/unraid-api/graph/resolvers/docker/docker-autostart.service.js'; +import { DockerAutostartService } from '@app/unraid-api/graph/resolvers/docker/docker-autostart.service.js';api/src/unraid-api/graph/resolvers/docker/docker-tailscale.service.ts-93-94 (1)
93-94: Unused variableactualHostname.The
actualHostnamevariable is computed from the DNS name but never used. The status object useshostnamefrom labels instead. If this is intentional (preferring the configured label over the DNS-derived name), consider removing the unused variable. IfactualHostnameshould be used, update line 120 accordingly.🔎 Proposed fix (if intentional - remove unused variable)
const dnsName = rawStatus.Self.DNSName; - const actualHostname = dnsName ? dnsName.split('.')[0] : undefined;web/src/composables/useDockerTableColumns.ts-188-228 (1)
188-228: Addrel="noopener noreferrer"to external links.External links with
target="_blank"should includerel="noopener noreferrer"to prevent the linked page from accessingwindow.opener, which is a security best practice.🔎 Proposed fix
projectUrl ? h( 'a', { href: projectUrl, target: '_blank', + rel: 'noopener noreferrer', title: 'Project Page', class: 'text-gray-500 hover:text-primary-500 dark:text-gray-400 dark:hover:text-primary-400', }, h(UIcon, { name: 'i-lucide-globe', class: 'w-4 h-4' }) ) : null, registryUrl ? h( 'a', { href: registryUrl, target: '_blank', + rel: 'noopener noreferrer', title: 'Registry', class: 'text-gray-500 hover:text-primary-500 dark:text-gray-400 dark:hover:text-primary-400', }, h(UIcon, { name: 'i-lucide-external-link', class: 'w-4 h-4' }) ) : null, supportUrl ? h( 'a', { href: supportUrl, target: '_blank', + rel: 'noopener noreferrer', title: 'Support', class: 'text-gray-500 hover:text-primary-500 dark:text-gray-400 dark:hover:text-primary-400', }, h(UIcon, { name: 'i-lucide-external-link', class: 'w-4 h-4' }) ) : null,web/src/components/Docker/DockerTailscaleIndicator.vue-25-27 (1)
25-27: Query variables won't update ifcontainerIdchanges.The
useLazyQueryis initialized with{ id: props.containerId }, but ifcontainerIdprop changes, the query won't use the new ID unless you pass it explicitly toload()orrefetch(). Therefetchcall on line 48 correctly passes the ID, butload()on line 40 doesn't.🔎 Proposed fix
if (open && !hasFetched.value && isRunning.value) { hasFetched.value = true; - load(); + load(undefined, { id: props.containerId }); }web/src/components/Docker/DockerConsoleViewer.vue-68-84 (1)
68-84: Unhandled fetch error and arbitrary delays.
- The
fetchcall on line 80 has no error handling - if the PHP endpoint fails, the error is silently ignored.- The 100ms and 300ms delays via
setTimeoutare unclear. Consider documenting why these delays are necessary, or using a more robust approach (e.g., waiting for a specific condition).🔎 Proposed fix with error handling
async function openFullscreen() { isPoppedOut.value = true; markPoppedOut(props.containerName); - await new Promise((resolve) => setTimeout(resolve, 100)); + // Brief delay to ensure session state is updated before making the terminal request + await new Promise((resolve) => setTimeout(resolve, 100)); const params = new URLSearchParams({ tag: 'docker', name: props.containerName, more: props.shell, }); - await fetch(`/webGui/include/OpenTerminal.php?${params.toString()}`); - await new Promise((resolve) => setTimeout(resolve, 300)); + try { + await fetch(`/webGui/include/OpenTerminal.php?${params.toString()}`); + // Allow backend to prepare the terminal session + await new Promise((resolve) => setTimeout(resolve, 300)); + } catch (error) { + console.error('Failed to open terminal:', error); + } window.open(socketPath.value, '_blank', 'width=1200,height=800'); }api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts-177-194 (1)
177-194: Multiple port placeholder handling may be incorrect.The code finds the first
[PORT:XXXX]match and resolves it, but the replacement on line 193 uses/\[PORT:\d+\]/gwhich replaces all port placeholders with the same resolved port. If a URL contains multiple different port placeholders (e.g.,http://[IP]:[PORT:80]/api:[PORT:443]), this would incorrectly replace both with the first port's resolution.🔎 Suggested fix for handling multiple port placeholders
- // Replace [PORT:XXXX] placeholder - const portMatch = resolvedUrl.match(/\[PORT:(\d+)\]/); - if (portMatch) { - const templatePort = parseInt(portMatch[1], 10); - let resolvedPort = templatePort; - - // Check if this port is mapped to a public port - if (container.ports) { - for (const port of container.ports) { - if (port.privatePort === templatePort && port.publicPort) { - resolvedPort = port.publicPort; - break; - } - } - } - - resolvedUrl = resolvedUrl.replace(/\[PORT:\d+\]/g, String(resolvedPort)); - } + // Replace all [PORT:XXXX] placeholders + resolvedUrl = resolvedUrl.replace(/\[PORT:(\d+)\]/g, (_, portStr) => { + const templatePort = parseInt(portStr, 10); + if (container.ports) { + for (const port of container.ports) { + if (port.privatePort === templatePort && port.publicPort) { + return String(port.publicPort); + } + } + } + return String(templatePort); + });api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts-262-265 (1)
262-265: Potential undefined access on container.names.
container.names[0]could be undefined if the names array is empty, and the check on line 263 would pass with an empty string, potentially causing issues downstream.🔎 Suggested fix
- const containerName = container.names[0]; - if (!containerName) { + const containerName = container.names?.[0]; + if (!containerName?.length) { return null; }api/src/unraid-api/graph/resolvers/docker/docker-container.resolver.ts-55-126 (1)
55-126: Potential N+1 query pattern for template details.Multiple resolver fields (
projectUrl,registryUrl,supportUrl,iconUrl,shell,templatePorts) independently callgetTemplateDetailson the same container. If a GraphQL query requests multiple fields, this causes redundant parsing of the same template file.Consider using DataLoader or request-scoped caching to deduplicate
getTemplateDetailscalls. DataLoader handles bulk retrieval and caching of requests on a per-request basis, preventing multiple calls for the same template path within a single GraphQL resolution. Alternatively, implement a simple in-memory cache in the resolver or service to batch calls within each request cycle.api/src/unraid-api/graph/resolvers/docker/docker.service.ts-318-344 (1)
318-344: Remove useless initial assignment.The initial assignment
let containers = await this.getContainers({ skipCache: true })on line 324 is immediately overwritten in the first iteration of the loop (line 328). This was flagged by static analysis.🔎 Proposed fix
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 containers: DockerContainer[]; let updatedContainer: DockerContainer | undefined; for (let i = 0; i < 5; i++) { await sleep(500); containers = await this.getContainers({ skipCache: true });web/src/components/Common/BaseTreeTable.vue-420-458 (1)
420-458: Remove debug console.log statements.Multiple
console.logstatements for drag events appear to be debug artifacts. These should be removed before merging to production.🔎 Proposed fix
onMousedown: (e: MouseEvent) => { - console.log('[DragHandle] mousedown', { - rowId: enhancedRow.original.id, - rowName: enhancedRow.original.name, - isBusy, - isDraggingThis, - draggable: !isBusy && !isDraggingThis, - target: e.target, - currentTarget: e.currentTarget, - }); }, onDragstart: (e: DragEvent) => { - console.log('[DragHandle] dragstart fired', { - rowId: enhancedRow.original.id, - rowName: enhancedRow.original.name, - isBusy, - dataTransfer: e.dataTransfer, - }); if (isBusy) { - console.log('[DragHandle] dragstart prevented - row is busy'); e.preventDefault(); return; } handleDragStart(e, enhancedRow.original); - console.log('[DragHandle] handleDragStart called, draggingIds:', draggingIds.value); }, onDragend: (e: DragEvent) => { - console.log('[DragHandle] dragend fired', { - rowId: enhancedRow.original.id, - dropEffect: e.dataTransfer?.dropEffect, - }); handleDragEnd(); }, onDrag: (e: DragEvent) => { - // Log occasionally during drag (throttled by checking if clientX changed significantly) - if (e.clientX % 50 < 5) { - console.log('[DragHandle] dragging...', { x: e.clientX, y: e.clientY }); - } },Note: The
onMousedownandonDraghandlers become empty and can be removed entirely if they serve no other purpose.api/src/unraid-api/organizer/organizer.ts-745-776 (1)
745-776: Remove unusedresourcesparameter.Similar to
moveItemsToPosition, theresourcesparameter is destructured but never used increateFolderWithItems. This was flagged by static analysis.🔎 Proposed fix
export interface CreateFolderWithItemsParams { view: OrganizerView; folderId: string; folderName: string; parentId: string; sourceEntryIds?: string[]; position?: number; - resources?: OrganizerV1['resources']; } export function createFolderWithItems(params: CreateFolderWithItemsParams): OrganizerView { - const { view, folderId, folderName, parentId, sourceEntryIds = [], position, resources } = params; + const { view, folderId, folderName, parentId, sourceEntryIds = [], position } = params;Committable suggestion skipped: line range outside the PR's diff.
api/src/unraid-api/organizer/organizer.ts-685-703 (1)
685-703: Remove unusedresourcesparameter.The
resourcesparameter is destructured but never used inmoveItemsToPosition. This was flagged by static analysis. Either remove it from the interface and function signature, or use it if it was intended for some purpose.🔎 Proposed fix
export interface MoveItemsToPositionParams { view: OrganizerView; sourceEntryIds: Set<string>; destinationFolderId: string; position: number; - resources?: OrganizerV1['resources']; } export function moveItemsToPosition(params: MoveItemsToPositionParams): OrganizerView { - const { view, sourceEntryIds, destinationFolderId, position, resources } = params; + const { view, sourceEntryIds, destinationFolderId, position } = params;Committable suggestion skipped: line range outside the PR's diff.
api/src/unraid-api/graph/resolvers/docker/docker.service.ts-346-372 (1)
346-372: Remove useless initial assignment.Same issue as
pause- the initial assignment on line 352 is immediately overwritten in the loop. This was flagged by static analysis.🔎 Proposed fix
public async unpause(id: string): Promise<DockerContainer> { const container = this.client.getContainer(id); await container.unpause(); 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: DockerContainer[]; let updatedContainer: DockerContainer | undefined; for (let i = 0; i < 5; i++) { await sleep(500); containers = await this.getContainers({ skipCache: true });
| // Check if there's a leftover patch from a previous run that needs to be rolled back | ||
| try { | ||
| await modification.rollback(true); | ||
| this.logger.log(`Rolled back previously applied modification: ${modification.id}`); | ||
| } catch { | ||
| // No patch file exists or rollback failed - this is expected when the modification was never applied | ||
| } |
There was a problem hiding this comment.
Distinguish between expected and unexpected rollback errors.
The empty catch block swallows all errors indiscriminately. While missing patch files are expected, other errors (permissions issues, I/O failures, file corruption) should be logged or handled differently to avoid masking real problems during cleanup.
🔎 Proposed fix
// Check if there's a leftover patch from a previous run that needs to be rolled back
try {
await modification.rollback(true);
this.logger.log(`Rolled back previously applied modification: ${modification.id}`);
- } catch {
- // No patch file exists or rollback failed - this is expected when the modification was never applied
+ } catch (error) {
+ // No patch file exists - this is expected when the modification was never applied
+ if (error instanceof Error && error.message.includes('ENOENT')) {
+ this.logger.debug(`No patch file to rollback for ${modification.id}`);
+ } else {
+ // Log unexpected rollback failures
+ this.logger.warn(`Failed to rollback ${modification.id}: ${error instanceof Error ? error.message : 'Unknown error'}`);
+ }
}🤖 Prompt for AI Agents
In api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts
around lines 130 to 136, the empty catch block after modification.rollback(...)
swallows all errors; change it to catch the error object, detect if it's the
expected "no patch file" case (e.g., error.code === 'ENOENT' or message
indicating missing file) and silently ignore only that case, otherwise log the
unexpected error with this.logger.error including modification.id and the error
details (or rethrow if appropriate) so permission/I/O/corruption issues are not
hidden.
| OnceExit(root) { | ||
| // Remove @layer at-rules after all rules have been scoped. | ||
| // Tailwind CSS v4 uses @layer directives (e.g., @layer utilities, @layer components) | ||
| // to organize CSS. After the Rule hook scopes all selectors within these layers, | ||
| // the @layer wrappers are no longer needed in the final output. | ||
| // | ||
| // This cleanup step: | ||
| // 1. Extracts all scoped rules from inside @layer blocks | ||
| // 2. Moves them to the parent container (outside the @layer) | ||
| // 3. Removes the now-empty @layer wrapper | ||
| // | ||
| // This produces cleaner CSS output, avoids potential browser compatibility issues | ||
| // with CSS layers, and ensures the final CSS only contains the scoped rules without | ||
| // the organizational layer structure. | ||
| root.walkAtRules('layer', (atRule: AtRule) => { | ||
| const removableAtRule = atRule as RemovableAtRule; | ||
| const parent = atRule.parent as ParentContainer | undefined; | ||
| if (!parent) { | ||
| return; | ||
| } | ||
|
|
||
| // Extract all nodes from the @layer and move them to the parent | ||
| if ( | ||
| Array.isArray(removableAtRule.nodes) && | ||
| removableAtRule.nodes.length > 0 && | ||
| typeof (parent as ParentContainer).insertBefore === 'function' | ||
| ) { | ||
| const parentContainer = parent as ParentContainer; | ||
| while (removableAtRule.nodes.length) { | ||
| const node = removableAtRule.nodes[0]!; | ||
| parentContainer.insertBefore?.(atRule as unknown as Container, node); | ||
| } | ||
| } | ||
|
|
||
| // Remove the empty @layer wrapper | ||
| if (typeof removableAtRule.remove === 'function') { | ||
| removableAtRule.remove(); | ||
| return; | ||
| } | ||
|
|
||
| (parent as ParentContainer).removeChild?.(atRule as unknown as Container); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reduce type casting to align with coding guidelines.
As per coding guidelines, "Avoid using casting whenever possible, prefer proper typing from the start." The implementation contains multiple type assertions (lines 196, 197, 208) and two double-cast escape hatches using as unknown as Container (lines 211, 221), which indicate the type definitions don't fully match the runtime API.
Consider importing PostCSS types directly or refining the custom type definitions to eliminate the need for casting, particularly the double-cast pattern.
Based on coding guidelines.
Simplify comments to reduce verbosity.
The comment block (lines 182-194) is quite verbose. As per coding guidelines, "Never add comments for obvious things." The functionality could be summarized more concisely since the code itself is reasonably self-explanatory.
Based on coding guidelines.
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>All Components - Unraid Component Test</title> | ||
| <script src="https://code.jquery.com/jquery-3.7.1.min.js"></script> |
There was a problem hiding this comment.
Add Subresource Integrity (SRI) check for CDN-loaded jQuery.
Loading jQuery from a CDN without integrity and crossorigin attributes exposes the page to potential CDN compromise. Even for test pages, this is a security best practice.
🔎 Proposed fix to add SRI attributes
- <script src="https://code.jquery.com/jquery-3.7.1.min.js"></script>
+ <script src="https://code.jquery.com/jquery-3.7.1.min.js"
+ integrity="sha256-/JqT3SQfawRcv/BIHPThkBvs0OEvtFFmqPF/lYI/Cxo="
+ crossorigin="anonymous"></script>📝 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.
| <script src="https://code.jquery.com/jquery-3.7.1.min.js"></script> | |
| <script src="https://code.jquery.com/jquery-3.7.1.min.js" | |
| integrity="sha256-/JqT3SQfawRcv/BIHPThkBvs0OEvtFFmqPF/lYI/Cxo=" | |
| crossorigin="anonymous"></script> |
🤖 Prompt for AI Agents
In web/public/test-pages/all-components.html around line 7, the jQuery CDN
script tag lacks Subresource Integrity and crossorigin attributes; add an
integrity attribute containing the correct SRI hash (use sha384 for jQuery 3.7.1
minified), add crossorigin="anonymous", and ensure the integrity value exactly
matches the file served by the CDN (or regenerate the hash from the downloaded
file and use that value) so the browser can verify the resource and mitigate CDN
compromise.
| const orderedColumns = computed(() => { | ||
| if (!props.table?.tableApi) return []; | ||
| const availableColumns = props.table.tableApi.getAllColumns().filter((column) => column.getCanHide()); | ||
| const columnIds = availableColumns.map((col) => col.id); | ||
| const order = props.columnOrder && props.columnOrder.length > 0 ? props.columnOrder : columnIds; | ||
| columnOrderState.value = order; | ||
| const columnMap = new Map(availableColumns.map((col) => [col.id, col])); | ||
| const ordered = order | ||
| .map((id) => columnMap.get(id)) | ||
| .filter((col): col is NonNullable<typeof col> => col !== undefined); | ||
| const missing = availableColumns.filter((col) => !order.includes(col.id)); | ||
| return [...ordered, ...missing]; | ||
| }); |
There was a problem hiding this comment.
Side effect in computed property: assignment to columnOrderState.
Line 37 assigns to columnOrderState.value inside a computed property, which is a side effect. Computed properties should be pure derivations. This can cause reactivity issues or unexpected re-computations.
🔎 Proposed fix - use a watcher instead
+watch(
+ [() => props.table?.tableApi, () => props.columnOrder],
+ ([tableApi, propOrder]) => {
+ if (!tableApi) return;
+ const availableColumns = tableApi.getAllColumns().filter((column) => column.getCanHide());
+ const columnIds = availableColumns.map((col) => col.id);
+ columnOrderState.value = propOrder && propOrder.length > 0 ? propOrder : columnIds;
+ },
+ { immediate: true }
+);
const orderedColumns = computed(() => {
if (!props.table?.tableApi) return [];
const availableColumns = props.table.tableApi.getAllColumns().filter((column) => column.getCanHide());
- const columnIds = availableColumns.map((col) => col.id);
-
- const order = props.columnOrder && props.columnOrder.length > 0 ? props.columnOrder : columnIds;
- columnOrderState.value = order;
const columnMap = new Map(availableColumns.map((col) => [col.id, col]));
- const ordered = order
+ const ordered = columnOrderState.value
.map((id) => columnMap.get(id))
.filter((col): col is NonNullable<typeof col> => col !== undefined);
- const missing = availableColumns.filter((col) => !order.includes(col.id));
+ const missing = availableColumns.filter((col) => !columnOrderState.value.includes(col.id));
return [...ordered, ...missing];
});📝 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 orderedColumns = computed(() => { | |
| if (!props.table?.tableApi) return []; | |
| const availableColumns = props.table.tableApi.getAllColumns().filter((column) => column.getCanHide()); | |
| const columnIds = availableColumns.map((col) => col.id); | |
| const order = props.columnOrder && props.columnOrder.length > 0 ? props.columnOrder : columnIds; | |
| columnOrderState.value = order; | |
| const columnMap = new Map(availableColumns.map((col) => [col.id, col])); | |
| const ordered = order | |
| .map((id) => columnMap.get(id)) | |
| .filter((col): col is NonNullable<typeof col> => col !== undefined); | |
| const missing = availableColumns.filter((col) => !order.includes(col.id)); | |
| return [...ordered, ...missing]; | |
| }); | |
| watch( | |
| [() => props.table?.tableApi, () => props.columnOrder], | |
| ([tableApi, propOrder]) => { | |
| if (!tableApi) return; | |
| const availableColumns = tableApi.getAllColumns().filter((column) => column.getCanHide()); | |
| const columnIds = availableColumns.map((col) => col.id); | |
| columnOrderState.value = propOrder && propOrder.length > 0 ? propOrder : columnIds; | |
| }, | |
| { immediate: true } | |
| ); | |
| const orderedColumns = computed(() => { | |
| if (!props.table?.tableApi) return []; | |
| const availableColumns = props.table.tableApi.getAllColumns().filter((column) => column.getCanHide()); | |
| const columnMap = new Map(availableColumns.map((col) => [col.id, col])); | |
| const ordered = columnOrderState.value | |
| .map((id) => columnMap.get(id)) | |
| .filter((col): col is NonNullable<typeof col> => col !== undefined); | |
| const missing = availableColumns.filter((col) => !columnOrderState.value.includes(col.id)); | |
| return [...ordered, ...missing]; | |
| }); |
🤖 Prompt for AI Agents
In web/src/components/Common/TableColumnMenu.vue around lines 30 to 48, the
computed property orderedColumns currently assigns to columnOrderState.value
(side effect). Remove that assignment so the computed is a pure derivation that
only returns the ordered list; then add a separate watcher (or watchEffect) that
observes props.columnOrder and props.table?.tableApi (or availableColumns
derived inputs) and updates columnOrderState.value with the chosen order (use
props.columnOrder when present, otherwise the derived columnIds), and make the
watcher immediate so columnOrderState is initialized without causing side
effects inside the computed.
| import { gql } from '@apollo/client'; | ||
|
|
||
| export const RESET_DOCKER_TEMPLATE_MAPPINGS = gql` | ||
| mutation ResetDockerTemplateMappings { | ||
| resetDockerTemplateMappings | ||
| } | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the mutation file exists and verify its structure
if [ -f "web/src/components/Docker/docker-reset-template-mappings.mutation.ts" ]; then
echo "=== File exists ==="
cat -n web/src/components/Docker/docker-reset-template-mappings.mutation.ts
else
echo "File not found"
fi
# Check if there's a pnpm codegen command in the web directory
if [ -f "web/package.json" ]; then
echo -e "\n=== Checking for codegen script in web/package.json ==="
grep -A 2 -B 2 "codegen" web/package.json || echo "No codegen script found"
fi
# Check for GraphQL code generation configuration files
echo -e "\n=== GraphQL codegen config files ==="
fd -e "codegen" -e "graphql.config" web/ 2>/dev/null || echo "No codegen config found"Repository: unraid/api
Length of output: 1040
Run pnpm codegen in the web directory to generate TypeScript types for this new mutation before merging.
🤖 Prompt for AI Agents
In web/src/components/Docker/docker-reset-template-mappings.mutation.ts lines
1-7: you added a new GraphQL mutation but did not generate TypeScript types for
it; run `pnpm codegen` from the web/ directory to regenerate the client types,
verify the generated files include the new ResetDockerTemplateMappings types,
and commit the updated generated files before merging.
| :busy-row-ids="busyRowIds" | ||
| :can-expand="() => false" | ||
| :can-select="(row: any) => row.type === 'container'" | ||
| :can-drag="() => true" | ||
| :can-drop-inside="() => false" | ||
| v-model:selected-ids="selectedIds" | ||
| @row:drop="handleDrop" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid using any type.
The can-select callback uses any type which violates coding guidelines. Use proper typing instead.
🔎 Suggested fix
- :can-select="(row: any) => row.type === 'container'"
+ :can-select="(row: TreeRow<AutostartEntry>) => row.type === 'container'"🤖 Prompt for AI Agents
In web/src/components/Docker/DockerAutostartSettings.vue around lines 400-407,
the can-select callback uses the unsafe any type; replace it with a proper typed
row parameter (e.g., define or import an interface such as AutostartRow |
DockerRow that includes at least a "type: string" field) and update the callback
signature to (row: AutostartRow) => row.type === 'container'; ensure the script
block uses lang="ts" and import or declare the interface in the component scope
so type checking is enforced.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
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/organizer/organizer.ts (1)
673-686: Complete the refactor by removingresourcesfrom the interface definition.The implementation correctly no longer destructures the
resourcesparameter, but the interfaceMoveItemsToPositionParamsstill declares it as an optional property on line 678. Remove this property from the interface to complete the refactor and prevent API confusion.🔎 Proposed fix to remove resources from the interface
export interface MoveItemsToPositionParams { view: OrganizerView; sourceEntryIds: Set<string>; destinationFolderId: string; position: number; - resources?: OrganizerV1['resources']; }
♻️ Duplicate comments (3)
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts (1)
5-5: LGTM! Unused import removed.This cleanup addresses the GitHub Advanced Security warning about the unused
AutoStartEntryimport.api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
16-16: LGTM! Unused import removed.This cleanup addresses the GitHub Advanced Security warning about the unused
ContainerPortTypeimport. The remaining imports (ContainerStateandDockerContainer) are correctly retained as they're used in the test cases.api/src/unraid-api/organizer/organizer.ts (1)
731-746: Complete the refactor by removingresourcesfrom the interface definition.The implementation correctly no longer destructures the
resourcesparameter, but the interfaceCreateFolderWithItemsParamsstill declares it as an optional property on line 738. Remove this property from the interface to complete the refactor and maintain API consistency.🔎 Proposed fix to remove resources from the interface
export interface CreateFolderWithItemsParams { view: OrganizerView; folderId: string; folderName: string; parentId: string; sourceEntryIds?: string[]; position?: number; - resources?: OrganizerV1['resources']; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/dev/configs/api.jsonapi/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-log.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-tailscale.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.model.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/organizer/organizer.tsweb/src/components/Common/BaseTreeTable.vueweb/src/components/Docker/SingleDockerLogViewer.vueweb/src/components/Logs/SingleLogViewer.vueweb/src/components/Notifications/CriticalNotifications.standalone.vueweb/src/composables/useContainerActions.ts
💤 Files with no reviewable changes (3)
- api/src/unraid-api/graph/resolvers/docker/docker-tailscale.service.ts
- api/src/unraid-api/graph/resolvers/docker/docker-log.service.spec.ts
- api/src/unraid-api/graph/resolvers/docker/docker.model.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/Notifications/CriticalNotifications.standalone.vue
- web/src/components/Common/BaseTreeTable.vue
- web/src/components/Logs/SingleLogViewer.vue
- api/dev/configs/api.json
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.tsapi/src/unraid-api/organizer/organizer.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsweb/src/composables/useContainerActions.ts
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.tsapi/src/unraid-api/organizer/organizer.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.tsapi/src/unraid-api/organizer/organizer.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsweb/src/composables/useContainerActions.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.tsapi/src/unraid-api/organizer/organizer.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.tsapi/src/unraid-api/organizer/organizer.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsweb/src/composables/useContainerActions.tsweb/src/components/Docker/SingleDockerLogViewer.vue
web/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Always run
pnpm codegenfor GraphQL code generation in the web directory
Files:
web/src/composables/useContainerActions.tsweb/src/components/Docker/SingleDockerLogViewer.vue
web/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure Vue reactivity imports are added to store files (computed, ref, watchEffect)
Files:
web/src/composables/useContainerActions.ts
🧠 Learnings (11)
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
Repo: unraid/api PR: 1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: This is a Nuxt.js app but tests are run with vitest outside of the Nuxt environment
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/*.test.ts : Mock external services and API calls
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.908Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.908Z
Learning: Applies to **/__test__/store/**/*.ts : Test Pinia store getter dependencies are properly mocked
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.908Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.908Z
Learning: Applies to **/__test__/store/**/*.ts : Mock external dependencies appropriately in Pinia store tests
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in Vue component tests
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:51:46.348Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-11-24T17:51:46.348Z
Learning: Applies to api/**/*.test.{ts,tsx} : Prefer to not mock simple dependencies
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:51:46.348Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-11-24T17:51:46.348Z
Learning: Applies to api/**/*.test.{ts,tsx} : Use Vitest for the test suite, not Jest
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.908Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.908Z
Learning: Applies to **/__test__/store/**/*.ts : Use factory functions for module mocks in Pinia store tests to avoid hoisting issues
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/*.test.ts : Use `vi.mock()` for module-level mocks
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-autostart.service.spec.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
Repo: unraid/api PR: 942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
🔇 Additional comments (5)
web/src/composables/useContainerActions.ts (2)
42-47: Excellent fix for the shared state issue!The conversion from module-scoped
letarrays to function-scopedrefarrays correctly addresses the race condition identified in the previous review. Each composable instance now maintains its own pending state, eliminating cross-component interference.
204-260: All ref accesses are correctly implemented.All assignments, reads, and resets consistently use
.valueto access the reactive state. The implementation properly follows Vue 3's ref API conventions.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (2)
324-324: LGTM! Explicit type annotation added.The explicit type annotation for
containersis appropriate and aligns with TypeScript best practices. The variable is assigned within the loop before use, so TypeScript's control flow analysis correctly recognizes it as initialized.
352-352: LGTM! Explicit type annotation added.The explicit type annotation for
containersis appropriate and aligns with TypeScript best practices. The variable is assigned within the loop before use, so TypeScript's control flow analysis correctly recognizes it as initialized.web/src/components/Docker/SingleDockerLogViewer.vue (1)
71-74: Memory leak fix implemented correctly.The code now properly removes
lineKeysentries when trimming old log lines, preventing unbounded Set growth. The slice captures the lines being removed, and the loop deletes their corresponding keys before truncating the array. This directly addresses the previous review concern.
Summary by CodeRabbit
Release Notes v4.29.2
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.