Skip to content

Cs 10652/introduce the cardthumbnail inside cardinfofield#4341

Open
lucaslyl wants to merge 19 commits intomainfrom
CS-10652/introduce-the-cardthumbnail-inside-cardinfofield
Open

Cs 10652/introduce the cardthumbnail inside cardinfofield#4341
lucaslyl wants to merge 19 commits intomainfrom
CS-10652/introduce-the-cardthumbnail-inside-cardinfofield

Conversation

@lucaslyl
Copy link
Copy Markdown
Contributor

@lucaslyl lucaslyl commented Apr 7, 2026

linear: https://linear.app/cardstack/issue/CS-10652/introduce-the-cardthumbnail-inside-cardinfofield

Fix cyclic deps:

  1. Move FileDef, ImageDef, and NumberField into card-api; add cardThumbnail field to CardInfo
  2. Consolidate definitions into card-api: FileDef, ImageDef, and NumberField have been moved into card-api.gts as their canonical home. file-api.gts and image-file-def.gts now re-export from card-api via shim modules to preserve backward compatibility for existing consumers.
  3. Update canonical module URLs: Since FileDef and ImageDef are now canonically defined in card-api, all test fixtures, query filters, and module references that previously pointed to file-api or image-file-def have been updated to point to card-api.
  4. Add cardThumbnail field to CardInfo

Demo:

Screen.Recording.2026-04-09.at.5.33.24.PM.mov

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Preview deployments

@lucaslyl lucaslyl force-pushed the CS-10652/introduce-the-cardthumbnail-inside-cardinfofield branch from 24941df to 5d71e37 Compare April 7, 2026 09:18
@cardstack cardstack deleted a comment from chatgpt-codex-connector bot Apr 7, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Host Test Results

2 194 tests  ±0   2 179 ✅ ±0   2h 34m 7s ⏱️ + 12m 4s
    1 suites ±0      15 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 0529689. ± Comparison against base commit b5cd4a5.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Realm Server Test Results

  1 files  ±0    1 suites  ±0   14m 46s ⏱️ + 1m 16s
844 tests ±0  844 ✅ ±0  0 💤 ±0  0 ❌ ±0 
915 runs  ±0  915 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0529689. ± Comparison against base commit b5cd4a5.

♻️ This comment has been updated with latest results.

@lucaslyl lucaslyl force-pushed the CS-10652/introduce-the-cardthumbnail-inside-cardinfofield branch from f681244 to 485ed43 Compare April 9, 2026 04:40
@lucaslyl
Copy link
Copy Markdown
Contributor Author

lucaslyl commented Apr 9, 2026

@codex review

@lucaslyl lucaslyl self-assigned this Apr 9, 2026
@lucaslyl lucaslyl requested a review from a team April 9, 2026 09:44
@lucaslyl lucaslyl marked this pull request as ready for review April 9, 2026 09:45
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8747621238

ℹ️ 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".

static atom: BaseDefComponent = Atom;
static fitted: BaseDefComponent = Fitted;
}
export { ImageDef as default } from './card-api';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Re-export ImageDef as named export from image-file-def

image-file-def previously exposed ImageDef as a named export, but this shim now exports only default. Any existing card/module that still does import { ImageDef } from 'https://cardstack.com/base/image-file-def' will fail module loading with a missing export error, which breaks the stated backward-compatibility goal for the shim and can break existing realms at runtime.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates FileDef, ImageDef, and NumberField into packages/base/card-api.gts (to break cyclic deps), updates canonical FileDef module references from file-api to card-api, and introduces a new cardThumbnail relationship on CardInfo to support a linked thumbnail image.

