Agent: bind sessions to lane worktrees; rebase#107
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds lane launch context resolution and binds PTYs, agent prompts, and runtimes to validated lane worktrees; implements lane-directive plumbing for agent chats; introduces auto-rebase sweeping, push/rollback and a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 (4)
apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx (1)
201-225:⚠️ Potential issue | 🟡 MinorThe failed-lane CTA drops the lane the user clicked.
In
LanesPage.tsx,onViewRebaseDetailsjust navigates to/prs?tab=rebase, so this row-specific button loses lane context and ends up behaving the same as the generic “View rebase details” action below it. With multiple failed lanes, users have to rediscover which lane needs follow-up. Please deep-link the failed lane (for example withlaneId) or remove the duplicate row-level CTA.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx` around lines 201 - 225, The row-level "OPEN REBASE TAB" button currently calls onViewRebaseDetails without context and drops the clicked lane; update the button to deep-link the specific lane by passing the lane id (e.g., call onViewRebaseDetails(status.laneId) or have it navigate to /prs?tab=rebase&laneId=<id>) so the rebase details view is lane-specific, or remove the duplicate row-level CTA; locate the button in LaneRebaseBanner.tsx where onViewRebaseDetails is used and ensure it accepts and uses a laneId (and update LanesPage.tsx handler if necessary) so failed-lane clicks retain lane context.apps/desktop/src/main/services/chat/agentChatService.ts (3)
7628-7631:⚠️ Potential issue | 🟠 MajorNew identity sessions keep the canonical lane’s
headShaStart.
createSession()recordsheadShaStartbeforeensureIdentitySession()immediately rebinds the fresh session toselectedExecutionLaneId. Since turn completion now usesresolveManagedExecutionLaneId(managed), a session created while a non-primary lane is selected can start and end with SHAs from different lanes on its very first turn.One way to realign the start SHA after rebinding
const managed = ensureManagedSession(created.id); managed.selectedExecutionLaneId = selectedExecutionLaneId; + if (selectedExecutionLaneId && selectedExecutionLaneId !== canonicalLaneId) { + const headStart = await computeHeadShaBestEffort(selectedExecutionLaneId).catch(() => null); + if (headStart) { + sessionService.setHeadShaStart(created.id, headStart); + } + } refreshReconstructionContext(managed, { includeConversationTail: usesIdentityContinuity(managed) });Also applies to: 8531-8534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 7628 - 7631, createSession currently computes headStart before ensureIdentitySession rebinds the session to selectedExecutionLaneId, causing headShaStart to come from the canonical lane instead of the bound execution lane; move the computeHeadShaBestEffort call (or recompute headStart) to after ensureIdentitySession (or after the rebinding logic) so that sessionService.setHeadShaStart(sessionId, headStart) uses the lane returned by ensureIdentitySession/selectedExecutionLaneId (or use resolveManagedExecutionLaneId(managed) to derive the correct lane before computing the SHA). Apply the same fix to the other occurrence around lines 8531-8534 where headShaStart is set.
4041-4054:⚠️ Potential issue | 🟠 Major
lastLaneDirectiveKeycan advance before any lane directive actually reaches the runtime.
prepareSendMessage()still returns alaneDirectiveKeyfor literal slash commands that skipcomposeLaunchDirectives(), andemitPreparedUserMessage()persists that key beforereview/start,turn/start,streamText(...), orv2Session.send(...)has succeeded. A first-turn/reviewor any early dispatch failure will therefore suppress the real lane directive on the next turn, and Claude can also reusesdkSessionIdunder the wrong assumption that the binding was already delivered.Possible direction for the slash-command half
- const laneDirectiveKey = executionContext.laneDirectiveKey; - const shouldInjectLaneDirective = laneDirectiveKey != null && managed.lastLaneDirectiveKey !== laneDirectiveKey; + const candidateLaneDirectiveKey = executionContext.laneDirectiveKey; + const shouldInjectLaneDirective = !isLiteralSlashCommand(trimmed) + && candidateLaneDirectiveKey != null + && managed.lastLaneDirectiveKey !== candidateLaneDirectiveKey; … - laneDirectiveKey, + laneDirectiveKey: shouldInjectLaneDirective ? candidateLaneDirectiveKey : null,After that, the
lastLaneDirectiveKeywrite should move out ofemitPreparedUserMessage()and into the provider-specific post-send success path.Also applies to: 7817-7851
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 4041 - 4054, emitPreparedUserMessage currently advances and persists managed.lastLaneDirectiveKey as soon as prepareSendMessage returns a laneDirectiveKey, which can occur for literal slash commands before any provider-side delivery (composeLaunchDirectives/review/start/turn/start/streamText/v2Session.send) succeeds; move the assignment and persistChatState(managed) out of emitPreparedUserMessage and instead set and persist lastLaneDirectiveKey only in the provider-specific post-send success paths (e.g., after successful review/start, turn/start, streamText completion, or v2Session.send success) so that the key advances only when the directive was actually delivered.
4918-4923:⚠️ Potential issue | 🟠 MajorQueued steers bypass lane rebinding.
These recursive calls skip
prepareSendMessage(), so a steer that drains after an execution-lane switch keeps the oldmanaged.laneWorktreePathand never recomputes the lane directive. For identity chats, the queued follow-up can run tools in the stale worktree.Also applies to: 5607-5612
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 4918 - 4923, Queued steers are being executed directly with runClaudeTurn and therefore bypass prepareSendMessage, causing stale managed.laneWorktreePath after an execution-lane switch; change the queued-steer handling so each steer goes through prepareSendMessage (or calls the same lane-rebinding logic) before invoking runClaudeTurn, i.e., for runtime.pendingSteers.shift() ensure you call prepareSendMessage(managed, { promptText: steerText, displayText: steerText, attachments: [] }) (or trigger the lane recompute helper used by prepareSendMessage) and only then call runClaudeTurn with the prepared message so the laneWorktreePath and directives are up-to-date for identity chats and tool usage.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/prs/prService.ts (1)
796-800: Consider caching the lane list instead of re-fetching on each iteration.Line 796 re-fetches all lanes after each child reparent, making the loop O(n²) in the number of children. For most cases this is fine, but if you expect stacks with many children, you could refactor to fetch once and update the local map.
♻️ Optional: Cache lane list and update locally
+ const refreshedLaneById = new Map(allLanes.map((lane) => [lane.id, lane])); + for (const child of directChildren) { // ... existing code ... try { reparentResult = await laneService.reparent({ laneId: child.id, newParentLaneId: successorParent.id, }); - const refreshedChild = (await laneService.list({ includeArchived: true })).find((lane) => lane.id === child.id) ?? { + // Invalidate and refresh cache once + laneService.invalidateCache?.(); + const refreshedLanes = await laneService.list({ includeArchived: true }); + for (const lane of refreshedLanes) { + refreshedLaneById.set(lane.id, lane); + } + const refreshedChild = refreshedLaneById.get(child.id) ?? { ...child, parentLaneId: successorParent.id, baseRef: successorParent.branchRef, };🤖 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 796 - 800, The code re-fetches all lanes inside the loop via laneService.list to compute refreshedChild, making the loop O(n²); instead, call laneService.list once before iterating, build a Map keyed by lane.id, and on each reparent/update modify that local Map (updating the child entry's parentLaneId and baseRef) so refreshedChild is obtained from the Map rather than re-calling laneService.list; update the existing references to refreshedChild, laneService.list, child, and successorParent in prService.ts accordingly and only call laneService.list again if a full refresh is explicitly required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/main.ts`:
- Line 824: createAutoRebaseService currently leaves pending timers in its
internal queueByRoot map and exports no cleanup; add a dispose/stop method on
the object returned by createAutoRebaseService that iterates queueByRoot, calls
clearTimeout on each timer id, and clears the map to prevent callbacks after
context shutdown. Export/use that new method from wherever autoRebaseService is
created and call it from disposeContextResources() alongside the other service
disposers (so timers are cleared before db.close()/context teardown). Ensure the
method name (e.g., dispose or stop) is added to the autoRebaseService interface
so callers can invoke it.
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 231-240: recordAttentionStatus currently writes an explicit
failure state then immediately calls emit(), but emit() uses listStatuses()
which prunes non-autoRebased entries when a lane isn't behind, causing newly
written attention states to be dropped; fix by making emit accept a flag to
bypass pruning (e.g., emit({ includeAttentionRegardlessOfBehind: true })) or add
a dedicated emitAttentionUpdate helper that calls listStatuses({ includeAll:
true }) so it returns recently set states, then call that from
recordAttentionStatus; update emit/listStatuses signatures and any callers
accordingly and reference the functions recordAttentionStatus, setStatus, emit,
and listStatuses when implementing the change.
- Around line 208-223: The helper maybeSweepRoots currently returns immediately
when sweepInFlight is true, which drops forced refreshes; change the
implementation so a forced call waits for the in-flight sweep to complete and
then proceeds. Concretely: replace or augment the boolean sweepInFlight with a
sweepPromise (or add a sweepPromise alongside sweepInFlight) in maybeSweepRoots
so that when a sweep is started you set sweepPromise = (async () => { ... })(),
and when maybeSweepRoots is invoked with options?.force === true and
sweepPromise exists you await sweepPromise before continuing to call
conflictService.scanRebaseNeeds() and queueRootsFromNeeds(...). Ensure you still
set/reset sweepPromise (or sweepInFlight) in try/finally and preserve existing
debounce logic for non-forced calls.
- Around line 319-329: The code currently treats an error from
conflictService.getRebaseNeed() as "no need" and clears non-autoRebased status;
instead, detect lookup failures and preserve current state (or surface a
transient lookup failure) rather than clearing. Modify the catch around
getRebaseNeed to set a local flag (e.g., lookupFailed = true) or return a
sentinel, and then change the if (!need) block to skip clearStatus(lane.id) when
lookupFailed (or call a new setStatus(lane.id, { state: "lookupFailed" }) if you
prefer surfacing the error); keep function names conflictService.getRebaseNeed,
loadStatus, clearStatus and the "autoRebased" check intact when applying this
change.
- Around line 191-205: queueRootsFromNeeds currently collapses every need to the
topmost ancestor via resolveRootLaneId, which over-serializes unrelated sibling
branches; change it to queue the affected chain/subtree instead of the global
root by deduplicating and enqueuing the nearest-affected lane id for each need
(e.g. use need.laneId or compute a chain-root that stops at the first ancestor
that represents the independent branch) rather than resolveRootLaneId(lane.id,
laneById), so replace the resolveRootLaneId call with logic that returns the
chain-level id per need and call queueRoot({ rootLaneId: thatId, reason:
`sweep:${reason}` }); ensure any deduplication still uses a Set of those chain
ids and keep processRoot blocking semantics local to each queued subtree if
processRoot assumes a single blocked state.
In `@apps/desktop/src/main/services/lanes/laneLaunchContext.ts`:
- Around line 55-60: The catch currently maps all failures from
resolvePathWithinRoot(...) to an "escapes lane" error; change the handler to
catch the error as e and only convert it to the lane-escape message when the
error indicates a path traversal escape (e.g., an error type or sentinel thrown
by resolvePathWithinRoot such as PathEscapeError or a specific
error.code/message). For other failures (missing directory, permission, etc.)
rethrow the original error or throw a new error preserving the original error
message/details so missing directories are reported correctly; reference
resolvePathWithinRoot, resolvedCwd, requestedCwd/requestedTarget, laneRoot, and
laneId when updating the catch logic.
In `@apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts`:
- Around line 125-132: In listSuggestions(), avoid fetching the same remote
tracking branch for each sibling by memoizing primary parent head resolution:
create a local Map keyed by parent.branchRef.trim() at the start of the scan,
and in the block where you handle parent.laneType === "primary" check the map
first; if absent call fetchRemoteTrackingBranch(...) and
readRefHeadSha(`origin/${parentBranch}`) (propagate/catch errors the same way),
then store parentHeadSha in the map and reuse it for subsequent lanes with the
same parentBranch; reference the functions/variables fetchRemoteTrackingBranch,
readRefHeadSha, listSuggestions, and parent.branchRef when implementing this
cache.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 504-510: The PTY session must persist the bound worktree/cwd from
resolveLaneLaunchContext so later lookups don't recompute a possibly different
lane; after resolveLaneLaunchContext (using launchContext.laneWorktreePath and
cwd) store those values on the PTY entry object (e.g., add laneWorktreePath and
boundCwd fields when creating the PTY entry), then update
summarizeSessionBestEffort(...) and the terminal title-generation path to read
and use those stored fields instead of calling
laneService.getLaneBaseAndBranch(...); ensure all references that previously
recomputed the worktree/head SHA fall back to the stored launchContext values so
the session is attributed to the actual worktree it started in.
In `@apps/desktop/src/renderer/components/run/RunPage.tsx`:
- Line 183: effectiveLaneId can be null which lets the runtime subscription skip
lane filtering and causes cross-lane events to leak; change the logic so
effectiveLaneId is undefined (not null) when no lane is selected or update the
runtime subscription filter to explicitly ignore null/undefined (e.g., only
append events when event.laneId === effectiveLaneId) — locate the
effectiveLaneId assignment (uses runLaneId and selectedLaneId) and the runtime
subscription/useEffect that appends runtimes and enforce a strict check for a
real lane id before adding events.
In `@apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx`:
- Around line 150-169: The toggle in LaneBehaviorSection remains interactive
while saveAutoRebaseSettings() is running, causing race conditions with
refresh(); disable the toggle when busy by wiring its disabled prop to the same
busy state used by the footer buttons (or short-circuit its onChange to return
if busy), so updates cannot be made during the save; update the toggle control
(the auto-rebase toggle handled alongside saveAutoRebaseSettings/refresh) to
check busy and either set disabled={busy} or ignore clicks when busy.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 7628-7631: createSession currently computes headStart before
ensureIdentitySession rebinds the session to selectedExecutionLaneId, causing
headShaStart to come from the canonical lane instead of the bound execution
lane; move the computeHeadShaBestEffort call (or recompute headStart) to after
ensureIdentitySession (or after the rebinding logic) so that
sessionService.setHeadShaStart(sessionId, headStart) uses the lane returned by
ensureIdentitySession/selectedExecutionLaneId (or use
resolveManagedExecutionLaneId(managed) to derive the correct lane before
computing the SHA). Apply the same fix to the other occurrence around lines
8531-8534 where headShaStart is set.
- Around line 4041-4054: emitPreparedUserMessage currently advances and persists
managed.lastLaneDirectiveKey as soon as prepareSendMessage returns a
laneDirectiveKey, which can occur for literal slash commands before any
provider-side delivery
(composeLaunchDirectives/review/start/turn/start/streamText/v2Session.send)
succeeds; move the assignment and persistChatState(managed) out of
emitPreparedUserMessage and instead set and persist lastLaneDirectiveKey only in
the provider-specific post-send success paths (e.g., after successful
review/start, turn/start, streamText completion, or v2Session.send success) so
that the key advances only when the directive was actually delivered.
- Around line 4918-4923: Queued steers are being executed directly with
runClaudeTurn and therefore bypass prepareSendMessage, causing stale
managed.laneWorktreePath after an execution-lane switch; change the queued-steer
handling so each steer goes through prepareSendMessage (or calls the same
lane-rebinding logic) before invoking runClaudeTurn, i.e., for
runtime.pendingSteers.shift() ensure you call prepareSendMessage(managed, {
promptText: steerText, displayText: steerText, attachments: [] }) (or trigger
the lane recompute helper used by prepareSendMessage) and only then call
runClaudeTurn with the prepared message so the laneWorktreePath and directives
are up-to-date for identity chats and tool usage.
In `@apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx`:
- Around line 201-225: The row-level "OPEN REBASE TAB" button currently calls
onViewRebaseDetails without context and drops the clicked lane; update the
button to deep-link the specific lane by passing the lane id (e.g., call
onViewRebaseDetails(status.laneId) or have it navigate to
/prs?tab=rebase&laneId=<id>) so the rebase details view is lane-specific, or
remove the duplicate row-level CTA; locate the button in LaneRebaseBanner.tsx
where onViewRebaseDetails is used and ensure it accepts and uses a laneId (and
update LanesPage.tsx handler if necessary) so failed-lane clicks retain lane
context.
---
Nitpick comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 796-800: The code re-fetches all lanes inside the loop via
laneService.list to compute refreshedChild, making the loop O(n²); instead, call
laneService.list once before iterating, build a Map keyed by lane.id, and on
each reparent/update modify that local Map (updating the child entry's
parentLaneId and baseRef) so refreshedChild is obtained from the Map rather than
re-calling laneService.list; update the existing references to refreshedChild,
laneService.list, child, and successorParent in prService.ts accordingly and
only call laneService.list again if a full refresh is explicitly required.
🪄 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: 4146c3fb-7eab-49b6-9222-d6bb543a8a33
📒 Files selected for processing (26)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/lanes/autoRebaseService.test.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/lanes/laneLaunchContext.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.tsapps/desktop/src/main/services/prs/prService.landAutoRebase.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/prs/PrRebaseBanner.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/shared/types/lanes.ts
| aiIntegrationService, | ||
| projectConfigService, | ||
| conflictService, | ||
| autoRebaseService, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
AUTO_FILE="$(fd 'autoRebaseService.ts$' apps/desktop/src/main/services/lanes | head -n 1)"
if [[ -z "${AUTO_FILE}" ]]; then
exit 1
fi
printf '== autoRebaseService timers / teardown ==\n'
rg -n -C3 'setTimeout|setInterval|clearTimeout|clearInterval|debounce|dispose|stop' "$AUTO_FILE"
printf '\n== project context teardown ==\n'
sed -n '2340,2475p' apps/desktop/src/main/main.tsRepository: arul28/ADE
Length of output: 3731
🏁 Script executed:
# Find the autoRebaseService file and check for dispose/stop method definitions
fd 'autoRebaseService.ts$' apps/desktop/src/main/services/lanes --exec sh -c '
echo "=== Full autoRebaseService structure ==="
wc -l "$1"
echo ""
echo "=== Method/function definitions ==="
rg -n "^\s*(async\s+)?(function|export|const|declare)\s+" "$1" | head -20
echo ""
echo "=== dispose/stop method search ==="
rg -n "dispose|stop" "$1" | grep -E "(function|const|async|=>)" | head -10
' sh {} \;Repository: arul28/ADE
Length of output: 1614
🏁 Script executed:
# Also check AppContext type definition to see if autoRebaseService is typed
rg -n "autoRebaseService" apps/desktop/src/main/main.ts | head -15Repository: arul28/ADE
Length of output: 490
🏁 Script executed:
# Check the createAutoRebaseService export and return type
rg -n "export.*createAutoRebaseService|function createAutoRebaseService" apps/desktop/src/main/services/lanes/autoRebaseService.ts -A 30Repository: arul28/ADE
Length of output: 956
🏁 Script executed:
# Check if the service has any cleanup/disposal methods in its interface
rg -n "dispose|stop|cleanup|teardown|unsubscribe|off|removeListener" apps/desktop/src/main/services/lanes/autoRebaseService.tsRepository: arul28/ADE
Length of output: 259
🏁 Script executed:
# Check how autoRebaseService is set to null in the context cleanup (line 2276)
sed -n '2270,2285p' apps/desktop/src/main/main.tsRepository: arul28/ADE
Length of output: 524
🏁 Script executed:
# Get the full createAutoRebaseService function return statement
sed -n '68,474p' apps/desktop/src/main/services/lanes/autoRebaseService.ts | tail -100Repository: arul28/ADE
Length of output: 3377
🏁 Script executed:
# Search for return statements in autoRebaseService
rg -n "return {" apps/desktop/src/main/services/lanes/autoRebaseService.ts -A 20Repository: arul28/ADE
Length of output: 133
Add cleanup hook for autoRebaseService timers and wire into disposeContextResources().
The service maintains a queueByRoot map with pending setTimeout calls but exports no dispose/stop method. Without cleanup, timers can fire after the lane project context closes—potentially writing to the database after db.close() or operating on stale state during project switches.
Add a cleanup method to createAutoRebaseService that clears all pending timers (iterate queueByRoot and clearTimeout each), export it, and call it in disposeContextResources() alongside the other service disposal calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/main.ts` at line 824, createAutoRebaseService currently
leaves pending timers in its internal queueByRoot map and exports no cleanup;
add a dispose/stop method on the object returned by createAutoRebaseService that
iterates queueByRoot, calls clearTimeout on each timer id, and clears the map to
prevent callbacks after context shutdown. Export/use that new method from
wherever autoRebaseService is created and call it from disposeContextResources()
alongside the other service disposers (so timers are cleared before
db.close()/context teardown). Ensure the method name (e.g., dispose or stop) is
added to the autoRebaseService interface so callers can invoke it.
apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/renderer/components/run/RunPage.tsx (1)
646-647:⚠️ Potential issue | 🟡 MinorLane selector allows empty selection when
effectiveLaneIdis null.When
effectiveLaneIdisnull, the select shows an empty value (""), but there's no<option>element with an empty value. This could cause the dropdown to show the first lane as selected visually while the internal state isnull, creating a confusing UI mismatch.🔧 Suggested fix: Add a placeholder option or sync state
<select value={effectiveLaneId ?? ""} onChange={(e) => selectRunLane(e.target.value || null)} style={{ height: 28, padding: "0 8px", background: COLORS.recessedBg, border: `1px solid ${COLORS.outlineBorder}`, borderRadius: 0, fontFamily: MONO_FONT, fontSize: 11, color: COLORS.textPrimary, outline: "none", minWidth: 120, }} > + {!effectiveLaneId && ( + <option value="" disabled> + Select lane + </option> + )} {lanes.map((lane) => ( <option key={lane.id} value={lane.id}> {lane.name} </option> ))} </select>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/run/RunPage.tsx` around lines 646 - 647, The select uses value={effectiveLaneId ?? ""} and onChange calls selectRunLane, causing a mismatch when effectiveLaneId is null because there is no <option> with value "". Add a placeholder option with an empty value (for example a disabled "Select lane" or "None" option) to the select so the UI reflects the null state, and ensure selectRunLane is invoked with null when the empty option is chosen; update RunPage.tsx where effectiveLaneId and selectRunLane are used.apps/desktop/src/renderer/components/terminals/TerminalView.tsx (1)
863-871:⚠️ Potential issue | 🟠 MajorBlur focused terminals before parking them.
Now that live runtimes stay parked until the PTY exits, this cleanup path can move a focused terminal into the hidden parking root and leave it alive there. Keystrokes after navigation can still go to that invisible terminal unless you clear focus before
parkRuntime(runtime).🔧 Minimal guard
if (runtime.host.parentElement === el) { + const activeElement = document.activeElement; + if (activeElement instanceof HTMLElement && runtime.host.contains(activeElement)) { + activeElement.blur(); + } parkRuntime(runtime); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx` around lines 863 - 871, Before calling parkRuntime(runtime) in the cleanup path where you check if runtime.host.parentElement === el, ensure the terminal element loses focus so keystrokes do not go to a parked-but-still-alive runtime; call the host element's blur() (or otherwise remove document.activeElement if it equals runtime.host) prior to parkRuntime(runtime). Apply this change in the block that updates runtime.refs and checks runtime.exitCode (the same block that calls scheduleRuntimeDispose(runtime, EXITED_RUNTIME_KEEPALIVE_MS)) so the focused terminal is cleared before being moved to the hidden parking root.apps/desktop/src/main/services/chat/agentChatService.test.ts (1)
239-267:⚠️ Potential issue | 🟡 MinorKeep the mock lane lookup consistent with dynamically created lanes.
create()gives new lanes their owngenerated-lane-Nworktree, butgetLaneBaseAndBranch()still only recognizeslane-1andlane-2and falls back totmpRootfor every other lane. Any path that creates a lane and then resolves launch context will exercise the wrong worktree, which can hide regressions in the new lane-binding coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.test.ts` around lines 239 - 267, The mock's getLaneBaseAndBranch currently only recognizes hardcoded "lane-1" and "lane-2" and falls back to tmpRoot for any newly created lanes, causing tests that create lanes via create() to resolve the wrong worktree; change getLaneBaseAndBranch to look up the lane in the shared lanes array (or update laneRoots when create() adds a lane) and return that lane's worktreePath, laneType and branchRef accordingly so newly created generated-lane-N entries are properly resolved during tests.apps/desktop/src/main/services/chat/agentChatService.ts (1)
7851-7882:⚠️ Potential issue | 🟠 MajorDon't mark slash-command turns as having delivered the lane directive.
For literal slash commands,
promptTextskipscomposeLaunchDirectives(), butlaneDirectiveKeystill flows downstream and later gets persisted. A session opened with/review,/login, etc. will therefore suppress the first real lane-binding prompt on the next normal turn.🛠️ Suggested fix
- const promptText = isLiteralSlashCommand(trimmed) - ? trimmed - : composeLaunchDirectives(trimmed, [ - shouldInjectLaneDirective - ? buildLaneWorktreeDirective({ - laneId: executionContext.laneId, - laneWorktreePath: executionContext.laneWorktreePath, - }) - : null, + const laneDirective = !isLiteralSlashCommand(trimmed) && shouldInjectLaneDirective + ? buildLaneWorktreeDirective({ + laneId: executionContext.laneId, + laneWorktreePath: executionContext.laneWorktreePath, + }) + : null; + const promptText = isLiteralSlashCommand(trimmed) + ? trimmed + : composeLaunchDirectives(trimmed, [ + laneDirective, buildExecutionModeDirective(executionMode, managed.session.provider), buildClaudeInteractionModeDirective(managed.session.interactionMode, managed.session.provider), buildComputerUseDirective( managed.session.computerUse, computerUseArtifactBrokerRef?.getBackendStatus() ?? null, ), ]); @@ - laneDirectiveKey: shouldInjectLaneDirective ? laneDirectiveKey : null, + laneDirectiveKey: laneDirective ? laneDirectiveKey : null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 7851 - 7882, The literal slash-command branch currently bypasses composeLaunchDirectives but still propagates laneDirectiveKey, causing slash commands to mark the lane directive as delivered; change the logic so that when isLiteralSlashCommand(trimmed) is true you do not propagate laneDirectiveKey (set laneDirectiveKey to null for that return), otherwise keep the existing shouldInjectLaneDirective ? laneDirectiveKey : null flow — update where promptText and laneDirectiveKey are computed/returned (symbols: isLiteralSlashCommand, promptText, composeLaunchDirectives, laneDirectiveKey, shouldInjectLaneDirective) so slash commands no longer suppress the next real lane-binding prompt.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/components/run/ProcessMonitor.tsx (3)
79-93: Log accumulation may grow unbounded during long sessions.The log event handler appends chunks to
logTextindefinitely:setLogText((prev) => normalizeLog(`${prev}${event.chunk}`));For long-running processes, this could cause memory issues. Consider truncating the in-memory log when it exceeds a threshold (e.g., keep the last
LOG_TAIL_MAX_BYTES).♻️ Suggested fix: Truncate log to prevent unbounded growth
React.useEffect(() => { const unsubscribe = window.ade.processes.onEvent((event: ProcessEvent) => { if (event.type !== "log") return; if (event.laneId !== laneIdRef.current) return; if (event.processId !== activeProcessIdRef.current) return; - setLogText((prev) => normalizeLog(`${prev}${event.chunk}`)); + setLogText((prev) => { + const updated = normalizeLog(`${prev}${event.chunk}`); + // Keep only the last ~220KB to prevent memory issues + if (updated.length > LOG_TAIL_MAX_BYTES) { + return updated.slice(-LOG_TAIL_MAX_BYTES); + } + return updated; + }); }); return () => { try { unsubscribe(); } catch { // ignore } }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx` around lines 79 - 93, The log handler in ProcessMonitor's useEffect currently appends chunks indefinitely via setLogText(prev => normalizeLog(`${prev}${event.chunk}`)); modify this to truncate the in-memory log once it exceeds a threshold: define or import LOG_TAIL_MAX_BYTES, append the new chunk to prev, then if the combined string length (or byte length) is > LOG_TAIL_MAX_BYTES slice to keep only the tail (e.g., last LOG_TAIL_MAX_BYTES characters/bytes) before passing it to normalizeLog; keep the existing guards for laneIdRef and activeProcessIdRef and update only the setLogText call inside the window.ade.processes.onEvent handler so memory usage is bounded.
38-43: Minor:formatEndedAtcould use locale-aware formatting.The function uses
toLocaleTimeStringwhich is good, but hardcodes the format options. For better i18n support, consider making this configurable or using the user's full locale preferences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx` around lines 38 - 43, The formatEndedAt function currently hardcodes toLocaleTimeString options; update formatEndedAt to use the user's locale and make format options configurable (e.g., accept an optional locale or options param or read navigator.language) so it respects user preferences and i18n; locate the function formatEndedAt and change the toLocaleTimeString call to use undefined or the runtime locale (navigator.language or a passed-in locale) and consider exposing the time-format options via a parameter or context so callers can override the hour/minute formatting.
23-29: Consider extracting shared status helpers to avoid duplication.
isActiveStatusduplicates the logic fromisActiveinCommandCard.tsx, andhasInspectableOutput/formatProcessStatusfollow similar patterns. Consider extracting these to a shared utility file (e.g.,processUtils.ts) to maintain consistency and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx` around lines 23 - 29, isActiveStatus and hasInspectableOutput duplicate logic already implemented elsewhere (e.g., isActive in CommandCard.tsx and formatProcessStatus); extract these helpers into a shared module (e.g., processUtils.ts) and update usages to import from that module. Specifically, move the implementations of isActiveStatus, hasInspectableOutput, and any formatProcessStatus-like logic into a single exported file and replace local definitions in ProcessMonitor.tsx and CommandCard.tsx to import the shared functions to keep behavior consistent and remove duplication.apps/desktop/src/main/services/pty/ptyService.test.ts (1)
24-35: Model realpath behavior in these lane-boundary tests.
resolvePathWithinRoot()only differs from a plainpath.resolve()when a realpath hop occurs. This mock never does that, so the new cwd rejection tests won't catch production-only cases like/tmpvs/private/tmpon macOS or a child symlink that points outside the lane. Add at least one case that overridesrealpathSyncfor an in-lane child to resolve outside the lane and expects launch rejection.Also applies to: 280-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.test.ts` around lines 24 - 35, The lane-boundary tests currently mock realpathSync to behave identically to path.resolve so resolvePathWithinRoot() never experiences a realpath hop; modify the tests in ptyService.test.ts to add at least one scenario where the mocked realpathSync for a specific in-lane child path returns a resolved path outside the lane (e.g., map "/tmp/child" -> "/private/tmp/child" or similar) and assert that resolvePathWithinRoot()/the launcher rejects launching that path; update the realpathSync mock (the Object.assign vi.fn(...) with .native) to handle a special-case input by returning the external path and ensure the test expects launch rejection to cover production-only realpath hops.apps/desktop/src/main/services/lanes/autoRebaseService.test.ts (1)
891-914: Consider asserting the final lane status after rollback.The test correctly verifies that
rebaseRollbackis called when auto-push fails. However, it would strengthen the test to also assert the final state of the lane's status record (e.g., that it remains or reverts torebasePending).💡 Suggested enhancement
expect(laneService.rebaseRollback).toHaveBeenCalledWith({ runId: "run-1" }); + + // Verify lane status reflects the rollback (e.g., still pending or appropriate failure state) + const finalStatus = db.getJson("auto_rebase:status:child-1") as AutoRebaseLaneStatus | null; + expect(finalStatus?.state).toBe("rebasePending");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/autoRebaseService.test.ts` around lines 891 - 914, Add an assertion after the existing expect(laneService.rebaseRollback) check to verify the lane status for "child-1" was set to the expected pending/reverted state (e.g., rebasePending true or the specific status object used by the code). Locate the test "rolls back a successful rebase when auto-push fails and leaves the lane pending" and, after the calls to service.onHeadChanged and timers, read the lane from laneList or the service/state accessor used in other tests and assert its status fields (such as rebasePending or rebaseInProgress) match the expected post-rollback values for the lane with id "child-1". Ensure you reference the same status shape used elsewhere in tests (e.g., status: { rebasePending: true, rebaseInProgress: false } or the canonical field names) so the assertion aligns with laneService.rebaseRollback being called.apps/desktop/src/main/services/lanes/autoRebaseService.ts (1)
371-379: Consider defensive handling forgetHeadShacalls in status-setting paths.The
await getHeadSha(parent.worktreePath)calls at lines 374, 396, and 439 can throw if the worktree is in an unexpected state. This would interrupt the cascade or (at line 439 inside the catch block) prevent the failure status from being recorded at all.Optional: add fallback for parentHeadSha
+ const safeGetHeadSha = async (path: string): Promise<string | null> => { + try { + return await getHeadSha(path); + } catch { + return null; + } + }; + // Then in setStatus calls: - parentHeadSha: await getHeadSha(parent.worktreePath), + parentHeadSha: await safeGetHeadSha(parent.worktreePath),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts` around lines 371 - 379, The getHeadSha(parent.worktreePath) call can throw and prevent setStatus from executing; wrap those calls in a safe helper (e.g., safeGetHeadSha(worktreePath) or inline try/catch) that returns a fallback (null or empty string) on error and does not rethrow, then use that fallback when populating parentHeadSha in setStatus (sites: the three places where setStatus includes await getHeadSha(parent.worktreePath) in autoRebaseService.ts, including the catch block). Log or debug the underlying error inside the helper/catch but ensure setStatus always runs with the safe value so failure status is recorded reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 2986-2991: refreshHeadShaStartForManagedExecutionLane currently
only runs inside ensureIdentitySession causing headShaStart to remain tied to
the old lane when requestExecutionLaneForIdentitySession moves a live identity
chat to a new/selected/fresh lane; update requestExecutionLaneForIdentitySession
to call refreshHeadShaStartForManagedExecutionLane(managed) immediately after a
lane change (i.e., after resolveManagedExecutionLaneId/assigning the new lane)
so computeHeadShaBestEffort(...) runs for the new lane and
sessionService.setHeadShaStart(...) is updated accordingly; reference
refreshHeadShaStartForManagedExecutionLane,
requestExecutionLaneForIdentitySession, computeHeadShaBestEffort,
resolveManagedExecutionLaneId, and sessionService.setHeadShaStart when making
the change.
- Around line 4067-4074: The persisted lastLaneDirectiveKey
(managed.lastLaneDirectiveKey) is too coarse and can outlive runtime/thread
resets causing skipped lane-binding directives; update
persistDeliveredLaneDirectiveKey (and related persistence via persistChatState)
to include a runtime/thread identity component (e.g., threadId, sdkSessionId, or
a managed.runtimeId) when composing the key so it becomes unique per runtime, or
alternatively clear managed.lastLaneDirectiveKey whenever provider-side history
is discarded (for example inside updateSession or wherever messages/sdkSessionId
is reset) so the new runtime will receive the lane directive; reference
persistDeliveredLaneDirectiveKey, ManagedChatSession.lastLaneDirectiveKey,
persistChatState, and updateSession when making the change.
- Around line 7660-7662: The call to computeHeadShaBestEffort is using
launchContext.laneWorktreePath but the function now resolves worktrees from a
lane ID; change the call to pass launchContext.laneId (or the correct lane
identifier available on launchContext) and keep the existing .catch(() => null)
behavior so headStart is correctly computed for new sessions; update the call
near sessionService.setHeadShaStart(sessionId, headStart) to use
computeHeadShaBestEffort(launchContext.laneId).
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 406-446: The catch block can trigger a rollback even when
rebasePush succeeded but a subsequent getHeadSha throws; update
autoRebaseService to determine push success before any fallible calls and only
attempt rebaseRollback if push did not succeed. Specifically, call
getHeadSha(parent.worktreePath) before the try/after rebasePush success check
(or compute parentHeadSha prior to the try), or set a boolean like pushSucceeded
when rebasePush returns and the pushedLane check passes, then in the catch guard
the rebaseRollback call with that flag (and use
pushedRun/pushedLane/pushedLaneIds to decide); ensure setStatus uses the
captured parentHeadSha when reporting state so you don't call getHeadSha inside
the error path.
In `@apps/desktop/src/main/services/lanes/laneLaunchContext.ts`:
- Around line 11-21: The function ensureDirectoryExists currently returns the
targetPath verbatim which can differ from callers that later resolve realpath;
update ensureDirectoryExists (and the similar helper used at lines 40-47) to
canonicalize and return the realpath by calling fs.realpathSync(targetPath)
after verifying existence and isDirectory, and throw the same error message if
realpathSync fails; this ensures callers always receive the canonical lane root
(avoiding /tmp vs /private/tmp mismatches) while keeping the existing error
behavior.
In `@apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts`:
- Around line 126-140: onParentHeadChanged() is still storing the raw local
postHeadSha for primary parents, whereas listSuggestions()/dismiss()/defer()
normalize primary parents to the origin/<branch> head; update
onParentHeadChanged() to resolve primary parent SHAs the same way: for parents
with laneType === "primary" trim parent.branchRef, fetch the remote tracking
branch and readRefHeadSha(`origin/${branch}`) (falling back to
getHeadSha(parent.worktreePath) if missing) and persist that resolved SHA (or
persist the normalized origin-based key) instead of the local postHeadSha so
primary-lane base identity matches listSuggestions()/dismiss()/defer(). Ensure
you call the same helper sequence used in the shown block
(fetchRemoteTrackingBranch, readRefHeadSha, getHeadSha) and update any storage
keys (e.g., primaryParentHeadByBranch usage) accordingly.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 2106-2120: The catch block on the advanceChildLanesAfterLand call
is currently returning blockCleanup: false which allows deletion/archival even
when child processing fails; change the catch handler to return blockCleanup:
true (and keep updatedLaneIds: [], failedLaneIds: [] as appropriate) so that on
any unexpected error the merged lane is preserved for manual recovery; update
the handler near advanceChildLanesAfterLand and childAdvanceResult to return
blockCleanup: true and keep the existing logger.warn with
getErrorMessage(error).
- Around line 816-835: The recordAttentionStatus() calls after pushing a rebased
child must not reject and trigger the rollback path that calls
restoreAutoRebaseChildLane() (which only resets local state while remote remains
rebased). Wrap each autoRebaseService?.recordAttentionStatus(...) invocation in
a try/catch; on error, log the failure, add the child.id to failedLaneIds (or
otherwise mark it as needing attention), but do not rethrow so control does not
fall into the rollback branch that runs restoreAutoRebaseChildLane(); ensure the
code continues to the next child after handling the logging/marking.
In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx`:
- Around line 461-471: The UI is showing the exit code twice for ended processes
(once in formatProcessStatus(activeRuntime) and again in the separate "exit
{activeRuntime.lastExitCode}" span); remove the redundant span rendering by
deleting or gating the block that renders the mono-font exit text in
ProcessMonitor (the JSX that checks activeRuntime.lastExitCode and
!isActiveStatus(activeRuntime.status)) so the exit code only appears via
formatProcessStatus(activeRuntime).
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 116-125: runningTerminalSessions is currently built from
displaySessions and can exclude the activeSession (which comes from the full
sessions map), causing the terminal overlay to go blank when the active
PTY-backed session isn't visible; update the runningTerminalSessions calculation
to include the activeSession when it is running, has a ptyId, and is not a chat
tool (use activeSession, session.status === "running", Boolean(session.ptyId),
and !isChatToolType(session.toolType)) so the overlay's
SessionSurface/TerminalView always finds a matching session; ensure you only add
the activeSession if it isn't already present to avoid duplicates.
- Around line 359-373: The hidden terminal panes are only visually/inert via CSS
in WorkViewArea rendering TerminalView (prop isActive) but TerminalView
currently only focuses the PTY on activation (see TerminalView.tsx focus logic
around lines 895-914) and lacks a deactivation path; update TerminalView to
watch the isActive prop and, when it flips to false, programmatically
blur/deactivate the terminal instance (call the same terminalRef/xterm blur or
your internal deactivate method), remove keyboard listeners that route input to
the PTY, and mark the root container as inaccessible (set aria-hidden=true and
remove tabbable elements or set tabIndex=-1 / apply inert) so hidden panes
cannot receive focus or keyboard events; keep the WorkViewArea mapping that sets
pointer-events-none/opacity-0 but rely on TerminalView to cleanly teardown
focus/input when isActive becomes false.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 239-267: The mock's getLaneBaseAndBranch currently only recognizes
hardcoded "lane-1" and "lane-2" and falls back to tmpRoot for any newly created
lanes, causing tests that create lanes via create() to resolve the wrong
worktree; change getLaneBaseAndBranch to look up the lane in the shared lanes
array (or update laneRoots when create() adds a lane) and return that lane's
worktreePath, laneType and branchRef accordingly so newly created
generated-lane-N entries are properly resolved during tests.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 7851-7882: The literal slash-command branch currently bypasses
composeLaunchDirectives but still propagates laneDirectiveKey, causing slash
commands to mark the lane directive as delivered; change the logic so that when
isLiteralSlashCommand(trimmed) is true you do not propagate laneDirectiveKey
(set laneDirectiveKey to null for that return), otherwise keep the existing
shouldInjectLaneDirective ? laneDirectiveKey : null flow — update where
promptText and laneDirectiveKey are computed/returned (symbols:
isLiteralSlashCommand, promptText, composeLaunchDirectives, laneDirectiveKey,
shouldInjectLaneDirective) so slash commands no longer suppress the next real
lane-binding prompt.
In `@apps/desktop/src/renderer/components/run/RunPage.tsx`:
- Around line 646-647: The select uses value={effectiveLaneId ?? ""} and
onChange calls selectRunLane, causing a mismatch when effectiveLaneId is null
because there is no <option> with value "". Add a placeholder option with an
empty value (for example a disabled "Select lane" or "None" option) to the
select so the UI reflects the null state, and ensure selectRunLane is invoked
with null when the empty option is chosen; update RunPage.tsx where
effectiveLaneId and selectRunLane are used.
In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx`:
- Around line 863-871: Before calling parkRuntime(runtime) in the cleanup path
where you check if runtime.host.parentElement === el, ensure the terminal
element loses focus so keystrokes do not go to a parked-but-still-alive runtime;
call the host element's blur() (or otherwise remove document.activeElement if it
equals runtime.host) prior to parkRuntime(runtime). Apply this change in the
block that updates runtime.refs and checks runtime.exitCode (the same block that
calls scheduleRuntimeDispose(runtime, EXITED_RUNTIME_KEEPALIVE_MS)) so the
focused terminal is cleared before being moved to the hidden parking root.
---
Nitpick comments:
In `@apps/desktop/src/main/services/lanes/autoRebaseService.test.ts`:
- Around line 891-914: Add an assertion after the existing
expect(laneService.rebaseRollback) check to verify the lane status for "child-1"
was set to the expected pending/reverted state (e.g., rebasePending true or the
specific status object used by the code). Locate the test "rolls back a
successful rebase when auto-push fails and leaves the lane pending" and, after
the calls to service.onHeadChanged and timers, read the lane from laneList or
the service/state accessor used in other tests and assert its status fields
(such as rebasePending or rebaseInProgress) match the expected post-rollback
values for the lane with id "child-1". Ensure you reference the same status
shape used elsewhere in tests (e.g., status: { rebasePending: true,
rebaseInProgress: false } or the canonical field names) so the assertion aligns
with laneService.rebaseRollback being called.
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 371-379: The getHeadSha(parent.worktreePath) call can throw and
prevent setStatus from executing; wrap those calls in a safe helper (e.g.,
safeGetHeadSha(worktreePath) or inline try/catch) that returns a fallback (null
or empty string) on error and does not rethrow, then use that fallback when
populating parentHeadSha in setStatus (sites: the three places where setStatus
includes await getHeadSha(parent.worktreePath) in autoRebaseService.ts,
including the catch block). Log or debug the underlying error inside the
helper/catch but ensure setStatus always runs with the safe value so failure
status is recorded reliably.
In `@apps/desktop/src/main/services/pty/ptyService.test.ts`:
- Around line 24-35: The lane-boundary tests currently mock realpathSync to
behave identically to path.resolve so resolvePathWithinRoot() never experiences
a realpath hop; modify the tests in ptyService.test.ts to add at least one
scenario where the mocked realpathSync for a specific in-lane child path returns
a resolved path outside the lane (e.g., map "/tmp/child" -> "/private/tmp/child"
or similar) and assert that resolvePathWithinRoot()/the launcher rejects
launching that path; update the realpathSync mock (the Object.assign vi.fn(...)
with .native) to handle a special-case input by returning the external path and
ensure the test expects launch rejection to cover production-only realpath hops.
In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx`:
- Around line 79-93: The log handler in ProcessMonitor's useEffect currently
appends chunks indefinitely via setLogText(prev =>
normalizeLog(`${prev}${event.chunk}`)); modify this to truncate the in-memory
log once it exceeds a threshold: define or import LOG_TAIL_MAX_BYTES, append the
new chunk to prev, then if the combined string length (or byte length) is >
LOG_TAIL_MAX_BYTES slice to keep only the tail (e.g., last LOG_TAIL_MAX_BYTES
characters/bytes) before passing it to normalizeLog; keep the existing guards
for laneIdRef and activeProcessIdRef and update only the setLogText call inside
the window.ade.processes.onEvent handler so memory usage is bounded.
- Around line 38-43: The formatEndedAt function currently hardcodes
toLocaleTimeString options; update formatEndedAt to use the user's locale and
make format options configurable (e.g., accept an optional locale or options
param or read navigator.language) so it respects user preferences and i18n;
locate the function formatEndedAt and change the toLocaleTimeString call to use
undefined or the runtime locale (navigator.language or a passed-in locale) and
consider exposing the time-format options via a parameter or context so callers
can override the hour/minute formatting.
- Around line 23-29: isActiveStatus and hasInspectableOutput duplicate logic
already implemented elsewhere (e.g., isActive in CommandCard.tsx and
formatProcessStatus); extract these helpers into a shared module (e.g.,
processUtils.ts) and update usages to import from that module. Specifically,
move the implementations of isActiveStatus, hasInspectableOutput, and any
formatProcessStatus-like logic into a single exported file and replace local
definitions in ProcessMonitor.tsx and CommandCard.tsx to import the shared
functions to keep behavior consistent and remove duplication.
🪄 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: 252177f4-912e-46bc-a4a0-3066fec2375a
📒 Files selected for processing (25)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/lanes/autoRebaseService.test.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/lanes/laneLaunchContext.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsxapps/desktop/src/renderer/components/lanes/LaneTerminalsPanel.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/run/CommandCard.tsxapps/desktop/src/renderer/components/run/ProcessMonitor.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/utils/terminalSessionSignals.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx
- apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx
- apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx
- apps/desktop/src/main/services/pty/ptyService.ts
apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
Outdated
Show resolved
Hide resolved
apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
8856-8869:⚠️ Potential issue | 🟠 Major
lastLaneDirectiveKeycan still outlive same-provider runtime resets.You clear
lastLaneDirectiveKeyon provider changes (and Codex), but same-provider resets (e.g., Claude→Claude model switch teardown, plus other runtime reset paths) can still start a fresh runtime and skip lane directive reinjection on the first turn.🔧 Proposed fix
if (managed.runtime && modelChanged) { teardownRuntime(managed); + managed.lastLaneDirectiveKey = null; refreshReconstructionContext(managed, { includeConversationTail: true }); }if (resetRuntimeForComputerUse && managed.runtime) { teardownRuntime(managed); + managed.lastLaneDirectiveKey = null; refreshReconstructionContext(managed, { includeConversationTail: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 8856 - 8869, The lastLaneDirectiveKey is only cleared on provider switches but not when the runtime is reset for the same provider (e.g., when modelChanged triggers teardownRuntime/refreshReconstructionContext), so clear managed.lastLaneDirectiveKey during runtime resets; update the code path that handles modelChanged (the block that calls teardownRuntime(managed) and refreshReconstructionContext(managed, ...)) to also set managed.lastLaneDirectiveKey = null (or add the clearing inside teardownRuntime or refreshReconstructionContext implementations) so lane directives cannot outlive a fresh runtime.apps/desktop/src/main/services/lanes/autoRebaseService.ts (2)
169-195:⚠️ Potential issue | 🟠 MajorDefault
emit()can still erase freshly written attention states.
processRoot()now derives work fromgetRebaseNeed(), butlistStatuses()still prunes every non-autoRebasedstate whenlane.status.behind <= 0. A lane can therefore hitrebaseConflictorrebaseFailedand disappear on the nextemit()if the behind counter is stale. Emit queue-run updates withincludeAll: true, or stop usingbehindas the pruning gate for freshly persisted attention states.🛠️ Minimal mitigation
- await emit(); + await emit({ includeAll: true });Also applies to: 214-218, 479-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts` around lines 169 - 195, listStatuses currently prunes any non-autoRebased attention state when lane.status.behind <= 0 which lets freshly persisted states like rebaseConflict or rebaseFailed disappear on the next emit; to fix, make emit (the code that drives queue runs / processRoot which relies on getRebaseNeed) call listStatuses with includeAll: true so those attention states are preserved, and update the other call sites that prune by behind (the other listStatuses usages around the noted ranges) to pass { includeAll: true } instead of relying on lane.status.behind.
364-379:⚠️ Potential issue | 🟠 Major
getRebaseNeed()failures are still indistinguishable from "no work".
apps/desktop/src/main/services/conflicts/conflictService.ts:4284-4349currently logs and returnsnullon lookup errors, so this local.catch()won't run for the common failure path. Those failures still fall through!needand clear any stored non-autoRebasedstatus. Please propagate a sentinel/error from the conflict service instead of overloadingnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts` around lines 364 - 379, The code treats a null return from conflictService.getRebaseNeed as "no work" and clears stored state; instead change the API so lookup failures are distinguishable (either by having getRebaseNeed throw on lookup errors or return a sentinel value like { lookupFailed: true }), then update this block in autoRebaseService.ts to detect that sentinel/exception (inspect the returned object for lookupFailed or handle the thrown error in the existing .catch) and only clearStatus/loadStatus when the result truly means "no rebase needed"; reference getRebaseNeed, lookupFailed, loadStatus, clearStatus and the surrounding autoRebaseService.ts logic when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 3024-3027: The current branch only tears down Claude/Codex when
laneWorktreeChanged, leaving "unified" runtimes bound to prior lane context;
update the condition in refreshManagedLaneLaunchContext (or the surrounding
logic that checks laneWorktreeChanged) to also reset unified runtimes by calling
teardownRuntime(managed) and then refreshReconstructionContext(managed, {
includeConversationTail: usesIdentityContinuity(managed) }) (or invoke the
existing helper that fully resets a runtime) when managed.runtime?.kind ===
"unified" so the in-memory model/context is cleared on lane switch.
In `@apps/desktop/src/main/services/lanes/autoRebaseService.test.ts`:
- Around line 123-126: The test harness is defaulting
conflictService.scanRebaseNeeds to return non-empty results which lets
listStatuses() trigger maybeSweepRoots("listStatuses") and start real timers;
change the base harness so conflictService.scanRebaseNeeds is a no-op (returns
empty array) by default, and only stub it to return non-empty RebaseNeed lists
in tests that explicitly exercise refreshActiveRebaseNeeds / cascade behavior;
update the mock definition (the conflictService object with getRebaseNeed and
scanRebaseNeeds) so scanRebaseNeeds resolves to [] unless a test overrides it,
and ensure tests that require sweep results opt into providing laneList.map(...)
results for scanRebaseNeeds.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 837-849: The retarget failure branch currently only pushes
child.id into failedLaneIds when recordAttentionStatusSafely returns falsy, but
the lane should be considered failed whenever retargetError is truthy; update
the block that checks retargetError (the branch using
recordAttentionStatusSafely) to always add child.id to failedLaneIds regardless
of the recorded result—do the push either immediately when retargetError is
detected or right after calling recordAttentionStatusSafely so the lane is
always tracked as failed; keep the existing call to recordAttentionStatusSafely
and the continue behavior intact.
In `@apps/desktop/src/main/services/pty/ptyService.test.ts`:
- Line 3: The test uses path.resolve inside the vi.hoisted() callback which runs
before imports are initialized; move the path.resolve logic out of vi.hoisted()
into the test setup (beforeEach) so imports are available. Specifically, remove
any usage of path.resolve from the vi.hoisted() block and instead set up the
realpath mocks in beforeEach by using mocks.realpathOverrides lookup and calling
path.resolve(p) there; update both mocks.realpathSync and
mocks.realpathSync.native to use that beforeEach implementation and keep the
vi.hoisted() callback limited to things that don't reference imported modules.
---
Duplicate comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 8856-8869: The lastLaneDirectiveKey is only cleared on provider
switches but not when the runtime is reset for the same provider (e.g., when
modelChanged triggers teardownRuntime/refreshReconstructionContext), so clear
managed.lastLaneDirectiveKey during runtime resets; update the code path that
handles modelChanged (the block that calls teardownRuntime(managed) and
refreshReconstructionContext(managed, ...)) to also set
managed.lastLaneDirectiveKey = null (or add the clearing inside teardownRuntime
or refreshReconstructionContext implementations) so lane directives cannot
outlive a fresh runtime.
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 169-195: listStatuses currently prunes any non-autoRebased
attention state when lane.status.behind <= 0 which lets freshly persisted states
like rebaseConflict or rebaseFailed disappear on the next emit; to fix, make
emit (the code that drives queue runs / processRoot which relies on
getRebaseNeed) call listStatuses with includeAll: true so those attention states
are preserved, and update the other call sites that prune by behind (the other
listStatuses usages around the noted ranges) to pass { includeAll: true }
instead of relying on lane.status.behind.
- Around line 364-379: The code treats a null return from
conflictService.getRebaseNeed as "no work" and clears stored state; instead
change the API so lookup failures are distinguishable (either by having
getRebaseNeed throw on lookup errors or return a sentinel value like {
lookupFailed: true }), then update this block in autoRebaseService.ts to detect
that sentinel/exception (inspect the returned object for lookupFailed or handle
the thrown error in the existing .catch) and only clearStatus/loadStatus when
the result truly means "no rebase needed"; reference getRebaseNeed,
lookupFailed, loadStatus, clearStatus and the surrounding autoRebaseService.ts
logic when making the change.
🪄 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: 94ea765f-5341-44ff-8562-f07675de3605
📒 Files selected for processing (18)
apps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/lanes/autoRebaseService.test.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/lanes/laneLaunchContext.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/run/CommandCard.tsxapps/desktop/src/renderer/components/run/ProcessMonitor.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/run/processUtils.tsapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx
- apps/desktop/src/renderer/components/run/CommandCard.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts
- apps/desktop/src/renderer/components/lanes/LanesPage.tsx
- apps/desktop/src/main/services/lanes/laneLaunchContext.ts
- apps/desktop/src/renderer/components/run/ProcessMonitor.tsx
- apps/desktop/src/renderer/components/run/RunPage.tsx
- apps/desktop/src/main/services/chat/agentChatService.test.ts
Bind AI agent sessions to the selected lane worktree and persist that binding, injecting an "ADE launch directive" into initial turns and CI-backed launches. Add lane launch context resolution (new laneLaunchContext module) and thread laneWorktreePath through MCP launches, tool creation, and system prompts; reset runtimes when the execution lane/worktree changes. Track a laneDirectiveKey in session state to avoid redundant injections and use the resolved execution lane for HEAD/sha computations and tool defaults. Revamp auto-rebase logic: introduce RebaseNeed usage and scanning, root sweeping with debounce, queuing roots from needs, auto-push handling (and rollback on failure), rebase attention status persistence/emission, and improved status messages. Add tests covering lane launch directives and many auto-rebase scenarios. Also wire autoRebaseService into app startup and add a brief systemPrompt warning that agents are bound to the worktree.
Multiple fixes and enhancements across auto-rebase, pty, chat, and UI wiring: - main: ensure autoRebaseService is disposed on shutdown. - agentChatService: only persist lane directive keys when a directive is actually delivered; refresh and record headShaStart for the managed execution lane; centralize prepare/execute flow for queued steers; use launchContext path for initial headStart. - ptyService/tests: track boundCwd and laneWorktreePath on Pty entries and use the bound cwd for AI title/summaries (even if lane mapping changes later); preserve non-escape cwd errors instead of rewriting them; wire tests to allow aiIntegrationService overrides. - autoRebaseService: added types, disposal, timer clearing, sweep concurrency guard and promise tracking, resolveAffectedChainLaneId to restrict chain resolution to affected sets, includeAll option for listing/emitting statuses, preserve existing status when lookup fails, and keep sibling chains independent during a sweep. Also added/updated tests for timer disposal, attention status, forced refresh behavior, and other edge cases. - rebaseSuggestionService/prService: cache primary parent head SHAs by branch to avoid redundant fetches; use allLanes map to avoid repeated list lookups and update it after changes. - renderer: pass laneId when opening rebase details and use defaultTrackedCliStartupCommand for CLI startup commands. - utils: adjust default resume command for codex to include --no-alt-screen and improve laneLaunchContext error handling. Overall these changes tighten lifecycle management, reduce redundant work, improve AI summary/title reliability, and make auto-rebase behavior more robust under concurrent/edge conditions.
Multiple fixes and improvements across lane management, auto-rebase, PR flow, terminals and process UIs/tests: - agentChatService: track previous execution lane and refresh head SHA when execution lane changes; clear lastLaneDirectiveKey on provider reset/provider change; compute head SHA best-effort using laneId; avoid injecting lane directive for literal slash commands. - lanes: include branchRef in lane summaries and mock services; ensure realpath usage for launch contexts and worktree paths; add safe getHeadSha with logging; improve auto-rebase flow (use cached parentHeadSha, handle push result robustly, only rollback on failed pushes, record rebase statuses); add tests for lane creation and auto-rebase status recording. - prs: add defensive wrapper for recording auto-rebase attention status and handle failures without crashing; mark cleanup blocking when necessary. - pty/terminal: tighten cwd realpath checks in tests; add test rejecting cwd that hops outside lane worktree; introduce runtime inputEnabled state and set runtime interaction attributes (aria-hidden, inert, tabIndex); ensure wrapper ref and blur behavior when deactivating runtime. - UI changes: CreateLaneDialog copy clarifications; LanesPage: add resolveCreateLaneRequest helper, validate base branch when creating root lanes, add "No lane selected" option and fix create flow; RunPage: handle empty select values correctly. - Processes: extract process helpers to new processUtils (isActiveProcessStatus, hasInspectableProcessOutput, formatProcessStatus) and consume in CommandCard and ProcessMonitor; ProcessMonitor improvements include locale-aware endedAt formatting, log tail trimming, include active session in running sessions, and multiple UI/behavior fixes. - Tests: add LanesPage unit tests and other test updates. Overall these changes improve robustness for rebase flows, avoid unsafe path usage, harden terminal interactions, and tidy process UI logic.
- Tear down unified runtimes on lane worktree switch (agentChatService) so in-memory model/context is cleared, matching claude/codex behavior - Clear lastLaneDirectiveKey on model-change runtime resets to prevent stale lane directives leaking into fresh runtimes - Fix prService retarget: always track lane as failed when retargetError is truthy, not only when recordAttentionStatusSafely returns falsy - Preserve rebaseConflict/rebaseFailed attention states across emits by passing includeAll to listStatuses from the queue runner - Make getRebaseNeed throw on lookup errors instead of returning null so autoRebaseService can distinguish failures from "no rebase needed" - Default scanRebaseNeeds mock to empty array in test harness to prevent unintended timer side-effects; tests that need sweep opt in explicitly - Move path.resolve out of vi.hoisted() in ptyService tests (was using imports before they were initialized) - Add resolveCodexExecutable mock to agentChatService tests (required after rebase onto main which added CLI executable resolution) - Fix environment-dependent cliExecutableResolver test that failed on machines with codex in /opt/homebrew/bin - Improve lane creation, PR creation, rebase tab, terminal sessions, session binding, and sync remote command handling - Update feature documentation for lanes, chat, terminals, and onboarding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
arul28
force-pushed
the
fix-rebase-and-new-lane-flow
branch
from
a6cb774 to
c0473f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/services/ai/cliExecutableResolver.test.ts (1)
44-66:⚠️ Potential issue | 🟡 MinorMove
vi.restoreAllMocks()toafterEachand make path comparisons platform-safe.The mock's hardcoded
/codexcheck fails on Windows paths that use backslashes, allowing system-installed binaries to incorrectly pass through. Additionally, if the assertion fails before line 66, the spy is never restored and leaks into subsequent tests.Suggested fix
afterEach(() => { + vi.restoreAllMocks(); setPlatform(originalPlatform); if (tempRoot) { fs.rmSync(tempRoot, { recursive: true, force: true }); tempRoot = null; } @@ - const realStatSync = fs.statSync; - vi.spyOn(fs, "statSync").mockImplementation(((p: fs.PathLike, opts?: any) => { - const s = String(p); - if (s.endsWith("/codex") && !s.startsWith(tempRoot!)) { + const realStatSync = fs.statSync; + vi.spyOn(fs, "statSync").mockImplementation(((p: fs.PathLike, opts?: any) => { + const rawPath = String(p); + const normalizedPath = rawPath.replace(/\\/g, "/"); + const normalizedRoot = tempRoot!.replace(/\\/g, "/"); + if (normalizedPath.endsWith("/codex") && !normalizedPath.startsWith(normalizedRoot)) { const err: NodeJS.ErrnoException = new Error("ENOENT"); err.code = "ENOENT"; throw err; } - return realStatSync(s, opts); + return realStatSync(p, opts); }) as typeof fs.statSync); @@ - vi.restoreAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/cliExecutableResolver.test.ts` around lines 44 - 66, Move the vi.restoreAllMocks() call out of the test body into an afterEach hook so the fs.statSync spy is always restored even if the assertion fails; update the fs.statSync mock in the test for resolveExecutableFromKnownLocations to perform platform-safe path checks by normalizing the incoming path (use path.normalize or path.sep) and compare against normalized target suffix (e.g., join or normalize "codex") and normalized tempRoot before throwing ENOENT, and keep references to tempRoot and the mocked fs.statSync/realStatSync to locate the change.apps/desktop/src/renderer/components/prs/CreatePrModal.tsx (1)
636-640:⚠️ Potential issue | 🟡 MinorTrim before applying the default target branch fallback.
(" " || default).trim()becomes"", so whitespace-only input is sent as an emptybaseBranch/targetBranchinstead of falling back to the primary lane branch.🛠️ Suggested fix
- const baseBranch = (integrationBaseBranch || branchNameFromRef(primaryLane?.branchRef ?? "main")).trim(); + const defaultBaseBranch = branchNameFromRef(primaryLane?.branchRef ?? "main"); + const baseBranch = integrationBaseBranch.trim() || defaultBaseBranch; ... - const baseBranch = (queueTargetBranch || branchNameFromRef(primaryLane?.branchRef ?? "main")).trim(); + const defaultBaseBranch = branchNameFromRef(primaryLane?.branchRef ?? "main"); + const baseBranch = queueTargetBranch.trim() || defaultBaseBranch;Also applies to: 676-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/CreatePrModal.tsx` around lines 636 - 640, The current code trims after applying the fallback which sends empty strings when integrationBaseBranch is whitespace; update the logic so you trim integrationBaseBranch first and then apply the default from branchNameFromRef(primaryLane?.branchRef ?? "main") if the trimmed value is empty. Locate the baseBranch construction around integrationBaseBranch, branchNameFromRef, and primaryLane in CreatePrModal (used before calling window.ade.prs.simulateIntegration) and change it to compute a trimmed value (e.g., trimmed = integrationBaseBranch?.trim()) then set baseBranch = trimmed || branchNameFromRef(primaryLane?.branchRef ?? "main").apps/desktop/src/main/services/lanes/autoRebaseService.ts (1)
179-182:⚠️ Potential issue | 🟠 MajorTop-level lane rebase needs are still unsurfaced.
scanRebaseNeeds()now produces PR-target needs for lanes with noparentLaneId, but this service still assumes every tracked lane has a parent: stored status is cleared here, the sweep skips those lanes here, and the parentless cascade would skip the lane itself even if it were queued. Those needs never make it to auto-rebase attention state.Also applies to: 235-238, 313-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts` around lines 179 - 182, The service incorrectly treats lanes with no parent (checks like if (!lane.parentLaneId) { clearStatus(lane.id); continue; }) as not needing processing, which drops PR-target rebase needs created by scanRebaseNeeds(); modify the logic in autoRebaseService so parentless lanes are not auto-cleared or skipped—remove the clearStatus + continue branch and allow processing of lanes without parentLaneId, update the sweep/cascade logic that refers to the same pattern (the other occurrences of the if (!lane.parentLaneId) checks) to treat parentless lanes as valid targets (enqueue or compute needs for the lane itself rather than skipping), and ensure clearStatus is only called when a lane is explicitly resolved/removed rather than merely parentless.
♻️ Duplicate comments (2)
apps/desktop/src/main/services/lanes/autoRebaseService.ts (1)
194-196:⚠️ Potential issue | 🟠 MajorExplicit attention states still disappear on the next normal refresh.
recordAttentionStatus()now emits withincludeAll, but any later plainlistStatuses()/emit()call still deletes non-autoRebasedstates wheneverbehind <= 0. A recordedrebaseFailedstate survives the first emit and then gets pruned again.Also applies to: 274-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts` around lines 194 - 196, The pruning logic in listStatuses()/emit() (the branch using clearStatus when !options?.includeAll && lane.status.behind <= 0) incorrectly deletes explicit attention states recorded via recordAttentionStatus() (e.g., rebaseFailed or other non-autoRebased states); update the condition so it only clears statuses that were created by the autoRebase flow (autoRebased true) or lack an explicit marker, i.e., check the recorded status metadata (for example a boolean like status.autoRebased or a source field) and skip clearStatus(lane.id) when the status is an explicit/manual attention state (see recordAttentionStatus, includeAll, clearStatus, and where lane.status.behind is checked); apply the same guard to the other pruning site referenced (the block around the 274-281 range) so explicit attention states persist across normal refreshes.apps/desktop/src/main/services/chat/agentChatService.ts (1)
4167-4174:⚠️ Potential issue | 🟠 MajorClear
lastLaneDirectiveKeyon every full runtime reset path.
persistDeliveredLaneDirectiveKey()keys only by lane/worktree, and in the computer-use reset path a fullteardownRuntime()happens without clearing that key. On the next same-lane turn, directive injection can be skipped for a brand-new runtime/thread.🔧 Suggested fix
if (resetRuntimeForComputerUse && managed.runtime) { teardownRuntime(managed); + managed.lastLaneDirectiveKey = null; refreshReconstructionContext(managed, { includeConversationTail: true }); }Also applies to: 9123-9126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 4167 - 4174, persistDeliveredLaneDirectiveKey only updates ManagedChatSession.lastLaneDirectiveKey when setting a new key, but teardown/reset paths (e.g., teardownRuntime / full runtime reset) do not clear that field, allowing directive injection to be skipped incorrectly for a fresh runtime; update the full-reset code path to explicitly clear managed.lastLaneDirectiveKey = undefined (or null) and call persistChatState(managed) so the persisted state no longer prevents directive injection, and ensure this same clear/persist logic is applied wherever runtime/threads are fully torn down or reinitialized (reference persistDeliveredLaneDirectiveKey, ManagedChatSession.lastLaneDirectiveKey, persistChatState, and teardownRuntime/reset functions).
🧹 Nitpick comments (4)
apps/desktop/src/renderer/components/run/ProcessMonitor.tsx (1)
88-117: Consider removing redundant ref assignment.Line 97 (
activeProcessIdRef.current = processId) is redundant since line 43 already synchronizes the ref withactiveProcessIdon every render. The staleness check at line 104 will work correctly using the value from line 43.🧹 Suggested simplification
let cancelled = false; const processId = activeRuntime.processId; - activeProcessIdRef.current = processId; setPauseAutoscroll(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx` around lines 88 - 117, In the ProcessMonitor component's useEffect that fetches the log tail, remove the redundant assignment activeProcessIdRef.current = processId (the line that sets the ref inside the effect) and rely on the existing synchronization of activeProcessIdRef with activeProcessId performed earlier on render; keep the staleness checks that compare activeProcessIdRef.current to processId in the .then and .catch handlers and leave all other logic (cancelled flag, setLogLoading/setLogError/setLogText, window.ade.processes.getLogTail) unchanged.apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx (1)
116-136: Extract the live-terminal predicate.The
"running" + ptyId + !chatcheck now decides both which PTYs stay mounted and whetherSessionSurfaceis suppressed. Keeping that logic duplicated here is brittle: if one branch changes first, tabs will regress back to either a blank pane or a double-mounted PTY.♻️ Suggested extraction
+function isRunningPtySession(session: TerminalSessionSummary) { + return session.status === "running" && Boolean(session.ptyId) && !isChatToolType(session.toolType); +} + export function WorkViewArea({ lanes, sessions, @@ const runningTerminalSessions = useMemo( () => { - const running = displaySessions.filter( - (session) => - session.status === "running" - && Boolean(session.ptyId) - && !isChatToolType(session.toolType), - ); + const running = displaySessions.filter(isRunningPtySession); if ( activeSession - && activeSession.status === "running" - && Boolean(activeSession.ptyId) - && !isChatToolType(activeSession.toolType) + && isRunningPtySession(activeSession) && !running.some((session) => session.id === activeSession.id) ) { running.push(activeSession); @@ - activeSession.status === "running" && activeSession.ptyId && !isChatToolType(activeSession.toolType) ? null : ( + isRunningPtySession(activeSession) ? null : ( <div className="absolute inset-0"> <SessionSurface session={activeSession} isActive onOpenChatSession={onOpenChatSession} /> </div>Also applies to: 392-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx` around lines 116 - 136, The predicate that determines a live terminal ("status === 'running' && Boolean(ptyId) && !isChatToolType(toolType)") is duplicated and brittle; extract it into a single helper (e.g., isLiveTerminalSession or isRunningPtySession) and replace the inline checks in the runningTerminalSessions useMemo (and the other location mentioned where SessionSurface suppression is decided) to call that helper with either a Session object or activeSession; update imports/exports or file scope so both places reference the same function and ensure the logic is unchanged.apps/desktop/src/renderer/components/prs/LanePrPanel.test.ts (1)
30-50: Add the missing parent-lane fallback case.
resolveDefaultBaseBranchalso falls back tolane.baseRefwhenparentLaneIdis set but the parent lane is unavailable. That partial-data path is easy to hit in the UI, and this new suite currently leaves it unguarded.Suggested test
describe("resolveDefaultBaseBranch", () => { it("uses lane.baseRef for unparented lanes", () => { expect( resolveDefaultBaseBranch({ lane: makeLane({ baseRef: "release-9", parentLaneId: null }), parentLane: null, primaryBranchRef: "fix-rebase-and-new-lane-flow", }), ).toBe("release-9"); }); + it("falls back to lane.baseRef when the parent lane is unavailable", () => { + expect( + resolveDefaultBaseBranch({ + lane: makeLane({ baseRef: "release-9", parentLaneId: "lane-parent" }), + parentLane: null, + primaryBranchRef: "main", + }), + ).toBe("release-9"); + }); + it("uses the parent branch for child lanes", () => { expect( resolveDefaultBaseBranch({ lane: makeLane({ parentLaneId: "lane-parent" }), parentLane: makeLane({ id: "lane-parent", branchRef: "feature/stack-root" }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/LanePrPanel.test.ts` around lines 30 - 50, Add a test to cover the fallback where resolveDefaultBaseBranch should use lane.baseRef even when lane.parentLaneId is set but parentLane is null: in the "resolveDefaultBaseBranch" describe block, add an it case that calls resolveDefaultBaseBranch with lane: makeLane({ parentLaneId: "some-id", baseRef: "release-9" }), parentLane: null, and a primaryBranchRef, and assert the result equals "release-9"; this mirrors the unparented case but ensures the partial-data path (parentLaneId present but parentLane missing) returns lane.baseRef.apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx (1)
201-225: Avoid two identical CTAs in the failed state.When
status.state === "rebaseFailed", both buttons callonViewRebaseDetails(status.laneId), so the row shows two actions with the same result. Keep one CTA or give the secondary button a different job.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx` around lines 201 - 225, The two buttons rendered when status.state === "rebaseFailed" both call onViewRebaseDetails(status.laneId), producing duplicate CTAs; update the secondary button so it either is removed or performs a different action (for example call onRebaseNowLocal(status.laneId) to retry a local rebase) instead of duplicating onViewRebaseDetails. Locate the conditional branch that renders the OPEN REBASE TAB button and the following "View rebase details" button and change the latter's onClick handler (or remove the button) so the two CTAs are distinct; relevant symbols: status.state, onViewRebaseDetails, onRebaseNowLocal, and the button rendering in LaneRebaseBanner.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/conflicts/conflictService.ts`:
- Around line 4421-4422: The catch in getRebaseNeed currently logs and rethrows
(see logger.warn in conflictService.getRebaseNeed), which breaks callers that
expect a nullable result (prRebaseResolver and the IPC rebaseGetNeed handler);
instead of throwing, swallow transient errors by returning null after logging so
callers keep the nullable contract (remove or replace the throw err with a
return null).
- Around line 4334-4345: When creating the PR-target need in needs.push, do not
hardcode groupContext, dismissedAt, or deferredUntil to null; instead preserve
the lane-scoped dismiss/defer state so snoozed PR-targets remain snoozed. Look
up the existing need for this PR in the lane's rebase/need collection (the same
place used by dismissRebase()/deferRebase()) and copy its groupContext,
dismissedAt, and deferredUntil into the object you push; fallback to null only
if no prior need exists.
- Around line 304-311: rebaseLane currently always uses comparisonRef as the
actual rebase target, which breaks for local-only base branches; update
rebaseLane to detect when comparisonRef (the remote origin/<baseRef>) is
unavailable or when fallbackRef is provided and prefer fallbackRef as the rebase
target for the git rebase operation. Specifically, change the logic in
rebaseLane to use fallbackRef if it exists (and if attempting to fetch/check
origin/<baseRef> fails or origin ref is missing) instead of blindly passing
comparisonRef to the rebase invocation; keep displayBaseBranch untouched for UI,
and ensure the same comparisonRef/fallbackRef pair produced earlier
(comparisonRef, fallbackRef, displayBaseBranch) is consumed so local-only repos
use fallbackRef during the actual rebase.
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 246-250: The entry guards (e.g., in maybeSweepRoots) only prevent
starting work but do not stop continued processing once awaits complete; after
each await or asynchronous boundary inside maybeSweepRoots and similar flows
(notably where processRoot(), sweepPromise, and calls to rebaseStart/rebasePush
occur) re-check disposed and return early if true. Update maybeSweepRoots to
check "if (disposed) return" immediately after awaiting sweepPromise and after
any await or long-running call, and apply the same pattern to the other affected
functions where processRoot(), rebaseStart, or rebasePush are awaited so the
service stops mid-cascade when disposed() has been called.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 1476-1480: Reject baseBranchOverride early for parented lanes:
after computing baseBranchOverride (via normalizeBranchName) and obtaining
target with getLaneRow(args.laneId), if persistBaseBranch is true and
target.parent_lane_id (or target.parentLaneId) is set, throw an error and do not
proceed; this prevents persisting an override for parented lanes and avoids
clearing parent_lane_id later. Apply the same guard to the other occurrences
noted (around the blocks at the other ranges) so any code path that would
persistBaseBranch first validates target.parent_lane_id before continuing.
In `@apps/desktop/src/main/services/sessions/sessionService.ts`:
- Around line 87-98: mapRow currently always normalizes resumeCommand using the
stored toolType (via normalizeResumeCommand(..., toolType)), which causes
read/write divergence when callers supply a preferred tool on write; update
write paths that persist resumeCommand (the handlers around the other
occurrences referenced at the diff spans and any create/update functions) to
normalize using the incoming/preferred tool type if provided, otherwise resolve
the session's current toolType and use that as the fallback before calling
normalizeResumeCommand; ensure the same logic is applied wherever resumeCommand
is written so mapRow and the write-side normalization agree (reference
normalizeResumeCommand, mapRow, and any create/update functions that set
resumeCommand).
In `@apps/desktop/src/main/utils/terminalSessionSignals.ts`:
- Around line 28-35: preferredTool may be an orchestrated variant (e.g.,
"claude-orchestrated" / "codex-orchestrated") but prefersTool only recognizes
base names; update normalizeResumeCommand to canonicalize preferredTool to its
base tool before calling prefersTool (map "claude-orchestrated" -> "claude",
"codex-orchestrated" -> "codex"), and apply the same canonicalization wherever
the same filtering occurs (the other similar block that uses prefersTool for
resume command filtering). Use the existing preferredTool variable and
prefersTool function names to locate and change the logic.
In `@apps/desktop/src/renderer/components/prs/CreatePrModal.tsx`:
- Around line 615-628: The effect that sets normalBaseBranch reinitializes it on
any lanes or primaryLane ref change, which can overwrite user edits; update the
effect for React.useEffect in CreatePrModal so it only reinitializes the base
branch when the selected lane actually changes (e.g., depend on [open,
selectedNormalLane]) or add a dirty/default guard before calling
setNormalBaseBranch (check if normalBaseBranch is empty or equals the previous
default from resolveDefaultBaseBranchForLane) and only then call
resolveDefaultBaseBranchForLane with selectedNormalLane, lanes, and
primaryLane?.branchRef to avoid clobbering user input.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 1148-1151: The banner/CTA in OverviewTab is re-lookuping the PR's
lane without excluding archived lanes, causing archived lanes to trigger the
target-diff banner; update OverviewTab to use the same computed laneForPr from
PrDetailPane (or perform the find with the same filter: lanes.find(l => l.id ===
pr.laneId && !l.archivedAt) ?? null) so the banner and "View Rebase Details" CTA
only appear for non-archived lanes; make the same change for the other lookup(s)
referenced around the 1227-1267 range to ensure consistent behavior.
In `@apps/desktop/src/renderer/components/prs/shared/laneBranchTargets.ts`:
- Around line 3-7: branchNameFromRef currently strips "refs/heads/" and
"origin/" but misses refs like "refs/remotes/origin/main", causing incorrect
comparisons; update branchNameFromRef to also detect and strip the
"refs/remotes/" prefix (and then still strip any leading "<remote>/" such as
"origin/") so the returned name is the bare branch (e.g., "main"); locate the
branchNameFromRef function and add logic to handle "refs/remotes/" before
falling back to existing checks so all remote/ref shapes normalize to the same
branch name.
In `@apps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsx`:
- Around line 109-113: The detail pane is still querying commit history from
selectedLane.parentLaneId while the UI shows selectedNeed.baseBranch for
pr_target needs; update the RebaseTab rendering logic (where commit history is
fetched and displayed) to detect pr_target (use isPrTargetNeed(selectedNeed) or
selectedNeed.mode === 'pr_target') and, for those needs, switch the query to use
the branch-based ref derived from selectedNeed.baseBranch (use
branchNameFromRef(selectedNeed.baseBranch)) instead of
selectedLane.parentLaneId; if selectedNeed.baseBranch is empty/null for a
pr_target, hide or skip rendering the drift/commit panel. Apply the same change
to the other occurrence referenced around the 615-627 block.
- Around line 31-33: The run matching currently keys runs only by laneId which
causes collisions between items distinguished by rebaseNeedKey (e.g., lane_base
vs pr_target); update the matching logic used when associating runs and when
persisting selected runs to use the full rebaseNeedKey (or at minimum laneId +
normalized baseBranch) instead of laneId alone—specifically, change places that
compute or compare run identity (the run association code referenced around the
run matching logic and the selection/start-run code at the block handling runs
around lines 128-133) to call rebaseNeedKey(need) and use that string as the run
key and when storing the active run selection so each need preserves its own run
state and actions remain correctly enabled/disabled.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 370-395: The current code mounts every TerminalView for each
runningTerminalSessions which keeps PTY and window/document listeners alive;
update either the rendering here or TerminalView to avoid that: (A) change this
component to only render the active session's TerminalView (render TerminalView
for session where activeSession?.id === session.id) and render non-active
sessions as a lightweight placeholder/SessionSurface, or (B) add a “parked” prop
to TerminalView and implement parked behavior inside TerminalView (in files
around TerminalView lines 668–700 and 878–908) so when parked it immediately
unsubscribes/detaches PTY and window/document listeners and only re-attaches
when parked=false and active; reference runningTerminalSessions, TerminalView,
activeSession, and isChatToolType when making the change.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ai/cliExecutableResolver.test.ts`:
- Around line 44-66: Move the vi.restoreAllMocks() call out of the test body
into an afterEach hook so the fs.statSync spy is always restored even if the
assertion fails; update the fs.statSync mock in the test for
resolveExecutableFromKnownLocations to perform platform-safe path checks by
normalizing the incoming path (use path.normalize or path.sep) and compare
against normalized target suffix (e.g., join or normalize "codex") and
normalized tempRoot before throwing ENOENT, and keep references to tempRoot and
the mocked fs.statSync/realStatSync to locate the change.
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 179-182: The service incorrectly treats lanes with no parent
(checks like if (!lane.parentLaneId) { clearStatus(lane.id); continue; }) as not
needing processing, which drops PR-target rebase needs created by
scanRebaseNeeds(); modify the logic in autoRebaseService so parentless lanes are
not auto-cleared or skipped—remove the clearStatus + continue branch and allow
processing of lanes without parentLaneId, update the sweep/cascade logic that
refers to the same pattern (the other occurrences of the if (!lane.parentLaneId)
checks) to treat parentless lanes as valid targets (enqueue or compute needs for
the lane itself rather than skipping), and ensure clearStatus is only called
when a lane is explicitly resolved/removed rather than merely parentless.
In `@apps/desktop/src/renderer/components/prs/CreatePrModal.tsx`:
- Around line 636-640: The current code trims after applying the fallback which
sends empty strings when integrationBaseBranch is whitespace; update the logic
so you trim integrationBaseBranch first and then apply the default from
branchNameFromRef(primaryLane?.branchRef ?? "main") if the trimmed value is
empty. Locate the baseBranch construction around integrationBaseBranch,
branchNameFromRef, and primaryLane in CreatePrModal (used before calling
window.ade.prs.simulateIntegration) and change it to compute a trimmed value
(e.g., trimmed = integrationBaseBranch?.trim()) then set baseBranch = trimmed ||
branchNameFromRef(primaryLane?.branchRef ?? "main").
---
Duplicate comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 4167-4174: persistDeliveredLaneDirectiveKey only updates
ManagedChatSession.lastLaneDirectiveKey when setting a new key, but
teardown/reset paths (e.g., teardownRuntime / full runtime reset) do not clear
that field, allowing directive injection to be skipped incorrectly for a fresh
runtime; update the full-reset code path to explicitly clear
managed.lastLaneDirectiveKey = undefined (or null) and call
persistChatState(managed) so the persisted state no longer prevents directive
injection, and ensure this same clear/persist logic is applied wherever
runtime/threads are fully torn down or reinitialized (reference
persistDeliveredLaneDirectiveKey, ManagedChatSession.lastLaneDirectiveKey,
persistChatState, and teardownRuntime/reset functions).
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 194-196: The pruning logic in listStatuses()/emit() (the branch
using clearStatus when !options?.includeAll && lane.status.behind <= 0)
incorrectly deletes explicit attention states recorded via
recordAttentionStatus() (e.g., rebaseFailed or other non-autoRebased states);
update the condition so it only clears statuses that were created by the
autoRebase flow (autoRebased true) or lack an explicit marker, i.e., check the
recorded status metadata (for example a boolean like status.autoRebased or a
source field) and skip clearStatus(lane.id) when the status is an
explicit/manual attention state (see recordAttentionStatus, includeAll,
clearStatus, and where lane.status.behind is checked); apply the same guard to
the other pruning site referenced (the block around the 274-281 range) so
explicit attention states persist across normal refreshes.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx`:
- Around line 201-225: The two buttons rendered when status.state ===
"rebaseFailed" both call onViewRebaseDetails(status.laneId), producing duplicate
CTAs; update the secondary button so it either is removed or performs a
different action (for example call onRebaseNowLocal(status.laneId) to retry a
local rebase) instead of duplicating onViewRebaseDetails. Locate the conditional
branch that renders the OPEN REBASE TAB button and the following "View rebase
details" button and change the latter's onClick handler (or remove the button)
so the two CTAs are distinct; relevant symbols: status.state,
onViewRebaseDetails, onRebaseNowLocal, and the button rendering in
LaneRebaseBanner.tsx.
In `@apps/desktop/src/renderer/components/prs/LanePrPanel.test.ts`:
- Around line 30-50: Add a test to cover the fallback where
resolveDefaultBaseBranch should use lane.baseRef even when lane.parentLaneId is
set but parentLane is null: in the "resolveDefaultBaseBranch" describe block,
add an it case that calls resolveDefaultBaseBranch with lane: makeLane({
parentLaneId: "some-id", baseRef: "release-9" }), parentLane: null, and a
primaryBranchRef, and assert the result equals "release-9"; this mirrors the
unparented case but ensures the partial-data path (parentLaneId present but
parentLane missing) returns lane.baseRef.
In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx`:
- Around line 88-117: In the ProcessMonitor component's useEffect that fetches
the log tail, remove the redundant assignment activeProcessIdRef.current =
processId (the line that sets the ref inside the effect) and rely on the
existing synchronization of activeProcessIdRef with activeProcessId performed
earlier on render; keep the staleness checks that compare
activeProcessIdRef.current to processId in the .then and .catch handlers and
leave all other logic (cancelled flag, setLogLoading/setLogError/setLogText,
window.ade.processes.getLogTail) unchanged.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 116-136: The predicate that determines a live terminal ("status
=== 'running' && Boolean(ptyId) && !isChatToolType(toolType)") is duplicated and
brittle; extract it into a single helper (e.g., isLiveTerminalSession or
isRunningPtySession) and replace the inline checks in the
runningTerminalSessions useMemo (and the other location mentioned where
SessionSurface suppression is decided) to call that helper with either a Session
object or activeSession; update imports/exports or file scope so both places
reference the same function and ensure the logic is unchanged.
🪄 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: 3b11d0fe-3771-45e2-9769-ada59a204eb7
⛔ Files ignored due to path filters (6)
.ade/ade.yamlis excluded by!.ade/**docs/architecture/UI_FRAMEWORK.mdis excluded by!docs/**docs/features/CHAT.mdis excluded by!docs/**docs/features/LANES.mdis excluded by!docs/**docs/features/ONBOARDING_AND_SETTINGS.mdis excluded by!docs/**docs/features/TERMINALS_AND_SESSIONS.mdis excluded by!docs/**
📒 Files selected for processing (55)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/cliExecutableResolver.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/conflicts/conflictService.test.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/lanes/autoRebaseService.test.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/lanes/laneLaunchContext.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.tsapps/desktop/src/main/services/prs/prService.landAutoRebase.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsxapps/desktop/src/renderer/components/lanes/LaneTerminalsPanel.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/prs/CreatePrModal.test.tsxapps/desktop/src/renderer/components/prs/CreatePrModal.tsxapps/desktop/src/renderer/components/prs/LanePrPanel.test.tsapps/desktop/src/renderer/components/prs/LanePrPanel.tsxapps/desktop/src/renderer/components/prs/PrRebaseBanner.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/laneBranchTargets.tsapps/desktop/src/renderer/components/prs/tabs/RebaseTab.test.tsapps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsxapps/desktop/src/renderer/components/run/CommandCard.tsxapps/desktop/src/renderer/components/run/ProcessMonitor.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/run/processUtils.tsapps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/shared/types/lanes.ts
✅ Files skipped from review due to trivial changes (9)
- apps/desktop/src/main/services/ai/tools/systemPrompt.ts
- apps/desktop/src/renderer/components/lanes/LaneTerminalsPanel.tsx
- apps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsx
- apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsx
- apps/desktop/src/renderer/components/lanes/LanesPage.test.ts
- apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx
- apps/desktop/src/renderer/components/run/processUtils.ts
- apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx
- apps/desktop/src/main/services/prs/prService.landAutoRebase.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/desktop/src/renderer/components/prs/PrRebaseBanner.tsx
- apps/desktop/src/main/main.ts
- apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts
- apps/desktop/src/renderer/components/run/CommandCard.tsx
- apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx
- apps/desktop/src/renderer/components/run/RunPage.tsx
- apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts
- apps/desktop/src/renderer/components/lanes/LanesPage.tsx
- apps/desktop/src/renderer/components/terminals/TerminalView.tsx
- apps/desktop/src/main/services/lanes/autoRebaseService.test.ts
- apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx
- apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx
- apps/desktop/src/renderer/components/terminals/cliLaunch.ts
- apps/desktop/src/main/services/prs/prService.ts
Multiple changes to improve auto-rebase behavior, UI and related services: - autoRebaseService: add disposed guards, treat parentless lanes as eligible for status display, preserve manual statuses, handle fallback rebase targets if comparison ref is missing, queue root resolution for parentless lanes, improve lookup failure handling and messaging, and ensure proper rollback/emit behavior. - RebaseTab/UI: better active-run association by computed run keys, only show drift panel when relevant, derive drift source lane correctly, fetch commit/files from the right lane, refine rendering/layout, and update CTA text for failed rebases in LaneRebaseBanner. - conflictService: preserve existing rebase-need metadata (groupContext, dismissedAt, deferredUntil), return null on lookup errors, and use fallback comparison logic when target ref is absent. - laneService: prevent persisting base branch overrides for parented lanes. - sessionService & terminalSessionSignals: more robust resumeCommand normalization (respect current toolType), canonicalize preferred tool names and prefer tool matching logic. - prs/CreatePrModal and helpers: avoid overwriting explicit user base branch edits by tracking previous default, trim integration base branch before fallback, add branchNameFromRef support for refs/remotes, and ensure PR detail uses non-archived lane for cleanup banner. - Tests and misc: normalize paths and restore mocks in cliExecutableResolver tests, adjust git push in tests to push HEAD:main, and add various test updates to reflect behavior changes. These changes tighten lifecycle handling, surface clearer statuses/messages, and make UI behavior more predictable when lanes or refs are missing.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/sessions/sessionService.ts (1)
225-229:⚠️ Potential issue | 🟠 MajorFallback resume commands won't replace stale cross-tool values.
Because Line 97 now normalizes incompatible stored commands to
null, this branch can run even while the rawresume_commandcolumn is still non-null. Theand resume_command is nullguard then suppresses the fallback write, so a tool-type-only switch can leave the session without a usable resume command.🛠️ Suggested fix
if (args.toolType !== undefined && !updated.resumeCommand) { const fallback = defaultResumeCommandForTool(updated.toolType); if (fallback) { - db.run("update terminal_sessions set resume_command = ? where id = ? and resume_command is null", [fallback, sessionId]); + db.run("update terminal_sessions set resume_command = ? where id = ?", [fallback, sessionId]); const withResume = this.get(sessionId); return withResume ?? updated; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sessions/sessionService.ts` around lines 225 - 229, The fallback write is being skipped by the SQL guard "and resume_command is null" because the in-memory normalization set updated.resumeCommand to null while the raw DB column still contains an incompatible value; update the write so the fallback is applied when updated.resumeCommand is falsy regardless of the raw column. In sessionService.ts, replace the db.run call in the branch that calls defaultResumeCommandForTool(updated.toolType) (the code using db.run("update terminal_sessions set resume_command = ? where id = ? and resume_command is null", [fallback, sessionId])) with an update that targets the session id without requiring resume_command IS NULL (i.e., remove the "and resume_command is null" condition) so the fallback resume command is stored when this code path runs; keep the surrounding logic that only executes when args.toolType !== undefined and !updated.resumeCommand and then return the updated session via this.get(sessionId).
🧹 Nitpick comments (3)
apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx (1)
361-379: Good fix: Only the active terminal is now mounted in tabs view.This overlay pattern addresses the previous concern about hidden PTYs staying hot. In tabs view, only the active running terminal session gets a
TerminalViewmounted; other sessions are rendered as tabs without their terminals consuming resources. When the active session is not a running PTY (chat or ended),SessionSurfacehandles it appropriately.One minor observation: the wrapper div on line 363 with
absolute inset-0aroundTerminalView(which also hasabsolute inset-0) is slightly redundant since the parent already providesrelativepositioning.,
Optional: Remove redundant wrapper div
<div className="relative min-h-0 flex-1" style={{ background: THEME_CARD }}> {activeRunningTerminalSession ? ( - <div className="absolute inset-0"> - <TerminalView - key={activeRunningTerminalSession.id} - ptyId={activeRunningTerminalSession.ptyId} - sessionId={activeRunningTerminalSession.id} - isActive - className="absolute inset-0 h-full w-full" - /> - </div> + <TerminalView + key={activeRunningTerminalSession.id} + ptyId={activeRunningTerminalSession.ptyId} + sessionId={activeRunningTerminalSession.id} + isActive + className="absolute inset-0 h-full w-full" + /> ) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx` around lines 361 - 379, The outer wrapper div that wraps TerminalView (the div with className "absolute inset-0" created when activeRunningTerminalSession is truthy) is redundant because TerminalView already uses className="absolute inset-0 h-full w-full"; remove that extra wrapper so TerminalView is mounted directly under the parent relative container (leave the surrounding conditional logic using activeRunningTerminalSession and the SessionSurface fallback unchanged).apps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsx (2)
153-153: Minor: Consider renamingactiveRunNeedKeyReffor clarity.This ref stores a value from
rebaseRunKey(), notrebaseNeedKey(). A name likeactiveRunKeyRefwould more accurately reflect its purpose and avoid confusion with the need-key concept.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsx` at line 153, Rename the ref activeRunNeedKeyRef to activeRunKeyRef throughout the RebaseTab component to reflect that it stores the value returned by rebaseRunKey(), not rebaseNeedKey(); update the declaration (const activeRunKeyRef = React.useRef<string | null>(null)) and all references/usages of activeRunNeedKeyRef within methods, effects, and JSX to the new name so identifiers remain consistent.
534-540:driftTouchedFilesis computed but never rendered.This memo computes a set of all touched files from drift commits, but the value isn't used in the JSX. It also appears in the
paneConfigsdependency array (line 1489), causing unnecessary recalculations. Consider removing it or adding the intended UI that uses this data.🧹 Proposed removal
- // Compute file overlap between drift commits and the lane's own files - const driftTouchedFiles = React.useMemo(() => { - const files = new Set<string>(); - for (const fileList of Object.values(commitFilesMap)) { - for (const f of fileList) files.add(f); - } - return files; - }, [commitFilesMap]);And remove
driftTouchedFilesfrom thepaneConfigsdependency array at line 1489.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsx` around lines 534 - 540, The React.useMemo that computes driftTouchedFiles from commitFilesMap is never used in the JSX and is only inflating the paneConfigs dependency array; remove the entire driftTouchedFiles memo (the constant driftTouchedFiles and its React.useMemo) and also remove driftTouchedFiles from the paneConfigs dependency array so paneConfigs no longer re-runs for this unused value; if the intent was to display these files instead, alternatively wire the computed Set into the component that should render it (e.g., pass driftTouchedFiles into the relevant pane or render list) and keep paneConfigs dependencies aligned to only actually-used values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 6827-6829: The temporary Codex runtime cleanup in
listCodexModelsFromAppServer currently calls runtime?.process.kill(), which
leaks child processes when the Codex process was spawned with detached:
process.platform !== "win32"; replace that direct kill with a call to
terminateChildProcessTree(...) so the entire process group is terminated (e.g.,
call terminateChildProcessTree(runtime?.process) or
terminateChildProcessTree(runtime.process) after null checks); update any error
handling/logging around the cleanup accordingly and keep references to the
existing runtime variable and terminateChildProcessTree function.
In `@apps/desktop/src/main/services/lanes/autoRebaseService.test.ts`:
- Around line 611-613: The mock for conflictService.scanRebaseNeeds is creating
a fabricated rebase need for the root lane because makeRebaseNeed defaults
behindBy > 0; update the mock to filter out the root lane from laneList before
mapping (e.g., exclude the lane with id 'root' or where lane.isRoot/identifier
matches), then map to makeRebaseNeed and filter by behindBy > 0 so the mocked
output matches the real scanner; adjust the expression around laneList.map(...)
to first filter out the root lane.
- Around line 880-908: Add an assertion that the child lane is marked
autoRebased after the push: after awaiting timers and promises, assert that the
lane with id "child-1" has status.autoRebased === true (e.g.
expect(laneList.find(l => l.id === "child-1")?.status.autoRebased).toBe(true));
keep the existing expects for laneService.rebaseStart and laneService.rebasePush
but also assert the post-push status update so regressions in marking the lane
are caught.
In `@apps/desktop/src/main/utils/terminalSessionSignals.ts`:
- Line 5: The RESUME_PLAIN_REGEX is too greedy (/[^\r\n]*/), allowing a first
match like "codex resume ..." to swallow a later valid "claude --resume ..."
command; update RESUME_PLAIN_REGEX to stop at the next assistant keyword or be
non-greedy so it doesn't span multiple commands (e.g., change to a non-greedy
quantifier and/or add a negative lookahead for "(?:claude|codex)" to end the
match before the next assistant token). Also apply the same fix to the matching
logic around the candidate filtering at the code that inspects matches (the
block referenced by lines 71-75) so rejected Codex candidates leave valid Claude
matches available.
In `@apps/desktop/src/renderer/components/prs/CreatePrModal.tsx`:
- Line 687: The code sets baseBranch using (queueTargetBranch ||
branchNameFromRef(...)).trim(), which lets whitespace-only queueTargetBranch
bypass the fallback; change to trim the input first and use that trimmed value
in the fallback check: compute a trimmedQueueTargetBranch (e.g., const
trimmedQueueTargetBranch = (queueTargetBranch ?? "").trim()) and then set
baseBranch = (trimmedQueueTargetBranch ||
branchNameFromRef(primaryLane?.branchRef ?? "main")).trim(); ensure you
reference the existing functions/variables baseBranch, queueTargetBranch,
branchNameFromRef, and primaryLane so the fallback behaves the same as the
earlier integration-mode pattern before calling createQueue.
In `@apps/desktop/src/renderer/components/run/ProcessMonitor.tsx`:
- Around line 97-105: When starting a new fetch in ProcessMonitor (before
calling window.ade.processes.getLogTail), clear the previous logText so stale
output can't appear under the new header: call setLogText("") (or a short
placeholder) immediately after setLogLoading(true)/setLogError(null) and before
the getLogTail promise; keep the existing checks using
activeProcessIdRef.current and processId, and continue to
setLogText(normalizeLog(log)) when the promise resolves.
---
Outside diff comments:
In `@apps/desktop/src/main/services/sessions/sessionService.ts`:
- Around line 225-229: The fallback write is being skipped by the SQL guard "and
resume_command is null" because the in-memory normalization set
updated.resumeCommand to null while the raw DB column still contains an
incompatible value; update the write so the fallback is applied when
updated.resumeCommand is falsy regardless of the raw column. In
sessionService.ts, replace the db.run call in the branch that calls
defaultResumeCommandForTool(updated.toolType) (the code using db.run("update
terminal_sessions set resume_command = ? where id = ? and resume_command is
null", [fallback, sessionId])) with an update that targets the session id
without requiring resume_command IS NULL (i.e., remove the "and resume_command
is null" condition) so the fallback resume command is stored when this code path
runs; keep the surrounding logic that only executes when args.toolType !==
undefined and !updated.resumeCommand and then return the updated session via
this.get(sessionId).
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsx`:
- Line 153: Rename the ref activeRunNeedKeyRef to activeRunKeyRef throughout the
RebaseTab component to reflect that it stores the value returned by
rebaseRunKey(), not rebaseNeedKey(); update the declaration (const
activeRunKeyRef = React.useRef<string | null>(null)) and all references/usages
of activeRunNeedKeyRef within methods, effects, and JSX to the new name so
identifiers remain consistent.
- Around line 534-540: The React.useMemo that computes driftTouchedFiles from
commitFilesMap is never used in the JSX and is only inflating the paneConfigs
dependency array; remove the entire driftTouchedFiles memo (the constant
driftTouchedFiles and its React.useMemo) and also remove driftTouchedFiles from
the paneConfigs dependency array so paneConfigs no longer re-runs for this
unused value; if the intent was to display these files instead, alternatively
wire the computed Set into the component that should render it (e.g., pass
driftTouchedFiles into the relevant pane or render list) and keep paneConfigs
dependencies aligned to only actually-used values.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 361-379: The outer wrapper div that wraps TerminalView (the div
with className "absolute inset-0" created when activeRunningTerminalSession is
truthy) is redundant because TerminalView already uses className="absolute
inset-0 h-full w-full"; remove that extra wrapper so TerminalView is mounted
directly under the parent relative container (leave the surrounding conditional
logic using activeRunningTerminalSession and the SessionSurface fallback
unchanged).
🪄 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: a7caf6dd-b803-4b74-b73c-2f94778e75c7
📒 Files selected for processing (17)
apps/desktop/src/main/services/ai/cliExecutableResolver.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/conflicts/conflictService.test.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/lanes/autoRebaseService.test.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsxapps/desktop/src/renderer/components/prs/CreatePrModal.tsxapps/desktop/src/renderer/components/prs/LanePrPanel.test.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/laneBranchTargets.tsapps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsxapps/desktop/src/renderer/components/run/ProcessMonitor.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/components/prs/LanePrPanel.test.ts
- apps/desktop/src/renderer/components/prs/shared/laneBranchTargets.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/main/services/conflicts/conflictService.test.ts
- apps/desktop/src/main/services/ai/cliExecutableResolver.test.ts
- apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx
- apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx
- apps/desktop/src/main/services/lanes/laneService.ts
- apps/desktop/src/main/services/lanes/autoRebaseService.ts
- apps/desktop/src/main/services/conflicts/conflictService.ts
Multiple fixes across main and renderer code: - agentChatService: use terminateChildProcessTree(runtime.process, null) to ensure child processes are killed safely. - sessionService: always set a fallback resume_command (removed "resume_command is null" condition) so resume commands are persisted reliably. - terminalSessionSignals: tighten RESUME_PLAIN_REGEX to avoid greedy matches that span into subsequent commands. - CreatePrModal: handle null/empty queueTargetBranch safely by trimming a default before deriving baseBranch. - RebaseTab: rename activeRunNeedKeyRef -> activeRunKeyRef, update all usages, and remove unused driftTouchedFiles memo to simplify state handling. - ProcessMonitor: clear logText when (re)loading logs to avoid showing stale content. - WorkViewArea: remove unnecessary wrapper div around TerminalView to simplify layout. - autoRebaseService.test: make scanRebaseNeeds mock filter only child lanes, switch to vi.waitFor for deterministic assertions, and add an assertion that db.setJson is updated with auto-rebase status. These changes fix process termination, session persistence, regex overmatching, UI edge cases, state tracking in the rebase UI, and make the rebase tests more robust and deterministic.
This was referenced Mar 31, 2026
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.
Bind AI agent sessions to the selected lane worktree and persist that binding, injecting an "ADE launch directive" into initial turns and CI-backed launches. Add lane launch context resolution (new laneLaunchContext module) and thread laneWorktreePath through MCP launches, tool creation, and system prompts; reset runtimes when the execution lane/worktree changes. Track a laneDirectiveKey in session state to avoid redundant injections and use the resolved execution lane for HEAD/sha computations and tool defaults.
Revamp auto-rebase logic: introduce RebaseNeed usage and scanning, root sweeping with debounce, queuing roots from needs, auto-push handling (and rollback on failure), rebase attention status persistence/emission, and improved status messages. Add tests covering lane launch directives and many auto-rebase scenarios. Also wire autoRebaseService into app startup and add a brief systemPrompt warning that agents are bound to the worktree.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements