fix(search): quote the field the query matched in snippets - #2481
fix(search): quote the field the query matched in snippets#2481DavidPivert wants to merge 2 commits into
Conversation
An FTS5 index is laid out as `id UNINDEXED, locale UNINDEXED, ...searchable fields`, so column 2 is whichever field happens to be searchable first — usually the title. snippet() was pinned to that column, so a match anywhere else returned the first field's text with no highlight: a hit in the body, an artist name or a tracklist all rendered as the bare title, telling the reader nothing about why the entry matched. Passing -1 lets FTS5 pick the column that actually matched, and a title match still quotes the title. This also explains the NULL snippets the surrounding comment described: snippet() returns NULL when the *targeted* column is empty for that row, even though the row matched elsewhere. Selecting the matched column removes that case rather than working around it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 886a499 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR fixes a real search bug: snippet() was hard-coded to FTS5 column 2 (the first searchable field), so matches in any other field returned that first field's text without highlighting. Passing -1 asks FTS5 to select the column that actually matched, which is the correct FTS5 semantics. The regression test covers both directions (body match and title match), and the SQL change is otherwise identical — no new query count, injection surface, or locale-filter change.
I found no logic bugs, security issues, or performance regressions. However, there are a handful of AGENTS.md / comment hygiene issues: the changeset is copied straight from the PR description, two existing search tests now have stale comments because the old fixed-column behavior no longer exists, and a few new comments read like PR narrative rather than evergreen code comments.
Findings
-
[needs fixing]
.changeset/fts-snippet-matched-column.md:5The changeset is the PR description pasted into release notes. AGENTS.md says a changeset is "release notes a user reads while upgrading — not a commit message, PR description, or summary of your diff" and should leave out internal mechanics (FTS5 column layout, column numbers, etc.).
Fixes search snippets so they quote the field the query matched instead of the first searchable field. -
[needs fixing]
packages/core/tests/integration/search/snippet-sanitization.test.ts:94This test name and the surrounding comments describe the old fixed-column behavior. With
snippet(..., -1, ...), a row with a NULL title that matches via thecontentcolumn now gets its snippet from the matched content column, not NULL. The test still passes as a no-crash guard, but its name and rationale are now misleading and no longer exercise the null-guard path they claim to.Rename and update the comment, e.g.:
it("does not crash when content matches while the title is NULL", async () => { // Enable a second searchable column so a row with a NULL title can // still match via content. A regression that drops the null-guard // throws "Cannot read properties of null (reading 'replace')" before // these assertions can run. -
[needs fixing]
packages/core/tests/integration/search/snippet-sanitization.test.ts:111This inline comment is also stale: the row now matches and snips from the content column, so the premise "matched via the content column so this row still surfaces in results" is the only relevant part.
// NULL title; the row must match via the searchable content field. -
[needs fixing]
packages/core/tests/integration/search/portable-text-indexing.test.ts:40This comment documents the old implementation (
snippet()pinned to FTS5 column 2). After this PR,snippet(..., -1, ...)selects the matched column, so the field creation order no longer determines which column is quoted. The comment is now stale and confusing.// These tests assert snippets contain prose from indexed Portable Text fields. -
[suggestion]
packages/core/src/search/query.ts:310The inline SQL comment narrates the rejected fixed-column alternative (
Hard-coding 2...). AGENTS.md asks for non-obvious "why" comments without recounting rejected approaches. The first line is sufficient.-- -1 selects the FTS5 column that matched the query. snippet("${sql.raw(ftsTable)}", -1, '<mark>', '</mark>', '...', 32) as snippet, -
[suggestion]
packages/core/tests/integration/search/snippet-column.test.ts:12This block reads like a PR description: it recounts the old behavior and the fix at length. AGENTS.md says comments should not be summaries of the change or narrative about rejected alternatives. A one-sentence invariant is enough.
/** * FTS5's snippet() defaults to the first searchable column unless passed -1 * to select the matched column. These tests pin both cases. */ -
[suggestion]
packages/core/tests/integration/search/snippet-column.test.ts:65Inline "before the fix" comments are historical narrative. The test name already captures the expectation.
expect(items).toHaveLength(1); expect(items[0].snippet).toContain("<mark>Mustang</mark>"); expect(items[0].snippet).toContain("Sally");
github-actions
Bot
added
review/needs-rereview
Overlapping PRsThis PR modifies files that are also changed by other open PRs: This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
1 participant
What does this PR do?
An FTS5 index is laid out as
id UNINDEXED, locale UNINDEXED, ...searchable fields, so column 2 is whichever field happens to be searchable first — usually the title.snippet()was pinned to that column:A match anywhere else therefore came back as the first field's text, unhighlighted. On my site, searching
mustangmatched an album through its tracklist and the snippet readEndless Night— the title, no highlight, no indication of why the entry matched. The same happened for hits on an artist name, a label, or the body of a post.Passing
-1lets FTS5 pick the column that actually matched. Reproduced standalone:A title match still quotes the title, so nothing regresses for the common case.
This also explains the NULL snippets the surrounding comment described —
snippet()returns NULL when the targeted column is empty for that row, "even if the row matched via a different searchable column". Selecting the matched column removes that case instead of working around it; I've adjusted the comment accordingly and kept the null guard, since an empty matched column is still possible.Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (oxlint --type-aware --deny-warnings)pnpm testpasses (targeted:tests/integration/search/— 10 files, 43 tests)pnpm formathas been runAI-generated code disclosure
Test output
tests/integration/search/snippet-column.test.tspins both directions. It fails onmainand passes with the fix:With the fix applied:
🤖 Generated with Claude Code