fix(cli): use fs.symlinkSync for paths with spaces, show version in status#833
fix(cli): use fs.symlinkSync for paths with spaces, show version in status#833
Conversation
…tatus Replace shell `ln -fs` with Node.js fs.symlinkSync to handle paths containing spaces. Extract nttVersion() into shared version.ts and print CLI version in `ntt status` output.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces shell-based symlink creation with a path-safe fs-based approach; adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli/src/__tests__/bugfix-regression.test.ts (1)
194-203: Make this a behavior test, not a source grep.This only proves
tag.tscurrently containsfs.symlinkSync. It would still pass ifcreateWorkTree()swapped the arguments, resolved the wrong path, or stopped handling spaces correctly while keeping the same call site. Please exercisecreateWorkTree()from a temp directory with spaces and assert the resultingoverrides.jsonsymlink target instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/__tests__/bugfix-regression.test.ts` around lines 194 - 203, Replace the current source-grep test with a behavior test that invokes createWorkTree from a temporary directory whose path contains spaces (use a tempdir helper to create "dir with spaces"), call the exported createWorkTree function from tag.ts to create the work tree, then assert that the resulting overrides.json is a symlink and that fs.readlinkSync (or fs.realpathSync) on that overrides.json resolves to the expected absolute path to the real overrides file (verifying argument order and space handling); ensure the test cleans up the temp dir and uses the same function name createWorkTree and file name overrides.json as referenced in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/tag.ts`:
- Around line 89-90: Prettier is required for this hunk: reformat the two-line
block so it matches project formatting (expand the compact try/catch into a
standard multi-line try { fs.unlinkSync(linkPath); } catch (e: any) { if (e.code
!== "ENOENT") throw e; } and ensure the following fs.symlinkSync(target,
linkPath) line is correctly indented/terminated); run the repository Prettier
config (or npm/yarn format script) to update the formatting for the
fs.unlinkSync/linkPath and fs.symlinkSync(target, linkPath) statements before
merging.
In `@cli/src/version.ts`:
- Around line 11-13: The code reads and splits versionFile into [commit,
installPath, version, remote] without validating the result, so missing fields
can be undefined and later break calls like includes(); update the parsing in
the function that sets versionFile and the variables commit, installPath,
version, remote to validate the array returned by versionFile.split("\n") (or
use destructuring with defaults) and explicitly reject or provide safe defaults
(e.g. version = "ntt version: unknown", commit = "", path = "", remote = "")
when there are fewer than four lines; ensure any downstream logic that calls
methods on these values (e.g. includes) only runs on defined strings.
---
Nitpick comments:
In `@cli/src/__tests__/bugfix-regression.test.ts`:
- Around line 194-203: Replace the current source-grep test with a behavior test
that invokes createWorkTree from a temporary directory whose path contains
spaces (use a tempdir helper to create "dir with spaces"), call the exported
createWorkTree function from tag.ts to create the work tree, then assert that
the resulting overrides.json is a symlink and that fs.readlinkSync (or
fs.realpathSync) on that overrides.json resolves to the expected absolute path
to the real overrides file (verifying argument order and space handling); ensure
the test cleans up the temp dir and uses the same function name createWorkTree
and file name overrides.json as referenced in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e3d1bc6-4c99-4771-9beb-aefc442aba3c
📒 Files selected for processing (4)
cli/src/__tests__/bugfix-regression.test.tscli/src/commands/status.tscli/src/tag.tscli/src/version.ts
Use nttVersion() null check instead of string comparison in index.ts. Replace source-level symlink assertion with an end-to-end test that exercises createWorkTree in a temp repo with spaces in the path.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/src/version.ts (1)
28-30: Consider guarding against emptyremotevalue.If
remoteis an empty string (e.g., due to a git error during install),remoteStringbecomes just"@"which would produce output likentt v1.0.0 (@abc123). This is a minor edge case since the installer should always write valid data.Optional defensive guard
- const remoteString = remote.includes("wormhole-foundation") + const remoteString = !remote || remote.includes("wormhole-foundation") ? "" : `${remote}@`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/version.ts` around lines 28 - 30, The code sets remoteString based on remote.includes(...) but doesn't guard against remote being empty, which yields "@" in output; update the remoteString assignment in cli/src/version.ts to first check that remote is non-empty (e.g., truthy and trimmed) before calling remote.includes, and only produce `${remote}@` when remote is present and does not include "wormhole-foundation"; otherwise set remoteString to an empty string—adjust the expression that defines remoteString to use this guard around remote.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/src/version.ts`:
- Around line 28-30: The code sets remoteString based on remote.includes(...)
but doesn't guard against remote being empty, which yields "@" in output; update
the remoteString assignment in cli/src/version.ts to first check that remote is
non-empty (e.g., truthy and trimmed) before calling remote.includes, and only
produce `${remote}@` when remote is present and does not include
"wormhole-foundation"; otherwise set remoteString to an empty string—adjust the
expression that defines remoteString to use this guard around remote.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f0d387a-d76e-48c1-b053-2a1a6c2a75d4
📒 Files selected for processing (3)
cli/src/__tests__/bugfix-regression.test.tscli/src/index.tscli/src/version.ts
Summary
Replace shell
ln -fswithfs.symlinkSyncto fix symlink creation in paths with spaces, and extract version info into a shared module sontt statusprints the CLI version.ln -fs $(pwd)/...withfs.symlinkSyncincreateWorkTreeto fix symlink creation failing when the working directory contains spacesnttVersion()into a sharedversion.tsmodule and print CLI version at the top ofntt statusoutput~/.ntt-cli/versionfiles (fewer than 4 lines)nttVersion()null check instead of string comparison inindex.tscreateWorkTreein a temp repo with spaces in the pathResolves #537
Resolves #512
Test plan
bun test src/__tests__/bugfix-regression.test.ts)ntt statusand verify version is printedSummary by CodeRabbit
New Features
Bug Fixes
Tests