diff --git a/.gitignore b/.gitignore index dc3c3d28..1ab099e2 100644 --- a/.gitignore +++ b/.gitignore @@ -42,3 +42,8 @@ execplan/ # Generated npm bundle output (local) cli/npm/main/ .ace-tool/ + +# Playwright e2e artifacts +test-results/ +playwright-report/ +e2e-output/ diff --git a/bun.lock b/bun.lock index 9ddaf407..29aa6863 100644 --- a/bun.lock +++ b/bun.lock @@ -5,8 +5,9 @@ "": { "name": "hapi", "devDependencies": { + "@playwright/test": "^1.60.0", "concurrently": "^9.2.1", - "playwright": "1.49.1", + "playwright": "1.60.0", "react-devtools-core": "^7.0.1", "vite-plugin-pwa": "^1.2.0", }, @@ -714,6 +715,8 @@ "@pinojs/redact": ["@pinojs/redact@0.4.0", "", {}, "sha512-k2ENnmBugE/rzQfEcdWHcCY+/FM3VLzH9cYEsbdsoqrvzAKRhUZeRNhAZvB8OitQJ1TBed3yqWtdjzS6wJKBwg=="], + "@playwright/test": ["@playwright/test@1.60.0", "", { "dependencies": { "playwright": "1.60.0" }, "bin": { "playwright": "cli.js" } }, "sha512-O71yZIbAh/PxDMNGns37GHBIfrVkEVyn+AXyIa5dOTfb4/xNvRWV+Vv/NMbNCtODB/pO7vLlF2OTmMVLhmr7Ag=="], + "@primer/octicons": ["@primer/octicons@19.23.1", "", { "dependencies": { "object-assign": "^4.1.1" } }, "sha512-CzjGmxkmNhyst6EekrS3SJPdtzgIkUMP/LSJch65y99/kmiFXbO1a+q7zoYe3hnI9NaOM0IN+ydDIbOmd8YqcA=="], "@radix-ui/number": ["@radix-ui/number@1.1.1", "", {}, "sha512-MkKCwxlXTgz6CFoJx3pCwn07GKp36+aZyu/u2Ln2VrA5DcdyCZkASEDBTd8x5whTQQL5CiYf4prXKLcgQdv29g=="], @@ -1066,6 +1069,8 @@ "@twsxtd/hapi-linux-x64": ["@twsxtd/hapi-linux-x64@0.19.0", "", { "os": "linux", "cpu": "x64", "bin": { "hapi": "bin/hapi" } }, "sha512-Xz2UD5fMZPSV0OPhzDo5FE7Tbs4wcHmjK3+NEu4kWTQkslMeQ+j96syR32MfbO1CzIz2kbDckVsnGj+ptfki9A=="], + "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.19.0", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-tGUAG2CKhoLWaCROEjB0sEPFojYp4mU2wyJ4p/V7Q6WO1ayXQHT3dxYo0MvXOFN/u4EXzVJYOTKwa6IKDks0vA=="], + "@types/aria-query": ["@types/aria-query@5.0.4", "", {}, "sha512-rfT93uj5s0PRL7EzccGMs3brplhcrghnDoV26NqKhCAS1hVo+WdNsPvE/yb6ilfr5hi2MEk6d5EWJTKdxg8jVw=="], "@types/babel__core": ["@types/babel__core@7.20.5", "", { "dependencies": { "@babel/parser": "^7.20.7", "@babel/types": "^7.20.7", "@types/babel__generator": "*", "@types/babel__template": "*", "@types/babel__traverse": "*" } }, "sha512-qoQprZvz5wQFJwMDqeseRXWv3rqMvhgpbXFfVyWhbx9X47POIA6i/+dXefEmZKoAgOaTdaIgNSMqMIU61yRyzA=="], @@ -2430,9 +2435,9 @@ "pkg-types": ["pkg-types@1.3.1", "", { "dependencies": { "confbox": "^0.1.8", "mlly": "^1.7.4", "pathe": "^2.0.1" } }, "sha512-/Jm5M4RvtBFVkKWRu2BLUTNP8/M2a+UwuAX+ae4770q1qVGtfjG+WTCupoZixokjmHiry8uI+dlY8KXYV5HVVQ=="], - "playwright": ["playwright@1.49.1", "", { "dependencies": { "playwright-core": "1.49.1" }, "optionalDependencies": { "fsevents": "2.3.2" }, "bin": { "playwright": "cli.js" } }, "sha512-VYL8zLoNTBxVOrJBbDuRgDWa3i+mfQgDTrL8Ah9QXZ7ax4Dsj0MSq5bYgytRnDVVe+njoKnfsYkH3HzqVj5UZA=="], + "playwright": ["playwright@1.60.0", "", { "dependencies": { "playwright-core": "1.60.0" }, "optionalDependencies": { "fsevents": "2.3.2" }, "bin": { "playwright": "cli.js" } }, "sha512-hheHdokM8cdqCb0lcE3s+zT4t4W+vvjpGxsZlDnikarzx8tSzMebh3UiFtgqwFwnTnjYQcsyMF8ei2mCO/tpeA=="], - "playwright-core": ["playwright-core@1.49.1", "", { "bin": { "playwright-core": "cli.js" } }, "sha512-BzmpVcs4kE2CH15rWfzpjzVGhWERJfmnXmniSyKeRZUs9Ws65m+RGIi7mjJK/euCegfn3i7jvqWeWyHe9y3Vgg=="], + "playwright-core": ["playwright-core@1.60.0", "", { "bin": { "playwright-core": "cli.js" } }, "sha512-9bW6zvX/m0lEbgTKJ6YppOKx8H3VOPBMOCFh2irXFOT4BbHgrx5hPjwJYLT40Lu+4qtD36qKc/Hn56StUW57IA=="], "pngjs": ["pngjs@5.0.0", "", {}, "sha512-40QW5YalBNfQo5yRYmiw7Yz6TKKVr3h6970B2YE+3fQpsWcrbj1PzJgxeJ19DRQjhMbKPIuMY8rFaXc8moolVw=="], diff --git a/e2e/scratchlist.spec.ts b/e2e/scratchlist.spec.ts new file mode 100644 index 00000000..0f279dd8 --- /dev/null +++ b/e2e/scratchlist.spec.ts @@ -0,0 +1,318 @@ +/* + * End-to-end coverage for the per-session scratchlist (issue #11 / PR + * #772). The unit tests in `web/src/components/AssistantChat/ + * ScratchlistPanel.test.tsx` exercise the component under jsdom, which + * does not honor `inert` for focus blocking, does not run the CSS + * `grid-template-rows` collapse animation, and does not exercise real + * `localStorage` round-tripping across full page loads. + * + * These specs drive a real Chromium against the + * `web/e2e-fixtures/scratchlist-fixture.html` page (vite dev), which + * mounts the production ScratchlistPanel + I18nProvider with stub + * promote callbacks exposed on `window.__scratchlistE2E`. + * + * Each test uses a unique `?session=...` query param so the keyed + * localStorage state is naturally isolated. + */ + +import { test, expect, Page } from '@playwright/test' + +type Harness = { + sessionId: string + promotedToComposer: string[] + promotedToQueue: string[] + queueSendMode: 'success' | 'failure' +} + +async function gotoFixture(page: Page, sessionId: string): Promise { + // We use a unique session id per test (the localStorage keys are + // namespaced by sessionId), so isolation is naturally per-test + // without needing to clear storage. Clearing on every page load + // would defeat the persistence + cross-navigation tests below. + await page.goto(`/e2e-fixtures/scratchlist-fixture.html?session=${encodeURIComponent(sessionId)}`) + await expect(page.getByTestId('scratchlist-panel')).toBeVisible() +} + +async function readHarness(page: Page): Promise { + return await page.evaluate(() => { + const h = window.__scratchlistE2E! + return { + sessionId: h.sessionId, + promotedToComposer: [...h.promotedToComposer], + promotedToQueue: [...h.promotedToQueue], + queueSendMode: h.queueSendMode, + } + }) +} + +async function setQueueMode(page: Page, mode: 'success' | 'failure'): Promise { + await page.evaluate((m) => { + if (window.__scratchlistE2E) { + window.__scratchlistE2E.queueSendMode = m + } + }, mode) +} + +async function expandPanel(page: Page): Promise { + const toggle = page.getByRole('button', { name: 'Scratchlist' }) + if ((await toggle.getAttribute('aria-expanded')) !== 'true') { + await toggle.click() + } + await expect(toggle).toHaveAttribute('aria-expanded', 'true') +} + +async function collapsePanel(page: Page): Promise { + const toggle = page.getByRole('button', { name: 'Scratchlist' }) + if ((await toggle.getAttribute('aria-expanded')) !== 'false') { + await toggle.click() + } + await expect(toggle).toHaveAttribute('aria-expanded', 'false') +} + +async function addEntry(page: Page, text: string): Promise { + const textarea = page.getByLabel('Add scratchlist entry') + await textarea.fill(text) + await page.getByRole('button', { name: 'Add', exact: true }).click() + await expect(textarea).toHaveValue('') +} + +test.describe('scratchlist e2e', () => { + test('starts collapsed, expands on click, collapses on second click', async ({ page }) => { + await gotoFixture(page, 'expand') + + const toggle = page.getByRole('button', { name: 'Scratchlist' }) + await expect(toggle).toHaveAttribute('aria-expanded', 'false') + + await toggle.click() + await expect(toggle).toHaveAttribute('aria-expanded', 'true') + + await toggle.click() + await expect(toggle).toHaveAttribute('aria-expanded', 'false') + }) + + test('collapsed inner is `inert`: textarea cannot be focused or clicked', async ({ page }) => { + // This is the regression that the upstream PR review flagged. + // jsdom can't verify it; only a real browser can. + await gotoFixture(page, 'inert') + + const inner = page.locator('.collapsible-inner').first() + await expect(inner).toHaveAttribute('inert', '') + + // Try to focus the (collapsed) textarea. Per the HTML spec, an + // inert subtree refuses focus. + const textarea = page.getByLabel('Add scratchlist entry') + await textarea.focus({ timeout: 1_000 }).catch(() => {}) + const focusedTagCollapsed = await page.evaluate( + () => document.activeElement?.tagName ?? 'NONE' + ) + expect(focusedTagCollapsed).not.toBe('TEXTAREA') + + // Clicks on inert children also have no effect: the panel must + // not collapse / submit / fill, the click is swallowed. + await textarea.click({ force: true, timeout: 1_000 }).catch(() => {}) + const stillCollapsed = await page + .getByRole('button', { name: 'Scratchlist' }) + .getAttribute('aria-expanded') + expect(stillCollapsed).toBe('false') + + // Expand and confirm focus works again. + await expandPanel(page) + const innerAttrAfter = await inner.getAttribute('inert') + expect(innerAttrAfter === null || innerAttrAfter === '' || innerAttrAfter === 'false').toBeTruthy() + await textarea.focus() + const focusedAfter = await page.evaluate( + () => document.activeElement?.tagName ?? 'NONE' + ) + expect(focusedAfter).toBe('TEXTAREA') + }) + + test('add: entry appears, draft clears, count updates', async ({ page }) => { + await gotoFixture(page, 'add') + await expandPanel(page) + + // Initial summary is "empty". + await expect(page.getByText('empty', { exact: true })).toBeVisible() + + await addEntry(page, 'Investigate the runner cold-start delay') + + await expect(page.getByText('Investigate the runner cold-start delay')).toBeVisible() + await expect(page.getByText('1 item', { exact: true })).toBeVisible() + + await addEntry(page, 'Second draft') + await expect(page.getByText('2 items', { exact: true })).toBeVisible() + }) + + test('persistence: entries survive a full page reload', async ({ page }) => { + await gotoFixture(page, 'persist') + await expandPanel(page) + await addEntry(page, 'First note') + await addEntry(page, 'Second note') + + await page.reload() + await expect(page.getByTestId('scratchlist-panel')).toBeVisible() + + // Collapsed-state preference is also remembered, so we expand + // again before asserting both entries reappear. The storage + // layer renders newest-first, so the most recent add is at + // index 0. + await expandPanel(page) + const items = page.locator('[data-testid="scratchlist-entry"]') + await expect(items).toHaveCount(2) + await expect(items.nth(0)).toContainText('Second note') + await expect(items.nth(1)).toContainText('First note') + }) + + test('promote-to-composer fires callback with entry text', async ({ page }) => { + await gotoFixture(page, 'promote-composer') + await expandPanel(page) + await addEntry(page, 'Draft a status update') + + await page.getByRole('button', { name: 'Copy into composer' }).first().click() + + const harness = await readHarness(page) + expect(harness.promotedToComposer).toEqual(['Draft a status update']) + // Promote-to-composer is a copy: the entry stays. + await expect(page.getByText('Draft a status update')).toBeVisible() + }) + + test('promote-to-queue (success) fires callback and removes entry', async ({ page }) => { + await gotoFixture(page, 'promote-queue') + await expandPanel(page) + await addEntry(page, 'Ship the patch release') + + await setQueueMode(page, 'success') + await page.getByRole('button', { name: 'Send to queue' }).first().click() + + await expect(page.getByText('Ship the patch release')).toHaveCount(0) + const harness = await readHarness(page) + expect(harness.promotedToQueue).toEqual(['Ship the patch release']) + }) + + test('promote-to-queue (failure) keeps the entry on the scratchlist', async ({ page }) => { + await gotoFixture(page, 'promote-queue-fail') + await expandPanel(page) + await addEntry(page, 'This send will fail') + + await setQueueMode(page, 'failure') + await page.getByRole('button', { name: 'Send to queue' }).first().click() + + // Failure path: the queue callback returned false, so the entry + // must remain on the scratchlist (operator can retry). + await expect(page.getByText('This send will fail')).toBeVisible() + const harness = await readHarness(page) + expect(harness.promotedToQueue).toEqual([]) + }) + + test('Ctrl+Shift+S expands the panel and focuses the input', async ({ page }) => { + await gotoFixture(page, 'shortcut') + await collapsePanel(page) + + await page.keyboard.press('Control+Shift+S') + + await expect(page.getByRole('button', { name: 'Scratchlist' })).toHaveAttribute( + 'aria-expanded', + 'true' + ) + await expect.poll( + async () => page.evaluate(() => document.activeElement?.tagName ?? 'NONE'), + { timeout: 2_000 } + ).toBe('TEXTAREA') + }) + + test('regression: in-place sessionId change does not leak entries (host keyed by sessionId)', async ({ page }) => { + // Reproduces the bug flagged by the upstream PR review: + // ScratchlistPanel reads `sessionId` once via useState and + // rehydrates in useEffect. If a parent stays mounted across + // session changes (SessionChat does, on same-route nav), the + // persist effect for the stale entries fires under the new + // sessionId BEFORE the rehydrate effect's setEntries triggers a + // correction render. That race writes A's entries into B's + // localStorage, even though the second render then overwrites + // it with [] - leaving a transient bad write in storage that + // a tab/network race could observe. + // + // Reading localStorage AFTER the dust settles is too late: the + // bug write has already been overwritten by the correction + // write. We instead install a setItem spy BEFORE mount so every + // write during the session switch is recorded, then assert no + // write to the new sessionId's key contained the old session's + // entry text. This catches the race deterministically. + // + // The fix is `key={sessionId}` on ScratchlistHost in + // SessionChat.tsx (and the equivalent `keyed=true` path in + // this fixture). With the key, React unmounts/remounts on + // session change, so the new mount reads B's storage from + // scratch and never touches B's key with A's data. + await page.addInitScript(() => { + const writes: { key: string; value: string }[] = [] + const orig = window.localStorage.setItem.bind(window.localStorage) + window.localStorage.setItem = (k: string, v: string) => { + writes.push({ key: String(k), value: String(v) }) + return orig(k, v) + } + ;(window as unknown as { __lsWrites: typeof writes }).__lsWrites = writes + }) + + await page.goto(`/e2e-fixtures/scratchlist-fixture.html?session=leak-A`) + await expect(page.getByTestId('scratchlist-panel')).toBeVisible() + await expandPanel(page) + await addEntry(page, 'A-only entry') + await expect(page.getByText('A-only entry')).toBeVisible() + + // Clear the recorded writes from the setup phase so the + // assertion below only inspects writes that happened DURING + // the session switch. + await page.evaluate(() => { + ;(window as unknown as { __lsWrites: { key: string; value: string }[] }).__lsWrites.length = 0 + }) + + // Switch sessionId in-place WITHOUT reloading the parent. + await page.evaluate(() => window.__scratchlistE2E!.setSessionId('leak-B')) + + // Wait for the dust to settle (effects + re-render). + await expect(page.getByRole('button', { name: 'Scratchlist' })).toHaveAttribute( + 'aria-expanded', + 'false' + ) + + // No write to leak-B's storage key should contain A's entry. + const writes = await page.evaluate( + () => (window as unknown as { __lsWrites: { key: string; value: string }[] }).__lsWrites.slice() + ) + const leakBKey = 'hapi.scratchlist.v1.leak-B' + const corruptingWrite = writes.find( + (w) => w.key === leakBKey && w.value.includes('A-only entry') + ) + expect(corruptingWrite, `found corrupting write to ${leakBKey}: ${JSON.stringify(corruptingWrite)}`).toBeUndefined() + + // Final state assertions: leak-B is empty, leak-A retains its + // entry on round-trip back. + await expandPanel(page) + await expect(page.getByText('empty', { exact: true })).toBeVisible() + await expect(page.getByText('A-only entry')).toHaveCount(0) + + await page.evaluate(() => window.__scratchlistE2E!.setSessionId('leak-A')) + await expandPanel(page) + await expect(page.getByText('A-only entry')).toBeVisible() + }) + + test('per-session isolation: full reload across sessions does not leak entries', async ({ page }) => { + await gotoFixture(page, 'session-a') + await expandPanel(page) + await addEntry(page, 'Note for session A') + + // Navigate to a different session id; localStorage is keyed by + // `hapi.scratchlist.v1.`, so session B must start + // empty even though A still has its entry persisted. + await page.goto(`/e2e-fixtures/scratchlist-fixture.html?session=session-b`) + await expect(page.getByTestId('scratchlist-panel')).toBeVisible() + await expandPanel(page) + await expect(page.getByText('empty', { exact: true })).toBeVisible() + await expect(page.getByText('Note for session A')).toHaveCount(0) + + // And navigating back to A still shows its note. + await page.goto(`/e2e-fixtures/scratchlist-fixture.html?session=session-a`) + await expandPanel(page) + await expect(page.getByText('Note for session A')).toBeVisible() + }) +}) diff --git a/package.json b/package.json index 3d57f453..c33c2acc 100644 --- a/package.json +++ b/package.json @@ -23,12 +23,15 @@ "test:hub": "cd hub && bun run test", "test:web": "cd web && bun run test", "test:shared": "cd shared && bun run test", + "test:e2e": "playwright test", + "test:e2e:ui": "playwright test --ui", "clean-session": "bun run hub/scripts/cleanup-sessions.ts", "release-all": "cd cli && bun run release-all" }, "devDependencies": { + "@playwright/test": "^1.60.0", "concurrently": "^9.2.1", - "playwright": "1.49.1", + "playwright": "1.60.0", "react-devtools-core": "^7.0.1", "vite-plugin-pwa": "^1.2.0" } diff --git a/playwright.config.ts b/playwright.config.ts new file mode 100644 index 00000000..779efd16 --- /dev/null +++ b/playwright.config.ts @@ -0,0 +1,48 @@ +import { defineConfig, devices } from '@playwright/test' + +const PORT = 5179 +const BASE_URL = `http://localhost:${PORT}` + +export default defineConfig({ + testDir: './e2e', + timeout: 30_000, + expect: { timeout: 5_000 }, + fullyParallel: false, + forbidOnly: !!process.env.CI, + retries: process.env.CI ? 1 : 0, + workers: 1, + reporter: process.env.CI ? [['list'], ['html', { open: 'never' }]] : 'list', + use: { + baseURL: BASE_URL, + trace: 'retain-on-failure', + screenshot: 'only-on-failure', + }, + projects: [ + { + name: 'chromium', + use: { + ...devices['Desktop Chrome'], + launchOptions: { + // The CI runner and most sandboxed dev environments + // run as root or under restricted user namespaces; + // without --no-sandbox chromium silently exits 0 a + // few seconds after launch and the page handshake + // times out. Keep the flag scoped to launchOptions + // so this is the only place a future maintainer has + // to revisit if they harden the runner. + args: ['--no-sandbox'], + }, + }, + }, + ], + webServer: { + // The fixture page mounts ScratchlistPanel in isolation; no hub + // is required, which is why this dev server doesn't proxy /api. + command: `bun run --cwd web dev -- --port ${PORT} --strictPort`, + url: `${BASE_URL}/e2e-fixtures/scratchlist-fixture.html`, + timeout: 60_000, + reuseExistingServer: !process.env.CI, + stdout: 'ignore', + stderr: 'pipe', + }, +}) diff --git a/web/README.md b/web/README.md index 4336d590..8e384a20 100644 --- a/web/README.md +++ b/web/README.md @@ -51,6 +51,11 @@ See `src/router.tsx` for route definitions. - Model selection (default/sonnet/sonnet[1m]/opus/opus[1m]). - Session abort and mode switch controls. - Context size display. +- Per-session scratchlist (`src/components/AssistantChat/ScratchlistPanel.tsx`) + - Workbench panel for held notes/drafts; **distinct from the queue**. + - Add/delete/reorder entries; promote to composer (copy) or queue (send). + - Persists across reloads via `localStorage` keyed per session. + - Keyboard shortcut: Ctrl/Cmd+Shift+S to focus the add-input. ### File browser (`src/routes/sessions/files.tsx`) @@ -135,6 +140,27 @@ If testing in Telegram, set: - `HAPI_PUBLIC_URL` to the public HTTPS URL of the dev server. - `CORS_ORIGINS` to include the dev server origin. +## Tests + +Unit tests run under vitest + jsdom: + +```bash +bun run test:web +``` + +End-to-end browser tests for the scratchlist component (real Chromium, real +`inert` focus blocking, real localStorage round-trips) live at the repo root +under `e2e/`: + +```bash +bun run test:e2e # headless +bun run test:e2e:ui # Playwright UI mode (debug) +``` + +The spec drives a Vite-served fixture page (`web/e2e-fixtures/scratchlist-fixture.html`) +that mounts the production `ScratchlistPanel` in isolation, so no hub / +auth / socket setup is required. + ## Build ```bash diff --git a/web/e2e-fixtures/scratchlist-fixture.html b/web/e2e-fixtures/scratchlist-fixture.html new file mode 100644 index 00000000..8071370c --- /dev/null +++ b/web/e2e-fixtures/scratchlist-fixture.html @@ -0,0 +1,18 @@ + + + + + + HAPI scratchlist e2e fixture + + + +
+ + + diff --git a/web/e2e-fixtures/scratchlist-fixture.tsx b/web/e2e-fixtures/scratchlist-fixture.tsx new file mode 100644 index 00000000..aee6dd7e --- /dev/null +++ b/web/e2e-fixtures/scratchlist-fixture.tsx @@ -0,0 +1,130 @@ +/* + * Standalone Vite-served fixture for the scratchlist Playwright e2e + * spec. Mounts only the ScratchlistPanel inside an I18nProvider so the + * spec can drive a real browser against the real component without + * having to mock the entire HAPI auth + socket stack. + * + * The session id is read from the `?session=...` query param (default + * `e2e`) so individual specs can isolate localStorage state simply by + * navigating to a unique URL. + * + * The fixture also exposes `window.__scratchlistE2E.setSessionId(id)` + * so a spec can switch sessions WITHOUT a full page reload — this + * reproduces the SessionChat pattern where the parent stays mounted + * across same-route navigation. Used by the regression test for the + * "stale entries leak from session A into session B" bug fixed in + * `SessionChat.tsx` by keying the host by `session.id`. + * + * Promote callbacks are exposed on `window.__scratchlistE2E` so the + * spec can assert that the right text reached `setText` (composer) + * and `onSend` (queue) without involving the real composer / queue. + */ + +import React from 'react' +import ReactDOM from 'react-dom/client' +import '../src/index.css' +import { I18nProvider } from '../src/lib/i18n-context' +import { ScratchlistPanel } from '../src/components/AssistantChat/ScratchlistPanel' + +declare global { + interface Window { + __scratchlistE2E?: { + sessionId: string + promotedToComposer: string[] + promotedToQueue: string[] + queueSendMode: 'success' | 'failure' + /** Whether the fixture's host wrapper applies `key={sessionId}`. + * Mirrors the SessionChat fix; toggle via `?key=0` to repro the + * pre-fix bug for red/green tests. Defaults to `true`. */ + keyByedSessionId: boolean + setSessionId(id: string): void + reset(): void + } + } +} + +function getInitialSessionId(): string { + const url = new URL(window.location.href) + return url.searchParams.get('session') ?? 'e2e' +} + +function getKeyByedSessionId(): boolean { + const url = new URL(window.location.href) + const raw = url.searchParams.get('key') + if (raw === '0' || raw === 'false') return false + return true +} + +/* + * Mirror of SessionChat's ScratchlistHost: a thin wrapper that owns + * the promote callbacks. The spec drives sessionId changes through + * the parent (App), while this host either keys by sessionId + * (production behaviour) or doesn't (pre-fix repro). + */ +function ScratchlistHost({ sessionId, keyed }: { sessionId: string; keyed: boolean }) { + const handlePromoteToComposer = React.useCallback((text: string) => { + window.__scratchlistE2E?.promotedToComposer.push(text) + }, []) + + const handlePromoteToQueue = React.useCallback(async (text: string) => { + const harness = window.__scratchlistE2E + if (!harness) return false + if (harness.queueSendMode === 'failure') { + return false + } + harness.promotedToQueue.push(text) + return true + }, []) + + return ( + + ) +} + +function App() { + const [sessionId, setSessionId] = React.useState(() => getInitialSessionId()) + const keyed = React.useMemo(() => getKeyByedSessionId(), []) + + React.useEffect(() => { + const harness: NonNullable = { + sessionId, + promotedToComposer: [], + promotedToQueue: [], + queueSendMode: 'success', + keyByedSessionId: keyed, + setSessionId: (id: string) => setSessionId(id), + reset() { + this.promotedToComposer = [] + this.promotedToQueue = [] + this.queueSendMode = 'success' + }, + } + window.__scratchlistE2E = harness + }, [keyed]) + + React.useEffect(() => { + if (window.__scratchlistE2E) { + window.__scratchlistE2E.sessionId = sessionId + } + }, [sessionId]) + + return ( + + + + ) +} + +const rootEl = document.getElementById('root') +if (rootEl) { + ReactDOM.createRoot(rootEl).render( + + + + ) +} diff --git a/web/src/components/AssistantChat/ScratchlistPanel.test.tsx b/web/src/components/AssistantChat/ScratchlistPanel.test.tsx new file mode 100644 index 00000000..06bb4de3 --- /dev/null +++ b/web/src/components/AssistantChat/ScratchlistPanel.test.tsx @@ -0,0 +1,237 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { I18nProvider } from '@/lib/i18n-context' +import { + persistScratchlist, + readScratchlist, + type ScratchlistEntry, +} from '@/lib/scratchlist' +import { ScratchlistPanel } from './ScratchlistPanel' + +const SID = 'session-test' + +function renderPanel(props?: { + onPromoteToComposer?: (text: string) => void + onPromoteToQueue?: (text: string) => Promise + sessionId?: string +}) { + const onPromoteToComposer = props?.onPromoteToComposer ?? vi.fn() + const onPromoteToQueue = props?.onPromoteToQueue ?? vi.fn(async () => true) + return { + onPromoteToComposer, + onPromoteToQueue, + ...render( + + + + ), + } +} + +function makeEntry(overrides: Partial & { id: string }): ScratchlistEntry { + return { + text: 'note', + createdAt: 1000, + ...overrides, + } +} + +function expandPanel(): void { + fireEvent.click(screen.getByRole('button', { name: /Scratchlist/ })) +} + +afterEach(() => { + cleanup() +}) + +beforeEach(() => { + localStorage.clear() +}) + +describe('ScratchlistPanel', () => { + it('renders the held / not-sent label so users distinguish it from the queue', () => { + renderPanel() + // The held-label is rendered inside the toggle button as visual chrome + // (aria-hidden) so use textContent rather than a name match. + const toggle = screen.getByRole('button', { name: /Scratchlist/ }) + expect(toggle.textContent).toContain('held') + }) + + it('starts collapsed by default; clicking the header expands it', () => { + renderPanel() + const toggle = screen.getByRole('button', { name: /Scratchlist/ }) + expect(toggle.getAttribute('aria-expanded')).toBe('false') + expandPanel() + expect(toggle.getAttribute('aria-expanded')).toBe('true') + }) + + it('marks the inner content `inert` while collapsed so hidden controls are not focusable', () => { + // Regression guard: upstream PR review flagged that under the + // CSS-only collapse the textarea + action buttons were still in + // the focus / a11y tree. The fix is `inert` on the inner; this + // test fails if anyone reverts that. + const { container } = renderPanel() + const inner = container.querySelector('.collapsible-inner') + expect(inner).not.toBeNull() + expect(inner!.hasAttribute('inert')).toBe(true) + + expandPanel() + // jsdom doesn't always reflect the React `inert={false}` prop as + // an attribute removal — accept either "absent" or empty string, + // which both indicate non-inert per the HTML spec. + const value = inner!.getAttribute('inert') + expect(value === null || value === 'false' || value === '').toBe(true) + }) + + it('hydrates entries that were persisted before mount', () => { + persistScratchlist(SID, [ + makeEntry({ id: 'persisted-1', text: 'persisted note' }), + ]) + renderPanel() + expandPanel() + expect(screen.getByText('persisted note')).toBeTruthy() + }) + + it('adds a new entry via the add button and persists it', () => { + renderPanel() + expandPanel() + const input = screen.getByLabelText('Add scratchlist entry') as HTMLTextAreaElement + fireEvent.change(input, { target: { value: 'first thought' } }) + fireEvent.click(screen.getByRole('button', { name: 'Add' })) + expect(screen.getByText('first thought')).toBeTruthy() + const stored = readScratchlist(SID) + expect(stored.map((e) => e.text)).toEqual(['first thought']) + }) + + it('adds a new entry on Enter; Shift+Enter does not add (preserves newline)', () => { + renderPanel() + expandPanel() + const input = screen.getByLabelText('Add scratchlist entry') as HTMLTextAreaElement + fireEvent.change(input, { target: { value: 'enter add' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + expect(screen.getByText('enter add')).toBeTruthy() + expect(readScratchlist(SID).map((e) => e.text)).toEqual(['enter add']) + + // Shift+Enter must not promote to a new entry (it falls through to + // textarea default newline behavior); the stored list stays unchanged. + fireEvent.change(input, { target: { value: 'with newline' } }) + fireEvent.keyDown(input, { key: 'Enter', shiftKey: true }) + expect(readScratchlist(SID).map((e) => e.text)).toEqual(['enter add']) + }) + + it('deletes an entry without a confirm prompt for short entries', () => { + persistScratchlist(SID, [makeEntry({ id: 'a', text: 'short' })]) + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true) + renderPanel() + expandPanel() + fireEvent.click(screen.getByRole('button', { name: 'Delete entry' })) + expect(confirmSpy).not.toHaveBeenCalled() + expect(screen.queryByText('short')).toBeNull() + expect(readScratchlist(SID)).toEqual([]) + confirmSpy.mockRestore() + }) + + it('asks for confirmation before deleting long entries (>100 chars)', () => { + const longText = 'x'.repeat(150) + persistScratchlist(SID, [makeEntry({ id: 'a', text: longText })]) + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false) + + renderPanel() + expandPanel() + fireEvent.click(screen.getByRole('button', { name: 'Delete entry' })) + + expect(confirmSpy).toHaveBeenCalled() + // Confirm rejected — entry stays. + expect(readScratchlist(SID).map((e) => e.id)).toEqual(['a']) + + confirmSpy.mockReturnValue(true) + fireEvent.click(screen.getByRole('button', { name: 'Delete entry' })) + expect(readScratchlist(SID)).toEqual([]) + confirmSpy.mockRestore() + }) + + it('reorders entries via the up / down arrow buttons', () => { + persistScratchlist(SID, [ + makeEntry({ id: 'top', text: 'top entry' }), + makeEntry({ id: 'bot', text: 'bot entry' }), + ]) + renderPanel() + expandPanel() + + // First entry is at index 0 — its up-button should be disabled. + const upButtons = screen.getAllByRole('button', { name: 'Move entry up' }) + const downButtons = screen.getAllByRole('button', { name: 'Move entry down' }) + expect(upButtons[0]?.hasAttribute('disabled')).toBe(true) + expect(downButtons[downButtons.length - 1]?.hasAttribute('disabled')).toBe(true) + + // Move bottom row up -> swaps order. + fireEvent.click(upButtons[1] as HTMLButtonElement) + const stored = readScratchlist(SID) + expect(stored.map((e) => e.id)).toEqual(['bot', 'top']) + }) + + it('promote-to-composer copies text via the callback and keeps the entry', () => { + persistScratchlist(SID, [makeEntry({ id: 'a', text: 'compose me' })]) + const onPromoteToComposer = vi.fn() + renderPanel({ onPromoteToComposer }) + expandPanel() + fireEvent.click(screen.getByRole('button', { name: 'Copy into composer' })) + expect(onPromoteToComposer).toHaveBeenCalledWith('compose me') + // Entry remains: promote-to-composer is a copy, not a move. + expect(readScratchlist(SID).map((e) => e.id)).toEqual(['a']) + }) + + it('promote-to-queue calls onSend and removes the entry on accepted send', async () => { + persistScratchlist(SID, [makeEntry({ id: 'a', text: 'queue me' })]) + const onPromoteToQueue = vi.fn(async () => true) + renderPanel({ onPromoteToQueue }) + expandPanel() + fireEvent.click(screen.getByRole('button', { name: 'Send to queue' })) + await waitFor(() => expect(onPromoteToQueue).toHaveBeenCalledWith('queue me')) + await waitFor(() => expect(screen.queryByText('queue me')).toBeNull()) + expect(readScratchlist(SID)).toEqual([]) + }) + + it('promote-to-queue keeps the entry when the send is rejected', async () => { + persistScratchlist(SID, [makeEntry({ id: 'a', text: 'queue me' })]) + const onPromoteToQueue = vi.fn(async () => false) + renderPanel({ onPromoteToQueue }) + expandPanel() + fireEvent.click(screen.getByRole('button', { name: 'Send to queue' })) + await waitFor(() => expect(onPromoteToQueue).toHaveBeenCalledWith('queue me')) + // Entry remains because the queue rejected the promotion. + expect(screen.getByText('queue me')).toBeTruthy() + expect(readScratchlist(SID).map((e) => e.id)).toEqual(['a']) + }) + + it('persists collapse state across mounts for the same session', () => { + const { unmount } = renderPanel() + expandPanel() + unmount() + + // Re-mount with the same session id; should remain expanded. + const second = renderPanel() + const toggle = second.getByRole('button', { name: /Scratchlist/ }) + expect(toggle.getAttribute('aria-expanded')).toBe('true') + }) + + it('isolates entries between sessions', () => { + persistScratchlist('session-A', [makeEntry({ id: 'a1', text: 'A note' })]) + persistScratchlist('session-B', [makeEntry({ id: 'b1', text: 'B note' })]) + + const a = renderPanel({ sessionId: 'session-A' }) + fireEvent.click(a.getByRole('button', { name: /Scratchlist/ })) + expect(a.getByText('A note')).toBeTruthy() + expect(a.queryByText('B note')).toBeNull() + a.unmount() + + const b = renderPanel({ sessionId: 'session-B' }) + fireEvent.click(b.getByRole('button', { name: /Scratchlist/ })) + expect(b.getByText('B note')).toBeTruthy() + expect(b.queryByText('A note')).toBeNull() + }) +}) diff --git a/web/src/components/AssistantChat/ScratchlistPanel.tsx b/web/src/components/AssistantChat/ScratchlistPanel.tsx new file mode 100644 index 00000000..b9f27451 --- /dev/null +++ b/web/src/components/AssistantChat/ScratchlistPanel.tsx @@ -0,0 +1,437 @@ +import { + type FormEvent as ReactFormEvent, + type KeyboardEvent as ReactKeyboardEvent, + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from 'react' +import { + addScratchlistEntry, + deleteScratchlistEntry, + moveScratchlistEntry, + persistScratchlist, + readScratchlist, + SCRATCHLIST_MAX_ENTRIES, + SCRATCHLIST_MAX_TEXT_LENGTH, + shouldConfirmDelete, + type ScratchlistEntry, +} from '@/lib/scratchlist' +import { useTranslation } from '@/lib/use-translation' + +const STORAGE_KEY_PREFIX = 'hapi.scratchlist-collapsed.v1.' + +function readCollapsedPref(sessionId: string): boolean { + if (typeof window === 'undefined') return true + try { + const raw = window.localStorage.getItem(`${STORAGE_KEY_PREFIX}${sessionId}`) + return raw === null ? true : raw === '1' + } catch { + return true + } +} + +function writeCollapsedPref(sessionId: string, collapsed: boolean): void { + if (typeof window === 'undefined') return + try { + window.localStorage.setItem( + `${STORAGE_KEY_PREFIX}${sessionId}`, + collapsed ? '1' : '0' + ) + } catch { + // Non-fatal. + } +} + +function NoteIcon() { + return ( + + ) +} + +function ChevronIcon({ open }: { open: boolean }) { + return ( + + ) +} + +function ArrowUpIcon() { + return ( + + ) +} + +function ArrowDownIcon() { + return ( + + ) +} + +function PencilIcon() { + return ( + + ) +} + +function SendIcon() { + return ( + + ) +} + +function TrashIcon() { + return ( + + ) +} + +/** + * Per-session scratchlist (issue #11) -- the operator's "workbench". + * + * Distinct from the queue (`QueuedMessagesBar`): + * - Queue = conveyor belt: messages auto-fire in order once the agent is idle. + * - Scratchlist = workbench: notes / drafts / parking-lot ideas held until the + * operator explicitly promotes them (to the composer or into the queue). + * + * The "held -- not sent" pill plus the amber accent is the visual signal + * that nothing here is being sent without an explicit action. + */ +export function ScratchlistPanel({ + sessionId, + onPromoteToComposer, + onPromoteToQueue, +}: { + sessionId: string + /** + * Copies the entry text into the composer for editing. Called with the + * raw entry text. Implementation lives in SessionChat (it owns the + * AssistantUI runtime that exposes setText). + */ + onPromoteToComposer: (text: string) => void + /** + * Sends the entry into the existing send-queue (same path as a normal + * composer send). Resolves true when the send was accepted, false when + * pre-mutation guards rejected it -- matches the contract of + * useSendMessage.sendMessage so the UI knows whether to remove the + * scratchlist entry on success. + */ + onPromoteToQueue: (text: string) => Promise +}) { + const { t } = useTranslation() + const [entries, setEntries] = useState(() => readScratchlist(sessionId)) + const [collapsed, setCollapsed] = useState(() => readCollapsedPref(sessionId)) + const [draft, setDraft] = useState('') + const [busyEntryId, setBusyEntryId] = useState(null) + const inputRef = useRef(null) + + // Re-hydrate when the session id changes (route navigation between sessions). + useEffect(() => { + setEntries(readScratchlist(sessionId)) + setCollapsed(readCollapsedPref(sessionId)) + setDraft('') + setBusyEntryId(null) + }, [sessionId]) + + // Persist on every change. The storage layer swallows quota / serialization + // errors so this won't throw. + useEffect(() => { + persistScratchlist(sessionId, entries) + }, [sessionId, entries]) + + // Global keyboard shortcut: Ctrl/Cmd + Shift + S focuses the add-input + // and expands the panel. Suggested by the handoff doc; matches the + // convention used by other composer-adjacent shortcuts (Ctrl/Cmd-m for + // model cycling) so it shouldn't collide with browser defaults that the + // app cares about. + useEffect(() => { + const onKeyDown = (e: globalThis.KeyboardEvent) => { + if ((e.metaKey || e.ctrlKey) && e.shiftKey && (e.key === 'S' || e.key === 's')) { + e.preventDefault() + setCollapsed(false) + writeCollapsedPref(sessionId, false) + queueMicrotask(() => inputRef.current?.focus()) + } + } + window.addEventListener('keydown', onKeyDown) + return () => window.removeEventListener('keydown', onKeyDown) + }, [sessionId]) + + const toggleCollapsed = useCallback(() => { + setCollapsed((prev) => { + const next = !prev + writeCollapsedPref(sessionId, next) + return next + }) + }, [sessionId]) + + const handleAdd = useCallback((rawText: string) => { + setEntries((prev) => addScratchlistEntry(prev, rawText).entries) + setDraft('') + }, []) + + const handleSubmit = useCallback((event: ReactFormEvent) => { + event.preventDefault() + handleAdd(draft) + }, [draft, handleAdd]) + + const handleKeyDown = useCallback((e: ReactKeyboardEvent) => { + // Plain Enter adds; Shift+Enter inserts a newline. Mirrors the + // composer's default keyboard-send behavior so muscle memory carries + // over and reduces accidental newlines in scratchlist titles. + if (e.key === 'Enter' && !e.shiftKey && !e.nativeEvent.isComposing) { + e.preventDefault() + handleAdd(draft) + } + }, [draft, handleAdd]) + + const handleDelete = useCallback((entry: ScratchlistEntry) => { + if (shouldConfirmDelete(entry)) { + const confirmed = typeof window !== 'undefined' + ? window.confirm(t('scratchlist.confirmDelete')) + : true + if (!confirmed) return + } + setEntries((prev) => deleteScratchlistEntry(prev, entry.id)) + }, [t]) + + const handleMove = useCallback((entry: ScratchlistEntry, direction: 'up' | 'down') => { + setEntries((prev) => moveScratchlistEntry(prev, entry.id, direction)) + }, []) + + const handlePromoteToComposer = useCallback((entry: ScratchlistEntry) => { + onPromoteToComposer(entry.text) + // Promote-to-composer is a copy, not a move: the entry stays in the + // scratchlist so the operator can iterate. Promote-to-queue is the + // destructive variant. + }, [onPromoteToComposer]) + + const handlePromoteToQueue = useCallback(async (entry: ScratchlistEntry) => { + if (busyEntryId) return + setBusyEntryId(entry.id) + try { + const accepted = await onPromoteToQueue(entry.text) + if (accepted) { + setEntries((prev) => deleteScratchlistEntry(prev, entry.id)) + } + } finally { + setBusyEntryId(null) + } + }, [busyEntryId, onPromoteToQueue]) + + const summary = useMemo(() => { + if (entries.length === 0) return t('scratchlist.empty') + if (entries.length === 1) return t('scratchlist.count.one') + return t('scratchlist.count.other', { n: entries.length }) + }, [entries.length, t]) + + const hasReachedCap = entries.length >= SCRATCHLIST_MAX_ENTRIES + + return ( +
+
+ + +
+ {/* + * `inert` removes the inner controls from the focus and + * pointer-events tree (and the accessibility tree) while + * collapsed. CSS-only collapse left the textarea + buttons + * focusable under aria-hidden, which is the regression + * flagged by the upstream PR review (a11y violation: + * focusable descendants inside an aria-hidden subtree). + * Using inert preserves the grid-template-rows expand + * animation while keeping the collapsed body unreachable. + */} +
+
+
+