From fea67b15bac161afe41f8063549c8c43b3306f0c Mon Sep 17 00:00:00 2001 From: SJCoetz Date: Mon, 3 Aug 2026 02:46:28 +0200 Subject: [PATCH] fix(collection-confirm): hoist onCostChange out of setEdit updater + preserve null rollback Co-authored-by: Orca --- .../(app)/collection-confirm/page.test.tsx | 60 +++++++++++++++++++ src/app/(app)/collection-confirm/page.tsx | 6 +- .../collection-confirm/items-table.test.tsx | 29 +++++++++ .../collection-confirm/items-table.tsx | 33 +++++----- 4 files changed, 111 insertions(+), 17 deletions(-) diff --git a/src/app/(app)/collection-confirm/page.test.tsx b/src/app/(app)/collection-confirm/page.test.tsx index ff41cd7..aafd3ed 100644 --- a/src/app/(app)/collection-confirm/page.test.tsx +++ b/src/app/(app)/collection-confirm/page.test.tsx @@ -472,4 +472,64 @@ describe("CollectionConfirmPage", () => { ).toBe(true), ); }); + + it("preserves a null prior cost_price when a cost-edit POST fails (does not coerce to 0)", async () => { + // Drive the page with one row whose cost_price is null. The + // /cost endpoint is forced to fail so the rollback path runs. + // The cell carries a `data-cost-value` attribute exposing the + // raw value (`"null"` vs `"0"`) — the displayed text is + // "R0.00" in both cases (the component uses `?? 0` for the + // formatted display) so we need the raw attribute to assert + // the rollback preserved `null` instead of stomping it to 0. + const nullCostRow = makeRow({ id: "row-null", cost_price: null }); + vi.spyOn(window, "fetch").mockImplementation(async (input) => { + const url = fetchUrl(input); + if (url === "/api/v1/collection-confirm") { + return jsonResponse([nullCostRow]); + } + if (typeof url === "string" && url.startsWith("/api/v1/order-items/")) { + return jsonResponse({}, { ok: false, status: 500 }); + } + return jsonResponse({}); + }); + + const user = userEvent.setup(); + render(); + + await waitFor(() => + expect(screen.getByTestId("po-card-PO-1")).toBeInTheDocument(), + ); + + // Before the edit, the cell exposes `null` as its raw value. + const cell = document.querySelector('[data-cost-value]'); + expect(cell).not.toBeNull(); + expect(cell!.getAttribute("data-cost-value")).toBe("null"); + + // Edit and submit — POST is mocked to fail. + await user.click(screen.getByTestId("cost-edit-row-null")); + const input = screen.getByTestId("cost-input-row-null"); + await user.clear(input); + await user.type(input, "42"); + await user.keyboard("{Enter}"); + + // After the rollback, the cell's raw value is back to "null" — + // not "0". This is the actual fix: the previous code coerced + // `prevCost ?? 0` and flipped the row's cost_price from null + // to 0 in the local state, leaving the cell to render as + // "R0.00" until the next load() refreshed the row from the + // server. + await waitFor(() => + expect( + document.querySelector('[data-cost-value]')?.getAttribute("data-cost-value"), + ).toBe("null"), + ); + + // The cost override is also dropped — opening the confirm + // dialog should show "0 cost overrides applied". + await user.click(screen.getByTestId("po-card-confirm-PO-1")); + const dialog = await screen.findByRole("dialog"); + expect( + within(dialog).getByText(/0 cost overrides applied/i), + ).toBeInTheDocument(); + }); }); diff --git a/src/app/(app)/collection-confirm/page.tsx b/src/app/(app)/collection-confirm/page.tsx index 0a19f84..30f70ad 100644 --- a/src/app/(app)/collection-confirm/page.tsx +++ b/src/app/(app)/collection-confirm/page.tsx @@ -134,10 +134,10 @@ export default function CollectionConfirmPage() { } catch (err) { const message = err instanceof Error ? err.message : String(err); toast.error(message); - // Snap back: restore the previous cost (or null) on the row - // and drop the override from the local map. + // Snap back: restore the previous cost (preserving null) + // on the row and drop the override from the local map. setRows((prev) => - prev?.map((r) => (r.id === id ? { ...r, cost_price: prevCost ?? 0 } : r)) ?? null, + prev?.map((r) => (r.id === id ? { ...r, cost_price: prevCost } : r)) ?? null, ); setCostOverrides((prev) => { if (!(id in prev)) return prev; diff --git a/src/components/collection-confirm/items-table.test.tsx b/src/components/collection-confirm/items-table.test.tsx index 4430cee..a0702c6 100644 --- a/src/components/collection-confirm/items-table.test.tsx +++ b/src/components/collection-confirm/items-table.test.tsx @@ -100,6 +100,35 @@ describe("ItemsTable", () => { expect(screen.queryByTestId("cost-input-row-1")).not.toBeInTheDocument(); }); + it("does not trigger a React warning when saving a cost on Enter", async () => { + // React's "Cannot update a component while rendering a different + // component" warning is logged via console.error. Spy on it and + // assert no such call was made after pressing Enter on the cost + // input — `onCostChange` must NOT run inside the `setEdit` + // updater. + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const user = userEvent.setup(); + const onCostChange = vi.fn(); + render(); + + await user.click(screen.getByTestId("cost-edit-row-1")); + const input = screen.getByTestId("cost-input-row-1"); + await user.clear(input); + await user.type(input, "77"); + await user.keyboard("{Enter}"); + + expect(onCostChange).toHaveBeenCalledWith("row-1", 77); + const reactWarnings = consoleSpy.mock.calls.filter((args) => + args.some( + (arg) => + typeof arg === "string" && + arg.includes("Cannot update a component") && + arg.includes("while rendering"), + ), + ); + expect(reactWarnings).toHaveLength(0); + }); + it("calls onCostChange on blur with the new value", async () => { const user = userEvent.setup(); const onCostChange = vi.fn(); diff --git a/src/components/collection-confirm/items-table.tsx b/src/components/collection-confirm/items-table.tsx index 52a0d84..d6f1a9b 100644 --- a/src/components/collection-confirm/items-table.tsx +++ b/src/components/collection-confirm/items-table.tsx @@ -48,19 +48,24 @@ export function ItemsTable({ rows, onCostChange }: Props) { const cancel = () => setEdit(null); const save = (rowId: string) => { - setEdit((current) => { - if (!current || current.id !== rowId) return current; - const trimmed = current.value.trim(); - if (trimmed === "") { - return { ...current, error: "Required" }; - } - const parsed = Number(trimmed); - if (Number.isNaN(parsed) || parsed < 0) { - return { ...current, error: "Must be a non-negative number" }; - } - onCostChange(rowId, parsed); - return null; - }); + // React forbids calling parent updaters from inside a `setState` + // updater because the updater may run twice or not at all under + // concurrent rendering. Read the latest `edit` from the closure + // (the event handler captures the latest `save` on every render) + // and validate + dispatch the side effect OUTSIDE `setEdit`. + if (!edit || edit.id !== rowId) return; + const trimmed = edit.value.trim(); + if (trimmed === "") { + setEdit({ ...edit, error: "Required" }); + return; + } + const parsed = Number(trimmed); + if (Number.isNaN(parsed) || parsed < 0) { + setEdit({ ...edit, error: "Must be a non-negative number" }); + return; + } + setEdit(null); + onCostChange(rowId, parsed); }; const updateValue = (rowId: string, value: string) => { @@ -135,7 +140,7 @@ export function ItemsTable({ rows, onCostChange }: Props) { ) : (
- R{(row.cost_price ?? 0).toFixed(2)} + R{(row.cost_price ?? 0).toFixed(2)} {isStockRow ? null : (