iOS: harden multi-Mac host switcher (session-drop, scoping, layering) - #5545
lawrencecchen wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds per-user pairedMacs state and four async operations (load, switch, forget, pair additional) to MobileShellComposite, centralizes connection teardown, and updates the host picker UI to dismiss the pairing sheet only after a confirmed pairing. ChangesPaired Mac Switching Operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (15 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: 09e33687bb
ℹ️ 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".
| /// one the live connection targets, switches on tap, forgets on swipe, and pairs | ||
| /// a new Mac by scanning its QR code without dropping the others. | ||
| struct MobileHostPickerView: View { | ||
| @Bindable var store: CMUXMobileShellStore |
There was a problem hiding this comment.
AGENTS.md's “Snapshot boundary for list subtrees” rule says any SwiftUI subtree containing a List/ForEach of rows must not hold an @Observable store reference, including @Bindable; this new picker keeps @Bindable var store while rendering List { ForEach(store.pairedMacs) ... }. When this sheet is open during unrelated shell/terminal updates, it reintroduces the list-row invalidation pattern the guideline calls out as causing LazyLayoutViewCache CPU spins; split this into a container that snapshots pairedMacs and passes closure actions into a value-only list/row view.
Useful? React with 👍 / 👎.
| var rescanQR: (() -> Void)? | ||
| var signOut: (() -> Void)? | ||
| /// The shell store, forwarded to Settings to drive the multi-Mac switcher. | ||
| /// `nil` in previews. |
There was a problem hiding this comment.
This adds a plain CMUXMobileShellStore reference to WorkspaceListView, whose body owns the workspace List/ForEach; AGENTS.md's snapshot-boundary rule explicitly bans even a plain store property below list boundaries because orthogonal observable changes can invalidate every row and revive the prior LazyLayoutViewCache spin-loop class. Pass a narrow settings/host-picker action or a value snapshot instead of threading the observable shell store through the list view.
Useful? React with 👍 / 👎.
Greptile SummaryThis is a focused hardening follow-up to the iOS multi-Mac host switcher, addressing three issues found during autoreview: session-drop on a failed "Pair Another Mac" scan, a cross-user reconnect hazard in the failure fallback, and a claimed package boundary cleanup.
Confidence Score: 5/5Safe to merge — the two functional fixes (session-drop guard and scoped-identity fallback) are correct and well-structured; the package boundary claim in the description is incomplete but does not affect runtime behavior. The core logic in Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swift — the direct CmuxMobilePairedMac import and Package.swift dependency were not removed as described, and the else-branch causes a redundant reload on scan failures. Important Files Changed
Sequence DiagramsequenceDiagram
participant View as MobileHostPickerView
participant Store as MobileShellComposite
participant PairingURL as connectPairingURLResult
participant MacStore as pairedMacStore
View->>Store: pairAdditionalMac(code)
Note over Store: capture hadLiveConnection<br/>capture fallbackStackUserID
Store->>PairingURL: await connectPairingURLResult(rawValue)
alt .connected
PairingURL-->>Store: .connected
Store->>MacStore: loadPairedMacs()
Store-->>View: true
View->>View: dismiss()
else .superseded
PairingURL-->>Store: .superseded
Store-->>View: false
View->>MacStore: loadPairedMacs()
else .failed
PairingURL-->>Store: .failed
alt "hadLiveConnection && fallbackStackUserID != nil"
Store->>MacStore: reconnectActiveMacIfAvailable(stackUserID:)
end
Store->>MacStore: loadPairedMacs()
Store-->>View: false
View->>MacStore: loadPairedMacs() ⚠️ redundant on .failed
end
Reviews (5): Last reviewed commit: "Fix iOS host picker compile: restore Cmu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try exec(""" | ||
| UPDATE paired_macs SET is_active = 0 | ||
| WHERE stack_user_id IS ( | ||
| SELECT stack_user_id FROM paired_macs WHERE mac_device_id = ? | ||
| ); | ||
| """, | ||
| binding: [.text(macDeviceID)]) |
There was a problem hiding this comment.
When mac_device_id no longer exists in the table (e.g., the user swipes to forget that exact Mac while switchToMac is suspended in await connectManualHost, then the successful-connect branch calls setActive), the scalar subquery returns NULL and WHERE stack_user_id IS NULL silently wipes the is_active flag for every row whose stack_user_id is NULL — including rows that belong to completely unrelated NULL-scoped users. The subsequent SET is_active = 1 then also finds no rows, so no Mac ends up active. Because MobileShellComposite is @MainActor async, a forgetMac task from a concurrent swipe action CAN interleave at the await connectManualHost suspension point, making this sequence reachable in production. Adding an EXISTS guard (or pre-fetching the stack_user_id before entering the transaction) prevents the NULL propagation.
| MobilePairingScannerSheet { code in | ||
| showingScanner = false | ||
| Task { | ||
| _ = await store.connectPairingURL(code) | ||
| await store.loadPairedMacs() | ||
| dismiss() | ||
| } | ||
| } |
There was a problem hiding this comment.
connectPairingURL is called and its result is discarded with _, then dismiss() is called regardless of success or failure. If the QR code is stale or the network is down, the user is returned to Settings with no error shown — the host picker and scanner are both gone, and any connectionError set on the store is only visible once the user navigates back to the workspace view. The existing direct-scan pairing flow surfaces errors inline; this secondary path in the picker breaks that expectation.
| public func switchToMac(macDeviceID: String) async { | ||
| guard let pairedMacStore, | ||
| let target = pairedMacs.first(where: { $0.macDeviceID == macDeviceID }) else { return } | ||
| if target.isActive, connectionState == .connected { return } | ||
| // The currently-active Mac to fall back to if the switch fails. | ||
| let previousActive = pairedMacs.first { $0.isActive && $0.macDeviceID != macDeviceID } | ||
| let supportedKinds = runtime?.supportedRouteKinds ?? [] | ||
| guard let (host, port) = Self.firstReconnectHostPortRoute( | ||
| target.routes, | ||
| supportedKinds: supportedKinds | ||
| ), let normalizedHost = MobileShellRouteAuthPolicy.normalizedManualHost(host) else { | ||
| mobileShellLog.error("switchToMac: no reconnectable route mac=\(macDeviceID, privacy: .public)") | ||
| return | ||
| } | ||
| await connectManualHost(name: target.displayName ?? host, host: host, port: port) | ||
| // Persist the active row only if the live connection is to THIS Mac's | ||
| // route. A different switch tapped while this connect was in flight | ||
| // supersedes it via `beginPairingAttempt`, leaving `connectionState` | ||
| // `.connected` for the other Mac; matching the live route prevents this | ||
| // superseded task from persisting a stale active target. | ||
| if connectionState == .connected, | ||
| case let .hostPort(liveHost, livePort)? = activeRoute?.endpoint, | ||
| liveHost == normalizedHost, livePort == port { | ||
| do { | ||
| try await pairedMacStore.setActive(macDeviceID: macDeviceID) | ||
| } catch { | ||
| mobileShellLog.error("paired mac store setActive failed mac=\(macDeviceID, privacy: .public) error=\(String(describing: error), privacy: .public)") | ||
| } | ||
| } else if previousActive != nil, connectionState != .connected { | ||
| // The switch did not connect and the destructive connect path dropped | ||
| // the previous session; reconnect to the still-active previous Mac so | ||
| // the user is not left stranded on a failed switch. | ||
| _ = await reconnectActiveMacIfAvailable(stackUserID: identityProvider?.currentUserID) | ||
| } | ||
| await loadPairedMacs() |
There was a problem hiding this comment.
switchToMac provides no in-progress feedback to the view layer
connectManualHost is a destructive async connect that can take several seconds. During the whole window the view shows the old checkmark (the isActive flag on pairedMacs is stale in memory) and no spinner or disabled row state. A user who taps a Mac, waits, and sees nothing may tap it again, triggering a second superseding task. The existing connection-state properties (connectionState, macConnectionStatus) could be surfaced in the row to indicate a switch is in flight; alternatively exposing a Bool flag such as isSwitchingMac that is set before the connect and cleared in loadPairedMacs() would be enough for the view to disable the row or show a progress indicator.
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!
![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/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swift`:
- Around line 206-214: The two UPDATEs mutate all rows with stack_user_id IS
NULL when macDeviceID is missing; fix by first resolving the target
stack_user_id and aborting if none found: run a SELECT stack_user_id FROM
paired_macs WHERE mac_device_id = ? (use the same macDeviceID) and if it returns
nil do not execute the UPDATEs; otherwise use that resolved stackUserID in the
exec calls (e.g. UPDATE paired_macs SET is_active = 0 WHERE stack_user_id = ?
and UPDATE paired_macs SET is_active = 1 WHERE mac_device_id = ?) to avoid the
NULL-scoped deactivation. Ensure you reference the same macDeviceID and the
exec(...) call sites when making this change.
🪄 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: 93d4c233-bb9f-4c3b-a7bd-e8030d7fbd80
📒 Files selected for processing (9)
Packages/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swiftPackages/CmuxMobilePairedMac/Tests/CmuxMobilePairedMacTests/MobilePairedMacStoreTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellUI/Package.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftios/cmux/Resources/Localizable.xcstrings
| try exec(""" | ||
| UPDATE paired_macs SET is_active = 0 | ||
| WHERE stack_user_id IS ( | ||
| SELECT stack_user_id FROM paired_macs WHERE mac_device_id = ? | ||
| ); | ||
| """, | ||
| binding: [.text(macDeviceID)]) | ||
| try exec("UPDATE paired_macs SET is_active = 1 WHERE mac_device_id = ?;", | ||
| binding: [.text(macDeviceID)]) |
There was a problem hiding this comment.
Prevent unintended NULL-scope deactivation when the target Mac row is missing.
If mac_device_id is absent, the subquery at Line 209 yields NULL, so Line 208 clears every stack_user_id IS NULL row, then Line 213 updates nothing. That mutates unrelated scope state on an invalid ID.
💡 Suggested fix
- try exec("""
- UPDATE paired_macs SET is_active = 0
- WHERE stack_user_id IS (
- SELECT stack_user_id FROM paired_macs WHERE mac_device_id = ?
- );
- """,
- binding: [.text(macDeviceID)])
+ try exec("""
+ UPDATE paired_macs
+ SET is_active = 0
+ WHERE EXISTS (
+ SELECT 1 FROM paired_macs target WHERE target.mac_device_id = ?
+ )
+ AND stack_user_id IS (
+ SELECT target.stack_user_id
+ FROM paired_macs target
+ WHERE target.mac_device_id = ?
+ );
+ """,
+ binding: [.text(macDeviceID), .text(macDeviceID)])🤖 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/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swift`
around lines 206 - 214, The two UPDATEs mutate all rows with stack_user_id IS
NULL when macDeviceID is missing; fix by first resolving the target
stack_user_id and aborting if none found: run a SELECT stack_user_id FROM
paired_macs WHERE mac_device_id = ? (use the same macDeviceID) and if it returns
nil do not execute the UPDATEs; otherwise use that resolved stackUserID in the
exec calls (e.g. UPDATE paired_macs SET is_active = 0 WHERE stack_user_id = ?
and UPDATE paired_macs SET is_active = 1 WHERE mac_device_id = ?) to avoid the
NULL-scoped deactivation. Ensure you reference the same macDeviceID and the
exec(...) call sites when making this change.
| let previousActive = pairedMacs.first { $0.isActive && $0.macDeviceID != macDeviceID } | ||
| let supportedKinds = runtime?.supportedRouteKinds ?? [] |
There was a problem hiding this comment.
switchToMac reads identityProvider?.currentUserID after the await connectManualHost suspension point, unlike pairAdditionalMac which explicitly pre-captures fallbackStackUserID for the same reason. If a sign-out races during connectManualHost, currentUserID can be nil or belong to a different user, causing reconnectActiveMacIfAvailable(stackUserID: nil) to use the all-users store query — the exact cross-user reconnect hazard the pairAdditionalMac comment describes as unsafe on shared devices.
| let previousActive = pairedMacs.first { $0.isActive && $0.macDeviceID != macDeviceID } | |
| let supportedKinds = runtime?.supportedRouteKinds ?? [] | |
| let previousActive = pairedMacs.first { $0.isActive && $0.macDeviceID != macDeviceID } | |
| // Capture before the destructive connect for the same reason pairAdditionalMac | |
| // does: if a sign-out races during the await, currentUserID becomes nil and | |
| // reconnectActiveMacIfAvailable(stackUserID: nil) would use the all-users query, | |
| // potentially reconnecting another Stack user's Mac on a shared device. | |
| let fallbackStackUserID = identityProvider?.currentUserID | |
| let supportedKinds = runtime?.supportedRouteKinds ?? [] |
… Mac The host picker's "Pair Another Mac" called connectPairingURL directly and ignored the result, dismissing unconditionally. That connect path is destructive (it begins a fresh pairing attempt and replaces the live remote client), so scanning a stale/expired/offline QR could tear down a working session with no fallback. Add MobileShellComposite.pairAdditionalMac, mirroring switchToMac's safety: capture whether a live connection exists, attempt the pairing connect, and on failure reconnect the still-active stored Mac so the user is not stranded. persistPairedMacFromTicket runs only on the success path, so the store's active row still points at the previous Mac on failure and the reconnect targets it, not the broken new one. The picker now dismisses only on a successful connect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ent Stack user pairAdditionalMac's failure fallback passed identityProvider?.currentUserID straight into reconnectActiveMacIfAvailable. When that is nil, the store's activeMac(stackUserID: nil) is an all-users query, so on a shared iOS device a failed "Pair Another Mac" scan could reconnect to another Stack user's active Mac, violating the same scoping guarantee loadPairedMacs enforces. Capture the scoped Stack user id before the destructive connect and only reconnect within that scope; skip the fallback entirely when there is no scoped identity rather than fall into the unscoped lookup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…route-equality P2 (layering): CmuxMobileShellUI depended directly on the CmuxMobilePairedMac service package only to name MobilePairedMac in the picker. CmuxMobileShell (the domain package the UI already depends on) `public import`s CmuxMobilePairedMac and exposes `pairedMacs: [MobilePairedMac]`, so the type is re-exported through the allowed domain->service edge. Name it through `import CmuxMobileShell` and drop the direct UI->service dependency + import to keep the package DAG within the UI-depends-on-domain rule. P1 (switchToMac active-row): documented as reject-with-evidence. connectManualHost mints a synthetic `manual-<host>:<port>` ticket id and persist skips `manual-` ids, so there is no real Mac id to reconcile against; route-equality is the best available signal. A reassigned host:port is an unhandleable manual-reconnect limitation shared with reconnectActiveMacIfAvailable, not a switch-specific regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
946c699 to
fea2abc
Compare
lawrencecchen
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fea2abc928
ℹ️ 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".
| "CmuxMobileCamera", | ||
| "CmuxMobileDiagnostics", | ||
| "CmuxMobilePairedMac", | ||
| "CmuxMobileShell", |
There was a problem hiding this comment.
With CmuxMobilePairedMac removed from this target's dependencies/imports, MobileHostPickerView still names MobilePairedMac directly in macRow(_:), so the iOS UI target no longer has that type in scope. public import CmuxMobilePairedMac inside CmuxMobileShell is not enough to make the type name available to clients that only import CmuxMobileShell; this will fail to type-check when the iOS sources are built. Please either keep the direct package dependency/import or expose a type/adapter from CmuxMobileShell that the UI can name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
237-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset and refresh the recovery user scope.
recoverMobileConnection()later reuseslastReconnectStackUserID, butsignOut()leaves the old value behind and these successful switch/pair paths never overwrite it. On a shared device, a later drop can auto-reconnect the previous user's active Mac.Suggested fix
public func signOut() { pairingAttemptID = UUID() connectionGeneration = UUID() + lastReconnectStackUserID = nil isSignedIn = false connectionState = .disconnected macConnectionStatus = .unavailable// In the shared "connect succeeded" path, refresh the recovery scope too. lastReconnectStackUserID = identityProvider?.currentUserIDAlso applies to: 648-662, 707-709
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 237 - 259, signOut() currently resets many session fields but does not clear/update lastReconnectStackUserID, which allows recoverMobileConnection() to reuse the previous user's recovery scope; update signOut() to set lastReconnectStackUserID = identityProvider?.currentUserID (or nil if appropriate) and also ensure the shared "connect succeeded" path refreshes lastReconnectStackUserID after a successful connection (where connect success logic runs) so the recovery scope always reflects the currently-signed-in user; reference the signOut() method, lastReconnectStackUserID, recoverMobileConnection(), and identityProvider?.currentUserID when making these changes.
845-855:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't forget the active Mac by transient ticket id.
After
connectManualHost/switchToMac,activeTicket.macDeviceIDis often the syntheticmanual-<host>:<port>value, not the persisted paired-Mac row. This removal can miss the real active record, so “Rescan QR” leaves that Mac active in storage and it can reconnect on the next launch.Suggested fix
public func disconnectAndForgetActiveMac() { - let staleMacID = activeTicket?.macDeviceID + let stackUserID = identityProvider?.currentUserID disconnectLiveConnection() - if let pairedMacStore, let macID = staleMacID { + if let pairedMacStore { // Fire-and-forget: forgetting the persisted mac is cleanup that must // not block the synchronous disconnect UI state update above. Task { do { - try await pairedMacStore.remove(macDeviceID: macID) + if let macID = try await pairedMacStore.activeMac(stackUserID: stackUserID)?.macDeviceID { + try await pairedMacStore.remove(macDeviceID: macID) + } } catch { mobileShellLog.error("forgetActiveMac removal failed: \(String(describing: error), privacy: .private)") } } }🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 845 - 855, disconnectAndForgetActiveMac currently removes by activeTicket.macDeviceID which may be a synthetic "manual-<host>:<port>" id and thus won't delete the persisted paired row; instead, detect synthetic manual ids (e.g. prefix "manual-") or when pairedMacStore.remove by that id fails, resolve the real persisted mac id from the store (lookup by host/port or other matching fields on activeTicket) and remove that real id; update disconnectAndForgetActiveMac to: (1) extract staleMacID = activeTicket?.macDeviceID, (2) if staleMacID starts with "manual-" (or removal returns not found), call pairedMacStore to find the persisted paired record that matches activeTicket's host/port/unique connection info, get its macDeviceID, and call pairedMacStore.remove(macDeviceID: realMacID); otherwise fall back to the existing remove(staleMacID) behavior.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 596-600: The scoped loadAll error path leaves stale pairedMacs in
memory; when pairedMacStore.loadAll(stackUserID: ...) throws, clear the cached
host list before returning by resetting the pairedMacs collection (or equivalent
storage used by MobileShellComposite) to an empty state so the UI won't show
another user's Macs; update the catch block around pairedMacStore.loadAll to set
pairedMacs = [] (or call the existing clear method) then log the error and
return.
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swift`:
- Around line 65-69: The else branch unconditionally calls await
store.loadPairedMacs(), which duplicates reload logic and breaks
pairAdditionalMac(code)'s supersession semantics; remove the else { await
store.loadPairedMacs() } block and let store.pairAdditionalMac(code) handle
reloads and supersession behavior (keep dismiss() on true as-is).
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 237-259: signOut() currently resets many session fields but does
not clear/update lastReconnectStackUserID, which allows
recoverMobileConnection() to reuse the previous user's recovery scope; update
signOut() to set lastReconnectStackUserID = identityProvider?.currentUserID (or
nil if appropriate) and also ensure the shared "connect succeeded" path
refreshes lastReconnectStackUserID after a successful connection (where connect
success logic runs) so the recovery scope always reflects the
currently-signed-in user; reference the signOut() method,
lastReconnectStackUserID, recoverMobileConnection(), and
identityProvider?.currentUserID when making these changes.
- Around line 845-855: disconnectAndForgetActiveMac currently removes by
activeTicket.macDeviceID which may be a synthetic "manual-<host>:<port>" id and
thus won't delete the persisted paired row; instead, detect synthetic manual ids
(e.g. prefix "manual-") or when pairedMacStore.remove by that id fails, resolve
the real persisted mac id from the store (lookup by host/port or other matching
fields on activeTicket) and remove that real id; update
disconnectAndForgetActiveMac to: (1) extract staleMacID =
activeTicket?.macDeviceID, (2) if staleMacID starts with "manual-" (or removal
returns not found), call pairedMacStore to find the persisted paired record that
matches activeTicket's host/port/unique connection info, get its macDeviceID,
and call pairedMacStore.remove(macDeviceID: realMacID); otherwise fall back to
the existing remove(staleMacID) behavior.
🪄 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: 20977a72-e648-493c-9537-2755ebae35d5
📒 Files selected for processing (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
237-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset and refresh the recovery user scope.
recoverMobileConnection()later reuseslastReconnectStackUserID, butsignOut()leaves the old value behind and these successful switch/pair paths never overwrite it. On a shared device, a later drop can auto-reconnect the previous user's active Mac.Suggested fix
public func signOut() { pairingAttemptID = UUID() connectionGeneration = UUID() + lastReconnectStackUserID = nil isSignedIn = false connectionState = .disconnected macConnectionStatus = .unavailable// In the shared "connect succeeded" path, refresh the recovery scope too. lastReconnectStackUserID = identityProvider?.currentUserIDAlso applies to: 648-662, 707-709
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 237 - 259, signOut() currently resets many session fields but does not clear/update lastReconnectStackUserID, which allows recoverMobileConnection() to reuse the previous user's recovery scope; update signOut() to set lastReconnectStackUserID = identityProvider?.currentUserID (or nil if appropriate) and also ensure the shared "connect succeeded" path refreshes lastReconnectStackUserID after a successful connection (where connect success logic runs) so the recovery scope always reflects the currently-signed-in user; reference the signOut() method, lastReconnectStackUserID, recoverMobileConnection(), and identityProvider?.currentUserID when making these changes.
845-855:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't forget the active Mac by transient ticket id.
After
connectManualHost/switchToMac,activeTicket.macDeviceIDis often the syntheticmanual-<host>:<port>value, not the persisted paired-Mac row. This removal can miss the real active record, so “Rescan QR” leaves that Mac active in storage and it can reconnect on the next launch.Suggested fix
public func disconnectAndForgetActiveMac() { - let staleMacID = activeTicket?.macDeviceID + let stackUserID = identityProvider?.currentUserID disconnectLiveConnection() - if let pairedMacStore, let macID = staleMacID { + if let pairedMacStore { // Fire-and-forget: forgetting the persisted mac is cleanup that must // not block the synchronous disconnect UI state update above. Task { do { - try await pairedMacStore.remove(macDeviceID: macID) + if let macID = try await pairedMacStore.activeMac(stackUserID: stackUserID)?.macDeviceID { + try await pairedMacStore.remove(macDeviceID: macID) + } } catch { mobileShellLog.error("forgetActiveMac removal failed: \(String(describing: error), privacy: .private)") } } }🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 845 - 855, disconnectAndForgetActiveMac currently removes by activeTicket.macDeviceID which may be a synthetic "manual-<host>:<port>" id and thus won't delete the persisted paired row; instead, detect synthetic manual ids (e.g. prefix "manual-") or when pairedMacStore.remove by that id fails, resolve the real persisted mac id from the store (lookup by host/port or other matching fields on activeTicket) and remove that real id; update disconnectAndForgetActiveMac to: (1) extract staleMacID = activeTicket?.macDeviceID, (2) if staleMacID starts with "manual-" (or removal returns not found), call pairedMacStore to find the persisted paired record that matches activeTicket's host/port/unique connection info, get its macDeviceID, and call pairedMacStore.remove(macDeviceID: realMacID); otherwise fall back to the existing remove(staleMacID) behavior.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 596-600: The scoped loadAll error path leaves stale pairedMacs in
memory; when pairedMacStore.loadAll(stackUserID: ...) throws, clear the cached
host list before returning by resetting the pairedMacs collection (or equivalent
storage used by MobileShellComposite) to an empty state so the UI won't show
another user's Macs; update the catch block around pairedMacStore.loadAll to set
pairedMacs = [] (or call the existing clear method) then log the error and
return.
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swift`:
- Around line 65-69: The else branch unconditionally calls await
store.loadPairedMacs(), which duplicates reload logic and breaks
pairAdditionalMac(code)'s supersession semantics; remove the else { await
store.loadPairedMacs() } block and let store.pairAdditionalMac(code) handle
reloads and supersession behavior (keep dismiss() on true as-is).
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 237-259: signOut() currently resets many session fields but does
not clear/update lastReconnectStackUserID, which allows
recoverMobileConnection() to reuse the previous user's recovery scope; update
signOut() to set lastReconnectStackUserID = identityProvider?.currentUserID (or
nil if appropriate) and also ensure the shared "connect succeeded" path
refreshes lastReconnectStackUserID after a successful connection (where connect
success logic runs) so the recovery scope always reflects the
currently-signed-in user; reference the signOut() method,
lastReconnectStackUserID, recoverMobileConnection(), and
identityProvider?.currentUserID when making these changes.
- Around line 845-855: disconnectAndForgetActiveMac currently removes by
activeTicket.macDeviceID which may be a synthetic "manual-<host>:<port>" id and
thus won't delete the persisted paired row; instead, detect synthetic manual ids
(e.g. prefix "manual-") or when pairedMacStore.remove by that id fails, resolve
the real persisted mac id from the store (lookup by host/port or other matching
fields on activeTicket) and remove that real id; update
disconnectAndForgetActiveMac to: (1) extract staleMacID =
activeTicket?.macDeviceID, (2) if staleMacID starts with "manual-" (or removal
returns not found), call pairedMacStore to find the persisted paired record that
matches activeTicket's host/port/unique connection info, get its macDeviceID,
and call pairedMacStore.remove(macDeviceID: realMacID); otherwise fall back to
the existing remove(staleMacID) behavior.
🪄 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: 20977a72-e648-493c-9537-2755ebae35d5
📒 Files selected for processing (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swift
🛑 Comments failed to post (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
596-600:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the cached host list when the scoped load fails.
This error path leaves the previous in-memory
pairedMacsintact. If user A's list is still cached and the scoped load for user B fails, the picker keeps rendering A's Macs on a shared device.Suggested fix
do { loaded = try await pairedMacStore.loadAll(stackUserID: stackUserID) } catch { mobileShellLog.error("paired mac store loadAll failed: \(String(describing: error), privacy: .public)") + pairedMacs = [] return }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.do { loaded = try await pairedMacStore.loadAll(stackUserID: stackUserID) } catch { mobileShellLog.error("paired mac store loadAll failed: \(String(describing: error), privacy: .public)") pairedMacs = [] return }🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 596 - 600, The scoped loadAll error path leaves stale pairedMacs in memory; when pairedMacStore.loadAll(stackUserID: ...) throws, clear the cached host list before returning by resetting the pairedMacs collection (or equivalent storage used by MobileShellComposite) to an empty state so the UI won't show another user's Macs; update the catch block around pairedMacStore.loadAll to set pairedMacs = [] (or call the existing clear method) then log the error and return.Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swift (1)
65-69:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRespect
pairAdditionalMacsupersession semantics in the false branch.
pairAdditionalMacalready reloads pairings on failure and intentionally avoids reloading when superseded. The unconditionalawait store.loadPairedMacs()onfalsereintroduces redundant refreshes and can interfere with the takeover path that superseded this attempt.Suggested fix
- if await store.pairAdditionalMac(code) { - dismiss() - } else { - await store.loadPairedMacs() - } + if await store.pairAdditionalMac(code) { + dismiss() + }🤖 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/MobileHostPickerView.swift` around lines 65 - 69, The else branch unconditionally calls await store.loadPairedMacs(), which duplicates reload logic and breaks pairAdditionalMac(code)'s supersession semantics; remove the else { await store.loadPairedMacs() } block and let store.pairAdditionalMac(code) handle reloads and supersession behavior (keep dismiss() on true as-is).
…edge fea2abc dropped both `import CmuxMobilePairedMac` from MobileHostPickerView and the CmuxMobilePairedMac product dependency from CmuxMobileShellUI's Package.swift, betting that CmuxMobileShell's `public import CmuxMobilePairedMac` re-export would make the bare `MobilePairedMac` type resolvable through `import CmuxMobileShell`. It does not: Swift does not transitively re-export a named type into a module that only `import`s the re-exporter, so the iOS build failed with `cannot find type 'MobilePairedMac' in scope` at MobileHostPickerView.swift:78 (red ios-simulator CI). Revert only the package-edge part of fea2abc: re-add the `CmuxMobilePairedMac` package reference + target dependency and the `import CmuxMobilePairedMac`. The P1 session-drop/scoping fixes (pairAdditionalMac, switchToMac route-equality docs) are untouched. Verified by building the CmuxMobileShellUI scheme for the iOS Simulator (arm64+x86_64): BUILD SUCCEEDED, so MobilePairedMac resolves at line 78. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bf35664. Configure here.
| return true | ||
| case .superseded: | ||
| // Another pairing/switch attempt took over; leave its state intact. | ||
| return false |
There was a problem hiding this comment.
Superseded pairing skips session reconnect
Medium Severity
A second “Pair Another Mac” attempt that supersedes an in-flight first scan can leave the user disconnected. hadLiveConnection is captured per call, so after the first attempt’s destructive connect clears the session the second may record no live session and skip reconnect on failure, while the superseded first attempt returns without running its .failed reconnect path.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bf35664. Configure here.
Reset to origin/main (composer #5876 landed), then merge old dog HEAD a300868 to preserve every feature not yet on main: notifications dismiss-sync (#5568), multi-Mac switcher hardening (#5545), foreground repaint (#5571), image paste too-large toast (#5572), hidden native input (#5596), workspace groups (#5625), wslist round-10 snapshot, scroll-to-bottom hysteresis, DEV dogfood pane, attachments button, arrow toolbar keys, terminal.paste capability gating. Conflict policy: main's reviewed composer-land form wins for composer core (keyed focus handshake, draft FIFO coalescing, paste submit partial-success), dog wins for unlanded feature surface. ghostty pinned to dog 34cbf18 (descendant of main's e5c962a). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Carries unlanded: multi-Mac #5545, notif-sync #5568, foreground-repaint #5571, image-paste toast #5572, hidden-input #5596, groups (iOS side) #5625, scroll-hysteresis, dogfood pane, capabilities superset. Main's reviewed forms win: TerminalController decomposition (Control*Context), notif-tap-deeplink #5927, mobile.terminal.* routing.
…#5596/#5625/#5628) over current main Beta queue (#5876/#5872/#5869/#5875/#5927/#5912/#5726/#5776/#5916) is now on main; conflicts resolved by taking main as authoritative for the merged workspace-list/notifications/read-state/close surface, while preserving the carry-set: terminal.paste capability (#5572), hidden-input strings (#5596), smooth-scroll/scroll-to-bottom (#5628), and the live notifications feed (notificationsStore + mobile.notifications.list/mark_read dispatch). Dropped the superseded mute design. Capability flags unified onto main's computed supportedHostCapabilities set (added computed supportsTerminalPaste + DEBUG supportsDogfoodChecklist). xcstrings merged (HEAD-precedence union, mute keys dropped). pbxproj took HEAD consistently; budget regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reconcile pairAdditionalMac with main's .needsUserApproval pairing path: host picker scan now uses the session-preserving pairAdditionalMac and defers version-skew approval to the compatibility alert.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Focused hardening follow-up to #5513 ("iOS: multi-Mac host switcher", @lawrencecchen), which has now merged to
main. This branch is rebased onto currentmain, so the diff is only the three fixes below (the switcher itself is already onmain).All three came out of an autoreview pass over the switcher code:
P1, session not dropped on a failed "Pair Another Mac". The host picker called
connectPairingURLdirectly and dismissed unconditionally; that connect path is destructive, so scanning a stale/expired/offline QR could tear down a working session with no fallback. NewMobileShellComposite.pairAdditionalMacmirrorsswitchToMac's safety: attempt the connect, and on failure reconnect the still-active stored Mac.persistPairedMacFromTicketruns only on the success path, so the store's active row still points at the previous Mac on failure and the reconnect targets it. The picker now dismisses only on a successful connect.P1, scoped the failed-pair reconnect fallback. The fallback passed
identityProvider?.currentUserIDstraight in; when nil,activeMac(stackUserID: nil)is the store's all-users query, so on a shared device a failed scan could reconnect to another Stack user's active Mac. The fallback now only runs with a non-nil scoped user id, matchingloadPairedMacs's discipline.P2, dropped the direct UI→service package edge.
CmuxMobileShellUIdepended directly on theCmuxMobilePairedMacservice package only to nameMobilePairedMac.CmuxMobileShell(the domain package the UI already depends on)public imports it and exposespairedMacs, so the type is reached through the allowed domain→service edge instead.The
switchToMacroute-equality active-row concern raised in review is documented as reject-with-evidence:connectManualHostmints a syntheticmanual-<host>:<port>ticket id, so there is no real Mac id to reconcile against and route-equality is the best available signal.Both
testWorkspaceToolbarCreatesWorkspaceAndTerminalandtestTUITerminalUsesAvailableViewportAndResizespass on this branch HEAD (verified via test-e2e at SHA 946c699, which also proves the package change compiles on iOS).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Note
Medium Risk
Changes live connection teardown and reconnect fallback during pairing; scoped user id avoids cross-account reconnect on shared devices but session handling must stay correct on superseded attempts.
Overview
Pair Another Mac no longer calls the destructive
connectPairingURLpath and dismisses unconditionally.MobileShellComposite.pairAdditionalMacruns pairing viaconnectPairingURLResult, returns success only when the new Mac connects, and on failure reconnects the prior session withreconnectActiveMacIfAvailable(stackUserID:)only when a scoped Stack user id was captured before connect—avoiding an unscoped store lookup on shared devices.MobileHostPickerViewdismisses the picker only ontrue; otherwise it refreshes paired Macs and leaves the sheet open.Adds documentation in
switchToMacthat persisting the active paired row must use live host:port route equality, because manual reconnect uses syntheticmanual-<host>:<port>ticket ids that cannot match stored Mac device ids.Reviewed by Cursor Bugbot for commit bf35664. Bugbot is set up for automated code reviews on this repo. Configure here.