From 96aa24efc9d32960b616efd69dde6c2b5af469c9 Mon Sep 17 00:00:00 2001 From: Gary Basin Date: Thu, 16 Jul 2026 21:26:05 -0400 Subject: [PATCH] test(e2e): seed real artifacts, and cover the markup card's clamp in a browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- surface/e2e/playwright.config.ts | 8 +- surface/e2e/tests/helpers.ts | 92 +++++++++++ surface/e2e/tests/markup-card-clamp.spec.ts | 148 ++++++++++++++++++ surface/web/src/components/EntryQuoteCard.tsx | 1 + 4 files changed, 247 insertions(+), 2 deletions(-) create mode 100644 surface/e2e/tests/markup-card-clamp.spec.ts diff --git a/surface/e2e/playwright.config.ts b/surface/e2e/playwright.config.ts index 5ee50adf..c3f46fb0 100644 --- a/surface/e2e/playwright.config.ts +++ b/surface/e2e/playwright.config.ts @@ -43,8 +43,12 @@ const centaurPort = Number(process.env.E2E_CENTAUR_PORT ?? 18100 + portOffset); process.env.E2E_SERVER_PORT = String(serverPort); process.env.E2E_WEB_PORT = String(webPort); process.env.E2E_CENTAUR_PORT = String(centaurPort); - const webServerTimeout = Number(process.env.E2E_WEBSERVER_TIMEOUT ?? 60_000); +// Same reason as the ports above: `seedArtifact` calls the node capture endpoint +// from a worker and must present the key the server was started with. Publishing +// it back keeps the two ends from drifting into an unexplained 401. +const captureApiKey = process.env.ARTIFACT_CAPTURE_API_KEY ?? 'e2e-capture-key'; +process.env.ARTIFACT_CAPTURE_API_KEY = captureApiKey; // The suite serves the web app as a static development-mode build (`vite build // --mode development` + `vite preview`) rather than from the dev server. The dev // server's on-demand transforms + dep optimizer compete with the tests for CPU on @@ -143,7 +147,7 @@ export default defineConfig({ LOG_LEVEL: process.env.LOG_LEVEL ?? 'error', ATRIUM_RATE_LIMIT: '0', ATRIUM_UNFURL_ALLOW_PRIVATE: '1', - ARTIFACT_CAPTURE_API_KEY: 'e2e-capture-key', + ARTIFACT_CAPTURE_API_KEY: captureApiKey, }, }, { diff --git a/surface/e2e/tests/helpers.ts b/surface/e2e/tests/helpers.ts index 608475a9..b86df100 100644 --- a/surface/e2e/tests/helpers.ts +++ b/surface/e2e/tests/helpers.ts @@ -11,6 +11,9 @@ import { Pool, type PoolClient } from 'pg'; export const baseURL = `http://127.0.0.1:${Number(process.env.E2E_WEB_PORT ?? 5273)}`; export const apiURL = `http://127.0.0.1:${Number(process.env.E2E_SERVER_PORT ?? 3101)}`; export const e2eDatabaseUrl = process.env.E2E_DATABASE_URL ?? 'postgres://atrium:atrium@localhost:5433/atrium_e2e'; +// Kept in step with the server's key by playwright.config.ts, which publishes it +// into the worker environment; the fallback only covers a bare `playwright test`. +const captureApiKey = process.env.ARTIFACT_CAPTURE_API_KEY ?? 'e2e-capture-key'; export async function seedEvent( client: PoolClient, @@ -771,3 +774,92 @@ export async function questionState(sessionId: string): Promise<{ await pool.end(); } } + +/** + * Seeds a real artifact with real bytes. + * + * This goes through the node capture endpoint rather than inserting rows: that + * is the same path node-sync uses, so the artifact lands in CAS and the ledger + * exactly like a captured one, and `/api/files/artifact/:id/content` serves it + * for real. Hand-built rows would drift from the write path and pass while the + * product broke. Capture is session-scoped, so a throwaway session is created + * to own the write unless one is supplied. + * + * The path must be canonically shared (`shared/channels//…`) for a reader + * to see it — a `scratch//…` artifact is private to that session. + */ +export async function seedArtifact(args: { + channelId: string; + body: string; + path?: string; + mime?: string; + sessionId?: string; +}): Promise<{ artifactId: string; handle: string; path: string; seq: number }> { + const path = args.path ?? `shared/channels/${args.channelId}/${unique('seeded')}.md`; + const pool = new Pool({ connectionString: e2eDatabaseUrl }); + try { + const sessionId = args.sessionId ?? (await createCaptureSession(pool, args.channelId)); + const ctx = await request.newContext({ baseURL: apiURL }); + try { + const res = await ctx.post( + `/api/internal/sessions/${sessionId}/artifacts/capture?path=${encodeURIComponent(path)}`, + { + headers: { + // Published by playwright.config.ts, which starts the server with the + // same value — so this cannot drift into a 401. + 'x-api-key': captureApiKey, + 'content-type': args.mime ?? 'text/markdown', + }, + data: args.body, + }, + ); + if (!res.ok()) throw new Error(`capture failed (${res.status()}): ${await res.text()}`); + const { seq } = (await res.json()) as { seq: number }; + const row = await pool.query<{ id: string }>( + 'SELECT id FROM artifacts WHERE channel_id = $1 AND path = $2 ORDER BY created_at DESC LIMIT 1', + [args.channelId, path], + ); + const artifactId = row.rows[0]?.id; + if (!artifactId) throw new Error(`captured artifact not found for path ${path}`); + return { artifactId, handle: `art_${artifactId}`, path, seq }; + } finally { + await ctx.dispose(); + } + } finally { + await pool.end(); + } +} + +/** A minimal session for capture to write through; the harness never runs it. */ +async function createCaptureSession(pool: Pool, channelIdValue: string): Promise { + const channel = await pool.query<{ workspace_id: string }>('SELECT workspace_id FROM channels WHERE id = $1', [ + channelIdValue, + ]); + const workspaceId = channel.rows[0]?.workspace_id; + if (!workspaceId) throw new Error(`no such channel: ${channelIdValue}`); + // `spawned_by` is a NOT NULL reference to a real user, so the session is + // owned by a member of the channel's own workspace rather than any user + // that happens to exist. + const owner = await pool.query<{ user_id: string }>( + 'SELECT user_id FROM workspace_members WHERE workspace_id = $1 ORDER BY user_id LIMIT 1', + [workspaceId], + ); + const ownerId = owner.rows[0]?.user_id; + if (!ownerId) throw new Error(`workspace ${workspaceId} has no members to own a capture session`); + // `cancelled`, because this session exists only to authorize the write and is never + // run — claiming `running` would be a lie the product reads. Every e2e user shares one + // default workspace and the sidebar groups running sessions across it, so a seed stuck + // at `running` forever is a phantom agent row other specs can see. (No existing spec + // was observed to fail on it — they assert on their own titles, not on counts — so + // this is hygiene against a future count-based assertion, not a fix for a known flake.) + const session = await pool.query<{ id: string }>( + `INSERT INTO sessions ( + workspace_id, channel_id, centaur_thread_key, harness, title, status, spawned_by, driver_id, + assignment_generation + ) + VALUES ($1, $2, $3, 'codex', $4, 'cancelled', $5, $5, 1) + RETURNING id`, + [workspaceId, channelIdValue, `thread-${unique('seed-artifact')}`, unique('artifact-seed'), ownerId], + ); + return session.rows[0]!.id; +} diff --git a/surface/e2e/tests/markup-card-clamp.spec.ts b/surface/e2e/tests/markup-card-clamp.spec.ts new file mode 100644 index 00000000..fac72477 --- /dev/null +++ b/surface/e2e/tests/markup-card-clamp.spec.ts @@ -0,0 +1,148 @@ +import { expect, test } from '@playwright/test'; +import { + apiAs, + channelId, + createTestChannel, + login, + openChannel, + postMessage, + seedArtifact, + unique, +} from './helpers.js'; + +// The markup card's clamp is measured (scrollHeight vs clientHeight), so jsdom +// — which has no layout and reports 0 for both — structurally cannot test it. +// Its unit tests stub the metrics, which means they assert the wiring and not +// the behaviour. Two real bugs shipped through that gap: 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; +// and the fade rode on the collapsed state rather than on overflow, dimming +// diffs that already fit. Both need a browser to see. + +const FRONTMATTER = ['---', 'title: "Seeded memo"', '---', ''].join('\n'); + +function markupBody(paragraphs: number, { separator = false } = {}): string { + const lines = [FRONTMATTER, '# Memo', '']; + lines.push('Keep {--old--}{++new++} wording and {==flag this==}{>>needs a source<<}.', ''); + // A lone `⋯` line is the hunk separator, which CriticMarkupView renders as an + // "omitted content" section carrying an absolutely positioned `sr-only` span. + if (separator) lines.push('⋯', ''); + for (let i = 0; i < paragraphs; i += 1) { + lines.push(`Paragraph ${i + 1}: ${'body text that wraps and takes vertical space. '.repeat(6)}`, ''); + } + return lines.join('\n'); +} + +const markupCard = 'article:has(:text-is("markup"))'; + +test('a markup diff that fits offers no toggle and no fade', async ({ page }) => { + const room = await createTestChannel('markup-fits'); + const handle = unique('markup-reader'); + const ctx = await apiAs(handle); + const id = await channelId(ctx, room); + + // Short: two changes, a couple of lines — nothing is hidden below the cut. + const artifact = await seedArtifact({ channelId: id, body: markupBody(0) }); + await postMessage(ctx, id, `seeded fits — /e/${artifact.handle}`); + + await login(page, handle); + await openChannel(page, room); + + const card = page.locator(markupCard); + await expect(card).toBeVisible(); + await expect(card.getByText('2 changes')).toBeVisible(); + // The diff rendered, so the card is the markup card and not the excerpt one. + await expect(card.locator('.atrium-critic-view-ins')).toBeVisible(); + + const clamp = card.locator('[data-testid="markup-clamp"]'); + await expect(clamp).toBeVisible(); + await expect.poll(async () => clamp.evaluate((el) => el.scrollHeight <= el.clientHeight + 1)).toBe(true); + + await expect(card.getByRole('button', { name: /Show all changes/ })).toHaveCount(0); + await expect(card.locator('[data-testid="markup-clamp-fade"]')).toHaveCount(0); + + await ctx.dispose(); +}); + +test('a markup diff that overflows offers a toggle and a fade, and expanding clears both', async ({ page }) => { + const room = await createTestChannel('markup-overflows'); + const handle = unique('markup-reader'); + const ctx = await apiAs(handle); + const id = await channelId(ctx, room); + + // Long enough to exceed the card's 19.6rem clamp at any sane viewport. + const artifact = await seedArtifact({ channelId: id, body: markupBody(14) }); + await postMessage(ctx, id, `seeded overflow — /e/${artifact.handle}`); + + await login(page, handle); + await openChannel(page, room); + + const card = page.locator(markupCard); + await expect(card).toBeVisible(); + const clamp = card.locator('[data-testid="markup-clamp"]'); + + // The measurement must actually run against real layout: a ref that attaches + // on a later render than the hook is invisible to an effect, and the toggle + // below could never appear at all. + await expect.poll(async () => clamp.evaluate((el) => el.scrollHeight > el.clientHeight + 1)).toBe(true); + + const collapsedHeight = await clamp.evaluate((el) => Math.round(el.getBoundingClientRect().height)); + const toggle = card.getByRole('button', { name: /Show all changes/ }); + await expect(toggle).toBeVisible(); + await expect(card.locator('[data-testid="markup-clamp-fade"]')).toBeVisible(); + await expect(toggle).toHaveAttribute('aria-expanded', 'false'); + + await toggle.click(); + + // Show more must GROW the diff. A nested clamp once made this shrink instead. + const expandedHeight = await clamp.evaluate((el) => Math.round(el.getBoundingClientRect().height)); + expect(expandedHeight).toBeGreaterThan(collapsedHeight); + await expect(card.getByRole('button', { name: 'Show fewer changes' })).toBeVisible(); + await expect(card.locator('[data-testid="markup-clamp-fade"]')).toHaveCount(0); + + await card.getByRole('button', { name: 'Show fewer changes' }).click(); + await expect(toggle).toBeVisible(); + await expect + .poll(async () => clamp.evaluate((el) => Math.round(el.getBoundingClientRect().height))) + .toBe(collapsedHeight); + + await ctx.dispose(); +}); + +test('the collapsed clamp keeps an absolutely positioned descendant inside its clip', async ({ page }) => { + // #544: `overflow: hidden` only clips descendants whose containing block runs + // through the clamping box, so a clamp holding `position: absolute` content + // must be `relative`. A hunk separator makes CriticMarkupView render one: an + // `sr-only` span, which Tailwind positions absolutely. + // + // Honest scope: this guards the invariant, not a measured regression. Unlike + // #544's footnote heading in MessageText — which escaped and inflated the + // channel's scroll height by ~3000px — this span is 1px with auto offsets, so + // escaping costs nothing observable (measured: scrollHeight is identical with + // the clamp static and relative). It is the containing block that must hold, + // because the next absolutely positioned thing this view renders may have size. + const room = await createTestChannel('markup-clip'); + const handle = unique('markup-reader'); + const ctx = await apiAs(handle); + const id = await channelId(ctx, room); + + const artifact = await seedArtifact({ channelId: id, body: markupBody(14, { separator: true }) }); + await postMessage(ctx, id, `seeded clip — /e/${artifact.handle}`); + + await login(page, handle); + await openChannel(page, room); + + const clamp = page.locator(markupCard).locator('[data-testid="markup-clamp"]'); + await expect(clamp).toBeVisible(); + + // The hazard has to be present, or this test guards nothing. + await expect(clamp.locator('.atrium-critic-view-separator')).toHaveCount(1); + await expect(clamp.locator('.sr-only')).toHaveCSS('position', 'absolute'); + await expect(clamp).toHaveCSS('position', 'relative'); + + // The clipped box stays at its clamped height no matter what it contains. + const height = await clamp.evaluate((el) => Math.round(el.getBoundingClientRect().height)); + expect(height).toBeLessThanOrEqual(Math.ceil(19.6 * 16) + 2); + + await ctx.dispose(); +}); diff --git a/surface/web/src/components/EntryQuoteCard.tsx b/surface/web/src/components/EntryQuoteCard.tsx index b6012442..09ab6c7e 100644 --- a/surface/web/src/components/EntryQuoteCard.tsx +++ b/surface/web/src/components/EntryQuoteCard.tsx @@ -525,6 +525,7 @@ export function EntryQuoteCard({