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 <carlosmflorencio@gmail.com>
This commit is contained in:
@@ -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<OfficeTask>) => {
|
||||
current = { ...current, ...patch };
|
||||
}),
|
||||
updateStatus: vi.fn(),
|
||||
onError: vi.fn(),
|
||||
} satisfies StatusDropDeps;
|
||||
return { d, getCurrent: () => current };
|
||||
}
|
||||
|
||||
function deferred<T>() {
|
||||
let resolve!: (value: T) => void;
|
||||
let reject!: (reason?: unknown) => void;
|
||||
const promise = new Promise<T>((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<void>();
|
||||
const newer = deferred<void>();
|
||||
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<void>();
|
||||
const newer = deferred<void>();
|
||||
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<void>();
|
||||
const newer = deferred<void>();
|
||||
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<void>();
|
||||
const newer = deferred<void>();
|
||||
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<void>();
|
||||
const newer = deferred<void>();
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<OfficeTask> {
|
||||
const patch: Partial<OfficeTask> = {};
|
||||
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).
|
||||
|
||||
@@ -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<Task>) => setLiveTask((t) => ({ ...t, ...patch })),
|
||||
restore: (snapshot: Task) => setLiveTask(snapshot),
|
||||
};
|
||||
return (
|
||||
<StateProvider>
|
||||
<TaskOptimisticContextProvider value={ctx}>
|
||||
<StatusPicker task={liveTask} />
|
||||
</TaskOptimisticContextProvider>
|
||||
</StateProvider>
|
||||
);
|
||||
}
|
||||
|
||||
function deferred<T>() {
|
||||
let resolve!: (value: T) => void;
|
||||
let reject!: (reason?: unknown) => void;
|
||||
const promise = new Promise<T>((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(<LiveWrapper initialTask={task} />);
|
||||
|
||||
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", () => {
|
||||
|
||||
@@ -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 */
|
||||
}
|
||||
};
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
const inner = (persisted.task as Record<string, unknown>) ?? 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,
|
||||
|
||||
@@ -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 (
|
||||
<StateProvider>
|
||||
@@ -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<typeof useOptimisticTaskMutation> | null } = {
|
||||
current: null,
|
||||
};
|
||||
const storeApiRef: { current: ReturnType<typeof useAppStoreApi> | null } = { current: null };
|
||||
|
||||
function Inner() {
|
||||
const [task, setTask] = useState(initialTask);
|
||||
taskRef.current = task;
|
||||
const ctxValue = useMemo(
|
||||
() => ({
|
||||
task,
|
||||
applyPatch: (patch: Partial<Task>) => setTask((t) => ({ ...t, ...patch })),
|
||||
restore: (snapshot: Task) => setTask(snapshot),
|
||||
}),
|
||||
[task],
|
||||
);
|
||||
return (
|
||||
<TaskOptimisticContextProvider value={ctxValue}>
|
||||
<GenericMutationProbe
|
||||
onReady={(m, s) => {
|
||||
mutateRef.current = m;
|
||||
storeApiRef.current = s;
|
||||
}}
|
||||
/>
|
||||
</TaskOptimisticContextProvider>
|
||||
);
|
||||
}
|
||||
|
||||
const StoreSeed = makeStoreSeed(initialOffice);
|
||||
|
||||
function Wrapper() {
|
||||
return (
|
||||
<StateProvider>
|
||||
<StoreSeed>
|
||||
<Inner />
|
||||
</StoreSeed>
|
||||
</StateProvider>
|
||||
);
|
||||
}
|
||||
|
||||
return { Wrapper, taskRef, mutateRef, storeApiRef };
|
||||
}
|
||||
|
||||
function deferred<T>() {
|
||||
let resolve!: (value: T) => void;
|
||||
let reject!: (reason?: unknown) => void;
|
||||
@@ -125,6 +187,20 @@ function TitleHookProbe({
|
||||
return null;
|
||||
}
|
||||
|
||||
function GenericMutationProbe({
|
||||
onReady,
|
||||
}: {
|
||||
onReady: (
|
||||
mutate: ReturnType<typeof useOptimisticTaskMutation>,
|
||||
storeApi: ReturnType<typeof useAppStoreApi>,
|
||||
) => 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(<Wrapper />);
|
||||
|
||||
const older = deferred<void>();
|
||||
const newer = deferred<void>();
|
||||
let p1!: Promise<void>;
|
||||
let p2!: Promise<void>;
|
||||
|
||||
// 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(<Wrapper />);
|
||||
|
||||
const older = deferred<void>();
|
||||
const newer = deferred<void>();
|
||||
let p1!: Promise<void>;
|
||||
let p2!: Promise<void>;
|
||||
|
||||
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(<Wrapper />);
|
||||
|
||||
const older = deferred<void>();
|
||||
const newer = deferred<void>();
|
||||
let p1!: Promise<void>;
|
||||
let p2!: Promise<void>;
|
||||
|
||||
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(<Wrapper />);
|
||||
|
||||
const older = deferred<void>();
|
||||
const newer = deferred<void>();
|
||||
let p1!: Promise<void>;
|
||||
let p2!: Promise<void>;
|
||||
|
||||
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(<Wrapper />);
|
||||
|
||||
const older = deferred<void>();
|
||||
const newer = deferred<void>();
|
||||
let p1!: Promise<void>;
|
||||
let p2!: Promise<void>;
|
||||
|
||||
// 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(<Wrapper />);
|
||||
|
||||
const older = deferred<void>();
|
||||
const newer = deferred<void>();
|
||||
let p1!: Promise<void>;
|
||||
let p2!: Promise<void>;
|
||||
|
||||
// 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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<Task>): Partial<OfficeTask> {
|
||||
return out;
|
||||
}
|
||||
|
||||
function toTaskScopeValues(patch: Partial<Task>): 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<Task>,
|
||||
): 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<OfficeTask> {
|
||||
const patch = toOfficeTaskPatch(values as Partial<Task>);
|
||||
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<Task>) => void,
|
||||
storeApi: ReturnType<typeof useAppStoreApi>,
|
||||
): void {
|
||||
applyLocalPatch(values as Partial<Task>);
|
||||
storeApi.getState().patchTaskInStore(taskId, toOfficeTaskRollbackPatch(values));
|
||||
}
|
||||
|
||||
type TaskMutationFailure = {
|
||||
taskId: string;
|
||||
sequence: number;
|
||||
err: unknown;
|
||||
snapshot: Task;
|
||||
storeSnapshot: OfficeTask | undefined;
|
||||
ctx: TaskOptimisticContextValue;
|
||||
storeApi: ReturnType<typeof useAppStoreApi>;
|
||||
};
|
||||
|
||||
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<Task> = { 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);
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
type CandidateKind = "refetch" | "write";
|
||||
|
||||
type CanonicalRecord = {
|
||||
@@ -23,16 +34,27 @@ type GuardState = {
|
||||
pendingWrites: Set<number>;
|
||||
};
|
||||
|
||||
type TaskScopeWrite = {
|
||||
patch: TaskScopeValues;
|
||||
};
|
||||
|
||||
type TaskScopeState = {
|
||||
baseline: TaskScopeValues;
|
||||
writes: Map<number, TaskScopeWrite>;
|
||||
outcomes: Map<number, TaskScopeValues | null>;
|
||||
};
|
||||
|
||||
const FIELDS: SyncField[] = ["title", "description"];
|
||||
|
||||
const canonicalByTaskField = new Map<string, Map<SyncField, CanonicalRecord>>();
|
||||
const guardByTaskField = new Map<string, Map<SyncField, GuardState>>();
|
||||
const guardByTaskField = new Map<string, Map<SyncScope, GuardState>>();
|
||||
const sequenceByTask = new Map<string, number>();
|
||||
const lastWriteIssueSequenceByTaskField = new Map<string, Map<SyncField, number>>();
|
||||
const lastSuccessfulWriteSequenceByTaskField = new Map<string, Map<SyncField, number>>();
|
||||
const lastWriteIssueSequenceByTaskField = new Map<string, Map<SyncScope, number>>();
|
||||
const lastSuccessfulWriteSequenceByTaskField = new Map<string, Map<SyncScope, number>>();
|
||||
const listenersByTaskField = new Map<string, Map<SyncField, Set<(value: string) => void>>>();
|
||||
const taskScopeByTask = new Map<string, TaskScopeState>();
|
||||
|
||||
function fieldMap<T>(store: Map<string, Map<SyncField, T>>, taskId: string): Map<SyncField, T> {
|
||||
function fieldMap<K, T>(store: Map<string, Map<K, T>>, taskId: string): Map<K, T> {
|
||||
let m = store.get(taskId);
|
||||
if (!m) {
|
||||
m = new Map();
|
||||
@@ -41,6 +63,10 @@ function fieldMap<T>(store: Map<string, Map<SyncField, T>>, 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<SyncField, GuardState>,
|
||||
scope: SyncScope,
|
||||
gm: Map<SyncScope, GuardState>,
|
||||
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<number>() };
|
||||
const guard = gm.get(scope) ?? { editorOpen: false, pendingWrites: new Set<number>() };
|
||||
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<number, TaskScopeWrite>(),
|
||||
outcomes: new Map<number, TaskScopeValues | null>(),
|
||||
};
|
||||
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();
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user