Lane Branch Selector#208
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR introduces lane-scoped branch switching with multi-step preview and confirmation workflow. Changes span CLI, IPC, git operations, lane services, database persistence, and both desktop and iOS UI layers to support checking out existing branches or creating new branches with configurable start points and base references, including active-work acknowledgment handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 26, 2026 22:46
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+779
to
+798
| db.run(` | ||
| create table if not exists lane_branch_profiles ( | ||
| id text primary key, | ||
| project_id text not null, | ||
| lane_id text not null, | ||
| branch_ref text not null, | ||
| normalized_branch_ref text not null, | ||
| base_ref text not null, | ||
| parent_lane_id text, | ||
| source_branch_ref text, | ||
| created_at text not null, | ||
| updated_at text not null, | ||
| last_checked_out_at text, | ||
| foreign key(project_id) references projects(id), | ||
| foreign key(lane_id) references lanes(id), | ||
| foreign key(parent_lane_id) references lanes(id) | ||
| ) | ||
| `); | ||
| db.run("create index if not exists idx_lane_branch_profiles_lane on lane_branch_profiles(project_id, lane_id)"); | ||
| db.run("create index if not exists idx_lane_branch_profiles_project_branch on lane_branch_profiles(project_id, normalized_branch_ref)"); |
There was a problem hiding this comment.
UNIQUE constraint on (project_id, lane_id, normalized_branch_ref)
The upsert in upsertBranchProfileForRow is a manual check-then-insert: it reads with getBranchProfileRow, and if no row is found, executes an INSERT. Without a unique constraint there is no database-level guard against concurrent callers (or retry logic) inserting duplicate (project_id, lane_id, normalized_branch_ref) tuples. The getBranchProfileRow query uses limit 1, so a duplicate would be silently hidden but would waste storage and could surface later if limit 1 ordering changes.
Add the constraint to the schema and the same addition is needed in DatabaseBootstrap.sql for the iOS side.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/state/kvDb.ts
Line: 779-798
Comment:
**Missing `UNIQUE` constraint on `(project_id, lane_id, normalized_branch_ref)`**
The upsert in `upsertBranchProfileForRow` is a manual check-then-insert: it reads with `getBranchProfileRow`, and if no row is found, executes an `INSERT`. Without a unique constraint there is no database-level guard against concurrent callers (or retry logic) inserting duplicate `(project_id, lane_id, normalized_branch_ref)` tuples. The `getBranchProfileRow` query uses `limit 1`, so a duplicate would be silently hidden but would waste storage and could surface later if `limit 1` ordering changes.
Add the constraint to the schema and the same addition is needed in `DatabaseBootstrap.sql` for the iOS side.
How can I resolve this? If you propose a fix, please make it concise.
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| logger.warn("laneService.repairLegacyPrimaryBaseRootLanes_failed", { error: err instanceof Error ? err.message : String(err) }); | ||
| } | ||
| try { | ||
| backfillLaneBranchProfiles(); |
There was a problem hiding this comment.
[🟡 Medium] [🟡 Investigate]
backfillLaneBranchProfiles() is called at line 1090 inside listLanes() — before the cache check at line 1095. The function iterates all lanes via getAllLaneRows(true) and calls upsertBranchProfileForRow for each, which issues a SELECT (via getBranchProfileRow) and potentially an INSERT/UPDATE per lane. Since listLanes is a hot path (called from the renderer on lane list refreshes, IPC handlers, etc.), this adds O(n) DB round-trips on every invocation regardless of whether the lane-list cache is warm.
// laneService.ts
try {
backfillLaneBranchProfiles();
} catch (err) { ... }
const cacheKey = ...;
const cached = laneListCache.get(cacheKey);
if (cached && cached.expiresAt > Date.now()) {
return cached.rows.map(cloneLaneSummary);
}The constructor already calls backfillLaneBranchProfiles() at initialization (line 1325), so the listLanes-level call appears to be a belt-and-suspenders measure for data that may have been created between constructor time and first list. However, the backfill is a migration/repair concern that becomes a no-op only after querying the DB. Consider guarding with a flag so the repeated backfill is skipped once the initial pass completes, or move it below the cache check so warm-cache calls skip it entirely.
| ); | ||
|
|
||
| const getRowForLaneBranch = (laneId: string, headBranch: string): PullRequestRow | null => { | ||
| const normalizedHead = normalizeBranchName(headBranch).trim(); |
There was a problem hiding this comment.
[🟡 Medium] [🟡 Investigate]
getRowForLaneBranch normalizes headBranch and returns null if the result is empty (line 869-870). The old getRowForLane had no such guard — it would always find a PR row for the lane regardless of branch. Now, if summary.headBranch is ever empty, whitespace-only, or normalizes to empty for any reason, the upsert at line 1280-1282 will fail to find the existing PR row and INSERT a duplicate instead of UPDATEing. While GitHub PR data should always have headBranch, defensive callers or edge cases (detached worktree, deleted remote branch) could trigger this.
const getRowForLaneBranch = (laneId: string, headBranch: string): PullRequestRow | null => {
const normalizedHead = normalizeBranchName(headBranch).trim();
if (!normalizedHead) return null;Verify that all callers of upsertPr always supply a non-empty headBranch, or add a fallback to getRowForLane when headBranch is empty to avoid silent PR duplication.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add gitOperationsService and laneService branchSwitch test suites - Extend cli + syncRemoteCommandService tests for new git checkout flags - Fix laneService.switchBranch stale-PR cleanup (UPDATE→DELETE; lane_id is NOT NULL) - Document branch switching contract in docs/features/lanes/README.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arul28
force-pushed
the
ade/lane-branch-selector-2ce11f13
branch
from
1e0952a to
aa2815f
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 26, 2026 22:56
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
apps/desktop/src/main/services/prs/prService.ts (1)
868-870:⚠️ Potential issue | 🟠 MajorAdd a lane-level fallback when normalized
headBranchis empty.If normalization trims to empty and returns
null, the upsert path can miss an existing lane row and drift into incorrect insert/adoption behavior. This concern was previously raised and still applies.Suggested fix
const getRowForLaneBranch = (laneId: string, headBranch: string): PullRequestRow | null => { const normalizedHead = normalizeBranchName(headBranch).trim(); - if (!normalizedHead) return null; + if (!normalizedHead) return getRowForLane(laneId); return db.get<PullRequestRow>(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 868 - 870, When normalizeBranchName(headBranch).trim() yields an empty string in getRowForLaneBranch, don’t immediately return null; instead implement a lane-level fallback that searches for an existing PullRequestRow for the given laneId (e.g., lookup existing rows keyed by laneId or scan the prs rows collection) and return that row if found to avoid creating a duplicate/new adoption path; only return null if no lane-level row exists. This change keeps the upsert/adoption logic consistent when normalizedHead is empty.
🧹 Nitpick comments (12)
apps/desktop/src/main/services/state/kvDb.ts (1)
792-794: ConsiderON DELETE CASCADEfor thelane_idFK.Profiles are scoped per-lane and become orphaned when a lane is deleted. The existing
FK_CONSTRAINTSmap in this file (lines 231‑261) is the right place to register"lane_branch_profiles:lane_id": { references: "lanes(id)", action: "on delete cascade" }so the retrofit migration applies it consistently to existing DBs as well. Without this, deleted lanes leave stale profile rows that can interfere with the duplicate-branch ownership checks added by the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/state/kvDb.ts` around lines 792 - 794, The lane_id foreign key on lane_branch_profiles should cascade deletes so per-lane profiles don't become orphaned; add an entry to the existing FK_CONSTRAINTS map for the key "lane_branch_profiles:lane_id" with references "lanes(id)" and action "on delete cascade", and update the lane_branch_profiles table FK definition to include ON DELETE CASCADE so the retrofit migration will apply the constraint to existing DBs; locate the FK_CONSTRAINTS map and the lane_branch_profiles table definition to make these changes.apps/ios/ADE/Views/Lanes/LaneListViewParts.swift (1)
136-146: Extract the duplicated lane title/branch header into a helper view.The lane-name + branch-ref header is now duplicated verbatim across the rebase-suggestion card (lines 136‑146) and the auto-rebase attention card (lines 206‑216). Extracting a small
@ViewBuilder(e.g.laneAttentionTitle(_ lane: LaneSummary)) keeps presentation consistent if either drifts later in the PR series.♻️ Sketch
`@ViewBuilder` private func laneAttentionTitle(_ lane: LaneSummary) -> some View { HStack(spacing: 6) { Text(lane.name) .font(.subheadline.weight(.semibold)) .foregroundStyle(ADEColor.textPrimary) .lineLimit(1) Text(lane.branchRef) .font(.system(.caption2, design: .monospaced)) .foregroundStyle(ADEColor.textMuted) .lineLimit(1) .truncationMode(.middle) } }Also applies to: 206-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift` around lines 136 - 146, The duplicated HStack rendering the lane title/branch (used in the rebase-suggestion card and the auto-rebase attention card) should be extracted into a small `@ViewBuilder` helper to keep presentation consistent; add a private function like laneAttentionTitle(_ lane: LaneSummary) that returns the HStack (matching the provided sketch) and replace the inline HStack usages (the Text(snapshot.lane.name)/Text(snapshot.lane.branchRef) blocks at the two duplicated sites) with calls to laneAttentionTitle(snapshot.lane) so both cards use the single shared view.apps/desktop/src/renderer/components/lanes/LaneStackPane.tsx (1)
259-277: Prefer displaying a normalized branch name in the lane label.Rendering raw
lane.branchRefcan be noisy (refs/heads/...) and truncates useful info earlier. Consider showing a cleaned branch label while keeping the full ref in tooltip/details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LaneStackPane.tsx` around lines 259 - 277, Replace the noisy raw branch ref shown in the second span by computing a normalized branch name (e.g., strip the "refs/heads/" prefix or other common prefixes) and render that normalized name in the visible text while keeping the full lane.branchRef in the title/tooltip; update the rendering in LaneStackPane (the span that currently uses lane.branchRef and title={`Branch: ${lane.branchRef}`}) to use the normalized value for display but retain lane.branchRef for the title/details so truncation still shows the cleaned branch name.apps/ade-cli/src/cli.ts (1)
1051-1058: Consider only sendingacknowledgeActiveWorkwhen the flag is set.
acknowledgeActiveWorkis currently always forwarded as a boolean (falsewhen the flag is absent). The RPC schema accepts that, but to keep payloads minimal and consistent withstartPoint/baseRef(which are spread conditionally), you may want to gate it the same way:♻️ Suggested change
steps: [actionCallStep("result", "git_checkout_branch", withLane({ branchName, mode: create ? "create" : "existing", ...(startPoint ? { startPoint } : {}), ...(baseRef ? { baseRef } : {}), - acknowledgeActiveWork, + ...(acknowledgeActiveWork ? { acknowledgeActiveWork: true } : {}), }))]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` around lines 1051 - 1058, The request is to stop always sending acknowledgeActiveWork as a boolean; update the argument passed into actionCallStep("result","git_checkout_branch", withLane({...})) so that acknowledgeActiveWork is only included when the flag is set (e.g. spread ...(acknowledgeActiveWork ? { acknowledgeActiveWork } : {}) similar to how startPoint and baseRef are handled), keeping branchName and mode unchanged and using the same conditional-spread pattern to minimize the RPC payload.apps/desktop/src/renderer/browserMock.ts (1)
2452-2467: MockswitchBranchalways returns the primary lane regardless of input.
switchBranch: resolvedArg({ lane: MOCK_LANES[0], ... })returnslane-main(the primary lane) for every invocation. This is fine to render the UI, but if any renderer component relies on the responselane.idmatching the requestedlaneId(e.g., to update local state for the lane that was switched), the browser mock will quietly drive the wrong lane. Consider resolving the lane from the argument so the mock mirrors the real IPC contract more closely.♻️ Suggested refinement
- switchBranch: resolvedArg({ - lane: MOCK_LANES[0], - previousBranchRef: "main", - activeWork: [], - }), + switchBranch: async (args: { laneId?: string } = {}) => { + const lane = + MOCK_LANES.find((l: any) => l.id === args?.laneId) ?? MOCK_LANES[0]; + return { + lane, + previousBranchRef: lane.branchRef ?? "main", + activeWork: [], + }; + },You may also want
previewBranchSwitchto echo the requestedlaneId/targetBranchReffor the same reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/browserMock.ts` around lines 2452 - 2467, The mock always returns MOCK_LANES[0] for switchBranch which causes incorrect lane.id in callers; change the resolved value for switchBranch to look up the requested laneId from the incoming args and return that lane (e.g., find by id in MOCK_LANES and fall back to MOCK_LANES[0] if not found), and similarly update previewBranchSwitch to echo the requested laneId/targetBranchRef from the args instead of hardcoding "mock"/"main", so the mock mirrors the real IPC contract (refer to switchBranch, previewBranchSwitch, and MOCK_LANES).apps/desktop/src/main/services/sync/syncRemoteCommandService.ts (2)
828-830: Minor: simplifyacknowledgeActiveWorkparsing.
asOptionalBooleanis invoked twice for the same value. A single binding keeps it tidier and avoids the re-evaluation:Proposed refactor
- ...(asOptionalBoolean(value.acknowledgeActiveWork) !== undefined - ? { acknowledgeActiveWork: asOptionalBoolean(value.acknowledgeActiveWork) } - : {}), + ...(typeof value.acknowledgeActiveWork === "boolean" + ? { acknowledgeActiveWork: value.acknowledgeActiveWork } + : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines 828 - 830, The code calls asOptionalBoolean(value.acknowledgeActiveWork) twice; capture it once in a local binding (e.g., const ack = asOptionalBoolean(value.acknowledgeActiveWork)) and then use that binding in the conditional that builds the object (use ack !== undefined ? { acknowledgeActiveWork: ack } : {}), updating the object spread in the same expression where acknowledgeActiveWork is set.
825-825: Unvalidatedmodecast.
value.modeis cast to the"existing" | "create"union with only anasTrimmedStringnon-empty check — any arbitrary string (e.g."foo") survives parsing and is passed downstream.laneService.switchBranchonly branches onmode === "create"(defaulting otherwise to "existing"), so an unknown value silently degrades to "existing" rather than rejecting the request. Consider validating against the allowed set, mirroring howparseLandPrArgs/parseSubmitPrReviewArgshandle their unions.Proposed fix
- ...(asTrimmedString(value.mode) ? { mode: value.mode as GitCheckoutBranchArgs["mode"] } : {}), + ...((): Partial<Pick<GitCheckoutBranchArgs, "mode">> => { + const mode = asTrimmedString(value.mode); + if (!mode) return {}; + if (mode !== "existing" && mode !== "create") { + throw new Error("git.checkoutBranch requires mode to be 'existing' or 'create'."); + } + return { mode }; + })(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` at line 825, The code currently casts value.mode to GitCheckoutBranchArgs["mode"] after only checking non-empty via asTrimmedString, allowing invalid strings to pass through; update the logic that constructs the options (the expression using ...(asTrimmedString(value.mode) ? { mode: value.mode as GitCheckoutBranchArgs["mode"] } : {})) to explicitly validate value.mode against the allowed union ("existing" | "create") before including it, mirroring the validation style used by parseLandPrArgs/parseSubmitPrReviewArgs; if the mode is not one of the allowed values either omit the field or return/throw a validation error so laneService.switchBranch only ever receives a valid "create" or "existing" value.apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts (1)
713-732: Tighten preview result assertion to match the requested mode.This test sends
mode: "create"but assertsmode: "existing"in the returned payload, which weakens intent clarity. Prefer overriding the mock for this test and assertingmode: "create".♻️ Suggested test clarity tweak
it("lanes.previewBranchSwitch routes to laneService.previewBranchSwitch with optional fields", async () => { + laneService.previewBranchSwitch.mockResolvedValueOnce({ + laneId: "lane-1", + currentBranchRef: "main", + targetBranchRef: "feature/foo", + mode: "create", + dirty: false, + duplicateLaneId: null, + duplicateLaneName: null, + activeWork: [], + targetProfile: null, + }); const result = await service.execute(makePayload("lanes.previewBranchSwitch", { laneId: "lane-1", branchName: "feature/foo", mode: "create", startPoint: "main", baseRef: "main", acknowledgeActiveWork: true, })); @@ - expect(result).toMatchObject({ laneId: "lane-1", mode: "existing" }); + expect(result).toMatchObject({ laneId: "lane-1", mode: "create" }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts` around lines 713 - 732, The test sends mode: "create" but the mocked laneService.previewBranchSwitch returns a preview with mode: "existing", so update the test to explicitly mock laneService.previewBranchSwitch to return a preview object with mode: "create" (e.g., call mockReturnValueOnce or mockResolvedValueOnce on laneService.previewBranchSwitch) before calling service.execute(makePayload(...)), then change the assertion to expect(result).toMatchObject({ laneId: "lane-1", mode: "create" }) so the returned payload matches the requested mode; locate references to service.execute, makePayload, and laneService.previewBranchSwitch in the test and add the mock override there.apps/ade-cli/src/cli.test.ts (1)
522-585: Add one socket-backed round-trip for checkout args.These cases only verify
buildCliPlan(). The same checkout fields are normalized again inapps/ade-cli/src/adeRpcServer.ts, so the desktop socket-backed path can drift from the headless planner without this suite failing. Based on learnings, For ADE CLI changes, verify both headless mode and the desktop socket-backed ADE RPC path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.test.ts` around lines 522 - 585, Add tests that exercise the desktop socket-backed ADE RPC path in addition to the headless planner: for each existing checkout test (those calling buildCliPlan), create a parallel assertion that sends the same CLI args through the ADE RPC server socket round-trip (use the RPC entrypoint implemented in adeRpcServer, e.g., the handler that accepts CLI requests) and assert the resulting step params match the expected git_checkout_branch shape; specifically call the RPC path used by adeRpcServer for checkout, deserialize the response to the same plan structure, and verify mode, branchName, startPoint/baseRef presence, and acknowledgeActiveWork to ensure the socket-backed normalization stays in sync with buildCliPlan.apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts (1)
2650-2668: Document the new checkout fields in the tool description and.describe()annotations.The schema gained
startPoint,baseRef, andacknowledgeActiveWork, but the tool description still reads "Switch to or create a git branch in a lane." and none of the new fields carry.describe()text. Since this is the LLM-facing contract, the model has no signal for whenstartPoint/baseRefapply (create vs. existing mode) or whatacknowledgeActiveWorkgates. Consider tightening the description and per-field docs so the operator emits these correctly with the new lane branch switching flow.📝 Proposed wording
tools.gitCheckoutBranch = tool({ - description: "Switch to or create a git branch in a lane.", + description: + "Switch to an existing branch or create a new branch in a lane. " + + "When create=true, optionally specify startPoint (commit/branch to base the new branch on) and baseRef (upstream tracking ref). " + + "Set acknowledgeActiveWork=true only after the user has confirmed it is safe to leave behind uncommitted or in-progress work in the current branch.", inputSchema: z.object({ - laneId: z.string().optional(), - branch: z.string().min(1), - create: z.boolean().optional().default(false), - startPoint: z.string().optional(), - baseRef: z.string().optional(), - acknowledgeActiveWork: z.boolean().optional().default(false), + laneId: z.string().optional().describe("Lane to operate in. Defaults to the primary lane."), + branch: z.string().min(1).describe("Branch name to switch to or create."), + create: z.boolean().optional().default(false).describe("If true, create a new branch instead of checking out an existing one."), + startPoint: z.string().optional().describe("Commit/branch to base a newly created branch on (only used when create=true)."), + baseRef: z.string().optional().describe("Upstream/base ref to track for the branch (only used when create=true)."), + acknowledgeActiveWork: z.boolean().optional().default(false).describe("Acknowledge that leaving active/uncommitted work is intentional. Required when the lane currently has dirty state."), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` around lines 2650 - 2668, Update the LLM-facing contract for tools.gitCheckoutBranch: expand the tool description to mention both create and existing branch modes and when startPoint, baseRef, and acknowledgeActiveWork apply, and add .describe() annotations on the inputSchema fields startPoint, baseRef, and acknowledgeActiveWork to give the model clear guidance (e.g., startPoint and baseRef are used when create=true to set the starting ref and base, and acknowledgeActiveWork is a boolean that must be true to proceed if there is uncommitted or in-progress work). Ensure the description and per-field .describe() text explicitly indicate the create vs existing behavior so the operator emits the correct arguments to gitService.checkoutBranch.apps/desktop/src/main/services/lanes/laneService.branchSwitch.test.ts (1)
476-522: Optional: Add a parented-lane case formode='create'.
lane-chere has no parent, so the suite doesn't observe what happens toparent_lane_id/base_refon the lane row when a child lane creates a fresh branch. GivenswitchBranchwritesparent_lane_idfrom the (just-upserted) target profile, a regression that re-roots a child lane would slip through. Worth a sibling test that seedslane-cwithparentLaneId: "lane-main"and asserts the post-switch lane row's parent/base reflect the intended design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/laneService.branchSwitch.test.ts` around lines 476 - 522, Add a sibling test that covers the parented-lane case for mode='create': seed the DB with lane-c having parentLaneId: "lane-main" (use insertLane with parentLaneId and branchRef "feature/c"), call service.switchBranch({ laneId: "lane-c", branchName: "feature/new", mode: "create", baseRef: "main" }) the same way as the existing test, then assert the updated lane row's parentLaneId and baseRef reflect the intended behavior (check service.listBranchProfiles("lane-c") and the returned result.lane to verify parentLaneId/baseRef); use the same mocked runGit/runGitOrThrow setup and ensure the checkout '-b' behavior is still asserted so regressions that re-root a child lane are detected.apps/desktop/src/renderer/components/lanes/laneUtils.ts (1)
239-271: Optional: TightenstripRemotePrefixand centralize branch-name rules.Two small things worth considering:
- The non-
refs/remotes/fallback unconditionally strips the first slash-delimited segment, so a stray call with a local-style name likefeature/foowould silently returnfoo. Today that's gated by callers passing only remote rows, but it's a footgun for future reuse — consider explicitly requiring anorigin/-style prefix before stripping.validateBranchNamehere andLaneBranchPickerSheet.validateBranchNameare kept in lock-step manually; any future rule update will need both. Since these rules already mirrorgit check-ref-format, you might keep a brief comment pointing to that spec so the two stay aligned.export function stripRemotePrefix(name: string): string { if (name.startsWith("refs/remotes/")) { const rest = name.slice("refs/remotes/".length); const slash = rest.indexOf("/"); return slash >= 0 ? rest.slice(slash + 1) : rest; } - const firstSlash = name.indexOf("/"); - if (firstSlash > 0 && firstSlash < name.length - 1) { - return name.slice(firstSlash + 1); - } - return name; + // Only strip a leading remote namespace (e.g. "origin/feature/x" → "feature/x"). + // Never collapse a regular slashed branch name like "feature/x" → "x". + if (name.startsWith("origin/")) { + return name.slice("origin/".length); + } + return name; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/laneUtils.ts` around lines 239 - 271, Update stripRemotePrefix to avoid stripping arbitrary local names: only remove a prefix when the name startsWith "refs/remotes/" or matches an explicit remote-style prefix like /^[^/]+\/.+/ (i.e., a single non-empty segment followed by a slash), otherwise return the name unchanged; change the fallback logic in stripRemotePrefix (the function) accordingly. Also centralize the branch-name rules by reusing validateBranchName from this module in LaneBranchPickerSheet.validateBranchName (import the function rather than duplicating rules) and add a short comment above validateBranchName referencing git check-ref-format so future edits stay aligned.
🤖 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/ade-cli/src/adeRpcServer.ts`:
- Around line 702-715: The RPC schema for the tool "git_checkout_branch"
accidentally advertises optional fields as required because the
inputSchema.required currently expands to every property; fix by ensuring the
required array only lists the truly mandatory key (keep required: ["branchName"]
and do not include mode, startPoint, baseRef, or acknowledgeActiveWork), i.e.,
leave mode, startPoint, baseRef, and acknowledgeActiveWork only under properties
so sanitizeToolSchema won’t mark them mandatory; reference the schema object for
name "git_checkout_branch" and the property keys mode, startPoint, baseRef, and
acknowledgeActiveWork when making the change.
- Around line 5265-5276: The code silently downgrades any non-"create" mode to
"existing"; instead validate toolArgs.mode and reject invalid values: check the
string value of toolArgs.mode (use the local mode variable) and if it is
provided and is not exactly "create" or "existing", return/throw an RPC
invalidParams error rather than mapping it to "existing"; then pass the
validated mode through to runtime.gitService.checkoutBranch (do not coerce
unknown strings to "existing"). This validation should live alongside where mode
is derived and before calling checkoutBranch to ensure checkoutBranch only
receives "create", "existing", or undefined.
In `@apps/ade-cli/src/cli.ts`:
- Around line 1044-1047: The VALUE_CARRIER_FLAGS set is missing the new
value-bearing options used by readValue (specifically "--start-point", "--from",
and "--base-ref"), causing hasHelpFlag to misinterpret a following "--help" as a
request instead of a value; update the VALUE_CARRIER_FLAGS collection to include
"--start-point", "--from", and "--base-ref" (keeping alphabetical/grouping
style) so hasHelpFlag treats help tokens as potential values for the flags read
by readValue (see references to VALUE_CARRIER_FLAGS, hasHelpFlag, and the
readValue calls for startPoint/baseRef).
In `@apps/desktop/src/main/services/git/gitOperationsService.ts`:
- Around line 1102-1107: The branch-picker path is calling laneService.list() on
every git.listBranches() request which causes an O(N) DB scan; replace the full
list call with a lightweight lookup or cached snapshot: add or use a fast lookup
like laneService.getByBranchRef/ findByBranchRef (query by branchRef) and call
that per branch instead of laneService.list(), or maintain a cached
activeLaneOwners map in gitOperationsService (updated on lane
create/update/delete events) and read from that cache synchronously; update the
code that currently uses laneService.list(), activeLaneOwners, and args.laneId
to use the new per-branch lookup or the cache to avoid enumerating all lanes.
- Around line 1098-1100: The call to laneService.listBranchProfiles(args.laneId)
inside listBranches() can throw and currently bubbles up, preventing the branch
picker from loading; wrap that call in a try/catch so the lookup is best-effort:
if listBranchProfiles(...) throws, catch the error, log or warn (similar to the
existing owner lookup behavior), and assign branchProfiles = new Set() so the
method continues gracefully; reference the
laneService.listBranchProfiles(args.laneId) invocation and the branchProfiles
variable inside the listBranches() function when making the change.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 2025-2153: Post-checkout DB writes (starting at
upsertBranchProfileForRow and including the update lanes statement and the
stale-PR deletes) are not atomic, so a later DB error can leave the worktree on
the new branch but the DB inconsistent; wrap the sequence from the profile
upsert through the stale PR DELETEs in a single transaction (e.g.,
db.transaction(...) or equivalent) so the upsertBranchProfileForRow, the update
lanes (db.run with branch_ref/base_ref/parent_lane_id), and the multi-step
deletes for pr_convergence_state / pr_pipeline_settings / pr_issue_inventory /
pr_group_members and pull_requests are committed or rolled back together while
keeping the runGitOrThrow checkout outside the transaction; ensure you still
call invalidateLaneListCache() after a successful commit (and do not swallow
errors from runGitOrThrow or the transaction).
- Around line 633-725: Add a DB-level UNIQUE constraint on lane_branch_profiles
for (project_id, lane_id, normalized_branch_ref) and a one-time dedup migration
that consolidates duplicates (preserve one row per unique key and update any FK
references if needed); update your bootstrap SQL (DatabaseBootstrap.sql) and any
KV DB schema code to include the same unique index. Replace the
check-then-insert logic in upsertBranchProfileForRow with a single atomic INSERT
... ON CONFLICT(project_id, lane_id, normalized_branch_ref) DO UPDATE ... that
sets the columns (branch_ref, base_ref, parent_lane_id, source_branch_ref,
updated_at, last_checked_out_at) and returns the row, so getBranchProfileRow and
listBranchProfiles can rely on a single canonical row per key. Ensure the
migration runs once before normal operation and remove/guard any client-side
duplicate-creation code paths in the code that call upsertBranchProfileForRow.
- Around line 2059-2120: When creating/upserting a branch profile for a branch
the lane hasn't checked out before (both the mode === "create" branch and the
existing branch-without-profile path), don't set parentLaneId to null; instead
inherit the lane's current parent and base ref. Update the
upsertBranchProfileForRow calls that set parentLaneId: null to use
existingProfile?.parentLaneId ?? row.parent_lane_id (or similar accessor) and
set baseRef to args.baseRef?.trim() || existingProfile?.baseRef || row.base_ref
|| defaultBaseRef; use the same logic when constructing the profile in both the
create branch block and the branch-without-prior-profile path so
targetBranchRef, upsertBranchProfileForRow, toLaneBranchProfile,
targetProfileRow, and getBranchProfileRow all see the inherited parent_lane_id
and base_ref rather than null.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 779-798: The lane_branch_profiles table lacks a uniqueness
constraint for the natural key, which allows duplicate (project_id, lane_id,
normalized_branch_ref) rows; add a UNIQUE index (e.g. CREATE UNIQUE INDEX IF NOT
EXISTS idx_lane_branch_profiles_unique ON lane_branch_profiles(project_id,
lane_id, normalized_branch_ref)) instead of an inline UNIQUE clause to avoid
retrofitLegacyPrimaryKeyNotNullSchema issues, and ensure you run a one-time
dedupe migration to remove existing duplicates before creating the unique index
so the index creation does not fail; update the same DB bootstrapping code that
creates idx_lane_branch_profiles_lane and
idx_lane_branch_profiles_project_branch to include this unique index creation.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 529-538: When branchLane changes (or before starting a new fetch)
clear the shared laneBranches so stale branches from the previous lane aren't
shown: inside the useEffect that watches branchDropdownOpen and branchLane?.id
(the effect using window.ade.git.listBranches), call setLaneBranches([])
immediately when entering the effect (or when branchLane changes) and ensure the
catch handler also clears laneBranches on failure; keep the cancelled flag and
setLaneBranchesLoading toggles as currently implemented to avoid race
conditions.
- Around line 547-553: The branch-dropdown effect currently only clears search
state but not the create-branch defaults, so pickers like newBranchStartPoint
and newBranchBaseRef persist across lanes; update the same useEffect (the one
watching branchDropdownOpen) to also reset those defaults by calling the state
setters for newBranchStartPoint and newBranchBaseRef (e.g.,
setNewBranchStartPoint(null) and setNewBranchBaseRef(null) or the appropriate
clearing values) when branchDropdownOpen becomes true so a ref picked in one
lane won't be reused in the next.
In `@apps/ios/ADE/Resources/DatabaseBootstrap.sql`:
- Around line 57-76: Add a unique index (not an inline UNIQUE constraint) on
lane_branch_profiles for the tuple (project_id, lane_id, normalized_branch_ref)
to prevent duplicate rows during concurrent upserts; follow the pattern used for
linear_issue_claims and name it clearly (e.g.,
idx_lane_branch_profiles_unique_project_lane_branch). Before creating the unique
index, deduplicate any existing rows in lane_branch_profiles so the migration
won't fail on installs that already contain duplicates. This change protects the
race in getBranchProfileRow / upsertBranchProfileForRow where a get-then-insert
can produce duplicates and avoids relying on inline constraints that the desktop
migration (kvDb.ts) strips.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 2839-2845: The checkoutPrimaryBranch function currently has
defaulted parameters (mode, startPoint, baseRef, acknowledgeActiveWork) which
lets existing callers bypass the new preview/confirm flow; change the function
signature of checkoutPrimaryBranch (the method defined here) to remove the
defaults so callers must explicitly pass mode, startPoint, baseRef, and
acknowledgeActiveWork, then update all call sites (e.g., the call in
LanesTabView.swift) to supply the appropriate explicit values for these
parameters (choose the correct mode string and nil or concrete values for
startPoint/baseRef and true/false for acknowledgeActiveWork) so
dirty-state/active-work/duplicate-ownership flows always go through the new
confirmation UI.
In `@apps/ios/ADE/Views/Lanes/LaneActionsCard.swift`:
- Around line 50-59: The branchSwitchDisabledReason getter wrongly treats a nil
laneRole as a worker lane; update the final conditional in
branchSwitchDisabledReason to only disable switching for explicit worker roles
(e.g., require laneRole == "worker") or check that laneRole is non-nil and
equals "worker" instead of using laneRole != "result"; keep the existing
attached-lane and result-lane checks (referencing branchSwitchDisabledReason,
laneType, missionId, laneRole) so callers that omit laneRole do not get
inadvertently locked out.
In `@apps/ios/ADE/Views/Lanes/LaneBranchPickerSheet.swift`:
- Around line 550-573: The branch-name validator validateBranchName currently
only treats ASCII space/tab/newline as illegal; change the whitespace check to
use Swift's Unicode-aware Character.isWhitespace (e.g., replace the illegal set
entry for " ", "\t", "\n" and the contains(where:) call to check
$0.isWhitespace) so characters like U+00A0/U+2028 are rejected consistent with
the TypeScript regex; update the corresponding error message in
validateBranchName if needed and ensure this runs before previewBranchSwitch is
invoked.
In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift`:
- Around line 137-145: Long lane names are starving the branch-ref Text in the
horizontal layout; update the Text for the branch ref (the Text showing
snapshot.lane.branchRef) to get higher layout priority so its monospaced middle
truncation remains useful (e.g., add .layoutPriority(1) to the branch-ref Text)
and/or reduce the name Text's priority (the Text showing snapshot.lane.name) so
the name truncates first; apply the same change to the second occurrence at the
other block (lines showing snapshot.lane.name and snapshot.lane.branchRef in the
later group).
---
Duplicate comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 868-870: When normalizeBranchName(headBranch).trim() yields an
empty string in getRowForLaneBranch, don’t immediately return null; instead
implement a lane-level fallback that searches for an existing PullRequestRow for
the given laneId (e.g., lookup existing rows keyed by laneId or scan the prs
rows collection) and return that row if found to avoid creating a duplicate/new
adoption path; only return null if no lane-level row exists. This change keeps
the upsert/adoption logic consistent when normalizedHead is empty.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 522-585: Add tests that exercise the desktop socket-backed ADE RPC
path in addition to the headless planner: for each existing checkout test (those
calling buildCliPlan), create a parallel assertion that sends the same CLI args
through the ADE RPC server socket round-trip (use the RPC entrypoint implemented
in adeRpcServer, e.g., the handler that accepts CLI requests) and assert the
resulting step params match the expected git_checkout_branch shape; specifically
call the RPC path used by adeRpcServer for checkout, deserialize the response to
the same plan structure, and verify mode, branchName, startPoint/baseRef
presence, and acknowledgeActiveWork to ensure the socket-backed normalization
stays in sync with buildCliPlan.
In `@apps/ade-cli/src/cli.ts`:
- Around line 1051-1058: The request is to stop always sending
acknowledgeActiveWork as a boolean; update the argument passed into
actionCallStep("result","git_checkout_branch", withLane({...})) so that
acknowledgeActiveWork is only included when the flag is set (e.g. spread
...(acknowledgeActiveWork ? { acknowledgeActiveWork } : {}) similar to how
startPoint and baseRef are handled), keeping branchName and mode unchanged and
using the same conditional-spread pattern to minimize the RPC payload.
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Around line 2650-2668: Update the LLM-facing contract for
tools.gitCheckoutBranch: expand the tool description to mention both create and
existing branch modes and when startPoint, baseRef, and acknowledgeActiveWork
apply, and add .describe() annotations on the inputSchema fields startPoint,
baseRef, and acknowledgeActiveWork to give the model clear guidance (e.g.,
startPoint and baseRef are used when create=true to set the starting ref and
base, and acknowledgeActiveWork is a boolean that must be true to proceed if
there is uncommitted or in-progress work). Ensure the description and per-field
.describe() text explicitly indicate the create vs existing behavior so the
operator emits the correct arguments to gitService.checkoutBranch.
In `@apps/desktop/src/main/services/lanes/laneService.branchSwitch.test.ts`:
- Around line 476-522: Add a sibling test that covers the parented-lane case for
mode='create': seed the DB with lane-c having parentLaneId: "lane-main" (use
insertLane with parentLaneId and branchRef "feature/c"), call
service.switchBranch({ laneId: "lane-c", branchName: "feature/new", mode:
"create", baseRef: "main" }) the same way as the existing test, then assert the
updated lane row's parentLaneId and baseRef reflect the intended behavior (check
service.listBranchProfiles("lane-c") and the returned result.lane to verify
parentLaneId/baseRef); use the same mocked runGit/runGitOrThrow setup and ensure
the checkout '-b' behavior is still asserted so regressions that re-root a child
lane are detected.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 792-794: The lane_id foreign key on lane_branch_profiles should
cascade deletes so per-lane profiles don't become orphaned; add an entry to the
existing FK_CONSTRAINTS map for the key "lane_branch_profiles:lane_id" with
references "lanes(id)" and action "on delete cascade", and update the
lane_branch_profiles table FK definition to include ON DELETE CASCADE so the
retrofit migration will apply the constraint to existing DBs; locate the
FK_CONSTRAINTS map and the lane_branch_profiles table definition to make these
changes.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts`:
- Around line 713-732: The test sends mode: "create" but the mocked
laneService.previewBranchSwitch returns a preview with mode: "existing", so
update the test to explicitly mock laneService.previewBranchSwitch to return a
preview object with mode: "create" (e.g., call mockReturnValueOnce or
mockResolvedValueOnce on laneService.previewBranchSwitch) before calling
service.execute(makePayload(...)), then change the assertion to
expect(result).toMatchObject({ laneId: "lane-1", mode: "create" }) so the
returned payload matches the requested mode; locate references to
service.execute, makePayload, and laneService.previewBranchSwitch in the test
and add the mock override there.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts`:
- Around line 828-830: The code calls
asOptionalBoolean(value.acknowledgeActiveWork) twice; capture it once in a local
binding (e.g., const ack = asOptionalBoolean(value.acknowledgeActiveWork)) and
then use that binding in the conditional that builds the object (use ack !==
undefined ? { acknowledgeActiveWork: ack } : {}), updating the object spread in
the same expression where acknowledgeActiveWork is set.
- Line 825: The code currently casts value.mode to GitCheckoutBranchArgs["mode"]
after only checking non-empty via asTrimmedString, allowing invalid strings to
pass through; update the logic that constructs the options (the expression using
...(asTrimmedString(value.mode) ? { mode: value.mode as
GitCheckoutBranchArgs["mode"] } : {})) to explicitly validate value.mode against
the allowed union ("existing" | "create") before including it, mirroring the
validation style used by parseLandPrArgs/parseSubmitPrReviewArgs; if the mode is
not one of the allowed values either omit the field or return/throw a validation
error so laneService.switchBranch only ever receives a valid "create" or
"existing" value.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 2452-2467: The mock always returns MOCK_LANES[0] for switchBranch
which causes incorrect lane.id in callers; change the resolved value for
switchBranch to look up the requested laneId from the incoming args and return
that lane (e.g., find by id in MOCK_LANES and fall back to MOCK_LANES[0] if not
found), and similarly update previewBranchSwitch to echo the requested
laneId/targetBranchRef from the args instead of hardcoding "mock"/"main", so the
mock mirrors the real IPC contract (refer to switchBranch, previewBranchSwitch,
and MOCK_LANES).
In `@apps/desktop/src/renderer/components/lanes/LaneStackPane.tsx`:
- Around line 259-277: Replace the noisy raw branch ref shown in the second span
by computing a normalized branch name (e.g., strip the "refs/heads/" prefix or
other common prefixes) and render that normalized name in the visible text while
keeping the full lane.branchRef in the title/tooltip; update the rendering in
LaneStackPane (the span that currently uses lane.branchRef and title={`Branch:
${lane.branchRef}`}) to use the normalized value for display but retain
lane.branchRef for the title/details so truncation still shows the cleaned
branch name.
In `@apps/desktop/src/renderer/components/lanes/laneUtils.ts`:
- Around line 239-271: Update stripRemotePrefix to avoid stripping arbitrary
local names: only remove a prefix when the name startsWith "refs/remotes/" or
matches an explicit remote-style prefix like /^[^/]+\/.+/ (i.e., a single
non-empty segment followed by a slash), otherwise return the name unchanged;
change the fallback logic in stripRemotePrefix (the function) accordingly. Also
centralize the branch-name rules by reusing validateBranchName from this module
in LaneBranchPickerSheet.validateBranchName (import the function rather than
duplicating rules) and add a short comment above validateBranchName referencing
git check-ref-format so future edits stay aligned.
In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift`:
- Around line 136-146: The duplicated HStack rendering the lane title/branch
(used in the rebase-suggestion card and the auto-rebase attention card) should
be extracted into a small `@ViewBuilder` helper to keep presentation consistent;
add a private function like laneAttentionTitle(_ lane: LaneSummary) that returns
the HStack (matching the provided sketch) and replace the inline HStack usages
(the Text(snapshot.lane.name)/Text(snapshot.lane.branchRef) blocks at the two
duplicated sites) with calls to laneAttentionTitle(snapshot.lane) so both cards
use the single shared view.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b41a880e-7913-47bb-9f9d-00d45b6aedfa
⛔ Files ignored due to path filters (1)
docs/features/lanes/README.mdis excluded by!docs/**
📒 Files selected for processing (31)
apps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/git/gitOperationsService.branchSwitch.test.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.branchSwitch.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/lanes/LaneStackPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/laneUtils.test.tsapps/desktop/src/renderer/components/lanes/laneUtils.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/git.tsapps/desktop/src/shared/types/lanes.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneActionsCard.swiftapps/ios/ADE/Views/Lanes/LaneBranchPickerSheet.swiftapps/ios/ADE/Views/Lanes/LaneDetailGitSection.swiftapps/ios/ADE/Views/Lanes/LaneListViewParts.swift
Test fix: - aiOrchestratorService health-sweep test: bump wait budget 40→200 (1s→5s) for CI loadiness; raise per-test timeout 10s→15s. Review comments addressed: - adeRpcServer: declare required[] on git.checkout_branch schema so optional fields stay optional; reject unknown checkout modes with invalidParams. - cli: add --start-point/--from/--base-ref to VALUE_CARRIER_FLAGS. - gitOperationsService: best-effort listBranchProfiles + new laneService.listBranchOwners (cheap, scoped) replaces full lane list per branch picker open. - laneService: wrap post-checkout writes (profile upsert + lanes update + stale-PR cleanup) in a single db.transaction; bootstrap-time lane_branch_profiles dedup sweep covers the missing UNIQUE on CRR tables. - prService: lane-level fallback when normalized headBranch trims to empty. - LanesPage: clear laneBranches on lane change/fetch failure; reset newBranchStartPoint and newBranchBaseRef when target lane changes. - iOS schema parity (DatabaseBootstrap.sql), explicit switch params at LanesTabView call sites, allow switch when laneRole nil, Unicode whitespace match for normalizeBranchKey, layoutPriority on LaneListViewParts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 26, 2026 23:29
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
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR introduces lane-scoped branch switching: any worktree lane can now switch to an existing branch or create a new one, with preview/acknowledgement for dirty worktrees, duplicate ownership, and active terminal/process sessions. The DB migration, iOS schema, test coverage, and renderer UX are all included.
sanitizeToolSchemaregression inadeRpcServer.ts: the new logic setsout.required = []for any tool whose schema has no explicitrequiredarray, where previously all properties were forced required. AnyTOOL_SPECSentry that relied on the old implicit all-required behaviour will now expose every parameter as optional to the LLM, causing silent omissions andassertNonEmptyStringfailures at call time.modenot validated inparseGitCheckoutBranchArgs: any non-empty string is cast directly to the"existing" | "create"type; an invalid value degrades silently to"existing"mode rather than returning a clear error.DatabaseBootstrap.sqldeduplication DELETE is unguarded: the cleanup DELETE onlane_branch_profilesruns unconditionally without error handling, unlike the desktop migration's wrappedtry/catch.Confidence Score: 3/5
Not safe to merge without addressing the sanitizeToolSchema regression; it silently broadens LLM tool parameter optionality across the entire tool suite.
One confirmed P1 in adeRpcServer.ts (sanitizeToolSchema change removes required-field enforcement for tools without explicit required arrays) pulls the score to 3. The rest of the feature — lane switching logic, DB migration, preview flow, iOS parity, and tests — is well-implemented.
apps/ade-cli/src/adeRpcServer.ts (sanitizeToolSchema change), apps/desktop/src/main/services/sync/syncRemoteCommandService.ts (mode validation)
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Renderer / iOS participant IPC as IPC / SyncCommand participant GOS as gitOperationsService participant LS as laneService participant DB as SQLite (kvDb) participant Git as git worktree UI->>IPC: previewBranchSwitch(laneId, branchName, mode) IPC->>LS: previewBranchSwitch(args) LS->>Git: show-ref (resolve branch existence) LS->>Git: git status --porcelain (dirty check) LS->>DB: findActiveBranchOwner (duplicate lane) LS->>DB: getActiveWorkForLane (terminals/processes) LS-->>UI: LaneBranchSwitchPreview {dirty, duplicateLaneId, activeWork} alt activeWork non-empty UI->>UI: show acknowledgement prompt UI->>IPC: git.checkoutBranch acknowledgeActiveWork=true else clean UI->>IPC: git.checkoutBranch end IPC->>GOS: checkoutBranch(args) GOS->>LS: switchBranch(args) LS->>LS: previewBranchSwitch (re-validate dirty/duplicate) LS->>DB: upsertBranchProfileForRow (old branch) alt mode=create LS->>Git: rev-parse --verify baseRef LS->>Git: checkout -b branchName startPoint else mode=existing LS->>Git: checkout [--track] branchName end LS->>DB: BEGIN LS->>DB: upsertBranchProfileForRow (new branch) LS->>DB: UPDATE lanes SET branch_ref, base_ref, parent_lane_id LS->>DB: DELETE stale pull_requests + child rows LS->>DB: COMMIT LS-->>GOS: LaneBranchSwitchResult GOS-->>UI: GitActionResultComments Outside Diff (3)
apps/desktop/src/main/services/lanes/laneService.ts, line 1089-1098 (link)backfillLaneBranchProfilesruns on everylistLanescall, including cache hitsbackfillLaneBranchProfiles()is invoked before the cache check, so every call tolistLanes— the 15-second poll, PTY data events, conflict updates — iterates all lane rows and issues one upsert per lane, even when the result would be served from cache. With many lanes this degrades the hot path noticeably.Move the backfill inside the cache-miss branch, or rely solely on the one-shot startup call (already present at the initialization block) and skip the per-call invocation entirely.
Prompt To Fix With AI
apps/desktop/src/main/services/lanes/laneService.ts, line 1196-1215 (link)ensureBranchProfileForRowcalls per lane listing on every cache missensureBranchProfileForRow(row)is called for every lane row while building theLaneSummarylist. Each call performs aSELECT(and potentially anUPDATE/INSERT) againstlane_branch_profiles. For a project with many lanes, this means N extra round-trips to SQLite on every cache miss, in addition to thebackfillLaneBranchProfilesthat already ran for all lanes earlier in the same code path.Consider pre-loading all profiles for the project in a single
SELECT * FROM lane_branch_profiles WHERE project_id = ?query before the loop, then indexing them bylane_idin aMapso that eachtoLaneSummarycall can do an O(1) lookup without hitting the database.Prompt To Fix With AI
apps/ade-cli/src/adeRpcServer.ts, line 2032-2037 (link)sanitizeToolSchemasilently removes required-field enforcement for tools without an explicitrequiredarrayThe old path forced
out.required = propKeyswhenever the array was absent or didn't include every property key. The new path only fires whenrequiredis not yet an array, setting it to[]. AnyTOOL_SPECSentry whoseinputSchema.propertiesobject carries norequiredfield will now be serialised to the LLM with no required parameters, whereas before those tools had every parameter marked required. This means the LLM can silently omit arguments that therunToolhandler expects (e.g.,assertNonEmptyString(...)calls that throw on undefined), causing runtime errors in tool execution.Only tools that already have an explicit, correct
requiredarray — like the updatedgit_checkout_branch— are safe. All other tools with norequireddeclaration need one added before this change ships.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ship: iter 1 — fix test-desktop (8) + 16..." | Re-trigger Greptile