From 34d50a21dcd8816163b54c19169f0dc61b84d865 Mon Sep 17 00:00:00 2001 From: David Nguyen Date: Wed, 1 Jul 2026 17:14:37 +1000 Subject: [PATCH] fix: improve editor autosave --- .../envelope-recipient-autosave-race.spec.ts | 199 ++++++++++++++++++ .../hooks/use-envelope-autosave.ts | 127 ++++++----- 2 files changed, 276 insertions(+), 50 deletions(-) create mode 100644 packages/app-tests/e2e/envelope-editor-v2/envelope-recipient-autosave-race.spec.ts diff --git a/packages/app-tests/e2e/envelope-editor-v2/envelope-recipient-autosave-race.spec.ts b/packages/app-tests/e2e/envelope-editor-v2/envelope-recipient-autosave-race.spec.ts new file mode 100644 index 000000000..1d1035527 --- /dev/null +++ b/packages/app-tests/e2e/envelope-editor-v2/envelope-recipient-autosave-race.spec.ts @@ -0,0 +1,199 @@ +import { prisma } from '@documenso/prisma'; +import { expect, type Page, test } from '@playwright/test'; + +import { + clickAddSignerButton, + clickEnvelopeEditorStep, + getRecipientEmailInputs, + openDocumentEnvelopeEditor, + setRecipientEmail, + setRecipientName, + type TEnvelopeEditorSurface, +} from '../fixtures/envelope-editor'; + +/** + * Reproduction for the recipient autosave race condition. + * + * Symptom (production only, where there is real network lag): + * 1. The author adds a recipient and types its name/email. + * 2. They navigate to the "Add Fields" step. + * 3. The recipient selector shows the default "Recipient 1" placeholder + * instead of the recipient they just typed, and the typed name/email is + * silently lost. + * + * Theory (see packages/lib/client-only/hooks/use-envelope-autosave.ts): + * When the author navigates, `flushAutosave()` is awaited before the Add + * Fields page renders. If an *earlier* (empty) recipient save is still + * in-flight at that moment, `flush()` awaits that in-flight save and returns + * WITHOUT committing the newer typed data sitting in `lastArgsRef` (whose + * debounce timer it just cleared). The typed data is dropped, the empty + * recipient persists, and the selector renders "Recipient 1". + * + * This only happens when a save is still in-flight at navigation time, which is + * why it never reproduces locally (fast saves) but does on a laggy network. + * + * The test below simulates that lag by holding the first `envelope.recipient.set` + * request open. It asserts the CORRECT behaviour (typed recipient survives), so + * it is RED while the bug exists and GREEN once the autosave hook is fixed. + */ + +const RECIPIENT_SET_PROCEDURE = 'envelope.recipient.set'; + +// How long to hold the first recipient autosave "in-flight" to emulate prod lag. +const SIMULATED_NETWORK_LAG_MS = 5000; + +const FIRST_RECIPIENT = { + name: 'Alice Author', + email: 'alice-autosave-race@example.com', +}; + +const SECOND_RECIPIENT = { + name: 'Bob Builder', + email: 'bob-autosave-race@example.com', +}; + +type RecipientSetLagHandle = { + /** Resolves the instant the first recipient.set request is in-flight on the client. */ + firstRecipientSetInFlight: Promise; + /** Raw request bodies of every recipient.set call we intercepted. */ + recipientSetRequestBodies: string[]; +}; + +/** + * Installs a fake "production network lag" on the recipient autosave mutation. + * + * Only the FIRST recipient.set request is held open for `lagMs` (this is the save + * that must still be in-flight at navigation time for the race to occur). It + * resolves `firstRecipientSetInFlight` the instant it is intercepted so the test + * can keep typing while that save is pending. Subsequent recipient.set requests + * (e.g. the follow-up save the fixed hook issues) are forwarded immediately so the + * test does not pay the lag twice. + */ +const installRecipientSetLag = async (page: Page, lagMs: number): Promise => { + let markFirstInFlight: () => void = () => {}; + + const firstRecipientSetInFlight = new Promise((resolve) => { + markFirstInFlight = resolve; + }); + + const recipientSetRequestBodies: string[] = []; + + await page.route('**/api/trpc/**', async (route) => { + const request = route.request(); + + if (request.method() !== 'POST' || !request.url().includes(RECIPIENT_SET_PROCEDURE)) { + await route.continue(); + return; + } + + const callIndex = recipientSetRequestBodies.length + 1; + recipientSetRequestBodies.push(request.postData() ?? ''); + + if (callIndex === 1) { + // eslint-disable-next-line no-console + console.log(`[test] holding first ${RECIPIENT_SET_PROCEDURE} for ${lagMs}ms (simulated network lag)`); + + // The empty save is now in-flight from the client's perspective. + markFirstInFlight(); + + await new Promise((resolve) => setTimeout(resolve, lagMs)); + } else { + // eslint-disable-next-line no-console + console.log(`[test] forwarding ${RECIPIENT_SET_PROCEDURE} #${callIndex} (no lag)`); + } + + await route.continue(); + }); + + return { firstRecipientSetInFlight, recipientSetRequestBodies }; +}; + +const assertEnvelopeRecipientsPersisted = async (surface: TEnvelopeEditorSurface) => { + if (!surface.envelopeId) { + throw new Error('Expected the document editor surface to have an envelopeId'); + } + + const envelope = await prisma.envelope.findFirstOrThrow({ + where: { id: surface.envelopeId }, + include: { + recipients: { + orderBy: { signingOrder: 'asc' }, + }, + }, + }); + + const persistedEmails = envelope.recipients.map((recipient) => recipient.email).filter(Boolean); + + // eslint-disable-next-line no-console + console.log( + '[test] persisted recipients:', + JSON.stringify( + envelope.recipients.map((recipient) => ({ name: recipient.name, email: recipient.email })), + null, + 2, + ), + ); + + expect(persistedEmails).toContain(FIRST_RECIPIENT.email); + expect(persistedEmails).toContain(SECOND_RECIPIENT.email); +}; + +test.describe('envelope editor recipient autosave race (network lag)', () => { + test('document editor: typed recipient survives navigation to Add Fields', async ({ page }) => { + const surface = await openDocumentEnvelopeEditor(page); + + const { firstRecipientSetInFlight, recipientSetRequestBodies } = await installRecipientSetLag( + page, + SIMULATED_NETWORK_LAG_MS, + ); + + // 1. Add a second signer row. A blank document already has one empty default + // signer, so this schedules an autosave of TWO empty recipients + // (name='' / email='') - this is the save that will be in-flight. + await clickAddSignerButton(surface.root); + await expect(getRecipientEmailInputs(surface.root)).toHaveCount(2); + + // 2. Wait until that empty autosave is actually in-flight on the client. This + // is the precondition the bug needs: a slow save holding the autosave lock. + await firstRecipientSetInFlight; + + // 3. The author now fills in the recipients they are adding. + await setRecipientName(surface.root, 0, FIRST_RECIPIENT.name); + await setRecipientEmail(surface.root, 0, FIRST_RECIPIENT.email); + await setRecipientName(surface.root, 1, SECOND_RECIPIENT.name); + await setRecipientEmail(surface.root, 1, SECOND_RECIPIENT.email); + + // 4. Immediately navigate to Add Fields (before the typed data's debounce + // fires). flushAutosave() awaits the in-flight EMPTY save; with the bug + // present it returns without ever committing the typed data. + await clickEnvelopeEditorStep(surface.root, 'addFields'); + + // 5. Wait for the Add Fields page to render (after the lagged flush resolves). + await expect(surface.root.getByText('Selected Recipient')).toBeVisible({ + timeout: SIMULATED_NETWORK_LAG_MS + 15000, + }); + + // Diagnostics - the request bodies show what actually reached the server. + // Buggy: only the first (empty) save is ever sent. Fixed: a follow-up save + // carrying the typed recipients is sent too. + // eslint-disable-next-line no-console + console.log('\n===== AUTOSAVE RACE DIAGNOSTICS ====='); + // eslint-disable-next-line no-console + console.log(`recipient.set requests sent to server: ${recipientSetRequestBodies.length}`); + // eslint-disable-next-line no-console + console.log( + `server ever received "${FIRST_RECIPIENT.email}": ${recipientSetRequestBodies.some((body) => body.includes(FIRST_RECIPIENT.email))}`, + ); + // eslint-disable-next-line no-console + console.log('=====================================\n'); + + // 6. THE USER-VISIBLE BUG: the selected recipient must be the one we typed + // (Alice), not the default "Recipient 1" placeholder. + const selectedRecipientSection = surface.root.locator('section').filter({ hasText: 'Selected Recipient' }); + + await expect(selectedRecipientSection.getByRole('combobox')).toContainText(FIRST_RECIPIENT.name); + + // 7. THE DATA LOSS: the typed recipients must actually be persisted. + await assertEnvelopeRecipientsPersisted(surface); + }); +}); diff --git a/packages/lib/client-only/hooks/use-envelope-autosave.ts b/packages/lib/client-only/hooks/use-envelope-autosave.ts index 1ca70c28c..c602c8d69 100644 --- a/packages/lib/client-only/hooks/use-envelope-autosave.ts +++ b/packages/lib/client-only/hooks/use-envelope-autosave.ts @@ -1,48 +1,97 @@ import { useCallback, useEffect, useRef, useState } from 'react'; +/** + * Debounced, self-serialising autosave helper. + * + * Guarantees: + * - At most one `saveFn` call is in-flight at any time, so saves can never run + * concurrently and land out of order. + * - The most recently queued payload is always committed. In particular calling + * `flush()` while an older save is still in-flight waits for that save AND then + * commits any newer payload queued in the meantime. + * + * The second guarantee is the important one: previously `flush()` would await an + * in-flight save and return immediately, silently dropping any newer payload that + * had been queued (and whose debounce timer it had just cleared). Under network + * lag this lost the user's latest changes - e.g. a freshly typed recipient would + * never be persisted and the editor would fall back to the "Recipient 1" + * placeholder. + */ export function useEnvelopeAutosave(saveFn: (data: T) => Promise, delay = 1000) { const timeoutRef = useRef | null>(null); - const lastArgsRef = useRef(null); - const pendingPromiseRef = useRef | null>(null); + + // The most recently queued, not-yet-saved payload. Wrapped in an object so that + // a queued falsy payload is still distinguishable from "nothing queued". + const pendingRef = useRef<{ value: T } | null>(null); + + // The in-flight commit pump. Concurrent callers await this same promise rather + // than starting a second, overlapping save. + const commitPromiseRef = useRef | null>(null); + + // Always invoke the latest `saveFn` (which may close over fresh state) without + // having to rebuild `triggerSave`/`flush` on every render. + const saveFnRef = useRef(saveFn); + saveFnRef.current = saveFn; const [isPending, setIsPending] = useState(false); const [isCommiting, setIsCommiting] = useState(false); + /** + * Drains the queued payload, running `saveFn` one save at a time until nothing + * is left to save. If a newer payload is queued while a save is in-flight it is + * picked up on the next loop iteration, so the latest changes are never lost. + */ + const commit = useCallback((): Promise => { + // A pump is already running → share it instead of starting a second one. + if (commitPromiseRef.current) { + return commitPromiseRef.current; + } + + // Nothing queued and nothing in-flight → no work to do. + if (!pendingRef.current) { + return Promise.resolve(); + } + + const pump = (async () => { + try { + setIsCommiting(true); + + while (pendingRef.current) { + const { value } = pendingRef.current; + pendingRef.current = null; + + await saveFnRef.current(value); + } + } finally { + // eslint-disable-next-line require-atomic-updates + commitPromiseRef.current = null; + setIsCommiting(false); + setIsPending(false); + } + })(); + + commitPromiseRef.current = pump; + + return pump; + }, []); + const triggerSave = useCallback( (data: T) => { - lastArgsRef.current = data; + pendingRef.current = { value: data }; - // A debounce or promise means something is pending + // A debounce timer or an in-flight save means something is pending. setIsPending(true); if (timeoutRef.current) { clearTimeout(timeoutRef.current); } - // eslint-disable-next-line @typescript-eslint/no-misused-promises - timeoutRef.current = setTimeout(async () => { - if (!lastArgsRef.current) { - return; - } - - const args = lastArgsRef.current; - lastArgsRef.current = null; + timeoutRef.current = setTimeout(() => { timeoutRef.current = null; - - setIsCommiting(true); - pendingPromiseRef.current = saveFn(args); - - try { - await pendingPromiseRef.current; - } finally { - // eslint-disable-next-line require-atomic-updates - pendingPromiseRef.current = null; - setIsCommiting(false); - setIsPending(false); - } + void commit(); }, delay); }, - [saveFn, delay], + [commit, delay], ); const flush = useCallback(async () => { @@ -51,34 +100,12 @@ export function useEnvelopeAutosave(saveFn: (data: T) => Promise, delay timeoutRef.current = null; } - if (pendingPromiseRef.current) { - // Already running → wait for it - await pendingPromiseRef.current; - return; - } - - if (lastArgsRef.current) { - const args = lastArgsRef.current; - lastArgsRef.current = null; - - setIsCommiting(true); - setIsPending(true); - - pendingPromiseRef.current = saveFn(args); - try { - await pendingPromiseRef.current; - } finally { - // eslint-disable-next-line require-atomic-updates - pendingPromiseRef.current = null; - setIsCommiting(false); - setIsPending(false); - } - } - }, [saveFn]); + await commit(); + }, [commit]); useEffect(() => { const handleBeforeUnload = () => { - if (timeoutRef.current || pendingPromiseRef.current) { + if (timeoutRef.current || pendingRef.current || commitPromiseRef.current) { void flush(); } };