Skip to content

test(e2e): seed real artifacts, and cover the markup card's clamp in a browser#561

Merged
gbasin merged 1 commit into
masterfrom
feat/e2e-artifact-seeding
Jul 17, 2026
Merged

test(e2e): seed real artifacts, and cover the markup card's clamp in a browser#561
gbasin merged 1 commit into
masterfrom
feat/e2e-artifact-seeding

Conversation

@gbasin

@gbasin gbasin commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Closes the gap #556 shipped with: the markup card's clamp had no real-browser coverage, and two real bugs used that gap to reach prod with the whole suite green.

Why unit tests could not catch them

The clamp is measuredscrollHeight vs clientHeight. jsdom has no layout and reports 0 for both, so its unit tests stub the metrics. That pins the wiring and says nothing about whether the thing measures. Both #556 bugs were invisible to it:

  • the clamp measured through a ref in an effect keyed on deps that never changed, so content mounting after a fetch was never measured and the toggle could not appear at all;
  • the fade rode on the collapsed state rather than on overflow, veiling diffs that already fit.

Both were found by hand in a browser. Neither should have needed that.

The helper

Nothing could cover the card because e2e had no way to make an artifact, and the card fetches its body over HTTP. seedArtifact posts bytes to the node capture endpoint, so the artifact lands through the real writeBackArtifact path — real CAS blob, real ledger rows, served for real by /api/files/artifact/:id/content. Hand-inserted rows would drift from the write path and pass while the product broke.

It needed no new plumbing: ARTIFACT_CAPTURE_API_KEY was already in the e2e webServer env. Only the key had to be published into workers the way the derived ports already are — otherwise the two ends drift into an unexplained 401.

The specs, and why they're worth their runtime

Each was run against the broken code first:

spec fails against
a diff that fits offers no toggle and no fade fade gated on clamped instead of clamped && overflows
a diff that overflows offers a toggle and a fade, and expanding clears both the ref trap — no observer, so no toggle, ever
the collapsed clamp keeps an absolutely positioned descendant inside its clip dropping relative from the clamp

A test that has never failed is a decoration.

This corrects the record on #556

#556 claimed this card "sat one position: static away from #544" and that adopting the clamp fixed a latent leak. That was overstated. Measured in a real browser: CriticMarkupView's absolutely positioned descendant is a 1px sr-only span with auto offsets, so escaping the clip costs nothing observable — scrollHeight is identical with the clamp static and relative (779 both ways). #544's ~3000px came from a sized element in MessageText. Same mechanism, nothing like the same impact.

The relative is a defensive invariant, not a bug fix, and the test now says exactly that instead of claiming a leak it cannot demonstrate. The real wins at that call site were the toggle and the fade. The test also renders a hunk separator (), because without one the card has no absolutely positioned descendant and the test would guard nothing — which is what my first draft did.

Notes

  • The seeded session is cancelled: it exists only to authorize the write and is never run, and every e2e user shares one workspace whose sidebar groups running sessions across it. No existing spec was observed to fail on a running seed — this is hygiene against a future count-based assertion, not a fix for a known flake.
  • The only product change is a data-testid on the clamp box.

Verification

pnpm lint ✓ · pnpm typecheck ✓ · 771 web unit tests ✓ · pnpm e2e: 122 passed (119 + 3 new) on a clean run of the final code. An earlier run showed one spine-aside failure; it passes in isolation and passed in both subsequent full runs — the known local rotating-cast flake, not this branch.

…a browser

The markup card's clamp is measured — `scrollHeight` against `clientHeight` —
and jsdom has no layout, so it reports 0 for both. Its unit tests stub the
metrics, which means they pin the wiring and can say nothing about whether the
thing measures. Two real bugs walked through that gap in #556 and were caught
only by hand in a browser: the clamp measured through a ref in an effect keyed
on deps that never changed, so content mounting after a fetch was never
measured and the toggle could not appear at all; and the fade rode on the
collapsed state rather than on overflow, veiling diffs that already fit. The
suite was green for both.

Nothing could cover it because e2e had no way to make an artifact, and the card
fetches its body over HTTP. `seedArtifact` closes that: it posts bytes to the
node capture endpoint, so the artifact lands through the real `writeBackArtifact`
path — real CAS blob, real ledger rows, served for real. Hand-built rows would
drift from the write path and pass while the product broke. This needed no new
plumbing: `ARTIFACT_CAPTURE_API_KEY` was already in the e2e webServer env, so
only the key had to be published to workers the way the derived ports already
are, or the two ends drift into an unexplained 401.

Each spec was run against the broken code first: the ref trap fails the overflow
test, fade-on-collapsed fails the fits test, and dropping `relative` fails the
clip test. A test that has never failed is a decoration.

The clip test also corrects the record. #556 claimed this card "sat one
position: static away from #544", and that was overstated: CriticMarkupView's
absolutely positioned descendant is a 1px `sr-only` span with auto offsets, so
escaping the clip costs nothing measurable — scrollHeight is identical with the
clamp static and relative. #544's ~3000px came from a sized element in
MessageText. The `relative` is a defensive invariant, not a fix, and the test
now says so rather than claiming a leak it cannot show. It also renders a hunk
separator, because without one there is no absolutely positioned descendant in
the card and the test would guard nothing.

The seeded session is `cancelled`: it exists only to authorize the write and is
never run, and every e2e user shares one workspace whose sidebar groups running
sessions across it. No existing spec was observed to fail on a `running` seed —
this is hygiene, not a fix for a known flake.
@gbasin
gbasin added this pull request to the merge queue Jul 17, 2026
Merged via the queue into master with commit bdf748d Jul 17, 2026
16 checks passed
@gbasin
gbasin deleted the feat/e2e-artifact-seeding branch July 17, 2026 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant