mirror of
https://github.com/formbricks/formbricks.git
synced 2026-04-20 19:30:41 -05:00
fix: make isRequestInProgress module-level and broaden post-await guard
Address CodeRabbit review feedback: - Move isRequestInProgress to a module-level Map (like isSyncing) so it survives ResponseQueue instance recreation across React re-renders. - Broaden the re-check after countPendingResponses await to also verify isRequestInProgress and queue.length, preventing a second processQueue call from slipping through during the yield. - Add cross-instance test verifying that a new ResponseQueue instance sees isRequestInProgress set by an older instance.
This commit is contained in:
@@ -32,23 +32,28 @@ export const delay = (ms: number): Promise<void> => {
|
||||
});
|
||||
};
|
||||
|
||||
// Module-level sync lock keyed by surveyId.
|
||||
// Survives ResponseQueue instance recreation (e.g. React useMemo recomputation)
|
||||
// so that only one sync runs at a time per survey, even across instances.
|
||||
// Module-level locks keyed by surveyId.
|
||||
// Survive ResponseQueue instance recreation (e.g. React useMemo recomputation)
|
||||
// so that only one sync/send runs at a time per survey, even across instances.
|
||||
const syncingBySurvey = new Map<string, boolean>();
|
||||
const requestInProgressBySurvey = new Map<string, boolean>();
|
||||
|
||||
/** @internal Exposed for tests only. */
|
||||
export const _syncLocks = {
|
||||
clear: () => syncingBySurvey.clear(),
|
||||
clear: () => {
|
||||
syncingBySurvey.clear();
|
||||
requestInProgressBySurvey.clear();
|
||||
},
|
||||
set: (surveyId: string, value: boolean) => syncingBySurvey.set(surveyId, value),
|
||||
get: (surveyId: string) => syncingBySurvey.get(surveyId) ?? false,
|
||||
setRequestInProgress: (surveyId: string, value: boolean) => requestInProgressBySurvey.set(surveyId, value),
|
||||
getRequestInProgress: (surveyId: string) => requestInProgressBySurvey.get(surveyId) ?? false,
|
||||
};
|
||||
|
||||
export class ResponseQueue {
|
||||
readonly queue: TResponseUpdate[] = [];
|
||||
readonly config: QueueConfig;
|
||||
private surveyState: SurveyState;
|
||||
private isRequestInProgress = false;
|
||||
readonly api: ApiClient;
|
||||
private responseRecaptchaToken?: string;
|
||||
// Maps in-memory queue index → IndexedDB id for cleanup after successful send
|
||||
@@ -73,6 +78,16 @@ export class ResponseQueue {
|
||||
}
|
||||
}
|
||||
|
||||
private get isRequestInProgress(): boolean {
|
||||
return this.config.surveyId ? (requestInProgressBySurvey.get(this.config.surveyId) ?? false) : false;
|
||||
}
|
||||
|
||||
private set isRequestInProgress(value: boolean) {
|
||||
if (this.config.surveyId) {
|
||||
requestInProgressBySurvey.set(this.config.surveyId, value);
|
||||
}
|
||||
}
|
||||
|
||||
setResponseRecaptchaToken(token?: string) {
|
||||
this.responseRecaptchaToken = token;
|
||||
}
|
||||
@@ -139,8 +154,8 @@ export class ResponseQueue {
|
||||
if (this.config.persistOffline && this.config.surveyId) {
|
||||
const pendingCount = await countPendingResponses(this.config.surveyId);
|
||||
|
||||
// Re-check after await — syncPersistedResponses may have started during the yield
|
||||
if (this.isSyncing) {
|
||||
// Re-check after await — another processQueue/sync may have started during the yield
|
||||
if (this.isSyncing || this.isRequestInProgress || this.queue.length === 0) {
|
||||
return { success: false };
|
||||
}
|
||||
|
||||
|
||||
@@ -338,7 +338,7 @@ describe("ResponseQueue", () => {
|
||||
const result = await offlineQueue.processQueue();
|
||||
expect(result.success).toBe(false);
|
||||
expect(syncSpy).toHaveBeenCalled();
|
||||
expect(offlineQueue["isRequestInProgress"]).toBe(false);
|
||||
expect(_syncLocks.getRequestInProgress("s1")).toBe(false);
|
||||
});
|
||||
|
||||
test("processQueue bails out if syncPersistedResponses starts during countPendingResponses await", async () => {
|
||||
@@ -417,15 +417,27 @@ describe("ResponseQueue", () => {
|
||||
});
|
||||
|
||||
test("syncPersistedResponses returns early when a processQueue request is in flight", async () => {
|
||||
_syncLocks.setRequestInProgress("s1", true);
|
||||
const offlineQueue = new ResponseQueue(
|
||||
getConfig({ persistOffline: true, surveyId: "s1" }),
|
||||
getSurveyState()
|
||||
);
|
||||
offlineQueue["isRequestInProgress"] = true;
|
||||
const result = await offlineQueue.syncPersistedResponses();
|
||||
expect(result).toEqual({ success: false, syncedCount: 0 });
|
||||
});
|
||||
|
||||
test("syncPersistedResponses on a new instance sees isRequestInProgress from an old instance", async () => {
|
||||
// Simulate instance A having a request in flight (module-level lock)
|
||||
_syncLocks.setRequestInProgress("s1", true);
|
||||
// Instance B is newly created (e.g. React useMemo recomputation)
|
||||
const instanceB = new ResponseQueue(
|
||||
getConfig({ persistOffline: true, surveyId: "s1" }),
|
||||
getSurveyState()
|
||||
);
|
||||
const result = await instanceB.syncPersistedResponses();
|
||||
expect(result).toEqual({ success: false, syncedCount: 0 });
|
||||
});
|
||||
|
||||
test("syncPersistedResponses sends entries and clears queue on success", async () => {
|
||||
const { getPendingResponses, removePendingResponse } = await import("./offline-storage");
|
||||
vi.mocked(getPendingResponses).mockResolvedValue([
|
||||
|
||||
Reference in New Issue
Block a user