Conversation
…ctId> Mirror v1 desktop's convention of keeping worktrees outside the primary checkout tree, and match v2's existing ~/.superset/repos/<projectId> layout for symmetry. Detection switches to the local `workspaces` table (plus the new root for orphan adoption), so legacy worktrees at <repo>/.worktrees/ keep working without a migration.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWorktree resolution and discovery were moved to a Superset-managed per-project directory at Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HostService as HostService (router)
participant DB as Workspaces DB
participant FS as Filesystem
participant Git as Git
Client->>HostService: request (list/create/adopt) with projectId
HostService->>DB: query workspaces for projectId (build knownPaths)
HostService->>FS: safeResolveWorktreePath(projectId, requestedPath)
alt resolved path not under managed root or invalid
HostService-->>Client: error (path traversal or invalid)
else resolved path under managed root
alt path missing on disk
HostService->>FS: mkdirSync(parentDir, {recursive:true})
HostService->>Git: git worktree add / checkout -> create worktree
else path present on disk
HostService->>Git: git checkout / adopt flow
end
HostService->>DB: update or create workspace row as needed
HostService-->>Client: response (branches/status)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 docstrings
🧪 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 |
Greptile SummaryThis PR relocates Superset-managed git worktrees from
Confidence Score: 4/5Safe to merge; the path relocation is correct, backward-compatible, and well-guarded. One minor error-handling improvement would harden the UX but is non-blocking. The core logic is sound: new worktrees land at the correct location, legacy worktrees are preserved via the DB knownPaths lookup, orphan detection at the new root works correctly, and path traversal protection is maintained. worktreePath is NOT NULL in the schema so no null-handling issues. The only actionable concern is that the three mkdirSync calls have no error handling, which would produce raw Node errors on permission/disk failures rather than structured TRPCErrors — a P2 polish issue that does not affect the primary flow. No files require special attention beyond the three unguarded mkdirSync calls in workspace-creation.ts.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts | Relocates v2 worktrees from <repo>/.worktrees/<branch> to ~/.superset/worktrees/<projectId>/<branch>, adds DB-backed orphan detection in listWorktreeBranches, and adds mkdirSync for the new nested directory structure; backward-compatible with legacy worktrees via knownPaths DB lookup |
Sequence Diagram
sequenceDiagram
participant Client
participant Router as workspaceCreationRouter
participant FS as FileSystem
participant Git
participant DB as SQLite (workspaces)
participant Cloud as Cloud API
Client->>Router: create / checkout (projectId, branch)
Router->>Router: safeResolveWorktreePath(projectId, branch)<br/>→ ~/.superset/worktrees/<projectId>/<branch>
Router->>FS: mkdirSync(dirname(worktreePath), { recursive: true })
Router->>Git: git worktree add <worktreePath> <branch>
Router->>Cloud: ensureV2Host + v2Workspace.create
Router->>DB: INSERT workspaces (worktreePath = newPath)
Router->>Client: { workspace, terminals, warnings }
Client->>Router: searchBranches / adopt (projectId)
Router->>DB: SELECT worktreePath WHERE projectId = ? → knownPaths
Router->>Git: git worktree list --porcelain
loop each worktree entry
Router->>Router: if knownPaths.has(path) OR path.startsWith(managedRoot/)<br/>→ add to worktreeMap
end
Router->>Client: { worktreeMap, checkedOutBranches }
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
Line: 789
Comment:
**Unhandled `mkdirSync` failure**
`mkdirSync` can throw (e.g. permission denied, disk full, a file already exists at that path) and is called outside any try/catch here. If it throws, the mutation will propagate a raw `EACCES`/`ENOSPC`/`EEXIST` Node error to the client rather than a structured `TRPCError`. The same pattern is repeated at lines 1133 and 1259.
Consider wrapping each call so failures surface with a clear, user-friendly message:
```ts
try {
mkdirSync(dirname(worktreePath), { recursive: true });
} catch (err) {
clearProgress(input.pendingId);
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message: `Failed to create worktree directory: ${err instanceof Error ? err.message : String(err)}`,
});
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(host-service): place worktrees under..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| localProject.id, | ||
| branchName, | ||
| ); | ||
| mkdirSync(dirname(worktreePath), { recursive: true }); |
There was a problem hiding this comment.
mkdirSync failure
mkdirSync can throw (e.g. permission denied, disk full, a file already exists at that path) and is called outside any try/catch here. If it throws, the mutation will propagate a raw EACCES/ENOSPC/EEXIST Node error to the client rather than a structured TRPCError. The same pattern is repeated at lines 1133 and 1259.
Consider wrapping each call so failures surface with a clear, user-friendly message:
try {
mkdirSync(dirname(worktreePath), { recursive: true });
} catch (err) {
clearProgress(input.pendingId);
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message: `Failed to create worktree directory: ${err instanceof Error ? err.message : String(err)}`,
});
}Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
Line: 789
Comment:
**Unhandled `mkdirSync` failure**
`mkdirSync` can throw (e.g. permission denied, disk full, a file already exists at that path) and is called outside any try/catch here. If it throws, the mutation will propagate a raw `EACCES`/`ENOSPC`/`EEXIST` Node error to the client rather than a structured `TRPCError`. The same pattern is repeated at lines 1133 and 1259.
Consider wrapping each call so failures surface with a clear, user-friendly message:
```ts
try {
mkdirSync(dirname(worktreePath), { recursive: true });
} catch (err) {
clearProgress(input.pendingId);
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message: `Failed to create worktree directory: ${err instanceof Error ? err.message : String(err)}`,
});
}
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts (1)
202-226:⚠️ Potential issue | 🟠 MajorNormalize Git and Node.js paths before ownership checks.
currentPathfromgit worktree list --porcelainuses forward slashes on Windows, while Node.jspath.resolve()produces backslashes. The direct comparisonsknownPaths.has(currentPath)andcurrentPath.startsWith(managedRoot + sep)will fail on Windows, causing Superset-managed worktrees to be missed.Proposed normalization
-import { dirname, join, resolve, sep } from "node:path"; +import { dirname, join, normalize, resolve, sep } from "node:path";function projectWorktreesRoot(projectId: string): string { return resolve(supersetWorktreesRoot(), projectId); } + +function pathKey(path: string): string { + const normalized = normalize(resolve(path)); + return process.platform === "win32" ? normalized.toLowerCase() : normalized; +} function safeResolveWorktreePath(- const managedRoot = projectWorktreesRoot(projectId); + const managedRootKey = pathKey(projectWorktreesRoot(projectId)); const knownPaths = new Set<string>( ctx.db .select({ path: workspaces.worktreePath }) .from(workspaces) .where(eq(workspaces.projectId, projectId)) .all() - .map((w) => w.path), + .map((w) => pathKey(w.path)), ); @@ } else if (line.startsWith("branch refs/heads/") && currentPath) { const branch = line.slice("branch refs/heads/".length).trim(); if (!branch) continue; checkedOutBranches.add(branch); + const currentPathKey = pathKey(currentPath); if ( - knownPaths.has(currentPath) || - currentPath.startsWith(managedRoot + sep) + knownPaths.has(currentPathKey) || + currentPathKey.startsWith(managedRootKey + sep) ) { worktreeMap.set(branch, currentPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts` around lines 202 - 226, currentPath from git.raw can contain POSIX slashes on Windows so comparisons against Node paths fail; normalize paths before checks by creating a normalizedCurrentPath and normalizedManagedRoot (use path.normalize and also replace '/' with path.sep or use path.resolve) and then use normalizedCurrentPath in knownPaths.has(...) and normalizedCurrentPath.startsWith(normalizedManagedRoot + sep); also use normalizedCurrentPath when inserting into worktreeMap or any subsequent path comparisons so currentPath, managedRoot, knownPaths, and the startsWith check are all using the same normalized form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 202-226: currentPath from git.raw can contain POSIX slashes on
Windows so comparisons against Node paths fail; normalize paths before checks by
creating a normalizedCurrentPath and normalizedManagedRoot (use path.normalize
and also replace '/' with path.sep or use path.resolve) and then use
normalizedCurrentPath in knownPaths.has(...) and
normalizedCurrentPath.startsWith(normalizedManagedRoot + sep); also use
normalizedCurrentPath when inserting into worktreeMap or any subsequent path
comparisons so currentPath, managedRoot, knownPaths, and the startsWith check
are all using the same normalized form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e68ca10e-6d90-466d-ad2a-d09ec40344db
📒 Files selected for processing (1)
packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…ind-where-were-creating-new-worktree-into-vs-the-pattern-we-had-in-v1 # Conflicts: # packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
Kitenite
deleted the
in-the-v2-version-please-find-where-were-creating-new-worktree-into-vs-the-pattern-we-had-in-v1
branch
1 participant
Summary
<repo>/.worktrees/<branch>; switch to~/.superset/worktrees/<projectId>/<branch>to mirror v1 desktop and match v2's existing~/.superset/repos/<projectId>layout.listWorktreeBranchesnow detects Superset-managed worktrees via the localworkspacestable (plus the new root, for orphan adoption) instead of a hardcoded path prefix.<repo>/.worktrees/*stay in place and keep working because theirworkspaces.worktreePathrows still match in detection. Only new worktrees use the new location.Test plan
~/.superset/worktrees/<projectId>/<branch>.<repo>/.worktrees/<branch>— verify they still show in the Worktree tab and can be opened.Summary by cubic
Move v2
host-serviceworktrees to~/.superset/worktrees/<projectId>/<branch>to mirror v1 and match~/.superset/repos/<projectId>, keeping them outside the repo. Legacy worktrees still work; no migration needed.workspacestable and the new managed root (to surface orphans for adoption).projectId; update worktree discovery and adoption accordingly.Written for commit c822cb0. Summary will update on new commits.
Summary by CodeRabbit
Chores
Documentation