Skip to content

Commit 7283d31

Browse files
authored
fix(recovery): stop Escape from destroying recovered work (#1515)
Escape in the crash-recovery sheet was bound to the destructive Discard button, so the gesture macOS teaches as "not now" irreversibly unlinked every recovered buffer. The keyboard policy now lives on the choice itself: a destructive choice cannot carry a key equivalent, and the same value decides the button's role, its title and what it deletes, so the three cannot drift apart. Escape and Cmd-. resolve to a new Later button, which deletes nothing. Review found the sheet was only one of several routes to the same loss. The clean-quit sweep emptied the whole recovery directory, so quitting with the sheet on screen destroyed exactly the snapshots it was showing; the sweep is now scoped to snapshots belonging to open tabs, and `deleteAllRecoveryFiles` is gone. The offer could also return the user's own live dirty buffers, where Discard deleted the crash protection of files they were looking at; the offer now filters live tab IDs and suppresses itself while a restore is in flight. The sheet states how long undecided work is kept, the retention window is single-sourced with the launch sweep, and the destructive button's VoiceOver hint no longer says "files" when it means unsaved changes. Russian and French now use deletion verbs for Discard, which previously read as "dismiss" beside the new Later button. Closes #1503
1 parent deaa1c4 commit 7283d31

18 files changed

Lines changed: 5442 additions & 146 deletions

Pine/AccessibilityIdentifiers.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,12 @@ nonisolated enum AccessibilityID {
165165
static let paneDropOverlay = "paneDropOverlay"
166166
static func paneLeaf(_ id: String) -> String { "paneLeaf_\(id)" }
167167

168+
// MARK: - Crash recovery sheet (#1503)
169+
static let recoverySheet = "recoverySheet"
170+
static let recoveryDiscardButton = "recoveryDiscardButton"
171+
static let recoveryLaterButton = "recoveryLaterButton"
172+
static let recoveryRecoverAllButton = "recoveryRecoverAllButton"
173+
168174
// MARK: - Toast notifications
169175
static let toastNotification = "toastNotification"
170176

Pine/AlertTemplate.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,12 @@ extension AlertTemplate {
292292
/// `.cancel` — Escape / ⌘-. target (Cancel, Keep). A single-button
293293
/// OK alert uses `.default` since there is nothing to cancel.
294294
/// `.destructive`— Don't Save / Discard / Quit — confirmed data loss.
295+
///
296+
/// No template ever gives one button both `.destructive` and `.cancel`:
297+
/// Escape is how people dismiss dialogs without reading them. The
298+
/// crash-recovery sheet is the one dialog in Pine built in SwiftUI rather
299+
/// than from these templates, and it restates the same policy in
300+
/// `RecoveryDialogChoice` (#1503). Change either and check the other.
295301
private var buttonRoles: [AlertButtonRole] {
296302
switch self {
297303
case .unsavedChangesSingle, .unsavedChangesBulk:

Pine/ContentView+Helpers.swift

Lines changed: 90 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,26 @@ extension ContentView {
7373
return .restored(result)
7474
}
7575

76-
func checkForRecovery() {
77-
guard let entries = projectManager.recoveryManager?.pendingRecoveryEntries(),
78-
!entries.isEmpty else { return }
76+
/// Discovers the crash-recovery offer for this project and presents it.
77+
///
78+
/// `async` and awaited by the caller rather than launched into a detached
79+
/// `Task`: `seedInitialTerminalIfNeeded(disposition:)` runs straight after
80+
/// it and its guard reads exactly the two properties this sets. Ordered
81+
/// explicitly so a pending offer can never lose the race and have a
82+
/// terminal seeded over the empty editor leaf the user is about to recover
83+
/// into — `theTaskAwaitsRecoveryDiscoveryBeforeSeeding` pins the sequence
84+
/// in `ContentView`'s `.task`.
85+
func checkForRecovery() async {
86+
// `pendingRecoveryOffer()` and not `pendingRecoveryEntries()`: SwiftUI
87+
// re-runs this `.task` on scene restoration and when the window is
88+
// closed and reopened, and the snapshots are deliberately still on
89+
// disk after "Later". Asking the project, which outlives the window,
90+
// is what keeps "not now" from meaning "again in ten seconds" (#1503).
91+
// It suspends: the directory listing reads and decodes every snapshot,
92+
// and those are whole unsaved buffers with no size limit, so it must
93+
// not happen on the main thread before the window draws.
94+
let entries = await projectManager.pendingRecoveryOffer()
95+
guard !entries.isEmpty else { return }
7996
recoveryEntries = entries
8097
showRecoveryDialog = true
8198
}
@@ -95,7 +112,9 @@ extension ContentView {
95112
/// - Only seeds on `.noSavedSession`. `restored`, `skipped`, and
96113
/// `deferred` never inject a terminal.
97114
/// - A pending recovery dialog means the user may restore real editor
98-
/// content — do not replace the empty leaf until they decide.
115+
/// content — do not replace the empty leaf until they decide. Since
116+
/// ``checkForRecovery()`` suspends, this guard is only meaningful
117+
/// because the caller `await`s it first; see the note there.
99118
/// - Defends against the empty editor leaf having been touched between
100119
/// the restore attempt and this call (e.g. a rapid sidebar click).
101120
func seedInitialTerminalIfNeeded(disposition: SessionStartupDisposition) {
@@ -144,24 +163,84 @@ extension ContentView {
144163
showRecoveryDialog = false
145164
recoveryEntries = []
146165

166+
// The restore is in flight from here until the `defer` below, and
167+
// `pendingRecoveryOffer()` is empty for as long as it is.
168+
//
169+
// Without that, `restorePendingEntries` parking on a large-file sheet
170+
// is a window in which a second sheet can be built from the same crash
171+
// entries: SwiftUI re-runs the scene's `.task` on restoration and on
172+
// close/reopen, `didAnswerRecoveryOffer` is deliberately still false
173+
// (a restore that never finishes must not silence the offer for good),
174+
// and both snapshots are still on disk under IDs no open tab owns. A
175+
// second Recover All then migrates them again — writing a snapshot
176+
// under a runtime ID no window owns, which comes back on the next
177+
// launch as a phantom "recovered file" — and leaves the parked restore
178+
// to resume against a detached `TabManager`.
179+
//
180+
// The flag and not `markRecoveryOfferAnswered()`, because they are
181+
// different claims: this says "being handled", that says "decided".
182+
// The two come apart exactly when the restorer hands entries back.
183+
//
184+
// No `Task.isCancelled` check: this is an unstructured task, which
185+
// inherits no cancellation, and its handle is discarded — nothing in
186+
// the app can cancel it, so the guard that used to sit after the
187+
// `await` could never fire and only made it look as though ⌘W were
188+
// being handled here. (It is not reachable in the first place: with
189+
// the sheet up, `documentWindow(for: NSApp.keyWindow)` resolves to the
190+
// sheet, whose delegate is not a `CloseDelegate`.)
191+
projectManager.beginRecoveryRestore()
147192
Task { @MainActor in
193+
defer { projectManager.endRecoveryRestore() }
148194
let retained = await recoveryManager.restorePendingEntries(
149195
entries,
150196
in: target,
151197
context: context
152198
)
153-
guard !Task.isCancelled else { return }
199+
// Answered once the restore has actually finished, not before the
200+
// `await`. A successful restore leaves live snapshots under the
201+
// recovered tabs' runtime IDs, and re-running `checkForRecovery()`
202+
// after a scene restart would otherwise offer the user their own
203+
// open buffers back as "recovered" (#1503). Anything the restorer
204+
// hands back stays on disk for the next launch either way.
205+
projectManager.markRecoveryOfferAnswered()
154206
recoveryEntries = retained
155207
showRecoveryDialog = !retained.isEmpty
156208
}
157209
}
158210

159-
func discardRecovery() {
160-
projectManager.recoveryManager?.deleteRecoveryFiles(
161-
for: recoveryEntries.map(\.0)
162-
)
163-
showRecoveryDialog = false
164-
recoveryEntries = []
211+
/// Applies the user's answer to the crash-recovery offer.
212+
///
213+
/// The single place in the app that can delete a displayed snapshot, and
214+
/// it holds no opinion about which answers delete: it asks the chosen
215+
/// option what it is allowed to unlink and passes that through.
216+
/// ``RecoveryDialogChoice/snapshotsToDelete(from:)`` answers with the
217+
/// empty list for everything but Discard, and it is the same value that
218+
/// decides the button's role and denies it a keyboard equivalent, so the
219+
/// three cannot drift apart. Written as an `if` here it would be one
220+
/// plausible "the guard above already handled the other case" edit away
221+
/// from #1503 — in a file the coverage gate excludes and no unit test
222+
/// loads.
223+
///
224+
/// Escape and ⌘-. resolve to ``RecoveryDialogChoice/later``, which unlinks
225+
/// nothing: the clean-quit sweep only removes snapshots belonging to open
226+
/// tabs (``RecoveryManager/deleteSnapshotsOfOpenTabs(_:)``), and these
227+
/// belong to none, so they stay on disk and the offer returns on the next
228+
/// launch (#1503).
229+
///
230+
/// Exhaustive and without a `default`, so a fourth choice is a compile
231+
/// error here rather than a silent "close the sheet and delete nothing".
232+
func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) {
233+
switch choice {
234+
case .recoverAll:
235+
recoverTabs()
236+
case .discard, .later:
237+
projectManager.recoveryManager?.deleteSnapshots(
238+
withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries)
239+
)
240+
projectManager.markRecoveryOfferAnswered()
241+
showRecoveryDialog = false
242+
recoveryEntries = []
243+
}
165244
}
166245

167246
/// Reads `PINE_SEARCH_QUERY` from the environment (used by UI tests) and

Pine/ContentView.swift

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,14 @@ struct ContentView: View {
164164
if case .restored(let result) = disposition, result.didRestoreEditorTabs {
165165
refreshLineDiffs()
166166
}
167-
checkForRecovery()
167+
// Awaited, not fired off: recovery discovery reads the snapshot
168+
// directory off the main actor (#1503), so it suspends, and the
169+
// seeding call below guards on `showRecoveryDialog` and
170+
// `recoveryEntries` — the two properties this sets. Running them
171+
// concurrently would let a terminal be seeded over the empty
172+
// editor leaf a pending offer is about to recover into.
173+
// `theTaskAwaitsRecoveryDiscoveryBeforeSeeding` pins this order.
174+
await checkForRecovery()
168175
// #1251: a project with no saved session and no pending recovery
169176
// opens directly into a focused terminal rooted in the project,
170177
// instead of an empty editor canvas. This runs only after session
@@ -194,8 +201,7 @@ struct ContentView: View {
194201
.sheet(isPresented: $showRecoveryDialog) {
195202
RecoveryDialogView(
196203
entries: recoveryEntries,
197-
onRecover: { recoverTabs() },
198-
onDiscard: { discardRecovery() }
204+
onChoose: { resolveRecoveryOffer($0) }
199205
)
200206
}
201207
.overlay {

0 commit comments

Comments
 (0)