fix(admin): extend server-side content search to the content picker and MCP#752
fix(admin): extend server-side content search to the content picker and MCP#752edrpls wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: c4cb0e9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
Scope checkThis PR changes 1,912 lines across 29 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
@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/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: |
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. |
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
129448f to
08fca45
Compare
|
Rebased on upstream main ( Local test runs after rebase, on top of
Bumping out of stale-warning territory. |
b8f394c to
6704774
Compare
|
Rebased on upstream Conflicts resolved (all orthogonal to #751's now-landed total/pagination machinery — same files, adjacent additions):
Local verification against
|
There was a problem hiding this comment.
Approach judgment: This is the right fix for a real bug. Filtering search client-side against an incomplete accumulator was fundamentally broken for any collection larger than a single API page. Moving the filter to the server via a q parameter is the correct architecture, and the repository implementation is careful about SQL safety (parameterized pragma_table_info, sql.ref() for columns, LIKE escape, input truncation).
What I checked: API schema, route wiring, ContentRepository.findMany/count, listTableColumns dialect helper, admin ContentList and ContentPickerModal, MCP tool exposure, and the test coverage.
Headline conclusion: The server-side query building is clean and safe, but the admin UI integration missed three edge cases where server-side search changes the meaning of items (from "loaded pages" to "filtered result set"). These aren't catastrophic, but they break pagination and create a UX trap when a search returns zero results.
Fixes needed:
ContentListpagination denominator (effectiveTotal) should trust the servertotalin server-side search mode.- The search input must remain visible even when a server-side search returns zero results.
- The "empty collection" state should not appear for a zero-result server-side search.
ContentPickerModalshould not hide the load-more button during search.
Addresses review feedback on PR emdash-cms#752 where server-side search changed the meaning of `items` from "loaded pages" to "filtered result set", breaking pagination and creating a zero-result UX trap. - ContentList denominator now trusts the server `total` in server-side search mode (it already reflects the filtered count) instead of collapsing to the loaded page size while a query is active. - The search input stays mounted when a server-side search returns zero results, so the user can still edit or clear the query. - A zero-result server-side search shows the "no results" message rather than the "empty collection / create your first one" CTA. - ContentPickerModal keeps the load-more button visible during search, since server-side search results are paginated too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the careful review — all four UI edge cases are fixed in 37749f3:
Added three |
37749f3 to
59d9e82
Compare
Addresses review feedback on PR emdash-cms#752 where server-side search changed the meaning of `items` from "loaded pages" to "filtered result set", breaking pagination and creating a zero-result UX trap. - ContentList denominator now trusts the server `total` in server-side search mode (it already reflects the filtered count) instead of collapsing to the loaded page size while a query is active. - The search input stays mounted when a server-side search returns zero results, so the user can still edit or clear the query. - A zero-result server-side search shows the "no results" message rather than the "empty collection / create your first one" CTA. - ContentPickerModal keeps the load-more button visible during search, since server-side search results are paginated too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
edrpls
left a comment
There was a problem hiding this comment.
Automated high-effort review (/code-review high) of the server-side search change. 9 findings below as inline comments, ranked by severity: 2 correctness gaps that matter for an i18n CMS, a count/denominator mismatch, two picker state races, a perf regression on D1, a Postgres dialect-parity issue, a debounce UI flash, and one reuse cleanup. None block the core fix; the non-ASCII and custom-field ones are the most worth addressing before merge.
…spec once Two fixes from the high-effort review of PR emdash-cms#752: - ContentList: in server-side search mode the row-count line above pagination reported the loaded-items count, not the server's filtered `total`, so it contradicted the pagination denominator until every page was fetched. renderItemCount now uses `total` for the match count in server-side mode (client-side mode still uses the locally-filtered count, since `total` there reflects the unfiltered set). - ContentRepository: a searched list resolved the search predicate twice — once in findMany and once in count — each running a table-column introspection query, doubling round-trips on D1. count now accepts an optional pre-resolved searchSpec; findMany passes its own through, so the introspection runs once per request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@emdashbot — ran a high-effort multi-agent review on this branch and pushed fixes for the two low-risk findings. Summary below, with a couple of design calls that need a maintainer decision before I go further. ✅ Fixed in
|
The core server-side content search (?q=) landed via emdash-cms#1226. Extend it to two surfaces that still post-filtered in memory: - ContentPickerModal now pushes its search box to the server (search option -> ?q=), so it finds entries anywhere in a large collection instead of only the rows already loaded. Uses keepPreviousData to avoid flashing to empty between keystrokes and keeps load-more available while searching. - MCP content_list gains a q parameter, so agents search server-side rather than post-filtering a page of results. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d087bc8 to
349a6f2
Compare
Replace the manual accumulator (allItems/nextCursor mirrored from a useQuery) with useInfiniteQuery, matching the ContentList pattern. The accumulator could lose loaded pages on any background refetch (window focus, cache invalidation) and, with keepPreviousData, could fire load-more with a stale cursor against a freshly-changed search. Deriving the list straight from query pages removes both windows; search stays in the query key so changing it starts a fresh page chain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Update — scope narrowed after rebase. Since my review note above, #1226 merged an independent implementation of the core server-side content search (
That moots most of the items above. Re-mapping each: Design calls — now #1226's territory
Lower-priority findings
All inline threads have been resolved accordingly. No maintainer decision is blocking this PR anymore — it's a focused follow-up on top of #1226. 🤖 update drafted with Claude Code. |
What does this PR do?
Extends the server-side content list search (
?q=) — which shipped via #1226 — to two surfaces that still post-filtered in memory:ContentPickerModal(used when linking content from the editor) now pushes its search box to the server (searchoption →?q=) instead of filtering only the rows already loaded, so it finds entries anywhere in a large collection. It useskeepPreviousDataso the list doesn't flash to empty between keystrokes, and keeps load-more available while searching (results can span multiple pages).content_listtool gains aqparameter, so agents can search a collection server-side rather than post-filtering a single page of results.This branch was rebased onto
mainafter #1226 merged an independent implementation of the core feature. The overlapping core-search work (handler, repository, schemas,ContentList, router, API client, index migration) is now provided by #1226; this PR keeps only the picker + MCP extensions on top of it.Related: #1219, #1226
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
MCP integration tests pass, including a new case asserting
content_listfilters byq: