iOS Devices sheet: always-available Add device entry that re-enters the pairing flow#6017
iOS Devices sheet: always-available Add device entry that re-enters the pairing flow#6017lawrencecchen wants to merge 5 commits into
Conversation
The Devices sheet previously only listed devices; once paired, the add-device pairing flow was unreachable except by digging into Settings -> Switch Mac (scanner only) or losing the connection. Add a shared beginAddDevice action exposed through two entry points (a list row and a toolbar plus) that re-enters the full first-run PairingView flow (QR scan or manual host) as a sheet over the tree. Cancel and dismissal semantics live in a pure, unit-tested DeviceTreeAddDevicePolicy in CmuxMobileShellModel: cancelling over a live connection must not call cancelPairing() (which would tear the connection down), and the sheet dismisses only when the attempt left the shell connected, refreshing the tree so the new device appears. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds DeviceTreeAddDevicePolicy (three predicate methods), unit tests for its cancel/dismiss/restore semantics, and an always-available add-device sheet in DeviceTreeView that uses the policy to decide cancel, dismiss, refresh, or restore flows. ChangesDevice pairing policy and tests
Device tree add-device flow integration
sequenceDiagram
participant DeviceTreeView
participant PairingView
participant ShellStore
participant Registry
DeviceTreeView->>PairingView: present sheet / beginAddDevice()
PairingView->>ShellStore: connect / cancel callbacks
ShellStore->>DeviceTreeView: report connectionState, previousMacDeviceID
DeviceTreeView->>ShellStore: finishAddDevice(previousMacDeviceID)
ShellStore->>Registry: refreshPairedMacs() & refreshRegistryDevices()
ShellStore->>ShellStore: switchToMac(macDeviceID) (restore path)
🎯 3 (Moderate) | ⏱️ ~20 minutes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (20 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bed9e483e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| finishAddDeviceIfConnected() | ||
| }, | ||
| connectManualHost: { name, host, port in | ||
| await store.connectManualHost(name: name, host: host, port: port) |
There was a problem hiding this comment.
When this sheet is opened from an already connected device tree, reusing the first-run connectManualHost path means ordinary failure cases still tear down the active Mac: validation failures set connectionState = .disconnected and call clearRemoteConnectionContext(), and later connect failures do the same in MobileShellComposite.swift (checked connectManualHost). A typo/offline host from the new Add device row therefore disconnects the session the user was using even though the sheet remains open for retry; the QR closure above has the same underlying first-run behavior on invalid/offline codes.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds an always-available "Add device" entry point to the iOS Devices sheet (
Confidence Score: 5/5Safe to merge. Policy logic is sound, cancel/restore invariants are correctly wired, and the acknowledged limitations are pre-existing and explicitly documented. The three changed files each address a single responsibility. Policy logic is pure and fully covered by 8 unit tests. The cancel-over-live-connection guard correctly suppresses cancelPairing() and the previousMacDeviceID capture+restore path is placed before the destructive async call. No new actor isolation issues, no new blocking primitives, no new user-facing strings. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant TV as DeviceTreeView
participant PV as PairingView (sheet)
participant ST as CMUXMobileShellStore
Note over TV: User opens Devices sheet (connected)
U->>TV: Tap Add device row or toolbar +
TV->>TV: "beginAddDevice() isShowingAddDevice = true"
TV->>PV: Present PairingView sheet
alt User cancels without starting attempt
U->>PV: Tap Cancel
PV->>TV: cancelPairing()
TV->>TV: "cancelResetsPairingState(.connected) = false"
Note over TV: cancelPairing() NOT called - live connection preserved
PV->>TV: "cancel() isShowingAddDevice = false"
else User starts pairing attempt
Note over TV: Capture liveMacDeviceID
U->>PV: Tap Connect / Scan QR
PV->>ST: connectPairingInput() or connectManualHost()
Note over ST: Destructive: tears down live client
alt Attempt succeeds
ST-->>TV: "connectionState = .connected"
TV->>TV: finishAddDevice() dismisses sheet
TV->>ST: "Task { loadPairedMacs() + loadRegistryDevices() }"
else Attempt cancelled (no connectionError)
ST-->>TV: "connectionState = .disconnected, no error"
TV->>TV: finishAddDevice() restores previous Mac
TV->>ST: "Task { switchToMac(macDeviceID: previousID) }"
else Attempt failed (connectionError set)
ST-->>TV: "connectionState = .disconnected, error set"
TV->>TV: finishAddDevice() no restore
Note over PV: Sheet stays up, error visible for retry
end
end
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Task { | ||
| await store.loadPairedMacs() | ||
| await store.loadRegistryDevices() | ||
| } |
There was a problem hiding this comment.
The Task { await store.loadPairedMacs(); await store.loadRegistryDevices() } is launched without a stored handle or structured cancellation. If DeviceTreeView is removed from the hierarchy (e.g., the user navigates away immediately after pairing) the refresh work continues in the background with no way to cancel it. Consider attaching this work to the view's lifetime by storing the handle or using a .task(id:) modifier keyed on a post-pairing signal, consistent with how the initial load is tied to the .task {} modifier above.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Review follow-up: the shared pairing path is destructive on failure (bad QR, offline preflight, manual-host validation, cancellation all leave the shell disconnected), so an add-device attempt started over a live connection could strand the user. Capture the live Mac's device id when the attempt starts and reconnect it via the existing switchToMac(macDeviceID:) when the attempt ends disconnected, matching that API's own never-strand-the-user failure contract. The restore runs in a fresh unstructured Task because a cancelled attempt's continuation executes in an already-cancelled task. Also reshape DeviceTreeAddDevicePolicy from a case-less namespace enum into a Sendable value type with an initializer per package API policy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| connectManualHost: { name, host, port in | ||
| let previousMacDeviceID = liveMacDeviceID | ||
| await store.connectManualHost(name: name, host: host, port: port) | ||
| finishAddDevice(previousMacDeviceID: previousMacDeviceID) |
There was a problem hiding this comment.
Retry loses restore Mac ID
Medium Severity
Each pairing attempt captures previousMacDeviceID from liveMacDeviceID at attempt start. After the first failure the shell is disconnected, so a retry captures nil and a second failed attempt will not invoke switchToMac to restore the Mac that was live when the sheet opened.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 21efbf9. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21efbf9f5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .sheet(isPresented: $isShowingAddDevice) { | ||
| addDeviceSheet | ||
| } |
There was a problem hiding this comment.
When a user starts a pairing attempt and then dismisses this sheet with the system swipe/background gesture, none of PairingView's Cancel button logic runs; startPairingTask creates an unstructured Task that is only canceled from that button. The in-flight connectPairingInput/connectManualHost can therefore continue after the user has dismissed the sheet and still replace the active Mac or reconnect in the background. Please disable interactive dismissal while pairing or make sheet dismissal invoke the same cancellation path.
Useful? React with 👍 / 👎.
| Task { | ||
| await store.switchToMac(macDeviceID: previousMacDeviceID) |
There was a problem hiding this comment.
When an add-device attempt that started over a live connection fails, this restore path calls switchToMac, which goes through connectManualHost and beginPairingAttempt, clearing the store's pairing error before reconnecting the previous Mac. That leaves the add-device sheet open for retry but removes the failure message/guidance from the attempt that just failed, so users get no explanation of the bad QR/offline host they need to fix. Preserve that error separately or restore the previous connection without clearing the add-device error state.
Useful? React with 👍 / 👎.
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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift`:
- Around line 185-192: The unstructured Task can perform a stale restore; before
calling store.switchToMac(macDeviceID:), re-check the current connection state
from the authoritative source and only proceed if the restore is still valid.
Concretely, inside the spawned Task capture previousMacDeviceID but query the
live state (e.g. via store.connectionState or an async accessor on store) and
call addDevicePolicy.restoresPreviousConnection(connectionState: currentState,
previousMacDeviceID: previousMacDeviceID) again; only if that returns true then
await store.switchToMac(macDeviceID: previousMacDeviceID). This ensures
switchToMac is skipped for stale attempts and prevents overwriting newer user
actions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9ba4365-2820-4e4e-88b3-f94bfca30536
📒 Files selected for processing (3)
Packages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/DeviceTreeAddDevicePolicy.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/DeviceTreeAddDevicePolicyTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift
| if addDevicePolicy.restoresPreviousConnection( | ||
| connectionState: state, | ||
| previousMacDeviceID: previousMacDeviceID | ||
| ), let previousMacDeviceID { | ||
| let store = store | ||
| Task { | ||
| await store.switchToMac(macDeviceID: previousMacDeviceID) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the restore task against stale-attempt races.
At Line 190, restore is launched in an unstructured Task without re-checking current state. If a user retries quickly, a stale restore from a prior failed attempt can still call switchToMac and override a newer attempt outcome.
Suggested fix
if addDevicePolicy.restoresPreviousConnection(
connectionState: state,
previousMacDeviceID: previousMacDeviceID
), let previousMacDeviceID {
let store = store
- Task {
- await store.switchToMac(macDeviceID: previousMacDeviceID)
+ Task { `@MainActor` in
+ // Skip stale restore if another attempt has already advanced state.
+ guard store.connectionState == .disconnected else { return }
+ await store.switchToMac(macDeviceID: previousMacDeviceID)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift`
around lines 185 - 192, The unstructured Task can perform a stale restore;
before calling store.switchToMac(macDeviceID:), re-check the current connection
state from the authoritative source and only proceed if the restore is still
valid. Concretely, inside the spawned Task capture previousMacDeviceID but query
the live state (e.g. via store.connectionState or an async accessor on store)
and call addDevicePolicy.restoresPreviousConnection(connectionState:
currentState, previousMacDeviceID: previousMacDeviceID) again; only if that
returns true then await store.switchToMac(macDeviceID: previousMacDeviceID).
This ensures switchToMac is skipped for stale attempts and prevents overwriting
newer user actions.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 790571e. Configure here.
| // `finishAddDevice` restores the previous Mac. | ||
| if addDevicePolicy.cancelResetsPairingState(connectionState: store.connectionState) { | ||
| store.cancelPairing() | ||
| } |
There was a problem hiding this comment.
Cancel leaves pairing attempt running
High Severity
When the user cancels during an in-flight add-device pairing, the sheet only calls cancelPairing() if connectionState is not .connected. The connect path can drop the live client while connectionState still reads .connected, so cancel skips cancelPairing() and never invalidates the attempt. Pairing can then finish after the sheet is dismissed, switching the session or calling finishAddDevice as success.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 790571e. Configure here.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The workflow-guard-tests failure (Validate Swift file length budget) is inherited from main, not this PR: #5651 grew Sources/TerminalNotificationStore.swift and Sources/Feed/FeedCoordinator.swift past the checked-in ledger. Fix: #6018. Once that lands, merging main here turns the guard green; this PR's own files are all under the 500-line threshold. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift (1)
189-198: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRe-check policy conditions inside the restore Task to prevent stale-attempt races.
The unstructured Task at lines 195-197 can perform a stale restore if the user retries quickly. The policy check at line 189 uses the captured
stateand livestore.connectionError, but between that check and the Task's execution, the store state may change:
- A successful new attempt connects to a different device → the stale Task would override by restoring the old Mac.
- A failed new attempt sets
connectionError→ the stale Task would callswitchToMac, clearing the actionable error the user needs to see (per the policy doc: "reconnect path begins fresh pairing which clearsconnectionError").🔒 Proposed fix: re-check the full policy condition inside the Task
if addDevicePolicy.restoresPreviousConnection( connectionState: state, previousMacDeviceID: previousMacDeviceID, hasConnectionError: store.connectionError != nil ), let previousMacDeviceID { let store = store + let policy = addDevicePolicy Task { `@MainActor` in + // Skip stale restore if state has changed (user already connected + // to another device, or a new attempt failed with an error). + guard policy.restoresPreviousConnection( + connectionState: store.connectionState, + previousMacDeviceID: previousMacDeviceID, + hasConnectionError: store.connectionError != nil + ) else { return } await store.switchToMac(macDeviceID: previousMacDeviceID) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift` around lines 189 - 198, The current code captures state and then launches a Task that unconditionally calls store.switchToMac(previousMacDeviceID), which can race and perform a stale restore; inside the Task re-evaluate addDevicePolicy.restoresPreviousConnection(connectionState: currentState, previousMacDeviceID: previousMacDeviceID, hasConnectionError: store.connectionError != nil) (using the live store/state rather than the earlier captured `state`) and only call store.switchToMac(previousMacDeviceID) if that re-check returns true and previousMacDeviceID is still non-nil; ensure you reference the same symbols (addDevicePolicy.restoresPreviousConnection, previousMacDeviceID, store.connectionError, store.switchToMac) so the Task uses up-to-date conditions before performing the restore.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift`:
- Around line 189-198: The current code captures state and then launches a Task
that unconditionally calls store.switchToMac(previousMacDeviceID), which can
race and perform a stale restore; inside the Task re-evaluate
addDevicePolicy.restoresPreviousConnection(connectionState: currentState,
previousMacDeviceID: previousMacDeviceID, hasConnectionError:
store.connectionError != nil) (using the live store/state rather than the
earlier captured `state`) and only call store.switchToMac(previousMacDeviceID)
if that re-check returns true and previousMacDeviceID is still non-nil; ensure
you reference the same symbols (addDevicePolicy.restoresPreviousConnection,
previousMacDeviceID, store.connectionError, store.switchToMac) so the Task uses
up-to-date conditions before performing the restore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f686bd1-d79f-459e-8d2f-dbd7ba2b0e8a
📒 Files selected for processing (3)
Packages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/DeviceTreeAddDevicePolicy.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/DeviceTreeAddDevicePolicyTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift
1 participant
Lawrence: "in the devices menu we should also be able to add a new device and go through the flow again."
Which side
The "devices menu" is the iOS Devices sheet (
DeviceTreeView, titled "Devices", opened from the workspace list toolbar). Once a phone was paired, the add-device pairing flow became unreachable from there: the rootPairingViewsheet only mounts on the disconnected branch, and the only connected-state path was Settings -> Switch Mac, which offers a bare QR scanner, not the full flow.The Mac side already has two re-entry points into its pairing window (Settings -> Mobile -> "Pair…" and the command palette "Connect iPhone/iPad") and has no Devices menu to extend, so no Mac changes here. A dedicated Mac menu item is a possible follow-up; left out to avoid colliding with the in-flight pairing-window changes.
Shared action and entry points
One shared
beginAddDevice()action inDeviceTreeView, invoked from two entry points per the shared-behavior policy:MobileDeviceTreeAddDeviceMobileDeviceTreeAddDeviceToolbarBoth present the existing first-run
PairingView(QR scan or manual host) as a sheet over the tree, driven by the same store mutations as the root pairing path (connectPairingInput/connectManualHost). No keyboard shortcut added. Pairing window internals (MobilePairingView/Model/WindowController, iOS URL scheme) are untouched.Re-entering the flow over a live connection needed explicit policy, extracted into
DeviceTreeAddDevicePolicy(a documented public value type inCmuxMobileShellModel, next toMobileConnectionState):cancelPairing()while still connected; that call setsconnectionState = .disconnectedand would tear down the live connection.loadPairedMacs+loadRegistryDevicesso the new device appears in the tree.connectionError— the store's cancellation path sets none) restores the previously live Mac via the existingswitchToMac(macDeviceID:), so backing out of the sheet never strands the user.connectionErrorand would erase the failure reason while the user reads it on the auto-presented pairing sheet.Review notes (autoreview, 3 rounds)
Fixed from review: the policy is a value type with an initializer (was a namespace enum), all public API has DocC, and the cancel-restore behavior above came out of round 1.
cmux-policy-checkis clean.Consciously deferred (one root cause, store-owned):
MobileShellComposite's pairing attempt is destructive — it can replace the live client before success, any new attempt clears the store-ownedconnectionError, and there is no way to invalidate an in-flight attempt without clearing the live context. Consequences the reviewer flagged: a genuinely failed add attempt still drops the live session (error stays readable, recovery is Settings -> Switch Mac), and a cancel during a mid-flight attempt cannot guarantee the attempt stops. Both are pre-existing in the shipped Settings -> Switch Mac -> "Pair Another Mac" path, which calls the same destructiveconnectPairingURLwhile connected with no restore and no cancellation at all; this PR's entry point is strictly safer than that path. The real fix is an attempt-scoped pairing API in the store (do not tear down the live client until the new connection succeeds; non-destructive cancel; failure reporting that survives a reconnect) — follow-up, and it intersects the concurrent pairing-flow work. Malformed input never reaches the destructive path:PairingViewvalidates manual hosts locally and the QR scanner accepts onlycmux-ios://pairing links.Tests
DeviceTreeAddDevicePolicyTests(8 tests) inCmuxMobileShellModelTests;swift test --package-path Packages/CmuxMobileShellModelgreen (38 tests, 7 suites). The policy lives in the model package becauseCmuxMobileShellUIdeclares iOS-only platforms and cannot host-runswift test.Builds verified: iOS simulator arm64 (
ios/cmux.xcworkspace, schemecmux-ios) and macOS (cmux.xcodeproj, schemecmux, build-only; the Mac app is behaviorally unchanged).Localization audit
No new user-facing strings. The row and the toolbar button's accessibility label reuse
mobile.addDevice.title("Add device");mobile.addDevice.title,mobile.common.done,mobile.deviceTree.titleverified present with en + ja inios/cmux/Resources/Localizable.xcstrings. Accessibility identifiers are not user-facing.Dogfood
ios/scripts/reload.sh --tag adddev), open the workspace list, tap the Devices toolbar icon.🤖 Generated with Claude Code
Note
Medium Risk
Changes mobile connection lifecycle around destructive pairing while connected; mitigated by explicit policy, restore via existing
switchToMac, and unit tests, with known pre-existing limits on mid-flight cancel documented in the PR.Overview
The iOS Devices sheet (
DeviceTreeView) now exposes Add device at all times—a list row (including empty state) and a toolbar +—both driving onebeginAddDevice()path that presents the existingPairingViewsheet over the tree.A new
DeviceTreeAddDevicePolicyin the model package encodes behavior when pairing re-enters over a live Mac session: Cancel must not callcancelPairing()while still connected; the sheet dismisses and refreshes the device list only on success; a cancelled attempt (disconnected, noconnectionError) restores the prior Mac viaswitchToMac; failed attempts keep the sheet and error visible and do not auto-restore (reconnect would clear the error).DeviceTreeAddDevicePolicyTests(8 cases) lock in those rules forswift teston the model package.Reviewed by Cursor Bugbot for commit 48ec656. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests