Conversation
|
Warning Rate limit exceeded@kmendell has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThis update introduces a complete "Create Container" feature to the application. It adds backend logic for container creation using Docker, type definitions for container configuration and metadata, and a new API endpoint to handle container creation requests. On the frontend, a dialog component is implemented for users to specify container settings, which is integrated into the containers page. The page now allows users to open the dialog, submit container creation requests, and receive feedback through toast notifications. Additional Docker resource data (volumes, networks, images) is fetched and provided to the dialog for configuration options. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContainersPage
participant CreateDialog
participant API as /api/containers (POST)
participant DockerService
User->>ContainersPage: Click "Create Container"
ContainersPage->>CreateDialog: Open dialog
User->>CreateDialog: Fill form, submit
CreateDialog->>ContainersPage: onSubmit(containerConfig)
ContainersPage->>API: POST /api/containers (containerConfig)
API->>DockerService: createContainer(config)
DockerService->>Docker: Create and start container
Docker-->>DockerService: Container details
DockerService-->>API: Container details
API-->>ContainersPage: Success response
ContainersPage->>ContainersPage: Show toast, refresh list
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 4
🧹 Nitpick comments (7)
src/routes/api/containers/+server.ts (1)
10-16: Consider additional validation for container configuration.While basic validation is implemented for required fields, consider adding validation for other critical configuration elements like port mappings (valid port ranges), volume paths, and environment variables to prevent potential issues or security risks.
// Example additional validation if (!config.name || !config.image) { return json( { success: false, error: "Container name and image are required" }, { status: 400 } ); } +// Validate port mappings if provided +if (config.ports?.length) { + for (const port of config.ports) { + const hostPort = parseInt(port.hostPort); + const containerPort = parseInt(port.containerPort); + + if (isNaN(hostPort) || hostPort < 1 || hostPort > 65535 || + isNaN(containerPort) || containerPort < 1 || containerPort > 65535) { + return json( + { success: false, error: "Invalid port mapping. Ports must be between 1-65535." }, + { status: 400 } + ); + } + } +}src/lib/services/docker-service.ts (2)
403-419: Remove redundant null checks in port configuration.The code contains redundant null checks that aren't necessary since the objects were already initialized earlier in the function.
if (config.ports && config.ports.length > 0) { containerOptions.ExposedPorts = {}; containerOptions.HostConfig = containerOptions.HostConfig || {}; containerOptions.HostConfig.PortBindings = {}; config.ports.forEach((port) => { const containerPort = `${port.containerPort}/tcp`; - if (!containerOptions.ExposedPorts) containerOptions.ExposedPorts = {}; containerOptions.ExposedPorts[containerPort] = {}; - if (!containerOptions.HostConfig) containerOptions.HostConfig = {}; - if (!containerOptions.HostConfig.PortBindings) - containerOptions.HostConfig.PortBindings = {}; containerOptions.HostConfig.PortBindings[containerPort] = [ { HostPort: port.hostPort }, ]; }); }
403-419: Add support for UDP port bindings.The current implementation only supports TCP ports (adding "/tcp" suffix). Consider supporting UDP ports as well for more versatile container configurations.
if (config.ports && config.ports.length > 0) { containerOptions.ExposedPorts = {}; containerOptions.HostConfig = containerOptions.HostConfig || {}; containerOptions.HostConfig.PortBindings = {}; config.ports.forEach((port) => { - const containerPort = `${port.containerPort}/tcp`; + // Extract protocol from containerPort or default to tcp + let containerPort = port.containerPort; + let protocol = "tcp"; + + if (containerPort.includes("/")) { + const parts = containerPort.split("/"); + containerPort = parts[0]; + protocol = parts[1].toLowerCase(); + } + + const portWithProtocol = `${containerPort}/${protocol}`; - if (!containerOptions.ExposedPorts) containerOptions.ExposedPorts = {}; - containerOptions.ExposedPorts[containerPort] = {}; + containerOptions.ExposedPorts[portWithProtocol] = {}; - if (!containerOptions.HostConfig) containerOptions.HostConfig = {}; - if (!containerOptions.HostConfig.PortBindings) - containerOptions.HostConfig.PortBindings = {}; - containerOptions.HostConfig.PortBindings[containerPort] = [ + containerOptions.HostConfig.PortBindings[portWithProtocol] = [ { HostPort: port.hostPort }, ]; }); }src/routes/containers/create-container-dialog.svelte (3)
14-16: Consider extending the onClose function to reset form state.The onClose function only sets the open state to false, but it doesn't reset the form state. This means if a user opens the dialog, fills in some values, closes it, and then reopens it, the previous values will still be present.
export function onClose() { open = false; + // Reset form state + containerName = ""; + selectedImage = ""; + ports = [{ hostPort: "", containerPort: "" }]; + volumeMounts = [{ source: "", target: "" }]; + envVars = [{ key: "", value: "" }]; + network = ""; + restartPolicy = "no"; }
134-191: Basic settings form is well-organized but could benefit from a bit more validation.The basic settings tab properly handles container name, image selection, and restart policy. However, consider adding a pattern validation for container name to ensure it follows Docker naming conventions.
<Input id="container-name" bind:value={containerName} placeholder="e.g., my-container" disabled={isCreating} + pattern="^[a-zA-Z0-9][a-zA-Z0-9_.-]*$" + title="Container name must start with a letter or number and can include letters, numbers, underscores, periods, and hyphens" />
250-310: Volume mount implementation lacks host path option.The volume mount implementation only allows selecting from existing named volumes. Consider adding an option for users to specify host paths directly, which is a common use case in Docker.
<div> <Label for={`volume-source-${index}`} class="mb-2 block"> Source Volume </Label> + <div class="grid grid-cols-1 gap-2"> + <Select.Root + type="single" + bind:value={mount.sourceType} + disabled={isCreating} + > + <Select.Trigger class="w-full"> + <span>{mount.sourceType || "Volume Type"}</span> + </Select.Trigger> + <Select.Content> + <Select.Item value="volume">Named Volume</Select.Item> + <Select.Item value="bind">Host Path</Select.Item> + </Select.Content> + </Select.Root> + </div> + + {#if mount.sourceType === 'volume'} <Select.Root type="single" bind:value={mount.source} disabled={isCreating} > <Select.Trigger class="w-full"> <span>{mount.source || "Select volume"}</span> </Select.Trigger> <Select.Content> {#each volumes as volume} <Select.Item value={volume.name}> {volume.name} </Select.Item> {/each} </Select.Content> </Select.Root> + {:else} + <Input + id={`volume-host-path-${index}`} + bind:value={mount.source} + placeholder="/host/path" + disabled={isCreating} + /> + {/if} </div>Note: You'll need to update your state to include sourceType:
let volumeMounts = $state<{ source: string; target: string; sourceType?: string }[]>([ { source: "", target: "", sourceType: "volume" }, ]);src/lib/types/docker.ts (1)
17-35: ContainerConfig interface provides clear structure for container creation.The interface is well-defined with all essential properties for container configuration. However, consider adding resource limits which are important for container management.
export interface ContainerConfig { name: string; image: string; ports?: Array<{ hostPort: string; containerPort: string; }>; volumes?: Array<{ source: string; target: string; }>; envVars?: Array<{ key: string; value: string; }>; network?: string; restart?: "no" | "always" | "on-failure" | "unless-stopped"; + resources?: { + cpuShares?: number; + memory?: number; // in MB + memorySwap?: number; + }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/services/docker-service.ts(2 hunks)src/lib/types/docker.ts(2 hunks)src/routes/api/containers/+server.ts(1 hunks)src/routes/containers/+page.server.ts(1 hunks)src/routes/containers/+page.svelte(4 hunks)src/routes/containers/create-container-dialog.svelte(1 hunks)src/routes/stacks/[stackId]/+page.svelte(0 hunks)
💤 Files with no reviewable changes (1)
- src/routes/stacks/[stackId]/+page.svelte
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/routes/containers/+page.server.ts (1)
src/lib/services/docker-service.ts (4)
listContainers(157-181)listVolumes(611-632)listNetworks(576-597)listImages(471-509)
src/lib/services/docker-service.ts (1)
src/lib/types/docker.ts (2)
ContainerConfig(18-35)ContainerCreate(13-15)
🔇 Additional comments (35)
src/routes/containers/+page.server.ts (4)
1-6: LGTM: Clean import organization.The additional imports for Docker resource functions are well organized and properly imported from the docker-service module.
11-17: Excellent use of Promise.all for concurrency.Using Promise.all to fetch all Docker resources in parallel is a performance optimization that reduces the total request time compared to sequential fetching, especially when dealing with multiple API calls.
19-24: LGTM: Properly returning fetched resources.The return object correctly includes all the fetched Docker resources to be consumed by the UI components.
26-33: LGTM: Comprehensive error handling.Good error handling with proper logging and returning empty arrays for all resource types. This ensures consistent data structure even when errors occur.
src/routes/api/containers/+server.ts (4)
1-5: LGTM: Clean imports and type usage.Imports are well organized and the appropriate types are being used from the docker module.
6-17: Good validation of required fields.The endpoint properly validates that essential fields (name and image) are present before proceeding with container creation, returning a 400 status when validation fails.
18-23: LGTM: Proper handling of container creation.The endpoint correctly calls the createContainer service function and returns a success response with the container details.
24-33: LGTM: Comprehensive error handling.Good error handling with appropriate error logging and returning a descriptive error message with a 500 status code when container creation fails.
src/routes/containers/+page.svelte (7)
8-10: LGTM: Good use of toast notifications and dialog component.The addition of toast notifications provides useful feedback to users, and importing the CreateContainerDialog component follows good component composition practices.
17-18: LGTM: Clean state management.The state variables for dialog visibility and creation status are well defined and follow the Svelte state management pattern.
41-43: LGTM: Simple dialog open handler.A clean, straightforward function to open the container creation dialog.
45-79: Well-structured container creation flow.The container creation handler is well-implemented with proper state management, error handling, and user feedback via toast notifications. The timeout before refreshing the data is a good UX touch to ensure the new container appears in the list.
146-150: LGTM: Well-placed create container button.The create container button is appropriately placed in the header of the container list card.
176-185: LGTM: Good empty state UX.Adding both refresh and create container buttons in the empty state provides a good user experience when no containers are found.
189-196: LGTM: Well-integrated dialog component.The CreateContainerDialog component is properly integrated with the parent component's state and provided with all the necessary data.
src/lib/services/docker-service.ts (7)
1-9: LGTM: Clean imports and type definitions.The additional type imports for container configuration and details are properly included.
381-385: Good function documentation.The function is well-documented with appropriate JSDoc comments explaining the purpose and return value.
386-401: LGTM: Clean container options initialization.The container creation options are properly initialized with the basic required properties from the configuration.
421-427: LGTM: Clean volume binding configuration.Volume binding configuration is properly implemented with a clean mapping from the container configuration to Docker's required format.
429-433: LGTM: Proper network configuration.Network mode is correctly set when provided in the configuration.
435-448: LGTM: Well-structured container creation flow.The container creation, starting, and inspection flow is well-implemented with a clean return of the relevant container details.
449-452: LGTM: Proper error handling.Good error handling with appropriate error logging and message propagation.
src/routes/containers/create-container-dialog.svelte (9)
1-12: Imports are well organized by category and purpose.The component properly imports all necessary UI components and types needed for the dialog implementation.
18-25: Props interface is well-structured and typed.The Props interface clearly defines the expected input properties for the component with appropriate TypeScript types.
27-34: LGTM: Props binding looks good and uses modern Svelte syntax.The component uses Svelte 5's new
$bindable,$state, and$propsfeatures correctly, with sensible defaults.
37-58: State initialization is well-structured.The internal state management is clearly organized with descriptive variable names and appropriate initial values.
61-83: Array manipulation functions are concise and effective.The functions to add and remove array items for ports, volume mounts, and environment variables are implemented consistently and correctly.
115-122: Dialog header is well-structured and informative.The dialog header provides a clear title and description of the purpose of the dialog.
124-131: Tab layout provides good organization of complex form.The tabs effectively separate the container configuration into logical sections, making the form more manageable.
366-389: Network selection is well-implemented.The network selection dropdown correctly handles both the default bridge network and custom networks.
394-415: Dialog footer has appropriate button states and visual feedback.The footer correctly disables actions during creation and shows a loading indicator. The submit button is appropriately disabled when required fields are missing.
src/lib/types/docker.ts (4)
1-2: Good import of Dockerode types.The import from 'dockerode' ensures type compatibility with the underlying Docker API library.
11-15: ContainerCreate interface extends dockerode types correctly.Extending the ContainerCreateOptions with an optional name property is appropriate as the Docker API requires the name to be specified separately from the create options.
45-58: ContainerState interface is comprehensive.The interface captures all relevant container state properties for monitoring and management.
37-43: 🛠️ Refactor suggestionContainerPort interface is missing protocol specification.
Docker port mappings can specify protocols (tcp/udp/sctp), but this is not reflected in the interface.
export interface ContainerPort { IP?: string; PrivatePort: number; PublicPort?: number; Type: string; + Protocol?: "tcp" | "udp" | "sctp"; }Likely an incorrect or invalid review comment.
Docstrings generation was requested by @kmendell. * #53 (comment) The following files were modified: * `src/lib/services/docker-service.ts`
|
Note Generated docstrings for this pull request at #54 |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
♻️ Duplicate comments (2)
src/routes/containers/create-container-dialog.svelte (2)
73-101: Robust port validation with helpful error messagesThe port validation logic is comprehensive, checking for valid numbers, proper range (1-65535), and flagging privileged ports (<1024) with appropriate warnings.
298-309: 🧹 Nitpick (assertive)Improve port input types for better validation
The current port inputs use text fields with pattern validation, but number inputs would be more appropriate.
<Input id={`host-port-${index}`} bind:value={port.hostPort} - placeholder="8080" - disabled={isCreating} - type="text" - pattern="[0-9]*" - inputmode="numeric" + placeholder="8080" + disabled={isCreating} + type="number" + min="1" + max="65535" + aria-invalid={port.hostError && port.hostPort ? "true" : undefined} class={port.hostError && port.hostPort ? "border-red-500" : ""} />
🛑 Comments failed to post (10)
src/routes/api/containers/+server.ts (4)
1-1: 🧹 Nitpick (assertive)
The
redirectimport is unusedYou're importing
redirectbut not using it anywhere in this file.-import { json, redirect } from "@sveltejs/kit"; +import { json } from "@sveltejs/kit";📝 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.import { json } from "@sveltejs/kit";
11-16: 🛠️ Refactor suggestion
Add more comprehensive validation of the container configuration
The current validation only checks if
nameandimageexist, but doesn't validate their format or other potentially required fields.Consider implementing more thorough validation:
// Validate required fields if (!config.name || !config.image) { return json( { success: false, error: "Container name and image are required" }, { status: 400 } ); } + // Validate container name format (alphanumeric, dashes, underscores only) + if (!/^[a-zA-Z0-9_-]+$/.test(config.name)) { + return json( + { success: false, error: "Container name must contain only alphanumeric characters, dashes, and underscores" }, + { status: 400 } + ); + } + + // Validate ports if provided + if (config.ports) { + for (const port of config.ports) { + const hostPort = parseInt(port.hostPort, 10); + const containerPort = parseInt(port.containerPort, 10); + if (isNaN(hostPort) || hostPort < 1 || hostPort > 65535 || + isNaN(containerPort) || containerPort < 1 || containerPort > 65535) { + return json( + { success: false, error: "Invalid port number. Ports must be between 1 and 65535" }, + { status: 400 } + ); + } + } + }📝 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.if (!config.name || !config.image) { return json( { success: false, error: "Container name and image are required" }, { status: 400 } ); } // Validate container name format (alphanumeric, dashes, underscores only) if (!/^[a-zA-Z0-9_-]+$/.test(config.name)) { return json( { success: false, error: "Container name must contain only alphanumeric characters, dashes, and underscores" }, { status: 400 } ); } // Validate ports if provided if (config.ports) { for (const port of config.ports) { const hostPort = parseInt(port.hostPort, 10); const containerPort = parseInt(port.containerPort, 10); if ( isNaN(hostPort) || hostPort < 1 || hostPort > 65535 || isNaN(containerPort) || containerPort < 1 || containerPort > 65535 ) { return json( { success: false, error: "Invalid port number. Ports must be between 1 and 65535" }, { status: 400 } ); } } }
24-33: 🧹 Nitpick (assertive)
Improve error handling for more specific error messages
The current error handling catches all errors in a single catch block and provides a generic message.
Consider more specific error handling:
} catch (error: any) { console.error("Error creating container:", error); + + // Provide more specific error messages based on error type + let status = 500; + let message = "Failed to create container"; + + if (error.name === "TypeError" || error.name === "SyntaxError") { + status = 400; + message = "Invalid container configuration: " + error.message; + } else if (error.message?.includes("network") || error.message?.includes("Network")) { + message = "Network configuration error: " + error.message; + } else if (error.message?.includes("volume") || error.message?.includes("Volume")) { + message = "Volume configuration error: " + error.message; + } else if (error.message?.includes("image") || error.message?.includes("Image")) { + message = "Image configuration error: " + error.message; + } + return json( { success: false, - error: error.message || "Failed to create container", + error: error.message || message, }, - { status: 500 } + { status } ); }Committable suggestion skipped: line range outside the PR's diff.
6-34: 💡 Verification agent
🧩 Analysis chain
Add CSRF protection to your API endpoint
This POST endpoint creates resources but lacks CSRF protection, which is important for endpoints called from browser contexts to prevent cross-site request forgery attacks.
Consider adding CSRF protection using SvelteKit's built-in mechanisms:
export const POST: RequestHandler = async ({ request }) => { try { + // Verify CSRF token or use SvelteKit's CSRF protection + // This could be handled via cookies and headers validation + const config = (await request.json()) as ContainerConfig; // Validate required fields
🌐 Web query:
How to implement CSRF protection in SvelteKit API endpoints?💡 Result:
How to Implement CSRF Protection in SvelteKit API Endpoints
SvelteKit has built-in CSRF protection, but for advanced use cases or tailored behavior, you may need to customize or implement your own logic. Here’s a practical guide based on the latest practices and community recommendations.
1. Understand SvelteKit's Default CSRF Protection
- SvelteKit checks the
Originheader of incoming POST (and, as of 1.15.1, also PUT, PATCH, DELETE) requests to ensure they match your server's origin. This protection targets form submissions and API requests with content-types likeapplication/x-www-form-urlencoded,multipart/form-data, andtext/plain[4][5][10].- This behavior is controlled by the
csrfconfig in yoursvelte.config.js:// svelte.config.js kit: { csrf: { checkOrigin: true // (default) } }
2. Disabling/Customizing Built-in CSRF Protection
If you need finer control (e.g., exclude certain API endpoints, or handle CSRF differently), you can disable SvelteKit's default check and implement your own:
- To disable built-in CSRF protection:
This is necessary if you want to write your own CSRF middleware or need to allow legitimate cross-origin requests (e.g., OAuth callbacks, webhooks)[2][7].// svelte.config.js kit: { csrf: { checkOrigin: false } }
3. Implementing Custom CSRF Middleware
Implement custom CSRF protection in a
src/hooks.server.ts(or.js) file. Here's a robust pattern, adapted from community examples[1][2]:// src/hooks.server.ts import { error, json, text, type Handle } from '@sveltejs/kit'; function isFormContentType(request: Request) { const type = request.headers.get('content-type')?.split(';', 1)[0].trim() ?? ''; return ['application/x-www-form-urlencoded', 'multipart/form-data', 'text/plain'].includes(type); } function csrfProtection(allowedPaths: string[] = []): Handle { return async ({ event, resolve }) => { const { request, url } = event; // Only check POST, PUT, PATCH, DELETE requests with form content types const protectedMethod = ['POST', 'PUT', 'PATCH', 'DELETE'].includes(request.method); const isAllowedPath = allowedPaths.includes(url.pathname); if ( protectedMethod && isFormContentType(request) && request.headers.get('origin') !== url.origin && !isAllowedPath ) { const csrfError = error(403, `Cross-site ${request.method} form submissions are forbidden`); if (request.headers.get('accept') === 'application/json') { return json(csrfError.body, { status: csrfError.status }); } return text(csrfError.body.message, { status: csrfError.status }); } return resolve(event); }; } // Export the custom handle for SvelteKit to use export const handle = csrfProtection(['/api/webhook', '/auth/callback']);
- This checks the origin for protected HTTP methods and supported content types, while allowing specific paths to bypass CSRF protection (e.g., webhooks or OAuth callbacks)[1][2].
4. Additional Best Practices
- Cookies: If using cookies for authentication, set
SameSite=StrictorLaxto help mitigate CSRF risk[8][10].- Tokens: For APIs primarily consumed via JavaScript, consider using double-submit cookies or CSRF tokens stored as HTTP-only cookies or in local storage (though HTTP-only cookies are safest for session cookies).
- HTTPS: Always serve your API over HTTPS.
- Authentication: Implement strong authentication and authorization for all sensitive endpoints[9].
Summary Table
Method Description Built-in SvelteKit Origin check on POST/PUT/PATCH/DELETE (enabled by default)[4][10] Custom Middleware Use a handlehook to conditionally check origin or token[1][2]Cookie Settings Set SameSitetoStrict/Laxfor session cookies[8][10]Excluding Endpoints Allow-list certain paths in your custom CSRF handler[1][2]
In most cases, SvelteKit’s default CSRF protection is sufficient. For more control or exceptions, use a handle hook and tailor your logic as shown above. Always complement CSRF protection with strong cookies and authentication practices.
Citations:
- 1: sveltejs/kit#6784
- 2: https://dev.to/maxiviper117/implementing-csrf-protection-in-sveltekit-3afb
- 3: GHSA-5p75-vc5g-8rv2
- 4: https://svelte.dev/docs/kit/configuration
- 5: https://www.cvedetails.com/cve/CVE-2023-29003/
- 6: https://sveltekit.io/blog/sveltekit-api
- 7: https://scottspence.com/posts/csrf-with-sveltekit-on-flyio
- 8: https://docs.directus.io/blog/directus-auth-sveltekit
- 9: https://www.cerbos.dev/blog/api-security-best-practices
- 10: https://lucia-auth.com/sessions/cookies/sveltekit
Enforce CSRF protection for JSON POST requests
SvelteKit’s default CSRF “Origin” check only applies to form submissions (e.g.
application/x-www-form-urlencoded,multipart/form-data,text/plain), notapplication/json. Since this endpoint consumes JSON, you should explicitly verify theOriginheader (or implement a token‐based check) before handling the request.• Add a global hook at
src/hooks.server.tsto block cross‐site JSON writes:// src/hooks.server.ts import type { Handle } from '@sveltejs/kit'; export const handle: Handle = async ({ event, resolve }) => { const { request, url } = event; // Protect JSON‐based POST/PUT/PATCH/DELETE against CSRF if ( ['POST','PUT','PATCH','DELETE'].includes(request.method) && request.headers.get('content-type')?.startsWith('application/json') ) { const origin = request.headers.get('origin'); if (origin !== url.origin) { return new Response( JSON.stringify({ success: false, error: 'CSRF validation failed' }), { status: 403, headers: { 'content-type': 'application/json' } } ); } } return resolve(event); };• (Optional) For per‐route enforcement, prepend this check in your handler:
export const POST: RequestHandler = async ({ request, url }) => { + const origin = request.headers.get('origin'); + if (origin !== url.origin) { + return json({ success: false, error: 'Forbidden' }, { status: 403 }); + } const config = (await request.json()) as ContainerConfig; … };• Ensure any auth cookies use
SameSite=LaxorStrict.These changes close the gap where JSON‐only endpoints bypass SvelteKit’s built‐in CSRF origin check.
src/routes/containers/+page.server.ts (3)
1-1: 🧹 Nitpick (assertive)
The
redirectimport is unusedYou're importing
redirectbut not using it anywhere in this file.-import { fail, redirect } from "@sveltejs/kit"; +import { fail } from "@sveltejs/kit";📝 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.-import { fail, redirect } from "@sveltejs/kit"; +import { fail } from "@sveltejs/kit";
66-66:
⚠️ Potential issueRemove debug logging before deployment to production
This log statement outputs the entire container configuration, which could include sensitive information like environment variables with passwords or tokens.
- // Log for debugging - console.log("Creating container with data:", containerData); + // Log just the container name for audit purposes + console.log("Creating container:", containerData.name);📝 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.// Log just the container name for audit purposes console.log("Creating container:", containerData.name);
53-55:
⚠️ Potential issueAdd error handling for JSON parsing
The current code assumes that the JSON string is always well-formed, but parsing could fail if the data is malformed.
- const containerData = JSON.parse( - containerDataString.toString() - ) as ContainerConfig; + let containerData: ContainerConfig; + try { + containerData = JSON.parse(containerDataString.toString()) as ContainerConfig; + } catch (parseError) { + return fail(400, { + success: false, + error: "Invalid container configuration format", + }); + }📝 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.let containerData: ContainerConfig; try { containerData = JSON.parse(containerDataString.toString()) as ContainerConfig; } catch (parseError) { return fail(400, { success: false, error: "Invalid container configuration format", }); }src/routes/containers/+page.svelte (1)
71-73: 🧹 Nitpick (assertive)
Replace setTimeout with more modern approach
Using setTimeout with a zero delay to trigger the form submission is a workaround pattern that could be replaced with cleaner alternatives.
- // This is key - explicitly submit the form after setting the data - setTimeout(() => { - formRef.requestSubmit(); - }, 0); + // Use queueMicrotask for cleaner async handling + queueMicrotask(() => { + formRef.requestSubmit(); + });📝 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.// Use queueMicrotask for cleaner async handling queueMicrotask(() => { formRef.requestSubmit(); });src/routes/containers/create-container-dialog.svelte (2)
162-207: 🛠️ Refactor suggestion
Add validation for container names
While port validation is handled well, the component doesn't validate container names against Docker naming rules.
function handleSubmit() { if (!selectedImage || !containerName.trim()) return; + // Validate container name (alphanumeric, dashes, underscores only) + if (!/^[a-zA-Z0-9_-]+$/.test(containerName.trim())) { + // Show an error message or toast notification + return; + } + // Validate all ports let hasInvalidPort = false; ports.forEach((port) => { // Check for invalid ports with content📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function handleSubmit() { if (!selectedImage || !containerName.trim()) return; // Validate container name (alphanumeric, dashes, underscores only) if (!/^[a-zA-Z0-9_-]+$/.test(containerName.trim())) { // Show an error message or toast notification return; } // Validate all ports let hasInvalidPort = false; ports.forEach((port) => { // Check for invalid ports with content if ( (port.hostPort && !validatePortNumber(port.hostPort).isValid) || (port.containerPort && !validatePortNumber(port.containerPort).isValid) ) { hasInvalidPort = true; } }); if (hasInvalidPort) { return; // Stop submission if there are invalid ports } // Filter out empty entries const filteredPorts = ports .filter((p) => p.hostPort.trim() && p.containerPort.trim()) .map(({ hostPort, containerPort }) => ({ hostPort, containerPort })); // Remove error properties const filteredVolumes = volumeMounts.filter( (v) => v.source.trim() && v.target.trim() ); const filteredEnvVars = envVars.filter((e) => e.key.trim()); const containerConfig: ContainerConfig = { name: containerName.trim(), image: selectedImage, ports: filteredPorts.length > 0 ? filteredPorts : undefined, volumes: filteredVolumes.length > 0 ? filteredVolumes : undefined, envVars: filteredEnvVars.length > 0 ? filteredEnvVars : undefined, network: network || undefined, restart: restartPolicy as | "no" | "always" | "on-failure" | "unless-stopped", }; // Pass the data to the parent component which handles the form submission onSubmit(containerConfig); }
377-409: 🛠️ Refactor suggestion
Validate volume mount target paths
The component doesn't validate that volume target paths are absolute paths as required by Docker.
<Input id={`volume-target-${index}`} bind:value={mount.target} placeholder="/data" disabled={isCreating} + onchange={() => { + // Validate that target is an absolute path + if (mount.target && !mount.target.startsWith('/')) { + mount.targetError = "Container path must be absolute (start with /)"; + } else { + mount.targetError = undefined; + } + }} + class={mount.targetError ? "border-red-500" : ""} /> +{#if mount.targetError} + <div class="flex items-center mt-1 text-xs text-red-500"> + <AlertCircle class="h-3 w-3 mr-1" /> + {mount.targetError} + </div> +{/if}You'll also need to update the
volumeMountsstate type to include the error field:let volumeMounts = $state<{ source: string; target: string; + targetError?: string; }[]>([ { source: "", target: "" }, ]);📝 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 lang="ts"> // Expand volumeMounts entries to carry a per‐mount error field let volumeMounts = $state<{ source: string; target: string; targetError?: string; }[]>([ { source: "", target: "" }, ]); </script> <!-- … --> <div class="flex-1 grid grid-cols-2 gap-4"> <div> <Label for={`volume-source-${index}`} class="mb-2 block"> Source Volume </Label> <Select.Root type="single" bind:value={mount.source} disabled={isCreating} > <Select.Trigger class="w-full"> <span>{mount.source || "Select volume"}</span> </Select.Trigger> <Select.Content> {#each volumes as volume} <Select.Item value={volume.name}> {volume.name} </Select.Item> {/each} </Select.Content> </Select.Root> </div> <div> <Label for={`volume-target-${index}`} class="mb-2 block"> Container Path </Label> <Input id={`volume-target-${index}`} bind:value={mount.target} placeholder="/data" disabled={isCreating} onchange={() => { // Validate that target is an absolute path if (mount.target && !mount.target.startsWith('/')) { mount.targetError = "Container path must be absolute (start with /)"; } else { mount.targetError = undefined; } }} class={mount.targetError ? "border-red-500" : ""} /> {#if mount.targetError} <div class="flex items-center mt-1 text-xs text-red-500"> <AlertCircle class="h-3 w-3 mr-1" /> {mount.targetError} </div> {/if} </div> </div>
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/services/docker-service.ts(3 hunks)
🔇 Additional comments (4)
src/lib/services/docker-service.ts (4)
2-9: Improved type imports for Docker functionality.The type imports have been properly updated to support the new container creation functionality. Removing the unused
Volumetype and adding specific container-related types improves code clarity.
317-325: Enhanced documentation clarity.The improved JSDoc comments for
getContainerLogsnow properly document parameters, return values, and error handling behavior, which helps developers understand how to use this function correctly.
385-392: Comprehensive JSDoc for new container creation function.The function documentation clearly describes the purpose, parameters, return values, and potential error scenarios.
443-455: Container creation implementation looks robust.The container creation, startup, and response handling are well-implemented. The code correctly handles container inspection after creation to retrieve the full details, and returns a clean object with relevant container information.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kyle Mendell <kmendell@outlook.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kyle Mendell <kmendell@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/lib/services/docker-service.ts (4)
397-407: 🧹 Nitpick (assertive)Container configuration implementation is good but could be enhanced.
The basic container options are well-implemented, but you could consider adding support for additional Docker features like container labels, resource constraints (CPU/memory), user configuration, and command customization.
410-426: 🛠️ Refactor suggestionSimplify port binding logic to avoid redundant initialization.
The code has redundant initialization checks that can be simplified since the objects are already initialized at the beginning of the block.
if (config.ports && config.ports.length > 0) { containerOptions.ExposedPorts = {}; containerOptions.HostConfig = containerOptions.HostConfig || {}; containerOptions.HostConfig.PortBindings = {}; config.ports.forEach((port) => { const containerPort = `${port.containerPort}/tcp`; - if (!containerOptions.ExposedPorts) containerOptions.ExposedPorts = {}; containerOptions.ExposedPorts[containerPort] = {}; - if (!containerOptions.HostConfig) containerOptions.HostConfig = {}; - if (!containerOptions.HostConfig.PortBindings) - containerOptions.HostConfig.PortBindings = {}; containerOptions.HostConfig.PortBindings[containerPort] = [ { HostPort: port.hostPort }, ]; }); }
436-440: 🧹 Nitpick (assertive)Network configuration could be enhanced.
Currently, only basic network mode is supported. Consider enhancing to support multiple networks or specific IP configurations.
385-460: 🧹 Nitpick (assertive)Consider adding health check configuration.
Docker supports container health checks which are important for monitoring container health status. The current implementation doesn't allow configuring health checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/services/docker-service.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/services/docker-service.ts (1)
src/lib/types/docker.ts (2)
ContainerConfig(18-35)ContainerCreate(13-15)
🔇 Additional comments (5)
src/lib/services/docker-service.ts (5)
2-9: Import changes look good.Updated imports to include necessary Docker-related types for the new container creation functionality.
318-324: Documentation improvement is well done.The expanded JSDoc comment for
getContainerLogsprovides much better context about the function's purpose, parameters, and return values.
456-459: Error handling has been improved.Good job implementing the suggested improvement to include the image name in the error message, which will make debugging easier.
442-455: Container creation and inspection is well implemented.The approach of creating the container, starting it, and immediately returning the inspected details is a good pattern that provides comprehensive information back to the caller.
385-392: Function documentation is clear and comprehensive.The JSDoc comment for the
createContainerfunction clearly explains its purpose, parameters, return values, and potential errors.
| // Set up volume mounts if provided | ||
| if (config.volumes && config.volumes.length > 0) { | ||
| containerOptions.HostConfig = containerOptions.HostConfig || {}; | ||
| containerOptions.HostConfig.Binds = config.volumes.map( | ||
| (vol) => `${vol.source}:${vol.target}${vol.readOnly ? ':ro' : ''}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Volume binding implementation includes readOnly property that's not defined in the interface.
The code is using vol.readOnly to set read-only volumes, but this property isn't defined in the ContainerConfig interface snippet provided in the context.
Either update the interface to include this property or remove the readOnly condition:
containerOptions.HostConfig.Binds = config.volumes.map(
- (vol) => `${vol.source}:${vol.target}${vol.readOnly ? ':ro' : ''}`
+ (vol) => `${vol.source}:${vol.target}`
);🏁 Script executed:
#!/bin/bash
# Verify if the ContainerConfig interface includes a readOnly property for volumes
rg -A 15 "export interface ContainerConfig" "$lib/types/docker.ts"Length of output: 119
It looks like the interface wasn’t found at the given path. Let’s locate and inspect it across the repo:
🏁 Script executed:
#!/bin/bash
set -e
# 1. Find which file defines ContainerConfig
echo "Searching for ContainerConfig definition..."
file=$(rg -l "export interface ContainerConfig" . | head -n1)
if [ -z "$file" ]; then
echo "No ContainerConfig interface found."
exit 1
fi
echo "Found in: $file"
# 2. Show the interface definition and nearby context
echo -e "\nDefinition and surrounding lines:"
rg -A 20 "export interface ContainerConfig" "$file"Length of output: 885
Add readOnly to the volume definition in ContainerConfig
The vol.readOnly check in your bind-mount logic assumes a readOnly flag, but the volumes item type only declares source and target. You should extend the interface to include an optional readOnly property so that TypeScript and runtime behavior stay in sync.
• File: src/lib/types/docker.ts
Update the volumes item type:
export interface ContainerConfig {
name: string;
image: string;
ports?: Array<{
hostPort: string;
containerPort: string;
}>;
- volumes?: Array<{
- source: string;
- target: string;
- }>;
+ volumes?: Array<{
+ source: string;
+ target: string;
+ /** mount as read-only if true */
+ readOnly?: boolean;
+ }>;
envVars?: Array<{
key: string;
value: string;
}>;
network?: string;
restart?: "no" | "always" | "on-failure" | "unless-stopped";
}With that in place, the existing bind-mount mapping in docker-service.ts can remain unchanged:
containerOptions.HostConfig.Binds = config.volumes.map(
vol => `${vol.source}:${vol.target}${vol.readOnly ? ":ro" : ""}`
);
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kyle Mendell <kmendell@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/lib/services/docker-service.ts (3)
424-430: Support for read-only volume mounts is properly implemented.The volume mount configuration correctly implements the read-only flag as suggested in previous reviews.
452-457: Error handling includes improved context as suggested.The error message includes the image name, providing additional context to help with debugging as suggested in a previous review.
432-436: 🧹 Nitpick (assertive)Network configuration implementation is minimal but functional.
Consider enhancing network configuration to support multiple networks and IP assignments as suggested in a previous review.
src/lib/types/docker.ts (3)
25-29: Volume configuration includes readOnly property as recommended.The volume configuration correctly includes the readOnly property that was suggested in a previous review.
61-94: 🛠️ Refactor suggestionAdd Health and RestartPolicy to ContainerDetails.
The ContainerDetails interface should include Health status and RestartPolicy fields to better align with Docker Engine API's inspect response, as suggested in previous reviews.
export interface ContainerDetails { Id: string; Names: string[]; Image: string; ImageID: string; Command: string; Created: number; Ports: ContainerPort[]; Labels: { [label: string]: string }; State: string; Status: string; + Health?: { + Status: 'starting' | 'healthy' | 'unhealthy' | 'none'; + FailingStreak: number; + Log: Array<{ + Start: string; + End: string; + ExitCode: number; + Output: string; + }>; + }; HostConfig: { NetworkMode: string; + RestartPolicy?: { + Name: string; + MaximumRetryCount?: number; + }; }; NetworkSettings: { // Existing code... }; }
17-36: 🧹 Nitpick (assertive)Consider extending ContainerConfig with health check options.
To support Docker health checks, consider adding health check configuration to the ContainerConfig interface.
export interface ContainerConfig { name: string; image: string; ports?: Array<{ hostPort: string; containerPort: string; }>; volumes?: Array<{ source: string; target: string; readOnly?: boolean; }>; envVars?: Array<{ key: string; value: string; }>; network?: string; restart?: "no" | "always" | "on-failure" | "unless-stopped"; + healthcheck?: { + test: string[]; + interval?: number; + timeout?: number; + retries?: number; + startPeriod?: number; + }; + labels?: { [key: string]: string }; + cpuLimit?: number; + memoryLimit?: number; + user?: string; + command?: string[]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/services/docker-service.ts(3 hunks)src/lib/types/docker.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/services/docker-service.ts
[error] 417-417: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 418-418: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 418-418: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🔇 Additional comments (10)
src/lib/services/docker-service.ts (5)
2-9: Imports updated to support container creation functionality.The imports are properly updated to support the new container creation functionality, removing unused imports and adding necessary Docker types.
317-325: Well-documented function with improved JSDoc.The JSDoc comments for
getContainerLogshave been enhanced with more detailed parameter descriptions, return type information, and exception documentation.
385-392: Well-structured JSDoc for the new createContainer function.The JSDoc clearly explains the function's purpose, parameters, return values, and possible exceptions.
393-407: Container options initialization looks good.The basic container options are correctly set up with name, image, environment variables, and restart policy.
438-451: Container creation, startup, and property mapping looks good.The code properly creates the container, starts it, and maps the container properties to the return object. The substring operation to remove the leading slash from container names is a nice touch.
src/lib/types/docker.ts (5)
1-2: Appropriate import from dockerode.The import of
ContainerCreateOptionsfrom dockerode provides a good foundation for extending Docker container types.
12-15: ContainerCreate extends dockerode types appropriately.The interface properly extends the ContainerCreateOptions and adds a useful optional name property.
17-36: ContainerConfig interface covers core container parameters.The ContainerConfig interface provides a clear structure for container creation parameters with appropriate types for ports, volumes, environment variables, network, and restart policy.
38-44: ContainerPort interface is well-structured.The interface effectively models the Docker port mapping structure.
46-59: ContainerState interface is comprehensive.The ContainerState interface thoroughly covers all the state attributes of a Docker container.
| // Set up port bindings if provided | ||
| if (config.ports && config.ports.length > 0) { | ||
| containerOptions.ExposedPorts = {}; | ||
| containerOptions.HostConfig = containerOptions.HostConfig || {}; | ||
| containerOptions.HostConfig.PortBindings = {}; | ||
|
|
||
| config.ports.forEach((port) => { | ||
| const containerPort = `${port.containerPort}/tcp`; | ||
| containerOptions.ExposedPorts![containerPort] = {}; | ||
| containerOptions.HostConfig!.PortBindings![containerPort] = [ | ||
| { HostPort: port.hostPort }, | ||
| ]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-null assertions with optional chaining to improve type safety.
The code uses TypeScript non-null assertions (!) which could lead to runtime errors if the properties are unexpectedly undefined.
config.ports.forEach((port) => {
const containerPort = `${port.containerPort}/tcp`;
- containerOptions.ExposedPorts![containerPort] = {};
- containerOptions.HostConfig!.PortBindings![containerPort] = [
+ containerOptions.ExposedPorts && (containerOptions.ExposedPorts[containerPort] = {});
+ containerOptions.HostConfig?.PortBindings && (containerOptions.HostConfig.PortBindings[containerPort] = [
{ HostPort: port.hostPort },
- ];
+ ]);
});📝 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.
| // Set up port bindings if provided | |
| if (config.ports && config.ports.length > 0) { | |
| containerOptions.ExposedPorts = {}; | |
| containerOptions.HostConfig = containerOptions.HostConfig || {}; | |
| containerOptions.HostConfig.PortBindings = {}; | |
| config.ports.forEach((port) => { | |
| const containerPort = `${port.containerPort}/tcp`; | |
| containerOptions.ExposedPorts![containerPort] = {}; | |
| containerOptions.HostConfig!.PortBindings![containerPort] = [ | |
| { HostPort: port.hostPort }, | |
| ]; | |
| }); | |
| } | |
| // Set up port bindings if provided | |
| if (config.ports && config.ports.length > 0) { | |
| containerOptions.ExposedPorts = {}; | |
| containerOptions.HostConfig = containerOptions.HostConfig || {}; | |
| containerOptions.HostConfig.PortBindings = {}; | |
| config.ports.forEach((port) => { | |
| const containerPort = `${port.containerPort}/tcp`; | |
| containerOptions.ExposedPorts && (containerOptions.ExposedPorts[containerPort] = {}); | |
| containerOptions.HostConfig?.PortBindings && (containerOptions.HostConfig.PortBindings[containerPort] = [ | |
| { HostPort: port.hostPort }, | |
| ]); | |
| }); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 417-417: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 418-418: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 418-418: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/lib/types/docker.ts (1)
65-98: Still missing Health & RestartPolicy fields inContainerDetailsA previous review already highlighted that the Docker Engine
inspectpayload containsState.HealthandHostConfig.RestartPolicy. Surfacing these here keeps the type aligned with reality and lets consumers surface container health in the UI.Would you like help updating the interface and the mapping code in
getContainer()?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/lib/services/docker-service.ts(3 hunks)src/lib/types/docker.ts(2 hunks)src/routes/containers/create-container-dialog.svelte(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/services/docker-service.ts
[error] 417-417: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 418-418: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 418-418: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 456-456: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 477-477: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/lib/types/docker.ts (1)
25-30: Volumes definition now includesreadOnly– nice additionAdding the
readOnly?: booleanflag aligns the schema with the bind-mount implementation indocker-service.ts.
No further action required.src/lib/services/docker-service.ts (1)
467-473: 🧹 Nitpick (assertive)Return the typed
ContainerDetails(or a superset) for consistencyThe function currently returns a custom subset of fields. Returning an object that satisfies
ContainerDetailskeeps shapes consistent and avoids type duplication.- return { - id: containerInfo.Id, - name: containerInfo.Name.substring(1), - state: containerInfo.State.Status, - status: containerInfo.State.Running ? "running" : "stopped", - created: containerInfo.Created, - }; + const { Id, Name, State, Created, ...rest } = containerInfo; + return { + Id, + Names: [Name.substring(1)], + Image: containerInfo.Image, + ImageID: containerInfo.Image, + Command: containerInfo.Path + " " + containerInfo.Args.join(" "), + Created, + Ports: containerInfo.NetworkSettings.Ports ?? [], + Labels: containerInfo.Config.Labels ?? {}, + State: State.Status, + Status: State.Running ? "running" : "stopped", + HostConfig: containerInfo.HostConfig, + NetworkSettings: containerInfo.NetworkSettings, + Mounts: containerInfo.Mounts, + ...rest, + } as ContainerDetails;Likely an incorrect or invalid review comment.
src/routes/containers/create-container-dialog.svelte (1)
76-105: Great to see comprehensive port validation built into the componentThe helper guards against non-numeric and out-of-range ports—nice touch.
| if (config.network) { | ||
| containerOptions.HostConfig = containerOptions.HostConfig || {}; | ||
| containerOptions.HostConfig.NetworkMode = config.network; | ||
|
|
||
| // If using user-defined networks and specific IP assignments | ||
| if ( | ||
| config.networkConfig && | ||
| config.network !== "host" && | ||
| config.network !== "none" && | ||
| config.network !== "bridge" | ||
| ) { | ||
| containerOptions.NetworkingConfig = { | ||
| EndpointsConfig: { | ||
| [config.network]: { | ||
| IPAMConfig: { | ||
| IPv4Address: config.networkConfig.ipv4Address || undefined, // Use undefined if empty | ||
| IPv6Address: config.networkConfig.ipv6Address || undefined, // Use undefined if empty | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
| // When specifying NetworkingConfig, NetworkMode in HostConfig should not be set for user-defined networks | ||
| // Docker handles attaching to the specified network via EndpointsConfig | ||
| delete containerOptions.HostConfig.NetworkMode; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer conditional assignment over delete for performance & clarity
delete containerOptions.HostConfig.NetworkMode works but is slower and flagged by static-analysis.
Skip setting the property in the first place:
- containerOptions.HostConfig.NetworkMode = config.network;
...
- delete containerOptions.HostConfig.NetworkMode;
+ // only set NetworkMode when we are NOT supplying EndpointsConfig
+ if (
+ !config.networkConfig ||
+ config.network === "host" ||
+ config.network === "none" ||
+ config.network === "bridge"
+ ) {
+ containerOptions.HostConfig.NetworkMode = config.network;
+ }🧰 Tools
🪛 Biome (1.9.4)
[error] 456-456: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| const isUserDefinedNetwork = $derived( | ||
| network && | ||
| network !== "" && | ||
| network !== "host" && | ||
| network !== "none" && | ||
| network !== "bridge" | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
isUserDefinedNetwork may hold a string instead of boolean
Because the expression returns network itself, the derived value could be 'mynet' rather than true/false.
Wrap the condition in Boolean() to guarantee a boolean result.
-const isUserDefinedNetwork = $derived(
- network &&
+const isUserDefinedNetwork = $derived(
+ Boolean(
+ network &&
network !== "" &&
network !== "host" &&
network !== "none" &&
- network !== "bridge"
-);
+ network !== "bridge"
+ )
+);📝 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 isUserDefinedNetwork = $derived( | |
| network && | |
| network !== "" && | |
| network !== "host" && | |
| network !== "none" && | |
| network !== "bridge" | |
| ); | |
| const isUserDefinedNetwork = $derived( | |
| Boolean( | |
| network && | |
| network !== "" && | |
| network !== "host" && | |
| network !== "none" && | |
| network !== "bridge" | |
| ) | |
| ); |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kyle Mendell <kmendell@outlook.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kyle Mendell <kmendell@outlook.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kyle Mendell <kmendell@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/lib/services/docker-service.ts (1)
465-468: 🛠️ Refactor suggestionAvoid
deleteoperator – skip settingNetworkModeinstead
delete containerOptions.HostConfig.NetworkModetriggers Biome’snoDeleterule and incurs a runtime de-opt.
BecauseNetworkModeis only required when we don’t supplyNetworkingConfig, we can drop the property earlier instead of creating & deleting it.- containerOptions.HostConfig.NetworkMode = config.network; - - // If using user-defined networks and specific IP assignments - if ( + // If using user-defined networks and specific IP assignments + if ( config.networkConfig && config.network !== "host" && config.network !== "none" && config.network !== "bridge" ) { ... - delete containerOptions.HostConfig.NetworkMode; + // do *not* set HostConfig.NetworkMode when NetworkingConfig is present }🧰 Tools
🪛 Biome (1.9.4)
[error] 467-467: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/routes/containers/create-container-dialog.svelte (2)
166-172:isUserDefinedNetworkmay evaluate to a string, not boolean
(reiterating previous feedback)Wrap the expression with
Boolean(...)to guarantee a boolean result:-const isUserDefinedNetwork = $derived( - network && +const isUserDefinedNetwork = $derived( + Boolean( + network && network !== "" && network !== "host" && network !== "none" && network !== "bridge" - ); + ) +);
20-24: DialogonClosedoesn’t notify parent components
(reiterating previous feedback)Emit an event so parents listening to
<CreateContainerDialog on:close>can react:import { createEventDispatcher } from "svelte"; const dispatch = createEventDispatcher(); export function onClose() { open = false; dispatch("close"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/services/docker-service.ts(3 hunks)src/routes/containers/create-container-dialog.svelte(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/services/docker-service.ts (1)
src/lib/types/docker.ts (2)
ContainerConfig(18-40)ContainerCreate(13-15)
🪛 Biome (1.9.4)
src/lib/services/docker-service.ts
[error] 467-467: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 488-488: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/lib/services/docker-service.ts
Outdated
| config.ports.forEach(({ hostPort, containerPort }) => { | ||
| const hp = Number(hostPort); | ||
| const cp = Number(containerPort); | ||
| if ( | ||
| !Number.isInteger(hp) || | ||
| !Number.isInteger(cp) || | ||
| hp < 1 || hp > 65535 || | ||
| cp < 1 || cp > 65535 | ||
| ) { | ||
| throw new Error( | ||
| `Invalid port mapping "${hostPort}:${containerPort}". Ports must be numeric (1-65535).` | ||
| ); | ||
| } | ||
|
|
||
| const key = `${cp}/tcp`; | ||
| containerOptions.ExposedPorts[key] = {}; | ||
| containerOptions.HostConfig.PortBindings[key] = [{ HostPort: `${hp}` }]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against duplicate port keys & protocol variations
When multiple mappings share the same containerPort/protocol pair, later iterations silently overwrite earlier ones:
containerOptions.ExposedPorts[key] = {};
containerOptions.HostConfig.PortBindings[key] = [{ HostPort: `${hp}` }];Consider:
- Throwing if
keyalready exists, or - Aggregating multiple host ports into one array.
Also, TCP is hard-coded; add port.protocol ?? "tcp" for future UDP support.
| } catch (error: any) { | ||
| console.error("Error creating container:", error); | ||
| // Provide more specific error feedback if possible | ||
| if (error.message && error.message.includes("IPAMConfig")) { | ||
| throw new Error( | ||
| `Failed to create container: Invalid IP address configuration for network "${config.network}". ${error.message}` | ||
| ); | ||
| } | ||
| throw new Error( | ||
| `Failed to create container with image "${config.image}": ${error.message}` | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Leverage optional-chaining for concise error checks
error.message && error.message.includes("IPAMConfig") can be condensed and made safer with optional-chaining:
- if (error.message && error.message.includes("IPAMConfig")) {
+ if (error.message?.includes("IPAMConfig")) {This removes a redundant logical check and aligns with modern TypeScript style.
📝 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.
| } catch (error: any) { | |
| console.error("Error creating container:", error); | |
| // Provide more specific error feedback if possible | |
| if (error.message && error.message.includes("IPAMConfig")) { | |
| throw new Error( | |
| `Failed to create container: Invalid IP address configuration for network "${config.network}". ${error.message}` | |
| ); | |
| } | |
| throw new Error( | |
| `Failed to create container with image "${config.image}": ${error.message}` | |
| ); | |
| } catch (error: any) { | |
| console.error("Error creating container:", error); | |
| // Provide more specific error feedback if possible | |
| if (error.message?.includes("IPAMConfig")) { | |
| throw new Error( | |
| `Failed to create container: Invalid IP address configuration for network "${config.network}". ${error.message}` | |
| ); | |
| } | |
| throw new Error( | |
| `Failed to create container with image "${config.image}": ${error.message}` | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 488-488: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| <script lang="ts"> | ||
| import { preventDefault } from "svelte/legacy"; | ||
| import type { ContainerConfig } from "$lib/types/docker"; |
There was a problem hiding this comment.
preventDefault import path is invalid
"svelte/legacy" is not part of the public Svelte API and will fail at build time.
Use the modifier or a local helper instead:
<form on:submit|preventDefault={handleSubmit}>or
function handleSubmit(e: SubmitEvent) {
e.preventDefault();
...
}| onclick={() => { | ||
| env.sensitive = !env.sensitive; | ||
| }} | ||
| disabled={isCreating} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mutating objects inside arrays is not reactive
env.sensitive = !env.sensitive won’t trigger Svelte reactivity.
Re-assign the array:
envVars[index].sensitive = !envVars[index].sensitive;
envVars = [...envVars];Otherwise the icon and input type may not update.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/routes/containers/create-container-dialog.svelte (4)
1-2:⚠️ Potential issueFix the invalid import path from "svelte/legacy"
The import from "svelte/legacy" is not part of the public Svelte API and will fail at build time.
Replace with the event modifier:
-<script lang="ts"> - import { preventDefault } from "svelte/legacy"; +<script lang="ts">And then update your form:
-<form onsubmit={preventDefault(handleSubmit)} class="space-y-6"> +<form on:submit|preventDefault={handleSubmit} class="space-y-6">Or handle it directly in the function:
-function handleSubmit() { +function handleSubmit(e: SubmitEvent) { + e.preventDefault(); if (!selectedImage || !containerName.trim()) return;
79-107:⚠️ Potential issueFix return type mismatch in validatePortNumber function
The function returns a
warningproperty for privileged ports, but this property isn't declared in the return type.Update the type definition to include the missing property:
function validatePortNumber(port: string | number): { isValid: boolean; error?: string; + warning?: string; } {
169-175:⚠️ Potential issueConvert network check to return a boolean instead of potentially a string
The derived value
isUserDefinedNetworkmay hold a string instead of a boolean because the expression returnsnetworkitself rather than a boolean result.Wrap the condition in
Boolean()to guarantee a boolean result:const isUserDefinedNetwork = $derived( - network && - network !== "" && - network !== "host" && - network !== "none" && - network !== "bridge" + Boolean( + network && + network !== "" && + network !== "host" && + network !== "none" && + network !== "bridge" + ) );
491-494:⚠️ Potential issueFix reactivity issue with environment variable sensitivity toggle
Mutating object properties directly within arrays won't trigger Svelte's reactivity.
Update the assignment to ensure reactivity:
onclick={() => { - env.sensitive = !env.sensitive; + envVars[index].sensitive = !envVars[index].sensitive; + envVars = [...envVars]; }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/routes/containers/create-container-dialog.svelte(1 hunks)
🔇 Additional comments (5)
src/routes/containers/create-container-dialog.svelte (5)
67-69: Good practice implementing sensitive value protectionThe environment variables implementation properly includes a sensitive flag defaulting to true, which helps protect passwords and secrets from casual exposure.
323-340: Port input implementation has proper validation and error displayThe host port input correctly implements:
- Numeric input with patterns and inputmode
- Visual error indication with border color
- Clear error messages with icons
- Validation logic for numeric range and privileged ports
This provides good user feedback and prevents invalid submissions.
479-504: Environment variable value visibility toggle is well implementedThe implementation includes:
- Password masking for sensitive values
- Toggle button with appropriate icons
- Clear title attributes for accessibility
- Proper type switching between password and text
This is a good security practice for handling potentially sensitive information.
24-27: Good implementation of the onClose function with event dispatchThe function correctly sets the
openstate to false and dispatches a "close" event to notify parent components, allowing them to react when the dialog is closed.
599-607: Well-implemented create button with loading stateThe create button properly:
- Disables when the form is being submitted
- Disables when required fields are missing
- Shows a loading spinner during the creation process
- Changes text to indicate the ongoing operation
This provides good user feedback and prevents multiple submissions.
| function handleSubmit() { | ||
| if (!selectedImage || !containerName.trim()) return; | ||
|
|
||
| // Validate all ports | ||
| let hasInvalidPort = false; | ||
| ports.forEach((port) => { | ||
| // Check for invalid ports with content | ||
| if ( | ||
| (port.hostPort && !validatePortNumber(port.hostPort).isValid) || | ||
| (port.containerPort && !validatePortNumber(port.containerPort).isValid) | ||
| ) { | ||
| hasInvalidPort = true; | ||
| } | ||
| }); | ||
|
|
||
| if (hasInvalidPort) { | ||
| return; // Stop submission if there are invalid ports | ||
| } | ||
|
|
||
| // Filter out empty entries | ||
| const filteredPorts = ports | ||
| .filter((p) => p.hostPort.trim() && p.containerPort.trim()) | ||
| .map(({ hostPort, containerPort }) => ({ hostPort, containerPort })); // Remove error properties | ||
|
|
||
| const filteredVolumes = volumeMounts.filter( | ||
| (v) => v.source.trim() && v.target.trim() | ||
| ); | ||
| const filteredEnvVars = envVars.filter((e) => e.key.trim()); | ||
|
|
||
| const containerConfig: ContainerConfig = { | ||
| name: containerName.trim(), | ||
| image: selectedImage, | ||
| ports: filteredPorts.length > 0 ? filteredPorts : undefined, | ||
| volumes: filteredVolumes.length > 0 ? filteredVolumes : undefined, | ||
| envVars: filteredEnvVars.length > 0 ? filteredEnvVars : undefined, | ||
| network: network || undefined, | ||
| restart: restartPolicy as | ||
| | "no" | ||
| | "always" | ||
| | "on-failure" | ||
| | "unless-stopped", | ||
| // Add networkConfig if a user-defined network is selected and IPs are provided | ||
| networkConfig: | ||
| isUserDefinedNetwork && (ipv4Address.trim() || ipv6Address.trim()) | ||
| ? { | ||
| ipv4Address: ipv4Address.trim() || undefined, | ||
| ipv6Address: ipv6Address.trim() || undefined, | ||
| } | ||
| : undefined, | ||
| }; | ||
|
|
||
| // Pass the data to the parent component which handles the form submission | ||
| onSubmit(containerConfig); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add additional validation for container name
The current form only checks if the container name is empty, but Docker container names have specific restrictions. They must match [a-zA-Z0-9][a-zA-Z0-9_.-]+.
Add validation for the container name:
// Add this validation function
function validateContainerName(name: string): { isValid: boolean; error?: string } {
if (!name || !name.trim()) {
return { isValid: false, error: "Container name is required" };
}
// Container name regex pattern (must conform to Docker naming rules)
const namePattern = /^[a-zA-Z0-9][a-zA-Z0-9_.-]+$/;
if (!namePattern.test(name)) {
return {
isValid: false,
error: "Invalid name. Use letters, numbers, underscore, period, or hyphen. Must start with letter or number."
};
}
return { isValid: true };
}
// Add state for container name validation
let containerNameError = $state<string | undefined>(undefined);
// Add effect for container name validation
$effect(() => {
if (containerName) {
const validation = validateContainerName(containerName);
containerNameError = validation.error;
} else {
containerNameError = undefined;
}
});
// Update handleSubmit to check container name validity
function handleSubmit() {
const nameValidation = validateContainerName(containerName);
if (!nameValidation.isValid) {
return; // Stop submission if container name is invalid
}
// Rest of the submission logic...
}Then update the container name input field to show validation errors:
<Input
id="container-name"
bind:value={containerName}
placeholder="e.g., my-container"
disabled={isCreating}
+ class={containerNameError ? "border-red-500" : ""}
/>
+{#if containerNameError}
+ <div class="flex items-center mt-1 text-xs text-red-500">
+ <AlertCircle class="h-3 w-3 mr-1" />
+ {containerNameError}
+ </div>
+{/if}Committable suggestion skipped: line range outside the PR's diff.
| {#if isUserDefinedNetwork} | ||
| <div class="border-t pt-4 mt-4 space-y-4"> | ||
| <p class="text-sm text-muted-foreground"> | ||
| Optional: Assign static IP addresses (requires network with | ||
| IPAM configured). | ||
| </p> | ||
| <div class="grid grid-cols-1 md:grid-cols-2 gap-4"> | ||
| <div class="grid grid-cols-1 gap-2"> | ||
| <Label for="ipv4-address">IPv4 Address</Label> | ||
| <Input | ||
| id="ipv4-address" | ||
| bind:value={ipv4Address} | ||
| placeholder="e.g., 172.20.0.10" | ||
| disabled={isCreating} | ||
| /> | ||
| </div> | ||
| <div class="grid grid-cols-1 gap-2"> | ||
| <Label for="ipv6-address">IPv6 Address</Label> | ||
| <Input | ||
| id="ipv6-address" | ||
| bind:value={ipv6Address} | ||
| placeholder="e.g., 2001:db8::10" | ||
| disabled={isCreating} | ||
| /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for IP address inputs
The IPv4 and IPv6 address inputs don't have any validation to ensure they're properly formatted. This could lead to invalid configurations being submitted.
Add validation functions for IP addresses:
// Add these validation functions after the port validation functions
function validateIPv4Address(address: string): { isValid: boolean; error?: string } {
if (!address || !address.trim()) return { isValid: true };
// IPv4 regex pattern
const ipv4Pattern = /^((25[0-5]|(2[0-4]|1\d|[1-9]|)\d)\.?\b){4}$/;
if (!ipv4Pattern.test(address)) {
return { isValid: false, error: "Invalid IPv4 address format" };
}
return { isValid: true };
}
function validateIPv6Address(address: string): { isValid: boolean; error?: string } {
if (!address || !address.trim()) return { isValid: true };
// Basic IPv6 validation - this could be improved with a more comprehensive regex
const ipv6Pattern = /^([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}$|^::$|^::1$|^([0-9a-fA-F]{1,4}::?){1,7}([0-9a-fA-F]{1,4})$|^([0-9a-fA-F]{1,4}:){1,7}:$|^:(:[0-9a-fA-F]{1,4}){1,7}$/;
if (!ipv6Pattern.test(address)) {
return { isValid: false, error: "Invalid IPv6 address format" };
}
return { isValid: true };
}
// Add state for IP validation errors
let ipv4Error = $state<string | undefined>(undefined);
let ipv6Error = $state<string | undefined>(undefined);
// Add effect for IP validation
$effect(() => {
if (ipv4Address) {
const validation = validateIPv4Address(ipv4Address);
ipv4Error = validation.error;
} else {
ipv4Error = undefined;
}
if (ipv6Address) {
const validation = validateIPv6Address(ipv6Address);
ipv6Error = validation.error;
} else {
ipv6Error = undefined;
}
});
// Update handleSubmit to check IP validity
function handleSubmit() {
// Existing validation code...
// Validate IP addresses if provided
if (isUserDefinedNetwork) {
if (ipv4Address && !validateIPv4Address(ipv4Address).isValid) {
return; // Stop submission if IPv4 is invalid
}
if (ipv6Address && !validateIPv6Address(ipv6Address).isValid) {
return; // Stop submission if IPv6 is invalid
}
}
// Rest of the submission logic...
}Then update the IP input fields to show validation errors:
<Input
id="ipv4-address"
bind:value={ipv4Address}
placeholder="e.g., 172.20.0.10"
disabled={isCreating}
+ class={ipv4Error ? "border-red-500" : ""}
/>
+{#if ipv4Error}
+ <div class="flex items-center mt-1 text-xs text-red-500">
+ <AlertCircle class="h-3 w-3 mr-1" />
+ {ipv4Error}
+ </div>
+{/if}Do the same for the IPv6 input.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
src/lib/types/docker.ts (1)
71-104: Health & RestartPolicy still missing fromContainerDetailsPrevious feedback noted that Docker’s inspect response includes
State.HealthandHostConfig.RestartPolicy, yet these properties are still absent.Keeping the type incomplete can lead to unsafe
anyescapes downstream. Please re-add the optional fields or tag this comment as resolved if the omission is intentional.src/lib/services/docker-service.ts (2)
468-469: 🧹 Nitpick (assertive)Avoid
deletefor performance & Biome compliance
delete containerOptions.HostConfig.NetworkMode;triggers thenoDeleterule and incurs hidden costs. Prefer conditional assignment:- delete containerOptions.HostConfig.NetworkMode; + // If NetworkingConfig is supplied, omit NetworkMode to satisfy API + containerOptions.HostConfig.NetworkMode = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 468-468: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
423-435: 🛠️ Refactor suggestionUnsafe non-null assertions & lack of port validation
ExposedPorts!andPortBindings!bypass type safety and may crash at runtime.
Additionally, user input is written directly into the binding map without confirming it is an integer ∈ 1-65535.- containerOptions.ExposedPorts = {}; - containerOptions.HostConfig = containerOptions.HostConfig || {}; - containerOptions.HostConfig.PortBindings = {}; - - config.ports.forEach((port) => { - const containerPort = `${port.containerPort}/tcp`; - containerOptions.ExposedPorts![containerPort] = {}; - containerOptions.HostConfig!.PortBindings![containerPort] = [ - { HostPort: port.hostPort }, - ]; - }); + containerOptions.ExposedPorts = {}; + containerOptions.HostConfig = containerOptions.HostConfig || {}; + containerOptions.HostConfig.PortBindings = {}; + + config.ports.forEach(({ hostPort, containerPort }) => { + const hp = Number(hostPort); + const cp = Number(containerPort); + if ( + !Number.isInteger(hp) || !Number.isInteger(cp) || + hp < 1 || hp > 65535 || cp < 1 || cp > 65535 + ) { + throw new Error( + `Invalid port mapping "${hostPort}:${containerPort}". ` + + `Ports must be integers between 1-65535.` + ); + } + + const key = `${cp}/tcp`; + containerOptions.ExposedPorts[key] = {}; + containerOptions.HostConfig.PortBindings[key] = [{ HostPort: `${hp}` }]; + });This removes
!, performs defensive validation, and guards against malformed input.🧰 Tools
🪛 Biome (1.9.4)
[error] 430-430: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 431-431: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 431-431: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/routes/containers/create-container-dialog.svelte (5)
198-204:isUserDefinedNetworkmay return a string rather than a booleanThe derived store currently resolves to the network name (truthy string) instead of a strict boolean, which can create subtle bugs in reactive conditions.
-const isUserDefinedNetwork = $derived( - network && +const isUserDefinedNetwork = $derived( + Boolean( + network && network !== "" && network !== "host" && network !== "none" && network !== "bridge" - ); + ) +);
603-606: Mutating array element won’t trigger reactivityToggling
env.sensitivemutates an object inside the array; Svelte won’t detect this change unless the array itself is re-assigned.- env.sensitive = !env.sensitive; + envVars[index].sensitive = !envVars[index].sensitive; + envVars = [...envVars];
123-127: Privileged-port “warning” still allows submissionReturning
isValid: truewith an error string renders red text yet doesn’t block form submission, confusing users. Consider introducing a separatewarningproperty or marking ports <1024 as invalid unless explicitly allowed.
1-3:⚠️ Potential issue
svelte/legacyimport is unsupported – use the modifier instead
preventDefaultis not exported from"svelte/legacy"and will break the build.- import { preventDefault } from "svelte/legacy"; ... - <form onsubmit={preventDefault(handleSubmit)} ...> + <!-- no extra import needed --> + <form on:submit|preventDefault={handleSubmit} ...>
440-471: 🧹 Nitpick (assertive)Use native numeric inputs & min/max constraints for ports
type="text"+ regex is fragile and allows invalid values (e.g.00999,65536).
Switch totype="number" min="1" max="65535"to leverage browser validation:- <Input - ... - type="text" - pattern="[0-9]*" - inputmode="numeric" + <Input + ... + type="number" + min="1" + max="65535"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/lib/services/docker-service.ts(3 hunks)src/lib/types/docker.ts(2 hunks)src/lib/utils/bytes.ts(1 hunks)src/routes/containers/create-container-dialog.svelte(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/utils/bytes.ts
[error] 12-12: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.
(lint/style/useNumberNamespace)
[error] 20-20: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.
(lint/style/useNumberNamespace)
[error] 22-22: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.
(lint/style/useNumberNamespace)
src/lib/services/docker-service.ts
[error] 430-430: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 431-431: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 431-431: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 468-468: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 488-488: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 494-494: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 497-497: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| export function parseBytes(input: string): number { | ||
| const valueStr = input.toLowerCase().trim(); | ||
| const unit = valueStr.charAt(valueStr.length - 1); | ||
| const value = parseFloat(valueStr.substring(0, valueStr.length - 1)); | ||
|
|
||
| if (isNaN(value)) { | ||
| throw new Error(`Invalid numeric value in memory string: ${input}`); | ||
| } | ||
|
|
||
| if (units[unit]) { | ||
| return Math.floor(value * units[unit]); | ||
| } else if (!isNaN(parseFloat(valueStr))) { | ||
| // Assume bytes if no unit | ||
| return Math.floor(parseFloat(valueStr)); | ||
| } else { | ||
| throw new Error(`Invalid memory unit: ${unit}. Use b, k, m, or g.`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Value-only inputs lose their last digit & violate number-namespace rule
- For inputs without an explicit unit (e.g.
"1024"), the current logic slices off the last character (substring(0, len-1)), so"1024"is parsed as102– a silent and dangerous mis-calculation. parseFloatis invoked via the global instead ofNumber.parseFloat, triggering Biome’suseNumberNamespacerule.
export function parseBytes(input: string): number {
- const valueStr = input.toLowerCase().trim();
- const unit = valueStr.charAt(valueStr.length - 1);
- const value = parseFloat(valueStr.substring(0, valueStr.length - 1));
+ const valueStr = input.toLowerCase().trim();
+
+ // Detect if the last char is an alpha unit (b/k/m/g); otherwise treat whole string as a number
+ const maybeUnit = valueStr.slice(-1);
+ const hasUnit = /^[bkmg]$/.test(maybeUnit);
+
+ const numericPortion = hasUnit ? valueStr.slice(0, -1) : valueStr;
+ const value = Number.parseFloat(numericPortion);
...
- if (units[unit]) {
- return Math.floor(value * units[unit]);
- } else if (!isNaN(parseFloat(valueStr))) {
- // Assume bytes if no unit
- return Math.floor(parseFloat(valueStr));
+ if (hasUnit) {
+ return Math.floor(value * units[maybeUnit]);
+ } else if (!Number.isNaN(value)) {
+ // No unit supplied → bytes
+ return Math.floor(value);
} else {
throw new Error(`Invalid memory unit: ${unit}. Use b, k, m, or g.`);
}
}This eliminates the off-by-one bug, complies with ES-2015 best-practice, and keeps behaviour unchanged for unit-suffixed inputs.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.
(lint/style/useNumberNamespace)
[error] 20-20: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.
(lint/style/useNumberNamespace)
[error] 22-22: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.
(lint/style/useNumberNamespace)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kyle Mendell <kmendell@outlook.com>
github-project-automation
bot
moved this from In Development
to Done
in Arcane Project Board
1 participant
Fixes: #5
Summary by CodeRabbit
New Features
Bug Fixes
Chores