From ede78a3ee1f5d4aaa3623703d8741f500c3469ea Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Mon, 3 Aug 2026 20:58:54 -0700 Subject: [PATCH 01/10] prep examples for indepedent windows --- Examples/UndoForMacOS/UndoForMacOSApp.swift | 31 +++++++++++++-------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/Examples/UndoForMacOS/UndoForMacOSApp.swift b/Examples/UndoForMacOS/UndoForMacOSApp.swift index 8546116..7fbab7a 100644 --- a/Examples/UndoForMacOS/UndoForMacOSApp.swift +++ b/Examples/UndoForMacOS/UndoForMacOSApp.swift @@ -5,11 +5,6 @@ import SwiftUI @main struct UndoForMacOSApp: App { - static let store = Store( - initialState: DemoFeature.State() - ) { - DemoFeature() - } init() { prepareDependencies { let database = try! makeDemoDatabase() @@ -22,7 +17,11 @@ struct UndoForMacOSApp: App { } var body: some Scene { WindowGroup { - DemoView(store: Self.store) + DemoView(store: Store( + initialState: DemoFeature.State() + ) { + DemoFeature() + }) } } } @@ -31,8 +30,16 @@ struct UndoForMacOSApp: App { struct DemoFeature { @ObservableState struct State { - @FetchAll(DemoItem.all) var items: [DemoItem] + let windowID: UUID var eventLog: [UndoEvent] = [] + @FetchAll(DemoItem.none) var items: [DemoItem] + init(windowID: UUID = UUID()) { + self._items = FetchAll( + wrappedValue: [], + DemoItem.all.where { $0.windowID.eq(windowID) } + ) + self.windowID = windowID + } } enum Action: UndoManageableAction { @@ -71,18 +78,18 @@ struct DemoFeature { try undoable("Add Item") { try database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") }.execute(db) } } } return .none case .addItemInBackground: - return .run { _ in + return .run { [windowID = state.windowID] _ in try await undoable("Add Item (Background)") { try await database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { DemoItem(id: nextID, windowID: windowID, name: "Item \(nextID)") }.execute(db) } } } @@ -92,7 +99,7 @@ struct DemoFeature { try withUndoDisabled { try database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") }.execute(db) } } } @@ -322,6 +329,7 @@ final class ObservableUndoManager { @Table struct DemoItem: Identifiable { var id: Int + var windowID: UUID var name: String = "" var count: Int = 0 } @@ -339,6 +347,7 @@ func makeDemoDatabase() throws -> any DatabaseWriter { """ CREATE TABLE "demoItems" ( "id" INTEGER PRIMARY KEY, + "windowID" TEXT NOT NULL, "name" TEXT NOT NULL DEFAULT '', "count" INTEGER NOT NULL DEFAULT 0 ) From 6aebc2592d68221765f9bee23e114bf4395da6d8 Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 07:33:35 -0700 Subject: [PATCH 02/10] update tests to use withBarrier --- Sources/SQLiteUndo/UndoCoordinator.swift | 39 ++ .../SQLiteUndoTests/CascadeTriggerTests.swift | 118 +++--- Tests/SQLiteUndoTests/ForeignKeyTests.swift | 66 ++-- .../SQLiteUndoTests/UndoBenchmarkTests.swift | 22 +- Tests/SQLiteUndoTests/UndoEngineTests.swift | 351 +++++++++--------- .../UnregisteredTableTests.swift | 35 +- 6 files changed, 328 insertions(+), 303 deletions(-) diff --git a/Sources/SQLiteUndo/UndoCoordinator.swift b/Sources/SQLiteUndo/UndoCoordinator.swift index 8d9e41e..adc635f 100644 --- a/Sources/SQLiteUndo/UndoCoordinator.swift +++ b/Sources/SQLiteUndo/UndoCoordinator.swift @@ -178,6 +178,45 @@ final class UndoCoordinator: Sendable { } } + /// Run an operation inside a barrier, returning the completed barrier. + /// + /// The barrier is cancelled if the operation throws. Changes must be made + /// within the operation to be captured. + /// + /// - Returns: The completed barrier, or nil if no changes were captured. + @discardableResult + func withBarrier(_ name: String, _ operation: () throws -> Void) throws -> UndoBarrier? { + let id = try beginBarrier(name) + do { + try operation() + return try endBarrier(id) + } catch { + try cancelBarrier(id) + throw error + } + } + + /// Run an async operation inside a barrier, returning the completed barrier. + /// + /// The barrier is cancelled if the operation throws. Changes must be made + /// within the operation to be captured. + /// + /// - Returns: The completed barrier, or nil if no changes were captured. + @discardableResult + func withBarrier( + _ name: String, + _ operation: @Sendable () async throws -> Void + ) async throws -> UndoBarrier? { + let id = try beginBarrier(name) + do { + try await operation() + return try endBarrier(id) + } catch { + try cancelBarrier(id) + throw error + } + } + /// Cancel a barrier without registering it for undo. /// /// Any changes made within the barrier remain in the database but won't diff --git a/Tests/SQLiteUndoTests/CascadeTriggerTests.swift b/Tests/SQLiteUndoTests/CascadeTriggerTests.swift index 1f8544e..840271a 100644 --- a/Tests/SQLiteUndoTests/CascadeTriggerTests.swift +++ b/Tests/SQLiteUndoTests/CascadeTriggerTests.swift @@ -25,14 +25,14 @@ struct CascadeTriggerTests { } } - let barrierId = try engine.beginBarrier("Update Value") - try database.write { db in - try db.execute( - sql: """ - UPDATE "cascadeItems" SET "value" = 'changed' WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update Value") { + try database.write { db in + try db.execute( + sql: """ + UPDATE "cascadeItems" SET "value" = 'changed' WHERE "id" = 1 + """) + } + }! // Verify the cascade fired: flag should be 1 try database.read { db in @@ -73,14 +73,14 @@ struct CascadeTriggerTests { } } - let barrierId = try engine.beginBarrier("Update A") - try database.write { db in - try db.execute( - sql: """ - UPDATE "cascadeItems" SET "value" = 'A-changed' WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update A") { + try database.write { db in + try db.execute( + sql: """ + UPDATE "cascadeItems" SET "value" = 'A-changed' WHERE "id" = 1 + """) + } + }! // Verify cascade: row A updated, row B got flag=1 try database.read { db in @@ -111,18 +111,18 @@ struct CascadeTriggerTests { func insertThenDeleteIsNoOp() throws { let (database, engine) = try makeCascadeDatabase(trigger: .none) - let barrierId = try engine.beginBarrier("Insert Then Delete") - try database.write { db in - try db.execute( - sql: """ - INSERT INTO "cascadeItems" ("id", "value", "flag") VALUES (1, 'temp', 0) - """) - try db.execute( - sql: """ - DELETE FROM "cascadeItems" WHERE "id" = 1 - """) + let barrier = try engine.withBarrier("Insert Then Delete") { + try database.write { db in + try db.execute( + sql: """ + INSERT INTO "cascadeItems" ("id", "value", "flag") VALUES (1, 'temp', 0) + """) + try db.execute( + sql: """ + DELETE FROM "cascadeItems" WHERE "id" = 1 + """) + } } - let barrier = try engine.endBarrier(barrierId) // The barrier may be nil (if reconciliation removes all entries) // or non-nil but undo should be a no-op @@ -140,18 +140,18 @@ struct CascadeTriggerTests { func insertThenUpdateUndoDeletesRow() throws { let (database, engine) = try makeCascadeDatabase(trigger: .none) - let barrierId = try engine.beginBarrier("Insert Then Update") - try database.write { db in - try db.execute( - sql: """ - INSERT INTO "cascadeItems" ("id", "value", "flag") VALUES (1, 'initial', 0) - """) - try db.execute( - sql: """ - UPDATE "cascadeItems" SET "value" = 'modified' WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Insert Then Update") { + try database.write { db in + try db.execute( + sql: """ + INSERT INTO "cascadeItems" ("id", "value", "flag") VALUES (1, 'initial', 0) + """) + try db.execute( + sql: """ + UPDATE "cascadeItems" SET "value" = 'modified' WHERE "id" = 1 + """) + } + }! try database.read { db in let item = try CascadeItem.find(1).fetchOne(db)! @@ -180,18 +180,18 @@ struct CascadeTriggerTests { } } - let barrierId = try engine.beginBarrier("Update Then Delete") - try database.write { db in - try db.execute( - sql: """ - UPDATE "cascadeItems" SET "value" = 'modified' WHERE "id" = 1 - """) - try db.execute( - sql: """ - DELETE FROM "cascadeItems" WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update Then Delete") { + try database.write { db in + try db.execute( + sql: """ + UPDATE "cascadeItems" SET "value" = 'modified' WHERE "id" = 1 + """) + try db.execute( + sql: """ + DELETE FROM "cascadeItems" WHERE "id" = 1 + """) + } + }! try database.read { db in let count = try CascadeItem.all.fetchCount(db) @@ -221,14 +221,14 @@ struct CascadeTriggerTests { } } - let barrierId = try engine.beginBarrier("Update") - try database.write { db in - try db.execute( - sql: """ - UPDATE "cascadeItems" SET "value" = 'changed' WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update") { + try database.write { db in + try db.execute( + sql: """ + UPDATE "cascadeItems" SET "value" = 'changed' WHERE "id" = 1 + """) + } + }! // Undo try engine.performUndo(barrier: barrier) diff --git a/Tests/SQLiteUndoTests/ForeignKeyTests.swift b/Tests/SQLiteUndoTests/ForeignKeyTests.swift index 4cb97fa..809e737 100644 --- a/Tests/SQLiteUndoTests/ForeignKeyTests.swift +++ b/Tests/SQLiteUndoTests/ForeignKeyTests.swift @@ -22,15 +22,15 @@ struct ForeignKeyTests { } } - let barrierId = try engine.beginBarrier("Delete Both") - try database.write { db in - try db.execute( - sql: """ - DELETE FROM "children" WHERE "id" = 1; - DELETE FROM "parents" WHERE "id" = 1; - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Delete Both") { + try database.write { db in + try db.execute( + sql: """ + DELETE FROM "children" WHERE "id" = 1; + DELETE FROM "parents" WHERE "id" = 1; + """) + } + }! try engine.performUndo(barrier: barrier) @@ -58,14 +58,14 @@ struct ForeignKeyTests { } } - let barrierId = try engine.beginBarrier("Delete Parent") - try database.write { db in - try db.execute( - sql: """ - DELETE FROM "parents" WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Delete Parent") { + try database.write { db in + try db.execute( + sql: """ + DELETE FROM "parents" WHERE "id" = 1 + """) + } + }! try engine.performUndo(barrier: barrier) @@ -94,14 +94,14 @@ struct ForeignKeyTests { } } - let barrierId = try engine.beginBarrier("Delete Parent") - try database.write { db in - try db.execute( - sql: """ - DELETE FROM "parents" WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Delete Parent") { + try database.write { db in + try db.execute( + sql: """ + DELETE FROM "parents" WHERE "id" = 1 + """) + } + }! let counts = try database.read { db in (try Parent.all.fetchCount(db), try Child.all.fetchCount(db)) @@ -137,14 +137,14 @@ struct ForeignKeyTests { } } - let barrierId = try engine.beginBarrier("Delete Parent") - try database.write { db in - try db.execute( - sql: """ - DELETE FROM "parents" WHERE "id" = 1 - """) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Delete Parent") { + try database.write { db in + try db.execute( + sql: """ + DELETE FROM "parents" WHERE "id" = 1 + """) + } + }! // Undo — restore parent and child try engine.performUndo(barrier: barrier) diff --git a/Tests/SQLiteUndoTests/UndoBenchmarkTests.swift b/Tests/SQLiteUndoTests/UndoBenchmarkTests.swift index 13c8dca..765c760 100644 --- a/Tests/SQLiteUndoTests/UndoBenchmarkTests.swift +++ b/Tests/SQLiteUndoTests/UndoBenchmarkTests.swift @@ -41,13 +41,13 @@ private func measureInsert(rows: Int, batched: Bool) throws -> Double { let (database, engine) = try makeUndoBenchmarkDatabase() // Insert rows in one barrier - let barrierId = try engine.beginBarrier("Insert") - try database.write { db in - for i in 1...rows { - try BenchRecord.insert { BenchRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + let barrier = try engine.withBarrier("Insert") { + try database.write { db in + for i in 1...rows { + try BenchRecord.insert { BenchRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! _undoBatchingDisabled = !batched @@ -81,11 +81,11 @@ private func measureUpdate(rows: Int, batched: Bool) throws -> Double { } // Update all rows in one barrier - let barrierId = try engine.beginBarrier("Update") - try database.write { db in - try BenchRecord.all.update { $0.value = 42 }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update") { + try database.write { db in + try BenchRecord.all.update { $0.value = 42 }.execute(db) + } + }! _undoBatchingDisabled = !batched diff --git a/Tests/SQLiteUndoTests/UndoEngineTests.swift b/Tests/SQLiteUndoTests/UndoEngineTests.swift index 58170a2..2520ade 100644 --- a/Tests/SQLiteUndoTests/UndoEngineTests.swift +++ b/Tests/SQLiteUndoTests/UndoEngineTests.swift @@ -68,14 +68,12 @@ enum UndoEngineTests { func beginAndEndBarrier() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Test Action") - #expect(barrierId != UUID()) - - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + let barrier = try engine.withBarrier("Test Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } } - let barrier = try engine.endBarrier(barrierId) #expect(barrier != nil) #expect(barrier?.name == "Test Action") #expect(barrier?.count ?? 0 > 0) @@ -85,8 +83,7 @@ enum UndoEngineTests { func endBarrierWithNoChanges() throws { let (_, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Empty Action") - let barrier = try engine.endBarrier(barrierId) + let barrier = try engine.withBarrier("Empty Action") {} #expect(barrier == nil) } @@ -95,14 +92,16 @@ enum UndoEngineTests { func cancelBarrier() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Cancelled Action") - - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + struct CancelError: Error {} + #expect(throws: CancelError.self) { + try engine.withBarrier("Cancelled Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + throw CancelError() + } } - try engine.cancelBarrier(barrierId) - // Verify the undolog entries were removed let undoLogCount = try database.read { db in try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") @@ -118,11 +117,11 @@ enum UndoEngineTests { func undoInsert() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Insert Item") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Insert Item") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -147,14 +146,14 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Update Item") - try database.write { db in - try TestRecord.find(1).update { - $0.name = "Updated" - $0.value = 20 - }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update Item") { + try database.write { db in + try TestRecord.find(1).update { + $0.name = "Updated" + $0.value = 20 + }.execute(db) + } + }! try database.read { db in let record = try TestRecord.find(1).fetchOne(db)! @@ -181,11 +180,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Delete Item") - try database.write { db in - try TestRecord.find(1).delete().execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Delete Item") { + try database.write { db in + try TestRecord.find(1).delete().execute(db) + } + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -211,11 +210,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Set Value") - try database.write { db in - try TestRecord.find(1).update { $0.value = 100 }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Set Value") { + try database.write { db in + try TestRecord.find(1).update { $0.value = 100 }.execute(db) + } + }! try engine.performUndo(barrier: barrier) @@ -236,13 +235,13 @@ enum UndoEngineTests { func multipleChangesInOneBarrier() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Batch Insert") - try database.write { db in - for i in 1...5 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + let barrier = try engine.withBarrier("Batch Insert") { + try database.write { db in + for i in 1...5 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -292,11 +291,11 @@ enum UndoEngineTests { } // Normal insert — trigger should fire - let barrierId = try engine.beginBarrier("Insert") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Alice") }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Alice") }.execute(db) + } + }! try database.read { db in let actions = try String.fetchAll(db, sql: "SELECT action FROM auditLog ORDER BY id") @@ -357,13 +356,12 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier("Set Name") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + try undoable("Set Name") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } } - try undoEngine.endBarrier(barrierId) #expect(testUndoManager.canUndo == true) #expect(testUndoManager.undoActionName == "Set Name") @@ -380,13 +378,12 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier("Insert") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + try undoable("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } } - try undoEngine.endBarrier(barrierId) let countBefore = try database.read { db in try TestRecord.all.fetchCount(db) } #expect(countBefore == 1) @@ -408,7 +405,6 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine try withUndoDisabled { try database.write { db in @@ -416,11 +412,11 @@ enum UndoEngineTests { } } - let barrierId = try undoEngine.beginBarrier("Update") - try database.write { db in - try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + try undoable("Update") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + } } - try undoEngine.endBarrier(barrierId) testUndoManager.undo() @@ -448,21 +444,18 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - // Create item 1 - let barrierId1 = try undoEngine.beginBarrier("Create Item 1") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + try undoable("Create Item 1") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) - // Create item 2 - let barrierId2 = try undoEngine.beginBarrier("Create Item 2") - try database.write { db in - try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + try undoable("Create Item 2") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) // Verify both items exist #expect(try database.read { db in try TestRecord.all.fetchCount(db) } == 2) @@ -508,16 +501,14 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - - let barrierId = try undoEngine.beginBarrier("Background Insert") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - // End the barrier from a background thread + // Run the whole barrier from a background thread DispatchQueue.global().sync { - try! undoEngine.endBarrier(barrierId) + try! undoable("Background Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + } } #expect(testUndoManager.canUndo == true) @@ -542,27 +533,26 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine @Dependency(\.defaultUndoStack) var undoStack // Initial state #expect(undoStack.currentState() == UndoStackState(undo: [], redo: [])) // Do "A" - let barrierId1 = try undoEngine.beginBarrier("A") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "A") }.execute(db) + try undoable("A") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "A") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) #expect(undoStack.currentState() == UndoStackState(undo: ["A"], redo: [])) // Do "B" - let barrierId2 = try undoEngine.beginBarrier("B") - try database.write { db in - try TestRecord.find(1).update { $0.name = "B" }.execute(db) + try undoable("B") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "B" }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) #expect(undoStack.currentState() == UndoStackState(undo: ["B", "A"], redo: [])) @@ -583,11 +573,11 @@ enum UndoEngineTests { #expect(undoStack.currentState() == UndoStackState(undo: ["B", "A"], redo: [])) // Do "C" - should clear redo stack - let barrierId3 = try undoEngine.beginBarrier("C") - try database.write { db in - try TestRecord.find(1).update { $0.name = "C" }.execute(db) + try undoable("C") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "C" }.execute(db) + } } - try undoEngine.endBarrier(barrierId3) #expect(undoStack.currentState() == UndoStackState(undo: ["C", "B", "A"], redo: [])) @@ -595,11 +585,11 @@ enum UndoEngineTests { testUndoManager.undo() #expect(undoStack.currentState() == UndoStackState(undo: ["B", "A"], redo: ["C"])) - let barrierId4 = try undoEngine.beginBarrier("D") - try database.write { db in - try TestRecord.find(1).update { $0.name = "D" }.execute(db) + try undoable("D") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "D" }.execute(db) + } } - try undoEngine.endBarrier(barrierId4) #expect(undoStack.currentState() == UndoStackState(undo: ["D", "B", "A"], redo: [])) } @@ -629,19 +619,19 @@ enum UndoEngineTests { func tracksUndoableActions() throws { #expect(undoStack.currentState() == []) - let barrierId1 = try undoEngine.beginBarrier("Add Item") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + try undoable("Add Item") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) #expect(undoStack.currentState() == ["Add Item"]) - let barrierId2 = try undoEngine.beginBarrier("Update Item") - try database.write { db in - try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + try undoable("Update Item") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) // Most recent first #expect(undoStack.currentState() == ["Update Item", "Add Item"]) @@ -649,20 +639,20 @@ enum UndoEngineTests { @Test func newActionClearsRedoStack() throws { - let barrierId1 = try undoEngine.beginBarrier("First Action") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + try undoable("First Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) #expect(undoStack.currentState() == ["First Action"]) // New action should clear redo stack (even though we can't undo in test mode) - let barrierId2 = try undoEngine.beginBarrier("Second Action") - try database.write { db in - try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + try undoable("Second Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) // Most recent first #expect(undoStack.currentState() == ["Second Action", "First Action"]) @@ -670,9 +660,8 @@ enum UndoEngineTests { @Test func emptyBarrierNotTracked() throws { - let barrierId = try undoEngine.beginBarrier("Empty Action") // No database changes - try undoEngine.endBarrier(barrierId) + try undoable("Empty Action") {} #expect(undoStack.currentState() == []) } @@ -684,13 +673,13 @@ enum UndoEngineTests { func bulkInsertUndoRedo() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Bulk Insert") - try database.write { db in - for i in 1...1000 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + let barrier = try engine.withBarrier("Bulk Insert") { + try database.write { db in + for i in 1...1000 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -730,11 +719,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Bulk Delete") - try database.write { db in - try TestRecord.all.delete().execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Bulk Delete") { + try database.write { db in + try TestRecord.all.delete().execute(db) + } + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -771,11 +760,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Bulk Update") - try database.write { db in - try TestRecord.all.update { $0.value = 42 }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Bulk Update") { + try database.write { db in + try TestRecord.all.update { $0.value = 42 }.execute(db) + } + }! try engine.performUndo(barrier: barrier) @@ -798,19 +787,19 @@ enum UndoEngineTests { func bulkMixedOperations() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Mixed Ops") - try database.write { db in - for i in 1...500 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) - } - for i in 1...250 { - try TestRecord.find(i).update { $0.value = 99 }.execute(db) - } - for i in 251...500 { - try TestRecord.find(i).delete().execute(db) + let barrier = try engine.withBarrier("Mixed Ops") { + try database.write { db in + for i in 1...500 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + } + for i in 1...250 { + try TestRecord.find(i).update { $0.value = 99 }.execute(db) + } + for i in 251...500 { + try TestRecord.find(i).delete().execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -842,11 +831,11 @@ enum UndoEngineTests { func eventEmittedOnUndo() async throws { let (database, coordinator) = try makeTestDatabaseWithUndo() - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! + let barrier = try await coordinator.withBarrier("Insert Item") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! var iterator = coordinator.events().makeAsyncIterator() try coordinator.performUndo(barrier: barrier) @@ -865,11 +854,11 @@ enum UndoEngineTests { func eventEmittedOnRedo() async throws { let (database, coordinator) = try makeTestDatabaseWithUndo() - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! + let barrier = try await coordinator.withBarrier("Insert Item") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! var iterator = coordinator.events().makeAsyncIterator() try coordinator.performUndo(barrier: barrier) @@ -891,13 +880,13 @@ enum UndoEngineTests { func affectedItemsForMultiRowBarrier() async throws { let (database, coordinator) = try makeTestDatabaseWithUndo() - let barrierId = try coordinator.beginBarrier("Batch Insert") - try await database.write { db in - for i in 1...3 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + let barrier = try await coordinator.withBarrier("Batch Insert") { + try await database.write { db in + for i in 1...3 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + } } - } - let barrier = try coordinator.endBarrier(barrierId)! + }! var iterator = coordinator.events().makeAsyncIterator() try coordinator.performUndo(barrier: barrier) @@ -923,11 +912,11 @@ enum UndoEngineTests { var first = coordinator.events().makeAsyncIterator() var second = coordinator.events().makeAsyncIterator() - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! + let barrier = try await coordinator.withBarrier("Insert Item") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! try coordinator.performUndo(barrier: barrier) let firstEvent = await first.next() @@ -947,11 +936,11 @@ enum UndoEngineTests { var iterator = coordinator.events().makeAsyncIterator() - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! + let barrier = try await coordinator.withBarrier("Insert Item") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! try coordinator.performUndo(barrier: barrier) let event = await iterator.next() @@ -962,20 +951,20 @@ enum UndoEngineTests { func eventsAreNotReplayedToLaterSubscribers() async throws { let (database, coordinator) = try makeTestDatabaseWithUndo() - let firstId = try coordinator.beginBarrier("First") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let first = try coordinator.endBarrier(firstId)! + let first = try await coordinator.withBarrier("First") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! try coordinator.performUndo(barrier: first) var iterator = coordinator.events().makeAsyncIterator() - let secondId = try coordinator.beginBarrier("Second") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 2, name: "Test") }.execute(db) - } - let second = try coordinator.endBarrier(secondId)! + let second = try await coordinator.withBarrier("Second") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "Test") }.execute(db) + } + }! try coordinator.performUndo(barrier: second) let event = await iterator.next() diff --git a/Tests/SQLiteUndoTests/UnregisteredTableTests.swift b/Tests/SQLiteUndoTests/UnregisteredTableTests.swift index 0f91a98..8813d56 100644 --- a/Tests/SQLiteUndoTests/UnregisteredTableTests.swift +++ b/Tests/SQLiteUndoTests/UnregisteredTableTests.swift @@ -28,14 +28,13 @@ struct UnregisteredTableTests { registeredTables: [ArticleRecord.tableName] ) - let barrierId = try coordinator.beginBarrier("Mixed Changes") - try database.write { db in - try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) - try AuditRecord.insert { AuditRecord(id: 1, data: "Created article") }.execute(db) - } - try withKnownIssue { - _ = try coordinator.endBarrier(barrierId) + try coordinator.withBarrier("Mixed Changes") { + try database.write { db in + try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) + try AuditRecord.insert { AuditRecord(id: 1, data: "Created article") }.execute(db) + } + } } matching: { issue in issue.description.contains("auditRecords") } @@ -61,13 +60,12 @@ struct UnregisteredTableTests { registeredTables: [ArticleRecord.tableName, AuditRecord.tableName] ) - let barrierId = try coordinator.beginBarrier("Both Registered") - try database.write { db in - try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) - try AuditRecord.insert { AuditRecord(id: 1, data: "Audit") }.execute(db) + let barrier = try coordinator.withBarrier("Both Registered") { + try database.write { db in + try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) + try AuditRecord.insert { AuditRecord(id: 1, data: "Audit") }.execute(db) + } } - - let barrier = try coordinator.endBarrier(barrierId) #expect(barrier != nil) } @@ -92,13 +90,12 @@ struct UnregisteredTableTests { untrackedTables: [AuditRecord.tableName] ) - let barrierId = try coordinator.beginBarrier("With Untracked") - try database.write { db in - try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) - try AuditRecord.insert { AuditRecord(id: 1, data: "Audit log entry") }.execute(db) + let barrier = try coordinator.withBarrier("With Untracked") { + try database.write { db in + try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) + try AuditRecord.insert { AuditRecord(id: 1, data: "Audit log entry") }.execute(db) + } } - - let barrier = try coordinator.endBarrier(barrierId) #expect(barrier != nil) } } From de91e886e639ca7dd45e6b7861cdfd132ed45289 Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 07:52:54 -0700 Subject: [PATCH 03/10] identify each undoable operation by barrierID instead of tracking sequence numbers --- README.md | 25 +- Sources/SQLiteUndo/UndoBarrier.swift | 31 +-- Sources/SQLiteUndo/UndoCoordinator.swift | 144 +++-------- Sources/SQLiteUndo/UndoEngine.swift | 20 +- Sources/SQLiteUndo/UndoOperations.swift | 135 +++++----- Sources/SQLiteUndo/UndoSchema.swift | 5 + Sources/SQLiteUndo/UndoTracked.swift | 18 +- Sources/SQLiteUndo/Undoable.swift | 6 +- Tests/SQLiteUndoTests/SQLParserTests.swift | 18 +- Tests/SQLiteUndoTests/UndoEngineTests.swift | 258 +++++++++++++++++++- 10 files changed, 419 insertions(+), 241 deletions(-) diff --git a/README.md b/README.md index b646926..00952dc 100644 --- a/README.md +++ b/README.md @@ -103,18 +103,31 @@ BEGIN END ``` -### With explicit barrier management +### What a barrier captures -```swift -@Dependency(\.defaultUndoEngine) var undoEngine +A barrier claims exactly the writes made inside its `undoable` block. Barriers may +overlap freely — concurrent barriers, or one opened inside another, each keep their +own changes, and undoing one never disturbs another. + +Only writes made inside a barrier are tracked. A write outside one is applied +normally but is not undoable: -let barrierId = try undoEngine.beginBarrier("Set Rating") -try database.write { db in +```swift +try database.write { db in // not undoable try Article.find(id).update { $0.rating = 5 }.execute(db) } -try undoEngine.endBarrier(barrierId) + +try undoable("Set Rating") { // undoable + try database.write { db in + try Article.find(id).update { $0.rating = 5 }.execute(db) + } +} ``` +Tracking follows Swift's structured concurrency, so it reaches through `async` +writes and child tasks. It does not reach into a `Task.detached`, whose writes are +outside the barrier and therefore untracked. + ### Undo events After each undo/redo, `UndoEngine` emits an `UndoEvent` with the affected table rows. Use this to drive UI responses like scrolling to a restored item or switching views. diff --git a/Sources/SQLiteUndo/UndoBarrier.swift b/Sources/SQLiteUndo/UndoBarrier.swift index fe012e4..fa991c2 100644 --- a/Sources/SQLiteUndo/UndoBarrier.swift +++ b/Sources/SQLiteUndo/UndoBarrier.swift @@ -1,37 +1,28 @@ import Foundation /// A barrier represents a single undoable user action, grouping all database -/// changes that occurred between `beginBarrier` and `endBarrier`. +/// changes made while it was open. /// /// When undo is performed, all changes within the barrier are reversed in /// reverse chronological order. /// -/// ## Sequence Numbers +/// ## Entry Ownership /// -/// The `startSeq` and `endSeq` store the ORIGINAL sequence range when the -/// barrier was created. However, after undo/redo operations, the actual -/// entries in the undolog move to new sequence positions (seq numbers grow, -/// they are not reused). `UndoEngine` tracks the current seq range separately -/// in `barrierSeqRanges` - see that documentation for details. +/// Undolog rows are stamped with the barrier's `id` as they are captured, so a +/// barrier owns its entries no matter what else writes concurrently. Replaying +/// a barrier re-stamps the newly captured reverse entries with the same `id`, +/// so ownership survives any number of undo/redo cycles. public struct UndoBarrier: Hashable, Sendable, Codable { - /// Unique identifier for this barrier. + /// Unique identifier for this barrier, and the key its undolog entries carry. public let id: UUID /// Display name for the action (shown in Edit > Undo menu). public let name: String - /// Original first sequence number when barrier was created (may not reflect current position). - let startSeq: Int - /// Original last sequence number when barrier was created (may not reflect current position). - let endSeq: Int + /// The number of undolog entries captured when this barrier closed. + public let count: Int - public init(id: UUID, name: String, startSeq: Int, endSeq: Int) { + public init(id: UUID, name: String, count: Int) { self.id = id self.name = name - self.startSeq = startSeq - self.endSeq = endSeq - } - - /// The number of undolog entries in this barrier. - public var count: Int { - endSeq - startSeq + 1 + self.count = count } } diff --git a/Sources/SQLiteUndo/UndoCoordinator.swift b/Sources/SQLiteUndo/UndoCoordinator.swift index adc635f..c36dad6 100644 --- a/Sources/SQLiteUndo/UndoCoordinator.swift +++ b/Sources/SQLiteUndo/UndoCoordinator.swift @@ -18,38 +18,8 @@ final class UndoCoordinator: Sendable { private let state = LockIsolated(State()) private struct State { - var openBarriers: [UUID: OpenBarrier] = [:] + var openBarriers: [UUID: String] = [:] var subscribers: [UUID: AsyncStream.Continuation] = [:] - - /// Tracks current seq range for each barrier. - /// - /// ## Why this is needed - /// - /// Following the sqlite.org/undoredo pattern, sequence numbers are NOT reused. - /// When you undo a barrier: - /// 1. Original entries (e.g., seq 1-2) are deleted - /// 2. Reverse SQL executes, triggers capture NEW entries (e.g., seq 3-4) - /// 3. The barrier's "current" range is now 3-4, not 1-2 - /// - /// The sqlite.org pattern stores `[begin, end]` pairs on undo/redo stacks, - /// pushing the NEW range after each operation. We can't do that because - /// NSUndoManager owns the stack and the barrier is captured in closures - /// with fixed `startSeq`/`endSeq` values. - /// - /// Instead, we track the current seq range per barrier here. When undo/redo - /// is performed, we look up the current range (not the original), execute - /// the SQL, and update the range to wherever the new entries landed. - var barrierSeqRanges: [UUID: SeqRange] = [:] - } - - private struct OpenBarrier { - let name: String - let startSeq: Int - } - - struct SeqRange { - var startSeq: Int - var endSeq: Int } init( @@ -87,42 +57,40 @@ final class UndoCoordinator: Sendable { /// Begin recording changes for a new undoable action. /// - /// All database changes after this call will be captured in the undolog - /// until `endBarrier` or `cancelBarrier` is called. + /// Changes are claimed by this barrier only while `_undoBarrierID` is set to its + /// ID — see ``withBarrier(_:_:)``, which scopes that for you. /// /// - Parameter name: The action name (shown in Edit > Undo menu) /// - Returns: A unique ID for this barrier func beginBarrier(_ name: String) throws -> UUID { let id = UUID() - try database.read { db in - let currentSeq = try db.undoLogMaxSeq() ?? 0 - let startSeq = currentSeq + 1 - state.withValue { - $0.openBarriers[id] = OpenBarrier(name: name, startSeq: startSeq) - } - } + state.withValue { $0.openBarriers[id] = name } logger.debug("Begin barrier: \(name) (id: \(id))") return id } - /// End a barrier and capture all changes made since it began. + /// End a barrier and capture all changes it claimed. /// /// If no changes were made within the barrier, returns nil. /// /// - Parameter id: The barrier ID returned from `beginBarrier` /// - Returns: The completed barrier, or nil if no changes were captured func endBarrier(_ id: UUID) throws -> UndoBarrier? { - guard let openBarrier = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { + guard let name = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { logger.warning("Attempted to end unknown barrier: \(id)") return nil } return try database.write { db in - guard let endSeq = try db.undoLogMaxSeq(), endSeq >= openBarrier.startSeq else { + // Reconcile duplicate entries from cascading BEFORE triggers + try db.reconcileUndoLogEntries(barrierID: id) + + let count = try db.undoLogCount(barrierID: id) + guard count > 0 else { let tables = registeredTables.sorted() logger.warning( """ - End barrier (empty): \(openBarrier.name) — no database changes were captured. + End barrier (empty): \(name) — no database changes were captured. Did you forget to register a table with the UndoEngine? @@ -133,33 +101,17 @@ final class UndoCoordinator: Sendable { return nil } - // Reconcile duplicate entries from cascading BEFORE triggers - try db.reconcileUndoLogEntries(from: openBarrier.startSeq, to: endSeq) - - // Re-read endSeq since reconciliation may have removed entries - guard let endSeq = try db.undoLogMaxSeq(), endSeq >= openBarrier.startSeq else { - return nil - } - - let barrier = UndoBarrier( - id: id, - name: openBarrier.name, - startSeq: openBarrier.startSeq, - endSeq: endSeq - ) + let barrier = UndoBarrier(id: id, name: name, count: count) // Check for unregistered tables if !registeredTables.isEmpty { - let modifiedTables = try db.tablesModifiedInRange( - from: openBarrier.startSeq, - to: endSeq - ) + let modifiedTables = try db.tablesModified(barrierID: id) let allowedTables = registeredTables.union(untrackedTables) let unknownTables = modifiedTables.subtracting(allowedTables) if !unknownTables.isEmpty { reportIssue( """ - Barrier '\(openBarrier.name)' modified tables not registered with UndoEngine: \ + Barrier '\(name)' modified tables not registered with UndoEngine: \ \(unknownTables.sorted().joined(separator: ", ")). \ These changes won't be undone. Register the tables with UndoEngine, \ or add them to 'untracked:' if this is intentional. @@ -168,11 +120,6 @@ final class UndoCoordinator: Sendable { } } - // Track the seq range for this barrier - state.withValue { - $0.barrierSeqRanges[id] = SeqRange(startSeq: barrier.startSeq, endSeq: barrier.endSeq) - } - logger.debug("End barrier: \(barrier.name) (\(barrier.count) entries)") return barrier } @@ -188,7 +135,7 @@ final class UndoCoordinator: Sendable { func withBarrier(_ name: String, _ operation: () throws -> Void) throws -> UndoBarrier? { let id = try beginBarrier(name) do { - try operation() + try $_undoBarrierID.withValue(id.uuidString) { try operation() } return try endBarrier(id) } catch { try cancelBarrier(id) @@ -209,7 +156,7 @@ final class UndoCoordinator: Sendable { ) async throws -> UndoBarrier? { let id = try beginBarrier(name) do { - try await operation() + try await $_undoBarrierID.withValue(id.uuidString) { try await operation() } return try endBarrier(id) } catch { try cancelBarrier(id) @@ -224,67 +171,44 @@ final class UndoCoordinator: Sendable { /// /// - Parameter id: The barrier ID returned from `beginBarrier` func cancelBarrier(_ id: UUID) throws { - guard let openBarrier = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { + guard let name = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { logger.warning("Attempted to cancel unknown barrier: \(id)") return } try database.write { db in - if let endSeq = try db.undoLogMaxSeq(), endSeq >= openBarrier.startSeq { - try db.deleteUndoLogEntries(from: openBarrier.startSeq, to: endSeq) - } + try db.deleteUndoLogEntries(barrierID: id) } - logger.debug("Cancel barrier: \(openBarrier.name)") + logger.debug("Cancel barrier: \(name)") } /// Perform undo for a barrier. /// /// Executes all reverse SQL in the barrier in reverse order. - /// The executed SQL is captured by triggers, becoming the redo SQL. - /// - /// The seq range used is looked up from `barrierSeqRanges` (not the barrier's - /// original values) because entries move to new seq positions after each - /// undo/redo. After execution, the tracked range is updated to the new positions. + /// The executed SQL is captured by triggers, becoming the redo SQL, and is + /// re-stamped with this barrier's ID so it stays owned across cycles. func performUndo(barrier: UndoBarrier) throws { - let seqRange = - state.withValue { $0.barrierSeqRanges[barrier.id] } - ?? SeqRange(startSeq: barrier.startSeq, endSeq: barrier.endSeq) - - let result = try database.write { db in - try db.performUndoRedo(startSeq: seqRange.startSeq, endSeq: seqRange.endSeq) - } - - if let result { - state.withValue { - $0.barrierSeqRanges[barrier.id] = result.seqRange - } - emit(UndoEvent(kind: .undo, name: barrier.name, affectedItems: result.affectedItems)) + if let affectedItems = try replay(barrier: barrier) { + emit(UndoEvent(kind: .undo, name: barrier.name, affectedItems: affectedItems)) } } /// Perform redo for a barrier. /// - /// Re-applies the original changes that were undone. - /// The executed SQL is captured by triggers, becoming the undo SQL again. - /// - /// The seq range used is looked up from `barrierSeqRanges` (not the barrier's - /// original values) because entries move to new seq positions after each - /// undo/redo. After execution, the tracked range is updated to the new positions. + /// Re-applies the original changes that were undone. The executed SQL is + /// captured by triggers, becoming the undo SQL again. func performRedo(barrier: UndoBarrier) throws { - let seqRange = - state.withValue { $0.barrierSeqRanges[barrier.id] } - ?? SeqRange(startSeq: barrier.startSeq, endSeq: barrier.endSeq) - - let result = try database.write { db in - try db.performUndoRedo(startSeq: seqRange.startSeq, endSeq: seqRange.endSeq) + if let affectedItems = try replay(barrier: barrier) { + emit(UndoEvent(kind: .redo, name: barrier.name, affectedItems: affectedItems)) } + } - if let result { - state.withValue { - $0.barrierSeqRanges[barrier.id] = result.seqRange - } - emit(UndoEvent(kind: .redo, name: barrier.name, affectedItems: result.affectedItems)) + /// Replay a barrier's entries. Undo and redo are the same operation — each + /// captures the reverse of what it executes. + private func replay(barrier: UndoBarrier) throws -> Set? { + try database.write { db in + try db.performUndoRedo(barrierID: barrier.id) } } } diff --git a/Sources/SQLiteUndo/UndoEngine.swift b/Sources/SQLiteUndo/UndoEngine.swift index c35c022..cc6c169 100644 --- a/Sources/SQLiteUndo/UndoEngine.swift +++ b/Sources/SQLiteUndo/UndoEngine.swift @@ -49,23 +49,26 @@ private let logger = Logger(subsystem: "SQLiteUndo", category: "UndoEngine") public struct UndoEngine: Sendable { /// Begin recording changes for a new undoable action. /// + /// Internal: a barrier only claims writes made while `_undoBarrierID` is set to + /// its ID, so barriers must be opened through ``undoable(_:operation:)-3cgh0``. + /// /// - Parameter name: The action name (shown in Edit > Undo menu) /// - Returns: A unique ID for this barrier - public var beginBarrier: @Sendable (_ name: String) throws -> UUID = { _ in UUID() } + var beginBarrier: @Sendable (_ name: String) throws -> UUID = { _ in UUID() } /// End a barrier and register with UndoManager. /// /// If no changes were made within the barrier, nothing is registered. /// /// - Parameter id: The barrier ID from `beginBarrier` - public var endBarrier: @Sendable (_ id: UUID) throws -> Void + var endBarrier: @Sendable (_ id: UUID) throws -> Void /// Cancel a barrier without registering it. /// /// Use this for aborted operations or error handling. /// /// - Parameter id: The barrier ID from `beginBarrier` - public var cancelBarrier: @Sendable (_ id: UUID) throws -> Void + var cancelBarrier: @Sendable (_ id: UUID) throws -> Void /// Stream of events emitted after each undo/redo operation. /// @@ -81,11 +84,22 @@ public struct UndoEngine: Sendable { /// Whether the undo system is replaying entries (undo/redo in progress). @TaskLocal var _undoIsReplaying = false +/// The barrier that owns writes made in the current scope, or nil when no barrier is open. +/// +/// Triggers stamp this onto each undolog row, which is how a barrier claims its +/// entries. Writes made outside a barrier are not tracked. +@TaskLocal var _undoBarrierID: String? + @DatabaseFunction("sqliteundo_isActive") func undoIsActiveFunction() -> Bool { _undoIsActive } +@DatabaseFunction("sqliteundo_barrierID") +func undoBarrierIDFunction() -> String? { + _undoBarrierID +} + @DatabaseFunction("sqliteundo_isReplaying") func undoIsReplayingFunction() -> Bool { _undoIsReplaying diff --git a/Sources/SQLiteUndo/UndoOperations.swift b/Sources/SQLiteUndo/UndoOperations.swift index dc8219b..bd0e2f5 100644 --- a/Sources/SQLiteUndo/UndoOperations.swift +++ b/Sources/SQLiteUndo/UndoOperations.swift @@ -31,31 +31,24 @@ extension Database { /// ## Sequence Numbers Grow, Not Reused /// /// The sqlite.org pattern does NOT try to reuse sequence numbers. After each - /// undo/redo, entries move to new (higher) seq positions. This avoids conflicts - /// when multiple barriers exist - each barrier's entries can move independently - /// without colliding with other barriers' seq ranges. + /// undo/redo, entries move to new (higher) seq positions. `seq` therefore only + /// orders entries; ownership is carried by `barrierID`, which replay re-stamps + /// onto the newly captured entries. Barriers may interleave freely. /// - /// The caller (UndoEngine) tracks the current seq range for each barrier and - /// updates it after this method returns. - /// - struct UndoRedoResult { - var seqRange: UndoCoordinator.SeqRange - var affectedItems: Set - } - - /// - Returns: The new seq range and affected items, or nil if no entries were executed. - func performUndoRedo(startSeq: Int, endSeq: Int) throws -> UndoRedoResult? { - logger.debug("Performing undo/redo: seq \(startSeq)...\(endSeq)") + /// - Returns: The affected items, or nil if no entries were executed. + func performUndoRedo(barrierID: UUID) throws -> Set? { + let id = barrierID.uuidString + logger.debug("Performing undo/redo for barrier \(id)") // Fetch entries to execute (in reverse order) let entries = try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(id) } .order { $0.seq.desc() } .fetchAll(self) guard !entries.isEmpty else { - logger.debug("No entries found for seq range \(startSeq)...\(endSeq)") + logger.debug("No entries found for barrier \(id)") return nil } @@ -67,80 +60,74 @@ extension Database { ) // Delete the entries - try deleteUndoLogEntries(from: startSeq, to: endSeq) - - // Get current max seq before executing (new entries will be added after this) - let seqBefore = try undoLogMaxSeq() ?? 0 + try deleteUndoLogEntries(barrierID: barrierID) // Execute with triggers ENABLED - this captures the reverse SQL. // Set isReplaying so app-level triggers suppress cascading writes. // The undo log already contains all effects (including cascades), // so replaying them individually is sufficient. + // Re-stamp the captured entries with this barrier so it keeps owning them. // Batch consecutive same-table, same-type entries for efficiency. - try $_undoIsReplaying.withValue(true) { - try #sql("PRAGMA defer_foreign_keys = ON").execute(self) - for sql in batchedSQL(from: entries) { - logger.trace("Executing SQL: \(sql)") - try #sql("\(raw: sql)").execute(self) - } - #if DEBUG - // Check for FK violations that will cause the commit to fail. - let violations = try #sql( - """ - SELECT "table" || ' rowid=' || rowid || ' parent=' || "parent" || ' fkid=' || fkid - FROM pragma_foreign_key_check - """, - as: String.self - ).fetchAll(self) - if !violations.isEmpty { - logger.error( - """ - Undo replay will fail due to foreign key violations - - Ensure all tables involved in foreign key relationships are undo-tracked, - and that undo-tracked tables do not have foreign keys to non-tracked tables. + try $_undoBarrierID.withValue(id) { + try $_undoIsReplaying.withValue(true) { + try #sql("PRAGMA defer_foreign_keys = ON").execute(self) + for sql in batchedSQL(from: entries) { + logger.trace("Executing SQL: \(sql)") + try #sql("\(raw: sql)").execute(self) + } + #if DEBUG + // Check for FK violations that will cause the commit to fail. + let violations = try #sql( """ - ) - for v in violations { - logger.error(" FK violation after undo replay: \(v)") + SELECT "table" || ' rowid=' || rowid || ' parent=' || "parent" || ' fkid=' || fkid + FROM pragma_foreign_key_check + """, + as: String.self + ).fetchAll(self) + if !violations.isEmpty { + logger.error( + """ + Undo replay will fail due to foreign key violations + + Ensure all tables involved in foreign key relationships are undo-tracked, + and that undo-tracked tables do not have foreign keys to non-tracked tables. + """ + ) + for v in violations { + logger.error(" FK violation after undo replay: \(v)") + } } - } - #endif - } - - // Get new seq range for captured entries - let seqAfter = try undoLogMaxSeq() ?? seqBefore - if seqAfter > seqBefore { - let newRange = UndoCoordinator.SeqRange(startSeq: seqBefore + 1, endSeq: seqAfter) - // No reconciliation needed during replay: _undoIsReplaying suppresses - // app-level cascade triggers, so each row produces exactly one reverse entry. - logger.debug("New seq range: \(newRange.startSeq)...\(newRange.endSeq)") - return UndoRedoResult(seqRange: newRange, affectedItems: affectedItems) + #endif + } } - return nil + // No reconciliation needed during replay: _undoIsReplaying suppresses + // app-level cascade triggers, so each row produces exactly one reverse entry. + return affectedItems } } extension Database { - /// Get the current maximum sequence number in the undolog. - func undoLogMaxSeq() throws -> Int? { - try #sql("SELECT MAX(seq) FROM undolog", as: Int?.self).fetchOne(self) ?? nil + /// Count the undolog entries owned by a barrier. + func undoLogCount(barrierID: UUID) throws -> Int { + try UndoLogEntry + .where { $0.barrierID.eq(barrierID.uuidString) } + .fetchCount(self) } - /// Delete undolog entries in a sequence range. - func deleteUndoLogEntries(from startSeq: Int, to endSeq: Int) throws { + /// Delete the undolog entries owned by a barrier. + func deleteUndoLogEntries(barrierID: UUID) throws { try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(barrierID.uuidString) } .delete() .execute(self) } - /// Get the set of table names modified in a sequence range. - func tablesModifiedInRange(from startSeq: Int, to endSeq: Int) throws -> Set { + /// Get the set of table names a barrier modified. + func tablesModified(barrierID: UUID) throws -> Set { let tableNames = try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(barrierID.uuidString) } .select { $0.tableName } .fetchAll(self) return Set(tableNames) @@ -154,12 +141,14 @@ extension Database { /// - INSERT (DELETE-reverse) + DELETE (INSERT-reverse) of same row → remove both (no-op) /// - INSERT (DELETE-reverse) + UPDATE → keep just the DELETE-reverse (undo = delete) /// - Multiple UPDATEs → keep first (true original values) - func reconcileUndoLogEntries(from startSeq: Int, to endSeq: Int) throws { + func reconcileUndoLogEntries(barrierID: UUID) throws { + let id = barrierID.uuidString + // Fast path: check if any duplicates exist before fetching all entries let hasDuplicates = try #sql( """ SELECT 1 FROM undolog - WHERE seq >= \(startSeq) AND seq <= \(endSeq) AND trackedRowid != 0 + WHERE barrierID = \(id) AND trackedRowid != 0 GROUP BY tableName, trackedRowid HAVING COUNT(*) > 1 LIMIT 1 @@ -171,7 +160,7 @@ extension Database { let entries = try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(id) } .order { $0.seq.asc() } .fetchAll(self) @@ -208,13 +197,13 @@ extension Database { // adding any columns not already present (first entry's values win). var mergedAssignments: [UndoSQL.UpdateSQL.Assignment]? var existingColumns: Set? - if case let .update(upd) = first.sql { + if case .update(let upd) = first.sql { mergedAssignments = upd.assignments existingColumns = Set(upd.assignments.map(\.column)) } for entry in group.dropFirst() { - if case let .update(upd) = entry.sql { + if case .update(let upd) = entry.sql { if var assignments = mergedAssignments, var columns = existingColumns { let additions = upd.assignments.filter { !columns.contains($0.column) } if !additions.isEmpty { @@ -228,7 +217,7 @@ extension Database { } } - if case let .update(upd) = first.sql, + if case .update(let upd) = first.sql, let assignments = mergedAssignments, assignments.count > upd.assignments.count { seqsToUpdate.append( diff --git a/Sources/SQLiteUndo/UndoSchema.swift b/Sources/SQLiteUndo/UndoSchema.swift index c58c72f..2e79c51 100644 --- a/Sources/SQLiteUndo/UndoSchema.swift +++ b/Sources/SQLiteUndo/UndoSchema.swift @@ -9,6 +9,8 @@ import SQLiteData struct UndoLogEntry: Sendable { /// Auto-incrementing sequence number for ordering. var seq: Int + /// The barrier that owns this entry. + var barrierID: String /// The name of the table that was modified. var tableName: String /// The rowid of the tracked row, for deduplication during reconciliation. @@ -78,15 +80,18 @@ extension DatabaseWriter { """ CREATE TABLE undolog ( seq INTEGER PRIMARY KEY AUTOINCREMENT, + barrierID TEXT NOT NULL, tableName TEXT NOT NULL, trackedRowid INTEGER NOT NULL DEFAULT 0, sql TEXT NOT NULL ) """ ).execute(db) + try #sql("CREATE INDEX undolog_barrierID ON undolog(barrierID)").execute(db) db.add(function: $undoIsActiveFunction) db.add(function: $undoIsReplayingFunction) + db.add(function: $undoBarrierIDFunction) } } } diff --git a/Sources/SQLiteUndo/UndoTracked.swift b/Sources/SQLiteUndo/UndoTracked.swift index 12c4014..a669c70 100644 --- a/Sources/SQLiteUndo/UndoTracked.swift +++ b/Sources/SQLiteUndo/UndoTracked.swift @@ -31,10 +31,10 @@ extension StructuredQueries.Table { """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_\(table)_insert AFTER INSERT ON "\(table)" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('\(table)', NEW.rowid, 'D'||char(9)||'\(table)'||char(9)||NEW.rowid); + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), '\(table)', NEW.rowid, 'D'||char(9)||'\(table)'||char(9)||NEW.rowid); END """ } @@ -54,11 +54,11 @@ extension StructuredQueries.Table { return """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_\(table)_update BEFORE UPDATE ON "\(table)" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL AND (\(changeChecks)) BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('\(table)', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), '\(table)', OLD.rowid, 'U'||char(9)||'\(table)'||char(9)||OLD.rowid || \(caseClauses) ); @@ -76,10 +76,10 @@ extension StructuredQueries.Table { return """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_\(table)_delete BEFORE DELETE ON "\(table)" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('\(table)', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), '\(table)', OLD.rowid, 'I'||char(9)||'\(table)'||char(9)||OLD.rowid || \(colValuePairs) ); diff --git a/Sources/SQLiteUndo/Undoable.swift b/Sources/SQLiteUndo/Undoable.swift index 4e3766b..21f0dba 100644 --- a/Sources/SQLiteUndo/Undoable.swift +++ b/Sources/SQLiteUndo/Undoable.swift @@ -20,7 +20,7 @@ public func undoable( let barrierId = try undoEngine.beginBarrier(actionName) do { - let result = try operation() + let result = try $_undoBarrierID.withValue(barrierId.uuidString) { try operation() } try undoEngine.endBarrier(barrierId) return result } catch { @@ -48,7 +48,9 @@ public func undoable( let barrierId = try undoEngine.beginBarrier(actionName) do { - let result = try await operation() + let result = try await $_undoBarrierID.withValue(barrierId.uuidString) { + try await operation() + } try undoEngine.endBarrier(barrierId) return result } catch { diff --git a/Tests/SQLiteUndoTests/SQLParserTests.swift b/Tests/SQLiteUndoTests/SQLParserTests.swift index 37ab7d3..99f723a 100644 --- a/Tests/SQLiteUndoTests/SQLParserTests.swift +++ b/Tests/SQLiteUndoTests/SQLParserTests.swift @@ -88,7 +88,7 @@ struct SQLParserTests { @Test func deleteParseValues() { let parsed = UndoSQL(tabDelimited: "D\tt\t42")! - guard case let .delete(d) = parsed else { + guard case .delete(let d) = parsed else { Issue.record("Expected delete, got \(parsed)") return } @@ -99,7 +99,7 @@ struct SQLParserTests { @Test func insertParseValues() { let parsed = UndoSQL(tabDelimited: "I\tt\t1\ta\t'hello'\tb\tNULL")! - guard case let .insert(ins) = parsed else { + guard case .insert(let ins) = parsed else { Issue.record("Expected insert, got \(parsed)") return } @@ -113,7 +113,7 @@ struct SQLParserTests { @Test func updateParseValues() { let parsed = UndoSQL(tabDelimited: "U\tt\t1\ta\t'x'\tb\t42")! - guard case let .update(upd) = parsed else { + guard case .update(let upd) = parsed else { Issue.record("Expected update, got \(parsed)") return } @@ -158,11 +158,11 @@ struct SQLParserTests { func updateDifferentAssignmentsNotBatched() { let entries: [UndoLogEntry] = [ UndoLogEntry( - seq: 0, tableName: "t", + seq: 0, barrierID: "b", tableName: "t", sql: .update( .init(table: "t", assignments: [.init(column: "a", value: "'x'")], rowids: ["1"]))), UndoLogEntry( - seq: 0, tableName: "t", + seq: 0, barrierID: "b", tableName: "t", sql: .update( .init(table: "t", assignments: [.init(column: "a", value: "'y'")], rowids: ["2"]))), ] @@ -178,11 +178,11 @@ struct SQLParserTests { func updateSameAssignmentsBatched() { let entries: [UndoLogEntry] = [ UndoLogEntry( - seq: 0, tableName: "t", + seq: 0, barrierID: "b", tableName: "t", sql: .update( .init(table: "t", assignments: [.init(column: "a", value: "'x'")], rowids: ["1"]))), UndoLogEntry( - seq: 0, tableName: "t", + seq: 0, barrierID: "b", tableName: "t", sql: .update( .init(table: "t", assignments: [.init(column: "a", value: "'x'")], rowids: ["2"]))), ] @@ -197,11 +197,11 @@ struct SQLParserTests { func sparseUpdateOnlyChangedColumns() { let entries: [UndoLogEntry] = [ UndoLogEntry( - seq: 0, tableName: "t", + seq: 0, barrierID: "b", tableName: "t", sql: .update( .init(table: "t", assignments: [.init(column: "value", value: "42")], rowids: ["1"]))), UndoLogEntry( - seq: 0, tableName: "t", + seq: 0, barrierID: "b", tableName: "t", sql: .update( .init(table: "t", assignments: [.init(column: "value", value: "42")], rowids: ["2"]))), ] diff --git a/Tests/SQLiteUndoTests/UndoEngineTests.swift b/Tests/SQLiteUndoTests/UndoEngineTests.swift index 2520ade..86f8cae 100644 --- a/Tests/SQLiteUndoTests/UndoEngineTests.swift +++ b/Tests/SQLiteUndoTests/UndoEngineTests.swift @@ -24,19 +24,19 @@ enum UndoEngineTests { """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_testRecords_insert AFTER INSERT ON "testRecords" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('testRecords', NEW.rowid, 'D'||char(9)||'testRecords'||char(9)||NEW.rowid); + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), 'testRecords', NEW.rowid, 'D'||char(9)||'testRecords'||char(9)||NEW.rowid); END CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_testRecords_update BEFORE UPDATE ON "testRecords" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL AND (OLD."id" IS NOT NEW."id" OR OLD."name" IS NOT NEW."name" OR OLD."value" IS NOT NEW."value") BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('testRecords', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), 'testRecords', OLD.rowid, 'U'||char(9)||'testRecords'||char(9)||OLD.rowid || CASE WHEN OLD."id" IS NOT NEW."id" THEN char(9)||'id'||char(9)||quote(OLD."id") ELSE '' END || CASE WHEN OLD."name" IS NOT NEW."name" THEN char(9)||'name'||char(9)||quote(OLD."name") ELSE '' END @@ -46,10 +46,10 @@ enum UndoEngineTests { CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_testRecords_delete BEFORE DELETE ON "testRecords" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('testRecords', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), 'testRecords', OLD.rowid, 'I'||char(9)||'testRecords'||char(9)||OLD.rowid || char(9)||'id'||char(9)||quote(OLD."id") || char(9)||'name'||char(9)||quote(OLD."name") @@ -110,6 +110,246 @@ enum UndoEngineTests { } } + @Suite + @MainActor + struct ScopeRoutingTests { + + /// Two "windows" share one database and engine but each has its own + /// UndoStack/UndoManager, supplied by its own dependency scope. + @Test + func barriersRegisterWithTheScopesUndoManager() throws { + let managerA = UndoManager() + let managerB = UndoManager() + // The stacks must outlive the scopes that install them: NSUndoManager does + // not retain its target, so a stack released while its manager still holds + // registrations leaves those registrations dangling. + let stackA = UndoStack.live(managerA) + let stackB = UndoStack.live(managerB) + + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + } operation: { + @Dependency(\.defaultDatabase) var database + + try withDependencies { + $0.defaultUndoStack = stackA + } operation: { + try undoable("From A") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + } + + try withDependencies { + $0.defaultUndoStack = stackB + } operation: { + try undoable("From B") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "b") }.execute(db) + } + } + } + + #expect(managerA.undoActionName == "From A") + #expect(managerB.undoActionName == "From B") + + // Undoing in A reverts only A's row. + managerA.undo() + + try database.read { db in + let rowA = try TestRecord.find(1).fetchOne(db) + let rowB = try TestRecord.find(2).fetchOne(db) + #expect(rowA == nil) + #expect(rowB?.name == "b") + } + + #expect(managerA.canUndo == false) + #expect(managerB.canUndo == true) + } + } + } + + @Suite + struct BarrierOwnershipTests { + + @Test + func openBarrierDoesNotClaimAnotherBarriersChanges() throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + // "Inner" opens, writes, and closes while "Outer" is still open. + var inner: UndoBarrier? + let outer = try engine.withBarrier("Outer") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "outer") }.execute(db) + } + inner = try engine.withBarrier("Inner") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "inner") }.execute(db) + } + } + }! + + #expect(outer.count == 1) + #expect(inner?.count == 1) + + // Undoing "Outer" must not revert "Inner"'s row. + try engine.performUndo(barrier: outer) + + try database.read { db in + let outerRow = try TestRecord.find(1).fetchOne(db) + let innerRow = try TestRecord.find(2).fetchOne(db) + #expect(outerRow == nil) + #expect(innerRow?.name == "inner") + } + + // "Inner" is still independently undoable. + try engine.performUndo(barrier: inner!) + + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 0) + } + } + + @Test + func concurrentBarriersOwnOnlyTheirOwnChanges() async throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + // Two barriers racing, as two windows would. Whatever the interleaving, + // each must end up owning exactly its own row. + async let a = engine.withBarrier("A") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + async let b = engine.withBarrier("B") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "b") }.execute(db) + } + } + let (barrierA, barrierB) = try await (a!, b!) + + #expect(barrierA.count == 1) + #expect(barrierB.count == 1) + + try engine.performUndo(barrier: barrierA) + + let (rowA, rowB) = try await database.read { db in + (try TestRecord.find(1).fetchOne(db), try TestRecord.find(2).fetchOne(db)) + } + #expect(rowA == nil) + #expect(rowB?.name == "b") + } + + @Test + func writesOutsideAnyBarrierAreNotTracked() throws { + let (database, _) = try makeTestDatabaseWithUndo() + + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "untracked") }.execute(db) + } + + let undoLogCount = try database.read { db in + try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") + } + #expect(undoLogCount == 0) + + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 1) + } + } + + @Test + func asyncBarrierCapturesChanges() async throws { + // Guards the mechanism the whole design rests on: the barrier TaskLocal + // must survive GRDB's async write, which hops to its own executor. + let (database, engine) = try makeTestDatabaseWithUndo() + + let barrier = try await engine.withBarrier("Async Insert") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + } + + #expect(barrier?.count == 1) + + try engine.performUndo(barrier: barrier!) + + let count = try await database.read { db in + try TestRecord.all.fetchCount(db) + } + #expect(count == 0) + } + + @Test + func detachedTaskWritesAreNotTracked() async throws { + // Documented limitation: a detached task starts a fresh task context, so it + // is outside the barrier. Its writes apply but are not undoable. + let (database, engine) = try makeTestDatabaseWithUndo() + + let barrier = try await engine.withBarrier("Detached") { + try await Task.detached { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "detached") }.execute(db) + } + }.value + } + + #expect(barrier == nil) + + let count = try await database.read { db in + try TestRecord.all.fetchCount(db) + } + #expect(count == 1) + } + + @Test + func ownershipSurvivesRepeatedUndoRedoCycles() throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + let first = try engine.withBarrier("First") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "first") }.execute(db) + } + }! + let second = try engine.withBarrier("Second") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "second") }.execute(db) + } + }! + + // Cycle the first barrier repeatedly; entries move to new seq positions + // each time but must stay owned by their barrier. + for _ in 1...3 { + try engine.performUndo(barrier: first) + try database.read { db in + let firstRow = try TestRecord.find(1).fetchOne(db) + let secondRow = try TestRecord.find(2).fetchOne(db) + #expect(firstRow == nil) + #expect(secondRow != nil) + } + try engine.performRedo(barrier: first) + try database.read { db in + let firstRow = try TestRecord.find(1).fetchOne(db) + #expect(firstRow?.name == "first") + } + } + + // The second barrier is unaffected by all that churn. + try engine.performUndo(barrier: second) + try database.read { db in + let firstRow = try TestRecord.find(1).fetchOne(db) + let secondRow = try TestRecord.find(2).fetchOne(db) + #expect(firstRow?.name == "first") + #expect(secondRow == nil) + } + } + } + @Suite struct UndoRedoTests { From fbba0df076902e30001e956fafce0393bf166597 Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 08:01:41 -0700 Subject: [PATCH 04/10] fix retain of undo mangager/stack --- Sources/SQLiteUndo/UndoStack.swift | 36 +++++++-------- Tests/SQLiteUndoTests/UndoEngineTests.swift | 49 ++++++++++++++++++--- 2 files changed, 60 insertions(+), 25 deletions(-) diff --git a/Sources/SQLiteUndo/UndoStack.swift b/Sources/SQLiteUndo/UndoStack.swift index 91def7d..fe3db8b 100644 --- a/Sources/SQLiteUndo/UndoStack.swift +++ b/Sources/SQLiteUndo/UndoStack.swift @@ -96,10 +96,14 @@ extension UndoStack: DependencyKey { public static func live(_ undoManager: UndoManager? = nil) -> UndoStack { let state = LockIsolated(UndoStackState(undo: [])) - // Target object for NSUndoManager registration - holds mutable UndoManager reference + // Target object for NSUndoManager registration - holds mutable UndoManager reference. + // + // `undoManager` is weak because the window owns its UndoManager, not us. That + // also keeps the registration closures below (which capture this target + // strongly, since NSUndoManager does not retain its target) from cycling. final class UndoTarget: @unchecked Sendable { let state: LockIsolated - var undoManager: UndoManager? + weak var undoManager: UndoManager? init(state: LockIsolated, undoManager: UndoManager?) { self.state = state @@ -131,23 +135,21 @@ extension UndoStack: DependencyKey { logger.debug("Registering undo: \(barrier.name)") undoManager.beginUndoGrouping() undoManager.setActionName(barrier.name) - undoManager.registerUndo(withTarget: self) { [weak self] target in + undoManager.registerUndo(withTarget: self) { [self] _ in MainActor.assumeIsolated { logger.debug("Performing undo: \(barrier.name)") do { try onUndo() - self?.state.withValue { + state.withValue { if let index = $0.undo.lastIndex(of: barrier.name) { $0.undo.remove(at: index) } $0.redo.append(barrier.name) } - if let self { - logger.info( - "\(self.currentState.logDescription(after: "undo \"\(barrier.name)\""))" - ) - } - target.registerRedo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) + logger.info( + "\(self.currentState.logDescription(after: "undo \"\(barrier.name)\""))" + ) + registerRedo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } catch { logger.error("Undo failed for \"\(barrier.name)\": \(error)") } @@ -173,23 +175,21 @@ extension UndoStack: DependencyKey { return } logger.debug("Registering redo: \(barrier.name)") - undoManager.registerUndo(withTarget: self) { [weak self] target in + undoManager.registerUndo(withTarget: self) { [self] _ in MainActor.assumeIsolated { logger.debug("Performing redo: \(barrier.name)") do { try onRedo() - self?.state.withValue { + state.withValue { if let index = $0.redo.lastIndex(of: barrier.name) { $0.redo.remove(at: index) } $0.undo.append(barrier.name) } - if let self { - logger.info( - "\(self.currentState.logDescription(after: "redo \"\(barrier.name)\""))" - ) - } - target.registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) + logger.info( + "\(self.currentState.logDescription(after: "redo \"\(barrier.name)\""))" + ) + registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } catch { logger.error("Redo failed for \"\(barrier.name)\": \(error)") } diff --git a/Tests/SQLiteUndoTests/UndoEngineTests.swift b/Tests/SQLiteUndoTests/UndoEngineTests.swift index 86f8cae..5c429b2 100644 --- a/Tests/SQLiteUndoTests/UndoEngineTests.swift +++ b/Tests/SQLiteUndoTests/UndoEngineTests.swift @@ -120,11 +120,6 @@ enum UndoEngineTests { func barriersRegisterWithTheScopesUndoManager() throws { let managerA = UndoManager() let managerB = UndoManager() - // The stacks must outlive the scopes that install them: NSUndoManager does - // not retain its target, so a stack released while its manager still holds - // registrations leaves those registrations dangling. - let stackA = UndoStack.live(managerA) - let stackB = UndoStack.live(managerB) try withDependencies { let database = try! makeTestDatabase() @@ -134,7 +129,7 @@ enum UndoEngineTests { @Dependency(\.defaultDatabase) var database try withDependencies { - $0.defaultUndoStack = stackA + $0.defaultUndoStack = .live(managerA) } operation: { try undoable("From A") { try database.write { db in @@ -144,7 +139,7 @@ enum UndoEngineTests { } try withDependencies { - $0.defaultUndoStack = stackB + $0.defaultUndoStack = .live(managerB) } operation: { try undoable("From B") { try database.write { db in @@ -170,6 +165,46 @@ enum UndoEngineTests { #expect(managerB.canUndo == true) } } + + /// NSUndoManager does not retain its registration target, so the stack that + /// registered a barrier may be released long before the undo is performed. + @Test + func undoWorksAfterTheRegisteringStackIsReleased() throws { + let manager = UndoManager() + + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + } operation: { + @Dependency(\.defaultDatabase) var database + + // The stack is owned by this scope alone and released when it exits. + try withDependencies { + $0.defaultUndoStack = .live(manager) + } operation: { + try undoable("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "test") }.execute(db) + } + } + } + + manager.undo() + + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 0) + } + + manager.redo() + + try database.read { db in + let row = try TestRecord.find(1).fetchOne(db) + #expect(row?.name == "test") + } + } + } } @Suite From 6fe38f11e8508ffe58fc67e299500a799070bfdb Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 08:25:47 -0700 Subject: [PATCH 05/10] move event stream to each independent undo stack --- Examples/UndoForMacOS/UndoForMacOSApp.swift | 38 ++++- README.md | 37 ++++- Sources/SQLiteUndo/UndoCoordinator.swift | 42 ++--- Sources/SQLiteUndo/UndoEngine.swift | 24 +-- Sources/SQLiteUndo/UndoStack.swift | 49 +++++- Sources/SQLiteUndoTCA/UndoManaging.swift | 3 +- .../UndoableEffectTests.swift | 40 +++++ Tests/SQLiteUndoTests/UndoEngineTests.swift | 152 +++++++++++------- 8 files changed, 272 insertions(+), 113 deletions(-) diff --git a/Examples/UndoForMacOS/UndoForMacOSApp.swift b/Examples/UndoForMacOS/UndoForMacOSApp.swift index 7fbab7a..e6989fc 100644 --- a/Examples/UndoForMacOS/UndoForMacOSApp.swift +++ b/Examples/UndoForMacOS/UndoForMacOSApp.swift @@ -17,15 +17,31 @@ struct UndoForMacOSApp: App { } var body: some Scene { WindowGroup { - DemoView(store: Store( - initialState: DemoFeature.State() - ) { - DemoFeature() - }) + DemoWindow() } } } +/// One window's worth of state. +/// +/// The database and engine are app-wide, but each window gets its own `UndoStack`, +/// which binds to that window's UndoManager. Barriers opened by this window's store +/// register with that manager, and only this window sees the resulting undo events. +/// Open a second window with ⌘N to see the two undo stacks operate independently. +struct DemoWindow: View { + @State private var store = withDependencies { + $0.defaultUndoStack = .live() + } operation: { + Store(initialState: DemoFeature.State()) { + DemoFeature() + } + } + + var body: some View { + DemoView(store: store) + } +} + @Reducer struct DemoFeature { @ObservableState @@ -78,7 +94,9 @@ struct DemoFeature { try undoable("Add Item") { try database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { + DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") + }.execute(db) } } } @@ -89,7 +107,9 @@ struct DemoFeature { try await undoable("Add Item (Background)") { try await database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, windowID: windowID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { + DemoItem(id: nextID, windowID: windowID, name: "Item \(nextID)") + }.execute(db) } } } @@ -99,7 +119,9 @@ struct DemoFeature { try withUndoDisabled { try database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { + DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") + }.execute(db) } } } diff --git a/README.md b/README.md index 00952dc..686e2df 100644 --- a/README.md +++ b/README.md @@ -130,10 +130,12 @@ outside the barrier and therefore untracked. ### Undo events -After each undo/redo, `UndoEngine` emits an `UndoEvent` with the affected table rows. Use this to drive UI responses like scrolling to a restored item or switching views. +After each undo/redo, the `UndoStack` that performed it emits an `UndoEvent` with the affected table rows. Use this to drive UI responses like scrolling to a restored item or switching views. ```swift -for await event in undoEngine.events() { +@Dependency(\.defaultUndoStack) var undoStack + +for await event in undoStack.events() { if let articleIds = event.ids(for: Article.self) { // scroll to restored articles } @@ -145,6 +147,8 @@ for await event in undoEngine.events() { `ids(for:)` returns `nil` when no rows of that table were affected, so `if let` naturally gates your response logic. +Events are scoped to the stack that performed the undo, so an undo in one window does not notify another. See [Multiple windows](#multiple-windows). + ## ComposableArchitecture/SwiftUI Integration ```swift @@ -196,6 +200,35 @@ struct MyView: View { } ``` +## Multiple windows + +An undo scope is one `UndoStack` bound to one `UndoManager`. The database, engine, and +undo log are app-wide; the stack is not. + +Give each window its own stack by scoping the dependency where its store is created: + +```swift +struct MyWindow: View { + @State private var store = withDependencies { + $0.defaultUndoStack = .live() + } operation: { + Store(initialState: MyFeature.State()) { MyFeature() } + } + + var body: some View { + MyView(store: store) + } +} +``` + +Each window then has its own undo/redo stack, its own Edit menu state, and its own +event stream. Barriers register with whichever stack is current when they close, and +undoing in one window never touches another's changes. + +Because AppKit resolves `UndoManager` up the responder chain, this also gives you the +document case for free: several windows onto one `NSDocument` resolve to the *same* +`UndoManager`, so give them the same stack and they correctly share one undo history. + ## License This library is released under the MIT license. See [LICENSE](LICENSE) for details. diff --git a/Sources/SQLiteUndo/UndoCoordinator.swift b/Sources/SQLiteUndo/UndoCoordinator.swift index c36dad6..8c4f7b8 100644 --- a/Sources/SQLiteUndo/UndoCoordinator.swift +++ b/Sources/SQLiteUndo/UndoCoordinator.swift @@ -19,7 +19,6 @@ final class UndoCoordinator: Sendable { private struct State { var openBarriers: [UUID: String] = [:] - var subscribers: [UUID: AsyncStream.Continuation] = [:] } init( @@ -33,28 +32,6 @@ final class UndoCoordinator: Sendable { self.untrackedTables = untrackedTables } - /// Create a new stream of undo/redo events. - /// - /// Each call creates an independent subscription that receives events emitted from - /// this point on; earlier events are not replayed. - func events() -> AsyncStream { - let id = UUID() - let (stream, continuation) = AsyncStream.makeStream() - state.withValue { $0.subscribers[id] = continuation } - continuation.onTermination = { [state] _ in - state.withValue { _ = $0.subscribers.removeValue(forKey: id) } - } - return stream - } - - /// Broadcast an event to all active subscribers. - private func emit(_ event: UndoEvent) { - // Copy out before yielding so `onTermination` can't re-enter the lock. - for continuation in state.withValue({ Array($0.subscribers.values) }) { - continuation.yield(event) - } - } - /// Begin recording changes for a new undoable action. /// /// Changes are claimed by this barrier only while `_undoBarrierID` is set to its @@ -188,20 +165,23 @@ final class UndoCoordinator: Sendable { /// Executes all reverse SQL in the barrier in reverse order. /// The executed SQL is captured by triggers, becoming the redo SQL, and is /// re-stamped with this barrier's ID so it stays owned across cycles. - func performUndo(barrier: UndoBarrier) throws { - if let affectedItems = try replay(barrier: barrier) { - emit(UndoEvent(kind: .undo, name: barrier.name, affectedItems: affectedItems)) - } + /// + /// - Returns: The event describing what changed, or nil if nothing was replayed. + /// The caller delivers it, since only it knows which undo scope this belongs to. + func performUndo(barrier: UndoBarrier) throws -> UndoEvent? { + guard let affectedItems = try replay(barrier: barrier) else { return nil } + return UndoEvent(kind: .undo, name: barrier.name, affectedItems: affectedItems) } /// Perform redo for a barrier. /// /// Re-applies the original changes that were undone. The executed SQL is /// captured by triggers, becoming the undo SQL again. - func performRedo(barrier: UndoBarrier) throws { - if let affectedItems = try replay(barrier: barrier) { - emit(UndoEvent(kind: .redo, name: barrier.name, affectedItems: affectedItems)) - } + /// + /// - Returns: The event describing what changed, or nil if nothing was replayed. + func performRedo(barrier: UndoBarrier) throws -> UndoEvent? { + guard let affectedItems = try replay(barrier: barrier) else { return nil } + return UndoEvent(kind: .redo, name: barrier.name, affectedItems: affectedItems) } /// Replay a barrier's entries. Undo and redo are the same operation — each diff --git a/Sources/SQLiteUndo/UndoEngine.swift b/Sources/SQLiteUndo/UndoEngine.swift index cc6c169..3a45064 100644 --- a/Sources/SQLiteUndo/UndoEngine.swift +++ b/Sources/SQLiteUndo/UndoEngine.swift @@ -69,13 +69,6 @@ public struct UndoEngine: Sendable { /// /// - Parameter id: The barrier ID from `beginBarrier` var cancelBarrier: @Sendable (_ id: UUID) throws -> Void - - /// Stream of events emitted after each undo/redo operation. - /// - /// Each call returns an independent subscription delivering events from that point - /// on; earlier events are not replayed. Cancelling one subscription leaves the others - /// unaffected, so callers may freely resubscribe. - public var events: @Sendable () -> AsyncStream = { .finished } } /// Whether undo tracking is active. Default true; set false inside `withUndoDisabled`. @@ -237,17 +230,24 @@ extension UndoEngine: DependencyKey { guard let barrier = try coordinator.endBarrier(id) else { return } + // Capturing `undoStack` binds this barrier — and the events its undo/redo + // produce — to the scope that registered it. undoStack.registerBarrier( barrier, - { try coordinator.performUndo(barrier: barrier) }, - { try coordinator.performRedo(barrier: barrier) } + { + if let event = try coordinator.performUndo(barrier: barrier) { + undoStack.emit(event) + } + }, + { + if let event = try coordinator.performRedo(barrier: barrier) { + undoStack.emit(event) + } + } ) }, cancelBarrier: { id in try coordinator.cancelBarrier(id) - }, - events: { - coordinator.events() } ) } diff --git a/Sources/SQLiteUndo/UndoStack.swift b/Sources/SQLiteUndo/UndoStack.swift index fe3db8b..10e59a7 100644 --- a/Sources/SQLiteUndo/UndoStack.swift +++ b/Sources/SQLiteUndo/UndoStack.swift @@ -49,6 +49,45 @@ public struct UndoStack: Sendable { /// For the `.live()` stack, this updates which UndoManager receives registrations. /// For the test stack, this is a no-op. public var setUndoManager: @Sendable (_ undoManager: UndoManager?) -> Void = { _ in } + + /// Stream of events emitted after each undo/redo performed on this stack. + /// + /// Events are scoped to the stack, so a window observes only the undos performed + /// against its own UndoManager. Windows sharing an UndoManager (as multiple + /// windows on one document do) share a stack, and so see the same events. + /// + /// Each call returns an independent subscription delivering events from that point + /// on; earlier events are not replayed. Cancelling one subscription leaves the + /// others unaffected, so callers may freely resubscribe. + public var events: @Sendable () -> AsyncStream = { .finished } + + /// Deliver an event to this stack's subscribers. + /// + /// Internal: called by `UndoEngine` from the undo/redo closures it registers, which + /// is what binds an event to the scope that performed it. + var emit: @Sendable (_ event: UndoEvent) -> Void = { _ in } +} + +/// Fan-out of undo events to any number of independent subscribers. +private final class UndoEventBroadcaster: Sendable { + private let subscribers = LockIsolated([UUID: AsyncStream.Continuation]()) + + func events() -> AsyncStream { + let id = UUID() + let (stream, continuation) = AsyncStream.makeStream() + subscribers.withValue { $0[id] = continuation } + continuation.onTermination = { [subscribers] _ in + subscribers.withValue { _ = $0.removeValue(forKey: id) } + } + return stream + } + + func emit(_ event: UndoEvent) { + // Copy out before yielding so `onTermination` can't re-enter the lock. + for continuation in subscribers.withValue({ Array($0.values) }) { + continuation.yield(event) + } + } } extension DependencyValues { @@ -69,6 +108,7 @@ extension UndoStack: DependencyKey { public static var testValue: UndoStack { let state = LockIsolated(UndoStackState(undo: [])) + let broadcaster = UndoEventBroadcaster() return UndoStack( registerBarrier: { barrier, onUndo, onRedo in @@ -83,7 +123,9 @@ extension UndoStack: DependencyKey { redo: state.value.redo.reversed() ) }, - setUndoManager: { _ in } + setUndoManager: { _ in }, + events: { broadcaster.events() }, + emit: { broadcaster.emit($0) } ) } @@ -95,6 +137,7 @@ extension UndoStack: DependencyKey { /// - Parameter undoManager: Optional initial UndoManager public static func live(_ undoManager: UndoManager? = nil) -> UndoStack { let state = LockIsolated(UndoStackState(undo: [])) + let broadcaster = UndoEventBroadcaster() // Target object for NSUndoManager registration - holds mutable UndoManager reference. // @@ -232,7 +275,9 @@ extension UndoStack: DependencyKey { } else { logger.warning("setUndoManager: nil") } - } + }, + events: { broadcaster.events() }, + emit: { broadcaster.emit($0) } ) } } diff --git a/Sources/SQLiteUndoTCA/UndoManaging.swift b/Sources/SQLiteUndoTCA/UndoManaging.swift index 18d32aa..6156f27 100644 --- a/Sources/SQLiteUndoTCA/UndoManaging.swift +++ b/Sources/SQLiteUndoTCA/UndoManaging.swift @@ -55,7 +55,6 @@ public enum UndoManagingAction: Sendable { /// ``` public struct UndoManagingReducer: Reducer { @Dependency(\.defaultUndoStack) var undoStack - @Dependency(\.defaultUndoEngine) var undoEngine private enum CancelID { case eventSubscription } @@ -68,7 +67,7 @@ public struct UndoManagingReducer Date: Tue, 4 Aug 2026 08:35:38 -0700 Subject: [PATCH 06/10] installDefaultUndoStack --- Examples/UndoForMacOS/UndoForMacOSApp.swift | 2 +- README.md | 4 +- Sources/SQLiteUndo/UndoEngine.swift | 2 - Sources/SQLiteUndo/UndoStack.swift | 43 +++++++++++++++---- .../UndoableEffectTests.swift | 6 +-- 5 files changed, 42 insertions(+), 15 deletions(-) diff --git a/Examples/UndoForMacOS/UndoForMacOSApp.swift b/Examples/UndoForMacOS/UndoForMacOSApp.swift index e6989fc..4a3267e 100644 --- a/Examples/UndoForMacOS/UndoForMacOSApp.swift +++ b/Examples/UndoForMacOS/UndoForMacOSApp.swift @@ -30,7 +30,7 @@ struct UndoForMacOSApp: App { /// Open a second window with ⌘N to see the two undo stacks operate independently. struct DemoWindow: View { @State private var store = withDependencies { - $0.defaultUndoStack = .live() + $0.installDefaultUndoStack() } operation: { Store(initialState: DemoFeature.State()) { DemoFeature() diff --git a/README.md b/README.md index 686e2df..2ebe28e 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ Give each window its own stack by scoping the dependency where its store is crea ```swift struct MyWindow: View { @State private var store = withDependencies { - $0.defaultUndoStack = .live() + $0.installDefaultUndoStack() } operation: { Store(initialState: MyFeature.State()) { MyFeature() } } @@ -221,6 +221,8 @@ struct MyWindow: View { } ``` +The default stack is already app-wide, so a single-window app needs no setup at all. `installDefaultUndoStack()` exists to create an *additional* scope — call it once per window. + Each window then has its own undo/redo stack, its own Edit menu state, and its own event stream. Barriers register with whichever stack is current when they close, and undoing in one window never touches another's changes. diff --git a/Sources/SQLiteUndo/UndoEngine.swift b/Sources/SQLiteUndo/UndoEngine.swift index 3a45064..34d50f7 100644 --- a/Sources/SQLiteUndo/UndoEngine.swift +++ b/Sources/SQLiteUndo/UndoEngine.swift @@ -16,7 +16,6 @@ private let logger = Logger(subsystem: "SQLiteUndo", category: "UndoEngine") /// ```swift /// prepareDependencies { /// $0.defaultDatabase = try! appDatabase() -/// $0.defaultUndoStack = .live(windowUndoManager) /// $0.defaultUndoEngine = try! UndoEngine( /// for: $0.defaultDatabase, /// tables: Item.self, Edit.self @@ -196,7 +195,6 @@ extension UndoEngine: DependencyKey { prepareDependencies { $0.defaultDatabase = try! appDatabase() - $0.defaultUndoStack = .live(windowUndoManager) $0.defaultUndoEngine = try! UndoEngine( for: $0.defaultDatabase, tables: MyTable1.self, MyTable2.self diff --git a/Sources/SQLiteUndo/UndoStack.swift b/Sources/SQLiteUndo/UndoStack.swift index 10e59a7..2e39303 100644 --- a/Sources/SQLiteUndo/UndoStack.swift +++ b/Sources/SQLiteUndo/UndoStack.swift @@ -10,14 +10,15 @@ private let logger = Logger(subsystem: "SQLiteUndo", category: "UndoStack") /// This type handles registration of undo/redo actions with NSUndoManager /// and tracks the undo/redo stack state for testing. /// +/// A stack is one undo scope: one undo/redo history, one Edit menu, one event +/// stream. The default stack is app-wide, which is all a single-window app needs. +/// For separate per-window histories, see ``Dependencies/DependencyValues/installDefaultUndoStack(_:)``. +/// /// ## Setup /// -/// In production, wrap the window's UndoManager: -/// ```swift -/// prepareDependencies { -/// $0.defaultUndoStack = .live(windowUndoManager) -/// } -/// ``` +/// The UndoManager usually arrives from the view's environment rather than being +/// known up front — `setUndoManager(_:)` connects it, which +/// `UndoManagingReducer` does for you in the TCA integration. /// /// In tests, use the automatic test implementation which tracks stack state /// without requiring a real UndoManager. @@ -95,6 +96,32 @@ extension DependencyValues { get { self[UndoStack.self] } set { self[UndoStack.self] = newValue } } + + /// Give the surrounding dependency scope its own undo stack. + /// + /// A stack is one undo scope: one undo/redo history, one Edit menu, one event + /// stream. The default stack is already app-wide, so a single-window app needs + /// none of this. + /// + /// Call this to create an *additional* scope — once per window, inside the + /// `withDependencies` that builds that window's store: + /// + /// ```swift + /// @State private var store = withDependencies { + /// $0.installDefaultUndoStack() + /// } operation: { + /// Store(initialState: MyFeature.State()) { MyFeature() } + /// } + /// ``` + /// + /// Windows meant to share one undo history should share one stack, so install it + /// once and hand the same value to each. + /// + /// - Parameter undoManager: The UndoManager to register with. Omit it when the + /// manager arrives later from the view's environment, as it does in SwiftUI. + public mutating func installDefaultUndoStack(_ undoManager: UndoManager? = nil) { + defaultUndoStack = .live(undoManager) + } } extension UndoStack: DependencyKey { @@ -168,7 +195,7 @@ extension UndoStack: DependencyKey { ) { guard let undoManager else { reportIssue( - "No UndoManager set. Call setUndoManager() or configure defaultUndoStack = .live(undoManager)" + "No UndoManager set. Call setUndoManager(), or install the stack with installDefaultUndoStack(undoManager)" ) logger.warning( "\(self.currentState.logDescription(after: "\"\(barrier.name)\" — undoManager is nil, registration dropped"))" @@ -210,7 +237,7 @@ extension UndoStack: DependencyKey { ) { guard let undoManager else { reportIssue( - "No UndoManager set. Call setUndoManager() or configure defaultUndoStack = .live(undoManager)" + "No UndoManager set. Call setUndoManager(), or install the stack with installDefaultUndoStack(undoManager)" ) logger.warning( "\(self.currentState.logDescription(after: "redo \"\(barrier.name)\" — undoManager is nil"))" diff --git a/Tests/SQLiteUndoTCATests/UndoableEffectTests.swift b/Tests/SQLiteUndoTCATests/UndoableEffectTests.swift index cd21f77..f82c845 100644 --- a/Tests/SQLiteUndoTCATests/UndoableEffectTests.swift +++ b/Tests/SQLiteUndoTCATests/UndoableEffectTests.swift @@ -21,7 +21,7 @@ struct UndoableEffectTests { try await withDependencies { let database = try! makeTestDatabase() $0.defaultDatabase = database - $0.defaultUndoStack = .live(testUndoManager) + $0.installDefaultUndoStack(testUndoManager) $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database @@ -47,7 +47,7 @@ struct UndoableEffectTests { try await withDependencies { let database = try! makeTestDatabase() $0.defaultDatabase = database - $0.defaultUndoStack = .live(testUndoManager) + $0.installDefaultUndoStack(testUndoManager) $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database @@ -77,7 +77,7 @@ struct UndoableEffectTests { await withDependencies { let database = try! makeTestDatabase() $0.defaultDatabase = database - $0.defaultUndoStack = .live(testUndoManager) + $0.installDefaultUndoStack(testUndoManager) $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { let store = TestStore(initialState: TestFeature.State()) { From 9a1ab00e3ae03a5cb62bc3236ce513fe169dfb0e Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 08:54:57 -0700 Subject: [PATCH 07/10] add warnings for mis-use of stack/undomanager --- Examples/UndoForMacOS/UndoForMacOSApp.swift | 1 - Sources/SQLiteUndo/UndoCoordinator.swift | 12 ++++ Sources/SQLiteUndo/UndoEngine.swift | 6 +- Sources/SQLiteUndo/UndoStack.swift | 48 ++++++++++--- Tests/SQLiteUndoTests/UndoEngineTests.swift | 79 +++++++++++++++++++++ 5 files changed, 136 insertions(+), 10 deletions(-) diff --git a/Examples/UndoForMacOS/UndoForMacOSApp.swift b/Examples/UndoForMacOS/UndoForMacOSApp.swift index 4a3267e..c8352eb 100644 --- a/Examples/UndoForMacOS/UndoForMacOSApp.swift +++ b/Examples/UndoForMacOS/UndoForMacOSApp.swift @@ -36,7 +36,6 @@ struct DemoWindow: View { DemoFeature() } } - var body: some View { DemoView(store: store) } diff --git a/Sources/SQLiteUndo/UndoCoordinator.swift b/Sources/SQLiteUndo/UndoCoordinator.swift index 8c4f7b8..de3b635 100644 --- a/Sources/SQLiteUndo/UndoCoordinator.swift +++ b/Sources/SQLiteUndo/UndoCoordinator.swift @@ -160,6 +160,18 @@ final class UndoCoordinator: Sendable { logger.debug("Cancel barrier: \(name)") } + /// Discard a closed barrier's undolog entries. + /// + /// Used when a barrier could not be registered for undo: nothing holds it, so its + /// entries can never be replayed and would otherwise sit in the log forever. The + /// database changes themselves stand — only the ability to undo them is gone. + func discardBarrier(_ id: UUID) throws { + try database.write { db in + try db.deleteUndoLogEntries(barrierID: id) + } + logger.warning("Discarded unregistered barrier \(id) — its changes are not undoable") + } + /// Perform undo for a barrier. /// /// Executes all reverse SQL in the barrier in reverse order. diff --git a/Sources/SQLiteUndo/UndoEngine.swift b/Sources/SQLiteUndo/UndoEngine.swift index 34d50f7..107f99b 100644 --- a/Sources/SQLiteUndo/UndoEngine.swift +++ b/Sources/SQLiteUndo/UndoEngine.swift @@ -230,7 +230,7 @@ extension UndoEngine: DependencyKey { } // Capturing `undoStack` binds this barrier — and the events its undo/redo // produce — to the scope that registered it. - undoStack.registerBarrier( + let registered = undoStack.registerBarrier( barrier, { if let event = try coordinator.performUndo(barrier: barrier) { @@ -243,6 +243,10 @@ extension UndoEngine: DependencyKey { } } ) + if !registered { + // Nothing holds this barrier, so its entries can never be replayed. + try coordinator.discardBarrier(barrier.id) + } }, cancelBarrier: { id in try coordinator.cancelBarrier(id) diff --git a/Sources/SQLiteUndo/UndoStack.swift b/Sources/SQLiteUndo/UndoStack.swift index 2e39303..d16ef42 100644 --- a/Sources/SQLiteUndo/UndoStack.swift +++ b/Sources/SQLiteUndo/UndoStack.swift @@ -27,12 +27,15 @@ public struct UndoStack: Sendable { /// Register a barrier for undo/redo with the UndoManager. /// /// Called by UndoEngine when a barrier completes with changes. + /// - Returns: Whether the barrier was registered. False means there was no + /// UndoManager to register with, so the barrier is unreachable and its + /// undolog entries should be discarded. public var registerBarrier: @Sendable ( _ barrier: UndoBarrier, _ onUndo: @escaping @Sendable () throws -> Void, _ onRedo: @escaping @Sendable () throws -> Void - ) -> Void = { _, _, _ in } + ) -> Bool = { _, _, _ in true } /// Returns the current undo/redo stack state. /// @@ -143,6 +146,7 @@ extension UndoStack: DependencyKey { $0.undo.append(barrier.name) $0.redo = [] } + return true }, currentState: { UndoStackState( @@ -188,11 +192,12 @@ extension UndoStack: DependencyKey { } @MainActor + @discardableResult func registerUndo( barrier: UndoBarrier, onUndo: @escaping @Sendable () throws -> Void, onRedo: @escaping @Sendable () throws -> Void - ) { + ) -> Bool { guard let undoManager else { reportIssue( "No UndoManager set. Call setUndoManager(), or install the stack with installDefaultUndoStack(undoManager)" @@ -200,7 +205,7 @@ extension UndoStack: DependencyKey { logger.warning( "\(self.currentState.logDescription(after: "\"\(barrier.name)\" — undoManager is nil, registration dropped"))" ) - return + return false } logger.debug("Registering undo: \(barrier.name)") undoManager.beginUndoGrouping() @@ -227,6 +232,7 @@ extension UndoStack: DependencyKey { } undoManager.endUndoGrouping() logger.info("\(self.currentState.logDescription(after: "register \"\(barrier.name)\""))") + return true } @MainActor @@ -277,17 +283,26 @@ extension UndoStack: DependencyKey { $0.redo = [] } // NSUndoManager requires main thread + let registered: Bool if Thread.isMainThread { - MainActor.assumeIsolated { + registered = MainActor.assumeIsolated { target.registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } } else { - DispatchQueue.main.sync { + registered = DispatchQueue.main.sync { MainActor.assumeIsolated { target.registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } } } + if !registered { + state.withValue { + if let index = $0.undo.lastIndex(of: barrier.name) { + $0.undo.remove(at: index) + } + } + } + return registered }, currentState: { UndoStackState( @@ -295,9 +310,26 @@ extension UndoStack: DependencyKey { redo: state.value.redo.reversed() ) }, - setUndoManager: { - target.undoManager = $0 - if $0 != nil { + setUndoManager: { newManager in + // Two windows sharing one stack shows up here: the second window's mount + // replaces the first's manager, and from then on every window's undo goes + // to whichever mounted last. + // + // `undoManager` is weak, so an UndoManager that was legitimately torn down + // has already gone nil. A *live* one being replaced by a different one means + // two of them are in play, which is the misconfiguration. + if let existing = target.undoManager, existing !== newManager, newManager != nil { + reportIssue( + """ + This UndoStack is being handed a second UndoManager while the first is + still in use, so undo will only ever reach whichever window registered + last. Give each window its own stack by calling installDefaultUndoStack() + in the withDependencies that builds that window's store. + """ + ) + } + target.undoManager = newManager + if newManager != nil { logger.info("setUndoManager: set") } else { logger.warning("setUndoManager: nil") diff --git a/Tests/SQLiteUndoTests/UndoEngineTests.swift b/Tests/SQLiteUndoTests/UndoEngineTests.swift index f3ce88e..766ddf3 100644 --- a/Tests/SQLiteUndoTests/UndoEngineTests.swift +++ b/Tests/SQLiteUndoTests/UndoEngineTests.swift @@ -221,6 +221,85 @@ enum UndoEngineTests { #expect(b?.name == "sentinel") } + /// Sharing one stack between windows silently sends every window's undo to + /// whichever mounted last, so it must be reported rather than left to discover. + /// This fires on the second window's mount, before any action is taken. + @Test + func warnsWhenOneStackIsHandedASecondUndoManager() { + let managerA = UndoManager() + let managerB = UndoManager() + let shared = UndoStack.live() + + shared.setUndoManager(managerA) // window A mounts + + withKnownIssue { + shared.setUndoManager(managerB) // window B mounts against the same stack + } matching: { issue in + issue.description.contains("installDefaultUndoStack") + } + } + + @Test + func noWarningWhenTheSameUndoManagerIsSetAgain() { + let manager = UndoManager() + let stack = UndoStack.live() + + // `.task(id: undoManager)` re-fires with the same manager; not a misconfiguration. + stack.setUndoManager(manager) + stack.setUndoManager(manager) + } + + @Test + func noWarningWhenThePreviousUndoManagerIsGone() { + let stack = UndoStack.live() + + // A window that closed: its manager deallocated, so the weak reference is + // already nil and the next window is not a conflict. + do { + let closing = UndoManager() + stack.setUndoManager(closing) + } + stack.setUndoManager(UndoManager()) + + // Clearing is likewise not a conflict. + stack.setUndoManager(nil) + } + + /// A barrier that could not be registered is unreachable, so its undolog + /// entries must not accumulate. + @Test + func unregisterableBarrierDiscardsItsEntries() throws { + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + $0.defaultUndoStack = .live() // never given an UndoManager + } operation: { + @Dependency(\.defaultDatabase) var database + + try withKnownIssue { + try undoable("Nowhere to register") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + } matching: { issue in + issue.description.contains("No UndoManager set") + } + + let undoLogCount = try database.read { db in + try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") + } + #expect(undoLogCount == 0, "entries for an unregisterable barrier should be discarded") + + // The write itself still stands — it just isn't undoable. + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 1) + } + } + } + /// NSUndoManager does not retain its registration target, so the stack that /// registered a barrier may be released long before the undo is performed. @Test From 3b232dcab3abfb1cb0c1f1b2aa90ceb238a2d37a Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 09:19:49 -0700 Subject: [PATCH 08/10] log the event's window --- Examples/UndoForMacOS/UndoForMacOSApp.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Examples/UndoForMacOS/UndoForMacOSApp.swift b/Examples/UndoForMacOS/UndoForMacOSApp.swift index c8352eb..12efaa5 100644 --- a/Examples/UndoForMacOS/UndoForMacOSApp.swift +++ b/Examples/UndoForMacOS/UndoForMacOSApp.swift @@ -78,6 +78,7 @@ struct DemoFeature { case .undoManager(.event(let event)): if let ids = event.ids(for: DemoItem.self) { print( + "window: \(state.windowID) received undo:", event.kind, event.name.debugDescription, ids.map { $0.formatted() } From e8b3c4b7f6593497a53ab5586e50c848ab08d85b Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 09:42:24 -0700 Subject: [PATCH 09/10] dry barrier closure --- Sources/SQLiteUndo/UndoBarrier.swift | 43 ++++++++++++++++++++++++ Sources/SQLiteUndo/UndoCoordinator.swift | 36 ++++++++++---------- Sources/SQLiteUndo/Undoable.swift | 34 ++++++++----------- 3 files changed, 76 insertions(+), 37 deletions(-) diff --git a/Sources/SQLiteUndo/UndoBarrier.swift b/Sources/SQLiteUndo/UndoBarrier.swift index fa991c2..862a07b 100644 --- a/Sources/SQLiteUndo/UndoBarrier.swift +++ b/Sources/SQLiteUndo/UndoBarrier.swift @@ -26,3 +26,46 @@ public struct UndoBarrier: Hashable, Sendable, Codable { self.count = count } } + +/// Run an operation inside a barrier: open one, claim the writes it makes, close it. +/// +/// This is the only place a barrier is paired with its `_undoBarrierID` scope. A +/// barrier claims writes solely while that task local carries its ID, so opening one +/// any other way captures nothing at all — keeping the pairing here means callers +/// can't get it wrong. +func withBarrierScope( + _ name: String, + begin: (String) throws -> UUID, + end: (UUID) throws -> Void, + cancel: (UUID) throws -> Void, + operation: () throws -> T +) throws -> T { + let id = try begin(name) + do { + let result = try $_undoBarrierID.withValue(id.uuidString) { try operation() } + try end(id) + return result + } catch { + try cancel(id) + throw error + } +} + +/// Run an async operation inside a barrier. See ``withBarrierScope(_:begin:end:cancel:operation:)``. +func withBarrierScope( + _ name: String, + begin: (String) throws -> UUID, + end: (UUID) throws -> Void, + cancel: (UUID) throws -> Void, + operation: @Sendable () async throws -> T +) async throws -> T { + let id = try begin(name) + do { + let result = try await $_undoBarrierID.withValue(id.uuidString) { try await operation() } + try end(id) + return result + } catch { + try cancel(id) + throw error + } +} diff --git a/Sources/SQLiteUndo/UndoCoordinator.swift b/Sources/SQLiteUndo/UndoCoordinator.swift index de3b635..c13fa5a 100644 --- a/Sources/SQLiteUndo/UndoCoordinator.swift +++ b/Sources/SQLiteUndo/UndoCoordinator.swift @@ -35,7 +35,7 @@ final class UndoCoordinator: Sendable { /// Begin recording changes for a new undoable action. /// /// Changes are claimed by this barrier only while `_undoBarrierID` is set to its - /// ID — see ``withBarrier(_:_:)``, which scopes that for you. + /// ID — see ``withBarrier(_:_:)-(_,()throws->Void)``, which scopes that for you. /// /// - Parameter name: The action name (shown in Edit > Undo menu) /// - Returns: A unique ID for this barrier @@ -110,14 +110,15 @@ final class UndoCoordinator: Sendable { /// - Returns: The completed barrier, or nil if no changes were captured. @discardableResult func withBarrier(_ name: String, _ operation: () throws -> Void) throws -> UndoBarrier? { - let id = try beginBarrier(name) - do { - try $_undoBarrierID.withValue(id.uuidString) { try operation() } - return try endBarrier(id) - } catch { - try cancelBarrier(id) - throw error - } + var barrier: UndoBarrier? + try withBarrierScope( + name, + begin: beginBarrier, + end: { barrier = try endBarrier($0) }, + cancel: cancelBarrier, + operation: operation + ) + return barrier } /// Run an async operation inside a barrier, returning the completed barrier. @@ -131,14 +132,15 @@ final class UndoCoordinator: Sendable { _ name: String, _ operation: @Sendable () async throws -> Void ) async throws -> UndoBarrier? { - let id = try beginBarrier(name) - do { - try await $_undoBarrierID.withValue(id.uuidString) { try await operation() } - return try endBarrier(id) - } catch { - try cancelBarrier(id) - throw error - } + var barrier: UndoBarrier? + try await withBarrierScope( + name, + begin: beginBarrier, + end: { barrier = try endBarrier($0) }, + cancel: cancelBarrier, + operation: operation + ) + return barrier } /// Cancel a barrier without registering it for undo. diff --git a/Sources/SQLiteUndo/Undoable.swift b/Sources/SQLiteUndo/Undoable.swift index 21f0dba..0961504 100644 --- a/Sources/SQLiteUndo/Undoable.swift +++ b/Sources/SQLiteUndo/Undoable.swift @@ -18,15 +18,13 @@ public func undoable( ) throws -> T { @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier(actionName) - do { - let result = try $_undoBarrierID.withValue(barrierId.uuidString) { try operation() } - try undoEngine.endBarrier(barrierId) - return result - } catch { - try undoEngine.cancelBarrier(barrierId) - throw error - } + return try withBarrierScope( + actionName, + begin: { try undoEngine.beginBarrier($0) }, + end: { try undoEngine.endBarrier($0) }, + cancel: { try undoEngine.cancelBarrier($0) }, + operation: operation + ) } /// Execute an async operation within an undoable barrier. @@ -46,17 +44,13 @@ public func undoable( ) async throws -> T { @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier(actionName) - do { - let result = try await $_undoBarrierID.withValue(barrierId.uuidString) { - try await operation() - } - try undoEngine.endBarrier(barrierId) - return result - } catch { - try undoEngine.cancelBarrier(barrierId) - throw error - } + return try await withBarrierScope( + actionName, + begin: { try undoEngine.beginBarrier($0) }, + end: { try undoEngine.endBarrier($0) }, + cancel: { try undoEngine.cancelBarrier($0) }, + operation: operation + ) } /// Execute an operation with undo tracking disabled. From 4aa83373bec01a078842ecc16bb70feb80b4d108 Mon Sep 17 00:00:00 2001 From: Ryan Carver Date: Tue, 4 Aug 2026 09:43:01 -0700 Subject: [PATCH 10/10] protect undo manager and fix ordering of ops for failed registration --- Sources/SQLiteUndo/UndoCoordinator.swift | 11 ++-- Sources/SQLiteUndo/UndoEngine.swift | 3 +- Sources/SQLiteUndo/UndoStack.swift | 60 ++++++++++++++------- Tests/SQLiteUndoTests/UndoEngineTests.swift | 60 +++++++++++++++++++++ 4 files changed, 108 insertions(+), 26 deletions(-) diff --git a/Sources/SQLiteUndo/UndoCoordinator.swift b/Sources/SQLiteUndo/UndoCoordinator.swift index c13fa5a..07450ce 100644 --- a/Sources/SQLiteUndo/UndoCoordinator.swift +++ b/Sources/SQLiteUndo/UndoCoordinator.swift @@ -148,18 +148,19 @@ final class UndoCoordinator: Sendable { /// Any changes made within the barrier remain in the database but won't /// be undoable as a group. Use this for aborted operations. /// + /// The entries are deleted whether or not the barrier is still open. A barrier that + /// threw on its way out of `endBarrier` has already been forgotten here, and its + /// rows would otherwise be orphaned in the log with nothing left to replay them. + /// /// - Parameter id: The barrier ID returned from `beginBarrier` func cancelBarrier(_ id: UUID) throws { - guard let name = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { - logger.warning("Attempted to cancel unknown barrier: \(id)") - return - } + let name = state.withValue { $0.openBarriers.removeValue(forKey: id) } try database.write { db in try db.deleteUndoLogEntries(barrierID: id) } - logger.debug("Cancel barrier: \(name)") + logger.debug("Cancel barrier: \(name ?? id.uuidString)") } /// Discard a closed barrier's undolog entries. diff --git a/Sources/SQLiteUndo/UndoEngine.swift b/Sources/SQLiteUndo/UndoEngine.swift index 107f99b..8a4fe48 100644 --- a/Sources/SQLiteUndo/UndoEngine.swift +++ b/Sources/SQLiteUndo/UndoEngine.swift @@ -49,7 +49,8 @@ public struct UndoEngine: Sendable { /// Begin recording changes for a new undoable action. /// /// Internal: a barrier only claims writes made while `_undoBarrierID` is set to - /// its ID, so barriers must be opened through ``undoable(_:operation:)-3cgh0``. + /// its ID. Never call this directly — ``withBarrierScope(_:begin:end:cancel:operation:)`` + /// is what pairs the two, and ``undoable(_:operation:)-3cgh0`` is the public door to it. /// /// - Parameter name: The action name (shown in Edit > Undo menu) /// - Returns: A unique ID for this barrier diff --git a/Sources/SQLiteUndo/UndoStack.swift b/Sources/SQLiteUndo/UndoStack.swift index d16ef42..8d02270 100644 --- a/Sources/SQLiteUndo/UndoStack.swift +++ b/Sources/SQLiteUndo/UndoStack.swift @@ -175,13 +175,41 @@ extension UndoStack: DependencyKey { // `undoManager` is weak because the window owns its UndoManager, not us. That // also keeps the registration closures below (which capture this target // strongly, since NSUndoManager does not retain its target) from cycling. + // + // It sits behind a lock because `setUndoManager` is called from wherever the view + // layer runs while registration reads it on the main actor. Racing loads and + // stores of a *weak* reference are not merely torn values — they corrupt the + // runtime's side table. final class UndoTarget: @unchecked Sendable { + private struct WeakManager { + weak var value: UndoManager? + } + let state: LockIsolated - weak var undoManager: UndoManager? + private let manager: LockIsolated init(state: LockIsolated, undoManager: UndoManager?) { self.state = state - self.undoManager = undoManager + self.manager = LockIsolated(WeakManager(value: undoManager)) + } + + var undoManager: UndoManager? { + manager.withValue { $0.value } + } + + /// Point this target at a new UndoManager. + /// + /// - Returns: Whether a different, still-live manager was displaced — the + /// signature of two windows sharing one stack. Read and write happen under + /// one lock so the answer can't be stale by the time it's reported. + func setUndoManager(_ newManager: UndoManager?) -> Bool { + manager.withValue { box in + defer { box.value = newManager } + // A manager that was legitimately torn down has already gone nil, and + // clearing the manager is never a conflict. + guard let existing = box.value, newManager != nil else { return false } + return existing !== newManager + } } var currentState: UndoStackState { @@ -231,7 +259,6 @@ extension UndoStack: DependencyKey { } } undoManager.endUndoGrouping() - logger.info("\(self.currentState.logDescription(after: "register \"\(barrier.name)\""))") return true } @@ -278,10 +305,6 @@ extension UndoStack: DependencyKey { return UndoStack( registerBarrier: { barrier, onUndo, onRedo in - state.withValue { - $0.undo.append(barrier.name) - $0.redo = [] - } // NSUndoManager requires main thread let registered: Bool if Thread.isMainThread { @@ -295,14 +318,16 @@ extension UndoStack: DependencyKey { } } } - if !registered { - state.withValue { - if let index = $0.undo.lastIndex(of: barrier.name) { - $0.undo.remove(at: index) - } - } + // Only a barrier that reached the UndoManager belongs on the stack. Pushing + // first and rolling back would clear a redo stack that is still perfectly + // valid, since nothing new was actually performed. + guard registered else { return false } + state.withValue { + $0.undo.append(barrier.name) + $0.redo = [] } - return registered + logger.info("\(target.currentState.logDescription(after: "register \"\(barrier.name)\""))") + return true }, currentState: { UndoStackState( @@ -314,11 +339,7 @@ extension UndoStack: DependencyKey { // Two windows sharing one stack shows up here: the second window's mount // replaces the first's manager, and from then on every window's undo goes // to whichever mounted last. - // - // `undoManager` is weak, so an UndoManager that was legitimately torn down - // has already gone nil. A *live* one being replaced by a different one means - // two of them are in play, which is the misconfiguration. - if let existing = target.undoManager, existing !== newManager, newManager != nil { + if target.setUndoManager(newManager) { reportIssue( """ This UndoStack is being handed a second UndoManager while the first is @@ -328,7 +349,6 @@ extension UndoStack: DependencyKey { """ ) } - target.undoManager = newManager if newManager != nil { logger.info("setUndoManager: set") } else { diff --git a/Tests/SQLiteUndoTests/UndoEngineTests.swift b/Tests/SQLiteUndoTests/UndoEngineTests.swift index 766ddf3..04314c5 100644 --- a/Tests/SQLiteUndoTests/UndoEngineTests.swift +++ b/Tests/SQLiteUndoTests/UndoEngineTests.swift @@ -108,6 +108,26 @@ enum UndoEngineTests { } #expect(undoLogCount == 0) } + + /// A barrier that threw on its way out of `endBarrier` is already forgotten by + /// the time the cancel runs, so cancelling must not depend on it still being open. + @Test + func cancellingAClosedBarrierStillRemovesItsEntries() throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + let barrier = try engine.withBarrier("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! + + try engine.cancelBarrier(barrier.id) + + let undoLogCount = try database.read { db in + try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") + } + #expect(undoLogCount == 0) + } } @Suite @@ -300,6 +320,46 @@ enum UndoEngineTests { } } + /// A dropped registration performed nothing, so it must not clear a redo stack + /// whose entries are still perfectly valid. + @Test + func droppedRegistrationLeavesTheRedoStackIntact() throws { + let manager = UndoManager() + + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + $0.defaultUndoStack = .live(manager) + } operation: { + @Dependency(\.defaultDatabase) var database + @Dependency(\.defaultUndoStack) var undoStack + + try undoable("A") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + manager.undo() + #expect(undoStack.currentState() == UndoStackState(undo: [], redo: ["A"])) + + // The window goes away, taking its UndoManager with it. + undoStack.setUndoManager(nil) + + try withKnownIssue { + try undoable("B") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "b") }.execute(db) + } + } + } matching: { issue in + issue.description.contains("No UndoManager set") + } + + #expect(undoStack.currentState() == UndoStackState(undo: [], redo: ["A"])) + } + } + /// NSUndoManager does not retain its registration target, so the stack that /// registered a barrier may be released long before the undo is performed. @Test