Repository navigation
Conversation
…ow-ai#3617) Per AGENTS.md regression test policy: this commit ships the failing test alone so CI proves it catches the bug. The fix lands in the next commit.
AuthManager.beginSignIn / handleCallbackURL previously swallowed every failure via NSLog only, so a user who clicked Settings -> Sign In... and saw the auth sheet flash and close was left with no visible error and no way to debug. Now AuthManager publishes the most recent failure as lastSignInError; AuthSettingsRow renders it in red below the subtitle. Suppresses the error for ASWebAuthenticationSessionError.canceledLogin (explicit user cancel) so the UX stays quiet when the user cancels themselves. Adds a new AuthManagerError.signInSessionFailed case for non-AuthManagerError throws (network, sandbox, etc.), with a localized 'Sign-in didn't complete. Please try again.' string in en/ja/uk/ko.
|
@psh4607 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds explicit sign-in error tracking and exposure: AuthManager records lastSignInError (new published property and AuthManagerError case), sign-in flow records/clears that state, UI displays the localized error, and tests cover invalid callback visibility. A localization key for signInSessionFailed was added. ChangesSign-In Error Visibility Feature
Sequence DiagramsequenceDiagram
actor User
participant UI as AuthSettingsRow
participant Manager as AuthManager
participant WebSession as ASWebAuthenticationSession
participant Callback as handleCallbackURL
User->>UI: beginSignIn()
UI->>Manager: beginSignIn()
Manager->>WebSession: start web auth
alt Web session fails / canceled
WebSession-->>Manager: failure/cancel
Manager->>Manager: recordSignInSessionError(.signInSessionFailed)
Manager->>Manager: lastSignInError = .signInSessionFailed
else Web session returns callback
WebSession-->>Manager: callback URL
Manager->>Callback: handleCallbackURL(url)
alt invalid callback
Callback->>Manager: lastSignInError = .invalidCallback
Callback-->>Manager: throw invalidCallback
else valid callback
Callback->>Callback: seed tokens & refresh session
Callback-->>Manager: clear lastSignInError
end
end
Manager->>UI: publish lastSignInError
UI->>User: display errorText (if present)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 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 |
Greptile SummarySurfaces browser sign-in failures that were previously swallowed silently:
Confidence Score: 5/5Safe to merge; error state is cleared at every auth-state transition, user-initiated cancels are suppressed correctly, and regression tests cover the two key invariants. All sign-in-success and sign-out paths now clear The three replaced NSLog sites in Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant AuthSettingsRow
participant AuthManager
participant ASWebAuthSession
User->>AuthSettingsRow: Tap "Sign In…"
AuthSettingsRow->>AuthManager: beginSignIn()
AuthManager->>AuthManager: lastSignInError = nil
AuthManager->>ASWebAuthSession: session.start()
alt session.start() returns false
AuthManager->>AuthManager: lastSignInError = .signInSessionFailed
AuthManager-->>AuthSettingsRow: publishes lastSignInError
AuthSettingsRow-->>User: shows red error text
else session starts
ASWebAuthSession-->>AuthManager: completion(callbackURL?, error?)
alt error == canceledLogin
AuthManager->>AuthManager: suppress (no error set)
else other error
AuthManager->>AuthManager: lastSignInError = .signInSessionFailed
AuthManager-->>AuthSettingsRow: publishes lastSignInError
AuthSettingsRow-->>User: shows red error text
else callbackURL received
AuthManager->>AuthManager: handleCallbackURL(url)
alt invalid payload
AuthManager->>AuthManager: lastSignInError = .invalidCallback
AuthManager-->>AuthSettingsRow: publishes lastSignInError
AuthSettingsRow-->>User: shows red error text
else valid payload
AuthManager->>AuthManager: tokenStore.seed + refreshSession()
AuthManager->>AuthManager: lastSignInError = nil
AuthManager-->>AuthSettingsRow: isAuthenticated = true, error cleared
AuthSettingsRow-->>User: shows signed-in state
end
end
end
Reviews (2): Last reviewed commit: "Address PR review feedback (#3621)" | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Surfaces previously silent authentication sign-in failures (notably ASWebAuthenticationSession failures and invalid callback URLs) to users by publishing the latest sign-in error from AuthManager and rendering it in Settings, with localized messaging and regression coverage.
Changes:
- Add
AuthManager.lastSignInError(published) and record failures frombeginSignIn/handleCallbackURL, including a new.signInSessionFailederror case. - Render the most recent sign-in error message in
AuthSettingsRowbelow the account subtitle. - Add a localization key for the generic sign-in failure and a regression test ensuring invalid callbacks populate
lastSignInError.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Sources/cmuxApp.swift | Displays AuthManager.lastSignInError text in Settings (Account row) when present. |
| Sources/Auth/AuthManager.swift | Introduces lastSignInError, new error case, and records/clears errors across sign-in and callback handling. |
| Resources/Localizable.xcstrings | Adds localized string for settings.account.error.signInSessionFailed. |
| cmuxTests/AuthManagerSignInErrorVisibilityTests.swift | Regression test asserting invalid callback URLs publish .invalidCallback via lastSignInError. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let asError = error as? ASWebAuthenticationSessionError, | ||
| asError.code == .canceledLogin { | ||
| return | ||
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @Published private(set) var isLoading = false | ||
| @Published private(set) var isRestoringSession = false | ||
| @Published private(set) var didCompleteBrowserSignIn = false | ||
| @Published private(set) var lastSignInError: AuthManagerError? |
There was a problem hiding this comment.
@Published state on ObservableObject — the cmux-swiftui-state-layout and cmux-swift-concurrency-modernization rules flag new @Published properties for new state when @Observable is available. AuthManager is already ObservableObject so this extends existing debt, but each new @Published widens the invalidation surface: every subscriber re-renders on every lastSignInError write. Migrating AuthManager to @Observable is the correct long-term fix.
| @Published private(set) var lastSignInError: AuthManagerError? | |
| // TODO: Migrate AuthManager to @Observable; adding @Published here widens | |
| // invalidation for all subscribers. See cmux-swiftui-state-layout rule. | |
| @Published private(set) var lastSignInError: AuthManagerError? |
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmuxTests/AuthManagerSignInErrorVisibilityTests.swift`:
- Line 16: The test classes trigger SwiftLint's required_deinit rule; add an
explicit deinit to each test-only final class to silence the warning: locate the
final classes AuthManagerSignInErrorVisibilityTests and NoopAuthTestClient and
add an empty deinit method (deinit { }) in each class so lint passes without
changing production behavior.
- Around line 38-41: The current catch clause "catch is AuthManagerError"
swallows any AuthManagerError variant; change the test's error handling around
handleCallbackURL to pattern-match the concrete AuthManagerError case you expect
(e.g., catch AuthManagerError.invalidCallback) and fail (or rethrow) for any
other AuthManagerError so unexpected cases like .signInSessionFailed surface at
the throw site; update the catch blocks in
AuthManagerSignInErrorVisibilityTests.swift near the handleCallbackURL
invocation to explicitly match AuthManagerError.invalidCallback and add a
XCTFail for other AuthManagerError cases.
In `@Sources/Auth/AuthManager.swift`:
- Line 137: lastSignInError is left set across different sign-in/sign-out flows
causing stale error UI; clear it whenever authentication state changes: set
lastSignInError = nil in the success paths of applySignInResult(_:) and
signInWithCredential(email:password:), after successful token seeding in
seedTokensFromCLI(...), and also clear it in signOut() and
clearSessionState(...) so every successful authenticated transition and session
clear removes the stale error (beginSignIn already clears it).
🪄 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: b5e25aae-a308-4ab9-959e-9eeb6efd5713
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/Auth/AuthManager.swiftSources/cmuxApp.swiftcmuxTests/AuthManagerSignInErrorVisibilityTests.swift
| /// published `lastSignInError`. Per AGENTS.md regression test policy this | ||
| /// failing test ships in its own commit so CI proves it catches the bug. | ||
| @MainActor | ||
| final class AuthManagerSignInErrorVisibilityTests: XCTestCase { |
There was a problem hiding this comment.
SwiftLint required_deinit warnings on test classes may fail CI.
Both AuthManagerSignInErrorVisibilityTests and NoopAuthTestClient are final class types without explicit deinit methods, triggering the required_deinit rule. Since these are test-only types, this has no production impact, but the warnings are flagged at the same severity level as production code and could fail the lint step.
🔧 Proposed fix
`@MainActor`
final class AuthManagerSignInErrorVisibilityTests: XCTestCase {
+ deinit {}
func testInvalidCallbackURLPublishesLastSignInError() async throws { private final class NoopAuthTestClient: AuthClientProtocol {
+ deinit {}
func currentUser() async throws -> CMUXAuthUser? { nil }Also applies to: 53-53
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 16-16: Classes should have an explicit deinit method
(required_deinit)
🤖 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/AuthManagerSignInErrorVisibilityTests.swift` at line 16, The test
classes trigger SwiftLint's required_deinit rule; add an explicit deinit to each
test-only final class to silence the warning: locate the final classes
AuthManagerSignInErrorVisibilityTests and NoopAuthTestClient and add an empty
deinit method (deinit { }) in each class so lint passes without changing
production behavior.
|
Locally tested the visibility fix on a Diagnostic result from this build: the surfaced error is That narrows #3617's root cause significantly:
Likely culprit is in Will follow up with a separate PR targeting |
Cubic 'Cmux Swift Logging' check: 3 NSLog calls in beginSignIn now use the existing authLog helper, which is #if DEBUG-guarded. Greptile P1 + Cubic 'Cmux Architecture Rethink' + CodeRabbit: lastSignInError is now cleared in clearSessionState (covers signOut), applySignInResult, signInWithCredential, and seedTokensFromCLI success paths so a stale error never renders alongside an authenticated row or after sign-out. CodeRabbit test rigor: tightened catch to match AuthManagerError.invalidCallback specifically and XCTFail on any other AuthManagerError variant. Added a second regression test that primes lastSignInError = .invalidCallback and asserts signOut() clears it.
|
Pushed
Skipped:
Verified locally with Diff size: +34/-4 across |
|
Closing — superseded by #3624, which lands the same visibility fix in a tighter shape: extracts The diagnostic data captured during local verification on this branch is still useful — particularly that the actual surfaced error on Debug builds is |
Summary
Closes #3617.
AuthManager.beginSignInandhandleCallbackURLpreviously swallowed every failure viaNSLogonly. A user clicking Settings → Sign In… who hit any ofASWebAuthenticationSessionError(canceled consent, sandbox/TCC, network unreachable…),handleCallbackURLthrowing.invalidCallbackbecause the after-sign-in handler emitted a malformed deep link, orsession.start()returning false saw the auth sheet flash and close with no visible error and no way to debug. The app stayed at "Not signed in" silently.This PR publishes the most recent failure as
AuthManager.lastSignInErrorand renders it in red below the Account row subtitle inAuthSettingsRow, so the failure mode that motivated #3617 is no longer invisible — regardless of which underlying root cause hits in any given environment.What changed
Sources/Auth/AuthManager.swiftAuthManagerErroris nowEquatableand gains asignInSessionFailedcase for non-AuthManagerErrorfailures (network, sandbox, etc.).@Published private(set) var lastSignInError: AuthManagerError?published so SwiftUI can render it.beginSignInclearslastSignInErroron entry, sets.signInSessionFailedonsession.start()failure, and routes the completion handler'serrorthrough a newrecordSignInSessionError(_:)helper that suppresses the error forASWebAuthenticationSessionError.canceledLogin(explicit user cancel) so the UX stays quiet when the user cancels themselves.handleCallbackURLsetslastSignInError = .invalidCallbackbefore the early throw and otherwise sets it to the typedAuthManagerError(or.signInSessionFailedfallback) on any throw fromtokenStore.seed/refreshSession. Clears it on success.Sources/cmuxApp.swift—AuthSettingsRowreadsauthManager.lastSignInError?.errorDescriptionand renders it in red under the existing subtitle. No layout change when no error is set.Resources/Localizable.xcstrings— addssettings.account.error.signInSessionFailedwithen/ja/uk/kotranslations, matching the existingsettings.account.error.*key pattern.This deliberately doesn't try to fix the underlying root cause(s) hypothesized in #3617 (
canceledLoginfrom a dismissed consent dialog, double-fire from view-body re-evaluation, sandbox denial, network failure). It makes those root causes diagnosable so the next PR can target the actual cause once a Console.app log is captured.Test plan
AGENTS.mdregression test policy is two commits:e0cd032a— adds the failing regression testcmuxTests/AuthManagerSignInErrorVisibilityTests.swift. The test referencesAuthManager.lastSignInError, which doesn't exist yet on this commit, so CI compile fails — proving the test catches the bug.0cbea3d8— ships the fix. The test now compiles and assertslastSignInError == .invalidCallbackafterhandleCallbackURLthrows on a payload-lesscmux://auth-callbackURL.The CI signal across the two commits is the proof: red on commit 1, green on commit 2.
Manual verification (not run locally per
AGENTS.md"Never run tests locally"):Sign-in didn't complete. Please try again.under the subtitle.canceledLogin).Out of scope (follow-up issues)
auth.webauth failed: …line from Console.app, then targeted fix per the canceledLogin / double-fire / sandbox / network branches in Settings → Sign In… opens auth sheet that closes immediately (no page rendered) #3617.web/app/handler/after-sign-in/OpenNativeClient.tsxUX nit — auto-redirect to the deep link instead of relying on a manual click. Different reviewer (web), different PR.AuthSettingsRow.buttonAction()(webAuthSession != nil/isLoading == true). Different concern, separate PR.Summary by cubic
Surfaced sign-in errors in Settings so users see why auth failed instead of a silent close. Fixes #3617 by publishing the last sign-in error, showing it under Account, and clearing it on success and sign-out.
Bug Fixes
lastSignInError;AuthManagerErrorisEquatablewith new.signInSessionFailed. Error is set onsession.start()failure, invalid callback, and other throws; suppressed forASWebAuthenticationSessionError.canceledLogin; cleared on successful flows and on sign-out to avoid stale messages.settings.account.error.signInSessionFailedwith en/ja/uk/ko translations.Refactors
NSLogcalls in the sign-in flow withauthLog(DEBUG-guarded).Written for commit d48988d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Tests