From 31c34a1d4a951052c6b1c1bd9245d6e472d65df8 Mon Sep 17 00:00:00 2001 From: Theo Weijmer Date: Thu, 1 Oct 2026 16:04:01 +0200 Subject: [PATCH] fix: history load notifies and never overwrites unreadable file; dictionary keeps ids across external edits, undo safety Co-Authored-By: Claude Sonnet 5.5 --- Sources/SstData/DictionaryFile.swift | 15 +++++++ Sources/SstData/DictionaryStore.swift | 5 ++- Sources/SstData/History.swift | 21 ++++++++-- Sources/SstData/ViewSupport.swift | 11 +++++- Tests/SstDataTests/DictionaryFileTests.swift | 19 +++++++++ Tests/SstDataTests/DictionaryStoreTests.swift | 17 ++++++++ Tests/SstDataTests/HistoryTests.swift | 29 ++++++++++++++ Tests/SstDataTests/ViewSupportTests.swift | 39 +++++++++++++++++++ 8 files changed, 150 insertions(+), 6 deletions(-) diff --git a/Sources/SstData/DictionaryFile.swift b/Sources/SstData/DictionaryFile.swift index 2185841..8418908 100644 --- a/Sources/SstData/DictionaryFile.swift +++ b/Sources/SstData/DictionaryFile.swift @@ -70,6 +70,21 @@ public struct DictionaryFile: Equatable, Sendable { return .entry(DictionaryEntry(kind: .term(trimmed))) } + /// Reuses the ids of `old` for entries with equal content (first unused match, in order), so ids survive a reload. + public func preservingIDs(from old: DictionaryFile) -> DictionaryFile { + var available = old.entries + var result = self + result.lines = lines.map { line in + guard case .entry(let entry) = line else { return line } + if let index = available.firstIndex(where: { $0.kind == entry.kind }) { + let reused = available.remove(at: index) + return .entry(DictionaryEntry(id: reused.id, kind: entry.kind)) + } + return line + } + return result + } + public func serialized() -> String { let out = lines.map { line -> String in switch line { diff --git a/Sources/SstData/DictionaryStore.swift b/Sources/SstData/DictionaryStore.swift index 20072c1..c846ed1 100644 --- a/Sources/SstData/DictionaryStore.swift +++ b/Sources/SstData/DictionaryStore.swift @@ -14,6 +14,8 @@ public final class DictionaryStore { public let fileURL: URL public private(set) var file = DictionaryFile() public var onChange: (() -> Void)? + /// Fired (after `onChange`) when the file was changed by something other than this store. + public var onExternalChange: (() -> Void)? private var lastText = "" private var watcher: DispatchSourceFileSystemObject? @@ -53,8 +55,9 @@ public final class DictionaryStore { public func reloadIfChanged() { guard let text = try? String(contentsOf: fileURL, encoding: .utf8), text != lastText else { return } lastText = text - file = DictionaryFile(parsing: text) + file = DictionaryFile(parsing: text).preservingIDs(from: file) onChange?() + onExternalChange?() } /// Watches the folder (editors replace files instead of editing them in place). diff --git a/Sources/SstData/History.swift b/Sources/SstData/History.swift index d97caf3..eb888e5 100644 --- a/Sources/SstData/History.swift +++ b/Sources/SstData/History.swift @@ -35,6 +35,8 @@ public final class HistoryStore { case empty case loaded case recoveredCorrupt(backup: URL) + /// The file exists but cannot be read; it is left alone and nothing is written over it. + case unreadable(String) } private struct Document: Codable { @@ -47,6 +49,7 @@ public final class HistoryStore { public var onChange: (() -> Void)? private let fileURL: URL + private var readOnly = false private var saveTask: Task? public init(fileURL: URL) { @@ -68,18 +71,29 @@ public final class HistoryStore { @discardableResult public func load() -> LoadOutcome { - guard let data = try? Data(contentsOf: fileURL) else { - entries = [] - return .empty + let data: Data + do { + data = try Data(contentsOf: fileURL) + } catch { + if !FileManager.default.fileExists(atPath: fileURL.path) { + entries = [] + onChange?() + return .empty + } + readOnly = true + lastError = error.localizedDescription + return .unreadable(error.localizedDescription) } do { entries = try Self.makeDecoder().decode(Document.self, from: data).entries + onChange?() return .loaded } catch { let stamp = ISO8601DateFormatter().string(from: Date()).replacingOccurrences(of: ":", with: "-") let backup = fileURL.deletingLastPathComponent().appending(path: "history.corrupt-\(stamp).json") try? FileManager.default.moveItem(at: fileURL, to: backup) entries = [] + onChange?() return .recoveredCorrupt(backup: backup) } } @@ -103,6 +117,7 @@ public final class HistoryStore { public func flush() { saveTask?.cancel() saveTask = nil + guard !readOnly else { return } do { try FileManager.default.createDirectory( at: fileURL.deletingLastPathComponent(), withIntermediateDirectories: true) diff --git a/Sources/SstData/ViewSupport.swift b/Sources/SstData/ViewSupport.swift index c34fa91..f7cfbd5 100644 --- a/Sources/SstData/ViewSupport.swift +++ b/Sources/SstData/ViewSupport.swift @@ -161,16 +161,23 @@ public final class DictionaryViewModel: ObservableObject { } public func undo() { - guard let snapshot = undoStack.popLast() else { return } - canUndo = !undoStack.isEmpty + guard let snapshot = undoStack.last else { return } do { try store.apply { $0 = snapshot } + undoStack.removeLast() + canUndo = !undoStack.isEmpty } catch { errorMessage = error.localizedDescription } reload() } + /// The file was edited elsewhere: older snapshots no longer describe it, so undo is dropped. + public func externalChangeDetected() { + undoStack.removeAll() + canUndo = false + } + private func mutate(_ change: (inout DictionaryFile) -> Void) { let snapshot = store.file do { diff --git a/Tests/SstDataTests/DictionaryFileTests.swift b/Tests/SstDataTests/DictionaryFileTests.swift index 6d0fcd6..e666d14 100644 --- a/Tests/SstDataTests/DictionaryFileTests.swift +++ b/Tests/SstDataTests/DictionaryFileTests.swift @@ -49,4 +49,23 @@ struct DictionaryFileTests { #expect(file.entries.isEmpty) #expect(file.serialized() == "# c\n") } + + @Test func preservingIDsKeepsIdsOfUnchangedLines() { + let old = DictionaryFile(parsing: "a\nb => c\nd\n") + let new = DictionaryFile(parsing: "z\na\nb => c\nd2\n").preservingIDs(from: old) + let oldIDs = old.entries.map(\.id), newIDs = new.entries.map(\.id) + #expect(newIDs[1] == oldIDs[0]) + #expect(newIDs[2] == oldIDs[1]) + #expect(!oldIDs.contains(newIDs[0])) + #expect(!oldIDs.contains(newIDs[3])) + #expect(new.serialized() == "z\na\nb => c\nd2\n") + } + + @Test func preservingIDsMapsDuplicatesOneToOneInOrder() { + let old = DictionaryFile(parsing: "a\na\n") + let new = DictionaryFile(parsing: "a\na\na\n").preservingIDs(from: old) + let oldIDs = old.entries.map(\.id), newIDs = new.entries.map(\.id) + #expect(Array(newIDs[0...1]) == oldIDs) + #expect(!oldIDs.contains(newIDs[2])) + } } diff --git a/Tests/SstDataTests/DictionaryStoreTests.swift b/Tests/SstDataTests/DictionaryStoreTests.swift index f614813..7f84126 100644 --- a/Tests/SstDataTests/DictionaryStoreTests.swift +++ b/Tests/SstDataTests/DictionaryStoreTests.swift @@ -43,6 +43,23 @@ struct DictionaryStoreTests { #expect(changes == 1) } + @Test func externalChangeFiresBothCallbacksAndKeepsIDs() throws { + let url = tempFile() + let store = DictionaryStore(fileURL: url) + try store.load() + try store.apply { $0.append(.term("Vercel")) } + let id = store.entries[0].id + var changes = 0, external = 0 + store.onChange = { changes += 1 } + store.onExternalChange = { external += 1 } + try "Nieuw\nVercel\n".write(to: url, atomically: true, encoding: .utf8) + store.reloadIfChanged() + #expect(changes == 1 && external == 1) + #expect(store.entries[1].id == id) + try store.apply { $0.append(.term("Eigen")) } + #expect(changes == 2 && external == 1) + } + @Test func ourOwnWriteDoesNotTriggerAReload() throws { let store = DictionaryStore(fileURL: tempFile()) try store.load() diff --git a/Tests/SstDataTests/HistoryTests.swift b/Tests/SstDataTests/HistoryTests.swift index 13c06e2..2571c8c 100644 --- a/Tests/SstDataTests/HistoryTests.swift +++ b/Tests/SstDataTests/HistoryTests.swift @@ -57,6 +57,35 @@ struct HistoryStoreTests { #expect(store.entries.isEmpty) } + @Test func loadNotifiesOnChange() { + let url = tempFile() + let writer = HistoryStore(fileURL: url) + writer.add(entry("a", at: 0)); writer.add(entry("b", at: 1)) + writer.flush() + + let store = HistoryStore(fileURL: url) + var calls = 0 + store.onChange = { calls += 1 } + #expect(store.load() == .loaded) + #expect(calls == 1) + #expect(store.entries.count == 2) + } + + @Test func unreadableFileIsReportedAndNeverOverwritten() throws { + let url = tempFile() + try FileManager.default.createDirectory(at: url, withIntermediateDirectories: true) // history.json is a directory + let store = HistoryStore(fileURL: url) + guard case .unreadable = store.load() else { + Issue.record("expected unreadable"); return + } + store.add(entry("a", at: 0)) + store.flush() + var isDirectory: ObjCBool = false + #expect(FileManager.default.fileExists(atPath: url.path, isDirectory: &isDirectory)) + #expect(isDirectory.boolValue) + #expect(store.lastError != nil) + } + @Test func onChangeFiresForEdits() { let store = HistoryStore(fileURL: tempFile()) var calls = 0 diff --git a/Tests/SstDataTests/ViewSupportTests.swift b/Tests/SstDataTests/ViewSupportTests.swift index c1b8ba7..47f8e3a 100644 --- a/Tests/SstDataTests/ViewSupportTests.swift +++ b/Tests/SstDataTests/ViewSupportTests.swift @@ -67,6 +67,20 @@ struct HistoryViewModelTests { return store } + @Test func daysAreFilledAfterTheStoreLoads() { + let url = FileManager.default.temporaryDirectory.appending(path: "sst-vm-\(UUID().uuidString)/h.json") + let writer = HistoryStore(fileURL: url) + writer.add(HistoryEntry(date: .now, duration: 1, rawText: "x", finalText: "x")) + writer.flush() + let store = HistoryStore(fileURL: url) + let model = HistoryViewModel(store: store) + store.onChange = { model.refresh() } + #expect(model.days.isEmpty) + store.load() + #expect(!model.days.isEmpty) + #expect(model.totalCount == 1) + } + @Test func listsAllEntriesNewestFirstAndFiltersByQuery() { let model = HistoryViewModel(store: store(["eerste", "tweede zin", "derde"])) #expect(model.totalCount == 3) @@ -152,6 +166,31 @@ struct DictionaryViewModelTests { #expect(store.entries == [term("Supabase")]) } + @Test func externalChangeClearsUndo() throws { + let (model, store) = try makeModel("Supabase\n") + model.add(.term("Vercel")) + #expect(model.canUndo) + model.externalChangeDetected() + #expect(!model.canUndo) + model.undo() + #expect(store.entries == [term("Supabase"), term("Vercel")]) + } + + @Test func failedUndoKeepsTheSnapshot() throws { + let (model, store) = try makeModel("Supabase\n") + model.add(.term("Vercel")) + let folder = store.fileURL.deletingLastPathComponent() + try FileManager.default.setAttributes([.posixPermissions: 0o500], ofItemAtPath: folder.path) + model.undo() + try FileManager.default.setAttributes([.posixPermissions: 0o700], ofItemAtPath: folder.path) + #expect(model.errorMessage != nil) + #expect(model.canUndo) + model.errorMessage = nil + model.undo() + #expect(store.entries == [term("Supabase")]) + #expect(!model.canUndo) + } + @Test func searchMatchesBothSidesIgnoringCaseAndDiacritics() throws { let (model, _) = try makeModel("cloud code => Claude Code\nCafé\nVercel\n") model.query = "CLAUDE"