refactor(devcontainer): restructure with project-specific templates #6
Conversation
📝 WalkthroughWalkthroughThis PR restructures dev container configuration by removing shared settings from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @features/fnm/devcontainer-feature.json:
- Around line 1-13: The manifest for the fnm feature is valid but could include
optional metadata to improve discoverability; add fields such as "author",
"homepage", "tags" (array of strings), and "license" at the top level alongside
"id" and "version" so consumers can surface the feature better, and ensure the
existing "options.nodeVersion" (default "lts", description unchanged) remains
intact.
In @features/fnm/install.sh:
- Around line 10-11: The script only sets FNM_PATH and PATH for the current
session; after those assignments, append persistent shell initialization so fnm
is automatically initialized in future sessions: detect common shells and add
the appropriate fnm initialization snippet to each shell’s startup file (for
example, add the bash/zsh init line and the fish init line) so that invoking
fnm/env happens on new shells; update the install script (around the FNM_PATH
and PATH assignments) to write or idempotently append those init lines to the
user’s shell rc files and ensure you don’t duplicate entries on repeated runs.
In @features/uv/devcontainer-feature.json:
- Around line 1-13: Add optional metadata fields to the feature manifest to
improve discoverability and maintainability: extend the top-level object that
already contains "id", "version", "name", "description", and "options" by adding
keys such as "displayName", "categories" (array), "maintainers" (array of
objects with name/email), "license", "repository" (url), and optional "tags" and
"postAttachCommand" so consumers can see human-friendly name, authorship,
licensing and source without changing the existing "pythonVersion" option.
In @projects/.template/.devcontainer/devcontainer.json:
- Line 8: The postCreateCommand currently only changes the ownership of the
workspace directory itself; update the "postCreateCommand" to perform a
recursive, owner-and-group change so all files under ${containerWorkspaceFolder}
are chowned correctly (use chown -R and include $(id -gn) or :$(id -gn) to set
the group) and ensure you still use $(id -un) for the user to preserve the
original intent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.vscode/extensions.jsonREADME.mdcompose-base/docker-compose.yamlfeatures/fnm/devcontainer-feature.jsonfeatures/fnm/install.shfeatures/uv/devcontainer-feature.jsonfeatures/uv/install.shprojects/.template/.devcontainer/devcontainer.jsonprojects/.template/docker-compose.yaml
💤 Files with no reviewable changes (1)
- compose-base/docker-compose.yaml
🧰 Additional context used
🪛 Shellcheck (0.11.0)
features/uv/install.sh
[info] 9-9: Not following: ./.local/bin/env was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (17)
.vscode/extensions.json (1)
2-7: LGTM!The addition of the MarkdownLint extension is a good choice for maintaining consistent markdown documentation quality, especially given the project contains documentation files. The reordering of extensions has no functional impact.
projects/.template/docker-compose.yaml (1)
5-5: LGTM!The updated relative path correctly references the base Dockerfile after the restructuring.
projects/.template/.devcontainer/devcontainer.json (4)
2-2: LGTM!Consolidating to a single
dockerComposeFileentry simplifies the configuration.
6-7: LGTM!Using
${localWorkspaceFolderBasename}for dynamic naming and settingoverrideCommandto true are appropriate for a devcontainer template.
10-10: LGTM!The workspace folder path correctly uses
${localWorkspaceFolderBasename}, aligning with the new template-based approach and removing the dependency on theVSCODE_CONTAINER_PROJECTenvironment variable.
3-5: No action needed — the mount configuration is correct.The
${containerWorkspaceFolder}variable is properly supported in devcontainer.json mounts and resolves to theworkspaceFolderproperty value. The configuration correctly mounts theworkspacevolume to/mnt/${localWorkspaceFolderBasename}, which aligns with the documented behavior in README.md.README.md (3)
3-3: LGTM!The updated description accurately reflects the new template-based workflow and removal of the shared
compose-base/structure.
12-12: LGTM!The simplified instructions correctly reflect the removal of the
VSCODE_CONTAINER_PROJECTenvironment variable requirement, making the setup process more streamlined.
14-14: Documentation is accurate. The workspace path/mnt/<your-project-name>correctly reflects theworkspaceFoldersetting in devcontainer.json (/mnt/${localWorkspaceFolderBasename}). The volume naming is also correct: theworkspacevolume source in devcontainer.json is automatically prefixed by Docker Compose with the project name (derived from the directory name<your-project-name>), resulting in the actual volume name<your-project-name>_workspace.features/fnm/install.sh (4)
1-4: LGTM: User context handling is correct.The script appropriately ensures execution under the designated remote user by re-invoking itself with a login shell.
6-6: LGTM: Strict shell options are enabled.
13-20: Node.js installation logic is correct.The conditional installation logic properly handles the "lts", "none", and specific version cases, matching the manifest's nodeVersion option. However, this logic depends on fnm being properly initialized (see previous comment about shell initialization).
8-8: No action needed. The installation URLhttps://fnm.vercel.app/installis the official and current method recommended in fnm's GitHub README for automatic installation on macOS/Linux.features/uv/install.sh (4)
1-4: LGTM: User context handling is correct.
6-6: LGTM: Strict shell options are enabled.
8-9: Installation URL is official and correct—no changes needed.The code correctly uses the official uv installer (https://astral.sh/uv/install.sh), and the installer is explicitly designed to create the
$HOME/.local/bin/envfile as part of its standard behavior. Sourcing without an existence check is appropriate here—if the installer fails to create this file, the script should fail at that point, which correctly signals an installation problem rather than silently skipping the environment setup.Likely an incorrect or invalid review comment.
11-14: Python installation logic is correct.The conditional installation properly handles default and specific Python versions, with appropriate skip behavior for "none". The
--defaultflag ensures the version becomes the default for uv.The uv installer (run on line 8) by default modifies shell configuration files (~/.bashrc, ~/.zshrc, etc.) to add PATH and autocompletion, so uv will be available in future shell sessions. The sourcing of
$HOME/.local/bin/envon line 9 ensures uv is available for the rest of the installation script execution.
| { | ||
| "id": "fnm", | ||
| "version": "1.0.0", | ||
| "name": "Fast Node Manager (fnm)", | ||
| "description": "Installs fnm and optionally a Node.js version.", | ||
| "options": { | ||
| "nodeVersion": { | ||
| "type": "string", | ||
| "default": "lts", | ||
| "description": "Node.js version to install with fnm. Use 'none' to skip." | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured manifest. Consider adding optional metadata.
The feature manifest is correctly structured with appropriate options that align with the install script. The nodeVersion option properly defaults to "lts" and clearly documents the "none" skip behavior.
📝 Optional: Add additional metadata
Consider enriching the manifest with optional fields for better discoverability:
{
"id": "fnm",
"version": "1.0.0",
"name": "Fast Node Manager (fnm)",
"description": "Installs fnm and optionally a Node.js version.",
+ "documentationURL": "https://github.com/Schniz/fnm",
+ "licenseURL": "https://github.com/Schniz/fnm/blob/master/LICENSE",
"options": {
"nodeVersion": {
"type": "string",
"default": "lts",
"description": "Node.js version to install with fnm. Use 'none' to skip."
}
}
}📝 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.
| { | |
| "id": "fnm", | |
| "version": "1.0.0", | |
| "name": "Fast Node Manager (fnm)", | |
| "description": "Installs fnm and optionally a Node.js version.", | |
| "options": { | |
| "nodeVersion": { | |
| "type": "string", | |
| "default": "lts", | |
| "description": "Node.js version to install with fnm. Use 'none' to skip." | |
| } | |
| } | |
| } | |
| { | |
| "id": "fnm", | |
| "version": "1.0.0", | |
| "name": "Fast Node Manager (fnm)", | |
| "description": "Installs fnm and optionally a Node.js version.", | |
| "documentationURL": "https://github.com/Schniz/fnm", | |
| "licenseURL": "https://github.com/Schniz/fnm/blob/master/LICENSE", | |
| "options": { | |
| "nodeVersion": { | |
| "type": "string", | |
| "default": "lts", | |
| "description": "Node.js version to install with fnm. Use 'none' to skip." | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In @features/fnm/devcontainer-feature.json around lines 1 - 13, The manifest for
the fnm feature is valid but could include optional metadata to improve
discoverability; add fields such as "author", "homepage", "tags" (array of
strings), and "license" at the top level alongside "id" and "version" so
consumers can surface the feature better, and ensure the existing
"options.nodeVersion" (default "lts", description unchanged) remains intact.
| FNM_PATH="$HOME/.local/share/fnm" | ||
| export PATH="$FNM_PATH:$PATH" |
There was a problem hiding this comment.
Missing fnm shell initialization for persistent activation.
While the PATH is set for the current session, fnm requires shell initialization to work correctly across sessions. The installation script should configure shell startup files to automatically initialize fnm.
🔧 Add fnm shell initialization
After line 11, add shell initialization to the user's shell configuration:
FNM_PATH="$HOME/.local/share/fnm"
export PATH="$FNM_PATH:$PATH"
+
+# Initialize fnm in shell startup files
+cat >> "$HOME/.bashrc" <<'EOF'
+
+# fnm
+export FNM_PATH="$HOME/.local/share/fnm"
+export PATH="$FNM_PATH:$PATH"
+eval "$(fnm env)"
+EOFThis ensures fnm is available in all future shell sessions and enables features like automatic version switching.
🤖 Prompt for AI Agents
In @features/fnm/install.sh around lines 10 - 11, The script only sets FNM_PATH
and PATH for the current session; after those assignments, append persistent
shell initialization so fnm is automatically initialized in future sessions:
detect common shells and add the appropriate fnm initialization snippet to each
shell’s startup file (for example, add the bash/zsh init line and the fish init
line) so that invoking fnm/env happens on new shells; update the install script
(around the FNM_PATH and PATH assignments) to write or idempotently append those
init lines to the user’s shell rc files and ensure you don’t duplicate entries
on repeated runs.
| { | ||
| "id": "uv", | ||
| "version": "1.0.0", | ||
| "name": "uv", | ||
| "description": "Installs uv and optionally a Python version.", | ||
| "options": { | ||
| "pythonVersion": { | ||
| "type": "string", | ||
| "default": "3", | ||
| "description": "Python version to install with uv. Use 'none' to skip." | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured manifest. Consider adding optional metadata.
The feature manifest is correctly structured with a pythonVersion option that aligns with the install script. The default value of "3" and "none" skip behavior are clearly documented.
📝 Optional: Add additional metadata
Consider enriching the manifest with optional fields:
{
"id": "uv",
"version": "1.0.0",
"name": "uv",
"description": "Installs uv and optionally a Python version.",
+ "documentationURL": "https://docs.astral.sh/uv/",
+ "licenseURL": "https://github.com/astral-sh/uv/blob/main/LICENSE-MIT",
"options": {
"pythonVersion": {
"type": "string",
"default": "3",
"description": "Python version to install with uv. Use 'none' to skip."
}
}
}🤖 Prompt for AI Agents
In @features/uv/devcontainer-feature.json around lines 1 - 13, Add optional
metadata fields to the feature manifest to improve discoverability and
maintainability: extend the top-level object that already contains "id",
"version", "name", "description", and "options" by adding keys such as
"displayName", "categories" (array), "maintainers" (array of objects with
name/email), "license", "repository" (url), and optional "tags" and
"postAttachCommand" so consumers can see human-friendly name, authorship,
licensing and source without changing the existing "pythonVersion" option.
| ], | ||
| "name": "${localWorkspaceFolderBasename}", | ||
| "overrideCommand": true, | ||
| "postCreateCommand": "sudo chown $(id -un) ${containerWorkspaceFolder}", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider enhancing the ownership change command.
The postCreateCommand only changes the owner of the workspace directory itself, not its contents. If files are already present in the workspace volume, they will retain their original ownership.
♻️ Suggested enhancement
Consider adding the -R flag for recursive ownership change and including the group:
- "postCreateCommand": "sudo chown $(id -un) ${containerWorkspaceFolder}",
+ "postCreateCommand": "sudo chown -R $(id -un):$(id -gn) ${containerWorkspaceFolder}",This ensures all files in the workspace have the correct ownership and group membership.
📝 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.
| "postCreateCommand": "sudo chown $(id -un) ${containerWorkspaceFolder}", | |
| "postCreateCommand": "sudo chown -R $(id -un):$(id -gn) ${containerWorkspaceFolder}", |
🤖 Prompt for AI Agents
In @projects/.template/.devcontainer/devcontainer.json at line 8, The
postCreateCommand currently only changes the ownership of the workspace
directory itself; update the "postCreateCommand" to perform a recursive,
owner-and-group change so all files under ${containerWorkspaceFolder} are
chowned correctly (use chown -R and include $(id -gn) or :$(id -gn) to set the
group) and ensure you still use $(id -un) for the user to preserve the original
intent.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Quick Notes
Title: Set the PR title to
@coderabbitaito have the bot generate one for you.Review: The bot reviews PRs by default. To opt out, add the
no bot reviewerlabel.