test(clerk-js): add tests verifying shouldIgnoreNullUpdate behavior#7753
test(clerk-js): add tests verifying shouldIgnoreNullUpdate behavior#7753jacekradko wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughAdds 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
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.
…bal state leakage
…dIgnoreNullUpdate tests
Summary
Adds tests for the
shouldIgnoreNullUpdatemechanism in theStateclass to verify the behavior described in USER-4372.Investigation Findings
The
shouldIgnoreNullUpdatefunction atpackages/clerk-js/src/core/state.ts:117-119is designed to prevent null resource updates from propagating when the previous resource has not been finalized:Tests verify that the mechanism works correctly:
nullcanBeDiscarded === falsefinalize()setscanBeDiscarded = trueKey Insight: Timing Issue in
finalize()The reported flash of the default sign-up form likely occurs due to the order of operations in
finalize():The sequence:
finalize()canBeDiscardedis set totrueimmediatelysetActive()is calledsetActive()triggersrouter.refresh()or navigationsign_up: nullcanBeDiscardedis alreadytrue, the null update is NOT ignoredRecommended Follow-up
If the flash occurs before
finalize()is called (as the issue title suggests), then either:previousResourceis unexpectedlynullConsider adding debug logging to capture:
shouldIgnoreNullUpdateis calledpreviousResourceandcanBeDiscardedvalues are at that momentTest plan
pnpm testpasses for new testsSummary by CodeRabbit
Note: No user-facing changes; this update only expands internal test coverage.