test(clerk-js): add tests for null resource update behavior#7753
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 553d6a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@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: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new test suite was added at packages/clerk-js/src/core/tests/state.test.ts covering Clerk JS State flow for SignUp and SignIn resources. Tests exercise initialization/reset of State, eventBus signaling, handling of null updates (first-from-null, ignored when previous resource cannot be discarded, allowed after finalize/reset flows), replacements by non-null resources, rapid successive updates, same-instance updates, and restoration of static clerk references. 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. No actionable comments were generated in the recent review. 🎉 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.
78da62d to
f419da1
Compare
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