Answer a live read's relationships without side-loading their targets - #6098
Conversation
A live card read or search assembles the read card's whole transitive link closure into `included[]`. Add the position between that and the one a prerender takes: resolve each card's relationships, so every field names the cards it points at, and side-load none of them, leaving the consumer to fetch the cards it displays. Off unless REALM_SERVER_LIVE_READS_RESOLVE_LINKS_ONLY is set, so an environment that has not set it keeps side-loading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ide-loaded' of https://github.com/cardstack/boxel into cs-12952-live-read-side-load-closure
A card read and a search each get a test for the closure they carry and one for the shape that carries none, so which one live traffic gets is visible in a diff rather than in a latency chart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ca5832702
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed the diff this branch adds on top of its base — not the base's own changes. The lens was what the early exit from the BFS skips downstream of it (the query-backed signal, the relationship rewrites, the two response caches), whether the wire shape it produces is one the host's deserializers already handle, and whether every live read got the setting. Concurrency, auth, and the prerender legs were not re-examined beyond confirming they still take the branch they took before.
No blocking issues. The mechanism holds: the query-backed signal stays correct, both cache keys are sound today, and the relationship shape is one the host already treats as not-loaded. The three asks are a missing guard, a live read the switch doesn't reach, and a test that would make the cacheability claim executable.
- The card+json response cache is process-wide while the setting is per-realm — see the comment on
#assembleCardJsoninrealm.ts. #entryHtmlResponse(getCardHtml/getFileMetaHtml) is a live read that still assembles the closure — see the comment onsearchEntriesResponseinrealm.ts.- Assert
queryBackedin the new engine-level test — see the comment on theresolveLinksOnlymodule inskip-query-backed-expansion-test.ts.
On the open question the description raises about card writes: leaving the POST/PATCH echoes assembling their closure is not a correctness problem. LinksTo.deserialize / LinksToMany.deserialize reach a cached instance through store.getCard / store.getFileMeta before consulting resourceFrom, and carry loadedValue forward when the target is absent, so a link the echo loaded stays loaded across a later closure-free GET rather than reverting to not-loaded. The asymmetry costs bytes on writes, not consistency.
Adjacent, out of scope: linkFields in the query engine's Options has no setter anywhere in the tree — the narrowing it drives in populateQueryFields and in step 2 is unreachable. Worth deleting by whoever next touches that block, not this PR.
…variant The card+html route is the third live read that assembles a closure: a fieldset naming no rendering falls back to an item, and the host's selective refresh asks for `item` directly. Without the setting it would put back exactly the closure a search had just left out. Both validators now say which shape they describe. A read that answers relationships without side-loading their targets serves a different representation of the same card at the same `indexed_at`, so without a variant every client holding a validator would be 304'd back to the shape it cached the moment the setting changed — and the response cache, keyed on that validator, would hold the two shapes under one key. Assert the query-backed signal on a document whose links were left unwalked. It survives only because the relationships are answered before the walk is left, so moving that exit earlier should go red rather than quietly make a query-backed document look retainable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…value Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lukemelia
left a comment
There was a problem hiding this comment.
I'm unsure of the consequences of intentionally fostering this kind of N+1 single card load behavior, but it's worth measuring.
yeah, this is behind a feature flag. we can coordinate between @christse and jacqueline for a good time to test this out in prod |
When the realm server answers a live card read or a live search, it does not
return only the card that was asked for. It walks that card's links
breadth-first and side-loads the whole transitive closure into
included[], soa client can render every linked field without going back to the server. A
prerender already asks for less: it skips the relationship pass outright,
because the host re-resolves every result from its own card+source read
regardless.
This adds the position between those two, and a way to put live traffic on it.
A live read can answer each returned card's relationships — so every field
names the cards it points at — and side-load none of them, leaving the consumer
to fetch the cards it actually displays.
Why the middle position rather than the prerender's
Skipping the relationship pass leaves a query-backed field with no umbrella at
all: no
links.search, no matched ids. A consumer reads that as an unansweredfield and runs the query itself. The answer to "which cards does this field
name?" is the part a client cannot cheaply recompute, and running more searches
from more clients is the opposite of the direction we want. The cards
themselves are the part a client can fetch for itself, one at a time, for the
links a template displays.
So the umbrella stays and the closure goes.
Sparse fieldsets are the other shape this could have taken, and it does not use
them. A sparse card is indistinguishable from a complete one once it is in the
store, so the store would end up holding partial cards believing they were
whole — and a store that has to go back for the rest of a card's fields has not
saved the round trip anyway.
Nothing changes unless the variable is set
REALM_SERVER_LIVE_READS_RESOLVE_LINKS_ONLY=trueputs a realm-server's livereads on the new shape. Absent, every live read assembles the closure exactly
as it does today. Absent is also what an environment that has never heard of
the variable has, so this is inert until something sets it.
A prerender is unaffected either way: it keeps asking for the narrower shape it
already asks for, through the flags it already sets.
All three live reads, not two
Three routes assemble a closure, and the setting reaches all of them: the
card+json GET, the search item leg, and the card+html GET behind
getCardHtml/getFileMetaHtml. The third is easy to miss and load-bearing —a fieldset naming no rendering falls back to an item, and the host's selective
refresh asks for
itemdirectly, so leaving it out would mean a searchreturned closure-free members and every per-member refresh put the closure
straight back.
Card writes are deliberately untouched: a POST or PATCH response still returns
the written card with its closure assembled.
Both validators say which shape they describe
A read that answers relationships without side-loading their targets serves a
different representation of the same card at the same
indexed_at. So thecard+json ETag takes its own variant and the card+html ETag its own segment —
the same job
CARD_JSON_ETAG_VARIANTalready does for a serialization change,on a value that varies per server rather than per revision.
Without that, changing the setting would leave every client holding a validator
being 304'd back to the shape it cached, and the card+json response cache —
which is keyed on that validator — holding both shapes under one key.
What the client already does with it
A relationship that names a target the document does not carry is the shape the
host reads as a link it has not loaded yet: the field deserializes to a
not-loaded value and
lazilyLoadLinkfetches the target's card+source, with aloading state on the field while it is in flight. A query-backed field's
umbrella is hydrated per-URL the same way. Both paths are the ones a prerender
exercises today, and both have acceptance coverage of a card whose
included[]was taken away rendering correctly and fetching what it needs.
On the query-backed signal
A card+json response is retained in the response cache only when its document
is not a function of a query this realm ran, since such a document changes
without its own validator moving. Two things raise that signal: a field this
realm resolved, reported by
applyQueryResults; and a peer's already-resolvedanswer arriving on a cross-realm resource, reported where that resource is
fetched. The new path leaves the walk before any cross-realm resource is
fetched, so it embeds no answer this realm did not run — the first reporter is
the only one that has anything to report, and it still runs. A test asserts
that, because it holds only while the exit sits after the relationships are
answered.
Tests
and for a search: the static link still names its target, the query-backed
field still names all three matches and keeps its search link, and the
per-item sub-entries that would each be read as a followable link come off
the wire with the resources.
it carries by default and one for carrying none under the setting. The
default-side tests are the control: they are what catches a route that stops
side-loading for some reason other than this change, which is otherwise
indistinguishable from the feature working.
Not in here
The variable is the switch, not the decision. Two things belong with flipping
it on rather than with adding it:
total bytes over the wire on a representative page, not server time alone —
moving work to the client only wins if the client does it faster than the
server did. That needs a realm with real link depth and a client to render
it.
its own, deduped per URL and in flight concurrently rather than serially, but
still one request per card. Whether that needs collapsing into one request
before this is worth turning on is a question the measurement answers.
🤖 Generated with Claude Code