fix(collection-confirm): hoist onCostChange out of setEdit updater + preserve null rollback

Co-authored-by: Orca <help@stably.ai>

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(<Page />);
+
+    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();
+  });
 });

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;

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(<ItemsTable rows={baseRows} onCostChange={onCostChange} />);
+
+    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();

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) {
                   </div>
                 ) : (
                   <div className="flex items-center justify-end gap-1">
-                    <span>R{(row.cost_price ?? 0).toFixed(2)}</span>
+                    <span data-cost-value={row.cost_price === null ? "null" : String(row.cost_price)}>R{(row.cost_price ?? 0).toFixed(2)}</span>
                     {isStockRow ? null : (
                       <button
                         type="button"