Repository navigation
feat(desktop): show a restored-session separator when a v2 terminal cold-restores - #5448
Conversation
…old-restores When the pty-daemon no longer owns a terminal's PTY (reboot, daemon loss), host-service silently respawned a fresh shell under the renderer's restored scrollback — a new prompt just appeared with no indication the session died. Inject a dim "─── Session Contents Restored ───" separator into the replay buffer ahead of the respawned shell's first output, so restored content and the fresh session are visibly distinct (like VS Code's "History restored"). Skipped when the daemon session is adopted (shell still alive across a host-service restart) — nothing died, so no separator.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughAdds a ChangesSession Restored Notice
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 5595d16 on Jul 4, 2026 12:21am UTC. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 1092-1097: The restored notice in terminal buffering can be
evicted by FIFO trimming in bufferOutput() before clients replay it. Update the
restored-notice path in terminal.ts so SESSION_RESTORED_NOTICE is protected from
MAX_BUFFER_BYTES eviction, either by reserving space for it or excluding it from
the oldest-chunk removal logic, and make sure the behavior is handled in the
restoredNotice/isAdopted branch that calls bufferOutput().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5010363-1fef-46e2-9a9d-34da0abecded
📒 Files selected for processing (2)
packages/host-service/src/terminal/terminal.adoption.node-test.tspackages/host-service/src/terminal/terminal.ts
Greptile SummaryThis PR injects a dim
Confidence Score: 4/5Safe to merge; the change is narrowly scoped to the WS cold-restore respawn path and does not touch adoption, fresh terminal creation, or any data path that existed before. The injection point (synchronously after sessions.set, before daemon.subscribe) guarantees ordering. The !isAdopted guard correctly suppresses the notice for live-session adoption and the session-already-exists fallback. The only note is a minor redundant dead-code pattern in both new tests. No files require special attention; the two new test cases cover the production paths well.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/terminal/terminal.ts | Adds SESSION_RESTORED_NOTICE constant and restoredNotice option to createTerminalSessionInternal; notice is injected synchronously into the empty buffer before daemon subscribe, correctly gated by !isAdopted; call site is limited to the WS cold-restore respawn path only. |
| packages/host-service/src/terminal/terminal.adoption.node-test.ts | Two new end-to-end tests covering the separator injection (fresh spawn) and absence (adopted session); tests are mutation-verified per the PR description. Minor: redundant assert.ok + if-guard pattern in both new tests. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant R as Renderer (WS client)
participant H as host-service (WS route)
participant D as pty-daemon
participant S as sessions Map
R->>H: WS connect (terminalId)
H->>H: resolveSessionForAttach()
H->>D: "adoptOnly=true, daemon.list()"
D-->>H: session not alive (cold restore)
Note over H: adoption failed
H->>D: daemon.open(terminalId, new shell)
D-->>H: pid (new PTY)
H->>S: sessions.set(terminalId, session)
Note over H: restoredNotice and not isAdopted
H->>S: bufferOutput SESSION_RESTORED_NOTICE
Note over S: buffer[0] = separator
H->>D: daemon.subscribe replay true
D-->>H: onOutput shell output bufferOutput
Note over S: buffer[1..n] = new shell output
H-->>R: attachSocketToSession replayBuffer
Note over R: Sees separator then new prompt
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant R as Renderer (WS client)
participant H as host-service (WS route)
participant D as pty-daemon
participant S as sessions Map
R->>H: WS connect (terminalId)
H->>H: resolveSessionForAttach()
H->>D: "adoptOnly=true, daemon.list()"
D-->>H: session not alive (cold restore)
Note over H: adoption failed
H->>D: daemon.open(terminalId, new shell)
D-->>H: pid (new PTY)
H->>S: sessions.set(terminalId, session)
Note over H: restoredNotice and not isAdopted
H->>S: bufferOutput SESSION_RESTORED_NOTICE
Note over S: buffer[0] = separator
H->>D: daemon.subscribe replay true
D-->>H: onOutput shell output bufferOutput
Note over S: buffer[1..n] = new shell output
H-->>R: attachSocketToSession replayBuffer
Note over R: Sees separator then new prompt
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/host-service/src/terminal/terminal.adoption.node-test.ts:352-354
**Dead-code guard after a throwing assertion**
`assert.ok(!("error" in result))` throws when the condition is false, so the `if ("error" in result) return;` immediately below is unreachable and adds noise. The identical pattern appears in the second test (`assert.ok(!("error" in second))` then `if ("error" in second) return;`). Pick one: keep only the `assert.ok` (preferred — gives a clear failure message), or drop the `assert.ok` and rely on the type-narrowing `if`.
Reviews (1): Last reviewed commit: "feat(desktop): show a restored-session s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| listed: true, | ||
| restoredNotice: true, | ||
| }); |
There was a problem hiding this comment.
assert.ok(!("error" in result)) throws when the condition is false, so the if ("error" in result) return; immediately below is unreachable and adds noise. The identical pattern appears in the second test (assert.ok(!("error" in second)) then if ("error" in second) return;). Pick one: keep only the assert.ok (preferred — gives a clear failure message), or drop the assert.ok and rely on the type-narrowing if.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/terminal.adoption.node-test.ts
Line: 352-354
Comment:
**Dead-code guard after a throwing assertion**
`assert.ok(!("error" in result))` throws when the condition is false, so the `if ("error" in result) return;` immediately below is unreachable and adds noise. The identical pattern appears in the second test (`assert.ok(!("error" in second))` then `if ("error" in second) return;`). Pick one: keep only the `assert.ok` (preferred — gives a clear failure message), or drop the `assert.ok` and rely on the type-narrowing `if`.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Keeping as-is: assert.ok gives the failure message and the if guard does the TypeScript narrowing — this pattern is the established idiom throughout this file (19 occurrences); changing only the new tests would be inconsistent.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/terminal/terminal.ts">
<violation number="1" location="packages/host-service/src/terminal/terminal.ts:1095">
P2: The restored-session separator is inserted as the oldest buffer chunk via `bufferOutput`, but the same function evicts oldest chunks once `MAX_BUFFER_BYTES` (64 KB) is exceeded. If the newly spawned shell produces a burst of output before any client replays (e.g., a verbose shell init), the notice will be the first thing evicted and no client will ever see it.
Consider pinning the notice so it's excluded from byte-budget eviction, or re-injecting it at replay time rather than storing it in the FIFO buffer.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…replay FIFO Review finding (CodeRabbit/cubic): the separator was injected as the oldest FIFO chunk, so a >64KB output burst before any client attached would evict it first. Track it as restoredNoticePending on the session instead and let replayBuffer prepend it (after the mode preamble) on first replay — immune to MAX_BUFFER_BYTES eviction by construction. Adds a flood e2e test that fails under the old FIFO-injection approach.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ady closed replayBuffer cleared restoredNoticePending and the FIFO before sendBytes, which silently no-ops on a non-open socket — losing both for the next attach. Bail early instead so a dead socket leaves replay state intact.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
…t in terminal tests
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Summary
─── Session Contents Restored ───separator into the replay buffer ahead of the new shell's first output — so users can tell restored scrollback from the fresh session instead of a new prompt silently appearing under dead content.Why / Context
In v2, a cold restore is host-side: the renderer paints its persisted scrollback, then the WS attach finds an
activesession row whose PTY the daemon no longer owns, and host-service respawns a fresh shell (packages/host-service/src/terminal/terminal.tsWS route fallback). Previously nothing marked the boundary — users saw their old content with a brand-new prompt below and no hint the session had died. Other terminal apps mark this (VS Code's "History restored", iTerm2's session-restored banner).How It Works
restoredNoticeoption oncreateTerminalSessionInternal, set only by the WS respawn fallback.createTerminalSessionInternal(not at the call site) dedupes concurrent attaches racing into the respawn path, and the!isAdoptedguard suppresses the notice when the daemon still owns a live shell (host-service restart adoption) — nothing died, so no separator.Manual QA Checklist
Verified end-to-end in the dev desktop app over CDP:
Testing
bun run lint(clean)bun run typecheckfor@superset/host-service(clean)node --experimental-strip-types --test src/terminal/terminal.adoption.node-test.ts— 17/17 pass, including two new tests: separator is first in the replay buffer on a respawn; absent on adoption. Both were mutation-verified (breaking the guard or removing the injection fails the corresponding test). Note: these run under node ABI; anode_modulescurrently rebuilt for Electron will ERR_DLOPEN until rebuilt for node.Summary by cubic
Add a dim "─── Session Contents Restored ───" separator on v2 terminal cold-restore so users see where restored scrollback ends and the new shell begins. Skipped when adopting a still-live daemon session; survives buffer eviction and closed-socket attaches.
New Features
restoredNoticetocreateTerminalSessionInternal; set only by the WS respawn path and ignored whenisAdoptedis true.Bug Fixes
replayBuffernow prepends it on first attach so large output bursts can’t evict it.replayBuffernow bails early instead of clearing replay state.Written for commit 5595d16. Summary will update on new commits.
Summary by CodeRabbit