Repository navigation
Conversation
What Changed: - Remove the registry guard that rejected `searchable: true` on blocks fields. - Route blocks fields through the same json_tree text extraction as Portable Text in FTSManager.searchValueExpr, so triggers, rebuildIndex and the searchable toggle all index Portable Text prose inside blocks. - Add an optional fieldTypes map to extractSearchableFields and a blocks branch that walks sub-fields and repeater rows for Portable Text arrays. - Add unit and integration tests, a minor changeset, and update the field-types reference and blocks guide. Why: A blocks field is the only way to give editors rich text inside multi-column or sectioned layouts, but a page whose body lives in blocks had no text in full-text search. Migration 064 is left as shipped: no searchable blocks field could exist when it ran, so there is nothing to backfill. Refs: emdash-cms#3670
What Changed: - Add `blocks` to the field editor's searchable types so the switch renders. - Stop forcing `searchable: false` when the blocks type is selected; required, unique and indexed are still forced off. - Show a note under the switch that Portable Text content inside blocks is indexed. - Add a FieldEditor test and a minor changeset for @emdash-cms/admin. Why: Core now accepts searchable blocks fields, but the field editor still hid the switch, so editors could only enable it through seeds, the API or MCP. Refs: emdash-cms#3670
🦋 Changeset detectedLatest commit: 7dd14ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
All contributors have signed the CLA ✍️ ✅ |
PR template validation failedPlease fix the following issues by editing your PR description:
See CONTRIBUTING.md for the full contribution policy. |
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.
The approach is sound. It removes the registry guard so blocks fields use the same json_tree text/alt/caption/code extraction as portableText, which covers the triggers, populateFromContent, populateRange and the searchable-toggle rebuild. Both reviewers found no runtime regression in the FTS path. The fixed-key limitation and the deferred AI-search integration are documented and consistent with the code. The remaining findings concern repository conventions, a docstring that overstates parity between the JS and SQL extractors, roadmap wording in public text, and test fixture realism. Two of them are needs_fixing under repo conventions, so the verdict is request_changes. Reviewers differed on the overall verdict: one approved with only suggestions, the other commented. Neither ran tests, builds or lint.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@emdash-cms/admin": minor | |||
There was a problem hiding this comment.
[needs fixing] This and searchable-blocks-fields.md split one feature across two package-specific changesets. .changeset/README.md requires multi-package changes to use one changeset listing all affected packages. Combine the entries with both emdash and @emdash-cms/admin in the frontmatter, retaining the usage example, indexing limitation, and field-editor behavior in the shared release description.
| * Migration `064_fts_plain_text.ts` covers the portableText case as | ||
| * shipped. Blocks fields were added later without a new migration because | ||
| * no pre-existing trigger could include a searchable blocks field (they | ||
| * could not be searchable before). If the expression changes for fields | ||
| * that already have triggers, add a new migration rather than editing | ||
| * shipped ones — migrations are forward-only. | ||
| */ |
There was a problem hiding this comment.
[needs fixing] The new paragraph narrates why this PR did not add a migration (“were added later without a new migration because…”). AGENTS.md explicitly excludes change history and justification from code comments. Keep the evergreen requirement to add a forward migration when changing expressions used by existing triggers, but remove the account of how searchable blocks were introduced.
| }); | ||
|
|
||
| it("indexes Portable Text inside repeater rows", async () => { | ||
| await repo.create({ | ||
| type: "pages", | ||
| slug: "faq", | ||
| status: "published", | ||
| data: { | ||
| title: "FAQ", | ||
| layout: [ | ||
| { | ||
| _type: "faq", | ||
| _key: "f1", | ||
| items: [{ _key: "r1", answer: prose("Refunds arrive within fortnight") }], | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| expect((await search("refunds")).items).toHaveLength(1); |
There was a problem hiding this comment.
[suggestion] This fixture does not represent a blocks value that normal content writes can store: only two_column is registered/allowed, faq is not defined, and REPEATER_SUB_FIELD_TYPES does not permit portableText. Direct repository writes bypass those validations, so this test demonstrates recursive JSON extraction rather than a supported repeater scenario. Consider replacing it with a registered/allowed block containing supported repeater sub-fields (for example, a string caption), and keep hypothetical Portable Text-in-repeater coverage clearly separate from production behavior. The preceding figure fixture should likewise register and allow its block type.
| * objects and arrays recursively (so repeater rows are covered), skips | ||
| * `_`-prefixed keys, and extracts only Portable Text arrays via | ||
| * `extractPlainText`. Plain string sub-fields are not emitted, which matches | ||
| * the SQL path (see FTSManager.searchValueExpr). |
There was a problem hiding this comment.
[suggestion] This docstring says that leaving out plain string sub-fields "matches the SQL path", but the two paths behave differently. searchValueExpr indexes any string under a text, alt, caption or code key at any depth. That includes a plain string sub-field slugged caption, and the new integration test (indexes a plain string sub-field slugged caption) relies on it. collectBlockText only emits Portable Text arrays, so the same block produces different text from extractSearchableFields than it does from FTS. Either match the SQL key rule here, or reword the comment to say the JS path is narrower than the SQL one, as the searchValueExpr docstring already says for extractPlainText.
| { slug: "layout", type: "blocks", searchable: true, validation: { allowedTypes: ["prose"] } } | ||
| ``` | ||
|
|
||
| Plain string and text sub-fields such as headings or labels are not indexed unless their sub-field slug happens to be `text`, `alt`, `caption` or `code`; a schema-aware key list is a possible follow-up. `extractSearchableFields()` also takes an optional `fieldTypes` map so it can read `blocks` fields. |
There was a problem hiding this comment.
[suggestion] "a schema-aware key list is a possible follow-up" is roadmap speculation. This file is copied verbatim into the CHANGELOG, and the same phrase also appears in docs/.../guides/blocks.mdx, docs/.../reference/field-types.mdx and the searchValueExpr docstring. Readers need the current behaviour (headings and labels are not indexed unless their slug is text, alt, caption or code), not a possible future change. I'd delete the clause in all four places and keep the follow-up in an issue.
|
I have read the CLA Document and I hereby sign the CLA |
What Changed: - Merge the core and admin changesets into one multi-package changeset. - Drop the change-history narrative from the createTriggers docstring and keep only the forward-migration rule. - Remove roadmap wording from the changeset, both docs pages and the searchValueExpr docstring; state current behavior only. - Correct the collectBlockText docstring: the JS path extracts only Portable Text arrays and is narrower than the SQL key rule. - Register and allow the figure and gallery block types in the indexing test, and replace the Portable Text-in-repeater case with a supported string caption repeater row. Why: The repository review flagged convention violations (one changeset per feature, no history in comments, no speculation in public text) and test fixtures that bypassed schema validation. Refs: emdash-cms#3670
There was a problem hiding this comment.
The approach remains sound: searchable blocks reuse the existing full-text extraction path and address the reported blocks-based body-search gap.
All five earlier findings are addressed. The changesets are consolidated, speculative roadmap wording is removed, the extraction docstring accurately distinguishes JS from SQL behavior, the migration comment states an evergreen requirement, and the integration fixtures register allowed block types with supported repeater sub-fields.
I reviewed the follow-up diff and relevant implementation, validation, tests, and documentation conventions. No new issues found in these changes. Review was static; tests, builds, and lint were not run.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
What does this PR do?
Lets a
blocksfield be markedsearchable: true, so page and post bodies built from blocks are covered by full-text search and the admin's entry search.I'm building a site on EmDash where every page body is a
layoutblocks field so editors can mix prose, two- and three-column sections and CTAs. Site search of body text is a hard requirement, and until now a blocks-based body had no text insearch(). I prototyped this as apnpm patchagainst 1.0.1 and verified it on D1 (details in #3670); this PR ports it to currentmain. @ascorbic asked for the PR in #3670 (comment).Core
SchemaRegistryno longer rejectssearchableon blocks fields.required,unique,indexedandwidgetare still rejected.FTSManager.searchValueExprtreatsblockslikeportableText: the sync triggers,rebuildIndexand the searchable toggle index every JSON string under atext,alt,captionorcodekey, so the prose of every Portable Text sub-field in every block is indexed instead of the raw JSON.extractSearchableFieldstakes an optionalfieldTypesmap (field slug to type) and walks blocks fields sub-field by sub-field, including repeater rows, extracting Portable Text arrays viaextractPlainText. Without the map a blocks array is indistinguishable from Portable Text by shape, so existing callers are unchanged.064_fts_plain_text.tsis left as shipped. Its copy ofsearchValueExpronly runs during that migration, and no searchable blocks field could exist then, so there is nothing to backfill. The "lock-step" comment increateTriggersis updated to say so.Admin
Docs and changesets
reference/field-types.mdxandguides/blocks.mdxno longer say blocks cannot be searchable and state the limitation below.emdashand@emdash-cms/admin.Known limitation. The SQL key list is fixed, so plain
string/textsub-fields such as headings or labels are indexed only when their sub-field slug happens to betext,alt,captionorcode. A schema-aware key list from the block type definitions would close that and is a possible follow-up.Follow-up.
@emdash-cms/plugin-ai-searchcallsextractSearchableFieldswithoutfieldTypes(packages/plugins/ai-search/src/documents.ts,entryText), so a searchable blocks field yields empty text for vectorize / AI search until the collection's field types are passed through. Left out of this PR to keep it to the FTS path the issue covers.Closes #3670
Type of change
Design PR: #4740 (feature plan recording the design approved on the issue; opened after this PR, per the retrospective path in
proposals/README.md).Your connection to this 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 assistance disclosure
Screenshots / test output
Field editor, adding a Blocks field to the Pages content type in
demos/simple.Tests added:
packages/core/tests/integration/search/blocks-indexing.test.ts: an entry whose only body text lives in a block's Portable Text sub-fields is found viasearch(); structural tokens (two_column,normal,span,markDefs) do not match; togglingsearchableon reindexes existing content; a draft is not found and is found after publish; update swaps the indexed word; soft delete removes the entry; a string sub-field sluggedcaptionis found while one sluggedheadingis not; Portable Text in a repeater row is found; a searchableportableTextfield still indexes and a non-searchable one does not.packages/core/tests/unit/search/text-extraction.test.ts: blocks branch ofextractSearchableFields.packages/core/tests/integration/schema/blocks-field-schema.test.ts:searchable: trueaccepted on a blocks field.packages/admin/tests/components/FieldEditor.blocks.test.tsx: the switch renders for blocks, Required does not, and save sendssearchable: true.Core search and schema suites: 381 passed, 4 skipped. Admin FieldEditor suites: 105 passed.