From 27a897e7cebfca9d63b6324a8dbc60f2ed0f8b7c Mon Sep 17 00:00:00 2001 From: Alex Southwell Date: Tue, 18 Aug 2026 09:02:20 +1000 Subject: [PATCH 1/3] feat(swift-ios): keep failed source control output visible with accessible Retry Recoverable source-control failures were invisible whenever a repository status had already loaded: the error string only ever reached the ContentUnavailableView fallback, so a failed commit, push, pull, or pull request silently did nothing. There was also no way to retry the exact failed operation and no accessibility focus handling. Retain the failure content in a banner that survives its own retry, keep a stable "Retry " accessibility label, replay the failed operation with its commit message, move VoiceOver focus to the retained failure and back to the refreshed repository status on recovery, and stop treating cancellation as a failure. --- .../Features/Shared/FeatureToolRecovery.swift | 167 +++++++++++++++ .../FeatureSourceControlView.swift | 163 ++++++++++---- .../FeatureToolRecoveryTests.swift | 198 ++++++++++++++++++ 3 files changed, 484 insertions(+), 44 deletions(-) create mode 100644 apps/swift-ios/Features/Shared/FeatureToolRecovery.swift create mode 100644 apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift diff --git a/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift b/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift new file mode 100644 index 000000000000..4c2093aed39e --- /dev/null +++ b/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift @@ -0,0 +1,167 @@ +import Foundation + +/// An operation on a tool surface that can fail recoverably and be retried unchanged. +public protocol FeatureRecoverableOperation: Equatable, Sendable { + /// Headline shown on the retained failure banner, e.g. "Push failed". + var failureTitle: String { get } + /// Stable accessibility label for the Retry control, e.g. "Retry push". + var retryAccessibilityLabel: String { get } + /// Spoken confirmation once the same operation succeeds. + var recoveryAnnouncement: String { get } +} + +/// Failure content retained for a tool surface. It survives the retry it triggers so the +/// useful output is never blanked while recovery is in flight. +public struct FeatureToolFailure: Identifiable, Sendable, Equatable, Hashable { + /// Distinct per presented failure so a repeat failure can move accessibility focus again. + public let id: Int + public var title: String + public var message: String + public var retryAccessibilityLabel: String + public var isRetrying: Bool + + public init( + id: Int, + title: String, + message: String, + retryAccessibilityLabel: String, + isRetrying: Bool = false + ) { + self.id = id + self.title = title + self.message = message + self.retryAccessibilityLabel = retryAccessibilityLabel + self.isRetrying = isRetrying + } + + /// Single spoken string so VoiceOver reads the retained content when focus lands on it. + public var accessibilityLabel: String { + isRetrying ? "\(title). \(message). Retrying." : "\(title). \(message)" + } +} + +/// Where accessibility focus belongs after a tool surface changes recovery state. +public enum FeatureToolRecoveryFocus: Hashable, Sendable { + /// The retained failure summary, read together with its content. + case failure + /// The first element of the recovered content. + case recoveredContent +} + +/// Recovery state for one tool surface: keeps failure content visible across retries, refuses +/// to report cancellation as a failure, and names a predictable accessibility focus target. +public struct FeatureToolFailureState: Sendable, Equatable { + public private(set) var failure: FeatureToolFailure? + /// The exact operation to run again, including any input the failed attempt carried. + public private(set) var retryOperation: Operation? + /// Set once when an operation recovers, so the surface can announce it exactly once. + public private(set) var recoveryAnnouncement: String? + private var presentedFailureCount = 0 + + public init() {} + + /// Marks an attempt as started. Retrying the failed operation keeps its content on screen + /// instead of blanking it; unrelated work leaves the retained failure untouched. + public mutating func begin(_ operation: Operation) { + recoveryAnnouncement = nil + guard failure != nil else { return } + failure?.isRetrying = retryOperation == operation + } + + /// Records the outcome of a failed attempt. Cancellation is not a failure: it never creates + /// one and never overwrites content already on screen. + public mutating func recordFailure(_ operation: Operation, error: Error) { + guard !Self.isCancellation(error) else { + failure?.isRetrying = false + return + } + presentedFailureCount += 1 + failure = FeatureToolFailure( + id: presentedFailureCount, + title: operation.failureTitle, + message: Self.message(for: error), + retryAccessibilityLabel: operation.retryAccessibilityLabel + ) + retryOperation = operation + } + + /// Records a successful attempt. The recovered content replaces the failure rather than + /// being stacked underneath it. + public mutating func recordSuccess(_ operation: Operation) { + let hadFailure = failure != nil + failure = nil + retryOperation = nil + recoveryAnnouncement = hadFailure ? operation.recoveryAnnouncement : nil + } + + /// Consumes the pending announcement so recovery is never spoken twice. + public mutating func takeRecoveryAnnouncement() -> String? { + defer { recoveryAnnouncement = nil } + return recoveryAnnouncement + } + + public var focusTarget: FeatureToolRecoveryFocus { + failure == nil ? .recoveredContent : .failure + } + + /// A dismissed sheet, a cancelled refresh, or a superseded request must not read as a failure. + public static func isCancellation(_ error: Error) -> Bool { + if error is CancellationError { return true } + let nsError = error as NSError + if nsError.domain == NSURLErrorDomain, nsError.code == NSURLErrorCancelled { return true } + if nsError.domain == NSCocoaErrorDomain, nsError.code == NSUserCancelledError { return true } + return false + } + + private static func message(for error: Error) -> String { + let described = error.localizedDescription + .trimmingCharacters(in: .whitespacesAndNewlines) + return described.isEmpty ? "The operation could not be completed." : described + } +} + +/// The retryable work of the source control surface. `action` carries the commit message so a +/// retry never asks for it again. +public enum FeatureSourceControlOperation: FeatureRecoverableOperation { + case load + case action(FeatureSourceControlAction, message: String?) + + public var isLoad: Bool { + if case .load = self { return true } + return false + } + + public var failureTitle: String { + switch self { + case .load: "Repository status failed to load" + case .action(let action, _): "\(action.title) failed" + } + } + + public var retryAccessibilityLabel: String { + switch self { + case .load: "Retry loading repository status" + case .action(let action, _): "Retry \(action.title.lowercased())" + } + } + + public var recoveryAnnouncement: String { + switch self { + case .load: "Repository status loaded." + case .action(let action, _): "\(action.title) succeeded. Repository status updated." + } + } +} + +public extension FeatureSourceControlAction { + var title: String { + switch self { + case .commit: "Commit changes" + case .push: "Push" + case .pull: "Pull latest" + case .createPullRequest: "Create pull request" + case .commitAndPush: "Commit and push" + case .commitPushAndCreatePullRequest: "Commit, push, and create PR" + } + } +} diff --git a/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift b/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift index ea26b9fcd97b..cb6aec97800b 100644 --- a/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift +++ b/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift @@ -7,9 +7,10 @@ public struct FeatureSourceControlView: View { @State private var status: FeatureSourceControlStatus? @State private var isLoading = true @State private var isRunningAction = false - @State private var errorMessage: String? + @State private var recovery = FeatureToolFailureState() @State private var commitMessage = "" @State private var pendingCommitAction: FeatureSourceControlAction? + @AccessibilityFocusState private var recoveryFocus: FeatureToolRecoveryFocus? public init(client: any FeatureClient, threadID: String) { self.client = client @@ -17,24 +18,29 @@ public struct FeatureSourceControlView: View { } public var body: some View { - Group { - if isLoading, status == nil { - ProgressView("Loading repository…") - .frame(maxWidth: .infinity, maxHeight: .infinity) - } else if let status, status.isRepository { - statusList(status) - } else { - ContentUnavailableView( - "Source control unavailable", - systemImage: "arrow.triangle.branch", - description: Text( - errorMessage - ?? (status?.isRepository == false + VStack(spacing: 0) { + if let failure = recovery.failure { + failureBanner(failure) + } + Group { + if isLoading, status == nil { + ProgressView("Loading repository…") + .frame(maxWidth: .infinity, maxHeight: .infinity) + } else if let status, status.isRepository { + statusList(status) + } else { + ContentUnavailableView( + "Source control unavailable", + systemImage: "arrow.triangle.branch", + description: Text( + status?.isRepository == false ? "This workspace is not a Git repository." - : "Repository status could not be loaded.") + : "Repository status could not be loaded." + ) ) - ) + } } + .frame(maxWidth: .infinity, maxHeight: .infinity) } .background(T3Colors.background) .navigationTitle("Source Control") @@ -60,13 +66,76 @@ public struct FeatureSourceControlView: View { } .disabled(commitMessage.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty) } + .onChange(of: recovery.failure?.id) { _, failureID in + guard failureID != nil else { return } + recoveryFocus = .failure + } + .onChange(of: recovery.recoveryAnnouncement) { _, _ in + guard let announcement = recovery.takeRecoveryAnnouncement() else { return } + recoveryFocus = .recoveredContent + AccessibilityNotification.Announcement(announcement).post() + } .task { await load() } } + /// Keeps the failed output on screen — including while its retry runs — with a labelled + /// Retry control immediately after it in the accessibility order. + private func failureBanner(_ failure: FeatureToolFailure) -> some View { + VStack(alignment: .leading, spacing: 10) { + VStack(alignment: .leading, spacing: 6) { + Label(failure.title, systemImage: "exclamationmark.triangle.fill") + .font(T3Typography.supportingStrong) + .foregroundStyle(T3Colors.danger) + Text(failure.message) + .font(T3Typography.tool) + .foregroundStyle(T3Colors.textSecondary) + .textSelection(.enabled) + .fixedSize(horizontal: false, vertical: true) + .frame(maxWidth: .infinity, alignment: .leading) + } + .accessibilityElement(children: .combine) + .accessibilityLabel(failure.accessibilityLabel) + .accessibilityIdentifier("source-control-failure") + .accessibilityFocused($recoveryFocus, equals: .failure) + + HStack(spacing: 10) { + Button { + guard let operation = recovery.retryOperation else { return } + Task { await run(operation) } + } label: { + Label("Retry", systemImage: "arrow.clockwise") + .font(T3Typography.control) + .frame(minHeight: T3Metrics.minimumTapTarget) + } + .buttonStyle(.borderedProminent) + .disabled(failure.isRetrying) + .accessibilityLabel(failure.retryAccessibilityLabel) + .accessibilityIdentifier("source-control-failure-retry") + + if failure.isRetrying { + ProgressView() + Text("Retrying…") + .font(T3Typography.supporting) + .foregroundStyle(T3Colors.textSecondary) + } + } + } + .padding(14) + .frame(maxWidth: .infinity, alignment: .leading) + .background(T3Colors.surfaceRaised, in: RoundedRectangle(cornerRadius: 12)) + .overlay { + RoundedRectangle(cornerRadius: 12) + .stroke(T3Colors.danger.opacity(0.4), lineWidth: 1) + } + .padding(.horizontal, 16) + .padding(.top, 12) + } + private func statusList(_ status: FeatureSourceControlStatus) -> some View { List { Section("Repository") { LabeledContent("Branch", value: status.branch ?? "Detached HEAD") + .accessibilityFocused($recoveryFocus, equals: .recoveredContent) if let upstream = status.upstream { LabeledContent("Upstream", value: upstream) } @@ -151,28 +220,45 @@ public struct FeatureSourceControlView: View { } private func load() async { - isLoading = true - defer { isLoading = false } - do { - status = try await client.sourceControlStatus(threadID: threadID) - errorMessage = nil - } catch { - errorMessage = error.localizedDescription - } + await run(.load) } private func perform(_ action: FeatureSourceControlAction, message: String?) async { - isRunningAction = true - defer { isRunningAction = false } + await run( + .action(action, message: message?.trimmingCharacters(in: .whitespacesAndNewlines)) + ) + } + + /// Single entry point for every source control request, so a retry replays the exact failed + /// operation — commit message included — instead of falling back to a plain reload. + private func run(_ operation: FeatureSourceControlOperation) async { + recovery.begin(operation) + if operation.isLoad { + isLoading = true + } else { + isRunningAction = true + } + defer { + if operation.isLoad { + isLoading = false + } else { + isRunningAction = false + } + } do { - status = try await client.performSourceControlAction( - threadID: threadID, - action: action, - message: message?.trimmingCharacters(in: .whitespacesAndNewlines) - ) - errorMessage = nil + switch operation { + case .load: + status = try await client.sourceControlStatus(threadID: threadID) + case .action(let action, let message): + status = try await client.performSourceControlAction( + threadID: threadID, + action: action, + message: message + ) + } + recovery.recordSuccess(operation) } catch { - errorMessage = error.localizedDescription + recovery.recordFailure(operation, error: error) } } } @@ -185,17 +271,6 @@ private extension FeatureSourceControlAction { } } - var title: String { - switch self { - case .commit: "Commit changes" - case .push: "Push" - case .pull: "Pull latest" - case .createPullRequest: "Create pull request" - case .commitAndPush: "Commit and push" - case .commitPushAndCreatePullRequest: "Commit, push, and create PR" - } - } - var icon: String { switch self { case .commit: "checkmark.circle" diff --git a/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift b/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift new file mode 100644 index 000000000000..cb2954e3ddff --- /dev/null +++ b/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift @@ -0,0 +1,198 @@ +import Foundation +import Testing +@testable import T3Code + +@Suite("Tool failure recovery") +struct FeatureToolRecoveryTests { + private struct StubError: LocalizedError { + let message: String + var errorDescription: String? { message } + } + + private func failedState( + _ operation: FeatureSourceControlOperation = .action(.push, message: nil), + message: String = "remote rejected: non-fast-forward" + ) -> FeatureToolFailureState { + var state = FeatureToolFailureState() + state.begin(operation) + state.recordFailure(operation, error: StubError(message: message)) + return state + } + + @Test + func failureRetainsContentAndNamesTheOperation() { + let state = failedState() + + #expect(state.failure?.title == "Push failed") + #expect(state.failure?.message == "remote rejected: non-fast-forward") + #expect(state.failure?.isRetrying == false) + #expect(state.retryOperation == .action(.push, message: nil)) + #expect(state.focusTarget == .failure) + } + + @Test + func retryKeepsFailureContentVisibleWhileItRuns() { + var state = failedState() + + state.begin(.action(.push, message: nil)) + + #expect(state.failure?.message == "remote rejected: non-fast-forward") + #expect(state.failure?.isRetrying == true) + #expect(state.focusTarget == .failure) + #expect(state.failure?.accessibilityLabel.hasSuffix("Retrying.") == true) + } + + @Test + func unrelatedWorkDoesNotMarkTheRetainedFailureAsRetrying() { + var state = failedState() + + state.begin(.load) + + #expect(state.failure?.message == "remote rejected: non-fast-forward") + #expect(state.failure?.isRetrying == false) + } + + @Test + func repeatedFailureUpdatesContentAndPresentsANewFocusIdentity() { + var state = failedState() + let firstID = state.failure?.id + + state.begin(.action(.push, message: nil)) + state.recordFailure( + .action(.push, message: nil), + error: StubError(message: "remote rejected: still behind") + ) + + #expect(state.failure?.message == "remote rejected: still behind") + #expect(state.failure?.isRetrying == false) + #expect(state.failure?.id != firstID) + } + + @Test + func cancellationNeverCreatesAFailure() { + var state = FeatureToolFailureState() + state.begin(.load) + + state.recordFailure(.load, error: CancellationError()) + + #expect(state.failure == nil) + #expect(state.retryOperation == nil) + #expect(state.focusTarget == .recoveredContent) + } + + @Test + func cancellingARetryPreservesTheOriginalFailureContent() { + var state = failedState() + state.begin(.action(.push, message: nil)) + + state.recordFailure( + .action(.push, message: nil), + error: URLError(.cancelled) + ) + + #expect(state.failure?.message == "remote rejected: non-fast-forward") + #expect(state.failure?.isRetrying == false) + #expect(state.retryOperation == .action(.push, message: nil)) + } + + @Test + func cancellationIsRecognizedAcrossTheErrorsAThreadDismissalProduces() { + typealias State = FeatureToolFailureState + + #expect(State.isCancellation(CancellationError())) + #expect(State.isCancellation(URLError(.cancelled))) + #expect(State.isCancellation(CocoaError(.userCancelled))) + #expect(State.isCancellation(URLError(.timedOut)) == false) + #expect(State.isCancellation(StubError(message: "boom")) == false) + } + + @Test + func recoveryClearsTheFailureAndAnnouncesItOnce() { + var state = failedState() + + state.begin(.action(.push, message: nil)) + state.recordSuccess(.action(.push, message: nil)) + + #expect(state.failure == nil) + #expect(state.retryOperation == nil) + #expect(state.focusTarget == .recoveredContent) + #expect(state.takeRecoveryAnnouncement() == "Push succeeded. Repository status updated.") + #expect(state.takeRecoveryAnnouncement() == nil) + } + + @Test + func successWithoutAPriorFailureAnnouncesNothing() { + var state = FeatureToolFailureState() + + state.begin(.load) + state.recordSuccess(.load) + + #expect(state.takeRecoveryAnnouncement() == nil) + #expect(state.focusTarget == .recoveredContent) + } + + @Test + func startingAnotherAttemptDropsAStaleRecoveryAnnouncement() { + var state = failedState(.load) + state.recordSuccess(.load) + + state.begin(.action(.pull, message: nil)) + + #expect(state.recoveryAnnouncement == nil) + } + + @Test + func retryReplaysTheExactFailedOperationIncludingItsCommitMessage() { + let operation = FeatureSourceControlOperation.action(.commit, message: "fix: retry me") + var state = FeatureToolFailureState() + + state.begin(operation) + state.recordFailure(operation, error: StubError(message: "pre-commit hook failed")) + + #expect(state.retryOperation == operation) + #expect(state.failure?.retryAccessibilityLabel == "Retry commit changes") + } + + @Test + func retryLabelIsStableAcrossRepeatedFailuresOfTheSameOperation() { + var state = failedState() + let firstLabel = state.failure?.retryAccessibilityLabel + + state.begin(.action(.push, message: nil)) + state.recordFailure(.action(.push, message: nil), error: StubError(message: "again")) + + #expect(firstLabel == "Retry push") + #expect(state.failure?.retryAccessibilityLabel == firstLabel) + } + + @Test + func everySourceControlOperationHasDistinctFailureAndRetryWording() { + let operations: [FeatureSourceControlOperation] = [.load] + + FeatureSourceControlAction.allCases.map { .action($0, message: nil) } + + let failureTitles = operations.map(\.failureTitle) + let retryLabels = operations.map(\.retryAccessibilityLabel) + + #expect(Set(failureTitles).count == operations.count) + #expect(Set(retryLabels).count == operations.count) + #expect(retryLabels.allSatisfy { $0.hasPrefix("Retry ") }) + #expect(failureTitles.contains("Repository status failed to load")) + #expect(retryLabels.contains("Retry loading repository status")) + } + + @Test + func emptyErrorTextStillLeavesReadableFailureContent() { + var state = FeatureToolFailureState() + + state.recordFailure(.load, error: StubError(message: " ")) + + #expect(state.failure?.message == "The operation could not be completed.") + #expect(state.failure?.accessibilityLabel.isEmpty == false) + } + + @Test + func loadOperationIsDistinguishedFromActions() { + #expect(FeatureSourceControlOperation.load.isLoad) + #expect(FeatureSourceControlOperation.action(.pull, message: nil).isLoad == false) + } +} From 42696d5847ae9625d671f02de9ca3bbf8b61f30a Mon Sep 17 00:00:00 2001 From: Alex Southwell Date: Fri, 21 Aug 2026 11:57:05 +1000 Subject: [PATCH 2/3] fix(swift-ios): avoid repeating completed source control actions --- apps/swift-ios/App/NativeFeatureClient.swift | 6 +- apps/swift-ios/DesignSystem/T3Theme.swift | 1 + .../Features/Shared/FeatureClient.swift | 6 +- .../Features/Shared/FeatureToolRecovery.swift | 47 ++++++++++++- .../FeatureSourceControlView.swift | 43 ++++++++---- .../FeatureToolRecoveryTests.swift | 70 +++++++++++++++++++ 6 files changed, 152 insertions(+), 21 deletions(-) diff --git a/apps/swift-ios/App/NativeFeatureClient.swift b/apps/swift-ios/App/NativeFeatureClient.swift index 71d0ab263a1f..b70045377640 100644 --- a/apps/swift-ios/App/NativeFeatureClient.swift +++ b/apps/swift-ios/App/NativeFeatureClient.swift @@ -2063,7 +2063,7 @@ final class NativeFeatureClient: FeatureClient, FeatureDeviceManaging, threadID: String, action: FeatureSourceControlAction, message: String? - ) async throws -> FeatureSourceControlStatus { + ) async throws { let route = try threadRoute(for: threadID) let client = route.client let context = try workspaceContext(route: route) @@ -2082,10 +2082,6 @@ final class NativeFeatureClient: FeatureClient, FeatureDeviceManaging, } } } - - return NativeWorkspaceMapper.sourceControl( - try await client.refreshVCSStatus(cwd: context.cwd) - ) } func terminalSnapshot( diff --git a/apps/swift-ios/DesignSystem/T3Theme.swift b/apps/swift-ios/DesignSystem/T3Theme.swift index ed4380ca9829..79939aef05f3 100644 --- a/apps/swift-ios/DesignSystem/T3Theme.swift +++ b/apps/swift-ios/DesignSystem/T3Theme.swift @@ -94,6 +94,7 @@ enum T3Typography { enum T3Metrics { static let minimumTapTarget: CGFloat = 44 + static let maximumToolFailureMessageHeight: CGFloat = 144 static let sidebarWidth: CGFloat = 320 static let minimumSidebarWidth: CGFloat = 280 static let maximumSidebarWidth: CGFloat = 380 diff --git a/apps/swift-ios/Features/Shared/FeatureClient.swift b/apps/swift-ios/Features/Shared/FeatureClient.swift index 09b6fd5427f9..da3834c4d071 100644 --- a/apps/swift-ios/Features/Shared/FeatureClient.swift +++ b/apps/swift-ios/Features/Shared/FeatureClient.swift @@ -166,11 +166,13 @@ public protocol FeatureClient: AnyObject { ) async throws -> FeatureReviewFileContents? func sourceControlStatus(threadID: String) async throws -> FeatureSourceControlStatus + /// Completes at the mutation boundary. Callers refresh status separately so a refresh + /// failure cannot make an already-completed non-idempotent action retryable. func performSourceControlAction( threadID: String, action: FeatureSourceControlAction, message: String? - ) async throws -> FeatureSourceControlStatus + ) async throws func terminalSnapshot(threadID: String, terminalID: String) async throws -> FeatureTerminalSnapshot func terminalEvents(threadID: String, terminalID: String) -> AsyncStream @@ -453,7 +455,7 @@ public extension FeatureClient { threadID: String, action: FeatureSourceControlAction, message: String? - ) async throws -> FeatureSourceControlStatus { + ) async throws { throw FeatureCapabilityUnavailable("Source control actions") } diff --git a/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift b/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift index 4c2093aed39e..d04fd970c9ed 100644 --- a/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift +++ b/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift @@ -88,10 +88,30 @@ public struct FeatureToolFailureState: S /// Records a successful attempt. The recovered content replaces the failure rather than /// being stacked underneath it. public mutating func recordSuccess(_ operation: Operation) { - let hadFailure = failure != nil + guard failure != nil, retryOperation == operation else { + recoveryAnnouncement = nil + return + } + failure = nil + retryOperation = nil + recoveryAnnouncement = operation.recoveryAnnouncement + } + + /// Records a follow-up failure after an operation already completed. A real failure becomes + /// the only retryable operation; cancellation stays silent and removes the completed work. + public mutating func recordFollowUpFailure( + _ followUpOperation: Operation, + afterCompletionOf completedOperation: Operation, + error: Error + ) { + guard Self.isCancellation(error) else { + recordFailure(followUpOperation, error: error) + return + } + guard retryOperation == completedOperation else { return } failure = nil retryOperation = nil - recoveryAnnouncement = hadFailure ? operation.recoveryAnnouncement : nil + recoveryAnnouncement = nil } /// Consumes the pending announcement so recovery is never spoken twice. @@ -120,6 +140,29 @@ public struct FeatureToolFailureState: S } } +/// Single-flight ownership for a tool surface. A second request cannot mutate recovery state +/// while the operation that acquired the surface is still running. +public struct FeatureToolRunState: Sendable, Equatable { + public private(set) var operation: Operation? + + public init() {} + + public var isBusy: Bool { + operation != nil + } + + public mutating func begin(_ operation: Operation) -> Bool { + guard self.operation == nil else { return false } + self.operation = operation + return true + } + + public mutating func finish(_ operation: Operation) { + guard self.operation == operation else { return } + self.operation = nil + } +} + /// The retryable work of the source control surface. `action` carries the commit message so a /// retry never asks for it again. public enum FeatureSourceControlOperation: FeatureRecoverableOperation { diff --git a/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift b/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift index cb6aec97800b..0262362a05e0 100644 --- a/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift +++ b/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift @@ -7,6 +7,7 @@ public struct FeatureSourceControlView: View { @State private var status: FeatureSourceControlStatus? @State private var isLoading = true @State private var isRunningAction = false + @State private var runState = FeatureToolRunState() @State private var recovery = FeatureToolFailureState() @State private var commitMessage = "" @State private var pendingCommitAction: FeatureSourceControlAction? @@ -48,7 +49,7 @@ public struct FeatureSourceControlView: View { .toolbar { ToolbarItem(placement: .topBarTrailing) { Button { Task { await load() } } label: { Image(systemName: "arrow.clockwise") } - .disabled(isLoading || isRunningAction) + .disabled(isLoading || runState.isBusy) .accessibilityLabel("Reload source control") } } @@ -64,7 +65,10 @@ public struct FeatureSourceControlView: View { } pendingCommitAction = nil } - .disabled(commitMessage.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty) + .disabled( + commitMessage.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty + || runState.isBusy + ) } .onChange(of: recovery.failure?.id) { _, failureID in guard failureID != nil else { return } @@ -86,12 +90,15 @@ public struct FeatureSourceControlView: View { Label(failure.title, systemImage: "exclamationmark.triangle.fill") .font(T3Typography.supportingStrong) .foregroundStyle(T3Colors.danger) - Text(failure.message) - .font(T3Typography.tool) - .foregroundStyle(T3Colors.textSecondary) - .textSelection(.enabled) - .fixedSize(horizontal: false, vertical: true) - .frame(maxWidth: .infinity, alignment: .leading) + ScrollView { + Text(failure.message) + .font(T3Typography.tool) + .foregroundStyle(T3Colors.textSecondary) + .textSelection(.enabled) + .fixedSize(horizontal: false, vertical: true) + .frame(maxWidth: .infinity, alignment: .leading) + } + .frame(maxHeight: T3Metrics.maximumToolFailureMessageHeight) } .accessibilityElement(children: .combine) .accessibilityLabel(failure.accessibilityLabel) @@ -108,7 +115,7 @@ public struct FeatureSourceControlView: View { .frame(minHeight: T3Metrics.minimumTapTarget) } .buttonStyle(.borderedProminent) - .disabled(failure.isRetrying) + .disabled(failure.isRetrying || runState.isBusy) .accessibilityLabel(failure.retryAccessibilityLabel) .accessibilityIdentifier("source-control-failure-retry") @@ -169,7 +176,7 @@ public struct FeatureSourceControlView: View { Label(action.title, systemImage: action.icon) .frame(maxWidth: .infinity, alignment: .leading) } - .disabled(isRunningAction) + .disabled(runState.isBusy) } } @@ -232,6 +239,7 @@ public struct FeatureSourceControlView: View { /// Single entry point for every source control request, so a retry replays the exact failed /// operation — commit message included — instead of falling back to a plain reload. private func run(_ operation: FeatureSourceControlOperation) async { + guard runState.begin(operation) else { return } recovery.begin(operation) if operation.isLoad { isLoading = true @@ -239,6 +247,7 @@ public struct FeatureSourceControlView: View { isRunningAction = true } defer { + runState.finish(operation) if operation.isLoad { isLoading = false } else { @@ -249,14 +258,24 @@ public struct FeatureSourceControlView: View { switch operation { case .load: status = try await client.sourceControlStatus(threadID: threadID) + recovery.recordSuccess(operation) case .action(let action, let message): - status = try await client.performSourceControlAction( + try await client.performSourceControlAction( threadID: threadID, action: action, message: message ) + do { + status = try await client.sourceControlStatus(threadID: threadID) + recovery.recordSuccess(operation) + } catch { + recovery.recordFollowUpFailure( + .load, + afterCompletionOf: operation, + error: error + ) + } } - recovery.recordSuccess(operation) } catch { recovery.recordFailure(operation, error: error) } diff --git a/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift b/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift index cb2954e3ddff..8630ec278e18 100644 --- a/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift +++ b/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift @@ -52,6 +52,18 @@ struct FeatureToolRecoveryTests { #expect(state.failure?.isRetrying == false) } + @Test("An unrelated success preserves the failed operation", .bug(id: 3801994163)) + func unrelatedSuccessDoesNotConsumeTheRetainedFailure() { + var state = failedState() + + state.begin(.load) + state.recordSuccess(.load) + + #expect(state.failure?.message == "remote rejected: non-fast-forward") + #expect(state.retryOperation == .action(.push, message: nil)) + #expect(state.recoveryAnnouncement == nil) + } + @Test func repeatedFailureUpdatesContentAndPresentsANewFocusIdentity() { var state = failedState() @@ -153,6 +165,43 @@ struct FeatureToolRecoveryTests { #expect(state.failure?.retryAccessibilityLabel == "Retry commit changes") } + @Test("A post-action refresh failure retries only the refresh", .bug(id: 3801994206)) + func postActionRefreshFailureCannotRepeatTheCompletedAction() { + let completedAction = FeatureSourceControlOperation.action( + .commit, + message: "fix: do not run twice" + ) + var state = FeatureToolFailureState() + + state.begin(completedAction) + state.recordFollowUpFailure( + .load, + afterCompletionOf: completedAction, + error: StubError(message: "connection lost during refresh") + ) + + #expect(state.failure?.title == "Repository status failed to load") + #expect(state.retryOperation == .load) + #expect(state.retryOperation != completedAction) + } + + @Test("A cancelled post-action refresh cannot leave the action retryable") + func cancelledPostActionRefreshDropsTheCompletedActionFailure() { + let completedAction = FeatureSourceControlOperation.action(.push, message: nil) + var state = failedState(completedAction) + + state.begin(completedAction) + state.recordFollowUpFailure( + .load, + afterCompletionOf: completedAction, + error: CancellationError() + ) + + #expect(state.failure == nil) + #expect(state.retryOperation == nil) + #expect(state.recoveryAnnouncement == nil) + } + @Test func retryLabelIsStableAcrossRepeatedFailuresOfTheSameOperation() { var state = failedState() @@ -195,4 +244,25 @@ struct FeatureToolRecoveryTests { #expect(FeatureSourceControlOperation.load.isLoad) #expect(FeatureSourceControlOperation.action(.pull, message: nil).isLoad == false) } + + @Test("Only one source-control request can own the recovery state", .bug(id: 3802036872)) + func runStateRejectsOverlappingOperations() { + var state = FeatureToolRunState() + let action = FeatureSourceControlOperation.action(.push, message: nil) + + let actionDidBegin = state.begin(action) + let overlappingLoadDidBegin = state.begin(.load) + + #expect(actionDidBegin) + #expect(state.isBusy) + #expect(overlappingLoadDidBegin == false) + #expect(state.operation == action) + + state.finish(.load) + #expect(state.operation == action) + + state.finish(action) + #expect(state.isBusy == false) + #expect(state.operation == nil) + } } From 47389c712e0b658a74adb0300ab9dea7ef8f14c2 Mon Sep 17 00:00:00 2001 From: Alex Southwell Date: Fri, 21 Aug 2026 12:16:38 +1000 Subject: [PATCH 3/3] fix(swift-ios): clear recovered load failures --- .../Features/Shared/FeatureToolRecovery.swift | 14 +++++++++----- .../SourceControl/FeatureSourceControlView.swift | 2 +- .../FeatureTests/FeatureToolRecoveryTests.swift | 13 +++++++++++++ 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift b/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift index d04fd970c9ed..d1993e60b181 100644 --- a/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift +++ b/apps/swift-ios/Features/Shared/FeatureToolRecovery.swift @@ -85,16 +85,20 @@ public struct FeatureToolFailureState: S retryOperation = operation } - /// Records a successful attempt. The recovered content replaces the failure rather than - /// being stacked underneath it. - public mutating func recordSuccess(_ operation: Operation) { - guard failure != nil, retryOperation == operation else { + /// Records successful work. One completion can satisfy related retained operations, such as + /// an action whose explicit follow-up refresh also recovers an earlier load failure. + public mutating func recordSuccess(_ operations: Operation...) { + guard + failure != nil, + let recoveredOperation = retryOperation, + operations.contains(recoveredOperation) + else { recoveryAnnouncement = nil return } failure = nil retryOperation = nil - recoveryAnnouncement = operation.recoveryAnnouncement + recoveryAnnouncement = recoveredOperation.recoveryAnnouncement } /// Records a follow-up failure after an operation already completed. A real failure becomes diff --git a/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift b/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift index 0262362a05e0..d1501ce22f66 100644 --- a/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift +++ b/apps/swift-ios/Features/SourceControl/FeatureSourceControlView.swift @@ -267,7 +267,7 @@ public struct FeatureSourceControlView: View { ) do { status = try await client.sourceControlStatus(threadID: threadID) - recovery.recordSuccess(operation) + recovery.recordSuccess(operation, .load) } catch { recovery.recordFollowUpFailure( .load, diff --git a/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift b/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift index 8630ec278e18..cf7bef87d578 100644 --- a/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift +++ b/apps/swift-ios/Tests/FeatureTests/FeatureToolRecoveryTests.swift @@ -185,6 +185,19 @@ struct FeatureToolRecoveryTests { #expect(state.retryOperation != completedAction) } + @Test("A successful action refresh consumes a retained load failure", .bug(id: 3826749394)) + func actionRefreshSuccessRecoversAnEarlierLoadFailure() { + let action = FeatureSourceControlOperation.action(.push, message: nil) + var state = failedState(.load) + + state.begin(action) + state.recordSuccess(action, .load) + + #expect(state.failure == nil) + #expect(state.retryOperation == nil) + #expect(state.takeRecoveryAnnouncement() == "Repository status loaded.") + } + @Test("A cancelled post-action refresh cannot leave the action retryable") func cancelledPostActionRefreshDropsTheCompletedActionFailure() { let completedAction = FeatureSourceControlOperation.action(.push, message: nil)