diff --git a/apps/yaak-client/lib/editSessionTracker.test.ts b/apps/yaak-client/lib/editSessionTracker.test.ts index dc54c5a28..e0ceb6bb7 100644 --- a/apps/yaak-client/lib/editSessionTracker.test.ts +++ b/apps/yaak-client/lib/editSessionTracker.test.ts @@ -39,6 +39,53 @@ describe("EditSessionTracker", () => { expect(captured).toEqual([["rq_1", "idle"]]); }); + test("each request keeps its own countdown when two are edited in turn", () => { + const { tracker: t, captured } = tracker(); + t.noteEdit("rq_a"); + vi.advanceTimersByTime(600); + // An edit to B, from this window, another one, or sync, must not touch A's countdown + t.noteEdit("rq_b"); + vi.advanceTimersByTime(400); + expect(captured).toEqual([["rq_a", "idle"]]); + + // B keeps writing; A is done and stays done + t.noteEdit("rq_b"); + vi.advanceTimersByTime(999); + expect(captured).toEqual([["rq_a", "idle"]]); + vi.advanceTimersByTime(1); + expect(captured).toEqual([ + ["rq_a", "idle"], + ["rq_b", "idle"], + ]); + }); + + test("a boundary capture settles that request's countdown, and only that one", () => { + const { tracker: t, captured } = tracker(); + t.noteActiveRequest("rq_a"); + t.noteEdit("rq_a"); + t.noteEdit("rq_b"); + t.noteActiveRequest("rq_c"); + expect(captured).toEqual([["rq_a", "switch"]]); + + vi.advanceTimersByTime(1000); + // A was already captured when it was left; B's countdown still ran + expect(captured).toEqual([ + ["rq_a", "switch"], + ["rq_b", "idle"], + ]); + }); + + test("a close can bring its own snapshot", () => { + const { tracker: t, captured } = tracker(); + const onClose: Capture[] = []; + t.noteActiveRequest("rq_1"); + t.noteEdit("rq_1"); + t.noteBoundary((id, reason) => onClose.push([id, reason])); + vi.advanceTimersByTime(1000); + expect(onClose).toEqual([["rq_1", "switch"]]); + expect(captured).toEqual([]); + }); + test("captures the request being left, not the one being opened", () => { const { tracker: t, captured } = tracker(); t.noteActiveRequest("rq_1"); diff --git a/apps/yaak-client/lib/editSessionTracker.ts b/apps/yaak-client/lib/editSessionTracker.ts index 17e43440a..951db0ad9 100644 --- a/apps/yaak-client/lib/editSessionTracker.ts +++ b/apps/yaak-client/lib/editSessionTracker.ts @@ -18,12 +18,16 @@ type Snapshot = (requestId: string, reason: ModelVersionReason) => void; * * It deliberately knows nothing about *what* changed. Versions are * content-addressed, so a boundary that turns out to have nothing behind it - * costs one query and creates nothing — which is what lets this stay a timer - * and two assignments instead of a change-tracking system. + * costs one query and creates nothing — which is what lets this stay a few + * timers instead of a change-tracking system. */ export class EditSessionTracker { - private timer: ReturnType | null = null; - private idleRequestId: string | null = null; + /** + * One idle countdown per request. Writes arrive for any request — from other + * windows and sync too — and an edit to one must never restart or cancel + * another's countdown. + */ + private timers = new Map>(); private activeRequestId: string | null = null; constructor( @@ -31,15 +35,16 @@ export class EditSessionTracker { private readonly idleMs: number = IDLE_MS, ) {} - /** A request was written. Restarts its idle countdown. */ + /** A request was written. Restarts that request's idle countdown. */ noteEdit(requestId: string) { - if (this.timer != null) clearTimeout(this.timer); - this.idleRequestId = requestId; - this.timer = setTimeout(() => { - this.timer = null; - const requestId = this.idleRequestId; - if (requestId != null) this.snapshot(requestId, "idle"); - }, this.idleMs); + this.clearTimer(requestId); + this.timers.set( + requestId, + setTimeout(() => { + this.timers.delete(requestId); + this.snapshot(requestId, "idle"); + }, this.idleMs), + ); } /** The user moved to a different request, so the one they left is finished. */ @@ -47,11 +52,27 @@ export class EditSessionTracker { if (requestId === this.activeRequestId) return; const left = this.activeRequestId; this.activeRequestId = requestId; - if (left != null) this.snapshot(left, "switch"); + if (left != null) this.capture(left, this.snapshot); } - /** The window lost focus or is closing. */ - noteBoundary() { - if (this.activeRequestId != null) this.snapshot(this.activeRequestId, "switch"); + /** + * The window lost focus or is closing. A close can't wait for anything, so it + * passes its own snapshot function. + */ + noteBoundary(snapshot: Snapshot = this.snapshot) { + if (this.activeRequestId != null) this.capture(this.activeRequestId, snapshot); + } + + /** Capture at a boundary, which also settles any idle countdown still pending */ + private capture(requestId: string, snapshot: Snapshot) { + this.clearTimer(requestId); + snapshot(requestId, "switch"); + } + + private clearTimer(requestId: string) { + const timer = this.timers.get(requestId); + if (timer == null) return; + clearTimeout(timer); + this.timers.delete(requestId); } } diff --git a/apps/yaak-client/lib/requestVersions.test.ts b/apps/yaak-client/lib/requestVersions.test.ts new file mode 100644 index 000000000..e47a04272 --- /dev/null +++ b/apps/yaak-client/lib/requestVersions.test.ts @@ -0,0 +1,60 @@ +import { beforeEach, describe, expect, test, vi } from "vite-plus/test"; + +const mocks = vi.hoisted(() => ({ + calls: [] as string[], + finishWrites: null as (() => void) | null, + flushAllModelWrites: vi.fn(), + rpc: vi.fn(), +})); + +vi.mock("@yaakapp-internal/models", () => ({ + flushAllModelWrites: mocks.flushAllModelWrites, + flushAllPendingPatches: vi.fn(), +})); +vi.mock("@yaakapp-internal/platform", () => ({ platform: {} })); +vi.mock("../hooks/useActiveRequestId", () => ({ activeRequestIdAtom: null })); +vi.mock("./jotai", () => ({ jotaiStore: {} })); +vi.mock("./rpc", () => ({ rpc: mocks.rpc })); + +describe("snapshotRequestVersion", () => { + beforeEach(() => { + mocks.calls = []; + mocks.flushAllModelWrites.mockImplementation( + () => + new Promise((resolve) => { + mocks.calls.push("flush"); + mocks.finishWrites = () => { + mocks.calls.push("writes landed"); + resolve(); + }; + }), + ); + mocks.rpc.mockImplementation(async (cmd: string) => { + mocks.calls.push(cmd); + return { id: "mv_1" }; + }); + }); + + test("asks for the snapshot only once pending writes have landed", async () => { + const { snapshotRequestVersion } = await import("./requestVersions"); + snapshotRequestVersion("rq_1", "switch"); + await Promise.resolve(); + expect(mocks.calls).toEqual(["flush"]); + + mocks.finishWrites?.(); + await vi.waitFor(() => + expect(mocks.calls).toEqual(["flush", "writes landed", "models_snapshot_request"]), + ); + expect(mocks.rpc).toHaveBeenCalledWith("models_snapshot_request", { + requestId: "rq_1", + reason: "switch", + }); + }); + + test("a failed write still lets the snapshot go ahead", async () => { + mocks.flushAllModelWrites.mockRejectedValue(new Error("write failed")); + const { snapshotRequestVersion } = await import("./requestVersions"); + snapshotRequestVersion("rq_1", "idle"); + await vi.waitFor(() => expect(mocks.rpc).toHaveBeenCalledTimes(1)); + }); +}); diff --git a/apps/yaak-client/lib/requestVersions.ts b/apps/yaak-client/lib/requestVersions.ts index 690529f6b..cf93fb811 100644 --- a/apps/yaak-client/lib/requestVersions.ts +++ b/apps/yaak-client/lib/requestVersions.ts @@ -1,4 +1,4 @@ -import { flushAllPendingPatches } from "@yaakapp-internal/models"; +import { flushAllModelWrites, flushAllPendingPatches } from "@yaakapp-internal/models"; import type { ModelPayload, ModelVersion, ModelVersionReason } from "@yaakapp-internal/models"; import { platform } from "@yaakapp-internal/platform"; import { EditSessionTracker } from "./editSessionTracker"; @@ -16,8 +16,31 @@ const REQUEST_MODELS = ["http_request", "grpc_request", "websocket_request"]; * anything. */ export function snapshotRequestVersion(requestId: string, reason: ModelVersionReason) { - // Edits reach the database on a debounce, so flush before asking for a - // version of what is in it + captureRequestVersion(requestId, reason).catch((err: unknown) => { + console.warn("Failed to snapshot request version", err); + }); +} + +async function captureRequestVersion(requestId: string, reason: ModelVersionReason) { + // Edits reach the database on a debounce. Wait until every pending write has + // landed, or the snapshot can read the row before the edit this boundary is + // meant to capture. A write that failed leaves nothing newer to wait for. + await flushAllModelWrites().catch((err: unknown) => { + console.warn("A pending write failed before a request snapshot", err); + }); + await rpc("models_snapshot_request", { requestId, reason }); +} + +/** + * The same, for a window that is closing, where nothing can be awaited. + * + * The pending writes go out first and the snapshot right behind them. In the + * browser that is enough: the worker runs commands in the order they arrive. + * On the desktop each command runs on its own, so the snapshot can still reach + * the database before the last write does. The edit itself is saved either + * way, and the request's next send versions it. + */ +function snapshotRequestVersionOnClose(requestId: string, reason: ModelVersionReason) { flushAllPendingPatches(); rpc("models_snapshot_request", { requestId, reason }).catch((err: unknown) => { console.warn("Failed to snapshot request version", err); @@ -46,5 +69,7 @@ export function initRequestVersionSnapshots() { // Closing is the last boundary there is. Nothing can be awaited here, but the // write is already on its way and the backend outlives the window. - window.addEventListener("beforeunload", () => tracker.noteBoundary()); + window.addEventListener("beforeunload", () => + tracker.noteBoundary(snapshotRequestVersionOnClose), + ); }