From 12eb9ae7b413e78df7d3814cc2c1fbb703f4b27c Mon Sep 17 00:00:00 2001 From: mforce Date: Sat, 10 Oct 2026 19:34:52 +0000 Subject: [PATCH 1/4] feat(web): add useLatestLoad and lint raw async effects --- .github/workflows/ci.yml | 11 ++- web/AGENTS.md | 6 +- web/lint/check-suppressions.mjs | 46 ++++++++---- web/lint/eslint-suppressions.json | 78 ++++++++++++++++++++ web/lint/eslint.config.js | 14 ++++ web/src/components/useLatestLoad.test.ts | 90 ++++++++++++++++++++++++ web/src/components/useLatestLoad.ts | 42 +++++++++++ 7 files changed, 268 insertions(+), 19 deletions(-) create mode 100644 web/src/components/useLatestLoad.test.ts create mode 100644 web/src/components/useLatestLoad.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 96adac169..a803001f6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -429,19 +429,18 @@ jobs: # Suppressions may only shrink (web/AGENTS.md). On push and dispatch there # is no PR base, and only the path check runs: main takes changes only - # through PRs, which ran the growth check. The first PR has no base file. + # through PRs, which ran the growth check. A rule the base config did not + # apply to a file may add that file's baseline. - name: Check lint suppressions env: BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | - base="$RUNNER_TEMP/base-suppressions.json" if [ -n "$BASE_SHA" ]; then git fetch --no-tags --depth=1 origin "$BASE_SHA" - if git cat-file -e "$BASE_SHA:web/lint/eslint-suppressions.json"; then - git show "$BASE_SHA:web/lint/eslint-suppressions.json" > "$base" - fi + node lint/check-suppressions.mjs --base "$BASE_SHA" + else + node lint/check-suppressions.mjs fi - if [ -f "$base" ]; then node lint/check-suppressions.mjs "$base"; else node lint/check-suppressions.mjs; fi - name: Test with coverage gate run: npm run test:coverage diff --git a/web/AGENTS.md b/web/AGENTS.md index e9f339199..ce93d039e 100644 --- a/web/AGENTS.md +++ b/web/AGENTS.md @@ -13,4 +13,8 @@ record you must read before changing the rule. ## Lint -- **`npm run lint` runs ESLint from its own install in `web/lint/`, and its suppressions file may only shrink.** Run `npm ci --prefix lint` once. typescript-eslint 8 declares `typescript <6.1.0` and needs the classic compiler API, which TypeScript 7 does not ship, so `web/lint/package.json` pins TypeScript 6 for the linter while the app builds with 7. `web/eslint.config.js` only re-exports `web/lint/eslint.config.js`, because ESLint 10 lints only files under the config's directory. The config enables `js` and typescript-eslint `recommended`, `react-hooks/rules-of-hooks`, `react-hooks/exhaustive-deps`, type-aware `@typescript-eslint/no-floating-promises`, and errors on unused disable comments. It turns off only style rules and `no-unused-vars`, which tsc already owns. Pre-existing violations live in `web/lint/eslint-suppressions.json`, counted per file and rule. A new violation fails, and so does a suppression that no longer matches: ESLint 10 exits 2 unless `--pass-on-unpruned-suppressions` is passed, which CI never does. ESLint never checks an entry whose file is gone, and it accepts any count the same PR writes, so CI also runs `web/lint/check-suppressions.mjs`. It rejects entries for missing files and, on a pull request, any file/rule entry that is new or higher than at the base. Renaming a file that has suppressions therefore means fixing its violations. After fixing a suppressed violation, run `npm run lint -- --prune-suppressions` and commit the smaller file. Delete entries for deleted files by hand: `--prune-suppressions` leaves them. Never run `--suppress-all` or `--suppress-rule` to admit a new violation, and never fix an `exhaustive-deps` finding by auto-fix: a wrong dependency array can loop renders. CI's `web` job runs the lint; the pre-commit hook lints only staged `web/src` files. +- **`npm run lint` runs ESLint from its own install in `web/lint/`, and its suppressions file may only shrink.** Run `npm ci --prefix lint` once. typescript-eslint 8 declares `typescript <6.1.0` and needs the classic compiler API, which TypeScript 7 does not ship, so `web/lint/package.json` pins TypeScript 6 for the linter while the app builds with 7. `web/eslint.config.js` only re-exports `web/lint/eslint.config.js`, because ESLint 10 lints only files under the config's directory. The config enables `js` and typescript-eslint `recommended`, `react-hooks/rules-of-hooks`, `react-hooks/exhaustive-deps`, type-aware `@typescript-eslint/no-floating-promises`, and errors on unused disable comments. It turns off only style rules and `no-unused-vars`, which tsc already owns. Pre-existing violations live in `web/lint/eslint-suppressions.json`, counted per file and rule. A new violation fails, and so does a suppression that no longer matches: ESLint 10 exits 2 unless `--pass-on-unpruned-suppressions` is passed, which CI never does. ESLint never checks an entry whose file is gone, and it accepts any count the same PR writes, so CI also runs `web/lint/check-suppressions.mjs`. It rejects entries for missing files and, on a pull request (`--base `), any file/rule entry that is new or higher than at the base. The one exception is a rule that the base's own config did not apply to that file, which is how a newly enabled rule records its existing sites. Renaming a file that has suppressions therefore means fixing its violations. After fixing a suppressed violation, run `npm run lint -- --prune-suppressions` and commit the smaller file. Delete entries for deleted files by hand: `--prune-suppressions` leaves them. Never run `--suppress-all` or `--suppress-rule` to admit a new violation, and never fix an `exhaustive-deps` finding by auto-fix: a wrong dependency array can loop renders. CI's `web` job runs the lint; the pre-commit hook lints only staged `web/src` files. + +## Data loading + +- **A screen reads data through `useLatestLoad` or `usePagedList`, never through `await` or `.then(` inside a `useEffect` (#1185).** A raw async effect lets a response for inputs the user has already left land after the newer one and overwrite it, or clear its error, loading flag or rows. Review caught it on 23 PRs (#467, the #703 series, #918 then #940). `useLatestLoad(inputs, load)` in `src/components/` aborts the superseded run and drops its result and its error; `usePagedList` does the same for paged lists, and `useDialogAction` covers dialog writes. A `no-restricted-syntax` rule in `web/lint/eslint.config.js` fails an `await` or `.then(` inside a `useEffect` callback in `src/routes/**` and `src/components/**`. The sites that existed when it landed are in the suppressions file and move to the hook when their screen is next touched. It cannot see an async helper defined outside the effect, nor a write followed by a refresh in an event handler; only the hooks cover those. diff --git a/web/lint/check-suppressions.mjs b/web/lint/check-suppressions.mjs index e871c1dd3..8b1f70c4e 100644 --- a/web/lint/check-suppressions.mjs +++ b/web/lint/check-suppressions.mjs @@ -1,11 +1,14 @@ // ESLint checks a suppression only for files it lints, and accepts any count a // PR writes. This enforces the rest of "suppressions may only shrink" -// (web/AGENTS.md): every entry names an existing file, and with a base file -// given, no file/rule entry is new or higher than at the base. -// Usage, from web/: node lint/check-suppressions.mjs [base-suppressions.json] -import { existsSync, readFileSync } from 'node:fs'; +// (web/AGENTS.md): every entry names an existing file, and with a base given, +// no file/rule entry is new or higher than at the base. The one exception is +// a rule that did not apply to that file under the base's own config: that is +// a newly enabled rule's baseline, not a new violation of an existing rule. +// Usage, from web/: node lint/check-suppressions.mjs [--base ] +import { execFileSync } from 'node:child_process'; +import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { ESLint } from 'eslint'; -const read = (path) => JSON.parse(readFileSync(path, 'utf8')); const entries = (suppressions) => Object.entries(suppressions).flatMap(([file, rules]) => Object.entries(rules).map(([rule, { count }]) => { @@ -13,16 +16,35 @@ const entries = (suppressions) => return { file, rule, count }; }), ); +const atRef = (ref, path) => { + try { + return execFileSync('git', ['show', `${ref}:web/${path}`], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); + } catch { + return null; + } +}; -const head = entries(read('lint/eslint-suppressions.json')); +const head = entries(JSON.parse(readFileSync('lint/eslint-suppressions.json', 'utf8'))); const errors = head.filter(({ file }) => !existsSync(file)).map(({ file, rule }) => `${file} ${rule}: file does not exist`); -const basePath = process.argv[2]; -if (basePath) { - const base = new Map(entries(read(basePath)).map(({ file, rule, count }) => [`${file} ${rule}`, count])); - for (const { file, rule, count } of head) { - const was = base.get(`${file} ${rule}`) ?? 0; - if (count > was) errors.push(`${file} ${rule}: count ${was} -> ${count}`); +const ref = process.argv[2] === '--base' ? process.argv[3] : undefined; +const baseSuppressions = ref && atRef(ref, 'lint/eslint-suppressions.json'); +if (baseSuppressions) { + const base = new Map(entries(JSON.parse(baseSuppressions)).map(({ file, rule, count }) => [`${file} ${rule}`, count])); + // Written beside this script so its imports resolve from web/lint/node_modules. + const baseConfig = 'lint/.base-eslint.config.js'; + writeFileSync(baseConfig, atRef(ref, 'lint/eslint.config.js')); + try { + const eslint = new ESLint({ overrideConfigFile: baseConfig }); + for (const { file, rule, count } of head) { + const was = base.get(`${file} ${rule}`) ?? 0; + if (count <= was || !existsSync(file)) continue; + const severity = (await eslint.calculateConfigForFile(file))?.rules?.[rule]?.[0] ?? 0; + if (was === 0 && severity === 0) continue; + errors.push(`${file} ${rule}: count ${was} -> ${count}`); + } + } finally { + rmSync(baseConfig); } } diff --git a/web/lint/eslint-suppressions.json b/web/lint/eslint-suppressions.json index 02d914c59..bf0305af7 100644 --- a/web/lint/eslint-suppressions.json +++ b/web/lint/eslint-suppressions.json @@ -14,12 +14,30 @@ "count": 1 } }, + "src/components/NamedEntityPicker.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, "src/routes/AppLayout.tsx": { "@typescript-eslint/no-floating-promises": { "count": 1 } }, + "src/routes/ConnectPage.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, + "src/routes/CustomersPage.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, "src/routes/DailyEntryPage.tsx": { + "no-restricted-syntax": { + "count": 5 + }, "react-hooks/exhaustive-deps": { "count": 1 } @@ -27,14 +45,43 @@ "src/routes/Dashboard.tsx": { "@typescript-eslint/no-floating-promises": { "count": 1 + }, + "no-restricted-syntax": { + "count": 7 + } + }, + "src/routes/ExpensesPage.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, + "src/routes/FeedPage.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, + "src/routes/FlocksPage.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, + "src/routes/GradesPage.tsx": { + "no-restricted-syntax": { + "count": 1 } }, "src/routes/HistoryPage.tsx": { + "no-restricted-syntax": { + "count": 1 + }, "react-hooks/exhaustive-deps": { "count": 1 } }, "src/routes/InventoryPage.tsx": { + "no-restricted-syntax": { + "count": 1 + }, "react-hooks/exhaustive-deps": { "count": 1 } @@ -42,13 +89,44 @@ "src/routes/Login.tsx": { "@typescript-eslint/no-floating-promises": { "count": 2 + }, + "no-restricted-syntax": { + "count": 1 + } + }, + "src/routes/ProductsPage.tsx": { + "no-restricted-syntax": { + "count": 1 } }, "src/routes/StockPage.tsx": { + "no-restricted-syntax": { + "count": 1 + }, "react-hooks/exhaustive-deps": { "count": 1 } }, + "src/routes/UsersPage.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, + "src/routes/WaterPage.tsx": { + "no-restricted-syntax": { + "count": 2 + } + }, + "src/routes/sales/SalesPage.tsx": { + "no-restricted-syntax": { + "count": 2 + } + }, + "src/routes/sales/usePayments.tsx": { + "no-restricted-syntax": { + "count": 1 + } + }, "src/theme/FarmThemeProvider.tsx": { "react-hooks/exhaustive-deps": { "count": 1 diff --git a/web/lint/eslint.config.js b/web/lint/eslint.config.js index 66e5271b3..90173f270 100644 --- a/web/lint/eslint.config.js +++ b/web/lint/eslint.config.js @@ -3,6 +3,10 @@ import reactHooks from 'eslint-plugin-react-hooks'; import globals from 'globals'; import tseslint from 'typescript-eslint'; +const inEffect = 'CallExpression:matches([callee.name="useEffect"], [callee.property.name="useEffect"])'; +const rawAsyncEffect = 'Load data with useLatestLoad or usePagedList (web/src/components/). ' + + 'A raw async effect lets a superseded response overwrite newer state (#467, #703, #918).'; + export default tseslint.config( { ignores: ['dist/', 'dev-dist/', 'coverage/', 'lint/'] }, { @@ -25,4 +29,14 @@ export default tseslint.config( 'preserve-caught-error': 'off', }, }, + { + files: ['src/routes/**/*.{ts,tsx}', 'src/components/**/*.{ts,tsx}'], + ignores: ['**/*.test.{ts,tsx}'], + rules: { + 'no-restricted-syntax': ['error', + { selector: `${inEffect} AwaitExpression`, message: rawAsyncEffect }, + { selector: `${inEffect} CallExpression[callee.property.name="then"]`, message: rawAsyncEffect }, + ], + }, + }, ); diff --git a/web/src/components/useLatestLoad.test.ts b/web/src/components/useLatestLoad.test.ts new file mode 100644 index 000000000..df956e2c7 --- /dev/null +++ b/web/src/components/useLatestLoad.test.ts @@ -0,0 +1,90 @@ +import { describe, it, expect, vi } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { useLatestLoad } from "./useLatestLoad"; + +// #1185 — each test settles deferred promises in a chosen ORDER, because the +// order the network answers in is the thing under test. + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (err: unknown) => void; + const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + return { promise, resolve, reject }; +} + +// One deferred per input value, and the signal each run was handed. +function loader() { + const pending = new Map>>(); + const signals = new Map(); + const load = (input: string) => (signal: AbortSignal) => { + const d = deferred(); + pending.set(input, d); + signals.set(input, signal); + return d.promise; + }; + return { pending, signals, load }; +} + +function host(first: string) { + const { pending, signals, load } = loader(); + const hook = renderHook(({ input }) => useLatestLoad([input], load(input)), { initialProps: { input: first } }); + return { ...hook, pending, signals }; +} + +describe("useLatestLoad", () => { + it("drops a superseded response that lands after the newer one", async () => { + const { result, rerender, pending } = host("a"); + rerender({ input: "b" }); + await act(async () => { pending.get("b")!.resolve("rows for b"); }); + await act(async () => { pending.get("a")!.resolve("rows for a"); }); + expect(result.current.data).toBe("rows for b"); + expect(result.current.loading).toBe(false); + }); + + it("drops a superseded response that lands first, and keeps loading for the newer one", async () => { + const { result, rerender, pending } = host("a"); + rerender({ input: "b" }); + await act(async () => { pending.get("a")!.resolve("rows for a"); }); + expect(result.current.data).toBeNull(); + expect(result.current.loading).toBe(true); + }); + + it("drops a superseded failure, so it cannot clear the newer result", async () => { + const { result, rerender, pending } = host("a"); + rerender({ input: "b" }); + await act(async () => { pending.get("b")!.resolve("rows for b"); }); + await act(async () => { pending.get("a")!.reject(new Error("a failed")); }); + expect(result.current).toMatchObject({ data: "rows for b", error: null, loading: false }); + }); + + it("aborts the superseded run's signal, and the last run's on unmount", () => { + const { rerender, unmount, signals } = host("a"); + rerender({ input: "b" }); + expect(signals.get("a")!.aborted).toBe(true); + expect(signals.get("b")!.aborted).toBe(false); + unmount(); + expect(signals.get("b")!.aborted).toBe(true); + }); + + it("reports the current run's failure with no data, and retry loads the same inputs again", async () => { + const load = vi.fn<(signal: AbortSignal) => Promise>() + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValueOnce("rows"); + const { result } = renderHook(() => useLatestLoad(["a"], load)); + await act(async () => {}); + expect(result.current).toMatchObject({ data: null, loading: false }); + expect(result.current.error).toEqual(new Error("offline")); + await act(async () => { result.current.retry(); }); + expect(result.current).toMatchObject({ data: "rows", error: null, loading: false }); + expect(load).toHaveBeenCalledTimes(2); + }); + + it("does not reload when only the load function's identity changes", async () => { + const load = vi.fn(async () => "rows"); + const { rerender } = renderHook(() => useLatestLoad(["a"], () => load())); + await act(async () => {}); + rerender(); + await act(async () => {}); + expect(load).toHaveBeenCalledTimes(1); + }); +}); diff --git a/web/src/components/useLatestLoad.ts b/web/src/components/useLatestLoad.ts new file mode 100644 index 000000000..04ce4b3cc --- /dev/null +++ b/web/src/components/useLatestLoad.ts @@ -0,0 +1,42 @@ +import { useCallback, useEffect, useEffectEvent, useState } from "react"; + +// #1185 — the supported way for a screen to READ data for a set of inputs +// (paged lists use usePagedList; dialog writes use useDialogAction). A raw +// `useEffect(() => { load().then(set) })` lets a response for inputs the user +// has already left land after the newer one and overwrite it (#467, #703, +// #918), and the lint in web/lint/eslint.config.js now refuses that shape. +// +// Each run owns an AbortController, which is also its generation: inputs +// changing, `retry` or unmount abort it, and an aborted run touches no state, +// in its failure path as much as its success path. `load` is read through an +// effect event, so an inline arrow does not reload on every render; only +// `inputs` and `retry` do. +// +// `data` is kept while a newer run loads (`loading` says it may belong to +// the inputs being left) and cleared by a failure: empty is honest, stale is +// not. +export function useLatestLoad( + inputs: readonly unknown[], + load: (signal: AbortSignal) => Promise, +): { data: T | null; error: unknown; loading: boolean; retry: () => void } { + const [state, setState] = useState<{ data: T | null; error: unknown; loading: boolean }>( + { data: null, error: null, loading: true }, + ); + const [attempt, setAttempt] = useState(0); + const run = useEffectEvent(load); + + useEffect(() => { + const controller = new AbortController(); + setState((prev) => ({ data: prev.data, error: null, loading: true })); + // eslint-disable-next-line no-restricted-syntax -- this hook is the guard the rule points to + run(controller.signal).then( + (data) => { if (!controller.signal.aborted) setState({ data, error: null, loading: false }); }, + (error: unknown) => { if (!controller.signal.aborted) setState({ data: null, error, loading: false }); }, + ); + return () => controller.abort(); + // eslint-disable-next-line react-hooks/exhaustive-deps -- the caller's inputs are the keys + }, [...inputs, attempt]); + + const retry = useCallback(() => setAttempt((n) => n + 1), []); + return { ...state, retry }; +} From 49456cdc083665e47d710b410004c01a77e764b7 Mon Sep 17 00:00:00 2001 From: mforce Date: Sat, 10 Oct 2026 19:48:34 +0000 Subject: [PATCH 2/4] fix(web): abort on retry and catch a throwing load in useLatestLoad --- web/src/components/useLatestLoad.test.ts | 29 ++++++++++++++++++++++++ web/src/components/useLatestLoad.ts | 16 +++++++++---- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/web/src/components/useLatestLoad.test.ts b/web/src/components/useLatestLoad.test.ts index df956e2c7..37e3adfce 100644 --- a/web/src/components/useLatestLoad.test.ts +++ b/web/src/components/useLatestLoad.test.ts @@ -79,6 +79,35 @@ describe("useLatestLoad", () => { expect(load).toHaveBeenCalledTimes(2); }); + it("drops the old run's response the moment retry is called, before the new run starts", async () => { + const calls: ReturnType>[] = []; + const { result } = renderHook(() => useLatestLoad(["a"], () => { + const d = deferred(); + calls.push(d); + return d.promise; + })); + const save = deferred(); + // A handler that saves, then retries: the old load answers in the gap + // after retry and before React runs the replacement effect. + await act(async () => { + const handler = save.promise.then(() => result.current.retry()); + save.resolve(); + await handler; + calls[0].resolve("rows from before the save"); + }); + expect(calls).toHaveLength(2); + expect(result.current).toMatchObject({ data: null, loading: true }); + }); + + it("reports a load that throws before returning a promise as an error, not a crash", async () => { + const { result } = renderHook(() => useLatestLoad(["a"], (): Promise => { + throw new RangeError("Invalid time value"); + })); + await act(async () => {}); + expect(result.current).toMatchObject({ data: null, loading: false }); + expect(result.current.error).toEqual(new RangeError("Invalid time value")); + }); + it("does not reload when only the load function's identity changes", async () => { const load = vi.fn(async () => "rows"); const { rerender } = renderHook(() => useLatestLoad(["a"], () => load())); diff --git a/web/src/components/useLatestLoad.ts b/web/src/components/useLatestLoad.ts index 04ce4b3cc..84feffe20 100644 --- a/web/src/components/useLatestLoad.ts +++ b/web/src/components/useLatestLoad.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useEffectEvent, useState } from "react"; +import { useCallback, useEffect, useEffectEvent, useRef, useState } from "react"; // #1185 — the supported way for a screen to READ data for a set of inputs // (paged lists use usePagedList; dialog writes use useDialogAction). A raw @@ -8,7 +8,10 @@ import { useCallback, useEffect, useEffectEvent, useState } from "react"; // // Each run owns an AbortController, which is also its generation: inputs // changing, `retry` or unmount abort it, and an aborted run touches no state, -// in its failure path as much as its success path. `load` is read through an +// in its failure path as much as its success path. `retry` aborts at once, not +// when the replacement effect runs: a retry after an awaited save must drop a +// response that lands in between. A `load` that throws before returning a +// promise is reported like a rejection. `load` is read through an // effect event, so an inline arrow does not reload on every render; only // `inputs` and `retry` do. // @@ -24,12 +27,14 @@ export function useLatestLoad( ); const [attempt, setAttempt] = useState(0); const run = useEffectEvent(load); + const active = useRef(null); useEffect(() => { const controller = new AbortController(); + active.current = controller; setState((prev) => ({ data: prev.data, error: null, loading: true })); // eslint-disable-next-line no-restricted-syntax -- this hook is the guard the rule points to - run(controller.signal).then( + new Promise((resolve) => resolve(run(controller.signal))).then( (data) => { if (!controller.signal.aborted) setState({ data, error: null, loading: false }); }, (error: unknown) => { if (!controller.signal.aborted) setState({ data: null, error, loading: false }); }, ); @@ -37,6 +42,9 @@ export function useLatestLoad( // eslint-disable-next-line react-hooks/exhaustive-deps -- the caller's inputs are the keys }, [...inputs, attempt]); - const retry = useCallback(() => setAttempt((n) => n + 1), []); + const retry = useCallback(() => { + active.current?.abort(); + setAttempt((n) => n + 1); + }, []); return { ...state, retry }; } From f2838704086bd6bf1cd6d38c17e0b27b3045fa1c Mon Sep 17 00:00:00 2001 From: mforce Date: Sat, 10 Oct 2026 19:50:10 +0000 Subject: [PATCH 3/4] test(web): let the old run settle fully in the retry interleaving --- web/src/components/useLatestLoad.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/web/src/components/useLatestLoad.test.ts b/web/src/components/useLatestLoad.test.ts index 37e3adfce..208469e81 100644 --- a/web/src/components/useLatestLoad.test.ts +++ b/web/src/components/useLatestLoad.test.ts @@ -94,6 +94,8 @@ describe("useLatestLoad", () => { save.resolve(); await handler; calls[0].resolve("rows from before the save"); + // Let the old run settle completely before act lets the effect run. + await new Promise((settled) => setTimeout(settled, 0)); }); expect(calls).toHaveLength(2); expect(result.current).toMatchObject({ data: null, loading: true }); From f74a46c487aa746dfe51edd047913ce655d46b94 Mon Sep 17 00:00:00 2001 From: mforce Date: Sat, 10 Oct 2026 19:56:54 +0000 Subject: [PATCH 4/4] fix(web): bound a new rule's suppressions by the base source and reject an unreadable base --- web/AGENTS.md | 2 +- web/lint/check-suppressions.mjs | 58 ++++++++++++++++++++++----------- 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/web/AGENTS.md b/web/AGENTS.md index ce93d039e..c643bd863 100644 --- a/web/AGENTS.md +++ b/web/AGENTS.md @@ -13,7 +13,7 @@ record you must read before changing the rule. ## Lint -- **`npm run lint` runs ESLint from its own install in `web/lint/`, and its suppressions file may only shrink.** Run `npm ci --prefix lint` once. typescript-eslint 8 declares `typescript <6.1.0` and needs the classic compiler API, which TypeScript 7 does not ship, so `web/lint/package.json` pins TypeScript 6 for the linter while the app builds with 7. `web/eslint.config.js` only re-exports `web/lint/eslint.config.js`, because ESLint 10 lints only files under the config's directory. The config enables `js` and typescript-eslint `recommended`, `react-hooks/rules-of-hooks`, `react-hooks/exhaustive-deps`, type-aware `@typescript-eslint/no-floating-promises`, and errors on unused disable comments. It turns off only style rules and `no-unused-vars`, which tsc already owns. Pre-existing violations live in `web/lint/eslint-suppressions.json`, counted per file and rule. A new violation fails, and so does a suppression that no longer matches: ESLint 10 exits 2 unless `--pass-on-unpruned-suppressions` is passed, which CI never does. ESLint never checks an entry whose file is gone, and it accepts any count the same PR writes, so CI also runs `web/lint/check-suppressions.mjs`. It rejects entries for missing files and, on a pull request (`--base `), any file/rule entry that is new or higher than at the base. The one exception is a rule that the base's own config did not apply to that file, which is how a newly enabled rule records its existing sites. Renaming a file that has suppressions therefore means fixing its violations. After fixing a suppressed violation, run `npm run lint -- --prune-suppressions` and commit the smaller file. Delete entries for deleted files by hand: `--prune-suppressions` leaves them. Never run `--suppress-all` or `--suppress-rule` to admit a new violation, and never fix an `exhaustive-deps` finding by auto-fix: a wrong dependency array can loop renders. CI's `web` job runs the lint; the pre-commit hook lints only staged `web/src` files. +- **`npm run lint` runs ESLint from its own install in `web/lint/`, and its suppressions file may only shrink.** Run `npm ci --prefix lint` once. typescript-eslint 8 declares `typescript <6.1.0` and needs the classic compiler API, which TypeScript 7 does not ship, so `web/lint/package.json` pins TypeScript 6 for the linter while the app builds with 7. `web/eslint.config.js` only re-exports `web/lint/eslint.config.js`, because ESLint 10 lints only files under the config's directory. The config enables `js` and typescript-eslint `recommended`, `react-hooks/rules-of-hooks`, `react-hooks/exhaustive-deps`, type-aware `@typescript-eslint/no-floating-promises`, and errors on unused disable comments. It turns off only style rules and `no-unused-vars`, which tsc already owns. Pre-existing violations live in `web/lint/eslint-suppressions.json`, counted per file and rule. A new violation fails, and so does a suppression that no longer matches: ESLint 10 exits 2 unless `--pass-on-unpruned-suppressions` is passed, which CI never does. ESLint never checks an entry whose file is gone, and it accepts any count the same PR writes, so CI also runs `web/lint/check-suppressions.mjs`. It rejects entries for missing files and, on a pull request (`--base `), any file/rule entry that is new or higher than at the base. The one exception is a rule that the base's own config did not apply to that file: the entry may then record at most the sites that rule finds in the file's BASE source. That is how a newly enabled rule, or a widened glob, records existing sites, while a moved or added file and any new site still have to be fixed. An unreadable `--base` exits 2 rather than skipping the check. Renaming a file that has suppressions therefore means fixing its violations. After fixing a suppressed violation, run `npm run lint -- --prune-suppressions` and commit the smaller file. Delete entries for deleted files by hand: `--prune-suppressions` leaves them. Never run `--suppress-all` or `--suppress-rule` to admit a new violation, and never fix an `exhaustive-deps` finding by auto-fix: a wrong dependency array can loop renders. CI's `web` job runs the lint; the pre-commit hook lints only staged `web/src` files. ## Data loading diff --git a/web/lint/check-suppressions.mjs b/web/lint/check-suppressions.mjs index 8b1f70c4e..eb6680d4d 100644 --- a/web/lint/check-suppressions.mjs +++ b/web/lint/check-suppressions.mjs @@ -2,13 +2,27 @@ // PR writes. This enforces the rest of "suppressions may only shrink" // (web/AGENTS.md): every entry names an existing file, and with a base given, // no file/rule entry is new or higher than at the base. The one exception is -// a rule that did not apply to that file under the base's own config: that is -// a newly enabled rule's baseline, not a new violation of an existing rule. +// a rule that did not apply to that file under the base's own config: then the +// entry may record at most the violations the rule finds in the BASE source of +// that file, so a newly enabled rule can record existing sites, while code +// moved or added under a widened glob still has to be fixed. // Usage, from web/: node lint/check-suppressions.mjs [--base ] import { execFileSync } from 'node:child_process'; import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { ESLint } from 'eslint'; +const args = process.argv.slice(2); +if (!(args.length === 0 || (args.length === 2 && args[0] === '--base' && args[1] !== ''))) { + console.error('Usage, from web/: node lint/check-suppressions.mjs [--base ]'); + process.exit(2); +} + +const root = execFileSync('git', ['rev-parse', '--show-toplevel'], { encoding: 'utf8' }).trim(); +const git = (...gitArgs) => execFileSync('git', gitArgs, { cwd: root, encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 }); +// null only when the commit has no such path; any git failure throws. +const atCommit = (commit, path) => + git('ls-tree', '--name-only', commit, '--', `web/${path}`).trim() === '' ? null : git('show', `${commit}:web/${path}`); + const entries = (suppressions) => Object.entries(suppressions).flatMap(([file, rules]) => Object.entries(rules).map(([rule, { count }]) => { @@ -16,32 +30,38 @@ const entries = (suppressions) => return { file, rule, count }; }), ); -const atRef = (ref, path) => { - try { - return execFileSync('git', ['show', `${ref}:web/${path}`], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); - } catch { - return null; - } -}; const head = entries(JSON.parse(readFileSync('lint/eslint-suppressions.json', 'utf8'))); const errors = head.filter(({ file }) => !existsSync(file)).map(({ file, rule }) => `${file} ${rule}: file does not exist`); -const ref = process.argv[2] === '--base' ? process.argv[3] : undefined; -const baseSuppressions = ref && atRef(ref, 'lint/eslint-suppressions.json'); +let base = null; +if (args.length === 2) { + try { + base = git('rev-parse', '--verify', '--end-of-options', `${args[1]}^{commit}`).trim(); + } catch { + console.error(`--base ${args[1]} is not a commit this checkout has. Fetch it first.`); + process.exit(2); + } +} +const baseSuppressions = base && atCommit(base, 'lint/eslint-suppressions.json'); +if (base && !baseSuppressions) console.log(`${args[1]} predates lint/eslint-suppressions.json: only the path check runs.`); if (baseSuppressions) { - const base = new Map(entries(JSON.parse(baseSuppressions)).map(({ file, rule, count }) => [`${file} ${rule}`, count])); + const was = new Map(entries(JSON.parse(baseSuppressions)).map(({ file, rule, count }) => [`${file} ${rule}`, count])); // Written beside this script so its imports resolve from web/lint/node_modules. const baseConfig = 'lint/.base-eslint.config.js'; - writeFileSync(baseConfig, atRef(ref, 'lint/eslint.config.js')); + writeFileSync(baseConfig, atCommit(base, 'lint/eslint.config.js')); try { - const eslint = new ESLint({ overrideConfigFile: baseConfig }); + const atBase = new ESLint({ overrideConfigFile: baseConfig }); for (const { file, rule, count } of head) { - const was = base.get(`${file} ${rule}`) ?? 0; - if (count <= was || !existsSync(file)) continue; - const severity = (await eslint.calculateConfigForFile(file))?.rules?.[rule]?.[0] ?? 0; - if (was === 0 && severity === 0) continue; - errors.push(`${file} ${rule}: count ${was} -> ${count}`); + const allowed = was.get(`${file} ${rule}`) ?? 0; + if (count <= allowed || !existsSync(file)) continue; + const appliedAtBase = ((await atBase.calculateConfigForFile(file))?.rules?.[rule]?.[0] ?? 0) !== 0; + const source = appliedAtBase ? null : atCommit(base, file); + const baseSites = source === null ? 0 : (await new ESLint({ ruleFilter: ({ ruleId }) => ruleId === rule }) + .lintText(source, { filePath: file }))[0].messages.filter((m) => m.ruleId === rule).length; + if (count > Math.max(allowed, baseSites)) { + errors.push(`${file} ${rule}: count ${allowed} -> ${count}` + (appliedAtBase ? '' : ` (the base source has ${baseSites})`)); + } } } finally { rmSync(baseConfig);