From a4bdc54b2ca8c46590348637f51b53a8e34e63a3 Mon Sep 17 00:00:00 2001 From: Amruth Pillai Date: Sat, 5 Sep 2026 18:31:10 -0700 Subject: [PATCH] fix(builder): save pending drafts before navigation (#3453) * fix(builder): save pending drafts before navigation * fix(builder): bound navigation waits for slow saves --- apps/web/locales/en-US.po | 4 + .../src/features/resume/builder/draft.test.ts | 134 ++++++++++++++++++ apps/web/src/features/resume/builder/draft.ts | 53 ++++++- .../e2e/specs/builder-save-navigation.spec.ts | 123 ++++++++++++++++ 4 files changed, 313 insertions(+), 1 deletion(-) create mode 100644 tests/e2e/specs/builder-save-navigation.spec.ts diff --git a/apps/web/locales/en-US.po b/apps/web/locales/en-US.po index 79199c2a1..a934b31f6 100644 --- a/apps/web/locales/en-US.po +++ b/apps/web/locales/en-US.po @@ -4794,6 +4794,10 @@ msgstr "Save the file with every page upright." msgid "Saved" msgstr "Saved" +#: src/features/resume/builder/draft.ts +msgid "Saving is taking longer than expected. Your changes are still open." +msgstr "Saving is taking longer than expected. Your changes are still open." + #: src/routes/builder/$resumeId/-components/header.tsx msgid "Saving…" msgstr "Saving…" diff --git a/apps/web/src/features/resume/builder/draft.test.ts b/apps/web/src/features/resume/builder/draft.test.ts index f91812b35..9db0f65e3 100644 --- a/apps/web/src/features/resume/builder/draft.test.ts +++ b/apps/web/src/features/resume/builder/draft.test.ts @@ -9,6 +9,7 @@ import { defaultResumeData } from "@reactive-resume/schema/resume/default"; import { isEditableElementFocused, useBuilderResumeUpdateSubscription, + useResumeCleanup, useResumeStore, useResumeUpdateSubscription, } from "./draft"; @@ -20,6 +21,8 @@ const orpcMocks = vi.hoisted(() => ({ updateResume: vi.fn(), })); +const useBlockerMock = vi.hoisted(() => vi.fn()); + const consumeEventIteratorMock = vi.hoisted(() => vi.fn()); const queryClientMock = vi.hoisted(() => ({ @@ -45,6 +48,7 @@ vi.mock("@tanstack/react-query", () => ({ vi.mock("@tanstack/react-router", () => ({ useParams: () => routerParamsMock.value, + useBlocker: useBlockerMock, })); vi.mock("@/libs/orpc/client", () => ({ @@ -115,9 +119,139 @@ async function flushMicrotasks() { } describe("builder resume autosave", () => { + it("waits for the latest draft to save before allowing navigation", async () => { + const initial = makeResume("navigation-debounce"); + useResumeStore.getState().initialize(initial); + routerParamsMock.value = { resumeId: initial.id }; + const hook = renderHook(() => useResumeCleanup()); + let complete!: (resume: Resume) => void; + orpcMocks.updateResume.mockImplementationOnce( + () => + new Promise((resolve) => { + complete = resolve; + }), + ); + useResumeStore.getState().updateResumeData((draft) => { + draft.basics.name = "Navigate safely"; + }); + const blocker = useBlockerMock.mock.lastCall?.[0]; + expect(blocker).toBeDefined(); + let settled = false; + const result = blocker.shouldBlockFn({ next: { params: {} } }).then((blocked: boolean) => { + settled = true; + return blocked; + }); + await flushMicrotasks(); + expect(settled).toBe(false); + expect(orpcMocks.updateResume.mock.lastCall?.[1].signal.aborted).toBe(false); + complete(withBasicsName(initial, "Navigate safely")); + expect(await result).toBe(false); + expect(useResumeStore.getState().saveStatus).toBe("saved"); + hook.unmount(); + }); + + it("waits for a queued edit after an in-flight save before navigating", async () => { + const initial = makeResume("navigation-queued"); + useResumeStore.getState().initialize(initial); + routerParamsMock.value = { resumeId: initial.id }; + const hook = renderHook(() => useResumeCleanup()); + const completions: Array<(resume: Resume) => void> = []; + orpcMocks.updateResume.mockImplementation( + () => + new Promise((resolve) => { + completions.push(resolve); + }), + ); + useResumeStore.getState().updateResumeData((draft) => { + draft.basics.name = "First"; + }); + await vi.advanceTimersByTimeAsync(500); + useResumeStore.getState().updateResumeData((draft) => { + draft.basics.name = "Latest"; + }); + const blocker = useBlockerMock.mock.lastCall?.[0]; + let settled = false; + const result = blocker.shouldBlockFn({ next: { params: {} } }).then((blocked: boolean) => { + settled = true; + return blocked; + }); + completions[0](withBasicsName(initial, "First")); + await flushMicrotasks(); + expect(settled).toBe(false); + expect(orpcMocks.updateResume.mock.lastCall?.[0].data.basics.name).toBe("Latest"); + completions[1](withBasicsName(initial, "Latest")); + expect(await result).toBe(false); + expect(useResumeStore.getState().resume?.data.basics.name).toBe("Latest"); + hook.unmount(); + }); + + it("ends a stalled navigation wait without aborting or discarding the pending save", async () => { + const initial = makeResume("navigation-timeout"); + useResumeStore.getState().initialize(initial); + routerParamsMock.value = { resumeId: initial.id }; + const hook = renderHook(() => useResumeCleanup()); + let complete!: (resume: Resume) => void; + orpcMocks.updateResume.mockImplementationOnce( + () => + new Promise((resolve) => { + complete = resolve; + }), + ); + useResumeStore.getState().updateResumeData((draft) => { + draft.basics.name = "Pending name"; + }); + const blocker = useBlockerMock.mock.lastCall?.[0]; + let settled = false; + const result = blocker.shouldBlockFn({ next: { params: {} } }).then((blocked: boolean) => { + settled = true; + return blocked; + }); + await vi.advanceTimersByTimeAsync(10000); + expect(settled).toBe(true); + expect(await result).toBe(true); + expect(useResumeStore.getState().saveStatus).toBe("saving"); + expect(useResumeStore.getState().resume?.data.basics.name).toBe("Pending name"); + expect(orpcMocks.updateResume.mock.lastCall?.[1].signal.aborted).toBe(false); + + orpcMocks.updateResume.mockResolvedValueOnce(withBasicsName(initial, "Latest name")); + useResumeStore.getState().updateResumeData((draft) => { + draft.basics.name = "Latest name"; + }); + await vi.advanceTimersByTimeAsync(500); + expect(orpcMocks.updateResume).toHaveBeenCalledTimes(1); + complete(withBasicsName(initial, "Pending name")); + await flushMicrotasks(); + expect(useResumeStore.getState().saveStatus).toBe("saved"); + expect(useResumeStore.getState().resume?.data.basics.name).toBe("Latest name"); + expect(await blocker.shouldBlockFn({ next: { params: {} } })).toBe(false); + expect(orpcMocks.updateResume).toHaveBeenCalledTimes(2); + hook.unmount(); + }); + + it("keeps a failed draft in the builder and retries on the next navigation", async () => { + const initial = makeResume("navigation-error"); + useResumeStore.getState().initialize(initial); + routerParamsMock.value = { resumeId: initial.id }; + const hook = renderHook(() => useResumeCleanup()); + orpcMocks.updateResume.mockRejectedValueOnce(new Error("Offline")); + useResumeStore.getState().updateResumeData((draft) => { + draft.basics.name = "Keep this draft"; + }); + const blocker = useBlockerMock.mock.lastCall?.[0]; + expect(blocker).toBeDefined(); + expect(await blocker.shouldBlockFn({ next: { params: {} } })).toBe(true); + expect(useResumeStore.getState().resume?.data.basics.name).toBe("Keep this draft"); + expect(blocker.enableBeforeUnload()).toBe(true); + orpcMocks.updateResume.mockResolvedValueOnce(withBasicsName(initial, "Keep this draft")); + expect(await blocker.shouldBlockFn({ next: { params: {} } })).toBe(false); + expect(blocker.enableBeforeUnload()).toBe(false); + hook.unmount(); + }); + beforeEach(() => { vi.useFakeTimers(); orpcMocks.getResumeById.mockReset(); + useBlockerMock.mockReset(); orpcMocks.patchResume.mockReset(); orpcMocks.streamSubscribe.mockReset(); orpcMocks.updateResume.mockReset(); diff --git a/apps/web/src/features/resume/builder/draft.ts b/apps/web/src/features/resume/builder/draft.ts index f395cc2aa..a390b59f8 100644 --- a/apps/web/src/features/resume/builder/draft.ts +++ b/apps/web/src/features/resume/builder/draft.ts @@ -4,7 +4,7 @@ import type { WritableDraft } from "immer"; import { t } from "@lingui/core/macro"; import { consumeEventIterator } from "@orpc/client"; import { useQueryClient } from "@tanstack/react-query"; -import { useParams } from "@tanstack/react-router"; +import { useBlocker, useParams } from "@tanstack/react-router"; import { debounce, isEqual } from "es-toolkit"; import { useCallback, useEffect, useState } from "react"; import { immer } from "zustand/middleware/immer"; @@ -65,6 +65,7 @@ type Runtime = { isSaving: boolean; pendingResume?: Resume; syncErrorToastId?: string; + slowSaveToastId?: string; syncResume: ReturnType void>>; beforeUnloadHandler?: () => void; deferredRemoteResume?: Resume; @@ -78,6 +79,7 @@ type ResumeUpdateSubscriptionOptions = { }; const SAVE_DEBOUNCE_MS = 500; +const NAVIGATION_SAVE_WAIT_MS = 10_000; // Rapid edits within this window coalesce into a single undo step (e.g. typing a word / dragging). const HISTORY_COALESCE_MS = 500; // Bounded stacks: keep undo/redo memory (whole-resume snapshots) predictable during a long session. @@ -231,6 +233,10 @@ async function flushResumeSave(id: string) { timeout: 0, }); } finally { + if (runtime.slowSaveToastId !== undefined) { + toast.close(runtime.slowSaveToastId); + runtime.slowSaveToastId = undefined; + } runtime.isSaving = false; if (runtime.pendingResume && runtime.syncErrorToastId === undefined) void flushResumeSave(id); } @@ -664,11 +670,56 @@ export function useBuilderResumeUpdateSubscription() { useResumeUpdateSubscription({ resumeId, onUpdate, onError }); } +// Route transitions can await a save; unmount cleanup and browser unload cannot. +function saveResumeBeforeLeaving(id: string): boolean | Promise { + const runtime = runtimes.get(id); + const current = useResumeStore.getState().resume; + if (!runtime?.hasPendingLocalChanges || current?.id !== id) return true; + + runtime.syncResume.cancel(); + runtime.pendingResume = cloneResume(current); + useResumeStore.getState().setSaveStatus("saving"); + + return new Promise((resolve) => { + const finish = (saved: boolean) => { + clearTimeout(timeout); + unsubscribe(); + resolve(saved); + }; + const unsubscribe = useResumeStore.subscribe((state) => { + if (state.resume?.id !== id || state.saveStatus === "error") { + finish(false); + } else if (state.saveStatus === "saved" && !runtime.hasPendingLocalChanges) { + finish(true); + } + }); + const timeout = setTimeout(() => { + finish(false); + // Keep the write in flight: it may already have reached the server. + runtime.slowSaveToastId = toast.add({ + type: "info", + description: t`Saving is taking longer than expected. Your changes are still open.`, + id: runtime.slowSaveToastId, + timeout: 0, + }); + }, NAVIGATION_SAVE_WAIT_MS); + void flushResumeSave(id); + }); +} + export function useResumeCleanup() { const params = useParams({ strict: false }) as { resumeId?: string }; const resumeId = params.resumeId; const reset = useResumeStore((state) => state.reset); + useBlocker({ + shouldBlockFn: async ({ next }) => { + if (!resumeId || ("resumeId" in next.params && next.params.resumeId === resumeId)) return false; + return !(await saveResumeBeforeLeaving(resumeId)); + }, + enableBeforeUnload: () => !!resumeId && (runtimes.get(resumeId)?.hasPendingLocalChanges ?? false), + }); + useEffect(() => { if (!resumeId) return; diff --git a/tests/e2e/specs/builder-save-navigation.spec.ts b/tests/e2e/specs/builder-save-navigation.spec.ts new file mode 100644 index 000000000..0e4de1824 --- /dev/null +++ b/tests/e2e/specs/builder-save-navigation.spec.ts @@ -0,0 +1,123 @@ +import type { Page, TestInfo } from "@playwright/test"; +import { createSampleResumeFromDashboard } from "../fixtures/resume"; +import { expect, test } from "../fixtures/test"; + +const updateUrl = "**/api/rpc/resume/update"; +function barrier() { + let resolve!: () => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} +function waitSave(page: Page) { + return page.waitForResponse( + (response) => new URL(response.url()).pathname === "/api/rpc/resume/update" && response.ok(), + ); +} + +async function prepareNavigationTest(page: Page, testInfo: TestInfo) { + await createSampleResumeFromDashboard(page, testInfo); + await page.reload(); + const warmup = waitSave(page); + await page.getByLabel("Headline", { exact: true }).fill("Navigation fixture ready"); + await warmup; + return page.url(); +} + +test("retries a failed autosave before leaving the builder", async ({ authPage: page }, testInfo) => { + const url = await prepareNavigationTest(page, testInfo); + + await page.route(updateUrl, async (route) => { + await route.abort("failed"); + }); + await page.getByLabel("Name", { exact: true }).fill("Draft recovered before leaving"); + await expect(page.getByText("Your latest changes could not be saved.", { exact: true })).toBeVisible(); + await page.unroute(updateUrl); + const arrived = barrier(); + const release = barrier(); + await page.route(updateUrl, async (route) => { + arrived.resolve(); + await release.promise; + await route.continue(); + }); + await page.getByRole("button", { name: "Go to resumes dashboard", exact: true }).click(); + await arrived.promise; + expect(page.url()).toBe(url); + await expect(page.getByLabel("Name", { exact: true })).toHaveValue("Draft recovered before leaving"); + release.resolve(); + await page.waitForURL(/\/dashboard/); + await page.goto(url); + await expect(page.getByLabel("Name", { exact: true })).toHaveValue("Draft recovered before leaving"); +}); + +test("retains the current draft when saving during navigation fails", async ({ authPage: page }, testInfo) => { + const url = await prepareNavigationTest(page, testInfo); + let attempts = 0; + await page.route(updateUrl, async (route) => { + attempts++; + await route.abort("failed"); + }); + await page.getByLabel("Name", { exact: true }).fill("Keep unsaved draft"); + await expect(page.getByText("Your latest changes could not be saved.", { exact: true })).toBeVisible(); + await page.getByRole("button", { name: "Go to resumes dashboard", exact: true }).click(); + await expect.poll(() => attempts).toBe(2); + await expect(page.getByRole("status").filter({ hasText: "Couldn't save" })).toBeVisible(); + expect(page.url()).toBe(url); + await expect(page.getByLabel("Name", { exact: true })).toHaveValue("Keep unsaved draft"); + await page.unroute(updateUrl); + await page.getByRole("button", { name: "Go to resumes dashboard", exact: true }).click(); + await page.waitForURL(/\/dashboard/); + await page.goto(url); + await expect(page.getByLabel("Name", { exact: true })).toHaveValue("Keep unsaved draft"); +}); + +test("stops waiting for a slow save while preserving late acknowledgements and queued edits", async ({ + authPage: page, +}, testInfo) => { + const url = await prepareNavigationTest(page, testInfo); + await page.clock.install(); + const arrived = barrier(); + const release = barrier(); + let attempts = 0; + await page.route(updateUrl, async (route) => { + attempts++; + if (attempts === 1) { + arrived.resolve(); + await release.promise; + } + await route.continue(); + }); + await page.getByLabel("Name", { exact: true }).fill("Slow save draft"); + await page.clock.fastForward(600); + await arrived.promise; + await page.getByRole("button", { name: "Go to resumes dashboard", exact: true }).click(); + await page.clock.fastForward(10000); + const slowNotice = page.getByText("Saving is taking longer than expected. Your changes are still open.", { + exact: true, + }); + await expect(slowNotice).toBeVisible(); + await expect(page.getByRole("status").filter({ hasText: "Saving" })).toBeVisible(); + expect(page.url()).toBe(url); + expect(attempts).toBe(1); + + await page.getByLabel("Headline", { exact: true }).fill("Latest edit during slow save"); + await page.clock.fastForward(600); + expect(attempts).toBe(1); + const latestSaved = page.waitForResponse( + (response) => + new URL(response.url()).pathname === "/api/rpc/resume/update" && + response.ok() && + (response.request().postData() ?? "").includes("Latest edit during slow save"), + ); + release.resolve(); + await latestSaved; + await expect(page.getByRole("status").filter({ hasText: "Saved" })).toBeVisible(); + await expect(slowNotice).toBeHidden(); + await page.getByRole("button", { name: "Go to resumes dashboard", exact: true }).click(); + await page.waitForURL(/\/dashboard/); + await page.goto(url); + await expect(page.getByLabel("Name", { exact: true })).toHaveValue("Slow save draft"); + await expect(page.getByLabel("Headline", { exact: true })).toHaveValue("Latest edit during slow save"); + expect(attempts).toBe(2); +});