-
Notifications
You must be signed in to change notification settings - Fork 12
Match type filters through re-export spellings via canonical code refs #5800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
lukemelia
wants to merge
2
commits into
main
Choose a base branch
from
cs-12168-type-filtered-search-expands-the-adoption-chain-for-card
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
115 changes: 115 additions & 0 deletions
115
packages/host/tests/unit/query-canonicalization-test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| import { module, test } from 'qunit'; | ||
|
|
||
| import { | ||
| canonicalizeFilterRefs, | ||
| rri, | ||
| type ResolvedCodeRef, | ||
| type Filter, | ||
| } from '@cardstack/runtime-common'; | ||
|
|
||
| const CANONICAL_FILE_DEF: ResolvedCodeRef = { | ||
| module: rri('https://cardstack.com/base/card-api'), | ||
| name: 'FileDef', | ||
| }; | ||
| const REEXPORT_FILE_DEF: ResolvedCodeRef = { | ||
| module: rri('https://cardstack.com/base/file-api'), | ||
| name: 'FileDef', | ||
| }; | ||
| const PERSON: ResolvedCodeRef = { | ||
| module: rri('http://example.com/person'), | ||
| name: 'Person', | ||
| }; | ||
|
|
||
| // Resolves the file-api re-export spelling to the canonical card-api ref, | ||
| // echoes already-canonical refs, and fails everything else. | ||
| async function resolve( | ||
| ref: ResolvedCodeRef, | ||
| ): Promise<ResolvedCodeRef | undefined> { | ||
| if ( | ||
| ref.module === REEXPORT_FILE_DEF.module && | ||
| ref.name === REEXPORT_FILE_DEF.name | ||
| ) { | ||
| return CANONICAL_FILE_DEF; | ||
| } | ||
| if ( | ||
| (ref.module === CANONICAL_FILE_DEF.module && | ||
| ref.name === CANONICAL_FILE_DEF.name) || | ||
| (ref.module === PERSON.module && ref.name === PERSON.name) | ||
| ) { | ||
| return ref; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| module('Unit | query-canonicalization', function () { | ||
| test('rewrites a top-level `type` ref to its canonical form', async function (assert) { | ||
| let { filter, incomplete } = await canonicalizeFilterRefs( | ||
| { type: REEXPORT_FILE_DEF }, | ||
| resolve, | ||
| ); | ||
| assert.deepEqual(filter, { type: CANONICAL_FILE_DEF }); | ||
| assert.false(incomplete); | ||
| }); | ||
|
|
||
| test('rewrites `on` refs nested through any/every/not', async function (assert) { | ||
| let input: Filter = { | ||
| any: [ | ||
| { on: PERSON, eq: { name: 'x' } }, | ||
| { | ||
| every: [ | ||
| { not: { on: REEXPORT_FILE_DEF, eq: { name: 'y' } } }, | ||
| { type: REEXPORT_FILE_DEF }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
| let { filter, incomplete } = await canonicalizeFilterRefs(input, resolve); | ||
| assert.deepEqual(filter, { | ||
| any: [ | ||
| { on: PERSON, eq: { name: 'x' } }, | ||
| { | ||
| every: [ | ||
| { not: { on: CANONICAL_FILE_DEF, eq: { name: 'y' } } }, | ||
| { type: CANONICAL_FILE_DEF }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| assert.false(incomplete); | ||
| // the input filter is not mutated | ||
| assert.deepEqual( | ||
| (input.any![1] as { every: Filter[] }).every[1], | ||
| { type: REEXPORT_FILE_DEF }, | ||
| 'input tree is left untouched', | ||
| ); | ||
| }); | ||
|
|
||
| test('an unresolvable ref stays as-given and marks the result incomplete', async function (assert) { | ||
| let bogus: ResolvedCodeRef = { | ||
| module: rri('http://example.com/nope'), | ||
| name: 'Nope', | ||
| }; | ||
| let { filter, incomplete } = await canonicalizeFilterRefs( | ||
| { every: [{ type: bogus }, { on: PERSON, eq: { name: 'x' } }] }, | ||
| resolve, | ||
| ); | ||
| assert.deepEqual(filter, { | ||
| every: [{ type: bogus }, { on: PERSON, eq: { name: 'x' } }], | ||
| }); | ||
| assert.true(incomplete); | ||
| }); | ||
|
|
||
| test('duplicate refs resolve once', async function (assert) { | ||
| let calls = 0; | ||
| await canonicalizeFilterRefs( | ||
| { | ||
| any: [{ type: REEXPORT_FILE_DEF }, { type: REEXPORT_FILE_DEF }], | ||
| }, | ||
| async (ref) => { | ||
| calls++; | ||
| return resolve(ref); | ||
| }, | ||
| ); | ||
| assert.strictEqual(calls, 1); | ||
| }); | ||
| }); |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Claude Code 🤖] Out-of-order canonicalization leaves a stale
sourceand stickily disables client reconciliation (regression introduced by this PR; non-blocking — it degrades to a safe server-only passthrough, but silently and until the next query change).The mechanism.
loadCanonicalizedFilterruns on every livemodify()(search.ts:424, beforethis.activeQuery = queryat :426). Its early-return guard (this.canonicalizedFilter?.source === filter, :269) compares only against the settled value, so while a first canonicalization for filter A is still in flight, a secondmodify()carrying filter B (a new object) passes the guard and starts a second concurrentcanonicalizeFilterRefs. This.thenthen writescanonicalizedFilter = { source: filter, … }unconditionally, so whichever promise settles last wins.The two resolutions are
loader.import(ref.module)calls: A triggers the cold import (possibly a network fetch); B, arriving later, resolves the same module from the loader cache almost immediately. So B settles first and setssource: B, then A settles and clobbers it withsource: A. Nowthis.activeQuery.filter === Bbutthis.canonicalizedFilter.source === A.The consequence.
isClientFilterEligiblerequirescanonicalized.source === filter(:712–717), so a stalesourcemakes it returnfalseanddisplayedInstancesreturns the server set untouched (:589–593). Results stay correct, but live client-side reconciliation (candidate add + localno-matchremoval) stays off until the nextmodify()whose canonicalization happens to win the race — a live-refresh realm event callsthis.search.perform(this.#previousQuery)without re-runningmodify(), so nothing re-triggers it in between. Sticky.The fix. Drop a stale result in the
.then. It fires synchronously afterthis.activeQuery = queryis set within the samemodify(), soactiveQuery.filteris already the latest by the time any resolution lands:Related. Because the guard and memo key on filter object identity, an unstable
() => querythunk that rebuilds a deeply-equal filter on each recompute re-triggers canonicalization (and a transient passthrough window) on everymodify(), even though thequerySignaturedeep-equal check at :537 already skips the redundant search. Keying eligibility offquerySignaturerather than identity would also close that gap — but the staleness guard above is the load-bearing fix.Non-blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Claude Code 🤖] Fixed in
2a58eb1006. The.thennow drops its result whenthis.activeQuery?.filter !== filter, so a late resolution (a coldloader.importsettling after a cache-warm sibling) can no longer overwritecanonicalizedFilterwith asourcethat points at a filter that is no longer active. The guard is sound because the.thenfires only afterthis.activeQuery = queryhas run synchronously within the samemodify(), so the comparison always sees the current filter.The related identity-keying note (an unstable
() => querythunk re-triggering canonicalization) is left as-is — the staleness guard makes the transient passthrough self-correct, and moving eligibility ontoquerySignatureis a larger change than this finding warrants.