Repository navigation
fix(native-chat): text typed during /clear follows the chat to its new conversation - #26054
brennanb2025 wants to merge 12 commits into
Conversation
…w conversation A structured chat's unsent draft is kept per conversation, and /clear moves the chat's tab to a new conversation. Whatever the box held while the clear ran stayed with the old conversation, which nothing shows any more, so it disappeared. When a chat's tab moves to another conversation, its draft now moves along (after anything the new conversation's draft already holds). The text of a send still waiting for its reply, such as the "/clear" itself, stays behind for that send to clear, since the tab can move before the reply arrives.
…-draft-follows # Conflicts: # src/renderer/src/components/native-chat/use-native-chat-composer-attachments.ts
|
Native-chat input entered while Review repairs removed retrospective transfer and document merging, made existing pending operations the authority for upload completion, restored refused clear only into a truly empty composer, and preserved picked editor nodes when completed file references are appended. The seventh review found no in-scope issues. A separate inherited SSH file-drop/picker path can lose an upload if its composer closes and is deferred; pending clipboard pastes and paired-file completion are covered here. Final-head verification: 315 tests across 23 explicit files on final Hidden macOS Electron validation at reviewed |
📝 WalkthroughWalkthroughThe changes preserve structured prompt documents when text or file references are appended. They track pending attachments across draft scopes and move eligible drafts when a tab changes agent sessions. The composer also handles host Priority: ⬇️ Low Merge Risk: 🔵 Low · up to This change moves unsent chat drafts to the new conversation after /clear. The only remaining issue is a cosmetic test-title placeholder, so the risk of merging is low. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
038fec43-b620-41e4-adfb-4f12173a04f9
📒 Files selected for processing (27)
src/renderer/src/app-shell/native-chat-draft-startup.test.tssrc/renderer/src/app-shell/native-chat-draft-startup.tssrc/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/native-chat-composer-draft-addition.test.tssrc/renderer/src/components/native-chat/native-chat-composer-draft-addition.tssrc/renderer/src/components/native-chat/native-chat-composer-draft-journal.tssrc/renderer/src/components/native-chat/native-chat-composer-draft-load.tssrc/renderer/src/components/native-chat/native-chat-composer-draft-store.tssrc/renderer/src/components/native-chat/native-chat-draft-cache.tssrc/renderer/src/components/native-chat/native-chat-pending-attachment-cache.test.tssrc/renderer/src/components/native-chat/native-chat-pending-attachment-cache.tssrc/renderer/src/components/native-chat/native-chat-prompt-document-text.test.tssrc/renderer/src/components/native-chat/native-chat-prompt-document-text.tssrc/renderer/src/components/native-chat/native-chat-prompt-document.tssrc/renderer/src/components/native-chat/native-chat-session-attachment-drop.tssrc/renderer/src/components/native-chat/structured-agent-session-draft-attachment-move.test.tsxsrc/renderer/src/components/native-chat/structured-agent-session-draft-file-upload.test.tsxsrc/renderer/src/components/native-chat/structured-agent-session-draft-move.test.tssrc/renderer/src/components/native-chat/structured-agent-session-draft-move.tssrc/renderer/src/components/native-chat/structured-agent-session-draft-upload-lifetime.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-composer-attachments.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-composer-attachments.tssrc/renderer/src/components/native-chat/use-native-chat-composer-paste.tssrc/renderer/src/components/native-chat/use-native-chat-external-attachments.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-paste-lifetime.tssrc/renderer/src/components/native-chat/use-native-chat-structured-composer-send.clear-move.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-structured-composer-send.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/components/native-chat/native-chat-composer-draft-journal.ts
- src/renderer/src/components/native-chat/native-chat-composer-draft-load.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| it.each(['local', 'ssh'])( | ||
| 'control: moved $s upload survives teardown and settles in destination', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the test-name placeholder in it.each.
The cases are plain strings. Vitest therefore does not interpolate $s in the title. Use %s so each case name shows the host.
Proposed fix
- 'control: moved $s upload survives teardown and settles in destination',
+ 'control: moved %s upload survives teardown and settles in destination',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.each(['local', 'ssh'])( | |
| 'control: moved $s upload survives teardown and settles in destination', | |
| it.each(['local', 'ssh'])( | |
| 'control: moved %s upload survives teardown and settles in destination', |
|
Closing this in favour of a simpler design, after comparing it with the common pattern for Why: this PR moves the draft (and queued text) from the cleared conversation to the new one that What happens to the parts of this PR:
|
ELI5
In a native chat, text and images entered while
/clearwas running disappeared when the tab switched to its fresh conversation. The unsent draft belonged to the old conversation, while the tab now showed the new one's empty composer. The draft now follows that tab into the fresh conversation.What Changed
/clearleaves the composer when submitted, so the command itself cannot follow the draft. A failed command is restored when the composer has no text, settled images or pending uploads. New text or images entered while it runs remain untouched. Other commands and normal message sends retain their existing submission behavior.Why
Keeping a single conversation identity across
/clearwould avoid this bug class, but requires changing host behavior beyond composer drafts. The host stores a committed source-to-replacement record, creates a fresh provider identity with no previous journal checkpoint, moves the host tab, and closes the source after publishing the replacement. Reusing the source identity would close the fresh conversation through that existing path and would require a new journal boundary plus coordinated client handling. Paired hosts and mobile clients already use this replacement contract; this change preserves it and needs no new wire field or capability.Drafts remain keyed by conversation so a rejected message can be returned to the right composer even without an open tab. Moving the draft through its existing store matches the common pattern for a view promoted to another conversation identity. Consuming
/clearat submission removes the previous in-memory tracking map for sends waiting on replies.Differences from the common pattern:
/clearcreates a distinct conversation to preserve the existing host records, separate journals, and close behavior described above./clear, keeping this fix focused on the command that changes conversation identity.Linked Issue
N/A — maintainer follow-up from a native chat audit.
Visual Proof
Hidden Electron validation on macOS at reviewed
728a8fda; the final sync to99ae403dpreserved all 27 reviewed files byte-for-byte. Automated checks run on the final head. The rendered check used an isolated folder workspace and stand-in OpenCode provider. Before clear, the real composer holds an unsent question, a real image, a picked Review skill and a literal suffix. An outside-composer client calls the real host clear operation; afterward the same tab shows a fresh conversation with the entire draft unchanged, and the saved source draft is removed. Both screenshots show the patched behavior before and after that action.After removing the carried input through the UI, submitting
/clearin the actual composer creates another empty conversation under the same tab. Neither replacement contains the command. These checks do not exercise typing during provider shutdown, a real provider, a phone/second UI window, live SSH, Windows or Linux; automated tests cover post-submit typing and upload completion.Testing
Twenty-three explicit files: 315 tests passed on macOS at final
99ae403d, including real host-tab snapshot projection, synchronous repeated clears, intact text/images/skill documents, destination conflict preservation, refused saves and ordinary flush repair, history preservation, pre-load upload settlement and journal recovery, pending-image refusal, skipped-move upload survival, moved paired file references, removed uploads, failed saves, caret insertion, input-method composition, moved and ordinary PDF/PNG completion with actual editor remount, picked-node preservation during delayed loading and crash recovery, and an empty clear.Temporarily removing each of 14 required behaviors made the corresponding test fail; all source files were restored. The final queued renderer, main-process and CLI typechecks pass after synchronizing with main. The earlier main-branch localization failure is repaired in the final sync. All 22 final static gates pass, including localization. Final-head CI run 37740067285 is complete: 18 checks passed and 17 were skipped, with no failures or pending checks. All ten unit shards actually executed and passed; relay integration, static checks, Linux and Windows packaging, the overall verification gate and the advisory report passed. The path-gated skipped checks are not counted as passes.
No new saved tab field or message between clients and execution hosts is introduced. No replacement relation is persisted or reconstructed at startup. Folder workspaces use the same tab binding. An older host that does not report the existing tab switching conversations may create a new tab instead; its earlier draft stays in history. SSH image paths keep their connection identifiers. Actual SSH, Windows, Linux and phone rendering were not verified by live QA; phone-origin clears use the shared host snapshot path covered in the renderer test.
I manually tested these changes locally
Automated tests added/updated
Review
Six reviews found and resolved gaps in draft movement, pending upload ownership, refusal restoration and picked-skill preservation during file completion. Loaded drafts move intact only into empty replacements; pending image pastes and paired file uploads finish in their current owning draft; the generic append path preserves picked editor nodes even when its composer is closed. History and destination-conflict limits are explicit above. The seventh review found no in-scope issues. A separate inherited SSH file-drop/picker upload path has no pending draft entry: if its composer closes before the upload finishes, the result can be lost. That separate path is deferred for a follow-up repair; the pending-paste and paired-file paths covered here are listed above.
Agent skill upstream boundary
Notes
No host/session identity or wire contract changes. No real-provider login or account configuration is required for QA. Final main synchronization is complete and the reviewed patch is unchanged. Final-head CI is fully complete with no failures; all ten unit shards ran and passed. The inherited SSH file-drop/picker limitation remains deferred as described above.
Checklist