feat(content): filter indexed custom fields#2213
Conversation
🦋 Changeset detectedLatest commit: abd8982 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 |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 1,474 lines across 47 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. |
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a coherent, architecture-aligned implementation of indexed custom-field filtering: it stores filterable fields as real columns, validates identifiers before they reach SQL, parameterizes values, and adds coverage across repository, handler, client, plugin, and MCP surfaces. The sorting/index plumbing and admin list-column work are related but distinct concerns, so the main judgement call is whether the PR should be split or retitled.
What I checked:
- SQL safety: dynamic column refs use
sql.ref(...)aftervalidateIdentifier, filter values are parameterized,inlists are built withsql.join, and the partial expression index design is correct. No injection vectors found. - Authorization/logged-out paths:
fieldFiltersflows throughhandleContentListand plugincontent:read, both authenticated; public loader filters via the pre-existing loader path are untouched. No new anonymous hot-path queries. - Locale filtering: content-table list queries already filter by
localewhen supplied; indexed field values are per-row, so this stays correct. - Index discipline:
createFieldIndexmatches thedeleted_at IS NULLfilter used by list/count queries. - API envelope: responses remain
{ items, total, nextCursor? }.
Blocking issues found: none. Issues to fix before merge:
- A new repository test asserts
new Set(...).toHaveLength(5), which will fail because Sets havesize, notlength. - The new admin list-column formatter emits hard-coded
"—","✓", and", ", and formats dates with the browser default locale instead of Lingui — violating the AGENTS.md localization rule.
Lower-priority suggestions:
- The PR title says "filtering" but the diff ships three separate changesets/features (filtering, sorting/indexes, list columns). Consider splitting or updating the title/description per scope discipline.
- Seed validation checks
admin.listColumnsshape but not that the slugs are declared fields in the collection. createFieldIndexcould be idempotent withIF NOT EXISTSto survive partial creates or re-runs.
Overall direction is solid; the test and localization items should be addressed.
|
|
||
| expect(values(ascending)).toEqual([null, null, 1, 2, 2]); | ||
| expect(values(descending)).toEqual([2, 2, 1, null, null]); | ||
| expect(new Set(ascending.map((item) => item.id))).toHaveLength(5); |
There was a problem hiding this comment.
[needs fixing] This assertion compares new Set(...).toHaveLength(5). Set instances expose .size, not .length, so toHaveLength will see undefined and fail. The same pattern is repeated on the next line for descending.
| expect(new Set(ascending.map((item) => item.id))).toHaveLength(5); | |
| expect(new Set(ascending.map((item) => item.id)).size).toBe(5); | |
| expect(new Set(descending.map((item) => item.id)).size).toBe(5); |
| function formatListColumnValue(column: ContentListColumn, value: unknown): string { | ||
| if (value === null || value === undefined || value === "") return "—"; |
There was a problem hiding this comment.
[needs fixing] formatListColumnValue hard-codes user-facing rendering: "—" for empty/unknown values (lines 1117, 1121, 1142, 1155), "✓" for true booleans (line 1145), a literal ", " separator for multi-select (line 1142), and date.toLocaleDateString() which uses the browser locale rather than the active Lingui catalog. AGENTS.md requires all admin UI strings to go through Lingui and admin layout to be RTL-safe; a plain em-dash/checkmark may also render oddly in RTL contexts.
formatListColumnValue is currently a module-level helper without access to the Lingui t function. Make ContentListCustomCell a proper component that calls useLingui(), then pass t (or MessageDescriptors) through to the formatter. For example:
const emptyLabel = msg`—`;
const trueLabel = msg`✓`;and inside the component:
const text = formatListColumnValue(t, column, value);The multi-select join and date formatting should also use Lingui/catalog-aware helpers rather than hard-coded ", " and toLocaleDateString().
| fields[field.slug] = entry; | ||
| } | ||
|
|
||
| const configuredListColumns = collection.admin?.listColumns ?? []; |
There was a problem hiding this comment.
[suggestion] The PR title and main changeset are about indexed custom-field filtering, but this hunk and the related schema/admin code introduce a third concern: collection-configured admin list columns (and the diff also ships a separate sorting/index feature). Per AGENTS.md scope discipline, unrelated/semi-related features should not ride along on a single PR. Consider either splitting this into focused PRs or updating the title/description/changeset set so reviewers can evaluate each feature on its own.
| errors.push(`${prefix}: label is required`); | ||
| } | ||
|
|
||
| if (collection.admin !== undefined) { |
There was a problem hiding this comment.
[suggestion] Seed validation checks that admin.listColumns entries are syntactically valid field slugs, but it does not verify that each slug corresponds to a field declared in the same collection. The runtime silently drops unknown/unsupported slugs with console.warn, so a typo in a seed file would pass validation but be ignored at runtime. Keep seed validation in sync with runtime rules by checking slugs against the collection's field list here.
| const columnName = this.getColumnName(fieldSlug); | ||
| const indexName = this.getFieldIndexName(fieldId); | ||
|
|
||
| await sql` |
There was a problem hiding this comment.
[suggestion] createFieldIndex runs CREATE INDEX without IF NOT EXISTS. If the generated idx_cf_<fieldId> index already exists — from a partial migration, a retried transaction, or a manual DBA change — this throws instead of being a no-op. Adding IF NOT EXISTS makes the runtime tolerant of re-runs and matches the defensive style used elsewhere (e.g., DROP INDEX IF EXISTS in the down migration).
80b9da5 to
abd8982
Compare
github-actions
Bot
added
review/needs-rereview
1 participant
What does this PR do?
Adds typed, server-backed filtering for custom fields that are explicitly marked
indexed.The content list REST API, core client, runtime, and plugin content access contract accept AND-combined
fieldFilters. Supported conditions include exact scalar matches, missing values, membership throughin, and inclusive or exclusive numeric or textual ranges. Field names and values are schema-validated, bounded, and resolved only against indexed fields.This lets plugins and admin experiences filter complete result sets by metadata such as ticket state, priority, workflow status, or SEO score without browser-side filtering or full-table scans.
This PR is stacked on #2212, which supplies the indexed-field storage and validation contract. Both PRs can be reviewed at the same time, but #2212 should merge first; GitHub will then narrow this PR to its unique filtering commit automatically.
Addresses the structured filtering portion of #2179.
Discussion: #1717
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runScreenshots / test output
Validated on EmDash 0.31 with Node 24 in WSL2:
pnpm typecheckpnpm lint:quickpnpm format:checkpnpm --filter emdash --filter @emdash-cms/admin build