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 7 commits into
Conversation
🦋 Changeset detectedLatest commit: 36e9e66 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 |
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/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: |
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
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
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
|
d087bc8 to
349a6f2
Compare
edrpls
changed the title
|
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. |
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>
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>
c4cb0e9 to
e45bf3b
Compare
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for a real bug. Moving the content picker and MCP content_list to the server-side q search introduced in #1226 is the correct architecture, and the implementation fits EmDash's patterns. The picker now uses useInfiniteQuery with keepPreviousData to avoid flashing empty lists between keystrokes, and it correctly keeps the load-more button available while searching. The MCP tool exposes q and passes it through to the handler layer, reusing the same SQL-safe repository path as the REST route.
I checked the picker refactor, the MCP schema and tool wiring, the new MCP integration test, the underlying handleContentList/ContentRepository.findMany search implementation, and the current ContentList component. The four findings from the prior review are addressed: the three ContentList issues (trusting the server total in search mode, keeping the search input visible, and showing a "no results" state instead of the empty-collection prompt for zero-result searches) are already fixed on this branch, and this PR fixes the picker's load-more-during-search issue.
Headline conclusion: The PR is clean and ready to land after addressing two minor nits — an MCP schema inconsistency and a test organization issue. No blocking logic, security, or convention problems.
| .describe("Filter by locale (e.g. 'en', 'fr'). Only relevant when i18n is enabled."), | ||
| q: z | ||
| .string() | ||
| .trim() |
There was a problem hiding this comment.
[suggestion] The q schema here accepts empty/whitespace strings, while the REST contentListQuery schema in packages/core/src/api/schemas/content.ts adds .min(1). An empty q is silently treated as "no search" by the handler (params.q?.trim()), but the two interfaces should reject/accept the same inputs. Add .min(1) after .trim() to match the REST contract.
| .trim() | |
| q: z | |
| .string() | |
| .trim() | |
| .min(1) | |
| .max(200) | |
| .optional() | |
| .describe("Case-insensitive substring search across title, name, and slug."), |
There was a problem hiding this comment.
Applied in 5c80fee — the MCP q schema now carries .min(1) after .trim(), matching contentListQuery.
Following up in 37040d3 with the regression test the constraint was missing: content_list with q: " " now asserts an error response. Verified it fails without the .min(1) (the blank q is silently accepted and treated as "no search"), so the two interfaces can't drift apart again unnoticed.
| expect(items[0]?.id).toBe(extractJson<{ item: { id: string } }>(b).item.id); | ||
| }); | ||
|
|
||
| it("content_list filters by the q search parameter", async () => { |
There was a problem hiding this comment.
[suggestion] This new test is nested inside describe("soft-delete visibility", () => { ... }), but it has nothing to do with soft-delete or trash visibility. Move it to a dedicated describe("content_list search", () => { ... }) block (or another appropriate top-level/grouped describe) so test failures and test reports accurately describe what behavior is being verified.
There was a problem hiding this comment.
Applied in 5c80fee — the search test moved out of soft-delete visibility into its own describe("content_list search") block with its own harness setup, so a failure names the behavior it covers. The file's header comment lists it alongside the other groups.
37040d3 adds a second case to that block for the blank-q rejection.
…h test Address review feedback on emdash-cms#752: - MCP `content_list` `q` now uses `.trim().min(1).max(200)`, matching the REST `contentListQuery` contract. It previously accepted empty/whitespace `q` and silently ignored it; it now rejects it, so the MCP and REST interfaces accept and reject the same inputs. - Move the `content_list` search test out of the unrelated "soft-delete visibility" describe into a dedicated "content_list search" block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The q schema gained min(1) to match the REST contract, but nothing asserted it — the constraint could be dropped without a test noticing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Merged current On the two red E2E checks from the previous run: they're the Local verification on the merged head: full |
2 participants
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: