feat(ui): scope LiveSearch results to the page locale - #2480
Conversation
🦋 Changeset detectedLatest commit: 714b0f4 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 |
@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: |
There was a problem hiding this comment.
This PR solves a real, well-scoped problem: LiveSearch sends requests to search endpoints that already support locale, but the component had no way to supply it, so multilingual sites got duplicated cross-language results. The approach is sound and fits EmDash’s existing component pattern.
What I checked:
- The
localeprop is optional, defaults to"", and is omitted from the query string when unset, preserving existing behavior. - The prop is forwarded for both
/api/searchand/api/search/suggestviaURLSearchParams. - The server schemas and handlers already validate
localeand filter by it, including resolving configured locale casing. - The client-side
Configinterface and default config are updated consistently. - Documentation and a minor changeset are included.
- No AGENTS.md violations: no SQL changes, no new auth/state/logged-out queries, no new hard-coded admin UI strings, no problematic comments.
The change is small and correct. The author correctly notes that the existing test harness only covers live-search-routing; plumbing this prop through to a query parameter is straightforward enough that the absence of a new custom-element fetch test is acceptable.
One thing I cannot verify statically: whether the linked Discussion (#2479) has maintainer approval. I’m assuming good faith on that since the PR includes it and ticks the checkbox.
|
Good catch, and you were right to flag it — the answer is no, #2479 has no maintainer approval. I opened it myself an hour before this PR, and it currently has no comments or labels. I've corrected the description: the checkbox is now unticked and there's a note explaining the situation. I ticked it originally because a Discussion was linked, which is not what the box asks. My mistake, and thanks for not letting it pass silently. The order here is backwards relative to CONTRIBUTING.md. I wrote the change first because I hit the problem on a live bilingual site and needed it working, so the PR shows code that has been running in production rather than a proposal. If you'd rather keep the process strict, I'm happy to close this and reopen once #2479 is approved — the branch isn't going anywhere. |
Both /_emdash/api/search and /_emdash/api/search/suggest already accept a locale query parameter, but LiveSearch had no way to pass one. On a multilingual site every entry therefore came back once per language: a French visitor searching from a French page saw each result twice, and the second one opened its English page. The prop is forwarded to whichever endpoint the component is using. Leaving it unset keeps the current all-locales behaviour, so existing sites are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows the maintainer's call on discussion emdash-cms#2479: rather than an opt-in prop, `LiveSearch` now reads `Astro.currentLocale` and forwards it as the `locale` query parameter, so results match the language of the page the search runs on. The prop stays, as an override, and `locale={null}` restores the previous all-locales behaviour. Sites without Astro's `i18n` configured are unaffected: `Astro.currentLocale` is undefined there, so no parameter is sent. The changeset now leads with the behaviour change, as asked. Adds a container render test covering the three paths: no i18n config (no locale sent), explicit prop (forwarded), and `null` (no locale sent). The `Astro.currentLocale` default itself is not covered there — the test container has no i18n configuration to derive a locale from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ad7979d to
714b0f4
Compare
What does this PR do?
/_emdash/api/searchand/_emdash/api/search/suggestboth filter on alocalequery parameter, butLiveSearchnever sent one. On a site with translated content the dropdown returned every entry once per locale.On my bilingual site (FR canonical, EN under
/en/), searchingendlessfrom a French page returned:Half the list is in the wrong language and duplicates the other half. Clicking the English result opens the French page, because the slug redirects to its translation.
This PR now implements option 2 from Discussion #2479, per @ascorbic's call (comment: "I'd go with 2, and highlight the changed behaviour in the changeset"). The first revision of this PR added an opt-in
localeprop; it now defaults to the page's own locale instead.Behaviour change
LiveSearchreadsAstro.currentLocale— which Astro derives from the URL — and forwards it to whichever endpoint it is using. Results now match the language of the page the search runs on.Who is affected:
i18nconfigured that relied on cross-locale results will get fewer results than before.locale={null}restores the old behaviour. This is called out at the top of the changeset.i18nare unaffected:Astro.currentLocaleisundefinedthere, so nolocaleparameter is sent and the search still spans everything.The earlier revision of this PR documented
locale={Astro.locals.locale}, which was wrong — that is a convention of my own theme's middleware, not an Astro API. @ascorbic caught it in the Discussion; the docs and the component's JSDoc now sayAstro.currentLocale.Discussion: #2479
Type of change
Checklist
pnpm buildthenpnpm typecheckpass (0 errors)pnpm lintpasses (oxlint --type-aware --deny-warnings, 0 warnings)pnpm testpasses — fullpackages/coresuite: 472 files, 5867 tests, 0 failures, pluspnpm test:repro(6 files, 29 tests)pnpm formathas been run (oxfmt+prettier --checkclean on the touched files)Note on the last box: when this PR was first opened, #2479 had no maintainer response and the box was left unticked with a note saying so. It is ticked now because @ascorbic has since responded twice and chosen this design — not because a link exists.
Tests
packages/core/tests/repro/live-search-locale.render.test.tsrenders the component throughAstroContainerand asserts the serialized config, covering three paths:i18nconfiguration → no locale sent (single-language sites unchanged)localeprop → forwardedlocale={null}→ no locale sent (cross-locale search)What that test does not cover: the
Astro.currentLocaledefault itself, since the test container has noi18nconfiguration to derive a locale from. That path is verified on a live bilingual site (FR canonical, EN under/en/), where/rendersfrand/en/…rendersen, including underi18n.routing: "manual".AI-generated code disclosure
Notes
The branch was rebased onto current
main(it was 39 commits behind), hence the force-push. The option-2 change is a separate commit on top of the original one, so the diff between the two revisions stays readable.