From f250bc7e88498b53ca2562dec4a951fb644367c5 Mon Sep 17 00:00:00 2001 From: Anshuman Pandey <54475686+pandeymangg@users.noreply.github.com> Date: Tue, 17 Mar 2026 14:27:07 +0530 Subject: [PATCH] fix: fixes race between setUserId and trigger (#7498) Co-authored-by: Dhruwang --- .../src/lib/survey/tests/widget.test.ts | 272 ++++++++++++++++++ packages/js-core/src/lib/survey/widget.ts | 19 ++ .../src/lib/user/tests/update-queue.test.ts | 100 +++++++ packages/js-core/src/lib/user/update-queue.ts | 37 ++- 4 files changed, 427 insertions(+), 1 deletion(-) diff --git a/packages/js-core/src/lib/survey/tests/widget.test.ts b/packages/js-core/src/lib/survey/tests/widget.test.ts index ff83be2ebb..57c5751d45 100644 --- a/packages/js-core/src/lib/survey/tests/widget.test.ts +++ b/packages/js-core/src/lib/survey/tests/widget.test.ts @@ -43,6 +43,17 @@ vi.mock("@/lib/common/utils", () => ({ handleHiddenFields: vi.fn(), })); +const mockUpdateQueue = { + hasPendingWork: vi.fn().mockReturnValue(false), + waitForPendingWork: vi.fn().mockResolvedValue(true), +}; + +vi.mock("@/lib/user/update-queue", () => ({ + UpdateQueue: { + getInstance: vi.fn(() => mockUpdateQueue), + }, +})); + describe("widget-file", () => { let getInstanceConfigMock: MockInstance<() => Config>; let getInstanceLoggerMock: MockInstance<() => Logger>; @@ -249,4 +260,265 @@ describe("widget-file", () => { widget.removeWidgetContainer(); expect(document.getElementById("formbricks-container")).toBeFalsy(); }); + + test("renderWidget waits for pending identification before rendering", async () => { + mockUpdateQueue.hasPendingWork.mockReturnValue(true); + mockUpdateQueue.waitForPendingWork.mockResolvedValue(true); + + const mockConfigValue = { + get: vi.fn().mockReturnValue({ + appUrl: "https://fake.app", + environmentId: "env_123", + environment: { + data: { + project: { + clickOutsideClose: true, + overlay: "none", + placement: "bottomRight", + inAppSurveyBranding: true, + }, + }, + }, + user: { + data: { + userId: "user_abc", + contactId: "contact_abc", + displays: [], + responses: [], + lastDisplayAt: null, + language: "en", + }, + }, + }), + update: vi.fn(), + }; + + getInstanceConfigMock.mockReturnValue(mockConfigValue as unknown as Config); + widget.setIsSurveyRunning(false); + + // @ts-expect-error -- mock window.formbricksSurveys + window.formbricksSurveys = { + renderSurvey: vi.fn(), + }; + + vi.useFakeTimers(); + + await widget.renderWidget({ + ...mockSurvey, + delay: 0, + } as unknown as TEnvironmentStateSurvey); + + expect(mockUpdateQueue.hasPendingWork).toHaveBeenCalled(); + expect(mockUpdateQueue.waitForPendingWork).toHaveBeenCalled(); + + vi.advanceTimersByTime(0); + + expect(window.formbricksSurveys.renderSurvey).toHaveBeenCalledWith( + expect.objectContaining({ + contactId: "contact_abc", + }) + ); + + vi.useRealTimers(); + }); + + test("renderWidget does not wait when no identification is pending", async () => { + mockUpdateQueue.hasPendingWork.mockReturnValue(false); + + const mockConfigValue = { + get: vi.fn().mockReturnValue({ + appUrl: "https://fake.app", + environmentId: "env_123", + environment: { + data: { + project: { + clickOutsideClose: true, + overlay: "none", + placement: "bottomRight", + inAppSurveyBranding: true, + }, + }, + }, + user: { + data: { + userId: "user_abc", + contactId: "contact_abc", + displays: [], + responses: [], + lastDisplayAt: null, + language: "en", + }, + }, + }), + update: vi.fn(), + }; + + getInstanceConfigMock.mockReturnValue(mockConfigValue as unknown as Config); + widget.setIsSurveyRunning(false); + + // @ts-expect-error -- mock window.formbricksSurveys + window.formbricksSurveys = { + renderSurvey: vi.fn(), + }; + + vi.useFakeTimers(); + + await widget.renderWidget({ + ...mockSurvey, + delay: 0, + } as unknown as TEnvironmentStateSurvey); + + expect(mockUpdateQueue.hasPendingWork).toHaveBeenCalled(); + expect(mockUpdateQueue.waitForPendingWork).not.toHaveBeenCalled(); + + vi.advanceTimersByTime(0); + expect(window.formbricksSurveys.renderSurvey).toHaveBeenCalled(); + + vi.useRealTimers(); + }); + + test("renderWidget reads contactId after identification wait completes", async () => { + let callCount = 0; + const mockConfigValue = { + get: vi.fn().mockImplementation(() => { + callCount++; + return { + appUrl: "https://fake.app", + environmentId: "env_123", + environment: { + data: { + project: { + clickOutsideClose: true, + overlay: "none", + placement: "bottomRight", + inAppSurveyBranding: true, + }, + }, + }, + user: { + data: { + // Simulate contactId becoming available after identification + userId: "user_abc", + contactId: callCount > 2 ? "contact_after_identification" : undefined, + displays: [], + responses: [], + lastDisplayAt: null, + language: "en", + }, + }, + }; + }), + update: vi.fn(), + }; + + getInstanceConfigMock.mockReturnValue(mockConfigValue as unknown as Config); + mockUpdateQueue.hasPendingWork.mockReturnValue(true); + mockUpdateQueue.waitForPendingWork.mockResolvedValue(true); + widget.setIsSurveyRunning(false); + + // @ts-expect-error -- mock window.formbricksSurveys + window.formbricksSurveys = { + renderSurvey: vi.fn(), + }; + + vi.useFakeTimers(); + + await widget.renderWidget({ + ...mockSurvey, + delay: 0, + } as unknown as TEnvironmentStateSurvey); + + vi.advanceTimersByTime(0); + + // The contactId passed to renderSurvey should be read after the wait + expect(window.formbricksSurveys.renderSurvey).toHaveBeenCalledWith( + expect.objectContaining({ + contactId: "contact_after_identification", + }) + ); + + vi.useRealTimers(); + }); + + test("renderWidget skips survey when identification fails and survey has segment filters", async () => { + mockUpdateQueue.hasPendingWork.mockReturnValue(true); + mockUpdateQueue.waitForPendingWork.mockResolvedValue(false); + + widget.setIsSurveyRunning(false); + + // @ts-expect-error -- mock window.formbricksSurveys + window.formbricksSurveys = { + renderSurvey: vi.fn(), + }; + + await widget.renderWidget({ + ...mockSurvey, + delay: 0, + segment: { id: "seg_1", filters: [{ type: "attribute", value: "plan" }] }, + } as unknown as TEnvironmentStateSurvey); + + expect(mockUpdateQueue.waitForPendingWork).toHaveBeenCalled(); + expect(mockLogger.debug).toHaveBeenCalledWith( + "User identification failed. Skipping survey with segment filters." + ); + expect(window.formbricksSurveys.renderSurvey).not.toHaveBeenCalled(); + }); + + test("renderWidget proceeds when identification fails but survey has no segment filters", async () => { + mockUpdateQueue.hasPendingWork.mockReturnValue(true); + mockUpdateQueue.waitForPendingWork.mockResolvedValue(false); + + const mockConfigValue = { + get: vi.fn().mockReturnValue({ + appUrl: "https://fake.app", + environmentId: "env_123", + environment: { + data: { + project: { + clickOutsideClose: true, + overlay: "none", + placement: "bottomRight", + inAppSurveyBranding: true, + }, + }, + }, + user: { + data: { + userId: null, + contactId: null, + displays: [], + responses: [], + lastDisplayAt: null, + language: "en", + }, + }, + }), + update: vi.fn(), + }; + + getInstanceConfigMock.mockReturnValue(mockConfigValue as unknown as Config); + widget.setIsSurveyRunning(false); + + // @ts-expect-error -- mock window.formbricksSurveys + window.formbricksSurveys = { + renderSurvey: vi.fn(), + }; + + vi.useFakeTimers(); + + await widget.renderWidget({ + ...mockSurvey, + delay: 0, + segment: undefined, + } as unknown as TEnvironmentStateSurvey); + + expect(mockLogger.debug).toHaveBeenCalledWith( + "User identification failed but survey has no segment filters. Proceeding." + ); + + vi.advanceTimersByTime(0); + expect(window.formbricksSurveys.renderSurvey).toHaveBeenCalled(); + + vi.useRealTimers(); + }); }); diff --git a/packages/js-core/src/lib/survey/widget.ts b/packages/js-core/src/lib/survey/widget.ts index d6200617b8..b7bc03eae3 100644 --- a/packages/js-core/src/lib/survey/widget.ts +++ b/packages/js-core/src/lib/survey/widget.ts @@ -11,6 +11,7 @@ import { handleHiddenFields, shouldDisplayBasedOnPercentage, } from "@/lib/common/utils"; +import { UpdateQueue } from "@/lib/user/update-queue"; import { type TEnvironmentStateSurvey, type TUserState } from "@/types/config"; import { type TTrackProperties } from "@/types/survey"; @@ -60,6 +61,24 @@ export const renderWidget = async ( setIsSurveyRunning(true); + // Wait for pending user identification to complete before rendering + const updateQueue = UpdateQueue.getInstance(); + if (updateQueue.hasPendingWork()) { + logger.debug("Waiting for pending user identification before rendering survey"); + const identificationSucceeded = await updateQueue.waitForPendingWork(); + if (!identificationSucceeded) { + const hasSegmentFilters = Array.isArray(survey.segment?.filters) && survey.segment.filters.length > 0; + + if (hasSegmentFilters) { + logger.debug("User identification failed. Skipping survey with segment filters."); + setIsSurveyRunning(false); + return; + } + + logger.debug("User identification failed but survey has no segment filters. Proceeding."); + } + } + if (survey.delay) { logger.debug(`Delaying survey "${survey.name}" by ${survey.delay.toString()} seconds.`); } diff --git a/packages/js-core/src/lib/user/tests/update-queue.test.ts b/packages/js-core/src/lib/user/tests/update-queue.test.ts index 40318a85b4..de95ed178c 100644 --- a/packages/js-core/src/lib/user/tests/update-queue.test.ts +++ b/packages/js-core/src/lib/user/tests/update-queue.test.ts @@ -169,4 +169,104 @@ describe("UpdateQueue", () => { "Formbricks can't set attributes without a userId! Please set a userId first with the setUserId function" ); }); + + test("hasPendingWork returns false when no updates and no flush in flight", () => { + expect(updateQueue.hasPendingWork()).toBe(false); + }); + + test("hasPendingWork returns true when updates are queued", () => { + updateQueue.updateUserId(mockUserId1); + expect(updateQueue.hasPendingWork()).toBe(true); + }); + + test("hasPendingWork returns true while processUpdates flush is in flight", () => { + (sendUpdates as Mock).mockReturnValue({ + ok: true, + data: { hasWarnings: false }, + }); + + updateQueue.updateUserId(mockUserId1); + // Start processing but don't await — the debounce means the flush is in-flight + void updateQueue.processUpdates(); + + expect(updateQueue.hasPendingWork()).toBe(true); + }); + + test("waitForPendingWork returns true immediately when no pending work", async () => { + const result = await updateQueue.waitForPendingWork(); + expect(result).toBe(true); + }); + + test("waitForPendingWork returns true when processUpdates succeeds", async () => { + (sendUpdates as Mock).mockReturnValue({ + ok: true, + data: { hasWarnings: false }, + }); + + updateQueue.updateUserId(mockUserId1); + void updateQueue.processUpdates(); + + const result = await updateQueue.waitForPendingWork(); + + expect(result).toBe(true); + expect(updateQueue.hasPendingWork()).toBe(false); + expect(sendUpdates).toHaveBeenCalled(); + }); + + test("waitForPendingWork returns false when processUpdates rejects", async () => { + loggerMock.mockReturnValue(mockLogger as unknown as Logger); + (sendUpdates as Mock).mockRejectedValue(new Error("network error")); + + updateQueue.updateUserId(mockUserId1); + // eslint-disable-next-line @typescript-eslint/no-empty-function -- intentionally swallowing rejection to avoid unhandled promise + const processPromise = updateQueue.processUpdates().catch(() => {}); + + const result = await updateQueue.waitForPendingWork(); + expect(result).toBe(false); + await processPromise; + }); + + test("waitForPendingWork returns false when flush hangs past timeout", async () => { + vi.useFakeTimers(); + + // sendUpdates returns a promise that never resolves, simulating a network hang + // eslint-disable-next-line @typescript-eslint/no-empty-function -- intentionally never-resolving promise + (sendUpdates as Mock).mockReturnValue(new Promise(() => {})); + + updateQueue.updateUserId(mockUserId1); + void updateQueue.processUpdates(); + + const resultPromise = updateQueue.waitForPendingWork(); + + // Advance past the debounce delay (500ms) so the handler fires and hangs on sendUpdates + await vi.advanceTimersByTimeAsync(500); + // Advance past the pending work timeout (5000ms) + await vi.advanceTimersByTimeAsync(5000); + + const result = await resultPromise; + expect(result).toBe(false); + + vi.useRealTimers(); + }); + + test("processUpdates reuses pending flush instead of creating orphaned promises", async () => { + (sendUpdates as Mock).mockReturnValue({ + ok: true, + data: { hasWarnings: false }, + }); + + updateQueue.updateUserId(mockUserId1); + + // First call creates the flush promise + const firstPromise = updateQueue.processUpdates(); + + // Second call while first is still pending should not create a new flush + updateQueue.updateAttributes({ name: mockAttributes.name }); + const secondPromise = updateQueue.processUpdates(); + + // Both promises should resolve (second is not orphaned) + await Promise.all([firstPromise, secondPromise]); + + expect(updateQueue.hasPendingWork()).toBe(false); + }); }); diff --git a/packages/js-core/src/lib/user/update-queue.ts b/packages/js-core/src/lib/user/update-queue.ts index c85286d129..41725a79d5 100644 --- a/packages/js-core/src/lib/user/update-queue.ts +++ b/packages/js-core/src/lib/user/update-queue.ts @@ -8,7 +8,9 @@ export class UpdateQueue { private static instance: UpdateQueue | null = null; private updates: TUpdates | null = null; private debounceTimeout: NodeJS.Timeout | null = null; + private pendingFlush: Promise | null = null; private readonly DEBOUNCE_DELAY = 500; + private readonly PENDING_WORK_TIMEOUT = 5000; private constructor() {} @@ -63,17 +65,45 @@ export class UpdateQueue { return !this.updates; } + public hasPendingWork(): boolean { + return this.updates !== null || this.pendingFlush !== null; + } + + public async waitForPendingWork(): Promise { + if (!this.hasPendingWork()) return true; + + const flush = this.pendingFlush ?? this.processUpdates(); + try { + const succeeded = await Promise.race([ + flush.then(() => true as const), + new Promise((resolve) => { + setTimeout(() => { + resolve(false); + }, this.PENDING_WORK_TIMEOUT); + }), + ]); + return succeeded; + } catch { + return false; + } + } + public async processUpdates(): Promise { const logger = Logger.getInstance(); if (!this.updates) { return; } + // If a flush is already in flight, reuse it instead of creating a new promise + if (this.pendingFlush) { + return this.pendingFlush; + } + if (this.debounceTimeout) { clearTimeout(this.debounceTimeout); } - return new Promise((resolve, reject) => { + const flushPromise = new Promise((resolve, reject) => { const handler = async (): Promise => { try { let currentUpdates = { ...this.updates }; @@ -147,8 +177,10 @@ export class UpdateQueue { } this.clearUpdates(); + this.pendingFlush = null; resolve(); } catch (error: unknown) { + this.pendingFlush = null; logger.error( `Failed to process updates: ${error instanceof Error ? error.message : "Unknown error"}` ); @@ -158,5 +190,8 @@ export class UpdateQueue { this.debounceTimeout = setTimeout(() => void handler(), this.DEBOUNCE_DELAY); }); + + this.pendingFlush = flushPromise; + return flushPromise; } }