fix(scratchlist): prevent cross-session leak in useScratchlist hook

Per upstream review on PR tiann#798 (github-actions[bot] [Major]):

  > useScratchlist persists current `entries` whenever `sessionId`
  > changes. On A -> B navigation, React first commits with B's id
  > and A's entries; after paint, this persist effect can write A's
  > entries to hapi.scratchlist.v1.B before the rehydrate effect
  > loads B. The previous keyed panel existed specifically to avoid
  > this race.

Lifting state out of the v1 panel (which sidestepped the race via
key={props.session.id} forced remount) re-introduced this same data-
loss window. The composer-controlled drawer in v1.1 cannot remount
on session change because its parent SessionChat doesn't either.

Fix: keep the loaded sessionId in state alongside the entries so they
swap atomically, and persist against the LOADED sessionId rather than
the prop. After A->B, the loaded sessionId is still A until rehydrate
runs, so a spurious persist re-writes A's storage with A's entries -
a no-op instead of a corruption.

Tests:
- New use-scratchlist.test.ts with 6 tests:
  - hydrates from localStorage on mount
  - add() persists to current session's storage only
  - rerender to a new session preserves the new session's existing entries
  - after switching, add() targets the new session
  - regression test that spies on Storage.prototype.setItem and asserts
    the rerender lifecycle never produces a (B-key, A-entries) write
  - remove()/move() target the loaded sessionId
- The setItem-spy test correctly fails against the buggy code (verified
  by temporarily reverting the fix) and passes with the fix in place.
- Full web suite: 88 files, 756 tests, all green. Typecheck clean.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: heavygee

web/src/lib/use-scratchlist.test.ts

@@ -0,0 +1,136 @@
+import { act, renderHook } from '@testing-library/react'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { addScratchlistEntry, persistScratchlist, readScratchlist } from './scratchlist'
+import { useScratchlist } from './use-scratchlist'
+
+const SESSION_A = 'session-a'
+const SESSION_B = 'session-b'
+
+describe('useScratchlist', () => {
+    beforeEach(() => {
+        localStorage.clear()
+    })
+
+    afterEach(() => {
+        localStorage.clear()
+    })
+
+    it('hydrates from localStorage on mount', () => {
+        const { entries: seeded } = addScratchlistEntry([], 'a-only', 1000)
+        persistScratchlist(SESSION_A, seeded)
+        const { result } = renderHook(({ id }: { id: string }) => useScratchlist(id), {
+            initialProps: { id: SESSION_A },
+        })
+        expect(result.current.entries.map((e) => e.text)).toEqual(['a-only'])
+    })
+
+    it('add() persists to the current sessions storage', () => {
+        const { result } = renderHook(({ id }: { id: string }) => useScratchlist(id), {
+            initialProps: { id: SESSION_A },
+        })
+        act(() => {
+            result.current.add('first')
+        })
+        expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['first'])
+        expect(readScratchlist(SESSION_B)).toEqual([])
+    })
+
+    it('switching sessions does NOT overwrite the new sessions storage with stale entries', () => {
+        // Regression test for the cross-session leak found by upstream review on PR #798.
+        // Seed both sessions distinctly; mount with A; rerender with B.
+        // The persist effect must not write A's entries into B's localStorage
+        // key during the brief render where the prop has changed but the
+        // rehydrate effect hasn't run yet.
+        const { entries: aEntries } = addScratchlistEntry([], 'a-original', 1000)
+        const { entries: bEntries } = addScratchlistEntry([], 'b-original', 2000)
+        persistScratchlist(SESSION_A, aEntries)
+        persistScratchlist(SESSION_B, bEntries)
+
+        const { rerender } = renderHook(({ id }: { id: string }) => useScratchlist(id), {
+            initialProps: { id: SESSION_A },
+        })
+
+        rerender({ id: SESSION_B })
+
+        // After the session switch, B's storage must still contain B's
+        // entry (not A's). Reading from disk because that's what the next
+        // mount of any other component would see.
+        expect(readScratchlist(SESSION_B).map((e) => e.text)).toEqual(['b-original'])
+        // A's storage stays intact too.
+        expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['a-original'])
+    })
+
+    it('after switching sessions, add() targets the new session', () => {
+        const { entries: aEntries } = addScratchlistEntry([], 'a-original', 1000)
+        persistScratchlist(SESSION_A, aEntries)
+
+        const { result, rerender } = renderHook(
+            ({ id }: { id: string }) => useScratchlist(id),
+            { initialProps: { id: SESSION_A } }
+        )
+        rerender({ id: SESSION_B })
+        act(() => {
+            result.current.add('b-only')
+        })
+        expect(readScratchlist(SESSION_B).map((e) => e.text)).toEqual(['b-only'])
+        expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['a-original'])
+    })
+
+    it('switching sessions never writes the previous sessions entries to the new sessions storage key', () => {
+        // The bot's review on PR #798 specifically called out the write
+        // window: between commit-with-new-id and the rehydrate effect
+        // running, the persist effect can fire one corrupting write
+        // (sessionId=B, entries=A's). That write self-heals on the next
+        // render once the rehydrate completes, so a "read after rerender"
+        // assertion would falsely pass. This test inspects every setItem
+        // call that happens during the rerender lifecycle and asserts no
+        // call wrote A's entries to B's storage key.
+        const { entries: aEntries } = addScratchlistEntry([], 'a-original', 1000)
+        const { entries: bEntries } = addScratchlistEntry([], 'b-original', 2000)
+        persistScratchlist(SESSION_A, aEntries)
+        persistScratchlist(SESSION_B, bEntries)
+
+        const { rerender } = renderHook(({ id }: { id: string }) => useScratchlist(id), {
+            initialProps: { id: SESSION_A },
+        })
+
+        const setItemSpy = vi.spyOn(Storage.prototype, 'setItem')
+        rerender({ id: SESSION_B })
+
+        // Storage format is a top-level array of entries (see writeScratchlist
+        // in scratchlist.ts), so unpack and inspect each entry directly.
+        const corruptingWrites = setItemSpy.mock.calls.filter(([key, value]) => {
+            if (typeof key !== 'string' || typeof value !== 'string') return false
+            if (!key.endsWith(SESSION_B)) return false
+            try {
+                const parsed = JSON.parse(value)
+                if (!Array.isArray(parsed)) return false
+                return parsed.some(
+                    (e: { text?: string }) => e?.text === 'a-original'
+                )
+            } catch {
+                return false
+            }
+        })
+        setItemSpy.mockRestore()
+
+        expect(corruptingWrites).toEqual([])
+    })
+
+    it('remove() and move() use the loaded sessionId', () => {
+        const { entries: seeded } = addScratchlistEntry([], 'first', 1000)
+        const { entries: seeded2 } = addScratchlistEntry(seeded, 'second', 2000)
+        persistScratchlist(SESSION_A, seeded2)
+
+        const { result } = renderHook(({ id }: { id: string }) => useScratchlist(id), {
+            initialProps: { id: SESSION_A },
+        })
+
+        const firstId = result.current.entries[0]!.id
+        act(() => {
+            result.current.remove(firstId)
+        })
+        expect(result.current.entries.map((e) => e.text)).toEqual(['first'])
+        expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['first'])
+    })
+})

