Repository navigation
Preserve agent chats and drafts through reconnects - #9242
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:
📝 WalkthroughWalkthroughNative chat composer state is now keyed by stable pane identity, with shared bounded scope caches and extracted composer types. Session resolution retains prior conversation identity during reconnect metadata gaps. Leaf routing keeps mounted chat ownership through transient eligibility loss and uses active-leaf eligibility for synchronization. PTY lifecycle wiring now handles confirmed agent exits and provider PTY rebinding. Remote PTY handling distinguishes stale handles from terminal removal, reconnects with replacement handles, and preserves rendered TUI and draft content. Tests cover these persistence, routing, and recovery paths. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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
Run ID: a9896164-30b4-4660-948c-85ca5a3b7fe6
📒 Files selected for processing (10)
src/renderer/src/components/native-chat/native-chat-leaf-routing.test.tssrc/renderer/src/components/native-chat/native-chat-leaf-routing.tssrc/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/pty-connection-types.tssrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts
- src/renderer/src/components/native-chat/native-chat-leaf-routing.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a78e5d00-9f47-4c65-9e5d-f12ff431fda2
📒 Files selected for processing (10)
config/reliability-gates.jsoncsrc/renderer/src/components/terminal-pane/pty-connection-types.tssrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tssrc/renderer/src/store/slices/terminal-pty-identity-replacement.test.tssrc/renderer/src/store/slices/terminals.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/renderer/src/components/terminal-pane/pty-connection-types.ts
- src/renderer/src/components/terminal-pane/pty-connection.test.ts
- src/renderer/src/components/terminal-pane/pty-connection.ts
- src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts
- src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts
| const nextSuppressedPtyExitIds = { ...s.suppressedPtyExitIds } | ||
| delete nextSuppressedPtyExitIds[ptyId] | ||
| if (legacyRemotePtyId) { | ||
| delete nextSuppressedPtyExitIds[legacyRemotePtyId] | ||
| if (replacementPtyId) { | ||
| delete nextSuppressedPtyExitIds[replacementPtyId] | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Transfer the exit-suppression marker instead of dropping it.
If an intentional shutdown races with handle replacement, deleting the old marker makes the replacement PTY’s exit appear unsuppressed and may trigger normal exit handling. Move an existing marker to ptyId, matching the other replacement-keyed metadata.
Proposed fix
+ const hasReplacementSuppressedExit = replacementPtyId
+ ? replacementPtyId in s.suppressedPtyExitIds
+ : false
const nextSuppressedPtyExitIds = { ...s.suppressedPtyExitIds }
delete nextSuppressedPtyExitIds[ptyId]
if (replacementPtyId) {
delete nextSuppressedPtyExitIds[replacementPtyId]
+ if (hasReplacementSuppressedExit) {
+ nextSuppressedPtyExitIds[ptyId] = true
+ }
}Add a rotation test seeded with suppressedPtyExitIds[firstPtyId] to verify the marker follows successive replacements.
📝 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.
| const nextSuppressedPtyExitIds = { ...s.suppressedPtyExitIds } | |
| delete nextSuppressedPtyExitIds[ptyId] | |
| if (legacyRemotePtyId) { | |
| delete nextSuppressedPtyExitIds[legacyRemotePtyId] | |
| if (replacementPtyId) { | |
| delete nextSuppressedPtyExitIds[replacementPtyId] | |
| } | |
| const hasReplacementSuppressedExit = replacementPtyId | |
| ? replacementPtyId in s.suppressedPtyExitIds | |
| : false | |
| const nextSuppressedPtyExitIds = { ...s.suppressedPtyExitIds } | |
| delete nextSuppressedPtyExitIds[ptyId] | |
| if (replacementPtyId) { | |
| delete nextSuppressedPtyExitIds[replacementPtyId] | |
| if (hasReplacementSuppressedExit) { | |
| nextSuppressedPtyExitIds[ptyId] = true | |
| } | |
| } |
…ve-agent-chat-reconnect
* fix(chat): preserve drafts through reconnects * fix(terminal): bound stale handle reconnect polling * refactor(chat): extract native view props * fix(chat): harden reconnect lifecycle * fix(terminal): replace stale reconnect identities atomically * fix(terminal): recover mirrors after reconnect polling * docs(reliability): record merged reconnect coverage * fix(terminal): preserve reconnect lifecycle state * fix(terminal): keep reconnect polling truly bounded
Summary
Root cause
Native chat routing treated temporary loss of agent eligibility as completion and unmounted the chat surface. Its composer state was also keyed by replaceable PTY identity. Separately, the remote runtime transport grouped
terminal_handle_stalewithterminal_exited, so reconnect handle rotation invoked the same pane-close lifecycle as a dead terminal.The initial reconnect path fixed the immediate rotation but could still leave a mounted terminal permanently frozen after its bounded inventory polling expired: later host snapshots updated store state, but nothing rebound the already-mounted transport. Concurrent split panes also issued duplicate full
session.tabs.listrequests, and snapshot-first PTY replacement could strand state indexed by the old identity.Validation
mainpnpm run typecheckgit diff --checkReact Doctor still reports the existing
NativeChatViewstate-reset warnings outside this PR's changed lines; changing that state model here would be unrelated risk. The local environment uses Node 26.5.0 while the repository requests Node 24; all commands above passed.iOS reconnect evidence
Before:
After reconnect:
Reliability contract
terminal-provider.ssh-remote-reattach-contract): a mounted web terminal mirror keeps its pane, transcript, composer, and PTY ownership while a stale host handle is replaced. It never subscribes the known-stale handle again; explicit exit or accepted removed-surface evidence still retires the mirror. A replacement atomically supersedes the stale PTY identity.terminal_handle_stalewas classified as terminal death, and the first reconnect patch could re-subscribe the stale handle, retire a temporarily pending host surface, abort permanently on one inventory failure, freeze after bounded polling, issue one inventory RPC per split pane, or miss stale PTY-indexed state after snapshot-first replacement.terminal-provider.ssh-remote-reattach-contract; the merged provider suite passed 10 files / 566 tests. Live remote-runtime evidence remains partial rather than promoted coverage.