Skip to content

fix(react): an invalid query update is rejected, not fatal - #553

Merged
blove merged 3 commits into
mainfrom
blove/invalid-query-seam
Aug 31, 2026
Merged

fix(react): an invalid query update is rejected, not fatal#553
blove merged 3 commits into
mainfrom
blove/invalid-query-seam

Conversation

@blove

@blove blove commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What this is

An invalid query arriving on the prop after mount used to destroy a live grid. The compiler's CompiledQueryValidationError was thrown synchronously out of the same layout effect that already guards derivations, so it escaped the commit and React unmounted the subtree — measured as rows 3 → 0, container 8702 → 0 bytes.

It is now a rejected write: the row model keeps the query it already had, the grid stays interactive and keeps painting the rows that query selected, and a later valid query still lands.

This is the sibling of #550. Spec: the 2026-08-31 amendment in docs/superpowers/specs/2026-08-27-invalid-derivations-are-rejected-design.md. Plan: docs/superpowers/plans/2026-08-31-invalid-query-is-rejected.md.

Measuring shrank the job, and corrected me twice

The follow-up assumed query rejection was a bigger design question than derivations, because a query is consumer-controlled state with an onQueryChange round trip. Probing all four doors showed only one is broken:

Path Result Verdict
Mount throws; grid never renders correct — fail-fast on config
query prop on update rows 3 → 0, bytes 8702 → 0 the only hazard
grid.setQuery while controlled no throw, no change correct — documented to report intent via onQueryChange and stop
grid.setQuery while uncontrolled throws, grid survives intact correct — a catchable throw from the consumer's own call

I first recorded the imperative door as "silently swallows the query" — a second defect. It isn't: my probe passed both query and onQueryChange, so the surface was controlled, where setQuery deliberately doesn't apply. Re-measuring uncontrolled showed a catchable throw that destroys nothing.

That also dissolved the design question. onQueryChange reports engine-originated changes, so a refused consumer prop must not fire it — pinned, not debated.

Also in here

The shared mechanism is extracted. The two guards had byte-identical executable logic. reportRejectedWrite (module-private, so no API surface) now owns the name check, rethrow, field reads with fallbacks, and the index-stripped warnOnce key. It is parameterized on a set of accepted error names because setRows — the remaining unguarded call on this seam — would need two: remapSetRowsError passes CompiledQueryValidationError through unchanged but wraps other faults as PretableSetRowsExecutionError. That is filed, not built.

The extraction's payoff is visible as data: mutations to the shared name set and to the key now red both test files, which is exactly the "fix one, miss the other" failure mode.

What the reviews caught

  • This branch made a comment fix(react): an invalid derivations update is rejected, not fatal #550 shipped false. It said the setQuery throw was "the pre-existing unguarded hazard, filed separately" — true when written, invalidated by this very change. Found by checking a comment the diff never touched. The implementer then found the same false claim in a second place no reviewer had named.
  • A fix introduced a third false comment, in the file whose stated purpose is comment accuracy: it claimed an assertion pins a spurious query-rejected warning that its fixture cannot produce. Proved by mutation — forcing the path failed exactly one test and every warn-count assertion stayed green.
  • Nothing pinned the headline promise. Every rejection test baselined on an empty query and asserted 3 rows — also the unfiltered count, so it could not distinguish "previous query kept" from "query cleared". A narrowing baseline now pins it, and a clear-the-query mutation kills it.

The chained applyQuery path — where a throw is an unhandled rejection rather than an unmount — was flagged in the plan as possibly untestable. It is testable, and the test proves which path it took: instrumentation confirms it takes .then(), and its assertion cannot pass on the synchronous path.

Verification

Gate Result
react · website · grid-core · row-model · core · ui 1705 · 608 · 169 · 719 · 9 · 99 — all green (react re-run post-rebase)
typecheck · lint · prettier · build · api:check clean; no report change
Hand probe, unmocked, outside the suite narrowed 1 row survives an invalid prop (bytes unchanged), warns once, then a valid query → 3 rows

The probe deliberately baselines on a narrowing query so the surviving count cannot be confused with a cleared one, and it was mutation-checked: disarming the guard makes both probe cases fail.

🤖 Generated with Claude Code

blove and others added 3 commits August 30, 2026 22:24
The derivations spec deferred this as a different design question. Measuring
showed it is smaller: only the query PROP on update is fatal (rows 3 to 0,
bytes 8702 to 0). Both grid.setQuery paths are already correct — controlled
reports intent and stops, uncontrolled throws catchably to its caller — so
they get pins, not changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An invalid `query` arriving on the prop after mount threw out of the layout
effect that reconciles it, escaping the commit and unmounting the live grid
(measured on 59835a4: rows 3 to 0, bytes 8702 to 0). It is now a rejected
write — the row model keeps the query it already had, the grid stays
interactive, and the fault is reported once per distinct fault via warnOnce.

The guard lives inside `applyQuery`, not around its call sites, because there
are two: the closure runs synchronously when no derivations transition is
pending and from a `.then()` callback when one is. Only the synchronous path
showed the fatal signature; both are covered and both are tested.

The mechanism itself is extracted as `reportRejectedWrite` and shared with the
derivations guard from #550, which was byte-identical in executable logic — the
name check, the rethrow, the cast, the three field reads and the warnOnce key
construction — so a fix to one no longer misses the other. It is parameterized
on a SET of accepted error names rather than one literal, because `setRows`,
the remaining unguarded call on this seam, would need two.

Two behaviours that were already correct are pinned, not changed: an invalid
query at mount still throws, and an uncontrolled `grid.setQuery` still throws
synchronously to its caller.

#550's comment claiming an open unguarded-`setQuery` hazard is now false and is
rewritten in both places it appears; the error-name pin's header and triage
table are rescoped from one guard to two.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
pretable Ignored Ignored Aug 31, 2026 5:26am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

Vercel preview ready

Preview: https://pretable-70c1zi5o8-cacheplane.vercel.app
Commit: 05127c835f1fce9160c2bd9df262294b7840d540

Updated automatically by the deploy-preview job.

@blove
blove merged commit 3ca63d2 into main Aug 31, 2026
21 checks passed
@blove
blove deleted the blove/invalid-query-seam branch August 31, 2026 05:43
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.

1 participant