Fix catalog remix leaving base-realm module refs unresolvable in the install destination#5478
Draft
richardhjtan wants to merge 4 commits into
Draft
Fix catalog remix leaving base-realm module refs unresolvable in the install destination#5478richardhjtan wants to merge 4 commits into
richardhjtan wants to merge 4 commits into
Conversation
…install destination planInstanceInstall/resolveTargetCodeRef checked base-realm membership against the symbolic https://cardstack.com/base/ URL, but a resolved codeRef.module is the base realm's real backing URL, so a base-realm instance (e.g. a Skill) was wrongly treated as needing to be copied into the install destination — producing an adoptsFrom.module relative to the new install folder instead of the actual base realm module. Also restores the @cardstack/catalog/ virtual-network mapping for live test runs specifically (resolvedCatalogRealmURL is nulled for environment === 'test', which breaks module resolution for content that imports catalog-realm commands, like the Listing card class). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 task
![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: 214355e4b1
ℹ️ 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".
Contributor
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 30m 51s ⏱️ - 5m 24s Results for commit 2ccc65a. ± Comparison against earlier commit b29ceea. Realm Server Test Results 1 files ±0 1 suites ±0 11m 13s ⏱️ -35s Results for commit 2ccc65a. ± Comparison against earlier commit b29ceea. |
…ealmURL param live-test.js already passes the catalog realm's real address into every live-test browser session via the realmURL query param (used to discover which realm to load test files from) — no need for a parallel rawCatalogRealmURL build-time config field duplicating the same skipCatalog logic. NetworkService now reads realmURL directly, the same way live-test.js's own config/environment shim already does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
network.ts already reads config.resolvedCatalogRealmURL normally — the gap was that live-test.js only patched a spread copy of the config module for its own bootstrapping loader, never the shared module object that every test's own app instance (via setupApplicationTest) imports normally. Mutating that shared object directly means every later-created app instance sees the real catalog realm address with no production runtime code touched at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
isInBaseRealm only normalized via VirtualNetwork.unresolveURL(), which requires a registered realm mapping to recognize a base-realm module — but a module ref built without one (the existing plan-install unit test uses a bare VirtualNetwork with no mappings, and this is also how base refs appear in serialized cards/specs) stays in the literal https://cardstack.com/base/ form and was wrongly treated as needing to be copied into the install destination. Check the literal form first, matching what the RealmPaths-based check did before. Co-Authored-By: Claude Sonnet 5 <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.
Summary
isInBaseRealm(used byplanInstanceInstall/resolveTargetCodeRefto decide whether a module needs to be copied into the install destination) only recognized a base-realm module via its resolved real backing URL (e.g.https://localhost:4201/base/skill, viaVirtualNetwork.unresolveURL()). A module still in its literal symbolichttps://cardstack.com/base/form — which is how base refs actually appear in serialized cards/specs, and in aVirtualNetworkwith no realm mappings registered — fell through and got wrongly treated as needing to be copied, producing a brokenadoptsFrom.modulerelative to the new install folder instead of the actual base realm module. Now checks both forms.live-test.jsalready patchedresolvedCatalogRealmURLfor its own bootstrapping loader, but only on a throwaway spread copy of the config module — not the shared module object every test's own app instance imports viasetupApplicationTest. One-line fix: mutate the shared object directly. No production runtime code (network.ts) needs to change at all.Companion fix in boxel-catalog#652 addresses the actual
adoptsFromrewrite bug this uncovered; both are needed for a catalog remix/install to correctly resolve module references end to end.Test plan
pnpm lint:typesclean inpackages/runtime-commonUnit | Catalog | Install Plan Buildersuite (13/13 pass, including the base-realm-literal-URL case flagged by Codex review)