Skip to content

refactor: centralize record status copy - #97

Open
rajvveer wants to merge 1 commit into
textrefs:stagingfrom
rajvveer:fix/48-record-status-drift
Open

refactor: centralize record status copy#97
rajvveer wants to merge 1 commit into
textrefs:stagingfrom
rajvveer:fix/48-record-status-drift

Conversation

@rajvveer

Copy link
Copy Markdown

Summary

  • remove the confirmed orphaned CitationBox component
  • centralize draft, withdrawn, and blocked state derivation and wording in a style-free library module
  • reuse the shared copy from both Tombstone and the lightweight reference page while preserving their separate markup and styling
  • add focused status-derivation regression coverage

Validation

  • 100 tests passed
  • fixture-backed Astro check completed with 0 errors
  • fixture-backed Astro build generated 105 pages and validated all internal links
  • targeted Prettier check passed

Closes #48

@maehr

maehr commented Aug 27, 2026

Copy link
Copy Markdown
Member

Thank you — the mechanical work here is clean. I verified it against staging at a5764fb rather than re-reading the diff:

  • npm run format:check clean, npm test 100/100, npm run check 0 errors, fixture build 105 pages with all internal links valid.
  • CitationBox.astro really is orphaned: no references anywhere in the repo, including docs.
  • I built the fixture site on both branches and diffed dist/. The banner markup is byte-identical on all four affected page types (/id/ref, /id/work, /id/system, /id/mapping). The only differences are Astro style-block ordering. No copy or whitespace drift, which is the thing that was easy to get wrong here.

Could you split this into two PRs? The two halves of #48 have different risk profiles.

Delete CitationBox.astro on its own — that half is ready. It is verified orphaned, nothing else in the issue touches it, and it can merge as soon as it is separated.

Hold the copy extraction behind #99. The purpose of record-status.ts is to freeze the wording and the isDraft / isTombstone derivation in one place. That derivation is exactly what is now open: ADR-0004 and specification.md:284 both call deprecated a tombstone status, while both templates test only withdrawn and blocked. I filed #99 for it. Whichever way it resolves, record-status.ts and record-status.test.ts are what changes — and as written the test would pin today's two-status answer as intended behaviour before that discussion happens. Deciding first means this lands once instead of twice.

That is a scoping problem on our side, not a defect in your patch. Sorry for the extra round-trip.

One design note for when the extraction does land:

Splitting the sentence into beforeStatus / status / afterStatus moves the duplication rather than removing it. Both call sites still reassemble it with their own {' '} placement, so rewording across the <code> boundary means editing three fields plus two templates — which is close to the drift risk #48 wants gone. src/lib/banner.ts already has the pattern that solves this: copy stored with its markup (UNSTABLE_BANNER.content holds an <a href>), rendered as HTML. A single constant containing <code>active</code> inline would be genuinely single-source, and adds no CSS, so the page-weight constraint in RecordPage.astro is untouched. [uuid]/index.astro already uses set:html for the Chicago string, so the mechanism is established.

Two smaller ones:

  • getRecordStatusBanner(status: string) should take the Status type from standard/schema/common.ts. The bare string is the reason nothing in the type system flags the deprecated case, and it would give exhaustiveness if the derivation grows to three states.
  • There is a third derivation of the same kind at src/lib/noindex.ts:18. It only tests draft, so it is correct today, but it belongs in a shared status module if one lands.

@maehr

maehr commented Aug 27, 2026

Copy link
Copy Markdown
Member

Follow-up: I split #48, so the two halves now have their own trackers.

If you would rather not carry this further, that is completely fine — say so and we will take it from here. Your branch is retained either way, and both issues point at it.

@maehr

maehr commented Aug 27, 2026

Copy link
Copy Markdown
Member

Thank you for this — the shape is right, and it is what #100 asks for. Two things have moved underneath it since you opened it, and both change what should be in this PR.

1. The CitationBox half has landed separately

#101 deletes src/components/CitationBox.astro. That is the half of the old #48 that was unblocked, which is why #48 was split: this PR could not merge as a unit while its other half waited on a decision. So the deletion here is now redundant and will conflict.

2. #99 is decided, and it changes what the module must encode

This was the blocker. Decided 2026-08-27:

A deprecated record renders the existing tombstone presentation, with deprecated-specific copy and the normal superseded_by treatment. deprecated, withdrawn and blocked stay one tombstone category; their user-facing messages distinguish their different semantics.

src/lib/record-status.ts here derives isTombstone from withdrawn || blocked, and record-status.test.ts pins that as intended. #100 flagged exactly this risk when it was filed. The module now needs:

  • isTombstone as deprecated || withdrawn || blocked, typed against the Status enum in standard/schema/common.ts rather than string — the bare string is why nothing in the type system flagged the deprecated case in the first place;
  • three title strings, one per status, not a boolean;
  • one extra line of copy that only deprecated shows.

#101 implements the decision in both templates without extracting it, so the duplication is three strings wider than when you started. The extraction is still worth doing.

3. On the extraction design

#100's body argues against the beforeStatus / status / afterStatus split this PR uses. Not because it is wrong, but because it moves the drift risk rather than removing it: rewording across the <code>active</code> boundary means editing three fields and two call sites, and each call site still places its own {' '}. src/lib/banner.ts has the pattern that works — one constant holding the copy with its inline markup, rendered with set:html. [uuid]/index.astro already uses set:html for the Chicago citation string, so the mechanism is established there, and it adds no CSS, which is the constraint RecordPage.astro documents.

What I suggest

Re-scope this PR to #100 alone and rebase it after v0.1.0 is tagged. #100 is deliberately out of the release (#68 §4) — it is an internal refactor with no user-visible change, and staging should stop moving before the tag. Nothing is lost by waiting, and rebasing after #101 means you extract the copy once, in its final three-status form, instead of twice.

Happy to review it then. If you would rather not carry the branch, say so and it can be picked up from refs/pull/97/head.

@maehr maehr added the post-v0.1.0 Deferred past the v0.1.0 baseline. Revisit if the need arises. label Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

post-v0.1.0 Deferred past the v0.1.0 baseline. Revisit if the need arises.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants