feat: mixed cards + files entry search with scope and client parity#5428
feat: mixed cards + files entry search with scope and client parity#5428FadhlanR wants to merge 2 commits into
Conversation
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 3h 3m 32s ⏱️ - 2m 2s Results for commit 3cde39e. ± Comparison against earlier commit 5b89f06. Realm Server Test Results 1 files ±0 1 suites ±0 11m 24s ⏱️ -45s Results for commit 3cde39e. ± Comparison against earlier commit 5b89f06. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56029a658b
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Updates the realm “entry” search pipeline to support mixed card-instance + file results in a single query (entryType: 'all'), and adjusts host + server consumers/tests to either (a) anchor queries to instances only, or (b) intentionally request mixed results while deduping dual-indexed card .json file rows via _isCardInstance: null.
Changes:
- Expand
IndexQueryEngine/RealmIndexQueryEngineto support mixed entry searching (GROUP BY (url, type), deterministic ordering, and distinct(url,type)counting). - Add a shared helper
anchorQueryToCardInstances()and update host consumers to use either anchoring (cards-only) or_isCardInstance: null(mixed-but-deduped). - Extend realm-server and host tests to cover mixed default behavior, dedup behavior, and cards-only invariants.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/realm-index-query-engine.ts | Assemble mixed entry results by branching per row (instance vs file) and pin single-URL card lookup to instance scope. |
| packages/runtime-common/query-field-utils.ts | Add anchorQueryToCardInstances() helper to force cards-only results when a query is otherwise untyped. |
| packages/runtime-common/index-query-engine.ts | Add 'all' mode to _search/search, group/count by (url,type), add deterministic type tiebreakers, and make _isCardInstance queryable as a synthetic field. |
| packages/realm-server/tests/search-entries-engine-test.ts | Add coverage for mixed default results, dedup-by-NULL filter, and cards-only anchoring behavior. |
| packages/host/tests/unit/card-search-query-builder-test.ts | Update expectations for _title term matching, dedup filter composition, and cardsOnly scoping. |
| packages/host/tests/unit/anchor-query-to-card-instances-test.ts | New unit tests for anchorQueryToCardInstances() behavior and edge cases. |
| packages/host/tests/integration/realm-test.gts | Anchor test queries to cards-only where the assertions expect card-only result sets. |
| packages/host/tests/integration/realm-querying-test.gts | Anchor the sort-by-card-display-name test to card instances to avoid mixed default interference. |
| packages/host/tests/integration/realm-indexing-test.gts | Anchor indexing-related test queries to cards-only to preserve intent under mixed default. |
| packages/host/tests/integration/components/operator-mode-ui-test.gts | Add integration coverage that cards-grid “All Cards” remains cards-only. |
| packages/host/app/utils/card-search/query-builder.ts | Switch term matching to _title and add scoping logic (cardsOnly anchor vs _isCardInstance: null dedup). |
| packages/host/app/tools/search-cards.ts | Anchor LLM-provided queries to card instances to preserve the tool’s contract. |
| packages/host/app/services/host-mode-service.ts | Add wire-level 'item._isCardInstance': null to avoid head-prefetch ambiguity with dual-indexed .json rows. |
| packages/host/app/components/operator-mode/create-listing-modal.gts | Add wire-level 'item._isCardInstance': null to dedup example-card lookups by cardUrls. |
| packages/host/app/components/card-search/panel-content.gts | Thread @cardsOnly into query building and ensure compact recents don’t double-surface card .json URLs. |
| packages/host/app/components/card-search/constants.ts | Switch A–Z sorting to _title so file rows sort meaningfully in mixed results. |
| packages/host/app/components/card-chooser/modal.gts | Ensure chooser uses cards-only search behavior and defensively anchor preselected-card queries. |
| packages/host/app/components/card-chooser/mini/index.gts | Set @cardsOnly for the mini chooser results. |
| packages/base/cards-grid.gts | Document why “All Cards” stays cards-only without a type anchor (preserves errored instances). |
| docs/cs-11775-11776-mixed-search-plan.md | Adds an internal planning document (currently includes private tracker references and merge-state notes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I believe for for "all" type of query it will only work with full text search using the "matches" filter, as there are no fields in common between file-def and card-def. we should return a 400 error if the client uses combines an "all" with something that is not a "matches" filter. |
|
Can you include the new wire format for the "all" mode? I'm having a hard time finding it the tests |
|
what happens when you mix |
|
what does |
|
[Claude Code 🤖] @habdelra answering the discussion points together, since they are related: Is there an There is no new wire member — the request format is unchanged. Searching both kinds is simply the endpoint's default now: with no type filter, a query matches On restricting mixed queries to That said — would you prefer an explicit wire member (e.g.
Agreed the current shape is confusing, and we're changing it. The key is stamped on a card |
|
so one thing we floated a bunch of times was using a type of baseDef to mean both card defs and file defs since they both inherit from this type, what did you think of that idea? |
|
(also i have claude working on a more thorough code review as we speak) |
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Full review, focused on how this lands against the query engine's architecture. TL;DR: the engine-level mechanics are right and I'd have built the _search layer the same way; the feedback is concentrated on the application layer — where kind-selection intent lives, and one engine limitation this PR works around that we should fix instead.
What lands right
The core design respects the engine's underpinnings precisely where it matters:
- Mixed scope belongs in
_search, expressed as row-kind gating (i.type INvia the OR'd branches), not as filter rewriting. Theinstance/filebranches keep their distinct error semantics, and the sharedeveryConditionmeans the data and count queries can't disagree. GROUP BY (url, type)is the correct grouping once a dual-indexed card.jsonputs two rows behind one url — and the count rewrite toCOUNT(*)over aDISTINCT (url, type)subquery correctly accounts for SQLite's single-expressionCOUNT(DISTINCT …)limit. Thetypetiebreaker added to every ORDER BY path (including the wrapped-projection variant) keeps pagination deterministic. This is exactly the discipline the two-adapter constraint demands.- Kind discrimination through the filter is sound because of how the search doc works: a card-only field filter requires an
on:type ref to resolve (which brings atypescondition that file rows can't match), and file rows simply lack card keys. The self-narrowing is a property of the schema walk, not a heuristic. - Deleting the
queryTargetsFileMetaall-or-nothing routing is a genuine simplification — a FileDef-typed filter now works through the same path as everything else. - The deployment/compat analysis in the description is thorough and honest about the hard direction.
On restricting mixed queries to matches (discussion follow-up)
I'd argue against a 400 for non-matches filters in mixed scope. The two kinds do share a queryable surface — it's just synthetic: _title (contains + sort), matches (GIN full-text), _cardType, and now the dedup key. The search sheet's own mixed query is every: [{not: {type: spec}}, {any: [{matches}, {contains: {_title}}]}, dedup] — a strict matches-only rule would reject it. The synthetic-key set effectively is the schema of a mixed search; rather than policing filters, I'd document that contract (a short section wherever the entry API wire format is documented: "these are the keys both row kinds carry; anything else narrows to one kind by construction").
On the baseDef type-anchor idea (discussion follow-up)
Conceptually attractive — one mechanism for everything — but three mechanical facts work against it today:
- BaseDef's key is in neither kind's
typesarray.getTypes(hostrender/meta.ts) explicitlybreaks atbaseRef, so instance chains end atCardDef; file rows carry[<file subtype>, FileDef](file-indexer.ts). A{type: baseDef}filter would match zero rows. Making it work needs an indexer change plus a full reindex of every realm. - Type conditions don't compose. All type refs in a query share one
jsonb_array_elements_text(types)cross-join alias, soevery: [{type: baseDef}, {not: {type: spec}}]doesn't mean what it reads as (verified against Postgres — see theanchorQueryToCardInstancesthread). A baseDef anchor would immediately collide with this. - Rows with empty
typesvanish whenever the filter references the types alias — errored instances would silently drop out of "everything" queries.
So: if we want baseDef-as-universe eventually, the typeCondition composability fix below is a prerequisite anyway. For now, an explicit wire scope (or the PR's ambient default) is mechanically cleaner.
Recommendations (details in the inline threads)
- Expose the scope on the wire (
scope: 'cards' | 'files' | 'all', default'all'). The PR already models scope internally (entryTypeScope) and pins it forcardDocument; wire callers deserve the same verb instead of filter incantations. It pinsi.typedirectly — no shared-alias heuristics, no dependence on index-stamp state, doesn't drop errored instances the way atypesanchor does, and it answers "what's the wire format for'all'?" by making the mode visible in the request. Compat cost is identical to what the PR already accepts for the new synthetic field. - Centralize the dedup idiom behind one named runtime-common export (and its wire spelling) instead of five scattered
_isCardInstance: nullliterals — which also makes the agreed_isCardSourceFilerename a one-file change. The rename's absent-as-falsesemantics need to live infieldEqFilter(polarity-aware), not thegetFieldshim — sketch in the inline comment. - File a follow-up: make
typeConditiona self-contained membership predicate (pgtypes @> '["key"]'::jsonb, sqliteEXISTS (SELECT 1 FROM json_each(types) WHERE value = ?)— a resolved per-adapter node, same pattern asJsonContains). This fixes a latent pre-existing bug (not: {type: spec}does not actually exclude Specs — verified), makes positive type conditions AND-composable, and letsanchorQueryToCardInstancesdrop its skip-heuristic and documentedany:leak. - Consider hoisting the cards-only anchor into
store.search— its doc comment already promises "instances only," and the helper's skip-on-positive-type behavior means typed (card or file) queries pass through untouched. One choke point instead of per-call-site patches, and futurestore.searchcallers can't forget. - Smaller items inline: trim the unused
markdown/isolated_htmlfromfileColumns(potentially large per-row payloads the mixed assembly never reads), and converge the duplicated file-row assembly between the new mixed branch andsearchEntriesFileMeta.
Tests
The engine tests are well-aimed (mixed default, NULL dedup, positive-anchor narrowing, and the _title sort-direction guard is a nice touch). Two gaps: an 'all' + includeErrors test (the asymmetry — errored instances in, errored files out — is load-bearing and currently only documented in a comment), and an engine-level result test for the anchored not-spec composition, which belongs with the follow-up in (3).
None of this blocks the PR conceptually — (1) and (2) are worth doing before merge since they shape the wire surface consumers will start depending on; (3)–(5) work fine as fast-follows.
|
(lol, these fable code reviews are fantastic 🤩 ) |
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Follow-up review on local search parity — the client-side counterpart of the query engine. The store runs live queries against in-memory instances for immediate response and reconciles the server's results as they arrive (SearchResource.displayedInstances in host/app/resources/search.ts, backed by runtime-common/instance-filter-matcher.ts, whose header promises it is "kept faithful to the server"). This PR teaches the server three new query idioms without teaching the client matcher any of them, and the failure mode is not "degraded instant results" — it's the reconciler actively removing correct server results.
The mechanics
displayedInstances keeps a server-returned instance only while it doesn't demonstrably fail the filter locally: 'no-match' removes it; 'unresolvable' keeps it. The matcher resolves a filter path via resolvePath, which looks fields up on the hydrated instance. A synthetic search-doc key (_title, _cardType, _isCardInstance) is not a field on any instance, so the path contributes no values and — critically — sawUnresolvable stays false, so the predicate returns 'no-match', not 'unresolvable'. The engine's getField has a shim that fabricates definitions for these keys; the matcher has no counterpart.
The three divergences this PR introduces
eq: { _isCardInstance: null }blanks a reconciled result set. Server: key absent from every instance row →IS NULL→ matches all cards. Client: no such field →'no-match'for every instance → every server result is filtered out of the display and no local candidate can be added. A liveSearchResourcequery using this PR's own documented dedup recipe renders zero rows while holding a correct server response.contains: { _title: term }does the same for recents-style term filters. The pre-PR spellingcardTitlewas a real field and evaluated correctly locally;_titleresolves to nothing →'no-match'for every instance.sort: [{ by: '_title', on: CardDef }]degrades to URL order locally.sortValueyieldsnullfor every instance → NULLS-LAST ties → id tiebreak — anddisplayedInstancesre-sorts the merged set with the local comparator, overriding the server's correct ordering. (Inline comment onconstants.ts.)
Why tests haven't caught it: today's consumers of these idioms (search sheet, recents) go over the entry API resource, which doesn't instance-match — so the breakage is latent there. But getSearchResource is precisely the surface handed to cards via @context.store, and the dedup filter is now the documented recipe for mixed-safe queries; the first card author who follows it gets a blank live query. There's also an amusing proof of how idiom-dependent this is: cards-grid's not: { eq: { _cardType: null } } accidentally agrees with the server locally (inner 'no-match' negates to 'match'), while the positive eq: { _isCardInstance: null } — same synthetic-key mechanics, opposite polarity — blanks the set.
Proposed approach
Mirror the engine's own pattern: the engine centralizes synthetic-key knowledge in its getField shim; give the matcher the exact counterpart, in the same file the engine documents them, so the next synthetic key updates both sides or neither.
- A synthetic-key resolution step ahead of
resolvePath(used bymatchEq/matchIn/matchContainsandsortValue), returning the value both sides agree the key denotes:_title→ the instance's title (cardTitlefor cards — the same value the index stamps; a FileDef's name)._cardType→ the card type's display name (what the meta route stamps)._isCardInstance→ for a hydratedCardDef, alwaysnull(the stamp exists only on a card.json's file row, which never hydrates as a CardDef) — soeq: nullmatches, restoring parity. For a hydratedFileDefthe stamp isn't derivable from the resource (it lives in the search doc, not the file's attributes), so return'unresolvable'— which the reconciler already treats as "trust the server."
- A conservative guard in
isClientEvaluable: a filter path whose head is an unshimmed underscore-prefixed key should make the query not client-evaluable (server-only passthrough). That turns "someone added a synthetic key on the server and forgot the matcher" from silently-blanked results into merely no-instant-results — the failure mode you want by default. makeInstanceComparator.sortValuetakes the same shim for_title(and_cardType) sorts.- When the agreed
_isCardSourceFilerename lands, its matcher shim should ship in the same commit as the engine-sidefieldEqFilterrewrite — the two files are one contract, and a cross-reference comment between the engine'sgetFieldshim and the matcher would make that hard to forget.
One adjacent structural note for a follow-up rather than this PR: displayedInstances dispatches a single candidate pool (cards or file-meta) by sniffing the filter's top-level type ref or the first server row. Once mixed result sets are the default, a mixed live query can only reconcile one kind, and first-row sniffing becomes order-dependent. Fine to defer — but worth capturing in the same ticket that tracks mixed-search parity, since the sheet will presumably want live file results eventually.
If it's useful I can sketch the shim as a concrete diff — it's small and self-contained (instance-filter-matcher.ts + one guard in isClientEvaluable).
habdelra
left a comment
There was a problem hiding this comment.
This PR actually breaks the local search parity in a destructive way. see the followup review comment for the details.
|
FYI, if it helps I used this skill to help guide this code review #5467 |
00f8008 to
5b89f06
Compare
|
[Claude Code 🤖] Rebased onto latest main and pushed a revised implementation addressing the review feedback. Summary of what changed since the last review: Engine — type conditions now compose (root-cause fix, not a workaround) Wire `scope` member Client search parity (the CHANGES_REQUESTED blocker) Key rename + BaseDef in the type chain Smaller items Tests (green locally on both adapters) Deployment note: requires a full reindex (stamp rename + BaseDef in `types`). There is no index-version knob and the deploy auto-reindex only fires on a boxel-ui dist checksum change, so trigger it explicitly post-deploy (`_grafana-full-reindex` or `REALM_SERVER_FULL_INDEX_ON_STARTUP=true`). Host-after-server deploy order (an old server 400s on the new `scope` member). |
Extend the realm `entry` search so one query can span card instances and
files, and address the review feedback on how kind selection and client-side
parity work.
Engine
- Replace the shared `jsonb_array_elements_text(types)` cross-join alias with a
self-contained `TypesContains` membership predicate (pg `@>`, sqlite
`json_each` EXISTS, COALESCE for NULL/absent types). Type conditions now
compose correctly: `not:{type}` genuinely excludes, `every:[{type},{type}]`
intersects, and rows with empty types survive negation.
- Support a mixed 'all' entry scope alongside 'instance'/'file'.
Wire scope
- Add `scope: 'cards' | 'files' | 'all'` (default 'all') to the entry wire
query; it pins boxel_index.type directly, independent of filter shape. The
federated-search cache key includes it. Consumers (store.search, host-mode
head prefetch, create-listing, recents, choosers) pin scope instead of a
synthetic-key incantation; `anchorQueryToCardInstances` is removed.
Client parity
- Teach the instance-filter matcher the synthetic search-doc keys (`_title`,
`_cardType`, `_isCardInstanceFile`) via a shim at the top of resolvePath, and
add an isClientEvaluable guard so an unshimmed underscore key is server-only
rather than blanking reconciled results. `friendlyCardType` moves to
runtime-common so the index stamp and matcher share one implementation.
Keys and types
- Name the dual-`.json` marker `_isCardInstanceFile` with canonical `eq: false`
(existence-test compile, adapter-agnostic); one home in search-doc-keys.ts
plus an `excludeCardInstanceFileRows()` helper.
- Append BaseDef to instance and file type chains so `{ type: baseRef }` spans
both kinds and composes.
Cleanup + docs
- cardsOnly choosers hide file types in the type picker; trim unread
markdown/isolated_html from the mixed file projection; fold
searchEntriesFileMeta into searchEntries(..., 'file').
- Document scope + the mixed-search shared-key contract in docs/search.md.
Requires a full reindex on deploy (stamp rename + BaseDef in types); there is
no index-version knob, so trigger it explicitly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5b89f06 to
3cde39e
Compare
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Follow-up review of the reworked revision. TL;DR: this is an exemplary response to review — every architectural thread from the last pass was adopted and, in several places, improved on. The remaining asks are small: one doc correction, a few comment-tightenings (inline), and one real must-do — the PR description still describes the superseded design and needs a rewrite, including its deployment section.
What this revision gets right
I verified each of these against the branch rather than taking the diff's word for it:
- Kind selection is now a first-class verb. The wire
scope: 'cards' | 'files' | 'all'member (validated with a 400 on bad values, folded into the response cache key) replaces both filter-anchor gymnastics and thequeryTargetsFileMetarouting heuristic. Internal callers and wire callers now speak the same language, and the single-URL GET pins scope by accept-header kind through the same parameter. - The dedup finally reads as what it does.
_isCardInstance: null→excludeCardInstanceFileRows()/eq: { _isCardInstanceFile: false }, compiled as an existence test (IS [NOT] NULL) — adapter-agnostic, polarity-clean, andeq: truegains a sensible meaning (select exactly the card-source.jsonrows, now tested). The rename plussearch-doc-keys.tsas the single source of the synthetic-key spellings addresses the scattered-literals concern completely. - Type conditions now mean what they say. The
TypesContainsmembership node (per-adapter rendering on theJsonContainspattern, with theCOALESCEempty-array guard) fixes all three artifacts of the shared cross-join alias:not: { type: X }genuinely excludes,everyof two type refs is an intersection, and empty-typesrows no longer vanish. The new engine tests pin each of these — including the not-spec case that was silently broken before. - BaseDef as the composable universe anchor — with the chains extended consistently in all three
getTypesimplementations, and the prerender loop smartly skipping the abstract BaseDef ancestor rendering so the index doesn't bloat. - One assembly loop.
searchEntriesFileMetais gone; the'file'scope routes through the unifiedsearchEntriespath, so file-row assembly can't drift. - Client parity is closed. The matcher shims all three synthetic keys (with
friendlyCardTypeextracted to runtime-common so the_cardTypestamp and the shim share one definition), files resolve asunresolvable(trust-the-server), and theisClientEvaluableunderscore-guard turns any future unshimmed synthetic key into "no instant results" instead of "reconciler blanks correct results". Tests cover each shim and the guard. - The projection trim, the boxel-cli scope passthrough (folding in what was a follow-up ticket), the cards-only type-picker filtering, and the
docs/search.mdcontract section all landed too.
On the clarity of "search both kinds"
That was the open design question, and I think this revision answers it well. There's now a layered, unambiguous vocabulary: scope says which row kinds a query spans (explicit 'all' for both); type refs narrow within that scope (with { type: BaseDef } as the kind-spanning anchor inside filter compositions, now that type conditions compose); excludeCardInstanceFileRows() is the one remaining special case — "each card once" in a mixed result — and its eq: false spelling plus the named helper make the intent legible at every call site. Two documentation nits keep that story crisp (inline on docs/search.md): _cardType is listed as a both-kinds key but is stamped on card rows only, and a sentence on when to reach for the BaseDef anchor vs scope: 'all' would pre-empt the "two spellings for the same thing" question.
The must-do: rewrite the PR description
The body still describes the previous design — "no wire member is added", the _isCardInstance: null idiom, anchorQueryToCardInstances, and a consumer table that no longer matches the code. Since this PR is the reference for how mixed search works, that's worth fixing before merge. In particular the deployment section needs re-deriving, because two things in this revision change the index-state story:
- The stamp rename: rows indexed under the old
_isCardInstancekey don't carry_isCardInstanceFile, so until a realm reindexes, its card.jsonfile rows pass the new dedup filter and appear as duplicates in mixed results. - BaseDef in
types:{ type: baseRef }matches nothing on rows indexed before this deploy.
Both self-heal through the deploy-triggered full reindex, but the visible window (transient dupes; BaseDef anchors under-matching) is exactly what the operational notes exist to describe — and the current text's "verify a reindex has completed since the stamping deploy" prerequisite is now obsolete (the relevant reindex is the one this deploy triggers).
Small things (inline threads)
store.search's "positive type ref selects a kind" premise now has two exceptions (BaseDef refs, mixed any:) worth naming in its comment; the fieldEqFilter special case implies a _-prefix reservation that should be stated once in search-doc-keys.ts; TableValuedEach and its SQLite adjustSQL rewrites are now dead code; a GIN index on types is the natural perf follow-up now that type filters are @>-served; and the cache key can normalize explicit scope: 'all' to match the absent default.
None of the inline items block merge on their own — the description rewrite is the one thing I'd insist on before this lands.
|
|
||
| - `matches` — full-text search over both kinds' content. | ||
| - `_title` — the row's display title (a card's `cardTitle`, a file's name); usable in `contains` and `sort`. | ||
| - `_cardType` — the card type's display name. |
There was a problem hiding this comment.
[Claude Code 🤖] One correction and one addition to this key-contract list — worth getting exactly right since this section is now the canonical answer to "how do I query across both kinds":
-
_cardTypedoesn't belong under "keys both row kinds carry" — it's stamped only during card render (render/meta.ts); file rows never get it (that asymmetry is exactly what makes cards-grid'snot: { eq: { _cardType: null } }a cards-only filter). Listing it here invites someone to sort a mixed result set on_cardTypeand wonder why every file sinks to the NULLS-LAST tail. I'd move it out of this list and describe it as a card-only synthetic — useful in mixed queries precisely because files lack it. -
A sentence on when
{ type: <BaseDef> }beatsscope: 'all'would head off the "we now have two spellings for everything" confusion:scopeis query-global (it pinsboxel_index.typefor the whole request), while a BaseDef anchor is a filter node — composable inside the tree, e.g. one branch of anany:that must span both kinds while a sibling branch is card-typed, or an intersection with another type condition. If a caller just wants "everything",scope: 'all'is the spelling; the BaseDef anchor is for kind-spanning within a filter composition.
| ? getTypeRefsFromFilter(query.filter) | ||
| : undefined; | ||
| let hasPositiveType = typeRefs?.some((r) => !r.negated) ?? false; | ||
| let scope: SearchEntryScope | undefined = hasPositiveType |
There was a problem hiding this comment.
[Claude Code 🤖] Making store.search the choke point is the right structure — every caller (search-cards tool, preselected-card query, and anything future) now inherits the pin without per-call-site edits. Two exceptions have crept into the premise "a positive type ref already selects a kind", though, both introduced elsewhere in this same PR:
{ type: baseRef }is now a positive ref that selects both kinds (BaseDef terminates both chains as of this PR). Astore.search({ filter: { type: baseRef } })therefore takes thehasPositiveTypebranch, stays'all'-scoped, and returns FileDef hydrations plus both rows of every card.json— probably not what astore.searchcaller ever wants, given the method's "instances only"-flavored contract.- A mixed
any:—any: [{ type: SomeCard }, { contains: { _title: 'x' } }]counts as positively typed, but its untyped branch still matches file rows in'all'scope.
Neither has a live caller today, so this is a comment-tightening ask, not a bug report: the doc comment should say the pass-through is for concrete card/file type refs, and name these two shapes as the known gaps. If you want to close gap 1 cheaply, treat a baseRef-equivalent ref as not kind-selecting (pin 'cards' for it) — store.search's contract arguably wants that regardless.
| polarity: FilterPolarity = 'positive', | ||
| ): CardExpression { | ||
| if ( | ||
| currentField(key) === CARD_INSTANCE_FILE_KEY && |
There was a problem hiding this comment.
[Claude Code 🤖] The existence-test compilation is the cleanest possible resolution of the null-vs-false ambiguity — eq: false now reads as what it does, IS [NOT] NULL sidesteps the pg-'true'-vs-sqlite-1 rendering divergence entirely, and polarity inverts for free. Nice.
One precedence subtlety worth a sentence in the comment: this special case keys on the leaf name (currentField(key)) and fires before any definition lookup, so a card that defines a real boolean field literally named _isCardInstanceFile gets existence semantics instead of value comparison (eq: false would fail to match a card whose field is explicitly false — the key is present). That's the opposite precedence from the getField shim below, which only fabricates a synthetic definition when the real-field lookup fails. Checking the definition first isn't practical here (this method is sync; lookup is async), and the matcher's isClientEvaluable guard already treats every unshimmed _-prefixed key as server-only — so the system has de facto reserved _-prefixed top-level keys for synthetics. I'd just make that reservation explicit: one sentence in search-doc-keys.ts (and the docs' key-contract section) saying user-defined fields must not use _-prefixed names at the search-doc top level, rather than trying to make this branch definition-aware.
| // semantics that miscompose under AND/NOT — this is a single per-row scalar | ||
| // predicate, so type conditions compose correctly (a real `NOT` exclusion, an | ||
| // AND intersection) and no GROUP BY is needed to recollapse the fan-out. | ||
| export interface TypesContains { |
There was a problem hiding this comment.
[Claude Code 🤖] This node is exactly the right shape — per-adapter rendering like JsonContains, and the COALESCE guard means an empty/never-indexed types is a definite FALSE instead of a vanished row, which fixes the third artifact of the old fan-out (rows disappearing whenever any filter branch referenced the alias). The engine tests pinning not:-exclusion and every:-intersection close out what were previously silent misfires.
Two small follow-ons, neither blocking:
TableValuedEachis now consumer-less.typeCondition,hasInstanceType, andhasFileTypewere its only emitters; what remains is the node kind, its factory, itsexpressionToSqlbranch, and — easy to forget — the SQLiteadjustSQLrewrites that existed solely for it (jsonb_array_elements_text(→json_each(, the_array_elementalias fixup, and thecase jsonb_typeof … endstrip). Dead per-adapter rewrite machinery is where bit-rot hides; worth deleting, here or as an immediate follow-up.- Postgres can now index-serve type filters. The old cross-join could never use an index;
types @> '["key"]'::jsonbcan, givenCREATE INDEX … USING gin (types jsonb_path_ops)onboxel_index(+_working). Type-anchored queries are the hottest filter shape in the system, so that migration is probably worth a ticket even if perf is acceptable today.
| // same query would collide on one ETag/body. Fold only when set (absent == | ||
| // the default 'all', so keying `undefined` would not fragment but omitting | ||
| // it keeps the key identical to pre-scope requests). | ||
| if (parsed.scope) { |
There was a problem hiding this comment.
[Claude Code 🤖] Good catch folding scope into the cache key — I'd missed that interaction entirely. One residual asymmetry: an explicit scope: 'all' is truthy, so it keys differently from an absent scope even though the two are semantically identical — same query, two cache entries/ETags. Normalizing the default away keeps the comment's own goal ("absent == the default 'all'") true for both spellings:
if (parsed.scope && parsed.scope !== 'all') {
cacheKeyOpts.scope = parsed.scope;
}The sheet's base filter was `any: [CardDef, FieldDef]`, which no file row
satisfies (file type chains run MarkdownDef → FileDef → BaseDef), so a typed
term could never surface a file even though the search runs at the mixed
scope — while picking a file type worked, because a picked type strips the
type-only base filter entirely.
- Base the sheet on `{ type: baseRef }`: BaseDef is the common ancestor
stamped on both instance and file type chains, so one ref spans cards
(including specs), field instances, and files.
- Send the wire scope explicitly: `searchScopeForOptions` now returns 'all'
for the mixed case instead of leaving scope unset.
- Root refs (BaseDef/CardDef/FieldDef/FileDef) no longer suppress the
card-json dedup filter: they span kinds rather than narrowing to one, so a
base filter built from them still needs `excludeCardInstanceFileRows()` —
otherwise every card would surface twice via its dual-indexed `.json` file
row. A picked (narrowing) type still skips the dedup.
- Add `baseFileRef` to the type picker's root set so the widened base filter
reads as unconstrained and base FileDef stays out of the picker options.
- Placeholder copy now mentions files.
Tests: new acceptance module covering files-by-term, card dedup, and spec
inclusion in the sheet; query-builder unit coverage for the root-ref dedup
rules; the chooser search test's term becomes "Mark Jackson" since word-AND
full-text over the now-included skills-realm docs also matched "Mark J".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes CS-11775, CS-11776.
What
Makes the search entry API return card instances and files from a single query, instead of routing all-or-nothing by filter type, and updates every consumer to the new mechanism so the API change and its consumers land together. Kind is discriminated entirely by the caller's filter — a card-type-anchored filter matches only
instancerows viatypes, a FileDef-anchored filter onlyfilerows — so no per-kind routing heuristic is needed and no wire member is added.API changes (CS-11775)
_searchgains an'all'mode (WHERE i.type IN ('instance','file')) beside the existing'instance'/'file'modes. Nothing is deduped by default: a dual-indexed card.json(aninstancerow and afilerow sharing oneurl) surfaces as two rows viaGROUP BY (url, type); COUNT/LIMIT/OFFSET run over the same grouping sototalstays consistent. Instance rows honorincludeErrors; file rows only surface healthy._isCardInstanceis a queryable synthetic field:eq: { item._isCardInstance: null }keeps cards + plain files and drops the card.jsonfile row (the key is stamped only on that row). The positive NULL match is load-bearing —not: { eq: { _isCardInstance: true } }would compare NULL on every other row and drop everything.search()carries the file-only columns in either projection when file rows are in scope;searchEntriesfolds the former instance and file-meta loops into one loop that branches per row onrow.type. The single-URL instance GET pins'instance'so a bare file URL never resolves as a card entry.Consumer changes (CS-11776)
Every consumer falls into one of two filter idioms:
{ type: baseCardRef }unless a positive type ref is already present (the engine shares onetypescross-join alias, so two positive type conditions cannot compose). Shared helperanchorQueryToCardInstanceslives inruntime-common/query-field-utils.ts.eq: { _isCardInstance: null }(legacy spelling) /'item._isCardInstance': null(wire spelling).query-builder.ts)_titlekey so files match by name; sheet queries compose the_isCardInstancededupcardsOnlyoption onbuildSearchQuery/buildRecentsQuery, passed as@cardsOnlythroughPanelContent; the preselected-cardstore.searchquery is anchored defensivelycardUrlsalso name the dual-indexed.jsonfile rows'item._isCardInstance': nullin the wire query (itscardUrlsmatched both rows and the file row's head could win the order tiebreak — the playwrighthead-tagsfailure){ type: baseCardRef }; the "All Files" group owns the file side. (Collateral:cards-gridwasn't in the original ticket scope — the mixed default forces it.)search-cardsAI commandWhen a positive type condition is present (a picked type or a typed base filter), neither anchor nor dedup is added: one kind is already selected, and a card
.jsonmatching an explicit file-type filter correctly surfaces once, as a file.Tests
eq item._isCardInstance nulldedups; positive card-type anchor stays cards-only; the head-scoped single-URL query mirrors the prefetch dedup._title+ scoping; implicitly cards-only realm/indexing test queries anchored; new "All Cards stays cards-only" integration test.Deployment & backward compatibility
No transitional wire mechanism ships with this change — compatibility is handled operationally. The asymmetry that drives the plan:
not: { eq: … }, or a positive type anchor all drop file rows, since file rows lack thosesearch_dockeys /types). Only filter-less/sort-only queries,cardUrlsqueries (a card.jsonURL matches both of its rows), and queries on keys files also carry (_title) see mixed rows — extrafile-metaentries that current clients already deserialize; no errors._isCardInstancesynthetic field is only queryable as of this PR, so an old realm-server rejects it as an unknown field.Prerequisite: index state (per environment, staging then prod)
Dedup relies on
_isCardInstance: truebeing stamped on every card-instance.jsonfile row (stamping landed separately with the CS-11774 indexing change; this PR only consumes it, and includes no reindex trigger). Before deploying, verify a full reindex has completed since that stamping deploy — e.g. spot-checkboxel_indexthatfile-type rows for card.jsons havesearch_doc->>'_isCardInstance' = 'true'. If not, force a full reindex and wait. Un-restamped cards appear twice in the search sheet until their realm reindexes.Deploy order
eq: { item._isCardInstance: null }and the skew window closes.Rolling back is the reverse: host first, then realm-server, so the deployment never passes through new-host + old-realm (the erroring direction). A realm-server rollback needs no index migration — the stamped key is inert to old code.
Post-deploy smoke tests
search-cardsAI command: card instances only.Client follow-up
boxel-cli's filter-less and
cardUrlssearches will show the extra file rows until it adopts the dedup filter — tracked in CS-12052, to be released together with or after the realm-server deploy (an old server rejects the new filter).🤖 Generated with Claude Code