Conversation
WalkthroughThis change introduces an automated update system for Docker containers and stacks, integrating both backend services and frontend controls. New services are added to periodically check and apply updates to containers and stacks labeled or flagged for auto-update, based on configurable global settings. The backend exposes new API endpoints for manual and scheduled update checks, and settings now include an interval for auto-update checks. The frontend is updated to allow users to enable or disable auto-update for containers and stacks and to configure the auto-update interval in the application settings UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant Scheduler
participant AutoUpdateService
participant Docker
User->>Frontend: Enable auto-update (container/stack or settings)
Frontend->>API: Save auto-update config (container/stack/settings)
API->>Scheduler: (On startup/settings change) initAutoUpdateScheduler()
Scheduler->>AutoUpdateService: Periodic checkAndUpdateContainers()/checkAndUpdateStacks()
AutoUpdateService->>Docker: List containers/stacks, pull images, restart/redeploy if needed
AutoUpdateService->>Scheduler: Return update results
Scheduler->>API: (On manual trigger) Run update checks, return results
API->>Frontend: Show update status/results
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/hooks.server.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/auto-update-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/container-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: 8
🔭 Outside diff range comments (1)
src/lib/services/docker/stack-service.ts (1)
486-494:⚠️ Potential issue
updateStackomitsmetain its returned objectConsumers (e.g. the auto-update UI and service layer) rely on
stack.meta.
Return the updated metadata to keep the contract consistent withgetStack,createStack, and the newlistStacks.updatedAt: updatedMeta.updatedAt, + meta: updatedMeta
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
src/hooks.server.ts(1 hunks)src/lib/services/docker/auto-update-service.ts(1 hunks)src/lib/services/docker/container-service.ts(1 hunks)src/lib/services/docker/scheduler-service.ts(1 hunks)src/lib/services/docker/stack-service.ts(3 hunks)src/lib/services/settings-service.ts(1 hunks)src/lib/types/docker/stack.type.ts(2 hunks)src/lib/types/settings.ts(1 hunks)src/routes/api/stacks/[stackId]/+server.ts(1 hunks)src/routes/api/system/auto-update/+server.ts(1 hunks)src/routes/containers/create-container-dialog.svelte(4 hunks)src/routes/settings/+page.server.ts(2 hunks)src/routes/settings/tabs/app-settings.svelte(2 hunks)src/routes/stacks/[stackId]/+page.server.ts(3 hunks)src/routes/stacks/[stackId]/+page.svelte(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/hooks.server.ts (2)
src/lib/services/docker/stack-service.ts (1)
initComposeService(22-32)src/lib/services/docker/scheduler-service.ts (1)
initAutoUpdateScheduler(10-35)
src/routes/stacks/[stackId]/+page.server.ts (1)
src/lib/services/docker/stack-service.ts (1)
updateStack(449-499)
src/routes/api/system/auto-update/+server.ts (2)
src/lib/services/settings-service.ts (1)
getSettings(129-182)src/lib/services/docker/auto-update-service.ts (2)
checkAndUpdateContainers(15-85)checkAndUpdateStacks(90-144)
src/lib/services/docker/stack-service.ts (1)
src/lib/types/docker/stack.type.ts (2)
Stack(19-32)StackMeta(1-6)
src/lib/services/docker/scheduler-service.ts (2)
src/lib/services/settings-service.ts (1)
getSettings(129-182)src/lib/services/docker/auto-update-service.ts (2)
checkAndUpdateContainers(15-85)checkAndUpdateStacks(90-144)
🪛 Biome (1.9.4)
src/routes/settings/+page.server.ts
[error] 29-29: 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/auto-update-service.ts
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/lib/services/docker/stack-service.ts
[error] 911-911: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
🔇 Additional comments (32)
src/lib/types/settings.ts (1)
4-4: LGTM - Clean interface extensionThis is a clean addition of the
autoUpdateIntervalproperty to theSettingsDatainterface, using the appropriate numeric type for a time interval value.src/lib/services/settings-service.ts (1)
19-19: Appropriate default value for auto-update intervalSetting the default auto-update interval to 5 minutes provides a reasonable balance between keeping containers updated and avoiding excessive system load from too-frequent checks.
src/lib/services/docker/container-service.ts (1)
68-69: Good addition of container labels supportIncluding container labels in the returned object is necessary for the auto-update feature to identify containers marked for automatic updates (via labels like
arcane.auto-update=true).src/routes/containers/create-container-dialog.svelte (4)
16-16: Appropriate import for the auto-update toggle.The Switch component import is correctly added for the auto-update feature implementation.
90-92: Good addition of auto-update state.The addition of a reactive state variable for auto-update is clear and follows the established pattern in the component.
225-229: Well-implemented auto-update label logic.The conditional addition of the
arcane.auto-updatelabel is a clean implementation that properly marks containers for auto-update when the feature is enabled by the user.
618-624: UI for auto-update is well integrated.The auto-update toggle UI is clearly implemented with appropriate labeling and description text. It's properly placed in the Advanced tab section with other configuration options.
src/routes/settings/+page.server.ts (2)
31-33: Good interval validation logic.The validation logic ensures the auto-update interval stays within reasonable bounds (5 minutes to 24 hours), which is important for preventing system overload from too-frequent checks or excessively delayed updates.
97-97: Properly adding autoUpdateInterval to the settings.The validated interval value is correctly included in the settings object that will be saved.
src/lib/types/docker/stack.type.ts (3)
5-5: Good addition of autoUpdate flag to StackMeta.The optional autoUpdate property is appropriately added to the StackMeta interface, enabling tracking of auto-update status for stacks.
31-31: Proper integration of meta information in Stack interface.Adding the optional meta property to the Stack interface provides a clean way to include metadata like autoUpdate without cluttering the main interface.
37-37: Well-structured StackUpdate interface extension.The autoUpdate flag is appropriately added to the StackUpdate interface, allowing for updating this setting independently from other stack properties.
src/routes/settings/tabs/app-settings.svelte (2)
20-20: Good derivation of autoUpdateInterval state.The reactive derived variable properly handles fallbacks from form values to settings to default, maintaining consistency with other settings in the file.
114-120: Well-implemented UI for autoUpdateInterval setting.The UI for configuring the auto-update interval is:
- Properly conditional on autoUpdate being enabled
- Includes appropriate min/max constraints (5-1440 minutes)
- Has clear labeling and helpful description text
- Follows the established UI pattern of other similar settings
src/routes/stacks/[stackId]/+page.svelte (6)
18-18: LGTM: Import Switch component for auto-update featureThis import adds the required UI component for toggling the auto-update feature.
33-33: LGTM: Add tracking for original autoUpdate valueThis properly adds tracking for the original autoUpdate value to detect changes.
35-35: LGTM: Update change detection logic to include autoUpdateChange detection now properly tracks modifications to the autoUpdate setting.
57-61: LGTM: Include autoUpdate in the stack update payloadThe autoUpdate property is now properly included in the JSON payload sent to the API.
75-75: LGTM: Reset original autoUpdate value after saveThis correctly updates the reference value after a successful save to reset the change detection.
216-222: LGTM: Add UI controls for auto-update featureGood implementation of the auto-update toggle with descriptive text explaining the feature to users.
src/routes/stacks/[stackId]/+page.server.ts (4)
21-21: LGTM: Add autoUpdate to editorStateThis correctly retrieves the autoUpdate value from stack metadata with a sensible default.
35-35: LGTM: Add default autoUpdate value in error caseProviding a default value for autoUpdate in the error case ensures consistent behavior.
49-49: LGTM: Extract autoUpdate from form dataGood handling of form data to extract the autoUpdate boolean value.
52-52: LGTM: Include autoUpdate in updateStack callThe autoUpdate parameter is correctly passed to the service layer.
src/routes/api/stacks/[stackId]/+server.ts (3)
6-7: LGTM: Simplified parameter extractionClean refactoring to directly extract the needed parameters from request and params.
10-11: LGTM: Update API call and responseProperly includes autoUpdate in the service call and simplifies the response.
13-14: LGTM: Consolidated error handlingError handling is now more concise while still providing useful information.
src/lib/services/docker/scheduler-service.ts (4)
1-6: LGTM: Set up imports and timer trackingGood approach to track the timer for proper cleanup and reuse of existing services.
10-35: LGTM: Robust scheduler initializationThe scheduler initialization handles settings correctly, clears existing timers properly, and uses appropriate defaults.
The initial run followed by scheduled runs ensures updates are checked immediately after initialization.
40-54: LGTM: Comprehensive update check implementationGood error handling and detailed logging of results for both container and stack updates.
59-65: LGTM: Clean scheduler shutdownProperly clears the interval and resets the timer reference to prevent memory leaks.
src/lib/services/docker/stack-service.ts (1)
457-463:⚠️ Potential issuePreserve existing
autoUpdateflag when no new value is suppliedSpreading
metafirst and then conditionally overriding withupdates.autoUpdateworks, but the current logic unintentionally removes an existing flag when the caller omits the property (i.e. suppliesundefined).
A safer pattern keeps the original value unless the caller explicitly sets it:-const updatedMeta: StackMeta = { - ...meta, - name: updates.name || meta.name, - updatedAt: new Date().toISOString(), - // Add auto-update flag if provided - ...(updates.autoUpdate !== undefined ? { autoUpdate: updates.autoUpdate } : {}) -}; +const updatedMeta: StackMeta = { + ...meta, + name: updates.name ?? meta.name, + updatedAt: new Date().toISOString(), + // Only override when caller explicitly supplies a boolean + autoUpdate: updates.autoUpdate !== undefined ? updates.autoUpdate : meta.autoUpdate +};Likely an incorrect or invalid review comment.
| try { | ||
| const [containerResults, stackResults] = await Promise.all([checkAndUpdateContainers(), checkAndUpdateStacks()]); | ||
|
|
||
| return json({ | ||
| success: true, | ||
| containers: containerResults, | ||
| stacks: stackResults | ||
| }); | ||
| } catch (error: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Manual trigger is skipped when global auto-update is disabled
checkAndUpdateContainers/Stacks return early when settings.autoUpdate === false, so a POST request does nothing unless the global switch is on.
That defeats the purpose of a manual “Check now” button.
Consider adding a force flag to the service functions or calling lower-level helpers directly to ensure the POST route always performs a check irrespective of global settings.
🤖 Prompt for AI Agents (early access)
In src/routes/api/system/auto-update/+server.ts around lines 28 to 36, the manual trigger POST request does nothing if the global auto-update setting is disabled because checkAndUpdateContainers and checkAndUpdateStacks return early. To fix this, modify these functions to accept a force flag that bypasses the global setting check, or call lower-level update helpers directly in the POST handler to ensure the manual trigger always performs the update check regardless of the global auto-update setting.
|
@codetabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
src/routes/settings/+page.server.ts (1)
61-63: 🧹 Nitpick (assertive)Use
Number.parseIntfor consistency
parseIntis still used here, so the Biome warning will persist.-const parsedInterval = parseInt(pollingIntervalStr, 10); +const parsedInterval = Number.parseInt(pollingIntervalStr, 10);🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: 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/stack-service.ts (1)
352-363: 🧹 Nitpick (assertive)Be aware of larger payloads when returning
composeContent
getStacknow returns the full compose file string. For large stacks this could be hundreds of KB and will travel over every API call that relies ongetStack. Make sure only endpoints/clients that truly need the compose are calling this, or consider an optionalincludeComposeflag to avoid unnecessary transfer.
♻️ Duplicate comments (1)
src/lib/services/docker/auto-update-service.ts (1)
70-73: SecondpullImagecall is still redundant
checkContainerImageUpdatealready pulled the image to detect updates; pulling again here doubles network I/O and slows the loop.Refer to the earlier review on the same point – the recommendation still applies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/hooks.server.ts(1 hunks)src/lib/services/docker/auto-update-service.ts(1 hunks)src/lib/services/docker/container-service.ts(2 hunks)src/lib/services/docker/stack-service.ts(3 hunks)src/routes/settings/+page.server.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/hooks.server.ts (2)
src/lib/services/docker/stack-service.ts (1)
initComposeService(22-32)src/lib/services/docker/scheduler-service.ts (1)
initAutoUpdateScheduler(10-35)
src/lib/services/docker/container-service.ts (3)
src/lib/types/docker/container.type.ts (1)
ServiceContainer(4-15)src/lib/services/docker/core.ts (1)
getDockerClient(24-32)src/lib/types/errors.ts (1)
DockerApiError(55-65)
src/lib/services/docker/stack-service.ts (1)
src/lib/types/docker/stack.type.ts (2)
Stack(19-32)StackMeta(1-6)
🪛 Biome (1.9.4)
src/lib/services/docker/container-service.ts
[error] 449-449: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (3)
src/hooks.server.ts (1)
5-10: Fail-fast initialization looks good – double-check top-levelawaitportabilityGreat job wrapping both service initializers in a single
try/await Promise.all(...)and exiting on failure – this satisfies the “don’t run in a degraded state” concern from the previous review.One small thing to verify: top-level
awaitrequires the code to be emitted as an ES module. SvelteKit’s default Vite setup handles this, but if the project ever targets an older Node runtime (< v14.8) or is compiled to CommonJS, the build will fail. Nothing to change in code right now—just confirm your tooling and deployment environment support TLA.src/routes/settings/+page.server.ts (1)
29-33: 👍 Switched toNumber.parseIntand added clampingThe new parsing plus min/max clamp (5 – 1440) removes the linter warning and guards against out-of-range input. Looks solid.
src/lib/services/docker/stack-service.ts (1)
460-463: Avoid silently omittingautoUpdatewhenfalseThe spread (
...(updates.autoUpdate !== undefined ? …)) means sending{ autoUpdate: false }works, but omitting the key keeps the previous value. That’s correct. Just ensure callers always sendfalseinstead ofundefinedwhen they intend to disable auto-update to prevent accidental persistence of an oldtruevalue.
| export async function listStacks(includeExternal = false): Promise<Stack[]> { | ||
| // Get managed stacks | ||
| const managedStacks = await loadComposeStacks(); | ||
|
|
||
| // Add meta information to managed stacks | ||
| const enrichedManagedStacks = await Promise.all( | ||
| managedStacks.map(async (stack) => { | ||
| try { | ||
| // Read the meta file to get autoUpdate property | ||
| const metaPath = await getStackMetaPath(stack.id); | ||
| const metaContent = await fs.readFile(metaPath, 'utf8'); | ||
| const meta = JSON.parse(metaContent) as StackMeta; | ||
|
|
||
| // Return stack with meta included | ||
| return { | ||
| ...stack, | ||
| meta | ||
| }; | ||
| } catch (err) { | ||
| console.warn(`Failed to read meta for stack ${stack.id}:`, err); | ||
| return stack; // Return stack without meta if there was an error | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| // Get external stacks if requested | ||
| let externalStacks: Stack[] = []; | ||
| if (includeExternal) { | ||
| externalStacks = await discoverExternalStacks(); | ||
| } | ||
|
|
||
| // Combine managed and external stacks | ||
| return [...enrichedManagedStacks, ...externalStacks]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
listStacks duplicates meta-file I/O – consider re-using work done in loadComposeStacks
loadComposeStacks() already reads each meta.json; listStacks reads it again for every stack, doubling disk I/O. An easy win:
- const managedStacks = await loadComposeStacks();
- const enrichedManagedStacks = await Promise.all(
- managedStacks.map(async (stack) => {
- ...
- })
- );
+ const enrichedManagedStacks = await loadComposeStacks(); // meta already present if we enrich there onceor enrich inside loadComposeStacks and return it in one pass.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/stack-service.ts around lines 911 to 944, the listStacks function redundantly reads the meta.json file for each stack after loadComposeStacks has already done so, causing duplicated disk I/O. To fix this, modify loadComposeStacks to include the meta information when it loads each stack so that listStacks can directly use the enriched stacks without re-reading meta files. This consolidation will eliminate the extra file reads and improve performance.
| const containers = await listContainers(true); // Get all containers including the new one | ||
| const newServiceContainer = containers.find((c) => c.id === newContainer.id); | ||
|
|
||
| if (!newServiceContainer) { | ||
| throw new Error(`Container ${newContainer.id} was created but not found in container list`); | ||
| } | ||
|
|
||
| return newServiceContainer; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Race: new container may not appear in listContainers immediately
Right after startContainer, Docker sometimes needs a few milliseconds before listContainers shows the new container. Instead of a second round-trip you can get the ID directly:
return {
id: newContainer.id,
names: [originalContainer.name],
name: originalContainer.name,
image: createOptions.Image,
imageId: '', // populate via newContainer.inspect() if needed
command: createOptions.Cmd?.join(' ') ?? '',
created: Date.now(),
state: 'running',
status: 'running',
ports: []
};Or poll inspect() on newContainer instead of calling listContainers.
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/container-service.ts around lines 492 to 499, the current code calls listContainers immediately after starting a new container, which may not list the container due to a race condition. To fix this, avoid relying on listContainers and instead construct the container object directly using newContainer.id and the original container's properties, or implement a polling mechanism using newContainer.inspect() until the container is available. This ensures you return the container info reliably without waiting for listContainers to update.
| HostConfig: { | ||
| PortBindings: originalContainer.networkSettings?.Ports || {}, | ||
| NetworkMode: originalContainer.networkSettings?.Networks?.bridge ? 'bridge' : Object.keys(originalContainer.networkSettings?.Networks || {})[0] || undefined, | ||
| Binds: originalContainer.mounts?.filter((mount) => mount.Type === 'bind' || mount.Type === 'volume').map((mount) => `${mount.Source}:${mount.Destination}${mount.RW ? '' : ':ro'}`) | ||
| }, | ||
| Cmd: originalContainer.config.Cmd, | ||
| Entrypoint: originalContainer.config.Entrypoint, | ||
| WorkingDir: originalContainer.config.WorkingDir, | ||
| User: originalContainer.config.User, | ||
| Volumes: originalContainer.config.Volumes, | ||
| Tty: originalContainer.config.Tty, | ||
| OpenStdin: originalContainer.config.OpenStdin, | ||
| StdinOnce: originalContainer.config.StdinOnce | ||
| }; | ||
|
|
||
| // If we need to add custom network configuration for non-default networks | ||
| if (originalContainer.networkSettings?.Networks) { | ||
| const networks = Object.entries(originalContainer.networkSettings.Networks); | ||
| // If container uses a non-default network, set it up | ||
| if (networks.length > 0 && networks[0][0] !== 'bridge') { | ||
| const [networkName, networkConfig] = networks[0]; | ||
| createOptions.HostConfig!.NetworkMode = networkName; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the non-null assertion on HostConfig
Static analysis flags HostConfig! as forbidden. We can avoid it by initialising the object earlier:
- HostConfig: {
- PortBindings: originalContainer.networkSettings?.Ports || {},
- NetworkMode: originalContainer.networkSettings?.Networks?.bridge
- ? 'bridge'
- : Object.keys(originalContainer.networkSettings?.Networks || {})[0] || undefined,
- Binds: originalContainer.mounts?.filter((m) => m.Type === 'bind' || m.Type === 'volume')
- .map((m) => `${m.Source}:${m.Destination}${m.RW ? '' : ':ro'}`)
- },
+ HostConfig: (() => {
+ const hc: Docker.ContainerCreateOptions['HostConfig'] = {
+ PortBindings: originalContainer.networkSettings?.Ports || {},
+ Binds: originalContainer.mounts
+ ?.filter((m) => m.Type === 'bind' || m.Type === 'volume')
+ .map((m) => `${m.Source}:${m.Destination}${m.RW ? '' : ':ro'}`)
+ };
+ hc.NetworkMode =
+ originalContainer.networkSettings?.Networks?.bridge
+ ? 'bridge'
+ : Object.keys(originalContainer.networkSettings?.Networks || {})[0] || undefined;
+ return hc;
+ })(),This removes the ! and keeps the intent clear.
📝 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.
| HostConfig: { | |
| PortBindings: originalContainer.networkSettings?.Ports || {}, | |
| NetworkMode: originalContainer.networkSettings?.Networks?.bridge ? 'bridge' : Object.keys(originalContainer.networkSettings?.Networks || {})[0] || undefined, | |
| Binds: originalContainer.mounts?.filter((mount) => mount.Type === 'bind' || mount.Type === 'volume').map((mount) => `${mount.Source}:${mount.Destination}${mount.RW ? '' : ':ro'}`) | |
| }, | |
| Cmd: originalContainer.config.Cmd, | |
| Entrypoint: originalContainer.config.Entrypoint, | |
| WorkingDir: originalContainer.config.WorkingDir, | |
| User: originalContainer.config.User, | |
| Volumes: originalContainer.config.Volumes, | |
| Tty: originalContainer.config.Tty, | |
| OpenStdin: originalContainer.config.OpenStdin, | |
| StdinOnce: originalContainer.config.StdinOnce | |
| }; | |
| // If we need to add custom network configuration for non-default networks | |
| if (originalContainer.networkSettings?.Networks) { | |
| const networks = Object.entries(originalContainer.networkSettings.Networks); | |
| // If container uses a non-default network, set it up | |
| if (networks.length > 0 && networks[0][0] !== 'bridge') { | |
| const [networkName, networkConfig] = networks[0]; | |
| createOptions.HostConfig!.NetworkMode = networkName; | |
| HostConfig: (() => { | |
| const hc: Docker.ContainerCreateOptions['HostConfig'] = { | |
| PortBindings: originalContainer.networkSettings?.Ports || {}, | |
| Binds: originalContainer.mounts | |
| ?.filter((m) => m.Type === 'bind' || m.Type === 'volume') | |
| .map((m) => `${m.Source}:${m.Destination}${m.RW ? '' : ':ro'}`) | |
| }; | |
| hc.NetworkMode = | |
| originalContainer.networkSettings?.Networks?.bridge | |
| ? 'bridge' | |
| : Object.keys(originalContainer.networkSettings?.Networks || {})[0] || undefined; | |
| return hc; | |
| })(), | |
| Cmd: originalContainer.config.Cmd, | |
| Entrypoint: originalContainer.config.Entrypoint, | |
| WorkingDir: originalContainer.config.WorkingDir, | |
| User: originalContainer.config.User, | |
| Volumes: originalContainer.config.Volumes, | |
| Tty: originalContainer.config.Tty, | |
| OpenStdin: originalContainer.config.OpenStdin, | |
| StdinOnce: originalContainer.config.StdinOnce | |
| }; | |
| // If we need to add custom network configuration for non-default networks | |
| if (originalContainer.networkSettings?.Networks) { | |
| const networks = Object.entries(originalContainer.networkSettings.Networks); | |
| // If container uses a non-default network, set it up | |
| if (networks.length > 0 && networks[0][0] !== 'bridge') { | |
| const [networkName, networkConfig] = networks[0]; | |
| createOptions.HostConfig!.NetworkMode = networkName; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 449-449: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/container-service.ts around lines 428 to 450, remove the non-null assertion operator (!) on HostConfig by initializing the HostConfig object explicitly before it is used. This avoids the forbidden non-null assertion and keeps the intent clear by ensuring HostConfig is always defined when accessed later in the code.
| mounts: inspectData.Mounts, | ||
| labels: inspectData.Config.Labels | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Return type out of sync
getContainer now returns labels (and mounts) but the return type is still implicit any. If callers start using these new props, explicit typing helps:
-export async function getContainer(containerId: string) {
+export async function getContainer(containerId: string): Promise<{
+ id: string;
+ name: string;
+ created: string;
+ path: string;
+ args: string[];
+ state: Docker.ContainerInspectInfo['State'];
+ image: string;
+ config: Docker.ContainerInspectInfo['Config'];
+ networkSettings: Docker.ContainerInspectInfo['NetworkSettings'];
+ mounts: Docker.ContainerInspectInfo['Mounts'];
+ labels: Record<string, string>;
+}> {📝 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.
| mounts: inspectData.Mounts, | |
| labels: inspectData.Config.Labels | |
| }; | |
| export async function getContainer(containerId: string): Promise<{ | |
| id: string; | |
| name: string; | |
| created: string; | |
| path: string; | |
| args: string[]; | |
| state: Docker.ContainerInspectInfo['State']; | |
| image: string; | |
| config: Docker.ContainerInspectInfo['Config']; | |
| networkSettings: Docker.ContainerInspectInfo['NetworkSettings']; | |
| mounts: Docker.ContainerInspectInfo['Mounts']; | |
| labels: Record<string, string>; | |
| }> { | |
| const inspectData = await docker.getContainer(containerId).inspect(); | |
| return { | |
| id: inspectData.Id, | |
| name: inspectData.Name, | |
| created: inspectData.Created, | |
| path: inspectData.Path, | |
| args: inspectData.Args, | |
| state: inspectData.State, | |
| image: inspectData.Image, | |
| config: inspectData.Config, | |
| networkSettings: inspectData.NetworkSettings, | |
| mounts: inspectData.Mounts, | |
| labels: inspectData.Config.Labels | |
| }; | |
| } |
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/container-service.ts around lines 68 to 70, the getContainer function now returns mounts and labels properties, but the return type is implicit any. To fix this, explicitly define the return type of getContainer to include mounts and labels with their correct types, ensuring callers have proper type information for these new properties.
| // Extract image references from compose file (simplified parsing) | ||
| const composeLines = fullStack.composeContent.split('\n'); | ||
| const imageLines = composeLines.filter((line) => line.trim().startsWith('image:') || line.includes(' image:')); | ||
|
|
||
| if (imageLines.length === 0) { | ||
| console.log(`No image references found in stack ${stack.name}.`); | ||
| return false; | ||
| } | ||
|
|
||
| const imageRefs = imageLines | ||
| .map((line) => { | ||
| const imagePart = line.split('image:')[1].trim(); | ||
| // Handle potential comments or extra content on the line | ||
| return imagePart.replace(/['"]/g, '').split(/[\s#]/)[0]; | ||
| }) | ||
| .filter((ref) => ref && (ref.includes(':') || ref.includes('/'))); // Filter out invalid/local refs | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Compose parsing is brittle – consider YAML parsing instead of line heuristics
Scanning lines for image: misses:
• multi-line mappings
• services defined with anchors/aliases
• images wrapped in quotes with leading spaces/tabs
Parsing the compose content once with a YAML parser (yaml, js-yaml) avoids these edge cases and yields a structured object whose services[svc].image fields can be traversed reliably.
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/auto-update-service.ts around lines 205 to 221, the current approach of extracting image references by scanning lines for 'image:' is brittle and misses complex YAML structures like multi-line mappings, anchors, and quoted images with spaces. Replace the line-based heuristic with a proper YAML parser such as 'yaml' or 'js-yaml' to parse the entire compose content into a structured object, then reliably traverse the services to extract the image fields from each service definition.
| // Process eligible containers | ||
| for (const container of eligibleContainers) { | ||
| const containerId = container.id; // Use consistent ID variable | ||
| try { | ||
| // Skip if already being updated | ||
| if (updatingContainers.has(containerId)) { | ||
| console.log(`Auto-update: Skipping ${container.name} (${containerId}), already in progress.`); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential race: add to updatingContainers before performing the expensive checks
A concurrent invocation of checkAndUpdateContainers can evaluate updateAvailable as true for the same container before the current call adds it to the updatingContainers set, leading to duplicate work.
Add the ID to the set immediately after passing the “already updating” guard:
- const updateAvailable = await checkContainerImageUpdate(container);
- if (updateAvailable) {
- updatingContainers.add(containerId);
+ updatingContainers.add(containerId);
+ const updateAvailable = await checkContainerImageUpdate(container);
+ if (updateAvailable) {and ensure you delete it in the finally block to maintain symmetry.
I’ll inspect the surrounding logic to confirm where updatingContainers is mutated and whether there’s a finally cleanup:
🏁 Script executed:
#!/bin/bash
set -e
# Show the block where containers are processed
rg -n "Process eligible containers" -A20 src/lib/services/docker/auto-update-service.ts
# Locate checkContainerImageUpdate calls
rg -n "checkContainerImageUpdate" -C5 src/lib/services/docker/auto-update-service.ts
# Check how and where we remove IDs from updatingContainers
rg -n "updatingContainers.delete" -C5 src/lib/services/docker/auto-update-service.tsLength of output: 2766
Prevent concurrent checks by pre-locking the container ID
To avoid two parallel invocations both calling the expensive checkContainerImageUpdate, acquire the lock immediately after the “already updating” guard and use a try…finally to always release it:
src/lib/services/docker/auto-update-service.ts
@@ 58,14 ↓
// Skip if already being updated
if (updatingContainers.has(containerId)) {
console.log(`Auto-update: Skipping ${container.name} (${containerId}), already in progress.`);
continue;
}
- const updateAvailable = await checkContainerImageUpdate(container);
- if (updateAvailable) {
- updatingContainers.add(containerId);
+ // Pre-lock to prevent duplicate checks
+ updatingContainers.add(containerId);
+ try {
+ const updateAvailable = await checkContainerImageUpdate(container);
+ if (!updateAvailable) {
+ console.log(`Auto-update: Container ${container.name} (${containerId}) is up-to-date.`);
+ return;
+ }
console.log(`Auto-update: Update found for container ${container.name} (${containerId}). Recreating...`);
await pullImage(container.image);
await recreateContainer(containerId);
- console.log(`Auto-update: Container ${container.name} recreated successfully`);
- updatingContainers.delete(containerId);
+ console.log(`Auto-update: Container ${container.name} recreated successfully`);
+ } finally {
+ updatingContainers.delete(containerId);
}This guarantees that once a container enters the update flow, no other invocation can re-check it, and the lock is always cleaned up—even on errors.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/auto-update-service.ts around lines 55 to 64, to prevent concurrent invocations from processing the same container simultaneously, add the container ID to the updatingContainers set immediately after the check that skips already updating containers. Then wrap the update logic in a try...finally block and ensure the container ID is removed from updatingContainers in the finally block to guarantee cleanup even if errors occur. This change prevents duplicate work by locking the container early in the update flow.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (9)
src/hooks.server.ts (1)
5-10: Great implementation of the error handling pattern.This is an excellent improvement over the previous implementation. The code now properly:
- Awaits concurrent initialization of both services
- Exits the application if either critical service fails to initialize
- Provides clear error logging
This matches the suggestion from the previous review and ensures the application doesn't run in a degraded state.
src/routes/api/system/auto-update/+server.ts (1)
27-46:⚠️ Potential issueManual update checks don't work when auto-update is globally disabled.
The current implementation skips manual checks if auto-update is globally disabled, defeating the purpose of a manual check.
Modify the implementation to allow manual checks regardless of the global setting:
export const POST: RequestHandler = async () => { try { - const [containerResults, stackResults] = await Promise.all([checkAndUpdateContainers(), checkAndUpdateStacks()]); + // Pass force=true to run updates even if global setting is disabled + const [containerResults, stackResults] = await Promise.all([ + checkAndUpdateContainers({ force: true }), + checkAndUpdateStacks({ force: true }) + ]); return json({ success: true, containers: containerResults, stacks: stackResults });You'll also need to modify the service functions to accept and use this parameter:
// In auto-update-service.ts: - export async function checkAndUpdateContainers(): Promise<{ + export async function checkAndUpdateContainers(options?: { force?: boolean }): Promise<{ checked: number; updated: number; errors: Array<{ id: string; error: string }>; }> { const settings = await getSettings(); // Skip if global auto-update is disabled - if (!settings.autoUpdate) { + if (!settings.autoUpdate && !options?.force) { return { checked: 0, updated: 0, errors: [] }; }And similarly for
checkAndUpdateStacks.src/lib/services/docker/stack-service.ts (1)
912-934: Meta file re-read duplicates work done byloadComposeStacks
This exact I/O duplication was already flagged in a previous review.Rather than re-reading every
meta.json, enhanceloadComposeStacks()to include themetaobject (it already reads the file) and directly return those enriched stacks here.Ref-example:
- const managedStacks = await loadComposeStacks(); - const enrichedManagedStacks = await Promise.all( - managedStacks.map(async (stack) => { ...read meta again... }) - ); + const enrichedManagedStacks = await loadComposeStacks(); // meta already presentThis removes two file-system calls per stack and simplifies the function.
src/lib/services/docker/container-service.ts (3)
68-71: Public return type now diverges – add an explicit interface
getContainer()gainedmounts&labelsbut still returnsany.
An explicit signature prevents silent breakage when callers access these new props.
(The same concern was raised earlier for this function.)
448-451: Avoid the non-null assertion – initialiseHostConfigfirst
createOptions.HostConfig!.NetworkMode = …triggers Biome’snoNonNullAssertionrule and can crash ifHostConfigis later unset.- createOptions.HostConfig!.NetworkMode = networkName; +createOptions.HostConfig = { ...(createOptions.HostConfig ?? {}), NetworkMode: networkName };🧰 Tools
🪛 Biome (1.9.4)
[error] 449-449: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
492-499: 🛠️ Refactor suggestionRace: new container may not appear in
listContainers()immediatelyImmediately after
startContainer, Docker may still be populating its container list.
Instead of a freshlistContainerscall, inspect the newly created container directly:- const containers = await listContainers(true); - const newServiceContainer = containers.find((c) => c.id === newContainer.id); +const inspected = await newContainer.inspect(); +const newServiceContainer: ServiceContainer = { + id: inspected.Id, + names: [inspected.Name.substring(1)], + name: inspected.Name.substring(1), + image: inspected.Config.Image, + imageId: inspected.Image, + command: inspected.Path + ' ' + inspected.Args.join(' '), + created: inspected.Created, + state: inspected.State.Status, + status: inspected.State.Running ? 'running' : 'stopped', + ports: inspected.NetworkSettings.Ports +};This removes the timing window and an extra Docker API round-trip.
src/lib/services/docker/auto-update-service.ts (3)
56-68: 🛠️ Refactor suggestionLock container earlier to avoid race conditions (re-iterating previous feedback)
updatingContainers.add(containerId)is executed after the expensivecheckContainerImageUpdatecall, leaving a window where two scheduler ticks can run the same network-heavy check and even both proceed to recreating the container.
Add the ID to the set immediately after passing the “already updating” guard, and release it in afinallyblock to guarantee cleanup:- const updateAvailable = await checkContainerImageUpdate(container); - if (updateAvailable) { - updatingContainers.add(containerId); + updatingContainers.add(containerId); // pre-lock + try { + const updateAvailable = await checkContainerImageUpdate(container); + if (!updateAvailable) { + console.log(`Auto-update: Container ${container.name} (${containerId}) is up-to-date.`); + continue; + }…and
finally { updatingContainers.delete(containerId); }.
70-74: 🧹 Nitpick (assertive)Redundant second
pullImage(previously pointed out)
checkContainerImageUpdatealready executedpullImage(imageRef).
Pulling again doubles registry traffic and slows the update loop.- console.log(`Auto-update: Pulling latest image ${container.image} for ${container.name}...`); - await pullImage(container.image); + // Latest image is already pulled inside checkContainerImageUpdate; skip duplicate pull.
206-223: 🛠️ Refactor suggestionLine-oriented compose parsing remains brittle (see prior review)
Scanning for
'image:'strings still misses multi-line mappings, YAML anchors, quoted values, etc.
Parsing once with a YAML library (yaml,js-yaml) gives you a structured object (services[svc].image) and eliminates these edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
src/hooks.server.ts(1 hunks)src/lib/services/docker/auto-update-service.ts(1 hunks)src/lib/services/docker/container-service.ts(2 hunks)src/lib/services/docker/scheduler-service.ts(1 hunks)src/lib/services/docker/stack-service.ts(3 hunks)src/lib/services/settings-service.ts(1 hunks)src/lib/types/docker/stack.type.ts(2 hunks)src/lib/types/settings.ts(1 hunks)src/routes/api/stacks/[stackId]/+server.ts(1 hunks)src/routes/api/system/auto-update/+server.ts(1 hunks)src/routes/containers/[id]/+page.svelte(5 hunks)src/routes/containers/create-container-dialog.svelte(4 hunks)src/routes/settings/+page.server.ts(2 hunks)src/routes/settings/tabs/app-settings.svelte(2 hunks)src/routes/stacks/[stackId]/+page.server.ts(3 hunks)src/routes/stacks/[stackId]/+page.svelte(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/lib/services/docker/scheduler-service.ts (2)
src/lib/services/settings-service.ts (1)
getSettings(129-182)src/lib/services/docker/auto-update-service.ts (2)
checkAndUpdateContainers(15-96)checkAndUpdateStacks(101-155)
src/lib/services/docker/stack-service.ts (1)
src/lib/types/docker/stack.type.ts (2)
Stack(19-32)StackMeta(1-6)
src/routes/api/system/auto-update/+server.ts (2)
src/lib/services/settings-service.ts (1)
getSettings(129-182)src/lib/services/docker/auto-update-service.ts (2)
checkAndUpdateContainers(15-96)checkAndUpdateStacks(101-155)
src/routes/stacks/[stackId]/+page.server.ts (1)
src/lib/services/docker/stack-service.ts (1)
updateStack(449-499)
🪛 Biome (1.9.4)
src/lib/services/docker/container-service.ts
[error] 449-449: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (29)
src/lib/types/settings.ts (1)
4-4: LGTM! The type definition is properly extended.The addition of
autoUpdateIntervalas a numeric property is clean and aligned with the auto-update feature being implemented.src/hooks.server.ts (1)
2-2: LGTM! Good import for the new service.The import of the auto-update scheduler is properly added.
src/routes/containers/create-container-dialog.svelte (3)
16-16: LGTM! Proper import for the UI component.The import of the Switch component is correctly added.
90-92: LGTM! Good state initialization.The auto-update boolean state is properly initialized with a default value of false.
225-229: LGTM! Well-implemented label integration.The code correctly adds the 'arcane.auto-update' label to the container configuration when the auto-update feature is enabled.
src/routes/settings/+page.server.ts (2)
29-32: Well implemented validation for autoUpdateInterval.The code correctly parses the autoUpdateInterval from form data, applies proper validation by clamping it between 5 and 1440 minutes. This is consistent with the frontend constraints and provides a good user experience by enforcing reasonable limits.
97-97: LGTM! Proper integration of the new setting.The validated autoUpdateInterval is correctly included in the updated settings object, making it available for the scheduler service to use.
src/lib/types/docker/stack.type.ts (3)
5-5: Good addition of the autoUpdate flag.The optional autoUpdate boolean property in the StackMeta interface is appropriate for indicating whether a stack should be automatically updated.
31-31: LGTM! Well-integrated metadata field.Making the meta field optional ensures backward compatibility with existing stacks that might not have metadata.
37-37: Proper extension of the StackUpdate interface.Adding the optional autoUpdate field to StackUpdate enables the API to update this setting while maintaining backward compatibility.
src/routes/settings/tabs/app-settings.svelte (2)
20-20: LGTM! Consistent reactive state management.The derived variable for autoUpdateInterval follows the same pattern as other settings and correctly handles form values and fallbacks.
114-120: Well-designed UI for auto-update interval.The auto-update interval UI is appropriately shown only when auto-update is enabled. The input field has proper constraints matching the server-side validation (5-1440 minutes) and includes helpful explanatory text.
src/routes/stacks/[stackId]/+page.svelte (6)
18-18: LGTM! Required import for the new UI component.The Switch component import is needed for the auto-update toggle UI.
33-33: LGTM! Proper state tracking for original value.Adding the derived variable for originalAutoUpdate follows the existing pattern and enables change detection.
35-35: LGTM! Comprehensive change detection.The hasChanges derived state now correctly includes checking for changes to the autoUpdate field, which ensures the save button is enabled appropriately.
57-61: LGTM! Complete API payload.The PATCH request body now properly includes the autoUpdate flag along with other stack properties.
75-75: LGTM! Proper state reset after save.After a successful save, the originalAutoUpdate value is properly updated to reset the change detection.
216-222: Well-designed UI for stack auto-update toggle.The auto-update toggle UI is cleanly integrated with the existing form, with a clear label and helpful explanatory text that explains the feature to users. The binding to editorState.autoUpdate ensures the value is properly tracked.
src/routes/api/stacks/[stackId]/+server.ts (2)
10-11: LGTM! Properly handles the new autoUpdate flag.The update function now correctly includes the autoUpdate parameter, and the response is appropriately simplified.
13-14: Error handling has been simplified and improved.The consolidated error handling is cleaner than handling specific error types separately, and it properly logs the error while returning a user-friendly message.
src/routes/stacks/[stackId]/+page.server.ts (4)
20-21: LGTM! Good addition of autoUpdate to editor state.The editor state now properly includes the autoUpdate flag from stack metadata with a sensible false default.
34-35: LGTM! Consistent application of autoUpdate in error state.The error fallback state correctly includes the autoUpdate property with the same false default.
49-49: LGTM! Proper extraction of autoUpdate from form data.The code correctly extracts and converts the autoUpdate checkbox value to a boolean.
52-52: LGTM! autoUpdate is now passed to updateStack.The autoUpdate flag is properly included in the stack update parameters, which aligns with the changes in the stack service.
src/lib/services/docker/scheduler-service.ts (4)
1-6: LGTM! Proper imports and state tracking.The imports correctly reference the necessary services, and the timer is properly tracked with a module-level variable.
10-35: LGTM! Well-structured scheduler initialization.The scheduler initialization is well-structured, with proper cleanup of existing timers, settings retrieval, and interval calculation. The code handles the disabled state correctly and logs informative messages.
40-54: LGTM! Comprehensive update check implementation.The update check function properly calls both container and stack update functions, logs detailed results, and handles errors gracefully.
59-65: LGTM! Clean scheduler shutdown.The function properly cleans up by clearing the interval and resetting the timer reference.
src/routes/api/system/auto-update/+server.ts (1)
6-25: LGTM! Clean GET endpoint with meaningful responses.The GET handler properly retrieves settings and returns the auto-update status with clear, user-friendly messages. Error handling is comprehensive.
| export const DEFAULT_SETTINGS: SettingsData = { | ||
| dockerHost: isDev ? (process.platform === 'win32' ? 'npipe:////./pipe/docker_engine' : 'unix:///var/run/docker.sock') : 'unix:///var/run/docker.sock', | ||
| autoUpdate: false, | ||
| autoUpdateInterval: 5, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a comment to clarify the time unit.
The default value of 5 for autoUpdateInterval is reasonable, but it would be helpful to add a comment explicitly mentioning that this is in minutes, especially for future developers.
- autoUpdateInterval: 5,
+ autoUpdateInterval: 5, // in minutes - determines how often container/stack updates are checked📝 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.
| autoUpdateInterval: 5, | |
| autoUpdateInterval: 5, // in minutes - determines how often container/stack updates are checked |
🤖 Prompt for AI Agents (early access)
In src/lib/services/settings-service.ts at line 19, add a comment next to the autoUpdateInterval property to clarify that the value 5 represents minutes. This will help future developers understand the time unit without ambiguity. Keep the comment concise and directly related to the time unit.
| <!-- Auto-update --> | ||
| <div class="flex items-center space-x-2 py-4 border-t"> | ||
| <Switch id="auto-update" name="autoUpdate" bind:checked={autoUpdate} /> | ||
| <Label for="auto-update" class="font-medium">Enable auto-update</Label> | ||
| <p class="text-xs text-muted-foreground">When enabled, Arcane will periodically check for newer versions of this container's image and automatically update it.</p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a warning for 'latest' tag images.
The auto-update UI is well implemented, but it might be helpful to warn users or provide guidance when they're enabling auto-update for containers with the 'latest' tag since these may not behave as expected with auto-updates.
<div class="flex items-center space-x-2 py-4 border-t">
<Switch id="auto-update" name="autoUpdate" bind:checked={autoUpdate} />
<Label for="auto-update" class="font-medium">Enable auto-update</Label>
- <p class="text-xs text-muted-foreground">When enabled, Arcane will periodically check for newer versions of this container's image and automatically update it.</p>
+ <p class="text-xs text-muted-foreground">When enabled, Arcane will periodically check for newer versions of this container's image and automatically update it.
+ {#if selectedImage.endsWith(':latest')}
+ <span class="text-warning-500">Note: This image uses the 'latest' tag, which may not trigger updates as expected.</span>
+ {/if}
+ </p>
</div>📝 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.
| <!-- Auto-update --> | |
| <div class="flex items-center space-x-2 py-4 border-t"> | |
| <Switch id="auto-update" name="autoUpdate" bind:checked={autoUpdate} /> | |
| <Label for="auto-update" class="font-medium">Enable auto-update</Label> | |
| <p class="text-xs text-muted-foreground">When enabled, Arcane will periodically check for newer versions of this container's image and automatically update it.</p> | |
| </div> | |
| </div> | |
| <!-- Auto-update --> | |
| <div class="flex items-center space-x-2 py-4 border-t"> | |
| <Switch id="auto-update" name="autoUpdate" bind:checked={autoUpdate} /> | |
| <Label for="auto-update" class="font-medium">Enable auto-update</Label> | |
| <p class="text-xs text-muted-foreground"> | |
| When enabled, Arcane will periodically check for newer versions of this container's image and automatically update it. | |
| {#if selectedImage.endsWith(':latest')} | |
| <span class="text-warning-500"> | |
| Note: This image uses the 'latest' tag, which may not trigger updates as expected. | |
| </span> | |
| {/if} | |
| </p> | |
| </div> |
🤖 Prompt for AI Agents (early access)
In src/routes/containers/create-container-dialog.svelte around lines 618 to 624, add a warning message or guidance below the auto-update switch that appears only when the container image tag is 'latest'. This warning should inform users that enabling auto-update on 'latest' tag images may lead to unexpected behavior. Implement this by conditionally rendering a small, styled text element that clearly communicates this caution to the user.
| const { stackId } = params; | ||
| const { name, composeContent, autoUpdate } = await request.json(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The destructured request properties lack validation.
The code extracts name, composeContent, and autoUpdate from the request JSON, but there's no validation for these values. The autoUpdate property should specifically be validated as a boolean.
- const { name, composeContent, autoUpdate } = await request.json();
+ const data = await request.json();
+ const name = data.name;
+ const composeContent = data.composeContent;
+ const autoUpdate = typeof data.autoUpdate === 'boolean' ? data.autoUpdate : false;📝 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 { stackId } = params; | |
| const { name, composeContent, autoUpdate } = await request.json(); | |
| const { stackId } = params; | |
| const data = await request.json(); | |
| const name = data.name; | |
| const composeContent = data.composeContent; | |
| const autoUpdate = typeof data.autoUpdate === 'boolean' ? data.autoUpdate : false; |
🤖 Prompt for AI Agents (early access)
In src/routes/api/stacks/[stackId]/+server.ts around lines 6 to 7, the destructured properties from the request JSON lack validation, especially the autoUpdate property which should be a boolean. Add validation logic after extracting name, composeContent, and autoUpdate to check that autoUpdate is indeed a boolean and that name and composeContent meet expected criteria (e.g., non-empty strings). If validation fails, return an appropriate error response to prevent processing invalid input.
| const intervalMinutes = settings.autoUpdateInterval || 60; // Default to 60 minutes | ||
| const intervalMs = intervalMinutes * 60 * 1000; | ||
|
|
||
| console.log(`Starting auto-update scheduler with interval of ${intervalMinutes} minutes`); | ||
|
|
||
| // Initial run | ||
| await runAutoUpdateChecks(); | ||
|
|
||
| // Schedule regular checks | ||
| autoUpdateTimer = setInterval(runAutoUpdateChecks, intervalMs); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider handling very short intervals and concurrent runs.
The scheduler doesn't account for very short intervals or long-running update checks that might overlap. Consider adding minimum interval validation and a mechanism to prevent concurrent runs.
const intervalMinutes = settings.autoUpdateInterval || 60; // Default to 60 minutes
+ // Ensure minimum interval is at least 5 minutes to prevent excessive updates
+ const safeIntervalMinutes = Math.max(5, intervalMinutes);
- const intervalMs = intervalMinutes * 60 * 1000;
+ const intervalMs = safeIntervalMinutes * 60 * 1000;
- console.log(`Starting auto-update scheduler with interval of ${intervalMinutes} minutes`);
+ console.log(`Starting auto-update scheduler with interval of ${safeIntervalMinutes} minutes`);
// Initial run
- await runAutoUpdateChecks();
+ // Use setTimeout for initial run to avoid blocking initialization
+ setTimeout(runAutoUpdateChecks, 1000);And in the runAutoUpdateChecks function:
+ // Flag to prevent concurrent runs
+ let isRunning = false;
+
async function runAutoUpdateChecks(): Promise<void> {
+ // Skip if already running
+ if (isRunning) {
+ console.log('Auto-update already in progress, skipping this run');
+ return;
+ }
+
+ isRunning = true;
console.log('Running scheduled auto-update checks...');
try {
// Check containers first
const containerResults = await checkAndUpdateContainers();
console.log(`Auto-update check completed for containers: Checked ${containerResults.checked}, Updated ${containerResults.updated}, Errors ${containerResults.errors.length}`);
// Then check stacks
const stackResults = await checkAndUpdateStacks();
console.log(`Auto-update check completed for stacks: Checked ${stackResults.checked}, Updated ${stackResults.updated}, Errors ${stackResults.errors.length}`);
} catch (error) {
console.error('Error during auto-update check:', error);
+ } finally {
+ isRunning = false;
}
}🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/scheduler-service.ts around lines 25 to 35, the scheduler lacks validation for very short intervals and does not prevent overlapping runs of runAutoUpdateChecks. Add a minimum interval check to ensure intervalMinutes is not below a safe threshold (e.g., 1 minute). Implement a concurrency guard, such as a boolean flag, to skip starting a new runAutoUpdateChecks execution if the previous one is still running, preventing concurrent executions.
| import type { RequestHandler } from './$types'; | ||
| import { json } from '@sveltejs/kit'; | ||
| import { getSettings } from '$lib/services/settings-service'; | ||
| import { checkAndUpdateContainers, checkAndUpdateStacks } from '$lib/services/docker/auto-update-service'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding API authentication/authorization.
These endpoints can trigger potentially impactful operations across your Docker infrastructure. Consider adding authentication or authorization checks.
For example, you could add a middleware or check session tokens:
+ import { validate_auth } from '$lib/server/auth';
export const GET: RequestHandler = async ({ request }) => {
+ // Validate user is authenticated and authorized
+ const auth = await validate_auth(request);
+ if (!auth.valid) {
+ return json({ error: 'Unauthorized' }, { status: 401 });
+ }
try {
const settings = await getSettings();📝 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 type { RequestHandler } from './$types'; | |
| import { json } from '@sveltejs/kit'; | |
| import { getSettings } from '$lib/services/settings-service'; | |
| import { checkAndUpdateContainers, checkAndUpdateStacks } from '$lib/services/docker/auto-update-service'; | |
| import type { RequestHandler } from './$types'; | |
| import { json } from '@sveltejs/kit'; | |
| import { getSettings } from '$lib/services/settings-service'; | |
| import { checkAndUpdateContainers, checkAndUpdateStacks } from '$lib/services/docker/auto-update-service'; | |
| + import { validate_auth } from '$lib/server/auth'; | |
| export const GET: RequestHandler = async ({ request }) => { | |
| + // Validate user is authenticated and authorized | |
| + const auth = await validate_auth(request); | |
| + if (!auth.valid) { | |
| + return json({ error: 'Unauthorized' }, { status: 401 }); | |
| + } | |
| try { | |
| const settings = await getSettings(); | |
| const containersResult = await checkAndUpdateContainers(settings); | |
| const stacksResult = await checkAndUpdateStacks(settings); | |
| return json({ | |
| containers: containersResult, | |
| stacks: stacksResult | |
| }); | |
| } catch (error) { | |
| return json({ error: String(error) }, { status: 500 }); | |
| } | |
| }; |
🤖 Prompt for AI Agents (early access)
In src/routes/api/system/auto-update/+server.ts around lines 1 to 5, the API endpoints lack authentication or authorization, which is critical since they perform impactful Docker operations. Implement an authentication mechanism by adding middleware or session token checks at the start of the request handler to verify the user's identity and permissions before proceeding with any updates. Ensure unauthorized requests are rejected with appropriate HTTP status codes.
| <h4 class="text-sm font-medium mb-2 flex items-center gap-2"> | ||
| <Network class="h-4 w-4 text-muted-foreground" /> Network I/O | ||
| </h4> | ||
| <div class="grid grid-cols-2 gap-4 bg-muted/30 p-3 rounded-md"> | ||
| <div> | ||
| <div class="text-xs text-muted-foreground">Received</div> | ||
| <div class="text-sm font-medium mt-1">{formatBytes(stats.networks?.eth0?.rx_bytes || 0)}</div> | ||
| </div> | ||
| <div> | ||
| <div class="text-xs text-muted-foreground">Transmitted</div> | ||
| <div class="text-sm font-medium mt-1">{formatBytes(stats.networks?.eth0?.tx_bytes || 0)}</div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Interface name hard-coded to eth0 – aggregate all networks
On Mac / Windows or custom bridges the primary interface might be en0, br0, etc. Summing all interfaces avoids empty metrics:
{:else}
<div class="grid grid-cols-2 gap-4 bg-muted/30 p-3 rounded-md">
<div>
<div class="text-xs text-muted-foreground">Received</div>
<div class="text-sm font-medium mt-1">
{formatBytes(Object.values(stats.networks ?? {}).reduce((a, n) => a + (n.rx_bytes || 0), 0))}
</div>
</div>
<div>
<div class="text-xs text-muted-foreground">Transmitted</div>
<div class="text-sm font-medium mt-1">
{formatBytes(Object.values(stats.networks ?? {}).reduce((a, n) => a + (n.tx_bytes || 0), 0))}
</div>
</div>
</div>🤖 Prompt for AI Agents (early access)
In src/routes/containers/[id]/+page.svelte around lines 683 to 695, the network interface name is hard-coded to 'eth0', which may cause missing data on systems using different interface names. Replace the direct access to stats.networks.eth0.rx_bytes and tx_bytes with a sum over all network interfaces by using Object.values(stats.networks ?? {}) and reducing to accumulate rx_bytes and tx_bytes respectively. This will aggregate the metrics across all interfaces and avoid empty or incorrect values.
| <!-- Memory Usage --> | ||
| <div> | ||
| <div class="flex justify-between items-center mb-2"> | ||
| <span class="text-sm font-medium flex items-center gap-2"><MemoryStick class="h-4 w-4 text-muted-foreground" /> Memory Usage</span> | ||
| <span class="text-sm font-semibold">{memoryUsageFormatted} / {memoryLimitFormatted}</span> | ||
| </div> | ||
| <div class="w-full bg-secondary rounded-full h-3 overflow-hidden"> | ||
| <div class="bg-primary h-3 rounded-full transition-all duration-300" style="width: {memoryUsagePercent.toFixed(2)}%"></div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clamp displayed memory-usage percentage
memoryUsagePercent can exceed 100 % on kernel-overcommit or cgroup quirks, yet the text is shown un-clamped (only the bar is). Presenting >100 % is confusing.
- <span class="text-sm font-semibold">{memoryUsageFormatted} / {memoryLimitFormatted}</span>
+<span class="text-sm font-semibold">
+ {memoryUsageFormatted} / {memoryLimitFormatted}
+ ({Math.min(memoryUsagePercent, 100).toFixed(2)}%)
+</span>📝 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.
| <!-- Memory Usage --> | |
| <div> | |
| <div class="flex justify-between items-center mb-2"> | |
| <span class="text-sm font-medium flex items-center gap-2"><MemoryStick class="h-4 w-4 text-muted-foreground" /> Memory Usage</span> | |
| <span class="text-sm font-semibold">{memoryUsageFormatted} / {memoryLimitFormatted}</span> | |
| </div> | |
| <div class="w-full bg-secondary rounded-full h-3 overflow-hidden"> | |
| <div class="bg-primary h-3 rounded-full transition-all duration-300" style="width: {memoryUsagePercent.toFixed(2)}%"></div> | |
| </div> | |
| <!-- Memory Usage --> | |
| <div> | |
| <div class="flex justify-between items-center mb-2"> | |
| <span class="text-sm font-medium flex items-center gap-2"> | |
| <MemoryStick class="h-4 w-4 text-muted-foreground" /> Memory Usage | |
| </span> | |
| <span class="text-sm font-semibold"> | |
| {memoryUsageFormatted} / {memoryLimitFormatted} | |
| ({Math.min(memoryUsagePercent, 100).toFixed(2)}%) | |
| </span> | |
| </div> | |
| <div class="w-full bg-secondary rounded-full h-3 overflow-hidden"> | |
| <div | |
| class="bg-primary h-3 rounded-full transition-all duration-300" | |
| style="width: {memoryUsagePercent.toFixed(2)}%" | |
| ></div> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents (early access)
In src/routes/containers/[id]/+page.svelte around lines 670 to 678, the displayed memory usage percentage text shows values above 100%, which can confuse users due to kernel overcommit or cgroup quirks. To fix this, clamp the displayed memoryUsagePercent value to a maximum of 100 before rendering it in the text span, while keeping the bar width styling unchanged.
| // Process eligible stacks | ||
| for (const stack of eligibleStacks) { | ||
| try { | ||
| // Skip if already being updated | ||
| if (updatingStacks.has(stack.id)) continue; | ||
|
|
||
| const updateAvailable = await checkStackImagesUpdate(stack); | ||
| if (updateAvailable) { | ||
| updatingStacks.add(stack.id); | ||
|
|
||
| console.log(`Auto-update: Redeploying stack ${stack.name} (${stack.id})`); | ||
| await fullyRedeployStack(stack.id); | ||
|
|
||
| console.log(`Auto-update: Stack ${stack.name} redeployed successfully`); | ||
| results.updated++; | ||
|
|
||
| updatingStacks.delete(stack.id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pre-lock stacks to prevent duplicate redeploys
Similar to containers, updatingStacks.add(stack.id) is executed only after the costly checkStackImagesUpdate.
Move the lock acquisition just after the “already updating” guard and wrap the block in try…finally so concurrent scheduler ticks cannot redeploy the same stack twice.
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/auto-update-service.ts around lines 126 to 143, the updatingStacks lock is added only after the costly checkStackImagesUpdate call, which can cause duplicate redeploys in concurrent scheduler ticks. Move updatingStacks.add(stack.id) immediately after the if (updatingStacks.has(stack.id)) continue guard, then wrap the redeploy logic in a try...finally block to ensure updatingStacks.delete(stack.id) is always called, preventing concurrent redeploys of the same stack.
| const imageRefs = imageLines | ||
| .map((line) => { | ||
| const imagePart = line.split('image:')[1].trim(); | ||
| // Handle potential comments or extra content on the line | ||
| return imagePart.replace(/['"]/g, '').split(/[\s#]/)[0]; | ||
| }) | ||
| .filter((ref) => ref && (ref.includes(':') || ref.includes('/'))); // Filter out invalid/local refs | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Skip digest-only image references
Digest-only refs (sha256:<hash>) cannot be updated by tag; pulling them always fails.
Reuse the regex you added for containers to skip such lines early:
if (/^sha256:[A-Fa-f0-9]{64}$/.test(imageRef)) {
continue; // digest-only, nothing to update
}This avoids noisy errors and wasted pulls.
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/auto-update-service.ts around lines 215 to 222, the code extracts image references but does not skip digest-only references like "sha256:<hash>", which cause pull failures. Add a check using the regex /^sha256:[A-Fa-f0-9]{64}$/ to detect digest-only refs and skip processing them early in the map or filter step to avoid attempting to update these immutable images.
| // Current image digest | ||
| const currentImage = await getImage(container.imageId); | ||
| if (!currentImage) return false; | ||
|
|
||
| // Pull the image to check for updates (but don't use it yet) | ||
| await pullImage(imageRef); | ||
|
|
||
| // Get the fresh image info | ||
| const lastColon = imageRef.lastIndexOf(':'); | ||
| const imageName = lastColon === -1 ? imageRef : imageRef.slice(0, lastColon); | ||
| const tag = lastColon === -1 ? 'latest' : imageRef.slice(lastColon + 1); | ||
| const freshImages = await listImages(); | ||
| const freshImage = freshImages.find((img) => (img.repo === imageName || img.repo.endsWith(`/${imageName}`)) && img.tag === tag); | ||
|
|
||
| if (!freshImage) return false; | ||
|
|
||
| // Compare image IDs - if different, update is available | ||
| return freshImage.id !== container.imageId; |
There was a problem hiding this comment.
Incorrect property casing causes perpetual “update found”
Dockerode’s image objects expose the identifier as Id (capital “I”), but the code compares freshImage.id (lower-case).
Because freshImage.id is undefined, the expression undefined !== container.imageId is always true, so every container is considered outdated in every run.
- return freshImage.id !== container.imageId;
+ return freshImage.Id !== container.imageId;Please audit all image-ID accesses (id, Id, imageId) for consistent casing.
📝 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.
| // Current image digest | |
| const currentImage = await getImage(container.imageId); | |
| if (!currentImage) return false; | |
| // Pull the image to check for updates (but don't use it yet) | |
| await pullImage(imageRef); | |
| // Get the fresh image info | |
| const lastColon = imageRef.lastIndexOf(':'); | |
| const imageName = lastColon === -1 ? imageRef : imageRef.slice(0, lastColon); | |
| const tag = lastColon === -1 ? 'latest' : imageRef.slice(lastColon + 1); | |
| const freshImages = await listImages(); | |
| const freshImage = freshImages.find((img) => (img.repo === imageName || img.repo.endsWith(`/${imageName}`)) && img.tag === tag); | |
| if (!freshImage) return false; | |
| // Compare image IDs - if different, update is available | |
| return freshImage.id !== container.imageId; | |
| // Current image digest | |
| const currentImage = await getImage(container.imageId); | |
| if (!currentImage) return false; | |
| // Pull the image to check for updates (but don't use it yet) | |
| await pullImage(imageRef); | |
| // Get the fresh image info | |
| const lastColon = imageRef.lastIndexOf(':'); | |
| const imageName = lastColon === -1 ? imageRef : imageRef.slice(0, lastColon); | |
| const tag = lastColon === -1 ? 'latest' : imageRef.slice(lastColon + 1); | |
| const freshImages = await listImages(); | |
| const freshImage = freshImages.find((img) => (img.repo === imageName || img.repo.endsWith(`/${imageName}`)) && img.tag === tag); | |
| if (!freshImage) return false; | |
| // Compare image IDs - if different, update is available | |
| return freshImage.Id !== container.imageId; |
🤖 Prompt for AI Agents (early access)
In src/lib/services/docker/auto-update-service.ts between lines 168 and 185, the code incorrectly accesses the Docker image ID property as freshImage.id instead of freshImage.Id, causing the update check to always return true. To fix this, replace all instances of freshImage.id with freshImage.Id to match Dockerode's property casing. Also, review all image ID references in this block to ensure consistent use of Id with a capital "I" to prevent similar issues.
1 participant
Summary by CodeRabbit