Fix race condition and migrate converted instant users#43
Merged
Conversation
Reviewer's GuideRefactors SuperTokens migration logic into a reusable hook to avoid race conditions around access token and auth level, wires it into both React and Next.js Rownd providers, and adds a Next.js example app (frontend + backend) to validate Rownd-to-SuperTokens lazy migration behavior while updating existing migration examples and typings. Sequence diagram for SuperTokens migration via useSuperTokensMigrationsequenceDiagram
actor User
participant RowndHub as Rownd_events
participant Hook as useSuperTokensMigration
participant Sync as syncUserToSuperTokens
participant ST as SuperTokens_backend
User->>RowndHub: sign_in flow
RowndHub-->>Hook: events.addEventListener(sign_in_completed)
RowndHub->>Hook: emit sign_in_completed(Event)
Hook->>Hook: getSignInCompletedUserType(event)
Hook->>Hook: shouldMigrateSignIn(userType, authLevelRef.current)
alt shouldMigrateSignIn === true
Hook->>Hook: pendingMigrationRef.current = true
Hook->>Hook: flushPendingMigration()
Hook->>Sync: syncUserToSuperTokens(accessTokenRef.current, supertokensAppInfoRef.current)
Sync->>ST: POST /auth/plugin/rownd/migrate
else shouldMigrateSignIn === false
Hook-->>RowndHub: ignore event
end
Hook-->>Hook: useEffect(accessToken, authLevel)
Hook-->>Hook: useEffect(appInfo)
Hook->>Hook: flushPendingMigration()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
useSuperTokensMigration,syncUserToSuperTokensis now called without the previous.catch(() => {})guard, which means network or server errors will surface as unhandled promise rejections; consider adding explicit error handling (e.g., try/catch, logging, or a controlled swallow) to keep behavior consistent and avoid noisy runtime errors. - The
useEffectthat syncssupertokensAppInfoRefinuseSuperTokensMigrationonly depends onappInfo?.appName,appInfo?.apiDomain, andappInfo?.apiBasePath; if other relevant fields are added toappInfoin the future they won’t trigger an update, so it may be more robust to depend on theappInfoobject (orsupertokensprop) directly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useSuperTokensMigration`, `syncUserToSuperTokens` is now called without the previous `.catch(() => {})` guard, which means network or server errors will surface as unhandled promise rejections; consider adding explicit error handling (e.g., try/catch, logging, or a controlled swallow) to keep behavior consistent and avoid noisy runtime errors.
- The `useEffect` that syncs `supertokensAppInfoRef` in `useSuperTokensMigration` only depends on `appInfo?.appName`, `appInfo?.apiDomain`, and `appInfo?.apiBasePath`; if other relevant fields are added to `appInfo` in the future they won’t trigger an update, so it may be more robust to depend on the `appInfo` object (or `supertokens` prop) directly.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
mhamann
reviewed
Jun 2, 2026
Contributor
mhamann
left a comment
mhamann
left a comment
There was a problem hiding this comment.
Solid, well-tested refactor: the migration logic is extracted into a clear shouldMigrateSignIn predicate, and the pending/flush pattern is a clean way to defer until the access token + appInfo are ready. The two new tests cover the race fix and the converted-instant path nicely. One correctness question on event ordering that I'd want answered before merge, plus a couple of nits.
Correctness
- Converted-instant migration depends on event/state ordering — the existing_user path only arms a migration if
sign_in_completedis observed whileauthLevelRef.currentis still'instant'. If the hub propagatesauth_level: 'verified'into the ref before the event handler runs, the user is silently skipped. The test forces the favorable ordering by firing the event before the rerender — see the line comment onuseSuperTokensMigration.ts:84.
Nits
- Dropped
voidin front of the fire-and-forgetsyncUserToSuperTokenscall (line 66). Harmless here since the util swallows its own errors, but it removes the explicit "intentional floating promise" marker. See line comment. flushPendingMigrationis recreated each render and called inside threeuseEffects without being in their dep arrays —react-hooks/exhaustive-depswill flag this. It's fine at runtime (it only reads refs), but worth a// eslint-disableor wrapping inuseCallbackto keep lint clean.
Tests
- The two added tests are good. Consider one more guarding the ordering concern above: fire
sign_in_completed(existing_user)after the component has already rerendered toauthLevel: 'verified', and assert whatever the intended behavior is — that pins down the contract.
Nothing here is a hard blocker; mainly want the ordering question confirmed.
mhamann
approved these changes
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Extract SuperTokens migration logic into a reusable hook and extend it to handle instant-to-normal user migration, while adding a Next.js example app for validating the flow.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: