Repository navigation
Conversation
Package search quoted the whole query as one FTS5 phrase with no prefix operator, so a partly typed word found nothing: `newslet` missed the Bulletin newsletter plugin while `newsletter` found it. Quote each term separately and add `*` to the last one. User input still can't form FTS5 operators, and terms with no letters or digits are dropped instead of becoming empty phrases. Part of emdash-cms#3620 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
github-actions
Bot
added
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
I checked the diff, the search route, the FTS table/tokenizer definitions (apps/aggregator/migrations/0001_init.sql and 0003_listing_projection.sql), and the existing test context. This is the right change for #3620 — the old single-phrase quoting really did defeat prefix search, and switching to per-term quoted phrases with a controlled trailing * matches the pattern used elsewhere in the repo (packages/core/src/search/query.ts).
The code is small and focused, the new regression tests exercise the reported failure, and there are no query-count or security concerns (the query is still passed as a bound ? value).
Two non-blocking observations:
quoteFtsQuerydrops lone*terms, but it does not strip*from terms that also contain word characters. An input likefoo*would become"foo*"*— a literal*inside a phrase plus a phrase-level prefix*. I can't run SQLite here to confirm, but that looks like a potential FTS5 syntax error that would break the existing "FTS-unsafe chars don't 500" invariant. Since the code already controls prefixing, sanitizing*out of user terms before quoting would remove that risk.- The regression test covers a partly typed last word inside a multi-word query. Adding a single-term test such as
q=newsletwould pin the exact reported case (a lone partly typed word) and guard against the prefix*accidentally being applied to the joined string rather than the final term.
Neither blocks merge, so I'm leaving these as suggestions.
| .filter((term) => WORD_CHAR_RE.test(term)) | ||
| .map((term) => `"${term.replace(FTS_QUOTE_RE, '""')}"`); | ||
| if (terms.length === 0) return '""'; | ||
| return `${terms.join(" ")}*`; |
There was a problem hiding this comment.
[suggestion] The function appends its own * after the last quoted term, but it doesn't remove user-supplied * characters from the term itself. An input like foo* would produce "foo*"* — a literal * inside a phrase plus a phrase-level prefix *. I can't run SQLite here to verify, but that looks like a potential FTS5 syntax error and would weaken the existing "FTS-unsafe chars don't 500" guarantee. Since the code already owns prefixing, consider stripping * from terms before quoting.
| return `${terms.join(" ")}*`; | |
| function quoteFtsQuery(raw: string): string { | |
| const terms = raw | |
| .replace(/\*/g, "") | |
| .split(WHITESPACE_RE) | |
| .filter((term) => WORD_CHAR_RE.test(term)) | |
| .map((term) => `"${term.replace(FTS_QUOTE_RE, '""')}"`); | |
| if (terms.length === 0) return '""'; | |
| return `${terms.join(" ")}*`; | |
| } |
| await expect(res.json()).resolves.toEqual({ packages: [] }); | ||
| }); | ||
|
|
||
| it("doesn't blow up on FTS-unsafe query chars (defensive quoting)", async () => { |
There was a problem hiding this comment.
[suggestion] The new regression test validates prefix matching inside a multi-word query (email newslet). Adding a single-term test for the exact reported case — e.g., q=newslet returning bulletin — would pin that a lone partly typed word now matches and guard against the prefix * being applied to the wrong part of the joined output.
| it("doesn't blow up on FTS-unsafe query chars (defensive quoting)", async () => { | |
| it("matches a single partly typed word as a prefix", async () => { | |
| await seedPackage({ slug: "bulletin", name: "Bulletin", description: "Email newsletters" }); | |
| const res = await SELF.fetch( | |
| `https://test/xrpc/${NSID.aggregatorSearchPackages}?q=${encodeURIComponent("newslet")}`, | |
| ); | |
| const body = (await res.json()) as { packages: Array<{ slug: string }> }; | |
| expect(body.packages.map((p) => p.slug)).toEqual(["bulletin"]); | |
| }); | |
emdashbot
Bot
added
review/awaiting-author
1 participant
What does this PR do?
Fixes the prefix case from #3620: a partly typed word finds nothing.
quoteFtsQuerywrapped the whole query as a single FTS5 phrase with no prefix operator, sonewsletreturned nothing whilenewsletterfound Bulletin. Porter stemming hid this for some words (analytworks) but not others.As suggested in the issue (
"foo" "bar"*), each whitespace-separated term is now quoted on its own and the last term gets*. Embedded quotes are still doubled, so user input still can't form FTS5 operators:alpha OR betastill matches nothing becauseORis a literal term. Terms with no letters or digits (for example a lone(or*) are dropped, since the tokenizer would turn them into empty phrases.One behavior change to be aware of: multi-word queries now match records that contain all the terms, rather than only the exact adjacent phrase. For example,
gallery imagenow finds a package described as "Image gallery".The remaining parts of #3620 (indexing slugs, handles and capabilities) need schema changes, so they aren't included here.
Part of #3620
Type of change
Checklist
pnpm typecheckpasses (rantsgo --noEmitinapps/aggregator)pnpm lintpasses (ran type-awareoxlint --deny-warningson the changed files)pnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. — n/a, no UI strings@emdash-cms/aggregatoris private and ignored by changesetsAI-generated code disclosure
Screenshots / test output
Not applicable (no UI change).
New tests in
apps/aggregator/test/read-api.test.ts:email newsletfinds a package described as "Email newsletters"( * ") returns 200 with no matchesThe existing operator-escaping and FTS-unsafe-character tests still pass; I updated the comment on the operator test to match the new quoting.
Without the change:
1 failed | 51 passed (52)(the prefix test). With it:52 passed (52). Full aggregator suite:12 passedfiles,326 passedtests.