diff --git a/CHANGELOG.md b/CHANGELOG.md index a11b01750..de2fdd525 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,16 @@ All notable changes to Turbo EA are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/). +## [2.32.1] - 2026-07-27 + +### Fixed +- **Changing an inventory filter while a slower request was still running could leave the grid showing the wrong cards.** Clearing the type filter fetches the whole repository; picking a specific type straight afterwards came back first, and then the slow response landed and overwrote the grid — so the grid listed every card while the sidebar said «Application». Filter-driven requests are now cancelled when a newer one starts, and a superseded response is discarded instead of applied. +- **The Matrix report could draw one pair of card types under the labels of another.** It never cleared the previous data, so after switching an axis the old grid stayed on screen beneath the new row and column headings, with nothing to indicate the two disagreed. Switching axes now shows the loading indicator until the matching data arrives. +- **Typing in the inventory search box fired a full repository request per keystroke.** The search term now settles for a moment before it is sent, and the grid keeps its loading indicator from the very first keystroke, so it never looks finished while it is still showing results for what you typed before. Type, approval and archived toggles stay instant. +- **The same stale-result problem affected the Cost, Dependency, Capability Map and Lifecycle reports, the Todos list and the PPM portfolio.** Every view whose data reloads when you change a picker now discards results for a selection you have moved on from. +- **The portfolio reports could sit on a loading spinner forever.** Switching card type cleared the chart before the new data arrived, and a request that failed left nothing to bring it back. A failure now shows an error message instead. +- **Card pickers could get stuck showing results for a search you had already replaced.** Changing the type filter or search term while a request was in flight dropped the new request outright and never retried it. This affected relation, parent and vendor pickers throughout the app, and the search fields in the survey builder, calculation tester, survey response form and End-of-Life linking. + ## [2.32.0] - 2026-07-27 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 6486e3fb9..4e182fa94 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -199,6 +199,8 @@ The **Extension Store** (Admin → Extensions) installs vendor-signed, licensed - Feature-specific components go in `src/features/{feature}/`. - Use `api.get()`, `api.post()`, `api.patch()`, `api.delete()` from `src/api/client.ts` for all API calls. - JWT token is in `sessionStorage` (not localStorage). Use `setToken()`/`clearToken()` from `client.ts`. +- **Filter-driven GETs must go through the request hooks — never a bare `api.get` in a `useEffect`.** Any fetch keyed on user-controlled state (a type picker, a search box, a tab, a metric toggle) races: the request for the *previous* value can land after the one for the current value and overwrite it, leaving the grid or chart showing data the controls say you are not looking at (#882). Use `useApiQuery(path | null)` (`frontend/src/hooks/useApiQuery.ts`) when one payload feeds one state; `useAbortableEffect(fn, deps)` (`frontend/src/hooks/useLatestRequest.ts`) when the effect writes several states or fans out over `Promise.all`; and the `useLatestRequest()` primitive when the fetch must also be callable imperatively — `InventoryPage`'s `loadData`, which 13 mutation handlers refresh through. All three abort the predecessor **and** bump a generation token. The token is not redundant: an already-resolved response cannot be aborted, and test doubles ignore `AbortSignal` entirely. Two rules are routinely missed and are exactly what re-breaks this: **(1)** guard *every* `setState` with `isCurrent()`, and **(2)** clear the loading flag **only** when `isCurrent()` — an unconditional `finally { setLoading(false) }` lets the aborted predecessor clear the spinner while the winner is still in flight, flashing "done" over stale rows. Debounce free-text search with `useDebouncedValue(value, 300)` and OR its `pending` flag into the loading prop, so a grid never looks settled while it is still showing the previous query's rows; keep discrete toggles instant. `api.get(path, { signal })` rejects with an `AbortError` — if you call it outside these hooks you own that rejection, so check `isAbortError(err)` from `src/api/client.ts`. `signal` is deliberately **not** offered on `post`/`patch`/`put`/`delete`: aborting a mutation does not undo it server-side. + - **Boot-time singleton hooks must use the inflight-promise pattern.** Hooks that fetch a process-wide value once (`useMetamodel`, `useDateFormat`, `useCurrency`, `useBpmEnabled`, `usePpmEnabled`, `useTurboLensReady`, …) keep a module-level `_cache` plus an `_inflight: Promise | null`. The fetch helper checks **both** — if `_cache` has the value, return it; if `_inflight` is non-null, return that promise; only otherwise start a new fetch and store it in `_inflight`, clearing it on settle. The naive `if (_cache === null) _fetch()` pattern races: when several components mount in the same tick they each see an empty cache and each fire their own request. Add an inflight guard to any new singleton hook of this shape. - **For new boot-time settings, prefer adding to `/settings/bootstrap` over creating a new singleton hook.** The frontend's `primeBootstrap()` (in `src/api/bootstrap.ts`) calls `GET /settings/bootstrap` once after login and pushes each value into the corresponding hook's singleton cache via the hook's top-level `invalidate*(value)` export. Every per-hook GET that fires before bootstrap arrives is a wasted round-trip; every value bootstrap doesn't carry is a duplicate fetch. When you add a settings hook, also add a top-level `invalidate*(value)` export, extend `BootstrapResponse` in `bootstrap.ts`, and call your invalidator from `primeBootstrap()`. - All TypeScript interfaces live in `src/types/index.ts`. diff --git a/VERSION b/VERSION index 7cca401c7..7780cec29 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.32.0 +2.32.1 diff --git a/frontend/UI_GUIDELINES.md b/frontend/UI_GUIDELINES.md index a3cf086fe..0497e9f57 100644 --- a/frontend/UI_GUIDELINES.md +++ b/frontend/UI_GUIDELINES.md @@ -227,6 +227,12 @@ Always render status through one of these: Skeleton loaders are not used today — that's a deliberate choice. If introduced, add a section here. +**Control-driven surfaces** (a grid or chart whose data reloads when a picker, tab or search box changes) have three additional rules — see the request-hook conventions in `CLAUDE.md` for the mechanics: + +- **The loading indicator starts when the control changes, not when the request does.** If the input is debounced, the debounce window is part of the loading state. A surface that looks settled while it is still showing the previous query's rows reads as a wrong answer, not a pending one. +- **A failed fetch renders an ``, never an endless spinner.** Keep the previous data behind the alert where there is any; blanking the view loses the user's context for no benefit. +- **Don't blank to a spinner on every picker change when the previous render is still meaningful.** The exception is a view whose labels are drawn from the *current* selection while the body comes from the response — a matrix, a cross-tab — where showing the previous payload under the new headers is worse than a spinner, because nothing on screen signals the mismatch. + ### 3.9 Icons - Use `` — never raw SVG. diff --git a/frontend/src/api/client.test.ts b/frontend/src/api/client.test.ts index 2151f5f98..0d1139074 100644 --- a/frontend/src/api/client.test.ts +++ b/frontend/src/api/client.test.ts @@ -8,6 +8,7 @@ import { isAuthenticated, setAuthenticated, auth, + isAbortError, } from "./client"; // --------------------------------------------------------------------------- @@ -294,3 +295,62 @@ describe("auth helpers", () => { expect(mockFetch.mock.calls[0][0]).toBe("/api/v1/auth/logout"); }); }); + +// --------------------------------------------------------------------------- +// Request cancellation (#882) +// --------------------------------------------------------------------------- + +describe("AbortSignal support", () => { + it("forwards a signal from api.get into fetch", async () => { + mockFetch.mockReturnValueOnce(jsonResponse({ ok: true })); + const ctrl = new AbortController(); + + await api.get("/cards", { signal: ctrl.signal }); + + const init = mockFetch.mock.calls[0][1]; + expect(init.signal).toBe(ctrl.signal); + // The signal must not displace the standard request configuration. + expect(init.credentials).toBe("same-origin"); + expect(init.headers["X-Turbo-EA-Origin"]).toBe("web"); + }); + + it("forwards a signal from api.getRaw into fetch", async () => { + mockFetch.mockReturnValueOnce(jsonResponse({ ok: true })); + const ctrl = new AbortController(); + + await api.getRaw("/cards/export/csv", { signal: ctrl.signal }); + + expect(mockFetch.mock.calls[0][1].signal).toBe(ctrl.signal); + }); + + it("omits the signal when no options are passed", async () => { + mockFetch.mockReturnValueOnce(jsonResponse({ ok: true })); + + await api.get("/cards"); + + expect(mockFetch.mock.calls[0][1].signal).toBeUndefined(); + }); + + it("propagates the abort rejection rather than swallowing it", async () => { + // `request()` must reject so `finally { setLoading(false) }` still runs. + // Only call sites that opted in by passing a signal can ever observe this. + const abortErr = new DOMException("The user aborted a request.", "AbortError"); + mockFetch.mockReturnValueOnce(Promise.reject(abortErr)); + + await expect(api.get("/cards", { signal: new AbortController().signal })).rejects.toBe( + abortErr, + ); + }); + + it("isAbortError recognises an abort and nothing else", () => { + expect(isAbortError(new DOMException("aborted", "AbortError"))).toBe(true); + // Duck-typed on purpose — jsdom/undici and test doubles may not be DOMException. + expect(isAbortError({ name: "AbortError" })).toBe(true); + + expect(isAbortError(new ApiError("boom", 500, null))).toBe(false); + expect(isAbortError(new Error("network"))).toBe(false); + expect(isAbortError("AbortError")).toBe(false); + expect(isAbortError(null)).toBe(false); + expect(isAbortError(undefined)).toBe(false); + }); +}); diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 990c4c721..57e253e2e 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -1,5 +1,28 @@ const BASE = "/api/v1"; +/** + * Options accepted by the read helpers. Deliberately narrower than `RequestInit` + * so call sites can't override method / credentials / headers. + */ +export interface GetOptions { + signal?: AbortSignal; +} + +/** + * True for a fetch aborted via an AbortSignal. A superseded request is not an + * error — never surface one to the user. + * + * Matches on `name` rather than `instanceof DOMException` so it also holds under + * jsdom/undici and for anything a test double throws. + */ +export function isAbortError(err: unknown): boolean { + return ( + typeof err === "object" && + err !== null && + (err as { name?: unknown }).name === "AbortError" + ); +} + /** Error with HTTP status and structured detail from the API. */ export class ApiError extends Error { status: number; @@ -92,8 +115,16 @@ async function requestRaw(path: string, options: RequestInit = {}): Promise(path: string) => request(path), - getRaw: (path: string) => requestRaw(path), + // Passing a `signal` means you own the resulting AbortError rejection. + // Prefer the request hooks in `src/hooks/` (`useApiQuery`, + // `useAbortableEffect`, `useLatestRequest`) — they abort the predecessor, + // discard superseded responses and swallow the abort for you. + // + // Deliberately not offered on the mutating helpers: aborting a POST/PATCH + // does not undo it server-side, so exposing `signal` there would advertise a + // cancellation guarantee we cannot make. + get: (path: string, opts?: GetOptions) => request(path, opts), + getRaw: (path: string, opts?: GetOptions) => requestRaw(path, opts), post: (path: string, body?: unknown) => request(path, { method: "POST", body: JSON.stringify(body) }), patch: (path: string, body?: unknown) => diff --git a/frontend/src/components/CreateCardDialog.tsx b/frontend/src/components/CreateCardDialog.tsx index bdc16f9ef..69f850eb1 100644 --- a/frontend/src/components/CreateCardDialog.tsx +++ b/frontend/src/components/CreateCardDialog.tsx @@ -36,6 +36,8 @@ import { useSubtypeLabel, } from "@/hooks/useResolveLabel"; import { useAiStatus } from "@/hooks/useAiStatus"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; +import { useDebouncedValue } from "@/hooks/useDebouncedValue"; import { api, ApiError } from "@/api/client"; import type { FieldDef, @@ -201,33 +203,41 @@ export default function CreateCardDialog({ } }, [open, initialType]); - // Auto-search EOL when name changes (debounced) - useEffect(() => { - if (!isEolEligible || !name.trim() || name.trim().length < 2) { - setEolSuggestions([]); - setEolAutoSearchDone(false); - return; - } - // Don't auto-search if already linked - if (eolProduct && eolCycle) return; + // Auto-search EOL when name changes (debounced). Cancelling the timer never + // cancelled a dispatched request, so suggestions for a half-typed name could + // land last and replace the ones for the finished name (#882). + const [debouncedName] = useDebouncedValue(name, 600); + useAbortableEffect( + async ({ signal, isCurrent }) => { + const trimmed = debouncedName.trim(); + if (!isEolEligible || !trimmed || trimmed.length < 2) { + setEolSuggestions([]); + setEolAutoSearchDone(false); + setEolSearching(false); + return; + } + // Don't auto-search if already linked + if (eolProduct && eolCycle) return; - const timer = setTimeout(async () => { setEolSearching(true); try { const results = await api.get( - `/eol/products/fuzzy?search=${encodeURIComponent(name.trim())}&limit=5` + `/eol/products/fuzzy?search=${encodeURIComponent(trimmed)}&limit=5`, + { signal }, ); + if (!isCurrent()) return; setEolSuggestions(results); setEolAutoSearchDone(true); } catch { + if (!isCurrent()) return; setEolSuggestions([]); setEolAutoSearchDone(true); } finally { - setEolSearching(false); + if (isCurrent()) setEolSearching(false); } - }, 600); - return () => clearTimeout(timer); - }, [name, isEolEligible, eolProduct, eolCycle]); + }, + [debouncedName, isEolEligible, eolProduct, eolCycle], + ); const setAttr = (key: string, value: unknown) => { setAttributes((prev) => ({ ...prev, [key]: value })); diff --git a/frontend/src/components/EolLinkSection.tsx b/frontend/src/components/EolLinkSection.tsx index b8ac32c8c..3044e690f 100644 --- a/frontend/src/components/EolLinkSection.tsx +++ b/frontend/src/components/EolLinkSection.tsx @@ -24,6 +24,8 @@ import LinearProgress from "@mui/material/LinearProgress"; import MaterialSymbol from "@/components/MaterialSymbol"; import { useSyncedExpanded } from "@/hooks/useSyncedExpanded"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; +import { useDebouncedValue } from "@/hooks/useDebouncedValue"; import { STATUS_COLORS } from "@/theme/tokens"; import type { Card, EolProduct, EolCycle, EolProductMatch } from "@/types"; @@ -129,53 +131,64 @@ function EolPicker({ onSelect, onCancel, initialProduct, resetKey, cardName }: E }, [resetKey]); // eslint-disable-line react-hooks/exhaustive-deps // Auto-fuzzy-search using card name (mirrors CreateCardDialog pattern) - useEffect(() => { - const trimmed = (cardName || "").trim(); - if (!trimmed || trimmed.length < 2 || selectedProduct) { - setEolSuggestions([]); - setEolAutoSearchDone(false); - return; - } - if (productSearch && productSearch !== trimmed) return; - const timer = setTimeout(async () => { + const [debouncedCardName] = useDebouncedValue(cardName, 600); + useAbortableEffect( + async ({ signal, isCurrent }) => { + const trimmed = (debouncedCardName || "").trim(); + if (!trimmed || trimmed.length < 2 || selectedProduct) { + setEolSuggestions([]); + setEolAutoSearchDone(false); + setEolSearching(false); + return; + } + if (productSearch && productSearch !== trimmed) return; setEolSearching(true); try { const res = await api.get( `/eol/products/fuzzy?search=${encodeURIComponent(trimmed)}&limit=5`, + { signal }, ); + if (!isCurrent()) return; setEolSuggestions(res); setEolAutoSearchDone(true); } catch { + if (!isCurrent()) return; setEolSuggestions([]); setEolAutoSearchDone(true); } finally { - setEolSearching(false); + if (isCurrent()) setEolSearching(false); } - }, 600); - return () => clearTimeout(timer); - }, [cardName, productSearch, selectedProduct]); + }, + [debouncedCardName, productSearch, selectedProduct], + ); - // Search products - useEffect(() => { - if (productSearch.length < 2) { - setProductOptions([]); - return; - } - const timer = setTimeout(async () => { + // Search products. The timer was cancelled on each keystroke but a dispatched + // request was not, so a stale response could replace newer options (#882). + const [debouncedProductSearch] = useDebouncedValue(productSearch, 300); + useAbortableEffect( + async ({ signal, isCurrent }) => { + if (debouncedProductSearch.length < 2) { + setProductOptions([]); + setProductLoading(false); + return; + } setProductLoading(true); try { const res = await api.get( - `/eol/products?search=${encodeURIComponent(productSearch)}` + `/eol/products?search=${encodeURIComponent(debouncedProductSearch)}`, + { signal }, ); + if (!isCurrent()) return; setProductOptions(res); } catch { + if (!isCurrent()) return; setProductOptions([]); } finally { - setProductLoading(false); + if (isCurrent()) setProductLoading(false); } - }, 300); - return () => clearTimeout(timer); - }, [productSearch]); + }, + [debouncedProductSearch], + ); // Fetch cycles when product is selected useEffect(() => { diff --git a/frontend/src/features/admin/CalculationsAdmin.tsx b/frontend/src/features/admin/CalculationsAdmin.tsx index c342f8c90..ce4505683 100644 --- a/frontend/src/features/admin/CalculationsAdmin.tsx +++ b/frontend/src/features/admin/CalculationsAdmin.tsx @@ -35,6 +35,8 @@ import Autocomplete from "@mui/material/Autocomplete"; import CodeEditor from "react-simple-code-editor"; import MaterialSymbol from "@/components/MaterialSymbol"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; +import { useDebouncedValue } from "@/hooks/useDebouncedValue"; import { useMetamodel } from "@/hooks/useMetamodel"; import { useTypeLabel, useRelationLabel, useFieldLabel } from "@/hooks/useResolveLabel"; import { useDateFormat } from "@/hooks/useDateFormat"; @@ -976,27 +978,33 @@ function TestDialog({ open, calculation, onClose }: TestDialogProps) { } }, [open]); - // Debounced card search - useEffect(() => { - if (!searchInput || searchInput.length < 2 || !calculation?.target_type_key) { - setOptions([]); - return; - } - setSearchLoading(true); - const timer = setTimeout(async () => { + // Debounced card search. `clearTimeout` cancelled the timer but never the + // request, so a stale response could replace newer options (#882). + const [debouncedSearchInput, searchDebouncePending] = useDebouncedValue(searchInput, 300); + useAbortableEffect( + async ({ signal, isCurrent }) => { + if (!debouncedSearchInput || debouncedSearchInput.length < 2 || !calculation?.target_type_key) { + setOptions([]); + setSearchLoading(false); + return; + } + setSearchLoading(true); try { const res = await api.get<{ items: CardItem[] }>( - `/cards?type=${encodeURIComponent(calculation.target_type_key)}&search=${encodeURIComponent(searchInput)}&page_size=15`, + `/cards?type=${encodeURIComponent(calculation.target_type_key)}&search=${encodeURIComponent(debouncedSearchInput)}&page_size=15`, + { signal }, ); + if (!isCurrent()) return; setOptions(res.items); } catch { + if (!isCurrent()) return; setOptions([]); } finally { - setSearchLoading(false); + if (isCurrent()) setSearchLoading(false); } - }, 300); - return () => clearTimeout(timer); - }, [searchInput, calculation?.target_type_key]); + }, + [debouncedSearchInput, calculation?.target_type_key], + ); const handleTest = async () => { if (!calculation || !selectedCard) return; @@ -1042,7 +1050,7 @@ function TestDialog({ open, calculation, onClose }: TestDialogProps) { filterOptions={(x) => x} getOptionLabel={(opt) => opt.name} isOptionEqualToValue={(opt, val) => opt.id === val.id} - loading={searchLoading} + loading={searchLoading || searchDebouncePending} noOptionsText={searchInput.length < 2 ? t("calculations.typeToSearch") : t("calculations.noCardsFound")} renderOption={({ key, ...optProps }, option) => (
  • @@ -1066,7 +1074,7 @@ function TestDialog({ open, calculation, onClose }: TestDialogProps) { ...params.InputProps, endAdornment: ( <> - {searchLoading ? : null} + {searchLoading || searchDebouncePending ? : null} {params.InputProps.endAdornment} ), diff --git a/frontend/src/features/admin/SurveyBuilder.tsx b/frontend/src/features/admin/SurveyBuilder.tsx index d8e5b68bf..94ee37426 100644 --- a/frontend/src/features/admin/SurveyBuilder.tsx +++ b/frontend/src/features/admin/SurveyBuilder.tsx @@ -28,6 +28,8 @@ import TableRow from "@mui/material/TableRow"; import MaterialSymbol from "@/components/MaterialSymbol"; import { useExtensionFieldTypes } from "@/lib/extensionHost"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; +import { useDebouncedValue } from "@/hooks/useDebouncedValue"; import { useMetamodel } from "@/hooks/useMetamodel"; import { useTypeLabel, @@ -144,43 +146,51 @@ export default function SurveyBuilder() { api.get("/stakeholder-roles").then(setRoles).catch(() => {}); }, []); - // Search cards for related filter - useEffect(() => { - if (!relatedSearch || relatedSearch.length < 2) { - setRelatedOptions([]); - return; - } - const timer = setTimeout(async () => { + // Search cards for related filter. `clearTimeout` only cancelled the timer — + // once a request was dispatched nothing stopped a stale response from + // replacing newer results (#882). + const [debouncedRelatedSearch] = useDebouncedValue(relatedSearch, 300); + useAbortableEffect( + async ({ signal, isCurrent }) => { + if (!debouncedRelatedSearch || debouncedRelatedSearch.length < 2) { + setRelatedOptions([]); + return; + } try { const res = await api.get<{ items: Card[] }>( - `/cards?search=${encodeURIComponent(relatedSearch)}&page_size=20`, + `/cards?search=${encodeURIComponent(debouncedRelatedSearch)}&page_size=20`, + { signal }, ); + if (!isCurrent()) return; setRelatedOptions(res.items); } catch { - // ignore + // ignore — empty option list } - }, 300); - return () => clearTimeout(timer); - }, [relatedSearch]); + }, + [debouncedRelatedSearch], + ); // Search cards for the "specific cards" picker — restricted to the target type - useEffect(() => { - if (!targetTypeKey || !cardSearch || cardSearch.length < 2) { - setCardOptions([]); - return; - } - const timer = setTimeout(async () => { + const [debouncedCardSearch] = useDebouncedValue(cardSearch, 300); + useAbortableEffect( + async ({ signal, isCurrent }) => { + if (!targetTypeKey || !debouncedCardSearch || debouncedCardSearch.length < 2) { + setCardOptions([]); + return; + } try { const res = await api.get<{ items: Card[] }>( - `/cards?type=${encodeURIComponent(targetTypeKey)}&search=${encodeURIComponent(cardSearch)}&page_size=20`, + `/cards?type=${encodeURIComponent(targetTypeKey)}&search=${encodeURIComponent(debouncedCardSearch)}&page_size=20`, + { signal }, ); + if (!isCurrent()) return; setCardOptions(res.items); } catch { - // ignore + // ignore — empty option list } - }, 300); - return () => clearTimeout(timer); - }, [cardSearch, targetTypeKey]); + }, + [debouncedCardSearch, targetTypeKey], + ); // Hydrate selected items so autocomplete chips render names when editing an existing survey useEffect(() => { diff --git a/frontend/src/features/inventory/InventoryPage.test.tsx b/frontend/src/features/inventory/InventoryPage.test.tsx index 06e4585b9..2eb2f0fc3 100644 --- a/frontend/src/features/inventory/InventoryPage.test.tsx +++ b/frontend/src/features/inventory/InventoryPage.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { render, screen, waitFor, within } from "@testing-library/react"; +import { render, screen, waitFor, within, act } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { MemoryRouter } from "react-router"; import InventoryPage from "./InventoryPage"; @@ -28,11 +28,17 @@ vi.mock("ag-grid-react", () => ({ ({ rowData, onSelectionChanged, + loading, }: { rowData: unknown[]; onSelectionChanged?: (event: { api: { getSelectedRows: () => unknown[] } }) => void; + loading?: boolean; }) => ( -
    +
    + } + > + {t("common:errors.occurred")} + + )} + {/* AG Grid */} ("/reports/ppm/group-options").then(setGroupOptions); }, []); - useEffect(() => { - setLoading(true); - Promise.all([ - api.get(`/reports/ppm/gantt?group_by=${groupBy}`), - api.get("/reports/ppm/dashboard"), - ]) - .then(([g, d]) => { + useAbortableEffect( + async ({ signal, isCurrent }) => { + setLoading(true); + try { + const [g, d] = await Promise.all([ + api.get(`/reports/ppm/gantt?group_by=${groupBy}`, { signal }), + api.get("/reports/ppm/dashboard", { signal }), + ]); + if (!isCurrent()) return; setItems(g); setDashboard(d); - }) - .finally(() => setLoading(false)); - }, [groupBy]); + } finally { + // Only the winner owns the spinner — changing grouping twice quickly + // used to let the first response settle the UI (#882). + if (isCurrent()) setLoading(false); + } + }, + [groupBy], + ); const typeConfig = getType("Initiative"); diff --git a/frontend/src/features/reports/CapabilityMapReport.tsx b/frontend/src/features/reports/CapabilityMapReport.tsx index 76acb6fb3..3b6b975bc 100644 --- a/frontend/src/features/reports/CapabilityMapReport.tsx +++ b/frontend/src/features/reports/CapabilityMapReport.tsx @@ -23,6 +23,7 @@ import type { TagGroup } from "@/types"; import MaterialSymbol from "@/components/MaterialSymbol"; import CardDetailSidePanel from "@/components/CardDetailSidePanel"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; import { readableTextColor } from "@/lib/color"; import { CARD_TYPE_COLORS } from "@/theme"; import { useCurrency } from "@/hooks/useCurrency"; @@ -728,23 +729,25 @@ export default function CapabilityMapReport() { return keys; }, [fieldsSchema]); - useEffect(() => { - api - .get<{ + // Four setStates, three of them conditional — `useAbortableEffect` rather + // than `useApiQuery`, so switching metric can't let the previous metric's + // response land last (#882). + useAbortableEffect( + async ({ signal, isCurrent }) => { + const r = await api.get<{ items: CapItem[]; filterable_types?: Record; fields_schema?: SectionDef[]; tag_groups?: TagGroupDef[]; - }>( - `/reports/capability-heatmap?metric=${metric}`, - ) - .then((r) => { - setData(r.items); - if (r.filterable_types) setFilterableTypes(r.filterable_types); - if (r.fields_schema) setFieldsSchema(r.fields_schema); - if (r.tag_groups) setTagGroupsData(r.tag_groups); - }); - }, [metric]); + }>(`/reports/capability-heatmap?metric=${metric}`, { signal }); + if (!isCurrent()) return; + setData(r.items); + if (r.filterable_types) setFilterableTypes(r.filterable_types); + if (r.fields_schema) setFieldsSchema(r.fields_schema); + if (r.tag_groups) setTagGroupsData(r.tag_groups); + }, + [metric], + ); // Compute date range from all app lifecycle dates const { dateRange, yearMarks } = useMemo(() => { diff --git a/frontend/src/features/reports/CostReport.tsx b/frontend/src/features/reports/CostReport.tsx index 3188cf065..f16eb03fa 100644 --- a/frontend/src/features/reports/CostReport.tsx +++ b/frontend/src/features/reports/CostReport.tsx @@ -31,6 +31,7 @@ import { useTypeLabel, useFieldLabel, useOptionLabel } from "@/hooks/useResolveL import CardDetailSidePanel from "@/components/CardDetailSidePanel"; import MaterialSymbol from "@/components/MaterialSymbol"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; import type { CardType, FieldDef, RelationType } from "@/types"; interface CostItem { @@ -331,40 +332,54 @@ export default function CostReport() { // cards linked to the parent of the deepest frame. const drillFrame = drillStack.length > 0 ? drillStack[drillStack.length - 1] : null; - useEffect(() => { - if (!canViewCostsGlobally) { - setRawItems([]); - setDrillPanels(null); - return; - } - if (drillFrame) { - // One round-trip per source so the panels render independently. - const sources = drillFrame.sources; - const parentId = drillFrame.cardId; - Promise.all(sources.map((s) => { - const p = new URLSearchParams({ - type: s.typeKey, - cost_field: s.fieldKey, - parent_card_id: parentId, - }); - return api.get<{ items: CostItem[]; total: number }>(`/reports/cost-treemap?${p}`); - })).then((rs) => { + // Two branches writing two different states, one of them a fan-out — drilling + // in and back out quickly used to let the slower branch land last and show + // the wrong panels (#882). The same signal aborts every leg of the fan-out. + useAbortableEffect( + async ({ signal, isCurrent }) => { + if (!canViewCostsGlobally) { + setRawItems([]); + setDrillPanels(null); + return; + } + if (drillFrame) { + // One round-trip per source so the panels render independently. + const sources = drillFrame.sources; + const parentId = drillFrame.cardId; + const rs = await Promise.all( + sources.map((s) => { + const p = new URLSearchParams({ + type: s.typeKey, + cost_field: s.fieldKey, + parent_card_id: parentId, + }); + return api.get<{ items: CostItem[]; total: number }>( + `/reports/cost-treemap?${p}`, + { signal }, + ); + }), + ); + if (!isCurrent()) return; setDrillPanels(rs.map((r, i) => ({ source: sources[i], items: r.items }))); setRawItems(null); - }); - } else { - const p = new URLSearchParams({ type: cardTypeKey }); - if (activeAggregates.length > 0) { - for (const a of activeAggregates) p.append("aggregate", a.value); } else { - p.set("cost_field", costField); - } - api.get<{ items: CostItem[]; total: number }>(`/reports/cost-treemap?${p}`).then((r) => { + const p = new URLSearchParams({ type: cardTypeKey }); + if (activeAggregates.length > 0) { + for (const a of activeAggregates) p.append("aggregate", a.value); + } else { + p.set("cost_field", costField); + } + const r = await api.get<{ items: CostItem[]; total: number }>( + `/reports/cost-treemap?${p}`, + { signal }, + ); + if (!isCurrent()) return; setRawItems(r.items); setDrillPanels(null); - }); - } - }, [cardTypeKey, costField, activeAggregates, drillFrame, canViewCostsGlobally]); + } + }, + [cardTypeKey, costField, activeAggregates, drillFrame, canViewCostsGlobally], + ); // Unify root and drilled data into a list of panels: depth-0 has one // anonymous panel; drilled levels have one labelled panel per source. diff --git a/frontend/src/features/reports/DependencyReport.tsx b/frontend/src/features/reports/DependencyReport.tsx index 175471ead..31ea78c79 100644 --- a/frontend/src/features/reports/DependencyReport.tsx +++ b/frontend/src/features/reports/DependencyReport.tsx @@ -32,6 +32,7 @@ import { useThumbnailCapture } from "@/hooks/useThumbnailCapture"; import { useTypeLabel, typeLabel as resolveTypeLabel } from "@/hooks/useResolveLabel"; import CardDetailSidePanel from "@/components/CardDetailSidePanel"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; import type { CardType } from "@/types"; /* ------------------------------------------------------------------ */ @@ -459,18 +460,27 @@ export default function DependencyReport() { }, [saved]); // eslint-disable-line react-hooks/exhaustive-deps // Fetch data — in LDV mode skip type filter to preserve cross-layer edges - useEffect(() => { - setLoading(true); - const p = new URLSearchParams(); - if (cardTypeKey && chartMode !== "c4") p.set("type", cardTypeKey); - api - .get<{ nodes: GNode[]; edges: GEdge[] }>(`/reports/dependencies?${p}`) - .then((r) => { + useAbortableEffect( + async ({ signal, isCurrent }) => { + setLoading(true); + const p = new URLSearchParams(); + if (cardTypeKey && chartMode !== "c4") p.set("type", cardTypeKey); + try { + const r = await api.get<{ nodes: GNode[]; edges: GEdge[] }>( + `/reports/dependencies?${p}`, + { signal }, + ); + if (!isCurrent()) return; setNodes(r.nodes); setEdges(r.edges); - setLoading(false); - }); - }, [cardTypeKey, chartMode]); + } finally { + // Previously only cleared inside `.then`, so a failed request span + // forever; and only the winner may clear it (#882). + if (isCurrent()) setLoading(false); + } + }, + [cardTypeKey, chartMode], + ); // Adjacency map const adjMap = useMemo(() => { diff --git a/frontend/src/features/reports/LifecycleReport.test.tsx b/frontend/src/features/reports/LifecycleReport.test.tsx index 6342c837b..6c92262b2 100644 --- a/frontend/src/features/reports/LifecycleReport.test.tsx +++ b/frontend/src/features/reports/LifecycleReport.test.tsx @@ -190,7 +190,10 @@ describe("LifecycleReport", () => { renderLifecycle(); await waitFor(() => { - expect(api.get).toHaveBeenCalledWith("/reports/roadmap"); + expect(api.get).toHaveBeenCalledWith( + "/reports/roadmap", + expect.objectContaining({ signal: expect.any(AbortSignal) }), + ); }); }); @@ -247,7 +250,10 @@ describe("LifecycleReport", () => { // Initially fetches without type await waitFor(() => { - expect(api.get).toHaveBeenCalledWith("/reports/roadmap"); + expect(api.get).toHaveBeenCalledWith( + "/reports/roadmap", + expect.objectContaining({ signal: expect.any(AbortSignal) }), + ); }); }); }); diff --git a/frontend/src/features/reports/LifecycleReport.tsx b/frontend/src/features/reports/LifecycleReport.tsx index 549aa636f..c7ba41552 100644 --- a/frontend/src/features/reports/LifecycleReport.tsx +++ b/frontend/src/features/reports/LifecycleReport.tsx @@ -27,6 +27,7 @@ import { useSavedReport } from "@/hooks/useSavedReport"; import { useThumbnailCapture } from "@/hooks/useThumbnailCapture"; import { useTypeLabel, useFieldLabel, useOptionLabel } from "@/hooks/useResolveLabel"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; /* ------------------------------------------------------------------ */ /* Types */ @@ -206,10 +207,17 @@ export default function LifecycleReport() { } }, [hasDateFields]); - useEffect(() => { - const params = cardTypeKey ? `?type=${cardTypeKey}` : ""; - api.get<{ items: RoadmapItem[] }>(`/reports/roadmap${params}`).then((r) => setData(r.items)); - }, [cardTypeKey]); + // Switching card type must not let the previous type's response land last + // and repopulate the timeline (#882). + useAbortableEffect( + async ({ signal, isCurrent }) => { + const params = cardTypeKey ? `?type=${cardTypeKey}` : ""; + const r = await api.get<{ items: RoadmapItem[] }>(`/reports/roadmap${params}`, { signal }); + if (!isCurrent()) return; + setData(r.items); + }, + [cardTypeKey], + ); const { items, totalMin, totalRange, viewMin, contentPct, todayPct, eolCount } = useMemo(() => { if (!data || !data.length) return { items: [], totalMin: 0, totalRange: 1, viewMin: 0, contentPct: 100, todayPct: 50, eolCount: 0 }; diff --git a/frontend/src/features/reports/MatrixReport.tsx b/frontend/src/features/reports/MatrixReport.tsx index e1851c35d..b7c8c8d4e 100644 --- a/frontend/src/features/reports/MatrixReport.tsx +++ b/frontend/src/features/reports/MatrixReport.tsx @@ -23,8 +23,8 @@ import { useMetamodel } from "@/hooks/useMetamodel"; import { useSavedReport } from "@/hooks/useSavedReport"; import { useThumbnailCapture } from "@/hooks/useThumbnailCapture"; import { useTypeLabel } from "@/hooks/useResolveLabel"; +import { useApiQuery } from "@/hooks/useApiQuery"; import CardDetailSidePanel from "@/components/CardDetailSidePanel"; -import { api } from "@/api/client"; import { type MatrixItem, type TreeNode, @@ -128,7 +128,6 @@ export default function MatrixReport() { const { chartRef, thumbnail, captureAndSave } = useThumbnailCapture(() => saved.setSaveDialogOpen(true)); const [rowType, setRowType] = useState("Application"); const [colType, setColType] = useState("BusinessCapability"); - const [data, setData] = useState(null); const [sidePanelCardId, setSidePanelCardId] = useState(null); const [cellMode, setCellMode] = useState("exists"); const [hideEmpty, setHideEmpty] = useState(false); @@ -214,9 +213,16 @@ export default function MatrixReport() { setColExpandedDepth(Infinity); }, [saved]); // eslint-disable-line react-hooks/exhaustive-deps - useEffect(() => { - api.get(`/reports/matrix?row_type=${rowType}&col_type=${colType}`).then(setData); - }, [rowType, colType]); + // The axis labels below render from `rowType`/`colType`, so data for the + // previous axes must never survive a switch — it would draw a complete, + // convincing matrix under the wrong headers with nothing to signal it. + // `keepPreviousData: false` falls back to the existing spinner instead, and + // the hook discards a superseded response outright (#882). + const { data: matrixData, loading: matrixLoading } = useApiQuery( + `/reports/matrix?row_type=${rowType}&col_type=${colType}`, + { keepPreviousData: false }, + ); + const data = matrixData ?? null; // Reset depth when switching types useEffect(() => { @@ -467,7 +473,7 @@ export default function MatrixReport() { return params; }, [rowLabel, colLabel, cellMode, sortRows, sortCols, hideEmpty, t]); - if (ml || data === null) + if (ml || matrixLoading || data === null) return ; const isHierarchyRowMode = sortRows === "hierarchy" && rowHasHierarchy && rowTreeFull !== null && rowTreeFull.maxDepth > 0; diff --git a/frontend/src/features/reports/PortfolioReport.test.tsx b/frontend/src/features/reports/PortfolioReport.test.tsx index e49cbbd73..2e8e82b32 100644 --- a/frontend/src/features/reports/PortfolioReport.test.tsx +++ b/frontend/src/features/reports/PortfolioReport.test.tsx @@ -14,9 +14,10 @@ vi.mock("react-router", async () => { return { ...actual, useNavigate: () => mockNavigate }; }); -vi.mock("@/api/client", () => ({ - api: { get: vi.fn(), post: vi.fn() }, -})); +vi.mock("@/api/client", async () => { + const actual = await vi.importActual("@/api/client"); + return { ...actual, api: { get: vi.fn(), post: vi.fn() } }; +}); vi.mock("@/hooks/useMetamodel", () => ({ useMetamodel: vi.fn(), @@ -192,8 +193,23 @@ describe("PortfolioReport", () => { renderPortfolio(); await waitFor(() => { - expect(api.get).toHaveBeenCalledWith("/reports/app-portfolio?type=Application"); + expect(api.get).toHaveBeenCalledWith( + "/reports/app-portfolio?type=Application", + expect.objectContaining({ signal: expect.any(AbortSignal) }), + ); + }); + }); + + it("shows an error instead of spinning forever when the fetch fails", async () => { + // The report cleared `data` on every type switch and never caught a + // failure, so a failed request left it on the spinner permanently. + vi.mocked(api.get).mockRejectedValueOnce(new Error("boom")); + renderPortfolio(); + + await waitFor(() => { + expect(screen.getByText("An error occurred")).toBeInTheDocument(); }); + expect(document.querySelector(".MuiCircularProgress-root")).toBeNull(); }); it("renders app chips in chart view", async () => { diff --git a/frontend/src/features/reports/PortfolioReport.tsx b/frontend/src/features/reports/PortfolioReport.tsx index a38954912..930b2ddd8 100644 --- a/frontend/src/features/reports/PortfolioReport.tsx +++ b/frontend/src/features/reports/PortfolioReport.tsx @@ -34,10 +34,11 @@ import TagPicker from "@/components/TagPicker"; import type { TagGroup } from "@/types"; import MaterialSymbol from "@/components/MaterialSymbol"; import CardDetailSidePanel from "@/components/CardDetailSidePanel"; -import { api } from "@/api/client"; +import { api, isAbortError } from "@/api/client"; import { readableTextColor } from "@/lib/color"; import { useMetamodel } from "@/hooks/useMetamodel"; import { useSavedReport } from "@/hooks/useSavedReport"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; import { useThumbnailCapture } from "@/hooks/useThumbnailCapture"; import { useTimeline } from "@/hooks/useTimeline"; import { @@ -562,6 +563,7 @@ export default function PortfolioReport({ // gate the defaults- and persist-effects so they don't fire while data is // stale (between picking a new type and the fresh fetch resolving). const [dataCardType, setDataCardType] = useState(null); + const [loadFailed, setLoadFailed] = useState(false); const [drawer, setDrawer] = useState(null); const [sidePanelCardId, setSidePanelCardId] = useState(null); const [view, setView] = useState<"chart" | "table">("chart"); @@ -673,24 +675,33 @@ export default function PortfolioReport({ }, [saved, initialCardType]); // eslint-disable-line react-hooks/exhaustive-deps // Fetch data — refetches whenever the user picks a different card type. - useEffect(() => { - let cancelled = false; - const fetchedFor = cardType; - setData(null); - setDataCardType(null); - setAiInsights(null); - setAiOpen(false); - api - .get(`/reports/app-portfolio?type=${encodeURIComponent(cardType)}`) - .then((r) => { - if (cancelled) return; + // A superseded request is aborted and its response discarded, so switching + // types quickly can't leave the chart showing the previous type (#882). + useAbortableEffect( + async ({ signal, isCurrent }) => { + const fetchedFor = cardType; + setData(null); + setDataCardType(null); + setLoadFailed(false); + setAiInsights(null); + setAiOpen(false); + try { + const r = await api.get( + `/reports/app-portfolio?type=${encodeURIComponent(cardType)}`, + { signal }, + ); + if (!isCurrent()) return; setData(r); setDataCardType(fetchedFor); - }); - return () => { - cancelled = true; - }; - }, [cardType]); + } catch (err) { + if (!isCurrent() || isAbortError(err)) return; + // Without this the report spun forever: `data` stayed null and nothing + // ever reset it. + setLoadFailed(true); + } + }, + [cardType], + ); // Switching card types invalidates field/relation/tag selections because // the keys they reference don't exist on the new type. Clear them so the @@ -1243,6 +1254,13 @@ export default function PortfolioReport({ return params; }, [groupByLabel, nestedActive, depthLabel, colorBy, colorByLabel, search, tl.printParam, view, activeFilterCount, t]); + if (loadFailed) + return ( + + {t("common:errors.occurred")} + + ); + if (!data) return ( diff --git a/frontend/src/features/surveys/SurveyRespond.tsx b/frontend/src/features/surveys/SurveyRespond.tsx index 14f20a747..a4257784e 100644 --- a/frontend/src/features/surveys/SurveyRespond.tsx +++ b/frontend/src/features/surveys/SurveyRespond.tsx @@ -29,6 +29,8 @@ import Tooltip from "@mui/material/Tooltip"; import Divider from "@mui/material/Divider"; import MaterialSymbol from "@/components/MaterialSymbol"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; +import { useDebouncedValue } from "@/hooks/useDebouncedValue"; import { ExtensionBoundary, useExtensionFieldTypes } from "@/lib/extensionHost"; import { FieldHelp } from "@/features/cards/sections/cardDetailUtils"; import type { SurveyRespondForm, SurveyField } from "@/types"; @@ -107,25 +109,33 @@ function RelationFieldEditor({ // Load candidates as soon as the dropdown opens (empty query returns the // first cards of the related type) and refine as the user types — users // can't be expected to know linkable card names by heart. - useEffect(() => { - if (!relatedTypeKey || !open) { - return; - } - setLoading(true); - const timer = setTimeout(async () => { + // `setLoading(true)` used to run outside the timer while `setLoading(false)` + // ran inside it, so a cancelled timer left the spinner on forever. The + // debounce window is now part of the loading state, and a superseded + // response can no longer replace newer options (#882). + const [debouncedSearch, searchPending] = useDebouncedValue(search, 250); + useAbortableEffect( + async ({ signal, isCurrent }) => { + if (!relatedTypeKey || !open) { + setLoading(false); + return; + } + setLoading(true); try { const res = await api.get<{ items: RelationRef[] }>( - `/cards?type=${encodeURIComponent(relatedTypeKey)}&search=${encodeURIComponent(search)}&page_size=20`, + `/cards?type=${encodeURIComponent(relatedTypeKey)}&search=${encodeURIComponent(debouncedSearch)}&page_size=20`, + { signal }, ); + if (!isCurrent()) return; setOptions(res.items.map((c) => ({ id: c.id, name: c.name }))); } catch { // ignore — empty option list } finally { - setLoading(false); + if (isCurrent()) setLoading(false); } - }, 250); - return () => clearTimeout(timer); - }, [search, relatedTypeKey, open]); + }, + [debouncedSearch, relatedTypeKey, open], + ); const merged = useMemo(() => { const ids = new Set(options.map((o) => o.id)); @@ -139,7 +149,7 @@ function RelationFieldEditor({ open={open} onOpen={() => setOpen(true)} onClose={() => setOpen(false)} - loading={loading} + loading={loading || searchPending} options={merged} getOptionLabel={(o) => o.name} isOptionEqualToValue={(opt, val) => opt.id === val.id} @@ -169,7 +179,7 @@ function RelationFieldEditor({ ...params.InputProps, endAdornment: ( <> - {loading ? : null} + {loading || searchPending ? : null} {params.InputProps.endAdornment} ), diff --git a/frontend/src/features/todos/TodosPage.tsx b/frontend/src/features/todos/TodosPage.tsx index 82e4a2a4b..1f28ef1e9 100644 --- a/frontend/src/features/todos/TodosPage.tsx +++ b/frontend/src/features/todos/TodosPage.tsx @@ -20,6 +20,7 @@ import Alert from "@mui/material/Alert"; import CircularProgress from "@mui/material/CircularProgress"; import MaterialSymbol from "@/components/MaterialSymbol"; import { api } from "@/api/client"; +import { useAbortableEffect } from "@/hooks/useLatestRequest"; import { useDateFormat } from "@/hooks/useDateFormat"; import { formatRecurrence } from "@/lib/recurrence/recurrenceLabel"; import type { RecurrenceUnit, Todo, MySurveyItem } from "@/types"; @@ -60,15 +61,22 @@ function TodosPanel() { const currentStatus = tab === 0 ? assignedStatus : createdStatus; const setCurrentStatus = tab === 0 ? setAssignedStatus : setCreatedStatus; - useEffect(() => { - const scope = tab === 0 ? "assigned_only=true" : "created_only=true"; - // "upcoming" maps to the dormant scheduled state; "all" omits the filter. - const statusParam = - currentStatus === "all" - ? "" - : `&status=${currentStatus === "upcoming" ? "scheduled" : currentStatus}`; - api.get(`/todos?${scope}${statusParam}`).then(setTodos); - }, [tab, currentStatus]); + // Flipping tab or status filter quickly must not let the earlier request + // land last and show the wrong list (#882). + useAbortableEffect( + async ({ signal, isCurrent }) => { + const scope = tab === 0 ? "assigned_only=true" : "created_only=true"; + // "upcoming" maps to the dormant scheduled state; "all" omits the filter. + const statusParam = + currentStatus === "all" + ? "" + : `&status=${currentStatus === "upcoming" ? "scheduled" : currentStatus}`; + const res = await api.get(`/todos?${scope}${statusParam}`, { signal }); + if (!isCurrent()) return; + setTodos(res); + }, + [tab, currentStatus], + ); const sortedTodos = useMemo(() => [...todos].sort(compareByDueDateAsc), [todos]); const showAssignee = tab === 1; diff --git a/frontend/src/hooks/useApiQuery.test.ts b/frontend/src/hooks/useApiQuery.test.ts new file mode 100644 index 000000000..95a3ecb33 --- /dev/null +++ b/frontend/src/hooks/useApiQuery.test.ts @@ -0,0 +1,164 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { renderHook, act, waitFor } from "@testing-library/react"; +import { api } from "@/api/client"; +import { useApiQuery } from "./useApiQuery"; + +vi.mock("@/api/client", async () => { + const actual = await vi.importActual("@/api/client"); + return { ...actual, api: { get: vi.fn() } }; +}); + +function deferred() { + let resolve!: (v: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +beforeEach(() => vi.mocked(api.get).mockReset()); + +describe("useApiQuery", () => { + it("fetches and exposes the payload", async () => { + vi.mocked(api.get).mockResolvedValue({ items: [1, 2] }); + + const { result } = renderHook(() => useApiQuery<{ items: number[] }>("/reports/matrix")); + + expect(result.current.loading).toBe(true); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.data).toEqual({ items: [1, 2] }); + expect(result.current.error).toBeNull(); + }); + + it("stays idle and cancels when the path is null", async () => { + const { result } = renderHook(() => useApiQuery(null)); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(api.get).not.toHaveBeenCalled(); + expect(result.current.data).toBeUndefined(); + }); + + it("discards a superseded response that resolves last", async () => { + // The #882 scenario: the request for the previous selection must never + // overwrite the one for the current selection. + const slow = deferred<{ id: string }>(); + vi.mocked(api.get).mockImplementationOnce(() => slow.promise as Promise); + vi.mocked(api.get).mockResolvedValueOnce({ id: "fresh" }); + + const { result, rerender } = renderHook(({ p }) => useApiQuery<{ id: string }>(p), { + initialProps: { p: "/reports/matrix?row_type=A" }, + }); + + rerender({ p: "/reports/matrix?row_type=B" }); + await waitFor(() => expect(result.current.data).toEqual({ id: "fresh" })); + + await act(async () => { + slow.resolve({ id: "stale" }); + await slow.promise; + }); + + expect(result.current.data).toEqual({ id: "fresh" }); + }); + + it("keeps the loading flag on until the winning request settles", async () => { + const slow = deferred<{ id: string }>(); + const second = deferred<{ id: string }>(); + vi.mocked(api.get).mockImplementationOnce(() => slow.promise as Promise); + vi.mocked(api.get).mockImplementationOnce(() => second.promise as Promise); + + const { result, rerender } = renderHook(({ p }) => useApiQuery<{ id: string }>(p), { + initialProps: { p: "/a" }, + }); + rerender({ p: "/b" }); + + // The superseded request settling must not clear the spinner. + await act(async () => { + slow.resolve({ id: "stale" }); + await slow.promise; + }); + expect(result.current.loading).toBe(true); + + await act(async () => { + second.resolve({ id: "fresh" }); + await second.promise; + }); + await waitFor(() => expect(result.current.loading).toBe(false)); + }); + + it("surfaces a real error and clears loading", async () => { + // `...Once` on purpose: a persistently-rejecting mock leaves an unconsumed + // rejected promise behind, which Vitest 4 reports as a test failure. + vi.mocked(api.get).mockRejectedValueOnce(new Error("boom")).mockResolvedValue({}); + + const { result } = renderHook(() => useApiQuery("/x")); + + await waitFor(() => expect(result.current.error?.message).toBe("boom")); + expect(result.current.loading).toBe(false); + }); + + it("does not refetch when only the select identity changes", async () => { + // CostReport-style call sites pass a fresh arrow every render; that must + // not become a fetch trigger. + vi.mocked(api.get).mockResolvedValue({ n: 1 }); + + const { rerender } = renderHook(() => + useApiQuery<{ n: number }, number>("/x", { select: (r) => r.n }), + ); + await waitFor(() => expect(api.get).toHaveBeenCalledTimes(1)); + + rerender(); + rerender(); + + expect(api.get).toHaveBeenCalledTimes(1); + }); + + it("applies select to the payload", async () => { + vi.mocked(api.get).mockResolvedValue({ n: 7 }); + + const { result } = renderHook(() => + useApiQuery<{ n: number }, number>("/x", { select: (r) => r.n }), + ); + + await waitFor(() => expect(result.current.data).toBe(7)); + }); + + it("keeps previous data across a path change by default", async () => { + const next = deferred<{ id: string }>(); + vi.mocked(api.get).mockResolvedValueOnce({ id: "first" }); + vi.mocked(api.get).mockImplementationOnce(() => next.promise as Promise); + + const { result, rerender } = renderHook(({ p }) => useApiQuery<{ id: string }>(p), { + initialProps: { p: "/a" }, + }); + await waitFor(() => expect(result.current.data).toEqual({ id: "first" })); + + rerender({ p: "/b" }); + // Still rendering the previous payload rather than blanking to a spinner. + expect(result.current.data).toEqual({ id: "first" }); + + await act(async () => { + next.resolve({ id: "second" }); + await next.promise; + }); + expect(result.current.data).toEqual({ id: "second" }); + }); + + it("refetch() re-issues the same path", async () => { + vi.mocked(api.get).mockResolvedValue({ n: 1 }); + const { result } = renderHook(() => useApiQuery<{ n: number }>("/x")); + await waitFor(() => expect(api.get).toHaveBeenCalledTimes(1)); + + act(() => result.current.refetch()); + + await waitFor(() => expect(api.get).toHaveBeenCalledTimes(2)); + }); + + it("forwards an abort signal to the client", async () => { + vi.mocked(api.get).mockResolvedValue({}); + renderHook(() => useApiQuery("/x")); + + await waitFor(() => expect(api.get).toHaveBeenCalled()); + const opts = vi.mocked(api.get).mock.calls[0][1]; + expect(opts?.signal).toBeInstanceOf(AbortSignal); + }); +}); diff --git a/frontend/src/hooks/useApiQuery.ts b/frontend/src/hooks/useApiQuery.ts new file mode 100644 index 000000000..ce4225f8d --- /dev/null +++ b/frontend/src/hooks/useApiQuery.ts @@ -0,0 +1,87 @@ +import { useCallback, useEffect, useRef, useState } from "react"; +import { api, isAbortError } from "@/api/client"; +import { useLatestRequest } from "./useLatestRequest"; + +export interface ApiQueryOptions { + /** + * Map the payload before it lands in state. Held in a ref, so passing a fresh + * arrow function on every render never triggers a refetch. + */ + select?: (raw: T) => S; + /** + * Keep the last successful data on screen while a new request is in flight. + * Defaults to true — blanking to a spinner on every picker change is jarring + * and hides a perfectly good previous render. + */ + keepPreviousData?: boolean; + /** Bump to force a refetch while `path` is unchanged (e.g. after a mutation). */ + refetchKey?: string | number; +} + +export interface ApiQueryResult { + data: S | undefined; + loading: boolean; + /** Never an abort — a superseded request is not an error. */ + error: Error | null; + refetch: () => void; +} + +/** + * A `GET` keyed on user-controlled state, with ordering guaranteed: the request + * for the previous value can never overwrite the one for the current value + * (#882). + * + * Pass `null` as the path to stay idle (a closed dropdown, an unselected type); + * that cancels anything in flight and clears the data. + * + * One payload feeding one piece of state is the case this covers. When the + * effect writes several states, or fans out over `Promise.all`, use + * `useAbortableEffect` instead of contorting `select`. + */ +export function useApiQuery( + path: string | null, + opts: ApiQueryOptions = {}, +): ApiQueryResult { + const { keepPreviousData = true, refetchKey } = opts; + const selectRef = useRef(opts.select); + selectRef.current = opts.select; + + const [data, setData] = useState(undefined); + const [loading, setLoading] = useState(path !== null); + const [error, setError] = useState(null); + const [nonce, setNonce] = useState(0); + const { run, cancel } = useLatestRequest(); + + useEffect(() => { + if (path === null) { + cancel(); + setData(undefined); + setLoading(false); + setError(null); + return; + } + void run(async ({ signal, isCurrent }) => { + setLoading(true); + setError(null); + if (!keepPreviousData) setData(undefined); + try { + const raw = await api.get(path, { signal }); + if (!isCurrent()) return; + const select = selectRef.current; + setData(select ? select(raw) : (raw as unknown as S)); + } catch (err) { + if (!isCurrent() || isAbortError(err)) return; + setError(err instanceof Error ? err : new Error(String(err))); + } finally { + // Only the winner may clear the spinner. An aborted predecessor + // rejecting first would otherwise flash "done" over stale data while + // the real request is still in flight. + if (isCurrent()) setLoading(false); + } + }); + }, [path, refetchKey, nonce, keepPreviousData, run, cancel]); + + const refetch = useCallback(() => setNonce((n) => n + 1), []); + + return { data, loading, error, refetch }; +} diff --git a/frontend/src/hooks/useCardSearch.test.ts b/frontend/src/hooks/useCardSearch.test.ts index 676307e14..fac0b1dcd 100644 --- a/frontend/src/hooks/useCardSearch.test.ts +++ b/frontend/src/hooks/useCardSearch.test.ts @@ -132,3 +132,45 @@ describe("useCardSearch", () => { expect(result.current.items.map((c) => c.id)).toEqual(["1", "2", "3"]); }); }); + +describe("useCardSearch superseding", () => { + beforeEach(() => { + vi.mocked(api.get).mockReset(); + }); + + it("refetches when the query changes mid-flight instead of dropping it", async () => { + // The in-flight guard used to drop the new query outright and never retry, + // so the picker kept showing results for the query the user had already + // replaced. This backs CardPicker, VendorField and the diagram card + // sidebar, so it was reachable from nearly every picker in the app. + let releaseFirst: (v: unknown) => void = () => {}; + const first = new Promise((r) => { + releaseFirst = r; + }); + vi.mocked(api.get) + .mockImplementationOnce(() => first as Promise) + .mockResolvedValue(page([{ id: "b", name: "Beta", type: "Application" }], 1)); + + const { result, rerender } = renderHook( + ({ search }) => useCardSearch({ types: ["Application"], search, enabled: true }), + { initialProps: { search: "al" } }, + ); + await waitFor(() => expect(api.get).toHaveBeenCalledTimes(1)); + + rerender({ search: "beta" }); + + // The second query must actually be issued, not swallowed. + await waitFor(() => expect(api.get).toHaveBeenCalledTimes(2)); + expect(vi.mocked(api.get).mock.calls[1][0]).toContain("search=beta"); + await waitFor(() => expect(result.current.items).toHaveLength(1)); + expect(result.current.items[0].name).toBe("Beta"); + + // The superseded response landing last must not replace the fresh results. + await act(async () => { + releaseFirst(page([{ id: "a", name: "Alpha", type: "Application" }], 1)); + await first; + }); + expect(result.current.items[0].name).toBe("Beta"); + expect(result.current.loading).toBe(false); + }); +}); diff --git a/frontend/src/hooks/useCardSearch.ts b/frontend/src/hooks/useCardSearch.ts index 07f5b2101..215733214 100644 --- a/frontend/src/hooks/useCardSearch.ts +++ b/frontend/src/hooks/useCardSearch.ts @@ -51,7 +51,12 @@ export function useCardSearch({ types, search, enabled, pageSize = 1000 }: Optio const fetchPage = useCallback( async (pageNum: number, append: boolean) => { - if (inflight.current) return; + // Only a duplicate scroll-sentinel `loadMore` may be dropped. Dropping a + // *filter* change was a bug: the request was discarded and never retried, + // so the picker kept showing results for the query the user had already + // replaced. A fresh page-1 request supersedes instead — the token check + // below discards whichever response loses. + if (append && inflight.current) return; inflight.current = true; const token = ++requestToken.current; setLoading(true); @@ -81,8 +86,12 @@ export function useCardSearch({ types, search, enabled, pageSize = 1000 }: Optio setTotal(0); } } finally { - if (token === requestToken.current) setLoading(false); - inflight.current = false; + // Only the winner owns the flags — a superseded request settling first + // must not report "idle" while the current one is still running. + if (token === requestToken.current) { + setLoading(false); + inflight.current = false; + } } }, [typeKey, trimmedSearch, pageSize], diff --git a/frontend/src/hooks/useDebouncedValue.test.ts b/frontend/src/hooks/useDebouncedValue.test.ts new file mode 100644 index 000000000..b52409b6a --- /dev/null +++ b/frontend/src/hooks/useDebouncedValue.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { useDebouncedValue } from "./useDebouncedValue"; + +beforeEach(() => vi.useFakeTimers()); +afterEach(() => vi.useRealTimers()); + +describe("useDebouncedValue", () => { + it("does not debounce the initial value", () => { + const { result } = renderHook(() => useDebouncedValue("SAP", 300)); + expect(result.current[0]).toBe("SAP"); + expect(result.current[1]).toBe(false); + }); + + it("settles rapid changes into a single value", () => { + const { result, rerender } = renderHook(({ v }) => useDebouncedValue(v, 300), { + initialProps: { v: "" }, + }); + + for (const v of ["S", "SA", "SAP", "SAP ", "SAP E", "SAP ER", "SAP ERP"]) { + rerender({ v }); + } + + // Still showing the old value, and flagged as pending. + expect(result.current[0]).toBe(""); + expect(result.current[1]).toBe(true); + + act(() => vi.advanceTimersByTime(300)); + + expect(result.current[0]).toBe("SAP ERP"); + expect(result.current[1]).toBe(false); + }); + + it("flags pending from the very first change", () => { + // This is what keeps a grid's loading overlay on from the first keystroke, + // before any request exists. + const { result, rerender } = renderHook(({ v }) => useDebouncedValue(v, 300), { + initialProps: { v: "" }, + }); + + rerender({ v: "S" }); + expect(result.current[1]).toBe(true); + + act(() => vi.advanceTimersByTime(299)); + expect(result.current[1]).toBe(true); + + act(() => vi.advanceTimersByTime(1)); + expect(result.current[1]).toBe(false); + }); + + it("clears pending when the value returns to where it started", () => { + const { result, rerender } = renderHook(({ v }) => useDebouncedValue(v, 300), { + initialProps: { v: "" }, + }); + + rerender({ v: "S" }); + expect(result.current[1]).toBe(true); + rerender({ v: "" }); + + expect(result.current[0]).toBe(""); + expect(result.current[1]).toBe(false); + }); + + it("passes through synchronously when the delay is zero", () => { + const { result, rerender } = renderHook(({ v }) => useDebouncedValue(v, 0), { + initialProps: { v: "a" }, + }); + + rerender({ v: "b" }); + + expect(result.current[0]).toBe("b"); + expect(result.current[1]).toBe(false); + }); +}); diff --git a/frontend/src/hooks/useDebouncedValue.ts b/frontend/src/hooks/useDebouncedValue.ts new file mode 100644 index 000000000..4a0ed4c18 --- /dev/null +++ b/frontend/src/hooks/useDebouncedValue.ts @@ -0,0 +1,48 @@ +import { useEffect, useRef, useState } from "react"; + +/** + * Settles a rapidly-changing value before anything expensive keys off it — + * typically a search box, so typing "SAP ERP" issues one request instead of + * seven (#882). + * + * Returns `[debounced, pending]`. `pending` goes true on the FIRST change and + * stays true until the value settles, so callers can OR it into a loading flag: + * a grid must never look finished while it is still showing rows for the + * previous query. + * + * There is no debounce on mount, and `delayMs <= 0` passes through + * synchronously. + */ +export function useDebouncedValue(value: T, delayMs: number): [T, boolean] { + const [debounced, setDebounced] = useState(value); + const [pending, setPending] = useState(false); + const first = useRef(true); + + useEffect(() => { + if (first.current) { + first.current = false; + return; + } + // Typed and deleted back to where we started — nothing to wait for. + if (Object.is(value, debounced)) { + setPending(false); + return; + } + if (delayMs <= 0) { + setDebounced(value); + setPending(false); + return; + } + setPending(true); + const timer = setTimeout(() => { + setDebounced(value); + setPending(false); + }, delayMs); + return () => clearTimeout(timer); + // `debounced` is read only for the equality short-circuit above; keying the + // effect on it would re-run on every settle and loop. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [value, delayMs]); + + return [debounced, pending]; +} diff --git a/frontend/src/hooks/useLatestRequest.test.ts b/frontend/src/hooks/useLatestRequest.test.ts new file mode 100644 index 000000000..260c638d9 --- /dev/null +++ b/frontend/src/hooks/useLatestRequest.test.ts @@ -0,0 +1,229 @@ +import { describe, it, expect, vi } from "vitest"; +import { renderHook, act, waitFor } from "@testing-library/react"; +import { useLatestRequest, useAbortableEffect } from "./useLatestRequest"; + +/** A promise plus its resolver, so a test can control completion order. */ +function deferred() { + let resolve!: (v: T) => void; + let reject!: (e: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +describe("useLatestRequest", () => { + it("keeps a stable identity across re-renders", () => { + // Callers put `run` in useCallback dependency arrays — a fresh object each + // render would rebuild their fetch callback and loop the effect forever. + const { result, rerender } = renderHook(() => useLatestRequest()); + const first = result.current; + rerender(); + expect(result.current).toBe(first); + expect(result.current.run).toBe(first.run); + }); + + it("discards the result of a superseded run when it resolves last", async () => { + // The #882 scenario in miniature. + const slow = deferred(); + const fast = deferred(); + const written: string[] = []; + const { result } = renderHook(() => useLatestRequest()); + + let slowSawCurrent = true; + act(() => { + void result.current.run(async ({ isCurrent }) => { + const v = await slow.promise; + slowSawCurrent = isCurrent(); + if (!isCurrent()) return; + written.push(v); + }); + void result.current.run(async ({ isCurrent }) => { + const v = await fast.promise; + if (!isCurrent()) return; + written.push(v); + }); + }); + + await act(async () => { + fast.resolve("fresh"); + await fast.promise; + }); + await act(async () => { + slow.resolve("stale"); + await slow.promise; + }); + + expect(slowSawCurrent).toBe(false); + expect(written).toEqual(["fresh"]); + }); + + it("aborts the predecessor's signal when a newer run starts", async () => { + const { result } = renderHook(() => useLatestRequest()); + let firstSignal: AbortSignal | undefined; + + act(() => { + void result.current.run(async ({ signal }) => { + firstSignal = signal; + await new Promise(() => {}); // never settles + }); + }); + expect(firstSignal?.aborted).toBe(false); + + act(() => { + void result.current.run(async () => {}); + }); + + expect(firstSignal?.aborted).toBe(true); + }); + + it("settles a superseded run instead of hanging", async () => { + // InventoryPage has 13 `await loadData()` call sites; if a superseded run + // never settled they would all hang. + const slow = deferred(); + const { result } = renderHook(() => useLatestRequest()); + let settled = false; + + let first: Promise; + act(() => { + first = result.current.run(async () => { + await slow.promise; + }); + void first.then(() => { + settled = true; + }); + void result.current.run(async () => {}); + }); + + await act(async () => { + slow.resolve(); + await slow.promise; + }); + await waitFor(() => expect(settled).toBe(true)); + }); + + it("swallows an AbortError but rethrows a real error from the current run", async () => { + const { result } = renderHook(() => useLatestRequest()); + + await act(async () => { + await expect( + result.current.run(async () => { + throw new DOMException("aborted", "AbortError"); + }), + ).resolves.toBeUndefined(); + }); + + await act(async () => { + await expect( + result.current.run(async () => { + throw new Error("500"); + }), + ).rejects.toThrow("500"); + }); + }); + + it("swallows a real error thrown by a superseded run", async () => { + const slow = deferred(); + const { result } = renderHook(() => useLatestRequest()); + let rejected = false; + + act(() => { + void result.current + .run(async () => { + await slow.promise; + throw new Error("stale failure"); + }) + .catch(() => { + rejected = true; + }); + void result.current.run(async () => {}); + }); + + await act(async () => { + slow.resolve(); + await slow.promise.catch(() => {}); + }); + + expect(rejected).toBe(false); + }); + + it("invalidates and aborts on unmount", async () => { + const { result, unmount } = renderHook(() => useLatestRequest()); + let signal: AbortSignal | undefined; + let currentAfterUnmount = true; + + act(() => { + void result.current.run(async ({ signal: s, isCurrent }) => { + signal = s; + await new Promise((r) => setTimeout(r, 0)); + currentAfterUnmount = isCurrent(); + }); + }); + + unmount(); + await act(async () => { + await new Promise((r) => setTimeout(r, 1)); + }); + + expect(signal?.aborted).toBe(true); + expect(currentAfterUnmount).toBe(false); + }); + + it("cancel() invalidates without starting a new request", async () => { + const { result } = renderHook(() => useLatestRequest()); + let current = true; + + act(() => { + void result.current.run(async ({ isCurrent }) => { + await new Promise((r) => setTimeout(r, 0)); + current = isCurrent(); + }); + result.current.cancel(); + }); + + await act(async () => { + await new Promise((r) => setTimeout(r, 1)); + }); + + expect(current).toBe(false); + }); +}); + +describe("useAbortableEffect", () => { + it("re-runs on dependency change and invalidates the previous run", async () => { + const seen: string[] = []; + const gate = deferred(); + + const { rerender } = renderHook( + ({ key }: { key: string }) => + useAbortableEffect( + async ({ isCurrent }) => { + if (key === "a") await gate.promise; + if (!isCurrent()) return; + seen.push(key); + }, + [key], + ), + { initialProps: { key: "a" } }, + ); + + rerender({ key: "b" }); + await act(async () => { + gate.resolve(); + await gate.promise; + }); + + await waitFor(() => expect(seen).toEqual(["b"])); + }); + + it("does not re-run when only the effect closure identity changes", async () => { + const runs = vi.fn(async () => {}); + const { rerender } = renderHook(() => useAbortableEffect(() => runs(), [1])); + + rerender(); + rerender(); + + await waitFor(() => expect(runs).toHaveBeenCalledTimes(1)); + }); +}); diff --git a/frontend/src/hooks/useLatestRequest.ts b/frontend/src/hooks/useLatestRequest.ts new file mode 100644 index 000000000..a1c258cd8 --- /dev/null +++ b/frontend/src/hooks/useLatestRequest.ts @@ -0,0 +1,112 @@ +import { useCallback, useEffect, useMemo, useRef } from "react"; +import type { DependencyList } from "react"; +import { isAbortError } from "@/api/client"; + +/** + * Context handed to a request body. Forward `signal` to every `api.get` the + * body issues, and check `isCurrent()` before every `setState`. + */ +export interface RequestCtx { + /** Aborted when a newer run starts, or when the component unmounts. */ + signal: AbortSignal; + /** + * False once a newer run has started (or the component unmounted). + * + * The generation token — not the abort — is the real guard: a response that + * has already resolved cannot be aborted, and test doubles ignore `signal` + * entirely. Check it before EVERY `setState`, including the one that clears + * a loading flag. + */ + isCurrent: () => boolean; +} + +export interface LatestRequest { + /** + * Runs `fn` as the newest request, aborting and invalidating any predecessor. + * + * Always settles, even when superseded, so `await run(...)` never hangs. + * Aborts and rejections from superseded runs are swallowed; a real error from + * the current run is rethrown for the caller's own `try`/`catch`. + */ + run: (fn: (ctx: RequestCtx) => Promise) => Promise; + /** Abort and invalidate whatever is in flight without starting anything new. */ + cancel: () => void; +} + +/** + * Serialises overlapping requests so that only the most recent one may write + * state — the fix for #882, where clearing an inventory filter (a slow + * whole-repository fetch) and then picking a type let the slow response land + * last and overwrite the grid. + * + * Aborts the predecessor AND compares a monotonic generation token. Both are + * needed: the abort saves the download and the re-render, the token is what + * actually guarantees ordering. + * + * Use this directly only when the fetch must also be callable imperatively + * (`InventoryPage`'s `loadData`, which 13 mutation handlers refresh through). + * Otherwise prefer `useAbortableEffect` below, or `useApiQuery`. + */ +export function useLatestRequest(): LatestRequest { + const genRef = useRef(0); + const ctrlRef = useRef(null); + + const cancel = useCallback(() => { + genRef.current += 1; + ctrlRef.current?.abort(); + ctrlRef.current = null; + }, []); + + const run = useCallback(async (fn: (ctx: RequestCtx) => Promise) => { + ctrlRef.current?.abort(); + const ctrl = new AbortController(); + ctrlRef.current = ctrl; + const gen = ++genRef.current; + const ctx: RequestCtx = { + signal: ctrl.signal, + isCurrent: () => gen === genRef.current, + }; + try { + await fn(ctx); + } catch (err) { + // A superseded run owns none of the state, and an abort is not a failure. + if (isAbortError(err) || gen !== genRef.current) return; + throw err; + } finally { + if (gen === genRef.current) ctrlRef.current = null; + } + }, []); + + useEffect(() => cancel, [cancel]); + + // MUST be identity-stable: callers put this in `useCallback` dependency + // arrays, so a fresh object each render would rebuild their fetch callback + // and turn the effect that calls it into an infinite request loop. + return useMemo(() => ({ run, cancel }), [run, cancel]); +} + +/** + * `useLatestRequest` wired to a dependency array — the drop-in replacement for + * `useEffect(() => { api.get(...).then(setX) }, deps)`. + * + * Use it when the effect writes several pieces of state, or fans out over + * `Promise.all`; use `useApiQuery` when one payload feeds one state. + */ +export function useAbortableEffect( + effect: (ctx: RequestCtx) => Promise, + deps: DependencyList, +): void { + const { run } = useLatestRequest(); + // Read the effect from a ref so a fresh closure each render isn't a dependency. + const effectRef = useRef(effect); + effectRef.current = effect; + + useEffect(() => { + run((ctx) => effectRef.current(ctx)).catch((err) => { + // The effect body owns its own error UI; this is the last-resort net so a + // failed fetch is never an unhandled rejection. + console.error(err); + }); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, deps); +}