From cedba81cfd9b8ef87d2f47f2860d7234623dc1f1 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Sat, 13 Jun 2026 17:39:03 +0100 Subject: [PATCH 01/11] feat(web,hub): scratchlist v2 - hub sync via typed table + session-updated piggyback (#893) Promotes scratchlist persistence from per-device localStorage to a hub- backed typed table so entries follow the operator across devices. v1 panel UI / FUE / shortcut / styling are deliberately unchanged - this is a backend + sync-layer feature. Hub side - New `session_scratchlist` typed table (sessionId, entryId, text, createdAt, updatedAt) with composite PK and FK ON DELETE CASCADE from sessions. Schema bumped V9 -> V10; idempotent migration added to the legacy + step ladders. - REST CRUD under `/api/sessions/:id/scratchlist[/:entryId]`, all routed through the existing `requireSessionFromParam` guard so namespace / ownership enforcement is identical to other session-scoped routes. - Per-session 200-entry cap enforced on POST. Duplicate entryId reported idempotently (200) so the migration retry path is safe. - `SessionPatchSchema` extended with `scratchlistUpdatedAt?: number`; every successful mutation emits a `session-updated` SSE patch with the token. (Following operator's piggyback decision; aligns with the parallel #884 patch-shape extension.) Web side - Hub becomes source of truth via TanStack Query (`queryKeys.scratchlist(sessionId)`); localStorage demoted to offline cache. Add / delete / update mutations are optimistic with rollback on error. - Silent first-load migration: existing localStorage entries are pushed to the hub preserving id + createdAt, and a one-time banner (mirroring `CursorMigrationBanner`) tells the operator their notes are now in the hub. Banner dismissal is per-session and persistent. - SSE handler queues a `scratchlist` invalidation when the patch carries `scratchlistUpdatedAt`, so cross-device + cross-tab updates land within an SSE round-trip. - Delete-session confirm copy now includes a count of scratchlist entries that will be cascade-deleted. Out of scope (separate tracking issue #894): "delete with summarize-and- migrate" UX flow. Tests - Hub: V9->V10 migration (fresh + multi-hop legacy + idempotent reopen + cascade-delete), `ScratchlistStore` CRUD + ordering, REST routes (happy path + 400/403/404/409), SyncEngine SSE emission. - Web: hook covers initial fetch, optimistic add/delete/update with rollback, localStorage migration + banner, cap enforcement, local-only reorder. Banner component renders only on `'completed'`. - Existing Playwright e2e (10 tests, panel UI regression) all pass unchanged. Co-authored-by: Cursor --- hub/src/store/index.ts | 58 ++- hub/src/store/migration-v10.test.ts | 329 +++++++++++++ hub/src/store/scratchlist.ts | 181 +++++++ hub/src/store/scratchlistStore.ts | 52 ++ hub/src/store/types.ts | 8 + hub/src/sync/sessionCache.ts | 29 ++ hub/src/sync/syncEngine-scratchlist.test.ts | 212 ++++++++ hub/src/sync/syncEngine.ts | 86 ++++ .../web/routes/sessions-scratchlist.test.ts | 347 +++++++++++++ hub/src/web/routes/sessions.ts | 128 +++++ shared/src/apiTypes.ts | 44 ++ shared/src/schemas.ts | 29 +- web/src/api/client.ts | 54 ++ .../ScratchlistMigrationBanner.test.tsx | 60 +++ .../ScratchlistMigrationBanner.tsx | 64 +++ web/src/components/SessionChat.tsx | 24 +- web/src/components/SessionHeader.tsx | 17 +- web/src/hooks/useSSE.ts | 28 +- web/src/lib/locales/en.ts | 5 + web/src/lib/locales/zh-CN.ts | 5 + web/src/lib/query-keys.ts | 1 + web/src/lib/use-hub-scratchlist.test.tsx | 360 ++++++++++++++ web/src/lib/use-hub-scratchlist.ts | 465 ++++++++++++++++++ web/src/lib/use-scratchlist-count.ts | 30 ++ 24 files changed, 2605 insertions(+), 11 deletions(-) create mode 100644 hub/src/store/migration-v10.test.ts create mode 100644 hub/src/store/scratchlist.ts create mode 100644 hub/src/store/scratchlistStore.ts create mode 100644 hub/src/sync/syncEngine-scratchlist.test.ts create mode 100644 hub/src/web/routes/sessions-scratchlist.test.ts create mode 100644 web/src/components/AssistantChat/ScratchlistMigrationBanner.test.tsx create mode 100644 web/src/components/AssistantChat/ScratchlistMigrationBanner.tsx create mode 100644 web/src/lib/use-hub-scratchlist.test.tsx create mode 100644 web/src/lib/use-hub-scratchlist.ts create mode 100644 web/src/lib/use-scratchlist-count.ts diff --git a/hub/src/store/index.ts b/hub/src/store/index.ts index b0b2c6b05d..aff94eaf91 100644 --- a/hub/src/store/index.ts +++ b/hub/src/store/index.ts @@ -5,6 +5,7 @@ import { dirname } from 'node:path' import { MachineStore } from './machineStore' import { MessageStore } from './messageStore' import { PushStore } from './pushStore' +import { ScratchlistStore } from './scratchlistStore' import { SessionStore } from './sessionStore' import { UserStore } from './userStore' @@ -12,6 +13,7 @@ export type { StoredMachine, StoredMessage, StoredPushSubscription, + StoredScratchlistEntry, StoredSession, StoredUser, VersionedUpdateResult @@ -20,16 +22,18 @@ export type { CancelQueuedMessageResult, LookupQueuedMessageResult } from './mes export { MachineStore } from './machineStore' export { MessageStore } from './messageStore' export { PushStore } from './pushStore' +export { ScratchlistStore } from './scratchlistStore' export { SessionStore } from './sessionStore' export { UserStore } from './userStore' -const SCHEMA_VERSION: number = 10 +const SCHEMA_VERSION: number = 11 const REQUIRED_TABLES = [ 'sessions', 'machines', 'messages', 'users', - 'push_subscriptions' + 'push_subscriptions', + 'session_scratchlist' ] as const export class Store { @@ -42,6 +46,7 @@ export class Store { readonly messages: MessageStore readonly users: UserStore readonly push: PushStore + readonly scratchlist: ScratchlistStore /** * Filesystem path of the underlying SQLite database, or ':memory:' for @@ -92,6 +97,7 @@ export class Store { this.messages = new MessageStore(this.db) this.users = new UserStore(this.db) this.push = new PushStore(this.db) + this.scratchlist = new ScratchlistStore(this.db) } close(): void { @@ -124,6 +130,7 @@ export class Store { 7: () => this.migrateFromV7ToV8(), 8: () => this.migrateFromV8ToV9(), 9: () => this.migrateFromV9ToV10(), + 10: () => this.migrateFromV10ToV11(), }) if (currentVersion === 0) { @@ -252,6 +259,18 @@ export class Store { UNIQUE(namespace, endpoint) ); CREATE INDEX IF NOT EXISTS idx_push_subscriptions_namespace ON push_subscriptions(namespace); + + CREATE TABLE IF NOT EXISTS session_scratchlist ( + session_id TEXT NOT NULL, + entry_id TEXT NOT NULL, + text TEXT NOT NULL, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + PRIMARY KEY (session_id, entry_id), + FOREIGN KEY (session_id) REFERENCES sessions(id) ON DELETE CASCADE + ); + CREATE INDEX IF NOT EXISTS idx_session_scratchlist_session_created + ON session_scratchlist(session_id, created_at DESC); `) } @@ -435,6 +454,41 @@ export class Store { } } + /** + * tiann/hapi#893 (scratchlist v2): introduce the per-session + * `session_scratchlist` typed table. Operator-decided schema choice + * over an opaque metadata blob - the eventual overseer-context use + * case wants `(sessionId, createdAt)` queryability without parsing + * JSON. + * + * Idempotent via `CREATE TABLE IF NOT EXISTS` + `CREATE INDEX IF NOT + * EXISTS`. Cascade-delete from `sessions(id)` handles delete-session + * cleanup. No data backfill: pre-v11 hubs never had this data; the + * web client's first-run migration (`hapi.scratchlist.v2.migrated.*` + * flag) pushes any existing `localStorage` entries up via the REST + * endpoint. + * + * Rollback: `DROP TABLE session_scratchlist; PRAGMA user_version = 10;` + * - the table is independent, so the drop is safe and loses only the + * v2 hub-side entries (web client retains its localStorage offline + * cache). + */ + private migrateFromV10ToV11(): void { + this.db.exec(` + CREATE TABLE IF NOT EXISTS session_scratchlist ( + session_id TEXT NOT NULL, + entry_id TEXT NOT NULL, + text TEXT NOT NULL, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + PRIMARY KEY (session_id, entry_id), + FOREIGN KEY (session_id) REFERENCES sessions(id) ON DELETE CASCADE + ); + CREATE INDEX IF NOT EXISTS idx_session_scratchlist_session_created + ON session_scratchlist(session_id, created_at DESC); + `) + } + private getSessionColumnNames(): Set { const rows = this.db.prepare('PRAGMA table_info(sessions)').all() as Array<{ name: string }> return new Set(rows.map((row) => row.name)) diff --git a/hub/src/store/migration-v10.test.ts b/hub/src/store/migration-v10.test.ts new file mode 100644 index 0000000000..daed5a5633 --- /dev/null +++ b/hub/src/store/migration-v10.test.ts @@ -0,0 +1,329 @@ +import { describe, expect, it } from 'bun:test' +import { Database } from 'bun:sqlite' +import { mkdtempSync, rmSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { Store } from './index' + +/** + * Tests for V9→V10 schema migration: introduces the `session_scratchlist` + * typed table for tiann/hapi#893 (scratchlist v2 hub sync). Mirrors the + * pattern in `migration-v9.test.ts`. + * + * The new table is independent (no backfill from existing rows) so the + * tests focus on: + * - presence on fresh DBs + * - presence on multi-hop legacy DBs (V6/V7/V8/V9 → V10) + * - idempotency on reopen + * - foreign-key cascade-delete from sessions(id) + * - the supporting (session_id, created_at) index + * - basic CRUD operations through the ScratchlistStore wrapper + */ +describe('Store V9→V10 migration: session_scratchlist table', () => { + it('fresh DB has session_scratchlist table with expected columns', () => { + const store = new Store(':memory:') + const cols = getColumns(store, 'session_scratchlist') + expect(cols).toContain('session_id') + expect(cols).toContain('entry_id') + expect(cols).toContain('text') + expect(cols).toContain('created_at') + expect(cols).toContain('updated_at') + }) + + it('fresh DB has the (session_id, created_at) index', () => { + const store = new Store(':memory:') + const db: Database = (store as unknown as { db: Database }).db + const rows = db.prepare( + "SELECT name FROM sqlite_master WHERE type='index' AND name='idx_session_scratchlist_session_created'" + ).all() as Array<{ name: string }> + expect(rows).toHaveLength(1) + }) + + it('V9 DB migrates to V10 via Store: session_scratchlist created', () => { + const dir = mkdtempSync(join(tmpdir(), 'hapi-migration-v10-test-')) + const dbPath = join(dir, 'test.db') + let store: Store | undefined + try { + const db = new Database(dbPath, { create: true, readwrite: true, strict: true }) + db.exec('PRAGMA journal_mode = WAL') + db.exec('PRAGMA foreign_keys = ON') + createV9Schema(db) + db.exec('PRAGMA user_version = 9') + db.exec(`INSERT INTO sessions (id, namespace, created_at, updated_at, seq) + VALUES ('s1', 'default', 1000, 1000, 0)`) + db.close() + + store = new Store(dbPath) + const cols = getColumns(store, 'session_scratchlist') + expect(cols).toContain('session_id') + expect(cols).toContain('text') + + // Existing session row remains intact through the migration. + const sessions = (store as unknown as { db: Database }).db.prepare( + 'SELECT id FROM sessions' + ).all() as Array<{ id: string }> + expect(sessions.map((r) => r.id)).toEqual(['s1']) + } finally { + store?.close() + rmSync(dir, { recursive: true, force: true }) + } + }) + + it('V8 DB migrates to V10 (multi-hop V8→V9→V10)', () => { + const dir = mkdtempSync(join(tmpdir(), 'hapi-migration-v8-to-v10-')) + const dbPath = join(dir, 'test.db') + let store: Store | undefined + try { + const db = new Database(dbPath, { create: true, readwrite: true, strict: true }) + db.exec('PRAGMA journal_mode = WAL') + db.exec('PRAGMA foreign_keys = ON') + createV9Schema(db) // V9 is a superset of V8's tables for our purposes + db.exec('PRAGMA user_version = 8') + db.close() + + store = new Store(dbPath) + // After ladder runs, both v8→v9 (scheduled_at) AND v9→v10 (table) applied. + const messageCols = getColumns(store, 'messages') + expect(messageCols).toContain('scheduled_at') + const scratchCols = getColumns(store, 'session_scratchlist') + expect(scratchCols).toContain('entry_id') + } finally { + store?.close() + rmSync(dir, { recursive: true, force: true }) + } + }) + + it('V10 DB reopen is idempotent: schema unchanged', () => { + const dir = mkdtempSync(join(tmpdir(), 'hapi-migration-v10-idempotent-')) + const dbPath = join(dir, 'test.db') + let store1: Store | undefined + let store2: Store | undefined + try { + store1 = new Store(dbPath) + const cols1 = getColumns(store1, 'session_scratchlist') + + store2 = new Store(dbPath) + const cols2 = getColumns(store2, 'session_scratchlist') + expect(cols2).toEqual(cols1) + } finally { + store2?.close() + store1?.close() + rmSync(dir, { recursive: true, force: true }) + } + }) + + it('cascade-delete: scratchlist entries are removed when their session is deleted', async () => { + const store = new Store(':memory:') + const session = store.sessions.getOrCreateSession('test', { path: '/tmp' }, null, 'default') + const create1 = store.scratchlist.create(session.id, 'note one') + const create2 = store.scratchlist.create(session.id, 'note two') + expect(create1.outcome).toBe('created') + expect(create2.outcome).toBe('created') + expect(store.scratchlist.count(session.id)).toBe(2) + + await store.sessions.deleteSession(session.id, 'default') + expect(store.scratchlist.count(session.id)).toBe(0) + }) +}) + +describe('ScratchlistStore: CRUD through the typed-table wrapper', () => { + function setup() { + const store = new Store(':memory:') + const session = store.sessions.getOrCreateSession('test', { path: '/tmp' }, null, 'default') + return { store, sessionId: session.id } + } + + it('create returns the canonical row and assigns an entryId when omitted', () => { + const { store, sessionId } = setup() + const result = store.scratchlist.create(sessionId, 'hello') + if (result.outcome !== 'created') { + throw new Error(`Expected created, got ${result.outcome}`) + } + expect(result.entry.text).toBe('hello') + expect(result.entry.entryId).toMatch(/[0-9a-f-]{8,}/) + expect(result.entry.createdAt).toBeGreaterThan(0) + expect(result.entry.updatedAt).toBe(result.entry.createdAt) + }) + + it('create preserves caller-supplied entryId and createdAt for migration path', () => { + const { store, sessionId } = setup() + const result = store.scratchlist.create(sessionId, 'migrated', { + entryId: 'legacy-id-1', + createdAt: 12345, + }) + if (result.outcome !== 'created') throw new Error(`Expected created, got ${result.outcome}`) + expect(result.entry.entryId).toBe('legacy-id-1') + expect(result.entry.createdAt).toBe(12345) + // updatedAt is always Date.now() on insert; differs from createdAt when caller supplies an old timestamp. + expect(result.entry.updatedAt).toBeGreaterThan(12345) + }) + + it('create with an existing entryId is reported as duplicate and returns the existing row', () => { + const { store, sessionId } = setup() + const first = store.scratchlist.create(sessionId, 'first', { entryId: 'dup-id' }) + if (first.outcome !== 'created') throw new Error(`Expected created`) + const second = store.scratchlist.create(sessionId, 'second', { entryId: 'dup-id' }) + if (second.outcome !== 'duplicate') { + throw new Error(`Expected duplicate, got ${second.outcome}`) + } + expect(second.entry.text).toBe('first') + }) + + it('create against a non-existent session reports session-not-found (not a SQLite error)', () => { + const store = new Store(':memory:') + const result = store.scratchlist.create('does-not-exist', 'orphan') + expect(result.outcome).toBe('session-not-found') + }) + + it('list returns entries in createdAt DESC order (newest first)', () => { + const { store, sessionId } = setup() + const a = store.scratchlist.create(sessionId, 'oldest', { entryId: 'a', createdAt: 1000 }) + const b = store.scratchlist.create(sessionId, 'middle', { entryId: 'b', createdAt: 2000 }) + const c = store.scratchlist.create(sessionId, 'newest', { entryId: 'c', createdAt: 3000 }) + expect(a.outcome).toBe('created') + expect(b.outcome).toBe('created') + expect(c.outcome).toBe('created') + const entries = store.scratchlist.list(sessionId) + expect(entries.map((e) => e.entryId)).toEqual(['c', 'b', 'a']) + }) + + it('update bumps updated_at without touching createdAt; returns null for missing entries', () => { + const { store, sessionId } = setup() + const created = store.scratchlist.create(sessionId, 'before', { + entryId: 'u1', + createdAt: 1000, + }) + if (created.outcome !== 'created') throw new Error('Expected created') + + const updated = store.scratchlist.update(sessionId, 'u1', 'after') + expect(updated).not.toBeNull() + expect(updated!.text).toBe('after') + expect(updated!.createdAt).toBe(1000) + expect(updated!.updatedAt).toBeGreaterThan(1000) + + const missing = store.scratchlist.update(sessionId, 'does-not-exist', 'noop') + expect(missing).toBeNull() + }) + + it('delete returns true when the row existed, false otherwise', () => { + const { store, sessionId } = setup() + store.scratchlist.create(sessionId, 'doomed', { entryId: 'd1' }) + expect(store.scratchlist.delete(sessionId, 'd1')).toBe(true) + expect(store.scratchlist.delete(sessionId, 'd1')).toBe(false) + }) + + it('count tracks current rows', () => { + const { store, sessionId } = setup() + expect(store.scratchlist.count(sessionId)).toBe(0) + store.scratchlist.create(sessionId, 'a', { entryId: 'a' }) + store.scratchlist.create(sessionId, 'b', { entryId: 'b' }) + expect(store.scratchlist.count(sessionId)).toBe(2) + store.scratchlist.delete(sessionId, 'a') + expect(store.scratchlist.count(sessionId)).toBe(1) + }) + + it('entries from session A are not visible to session B', () => { + const store = new Store(':memory:') + const a = store.sessions.getOrCreateSession('a', { path: '/a' }, null, 'default') + const b = store.sessions.getOrCreateSession('b', { path: '/b' }, null, 'default') + store.scratchlist.create(a.id, 'A note', { entryId: 'shared-id' }) + expect(store.scratchlist.list(b.id)).toEqual([]) + expect(store.scratchlist.get(a.id, 'shared-id')).not.toBeNull() + expect(store.scratchlist.get(b.id, 'shared-id')).toBeNull() + }) +}) + +function getColumns(store: Store, table: string): string[] { + const db: Database = (store as unknown as { db: Database }).db + const rows = db.prepare(`PRAGMA table_info(${table})`).all() as Array<{ name: string }> + return rows.map((r) => r.name) +} + +/** + * V9 schema: full pre-V10 shape. Used to seed legacy DBs to verify the + * V9→V10 step adds the new table without disturbing existing rows. + */ +function createV9Schema(db: Database): void { + db.exec(` + CREATE TABLE IF NOT EXISTS sessions ( + id TEXT PRIMARY KEY, + tag TEXT, + namespace TEXT NOT NULL DEFAULT 'default', + machine_id TEXT, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + metadata TEXT, + metadata_version INTEGER DEFAULT 1, + agent_state TEXT, + agent_state_version INTEGER DEFAULT 1, + model TEXT, + model_reasoning_effort TEXT, + effort TEXT, + todos TEXT, + todos_updated_at INTEGER, + team_state TEXT, + team_state_updated_at INTEGER, + active INTEGER DEFAULT 0, + active_at INTEGER, + seq INTEGER DEFAULT 0 + ); + CREATE INDEX IF NOT EXISTS idx_sessions_tag ON sessions(tag); + CREATE INDEX IF NOT EXISTS idx_sessions_tag_namespace ON sessions(tag, namespace); + + CREATE TABLE IF NOT EXISTS machines ( + id TEXT PRIMARY KEY, + namespace TEXT NOT NULL DEFAULT 'default', + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + metadata TEXT, + metadata_version INTEGER DEFAULT 1, + runner_state TEXT, + runner_state_version INTEGER DEFAULT 1, + active INTEGER DEFAULT 0, + active_at INTEGER, + seq INTEGER DEFAULT 0 + ); + CREATE INDEX IF NOT EXISTS idx_machines_namespace ON machines(namespace); + + CREATE TABLE IF NOT EXISTS messages ( + id TEXT PRIMARY KEY, + session_id TEXT NOT NULL, + content TEXT NOT NULL, + created_at INTEGER NOT NULL, + seq INTEGER NOT NULL, + local_id TEXT, + invoked_at INTEGER, + scheduled_at INTEGER, + FOREIGN KEY (session_id) REFERENCES sessions(id) ON DELETE CASCADE + ); + CREATE INDEX IF NOT EXISTS idx_messages_session ON messages(session_id, seq); + CREATE UNIQUE INDEX IF NOT EXISTS idx_messages_local_id ON messages(session_id, local_id) WHERE local_id IS NOT NULL; + CREATE INDEX IF NOT EXISTS idx_messages_session_position + ON messages(session_id, COALESCE(invoked_at, created_at) DESC, seq DESC); + CREATE INDEX IF NOT EXISTS idx_messages_scheduled_pending + ON messages(scheduled_at) + WHERE scheduled_at IS NOT NULL AND invoked_at IS NULL; + + CREATE TABLE IF NOT EXISTS users ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + platform TEXT NOT NULL, + platform_user_id TEXT NOT NULL, + namespace TEXT NOT NULL DEFAULT 'default', + created_at INTEGER NOT NULL, + UNIQUE(platform, platform_user_id) + ); + CREATE INDEX IF NOT EXISTS idx_users_platform ON users(platform); + CREATE INDEX IF NOT EXISTS idx_users_platform_namespace ON users(platform, namespace); + + CREATE TABLE IF NOT EXISTS push_subscriptions ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + namespace TEXT NOT NULL, + endpoint TEXT NOT NULL, + p256dh TEXT NOT NULL, + auth TEXT NOT NULL, + created_at INTEGER NOT NULL, + UNIQUE(namespace, endpoint) + ); + CREATE INDEX IF NOT EXISTS idx_push_subscriptions_namespace ON push_subscriptions(namespace); + `) +} diff --git a/hub/src/store/scratchlist.ts b/hub/src/store/scratchlist.ts new file mode 100644 index 0000000000..e8305ce579 --- /dev/null +++ b/hub/src/store/scratchlist.ts @@ -0,0 +1,181 @@ +import type { Database } from 'bun:sqlite' +import { randomUUID } from 'node:crypto' + +import type { StoredScratchlistEntry } from './types' + +/** + * Per-session scratchlist storage (tiann/hapi#893, scratchlist v2). + * + * The hub is the source of truth for scratchlist entries; web treats + * `localStorage` as an offline cache only. All queries are scoped by + * `session_id` + (where it matters) the session's namespace - the latter + * is enforced one layer up in `SyncEngine` / web routes via + * `requireSessionFromParam`, so the SQL layer here treats `session_id` + * as the primary scope. + * + * Mental model carried from v1 (#798): scratchlist != queue. Entries are + * notes / drafts / parking-lot ideas, never auto-sent. The hub-side + * representation is deliberately lean: + * + * - `text` plain string (no markdown rendering planned for v2) + * - `created_at` immutable since insert + * - `updated_at` bumped on edits to drive the SSE patch token + * - cascade-delete from `sessions(id)` covers the delete-session path + * + * Per-session caps live in `@hapi/protocol/apiTypes` + * (`SCRATCHLIST_MAX_ENTRIES`, `SCRATCHLIST_MAX_TEXT_LENGTH`); the route + * layer enforces them at write time. The SQL layer accepts whatever it's + * given - the cap is policy, not schema. + */ + +type DbScratchlistRow = { + session_id: string + entry_id: string + text: string + created_at: number + updated_at: number +} + +function toStoredEntry(row: DbScratchlistRow): StoredScratchlistEntry { + return { + sessionId: row.session_id, + entryId: row.entry_id, + text: row.text, + createdAt: row.created_at, + updatedAt: row.updated_at + } +} + +export function listScratchlistEntries( + db: Database, + sessionId: string +): StoredScratchlistEntry[] { + const rows = db.prepare( + `SELECT session_id, entry_id, text, created_at, updated_at + FROM session_scratchlist + WHERE session_id = ? + ORDER BY created_at DESC, entry_id DESC` + ).all(sessionId) as DbScratchlistRow[] + return rows.map(toStoredEntry) +} + +export function countScratchlistEntries(db: Database, sessionId: string): number { + const row = db.prepare( + 'SELECT COUNT(*) AS n FROM session_scratchlist WHERE session_id = ?' + ).get(sessionId) as { n: number } | undefined + return row?.n ?? 0 +} + +export function getScratchlistEntry( + db: Database, + sessionId: string, + entryId: string +): StoredScratchlistEntry | null { + const row = db.prepare( + `SELECT session_id, entry_id, text, created_at, updated_at + FROM session_scratchlist + WHERE session_id = ? AND entry_id = ?` + ).get(sessionId, entryId) as DbScratchlistRow | undefined + return row ? toStoredEntry(row) : null +} + +/** + * Insert a new scratchlist entry. Returns the stored row on success, or + * `{ outcome: 'duplicate' }` when the supplied `entryId` already exists + * (the migration path can collide on retry; clients should treat that as + * idempotent). `{ outcome: 'session-not-found' }` is returned when the FK + * to `sessions` would fail - keeps the route handler from having to + * pre-check session existence. + */ +export type CreateScratchlistResult = + | { outcome: 'created'; entry: StoredScratchlistEntry } + | { outcome: 'duplicate'; entry: StoredScratchlistEntry } + | { outcome: 'session-not-found' } + +export function createScratchlistEntry( + db: Database, + sessionId: string, + text: string, + options?: { entryId?: string; createdAt?: number } +): CreateScratchlistResult { + const now = Date.now() + const entryId = options?.entryId ?? randomUUID() + const createdAt = options?.createdAt ?? now + const updatedAt = now + + // Pre-check FK so the route layer can return a clean 404. Doing this + // before the INSERT keeps the error-handling path narrower (no + // SQLite-error string parsing). + const sessionExists = db.prepare( + 'SELECT 1 FROM sessions WHERE id = ? LIMIT 1' + ).get(sessionId) as { 1: number } | undefined + if (!sessionExists) { + return { outcome: 'session-not-found' } + } + + const existing = getScratchlistEntry(db, sessionId, entryId) + if (existing) { + return { outcome: 'duplicate', entry: existing } + } + + db.prepare( + `INSERT INTO session_scratchlist + (session_id, entry_id, text, created_at, updated_at) + VALUES (@session_id, @entry_id, @text, @created_at, @updated_at)` + ).run({ + session_id: sessionId, + entry_id: entryId, + text, + created_at: createdAt, + updated_at: updatedAt + }) + + const created = getScratchlistEntry(db, sessionId, entryId) + if (!created) { + // Should be unreachable: we just inserted under the same scope. + throw new Error('Failed to read scratchlist entry after insert') + } + return { outcome: 'created', entry: created } +} + +/** + * Update an existing entry's `text`. Bumps `updated_at` to `Date.now()`. + * Returns `null` when the entry does not exist (route layer turns into a + * 404). Note: `created_at` is intentionally NOT updated. + */ +export function updateScratchlistEntry( + db: Database, + sessionId: string, + entryId: string, + text: string +): StoredScratchlistEntry | null { + const now = Date.now() + const result = db.prepare( + `UPDATE session_scratchlist + SET text = @text, + updated_at = @updated_at + WHERE session_id = @session_id + AND entry_id = @entry_id` + ).run({ + session_id: sessionId, + entry_id: entryId, + text, + updated_at: now + }) + if (result.changes === 0) { + return null + } + return getScratchlistEntry(db, sessionId, entryId) +} + +export function deleteScratchlistEntry( + db: Database, + sessionId: string, + entryId: string +): boolean { + const result = db.prepare( + `DELETE FROM session_scratchlist + WHERE session_id = ? AND entry_id = ?` + ).run(sessionId, entryId) + return result.changes > 0 +} diff --git a/hub/src/store/scratchlistStore.ts b/hub/src/store/scratchlistStore.ts new file mode 100644 index 0000000000..fd285ca6d0 --- /dev/null +++ b/hub/src/store/scratchlistStore.ts @@ -0,0 +1,52 @@ +import type { Database } from 'bun:sqlite' + +import type { StoredScratchlistEntry } from './types' +import { + countScratchlistEntries, + createScratchlistEntry, + deleteScratchlistEntry, + getScratchlistEntry, + listScratchlistEntries, + updateScratchlistEntry, + type CreateScratchlistResult +} from './scratchlist' + +export class ScratchlistStore { + private readonly db: Database + + constructor(db: Database) { + this.db = db + } + + list(sessionId: string): StoredScratchlistEntry[] { + return listScratchlistEntries(this.db, sessionId) + } + + count(sessionId: string): number { + return countScratchlistEntries(this.db, sessionId) + } + + get(sessionId: string, entryId: string): StoredScratchlistEntry | null { + return getScratchlistEntry(this.db, sessionId, entryId) + } + + create( + sessionId: string, + text: string, + options?: { entryId?: string; createdAt?: number } + ): CreateScratchlistResult { + return createScratchlistEntry(this.db, sessionId, text, options) + } + + update( + sessionId: string, + entryId: string, + text: string + ): StoredScratchlistEntry | null { + return updateScratchlistEntry(this.db, sessionId, entryId, text) + } + + delete(sessionId: string, entryId: string): boolean { + return deleteScratchlistEntry(this.db, sessionId, entryId) + } +} diff --git a/hub/src/store/types.ts b/hub/src/store/types.ts index ae3f648291..457462cfe1 100644 --- a/hub/src/store/types.ts +++ b/hub/src/store/types.ts @@ -64,6 +64,14 @@ export type StoredPushSubscription = { createdAt: number } +export type StoredScratchlistEntry = { + sessionId: string + entryId: string + text: string + createdAt: number + updatedAt: number +} + export type VersionedUpdateResult = | { result: 'success'; version: number; value: T } | { result: 'version-mismatch'; version: number; value: T } diff --git a/hub/src/sync/sessionCache.ts b/hub/src/sync/sessionCache.ts index 86dd02a564..eb56134575 100644 --- a/hub/src/sync/sessionCache.ts +++ b/hub/src/sync/sessionCache.ts @@ -369,6 +369,35 @@ export class SessionCache { }) } + /** + * tiann/hapi#893 (scratchlist v2): emit a `session-updated` SSE patch + * carrying `scratchlistUpdatedAt` so other clients viewing the same + * session refetch the entries query. Called by `SyncEngine` after + * any successful scratchlist mutation. The timestamp is the trigger, + * not the payload - clients use it as a change-detection token and + * pull entries via the dedicated REST query. + * + * Per operator decision (see brief): piggyback on `session-updated` + * rather than introduce a new event type, because scratchlist + * mutations are exceedingly rare relative to keep-alive patches. + * + * Resolves the namespace from the in-memory session map (or the DB + * row as a fallback) so the SSE manager can scope the broadcast + * correctly even if the cache is cold. + */ + emitScratchlistChanged(sessionId: string, updatedAt: number = Date.now()): void { + const cached = this.sessions.get(sessionId) + const namespace = cached?.namespace + ?? this.store.sessions.getSession(sessionId)?.namespace + if (!namespace) return + this.publisher.emit({ + type: 'session-updated', + sessionId, + namespace, + data: { scratchlistUpdatedAt: updatedAt } satisfies SessionPatch + }) + } + handleSessionEnd(payload: { sid: string; time: number }): void { const t = clampAliveTime(payload.time) ?? Date.now() diff --git a/hub/src/sync/syncEngine-scratchlist.test.ts b/hub/src/sync/syncEngine-scratchlist.test.ts new file mode 100644 index 0000000000..6559cc34e2 --- /dev/null +++ b/hub/src/sync/syncEngine-scratchlist.test.ts @@ -0,0 +1,212 @@ +import { describe, expect, it } from 'bun:test' +import type { SyncEvent } from '@hapi/protocol/types' +import { Store } from '../store' +import { RpcRegistry } from '../socket/rpcRegistry' +import type { EventPublisher } from './eventPublisher' +import { SessionCache } from './sessionCache' +import { SyncEngine } from './syncEngine' + +/** + * Tests for scratchlist v2 (tiann/hapi#893) wiring at the SyncEngine / + * SessionCache layer: + * - every successful mutation emits a `session-updated` SyncEvent + * carrying `scratchlistUpdatedAt` + * - failed mutations (entry not found, duplicate) emit nothing + * - the patch is namespace-scoped to the session's own namespace so + * the SSE manager doesn't broadcast across operators + * + * The web client uses the patch as a refetch trigger; the timestamp + * itself is the only signal, the entries arrive via the dedicated + * `/api/sessions/:id/scratchlist` GET endpoint. + */ + +function createCapturingPublisher(events: SyncEvent[]): EventPublisher { + return { + emit: (event: SyncEvent) => { + events.push(event) + } + } as unknown as EventPublisher +} + +describe('SessionCache.emitScratchlistChanged', () => { + it('emits a session-updated patch carrying scratchlistUpdatedAt', () => { + const store = new Store(':memory:') + const events: SyncEvent[] = [] + const cache = new SessionCache(store, createCapturingPublisher(events)) + const session = cache.getOrCreateSession( + 'tag', + { path: '/tmp', host: 'localhost', flavor: 'codex' }, + null, + 'default' + ) + // Drain spawn events so we can assert on the scratchlist + // emission alone. + events.length = 0 + + cache.emitScratchlistChanged(session.id, 9999) + + expect(events).toHaveLength(1) + const event = events[0]! + expect(event.type).toBe('session-updated') + if (event.type !== 'session-updated') throw new Error('unreachable') + expect(event.sessionId).toBe(session.id) + expect(event.namespace).toBe('default') + expect(event.data).toEqual({ scratchlistUpdatedAt: 9999 }) + }) + + it('does not emit when the session is unknown (no namespace to scope to)', () => { + const store = new Store(':memory:') + const events: SyncEvent[] = [] + const cache = new SessionCache(store, createCapturingPublisher(events)) + cache.emitScratchlistChanged('does-not-exist', 9999) + expect(events).toHaveLength(0) + }) +}) + +describe('SyncEngine scratchlist mutations emit session-updated patches', () => { + function setup() { + const store = new Store(':memory:') + const events: SyncEvent[] = [] + const cache = new SessionCache(store, createCapturingPublisher(events)) + // We attach the EventPublisher to SyncEngine via a private field + // path so the route-layer surface (createScratchlistEntry, etc.) + // exercises the same code path used in production. We only need + // the cache for `getOrCreateSession`; the engine reuses the + // store internally. + const engine = new SyncEngine( + store, + {} as never, + new RpcRegistry(), + { broadcast() {} } as never + ) + // SyncEngine constructs its own SessionCache internally - shimming + // the inner one would be brittle. Use the engine's events stream + // directly via subscription. + const engineEvents: SyncEvent[] = [] + engine.subscribe((e) => { engineEvents.push(e) }) + return { engine, store, events, cache, engineEvents } + } + + it('createScratchlistEntry emits a session-updated patch on success', () => { + const { engine, engineEvents } = setup() + const session = engine.getOrCreateSession( + 'tag-create', + { path: '/tmp', host: 'localhost', flavor: 'codex' }, + null, + 'default' + ) + // Drain events from the spawn so we can assert on the mutation + // emission alone. + engineEvents.length = 0 + + const result = engine.createScratchlistEntry(session.id, 'note', { entryId: 'e1' }) + expect(result.outcome).toBe('created') + + const matching = engineEvents.filter( + (e) => e.type === 'session-updated' && (e.data as Record).scratchlistUpdatedAt !== undefined + ) + expect(matching).toHaveLength(1) + const patch = matching[0] + if (!patch || patch.type !== 'session-updated') throw new Error('unreachable') + expect(patch.sessionId).toBe(session.id) + expect(patch.namespace).toBe('default') + + engine.stop() + }) + + it('updateScratchlistEntry emits a session-updated patch on success', () => { + const { engine, engineEvents } = setup() + const session = engine.getOrCreateSession( + 'tag-update', + { path: '/tmp', host: 'localhost', flavor: 'codex' }, + null, + 'default' + ) + engine.createScratchlistEntry(session.id, 'before', { entryId: 'e1' }) + engineEvents.length = 0 + + const updated = engine.updateScratchlistEntry(session.id, 'e1', 'after') + expect(updated).not.toBeNull() + const matching = engineEvents.filter( + (e) => e.type === 'session-updated' && (e.data as Record).scratchlistUpdatedAt !== undefined + ) + expect(matching).toHaveLength(1) + + engine.stop() + }) + + it('updateScratchlistEntry on a missing entry emits nothing', () => { + const { engine, engineEvents } = setup() + const session = engine.getOrCreateSession( + 'tag-update-missing', + { path: '/tmp', host: 'localhost', flavor: 'codex' }, + null, + 'default' + ) + engineEvents.length = 0 + const updated = engine.updateScratchlistEntry(session.id, 'never-existed', 'whatever') + expect(updated).toBeNull() + const matching = engineEvents.filter( + (e) => e.type === 'session-updated' && (e.data as Record).scratchlistUpdatedAt !== undefined + ) + expect(matching).toHaveLength(0) + engine.stop() + }) + + it('deleteScratchlistEntry emits a session-updated patch on success', () => { + const { engine, engineEvents } = setup() + const session = engine.getOrCreateSession( + 'tag-delete', + { path: '/tmp', host: 'localhost', flavor: 'codex' }, + null, + 'default' + ) + engine.createScratchlistEntry(session.id, 'doomed', { entryId: 'e1' }) + engineEvents.length = 0 + const removed = engine.deleteScratchlistEntry(session.id, 'e1') + expect(removed).toBe(true) + const matching = engineEvents.filter( + (e) => e.type === 'session-updated' && (e.data as Record).scratchlistUpdatedAt !== undefined + ) + expect(matching).toHaveLength(1) + engine.stop() + }) + + it('deleteScratchlistEntry on a missing entry emits nothing', () => { + const { engine, engineEvents } = setup() + const session = engine.getOrCreateSession( + 'tag-delete-missing', + { path: '/tmp', host: 'localhost', flavor: 'codex' }, + null, + 'default' + ) + engineEvents.length = 0 + const removed = engine.deleteScratchlistEntry(session.id, 'no-such-entry') + expect(removed).toBe(false) + const matching = engineEvents.filter( + (e) => e.type === 'session-updated' && (e.data as Record).scratchlistUpdatedAt !== undefined + ) + expect(matching).toHaveLength(0) + engine.stop() + }) + + it('createScratchlistEntry on duplicate does not emit an extra patch', () => { + const { engine, engineEvents } = setup() + const session = engine.getOrCreateSession( + 'tag-dup', + { path: '/tmp', host: 'localhost', flavor: 'codex' }, + null, + 'default' + ) + engine.createScratchlistEntry(session.id, 'first', { entryId: 'dup' }) + engineEvents.length = 0 + const result = engine.createScratchlistEntry(session.id, 'second', { entryId: 'dup' }) + if (result.outcome === 'session-not-found') throw new Error('unexpected') + expect(result.outcome).toBe('duplicate') + const matching = engineEvents.filter( + (e) => e.type === 'session-updated' && (e.data as Record).scratchlistUpdatedAt !== undefined + ) + expect(matching).toHaveLength(0) + engine.stop() + }) +}) diff --git a/hub/src/sync/syncEngine.ts b/hub/src/sync/syncEngine.ts index 7fe09087c1..7fd1a8d4e9 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -353,6 +353,92 @@ export class SyncEngine { this.sessionCache.recordSessionActivity(sessionId, updatedAt) } + /** + * tiann/hapi#893 (scratchlist v2). Read-side: list entries for a + * session. Auth / namespace check is the route layer's job (via + * `requireSessionFromParam`); by the time we get here the caller + * already proved access. + */ + listScratchlistEntries(sessionId: string): Array<{ + entryId: string + text: string + createdAt: number + updatedAt: number + }> { + return this.store.scratchlist.list(sessionId).map((row) => ({ + entryId: row.entryId, + text: row.text, + createdAt: row.createdAt, + updatedAt: row.updatedAt + })) + } + + countScratchlistEntries(sessionId: string): number { + return this.store.scratchlist.count(sessionId) + } + + /** + * Insert a scratchlist entry. Returns the canonical row on success + * (so the route layer can serialise it without a follow-up read). + * Emits a `session-updated` SSE patch carrying `scratchlistUpdatedAt` + * so other clients viewing the same session refetch. + * + * `outcome: 'duplicate'` covers the migration path's idempotency: + * the web client may retry pushing a localStorage entry after a + * partial failure; the second attempt should be a no-op rather than + * a hard error. Route layer maps duplicate → 200/conflict per its + * own contract; this layer just reports it. + */ + createScratchlistEntry( + sessionId: string, + text: string, + options?: { entryId?: string; createdAt?: number } + ): { + outcome: 'created' | 'duplicate' + entry: { entryId: string; text: string; createdAt: number; updatedAt: number } + } | { outcome: 'session-not-found' } { + const result = this.store.scratchlist.create(sessionId, text, options) + if (result.outcome === 'session-not-found') { + return result + } + if (result.outcome === 'created') { + this.sessionCache.emitScratchlistChanged(sessionId, result.entry.updatedAt) + } + return { + outcome: result.outcome, + entry: { + entryId: result.entry.entryId, + text: result.entry.text, + createdAt: result.entry.createdAt, + updatedAt: result.entry.updatedAt + } + } + } + + updateScratchlistEntry( + sessionId: string, + entryId: string, + text: string + ): { entryId: string; text: string; createdAt: number; updatedAt: number } | null { + const updated = this.store.scratchlist.update(sessionId, entryId, text) + if (!updated) return null + this.sessionCache.emitScratchlistChanged(sessionId, updated.updatedAt) + return { + entryId: updated.entryId, + text: updated.text, + createdAt: updated.createdAt, + updatedAt: updated.updatedAt + } + } + + deleteScratchlistEntry(sessionId: string, entryId: string): boolean { + const removed = this.store.scratchlist.delete(sessionId, entryId) + if (removed) { + this.sessionCache.emitScratchlistChanged(sessionId, Date.now()) + } + return removed + } + handleMachineAlive(payload: { machineId: string; time: number }): void { this.machineCache.handleMachineAlive(payload) } diff --git a/hub/src/web/routes/sessions-scratchlist.test.ts b/hub/src/web/routes/sessions-scratchlist.test.ts new file mode 100644 index 0000000000..33e2b7717a --- /dev/null +++ b/hub/src/web/routes/sessions-scratchlist.test.ts @@ -0,0 +1,347 @@ +import { describe, expect, it } from 'bun:test' +import { Hono } from 'hono' +import type { Session, SyncEngine } from '../../sync/syncEngine' +import type { WebAppEnv } from '../middleware/auth' +import { createSessionsRoutes } from './sessions' + +/** + * Tests for the scratchlist v2 (tiann/hapi#893) REST routes: + * GET /api/sessions/:id/scratchlist + * POST /api/sessions/:id/scratchlist + * PUT /api/sessions/:id/scratchlist/:entryId + * DELETE /api/sessions/:id/scratchlist/:entryId + * + * The routes call into a small surface on `SyncEngine` (list/create/ + * update/delete + count). We mock that surface here so the assertions + * focus on: + * - happy-path response shapes + * - auth + namespace gating via `requireSessionFromParam` + * - validation (text required, max length) + * - cap enforcement at SCRATCHLIST_MAX_ENTRIES + * - 404 paths (missing session, missing entry) + * - 200 vs 201 split (created vs duplicate during migration retries) + * + * SSE emission is exercised at the SyncEngine + SessionCache layer in a + * separate test (`syncEngine-scratchlist.test.ts`). + */ + +function createSession(overrides?: Partial): Session { + const baseMetadata = { + path: '/tmp/project', + host: 'localhost', + flavor: 'codex' as const + } + const base: Session = { + id: 'session-1', + namespace: 'default', + seq: 1, + createdAt: 1, + updatedAt: 1, + active: true, + activeAt: 1, + metadata: baseMetadata, + metadataVersion: 1, + agentState: { + controlledByUser: false, + requests: {}, + completedRequests: {} + }, + agentStateVersion: 1, + thinking: false, + thinkingAt: 1, + model: 'gpt-5.4', + modelReasoningEffort: null, + effort: null, + permissionMode: 'default', + collaborationMode: 'default' + } + return { ...base, ...overrides } +} + +type EngineOverrides = Partial<{ + listScratchlistEntries: SyncEngine['listScratchlistEntries'] + countScratchlistEntries: SyncEngine['countScratchlistEntries'] + createScratchlistEntry: SyncEngine['createScratchlistEntry'] + updateScratchlistEntry: SyncEngine['updateScratchlistEntry'] + deleteScratchlistEntry: SyncEngine['deleteScratchlistEntry'] + sessionAccess: 'ok' | 'not-found' | 'wrong-namespace' + callerNamespace: string +}> + +function createApp(session: Session, overrides: EngineOverrides = {}) { + const engine = { + resolveSessionAccess: () => { + if (overrides.sessionAccess === 'not-found') { + return { ok: false, reason: 'not-found' as const } + } + if (overrides.sessionAccess === 'wrong-namespace') { + return { ok: false, reason: 'access-denied' as const } + } + return { ok: true, sessionId: session.id, session } + }, + listScratchlistEntries: overrides.listScratchlistEntries ?? (() => []), + countScratchlistEntries: overrides.countScratchlistEntries ?? (() => 0), + createScratchlistEntry: overrides.createScratchlistEntry + ?? ((sessionId: string, text: string) => ({ + outcome: 'created' as const, + entry: { + entryId: `auto-${Date.now()}`, + text, + createdAt: 1000, + updatedAt: 1000 + } + })), + updateScratchlistEntry: overrides.updateScratchlistEntry + ?? ((sessionId: string, entryId: string, text: string) => ({ + entryId, + text, + createdAt: 1000, + updatedAt: 2000 + })), + deleteScratchlistEntry: overrides.deleteScratchlistEntry ?? (() => true) + } as unknown as SyncEngine + + const app = new Hono() + app.use('*', async (c, next) => { + c.set('namespace', overrides.callerNamespace ?? 'default') + await next() + }) + app.route('/api', createSessionsRoutes(() => engine)) + return app +} + +describe('GET /api/sessions/:id/scratchlist', () => { + it('returns the entries returned by the engine', async () => { + const session = createSession() + const app = createApp(session, { + listScratchlistEntries: () => [ + { entryId: 'a', text: 'note A', createdAt: 1000, updatedAt: 1000 }, + { entryId: 'b', text: 'note B', createdAt: 2000, updatedAt: 2500 } + ] + }) + const res = await app.request('/api/sessions/session-1/scratchlist') + expect(res.status).toBe(200) + const body = await res.json() as { entries: Array<{ entryId: string }> } + expect(body.entries.map((e) => e.entryId)).toEqual(['a', 'b']) + }) + + it('returns 404 when the session is not visible to the caller', async () => { + const session = createSession() + const app = createApp(session, { sessionAccess: 'not-found' }) + const res = await app.request('/api/sessions/session-1/scratchlist') + expect(res.status).toBe(404) + }) + + it('returns 403 when the session belongs to a different namespace', async () => { + const session = createSession({ namespace: 'other' }) + const app = createApp(session, { sessionAccess: 'wrong-namespace' }) + const res = await app.request('/api/sessions/session-1/scratchlist') + expect(res.status).toBe(403) + }) +}) + +describe('POST /api/sessions/:id/scratchlist', () => { + it('creates an entry and returns 201 with the canonical row', async () => { + const session = createSession() + const calls: Array<{ sessionId: string; text: string; entryId?: string; createdAt?: number }> = [] + const app = createApp(session, { + createScratchlistEntry: (sessionId, text, options) => { + calls.push({ sessionId, text, entryId: options?.entryId, createdAt: options?.createdAt }) + return { + outcome: 'created' as const, + entry: { + entryId: options?.entryId ?? 'fresh-id', + text, + createdAt: options?.createdAt ?? 1000, + updatedAt: 1000 + } + } + } + }) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'first thought' }) + }) + expect(res.status).toBe(201) + const body = await res.json() as { entry: { text: string; entryId: string } } + expect(body.entry.text).toBe('first thought') + expect(calls).toHaveLength(1) + expect(calls[0]?.sessionId).toBe('session-1') + }) + + it('returns 200 with the existing row on duplicate (migration idempotency path)', async () => { + const session = createSession() + const app = createApp(session, { + createScratchlistEntry: () => ({ + outcome: 'duplicate' as const, + entry: { entryId: 'dup', text: 'pre-existing', createdAt: 100, updatedAt: 100 } + }) + }) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'replay', entryId: 'dup' }) + }) + expect(res.status).toBe(200) + const body = await res.json() as { entry: { text: string } } + expect(body.entry.text).toBe('pre-existing') + }) + + it('rejects empty text with 400', async () => { + const session = createSession() + const app = createApp(session) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: '' }) + }) + expect(res.status).toBe(400) + }) + + it('rejects oversize text (>10_000 chars) with 400', async () => { + const session = createSession() + const app = createApp(session) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'x'.repeat(10_001) }) + }) + expect(res.status).toBe(400) + }) + + it('returns 409 when the session is at the cap', async () => { + const session = createSession() + const app = createApp(session, { + countScratchlistEntries: () => 200 + }) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'one too many' }) + }) + expect(res.status).toBe(409) + const body = await res.json() as { code: string } + expect(body.code).toBe('scratchlist_at_cap') + }) + + it('returns 404 when the engine reports session-not-found post-auth', async () => { + // This path covers a race: auth said the session was visible + // (resolveSessionAccess.ok), but by the time we INSERT the row the + // session is gone. The engine returns `session-not-found` and the + // route surfaces a 404. + const session = createSession() + const app = createApp(session, { + createScratchlistEntry: () => ({ outcome: 'session-not-found' as const }) + }) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'never lands' }) + }) + expect(res.status).toBe(404) + }) + + it('returns 404 when the session is not visible to the caller', async () => { + const session = createSession() + const app = createApp(session, { sessionAccess: 'not-found' }) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'auth gate' }) + }) + expect(res.status).toBe(404) + }) +}) + +describe('PUT /api/sessions/:id/scratchlist/:entryId', () => { + it('returns the updated entry on success', async () => { + const session = createSession() + const app = createApp(session, { + updateScratchlistEntry: (_sessionId, entryId, text) => ({ + entryId, + text, + createdAt: 1000, + updatedAt: 5000 + }) + }) + const res = await app.request('/api/sessions/session-1/scratchlist/entry-1', { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'edited' }) + }) + expect(res.status).toBe(200) + const body = await res.json() as { entry: { text: string; entryId: string } } + expect(body.entry.text).toBe('edited') + expect(body.entry.entryId).toBe('entry-1') + }) + + it('returns 404 when the entry does not exist', async () => { + const session = createSession() + const app = createApp(session, { + updateScratchlistEntry: () => null + }) + const res = await app.request('/api/sessions/session-1/scratchlist/missing-id', { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'oops' }) + }) + expect(res.status).toBe(404) + }) + + it('rejects empty text with 400', async () => { + const session = createSession() + const app = createApp(session) + const res = await app.request('/api/sessions/session-1/scratchlist/e1', { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: '' }) + }) + expect(res.status).toBe(400) + }) + + it('returns 403 when the session is in another namespace', async () => { + const session = createSession({ namespace: 'other' }) + const app = createApp(session, { sessionAccess: 'wrong-namespace' }) + const res = await app.request('/api/sessions/session-1/scratchlist/e1', { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'cross-ns' }) + }) + expect(res.status).toBe(403) + }) +}) + +describe('DELETE /api/sessions/:id/scratchlist/:entryId', () => { + it('returns ok:true when the row was removed', async () => { + const session = createSession() + const app = createApp(session, { + deleteScratchlistEntry: () => true + }) + const res = await app.request('/api/sessions/session-1/scratchlist/e1', { + method: 'DELETE' + }) + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ ok: true }) + }) + + it('returns 404 when the row did not exist', async () => { + const session = createSession() + const app = createApp(session, { + deleteScratchlistEntry: () => false + }) + const res = await app.request('/api/sessions/session-1/scratchlist/missing', { + method: 'DELETE' + }) + expect(res.status).toBe(404) + }) + + it('returns 404 when the session is not visible to the caller', async () => { + const session = createSession() + const app = createApp(session, { sessionAccess: 'not-found' }) + const res = await app.request('/api/sessions/session-1/scratchlist/e1', { + method: 'DELETE' + }) + expect(res.status).toBe(404) + }) +}) diff --git a/hub/src/web/routes/sessions.ts b/hub/src/web/routes/sessions.ts index 20cffad9dd..d0133043bb 100644 --- a/hub/src/web/routes/sessions.ts +++ b/hub/src/web/routes/sessions.ts @@ -5,6 +5,9 @@ import { isPermissionModeAllowedForFlavor, RenameSessionRequestSchema, ResumeSessionRequestSchema, + SCRATCHLIST_MAX_ENTRIES, + ScratchlistEntryCreateRequestSchema, + ScratchlistEntryUpdateRequestSchema, SessionCollaborationModeRequestSchema, SessionEffortRequestSchema, SessionModelReasoningEffortRequestSchema, @@ -647,6 +650,131 @@ export function createSessionsRoutes(getSyncEngine: () => SyncEngine | null): Ho } }) + /* + * Scratchlist v2 (tiann/hapi#893). + * + * Operator-private notes attached to a session. All four routes use + * the existing `requireSessionFromParam` guard so the same auth / + * namespace check applies as every other session-scoped route - + * scratchlist contents must NOT leak across namespaces, and a 403 / + * 404 is returned for sessions the caller cannot access. + * + * SSE: every successful mutation emits a `session-updated` patch + * carrying `scratchlistUpdatedAt` (handled in `SyncEngine`). The web + * client uses that as a cache-invalidation token to refetch GET. + */ + + app.get('/sessions/:id/scratchlist', (c) => { + const engine = requireSyncEngine(c, getSyncEngine) + if (engine instanceof Response) { + return engine + } + const sessionResult = requireSessionFromParam(c, engine) + if (sessionResult instanceof Response) { + return sessionResult + } + const entries = engine.listScratchlistEntries(sessionResult.sessionId) + return c.json({ entries }) + }) + + app.post('/sessions/:id/scratchlist', async (c) => { + const engine = requireSyncEngine(c, getSyncEngine) + if (engine instanceof Response) { + return engine + } + const sessionResult = requireSessionFromParam(c, engine) + if (sessionResult instanceof Response) { + return sessionResult + } + + const body = await c.req.json().catch(() => null) + const parsed = ScratchlistEntryCreateRequestSchema.safeParse(body) + if (!parsed.success) { + return c.json({ error: 'Invalid body', issues: parsed.error.issues }, 400) + } + + // Server-side cap enforcement. Mirrors the web-side cap so a + // malicious / runaway client can't drive the table without + // bound. Bypassing the optimistic add path on the web client + // (e.g. direct REST call) hits this guard. Bumped only with the + // shared SCRATCHLIST_MAX_ENTRIES constant. + const currentCount = engine.countScratchlistEntries(sessionResult.sessionId) + if (currentCount >= SCRATCHLIST_MAX_ENTRIES) { + return c.json({ + error: `Scratchlist is at its ${SCRATCHLIST_MAX_ENTRIES}-entry cap`, + code: 'scratchlist_at_cap' + }, 409) + } + + const result = engine.createScratchlistEntry( + sessionResult.sessionId, + parsed.data.text, + { + entryId: parsed.data.entryId, + createdAt: parsed.data.createdAt + } + ) + if (result.outcome === 'session-not-found') { + return c.json({ error: 'Session not found' }, 404) + } + // `duplicate` (same entryId already exists) returns 200 with the + // canonical row so the migration path can retry idempotently. + // The web client treats 200-with-existing as success either way. + return c.json({ entry: result.entry }, result.outcome === 'created' ? 201 : 200) + }) + + app.put('/sessions/:id/scratchlist/:entryId', async (c) => { + const engine = requireSyncEngine(c, getSyncEngine) + if (engine instanceof Response) { + return engine + } + const sessionResult = requireSessionFromParam(c, engine) + if (sessionResult instanceof Response) { + return sessionResult + } + + const entryId = c.req.param('entryId') + if (!entryId) { + return c.json({ error: 'Missing entryId' }, 400) + } + + const body = await c.req.json().catch(() => null) + const parsed = ScratchlistEntryUpdateRequestSchema.safeParse(body) + if (!parsed.success) { + return c.json({ error: 'Invalid body', issues: parsed.error.issues }, 400) + } + + const updated = engine.updateScratchlistEntry( + sessionResult.sessionId, + entryId, + parsed.data.text + ) + if (!updated) { + return c.json({ error: 'Scratchlist entry not found' }, 404) + } + return c.json({ entry: updated }) + }) + + app.delete('/sessions/:id/scratchlist/:entryId', (c) => { + const engine = requireSyncEngine(c, getSyncEngine) + if (engine instanceof Response) { + return engine + } + const sessionResult = requireSessionFromParam(c, engine) + if (sessionResult instanceof Response) { + return sessionResult + } + const entryId = c.req.param('entryId') + if (!entryId) { + return c.json({ error: 'Missing entryId' }, 400) + } + const removed = engine.deleteScratchlistEntry(sessionResult.sessionId, entryId) + if (!removed) { + return c.json({ error: 'Scratchlist entry not found' }, 404) + } + return c.json({ ok: true }) + }) + app.get('/sessions/:id/slash-commands', async (c) => { const engine = requireSyncEngine(c, getSyncEngine) if (engine instanceof Response) { diff --git a/shared/src/apiTypes.ts b/shared/src/apiTypes.ts index e6381da723..0611f15c59 100644 --- a/shared/src/apiTypes.ts +++ b/shared/src/apiTypes.ts @@ -163,6 +163,50 @@ export const RenameSessionRequestSchema = z.object({ export type RenameSessionRequest = z.infer +/** + * Scratchlist v2 (tiann/hapi#893) per-entry caps. + * + * `MAX_ENTRIES` (200) is a per-session ceiling: refuses to create entry + * 201 on the hub. Mirrors `SCRATCHLIST_MAX_ENTRIES` in + * `web/src/lib/scratchlist.ts` so the hub and web agree on the limit - + * the web side has UX for the cap (disabled add button + atCap hint), + * the hub side enforces it as a server-side guard against malicious / + * runaway clients writing arbitrary amounts. + * + * `MAX_TEXT_LENGTH` (10_000) is the per-entry text cap. Mirrors + * `SCRATCHLIST_MAX_TEXT_LENGTH`. The web side truncates rather than + * rejects; the hub-side schema allows up to this length and rejects + * anything longer with 400. + */ +export const SCRATCHLIST_MAX_ENTRIES = 200 +export const SCRATCHLIST_MAX_TEXT_LENGTH = 10_000 + +export const ScratchlistEntryCreateRequestSchema = z.object({ + /** + * Optional client-supplied entry id. Lets the web client preserve its + * pre-v2 localStorage entry ids during migration so the optimistic- + * update path doesn't have to re-key entries already in the React + * tree. New entries created post-v2 can omit this and let the hub + * generate one. + */ + entryId: z.string().min(1).optional(), + text: z.string().min(1).max(SCRATCHLIST_MAX_TEXT_LENGTH), + /** + * Optional client-supplied createdAt. Used by the migration path to + * preserve the original timestamps from localStorage. New entries + * omit this and let the hub stamp `Date.now()`. + */ + createdAt: z.number().int().nonnegative().optional() +}) + +export type ScratchlistEntryCreateRequest = z.infer + +export const ScratchlistEntryUpdateRequestSchema = z.object({ + text: z.string().min(1).max(SCRATCHLIST_MAX_TEXT_LENGTH) +}) + +export type ScratchlistEntryUpdateRequest = z.infer + /** Per-session legacy stream-json → ACP migrator request. See tiann/hapi#824. */ export const CursorMigrateToAcpRequestSchema = z.object({ /** Skip removing the legacy ~/.cursor/chats source store.db even after verify passes. */ diff --git a/shared/src/schemas.ts b/shared/src/schemas.ts index 1b891fbef5..378df16bc7 100644 --- a/shared/src/schemas.ts +++ b/shared/src/schemas.ts @@ -242,11 +242,38 @@ export const SessionPatchSchema = z.object({ serviceTier: z.string().nullable().optional(), permissionMode: PermissionModeSchema.optional(), collaborationMode: CodexCollaborationModeSchema.optional(), - backgroundTaskCount: z.number().optional() + backgroundTaskCount: z.number().optional(), + // tiann/hapi#893 (scratchlist v2). Bumped whenever any entry on the + // session_scratchlist table mutates. Web client uses the change as a + // trigger to refetch the entries query - the timestamp itself is the + // signal, not the payload. Keep this minimal: per the operator's 80/20 + // ruling, scratchlist mutations are rare relative to keep-alive + // patches, so a fresh event type would be overkill. + scratchlistUpdatedAt: z.number().optional() }).strict() export type SessionPatch = z.infer +// tiann/hapi#893: per-session scratchlist entries (operator notes / +// drafts / parking-lot ideas). Hub-side typed-table source of truth; +// web treats localStorage as offline cache only. Single-user notes - +// no collaborative edit semantics (no version field, no conflict +// resolution beyond last-write-wins). +export const ScratchlistEntrySchema = z.object({ + entryId: z.string().min(1), + text: z.string(), + createdAt: z.number(), + updatedAt: z.number() +}) + +export type ScratchlistEntry = z.infer + +export const ScratchlistEntriesResponseSchema = z.object({ + entries: z.array(ScratchlistEntrySchema) +}) + +export type ScratchlistEntriesResponse = z.infer + export const MachineMetadataSchema = z.object({ host: z.string(), platform: z.string(), diff --git a/web/src/api/client.ts b/web/src/api/client.ts index d37e0828bc..e8805226d9 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -679,6 +679,60 @@ export class ApiClient { }) } + /* + * Scratchlist v2 (tiann/hapi#893). + * + * The hub is the durable store; localStorage is demoted to an + * offline cache. Mutations return the canonical entry so optimistic + * updates can reconcile with the hub-stamped `updatedAt`. + */ + + async getScratchlist(sessionId: string): Promise<{ + entries: Array<{ entryId: string; text: string; createdAt: number; updatedAt: number }> + }> { + return await this.request( + `/api/sessions/${encodeURIComponent(sessionId)}/scratchlist` + ) + } + + async createScratchlistEntry( + sessionId: string, + body: { text: string; entryId?: string; createdAt?: number } + ): Promise<{ + entry: { entryId: string; text: string; createdAt: number; updatedAt: number } + }> { + return await this.request( + `/api/sessions/${encodeURIComponent(sessionId)}/scratchlist`, + { + method: 'POST', + body: JSON.stringify(body) + } + ) + } + + async updateScratchlistEntry( + sessionId: string, + entryId: string, + text: string + ): Promise<{ + entry: { entryId: string; text: string; createdAt: number; updatedAt: number } + }> { + return await this.request( + `/api/sessions/${encodeURIComponent(sessionId)}/scratchlist/${encodeURIComponent(entryId)}`, + { + method: 'PUT', + body: JSON.stringify({ text }) + } + ) + } + + async deleteScratchlistEntry(sessionId: string, entryId: string): Promise { + await this.request( + `/api/sessions/${encodeURIComponent(sessionId)}/scratchlist/${encodeURIComponent(entryId)}`, + { method: 'DELETE' } + ) + } + async fetchVoiceToken(options?: { customAgentId?: string; customApiKey?: string; voiceId?: string }): Promise<{ allowed: boolean token?: string diff --git a/web/src/components/AssistantChat/ScratchlistMigrationBanner.test.tsx b/web/src/components/AssistantChat/ScratchlistMigrationBanner.test.tsx new file mode 100644 index 0000000000..d1b8be7556 --- /dev/null +++ b/web/src/components/AssistantChat/ScratchlistMigrationBanner.test.tsx @@ -0,0 +1,60 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { cleanup, fireEvent, render, screen } from '@testing-library/react' +import { I18nProvider } from '@/lib/i18n-context' +import { ScratchlistMigrationBanner } from './ScratchlistMigrationBanner' + +afterEach(() => cleanup()) + +function renderBanner(props: { + migrationStatus: 'idle' | 'migrating' | 'completed' | 'dismissed' | 'pre-migrated' + onDismiss?: () => void +}) { + return render( + + + + ) +} + +describe('ScratchlistMigrationBanner', () => { + it('renders nothing in idle state', () => { + const { container } = renderBanner({ migrationStatus: 'idle' }) + expect(container.firstChild).toBeNull() + }) + + it('renders nothing while the migration is in flight', () => { + const { container } = renderBanner({ migrationStatus: 'migrating' }) + expect(container.firstChild).toBeNull() + }) + + it('renders nothing for the dismissed state', () => { + const { container } = renderBanner({ migrationStatus: 'dismissed' }) + expect(container.firstChild).toBeNull() + }) + + it('renders nothing for pre-migrated sessions (operator already saw the banner once)', () => { + const { container } = renderBanner({ migrationStatus: 'pre-migrated' }) + expect(container.firstChild).toBeNull() + }) + + it('renders the banner with title, body, and dismiss button when status is completed', () => { + renderBanner({ migrationStatus: 'completed' }) + expect(screen.getByTestId('scratchlist-migration-banner')).toBeTruthy() + // Title contains the cross-device cue. + expect(screen.getByText(/syncs across devices/i)).toBeTruthy() + // Body contains the "nothing was lost" reassurance. + expect(screen.getByText(/nothing was lost/i)).toBeTruthy() + // Dismiss button exists. + expect(screen.getByTestId('scratchlist-migration-banner-dismiss')).toBeTruthy() + }) + + it('calls onDismiss when the dismiss button is clicked', () => { + const onDismiss = vi.fn() + renderBanner({ migrationStatus: 'completed', onDismiss }) + fireEvent.click(screen.getByTestId('scratchlist-migration-banner-dismiss')) + expect(onDismiss).toHaveBeenCalledTimes(1) + }) +}) diff --git a/web/src/components/AssistantChat/ScratchlistMigrationBanner.tsx b/web/src/components/AssistantChat/ScratchlistMigrationBanner.tsx new file mode 100644 index 0000000000..eb6520afcb --- /dev/null +++ b/web/src/components/AssistantChat/ScratchlistMigrationBanner.tsx @@ -0,0 +1,64 @@ +import { useTranslation } from '@/lib/use-translation' + +/** + * tiann/hapi#893 (scratchlist v2): one-time banner shown the first time + * a v2-aware client encounters localStorage entries on a session and + * pushes them up to the hub silently. + * + * Visibility contract: + * - Renders only when `migrationStatus === 'completed'` (the hook's + * signal that the migration just ran in this session). + * - Operator-affirmative dismissal: clicking the dismiss button writes + * the per-session `hapi.scratchlist.v2.banner-dismissed.${id}` flag + * so the banner does not reappear on reload. + * - Mirrors the dismissal pattern of `CursorMigrationBanner.tsx` so + * the surface is familiar to operators. + * + * Copy explains what was migrated and confirms nothing was lost. We + * deliberately do not show entry counts - the banner is informational, + * not transactional, and a count would imply the operator should + * verify, which we don't want them to feel they need to do. + */ +export function ScratchlistMigrationBanner({ + migrationStatus, + onDismiss +}: { + migrationStatus: + | 'idle' + | 'migrating' + | 'completed' + | 'dismissed' + | 'pre-migrated' + onDismiss: () => void +}) { + const { t } = useTranslation() + if (migrationStatus !== 'completed') { + return null + } + return ( +
+
+
+
+ {t('scratchlist.migrationBanner.title')} +
+
+ {t('scratchlist.migrationBanner.body')} +
+
+ +
+
+ ) +} diff --git a/web/src/components/SessionChat.tsx b/web/src/components/SessionChat.tsx index af612bce5e..64daa54d80 100644 --- a/web/src/components/SessionChat.tsx +++ b/web/src/components/SessionChat.tsx @@ -27,7 +27,8 @@ import { resolvePendingSchedule } from '@/components/AssistantChat/ScheduleTimeP import { HappyThread } from '@/components/AssistantChat/HappyThread' import { QueuedMessagesBar } from '@/components/AssistantChat/QueuedMessagesBar' import { ScratchlistDrawer } from '@/components/AssistantChat/ScratchlistPanel' -import { useScratchlist } from '@/lib/use-scratchlist' +import { useHubScratchlist } from '@/lib/use-hub-scratchlist' +import { ScratchlistMigrationBanner } from '@/components/AssistantChat/ScratchlistMigrationBanner' import { useHappyRuntime } from '@/lib/assistant-runtime' import { createAttachmentAdapter } from '@/lib/attachmentAdapter' import { consumeSharePendingTransfer } from '@/lib/sharePendingState' @@ -263,9 +264,9 @@ function ShareSeedConsumer(props: { sessionId: string; sessionActive: boolean }) * composer-toolbar counter and the drawer share one source of truth. */ export function ScratchlistDrawerHost(props: { - entries: ReturnType['entries'] - onMove: ReturnType['move'] - onDelete: ReturnType['remove'] + entries: ReturnType['entries'] + onMove: ReturnType['move'] + onDelete: ReturnType['remove'] onSend: (text: string, attachments?: AttachmentMetadata[], scheduledAt?: number | null) => Promise /** * Called when the operator promotes an entry to the composer. @@ -410,7 +411,7 @@ function SessionChatInner(props: SessionChatProps) { const [outlineOpen, setOutlineOpen] = useState(false) const [cursorSelectedBase, setCursorSelectedBase] = useState('auto') const lastSyncedCursorModelRef = useRef(undefined) - const scratchlist = useScratchlist(props.session.id) + const scratchlist = useHubScratchlist(props.session.id, props.api) const [scratchlistMode, setScratchlistMode] = useState(false) // Mode resets across sessions implicitly: SessionChat is keyed by // session.id at the public-export boundary, so a session switch @@ -1159,6 +1160,19 @@ function SessionChatInner(props: SessionChatProps) { ) : null} + {/* + * tiann/hapi#893: one-time banner shown on first + * v2-load when localStorage entries got migrated to + * the hub. Sits above the drawer so the operator + * sees it whether or not the drawer is open. + * Auto-renders nothing unless `migrationStatus === + * 'completed'`. + */} + +
{/* * Scratchlist drawer - composer-controlled. Only diff --git a/web/src/components/SessionHeader.tsx b/web/src/components/SessionHeader.tsx index a65f84d0fe..c37499b414 100644 --- a/web/src/components/SessionHeader.tsx +++ b/web/src/components/SessionHeader.tsx @@ -7,6 +7,7 @@ import { SessionActionMenu } from '@/components/SessionActionMenu' import { SessionExportDialog } from '@/components/SessionExportDialog' import { RenameSessionDialog } from '@/components/RenameSessionDialog' import { ConfirmDialog } from '@/components/ui/ConfirmDialog' +import { useScratchlistCount } from '@/lib/use-scratchlist-count' import { formatReopenError } from '@/lib/reopenError' import { getSessionModelLabel } from '@/lib/sessionModelLabel' import { useTranslation } from '@/lib/use-translation' @@ -117,6 +118,11 @@ export function SessionHeader(props: { session.metadata?.flavor ?? null ) const [reopenError, setReopenError] = useState(null) + // tiann/hapi#893: surface the scratchlist entry count in the + // delete-confirm copy so the operator knows what cascades when they + // confirm. Read-only hook reuses the cache filled by SessionChat - + // no extra network when both components are mounted. + const scratchlistCount = useScratchlistCount(session.id, api) const handleDelete = async () => { await deleteSession() @@ -290,7 +296,16 @@ export function SessionHeader(props: { isOpen={deleteOpen} onClose={() => setDeleteOpen(false)} title={t('dialog.delete.title')} - description={t('dialog.delete.description', { name: title })} + description={ + scratchlistCount > 0 + ? `${t('dialog.delete.description', { name: title })} ${t( + scratchlistCount === 1 + ? 'dialog.delete.scratchlist.one' + : 'dialog.delete.scratchlist.other', + { n: String(scratchlistCount) } + )}` + : t('dialog.delete.description', { name: title }) + } confirmLabel={t('dialog.delete.confirm')} confirmingLabel={t('dialog.delete.confirming')} onConfirm={handleDelete} diff --git a/web/src/hooks/useSSE.ts b/web/src/hooks/useSSE.ts index fe462d82af..91d8c8037b 100644 --- a/web/src/hooks/useSSE.ts +++ b/web/src/hooks/useSSE.ts @@ -137,7 +137,8 @@ export function useSSE(options: { sessions: boolean machines: boolean sessionIds: Set - }>({ sessions: false, machines: false, sessionIds: new Set() }) + scratchlistSessionIds: Set + }>({ sessions: false, machines: false, sessionIds: new Set(), scratchlistSessionIds: new Set() }) const reconnectTimerRef = useRef | null>(null) const reconnectAttemptRef = useRef(0) const lastActivityAtRef = useRef(0) @@ -182,6 +183,7 @@ export function useSSE(options: { pendingInvalidationsRef.current.sessions = false pendingInvalidationsRef.current.machines = false pendingInvalidationsRef.current.sessionIds.clear() + pendingInvalidationsRef.current.scratchlistSessionIds.clear() if (reconnectTimerRef.current) { clearTimeout(reconnectTimerRef.current) reconnectTimerRef.current = null @@ -240,17 +242,24 @@ export function useSSE(options: { const flushInvalidations = () => { const pending = pendingInvalidationsRef.current - if (!pending.sessions && !pending.machines && pending.sessionIds.size === 0) { + if ( + !pending.sessions + && !pending.machines + && pending.sessionIds.size === 0 + && pending.scratchlistSessionIds.size === 0 + ) { return } const shouldInvalidateSessions = pending.sessions const shouldInvalidateMachines = pending.machines const sessionIds = Array.from(pending.sessionIds) + const scratchlistSessionIds = Array.from(pending.scratchlistSessionIds) pending.sessions = false pending.machines = false pending.sessionIds.clear() + pending.scratchlistSessionIds.clear() const tasks: Array> = [] if (shouldInvalidateSessions) { @@ -259,6 +268,9 @@ export function useSSE(options: { for (const sessionId of sessionIds) { tasks.push(queryClient.invalidateQueries({ queryKey: queryKeys.session(sessionId) })) } + for (const sessionId of scratchlistSessionIds) { + tasks.push(queryClient.invalidateQueries({ queryKey: queryKeys.scratchlist(sessionId) })) + } if (shouldInvalidateMachines) { tasks.push(queryClient.invalidateQueries({ queryKey: queryKeys.machines })) } @@ -289,6 +301,11 @@ export function useSSE(options: { scheduleInvalidationFlush() } + const queueScratchlistInvalidation = (sessionId: string) => { + pendingInvalidationsRef.current.scratchlistSessionIds.add(sessionId) + scheduleInvalidationFlush() + } + const queueMachinesInvalidation = () => { pendingInvalidationsRef.current.machines = true scheduleInvalidationFlush() @@ -507,6 +524,13 @@ export function useSSE(options: { if (!summaryPatched) { queueSessionListInvalidation() } + // tiann/hapi#893: piggybacked scratchlist token. + // The patch itself does not carry the entries - + // the timestamp is the change-detection signal, + // which triggers the dedicated query refetch. + if (Object.prototype.hasOwnProperty.call(patch, 'scratchlistUpdatedAt')) { + queueScratchlistInvalidation(event.sessionId) + } } else { queueSessionDetailInvalidation(event.sessionId) queueSessionListInvalidation() diff --git a/web/src/lib/locales/en.ts b/web/src/lib/locales/en.ts index d174da5a0d..f7795281cf 100644 --- a/web/src/lib/locales/en.ts +++ b/web/src/lib/locales/en.ts @@ -176,6 +176,8 @@ export default { 'dialog.reopen.dismiss': 'Dismiss', 'dialog.delete.title': 'Delete Session', 'dialog.delete.description': 'Are you sure you want to delete "{name}"? This action cannot be undone.', + 'dialog.delete.scratchlist.one': 'This will also delete 1 scratchlist entry.', + 'dialog.delete.scratchlist.other': 'This will also delete {n} scratchlist entries.', 'dialog.delete.confirm': 'Delete', 'dialog.delete.confirming': 'Deleting…', 'dialog.error.default': 'Operation failed. Please try again.', @@ -448,6 +450,9 @@ export default { 'scratchlist.action.copy': 'Copy to clipboard', 'scratchlist.action.copied': 'Copied!', 'scratchlist.action.delete': 'Delete entry', + 'scratchlist.migrationBanner.title': 'Scratchlist now syncs across devices', + 'scratchlist.migrationBanner.body': 'Your existing notes were copied to the hub - nothing was lost. From now on, edits in this session will appear on every device that you use.', + 'scratchlist.migrationBanner.dismiss': 'Got it', 'fue.newFeatureDot': 'New feature available', 'fue.gotIt': 'Got it', 'fue.closeAriaLabel': 'Close explainer', diff --git a/web/src/lib/locales/zh-CN.ts b/web/src/lib/locales/zh-CN.ts index a093dda4ed..08e98c7961 100644 --- a/web/src/lib/locales/zh-CN.ts +++ b/web/src/lib/locales/zh-CN.ts @@ -179,6 +179,8 @@ export default { 'dialog.delete.title': '删除会话', 'dialog.delete.description': '确定要删除 "{name}" 吗？此操作无法撤销。', + 'dialog.delete.scratchlist.one': '这也将删除 1 条草稿清单条目。', + 'dialog.delete.scratchlist.other': '这也将删除 {n} 条草稿清单条目。', 'dialog.delete.confirm': '删除', 'dialog.delete.confirming': '删除中…', @@ -452,6 +454,9 @@ export default { 'scratchlist.action.copy': '复制到剪贴板', 'scratchlist.action.copied': '已复制！', 'scratchlist.action.delete': '删除条目', + 'scratchlist.migrationBanner.title': '草稿清单现已跨设备同步', + 'scratchlist.migrationBanner.body': '您现有的笔记已复制到 hub - 没有丢失任何内容。从现在开始,在此会话中的编辑会在您使用的每台设备上显示。', + 'scratchlist.migrationBanner.dismiss': '知道了', 'fue.newFeatureDot': '新功能可用', 'fue.gotIt': '知道了', 'fue.closeAriaLabel': '关闭说明', diff --git a/web/src/lib/query-keys.ts b/web/src/lib/query-keys.ts index e7adcfb309..6991a47122 100644 --- a/web/src/lib/query-keys.ts +++ b/web/src/lib/query-keys.ts @@ -23,4 +23,5 @@ export const queryKeys = { sessionOpencodeReasoningEffortOptions: (sessionId: string) => ['session-opencode-reasoning-effort-options', sessionId] as const, machineOpencodeModelsForCwd: (machineId: string, cwd: string) => ['machine-opencode-models', machineId, cwd] as const, skills: (sessionId: string) => ['skills', sessionId] as const, + scratchlist: (sessionId: string) => ['scratchlist', sessionId] as const, } diff --git a/web/src/lib/use-hub-scratchlist.test.tsx b/web/src/lib/use-hub-scratchlist.test.tsx new file mode 100644 index 0000000000..74883ac824 --- /dev/null +++ b/web/src/lib/use-hub-scratchlist.test.tsx @@ -0,0 +1,360 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { renderHook, act, waitFor } from '@testing-library/react' +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import type { ReactNode } from 'react' +import type { ApiClient } from '@/api/client' +import { useHubScratchlist } from './use-hub-scratchlist' + +/** + * Tests for the v2 hub-backed scratchlist hook (tiann/hapi#893). + * Covers: + * - initial fetch + * - optimistic add + rollback on error + * - optimistic delete + rollback on error + * - update mutation + * - first-load localStorage → hub migration + banner status flip + * - banner dismissal persistence + * - per-session migration flag prevents re-migration + * - cap enforcement returns false from add() + * - local-only reorder via move() + * + * Per-test session id: each test calls `makeSid()` to get a fresh + * session-scoped localStorage namespace. The hook's offline-cache + * useEffect mirrors entries to `hapi.scratchlist.v1.${sessionId}` and + * the cleanup effect can flush AFTER `afterEach` clears localStorage + * for the next test, leaking entries that re-trigger the migration + * path. Unique session ids sidestep the race. + */ + +type HubEntry = { entryId: string; text: string; createdAt: number; updatedAt: number } + +function createWrapper() { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: Infinity }, + mutations: { retry: false } + } + }) + return function Wrapper({ children }: { children: ReactNode }) { + return {children} + } +} + +function createMockApi(overrides: Partial<{ + getScratchlist: (sessionId: string) => Promise<{ entries: HubEntry[] }> + createScratchlistEntry: (sessionId: string, body: { text: string; entryId?: string; createdAt?: number }) => Promise<{ entry: HubEntry }> + updateScratchlistEntry: (sessionId: string, entryId: string, text: string) => Promise<{ entry: HubEntry }> + deleteScratchlistEntry: (sessionId: string, entryId: string) => Promise +}> = {}): ApiClient { + return { + getScratchlist: overrides.getScratchlist ?? (async () => ({ entries: [] })), + createScratchlistEntry: overrides.createScratchlistEntry + ?? (async (_sessionId, body) => ({ + entry: { + entryId: body.entryId ?? `auto-${Math.random()}`, + text: body.text, + createdAt: body.createdAt ?? Date.now(), + updatedAt: Date.now() + } + })), + updateScratchlistEntry: overrides.updateScratchlistEntry + ?? (async (_sessionId, entryId, text) => ({ + entry: { entryId, text, createdAt: 1000, updatedAt: 5000 } + })), + deleteScratchlistEntry: overrides.deleteScratchlistEntry ?? (async () => undefined) + } as unknown as ApiClient +} + +let nextSessionIdCounter = 0 +function makeSid(): string { + nextSessionIdCounter += 1 + return `s-${nextSessionIdCounter}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}` +} + +beforeEach(() => { + localStorage.clear() +}) + +afterEach(() => { + localStorage.clear() + vi.restoreAllMocks() +}) + +describe('useHubScratchlist - initial fetch', () => { + it('exposes entries returned by the hub', async () => { + const sid = makeSid() + const api = createMockApi({ + getScratchlist: async () => ({ + entries: [ + { entryId: 'a', text: 'first', createdAt: 1000, updatedAt: 1000 }, + { entryId: 'b', text: 'second', createdAt: 2000, updatedAt: 2000 } + ] + }) + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.entries.length).toBe(2)) + expect(result.current.entries.map((e) => e.id)).toEqual(['a', 'b']) + }) +}) + +describe('useHubScratchlist - add', () => { + it('optimistically inserts the new entry then reconciles with the hub-returned row', async () => { + const sid = makeSid() + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }), + createScratchlistEntry: async (_s, body) => ({ + entry: { entryId: 'hub-id', text: body.text, createdAt: 5000, updatedAt: 5000 } + }) + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.isLoading).toBe(false)) + + let added: boolean | undefined + await act(async () => { + added = await result.current.add('new note') + }) + expect(added).toBe(true) + await waitFor(() => expect(result.current.entries.length).toBe(1)) + expect(result.current.entries[0]?.id).toBe('hub-id') + expect(result.current.entries[0]?.text).toBe('new note') + }) + + it('rolls back when the hub rejects the create', async () => { + const sid = makeSid() + const api = createMockApi({ + getScratchlist: async () => ({ + entries: [{ entryId: 'a', text: 'existing', createdAt: 1000, updatedAt: 1000 }] + }), + createScratchlistEntry: async () => { + throw new Error('HTTP 500') + } + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.entries.length).toBe(1)) + + let added: boolean | undefined + await act(async () => { + added = await result.current.add('doomed') + }) + expect(added).toBe(false) + // After rollback, the original list is intact (no optimistic ghost). + expect(result.current.entries.map((e) => e.id)).toEqual(['a']) + }) + + it('refuses to add empty text without calling the hub', async () => { + const sid = makeSid() + const create = vi.fn(async () => ({ + entry: { entryId: 'x', text: '', createdAt: 0, updatedAt: 0 } + })) + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }), + createScratchlistEntry: create + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.isLoading).toBe(false)) + + let added: boolean | undefined + await act(async () => { + added = await result.current.add(' ') + }) + expect(added).toBe(false) + expect(create).not.toHaveBeenCalled() + }) + + it('refuses to add when at the 200-entry cap', async () => { + const sid = makeSid() + const existing: HubEntry[] = Array.from({ length: 200 }, (_, i) => ({ + entryId: `id-${i}`, + text: `note-${i}`, + createdAt: i, + updatedAt: i + })) + const create = vi.fn() + const api = createMockApi({ + getScratchlist: async () => ({ entries: existing }), + createScratchlistEntry: create as never + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.entries.length).toBe(200)) + + let added: boolean | undefined + await act(async () => { + added = await result.current.add('overflow') + }) + expect(added).toBe(false) + expect(create).not.toHaveBeenCalled() + }) +}) + +describe('useHubScratchlist - delete', () => { + it('optimistically removes the entry and survives a network error via rollback', async () => { + const sid = makeSid() + const api = createMockApi({ + getScratchlist: async () => ({ + entries: [ + { entryId: 'a', text: 'A', createdAt: 1, updatedAt: 1 }, + { entryId: 'b', text: 'B', createdAt: 2, updatedAt: 2 } + ] + }), + deleteScratchlistEntry: async () => { + throw new Error('HTTP 500') + } + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.entries.length).toBe(2)) + + await act(async () => { + await result.current.remove('a') + }) + // After rollback the entry is restored. + await waitFor(() => expect(result.current.entries.length).toBe(2)) + expect(result.current.entries.map((e) => e.id).sort()).toEqual(['a', 'b']) + }) +}) + +describe('useHubScratchlist - update', () => { + it('optimistically updates text and reconciles with the hub-returned row', async () => { + const sid = makeSid() + const api = createMockApi({ + getScratchlist: async () => ({ + entries: [{ entryId: 'a', text: 'before', createdAt: 1, updatedAt: 1 }] + }), + updateScratchlistEntry: async (_s, entryId, text) => ({ + entry: { entryId, text, createdAt: 1, updatedAt: 5 } + }) + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.entries.length).toBe(1)) + + await act(async () => { + await result.current.update('a', 'after') + }) + await waitFor(() => expect(result.current.entries[0]?.text).toBe('after')) + }) +}) + +describe('useHubScratchlist - localStorage migration', () => { + function seedV1Entries(sid: string) { + localStorage.setItem( + `hapi.scratchlist.v1.${sid}`, + JSON.stringify([ + { id: 'old-1', text: 'pre-v2 note', createdAt: 100 }, + { id: 'old-2', text: 'another', createdAt: 200 } + ]) + ) + } + + it('uploads localStorage entries when the hub returns empty and flips status to completed', async () => { + const sid = makeSid() + seedV1Entries(sid) + const create = vi.fn(async (_s: string, body: { text: string; entryId?: string; createdAt?: number }) => ({ + entry: { + entryId: body.entryId ?? 'fresh', + text: body.text, + createdAt: body.createdAt ?? 999, + updatedAt: 999 + } + })) + let fetchCount = 0 + const api = createMockApi({ + getScratchlist: async () => { + fetchCount += 1 + if (fetchCount === 1) { + return { entries: [] } + } + return { + entries: [ + { entryId: 'old-1', text: 'pre-v2 note', createdAt: 100, updatedAt: 100 }, + { entryId: 'old-2', text: 'another', createdAt: 200, updatedAt: 200 } + ] + } + }, + createScratchlistEntry: create + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + + await waitFor(() => expect(result.current.migrationStatus).toBe('completed')) + expect(create).toHaveBeenCalledTimes(2) + const entryIds = create.mock.calls.map((c) => (c[1] as { entryId?: string }).entryId) + expect(entryIds.sort()).toEqual(['old-1', 'old-2']) + expect(localStorage.getItem(`hapi.scratchlist.v2.migrated.${sid}`)).toBe('1') + }) + + it('does not re-migrate on a mount where the migrated flag is already set', async () => { + const sid = makeSid() + seedV1Entries(sid) + localStorage.setItem(`hapi.scratchlist.v2.migrated.${sid}`, '1') + const create = vi.fn() + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }), + createScratchlistEntry: create as never + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.isLoading).toBe(false)) + await new Promise((r) => setTimeout(r, 30)) + expect(create).not.toHaveBeenCalled() + expect(['pre-migrated', 'idle']).toContain(result.current.migrationStatus) + }) + + it('dismissMigrationBanner persists the dismissal flag and flips status to dismissed', async () => { + const sid = makeSid() + seedV1Entries(sid) + const api = createMockApi({ + getScratchlist: async () => ({ + entries: [{ entryId: 'old-1', text: 'pre-v2 note', createdAt: 100, updatedAt: 100 }] + }), + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.entries.length).toBe(1)) + + // Dismissal is independent of how `completed` was reached - the + // status flip is what matters for banner visibility (the banner + // only renders for `completed`, so dismissing flips it off). + act(() => { + result.current.dismissMigrationBanner() + }) + expect(result.current.migrationStatus).toBe('dismissed') + expect(localStorage.getItem(`hapi.scratchlist.v2.banner-dismissed.${sid}`)).toBe('1') + }) + + it('skips migration when localStorage is empty but still sets the flag (so future loads do not probe again)', async () => { + const sid = makeSid() + const create = vi.fn() + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }), + createScratchlistEntry: create as never + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.isLoading).toBe(false)) + await waitFor(() => expect(localStorage.getItem(`hapi.scratchlist.v2.migrated.${sid}`)).toBe('1')) + expect(create).not.toHaveBeenCalled() + expect(result.current.migrationStatus).toBe('idle') + }) +}) + +describe('useHubScratchlist - reorder (local-only)', () => { + it('move() reorders entries in-place without calling the hub', async () => { + const sid = makeSid() + const updateMock = vi.fn() + const api = createMockApi({ + getScratchlist: async () => ({ + entries: [ + { entryId: 'top', text: 'top', createdAt: 100, updatedAt: 100 }, + { entryId: 'bot', text: 'bot', createdAt: 50, updatedAt: 50 } + ] + }), + updateScratchlistEntry: updateMock as never + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.entries.length).toBe(2)) + expect(result.current.entries.map((e) => e.id)).toEqual(['top', 'bot']) + + await act(async () => { + result.current.move('bot', 'up') + }) + await waitFor(() => { + expect(result.current.entries.map((e) => e.id)).toEqual(['bot', 'top']) + }) + expect(updateMock).not.toHaveBeenCalled() + }) +}) diff --git a/web/src/lib/use-hub-scratchlist.ts b/web/src/lib/use-hub-scratchlist.ts new file mode 100644 index 0000000000..ca17b07296 --- /dev/null +++ b/web/src/lib/use-hub-scratchlist.ts @@ -0,0 +1,465 @@ +import { useCallback, useEffect, useRef, useState } from 'react' +import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query' +import type { ApiClient } from '@/api/client' +import { queryKeys } from '@/lib/query-keys' +import { + moveScratchlistEntry, + readScratchlist, + SCRATCHLIST_MAX_ENTRIES, + SCRATCHLIST_MAX_TEXT_LENGTH, + type ScratchlistEntry, +} from '@/lib/scratchlist' + +/** + * tiann/hapi#893 (scratchlist v2): hub-synced replacement for the v1 + * `useScratchlist` localStorage-only hook. + * + * Source-of-truth shift + * --------------------- + * v1: `localStorage` was canonical, persisted on every mutation, read on + * mount. v2: hub becomes canonical (durable + cross-device); localStorage + * is demoted to an offline cache. This hook fetches via TanStack Query + * keyed by `queryKeys.scratchlist(sessionId)`; the SSE handler in + * `useSSE.ts` invalidates that key when a `session-updated` patch + * carries `scratchlistUpdatedAt`, so a write in tab A surfaces in tab B + * within ~1 SSE round-trip. + * + * Optimistic mutations + * -------------------- + * Add/delete/update apply optimistically to the cached entries list and + * roll back on error using TanStack's `onMutate` / `onError` snapshot + * pattern. The server returns the canonical row (with hub-stamped + * `updatedAt`) on success and we reconcile. + * + * Reorder (move) + * -------------- + * Reorder is local-only in v2.0: the hub stores entries with stable + * `createdAt` (used by future overseer queries per operator decision), + * and adding a `position` column / cross-device order semantics is a + * v2.1 concern. The move is applied to the cached array client-side; a + * subsequent invalidation refetch will reset the order. This is a + * documented limitation, not a bug - see `tiann/hapi#893` body. + * + * Migration on first v2-load + * -------------------------- + * When the hook mounts on a session that has localStorage entries from + * v1 AND the hub returns no entries AND the per-session migration flag + * has not been set, we push the localStorage entries up via POST, + * preserving their original `id` and `createdAt`. The flag + * `hapi.scratchlist.v2.migrated.${sessionId}` then prevents repeated + * migrations across reloads. The per-session banner status reflects + * whether the migration just ran (`completed`) or was acknowledged + * (`dismissed`); the banner component listens for this signal. + */ + +const MIGRATION_FLAG_PREFIX = 'hapi.scratchlist.v2.migrated.' +const MIGRATION_BANNER_DISMISSED_PREFIX = 'hapi.scratchlist.v2.banner-dismissed.' + +export type ScratchlistMigrationStatus = + | 'idle' // no localStorage entries; nothing to migrate + | 'migrating' // POSTs in flight + | 'completed' // migration ran in this mount; banner should show + | 'dismissed' // banner was acknowledged; do not surface again + | 'pre-migrated' // migration was completed in a prior session and the user already saw the banner + +type HubEntry = { + entryId: string + text: string + createdAt: number + updatedAt: number +} + +type ScratchlistResponse = { entries: HubEntry[] } + +function readMigrationFlag(sessionId: string): boolean { + if (typeof window === 'undefined') return false + try { + return window.localStorage.getItem(`${MIGRATION_FLAG_PREFIX}${sessionId}`) === '1' + } catch { + return false + } +} + +function writeMigrationFlag(sessionId: string): void { + if (typeof window === 'undefined') return + try { + window.localStorage.setItem(`${MIGRATION_FLAG_PREFIX}${sessionId}`, '1') + } catch { + // Storage quota / private mode: non-fatal. Worst case the migration + // re-runs next mount; the hub returns 200/duplicate for collisions + // (see hub/src/store/scratchlist.ts createScratchlistEntry). + } +} + +function readBannerDismissed(sessionId: string): boolean { + if (typeof window === 'undefined') return false + try { + return window.localStorage.getItem(`${MIGRATION_BANNER_DISMISSED_PREFIX}${sessionId}`) === '1' + } catch { + return false + } +} + +function writeBannerDismissed(sessionId: string): void { + if (typeof window === 'undefined') return + try { + window.localStorage.setItem(`${MIGRATION_BANNER_DISMISSED_PREFIX}${sessionId}`, '1') + } catch { + // Non-fatal: banner reappears on next mount until storage works. + } +} + +/** + * Convert hub entries into the in-memory shape the panel components + * expect (`ScratchlistEntry` from `lib/scratchlist.ts`). Hub `entryId` + * maps to local `id`. Hub `updatedAt` is dropped from the local view + * because v1 components don't render it - it's tracked in the cache for + * SSE reconciliation, not in the props. + */ +function toLocalEntry(hub: HubEntry): ScratchlistEntry { + return { + id: hub.entryId, + text: hub.text, + createdAt: hub.createdAt + } +} + +function makeOptimisticHubEntry(text: string, now: number): HubEntry { + const fallbackId = typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function' + ? crypto.randomUUID() + : `scratch-${now}-${Math.random().toString(36).slice(2, 10)}` + return { + entryId: fallbackId, + text, + createdAt: now, + updatedAt: now + } +} + +export function useHubScratchlist( + sessionId: string, + api: ApiClient | null +): { + entries: ScratchlistEntry[] + isLoading: boolean + add: (text: string) => Promise + remove: (id: string) => Promise + update: (id: string, text: string) => Promise + move: (id: string, direction: 'up' | 'down') => void + migrationStatus: ScratchlistMigrationStatus + dismissMigrationBanner: () => void +} { + const queryClient = useQueryClient() + const queryKey = queryKeys.scratchlist(sessionId) + const enabled = Boolean(api && sessionId) + const migrationAttemptedRef = useRef(false) + const [migrationStatus, setMigrationStatus] = useState(() => { + if (!sessionId) return 'idle' + if (readBannerDismissed(sessionId)) return 'dismissed' + if (readMigrationFlag(sessionId)) return 'pre-migrated' + return 'idle' + }) + + const query = useQuery({ + queryKey, + queryFn: async () => { + if (!api || !sessionId) { + return { entries: [] } + } + return await api.getScratchlist(sessionId) + }, + enabled, + // 30s - matches `useSession` cache freshness so cross-tab SSE + // invalidation is the dominant refresh signal, not stale-time + // expiry. + staleTime: 30_000, + }) + + // Reset migration tracking when the session id changes. The ref-based + // gate prevents the migration effect from re-firing on every render + // for the same session even if the query data fluctuates between + // empty and non-empty during in-flight optimistic add/rollback. + useEffect(() => { + migrationAttemptedRef.current = false + if (!sessionId) { + setMigrationStatus('idle') + return + } + if (readBannerDismissed(sessionId)) { + setMigrationStatus('dismissed') + } else if (readMigrationFlag(sessionId)) { + setMigrationStatus('pre-migrated') + } else { + setMigrationStatus('idle') + } + }, [sessionId]) + + // Migration trigger: runs ONCE per session when: + // - api is available + // - hub returned an empty list + // - migration flag is unset + // - localStorage holds v1 entries + // The actual POSTs are sequential to keep retry semantics simple + // and to avoid bursts that could trip rate-limit guards. For the + // typical case of "a handful of stale entries" this is fine. + useEffect(() => { + if (!api || !sessionId) return + if (migrationAttemptedRef.current) return + if (query.isLoading || query.isFetching) return + if (!query.data) return + if (query.data.entries.length > 0) return + if (readMigrationFlag(sessionId)) return + + const localEntries = readScratchlist(sessionId) + if (localEntries.length === 0) { + // Nothing to migrate but we still mark the session migrated + // so subsequent loads skip the localStorage probe. + writeMigrationFlag(sessionId) + return + } + + migrationAttemptedRef.current = true + setMigrationStatus('migrating') + + void (async () => { + try { + // Preserve creation order by POSTing in the order + // localStorage holds them. The hub orders by createdAt + // DESC at read time, so source order doesn't actually + // matter for visual layout - but we keep it deterministic + // for the migration retry path. + for (const entry of localEntries) { + const text = entry.text.length > SCRATCHLIST_MAX_TEXT_LENGTH + ? entry.text.slice(0, SCRATCHLIST_MAX_TEXT_LENGTH) + : entry.text + if (text.trim().length === 0) continue + try { + await api.createScratchlistEntry(sessionId, { + text, + entryId: entry.id, + createdAt: entry.createdAt + }) + } catch { + // Per-entry failure is non-fatal: the hub returns + // 200 with the canonical row for duplicates, and + // any genuine rejection (e.g. cap reached) just + // drops the migrated entry. Logging to console + // would be noise; the user can re-add manually. + } + } + writeMigrationFlag(sessionId) + await queryClient.invalidateQueries({ queryKey }) + setMigrationStatus('completed') + } catch { + // Whole-flow failure (network out, etc): leave the flag + // unset so a future mount retries; clear the banner + // status so we don't show "completed" for a half-done + // migration. + migrationAttemptedRef.current = false + setMigrationStatus('idle') + } + })() + }, [api, sessionId, query.data, query.isLoading, query.isFetching, queryClient, queryKey]) + + const dismissMigrationBanner = useCallback(() => { + writeBannerDismissed(sessionId) + setMigrationStatus('dismissed') + }, [sessionId]) + + const addMutation = useMutation< + { entry: HubEntry }, + Error, + { text: string }, + { previousData: ScratchlistResponse | undefined; optimisticEntryId: string } + >({ + mutationFn: async ({ text }) => { + if (!api || !sessionId) throw new Error('Scratchlist unavailable') + return await api.createScratchlistEntry(sessionId, { text }) + }, + onMutate: async ({ text }) => { + await queryClient.cancelQueries({ queryKey }) + const previousData = queryClient.getQueryData(queryKey) + const optimistic = makeOptimisticHubEntry(text, Date.now()) + queryClient.setQueryData(queryKey, (prev) => { + const prior = prev?.entries ?? [] + return { entries: [optimistic, ...prior] } + }) + return { previousData, optimisticEntryId: optimistic.entryId } + }, + onError: (_error, _variables, context) => { + if (context?.previousData !== undefined) { + queryClient.setQueryData(queryKey, context.previousData) + } + }, + onSuccess: (data, _variables, context) => { + // Replace the optimistic entry with the hub-canonical row so + // subsequent updates target the real entryId. If the cache + // already invalidated (SSE round-trip beat the response), + // the canonical row will arrive via refetch anyway. + queryClient.setQueryData(queryKey, (prev) => { + if (!prev) return { entries: [data.entry] } + const without = prev.entries.filter((e) => e.entryId !== context?.optimisticEntryId) + return { entries: [data.entry, ...without] } + }) + } + }) + + const updateMutation = useMutation< + { entry: HubEntry }, + Error, + { entryId: string; text: string }, + { previousData: ScratchlistResponse | undefined } + >({ + mutationFn: async ({ entryId, text }) => { + if (!api || !sessionId) throw new Error('Scratchlist unavailable') + return await api.updateScratchlistEntry(sessionId, entryId, text) + }, + onMutate: async ({ entryId, text }) => { + await queryClient.cancelQueries({ queryKey }) + const previousData = queryClient.getQueryData(queryKey) + const now = Date.now() + queryClient.setQueryData(queryKey, (prev) => { + if (!prev) return prev + return { + entries: prev.entries.map((e) => + e.entryId === entryId ? { ...e, text, updatedAt: now } : e + ) + } + }) + return { previousData } + }, + onError: (_error, _variables, context) => { + if (context?.previousData !== undefined) { + queryClient.setQueryData(queryKey, context.previousData) + } + } + }) + + const deleteMutation = useMutation< + void, + Error, + { entryId: string }, + { previousData: ScratchlistResponse | undefined } + >({ + mutationFn: async ({ entryId }) => { + if (!api || !sessionId) throw new Error('Scratchlist unavailable') + await api.deleteScratchlistEntry(sessionId, entryId) + }, + onMutate: async ({ entryId }) => { + await queryClient.cancelQueries({ queryKey }) + const previousData = queryClient.getQueryData(queryKey) + queryClient.setQueryData(queryKey, (prev) => { + if (!prev) return prev + return { entries: prev.entries.filter((e) => e.entryId !== entryId) } + }) + return { previousData } + }, + onError: (_error, _variables, context) => { + if (context?.previousData !== undefined) { + queryClient.setQueryData(queryKey, context.previousData) + } + } + }) + + const add = useCallback(async (rawText: string): Promise => { + const text = rawText.trim() + if (text.length === 0) return false + const truncated = text.length > SCRATCHLIST_MAX_TEXT_LENGTH + ? text.slice(0, SCRATCHLIST_MAX_TEXT_LENGTH) + : text + const current = queryClient.getQueryData(queryKey)?.entries ?? [] + if (current.length >= SCRATCHLIST_MAX_ENTRIES) { + return false + } + try { + await addMutation.mutateAsync({ text: truncated }) + return true + } catch { + return false + } + }, [addMutation, queryClient, queryKey]) + + const remove = useCallback(async (id: string) => { + try { + await deleteMutation.mutateAsync({ entryId: id }) + } catch { + // Rollback already happened in onError; surface to caller via + // the rejected promise would force the panel to add error UI + // we don't have copy for. Swallow here; SSE refetch on next + // hub state change will reconcile. + } + }, [deleteMutation]) + + const updateEntry = useCallback(async (id: string, rawText: string) => { + const text = rawText.trim() + if (text.length === 0) return + const truncated = text.length > SCRATCHLIST_MAX_TEXT_LENGTH + ? text.slice(0, SCRATCHLIST_MAX_TEXT_LENGTH) + : text + try { + await updateMutation.mutateAsync({ entryId: id, text: truncated }) + } catch { + // see `remove` rationale. + } + }, [updateMutation]) + + /** + * Local-only reorder. Mutates the cached array so the UI updates + * immediately; no hub call. The next invalidation refetch will reset + * the order to `createdAt DESC` - documented limitation per + * `tiann/hapi#893`. (v2.1 may add a `position` column.) + */ + const move = useCallback((id: string, direction: 'up' | 'down') => { + queryClient.setQueryData(queryKey, (prev) => { + if (!prev) return prev + const local = prev.entries.map(toLocalEntry) + const reordered = moveScratchlistEntry(local, id, direction) + // Rebuild the hub-shaped list using the reordered ids while + // preserving each entry's hub-stamped fields. Map by id for + // O(1) lookup. + const byId = new Map(prev.entries.map((e) => [e.entryId, e] as const)) + const next: HubEntry[] = [] + for (const r of reordered) { + const hub = byId.get(r.id) + if (hub) next.push(hub) + } + return { entries: next } + }) + }, [queryClient, queryKey]) + + // Mirror entries into localStorage as an offline cache. Keeps the v1 + // surface (e.g. the standalone `ScratchlistPanel` used by tests) + // working when offline, and protects against losing freshly-added + // entries if the hub goes away mid-session. + useEffect(() => { + if (!sessionId) return + const data = query.data + if (!data) return + try { + const cached = data.entries.map((e) => ({ + id: e.entryId, + text: e.text, + createdAt: e.createdAt + })) + window.localStorage.setItem( + `hapi.scratchlist.v1.${sessionId}`, + JSON.stringify(cached) + ) + } catch { + // Non-fatal: storage quota / private mode. + } + }, [sessionId, query.data]) + + const entries: ScratchlistEntry[] = (query.data?.entries ?? []).map(toLocalEntry) + + return { + entries, + isLoading: query.isLoading, + add, + remove, + update: updateEntry, + move, + migrationStatus, + dismissMigrationBanner + } +} diff --git a/web/src/lib/use-scratchlist-count.ts b/web/src/lib/use-scratchlist-count.ts new file mode 100644 index 0000000000..291db91ad8 --- /dev/null +++ b/web/src/lib/use-scratchlist-count.ts @@ -0,0 +1,30 @@ +import { useQuery } from '@tanstack/react-query' +import type { ApiClient } from '@/api/client' +import { queryKeys } from '@/lib/query-keys' + +/** + * tiann/hapi#893: read-only count of scratchlist entries for a session. + * + * Reuses the same TanStack Query cache key as `useHubScratchlist`, so + * the cost of calling it here in `SessionHeader` is zero when the same + * session is rendered in `SessionChat` - both components share one + * fetch. + * + * Used by the delete-session confirmation dialog to surface + * "this will also delete N scratchlist entries" copy. The signal is the + * count, not the entries themselves; we deliberately do not list them + * inline because the list could be long and would compete with the + * confirm action for attention. + */ +export function useScratchlistCount(sessionId: string, api: ApiClient | null): number { + const query = useQuery<{ entries: Array }>({ + queryKey: queryKeys.scratchlist(sessionId), + queryFn: async () => { + if (!api) return { entries: [] } + return await api.getScratchlist(sessionId) + }, + enabled: Boolean(api && sessionId), + staleTime: 30_000, + }) + return query.data?.entries.length ?? 0 +} From 3a86332c2ebd170abaaa977b285e9850f6e7214e Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Sat, 13 Jun 2026 18:18:50 +0100 Subject: [PATCH 02/11] fix(scratchlist): address HAPI Bot Major findings on PR #896 Two real data-correctness paths the bot caught on the initial review. 1. Migration partial-failure data loss The migration loop swallowed each failed POST and still wrote the `migrated` flag, while the offline-cache effect mirrored the (partial) hub state back into `hapi.scratchlist.v1.` - so a transient error or cap rejection could leave entries neither on the hub nor in localStorage. Fix: - Track failed entries during migration and persist them back to localStorage; do NOT advance the flag if any entry failed, so a future mount retries. - Gate the offline-cache effect on the migration flag. Pre- migration, localStorage holds the v1 entries the migration reads; mirroring an empty hub fetch over them was the wipe. - Drop the "skip migration when hub is non-empty" gate. Combined with the duplicate-idempotent POST short-circuit (below), a retry against a session that another device already populated is a safe union. 2. Duplicate POST returned 409 at cap The route checked `count >= SCRATCHLIST_MAX_ENTRIES` BEFORE asking the store whether the supplied `entryId` already existed, so an idempotent migration retry against a 200-row session returned 409 instead of 200. Fix: check duplicate first via a new `SyncEngine.getScratchlistEntry`, return the existing row with 200, and only run the cap check for genuinely new ids. Tests added: - hub/routes: at-cap + duplicate entryId returns 200 (not 409); at-cap + new entryId still 409. - web/hook: partial-failure persists the failed entries back to localStorage and leaves the flag unset; offline-cache effect does not wipe pre-migration localStorage. Co-authored-by: Cursor --- hub/src/sync/syncEngine.ts | 21 +++++ .../web/routes/sessions-scratchlist.test.ts | 61 ++++++++++++++ hub/src/web/routes/sessions.ts | 17 ++++ web/src/lib/use-hub-scratchlist.test.tsx | 80 +++++++++++++++++++ web/src/lib/use-hub-scratchlist.ts | 64 +++++++++++---- 5 files changed, 228 insertions(+), 15 deletions(-) diff --git a/hub/src/sync/syncEngine.ts b/hub/src/sync/syncEngine.ts index 7fd1a8d4e9..a85a484786 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -377,6 +377,27 @@ export class SyncEngine { return this.store.scratchlist.count(sessionId) } + /** + * Read a single entry by id. The route layer uses this to short- + * circuit duplicate POSTs (migration retry) BEFORE running the + * server-side cap check; otherwise an idempotent retry against a + * session that has hit `SCRATCHLIST_MAX_ENTRIES` would 409 when it + * should 200 with the existing row. + */ + getScratchlistEntry( + sessionId: string, + entryId: string + ): { entryId: string; text: string; createdAt: number; updatedAt: number } | null { + const row = this.store.scratchlist.get(sessionId, entryId) + if (!row) return null + return { + entryId: row.entryId, + text: row.text, + createdAt: row.createdAt, + updatedAt: row.updatedAt + } + } + /** * Insert a scratchlist entry. Returns the canonical row on success * (so the route layer can serialise it without a follow-up read). diff --git a/hub/src/web/routes/sessions-scratchlist.test.ts b/hub/src/web/routes/sessions-scratchlist.test.ts index 33e2b7717a..0ba3cbb57f 100644 --- a/hub/src/web/routes/sessions-scratchlist.test.ts +++ b/hub/src/web/routes/sessions-scratchlist.test.ts @@ -61,6 +61,7 @@ function createSession(overrides?: Partial): Session { type EngineOverrides = Partial<{ listScratchlistEntries: SyncEngine['listScratchlistEntries'] countScratchlistEntries: SyncEngine['countScratchlistEntries'] + getScratchlistEntry: SyncEngine['getScratchlistEntry'] createScratchlistEntry: SyncEngine['createScratchlistEntry'] updateScratchlistEntry: SyncEngine['updateScratchlistEntry'] deleteScratchlistEntry: SyncEngine['deleteScratchlistEntry'] @@ -81,6 +82,7 @@ function createApp(session: Session, overrides: EngineOverrides = {}) { }, listScratchlistEntries: overrides.listScratchlistEntries ?? (() => []), countScratchlistEntries: overrides.countScratchlistEntries ?? (() => 0), + getScratchlistEntry: overrides.getScratchlistEntry ?? (() => null), createScratchlistEntry: overrides.createScratchlistEntry ?? ((sessionId: string, text: string) => ({ outcome: 'created' as const, @@ -225,6 +227,65 @@ describe('POST /api/sessions/:id/scratchlist', () => { expect(body.code).toBe('scratchlist_at_cap') }) + it('still returns 200 for a duplicate entryId even when the session is at the cap (HAPI Bot, PR #896)', async () => { + // The cap check used to fire BEFORE the duplicate check, which + // turned an idempotent migration retry into a hard 409 the + // moment a session reached `SCRATCHLIST_MAX_ENTRIES`. The fix + // short-circuits on getScratchlistEntry first; this test pins + // that ordering. + const session = createSession() + const createCalls: number[] = [] + const app = createApp(session, { + countScratchlistEntries: () => 200, + getScratchlistEntry: (_sessionId, entryId) => { + if (entryId === 'pre-existing') { + return { + entryId: 'pre-existing', + text: 'already there', + createdAt: 100, + updatedAt: 100 + } + } + return null + }, + createScratchlistEntry: () => { + createCalls.push(1) + return { + outcome: 'created' as const, + entry: { entryId: 'should-not-fire', text: 'noop', createdAt: 0, updatedAt: 0 } + } + } + }) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'replay', entryId: 'pre-existing' }) + }) + expect(res.status).toBe(200) + const body = await res.json() as { entry: { text: string; entryId: string } } + expect(body.entry.entryId).toBe('pre-existing') + expect(body.entry.text).toBe('already there') + // The route must NOT have called createScratchlistEntry: the + // duplicate short-circuit returns BEFORE reaching the engine. + expect(createCalls).toHaveLength(0) + }) + + it('still returns 409 for a NEW entryId at the cap', async () => { + // Mirror of the test above for the not-duplicate case: a fresh + // POST at cap stays a 409. + const session = createSession() + const app = createApp(session, { + countScratchlistEntries: () => 200, + getScratchlistEntry: () => null + }) + const res = await app.request('/api/sessions/session-1/scratchlist', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'fresh', entryId: 'never-seen' }) + }) + expect(res.status).toBe(409) + }) + it('returns 404 when the engine reports session-not-found post-auth', async () => { // This path covers a race: auth said the session was visible // (resolveSessionAccess.ok), but by the time we INSERT the row the diff --git a/hub/src/web/routes/sessions.ts b/hub/src/web/routes/sessions.ts index d0133043bb..2e9c51f1fb 100644 --- a/hub/src/web/routes/sessions.ts +++ b/hub/src/web/routes/sessions.ts @@ -693,6 +693,23 @@ export function createSessionsRoutes(getSyncEngine: () => SyncEngine | null): Ho return c.json({ error: 'Invalid body', issues: parsed.error.issues }, 400) } + // Idempotent-retry short-circuit (HAPI Bot, PR #896 review): + // when the caller supplies an explicit entryId AND that id + // already exists, return the canonical row with 200 BEFORE the + // cap check fires. Otherwise a session sitting at the + // 200-entry cap would 409 a duplicate POST that should be a + // no-op - which is exactly the path the localStorage migration + // retry uses after a partial failure. + if (parsed.data.entryId) { + const existing = engine.getScratchlistEntry( + sessionResult.sessionId, + parsed.data.entryId + ) + if (existing) { + return c.json({ entry: existing }, 200) + } + } + // Server-side cap enforcement. Mirrors the web-side cap so a // malicious / runaway client can't drive the table without // bound. Bypassing the optimistic add path on the web client diff --git a/web/src/lib/use-hub-scratchlist.test.tsx b/web/src/lib/use-hub-scratchlist.test.tsx index 74883ac824..b69683ea42 100644 --- a/web/src/lib/use-hub-scratchlist.test.tsx +++ b/web/src/lib/use-hub-scratchlist.test.tsx @@ -330,6 +330,86 @@ describe('useHubScratchlist - localStorage migration', () => { expect(create).not.toHaveBeenCalled() expect(result.current.migrationStatus).toBe('idle') }) + + it('persists FAILED entries back to localStorage and leaves the flag unset (HAPI Bot, PR #896)', async () => { + // Migration partial failure: 2 entries in localStorage, the + // first POST succeeds and the second throws. Per the bot + // review, the failed entry must be written back to + // localStorage and the migration flag must NOT advance, so a + // future mount can retry. The status drops back to 'idle' + // (banner does not render). + const sid = makeSid() + seedV1Entries(sid) + let postCall = 0 + const create = vi.fn(async (_s: string, body: { text: string; entryId?: string; createdAt?: number }) => { + postCall += 1 + if (postCall === 1) { + return { + entry: { + entryId: body.entryId ?? 'a', + text: body.text, + createdAt: body.createdAt ?? 0, + updatedAt: 0 + } + } + } + throw new Error('HTTP 500: hub flaked on entry 2') + }) + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }), + createScratchlistEntry: create + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(create).toHaveBeenCalledTimes(2)) + await waitFor(() => expect(result.current.migrationStatus).toBe('idle')) + + // Flag must NOT be set: a future mount must retry. + expect(localStorage.getItem(`hapi.scratchlist.v2.migrated.${sid}`)).toBeNull() + // The failed entry (the second one) must be back in localStorage. + const persisted = localStorage.getItem(`hapi.scratchlist.v1.${sid}`) + expect(persisted).not.toBeNull() + const parsed = JSON.parse(persisted!) as Array<{ id: string; text: string }> + expect(parsed.map((e) => e.id)).toEqual(['old-2']) + expect(parsed[0]?.text).toBe('another') + }) + + it('does NOT mirror an empty hub fetch into localStorage before migration runs (HAPI Bot, PR #896)', async () => { + // Pre-fix the offline-cache effect would clobber the v1 + // entries with `[]` the moment the initial fetch returned an + // empty list, racing the migration effect's localStorage + // read on a future mount. The fix gates the cache mirror on + // the migration flag; this test pins it. + const sid = makeSid() + seedV1Entries(sid) + const apiCalls: number[] = [] + const api = createMockApi({ + // Block on first fetch so we can inspect localStorage + // BEFORE the migration effect kicks off. + getScratchlist: async () => { + apiCalls.push(Date.now()) + if (apiCalls.length === 1) { + await new Promise((r) => setTimeout(r, 25)) + return { entries: [] } + } + return { + entries: [ + { entryId: 'old-1', text: 'pre-v2 note', createdAt: 100, updatedAt: 100 }, + { entryId: 'old-2', text: 'another', createdAt: 200, updatedAt: 200 } + ] + } + } + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + // Wait for migration to complete (flag set + status flips). + await waitFor(() => expect(result.current.migrationStatus).toBe('completed'), { timeout: 2000 }) + // localStorage now mirrors hub state (post-migration). It must + // contain the v1 entries that round-tripped through the hub + // fetch, NOT an empty array. + const persisted = localStorage.getItem(`hapi.scratchlist.v1.${sid}`) + expect(persisted).not.toBeNull() + const parsed = JSON.parse(persisted!) as Array<{ id: string }> + expect(parsed.map((e) => e.id).sort()).toEqual(['old-1', 'old-2']) + }) }) describe('useHubScratchlist - reorder (local-only)', () => { diff --git a/web/src/lib/use-hub-scratchlist.ts b/web/src/lib/use-hub-scratchlist.ts index ca17b07296..a6466742ec 100644 --- a/web/src/lib/use-hub-scratchlist.ts +++ b/web/src/lib/use-hub-scratchlist.ts @@ -4,6 +4,7 @@ import type { ApiClient } from '@/api/client' import { queryKeys } from '@/lib/query-keys' import { moveScratchlistEntry, + persistScratchlist, readScratchlist, SCRATCHLIST_MAX_ENTRIES, SCRATCHLIST_MAX_TEXT_LENGTH, @@ -196,18 +197,21 @@ export function useHubScratchlist( // Migration trigger: runs ONCE per session when: // - api is available - // - hub returned an empty list // - migration flag is unset // - localStorage holds v1 entries - // The actual POSTs are sequential to keep retry semantics simple - // and to avoid bursts that could trip rate-limit guards. For the - // typical case of "a handful of stale entries" this is fine. + // Hub being non-empty does NOT block the migration: each POST uses + // the entry's original id and the route returns 200 for an + // already-existing id (idempotent). So a session that another + // device already populated is safely treated as a union with this + // device's local entries. The actual POSTs are sequential to keep + // retry semantics simple and to avoid bursts that could trip + // rate-limit guards. For the typical case of "a handful of stale + // entries" this is fine. useEffect(() => { if (!api || !sessionId) return if (migrationAttemptedRef.current) return if (query.isLoading || query.isFetching) return if (!query.data) return - if (query.data.entries.length > 0) return if (readMigrationFlag(sessionId)) return const localEntries = readScratchlist(sessionId) @@ -222,6 +226,14 @@ export function useHubScratchlist( setMigrationStatus('migrating') void (async () => { + // HAPI Bot review on PR #896 caught a data-loss path here: + // swallowing per-entry POST failures and still writing the + // migration flag would strand entries (the offline-cache + // mirror would replace the original localStorage with the + // partial hub state). Track failed entries and persist them + // back so a future mount retries; do NOT set the flag until + // every local entry is reconciled. + const failedEntries: ScratchlistEntry[] = [] try { // Preserve creation order by POSTing in the order // localStorage holds them. The hub orders by createdAt @@ -240,21 +252,34 @@ export function useHubScratchlist( createdAt: entry.createdAt }) } catch { - // Per-entry failure is non-fatal: the hub returns - // 200 with the canonical row for duplicates, and - // any genuine rejection (e.g. cap reached) just - // drops the migrated entry. Logging to console - // would be noise; the user can re-add manually. + // Genuine rejection (cap, network, 5xx...). The + // hub-side route returns 200 for duplicate + // entryId so an idempotent retry doesn't land + // here; only "really did not stick" failures do. + failedEntries.push(entry) } } + if (failedEntries.length > 0) { + // Write the unsynced subset back to localStorage so + // a future mount can retry them; leave the flag + // unset so the migration effect re-fires next time. + persistScratchlist(sessionId, failedEntries) + migrationAttemptedRef.current = false + setMigrationStatus('idle') + return + } writeMigrationFlag(sessionId) await queryClient.invalidateQueries({ queryKey }) setMigrationStatus('completed') } catch { - // Whole-flow failure (network out, etc): leave the flag - // unset so a future mount retries; clear the banner - // status so we don't show "completed" for a half-done - // migration. + // Whole-flow failure (network out, etc): persist the + // entries that hadn't been attempted yet plus any that + // failed up to the throw, leave the flag unset, and + // clear the banner status so we don't show "completed" + // for a half-done migration. + if (failedEntries.length > 0) { + persistScratchlist(sessionId, failedEntries) + } migrationAttemptedRef.current = false setMigrationStatus('idle') } @@ -431,8 +456,17 @@ export function useHubScratchlist( // surface (e.g. the standalone `ScratchlistPanel` used by tests) // working when offline, and protects against losing freshly-added // entries if the hub goes away mid-session. + // + // CRITICAL: gate on the migration flag. Pre-migration, localStorage + // holds the v1 entries that the migration effect needs to read; if + // we mirrored an empty hub fetch into localStorage on first render + // we'd wipe the very entries we're about to upload (HAPI Bot + // review on PR #896 caught a closely-related data-loss path). The + // flag also stays unset on partial-failure migrations, which keeps + // the failed-entry localStorage write from being clobbered. useEffect(() => { if (!sessionId) return + if (!readMigrationFlag(sessionId)) return const data = query.data if (!data) return try { @@ -448,7 +482,7 @@ export function useHubScratchlist( } catch { // Non-fatal: storage quota / private mode. } - }, [sessionId, query.data]) + }, [sessionId, query.data, migrationStatus]) const entries: ScratchlistEntry[] = (query.data?.entries ?? []).map(toLocalEntry) From 4ae02c21f70f770112f192135b8bd90fe7dfa888 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Sat, 13 Jun 2026 18:29:54 +0100 Subject: [PATCH 03/11] feat(web/scratchlist): per-entry age indicator (clock icon + tooltip) Surfaces the smart-relative time the entry was last saved on every scratchlist row, mirroring the bucketing used in the session list: just-now -> Nm -> Nh -> Nd -> absolute date. Implementation: - Extract the existing `formatRelativeTime` helper out of SessionList into `web/src/lib/relative-time.ts` so the panel can reuse the same buckets and i18n keys (no copy-paste drift between surfaces). Also add `formatAbsoluteDateTime` for the precise-stamp tooltip line. - Add `updatedAt?: number` to the local `ScratchlistEntry` shape. v1-only callers stay valid (the field is optional and `isEntry` now accepts rows that omit it). The hub hook forwards the hub's `updatedAt` so the indicator reflects edits, not just creation. - New `EntryAgeIndicator` component: clock SVG in the same style as the existing action icons, rendered inside both panel surfaces (the older `ScratchlistList` and the drawer variant). Falls back to `createdAt` when `updatedAt` is missing (legacy v1 rows during the migration window) and renders nothing if neither timestamp is usable. - Tooltip carries the relative bucket plus the absolute timestamp on a second line; aria-label carries the relative bucket only so screen readers stay terse. - Mirror `updatedAt` into the localStorage offline cache so an offline reload still has accurate ages. Tests: - `relative-time.test.ts`: bucket math, seconds-vs-ms detection, non-finite guard. - `ScratchlistPanel.test.tsx`: indicator renders with the right smart-relative bucket, falls back to `createdAt` when `updatedAt` is absent, and renders nothing when both timestamps are zero. Co-authored-by: Cursor --- .../AssistantChat/ScratchlistPanel.test.tsx | 68 ++++++++++++++++ .../AssistantChat/ScratchlistPanel.tsx | 54 +++++++++++++ web/src/components/SessionList.tsx | 1 - web/src/lib/locales/en.ts | 2 + web/src/lib/locales/zh-CN.ts | 2 + web/src/lib/relativeTime.test.ts | 80 +++++++++++++++++++ web/src/lib/relativeTime.ts | 10 +++ web/src/lib/scratchlist.ts | 27 +++++-- web/src/lib/use-hub-scratchlist.ts | 13 +-- 9 files changed, 244 insertions(+), 13 deletions(-) create mode 100644 web/src/lib/relativeTime.test.ts diff --git a/web/src/components/AssistantChat/ScratchlistPanel.test.tsx b/web/src/components/AssistantChat/ScratchlistPanel.test.tsx index 79cd55c6bb..47768e7c03 100644 --- a/web/src/components/AssistantChat/ScratchlistPanel.test.tsx +++ b/web/src/components/AssistantChat/ScratchlistPanel.test.tsx @@ -306,4 +306,72 @@ describe('ScratchlistPanel', () => { expect(b.getByText('B note')).toBeTruthy() expect(b.queryByText('A note')).toBeNull() }) + + it('renders the per-entry age indicator with a tooltip showing the smart-relative time', () => { + // 5 minutes ago, deterministic via fake timers below. + vi.useFakeTimers() + const now = new Date('2026-06-13T17:00:00Z').getTime() + vi.setSystemTime(new Date(now)) + try { + persistScratchlist(SID, [ + makeEntry({ + id: 'aged', + text: 'aged note', + createdAt: now - 10 * 60_000, + updatedAt: now - 5 * 60_000, + }), + ]) + renderPanel() + expandPanel() + const indicator = screen.getByTestId('scratchlist-entry-age') + // Tooltip carries the smart-relative time + an absolute + // timestamp; aria-label carries the relative time only. + const title = indicator.getAttribute('title') ?? '' + expect(title).toContain('5m ago') + expect(title).toContain('Saved') + const aria = indicator.getAttribute('aria-label') ?? '' + expect(aria).toContain('5m ago') + // data-entry-age mirrors the relative bucket so a future + // assertion can target it without scraping the title. + expect(indicator.getAttribute('data-entry-age')).toBe('5m ago') + } finally { + vi.useRealTimers() + } + }) + + it('falls back to createdAt when updatedAt is absent (legacy v1 row)', () => { + vi.useFakeTimers() + const now = new Date('2026-06-13T17:00:00Z').getTime() + vi.setSystemTime(new Date(now)) + try { + persistScratchlist(SID, [ + makeEntry({ + id: 'legacy', + text: 'legacy v1 note', + createdAt: now - 2 * 60 * 60_000, + // updatedAt deliberately omitted - simulates a + // localStorage row written by v1 before the v2 hub + // sync work added the column. + }), + ]) + renderPanel() + expandPanel() + const indicator = screen.getByTestId('scratchlist-entry-age') + expect(indicator.getAttribute('data-entry-age')).toBe('2h ago') + } finally { + vi.useRealTimers() + } + }) + + it('renders no age indicator when both timestamps are unusable', () => { + // The schema validator (`isEntry`) rejects rows with a + // non-finite `createdAt`, so the only realistic path to a + // missing-stamp entry is rendering an in-memory entry directly. + // We simulate that by writing a row with a sentinel `0` + // createdAt - the indicator returns null per its guard. + persistScratchlist(SID, [makeEntry({ id: 'no-stamp', text: 'note', createdAt: 0 })]) + renderPanel() + expandPanel() + expect(screen.queryByTestId('scratchlist-entry-age')).toBeNull() + }) }) diff --git a/web/src/components/AssistantChat/ScratchlistPanel.tsx b/web/src/components/AssistantChat/ScratchlistPanel.tsx index f684b69229..c9534f9a12 100644 --- a/web/src/components/AssistantChat/ScratchlistPanel.tsx +++ b/web/src/components/AssistantChat/ScratchlistPanel.tsx @@ -20,6 +20,7 @@ import { } from '@/lib/scratchlist' import { safeCopyToClipboard } from '@/lib/clipboard' import { useTranslation } from '@/lib/use-translation' +import { formatAbsoluteDateTime, formatRelativeTime } from '@/lib/relativeTime' const STORAGE_KEY_PREFIX = 'hapi.scratchlist-collapsed.v1.' @@ -150,6 +151,57 @@ function CopyIcon() { ) } +function ClockIcon() { + return ( + + ) +} + +/** + * Per-entry age indicator: clock icon with a tooltip showing + * smart-relative time (e.g. "2m ago") and the absolute timestamp on a + * second line, so an operator can tell at-a-glance how stale a note is. + * + * Renders nothing when no usable timestamp is available - this happens + * for legacy localStorage entries that pre-date the v2 hub-sync work + * (no `updatedAt` recorded) AND have no `createdAt` either, which is + * vanishingly rare but still a guard against `NaN` titles. + * + * Falls back to `createdAt` when `updatedAt` is missing so newly-loaded + * v1-only rows still get a useful tooltip during the migration window. + */ +function EntryAgeIndicator({ + entry, +}: { + entry: ScratchlistEntry +}) { + const { t } = useTranslation() + const stamp = entry.updatedAt ?? entry.createdAt + if (!Number.isFinite(stamp) || stamp <= 0) return null + const relative = formatRelativeTime(stamp, t) + if (!relative) return null + const absolute = formatAbsoluteDateTime(stamp) + const ariaLabel = t('scratchlist.entry.lastSavedAriaLabel', { time: relative }) + const title = absolute + ? `${t('scratchlist.entry.lastSaved', { time: relative })}\n${absolute}` + : t('scratchlist.entry.lastSaved', { time: relative }) + return ( + + + + ) +} + function ClipboardCheckIcon() { return (
+