routing issue - #3827
Conversation
📝 WalkthroughWalkthroughOAuth callback requests now bypass unauthenticated login redirects. ChangesOAuth handoff flow
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant middleware
participant AuthEffect
participant SessionSync
Browser->>middleware: Request with OAuth callback parameters
middleware-->>Browser: Allow callback without login redirect
Browser->>AuthEffect: Load with OAuth token in URL hash
AuthEffect->>SessionSync: Synchronize authentication and session state
AuthEffect-->>Browser: Render Completing sign in...
SessionSync-->>AuthEffect: Complete handoff
AuthEffect-->>Browser: Render children
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/beacon/components/providers/auth-provider.tsx (1)
38-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isHandlingHandoffin the dependency array triggers a redundant re-run.When
setIsHandlingHandoff(false)fires on line 40, this effect re-runs (sinceisHandlingHandoffis in the deps). The second run findsisHandlingHandoffalreadyfalse, skips the if-block, and does nothing useful. Consider removingisHandlingHandofffrom the dependency array and resetting it in the handoff effect instead, or suppress the lint with a documented justification.🤖 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 `@src/beacon/components/providers/auth-provider.tsx` around lines 38 - 46, Remove isHandlingHandoff from the dependency array of the session/status effect to avoid its redundant rerun, and move the setIsHandlingHandoff(false) reset into the handoff-specific effect where the handoff is completed. Preserve the existing authentication token and user-data updates in the session/status effect.
🤖 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 `@src/beacon/components/providers/auth-provider.tsx`:
- Around line 11-16: Update the isHandlingHandoff initializer to parse
window.location.hash with URLSearchParams and check for OAUTH_FRAGMENT_TOKEN_KEY
using get, matching consumeOAuthTokenHandoffFromUrl. Preserve the existing
server-side false fallback and avoid substring-based token detection.
In `@src/beacon/middleware.ts`:
- Around line 40-44: Update OAuth callback detection in the middleware to accept
only the legitimate callback signal, rather than treating arbitrary `success` or
`token` query parameters as authorization. Remove the dead
`searchParams.has('token')` check and validate `success` against the expected
OAuth callback value or an additional trusted signal, while preserving client
handling of valid callbacks and redirects for unauthenticated protected
requests.
---
Nitpick comments:
In `@src/beacon/components/providers/auth-provider.tsx`:
- Around line 38-46: Remove isHandlingHandoff from the dependency array of the
session/status effect to avoid its redundant rerun, and move the
setIsHandlingHandoff(false) reset into the handoff-specific effect where the
handoff is completed. Preserve the existing authentication token and user-data
updates in the session/status effect.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df49636e-d3d1-4abb-b6cf-a68c3288ad37
📒 Files selected for processing (2)
src/beacon/components/providers/auth-provider.tsxsrc/beacon/middleware.ts
|
New azure beacon changes available for preview here |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit