fix: improve editor autosave

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<void>;
+  /** 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<RecipientSetLagHandle> => {
+  let markFirstInFlight: () => void = () => {};
+
+  const firstRecipientSetInFlight = new Promise<void>((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);
+  });
+});

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<T>(saveFn: (data: T) => Promise<void>, delay = 1000) {
   const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
-  const lastArgsRef = useRef<T | null>(null);
-  const pendingPromiseRef = useRef<Promise<void> | 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<Promise<void> | 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<void> => {
+    // 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<T>(saveFn: (data: T) => Promise<void>, 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();
       }
     };