Skip to content

test(clerk-js): add tests verifying shouldIgnoreNullUpdate behavior#7753

Open
jacekradko wants to merge 3 commits intomainfrom
jacek/user-4372-verify-null-resource-updates-when-previous-resource-is-not
Open

test(clerk-js): add tests verifying shouldIgnoreNullUpdate behavior#7753
jacekradko wants to merge 3 commits intomainfrom
jacek/user-4372-verify-null-resource-updates-when-previous-resource-is-not

Conversation

@jacekradko
Copy link
Member

@jacekradko jacekradko commented Feb 3, 2026

Summary

Adds tests for the shouldIgnoreNullUpdate mechanism in the State class to verify the behavior described in USER-4372.

Investigation Findings

The shouldIgnoreNullUpdate function at packages/clerk-js/src/core/state.ts:117-119 is designed to prevent null resource updates from propagating when the previous resource has not been finalized:

function shouldIgnoreNullUpdate(previousResource, newResource) {
  return !newResource?.id && previousResource && previousResource.__internal_future?.canBeDiscarded === false;
}

Tests verify that the mechanism works correctly:

  1. ✅ Allows first resource update when previous resource is null
  2. ✅ Ignores null update when previous resource exists and canBeDiscarded === false
  3. ✅ Allows null update after finalize() sets canBeDiscarded = true
  4. ✅ Allows updates when new resource has an id (not a null update)
  5. ✅ Handles rapid successive updates correctly
  6. ✅ Handles updates with same instance correctly

Key Insight: Timing Issue in finalize()

The reported flash of the default sign-up form likely occurs due to the order of operations in finalize():

async finalize(...) {
  this.#canBeDiscarded = true;  // ← Set BEFORE setActive
  await SignUp.clerk.setActive({ session: this.#resource.createdSessionId, navigate });
}

The sequence:

  1. User verifies email → sign up is complete
  2. User/code calls finalize()
  3. canBeDiscarded is set to true immediately
  4. setActive() is called
  5. In Next.js, setActive() triggers router.refresh() or navigation
  6. During refresh, client is re-fetched with sign_up: null
  7. Because canBeDiscarded is already true, the null update is NOT ignored
  8. Signal updates with empty SignUp → FLASH of default form
  9. Navigation completes

Recommended Follow-up

If the flash occurs before finalize() is called (as the issue title suggests), then either:

  1. There's a race condition where previousResource is unexpectedly null
  2. Something in Next.js is causing a full page reload that resets module-level signals

Consider adding debug logging to capture:

  • When shouldIgnoreNullUpdate is called
  • What previousResource and canBeDiscarded values are at that moment
  • The exact timing of the flash in relation to these events

Test plan

  • pnpm test passes for new tests
  • Tests cover the scenarios described in USER-4372

Summary by CodeRabbit

  • Tests
    • Added extensive test coverage for state signal behavior around sign-up and sign-in flows, including initialization, resets, null vs. non-null updates, finalize-like flows, client refresh scenarios, and various edge cases.

Note: No user-facing changes; this update only expands internal test coverage.

@vercel
Copy link

vercel bot commented Feb 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clerk-js-sandbox Ready Ready Preview, Comment Feb 4, 2026 1:31am

Request Review

@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2026

⚠️ No Changeset found

Latest commit: a658ac5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 3, 2026

Open in StackBlitz

@clerk/agent-toolkit

npm i https://pkg.pr.new/@clerk/agent-toolkit@7753

@clerk/astro

npm i https://pkg.pr.new/@clerk/astro@7753

@clerk/backend

npm i https://pkg.pr.new/@clerk/backend@7753

@clerk/chrome-extension

npm i https://pkg.pr.new/@clerk/chrome-extension@7753

@clerk/clerk-js

npm i https://pkg.pr.new/@clerk/clerk-js@7753

@clerk/dev-cli

npm i https://pkg.pr.new/@clerk/dev-cli@7753

@clerk/expo

npm i https://pkg.pr.new/@clerk/expo@7753

@clerk/expo-passkeys

npm i https://pkg.pr.new/@clerk/expo-passkeys@7753

@clerk/express

npm i https://pkg.pr.new/@clerk/express@7753

@clerk/fastify

npm i https://pkg.pr.new/@clerk/fastify@7753

@clerk/localizations

npm i https://pkg.pr.new/@clerk/localizations@7753

@clerk/nextjs

npm i https://pkg.pr.new/@clerk/nextjs@7753

@clerk/nuxt

npm i https://pkg.pr.new/@clerk/nuxt@7753

@clerk/react

npm i https://pkg.pr.new/@clerk/react@7753

@clerk/react-router

npm i https://pkg.pr.new/@clerk/react-router@7753

@clerk/shared

npm i https://pkg.pr.new/@clerk/shared@7753

@clerk/tanstack-react-start

npm i https://pkg.pr.new/@clerk/tanstack-react-start@7753

@clerk/testing

npm i https://pkg.pr.new/@clerk/testing@7753

@clerk/ui

npm i https://pkg.pr.new/@clerk/ui@7753

@clerk/upgrade

npm i https://pkg.pr.new/@clerk/upgrade@7753

@clerk/vue

npm i https://pkg.pr.new/@clerk/vue@7753

commit: a658ac5

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 3, 2026

📝 Walkthrough

Walkthrough

Adds a comprehensive test suite for the State module in Clerk JS that validates SignUp and SignIn resource-signal behavior. Tests cover initialization and reset of signals, handling of null resource updates depending on whether a previous resource is finalized/canBeDiscarded, replacement by non-null resources, simulated client-refresh scenarios returning null from the server, finalize/reset flows that flip discardability, and edge cases like rapid successive updates and same-instance updates. No public API signatures were changed.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding tests for shouldIgnoreNullUpdate behavior, matching the test suite additions in the changeset.
Linked Issues check ✅ Passed The PR successfully implements comprehensive tests covering all USER-4372 objectives: null update delays, finalize behavior, canBeDiscarded state transitions, and various edge cases.
Out of Scope Changes check ✅ Passed All changes are within scope—only test additions for shouldIgnoreNullUpdate behavior with no modifications to production code or unrelated functionality.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@packages/clerk-js/src/core/__tests__/state.test.ts`:
- Around line 12-25: Tests modify the static SignUp.clerk and never restore it,
causing global state leakage; capture the original value (e.g., const
_originalSignUpClerk = SignUp.clerk) before mutating in your test setup
(beforeEach or beforeAll) and reassign SignUp.clerk = _originalSignUpClerk in
afterEach along with the existing signUpResourceSignal({ resource: null }) /
signInResourceSignal({ resource: null }) and vi.clearAllMocks(); apply the same
snapshot-and-restore pattern wherever SignUp.clerk is overwritten in tests
(references: SignUp.clerk, State constructor that registers handlers,
signUpResourceSignal, signInResourceSignal).
- Around line 55-100: Add concrete assertions to verify null-update handling: in
the first test, call the code path that would trigger a null resource update
(e.g., simulate finalize() behavior or invoke the internal method that sets
canBeDiscarded on the existing SignUp) and assert that
signUpResourceSignal().resource becomes null (or remains the existing resource)
according to shouldIgnoreNullUpdate semantics; in the second test, after
awaiting existingSignUp.__internal_future.reset(), assert that
signUpResourceSignal().resource is a new SignUp with null/undefined id (or that
the previous id 'signup_123' is gone) in addition to
expect(mockClient.resetSignUp).toHaveBeenCalled(), so both the call and the
resulting signal update are validated for SignUp, signUpResourceSignal, and
__internal_future.reset.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant