Let a card's own live searches skip a write to a type they aren't anchored on - #6142
Conversation
The index-event type gate lived on the entry-API resource only, so every query held through `getCards` — which is what a card declares — still re-queried on every write in the realm. Lift the gate into a shared `LiveSearchTypeGate` and consult it from both resources. A write announces itself on two channels, so gating one moves nothing. The prerender_html event carries no types of its own; it is judged through the index events that produced the same rows, which did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ent-type-gate-never-reaches-a-cards-own-queries
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after the two claims the change rests on — the type-disjointness skip and the prerender_html correlation — by tracing invalidatedTypes and both channels' invalidation sets back to where the realm builds them, and by walking the anchor path from a card-rooted query through the wire translation to what the query engine compiles. It did not run the host suite, so the statements below about what the new tests would still pass with are argued from string equality in the code rather than from a run.
No blocking issues. Three non-blocking asks: a comment that misstates the channel asymmetry it exists to bridge (and the test gap that hides), a cap whose justification doesn't match its mechanism, and one aliasing question the gate newly depends on. None of them change behavior.
Recommendations
- Correct the channel-spelling comment on
stripJsonSuffix, and make at least one render-skip test cross the spelling boundary — thread onlive-search-type-gate.ts. - Give
#unrelatedURLsa recency bound, or restate the cap's justification as lifetime accumulation — thread onMAX_CORRELATED_URLS. - Say what rules out an in-place query mutation, now that the gate caches keys off
#previousQuery— thread on#adoptQueryinsearch.ts.
Adjacent, out of scope
index-writer.ts's comment on #touchedTypes says a row with no chain — "a plain file, a module" — "cannot satisfy a type-anchored filter in either state". A plain file can: file-indexer.ts stamps every file row with [<extension def>, FileDef, BaseDef] and typeCondition compiles against types, which is how the file-meta search path in search.ts finds its rows at all. The code is right — #prepareIndexRow records those chains and tombstoneEntries reads them back — so nothing is broken today. But that sentence reads as permission to stop recording exactly the chains a file-anchored live query now depends on for its wake-ups. Worth a correction from whoever next touches that file; not asked of this PR.
| // A realm event names the underlying file (`books/1.json`), which backs two | ||
| // index rows: the card instance (`books/1` — instance ids never carry the | ||
| // extension) and the file-meta row (`books/1.json` — every file gets one). | ||
| // Reducing both spellings to the extension-free one lets the two channels' | ||
| // invalidation lists be compared without knowing which row kind produced | ||
| // either. For every other file kind (`notes.md`, `book.gts`) the strip is a | ||
| // no-op and the comparison is exact. | ||
| export function stripJsonSuffix(url: string): string { | ||
| return url.replace(/\.json$/, ''); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] This describes the strip as disambiguating two row kinds under a file name both channels spell the same way. They don't spell it the same way, and that is the whole reason the strip is load-bearing.
The incremental index event never names the file. realm-index-updater.ts strips the extension before the event goes out — invalidations.map((href) => new URL(href.replace(/\.json$/, ''))) — and the realm-server endpoint test pins that: a card write and a card delete both report the extension-free URL. Only the render event names the file: prerender-html-visit.ts returns batch.invalidations, which index-writer.ts fills with the raw boxel_index.url (this.#invalidations.add(url.href)). So the record side already arrives extension-free and the check side does not; stripJsonSuffix is the only thing bridging that, not a defensive no-op.
Nothing tests the boundary. relayIndexEvent and relayPrerenderHtmlEvent in search-test.ts both default to the same ${testRealmURL}observations/1.json, so the normalization is applied to identical strings on both sides of the comparison. Remove stripJsonSuffix from all three call sites (prerenderEventCannotMatch, and both branches of #recordIndexVerdict) and the render-skip test, the no-verdict test and the withdrawal test all stay green — while in production the record side would hold …/observations/1, the check side would ask for …/observations/1.json, prerenderEventCannotMatch would answer false forever, and the second channel would silently stop gating with no test moving.
Two asks: restate the comment so it says which channel carries which spelling, and default relayIndexEvent to the extension-free form so at least one test crosses the boundary. A realm-server test that captures both events for a single write and asserts the render set (after the strip) is contained in the index set is the stronger version, but that's a follow-up, not this PR.
Regression — the comment and the coverage gap both arrive with this change. Non-blocking: the code itself is correct.
There was a problem hiding this comment.
[Claude Code 🤖] Right on both counts, and the test half was the serious one — fixed in ee9f5de.
The comment now states the asymmetry: an incremental index event names the instance, because RealmIndexUpdater strips the extension off every invalidation before the event goes out; a prerender_html event names the file, the spelling boxel_index stores. The strip is what lets one channel's list be compared against the other's, and it still reaches both rows a card's file backs, so the comment keeps that second job rather than trading one half-truth for another.
The relays now carry their own channel's spelling — relayIndexEvent defaults to observations/1, relayPrerenderHtmlEvent to observations/1.json. Neutralizing the three stripJsonSuffix call sites and rebuilding now reddens a prerender_html event over rows an index event judged unrelated is skipped and an index event that could move a member withdraws its rows from the render gate; before this commit all six stayed green.
| // Past this many remembered URLs the prerender correlation has stopped being | ||
| // a correlation and become a copy of the realm: a realm-wide module rewrite | ||
| // fans out to every row, and holding that list per live query — a dashboard | ||
| // holds several — costs more than the re-runs it saves. The realm write | ||
| // cadence this bridges is a handful of URLs per event over the seconds | ||
| // between an index pass and the render pass it spawns, so a burst reaches the | ||
| // ceiling only when the fan-out is realm-scale. Overflow clears the record | ||
| // rather than freezing it, so the gate refills from the next events instead | ||
| // of wedging on whatever filled it. | ||
| const MAX_CORRELATED_URLS = 256; |
There was a problem hiding this comment.
[Claude Code 🤖] The cap's justification describes a window the record doesn't have.
#unrelatedURLs is cleared only by forget(), by a non-disjoint event naming the URL, and by this overflow. There is no recency bound, so the set accumulates for as long as the query is mounted — not "over the seconds between an index pass and the render pass it spawns". A dashboard left open on a busy realm reaches 256 through ordinary write cadence, and the clear then drops every verdict, including the one whose render event is seconds away.
The behavior is safe in both directions (overflow clears rather than freezes, and a missing verdict re-runs), so this isn't a defect. The ask is about the sentence the next author will reason from: "a burst reaches the ceiling only when the fan-out is realm-scale" is exactly what someone raises the cap on.
Either give the record a recency bound — the URLs of the last N index events, which is all the correlation needs, since a render event trails its index event by seconds — or restate the comment as lifetime accumulation with a periodic reset. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Agreed — the comment described an intent the record does not implement. Rewritten in ee9f5de.
It now says what is true: a verdict has no expiry, so the set grows with every distinct URL written to the realm while the query is mounted, and a long-lived page on a busy realm reaches the ceiling through ordinary cadence rather than only through a realm-wide rewrite. The bound is there because the record is a per-query cache of a cheap answer and a page holding several queries should not accumulate a copy of the realm's URL list in each of them. The value is unchanged, and so is the overflow behaviour — clearing costs a window of unconditional re-runs, never a missed one.
| #adoptQuery(query: Query): void { | ||
| this.#previousQuery = query; | ||
| this.#typeGate.forget(); |
There was a problem hiding this comment.
[Claude Code 🤖] #previousQuery is the caller's object here, not a snapshot. search-entries.ts does structuredClone(query) at its equivalent site, with a comment naming the hazard: "a caller that mutates a long-lived query object in place would otherwise be compared against itself and never re-run."
That difference now decides more than an equality check. #anchors() reads #previousQuery live, but #typeKeys() resolves once and caches, and this method is the only thing that drops the cache. So a caller that mutates its query's type anchor in place leaves the gate skipping events for the previous anchor while #flushLiveRefresh performs searches against the mutated object — and modify() can't rescue it, because the mutated object compares equal to itself and the early return fires before #adoptQuery.
What rules that out for this resource? If nothing does, snapshotting here the way the entry resource does closes both halves at once.
Pre-existing aliasing, newly load-bearing. Non-blocking, and an answer is as good as a change.
There was a problem hiding this comment.
[Claude Code 🤖] Fair question, and the answer is that the alias should go — #adoptQuery now does structuredClone, in ee9f5de.
Three things are derived from this query and all three describe it as it stood at adoption: the signature it is compared by, the floors stamped on its results, and the gate's resolved anchor keys. An in-place mutation would move the filter out from under all of them, and the gate is the one that turns that into a wrong answer rather than a redundant one — skipping events for an anchor the searches are no longer running. The signature comparison happens to survive it, since it snapshots a string, but that is an accident of how the comparison is spelled and not something the gate should rest on.
The sites that read #previousQuery all want "the query this resource is running" — the live-refresh perform, the query-errors re-run, and the load telemetry — so each is more consistent against the snapshot than against a caller's mutable object. Search resource 60/60 and search-entries 38/38 stay green.
The two realm event channels spell one row's URL differently: the index event names the instance (the updater strips the extension before broadcasting), the prerender_html event names the file. The strip is what lets the correlation compare them, and the comment said the opposite. The tests relayed the file spelling on both channels, so they never crossed the difference and stayed green with the strip removed. Each relay now carries its own channel's spelling; removing the strip reddens the two correlation tests. Bound the cap's reasoning to what the record does: a verdict has no expiry, so the set grows with ordinary write cadence, not only a realm-wide rewrite. Snapshot the query rather than aliasing the caller's object, so an in-place mutation cannot move the filter out from under the signature, the floors, and the gate's resolved anchors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ent-type-gate-never-reaches-a-cards-own-queries The only conflict is the test file's import block, where both sides added imports: the throttle's destroy/setOwner/run and this branch's Owner type. Kept both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
A live search anchored on a card type cannot gain or lose a member from a write
to an unrelated type: a row only enters or leaves a type-anchored result set by
being one of the anchored types, and an incremental index event names the whole
adoption chain of every row its pass touched. The entry-API resource already
skipped those events. The resource behind
getCards— which is the surface acard actually declares its queries through — did not, so a card holding seven
live queries re-ran all seven on every write in the realm, on every client
holding them.
What this does
LiveSearchTypeGate(packages/host/app/lib/live-search-type-gate.ts) holdsthe rule both live-search resources now consult: resolve the query's type
anchors to the keys the indexer stamps rows with, and skip an event whose types
are disjoint from them. Anchors are read through each resource's own filter
grammar;
SearchResourcegets there by translating its card-rooted query intothe entry wire form, so there is one set of anchor rules rather than two.
The conservative direction is preserved throughout — an event carrying no
types, a filter that admits a card of any type, an anchor that will not resolve,
and a module rewrite that invalidates the very module the keys were resolved
through all fall back to the unconditional re-run.
The second channel
One write announces itself twice: the index event, then the
prerender_htmlevent for the render pass that index pass spawned. Gating only the first moves
nothing.
SearchResourcehas to treat the render channel as a membershiptrigger in its own right, because its projection excludes rows with an
effective error and a render error lands on that channel.
A
prerender_htmlevent carries no types of its own, so it is judged throughthe index events that produced the same rows. A render pass renders the
invalidation set of the pass that spawned it, and that pass named its types —
so a render whose every URL an index event already judged unrelated cannot move
this query either. A URL with no such verdict takes the re-run: a repair pass
for rows an earlier generation left stale, an event that outran the index event
it belongs to, and a subscription that started mid-burst all land there. An
index event that could move a member withdraws its rows from the record, since
which of them changed type is not readable off the event.
The entry-API resource keeps routing that channel through its selective
per-member refresh, which already costs nothing when the event names none of
its members, so it does not carry the correlation.
Guard
A write to a subtype still wakes a query anchored on an ancestor. The event
carries the written row's full adoption chain precisely so the containment test
stays sound: the comparison is "does the event's type set contain the query's
anchor", never the reverse.
Tests
Six cases in
search-test.tscovering both channels: the skip, an event namingthe anchored type, an event carrying no types, the subtype-wakes-ancestor
guard, a render skipped through its index event's verdict, a render with no
verdict, a re-typed row withdrawing its verdict, an unanchored filter that
gates nothing, and a query change re-resolving before it gates again.
With the gate stubbed out, five of the six fail. The sixth asserts the
unanchored filter still re-runs on everything, so it passes either way — that
is the over-gating guard.
Green locally against the test-services stack: search resource 60/60,
search-entries resource 38/38, query-field relationship status 15/15, nested
query-field rendering 8/8, Query Fields acceptance 11/11.
🤖 Generated with Claude Code