fix(recovery): stop Escape from destroying recovered work - #1515
Merged
Conversation
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
Contributor
✅ Code Coverage: 79.2415%Threshold: 70% Logic-only lines: 80259/101284 Coverage is at or above the required threshold. Generated by CI — see job summary for detailed file-level breakdown. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1503.
The reported bug
RecoveryDialogViewbound.keyboardShortcut(.cancelAction)to the destructive Discard button. Escape — the gesture macOS teaches as "not now" — irreversibly unlinked every recovered buffer, immediately after a crash, with no confirmation and no undo.What review found underneath it
Four independent review rounds, two to three reviewers each, found that the sheet was one of several routes to the same loss. Fixing the keystroke alone would have left the rest.
Quitting cleanly with the sheet on screen destroyed the snapshots it was showing.
applicationWillTerminateemptied the whole recovery directory whenever no tab was dirty — and recovery snapshots are not tabs, so a freshly restored session reported no unsaved changes. Cmd+Q instead of Escape, same outcome. The sweep is now scoped to snapshots belonging to open tabs, anddeleteAllRecoveryFilesno longer exists: the directory-wide delete cannot be called because there is no API for it.The sheet could offer the user their own live, open, dirty buffers. No crash required: edit a file, ⌘W (the project is retained in the background and deliberately snapshots), reopen — and the sheet offered the snapshots of the tabs on screen, under "unsaved changes from a previous session". Discard there deleted the crash protection of live work. The offer now filters live tab IDs.
A second offer could appear while a restore was still parked.
recoverTabscan suspend indefinitely on the large-file prompt; a scene re-run then re-offered the same entries, and a second Recover All produced an orphaned snapshot owned by no window. The offer is now suppressed while a restore is in flight.The shape that made the fix hold
Three review rounds each found the same class of defect one layer down: the destructive decision sitting somewhere no test executes. So the decision moved onto the type.
RecoveryDialogChoicederives the button's role, its title, its key equivalents and what it deletes from one value —snapshotsToDelete(from:)returns an empty array for every non-destructive choice, and the resolver calls delete unconditionally inside an exhaustiveswitchwith nodefault:. A destructive choice cannot carry a key equivalent, structurally.Where the call site is unreachable to unit tests (
ContentView+Helpers.swiftis excluded from the coverage gate and cannot be loaded), its shape is pinned by source scanners that are themselves tested against violating fixtures. The scanners' blind spots are enumerated in the file, including the two found in the last round.Verification
Four hosted tests drive a real
NSHostingView, send realperformKeyEquivalentevents (Escape, Cmd-., Return, keypad Enter), through a realRecoveryManagerover a real temp directory, and then look at the.jsonfiles. Two tests run the realAppDelegate.applicationWillTerminatethrough the registry.Across the five iterations, mutations were applied one at a time, reverted, and verified byte-identical by sha256. The final round's fourteen include the original bug verbatim, the resolver's three unpinned bookkeeping statements (dropping
showRecoveryDialog = falsemakes the sheet unclosable — Escape routes into that branch, so the only exits become the irreversible button or force-quit), the schema gate that deleted readable work,pm.allTabsnarrowed to one pane, and a retention-key change that renders the raw key on screen while two older tests stay green.6873 tests / 444 suites serially: 4 failures, all
AgentInboxToolbarButtonSnapshotTestspixel drift (2.9–3.3% against a 2% tolerance) on baselines recorded on CI macOS 26; per project policy they are not re-recorded locally on macOS 27.swiftlint --strictclean across 784 files;check-no-post-under-inout.pyclean;xcodebuild buildsucceeded. Environment: macOS 27.0 (26A5416b), Xcode 27.0 (27A5237l), SDK 27.0 (26A5406c). UI tests were not run locally.Localization
Three new keys across all nine languages, inserted as targeted text edits with no reserialization. Russian and French Discard changed from «Отклонить» / « Ignorer » to «Удалить» / « Supprimer » — beside the new Later button, the old wording read as "just dismiss this", which pointed users at the irreversible control. The VoiceOver hint no longer says "files" when it means unsaved changes, and neither does the translator comment that produced those mistranslations. Plural forms verified per CLDR category, including that Russian selects
manyfor 7 and that «в течение» requires the genitive.Known limitations, deliberately not addressed here
open—EACCES, a bad block — is kept whatever its dates say, because a file the process could not read has not told anybody how old it is, and launch is when every language server, watcher and terminal starts at once. Such a file also prevents its project's recovery subdirectory from ever being collected, so accumulation is unbounded. Trading possible loss of real unsaved work against a leaked directory is the right way round, but the leak is real.hasPendingRecoveryis now dead production API, called only by tests. Left in place rather than removed in a branch already this wide.TabManagerdetached byremovePaneis not in the live set, so its tabs' snapshots stay offerable. That is the safe direction — a closed pane's unsaved buffer is exactly what the sheet exists for — and it is named in the docstring..isDirectoryKeyreportsfalsefor a symlink to a directory, so the sweep does not descend into linked directories;removeItemon a directory containing a symlink removes the link, not its target; and there is no write race, because the sweep is synchronous on the main actor before any scene exists.Two issues were filed from findings that belong outside this branch: #1512 (cold open can lose a race with recovery setup and skip the offer) and #1507.