Skip to content

chore(web): add useLatestLoad and lint raw async effects - #1190

Merged
mforce merged 5 commits into
mainfrom
chore/correct-latest-load
Oct 10, 2026
Merged

mforce merged 5 commits into
mainfrom
chore/correct-latest-load

Conversation

@mforce

@mforce mforce commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Adds one supported way for a screen to load data, and a lint that stops new raw async effects.

Closes #1185

The hook

useLatestLoad(inputs, load) in web/src/components/, next to usePagedList. It returns { data, error, loading, retry }.

  • Each run gets its own AbortController, which is also its generation. Changing inputs, calling retry, or unmounting aborts it. An aborted run sets no state, in the failure path as much as the success path.
  • load is read through useEffectEvent, so an inline arrow does not reload on every render. Only inputs and retry reload.
  • data stays while a newer run loads, so loading tells a screen it may belong to the inputs being left. A failure clears it, as usePagedList does: empty is honest, stale is not.

The lint

no-restricted-syntax in web/lint/eslint.config.js, for src/routes/** and src/components/** (test files excluded). It flags an await or a .then( call anywhere inside a useEffect(...) / React.useEffect(...) call:

Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918).

There are 30 existing sites in 18 files. I read each flagged line, and none is a false positive. They are recorded in web/lint/eslint-suppressions.json and none is migrated here. The hook carries the one reasoned eslint-disable, because it is the guard the rule points to.

Ceiling: the rule cannot see an async helper defined outside the effect and called from it, nor a write followed by a refresh in an event handler. Only the hooks cover those. web/AGENTS.md says so.

The suppressions growth check needed one change

The growth check from #1155 did not allow this baseline. It refused any file/rule entry that was new against the base, so this PR's 30 new entries failed it. check-suppressions.mjs now has one exception. When a rule did not apply to a file under the base's own ESLint config (calculateConfigForFile on the base eslint.config.js, presets included), that file's entry may record at most the sites the rule finds in the file's base source. So a newly enabled rule, or a widened glob, can record sites that already existed. A moved or new file, and any new site, still have to be fixed. Rules already on at the base, such as rules-of-hooks or no-debugger, are ratcheted as before.

The script takes --base <git-ref>, so CI and a local run use the same command: node lint/check-suppressions.mjs --base origin/main. A bare --base or a ref the checkout cannot resolve exits 2. A commit that predates the suppressions file runs the path check only, and the script says so.

Checks run locally

  • npm run lint passes. node lint/check-suppressions.mjs --base origin/main passes, and so does the run without a base. npm run typecheck passes.
  • npx vitest run src/components/useLatestLoad.test.ts src/components/usePagedList.test.tsx --maxWorkers=2: 56 passed at f74a46c4.
  • Hook mutations, each reverted before the next:
Mutation in useLatestLoad.ts Result
Success path ignores aborted red: 2 tests ("drops a superseded response…" ×2)
Failure path ignores aborted red: "drops a superseded failure…"
Cleanup does not abort red: 4 tests
load added to the effect keys the suite never finishes (endless reload with an inline load), killed at 120 s

Proof

The lint proof was run at 12eb9ae7; the growth table below was re-run at f74a46c4. Every scratch edit was reverted (git status clean afterwards). Absolute paths in the output are shortened to repo-relative.

The lint catches #918's pattern

#918's squash diff. ESLint, with this PR's rule and no suppressions, linted Dashboard.tsx at f4fa0570^ and at f4fa0570. Before #918 the rule fires at line 71. After it, the rule fires at lines 138, 189 and 211, which are #918's three effects. So the per-file count goes from 1 to 3, and the ratchet would have failed #918.

On today's code. A second copy of #918's yesterday-close effect, taken verbatim from f4fa0570, was added to Dashboard.tsx. That file has 7 suppressed sites.

A1. Effect added:

  useEffect(() => {
    let cancelled = false;
    getProductionReport(daysBefore(today, 1), daysBefore(today, 1))
      .then((r) => {
        if (cancelled) return;
        const yesterday = r.days[0] ?? null;
        setYesterdayClose(yesterday && yesterday.recordedFlocks > 0 && yesterday.missingFlocks === 0 ? yesterday.totalEggs : null);
      })
      .catch(() => { if (!cancelled) setYesterdayClose(null); });
    return () => { cancelled = true; };
  }, [today]);

$ npm run lint

src/routes/Dashboard.tsx
  305:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
  313:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
  317:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
  326:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
  364:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
  401:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
  412:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
  424:5  error  Load data with useLatestLoad or usePagedList (web/src/components/). A raw async effect lets a superseded response overwrite newer state (#467, #703, #918)  no-restricted-syntax
✖ 8 problems (8 errors, 0 warnings)
exit=1

A2. Reverted:
$ npm run lint

exit=0

A3. The same read through useLatestLoad instead:

  const yesterdayReport = useLatestLoad([today], () => getProductionReport(daysBefore(today, 1), daysBefore(today, 1)));

$ npm run lint

exit=0

Once the count goes over the suppressed 7, ESLint reports every site of the rule in the file, the suppressed ones included.

The growth check on a later PR

node lint/check-suppressions.mjs --base <ref>, with only the suppressions JSON edited. origin/main is this PR's base. HEAD stands in for the next PR's base.

Suppressions change Base Exit Output
This PR's 30 no-restricted-syntax entries, unchanged origin/main 0
+ rules-of-hooks entry for StockBar (rule on at the base) origin/main 1 src/components/StockBar.tsx react-hooks/rules-of-hooks: count 0 -> 1
+ no-debugger entry (on at the base through js recommended) origin/main 1 src/components/StockBar.tsx no-debugger: count 0 -> 1
+ no-restricted-syntax entry for StockBar (rule off at the base, but its base source has no site) origin/main 1 src/components/StockBar.tsx no-restricted-syntax: count 0 -> 1 (the base source has 0)
Dashboard no-restricted-syntax 7 -> 8 (rule off at the base; its base source has 7) origin/main 1 src/routes/Dashboard.tsx no-restricted-syntax: count 0 -> 8 (the base source has 7)
Unchanged, against this PR's head as the next PR's base HEAD 0
Dashboard no-restricted-syntax 7 -> 8 HEAD 1 src/routes/Dashboard.tsx no-restricted-syntax: count 7 -> 8

Rows 4 and 5 were green at 12eb9ae7. They are red now, because the base source caps a new rule's entries. That closes the ceiling the earlier version of this body described: the PR that enables a rule can no longer baseline a fresh violation of it. An entry with no matching violation still fails ESLint itself (npm run lint exits 2).

Review round 1 fixes

Finding Fix Commit
1. Moving code under a widened glob let a suppression grow A new-rule entry is capped at the sites in the file's base source; a path absent at the base gets 0 f74a46c4
2. retry() left the old run live until the effect ran The active controller is kept in a ref and aborted synchronously in retry() 49456cdc, test hardened in f2838704
3. A load that throws synchronously escaped the hook load is invoked inside new Promise(resolve => resolve(...)), so a throw takes the guarded rejection path 49456cdc
4. An unreadable --base skipped the growth check The ref is resolved to a commit first; a bare --base or an unknown ref exits 2; "predates the file" is told apart from a git error f74a46c4

Findings 2 and 3. Two new tests: "drops the old run's response the moment retry is called, before the new run starts" (the save-then-retry interleaving) and "reports a load that throws before returning a promise as an error, not a crash". Against the hook as at 12eb9ae7, both fail. With the fix, 8 of 8 pass. Mutations: removing the abort in retry() fails the first test, and invoking load unguarded fails the second. My first version of the retry test passed without the abort, because the new Promise wrapper adds a microtask. f2838704 lets the old run settle fully (a setTimeout(0)) before act ends, and the abort mutation then fails it.

Finding 1. The reviewer's repro, plus two controls. Base = HEAD, where the rule is on for routes and components. "Before fix" runs the check as at f2838704.

Scenario (base = HEAD, where the rule is on for routes/components) npm run lint Check before fix (f2838704) Check after fix Output after fix
NamedEntityPicker moved to src/lib/ behind a re-export, glob widened, a second async effect added, entry moved and raised 1 -> 2, bannerCache.ts's existing site recorded 0 0 1 src/lib/review1190NamedEntityPicker.tsx no-restricted-syntax: count 0 -> 2 (the base source has 0)
Glob widened to src/farm/**, its existing sites recorded with --suppress-rule (1 new entries) 0 0 0
Same widening plus one new async effect in src/farm/useLogoObjectUrl.ts, its entry raised by 1 0 0 1 src/farm/useLogoObjectUrl.ts no-restricted-syntax: count 0 -> 2 (the base source has 1)

Finding 4.

--base argument, with Dashboard no-floating-promises raised 1 -> 2 Check before fix Check after fix Output after fix
--base HEAD 1 1 src/routes/Dashboard.tsx @typescript-eslint/no-floating-promises: count 1 -> 2
--base no-such-ref-1190 0 2 --base no-such-ref-1190 is not a commit this checkout has. Fetch it first.
--base 0 2 Usage, from web/: node lint/check-suppressions.mjs [--base <git-ref>]

@mforce mforce added this to the Platform hardening milestone Oct 10, 2026
@mforce

mforce commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Review round 1 (Codex gpt-6-astra, head 12eb9ae7): 4 realistic findings

  1. P2: moving code into a newly covered path lets suppressions grow. A real move of NamedEntityPicker behind a re-export raised its async-effect count from 1 to 2, and the checker, lint and typecheck all passed. Fix: grant the new-rule exception only to paths that exist at the base. For an existing file newly brought into a glob, allow only the violations in the base source, not a count HEAD supplies.
  2. P2: retry() does not cancel the old run until the effect runs. A response that settles in between, as after an awaited save, can be kept. Fix: keep the active controller in a ref and abort it synchronously at the start of retry(). Add the save-then-retry interleaving as a test.
  3. P2: a loader that throws synchronously escapes the hook. Fix: catch invocation failures as well as promise rejections, and add a synchronous-throw test that waits for the error state.
  4. P2: an invalid --base silently turns the growth check off (locally; CI passes the right SHA). Fix: resolve --base to a commit and fail on an unreadable ref. Tell "a commit that predates the file" apart from a git error.

Verified: the lint selector catches nested async helpers, React.useEffect, async IIFEs and .then on variables. A base config that fails to load fails closed. CI fetches and passes the PR's base SHA. Removing any of the hook's success, error or cleanup guards turns the tests red.

@mforce

mforce commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Round 1 review of #1190: four realistic findings (suppression growth through a widened glob, retry() not cancelling at once, a synchronous load throw escaping, an unreadable --base skipping the growth check).

All four are fixed. Head: f74a46c4.

  1. Widened glob (f74a46c4). A rule that did not apply to a file at the base may now record at most the sites it finds in that file's base source. A path absent at the base gets 0. Your repro (NamedEntityPicker moved to src/lib/ behind a re-export, glob widened, entry raised 1 -> 2) exits 0 before the fix and 1 after. A legitimate widening to src/farm/** that records its one existing site still passes. That widening plus a new site in the same file fails.
  2. retry() (49456cdc, test hardened in f2838704). The active controller is kept in a ref and aborted synchronously in retry(). The save-then-retry test fails against the hook as at 12eb9ae7 and when the abort is removed.
  3. Synchronous throw (49456cdc). load runs inside new Promise(resolve => resolve(load(signal))), so a throw takes the guarded rejection path. The new test fails against 12eb9ae7 and when load is invoked unguarded.
  4. Unreadable --base (f74a46c4). The ref is resolved to a commit first. A bare --base and an unknown ref now exit 2, where the old script exited 0 with the same growth. A commit that predates the suppressions file runs the path check only and says so.

The fix for 1 also closes the ceiling I had noted: the PR that enables a rule can no longer baseline a fresh violation of it. The tables are in the PR body under "Review round 1 fixes" and "The growth check on a later PR".

@mforce

mforce commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Review round 2 (Codex gpt-6-astra, head f74a46c4): no findings.

All four round 1 repros are closed: the NamedEntityPicker move, save-then-retry, a synchronous throw, and a bad or bare --base.

  • Suppression checks: lower counts pass. Excess counts and unreadable base inputs fail.
  • Hook: all 17 probes pass. Removing either hook fix turns its test red.
  • Required checks: lint, the suppression check (with and without a base), typecheck and the hook tests pass.

The review loop stops here.

@mforce
mforce merged commit 734a834 into main Oct 10, 2026
20 checks passed
@mforce
mforce deleted the chore/correct-latest-load branch October 10, 2026 22:18
mforce added a commit that referenced this pull request Oct 10, 2026
Folds #806's OAuth audience and MCP tool-gate rules into src/AGENTS.md, and
ports the rules #1188, #1190, #1193 and #1199 added to the instruction files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop superseded API responses overwriting newer SPA state

1 participant