fix(fork): show Fork button on AI messages and correct role mapping#857
fix(fork): show Fork button on AI messages and correct role mapping#857pedramamini merged 4 commits intoRunMaestro:rcfrom
Conversation
… mapping Fork button never appeared on AI responses because the UI condition checked `source === 'ai'`, but AI response text is actually stored with `source: 'stdout'` (see useBatchedSessionUpdates.ts). Extend the TerminalOutput condition to accept 'user' | 'ai' | 'stdout' (still gated by `isAIMode`, which excludes shell terminal stdout). Also fix the forked-context role mapping in useForkConversation: in AI mode, `stdout` IS the AI response, not tool output, so map stdout → Assistant. Only `source === 'tool'` remains labeled Tool Output. Without this, AI responses in the forked prompt were mislabeled as "Tool Output: ...", misleading the downstream agent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughFork conversation now supports Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Store as SessionStore
participant Spawner as AgentSpawner
participant Notif as NotificationService
participant Logger as Sentry
UI->>Store: request fork(logId)
Store->>Store: locate active session & tab, compute endIndex (extend through assistant/tool/stdout chunks)
Store->>Store: createTabAtPosition(new AI tab), update sessions & set active tab, set inputMode='ai'
Store->>Spawner: spawn agent with sessionId `${session.id}-ai-${newTab.id}` and payload (forked user-context)
Spawner-->>Store: spawn success (pid) -> stream chunks appended to new tab logs
Store->>Notif: show "Conversation Forked" toast referencing session.id and newTab.id
Note over Spawner,Store: on spawn error -> Logger.captureException, set tab state idle, append system error log
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 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 |
Greptile SummaryThis PR fixes two related bugs in the Fork conversation feature: the Fork button was never visible on AI responses because
Confidence Score: 4/5Mergeable after addressing the incomplete-context bug in useForkConversation — the UI fix is correct but the slice logic needs to extend through all consecutive stdout/ai chunks for the clicked response. One P1 bug: forking from a collapsed AI response that spans multiple raw stdout entries produces a truncated fork context because slicing stops at the first chunk's index. This is directly triggered by the newly-visible Fork button on AI messages — the code path didn't exist before this PR. The role mapping fix and UI condition change are both correct. src/renderer/hooks/agent/useForkConversation.ts — the slice boundary logic at lines 25-29 Important Files Changed
|
Addresses PR RunMaestro#857 review: P1 (greptile): When an AI response spans multiple raw stdout entries (chunks arriving >500ms apart, per useBatchedSessionUpdates), the UI collapses them into one visual block keyed by the FIRST entry's id. The fork click passed that first-chunk id, so slice(0, rawLogIndex+1) truncated the forked context to just the opening chunk of the response, silently dropping the rest. Fix: after resolving rawLogIndex, walk forward through consecutive entries that are NOT user/tool/thinking (mirroring collapsedLogs' grouping rule in TerminalOutput) and extend endIndex to the last chunk of the same block. Slice uses endIndex+1. User messages and tool/ thinking entries are not collapsed in the UI, so endIndex stays put for those — preserving existing behavior when forking from a user turn or a tool/thinking log. P2 (greptile): Update stale JSDoc on onForkConversation prop in TerminalOutput.tsx to reflect that the button also renders for source='stdout' (AI responses in AI mode), not just 'user' and 'ai'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… agent Fork Conversation now adds an AI tab to the current session (via createTabAtPosition) and reuses session.id for the agent spawn, matching user expectations. Previously it created a brand-new session, which surfaced in the Left Bar as a separate agent. Adds a 10-case test suite locking in the contract: same-session invariant, tab inserted after source, busy/active/order state, multi- chunk AI response capture, early-return guards, and spawn-failure rollback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/agent/useForkConversation.ts (1)
227-246:⚠️ Potential issue | 🟠 MajorDon’t mark the whole session idle if another tab is still busy.
Now that fork creates a tab in the existing session, this failure path can clear
session.state,busySource, andthinkingStartTimeeven when another AI tab in the same session is still running.🐛 Proposed fix
setSessions((prev) => prev.map((s) => { if (s.id !== session.id) return s; + const aiTabs = s.aiTabs.map((tab) => + tab.id === newTabId + ? { + ...tab, + state: 'idle' as const, + thinkingStartTime: undefined, + awaitingSessionId: false, + logs: [...tab.logs, errorLog], + } + : tab + ); + const hasOtherBusyAiTab = aiTabs.some( + (tab) => tab.id !== newTabId && tab.state === 'busy' + ); return { ...s, - state: 'idle', - busySource: undefined, - thinkingStartTime: undefined, - aiTabs: s.aiTabs.map((tab) => - tab.id === newTabId - ? { - ...tab, - state: 'idle' as const, - thinkingStartTime: undefined, - awaitingSessionId: false, - logs: [...tab.logs, errorLog], - } - : tab - ), + state: hasOtherBusyAiTab ? 'busy' : 'idle', + busySource: hasOtherBusyAiTab ? 'ai' : undefined, + thinkingStartTime: hasOtherBusyAiTab ? s.thinkingStartTime : undefined, + aiTabs, }; }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useForkConversation.ts` around lines 227 - 246, The current failure branch in the setSessions callback unconditionally sets the entire session (session.state, busySource, thinkingStartTime) to 'idle' even though other AI tabs in the same session may still be busy; update the map callback in setSessions so that after updating the specific tab (tab.id === newTabId) you compute whether any aiTabs (including the newly-updated one) still have a non-idle busy state and only set session.state to 'idle' / clear busySource and thinkingStartTime when no other aiTabs remain busy; keep the existing per-tab update (setting that tab to idle, clearing its thinkingStartTime/awaitingSessionId, and appending errorLog) but derive session-level fields from s.aiTabs.some(tab => tab.state !== 'idle') (or equivalent) so you don't clear session-level busy flags while another tab is running.
🧹 Nitpick comments (1)
src/__tests__/renderer/hooks/useForkConversation.test.ts (1)
144-172: Add a unified-order assertion for non-tail source tabs.This test verifies
aiTabsplacement but would still pass ifunifiedTabOrderappends the forked tab aftertab-tail, causing the visible unified tab strip to be ordered incorrectly.🧪 Proposed test assertion
const newTab = aiTabs[2]; expect(newTab.id).not.toBe(sourceTab.id); expect(newTab.name).toBe('Forked: Source'); + expect(getSessions()[0].unifiedTabOrder).toEqual([ + { type: 'ai', id: firstTab.id }, + { type: 'ai', id: sourceTab.id }, + { type: 'ai', id: newTab.id }, + { type: 'ai', id: tailTab.id }, + ]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useForkConversation.test.ts` around lines 144 - 172, Test is missing a check that the fork also updates unifiedTabOrder so the new entry appears immediately after the source tab (not appended after tail); after calling fork('log-s') use getSessions() to inspect unifiedTabOrder on the session and assert that there is a new { type: 'ai', id: <newTab.id> } entry directly after the entry whose id equals sourceTab.id and before the entry with id equal to tailTab.id; update the test surrounding the fork/getSessions calls (ref: fork, getSessions, unifiedTabOrder, sourceTab.id, tailTab.id) to include this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/agent/useForkConversation.ts`:
- Around line 123-148: The current logic appends the new AI tab ref to
unifiedTabOrder (updatedOrder = [...updatedOrder, { type: 'ai', id: newTabId }])
which places the fork at the end rather than immediately after the source;
change the insertion to locate the source entry in unifiedTabOrder (find index
where entry.type === 'ai' && entry.id === sourceTabId), compute insert position
= sourceIndex >= 0 ? sourceIndex + 1 : updatedOrder.length, and splice/construct
updatedOrder to insert { type: 'ai', id: newTabId } at that position (do this
inside the !hasNewTab branch where aiTabs and updatedOrder are built) so the
fork appears directly after the source tab.
---
Outside diff comments:
In `@src/renderer/hooks/agent/useForkConversation.ts`:
- Around line 227-246: The current failure branch in the setSessions callback
unconditionally sets the entire session (session.state, busySource,
thinkingStartTime) to 'idle' even though other AI tabs in the same session may
still be busy; update the map callback in setSessions so that after updating the
specific tab (tab.id === newTabId) you compute whether any aiTabs (including the
newly-updated one) still have a non-idle busy state and only set session.state
to 'idle' / clear busySource and thinkingStartTime when no other aiTabs remain
busy; keep the existing per-tab update (setting that tab to idle, clearing its
thinkingStartTime/awaitingSessionId, and appending errorLog) but derive
session-level fields from s.aiTabs.some(tab => tab.state !== 'idle') (or
equivalent) so you don't clear session-level busy flags while another tab is
running.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useForkConversation.test.ts`:
- Around line 144-172: Test is missing a check that the fork also updates
unifiedTabOrder so the new entry appears immediately after the source tab (not
appended after tail); after calling fork('log-s') use getSessions() to inspect
unifiedTabOrder on the session and assert that there is a new { type: 'ai', id:
<newTab.id> } entry directly after the entry whose id equals sourceTab.id and
before the entry with id equal to tailTab.id; update the test surrounding the
fork/getSessions calls (ref: fork, getSessions, unifiedTabOrder, sourceTab.id,
tailTab.id) to include this assertion.
🪄 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: d6e36f18-03c8-4b5e-bc8e-6ba0b1e28dda
📒 Files selected for processing (3)
src/__tests__/renderer/hooks/useForkConversation.test.tssrc/renderer/App.tsxsrc/renderer/hooks/agent/useForkConversation.ts
Previously appended to the end, which misplaced the fork when the source tab was not last in the unified strip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
source === 'ai', but AI response text is actually stored withsource: 'stdout'(seeuseBatchedSessionUpdates.ts:223), so the button never rendered on AI messages.'stdout') were being labeled"Tool Output:"instead of"Assistant:", which confused the downstream agent. Onlysource === 'tool'is now labeled Tool Output.Repro (before fix)
Hover an AI response in AI mode → action buttons show: Markdown toggle, Copy, Save, Publish to Gist. The Fork button is missing. User report confirmed via screenshot.
Changes
src/renderer/components/TerminalOutput.tsx— broaden the Fork button condition to'user' | 'ai' | 'stdout', still gated byisAIModeso terminal-mode stdout is excluded.src/renderer/hooks/agent/useForkConversation.ts— role mapping:user→ User,tool→ Tool Output, everything else in the filtered set (ai,stdout) → Assistant. Added a comment pointing touseBatchedSessionUpdatesas the source-of-truth for why stdout is the AI's output in AI mode.Test plan
Assistant:(notTool Output:).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation