From 13b39a44cbaaf71b1d3fe2cdb466139bd928dd05 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Wed, 10 Jun 2026 08:58:19 +1000 Subject: [PATCH 1/3] Stop wiping the index when a watched folder can't be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After installing a new build, Vista appeared to lose its whole index and re-OCR everything. The index file actually survives — the bug was in the initial-scan reconcile: it deleted every row whose file wasn't seen this scan. The screenshots live in iCloud Drive, a TCC-protected location, and a fresh binary can lose Full Disk Access, so the folder enumerated empty and the reconcile wiped all ~6000 rows. - Track per-folder scan health. Only delete a row when its folder was genuinely readable, the file wasn't found, and fileExists confirms it's actually gone (iCloud dataless placeholders still report present, so they're preserved). A readable folder that returns nothing while we hold rows for it trips a circuit breaker instead of deleting. - When a folder we have rows for can't be read, emit .accessBlocked. The menu and the panel now show a 'grant access' message with a button to the Full Disk Access pane, instead of a misleading 'no screenshots indexed' empty state. Granting access resumes with zero re-OCR. - Reload the grid on every panel show so freshly indexed screenshots appear, instead of only after the reset timeout. - Clearer status copy: 'Reading text from screenshots · X of Y · Z ready', 'N screenshots ready', drop the OCR jargon. The working state now shows how many are already searchable so a backlog never reads as empty. - Pure reconcile helper with unit tests covering the no-wipe guard. Co-authored-by: Claude Co-authored-by: GitButler --- Sources/Vista/AppState.swift | 13 ++- Sources/Vista/MenuBarContentView.swift | 47 +++++++- Sources/Vista/PanelContentView.swift | 33 +++++- Sources/Vista/PanelController.swift | 21 +++- Sources/VistaCore/Indexer.swift | 109 +++++++++++++++--- .../IndexerReconcileTests.swift | 88 ++++++++++++++ 6 files changed, 283 insertions(+), 28 deletions(-) create mode 100644 Tests/VistaCoreTests/IndexerReconcileTests.swift diff --git a/Sources/Vista/AppState.swift b/Sources/Vista/AppState.swift index 57a2389..1476061 100644 --- a/Sources/Vista/AppState.swift +++ b/Sources/Vista/AppState.swift @@ -22,10 +22,18 @@ final class AppState { /// the Permissions tab. var indexedCount: Int { if case .watching(let n) = indexingProgress { return n } - if case .indexing(let done, _) = indexingProgress { return done } + if case .indexing(_, _, let indexed) = indexingProgress { return indexed } return 0 } + /// Folders we hold indexed rows for but couldn't read on the last scan. + /// Non-empty drives the "grant access" messaging in the menu and panel; + /// the index itself is preserved untouched while this is set. + var accessBlockedFolders: [URL] { + if case .accessBlocked(let folders) = indexingProgress { return folders } + return [] + } + let preferences = Preferences() // MARK: - Private state @@ -68,7 +76,8 @@ final class AppState { store: store, thumbnails: thumbnails, actions: actions, - preferences: prefs + preferences: prefs, + appState: self ) // Register the current hotkey chord. diff --git a/Sources/Vista/MenuBarContentView.swift b/Sources/Vista/MenuBarContentView.swift index 9ba49fb..05c92ab 100644 --- a/Sources/Vista/MenuBarContentView.swift +++ b/Sources/Vista/MenuBarContentView.swift @@ -18,6 +18,15 @@ struct MenuBarContentView: View { var body: some View { Text(statusLine) + // Surfaced only when a folder we already indexed can't be read. + // The index is preserved while this shows — granting access lets + // the scan resume without re-processing anything. + if !appState.accessBlockedFolders.isEmpty { + Button("Grant Folder Access…") { + openFullDiskAccessSettings() + } + } + Divider() Button("Search Screenshots…") { @@ -108,21 +117,47 @@ struct MenuBarContentView: View { private var statusLine: String { switch appState.indexingProgress { case .idle: - return "Vista — idle" + return "Vista — up to date" case .enumerating(let folders): - return folders == 1 ? "Scanning folder…" : "Scanning \(folders) folders…" - case .indexing(let done, let total): + return folders == 1 + ? "Scanning your screenshots folder…" + : "Scanning your screenshots folders…" + case .indexing(let done, let total, let indexed): if total == 0 { // Empty queue = fully resumed from the DB, everything // was already indexed. Jump straight to the steady-state // message so the user doesn't see a flash of "0 / 0". - return "Up to date" + return "\(Self.grouped(indexed)) screenshots ready" } - return "OCR'ing \(done) / \(total) new images" + // Show the work left to do plus how many are already searchable, + // so a large backlog never reads as "nothing indexed". + return "Reading text from screenshots · \(Self.grouped(done)) of \(Self.grouped(total)) · \(Self.grouped(indexed)) ready" case .watching(let indexed): - return "\(indexed) screenshots indexed" + return "\(Self.grouped(indexed)) screenshots ready" + case .accessBlocked: + return "Can't read your screenshots folder — grant access" } } + + /// Opens System Settings → Privacy & Security → Full Disk Access, where + /// the user can re-enable Vista's access to the (TCC-protected) iCloud + /// screenshots folder. + private func openFullDiskAccessSettings() { + if let url = URL(string: "x-apple.systempreferences:com.apple.preference.security?Privacy_AllFiles") { + NSWorkspace.shared.open(url) + } + } + + private static let groupingFormatter: NumberFormatter = { + let f = NumberFormatter() + f.numberStyle = .decimal + return f + }() + + /// Thousands-grouped count ("6,035") for the status line. + private static func grouped(_ n: Int) -> String { + groupingFormatter.string(from: NSNumber(value: n)) ?? "\(n)" + } } extension HotKeyChord { diff --git a/Sources/Vista/PanelContentView.swift b/Sources/Vista/PanelContentView.swift index b752ddf..8e87fc0 100644 --- a/Sources/Vista/PanelContentView.swift +++ b/Sources/Vista/PanelContentView.swift @@ -28,6 +28,7 @@ struct PanelContentView: View { let thumbnails: ThumbnailCache let actions: ActionHandlers let preferences: Preferences + let appState: AppState let dismiss: () -> Void // Drives keyboard focus so the search field is live the moment the @@ -57,7 +58,9 @@ struct PanelContentView: View { VStack(spacing: 0) { searchBar Divider() - if model.results.isEmpty { + if !appState.accessBlockedFolders.isEmpty, model.results.isEmpty { + accessBlockedState + } else if model.results.isEmpty { emptyState } else { resultsGrid @@ -136,6 +139,34 @@ struct PanelContentView: View { .frame(maxWidth: .infinity, maxHeight: .infinity) } + /// Shown instead of the "nothing indexed" empty state when a folder we + /// already indexed can't be read. The distinction matters: the index is + /// intact, so the message reassures rather than implying data loss, and + /// points the user at the one action that fixes it. + private var accessBlockedState: some View { + VStack(spacing: 10) { + Image(systemName: "lock.shield") + .font(.system(size: 48, weight: .light)) + .foregroundStyle(.tertiary) + Text("Vista can't read your screenshots folder") + .font(.headline) + .foregroundStyle(.secondary) + Text("Your index is safe — nothing was deleted. Grant access and Vista picks up where it left off.") + .font(.subheadline) + .foregroundStyle(.tertiary) + .multilineTextAlignment(.center) + .padding(.horizontal, 32) + Button("Grant Folder Access…") { + if let url = URL(string: "x-apple.systempreferences:com.apple.preference.security?Privacy_AllFiles") { + NSWorkspace.shared.open(url) + } + } + .buttonStyle(.borderedProminent) + .padding(.top, 4) + } + .frame(maxWidth: .infinity, maxHeight: .infinity) + } + /// Hint text for the empty state. Picks the most relevant folder to /// name — a user-added folder if one exists, otherwise the system /// default if it's being watched, otherwise tells the user to add one. diff --git a/Sources/Vista/PanelController.swift b/Sources/Vista/PanelController.swift index 1f019a0..07884f7 100644 --- a/Sources/Vista/PanelController.swift +++ b/Sources/Vista/PanelController.swift @@ -16,6 +16,9 @@ public final class PanelController { private let thumbnails: ThumbnailCache private let actions: ActionHandlers private let preferences: Preferences + // Held so the panel's empty state can react to access-blocked status. + // App-lifetime object, so the strong reference is intentional. + private unowned let appState: AppState /// The app that was frontmost when the user invoked vista's hotkey — /// captured before we steal focus so "Paste to Front App" can aim @@ -28,16 +31,18 @@ public final class PanelController { /// the view-model is already fresh and there's nothing to reset. private var lastHiddenAt: Date? - public init( + init( store: ScreenshotStore, thumbnails: ThumbnailCache, actions: ActionHandlers, - preferences: Preferences + preferences: Preferences, + appState: AppState ) { self.store = store self.thumbnails = thumbnails self.actions = actions self.preferences = preferences + self.appState = appState // Wire the paste-to-front-app action. The closure runs on the // main actor (ActionHandlers is @MainActor) so it's safe to touch @@ -65,12 +70,19 @@ public final class PanelController { // previous query is unlikely to still be relevant, wipe back to // a clean state before the window is visible. Decided on each // show so changing the timeout in Settings takes effect - // immediately. Skip on the very first show of the session - // (lastHiddenAt == nil) — the view-model is already fresh. + // immediately. Any shorter gap (and the very first show, when + // lastHiddenAt is nil) falls through to a plain reload so freshly + // indexed screenshots appear. if let last = lastHiddenAt, let timeout = preferences.panelResetTimeout.seconds, Date().timeIntervalSince(last) >= timeout { viewModel?.resetState() + } else { + // Otherwise keep the user's query but refresh the rows — the + // indexer may have added screenshots since the panel was last + // shown, and without this the grid would keep showing the stale + // (possibly empty) result set from a previous open. + viewModel?.reload() } // Apply the latest panel-size preference every show so Appearance // slider changes take effect without needing a relaunch. @@ -149,6 +161,7 @@ public final class PanelController { thumbnails: thumbnails, actions: actions, preferences: preferences, + appState: appState, dismiss: { [weak self] in self?.hidePanel() } ) diff --git a/Sources/VistaCore/Indexer.swift b/Sources/VistaCore/Indexer.swift index 86cfb23..e499bb3 100644 --- a/Sources/VistaCore/Indexer.swift +++ b/Sources/VistaCore/Indexer.swift @@ -24,11 +24,18 @@ public actor Indexer { /// need to read + OCR), not all files on disk. A relaunch with /// a fresh DB might show total=4762; the next relaunch with the /// DB populated typically shows total=0 → we skip straight to - /// `.watching`. - case indexing(done: Int, total: Int) + /// `.watching`. `indexed` is the live total row count so the UI + /// can show how many are already searchable while OCR catches up. + case indexing(done: Int, total: Int, indexed: Int) /// Live-watching mode. `indexed` is the current total row count /// so the UI can render "1,234 screenshots indexed". case watching(indexed: Int) + /// One or more folders we already hold indexed rows for couldn't be + /// read this scan (missing Full Disk Access, or an iCloud folder + /// that enumerated empty). We deliberately do NOT delete those rows + /// — the UI surfaces this so the user can restore access, and the + /// index resumes untouched once they do. + case accessBlocked(folders: [URL]) } private let store: ScreenshotStore @@ -144,8 +151,11 @@ public actor Indexer { VistaLog.log("initialScan starting with \(watchedFolders.count) folder(s)") progressContinuation?.yield(.enumerating(folders: watchedFolders.count)) - // --- Phase 1: walk each watched folder and collect candidates. + // --- Phase 1: walk each watched folder and collect candidates, + // tracking per-root access health so a folder we simply couldn't + // read can never be mistaken for a folder that's genuinely empty. var discovered: [URL] = [] + var rootScans: [RootScanResult] = [] for root in watchedFolders { // Probe the folder before enumerating so a permission failure // surfaces in the logs instead of silently returning zero. @@ -153,7 +163,8 @@ public actor Indexer { let exists = fm.fileExists(atPath: root.path, isDirectory: &isDir) VistaLog.log(" \(root.path) exists=\(exists) isDir=\(isDir.boolValue)") guard exists, isDir.boolValue else { - VistaLog.log(" skipping — path does not exist or is not a directory") + VistaLog.log(" inaccessible — path does not exist or is not a directory") + rootScans.append(RootScanResult(rootPath: root.path, accessible: false, discoveredPaths: [])) continue } @@ -170,12 +181,13 @@ public actor Indexer { return true } ) else { - VistaLog.log(" could not build enumerator for \(root.path)") + VistaLog.log(" could not build enumerator for \(root.path) — treating as inaccessible") + rootScans.append(RootScanResult(rootPath: root.path, accessible: false, discoveredPaths: [])) continue } var seen = 0 - var matched = 0 + var rootDiscovered = Set() // `for in enumerator` drives NSFastEnumeration's makeIterator, // which Swift 6 treats as unavailable from async contexts. Manual // nextObject() avoids the sync/async mismatch without changing @@ -185,21 +197,33 @@ public actor Indexer { guard Self.isImageCandidate(next) else { continue } let values = try? next.resourceValues(forKeys: [.isRegularFileKey]) guard values?.isRegularFile == true else { continue } - matched += 1 + rootDiscovered.insert(next.path) discovered.append(next) } - VistaLog.log(" enumerated \(seen) entries, matched \(matched) images under \(root.path)") + VistaLog.log(" enumerated \(seen) entries, matched \(rootDiscovered.count) images under \(root.path)") + rootScans.append(RootScanResult(rootPath: root.path, accessible: true, discoveredPaths: rootDiscovered)) } VistaLog.log("initialScan discovered \(discovered.count) candidate files across \(watchedFolders.count) folder(s)") - // --- Phase 2: drop rows for files that have disappeared since - // the last scan. Cheap — one SELECT + one DELETE per stale row. + // --- Phase 2: reconcile. Only drop rows whose file is genuinely + // gone from a folder we could actually read. A folder that failed + // to enumerate — or came back empty while we still hold rows for it + // (the fresh-install / iCloud-not-materialised case) — never causes + // deletions; instead we surface `.accessBlocked` so the user can + // restore access and the index resumes untouched. The per-file + // `fileExists` check is the final belt: iCloud dataless placeholders + // still report true, so they're preserved even if discovery missed + // them. let known = try store.pathsOnDisk() - let discoveredPaths = Set(discovered.map(\.path)) - for stale in known.subtracting(discoveredPaths) { + let reconcile = Self.reconcileDeletions(known: known, roots: rootScans) + for stale in reconcile.delete where !fm.fileExists(atPath: stale) { try? store.delete(path: URL(fileURLWithPath: stale)) } + if !reconcile.blockedRoots.isEmpty { + VistaLog.log(" ACCESS BLOCKED for \(reconcile.blockedRoots.count) folder(s) holding indexed rows — preserving index, not deleting") + progressContinuation?.yield(.accessBlocked(folders: reconcile.blockedRoots.map { URL(fileURLWithPath: $0) })) + } // --- Phase 3: fingerprint-filter. Anything whose (mtime, size) // matches the DB entry is already indexed; skip without OCR. @@ -225,7 +249,7 @@ public actor Indexer { // reported against this shorter list so the counter reflects // actual work, not fingerprint-check flybys. let total = toIndex.count - progressContinuation?.yield(.indexing(done: 0, total: total)) + progressContinuation?.yield(.indexing(done: 0, total: total, indexed: (try? store.count()) ?? 0)) var done = 0 for url in toIndex { @@ -238,9 +262,11 @@ public actor Indexer { done += 1 // One progress event per 5 files keeps the UI lively during // OCR without drowning the observer. Also emit on the last - // file so the counter lands exactly at total. + // file so the counter lands exactly at total. `indexed` is the + // live row count so the menu can show how many are already + // searchable while OCR works through the backlog. if done % 5 == 0 || done == total { - progressContinuation?.yield(.indexing(done: done, total: total)) + progressContinuation?.yield(.indexing(done: done, total: total, indexed: (try? store.count()) ?? 0)) } } @@ -328,6 +354,59 @@ public actor Indexer { try store.upsert(record) } + // MARK: - Reconcile + + /// Outcome of walking one watched root, fed to `reconcileDeletions`. + /// `accessible` is the raw walk health (the folder existed, was a + /// directory, and built an enumerator); the empty-while-known circuit + /// breaker lives in `reconcileDeletions`, not here. + struct RootScanResult: Sendable, Equatable { + let rootPath: String + let accessible: Bool + let discoveredPaths: Set + } + + /// Decides which known rows are safe to delete during a reconcile, and + /// which roots are access-blocked. Pure so it can be unit-tested without + /// touching the filesystem or OCR. + /// + /// A known path is deleted only when its root was readable AND the path + /// wasn't seen this scan. A readable root that returned zero files while + /// we still hold rows under it is treated as blocked, not empty — that's + /// the fresh-install / lost-permission / iCloud-not-materialised case, + /// and deleting there is exactly the data-loss bug we're guarding. Rows + /// not under any watched root are left alone (folder removal reconciles + /// through `updateWatchedFolders`, not here). + static func reconcileDeletions( + known: Set, + roots: [RootScanResult] + ) -> (delete: [String], blockedRoots: [String]) { + var delete: [String] = [] + var blockedRoots: [String] = [] + for root in roots { + let knownUnder = known.filter { isPath($0, under: root.rootPath) } + // Circuit breaker: an "accessible" root that found nothing while + // we hold rows for it is not trustworthy — never wipe on it. + let trustworthy = root.accessible + && !(root.discoveredPaths.isEmpty && !knownUnder.isEmpty) + guard trustworthy else { + if !knownUnder.isEmpty { blockedRoots.append(root.rootPath) } + continue + } + for path in knownUnder where !root.discoveredPaths.contains(path) { + delete.append(path) + } + } + return (delete, blockedRoots) + } + + /// True when `path` lives inside `root` (strictly below it, so the root + /// directory itself never counts as one of its own file rows). + static func isPath(_ path: String, under root: String) -> Bool { + let prefix = root.hasSuffix("/") ? root : root + "/" + return path.hasPrefix(prefix) + } + // MARK: - Helpers private static let imageExtensions: Set = [ diff --git a/Tests/VistaCoreTests/IndexerReconcileTests.swift b/Tests/VistaCoreTests/IndexerReconcileTests.swift new file mode 100644 index 0000000..26b7cd8 --- /dev/null +++ b/Tests/VistaCoreTests/IndexerReconcileTests.swift @@ -0,0 +1,88 @@ +// IndexerReconcileTests.swift — Guards the "never wipe on a bad scan" rule. +// +// These cover the pure decision helper behind the data-loss fix: a folder +// we couldn't read (or that came back empty while we still hold rows for it) +// must never cause deletions. Instead it's reported as access-blocked and +// the index is left intact. + +import XCTest +@testable import VistaCore + +final class IndexerReconcileTests: XCTestCase { + + private let root = "/Users/test/screenshots" + private func path(_ name: String) -> String { "\(root)/\(name)" } + + // A folder that failed to enumerate (missing, not a dir, permission + // denied) never deletes the rows we hold for it — it's reported blocked. + func testInaccessibleRootDeletesNothingAndIsBlocked() { + let known: Set = [path("a.png"), path("b.png"), path("c.png")] + let roots = [Indexer.RootScanResult(rootPath: root, accessible: false, discoveredPaths: [])] + + let result = Indexer.reconcileDeletions(known: known, roots: roots) + + XCTAssertTrue(result.delete.isEmpty, "no rows should be deleted for an unreadable folder") + XCTAssertEqual(result.blockedRoots, [root]) + } + + // The fresh-install / iCloud-not-materialised case: the folder reads as + // "accessible" but returns zero files while we still hold rows for it. + // The circuit breaker treats this as blocked, not empty. + func testEmptyScanWhileKnownRowsExistTripsBreaker() { + let known: Set = [path("a.png"), path("b.png")] + let roots = [Indexer.RootScanResult(rootPath: root, accessible: true, discoveredPaths: [])] + + let result = Indexer.reconcileDeletions(known: known, roots: roots) + + XCTAssertTrue(result.delete.isEmpty, "an empty scan must not wipe a populated index") + XCTAssertEqual(result.blockedRoots, [root]) + } + + // A genuinely-deleted file (root readable, file no longer discovered) + // is the one case where deletion is correct. + func testReadableRootDeletesOnlyTrulyMissingFiles() { + let known: Set = [path("a.png"), path("b.png"), path("gone.png")] + let discovered: Set = [path("a.png"), path("b.png")] + let roots = [Indexer.RootScanResult(rootPath: root, accessible: true, discoveredPaths: discovered)] + + let result = Indexer.reconcileDeletions(known: known, roots: roots) + + XCTAssertEqual(result.delete, [path("gone.png")]) + XCTAssertTrue(result.blockedRoots.isEmpty) + } + + // Everything still on disk → nothing to do. + func testReadableRootWithEverythingPresentDeletesNothing() { + let known: Set = [path("a.png"), path("b.png")] + let roots = [Indexer.RootScanResult(rootPath: root, accessible: true, discoveredPaths: known)] + + let result = Indexer.reconcileDeletions(known: known, roots: roots) + + XCTAssertTrue(result.delete.isEmpty) + XCTAssertTrue(result.blockedRoots.isEmpty) + } + + // An unreadable folder we hold no rows for isn't a problem to surface — + // there's no data at risk, so it's neither deleted-from nor blocked. + func testInaccessibleRootWithNoKnownRowsIsNotBlocked() { + let known: Set = ["/Users/test/other/x.png"] + let roots = [Indexer.RootScanResult(rootPath: root, accessible: false, discoveredPaths: [])] + + let result = Indexer.reconcileDeletions(known: known, roots: roots) + + XCTAssertTrue(result.delete.isEmpty) + XCTAssertTrue(result.blockedRoots.isEmpty, "no rows under the folder means nothing to warn about") + } + + // Rows not under any watched root are left alone here — removing a + // watched folder reconciles through updateWatchedFolders, not this path. + func testRowsOutsideAnyWatchedRootAreLeftAlone() { + let known: Set = [path("a.png"), "/Users/test/elsewhere/orphan.png"] + let roots = [Indexer.RootScanResult(rootPath: root, accessible: true, discoveredPaths: [path("a.png")])] + + let result = Indexer.reconcileDeletions(known: known, roots: roots) + + XCTAssertTrue(result.delete.isEmpty, "orphans outside the watched root aren't this function's job") + XCTAssertTrue(result.blockedRoots.isEmpty) + } +} From b09b8112633c09890169bbd065cd1cdaac96669f Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Wed, 10 Jun 2026 09:32:28 +1000 Subject: [PATCH 2/3] Address review: keep the access-blocked CTA, restore unwatch cleanup - Carry access-blocked status on a dedicated stream instead of the progress enum. The .accessBlocked progress event was immediately overwritten by the .indexing/.watching events that the same scan emits next, so the 'grant access' CTA never rendered. It's now sticky AppState fed by accessUpdates, cleared only by a later scan. - Restore cleanup of rows from unwatched folders, which the first cut leaked. reconcileDeletions now returns deleteMissing (gone from a readable folder) and deleteOrphans (no longer under any watched root) separately; orphans are deleted unconditionally since their files still exist on disk. Orphan deletion is gated on at least one accessible root so a folder list that failed to load can't make every row look orphaned and wipe the index. - Group known paths by root in a single pass with prefixes computed once, so reconcile stays O(known) on large libraries. - Use Int.formatted() instead of a static NumberFormatter (thread-safe, no non-Sendable static). - Tests updated for the new signature, plus orphan-guard and empty-root-set cases. Co-authored-by: Claude Co-authored-by: GitButler --- Sources/Vista/AppState.swift | 21 ++-- Sources/Vista/MenuBarContentView.swift | 23 ++--- Sources/VistaCore/Indexer.swift | 99 +++++++++++++------ .../IndexerReconcileTests.swift | 59 ++++++++--- 4 files changed, 141 insertions(+), 61 deletions(-) diff --git a/Sources/Vista/AppState.swift b/Sources/Vista/AppState.swift index 1476061..5d15fdd 100644 --- a/Sources/Vista/AppState.swift +++ b/Sources/Vista/AppState.swift @@ -27,12 +27,12 @@ final class AppState { } /// Folders we hold indexed rows for but couldn't read on the last scan. - /// Non-empty drives the "grant access" messaging in the menu and panel; - /// the index itself is preserved untouched while this is set. - var accessBlockedFolders: [URL] { - if case .accessBlocked(let folders) = indexingProgress { return folders } - return [] - } + /// Sticky state, NOT derived from `indexingProgress`: each scan emits + /// `.indexing` / `.watching` after the access check, which would clobber + /// a progress-derived value before the UI rendered it. Fed instead by the + /// indexer's dedicated `accessUpdates` stream, so the "grant access" CTA + /// stays up until a later scan clears it. + var accessBlockedFolders: [URL] = [] let preferences = Preferences() @@ -94,6 +94,15 @@ final class AppState { } } + // Access-blocked stream → sticky observable property. Kept off the + // progress stream so later `.indexing`/`.watching` events can't + // clear the "grant access" CTA before it's seen. + Task { [weak self] in + for await folders in indexer.accessUpdates { + await MainActor.run { self?.accessBlockedFolders = folders } + } + } + // Preferences stream → targeted update per change. Task { [weak self] in let stream = prefs.changes diff --git a/Sources/Vista/MenuBarContentView.swift b/Sources/Vista/MenuBarContentView.swift index 05c92ab..ed15ad6 100644 --- a/Sources/Vista/MenuBarContentView.swift +++ b/Sources/Vista/MenuBarContentView.swift @@ -115,6 +115,11 @@ struct MenuBarContentView: View { } private var statusLine: String { + // Access-blocked is sticky state that outlives a single progress + // event, so it takes precedence over whatever the scan reports next. + if !appState.accessBlockedFolders.isEmpty { + return "Can't read your screenshots folder — grant access" + } switch appState.indexingProgress { case .idle: return "Vista — up to date" @@ -127,15 +132,13 @@ struct MenuBarContentView: View { // Empty queue = fully resumed from the DB, everything // was already indexed. Jump straight to the steady-state // message so the user doesn't see a flash of "0 / 0". - return "\(Self.grouped(indexed)) screenshots ready" + return "\(indexed.formatted()) screenshots ready" } // Show the work left to do plus how many are already searchable, // so a large backlog never reads as "nothing indexed". - return "Reading text from screenshots · \(Self.grouped(done)) of \(Self.grouped(total)) · \(Self.grouped(indexed)) ready" + return "Reading text from screenshots · \(done.formatted()) of \(total.formatted()) · \(indexed.formatted()) ready" case .watching(let indexed): - return "\(Self.grouped(indexed)) screenshots ready" - case .accessBlocked: - return "Can't read your screenshots folder — grant access" + return "\(indexed.formatted()) screenshots ready" } } @@ -148,16 +151,6 @@ struct MenuBarContentView: View { } } - private static let groupingFormatter: NumberFormatter = { - let f = NumberFormatter() - f.numberStyle = .decimal - return f - }() - - /// Thousands-grouped count ("6,035") for the status line. - private static func grouped(_ n: Int) -> String { - groupingFormatter.string(from: NSNumber(value: n)) ?? "\(n)" - } } extension HotKeyChord { diff --git a/Sources/VistaCore/Indexer.swift b/Sources/VistaCore/Indexer.swift index e499bb3..5db6f42 100644 --- a/Sources/VistaCore/Indexer.swift +++ b/Sources/VistaCore/Indexer.swift @@ -30,12 +30,6 @@ public actor Indexer { /// Live-watching mode. `indexed` is the current total row count /// so the UI can render "1,234 screenshots indexed". case watching(indexed: Int) - /// One or more folders we already hold indexed rows for couldn't be - /// read this scan (missing Full Disk Access, or an iCloud folder - /// that enumerated empty). We deliberately do NOT delete those rows - /// — the UI surfaces this so the user can restore access, and the - /// index resumes untouched once they do. - case accessBlocked(folders: [URL]) } private let store: ScreenshotStore @@ -62,11 +56,24 @@ public actor Indexer { // is Sendable — letting consumers `for await` without awaiting the actor. public nonisolated let progressUpdates: AsyncStream + // Separate from `progressUpdates` on purpose: access-blocked status must + // not ride the progress stream, because each scan also emits `.indexing` + // / `.watching` afterwards, which would immediately overwrite it and the + // UI would never render the "grant access" state. This stream carries the + // blocked-folder list (empty when access is fine) exactly once per scan, + // so consumers can latch it as sticky state until the next scan clears it. + private var accessContinuation: AsyncStream<[URL]>.Continuation? + public nonisolated let accessUpdates: AsyncStream<[URL]> + public init(store: ScreenshotStore, ocr: OCRRecognizer, watchedFolders: [URL]) { self.store = store self.ocr = ocr self.watchedFolders = watchedFolders + var accessCont: AsyncStream<[URL]>.Continuation! + self.accessUpdates = AsyncStream { accessCont = $0 } + self.accessContinuation = accessCont + var cont: AsyncStream.Continuation! self.progressUpdates = AsyncStream { cont = $0 } self.progressContinuation = cont @@ -98,6 +105,7 @@ public actor Indexer { eventTask?.cancel() workTask?.cancel() progressContinuation?.finish() + accessContinuation?.finish() } public func setPaused(_ paused: Bool) { @@ -210,20 +218,33 @@ public actor Indexer { // gone from a folder we could actually read. A folder that failed // to enumerate — or came back empty while we still hold rows for it // (the fresh-install / iCloud-not-materialised case) — never causes - // deletions; instead we surface `.accessBlocked` so the user can + // deletions; instead we emit it on `accessUpdates` so the user can // restore access and the index resumes untouched. The per-file // `fileExists` check is the final belt: iCloud dataless placeholders // still report true, so they're preserved even if discovery missed // them. let known = try store.pathsOnDisk() let reconcile = Self.reconcileDeletions(known: known, roots: rootScans) - for stale in reconcile.delete where !fm.fileExists(atPath: stale) { + // Files missing from a folder we could read: delete, but only after + // fileExists confirms they're really gone (iCloud placeholders report + // present, so they survive). + for stale in reconcile.deleteMissing where !fm.fileExists(atPath: stale) { try? store.delete(path: URL(fileURLWithPath: stale)) } + // Orphans belong to folders no longer watched — their files still + // exist on disk, so the fileExists guard would wrongly keep them. + // Delete unconditionally. reconcileDeletions only reports orphans when + // the watched-root set is trustworthy, so a failed-to-load folder list + // can't turn every row into an "orphan" and wipe the index. + for orphan in reconcile.deleteOrphans { + try? store.delete(path: URL(fileURLWithPath: orphan)) + } + // Emit access state every scan (empty when fine) so the UI can latch + // it as sticky state — see `accessUpdates`. if !reconcile.blockedRoots.isEmpty { VistaLog.log(" ACCESS BLOCKED for \(reconcile.blockedRoots.count) folder(s) holding indexed rows — preserving index, not deleting") - progressContinuation?.yield(.accessBlocked(folders: reconcile.blockedRoots.map { URL(fileURLWithPath: $0) })) } + accessContinuation?.yield(reconcile.blockedRoots.map { URL(fileURLWithPath: $0) }) // --- Phase 3: fingerprint-filter. Anything whose (mtime, size) // matches the DB entry is already indexed; skip without OCR. @@ -370,21 +391,42 @@ public actor Indexer { /// which roots are access-blocked. Pure so it can be unit-tested without /// touching the filesystem or OCR. /// - /// A known path is deleted only when its root was readable AND the path - /// wasn't seen this scan. A readable root that returned zero files while - /// we still hold rows under it is treated as blocked, not empty — that's - /// the fresh-install / lost-permission / iCloud-not-materialised case, - /// and deleting there is exactly the data-loss bug we're guarding. Rows - /// not under any watched root are left alone (folder removal reconciles - /// through `updateWatchedFolders`, not here). + /// Three buckets: + /// - `deleteMissing` — under a readable root but not seen this scan: the + /// file is genuinely gone. A readable root that returned zero files + /// while we still hold rows under it is treated as *blocked*, not empty + /// — the fresh-install / lost-permission / iCloud-not-materialised case, + /// and deleting there is exactly the data-loss bug we're guarding. + /// - `deleteOrphans` — not under any watched root, i.e. a folder the user + /// unwatched. Returned ONLY when at least one root was accessible, so a + /// watched-folder list that failed to load can't make every row look + /// like an orphan and wipe the index. + /// - `blockedRoots` — readable-but-untrustworthy or unreadable roots that + /// still hold rows; surfaced to the UI, never deleted from. + /// + /// Known paths are grouped by root in a single pass with prefixes computed + /// once, so this stays O(known) regardless of library size. static func reconcileDeletions( known: Set, roots: [RootScanResult] - ) -> (delete: [String], blockedRoots: [String]) { - var delete: [String] = [] + ) -> (deleteMissing: [String], deleteOrphans: [String], blockedRoots: [String]) { + var deleteMissing: [String] = [] + var deleteOrphans: [String] = [] var blockedRoots: [String] = [] - for root in roots { - let knownUnder = known.filter { isPath($0, under: root.rootPath) } + + let prefixes = roots.map { $0.rootPath.hasSuffix("/") ? $0.rootPath : $0.rootPath + "/" } + var pathsByRoot: [Int: [String]] = [:] + var orphans: [String] = [] + for path in known { + if let index = prefixes.firstIndex(where: { path.hasPrefix($0) }) { + pathsByRoot[index, default: []].append(path) + } else { + orphans.append(path) + } + } + + for (index, root) in roots.enumerated() { + let knownUnder = pathsByRoot[index] ?? [] // Circuit breaker: an "accessible" root that found nothing while // we hold rows for it is not trustworthy — never wipe on it. let trustworthy = root.accessible @@ -394,17 +436,18 @@ public actor Indexer { continue } for path in knownUnder where !root.discoveredPaths.contains(path) { - delete.append(path) + deleteMissing.append(path) } } - return (delete, blockedRoots) - } - /// True when `path` lives inside `root` (strictly below it, so the root - /// directory itself never counts as one of its own file rows). - static func isPath(_ path: String, under root: String) -> Bool { - let prefix = root.hasSuffix("/") ? root : root + "/" - return path.hasPrefix(prefix) + // Only act on orphans when the root set is trustworthy enough to trust + // the "not under any root" conclusion. With no accessible root, the + // folder list may simply have failed to resolve — preserve everything. + if roots.contains(where: { $0.accessible }) { + deleteOrphans = orphans + } + + return (deleteMissing, deleteOrphans, blockedRoots) } // MARK: - Helpers diff --git a/Tests/VistaCoreTests/IndexerReconcileTests.swift b/Tests/VistaCoreTests/IndexerReconcileTests.swift index 26b7cd8..ac5d96e 100644 --- a/Tests/VistaCoreTests/IndexerReconcileTests.swift +++ b/Tests/VistaCoreTests/IndexerReconcileTests.swift @@ -2,8 +2,8 @@ // // These cover the pure decision helper behind the data-loss fix: a folder // we couldn't read (or that came back empty while we still hold rows for it) -// must never cause deletions. Instead it's reported as access-blocked and -// the index is left intact. +// must never cause deletions. Orphans from unwatched folders are cleaned up, +// but only when the watched-root set is trustworthy. import XCTest @testable import VistaCore @@ -21,7 +21,8 @@ final class IndexerReconcileTests: XCTestCase { let result = Indexer.reconcileDeletions(known: known, roots: roots) - XCTAssertTrue(result.delete.isEmpty, "no rows should be deleted for an unreadable folder") + XCTAssertTrue(result.deleteMissing.isEmpty, "no rows should be deleted for an unreadable folder") + XCTAssertTrue(result.deleteOrphans.isEmpty) XCTAssertEqual(result.blockedRoots, [root]) } @@ -34,12 +35,13 @@ final class IndexerReconcileTests: XCTestCase { let result = Indexer.reconcileDeletions(known: known, roots: roots) - XCTAssertTrue(result.delete.isEmpty, "an empty scan must not wipe a populated index") + XCTAssertTrue(result.deleteMissing.isEmpty, "an empty scan must not wipe a populated index") + XCTAssertTrue(result.deleteOrphans.isEmpty) XCTAssertEqual(result.blockedRoots, [root]) } // A genuinely-deleted file (root readable, file no longer discovered) - // is the one case where deletion is correct. + // is the one case where missing-file deletion is correct. func testReadableRootDeletesOnlyTrulyMissingFiles() { let known: Set = [path("a.png"), path("b.png"), path("gone.png")] let discovered: Set = [path("a.png"), path("b.png")] @@ -47,7 +49,8 @@ final class IndexerReconcileTests: XCTestCase { let result = Indexer.reconcileDeletions(known: known, roots: roots) - XCTAssertEqual(result.delete, [path("gone.png")]) + XCTAssertEqual(result.deleteMissing, [path("gone.png")]) + XCTAssertTrue(result.deleteOrphans.isEmpty) XCTAssertTrue(result.blockedRoots.isEmpty) } @@ -58,7 +61,8 @@ final class IndexerReconcileTests: XCTestCase { let result = Indexer.reconcileDeletions(known: known, roots: roots) - XCTAssertTrue(result.delete.isEmpty) + XCTAssertTrue(result.deleteMissing.isEmpty) + XCTAssertTrue(result.deleteOrphans.isEmpty) XCTAssertTrue(result.blockedRoots.isEmpty) } @@ -70,19 +74,50 @@ final class IndexerReconcileTests: XCTestCase { let result = Indexer.reconcileDeletions(known: known, roots: roots) - XCTAssertTrue(result.delete.isEmpty) + // The /other/ path is an orphan, but with no accessible root we don't + // trust the "orphan" conclusion, so nothing is deleted. + XCTAssertTrue(result.deleteMissing.isEmpty) + XCTAssertTrue(result.deleteOrphans.isEmpty) XCTAssertTrue(result.blockedRoots.isEmpty, "no rows under the folder means nothing to warn about") } - // Rows not under any watched root are left alone here — removing a - // watched folder reconciles through updateWatchedFolders, not this path. - func testRowsOutsideAnyWatchedRootAreLeftAlone() { + // Rows not under any watched root are orphans from an unwatched folder. + // With a readable root present, they're cleaned up unconditionally — the + // files still exist on disk, so the fileExists guard alone wouldn't. + func testRowsOutsideAnyWatchedRootAreReturnedAsOrphans() { let known: Set = [path("a.png"), "/Users/test/elsewhere/orphan.png"] let roots = [Indexer.RootScanResult(rootPath: root, accessible: true, discoveredPaths: [path("a.png")])] let result = Indexer.reconcileDeletions(known: known, roots: roots) - XCTAssertTrue(result.delete.isEmpty, "orphans outside the watched root aren't this function's job") + XCTAssertTrue(result.deleteMissing.isEmpty) + XCTAssertEqual(result.deleteOrphans, ["/Users/test/elsewhere/orphan.png"]) + XCTAssertTrue(result.blockedRoots.isEmpty) + } + + // The orphan-wipe guard: if NO root was accessible, an apparently-orphan + // path might just mean the watched-folder list failed to load. Preserve + // everything rather than deleting on an untrustworthy root set. + func testOrphansPreservedWhenNoAccessibleRoot() { + let known: Set = [path("a.png"), "/Users/test/elsewhere/orphan.png"] + // Only an inaccessible root in the set → don't trust orphan status. + let roots = [Indexer.RootScanResult(rootPath: root, accessible: false, discoveredPaths: [])] + + let result = Indexer.reconcileDeletions(known: known, roots: roots) + + XCTAssertTrue(result.deleteOrphans.isEmpty, "no accessible root means we can't trust 'orphan'") + XCTAssertTrue(result.deleteMissing.isEmpty) + XCTAssertEqual(result.blockedRoots, [root], "the inaccessible root still holds a row → blocked") + } + + // Empty root set (folder list failed to resolve) must never wipe. + func testEmptyRootSetDeletesNothing() { + let known: Set = [path("a.png"), path("b.png")] + + let result = Indexer.reconcileDeletions(known: known, roots: []) + + XCTAssertTrue(result.deleteMissing.isEmpty) + XCTAssertTrue(result.deleteOrphans.isEmpty, "with no roots at all, every row would look orphaned — preserve") XCTAssertTrue(result.blockedRoots.isEmpty) } } From e0e6047f615cc14a486dc499ed73dfd64a4b009a Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Wed, 10 Jun 2026 09:35:37 +1000 Subject: [PATCH 3/3] Address review: run the panel search off-main, fix unowned comment - runQuery now performs store.search in a detached task and applies on the main actor, guarded by a generation token so a slower earlier search can't land on top of a newer one. reload() runs on every panel show, so the read must not block the main thread behind the indexer's writes on the shared serial queue (same reasoning as loadMore). - Correct the PanelController.appState comment: the reference is unowned to avoid the AppState <-> PanelController retain cycle, not a strong ref. Co-authored-by: Claude Co-authored-by: GitButler --- Sources/Vista/PanelController.swift | 4 ++- Sources/Vista/SearchViewModel.swift | 46 ++++++++++++++++++++--------- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/Sources/Vista/PanelController.swift b/Sources/Vista/PanelController.swift index 07884f7..dd9bfa3 100644 --- a/Sources/Vista/PanelController.swift +++ b/Sources/Vista/PanelController.swift @@ -17,7 +17,9 @@ public final class PanelController { private let actions: ActionHandlers private let preferences: Preferences // Held so the panel's empty state can react to access-blocked status. - // App-lifetime object, so the strong reference is intentional. + // `unowned` on purpose: AppState owns this PanelController, so a strong + // ref back would form a retain cycle. Both live for the whole app, so + // unowned is safe (it's never accessed after AppState is gone). private unowned let appState: AppState /// The app that was frontmost when the user invoked vista's hotkey — diff --git a/Sources/Vista/SearchViewModel.swift b/Sources/Vista/SearchViewModel.swift index 406b783..66b8168 100644 --- a/Sources/Vista/SearchViewModel.swift +++ b/Sources/Vista/SearchViewModel.swift @@ -40,6 +40,12 @@ public final class SearchViewModel { private let store: ScreenshotStore private var debounceTask: Task? + /// Bumped on every query. Each in-flight search captures the value at + /// launch and only applies its results if it's still current — so a + /// slower earlier search can't land on top of a newer one when several + /// reloads / keystrokes overlap. + private var queryGeneration = 0 + public init(store: ScreenshotStore) { self.store = store reload() @@ -91,20 +97,32 @@ public final class SearchViewModel { } private func runQuery(_ text: String) { - do { - let query = QueryParser.parse(text) - let page = try store.search(query, limit: pageSize) - self.results = page - self.selectedIndex = 0 - // A full page means there may be more behind it; a short page - // (or empty) means we've hit the end of the index. - self.canLoadMore = page.count == pageSize - self.isLoadingMore = false - } catch { - NSLog("vista: search failed: \(error)") - self.results = [] - self.canLoadMore = false - self.isLoadingMore = false + queryGeneration &+= 1 + let generation = queryGeneration + let query = QueryParser.parse(text) + Task { [weak self, store, pageSize] in + do { + // Run the read off the main actor: `store`'s serial queue is + // shared with the indexer's writes, so a synchronous search on + // the main thread can stall the UI behind a write batch — and + // this now runs on every panel show, not just on keystrokes. + let page = try await Task.detached(priority: .userInitiated) { + try store.search(query, limit: pageSize) + }.value + guard let self, generation == self.queryGeneration else { return } + self.results = page + self.selectedIndex = 0 + // A full page means there may be more behind it; a short page + // (or empty) means we've hit the end of the index. + self.canLoadMore = page.count == pageSize + self.isLoadingMore = false + } catch { + guard let self, generation == self.queryGeneration else { return } + NSLog("vista: search failed: \(error)") + self.results = [] + self.canLoadMore = false + self.isLoadingMore = false + } } }