Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce support for naming Docker stack directories after their stack names instead of non-human-readable IDs. This includes migration utilities for legacy stacks, updates to stack creation and metadata handling, API endpoints and UI components for migration, and utility functions for filesystem checks. Type definitions are updated accordingly. Changes
Assessment against linked issues
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| const metaPath = path.join(oldDirPath, 'meta.json'); | ||
| const newMetaPath = path.join(oldDirPath, '.stack.json'); | ||
| const oldComposePath = path.join(oldDirPath, 'docker-compose.yml'); |
Check failure
Code scanning / ESLint
Disallow unused variables Error
| const metaPath = path.join(oldDirPath, 'meta.json'); | ||
| const newMetaPath = path.join(oldDirPath, '.stack.json'); | ||
| const oldComposePath = path.join(oldDirPath, 'docker-compose.yml'); | ||
| const newComposePath = path.join(oldDirPath, 'compose.yaml'); |
Check failure
Code scanning / ESLint
Disallow unused variables Error
| try { | ||
| await fs.access(newMetaPath); | ||
| continue; // Already migrated | ||
| } catch {} |
Check failure
Code scanning / ESLint
Disallow empty block statements Error
|
|
||
| const metaPath = path.join(oldDirPath, 'meta.json'); | ||
| const newMetaPath = path.join(oldDirPath, '.stack.json'); | ||
| const oldComposePath = path.join(oldDirPath, 'docker-compose.yml'); |
Check failure
Code scanning / ESLint
Disallow unused variables Error
| const metaPath = path.join(oldDirPath, 'meta.json'); | ||
| const newMetaPath = path.join(oldDirPath, '.stack.json'); | ||
| const oldComposePath = path.join(oldDirPath, 'docker-compose.yml'); | ||
| const newComposePath = path.join(oldDirPath, 'compose.yaml'); |
Check failure
Code scanning / ESLint
Disallow unused variables Error
| console.error(`Failed to cleanup partially created stack directory ${stackDir}:`, cleanupErr); | ||
| let serviceCount = 0; | ||
| try { | ||
| const composeData = yaml.load(composeContent) as any; |
Check failure
Code scanning / ESLint
Disallow the `any` type Error
| try { | ||
| [metaContent, composeContent, envContent] = await Promise.all([fs.readFile(newMetaPath, 'utf8'), fs.readFile(newComposePath, 'utf8'), loadEnvFile(stackId)]); | ||
| meta = JSON.parse(metaContent) as StackMeta; | ||
| } catch (newWayErr) { |
Check failure
Code scanning / ESLint
Disallow unused variables Error
| try { | ||
| const stats = await fs.stat(filePath); | ||
| return stats.isFile(); | ||
| } catch (err) { |
Check failure
Code scanning / ESLint
Disallow unused variables Error
| try { | ||
| const stats = await fs.stat(dir); | ||
| return stats.isDirectory(); | ||
| } catch (err) { |
Check failure
Code scanning / ESLint
Disallow unused variables Error
| try { | ||
| await migrateStackToNameFolder(stackId); | ||
| return json({ success: true, message: `Stack "${stackId}" migrated successfully.` }); | ||
| } catch (err: any) { |
Check failure
Code scanning / ESLint
Disallow the `any` type Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/lib/utils/fs.utils.ts (2)
6-13: Consider addressing the unused error variableThe implementation of
fileExistsis clean and focused, but the ESLint warning indicates that theerrvariable in the catch block is unused. Either use it or remove it to address the linting issue.- try { - const stats = await fs.stat(filePath); - return stats.isFile(); - } catch (err) { - return false; - } + try { + const stats = await fs.stat(filePath); + return stats.isFile(); + } catch (_) { + return false; + }🧰 Tools
🪛 GitHub Check: ESLint
[failure] 10-10: Disallow unused variables
'err' is defined but never used.
15-22: Fix unused error variable in catch blockSimilarly, the
errvariable in thedirectoryExistsfunction is unused. Consider replacing it with an underscore to indicate it's intentionally ignored.- try { - const stats = await fs.stat(dir); - return stats.isDirectory(); - } catch (err) { - return false; - } + try { + const stats = await fs.stat(dir); + return stats.isDirectory(); + } catch (_) { + return false; + }🧰 Tools
🪛 GitHub Check: ESLint
[failure] 19-19: Disallow unused variables
'err' is defined but never used.src/routes/api/stacks/[stackId]/migrate/+server.ts (2)
1-19: API endpoint structure looks good, but type should be improved.The implementation correctly extracts the stackId parameter, validates it, calls the migration service, and handles responses appropriately.
The
anytype on line 15 should be replaced with a more specific type:- } catch (err: any) { + } catch (err: unknown) {This maintains type safety while still allowing you to check for specific error properties.
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 15-15: Disallow the
anytype
Unexpected any. Specify a different type.
12-18: Consider adding more detailed error handling and logging.The error handling is basic but functional. For better debugging and user feedback, consider including more context in the error message.
try { await migrateStackToNameFolder(stackId); return json({ success: true, message: `Stack "${stackId}" migrated successfully.` }); } catch (err: unknown) { console.error(err); - throw error(500, err?.message || 'Failed to migrate stack'); + const errorMessage = err instanceof Error ? err.message : 'Unknown error'; + console.error(`Stack migration failed for ID ${stackId}: ${errorMessage}`); + throw error(500, `Failed to migrate stack: ${errorMessage}`); }🧰 Tools
🪛 GitHub Check: ESLint
[failure] 15-15: Disallow the
anytype
Unexpected any. Specify a different type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(1 hunks)src/lib/services/api/stack-api-service.ts(1 hunks)src/lib/services/docker/stack-migration-service.ts(1 hunks)src/lib/services/docker/stack-service.ts(7 hunks)src/lib/types/actions.type.ts(1 hunks)src/lib/types/docker/stack.type.ts(2 hunks)src/lib/utils/fs.utils.ts(1 hunks)src/routes/api/stacks/[stackId]/migrate/+server.ts(1 hunks)src/routes/stacks/+page.svelte(5 hunks)src/routes/stacks/new/+page.svelte(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/lib/services/docker/stack-migration-service.ts (1)
src/lib/services/docker/stack-service.ts (4)
ensureStacksDir(52-64)isStackRunning(1032-1056)stopStack(613-712)startStack(589-600)
src/lib/services/docker/stack-service.ts (4)
src/lib/types/docker/stack.type.ts (2)
StackMeta(1-9)Stack(22-36)src/lib/services/settings-service.ts (1)
ensureStacksDirectory(56-74)src/lib/utils/fs.utils.ts (1)
directoryExists(15-22)src/lib/services/docker/core.ts (1)
getDockerClient(24-32)
🪛 GitHub Check: ESLint
src/routes/api/stacks/[stackId]/migrate/+server.ts
[failure] 15-15: Disallow the any type
Unexpected any. Specify a different type.
src/lib/utils/fs.utils.ts
[failure] 10-10: Disallow unused variables
'err' is defined but never used.
[failure] 19-19: Disallow unused variables
'err' is defined but never used.
src/lib/services/docker/stack-migration-service.ts
[failure] 19-19: Disallow unused variables
'oldComposePath' is assigned a value but never used.
[failure] 20-20: Disallow unused variables
'newComposePath' is assigned a value but never used.
[failure] 31-31: Disallow empty block statements
Empty block statement.
[failure] 113-113: Disallow unused variables
'oldComposePath' is assigned a value but never used.
[failure] 114-114: Disallow unused variables
'newComposePath' is assigned a value but never used.
[failure] 125-125: Disallow empty block statements
Empty block statement.
src/lib/services/docker/stack-service.ts
[failure] 342-342: Disallow unused variables
'newWayErr' is defined but never used.
[failure] 441-441: Disallow the any type
Unexpected any. Specify a different type.
[failure] 481-481: Disallow unused variables
'newWayErr' is defined but never used.
🔇 Additional comments (19)
package.json (1)
69-69: LGTM: Addition of slugify dependencyThe addition of the
slugifydependency is appropriate for supporting the new functionality of using human-readable stack names as folder names.src/routes/stacks/new/+page.svelte (1)
35-35:Details
❓ Verification inconclusive
Verify URL safety of stack names for navigation
The navigation target now uses the stack name instead of ID, which aligns with the PR objective. However, ensure that stack names are properly slugified before being used in URLs to prevent issues with spaces or special characters.
🏁 Script executed:
#!/bin/bash # Search for where the stack name is processed for URL usage rg -A 5 "slug|slugify" --type typescriptLength of output: 77
Ensure stack names are slugified or URL-encoded before navigation
I didn’t find any slugify or URL-encoding logic in the codebase. Confirm that stack names are safe for use in URLs by applying slugification or
encodeURIComponentbefore callinggoto.
- In
src/routes/stacks/new/+page.svelte, wrapdata.stack.namewith a slugify utility orencodeURIComponent(data.stack.name)when building the path.- If you normalize names server-side (e.g. on creation), verify that the persisted
stack.namecontains only URL-safe characters.- Test navigation with stack names containing spaces, punctuation, and non-ASCII characters to ensure the routes resolve correctly.
src/lib/services/api/stack-api-service.ts (1)
61-64: LGTM: New migrate method for stack migrationThe new
migratemethod is well-implemented and follows the same pattern as other methods in the class. It provides an API endpoint for migrating legacy stacks to the new naming convention.src/lib/types/actions.type.ts (1)
1-1: Type definition enhanced to support migration feature.The addition of 'migrate' to the StackActions type supports the new stack migration functionality, allowing type-safe usage throughout the application.
src/lib/types/docker/stack.type.ts (2)
2-8: StackMeta interface enhanced to support new naming structure.The addition of
id,path, and optionaldirNameproperties provides the necessary type support for the new human-readable stack directory naming conventions.
25-35: Stack interface appropriately updated for migration feature.The interface changes appropriately support the migration feature:
- Making properties optional provides flexibility when working with different stack formats
- Adding
isLegacyandisExternalflags allows for clear identification of stack types in the UI and backend logicsrc/routes/stacks/+page.svelte (4)
32-33: New loading state for migration action correctly added.Loading state for the 'migrate' action ensures proper user feedback during migration operations.
116-125: Migration action handler implementation looks good.The implementation follows the same pattern as other action handlers, making appropriate API calls and providing user feedback.
243-254: Well-implemented visual indicator for legacy stacks.The addition of an amber alert icon with a helpful tooltip provides clear visual feedback to users that a stack needs migration.
315-322: Migration option properly conditionally rendered for legacy stacks.The dropdown menu item is correctly shown only for legacy stacks, with appropriate styling and user guidance.
src/lib/services/docker/stack-migration-service.ts (5)
19-21: Remove unused compose-path variables
oldComposePathandnewComposePathare declared but never referenced, triggering the ESLint complaints and slightly confusing future readers.- const oldComposePath = path.join(oldDirPath, 'docker-compose.yml'); - const newComposePath = path.join(oldDirPath, 'compose.yaml');[ suggest_nitpick ]
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 19-19: Disallow unused variables
'oldComposePath' is assigned a value but never used.
[failure] 20-20: Disallow unused variables
'newComposePath' is assigned a value but never used.
28-31: Avoid emptycatch {}blocks – surface intentCompletely empty catch blocks hide errors and violate the project’s ESLint rules.
Either (a) document the intent with a comment or (b) collapse the twoaccesscalls into one wrapped helper that returns a boolean.- } catch {} + } catch { + /* meta already migrated – noop */ + }[ suggest_optional_refactor ]
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 31-31: Disallow empty block statements
Empty block statement.
54-60: Name-collision bug when several stacks slugify to the same folder
dirsis the snapshot before any renames. As soon as you migrate one stack, its new folder (newDirName) is not in that array, so a second stack that slugifies to the same name will happily pass theincludescheck and the subsequentfs.renamewill throwEEXIST.Fix by probing the live FS instead of the stale list (same change is needed further down in
migrateStackToNameFolder).- while (dirs.includes(newDirName) && newDirName !== dir) { + /* ensure real-time uniqueness */ + while (await fs.stat(path.join(stacksDir, newDirName)).then(() => true, () => false) && + newDirName !== dir) { newDirName = `${slug}-${counter++}`; }[ raise_critical_issue ]
81-89: Meta-file update should refreshupdatedAt
meta.updatedAtremains unchanged after the migration even though the stack has materially changed. Consider stamping a fresh ISO date here.meta.dirName = newDirName; meta.path = newDirPath; + meta.updatedAt = new Date().toISOString();[ suggest_optional_refactor ]
103-195: Duplicate migration logic – factor into helper
migrateStackToNameFolderrepeats ~90 % ofmigrateStacksToNameFolders. Extract common steps (stop, slugify, rename, compose rename, meta rewrite, restart) into a private helper to keep maintenance cost down.No immediate bug, but technical debt will grow as the migration evolves.
[ suggest_optional_refactor ]🧰 Tools
🪛 GitHub Check: ESLint
[failure] 113-113: Disallow unused variables
'oldComposePath' is assigned a value but never used.
[failure] 114-114: Disallow unused variables
'newComposePath' is assigned a value but never used.
[failure] 125-125: Disallow empty block statements
Empty block statement.src/lib/services/docker/stack-service.ts (4)
342-345: Remove the unusednewWayErrvariableThe caught error is not referenced – drop the binding or rename to
_to satisfy ESLint.- } catch (newWayErr) { + } catch {Same applies to the catch on line 481.
[ suggest_nitpick ]🧰 Tools
🪛 GitHub Check: ESLint
[failure] 342-342: Disallow unused variables
'newWayErr' is defined but never used.
398-415: Keep globalSTACKS_DIRin sync during stack creation
createStackcallsensureStacksDirectory()directly, which does not update the module-levelSTACKS_DIR. Subsequent calls that rely onSTACKS_DIR(e.g.getStackDir) may therefore pick up an outdated value ifinitComposeServicehasn’t run.- const stacksDir = await ensureStacksDirectory(); + const stacksDir = await ensureStacksDir(); // also refreshes STACKS_DIR[ suggest_essential_refactor ]
441-444: Replaceanywith a typed orunknownparseUsing
yaml.load(composeContent) as anybypasses type safety. At minimum:const composeData = yaml.load(composeContent) as unknown as { services?: Record<string, unknown>; };or define a small interface for the subset you need.
[ suggest_optional_refactor ]🧰 Tools
🪛 GitHub Check: ESLint
[failure] 441-441: Disallow the
anytype
Unexpected any. Specify a different type.
1032-1056:isStackRunningduplicates container-query logicThe function’s body duplicates code from
stopStackand other helpers. Consider extracting a shared internal util (e.g.getStackContainers(stackId)) to DRY up container-filter logic and centralise any future fixes (label changes, edge-cases, etc.).
[ suggest_optional_refactor ]
1 participant
Fixes: #140
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores