Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces a new custom Docker stack management service, replacing previous stack lifecycle functions and API endpoints with enhanced capabilities. The confirm dialog component now supports dynamic checkboxes, allowing users to specify options such as removing stack files and volumes. Stack API methods and routes are updated to match the new service, and related UI components are adjusted to align with these backend changes. Several obsolete server-side handlers are removed or refactored, and utility functions are re-exported for external use. Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
src/routes/api/stacks/[stackId]/down/+server.ts (2)
13-17:⚠️ Potential issueError messages don't match the operation being performed.
The error messages still refer to "starting" a stack instead of "stopping" it, which doesn't align with the actual operation being performed by the stopStack function.
- console.error(`API Error starting stack ${id}:`, result.error); + console.error(`API Error stopping stack ${id}:`, result.error); const response: ApiErrorResponse = { success: false, - error: result.error.message || 'Failed to start stack', + error: result.error.message || 'Failed to stop stack', code: ApiErrorCode.INTERNAL_SERVER_ERROR, details: result.error
23-26:⚠️ Potential issueSuccess message doesn't match the operation being performed.
The success message indicates "Stack started successfully" but this endpoint is for stopping a stack.
return json({ success: true, - message: `Stack started successfully` + message: `Stack stopped successfully` });src/lib/services/docker/stack-service.ts (2)
94-105: 🛠️ Refactor suggestion
⚠️ Potential issueSanitize
stackIdmore rigorously to avoid directory-traversal & collision issues
path.basename()strips traversal tokens (../) but also discards every path segment but the last one, so an attacker could craft two different IDs (../../fooandfoo) that both resolve to the same directory, or even override another legitimate stack.
It is safer to:
- Reject IDs that are not plain file/dir names (
^[a-zA-Z0-9._-]+$or a similar tight regex).- Throw instead of silently coercing the ID – this will surface mis-usage early.
-const safeId = path.basename(stackId); // Use path.basename for safety -if (safeId !== stackId) { - console.warn(`Original stackId "${stackId}" was sanitized to "${safeId}". Ensure this is expected.`); -} -return path.join(stacksDirAbs, safeId); +if (!/^[A-Za-z0-9_.-]+$/.test(stackId)) { + throw new Error(`Invalid stack id "${stackId}" – contains unsupported characters or path separators.`); +} +return path.join(stacksDirAbs, stackId);
1630-1687: 💡 Verification agent🧩 Analysis chain
Unresolved
${VAR}placeholders are only logged – consider fail-fast to avoid broken deployments
normalizeHealthcheckTestlogs a critical message when acontainer_namestill contains${VAR}but continues execution.
If a variable is actually missing,dockerwill refuse to create the container later and the user receives a much more obscure error. Failing early here will save time and make the root cause explicit.-// depending on desired behavior, you might throw an error here -// throw new Error(`Unresolved variable in container_name …`); +throw new Error( + `Unresolved variable in container_name for service '${serviceName}': ${service.container_name}. ` + + `Make sure the corresponding environment variable is defined.` +);
🏁 Script executed:
#!/bin/bash # Locate all references to normalizeHealthcheckTest to assess impact of throwing an error rg -n "normalizeHealthcheckTest" -n .Length of output: 1213
Fail fast on unresolved
${VAR}incontainer_nameby throwing an error
Currently,normalizeHealthcheckTestlogs a critical message when it detects an unresolved variable inservice.container_namebut then continues processing. This defers the failure to Docker with an opaque error. To surface the root cause immediately, throw an exception instead of silently proceeding.Locations to update:
src/lib/services/docker/stack-service.ts, inside thenormalizeHealthcheckTestfunction’sif (modified)block, within the loop overdoc.services.Suggested diff:
- if (service && typeof service.container_name === 'string' && service.container_name.includes('${')) { - console.error( - `CRITICAL: Unresolved variable in container_name for service '${serviceName}': ${service.container_name}. ` + - `This will likely cause Docker to fail. Ensure the environment variable is defined.` - ); - // Depending on desired behavior, you might throw an error here - // throw new Error(`Unresolved variable in container_name for service '${serviceName}': ${service.container_name}`); - } + if (service && typeof service.container_name === 'string' && service.container_name.includes('${')) { + throw new Error( + `Unresolved variable in container_name for service '${serviceName}': ${service.container_name}. ` + + `Make sure the corresponding environment variable is defined.` + ); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 1650-1650: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (16)
src/routes/api/containers/[containerId]/logs/stream/+server.ts (3)
36-38: Use optional chaining for cleaner code.Consider using optional chaining for more concise code.
- if (logStream && logStream.destroy) { - logStream.destroy(); - } + logStream?.destroy?.();🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
48-48: Avoid usinganytype for better type safety.Replace
anywith a more specific error type for better type safety.- } catch (error: any) { + } catch (error: unknown) {If you need to access properties like
error.code, add a type guard:if (error instanceof Error && 'code' in error) { if (error.code === 'ERR_INVALID_STATE') { // ... } }
85-87: Use optional chaining for cleaner code.As with the previous instance, optional chaining would make this code more concise.
- if (self.logStream && self.logStream.destroy) { - self.logStream.destroy(); - } + self.logStream?.destroy?.();🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/routes/api/stacks/[stackId]/deploy/+server.ts (2)
12-21: Terminology mismatch in error message.The error message refers to "starting" a stack, but the function and endpoint are about "deploying" a stack. This could create confusion in logs and error tracking.
- console.error(`API Error starting stack ${id}:`, result.error); + console.error(`API Error deploying stack ${id}:`, result.error); const response: ApiErrorResponse = { success: false, - error: result.error.message || 'Failed to start stack', + error: result.error.message || 'Failed to deploy stack', code: ApiErrorCode.INTERNAL_SERVER_ERROR, details: result.error };
23-27: Terminology mismatch in success message.The success message should use "deployed" instead of "started" to maintain consistency with the endpoint name and function.
return json({ success: true, - message: `Stack started successfully` + message: `Stack deployed successfully` });src/routes/api/stacks/[stackId]/destroy/+server.ts (2)
24-32: HTTP status code may not be appropriate for all error cases.The error handler returns status 409 (Conflict) for all errors, but a general failure might be better represented as 500 (Internal Server Error), reserving 409 for specific conflict scenarios.
- return json(response, { status: 409 }); + return json(response, { status: 500 });
41-47: Terminology inconsistency in error message.While other messages were updated to use "destroy" terminology, this error message still uses "remove".
const response: ApiErrorResponse = { success: false, - error: 'Failed to remove stack', + error: 'Failed to destroy stack', code: ApiErrorCode.BAD_REQUEST };src/lib/services/api/stack-api-service.ts (2)
19-32: Remove debugging console logs from production code.The
destroymethod contains console logging statements that should be removed before deploying to production.async destroy(id: string, removeVolumes = false, removeFiles = false) { - console.log('API service - removeVolumes:', removeVolumes, 'removeFiles:', removeFiles); - const queryParams = { removeVolumes: removeVolumes ? 'true' : 'false', removeFiles: removeFiles ? 'true' : 'false' }; - console.log('Query params:', queryParams); - const res = await this.api.delete(`/stacks/${id}/destroy`, { params: queryParams }); return res.data; }
22-31: Simplify query parameter handling.The query parameters approach could be optimized to only include the parameters when they're true, reducing request size.
const queryParams = { - removeVolumes: removeVolumes ? 'true' : 'false', - removeFiles: removeFiles ? 'true' : 'false' + ...(removeVolumes && { removeVolumes: 'true' }), + ...(removeFiles && { removeFiles: 'true' }) };src/lib/components/confirm-dialog/confirm-dialog.svelte (2)
9-27: Implement state management with potential improvementsThe code adds reactive state management for checkboxes in the confirm dialog, which works well. However, there are a couple of improvements to consider:
- The console.log statement on line 24 should be removed before production.
- There appears to be duplicate initialization logic for checkbox states.
let checkboxStates = $state<Record<string, boolean>>({}); $effect(() => { if ($confirmDialogStore.open && $confirmDialogStore.checkboxes) { const newStates: Record<string, boolean> = {}; for (const checkbox of $confirmDialogStore.checkboxes) { newStates[checkbox.id] = Boolean(checkbox.initialState); } checkboxStates = newStates; } }); function handleConfirm() { - console.log('Final checkbox states before confirm:', checkboxStates); $confirmDialogStore.confirm.action(checkboxStates); $confirmDialogStore.open = false; }
43-59: Improve checkbox rendering logicThe checkbox rendering implementation works, but has unnecessary conditional logic in lines 48-52 that can be simplified.
<!-- Checkboxes --> {#if $confirmDialogStore.checkboxes && $confirmDialogStore.checkboxes.length > 0} <div class="flex flex-col gap-3 pt-4 pb-2 mt-4 border-t border-border"> {#each $confirmDialogStore.checkboxes as checkbox (checkbox.id)} <div class="flex items-center space-x-2"> - {#if checkboxStates[checkbox.id] !== undefined} - <Checkbox id={checkbox.id} bind:checked={checkboxStates[checkbox.id]} aria-labelledby={`${checkbox.id}-label`} /> - {:else} - <Checkbox id={checkbox.id} checked={false} onchange={(e) => (checkboxStates[checkbox.id] = true)} aria-labelledby={`${checkbox.id}-label`} /> - {/if} + <Checkbox id={checkbox.id} bind:checked={checkboxStates[checkbox.id]} aria-labelledby={`${checkbox.id}-label`} /> <Label id={`${checkbox.id}-label`} for={checkbox.id} class="text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70"> {checkbox.label} </Label> </div> {/each} </div> {/if}The conditional rendering is unnecessary since the
$effecthook already initializes all checkbox states. The fallback case could lead to unexpected behavior by forcibly setting the state to true.src/lib/components/action-buttons.svelte (1)
59-67: Remove debug logging codeThe code contains debug console.log statements that should be removed before deployment to production.
confirm: { label: type === 'stack' ? 'Destroy' : 'Remove', destructive: true, action: async (checkboxStates) => { - console.log('Debug - received checkbox states:', checkboxStates); - // Ensure these are proper booleans const removeFiles = checkboxStates['removeFiles'] === true; const removeVolumes = checkboxStates['removeVolumes'] === true; - - console.log('Debug - removeFiles:', removeFiles, 'removeVolumes:', removeVolumes); - isLoading.remove = true;src/routes/stacks/[stackId]/+page.server.ts (1)
84-107: Consider removing commented codeThe file contains a large block of commented-out code that was likely part of the previous implementation. Since this is part of a refactoring effort, it would be cleaner to remove this code entirely rather than keeping it as comments.
-// export const actions: Actions = { -// update: async ({ params, request }) => { -// const { stackId } = params; -// const formData = await request.formData(); - -// const name = formData.get('name')?.toString() || ''; -// const composeContent = formData.get('composeContent')?.toString() || ''; -// const autoUpdate = formData.get('autoUpdate') === 'on'; - -// const result = await tryCatch(updateStack(stackId, { name, composeContent, autoUpdate })); -// if (!result.error) { -// return { -// success: true, -// message: 'Stack updated successfully' -// }; -// } else { -// console.error('Error updating stack:', result.error); -// return { -// success: false, -// error: result.error instanceof Error ? result.error.message : 'Failed to update stack' -// }; -// } -// } -// };This would improve code maintenance and readability. If needed, you can always reference this code from version control history.
src/lib/services/docker/stack-custom-service.ts (3)
298-325: Race-condition when removing an existent container – addforce: true& wait for stop
createAndStartContainersremoves a pre-existing container:if (existingContainers[0].State === 'running') { await container.stop(); } await container.remove();Docker may still have the container in
removal in progressstate; a rapid subsequentcreateContainerwith the same name can fail withConflict. The container name is already in use.
Usingstop({t:10})andremove({force: true}), then polling until the container disappears (or just retrycreateContaineron 409) hardens the flow.
548-595: Use optional chaining to silence undefined-access & simplify codeStatic analysis already flagged the
composeData.volumes && composeData.volumes[source]pattern. Optional chaining keeps the intent while avoiding accidentalundefinedlook-ups.-if (composeData.volumes && composeData.volumes[source]) { +if (composeData.volumes?.[source]) {You can apply the same pattern in a few sibling expressions inside this block.
🧰 Tools
🪛 Biome (1.9.4)
[error] 581-581: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
311-315:docker.listContainersfilter format differs across calls – standardise to avoid subtle bugsHere you send a plain object:
filters: { name: [containerName] }Whereas other calls (
cleanupFailedDeployment,restartStack) JSON-stringify the filters.
Both ways might work today thanks to dockerode’s implicit serialisation, yet they rely on undocumented behaviour and complicate grepping for identical logic.
For consistency (and to match Docker’s spec) always passJSON.stringify({...}).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/lib/components/action-buttons.svelte(3 hunks)src/lib/components/confirm-dialog/confirm-dialog.svelte(2 hunks)src/lib/components/confirm-dialog/index.ts(3 hunks)src/lib/services/api/stack-api-service.ts(2 hunks)src/lib/services/docker/auto-update-service.ts(2 hunks)src/lib/services/docker/stack-custom-service.ts(1 hunks)src/lib/services/docker/stack-service.ts(5 hunks)src/routes/api/containers/[containerId]/logs/stream/+server.ts(1 hunks)src/routes/api/stacks/[stackId]/deploy/+server.ts(1 hunks)src/routes/api/stacks/[stackId]/destroy/+server.ts(3 hunks)src/routes/api/stacks/[stackId]/down/+server.ts(2 hunks)src/routes/api/stacks/[stackId]/pull/+server.ts(0 hunks)src/routes/api/stacks/[stackId]/redeploy/+server.ts(1 hunks)src/routes/api/stacks/[stackId]/restart/+server.ts(1 hunks)src/routes/api/stacks/[stackId]/stop/+server.ts(0 hunks)src/routes/stacks/+page.svelte(2 hunks)src/routes/stacks/[stackId]/+page.server.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/routes/api/stacks/[stackId]/pull/+server.ts
- src/routes/api/stacks/[stackId]/stop/+server.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/routes/api/stacks/[stackId]/down/+server.ts (3)
src/lib/utils/try-catch.ts (1)
tryCatch(14-21)src/lib/services/docker/stack-custom-service.ts (1)
stopStack(830-840)src/lib/services/docker/stack-service.ts (1)
stopStack(1160-1263)
src/lib/services/docker/auto-update-service.ts (1)
src/lib/services/docker/stack-custom-service.ts (1)
redeployStack(845-867)
src/routes/api/stacks/[stackId]/deploy/+server.ts (4)
src/lib/utils/try-catch.ts (1)
tryCatch(14-21)src/lib/services/docker/stack-custom-service.ts (1)
deployStack(31-108)src/lib/types/errors.type.ts (1)
ApiErrorResponse(19-25)src/lib/types/errors.ts (1)
ApiErrorCode(4-17)
src/routes/api/stacks/[stackId]/destroy/+server.ts (3)
src/lib/utils/try-catch.ts (1)
tryCatch(14-21)src/lib/services/docker/stack-custom-service.ts (1)
destroyStack(927-1018)src/lib/types/errors.type.ts (1)
ApiErrorResponse(19-25)
🪛 Biome (1.9.4)
src/routes/api/containers/[containerId]/logs/stream/+server.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/lib/services/docker/stack-custom-service.ts
[error] 581-581: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (27)
src/routes/api/containers/[containerId]/logs/stream/+server.ts (5)
2-8: Good interface design for stream handling.The new custom interface provides clean type safety for the stream source while properly extending the standard
UnderlyingDefaultSource. This helps TypeScript understand the additionallogStreamproperty needed for cleanup operations.
11-13: Improved error handling with early return.The early validation pattern for containerId enhances user experience by returning a descriptive 400 response immediately, rather than letting the error occur later in the request lifecycle.
19-79: Well-structured stream implementation with robust error handling.The ReadableStream implementation properly manages the Docker log stream lifecycle with appropriate error handling and resource cleanup. The state tracking with
isControllerClosedprevents potential issues with operations on closed controllers.🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
81-89: Good resource cleanup on client disconnect.The cancel handler properly cleans up resources when clients disconnect, preventing memory leaks and hanging connections.
🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
91-97: Correct SSE headers setup.The response includes all the necessary headers for Server-Sent Events (SSE), ensuring proper browser handling of the streaming response.
src/routes/api/stacks/[stackId]/restart/+server.ts (1)
3-3: The import switch to stack-custom-service looks good.The change properly aligns with the broader refactoring effort to centralize Docker stack operations in the new custom service implementation.
src/routes/api/stacks/[stackId]/down/+server.ts (2)
1-1: Import updated correctly to use the new stack-custom-service.This change properly aligns with the refactoring effort to use the new custom stack implementation.
10-10: Function call updated correctly to use stopStack.The implementation now uses the new stopStack function from the custom service.
src/lib/services/docker/auto-update-service.ts (2)
2-3: Imports properly separated between services.The imports have been correctly refactored to maintain the list/get operations from stack-service while importing the redeploy functionality from the new stack-custom-service.
152-152: Function call updated to use the new redeployStack implementation.The call has been correctly updated to use the new implementation from stack-custom-service.
src/routes/api/stacks/[stackId]/redeploy/+server.ts (2)
3-3: Import correctly updated to use the new implementation.The change properly aligns with the broader refactoring effort to centralize Docker stack operations in the new custom service.
10-10: Function call updated to use the new redeployStack implementation.The call has been correctly updated to use the new implementation from stack-custom-service.
src/routes/api/stacks/[stackId]/deploy/+server.ts (2)
1-5: Good import structure and utility usage.The imports look appropriate, with proper use of the new custom stack service and utility functions for error handling.
7-11: LGTM: Clean implementation of the request handler.The handler correctly extracts the stackId from params and uses tryCatch for robust error handling.
src/routes/stacks/+page.svelte (1)
48-51: API method usage updated appropriately.The change from
stackApi.start(id)tostackApi.deploy(id)correctly aligns with the renamed API method.src/routes/api/stacks/[stackId]/destroy/+server.ts (4)
3-5: Good update to use the new service implementation.Import updated properly to use the new custom stack service implementation.
7-10: Well-implemented query parameter handling.The handler now correctly extracts the optional removal preferences from query parameters.
21-22: Good implementation of the new destroyStack function call.The function now properly passes the removal preferences to the service.
37-39: Good enhanced success message.The success message now conditionally includes information about file removal when applicable.
src/lib/services/api/stack-api-service.ts (2)
4-7: Method renamed to match the new endpoint pattern.The
startmethod has been appropriately renamed todeployto match the updated API endpoint.
9-12: Endpoint updated to match the new API structure.The endpoint for stopping a stack has been correctly updated to use
/downinstead of/stop.src/lib/components/action-buttons.svelte (2)
81-84: Consider adding a conditional check for checkboxesThe checkboxes are currently always added to the confirm dialog for stacks, but they're only relevant for stack destruction. Consider conditionally adding them only for stack removal.
The implementation is well-structured, providing clear options for users when destroying a stack. This enhances the UX by explicitly allowing users to choose whether to remove associated files and volumes.
112-113: API method name change from "start" to "deploy"The function now correctly uses
stackApi.deploy()instead ofstackApi.start()for stacks, aligning with the backend API changes.This change correctly implements the new API naming convention, ensuring consistent terminology between frontend and backend.
src/routes/stacks/[stackId]/+page.server.ts (1)
1-7: Import cleanup after refactoringThe imports have been appropriately updated to reflect the architectural changes.
The simplified imports align with the move from server-side actions to API endpoints, removing unnecessary imports that are no longer used after the refactoring.
src/lib/components/confirm-dialog/index.ts (3)
4-18: Well-structured TypeScript interface for dialog storeThe implementation of a proper TypeScript interface for the confirm dialog store is a significant improvement. It clearly defines the structure and enhances type safety throughout the codebase.
The interface is well-designed with optional and required properties clearly marked. The addition of the checkboxes array with typed properties (id, label, initialState) provides good structure for the enhanced functionality.
42-48: Updated action callback signature for checkbox statesThe action callback now correctly accepts a record of checkbox states, aligning with the implementation in the dialog component.
This change ensures type safety when passing checkbox states from the dialog component to the callback function. The signature
action: (checkboxStates: Record<string, boolean>) => voidproperly defines the expected input structure.
58-59: Store update includes checkboxes propertyThe confirmDialogStore update now correctly includes the checkboxes property, ensuring it's passed to the dialog component.
This small but crucial change ensures that the checkbox configurations are properly passed to the store when opening the dialog.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores