Skip to content

Clear flow components and additional data on flow state reset - #50

Open
PasinduYeshan wants to merge 1 commit into
thunder-id:mainfrom
PasinduYeshan:fix/clear-components-on-terminal-flow-error
Open

Clear flow components and additional data on flow state reset#50
PasinduYeshan wants to merge 1 commit into
thunder-id:mainfrom
PasinduYeshan:fix/clear-components-on-terminal-flow-error

Conversation

@PasinduYeshan

@PasinduYeshan PasinduYeshan commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Purpose

clearFlowState() nulled the executionId but left the rendered step in components. On a terminal failure the previous step stayed on screen, which meant:

  • the form was dead — a retry hit throw new Error('No active flow ID'), which sits before the try block in handleSubmit and so surfaced as an unhandled rejection rather than an error the UI could show;
  • additionalData.stepTimeout is an absolute timestamp, and since setError flips isFlowInitialized back to true, the timeout effect could re-fire with remaining <= 0 and overwrite the real error with "Time allowed to complete the step has expired."

Consumers also had no way to distinguish "flow is dead" from "still loading", because an error with a stale components array looks identical to a recoverable one.

Approach

Clear components and additionalData inside clearFlowState() rather than patching individual catch sites, and apply the whole reset synchronously ahead of the first await.

Why clearing alongside a null executionId is correct. setComponents has three call sites: the clear itself, initializeFlow success, and handleSubmit success. Both populate sites sit inside if (normalizedExecutionId && normalizedComponents) and are immediately preceded by setExecutionId(normalizedExecutionId) — components are only ever populated together with a non-null executionId. clearFlowState was the only place executionId went null while components could remain non-empty, so clearing them together restores that invariant.

The one branch that sets an executionId without touching components is the recoverable case in handleTerminalResponse (flowStatus=ERROR with an executionId — session still alive, form deliberately kept up for retry). It does not call clearFlowState, so it is unaffected.

Why the reset moved before the await. setError sets isFlowInitialized = true, and the initialization effect is gated on !isFlowInitialized. Three of the five callers (handleTerminalResponse terminal branch, the initializeFlow catch, and the handleSubmit catch) await clearFlowState(), so the reset lands first and setError wins. The other two — handleOAuthError and useOAuthCallback.onError — do not await it. With setIsFlowInitialized(false) sitting after await setChallengeToken(null), it ran a microtask after setError, so false won: the effect re-fired, initializeFlow ran setFlowError(null), and the user saw the error flash before being dropped back on a fresh form.

Doing the whole reset synchronously makes all five callers behave identically — the flow state is cleared, then setError marks the flow initialized so the error renders, and no implicit re-initialization is triggered. Components are repopulated only by an explicit initialize() from the render props or a fresh page load, both of which go through initializeFlow and set executionId and components together.

Related Issues

Related PRs

Checklist

  • Followed the contribution guidelines.
  • Manual test round performed and verified.
  • Documentation provided. (Add links if there are any)
  • Tests provided. (Add links if there are any)

Security checks

  • Followed secure coding standards in WSO2 Secure Coding Guidelines
  • Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The sign-in flow cleanup now clears the execution ID, rendered flow components, and additional data when the flow state is expired or invalid.

Changes

Sign-in flow cleanup

Layer / File(s) Summary
Reset rendered sign-in flow state
packages/react/src/components/presentation/auth/SignIn/SignIn.tsx
clearFlowState now resets components and additionalData in addition to the execution ID.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

Suggested reviewers: brionmario

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: clearing flow components and additional data during flow state reset.
Description check ✅ Passed The description covers the purpose, approach, related work, checklist status, and security checks, with clear technical detail.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx`:
- Around line 325-327: Update handleOAuthError and the useOAuthCallback.onError
callback to await the asynchronous clearFlowState cleanup before calling
setError, ensuring isFlowInitialized cannot be reset after the error state is
presented; alternatively, move the synchronous cleanup updates before the first
await while preserving the existing error UI behavior.
🪄 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: CHILL

Plan: Pro Plus

Run ID: eec142cb-dddb-440e-acbf-0c6779a452b4

📥 Commits

Reviewing files that changed from the base of the PR and between c76184f and 6546238.

📒 Files selected for processing (1)
  • packages/react/src/components/presentation/auth/SignIn/SignIn.tsx

Comment thread packages/react/src/components/presentation/auth/SignIn/SignIn.tsx
clearFlowState nulled the executionId but left the rendered step in place,
so a terminal failure kept a form that submits into a null executionId and
a stale stepTimeout that could overwrite the real error with a timeout
message. Components are only ever set together with a non-null executionId,
so clear them together.

Apply the whole reset synchronously, before the first await, so callers that
do not await clearFlowState still complete it ahead of the setError that
follows.
@PasinduYeshan
PasinduYeshan force-pushed the fix/clear-components-on-terminal-flow-error branch from 6546238 to 6c3d69e Compare August 3, 2026 13:02
@PasinduYeshan

Copy link
Copy Markdown
Contributor Author

Should be merged after following PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants