Pre-push review: T-2232 heartbeat context isolation

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex d74cfde..a176dd8 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  ## [Unreleased] +- T-2232: Sync heartbeats now fetch, mutate, and save their singleton through a fresh `ModelContext` over the live container, so background sync writes cannot persist unrelated changes pending in the shared app context. Failed heartbeat saves are rolled back and logged before the isolated context is discarded; deterministic regressions cover successful writes, fetch failures, save failures, and retry recovery.+ - Restored the original width-adaptive dashboard layout: iPhone portrait keeps the multi-column Kanban board whenever at least two 200-point columns fit, and uses segmented single-column navigation only below that boundary. This supersedes T-1802 / PR #205 and corrects requirement 13.1. - Dashboard UI tests now dismiss the iOS search presentation, navigate toolbar overflow actions, match combined detail-row accessibility labels, and disambiguate nested alert buttons so the full iPhone 17 suite covers the restored multi-column portrait layout reliably. 

diff --git a/Transit/Transit/Services/SyncManager.swift b/Transit/Transit/Services/SyncManager.swiftindex 26a4edd..b97324b 100644--- a/Transit/Transit/Services/SyncManager.swift+++ b/Transit/Transit/Services/SyncManager.swift@@ -40,7 +40,11 @@ final class SyncManager {     typealias HeartbeatFetcher =         (ModelContext, FetchDescriptor<SyncHeartbeat>) throws -> [SyncHeartbeat] +    /// Saves a heartbeat context. Injectable so tests can exercise write failures.+    typealias HeartbeatSaver = (ModelContext) throws -> Void+     private let heartbeatFetcher: HeartbeatFetcher+    private let heartbeatSaver: HeartbeatSaver      /// True when the preference no longer matches the mode the live container runs in,     /// i.e. the user changed the toggle and has not relaunched yet.@@ -51,6 +55,9 @@ final class SyncManager {     init(         heartbeatFetcher: @escaping HeartbeatFetcher = { context, descriptor in             try context.fetch(descriptor)+        },+        heartbeatSaver: @escaping HeartbeatSaver = { context in+            try context.save()         }     ) {         // Default to enabled if never set@@ -62,6 +69,7 @@ final class SyncManager {         self.isSyncEnabled = enabled         self.isCloudSyncActive = enabled         self.heartbeatFetcher = heartbeatFetcher+        self.heartbeatSaver = heartbeatSaver     }      // MARK: - Public API@@ -113,7 +121,7 @@ final class SyncManager {     /// Starts a 60-second repeating heartbeat that writes to SwiftData,     /// triggering CloudKit to pull pending remote changes. Scheduling is gated on the     /// launch-fixed `isCloudSyncActive` mode, not the mutable sync preference.-    func startHeartbeat(context: ModelContext) {+    func startHeartbeat(container: ModelContainer) {         stopHeartbeat()         guard isCloudSyncActive else { return } @@ -121,7 +129,7 @@ final class SyncManager {             while !Task.isCancelled {                 try? await Task.sleep(for: .seconds(60))                 guard !Task.isCancelled else { break }-                beat(context: context)+                beat(container: container)             }         }     }@@ -134,7 +142,12 @@ final class SyncManager {      /// Writes a timestamp to the `SyncHeartbeat` singleton, triggering a     /// CloudKit sync cycle that pulls pending remote changes.-    func beat(context: ModelContext) {+    ///+    /// Each beat owns a fresh context so its save cannot commit changes pending in+    /// the app's shared context. The context never escapes this method, so rolling it+    /// back and discarding it is sufficient cleanup after a failed save.+    func beat(container: ModelContainer) {+        let context = ModelContext(container)         let singletonID = SyncHeartbeat.singletonID         let descriptor = FetchDescriptor<SyncHeartbeat>(             predicate: #Predicate { $0.id == singletonID }@@ -155,7 +168,15 @@ final class SyncManager {         if heartbeat.modelContext == nil {             context.insert(heartbeat)         }-        try? context.save()++        do {+            try heartbeatSaver(context)+        } catch {+            context.rollback()+            Self.logger.error(+                "Skipping sync heartbeat because the save failed: \(error.localizedDescription)"+            )+        }     }      // MARK: - CloudKit Schema

diff --git a/Transit/Transit/TransitApp.swift b/Transit/Transit/TransitApp.swiftindex 6176c98..5440def 100644--- a/Transit/Transit/TransitApp.swift+++ b/Transit/Transit/TransitApp.swift@@ -288,7 +288,7 @@ struct TransitApp: App {         // Skip MCP server in unit test host to avoid port conflicts across test runs         guard mcpSettings.isEnabled, !Self.isUnitTestHost else { return }         await mcpServer.start(port: mcpSettings.port)-        syncManager.startHeartbeat(context: container.mainContext)+        syncManager.startHeartbeat(container: container)     }     #endif 

diff --git a/Transit/Transit/Views/Settings/SettingsView.swift b/Transit/Transit/Views/Settings/SettingsView.swiftindex c9a3959..0f8dca6 100644--- a/Transit/Transit/Views/Settings/SettingsView.swift+++ b/Transit/Transit/Views/Settings/SettingsView.swift@@ -350,7 +350,7 @@ extension SettingsView {                         .onChange(of: mcpSettings.isEnabled) { _, enabled in                             if enabled {                                 scheduleMCP(.start)-                                syncManager.startHeartbeat(context: modelContext)+                                syncManager.startHeartbeat(container: modelContext.container)                             } else {                                 mcpPortChangeCoordinator.cancelPendingPortChange()                                 scheduleMCP(.stop)

diff --git a/Transit/TransitTests/SyncManagerTests.swift b/Transit/TransitTests/SyncManagerTests.swiftindex 1c3b3cb..8acb0ec 100644--- a/Transit/TransitTests/SyncManagerTests.swift+++ b/Transit/TransitTests/SyncManagerTests.swift@@ -72,7 +72,7 @@ struct SyncManagerTests {             let fixture = try TestModelContainer()             let manager = SyncManager()             manager.recordActiveCloudSync(true)-            manager.startHeartbeat(context: fixture.context)+            manager.startHeartbeat(container: fixture.container)             defer { manager.stopHeartbeat() }              #expect(manager.isHeartbeatRunning,@@ -87,7 +87,7 @@ struct SyncManagerTests {             let fixture = try TestModelContainer()             let manager = SyncManager()             manager.recordActiveCloudSync(true)-            manager.startHeartbeat(context: fixture.context)+            manager.startHeartbeat(container: fixture.container)             defer { manager.stopHeartbeat() }              manager.setSyncEnabled(false)@@ -107,7 +107,7 @@ struct SyncManagerTests {             let manager = SyncManager()             manager.recordActiveCloudSync(false)             manager.setSyncEnabled(true)-            manager.startHeartbeat(context: fixture.context)+            manager.startHeartbeat(container: fixture.container)              #expect(manager.isSyncEnabled)             #expect(manager.syncChangeRequiresRestart)@@ -123,7 +123,7 @@ struct SyncManagerTests {             let fixture = try TestModelContainer()             let manager = SyncManager()             manager.recordActiveCloudSync(true)-            manager.startHeartbeat(context: fixture.context)+            manager.startHeartbeat(container: fixture.container)             defer { manager.stopHeartbeat() }              manager.setSyncEnabled(false)@@ -142,7 +142,7 @@ struct SyncManagerTests {             let fixture = try TestModelContainer()             let manager = SyncManager()             manager.recordActiveCloudSync(true)-            manager.startHeartbeat(context: fixture.context)+            manager.startHeartbeat(container: fixture.container)             manager.setSyncEnabled(false)              manager.stopHeartbeat()@@ -155,6 +155,7 @@ struct SyncManagerTests {     // MARK: - T-1699 Regression: heartbeat singleton fetch failures      private struct FetchFailure: Swift.Error {}+    private struct SaveFailure: Swift.Error {}      private final class HeartbeatFetchController {         var shouldFail = false@@ -170,21 +171,44 @@ struct SyncManagerTests {         }     } +    private final class HeartbeatSaveController {+        var shouldFail = false++        func save(context: ModelContext) throws {+            if shouldFail {+                throw SaveFailure()+            }+            try context.save()+        }+    }+     private func heartbeatCount(in context: ModelContext) throws -> Int {         try context.fetch(FetchDescriptor<SyncHeartbeat>()).count     } +    private func projectCount(in context: ModelContext) throws -> Int {+        try context.fetch(FetchDescriptor<Project>()).count+    }++    // MARK: - T-2232 Regression: heartbeat context isolation+     @Test-    func heartbeatWithMissingSingletonInsertsAndSavesOneRecord() throws {+    func successfulHeartbeatDoesNotSaveUnrelatedPendingChanges() throws {         let fixture = try TestModelContainer()         let context = fixture.context+        context.insert(Project(name: "Pending", description: "", gitRepo: nil, colorHex: ""))         let manager = SyncManager() -        manager.beat(context: context)+        manager.beat(container: fixture.container) -        #expect(try heartbeatCount(in: context) == 1)-        #expect(!context.hasChanges,-                "A successful missing-singleton heartbeat must save its newly inserted record")+        // Expected: the heartbeat commits independently. Actual before T-2232: saving+        // the caller's context also committed this pending project.+        #expect(context.hasChanges,+                "A successful heartbeat must leave the caller's pending changes unsaved")+        let verificationContext = ModelContext(fixture.container)+        #expect(try heartbeatCount(in: verificationContext) == 1)+        #expect(try projectCount(in: verificationContext) == 0,+                "A successful heartbeat must not persist unrelated caller-context models")     }      @Test@@ -197,38 +221,83 @@ struct SyncManagerTests {         try context.save()         let manager = SyncManager() -        manager.beat(context: context)+        manager.beat(container: fixture.container) -        #expect(try heartbeatCount(in: context) == 1)-        #expect(existing.lastBeat > .distantPast)-        #expect(!context.hasChanges,-                "A successful existing-singleton heartbeat must save its updated timestamp")+        let verificationContext = ModelContext(fixture.container)+        let storedHeartbeat = try #require(+            verificationContext.fetch(FetchDescriptor<SyncHeartbeat>()).first+        )+        #expect(try heartbeatCount(in: verificationContext) == 1)+        #expect(storedHeartbeat.lastBeat > .distantPast)     }      @Test-    func heartbeatFetchFailureDoesNotInsertOrSaveAndNextBeatRecovers() throws {+    func heartbeatFetchFailureDoesNotInsertOrSaveAndNextBeatRecoversInIsolation() throws {         let fixture = try TestModelContainer()         let context = fixture.context-        let pendingProject = Project(name: "Pending", description: "", gitRepo: nil, colorHex: "")-        context.insert(pendingProject)+        context.insert(Project(name: "Pending", description: "", gitRepo: nil, colorHex: ""))         let fetcher = HeartbeatFetchController()         let manager = SyncManager(heartbeatFetcher: fetcher.fetch)         fetcher.shouldFail = true -        manager.beat(context: context)+        manager.beat(container: fixture.container) -        #expect(try heartbeatCount(in: context) == 0,+        let failedBeatVerification = ModelContext(fixture.container)+        #expect(try heartbeatCount(in: failedBeatVerification) == 0,                 "A failed singleton fetch must not be treated as a missing record")         #expect(context.hasChanges,-                "A failed singleton fetch must return before it can save unrelated pending changes")+                "A failed singleton fetch must leave unrelated caller changes pending")          fetcher.shouldFail = false-        manager.beat(context: context)+        manager.beat(container: fixture.container) -        #expect(try heartbeatCount(in: context) == 1,+        let recoveredBeatVerification = ModelContext(fixture.container)+        #expect(try heartbeatCount(in: recoveredBeatVerification) == 1,                 "The next heartbeat must retry normally after a transient fetch failure")-        #expect(!context.hasChanges,-                "The recovered heartbeat should retain the normal best-effort save behavior")+        #expect(context.hasChanges,+                "A recovered heartbeat must still leave unrelated caller changes pending")+        #expect(try projectCount(in: recoveredBeatVerification) == 0,+                "Recovery must not persist unrelated caller-context models")+    }++    @Test+    func heartbeatSaveFailureDoesNotDirtyCallerContextAndNextBeatRecovers() throws {+        let fixture = try TestModelContainer()+        let context = fixture.context+        let existing = SyncHeartbeat()+        existing.lastBeat = .distantPast+        context.insert(existing)+        try context.save()+        context.insert(Project(name: "Pending", description: "", gitRepo: nil, colorHex: ""))+        let saver = HeartbeatSaveController()+        let manager = SyncManager(heartbeatSaver: saver.save)+        saver.shouldFail = true++        manager.beat(container: fixture.container)++        // Expected: only an isolated heartbeat context becomes dirty and is discarded.+        // Actual before T-2232: the shared heartbeat model remains mutated in the caller.+        #expect(existing.lastBeat == .distantPast,+                "A failed heartbeat save must not leave heartbeat state dirty in the caller context")+        #expect(context.hasChanges,+                "The caller's unrelated pending project must remain pending")+        let failedBeatVerification = ModelContext(fixture.container)+        let storedAfterFailure = try #require(+            failedBeatVerification.fetch(FetchDescriptor<SyncHeartbeat>()).first+        )+        #expect(storedAfterFailure.lastBeat == .distantPast)+        #expect(try projectCount(in: failedBeatVerification) == 0)++        saver.shouldFail = false+        manager.beat(container: fixture.container)++        let recoveredBeatVerification = ModelContext(fixture.container)+        let storedAfterRecovery = try #require(+            recoveredBeatVerification.fetch(FetchDescriptor<SyncHeartbeat>()).first+        )+        #expect(storedAfterRecovery.lastBeat > .distantPast)+        #expect(context.hasChanges)+        #expect(try projectCount(in: recoveredBeatVerification) == 0)     }      @Test@@ -240,9 +309,9 @@ struct SyncManagerTests {             fetcher.shouldFail = true             let manager = SyncManager(heartbeatFetcher: fetcher.fetch) -            manager.startHeartbeat(context: fixture.context)+            manager.startHeartbeat(container: fixture.container)             defer { manager.stopHeartbeat() }-            manager.beat(context: fixture.context)+            manager.beat(container: fixture.container)              #expect(manager.isHeartbeatRunning,                     "A failed beat must leave the existing timer scheduled to retry at its next interval")

diff --git a/specs/bugfixes/sync-heartbeat-context-isolation/report.md b/specs/bugfixes/sync-heartbeat-context-isolation/report.mdnew file mode 100644index 0000000..0900100--- /dev/null+++ b/specs/bugfixes/sync-heartbeat-context-isolation/report.md@@ -0,0 +1,104 @@+# Bugfix Report: Sync Heartbeat Context Isolation++**Date:** 2026-08-30+**Status:** Fixed++## Description of the Issue++The periodic sync heartbeat writes through the caller-provided `ModelContext`, which is the app's shared main context in production. `ModelContext.save()` persists every pending change in that context, so a background heartbeat can commit unrelated work before its owning UI or automation workflow chooses to save it. If the heartbeat save fails, the swallowed error can also leave the heartbeat mutation dirty in the shared context.++**Reproduction steps:**+1. Insert or modify an unrelated model in the context passed to `SyncManager.beat(context:)` without saving it.+2. Run a successful heartbeat, or force a heartbeat save failure and then retry.+3. Observe that a successful beat persists the unrelated model, while a failed save leaves heartbeat state mixed into the caller's pending changes.++**Impact:** Background maintenance can violate transaction ownership for any pending insert, update, or delete on the shared app context. The 60-second timer makes this timing-dependent and can affect UI and automation workflows.++## Investigation Summary++A structured inspection traced the heartbeat from app and Settings call sites through its fetch, mutation, and save behavior.++- **Symptoms examined:** Successful heartbeat saves clear unrelated caller changes; failed heartbeat saves are swallowed after mutating a caller-owned model.+- **Code inspected:** `TransitApp.swift`, `SettingsView.swift`, `Services/SyncManager.swift`, `Models/SyncHeartbeat.swift`, and `TransitTests/SyncManagerTests.swift`.+- **Hypotheses tested:** The singleton fetch guard prevents duplicate insertion and fetch failures, but does not isolate writes. Timer lifecycle and CloudKit launch-mode gating are unrelated to the transaction leak.++### Systematic inspection defects++1. **Data flow — `SyncManager.swift` heartbeat API:** `startHeartbeat(context:)` retains a caller-owned context and supplies it to each maintenance beat.+2. **Database interaction — `SyncManager.swift` beat implementation:** The heartbeat singleton is fetched and mutated in that same caller-owned context.+3. **Transaction handling — `SyncManager.swift` save:** Saving the context commits all registered pending changes, not only `SyncHeartbeat`.+4. **Error handling — `SyncManager.swift` save:** `try?` suppresses write failures without cleaning up the heartbeat mutation from the shared context.+5. **Test expectation — `SyncManagerTests.swift`:** The prior recovery test treated clearing all shared-context changes as normal behavior, encoding the defect rather than transaction isolation.++## Discovered Root Cause++The heartbeat assumes a SwiftData context save is scoped to the heartbeat model. A `ModelContext` is actually a unit of work: its save persists every pending insert, update, and delete registered in that context.++**Defect type:** Data-flow and transaction-isolation error.++**Five Whys:**+1. Why are unrelated changes persisted? Because the heartbeat calls `save()` on the context containing them.+2. Why does the heartbeat receive that context? Because app and Settings integrations pass their shared environment/main context into `startHeartbeat`.+3. Why is that unsafe? Because the heartbeat is background maintenance with independent transaction ownership, while the shared context belongs to interactive and automation workflows.+4. Why can failure contaminate later saves? Because the heartbeat mutates a model before a fallible save and suppresses the error without rollback or disposal.+5. Why did tests allow it? Because the existing recovery assertion described a context-wide save as expected best-effort behavior rather than asserting isolation.++**Contributing factors:** The heartbeat must touch the live container to trigger CloudKit, but that requirement was conflated with using the live container's shared context. SwiftData permits an independent `ModelContext` over the same `ModelContainer`, which separates change tracking while retaining the same store and CloudKit configuration.++## Resolution for the Issue++`SyncManager.startHeartbeat` and `beat` now accept the live `ModelContainer` rather than a caller-owned `ModelContext`. Every beat creates a fresh `ModelContext(container)` and performs the singleton fetch, timestamp mutation, insertion when needed, and save within that isolated unit of work. A successful heartbeat therefore persists only heartbeat-context changes and cannot clear or commit pending work in the app's shared context.++Fetch failures are logged and return before mutation. Save failures call `context.rollback()`, log the error, and then discard the isolated context. Ordinary rollback is safe here because no fetched heartbeat model or context escapes `beat`; the context is immediately abandoned, so the SwiftData re-fault caveat for caller-observed models does not apply.++The app launch and Settings call sites now pass their live container. A deterministic save seam remains for failure-path regression coverage.++**Alternative considered:** Continue accepting a caller context and derive `context.container` inside the heartbeat. This would isolate persistence, but was rejected because a container-based API makes transaction ownership explicit and prevents future callers from assuming the supplied context participates in the beat.++## Regression Test++**Test file:** `Transit/TransitTests/SyncManagerTests.swift`++**Tests:**+- `successfulHeartbeatDoesNotSaveUnrelatedPendingChanges`+- `heartbeatFetchFailureDoesNotInsertOrSaveAndNextBeatRecoversInIsolation`+- `heartbeatSaveFailureDoesNotDirtyCallerContextAndNextBeatRecovers`++**What they verify:** Successful, fetch-failed, and save-failed heartbeat paths leave unrelated caller-context changes pending and unpersisted; heartbeat retries still commit their own isolated record.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Services/SyncManager.swift` | Creates a fresh heartbeat context over the supplied container; rolls back and logs failed saves |+| `Transit/Transit/TransitApp.swift` | Passes the live model container when starting the app heartbeat |+| `Transit/Transit/Views/Settings/SettingsView.swift` | Passes the environment context's container when starting from Settings |+| `Transit/TransitTests/SyncManagerTests.swift` | Verifies isolation for success, fetch failure, save failure, and retry paths |+| `CHANGELOG.md` | Records the completed T-2232 fix |+| `specs/bugfixes/sync-heartbeat-context-isolation/report.md` | Root-cause, resolution, and verification record |++## Verification++**Automated:**+- [x] Regression tests pass (`make test-quick`)+- [x] Full iOS test suite passes (`make test`)+- [x] UI test suite passes (`make test-ui`)+- [x] Linters/validators pass (`make lint`)++**Manual verification:**+- The red phase failed only the three new T-2232 isolation regressions against the shared-context implementation.+- The same regressions passed after moving heartbeat persistence into a fresh context.++## Prevention++**Recommendations to avoid similar bugs:**+- Give background maintenance operations their own `ModelContext` when they do not own pending UI/service changes.+- Do not assume `ModelContext.save()` is model-scoped.+- Exercise both save success and save failure when testing background persistence paths.++## Related++- Transit T-2232+- Prior context-isolation constraint: T-173 applies to services mutating view-provided models; this heartbeat is the opposite case because it owns no view model and requires an isolated maintenance transaction.