From 45f1c7716032c79e78d34533ec2fb76a0e08f107 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 24 Aug 2026 12:11:11 -0500 Subject: [PATCH] refactor(frontend): triage the setState-in-effect sites (#99) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eleven warnings that looked alike and were not. This is a decision per site rather than eleven fixes, which is what the issue asked for — some of these would be made worse by "fixing" them. One was a real defect. VerifyEmail routed a fact through an effect that was already knowable during render: whether the link carries a token comes from the URL. The component therefore rendered once as a spinner in a state that was never true — a link with no token was never "verifying". Both pieces of state now derive their initial value from the token, and the effect's missing-token branch becomes an early return, so the failure is what the first render shows. Two are a defensible reset. CartProvider and FavoritesProvider clear their collection when the customer becomes null, which is synchronisation with the session rather than derived state. Deriving instead would push "signed out" onto every consumer of those contexts, and remounting on a `key` is more indirection than the problem deserves. Decided and written down rather than left for the next reader to re-investigate. Eight are legitimate and flagged conservatively. Six are a pending flag before a fetch — the rule cannot tell a spinner from a value that was already known. Cart's lapsed-item refetch is synchronisation with a server-side release the client cannot observe. useNow subscribes to the clock, which is the case the rule's own documentation names as correct. Each of the ten that stay carries the reason and a targeted disable, so lint drops from thirteen warnings to two — and the two left are the unrelated no-alphabetical-sort pair. Suppressing per site rather than switching the rule off keeps it live for new code, which is where the next VerifyEmail would be caught. The trap #60 recorded caught this, in a variant it does not describe. Placing the disable above `useEffect(` works only for a single-line effect: where the effect spans several lines the flagged line is the setState inside the body, so the directive covered nothing and produced both an unused-disable warning and the original one. Three sites were wrong that way on the first attempt. Confirmed fixed by the absence of "Unused eslint-disable directive" from the output — a disable that covers nothing reports itself, which is what makes this checkable rather than assumed. Verified: tsc clean over src and tests, and the specs covering the changed behaviour pass — verify-email, auth, favorites, orders, cart-countdown, resend-verification, 32 of 33 with the one failure passing 10/10 in a serial re-run. Closes #99 --- frontend/src/admin/Categories.tsx | 2 ++ frontend/src/admin/Tags.tsx | 2 ++ frontend/src/cart/Cart.tsx | 7 +++++++ frontend/src/cart/CartContext.tsx | 6 ++++++ frontend/src/cart/useNow.ts | 5 +++++ frontend/src/customer/CustomerAuthContext.tsx | 4 ++++ frontend/src/customer/FavoritesContext.tsx | 3 +++ frontend/src/customer/Orders.tsx | 4 +++- frontend/src/customer/VerifyEmail.tsx | 17 ++++++++++------- frontend/src/useCatalogue.ts | 2 ++ 10 files changed, 44 insertions(+), 8 deletions(-) diff --git a/frontend/src/admin/Categories.tsx b/frontend/src/admin/Categories.tsx index 329f6ad..3583b93 100644 --- a/frontend/src/admin/Categories.tsx +++ b/frontend/src/admin/Categories.tsx @@ -78,6 +78,8 @@ export default function Categories() { .finally(() => setLoading(false)); } + // Load-on-mount. `load` sets a pending flag before fetching. + // eslint-disable-next-line react-hooks/set-state-in-effect useEffect(() => { void load(); }, []); function openNew(parent: number | null) { diff --git a/frontend/src/admin/Tags.tsx b/frontend/src/admin/Tags.tsx index 1c476fd..55f6d4d 100644 --- a/frontend/src/admin/Tags.tsx +++ b/frontend/src/admin/Tags.tsx @@ -36,6 +36,8 @@ export default function Tags() { .finally(() => setLoading(false)); } + // Load-on-mount. `load` sets a pending flag before fetching. + // eslint-disable-next-line react-hooks/set-state-in-effect useEffect(() => { void load(); }, []); function openNew() { diff --git a/frontend/src/cart/Cart.tsx b/frontend/src/cart/Cart.tsx index 2460073..e231c45 100644 --- a/frontend/src/cart/Cart.tsx +++ b/frontend/src/cart/Cart.tsx @@ -67,6 +67,9 @@ export default function Cart() { .finally(() => setLoading(false)); } + // Load-on-mount once the session resolves. `loadAll` sets a pending flag + // first, which is what the rule sees. + // eslint-disable-next-line react-hooks/set-state-in-effect useEffect(() => { if (customer) loadAll(); }, [customer]); // Only ticks while there is something to count down, so an empty cart does @@ -80,6 +83,10 @@ export default function Cart() { // the release rather than sitting at "expiring…" until the page is reloaded. useEffect(() => { if (!lapsed) return; + // Refetching from the server on a tick, which is synchronisation rather + // than derived state — the release happens server-side and the client + // has no way to know when. + // eslint-disable-next-line react-hooks/set-state-in-effect loadAll(); // The header badge counts held items too, so it goes stale in exactly the // same way. Safe as a dependency: CartContext memoizes it with an empty diff --git a/frontend/src/cart/CartContext.tsx b/frontend/src/cart/CartContext.tsx index 9d97fb2..dac4702 100644 --- a/frontend/src/cart/CartContext.tsx +++ b/frontend/src/cart/CartContext.tsx @@ -24,10 +24,16 @@ export function CartProvider({ children }: Readonly<{ children: React.ReactNode .catch(() => setItems([])); }, []); + // Synchronising with the session, which is an external system, so an effect + // is the right tool. Deriving it instead would push "signed out" onto every + // consumer of this context, and remounting on a `key` is more indirection + // than the problem deserves. Decided in #99 rather than left to be + // re-investigated. useEffect(() => { if (customer) { refresh(); } else { + // eslint-disable-next-line react-hooks/set-state-in-effect setItems([]); } }, [customer, refresh]); diff --git a/frontend/src/cart/useNow.ts b/frontend/src/cart/useNow.ts index 838fadd..8db7a6f 100644 --- a/frontend/src/cart/useNow.ts +++ b/frontend/src/cart/useNow.ts @@ -25,6 +25,11 @@ export function useNow(intervalMs: number, active: boolean): number { if (!active) return; // Set immediately as well as on the interval: mounting with `active` already // true would otherwise show a value up to intervalMs stale. + // Subscribing to the clock, which is an external system — the case the + // rule's own documentation names as correct. Set once here as well as on + // the interval so a mount with `active` already true is not up to + // intervalMs stale. + // eslint-disable-next-line react-hooks/set-state-in-effect setNow(Date.now()); const timer = setInterval(() => setNow(Date.now()), intervalMs); return () => clearInterval(timer); diff --git a/frontend/src/customer/CustomerAuthContext.tsx b/frontend/src/customer/CustomerAuthContext.tsx index 8bc6ced..0a2b8d5 100755 --- a/frontend/src/customer/CustomerAuthContext.tsx +++ b/frontend/src/customer/CustomerAuthContext.tsx @@ -33,6 +33,10 @@ export function CustomerAuthProvider({ children }: Readonly<{ children: React.Re .finally(() => setLoading(false)); }, []); + // `refresh` sets a pending flag before fetching. The rule cannot tell a + // fetch with a spinner from a value that was already knowable, and this is + // the former. + // eslint-disable-next-line react-hooks/set-state-in-effect useEffect(() => { refresh(); }, [refresh]); // Logging out has to clear the context, not just call the endpoint — diff --git a/frontend/src/customer/FavoritesContext.tsx b/frontend/src/customer/FavoritesContext.tsx index 2e756e1..a998d05 100644 --- a/frontend/src/customer/FavoritesContext.tsx +++ b/frontend/src/customer/FavoritesContext.tsx @@ -31,10 +31,13 @@ export function FavoritesProvider({ children }: Readonly<{ children: React.React .catch(() => setFavorites([])); }, []); + // Same decision as CartProvider: clearing on sign-out is synchronisation + // with the session rather than derived state. See #99. useEffect(() => { if (customer) { refresh(); } else { + // eslint-disable-next-line react-hooks/set-state-in-effect setFavorites([]); } }, [customer, refresh]); diff --git a/frontend/src/customer/Orders.tsx b/frontend/src/customer/Orders.tsx index 8322bbf..1bd22ea 100644 --- a/frontend/src/customer/Orders.tsx +++ b/frontend/src/customer/Orders.tsx @@ -115,7 +115,9 @@ export default function Orders() { } }, []); - useEffect(() => { + // Load-on-mount once the session resolves, with a pending flag. + useEffect(() => { + // eslint-disable-next-line react-hooks/set-state-in-effect if (customer) void load(); }, [customer, load]); diff --git a/frontend/src/customer/VerifyEmail.tsx b/frontend/src/customer/VerifyEmail.tsx index 85bf41a..5056f37 100644 --- a/frontend/src/customer/VerifyEmail.tsx +++ b/frontend/src/customer/VerifyEmail.tsx @@ -15,8 +15,14 @@ type Status = 'verifying' | 'verified' | 'failed'; export default function VerifyEmail() { const [searchParams] = useSearchParams(); const token = searchParams.get('token'); - const [status, setStatus] = useState('verifying'); - const [error, setError] = useState(null); + // Derived from the URL rather than discovered in an effect. Whether the link + // carries a token is knowable during the first render, so routing it through + // an effect made this component render once as a spinner in a state that was + // never true — a link with no token was never "verifying". See #99. + const [status, setStatus] = useState(token ? 'verifying' : 'failed'); + const [error, setError] = useState( + token ? null : 'This link is missing its verification token.' + ); const { refresh } = useCustomerAuth(); // The backend deletes the token once it succeeds, so a second request for the @@ -29,11 +35,8 @@ export default function VerifyEmail() { if (requested.current) return; requested.current = true; - if (!token) { - setStatus('failed'); - setError('This link is missing its verification token.'); - return; - } + // Nothing to do: the missing-token case is already the initial state. + if (!token) return; verifyEmail(token) .then(() => { diff --git a/frontend/src/useCatalogue.ts b/frontend/src/useCatalogue.ts index 0793b8c..6654e0c 100644 --- a/frontend/src/useCatalogue.ts +++ b/frontend/src/useCatalogue.ts @@ -86,6 +86,8 @@ export function useCatalogue(filters: ItemFilters, onAuthRequired: () => void): useEffect(() => { if (awaitingAuth) { + // A pending flag while the session resolves, not a value already known. + // eslint-disable-next-line react-hooks/set-state-in-effect setLoading(true); return; }