Conversation
Site code had no way to enumerate bylines. The public API offered only
identifier-keyed lookups, so the only option was querying a content
collection and deduping entry.data.bylines -- a list bounded by whichever
entries were fetched, and one that drops anyone with no resolvable credits
(a guest author added ahead of publication, an author whose entries are all
drafts, an author whose entries exist only in another locale).
getBylines() reads _emdash_bylines directly: one query, alphabetical by
display name, cursor-paginated on (display_name, id), with the avatar's
media columns joined in so rendering avatars costs no extra query per
byline. Locale is strict, resolved like getTaxonomyTerms. No custom-field
hydration -- customFields comes back {}, as skipHydration does elsewhere.
Opt-in, so no route's query count moves. No migration, no schema change.
Implements emdash-cms#2333
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5142e2f 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 |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR proposes a new public getBylines() runtime API and the underlying BylineRepository.findManyAlphabetical(). The implementation matches EmDash's existing patterns: cursor pagination, strict per-locale filtering via resolveLocale, media joined in a single query, and customFields deliberately omitted. The tests cover ordering, pagination, cursor tie-breaking, locale filtering, the media join, and a SQLite query-plan regression guard. The changeset and docs updates look correct.
However, the PR should not merge as-is for process reasons. AGENTS.md requires a prior maintainer-approved Discussion for any feature, and the PR description explicitly notes that Discussion #2333 has no maintainer response yet. Treating this as a proposal is the right posture, but that means the appropriate next step is maintainer approval of the Discussion before the feature lands — not code review as a merge candidate. Code quality aside, merging without that approval would violate the project's contribution policy.
On the code: I found one real robustness gap. The new public function does not degrade gracefully when the bylines table is missing, while the analogous getTaxonomyTerms() returns [] on isMissingTableError. Because bylines were added in migration 031 and the table may be absent in pre-migration or isolated database contexts, the new API should follow the same pattern. I would also suggest evaluating whether this list should use the object cache (cachedQuery under CacheNamespace.BYLINES) like getTaxonomyTerms(), since a public author index is exactly the kind of rarely-changing, locale-keyed list that benefits from cross-request caching.
Overall: technically sound proposal, but it needs the approved Discussion first and the missing-table guard before it could safely ship.
github-actions
Bot
added
the
review/awaiting-author
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
This looks good. One change before merge. Deferring the object-cache layer to a follow-up is fine.
getBylines() threw where its sibling enumeration helpers return empty:
getTaxonomyTerms returns [], and getBylinesForEntries in the same file
already catches the missing-table case. Catch isMissingTableError and
return { items: [] } so a public helper behaves like the rest.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
area/core
area/docs
size/XL
review/needs-rereview
@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: |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Two correctness issues block the advertised alphabetical list and fresh same-request reads.
…ites Ordering `findManyAlphabetical` on `display_name` used the database's own collation — BINARY on SQLite, which files `Adam, Zoe, alice, Álvaro`. Migration 059 adds `_emdash_bylines.display_name_sort`, a casefolded, accent-stripped copy of the display name written on create and update and backfilled for existing rows. The list orders and seeks its cursor on that column, so the `ORDER BY` and the cursor predicate compare the same thing, and a Latin-script key is plain ASCII, so SQLite and Postgres agree on the order. `getBylines()` is request-cached under keys carrying locale, limit, and cursor, which a write path can't reconstruct, so a read → write → read sequence within one request served the stale first result. Byline create, update, and delete now clear the whole `bylines:` namespace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
main claimed 059-064 while this branch was open, so the byline sort-key migration is renumbered 059 -> 065. Nothing about it changes but the number; none of main's new migrations touch _emdash_bylines, so running last is safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MA2153
added
needs-rebase
bot:review
There was a problem hiding this comment.
Re-review of #2352. The code-level issues from the previous review have been addressed: getBylines() now catches isMissingTableError and returns { items: [] }, matching the graceful degradation of getTaxonomyTerms(). The author explicitly links the PR to Discussion #2333 and ticks the "approved Discussion" checklist item; I cannot verify the current approval status from the repository alone, so I am treating it as claimed-resolved rather than confirmed.
What I checked:
- The new public
getBylines()runtime API, its request-cache keying, and its strict per-locale resolution viaresolveLocale(). - The new
BylineRepository.findManyAlphabetical()implementation: cursor pagination on(display_name_sort, id), media join for avatars,customFields = {}, andlimitclamping. - Migration
065_byline_display_name_sort: safe parameterized backfill, idempotentup(), guardeddown(), and batching under D1's bind-parameter ceiling. - Write-path cache invalidation (
clearRequestCachePrefix) applied increate,update, anddelete. - Type changes to
BylineTable/BylineRowWithAvatarand export wiring. - Tests covering missing-table fallback, locale filtering, cursor pagination, ordering, sort-key backfill, query plans, and write-time request-cache invalidation.
I found no logic bugs, SQL-safety issues, regressions, or AGENTS.md convention violations in the diff. The implementation follows EmDash's established patterns for cursor pagination, locale filtering, media joins, request caching, and migration discipline. If the maintainer confirms Discussion #2333 is approved, the code is ready to merge.
github-actions
Bot
added
review/awaiting-author
main claimed 065 for the media-usage collection-deletion migration, so the byline display_name_sort migration moves to 066.
|
I'm afraid It needs a migration renumbering. |
main claimed 066 for 066_media_usage_reconciliation, so the byline sort-key migration is renumbered to 067. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main claimed 067 for 067_indexed_content_fields, so the byline sort-key migration is renumbered to 068. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Renumbered |
|
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. |
2 participants
What does this PR do?
There is no way for site code to enumerate bylines. The public API offers only identifier-keyed lookups —
getByline(id),getBylineBySlug(slug)— plusgetEntriesByByline(), all of which need a byline you already have. The only site-wide enumeration,BylineRepository.findMany(), is reachable solely through the admin-gatedGET /_emdash/api/admin/bylines.So the only thing a frontend can do today is query a content collection and dedupe
entry.data.bylines. That reaches_emdash_bylinesthrough a join on_emdash_content_bylines(itself reached through a content query) to read columns sitting on the byline row, and the result is a function of which entries you fetched: bounded by the content query'slimit/cursor, and silently missing anyone with no resolvable credits — a guest author added ahead of publication, an author whose posts are all draft or scheduled, an author whose entries exist only in another locale.This adds one export:
backed by
BylineRepository.findManyAlphabetical(), a byline-table-driven read:LEFT JOIN mediaonavatar_media_id, the same joingetContentBylinesManyalready performs. Without it every caller rendering avatars does aMediaRepository.findByIdper byline — an N+1 across exactly the page being built.getTaxonomyTerms. Byline rows are per-locale, so filtering on the resolved locale yields one row per person and needs no translation-group dedupe.findManycallswithCustomFieldsunconditionally, costing extra queries against the EAV tables. A list of names and avatars doesn't need them;customFieldscomes back{}, matching the existingskipHydrationbehaviour. AnincludeCustomFields: trueflag would be additive later.findManyorders bycreated_at DESC, which is wrong for an author index. Cursor keyed on(display_name, id)via the existingencodeCursor/decodeCursor; returns{ items, nextCursor? }per the pagination convention.requestCached, keyed on every argument.Cost: one query. Opt-in, so no existing route's query count moves and the snapshots are unaffected. One additive migration (
068, a column on_emdash_bylines— see the review follow-up below), no change to existing behaviour or exports — not a breaking change.Deliberately left out, each additive later:
search/isGuestfilters, anincludeCustomFieldsflag, and adding the media join tofindManyso the admin list stops N+1-ing avatars (a separate change to an admin path that shouldn't ride along).Index behaviour
_emdash_bylineshas single-column indexes ondisplay_nameandlocale(migration040), but no composite. The locale filter is indexed; the ordering is not, so each page sorts the locale partition beforeLIMITapplies and the cursor is not a true seek. This is index-availability only — it does not depend onsqlite_stat1. At the byline counts real sites carry, sorting the partition is negligible, and a(locale, display_name_sort)composite is a forward-only migration whenever that stops being true. That belongs with #1532, which covers the same concern across taxonomy terms, bylines, users, and media.Ordering moved to
display_name_sort(below), which carries no index at all, so the plan is unchanged: the planner never usedidx_bylines_display_namefor this query either — the locale filter won, and the sort was already a temp B-tree.068deliberately adds no index, to keep the composite question in one place.Review follow-up
Pre-migration databases (#2352 (comment)). Fixed in 807c633 —
getBylines()was the odd one out:getTaxonomyTermsreturns[]andgetBylinesForEntriesfurther down the same file already catches this, whilegetBylines()propagated theno such tableerror. It now catchesisMissingTableErrorand returns{ items: [] }. Covered by a new case intests/unit/bylines/bylines-query.test.tsthat drops_emdash_bylinesand asserts the empty result; it fails with theSqliteErrorbefore the fix.Collation (#2352 (comment)). Fixed in 1dbe279. Confirmed: ordering on
display_nameused the database's own collation, BINARY on SQLite, so the list readAdam, Zoe, alice, Álvaro— andØyvindlanded afterZoetoo, sinceødoesn't decompose.Migration
068adds_emdash_bylines.display_name_sort: the display name casefolded, NFD-decomposed with combining marks dropped, and with the Latin letters that carry no mark to drop (æ œ ø ð þ ß đ ł ħ ı ŋ ŧ ĸ) expanded (utils/sort-key.ts).createandupdatewrite it, and the migration backfills existing rows in batches sized for D1's parameter ceiling, following056's pattern.findManyAlphabeticalorders on that column and seeks its cursor on it, so the two compare the same value by construction — which is the half that actually breaks pagination if they diverge.It was numbered
059when those threads were written;mainhas since claimed059–067, so successive merges have renumbered it to068. Nothing about it changed but the number.Stored rather than computed in the query, for two reasons: neither dialect can produce this key in SQL (SQLite's
lower()is ASCII-only,NOCASEfolds ASCII case and nothing else, and no dialect hasunaccentavailable on D1), and a cursor value computed in JS has to compare equal to what SQL sorted. A side effect is that the order no longer depends on the dialect's collation: a Latin-script key is plain ASCII, so SQLite and Postgres agree. Scripts outside Latin keep their letters and still sort by code point — that needs ICU, which D1 doesn't carry, and it's no worse than today.Covered by
tests/unit/database/migrations/068_byline_display_name_sort.test.ts(backfill, including a row past the first batch; a re-run against a database that already has the column;down()), plus three cases in the repository test: the mixed-case/accent ordering, the same list walked page by page (this is the one that fails ifORDER BYand the cursor disagree), and a rename re-sorting the row. The first two fail onmain's ordering; the rename case fails ifupdatestops writing the key — I checked each by reverting the relevant line.Stale request cache (#2352 (comment)). Fixed in 1dbe279. Confirmed, and the keys carry locale, limit, and cursor, so a write path can't name the entries it invalidates — hence
clearRequestCachePrefix, alongside the existingclearRequestCacheEntry.create,update, anddeletedrop the wholebylines:namespace, next to theinvalidateBylineObjectCache()call each already makes. The key is built in one place (bylines/cache-keys.ts) so the reader and the write paths can't drift. Covered by a case that reads, creates, renames, and deletes inside onerunWithContext, asserting the list after each; it returns the stale first page before the fix.Scoped to the key this PR introduces.
byline-by-slug:,taxonomy-terms:,menu:, andsiteSetting:have the same gap today and none are cleared on write — worth a sweep, but not from this PR.Object-cache layer deferred to a follow-up, as agreed.
getBylines()stays onrequestCachedonly for now.Implements #2333. No issue to close.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change) — fullpackages/coresuite: 5668 passed, 9 skipped.packages/registry-verificationfails on my machine for an unrelated environment reason (node choking on the pnpm binary); it fails identically on a clean checkout ofmain.pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Tests, all new:
tests/unit/database/repositories/byline.test.ts— alphabetical order with pagination covering every row exactly once; mixed-case and accented names alphabetized, and that same list walked page by page; a rename re-sorting the row; a display-name tie split across a page boundary (theidhalf of the cursor); the media join populatingavatarStorageKey/alt/blurhash/dominant colour;localereturning one row per person;customFieldsstaying{}whilefindByIdstill reads the seeded value.tests/unit/bylines/bylines-query.test.ts— a guest author and a draft-only author both appearing in the list (the gap this closes); implicit locale resolving to the configured default rather than returning every locale variant; no fallback when the requested locale has no row; cursor pagination; an empty result on a database missing_emdash_bylines; writes during a request invalidating the list read earlier in it.tests/unit/database/migrations/068_byline_display_name_sort.test.ts— the backfill, a row past the first batch, a re-run against a database that already has the column, anddown().tests/integration/byline-list-plan.test.ts— a query-plan guard following the existing*-plan.test.tspattern (noANALYZE, since D1 never maintainssqlite_stat1): locale-index seek with and without a cursor, media reached by primary key, noSCANon either table. Index names are matched by prefix regex so a later locale-leading composite doesn't fail the test on an improvement.I checked the plan test fails on a real regression rather than just passing: dropping
idx__emdash_bylines_localefails the two seek assertions, and removing the media join fails the primary-key one.🤖 Generated with Claude Code