Only auto-restore the previous Mac after a cancelled add-device attempt

Review follow-up: restoring via switchToMac begins a fresh pairing
attempt, which clears connectionError and erased the failure reason the
auto-presented pairing sheet was showing. Restore now requires the
attempt to have ended disconnected with no connection error, which is
exactly the store's cancellation path (every real failure sets one).
Failed attempts keep the error visible for an in-place retry;
reconnecting the previous Mac without losing the error needs a
store-owned error channel that survives reconnects (follow-up). Local
input validation in PairingView and the scanner's pairing-link filter
keep malformed input from reaching the destructive path at all.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: lawrencecchen

Packages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/DeviceTreeAddDevicePolicy.swift

@@ -35,18 +35,28 @@ public struct DeviceTreeAddDevicePolicy: Sendable {
     /// Whether a completed add-device attempt should reconnect the Mac that
     /// was live when the attempt started.
     ///
-    /// The underlying pairing path is destructive: a failed (or cancelled)
-    /// attempt leaves the shell `.disconnected` even when a healthy connection
-    /// existed before it began. When the attempt started over a live
-    /// connection (`previousMacDeviceID` non-nil) and ended disconnected,
-    /// restore that Mac via `switchToMac` instead of stranding the user —
-    /// the same "never strand the user" contract `switchToMac` documents for
-    /// its own failure path. A successful attempt is connected (to the new
-    /// device), so no restore happens.
+    /// The underlying pairing path is destructive: a cancelled attempt leaves
+    /// the shell `.disconnected` even when a healthy connection existed before
+    /// it began. When the attempt started over a live connection
+    /// (`previousMacDeviceID` non-nil), ended disconnected, and reported no
+    /// connection error (the store's cancellation path sets none, while every
+    /// real failure does), restore that Mac via `switchToMac` instead of
+    /// stranding the user — the same "never strand the user" contract
+    /// `switchToMac` documents for its own failure path.
+    ///
+    /// A failed attempt (`hasConnectionError`) deliberately does NOT restore:
+    /// the reconnect path begins a fresh pairing attempt, which clears
+    /// `connectionError` and would erase the actionable failure reason while
+    /// the user is reading it. The disconnected shell auto-presents the
+    /// pairing sheet with that error for an in-place retry; reconnecting the
+    /// previous Mac without losing the error needs a store-owned error
+    /// channel that survives reconnects (follow-up). A successful attempt is
+    /// connected (to the new device), so no restore happens either.
     public func restoresPreviousConnection(
         connectionState: MobileConnectionState,
-        previousMacDeviceID: String?
+        previousMacDeviceID: String?,
+        hasConnectionError: Bool
     ) -> Bool {
-        connectionState != .connected && previousMacDeviceID != nil
+        connectionState != .connected && previousMacDeviceID != nil && !hasConnectionError
     }
 }

Packages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/DeviceTreeAddDevicePolicyTests.swift

@@ -32,13 +32,27 @@ import Testing
         #expect(!policy.dismissesAfterPairingAttempt(connectionState: .disconnected))
     }
 
-    /// A failed or cancelled attempt that started over a live connection
+    /// A cancelled attempt (disconnected, no connection error — the store's
+    /// cancellation path sets none) that started over a live connection
     /// reconnects that Mac: the pairing path is destructive, and the user must
-    /// not lose a working session to a bad QR code or a cancelled attempt.
-    @Test func failedAttemptOverLiveConnectionRestoresIt() {
+    /// not lose a working session just by backing out of the sheet.
+    @Test func cancelledAttemptOverLiveConnectionRestoresIt() {
         #expect(policy.restoresPreviousConnection(
             connectionState: .disconnected,
-            previousMacDeviceID: "mac-1"
+            previousMacDeviceID: "mac-1",
+            hasConnectionError: false
+        ))
+    }
+
+    /// A failed attempt (connection error set) must NOT auto-restore: the
+    /// reconnect path begins a fresh pairing attempt, which clears
+    /// `connectionError` and would erase the failure reason while the user is
+    /// reading it on the auto-presented pairing sheet.
+    @Test func failedAttemptKeepsErrorInsteadOfRestoring() {
+        #expect(!policy.restoresPreviousConnection(
+            connectionState: .disconnected,
+            previousMacDeviceID: "mac-1",
+            hasConnectionError: true
         ))
     }
 
@@ -47,16 +61,18 @@ import Testing
     @Test func successfulAttemptDoesNotRestorePreviousMac() {
         #expect(!policy.restoresPreviousConnection(
             connectionState: .connected,
-            previousMacDeviceID: "mac-1"
+            previousMacDeviceID: "mac-1",
+            hasConnectionError: false
         ))
     }
 
     /// An attempt that started without a live connection (first pair from the
     /// tree's empty state) has nothing to restore.
-    @Test func failedAttemptWithNoPreviousMacDoesNotRestore() {
+    @Test func cancelledAttemptWithNoPreviousMacDoesNotRestore() {
         #expect(!policy.restoresPreviousConnection(
             connectionState: .disconnected,
-            previousMacDeviceID: nil
+            previousMacDeviceID: nil,
+            hasConnectionError: false
         ))
     }
 }

Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift

@@ -166,12 +166,16 @@ struct DeviceTreeView: View {
     ///
     /// Success (the shell is connected, to the added device): dismiss the
     /// sheet and refresh both device sources so the new device appears in the
-    /// tree. Failure or cancellation over what was a live connection: the
-    /// pairing path has already torn that connection down, so reconnect the
-    /// previous Mac via ``CMUXMobileShellStore/switchToMac(macDeviceID:)``
-    /// instead of stranding the user disconnected. The restore runs in a
-    /// fresh unstructured `Task` because a cancelled attempt's continuation
-    /// executes in an already-cancelled task.
+    /// tree. Cancellation over what was a live connection (disconnected, no
+    /// connection error): the pairing path has already torn that connection
+    /// down, so reconnect the previous Mac via
+    /// ``CMUXMobileShellStore/switchToMac(macDeviceID:)`` instead of
+    /// stranding the user. The restore runs in a fresh unstructured `Task`
+    /// because a cancelled attempt's continuation executes in an
+    /// already-cancelled task. A real failure (connection error set) does not
+    /// restore — the reconnect would clear the error the user needs to read;
+    /// the disconnected shell auto-presents the pairing sheet with that error
+    /// for an in-place retry (see the policy's rationale).
     private func finishAddDevice(previousMacDeviceID: String?) {
         let state = store.connectionState
         if addDevicePolicy.dismissesAfterPairingAttempt(connectionState: state) {
@@ -184,7 +188,8 @@ struct DeviceTreeView: View {
         }
         if addDevicePolicy.restoresPreviousConnection(
             connectionState: state,
-            previousMacDeviceID: previousMacDeviceID
+            previousMacDeviceID: previousMacDeviceID,
+            hasConnectionError: store.connectionError != nil
         ), let previousMacDeviceID {
             let store = store
             Task {