Conversation
The existing case covers a card whose links are one level deep and plural. Add the singular, two-level one — a person linking to a person who links to three pets — and assert the whole document: the target is a reference, and neither it nor its own links appear. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A card file holds the card's own resource and no linked neighbors, which is why this route strips `included` and every resolved relationship `data` from what it serializes. Building that graph first is pure cost: the searchable settle leaves the whole graph resident, so the serializer walks each link target — and each target's own targets — into an `included[]` with no reader. Ask for the document directly instead. An excluded target still emits its relationship entry, so the serialized instance is unchanged; the walk behind it is not taken. The `included` delete stays as a guard, now against a card whose own serialize hook pushes one. 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. |
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 41m 41s ⏱️ + 4m 12s Results for commit d639a0f. ± Comparison against earlier commit 23679cc. Realm Server Test Results 1 files ± 0 236 suites +1 1h 20m 30s ⏱️ - 9m 54s Results for commit d639a0f. ± Comparison against earlier commit 23679cc. |
A prerender's reads already come back links-only, so the store holds the link targets the search doc pulled rather than the card's whole graph. The walk the scope avoids is over that resident set, transitively — which is what the comment this replaces claimed more of than it should. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after one thing: whether 'none' can produce a different serialized.data than 'all' on any path this route reaches, and after what the new test actually pins. It did not run the host suite.
No blocking issues — the output-equivalence claim holds on every path this route reaches, including the nested-field ones, and nothing downstream of serializeCard reads what the walk used to build. Three non-blocking asks below, plus a red check.
Recommendations
- The new test's stated premise isn't established by the test, and the searchable walk isn't what delivers it — see the comment on
prerender-meta-test.gts. - "at any wider scope" should be "at the default
'all'scope" — see the comment onmeta.ts. includedScope: 'all'now has no production producer. After this change the repo's only twoapi.serializeCardcall sites —card-service.tsand this route — both name a scope, so'all'survives as the parameter's default and in tests. The next direct caller silently opts into the walk this PR removes. Worth considering whether the default should become'none', or be dropped so a caller must choose. Follow-up, not this PR..claude/skills/indexing-diagnostics/SKILL.mdcalibrates the numbers this changes. It thresholdscomputedCallsabsolutely (> 1000→ computed-field hot path) and againstcomputedCalls / (searchDocMs + serializeMs). Both shift downward for exactly the link-heavy cards those thresholds get used on. The description says readers should expect the shift; the skill is where they'll meet the numbers. Non-blocking.
Red check
percy/-cardstack-host is red with 2 visual changes needing review. On a change whose claim is that the serialized output is identical, that's either noise or the one signal contradicting the claim — worth confirming which before merge.
Adjacent, out of scope
The maybeRelativeReference closure this route builds from vn.toURL(...) never runs: serializeCard in card-serialization.ts spreads opts and then overwrites maybeRelativeReference with relativeReferenceFor(model) before anything reads it. Pre-existing and not implicated here — flagging it for whoever touches that call next.
Generated by Claude Code
| // link targets resident, and at any wider scope the serializer would | ||
| // walk whatever it finds there — each resident target, and |
There was a problem hiding this comment.
[Claude Code 🤖] "at any wider scope" overstates it — only 'all' takes that walk. isExcludedByIncludedScope in card-api.gts reads value.id ? includedScope !== 'all' : includedScope === 'none', so a target with an id is excluded under every scope but 'all', and on this path every resident target is saved. 'local' here would serialize exactly what 'none' does.
As written the paragraph tells the next reader that 'local' is dangerous on this path too, which is the opposite of the write path's reasoning in card-service.ts. "at the default 'all' scope" covers it.
Non-blocking; comment accuracy.
Generated by Claude Code
| // Jade links to Hassan, who links to three pets. The searchable settle | ||
| // leaves that whole graph resident, so this is the case where walking it | ||
| // would cost the most and contribute the least: a link target is a | ||
| // reference here, and its own links are not the card's to carry. | ||
| let url = `${testRealmURL}Person/jade.json`; | ||
| await visit(renderPath(url, '/meta')); |
There was a problem hiding this comment.
[Claude Code 🤖] "The searchable settle leaves that whole graph resident" is the only thing that makes this a two-level case, and it isn't the searchable walk that delivers it — that walk never reaches the pets.
friend is searchable: true, which seeds the one-hop route friend, and searchDocFromFields reads field.searchable only on the indexed card, so friend.pets is never a route. In searchable.ts an unmatched link takes the if (!matched) return { id: makeAbsoluteURL(rawValue.reference) } branch — the reference straight off the NotLoadedValue, no load. The existing can generate search doc that includes linksTo field case shows the result: friend.pets comes out as three bare { id } entries.
What pulls the pets in is Person.numOfPets, a computed reading this.pets, firing getter loads that the settle loop waits out — the category meta.ts itself calls out as bypassing the generator's collector. So the depth this test is named for rests on an incidental computed in the fixture: change numOfPets to stop reading this.pets and the test silently degrades to the one-level case can generate serialized instance already covers, still passing and still claiming "however deep the resident graph".
Since the assertion can't distinguish the scopes on its own, residency is the whole of what makes it a guard. This file already has the idiom for establishing it deliberately — can generate search doc that includes linksTo field visits /html/isolated/0 first and says that's what pulls the linked fields. Either do the same here, or name numOfPets in the comment so whoever next edits the fixture knows this test depends on it.
Non-blocking; the gap is in the test's coverage claim, not in the change.
Generated by Claude Code
2 participants
Problem
render/meta.tsbuilds the indexed document for every card, then strips it back to the card's own resource — deletingincludedand every resolved relationshipdata, because a card file holds no linked neighbors.The strip is correct. The build in front of it is waste. The searchable settle that runs first leaves link targets resident in the store, and at the default
includedScope: 'all'the serializer walks whatever it finds there — each resident target, and transitively each target's own — to assemble anincluded[]that the next four lines delete. This is the indexing path: per card, per prerender visit, per realm, scaling with how much of the graph the settle left resident.(A prerender's own reads already come back links-only, so this is the store's resident set rather than the realm's whole graph — the walk is still unbounded in depth over it, and still has no reader.)
The file already carries a narrow fix for the same hazard:
omitQueryFields: truewas justified in part because "deep-serializing the query closure intoincluded[]is what wedges a densely cross-linked realm".Fix
Ask for the document the route actually wants —
includedScope: 'none', the option added in #6161.An excluded target still emits its relationship entry (
links.self+data, the same shape the full walk's tail emits), so the serialized instance is unchanged. What changes is that the walk behind it is never taken.delete serialized.includedstays, now as a guard against a card whose ownserializehook pushes one regardless.delete relationship.datais untouched and still load-bearing — a saved target emitsdata: { type, id }at every scope.Why it's safe
dataas deep-equal across'all','local'and'none'at the unit level. At this level, the existingcan generate serialized instancecase already deep-equals the whole document for a card with three resident link targets, and stays green.depsis assembled at meta.ts:193 fromcapturedDeps+snapshotRuntimeDependencies+searchableDeps— all beforeserializeCardruns, so the walk contributes no edges.searchable.searchDocFromFields(…)on a separate path, earlier.included. There is no consumer between the assignment and the delete.Expected diagnostics shift
beginComputePass/endComputePasswrapsserializeCard, socomputedCallsandcomputedCacheHitsin the per-row diagnostics currently include computes performed while serializing link targets. Those counts will drop, andserializeMswith them. That is the point of the change, but anything reading those numbers should expect the shift rather than read it as a regression.Test
Adds the singular, two-level case the suite lacked — a person linking to a person who links to three pets — asserting the whole serialized document.
It passes before and after, which is what makes it a guard rather than a demonstration: its job is to fail if
'none'ever stops being output-equivalent.Verification
ember-tsc: clean locally🤖 Generated with Claude Code