Repository navigation
Fix session scrollback loss on Sparkle updates and red-X window close - #2894
KochMartin wants to merge 2 commits into
Conversation
`updaterWillRelaunchApplication` wrapped its cleanup in `Task { @mainactor in … }`,
which schedules the body asynchronously and returns to Sparkle immediately.
Sparkle then proceeded to terminate + relaunch while the snapshot save raced
with `applicationWillTerminate`'s own save + `TerminalController.stop()`
sequence, occasionally losing scrollback on update flows.
Run the body synchronously with `MainActor.assumeIsolated` so the snapshot
is flushed to disk before Sparkle tears anything down.
When a user closed a window with the traffic-light button while the app stayed running, `unregisterMainWindow` saved the session with `includeScrollback: false` and `removeWhenEmpty: true`. The effects: 1. Remaining windows' scrollback was dropped on every close. 2. Closing the last window deleted the snapshot file entirely, so the next launch started blank instead of restoring the prior session. Capture scrollback for the remaining windows and keep the snapshot file around even when no windows remain. The `shouldPersistSnapshotOnWindowUnregister` gate still skips this save during app termination (where `applicationWillTerminate` has already written a full snapshot). Drop the now-unused `shouldRemoveSnapshotWhenNoWindowsRemainOnWindowUnregister` policy function and its test assertions.
|
@KochMartin is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughWindow-unregister snapshot persistence logic is simplified by removing a helper function, and update-relaunch operations are converted from asynchronous Task scheduling to synchronous main-actor execution to prevent races with application termination. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes two independent snapshot-loss bugs: a Sparkle relaunch race where The only nit is a comment at Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User / Sparkle
participant UD as UpdateDelegate
participant AD as AppDelegate
participant SP as SessionPersistenceStore
Note over U,SP: Sparkle update relaunch (fixed)
U->>UD: updaterWillRelaunchApplication()
UD->>AD: "MainActor.assumeIsolated { persistSessionForUpdateRelaunch() }"
AD->>SP: saveSessionSnapshot(includeScrollback: true) [sync]
SP-->>AD: snapshot written ✓
AD-->>UD: return
UD-->>U: return
U->>U: terminate + relaunch
Note over U,SP: Red-X window close (fixed)
U->>AD: window willClose (NSWindow.willCloseNotification)
AD->>AD: unregisterMainWindow(window)
AD->>AD: "shouldPersistSnapshotOnWindowUnregister? (isTerminatingApp=false → true)"
AD->>SP: saveSessionSnapshot(includeScrollback: true, removeWhenEmpty: false) [async]
SP-->>AD: snapshot preserved (or no-op if last window) ✓
|
2 participants
Summary
Two independent bugs caused the session snapshot (including scrollback) to be lost in ways that matched real-world reports of "cmux forgets my terminal content after restart":
updaterWillRelaunchApplicationwrapped its cleanup inTask { @MainActor in … }, which schedules the body asynchronously and returns to Sparkle immediately. Sparkle then proceeded to terminate + relaunch while the snapshot save raced withapplicationWillTerminate's own save +TerminalController.stop()sequence. Depending on the interleaving, the update could either skip the scrollback save entirely or overwrite a good snapshot with one captured after terminals had been torn down. Fix: run the body synchronously withMainActor.assumeIsolatedso the snapshot is flushed to disk before Sparkle touches anything else. Pattern matches existing usage inGhosttyTerminalView.swift.unregisterMainWindowsaved withincludeScrollback: falseandremoveWhenEmpty: truefor every red-X close while the app stayed running. That meant (a) remaining windows' scrollback was cleared on every close, and (b) closing the last window deleted the snapshot file entirely — so the next launch started blank instead of restoring the prior session. Fix: capture scrollback for the remaining windows and keep the snapshot file around even when no windows remain. TheshouldPersistSnapshotOnWindowUnregistergate still skips this save during app termination, whereapplicationWillTerminatehas already written a full snapshot with scrollback. The now-unusedshouldRemoveSnapshotWhenNoWindowsRemainOnWindowUnregisterpolicy function and its test assertions are removed.Scope is intentionally limited to these two fixes. Broader capture-path improvements (scrollback on autosave, sleep/wake observers, etc.) are out of scope for this PR.
Test plan
testWindowUnregisterSnapshotPersistencePolicystill passes with the trimmed assertions.~/Library/Application Support/cmux/session-*.jsoncontainsscrollbackfields after each of the above flows.Notes for reviewers
I could not run a full local build — the machine doesn't have
ziginstalled andGhosttyKit.xcframeworkis not prebuilt. The Swift diff is small (~20 lines across 3 files) and has been reviewed via the code-reviewer agent. Please run CI and a local build before merging.Summary by cubic
Fixes two cases where session scrollback was lost: during
Sparkleupdate relaunch and when closing windows with the red X. Scrollback now survives updates and relaunches, even after closing the last window.Bug Fixes
updaterWillRelaunchApplicationrun synchronously viaMainActor.assumeIsolatedso the snapshot is flushed beforeSparkleterminates, avoiding races withapplicationWillTerminate/TerminalController.stop().unregisterMainWindow, save withincludeScrollback: trueandremoveWhenEmpty: falsewhile the app stays running, preserving scrollback and keeping the snapshot after last-window close; still skipped during app termination.Refactors
shouldRemoveSnapshotWhenNoWindowsRemainOnWindowUnregisterand its test assertions.Written for commit 04846b4. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor