From 8648d0fdec957fc45a7b4e8be64f552e1685271c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A4=D0=B5=D0=B4=D0=BE=D1=80=20=D0=91=D0=B0=D1=82=D0=BE?= =?UTF-8?q?=D0=BD=D0=BE=D0=B3=D0=BE=D0=B2?= Date: Fri, 21 Aug 2026 12:33:47 +0300 Subject: [PATCH] fix(recovery): stop Escape from destroying recovered work 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 --- Pine/AccessibilityIdentifiers.swift | 6 + Pine/AlertTemplate.swift | 6 + Pine/ContentView+Helpers.swift | 101 +- Pine/ContentView.swift | 12 +- Pine/Localizable.xcstrings | 378 +++- Pine/PineApp.swift | 19 +- Pine/ProjectManager.swift | 100 ++ Pine/RecoveryDialogView.swift | 331 +++- Pine/RecoveryEntry.swift | 33 +- Pine/RecoveryManager.swift | 402 ++++- Pine/Strings.swift | 12 + .../DestructiveShortcutPolicyTests.swift | 1256 +++++++++++++ PineTests/ErrorHandlingTests.swift | 4 +- PineTests/PersistenceFixtureCorpusTests.swift | 15 +- .../RecoveryDialogEscapeSafetyTests.swift | 1565 +++++++++++++++++ PineTests/RecoveryManagerExtendedTests.swift | 46 +- PineTests/RecoveryManagerTests.swift | 75 +- PineTests/RecoveryTerminationSweepTests.swift | 1227 +++++++++++++ 18 files changed, 5442 insertions(+), 146 deletions(-) create mode 100644 PineTests/DestructiveShortcutPolicyTests.swift create mode 100644 PineTests/RecoveryDialogEscapeSafetyTests.swift create mode 100644 PineTests/RecoveryTerminationSweepTests.swift diff --git a/Pine/AccessibilityIdentifiers.swift b/Pine/AccessibilityIdentifiers.swift index 29655089..dcbeb6f9 100644 --- a/Pine/AccessibilityIdentifiers.swift +++ b/Pine/AccessibilityIdentifiers.swift @@ -165,6 +165,12 @@ nonisolated enum AccessibilityID { static let paneDropOverlay = "paneDropOverlay" static func paneLeaf(_ id: String) -> String { "paneLeaf_\(id)" } + // MARK: - Crash recovery sheet (#1503) + static let recoverySheet = "recoverySheet" + static let recoveryDiscardButton = "recoveryDiscardButton" + static let recoveryLaterButton = "recoveryLaterButton" + static let recoveryRecoverAllButton = "recoveryRecoverAllButton" + // MARK: - Toast notifications static let toastNotification = "toastNotification" diff --git a/Pine/AlertTemplate.swift b/Pine/AlertTemplate.swift index aff36d76..263ed5d2 100644 --- a/Pine/AlertTemplate.swift +++ b/Pine/AlertTemplate.swift @@ -292,6 +292,12 @@ extension AlertTemplate { /// `.cancel` — Escape / ⌘-. target (Cancel, Keep). A single-button /// OK alert uses `.default` since there is nothing to cancel. /// `.destructive`— Don't Save / Discard / Quit — confirmed data loss. + /// + /// No template ever gives one button both `.destructive` and `.cancel`: + /// Escape is how people dismiss dialogs without reading them. The + /// crash-recovery sheet is the one dialog in Pine built in SwiftUI rather + /// than from these templates, and it restates the same policy in + /// `RecoveryDialogChoice` (#1503). Change either and check the other. private var buttonRoles: [AlertButtonRole] { switch self { case .unsavedChangesSingle, .unsavedChangesBulk: diff --git a/Pine/ContentView+Helpers.swift b/Pine/ContentView+Helpers.swift index b8f1bb1e..3eff56a2 100644 --- a/Pine/ContentView+Helpers.swift +++ b/Pine/ContentView+Helpers.swift @@ -73,9 +73,26 @@ extension ContentView { return .restored(result) } - func checkForRecovery() { - guard let entries = projectManager.recoveryManager?.pendingRecoveryEntries(), - !entries.isEmpty else { return } + /// Discovers the crash-recovery offer for this project and presents it. + /// + /// `async` and awaited by the caller rather than launched into a detached + /// `Task`: `seedInitialTerminalIfNeeded(disposition:)` runs straight after + /// it and its guard reads exactly the two properties this sets. Ordered + /// explicitly so a pending offer can never lose the race and have a + /// terminal seeded over the empty editor leaf the user is about to recover + /// into — `theTaskAwaitsRecoveryDiscoveryBeforeSeeding` pins the sequence + /// in `ContentView`'s `.task`. + func checkForRecovery() async { + // `pendingRecoveryOffer()` and not `pendingRecoveryEntries()`: SwiftUI + // re-runs this `.task` on scene restoration and when the window is + // closed and reopened, and the snapshots are deliberately still on + // disk after "Later". Asking the project, which outlives the window, + // is what keeps "not now" from meaning "again in ten seconds" (#1503). + // It suspends: the directory listing reads and decodes every snapshot, + // and those are whole unsaved buffers with no size limit, so it must + // not happen on the main thread before the window draws. + let entries = await projectManager.pendingRecoveryOffer() + guard !entries.isEmpty else { return } recoveryEntries = entries showRecoveryDialog = true } @@ -95,7 +112,9 @@ extension ContentView { /// - Only seeds on `.noSavedSession`. `restored`, `skipped`, and /// `deferred` never inject a terminal. /// - A pending recovery dialog means the user may restore real editor - /// content — do not replace the empty leaf until they decide. + /// content — do not replace the empty leaf until they decide. Since + /// ``checkForRecovery()`` suspends, this guard is only meaningful + /// because the caller `await`s it first; see the note there. /// - Defends against the empty editor leaf having been touched between /// the restore attempt and this call (e.g. a rapid sidebar click). func seedInitialTerminalIfNeeded(disposition: SessionStartupDisposition) { @@ -144,24 +163,84 @@ extension ContentView { showRecoveryDialog = false recoveryEntries = [] + // The restore is in flight from here until the `defer` below, and + // `pendingRecoveryOffer()` is empty for as long as it is. + // + // Without that, `restorePendingEntries` parking on a large-file sheet + // is a window in which a second sheet can be built from the same crash + // entries: SwiftUI re-runs the scene's `.task` on restoration and on + // close/reopen, `didAnswerRecoveryOffer` is deliberately still false + // (a restore that never finishes must not silence the offer for good), + // and both snapshots are still on disk under IDs no open tab owns. A + // second Recover All then migrates them again — writing a snapshot + // under a runtime ID no window owns, which comes back on the next + // launch as a phantom "recovered file" — and leaves the parked restore + // to resume against a detached `TabManager`. + // + // The flag and not `markRecoveryOfferAnswered()`, because they are + // different claims: this says "being handled", that says "decided". + // The two come apart exactly when the restorer hands entries back. + // + // No `Task.isCancelled` check: this is an unstructured task, which + // inherits no cancellation, and its handle is discarded — nothing in + // the app can cancel it, so the guard that used to sit after the + // `await` could never fire and only made it look as though ⌘W were + // being handled here. (It is not reachable in the first place: with + // the sheet up, `documentWindow(for: NSApp.keyWindow)` resolves to the + // sheet, whose delegate is not a `CloseDelegate`.) + projectManager.beginRecoveryRestore() Task { @MainActor in + defer { projectManager.endRecoveryRestore() } let retained = await recoveryManager.restorePendingEntries( entries, in: target, context: context ) - guard !Task.isCancelled else { return } + // Answered once the restore has actually finished, not before the + // `await`. A successful restore leaves live snapshots under the + // recovered tabs' runtime IDs, and re-running `checkForRecovery()` + // after a scene restart would otherwise offer the user their own + // open buffers back as "recovered" (#1503). Anything the restorer + // hands back stays on disk for the next launch either way. + projectManager.markRecoveryOfferAnswered() recoveryEntries = retained showRecoveryDialog = !retained.isEmpty } } - func discardRecovery() { - projectManager.recoveryManager?.deleteRecoveryFiles( - for: recoveryEntries.map(\.0) - ) - showRecoveryDialog = false - recoveryEntries = [] + /// Applies the user's answer to the crash-recovery offer. + /// + /// The single place in the app that can delete a displayed snapshot, and + /// it holds no opinion about which answers delete: it asks the chosen + /// option what it is allowed to unlink and passes that through. + /// ``RecoveryDialogChoice/snapshotsToDelete(from:)`` answers with the + /// empty list for everything but Discard, and it is the same value that + /// decides the button's role and denies it a keyboard equivalent, so the + /// three cannot drift apart. Written as an `if` here it would be one + /// plausible "the guard above already handled the other case" edit away + /// from #1503 — in a file the coverage gate excludes and no unit test + /// loads. + /// + /// Escape and ⌘-. resolve to ``RecoveryDialogChoice/later``, which unlinks + /// nothing: the clean-quit sweep only removes snapshots belonging to open + /// tabs (``RecoveryManager/deleteSnapshotsOfOpenTabs(_:)``), and these + /// belong to none, so they stay on disk and the offer returns on the next + /// launch (#1503). + /// + /// Exhaustive and without a `default`, so a fourth choice is a compile + /// error here rather than a silent "close the sheet and delete nothing". + func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) { + switch choice { + case .recoverAll: + recoverTabs() + case .discard, .later: + projectManager.recoveryManager?.deleteSnapshots( + withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries) + ) + projectManager.markRecoveryOfferAnswered() + showRecoveryDialog = false + recoveryEntries = [] + } } /// Reads `PINE_SEARCH_QUERY` from the environment (used by UI tests) and diff --git a/Pine/ContentView.swift b/Pine/ContentView.swift index 2c9603b0..37680c6e 100644 --- a/Pine/ContentView.swift +++ b/Pine/ContentView.swift @@ -164,7 +164,14 @@ struct ContentView: View { if case .restored(let result) = disposition, result.didRestoreEditorTabs { refreshLineDiffs() } - checkForRecovery() + // Awaited, not fired off: recovery discovery reads the snapshot + // directory off the main actor (#1503), so it suspends, and the + // seeding call below guards on `showRecoveryDialog` and + // `recoveryEntries` — the two properties this sets. Running them + // concurrently would let a terminal be seeded over the empty + // editor leaf a pending offer is about to recover into. + // `theTaskAwaitsRecoveryDiscoveryBeforeSeeding` pins this order. + await checkForRecovery() // #1251: a project with no saved session and no pending recovery // opens directly into a focused terminal rooted in the project, // instead of an empty editor canvas. This runs only after session @@ -194,8 +201,7 @@ struct ContentView: View { .sheet(isPresented: $showRecoveryDialog) { RecoveryDialogView( entries: recoveryEntries, - onRecover: { recoverTabs() }, - onDiscard: { discardRecovery() } + onChoose: { resolveRecoveryOffer($0) } ) } .overlay { diff --git a/Pine/Localizable.xcstrings b/Pine/Localizable.xcstrings index 5c96abf0..37826ca5 100644 --- a/Pine/Localizable.xcstrings +++ b/Pine/Localizable.xcstrings @@ -11850,7 +11850,7 @@ } }, "recovery.discard": { - "comment": "Button to discard recovered files after a crash.", + "comment": "Destructive button that permanently deletes the recovered unsaved changes. The user's saved files on disk are never affected. Must read as deletion, not as dismissing the dialog.", "extractionState": "manual", "localizations": { "de": { @@ -11874,7 +11874,7 @@ "fr": { "stringUnit": { "state": "translated", - "value": "Ignorer" + "value": "Supprimer" } }, "ja": { @@ -11898,7 +11898,7 @@ "ru": { "stringUnit": { "state": "translated", - "value": "Отклонить" + "value": "Удалить" } }, "zh-Hans": { @@ -11909,6 +11909,126 @@ } } }, + "recovery.discardHint": { + "comment": "VoiceOver hint for the destructive Discard button in the crash recovery dialog. Must say that only the recovered unsaved changes go, never the user's files on disk.", + "extractionState": "manual", + "localizations": { + "de": { + "stringUnit": { + "state": "translated", + "value": "Löscht die wiederhergestellten ungesicherten Änderungen endgültig. Ihre gespeicherten Dateien sind nicht betroffen." + } + }, + "en": { + "stringUnit": { + "state": "translated", + "value": "Permanently deletes the recovered unsaved changes. Your saved files are not affected." + } + }, + "es": { + "stringUnit": { + "state": "translated", + "value": "Elimina definitivamente los cambios sin guardar recuperados. Los archivos guardados no se ven afectados." + } + }, + "fr": { + "stringUnit": { + "state": "translated", + "value": "Supprime définitivement les modifications non enregistrées récupérées. Vos fichiers enregistrés ne sont pas affectés." + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "復元された未保存の変更を完全に削除します。保存済みのファイルには影響しません。" + } + }, + "ko": { + "stringUnit": { + "state": "translated", + "value": "복구된 저장되지 않은 변경 사항을 영구적으로 삭제합니다. 저장된 파일은 영향을 받지 않습니다." + } + }, + "pt-BR": { + "stringUnit": { + "state": "translated", + "value": "Exclui permanentemente as alterações não salvas recuperadas. Seus arquivos salvos não são afetados." + } + }, + "ru": { + "stringUnit": { + "state": "translated", + "value": "Безвозвратно удаляет восстановленные несохранённые изменения. Сохранённые файлы не затрагиваются." + } + }, + "zh-Hans": { + "stringUnit": { + "state": "translated", + "value": "永久删除已恢复的未保存更改。已保存的文件不受影响。" + } + } + } + }, + "recovery.later": { + "comment": "Button that closes the crash recovery dialog without deleting the recovered unsaved changes. The offer returns on the next launch.", + "extractionState": "manual", + "localizations": { + "de": { + "stringUnit": { + "state": "translated", + "value": "Später" + } + }, + "en": { + "stringUnit": { + "state": "translated", + "value": "Later" + } + }, + "es": { + "stringUnit": { + "state": "translated", + "value": "Más tarde" + } + }, + "fr": { + "stringUnit": { + "state": "translated", + "value": "Plus tard" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "後で" + } + }, + "ko": { + "stringUnit": { + "state": "translated", + "value": "나중에" + } + }, + "pt-BR": { + "stringUnit": { + "state": "translated", + "value": "Mais tarde" + } + }, + "ru": { + "stringUnit": { + "state": "translated", + "value": "Позже" + } + }, + "zh-Hans": { + "stringUnit": { + "state": "translated", + "value": "稍后" + } + } + } + }, "recovery.message": { "comment": "Message shown in recovery dialog after a crash.", "extractionState": "manual", @@ -12029,6 +12149,258 @@ } } }, + "recovery.retentionNotice %lld": { + "comment": "Footnote in the crash recovery dialog. States what choosing Later means and when the retention clock starts (the crash), because it sits directly above the destructive Discard button. %lld is RecoveryManager.staleEntryRetentionDays.", + "extractionState": "manual", + "localizations": { + "de": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "one": { + "stringUnit": { + "state": "translated", + "value": "Wenn Sie „Später“ wählen, bleiben diese ungesicherten Änderungen %lld Tag nach dem Absturz verfügbar." + } + }, + "other": { + "stringUnit": { + "state": "translated", + "value": "Wenn Sie „Später“ wählen, bleiben diese ungesicherten Änderungen %lld Tage nach dem Absturz verfügbar." + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "en": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "one": { + "stringUnit": { + "state": "translated", + "value": "If you choose Later, these unsaved changes stay available for %lld day after the crash." + } + }, + "other": { + "stringUnit": { + "state": "translated", + "value": "If you choose Later, these unsaved changes stay available for %lld days after the crash." + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "es": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "one": { + "stringUnit": { + "state": "translated", + "value": "Si elige «Más tarde», estos cambios sin guardar seguirán disponibles %lld día después del fallo." + } + }, + "other": { + "stringUnit": { + "state": "translated", + "value": "Si elige «Más tarde», estos cambios sin guardar seguirán disponibles %lld días después del fallo." + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "fr": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "one": { + "stringUnit": { + "state": "translated", + "value": "Si vous choisissez « Plus tard », ces modifications non enregistrées restent disponibles %lld jour après le plantage." + } + }, + "other": { + "stringUnit": { + "state": "translated", + "value": "Si vous choisissez « Plus tard », ces modifications non enregistrées restent disponibles %lld jours après le plantage." + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "ja": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "other": { + "stringUnit": { + "state": "translated", + "value": "「後で」を選択すると、これらの未保存の変更はクラッシュから%lld日間利用できます。" + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "ko": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "other": { + "stringUnit": { + "state": "translated", + "value": "‘나중에’를 선택하면 저장되지 않은 이 변경 사항은 충돌 후 %lld일 동안 사용할 수 있습니다." + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "pt-BR": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "one": { + "stringUnit": { + "state": "translated", + "value": "Se você escolher “Mais tarde”, estas alterações não salvas continuarão disponíveis por %lld dia após a falha." + } + }, + "other": { + "stringUnit": { + "state": "translated", + "value": "Se você escolher “Mais tarde”, estas alterações não salvas continuarão disponíveis por %lld dias após a falha." + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "ru": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "few": { + "stringUnit": { + "state": "translated", + "value": "Если выбрать «Позже», эти несохранённые изменения останутся доступны в течение %lld дней после сбоя." + } + }, + "many": { + "stringUnit": { + "state": "translated", + "value": "Если выбрать «Позже», эти несохранённые изменения останутся доступны в течение %lld дней после сбоя." + } + }, + "one": { + "stringUnit": { + "state": "translated", + "value": "Если выбрать «Позже», эти несохранённые изменения останутся доступны в течение %lld дня после сбоя." + } + }, + "other": { + "stringUnit": { + "state": "translated", + "value": "Если выбрать «Позже», эти несохранённые изменения останутся доступны в течение %lld дней после сбоя." + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + }, + "zh-Hans": { + "substitutions": { + "days": { + "argNum": 1, + "formatSpecifier": "lld", + "variations": { + "plural": { + "other": { + "stringUnit": { + "state": "translated", + "value": "如果选择“稍后”，这些未保存的更改将在崩溃后保留 %lld 天。" + } + } + } + } + } + }, + "stringUnit": { + "state": "translated", + "value": "%#@days@" + } + } + } + }, "recovery.title": { "comment": "Title of the crash recovery dialog.", "extractionState": "manual", diff --git a/Pine/PineApp.swift b/Pine/PineApp.swift index 3cdba0d9..34433145 100644 --- a/Pine/PineApp.swift +++ b/Pine/PineApp.swift @@ -1431,8 +1431,12 @@ class AppDelegate: NSObject, NSApplicationDelegate, SPUUpdaterDelegate, tabSwitcherKeyController.delegate = self tabSwitcherKeyController.install() - // Clean up stale recovery files older than 7 days across all projects - RecoveryManager.cleanupAllStaleEntries(olderThan: 7) + // Clean up stale recovery files across all projects. The recovery + // sheet states this same window, so a snapshot the user keeps putting + // off has a published lifetime rather than a silent one (#1503). + RecoveryManager.cleanupAllStaleEntries( + olderThan: RecoveryManager.staleEntryRetentionDays + ) // Arm the global quick-terminal hotkey (#1113). Carbon hotkeys work // in the App Sandbox without Accessibility permission; disabled by @@ -2103,9 +2107,16 @@ class AppDelegate: NSObject, NSApplicationDelegate, SPUUpdaterDelegate, pm.cleanupEditorContext() // Shut down language servers so no orphan process survives (#1010). pm.shutdownLanguageServers() - // Clean up recovery files if all tabs are saved + // Clean up the recovery snapshots this session is answerable for: + // the ones belonging to its open tabs. Emptying the whole + // directory would also take the crash snapshots nobody has decided + // about yet — including the ones the recovery sheet is showing on + // screen at this very moment, which `hasUnsavedChanges` cannot + // see because recovery snapshots are not tabs (#1503). if !pm.hasUnsavedChanges { - pm.recoveryManager?.deleteAllRecoveryFiles() + pm.recoveryManager?.deleteSnapshotsOfOpenTabs( + pm.allTabs.map(\.id) + ) } pm.recoveryManager?.stopPeriodicSnapshots() } diff --git a/Pine/ProjectManager.swift b/Pine/ProjectManager.swift index 4aeb68ae..29094a4e 100644 --- a/Pine/ProjectManager.swift +++ b/Pine/ProjectManager.swift @@ -1534,6 +1534,37 @@ final class ProjectManager { let taskRunStore = UserTaskRunStore() /// Recovery snapshots and their lifecycle are owned by the main actor. private(set) var recoveryManager: RecoveryManager? + /// Whether the user has already answered this project's crash-recovery + /// offer — by recovering, discarding, or closing the sheet without + /// choosing (the "Later" button, which is where Escape and ⌘-. land). + /// Read through ``pendingRecoveryOffer()``, set by + /// ``markRecoveryOfferAnswered()``. + /// + /// Session-scoped and never persisted: "not now" has to mean the offer + /// comes back on the *next launch*, not that it reappears every time + /// SwiftUI re-runs the project scene's `.task` — which happens on scene + /// restoration and on closing and reopening the window. It lives here + /// rather than in `ContentView`'s `@State` precisely because that state + /// dies with the window while this project manager outlives it (#1503). + @ObservationIgnored + private var didAnswerRecoveryOffer = false + /// Whether a Recover All is running right now. + /// + /// Restoring is not instantaneous: a snapshot past the large-file + /// threshold parks the whole restore on a native sheet until the user + /// answers it. While that sheet is up the offer has been made and taken — + /// it has just not finished — and `pendingRecoveryOffer()` must not hand + /// the same crash entries to a second sheet, which would migrate them a + /// second time, write a snapshot under a runtime ID no window owns, and + /// leave the parked restore to resume against a `TabManager` that is no + /// longer attached to anything (#1503). + /// + /// Deliberately *not* ``markRecoveryOfferAnswered()``: nobody has answered + /// anything yet, and the two claims come apart when the restore hands + /// entries back. Read through ``pendingRecoveryOffer()``, set and cleared + /// by ``beginRecoveryRestore()`` / ``endRecoveryRestore()``. + @ObservationIgnored + private(set) var isRestoringRecoveryOffer = false /// Project-scoped session persistence. Production uses `.standard`; /// process-level lifecycle tests inject a namespaced suite so launching a /// second real Pine process cannot read or overwrite developer state. @@ -1553,6 +1584,75 @@ final class ProjectManager { @ObservationIgnored private var agentTaskFilesystemIdentity: AgentTaskProjectIdentity? + /// The crash-recovery entries still worth putting in front of the user. + /// + /// Empty once ``markRecoveryOfferAnswered()`` has been called, even though + /// the snapshots are still on disk: "Later" means the offer waits for the + /// next launch, not that it reappears the moment the scene re-runs its + /// `.task` (#1503). Empty as well while a Recover All is still running — + /// see ``isRestoringRecoveryOffer``. + /// + /// Snapshots belonging to tabs that are open right now are filtered out, + /// and no crash is needed to produce one. Open a project, edit a file, + /// close the window: the project is held alive because it has dirty tabs, + /// and `suspendEditorServices` deliberately snapshots them. Reopen it and + /// `pendingRecoveryEntries()` — which only knows "there is a JSON file + /// named after a UUID" — hands back the buffers the user is looking at. + /// Offering those is worse than noise: Discard would delete the crash + /// protection of live unsaved work, and Recover All would clone the tab + /// and then have `migrateRecoverySnapshot` unlink the original's live + /// snapshot. A snapshot whose ID is an open tab's ID is by definition not + /// a crash leftover, so it is not an offer. + /// + /// "Open right now" is `allTabs`, i.e. every `TabManager` still in + /// `paneManager.tabManagers`. A manager detached by `removePane` is not in + /// there, so its tabs' snapshots stay offerable — which is the safe + /// direction (a closed pane's unsaved buffer is exactly what the sheet is + /// for) but worth naming, because it means the filter is "attached to a + /// pane", not "alive somewhere". + /// + /// `async` because the listing is not: reading and decoding every snapshot + /// in the directory is unbounded file I/O — a snapshot is a whole unsaved + /// buffer — and this runs from the project scene's `.task` before the + /// window draws. Only the read leaves the main actor + /// (``RecoveryManager/pendingRecoveryEntriesOffMainActor()``); the two + /// suppression flags and the live-tab filter are main-actor state and stay + /// here. Both flags are re-read *after* the suspension as well as before + /// it: the window they are protecting is exactly the one the `await` opens, + /// and a Recover All or an answered offer that lands during the read must + /// not be overwritten by a listing taken before it. + func pendingRecoveryOffer() async -> [(UUID, RecoveryEntry)] { + guard !didAnswerRecoveryOffer, !isRestoringRecoveryOffer, + let recoveryManager else { + return [] + } + let entries = await recoveryManager.pendingRecoveryEntriesOffMainActor() + guard !didAnswerRecoveryOffer, !isRestoringRecoveryOffer else { + return [] + } + let live = Set(allTabs.map(\.id)) + return entries.filter { !live.contains($0.0) } + } + + /// Records that the user has answered this project's recovery offer, by + /// recovering, discarding, or closing the sheet without choosing. + func markRecoveryOfferAnswered() { + didAnswerRecoveryOffer = true + } + + /// Marks a Recover All as being in flight. Paired with + /// ``endRecoveryRestore()`` from a `defer`, so an early return or a thrown + /// error cannot leave the offer suppressed. See + /// ``isRestoringRecoveryOffer``. + func beginRecoveryRestore() { + isRestoringRecoveryOffer = true + } + + /// Marks the in-flight Recover All as finished, whatever it finished with. + func endRecoveryRestore() { + isRestoringRecoveryOffer = false + } + #if DEBUG func removeRecoveryManagerForTesting() { recoveryManager?.cancelScheduledSnapshot() diff --git a/Pine/RecoveryDialogView.swift b/Pine/RecoveryDialogView.swift index 6e74cbce..d9524751 100644 --- a/Pine/RecoveryDialogView.swift +++ b/Pine/RecoveryDialogView.swift @@ -5,11 +5,270 @@ import SwiftUI +/// One choice offered by the crash-recovery sheet, paired with the keyboard +/// equivalents it is allowed to answer to and the snapshots it is allowed to +/// unlink. +/// +/// The shortcut policy *and* the deletion policy live on this type instead of +/// being spelled out at each `Button` and at the call site that applies the +/// answer, because this sheet is the one SwiftUI surface in Pine where a +/// single keystroke can unlink unsaved work. Escape used to be bound to +/// ``discard``, so dismissing the sheet the way macOS teaches you to deleted +/// every recovered buffer — no confirmation, no undo (#1503). Keeping "which +/// key runs which choice" and "what that choice deletes" on the same value +/// makes both checked invariants instead of layout details, and it puts them +/// where a test can enumerate them: the resolver in `ContentView+Helpers` +/// lives in a file the coverage gate excludes and no unit test loads. +/// +/// This is the SwiftUI half of a policy `AlertTemplate` already implements for +/// every `NSAlert` in the app: exactly one default button, at most one +/// cancellation target, cancellation reachable by both Escape and ⌘-., and the +/// destructive button reachable by neither. Change one of the two and check +/// the other — `AlertTemplate.buttonRoles` carries the matching note. +enum RecoveryDialogChoice: String, CaseIterable, Sendable { + /// Deletes the recovery snapshots. Irreversible. + case discard + /// Closes the sheet and leaves the snapshots on disk for the next launch. + case later + /// Restores every snapshot into the active editor pane. + case recoverAll + + /// Whether choosing this destroys recovered content. + var isDestructive: Bool { self == .discard } + + /// Every keyboard equivalent bound to this choice, in installation order: + /// the first rides the visible button, any further one rides an invisible + /// proxy, because SwiftUI allows a control only one key equivalent. + /// + /// A destructive choice must carry none. Escape and Return are reflexes, + /// and a reflex must not be able to delete unsaved work, so Escape (and + /// ⌘-., macOS's other cancellation gesture, which every Pine alert honours + /// via `AlertTemplate`) resolves to ``later`` — which leaves the snapshots + /// on disk and brings the offer back on the next launch. + /// + /// The guard is what carries the invariant: a case added to this enum + /// inherits "destructive implies unreachable by keyboard" from the type + /// rather than from whoever remembers to write `[]` in the right branch. + /// The `discard` arm below is unreachable while `discard` is the only + /// destructive case, and is kept solely so the switch stays exhaustive — + /// a `default:` here would silently hand a *new* case no shortcuts at all. + /// The two therefore agree by construction and no test can tell them + /// apart; do not read a green suite as evidence that either one alone is + /// doing the work. + var keyboardShortcuts: [KeyboardShortcut] { + guard !isDestructive else { return [] } + switch self { + case .discard: + return [] + case .later: + return [.cancelAction, KeyboardShortcut(".", modifiers: .command)] + case .recoverAll: + return [.defaultAction] + } + } + + /// Whether this is the sheet's default action — the one Return runs, and + /// by macOS convention the one that sits last in the button row. + var isDefaultAction: Bool { + keyboardShortcuts.contains(.defaultAction) + } + + /// The snapshot IDs this choice is allowed to unlink, out of the ones the + /// sheet is showing. + /// + /// The deletion decision belongs here, on the same value that already + /// carries ``isDestructive``, the button's role and its (empty) shortcut + /// list, so the resolver can call it unconditionally. An `if` at the call + /// site is a place where a plausible "the function already handled the + /// other case" simplification silently restores #1503, and the call site + /// is in `ContentView+Helpers.swift` — excluded from the coverage gate and + /// not loaded by any unit test. Here it is enumerable: every case's answer + /// is asserted in `RecoveryDialogEscapeSafetyTests`. + /// + /// Generic in the payload so a test can drive it with the same tuple shape + /// the sheet uses without constructing `RecoveryEntry` values. + func snapshotsToDelete( + from entries: [(UUID, Payload)] + ) -> [UUID] { + isDestructive ? entries.map { $0.0 } : [] + } + + /// Localized button title. + var title: LocalizedStringKey { + switch self { + case .discard: Strings.recoveryDiscard + case .later: Strings.recoveryLater + case .recoverAll: Strings.recoveryRecoverAll + } + } + + /// Stable identifier so UI tests can address the button. + var accessibilityIdentifier: String { + switch self { + case .discard: AccessibilityID.recoveryDiscardButton + case .later: AccessibilityID.recoveryLaterButton + case .recoverAll: AccessibilityID.recoveryRecoverAllButton + } + } + + /// VoiceOver hint, `nil` for the choices that need none. + /// + /// SwiftUI's `ButtonRole.destructive` only recolors the button; it does not + /// set AppKit's `hasDestructiveAction`, which is the trait `AlertTemplate` + /// sets by hand so a screen reader announces intent. Until SwiftUI exposes + /// that trait, this hint is what tells a VoiceOver user that Discard is + /// not an ordinary button. + /// + /// Optional rather than an empty key: `LocalizedStringKey("")` is a real + /// lookup for a key the catalog does not contain, and the hint it would + /// attach is an empty announcement rather than no announcement. + var accessibilityHint: LocalizedStringKey? { + isDestructive ? Strings.recoveryDiscardHint : nil + } +} + +/// The recovery sheet's footer. +/// +/// Laid out like the standard macOS "Don't Save / Cancel / Save" alert: the +/// irreversible choice sits apart on the leading edge, the two safe choices on +/// the trailing edge with the default action last. +/// +/// Split out of ``RecoveryDialogView`` so the row can be measured on its own — +/// three buttons is the widest thing in the sheet in the longer locales. +struct RecoveryDialogFooter: View { + /// Reports the choice the user made. One callback keyed by the choice, not + /// three interchangeable `() -> Void` parameters: every button gets its + /// role, its keystrokes, its title *and* its outcome from the same enum + /// value, so no edit can leave Escape pointing at a button that reports + /// something else (#1503). + let onChoose: (RecoveryDialogChoice) -> Void + + /// Every choice, placed by what it is rather than by name, so a case added + /// to ``RecoveryDialogChoice`` appears on screen instead of existing only + /// in the type. Destructive choices lead, the default action goes last. + private static let leading = RecoveryDialogChoice.allCases + .filter(\.isDestructive) + private static let trailing = RecoveryDialogChoice.allCases + .filter { !$0.isDestructive && !$0.isDefaultAction } + + RecoveryDialogChoice.allCases + .filter { !$0.isDestructive && $0.isDefaultAction } + + /// The buttons in the order they are laid out, derived from the two lists + /// the body iterates so it cannot describe a row that is not drawn. + /// Exposed for `RecoveryDialogEscapeSafetyTests`, which is what turns + /// "every case reaches the screen" into something a compiler-silent + /// addition to the enum cannot break quietly. + static let displayOrder = leading + trailing + + var body: some View { + HStack(spacing: 12) { + ForEach(Self.leading, id: \.self) { button(for: $0) } + Spacer(minLength: 12) + ForEach(Self.trailing, id: \.self) { button(for: $0) } + } + } + + @ViewBuilder + private func button(for choice: RecoveryDialogChoice) -> some View { + let control = Button(role: choice.isDestructive ? .destructive : nil) { + onChoose(choice) + } label: { + Text(choice.title) + } + .keyboardShortcut(choice.keyboardShortcuts.first) + .accessibilityIdentifier(choice.accessibilityIdentifier) + .background { + shortcutProxies(for: choice) + } + + if let hint = choice.accessibilityHint { + control.accessibilityHint(hint) + } else { + control + } + } + + /// Invisible controls carrying the choice's second and further key + /// equivalents — the SwiftUI form of `AlertTemplate.installShortcutProxy`, + /// which does the same for ⌘-. on every `NSAlert` in Pine. They are hidden + /// from accessibility so VoiceOver and XCUITest still see three buttons, + /// and they report the same choice as the visible button. + /// + /// `.focusable(false)` as well as `.accessibilityHidden(true)`: hiding a + /// control from the accessibility tree is not documented to take it out of + /// SwiftUI's focus ring, and with Full Keyboard Access on, a Tab stop at a + /// 0×0 control draws its focus ring nowhere and strands the traversal. + private func shortcutProxies( + for choice: RecoveryDialogChoice + ) -> some View { + ForEach( + Array(choice.keyboardShortcuts.dropFirst().enumerated()), + id: \.offset + ) { _, shortcut in + Button { onChoose(choice) } label: { Color.clear } + .buttonStyle(.plain) + .keyboardShortcut(shortcut) + } + .frame(width: 0, height: 0) + .accessibilityHidden(true) + .focusable(false) + } +} + +/// One recovered buffer in the sheet's list. +/// +/// Its own view so the width it asks for can be measured on its own. Inside +/// the `List` it cannot be: a list is a scroll view and absorbs whatever its +/// rows want, so a row that asks for a thousand points is clipped silently +/// rather than showing up in the sheet's fitting size. That is the failure +/// this row is shaped to avoid, and a test measuring the sheet would never see +/// it happen. +struct RecoveryEntryRow: View { + let entry: RecoveryEntry + + var body: some View { + HStack { + Image(systemName: "doc.text") + VStack(alignment: .leading) { + // Generated bundles, downloads and dated exports routinely + // produce names well past a hundred characters, and a `Text` + // has no natural width to stop at. Middle truncation keeps + // both ends — the extension is what identifies the file, and + // the beginning is what distinguishes two exports of the same + // thing. + Text(Self.fileName(from: entry)) + .font(.body) + .lineLimit(1) + .truncationMode(.middle) + Text(entry.timestamp, style: .relative) + .font(.caption) + .foregroundStyle(.secondary) + .lineLimit(1) + } + Spacer(minLength: 0) + } + .frame(maxWidth: RecoveryDialogView.contentWidth, alignment: .leading) + } + + static func fileName(from entry: RecoveryEntry) -> String { + if entry.originalPath.isEmpty { + return entry.untitledName ?? Strings.recoveryUntitled + } + return (entry.originalPath as NSString).lastPathComponent + } +} + /// Shows a dialog listing recovered unsaved files after a crash. struct RecoveryDialogView: View { let entries: [(UUID, RecoveryEntry)] - let onRecover: () -> Void - let onDiscard: () -> Void + /// Reports which of the three choices the user made. See + /// ``RecoveryDialogFooter/onChoose`` for why this is one callback. + let onChoose: (RecoveryDialogChoice) -> Void + + /// Width of the text column, and the sheet's resting width once the 24pt + /// padding on each edge is added back. Shared with ``RecoveryEntryRow``, + /// which caps itself to the same column. + static let contentWidth: CGFloat = 352 var body: some View { VStack(spacing: 16) { @@ -19,54 +278,54 @@ struct RecoveryDialogView: View { Text(Strings.recoveryTitle) .font(.headline) + .multilineTextAlignment(.center) + .frame(maxWidth: Self.contentWidth) Text(Strings.recoveryMessage) .font(.subheadline) .foregroundStyle(.secondary) .multilineTextAlignment(.center) + .frame(maxWidth: Self.contentWidth) List { ForEach(entries, id: \.0) { _, entry in - HStack { - Image(systemName: "doc.text") - VStack(alignment: .leading) { - Text(fileName(from: entry)) - .font(.body) - Text(entry.timestamp, style: .relative) - .font(.caption) - .foregroundStyle(.secondary) - } - } + RecoveryEntryRow(entry: entry) } } .frame(minHeight: 100, maxHeight: 200) - HStack(spacing: 12) { - Button(role: .destructive) { - onDiscard() - } label: { - Text(Strings.recoveryDiscard) - .frame(maxWidth: .infinity) - } - .keyboardShortcut(.cancelAction) + // Closing this sheet without choosing is safe but not unlimited: + // the launch-time sweep collects undecided snapshots eventually. + // Saying so is the honest half of making Escape mean "later" + // instead of "delete" (#1503). The wording names the choice it + // describes and the moment the clock starts, because it sits + // directly above a row whose leading button is Discard: read as a + // bare "nothing here is final for a week" it would make the one + // irreversible click on this sheet feel cheap, and Discard does + // not honour the window at all. + Text(Strings.recoveryRetentionNotice( + days: RecoveryManager.staleEntryRetentionDays + )) + .font(.caption) + .foregroundStyle(.secondary) + .multilineTextAlignment(.center) + .frame(maxWidth: Self.contentWidth) - Button { - onRecover() - } label: { - Text(Strings.recoveryRecoverAll) - .frame(maxWidth: .infinity) - } - .keyboardShortcut(.defaultAction) - } + RecoveryDialogFooter(onChoose: onChoose) } .padding(24) - .frame(width: 400) - } - - private func fileName(from entry: RecoveryEntry) -> String { - if entry.originalPath.isEmpty { - return entry.untitledName ?? Strings.recoveryUntitled - } - return (entry.originalPath as NSString).lastPathComponent + // `minWidth`, not `width`: the footer is the widest row here, and in + // German it already asks for 349 of the 352pt a fixed 400pt sheet can + // give it. A fixed width turns any future growth — a longer + // translation, a change in AppKit's button metrics — into a truncated + // button label. Growing the sheet instead is the recoverable failure. + // The two text rows and the list rows are capped above so they cannot + // drive that growth themselves; without the cap the German message + // alone would stretch the sheet to 634pt rather than wrapping. + .frame(minWidth: 2 * 24 + Self.contentWidth) + // `children: .contain` so the identifier names this container and does + // not flatten the buttons inside it into one element. + .accessibilityElement(children: .contain) + .accessibilityIdentifier(AccessibilityID.recoverySheet) } } diff --git a/Pine/RecoveryEntry.swift b/Pine/RecoveryEntry.swift index a48c3d36..c5710fab 100644 --- a/Pine/RecoveryEntry.swift +++ b/Pine/RecoveryEntry.swift @@ -6,7 +6,15 @@ import Foundation /// Represents a snapshot of unsaved editor content for crash recovery. -struct RecoveryEntry: Codable, Sendable { +/// +/// `nonisolated` because the module compiles with `-default-isolation=MainActor` +/// and `InferIsolatedConformances`: without it the `Decodable` conformance is +/// main-actor-isolated and cannot be used from +/// ``RecoveryManager/readEntries(in:)``, which decodes the snapshot directory +/// off the main actor so a launch does not block on it (#1503). This is a pure +/// value type with no mutable state, so there is nothing for the isolation to +/// protect. +nonisolated struct RecoveryEntry: Codable, Sendable { static let currentSchemaVersion = 1 /// Missing for snapshots written before persistence versioning. let schemaVersion: Int? @@ -27,8 +35,29 @@ struct RecoveryEntry: Codable, Sendable { } var hasSupportedSchema: Bool { + Self.isSupportedSchema(schemaVersion) + } + + /// Whether a bare schema stamp names a shape this build can read. + /// + /// Split off the instance property so the stale-entry sweep can answer the + /// question without a full `init(from:)`. A snapshot written by a newer + /// build may have renamed a field or added a required one, in which case + /// decoding a whole ``RecoveryEntry`` throws before the version is ever + /// read — and the longer retention horizon that exists precisely for that + /// file would never be applied to it (#1503). + static func isSupportedSchema(_ schemaVersion: Int?) -> Bool { guard let schemaVersion else { return true } - return (0...Self.currentSchemaVersion).contains(schemaVersion) + return (0...currentSchemaVersion).contains(schemaVersion) + } + + /// Just the schema stamp, decoded from any JSON object that carries one. + /// + /// Every other field is ignored, so this keeps working across a rename, a + /// type change, or a newly required field — the schema changes this + /// version stamp exists to survive. + struct SchemaProbe: Decodable { + let schemaVersion: Int? } init( diff --git a/Pine/RecoveryManager.swift b/Pine/RecoveryManager.swift index 11218da9..a6fb552d 100644 --- a/Pine/RecoveryManager.swift +++ b/Pine/RecoveryManager.swift @@ -15,7 +15,9 @@ import os @MainActor final class RecoveryManager { - private static let logger = Logger.app + /// `nonisolated` so the off-main-actor listing in ``readEntries(in:)`` can + /// report what it skipped. + nonisolated private static let logger = Logger.app /// Root recovery directory under Application Support. nonisolated static var rootDirectory: URL { @@ -36,6 +38,44 @@ final class RecoveryManager { /// Debounce delay for edit-triggered snapshots. nonisolated static let debounceDelay: TimeInterval = 5 + /// How long a snapshot nobody has decided about survives before the + /// launch-time sweep collects it. + /// + /// The recovery sheet prints this number, so closing the sheet without + /// choosing is a bounded promise the user can read rather than a silent + /// expiry on the eighth day (#1503). Both the sweep in + /// `AppDelegate.applicationDidFinishLaunching` and the sheet's footnote + /// read it from here so the two cannot drift. + nonisolated static let staleEntryRetentionDays = 7 + + /// Multiple of ``staleEntryRetentionDays`` granted to a snapshot written + /// by a schema this build cannot read. + /// + /// The normal horizon bounds files the user was *shown* and chose to leave + /// alone. A snapshot from a newer build — a beta wrote `schemaVersion: 2` + /// and they went back to a release — has had no such decision made about + /// it: either it decoded and was offered on a build that will be replaced + /// again by the newer one, or it did not decode and could not be shown at + /// all. Long enough to survive a downgrade and a return, finite so a + /// permanently orphaned file still leaves and its project subdirectory can + /// be collected. + /// + /// This buys time; it does not decide visibility. + /// ``readEntries(in:)`` offers every snapshot that decodes, whatever its + /// stamp says, precisely so no path can delete a buffer this build could + /// read but refused to show (#1503). + nonisolated static let unsupportedSchemaRetentionMultiplier = 4 + + /// How far ahead of the sweep's own clock a date may sit before it is + /// treated as a broken clock rather than as jitter. + /// + /// NTP steps, a network volume with its own idea of the time, and a + /// snapshot written in the same second the sweep runs all produce dates a + /// hair in the future. Re-anchoring on a zero tolerance would rewrite the + /// modification date of a file the user is actively editing, and that date + /// is the only record of when an undecodable snapshot was written. + nonisolated static let futureDateTolerance: TimeInterval = 60 + private let recoveryDirectory: URL private let faultInjector: PersistenceFaultInjector private var periodicTimer: Timer? @@ -85,35 +125,45 @@ final class RecoveryManager { deleteSupersededRecoveryFiles(for: tabID) } - /// Removes only the explicitly displayed recovery IDs. + /// Removes exactly the recovery IDs it is given and nothing linked to + /// them. /// /// Used by the recovery sheet's Discard action so successfully migrated /// runtime snapshots in the same project directory remain intact. - func deleteRecoveryFiles(for tabIDs: [UUID]) { - for tabID in Set(tabIDs) where removeRecoveryFileOnly(for: tabID) { - forgetSupersededReference(to: tabID) + /// + /// Named apart from ``deleteSnapshotsOfOpenTabs(_:)`` on purpose. The two + /// used to differ by one argument label while differing by their entire + /// blast radius: this one takes crash IDs the user just looked at, that + /// one takes live tab IDs and also collects whatever crash file each tab + /// superseded. Passing one's argument to the other is a silent data-loss + /// bug, so there is no shared base name to autocomplete into. + func deleteSnapshots(withRecoveryIDs recoveryIDs: [UUID]) { + for recoveryID in Set(recoveryIDs) + where removeRecoveryFileOnly(for: recoveryID) { + forgetSupersededReference(to: recoveryID) } } - /// Removes all recovery files for this project (e.g., on clean quit). - func deleteAllRecoveryFiles() { - let files: [URL] - do { - files = try FileManager.default.contentsOfDirectory( - at: recoveryDirectory, - includingPropertiesForKeys: nil - ) - } catch { - Self.logger.warning("Cannot list recovery directory: \(error)") - return - } - - for file in files where file.pathExtension == "json" { - do { - try FileManager.default.removeItem(at: file) - } catch { - Self.logger.error("Failed to delete recovery file \(file.lastPathComponent): \(error)") - } + /// Removes the snapshots this session is answerable for: one per open tab, + /// plus any superseded crash-ID file still linked to it. Used by the + /// clean-quit sweep in `AppDelegate.applicationWillTerminate`. + /// + /// Deliberately scoped instead of emptying the directory. Everything in + /// there that no open tab accounts for is crash payload nobody has decided + /// about yet — the entries the recovery sheet is showing right now, the + /// ones a user closed the sheet on without choosing, and the ones + /// ``restorePendingEntries(_:in:context:)`` handed back because they could + /// not be restored. A directory-wide sweep deletes exactly the work the + /// recovery sheet exists to protect, and `ProjectManager.hasUnsavedChanges` + /// cannot veto it because recovery snapshots are not tabs (#1503). + /// + /// The one thing this leaves behind is a snapshot for a tab closed earlier + /// in the session whose unlink failed. That is collected by + /// ``cleanupAllStaleEntries(olderThan:)`` — offering a stale file once is + /// recoverable, deleting an undecided one is not. + func deleteSnapshotsOfOpenTabs(_ tabIDs: [UUID]) { + for tabID in Set(tabIDs) { + deleteRecoveryFile(for: tabID) } } @@ -121,15 +171,75 @@ final class RecoveryManager { /// Returns all pending recovery entries as (tabID, entry) pairs. /// Corrupted or non-JSON files are logged and skipped. + /// + /// Synchronous, and it reads and decodes every snapshot in the directory. + /// The path a launching window takes is + /// ``pendingRecoveryEntriesOffMainActor()`` — see that method for why the + /// main actor must not do this work. func pendingRecoveryEntries() -> [(UUID, RecoveryEntry)] { + Self.readEntries(in: recoveryDirectory) + } + + /// The same listing, performed off the main actor. + /// + /// `pendingRecoveryEntries()` opens and fully decodes every snapshot in + /// the directory, and a snapshot is a whole unsaved buffer — nothing + /// bounds its size, because ``snapshotDirtyTabs(_:)`` selects on + /// `isDirty && kind == .text` and the 1 MB large-file threshold is not + /// applied to it. Before this branch the directory was emptied on every + /// clean quit, so the cost was paid at most once, on the launch after a + /// crash. Now snapshots are held for ``staleEntryRetentionDays`` after a + /// "Later", and the discovery runs from a scene `.task` that SwiftUI + /// re-runs on scene restoration and on closing and reopening the window: + /// three 40 MB buffers left for later would otherwise block the main + /// thread on ~120 MB of reading and decoding before the window draws, + /// every launch and every reopen, for a week (AGENTS.md: never block the + /// main thread with file I/O). + /// + /// Only the listing moves. The decision about what is worth offering stays + /// on the main actor in ``ProjectManager/pendingRecoveryOffer()``, which + /// needs the live tab IDs to filter against. + func pendingRecoveryEntriesOffMainActor() async -> [(UUID, RecoveryEntry)] { + let directory = recoveryDirectory + return await Task.detached(priority: .userInitiated) { + Self.readEntries(in: directory) + }.value + } + + /// Reads and decodes every snapshot in `directory`. + /// + /// `nonisolated` and taking the directory as a parameter so it can run on + /// a background executor. It touches no instance state — the recovery + /// directory is the whole input. + /// + /// **Every entry that decodes is returned, whatever its schema stamp + /// says.** An unsupported stamp used to be a `continue` here, and that + /// combination is a trap: a purely additive `schemaVersion: 2` written by + /// a beta decodes into today's `RecoveryEntry` with real `content`, a real + /// `originalPath` and a real `timestamp`, and the release build the user + /// went back to would refuse to show it — while + /// ``cleanupStaleEntries(olderThan:)`` deleted it after + /// ``unsupportedSchemaRetentionMultiplier`` × the window. A build that can + /// read a buffer must not both hide it and eventually destroy it; the + /// whole point of this branch is that nothing deletes work the user was + /// never given the chance to decide about (#1503). ``RecoveryEntry/hasSupportedSchema`` + /// stays as a signal — it is logged here, and it still buys the longer + /// retention horizon in the sweep — but it no longer suppresses the offer. + /// + /// What cannot be *decoded* is still not offered, and that is not the same + /// thing: a file this build cannot turn into content is not content it + /// could have shown. + nonisolated static func readEntries( + in directory: URL + ) -> [(UUID, RecoveryEntry)] { let files: [URL] do { files = try FileManager.default.contentsOfDirectory( - at: recoveryDirectory, + at: directory, includingPropertiesForKeys: nil ) } catch { - Self.logger.warning("Cannot list recovery directory: \(error)") + logger.warning("Cannot list recovery directory: \(error)") return [] } @@ -143,15 +253,15 @@ final class RecoveryManager { do { let data = try Data(contentsOf: file) let entry = try decoder.decode(RecoveryEntry.self, from: data) - guard entry.hasSupportedSchema else { - Self.logger.error( - "Refusing unsupported recovery schema in \(name)" + if !entry.hasSupportedSchema { + let stamp = entry.schemaVersion ?? -1 + logger.warning( + "Recovery entry \(name) carries schema \(stamp), which this build does not know; offering it anyway because it decoded" ) - continue } results.append((uuid, entry)) } catch { - Self.logger.error("Failed to read recovery entry \(name): \(error)") + logger.error("Failed to read recovery entry \(name): \(error)") } } return results @@ -310,13 +420,26 @@ final class RecoveryManager { // MARK: - Stale cleanup /// Removes recovery files with timestamps older than the given number of days - /// across *all* project subdirectories. + /// across *all* project subdirectories, then collects the subdirectories + /// that hold nothing worth keeping. + /// + /// Collecting the per-project subdirectory is load-bearing since #1503: + /// the clean-quit sweep no longer empties a project's directory, so a + /// directory is now expected to outlive the project's session and the only + /// thing that ever removes it is this pass. static func cleanupAllStaleEntries(olderThan days: Int) { + cleanupAllStaleEntries(in: rootDirectory, olderThan: days) + } + + /// The body of ``cleanupAllStaleEntries(olderThan:)``, against an explicit + /// root so it can be exercised without writing into the real Application + /// Support directory. + static func cleanupAllStaleEntries(in root: URL, olderThan days: Int) { let fm = FileManager.default let subdirs: [URL] do { subdirs = try fm.contentsOfDirectory( - at: rootDirectory, + at: root, includingPropertiesForKeys: [.isDirectoryKey] ) } catch { @@ -325,18 +448,38 @@ final class RecoveryManager { } for subdir in subdirs { + // `.isDirectoryKey` was prefetched here and then never consulted, + // so a stray regular file at the root — `.DS_Store` above all — + // was handed to a `RecoveryManager` as if it were a project + // directory, and every listing of it failed and logged. + let isDirectory = (try? subdir.resourceValues( + forKeys: [.isDirectoryKey] + ).isDirectory) ?? false + guard isDirectory else { continue } + let manager = RecoveryManager(recoveryDirectory: subdir) manager.cleanupStaleEntries(olderThan: days) - // Remove empty subdirectories do { let remaining = try fm.contentsOfDirectory(atPath: subdir.path) - if remaining.isEmpty { - do { - try fm.removeItem(at: subdir) - } catch { - logger.error("Failed to remove empty recovery subdir: \(error)") - } + // Not `remaining.isEmpty`. Snapshots are written with + // `Data.write(options: .atomic)`, which stages a hidden + // temporary beside the destination; a crash between the + // staging and the rename leaves that temporary behind forever. + // It is not a `.json`, so the sweep above cannot see it — but + // an emptiness test does, and one orphaned temporary used to + // pin its project's directory open for the life of the + // machine. `.DS_Store` did the same. Anything hidden here is + // leftovers by construction: this directory holds nothing but + // `.json` files that Pine itself writes. + guard remaining.allSatisfy({ $0.hasPrefix(".") }) else { + continue + } + do { + // Recursive, because the leftovers go with it. + try fm.removeItem(at: subdir) + } catch { + logger.error("Failed to remove empty recovery subdir: \(error)") } } catch { logger.warning("Cannot list recovery subdir \(subdir.lastPathComponent): \(error)") @@ -344,9 +487,76 @@ final class RecoveryManager { } } - /// Removes recovery files with timestamps older than the given number of days. + /// Removes recovery files older than the given number of days. + /// + /// **Stats before it reads.** This runs on the main actor from + /// `applicationDidFinishLaunching`, and since #1503 the directory + /// deliberately *keeps* crash payload nobody has decided about instead of + /// being emptied on every clean quit — so the cost of opening every file + /// to read one `Date` now scales with how much unrecovered work the user + /// is holding, and a snapshot carries a whole unsaved buffer (Pine's own + /// large-file threshold is 1MB, and nothing caps a snapshot at it). + /// `AGENTS.md` forbids blocking the main thread with file I/O. A file + /// whose modification date is inside the window can only be kept, so one + /// `stat` settles it, and in steady state — where everything on disk is + /// younger than the window — nothing is read at all. The date is read + /// through `includingPropertiesForKeys:` so the values are already + /// prefetched by the directory enumeration. + /// + /// **Which date decides.** The entry's own `timestamp` is the authority + /// whenever it is usable. The filesystem date only stands in for a file + /// that has no usable one, and it is never a second opinion that can + /// overrule a timestamp *into* a deletion: taking the earlier of the two + /// would let anything that moves an mtime backwards — a sync client, a + /// restore tool, `touch -t`, a volume with coarser timestamps — delete + /// undecided work on the filesystem's word against the file's own. + /// + /// **Three kinds of file used to be immortal here.** Each was `continue`d + /// past without ever being reconsidered, so its project's subdirectory + /// could never be collected by ``cleanupAllStaleEntries(olderThan:)`` long + /// after the project itself was gone, and it logged an error on every + /// single launch: + /// + /// - **Undecodable.** Truncated by the crash that was happening while it + /// was being written, or corrupted since. It ages out on the + /// filesystem's own evidence at the normal horizon. This does not weaken + /// "never delete a snapshot nobody has decided about": a file that + /// cannot be decoded is not content that could have been offered, so + /// there was never a decision to make. *Unreadable* is a different + /// thing and is not treated as evidence of age at all — see below. + /// - **A schema this build does not support.** This one *is* the user's + /// work, so it gets ``unsupportedSchemaRetentionMultiplier`` times the + /// horizon rather than the same one — see that constant. The version + /// stamp is probed on its own, before the full decode, because a schema + /// change is exactly what makes `RecoveryEntry.init(from:)` throw before + /// the version is ever read; deciding the horizon from a successful + /// decode would grant the longer window only to purely additive + /// schemas, which are the ones that least need it. If it decodes, it is + /// also *offered* — ``readEntries(in:)`` does not consult the stamp — so + /// the longer horizon covers a file the user has seen and put off, not + /// one being destroyed behind their back. + /// - **Dated in the future.** A clock moved forward, a restored backup, a + /// VM snapshot: `timestamp < cutoff` can never come true, so the file + /// outlived every sweep. If any date it carries is usable it is judged + /// by that one; if every date is in the future by more than + /// ``futureDateTolerance`` the file is restamped to now, which gives it + /// a real anchor to age from — this sweep's first sighting — instead of + /// waiting for the calendar to catch up with a wrong clock. + /// + /// A file that cannot be *read* is kept, whatever its dates say. Launch is + /// when every language server, file-system watcher and terminal in the app + /// is starting at once, so `EIO` and `EMFILE` are live possibilities here, + /// and a file the process could not open has not told anybody how old it + /// is. func cleanupStaleEntries(olderThan days: Int) { - let cutoff = Date().addingTimeInterval(-Double(days) * 24 * 3600) + let now = Date() + let day = 24.0 * 3600 + let cutoff = now.addingTimeInterval(-Double(days) * day) + let unsupportedCutoff = now.addingTimeInterval( + -Double(days * Self.unsupportedSchemaRetentionMultiplier) * day + ) + // The latest moment a date may claim and still be believed. + let believable = now.addingTimeInterval(Self.futureDateTolerance) let decoder = JSONDecoder() decoder.dateDecodingStrategy = .iso8601 @@ -354,7 +564,7 @@ final class RecoveryManager { do { files = try FileManager.default.contentsOfDirectory( at: recoveryDirectory, - includingPropertiesForKeys: nil + includingPropertiesForKeys: [.contentModificationDateKey] ) } catch { Self.logger.warning("Cannot list recovery directory for cleanup: \(error)") @@ -362,25 +572,99 @@ final class RecoveryManager { } for file in files where file.pathExtension == "json" { + let modified = Self.modificationDate(of: file) + // Inside the window by the filesystem's reckoning: keep it without + // reading it. A date well in the future is not "inside the + // window", it is a date to distrust, so it does not take this path. + if let modified, modified >= cutoff, modified <= believable { + continue + } + + guard let data = try? Data(contentsOf: file) else { + Self.logger.error( + "Cannot read recovery file during cleanup, keeping it: \(file.lastPathComponent)" + ) + continue + } + + // Probed separately and first: see the docstring. + let probe = try? decoder.decode( + RecoveryEntry.SchemaProbe.self, + from: data + ) + let horizon = RecoveryEntry.isSupportedSchema(probe?.schemaVersion) + ? cutoff + : unsupportedCutoff + + var recorded: Date? do { - let data = try Data(contentsOf: file) - let entry = try decoder.decode(RecoveryEntry.self, from: data) - guard entry.hasSupportedSchema else { - Self.logger.error( - "Refusing unsupported recovery schema during cleanup" - ) - continue - } - if entry.timestamp < cutoff { - do { - try FileManager.default.removeItem(at: file) - } catch { - Self.logger.error("Failed to remove stale recovery file \(file.lastPathComponent): \(error)") - } - } + recorded = try decoder + .decode(RecoveryEntry.self, from: data) + .timestamp } catch { - Self.logger.error("Failed to read recovery file for cleanup \(file.lastPathComponent): \(error)") + Self.logger.error( + "Undecodable recovery file, ageing it out by its modification date \(file.lastPathComponent): \(error)" + ) + } + + let age: Date + if let recorded, recorded <= believable { + // The file's own account of itself, believed over the + // filesystem's in both directions. + age = recorded + } else if let modified, modified <= believable { + // No usable timestamp — either the entry has none or the one + // it has is from a broken clock — so the filesystem stands in. + age = modified + } else if recorded != nil || modified != nil { + Self.restampAsSeenNow(file, at: now) + continue + } else { + // Nothing to judge it by, and guessing would mean guessing at + // a deletion. + continue } + + guard age < horizon else { continue } + do { + try FileManager.default.removeItem(at: file) + } catch { + Self.logger.error("Failed to remove stale recovery file \(file.lastPathComponent): \(error)") + } + } + } + + /// The file's modification date, or `nil` if the filesystem will not say. + private static func modificationDate(of file: URL) -> Date? { + try? file.resourceValues( + forKeys: [.contentModificationDateKey] + ).contentModificationDate + } + + /// Anchors a file whose every recorded date lies in the future to this + /// sweep, so the next one can age it out normally. + /// + /// Touching the metadata and not the content: the snapshot itself stays + /// byte-for-byte what the user's editor wrote, and only the clock claim + /// that made it un-collectable is replaced. + /// + /// It is still destructive in one respect, which is why it is fenced off + /// behind ``futureDateTolerance`` rather than firing on any date past + /// `now`: for a snapshot whose JSON cannot be decoded, the modification + /// date is the *only* surviving record of when it was written, and this + /// overwrites it. The trade is deliberate — a date nothing can reach is a + /// file no sweep can ever collect — but it is a one-way loss of the one + /// piece of provenance such a file has left. + private static func restampAsSeenNow(_ file: URL, at now: Date) { + do { + try FileManager.default.setAttributes( + [.modificationDate: now], + ofItemAtPath: file.path + ) + } catch { + logger.error( + "Cannot restamp future-dated recovery file \(file.lastPathComponent): \(error)" + ) } } diff --git a/Pine/Strings.swift b/Pine/Strings.swift index 32f477fb..8bb6df60 100644 --- a/Pine/Strings.swift +++ b/Pine/Strings.swift @@ -2152,6 +2152,18 @@ enum Strings { static let recoveryMessage: LocalizedStringKey = "recovery.message" static let recoveryRecoverAll: LocalizedStringKey = "recovery.recoverAll" static let recoveryDiscard: LocalizedStringKey = "recovery.discard" + static let recoveryLater: LocalizedStringKey = "recovery.later" + static let recoveryDiscardHint: LocalizedStringKey = "recovery.discardHint" + + /// How long snapshots nobody decided about are kept. + /// + /// Returns a `LocalizedStringKey` rather than a resolved `String` because + /// it is rendered inside the recovery sheet: only the key form follows the + /// SwiftUI environment locale, which is what lets the sheet be measured in + /// every supported locale. + static func recoveryRetentionNotice(days: Int) -> LocalizedStringKey { + "recovery.retentionNotice \(days)" + } static var recoveryUntitled: String { String(localized: "recovery.untitled") diff --git a/PineTests/DestructiveShortcutPolicyTests.swift b/PineTests/DestructiveShortcutPolicyTests.swift new file mode 100644 index 00000000..7b269ef4 --- /dev/null +++ b/PineTests/DestructiveShortcutPolicyTests.swift @@ -0,0 +1,1256 @@ +// +// DestructiveShortcutPolicyTests.swift +// PineTests +// +// Repo-wide guard for the bug class behind #1503: a `Button(role: +// .destructive)` that also answers to a reflex key. Escape and Return are +// pressed without looking; binding either to an irreversible action means a +// user can destroy data by dismissing a dialog the way macOS taught them to. +// +// These tests read production Swift as text. SwiftUI draws its own buttons, +// so there is no `NSButton` in the hosted hierarchy whose `keyEquivalent` a +// test could inspect; what the recovery sheet actually does with each key is +// asserted by pressing them in `RecoveryDialogEscapeSafetyTests`. Scanning +// the sources is what extends the invariant to code nobody has written yet. +// +// A source scanner is only worth its runtime if it is itself tested, so the +// scanner runs against built-in fixtures with known answers before it is +// turned on the repository: one that must be reported, one that must not. +// +// What it cannot see, stated plainly so nobody mistakes a green run for a +// proof. Each of these has a fixture below asserting the blindness, so the +// list cannot rot and closing a hole is a test that starts failing: +// +// - a shortcut held in a variable or computed property, rather than written +// at the call site — which is precisely how `RecoveryDialogView` now does +// it (`.keyboardShortcut(choice.keyboardShortcuts.first)`). That file is +// covered instead by `recoverySheetHasNoLiteralShortcut` below and by the +// `isDestructive` guard inside `RecoveryDialogChoice.keyboardShortcuts`, +// which makes the offending combination unrepresentable in the type; +// - a proxy written as `Button(action:label:)` rather than with a trailing +// closure. An invisible control in a destructive button's `.background` +// carrying the reflex key — the other idiom this sheet introduces — *is* +// caught when it is spelled `Button { … } label: { … }`, because the +// window only ends at a literal `Button(`; spelled with an argument list +// it ends the window instead and takes its own shortcut with it; +// - `KeyboardShortcut.cancelAction` and `KeyboardShortcut(.escape)` spelled +// in full, `.init(.escape)`, and a raw `"\u{1B}"` or `"\r"` character — +// partially closed below, but only in the forms enumerated there; +// - anything not written as a literal `Button(` — a wrapper view, a +// `ForEach` over a table of actions, an AppKit `NSButton`. +// +// In other words: this catches the bug as it was written in #1503 and the +// obvious ways of rewriting it. It is a floor, not a ceiling. +// + +import Foundation +import Testing + +@Suite("Destructive buttons answer to no reflex key") +struct DestructiveShortcutPolicyTests { + + /// Ways of naming a keystroke a user produces without deciding anything. + /// + /// Matched against a window that also contains `.keyboardShortcut(`, so + /// the spelling of the modifier and the spelling of the key can vary + /// independently: `.keyboardShortcut(.cancelAction)` and + /// `.keyboardShortcut(KeyboardShortcut.cancelAction)` are the same bug. + private static let reflexShortcuts = [ + ".cancelAction", + ".defaultAction", + "(.escape", + "(.return", + ] + + // MARK: - The scanner, tested + + @Test("The scanner reports a destructive button bound to Escape") + func scannerCatchesTheViolationItExistsFor() { + let offenders = Self.offenders(in: Self.violationFixture) + + #expect( + offenders.count == 1, + "Expected exactly one offender, got \(offenders)" + ) + #expect(offenders.first?.contains(".cancelAction") == true) + } + + @Test("The scanner reports a conditionally destructive button too") + func scannerCatchesAConditionalRole() { + // The role does not have to be a literal. `role: isDestructive ? + // .destructive : nil` is the form this repository's recovery sheet + // uses, and an earlier version of this scanner was blind to it. + let offenders = Self.offenders(in: Self.conditionalRoleFixture) + + #expect( + offenders.count == 1, + "A conditional destructive role slipped past the scanner: \(offenders)" + ) + } + + @Test("The scanner reports a destructive button with a wrapped call") + func scannerCatchesAMultiLineArgumentList() { + let offenders = Self.offenders(in: Self.wrappedCallFixture) + + #expect( + offenders.count == 1, + "A wrapped `Button(` call slipped past the scanner: \(offenders)" + ) + } + + @Test("The scanner does not blame a safe button standing before a destructive one") + func scannerDoesNotBlameASafeNeighbour() { + // The shape at `TerminalBarView.swift`: a plain `Button(title, + // action:)` followed by a destructive one. Slicing the first button's + // head up to the next `{` swallows the *second* button's role and + // reports the safe button — with a file:line that points at innocent + // code and a CI failure nobody can act on. + #expect(Self.offenders(in: Self.safeNeighbourFixture).isEmpty) + #expect( + Self.destructiveButtonWindows(in: Self.safeNeighbourFixture).count == 1, + "The scanner must still see the destructive button in that pair" + ) + } + + @Test("The scanner leaves a destructive button with no shortcut alone") + func scannerAcceptsADestructiveButtonWithoutAShortcut() { + #expect(Self.offenders(in: Self.compliantFixture).isEmpty) + #expect( + Self.destructiveButtonWindows(in: Self.compliantFixture).count == 1 + ) + } + + @Test("The scanner's window reaches past the button's own closure") + func scannerWindowCoversTheModifierChain() throws { + // The modifier that carries the shortcut sits after the label closure + // closes. A window that stops at the first `}` sees nothing and turns + // the whole suite into a no-op that passes while the bug is live. + let window = try #require( + Self.destructiveButtonWindows(in: Self.violationFixture).first + ) + #expect(window.contains(".keyboardShortcut")) + } + + @Test("The scanner reports the spelled-out and constructed forms too") + func scannerCatchesShortcutsThatAreNotDotShorthand() { + #expect( + Self.offenders(in: Self.spelledOutShortcutFixture).count == 1, + "`KeyboardShortcut.cancelAction` written in full slipped past" + ) + #expect( + Self.offenders(in: Self.constructedShortcutFixture).count == 1, + "`KeyboardShortcut(.escape)` slipped past" + ) + } + + @Test("The scanner's blind spots are exactly the documented ones") + func blindSpotsAreWhereTheyAreDocumented() { + // A hole named in this file's header being closed is good news, and it + // fails here so the header gets updated with it. A silent *new* hole + // is what the header exists to prevent, and only a reader can catch + // that — which is why the list is prose that must be maintained rather + // than a comment nobody has to touch. + #expect( + Self.offenders(in: Self.indirectShortcutFixture).isEmpty, + """ + The scanner now follows a shortcut through a property. Update the \ + header: `RecoveryDialogView` is no longer covered only by \ + `recoverySheetHasNoLiteralShortcut` and the `isDestructive` guard + """ + ) + #expect( + Self.offenders(in: Self.proxyShortcutFixture).count == 1, + """ + The scanner stopped seeing a reflex key on a proxy control inside \ + a destructive button's `.background`. That is the second idiom \ + this sheet introduces, and losing it is a hole, not a cleanup + """ + ) + // …and the destructive button is still *found* in both, so the + // blindness is about the shortcut, not about the role. + #expect( + Self.destructiveButtonWindows( + in: Self.indirectShortcutFixture + ).count == 1 + ) + } + + // MARK: - The repository + + @Test("No destructive button in the app binds Escape or Return") + func noDestructiveButtonBindsAReflexKey() throws { + var offenders: [String] = [] + var scanned = 0 + + for source in try Self.productionSources() { + for window in Self.destructiveButtonWindows(in: source.text) { + scanned += 1 + guard window.contains(".keyboardShortcut(") else { continue } + for reflex in Self.reflexShortcuts where window.contains(reflex) { + offenders.append( + "\(source.name): \(Self.condense(window))" + ) + } + } + } + + #expect( + scanned > 0, + "The scanner found no destructive buttons at all — it stopped working" + ) + #expect( + offenders.isEmpty, + """ + A destructive button must not answer to a reflex key: Escape and \ + Return get pressed without deciding, and there is no confirmation \ + and no undo behind them (#1503). Offenders: + \(offenders.joined(separator: "\n")) + """ + ) + } + + @Test("The scanner still sees the destructive buttons it is meant to guard") + func scannerStillSeesKnownDestructiveButtons() throws { + let files = try Self.productionSources() + .filter { !Self.destructiveButtonWindows(in: $0.text).isEmpty } + .map(\.name) + + // A refactor that hides every destructive button behind a helper would + // silently turn this suite into a no-op. These files are the canaries — + // `RecoveryDialogView.swift` above all, since it is the file the whole + // guard was written for and it states its role conditionally. + for expected in [ + "FileNodeRow.swift", + "EditorTabBar.swift", + "RecoveryDialogView.swift", + ] { + #expect( + files.contains(expected), + "\(expected) no longer exposes a destructive button to the scanner" + ) + } + } + + @Test("The recovery sheet spells no keystroke out at its buttons") + func recoverySheetHasNoLiteralShortcut() throws { + let source = try Self.source(named: "Pine/RecoveryDialogView.swift") + let literals = [ + ".cancelAction", + ".defaultAction", + ".escape", + ".return", + "KeyboardShortcut(", + ] + let offenders = source + .split(separator: "\n", omittingEmptySubsequences: false) + .enumerated() + .filter { _, line in + line.contains(".keyboardShortcut(") + && literals.contains { line.contains($0) } + } + .map { + "RecoveryDialogView.swift:\($0.offset + 1): " + + $0.element.trimmingCharacters(in: .whitespaces) + } + + #expect( + offenders.isEmpty, + """ + Every keystroke in the recovery sheet must come from \ + `RecoveryDialogChoice.keyboardShortcuts`, where the destructive \ + case is asserted to carry none (#1503). Offenders: \(offenders) + """ + ) + // …and the property that owns them must still be the one being used. + #expect(source.contains("choice.keyboardShortcuts")) + } + + // MARK: - The seam between the sheet and the filesystem + // + // `ContentView+Helpers.swift` is where the sheet's answer becomes an + // `unlink`, and it is invisible to everything else that guards this area: + // the coverage gate excludes it by name + // (`.github/scripts/check_coverage.py`), no unit test loads `ContentView`, + // and the hosted sheet tests stop at the enum the view emits. So the seam + // is asserted here, as text — the same way this file already asserts that + // the sheet spells no keystroke out. + // + // **What these two scanners are.** A required-substring list and a + // blacklist, run over one function's body with its whitespace collapsed. + // They pin the shape the reviewed code has; they do not understand it. + // They do catch, and there are fixtures below proving each: + // + // - a branch that builds its own list of IDs instead of asking the + // choice (the exact shape of #1503, reintroduced at the call site); + // - the two branches swapped, so Recover All deletes and Discard + // restores — the condensed match pins `case .recoverAll:` to the call + // that follows it; + // - any of the three statements that *end* the safe branch going + // missing. Only the deleting call used to be pinned, which left the + // rest of the branch free: dropping `showRecoveryDialog = false` + // makes the sheet unclosable, because it has no system dismissal and + // Escape now lands in this very branch — leaving Discard and Force + // Quit as the only ways out, which is #1503 by a third road and with + // a green suite. Dropping `markRecoveryOfferAnswered()` turns "not + // now" into "again in ten seconds", since the snapshots are still on + // disk, own no live tab, and the scene `.task` re-runs on restoration + // and on close/reopen. Both are pinned as one condensed sequence, so + // they cannot be reordered or separated either; + // - the same three-statement tail after the `await` in `recoverTabs`, + // where the consequence is milder (migrated snapshots carry live tab + // IDs and are filtered out of the offer) but the shape is identical; + // - `checkForRecovery` going back to `pendingRecoveryEntries()`. + // + // What they do not catch, stated so nobody reads green as proof: + // + // - a *second* deleting call added next to the required one. The + // required substring is still there and the blacklist does not know + // about the new call; + // - a list of IDs spelled some third way — `map { entry in entry.0 }`, + // a helper, a stored property — the blacklist names two spellings; + // - a `/* … */` comment. `strippingLineComments` only knows `//`, so a + // block comment's contents reach the needles as if they were code — + // a required substring quoted inside one satisfies its rule, and a + // forbidden one mentioned inside one fails it. Neither function these + // rules run against contains a block comment; + // - a function body that closes early. `functionBody` ends at the first + // `"\n }\n"` — a line closing at method indentation — so a nested + // closure or type indented back to four spaces truncates the body + // there, and everything after it is invisible to every rule. The + // bodies scanned here indent their closures deeper than that; + // - anything at all in `RecoveryManager`, which is where the deletion + // actually happens. That side is covered by behaviour, in + // `RecoveryTerminationSweepTests` and `RecoveryManagerTests`. + + private struct SourceRule { + let needle: String + let mustBePresent: Bool + let reason: String + } + + /// The resolver's shape: what applying a choice must and must not say. + private static let resolverRules: [SourceRule] = [ + SourceRule( + needle: "withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries)", + mustBePresent: true, + reason: """ + The deletion decision has to stay on `RecoveryDialogChoice`, \ + where it is enumerated by test and shares a value with the \ + button role and the empty shortcut list (#1503) + """ + ), + SourceRule( + needle: "switch choice {", + mustBePresent: true, + reason: """ + The resolver must switch over the choice, so a fourth case is \ + a compile error rather than a silent fall-through + """ + ), + SourceRule( + needle: "case .recoverAll: recoverTabs() case .discard, .later:", + mustBePresent: true, + reason: """ + Recover All must run the restorer and nothing else, and the \ + other two must share the deleting branch. Matched as one \ + condensed sequence so the branches cannot be swapped or \ + have anything slipped between them — `case .recoverAll: \ + showRecoveryDialog = false` is a Recover All that silently \ + does nothing, and it satisfies every separate substring + """ + ), + SourceRule( + needle: """ + projectManager.markRecoveryOfferAnswered() \ + showRecoveryDialog = false recoveryEntries = [] + """, + mustBePresent: true, + reason: """ + The safe branch must answer the offer, close the sheet and \ + clear the list — all three, in that order. Only the deleting \ + call above was pinned, which left the rest of this branch \ + free to be edited away one statement at a time. Dropping \ + `showRecoveryDialog = false` leaves the sheet with no way \ + out at all: a SwiftUI sheet has no system dismissal here, \ + and Escape resolves to `.later`, which is this branch — so \ + the only exits become the irreversible Discard and Force \ + Quit. Dropping `markRecoveryOfferAnswered()` makes "Later" \ + mean "again in ten seconds": the snapshots are deliberately \ + still on disk, they belong to no open tab so the live filter \ + keeps them, and the scene `.task` re-runs on restoration and \ + on close/reopen. Matched condensed so the three cannot be \ + separated or reordered (#1503) + """ + ), + SourceRule( + needle: "default:", + mustBePresent: false, + reason: """ + A `default:` turns a fourth choice into "close the sheet, \ + delete nothing, mark it answered" instead of a compile error + """ + ), + SourceRule( + needle: "if choice.isDestructive", + mustBePresent: false, + reason: """ + The deletion is behind a condition again. Escape resolves to \ + `.later` and reaches the same call: the only thing that keeps \ + it from deleting is what the choice hands back, not what the \ + call site tests (#1503) + """ + ), + SourceRule( + needle: "recoveryEntries.map(\\.0)", + mustBePresent: false, + reason: "The resolver builds its own list of IDs to delete (#1503)" + ), + SourceRule( + needle: "recoveryEntries.map { $0.0 }", + mustBePresent: false, + reason: "The resolver builds its own list of IDs to delete (#1503)" + ), + ] + + /// What starting a Recover All must and must not say. + private static let restoreRules: [SourceRule] = [ + SourceRule( + needle: "projectManager.beginRecoveryRestore() Task { @MainActor in defer { projectManager.endRecoveryRestore() }", + mustBePresent: true, + reason: """ + The restore has to be marked in flight *before* the task is \ + started and cleared from a `defer` inside it. Restoring parks \ + on a native large-file sheet, and a scene `.task` re-running \ + in that window otherwise builds a second recovery sheet from \ + the same crash entries — which migrates them twice, leaves \ + the parked restore resuming against a detached `TabManager`, \ + and writes a snapshot under a runtime ID no window owns \ + (#1503). Matched as one condensed sequence so the call cannot \ + drift after the `Task` or lose its `defer` + """ + ), + SourceRule( + needle: """ + projectManager.markRecoveryOfferAnswered() \ + recoveryEntries = retained \ + showRecoveryDialog = !retained.isEmpty + """, + mustBePresent: true, + reason: """ + The same three-statement tail as the safe branch of the \ + resolver, and free for the same reason: nothing else pinned \ + it. It has to run *after* the `await`, because a restore that \ + never finishes must not silence the offer, and it has to run \ + in full — `showRecoveryDialog = !retained.isEmpty` is what \ + puts anything the restorer handed back in front of the user \ + again instead of dropping it silently. Milder than the \ + resolver's copy (a migrated snapshot carries a live tab's ID \ + and the offer filters it out) but the same shape (#1503) + """ + ), + SourceRule( + needle: "Task.isCancelled", + mustBePresent: false, + reason: """ + An unstructured `Task` inherits no cancellation and this one's \ + handle is discarded, so nothing in the app can cancel it and \ + the guard can never fire. It reads like ⌘W is handled here; \ + ⌘W does not even reach `closeActiveTab()` while the sheet is \ + up, because `documentWindow(for: NSApp.keyWindow)` resolves to \ + the sheet, whose delegate is not a `CloseDelegate` + """ + ), + ] + + /// What deciding whether to show the sheet must and must not say. + private static let offerRules: [SourceRule] = [ + SourceRule( + needle: "projectManager.pendingRecoveryOffer()", + mustBePresent: true, + reason: """ + The offer has to come from the project, which outlives the \ + window and remembers being answered, and which filters out \ + snapshots belonging to tabs that are open right now (#1503) + """ + ), + SourceRule( + needle: "pendingRecoveryEntries", + mustBePresent: false, + reason: """ + `pendingRecoveryEntries()` is every JSON file in the \ + directory. Going back to it re-offers on every scene `.task` \ + — SwiftUI re-runs those on restoration and on close/reopen — \ + and puts live dirty tabs into `recoveryEntries`, where \ + Discard deletes the crash protection of buffers the user is \ + looking at (#1503) + """ + ), + ] + + /// Drops `//` comments so a body can explain, in prose, the mistake its + /// blacklist forbids — `checkForRecovery` names `pendingRecoveryEntries()` + /// in a comment saying why it does not call it. + /// + /// Naive about `//` inside a string literal. Neither function these rules + /// run against contains one, and a scanner that silently matched a comment + /// would be worse than one that occasionally has to be taught about a URL. + /// + /// Naive about `/* … */` as well, and that one fails the other way: a + /// block comment's contents survive into the condensed text and are + /// matched as if they were code. Listed in this file's header among the + /// blind spots; neither scanned function contains one. + private static func strippingLineComments(_ body: String) -> String { + body + .split(separator: "\n", omittingEmptySubsequences: false) + .map { line -> Substring in + guard let comment = line.range(of: "//") else { return line } + return line[line.startIndex.. [String] { + let condensed = condense(strippingLineComments(body)) + return rules.compactMap { rule in + guard condensed.contains(rule.needle) != rule.mustBePresent else { + return nil + } + let verb = rule.mustBePresent ? "missing" : "present" + return "\(verb) `\(rule.needle)`: \(rule.reason)" + } + } + + @Test("The resolver asks the choice what it may delete, and asks always") + func theResolverRoutesDeletionThroughTheChoice() throws { + let body = try Self.functionBody( + startingAt: "func resolveRecoveryOffer(", + in: try Self.source(named: "Pine/ContentView+Helpers.swift") + ) + + #expect( + Self.violations(of: Self.resolverRules, in: body).isEmpty, + """ + `resolveRecoveryOffer` no longer has the shape #1503 was fixed \ + into: \(Self.violations(of: Self.resolverRules, in: body)) + """ + ) + } + + @Test("The offer comes from the project, not from the directory listing") + func checkForRecoveryAsksTheProjectForTheOffer() throws { + let body = try Self.functionBody( + startingAt: "func checkForRecovery(", + in: try Self.source(named: "Pine/ContentView+Helpers.swift") + ) + + #expect( + Self.violations(of: Self.offerRules, in: body).isEmpty, + """ + `checkForRecovery` no longer asks the project what is worth \ + offering: \(Self.violations(of: Self.offerRules, in: body)) + """ + ) + } + + @Test("A Recover All marks itself in flight for as long as it runs") + func recoverTabsMarksTheRestoreInFlight() throws { + let body = try Self.functionBody( + startingAt: "func recoverTabs(", + in: try Self.source(named: "Pine/ContentView+Helpers.swift") + ) + + #expect( + Self.violations(of: Self.restoreRules, in: body).isEmpty, + """ + `recoverTabs` no longer fences its in-flight window: \ + \(Self.violations(of: Self.restoreRules, in: body)) + """ + ) + } + + @Test("The resolver scanner fails on a resolver that deletes on Later") + func resolverScannerCatchesAMutatedResolver() { + // Without these, both scans above are assertions nobody has ever seen + // fail — the shape of guard that passes because its needle happens to + // be somewhere in the file. The shortcut scanner in this file is + // tested against fixtures; so are these. + #expect( + Self.violations( + of: Self.resolverRules, + in: Self.compliantResolverFixture + ).isEmpty, + "The scanner reports the shape the repository actually ships" + ) + #expect( + !Self.violations( + of: Self.resolverRules, + in: Self.laterDeletesResolverFixture + ).isEmpty, + """ + A resolver whose `.later` branch builds its own list of IDs — \ + #1503 rewritten at the call site — passed the scan + """ + ) + #expect( + !Self.violations( + of: Self.resolverRules, + in: Self.swappedBranchesResolverFixture + ).isEmpty, + """ + A resolver with Recover All and Discard transposed passed the \ + scan: every required substring is present, only the pairing is \ + wrong, and that is what the condensed sequence exists to pin + """ + ) + #expect( + !Self.violations( + of: Self.resolverRules, + in: Self.silentRecoverAllResolverFixture + ).isEmpty, + "A Recover All that closes the sheet and restores nothing passed" + ) + #expect( + !Self.violations( + of: Self.resolverRules, + in: Self.unclosableSheetResolverFixture + ).isEmpty, + """ + A resolver whose safe branch never closes the sheet passed the \ + scan. There is no system dismissal behind this sheet and Escape \ + lands in that branch, so the only remaining ways out are the \ + irreversible Discard and Force Quit — #1503 by a third road + """ + ) + #expect( + !Self.violations( + of: Self.resolverRules, + in: Self.unansweredOfferResolverFixture + ).isEmpty, + """ + A resolver whose safe branch never marks the offer answered \ + passed the scan: "Later" becomes "again on the next scene task" + """ + ) + } + + @Test("The offer scanner fails on a checkForRecovery that lists the directory") + func offerScannerCatchesADirectoryListing() { + #expect( + Self.violations( + of: Self.offerRules, + in: Self.compliantOfferFixture + ).isEmpty + ) + #expect( + !Self.violations( + of: Self.offerRules, + in: Self.directoryListingOfferFixture + ).isEmpty, + """ + `checkForRecovery` reading `pendingRecoveryEntries()` directly \ + passed the scan + """ + ) + } + + @Test("The restore scanner fails on a restore that drops its tail") + func restoreScannerCatchesAMutatedRestore() { + #expect( + Self.violations( + of: Self.restoreRules, + in: Self.compliantRestoreFixture + ).isEmpty, + "The scanner reports the shape the repository actually ships" + ) + #expect( + !Self.violations( + of: Self.restoreRules, + in: Self.unansweredRestoreFixture + ).isEmpty, + """ + A Recover All that finishes without answering the offer passed \ + the scan: the restored buffers' snapshots now carry live tab IDs \ + and are filtered out, but anything the restorer handed back is \ + offered again on the next scene task + """ + ) + #expect( + !Self.violations( + of: Self.restoreRules, + in: Self.droppedRetainedRestoreFixture + ).isEmpty, + """ + A Recover All that swallows the entries the restorer could not \ + restore passed the scan: the user cancelled a large-file prompt \ + and is never told the buffer is still waiting + """ + ) + } + + @Test("Recovery discovery is awaited before a terminal can be seeded") + func theTaskAwaitsRecoveryDiscoveryBeforeSeeding() throws { + // `checkForRecovery()` suspends now — it reads the snapshot directory + // off the main actor (#1503) — and the very next call, + // `seedInitialTerminalIfNeeded(disposition:)`, guards on + // `showRecoveryDialog` and `recoveryEntries`, the two properties + // `checkForRecovery` sets. Dropping the `await` (or moving the call + // into a detached `Task`) compiles with a warning at most, and turns + // a pending offer into a race: the seeding guard reads the flags + // before they are set, replaces the empty editor leaf with a + // terminal, and the sheet then offers to recover into a pane that is + // no longer there. + // + // Whole-file rather than scoped: this pair lives in a `.task` + // closure, not in a `func`, so `functionBody` cannot reach it. The + // condensed form tolerates the comment block between the two calls — + // see `condense`. + let source = try Self.condense( + Self.strippingLineComments( + Self.source(named: "Pine/ContentView.swift") + ) + ) + + #expect( + source.contains( + "await checkForRecovery() seedInitialTerminalIfNeeded(disposition: disposition)" + ), + """ + The project scene's `.task` no longer awaits recovery discovery \ + immediately before seeding an initial terminal (#1503, #1251) + """ + ) + } + + @Test("The recovery sheet's button role comes from the choice") + func recoverySheetDerivesTheButtonRoleFromTheChoice() throws { + // Inverting the ternary paints the two safe buttons red and the + // irreversible one neutral. Nothing else sees it: SwiftUI's + // `ButtonRole` is not readable from the hosted hierarchy, the shortcut + // scanner only asks whether `.destructive` appears in the argument + // list — it still does — and every behavioural test in + // `RecoveryDialogEscapeSafetyTests` stops at which choice came back. + let source = try Self.source(named: "Pine/RecoveryDialogView.swift") + + #expect( + source.contains( + "Button(role: choice.isDestructive ? .destructive : nil)" + ), + """ + The recovery sheet no longer derives each button's role from \ + `RecoveryDialogChoice.isDestructive` in the reviewed form. The \ + colour of the only irreversible control on this sheet is the one \ + warning a sighted user gets before clicking it (#1503) + """ + ) + } + + @Test("The recovery sheet still offers an irreversible discard") + func recoverySheetStillOffersDiscard() throws { + let source = try Self.source(named: "Pine/RecoveryDialogView.swift") + + // Removing Escape must not have been achieved by removing the choice: + // a deliberate discard is still a supported, wanted action. + #expect(source.contains(".destructive")) + #expect(source.contains("case discard")) + } + + // MARK: - Fixtures + + private static let violationFixture = """ + var body: some View { + Button(role: .destructive) { + deleteEverything() + } label: { + Text("Discard") + } + .keyboardShortcut(.cancelAction) + } + """ + + private static let conditionalRoleFixture = """ + var body: some View { + Button(role: isDangerous ? .destructive : nil) { + deleteEverything() + } label: { + Text("Discard") + } + .keyboardShortcut(.defaultAction) + } + """ + + private static let wrappedCallFixture = """ + var body: some View { + Button( + role: .destructive, + action: deleteEverything + ) { + Text("Discard") + } + .keyboardShortcut(.escape) + } + """ + + private static let safeNeighbourFixture = """ + var body: some View { + HStack { + Button(resume.title, action: resume.action) + .keyboardShortcut(.cancelAction) + Button(role: .destructive) { + onClose() + } label: { + Label("Close", systemImage: "xmark") + } + .disabled(!canClose) + } + } + """ + + private static let spelledOutShortcutFixture = """ + var body: some View { + Button(role: .destructive) { + deleteEverything() + } label: { + Text("Discard") + } + .keyboardShortcut(KeyboardShortcut.cancelAction) + } + """ + + private static let constructedShortcutFixture = """ + var body: some View { + Button(role: .destructive) { + deleteEverything() + } label: { + Text("Discard") + } + .keyboardShortcut(KeyboardShortcut(.escape)) + } + """ + + /// A destructive button whose keystroke arrives through a property. The + /// scanner cannot follow it, and `blindSpotsAreWhereTheyAreDocumented` + /// asserts exactly that so the limitation is checked rather than claimed. + private static let indirectShortcutFixture = """ + var body: some View { + Button(role: .destructive) { + deleteEverything() + } label: { + Text("Discard") + } + .keyboardShortcut(choice.shortcut) + } + """ + + /// A destructive button carrying Escape on an invisible proxy in its + /// `.background` — the idiom the recovery sheet uses for its second + /// cancellation gesture, and one the scanner does catch: a trailing-closure + /// `Button {` is not the literal `Button(` that ends a window, so the proxy + /// and its shortcut stay inside the destructive button's slice. + private static let proxyShortcutFixture = """ + var body: some View { + Button(role: .destructive) { + deleteEverything() + } label: { + Text("Discard") + } + .background { + Button { deleteEverything() } label: { Color.clear } + .keyboardShortcut(.cancelAction) + } + } + """ + + /// The resolver as this branch ships it, so the rules are known to pass + /// something other than the file they were written against. + private static let compliantResolverFixture = """ + func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) { + switch choice { + case .recoverAll: + recoverTabs() + case .discard, .later: + projectManager.recoveryManager?.deleteSnapshots( + withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries) + ) + projectManager.markRecoveryOfferAnswered() + showRecoveryDialog = false + recoveryEntries = [] + } + } + """ + + /// #1503 rewritten one layer down: the choice is still switched on, but + /// the branch Escape lands in deletes everything the sheet was showing. + private static let laterDeletesResolverFixture = """ + func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) { + switch choice { + case .recoverAll: + recoverTabs() + case .discard, .later: + projectManager.recoveryManager?.deleteSnapshots( + withRecoveryIDs: recoveryEntries.map(\\.0) + ) + projectManager.markRecoveryOfferAnswered() + showRecoveryDialog = false + recoveryEntries = [] + } + } + """ + + /// Every required substring present, both branches intact — and wired to + /// each other the wrong way round. + private static let swappedBranchesResolverFixture = """ + func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) { + switch choice { + case .discard, .later: + recoverTabs() + case .recoverAll: + projectManager.recoveryManager?.deleteSnapshots( + withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries) + ) + projectManager.markRecoveryOfferAnswered() + showRecoveryDialog = false + recoveryEntries = [] + } + } + """ + + /// Recover All that closes the sheet and restores nothing: the user's + /// work stays on disk and they are told it was recovered. + private static let silentRecoverAllResolverFixture = """ + func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) { + switch choice { + case .recoverAll: + showRecoveryDialog = false + case .discard, .later: + projectManager.recoveryManager?.deleteSnapshots( + withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries) + ) + projectManager.markRecoveryOfferAnswered() + showRecoveryDialog = false + recoveryEntries = [] + } + } + """ + + /// The in-flight fence and the post-`await` tail as this branch ships + /// them, comment block included — which is also what proves `condense` + /// does not let a comment split a pinned sequence. + private static let compliantRestoreFixture = """ + func recoverTabs() { + projectManager.beginRecoveryRestore() + Task { @MainActor in + defer { projectManager.endRecoveryRestore() } + let retained = await recoveryManager.restorePendingEntries( + entries, + in: target, + context: context + ) + // Answered once the restore has actually finished, not + // before the `await`. + projectManager.markRecoveryOfferAnswered() + recoveryEntries = retained + showRecoveryDialog = !retained.isEmpty + } + } + """ + + /// A restore that finishes without recording that the offer was answered. + private static let unansweredRestoreFixture = """ + func recoverTabs() { + projectManager.beginRecoveryRestore() + Task { @MainActor in + defer { projectManager.endRecoveryRestore() } + let retained = await recoveryManager.restorePendingEntries( + entries, + in: target, + context: context + ) + recoveryEntries = retained + showRecoveryDialog = !retained.isEmpty + } + } + """ + + /// A restore that throws away whatever the restorer could not restore. + private static let droppedRetainedRestoreFixture = """ + func recoverTabs() { + projectManager.beginRecoveryRestore() + Task { @MainActor in + defer { projectManager.endRecoveryRestore() } + _ = await recoveryManager.restorePendingEntries( + entries, + in: target, + context: context + ) + projectManager.markRecoveryOfferAnswered() + recoveryEntries = [] + showRecoveryDialog = false + } + } + """ + + /// The safe branch with `showRecoveryDialog = false` taken out. Every + /// previously pinned substring is still present — the deleting call, the + /// switch, the branch pairing — and the sheet can no longer be closed at + /// all: Escape resolves to `.later`, which is this branch. + private static let unclosableSheetResolverFixture = """ + func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) { + switch choice { + case .recoverAll: + recoverTabs() + case .discard, .later: + projectManager.recoveryManager?.deleteSnapshots( + withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries) + ) + projectManager.markRecoveryOfferAnswered() + recoveryEntries = [] + } + } + """ + + /// The safe branch that never records the answer: the sheet closes, the + /// snapshots stay on disk owned by no live tab, and the next scene `.task` + /// — scene restoration, or the window closed and reopened — offers them + /// straight back. + private static let unansweredOfferResolverFixture = """ + func resolveRecoveryOffer(_ choice: RecoveryDialogChoice) { + switch choice { + case .recoverAll: + recoverTabs() + case .discard, .later: + projectManager.recoveryManager?.deleteSnapshots( + withRecoveryIDs: choice.snapshotsToDelete(from: recoveryEntries) + ) + showRecoveryDialog = false + recoveryEntries = [] + } + } + """ + + private static let compliantOfferFixture = """ + func checkForRecovery() async { + let entries = await projectManager.pendingRecoveryOffer() + guard !entries.isEmpty else { return } + recoveryEntries = entries + showRecoveryDialog = true + } + """ + + private static let directoryListingOfferFixture = """ + func checkForRecovery() async { + guard let entries = projectManager.recoveryManager?.pendingRecoveryEntries(), + !entries.isEmpty else { return } + recoveryEntries = entries + showRecoveryDialog = true + } + """ + + private static let compliantFixture = """ + var body: some View { + Button(role: .destructive) { + deleteEverything() + } label: { + Text("Discard") + } + .disabled(!canDelete) + } + """ + + // MARK: - Helpers + + private struct Source { + let name: String + let text: String + } + + private static func offenders(in text: String) -> [String] { + destructiveButtonWindows(in: text).filter { window in + window.contains(".keyboardShortcut(") + && reflexShortcuts.contains { window.contains($0) } + } + .map(condense) + } + + /// Source slices that begin at a destructive `Button(` and end where its + /// modifier chain plausibly ends: the next `Button(`, the next + /// `Divider()`, or the next blank line — whichever comes first. Modifier + /// chains are written without blank lines in this codebase, so a window + /// covers the whole chain, including the modifiers that follow the label + /// closure, without spilling into a sibling declaration. + private static func destructiveButtonWindows(in text: String) -> [String] { + var windows: [String] = [] + var searchStart = text.startIndex + + while let buttonStart = text.range( + of: "Button(", + range: searchStart.. Range? { + var parenDepth = 1 + var braceDepth = 0 + var index = open + var inString = false + var escaped = false + var scanned = 0 + + while index < text.endIndex, scanned < 1_000 { + let character = text[index] + scanned += 1 + + if inString { + if escaped { + escaped = false + } else if character == "\\" { + escaped = true + } else if character == "\"" { + inString = false + } + } else { + switch character { + case "\"": inString = true + case "(": parenDepth += 1 + case "{": braceDepth += 1 + case "}": + braceDepth -= 1 + if braceDepth < 0 { return nil } + case ")": + guard braceDepth == 0 else { break } + parenDepth -= 1 + if parenDepth == 0 { return open.. [Source] { + let root = repositoryRoot().appendingPathComponent("Pine") + // Deliberately no `.skipsHiddenFiles`: that option treats every file + // as hidden when any ancestor directory is (agents run this repo from + // `.claude/worktrees/…`), which would silently reduce the whole scan + // to zero files and turn this guard into a test that always passes. + let enumerator = try #require( + FileManager.default.enumerator( + at: root, + includingPropertiesForKeys: [.isRegularFileKey] + ) + ) + let urls = enumerator.compactMap { $0 as? URL } + .filter { $0.pathExtension == "swift" } + .filter { url in + !url.pathComponents + .dropFirst(root.pathComponents.count) + .contains { $0.hasPrefix(".") } + } + .sorted { $0.path < $1.path } + #expect(!urls.isEmpty, "Production sources must be discoverable") + + return try urls.map { + Source( + name: $0.lastPathComponent, + text: try String(contentsOf: $0, encoding: .utf8) + ) + } + } + + /// The text of one function, from its `func` keyword to the line that + /// closes it at method indentation. + /// + /// Scoped rather than whole-file, because a whole-file `contains` is a + /// guard that reports whatever some unrelated declaration happens to say — + /// `ContentView+Helpers.swift` has a `default:` two hundred lines below + /// the resolver, and matching it would make the exhaustiveness check pass + /// or fail for reasons nobody intended. + /// + /// "Closes it" means the first line that is exactly ` }` — four spaces, + /// method indentation. A nested closure or nested type whose own closing + /// brace lands at that column ends the body early and hides everything + /// after it from every rule, silently. The two bodies scanned here indent + /// their closures deeper; the limitation is in this file's header. + private static func functionBody( + startingAt signature: String, + in source: String + ) throws -> String { + let start = try #require( + source.range(of: signature), + "\(signature) is gone from the file this guard reads" + ) + let end = try #require( + source.range( + of: "\n }\n", + range: start.upperBound.. String { + try String( + contentsOf: repositoryRoot().appendingPathComponent(relativePath), + encoding: .utf8 + ) + } + + private static func repositoryRoot() -> URL { + URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() + .deletingLastPathComponent() + } + + /// Collapses a source slice onto one line, one space between statements. + /// + /// Blank lines are dropped *after* trimming, not only before it: a line + /// that held nothing but a `//` comment comes out of + /// ``strippingLineComments(_:)`` as whitespace, which `split` still counts + /// as a line and which would otherwise join as an empty string and put two + /// spaces into the middle of a condensed sequence. Every multi-statement + /// needle in this file would then depend on whether anyone had written a + /// comment inside the run it pins — a failure with nothing wrong behind + /// it. `recoverTabs` has eight such lines between its `await` and the tail + /// that follows it. + private static func condense(_ window: String) -> String { + window + .split(separator: "\n") + .map { $0.trimmingCharacters(in: .whitespaces) } + .filter { !$0.isEmpty } + .joined(separator: " ") + } +} diff --git a/PineTests/ErrorHandlingTests.swift b/PineTests/ErrorHandlingTests.swift index 9655fba6..ce98ded0 100644 --- a/PineTests/ErrorHandlingTests.swift +++ b/PineTests/ErrorHandlingTests.swift @@ -46,11 +46,11 @@ struct ErrorHandlingTests { // No crash = success } - @Test func deleteAllRecoveryFilesHandlesNonexistentDirectory() { + @Test func terminationSweepHandlesNonexistentDirectory() { let dir = FileManager.default.temporaryDirectory .appendingPathComponent("PineErrorTests-nonexistent-\(UUID().uuidString)") let manager = RecoveryManager(recoveryDirectory: dir) - manager.deleteAllRecoveryFiles() + manager.deleteSnapshotsOfOpenTabs([UUID()]) // No crash = success } diff --git a/PineTests/PersistenceFixtureCorpusTests.swift b/PineTests/PersistenceFixtureCorpusTests.swift index 9ce92d03..b5d2b45f 100644 --- a/PineTests/PersistenceFixtureCorpusTests.swift +++ b/PineTests/PersistenceFixtureCorpusTests.swift @@ -176,11 +176,24 @@ struct PersistenceFixtureCorpusTests { manager.deleteRecoveryFile(for: tab.id) } + // A snapshot from a newer build. `schemaVersion: 999` with an extra + // field this build has never heard of still decodes into a + // `RecoveryEntry` with real content, so it is offered — refusing to + // show it while the launch sweep collected it on a schedule was the + // one path that could destroy a readable buffer the user was never + // asked about (#1503). What must not happen is a rewrite: the + // `futureOnlyState` this build cannot represent has to survive + // byte-for-byte until the newer build reads it again. let id = UUID() let url = recovery.appendingPathComponent("\(id.uuidString).json") let future = try fixtureData("recovery-future.json", project: project) try future.write(to: url) - #expect(manager.pendingRecoveryEntries().isEmpty) + let offered = try #require( + manager.pendingRecoveryEntries().first { $0.0 == id }?.1 + ) + #expect(offered.schemaVersion == 999) + #expect(offered.hasSupportedSchema == false) + #expect(offered.content == "future unsaved buffer\n") #expect(try Data(contentsOf: url) == future) } diff --git a/PineTests/RecoveryDialogEscapeSafetyTests.swift b/PineTests/RecoveryDialogEscapeSafetyTests.swift new file mode 100644 index 00000000..7914cfbd --- /dev/null +++ b/PineTests/RecoveryDialogEscapeSafetyTests.swift @@ -0,0 +1,1565 @@ +// +// RecoveryDialogEscapeSafetyTests.swift +// PineTests +// +// #1503: the crash-recovery sheet bound `.keyboardShortcut(.cancelAction)` — +// Escape, the universal macOS "not now" — to a `Button(role: .destructive)` +// that unlinked every recovered buffer. Dismissing the sheet the way macOS +// teaches you to destroyed exactly the unsaved work the sheet exists to +// protect, with no confirmation and no undo. +// +// The tests that matter here host the real sheet in a real `NSWindow` and +// press real keys through `performKeyEquivalent(with:)`, then look at which +// choice came back. Asserting the shortcut table as data cannot see a button +// wired to the wrong action, which is the shape the original bug had. +// + +import AppKit +import Foundation +import SwiftUI +import Testing + +@testable import Pine + +@Suite("Recovery dialog keyboard safety", .serialized) +@MainActor +struct RecoveryDialogEscapeSafetyTests { + + // MARK: - Hosted key events + + @Test("Escape dismisses the hosted sheet without choosing destruction") + func escapeResolvesToTheSafeDismissal() { + let hosted = Self.hostSheet() + defer { hosted.window.close() } + + Self.sendEscape(to: hosted.window) + + #expect(hosted.recorder.chosen == [.later]) + #expect(hosted.recorder.chosen.allSatisfy { !$0.isDestructive }) + } + + @Test("Return in the hosted sheet recovers instead of discarding") + func returnResolvesToRecoverAll() { + let hosted = Self.hostSheet() + defer { hosted.window.close() } + + Self.sendReturn(to: hosted.window) + + #expect(hosted.recorder.chosen == [.recoverAll]) + #expect(hosted.recorder.chosen.allSatisfy { !$0.isDestructive }) + } + + @Test("⌘-. dismisses the hosted sheet exactly like Escape") + func commandPeriodMatchesEscape() { + // Every NSAlert in Pine answers to both Escape and ⌘-. + // (`AlertTemplate.makeAlert`). This sheet is the one dialog built in + // SwiftUI, and it must not be the one place where the second + // cancellation gesture does nothing — or worse, falls through to a + // different responder. + let hosted = Self.hostSheet() + defer { hosted.window.close() } + + Self.sendCommandPeriod(to: hosted.window) + + #expect(hosted.recorder.chosen == [.later]) + } + + @Test("The keypad Enter key is also the default action, never a delete") + func keypadEnterResolvesToRecoverAll() { + let hosted = Self.hostSheet() + defer { hosted.window.close() } + + Self.sendKey( + to: hosted.window, + characters: "\u{3}", + keyCode: 76 + ) + + #expect(hosted.recorder.chosen == [.recoverAll]) + } + + @Test("No sequence of reflex keys ever reaches the destructive choice") + func reflexKeysNeverReachDiscard() { + let hosted = Self.hostSheet() + defer { hosted.window.close() } + + // Hammering the keys a panicking user reaches for, in an order nobody + // designed for: the sheet stays open in production because the real + // callbacks dismiss it, but the view under test does not, so every + // keystroke is delivered to the same live hierarchy. + for _ in 0..<3 { + Self.sendEscape(to: hosted.window) + Self.sendCommandPeriod(to: hosted.window) + Self.sendReturn(to: hosted.window) + } + + #expect(hosted.recorder.chosen.count == 9) + #expect(!hosted.recorder.chosen.contains { $0.isDestructive }) + #expect( + Set(hosted.recorder.chosen) == [.later, .recoverAll], + """ + A reflex key resolved to something other than the two safe \ + choices: \(hosted.recorder.chosen) + """ + ) + } + + @Test("An empty entry list is still dismissible by Escape") + func emptySheetStillAnswersEscape() { + // A degenerate list must not remove the safe way out and leave the + // user with nothing but the destructive button. + let hosted = Self.hostSheet(entries: []) + defer { hosted.window.close() } + + Self.sendEscape(to: hosted.window) + + #expect(hosted.recorder.chosen == [.later]) + } + + @Test("A hundred entries do not change what Escape means") + func aLongListStillAnswersEscape() { + let entries = (0..<100).map { index in + ( + UUID(), + RecoveryEntry( + originalPath: "/tmp/project/file-\(index).swift", + content: "unsaved \(index)" + ) + ) + } + let hosted = Self.hostSheet(entries: entries) + defer { hosted.window.close() } + + Self.sendEscape(to: hosted.window) + + #expect(hosted.recorder.chosen == [.later]) + } + + @Test("Hosting and laying out the sheet chooses nothing on its own") + func hostingTheSheetChoosesNothing() { + let hosted = Self.hostSheet() + defer { hosted.window.close() } + + hosted.window.contentView?.layoutSubtreeIfNeeded() + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) + hosted.window.contentView?.layoutSubtreeIfNeeded() + + #expect(hosted.recorder.chosen.isEmpty) + } + + @Test("Escape means the same thing in every supported locale") + func escapeIsSafeInEveryLocale() { + for identifier in Self.supportedLocales { + let hosted = Self.hostSheet(locale: identifier) + defer { hosted.window.close() } + + Self.sendEscape(to: hosted.window) + + #expect( + hosted.recorder.chosen == [.later], + "Escape resolved to \(hosted.recorder.chosen) in \(identifier)" + ) + } + } + + // MARK: - From the keystroke to the filesystem + + // Everything above stops at the choice the sheet emits. That is one layer + // short of the bug: #1503 was a keystroke reaching `removeItem`, and a + // suite that only checks which enum case came back cannot tell a resolver + // that deletes on `.later` from one that does not. These drive a real + // `RecoveryManager` over a real directory through the seam production + // uses — `choice.snapshotsToDelete(from:)` into + // `deleteSnapshots(withRecoveryIDs:)`, exactly as + // `ContentView.resolveRecoveryOffer` spells it — and then look at the + // files. + + @Test("Escape leaves every snapshot file where it was") + func escapeLeavesTheSnapshotsOnDisk() throws { + try Self.withSnapshotFixture { fixture in + let hosted = Self.hostResolvingSheet(fixture) + defer { hosted.window.close() } + let before = fixture.files + + Self.sendEscape(to: hosted.window) + + #expect(hosted.recorder.chosen == [.later]) + #expect( + fixture.files == before, + "Escape unlinked a recovered buffer (#1503)" + ) + #expect(fixture.manager.pendingRecoveryEntries().count == 2) + } + } + + @Test("⌘-. leaves every snapshot file where it was") + func commandPeriodLeavesTheSnapshotsOnDisk() throws { + try Self.withSnapshotFixture { fixture in + let hosted = Self.hostResolvingSheet(fixture) + defer { hosted.window.close() } + let before = fixture.files + + Self.sendCommandPeriod(to: hosted.window) + + // The choice, not only the filesystem: a sheet that answered ⌘-. + // with nothing at all would leave the files alone too, and pass. + #expect(hosted.recorder.chosen == [.later]) + #expect(fixture.files == before) + } + } + + @Test("Hammering the reflex keys leaves every snapshot file where it was") + func reflexKeysLeaveTheSnapshotsOnDisk() throws { + try Self.withSnapshotFixture { fixture in + let hosted = Self.hostResolvingSheet(fixture) + defer { hosted.window.close() } + let before = fixture.files + + // Return resolves to `.recoverAll`, which in production hands off + // to the restorer instead of this resolver; what matters here is + // that no reflex key can reach the deletion path from the sheet. + for _ in 0..<3 { + Self.sendEscape(to: hosted.window) + Self.sendReturn(to: hosted.window) + Self.sendCommandPeriod(to: hosted.window) + } + + #expect(hosted.recorder.chosen.count == 9) + #expect(fixture.files == before) + } + } + + @Test("The same wire does delete when the choice is Discard") + func discardOverTheSameWireDeletesTheSnapshots() throws { + // Without this, every test above could be passing because the seam is + // dead — a resolver that never deletes anything would satisfy them all + // and ship a Discard button that does nothing. + try Self.withSnapshotFixture { fixture in + let hosted = Self.hostResolvingSheet(fixture) + defer { hosted.window.close() } + #expect(fixture.files.count == 2) + + // Discard carries no key equivalent by design, so it is reached + // the only way it can be: by choosing it. + hosted.resolve(.discard) + + #expect(fixture.files.isEmpty) + #expect(fixture.manager.pendingRecoveryEntries().isEmpty) + } + } + + @Test("Discard takes only what the sheet was showing") + func discardLeavesSnapshotsThatWereNotOffered() throws { + let dir = try Self.makeTempDir() + defer { Self.removeTempDir(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + manager.snapshotDirtyTabs([Self.dirtyTab(path: "/tmp/offered.swift")]) + let offered = manager.pendingRecoveryEntries() + // A snapshot written after the sheet was built — the periodic timer + // does this while the sheet is open. + let later = Self.dirtyTab(path: "/tmp/written-later.swift") + manager.snapshotDirtyTabs([later]) + + manager.deleteSnapshots( + withRecoveryIDs: RecoveryDialogChoice.discard + .snapshotsToDelete(from: offered) + ) + + #expect(manager.pendingRecoveryEntries().map(\.0) == [later.id]) + } + + // MARK: - What each choice is allowed to unlink + + @Test("No safe choice may unlink anything the sheet is showing") + func safeChoicesDeleteNothing() { + let entries = Self.makeEntries() + + for choice in RecoveryDialogChoice.allCases where !choice.isDestructive { + #expect( + choice.snapshotsToDelete(from: entries).isEmpty, + """ + \(choice.rawValue) is not destructive, so it must unlink \ + nothing — Escape and ⌘-. resolve to `.later` and a user who \ + dismissed the sheet has to find their work again on the next \ + launch (#1503) + """ + ) + } + } + + @Test("Discard is allowed to unlink exactly what is on screen") + func discardDeletesEveryDisplayedEntry() { + let entries = Self.makeEntries() + + #expect( + RecoveryDialogChoice.discard.snapshotsToDelete(from: entries) + == entries.map(\.0) + ) + } + + @Test("An empty sheet gives every choice nothing to unlink") + func anEmptySheetDeletesNothingWhateverIsChosen() { + let empty: [(UUID, RecoveryEntry)] = [] + + for choice in RecoveryDialogChoice.allCases { + #expect(choice.snapshotsToDelete(from: empty).isEmpty) + } + } + + @Test("Exactly one choice is allowed to unlink anything") + func onlyOneChoiceCanDelete() { + let entries = Self.makeEntries() + let deleters = RecoveryDialogChoice.allCases.filter { + !$0.snapshotsToDelete(from: entries).isEmpty + } + + #expect(deleters == [.discard]) + } + + // MARK: - Shortcut policy + + @Test("No destructive recovery choice carries a keyboard equivalent") + func destructiveChoicesCarryNoKeyboardShortcut() { + let destructive = RecoveryDialogChoice.allCases.filter(\.isDestructive) + + #expect( + !destructive.isEmpty, + "The sheet must still offer an irreversible discard" + ) + for choice in destructive { + #expect( + choice.keyboardShortcuts.isEmpty, + """ + \(choice.rawValue) destroys recovered work, so it must be \ + reachable only by a deliberate click (#1503) + """ + ) + } + } + + @Test("Discard is the only destructive choice") + func discardIsTheOnlyDestructiveChoice() { + #expect( + RecoveryDialogChoice.allCases.filter(\.isDestructive) == [.discard] + ) + } + + @Test("Escape and Return belong to exactly one safe choice each") + func reflexShortcutsBelongToSafeChoices() { + let owners: (KeyboardShortcut) -> [RecoveryDialogChoice] = { shortcut in + RecoveryDialogChoice.allCases.filter { + $0.keyboardShortcuts.contains(shortcut) + } + } + + #expect(owners(.cancelAction) == [.later]) + #expect(owners(.defaultAction) == [.recoverAll]) + #expect( + owners(KeyboardShortcut(".", modifiers: .command)) == [.later] + ) + } + + @Test("No two choices answer to the same keystroke") + func shortcutsAreUnique() { + let shortcuts = RecoveryDialogChoice.allCases + .flatMap(\.keyboardShortcuts) + + #expect(Set(shortcuts).count == shortcuts.count) + } + + @Test("The sheet keeps every choice it is supposed to offer") + func everyChoiceIsPresent() { + #expect( + Set(RecoveryDialogChoice.allCases) + == [.discard, .later, .recoverAll] + ) + } + + @Test("Only the destructive choice carries a VoiceOver hint") + func onlyTheDestructiveChoiceHasAHint() { + for choice in RecoveryDialogChoice.allCases { + #expect( + (choice.accessibilityHint != nil) == choice.isDestructive, + """ + \(choice.rawValue) disagrees with itself: the hint exists to \ + announce irreversibility, and `nil` rather than "" is what \ + keeps a safe button from carrying an empty announcement and \ + a lookup for a key the catalog does not have + """ + ) + } + + // Presence is not the claim. `isDestructive ? Strings.recoveryLater : + // nil` satisfies every assertion above and announces "Later" as the + // warning on the one irreversible control in this sheet. + #expect( + RecoveryDialogChoice.discard.accessibilityHint + == Strings.recoveryDiscardHint, + """ + The Discard hint is pointed at another catalog string. SwiftUI's \ + `ButtonRole.destructive` does not set AppKit's \ + `hasDestructiveAction`, so this hint is the only warning a \ + VoiceOver user gets before an unrecoverable click (#1503) + """ + ) + } + + @Test("The shortcut proxies stay out of the accessibility tree") + func theHostedSheetExposesExactlyThreeButtons() throws { + // ⌘-. rides an invisible 0×0 `Button` in the visible button's + // `.background`, because SwiftUI gives a control only one key + // equivalent. Nothing but `.accessibilityHidden(true)` keeps it out of + // the accessibility tree, and removing that modifier is a one-line + // edit no other test in this file can see: every behavioural + // assertion here goes through `performKeyEquivalent`, which the proxy + // answers either way. What it costs is a VoiceOver user hearing four + // buttons on a three-button sheet, two of them announced "Later" — on + // the one sheet in Pine where picking the wrong button is + // unrecoverable. + let hosted = Self.hostSheet() + defer { hosted.window.close() } + + let identifiers = try Self.accessibilityButtonIdentifiers( + under: hosted.window.contentView + ) + + #expect( + Set(identifiers) + == Set(RecoveryDialogChoice.allCases.map(\.accessibilityIdentifier)), + """ + The recovery sheet exposes \(identifiers.count) accessibility \ + buttons instead of \(RecoveryDialogChoice.allCases.count): \ + \(identifiers). An unidentified extra one is a shortcut proxy \ + that lost `.accessibilityHidden(true)` — VoiceOver would then \ + announce two buttons called "Later" on the sheet where the \ + neighbouring button is unrecoverable (#1503) + """ + ) + #expect( + identifiers.count == RecoveryDialogChoice.allCases.count, + "Duplicate accessibility identifiers: \(identifiers)" + ) + } + + @Test("The footer lays out every choice, destructive first, default last") + func theFooterDrawsEveryChoice() { + // The row used to name its three buttons one by one, so a fourth + // choice would have existed in the type, been reachable by nothing, + // and shown up nowhere. It is built by walking `allCases` now, and + // this is the assertion that keeps it that way. + let order = RecoveryDialogFooter.displayOrder + + #expect(Set(order) == Set(RecoveryDialogChoice.allCases)) + #expect(order.count == RecoveryDialogChoice.allCases.count) + #expect( + order.first?.isDestructive == true, + "The irreversible choice must stand apart on the leading edge" + ) + #expect( + order.last?.isDefaultAction == true, + "macOS puts the default action last; Return must land on it" + ) + #expect( + order.dropFirst().allSatisfy { !$0.isDestructive }, + "A destructive choice ended up in the trailing group: \(order)" + ) + } + + @Test("Every choice has its own accessibility identifier") + func accessibilityIdentifiersAreDistinct() { + let identifiers = RecoveryDialogChoice.allCases + .map(\.accessibilityIdentifier) + + #expect(Set(identifiers).count == identifiers.count) + #expect(identifiers.allSatisfy { !$0.isEmpty }) + } + + // MARK: - Localization + + @Test("Every locale gives the three choices three different titles") + func choiceTitlesAreDistinctInEveryLocale() throws { + let catalog = try Self.loadCatalog() + let keys = ["recovery.discard", "recovery.later", "recovery.recoverAll"] + + for locale in Self.supportedLocales { + let titles = try keys.map { key -> String in + try #require( + Self.value(for: key, locale: locale, in: catalog), + "Missing \(key) [\(locale)]" + ) + } + #expect( + Set(titles).count == keys.count, + """ + \(locale) gives two recovery choices the same title, so the \ + row has two buttons a user cannot tell apart: \(titles) + """ + ) + #expect(titles.allSatisfy { !$0.isEmpty }) + } + } + + @Test("The destructive button's title is pinned in every locale") + func discardTitleIsPinnedPerLocale() throws { + // What this enforces is a review gate, not a linguistic property: + // changing the word on the one irreversible control in this sheet has + // to cost a conversation with someone who speaks the language. There + // is no way to ask a string whether a native speaker hears deletion + // in it, and the pinned set below is not internally consistent about + // it either — German `Verwerfen`, Spanish and Brazilian Portuguese + // `Descartar` and Chinese `放弃` are all "discard/abandon" words, the + // same class as the Russian «Отклонить» and French « Ignorer » this + // branch replaced. They stay: `Verwerfen` is the platform-standard + // wording macOS itself uses for Don't Save, and matching the platform + // beats matching a rule invented here. The ones that were changed + // were changed because they read as *dismissing the dialog*, which is + // now a different button sitting right next to them (#1503). + let expected = [ + "de": "Verwerfen", + "en": "Discard", + "es": "Descartar", + "fr": "Supprimer", + "ja": "破棄", + "ko": "삭제", + "pt-BR": "Descartar", + "ru": "Удалить", + "zh-Hans": "放弃", + ] + let catalog = try Self.loadCatalog() + + #expect(Set(expected.keys) == Set(Self.supportedLocales)) + for (locale, title) in expected { + #expect( + Self.value(for: "recovery.discard", locale: locale, in: catalog) + == title, + """ + The Discard title changed in \(locale). It is the only \ + irreversible control on this sheet and it sits next to \ + Later — confirm with a native speaker that the new word \ + cannot be read as "just close this", then update this \ + pin (#1503) + """ + ) + } + } + + @Test("Each choice's button title is its own catalog string") + func choiceTitlesComeFromTheirOwnKeys() throws { + // Nothing else in this suite reads `\.title`. Swapping two of them — + // + // case .discard: Strings.recoveryLater + // case .later: Strings.recoveryDiscard + // + // compiles, keeps every other assertion in this file green, and puts + // "Later" on the irreversible button and "Discard" on the safe one. + // That is #1503's harm reached with the mouse instead of the keyboard, + // and it is worse than the original: the shortcut policy is intact, so + // the user who presses Escape is fine and the one who reads the row is + // the one who loses their work. + #expect(RecoveryDialogChoice.discard.title == Strings.recoveryDiscard) + #expect(RecoveryDialogChoice.later.title == Strings.recoveryLater) + #expect( + RecoveryDialogChoice.recoverAll.title == Strings.recoveryRecoverAll + ) + + // The identity check above is blind to the other half of the same + // swap — done inside `Strings`, where `recoveryDiscard` is pointed at + // "recovery.later" — and so is `discardTitleIsPinnedPerLocale`, which + // reads the catalog by literal key rather than through `Strings`. + // Rendering closes the loop: `Text(choice.title)` and a + // `Text(verbatim:)` of the value the catalog holds under the expected + // key lay out to the same width, and to different widths otherwise. + // Same technique, and same tolerance, as + // `localeSubstitutionChangesWhatIsMeasured`. + // + // It is a width comparison, so it can only see a substitution that + // changes the width. Two titles that happen to typeset alike in one + // locale (Japanese 破棄 against 後で) would hide a swap *there* — but + // the swap is one edit affecting every locale at once, and «Удалить» + // against «Позже» is not a close call. + let catalog = try Self.loadCatalog() + let keys: [RecoveryDialogChoice: String] = [ + .discard: "recovery.discard", + .later: "recovery.later", + .recoverAll: "recovery.recoverAll", + ] + #expect( + Set(keys.keys) == Set(RecoveryDialogChoice.allCases), + "A choice was added to the enum and has no expected title here" + ) + + for locale in Self.supportedLocales { + for choice in RecoveryDialogChoice.allCases { + let key = try #require(keys[choice]) + let expected = try #require( + Self.value(for: key, locale: locale, in: catalog), + "Missing \(key) [\(locale)]" + ) + let rendered = Self.measuredWidth( + of: Text(choice.title), + locale: locale + ) + let fromCatalog = Self.measuredWidth( + of: Text(verbatim: expected), + locale: locale + ) + #expect( + abs(rendered - fromCatalog) <= Self.typesettingTolerance, + """ + The \(choice.rawValue) button in \(locale) does not render \ + \(key)'s value "\(expected)" — \(rendered)pt against \ + \(fromCatalog)pt. Its title has been pointed at another \ + string, and on this sheet that means a button whose label \ + is not what pressing it does (#1503) + """ + ) + } + } + } + + @Test("The destructive button carries a warning hint in every locale") + func discardHintIsLocalizedEverywhere() throws { + // SwiftUI's `ButtonRole.destructive` does not set AppKit's + // `hasDestructiveAction`, so this hint is the only thing telling a + // VoiceOver user that Discard is not an ordinary button. + let catalog = try Self.loadCatalog() + + for locale in Self.supportedLocales { + let hint = Self.value( + for: "recovery.discardHint", + locale: locale, + in: catalog + ) + #expect(hint?.isEmpty == false, "Missing discard hint [\(locale)]") + } + } + + @Test("The discard hint says the saved files are safe") + func discardHintDoesNotThreatenTheUsersFiles() throws { + // This is the one string whose job is to warn a blind user before an + // irreversible action, and it used to say Discard "permanently deletes + // the recovered files" — which reads as the sources in the project. + // Discard deletes snapshots of unsaved changes and touches nothing on + // disk that the user saved; `recovery.message` in the same sheet says + // so correctly, and the two must not disagree about what is at stake. + let catalog = try Self.loadCatalog() + // Second clause, in each language's own words for "saved" and "not". + let reassurance = [ + "de": "gespeicherten", + "en": "saved files are not affected", + "es": "guardados", + "fr": "enregistrés", + "ja": "保存済み", + "ko": "저장된 파일", + "pt-BR": "salvos", + "ru": "Сохранённые файлы", + "zh-Hans": "已保存的文件", + ] + + for locale in Self.supportedLocales { + let hint = try #require( + Self.value( + for: "recovery.discardHint", + locale: locale, + in: catalog + ) + ) + let needle = try #require(reassurance[locale]) + #expect( + hint.contains(needle), + """ + The discard hint in \(locale) no longer tells the user their \ + saved files survive: \(hint) + """ + ) + } + } + + @Test("The retention footnote is localized for every supported locale") + func retentionNoticeIsLocalizedEverywhere() throws { + // "Later" is a bounded promise: the launch sweep collects undecided + // snapshots after `staleEntryRetentionDays`. The sheet says so, so the + // string has to exist wherever the sheet can be shown. + let catalog = try Self.loadCatalog() + let entry = try #require( + catalog["recovery.retentionNotice %lld"] as? [String: Any] + ) + let localizations = try #require( + entry["localizations"] as? [String: Any] + ) + + for locale in Self.supportedLocales { + let localization = try #require( + localizations[locale] as? [String: Any], + "Missing retention notice [\(locale)]" + ) + let substitutions = try #require( + localization["substitutions"] as? [String: Any] + ) + #expect( + substitutions["days"] != nil, + "Retention notice in \(locale) does not substitute the count" + ) + } + #expect(RecoveryManager.staleEntryRetentionDays > 0) + } + + @Test("The retention footnote the sheet renders is the catalog's") + func theRenderedRetentionNoticeResolves() throws { + // Everything above reads the catalog and never asks whether the sheet + // can reach it. The key is built by interpolation — + // `"recovery.retentionNotice \(days)"` — so its *format specifier* is + // part of its name: changing the parameter to `Double` makes the key + // `recovery.retentionNotice %lf`, and renaming the catalog entry does + // the same from the other side. Either way the footnote prints the + // literal "recovery.retentionNotice 7" and both + // `retentionNoticeIsLocalizedEverywhere` (which reads the file) and + // `theSheetStatesTheRetentionWindow` (which greps the source) stay + // green while this branch's central promise — that "Later" is a + // bounded window the user can read — becomes a raw key on screen. + // + // Same technique and tolerance as + // `localeSubstitutionChangesWhatIsMeasured`: a resolved + // `Text(LocalizedStringKey)` and a `Text(verbatim:)` of the same + // characters lay out to the same width, an unresolved key does not. + let catalog = try Self.loadCatalog() + let days = 7 + + for locale in Self.supportedLocales { + let expected = try Self.retentionNotice( + days: days, + locale: locale, + in: catalog + ) + let rendered = Self.measuredNoticeWidth( + Text(Strings.recoveryRetentionNotice(days: days)), + locale: locale + ) + let fromCatalog = Self.measuredNoticeWidth( + Text(verbatim: expected), + locale: locale + ) + + #expect( + abs(rendered - fromCatalog) <= Self.typesettingTolerance, + """ + The retention footnote in \(locale) does not render the \ + catalog's "\(expected)" — \(rendered)pt against \ + \(fromCatalog)pt. The lookup did not resolve, so the sheet is \ + printing a raw key where it promises the user how long \ + "Later" lasts (#1503) + """ + ) + } + } + + @Test("A wrong retention key is something the render check can see") + func theRenderedRetentionNoticeCheckHasTeeth() throws { + // The positive control for the test above: an unresolved key is not + // within a couple of points of the sentence it should have produced. + // Without this, a measurement that silently returned the same number + // for everything would make that test pass forever. + let catalog = try Self.loadCatalog() + let expected = try Self.retentionNotice( + days: 7, + locale: "en", + in: catalog + ) + // What SwiftUI draws for a key the catalog does not contain: the key + // itself. This is exactly what `%lf` or a renamed entry produces. + let unresolved = Self.measuredNoticeWidth( + Text(LocalizedStringKey("recovery.retentionNotice \(7.0)")), + locale: "en" + ) + let fromCatalog = Self.measuredNoticeWidth( + Text(verbatim: expected), + locale: "en" + ) + + #expect( + abs(unresolved - fromCatalog) > Self.typesettingTolerance, + """ + A key that resolves to nothing measured the same as the resolved \ + footnote (\(unresolved)pt against \(fromCatalog)pt), so \ + `theRenderedRetentionNoticeResolves` cannot tell them apart either + """ + ) + } + + // MARK: - Layout + + @Test("The footer fits inside the width the sheet actually takes") + func footerFitsTheSheetItGets() { + // Measured against the sheet's own fitting width, not a hardcoded + // number: the point is that the two agree, whatever they are. + for identifier in Self.supportedLocales { + let footer = Self.measuredFooterWidth(locale: identifier) + let sheet = Self.measuredSheetWidth(locale: identifier) + + #expect( + footer > 100, + "Footer measured \(footer)pt in \(identifier) — it did not lay out" + ) + #expect( + sheet >= footer + Self.sheetPadding, + """ + The three buttons need \(footer)pt plus \ + \(Self.sheetPadding)pt of padding in \(identifier), but the \ + sheet only takes \(sheet)pt — the row is being squeezed + """ + ) + } + } + + @Test("The sheet grows for a footer that outgrows its resting width") + func theSheetGrowsRatherThanSqueezingTheFooter() { + // German already asks for 349 of the 352pt a 400pt sheet can give the + // button row, so the headroom is three points. `.controlSize` stands + // in here for the thing that will actually consume it: a change in + // AppKit's button metrics on a newer macOS. A fixed-width sheet + // answers that by truncating a button label; this one widens. + var grew = false + + for identifier in Self.supportedLocales { + let footer = Self.measuredFooterWidth( + locale: identifier, + controlSize: .extraLarge + ) + let sheet = Self.measuredSheetWidth( + locale: identifier, + controlSize: .extraLarge + ) + + #expect( + sheet >= footer + Self.sheetPadding, + """ + With larger controls the footer needs \(footer)pt in \ + \(identifier) and the sheet stopped at \(sheet)pt — a fixed \ + width would clip the button labels here (#1503) + """ + ) + if sheet > Self.restingSheetWidth { grew = true } + } + + #expect( + grew, + """ + No locale pushed the sheet past its resting width even with \ + oversized controls, so this test can no longer tell a growing \ + sheet from a fixed one + """ + ) + } + + @Test("The sheet keeps its resting width at the normal control size") + func theSheetRestsAtItsDesignedWidth() { + // A band, not an equality. `minWidth` was chosen over `width` + // precisely so a locale that needs a few more points can have them, + // and pinning the exact number would turn that flexibility into a + // failing test the first time it is used. The ceiling is what the + // test is really for: it separates "a translation grew" from "a width + // cap was removed and some row is now driving the sheet to a thousand + // points", which is the failure mode the caps in `body` exist to stop. + for identifier in Self.supportedLocales { + let width = Self.measuredSheetWidth(locale: identifier) + + #expect( + width >= Self.restingSheetWidth, + "The sheet shrank below its `minWidth` in \(identifier): \(width)pt" + ) + #expect( + width <= Self.restingSheetWidth + Self.widthHeadroom, + """ + The sheet wants \(width)pt in \(identifier), more than \ + \(Self.restingSheetWidth + Self.widthHeadroom)pt — something \ + inside it is no longer capped to the content width + """ + ) + } + } + + @Test("A pathological file name is truncated, not laid out in full") + func aVeryLongFileNameIsTruncatedRatherThanWidening() { + // Generated bundles, downloads and dated exports routinely produce + // names well past a hundred characters, and a `Text` has no natural + // width to stop at. + // + // Measured on the row and not on the sheet, deliberately: a `List` is + // a scroll view, so it absorbs a row that asks for a thousand points + // and the sheet's fitting size never changes. A test that watched the + // sheet would pass whether the row was capped or not — it would look + // like a regression test and assert nothing. What actually goes wrong + // without the cap is inside the list: a name clipped mid-word with no + // ellipsis, and the relative timestamp pushed out of view. + // + // Width alone does not say "truncated": `.frame(maxWidth:)` on its own + // satisfies it, and a row that lost `.lineLimit(1)` still reports + // 352pt — an unconstrained `fittingSize` proposes nothing, so the + // `Text` lays out on one ideal-width line and the frame merely clamps + // the number that comes back. The height has to be measured under a + // proposal the row cannot ignore, so it is taken again inside a + // container fixed at the text column: there the name either fits on + // one line or reflows onto a dozen, and only the line limit decides + // which. What neither measurement can see is the truncation *mode* — + // a `.tail` ellipsis lays out exactly like a `.middle` one — so + // keeping the file extension visible stays a reviewed choice rather + // than a checked one. + let long = String(repeating: "extremely-long-generated-name-", count: 8) + let ceiling = RecoveryDialogView.contentWidth + Self.typesettingTolerance + let entries = [ + RecoveryEntry( + originalPath: "/tmp/project/\(long).swift", + content: "unsaved" + ), + RecoveryEntry( + originalPath: "", + untitledName: long, + content: "draft" + ), + ] + let short = RecoveryEntry(originalPath: "/tmp/a.swift", content: "x") + + for identifier in Self.supportedLocales { + let singleLine = Self.heightInTextColumn( + of: short, + locale: identifier + ) + + for entry in entries { + let width = Self.measuredWidth( + of: RecoveryEntryRow(entry: entry), + locale: identifier + ) + let height = Self.heightInTextColumn( + of: entry, + locale: identifier + ) + + #expect( + width <= ceiling, + """ + A \(long.count)-character name made the row ask for \ + \(width)pt in \(identifier), past the \(ceiling)pt \ + text column — the row lost its width cap + """ + ) + #expect( + abs(height - singleLine) <= Self.typesettingTolerance, + """ + Given the \(RecoveryDialogView.contentWidth)pt text \ + column, a \(long.count)-character name made the row \ + \(height)pt tall in \(identifier) against \ + \(singleLine)pt for a short name — the name is reflowing \ + onto more lines instead of being truncated on one, so the \ + row lost its line limit + """ + ) + } + } + + // …and the sheet itself still rests where it should with them in it. + let sheetEntries = entries.map { (UUID(), $0) } + #expect( + Self.measuredSheetWidth(locale: "en", entries: sheetEntries) + <= Self.restingSheetWidth + Self.widthHeadroom + ) + } + + @Test("Locale substitution actually reaches the rendered footer") + func localeSubstitutionChangesWhatIsMeasured() throws { + // Without a positive control, every measurement above could silently + // be the English one and the locale tests would pass for the wrong + // reason. Distinct widths is too weak on its own: eight locales could + // fall back to English and one could differ, and the set would still + // have two members. + // + // So each locale is checked against the value in the catalog. A + // rendered `Text(LocalizedStringKey)` and a `Text(verbatim:)` of the + // same characters lay out to the same width, and to a different one + // otherwise — which is exactly the question "did the lookup resolve + // to this locale's string, or did it fall back?". + let catalog = try Self.loadCatalog() + let widths = Self.supportedLocales.map { + Self.measuredFooterWidth(locale: $0) + } + + #expect( + Set(widths).count > 1, + """ + All nine locales measured the same footer width \(widths) — \ + `.environment(\\.locale, …)` is not reaching the button titles, \ + so the localized layout is not being tested at all + """ + ) + + for locale in Self.supportedLocales { + let expected = try #require( + Self.value(for: "recovery.later", locale: locale, in: catalog) + ) + let rendered = Self.measuredWidth( + of: Text(Strings.recoveryLater), + locale: locale + ) + let fromCatalog = Self.measuredWidth( + of: Text(verbatim: expected), + locale: locale + ) + // Within a point or two, not exactly: a localized `Text` carries + // the locale's typesetting language and a `Text(verbatim:)` does + // not, which moves Japanese by 1pt. The tolerance is far below the + // gap any fallback would open — "後で" against "Later" is tens of + // points — so it costs the test nothing it was there to catch. + #expect( + abs(rendered - fromCatalog) <= Self.typesettingTolerance, + """ + The Later button in \(locale) does not render the catalog's \ + "\(expected)" — \(rendered)pt rendered against \(fromCatalog)pt \ + for the catalog value. The lookup fell back to another locale, \ + so every measurement taken for \(locale) is measuring the \ + wrong string + """ + ) + } + } + + @Test("An empty entry list still lays the sheet out") + func hostedSheetSurvivesAnEmptyEntryList() { + let recorder = ChoiceRecorder() + let hosted = NSHostingView( + rootView: RecoveryDialogView(entries: []) { recorder.record($0) } + ) + hosted.frame = NSRect(x: 0, y: 0, width: 420, height: 520) + hosted.layoutSubtreeIfNeeded() + + #expect(recorder.chosen.isEmpty) + #expect(hosted.fittingSize.width == Self.restingSheetWidth) + // The list has a 100pt floor, so an empty sheet is still a real sheet + // and not a collapsed strip with three buttons in it. + #expect(hosted.fittingSize.height > 200) + } + + // MARK: - Helpers + + private static let supportedLocales = [ + "de", "en", "es", "fr", "ja", "ko", "pt-BR", "ru", "zh-Hans", + ] + + /// 24pt of padding on each edge, from `RecoveryDialogView.body`. + private static let sheetPadding: CGFloat = 48 + private static let restingSheetWidth: CGFloat = 400 + /// How much a shipped translation may add to the resting width before the + /// growth stops being a translation and starts being a missing cap. + private static let widthHeadroom: CGFloat = 40 + /// Slack between a localized `Text` and a verbatim one holding the same + /// characters. See `localeSubstitutionChangesWhatIsMeasured`. + private static let typesettingTolerance: CGFloat = 2 + + private final class HostedTestWindow: NSWindow { + override var canBecomeKey: Bool { true } + } + + private struct HostedSheet { + let window: NSWindow + let recorder: ChoiceRecorder + let onChoose: (RecoveryDialogChoice) -> Void + + /// Applies a choice the way the sheet's own callback does. The only + /// way to reach Discard from a test, because Discard is denied a key + /// equivalent on purpose and there is no click to send. + @MainActor + func resolve(_ choice: RecoveryDialogChoice) { + recorder.record(choice) + onChoose(choice) + } + } + + /// A real recovery directory with two snapshots in it. + private struct SnapshotFixture { + let directory: URL + let manager: RecoveryManager + let entries: [(UUID, RecoveryEntry)] + + /// The snapshot files actually on disk right now. + var files: Set { + let names = (try? FileManager.default.contentsOfDirectory( + atPath: directory.path + )) ?? [] + return Set(names.filter { $0.hasSuffix(".json") }) + } + } + + private static func withSnapshotFixture( + _ body: (SnapshotFixture) throws -> Void + ) throws { + let dir = try makeTempDir() + defer { removeTempDir(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + manager.snapshotDirtyTabs([ + dirtyTab(path: "/tmp/project/README.md"), + dirtyTab(path: "/tmp/project/notes.swift"), + ]) + let entries = manager.pendingRecoveryEntries() + #expect(entries.count == 2, "The fixture did not write its snapshots") + try body( + SnapshotFixture( + directory: dir, + manager: manager, + entries: entries + ) + ) + } + + /// Hosts the sheet wired to the fixture's manager through the production + /// seam: the choice decides what may be unlinked, and the call is made + /// whatever the choice is. + private static func hostResolvingSheet( + _ fixture: SnapshotFixture + ) -> HostedSheet { + hostSheet(entries: fixture.entries) { choice in + fixture.manager.deleteSnapshots( + withRecoveryIDs: choice.snapshotsToDelete( + from: fixture.entries + ) + ) + } + } + + private static func makeTempDir() throws -> URL { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent( + "PineRecoveryEscapeTests-\(UUID().uuidString)" + ) + try FileManager.default.createDirectory( + at: url, + withIntermediateDirectories: true + ) + return url + } + + private static func removeTempDir(_ url: URL) { + try? FileManager.default.removeItem(at: url) + } + + private static func dirtyTab(path: String) -> EditorTab { + EditorTab( + url: URL(fileURLWithPath: path), + content: "unsaved", + savedContent: "saved" + ) + } + + private static func hostSheet( + entries: [(UUID, RecoveryEntry)]? = nil, + locale: String = "en", + onChoose: @escaping (RecoveryDialogChoice) -> Void = { _ in } + ) -> HostedSheet { + let recorder = ChoiceRecorder() + let hosted = NSHostingView( + rootView: RecoveryDialogView( + entries: entries ?? makeEntries() + ) { + recorder.record($0) + onChoose($0) + } + .environment(\.locale, Locale(identifier: locale)) + ) + hosted.frame = NSRect(x: 0, y: 0, width: 420, height: 520) + // Mirrors `AgentHistoryUndoReviewHostedTests`: borderless and parked + // off screen so the window never flashes in front of whoever is + // running the suite, and `isReleasedWhenClosed = false` because the + // AppKit default would free it under the strong reference this test + // still holds. + let window = HostedTestWindow( + contentRect: hosted.frame, + styleMask: [.borderless], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + window.contentView = hosted + window.setFrameOrigin(NSPoint(x: -10_000, y: -10_000)) + window.makeKeyAndOrderFront(nil) + window.makeFirstResponder(hosted) + hosted.layoutSubtreeIfNeeded() + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.02)) + hosted.layoutSubtreeIfNeeded() + return HostedSheet( + window: window, + recorder: recorder, + onChoose: onChoose + ) + } + + private static func measuredWidth( + of view: some View, + locale: String + ) -> CGFloat { + measuredSize(of: view, locale: locale).width + } + + /// The row's height when it is handed exactly the text column it is + /// designed for. + /// + /// An unconstrained `fittingSize` proposes nothing, so a `Text` reports + /// one ideal-width line however long its content is and `maxWidth` only + /// clamps the number that comes back — a row that lost `.lineLimit(1)` + /// measures identically. Wrapping the row in a fixed-width container is + /// what forces the question the list will ask it in production. + private static func heightInTextColumn( + of entry: RecoveryEntry, + locale: String + ) -> CGFloat { + measuredSize( + of: RecoveryEntryRow(entry: entry) + .frame(width: RecoveryDialogView.contentWidth), + locale: locale + ).height + } + + private static func measuredSize( + of view: some View, + locale: String + ) -> NSSize { + let hosted = NSHostingView( + rootView: view + .environment(\.locale, Locale(identifier: locale)) + ) + hosted.layoutSubtreeIfNeeded() + return hosted.fittingSize + } + + private static func measuredFooterWidth( + locale: String, + controlSize: ControlSize = .regular + ) -> CGFloat { + let hosted = NSHostingView( + rootView: RecoveryDialogFooter(onChoose: { _ in }) + .controlSize(controlSize) + .environment(\.locale, Locale(identifier: locale)) + ) + hosted.layoutSubtreeIfNeeded() + return hosted.fittingSize.width + } + + private static func measuredSheetWidth( + locale: String, + controlSize: ControlSize = .regular, + entries: [(UUID, RecoveryEntry)]? = nil + ) -> CGFloat { + let hosted = NSHostingView( + rootView: RecoveryDialogView(entries: entries ?? makeEntries()) { _ in } + .controlSize(controlSize) + .environment(\.locale, Locale(identifier: locale)) + ) + hosted.frame = NSRect(x: 0, y: 0, width: 420, height: 520) + hosted.layoutSubtreeIfNeeded() + return hosted.fittingSize.width + } + + private static func sendReturn(to window: NSWindow) { + sendKey(to: window, characters: "\r", keyCode: 36) + } + + private static func sendEscape(to window: NSWindow) { + sendKey(to: window, characters: "\u{1B}", keyCode: 53) + } + + private static func sendCommandPeriod(to window: NSWindow) { + sendKey( + to: window, + characters: ".", + keyCode: 47, + modifierFlags: .command + ) + } + + private static func sendKey( + to window: NSWindow, + characters: String, + keyCode: UInt16, + modifierFlags: NSEvent.ModifierFlags = [] + ) { + guard let event = NSEvent.keyEvent( + with: .keyDown, + location: .zero, + modifierFlags: modifierFlags, + timestamp: ProcessInfo.processInfo.systemUptime, + windowNumber: window.windowNumber, + context: nil, + characters: characters, + charactersIgnoringModifiers: characters, + isARepeat: false, + keyCode: keyCode + ) else { + Issue.record("Could not construct a hosted key event") + return + } + if !window.performKeyEquivalent(with: event) { + window.sendEvent(event) + } + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.02)) + window.contentView?.layoutSubtreeIfNeeded() + } + + private static func makeEntries() -> [(UUID, RecoveryEntry)] { + [ + ( + UUID(), + RecoveryEntry( + originalPath: "/tmp/project/README.md", + content: "unsaved" + ) + ), + ( + UUID(), + RecoveryEntry( + originalPath: "", + untitledName: "Untitled 2", + content: "draft" + ) + ), + ] + } + + private static func repositoryRoot() -> URL { + URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() + .deletingLastPathComponent() + } + + private static func loadCatalog() throws -> [String: Any] { + let data = try Data( + contentsOf: repositoryRoot() + .appendingPathComponent("Pine/Localizable.xcstrings") + ) + let root = try #require( + JSONSerialization.jsonObject(with: data) as? [String: Any] + ) + return try #require(root["strings"] as? [String: Any]) + } + + /// Width of one retention footnote, with the typesetting language pinned. + /// + /// The footnote is a whole sentence, and in `ja` and `zh-Hans` a sentence + /// carries punctuation that the typesetter squeezes — 「」、。 and ，。 — + /// which a `Text(verbatim:)` does not get, because only a localized + /// `Text` picks up the locale's typesetting language. Left alone that is a + /// 33pt gap in Japanese between two strings that are character-for- + /// character identical, which has nothing to do with the question being + /// asked. Setting the language explicitly on both sides puts them in the + /// same typesetting regime, and the two-point tolerance then measures only + /// what it is meant to: did the catalog lookup resolve. + private static func measuredNoticeWidth( + _ text: Text, + locale: String + ) -> CGFloat { + measuredWidth( + of: text.typesettingLanguage( + .explicit(Locale.Language(identifier: locale)) + ), + locale: locale + ) + } + + /// Which plural category `7` selects, per supported locale. + /// + /// Spelled out rather than derived: Foundation exposes no public plural + /// rules, and the alternative — accepting whichever variation happens to + /// match — would stop the render check from noticing that the sheet picked + /// the wrong form. Russian is the one that is not `other`: 5…20 is `many`, + /// and `other` there is only reached by fractions. + private static let pluralCategoryForSeven = [ + "de": "other", + "en": "other", + "es": "other", + "fr": "other", + "ja": "other", + "ko": "other", + "pt-BR": "other", + "ru": "many", + "zh-Hans": "other", + ] + + /// The retention footnote as the catalog holds it, with the count already + /// substituted — the string the sheet must be rendering. + private static func retentionNotice( + days: Int, + locale: String, + in catalog: [String: Any] + ) throws -> String { + let entry = try #require( + catalog["recovery.retentionNotice %lld"] as? [String: Any], + "The retention notice is no longer keyed on an `%lld` count" + ) + let localizations = try #require(entry["localizations"] as? [String: Any]) + let localization = try #require( + localizations[locale] as? [String: Any], + "Missing retention notice [\(locale)]" + ) + let substitutions = try #require( + localization["substitutions"] as? [String: Any] + ) + let daysSubstitution = try #require( + substitutions["days"] as? [String: Any], + "The retention notice in \(locale) no longer substitutes `days`" + ) + let variations = try #require( + daysSubstitution["variations"] as? [String: Any] + ) + let plural = try #require(variations["plural"] as? [String: Any]) + let category = try #require( + pluralCategoryForSeven[locale], + "No expected plural category for \(locale)" + ) + let form = try #require( + plural[category] as? [String: Any], + """ + The retention notice in \(locale) has no "\(category)" plural \ + form, which is the one \(days) selects there + """ + ) + let unit = try #require(form["stringUnit"] as? [String: Any]) + let variationValue = try #require(unit["value"] as? String) + let outer = try #require( + (localization["stringUnit"] as? [String: Any])?["value"] as? String + ) + + return outer + .replacingOccurrences(of: "%#@days@", with: variationValue) + .replacingOccurrences(of: "%lld", with: String(days)) + } + + // MARK: - Reading the accessibility tree + // + // SwiftUI does not put its accessibility elements in the view hierarchy: + // under an `NSHostingView` they are instances of SwiftUI's own + // `AccessibilityNode`, reachable only through `accessibilityChildren()`, + // and they are built lazily — before any accessibility client has asked, + // the hosting view reports none at all. `AccessibilityNode` is an + // `NSObject` that answers the usual accessibility selectors but does not + // declare `NSAccessibilityProtocol` conformance, so Swift cannot call + // them directly; KVC can, guarded by `responds(to:)`. + // + // Neither is a hack for its own sake: this is the tree VoiceOver reads, + // and reading it any other way (subviews, focus order) would be asserting + // about something else. + + /// Makes AppKit materialise the accessibility tree for this process. + /// + /// Querying our *own* pid needs no TCC grant — the trust check gates + /// inspecting other processes — and it is the query itself that switches + /// accessibility on, after which the hosting view's children exist. + private static func awakenAccessibility() { + let application = AXUIElementCreateApplication(getpid()) + var children: CFTypeRef? + _ = AXUIElementCopyAttributeValue( + application, + kAXChildrenAttribute as CFString, + &children + ) + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) + } + + private static func accessibilityValue( + _ name: String, + of element: NSObject + ) -> Any? { + guard element.responds(to: Selector((name))) else { return nil } + return element.value(forKey: name) + } + + /// The accessibility identifiers of every element the tree under the + /// recovery sheet's own container calls a button. + /// + /// Scoped to the container rather than to the window so the count cannot + /// be padded by anything AppKit puts beside the sheet, and returning + /// identifiers rather than a number so a failure names what appeared. + private static func accessibilityButtonIdentifiers( + under root: NSView? + ) throws -> [String] { + guard let root else { return [] } + awakenAccessibility() + root.layoutSubtreeIfNeeded() + + let sheet = try #require( + descendant(of: root) { + accessibilityValue("accessibilityIdentifier", of: $0) as? String + == AccessibilityID.recoverySheet + }, + """ + The sheet's container element is not in the accessibility tree \ + under \(AccessibilityID.recoverySheet), so nothing below can be \ + counted + """ + ) + + var identifiers: [String] = [] + forEachDescendant(of: sheet) { element in + guard accessibilityValue("accessibilityRole", of: element) + as? String == NSAccessibility.Role.button.rawValue else { + return + } + identifiers.append( + accessibilityValue("accessibilityIdentifier", of: element) + as? String ?? "«unidentified»" + ) + } + return identifiers + } + + private static func descendant( + of root: NSObject, + where matches: (NSObject) -> Bool + ) -> NSObject? { + var result: NSObject? + forEachDescendant(of: root) { element in + if result == nil, matches(element) { result = element } + } + return result + } + + private static func forEachDescendant( + of root: NSObject, + _ body: (NSObject) -> Void + ) { + var stack: [NSObject] = [root] + var visited = 0 + + while let current = stack.popLast(), visited < 10_000 { + visited += 1 + body(current) + let children = accessibilityValue( + "accessibilityChildren", + of: current + ) as? [Any] ?? [] + for child in children { + guard let child = child as? NSObject else { continue } + stack.append(child) + } + } + } + + private static func value( + for key: String, + locale: String, + in catalog: [String: Any] + ) -> String? { + guard let entry = catalog[key] as? [String: Any], + let localizations = entry["localizations"] as? [String: Any], + let localization = localizations[locale] as? [String: Any], + let unit = localization["stringUnit"] as? [String: Any], + let value = unit["value"] as? String else { + return nil + } + return value + } + + @MainActor + private final class ChoiceRecorder { + private(set) var chosen: [RecoveryDialogChoice] = [] + + func record(_ choice: RecoveryDialogChoice) { + chosen.append(choice) + } + } +} diff --git a/PineTests/RecoveryManagerExtendedTests.swift b/PineTests/RecoveryManagerExtendedTests.swift index ceae45ac..5fdf3861 100644 --- a/PineTests/RecoveryManagerExtendedTests.swift +++ b/PineTests/RecoveryManagerExtendedTests.swift @@ -52,15 +52,16 @@ struct RecoveryManagerExtendedTests { #expect(manager.hasPendingRecovery == true) } - @Test func hasPendingRecovery_falseAfterDeleteAll() throws { + @Test func hasPendingRecovery_falseAfterSweepingTheOpenTab() throws { let dir = try makeTempDir() defer { cleanup(dir) } let manager = RecoveryManager(recoveryDirectory: dir) - manager.snapshotDirtyTabs([makeDirtyTab()]) + let tab = makeDirtyTab() + manager.snapshotDirtyTabs([tab]) #expect(manager.hasPendingRecovery == true) - manager.deleteAllRecoveryFiles() + manager.deleteSnapshotsOfOpenTabs([tab.id]) #expect(manager.hasPendingRecovery == false) } @@ -166,19 +167,29 @@ struct RecoveryManagerExtendedTests { try FileManager.default.createDirectory(at: projA, withIntermediateDirectories: true) try FileManager.default.createDirectory(at: projB, withIntermediateDirectories: true) - // Write old recovery entries in both + // Write old recovery entries in both. The modification date matches + // the timestamp because a single write sets both in production, and + // the sweep now settles fresh files by that date instead of decoding + // every snapshot on the main thread at launch (#1503). + let writtenAt = Date().addingTimeInterval(-10 * 24 * 3600) // 10 days ago let encoder = JSONEncoder() encoder.dateEncodingStrategy = .iso8601 let oldEntry = RecoveryEntry( originalPath: "/tmp/old.swift", content: "old", - timestamp: Date().addingTimeInterval(-10 * 24 * 3600), // 10 days ago + timestamp: writtenAt, encoding: .utf8 ) let data = try encoder.encode(oldEntry) - try data.write(to: projA.appendingPathComponent("\(UUID().uuidString).json")) - try data.write(to: projB.appendingPathComponent("\(UUID().uuidString).json")) + for dir in [projA, projB] { + let url = dir.appendingPathComponent("\(UUID().uuidString).json") + try data.write(to: url) + try FileManager.default.setAttributes( + [.modificationDate: writtenAt], + ofItemAtPath: url.path + ) + } // Verify both have entries let mgrA = RecoveryManager(recoveryDirectory: projA) @@ -202,15 +213,22 @@ struct RecoveryManagerExtendedTests { let encoder = JSONEncoder() encoder.dateEncodingStrategy = .iso8601 - // Old entry (10 days ago) + // Old entry (10 days ago), with the matching modification date a real + // write would have left behind (#1503). + let writtenAt = Date().addingTimeInterval(-10 * 24 * 3600) let oldEntry = RecoveryEntry( originalPath: "/tmp/old.swift", content: "old", - timestamp: Date().addingTimeInterval(-10 * 24 * 3600), + timestamp: writtenAt, encoding: .utf8 ) let oldID = UUID() - try encoder.encode(oldEntry).write(to: dir.appendingPathComponent("\(oldID.uuidString).json")) + let oldURL = dir.appendingPathComponent("\(oldID.uuidString).json") + try encoder.encode(oldEntry).write(to: oldURL) + try FileManager.default.setAttributes( + [.modificationDate: writtenAt], + ofItemAtPath: oldURL.path + ) // New entry (just now) let manager = RecoveryManager(recoveryDirectory: dir) @@ -228,17 +246,17 @@ struct RecoveryManagerExtendedTests { // MARK: - Edge cases - @Test func deleteAllRecoveryFiles_noOpOnEmptyDirectory() throws { + @Test func terminationSweep_noOpOnEmptyDirectory() throws { let dir = try makeTempDir() defer { cleanup(dir) } let manager = RecoveryManager(recoveryDirectory: dir) - manager.deleteAllRecoveryFiles() // Should not crash + manager.deleteSnapshotsOfOpenTabs([UUID()]) // Should not crash } - @Test func deleteAllRecoveryFiles_noOpOnNonExistentDirectory() { + @Test func terminationSweep_noOpOnNonExistentDirectory() { let dir = URL(fileURLWithPath: "/tmp/nonexistent-\(UUID().uuidString)") let manager = RecoveryManager(recoveryDirectory: dir) - manager.deleteAllRecoveryFiles() // Should not crash + manager.deleteSnapshotsOfOpenTabs([UUID()]) // Should not crash } @Test func cleanupStaleEntries_noOpOnNonExistentDirectory() { diff --git a/PineTests/RecoveryManagerTests.swift b/PineTests/RecoveryManagerTests.swift index e337f990..eb22428e 100644 --- a/PineTests/RecoveryManagerTests.swift +++ b/PineTests/RecoveryManagerTests.swift @@ -90,7 +90,20 @@ struct RecoveryManagerTests { #expect(entry.schemaVersion == RecoveryEntry.currentSchemaVersion) } - @Test func futureSchemaFailsClosedWithoutOverwrite() throws { + /// A newer schema stamp on a snapshot that still decodes is offered, and + /// the file is never rewritten (#1503). + /// + /// It used to be refused. That combination was the trap: this build can + /// read the buffer — `schemaVersion: 2` from a beta with nothing but + /// added fields decodes into today's `RecoveryEntry` with real content, a + /// real path and a real timestamp — but refused to show it, while the + /// launch sweep deleted it after + /// `unsupportedSchemaRetentionMultiplier` × the retention window. Work + /// the user was never allowed to decide about was destroyed on a schedule + /// nobody told them about, in exactly the downgrade scenario that + /// constant exists for. Reading it and hiding it are now the same + /// decision: if it decodes, it is offered. + @Test func futureSchemaIsOfferedWithoutOverwrite() throws { let dir = try makeTempDir() defer { cleanup(dir) } let manager = RecoveryManager(recoveryDirectory: dir) @@ -106,10 +119,40 @@ struct RecoveryManagerTests { let data = try encoder.encode(entry) try data.write(to: file) - #expect(manager.pendingRecoveryEntries().isEmpty) + let offered = manager.pendingRecoveryEntries() + + #expect(offered.count == 1) + #expect(offered.first?.0 == id) + #expect(offered.first?.1.content == "future contents") + #expect( + offered.first?.1.hasSupportedSchema == false, + """ + The stamp is still reported — it is what buys the longer \ + retention horizon in the sweep — it just no longer suppresses \ + the offer + """ + ) + // Still never written back: reading a newer build's snapshot must not + // downgrade it in place. #expect(try Data(contentsOf: file) == data) } + @Test func unreadableSchemaIsNotOffered() throws { + // The other half of the same rule, and the reason it is not simply + // "offer everything": a file this build cannot turn into content is + // not content it could have shown, so leaving it out of the offer + // takes no decision away from anyone. + let dir = try makeTempDir() + defer { cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let file = dir.appendingPathComponent("\(UUID().uuidString).json") + try Data(#"{"schemaVersion": 2, "renamedField": "x"}"#.utf8) + .write(to: file) + + #expect(manager.pendingRecoveryEntries().isEmpty) + #expect(FileManager.default.fileExists(atPath: file.path)) + } + @Test func legacySnapshotWithoutSchemaRemainsReadable() throws { let dir = try makeTempDir() defer { cleanup(dir) } @@ -341,7 +384,7 @@ struct RecoveryManagerTests { Set([cancelledCrashTab.id, recoveredTab.id]) ) - manager.deleteRecoveryFiles(for: retained.map(\.0)) + manager.deleteSnapshots(withRecoveryIDs: retained.map(\.0)) #expect( manager.pendingRecoveryEntries().map(\.0) == @@ -668,21 +711,22 @@ struct RecoveryManagerTests { #expect(manager.pendingRecoveryEntries().isEmpty) } - // MARK: - Delete all + // MARK: - Clean-quit sweep - @Test func deleteAllRecoveryFilesRemovesEverything() throws { + @Test func terminationSweepRemovesEveryOpenTabSnapshot() throws { let dir = try makeTempDir() defer { cleanup(dir) } let manager = RecoveryManager(recoveryDirectory: dir) - manager.snapshotDirtyTabs([ + let tabs = [ makeDirtyTab(content: "a"), makeDirtyTab(content: "b"), makeDirtyTab(content: "c") - ]) + ] + manager.snapshotDirtyTabs(tabs) #expect(manager.pendingRecoveryEntries().count == 3) - manager.deleteAllRecoveryFiles() + manager.deleteSnapshotsOfOpenTabs(tabs.map(\.id)) #expect(manager.pendingRecoveryEntries().isEmpty) } @@ -723,11 +767,16 @@ struct RecoveryManagerTests { defer { cleanup(dir) } let manager = RecoveryManager(recoveryDirectory: dir) - // Create a recovery file manually with old timestamp + // Create a recovery file manually with an old timestamp. The + // modification date is set to match: one write sets both in + // production, and since #1503 the sweep uses the modification date as + // a fast path so it does not have to decode a megabyte of unsaved + // buffer on the main thread at launch just to read one date. + let writtenAt = Date().addingTimeInterval(-8 * 24 * 3600) // 8 days ago let oldEntry = RecoveryEntry( originalPath: "/tmp/old.swift", content: "old content", - timestamp: Date().addingTimeInterval(-8 * 24 * 3600), // 8 days ago + timestamp: writtenAt, encoding: .utf8 ) let oldID = UUID() @@ -736,6 +785,10 @@ struct RecoveryManagerTests { let data = try encoder.encode(oldEntry) let filePath = dir.appendingPathComponent("\(oldID.uuidString).json") try data.write(to: filePath, options: .atomic) + try FileManager.default.setAttributes( + [.modificationDate: writtenAt], + ofItemAtPath: filePath.path + ) #expect(manager.pendingRecoveryEntries().count == 1) @@ -919,7 +972,7 @@ struct RecoveryManagerTests { #expect(managerA.pendingRecoveryEntries()[0].1.content == "from project A") #expect(managerB.pendingRecoveryEntries()[0].1.content == "from project B") - managerA.deleteAllRecoveryFiles() + managerA.deleteSnapshotsOfOpenTabs([tabA.id]) #expect(managerA.pendingRecoveryEntries().isEmpty) #expect(managerB.pendingRecoveryEntries().count == 1) } diff --git a/PineTests/RecoveryTerminationSweepTests.swift b/PineTests/RecoveryTerminationSweepTests.swift new file mode 100644 index 00000000..9ae205a7 --- /dev/null +++ b/PineTests/RecoveryTerminationSweepTests.swift @@ -0,0 +1,1227 @@ +// +// RecoveryTerminationSweepTests.swift +// PineTests +// +// #1503: Escape in the crash-recovery sheet now means "later", not "delete". +// "Later" is only honest if the snapshots survive the clean-quit sweep that +// `applicationWillTerminate` runs when nothing is unsaved — and the sweep +// used to empty the whole recovery directory, so quitting with the sheet on +// screen destroyed the very files it was offering. The sweep is now scoped to +// the snapshots belonging to the session's open tabs; these tests pin that +// boundary, from both sides. +// + +import AppKit +import Foundation +import Testing + +@testable import Pine + +// `.serialized`: several tests here drive a real `AppDelegate` against a real +// `ProjectRegistry`, and one of them is `async` — without serialisation its +// suspension is a window in which another test's delegate can touch the same +// process-global singletons. +@Suite( + "The clean-quit sweep only takes what this session owns", + .serialized +) +@MainActor +struct RecoveryTerminationSweepTests { + + // MARK: - Core contract + + @Test("Crash snapshots nobody decided about survive the sweep") + func undecidedSnapshotsSurviveTheSweep() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + manager.snapshotDirtyTabs([ + Self.dirtyTab(path: "/tmp/a.swift"), + Self.dirtyTab(path: "/tmp/b.swift"), + ]) + let ids = Set(manager.pendingRecoveryEntries().map(\.0)) + #expect(ids.count == 2) + + // A crash snapshot belongs to no open tab — that is exactly what makes + // it a crash snapshot. Quitting cleanly must leave it alone. + manager.deleteSnapshotsOfOpenTabs([]) + + #expect(Set(manager.pendingRecoveryEntries().map(\.0)) == ids) + } + + @Test("Snapshots of this session's open tabs are swept") + func openTabSnapshotsAreSwept() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let tabs = [ + Self.dirtyTab(path: "/tmp/a.swift"), + Self.dirtyTab(path: "/tmp/b.swift"), + ] + manager.snapshotDirtyTabs(tabs) + #expect(manager.pendingRecoveryEntries().count == 2) + + manager.deleteSnapshotsOfOpenTabs(tabs.map(\.id)) + + #expect(manager.pendingRecoveryEntries().isEmpty) + } + + @Test("The sweep takes the tabs it is given and nothing beside them") + func theSweepIsScopedToTheTabsItIsGiven() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let openTab = Self.dirtyTab(path: "/tmp/open.swift") + manager.snapshotDirtyTabs([openTab]) + let crashTab = Self.dirtyTab(path: "/tmp/from-the-crash.swift") + manager.snapshotDirtyTabs([crashTab]) + #expect(manager.pendingRecoveryEntries().count == 2) + + manager.deleteSnapshotsOfOpenTabs([openTab.id]) + + #expect(manager.pendingRecoveryEntries().map(\.0) == [crashTab.id]) + } + + @Test("A superseded crash snapshot leaves with the tab that replaced it") + func aSupersededSnapshotGoesWithItsOpenTab() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let crashTab = Self.dirtyTab(path: "/tmp/crashed.swift") + manager.snapshotDirtyTabs([crashTab]) + let runtimeTab = Self.dirtyTab(path: "/tmp/crashed.swift") + + // Block the destination so the migration cannot write the runtime + // snapshot: the old crash file stays on disk and is recorded against + // the runtime tab instead of being orphaned. + let blocked = dir.appendingPathComponent("\(runtimeTab.id.uuidString).json") + try FileManager.default.createDirectory( + at: blocked, + withIntermediateDirectories: false + ) + #expect( + manager.migrateRecoverySnapshot( + from: crashTab.id, + to: runtimeTab + ) == false + ) + try FileManager.default.removeItem(at: blocked) + #expect(manager.pendingRecoveryEntries().map(\.0) == [crashTab.id]) + + // Sweeping the runtime tab must take the crash file it inherited… + manager.deleteSnapshotsOfOpenTabs([runtimeTab.id]) + #expect(manager.pendingRecoveryEntries().isEmpty) + } + + @Test("A superseded crash snapshot stays when its tab is not swept") + func aSupersededSnapshotStaysWithoutItsOpenTab() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let crashTab = Self.dirtyTab(path: "/tmp/crashed.swift") + manager.snapshotDirtyTabs([crashTab]) + let runtimeTab = Self.dirtyTab(path: "/tmp/crashed.swift") + let blocked = dir.appendingPathComponent("\(runtimeTab.id.uuidString).json") + try FileManager.default.createDirectory( + at: blocked, + withIntermediateDirectories: false + ) + _ = manager.migrateRecoverySnapshot(from: crashTab.id, to: runtimeTab) + try FileManager.default.removeItem(at: blocked) + + // …and only then. A sweep that was handed no tabs takes nothing. + manager.deleteSnapshotsOfOpenTabs([]) + #expect(manager.pendingRecoveryEntries().map(\.0) == [crashTab.id]) + } + + @Test("Entries the restorer could not restore survive the sweep") + func retainedEntriesSurviveTheSweep() async throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + + // Two crash snapshots: one restores, one the user backs out of at the + // large-file prompt. The second is the clearest case of "undecided" — + // the user asked to recover and did not get to finish — so the sweep + // must not finish the job by deleting it. + let restoredURL = dir.appendingPathComponent("restored.swift") + try "on disk".write(to: restoredURL, atomically: true, encoding: .utf8) + let cancelledURL = dir.appendingPathComponent("cancelled.swift") + let huge = String( + repeating: "c", + count: TabManager.largeFileThreshold + 1 + ) + try huge.write(to: cancelledURL, atomically: true, encoding: .utf8) + let restoredCrashTab = EditorTab( + url: restoredURL, + content: "recovered", + savedContent: "on disk" + ) + let cancelledCrashTab = EditorTab( + url: cancelledURL, + content: "still undecided", + savedContent: huge + ) + manager.snapshotDirtyTabs([restoredCrashTab, cancelledCrashTab]) + + let tabManager = TabManager() + tabManager.largeFileAlertPresenter = { _, _, _ in .abort } + let retained = await manager.restorePendingEntries( + manager.pendingRecoveryEntries(), + in: tabManager, + context: .unscoped + ) + #expect(retained.map { $0.0 } == [cancelledCrashTab.id]) + + let openTabIDs: [UUID] = tabManager.tabs.map(\.id) + manager.deleteSnapshotsOfOpenTabs(openTabIDs) + + #expect( + manager.pendingRecoveryEntries().map(\.0) + == [cancelledCrashTab.id] + ) + } + + // MARK: - Repetition and degenerate input + + @Test("Sweeping twice is the same as sweeping once") + func theSweepIsIdempotent() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let openTab = Self.dirtyTab(path: "/tmp/open.swift") + let crashTab = Self.dirtyTab(path: "/tmp/crash.swift") + manager.snapshotDirtyTabs([openTab]) + manager.snapshotDirtyTabs([crashTab]) + + for _ in 0..<3 { + manager.deleteSnapshotsOfOpenTabs([openTab.id, openTab.id]) + #expect(manager.pendingRecoveryEntries().map(\.0) == [crashTab.id]) + } + } + + @Test("Sweeping IDs with no file on disk destroys nothing") + func sweepingUnknownIDsDestroysNothing() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + manager.snapshotDirtyTabs([Self.dirtyTab(path: "/tmp/a.swift")]) + let ids = manager.pendingRecoveryEntries().map(\.0) + + manager.deleteSnapshotsOfOpenTabs([UUID(), UUID()]) + + #expect(manager.pendingRecoveryEntries().map(\.0) == ids) + } + + @Test("The sweep leaves files that are not recovery snapshots") + func theSweepLeavesForeignFiles() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let openTab = Self.dirtyTab(path: "/tmp/open.swift") + manager.snapshotDirtyTabs([openTab]) + let junk = dir.appendingPathComponent("not-a-uuid.json") + try Data("{}".utf8).write(to: junk) + let sidecar = dir.appendingPathComponent("notes.txt") + try Data("keep me".utf8).write(to: sidecar) + + manager.deleteSnapshotsOfOpenTabs([openTab.id]) + + // Neither was ever this session's to delete. The stale sweep collects + // orphaned JSON — including the undecodable kind, which used to be + // immortal — once the filesystem agrees it is old enough; nothing + // collects a foreign extension. + #expect(FileManager.default.fileExists(atPath: junk.path)) + #expect(FileManager.default.fileExists(atPath: sidecar.path)) + } + + @Test("A sweep against a missing directory does not crash") + func theSweepToleratesAMissingDirectory() { + let missing = URL( + fileURLWithPath: "/tmp/pine-missing-\(UUID().uuidString)" + ) + let manager = RecoveryManager(recoveryDirectory: missing) + + manager.deleteSnapshotsOfOpenTabs([UUID()]) + + #expect(manager.pendingRecoveryEntries().isEmpty) + } + + @Test("One project's sweep cannot reach another project's snapshots") + func theSweepIsScopedToItsOwnProject() throws { + let dirA = try Self.makeTempDir() + let dirB = try Self.makeTempDir() + defer { + Self.cleanup(dirA) + Self.cleanup(dirB) + } + let managerA = RecoveryManager(recoveryDirectory: dirA) + let managerB = RecoveryManager(recoveryDirectory: dirB) + let tabA = Self.dirtyTab(path: "/tmp/a.swift") + managerA.snapshotDirtyTabs([tabA]) + managerB.snapshotDirtyTabs([Self.dirtyTab(path: "/tmp/b.swift")]) + + managerB.deleteSnapshotsOfOpenTabs([tabA.id]) + + #expect(managerA.pendingRecoveryEntries().count == 1) + #expect(managerB.pendingRecoveryEntries().count == 1) + } + + // MARK: - The line in `applicationWillTerminate` itself + + @Test("Quitting cleanly takes this session's snapshots and keeps the crash ones") + func cleanTerminationSweepsOnlyTheOpenTabs() throws { + // Everything above drives `deleteSnapshotsOfOpenTabs(_:)` directly. + // That leaves the one production line that decides what a clean quit + // destroys — the `pm.allTabs.map(\.id)` in + // `AppDelegate.applicationWillTerminate` — executed by nothing. A + // plausible tidy-up: + // + // pm.recoveryManager?.deleteSnapshotsOfOpenTabs( + // (pm.recoveryManager?.pendingRecoveryEntries() ?? []).map(\.0) + // ) + // + // compiles, reads like a simplification, and restores the original + // bug in full: quitting with the recovery sheet on screen unlinks + // every snapshot it was offering. So this drives the real delegate. + // + // Two open tabs, in two different panes, because the line under test + // says `pm.allTabs` and a single tab in `primaryTabManager` cannot + // tell that apart from `pm.primaryTabManager.tabs`. With the split, + // the narrower spelling leaves the second pane's snapshot on disk — + // a file this session is answerable for, coming back on the next + // launch as a "recovered file" for a buffer that was saved. + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + let manager = try #require(project.recoveryManager) + + // A snapshot belonging to a tab that is open and — by the time we + // quit — saved. This session is answerable for it. + let file = dir.appendingPathComponent("open.swift") + try "original".write(to: file, atomically: true, encoding: .utf8) + project.primaryTabManager.autoSavePreferenceProvider = { false } + project.primaryTabManager.openTab(url: file) + project.primaryTabManager.updateContent("modified") + + // …and the same thing again in a second editor pane. + let split = dir.appendingPathComponent("split.swift") + try "original".write(to: split, atomically: true, encoding: .utf8) + let secondPaneID = try #require( + project.paneManager.splitPane( + project.paneManager.activePaneID, + axis: .horizontal + ) + ) + let secondPane = try #require( + project.paneManager.tabManager(for: secondPaneID) + ) + secondPane.autoSavePreferenceProvider = { false } + secondPane.openTab(url: split) + secondPane.updateContent("modified") + + manager.snapshotDirtyTabs(project.allTabs) + project.primaryTabManager.updateContent("original") + secondPane.updateContent("original") + let openTabIDs = Set(project.allTabs.map(\.id)) + #expect( + openTabIDs.count == 2, + "The fixture needs a tab in each pane for `allTabs` to matter" + ) + #expect( + project.primaryTabManager.tabs.count == 1, + """ + Both tabs ended up in the primary pane, so this test can no \ + longer see the difference between `allTabs` and \ + `primaryTabManager.tabs` + """ + ) + + // …and a snapshot from the crash, belonging to no tab at all. + let crashed = Self.crashedTab(in: dir) + manager.snapshotDirtyTabs([crashed]) + + #expect(!project.hasUnsavedChanges, "The clean-quit branch must run") + #expect( + Set(manager.pendingRecoveryEntries().map(\.0)) + == openTabIDs.union([crashed.id]) + ) + + let delegate = AppDelegate() + delegate.registry = registry + delegate.applicationWillTerminate( + Notification(name: NSApplication.willTerminateNotification) + ) + + #expect( + manager.pendingRecoveryEntries().map(\.0) == [crashed.id], + """ + A clean quit did not leave exactly the undecided crash snapshot \ + behind. Deleting it is #1503: the user quits Pine with the \ + recovery sheet open, or after closing it with Escape, and the \ + work it was offering is gone (#1503) + """ + ) + } + + @Test("Quitting with unsaved work leaves every snapshot alone") + func terminationWithUnsavedWorkSweepsNothing() throws { + // The other side of the same `if`: the sweep is gated on the session + // having nothing unsaved, and a dirty tab's snapshot is the crash + // protection for work that is still on screen. + // + // The unsaved buffer is deliberately in the *second* pane, with the + // primary holding a saved one. Both the gate (`hasUnsavedChanges`) and + // the sweep's argument (`pm.allTabs`) walk every pane, and with a + // single tab in `primaryTabManager` a narrowing of either to the + // primary pane is invisible: here it opens the gate on a session that + // has unsaved work and unlinks the crash protection of a buffer the + // user is looking at. + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + let manager = try #require(project.recoveryManager) + + let file = dir.appendingPathComponent("saved.swift") + try "original".write(to: file, atomically: true, encoding: .utf8) + project.primaryTabManager.autoSavePreferenceProvider = { false } + project.primaryTabManager.openTab(url: file) + project.primaryTabManager.updateContent("modified") + + let split = dir.appendingPathComponent("dirty.swift") + try "original".write(to: split, atomically: true, encoding: .utf8) + let secondPaneID = try #require( + project.paneManager.splitPane( + project.paneManager.activePaneID, + axis: .horizontal + ) + ) + let secondPane = try #require( + project.paneManager.tabManager(for: secondPaneID) + ) + secondPane.autoSavePreferenceProvider = { false } + secondPane.openTab(url: split) + secondPane.updateContent("modified") + + manager.snapshotDirtyTabs(project.allTabs) + // Only the primary pane's buffer goes back to its saved contents. + project.primaryTabManager.updateContent("original") + let crashed = Self.crashedTab(in: dir) + manager.snapshotDirtyTabs([crashed]) + let before = Set(manager.pendingRecoveryEntries().map(\.0)) + #expect(before.count == 3) + #expect(project.hasUnsavedChanges) + #expect( + !project.primaryTabManager.hasUnsavedChanges, + """ + The unsaved buffer has to live outside the primary pane, or a \ + gate narrowed to `primaryTabManager` still reads "dirty" and this \ + test proves nothing + """ + ) + + let delegate = AppDelegate() + delegate.registry = registry + delegate.applicationWillTerminate( + Notification(name: NSApplication.willTerminateNotification) + ) + + #expect(Set(manager.pendingRecoveryEntries().map(\.0)) == before) + } + + // MARK: - The published retention window + + @Test("The sheet's footnote and the launch sweep read the same number") + func retentionWindowIsASingleNumber() throws { + // "Later" is safe but not unlimited, and the sheet says how long. + // Two literals would let the promise drift from the behaviour. + #expect(RecoveryManager.staleEntryRetentionDays == 7) + let source = try String( + contentsOf: Self.repositoryRoot() + .appendingPathComponent("Pine/PineApp.swift"), + encoding: .utf8 + ) + #expect( + !source.contains("cleanupAllStaleEntries(olderThan: 7)"), + "The launch sweep hardcodes its own retention window again" + ) + } + + @Test("The sheet still prints the retention window it promises") + func theSheetStatesTheRetentionWindow() throws { + // `retentionNoticeIsLocalizedEverywhere` only proves the string exists + // in the catalog. Deleting the `Text` from the sheet's `body` breaks + // nothing else — the sheet is sized by `minWidth`, so it does not even + // change shape — and this branch's central claim, that "Later" is a + // bounded promise the user can read, silently becomes false with the + // whole suite green. The literal is banned here as well as in + // `PineApp.swift`: the sheet is the surface the number is *read* on, + // so it is the one place a drifting copy does its damage. + let source = try String( + contentsOf: Self.repositoryRoot() + .appendingPathComponent("Pine/RecoveryDialogView.swift"), + encoding: .utf8 + ) + + #expect( + source.contains("Strings.recoveryRetentionNotice("), + "The recovery sheet no longer tells the user how long Later lasts" + ) + #expect( + source.contains("RecoveryManager.staleEntryRetentionDays"), + "The sheet's footnote no longer reads the sweep's own constant" + ) + #expect( + !source.contains("days: 7"), + "The sheet hardcodes a retention window that can drift from the sweep" + ) + } + + @Test("A snapshot older than the retention window is collected") + func snapshotsExpireAtTheStatedBoundary() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let days = Double(RecoveryManager.staleEntryRetentionDays) + + let fresh = try Self.writeSnapshot(daysOld: days - 1, in: dir) + try Self.writeSnapshot(daysOld: days + 1, in: dir) + + manager.cleanupStaleEntries( + olderThan: RecoveryManager.staleEntryRetentionDays + ) + + #expect(manager.pendingRecoveryEntries().map(\.0) == [fresh]) + } + + @Test("A file the filesystem calls fresh is kept without being decoded") + func aFreshModificationDateKeepsTheFileWhateverItClaims() throws { + // The launch sweep runs on the main actor and a snapshot carries a + // whole unsaved buffer, so it now settles a file by its modification + // date whenever it can and only decodes what that leaves undecided — + // in steady state, nothing (AGENTS.md: never block the main thread + // with file I/O). The fast path is observable exactly here: an entry + // dated outside the window in a file written moments ago is kept. + // + // One write sets both dates, so production cannot make them disagree; + // a restored backup can, and the disagreement is resolved toward + // keeping the file, never toward deleting it. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let id = try Self.writeSnapshot(daysOld: 30, in: dir) + try FileManager.default.setAttributes( + [.modificationDate: Date()], + ofItemAtPath: dir.appendingPathComponent("\(id.uuidString).json").path + ) + + manager.cleanupStaleEntries(olderThan: 7) + + #expect(manager.pendingRecoveryEntries().map(\.0) == [id]) + } + + // MARK: - The files that used to be immortal + + @Test("An unreadable snapshot is collected instead of living forever") + func anUndecodableSnapshotIsCollected() throws { + // Truncated by the crash that was happening while it was written. + // `pendingRecoveryEntries()` cannot show it and the sweep used to + // `continue` past it, so the user could neither see it nor delete it, + // it kept its project's directory alive forever, and it logged an + // error on every launch. It is not content anybody could have decided + // about, so ageing it out is not a decision taken on their behalf. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let url = dir.appendingPathComponent("\(UUID().uuidString).json") + try Data("{\"originalPath\": \"/tmp/a.swift\", tru".utf8).write(to: url) + try FileManager.default.setAttributes( + [.modificationDate: Date().addingTimeInterval(-8 * 24 * 3600)], + ofItemAtPath: url.path + ) + + manager.cleanupStaleEntries(olderThan: 7) + + #expect(!FileManager.default.fileExists(atPath: url.path)) + } + + @Test("A recent unreadable snapshot is left alone") + func aRecentUndecodableSnapshotIsKept() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let url = dir.appendingPathComponent("\(UUID().uuidString).json") + try Data("not json at all".utf8).write(to: url) + + manager.cleanupStaleEntries(olderThan: 7) + + #expect(FileManager.default.fileExists(atPath: url.path)) + } + + @Test("A snapshot from a newer schema outlives the normal window") + func anUnsupportedSchemaGetsALongerHorizon() throws { + // The real producer: a beta wrote `schemaVersion: 2` and the user went + // back to a release build. The normal window bounds files a user was + // shown and left alone for a week; a stamp from a build that is not + // running right now says nothing about whether they have decided, so + // it gets a multiple of the window — and still leaves eventually, so + // a permanently orphaned file cannot pin its project's directory + // open. These two decode, so they are also offered (#1503): the + // longer horizon buys time, it does not decide visibility. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let days = RecoveryManager.staleEntryRetentionDays + let multiplier = RecoveryManager.unsupportedSchemaRetentionMultiplier + #expect(multiplier > 1) + + let survivor = try Self.writeSnapshot( + daysOld: Double(days) + 1, + schemaVersion: RecoveryEntry.currentSchemaVersion + 1, + in: dir + ) + let expired = try Self.writeSnapshot( + daysOld: Double(days * multiplier) + 1, + schemaVersion: RecoveryEntry.currentSchemaVersion + 1, + in: dir + ) + + manager.cleanupStaleEntries(olderThan: days) + + #expect(Self.snapshotIDs(in: dir) == [survivor]) + #expect(!Self.snapshotIDs(in: dir).contains(expired)) + // …and what survived is offered, not silently held. A build that can + // read a buffer must not both hide it and eventually delete it, which + // is what the old "unsupported schema is not shown" rule amounted to + // on precisely this file (#1503). + #expect(manager.pendingRecoveryEntries().map(\.0) == [survivor]) + } + + @Test("A future-dated snapshot is anchored to the sweep, then collected") + func aFutureDatedSnapshotStopsBeingImmortal() throws { + // A clock moved forward, a restored VM snapshot, a bad RTC. Judged by + // `timestamp < cutoff` alone the comparison can never come true, so + // the file outlived every sweep there would ever be. The first sweep + // that sees it gives it a real anchor — now — and the next one that + // finds that anchor old collects it. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let id = try Self.writeSnapshot(daysOld: -365, in: dir) + let url = dir.appendingPathComponent("\(id.uuidString).json") + + manager.cleanupStaleEntries(olderThan: 7) + + // Kept — a date nobody can trust is not a reason to delete work… + #expect(FileManager.default.fileExists(atPath: url.path)) + // Through `FileManager`, not `URL.resourceValues`: a `URL` caches the + // resource values it has been asked for, so re-reading the same `URL` + // after the sweep hands back the date from before it. + let stamped = try #require( + FileManager.default.attributesOfItem(atPath: url.path)[ + .modificationDate + ] as? Date + ) + #expect( + abs(stamped.timeIntervalSinceNow) < 60, + """ + The sweep left the file's date where it was (\(stamped)) instead \ + of anchoring it to this run. A date the sweep cannot reach is a \ + file the sweep can never collect + """ + ) + + // …and it is no longer immortal: the anchor ages like any other date. + try FileManager.default.setAttributes( + [.modificationDate: Date().addingTimeInterval(-8 * 24 * 3600)], + ofItemAtPath: url.path + ) + manager.cleanupStaleEntries(olderThan: 7) + + #expect(!FileManager.default.fileExists(atPath: url.path)) + } + + @Test("A snapshot a few seconds ahead of the clock is not restamped") + func aSlightlyFutureDateIsToleratedRatherThanRewritten() throws { + // Re-anchoring is a one-way loss: for a snapshot whose JSON cannot be + // decoded, the modification date is the only surviving record of when + // it was written, and restamping overwrites it. Firing on any date + // past `now` would do that to ordinary files — an NTP step, a network + // volume a second ahead, a snapshot written in the same second the + // sweep runs. `futureDateTolerance` is the fence. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + #expect(RecoveryManager.futureDateTolerance > 0) + let id = try Self.writeSnapshot(daysOld: -10.0 / 86_400, in: dir) + let url = dir.appendingPathComponent("\(id.uuidString).json") + let before = try Self.modificationDate(of: url) + + manager.cleanupStaleEntries(olderThan: 7) + + let after = try Self.modificationDate(of: url) + #expect( + abs(after.timeIntervalSince(before)) < 1, + """ + A snapshot ten seconds ahead of the sweep's clock was re-anchored \ + (\(before) became \(after)). Clock jitter is not a broken clock, \ + and the date it destroys is provenance nothing else records + """ + ) + #expect(manager.pendingRecoveryEntries().map(\.0) == [id]) + } + + @Test("An old snapshot with a future modification date still expires") + func aFutureModificationDateDoesNotProtectAnOldEntry() throws { + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let id = try Self.writeSnapshot(daysOld: 30, in: dir) + let url = dir.appendingPathComponent("\(id.uuidString).json") + try FileManager.default.setAttributes( + [.modificationDate: Date().addingTimeInterval(365 * 24 * 3600)], + ofItemAtPath: url.path + ) + + manager.cleanupStaleEntries(olderThan: 7) + + #expect(!FileManager.default.fileExists(atPath: url.path)) + } + + @Test("A modification date older than the entry cannot delete it") + func anOlderModificationDateCannotDeleteAFreshEntry() throws { + // The mirror of `aFreshModificationDateKeepsTheFileWhateverItClaims`, + // and the direction nothing pinned. Taking the earlier of the two + // dates reads like caution and is the opposite: it lets the + // filesystem's word delete a snapshot the entry itself says is a day + // old. Anything that moves an mtime backwards produces it — a sync + // client, a restore tool, `touch -t`, a volume whose timestamps are + // coarser than those of the one the file came from (SMB, exFAT). + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let id = try Self.writeSnapshot(daysOld: 1, in: dir) + try FileManager.default.setAttributes( + [.modificationDate: Date().addingTimeInterval(-30 * 24 * 3600)], + ofItemAtPath: dir + .appendingPathComponent("\(id.uuidString).json").path + ) + + manager.cleanupStaleEntries(olderThan: 7) + + #expect( + manager.pendingRecoveryEntries().map(\.0) == [id], + """ + A backdated modification date deleted a snapshot whose own \ + timestamp is a day old. The filesystem date is allowed to keep a \ + file, never to condemn one (#1503) + """ + ) + } + + @Test("A newer schema that changed shape also gets the longer horizon") + func aNonAdditiveSchemaGetsALongerHorizon() throws { + // `anUnsupportedSchemaGetsALongerHorizon` writes `schemaVersion + 1` + // into an otherwise current entry — a purely additive change, and the + // one case where the whole `RecoveryEntry` still decodes so the + // version can be read off the decoded value. The schema changes that + // actually motivate a version stamp do not decode: rename a field, + // change its type, add a required one, and `init(from:)` throws before + // it ever reaches `schemaVersion`. Deciding the horizon from a + // successful decode grants the long window to exactly the files that + // least need it, and deletes this user's unrecoverable work — which + // this build refuses to show them — on the seventh day. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let days = RecoveryManager.staleEntryRetentionDays + let multiplier = RecoveryManager.unsupportedSchemaRetentionMultiplier + // `body` where this build wants `content`, and a numeric `timestamp`. + let json = """ + {"schemaVersion": \(RecoveryEntry.currentSchemaVersion + 1), \ + "originalPath": "/tmp/a.swift", "body": "unsaved", \ + "timestamp": 776000000, "encodingRawValue": 4} + """ + let decoder = JSONDecoder() + decoder.dateDecodingStrategy = .iso8601 + #expect(throws: (any Error).self) { + try decoder.decode(RecoveryEntry.self, from: Data(json.utf8)) + } + + let survivor = try Self.writeRawSnapshot( + json, + daysOld: Double(days) + 1, + in: dir + ) + let expired = try Self.writeRawSnapshot( + json, + daysOld: Double(days * multiplier) + 1, + in: dir + ) + + manager.cleanupStaleEntries(olderThan: days) + + #expect( + Self.snapshotIDs(in: dir) == [survivor], + """ + A snapshot from a schema this build cannot decode was collected \ + at the normal horizon. The version stamp has to be read before \ + the full decode, or the constant that exists for this file never \ + applies to it (#1503) + """ + ) + #expect(!Self.snapshotIDs(in: dir).contains(expired)) + } + + @Test("A file the sweep cannot read is kept, not aged out") + func anUnreadableSnapshotIsKept() throws { + // Launch is when every language server, file-system watcher and + // terminal in the app is starting at once, so a transient `EIO` or + // `EMFILE` here is a live possibility. A file the process could not + // open has not told anybody how old it is, and ageing it out by its + // modification date means deleting undecided work on the strength of + // a read that failed. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + let id = try Self.writeSnapshot(daysOld: 30, in: dir) + let url = dir.appendingPathComponent("\(id.uuidString).json") + try FileManager.default.setAttributes( + [.posixPermissions: 0], + ofItemAtPath: url.path + ) + defer { + try? FileManager.default.setAttributes( + [.posixPermissions: 0o600], + ofItemAtPath: url.path + ) + } + guard (try? Data(contentsOf: url)) == nil else { + // Root, or a filesystem that ignores mode bits: the premise + // cannot be established, so there is nothing here to assert. + return + } + + manager.cleanupStaleEntries(olderThan: 7) + + #expect( + FileManager.default.fileExists(atPath: url.path), + """ + An unreadable snapshot was deleted by its modification date. \ + Unreadable is not evidence of age (#1503) + """ + ) + } + + // MARK: - Collecting the per-project subdirectories + + @Test("A subdirectory holding nothing but leftovers is collected") + func aSubdirectoryWithOnlyLeftoversIsCollected() throws { + // Snapshots are written with `Data.write(options: .atomic)`, which + // stages a hidden temporary beside the destination. A crash between + // the staging and the rename leaves it behind — and it is not a + // `.json`, so the stale sweep cannot see it, while an emptiness test + // can. One orphaned temporary used to pin its project's directory + // open for the life of the machine, long after the project was gone. + let root = try Self.makeTempDir() + defer { Self.cleanup(root) } + let orphaned = root.appendingPathComponent("orphaned") + let live = root.appendingPathComponent("live") + for dir in [orphaned, live] { + try FileManager.default.createDirectory( + at: dir, + withIntermediateDirectories: true + ) + } + try Data("half a snapshot".utf8).write( + to: orphaned.appendingPathComponent(".dat.nosync0f12.QwErTy") + ) + try Data("".utf8).write(to: orphaned.appendingPathComponent(".DS_Store")) + try Self.writeSnapshot(daysOld: 0, in: live) + + RecoveryManager.cleanupAllStaleEntries(in: root, olderThan: 7) + + #expect( + !FileManager.default.fileExists(atPath: orphaned.path), + "A directory holding only leftovers was kept forever" + ) + #expect( + FileManager.default.fileExists(atPath: live.path), + "A directory with a live snapshot in it was collected" + ) + } + + @Test("A stray file at the recovery root is not swept as a project") + func aStrayFileAtTheRootIsNotAProject() throws { + // `.isDirectoryKey` was prefetched here and never consulted, so a + // `.DS_Store` at the root was handed to a `RecoveryManager` as a + // project directory and logged a failure on every launch. + let root = try Self.makeTempDir() + defer { Self.cleanup(root) } + let stray = root.appendingPathComponent(".DS_Store") + try Data("not a project".utf8).write(to: stray) + let project = root.appendingPathComponent("project") + try FileManager.default.createDirectory( + at: project, + withIntermediateDirectories: true + ) + let old = try Self.writeSnapshot(daysOld: 30, in: project) + + RecoveryManager.cleanupAllStaleEntries(in: root, olderThan: 7) + + #expect(FileManager.default.fileExists(atPath: stray.path)) + #expect(!Self.snapshotIDs(in: project).contains(old)) + } + + // MARK: - Not offering the same sheet twice in one session + + @Test("An answered offer stops coming back while the files stay") + func answeringTheOfferSuppressesItWithoutDeletingAnything() async throws { + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + project.recoveryManager?.snapshotDirtyTabs([Self.crashedTab(in: dir)]) + + #expect(await project.pendingRecoveryOffer().count == 1) + + project.markRecoveryOfferAnswered() + + // The sheet does not come back — SwiftUI re-runs the scene's `.task` + // on restoration and on window close/reopen, and the project outlives + // both, which is the whole reason the flag lives here. + #expect(await project.pendingRecoveryOffer().isEmpty) + #expect(await project.pendingRecoveryOffer().isEmpty) + // …but nothing was deleted, so the next launch still gets the offer. + #expect( + project.recoveryManager?.pendingRecoveryEntries().count == 1 + ) + await project.workspace.waitForLoadingComplete() + } + + @Test("A restore in flight suppresses the offer without answering it") + func anInFlightRestoreSuppressesTheOffer() async throws { + // Recover All is not instantaneous. A snapshot past the large-file + // threshold parks the whole restore on a native sheet, and the sheet's + // own state was already cleared synchronously — so a scene `.task` + // re-running in that window (restoration, or the window closed and + // reopened) finds both crash files still on disk, owned by no open + // tab, and `didAnswerRecoveryOffer` still false. It builds the same + // sheet again. A second Recover All then migrates the same entries a + // second time: the parked restore resumes against a detached + // `TabManager`, and the migration writes a snapshot under a runtime ID + // no window owns, which comes back on the next launch as a phantom + // "recovered file" nobody can account for. + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + project.recoveryManager?.snapshotDirtyTabs([Self.crashedTab(in: dir)]) + #expect(await project.pendingRecoveryOffer().count == 1) + + project.beginRecoveryRestore() + + #expect( + await project.pendingRecoveryOffer().isEmpty, + """ + A second sheet can be built from the same crash entries while the \ + first restore is still parked (#1503) + """ + ) + // …and this is not an answer. Nothing was deleted, nothing was + // marked, and when the restore finishes the offer is available again + // for whatever it hands back. + #expect(project.recoveryManager?.pendingRecoveryEntries().count == 1) + + project.endRecoveryRestore() + + #expect( + await project.pendingRecoveryOffer().count == 1, + """ + Suppressing the offer during a restore outlived the restore. \ + "Being handled" and "answered" are different claims, and only the \ + second one is allowed to survive the `defer` + """ + ) + await project.workspace.waitForLoadingComplete() + } + + @Test("An unanswered offer keeps being offered") + func anUnansweredOfferKeepsComingBack() async throws { + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + // A snapshot from the session that crashed: its ID names a tab that no + // longer exists. Writing this with an *open* tab's ID instead would + // pin the opposite of what the sheet should do — see + // `theOfferNeverContainsATabTheUserHasOpen`. + project.recoveryManager?.snapshotDirtyTabs([Self.crashedTab(in: dir)]) + + for _ in 0..<3 { + #expect(await project.pendingRecoveryOffer().count == 1) + } + await project.workspace.waitForLoadingComplete() + } + + @Test("The offer never contains a tab the user has open") + func theOfferNeverContainsATabTheUserHasOpen() async throws { + // No crash needed to reach this. Open a project, edit a file, close + // the window: the project is held alive because it has dirty tabs, and + // `suspendEditorServices` deliberately writes snapshots for them. + // Reopen it and `checkForRecovery()` runs against the same + // `ProjectManager`, asking `pendingRecoveryEntries()`, which only + // knows "there is a JSON file named after a UUID". + // + // Offering those back is not cosmetic. Discard would delete the crash + // protection of tabs the user is looking at, and Recover All would + // clone each one and then have `migrateRecoverySnapshot` unlink the + // original's live snapshot. A snapshot whose ID is an open tab's ID is + // by definition not a crash leftover. + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + let file = dir.appendingPathComponent("dirty.swift") + try "original".write(to: file, atomically: true, encoding: .utf8) + project.primaryTabManager.autoSavePreferenceProvider = { false } + project.primaryTabManager.openTab(url: file) + project.primaryTabManager.updateContent("modified") + project.recoveryManager?.snapshotDirtyTabs(project.allTabs) + + // The snapshot exists — the crash protection is doing its job… + #expect(project.recoveryManager?.pendingRecoveryEntries().count == 1) + // …and it is not an offer, because its tab is on screen. + #expect(await project.pendingRecoveryOffer().isEmpty) + await project.workspace.waitForLoadingComplete() + } + + @Test("The directory listing runs off the main actor") + func theListingRunsOffTheMainActor() async throws { + // `pendingRecoveryEntries()` opens and fully decodes every file in the + // directory, and each one is a whole unsaved buffer with no size cap — + // `snapshotDirtyTabs` selects on `isDirty && kind == .text`, and the + // 1 MB large-file threshold is not applied to it. Before this branch + // the directory was emptied on every clean quit, so a launch paid for + // it at most once, after a crash. Now snapshots are held for + // `staleEntryRetentionDays` after a "Later" and the scene `.task` + // re-runs on restoration and on close/reopen, which would put three + // 40 MB buffers in front of the window on every launch and every + // reopen for a week. AGENTS.md: never block the main thread with file + // I/O. + // + // This test does not compile at all unless the listing is + // `nonisolated` — that is half the assertion — and the thread check is + // the other half. + let dir = try Self.makeTempDir() + defer { Self.cleanup(dir) } + let manager = RecoveryManager(recoveryDirectory: dir) + manager.snapshotDirtyTabs([ + Self.dirtyTab(path: "/tmp/a.swift"), + Self.dirtyTab(path: "/tmp/b.swift"), + ]) + + let observed = await Task.detached(priority: .userInitiated) { + ( + count: RecoveryManager.readEntries(in: dir).count, + onMain: pthread_main_np() != 0 + ) + }.value + + #expect(observed.count == 2) + #expect( + observed.onMain == false, + "The snapshot listing is still running on the main thread" + ) + } + + @Test("An offer answered while the directory is read is not delivered") + func answeringDuringTheListingSuppressesTheOffer() async throws { + // The suspension `pendingRecoveryOffer()` gained is a window, and the + // two flags it checks are exactly what that window can invalidate: a + // Recover All can begin, or the user can answer the sheet, between the + // listing starting and its results coming back. Checking only before + // the `await` would let a stale listing repopulate `recoveryEntries` + // and put a second sheet on screen over the answer that was just given + // (#1503). + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + project.recoveryManager?.snapshotDirtyTabs([Self.crashedTab(in: dir)]) + #expect(await project.pendingRecoveryOffer().count == 1) + + let inFlight = Task { @MainActor in + await project.pendingRecoveryOffer() + } + // Hands the main actor to the task above, which runs as far as its own + // suspension — the off-actor listing — and stops there. + await Task.yield() + project.markRecoveryOfferAnswered() + + #expect( + await inFlight.value.isEmpty, + """ + A listing that started before the offer was answered still \ + delivered its entries + """ + ) + // …and the files are all still there, so the next launch offers them. + #expect(project.recoveryManager?.pendingRecoveryEntries().count == 1) + await project.workspace.waitForLoadingComplete() + } + + @Test("A live tab's snapshot is filtered out, a crash snapshot is not") + func theOfferKeepsCrashSnapshotsAndDropsLiveOnes() async throws { + let dir = try Self.makeTempDir() + defer { Self.cleanupProject(dir) } + let registry = ProjectRegistry() + let project = try #require(registry.projectManager(for: dir)) + let file = dir.appendingPathComponent("dirty.swift") + try "original".write(to: file, atomically: true, encoding: .utf8) + project.primaryTabManager.autoSavePreferenceProvider = { false } + project.primaryTabManager.openTab(url: file) + project.primaryTabManager.updateContent("modified") + let crashed = Self.crashedTab(in: dir) + project.recoveryManager?.snapshotDirtyTabs(project.allTabs + [crashed]) + + #expect(project.recoveryManager?.pendingRecoveryEntries().count == 2) + #expect(await project.pendingRecoveryOffer().map(\.0) == [crashed.id]) + await project.workspace.waitForLoadingComplete() + } + + // MARK: - Helpers + + private static func repositoryRoot() -> URL { + URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() + .deletingLastPathComponent() + } + + private static func makeTempDir() throws -> URL { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent( + "PineRecoverySweepTests-\(UUID().uuidString)" + ) + try FileManager.default.createDirectory( + at: url, + withIntermediateDirectories: true + ) + return url + } + + private static func cleanup(_ url: URL) { + try? FileManager.default.removeItem(at: url) + } + + /// Removes a project directory *and* the recovery subdirectory Pine + /// created for it under Application Support. + /// + /// Any test that builds a real `ProjectManager` gets a real + /// `RecoveryManager` rooted at `RecoveryManager.directory(for:)` — a + /// SHA-256 of the project path under the user's own Application Support, + /// not under the temporary directory the test cleans up. Removing only the + /// project left one subdirectory per test run behind. (Since #1503 the + /// launch sweep does eventually collect them, leftovers and all, but a + /// test suite should not be leaning on that.) + private static func cleanupProject(_ projectURL: URL) { + cleanup(projectURL) + cleanup(RecoveryManager.directory(for: projectURL)) + } + + private static func dirtyTab(path: String) -> EditorTab { + EditorTab( + url: URL(fileURLWithPath: path), + content: "unsaved", + savedContent: "saved" + ) + } + + /// A dirty tab that belongs to no window: the shape of what a crash leaves + /// behind, and the only shape that should ever become an offer. + private static func crashedTab(in dir: URL) -> EditorTab { + EditorTab( + url: dir.appendingPathComponent("crashed.swift"), + content: "unsaved", + savedContent: "saved" + ) + } + + /// Writes a snapshot the way production writes one: the entry's timestamp + /// and the file's modification date are the same moment, because a single + /// write sets both. Tests that fabricate an old entry inside a + /// just-written file are describing a file the app cannot produce. + /// + /// A negative `daysOld` puts the snapshot in the future. + @discardableResult + private static func writeSnapshot( + id: UUID = UUID(), + daysOld: Double, + schemaVersion: Int? = RecoveryEntry.currentSchemaVersion, + in dir: URL + ) throws -> UUID { + let when = Date().addingTimeInterval(-daysOld * 24 * 3600) + let encoder = JSONEncoder() + encoder.dateEncodingStrategy = .iso8601 + let entry = RecoveryEntry( + schemaVersion: schemaVersion, + originalPath: "/tmp/\(id.uuidString).swift", + content: "unsaved", + timestamp: when, + encoding: .utf8 + ) + let url = dir.appendingPathComponent("\(id.uuidString).json") + try encoder.encode(entry).write(to: url) + try FileManager.default.setAttributes( + [.modificationDate: when], + ofItemAtPath: url.path + ) + return id + } + + /// Writes an arbitrary JSON body under a snapshot's name, with a matching + /// modification date. For shapes this build's `RecoveryEntry` cannot + /// decode, which is the interesting half of schema versioning. + @discardableResult + private static func writeRawSnapshot( + _ json: String, + daysOld: Double, + in dir: URL + ) throws -> UUID { + let id = UUID() + let when = Date().addingTimeInterval(-daysOld * 24 * 3600) + let url = dir.appendingPathComponent("\(id.uuidString).json") + try Data(json.utf8).write(to: url) + try FileManager.default.setAttributes( + [.modificationDate: when], + ofItemAtPath: url.path + ) + return id + } + + /// Read through `FileManager`, not `URL.resourceValues`: a `URL` caches + /// the resource values it has been asked for, so re-reading the same + /// `URL` after a sweep hands back the date from before it. + private static func modificationDate(of url: URL) throws -> Date { + try #require( + FileManager.default.attributesOfItem(atPath: url.path)[ + .modificationDate + ] as? Date + ) + } + + /// Every snapshot file in the directory, readable or not — the sweep's + /// own view, which `pendingRecoveryEntries()` cannot give. + private static func snapshotIDs(in dir: URL) -> Set { + let names = (try? FileManager.default.contentsOfDirectory( + atPath: dir.path + )) ?? [] + return Set( + names + .filter { $0.hasSuffix(".json") } + .compactMap { UUID(uuidString: String($0.dropLast(5))) } + ) + } +}