Changes:

  • Move FileDef, ImageDef, and NumberField implementations into card-api.gts, converting file-api.gts, image-file-def.gts, and number.gts into shim/re-export modules.
  • Add CardInfo.cardThumbnail (links to ImageDef) and update CardDef.cardThumbnailURL to prefer the linked image URL with fallback to the legacy string field.
  • Update query filters / fixtures / index assumptions and test expectations to use card-api as the canonical FileDef module reference.

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/runtime-common/realm-index-query-engine.ts Default FileDef adoptsFrom module updated to card-api.
packages/runtime-common/constants.ts baseFileRef now points at card-api instead of file-api.
packages/realm-server/tests/search-prerendered-test.ts Test query filter updated to card-api FileDef.
packages/realm-server/tests/realm-endpoints/search-test.ts Test filters updated to card-api FileDef.
packages/realm-server/tests/prerendering-test.ts Prerender test fixture updated to card-api FileDef.
packages/realm-server/tests/indexing-test.ts Indexed doc fixture updated to card-api FileDef.
packages/host/tests/unit/services/render-service-test.ts File meta adoptsFrom fixture updated to card-api.
packages/host/tests/integration/store-test.gts File meta adoptsFrom fixtures updated to card-api.
packages/host/tests/integration/resources/search-test.ts Search filters updated to card-api FileDef.
packages/host/tests/integration/resources/search-data-test.ts Data search filter updated to card-api FileDef.
packages/host/tests/integration/realm-querying-test.gts Querying tests now use card-api FileDef refs.
packages/host/tests/integration/realm-indexing-test.gts Expected dependency references updated for moved implementations/new imports.
packages/host/tests/integration/components/prerendered-card-search-test.gts Query filter updated to card-api FileDef.
packages/host/tests/integration/components/file-def-serialization-test.gts Serialization expectation updated to card-api FileDef.
packages/host/tests/helpers/interact-submode-setup.gts Dynamic import adjusted for image-file-def default export.
packages/host/tests/cards/file-query-card.gts Query card’s filter updated to card-api FileDef.
packages/experiments-realm/image-def-playground.gts Import ImageDef now comes from card-api.
packages/experiments-realm/file-search-playground.gts Query filters updated to card-api FileDef.
packages/base/webp-image-def.gts Updated ImageDef import to match new default export shim.
packages/base/svg-image-def.gts Updated ImageDef import to match new default export shim.
packages/base/png-image-def.gts Updated ImageDef import to match new default export shim.
packages/base/number.gts Shim now re-exports NumberField + helpers from card-api.
packages/base/jpg-image-def.gts Updated ImageDef import to match new default export shim.
packages/base/image-file-def.gts Converted to shim re-exporting ImageDef from card-api.
packages/base/gif-image-def.gts Updated ImageDef import to match new default export shim.
packages/base/file-api.gts Converted to shim re-exporting FileDef and related helpers/types from card-api.
packages/base/default-templates/card-info.gts Adds cardThumbnail editor UI and adjusts thumbnail placeholder logic.
packages/base/card-api.gts New canonical home for FileDef, ImageDef, NumberField, and adds CardInfo.cardThumbnail + updated cardThumbnailURL behavior.
packages/base/avif-image-def.gts Updated ImageDef import to match new default export shim.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

static atom: BaseDefComponent = Atom;
static fitted: BaseDefComponent = Fitted;
}
export { ImageDef as default } from './card-api';
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image-file-def.gts used to export a named ImageDef class, but now the shim only re-exports it as the module default. That breaks existing consumers that import { ImageDef } from https://cardstack.com/base/image-file-def. To preserve backward compatibility, re-export ImageDef as a named export as well (and optionally keep the default export).

Suggested change
export { ImageDef as default } from './card-api';
export { ImageDef, ImageDef as default } from './card-api';

Copilot uses AI. Check for mistakes.
@field cardThumbnailURL = contains(MaybeBase64Field, {
computeVia: function (this: CardDef) {
return this.cardInfo.cardThumbnailURL;
return this.cardInfo.cardThumbnail?.url ?? this.cardInfo.cardThumbnailURL;
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new cardThumbnailURL computeVia reads this.cardInfo.cardThumbnail?.url. Accessing a linksTo field triggers lazilyLoadLink() when the relationship is not loaded, which will start an async store.loadFileMetaDocument() fetch even though this computeVia can’t await it and will usually fall back to cardThumbnailURL anyway. This can introduce unnecessary background loads/N+1 behavior during serialization/prerendering. Consider avoiding the linksTo getter here (e.g., inspect the raw stored relationship value first, or only use cardThumbnail?.url when the linked ImageDef is already loaded).

Suggested change
return this.cardInfo.cardThumbnail?.url ?? this.cardInfo.cardThumbnailURL;
let rawCardThumbnail = getDataBucket(this.cardInfo).get('cardThumbnail');
let loadedThumbnailURL =
rawCardThumbnail &&
typeof rawCardThumbnail === 'object' &&
'url' in rawCardThumbnail &&
typeof rawCardThumbnail.url === 'string'
? rawCardThumbnail.url
: undefined;
return loadedThumbnailURL ?? this.cardInfo.cardThumbnailURL;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a really bad suggestion. it should be ok to pull on a linksTo field in the computed. we will only lazily load it if this field is included in a template, which is exactly the behavior that we want. we should NEVER reach directly into the low level data bucket for a card. that is not a userland API. ignore this review comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with it. Sometimes Copilot gives feedback that feels a bit aggressive, which is not the behavior we want.

Copy link
Copy Markdown
Contributor

@burieberry burieberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the amount of code added to the card-api to a minimum, you can place any large component or helpers/etc that doesn't cause cyclic error outside of the card-api. You'll see we're doing this with a bunch of templates in /default-templates.

@lucaslyl lucaslyl requested a review from a team April 10, 2026 13:31
@lucaslyl
Copy link
Copy Markdown
Contributor Author

@habdelra I’ve confirmed that all tests pass locally, but they seem to be flaky during CI checks (failing randomly on each rerun attempt). Do you have any idea what might be causing this

@habdelra
Copy link
Copy Markdown
Contributor

habdelra commented Apr 10, 2026

@habdelra I’ve confirmed that all tests pass locally, but they seem to be flaky during CI checks (failing randomly on each rerun attempt). Do you have any idea what might be causing this

thanks for flagging this. off the top of my my head no. but claude is pretty good at figuring this out (i usually use thinking "high" for this kind of stuff). you can point out the failing tests to claude, and it might not be a bad idea for it to see it in context with the passing tests, so that it can determine if there is state leakage. you can also ask claude to add additional logging to help diagnose the flakiness in CI, and then just feedback the logs from CI back into claude for analysis. generally i find that this kind of approach solves most of the flaky issues

@habdelra
Copy link
Copy Markdown
Contributor

habdelra commented Apr 10, 2026

another thought based on some of the failures you are getting is that there is a memory leak. it might be worthwhile to have claude do a flame chart analysis using chrome mcp on test suites that have tests that time out at the end (that is indicative of an out of memory error). like this:

    ---
        message: >
            Error: Browser timeout exceeded: 60s
            Error while executing test: Integration | serialization > base cards > BigIntegerField: can perform bigint operations with computed
            Stderr: 
             [7101:7101:0410/141631.148233:ERROR:base/memory/shared_memory_switch.cc:289] Failed global descriptor lookup: 7
            
            DevTools listening on ws://127.0.0.1:46331/devtools/browser/2a535e2f-8636-41d1-b8a8-ea974f1be0fc
            [7028:7072:0410/141735.402808:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
            Created TensorFlow Lite XNNPACK delegate for CPU.
            [7028:7072:0410/141803.881843:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
            [7028:7072:0410/141859.309882:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
            [7028:7072:0410/142041.643824:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
            [7028:7072:0410/142419.606458:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
            [0410/142840.790381:ERROR:third_party/crashpad/crashpad/util/file/file_io_posix.cc:145] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq: No such file or directory (2)
            [0410/142840.790447:ERROR:third_party/crashpad/crashpad/util/file/file_io_posix.cc:145] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq: No such file or directory (2)
            [7028:7072:0410/143002.840930:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
            
            
        browser log: |
            {"type":"error","text":"Error: Browser timeout exceeded: 60s"}
            {"type":"error","text":"Error while executing test: Integration | serialization > base cards > BigIntegerField: can perform bigint operations with computed"}
            {"type":"error","text":"[7101:7101:0410/141631.148233:ERROR:base/memory/shared_memory_switch.cc:289] Failed global descriptor lookup: 7\n\nDevTools listening on ws://127.0.0.1:46331/devtools/browser/2a535e2f-8636-41d1-b8a8-ea974f1be0fc\n[7028:7072:0410

also make sure you have merged upstream from main to to make sure there arent any fixes that you are missing

@lucaslyl
Copy link
Copy Markdown
Contributor Author

thanks for flagging this. off the top of my my head no. but claude is pretty good at figuring this out (i usually use thinking "high" for this kind of stuff). you can point out the failing tests to claude, and it might not be a bad idea for it to see it in context with the passing tests, so that it can determine if there is state leakage. you can also ask claude to add additional logging to help diagnose the flakiness in CI, and then just feedback the logs from CI back into claude for analysis. generally i find that this kind of approach solves most of the flaky issues

Thanks, I appreciate the suggestions. I’ll look into this following your approach and see what I find

);
assert.strictEqual(
hassan.relationships['cardInfo.theme'].links.self,
null,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the assert pattern because this response combines cards serialized from two different paths: local test-realm cards (data) and cross-realm included cards (included). For optional null relationships like cardInfo.cardThumbnail, the local path may omit the key while the cross-realm included payload may still include it, even though both represent valid equivalent state. That made full-object deepEqual brittle and caused false failures based on JSON shape differences rather than behavior. The new assertion style checks the required semantics (IDs, links, required relationships like cardInfo.theme, included-card presence, and key metadata) without over-constraining optional relationship encoding.

@lucaslyl lucaslyl requested review from burieberry and habdelra April 13, 2026 07:52
store.resetCache();
let renderStore = getService('render-store');
renderStore.resetCache({ preserveReferences: true });
renderStore.resetCache();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@habdelra I fixed the intermittent 60s CI timeout by removing cross-test reference retention during test teardown. Based on Codex’s analysis, the root cause was that preserveReferences: true kept store reference counts between tests. In large serialization suites, that state accumulated over time, increasing memory/GC pressure and causing late-run browser stalls/timeouts.

I don’t have much knowledge of the store and loader, does this fix suggestion make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants