iOS pairing: surface network / auth / trust as individual check marks#6109
iOS pairing: surface network / auth / trust as individual check marks#6109austinywang wants to merge 11 commits into
Conversation
The pairing flow collapsed every failure into one opaque "could not connect", so users couldn't tell a network problem from an auth or trust problem (#6084). This adds a discrete network / authentication / trust checklist whose gates resolve individually, each with an in-progress, success, or failure state and an actionable message on failure. - MobilePairingChecklist model (CmuxMobileShellModel): three gates (network/authentication/trust) each holding pending / inProgress / succeeded / failed(message, guidance), plus .connecting and .connected snapshots. - MobilePairingFailureCategory.stage + clearsPriorGates and a pure MobilePairingChecklist.resolving(_:) builder: the single projection from a classified failure to which check mark fails and which earlier gates the failure proves were cleared (an on-the-wire auth/account rejection proves the network gate; a pre-transport or route-refused failure proves nothing). - MobileShellComposite paints the checklist through the existing failure/success choke points (begin/resolve/connected/clear), gated on an in-flight attempt. - PairingView renders the checklist once the user starts a pairing attempt from the screen, so a background reconnect never shows a stale checklist; the failed gate carries the headline + guidance inline (replacing the single error banner for connection attempts). - Localized the new strings (en + ja) in both string catalogs. - Tests: pure stage/clearsPriorGates/resolving mapping, plus composite end-to-end coverage for offline, auth rejection, account mismatch, and success. Fixes #6084 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a three-gate pairing checklist (network → authentication → trust) to the iOS pairing flow. A new ChangesMobile Pairing Checklist Feature
Sequence DiagramsequenceDiagram
participant User
participant PairingView
participant MobileShellComposite
participant MobileCoreRPCClient
participant MobilePairingChecklist
User->>PairingView: Taps "Connect"
PairingView->>MobileShellComposite: beginPairingAttempt(method:)
MobileShellComposite->>MobileShellComposite: isForegroundPairingAttempt = true
MobileShellComposite->>MobileShellComposite: pairingAttemptReachedMac = false
MobileShellComposite->>MobilePairingChecklist: beginPairingChecklist() → .connecting
MobileShellComposite-->>PairingView: pairingChecklist = .connecting
rect rgba(100, 150, 200, 0.5)
Note over MobileShellComposite,MobileCoreRPCClient: Transport connection loop
MobileShellComposite->>MobileCoreRPCClient: send RPC request
MobileCoreRPCClient->>MobileCoreRPCClient: ensureConnected() succeeds
MobileCoreRPCClient->>MobileCoreRPCClient: didAttemptSend = true
end
MobileShellComposite->>MobileCoreRPCClient: await didAttemptHostSend()
MobileCoreRPCClient-->>MobileShellComposite: true/false
MobileShellComposite->>MobileShellComposite: pairingAttemptReachedMac = true
alt Pairing succeeds
MobileShellComposite->>MobilePairingChecklist: markPairingChecklistConnected() → .connected
MobileShellComposite-->>PairingView: pairingChecklist = .connected
else Pairing fails
MobileShellComposite->>MobilePairingChecklist: resolvePairingChecklist(failureCategory)
MobileShellComposite-->>PairingView: pairingChecklist = resolved
end
PairingView->>PairingView: renders PairingChecklistRows per gate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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 |
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 `@Resources/Localizable.xcstrings`:
- Around line 198459-198463: The Japanese localization string value for the
trust-detail copy at the ja locale entry is grammatically incorrect and unclear.
Replace the current value "あなたの Mac か確認" with grammatically correct and clear
Japanese text that properly conveys the intended meaning of verifying or
checking the user's Mac. Ensure the corrected text follows proper Japanese
grammar and is production-ready for user-facing display.
🪄 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: 50addaf8-4f77-4773-8211-405af64faffa
📒 Files selected for processing (11)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingChecklistTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositeChecklistTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobilePairingChecklist.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePairingStage+Display.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/PairingChecklistView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/PairingView.swiftResources/Localizable.xcstringsios/cmux/Resources/Localizable.xcstrings
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "あなたの Mac か確認" | ||
| } |
There was a problem hiding this comment.
Fix Japanese trust-detail copy for readability.
Line 198462 ("あなたの Mac か確認") is grammatically awkward and likely user-confusing. Please correct the phrasing before release.
As per coding guidelines, user-facing localization text should be production-ready and clear.
Suggested patch
- "value": "あなたの Mac か確認"
+ "value": "あなたの Mac であることを確認"📝 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.
| "ja": { | |
| "stringUnit": { | |
| "state": "translated", | |
| "value": "あなたの Mac か確認" | |
| } | |
| "ja": { | |
| "stringUnit": { | |
| "state": "translated", | |
| "value": "あなたの Mac であることを確認" | |
| } |
🤖 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 `@Resources/Localizable.xcstrings` around lines 198459 - 198463, The Japanese
localization string value for the trust-detail copy at the ja locale entry is
grammatically incorrect and unclear. Replace the current value "あなたの Mac か確認"
with grammatically correct and clear Japanese text that properly conveys the
intended meaning of verifying or checking the user's Mac. Ensure the corrected
text follows proper Japanese grammar and is production-ready for user-facing
display.
Source: Coding guidelines
Greptile SummaryThis PR replaces the opaque "could not connect" failure in the iOS pairing sheet with a three-gate Network → Authentication → Trust checklist, where each gate individually resolves to pending, in-progress, succeeded, or failed with an inline actionable message.
Confidence Score: 4/5Safe to merge with the localization gap acknowledged; the new checklist logic, generation guards, and foreground/background gating are all correct and well-tested. The implementation is thorough — pure model types, generation-checked writes, per-client didAttemptSend reset, and end-to-end tests for every failure category. The one concrete gap is that the 11 new mobile.pairing.checklist.* strings in Resources/Localizable.xcstrings carry only en and ja translations while that catalog supports 20 locales. All pre-existing mobile.* strings in the same catalog follow the identical two-locale pattern, suggesting a deliberate scoping policy, but the policy is undocumented and new entries technically violate the full-internationalization rule. Resources/Localizable.xcstrings — new checklist entries are missing translations for 18 supported locales. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User taps Pair / scans QR] --> B[isForegroundPairingAttempt = true\nhasStartedPairing = true]
B --> C[beginPairingValidationAttempt\npairingAttemptReachedMac = false\npairingChecklist = .connecting]
C --> D{Transport\nensureConnected?}
D -- fails --> E[didAttemptSend = false\ndidReachHost = false]
E --> F[resolving category\nreachedMac: false]
F --> G[Network X\nAuth pending\nTrust pending]
D -- succeeds --> H[didAttemptSend = true\ndidReachHost = true\npairingAttemptReachedMac = true]
H --> I{RPC response}
I -- unauthorized / ticketExpired --> J[resolving authFailed\nreachedMac: true]
J --> K[Network OK\nAuth X\nTrust pending]
I -- account_mismatch --> L[resolving accountMismatch\nreachedMac: true]
L --> M[Network OK\nAuth OK\nTrust X]
I -- success --> N[markPairingChecklistConnected]
N --> O[Network OK\nAuth OK\nTrust OK]
P[Background reconnect] --> Q[isForegroundPairingAttempt = false\nbeginPairingChecklist to nil]
Q --> R[pairingChecklist = nil\nno checklist painted]
Reviews (8): Last reviewed commit: "Pairing checklist: a superseding backgro..." | Re-trigger Greptile |
| @ViewBuilder | ||
| private var statusIcon: some View { | ||
| switch status { | ||
| case .inProgress: | ||
| ProgressView() | ||
| .controlSize(.small) | ||
| default: | ||
| Image(systemName: status.symbolName) | ||
| .font(.title3) | ||
| .foregroundStyle(status.tintColor) | ||
| } | ||
| } |
There was a problem hiding this comment.
SF Symbol name leaks into VoiceOver combined label
children: .combine folds every child's accessibility representation — including the SF Symbol image — into the row's spoken label. For the "succeeded" state the combined label would include "checkmark circle fill" from Image(systemName: "checkmark.circle.fill"). The status is already conveyed via accessibilityValue, so the image adds noise. Adding .accessibilityHidden(true) to statusIcon (or .accessibilityLabel("")) and providing an explicit .accessibilityLabel("\(stage.title)") on the outer HStack would give VoiceOver a clean label like "Network — Not started" rather than "checkmark circle fill Network Reaching your Mac Succeeded".
| /// Set the first time the user starts a pairing attempt from this screen, so | ||
| /// the connection checklist appears only for an attempt the user initiated | ||
| /// here — never for a stale background reconnect's result. | ||
| @State private var hasStartedPairing = false | ||
| @State private var pairingTaskID: UUID? | ||
| @State private var pairingTask: Task<Void, Never>? |
There was a problem hiding this comment.
hasStartedPairing is never cleared within a single view instance
Once set to true, every subsequent instrumented pairing attempt — including background reconnects from other connections active in the same MobileShellComposite — can repaint the checklist while the user is still on this screen. The PR description says "a background reconnect never surfaces a stale checklist here", but the guard is only effective before the user's first tap on Pair; after that, showsChecklist stays true for the lifetime of the view instance. If a background reconnect fires while the user is editing host/port fields after a prior failure, the checklist would update from the reconnect attempt's result, potentially showing misleading intermediate "Network in progress" state unrelated to anything the user triggered.
| /// The checklist while an attempt is in flight: the network gate is being | ||
| /// attempted; the later gates wait their turn. | ||
| public static let connecting = MobilePairingChecklist( | ||
| network: .inProgress, | ||
| authentication: .pending, | ||
| trust: .pending | ||
| ) |
There was a problem hiding this comment.
connecting snapshot always shows only the network gate as in-progress
The connecting static holds network: .inProgress, authentication: .pending, trust: .pending for the entire duration of a pairing attempt. In practice, authentication and trust verification happen inside the same connect() call once the network is established, so the checklist never independently transitions the authentication or trust gate to .inProgress. For fast connections this is fine, but for a slow auth round-trip the user sees "Network" spinning indefinitely even once transport is clearly up. Consider whether the checklist design intentionally omits mid-attempt state updates, or whether this is a gap worth addressing in a follow-up.
Resolve conflicts from the iOS pairing version-warning + email-match work (#6028) landing on main alongside this branch's pairing checklist: - MobileShellComposite: keep both the checklist state/helpers and main's pairingVersionWarning state + emailFailure/versionWarning helpers; thread the pairing phase into the checklist resolver so a pre-network failure never shows a false cleared gate. - MobilePairingFailureCategory: map the new .emailMismatch case to the trust gate; resolving(_:reachedMac:) gates "prior gates cleared" on actually reaching the Mac. - PairingView / CMUXMobileRootView: keep both the version-warning section and the network/auth/trust checklist; match the merged memberwise-init argument order. - Tests: cover emailMismatch and the pre-network authFailed path; drive the composite checklist tests (and the shared makeConnectedStore helper) through the new "Continue anyway" version-warning approval so the scripted connect proceeds. - .github/swift-file-length-budget.tsv: regenerated for the merged tree via scripts/swift_file_length_budget.py --write-budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… reached Autoreview caught a false-success: `reachedMac` was inferred from the coarse pairing phase string, but `MobileCoreRPCClient` can throw `.authorizationFailed` /`.attachTicketExpired` locally (Stack token provider fails, or an attach token is expired) before any packet leaves the device. Those categories clear prior gates, so the network check mark went green for a purely local auth/session failure — actively misleading. Replace the phase guess with a real signal: `MobileCoreRPCSession` records `didAttemptSend` once a request reaches the transport-send stage (after its auth is built), exposed via `MobileCoreRPCClient.didAttemptHostSend()`. `connect()` records whether any client reached that stage into `pairingAttemptReachedMac` (reset per attempt in the shared `beginPairingValidationAttempt` funnel), and the checklist resolver uses it instead of the phase. A host-returned rejection still clears the network gate; a pre-send local failure leaves it untested. Adds a regression test (`preSendTokenFailureLeavesNetworkGateUntested`) for the exact scenario. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the budget TSV conflict (cross-PR churn) by regenerating it for the merged tree via scripts/swift_file_length_budget.py --write-budget. No code conflicts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nects Autoreview follow-up: `didAttemptSend` was flipped before `ensureConnected()`, so a route that failed to connect still reported a host send. With the per-attempt sticky `pairingAttemptReachedMac`, a multi-route ticket whose first route was unreachable could then green the network gate for a later local pre-send auth failure — the exact false positive the feature avoids. Move the flag to after `ensureConnected()` succeeds, so it reflects a genuinely connected channel. Add regression coverage: - CmuxMobileRPC: a connect-failing transport leaves `didAttemptHostSend()` false. - CmuxMobileShell: an unreachable first route followed by a pre-send token failure leaves the network gate untested (`.pending`), not cleared. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Autoreview follow-up: `connect()` awaited `client.didAttemptHostSend()` before re-checking the connection generation. On the `@MainActor` store that await can suspend while a newer attempt resets `pairingAttemptReachedMac`; the superseded attempt could then set it back to true, falsely greening the network gate for the new attempt's later local failure. Read the per-client signal into a local, re-check `isCurrentConnectionAttempt` after the await, and only write the shared flag when still current. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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)
2943-2962:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check the connection attempt after the store write.
isCurrentConnectionAttempt(generation)is only enforced beforeawait persistPairedMacFromTicket(ticket). If a new pairing or switch starts while that await is suspended, this stale success path still resumes and overwritesworkspaces,connectionState, and the live client/polling state for the newer attempt.💡 Suggested guard placement
clearPairingError() await persistPairedMacFromTicket(ticket) + guard isCurrentConnectionAttempt(generation), remoteClient === client else { + return nil + } applyRemoteWorkspaceList(response, preferActiveTicketTarget: workspaceListRequest.preferActiveTicketTarget) syncSelectedTerminalForWorkspace() connectionState = .connected🤖 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 2943 - 2962, The isCurrentConnectionAttempt(generation) validation only happens at the start of this connection handling block, but there is an await call to persistPairedMacFromTicket(ticket) that suspends execution, allowing a new pairing or connection switch to start before this stale attempt resumes. After the await in persistPairedMacFromTicket(ticket) returns, re-check isCurrentConnectionAttempt(generation) and return early if the generation is no longer current, preventing the stale attempt from overwriting workspaces, connectionState, client, and polling state that belong to the newer connection attempt.
3197-3403: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the pairing flow out of
MobileShellComposite.This change adds another full state machine here: QR validation, version-warning gating, attempt bookkeeping, checklist resolution, and analytics. In a production file that is already far past the repo’s Swift size boundary, that keeps networking, persistence, terminal transport, and UI/pairing state coupled in one place. Please move this pairing flow into a dedicated collaborator instead of extending the monolith further.
As per coding guidelines, "Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility."
🤖 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 3197 - 3403, Extract the pairing flow out of MobileShellComposite into a dedicated collaborator class to address the file size and separation of concerns issue. Move all pairing-related methods—beginPairingValidationAttempt, recordPairingSucceeded, recordPairingFailed, isCurrentPairingAttempt, invalidatePairingAttempt, applyPairingFailure, applyPairingValidationFailure, clearPairingError, beginPairingChecklist, resolvePairingChecklist, markPairingChecklistConnected, clearPairingChecklist, clearPairingVersionWarning, emailFailure, and versionWarning—along with their corresponding state properties (pairingAttemptID, pairingAttemptMethod, pairingAttemptStartedAt, pairingAttemptIsFirstPair, pairingAttemptReachedMac, pairingChecklist, pairingVersionWarning, pendingPairingVersionWarningURL) into a new PairingFlowManager or similar collaborator class. Update MobileShellComposite to instantiate and delegate pairing operations to this new class, removing the pairing state machine logic from the monolith.Source: Coding guidelines
🤖 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 3361-3365: In the MobileShellComposite file where the macUserID
mismatch check occurs (the guard statement checking actualUserID ==
expectedUserID), replace the return value of .authFailed with an appropriate
trust-stage or identity mismatch error that aligns with how .emailMismatch is
handled. This ensures that a macUserID mismatch (indicating a QR code bound to a
different account) is treated as an identity/trust problem rather than an
authentication failure, maintaining consistency with the stage contract for
identity and email failures.
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`:
- Around line 400-407: The dismissAddDeviceSheet() function in
CMUXMobileRootView is calling cancelPairing() when store.pairingVersionWarning
is not nil, but the cancel button in PairingView already calls cancelPairing()
before invoking the cancel callback that triggers dismissAddDeviceSheet(). This
results in cancelPairing() executing twice for a single user action. Remove the
redundant cancelPairing() call from the dismissAddDeviceSheet() function and
keep only the clearAttachTicketAuthenticationIfNeeded() logic, or determine what
other cleanup is needed for the pairingVersionWarning case without duplicating
the cancellation.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2943-2962: The isCurrentConnectionAttempt(generation) validation
only happens at the start of this connection handling block, but there is an
await call to persistPairedMacFromTicket(ticket) that suspends execution,
allowing a new pairing or connection switch to start before this stale attempt
resumes. After the await in persistPairedMacFromTicket(ticket) returns, re-check
isCurrentConnectionAttempt(generation) and return early if the generation is no
longer current, preventing the stale attempt from overwriting workspaces,
connectionState, client, and polling state that belong to the newer connection
attempt.
- Around line 3197-3403: Extract the pairing flow out of MobileShellComposite
into a dedicated collaborator class to address the file size and separation of
concerns issue. Move all pairing-related methods—beginPairingValidationAttempt,
recordPairingSucceeded, recordPairingFailed, isCurrentPairingAttempt,
invalidatePairingAttempt, applyPairingFailure, applyPairingValidationFailure,
clearPairingError, beginPairingChecklist, resolvePairingChecklist,
markPairingChecklistConnected, clearPairingChecklist,
clearPairingVersionWarning, emailFailure, and versionWarning—along with their
corresponding state properties (pairingAttemptID, pairingAttemptMethod,
pairingAttemptStartedAt, pairingAttemptIsFirstPair, pairingAttemptReachedMac,
pairingChecklist, pairingVersionWarning, pendingPairingVersionWarningURL) into a
new PairingFlowManager or similar collaborator class. Update
MobileShellComposite to instantiate and delegate pairing operations to this new
class, removing the pairing state machine logic from the monolith.
🪄 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: 600cdb6e-ab18-4265-aee3-248a23a8fbcc
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (12)
Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/TransportTestDoubles.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingChecklistTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositeChecklistTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/PairingView.swiftios/cmux/Resources/Localizable.xcstrings
💤 Files with no reviewable changes (1)
- ios/cmux/Resources/Localizable.xcstrings
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)
2943-2962:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check the connection attempt after the store write.
isCurrentConnectionAttempt(generation)is only enforced beforeawait persistPairedMacFromTicket(ticket). If a new pairing or switch starts while that await is suspended, this stale success path still resumes and overwritesworkspaces,connectionState, and the live client/polling state for the newer attempt.💡 Suggested guard placement
clearPairingError() await persistPairedMacFromTicket(ticket) + guard isCurrentConnectionAttempt(generation), remoteClient === client else { + return nil + } applyRemoteWorkspaceList(response, preferActiveTicketTarget: workspaceListRequest.preferActiveTicketTarget) syncSelectedTerminalForWorkspace() connectionState = .connected🤖 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 2943 - 2962, The isCurrentConnectionAttempt(generation) validation only happens at the start of this connection handling block, but there is an await call to persistPairedMacFromTicket(ticket) that suspends execution, allowing a new pairing or connection switch to start before this stale attempt resumes. After the await in persistPairedMacFromTicket(ticket) returns, re-check isCurrentConnectionAttempt(generation) and return early if the generation is no longer current, preventing the stale attempt from overwriting workspaces, connectionState, client, and polling state that belong to the newer connection attempt.
3197-3403: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the pairing flow out of
MobileShellComposite.This change adds another full state machine here: QR validation, version-warning gating, attempt bookkeeping, checklist resolution, and analytics. In a production file that is already far past the repo’s Swift size boundary, that keeps networking, persistence, terminal transport, and UI/pairing state coupled in one place. Please move this pairing flow into a dedicated collaborator instead of extending the monolith further.
As per coding guidelines, "Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility."
🤖 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 3197 - 3403, Extract the pairing flow out of MobileShellComposite into a dedicated collaborator class to address the file size and separation of concerns issue. Move all pairing-related methods—beginPairingValidationAttempt, recordPairingSucceeded, recordPairingFailed, isCurrentPairingAttempt, invalidatePairingAttempt, applyPairingFailure, applyPairingValidationFailure, clearPairingError, beginPairingChecklist, resolvePairingChecklist, markPairingChecklistConnected, clearPairingChecklist, clearPairingVersionWarning, emailFailure, and versionWarning—along with their corresponding state properties (pairingAttemptID, pairingAttemptMethod, pairingAttemptStartedAt, pairingAttemptIsFirstPair, pairingAttemptReachedMac, pairingChecklist, pairingVersionWarning, pendingPairingVersionWarningURL) into a new PairingFlowManager or similar collaborator class. Update MobileShellComposite to instantiate and delegate pairing operations to this new class, removing the pairing state machine logic from the monolith.Source: Coding guidelines
🤖 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 3361-3365: In the MobileShellComposite file where the macUserID
mismatch check occurs (the guard statement checking actualUserID ==
expectedUserID), replace the return value of .authFailed with an appropriate
trust-stage or identity mismatch error that aligns with how .emailMismatch is
handled. This ensures that a macUserID mismatch (indicating a QR code bound to a
different account) is treated as an identity/trust problem rather than an
authentication failure, maintaining consistency with the stage contract for
identity and email failures.
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`:
- Around line 400-407: The dismissAddDeviceSheet() function in
CMUXMobileRootView is calling cancelPairing() when store.pairingVersionWarning
is not nil, but the cancel button in PairingView already calls cancelPairing()
before invoking the cancel callback that triggers dismissAddDeviceSheet(). This
results in cancelPairing() executing twice for a single user action. Remove the
redundant cancelPairing() call from the dismissAddDeviceSheet() function and
keep only the clearAttachTicketAuthenticationIfNeeded() logic, or determine what
other cleanup is needed for the pairingVersionWarning case without duplicating
the cancellation.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2943-2962: The isCurrentConnectionAttempt(generation) validation
only happens at the start of this connection handling block, but there is an
await call to persistPairedMacFromTicket(ticket) that suspends execution,
allowing a new pairing or connection switch to start before this stale attempt
resumes. After the await in persistPairedMacFromTicket(ticket) returns, re-check
isCurrentConnectionAttempt(generation) and return early if the generation is no
longer current, preventing the stale attempt from overwriting workspaces,
connectionState, client, and polling state that belong to the newer connection
attempt.
- Around line 3197-3403: Extract the pairing flow out of MobileShellComposite
into a dedicated collaborator class to address the file size and separation of
concerns issue. Move all pairing-related methods—beginPairingValidationAttempt,
recordPairingSucceeded, recordPairingFailed, isCurrentPairingAttempt,
invalidatePairingAttempt, applyPairingFailure, applyPairingValidationFailure,
clearPairingError, beginPairingChecklist, resolvePairingChecklist,
markPairingChecklistConnected, clearPairingChecklist,
clearPairingVersionWarning, emailFailure, and versionWarning—along with their
corresponding state properties (pairingAttemptID, pairingAttemptMethod,
pairingAttemptStartedAt, pairingAttemptIsFirstPair, pairingAttemptReachedMac,
pairingChecklist, pairingVersionWarning, pendingPairingVersionWarningURL) into a
new PairingFlowManager or similar collaborator class. Update
MobileShellComposite to instantiate and delegate pairing operations to this new
class, removing the pairing state machine logic from the monolith.
🪄 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: 600cdb6e-ab18-4265-aee3-248a23a8fbcc
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (12)
Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/TransportTestDoubles.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingChecklistTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositeChecklistTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/PairingView.swiftios/cmux/Resources/Localizable.xcstrings
💤 Files with no reviewable changes (1)
- ios/cmux/Resources/Localizable.xcstrings
🛑 Comments failed to post (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
3361-3365:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn a trust-stage mismatch here, not
.authFailed.A
ticket.macUserIDmismatch is the same identity/trust problem as.emailMismatch, but this branch maps it to.authFailed. That will fail the authentication gate and show the wrong guidance for QR codes bound to a different account, which breaks the stage contract this PR introduces for identity/email failures.🤖 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 3361 - 3365, In the MobileShellComposite file where the macUserID mismatch check occurs (the guard statement checking actualUserID == expectedUserID), replace the return value of .authFailed with an appropriate trust-stage or identity mismatch error that aligns with how .emailMismatch is handled. This ensures that a macUserID mismatch (indicating a QR code bound to a different account) is treated as an identity/trust problem rather than an authentication failure, maintaining consistency with the stage contract for identity and email failures.Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift (1)
400-407:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid double-cancel side effects in the cancel callback chain.
Line 403 makes
dismissAddDeviceSheet()callcancelPairing()whenstore.pairingVersionWarning != nil, butPairingView’s cancel button already callscancelPairing()beforecancel()(Line 278 inPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/PairingView.swift). This can execute cancellation logic twice for one user action.Proposed fix
private func dismissAddDeviceSheet() { isShowingAddDeviceSheet = false - if store.pairingVersionWarning != nil { - cancelPairing() - } else { - clearAttachTicketAuthenticationIfNeeded() - } + clearAttachTicketAuthenticationIfNeeded() }🤖 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/CMUXMobileRootView.swift` around lines 400 - 407, The dismissAddDeviceSheet() function in CMUXMobileRootView is calling cancelPairing() when store.pairingVersionWarning is not nil, but the cancel button in PairingView already calls cancelPairing() before invoking the cancel callback that triggers dismissAddDeviceSheet(). This results in cancelPairing() executing twice for a single user action. Remove the redundant cancelPairing() call from the dismissAddDeviceSheet() function and keep only the clearAttachTicketAuthenticationIfNeeded() logic, or determine what other cleanup is needed for the pairingVersionWarning case without duplicating the cancellation.
Autoreview follow-up: the store painted `pairingChecklist` for every instrumented attempt, including background reconnects (`reconnectActiveMacIfAvailable`), host switches (`switchToMac`), and device-tree taps (`connectToRegistryInstance`), which all route through `connectManualHost`. Combined with the sheet's sticky `hasStartedPairing`, a background reconnect while the Add Device sheet stayed open could overwrite and render a checklist for an attempt the user never started. Split `connectManualHost` into the public foreground entry (the Pair button) and an internal `performConnectManualHost(isForegroundPairing:)`; mark QR/link pairing foreground too. Only foreground attempts publish the checklist (begin/resolve/ connected are gated on `isForegroundPairingAttempt`); the non-foreground callers pass `false`. Adds a regression test that a background reconnect leaves `pairingChecklist` nil. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…he Mac Autoreview follow-up: the foreground manual-host flow runs a pre-connect `mobile.attach_ticket.create` RPC before `connect(ticket:)`. A host rejection there (unauthorized / account mismatch) was resolved with `reachedMac: false`, so the checklist showed Network/Authentication untested even though the Mac was reached. Sample the probe client's `didAttemptHostSend()` (re-checking the connection generation after the await) and set `pairingAttemptReachedMac` so an auth/trust rejection from that probe clears the earlier gates. Adds a regression test driving the manual flow's attach-ticket probe to an account-mismatch rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address autoreview (Aziz) policy findings: split MobilePairingChecklist.swift into MobilePairingStage.swift, MobilePairingStageStatus.swift, and MobilePairingChecklist.swift (matching the package's one-type-per-file convention), and add the missing Swift-DocC comments to the checklist's public properties and initializer. (Co-located private SwiftUI sub-views like PairingChecklistRow and the shared test-doubles files are left as-is, consistent with existing patterns such as AddDeviceField in PairingView.swift and the multi-double TransportTestDoubles.swift.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1126-1160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear or reinitialize the checklist before manual-input validation returns.
On Lines 1134-1160, the invalid-host / invalid-port exits happen before
beginPairingAttempt(method: "manual"), so a prior non-nilpairingChecklistsurvives this new submission. BecausePairingViewrenders any stored checklist snapshot, the Add Device sheet can keep showing stale stage results for a fresh validation error.Suggested fix
func performConnectManualHost( name: String, host: String, port: Int, isForegroundPairing: Bool ) async { isForegroundPairingAttempt = isForegroundPairing + if isForegroundPairing { + clearPairingChecklist() + } let trimmedName = name.trimmingCharacters(in: .whitespacesAndNewlines) guard let normalizedHost = MobileShellRouteAuthPolicy.normalizedManualHost(host) else {🤖 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 1126 - 1160, In the performConnectManualHost function, the early return statements in both the invalid-host guard block (after normalizedManualHost check fails) and the invalid-port guard block (after port range check fails) do not clear the pairingChecklist. This allows stale checklist data from previous pairing attempts to persist and display in PairingView when new validation errors occur. Add code to clear or reinitialize the pairingChecklist property to nil before each of these two return statements, ensuring the UI does not show outdated stage results when validation fails on a fresh submission.
3239-3395: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the pairing-checklist state machine out of
MobileShellComposite.These additions add another attempt-lifecycle subsystem to a production type that is already far past the repo’s size limit. Moving the checklist / foreground-attempt bookkeeping into a focused collaborator or dedicated file would make the connection flow easier to reason about and test without expanding this 5k+ line class further.
As per coding guidelines, "Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility."
🤖 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 3239 - 3395, Extract the pairing-checklist state machine out of MobileShellComposite into a focused collaborator type to reduce the size of the already oversized class. Create a new dedicated type (either a struct or class) that encapsulates the checklist state (pairingChecklist, pairingAttemptReachedMac, isForegroundPairingAttempt) and owns the four checklist-related methods: beginPairingChecklist(), resolvePairingChecklist(), markPairingChecklistConnected(), and clearPairingChecklist(). Then refactor MobileShellComposite to instantiate and delegate to this new collaborator instead of managing the checklist logic directly, passing necessary dependencies like isForegroundPairingAttempt and pairingAttemptMethod through method parameters or initializer as needed.Source: Coding guidelines
🤖 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 2443-2467: The code currently sets pairingAttemptReachedMac to
true only in the catch block when the mobile.attach_ticket.create RPC fails (but
only if an attempt to send was made). However, a successful RPC call is equally
valid proof that the Mac was reached. Add pairingAttemptReachedMac = true in the
success path as well, after the do block completes successfully (after the try
await client.sendRequest call succeeds). This ensures that whether the RPC
succeeds or fails with a send attempt, the flag is properly recorded, preventing
resolvePairingChecklist from incorrectly treating the network gate as unproven.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1126-1160: In the performConnectManualHost function, the early
return statements in both the invalid-host guard block (after
normalizedManualHost check fails) and the invalid-port guard block (after port
range check fails) do not clear the pairingChecklist. This allows stale
checklist data from previous pairing attempts to persist and display in
PairingView when new validation errors occur. Add code to clear or reinitialize
the pairingChecklist property to nil before each of these two return statements,
ensuring the UI does not show outdated stage results when validation fails on a
fresh submission.
- Around line 3239-3395: Extract the pairing-checklist state machine out of
MobileShellComposite into a focused collaborator type to reduce the size of the
already oversized class. Create a new dedicated type (either a struct or class)
that encapsulates the checklist state (pairingChecklist,
pairingAttemptReachedMac, isForegroundPairingAttempt) and owns the four
checklist-related methods: beginPairingChecklist(), resolvePairingChecklist(),
markPairingChecklistConnected(), and clearPairingChecklist(). Then refactor
MobileShellComposite to instantiate and delegate to this new collaborator
instead of managing the checklist logic directly, passing necessary dependencies
like isForegroundPairingAttempt and pairingAttemptMethod through method
parameters or initializer as needed.
🪄 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: b7f3e639-135d-4805-9bae-cb715ba69533
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositeChecklistTests.swift
…c reached Autoreview follow-up: `requestManualAttachTicket` only recorded the reached-Mac signal on its catch path. A successful `mobile.attach_ticket.create` round trip also proves the Mac was reached, so if the subsequent `connect(ticket:)` failed locally before sending (e.g. the Stack token provider fails on the workspace-list request), the checklist showed Network untested. Set `pairingAttemptReachedMac` on the probe's success path too (with the same generation check). Adds a regression test driving a successful probe followed by a pre-send token failure in `connect`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Autoreview follow-up: the foreground/background gate was applied only when publishing, so a non-foreground attempt (background reconnect, host switch, device-tree tap) that superseded an in-flight Add Device attempt left the old `pairingChecklist` (e.g. a stuck `.connecting`) in the store. In PairingView a non-nil checklist hides `connectionError`, so the user could see a stale spinner with the real failure suppressed. Make the checklist attempt-scoped like `connectionError`: reset it at the start of every instrumented attempt — a foreground attempt starts the network gate, any other attempt clears it. Adds a regression test that a superseding background attempt clears a foreground checklist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes #6084.
The iOS pairing flow collapsed every failure into one opaque "could not connect", so a network problem was indistinguishable from an auth or trust problem. This adds a discrete network → authentication → trust checklist whose gates resolve individually — each with an in-progress, success, or failure state and an actionable message on failure.
What changed
MobilePairingChecklistmodel (CmuxMobileShellModel): three gates (network/authentication/trust), eachpending/inProgress/succeeded/failed(message, guidance), plus.connectingand.connectedsnapshots.MobilePairingFailureCategory.stage+clearsPriorGatesand a pureMobilePairingChecklist.resolving(_:)builder — the single projection from a classified failure to which check mark fails and which earlier gates the failure proves were cleared:authFailed,ticketExpired) → network ✓, auth ✗MobileShellCompositepaints the checklist through its existing failure/success choke points (begin/resolve/connected/clear), gated on an in-flight attempt.PairingViewrenders the checklist once the user starts a pairing attempt from the screen (so a background reconnect never surfaces a stale checklist); the failed gate carries the headline + guidance inline, replacing the single error banner for connection attempts.Localizable.xcstringscatalogs.Tests
category.stage,clearsPriorGates, andMobilePairingChecklist.resolving(_:)for every category.unauthorized→ auth ✗ (network ✓);account_mismatch→ trust ✗ (network ✓, auth ✓); success → all ✓.All
CmuxMobileShellpackage tests pass locally (162 tests). The model package and the two new standalone view files build withswift build; the full iOS UI/app build is validated by the iOS CI lanes (GhosttyKit is not built in this sandbox).How to verify on the iOS Simulator
MobilePairingChecklistRow.{network,authentication,trust}.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a three-step pairing checklist on iOS (Network → Authentication → Trust) with per-stage progress and inline guidance, shown only for foreground Add Device attempts. Network ✓ appears only when a request reached a connected transport (including the manual attach‑ticket probe), and a superseding background attempt now clears any stale checklist. Fixes #6084.
New Features
CmuxMobileShellModel(MobilePairingStage,MobilePairingStageStatus,MobilePairingChecklist) with.connecting/.connectedsnapshots and DocC on public members.CmuxMobileShell’sMobileShellCompositeacross begin/resolve/connected/clear;PairingViewshows it only after the user starts pairing and inlines the failed gate’s message + guidance. Localized strings (en, ja). Tests cover mapping and offline/auth/account/email mismatch and success.Bug Fixes
CmuxMobileRPC,MobileCoreRPCSessionsetsdidAttemptSendpost-ensureConnected(), exposed viaMobileCoreRPCClient.didAttemptHostSend(), andMobileShellCompositeuses it to gate Network ✓. Regression tests cover a connect‑failing transport and a multi‑route pre‑send token failure that leaves Network pending.mobile.attach_ticket.createprobe as reaching the Mac when it connects, for both host rejections and successful round trips, so later local failures still keep Network ✓. Regression tests added.Written for commit d27db55. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Localization
Tests