[POC] fix(desktop): auto-repair moved tracked worktrees#2260
Closed
Kitenite wants to merge 20 commits into
Closed
Conversation
When a worktree directory is moved or unnested, the stored path in the local database becomes stale, causing terminal connections to fail with "Workspace path does not exist" errors. This adds auto-detection of the new path by querying `git worktree list` and matching by branch name, then updating the database so the terminal (and git status) can reconnect.
`git worktree list` includes the main worktree. If the stale worktree's branch happens to be checked out in the main repo, the repair would incorrectly rebind the worktree row to mainRepoPath. Guard against this by comparing the candidate path against the project's main repo path.
… tests - Replace resolve() with realpathSync() to canonicalize symlinks (e.g. /var → /private/var on macOS) when comparing candidate path against main repo path. - Add 6 regression tests covering: valid path no-op, successful repair after git worktree move, main repo rejection, missing project/worktree, and branch not found by git.
Use git rev-parse instead of hardcoding "main" so the test exercises the guard on environments where git init defaults to "master".
…hGitStatus After attempting path repair, check existsSync again before calling getAheadBehindCount. Without this, a stale path silently produces ahead=0/behind=0 which misleads the UI into showing the workspace as up-to-date.
# Conflicts: # apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a repair-and-resolution subsystem for tracked Git worktree paths, replaces direct DB worktree.path usage across TRPC procedures with resolution/repair helpers, converts several handlers to async, introduces git repair utilities, and adds comprehensive tests for repair, external-worktree, and delete flows. Changes
Sequence DiagramsequenceDiagram
participant Client as Terminal Client
participant Hook as useTerminalConnection
participant Router as TRPC Router
participant Resolver as RepairResolver
participant LocalDB as Local Database
participant GitRepo as Git Repository
Client->>Hook: createOrAttach(workspace)
Hook->>Router: createOrAttachMutation.mutate(...)
Router->>Resolver: resolveWorktreePathWithRepair(worktreeId)
Resolver->>LocalDB: read worktree record
LocalDB-->>Resolver: worktree metadata
Resolver->>GitRepo: inspect worktree registration / path
alt Path valid
GitRepo-->>Resolver: path confirmed
Resolver-->>Router: return resolved path
else Path moved / repairable
Resolver->>GitRepo: run git worktree repair
GitRepo-->>Resolver: repair succeeded
Resolver->>LocalDB: update stored path
Resolver-->>Router: return resolved path
else Missing / not found
Resolver-->>Router: return missing / throw NOT_FOUND
end
Router-->>Hook: mutation result
Hook->>Client: success / error callback
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts:168">
P2: Do not swallow metadata read failures with an empty catch; log context so auto-repair misses are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts:216">
P1: Handle `getTrackedWorktreePath` failures in the delete mutation and clear deleting status before returning. Right now a `git_repair_required` throw can leave the workspace stuck in `deletingAt` state.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts:173">
P2: Using `split("/")` to derive the worktree name is not cross-platform and breaks on Windows paths. Use a separator-agnostic basename extraction.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
# Conflicts: # apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
178-180:⚠️ Potential issue | 🟠 MajorResolve the worktree path before mutating delete state.
getTrackedWorktreePath()can now throw from the repair flow, butmarkWorkspaceAsDeleting(),updateActiveWorkspaceIfRemoved(), andkillByWorkspaceId()have already been triggered by then. A repair-required workspace will therefore fail deletion after we've hidden it and started killing terminals, with no rollback here.Also applies to: 207-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts` around lines 178 - 180, Compute/resolve the workspace worktree path by calling getTrackedWorktreePath(input.id) (and handle its potential throw) before making any state changes; only after successfully obtaining the path proceed to call markWorkspaceAsDeleting(input.id), updateActiveWorkspaceIfRemoved(input.id), and killByWorkspaceId(input.id). Apply the same change to the other deletion branch that calls these three mutators (the block referenced as 207-218), ensuring the path resolution happens first and errors abort before hiding/starting kills.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts`:
- Around line 340-345: Don't short-circuit and treat worktree.path as resolved
just because existsSync returned true; instead remove the immediate return for
worktree.path and run the same validation used later (check for .git/metadata
and call isMainRepoPath()) before returning status:"resolved". Apply this change
to the early branch that currently returns when existsSync(worktree.path) and to
the analogous block around the 393-400 area so both locations perform
metadata/.git validation and isMainRepoPath() guard prior to marking the path
resolved.
---
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts`:
- Around line 178-180: Compute/resolve the workspace worktree path by calling
getTrackedWorktreePath(input.id) (and handle its potential throw) before making
any state changes; only after successfully obtaining the path proceed to call
markWorkspaceAsDeleting(input.id), updateActiveWorkspaceIfRemoved(input.id), and
killByWorkspaceId(input.id). Apply the same change to the other deletion branch
that calls these three mutators (the block referenced as 207-218), ensuring the
path resolution happens first and errors abort before hiding/starting kills.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c526a2a5-bb99-4b38-82e5-bf9e0c22e8ca
📒 Files selected for processing (9)
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts
Comment on lines
+340
to
+345
| if (existsSync(worktree.path)) { | ||
| return { | ||
| status: "resolved", | ||
| path: worktree.path, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't treat worktree.path as resolved just because it exists.
Line 340 short-circuits on raw filesystem existence, which bypasses both the .git/metadata validation and the isMainRepoPath() guard below. If the old location gets recreated after a manual move—or an older row already contains the main repo path—we'll hand downstream delete/status/terminal flows the wrong directory.
Also applies to: 393-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts`
around lines 340 - 345, Don't short-circuit and treat worktree.path as resolved
just because existsSync returned true; instead remove the immediate return for
worktree.path and run the same validation used later (check for .git/metadata
and call isMainRepoPath()) before returning status:"resolved". Apply this change
to the early branch that currently returns when existsSync(worktree.path) and to
the analogous block around the 393-400 area so both locations perform
metadata/.git validation and isMainRepoPath() guard prior to marking the path
resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 85-89: The helper getTrackedWorktreePath currently hides
resolution failures by returning resolveWorktreePathOrThrow(worktree.id) ??
worktree.path; change getTrackedWorktreePath to return Promise<string | null>
and return the raw result of resolveWorktreePathOrThrow (no "?? worktree.path"
fallback) so unresolved worktrees yield null; then update callers
(handleExistingWorktree, openWorktree and any branch-reopen logic) to handle a
null return from getTrackedWorktreePath/resolveWorktreePathOrThrow instead of
assuming worktree.path, and use worktree.id/worktree.path only when resolution
succeeded.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts`:
- Around line 209-217: Currently we eagerly call getWorkspacePathForQuery (which
may invoke resolveWorktreePathWithRepair) for every entry in allWorkspaces;
instead, first filter allWorkspaces by whether groupsMap has the
workspace.projectId (i.e. only keep visible projects) and then run Promise.all
on that filtered list so we don't trigger filesystem/git repairs for workspaces
that will be dropped; update the code around allWorkspaces, groupsMap, and
getWorkspacePathForQuery to perform the prefiltering before resolving worktree
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4044ee99-d1dc-4437-aef9-7d5b70cc2dee
📒 Files selected for processing (4)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts (1)
185-200: Consider logging or re-throwing unexpected errors in the catch block.The empty
catch {}at line 199 silently swallows all errors, including unexpected ones beyond file read failures. This could make debugging difficult if there are issues with the metadata parsing logic.♻️ Suggested improvement
try { const head = readFileSync(headPath, "utf8").trim(); const rawGitdir = readFileSync(gitdirPath, "utf8").trim(); const registeredGitdir = isAbsolute(rawGitdir) ? safeResolvePath(rawGitdir) : safeResolvePath(resolve(metadataDir, rawGitdir)); const registeredPath = dirname(registeredGitdir); if ( head === `ref: refs/heads/${input.context.worktree.branch}` || safeResolvePath(registeredPath) === expectedStoredPath ) { return { metadataDir, registeredPath }; } - } catch {} + } catch (error) { + // Expected: ENOENT, EACCES for inaccessible entries + // Continue scanning other metadata directories + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts` around lines 185 - 200, The empty catch in the loop around reading headPath/gitdirPath (inside repair-worktree-path logic) swallows all errors; update the catch to surface unexpected failures by catching (err) and either logging the error with context (include headPath, gitdirPath, metadataDir and the caught error) using the repo's logger or re-throwing the error after logging; ensure you still silently continue only for known benign errors (e.g., ENOENT) if intended, but do not leave catch {} — modify the try/catch that reads head, rawGitdir and computes registeredGitdir/registeredPath to report the error via the existing logging utility or rethrow it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts`:
- Around line 456-469: In resolveTrackedWorktreePath, don’t short-circuit by
returning buildResolvedResult(context.worktree.path) solely because existsSync
is true; first validate the stored path using
isMainRepoPath(context.worktree.path) to reject main-repo paths and use
parseGitdirReference(context.worktree.path) (and/or verify the referenced gitdir
exists) to ensure the directory is a real Git worktree; if either check fails
return buildMissingResolutionResult() (or fall through to
resolveTrackedWorktreePathFromGitState(context)), otherwise return
buildResolvedResult(...). This preserves existing calls to
resolveTrackedWorktreePathFromGitState and uses the already-defined
parseGitdirReference and isMainRepoPath helpers.
- Around line 578-596: The comparison in findProjectWorktreeByCurrentPath can
miss matches due to differing path formats; normalize both worktreePath and
trackedWorktree.worktree.path before comparing (e.g., use
path.resolve/path.normalize and, where appropriate, fs.realpath or toLowerCase
on case-insensitive platforms) so trailing slashes, ./ segments and symlinks are
handled consistently; update the loop in findProjectWorktreeByCurrentPath to
compute normalized/real paths for worktreePath and trackedWorktree.worktree.path
(or cache normalized values from listProjectWorktreesWithCurrentPaths) and
compare those normalized strings instead of strict equality on the raw values.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts`:
- Around line 185-200: The empty catch in the loop around reading
headPath/gitdirPath (inside repair-worktree-path logic) swallows all errors;
update the catch to surface unexpected failures by catching (err) and either
logging the error with context (include headPath, gitdirPath, metadataDir and
the caught error) using the repo's logger or re-throwing the error after
logging; ensure you still silently continue only for known benign errors (e.g.,
ENOENT) if intended, but do not leave catch {} — modify the try/catch that reads
head, rawGitdir and computes registeredGitdir/registeredPath to report the error
via the existing logging utility or rethrow it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75d209e9-034b-413e-87ec-2b77f48d3def
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.ts
Comment on lines
+456
to
+469
| export async function resolveTrackedWorktreePath( | ||
| worktreeId: string, | ||
| ): Promise<ResolveTrackedWorktreePathResult> { | ||
| const context = getTrackedWorktreeContext(worktreeId); | ||
| if (!context) { | ||
| return buildMissingResolutionResult(); | ||
| } | ||
|
|
||
| if (existsSync(context.worktree.path)) { | ||
| return buildResolvedResult(context.worktree.path); | ||
| } | ||
|
|
||
| return resolveTrackedWorktreePathFromGitState(context); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate worktree path before short-circuiting on existence.
Lines 464-466 return the stored path as resolved if existsSync returns true, but this bypasses:
- The
isMainRepoPath()guard that protects against accidentally operating on the main repo - Validation that the directory is actually a Git worktree (has valid
.gitfile with gitdir reference)
If the old location gets recreated as a regular directory after a manual move, or if the stored path points to the main repo, downstream operations (terminal, delete, git-status) could operate on the wrong directory.
🔧 Proposed fix
export async function resolveTrackedWorktreePath(
worktreeId: string,
): Promise<ResolveTrackedWorktreePathResult> {
const context = getTrackedWorktreeContext(worktreeId);
if (!context) {
return buildMissingResolutionResult();
}
- if (existsSync(context.worktree.path)) {
- return buildResolvedResult(context.worktree.path);
+ if (existsSync(context.worktree.path)) {
+ // Validate it's actually a worktree and not the main repo
+ const gitdirRef = parseGitdirReference(context.worktree.path);
+ if (gitdirRef && !isMainRepoPath(context, context.worktree.path)) {
+ return buildResolvedResult(context.worktree.path);
+ }
+ // Path exists but is not a valid worktree - fall through to git state resolution
}
return resolveTrackedWorktreePathFromGitState(context);
}Note: parseGitdirReference and isMainRepoPath are already defined in this module and can be reused here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts`
around lines 456 - 469, In resolveTrackedWorktreePath, don’t short-circuit by
returning buildResolvedResult(context.worktree.path) solely because existsSync
is true; first validate the stored path using
isMainRepoPath(context.worktree.path) to reject main-repo paths and use
parseGitdirReference(context.worktree.path) (and/or verify the referenced gitdir
exists) to ensure the directory is a real Git worktree; if either check fails
return buildMissingResolutionResult() (or fall through to
resolveTrackedWorktreePathFromGitState(context)), otherwise return
buildResolvedResult(...). This preserves existing calls to
resolveTrackedWorktreePathFromGitState and uses the already-defined
parseGitdirReference and isMainRepoPath helpers.
Comment on lines
+578
to
+596
| export async function findProjectWorktreeByCurrentPath( | ||
| projectId: string, | ||
| worktreePath: string, | ||
| ): Promise<SelectWorktree | null> { | ||
| const trackedWorktrees = | ||
| await listProjectWorktreesWithCurrentPaths(projectId); | ||
|
|
||
| for (const trackedWorktree of trackedWorktrees) { | ||
| if (!trackedWorktree.existsOnDisk) { | ||
| continue; | ||
| } | ||
|
|
||
| if (trackedWorktree.worktree.path === worktreePath) { | ||
| return trackedWorktree.worktree; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize paths before comparison to avoid false negatives.
Line 590 uses strict equality for path comparison, which could fail to match equivalent paths with different formats (e.g., trailing slashes, symlink resolution, or case differences on case-insensitive filesystems).
🔧 Proposed fix
export async function findProjectWorktreeByCurrentPath(
projectId: string,
worktreePath: string,
): Promise<SelectWorktree | null> {
const trackedWorktrees =
await listProjectWorktreesWithCurrentPaths(projectId);
+ const normalizedInputPath = safeRealpath(worktreePath);
for (const trackedWorktree of trackedWorktrees) {
if (!trackedWorktree.existsOnDisk) {
continue;
}
- if (trackedWorktree.worktree.path === worktreePath) {
+ if (safeRealpath(trackedWorktree.worktree.path) === normalizedInputPath) {
return trackedWorktree.worktree;
}
}
return null;
}📝 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.
Suggested change
| export async function findProjectWorktreeByCurrentPath( | |
| projectId: string, | |
| worktreePath: string, | |
| ): Promise<SelectWorktree | null> { | |
| const trackedWorktrees = | |
| await listProjectWorktreesWithCurrentPaths(projectId); | |
| for (const trackedWorktree of trackedWorktrees) { | |
| if (!trackedWorktree.existsOnDisk) { | |
| continue; | |
| } | |
| if (trackedWorktree.worktree.path === worktreePath) { | |
| return trackedWorktree.worktree; | |
| } | |
| } | |
| return null; | |
| } | |
| export async function findProjectWorktreeByCurrentPath( | |
| projectId: string, | |
| worktreePath: string, | |
| ): Promise<SelectWorktree | null> { | |
| const trackedWorktrees = | |
| await listProjectWorktreesWithCurrentPaths(projectId); | |
| const normalizedInputPath = safeRealpath(worktreePath); | |
| for (const trackedWorktree of trackedWorktrees) { | |
| if (!trackedWorktree.existsOnDisk) { | |
| continue; | |
| } | |
| if (safeRealpath(trackedWorktree.worktree.path) === normalizedInputPath) { | |
| return trackedWorktree.worktree; | |
| } | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.ts`
around lines 578 - 596, The comparison in findProjectWorktreeByCurrentPath can
miss matches due to differing path formats; normalize both worktreePath and
trackedWorktree.worktree.path before comparing (e.g., use
path.resolve/path.normalize and, where appropriate, fs.realpath or toLowerCase
on case-insensitive platforms) so trailing slashes, ./ segments and symlinks are
handled consistently; update the loop in findProjectWorktreeByCurrentPath to
compute normalized/real paths for worktreePath and trackedWorktree.worktree.path
(or cache normalized values from listProjectWorktreesWithCurrentPaths) and
compare those normalized strings instead of strict equality on the raw values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (1)
910-923:⚠️ Potential issue | 🟠 MajorUse the resolved branch for post-import side effects.
resolveExternalWorktreeOpenTarget()can correct a stale request and return a different branch thaninput.branch. This block already persistsworktreeTarget.branchto the worktree/workspace rows, buttrack()andsetBranchBaseConfig()still use the stale input, so base-branch config can be written for the wrong branch.Suggested fix
track("workspace_created", { workspace_id: workspace.id, project_id: project.id, - branch: input.branch, + branch: worktreeTarget.branch, base_branch: baseBranch, source: "external_import", }); await setBranchBaseConfig({ repoPath: project.mainRepoPath, - branch: input.branch, + branch: worktreeTarget.branch, baseBranch, isExplicit: false, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around lines 910 - 923, The telemetry and post-import config writes are still using the original input.branch instead of the resolved branch from resolveExternalWorktreeOpenTarget; update the calls so they use the resolved worktreeTarget.branch (the value persisted to the worktree/workspace) when invoking track("workspace_created", {...}) and when calling setBranchBaseConfig({ repoPath: project.mainRepoPath, branch: ..., baseBranch, isExplicit: false }), ensuring you reference worktreeTarget.branch rather than input.branch so base-branch config and analytics reflect the actual opened branch.
♻️ Duplicate comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (1)
90-94:⚠️ Potential issue | 🟠 MajorStop falling back to the stale DB path here.
Line 93 hides unresolved tracked paths behind
worktree.path, so reopen/open/setup flows keep operating on the stale location whenresolveWorktreePathOrThrow()returnsnull. Let this helper returnnulland make callers handle the unresolved state explicitly instead of silently reviving the old path.Suggested fix
async function getTrackedWorktreePath( worktree: typeof worktrees.$inferSelect, -): Promise<string> { - return (await resolveWorktreePathOrThrow(worktree.id)) ?? worktree.path; +): Promise<string | null> { + return await resolveWorktreePathOrThrow(worktree.id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around lines 90 - 94, The helper getTrackedWorktreePath should not fall back to the stale DB path; change its implementation so it returns the resolved path or null (i.e., return await resolveWorktreePathOrThrow(worktree.id) without defaulting to worktree.path) and update callers of getTrackedWorktreePath to explicitly handle the null/unresolved case instead of assuming a valid path; reference getTrackedWorktreePath, resolveWorktreePathOrThrow, and worktree.path when locating the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/external-worktrees.ts`:
- Around line 57-69: The branch-match fallbacks in
resolveExternalWorktreeOpenTarget are reusing tracked rows even when their
recorded paths are dead; change the fallback logic that uses
__testOnlyExternalWorktreeDeps.findWorktreeWorkspaceByBranch and
__testOnlyExternalWorktreeDeps.findOrphanedWorktreeByBranch so you only accept
their worktree if its currentPath actually exists on disk (same criterion used
by listImportableExternalWorktrees). In practice, after getting a branch match
from findWorktreeWorkspaceByBranch(...)? .worktree or
findOrphanedWorktreeByBranch(...), verify the worktree.currentPath is present
(e.g., resolve and stat/exist check) before assigning trackedWorktree; apply the
same guarded check to the other occurrence around lines 113-129.
---
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 910-923: The telemetry and post-import config writes are still
using the original input.branch instead of the resolved branch from
resolveExternalWorktreeOpenTarget; update the calls so they use the resolved
worktreeTarget.branch (the value persisted to the worktree/workspace) when
invoking track("workspace_created", {...}) and when calling
setBranchBaseConfig({ repoPath: project.mainRepoPath, branch: ..., baseBranch,
isExplicit: false }), ensuring you reference worktreeTarget.branch rather than
input.branch so base-branch config and analytics reflect the actual opened
branch.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 90-94: The helper getTrackedWorktreePath should not fall back to
the stale DB path; change its implementation so it returns the resolved path or
null (i.e., return await resolveWorktreePathOrThrow(worktree.id) without
defaulting to worktree.path) and update callers of getTrackedWorktreePath to
explicitly handle the null/unresolved case instead of assuming a valid path;
reference getTrackedWorktreePath, resolveWorktreePathOrThrow, and worktree.path
when locating the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 409d3234-19b9-4207-acf3-f5531287a2d3
📒 Files selected for processing (4)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/external-worktrees.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/external-worktrees.ts
# Conflicts: # apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
Kitenite
changed the title
Contributor
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.ts:538">
P3: Use a cross-platform move command in this test; `mv` will fail under Windows `cmd` when `execSync` runs.
(Based on your team's feedback about gating platform-specific changes with runtime checks.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| `git -C "${mainRepo}" worktree add "${oldPath}" -b feat-backoff HEAD`, | ||
| { stdio: "ignore" }, | ||
| ); | ||
| execSync(`mv "${oldPath}" "${newPath}"`, { stdio: "ignore" }); |
Contributor
There was a problem hiding this comment.
P3: Use a cross-platform move command in this test; mv will fail under Windows cmd when execSync runs.
(Based on your team's feedback about gating platform-specific changes with runtime checks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/utils/repair-worktree-path.test.ts, line 538:
<comment>Use a cross-platform move command in this test; `mv` will fail under Windows `cmd` when `execSync` runs.
(Based on your team's feedback about gating platform-specific changes with runtime checks.) </comment>
<file context>
@@ -481,5 +516,56 @@ describe("repair-worktree-path", () => {
+ `git -C "${mainRepo}" worktree add "${oldPath}" -b feat-backoff HEAD`,
+ { stdio: "ignore" },
+ );
+ execSync(`mv "${oldPath}" "${newPath}"`, { stdio: "ignore" });
+
+ mockWorktrees.set("wt-backoff-1", {
</file context>
Suggested change
| execSync(`mv "${oldPath}" "${newPath}"`, { stdio: "ignore" }); | |
| execSync( | |
| process.platform === "win32" | |
| ? `move "${oldPath}" "${newPath}"` | |
| : `mv "${oldPath}" "${newPath}"`, | |
| { stdio: "ignore" }, | |
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
git worktree repairTesting
Summary by cubic
Auto-repair moved tracked worktrees so terminal, git status, queries, and delete keep working after directory renames or unnesting. Sidebar queries now use a lightweight display-only repair state to avoid UI lag; full repair runs only on actions like open/attach.
Bug Fixes
deletingAton errors.NOT_FOUNDinstead of showing 0/0; derive worktree name from the resolved path.getWorkspaceCwduse the repair resolver; on attach, invalidateworkspaces.get,workspaces.getAllGrouped, andterminal.getWorkspaceCwd.realpathSyncto canonicalize symlinks.Refactors
repair-worktree-pathandrepairWorktreeRegistration; addedexternal-worktreeshelpers (resolveExternalWorktreeOpenTarget,listImportableExternalWorktrees) and adopted them across create/import, status, query, and terminal flows.worktreePath,existsOnDisk, and repair metadata (state/message/command). The sidebar now renders this display-only state without triggering repair, improving responsiveness.Written for commit 386cebb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests