Change virtual network parameters to be required#5109
Open
backspace wants to merge 8 commits into
Open
Conversation
Contributor
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 1h 51m 13s ⏱️ - 3m 20s Results for commit f0f6915. ± Comparison against earlier commit 21e2156. Realm Server Test Results 1 files ±0 1 suites ±0 8m 25s ⏱️ -19s Results for commit f0f6915. ± Comparison against earlier commit 21e2156. |
Flip SerializeOpts.virtualNetwork from optional to required. The maybeRelativeReference closures in serializeCard / serializeFileDef no longer carry no-VN branches — the type system enforces a VN at every call site instead. opts on both entry points is also tightened from optional to required. Callers that didn't supply a VN have been updated: - copy-and-edit.ts:findRelationshipPath pulls the VN off the loader - tests/helpers/adapter.ts:openFile, tests/helpers/index.gts's setCardAsSavedWithId helper, and tests/helpers/indexer.ts's serializeCard helper all assert the loader has a VN - tests/helpers/base-realm.ts wraps serializeCard / serializeFileDef so the many bare `serializeCard(t)` call sites in test bodies stay working — the wrapper supplies VN from the active loader card-service.ts:serializeCard widens its opts to `Omit<SerializeOpts, 'virtualNetwork'>` because it threads VN internally; callers shouldn't have to. file-def-manager.ts's local opts annotation gets the same Omit. serializers/code-ref.ts's serialize and codeRefAdjustments take `Omit<SerializeOpts, 'virtualNetwork'>` with their own optional VN — that surface is driven by the framework's serializer registry, not by direct callers, and field-deserialize lands there without opts. CS-11374 item 4 will tighten this once VN is threaded through the field- deserialize protocol. serializeCardResource keeps opts? optional so the recursive symbol- method path (which legitimately has no opts) continues to work. The in-place merge that combines opts + overrides is cast through `SerializeOpts | undefined` because that code path never reads virtualNetwork. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
resolveRef previously walked through `store?.virtualNetwork` internally, making the store the dependency for prefix/RRI resolution. Reshape the helper to take VN as its first parameter so the dependency at each call site is the VN itself, not the store. Every call site already has a store in scope, so the change is mechanical — replace `resolveRef(store, ref, relativeTo)` with `resolveRef(store.virtualNetwork, ref, relativeTo)`. The function's no-VN graceful-degrade fallback stays in place; CS-11374 item 2b follows to make CardStore.virtualNetwork required so the `?: VN` at the resolveRef parameter can also tighten. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Tighten the CardStore interface so every implementation must expose a VirtualNetwork. Implementations updated: - gc-card-store.ts's CardStoreWithGarbageCollection already had it - render-service.ts's CardStoreWithErrors gains a public getter that exposes its existing #virtualNetwork field - field-configuration-test.gts's DeferredLinkStore test stub gains a `virtualNetwork: new VirtualNetwork()` field - card-api.gts's FallbackCardStore.virtualNetwork getter throws when the active loader can't supply one (was `undefined` before); the loadCardDocument / loadFileMetaDocument methods that previously threw on missing VN drop their now-redundant runtime checks virtualNetworkFor's external contract stays `VirtualNetwork | undefined` — that's the documented out-of-scope item for CS-11374 (detached instances, static parsers). It wraps the getStore(instance).virtualNetwork access in try/catch so a FallbackCardStore-without-loader-VN reaches the function as undefined the way callers already expect. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`codeRefAdjustments` carried a no-VN branch reachable from the `deserializeAbsolute` path because the framework field-deserialize machinery called it without opts. Now `deserializeAbsolute` accepts the framework's store parameter and threads `store.virtualNetwork` into `codeRefAdjustments`, so the no-VN branch in the inner `resolve` helper is unreachable from any framework-driven path. Tighten `codeRefAdjustments` to require `opts.virtualNetwork` and drop the no-VN URL-join fallback. `serialize` also tightens its opts.virtualNetwork to required; the early-out when opts is missing preserves the public surface for direct callers that pass no opts. `deserializeAbsolute` has an early-out when no store is supplied — only direct test callers reach that path, and they get a passthrough. The code-ref-test.ts unit test that exercised the `serialize` no-VN path is updated to pass an empty `new VirtualNetwork()`; the assertion is unchanged because the URL-form base+ref still resolves the same way through VN. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
backspace
force-pushed
the
require-virtual-network-cs-11374
branch
from
0eba1c7 to
70424d7
Compare
backspace
changed the base branch from
remove-global-prefix-mappings-cs-10752
to
main
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70424d7214
ℹ️ 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".
Contains.serialize's primitive branch passes its opts argument to
callSerializeHook; the non-primitive branch dropped opts on the floor,
which made the recursive serializeCardResource synthesize an opts
object with no virtualNetwork (via the `{ ...opts ?? {}, overrides }`
merge). A code-ref field on the contained card then reached its
serializer with vn=undefined and `vn.toURL(doc.data.id)` threw.
Pass opts through so the recursion sees the same virtualNetwork the
top-level serializeCard supplied. This keeps the serializer's
required-VN type signature honest at runtime — no defensive no-VN
guard is needed at the serializer because the propagation chain now
matches what the type says.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ializer Propagating opts through Contains.serialize's non-primitive branch (the previous "root-cause" fix) broke several serialization tests: nested-FieldDef values stopped being isolated from the outer card's opts, so `includeComputeds`/`includeUnrenderedFields` leaked into the inner serializeCardResource recursion. Nested FieldDef computeds started serializing where they previously didn't (e.g. Person.cardTitle inside Appointment.contact), and inner CardInfoField linksTo fields started landing in the outer card's relationships block (e.g. 'cardInfo.theme', 'cardInfo.cardThumbnail') where the tests expect nothing. The Contains-doesn't-propagate-opts shape is intentional, isolating inner serialization from outer opts. Revert that change and take the bot's original recommendation: handle the no-VN case defensively at the serializer boundary. `code-ref` serializer's `serialize` and `codeRefAdjustments` accept `opts.virtualNetwork` as optional again and degrade to URL math + raw-ref fallback (matching the contract `deserializeAbsolute` already had since CS-11374 item 4). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a small followup to #5081. That removed the global mappings like
@cardstack/catalogin favour of virtual networks handling them, but it left some virtual network parameters optional. This makes them required, except for inMarkdownField, which is addressed in #5131.