fix(shell-ui): surface OAuth callback errors from ?auth_error#1554
fix(shell-ui): surface OAuth callback errors from ?auth_error#1554kgarg2468 wants to merge 3 commits into
Conversation
Read auth_error/error params in bootstrap, clean the URL, and show an
actionable banner (reuses ConnectionErrorBanner with errorKind).
Fixes rocketride-ai/rocketride-saas#301
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesOAuth failure recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserURL
participant ConnectionManager
participant ConnectionStatus
participant ConnectionErrorBanner
BrowserURL->>ConnectionManager: Provide OAuth callback error parameters
ConnectionManager->>ConnectionStatus: Set AUTH_FAILED with errorKind oauth-callback
ConnectionStatus->>ConnectionErrorBanner: Expose latched failure category
ConnectionErrorBanner->>ConnectionManager: Trigger retry action
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsx (1)
65-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
failure?.errorKindto theuseEffectdependency array.If
lastFailuretransitions betweenerrorKindvalues ('session'→'oauth-callback') with the samestateandlastError, the dismissed state won't reset because the deps don't captureerrorKindchanges. The banner would stay dismissed for the new failure type.🔧 Proposed fix
useEffect(() => { setDismissed(false); - }, [failure?.state, failure?.lastError]); + }, [failure?.state, failure?.lastError, failure?.errorKind]);🤖 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 `@apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsx` around lines 65 - 67, Update the useEffect in ConnectionErrorBanner to include failure?.errorKind in its dependency array alongside failure?.state and failure?.lastError, so dismissed resets when the failure type changes even if the other values remain unchanged.
🤖 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 `@apps/shell-ui/src/connection/connection.ts`:
- Around line 457-458: Update the errorDescription assignment in the OAuth
error-handling flow to use logical OR chaining instead of nullish coalescing, so
empty auth_error or error_description values fall through to the next available
parameter and the existing if (errorDescription) banner path is preserved.
---
Outside diff comments:
In `@apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsx`:
- Around line 65-67: Update the useEffect in ConnectionErrorBanner to include
failure?.errorKind in its dependency array alongside failure?.state and
failure?.lastError, so dismissed resets when the failure type changes even if
the other values remain unchanged.
🪄 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: 327198e5-8aec-4864-8dc0-7857195b279c
📒 Files selected for processing (3)
apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsxapps/shell-ui/src/connection/connection.tspackages/shared-ui/src/types/connection.ts
|
@coderabbitai Addressed in 3e4672a — error-param fallback uses logical OR so empty strings fall through, and failure?.errorKind joined the effect dependency array. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5 3e4672af 2>/dev/null || echo "commit not found locally"
git show 3e4672af --stat 2>/dev/null
git show 3e4672af -- apps/shell-ui/src/connection/connection.ts 2>/dev/null | head -100Length of output: 2043 🏁 Script executed: #!/bin/bash
git show 3e4672af -- apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsxLength of output: 1032
|
dylan-savage
left a comment
dylan-savage
left a comment
There was a problem hiding this comment.
Over-broad param capture (connection.ts:456): matching bare error / error_description means any app URL landing on the redirect root with an unrelated ?error=… param (e.g. a deep link) is hijacked — the param is stripped and a false "Sign-in didn't complete" banner shown. Zitadel's own failures use auth_error; gate the generic OAuth params on a callback signal (presence of state / no code) rather than treating every bootstrap as a potential callback.
Addresses review: generic error/error_description could be hijacked by an unrelated app deep-link. Our redirect_uri is the server callback, which wraps every Zitadel failure as auth_error, so generic params never legitimately reach the app root. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch @dylan-savage — fixed in 4547b78. Rather than gate the generic params on a callback signal, I dropped Verified locally: |
2 participants
Zitadel failures never reach the user:
?auth_erroris set by the callback and read by nobodypackages/ai/src/ai/web/endpoints/auth_callback.pyredirects OAuth failures back to the app as?auth_error=<description>— but no web client ever reads the parameter (reported in rocketride-ai/rocketride-saas#301). Users who cancel at Zitadel, or hit any IdP-side failure, land back on the logged-out page with zero feedback, and the error param sticks around in the URL.Fix:
_bootstrap()now readsauth_error(plus standarderror/error_description) right where it already parses?code, strips it from the URL via the existinghistory.replaceStatecleanup, and surfaces it through the #1553 recovery banner with a newerrorKind: 'oauth-callback' | 'session'discriminator onConnectionStatus: "Sign-in didn't complete: <IdP description>" with a Try again action that restarts the OAuth flow.Stacked on #1553 (base is
fix/305-network-vs-auth-errors; GitHub will retarget todevelopwhen it merges — the diff here is one commit).Testing
/?auth_error=User%20cancelled%20the%20request→ banner "Sign-in didn't complete: User cancelled the request", URL cleaned to/, logged-out landing intact, Try again navigates to the Zitadel login page.tsc --noEmitclean.Linked Issue
Fixes rocketride-ai/rocketride-saas#301
🤖 Generated with Claude Code
Summary by CodeRabbit