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

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,
       },
     },
     {

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/<id>/…`) for a reader
+ * to see it — a `scratch/<session>/…` 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<string> {
+  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;
+}

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();
+});

surface/web/src/components/EntryQuoteCard.tsx

@@ -525,6 +525,7 @@ export function EntryQuoteCard({
         <div className="relative mt-2">
           <div
             ref={changes.contentRef}
+            data-testid="markup-clamp"
             // `relative` is load-bearing: `overflow: hidden` only clips
             // descendants whose containing block runs through this box.
             // CriticMarkupView renders a `sr-only` span (position: absolute), and