refactor: reorganize services and types#71
Conversation
WalkthroughThis change refactors the Docker service layer by splitting the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Layer
participant Core as docker/core.ts
participant Container as container-service.ts
participant Image as image-service.ts
participant Network as network-service.ts
participant Volume as volume-service.ts
participant Stack as stack-service.ts
App->>Core: getDockerClient(), initializeDocker(), etc.
App->>Container: listContainers(), createContainer(), etc.
App->>Image: listImages(), pullImage(), etc.
App->>Network: listNetworks(), createNetwork(), etc.
App->>Volume: listVolumes(), createVolume(), etc.
App->>Stack: loadComposeStacks(), createStack(), etc.
Container->>Core: getDockerClient()
Image->>Core: getDockerClient()
Network->>Core: getDockerClient()
Volume->>Core: getDockerClient()
Stack->>Core: getDockerClient()
Assessment against linked issues
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/lib/services/docker/core.tsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-node' imported from /svelte.config.js src/lib/services/docker/volume-service.tsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-node' imported from /svelte.config.js src/lib/services/docker/stack-service.tsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-node' imported from /svelte.config.js
✨ 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: 21
🔭 Outside diff range comments (1)
src/routes/api/docker/test-connection/+server.ts (1)
18-18: 💡 Verification agent🧩 Analysis chain
Critical: Pass
hostToTestintogetDockerInfo
You retrievehostToTestfrom the query but invokegetDockerInfo()without it. This will ignore the specified host. Update the call to:- await getDockerInfo(); + await getDockerInfo(hostToTest);Confirm that
getDockerInfoaccepts a host parameter and adjust its signature if necessary.
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Searching for all occurrences of getDockerInfo in the repo" rg -n "getDockerInfo" . echo echo "📄 Displaying definitions or exports of getDockerInfo for context" rg -n "export function getDockerInfo" -A3 -B1 . rg -n "export const getDockerInfo" -A3 -B1 . rg -n "function getDockerInfo" -A3 -B1 .Length of output: 1288
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Displaying the signature of getDockerInfo in core.ts" rg -n "export async function getDockerInfo" -A5 -B2 src/lib/services/docker/core.tsLength of output: 424
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Context around hostToTest in test-connection route:" rg -n "hostToTest" -A3 -B3 src/routes/api/docker/test-connection/+server.ts || true echo echo "🔍 Context around getDockerInfo call in test-connection route:" rg -n "getDockerInfo" -A3 -B3 src/routes/api/docker/test-connection/+server.tsLength of output: 1688
Critical: Pass
hostToTestintogetDockerInfoand update its signatureThe
GEThandler insrc/routes/api/docker/test-connection/+server.tsextractshostToTestbut never passes it along—so you’re still connecting to the default Docker target. You need to:• In
src/routes/api/docker/test-connection/+server.ts, change at line 18:- await getDockerInfo(); + await getDockerInfo(hostToTest);• In
src/lib/services/docker/core.ts, update thegetDockerInfosignature to accept an optional host and forward it to the client:-export async function getDockerInfo() { +export async function getDockerInfo(host?: string) { const docker = getDockerClient(host); return await docker.info(); }• If
getDockerClientdoesn’t yet accept ahostparameter, adjust its signature and implementation to use the passed-in host (e.g. TCP socket or environment override).This ensures the route actually tests the specified Docker endpoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (47)
src/hooks.server.ts(1 hunks)src/lib/services/compose.ts(0 hunks)src/lib/services/docker-service.ts(0 hunks)src/lib/services/docker/container-service.ts(1 hunks)src/lib/services/docker/core.ts(1 hunks)src/lib/services/docker/image-service.ts(1 hunks)src/lib/services/docker/network-service.ts(1 hunks)src/lib/services/docker/stack-service.ts(1 hunks)src/lib/services/docker/volume-service.ts(1 hunks)src/lib/types/docker/connection.type.ts(1 hunks)src/lib/types/docker/container.type.ts(2 hunks)src/lib/types/docker/image.type.ts(1 hunks)src/lib/types/docker/index.ts(1 hunks)src/lib/types/docker/network.type.ts(1 hunks)src/lib/types/docker/volume.type.ts(1 hunks)src/lib/types/image.ts(0 hunks)src/routes/+page.server.ts(1 hunks)src/routes/+page.svelte(0 hunks)src/routes/api/containers/+server.ts(1 hunks)src/routes/api/docker/containers/+server.ts(1 hunks)src/routes/api/docker/test-connection/+server.ts(1 hunks)src/routes/api/images/[id]/+server.ts(1 hunks)src/routes/api/images/prune/+server.ts(1 hunks)src/routes/api/images/pull-stream/[...name]/+server.ts(1 hunks)src/routes/api/images/pull/[...name]/+server.ts(1 hunks)src/routes/api/networks/[id]/+server.ts(1 hunks)src/routes/api/networks/create/+server.ts(1 hunks)src/routes/api/stacks/create/+server.ts(1 hunks)src/routes/api/stacks/import/+server.ts(1 hunks)src/routes/api/volumes/+server.ts(1 hunks)src/routes/api/volumes/[name]/+server.ts(1 hunks)src/routes/columns.ts(1 hunks)src/routes/containers/+page.server.ts(1 hunks)src/routes/containers/+page.svelte(2 hunks)src/routes/containers/[id]/+page.server.ts(1 hunks)src/routes/containers/columns.ts(1 hunks)src/routes/containers/create-container-dialog.svelte(1 hunks)src/routes/images/+page.server.ts(1 hunks)src/routes/images/+page.svelte(1 hunks)src/routes/images/columns.ts(1 hunks)src/routes/networks/+page.server.ts(1 hunks)src/routes/networks/columns.ts(1 hunks)src/routes/stacks/+page.server.ts(1 hunks)src/routes/stacks/[stackId]/+page.server.ts(1 hunks)src/routes/stacks/columns.ts(1 hunks)src/routes/volumes/+page.server.ts(1 hunks)src/routes/volumes/columns.ts(1 hunks)
💤 Files with no reviewable changes (4)
- src/routes/+page.svelte
- src/lib/types/image.ts
- src/lib/services/compose.ts
- src/lib/services/docker-service.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/lib/services/docker/core.ts (1)
src/lib/types/docker/connection.type.ts (1)
DockerConnectionOptions(1-8)
src/lib/services/docker/volume-service.ts (2)
src/lib/types/docker/volume.type.ts (1)
ServiceVolume(1-7)src/lib/services/docker/core.ts (2)
getDockerClient(24-49)dockerHost(5-5)
src/lib/services/docker/image-service.ts (2)
src/lib/types/docker/image.type.ts (1)
ServiceImage(1-11)src/lib/services/docker/core.ts (2)
getDockerClient(24-49)dockerHost(5-5)
src/lib/services/docker/stack-service.ts (3)
src/lib/services/settings-service.ts (2)
getSettings(128-181)ensureStacksDirectory(65-83)src/lib/services/docker/core.ts (1)
getDockerClient(24-49)src/lib/types/docker/stack.type.ts (4)
StackService(7-16)Stack(18-30)StackMeta(1-5)StackUpdate(32-35)
🪛 Biome (1.9.4)
src/lib/services/docker/core.ts
[error] 5-5: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 26-26: This let declares a variable that is only assigned once.
'connectionOpts' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 34-34: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 38-38: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 61-61: This let declares a variable that is only assigned once.
'connectionOpts' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 100-100: This let declares a variable that is only assigned once.
'connectionOpts' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 110-110: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 115-115: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
src/lib/services/docker/volume-service.ts
[error] 125-125: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/lib/services/docker/image-service.ts
[error] 61-61: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/lib/services/docker/stack-service.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 351-351: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/lib/services/docker/container-service.ts
[error] 264-264: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 265-265: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 265-265: 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] 293-293: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 313-313: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 317-317: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 320-320: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (39)
src/lib/types/docker/network.type.ts (1)
1-9: Well-defined ServiceNetwork type
The newServiceNetworkalias cleanly models Docker network entities with required and nullable fields, improving type safety and consistency.src/lib/types/docker/volume.type.ts (1)
1-7: Clear ServiceVolume definition
TheServiceVolumetype accurately represents Docker volumes, and the explicitlabels: { [label: string]: string } | nullaligns with expected metadata usage.src/routes/images/+page.svelte (1)
3-3: Use centralized Docker types barrel
Switching to importEnhancedImageInfofrom$lib/types/dockersimplifies and standardizes type imports across the application.src/routes/containers/create-container-dialog.svelte (1)
3-3: Updated ContainerConfig import path
ImportingContainerConfigfrom the specificcontainer.typemodule aligns with the new, more granular Docker type organization.src/routes/stacks/columns.ts (1)
1-1: Adjusted Stack type import
Importing theStacktype from$lib/types/docker/stack.typereflects the refactored types directory structure for Docker stacks.src/routes/api/images/pull/[...name]/+server.ts (1)
3-3: Import path updated to new image service module
ImportingpullImagefrom$lib/services/docker/image-servicecorrectly reflects the refactored service structure and replaces the deprecated monolithic service.src/routes/api/images/pull-stream/[...name]/+server.ts (1)
2-2: Import path updated to Docker core module
Switching togetDockerClientfrom$lib/services/docker/corealigns with the new modular client management and replaces the old monolithic service.src/routes/api/volumes/+server.ts (1)
3-3: Import path updated to new volume service module
UsingcreateVolumefrom$lib/services/docker/volume-servicematches the reorganized Docker services and removes the outdated unified service.src/routes/images/columns.ts (1)
1-1: Type import updated to centralized Docker types
ImportingServiceImagefrom the$lib/types/dockerbarrel ensures consistency with the new consolidated type definitions.src/routes/volumes/columns.ts (1)
5-5: Type import updated to centralized Docker types
Switching toServiceVolumefrom the$lib/types/dockerbarrel aligns with the refactored type modules and maintains consistency across UI columns.src/routes/api/containers/+server.ts (1)
3-3: Modular import path updated correctly
ImportingcreateContainerfromdocker/container-servicealigns with the refactor. Ensure that this service exports the expected function and that there are no lingering references to the olddocker-servicemodule in this route.src/routes/api/images/prune/+server.ts (1)
3-3: Image pruning import path is correct
ThepruneImagesimport from the newdocker/image-servicemodule matches the intended modularization. No further action required here.src/routes/api/stacks/create/+server.ts (1)
3-3: Stack creation import path is accurate
Switching tocreateStackfromdocker/stack-servicecorrectly reflects the split of compose logic into its own module.src/routes/api/networks/create/+server.ts (1)
3-3: Network creation import path is accurate
ThecreateNetworkimport fromdocker/network-serviceis consistent with the new modular architecture.src/routes/containers/columns.ts (1)
1-1: Import path updated to new centralized Docker types
TheServiceContainertype import has been updated to point at the new$lib/types/dockermodule, reflecting the migration from the monolithic service module to a dedicated types directory.src/routes/api/images/[id]/+server.ts (1)
3-3: Use new dedicated image-service module
TheremoveImageimport has been switched to$lib/services/docker/image-service, aligning this API route with the new modular service structure for Docker images.src/routes/api/volumes/[name]/+server.ts (1)
3-3: Use new dedicated volume-service module
TheremoveVolumeimport now references$lib/services/docker/volume-service, consistent with the refactoring of Docker services into resource-specific modules.src/routes/api/stacks/import/+server.ts (1)
3-3: Use new dedicated stack-service module
TheimportExternalStackimport path has been updated to$lib/services/docker/stack-service, matching the consolidation of stack-related operations into the new stack service.src/routes/api/networks/[id]/+server.ts (1)
3-3: Use new dedicated network-service module
TheremoveNetworkimport has been migrated to$lib/services/docker/network-service, ensuring this API handler uses the new modular network service.src/routes/columns.ts (1)
2-2: Update type imports to centralized Docker types.
ImportingServiceContainerandServiceImagefrom the new$lib/types/dockerbarrel aligns perfectly with the refactored type organization and ensures consistency across the codebase.src/hooks.server.ts (1)
1-1: Import service initialization from the new stack-service module.
Switching theinitComposeServiceimport to$lib/services/docker/stack-servicecorrectly reflects the relocation of Docker Compose initialization logic into its dedicated module.src/routes/stacks/+page.server.ts (1)
2-2: Redirect stack-load functions to the new stack-service.
UpdatingloadComposeStacksanddiscoverExternalStacksimports to$lib/services/docker/stack-serviceis consistent with the modularization of stack management.src/routes/api/docker/containers/+server.ts (1)
3-3: Point container listing to the dedicated container-service.
Changing the import oflistContainersto$lib/services/docker/container-servicecorrectly reflects the split of the monolithic Docker service into granular modules.src/routes/stacks/[stackId]/+page.server.ts (1)
1-2: Update stack-related imports and remove unused redirect.
Removing the unusedredirectimport and switching all stack operations (getStack,updateStack,startStack,stopStack,restartStack,removeStack) to$lib/services/docker/stack-servicealigns this route with the new service structure.src/routes/images/+page.server.ts (1)
2-3: Imports Updated to Dedicated Service and Type ModulesThe import paths for
listImagesandisImageInUsehave been correctly updated to$lib/services/docker/image-service, andEnhancedImageInfonow comes from the centralized$lib/types/dockerbarrel. These changes align with the modularization refactor and maintain consistency throughout the codebase.src/routes/volumes/+page.server.ts (1)
2-3: Imports Updated to Dedicated Volume Service and Type ModulesThe
listVolumesandisVolumeInUsefunctions have been correctly imported from$lib/services/docker/volume-service, andServiceVolumenow comes from the centralized$lib/types/dockerbarrel. This aligns with the new modular structure and ensures consistency.src/routes/containers/[id]/+page.server.ts (1)
2-2: Import Path Updated to Container ServiceThe Docker container management functions are now imported from the specialized
$lib/services/docker/container-servicemodule, reflecting the refactor. This maintains functionality and improves modularity.src/routes/networks/columns.ts (1)
5-5: Import Type from Centralized Docker Types ModuleThe
ServiceNetworktype import has been updated to$lib/types/docker, consolidating Docker-related type definitions. This change aligns with the broader modularization of types.src/routes/networks/+page.server.ts (1)
2-3: Imports Refactored to Network Service and Type ModulesThe
listNetworksfunction is now correctly imported from$lib/services/docker/network-service, andServiceNetworkis pulled from the centralized$lib/types/dockerbarrel. This matches the modular structure and improves maintainability.src/routes/containers/+page.server.ts (1)
1-5: Great refactoring of imports for better modularityThe changes to import from granular service modules rather than a monolithic docker service improves code organization and maintainability. This aligns well with the separation of concerns principle.
src/routes/containers/+page.svelte (1)
10-10: Import path correctly updated to reflect new type organizationThe updated import path aligns with the modular structure created in this refactoring effort.
src/lib/types/docker/index.ts (1)
1-7: Good use of barrel file for simplifying importsCreating a barrel file to re-export all Docker-related types is a good practice that will simplify imports throughout the codebase.
src/lib/types/docker/connection.type.ts (1)
1-8: Well-structured interface for Docker connection optionsThe
DockerConnectionOptionsinterface is well-defined with appropriate optional fields for Docker connection configuration. This supports the modular service architecture by centralizing connection types.src/routes/+page.server.ts (1)
2-5: Import paths updated correctly to reflect new modular structureThe import statements have been properly updated to use the new specialized service modules and type imports, which aligns with the modular architecture refactoring. This separation of concerns improves maintainability by making it clearer which specific Docker service each function belongs to.
src/lib/types/docker/image.type.ts (2)
1-11: Well-structured type definition for Docker imagesThe
ServiceImagetype provides a comprehensive representation of Docker image metadata with proper typing for all properties. This type centralization in a dedicated file follows TypeScript best practices and will make maintenance easier.
13-15: Good extension pattern for enhanced image informationUsing the intersection type pattern to extend
ServiceImagewith additional properties is a clean approach. TheinUseflag will be useful for UI components that need to display whether an image is currently being used by containers.src/lib/types/docker/container.type.ts (2)
1-15: Improved container type definition with proper Docker.Port typingThe
ServiceContainertype definition is well-structured with appropriate typing for all properties. The use ofDocker.Port[]from dockerode is a good practice as it leverages the library's type definitions rather than recreating them.
67-109: Well-documented container configuration interfacesThe container-related interfaces are clearly structured and adequately documented with comments. These types provide a comprehensive representation of the data needed for container operations, which will improve type safety throughout the application.
src/lib/services/docker/core.ts (1)
137-146: Good error handling in connection testThe implementation of
testDockerConnectionis well done with proper error handling and logging. This will be helpful for diagnosing Docker connection issues.
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
src/lib/services/docker/core.ts (1)
142-147: 🧹 Nitpick (assertive)Consider simplifying default export documentation.
The current comment on the default export is quite verbose. Consider simplifying it to be more concise while still explaining its purpose.
-/* The statement `export default getDockerClient;` is exporting the `getDockerClient` function as the -default export of the module. This means that when another module imports from this module without -specifying a particular named export, it will receive the `getDockerClient` function as the default -export. */ +/** + * Export getDockerClient as the default for convenient importing + */ export default getDockerClient;
♻️ Duplicate comments (7)
src/routes/containers/+page.svelte (1)
22-23: Improved type safety by replacing 'any' with specific type.Replacing the
anytype with the properServiceContainertype in the derived filter methods enhances type safety and catches potential errors at compile time rather than runtime.src/lib/types/docker/index.ts (1)
1-6: 🧹 Nitpick (assertive)Effective use of barrel file for centralized type exports.
Creating a barrel file to centralize and re-export Docker-related types is a good practice that simplifies imports across the codebase. This aligns well with the PR's goal of reorganizing services and types.
However, add a semicolon at the end of line 6 for consistency with the other export statements:
-export * from './connection.type' +export * from './connection.type';src/lib/services/docker/core.ts (1)
1-5: 🧹 Nitpick (assertive)Good module structure with clear imports and defaults.
The module properly imports the Docker client and the connection options type. Setting a default Docker host is a good practice.
However, the type annotation for
dockerHostcan be omitted as the type is inferred from the initial value:-export let dockerHost = 'unix:///var/run/docker.sock'; +export let dockerHost = 'unix:///var/run/docker.sock';src/lib/services/docker/volume-service.ts (1)
49-55: Previous note about sequentialinspectstill appliesThe container-by-container
awaitloop is O(N) sequential and slows down hosts with many containers. Parallelising (or early-return on first hit) would cut latency.src/lib/services/docker/network-service.ts (1)
48-56: Protect against default networks when caller passes an ID, not just a nameEarlier feedback suggested inspecting the network to compare
details.Nameas well as the supplied identifier. The current implementation still only checks the rawnetworkId, letting Docker’s builtin networks slip through when the caller passes their ID.- if (DEFAULT_NETWORK_NAMES.has(networkId)) { - throw new Error(`Network "${networkId}" is managed by Docker and cannot be removed.`); - } - const docker = getDockerClient(); - const network = docker.getNetwork(networkId); + const docker = getDockerClient(); + const network = docker.getNetwork(networkId); + const details = await network.inspect(); + if (DEFAULT_NETWORK_NAMES.has(networkId) || DEFAULT_NETWORK_NAMES.has(details.Name)) { + throw new Error(`Network "${details.Name}" is managed by Docker and cannot be removed.`); + }src/lib/services/docker/container-service.ts (2)
50-74: 🛠️ Refactor suggestionAdd a specific return type to
getContainerto improve type safety.The function lacks an explicit return type, which weakens TypeScript's type checking and makes it harder for consumers to understand the function's contract.
-export async function getContainer(containerId: string) { +export async function getContainer(containerId: string): Promise<ContainerDetails | null> {You'll need to import the
ContainerDetailstype from your Docker types module and ensure it matches the structure of the object being returned.
174-194:⚠️ Potential issueFix handling of stream when
follow === trueto prevent runtime errors.When
followis set totrue, dockerode returns aReadableStreaminstead of aBuffer, causing the.toString()call to throw aTypeError.-const logsBuffer = options.follow === true ? await container.logs({ ...logOptions, follow: true }) : await container.logs({ ...logOptions, follow: false }); -let logString = logsBuffer.toString(); +const raw = await container.logs({ ...logOptions, follow: !!options.follow }); + +// Handle both Buffer and Stream return types +const logsBuffer: Buffer = Buffer.isBuffer(raw) + ? raw + : await new Promise<Buffer>((resolve, reject) => { + const chunks: Buffer[] = []; + raw.on('data', (chunk: Buffer) => chunks.push(chunk)); + raw.on('end', () => resolve(Buffer.concat(chunks))); + raw.on('error', reject); + }); + +let logString = logsBuffer.toString();This change correctly handles both return types from the dockerode logs method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.prettierrc(1 hunks)src/lib/services/docker/container-service.ts(1 hunks)src/lib/services/docker/core.ts(1 hunks)src/lib/services/docker/image-service.ts(1 hunks)src/lib/services/docker/network-service.ts(1 hunks)src/lib/services/docker/stack-service.ts(17 hunks)src/lib/services/docker/volume-service.ts(1 hunks)src/lib/types/docker/container.type.ts(2 hunks)src/lib/types/docker/index.ts(1 hunks)src/routes/containers/+page.svelte(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/lib/services/docker/image-service.ts (2)
src/lib/types/docker/image.type.ts (1)
ServiceImage(1-11)src/lib/services/docker/core.ts (2)
getDockerClient(24-32)dockerHost(5-5)
src/lib/services/docker/core.ts (1)
src/lib/types/docker/connection.type.ts (1)
DockerConnectionOptions(1-8)
src/lib/services/docker/network-service.ts (2)
src/lib/types/docker/network.type.ts (1)
ServiceNetwork(1-9)src/lib/services/docker/core.ts (2)
getDockerClient(24-32)dockerHost(5-5)
src/lib/services/docker/volume-service.ts (2)
src/lib/types/docker/volume.type.ts (1)
ServiceVolume(1-7)src/lib/services/docker/core.ts (2)
getDockerClient(24-32)dockerHost(5-5)
src/lib/services/docker/container-service.ts (2)
src/lib/types/docker/container.type.ts (3)
ServiceContainer(4-15)ContainerConfig(73-101)ContainerCreate(68-70)src/lib/services/docker/core.ts (2)
getDockerClient(24-32)dockerHost(5-5)
src/lib/services/docker/stack-service.ts (1)
src/lib/types/docker/stack.type.ts (1)
Stack(18-30)
🪛 Biome (1.9.4)
src/lib/services/docker/image-service.ts
[error] 64-64: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/lib/services/docker/volume-service.ts
[error] 117-117: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/lib/services/docker/container-service.ts
[error] 314-314: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 318-318: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 321-321: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 256-256: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (10)
src/routes/containers/+page.svelte (1)
10-10: Good use of specific type imports.Importing both
ServiceContainerandContainerConfigtypes directly from the container type file improves type safety and aligns with the refactoring goals of the PR.src/lib/types/docker/container.type.ts (2)
1-15: Well-structured type definition for container data.Creating a dedicated
ServiceContainertype that maps to the Docker container data structure improves type safety when working with containers. The type includes all essential container properties and properly uses the Docker types from dockerode.
66-109: Good organization of container-related interfaces.Moving the interfaces for container creation and configuration to a logical location in the file improves readability. The types are well-documented with comments that explain their purpose.
src/lib/services/docker/core.ts (6)
7-16: Well-documented getDockerInfo function.The
getDockerInfofunction includes comprehensive JSDoc comments that explain its purpose and return value. The implementation is clean and straightforward.
18-32: Improved Docker client initialization.The
getDockerClientfunction now uses the extractedcreateConnectionOptionshelper function, which reduces code duplication and improves maintainability.
34-64: Well-structured Docker initialization function.The
initializeDockerfunction properly handles different connection types and TLS options. UsingRecord<string, any>instead of justanyprovides better type safety while maintaining flexibility.
66-91: Good error handling in updateDockerConnection.The function includes proper error handling with try/catch and logs helpful information both for successful connections and errors. Good validation to check for empty host strings.
93-109: Useful Docker connection testing function.The
testDockerConnectionfunction provides a convenient way to validate whether the Docker connection is working properly. The error handling is appropriate and the return type is clearly defined.
111-140: Well-implemented connection options helper function.The
createConnectionOptionsfunction effectively centralizes the logic for parsing different types of Docker connection strings. It handles Unix sockets, TCP, and HTTPS connections with appropriate defaults for ports.src/lib/services/docker/volume-service.ts (1)
80-92:⚠️ Potential issue
createVolumereturns undefined fields – inspect after creation
docker.createVolume()yields aVolumehandle, not an inspected object. Properties such asName,Driver, etc. are thereforeundefined, which breaks callers relying onServiceVolume.- const volume = await docker.createVolume(options); - // Return the creation response which contains basic info - return { - name: volume.Name, - driver: volume.Driver, - mountpoint: volume.Mountpoint, - labels: volume.Labels || {}, - scope: volume.Scope || 'local' - }; + const volumeHandle = await docker.createVolume(options); + const inspectInfo = await volumeHandle.inspect(); + + return { + name: inspectInfo.Name, + driver: inspectInfo.Driver, + mountpoint: inspectInfo.Mountpoint, + labels: inspectInfo.Labels || {}, + scope: inspectInfo.Scope || 'local' + };Likely an incorrect or invalid review comment.
| "singleQuote": true, | ||
| "trailingComma": "none", | ||
| "printWidth": 600, | ||
| "printWidth": 450, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Reduced printWidth to 450 for improved readability
Lowering the maximum line length helps enforce clearer formatting across the newly modularized services.
Consider verifying that no existing lines exceed this new threshold to avoid unexpected formatting diffs.
Run the following script to detect any lines longer than 450 characters:
🏁 Script executed:
#!/bin/bash
# Verify no lines exceed the new printWidth of 450
rg -n '.{451}' .Length of output: 4742
Lines exceeding the new printWidth of 450 detected
Lowering printWidth to 450 helps enforce clearer formatting, but the following lines will be reformatted unexpectedly unless addressed. Please break these up (or extract long class strings into variables) or reconsider the threshold.
• src/routes/+page.svelte:307
• src/lib/components/ui/checkbox/checkbox.svelte:10
• src/lib/components/ui/tabs/tabs-trigger.svelte:8
• src/lib/components/ui/select/select-content.svelte:24
• src/lib/components/ui/dialog/dialog-content.svelte:22
• src/lib/components/ui/dropdown-menu/dropdown-menu-content.svelte:17
• src/lib/components/ui/switch/switch.svelte:8
• src/lib/components/ui/input/input.svelte:14
Suggested actions:
- Break long attribute strings across multiple lines.
- Extract large
class={…}values into constants. - Or, if these patterns are unavoidable, consider increasing
printWidth.
| const stoppedContainers = $derived(containers?.filter((c: ServiceContainer) => c.state === 'exited').length || 0); | ||
|
|
||
| // Calculate total containers | ||
| const totalContainers = $derived(containers?.length || 0); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding type annotation to totalContainers for consistency.
While you've updated the type annotations for runningContainers and stoppedContainers, the totalContainers derived store remains untyped. For consistency, consider adding the same type annotation here.
-const totalContainers = $derived(containers?.length || 0);
+const totalContainers = $derived(containers?.length || 0);Committable suggestion skipped: line range outside the PR's diff.
| export async function removeImage(imageId: string, force: boolean = false): Promise<void> { | ||
| try { | ||
| const docker = getDockerClient(); | ||
| const image = docker.getImage(imageId); | ||
| await image.remove({ force }); | ||
| console.log(`Docker Service: Image "${imageId}" removed successfully.`); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove the redundant type annotation on force & satisfy Biome
Biome flags force: boolean = false as a “trivially inferred” type. Dropping the explicit type keeps the signature shorter and makes the linter happy.
-export async function removeImage(imageId: string, force: boolean = false): Promise<void> {
+export async function removeImage(imageId: string, force = false): Promise<void> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function removeImage(imageId: string, force: boolean = false): Promise<void> { | |
| try { | |
| const docker = getDockerClient(); | |
| const image = docker.getImage(imageId); | |
| await image.remove({ force }); | |
| console.log(`Docker Service: Image "${imageId}" removed successfully.`); | |
| export async function removeImage(imageId: string, force = false): Promise<void> { | |
| try { | |
| const docker = getDockerClient(); | |
| const image = docker.getImage(imageId); | |
| await image.remove({ force }); | |
| console.log(`Docker Service: Image "${imageId}" removed successfully.`); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
| const parseRepoTag = (tag: string | undefined): { repo: string; tag: string } => { | ||
| if (!tag || tag === '<none>:<none>') { | ||
| return { repo: '<none>', tag: '<none>' }; | ||
| } | ||
| const withoutDigest = tag.split('@')[0]; | ||
| const lastSlash = withoutDigest.lastIndexOf('/'); | ||
| const lastColon = withoutDigest.lastIndexOf(':'); | ||
| if (lastColon === -1 || lastColon < lastSlash) { | ||
| return { repo: withoutDigest, tag: 'latest' }; | ||
| } | ||
| return { | ||
| repo: withoutDigest.substring(0, lastColon), | ||
| tag: withoutDigest.substring(lastColon + 1) | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract parseRepoTag into a shared util to avoid re-allocation on every call
parseRepoTag is re-created each time listImages runs. Hoisting it (or moving it to a small util module) removes the per-call allocation and makes it re-usable from other services that need the same parsing logic.
- const parseRepoTag = (tag: string | undefined): { repo: string; tag: string } => {
+function parseRepoTag(tag: string | undefined): { repo: string; tag: string } {Committable suggestion skipped: line range outside the PR's diff.
| const pullTimeout = 10 * 60 * 1000; // 10 min | ||
|
|
||
| // Wait for the pull to complete by consuming the stream with 10 minute timeout | ||
| const result = await Promise.race([new Promise((resolve, reject) => docker.modem.followProgress(stream, (err, out) => (err ? reject(err) : resolve(out)))), new Promise((_, reject) => setTimeout(() => reject(new Error(`Pull timed-out after ${pullTimeout} ms`)), pullTimeout))]); | ||
|
|
||
| console.log(`Docker Service: Image "${imageRef}" pulled successfully.`); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
result is unused – drop the binding or expose pull feedback
The value returned from Promise.race is stored in result but never referenced. Either propagate the data to the caller or simply await the promise to avoid the unused-variable overhead.
- const result = await Promise.race([
+ await Promise.race([📝 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 pullTimeout = 10 * 60 * 1000; // 10 min | |
| // Wait for the pull to complete by consuming the stream with 10 minute timeout | |
| const result = await Promise.race([new Promise((resolve, reject) => docker.modem.followProgress(stream, (err, out) => (err ? reject(err) : resolve(out)))), new Promise((_, reject) => setTimeout(() => reject(new Error(`Pull timed-out after ${pullTimeout} ms`)), pullTimeout))]); | |
| console.log(`Docker Service: Image "${imageRef}" pulled successfully.`); | |
| const pullTimeout = 10 * 60 * 1000; // 10 min | |
| // Wait for the pull to complete by consuming the stream with 10 minute timeout | |
| await Promise.race([ | |
| new Promise((resolve, reject) => | |
| docker.modem.followProgress( | |
| stream, | |
| (err, out) => (err ? reject(err) : resolve(out)) | |
| ) | |
| ), | |
| new Promise((_, reject) => | |
| setTimeout( | |
| () => | |
| reject( | |
| new Error(`Pull timed-out after ${pullTimeout} ms`) | |
| ), | |
| pullTimeout | |
| ) | |
| ) | |
| ]); | |
| console.log(`Docker Service: Image "${imageRef}" pulled successfully.`); |
| if (config.ports?.length) { | ||
| const exposedPorts: Record<string, {}> = {}; | ||
| const portBindings: Record<string, Array<{ HostPort: string }>> = {}; | ||
| for (const p of config.ports) { | ||
| const key = `${p.containerPort}/tcp`; | ||
| exposedPorts[key] = {}; | ||
| portBindings[key] = [{ HostPort: p.hostPort }]; | ||
| } | ||
| containerOptions.ExposedPorts = exposedPorts; | ||
| containerOptions.HostConfig = { | ||
| ...(containerOptions.HostConfig ?? {}), | ||
| PortBindings: portBindings | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove empty object type {} in Record to improve type safety.
Using {} as a type is discouraged as it can accept more values than intended. Biome flagged this as a violation of the lint/complexity/noBannedTypes rule.
- const exposedPorts: Record<string, {}> = {};
+ const exposedPorts: Record<string, object> = {};Alternatively, you could be even more explicit with:
const exposedPorts: Record<string, Record<string, never>> = {};🧰 Tools
🪛 Biome (1.9.4)
[error] 256-256: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| if (error.message && error.message.includes('IPAMConfig')) { | ||
| throw new Error(`Failed to create container: Invalid IP address configuration for network "${config.network}". ${error.message}`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use optional chaining when checking error messages.
The current approach using the logical AND operator works but is less concise than using optional chaining. This was flagged by the static analyzer.
-if (error.message && error.message.includes('IPAMConfig')) {
+if (error.message?.includes('IPAMConfig')) {
throw new Error(`Failed to create container: Invalid IP address configuration for network "${config.network}". ${error.message}`);
}Apply the same change to similar error handling patterns on lines 318 and 321.
📝 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 (error.message && error.message.includes('IPAMConfig')) { | |
| throw new Error(`Failed to create container: Invalid IP address configuration for network "${config.network}". ${error.message}`); | |
| } | |
| if (error.message?.includes('IPAMConfig')) { | |
| throw new Error(`Failed to create container: Invalid IP address configuration for network "${config.network}". ${error.message}`); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 314-314: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| // 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.
🧹 Nitpick (assertive)
Remove redundant null check with nullish coalescing operator.
The current implementation has a conditional assignment followed by potential use, which could be simplified.
if (config.volumes && config.volumes.length > 0) {
- containerOptions.HostConfig = containerOptions.HostConfig || {};
+ containerOptions.HostConfig = containerOptions.HostConfig ?? {};
containerOptions.HostConfig.Binds = config.volumes.map((vol) => `${vol.source}:${vol.target}${vol.readOnly ? ':ro' : ''}`);
}This matches the pattern used for port bindings and is more consistent with modern TypeScript patterns.
📝 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 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' : ''}`); | |
| } | |
| // 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' : ''}`); | |
| } |
| if (error.message && error.message.includes('NanoCpus')) { | ||
| throw new Error(`Invalid CPU limit specified: ${error.message}`); | ||
| } | ||
| if (error.message && error.message.includes('Memory')) { | ||
| throw new Error(`Invalid Memory limit specified: ${error.message}`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use optional chaining consistently in error handling.
Similar to the previous comment, apply optional chaining to the remaining error checks.
-if (error.message && error.message.includes('NanoCpus')) {
+if (error.message?.includes('NanoCpus')) {
throw new Error(`Invalid CPU limit specified: ${error.message}`);
}
-if (error.message && error.message.includes('Memory')) {
+if (error.message?.includes('Memory')) {
throw new Error(`Invalid Memory limit specified: ${error.message}`);
}📝 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 (error.message && error.message.includes('NanoCpus')) { | |
| throw new Error(`Invalid CPU limit specified: ${error.message}`); | |
| } | |
| if (error.message && error.message.includes('Memory')) { | |
| throw new Error(`Invalid Memory limit specified: ${error.message}`); | |
| } | |
| if (error.message?.includes('NanoCpus')) { | |
| throw new Error(`Invalid CPU limit specified: ${error.message}`); | |
| } | |
| if (error.message?.includes('Memory')) { | |
| throw new Error(`Invalid Memory limit specified: ${error.message}`); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 318-318: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 321-321: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| containerOptions.HostConfig = containerOptions.HostConfig || {}; | ||
| if (!containerOptions.NetworkingConfig) { | ||
| containerOptions.HostConfig.NetworkMode = config.network; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use nullish coalescing for HostConfig assignment.
Similar to the volumes section, this should use the nullish coalescing operator for consistency.
if (config.network) {
- containerOptions.HostConfig = containerOptions.HostConfig || {};
+ containerOptions.HostConfig = containerOptions.HostConfig ?? {};
if (!containerOptions.NetworkingConfig) {
containerOptions.HostConfig.NetworkMode = config.network;
}📝 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.
| containerOptions.HostConfig = containerOptions.HostConfig || {}; | |
| if (!containerOptions.NetworkingConfig) { | |
| containerOptions.HostConfig.NetworkMode = config.network; | |
| } | |
| if (config.network) { | |
| containerOptions.HostConfig = containerOptions.HostConfig ?? {}; | |
| if (!containerOptions.NetworkingConfig) { | |
| containerOptions.HostConfig.NetworkMode = config.network; | |
| } | |
| } |
1 participant
Fixes: #27
Summary by CodeRabbit
New Features
Refactor
Chores
Style