[codex] Add configurable worktree locations#4887
Merged
Merged
Conversation
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:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds host- and project-level worktree base directory and branch-prefix configuration: DB migration and Drizzle schema, tRPC routers and hooks, path normalization utilities, branch-prefix resolution and tests, workspace creation integration, renderer UI components (pickers/selectors), settings registry updates, and integration tests. ChangesConfiguration Infrastructure and APIs
Renderer UI Components and Settings Pages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 10 individual chapters for you: 
Chapters generated by Stage for commit acf777d on May 28, 2026 1:23am UTC. |
Contributor
Greptile SummaryThis PR adds configurable worktree base directories at user, host, and project levels. It introduces a new
Confidence Score: 3/5The workspace creation path is correct and well-tested, but two backend issues need attention: a path traversal gap in tilde expansion and a missing server-side ownership check on the host-level settings mutation. Tilde expansion in
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/settings/worktree-location.ts | New settings router for host-level worktree base dir with legacy env var seeding; set has no server-side ownership check. |
| packages/host-service/src/trpc/router/project/project.ts | Adds setWorktreeBaseDir mutation; update runs before existence check and no authorization is enforced. |
| packages/host-service/src/trpc/router/workspace-creation/shared/worktree-paths.ts | Adds normalizeWorktreeBaseDir; tilde expansion does not block ~/../../ traversal. |
| packages/host-service/src/trpc/router/workspaces/workspaces.ts | Threads effective worktree base dir through workspace creation correctly. |
| packages/host-service/test/integration/workspace-create-delete.integration.test.ts | Three new integration tests for host config, legacy seeding, and project override; clean and well-structured. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/host-service/src/trpc/router/settings/worktree-location.ts:82-98
**Missing server-side ownership check on host settings mutation**
`worktreeLocationRouter.set` is guarded only by `protectedProcedure` (authentication), not by host ownership. The UI correctly limits the button to owners via `canEdit={isOwner}`, but any authenticated member of the host service can call this endpoint directly and change the host-wide worktree base dir — affecting where new workspaces land for every project on that host. The same gap applies to `project.setWorktreeBaseDir` in `project.ts`.
### Issue 2 of 3
packages/host-service/src/trpc/router/workspace-creation/shared/worktree-paths.ts:17-22
**Tilde expansion does not block upward traversal sequences**
`normalize(join(homedir(), rest))` resolves `..` segments in the path, so an input like `~/../../etc` is accepted and stored as `/etc` (or similar). A host owner (or any authenticated member if the server-side ownership check is absent) can redirect workspace creation for all members to an unexpected path on the host filesystem.
```suggestion
if (trimmed.startsWith("~")) {
const rest = trimmed.slice(1);
if (rest === "" || rest.startsWith("/") || rest.startsWith("\\")) {
const expanded = resolve(join(homedir(), rest));
if (!expanded.startsWith(homedir() + sep)) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Worktree location must not traverse above the home directory",
});
}
return expanded;
}
}
```
### Issue 3 of 3
packages/host-service/src/trpc/router/project/project.ts:71-97
**Update runs before project existence check**
The `UPDATE` executes unconditionally; if the `projectId` doesn't exist it silently writes zero rows, and only the subsequent `findFirst` discovers the gap and throws `NOT_FOUND`. Swapping the order — guard first, update only when the project is confirmed — is the safer pattern and avoids a wasted write on bad input.
```suggestion
.mutation(({ ctx, input }) => {
const worktreeBaseDir = normalizeWorktreeBaseDir(input.path);
const project = ctx.db.query.projects
.findFirst({ where: eq(projects.id, input.projectId) })
.sync();
if (!project) {
throw new TRPCError({
code: "NOT_FOUND",
message: "Project is not set up on this host",
});
}
ctx.db
.update(projects)
.set({ worktreeBaseDir })
.where(eq(projects.id, input.projectId))
.run();
return {
id: project.id,
worktreeBaseDir: worktreeBaseDir ?? null,
effectiveWorktreeBaseDir: getEffectiveWorktreeBaseDir({
ctx,
projectWorktreeBaseDir: worktreeBaseDir,
}),
};
}),
```
Reviews (1): Last reviewed commit: "Add configurable worktree locations" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+82
to
+98
| set: protectedProcedure | ||
| .input(z.object({ path: z.string().nullable() })) | ||
| .mutation(({ ctx, input }) => { | ||
| const worktreeBaseDir = normalizeWorktreeBaseDir(input.path); | ||
| ctx.db | ||
| .insert(hostSettings) | ||
| .values({ | ||
| id: HOST_SETTINGS_ID, | ||
| worktreeBaseDir, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: hostSettings.id, | ||
| set: { worktreeBaseDir }, | ||
| }) | ||
| .run(); | ||
| return toOutput(worktreeBaseDir); | ||
| }), |
Contributor
There was a problem hiding this comment.
worktreeLocationRouter.set is guarded only by protectedProcedure (authentication), not by host ownership. The UI correctly limits the button to owners via canEdit={isOwner}, but any authenticated member of the host service can call this endpoint directly and change the host-wide worktree base dir — affecting where new workspaces land for every project on that host. The same gap applies to project.setWorktreeBaseDir in project.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/settings/worktree-location.ts
Line: 82-98
Comment:
**Missing server-side ownership check on host settings mutation**
`worktreeLocationRouter.set` is guarded only by `protectedProcedure` (authentication), not by host ownership. The UI correctly limits the button to owners via `canEdit={isOwner}`, but any authenticated member of the host service can call this endpoint directly and change the host-wide worktree base dir — affecting where new workspaces land for every project on that host. The same gap applies to `project.setWorktreeBaseDir` in `project.ts`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+17
to
+22
| if (trimmed.startsWith("~")) { | ||
| const rest = trimmed.slice(1); | ||
| if (rest === "" || rest.startsWith("/") || rest.startsWith("\\")) { | ||
| return normalize(join(homedir(), rest)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
normalize(join(homedir(), rest)) resolves .. segments in the path, so an input like ~/../../etc is accepted and stored as /etc (or similar). A host owner (or any authenticated member if the server-side ownership check is absent) can redirect workspace creation for all members to an unexpected path on the host filesystem.
Suggested change
| if (trimmed.startsWith("~")) { | |
| const rest = trimmed.slice(1); | |
| if (rest === "" || rest.startsWith("/") || rest.startsWith("\\")) { | |
| return normalize(join(homedir(), rest)); | |
| } | |
| } | |
| if (trimmed.startsWith("~")) { | |
| const rest = trimmed.slice(1); | |
| if (rest === "" || rest.startsWith("/") || rest.startsWith("\\")) { | |
| const expanded = resolve(join(homedir(), rest)); | |
| if (!expanded.startsWith(homedir() + sep)) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Worktree location must not traverse above the home directory", | |
| }); | |
| } | |
| return expanded; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/shared/worktree-paths.ts
Line: 17-22
Comment:
**Tilde expansion does not block upward traversal sequences**
`normalize(join(homedir(), rest))` resolves `..` segments in the path, so an input like `~/../../etc` is accepted and stored as `/etc` (or similar). A host owner (or any authenticated member if the server-side ownership check is absent) can redirect workspace creation for all members to an unexpected path on the host filesystem.
```suggestion
if (trimmed.startsWith("~")) {
const rest = trimmed.slice(1);
if (rest === "" || rest.startsWith("/") || rest.startsWith("\\")) {
const expanded = resolve(join(homedir(), rest));
if (!expanded.startsWith(homedir() + sep)) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Worktree location must not traverse above the home directory",
});
}
return expanded;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+71
to
+97
| .mutation(({ ctx, input }) => { | ||
| const worktreeBaseDir = normalizeWorktreeBaseDir(input.path); | ||
| ctx.db | ||
| .update(projects) | ||
| .set({ worktreeBaseDir }) | ||
| .where(eq(projects.id, input.projectId)) | ||
| .run(); | ||
|
|
||
| const project = ctx.db.query.projects | ||
| .findFirst({ where: eq(projects.id, input.projectId) }) | ||
| .sync(); | ||
| if (!project) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Project is not set up on this host", | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| id: project.id, | ||
| worktreeBaseDir: project.worktreeBaseDir ?? null, | ||
| effectiveWorktreeBaseDir: getEffectiveWorktreeBaseDir({ | ||
| ctx, | ||
| projectWorktreeBaseDir: project.worktreeBaseDir, | ||
| }), | ||
| }; | ||
| }), |
Contributor
There was a problem hiding this comment.
The UPDATE executes unconditionally; if the projectId doesn't exist it silently writes zero rows, and only the subsequent findFirst discovers the gap and throws NOT_FOUND. Swapping the order — guard first, update only when the project is confirmed — is the safer pattern and avoids a wasted write on bad input.
Suggested change
| .mutation(({ ctx, input }) => { | |
| const worktreeBaseDir = normalizeWorktreeBaseDir(input.path); | |
| ctx.db | |
| .update(projects) | |
| .set({ worktreeBaseDir }) | |
| .where(eq(projects.id, input.projectId)) | |
| .run(); | |
| const project = ctx.db.query.projects | |
| .findFirst({ where: eq(projects.id, input.projectId) }) | |
| .sync(); | |
| if (!project) { | |
| throw new TRPCError({ | |
| code: "NOT_FOUND", | |
| message: "Project is not set up on this host", | |
| }); | |
| } | |
| return { | |
| id: project.id, | |
| worktreeBaseDir: project.worktreeBaseDir ?? null, | |
| effectiveWorktreeBaseDir: getEffectiveWorktreeBaseDir({ | |
| ctx, | |
| projectWorktreeBaseDir: project.worktreeBaseDir, | |
| }), | |
| }; | |
| }), | |
| .mutation(({ ctx, input }) => { | |
| const worktreeBaseDir = normalizeWorktreeBaseDir(input.path); | |
| const project = ctx.db.query.projects | |
| .findFirst({ where: eq(projects.id, input.projectId) }) | |
| .sync(); | |
| if (!project) { | |
| throw new TRPCError({ | |
| code: "NOT_FOUND", | |
| message: "Project is not set up on this host", | |
| }); | |
| } | |
| ctx.db | |
| .update(projects) | |
| .set({ worktreeBaseDir }) | |
| .where(eq(projects.id, input.projectId)) | |
| .run(); | |
| return { | |
| id: project.id, | |
| worktreeBaseDir: worktreeBaseDir ?? null, | |
| effectiveWorktreeBaseDir: getEffectiveWorktreeBaseDir({ | |
| ctx, | |
| projectWorktreeBaseDir: worktreeBaseDir, | |
| }), | |
| }; | |
| }), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/project/project.ts
Line: 71-97
Comment:
**Update runs before project existence check**
The `UPDATE` executes unconditionally; if the `projectId` doesn't exist it silently writes zero rows, and only the subsequent `findFirst` discovers the gap and throws `NOT_FOUND`. Swapping the order — guard first, update only when the project is confirmed — is the safer pattern and avoids a wasted write on bad input.
```suggestion
.mutation(({ ctx, input }) => {
const worktreeBaseDir = normalizeWorktreeBaseDir(input.path);
const project = ctx.db.query.projects
.findFirst({ where: eq(projects.id, input.projectId) })
.sync();
if (!project) {
throw new TRPCError({
code: "NOT_FOUND",
message: "Project is not set up on this host",
});
}
ctx.db
.update(projects)
.set({ worktreeBaseDir })
.where(eq(projects.id, input.projectId))
.run();
return {
id: project.id,
worktreeBaseDir: worktreeBaseDir ?? null,
effectiveWorktreeBaseDir: getEffectiveWorktreeBaseDir({
ctx,
projectWorktreeBaseDir: worktreeBaseDir,
}),
};
}),
```
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/trpc/router/settings/worktree-location.ts`:
- Around line 48-63: The lazy initialization in the settings read path can race:
two requests may both see no row and both execute the insert for hostSettings
with HOST_SETTINGS_ID causing a PK conflict; to fix, make the insertion
idempotent and conflict-safe by changing the logic around ctx.db.select/insert
to either perform an upsert/insert...on conflict do nothing (then re-select) or
wrap the check+insert in a DB transaction/lock so only one insert succeeds; use
the existing symbols hostSettings, HOST_SETTINGS_ID, ctx.db and
getLegacyWorktreeBaseDir() to locate and replace the current select/insert
sequence with an atomic upsert or a retry-after-no-op-insert-then-select flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45cc498f-169d-4540-be9d-c4e3e27b2cf2
📒 Files selected for processing (24)
apps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/V2WorktreeLocationPicker/V2WorktreeLocationPicker.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/V2WorktreeLocationPicker/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/git/components/GitSettings/GitSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/git/components/GitSettings/components/UserWorktreeLocationSection/UserWorktreeLocationSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/git/components/GitSettings/components/UserWorktreeLocationSection/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/hosts/$hostId/components/HostSettings/HostSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/hosts/$hostId/components/HostSettings/components/WorktreeLocationSection/WorktreeLocationSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/hosts/$hostId/components/HostSettings/components/WorktreeLocationSection/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.tsapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/WorktreeLocationSection/WorktreeLocationSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/WorktreeLocationSection/index.tspackages/host-service/drizzle/0005_big_mach_iv.sqlpackages/host-service/drizzle/meta/0005_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/settings/index.tspackages/host-service/src/trpc/router/settings/worktree-location.tspackages/host-service/src/trpc/router/workspace-creation/shared/worktree-paths.tspackages/host-service/src/trpc/router/workspaces/workspaces.tspackages/host-service/test/integration/workspace-create-delete.integration.test.ts
Comment on lines
+48
to
+63
| const existing = ctx.db | ||
| .select({ worktreeBaseDir: hostSettings.worktreeBaseDir }) | ||
| .from(hostSettings) | ||
| .where(eq(hostSettings.id, HOST_SETTINGS_ID)) | ||
| .get(); | ||
| if (existing) return existing.worktreeBaseDir ?? null; | ||
|
|
||
| const legacyWorktreeBaseDir = getLegacyWorktreeBaseDir(); | ||
| ctx.db | ||
| .insert(hostSettings) | ||
| .values({ | ||
| id: HOST_SETTINGS_ID, | ||
| worktreeBaseDir: legacyWorktreeBaseDir, | ||
| }) | ||
| .run(); | ||
| return legacyWorktreeBaseDir; |
Contributor
There was a problem hiding this comment.
Make lazy initialization conflict-safe.
Two concurrent requests can both miss the row check and then both insert id = 1, causing a primary-key conflict on a read path.
💡 Suggested fix
export function getHostWorktreeBaseDir(
ctx: Pick<HostServiceContext, "db">,
): string | null {
const existing = ctx.db
.select({ worktreeBaseDir: hostSettings.worktreeBaseDir })
.from(hostSettings)
.where(eq(hostSettings.id, HOST_SETTINGS_ID))
.get();
if (existing) return existing.worktreeBaseDir ?? null;
const legacyWorktreeBaseDir = getLegacyWorktreeBaseDir();
ctx.db
.insert(hostSettings)
.values({
id: HOST_SETTINGS_ID,
worktreeBaseDir: legacyWorktreeBaseDir,
})
+ .onConflictDoNothing({ target: hostSettings.id })
.run();
- return legacyWorktreeBaseDir;
+ const current = ctx.db
+ .select({ worktreeBaseDir: hostSettings.worktreeBaseDir })
+ .from(hostSettings)
+ .where(eq(hostSettings.id, HOST_SETTINGS_ID))
+ .get();
+ return current?.worktreeBaseDir ?? legacyWorktreeBaseDir ?? null;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/trpc/router/settings/worktree-location.ts` around
lines 48 - 63, The lazy initialization in the settings read path can race: two
requests may both see no row and both execute the insert for hostSettings with
HOST_SETTINGS_ID causing a PK conflict; to fix, make the insertion idempotent
and conflict-safe by changing the logic around ctx.db.select/insert to either
perform an upsert/insert...on conflict do nothing (then re-select) or wrap the
check+insert in a DB transaction/lock so only one insert succeeds; use the
existing symbols hostSettings, HOST_SETTINGS_ID, ctx.db and
getLegacyWorktreeBaseDir() to locate and replace the current select/insert
sequence with an atomic upsert or a retry-after-no-op-insert-then-select flow.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 24 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Extracts useSetV2WorktreeBaseDir so the user/host sections share one mutation hook, and drops the now-unused effectiveWorktreeBaseDir field plus the getEffectiveWorktreeBaseDir helper — callers can resolve with ?? getHostWorktreeBaseDir(ctx).
The V2 Project settings page already had an inline dropdown for switching between the hosts a project lives on; lift it into a shared HostSelect under settings/components and use it in the Git settings Worktree location section. When the user has access to more than one device, the section now picks which device the setting applies to instead of being implicitly the local machine.
- Cut the 5-min GitHub-username cache in the host-service resolver. Workspace creates aren't a hot path and the cache was a stale-account footgun on `gh auth switch`. The renderer preview already has a 5-min `staleTime`. - Single source for `BranchPrefixMode` — `@superset/local-db` re-exports from `@superset/shared/workspace-launch`. No more drift risk between the two. - Extract `BranchPrefixControl` used by both `V2GitSettings` (host-wide) and `BranchPrefixSection` (per-project, `showDefault`). Empty custom-prefix on blur no longer fires a misleading `mode=custom, customPrefix=null` write.
…ngsRow Mirror the V2ProjectSettings host-targeting pattern in V2GitSettings: header + device picker on the right (shown only when 2+ devices), SettingsRow body, all branchPrefix tRPC calls routed through useHostUrl(hostId) so the chosen host's setting is what's edited. Add ?hostId= search param to the /settings/git route. Promote SettingsRow from v2-project-only to settings/components so both pages share it.
The inline Select markup mirrored what HostSelect already does. Drop the duplicate options interface and icon/dot rendering — V2GitSettings now uses the same component V2ProjectSettings does.
Worktree-location and branch-prefix both add columns to the same host_settings row + projects table. Replace the original 0005_big_mach_iv migration with a single 0005_host_settings_and_project_overrides that applies all four columns at once, so anyone applying this PR's migrations gets one statement, not two.
…prefix V2GitSettings was only rendering the branch-prefix row, so once the route started serving the v2 page, the worktree-location knob from #4887 disappeared from the UI. Wire V2WorktreeLocationPicker into the same SettingsRow layout — both rows now share the page's device picker. (Side effect: UserWorktreeLocationSection's V2Body is no longer reached from any v2 path. Leaving it for a follow-up clean-up so this commit stays focused on the missing-row fix.)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts (1)
1093-1093: ⚡ Quick winConsider more generic description for cross-variant setting.
The description "Override the host worktree directory for this project" uses v2-specific terminology ("host worktree directory"), but this setting is marked as "shared" (line 178), meaning it appears in both v1 and v2 UIs. In v1, there is no host-level setting—only a user-level worktree location. A more generic description would be clearer for both variants, such as: "Override the default worktree directory for this project".
Suggested description
- description: "Override the host worktree directory for this project", + description: "Override the default worktree directory for this project",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts` at line 1093, The description for the shared setting currently reads "Override the host worktree directory for this project" which is v2-specific; update the description string in settings-search.ts for the shared setting (the entry whose description currently equals "Override the host worktree directory for this project") to a generic phrase such as "Override the default worktree directory for this project" so it is appropriate for both v1 and v2 UIs.plans/v2-branch-prefixes-design.md (2)
1-4: 💤 Low valueDocument scope narrower than PR scope.
This document is titled "V2 configurable branch prefixes" and focuses exclusively on that feature, but the PR ("[codex] Add configurable worktree locations") implements both worktree base directory configuration and branch prefixes. Consider either:
- Updating the title to reflect both features
- Adding a note clarifying this doc covers only the branch-prefix aspect
- Creating a companion design doc for the worktree-location feature
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/v2-branch-prefixes-design.md` around lines 1 - 4, This design doc titled "V2 configurable branch prefixes" currently documents only branch-prefix behavior while the associated PR ([codex] Add configurable worktree locations, Ticket SUPER-835, branch configurable-branch-prefi) also implements configurable worktree base directories; update the doc to either (a) change the title to something like "V2 configurable branch prefixes and worktree locations", or (b) add a clear scope section or note at the top stating the doc covers only branch-prefixes and link to a new companion design doc for worktree-location, and if choosing (b) create a separate design doc that documents the worktree base directory design and link both docs together for clarity.
107-114: 💤 Low valuetRPC surface table is incomplete.
This table lists only the branch-prefix endpoints but omits the worktree-location endpoints also added in this PR (
settings.worktreeLocation.get,settings.worktreeLocation.set,project.setWorktreeBaseDir). Consider documenting the complete tRPC surface or clarifying that this table shows only branch-prefix procedures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/v2-branch-prefixes-design.md` around lines 107 - 114, The tRPC surface table currently lists only branch-prefix procedures; update it to either include the worktree-location procedures or clarify it's partial: add rows for settings.worktreeLocation.get, settings.worktreeLocation.set, and project.setWorktreeBaseDir (with their return/argument shapes), or add a note above the table stating it documents only branch-prefix endpoints and point to where worktree endpoints are described; ensure the unique symbols settings.worktreeLocation.get, settings.worktreeLocation.set, and project.setWorktreeBaseDir are referenced so readers can locate the related implementations.packages/host-service/src/trpc/router/workspace-creation/utils/branch-prefix.test.ts (1)
56-140: ⚡ Quick winAdd one test for
githubmode resolution/fallback.This suite is solid, but it currently misses the
githubmode path (including fallback behavior when GH username is unavailable). Adding one focused case would lock down the new precedence logic better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/trpc/router/workspace-creation/utils/branch-prefix.test.ts` around lines 56 - 140, Add a test in the existing describe("resolveProjectBranchPrefix") block that exercises the "github" mode and its fallback: call resolveProjectBranchPrefix twice using makeCtx/createTestDb and makeProject({ branchPrefixMode: "github" }) — first with a git fixture that exposes a GitHub username (use the existing git helper that can provide a github username, e.g. gitWithGithub("octocat") or similar) and expect the result to equal the formatted GitHub name ("octocat" or formatted variant), then call it again with the git fixture lacking a GitHub username but having an author name (gitWithGithub(null) / gitWithAuthor("Jane Doe")) and expect it to fall back to the author-derived prefix ("Jane-Doe"); reference resolveProjectBranchPrefix, makeProject, gitWithGithub/gitWithAuthor to locate the right helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/git/components/V2GitSettings/V2GitSettings.tsx`:
- Around line 89-103: The branch-prefix queries are still running and the
control remains enabled even when the selected host is offline; update
branchPrefixQuery's enabled predicate to also require the host be online (e.g.,
enabled: !!targetHostUrl && gitInfoQuery.data?.online or gitInfoQuery.isSuccess)
so reads/writes don't run for offline hosts, and likewise guard the UI control
(the input/toggle rendered around lines referencing the branch prefix) so it is
disabled when gitInfoQuery indicates the host is offline (use gitInfoQuery.data
or gitInfoQuery.isSuccess/isLoading to determine online status); reference
branchPrefixQuery, gitInfoQuery, targetHostUrl and getHostServiceClientByUrl to
locate the code to change.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/git/page.tsx`:
- Around line 11-13: In validateSearch, normalize hostId so empty or
whitespace-only strings are treated as absent: in the validateSearch function
(the validateSearch: (search: Record<string, unknown>): { hostId?: string } =>
...) check that search.hostId is a string and after trimming it's non-empty; if
trimmed is empty, return hostId as undefined, otherwise return the trimmed
string. Ensure you reference the validateSearch and hostId fields so callers
receive undefined for blank/whitespace values.
In `@packages/host-service/src/db/schema.ts`:
- Around line 55-66: The host_settings table promises a singleton row but the
schema (hostSettings, sqliteTable and its id column defined via
integer().primaryKey().default(1)) doesn't enforce id = 1; add a table-level
CHECK constraint (or column-level check) that enforces id = 1 so no other rows
can be inserted (e.g., add CONSTRAINT check_id_singleton CHECK (id = 1) or
equivalent via your schema builder) and keep the existing primaryKey/default
behavior on id.
In `@plans/v2-branch-prefixes-design.md`:
- Around line 55-63: Update the SQL snippet to include the missing
worktree_base_dir column in both host_settings and projects so the docs match
the actual migration/schema.ts; specifically, add worktree_base_dir (alongside
branch_prefix_mode and branch_prefix_custom) to the CREATE TABLE host_settings
block and add ALTER TABLE projects ADD worktree_base_dir text; so the symbols
worktree_base_dir, branch_prefix_mode, branch_prefix_custom, host_settings and
projects align with schema.ts.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts`:
- Line 1093: The description for the shared setting currently reads "Override
the host worktree directory for this project" which is v2-specific; update the
description string in settings-search.ts for the shared setting (the entry whose
description currently equals "Override the host worktree directory for this
project") to a generic phrase such as "Override the default worktree directory
for this project" so it is appropriate for both v1 and v2 UIs.
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/branch-prefix.test.ts`:
- Around line 56-140: Add a test in the existing
describe("resolveProjectBranchPrefix") block that exercises the "github" mode
and its fallback: call resolveProjectBranchPrefix twice using
makeCtx/createTestDb and makeProject({ branchPrefixMode: "github" }) — first
with a git fixture that exposes a GitHub username (use the existing git helper
that can provide a github username, e.g. gitWithGithub("octocat") or similar)
and expect the result to equal the formatted GitHub name ("octocat" or formatted
variant), then call it again with the git fixture lacking a GitHub username but
having an author name (gitWithGithub(null) / gitWithAuthor("Jane Doe")) and
expect it to fall back to the author-derived prefix ("Jane-Doe"); reference
resolveProjectBranchPrefix, makeProject, gitWithGithub/gitWithAuthor to locate
the right helpers.
In `@plans/v2-branch-prefixes-design.md`:
- Around line 1-4: This design doc titled "V2 configurable branch prefixes"
currently documents only branch-prefix behavior while the associated PR ([codex]
Add configurable worktree locations, Ticket SUPER-835, branch
configurable-branch-prefi) also implements configurable worktree base
directories; update the doc to either (a) change the title to something like "V2
configurable branch prefixes and worktree locations", or (b) add a clear scope
section or note at the top stating the doc covers only branch-prefixes and link
to a new companion design doc for worktree-location, and if choosing (b) create
a separate design doc that documents the worktree base directory design and link
both docs together for clarity.
- Around line 107-114: The tRPC surface table currently lists only branch-prefix
procedures; update it to either include the worktree-location procedures or
clarify it's partial: add rows for settings.worktreeLocation.get,
settings.worktreeLocation.set, and project.setWorktreeBaseDir (with their
return/argument shapes), or add a note above the table stating it documents only
branch-prefix endpoints and point to where worktree endpoints are described;
ensure the unique symbols settings.worktreeLocation.get,
settings.worktreeLocation.set, and project.setWorktreeBaseDir are referenced so
readers can locate the related implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72badd78-fc6d-419a-98b8-3074d36104f3
📒 Files selected for processing (25)
apps/desktop/src/renderer/routes/_authenticated/settings/components/BranchPrefixControl/BranchPrefixControl.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/BranchPrefixControl/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsRow/SettingsRow.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsRow/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/git/components/V2GitSettings/V2GitSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/git/components/V2GitSettings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/git/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/BranchPrefixSection/BranchPrefixSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/BranchPrefixSection/index.tspackages/host-service/drizzle/0005_host_settings_and_project_overrides.sqlpackages/host-service/drizzle/meta/0005_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/trpc/router/project/project.tspackages/host-service/src/trpc/router/settings/branch-prefix.tspackages/host-service/src/trpc/router/settings/index.tspackages/host-service/src/trpc/router/workspace-creation/utils/branch-prefix.test.tspackages/host-service/src/trpc/router/workspace-creation/utils/branch-prefix.tspackages/host-service/src/trpc/router/workspaces/workspaces.tspackages/local-db/src/schema/zod.tspackages/shared/src/workspace-launch/branch.tsplans/v2-branch-prefixes-design.mdplans/v2-branch-prefixes.md
✅ Files skipped from review due to trivial changes (5)
- apps/desktop/src/renderer/routes/_authenticated/settings/components/BranchPrefixControl/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/BranchPrefixSection/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/git/components/V2GitSettings/index.ts
- packages/host-service/drizzle/0005_host_settings_and_project_overrides.sql
- packages/host-service/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/host-service/drizzle/meta/0005_snapshot.json
- packages/host-service/src/trpc/router/project/project.ts
Comment on lines
+11
to
+13
| validateSearch: (search: Record<string, unknown>): { hostId?: string } => ({ | ||
| hostId: typeof search.hostId === "string" ? search.hostId : undefined, | ||
| }), |
Contributor
There was a problem hiding this comment.
Normalize empty hostId query values to “absent”.
Line 12 treats hostId="" as valid, which can lead to inconsistent host targeting. Normalize blank/whitespace values to undefined.
💡 Suggested fix
export const Route = createFileRoute("/_authenticated/settings/git/")({
component: GitSettingsPage,
- validateSearch: (search: Record<string, unknown>): { hostId?: string } => ({
- hostId: typeof search.hostId === "string" ? search.hostId : undefined,
- }),
+ validateSearch: (search: Record<string, unknown>): { hostId?: string } => {
+ const hostId =
+ typeof search.hostId === "string" ? search.hostId.trim() : "";
+ return {
+ hostId: hostId.length > 0 ? hostId : undefined,
+ };
+ },
});📝 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
| validateSearch: (search: Record<string, unknown>): { hostId?: string } => ({ | |
| hostId: typeof search.hostId === "string" ? search.hostId : undefined, | |
| }), | |
| export const Route = createFileRoute("/_authenticated/settings/git/")({ | |
| component: GitSettingsPage, | |
| validateSearch: (search: Record<string, unknown>): { hostId?: string } => { | |
| const hostId = | |
| typeof search.hostId === "string" ? search.hostId.trim() : ""; | |
| return { | |
| hostId: hostId.length > 0 ? hostId : undefined, | |
| }; | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/git/page.tsx` around
lines 11 - 13, In validateSearch, normalize hostId so empty or whitespace-only
strings are treated as absent: in the validateSearch function (the
validateSearch: (search: Record<string, unknown>): { hostId?: string } => ...)
check that search.hostId is a string and after trimming it's non-empty; if
trimmed is empty, return hostId as undefined, otherwise return the trimmed
string. Ensure you reference the validateSearch and hostId fields so callers
receive undefined for blank/whitespace values.
Comment on lines
+55
to
+66
| /** | ||
| * Single-row host-wide settings (always `id = 1`). The host-service has no | ||
| * generic settings store yet; this row holds host-wide knobs (worktree base | ||
| * dir, branch-prefix default) that projects fall back to when they have no | ||
| * override of their own. | ||
| */ | ||
| export const hostSettings = sqliteTable("host_settings", { | ||
| id: integer().primaryKey().default(1), | ||
| worktreeBaseDir: text("worktree_base_dir"), | ||
| branchPrefixMode: text("branch_prefix_mode").$type<BranchPrefixMode>(), | ||
| branchPrefixCustom: text("branch_prefix_custom"), | ||
| }); |
Contributor
There was a problem hiding this comment.
Enforce the singleton invariant for host_settings at the schema level.
The comment says this table is always single-row (id = 1), but the schema does not enforce that. If any non-1 row is inserted, callers using unscoped .get() can read the wrong row.
Suggested direction
- export const hostSettings = sqliteTable("host_settings", {
+ export const hostSettings = sqliteTable("host_settings", {
id: integer().primaryKey().default(1),
worktreeBaseDir: text("worktree_base_dir"),
branchPrefixMode: text("branch_prefix_mode").$type<BranchPrefixMode>(),
branchPrefixCustom: text("branch_prefix_custom"),
- });
+ }, (table) => [
+ // enforce singleton semantics at DB level (id must be 1)
+ // add CHECK constraint via migration/schema helper used in this repo
+ ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/db/schema.ts` around lines 55 - 66, The
host_settings table promises a singleton row but the schema (hostSettings,
sqliteTable and its id column defined via integer().primaryKey().default(1))
doesn't enforce id = 1; add a table-level CHECK constraint (or column-level
check) that enforces id = 1 so no other rows can be inserted (e.g., add
CONSTRAINT check_id_singleton CHECK (id = 1) or equivalent via your schema
builder) and keep the existing primaryKey/default behavior on id.
Comment on lines
+55
to
+63
| ```sql | ||
| CREATE TABLE host_settings ( | ||
| id integer PRIMARY KEY DEFAULT 1 NOT NULL, | ||
| branch_prefix_mode text, | ||
| branch_prefix_custom text | ||
| ); | ||
| ALTER TABLE projects ADD branch_prefix_mode text; | ||
| ALTER TABLE projects ADD branch_prefix_custom text; | ||
| ``` |
Contributor
There was a problem hiding this comment.
SQL schema snippet is incomplete.
The schema shown omits the worktree_base_dir column that is also added to both host_settings and projects tables in the same migration. According to the actual schema (context snippet from schema.ts), both tables include worktreeBaseDir alongside the branch-prefix fields. Since this PR adds both configurable worktree locations and branch prefixes, the documentation should reflect the complete schema.
📝 Complete schema representation
CREATE TABLE host_settings (
id integer PRIMARY KEY DEFAULT 1 NOT NULL,
worktree_base_dir text,
branch_prefix_mode text,
branch_prefix_custom text
);
ALTER TABLE projects ADD worktree_base_dir text;
ALTER TABLE projects ADD branch_prefix_mode text;
ALTER TABLE projects ADD branch_prefix_custom text;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/v2-branch-prefixes-design.md` around lines 55 - 63, Update the SQL
snippet to include the missing worktree_base_dir column in both host_settings
and projects so the docs match the actual migration/schema.ts; specifically, add
worktree_base_dir (alongside branch_prefix_mode and branch_prefix_custom) to the
CREATE TABLE host_settings block and add ALTER TABLE projects ADD
worktree_base_dir text; so the symbols worktree_base_dir, branch_prefix_mode,
branch_prefix_custom, host_settings and projects align with schema.ts.
Contributor
There was a problem hiding this comment.
4 issues found across 26 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="packages/host-service/src/trpc/router/settings/branch-prefix.ts">
<violation number="1" location="packages/host-service/src/trpc/router/settings/branch-prefix.ts:18">
P2: Read the host settings row by `id = 1` instead of unfiltered `.get()` to avoid returning the wrong settings record.</violation>
<violation number="2" location="packages/host-service/src/trpc/router/settings/branch-prefix.ts:30">
P2: Validate that `customPrefix` is present and non-empty when `mode` is `custom` to prevent inconsistent branch-prefix settings.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| .input( | ||
| z.object({ | ||
| mode: z.enum(BRANCH_PREFIX_MODES), | ||
| customPrefix: z.string().nullable().optional(), |
Contributor
There was a problem hiding this comment.
P2: Validate that customPrefix is present and non-empty when mode is custom to prevent inconsistent branch-prefix settings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/settings/branch-prefix.ts, line 30:
<comment>Validate that `customPrefix` is present and non-empty when `mode` is `custom` to prevent inconsistent branch-prefix settings.</comment>
<file context>
@@ -0,0 +1,59 @@
+ .input(
+ z.object({
+ mode: z.enum(BRANCH_PREFIX_MODES),
+ customPrefix: z.string().nullable().optional(),
+ }),
+ )
</file context>
| export const branchPrefixRouter = router({ | ||
| /** The host-wide default. `none` when never configured. */ | ||
| get: protectedProcedure.query(({ ctx }) => { | ||
| const row = ctx.db.select().from(hostSettings).get(); |
Contributor
There was a problem hiding this comment.
P2: Read the host settings row by id = 1 instead of unfiltered .get() to avoid returning the wrong settings record.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/settings/branch-prefix.ts, line 18:
<comment>Read the host settings row by `id = 1` instead of unfiltered `.get()` to avoid returning the wrong settings record.</comment>
<file context>
@@ -0,0 +1,59 @@
+export const branchPrefixRouter = router({
+ /** The host-wide default. `none` when never configured. */
+ get: protectedProcedure.query(({ ctx }) => {
+ const row = ctx.db.select().from(hostSettings).get();
+ return {
+ mode: (row?.branchPrefixMode ?? "none") satisfies BranchPrefixMode,
</file context>
Avoids failing fetches and error toasts when the picker targets an offline remote host; mirrors the existing worktree-query gating.
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
HostSelectdevice picker on/settings/git, project, and host settings; render worktree-location row alongside branch prefix in v2 Git settingsImpact
Users can choose where new worktree workspaces are created and customize branch prefixes at the global/user, host, and project levels. v2 workspaces now use host-service settings instead of only the legacy desktop local setting.
Validation
bun test apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.tsbun test packages/host-service/test/integration/workspace-create-delete.integration.test.tsbun test packages/host-service/src/trpc/router/workspaces/workspace-creation/utils/branch-prefix.test.tsbun run --cwd apps/desktop typecheckbun run lint:fixbun run lintSummary by cubic
Adds configurable worktree locations and branch prefixes for v2 workspaces, with host defaults and per-project overrides applied during workspace creation. The Git settings page now targets any device with a unified picker and seeds the legacy worktree location.
New Features
host_settingsandprojects.worktree_base_dir; TRPCsettings.worktreeLocation.get/set,project.setWorktreeBaseDir; resolution order is project override → host setting →~/.superset/worktrees; host is auto-seeded fromSUPERSET_LEGACY_WORKTREE_BASE_DIR. V2 UI adds a remote path picker; Host and Project settings include Worktrees;/settings/gitsupports?hostIdand shows both Worktree Location and Branch Prefix.none/github/author/custom, with a host-wide default and optional per-project override; TRPCsettings.branchPrefix.get/set,project.setBranchPrefix; applied only to new branches (collision-safe) byworkspace.create. Single source of truth forBranchPrefixModein@superset/shared/workspace-launch; sharedBranchPrefixControlandHostSelect. Settings search keeps the Git tab visible in v2 and adds a host worktree item.Bug Fixes
Written for commit acf777d. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
New Features
Tests
Documentation