Repository navigation
Surface Sign In errors in Account settings - #3624
Closed
austinywang wants to merge 51 commits into
Closed
austinywang wants to merge 51 commits into
austinywang wants to merge 51 commits into
Conversation
Settings sign-in errors currently disappear into logs, so this adds a behavioral regression test for invalid callback handling before the production error surface exists. Constraint: Do not run local tests for this repository; CI owns test execution. Confidence: high Scope-risk: narrow Tested: Not run locally by instruction. Not-tested: Local cmux-unit execution.
Settings auth previously converted ASWebAuthenticationSession and callback failures into log lines only, so users saw the web sheet disappear without any actionable state. AuthManager now stores a typed visible error, keeps in-flight web sessions from being cancelled by duplicate entry, and clears stale errors on successful sign-in paths. Constraint: Settings strings must use String(localized:) and Localizable.xcstrings entries. Constraint: Local tests are intentionally not run in this repo; CI owns test execution. Rejected: Keep using NSLog-only failures | leaves issue 3617 with no visible user feedback. Confidence: medium Scope-risk: narrow Tested: git diff --check; python3 -m json.tool Resources/Localizable.xcstrings Not-tested: Local unit/UI tests by instruction.
|
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 user-visible sign-in error surfacing: extracts auth errors to a new errors file, exposes ChangesSign-In Error Visibility & Settings UI
Sequence Diagram(s)sequenceDiagram
participant User as "User / Settings UI"
participant UI as "AuthSettingsRow"
participant AM as "AuthManager"
participant AS as "ASWebAuthenticationSession"
participant TS as "TokenStore"
User->>UI: Tap "Sign In…"
UI->>AM: beginSignIn()
AM->>AS: create & start session (auth URL)
AS-->>AM: completion(callbackURL?, error?)
alt error (canceled or other)
AM->>AM: shouldSuppressWebAuthError? -> log & return OR
AM->>AM: set lastSignInError = .message(...) or .authManager(...)
AM-->>UI: published lastSignInError
UI-->>User: show localized error
else callbackURL present
AM->>AM: handleCallbackURL(callbackURL)
AM->>TS: seed/refresh tokens
TS-->>AM: tokens
AM->>UI: publish authenticated state (lastSignInError = nil)
UI-->>User: update to signed-in state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 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 |
The workflow guard rejects growth in already-large Swift files, so the new sign-in error model and account settings row live in focused sub-500-line files instead of increasing AuthManager.swift and cmuxApp.swift. Constraint: CI enforces .github/swift-file-length-budget.tsv. Rejected: Refresh the Swift length budget | this change can stay within the existing budget by splitting focused code. Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: git diff --check Tested: python3 -m json.tool Resources/Localizable.xcstrings Not-tested: Local unit/UI tests by instruction.
Contributor
Greptile SummaryThis PR surfaces sign-in failures in the Settings > Account row by adding
Confidence Score: 5/5Safe to merge. Auth state transitions are correctly guarded by mutation generations, token cleanup is handled by keepAuthMutationIfCurrent, and all previous review findings have been addressed. All seven previously flagged issues have been resolved. The mutation-generation approach correctly handles concurrent sign-out and timeout cases, confirmed by the new test suite. Localization is now comprehensive across all 19 supported locales. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as AuthSettingsRow
participant AM as AuthManager
participant TS as TokenStore
Note over UI,TS: Browser sign-in happy path
UI->>AM: beginSignIn()
AM->>AM: "lastSignInError = nil, beginAuthMutation(G1)"
AM->>AM: ASWebAuthenticationSession.start()
AM-->>UI: "isLoading = true"
AM->>AM: handleCallbackURL(url)
AM->>TS: setTokens(access, refresh)
AM->>AM: keepAuthMutationIfCurrent(G1) → true
AM-->>UI: "isAuthenticated = true, lastSignInError = nil"
Note over UI,TS: Browser sign-in timeout path
UI->>AM: beginSignInAndAwait(timeout)
AM->>AM: waitForSignInSettled(timeout) → false
AM->>AM: timeOutBrowserSignInAttempt()
AM->>AM: beginAuthMutation(.signInCancellation, G2)
AM-->>UI: "isLoading = false, lastSignInError = .message(...)"
Note over UI,TS: Credential sign-in superseded by signOut
UI->>AM: signInWithCredential(email, pwd)
AM->>AM: "beginAuthMutation(G1), isLoading = true"
AM->>AM: credentialSignIn(email, pwd) [awaiting]
UI->>AM: signOut()
AM->>AM: beginAuthMutation(.signOut, G2)
AM->>AM: "cancelBrowserSignInForSignOut() → isLoading = false"
AM->>TS: clearTokens()
AM-->>UI: "isAuthenticated = false"
AM->>TS: setTokens(G1 result) [G1 completes]
AM->>AM: keepAuthMutationIfCurrent(G1) → false, clearSessionState()
AM-->>UI: tokens cleared, no error shown
Reviews (41): 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}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
4 tasks
Contributor
|
Sharing diagnostic data from local verification of the same fix shape on a separate branch — should help triage what comes after this PR lands. On a The specific failure shape in
After this PR lands the visibility fix, users will see "로그인에 실패했습니다. 다시 시도하세요." but the sign-in still won't complete because the deep link itself is malformed. I'm opening a separate web-only PR against Will link the follow-up PR here once it's open. |
ASWebAuthenticationSession reports user-dismissed sheets as canceledLogin. That path should end loading without presenting a failure banner, while real web-auth errors still surface a visible Settings error. Constraint: AuthManager.swift must stay within the Swift file length budget. Rejected: Surface canceledLogin as a generic failure | normal user dismissal is not an actionable sign-in failure. Confidence: medium Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: git diff --check Not-tested: Local unit/UI tests by instruction.
Generic sign-in failures should be visible without exposing platform NSError descriptions to users. The original message payload still travels in AuthSignInError for diagnostics and mapping, while the user-facing string stays localized and stable. Constraint: Settings copy must use localized strings and avoid leaking OAuth/session implementation details. Rejected: Append NSError.localizedDescription to the generic string | produces developer-facing hybrid text in the Account settings UI. Confidence: high Scope-risk: narrow Tested: git diff --check; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Local tests/build omitted per instruction to avoid local test runs and direct xcodebuild.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
|
Acknowledged the diagnostic about the malformed web callback path. This PR is intentionally scoped to the native visible-error surface for #3617: the invalid callback is now shown instead of being swallowed, and the separate web-side token/deep-link emission fix can land independently without changing the native error UX in this branch. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 7-8: Add a single blank line immediately after the "### Fixed"
heading in CHANGELOG.md so the heading is separated from the following list
(fixing MD022 / blanks-around-headings); ensure the file contains "### Fixed"
followed by an empty line before the "- Surface Settings > Sign In..." list
entry.
In `@Sources/Auth/AuthManager.swift`:
- Around line 207-213: When session.start() returns false, don't assign an empty
string to lastSignInError; instead capture a meaningful message so the error
state retains context for debugging. Update the failure branch where
webAuthSession = session and session.start() is false (the block that currently
calls Self.authLogger.error("auth.webauth session.start returned false") and
sets lastSignInError = .message("")) to set lastSignInError =
.message("auth.webauth session.start returned false") or a similar descriptive
message that mirrors the logged text (referencing webAuthSession,
session.start(), Self.authLogger.error and lastSignInError) so the stored error
payload is informative.
🪄 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: b055e2f3-b634-461b-9525-b151ee7e6010
📒 Files selected for processing (8)
CHANGELOG.mdGhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/Auth/AuthManager.swiftSources/Auth/AuthManagerErrors.swiftSources/Settings/AuthSettingsRow.swiftSources/cmuxApp.swiftcmuxTests/AuthManagerSignInErrorTests.swift
💤 Files with no reviewable changes (1)
- Sources/cmuxApp.swift
The native sign-in fix is unchanged; hosted macOS CI failed while installing Zig from ziglang.org, and CircleCI cannot be rerun from this workspace without a CircleCI token. Constraint: User requires CI green before the final tagged reload and local launch. Rejected: Change app code without CI logs | no branch-specific failure evidence was available. Rejected: Modify CI installer immediately | a no-op retry preserves PR scope before adding workflow hardening. Confidence: medium Scope-risk: narrow Tested: No code changes; GitHub/Vercel/review checks were passing before retry. Not-tested: CircleCI step logs unavailable via public API; local tests/builds omitted per instruction.
Review feedback showed that callback handling could clear the loading state while an ASWebAuthenticationSession was still active. The callback path now only owns loading state for out-of-band callbacks, while the browser-session completion remains responsible for in-flight sessions. The session.start failure path also records a useful diagnostic payload, and the changelog heading spacing matches the markdown rule.\n\nConstraint: Do not run local tests or direct xcodebuild for this branch.\nConfidence: high\nScope-risk: narrow\nDirective: Keep web-auth-session state ownership in one place; callback helpers should not make the Sign In button appear idle while a session is still in flight.\nTested: git diff --check; python3 -m json.tool Resources/Localizable.xcstrings; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv\nNot-tested: Local unit tests and app build per user instruction; CI will verify after push.
lawrencecchen
dismissed
coderabbitai[bot]’s stale review
May 6, 2026 17:22
Addressed by ab73452: the changelog spacing and session.start diagnostic payload comments are fixed, CodeRabbit's current status is passing, and the inline review threads are resolved.
The required GitHub checks and review feedback were clean on ab73452, but Vercel and CircleCI left optional commit statuses pending without starting jobs or exposing failure logs. This empty commit refreshes those external contexts so the PR can reach an observable terminal state without changing product code.\n\nConstraint: No direct xcodebuild or local tests per user instruction.\nRejected: Patch CI config without logs | no evidence of a repository-side failure.\nConfidence: medium\nScope-risk: narrow\nDirective: Do not treat this as product behavior; it is an external CI/status retry only.\nTested: Required PR checks were green before this retry; no file changes in this commit.\nNot-tested: Local tests/build intentionally not run.
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 `@Sources/Auth/AuthManager.swift`:
- Around line 207-212: When session.start() returns false in AuthManager (the
block that sets webAuthSession, lastSignInError and isLoading), short-circuit
the awaited sign-in path instead of only flipping isLoading: make beginSignIn()
return a started/failed result (e.g., Bool or an enum) or have it throw on
synchronous failure, and have callers of beginSignInAndAwait() check that result
and return immediately when start failed; alternatively, have
beginSignInAndAwait() detect the synchronous failure by checking
webAuthSession/lastSignInError right after beginSignIn() and return the failure
immediately so awaiting callers are not left waiting for an observed $isLoading
change. Ensure references: webAuthSession, session.start(), lastSignInError,
isLoading, beginSignIn(), and beginSignInAndAwait().
🪄 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: bb98641e-5f6c-4803-aed5-a7fc89d4c689
📒 Files selected for processing (2)
CHANGELOG.mdSources/Auth/AuthManager.swift
ASWebAuthenticationSession.start() can fail synchronously, leaving beginSignInAndAwait() to subscribe after isLoading has already returned to false. The awaited path now returns immediately in that settled state instead of waiting for a publisher transition that already happened.\n\nConstraint: No direct xcodebuild or local tests per user instruction.\nConfidence: high\nScope-risk: narrow\nDirective: Awaited auth helpers must handle synchronous settle paths as well as publisher-delivered transitions.\nTested: git diff --check; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv\nNot-tested: Local unit tests/build intentionally not run.
…-3617-sign-in-error-surface
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 7ecf7d6. Configure here.
…-3617-sign-in-error-surface # Conflicts: # Sources/cmuxApp.swift # cmux.xcodeproj/project.pbxproj
Contributor
|
On current I am not looking to replace the broader work in this PR. Would it be okay if I opened a narrowly scoped follow-up PR against the current architecture that only wires this existing failure state into the macOS Settings UI, with localization and behavior-level coverage? |
lawrencecchen
added
the
stale-revisit
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
4 participants
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.
Closes #3617
Fixes #3617
Summary
Validation
Note
Medium Risk
Touches authentication flows, token persistence, and concurrent sign-in/sign-out/callback handling; behavior is well covered by new tests but mistakes could affect session state.
Overview
Account Sign In failures now show a generic localized message under the button in Settings instead of failing silently (fixes #3617). The UI reads
AuthManager.lastSignInError; user cancel onASWebAuthenticationSessionstays suppressed.AuthManagernow records errors for web-auth failures, invalid callbacks (only during an active browser attempt),session.start()failure, callback refresh failures, and credential sign-in failures, and clears them on success, a new attempt, or sign-out.beginSignInAndAwaitcan time out and cancel the web session so late callbacks do not sign the user in after sign-out or timeout. Credential andapplySignInResultpaths use the same auth-mutation gating as browser sign-in so concurrent sign-out does not leave tokens or a stuck loading state.Errors and helpers move to
AuthManagerErrors.swift; the account row moves toSettings/AuthSettingsRow.swiftwith red trailing error text and a guard while loading/restoring. NewAuthManagerSignInErrorTestscover the concurrency and error-display cases; changelog andsettings.account.error.signInFailedstrings are added.Reviewed by Cursor Bugbot for commit ba35199. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Surfaces a localized, generic error when Sign In fails in Settings > Account and hardens auth concurrency to prevent stale or duplicate updates. Keeps raw error strings out of the UI and resolves #3617.
Bug Fixes
canceledLogin.session.startfailures; time out and cancel stalled sessions; prevent delayed callbacks (timeout or sign-out) from authenticating; keep loading ownership with the active web session;beginSignInAndAwaitreturns authoritative state on sync-start failures and timeouts.applySignInResult; gate loading/auth updates behind the active mutation; prevent auth restore after concurrent sign-out; suppress stale callback/credential errors; keep superseded credential sign-ins internal.Refactors
AuthSignInError/AuthManagerErrortoAuthManagerErrors.swiftwith localized copy (settings.account.error.signInFailed); move the UI row toSettings/AuthSettingsRow.swift; inject credential sign-in for tests; add translations, tests (AuthManagerSignInErrorTests), and CHANGELOG entry.Written for commit ba35199. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Localization
Tests