mirror of
https://github.com/wu736139669/hapi.git
synced 2026-08-05 06:24:37 +00:00
feat(web): per-session scratchlist (workbench) panel (#772)
* feat(web): per-session scratchlist (workbench) panel Adds a per-session "scratchlist" panel above the composer for parking notes / drafts / parking-lot ideas that are explicitly held — never auto-sent. This is distinct from the existing queue (QueuedMessagesBar): - Queue = conveyor belt: messages auto-fire once the agent is idle. - Scratchlist = workbench: held until the operator promotes them. The amber accent and "held — not sent" pill make the visual distinction obvious so operators don't mistake one for the other. Features: - Collapsible per-session panel (collapsed by default, persisted in localStorage). - Add (Enter) / delete / reorder (up/down) entries. - Promote-to-composer copies into the composer for editing (entry stays — copy semantics). - Promote-to-queue routes through the existing onSend path so the entry shows up in QueuedMessagesBar; entry is removed only on accepted send. - Entries persist per session under hapi.scratchlist.v1.<sessionId>. - Confirm-on-delete only for entries longer than 100 chars. - Ctrl/Cmd+Shift+S focuses the add-input. - en + zh-CN strings. v1 scope: localStorage-only. Hub-sync deferred to v2 to keep the diff small and reviewable. Test coverage: - web/src/lib/scratchlist.test.ts — 21 tests (storage round-trip, add/delete/reorder/cap, malformed-JSON resilience, confirm threshold). - web/src/components/AssistantChat/ScratchlistPanel.test.tsx — 13 tests (collapse persistence, hydration, add/delete/reorder UI, promote-to-composer copy semantics, promote-to-queue accepted / rejected paths, per-session isolation). Closes #11 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): block focus into collapsed panel via inert Upstream review (tiann/hapi#772, codex bot) flagged that the collapsed scratchlist body was visually hidden via CSS only - the textarea and action buttons stayed mounted, focusable, and clickable while their ancestor was aria-hidden. Tab into invisible controls + a hidden subtree with focusable descendants is an a11y violation. Apply `inert` to the inner content, gated on the collapsed state. This removes the subtree from the focus, pointer, and accessibility trees while keeping the grid-template-rows expand animation intact (no conditional remount, so the open/close transition still runs). Add a regression test that asserts `inert` is present while collapsed and removed (or empty) while expanded, so a future revert of the fix trips immediately. Co-authored-by: Cursor <cursoragent@cursor.com> * test(scratchlist): add Playwright e2e + isolated fixture page The unit suite under jsdom can't verify the parts of the scratchlist that actually live in the browser: - `inert` blocks focus (jsdom ignores `inert`) - the grid-template-rows collapse animation - localStorage surviving a full page reload - per-session keying surviving cross-route navigation - Ctrl/Cmd+Shift+S firing the global expand+focus shortcut Add a Playwright config + spec that drives a real Chromium against a new Vite-served fixture (`web/e2e-fixtures/scratchlist-fixture.html`). The fixture mounts the production `ScratchlistPanel` in isolation inside an `I18nProvider` and exposes the promote callbacks on `window.__scratchlistE2E` so the spec can assert that promote-to- composer and promote-to-queue receive the right text without having to spin up the hub, auth, or socket layer. Nine specs cover: 1. starts collapsed, toggles 2. collapsed inner is `inert` and refuses focus / pointer 3. add: entry appears, draft clears, count updates 4. persistence across full page reload 5. promote-to-composer fires callback (entry stays - copy semantics) 6. promote-to-queue success path (entry removed) 7. promote-to-queue failure path (entry retained for retry) 8. Ctrl+Shift+S expands + focuses input 9. per-session isolation across navigation Wires `bun run test:e2e` and `test:e2e:ui` at the repo root and documents the harness in `web/README.md`. Bumps `playwright` 1.49.1 -> 1.60.0 alongside the new `@playwright/test` dep so the bundled chromium-headless-shell-1223 (Chrome 148) is used; the older 131 binary SIGTRAPs on this kernel during launch. Adds `test-results/` and `playwright-report/` to `.gitignore`. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): key host by session.id to prevent cross-session leak Upstream review (tiann/hapi#772, codex bot follow-up) flagged a state leak across same-route session switches. ScratchlistPanel reads `sessionId` once via `useState(() => readScratchlist(sessionId))` and rehydrates in a `useEffect`. SessionChat stays mounted when the operator switches sessions on the same `/sessions/$sessionId` route, so the panel sees a new `sessionId` prop without unmounting. Effect order during the prop change: 1. render with sessionId=B but stale entries=[A's items] 2. rehydrate effect: setEntries(read(B)) -> queues correction 3. persist effect (deps [sessionId, entries] both changed): persistScratchlist(B, [A's items]) -> writes A into B 4. re-render with sessionId=B, entries=B's items 5. persist effect: persistScratchlist(B, B's items) -> overwrites the bug write The bug is transient (step 3's write is corrected by step 5) but real: any read between steps 3 and 5 (another tab, a SW prefetch, manual inspection) sees A's data under B's key. Fix is one line: `key={props.session.id}` on `<ScratchlistHost>`. React unmounts and remounts the host when the key changes, so the new mount's useState initializer reads B's storage from scratch and never touches B's key with A's data. This is the React-canonical "reset state on prop change" pattern; cleaner than chasing the race inside the panel. Add an e2e regression test that: - installs a `localStorage.setItem` spy in `addInitScript` - mounts the fixture under session A and adds an entry - clears the spy, then switches to session B in-place via `window.__scratchlistE2E.setSessionId('leak-B')` (no page reload) - asserts no recorded write to `hapi.scratchlist.v1.leak-B` contained A's text (catches the transient corrupting write deterministically, before the correction overwrites it) - round-trips back to A to confirm A's storage is intact The fixture grows a `?key=0` mode that drops the host's `key=` prop. Verified red/green: with `key=0` the regression test fails on the spy-detected corrupting write; with the fix in place (default), all 10 e2e specs pass. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
HeavyGee
@@ -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<boolean>
|
||||
sessionId?: string
|
||||
}) {
|
||||
const onPromoteToComposer = props?.onPromoteToComposer ?? vi.fn()
|
||||
const onPromoteToQueue = props?.onPromoteToQueue ?? vi.fn(async () => true)
|
||||
return {
|
||||
onPromoteToComposer,
|
||||
onPromoteToQueue,
|
||||
...render(
|
||||
<I18nProvider>
|
||||
<ScratchlistPanel
|
||||
sessionId={props?.sessionId ?? SID}
|
||||
onPromoteToComposer={onPromoteToComposer}
|
||||
onPromoteToQueue={onPromoteToQueue}
|
||||
/>
|
||||
</I18nProvider>
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
function makeEntry(overrides: Partial<ScratchlistEntry> & { 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()
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user