Redirect when a sign-in flow completes on its first step#4
Conversation
📝 WalkthroughWalkthroughTerminal flow-status handling (Error and Complete) in SignIn.tsx is refactored into a new shared async helper, ChangesTerminal Response Handling Refactor
Estimated code review effort: 2 (Simple) | ~10 minutes Related PRs: None specified. Suggested labels: refactor, react Suggested reviewers: None specified. 🚥 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 |
madurangasiriwardena
force-pushed
the
feature/session-poc-2
branch
2 times, most recently
from
b073a63 to
b03ad5e
Compare
madurangasiriwardena
force-pushed
the
feature/session-poc-2
branch
from
b03ad5e to
2f16910
Compare
The v2 SignIn handled terminal flow statuses (Complete/Error) only after a submission, not on the initial flow load. When a flow completes on its first step — e.g. a reused browser SSO session lets the backend return COMPLETE immediately with no UI components — the gate fell through with no components and spun on a loader forever. Extract the terminal-response handling into a shared helper and run it on the initial load (initializeFlow) as well as after a submission (handleSubmit), so the flow redirects to the callback instead of hanging.
madurangasiriwardena
force-pushed
the
feature/session-poc-2
branch
from
2f16910 to
184ca6f
Compare
![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
🧹 Nitpick comments (2)
packages/react/src/components/presentation/auth/SignIn/SignIn.tsx (2)
453-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete cleanup largely duplicates
clearFlowState; consider reusing it.Lines 462-471 re-implement most of
clearFlowState(execution id reset, challenge-token clear,isFlowInitialized=false, hybridauthIdremoval). The explicitsessionStorage.removeItem('thunderid_execution_id')(Line 465) is redundant sincesetExecutionId(null)already removes it (Line 267). Delegating toclearFlowState()would reduce duplication and prevent the two cleanup paths from diverging over time. Note thatclearFlowStateadditionally resetsoauthCodeProcessedRef/isTimeoutDisabled, so verify that difference is acceptable on the Complete path before consolidating.🤖 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 `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx` around lines 453 - 491, The completion cleanup in SignIn’s success branch is duplicating the logic already centralized in clearFlowState, so refactor it to reuse that helper instead of repeating execution-id, challenge-token, isFlowInitialized, and hybrid authId cleanup. Remove the redundant sessionStorage deletion for thunderid_execution_id since setExecutionId(null) already handles it, and verify whether the extra reset of oauthCodeProcessedRef and isTimeoutDisabled in clearFlowState is acceptable for the Complete flow before consolidating.
455-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLint:
||here is intentional, but newany/nullish findings may block CI.The
||on Lines 455-456 is correct for URL fallback (an empty string should fall through), so@typescript-eslint/prefer-nullish-coalescingis a false positive here — add a targetedeslint-disable-next-lineif the rule is enforced. The surroundingno-explicit-any/no-unsafe-*findings mirror the file's existing style; please confirm they don't fail the pipeline for the newly added lines.🤖 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 `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx` around lines 455 - 456, The fallback logic in SignIn.tsx uses intentional || behavior for redirectUrl and finalRedirectUrl, so keep that behavior and add a targeted eslint-disable-next-line for the nullish-coalescing rule if needed around the redirectUrl/afterSignInUrl assignment. Also review the new any casts on response and redirectUrl in SignIn to ensure they do not introduce fresh no-explicit-any or no-unsafe-* CI failures, while keeping the existing style consistent.Source: Linters/SAST tools
🤖 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 `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx`:
- Around line 458-485: The no-redirect completion path in SignIn leaves
BaseSignIn stuck in a loading state because setIsFlowInitialized(false) is
called without ever surfacing a terminal outcome when finalRedirectUrl is empty.
Update the completion logic in SignIn so that, after clearing OAuth state, it
explicitly handles the empty finalRedirectUrl case by setting a terminal error
or completed state and returning early instead of falling through to the
redirect path. Use the SignIn component flow around setIsFlowInitialized,
cleanupOAuthUrlParams, and onSuccess to ensure BaseSignIn no longer stays
blocked on isLoading || !isInitialized || !isFlowInitialized.
---
Nitpick comments:
In `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx`:
- Around line 453-491: The completion cleanup in SignIn’s success branch is
duplicating the logic already centralized in clearFlowState, so refactor it to
reuse that helper instead of repeating execution-id, challenge-token,
isFlowInitialized, and hybrid authId cleanup. Remove the redundant
sessionStorage deletion for thunderid_execution_id since setExecutionId(null)
already handles it, and verify whether the extra reset of oauthCodeProcessedRef
and isTimeoutDisabled in clearFlowState is acceptable for the Complete flow
before consolidating.
- Around line 455-456: The fallback logic in SignIn.tsx uses intentional ||
behavior for redirectUrl and finalRedirectUrl, so keep that behavior and add a
targeted eslint-disable-next-line for the nullish-coalescing rule if needed
around the redirectUrl/afterSignInUrl assignment. Also review the new any casts
on response and redirectUrl in SignIn to ensure they do not introduce fresh
no-explicit-any or no-unsafe-* CI failures, while keeping the existing style
consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bce4b354-a878-439e-a45d-9143c4bdda47
📒 Files selected for processing (1)
packages/react/src/components/presentation/auth/SignIn/SignIn.tsx
| // Clear submitting state before redirect | ||
| setIsSubmitting(false); | ||
|
|
||
| // Clear all OAuth-related storage on successful completion | ||
| setExecutionId(null); | ||
| await setChallengeToken(null); | ||
| setIsFlowInitialized(false); | ||
| sessionStorage.removeItem('thunderid_execution_id'); | ||
| try { | ||
| const storageManager: any = await getStorageManager(); | ||
| await storageManager?.removeHybridDataParameter?.('authId'); | ||
| } catch { | ||
| logger.warn('Failed to clear authId from hybrid storage after completion.'); | ||
| } | ||
|
|
||
| // Clean up OAuth URL params before redirect | ||
| cleanupOAuthUrlParams(true); | ||
|
|
||
| if (onSuccess) { | ||
| onSuccess({ | ||
| redirectUrl: finalRedirectUrl, | ||
| ...(response.data || {}), | ||
| }); | ||
| } | ||
|
|
||
| if (finalRedirectUrl && window?.location) { | ||
| window.location.href = finalRedirectUrl; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant slices of SignIn.tsx and the loading condition in BaseSignIn.
sed -n '430,500p' packages/react/src/components/presentation/auth/SignIn/SignIn.tsx
printf '\n---\n'
sed -n '970,1020p' packages/react/src/components/presentation/auth/SignIn/SignIn.tsx
printf '\n---\n'
rg -n "afterSignInUrl|redirectUrl|redirect_uri|isFlowInitialized|isLoading =" packages/react/src/components/presentation/auth/SignIn/SignIn.tsxRepository: thunder-id/javascript-sdks
Length of output: 4720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '640,680p' packages/react/src/components/presentation/auth/SignIn/SignIn.tsx
printf '\n---\n'
sed -n '680,720p' packages/react/src/components/presentation/auth/SignIn/SignIn.tsxRepository: thunder-id/javascript-sdks
Length of output: 3131
Handle the no-redirect Complete path explicitly.
setIsFlowInitialized(false) leaves BaseSignIn stuck loading when finalRedirectUrl is empty (isLoading || !isInitialized || !isFlowInitialized stays true). Surface a terminal error/state before returning if there’s no redirect target.
🧰 Tools
🪛 ESLint
[error] 467-467: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
[error] 467-467: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 468-468: Unsafe call of an any typed value.
(@typescript-eslint/no-unsafe-call)
[error] 468-468: Unsafe member access .removeHybridDataParameter on an any value.
(@typescript-eslint/no-unsafe-member-access)
[error] 478-478: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
[error] 484-484: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
🤖 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 `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx` around
lines 458 - 485, The no-redirect completion path in SignIn leaves BaseSignIn
stuck in a loading state because setIsFlowInitialized(false) is called without
ever surfacing a terminal outcome when finalRedirectUrl is empty. Update the
completion logic in SignIn so that, after clearing OAuth state, it explicitly
handles the empty finalRedirectUrl case by setting a terminal error or completed
state and returning early instead of falling through to the redirect path. Use
the SignIn component flow around setIsFlowInitialized, cleanupOAuthUrlParams,
and onSuccess to ensure BaseSignIn no longer stays blocked on isLoading ||
!isInitialized || !isFlowInitialized.
2 participants
Purpose
Fix the login gate hanging on a perpetual loading spinner when an authentication flow completes on its very first /flow/execute step — i.e. when the backend returns flowStatus: COMPLETE immediately, before any UI components are rendered (for example, when a reused browser SSO session lets the flow finish without a credential prompt).
In the SignIn component (presentation/auth/SignIn/SignIn.tsx), terminal flow statuses (Complete / Error) were handled only in the submission path (handleSubmit), not on the initial flow load (initializeFlow). On an initial COMPLETE response there are no components to render, so the initialize path fell through and the UI stayed on CircularProgress forever instead of redirecting to the callback.
This is a general client-side correctness fix: any flow that completes (or errors) on its first step is now finished properly. It is self-contained — no dependency on any backend feature, and no behavior change for existing flows that begin with a prompt (the new branch simply isn't hit until a flow returns COMPLETE on the first step).
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit