diff --git a/hub/src/store/index.ts b/hub/src/store/index.ts index c4c0e2f6..b5c594d7 100644 --- a/hub/src/store/index.ts +++ b/hub/src/store/index.ts @@ -6,6 +6,7 @@ import { MachineStore } from './machineStore' import { MessageStore } from './messageStore' import { PushStore } from './pushStore' import { FcmStore } from './fcmStore' +import { ScratchlistStore } from './scratchlistStore' import { SessionStore } from './sessionStore' import { UserStore } from './userStore' @@ -14,6 +15,7 @@ export type { StoredMessage, StoredPushSubscription, StoredFcmDevice, + StoredScratchlistEntry, StoredSession, StoredUser, VersionedUpdateResult @@ -23,17 +25,19 @@ export { MachineStore } from './machineStore' export { MessageStore } from './messageStore' export { PushStore } from './pushStore' export { FcmStore } from './fcmStore' +export { ScratchlistStore } from './scratchlistStore' export { SessionStore } from './sessionStore' export { UserStore } from './userStore' -const SCHEMA_VERSION: number = 11 +const SCHEMA_VERSION: number = 12 const REQUIRED_TABLES = [ 'sessions', 'machines', 'messages', 'users', 'push_subscriptions', - 'fcm_devices' + 'fcm_devices', + 'session_scratchlist' ] as const export class Store { @@ -47,6 +51,7 @@ export class Store { readonly users: UserStore readonly push: PushStore readonly fcm: FcmStore + readonly scratchlist: ScratchlistStore /** * Filesystem path of the underlying SQLite database, or ':memory:' for @@ -98,6 +103,7 @@ export class Store { this.users = new UserStore(this.db) this.push = new PushStore(this.db) this.fcm = new FcmStore(this.db) + this.scratchlist = new ScratchlistStore(this.db) } close(): void { @@ -131,6 +137,7 @@ export class Store { 8: () => this.migrateFromV8ToV9(), 9: () => this.migrateFromV9ToV10(), 10: () => this.migrateFromV10ToV11(), + 11: () => this.migrateFromV11ToV12(), }) if (currentVersion === 0) { @@ -272,6 +279,18 @@ export class Store { ); CREATE INDEX IF NOT EXISTS idx_fcm_devices_namespace ON fcm_devices(namespace); CREATE INDEX IF NOT EXISTS idx_fcm_devices_token ON fcm_devices(token); + + 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); `) } @@ -472,6 +491,34 @@ export class Store { `) } + /** + * tiann/hapi#893 (scratchlist v2): introduce the per-session + * `session_scratchlist` typed table. Upstream main took V10→V11 for + * `fcm_devices`; scratchlist is V11→V12. + * + * Idempotent via `CREATE TABLE IF NOT EXISTS` + `CREATE INDEX IF NOT + * EXISTS`. Cascade-delete from `sessions(id)` handles delete-session + * cleanup. No data backfill: the web client's first-run migration + * pushes any existing `localStorage` entries up via REST. + * + * Rollback: `DROP TABLE session_scratchlist; PRAGMA user_version = 11;` + */ + private migrateFromV11ToV12(): 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-v12.test.ts b/hub/src/store/migration-v12.test.ts new file mode 100644 index 00000000..13986cb3 --- /dev/null +++ b/hub/src/store/migration-v12.test.ts @@ -0,0 +1,337 @@ +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 V11→V12 schema migration: introduces the `session_scratchlist` + * typed table for tiann/hapi#893 (scratchlist v2 hub sync). + * + * Upstream main: V9→V10 = service_tier, V10→V11 = fcm_devices. + * Scratchlist v2 takes V11→V12 for the new table. + */ +describe('Store V11→V12 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('V11 DB migrates to V12 via Store: session_scratchlist created', () => { + const dir = mkdtempSync(join(tmpdir(), 'hapi-migration-v12-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') + createV11Schema(db) + db.exec('PRAGMA user_version = 11') + 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') + + 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('V9 DB migrates to V12 (multi-hop service_tier + fcm_devices + scratchlist)', () => { + const dir = mkdtempSync(join(tmpdir(), 'hapi-migration-v9-to-v12-')) + 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.close() + + store = new Store(dbPath) + const sessionCols = getColumns(store, 'sessions') + expect(sessionCols).toContain('service_tier') + const scratchCols = getColumns(store, 'session_scratchlist') + expect(scratchCols).toContain('entry_id') + } finally { + store?.close() + rmSync(dir, { recursive: true, force: true }) + } + }) + + it('V12 DB reopen is idempotent: schema unchanged', () => { + const dir = mkdtempSync(join(tmpdir(), 'hapi-migration-v12-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) + 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) +} + +/** Pre-V10 shape (no service_tier, no session_scratchlist). */ +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_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 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) + ); + `) +} + +/** Post-V9→V10 shape (service_tier present). */ +function createV10Schema(db: Database): void { + createV9Schema(db) + db.exec('ALTER TABLE sessions ADD COLUMN service_tier TEXT') +} + +/** Post-V10→V11 shape (fcm_devices present, no session_scratchlist yet). */ +function createV11Schema(db: Database): void { + createV10Schema(db) + db.exec(` + CREATE TABLE IF NOT EXISTS fcm_devices ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + namespace TEXT NOT NULL, + token TEXT NOT NULL, + platform TEXT NOT NULL, + device_id TEXT NOT NULL, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + UNIQUE(namespace, device_id, platform) + ); + CREATE INDEX IF NOT EXISTS idx_fcm_devices_namespace ON fcm_devices(namespace); + CREATE INDEX IF NOT EXISTS idx_fcm_devices_token ON fcm_devices(token); + `) +} + diff --git a/hub/src/store/scratchlist.ts b/hub/src/store/scratchlist.ts new file mode 100644 index 00000000..8d286f35 --- /dev/null +++ b/hub/src/store/scratchlist.ts @@ -0,0 +1,240 @@ +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 +} + +/** + * Re-point all scratchlist rows from `fromSessionId` to `toSessionId`. + * + * Required by tiann/hapi#920: `mergeSessionData` in `sessionCache.ts` + * deletes the old session row at the end of every merge codepath, and + * `session_scratchlist.session_id` is FK'd with `ON DELETE CASCADE`. + * Without an explicit transfer step, scratchlist entries are silently + * destroyed every time the hub dedups two sessions or rotates the HAPI + * id during resume - both of which fire on the upstream + * hub-restart-cascade path documented in tiann/hapi#915. + * + * Strategy: + * 1. `UPDATE OR IGNORE` the session_id column. Rows that would + * collide with an existing (toSessionId, entry_id) PK simply do + * not move - the dedup target's copy wins, which matches the + * operator's mental model that the consolidated session is the + * authoritative one. + * 2. `DELETE` whatever didn't move. This is the collision-loser + * cleanup; the cascade from `deleteSession` would do the same + * thing, but doing it here makes the no-delete codepath + * (`mergeSessionHistory`) symmetric and avoids leaving stale + * duplicates on the old row when it stays alive. + * + * Wrapped in BEGIN/COMMIT so the move is atomic w.r.t. concurrent + * writers. Returns counts so the caller can decide whether to fire + * SSE patches. + */ +export function transferScratchlistEntries( + db: Database, + fromSessionId: string, + toSessionId: string +): { moved: number; collided: number } { + if (fromSessionId === toSessionId) { + return { moved: 0, collided: 0 } + } + + try { + db.exec('BEGIN') + const before = db.prepare( + 'SELECT COUNT(*) AS n FROM session_scratchlist WHERE session_id = ?' + ).get(fromSessionId) as { n: number } | undefined + const total = before?.n ?? 0 + const moved = db.prepare( + 'UPDATE OR IGNORE session_scratchlist SET session_id = ? WHERE session_id = ?' + ).run(toSessionId, fromSessionId).changes + const collided = total - moved + if (collided > 0) { + db.prepare( + 'DELETE FROM session_scratchlist WHERE session_id = ?' + ).run(fromSessionId) + } + db.exec('COMMIT') + return { moved, collided } + } catch (error) { + db.exec('ROLLBACK') + throw error + } +} diff --git a/hub/src/store/scratchlistStore.ts b/hub/src/store/scratchlistStore.ts new file mode 100644 index 00000000..c75bc6cd --- /dev/null +++ b/hub/src/store/scratchlistStore.ts @@ -0,0 +1,64 @@ +import type { Database } from 'bun:sqlite' + +import type { StoredScratchlistEntry } from './types' +import { + countScratchlistEntries, + createScratchlistEntry, + deleteScratchlistEntry, + getScratchlistEntry, + listScratchlistEntries, + transferScratchlistEntries, + 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) + } + + /** + * Re-point rows during a session merge. See + * `transferScratchlistEntries` for the contract; the wrapper just + * forwards through. Must be called BEFORE `deleteSession` so + * `ON DELETE CASCADE` on `session_scratchlist.session_id` doesn't + * race the migration. Required by tiann/hapi#920. + */ + transfer(fromSessionId: string, toSessionId: string): { moved: number; collided: number } { + return transferScratchlistEntries(this.db, fromSessionId, toSessionId) + } +} diff --git a/hub/src/store/types.ts b/hub/src/store/types.ts index 6917c19a..28683c99 100644 --- a/hub/src/store/types.ts +++ b/hub/src/store/types.ts @@ -74,6 +74,14 @@ export type StoredFcmDevice = { updatedAt: 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-merge-scratchlist.test.ts b/hub/src/sync/sessionCache-merge-scratchlist.test.ts new file mode 100644 index 00000000..d528aae0 --- /dev/null +++ b/hub/src/sync/sessionCache-merge-scratchlist.test.ts @@ -0,0 +1,220 @@ +import { describe, expect, it } from 'bun:test' +import type { SyncEvent } from '@hapi/protocol/types' +import { Store } from '../store' +import type { EventPublisher } from './eventPublisher' +import { SessionCache } from './sessionCache' + +/** + * Regression tests for tiann/hapi#920: scratchlist rows must survive + * the `mergeSessionData` codepath in `SessionCache`. + * + * Background: `mergeSessionData` ends with `deleteSession(oldSessionId)` + * which fires `ON DELETE CASCADE` on every FK-tied table. The + * `session_scratchlist` table joins on `sessions(id)` with cascade + * delete, so without an explicit transfer step every dedup + * (#448 agent-id collision) and every resume-of-inactive path + * (`syncEngine.resumeSession`) silently destroys the operator's notes. + * + * Two codepaths: + * - `mergeSessions(old, new, ns)` -> `mergeSessionData(deleteOld=true)` + * - `mergeSessionHistory(old, new, ns, opts)` -> `mergeSessionData(deleteOld=false)` + * + * Both must transfer scratchlist rows. We pin both with their own + * happy-path test plus a PK-collision test (same `entryId` on both + * sides; the dedup target wins). + */ + +function createCapturingPublisher(events: SyncEvent[]): EventPublisher { + return { + emit: (event: SyncEvent) => { + events.push(event) + } + } as unknown as EventPublisher +} + +function setup() { + const store = new Store(':memory:') + const events: SyncEvent[] = [] + const cache = new SessionCache(store, createCapturingPublisher(events)) + return { store, events, cache } +} + +function makeSessions(cache: SessionCache, ns: string = 'default') { + const oldSession = cache.getOrCreateSession( + 'agent-merge-old-' + Math.random().toString(36).slice(2, 8), + { path: '/tmp/project', host: 'localhost', flavor: 'codex' }, + null, + ns + ) + const newSession = cache.getOrCreateSession( + 'agent-merge-new-' + Math.random().toString(36).slice(2, 8), + { path: '/tmp/project', host: 'localhost', flavor: 'codex' }, + null, + ns + ) + return { oldSession, newSession } +} + +describe('mergeSessions (deleteOldSession=true) - scratchlist transfer', () => { + it('moves scratchlist rows from old to new before the cascade-delete fires', async () => { + const { store, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + + store.scratchlist.create(oldSession.id, 'note one', { entryId: 'e-1', createdAt: 100 }) + store.scratchlist.create(oldSession.id, 'note two', { entryId: 'e-2', createdAt: 200 }) + + await cache.mergeSessions(oldSession.id, newSession.id, 'default') + + // New session now owns the rows. + const onNew = store.scratchlist.list(newSession.id).map((e) => e.entryId).sort() + expect(onNew).toEqual(['e-1', 'e-2']) + + // Old session is gone (deleteOldSession=true) AND its rows + // are not stranded on a phantom session id. + expect(store.scratchlist.list(oldSession.id)).toEqual([]) + }) + + it('handles entryId PK collision by keeping the dedup target row (operator-visible session wins)', async () => { + const { store, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + + // Both sessions have an entry at the same id - the new wins. + store.scratchlist.create(oldSession.id, 'OLD copy', { entryId: 'shared-id', createdAt: 100 }) + store.scratchlist.create(newSession.id, 'NEW copy', { entryId: 'shared-id', createdAt: 200 }) + store.scratchlist.create(oldSession.id, 'unique to old', { entryId: 'old-only', createdAt: 50 }) + + await cache.mergeSessions(oldSession.id, newSession.id, 'default') + + const final = store.scratchlist.list(newSession.id) + const byId = new Map(final.map((e) => [e.entryId, e.text])) + expect(byId.get('shared-id')).toBe('NEW copy') + expect(byId.get('old-only')).toBe('unique to old') + expect(final).toHaveLength(2) + }) + + it('emits scratchlistUpdatedAt on the new session (and not on the old one - it is about to be removed)', async () => { + const { store, events, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + store.scratchlist.create(oldSession.id, 'note', { entryId: 'e-1', createdAt: 100 }) + events.length = 0 + + await cache.mergeSessions(oldSession.id, newSession.id, 'default') + + const scratchPatches = events.filter((e) => { + return e.type === 'session-updated' + && typeof e.data === 'object' && e.data !== null + && 'scratchlistUpdatedAt' in (e.data as Record) + }) + // Exactly one - on the new session id. + expect(scratchPatches).toHaveLength(1) + expect(scratchPatches[0]!.type === 'session-updated' && scratchPatches[0]!.sessionId).toBe(newSession.id) + }) + + it('is a no-op (no extra emit) when the old session has no scratchlist rows', async () => { + const { events, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + events.length = 0 + + await cache.mergeSessions(oldSession.id, newSession.id, 'default') + + const scratchPatches = events.filter((e) => { + return e.type === 'session-updated' + && typeof e.data === 'object' && e.data !== null + && 'scratchlistUpdatedAt' in (e.data as Record) + }) + expect(scratchPatches).toHaveLength(0) + }) +}) + +describe('mergeSessionHistory (deleteOldSession=false) - scratchlist transfer', () => { + it('moves scratchlist rows even when the old session row stays alive', async () => { + const { store, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + + store.scratchlist.create(oldSession.id, 'still-need-this', { entryId: 'e-1', createdAt: 100 }) + + // Active-duplicate codepath: keeps the live socket but moves + // the persisted history into the dedup target. Scratchlist + // is "persisted history" for this purpose. + await cache.mergeSessionHistory(oldSession.id, newSession.id, 'default', { mergeAgentState: false }) + + expect(store.scratchlist.list(newSession.id).map((e) => e.entryId)).toEqual(['e-1']) + // Old row is still alive but its scratchlist is empty - the + // operator-facing dedup target is now the source of truth. + expect(store.scratchlist.list(oldSession.id)).toEqual([]) + }) + + it('emits scratchlistUpdatedAt on BOTH the new and the still-alive old session id', async () => { + const { store, events, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + store.scratchlist.create(oldSession.id, 'note', { entryId: 'e-1', createdAt: 100 }) + events.length = 0 + + await cache.mergeSessionHistory(oldSession.id, newSession.id, 'default', { mergeAgentState: false }) + + const scratchPatches = events.filter((e) => { + return e.type === 'session-updated' + && typeof e.data === 'object' && e.data !== null + && 'scratchlistUpdatedAt' in (e.data as Record) + }) + // Two emits: one per session id, so any client looking at + // either side invalidates and refetches. + const ids = scratchPatches + .map((e) => e.type === 'session-updated' ? e.sessionId : '') + .sort() + expect(ids).toEqual([oldSession.id, newSession.id].sort()) + }) + + it('emits scratchlistUpdatedAt on the still-alive old session when every entry collides (moved=0)', async () => { + // HAPI Bot PR #896: all-collision transfer deletes old rows + // without moving any; the old session stays visible in the + // mergeSessionHistory path and must still get an invalidation. + const { store, events, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + store.scratchlist.create(oldSession.id, 'OLD copy', { entryId: 'shared-only', createdAt: 100 }) + store.scratchlist.create(newSession.id, 'NEW copy', { entryId: 'shared-only', createdAt: 200 }) + events.length = 0 + + await cache.mergeSessionHistory(oldSession.id, newSession.id, 'default', { mergeAgentState: false }) + + expect(store.scratchlist.list(oldSession.id)).toEqual([]) + expect(store.scratchlist.list(newSession.id)).toHaveLength(1) + + const scratchPatches = events.filter((e) => { + return e.type === 'session-updated' + && typeof e.data === 'object' && e.data !== null + && 'scratchlistUpdatedAt' in (e.data as Record) + }) + const ids = scratchPatches + .map((e) => e.type === 'session-updated' ? e.sessionId : '') + expect(ids).toContain(oldSession.id) + expect(ids).not.toContain(newSession.id) + }) +}) + +describe('cascade-delete safety (regression)', () => { + it('without the transfer, the cascade would have nuked them - confirm by deleting the new session at the end', async () => { + // This is a smoke test for the ON DELETE CASCADE on + // `session_scratchlist.session_id` itself: after the merge + // moves rows to the new session and the new session is + // later deleted (e.g. operator clicks Delete), the rows + // disappear too. This is the cascade we DO want; the bug + // is that the merge codepath was triggering it on the OLD + // id while the operator expected the data to follow the + // new id. + const { store, cache } = setup() + const { oldSession, newSession } = makeSessions(cache) + store.scratchlist.create(oldSession.id, 'note', { entryId: 'e-1', createdAt: 100 }) + + await cache.mergeSessions(oldSession.id, newSession.id, 'default') + expect(store.scratchlist.list(newSession.id)).toHaveLength(1) + + // Now an explicit operator-driven delete of the new session. + // Mark it inactive first because deleteSession refuses to + // delete an active session. + const cached = cache.getSession(newSession.id) + if (cached) cached.active = false + await cache.deleteSession(newSession.id) + expect(store.scratchlist.list(newSession.id)).toEqual([]) + }) +}) diff --git a/hub/src/sync/sessionCache.ts b/hub/src/sync/sessionCache.ts index 0fd6de7f..7b189391 100644 --- a/hub/src/sync/sessionCache.ts +++ b/hub/src/sync/sessionCache.ts @@ -416,6 +416,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() @@ -879,6 +908,28 @@ export class SessionCache { this.publisher.emit({ type: 'messages-invalidated', sessionId: newSessionId, namespace }) } + // tiann/hapi#920: transfer scratchlist rows BEFORE the + // deleteSession() call below fires `ON DELETE CASCADE` on + // `session_scratchlist.session_id`. Without this step every + // dedup (#448 agent-id collision) and every resume-of-inactive + // path (`syncEngine.resumeSession` -> here) silently destroys + // the operator's per-session notes, contradicting the v2.0 + // promise that scratchlist survives reloads. + const movedScratchlist = this.store.scratchlist.transfer(oldSessionId, newSessionId) + if (movedScratchlist.moved > 0) { + // Rows landed on the consolidated session - invalidate so + // any client on the new id refetches. + this.emitScratchlistChanged(newSessionId) + } + if (!options.deleteOldSession && (movedScratchlist.moved > 0 || movedScratchlist.collided > 0)) { + // HAPI Bot PR #896: when every old entry collides (moved=0, + // collided>0) the transfer still deletes rows from the + // still-alive old session. Emit even when moved=0 so web + // clients viewing the old id drop stale cache entries that + // would 404 on edit/delete. + this.emitScratchlistChanged(oldSessionId) + } + const mergedMetadata = this.mergeSessionMetadata(oldStored.metadata, newStored.metadata) if (mergedMetadata !== null && mergedMetadata !== newStored.metadata) { for (let attempt = 0; attempt < 2; attempt += 1) { diff --git a/hub/src/sync/syncEngine-scratchlist.test.ts b/hub/src/sync/syncEngine-scratchlist.test.ts new file mode 100644 index 00000000..6559cc34 --- /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 6c312c3d..ec0ec31c 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -434,6 +434,113 @@ 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) + } + + /** + * 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). + * 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; health?: unknown }): 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 00000000..a9b5b972 --- /dev/null +++ b/hub/src/web/routes/sessions-scratchlist.test.ts @@ -0,0 +1,424 @@ +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, + serviceTier: null, + permissionMode: 'default', + collaborationMode: 'default' + } + return { ...base, ...overrides } +} + +type EngineOverrides = Partial<{ + listScratchlistEntries: SyncEngine['listScratchlistEntries'] + countScratchlistEntries: SyncEngine['countScratchlistEntries'] + getScratchlistEntry: SyncEngine['getScratchlistEntry'] + 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), + getScratchlistEntry: overrides.getScratchlistEntry ?? (() => null), + 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('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('rejects oversized entryId with 400 (HAPI Bot, PR #896 follow-up)', async () => { + // Server-side guard for the SQLite primary key: an authenticated + // client could otherwise grow the table and its index with + // arbitrarily large keys. 129 chars is one over the 128-char + // cap defined in SCRATCHLIST_MAX_ENTRY_ID_LENGTH. + 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: 'oversized id', entryId: 'x'.repeat(129) }) + }) + expect(res.status).toBe(400) + }) + + 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 19776c5f..dfd2ef62 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, @@ -695,6 +698,148 @@ 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) + } + + // 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 + // (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 441338cf..9a122c86 100644 --- a/shared/src/apiTypes.ts +++ b/shared/src/apiTypes.ts @@ -224,6 +224,58 @@ 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 +/** + * Hard cap on client-supplied entry id length. The id is persisted as + * part of the SQLite primary key, so without a bound an authenticated + * client could grow the table and its index with arbitrarily large + * keys. 128 chars comfortably fits a UUID (36) plus any prefix scheme + * we might layer on later. + */ +export const SCRATCHLIST_MAX_ENTRY_ID_LENGTH = 128 + +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).max(SCRATCHLIST_MAX_ENTRY_ID_LENGTH).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 38babfc2..7d8f104e 100644 --- a/shared/src/schemas.ts +++ b/shared/src/schemas.ts @@ -247,11 +247,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 521b53aa..085059c8 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -738,6 +738,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 00000000..0ea81be4 --- /dev/null +++ b/web/src/components/AssistantChat/ScratchlistMigrationBanner.test.tsx @@ -0,0 +1,61 @@ +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' + 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() + }) + + // 'pre-migrated' was removed by the HAPI Bot PR #896 follow-up: + // a session whose migration ran in a prior mount but was not yet + // dismissed should now show the banner, not hide it. The + // dismissal flag is the only thing that suppresses the banner - + // see ScratchlistMigrationBanner doc-comment. + + 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 00000000..25b11b9c --- /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 after a v2- + * aware client migrates a session's localStorage entries to the hub. + * + * Visibility contract: + * - Renders whenever `migrationStatus === 'completed'`, which is + * sticky across reloads until the operator clicks dismiss (HAPI + * Bot, PR #896 follow-up - the previous behavior swallowed the + * banner if the user reloaded before clicking). + * - 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' + 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/AssistantChat/ScratchlistPanel.test.tsx b/web/src/components/AssistantChat/ScratchlistPanel.test.tsx index 79cd55c6..47768e7c 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 f684b692..c9534f9a 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 (
+
) : 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 5882a3b5..453e958d 100644 --- a/web/src/components/SessionHeader.tsx +++ b/web/src/components/SessionHeader.tsx @@ -8,6 +8,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 { formatCodexReasoningLabel, shouldShowCodexReasoningLabel } from '@/lib/codexStatusLabels' import { getSessionModelLabel } from '@/lib/sessionModelLabel' @@ -135,6 +136,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() @@ -366,7 +372,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 6d6a419f..d27455c1 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() @@ -510,6 +527,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 87084233..be455cf2 100644 --- a/web/src/lib/locales/en.ts +++ b/web/src/lib/locales/en.ts @@ -212,6 +212,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.', @@ -549,6 +551,11 @@ export default { 'scratchlist.action.copy': 'Copy to clipboard', 'scratchlist.action.copied': 'Copied!', 'scratchlist.action.delete': 'Delete entry', + 'scratchlist.entry.lastSaved': 'Saved {time}', + 'scratchlist.entry.lastSavedAriaLabel': 'Entry saved {time}', + '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 228f03c8..00527432 100644 --- a/web/src/lib/locales/zh-CN.ts +++ b/web/src/lib/locales/zh-CN.ts @@ -215,6 +215,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': '删除中…', @@ -553,6 +555,11 @@ export default { 'scratchlist.action.copy': '复制到剪贴板', 'scratchlist.action.copied': '已复制！', 'scratchlist.action.delete': '删除条目', + 'scratchlist.entry.lastSaved': '保存于 {time}', + 'scratchlist.entry.lastSavedAriaLabel': '条目保存于 {time}', + '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 cb5cdc18..4fe1643e 100644 --- a/web/src/lib/query-keys.ts +++ b/web/src/lib/query-keys.ts @@ -26,4 +26,5 @@ export const queryKeys = { sessionGrokModels: (sessionId: string) => ['session-grok-models', sessionId] as const, sessionGrokReasoningEffortOptions: (sessionId: string) => ['session-grok-reasoning-effort-options', sessionId] as const, skills: (sessionId: string) => ['skills', sessionId] as const, + scratchlist: (sessionId: string) => ['scratchlist', sessionId] as const, } diff --git a/web/src/lib/relativeTime.test.ts b/web/src/lib/relativeTime.test.ts index 24f49e4e..aed50535 100644 --- a/web/src/lib/relativeTime.test.ts +++ b/web/src/lib/relativeTime.test.ts @@ -1,7 +1,16 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' -import { formatRelativeTime, formatSessionListDate } from '@/lib/relativeTime' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { formatAbsoluteDateTime, formatRelativeTime, formatSessionListDate } from '@/lib/relativeTime' -const t = (key: string) => key +type TFunc = (key: string, params?: Record) => string + +const t: TFunc = (key, params) => { + if (!params) return key + let s = key + for (const [k, v] of Object.entries(params)) { + s = s.replaceAll(`{${k}}`, String(v)) + } + return s +} afterEach(() => { vi.useRealTimers() @@ -14,10 +23,68 @@ describe('formatSessionListDate', () => { }) describe('formatRelativeTime', () => { - it('uses the padded date after the relative-time window', () => { - vi.useFakeTimers() - vi.setSystemTime(new Date(2026, 6, 21, 12, 0)) + const NOW = new Date('2026-06-13T17:00:00Z').getTime() + beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(new Date(NOW)) + }) + + it('returns just-now bucket for sub-minute deltas', () => { + expect(formatRelativeTime(NOW - 30_000, t)).toBe('session.time.justNow') + expect(formatRelativeTime(NOW - 59_000, t)).toBe('session.time.justNow') + }) + + it('returns minutes bucket for sub-hour deltas', () => { + expect(formatRelativeTime(NOW - 5 * 60_000, t)).toBe('session.time.minutesAgo') + expect(formatRelativeTime(NOW - 5 * 60_000, ((key, params) => { + return `${key}:${params?.n ?? ''}` + }) as TFunc)).toBe('session.time.minutesAgo:5') + }) + + it('returns hours bucket for sub-day deltas', () => { + expect(formatRelativeTime(NOW - 3 * 60 * 60_000, ((key, params) => { + return `${key}:${params?.n ?? ''}` + }) as TFunc)).toBe('session.time.hoursAgo:3') + }) + + it('returns days bucket for sub-week deltas', () => { + expect(formatRelativeTime(NOW - 4 * 24 * 60 * 60_000, ((key, params) => { + return `${key}:${params?.n ?? ''}` + }) as TFunc)).toBe('session.time.daysAgo:4') + }) + + it('falls back to the padded session-list date for >= 1 week', () => { + const tenDaysAgo = NOW - 10 * 24 * 60 * 60_000 + const out = formatRelativeTime(tenDaysAgo, t) + expect(out).toBe(formatSessionListDate(new Date(tenDaysAgo))) + }) + + it('uses the padded date after the relative-time window', () => { + vi.setSystemTime(new Date(2026, 6, 21, 12, 0)) expect(formatRelativeTime(new Date(2026, 6, 7, 9, 0).getTime(), t)).toBe('2026/07/07') }) + + it('treats Unix-second timestamps the same as ms (auto-detect)', () => { + const secs = Math.floor((NOW - 30_000) / 1000) + expect(formatRelativeTime(secs, t)).toBe('session.time.justNow') + }) + + it('returns null for non-finite values', () => { + expect(formatRelativeTime(Number.NaN, t)).toBeNull() + expect(formatRelativeTime(Number.POSITIVE_INFINITY, t)).toBeNull() + }) +}) + +describe('formatAbsoluteDateTime', () => { + it('returns a non-null string for finite ms timestamps', () => { + const out = formatAbsoluteDateTime(new Date('2026-06-13T17:00:00Z').getTime()) + expect(out).not.toBeNull() + expect(typeof out).toBe('string') + }) + + it('returns null for non-finite values', () => { + expect(formatAbsoluteDateTime(Number.NaN)).toBeNull() + expect(formatAbsoluteDateTime(Number.POSITIVE_INFINITY)).toBeNull() + }) }) diff --git a/web/src/lib/relativeTime.ts b/web/src/lib/relativeTime.ts index ac014256..f0050fb3 100644 --- a/web/src/lib/relativeTime.ts +++ b/web/src/lib/relativeTime.ts @@ -27,3 +27,13 @@ export function formatRelativeTime( if (days < 7) return t('session.time.daysAgo', { n: days }) return formatSessionListDate(new Date(ms)) } + +/** + * Absolute date+time string for tooltips that want the precise stamp + * alongside the smart-relative label. Locale-aware. + */ +export function formatAbsoluteDateTime(value: number): string | null { + const ms = value < 1_000_000_000_000 ? value * 1000 : value + if (!Number.isFinite(ms)) return null + return new Date(ms).toLocaleString() +} diff --git a/web/src/lib/scratchlist.ts b/web/src/lib/scratchlist.ts index 94ecf9ca..89106947 100644 --- a/web/src/lib/scratchlist.ts +++ b/web/src/lib/scratchlist.ts @@ -24,6 +24,13 @@ export type ScratchlistEntry = { id: string text: string createdAt: number + /** + * Last-saved timestamp surfaced by the entry-age indicator (clock + * icon + tooltip). Optional so v1-only callers (the standalone + * panel fixture, legacy localStorage rows that pre-date v2) keep + * working - readers fall back to `createdAt` when absent. + */ + updatedAt?: number } function getStorageKey(sessionId: string): string { @@ -44,13 +51,19 @@ function getLocalStorage(): Storage | null { function isEntry(value: unknown): value is ScratchlistEntry { if (!value || typeof value !== 'object') return false const entry = value as Record - return ( - typeof entry.id === 'string' - && entry.id.length > 0 - && typeof entry.text === 'string' - && typeof entry.createdAt === 'number' - && Number.isFinite(entry.createdAt) - ) + if ( + typeof entry.id !== 'string' + || entry.id.length === 0 + || typeof entry.text !== 'string' + || typeof entry.createdAt !== 'number' + || !Number.isFinite(entry.createdAt) + ) return false + if (entry.updatedAt !== undefined) { + if (typeof entry.updatedAt !== 'number' || !Number.isFinite(entry.updatedAt)) { + return false + } + } + return true } export function readScratchlist(sessionId: string): ScratchlistEntry[] { 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 00000000..0cca9086 --- /dev/null +++ b/web/src/lib/use-hub-scratchlist.test.tsx @@ -0,0 +1,676 @@ +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 { ApiError } from '@/api/client' +import { useHubScratchlist } from './use-hub-scratchlist' +import { queryKeys } from './query-keys' + +/** + * 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('dedupes when SSE refetch lands before POST resolves (HAPI Bot, PR #896)', async () => { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: Infinity }, + mutations: { retry: false } + } + }) + const sid = makeSid() + const canonical: HubEntry = { entryId: 'hub-id', text: 'new note', createdAt: 5000, updatedAt: 5000 } + let releaseCreate: (value: { entry: HubEntry }) => void = () => undefined + const createDeferred = new Promise<{ entry: HubEntry }>((resolve) => { + releaseCreate = resolve + }) + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }), + createScratchlistEntry: async () => createDeferred + }) + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper }) + await waitFor(() => expect(result.current.isLoading).toBe(false)) + + let addPromise: Promise | undefined + await act(async () => { + addPromise = result.current.add('new note') + }) + await waitFor(() => expect(result.current.entries.length).toBe(1)) + const optimisticId = result.current.entries[0]?.id + expect(optimisticId).not.toBe('hub-id') + + // Simulate SSE invalidation/refetch beating the POST response. + await act(async () => { + queryClient.setQueryData(queryKeys.scratchlist(sid), { + entries: [ + canonical, + { + entryId: optimisticId!, + text: 'new note', + createdAt: 0, + updatedAt: 0 + } + ] + }) + }) + + await act(async () => { + releaseCreate({ entry: canonical }) + await addPromise + }) + + await waitFor(() => { + expect(result.current.entries.filter((e) => e.id === 'hub-id')).toHaveLength(1) + }) + expect(result.current.entries).toHaveLength(1) + }) + + 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('removes optimistic ghost when create fails before initial fetch populates (HAPI Bot, PR #896)', async () => { + const sid = makeSid() + let releaseFetch: (value: { entries: HubEntry[] }) => void = () => undefined + const fetchDeferred = new Promise<{ entries: HubEntry[] }>((resolve) => { + releaseFetch = resolve + }) + const api = createMockApi({ + getScratchlist: async () => fetchDeferred, + createScratchlistEntry: async () => { + throw new Error('HTTP 409: scratchlist_at_cap') + } + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + // Do not wait for fetch - race create against empty previousData. + let added: boolean | undefined + await act(async () => { + added = await result.current.add('ghost') + }) + expect(added).toBe(false) + await waitFor(() => expect(result.current.entries).toHaveLength(0)) + + await act(async () => { + releaseFetch({ entries: [] }) + }) + await waitFor(() => expect(result.current.isLoading).toBe(false)) + expect(result.current.entries).toHaveLength(0) + }) + + 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']) + }) + + it('keeps entry removed when hub returns 404 (deleted elsewhere) (HAPI Bot, PR #896)', async () => { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: Infinity }, + mutations: { retry: false } + } + }) + const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries') + const sid = makeSid() + let fetchCount = 0 + const api = createMockApi({ + getScratchlist: async () => { + fetchCount += 1 + if (fetchCount === 1) { + return { + entries: [ + { entryId: 'a', text: 'A', createdAt: 1, updatedAt: 1 }, + { entryId: 'b', text: 'B', createdAt: 2, updatedAt: 2 } + ] + } + } + return { + entries: [{ entryId: 'b', text: 'B', createdAt: 2, updatedAt: 2 }] + } + }, + deleteScratchlistEntry: async () => { + throw new ApiError('Not found', 404) + } + }) + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper }) + await waitFor(() => expect(result.current.entries.length).toBe(2)) + + await act(async () => { + await result.current.remove('a') + }) + await waitFor(() => expect(result.current.entries.map((e) => e.id)).toEqual(['b'])) + expect(invalidateSpy).toHaveBeenCalled() + }) +}) + +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')) + }) + + it('drops entry when update returns 404 (deleted elsewhere) (HAPI Bot, PR #896)', async () => { + const sid = makeSid() + let fetchCount = 0 + const api = createMockApi({ + getScratchlist: async () => { + fetchCount += 1 + if (fetchCount === 1) { + return { + entries: [{ entryId: 'a', text: 'before', createdAt: 1, updatedAt: 1 }] + } + } + return { entries: [] } + }, + updateScratchlistEntry: async () => { + throw new ApiError('Not found', 404) + } + }) + 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).toHaveLength(0)) + }) +}) + +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() + // HAPI Bot, PR #896 follow-up: migrationFlag-set without a + // dismiss flag now means 'completed' (banner shows on + // remount) so the operator gets a chance to see and dismiss + // the banner across page reloads. + expect(result.current.migrationStatus).toBe('completed') + }) + + it('reload-before-dismiss leaves the banner visible (PR #896 follow-up)', async () => { + // Mount #1: real v1 entries exist, migration runs, status flips + // to 'completed', banner is shown but the operator reloads + // before clicking dismiss. + const sid = makeSid() + seedV1Entries(sid) + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }), + createScratchlistEntry: async (_id: string, body: { entryId?: string; text: string; createdAt?: number }) => ({ + entry: { + entryId: body.entryId ?? 'srv-' + body.text, + text: body.text, + createdAt: body.createdAt ?? Date.now(), + updatedAt: Date.now() + } + }) + }) + const first = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(first.result.current.migrationStatus).toBe('completed')) + first.unmount() + + // Mount #2: simulating a page reload with the migration flag + // set but the dismiss flag still absent. Pre-fix the hook + // mapped this to 'pre-migrated' and the banner stayed hidden + // forever; post-fix the hook maps it to 'completed' so the + // banner renders again until the operator clicks dismiss. + expect(localStorage.getItem(`hapi.scratchlist.v2.migrated.${sid}`)).toBe('1') + expect(localStorage.getItem(`hapi.scratchlist.v2.banner-dismissed.${sid}`)).toBeNull() + const second = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(second.result.current.isLoading).toBe(false)) + expect(second.result.current.migrationStatus).toBe('completed') + }) + + it('opts fresh sessions (no v1 entries) out of the banner pre-emptively', async () => { + // Companion to the above: a session that NEVER had v1 + // entries should write BOTH the migrated and dismissed flags + // up front so the banner never appears (now or on reload). + // Without this opt-out the PR #896 fix would otherwise spam + // every brand-new v2 session with a banner that has nothing + // to announce. + const sid = makeSid() + // No seedV1Entries - localStorage is empty for this sid. + const api = createMockApi({ + getScratchlist: async () => ({ entries: [] }) + }) + const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() }) + await waitFor(() => expect(result.current.migrationStatus).toBe('dismissed')) + expect(localStorage.getItem(`hapi.scratchlist.v2.migrated.${sid}`)).toBe('1') + expect(localStorage.getItem(`hapi.scratchlist.v2.banner-dismissed.${sid}`)).toBe('1') + }) + + 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 and pre-dismisses the banner (HAPI Bot, PR #896 follow-up)', 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() + // Fresh sessions (no v1 entries) get the banner pre-dismissed + // so the bot's banner-stickiness fix does not surface a + // banner that has nothing to announce. + expect(localStorage.getItem(`hapi.scratchlist.v2.banner-dismissed.${sid}`)).toBe('1') + expect(result.current.migrationStatus).toBe('dismissed') + }) + + 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 retry failed migration in a tight loop within the same mount (HAPI Bot, PR #896)', async () => { + const sid = makeSid() + seedV1Entries(sid) + const create = vi.fn(async () => { + throw new Error('HTTP 409: scratchlist_at_cap') + }) + 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')) + const callsAfterFailure = create.mock.calls.length + + // Let any spurious effect churn settle - must not hammer the hub. + await act(async () => { + await new Promise((r) => setTimeout(r, 100)) + }) + expect(create).toHaveBeenCalledTimes(callsAfterFailure) + }) + + 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)', () => { + 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 00000000..57b4811c --- /dev/null +++ b/web/src/lib/use-hub-scratchlist.ts @@ -0,0 +1,559 @@ +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query' +import type { ApiClient } from '@/api/client' +import { ApiError } from '@/api/client' +import { queryKeys } from '@/lib/query-keys' +import { + moveScratchlistEntry, + persistScratchlist, + 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 or a prior one) and + // the user has not yet dismissed the banner. The + // banner shows in this state, including across + // reloads, until the dismiss flag is written. + | 'dismissed' // banner was acknowledged; do not surface again + +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`. `updatedAt` is forwarded so the per-entry age + * indicator (clock icon + tooltip) can render the smart-relative time + * the operator asked for; v1 callers that don't render it just ignore + * the field. + */ +function toLocalEntry(hub: HubEntry): ScratchlistEntry { + return { + id: hub.entryId, + text: hub.text, + createdAt: hub.createdAt, + updatedAt: hub.updatedAt + } +} + +function isScratchlistNotFound(error: unknown): boolean { + return error instanceof ApiError && error.status === 404 +} + +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() + // Stable identity: queryKeys.scratchlist() returns a fresh array + // each call; without useMemo the migration effect's queryKey dep + // changes every render and re-triggers after a failed POST clears + // migrationAttemptedRef (tight retry loop on persistent 409/offline). + const queryKey = useMemo(() => queryKeys.scratchlist(sessionId), [sessionId]) + const enabled = Boolean(api && sessionId) + const migrationAttemptedRef = useRef(false) + const [migrationStatus, setMigrationStatus] = useState(() => { + if (!sessionId) return 'idle' + if (readBannerDismissed(sessionId)) return 'dismissed' + // HAPI Bot, PR #896 follow-up: the migration flag alone does + // NOT mean the operator saw the banner. If they reloaded + // before clicking dismiss, they need to see it again on + // remount - so 'completed' is sticky until the dismiss flag + // is written. Sessions that had nothing to migrate write the + // dismiss flag pre-emptively in the migration effect, so they + // bypass this branch entirely and land in 'dismissed' above. + if (readMigrationFlag(sessionId)) return 'completed' + 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)) { + // See useState init comment: migration-flag-set is the + // 'banner shows until dismissed' state. + setMigrationStatus('completed') + } else { + setMigrationStatus('idle') + } + }, [sessionId]) + + // Migration trigger: runs ONCE per session when: + // - api is available + // - migration flag is unset + // - localStorage holds v1 entries + // 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 (readMigrationFlag(sessionId)) return + + const localEntries = readScratchlist(sessionId) + if (localEntries.length === 0) { + // Nothing to migrate. Mark the session migrated AND + // pre-dismiss the banner: there is no v1->v2 transition + // to surface for this session, so the operator should not + // see the banner at all (now or after a reload). The + // init logic now treats migrationFlag-without-dismiss as + // 'banner shows', so we have to opt this fresh-session + // case out explicitly. Keeps the bot's PR #896 follow-up + // banner-stickiness fix from spamming new sessions with + // a migration banner they have nothing to migrate from. + writeMigrationFlag(sessionId) + writeBannerDismissed(sessionId) + setMigrationStatus('dismissed') + return + } + + migrationAttemptedRef.current = true + 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 + // 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 { + // 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): 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') + } + })() + }, [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) => { + // When previousData is missing (initial fetch cancelled before + // populate), still drop the optimistic ghost so a rejected POST + // cannot leave an unsaved note in the UI. + if (context?.previousData !== undefined) { + queryClient.setQueryData(queryKey, context.previousData) + return + } + if (context?.optimisticEntryId) { + queryClient.setQueryData(queryKey, (prev) => ({ + entries: (prev?.entries ?? []).filter((e) => e.entryId !== context.optimisticEntryId) + })) + } + }, + onSuccess: (data, _variables, context) => { + // Replace the optimistic entry with the hub-canonical row. + // If SSE invalidation/refetch landed the canonical row before + // POST resolved, also drop any existing row with the same + // entryId so we do not show duplicates client-side. + queryClient.setQueryData(queryKey, (prev) => { + if (!prev) return { entries: [data.entry] } + const without = prev.entries.filter((e) => + e.entryId !== context?.optimisticEntryId + && e.entryId !== data.entry.entryId + ) + 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 (isScratchlistNotFound(error)) { + queryClient.setQueryData(queryKey, (prev) => { + if (!prev) return prev + return { entries: prev.entries.filter((e) => e.entryId !== _variables.entryId) } + }) + void queryClient.invalidateQueries({ queryKey }) + return + } + 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 (isScratchlistNotFound(error)) { + queryClient.setQueryData(queryKey, (prev) => { + if (!prev) return prev + return { entries: prev.entries.filter((e) => e.entryId !== variables.entryId) } + }) + void queryClient.invalidateQueries({ queryKey }) + return + } + 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. + // + // 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 { + const cached = data.entries.map((e) => ({ + id: e.entryId, + text: e.text, + createdAt: e.createdAt, + updatedAt: e.updatedAt + })) + window.localStorage.setItem( + `hapi.scratchlist.v1.${sessionId}`, + JSON.stringify(cached) + ) + } catch { + // Non-fatal: storage quota / private mode. + } + }, [sessionId, query.data, migrationStatus]) + + 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 00000000..291db91a --- /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 +}