Repository navigation
feat: filter the content list by taxonomy term - #3046
eisenbruch wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 934bb36 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 |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 506 lines across 11 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. |
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. |
@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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@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: |
Visual regression: 10 screens changedThese admin screens render differently on this PR. Confirm every change below is intended before accepting.
Maintainers: if every change above is intended, comment Measured head: |
|
Two notes on the CI run, to save a reviewer the digging:
|
github-actions
Bot
added
needs-rebase
and removed
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds termFilters to the content list API and admin UI, aligned with the shape approved in Discussion #3055. The approach is sound: termFilters is a sibling to fieldFilters (avoiding name collisions), applies OR within a taxonomy and AND across taxonomies via correlated EXISTS subqueries, and is mirrored in countWithResolvedFilters so cursor totals stay stable. The handler correctly rejects taxonomies not attached to the collection instead of silently ignoring them, and the changeset documents the new parameter and failure behavior well.
I checked the diff, AGENTS.md, and related code. The core implementation is clean and the new tests cover the junction semantics. Two admin issues need fixing before merge: FilterBar does not treat term filters as active filters, so the clear button can be hidden and won't clear term filters; and TermFilters lowercases taxonomy labels before passing them to Lingui, which is English-specific and harms other locales. TermFilters also indents hierarchical terms with literal non-breaking spaces, which is not RTL-safe per AGENTS.md. The verbose justifying comment in the handler can be trimmed once the changeset/PR cover it.
Conclusion: approach LGTM, but fix the admin filter-state handling and i18n/RTL issues.
Findings
-
[needs fixing]
packages/admin/src/components/ContentList.tsx:888-904hasActiveFilterignorestermFilterandhandleClearnever resets it. As a result, when only a term filter is selected theClear filtersbutton (rendered fromhasActiveFilterat line 986) is hidden, and when other filters are present the button leaves the term filter applied after clearing everything else.Add a helper to check for active term selections and reset them in
handleClear:const isTermFilterActive = (filter: TermFilterState) => Object.values(filter).some((slugs) => slugs.length > 0); const hasActiveFilter = statusFilter !== "all" || authorFilter !== "" || !!dateFilter.from || !!dateFilter.to || isBylineFilterActive(bylineFilter) || isTermFilterActive(termFilter ?? {}); const handleClear = () => { onStatusFilterChange("all"); onAuthorFilterChange?.(""); onDateFilterChange?.(EMPTY_DATE_FILTER); // Clearing drops the selection but keeps the inferred-byline // preference, which is a display choice rather than an active filter. onBylineFilterChange?.({ ...EMPTY_BYLINE_FILTER, includeInferred: bylineFilter.includeInferred, }); onTermFilterChange?.({}); }; -
[needs fixing]
packages/admin/src/components/TermFilters.tsx:74-75Lowercasing taxonomy labels before passing them into Lingui placeholders is English-specific and makes the message wrong for locales where taxonomy names keep capitals (or follow other casing rules). The translators only see the already-lowercased placeholder value.
Drop the
.toLowerCase()calls and let the original label be the placeholder:allLabel={t`All ${def.label}`} ariaLabel={t`Filter by ${def.label}`} -
[needs fixing]
packages/admin/src/components/TermFilters.tsx:123Hierarchical indentation is done with literal non-breaking-space characters inside the option text. That is physical indentation and breaks RTL layout expectations: AGENTS.md requires RTL-safe logical Tailwind (
ps-*,padding-inline-start, etc.) in admin UI.Use an element with logical start padding instead, for example:
{options.map(({ term, depth }) => ( <Select.Option key={term.id} value={term.slug}> <span className={depth > 0 ? `ps-${Math.min(depth * 4, 12)}` : undefined} style={depth > 0 ? { paddingInlineStart: `${depth * 1}rem` } : undefined} > {term.label} </span> </Select.Option> ))} -
[suggestion]
packages/core/src/api/handlers/content.ts:553-556This comment is justification addressed to the reviewer rather than a future reader of the code. The same rationale and failure choice already appear in the PR description and the changeset, which are the right places for it. Once this block is merged, the behavior is clear from the validation code itself; trim the comment to the single line that states the invariant, or remove it.
e13a7f6 to
dca23a1
Compare
|
Rebased onto
The rebase conflicted only on the imports in |
dca23a1 to
8520217
Compare
|
CI on
|
8520217 to
16f91a9
Compare
A collection's list can be filtered by status, author, byline, date range, free text and any indexed custom field, but not by a taxonomy term — in the admin or over the API — even where a taxonomy is attached and every entry carries one. Adds `termFilters`, keyed by taxonomy name and valued by term slugs: OR within a taxonomy, AND across taxonomies. That mirrors `fieldFilters`, which ANDs its keys, and matches how the controls read in a UI, where two terms of one taxonomy widen a search and two taxonomies narrow it. Resolution is an EXISTS subquery per key, applied in both `findMany` and `countWithResolvedFilters` so totals and pages agree. Two details the junction imposes, both of which match nothing rather than erroring when got wrong: `content_taxonomies.entry_id` stores the ec_* row's translation_group rather than its id, unlike the byline junction alongside it; and it points at `taxonomies.translation_group`, so a term matches through any locale variant unless the list is locale-scoped. A taxonomy not attached to the collection is rejected as a validation error rather than ignored, and a key that resolves to no slugs matches nothing rather than degrading to "no filter". Both are deliberate: a silently dropped filter returns a complete, plausible, unfiltered list with a 200, which is the hardest kind of wrong answer to notice. Refs emdash-cms#3043
Adds one dropdown per taxonomy applied to the collection, beside the status, author and byline filters, driving the `termFilters` parameter added to the content list in the previous commit. Nothing is configured per site. A collection already declares which taxonomies apply to it, so the controls are derived from that: a collection with no taxonomy renders nothing new, and one that gains a taxonomy gains its filter. Terms load per taxonomy and only for the ones on screen, so a site with several does not pay for all of them at once. A hierarchical taxonomy is flattened depth-first and indented, matching how the entry editor's picker reads. "No selection" is represented by the absence of a key rather than an empty array, at every layer. An empty array means "match nothing" to the server, and an untouched control must not mean that. Refs emdash-cms#3043
applyTermFilters had been inserted between applyBylineFilter's doc comment and the method, so the byline docs sat on the wrong function. Also tidies punctuation and spelling in the new comments and changeset.
…nt terms with logical padding
|
Rebased onto current Checked on the rebased branch: |
16f91a9 to
934bb36
Compare
|
Hi! There is a new feature process in place: https://github.com/emdash-cms/emdash/blob/main/proposals/README.md Could you please submit a Design PR first? Thank you! |
|
Thanks. Design PR is up as #4087, a retrospective feature plan for this one. |
|
This PR is waiting on its author because it is still a draft, and it has had no activity for 14 days. It will be closed in 7 days unless there is activity: push an update or leave a comment to keep it open. |
|
Still active: this stays a draft while its design PR, #4087, waits for review, as |
What does this PR do?
Adds
termFiltersto the content list and a dropdown per applied taxonomy in the admin, so a collection organized by a taxonomy can be filtered by it, in the screen editors work in, and over the API.Related: #3043 (closed in favor of Discussion #3055)
Why this shape
fieldFiltersalready ANDs its keys over indexed columns. Terms are not columns, so this is a sibling parameter rather than an extension of that one: it keeps "column" and "term" distinguishable, and avoids a collision when a field and a taxonomy share a name.OR within a taxonomy, AND across taxonomies, which is how the controls read: two terms of one taxonomy widen a search, two taxonomies narrow it.
Core
One
EXISTSsubquery per key, applied in bothfindManyandcountWithResolvedFiltersso totals and pages agree.Two details the junction imposes, both of which match nothing rather than erroring when got wrong, so they are worth stating:
content_taxonomies.entry_idstores the ec_* row'stranslation_group, not its id, unlike_emdash_content_bylines.content_idalongside it, which stores the row id.taxonomies.translation_group, so a term matches through any of its locale variants unless the list is locale-scoped.Two deliberate failure choices
Both are the same principle: a filter that silently matches everything returns a complete, plausible, unfiltered list with a 200, which is the hardest kind of wrong answer to notice.
VALIDATION_ERROR, not an empty result and not a no-op.At the API-client and UI layers the same idea appears as: "no selection" is the absence of a key, never an empty array.
Admin
Nothing is configured per site. A collection already declares which taxonomies apply to it, so the controls derive from that: a collection with no taxonomy renders nothing new, and one that gains a taxonomy gains its filter. Terms load per taxonomy and only for those on screen. A hierarchical taxonomy is flattened depth-first and indented, matching the entry editor's picker.
Not included
Sorting by a term, and multi-select within a taxonomy. Both fit the shape (the parameter already takes an array), but neither is needed for the filtering case and I would rather not guess at the UI for them.
Discussion
Discussion: #3055. I opened this PR before it, which was the wrong order; the Discussion sets out the shape, the two failure choices, how this relates to #1928 and #2799, and a separate question about unknown query parameters being stripped.
Type of change
Checklist
pnpm typecheckpasses:packages/coreandpackages/adminboth clean.packages/adminhas one pre-existing error insrc/components/editor/ordered-list.ts, present identically on a cleanorigin/mainin my environment.pnpm lintpasses:oxlint --type-awareclean on every changed filepnpm testpasses (or targeted tests for my change): see the note belowpnpm formathas been runuseLingui; nomessages.pochanges includedAI-generated code disclosure
Screenshots / test output
The new control, on a collection with the
categorytaxonomy applied, and opened to show that taxonomy's terms. Captured onpnpm ux:site:startwith theeditorial-smallprofile.Verified against a running site (
pnpm ux:site:start,editorial-small, patched core and admin), which is the more useful evidence anyway:The admin request carries it correctly:
…&termFilters=%7B%22category%22%3A%5B%22opinion%22%5D%7D→ 200.Worth recording that the middle case is exactly the bug being fixed: before rebuilding core, that same request returned all three rows, because the unknown parameter was dropped silently.
Unit tests: seven added, covering OR within a taxonomy, AND across taxonomies, an empty filter matching nothing, an unknown slug matching nothing, totals staying stable across cursor pages, and combining with an indexed field filter. They seed
content_taxonomiesby hand precisely to pin the two junction details above.On the wider suite, stated plainly rather than hidden:
tests/unit tests/databasereports 36 failed files / 458 failed tests on this branch, and exactly the same 36 / 458 on a cleanorigin/mainin my environment, from what looks like a localbetter-sqlite3native module problem. Same run: 3,954 passing here against 3,947 on main, i.e. the seven added. I could not get a green baseline to compare against, so I am flagging it rather than claiming green.tests/database/repositories/content.test.tsalso passes on Node 22.23.2 (77 passed), after a follow-up commit that movesapplyTermFiltersbelowapplyBylineFilterso the byline method keeps its own doc comment.