web/src/lib/use-scratchlist.ts

@@ -12,36 +12,73 @@ import {
  * useScratchlist - per-session scratchlist state hook.
  *
  * Originally the entries lived inside ScratchlistPanel's useState. The
- * composer-controlled drawer needs the same data exposed in two places
- * (the panel + the composer-toolbar counter), so the state gets lifted
- * here. localStorage stays the source of truth; this hook is the React
- * mirror.
+ * composer-controlled drawer (v1.1) needs the same data exposed in two
+ * places (the drawer + the composer-toolbar counter), so the state is
+ * lifted here. localStorage stays the source of truth; this hook is the
+ * React mirror.
+ *
+ * Cross-session race protection
+ * -----------------------------
+ * The naive shape (entries: useState, sessionId: prop, two useEffects)
+ * leaks across session navigation:
+ *
+ *   1. Mount with sessionId=A → entries = readScratchlist(A) = [a1, a2]
+ *   2. Parent rerenders with sessionId=B (same component instance — the
+ *      v1 panel sidestepped this with key={props.session.id}; the v1.1
+ *      lifted hook can't, because its parent SessionChat *isn't*
+ *      remounted on session switch).
+ *   3. React commits with sessionId=B but `entries` is still A's data.
+ *   4. Persist effect fires: persistScratchlist(B, [a1, a2]) —
+ *      OVERWRITES B's storage with A's entries before the rehydrate
+ *      effect has a chance to run.
+ *
+ * Fix (per upstream review on PR #798): keep the loaded sessionId in
+ * state alongside the entries so they can swap atomically, and persist
+ * against the LOADED sessionId, not the current prop. After step 2 the
+ * loaded sessionId is still A (until the rehydrate effect runs), so a
+ * spurious persist re-writes A's storage with A's entries — a no-op
+ * instead of a corruption.
  */
 export function useScratchlist(sessionId: string) {
-    const [entries, setEntries] = useState<ScratchlistEntry[]>(() => readScratchlist(sessionId))
+    const [{ sessionId: loadedSessionId, entries }, setScratchlist] = useState<{
+        sessionId: string
+        entries: ScratchlistEntry[]
+    }>(() => ({ sessionId, entries: readScratchlist(sessionId) }))
 
+    // Rehydrate when the parent navigates to a different session. This
+    // atomically swaps both the loaded sessionId and the entries, so the
+    // persist effect below sees a consistent (sessionId, entries) pair.
     useEffect(() => {
-        setEntries(readScratchlist(sessionId))
+        setScratchlist({ sessionId, entries: readScratchlist(sessionId) })
     }, [sessionId])
 
+    // Persist using the LOADED sessionId, not the prop. If the prop has
+    // moved ahead of the rehydrate effect, this still writes back to the
+    // session whose entries we currently hold — no cross-session leak.
     useEffect(() => {
-        persistScratchlist(sessionId, entries)
-    }, [sessionId, entries])
+        persistScratchlist(loadedSessionId, entries)
+    }, [loadedSessionId, entries])
 
     const add = useCallback((rawText: string): boolean => {
         const result = addScratchlistEntry(entries, rawText)
         if (result.entries === entries) return false
-        setEntries(result.entries)
+        setScratchlist({ sessionId: loadedSessionId, entries: result.entries })
         return true
-    }, [entries])
+    }, [entries, loadedSessionId])
 
     const remove = useCallback((id: string) => {
-        setEntries((prev) => deleteScratchlistEntry(prev, id))
+        setScratchlist((prev) => ({
+            sessionId: prev.sessionId,
+            entries: deleteScratchlistEntry(prev.entries, id),
+        }))
     }, [])
 
     const move = useCallback((id: string, direction: 'up' | 'down') => {
-        setEntries((prev) => moveScratchlistEntry(prev, id, direction))
+        setScratchlist((prev) => ({
+            sessionId: prev.sessionId,
+            entries: moveScratchlistEntry(prev.entries, id, direction),
+        }))
     }, [])
 
-    return { entries, add, remove, move, setEntries }
+    return { entries, add, remove, move }
 }