From 887c5e735dff25213effae6d61f102535ad8f0f0 Mon Sep 17 00:00:00 2001 From: Jacek Date: Tue, 3 Feb 2026 15:56:00 -0600 Subject: [PATCH 1/3] test(clerk-js): add tests for shouldIgnoreNullUpdate behavior in State class --- .../clerk-js/src/core/__tests__/state.test.ts | 239 ++++++++++++++++++ 1 file changed, 239 insertions(+) create mode 100644 packages/clerk-js/src/core/__tests__/state.test.ts diff --git a/packages/clerk-js/src/core/__tests__/state.test.ts b/packages/clerk-js/src/core/__tests__/state.test.ts new file mode 100644 index 00000000000..bcbad8c5bb7 --- /dev/null +++ b/packages/clerk-js/src/core/__tests__/state.test.ts @@ -0,0 +1,239 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { eventBus } from '../events'; +import { SignIn } from '../resources/SignIn'; +import { SignUp } from '../resources/SignUp'; +import { signInResourceSignal, signUpResourceSignal } from '../signals'; +import { State } from '../state'; + +describe('State', () => { + let state: State; + + beforeEach(() => { + // Reset signals to initial state + signUpResourceSignal({ resource: null }); + signInResourceSignal({ resource: null }); + // Create a new State instance which registers event handlers + state = new State(); + }); + + afterEach(() => { + vi.clearAllMocks(); + // Reset signals after each test + signUpResourceSignal({ resource: null }); + signInResourceSignal({ resource: null }); + }); + + describe('shouldIgnoreNullUpdate behavior', () => { + describe('SignUp', () => { + it('should allow first resource update when previous resource is null', () => { + // Arrange: Signal starts with null + expect(signUpResourceSignal().resource).toBeNull(); + + // Act: Emit a resource update with a SignUp that has an id + const signUp = new SignUp({ id: 'signup_123', status: 'missing_requirements' } as any); + + // Assert: Signal should be updated + expect(signUpResourceSignal().resource).toBe(signUp); + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + }); + + it('should ignore null resource update when previous resource exists and canBeDiscarded is false', () => { + // Arrange: Set up a SignUp with id and canBeDiscarded = false (default) + const existingSignUp = new SignUp({ id: 'signup_123', status: 'missing_requirements' } as any); + expect(signUpResourceSignal().resource).toBe(existingSignUp); + expect(existingSignUp.__internal_future.canBeDiscarded).toBe(false); + + // Act: Emit a resource update with a null SignUp (simulating client refresh with null sign_up) + const nullSignUp = new SignUp(null); + + // Assert: Signal should NOT be updated - should still have the existing SignUp + expect(signUpResourceSignal().resource).toBe(existingSignUp); + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + }); + + it('should allow null resource update when previous resource exists and canBeDiscarded is true', () => { + // Arrange: Set up a SignUp with id + const existingSignUp = new SignUp({ + id: 'signup_123', + status: 'complete', + created_session_id: 'session_123', + } as any); + expect(signUpResourceSignal().resource).toBe(existingSignUp); + + // Simulate finalize() being called - which sets canBeDiscarded to true + // We need to access the private field, so we'll use a workaround + // by calling the internal method that sets it + const mockSetActive = vi.fn().mockResolvedValue({}); + SignUp.clerk = { setActive: mockSetActive } as any; + + // Note: We can't easily call finalize() here because it's async and calls setActive + // Instead, let's directly test the scenario by creating a new SignUp and checking behavior + + // For this test, we'll verify the opposite case is working + // The null update should be blocked when canBeDiscarded is false + }); + + it('should allow null resource update after reset() is called', async () => { + // Arrange: Set up mock client + const mockClient = { + signUp: new SignUp(null), + resetSignUp: vi.fn().mockImplementation(function (this: typeof mockClient) { + const newSignUp = new SignUp(null); + this.signUp = newSignUp; + eventBus.emit('resource:error', { resource: newSignUp, error: null }); + }), + }; + SignUp.clerk = { client: mockClient } as any; + + // Create a SignUp with id + const existingSignUp = new SignUp({ id: 'signup_123', status: 'missing_requirements' } as any); + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + + // Act: Call reset() - this sets canBeDiscarded to true before resetting + await existingSignUp.__internal_future.reset(); + + // Assert: After reset, the signal should have a new null SignUp + // Note: reset() calls client.resetSignUp() which emits resource:error, not resource:update + // So the signal update happens through a different path + expect(mockClient.resetSignUp).toHaveBeenCalled(); + }); + + it('should allow resource update when new resource has an id (not a null update)', () => { + // Arrange: Set up a SignUp with id + const existingSignUp = new SignUp({ id: 'signup_123', status: 'missing_requirements' } as any); + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + + // Act: Emit a resource update with a different SignUp that also has an id + const newSignUp = new SignUp({ id: 'signup_456', status: 'complete' } as any); + + // Assert: Signal should be updated with the new SignUp + expect(signUpResourceSignal().resource).toBe(newSignUp); + expect(signUpResourceSignal().resource?.id).toBe('signup_456'); + }); + }); + + describe('SignIn', () => { + it('should allow first resource update when previous resource is null', () => { + // Arrange: Signal starts with null + expect(signInResourceSignal().resource).toBeNull(); + + // Act: Emit a resource update with a SignIn that has an id + const signIn = new SignIn({ id: 'signin_123', status: 'needs_identifier' } as any); + + // Assert: Signal should be updated + expect(signInResourceSignal().resource).toBe(signIn); + expect(signInResourceSignal().resource?.id).toBe('signin_123'); + }); + + it('should ignore null resource update when previous resource exists and canBeDiscarded is false', () => { + // Arrange: Set up a SignIn with id and canBeDiscarded = false (default) + const existingSignIn = new SignIn({ id: 'signin_123', status: 'needs_identifier' } as any); + expect(signInResourceSignal().resource).toBe(existingSignIn); + expect(existingSignIn.__internal_future.canBeDiscarded).toBe(false); + + // Act: Emit a resource update with a null SignIn (simulating client refresh with null sign_in) + const nullSignIn = new SignIn(null); + + // Assert: Signal should NOT be updated - should still have the existing SignIn + expect(signInResourceSignal().resource).toBe(existingSignIn); + expect(signInResourceSignal().resource?.id).toBe('signin_123'); + }); + }); + }); + + describe('Simulated client refresh scenarios', () => { + it('should protect against null update during email verification (before finalize)', () => { + // This test simulates the scenario described in USER-4372: + // 1. User is in the middle of sign up with verified email + // 2. Client refresh happens (e.g., router.refresh in Next.js) + // 3. Server returns sign_up: null because sign up is complete on server side + // 4. The null update should be IGNORED to prevent flash of default form + + // Arrange: User has completed email verification but hasn't called finalize yet + const signUp = new SignUp({ + id: 'signup_123', + status: 'complete', + created_session_id: 'session_123', + verifications: { + email_address: { status: 'verified' }, + }, + } as any); + + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + expect(signUp.__internal_future.canBeDiscarded).toBe(false); + + // Act: Simulate client refresh returning null sign_up + // This is what happens in Client.fromJSON when server returns sign_up: null + const nullSignUp = new SignUp(null); + + // Assert: The null update should be IGNORED + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + expect(signUpResourceSignal().resource).toBe(signUp); + }); + + it('should allow null update after finalize sets canBeDiscarded to true', async () => { + // This test verifies that after finalize() is called, + // the null update IS allowed (because canBeDiscarded is true) + + // Arrange + const mockSetActive = vi.fn().mockResolvedValue({}); + SignUp.clerk = { setActive: mockSetActive } as any; + + const signUp = new SignUp({ + id: 'signup_123', + status: 'complete', + created_session_id: 'session_123', + } as any); + + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + expect(signUp.__internal_future.canBeDiscarded).toBe(false); + + // Act: Call finalize - this sets canBeDiscarded to true + await signUp.__internal_future.finalize(); + + // Verify canBeDiscarded is now true + expect(signUp.__internal_future.canBeDiscarded).toBe(true); + + // Now simulate a null update + const nullSignUp = new SignUp(null); + + // Assert: The null update SHOULD be allowed now + // Because canBeDiscarded is true, shouldIgnoreNullUpdate returns false + expect(signUpResourceSignal().resource).toBe(nullSignUp); + expect(signUpResourceSignal().resource?.id).toBeUndefined(); + }); + }); + + describe('Edge cases', () => { + it('should handle rapid successive updates correctly', () => { + // First update with valid SignUp + const signUp1 = new SignUp({ id: 'signup_1', status: 'missing_requirements' } as any); + expect(signUpResourceSignal().resource?.id).toBe('signup_1'); + + // Second update with another valid SignUp + const signUp2 = new SignUp({ id: 'signup_2', status: 'missing_requirements' } as any); + expect(signUpResourceSignal().resource?.id).toBe('signup_2'); + + // Null update should be ignored + const nullSignUp = new SignUp(null); + expect(signUpResourceSignal().resource?.id).toBe('signup_2'); + + // Another valid update should work + const signUp3 = new SignUp({ id: 'signup_3', status: 'complete' } as any); + expect(signUpResourceSignal().resource?.id).toBe('signup_3'); + }); + + it('should handle update with same instance correctly', () => { + // Create a SignUp + const signUp = new SignUp({ id: 'signup_123', status: 'missing_requirements' } as any); + expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + + // Manually emit update with the same instance (simulating fromJSON on same instance) + eventBus.emit('resource:update', { resource: signUp }); + + // Signal should still have the same instance + expect(signUpResourceSignal().resource).toBe(signUp); + }); + }); +}); From 6fd354366d934bb805098b83263863c7fa662deb Mon Sep 17 00:00:00 2001 From: Jacek Date: Tue, 3 Feb 2026 19:29:20 -0600 Subject: [PATCH 2/3] fix: restore SignUp.clerk and SignIn.clerk after tests to prevent global state leakage --- packages/clerk-js/src/core/__tests__/state.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/clerk-js/src/core/__tests__/state.test.ts b/packages/clerk-js/src/core/__tests__/state.test.ts index bcbad8c5bb7..d881ca29d76 100644 --- a/packages/clerk-js/src/core/__tests__/state.test.ts +++ b/packages/clerk-js/src/core/__tests__/state.test.ts @@ -9,6 +9,10 @@ import { State } from '../state'; describe('State', () => { let state: State; + // Capture original static clerk references to restore after tests + const originalSignUpClerk = SignUp.clerk; + const originalSignInClerk = SignIn.clerk; + beforeEach(() => { // Reset signals to initial state signUpResourceSignal({ resource: null }); @@ -22,6 +26,9 @@ describe('State', () => { // Reset signals after each test signUpResourceSignal({ resource: null }); signInResourceSignal({ resource: null }); + // Restore original clerk references to prevent global state leakage + SignUp.clerk = originalSignUpClerk; + SignIn.clerk = originalSignInClerk; }); describe('shouldIgnoreNullUpdate behavior', () => { From a658ac58b4df8fc65b1fb03740f3897a4c3fb744 Mon Sep 17 00:00:00 2001 From: Jacek Date: Tue, 3 Feb 2026 19:30:52 -0600 Subject: [PATCH 3/3] test: add concrete assertions to verify null-update handling in shouldIgnoreNullUpdate tests --- .../clerk-js/src/core/__tests__/state.test.ts | 56 +++++++++++++------ 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/packages/clerk-js/src/core/__tests__/state.test.ts b/packages/clerk-js/src/core/__tests__/state.test.ts index d881ca29d76..85c79bb94f8 100644 --- a/packages/clerk-js/src/core/__tests__/state.test.ts +++ b/packages/clerk-js/src/core/__tests__/state.test.ts @@ -59,36 +59,48 @@ describe('State', () => { expect(signUpResourceSignal().resource?.id).toBe('signup_123'); }); - it('should allow null resource update when previous resource exists and canBeDiscarded is true', () => { - // Arrange: Set up a SignUp with id + it('should allow null resource update when previous resource exists and canBeDiscarded is true', async () => { + // Arrange: Set up a SignUp with id and mock setActive + const mockSetActive = vi.fn().mockResolvedValue({}); + SignUp.clerk = { setActive: mockSetActive } as any; + const existingSignUp = new SignUp({ id: 'signup_123', status: 'complete', created_session_id: 'session_123', } as any); expect(signUpResourceSignal().resource).toBe(existingSignUp); + expect(existingSignUp.__internal_future.canBeDiscarded).toBe(false); - // Simulate finalize() being called - which sets canBeDiscarded to true - // We need to access the private field, so we'll use a workaround - // by calling the internal method that sets it - const mockSetActive = vi.fn().mockResolvedValue({}); - SignUp.clerk = { setActive: mockSetActive } as any; + // Act: Call finalize() which sets canBeDiscarded to true + await existingSignUp.__internal_future.finalize(); + + // Verify canBeDiscarded is now true + expect(existingSignUp.__internal_future.canBeDiscarded).toBe(true); + expect(mockSetActive).toHaveBeenCalledWith({ session: 'session_123', navigate: undefined }); - // Note: We can't easily call finalize() here because it's async and calls setActive - // Instead, let's directly test the scenario by creating a new SignUp and checking behavior + // Now emit a null resource update (simulating client refresh with null sign_up) + const nullSignUp = new SignUp(null); - // For this test, we'll verify the opposite case is working - // The null update should be blocked when canBeDiscarded is false + // Assert: The null update SHOULD be allowed because canBeDiscarded is true + // shouldIgnoreNullUpdate returns false when canBeDiscarded is true + expect(signUpResourceSignal().resource).toBe(nullSignUp); + expect(signUpResourceSignal().resource?.id).toBeUndefined(); }); it('should allow null resource update after reset() is called', async () => { - // Arrange: Set up mock client + // Arrange: Set up mock client that tracks the new SignUp created during reset + let newSignUpFromReset: SignUp | null = null; const mockClient = { signUp: new SignUp(null), resetSignUp: vi.fn().mockImplementation(function (this: typeof mockClient) { - const newSignUp = new SignUp(null); - this.signUp = newSignUp; - eventBus.emit('resource:error', { resource: newSignUp, error: null }); + newSignUpFromReset = new SignUp(null); + this.signUp = newSignUpFromReset; + // reset() emits resource:error to clear errors, but the signal update + // happens via resource:update when the new SignUp is created + eventBus.emit('resource:error', { resource: newSignUpFromReset, error: null }); + // Emit resource:update to update the signal (simulating what happens in real flow) + eventBus.emit('resource:update', { resource: newSignUpFromReset }); }), }; SignUp.clerk = { client: mockClient } as any; @@ -96,14 +108,22 @@ describe('State', () => { // Create a SignUp with id const existingSignUp = new SignUp({ id: 'signup_123', status: 'missing_requirements' } as any); expect(signUpResourceSignal().resource?.id).toBe('signup_123'); + expect(existingSignUp.__internal_future.canBeDiscarded).toBe(false); // Act: Call reset() - this sets canBeDiscarded to true before resetting await existingSignUp.__internal_future.reset(); - // Assert: After reset, the signal should have a new null SignUp - // Note: reset() calls client.resetSignUp() which emits resource:error, not resource:update - // So the signal update happens through a different path + // Assert: Verify reset was called expect(mockClient.resetSignUp).toHaveBeenCalled(); + + // Assert: Verify canBeDiscarded was set to true on the original SignUp + expect(existingSignUp.__internal_future.canBeDiscarded).toBe(true); + + // Assert: Verify the signal was updated with a new SignUp that has no id + // The previous id 'signup_123' should be gone + expect(signUpResourceSignal().resource).toBe(newSignUpFromReset); + expect(signUpResourceSignal().resource?.id).toBeUndefined(); + expect(signUpResourceSignal().resource).not.toBe(existingSignUp); }); it('should allow resource update when new resource has an id (not a null update)', () => {