Skip to content

feat: extensible bylines#1258

Merged
ascorbic merged 39 commits into
emdash-cms:mainfrom
MohamedH1998:feat/extensible-bylines
Jun 3, 2026
Merged

feat: extensible bylines#1258
ascorbic merged 39 commits into
emdash-cms:mainfrom
MohamedH1998:feat/extensible-bylines

Conversation

@MohamedH1998
Copy link
Copy Markdown
Contributor

@MohamedH1998 MohamedH1998 commented Jun 1, 2026

What does this PR do?

Adds support for site-specific byline metadata — Twitter handle, pronouns,
company, localised job title, etc. — via a new /byline-schema admin
screen. Sites can register custom fields with their own type, validation,
and per-locale storage policy without editing code; values surface on
BylineSummary.customFields for the frontend.

Custom-field values can be set at both create and update time through
the same customFields map on POST and PUT /_emdash/api/admin/bylines.
In the admin, registered fields render inline with Name, Bio, etc. — no
"Custom fields" section header — and are available in both the New byline
and Edit byline dialogs. The schema link sits at the top of the
Bylines page (admin-only) rather than the global sidebar so admins find
it in context.

Design choices worth flagging

  • Schema in the database, not config. _emdash_byline_fields is the
    source of truth, with options.byline_fields_version driving cache
    invalidation. Admins register fields through the admin UI; no code
    edits required.
  • Per-field translatable flag. Translatable values are stored per
    locale (one row per locale in the byline's translation_group).
    Non-translatable values are stored once per translation_group and
    surface on every locale variant. The flag is locked once values
    reference the field — flipping would orphan rows in the wrong table.
  • url scheme allowlist. URL fields require http: or https:;
    javascript:/data:/mailto:/ftp:/file: reject. Mirrors
    httpUrl in api/schemas/common.ts — custom URLs typically render
    as <a href> so this closes the XSS footgun.
  • Create + update share one transaction on Node/PG. Byline row
    write and per-field writes roll back together on partial failure.
  • D1 retry-safe create via idempotent recovery. D1 has no
    transactions, so a crash between the row insert and the field writes
    leaves a partial byline that the API would otherwise refuse to
    recover from. A retry POST is treated as completing the abandoned
    create iff the full fixed-column payload, the translation-group
    identity, and the existing custom-field subset all match the
    incoming request. Anything else collapses to a standard
    duplicate-slug CONFLICT — recovery only fires when the retry is
    provably the same request as the original.
  • Cache coherency via parity bit. options.byline_fields_version
    carries meaning in its parity — odd = mutation in flight (or
    crashed), even = stable. The cache bypasses the global holder while
    odd. markVersionDirty is parity-aware idempotent; markVersionClean
    always advances to a new even value so two concurrent mutators
    can't collapse on the same key and pin a stale snapshot.
    Idempotent-retry exits also run markVersionClean — same code path
    doubles as crash recovery and false-clean recovery.

BylineSummary gains an optional
customFields: Record<string, CustomFieldValue>. Existing
object-literal consumers stay source-compatible — the property is
optional and runtime always returns {} when no fields are registered.
Builds on the bylines-i18n foundation from #1146.

What's deferred

  • required flag enforcement. The admin UI exposes a Required
    toggle and the value persists on the field definition, but the
    server accepts missing values and null for any field. Needs a
    design call on the enforcement model (reject on first set only?
    reject on every save? coerce vs reject?). TODOs are in
    BylineRepository.coerceFieldValue and BylineFieldEditor.
  • Full multi-isolate concurrency on schema mutations. The
    parity-aware bookend has a residual race between two concurrent
    markVersionClean calls — bounded by the inter-clean duration
    (~ms). Schema mutations are admin-only and rare; acceptable for
    this PR. A CAS-on-bump or dialect-specific row lock would close
    it but raises dialect-divergence scope.
  • Idempotency keys for D1 retries. The recovery branch carries
    multiple conjunctive guards approximating "same request as before"
    via state inspection. If similar D1 retry-safety problems recur on
    other create endpoints, an explicit Idempotency-Key header would
    replace the apparatus with one lookup. Worth a Discussion before
    the next D1 retry surface comes up.
  • translationOf-flavoured create recovery. The sibling-locale
    guard returns CONFLICT before the recovery branch can fire on
    translation creates, so partial translationOf retries fall through
    to manual recovery via the edit flow. Uncommon enough that
    reordering the guard wasn't justified for this PR.

Closes #1174

Closes #

Type of change

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • User-visible strings in the admin UI are wrapped for translation (if applicable). Do not include messages.po changes except in translation PRs — a workflow extracts catalogs on merge to main.
  • I have added a changeset (if this PR changes a published package)
  • New features link to an approved Discussion: Custom fields on bylines #1174

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: Claude Opus 4.7, GPT-5.5

Screenshots / test output

Screen.Recording.2026-06-01.at.11.26.51.mov

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 1, 2026

🦋 Changeset detected

Latest commit: e6d45e9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
emdash Minor
@emdash-cms/admin Minor
@emdash-cms/cloudflare Minor
@emdash-cms/sandbox-workerd Patch
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/auth Minor
@emdash-cms/blocks Minor
@emdash-cms/gutenberg-to-portable-text Minor
@emdash-cms/x402 Minor
create-emdash Minor
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

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 github-actions Bot added query-count changed PR diff modifies query-count snapshot files area/admin labels Jun 1, 2026
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/needs-review No maintainer or bot review yet labels Jun 1, 2026
MohamedH1998 and others added 2 commits June 1, 2026 14:08
@ascorbic
Copy link
Copy Markdown
Collaborator

ascorbic commented Jun 1, 2026

/review

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Jun 1, 2026

delete() does NOT clear the request-cache entry for byline-field-group-values:${group} when removing the last sibling. This means within the same request, a sequence findById → delete → findById (different byline in same group) would see stale cache. But since the byline row is deleted entirely, there's no follow-up findById for the same id... however, this is a minor issue mostly.

Actually wait — if the delete is the last sibling, and _emdash_byline_field_group_values rows for that group are deleted but the cache entry remains with old values, AND if some other code path in the same request tries to read group values for that translation_group (e.g. via a stale credit not yet purged), it would see stale data. But the delete() cleanup of _emdash_content_bylines.byline_id = group already removes the credits, so this is moot.

Still worth a comment maybe. But I'm getting into super-nit territory. Let me focus on finalizing my review.

Given the prior ask-bonk review already covered the most substantive nits well (URL empty string, getVersion JSDoc, staleTime, getBylineBySlug request-cache), and the PR is genuinely well-engineered with extensive tests, I'll respond with LGTM!.

LGTM!

github run

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Overlapping PRs

This PR modifies files that are also changed by other open PRs:

This may cause merge conflicts or duplicated work. A maintainer will coordinate.

Comment on lines +113 to +114
const defs = await new BylineSchemaRegistry(db).listFields();
holder.cached = defs;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The jsdoc says that it caches the promise not the resolved value, but we're awaiting it before assigning. It's better to cache the promise because, as the comment says, it allows us to coalesce concurrent requests to use a single db query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-rereview Author pushed changes since the last review labels Jun 2, 2026
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review needs-rebase and removed review/awaiting-author Reviewed; waiting on the author to respond labels Jun 2, 2026
Merging emdash-cms#1298 (byline avatar storage key hydration) exposed a silent
regression: getContentBylines/getContentBylinesMany reshape the joined
rows into a minimal BylineRow before withCustomFields, which dropped the
avatar_storage_key/avatar_alt columns the new media LEFT JOIN selects, so
avatarStorageKey/avatarAlt always came back null. Carry both columns
through the reshape (typed BylineRowWithAvatar). findByUserIds passes the
joined rows directly and was already correct.

Query-count snapshots regenerated for the merged tree.
@ascorbic
Copy link
Copy Markdown
Collaborator

ascorbic commented Jun 3, 2026

Pushed a merge of main into this branch (e6d45e93, with Mohamed's go-ahead) to recover a fix that the merge would otherwise silently drop.

Background: #1298 (byline avatar storage key) landed on main and adds a LEFT JOIN media to the byline hydration queries, surfacing avatarStorageKey/avatarAlt on BylineSummary. This branch's custom-field work reshapes the joined rows into a minimal BylineRow before withCustomFields, and that reshape dropped the two new avatar columns — so a plain merge compiles fine and all tests pass, but avatarStorageKey silently comes back null (the join runs, its values are discarded).

The fix carries avatar_storage_key/avatar_alt through the reshape (typed BylineRowWithAvatar[]) in getContentBylines and getContentBylinesMany. findByUserIds passes the joined rows straight through, so it was already correct. Query-count snapshots regenerated for the merged tree. Byline tests (incl. the avatar hydration cases from #1298) pass.

Note: the merged tree shows a uniform +1 query/route from the per-request byline_fields_version read. That's expected and accepted for now (it buys immediate cross-isolate invalidation); consolidating cold-start option reads is a separate follow-up, not a blocker.

@ascorbic ascorbic merged commit 28432b9 into emdash-cms:main Jun 3, 2026
35 checks passed
@emdashbot emdashbot Bot mentioned this pull request Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/admin area/core cla: signed overlap query-count changed PR diff modifies query-count snapshot files review/needs-rereview Author pushed changes since the last review size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants