Repository navigation
fix(native-chat): keep a tab launch draft out of split sibling composers - #16883
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds tab-wide launch draft ownership tracking for native chat panes. Leaf routing determines ownership only for a single-pane tab. Terminal panes pass ownership through Merge Risk: 🔵 Low · up to The change prevents split siblings from adopting a tab launch draft, but shared-draft cleanup can still let a sibling submission or identical composer text remove pending context from the intended pane. The impact is limited to local split-pane draft state and is mergeable with explicit owner awareness and follow-up. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed context, root cause, implementation details, linked issue, risks, and test results. However, it omits the required Visual Proof section or an explicit N/A statement, and it does not follow several template headings and checklist sections. Resolution Add the required Visual Proof section with before-and-after attachments, or write N/A with a reason if visual proof is not applicable. Also include the required template sections and checklist items, including AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. Full details: Linked Issues checkExplanation The changes directly address the renderer launch-draft path in Full details: Out of Scope Changes checkExplanation The changes are within scope. Ownership predicate extraction, prop consolidation, view plumbing, composer adoption logic, and regression tests all support preventing stale launch drafts in split sibling composers.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a82cf2d-a16a-4cba-9d5f-b1e1a47c1874
📒 Files selected for processing (9)
src/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/NativeChatView.tsxsrc/renderer/src/components/native-chat/native-chat-composer-types.tssrc/renderer/src/components/native-chat/native-chat-leaf-routing.test.tssrc/renderer/src/components/native-chat/native-chat-leaf-routing.tssrc/renderer/src/components/native-chat/native-chat-view-types.tssrc/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.tssrc/renderer/src/components/terminal-pane/TerminalPane.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
The launch draft is stored per terminal tab, but the desktop composer selected it with no pane gate, so any pane of that tab — including one created by splitting, hours or days later — adopted the tab's unsent launch text into its empty composer. Gate the desktop draft on the same pane ownership the launch agent and the mobile launch-draft publisher already use, extracted as nativeChatLeafOwnsTabWideEvidence. Fixes #16695
…achine Gating pane ownership inside useNativeChatLaunchDraftSignal nulled the seed for every pane of a split tab, and the adoption effect early-returns on a null seed — so it also killed the resolved/cleanup branch. A pane that had already mirrored the seed into its composer kept that text forever once the tab was split and the prompt was submitted TUI-side, and a later Enter re-sent it. Keep the signal pane-agnostic and gate the only branch that writes text into a composer. Cleanup now runs for the pane still holding the untouched copy, while a pane that neither owns the seed nor holds its copy cannot drop it out from under the owner. Fold the composer's launch-draft props into one `launchSeed` object: they are one unit produced by one hook, and the view already passed them as a group. Refs #16695
ed7cad3 to
890ffa2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Greptile SummaryThe PR adds pane ownership to native-chat launch-draft adoption so split siblings cannot claim tab-wide seeded text, while preserving transcript-driven cleanup after ownership changes.
Confidence Score: 4/5The PR should not merge until cleanup can identify the actual adopting pane without deleting equal user-entered text from a split sibling. Launch-draft ownership is correctly gated during adoption, but resolved cleanup conflates text equality with provenance and can erase an independent pane-scoped draft. Files Needing Attention: src/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.ts
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.ts | Adds ownership-gated adoption and split-safe cleanup, but text equality plus a tab-wide adopted bit can erase an independently entered sibling draft. |
| src/renderer/src/components/terminal-pane/TerminalPane.tsx | Computes launch-draft ownership from the bound owner leaf and current live pane topology, then propagates it into native chat. |
| src/renderer/src/components/native-chat/native-chat-leaf-routing.ts | Extracts the sole-leaf ownership predicate and reuses it without changing launch-agent routing semantics. |
| src/renderer/src/components/native-chat/NativeChatView.tsx | Propagates pane ownership and combines it with the pane-agnostic launch-draft signal. |
| src/renderer/src/components/native-chat/NativeChatComposer.tsx | Replaces separate launch-draft props with the optional launchSeed object for adoption and send behavior. |
| src/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.test.tsx | Adds broad split-pane regression coverage but does not cover independently entered sibling text equal to an adopted seed. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
S[Tab-wide launch seed] --> O{Leaf owns tab-wide evidence?}
O -->|Sole owning leaf| A[Adopt into pane-scoped composer]
O -->|Split tab| N[Do not adopt]
A --> B[Tab-wide adopted flag]
B --> R{Transcript resolves seed}
N --> R
R --> E{Adopted flag and draft text equals seed}
E -->|Yes| C[Clear current pane draft and seed]
E -->|No, but current owner| D[Clear seed only]
E -->|Neither| K[Leave seed and draft unchanged]
Reviews (1): Last reviewed commit: "fix(native-chat): gate the launch-draft ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const holdsUntouchedCopy = launchDraft.adopted && draft === launchDraft.text | ||
| if (holdsUntouchedCopy) { | ||
| setDraft('') | ||
| setCaret(0) | ||
| } | ||
| useAppStore.getState().clearNativeChatLaunchDraft(terminalTabId) | ||
| // A pane that neither owns the seed nor holds its copy has no standing to |
There was a problem hiding this comment.
When the original pane adopts a seed, a split sibling later contains independently entered text equal to that seed, and the transcript resolves, holdsUntouchedCopy treats the sibling's text as the adopted copy because adopted is tab-wide. The cleanup then erases the sibling's user-entered draft and retires the shared seed.
Knowledge Base Used: Agent workspaces and sessions
1 participant
ELI5
When Orca starts an agent with a prompt it does not send (an issue link, a brief), it mirrors that unsent text into the GUI chat composer so the context is visible. That mirror was stored per tab, not per pane, so any pane of the tab could claim it. Split a pane in that tab — even a day later — open chat in the new pane, and yesterday's unsent prompt appeared in its empty composer, ready to be submitted by an Enter or
orca terminal send --enter. Now only the pane the draft was seeded for can adopt it; a split sibling always starts empty.Root cause
src/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.tsThe only additional filter was
launchDraft?.agent === args.agent. There is no pane/leaf gate, so every pane of the tab that resolves to the same agent sees the same draft, anduseNativeChatLaunchDraftAdoptionwrites it into whichever composer mounted with an empty draft.The asymmetry was local and explicit: in the same render tree
TerminalPane.tsx:2976gates the tab's launch agent throughnativeChatLaunchAgentForLeaf, which refuses tab-wide evidence as soon asleafIds.length !== 1, and the mobile publisherbuildMobileLaunchDraftsByPaneKey(src/renderer/src/runtime/sync-runtime-graph.ts:1900) runs the same store draft through that identical predicate before emittinglaunchDraftByPaneKey. Only the desktop composer's draft signal skipped it.Fix: extract that ownership rule as
nativeChatLeafOwnsTabWideEvidenceinnative-chat-leaf-routing.ts(now also the body ofnativeChatLaunchAgentForLeaf, so desktop and mobile share one predicate), compute it for the chat pane inTerminalPane, and apply it to the composer write.Where the gate goes, and why not in the signal
The first revision of this PR gated the signal:
useNativeChatLaunchDraftSignalreturnednullfor a non-owning pane. That was wrong.useNativeChatLaunchDraftAdoptionearly-returns on!launchDraft, so nulling the signal did not merely block the pickup branch — it disabled the whole per-draft state machine for every pane of a split tab, including the resolution branch that callssetDraft('')andclearNativeChatLaunchDraft.Splitting flips
ownsTabWideLaunchDrafttofalsefor both panes (the predicate requiresleafIds.length === 1), so the pane that had already mirrored the seed lost its cleanup path too: launch with a seeded prompt, open chat so the owning pane adopts it, split the tab, submit from the TUI — and the seeded text stayed in the original pane's composer permanently (useNativeChatDraft(paneKey)is pane-scoped and survives TUI/GUI toggles), where a later Enter re-submits an already-submitted prompt. That is #16695's own harm class, reintroduced in the original pane.The gate now sits on the only branch that writes text into a composer:
Cleanup is keyed on holding the copy rather than on ownership, because ownership is gone for every pane once the tab is split. The
clearNativeChatLaunchDraftguard is the mirror-image protection: a pane that neither owns the seed nor holds its copy must not drop it, or a sibling's transcript could destroy a seed the owner has not adopted yet.The composer's three launch-draft props are folded into one
launchSeedobject (NativeChatLaunchSeed) — they are one unit produced by one hook, andNativeChatViewalready passed them as a group.Proof
Test file:
src/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.test.tsxCommand:
npx vitest run --config config/vitest.config.ts src/renderer/src/components/native-chat/use-native-chat-launch-draft-adoption.test.tsxBEFORE the fix (tests written first, run against
ad4e8bd's source): 12 of 23 fail. Most fail because the ownership flag moved from the signal hook to the adoption hook, but the load-bearing one is the regression itself — an adopted seed on a split tab, resolved by the transcript, is never cleaned up:AFTER the fix:
Wider runs, all green:
oxlintandoxfmt --checkare clean over both directories.pnpm run tc:webreports one pre-existing error unrelated to this change (useSessionRestoredBannerDismiss.test.tsx(26,3), aquerySelectorcast in an untouched file).Regression guards now live in the composed signal-plus-adoption suite, so they assert composer behavior rather than signal shape. The two earlier tests that asserted a sibling's
launchDraftisnullwere rewritten, since the signal is deliberately pane-agnostic now:never fills a non-owning pane's composer with the tab's launch draft— [Bug]: splitting a pane can restore a stale unsent composer draft into the new pane #16695's original protection.never lets a non-owning pane destroy a seed nobody has adopted yet.still mirrors the launch draft into the owning pane composer.clears the adopted copy after a split once the transcript resolves the seed— the regression above.does not mirror the seed into a pane that does not own the tab-wide evidence,leaves an unadopted seed alone when a non-owning pane resolves it) and the pure-predicate test innative-chat-leaf-routing.test.ts.Risk
Low. Renderer-only; the gate reuses the predicate that already governs the launch agent on the very same pane.
nativeChatLaunchAgentForLeafwas refactored to call the extracted predicate with identical semantics (!launchAgentshort-circuit first, then the sameleafIds.length === 1 && leafIds[0] === leafId && ownerLeafId === leafId), and its existing tests pass unchanged.NativeChatComposer'slaunchDraft/launchDraftResolvedprops became one optionallaunchSeedprop; the component has a single production call site and no test passes those props. No persisted state, no store shape change, no IPC/RPC or remote-wire field, no mobile publisher behavior change, no platform-specific code.User-facing regressions
nativeChatLeafOwnsTabWideEvidence({ ownerLeafId: getTabWideAgentHintLeafId(), leafId, leafIds: getNativeChatLeafIds() }).tabWideAgentHintLeafIdbinds once, butgetNativeChatLeafIds()is recomputed on every call — it unions the live pane list withexpectedLayoutLeafIds, which is derived from the tab's layout root and shrinks when a pane closes. So after split-then-close-sibling,leafIds.lengthreturns to1and matches the bound owner leaf, and the original pane adopts the seed again — unless the transcript has already resolved it, in which case the resolved branch runs first and drops the seed instead of re-offering it. An earlier revision of this description claimed adoption was lost permanently once a tab had been split; that was wrong.clearNativeChatLaunchDraft, so a split sibling cannot delete a seed the owner has yet to adopt.nativeChatLaunchDraftByTabIduntil ownership returns, the transcript resolves it in the pane holding the copy, or the tab closes. It is inert in that state: the pickup branch refuses it and the cleanup branch is its only other consumer. No growth beyond one entry per tab.nativeChatLaunchPromptByTabId— the submitted launch prompt rendered as a transcript bubble — is still tab-keyed and can render in a split sibling's chat view. It is display-only, cannot be submitted by an Enter, and untangling it fromshouldPruneLaunchPromptis a separate change.Why
Refs, notFixesThis lands the pane gate the reported scenario needs, but it must not auto-close #16695. The reporter says Ctrl-U cleared the text. Per the #16290 discriminator, Ctrl-U is a PTY-side line kill: text that Ctrl-U erases was living in the agent TUI's input buffer, not in Orca's GUI composer (a React textarea, which does not respond to Ctrl-U that way). So the occurrence actually reported was PTY-side, and this renderer-layer change cannot be shown to fix it. Leaving the issue open keeps that unexplained PTY-side path visible.
Refs #16695