Repository navigation
Fix Settings sign-in by using the default browser - #4974
austinywang wants to merge 14 commits into
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:
📝 WalkthroughWalkthroughBrowser sign-in now uses per-attempt state, surfaces localized sign-in errors, adds a sign-in timeout, centralizes Settings button logic and error display, removes web URL scheme registration, extracts native handoff helpers, refactors after-sign-in logic, adds client-side native redirect, and introduces tests and project updates. ChangesAuth error surface and state tracking
Native handoff module and web refactor
Estimated code review effort: 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 (14 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Auth/AuthManager.swift (1)
743-743:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwift 6 concurrency warning: main actor isolated property accessed from nonisolated context.
The pipeline flagged a Swift 6 concurrency warning:
logTimestampFormatteris main actor-isolated but referenced from thenonisolated static func authLog. While not blocking, this may cause issues when strict concurrency checking is enabled.🔧 Possible fix
Mark
logTimestampFormatterasnonisolated:-private static let logTimestampFormatter: ISO8601DateFormatter = { +private nonisolated static let logTimestampFormatter: ISO8601DateFormatter = { let f = ISO8601DateFormatter() f.formatOptions = [.withInternetDateTime] return f }()ISO8601DateFormatter is thread-safe, so this annotation is safe.
🤖 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 `@Sources/Auth/AuthManager.swift` at line 743, The static authLog function references the main-actor isolated property logTimestampFormatter causing a Swift 6 concurrency warning; mark the static property logTimestampFormatter as nonisolated (or otherwise annotate it to be safely accessible from nonisolated contexts) so authLog can call Self.logTimestampFormatter.string(from:) without main-actor isolation conflicts; ensure the change is applied to the declaration of logTimestampFormatter (not authLog) and keep the formatter as an ISO8601DateFormatter since it is thread-safe.
🤖 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 `@CHANGELOG.md`:
- Around line 5-9: Update the Unreleased changelog entry under "Fixed" to remove
implementation details like "auth callback" and "native auth handoffs" and
instead describe user-visible behavior: state that Account sign-in now opens in
the user's default browser (replacing the embedded Safari sheet) and that
sign-in errors (e.g., timeouts) are now surfaced to users with localized error
messages; keep the entry concise and product-focused while referencing the
existing bullet "Open Settings > Account sign-in" to replace the current
technical phrasing.
In `@Sources/Auth/AuthManager.swift`:
- Around line 437-440: The defer block currently uses "guard let
browserSignInAttemptID else { return }" which is illegal because defer cannot
transfer control; change it to a conditional binding: inside the defer use "if
let browserSignInAttemptID = browserSignInAttemptID {
finishBrowserSignInAttempt(browserSignInAttemptID) }" (i.e., replace the guard
with an if-let and call finishBrowserSignInAttempt(browserSignInAttemptID) only
when non-nil).
In `@Sources/cmuxApp.swift`:
- Around line 8437-8439: The buttonIsDisabled computed property currently only
disables the button during session restore or when authenticated+loading,
leaving it enabled during sign-in loading and allowing duplicate beginSignIn()
calls; change the condition to disable whenever any auth operation is in-flight
by using authManager.isRestoringSession || authManager.isLoading (i.e., remove
the dependency on isAuthenticated) so the UI prevents additional beginSignIn()
invocations while loading.
- Around line 8388-8392: The code is rendering raw
lastSignInError?.localizedDescription which may expose internal/upstream
details; update the UI to use a sanitized, user-facing message provided by
AuthManager (e.g., expose a computed property like userFacingMessage or
userFacingErrorMessage on AuthManager) instead of localizedDescription; if such
a property doesn't exist, add a mapping in AuthManager that converts known error
types to localized, user-safe strings and returns a generic fallback for unknown
errors, then change the SwiftUI view to display authManager.userFacingMessage
(with the same font/color) rather than lastSignInError?.localizedDescription.
In `@web/app/handler/after-sign-in/page.tsx`:
- Around line 130-137: The isNativeReturnScheme(nativeReturnTo) check is
redundant because nativeCallbackHref is null for non-native schemes; remove that
guard from the if condition and simplify the block to rely on
shouldEmitNativeHandoff({ refreshToken, accessToken }) && nativeCallbackHref,
keeping the existing buildNativeHref(nativeCallbackHref, refreshToken,
accessCookie) call and the OpenNativeClient return path (functions/identifiers
to locate: shouldEmitNativeHandoff, isNativeReturnScheme, nativeCallbackHref,
buildNativeHref, OpenNativeClient).
- Around line 149-161: Remove the redundant fallback block that computes
fallbackReturnTo via nativeAuthCallbackForReturnTo and the subsequent if that
calls buildNativeHref and returns <OpenNativeClient ... />; the same
nativeAuthCallbackForReturnTo value is already used in the primary native
handoff path guarded by shouldEmitNativeHandoff and buildNativeHref, so delete
the fallbackReturnTo variable and the entire fallback if/return (references:
fallbackReturnTo, nativeAuthCallbackForReturnTo, shouldEmitNativeHandoff,
buildNativeHref, OpenNativeClient, accessCookie) to clarify control flow and
avoid dead code.
---
Outside diff comments:
In `@Sources/Auth/AuthManager.swift`:
- Line 743: The static authLog function references the main-actor isolated
property logTimestampFormatter causing a Swift 6 concurrency warning; mark the
static property logTimestampFormatter as nonisolated (or otherwise annotate it
to be safely accessible from nonisolated contexts) so authLog can call
Self.logTimestampFormatter.string(from:) without main-actor isolation conflicts;
ensure the change is applied to the declaration of logTimestampFormatter (not
authLog) and keep the formatter as an ISO8601DateFormatter since it is
thread-safe.
🪄 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: fcab30d0-21b1-478e-b0b3-183c4852a5a0
📒 Files selected for processing (13)
CHANGELOG.mdResources/Info.plistResources/Localizable.xcstringsSources/Auth/AuthCallbackRouter.swiftSources/Auth/AuthEnvironment.swiftSources/Auth/AuthManager.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AuthManagerExternalBrowserSignInTests.swiftweb/app/handler/after-sign-in/OpenNativeClient.tsxweb/app/handler/after-sign-in/native-handoff.tsweb/app/handler/after-sign-in/page.tsxweb/tests/native-handoff.test.ts
💤 Files with no reviewable changes (1)
- Resources/Info.plist
Greptile SummaryThis PR replaces the embedded
Confidence Score: 5/5Safe to merge — the changed files are a clean, well-scoped replacement of ASWebAuthenticationSession with NSWorkspace.open, and the continuation lifecycle in HostBrowserSignInFlow correctly handles the dropped completion through external-callback, timeout, and cancel paths. The new DefaultBrowserAuthSessionFactory is small and correctly satisfies the HostBrowserAuthSessionFactory contract for the external-callback architecture: start() propagates the opener's success/failure, cancel() is a documented no-op, and the dropped completion closure does not leak the withCheckedContinuation because HostBrowserSignInFlow's three other resumption paths (handleCallbackURL, timeout, cancelActiveAttempt) always cover it. Actor isolation is correct throughout. The Info.plist change is precise. No new issues were found in the diff; prior threads already captured the outstanding concerns in other parts of the PR. No files in this diff require special attention. The HostBrowserSignInFlow (not in this diff) contains the state-matching and race-guard logic that was already flagged in earlier review threads. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
actor User
participant Settings
participant HBSF as HostBrowserSignInFlow
participant Factory as DefaultBrowserAuthSessionFactory
participant Session as DefaultBrowserAuthSession
participant NSW as NSWorkspace
participant Browser as Default Browser
participant AppDelegate
User->>Settings: Sign In
Settings->>HBSF: beginSignIn()
HBSF->>Factory: makeSession(signInURL, completion_dropped)
Factory->>Session: init(signInURL, opener)
HBSF->>Session: start()
Session->>NSW: open(signInURL)
NSW->>Browser: launch sign-in URL
Browser->>User: sign-in page
User->>Browser: completes sign-in
Browser->>AppDelegate: "cmux://auth-callback?cmux_auth_state=UUID"
AppDelegate->>HBSF: handleCallbackURL
HBSF->>HBSF: validate state equals activeCallbackState
HBSF->>HBSF: seed tokens and completeExternalSignIn
HBSF->>Settings: "isSigningIn = false"
%%{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
actor User
participant Settings
participant HBSF as HostBrowserSignInFlow
participant Factory as DefaultBrowserAuthSessionFactory
participant Session as DefaultBrowserAuthSession
participant NSW as NSWorkspace
participant Browser as Default Browser
participant AppDelegate
User->>Settings: Sign In
Settings->>HBSF: beginSignIn()
HBSF->>Factory: makeSession(signInURL, completion_dropped)
Factory->>Session: init(signInURL, opener)
HBSF->>Session: start()
Session->>NSW: open(signInURL)
NSW->>Browser: launch sign-in URL
Browser->>User: sign-in page
User->>Browser: completes sign-in
Browser->>AppDelegate: "cmux://auth-callback?cmux_auth_state=UUID"
AppDelegate->>HBSF: handleCallbackURL
HBSF->>HBSF: validate state equals activeCallbackState
HBSF->>HBSF: seed tokens and completeExternalSignIn
HBSF->>Settings: "isSigningIn = false"
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
| }, | ||
| "settings.account.error.signInTimedOut": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Sign in timed out. Try again." | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "サインインがタイムアウトしました。もう一度お試しください。" | ||
| } | ||
| }, | ||
| "uk": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Час очікування входу минув. Спробуйте ще раз." | ||
| } | ||
| }, | ||
| "ko": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "로그인 시간이 초과되었습니다. 다시 시도하세요." | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "remote.state.connected.vmNoProxy": { | ||
| "extractionState": "manual", | ||
| "localizations": { |
There was a problem hiding this comment.
The new settings.account.error.signInTimedOut string is only translated into en, ja, uk, and ko. The catalog already supports ar, bs, da, de, es, fr, it, km, nb, pl, pt-BR, ru, th, tr, zh-Hans, and zh-Hant — users on any of those locales will see the raw English fallback "Sign in timed out. Try again." when the timeout fires. The settings.account.error.missingRefreshToken and settings.account.error.invalidCallback strings added in companion PRs each carry all 20 translations; this one is the odd one out.
Rule Used: Flag production user-facing text that is not fully... (source)
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!
| authLog("auth.browserSignIn begin url=\(Self.redactedURLDescription(signInURL))") | ||
| urlOpener(signInURL) | ||
| guard isCurrentBrowserSignInAttempt(attemptID) else { | ||
| return | ||
| } | ||
| session.presentationContextProvider = AuthPresentationContext.shared | ||
| session.prefersEphemeralWebBrowserSession = false | ||
| } |
There was a problem hiding this comment.
guard isCurrentBrowserSignInAttempt(attemptID) check after urlOpener(signInURL) is unreachable dead code. urlOpener is a synchronous (URL) -> Void closure; because beginSignIn() runs on the main actor, no other actor-isolated code can execute during the call, so activeBrowserSignInAttemptID cannot change while urlOpener is running. Whether the guard passes or fails, there is no following work — the function ends in both branches. Remove it to avoid misleading future readers.
| authLog("auth.browserSignIn begin url=\(Self.redactedURLDescription(signInURL))") | |
| urlOpener(signInURL) | |
| guard isCurrentBrowserSignInAttempt(attemptID) else { | |
| return | |
| } | |
| session.presentationContextProvider = AuthPresentationContext.shared | |
| session.prefersEphemeralWebBrowserSession = false | |
| } | |
| authLog("auth.browserSignIn begin url=\(Self.redactedURLDescription(signInURL))") | |
| urlOpener(signInURL) | |
| } |
| // Fallback: try native app only when we can preserve the requested native scheme. | ||
| const fallbackReturnTo = nativeAuthCallbackForReturnTo(nativeReturnTo); | ||
| if ( | ||
| shouldEmitNativeHandoff({ refreshToken, accessToken }) && | ||
| fallbackReturnTo | ||
| ) { | ||
| const fallback = buildNativeHref( | ||
| fallbackReturnTo, | ||
| refreshToken, | ||
| accessCookie, | ||
| ); |
There was a problem hiding this comment.
The fallback's conditions — shouldEmitNativeHandoff({ refreshToken, accessToken }) && fallbackReturnTo — are identical to the reduced conditions of the primary block above (lines 130–137). When both tokens are present and nativeReturnTo is a native scheme, the primary block's buildNativeHref call will always succeed (because accessCookie is guaranteed non-empty by the JSON.stringify([refreshToken, accessToken]) assignment on line 117 when both tokens are truthy). So the primary block always returns when its conditions hold, and the fallback's conditions can never be independently true. This block is dead code and can be removed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/AuthManagerExternalBrowserSignInTests.swift (1)
31-33: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider moving
signOut()to cleanup rather than test body.Calling
signOut()at Line 33 before the assertions (Lines 35-45) is unusual ordering. If this is cleanup, usedeferor a test teardown phase; if it's part of the test flow, add assertions to verify sign-out behavior.🤖 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 `@cmuxTests/AuthManagerExternalBrowserSignInTests.swift` around lines 31 - 33, The call to manager.signOut() is placed before the post-beginSignIn assertions, which mixes cleanup with test verification; either move the signOut() call into test teardown (use defer { try? await manager.signOut() } or the test class tearDown/async tearDown) to ensure assertions about manager.isLoading and related state run unmodified, or keep signOut() in the flow but add explicit assertions verifying sign-out behavior (e.g., that isAuthenticated == false) after calling manager.signOut(); locate the calls around manager.beginSignIn(), manager.isLoading and manager.signOut() in AuthManagerExternalBrowserSignInTests.swift to implement the change.Sources/Auth/AuthManager.swift (1)
33-37:⚠️ Potential issue | 🟠 MajorAdd missing localized strings for
settings.account.error.signInTimedOutfor all supported locales.
Resources/Localizable.xcstringsonly provides this key foren,ja,ko, anduk; missing:ar,bs,da,de,es,fr,it,km,no,pl,pt-BR,ru,th,tr,zh-CN,zh-TW.🤖 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 `@Sources/Auth/AuthManager.swift` around lines 33 - 37, The localized string key "settings.account.error.signInTimedOut" used in the AuthManager.swift case .signInTimedOut is only present for en/ja/ko/uk; add this key with appropriate translated values to Resources/Localizable.xcstrings for the missing locales (ar, bs, da, de, es, fr, it, km, no, pl, pt-BR, ru, th, tr, zh-CN, zh-TW). Ensure each entry uses the same key exactly ("settings.account.error.signInTimedOut") and follows the existing .xcstrings format and context/notes conventions so String(localized:) resolves correctly at runtime. Validate by building and testing that String(localized: "settings.account.error.signInTimedOut", defaultValue: ...) returns the localized text for each locale.
🤖 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.
Outside diff comments:
In `@cmuxTests/AuthManagerExternalBrowserSignInTests.swift`:
- Around line 31-33: The call to manager.signOut() is placed before the
post-beginSignIn assertions, which mixes cleanup with test verification; either
move the signOut() call into test teardown (use defer { try? await
manager.signOut() } or the test class tearDown/async tearDown) to ensure
assertions about manager.isLoading and related state run unmodified, or keep
signOut() in the flow but add explicit assertions verifying sign-out behavior
(e.g., that isAuthenticated == false) after calling manager.signOut(); locate
the calls around manager.beginSignIn(), manager.isLoading and manager.signOut()
in AuthManagerExternalBrowserSignInTests.swift to implement the change.
In `@Sources/Auth/AuthManager.swift`:
- Around line 33-37: The localized string key
"settings.account.error.signInTimedOut" used in the AuthManager.swift case
.signInTimedOut is only present for en/ja/ko/uk; add this key with appropriate
translated values to Resources/Localizable.xcstrings for the missing locales
(ar, bs, da, de, es, fr, it, km, no, pl, pt-BR, ru, th, tr, zh-CN, zh-TW).
Ensure each entry uses the same key exactly
("settings.account.error.signInTimedOut") and follows the existing .xcstrings
format and context/notes conventions so String(localized:) resolves correctly at
runtime. Validate by building and testing that String(localized:
"settings.account.error.signInTimedOut", defaultValue: ...) returns the
localized text for each locale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 860c0204-7f7e-41e7-ab88-0c56e0db8691
📒 Files selected for processing (2)
Sources/Auth/AuthManager.swiftcmuxTests/AuthManagerExternalBrowserSignInTests.swift
| if let activeState = activeBrowserSignInAttemptState, | ||
| callbackState != activeState { | ||
| authLog("auth.browserSignIn ignored stale callback state=\(callbackState ?? "nil")") | ||
| return | ||
| } |
There was a problem hiding this comment.
activeBrowserSignInAttemptState is non-nil — if the sign-in attempt already finished (or timed out) and then the user signed out without a pending attempt, cancelBrowserSignInForSignOut records nothing, both activeBrowserSignInAttemptState and signOutCancelledBrowserSignInAttemptState are nil, and a stale callback with the original state UUID re-authenticates the user. Replacing the if let opt-in with an unconditional guard equality check closes the gap: nil == nil still passes legacy stateless callbacks while "uuid-old" == nil is rejected.
| if let activeState = activeBrowserSignInAttemptState, | |
| callbackState != activeState { | |
| authLog("auth.browserSignIn ignored stale callback state=\(callbackState ?? "nil")") | |
| return | |
| } | |
| guard callbackState == activeBrowserSignInAttemptState else { | |
| authLog("auth.browserSignIn ignored stale callback state=\(callbackState ?? "nil") activeState=\(activeBrowserSignInAttemptState ?? "nil")") | |
| return | |
| } |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Auth/AuthManager.swift (1)
431-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire active state to be non-nil before matching.
The guard
callbackState == activeBrowserSignInAttemptStatepasses when both arenil, allowing a callback (possibly from a pre-state-tracking flow or malformed URL) to be processed when no sign-in attempt is active. The intent is to ignore callbacks that don't match an active attempt's state.🔧 Proposed fix
- guard callbackState == activeBrowserSignInAttemptState else { + guard let activeState = activeBrowserSignInAttemptState, + callbackState == activeState else { authLog("auth.browserSignIn ignored stale callback state=\(callbackState ?? "nil") activeState=\(activeBrowserSignInAttemptState ?? "nil")") 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 `@Sources/Auth/AuthManager.swift` around lines 431 - 434, The guard currently allows both callbackState and activeBrowserSignInAttemptState to be nil and thus proceeds; update the condition in the AuthManager logic so that it requires an activeBrowserSignInAttemptState to be non-nil before matching (e.g., check activeBrowserSignInAttemptState != nil && callbackState == activeBrowserSignInAttemptState) so stale or unsolicited callbacks are ignored; locate the check that uses callbackState and activeBrowserSignInAttemptState in AuthManager.swift and modify that guard accordingly, keeping the existing authLog and return behavior when the guard fails.
🤖 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.
Outside diff comments:
In `@Sources/Auth/AuthManager.swift`:
- Around line 431-434: The guard currently allows both callbackState and
activeBrowserSignInAttemptState to be nil and thus proceeds; update the
condition in the AuthManager logic so that it requires an
activeBrowserSignInAttemptState to be non-nil before matching (e.g., check
activeBrowserSignInAttemptState != nil && callbackState ==
activeBrowserSignInAttemptState) so stale or unsolicited callbacks are ignored;
locate the check that uses callbackState and activeBrowserSignInAttemptState in
AuthManager.swift and modify that guard accordingly, keeping the existing
authLog and return behavior when the guard fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2f4bfa7-ddca-4fb2-8312-4dbb93902087
📒 Files selected for processing (1)
Sources/Auth/AuthManager.swift
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 386039a. Configure here.
|
What's the status on this getting merged? |
# Conflicts: # CHANGELOG.md # Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/BrowserSignIn/AuthCallbackRouter.swift # Sources/Auth/AuthEnvironment.swift # Sources/Auth/AuthManager.swift # Sources/cmuxApp.swift # cmux.xcodeproj/project.pbxproj # cmuxTests/SocketControlPasswordStoreTests.swift # web/app/handler/after-sign-in/OpenNativeClient.tsx # web/app/handler/after-sign-in/page.tsx
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
4 participants
Closes #3617.
Summary
This consolidates the durable pieces of the existing issue-3617 work and changes the Settings sign-in architecture from an embedded
ASWebAuthenticationSessionsheet to the system default browser plus cmux's existing native auth callback.AuthEnvironment.signInURL()throughNSWorkspace, so Arc/Chrome/Firefox/etc. are honored instead of always using Safari's AuthenticationServices web stack.state; stale callbacks from an older or signed-out attempt are ignored instead of reauthenticating unexpectedly.http/httpsURL handler, matching the LaunchServices loop fix from Drop vestigial http/https URL handler so ASWebAuthenticationSession works against http://localhost #3650.cmux*://auth-callbackreturn while keeping the manual Return to cmux link as a fallback.Why this architecture
I weighed the two options called out in the issue:
ASWebAuthenticationSessionplus Drop vestigial http/https URL handler so ASWebAuthenticationSession works against http://localhost #3650/Require valid accessToken for native app handoff to prevent auth crashes #3634/Surface Sign In errors in Account settings #3624 would address known failure modes, but it still cannot honor the user's default browser. Apple's auth session uses Safari's web stack by design, so the paid-user Arc expectation would remain unmet.This PR chooses the system-browser architecture. It also keeps the LaunchServices and token-shape fixes because they are still valid defense-in-depth for callback reliability.
Related work credited
http/httpsLaunchServices registration loop.Paid-user reproduction folded in
A paid user reported Arc as their macOS default browser, but cmux Settings sign-in opened Safari and then auto-closed before sign-in. They completed sign-in only by capturing the auth URL from LaunchServices logs and pasting it into Arc manually. This PR makes that workaround the normal path: cmux opens the auth URL in the default browser directly, then completes via the native callback.
Regression coverage
The first commit adds a failing Swift Testing regression for the new invariant:
AuthManager.beginSignIn()must hand the generated sign-in URL to the injected browser opener. That fails on the old embedded-session implementation and passes with this change.This PR also adds Bun coverage for the web native-handoff guard: native schemes are recognized, callback targets are coerced to
auth-callback, callback state is preserved, and empty access tokens do not emit native handoffs.Verification
Local static checks only, per repo instruction not to run local tests or builds:
python3 scripts/normalize-pbxproj.py --check cmux.xcodeproj/project.pbxprojpython3 -m json.tool Resources/Localizable.xcstrings >/dev/nullgit diff --checkI did not run
./scripts/reload.sh,xcodebuild, or local tests. I did not use cloud-mac because real sign-in completion needs live credentials; the fix is verified by code-path ownership and regression coverage rather than a credentialed recording.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
High Risk
Changes authentication UX and callback validation (token handoff, state matching, URL handler registration); mistakes could block sign-in or accept wrong callbacks.
Overview
Settings account sign-in no longer uses an embedded
ASWebAuthenticationSessionsheet. It opens the Stack sign-in URL in the system default browser (viaNSWorkspace), then completes when the app receives acmux*://auth-callbackdeep link.Each attempt gets a unique
stateon the callback URL;AuthManageronly accepts callbacks that match the active attempt, and ignores stale, sign-out-cancelled, or stateless callbacks. Sign-in can time out (default 5 minutes) with a new localized “Sign in timed out” message shown in Settings.Plumbing:
Info.plistdropshttp/httpsURL-handler registration so LaunchServices is less likely to route auth URLs back into cmux. The web after-sign-in handler only deep-links when both tokens are present, normalizes native return URLs toauth-callback, preservesstate, and auto-redirects viaOpenNativeClient.Tests: Swift tests for external-browser sign-in and timeout/stale callbacks; Bun tests for native handoff helpers; existing sign-out race tests updated for
state.Reviewed by Cursor Bugbot for commit d3a5769. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Settings sign-in now opens in the default browser and completes via cmux*://auth-callback. Each attempt uses a unique state, times out after 5 minutes with a localized error in Settings, and ignores stale, stateless, or cancelled callbacks (fixes #3617).
NSWorkspacewith a newDefaultBrowserAuthSessionFactory(wired inMacAuthComposition); build the URL withAuthEnvironment.signInURL(callbackState:); keep loading scoped to the active attempt; showlastSignInErroron timeout/settle.state; ignore stateless callbacks, stale callbacks (including after timeout), and callbacks from sign‑out–cancelled attempts; reject idle stateless callbacks.http/httpsURL handler fromInfo.plistand defensively avoid launching cmux as the browser to prevent LaunchServices loops..../auth-callback, preservesstate, and auto‑redirects viaOpenNativeClient; tests addDefaultBrowserAuthSessionFactoryTests(success/failure) and cover external‑browser opener, timeouts, and stale/stateless/cancelled callbacks.Written for commit 844a117. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
UI
Localization
New Features
Tests