Repository navigation
Conversation
Terminals restore with no scrollback whenever shell-integration state is unavailable (.unknown/nil): the conservative needsConfirmClose fallback routes through close-confirmation and skips persistence. This test encodes the correct contract (persist unless a command is positively running) and fails against the current implementation. Co-authored-by: Claude <noreply@anthropic.com>
…i#2823) Regression introduced by 5f074f8 (2026-03-13, "Fix session restore replay for transient terminal states"). Before that commit, session snapshots always captured terminal scrollback when includeScrollback was set. It added a shouldPersistScrollback gate to avoid persisting transient/mid-command screens; the gate became `!closeConfirmationRequired`, derived from the panel's shell-activity state: .promptIdle -> persist .commandRunning -> skip .unknown / nil -> needsConfirmClose() fallback (conservative; usually skip) So any terminal whose shell-activity state is .unknown/nil at snapshot time silently stopped persisting scrollback and restores empty. Fix (faithful to 5f074f8's intent of skipping only transient states): persist for .promptIdle AND .unknown/nil, skip only .commandRunning. shouldReplaySessionScrollback still excludes agent/tmux/resume terminals; the close-confirmation *warning* path (resolveCloseConfirmation) is unchanged. This is defense-in-depth: scrollback should never be dropped just because we are unsure the shell is idle. How it was found / repro: - Prod session files have no `scrollback` key for plain idle terminals (live prod: 7 plain terminals, 0 with scrollback). - Instrumenting the snapshot gate showed capturedScrollback was skipped because shellState==nil -> closeConfirmationRequired==true -> shouldPersist==false; the read path (VT export / live-surface) was never reached. - Harness gotcha: builds via scripts/reload.sh have working shell-integration reporting (shellState becomes .promptIdle, scrollback persists), while plain `xcodebuild` builds do not (report_shell_state never fires). Debug-vs-Release was a red herring. Verify persistence behavior with shellState forced (WorkspaceSessionRestorePolicyServiceTests), not by eyeballing a local build. Open follow-up (separate from this change): prod's shell integration DOES report (socket live, CMUX_TAB_ID/PANEL_ID set, sends "prompt"), and v0.64.16 has the full report->parse chain, yet shellState is still .unknown at the quit snapshot. The reported state is not landing as .promptIdle by snapshot time -- likely a recording/keying/timing issue in the report->shellState pipeline (candidate fixes already on main, e.g. "stable surface ids across restore"). This change makes scrollback robust regardless, but the reporting-to-snapshot gap is worth its own investigation. Co-authored-by: Claude <noreply@anthropic.com>
|
@grantland is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
@grantland I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,260 of the 240,000 allowed lines of code this month. Reviews resume on 1 July 2026 (in 9 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesScrollback Persistence Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (21 passed)
✨ 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 |
|
✅ Action performedReview finished.
|
Greptile SummaryThis PR narrows the scrollback persistence gate from
Confidence Score: 5/5Safe to merge — the change is a targeted one-liner fix with exhaustive test coverage at both the package and app level. The gate change is correct for all three enum cases plus nil: PanelShellActivityState has exactly .unknown, .promptIdle, and .commandRunning — all are covered by the tests. All call sites have been updated, the prior dead-parameter issue flagged in review threads is fully resolved, and resolveCloseConfirmation remains in place for the close-confirmation warning path (unchanged). The only acknowledged open item is a deeper record-side issue (shell state staying .unknown even when reports arrive) which is explicitly tracked as a follow-up and does not affect the correctness of this fix. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Session Save / Quit] --> B[panelShellActivityStates panelId]
B --> C{shellActivityState?}
C -- nil --> D[Persist scrollback ✅ was: conservative needsConfirmClose → skip ❌]
C -- .unknown --> E[Persist scrollback ✅ was: needsConfirmClose → skip ❌]
C -- .promptIdle --> F[Persist scrollback ✅]
C -- .commandRunning --> G[Skip scrollback ✅]
D --> H[TerminalController readTerminalTextForSnapshot]
E --> H
F --> H
H --> I[SessionTerminalPanelSnapshot scrollback: content]
G --> J[SessionTerminalPanelSnapshot scrollback: nil]
%%{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"}}}%%
flowchart TD
A[Session Save / Quit] --> B[panelShellActivityStates panelId]
B --> C{shellActivityState?}
C -- nil --> D[Persist scrollback ✅ was: conservative needsConfirmClose → skip ❌]
C -- .unknown --> E[Persist scrollback ✅ was: needsConfirmClose → skip ❌]
C -- .promptIdle --> F[Persist scrollback ✅]
C -- .commandRunning --> G[Skip scrollback ✅]
D --> H[TerminalController readTerminalTextForSnapshot]
E --> H
F --> H
H --> I[SessionTerminalPanelSnapshot scrollback: content]
G --> J[SessionTerminalPanelSnapshot scrollback: nil]
Reviews (5): Last reviewed commit: "Remove now-dead fallbackNeedsConfirmClos..." | Re-trigger Greptile |
Greptile SummaryThis PR fixes terminal scrollback not being captured at session save, causing terminals to restore empty after quit/relaunch. The root cause was that the scrollback persistence gate was coupled to close-confirmation logic: when
Confidence Score: 4/5The core change is narrow and well-tested; the static wrapper's dead The scrollback persistence logic is simple and correct, and both a package-level unit test and an app-level regression test cover all meaningful states. The one rough edge is the static Sources/Workspace.swift — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Session snapshot triggered] --> B{shellActivityState}
B -->|commandRunning| C[shouldPersist = false, scrollback skipped]
B -->|promptIdle| D[shouldPersist = true]
B -->|unknown or nil| E[shouldPersist = true, FIXED]
D --> F{shouldReplaySessionScrollback?}
E --> F
F -->|hasRestorableAgent / tmux / resumeWork| G[replay = false, no scrollback written]
F -->|plain idle terminal| H[readTerminalTextForSnapshot]
H --> I[scrollback key written to snapshot]
%%{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"}}}%%
flowchart TD
A[Session snapshot triggered] --> B{shellActivityState}
B -->|commandRunning| C[shouldPersist = false, scrollback skipped]
B -->|promptIdle| D[shouldPersist = true]
B -->|unknown or nil| E[shouldPersist = true, FIXED]
D --> F{shouldReplaySessionScrollback?}
E --> F
F -->|hasRestorableAgent / tmux / resumeWork| G[replay = false, no scrollback written]
F -->|plain idle terminal| H[readTerminalTextForSnapshot]
H --> I[scrollback key written to snapshot]
Reviews (1): Last reviewed commit: "Persist scrollback unless a command is p..." | Re-trigger Greptile |
| nonisolated static func shouldPersistSessionScrollback( | ||
| shellActivityState: PanelShellActivityState?, | ||
| fallbackNeedsConfirmClose: Bool | ||
| ) -> Bool { | ||
| makeSessionRestorePolicyService().shouldPersistSessionScrollback( | ||
| closeConfirmationRequired: resolveCloseConfirmation( | ||
| shellActivityState: shellActivityState, | ||
| fallbackNeedsConfirmClose: fallbackNeedsConfirmClose | ||
| ) | ||
| _ = fallbackNeedsConfirmClose | ||
| return makeSessionRestorePolicyService().shouldPersistSessionScrollback( | ||
| shellActivityState: shellActivityState | ||
| ) | ||
| } |
There was a problem hiding this comment.
The
fallbackNeedsConfirmClose parameter is now explicitly discarded via _ = fallbackNeedsConfirmClose. The only callers of this static overload today are in the test target, and they pass fallbackNeedsConfirmClose: true specifically to verify the parameter is ignored — confirming it has no production callers. Keeping a dead public parameter in a nonisolated static production method leaves a misleading API surface: any future caller that passes true expecting conservative gating will silently get persistence instead. Consider removing the parameter (the test target can be updated, or the tests can call the new instance-method form directly via @testable import).
| nonisolated static func shouldPersistSessionScrollback( | |
| shellActivityState: PanelShellActivityState?, | |
| fallbackNeedsConfirmClose: Bool | |
| ) -> Bool { | |
| makeSessionRestorePolicyService().shouldPersistSessionScrollback( | |
| closeConfirmationRequired: resolveCloseConfirmation( | |
| shellActivityState: shellActivityState, | |
| fallbackNeedsConfirmClose: fallbackNeedsConfirmClose | |
| ) | |
| _ = fallbackNeedsConfirmClose | |
| return makeSessionRestorePolicyService().shouldPersistSessionScrollback( | |
| shellActivityState: shellActivityState | |
| ) | |
| } | |
| nonisolated static func shouldPersistSessionScrollback( | |
| shellActivityState: PanelShellActivityState? | |
| ) -> Bool { | |
| makeSessionRestorePolicyService().shouldPersistSessionScrollback( | |
| shellActivityState: shellActivityState | |
| ) | |
| } |
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!
| nonisolated static func shouldPersistSessionScrollback( | ||
| shellActivityState: PanelShellActivityState?, | ||
| fallbackNeedsConfirmClose: Bool | ||
| ) -> Bool { | ||
| makeSessionRestorePolicyService().shouldPersistSessionScrollback( | ||
| closeConfirmationRequired: resolveCloseConfirmation( | ||
| shellActivityState: shellActivityState, | ||
| fallbackNeedsConfirmClose: fallbackNeedsConfirmClose | ||
| ) | ||
| _ = fallbackNeedsConfirmClose | ||
| return makeSessionRestorePolicyService().shouldPersistSessionScrollback( | ||
| shellActivityState: shellActivityState | ||
| ) | ||
| } |
There was a problem hiding this comment.
Dead parameter misleads future callers
fallbackNeedsConfirmClose is retained in this internal static wrapper but immediately discarded with _ = fallbackNeedsConfirmClose, meaning every caller sees a parameter that appears to influence persistence but has no effect. Because this is a nonisolated static (not public) method whose only callers are the test file, the parameter can be removed without an API compatibility concern — and doing so makes the contract unambiguous at every call site. The current form leaves bad state representable: a reader (or future caller) passing fallbackNeedsConfirmClose: true cannot tell without reading the body that the value is ignored.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
Greptile SummaryThis PR fixes a regression where terminal scrollback was silently dropped at session save whenever
Confidence Score: 4/5The logic change is narrow and correct: scrollback now persists for idle and unknown shell states, skipping only when a command is positively running. The core fix in WorkspaceSessionRestorePolicyService and the Workspace.swift snapshot call-site are clean. The one remaining rough edge is the static wrapper retaining a fallbackNeedsConfirmClose parameter that is silently discarded — any future caller passing it believes it has an effect when it does not. Sources/Workspace.swift — the static shouldPersistSessionScrollback wrapper retains and silently discards fallbackNeedsConfirmClose. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Session snapshot] --> B{shellActivityState}
B -->|nil or unknown| C[shouldPersistScrollback = true]
B -->|promptIdle| C
B -->|commandRunning| D[shouldPersistScrollback = false]
C --> E{shouldReplayScrollback?}
E -->|agent / tmux / resume| F[skip - no scrollback key]
E -->|plain terminal| G[capture via readTerminalTextForSnapshot]
G --> H[scrollback key written to snapshot]
D --> F
H --> I[terminal restores with content]
F --> J[terminal restores empty]
%%{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"}}}%%
flowchart TD
A[Session snapshot] --> B{shellActivityState}
B -->|nil or unknown| C[shouldPersistScrollback = true]
B -->|promptIdle| C
B -->|commandRunning| D[shouldPersistScrollback = false]
C --> E{shouldReplayScrollback?}
E -->|agent / tmux / resume| F[skip - no scrollback key]
E -->|plain terminal| G[capture via readTerminalTextForSnapshot]
G --> H[scrollback key written to snapshot]
D --> F
H --> I[terminal restores with content]
F --> J[terminal restores empty]
Reviews (3): Last reviewed commit: "Persist scrollback unless a command is p..." | Re-trigger Greptile |
| nonisolated static func shouldPersistSessionScrollback( | ||
| shellActivityState: PanelShellActivityState?, | ||
| fallbackNeedsConfirmClose: Bool | ||
| ) -> Bool { | ||
| makeSessionRestorePolicyService().shouldPersistSessionScrollback( | ||
| closeConfirmationRequired: resolveCloseConfirmation( | ||
| shellActivityState: shellActivityState, | ||
| fallbackNeedsConfirmClose: fallbackNeedsConfirmClose | ||
| ) | ||
| _ = fallbackNeedsConfirmClose | ||
| return makeSessionRestorePolicyService().shouldPersistSessionScrollback( | ||
| shellActivityState: shellActivityState | ||
| ) | ||
| } |
There was a problem hiding this comment.
Dead parameter silently ignored in static wrapper.
fallbackNeedsConfirmClose is accepted but immediately discarded via _ = fallbackNeedsConfirmClose. The only callers today are in cmuxTests/SessionPersistenceTests.swift, so the parameter does nothing in production or in tests. This leaves future callers with a plausible-but-false belief that the parameter influences the result. Either remove it outright and update the test call-sites, or mark it deprecated — the tests already assert the new contract without relying on fallbackNeedsConfirmClose doing anything.
| nonisolated static func shouldPersistSessionScrollback( | |
| shellActivityState: PanelShellActivityState?, | |
| fallbackNeedsConfirmClose: Bool | |
| ) -> Bool { | |
| makeSessionRestorePolicyService().shouldPersistSessionScrollback( | |
| closeConfirmationRequired: resolveCloseConfirmation( | |
| shellActivityState: shellActivityState, | |
| fallbackNeedsConfirmClose: fallbackNeedsConfirmClose | |
| ) | |
| _ = fallbackNeedsConfirmClose | |
| return makeSessionRestorePolicyService().shouldPersistSessionScrollback( | |
| shellActivityState: shellActivityState | |
| ) | |
| } | |
| @available(*, deprecated, message: "fallbackNeedsConfirmClose is no longer consulted; pass shellActivityState only.") | |
| nonisolated static func shouldPersistSessionScrollback( | |
| shellActivityState: PanelShellActivityState?, | |
| fallbackNeedsConfirmClose: Bool | |
| ) -> Bool { | |
| _ = fallbackNeedsConfirmClose | |
| return makeSessionRestorePolicyService().shouldPersistSessionScrollback( | |
| shellActivityState: shellActivityState | |
| ) | |
| } |
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Addresses Greptile P2: the static Workspace.shouldPersistSessionScrollback wrapper kept `fallbackNeedsConfirmClose` but discarded it (`_ = fallbackNeedsConfirmClose`), leaving a misleading API surface. The parameter was only retained for red/green test stability; persistence now depends solely on shell-activity state. Drop the parameter and update the test call sites to the single-argument form. Co-authored-by: Claude <noreply@anthropic.com>
|
Addressed in cc515c7 — removed the dead |
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
Wire TabManager.workspaceTabsWillChange to drain the pending buffer and apply each report against the now-reachable workspace. A report that landed before its workspace joined a manager now replays once it does, so idle terminals are recognized as .promptIdle instead of being stranded at .unknown. Only landed reports are cleared, so a surface still missing its panel retries on the next registration. This is the record-side half of the scrollback-restore work in #2823; the persistence-side workaround in #6615 only made scrollback tolerant of .unknown. Fixes the shellState gap that also degrades close-confirmation warnings and PR probing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire TabManager.workspaceTabsWillChange to drain the pending buffer and apply each report against the now-reachable workspace. A report that landed before its workspace joined a manager now replays once it does, so idle terminals are recognized as .promptIdle instead of being stranded at .unknown. Only landed reports are cleared, so a surface still missing its panel retries on the next registration. This is the record-side half of the scrollback-restore work in #2823; the persistence-side workaround in #6615 only made scrollback tolerant of .unknown. Fixes the shellState gap that also degrades close-confirmation warnings and PR probing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire TabManager.workspaceTabsWillChange to drain the pending buffer and apply each report against the now-reachable workspace. A report that landed before its workspace joined a manager now replays once it does, so idle terminals are recognized as .promptIdle instead of being stranded at .unknown. Only landed reports are cleared, so a surface still missing its panel retries on the next registration. This is the record-side half of the scrollback-restore work in #2823; the persistence-side workaround in #6615 only made scrollback tolerant of .unknown. Fixes the shellState gap that also degrades close-confirmation warnings and PR probing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The unknown shell-state scrollback loss remains in main’s current session restore policy; keeping this open for a current-main port. |
Summary
Fixes #2823. Likely also resolves #2016 (restore terminal scrollback content across app restarts) — confirm scope and change to
Fixes #2016if it fits.(The earlier
150dc9036addressed the glued prompt replay sub-symptom of #2823; this fixes the remaining half — scrollback was never captured/persisted, so terminals restored empty. Capture + the existing replay fix together make scrollback restore.)Related / adjacent (not closed by this PR):
ghostty_surface_needs_confirm_quit(same close-confirm machinery this PR decouples scrollback from)report_shell_statebutshellStatestays.unknownat snapshot time — tracked as its own follow-up.What changed?
Terminal scrollback is now persisted at session save unless the shell is positively running a command. Previously persistence was gated on
!closeConfirmationRequired, which coupled it to idle/close-confirmation detection: when a panel's shell-activity state was unavailable (.unknown/nil), the conservativeneedsConfirmClosefallback skipped persistence and the terminal restored empty.WorkspaceSessionRestorePolicyService.shouldPersistSessionScrollbacknow takesshellActivityStateand returns(state ?? .unknown) != .commandRunning(persist for.promptIdleand.unknown/nil; skip only.commandRunning).Workspace.swiftpasses the panel's shell state directly (the now-unusedcloseConfirmationRequiredlocal is removed).Workspace.shouldPersistSessionScrollbackstatic wrapper keeps its signature;fallbackNeedsConfirmCloseis explicitly retired (documented).shouldReplaySessionScrollbackstill excludes agent/tmux/resume terminals; the close-confirmation warning path (resolveCloseConfirmation) is unchanged.Why?
Regression from
5f074f810(2026-03-13, "Fix session restore replay for transient terminal states"). Before it, snapshots always captured scrollback whenincludeScrollbackwas set. That commit added a gate to avoid persisting transient/mid-command screens, but it over-applied: it treated.unknown(no shell-integration report yet) the same as transient, so any terminal without a fresh prompt-idle report stopped persisting scrollback. This fix narrows the gate to its original intent — skip only states we positively know are mid-command — so scrollback is no longer silently dropped when the shell state is merely unknown.Note: this is the resilient behavior fix. There is a separate, deeper record-side issue (see Testing) where prod delivers prompt-idle reports but
shellStatestill ends up.unknownat snapshot time; that is tracked as a follow-up and is not addressed here.Testing
How did you test this change?
SessionPersistenceTests.testSessionScrollbackPersistsUnlessCommandRunning) that asserts the corrected contract, then the fix. Added a package-level unit test (WorkspaceSessionRestorePolicyServiceTests.scrollbackPersistencePolicy) covering.promptIdle/.unknown/nil/.commandRunning.shellStateis always.unknown(the exact failing condition) now captures scrollback at quit (session snapshot gains ascrollbackkey with content) where it previously produced noscrollbackkey at all.What did you verify manually?
scrollbackkey for plain idle terminals (live prod: 7 plain terminals, 0 with scrollback).shellState == nil → closeConfirmationRequired == true → shouldPersistScrollback == false; the read path (VT export / live-surface) was never reached.CMUX_TAB_ID/CMUX_PANEL_IDset,_CMUX_SEND_TOOL=nc, socket repliesOK) — so the bug is record-side, not send-side.Manually tested locally: built the fix into a local Release-config app and confirmed at quit that a plain terminal with
shellState=nilnow writes ascrollbackkey with content to the session snapshot (previously the key was absent). Caveat: the automated red/green suites (swift test/xcodebuild test) run on CI — please confirm green there. Harness note: builds viascripts/reload.shhave working shell-integration reporting (shellStatebecomes.promptIdle), while plainxcodebuildbuilds do not — Debug-vs-Release was a red herring. Verify persistence withshellStateforced (the unit tests), not by eyeballing a local build.Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit