From d4f5e418b264828557ea265a9db909d5334457b3 Mon Sep 17 00:00:00 2001 From: Demid Ostiakov Date: Thu, 3 Sep 2026 00:28:56 +0200 Subject: [PATCH] Fix after-effects after resolving conflicts: Answer the dependency rule where the loader allows it, and say why where it does not --- minibank-fraud-web/src/FraudDesk.tsx | 23 +++++++++--- minibank-fraud-web/src/HistoryScreen.tsx | 30 ++++++++++------ minibank-fraud-web/src/NewPaymentScreen.tsx | 35 ++++++++++++------- .../src/WaitingAuthorizationsScreen.tsx | 17 +++++++++ 4 files changed, 76 insertions(+), 29 deletions(-) diff --git a/minibank-fraud-web/src/FraudDesk.tsx b/minibank-fraud-web/src/FraudDesk.tsx index ec3a74d..b42cb0d 100644 --- a/minibank-fraud-web/src/FraudDesk.tsx +++ b/minibank-fraud-web/src/FraudDesk.tsx @@ -663,11 +663,6 @@ export default function FraudDesk(props: { ? historyErr.failure : null; - // amountReason belongs in here beside filters. Typing something that cannot be read into an - // already empty field leaves the filters untouched, so on filters alone nothing would re-run - // and the analyst would be told nothing at all. - useEffect(() => { void reloadList(); }, [filters, amountReason]); - /* * The address, when it says something other than what is on screen. * @@ -767,6 +762,24 @@ export default function FraudDesk(props: { } } + /* + * WATCHED BY HAND, and the rule is silenced rather than answered. + * + * The array names what should re-read the queue: the filters, and amountReason beside them, + * because typing something unreadable into an already empty field leaves the filters untouched + * and on filters alone nothing would re-run and the analyst would be told nothing at all. + * + * reloadList is left out, and the reason is what it closes over. Wrapping it in useCallback was + * tried and the rule answered with the rest of the list: it reads alerts.length, to ask for a + * page as large as the one on screen, and selectedId, to keep or drop the open case. An honest + * callback is therefore rebuilt whenever either changes, and an effect naming it would re-read + * the whole queue every time an analyst opened an alert. The history screen and the payment + * form ARE wrapped, because their loaders close over nothing that changes between renders; + * this one is not like them. + */ + // eslint-disable-next-line react-hooks/exhaustive-deps + useEffect(() => { void reloadList(); }, [filters, amountReason]); + /** * How many alerts the payment-status exclusion is keeping off the queue. * diff --git a/minibank-fraud-web/src/HistoryScreen.tsx b/minibank-fraud-web/src/HistoryScreen.tsx index bb8badd..de3a9cb 100644 --- a/minibank-fraud-web/src/HistoryScreen.tsx +++ b/minibank-fraud-web/src/HistoryScreen.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState, type ReactNode } from 'react'; +import { useCallback, useEffect, useRef, useState, type ReactNode } from 'react'; import { fetchMyTransfers, formatMoney, type HistoryItem, type Page } from './api'; import ErrorBox from './ErrorBox'; import TableFrame from './TableFrame'; @@ -224,20 +224,20 @@ export default function HistoryScreen() { the loader below is called from the effect and from the retry beside its own error box. */ const alive = useRef(true); - useEffect(() => { - alive.current = true; - void loadFirst(); - return () => { - alive.current = false; - }; - }, []); - /** * The first page, and the way back from a first page that failed. The retry is the same call * rather than a reload of the browser tab: the screen is reached from the rail, and pressing * the entry somebody is already standing on mounts nothing. + * + * useCallback with an empty list, and the list is empty because it is true rather than to + * silence anything: everything this closes over is stable across renders - the state setters, + * the ref, the page size and the fetch - so the function never has to be rebuilt. That is what + * lets the effect below name it and still run once, which is the whole point. Declared above + * the effect and not below it: a name in a dependency array is read while the component + * renders, and a const read before its own line is a ReferenceError rather than a hoisted + * function. */ - async function loadFirst() { + const loadFirst = useCallback(async () => { try { setLoading(true); setError(null); @@ -251,7 +251,15 @@ export default function HistoryScreen() { } finally { if (alive.current) setLoading(false); } - } + }, []); + + useEffect(() => { + alive.current = true; + void loadFirst(); + return () => { + alive.current = false; + }; + }, [loadFirst]); /** * The next page, appended under the rows already on screen. diff --git a/minibank-fraud-web/src/NewPaymentScreen.tsx b/minibank-fraud-web/src/NewPaymentScreen.tsx index 9ced491..e531809 100644 --- a/minibank-fraud-web/src/NewPaymentScreen.tsx +++ b/minibank-fraud-web/src/NewPaymentScreen.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { createPayment, fetchMyBeneficiaries, @@ -176,14 +176,6 @@ export default function NewPaymentScreen({ parked }: { parked: boolean }) { */ const quoteToken = useRef(0); - useEffect(() => { - alive.current = true; - void loadAccounts(); - void loadBeneficiaries(); - return () => { - alive.current = false; - }; - }, []); /** * The accounts, without which there is no form at all. @@ -191,7 +183,13 @@ export default function NewPaymentScreen({ parked }: { parked: boolean }) { * Its failure is the one on this screen that leaves nothing to do, and it is a read that costs * nothing to ask twice, which is exactly the case a retry is for. */ - async function loadAccounts() { + /* + * useCallback with an empty list, and the list is empty because it is true rather than to + * silence anything: everything this closes over is stable across renders - the state setters, + * the ref and the fetch - so the function never has to be rebuilt, and the effect below can + * name it and still run once. + */ + const loadAccounts = useCallback(async () => { try { setLoadingAccounts(true); setAccountsError(null); @@ -210,7 +208,7 @@ export default function NewPaymentScreen({ parked }: { parked: boolean }) { } finally { if (alive.current) setLoadingAccounts(false); } - } + }, []); /** * The address book, fetched beside the accounts and gating nothing. @@ -218,7 +216,7 @@ export default function NewPaymentScreen({ parked }: { parked: boolean }) { * A slow list of payees must not delay a payment, so this has no loading line of its own: the * row simply is not there until the names arrive. */ - async function loadBeneficiaries() { + const loadBeneficiaries = useCallback(async () => { try { const saved = await fetchMyBeneficiaries(); if (!alive.current) return; @@ -229,7 +227,18 @@ export default function NewPaymentScreen({ parked }: { parked: boolean }) { setBeneficiaries([]); setBeneficiariesFailed(true); } - } + }, []); + + /* Declared after both loaders and not before them: a name in a dependency array is read while + the component renders, and a const read before its own line is a ReferenceError. */ + useEffect(() => { + alive.current = true; + void loadAccounts(); + void loadBeneficiaries(); + return () => { + alive.current = false; + }; + }, [loadAccounts, loadBeneficiaries]); /** * A name was chosen, or the choice was given up. diff --git a/minibank-fraud-web/src/WaitingAuthorizationsScreen.tsx b/minibank-fraud-web/src/WaitingAuthorizationsScreen.tsx index a1926ba..7d0ee35 100644 --- a/minibank-fraud-web/src/WaitingAuthorizationsScreen.tsx +++ b/minibank-fraud-web/src/WaitingAuthorizationsScreen.tsx @@ -283,6 +283,23 @@ export default function WaitingAuthorizationsScreen() { useEffect(() => { void loadList(); + /* + * THE ONE EFFECT IN THIS WINDOW THAT IS SILENCED RATHER THAN ANSWERED, and the difference from + * the other three is in what the loader reads. + * + * The history screen, the payment form and the desk all wrap their loader in useCallback and + * name it here, honestly: two of them close over nothing that changes between renders, and the + * desk's closes over exactly the pair its effect already watched. loadList is not like them. It + * reads items.length, to ask for a page as large as the list already on screen, and selectedId, + * to drop a selection that has left it. An honest useCallback would therefore be rebuilt on + * every change to either, and an effect naming it would re-read the whole list every time a + * customer picked a payment. + * + * Keeping it mount-only through a callback means reading those two through refs instead, which + * is a second copy of two pieces of state kept in step by hand for the sake of a lint rule. The + * array is empty because the effect is a mount, and that is said here rather than worked around. + */ + // eslint-disable-next-line react-hooks/exhaustive-deps }, []); /* The deadline of the payment on screen, which is what the tick below is started for. */