ADE-29: Reorganize lane stack from Manage Lane#311
Merged
Conversation
📝 WalkthroughWalkthroughThis PR integrates lane reparenting with optional base-branch customization, GitHub snapshot support for closed external PRs, PTY transcript tail merging, agent chat event history snapshots with session-found tracking, project binding lifecycle management during transitions, subagent parent/child linkage tracking, and comprehensive UI/scroll-event handling improvements across CLI, desktop, and iOS platforms. ChangesUnified feature cohort: lane reparenting, GitHub snapshots, chat history, and infra
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
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. |
…anch - Extend lanes.reparent with stackBaseBranchRef (resolved in repo, prefers origin). - Skip no-op reparent when parent link and base_ref are unchanged. - Manage Lane dialog: parent lane selector, optional base branch, apply runs reparent. - Parse stackBaseBranchRef for remote lanes.reparent in ade-cli sync. ADE-29 Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
- LaneDialogShell: viewport-centered, max-height, scrollable body (all lane modals). - laneDialogTokens: hero and accent section styles for lane dialogs. - Manage Lane: shell description, per-section guidance, batch vs single callouts, adopt/archive/delete copy, stack accent panel and select styling. ADE-29 Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
Picks up uncommitted lane work across desktop, ade-cli, ios, and docs: ManageLaneDialog refactor (singleLane), chat pane turn-active error handling, iOS sync command helpers (lanes.reparent), sync host / remote command updates, project transition state, PR detail/route state cleanup, and broad doc refreshes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
arul28
force-pushed
the
cursor/reorganize-lanes-manage-ade29-7d0c
branch
from
e93f67c to
56a8469
Compare
Owner
Author
|
@copilot review but do not make fixes |
Contributor
Reviewed only (no fixes) on commit e93f67c. Findings:
No code changes were made. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/desktop/src/main/services/lanes/laneService.ts (2)
3434-3443:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the rollback-path DB updates in a transaction.
Similar to the success path, the rollback restores the lane's parent/base (line 3434) and branch profile (line 3438) in two separate DB operations. If the profile upsert fails after the lane restore succeeds, the rollback itself will be incomplete, leaving inconsistent state after a failed rebase.
🔒 Proposed fix to add transaction
+ db.run("begin"); + try { db.run( "update lanes set parent_lane_id = ?, base_ref = ? where id = ? and project_id = ?", [previousParentLaneId, previousBaseRef, lane.id, projectId] ); upsertBranchProfileForRow(lane, { branchRef: lane.branch_ref, baseRef: previousBaseRef, parentLaneId: previousParentLaneId, }); + db.run("commit"); + } catch (err) { + try { db.run("rollback"); } catch { /* keep the original rebase error */ } + // Fall through to throw the original rebase error below + } invalidateLaneListCache();🤖 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/main/services/lanes/laneService.ts` around lines 3434 - 3443, The rollback path currently performs two separate DB ops (db.run("update lanes ...", [...]) and upsertBranchProfileForRow(...)) then calls invalidateLaneListCache(), which can leave the DB inconsistent if the second op fails; wrap the rollback updates in a single transaction so both the lane restore and the branch-profile upsert succeed or both rollback: begin a transaction, perform db.run(...) for restoring lane (use lane.id and projectId), call upsertBranchProfileForRow(lane, { branchRef: lane.branch_ref, baseRef: previousBaseRef, parentLaneId: previousParentLaneId }) inside the same transaction, on success commit and then call invalidateLaneListCache(), and on any error rollback and rethrow/log so the rollback is atomic.
3415-3424:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the success-path DB updates in a transaction.
The lane parent/base update (line 3415) and branch profile upsert (line 3419) are two separate DB operations. If the profile upsert fails after the lane update succeeds, the lane and branch profile tables will be inconsistent. Other methods in this file use transactions for multi-step updates (e.g.,
switchBranchat lines 2743-2798).🔒 Proposed fix to add transaction
+ db.run("begin"); + try { db.run( "update lanes set parent_lane_id = ?, base_ref = ? where id = ? and project_id = ?", [persistedParentLaneId, newBaseRef, lane.id, projectId] ); upsertBranchProfileForRow(lane, { branchRef: lane.branch_ref, baseRef: newBaseRef, parentLaneId: persistedParentLaneId, }); + db.run("commit"); + } catch (err) { + try { db.run("rollback"); } catch { /* swallow rollback failures */ } + throw err; + } invalidateLaneListCache();🤖 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/main/services/lanes/laneService.ts` around lines 3415 - 3424, The update to the lanes table (db.run(...)) and the subsequent upsertBranchProfileForRow(...) must be executed inside a single DB transaction so they either both succeed or both roll back; wrap the sequence (start transaction, perform db.run update, call upsertBranchProfileForRow, call invalidateLaneListCache, commit) and ensure any thrown error causes a rollback and the error to be propagated; use the same DB connection used for db.run and upsertBranchProfileForRow, and handle errors to call rollback before rethrowing.apps/ade-cli/src/cli.ts (1)
2486-2512:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce required parent for
lanes reparent.
newParentLaneIdcan still be omitted, which defers validation to downstream services and yields less actionable CLI errors. Validate it here to match the command contract.💡 Suggested fix
const reparentArgs: JsonObject = { laneId, - newParentLaneId: - readValue(args, [ - "--parent", - "--parent-lane", - "--parent-lane-id", - ]) ?? firstPositional(args), + newParentLaneId: requireValue( + readValue(args, [ + "--parent", + "--parent-lane", + "--parent-lane-id", + ]) ?? firstPositional(args), + "newParentLaneId", + ), };🤖 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/ade-cli/src/cli.ts` around lines 2486 - 2512, The reparent flow allows newParentLaneId to be omitted (it’s set from readValue(...) ?? firstPositional(...)) which defers validation; update the CLI handler so that after building reparentArgs you validate that reparentArgs.newParentLaneId is present and, if not, fail fast with a clear CLI error (e.g., throw or return a user-facing error/exit) before creating the execute step; modify the logic around reparentArgs, readValue, firstPositional, and before the actionStep/collectGenericObjectArgs call to enforce the required parent and surface a concise message like "missing required --parent/--parent-lane-id" instead of letting downstream services handle it.apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
5199-5210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the optimistic bubble before falling back to
steer().This branch keeps the optimistic
user_messagein state, then sends a steer. Steer-backed events carrysteerId, andisMatchingOptimisticUserMessage()explicitly refuses to reconcile those, so the duplicate bubble can stick around until remount.Proposed fix
} catch (sendError) { // Race condition: the turn may have started between our state check // and the backend call. If so, automatically fall back to steer // instead of surfacing a confusing error to the user. if (isTurnAlreadyActiveError(sendError)) { + setOptimisticOutgoingMessageSynced(null); try { await steerMessage(); } catch (steerError) { if (!isNoActiveTurnToSteerError(steerError) || !retryOnStaleSteer) throw steerError; setTurnActiveBySession((prev) => ({ ...prev, [sessionId]: false }));🤖 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/components/chat/AgentChatPane.tsx` around lines 5199 - 5210, The code falls back to steerMessage() when isTurnAlreadyActiveError(sendError) occurs but leaves the optimistic user_message bubble in state so steer events (which carry steerId) won't reconcile it (see isMatchingOptimisticUserMessage()); before calling steerMessage() clear the optimistic user_message/optimistic bubble from state (invoke the existing state setter that removes the optimistic user message) so the steer-backed events can render correctly, then proceed with steerMessage() and the existing error handling (references: isTurnAlreadyActiveError, steerMessage, isNoActiveTurnToSteerError, setTurnActiveBySession, sendMessageOrSteerIfBusy, isMatchingOptimisticUserMessage).apps/desktop/src/main/services/chat/agentChatService.ts (1)
12366-12404:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale placeholder leak on
spawn_agentfailure.When
spawnStatus === "failed"andreceiverIdsis non-empty (and doesn't includeitemId), the placeholder entry keyed byitemIdis read via the?? runtime.activeSubagents.get(itemId)fallback but never removed fromruntime.activeSubagents. The success branch right below explicitly handles this withruntime.activeSubagents.delete(itemId); the failure branch should do the same so the placeholder doesn't survive a failed spawn.🔧 Proposed fix
if (spawnStatus === "failed") { + const placeholder = runtime.activeSubagents.get(itemId); for (const taskId of receiverIds.length ? receiverIds : [itemId]) { const existing = runtime.activeSubagents.get(taskId) ?? runtime.activeSubagents.get(itemId); runtime.activeSubagents.delete(taskId); emitChatEvent(managed, { type: "subagent_result", taskId, parentToolUseId: existing?.parentToolUseId ?? itemId, status: "failed", summary: String(item.error ?? item.result ?? "Agent spawn failed"), turnId, }); } + if (placeholder && receiverIds.length && !receiverIds.includes(itemId)) { + runtime.activeSubagents.delete(itemId); + } } else {🤖 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/main/services/chat/agentChatService.ts` around lines 12366 - 12404, The failure branch for spawn_agent (when mapCodexCollabAgentStatus(item.status) === "failed") leaks the placeholder stored under runtime.activeSubagents keyed by itemId because it only deletes per taskId and uses runtime.activeSubagents.get(itemId) as a fallback; ensure the placeholder is removed: before emitting subagent_result events, delete the itemId key from runtime.activeSubagents (runtime.activeSubagents.delete(itemId)) so the placeholder doesn't persist when receiverIds is non-empty and spawn fails; adjust the logic in the failed branch that iterates receiverIds/itemId accordingly (functions/vars: spawn_agent, mapCodexCollabAgentStatus, runtime.activeSubagents, itemId, receiverIds, emitChatEvent).
🧹 Nitpick comments (4)
apps/desktop/src/main/services/lanes/laneService.ts (1)
3384-3395: 💤 Low valueConsider adding a comment to explain the no-op optimization.
The early return when the parent and base are already set to the target values is a good optimization, but would benefit from a brief comment explaining that we skip the rebase entirely when nothing would change.
💡 Suggested comment
+ // No-op fast path: if the requested parent and base already match the current + // values, skip the rebase and return immediately with unchanged HEAD. if (lane.parent_lane_id === persistedParentLaneId && newBaseRef === previousBaseRef) { const headSha = await getHeadSha(lane.worktree_path);🤖 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/main/services/lanes/laneService.ts` around lines 3384 - 3395, Add a brief inline comment above the early-return block that explains this is a no-op optimization: when lane.parent_lane_id equals persistedParentLaneId and newBaseRef equals previousBaseRef we skip doing any rebase/parent-change work and simply return the current head SHA; place the comment near the block referencing lane.parent_lane_id, persistedParentLaneId, newBaseRef, previousBaseRef and the getHeadSha call so future readers understand why we return unchanged preHeadSha/postHeadSha.apps/ios/ADE/Views/PRs/PrsRootScreen.swift (1)
397-400: ⚡ Quick winConsider task deduplication for rapid filter changes.
The
onChangehandler creates a newTaskon each filter change without storing or cancelling previous tasks. If the user rapidly toggles between filter options, multiple concurrent tasks may spawn and executeloadGitHubExternalHistoryIfNeededbefore the flag check can prevent duplicates.While the
githubExternalHistoryLoadedguard provides some protection, consider storing the task (e.g.,@State private var externalHistoryLoadTask: Task<Void, Never>?) and cancelling it before creating a new one, or using a dedicated actor/serial queue to serialize external history loads.🤖 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/ios/ADE/Views/PRs/PrsRootScreen.swift` around lines 397 - 400, The onChange handler spawns an untracked Task for every githubStatusFilterRawValue change which can lead to concurrent loadGitHubExternalHistoryIfNeeded calls; add a cancellable Task holder (e.g., `@State` private var externalHistoryLoadTask: Task<Void, Never>?) and before creating a new Task cancel the previous one, assign the new Task to externalHistoryLoadTask, and ensure you clear externalHistoryLoadTask when the Task completes or is cancelled; alternatively serialize calls via an actor or dedicated queue in the code path that runs loadGitHubExternalHistoryIfNeeded and reference githubStatusFilterRawValue and selectedGitHubStatusFilter to keep existing guard logic.apps/desktop/src/main/services/pty/ptyService.ts (1)
99-99: ⚡ Quick winHot-path string allocation in
appendRecentOutput.
pty.onDatafires on every raw PTY chunk (before thePTY_DATA_BATCH_INTERVAL_MScoalescing) and each call doestailString(${entry.recentOutputTail}${data}, 2_000_000). Once the buffer is full this allocates a ~2 M-char string (≈ 4 MB UTF-16) and slices it on every chunk, so heavy producers (build/install/log tailing) pay a sizeable per-chunk copy. Across many concurrent terminals the steady-state resident cost of the buffer alone is also substantial.If the hydration window only needs to cover the first TUI frame before the WriteStream flushes, a much smaller cap (e.g. 256–512 KB) would be sufficient and would bound both the per-call copy and the per-session memory. If you want to keep the larger window, consider a chunk-list buffer that only flattens on read:
♻️ Sketch — chunk list, flatten on read
- recentOutputTail: string; + recentOutputChunks: string[]; + recentOutputChars: number;- const appendRecentOutput = (entry: PtyEntry, data: string) => { - if (!data) return; - entry.recentOutputTail = tailString(`${entry.recentOutputTail}${data}`, LIVE_TRANSCRIPT_TAIL_BUFFER_CHARS); - }; + const appendRecentOutput = (entry: PtyEntry, data: string) => { + if (!data) return; + entry.recentOutputChunks.push(data); + entry.recentOutputChars += data.length; + while ( + entry.recentOutputChunks.length > 1 && + entry.recentOutputChars - (entry.recentOutputChunks[0]?.length ?? 0) >= LIVE_TRANSCRIPT_TAIL_BUFFER_CHARS + ) { + entry.recentOutputChars -= entry.recentOutputChunks.shift()!.length; + } + }; + + const readRecentOutputTail = (entry: PtyEntry): string => + tailString(entry.recentOutputChunks.join(""), LIVE_TRANSCRIPT_TAIL_BUFFER_CHARS);Then in
readTranscriptTail:- const live = liveEntryBySessionId(sessionId)?.[1].recentOutputTail ?? ""; + const liveEntry = liveEntryBySessionId(sessionId)?.[1] ?? null; + const live = liveEntry ? readRecentOutputTail(liveEntry) : "";Also applies to: 2093-2096
🤖 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/main/services/pty/ptyService.ts` at line 99, The LIVE_TRANSCRIPT_TAIL_BUFFER_CHARS constant is too large and causes heavy per-chunk string allocations in appendRecentOutput (called from pty.onData); either reduce the cap (e.g., set LIVE_TRANSCRIPT_TAIL_BUFFER_CHARS to 256*1024–512*1024) to bound per-call copies and per-session memory, or replace the simple string-slice approach in appendRecentOutput and readTranscriptTail with a chunk-list buffer that appends incoming chunks to an array and only flattens (joins) to a single string when readTranscriptTail is called, ensuring you update appendRecentOutput, the buffer storage used by entry.recentOutputTail, and readTranscriptTail to manage chunk eviction to keep the total buffered size under the new cap.apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx (1)
384-384: ⚡ Quick winModule-level test state reduces test isolation.
The
iosEventListenervariable is declared at module scope and mutated by individual tests. WhilebeforeEachresets it tonull, this pattern can cause issues:
- Test isolation: If a test fails before cleanup, subsequent tests may see stale state.
- Future-proofing: Parallel test execution would create race conditions.
- Clarity: Tests directly inspecting module variables are harder to understand than explicit returns from setup functions.
Consider refactoring to return the listener reference from
installAdeMocks(similar toemitChatEvent,emitSessionChanged), or using a test-scoped closure.♻️ Proposed refactor
-let iosEventListener: ((event: { type: string; chatSessionId?: string; laneId?: string; mode?: string }) => void) | null = null; - beforeEach(() => { invalidateAiDiscoveryCache(); window.localStorage.clear(); window.sessionStorage.clear(); - iosEventListener = null; Object.defineProperty(window.navigator, "platform", { configurable: true, value: "MacIntel", }); resetChatTestStore(); });Inside
installAdeMocks:+ let iosEventListener: ((event: { type: string; chatSessionId?: string; laneId?: string; mode?: string }) => void) | null = null; + // ... existing code ... return { send, steer, // ... other mocks ... + getIosEventListener: () => iosEventListener, emitChatEvent: (event: AgentChatEventEnvelope) => { for (const listener of chatEventListeners) { listener(event); } },In tests:
- await waitFor(() => { - expect(typeof iosEventListener).toBe("function"); - }); - act(() => { - iosEventListener?.({ + const mocks = installAdeMocks({ sessions: [session] }); + await waitFor(() => { + expect(typeof mocks.getIosEventListener()).toBe("function"); + }); + act(() => { + mocks.getIosEventListener()?.({Also applies to: 390-390, 324-327
🤖 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/components/chat/AgentChatPane.submit.test.tsx` at line 384, The module-level mutable variable iosEventListener breaks test isolation; instead refactor installAdeMocks to return the listener (or an accessor) so tests receive a test-scoped reference rather than mutating iosEventListener directly; update tests that currently read/write iosEventListener to call installAdeMocks() and use its returned listener (similar to existing emitChatEvent and emitSessionChanged), remove the module-scope iosEventListener declaration, and ensure beforeEach/afterEach use the returned reference for cleanup.
🤖 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/ade-cli/src/tuiClient/app.tsx`:
- Around line 620-622: The current summary selection uses summaryFromEvent =
[event.summary, event.finalSummary, event.text, event.description].find((value):
value is string => typeof value === "string") which treats empty strings as
valid and blocks later non-empty values; update the predicate used by find to
only accept non-empty strings (e.g., typeof value === "string" && value.trim()
!== "") so summaryFromEvent will skip blank summaries and allow populated
finalSummary/text/description to be chosen before falling back to
existing?.summary or "".
- Around line 3156-3181: When history.sessionFound === false we clear session UI
state but don't reset the interrupted flag, which can leave a stale interrupted
state shown; update the branch that handles history.sessionFound === false (the
if branch using selectedSessionFound, setCurrentGoal, setContextPercent,
setTokenSummary, setStatusLineStats, eventCountRef, loadedSessionIdRef,
nextEvents) to also call setInterrupted(false) so interrupted is cleared
whenever the selected session is missing; keep the existing later logic that
sets setInterrupted(false) for the active selected session but ensure this
immediate branch explicitly resets interrupted to false.
- Around line 714-719: The resolveLaneReference function currently falls back to
the first partial match which can silently pick the wrong lane; update it so
after exact id/name matches are attempted, collect all lanes where
lane.name.toLowerCase().includes(normalized) into an array and only return that
lane if the array length is exactly 1, otherwise return null (i.e., reject
ambiguous partial matches); keep the initial exact-match checks (id/name)
unchanged and reference resolveLaneReference and LaneSummary when making this
change.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 3767-3784: The cache currently sets expiresAtMs when the request
starts so slow in-flight getProjectDetail requests can expire and be re-fetched;
change getCachedProjectDetail to keep in-flight promises cached by inserting the
entry with expiresAtMs = Infinity (or a sentinel) when creating promise, then on
promise.then set expiresAtMs = Date.now() + PROJECT_DETAIL_CACHE_TTL_MS and
update the cached.promise if still the same, and on promise.catch keep the
existing check that deletes the cache only if
projectDetailCache.get(rootPath)?.promise === promise; reference
getCachedProjectDetail, projectDetailCache, getProjectDetail,
PROJECT_DETAIL_CACHE_TTL_MS and the existing promise.catch to locate where to
apply these changes.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 3224-3247: Add unit tests in ptyService.test.ts covering two
missing cases: (1) disk-only path where liveEntryBySessionId returns undefined
or no live entry exists — mock sessionService.get to return a session and mock
sessionService.readTranscriptTail to return a non-empty disk tail, call
readTranscriptTail and assert the returned merged content equals the disk tail
(respecting maxBytes/clamping if relevant); (2) ANSI stripping when raw is
false/undefined — mock a disk tail (or merged result via
mergeTranscriptTailWithLiveOutput) containing ANSI sequences, ensure
liveEntryBySessionId is set or unset appropriately, call readTranscriptTail
without raw or with raw: false and assert the result has ANSI removed (i.e.,
compare to stripAnsi(diskTail)). Use the same helpers/mocks in existing tests
and reference readTranscriptTail, sessionService.readTranscriptTail,
liveEntryBySessionId, mergeTranscriptTailWithLiveOutput, and stripAnsi to locate
the code under test.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 2592-2601: The post-action cleanup for openRepo (and switchToPath)
is calling rememberProjectBinding(null) because clearAround's cleanup runs both
before and after the action; remove or move that nulling so it only runs before
the action. Specifically, update the clearAround calls that wrap
runProjectRuntimeTransition(() => ipcRenderer.invoke(IPC.projectOpenRepo, ...))
and the analogous switchToPath call so the pre-action callback still calls
rememberProjectBinding(null) and clearProjectScopedReadCaches(), but the
post-action callback does NOT call rememberProjectBinding(null) (leave other
safe post-cleanup tasks intact), preventing clobbering of a freshly updated
binding and restoring runtime routing/event pumping.
In `@apps/desktop/src/renderer/components/chat/chatExecutionSummary.ts`:
- Around line 84-90: The code is collapsing merged subagent snapshots by
overwriting snapshot.taskId with the latest event.taskId (via adoptEventTaskId),
which loses merged identity used by deriveSubagentTimeline; change the
assignment in snapshots.set so that when a parent-based match exists you
preserve the stable merged identity (use existing?.taskId or parentMatch's
taskId) instead of adopting event.taskId, and apply the same fix in the other
similar block (the later snapshot update around the second occurrence). Locate
findSnapshotByParent, snapshots, key, event.taskId, adoptEventTaskId and ensure
snapshot.taskId remains the merged id (existing?.taskId ?? event.taskId) rather
than replacing it when parentMatch is present.
In `@apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx`:
- Around line 791-800: The logic for baseChanged ignores the user clearing the
base field because it only considers non-empty input; update the computation so
an empty trimmed baseBranchInput counts as a change when the lane currently has
a base override. For example, change baseChanged (used by canApply) to true when
(1) baseOverrideTrim is non-empty and differs from lane.baseRef, or (2)
baseOverrideTrim is empty but lane.baseRef is set (i.e. clearing will remove the
override). Refer to baseOverrideTrim, baseBranchInput, baseChanged, canApply,
and lane.baseRef when making this change.
In `@apps/ios/ADE/Views/Lanes/LaneManageSheet.swift`:
- Around line 91-100: reparentBaseChanged currently ignores empty
trimmedBaseOverride so clearing the override doesn't count as a change; update
its logic to simply compare trimmedBaseOverride to snapshot.lane.baseRef (i.e.,
change reparentBaseChanged to "trimmedBaseOverride != snapshot.lane.baseRef") so
clearing the field is treated as a change and canApplyReparent will allow
resetting the base; apply the same fix to the duplicated logic mentioned (lines
270-277) and keep references to selectedParentLaneId and reparentParentChanged
unchanged.
In `@apps/ios/ADE/Views/PRs/PrsRootScreen.swift`:
- Around line 397-400: The onChange handler for githubStatusFilterRawValue and
the reload task both check githubExternalHistoryLoaded and can concurrently call
fetchGitHubPullRequestSnapshot(includeExternalClosed: true); to fix, add a
synchronous guard flag (e.g., isLoadingExternalHistory) that is set true before
awaiting any async fetch and cleared on completion, and update
loadGitHubExternalHistoryIfNeeded, reload (task with id:
prsProjectionReloadKey), and any other paths (e.g., the blocks around lines
referencing githubExternalHistoryLoaded and fetchGitHubPullRequestSnapshot) to
check/set this flag atomically so only one fetchGitHubPullRequestSnapshot runs
at a time; alternatively consolidate external history loading into a single
deduplicated function that uses the same isLoadingExternalHistory and
githubExternalHistoryLoaded flags.
---
Outside diff comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 2486-2512: The reparent flow allows newParentLaneId to be omitted
(it’s set from readValue(...) ?? firstPositional(...)) which defers validation;
update the CLI handler so that after building reparentArgs you validate that
reparentArgs.newParentLaneId is present and, if not, fail fast with a clear CLI
error (e.g., throw or return a user-facing error/exit) before creating the
execute step; modify the logic around reparentArgs, readValue, firstPositional,
and before the actionStep/collectGenericObjectArgs call to enforce the required
parent and surface a concise message like "missing required
--parent/--parent-lane-id" instead of letting downstream services handle it.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 12366-12404: The failure branch for spawn_agent (when
mapCodexCollabAgentStatus(item.status) === "failed") leaks the placeholder
stored under runtime.activeSubagents keyed by itemId because it only deletes per
taskId and uses runtime.activeSubagents.get(itemId) as a fallback; ensure the
placeholder is removed: before emitting subagent_result events, delete the
itemId key from runtime.activeSubagents (runtime.activeSubagents.delete(itemId))
so the placeholder doesn't persist when receiverIds is non-empty and spawn
fails; adjust the logic in the failed branch that iterates receiverIds/itemId
accordingly (functions/vars: spawn_agent, mapCodexCollabAgentStatus,
runtime.activeSubagents, itemId, receiverIds, emitChatEvent).
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 3434-3443: The rollback path currently performs two separate DB
ops (db.run("update lanes ...", [...]) and upsertBranchProfileForRow(...)) then
calls invalidateLaneListCache(), which can leave the DB inconsistent if the
second op fails; wrap the rollback updates in a single transaction so both the
lane restore and the branch-profile upsert succeed or both rollback: begin a
transaction, perform db.run(...) for restoring lane (use lane.id and projectId),
call upsertBranchProfileForRow(lane, { branchRef: lane.branch_ref, baseRef:
previousBaseRef, parentLaneId: previousParentLaneId }) inside the same
transaction, on success commit and then call invalidateLaneListCache(), and on
any error rollback and rethrow/log so the rollback is atomic.
- Around line 3415-3424: The update to the lanes table (db.run(...)) and the
subsequent upsertBranchProfileForRow(...) must be executed inside a single DB
transaction so they either both succeed or both roll back; wrap the sequence
(start transaction, perform db.run update, call upsertBranchProfileForRow, call
invalidateLaneListCache, commit) and ensure any thrown error causes a rollback
and the error to be propagated; use the same DB connection used for db.run and
upsertBranchProfileForRow, and handle errors to call rollback before rethrowing.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 5199-5210: The code falls back to steerMessage() when
isTurnAlreadyActiveError(sendError) occurs but leaves the optimistic
user_message bubble in state so steer events (which carry steerId) won't
reconcile it (see isMatchingOptimisticUserMessage()); before calling
steerMessage() clear the optimistic user_message/optimistic bubble from state
(invoke the existing state setter that removes the optimistic user message) so
the steer-backed events can render correctly, then proceed with steerMessage()
and the existing error handling (references: isTurnAlreadyActiveError,
steerMessage, isNoActiveTurnToSteerError, setTurnActiveBySession,
sendMessageOrSteerIfBusy, isMatchingOptimisticUserMessage).
---
Nitpick comments:
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 3384-3395: Add a brief inline comment above the early-return block
that explains this is a no-op optimization: when lane.parent_lane_id equals
persistedParentLaneId and newBaseRef equals previousBaseRef we skip doing any
rebase/parent-change work and simply return the current head SHA; place the
comment near the block referencing lane.parent_lane_id, persistedParentLaneId,
newBaseRef, previousBaseRef and the getHeadSha call so future readers understand
why we return unchanged preHeadSha/postHeadSha.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Line 99: The LIVE_TRANSCRIPT_TAIL_BUFFER_CHARS constant is too large and
causes heavy per-chunk string allocations in appendRecentOutput (called from
pty.onData); either reduce the cap (e.g., set LIVE_TRANSCRIPT_TAIL_BUFFER_CHARS
to 256*1024–512*1024) to bound per-call copies and per-session memory, or
replace the simple string-slice approach in appendRecentOutput and
readTranscriptTail with a chunk-list buffer that appends incoming chunks to an
array and only flattens (joins) to a single string when readTranscriptTail is
called, ensuring you update appendRecentOutput, the buffer storage used by
entry.recentOutputTail, and readTranscriptTail to manage chunk eviction to keep
the total buffered size under the new cap.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx`:
- Line 384: The module-level mutable variable iosEventListener breaks test
isolation; instead refactor installAdeMocks to return the listener (or an
accessor) so tests receive a test-scoped reference rather than mutating
iosEventListener directly; update tests that currently read/write
iosEventListener to call installAdeMocks() and use its returned listener
(similar to existing emitChatEvent and emitSessionChanged), remove the
module-scope iosEventListener declaration, and ensure beforeEach/afterEach use
the returned reference for cleanup.
In `@apps/ios/ADE/Views/PRs/PrsRootScreen.swift`:
- Around line 397-400: The onChange handler spawns an untracked Task for every
githubStatusFilterRawValue change which can lead to concurrent
loadGitHubExternalHistoryIfNeeded calls; add a cancellable Task holder (e.g.,
`@State` private var externalHistoryLoadTask: Task<Void, Never>?) and before
creating a new Task cancel the previous one, assign the new Task to
externalHistoryLoadTask, and ensure you clear externalHistoryLoadTask when the
Task completes or is cancelled; alternatively serialize calls via an actor or
dedicated queue in the code path that runs loadGitHubExternalHistoryIfNeeded and
reference githubStatusFilterRawValue and selectedGitHubStatusFilter to keep
existing guard logic.
🪄 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: f94d4f31-db36-47e3-bee8-dce4dcf1393c
⛔ Files ignored due to path filters (12)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/codex migration plan.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/lanes/stacking.mdis excluded by!docs/**docs/features/project-home/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (53)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/types.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/projects/projectBrowserService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatExecutionSummary.test.tsapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsxapps/desktop/src/renderer/components/lanes/laneDialogTokens.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/prsRouteState.tsapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/lanes.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneManageSheet.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/PRs/PrsRootScreen.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsx
Greptile P1/P2 + CodeRabbit 10 + Copilot review-only on commit 56a8469. - ManageLaneDialog + LaneManageSheet (iOS): normalize baseChanged compare (strip refs/heads/, origin/), and treat empty input as a change when an existing override is set so users can clear the base override. - ptyService: zero out recentOutputTail in closeEntry + dispose to release up-to-2MB tail buffer when a session ends. - ptyService.test: add coverage for disk-only readTranscriptTail and stripAnsi invocation on merged tail. - ade-cli app.tsx: require trimmed summary length > 0; reject ambiguous partial lane reference in resolveLaneReference; clear interrupted state when sessionFound === false. - registerIpc: keep in-flight projectGetDetail entries non-expiring until the promise settles, then start TTL from resolve time. - preload: do not null rememberProjectBinding inside clearAround's twice- running cleanup for openRepo / switchToPath. - chatExecutionSummary: preserve merged taskId on subagent_progress / subagent_result so timeline doesn't collapse to the latest entry. - PrsRootScreen (iOS): guard concurrent external history fetches behind isLoadingExternalHistory flag to remove race on filter change. addressed: 3246905124 3246905210 3246917318 3246917328 3246917331 3246917349 3246917363 3246917372 3246917378 3246917382 3246917386 3246917389 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Iter 2's baseChanged/reparentBaseChanged change made Apply enable on initial dialog open whenever lane.baseRef was non-empty, because lane.baseRef is a non-nullable string. That could fire an unintended rebase on an unsuspecting click. Now the empty-override branch flips true only when lane.baseRef actually diverges from the selected parent's default branch (compared with the existing normalizeBranchRefForCompare helper). Three states: - empty override, lane.baseRef == default → baseChanged false - empty override, lane.baseRef != default → baseChanged true (clear) - non-empty override differing from existing → baseChanged true Same fix applied to apps/ios/ADE/Views/Lanes/LaneManageSheet.swift. addressed: 3247162053 3247162141 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
May 15, 2026 09:42
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 `@apps/desktop/src/preload/preload.ts`:
- Around line 1214-1225: The current guard that throws when
projectRuntimeTransitionDepth > 0 is applied unconditionally for domain ===
"chat" (freshBinding), which blocks read-only chat operations; update the logic
in the helper around freshBinding/domain === "chat" so it does not throw for
read-only chat calls: either remove the transition guard here and relocate it
into mutating chat entrypoints (e.g., send, respondToInput, approve) or detect
read-only actions and immediately return { handled: false } so
callRemoteProjectActionIfBound and callLocalProjectActionIfBound can fallback to
IPC; keep references to freshBinding, projectRuntimeTransitionDepth, domain ===
"chat", callRemoteProjectActionIfBound, and callLocalProjectActionIfBound to
locate the code.
🪄 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: 6309f680-4840-4204-95b1-7f721944f5a9
⛔ Files ignored due to path filters (9)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/lanes/stacking.mdis excluded by!docs/**docs/features/project-home/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (49)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/types.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/projects/projectBrowserService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatExecutionSummary.test.tsapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsxapps/desktop/src/renderer/components/lanes/laneDialogTokens.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/lanes.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneManageSheet.swiftapps/ios/ADE/Views/PRs/PrsRootScreen.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsx
✅ Files skipped from review due to trivial changes (3)
- apps/ade-cli/README.md
- apps/ade-cli/src/tuiClient/commands.ts
- apps/ade-cli/src/services/sync/syncHostService.test.ts
🚧 Files skipped from review as they are similar to previous changes (41)
- apps/desktop/src/shared/types/chat.ts
- apps/desktop/src/main/services/projects/projectBrowserService.ts
- apps/desktop/src/renderer/components/lanes/LanesPage.tsx
- apps/ade-cli/src/adeRpcServer.test.ts
- apps/desktop/src/renderer/components/app/AppShell.tsx
- apps/ade-cli/src/tuiClient/tests/commands.test.ts
- apps/desktop/src/preload/preload.test.ts
- apps/desktop/src/shared/types/lanes.ts
- apps/ade-cli/src/tuiClient/types.ts
- apps/desktop/src/renderer/components/chat/chatExecutionSummary.test.ts
- apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsx
- apps/ade-cli/src/services/sync/syncHostService.ts
- apps/ade-cli/src/cli.test.ts
- apps/desktop/src/renderer/state/appStore.test.ts
- apps/ade-cli/src/adeRpcServer.ts
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx
- apps/ade-cli/src/tuiClient/components/RightPane.tsx
- apps/desktop/src/main/services/sync/syncHostService.test.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/renderer/components/app/TopBar.tsx
- apps/ios/ADE/Views/Lanes/LaneManageSheet.swift
- apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
- apps/ios/ADE/Views/PRs/PrsRootScreen.swift
- apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx
- apps/ade-cli/src/tuiClient/tests/appInput.test.ts
- apps/desktop/src/renderer/components/app/TopBar.test.tsx
- apps/desktop/src/main/services/pty/ptyService.ts
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/renderer/components/chat/chatExecutionSummary.ts
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/main/services/lanes/laneService.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
- apps/ade-cli/src/cli.ts
- apps/ios/ADE/Services/SyncService.swift
- apps/desktop/src/main/services/lanes/laneService.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/ade-cli/src/tuiClient/app.tsx
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/desktop/src/main/services/chat/agentChatService.ts
…t Major)
callProjectRuntimeActionIfBound previously threw for every chat-domain
call when projectRuntimeTransitionDepth > 0, including reads like list,
getSummary, getAvailableModels, and getEventHistory. That blocked the UI
from rendering chat summaries/history during a project switch.
Now only state-changing chat actions (MUTATING_CHAT_ACTIONS allowlist:
sendMessage, respondToInput, approveToolUse, interrupt, steer family,
session lifecycle, ensureCtoSession, ensureAgentIdentitySession, etc.)
throw the transition error. Read-only chat actions return
{handled:false} so the caller falls through to the IPC-backed read APIs.
Added preload.test coverage for read-only chat falling through to IPC
during an in-flight switchToPath.
addressed: 3247309433
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
May 15, 2026 10:18
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
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
Implements ADE-29 (reorganize lane stack) via Manage Lane, with responsive lane modals and clearer Manage Lane UX (what each section does, rebase disclosure, ADE-aligned panels).
Demo / screen capture (honest status)
An earlier PR revision attached an empty screen recording — that was not useful. Root cause: the recorder ran against a static desktop with no Electron window in focus, so the capture was blank.
Follow-up work in the agent environment:
ADE_DISABLE_HARDWARE_ACCEL=1,ADE_PROJECT_ROOT=/workspace,DISPLAY=:1)./tmp/computer-use/(e.g. lanes surface + command palette). Those are not a full reparent video.Practical takeaway: the best proof of stack moves is still on your Mac with a real repo (see steps below) — the cloud Linux lane is missing the “already seeded multi-lane + git remotes” comfort of a dev laptop, and GUI automation did not finish Create lane → Manage lane → reparent → stack dropdown end-to-end in one pass.
Steps to reproduce on your machine (recommended)
Parent lane vs base branch
parent_lane_id;nullwhen rooted on primary).base_ref): git branch name for ahead/behind and rebase target; optional override in Manage lane.Code changes
ReparentLaneArgs.stackBaseBranchRef;laneService.reparent+ no-op skip; tests.max-h, scroll).SECTION_HERO_CLASS_NAME,SECTION_ACCENT_CLASS_NAME.stackBaseBranchRefforlanes.reparent.Tests
npx vitest run src/main/services/lanes/laneService.test.ts -t "laneService reparent"Summary by CodeRabbit
Release Notes
New Features
Improvements
Greptile Summary
This PR implements ADE-29 lane stack reorganization via Manage Lane, adding
stackBaseBranchRefsupport tolaneService.reparent, a newStackPositionSectionUI on both desktop and iOS, and a suite of supporting fixes (stale-closure inTopBar,recentOutputTailmemory cleanup, chat steer/send retry hardening, project-switch binding reset, and external PR history lazy-loading on iOS).laneService.reparentaccepts an optionalstackBaseBranchRef; a no-op early-return skips git rebase when parent and base are unchanged, andupsertBranchProfileForRowis properly rolled back on rebase failure. Frontend/iOSbaseChanged/reparentBaseChangedlogic correctly normalizes branch refs before comparison, fixing the previously reported always-enabled Apply button.TopBartab-close callback reads latest state from refs/getState()instead of stale closures;AgentChatPaneadds robust send\u2192steer retry with stale-turn recovery;ptyServiceclears the 2 MBrecentOutputTailbuffer on session dispose.LaneManageSheetmirrors the desktop stack-position UI;PrsRootScreenadds lazy external-history loading with dedup guards.Confidence Score: 5/5
Safe to merge; all core reparent paths are correct and well-tested, and the one remaining finding is a cosmetic self-correcting race.
All core reparent paths — service, IPC, desktop UI, iOS sheet, CLI parsing — look correct and are backed by four new unit tests. Previously reported issues (always-enabled Apply button, recentOutputTail leak) are properly addressed. The only remaining finding is a brief overwrite of the iOS external PR history snapshot that self-corrects within the next poll cycle.
apps/ios/ADE/Views/PRs/PrsRootScreen.swift — concurrent snapshot-write race between reload and loadGitHubExternalHistoryIfNeeded
Important Files Changed
stackBaseBranchReftoreparent: normalizes override, resolves rebase target, adds no-op early-return when parent+base unchanged, and correctly rolls backupsertBranchProfileForRowon rebase failure.StackPositionSectionwith parent-lane select and optional base-branch override;baseChangedcorrectly normalizes vianormalizeBranchRefForCompareto mirror backend.getCachedProjectDetailwith 10 s TTL, in-flight dedup, and.catcheviction of failed entries. Wires newlanes.reparentIPC handler forstackBaseBranchRef.recentOutputTaillive-buffer perPtyEntry, cleared on dispose to prevent 2 MB-per-session leak; exposesreadTranscriptTailwith KMP overlap dedup.reparentBaseChangednow compares normalized values againstnormalizedDefault, fixing the previously reported always-true bug.isLoadingExternalHistory+loadGitHubExternalHistoryIfNeededfor lazy external PR history; narrow race allowsreloadto briefly overwrite external history, self-correcting on next poll.useAppStore.getState().projectTransitionBlocksChatplaceholder,clearSessionViewhelper, robust send→steer retry logic, andsessionFoundguard inloadHistory.stackBaseBranchRefoverride, rebase-failure rollback with branch-profile restoration, and no-op skip.Sequence Diagram
sequenceDiagram participant UI as ManageLaneDialog participant IPC as registerIpc participant SVC as laneService.reparent participant GIT as git participant DB as SQLite UI->>IPC: "lanes.reparent({laneId, newParentLaneId, stackBaseBranchRef?})" IPC->>SVC: reparent(args) SVC->>SVC: normalize stackBaseBranchRef alt parent+base unchanged SVC-->>IPC: no-op result else change detected SVC->>GIT: resolveBranchRebaseTarget GIT-->>SVC: targetSha SVC->>DB: UPDATE lanes + upsertBranchProfile SVC->>GIT: git rebase targetSha alt success SVC-->>IPC: ReparentLaneResult else failure SVC->>DB: ROLLBACK lanes + upsertBranchProfile SVC-->>IPC: throw error end end IPC-->>UI: result or errorPrompt To Fix All With AI
Reviews (5): Last reviewed commit: "ship: iter 4 — exempt read-only chat fro..." | Re-trigger Greptile