From 96c96d0b7d97844cabf085553673d01e3edf59de Mon Sep 17 00:00:00 2001 From: Henry Date: Thu, 10 Sep 2026 21:49:17 +0800 Subject: [PATCH] fix(web): stop a task status from reverting when an older update finishes late (#3528) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../app/office/tasks/use-board-drag.test.ts | 182 +++++++++- apps/web/app/office/tasks/use-board-drag.ts | 75 +++- .../simple/components/status-picker.test.tsx | 83 ++++- .../task/simple/components/status-picker.tsx | 18 +- .../e2e/tests/office/property-pickers.spec.ts | 100 +++++- ...c-task-mutation-rollback-ordering.test.tsx | 333 +++++++++++++++++- .../web/hooks/use-optimistic-task-mutation.ts | 148 +++++++- .../state/office-task-content-sync.test.ts | 50 +++ .../web/lib/state/office-task-content-sync.ts | 164 +++++++-- 9 files changed, 1069 insertions(+), 84 deletions(-) diff --git a/apps/web/app/office/tasks/use-board-drag.test.ts b/apps/web/app/office/tasks/use-board-drag.test.ts index 16bee7fd57..29e19b2d5a 100644 --- a/apps/web/app/office/tasks/use-board-drag.test.ts +++ b/apps/web/app/office/tasks/use-board-drag.test.ts @@ -1,7 +1,14 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { applyStatusDrop, type StatusDropDeps } from "./use-board-drag"; import { ApprovalGateError } from "@/lib/api/domains/office-status-gate"; import type { OfficeTask, OfficeTaskStatus } from "@/lib/state/slices/office/types"; +import { __resetOfficeTaskContentSyncForTests } from "@/lib/state/office-task-content-sync"; + +const GATE_MESSAGE = "Cannot mark done: awaiting approval from Ada, Grace"; + +afterEach(() => { + __resetOfficeTaskContentSyncForTests(); +}); function task(id: string, status: OfficeTaskStatus): OfficeTask { return { @@ -28,6 +35,35 @@ function deps( } satisfies StatusDropDeps; } +/** + * Unlike `deps`, `getTask` here reflects every prior `patchTask` call, the + * way the real board (backed by the Zustand store) does. The interleaving + * tests below need a second drop to see the first drop's still-optimistic + * patch, not a snapshot frozen at test setup. + */ +function liveDeps(initial: OfficeTask) { + let current = initial; + const d = { + getTask: vi.fn(() => current), + patchTask: vi.fn((_id: string, patch: Partial) => { + current = { ...current, ...patch }; + }), + updateStatus: vi.fn(), + onError: vi.fn(), + } satisfies StatusDropDeps; + return { d, getCurrent: () => current }; +} + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + describe("applyStatusDrop", () => { it("patches the store then sends the status when a card crosses columns", async () => { const d = deps(task("t1", "todo")); @@ -72,10 +108,9 @@ describe("applyStatusDrop", () => { expect(d.onError).not.toHaveBeenCalled(); }); - it("rolls the card back to its whole prior snapshot when the mutation fails", async () => { + it("rolls the card back to its prior status when the mutation fails", async () => { const before = task("t1", "in_review"); - // rawStatus is set by the store on ingestion and must survive a rollback, - // which is why the snapshot goes back whole rather than status-only. + // rawStatus is set by the store on ingestion and must survive a rollback. before.rawStatus = "REVIEW"; const d = deps(before, async () => { throw new Error("boom"); @@ -84,14 +119,17 @@ describe("applyStatusDrop", () => { await applyStatusDrop("t1", "done", d); expect(d.patchTask).toHaveBeenNthCalledWith(1, "t1", { status: "done" }); - expect(d.patchTask).toHaveBeenNthCalledWith(2, "t1", before); + expect(d.patchTask).toHaveBeenNthCalledWith(2, "t1", { + status: before.status, + rawStatus: before.rawStatus, + }); expect(d.onError).toHaveBeenCalledWith("boom"); }); it("surfaces the approver-gate sentence rather than a bare failure", async () => { // A plain Error (anything that isn't the typed ApprovalGateError below) // still rolls back to the snapshot and surfaces its message verbatim. - const gate = "Cannot mark done: awaiting approval from Ada, Grace"; + const gate = GATE_MESSAGE; const d = deps(task("t1", "in_review"), async () => { throw new Error(gate); }); @@ -108,7 +146,7 @@ describe("applyStatusDrop", () => { // for exactly this case). Rolling back to the pre-drop snapshot here // would show a status the server no longer holds. const before = task("t1", "todo"); - const gate = "Cannot mark done: awaiting approval from Ada, Grace"; + const gate = GATE_MESSAGE; const d = deps(before, async () => { throw new ApprovalGateError(gate, "in_review"); }); @@ -132,7 +170,135 @@ describe("applyStatusDrop", () => { await applyStatusDrop("t1", "blocked", d); - expect(d.patchTask).toHaveBeenNthCalledWith(2, "t1", before); + expect(d.patchTask).toHaveBeenNthCalledWith(2, "t1", { + status: before.status, + rawStatus: before.rawStatus, + }); expect(d.onError).toHaveBeenCalledTimes(1); }); }); + +describe("applyStatusDrop generation guard — overlapping status mutations", () => { + it("older-fails-after-newer-succeeds: the older drop's rollback does not clobber the newer, server-confirmed status", async () => { + const { d, getCurrent } = liveDeps(task("t1", "todo")); + const older = deferred(); + const newer = deferred(); + d.updateStatus.mockImplementationOnce(() => older.promise); + d.updateStatus.mockImplementationOnce(() => newer.promise); + + // Two fast consecutive drags on the same card: the first (todo -> + // in_progress) is still in flight when the second (in_progress -> + // blocked) is dropped. + const p1 = applyStatusDrop("t1", "in_progress", d); + const p2 = applyStatusDrop("t1", "blocked", d); + + newer.resolve(); + await p2; + expect(getCurrent().status).toBe("blocked"); + + older.reject(new Error("boom")); + await p1; + + // The stale failure must not roll the board back past the newer, + // already-succeeded status, but it still surfaces its own toast. + expect(getCurrent().status).toBe("blocked"); + expect(d.onError).toHaveBeenCalledTimes(1); + }); + + it("gate-redirect superseded by a newer success does not clobber the newer status", async () => { + const { d, getCurrent } = liveDeps(task("t1", "todo")); + const older = deferred(); + const newer = deferred(); + d.updateStatus.mockImplementationOnce(() => older.promise); + d.updateStatus.mockImplementationOnce(() => newer.promise); + + const p1 = applyStatusDrop("t1", "done", d); + const p2 = applyStatusDrop("t1", "blocked", d); + + newer.resolve(); + await p2; + expect(getCurrent().status).toBe("blocked"); + + // The older drop's gate redirect resolves after the newer drop already + // succeeded: redirecting to "in_review" here would clobber the newer, + // server-confirmed "blocked" status the same way an unconditional + // rollback would. + const gate = GATE_MESSAGE; + older.reject(new ApprovalGateError(gate, "in_review")); + await p1; + + expect(getCurrent().status).toBe("blocked"); + expect(d.onError).toHaveBeenCalledWith(gate); + }); + + it("a newer gate redirect is not clobbered by an older, later-resolving plain failure", async () => { + const { d, getCurrent } = liveDeps(task("t1", "todo")); + const older = deferred(); + const newer = deferred(); + d.updateStatus.mockImplementationOnce(() => older.promise); + d.updateStatus.mockImplementationOnce(() => newer.promise); + + // The older drop (todo -> in_progress) is still in flight when the newer + // one (-> done) is dropped and hits the approver gate first, redirecting + // to "in_review" server-side. + const p1 = applyStatusDrop("t1", "in_progress", d); + const p2 = applyStatusDrop("t1", "done", d); + + const gate = GATE_MESSAGE; + newer.reject(new ApprovalGateError(gate, "in_review")); + await p2; + expect(getCurrent().status).toBe("in_review"); + + // The older drop then fails with a plain error. It must not be able to + // roll the board back past the newer, server-confirmed gate redirect. + older.reject(new Error("boom")); + await p1; + + expect(getCurrent().status).toBe("in_review"); + expect(getCurrent().rawStatus).toBe("in_review"); + expect(d.onError).toHaveBeenCalledTimes(2); + }); + + it("restores the baseline when an older drop fails before a newer drop", async () => { + const { d, getCurrent } = liveDeps(task("t1", "todo")); + const older = deferred(); + const newer = deferred(); + d.updateStatus.mockImplementationOnce(() => older.promise); + d.updateStatus.mockImplementationOnce(() => newer.promise); + + const p1 = applyStatusDrop("t1", "in_progress", d); + const p2 = applyStatusDrop("t1", "blocked", d); + + older.reject(new Error("older failed")); + await p1; + expect(getCurrent().status).toBe("blocked"); + + newer.reject(new Error("newer failed")); + await p2; + + expect(getCurrent().status).toBe("todo"); + expect(d.onError).toHaveBeenCalledTimes(2); + }); + + it("keeps an older approval-gate redirect when a newer drop fails", async () => { + const { d, getCurrent } = liveDeps(task("t1", "todo")); + const older = deferred(); + const newer = deferred(); + d.updateStatus.mockImplementationOnce(() => older.promise); + d.updateStatus.mockImplementationOnce(() => newer.promise); + + const p1 = applyStatusDrop("t1", "done", d); + const p2 = applyStatusDrop("t1", "blocked", d); + + older.reject(new ApprovalGateError(GATE_MESSAGE, "in_review")); + await p1; + expect(getCurrent().status).toBe("blocked"); + + newer.reject(new Error("newer failed")); + await p2; + + expect(getCurrent().status).toBe("in_review"); + expect(getCurrent().rawStatus).toBe("in_review"); + expect(d.onError).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/web/app/office/tasks/use-board-drag.ts b/apps/web/app/office/tasks/use-board-drag.ts index 7c9f63f57d..1dd35bb010 100644 --- a/apps/web/app/office/tasks/use-board-drag.ts +++ b/apps/web/app/office/tasks/use-board-drag.ts @@ -11,6 +11,16 @@ import { import { toast } from "@/lib/toast/sonner"; import { t } from "@/lib/i18n"; import type { OfficeTask, OfficeTaskStatus } from "@/lib/state/slices/office/types"; +import { + beginWrite, + endWrite, + nextTaskSequence, + recordWriteFailed, + recordWriteSettled, + shouldRestoreAfterFailedWrite, + TASK_SCOPE, + type TaskScopeValues, +} from "@/lib/state/office-task-content-sync"; /** * Collaborators for `applyStatusDrop`, injected so the drop rules can be @@ -48,19 +58,59 @@ export async function applyStatusDrop( // pixels, not a move. Sending it would burn a PATCH and a WS round-trip. if (snapshot.status === targetStatus) return; + const sequence = nextTaskSequence(taskId); + const taskScopeBefore: TaskScopeValues = { + status: snapshot.status, + rawStatus: snapshot.rawStatus, + }; + const taskScopePatch: TaskScopeValues = { + status: targetStatus, + rawStatus: targetStatus, + }; + beginWrite(taskId, TASK_SCOPE, sequence, taskScopeBefore, taskScopePatch); + deps.patchTask(taskId, { status: targetStatus }); try { await deps.updateStatus(taskId, targetStatus); + recordWriteSettled(taskId, TASK_SCOPE, sequence, taskScopePatch); + const reconciliation = endWrite(taskId, TASK_SCOPE, sequence); + if (reconciliation) { + deps.patchTask(taskId, toOfficeTaskReconciliationPatch(reconciliation)); + } } catch (err) { if (err instanceof ApprovalGateError) { - // The backend already redirected and persisted this status server-side - // before returning the error (see ApprovalGateError), so the board is - // wrong if it rolls back to the pre-drop snapshot here. Patch status - // only: spreading the snapshot would reinstate its stale rawStatus and - // the card would re-normalize back to the old column. - deps.patchTask(taskId, { status: err.redirectedStatus }); + // The backend has already persisted the redirected status at this + // write's sequence regardless of whether the UI ends up showing it, so + // a later-failing, lower-sequence move must see this as settled rather + // than treating it as unresolved and clobbering it. + recordWriteSettled(taskId, TASK_SCOPE, sequence, { + status: err.redirectedStatus, + rawStatus: err.redirectedStatus, + }); } else { - deps.patchTask(taskId, snapshot); + recordWriteFailed(taskId, TASK_SCOPE, sequence); + } + // Only settle onto this failure's outcome if no later-sequenced move on + // this task has already succeeded or is still in flight — otherwise this + // stale failure would clobber newer, server-confirmed state. + const shouldRestore = shouldRestoreAfterFailedWrite(taskId, TASK_SCOPE, sequence); + const reconciliation = endWrite(taskId, TASK_SCOPE, sequence); + if (reconciliation) { + deps.patchTask(taskId, toOfficeTaskReconciliationPatch(reconciliation)); + } else if (shouldRestore) { + if (err instanceof ApprovalGateError) { + // The backend already redirected and persisted this status server-side + // before returning the error (see ApprovalGateError), so the board is + // wrong if it rolls back to the pre-drop snapshot here. Patch status + // only: spreading the snapshot would reinstate its stale rawStatus and + // the card would re-normalize back to the old column. + deps.patchTask(taskId, { status: err.redirectedStatus }); + } else { + deps.patchTask(taskId, { + status: snapshot.status, + rawStatus: snapshot.rawStatus, + }); + } } // The approver gate arrives here already translated into a sentence // naming who still has to sign off. @@ -68,6 +118,17 @@ export async function applyStatusDrop( } } +function toOfficeTaskReconciliationPatch(values: TaskScopeValues): Partial { + const patch: Partial = {}; + if (Object.prototype.hasOwnProperty.call(values, "status")) { + patch.status = values.status as OfficeTaskStatus; + } + if (Object.prototype.hasOwnProperty.call(values, "rawStatus")) { + patch.rawStatus = values.rawStatus as string | undefined; + } + return patch; +} + /** * Wires the office task board to dnd-kit. Droppable ids are status values and * draggable ids are task ids, so a drag end reads as (taskId -> status). diff --git a/apps/web/components/task/simple/components/status-picker.test.tsx b/apps/web/components/task/simple/components/status-picker.test.tsx index 6bd53999a0..65e0eb6ec5 100644 --- a/apps/web/components/task/simple/components/status-picker.test.tsx +++ b/apps/web/components/task/simple/components/status-picker.test.tsx @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; -import type { ReactNode } from "react"; +import { useState, type ReactNode } from "react"; import { StateProvider } from "@/components/state-provider"; import { TaskOptimisticContextProvider } from "@/hooks/use-optimistic-task-mutation"; import { ApiError } from "@/lib/api/client"; @@ -73,6 +73,46 @@ function Wrapper({ children }: { children: ReactNode }) { ); } +/** + * Unlike `Wrapper`, `ctx.task` here is backed by real React state, so the + * picker's displayed status reflects each `handleSelect` call's actual + * effect instead of a fixed prop. Needed to interleave two real picks + * through the component itself, the way a user clicking twice would. + */ +function LiveWrapper({ initialTask }: { initialTask: Task }) { + const [liveTask, setLiveTask] = useState(initialTask); + const ctx = { + task: liveTask, + applyPatch: (patch: Partial) => setLiveTask((t) => ({ ...t, ...patch })), + restore: (snapshot: Task) => setLiveTask(snapshot), + }; + return ( + + + + + + ); +} + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +async function selectStatus(testId: string) { + fireEvent.click(screen.getByTestId(TRIGGER_TEST_ID)); + const option = await screen.findByTestId(testId); + await act(async () => { + fireEvent.click(option); + }); +} + describe("StatusPicker", () => { it("renders the current status label on the trigger", () => { render( @@ -138,7 +178,9 @@ describe("StatusPicker", () => { "Cannot mark done: awaiting approval from CEO, Eng Lead", ); }); +}); +describe("StatusPicker approval gate redirect", () => { it("settles on the redirected status instead of the pre-drag snapshot", async () => { updateTaskMock.mockRejectedValueOnce( new ApiError(APPROVALS_PENDING_MESSAGE, 409, { @@ -158,11 +200,44 @@ describe("StatusPicker", () => { fireEvent.click(option); }); - // The hook's own rollback restores "todo" first; the picker's catch - // must then re-apply the server's redirected status on top of it, so - // the final patch is the redirect, not the rollback. + // The hook settles on the server's redirected status directly, in place + // of its usual rollback, so the final patch is the redirect. expect(applyPatchMock).toHaveBeenLastCalledWith({ status: "in_review" }); }); + + it("a delayed gate redirect does not clobber a newer, already-succeeded pick", async () => { + // Two real picks through the component itself: A ("done") hits the + // approver gate and stays pending; B ("blocked") is picked immediately + // after and succeeds first. A's gate redirect must not then overwrite + // B's newer, server-confirmed status. + const older = deferred<{ ok: true }>(); + const newer = deferred<{ ok: true }>(); + updateTaskMock.mockReturnValueOnce(older.promise); + updateTaskMock.mockReturnValueOnce(newer.promise); + + render(); + + await selectStatus("status-picker-option-done"); + await selectStatus("status-picker-option-blocked"); + + await act(async () => { + newer.resolve({ ok: true }); + await Promise.resolve(); + }); + expect(screen.getByTestId(TRIGGER_TEST_ID).textContent).toContain("Blocked"); + + await act(async () => { + older.reject( + new ApiError(APPROVALS_PENDING_MESSAGE, 409, { + error: APPROVALS_PENDING_MESSAGE, + pending_approvers: [{ agent_profile_id: "a1", name: "CEO" }], + status: "in_review", + }), + ); + await Promise.resolve(); + }); + expect(screen.getByTestId(TRIGGER_TEST_ID).textContent).toContain("Blocked"); + }); }); describe("formatPendingApproversMessage", () => { diff --git a/apps/web/components/task/simple/components/status-picker.tsx b/apps/web/components/task/simple/components/status-picker.tsx index aa7f22dc78..a1995b8a94 100644 --- a/apps/web/components/task/simple/components/status-picker.tsx +++ b/apps/web/components/task/simple/components/status-picker.tsx @@ -5,10 +5,7 @@ import { IconChevronDown } from "@tabler/icons-react"; import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; import { cn } from "@/lib/utils"; import { useOptimisticTaskMutation } from "@/hooks/use-optimistic-task-mutation"; -import { - ApprovalGateError, - updateTaskStatusOrTranslateGate, -} from "@/lib/api/domains/office-status-gate"; +import { updateTaskStatusOrTranslateGate } from "@/lib/api/domains/office-status-gate"; import type { Task, TaskStatus } from "@/app/office/tasks/[id]/types"; import { StatusIcon } from "@/app/office/tasks/[id]/status-icon"; import { normalizeTaskStatus } from "@/lib/api/domains/office-task-normalize"; @@ -57,18 +54,7 @@ export function StatusPicker({ task }: StatusPickerProps) { await mutate(task.id, { status: value }, () => updateTaskStatusOrTranslateGate(task.id, value), ); - } catch (err) { - // The hook already rolled back to the pre-mutation snapshot. The - // backend redirected and persisted this status server-side before - // returning the error (see ApprovalGateError), so a plain rollback - // shows a status the server no longer holds until the WS event - // reconciles it. Re-apply the redirected status now via a no-op - // mutation, matching use-board-drag.ts's board-drag path. - if (err instanceof ApprovalGateError) { - await mutate(task.id, { status: err.redirectedStatus as TaskStatus }, () => - Promise.resolve(), - ); - } + } catch { /* toast already raised by hook */ } }; diff --git a/apps/web/e2e/tests/office/property-pickers.spec.ts b/apps/web/e2e/tests/office/property-pickers.spec.ts index 5a1803c18f..7151015749 100644 --- a/apps/web/e2e/tests/office/property-pickers.spec.ts +++ b/apps/web/e2e/tests/office/property-pickers.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from "../../fixtures/office-fixture"; import type { Page } from "@playwright/test"; -import { waitForHttp } from "../../helpers/causal-waits"; +import { injectLatency, waitForHttp } from "../../helpers/causal-waits"; /** * E2E coverage for office task property pickers (status, priority, @@ -375,6 +375,104 @@ test.describe("property pickers", () => { }).toPass({ timeout: 5_000 }); }); + test("a stale failed mutation does not clobber a newer successful one", async ({ + testPage, + apiClient, + officeApi, + officeSeed, + }) => { + // Regression test: useOptimisticTaskMutation used to snapshot-and-restore + // unconditionally on failure, with no sequencing. If an older mutation for + // the same task failed *after* a newer one had already succeeded, the + // older failure's rollback clobbered the newer, server-confirmed state. + // Concrete repro: drag todo -> in_progress (request A in flight), then + // immediately in_progress -> blocked (request B). B succeeds; A then + // fails. Pre-fix the UI settled back on the pre-A status instead of + // "blocked". The fix threads a per-task sequence guard through + // office-task-content-sync.ts so a failure only rolls back when no + // later-sequenced write has already succeeded or is still in flight. + const task = await apiClient.createTask(officeSeed.workspaceId, "Picker Race Rollback Task", { + workflow_id: officeSeed.workflowId, + }); + await gotoTaskPage(testPage, task.id, "Picker Race Rollback Task"); + + const patchPathname = `/api/v1/office/tasks/${task.id}`; + let patchCallCount = 0; + await testPage.route( + (url) => url.pathname === patchPathname, + async (route) => { + if (route.request().method() !== "PATCH") { + await route.continue(); + return; + } + patchCallCount += 1; + if (patchCallCount === 1) { + // Hold request A open well past request B's real round trip, so B + // is guaranteed to settle first. Fail it with no real backend + // effect: the backend's status after this test is driven solely by + // request B. + await injectLatency( + 1200, + "force the older mutation's response to arrive after the newer one", + ); + await route.fulfill({ + status: 500, + contentType: "application/json", + body: JSON.stringify({ error: "forced older-mutation failure" }), + }); + return; + } + // Request B: let it hit the real backend and succeed. + await route.continue(); + }, + ); + + const settleOrder: string[] = []; + const trigger = testPage.getByTestId("status-picker-trigger"); + const olderFailure = waitForHttp(testPage, "PATCH", new RegExp(`^${patchPathname}$`), { + predicate: (response) => response.status() === 500, + }).then((response) => { + settleOrder.push("older"); + return response; + }); + await trigger.click(); + await testPage.getByTestId("status-picker-option-in_progress").click(); + await expect(trigger).toContainText(/In Progress/i, { timeout: 5_000 }); + + const newerSuccess = waitForHttp(testPage, "PATCH", new RegExp(`^${patchPathname}$`), { + predicate: (response) => response.ok(), + }).then((response) => { + settleOrder.push("newer"); + return response; + }); + await trigger.click(); + await testPage.getByTestId("status-picker-option-blocked").click(); + await expect(trigger).toContainText(/Blocked/i, { timeout: 5_000 }); + + // Wait for both requests to actually land, in order, before asserting + // the settled state — otherwise the assertion below could pass for the + // wrong reason (the stale failure hasn't been processed yet). + await newerSuccess; + await olderFailure; + expect(settleOrder).toEqual(["newer", "older"]); + + // Read the backend's settled value before asserting the UI. This HTTP + // round trip gives the older failure's own catch handler — a same-tick + // continuation of the response `waitForHttp` already observed above — + // time to run, so the UI assertion below checks the truly settled label + // instead of racing a still-optimistic "Blocked" that a buggy rollback + // has not yet overwritten. + const persisted = (await officeApi.getTask(task.id)) as Record; + const inner = (persisted.task as Record) ?? persisted; + const status = (inner.status as string) ?? (inner.state as string) ?? ""; + expect(status.toLowerCase()).toContain("blocked"); + + // The stale failure's catch handler must not roll the UI back to "todo" + // (the pre-mutation snapshot) or "In Progress" (request A's optimistic + // patch) — the newer, server-confirmed "Blocked" must stand. + await expect(trigger).toContainText(/Blocked/i, { timeout: 5_000 }); + }); + test("started and completed rows show timestamps after a todo -> in_progress -> done transition", async ({ testPage, apiClient, diff --git a/apps/web/hooks/use-optimistic-task-mutation-rollback-ordering.test.tsx b/apps/web/hooks/use-optimistic-task-mutation-rollback-ordering.test.tsx index d47bf606ac..305bce4a5b 100644 --- a/apps/web/hooks/use-optimistic-task-mutation-rollback-ordering.test.tsx +++ b/apps/web/hooks/use-optimistic-task-mutation-rollback-ordering.test.tsx @@ -1,8 +1,12 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, render } from "@testing-library/react"; -import type { ReactNode } from "react"; +import { useMemo, useState, type ReactNode } from "react"; import { StateProvider, useAppStoreApi } from "@/components/state-provider"; -import { TaskOptimisticContextProvider, useCommitTaskTitle } from "./use-optimistic-task-mutation"; +import { + TaskOptimisticContextProvider, + useCommitTaskTitle, + useOptimisticTaskMutation, +} from "./use-optimistic-task-mutation"; import type { Task } from "@/app/office/tasks/[id]/types"; import type { OfficeTask } from "@/lib/state/slices/office/types"; import type { Task as HttpTask } from "@/lib/types/http"; @@ -10,6 +14,7 @@ import { __resetOfficeTaskContentSyncForTests, seedInitialCanonical, } from "@/lib/state/office-task-content-sync"; +import { ApprovalGateError } from "@/lib/api/domains/office-status-gate"; vi.mock("sonner", () => ({ toast: { @@ -67,6 +72,16 @@ const baseOfficeTask: OfficeTask = { updatedAt: TS, }; +function makeStoreSeed(initialOffice: OfficeTask | null) { + return function StoreSeed({ children }: { children: ReactNode }) { + const api = useAppStoreApi(); + if (initialOffice) { + api.getState().setTasks([initialOffice]); + } + return <>{children}; + }; +} + function makeHarness(initialTask: Task, initialOffice: OfficeTask | null) { const state = { task: initialTask, @@ -82,13 +97,7 @@ function makeHarness(initialTask: Task, initialOffice: OfficeTask | null) { state.task = snapshot; }, }; - function StoreSeed({ children }: { children: ReactNode }) { - const api = useAppStoreApi(); - if (initialOffice) { - api.getState().setTasks([initialOffice]); - } - return <>{children}; - } + const StoreSeed = makeStoreSeed(initialOffice); function Wrapper({ children }: { children: ReactNode }) { return ( @@ -101,6 +110,59 @@ function makeHarness(initialTask: Task, initialOffice: OfficeTask | null) { return { Wrapper, state }; } +/** + * Unlike `makeHarness`, `ctx.task` here is backed by real React state so it + * reflects each mutate call's actual pre-patch board state (including a + * still-unresolved earlier call's optimistic patch) instead of a snapshot + * frozen at harness creation. The generic mutation hook's rollback restores + * to `ctx.task` captured at issue time, so an interleaving test needs that + * value to behave the way a real page component's props would. + */ +function makeLiveHarness(initialTask: Task, initialOffice: OfficeTask | null) { + const taskRef: { current: Task } = { current: initialTask }; + const mutateRef: { current: ReturnType | null } = { + current: null, + }; + const storeApiRef: { current: ReturnType | null } = { current: null }; + + function Inner() { + const [task, setTask] = useState(initialTask); + taskRef.current = task; + const ctxValue = useMemo( + () => ({ + task, + applyPatch: (patch: Partial) => setTask((t) => ({ ...t, ...patch })), + restore: (snapshot: Task) => setTask(snapshot), + }), + [task], + ); + return ( + + { + mutateRef.current = m; + storeApiRef.current = s; + }} + /> + + ); + } + + const StoreSeed = makeStoreSeed(initialOffice); + + function Wrapper() { + return ( + + + + + + ); + } + + return { Wrapper, taskRef, mutateRef, storeApiRef }; +} + function deferred() { let resolve!: (value: T) => void; let reject!: (reason?: unknown) => void; @@ -125,6 +187,20 @@ function TitleHookProbe({ return null; } +function GenericMutationProbe({ + onReady, +}: { + onReady: ( + mutate: ReturnType, + storeApi: ReturnType, + ) => void; +}) { + const mutate = useOptimisticTaskMutation(); + const storeApi = useAppStoreApi(); + onReady(mutate, storeApi); + return null; +} + describe("useCommitTaskTitle rollback ordering — AC-65 chained failures", () => { it("chained double failure: restores only once both commits have resolved, regardless of resolution order (AC-65)", async () => { seedInitialCanonical("t-1", "title", ORIGINAL_TITLE, TS); @@ -226,3 +302,242 @@ describe("useCommitTaskTitle rollback ordering — AC-65 chained failures", () = expect(toast.error).toHaveBeenCalledTimes(2); }); }); + +describe("useOptimisticTaskMutation generation guard — overlapping status mutations", () => { + it("older-fails-after-newer-succeeds: the older failure's rollback does not clobber the newer, server-confirmed state, but it still toasts and rejects", async () => { + const { Wrapper, taskRef, storeApiRef, mutateRef } = makeLiveHarness(baseTask, baseOfficeTask); + render(); + + const older = deferred(); + const newer = deferred(); + let p1!: Promise; + let p2!: Promise; + + // The older mutation issues first (todo -> in_progress); the newer one + // issues while it is still pending (in_progress -> blocked), matching two + // fast consecutive board drags on the same card. + act(() => { + p1 = mutateRef.current!("t-1", { status: "in_progress" }, () => older.promise); + }); + act(() => { + p2 = mutateRef.current!("t-1", { status: "blocked" }, () => newer.promise); + }); + const p1Settled = p1.catch(() => undefined); + + // The newer mutation resolves first and succeeds. + await act(async () => { + newer.resolve(); + await p2; + }); + expect(taskRef.current.status).toBe("blocked"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("blocked"); + + // The older mutation then fails. Its rollback must be suppressed: the + // newer, already-succeeded status must survive. + await act(async () => { + older.reject(new Error(WRITE_FAILURE_MESSAGE)); + await p1Settled; + }); + expect(taskRef.current.status).toBe("blocked"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("blocked"); + await expect(p1).rejects.toThrow(WRITE_FAILURE_MESSAGE); + expect(toast.error).toHaveBeenCalledTimes(1); + }); + + it("chained double failure: the newer mutation restores its own pre-patch snapshot first, then the older's own resolution converges the board back to the true baseline", async () => { + const { Wrapper, taskRef, storeApiRef, mutateRef } = makeLiveHarness(baseTask, baseOfficeTask); + render(); + + const older = deferred(); + const newer = deferred(); + let p1!: Promise; + let p2!: Promise; + + act(() => { + p1 = mutateRef.current!("t-1", { status: "in_progress" }, () => older.promise); + }); + act(() => { + p2 = mutateRef.current!("t-1", { status: "blocked" }, () => newer.promise); + }); + const p1Settled = p1.catch(() => undefined); + const p2Settled = p2.catch(() => undefined); + + // The newer mutation fails first while the older is still pending: it is + // the last word standing, so it restores — but only to its own captured + // snapshot (the board state right before its own patch, i.e. still + // showing the older mutation's still-unresolved optimistic status). + await act(async () => { + newer.reject(new Error(WRITE_FAILURE_MESSAGE)); + await p2Settled; + }); + expect(taskRef.current.status).toBe("in_progress"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("in_progress"); + + // The older mutation then also fails. Nothing is pending ahead of it any + // longer, so its own restore now fires too, converging the board on the + // true pre-mutation baseline. + await act(async () => { + older.reject(new Error(WRITE_FAILURE_MESSAGE)); + await p1Settled; + }); + expect(taskRef.current.status).toBe("todo"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("todo"); + expect(toast.error).toHaveBeenCalledTimes(2); + }); +}); + +describe("useOptimisticTaskMutation retained rollback outcomes", () => { + it("chained double failure: an older failure does not leave its optimistic value after the newer failure", async () => { + const { Wrapper, taskRef, storeApiRef, mutateRef } = makeLiveHarness(baseTask, baseOfficeTask); + render(); + + const older = deferred(); + const newer = deferred(); + let p1!: Promise; + let p2!: Promise; + + act(() => { + p1 = mutateRef.current!("t-1", { status: "in_progress" }, () => older.promise); + }); + act(() => { + p2 = mutateRef.current!("t-1", { status: "blocked" }, () => newer.promise); + }); + const p1Settled = p1.catch(() => undefined); + const p2Settled = p2.catch(() => undefined); + + await act(async () => { + older.reject(new Error(WRITE_FAILURE_MESSAGE)); + await p1Settled; + }); + expect(taskRef.current.status).toBe("blocked"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("blocked"); + + await act(async () => { + newer.reject(new Error(WRITE_FAILURE_MESSAGE)); + await p2Settled; + }); + expect(taskRef.current.status).toBe("todo"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("todo"); + await expect(p1).rejects.toThrow(WRITE_FAILURE_MESSAGE); + await expect(p2).rejects.toThrow(WRITE_FAILURE_MESSAGE); + }); + + it("keeps an older approval-gate redirect when a newer mutation fails", async () => { + const { Wrapper, taskRef, storeApiRef, mutateRef } = makeLiveHarness(baseTask, baseOfficeTask); + render(); + + const older = deferred(); + const newer = deferred(); + let p1!: Promise; + let p2!: Promise; + + act(() => { + p1 = mutateRef.current!("t-1", { status: "done" }, () => older.promise); + }); + act(() => { + p2 = mutateRef.current!("t-1", { status: "blocked" }, () => newer.promise); + }); + const p1Settled = p1.catch(() => undefined); + const p2Settled = p2.catch(() => undefined); + + const gate = "Cannot mark done: awaiting approval from Ada, Grace"; + await act(async () => { + older.reject(new ApprovalGateError(gate, "in_review")); + await p1Settled; + }); + expect(taskRef.current.status).toBe("blocked"); + + await act(async () => { + newer.reject(new Error(WRITE_FAILURE_MESSAGE)); + await p2Settled; + }); + expect(taskRef.current.status).toBe("in_review"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("in_review"); + await expect(p1).rejects.toThrow(gate); + await expect(p2).rejects.toThrow(WRITE_FAILURE_MESSAGE); + }); +}); + +describe("useOptimisticTaskMutation generation guard — approval gate redirect", () => { + it("gate-redirect superseded by a newer success does not clobber the newer status", async () => { + const { Wrapper, taskRef, storeApiRef, mutateRef } = makeLiveHarness(baseTask, baseOfficeTask); + render(); + + const older = deferred(); + const newer = deferred(); + let p1!: Promise; + let p2!: Promise; + + // The older mutation (todo -> done) hits the approver gate; the newer one + // (in_progress -> blocked) issues while it is still pending and succeeds + // first, matching a status pick immediately followed by a board drag. + act(() => { + p1 = mutateRef.current!("t-1", { status: "done" }, () => older.promise); + }); + act(() => { + p2 = mutateRef.current!("t-1", { status: "blocked" }, () => newer.promise); + }); + const p1Settled = p1.catch(() => undefined); + + await act(async () => { + newer.resolve(); + await p2; + }); + expect(taskRef.current.status).toBe("blocked"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("blocked"); + + // The older mutation's gate redirect resolves after the newer mutation + // already succeeded: settling on "in_review" here would clobber the + // newer, server-confirmed "blocked" status the same way an unconditional + // rollback would. + const gate = "Cannot mark done: awaiting approval from Ada, Grace"; + await act(async () => { + older.reject(new ApprovalGateError(gate, "in_review")); + await p1Settled; + }); + expect(taskRef.current.status).toBe("blocked"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("blocked"); + await expect(p1).rejects.toThrow(gate); + expect(toast.error).toHaveBeenCalledTimes(1); + }); + + it("a newer gate redirect is not clobbered by an older, later-resolving plain failure", async () => { + const { Wrapper, taskRef, storeApiRef, mutateRef } = makeLiveHarness(baseTask, baseOfficeTask); + render(); + + const older = deferred(); + const newer = deferred(); + let p1!: Promise; + let p2!: Promise; + + // The older mutation (todo -> in_progress) is still in flight when the + // newer one (-> done) issues and hits the approver gate first, redirecting + // to "in_review" server-side. + act(() => { + p1 = mutateRef.current!("t-1", { status: "in_progress" }, () => older.promise); + }); + act(() => { + p2 = mutateRef.current!("t-1", { status: "done" }, () => newer.promise); + }); + const p1Settled = p1.catch(() => undefined); + const p2Settled = p2.catch(() => undefined); + + const gate = "Cannot mark done: awaiting approval from Ada, Grace"; + await act(async () => { + newer.reject(new ApprovalGateError(gate, "in_review")); + await p2Settled; + }); + expect(taskRef.current.status).toBe("in_review"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("in_review"); + + // The older mutation then fails with a plain error. It must not be able + // to roll the board back past the newer, server-confirmed gate redirect. + await act(async () => { + older.reject(new Error(WRITE_FAILURE_MESSAGE)); + await p1Settled; + }); + expect(taskRef.current.status).toBe("in_review"); + expect(storeApiRef.current!.getState().office.tasks.items[0]?.status).toBe("in_review"); + expect(toast.error).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/web/hooks/use-optimistic-task-mutation.ts b/apps/web/hooks/use-optimistic-task-mutation.ts index bd61db71c7..437ca625c0 100644 --- a/apps/web/hooks/use-optimistic-task-mutation.ts +++ b/apps/web/hooks/use-optimistic-task-mutation.ts @@ -3,17 +3,22 @@ import { createContext, useCallback, useContext } from "react"; import { toast } from "@/lib/toast/sonner"; import { useAppStoreApi } from "@/components/state-provider"; -import type { Task } from "@/app/office/tasks/[id]/types"; +import type { Task, TaskStatus } from "@/app/office/tasks/[id]/types"; import type { OfficeTask } from "@/lib/state/slices/office/types"; import { t } from "@/lib/i18n"; import { updateTask } from "@/lib/api/domains/kanban-api"; +import { ApprovalGateError } from "@/lib/api/domains/office-status-gate"; import { beginWrite, endWrite, getCanonicalValue, nextTaskSequence, + recordWriteFailed, + recordWriteSettled, recordWriteSuccess, shouldRestoreAfterFailedWrite, + TASK_SCOPE, + type TaskScopeValues, } from "@/lib/state/office-task-content-sync"; /** @@ -63,6 +68,11 @@ export function useOptimisticTaskMutation() { const snapshot = ctx.task; const storePatch = toOfficeTaskPatch(patch); const storeSnapshot = storeApi.getState().office.tasks.items.find((t) => t.id === taskId); + const taskScopePatch = toTaskScopeValues(patch); + const taskScopeBefore = taskScopeBeforeValues(snapshot, storeSnapshot, patch); + + const sequence = nextTaskSequence(taskId); + beginWrite(taskId, TASK_SCOPE, sequence, taskScopeBefore, taskScopePatch); // Apply optimistic patches (local + store). ctx.applyPatch(patch); @@ -72,21 +82,21 @@ export function useOptimisticTaskMutation() { try { await apiCall(); - } catch (err) { - // Rollback both layers. This hook never patches title/description - // (see `toOfficeTaskPatch` above), so the rollback must not touch - // them either — those two fields are governed exclusively by the - // per-field guard in office-task-content-sync.ts and have their own - // dedicated writers (useCommitTaskTitle/useCommitTaskDescription). - // Restoring the full pre-mutation snapshot here would silently - // revert a confirmed title/description edit whenever an unrelated - // picker mutation fails (AC-61: exactly two writers may touch a - // guarded field). - ctx.restore(snapshot); - if (storeSnapshot) { - const { title: _title, description: _description, ...storeRollback } = storeSnapshot; - storeApi.getState().patchTaskInStore(taskId, storeRollback); + recordWriteSettled(taskId, TASK_SCOPE, sequence, taskScopePatch); + const reconciliation = endWrite(taskId, TASK_SCOPE, sequence); + if (reconciliation) { + applyTaskScopeReconciliation(taskId, reconciliation, ctx.applyPatch, storeApi); } + } catch (err) { + handleTaskMutationFailure({ + taskId, + sequence, + err, + snapshot, + storeSnapshot, + ctx, + storeApi, + }); toastUpdateFailure(err); throw err; } @@ -119,6 +129,114 @@ function toOfficeTaskPatch(patch: Partial): Partial { return out; } +function toTaskScopeValues(patch: Partial): TaskScopeValues { + const values: TaskScopeValues = {}; + for (const key of Object.keys(patch)) { + values[key] = patch[key as keyof Task]; + } + if (Object.prototype.hasOwnProperty.call(patch, "status")) { + values.rawStatus = patch.status; + } + return values; +} + +function taskScopeBeforeValues( + snapshot: Task, + storeSnapshot: OfficeTask | undefined, + patch: Partial, +): TaskScopeValues { + const values: TaskScopeValues = {}; + for (const key of Object.keys(patch)) { + values[key] = snapshot[key as keyof Task]; + } + if (Object.prototype.hasOwnProperty.call(patch, "status")) { + values.rawStatus = storeSnapshot?.rawStatus ?? snapshot.rawStatus; + } + return values; +} + +function toOfficeTaskRollbackPatch(values: TaskScopeValues): Partial { + const patch = toOfficeTaskPatch(values as Partial); + if (Object.prototype.hasOwnProperty.call(values, "status")) { + patch.status = values.status as OfficeTask["status"]; + } + if (Object.prototype.hasOwnProperty.call(values, "rawStatus")) { + patch.rawStatus = values.rawStatus as string | undefined; + } + return patch; +} + +function applyTaskScopeReconciliation( + taskId: string, + values: TaskScopeValues, + applyLocalPatch: (patch: Partial) => void, + storeApi: ReturnType, +): void { + applyLocalPatch(values as Partial); + storeApi.getState().patchTaskInStore(taskId, toOfficeTaskRollbackPatch(values)); +} + +type TaskMutationFailure = { + taskId: string; + sequence: number; + err: unknown; + snapshot: Task; + storeSnapshot: OfficeTask | undefined; + ctx: TaskOptimisticContextValue; + storeApi: ReturnType; +}; + +function handleTaskMutationFailure({ + taskId, + sequence, + err, + snapshot, + storeSnapshot, + ctx, + storeApi, +}: TaskMutationFailure): void { + if (err instanceof ApprovalGateError) { + // The backend has already persisted the redirected status at this + // write's sequence, so retain it as a settled outcome for later writes. + recordWriteSettled(taskId, TASK_SCOPE, sequence, { + status: err.redirectedStatus, + rawStatus: err.redirectedStatus, + }); + } else { + recordWriteFailed(taskId, TASK_SCOPE, sequence); + } + + // Only restore if no later-sequenced mutation on this task has already + // succeeded or is still in flight. + const shouldRestore = shouldRestoreAfterFailedWrite(taskId, TASK_SCOPE, sequence); + const reconciliation = endWrite(taskId, TASK_SCOPE, sequence); + if (reconciliation) { + applyTaskScopeReconciliation(taskId, reconciliation, ctx.applyPatch, storeApi); + return; + } + if (!shouldRestore) return; + + if (err instanceof ApprovalGateError) { + // The backend redirected and persisted this status before returning the + // error. Settle on the status only so stale rawStatus cannot re-normalize + // the card back to its pre-mutation column. + const redirectPatch: Partial = { status: err.redirectedStatus as TaskStatus }; + ctx.applyPatch(redirectPatch); + if (storeSnapshot) { + storeApi.getState().patchTaskInStore(taskId, toOfficeTaskPatch(redirectPatch)); + } + return; + } + + // This hook never patches title/description, so the rollback must not touch + // them either. Those fields have their own dedicated writers. + ctx.restore(snapshot); + if (storeSnapshot) { + const { title: _title, description: _description, ...storeRollback } = storeSnapshot; + storeApi.getState().patchTaskInStore(taskId, storeRollback); + } +} + function toastUpdateFailure(err: unknown): void { const message = err instanceof Error ? err.message : t("task:updateFailed"); toast.error(message); diff --git a/apps/web/lib/state/office-task-content-sync.test.ts b/apps/web/lib/state/office-task-content-sync.test.ts index 2b8201593d..e4d12ebff7 100644 --- a/apps/web/lib/state/office-task-content-sync.test.ts +++ b/apps/web/lib/state/office-task-content-sync.test.ts @@ -11,10 +11,12 @@ import { openFieldEditor, parseUpdatedAt, recordRefetchCandidate, + recordWriteSettled, recordWriteSuccess, seedInitialCanonical, shouldRestoreAfterFailedWrite, subscribeField, + TASK_SCOPE, } from "./office-task-content-sync"; const TASK = "task-1"; @@ -424,6 +426,54 @@ describe("shouldRestoreAfterFailedWrite (AC-9, AC-44, AC-53, R5-F6)", () => { }); }); +describe('"task" scope — generic whole-task mutation guard', () => { + it("suppresses restore for a stale failure once a later-sequenced task-scope write has succeeded", () => { + const failingSeq = nextTaskSequence(TASK); + beginWrite(TASK, TASK_SCOPE, failingSeq); + const laterSeq = nextTaskSequence(TASK); + beginWrite(TASK, TASK_SCOPE, laterSeq); + recordWriteSettled(TASK, TASK_SCOPE, laterSeq); + endWrite(TASK, TASK_SCOPE, laterSeq); + + expect(shouldRestoreAfterFailedWrite(TASK, TASK_SCOPE, failingSeq)).toBe(false); + }); + + it("suppresses restore for a stale failure while a later-sequenced task-scope write is still pending", () => { + const failingSeq = nextTaskSequence(TASK); + beginWrite(TASK, TASK_SCOPE, failingSeq); + const laterSeq = nextTaskSequence(TASK); + beginWrite(TASK, TASK_SCOPE, laterSeq); + + expect(shouldRestoreAfterFailedWrite(TASK, TASK_SCOPE, failingSeq)).toBe(false); + }); + + it("restores when it is the only or the last-standing task-scope write", () => { + const seq = nextTaskSequence(TASK); + beginWrite(TASK, TASK_SCOPE, seq); + expect(shouldRestoreAfterFailedWrite(TASK, TASK_SCOPE, seq)).toBe(true); + }); + + it("keeps the 'task' scope and a field scope independent: a task-scope write does not guard or suppress a title write", () => { + seedInitialCanonical(TASK, "title", "original", T10); + + const taskSeq = nextTaskSequence(TASK); + beginWrite(TASK, TASK_SCOPE, taskSeq); + + // The title field is untouched by the concurrent task-scope write: it is + // not guarded, its canonical value is unaffected, and a title write's own + // restore decision ignores the task-scope write entirely. + expect(isFieldGuarded(TASK, "title")).toBe(false); + expect(getCanonicalValue(TASK, "title")).toBe("original"); + + const titleSeq = nextTaskSequence(TASK); + beginWrite(TASK, "title", titleSeq); + expect(shouldRestoreAfterFailedWrite(TASK, "title", titleSeq)).toBe(true); + + endWrite(TASK, TASK_SCOPE, taskSeq); + endWrite(TASK, "title", titleSeq); + }); +}); + describe("recordWriteSuccess ordering relative to guard release (AC-58)", () => { it("records the canonical value while the write's guard contribution is still held, before the caller releases it", () => { seedInitialCanonical(TASK, "title", "v1", T10); diff --git a/apps/web/lib/state/office-task-content-sync.ts b/apps/web/lib/state/office-task-content-sync.ts index df39c875c6..9a4e832162 100644 --- a/apps/web/lib/state/office-task-content-sync.ts +++ b/apps/web/lib/state/office-task-content-sync.ts @@ -10,6 +10,17 @@ */ export type SyncField = "title" | "description"; +/** + * Write-guard scope: `SyncField` for the title/description canonical-value + * protocol, plus `"task"` for the whole-task generic mutation path (status, + * priority, assignee, etc. — fields with no single canonical value). Only + * `beginWrite`/`endWrite`/`shouldRestoreAfterFailedWrite` accept the widened + * type; the canonical-value API stays on `SyncField`. + */ +export type SyncScope = SyncField | "task"; +export const TASK_SCOPE: SyncScope = "task"; +/** Values captured for a whole-task optimistic mutation. */ +export type TaskScopeValues = Record; type CandidateKind = "refetch" | "write"; type CanonicalRecord = { @@ -23,16 +34,27 @@ type GuardState = { pendingWrites: Set; }; +type TaskScopeWrite = { + patch: TaskScopeValues; +}; + +type TaskScopeState = { + baseline: TaskScopeValues; + writes: Map; + outcomes: Map; +}; + const FIELDS: SyncField[] = ["title", "description"]; const canonicalByTaskField = new Map>(); -const guardByTaskField = new Map>(); +const guardByTaskField = new Map>(); const sequenceByTask = new Map(); -const lastWriteIssueSequenceByTaskField = new Map>(); -const lastSuccessfulWriteSequenceByTaskField = new Map>(); +const lastWriteIssueSequenceByTaskField = new Map>(); +const lastSuccessfulWriteSequenceByTaskField = new Map>(); const listenersByTaskField = new Map void>>>(); +const taskScopeByTask = new Map(); -function fieldMap(store: Map>, taskId: string): Map { +function fieldMap(store: Map>, taskId: string): Map { let m = store.get(taskId); if (!m) { m = new Map(); @@ -41,6 +63,10 @@ function fieldMap(store: Map>, taskId: string): Map return m; } +function isSyncField(scope: SyncScope): scope is SyncField { + return scope !== TASK_SCOPE; +} + /** Monotonic per-task counter; call at request-issue time (refetch or write). */ export function nextTaskSequence(taskId: string): number { const next = (sequenceByTask.get(taskId) ?? 0) + 1; @@ -203,6 +229,39 @@ export function recordRefetchCandidate( return recordCandidate(taskId, field, { rawValue, updatedAtRaw, sequence, kind: "refetch" }); } +/** + * Records a write's latest settled sequence and, for whole-task writes, its + * persisted values when supplied. + * The values are replayed in issue order after the final overlapping write + * settles, so an older failure cannot discard a newer failure's retained + * rollback baseline (or an approval-gate redirect). + */ +export function recordWriteSettled( + taskId: string, + scope: SyncScope, + sequence: number, + settledValues?: TaskScopeValues, +): void { + const fm = fieldMap(lastSuccessfulWriteSequenceByTaskField, taskId); + const current = fm.get(scope); + if (current === undefined || sequence > current) { + fm.set(scope, sequence); + } + if (scope !== TASK_SCOPE) return; + const state = taskScopeByTask.get(taskId); + if (!state) return; + const write = state.writes.get(sequence); + state.outcomes.set(sequence, settledValues ? { ...settledValues } : { ...write?.patch }); +} + +/** Records a whole-task write that did not persist any values. */ +export function recordWriteFailed(taskId: string, scope: SyncScope, sequence: number): void { + if (scope === TASK_SCOPE) { + const state = taskScopeByTask.get(taskId); + if (state) state.outcomes.set(sequence, null); + } +} + /** * AC-58: records a successful write's own response value as canonical. * Must be called before the caller releases the write's guard contribution @@ -215,11 +274,7 @@ export function recordWriteSuccess( updatedAtRaw: string | null | undefined, sequence: number, ): boolean { - const fm = fieldMap(lastSuccessfulWriteSequenceByTaskField, taskId); - const current = fm.get(field); - if (current === undefined || sequence > current) { - fm.set(field, sequence); - } + recordWriteSettled(taskId, field, sequence); return recordCandidate(taskId, field, { rawValue, updatedAtRaw, sequence, kind: "write" }); } @@ -233,13 +288,13 @@ export function recordWriteSuccess( */ export function shouldRestoreAfterFailedWrite( taskId: string, - field: SyncField, + scope: SyncScope, sequence: number, ): boolean { - const lastSuccess = lastSuccessfulWriteSequenceByTaskField.get(taskId)?.get(field); + const lastSuccess = lastSuccessfulWriteSequenceByTaskField.get(taskId)?.get(scope); if (lastSuccess !== undefined && lastSuccess > sequence) return false; // AC-44 - const pending = guardByTaskField.get(taskId)?.get(field)?.pendingWrites; + const pending = guardByTaskField.get(taskId)?.get(scope)?.pendingWrites; if (pending) { for (const pendingSeq of pending) { if (pendingSeq > sequence) return false; // AC-53 @@ -250,13 +305,18 @@ export function shouldRestoreAfterFailedWrite( function releaseIfUnguarded( taskId: string, - field: SyncField, - gm: Map, + scope: SyncScope, + gm: Map, guard: GuardState, ): void { if (!guard.editorOpen && guard.pendingWrites.size === 0) { - gm.delete(field); - notifyField(taskId, field); + gm.delete(scope); + if (isSyncField(scope)) { + notifyField(taskId, scope); + } + if (gm.size === 0) { + guardByTaskField.delete(taskId); + } } } @@ -281,21 +341,76 @@ export function closeFieldEditor(taskId: string, field: SyncField): void { * Registers an in-flight write's guard contribution and records this write * as the field's most-recently-issued write for AC-52 staleness comparisons. */ -export function beginWrite(taskId: string, field: SyncField, sequence: number): void { +export function beginWrite( + taskId: string, + scope: SyncScope, + sequence: number, + before?: TaskScopeValues, + patch?: TaskScopeValues, +): void { const gm = fieldMap(guardByTaskField, taskId); - const guard = gm.get(field) ?? { editorOpen: false, pendingWrites: new Set() }; + const guard = gm.get(scope) ?? { editorOpen: false, pendingWrites: new Set() }; guard.pendingWrites.add(sequence); - gm.set(field, guard); - fieldMap(lastWriteIssueSequenceByTaskField, taskId).set(field, sequence); + gm.set(scope, guard); + fieldMap(lastWriteIssueSequenceByTaskField, taskId).set(scope, sequence); + if (scope !== TASK_SCOPE || !before || !patch) return; + + const state = taskScopeByTask.get(taskId) ?? { + baseline: {}, + writes: new Map(), + outcomes: new Map(), + }; + for (const key of Object.keys(patch)) { + if (!Object.prototype.hasOwnProperty.call(state.baseline, key)) { + state.baseline[key] = before[key]; + } + } + state.writes.set(sequence, { patch: { ...patch } }); + taskScopeByTask.set(taskId, state); +} + +function reconcileTaskScope(taskId: string): TaskScopeValues | undefined { + const state = taskScopeByTask.get(taskId); + if (!state) return undefined; + taskScopeByTask.delete(taskId); + if (state.writes.size < 2) return undefined; + + const reconciliation = { ...state.baseline }; + const settledSequences = [...state.outcomes.keys()].sort((a, b) => a - b); + for (const settledSequence of settledSequences) { + const outcome = state.outcomes.get(settledSequence); + if (outcome) Object.assign(reconciliation, outcome); + } + return reconciliation; +} + +function clearTaskScopeSequenceBookkeeping(taskId: string): void { + const issued = lastWriteIssueSequenceByTaskField.get(taskId); + issued?.delete(TASK_SCOPE); + if (issued?.size === 0) lastWriteIssueSequenceByTaskField.delete(taskId); + + const settled = lastSuccessfulWriteSequenceByTaskField.get(taskId); + settled?.delete(TASK_SCOPE); + if (settled?.size === 0) lastSuccessfulWriteSequenceByTaskField.delete(taskId); } /** AC-70: ends a write's guard contribution identically whether it succeeded or failed. */ -export function endWrite(taskId: string, field: SyncField, sequence: number): void { +export function endWrite( + taskId: string, + scope: SyncScope, + sequence: number, +): TaskScopeValues | undefined { const gm = guardByTaskField.get(taskId); - const guard = gm?.get(field); - if (!gm || !guard) return; + const guard = gm?.get(scope); + if (!gm || !guard) return undefined; guard.pendingWrites.delete(sequence); - releaseIfUnguarded(taskId, field, gm, guard); + const taskScopeSettled = scope === TASK_SCOPE && guard.pendingWrites.size === 0; + const reconciliation = taskScopeSettled ? reconcileTaskScope(taskId) : undefined; + if (taskScopeSettled) { + clearTaskScopeSequenceBookkeeping(taskId); + } + releaseIfUnguarded(taskId, scope, gm, guard); + return reconciliation; } /** @@ -323,4 +438,5 @@ export function __resetOfficeTaskContentSyncForTests(): void { lastWriteIssueSequenceByTaskField.clear(); lastSuccessfulWriteSequenceByTaskField.clear(); listenersByTaskField.clear(); + taskScopeByTask.clear(); }