Make the renderer's contract reachable from the DOM tests, so the fixtures stop being a copy (#94)

[mayankguptadotcom]

Closes #94.

The seam, and why this one

The issue proposed extracting COMMENT_CLASS_NAMES into a dependency-free module. I went one layer deeper instead, because the class-name list was a symptom rather than the coupling.

src/render/comments.ts type-imported RenderableComment from src/db, and src/db is typed against D1Database. The dom vitest project deliberately has the DOM lib and not the Worker's ambient types, so importing the renderer from test/dom/** pulled the whole data layer into a program that cannot type it:

src/db/index.ts(182,7): error TS2552: Cannot find name 'D1Database'. Did you mean 'IDBDatabase'?
… 12 more of the same
test/dom/embed/mount.test.ts(20,15): error TS2724: '"../../../src/render"' has no exported member named 'RenderableComment'.

So RenderableComment moves to src/render/comments.ts, and src/db imports it. That is the direction the dependency should already have run: the renderer is the general half of the pair — D1 is one source of rows and the v1.1 build-time path takes them from a site generator — and a shape owned by the data layer drags Cloudflare's ambient types into every program that renders.

Two consequences worth flagging in review:

• src/db does not re-export it. One type with two import paths is how a second declaration shows up later. The two worker render tests that took it from src/db now take it from src/render, which is where a render test should have been asking anyway.
• Extracting only the class names would not have been enough. It would have made the names importable while the fixture markup stayed hand-written, so the shape of the markup could still drift. It already had, twice — see below.

Rejected: widening test/dom/tsconfig.json to admit the ambient types. It papers over the coupling and gives the browser half of the repo Cloudflare's fetch and Response, which is the exact thing the split exists to prevent.

What the fixtures do now

test/dom/embed/mount.test.ts builds its bodies by calling renderComments, mirroring both wire shapes — threadHtml() is the page read (src/read/route.ts), postedHtml() is the submit echo flattened to a root the way src/submit/pipeline.ts flattens it.

The hand-written copy had already drifted from both, which is the concrete cost of the gap:

• the invitation was Be the first to comment. — the renderer emits no full stop;
• a posted comment was a bare <li>, where the Worker sends a whole rendered <ol>.

The selectors in the assertions stay string literals on purpose. mount.ts reads those names as literals too, so a selector derived from the same source as the markup would follow a rename and prove nothing about the file under test. The literals are the pin; the fixture is what moves under them.

Kill-shot

Both runs apply the rename consistently across the renderer and its own worker test — the scenario #94 describes, which went fully green before this change.

1. Rename a class: charcha-comment-author → charcha-comment-name (in src/render/comments.ts, COMMENT_CLASS_NAMES, and test/worker/render/comments.test.ts)

$ pnpm exec vitest run --project worker
 Test Files  41 passed (41)

$ pnpm exec vitest run --project dom
 ❯ |dom| test/dom/embed/mount.test.ts (28 tests | 1 failed) 2239ms
     × names the author as text, so a name that looks like markup stays a name 54ms

 FAIL  |dom| test/dom/embed/mount.test.ts > replying to a comment > names the author as text…
AssertionError: expected 'Replying to ' to be 'Replying to <img src=x onerror=alert(…' // Object.is equality

Expected: "Replying to <img src=x onerror=alert(1)>"
Received: "Replying to "

 Test Files  1 failed (1)
      Tests  1 failed | 27 passed (28)

2. Rename the id template: id="charcha-comment-${…}" → id="charcha-c-${…}" — this is what mount.ts parses back out with comment.id.replace('charcha-comment-', '').

$ pnpm exec vitest run --project worker
 Test Files  41 passed (41)

$ pnpm exec vitest run --project dom
     × reads the page it is on, and shows what came back 63ms
     × offers a Reply on every root comment, and on no reply 54ms
     × reads again when the retry is pressed, and renders what arrives 1056ms
     × clears the previous error before the next attempt 1109ms
     × puts the reader’s own comment on the page, badged as pending 1055ms
     × posts when the reader presses the composer’s own button 1059ms
     × badges nothing when the comment was published outright 1057ms
     × replaces the invitation rather than commenting underneath it 1054ms
     × moves focus to the comment it just placed 1053ms
     × empties the body but keeps the name, so a second comment is not retyped 1055ms
     × moves focus into the composer, not just the composer to the comment 55ms
     × mounts the one composer beneath the comment being answered 53ms
     × keeps what the reader had already typed when the composer moves 55ms
     × names the author as text, so a name that looks like markup stays a name 54ms
     × sends the reply with the parent it was written under 54ms
     × nests the reply under its parent and sends the composer home 55ms
     × returns focus to the Reply button when the reply is cancelled 55ms
 Test Files  1 failed (1)

Both restored from backup; suite green afterwards (47 passed (47), 823 passed (823)).

The rendered HTML is unchanged

Not inferred from green tests. The renderer diff is a comment block, a moved type declaration and one deleted import — no emitted string is touched — and that was checked mechanically: both revisions of src/render were bundled with esbuild and handed the same rows (empty page, nested replies, an out-of-range timestamp, an orphaned reply, a markup-shaped author name, a custom strings table, and the truncation notice).

    3169 old.html
    3169 new.html
b602202bcbb18515616f9b784bda2eb498fb0e5ee414d58c2183d408a5b65af9  old.html
b602202bcbb18515616f9b784bda2eb498fb0e5ee414d58c2183d408a5b65af9  new.html
IDENTICAL — renderComments output is byte-for-byte unchanged

test/worker/render/comments.test.ts asserts the full markup string literally, and it is untouched apart from its import line.

The embed bundle is unaffected — nothing under src/embed/ imports src/render, and check:size reports the same 6791 B gzipped of 10240 B allowed.

Reviewing the vocabulary.test.ts hunk

GitHub will show it as Bin 8436 -> 8398 bytes. That is not this change — the file contains a raw NUL byte in an XSS payload on line 73, so git has always classified it as binary. Filed as #114. The delta here is exactly 38 bytes, which is the two import lines collapsing into one:

$ git diff --text test/worker/render/vocabulary.test.ts
 import { describe, expect, it } from 'vitest'
-import type { RenderableComment } from '../../../src/db'
-import type { CommentStrings } from '../../../src/render'
+import type { CommentStrings, RenderableComment } from '../../../src/render'
 import {

Verification

$ pnpm check
 Test Files  47 passed (47)
      Tests  823 passed (823)
[ok] embed: src/embed/index.ts is 6791 B gzipped of 10240 B allowed
[ok] pnpm is the only lockfile here, its version is pinned to a major the Cloudflare build image runs (10.x), and its format is one that image reads (9.0)
[ok] every package licence is within policy
[ok] every action reference is pinned to a full commit SHA
No known vulnerabilities found
pnpm check EXIT=0

Skills used: api-and-interface-design (which module owns the shared type), test-driven-development (the failing DOM typecheck came first), code-review-and-quality, improve scoped to the branch — which is what turned up #114 — and superpowers:verification-before-completion.

For the owner

• This touches one block of src/db/index.ts (the interface declaration becomes an import). That file belongs to the parallel dashboard session, so expect a possible conflict there and nowhere else in this PR.
• src/embed/mount.ts is untouched — the A 201/202 whose body carries no comment tells the reader it worked and shows them nothing #93 session owns it. The remaining half of The renderer's class-name contract cannot reach the DOM tests, so their fixtures are an uncoupled copy #94's "worth checking at the same time" note is whether mount.ts should select those names through the shared module. My read is that it should not: literals on both sides are what make the rename break. Happy to be overruled.

🤖 Generated with Claude Code

[mayankguptadotcom]

Coordinator verification

The hole is closed, and I reproduced the proof

I renamed a class consistently across the renderer and its own worker test — the exact scenario #94 says goes green today:

worker project →  41 files, 686 tests passed     ← still blind, as #94 described
dom project    →  × names the author as text, so a name that looks like markup stays a name
                  1 failed | 27 passed (28)

That is the issue in two lines. The worker suite cannot see a coordinated rename because the renderer and its tests move together; the DOM tests now can, because their fixtures are built by calling renderComments rather than restating its output. Tree clean afterwards.

The chosen seam is better than the one I briefed

I suggested making the class names importable. That would have made the names shared while the fixture markup stayed hand-written and free to drift — a narrower fix that leaves the same class of hole one level down.

Moving RenderableComment from src/db to src/render is the right cut, and the dependency direction argument is correct: the renderer is the general half. D1 is one source of rows; the v1.1 build-time path takes rows from a site generator and never touches D1 at all. A shape owned by the data layer was dragging Cloudflare's ambient types into every program that renders, which is the actual cause rather than a symptom of it.

Rejecting the tsconfig widening is right too — it would have handed the browser half of the repo Cloudflare's fetch/Response types to paper over a coupling that can simply be removed. CLAUDE.md's rule that a refactor relocating complexity is not a simplification, applied correctly.

Both judgement calls hold up

Not re-exporting from src/db — one type, one import path. The alternative invites a second declaration that drifts.

DOM selectors stay string literals. This is the subtle one and it is right: mount.ts reads those names as literals, so a selector derived from the same source as the markup would follow a rename and assert nothing. The literals are the pin; the fixture moves under them. Deriving both sides would have produced a test that can never fail — which is the exact defect #91 and #94 exist to remove.

Byte-identical output, demonstrated rather than asserted

Bundling both revisions of src/render and rendering the same rows — empty page, nested replies, out-of-range timestamp, orphaned reply, markup-shaped author, custom strings, truncation — gives 3,169 bytes and sha256 b602202b…5af9 on both sides. For a refactor whose entire promise is "behaviour-preserving", a hash on both sides is the proof; a passing suite alone would not have been.

One thing for merge ordering

This touches src/db/index.ts, which the parallel dashboard session owns. The edit is confined to the top of the file — an import replacing a type declaration — so it is unlikely to collide with queries added lower down, but it is a real overlap and unavoidable: "have src/db conform to the type" is an edit to that file.

Worth merging before the dashboard branch grows, rather than after.

#114 is a good catch

test/worker/render/vocabulary.test.ts carries a raw NUL byte inside an XSS payload, so git classifies the file as binary and its diffs render as Bin 8436 -> 8398 bytes — unreviewable in the GitHub UI. Pre-existing, and exactly the kind of thing that hides a change in a security test file. Including the git diff --text output in the PR body so the hunk stays reviewable was the right call.