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({