Repository navigation
fix: expand tilde in project path input - #851
ranvier2d2 wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
github-actions
Bot
added
the
vouch:unvouched
| function expandTilde(p: string): string { | ||
| if (p === "~") return OS.homedir(); | ||
| if (p.startsWith("~/") || p.startsWith("~\\")) return path.join(OS.homedir(), p.slice(2)); | ||
| return p; |
There was a problem hiding this comment.
Nice change!
I would ask that we move to something like this instead though:
| function expandTilde(p: string): string { | |
| if (p === "~") return OS.homedir(); | |
| if (p.startsWith("~/") || p.startsWith("~\\")) return path.join(OS.homedir(), p.slice(2)); | |
| return p; | |
| function expandTilde(pathStr: string): string { | |
| return pathStr.replace(/^~(?=$|[\\/])/, OS.homedir()); |
- Using a full parameter name makes it easier for people to understand what to pass via lsp if there are no jsDocs for the function.
- We are doing less comparisons, so there will be added performance
- We only have one return point over 3
There was a problem hiding this comment.
Just read this! WIll review. THanks
There was a problem hiding this comment.
Nice change!
I would ask that we move to something like this instead though:
- Using a full parameter name makes it easier for people to understand what to pass via lsp if there are no jsDocs for the function.
- We are doing less comparisons, so there will be added performance
- We only have one return point over 3
Applied. Much cleaner with the regex. Also renamed the param to pathStr per your suggestion. Thanks for the review!
| @@ -16,6 +16,11 @@ export function fixPath(): void { | |||
| } | |||
| } | |||
|
|
|||
| /** Plain synchronous tilde expansion (no Effect dependency). */ | |||
| export function expandTilde(pathStr: string): string { | |||
There was a problem hiding this comment.
can we replace the old expandHomePath with this? don't like having 2 doing the same thing
There was a problem hiding this comment.
Replaced expandHomePath with expandTilde and updated the two call sites (resolveStateDir in os-jank.ts and normalizeProjectWorkspaceRoot in wsServer.ts). Single function now handles all tilde expansion.
Replace the Effect-based expandHomePath with a plain synchronous expandTilde using regex replacement. Apply it in resolveThreadWorkspaceCwd (read-path for legacy data), resolveStateDir, and normalizeProjectWorkspaceRoot (write-path, previously using expandHomePath). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
Changes
expandTilde()helper inapps/server/src/os-jank.ts(regex-based, per review feedback from @binbandit)resolveThreadWorkspaceCwd()inapps/server/src/checkpointing/Utils.ts, which is the choke point used by CheckpointReactor, CheckpointDiffQuery, ProviderCommandReactor, and ProviderRuntimeIngestionapps/server/src/os-jank.test.tsContext
PR #320 already added tilde expansion at project-creation time via
normalizeProjectWorkspaceRootinwsServer.ts. This PR complements that by adding read-time expansion inresolveThreadWorkspaceCwd, so any legacy database entries stored before #320 also resolve correctly.Test plan
bun run testpasses foros-jank.test.ts(6 tests)Note
Expand tilde in project path inputs across workspace resolution
expandHomePathwith a pureexpandTildefunction in os-jank.ts that uses a regex andOS.homedir()to expand a leading~.expandTildeinresolveThreadWorkspaceCwd(Utils.ts) andnormalizeProjectWorkspaceRoot(wsServer.ts) so~-prefixed paths resolve correctly at runtime.~,~/,~\, non-leading~, absolute paths, and empty strings.expandHomePathis no longer exported; callers must useexpandTildeinstead.Macroscope summarized 5a79f28.