Repository navigation
Fix native pairing sign-in failures - #6457
Merged
Merged
Conversation
|
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 ChangesAuth Failure State Tracking and Pairing UI
Sequence Diagram(s)sequenceDiagram
participant MobilePairingView
participant HostBrowserSignInFlow
participant HostBrowserSession
participant Callback Handler
MobilePairingView->>HostBrowserSignInFlow: startAttempt()
Note over HostBrowserSignInFlow: lastFailure = nil, isSigningIn = true
HostBrowserSignInFlow->>HostBrowserSession: creates browser session
alt Callback state mismatch
Callback Handler->>HostBrowserSignInFlow: handleCallbackURL(mismatched state)
HostBrowserSignInFlow->>HostBrowserSignInFlow: lastFailure = .invalidCallback
HostBrowserSignInFlow->>MobilePairingView: publish lastFailure change
MobilePairingView->>MobilePairingView: show orange error caption
else Timeout
HostBrowserSignInFlow->>HostBrowserSignInFlow: browserAttemptTimeout fires
HostBrowserSignInFlow->>HostBrowserSignInFlow: lastFailure = .timedOut
HostBrowserSignInFlow->>HostBrowserSession: cancel session
else Slow signing in
MobilePairingView->>MobilePairingView: signInIsSlow = true
MobilePairingView->>MobilePairingView: show slowSignInFallback
MobilePairingView->>HostBrowserSignInFlow: openBrowserFallback(activeAttemptSignInURL)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (22 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Greptile SummaryThis PR fixes native pairing sign-in failures by replacing the opaque
Confidence Score: 5/5Safe to merge. The failure classification, lastFailure propagation, and generation guards are all correct; i18n is complete across every supported locale. The typed result enum replaces a stringly-absent URL?, every lastFailure write is guarded by the existing sign-out generation check, the AsyncStream-based deadline helper is race-free under cooperative MainActor scheduling, and the previous thread's generation-guard fix is present in the catch branch. New regression tests cover all the introduced failure paths. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as MobilePairingView
participant Flow as HostBrowserSignInFlow
participant Factory as ASWebBrowserAuthSessionFactory
participant OS as ASWebAuthenticationSession
participant Coord as AuthCoordinator
UI->>Flow: beginSignIn() / signIn(timeout:)
Flow->>Flow: "lastFailure = nil"
Flow->>Factory: makeSession(signInURL:callbackScheme:)
Factory->>OS: session.start()
OS-->>Factory: completion(URL?, Error?) [off-main]
Factory->>Factory: sessionResult(callbackURL:error:)
Note over Factory: Maps to .callback / .cancelled / .failed
Factory-->>Flow: "@MainActor (HostBrowserAuthSessionResult)"
alt .callback(url)
Flow->>Coord: completeExternalSignIn()
Coord-->>Flow: success
Flow->>Flow: "lastFailure = nil"
Flow-->>UI: "isSigningIn = false (authenticated)"
else .cancelled
Flow-->>UI: "isSigningIn = false (no failure recorded)"
else .failed(reason)
Flow->>Flow: "lastFailure = .browserSignInFailed(reason)"
Flow-->>UI: "isSigningIn = false"
UI->>UI: show lastFailure in signedOut prompt
end
alt External callback URL arrives
UI->>Flow: handleCallbackURL(url)
alt state matches active attempt
Flow->>Coord: completeExternalSignIn()
Flow->>Flow: "lastFailure = nil"
else state mismatch
Flow->>Flow: "lastFailure = .invalidCallback"
end
end
alt Attempt timeout fires
Flow->>Flow: "lastFailure = .timedOut"
Flow->>Flow: cancelActiveAttempt()
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as MobilePairingView
participant Flow as HostBrowserSignInFlow
participant Factory as ASWebBrowserAuthSessionFactory
participant OS as ASWebAuthenticationSession
participant Coord as AuthCoordinator
UI->>Flow: beginSignIn() / signIn(timeout:)
Flow->>Flow: "lastFailure = nil"
Flow->>Factory: makeSession(signInURL:callbackScheme:)
Factory->>OS: session.start()
OS-->>Factory: completion(URL?, Error?) [off-main]
Factory->>Factory: sessionResult(callbackURL:error:)
Note over Factory: Maps to .callback / .cancelled / .failed
Factory-->>Flow: "@MainActor (HostBrowserAuthSessionResult)"
alt .callback(url)
Flow->>Coord: completeExternalSignIn()
Coord-->>Flow: success
Flow->>Flow: "lastFailure = nil"
Flow-->>UI: "isSigningIn = false (authenticated)"
else .cancelled
Flow-->>UI: "isSigningIn = false (no failure recorded)"
else .failed(reason)
Flow->>Flow: "lastFailure = .browserSignInFailed(reason)"
Flow-->>UI: "isSigningIn = false"
UI->>UI: show lastFailure in signedOut prompt
end
alt External callback URL arrives
UI->>Flow: handleCallbackURL(url)
alt state matches active attempt
Flow->>Coord: completeExternalSignIn()
Flow->>Flow: "lastFailure = nil"
else state mismatch
Flow->>Flow: "lastFailure = .invalidCallback"
end
end
alt Attempt timeout fires
Flow->>Flow: "lastFailure = .timedOut"
Flow->>Flow: cancelActiveAttempt()
end
Reviews (13): Last reviewed commit: "Localize browser sign-in failure message" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 `@Resources/Localizable.xcstrings`:
- Around line 19406-19439: Remove the incorrect CLI surface error keys
(cli.claude-hook.error.invalidSurface and cli.claude-hook.error.surfaceNotFound)
from the localization strings file. Instead, add the two required keys that are
actually referenced in the code: auth.error.invalid_callback (needed by
AuthError.swift for the invalidCallback case) and
mobile.pairing.signIn.connecting (needed by MobilePairingView for the connecting
state). Each key must include both English (en) and Japanese (ja) translations
with appropriate string values for their respective use cases.
🪄 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: ca2af455-1aba-4a88-a9a3-57efefa90be7
📒 Files selected for processing (1)
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: 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 19406-19439: Remove the incorrect CLI surface error keys
(cli.claude-hook.error.invalidSurface and cli.claude-hook.error.surfaceNotFound)
from the localization strings file. Instead, add the two required keys that are
actually referenced in the code: auth.error.invalid_callback (needed by
AuthError.swift for the invalidCallback case) and
mobile.pairing.signIn.connecting (needed by MobilePairingView for the connecting
state). Each key must include both English (en) and Japanese (ja) translations
with appropriate string values for their respective use cases.
🪄 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: ca2af455-1aba-4a88-a9a3-57efefa90be7
📒 Files selected for processing (1)
Resources/Localizable.xcstrings
🛑 Comments failed to post (1)
Resources/Localizable.xcstrings (1)
19406-19439:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical inconsistency: Wrong localization keys added.
The PR objectives, AI summary, and code snippets (AuthError.swift, HostBrowserSignInFlow.swift) all indicate this layer should add:
auth.error.invalid_callback(for AuthError.invalidCallback case)mobile.pairing.signIn.connecting(for MobilePairingView connecting state)Instead, this diff adds unrelated CLI surface error keys:
cli.claude-hook.error.invalidSurfacecli.claude-hook.error.surfaceNotFoundAuthError.swift expects
"auth.error.invalid_callback"to exist (see context snippet 1, line 62), but it's not present in this diff. This will cause runtime localization failures when the auth error is displayed.Please verify the correct diff was provided, or add the expected
auth.error.invalid_callbackandmobile.pairing.signIn.connectingkeys with en+ja translations. Based on learnings, onlyenandjalocales are required for new keys.🤖 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 19406 - 19439, Remove the incorrect CLI surface error keys (cli.claude-hook.error.invalidSurface and cli.claude-hook.error.surfaceNotFound) from the localization strings file. Instead, add the two required keys that are actually referenced in the code: auth.error.invalid_callback (needed by AuthError.swift for the invalidCallback case) and mobile.pairing.signIn.connecting (needed by MobilePairingView for the connecting state). Each key must include both English (en) and Japanese (ja) translations with appropriate string values for their respective use cases.
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 (1)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/BrowserSignIn/HostBrowserSignInFlow.swift (1)
165-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly consume fallback state after the callback succeeds.
Line 169 clears
pendingFallbackCallbackStatebeforecompleteCallbackvalidates the payload and publishes the session. If that completion fails transiently, the issued fallback state is burned and a retry of the same browser callback is rejected. Also clearlastFailureon success so a prior rejected callback in the same attempt does not leave stale failure UI state.Proposed fix
if callbackRouter.isAuthCallbackURL(url), let state = authCallbackState(from: url), state == pendingFallbackCallbackState { log.log("auth.callback.external.routeToIssuedFallback") - pendingFallbackCallbackState = nil return await completeCallback(url: url, attemptID: nil, acceptedExternalState: state) } @@ if authCallbackState(from: url) == pendingFallbackCallbackState { pendingFallbackCallbackState = nil } + lastFailure = nil return trueAlso applies to: 464-467
🤖 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/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/BrowserSignIn/HostBrowserSignInFlow.swift` around lines 165 - 170, The pendingFallbackCallbackState is being cleared before completeCallback validates and succeeds, which causes the state to be consumed even if the callback completion fails transiently. Move the statement that sets pendingFallbackCallbackState to nil to only execute after completeCallback succeeds. Additionally, clear lastFailure when the callback completes successfully to prevent stale failure UI state from persisting after a prior rejected callback in the same attempt. Apply this same fix pattern to the similar callback handling code also mentioned in the review (the other location around lines 464-467).
🤖 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/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/HostBrowserSignInFlowTestSupport.swift`:
- Around line 71-85: The waitForHostBrowserSession and
waitForHostBrowserCondition functions use unbounded while loops that can cause
tests to hang indefinitely if conditions never become true. Add an explicit
deadline or timeout to both functions by tracking elapsed time or using a
deadline API, and throw a clear timeout error when the deadline is exceeded
instead of continuing to poll indefinitely. This ensures tests fail fast with
meaningful error messages rather than hanging.
---
Outside diff comments:
In
`@Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/BrowserSignIn/HostBrowserSignInFlow.swift`:
- Around line 165-170: The pendingFallbackCallbackState is being cleared before
completeCallback validates and succeeds, which causes the state to be consumed
even if the callback completion fails transiently. Move the statement that sets
pendingFallbackCallbackState to nil to only execute after completeCallback
succeeds. Additionally, clear lastFailure when the callback completes
successfully to prevent stale failure UI state from persisting after a prior
rejected callback in the same attempt. Apply this same fix pattern to the
similar callback handling code also mentioned in the review (the other location
around lines 464-467).
🪄 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: 952f6c29-b3ce-4b1c-a57a-31975737e91b
📒 Files selected for processing (5)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/BrowserSignIn/HostBrowserSignInFlow.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/BrowserSignIn/HostBrowserSignInFlowHelpers.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/HostBrowserSignInFlowFailureTests.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/HostBrowserSignInFlowTestSupport.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/HostBrowserSignInFlowTests.swift
…uth-hang # Conflicts: # .github/swift-file-length-budget.tsv
azooz2003-bit
force-pushed
the
task-connect-device-auth-hang
branch
from
June 20, 2026 03:46
dc6e941 to
d544669
Compare
azooz2003-bit
force-pushed
the
task-connect-device-auth-hang
branch
from
June 20, 2026 03:50
d544669 to
ba5768b
Compare
This was referenced Jun 24, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Diagnosis
Testing
Localization
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes native pairing sign‑in failures by classifying browser session outcomes, surfacing safe errors in the UI, and adding a reliable “Connecting…” flow with a default‑browser fallback. Adds
HostBrowserAuthSessionResult, newAuthErrorcases, localized messages (including a generic browser sign‑in failure), and stronger guards around external callbacks and state.Bug Fixes
ASWebAuthenticationSessioncompletions intoHostBrowserAuthSessionResult(.callback/.cancelled/.failed), mapping OS/network/start errors; treat external callbacks during an active attempt as a cancellation.lastFailure(.invalidCallback,.browserSignInFailed(_),.timedOut); clear on new attempts, success, or sign‑out..invalidCallback; map unsafe validation errors to.serverError(0, "auth_failed").cmux-dev-<tag>.New Features
lastFailureon the signed‑out prompt; add a Connecting state with a slow‑sign‑in hint and an “Open in Browser” fallback that reuses the active attempt’s state.AuthError.invalidCallbackandAuthError.browserSignInFailed(_)with generic, localized UI messages on macOS/iOS.Written for commit 8245509. Summary will update on new commits.
Summary by CodeRabbit