fix(quotes): address UI toggle code review

Addresses the 2 IMPORTANT and 4 NIT findings from the Task 9
code-quality review:

- Initial state bug: useState read 'quote' on first render when it
  was null, sealing isNoQuote as false. A previously-persisted
  is_no_quote: true quote would show the toggle as off. Replace
  with a useEffect that syncs once per loaded quote id.

- Dark mode: add dark: variants to the amber card border/background
  to match the project precedent (collection-pool/components.tsx:175).

- Label size: drop from text-sm to text-xs for consistency with
  every other <Label> in the file.

- Error surface: parse the PATCH 400 body and surface data.error
  rather than a generic toast, matching the lookupItem/generatePdf
  pattern in the same file.

- useCallback deps: stop recreating the callback on every toggle
  by stashing the current value in a ref.

- quote-notes-card id: add a comment so the wrapper div's purpose
  is clear.
This commit is contained in:
Dave 2026-08-26 23:47:27 +02:00
parent 0b6226cdf1
commit 6d9b6a8274

View file

@ -1,6 +1,6 @@
"use client";
import { useState, useEffect, useCallback, useMemo, Fragment } from "react";
import { useState, useEffect, useCallback, useMemo, useRef, Fragment } from "react";
import { use } from "react";
import { useRouter } from "next/navigation";
import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card";
@ -146,7 +146,8 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str
* in the same flow (the PDF route bypasses pricing gates when this
* flag is set — see Task 7).
*/
const [isNoQuote, setIsNoQuote] = useState(!!quote?.is_no_quote);
const [isNoQuote, setIsNoQuote] = useState(false);
const [noQuoteSyncedFor, setNoQuoteSyncedFor] = useState<string | null>(null);
const [noQuoteSaving, setNoQuoteSaving] = useState(false);
/**
@ -532,6 +533,22 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str
useEffect(() => { fetchData(); }, [fetchData]);
/**
* Sync the `isNoQuote` toggle once per loaded quote id. The local
* state is otherwise sealed at mount time, when `quote` is still
* null (useState only reads its initial value on the first render),
* so a previously-persisted `is_no_quote: true` quote would render
* the toggle as off. Tracking the last synced id protects
* optimistic toggles from being clobbered by unrelated `fetchData()`
* refreshes once the initial sync is in place.
*/
useEffect(() => {
if (quote && quote.id !== noQuoteSyncedFor) {
setIsNoQuote(!!quote.is_no_quote);
setNoQuoteSyncedFor(quote.id);
}
}, [quote, noQuoteSyncedFor]);
/**
* Items to render in the table. Auto-sort: incomplete first, done
* last. Original row number preserved via `originalIdx` so the
@ -761,9 +778,12 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str
* When flipped on (true), scrolls the `quote_notes` editor into view
* so the operator can type the reason in the same flow.
*/
const isNoQuoteRef = useRef(false);
isNoQuoteRef.current = isNoQuote;
const onToggleNoQuote = useCallback(async (next: boolean) => {
if (!quote) return;
const previous = isNoQuote;
const previous = isNoQuoteRef.current;
setIsNoQuote(next); // optimistic
setNoQuoteSaving(true);
try {
@ -774,7 +794,8 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str
});
if (!res.ok) {
setIsNoQuote(previous); // rollback
toast.error("Couldn't update — try again.");
const data = (await res.json().catch(() => null)) as { error?: string } | null;
toast.error(data?.error || "Couldn't update — try again.");
} else if (next) {
// Scroll the quote_notes editor into view so the operator can
// type the reason immediately.
@ -788,7 +809,7 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str
} finally {
setNoQuoteSaving(false);
}
}, [isNoQuote, quote]);
}, [quote]);
/**
* Swap the customer on this quote. Called by the `<CustomerSearch>`
@ -937,12 +958,12 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str
className={cn(
"mt-3 flex items-center justify-between rounded-md border p-3",
isNoQuote
? "border-amber-200 bg-amber-50"
: "border-transparent bg-transparent",
? "border-amber-200 bg-amber-50 dark:border-amber-500/30 dark:bg-amber-500/10"
: "border-transparent bg-transparent"
)}
>
<div className="space-y-0.5">
<Label htmlFor="is_no_quote" className="text-sm font-medium">
<Label htmlFor="is_no_quote" className="text-xs font-medium">
No-quote letter
</Label>
<p className="text-xs text-muted-foreground">
@ -1600,6 +1621,7 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str
generate in one continuous flow. The wrapping div carries the
`quote-notes-card` id so the no-quote toggle can scrollIntoView
here when the operator flips the toggle on. */}
{/* scroll target for the no-quote toggle */}
<div id="quote-notes-card">
<QuoteNoteCard
quoteId={quote.id}