-
Notifications
You must be signed in to change notification settings - Fork 0
Review 4319
#4319 · HelloOjasMutreja · aged-PR cleanup Step 3, batch 3, 2026-08-23 · two rounds
Verdict: approve — MERGED c5c7e54
Drift between src and docs for Citation and Markdown, per #4163. Last of the six-PR reconciliation set.
As opened: adds title/url/icon to Citation's documented props, components to Markdown's, plus a contract test and a patch changeset.
As merged: Markdown's components entry and the changeset. Nothing else.
no API change (docs only)
no new theme targets
API no · Visual no · Theme no
Round 1. CitationProps is {ref, source, number, variant} (Citation.tsx:61-66); title/url/icon are fields of CitationSource, and icon is ReactNode, not string. Markdown's components?: Partial<MarkdownComponents> is real (Markdown.tsx:183) and MarkdownComponents is exported from the core barrel (Markdown/index.ts:15). Checked the zh omission and did NOT raise it — component-loader treats translations as an overlay with per-prop English fallback (componentDocOverlay.test.mjs).
Round 2, against head 34823970: Citation.doc.mjs is out of the diff entirely, and on main it still lists exactly source, number, variant. The contract test was inverted rather than left pinning the bad shape — it now asserts title/url/icon are absent. Both halves of the round-1 ask are met.
MarkdownComponents has nine keys (Markdown.tsx:102-127); the description enumerated eight, omitting inlineCode.
Round 1: request changes. [BLOCKS] three documented props that do not exist, pinned by a new test → a builder writes <Citation title=… url=… /> and nothing renders. Also answered the author's 22-day-old question: pr-a11y succeeded on 4fba5d4 (run 30893800834, job completed 2026-08-04T20:50:25Z), so there is nothing to baseline.
Round 2: approve, with one maintainer commit rather than a third round — the author turned round 1 around in 67 minutes and the remaining edits were ours to make, not his to guess at.
1. [dropped] packages/core/src/__tests__/contentComponentContract.test.tsx
the seventh test of the shape #5382 deleted five of and #5393 deletes a
sixth. Inverting it to a negative assertion does not rescue it: it still
reads only the docs side, so it stays green whether or not the docs match
Citation.tsx — the drift it looks like it catches
2. [scoped] the changeset still said "(Citation, Markdown)" after the Citation
half came off. That line goes into the public changelog
3. [completed] inlineCode added to the components description's list, the one
MarkdownComponents key it omitted
#5382 deleted five of these at 01:58 UTC; #4316 merged at 04:14 UTC carrying a sixth, and #5393 is open to remove it. This PR held a seventh — same getProps helper copied inline, same assertion that a .doc.mjs lists the prop names the same commit wrote into it, true at merge time by construction.
The collision is structural, not bad luck: file-deleting PRs never conflict with file-adding ones, so nothing in CI flags a new instance of a pattern that was just removed. Landing it would have meant an eighth PR to take it out again. The repo's real guards — docPropReferences.test.ts, docPropLiterals.test.ts — derive expectations by scanning sources, and #4163's planned derived gate is still the right way to close the drift gap.
Worth noting the round-1 ask made this look discharged: the test was named in the review as part of the problem, the author fixed exactly what was named, and the fixed test is still worthless. "The ask was met" and "the file should land" are separate questions.
Author spoke last on 2026-08-01 asking a direct question; unanswered until round 1. Vercel red on the PR all evening — not a required check, and failing on every fork PR tonight.
Round 1 and round 2 both on #4319. Attribution: [Reviewed by Robohands].
(posted directly by the loop; round 2 under "4319 get it merged" — owned end to end, no approval round-trip)