Skip to content

build: turn on no-floating-promises and no-misused-promises, and fix what they found - #287

Open
zizzfizzix wants to merge 7 commits into
mainfrom
claude/resolve-277-zxjyer
Open

build: turn on no-floating-promises and no-misused-promises, and fix what they found#287
zizzfizzix wants to merge 7 commits into
mainfrom
claude/resolve-277-zxjyer

Conversation

@zizzfizzix

@zizzfizzix zizzfizzix commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Closes #277.

The pair type-aware linting was switched on for in #7, staged in DEFERRED with 128 and 35 violations. Both are now enabled with no options at all, in a new PROMISE_RULES group beside ALREADY_CLEAN, and pnpm lint is clean.

trackEvent returns void

Its body was already inside one try/catch, so the promise it handed back could not reject and no caller had anything to do with it — only the type said otherwise. The awaitable half is now captureEvent, which the tests use; the wrapper discards it. That settles 65 of the 128 floating promises with one signature rather than 65 voids.

queueEvent behind it needed no such change: every call site already awaits it, so the rule never had anything to say about it.

The rest were read one at a time

Work that genuinely has nobody to await it goes through the new src/utils/report-rejection.ts:

  • reportRejection(task) — for a caller that cannot await: the synchronous background entrypoint, a JSX event handler, the message router that must return true before its dispatcher finishes.
  • reportingListener(handler) — the same thing shaped for a callback position that ignores the return value: browser.*.addListener, storage.watch, addEventListener.

Both report the rejection, which is what the rules were warning about and what a bare void would still lose. A bare void is kept only where the callee already logs its own failures and resolves either way — getConsentState, getAllPresets, setupUninstallUrl — and each of those sites says so, following the comment already in install.ts.

Six that should have been awaited

Not discards — bugs the rules found, which now cannot come back:

  • trackEvent from a content script never caught a failed sendMessage: the try wrapped an un-awaited call, so the catch was decorative.
  • injectContentScriptToAllTabs on install could reject into nothing.
  • Five E2E assertions ran against a tab bringToFront() had not finished raising.
  • Two E2E fixture helpers returned before their storage write landed.
  • The content-script test harness resolved as soon as the listeners had been called, not when they had finished answering.

No configured exceptions

no-misused-promises first went in with checksVoidReturn.attributes off, on the argument that the rewrite it wanted — onClick={() => void handle()} — dropped the same rejection as the async handler and only added syntax.

That argument does not survive the helper added alongside it. onClick={() => reportRejection(handleSave())} reports what the handler threw instead of discarding it, so the rule is asking for a real handler, not ceremony; a rejected click handler is exactly as invisible as the service-worker listener this rule is usually credited with catching. The third commit turns the option back on and routes all 23 JSX attributes through reportRejection. Two long inline handlers became named copyRow functions on the way, in DataTable and FullDataViewApp, rather than growing a wrapper around a 15-line arrow.

SidePanel passes one resetSystemPresets to both of its Footers. Wrapping at each site would have given the splash branch its own arrow that no test clicks; the two sites always wanted the same discard, so it is written once.

Two helpers, named late

Both went in under names that described the situation rather than the behaviour, and the last two commits fix that.

fireAndForget named the thing it exists to replace: void promise is fire-and-forget, and the entire argument for the helper — and for turning checksVoidReturn.attributes back on — is that a bare void drops the rejection and this does not. Its doc comment had to end with "plus the report that makes it worth writing" to correct its own name. It and asListener are now reportRejection and reportingListener.

settle, the test helper, had the milder version of the same fault. Settled is the Promise spec's word — it is what Promise.allSettled waits for — so beside the other two it read as promise vocabulary while settling nothing; and it promised an outcome that one macrotask turn cannot deliver once a handler waits on a real timer. It is now flushMicrotasks, which names the mechanism, and its comment says what it does not do.

Both renames are behaviour-free; each is the whole of its commit. The smell test is CLAUDE.md's own: when a comment is needed to explain what something does, the name is what should change.

Also here

AsyncMessageHandler for the two message dispatchers, since the router relies on them being async; tests/support/flush-microtasks.ts for tests that trigger a listener reportingListener left unawaited; CLAUDE.md updated.

The second commit is unrelated to the rules and fixes something that has been failing on main: ensureVisible in ConfigForm scrolls from inside a requestAnimationFrame and nothing waited for that frame, so whether its "nothing to scroll to" arm ran at all was a race. Since #285 that has been failing pnpm test:coverage intermittently — reproducibly with six busy cores, three runs out of three at 1749/1751, on main as much as here. The branch was always reachable; only the test was racy, so the fix drives the frame by hand rather than touching the component. It covers existing branches rather than adding any, so the denominator is unchanged at 1751.

Verification

fmt-check, lint, compile and build pass. test:coverage is at 100/100/100/100 with the fidelity check green, and holds under the load harness that reproduced the flake.

The E2E suite was not run locally: this environment's pre-installed Playwright browser does not match the version the repo pins, and the matching build cannot be downloaded here, so all 117 specs fail at launchPersistentContext before any spec code runs. The six E2E edits type-check and lint under the type-aware config, but CI is the first place they actually execute. Worth a closer look there than usual, because the JSX rewrites in the third commit touch handlers the E2E suite drives.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LakMrJTG2mdy4N6s1AHEnm

The pair type-aware linting was switched on for in #7, staged in DEFERRED
with 128 and 35 violations. Both are now enabled and clean.

`trackEvent` returns `void`. Its body was already inside one `try`/`catch`,
so the promise it handed back could not reject and no caller had anything to
do with it — only the type said otherwise. The awaitable half is now
`captureEvent`, which the tests use; the wrapper discards it. That settles 65
of the 128 floating promises with one signature.

The rest were read one at a time. Genuinely fire-and-forget work goes through
`src/utils/fire-and-forget.ts`: `fireAndForget(task)` for a caller that cannot
await (the synchronous background entrypoint, a React event handler, the
message router that must return `true` before its dispatcher finishes) and
`asListener(handler)` for a callback position that ignores the return value.
Both report the rejection, which is what the rule was warning about and what a
bare `void` would still lose. A `void` is kept only where the callee already
logs its own failures and resolves either way, and each of those sites says so.

Several were "should have been awaited" rather than fire-and-forget, and are
now bugs that cannot come back:

- `trackEvent` from a content script never caught a failed `sendMessage`,
  because the `try` wrapped an un-awaited call.
- `injectContentScriptToAllTabs` on install could reject into nothing.
- Five E2E assertions ran against a tab `bringToFront()` had not finished
  raising, and two fixture helpers returned before their storage write landed.
- The content-script test harness resolved as soon as the listeners had been
  called, not when they had finished answering.

`no-misused-promises` runs with `checksVoidReturn.attributes` off, with the
reason in the config: a React handler prop is `() => void` because React has
no other spelling for one, so the rewrite the rule wants discards exactly the
same promise with more syntax. Every other void-return position stays checked.

Coverage stays at 100/100/100/100.
`ensureVisible` in `ConfigForm` scrolls the highlighted row into view from
inside a `requestAnimationFrame`, and nothing waited for that frame. Whether
its "nothing to scroll to" arm ever ran was a race: on an idle machine a late
frame landed after the list had gone and covered it, under load no frame did
and the ternary's `: null` and the `if (el)` else reported as dead.

That has been failing `pnpm test:coverage` intermittently since #285 —
reproducibly with six busy cores, three runs out of three at 1749/1751, on
`main` as much as here. The branch was always reachable; only the test was
racy, so this drives the frame by hand rather than touching the component.
Same three-run load harness now reports 1751/1751.
@zizzfizzix zizzfizzix changed the title build: turn on no-floating-promises and no-misused-promises build: turn on no-floating-promises and no-misused-promises, and fix what they found Sep 2, 2026
`no-misused-promises` was enabled with `checksVoidReturn.attributes` off, on
the argument that the rewrite it wanted — `onClick={() => void handle()}` —
dropped the same rejection as the async handler and only added syntax.

That argument does not survive `fireAndForget`, added in the previous commit.
`onClick={() => fireAndForget(handleSave())}` reports what the handler threw
instead of discarding it, so the rule is asking for a real handler, not
ceremony. A rejected click handler is exactly as invisible as the
service-worker listener this rule is usually credited with catching, and there
are 23 of them here.

So the rule runs with no options at all, and every JSX attribute that took an
async function now routes through `fireAndForget`. Two long inline handlers
became named `copyRow` functions on the way, in `DataTable` and
`FullDataViewApp`, rather than growing a wrapper around a 15-line arrow.

`SidePanel` passes one `resetSystemPresets` to both of its `Footer`s. Wrapping
at each site would have given the splash branch its own arrow that no test
clicks; the two sites always wanted the same discard, so it is written once.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LakMrJTG2mdy4N6s1AHEnm
…place

`fireAndForget` named the thing it exists to replace. `void promise` is
fire-and-forget; the whole argument for adding the helper — and for turning
`checksVoidReturn.attributes` back on in the previous commit — was that a bare
`void` drops the rejection and this does not. So the function named "forget"
was the one that specifically remembers.

The give-away was in its own doc comment, which had to end with "plus the
report that makes it worth writing" to correct the name above it. CLAUDE.md
already says that when a comment is needed to explain what something does, the
name is what should change.

`fireAndForget` is now `reportRejection` and `asListener` is
`reportingListener`, so both carry the reporting rather than only the first.
The module follows to `src/utils/report-rejection.ts`, and the log line drops
the same phrase: an unawaited task is what it is, a forgotten one is what this
is not.

No behaviour change; the rename is the whole diff.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LakMrJTG2mdy4N6s1AHEnm
`settled` is the Promise spec's word — a promise is settled when it is
fulfilled or rejected, which is what `Promise.allSettled` waits for. Sitting
beside `reportRejection` and `reportingListener`, `settle()` read as part of
that vocabulary while settling nothing: it takes no promise, awaits no promise,
and yields a turn of the event loop.

It also promised an outcome it cannot deliver. One macrotask turn is enough
here only because every collaborator in these tests is a resolved mock, so the
pending work is all microtasks. Point one of those handlers at a real timer and
nothing has settled, and the test goes quietly flaky.

`flushMicrotasks` names the mechanism, which here is the whole of the
guarantee, and the doc comment now says what it does not do — timers are not
advanced, and `vi.waitFor` is the tool when one is involved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LakMrJTG2mdy4N6s1AHEnm
An audit of the 19 comment sites this branch authored, against the rule in
CLAUDE.md that a comment earns its place by saying something the code cannot.

Four were deleted outright. `startBackground` no longer explains that its three
tasks are started rather than awaited, and `SidePanel` no longer defends one
constant serving two identical props: renaming `fireAndForget` to
`reportRejection` moved both of those facts into the code. The E2E button's
comment said a click handler cannot await, which is true of every click
handler, and then restated the line under it.

Five were shortened to the part that is not visible locally. `install.ts` kept
the consequence of not awaiting and dropped the sentence restating the callee's
name; `trackEvent` kept why `void` is honest and dropped the lint-violation
arithmetic, which justified the pull request rather than the code; the
`no-misused-promises` note and `reportRejection`'s own doc lost the passages
that enumerated call sites CLAUDE.md already lists.

The ones that stayed all name a constraint that is invisible at the call site:
why a clipboard write must not be awaited before `window.open`, why fake-browser's
`trigger` does not mean the listeners have finished, why the mocks in three test
files must resolve rather than return `undefined`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LakMrJTG2mdy4N6s1AHEnm
Extracting `flushMicrotasks` two commits ago created a canonical home for
`new Promise((resolve) => setTimeout(resolve, 0))` and then left eight
byte-identical copies of that line in place under other names — six
`flushWatchers`, two `flush` — plus two written out inline in
`Settings.test.tsx`, each with its own copy of the same doc comment. Five
files using the shared helper while eight kept their own is worse than either
end of that: the next person has to work out which convention applies.

All ten now import it. The line survives in exactly one place, and CLAUDE.md
says to import rather than re-declare.

`FullDataViewApp` also had `setupWatchers().catch(log.error)`, which is
`reportRejection` spelled by hand. `no-floating-promises` accepts a `.catch`,
so nothing flagged it; it is the same idea and now uses the same helper.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LakMrJTG2mdy4N6s1AHEnm
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.

dev: turn on no-floating-promises and no-misused-promises

2 participants