From 4a09541af544f7c5e7e32f5f1b1fc848032e4cba Mon Sep 17 00:00:00 2001 From: Alex Southwell Date: Fri, 11 Sep 2026 17:33:54 +1000 Subject: [PATCH 1/3] fix(swift-ios): ignore stale device reads after revocation A device list reload that finishes after a revoke could put the removed device back on screen. Each reload and revoke now takes a generation, and only the newest operation may write results or clear its spinner. Co-Authored-By: Claude Opus 5 (1M context) --- .../Features/Devices/DevicesView.swift | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/apps/swift-ios/Features/Devices/DevicesView.swift b/apps/swift-ios/Features/Devices/DevicesView.swift index 002a06bea64f4..2920d01e71ef2 100644 --- a/apps/swift-ios/Features/Devices/DevicesView.swift +++ b/apps/swift-ios/Features/Devices/DevicesView.swift @@ -9,6 +9,7 @@ public struct DevicesView: View { @State private var errorMessage: String? @State private var revokeTarget: FeatureDeviceSession? @State private var showingRevokeOthers = false + @State private var operationGeneration: UInt64 = 0 public init(manager: any FeatureDeviceManaging) { self.manager = manager @@ -184,43 +185,65 @@ public struct DevicesView: View { @MainActor private func reload() async { + guard !isRevoking else { return } + operationGeneration &+= 1 + let generation = operationGeneration isLoading = true - defer { isLoading = false } + defer { + if operationGeneration == generation { isLoading = false } + } do { - sessions = FeatureDeviceSession.sortedForDisplay( + let loaded = FeatureDeviceSession.sortedForDisplay( try await manager.loadDeviceSessions() ) + guard operationGeneration == generation else { return } + sessions = loaded errorMessage = nil } catch { + guard operationGeneration == generation else { return } errorMessage = DeviceManagementErrorCopy.message(for: error) } } @MainActor private func revoke(_ session: FeatureDeviceSession) async { + operationGeneration &+= 1 + let generation = operationGeneration + isLoading = false isRevoking = true defer { - isRevoking = false - revokeTarget = nil + if operationGeneration == generation { + isRevoking = false + revokeTarget = nil + } } do { try await manager.revokeDeviceSession(id: session.id) + guard operationGeneration == generation else { return } sessions.removeAll { $0.id == session.id } errorMessage = nil } catch { + guard operationGeneration == generation else { return } errorMessage = DeviceManagementErrorCopy.message(for: error) } } @MainActor private func revokeOthers() async { + operationGeneration &+= 1 + let generation = operationGeneration + isLoading = false isRevoking = true - defer { isRevoking = false } + defer { + if operationGeneration == generation { isRevoking = false } + } do { try await manager.revokeOtherDeviceSessions() + guard operationGeneration == generation else { return } sessions.removeAll { !$0.isCurrent } errorMessage = nil } catch { + guard operationGeneration == generation else { return } errorMessage = DeviceManagementErrorCopy.message(for: error) } } From f633e6790fecd0813f8fa380836d1f2c1608f907 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 22:19:03 +1000 Subject: [PATCH 2/3] test(swift-ios): hold a device reload open across a revoke Move device access state into DevicesModel so the race is testable. The tests hold a reload open, revoke a device, then release the stale result and check that the device stays removed and the newer feedback survives. Only reloads are generation-checked. One access change runs at a time: removal actions are disabled while access is changing, and the model refuses an overlapping change, so a second removal cannot clear the first one's error. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../Features/Devices/DevicesView.swift | 216 +++++++++--------- .../FeatureTests/DeviceManagementTests.swift | 162 +++++++++++++ 2 files changed, 271 insertions(+), 107 deletions(-) diff --git a/apps/swift-ios/Features/Devices/DevicesView.swift b/apps/swift-ios/Features/Devices/DevicesView.swift index 2920d01e71ef2..1c7ad264d03c9 100644 --- a/apps/swift-ios/Features/Devices/DevicesView.swift +++ b/apps/swift-ios/Features/Devices/DevicesView.swift @@ -1,38 +1,111 @@ +import Observation import SwiftUI -public struct DevicesView: View { - private let manager: any FeatureDeviceManaging +/// Device access state. One access change runs at a time, and reloads that +/// overlap it are discarded, so a slow read cannot restore a removed device or +/// replace newer feedback. +@MainActor +@Observable +final class DevicesModel { + private(set) var sessions: [FeatureDeviceSession] = [] + private(set) var isLoading = true + private(set) var isRevoking = false + private(set) var errorMessage: String? + var revokeTarget: FeatureDeviceSession? + + let manager: any FeatureDeviceManaging + private var loadGeneration: UInt64 = 0 + + init(manager: any FeatureDeviceManaging) { + self.manager = manager + } + + var currentSession: FeatureDeviceSession? { + sessions.first(where: \.isCurrent) + } + + var otherSessions: [FeatureDeviceSession] { + sessions.filter { !$0.isCurrent } + } - @State private var sessions: [FeatureDeviceSession] = [] - @State private var isLoading = true - @State private var isRevoking = false - @State private var errorMessage: String? - @State private var revokeTarget: FeatureDeviceSession? + func reload() async { + guard !isRevoking else { return } + loadGeneration &+= 1 + let generation = loadGeneration + isLoading = true + do { + let loaded = FeatureDeviceSession.sortedForDisplay( + try await manager.loadDeviceSessions() + ) + guard loadGeneration == generation else { return } + sessions = loaded + errorMessage = nil + } catch { + guard loadGeneration == generation else { return } + errorMessage = DeviceManagementErrorCopy.message(for: error) + } + isLoading = false + } + + func revoke(_ session: FeatureDeviceSession) async { + await updateAccess(removing: { $0.id == session.id }) { + try await manager.revokeDeviceSession(id: session.id) + } + revokeTarget = nil + } + + func revokeOthers() async { + await updateAccess(removing: { !$0.isCurrent }) { + try await manager.revokeOtherDeviceSessions() + } + } + + private func updateAccess( + removing shouldRemove: (FeatureDeviceSession) -> Bool, + _ operation: () async throws -> Void + ) async { + guard !isRevoking else { return } + // Reloads cannot start while access is changing, so only earlier reads are stale. + loadGeneration &+= 1 + isLoading = false + isRevoking = true + defer { isRevoking = false } + do { + try await operation() + sessions.removeAll(where: shouldRemove) + errorMessage = nil + } catch { + errorMessage = DeviceManagementErrorCopy.message(for: error) + } + } +} + +public struct DevicesView: View { + @State private var model: DevicesModel @State private var showingRevokeOthers = false - @State private var operationGeneration: UInt64 = 0 public init(manager: any FeatureDeviceManaging) { - self.manager = manager + _model = State(initialValue: DevicesModel(manager: manager)) } public var body: some View { Group { - if isLoading, sessions.isEmpty { + if model.isLoading, model.sessions.isEmpty { Text("Loading devices") .font(T3Typography.supporting) .foregroundStyle(T3Colors.textTertiary) - } else if let errorMessage, sessions.isEmpty { + } else if let errorMessage = model.errorMessage, model.sessions.isEmpty { ContentUnavailableView { Label("Couldn’t load devices", systemImage: "exclamationmark.circle") } description: { Text(errorMessage) } actions: { Button("Try again") { - Task { await reload() } + Task { await model.reload() } } .buttonStyle(.borderedProminent) } - } else if sessions.isEmpty { + } else if model.sessions.isEmpty { ContentUnavailableView { Label("No devices found", systemImage: "laptopcomputer.and.iphone") } description: { @@ -47,7 +120,7 @@ public struct DevicesView: View { .navigationTitle("Devices") .navigationBarTitleDisplayMode(.inline) .toolbar { - if !otherSessions.isEmpty { + if !model.otherSessions.isEmpty { ToolbarItem(placement: .primaryAction) { Menu { Button(role: .destructive) { @@ -58,29 +131,29 @@ public struct DevicesView: View { } label: { Image(systemName: "ellipsis.circle") } - .disabled(isRevoking) + .disabled(model.isRevoking) .accessibilityLabel("Device actions") } } } .task { - await reload() + await model.reload() } .alert( "Remove this device?", isPresented: Binding( - get: { revokeTarget != nil }, - set: { if !$0 { revokeTarget = nil } } + get: { model.revokeTarget != nil }, + set: { if !$0 { model.revokeTarget = nil } } ), - presenting: revokeTarget + presenting: model.revokeTarget ) { device in - Button(manager.managesServerSessions ? "Remove access" : "Remove device", role: .destructive) { - Task { await revoke(device) } + Button(model.manager.managesServerSessions ? "Remove access" : "Remove device", role: .destructive) { + Task { await model.revoke(device) } } Button("Cancel", role: .cancel) {} } message: { device in Text( - manager.managesServerSessions + model.manager.managesServerSessions ? "\(device.displayName) will need a new pairing code to reconnect." : "\(device.displayName) will stop receiving T3 Connect notifications." ) @@ -90,13 +163,13 @@ public struct DevicesView: View { isPresented: $showingRevokeOthers, titleVisibility: .visible ) { - Button("Remove \(otherSessions.count) devices", role: .destructive) { - Task { await revokeOthers() } + Button("Remove \(model.otherSessions.count) devices", role: .destructive) { + Task { await model.revokeOthers() } } Button("Cancel", role: .cancel) {} } message: { Text( - manager.managesServerSessions + model.manager.managesServerSessions ? "Every other phone, tablet, browser, and desktop will be signed out." : "Other registered devices will stop receiving T3 Connect notifications." ) @@ -105,7 +178,7 @@ public struct DevicesView: View { private var deviceList: some View { List { - if let currentSession { + if let currentSession = model.currentSession { Section { DeviceSessionRow(session: currentSession) } header: { @@ -113,22 +186,24 @@ public struct DevicesView: View { } } - if !otherSessions.isEmpty { + if !model.otherSessions.isEmpty { Section { - ForEach(otherSessions) { session in + ForEach(model.otherSessions) { session in DeviceSessionRow(session: session) .contentShape(Rectangle()) .swipeActions { Button("Remove", role: .destructive) { - revokeTarget = session + model.revokeTarget = session } + .disabled(model.isRevoking) } .contextMenu { Button(role: .destructive) { - revokeTarget = session + model.revokeTarget = session } label: { Label("Remove access", systemImage: "trash") } + .disabled(model.isRevoking) } } } header: { @@ -136,14 +211,14 @@ public struct DevicesView: View { } } - if let errorMessage { + if let errorMessage = model.errorMessage { Section { VStack(alignment: .leading, spacing: 10) { Label(errorMessage, systemImage: "exclamationmark.circle") .font(T3Typography.control) .foregroundStyle(T3Colors.warning) Button("Try again") { - Task { await reload() } + Task { await model.reload() } } .font(T3Typography.control.weight(.semibold)) } @@ -155,10 +230,10 @@ public struct DevicesView: View { .scrollContentBackground(.hidden) .background(T3Colors.background) .refreshable { - await reload() + await model.reload() } .overlay(alignment: .top) { - if isRevoking { + if model.isRevoking { Text("Updating device access") .font(T3Typography.supporting) .foregroundStyle(T3Colors.textTertiary) @@ -174,79 +249,6 @@ public struct DevicesView: View { .textCase(nil) .accessibilityAddTraits(.isHeader) } - - private var currentSession: FeatureDeviceSession? { - sessions.first(where: \.isCurrent) - } - - private var otherSessions: [FeatureDeviceSession] { - sessions.filter { !$0.isCurrent } - } - - @MainActor - private func reload() async { - guard !isRevoking else { return } - operationGeneration &+= 1 - let generation = operationGeneration - isLoading = true - defer { - if operationGeneration == generation { isLoading = false } - } - do { - let loaded = FeatureDeviceSession.sortedForDisplay( - try await manager.loadDeviceSessions() - ) - guard operationGeneration == generation else { return } - sessions = loaded - errorMessage = nil - } catch { - guard operationGeneration == generation else { return } - errorMessage = DeviceManagementErrorCopy.message(for: error) - } - } - - @MainActor - private func revoke(_ session: FeatureDeviceSession) async { - operationGeneration &+= 1 - let generation = operationGeneration - isLoading = false - isRevoking = true - defer { - if operationGeneration == generation { - isRevoking = false - revokeTarget = nil - } - } - do { - try await manager.revokeDeviceSession(id: session.id) - guard operationGeneration == generation else { return } - sessions.removeAll { $0.id == session.id } - errorMessage = nil - } catch { - guard operationGeneration == generation else { return } - errorMessage = DeviceManagementErrorCopy.message(for: error) - } - } - - @MainActor - private func revokeOthers() async { - operationGeneration &+= 1 - let generation = operationGeneration - isLoading = false - isRevoking = true - defer { - if operationGeneration == generation { isRevoking = false } - } - do { - try await manager.revokeOtherDeviceSessions() - guard operationGeneration == generation else { return } - sessions.removeAll { !$0.isCurrent } - errorMessage = nil - } catch { - guard operationGeneration == generation else { return } - errorMessage = DeviceManagementErrorCopy.message(for: error) - } - } } private struct DeviceSessionRow: View { diff --git a/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift b/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift index a8811b44399f5..93fe1c11be8f2 100644 --- a/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift +++ b/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift @@ -82,6 +82,108 @@ struct DeviceManagementTests { #expect(manager.loadCount == 1) } + @Test @MainActor + func staleReloadCannotRestoreRevokedDevice() async { + let current = session(id: "current", at: .now, isCurrent: true) + let laptop = session(id: "laptop", at: .now) + let manager = HeldDeviceManager() + let model = DevicesModel(manager: manager) + await load([current, laptop], into: model, from: manager) + + let reload = Task { await model.reload() } + await manager.waitForLoad() + await model.revoke(laptop) + manager.finishLoad(.success([current, laptop])) + await reload.value + + #expect(manager.revokedIDs == ["laptop"]) + #expect(model.sessions.map(\.id) == ["current"]) + #expect(model.isLoading == false) + #expect(model.isRevoking == false) + } + + @Test @MainActor + func staleReloadKeepsNewerRevocationFeedback() async { + let current = session(id: "current", at: .now, isCurrent: true) + let laptop = session(id: "laptop", at: .now) + let manager = HeldDeviceManager() + manager.revokeError = URLError(.notConnectedToInternet) + let model = DevicesModel(manager: manager) + + let reload = Task { await model.reload() } + await manager.waitForLoad() + await model.revoke(laptop) + let feedback = model.errorMessage + manager.finishLoad(.success([current, laptop])) + await reload.value + + #expect(feedback != nil) + #expect(model.errorMessage == feedback) + } + + @Test @MainActor + func staleReloadFailureDoesNotReplaceRevocationSuccess() async { + let laptop = session(id: "laptop", at: .now) + let manager = HeldDeviceManager() + let model = DevicesModel(manager: manager) + + let reload = Task { await model.reload() } + await manager.waitForLoad() + await model.revoke(laptop) + manager.finishLoad(.failure(URLError(.timedOut))) + await reload.value + + #expect(model.errorMessage == nil) + } + + @Test @MainActor + func currentReloadStillAppliesAfterRevocation() async { + let current = session(id: "current", at: .now, isCurrent: true) + let laptop = session(id: "laptop", at: .now) + let manager = HeldDeviceManager() + let model = DevicesModel(manager: manager) + + await model.revoke(laptop) + let reload = Task { await model.reload() } + await manager.waitForLoad() + manager.finishLoad(.success([current])) + await reload.value + + #expect(model.sessions.map(\.id) == ["current"]) + #expect(model.isLoading == false) + } + + @Test @MainActor + func secondRevocationWaitsForFirstAndKeepsItsFeedback() async { + let laptop = session(id: "laptop", at: .now) + let tablet = session(id: "tablet", at: .now) + let manager = HeldDeviceManager() + manager.holdsNextRevocation = true + let model = DevicesModel(manager: manager) + + let first = Task { await model.revoke(laptop) } + await manager.revocations.waitForCall() + await model.revoke(tablet) + manager.revocations.finish(.failure(URLError(.notConnectedToInternet))) + await first.value + + #expect(manager.revokedIDs == ["laptop"]) + #expect(model.errorMessage != nil) + #expect(model.isRevoking == false) + } + + @MainActor + private func load( + _ sessions: [FeatureDeviceSession], + into model: DevicesModel, + from manager: HeldDeviceManager + ) async { + let reload = Task { await model.reload() } + await manager.waitForLoad() + manager.finishLoad(.success(sessions)) + await reload.value + } + private func relayDevice(id: String) -> T3ConnectRelayDevice { T3ConnectRelayDevice( deviceId: id, @@ -139,3 +241,63 @@ private final class T3ConnectDeviceManagerStub: T3ConnectDeviceManaging { func unregisterDevice(id: String) async throws {} } + +/// Holds each device list read, and optionally the next revocation, open +/// until the test releases it. +@MainActor +private final class HeldDeviceManager: FeatureDeviceManaging { + var revokeError: Error? + var holdsNextRevocation = false + private(set) var revokedIDs: [String] = [] + let loads = HeldCalls<[FeatureDeviceSession]>() + let revocations = HeldCalls() + + func loadDeviceSessions() async throws -> [FeatureDeviceSession] { + try await loads.call() + } + + func waitForLoad() async { + await loads.waitForCall() + } + + func finishLoad(_ result: Result<[FeatureDeviceSession], Error>) { + loads.finish(result) + } + + func revokeDeviceSession(id: String) async throws { + revokedIDs.append(id) + if holdsNextRevocation { + holdsNextRevocation = false + try await revocations.call() + } + if let revokeError { throw revokeError } + } + + func revokeOtherDeviceSessions() async throws { + if let revokeError { throw revokeError } + } +} + +@MainActor +private final class HeldCalls { + private var pending: [CheckedContinuation] = [] + private var waiters: [CheckedContinuation] = [] + + func call() async throws -> Value { + try await withCheckedThrowingContinuation { continuation in + pending.append(continuation) + let ready = waiters + waiters = [] + ready.forEach { $0.resume() } + } + } + + func waitForCall() async { + guard pending.isEmpty else { return } + await withCheckedContinuation { waiters.append($0) } + } + + func finish(_ result: Result) { + pending.removeFirst().resume(with: result) + } +} From d1c473b8c44f1373df9169a398bfb0f58e48829a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 2 Oct 2026 09:45:52 +1000 Subject: [PATCH 3/3] test(swift-ios): keep held device test results sendable The held-call helper resumed continuations with a non-Sendable result, which CI reported as a data-race warning and Swift 6 mode rejects. Co-Authored-By: Claude Opus 5.5 (1M context) --- apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift b/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift index 93fe1c11be8f2..bbcd6b285b0c1 100644 --- a/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift +++ b/apps/swift-ios/Tests/FeatureTests/DeviceManagementTests.swift @@ -279,7 +279,7 @@ private final class HeldDeviceManager: FeatureDeviceManaging { } @MainActor -private final class HeldCalls { +private final class HeldCalls { private var pending: [CheckedContinuation] = [] private var waiters: [CheckedContinuation] = []