feat(web,hub): scratchlist v2 - hub sync via typed table + session-updated piggyback

[heavygee]

Closes #893. Tracking issue for v2.1 (delete with summarize-and-migrate): #894.

Summary

Promotes per-session scratchlist persistence from per-device localStorage to a hub-backed typed table so notes follow the operator across devices. The v1 panel UI / styling / FUE / keyboard shortcut / copy-to-clipboard surface are deliberately unchanged - this is a backend + sync-layer feature.

Hub side

• New session_scratchlist typed table with (session_id, entry_id) PK, text, created_at, updated_at, FK on sessions(id) ON DELETE CASCADE, and (session_id, created_at DESC) index. Schema bumped V9 → V10; idempotent migration added to both the legacy backfill and step ladders.
• REST CRUD under /api/sessions/:id/scratchlist[/:entryId], all routed through the existing requireSessionFromParam guard so namespace / ownership enforcement is identical to other session-scoped routes.
• Per-session 200-entry cap enforced on POST. Duplicate entryId reported idempotently (200) so the migration retry path is safe even after a partial failure.
• SessionPatchSchema extended with scratchlistUpdatedAt?: number; every successful mutation emits a session-updated SSE patch carrying the token. (Per the operator's piggyback decision in the brief - aligns with the parallel Performance: web client useSession refetch storm dominates hub access logs (~95% of syslog volume on busy installs) #884 patch-shape extension; either order merges cleanly.)

Web side

• Hub becomes source of truth via TanStack Query (queryKeys.scratchlist(sessionId)); localStorage demoted to offline cache. Add / delete / update mutations are optimistic with rollback on error.
• Silent first-load migration: existing localStorage entries are pushed to the hub preserving id + createdAt, and a one-time banner (mirroring CursorMigrationBanner) tells the operator their notes are now in the hub. Banner dismissal is per-session and persistent.
• SSE handler queues a scratchlist invalidation when the patch carries scratchlistUpdatedAt, so cross-device + cross-tab updates land within an SSE round-trip.
• Delete-session confirm copy now includes a count of scratchlist entries that will be cascade-deleted.

Out of scope

The "delete with summarize-and-migrate" UX flow (separate tracking issue #894) is not implemented here. v2.0 does the simpler "This will also delete N scratchlist entries" confirm-copy variant only.

Test plan

• Hub: V9→V10 migration covered (fresh DB, multi-hop V8/V9 legacy, idempotent reopen, FK cascade-delete), ScratchlistStore CRUD + ordering + cross-session isolation.
• Hub routes: happy paths, 400 (invalid body / missing entryId), 403 (cross-namespace), 404 (unknown session / entry), 409 (cap reached), idempotent duplicate POST.
• Hub sync: SyncEngine mutations emit one session-updated patch per change with scratchlistUpdatedAt.
• Web hook: optimistic add / delete / update with rollback, cap enforcement, local-only reorder, localStorage→hub migration with retry-safe duplicates, banner state machine.
• Web banner component: renders only on migrationStatus === 'completed', dismissal persists.
• Existing Playwright e2e (10 tests covering panel UI / inert / persistence / promote / shortcut / regression) - all pass unchanged, confirming no UI regression.
• bun typecheck clean across cli / web / hub.
• bun run test clean for hub (456) and web (982); cli (964 passing) - one pre-existing runner.integration.test.ts failure on upstream/main is unrelated.

Made with Cursor

[github-actions]

Findings

• [Major] Migration can erase local-only notes after a partial upload - the migration loop swallows each failed POST and still writes the migrated flag; meanwhile the offline-cache effect mirrors the initial empty hub response back into hapi.scratchlist.v1.<sessionId>, so a transient error or cap rejection can leave entries neither on the hub nor in localStorage. Evidence web/src/lib/use-hub-scratchlist.ts:242.
  Suggested fix:

  const failedEntries: ScratchlistEntry[] = []
  for (const entry of localEntries) {
      try {
          await api.createScratchlistEntry(sessionId, {
              text,
              entryId: entry.id,
              createdAt: entry.createdAt
          })
      } catch {
          failedEntries.push(entry)
      }
  }
  if (failedEntries.length > 0) {
      persistScratchlist(sessionId, failedEntries)
      migrationAttemptedRef.current = false
      setMigrationStatus("idle")
      return
  }
  writeMigrationFlag(sessionId)

• [Major] Duplicate migration retry is blocked once the session is at the cap - the POST route checks count >= SCRATCHLIST_MAX_ENTRIES before asking the store whether the supplied entryId already exists, so retrying an already-created entry at 200 rows returns 409 instead of the documented idempotent 200. Evidence hub/src/web/routes/sessions.ts:661.
  Suggested fix:

  if (parsed.data.entryId) {
      const existing = engine.getScratchlistEntry(sessionResult.sessionId, parsed.data.entryId)
      if (existing) {
          return c.json({ entry: existing }, 200)
      }
  }
  
  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)
  }

Summary

• Review mode: initial
• Found two Major issues in the new scratchlist migration/API path: one can lose local-only migrated notes on partial upload failure, and one breaks the duplicate POST idempotency contract at the cap boundary.

Testing

• Not run (automation)

HAPI Bot

[heavygee]

Thanks - both Major findings were valid. Pushed 29a0f6ee addressing both:

Major 1 (migration data loss): the migration loop now tracks failed entries, persists them back to hapi.scratchlist.v1.<sessionId>, and leaves the migrated flag unset so a future mount retries. The offline-cache effect is now gated on the migration flag too, so an empty initial hub fetch can no longer clobber the v1 entries the migration is about to read. I also relaxed the "skip migration when hub is non-empty" gate; combined with the idempotent duplicate POST short-circuit (Major 2 below), a retry against a session another device populated is now a safe union.

Major 2 (duplicate at cap returns 409): the route now checks duplicate via a new SyncEngine.getScratchlistEntry BEFORE the cap check, returning 200 with the existing row. Cap check still fires for genuinely new ids.

Tests added:

• sessions-scratchlist.test.ts: at-cap + duplicate entryId returns 200 (not 409); at-cap + new entryId still 409.
• use-hub-scratchlist.test.tsx: partial-migration failure persists the failed entries back to localStorage and leaves the flag unset; the offline-cache effect does not wipe pre-migration localStorage.

[github-actions]

Findings

• No issues found.

Summary

• Review mode: follow-up after new commits
• Full PR diff reviewed at the latest head. The two prior Major findings were addressed in the follow-up changes; no new high-confidence correctness, security, data-loss, performance, or maintainability issues found in the added/modified lines.
• Residual risk: web coverage is unit-focused around the hook and banner; no browser/e2e run was performed in this review pass.

Testing

• Not run (automation)

HAPI Bot

[github-actions]

Findings

• [Minor] Bound client-supplied scratchlist entry IDs — entryId is validated only as non-empty while the new scratchlist text and entry-count limits are bounded. Because POST persists this client-supplied value as part of the SQLite primary key, an authenticated/direct client can store oversized keys and grow the table/index beyond the intended scratchlist limits. Evidence shared/src/apiTypes.ts:176.
  Suggested fix:

  export const SCRATCHLIST_MAX_ENTRY_ID_LENGTH = 128
  
  export const ScratchlistEntryCreateRequestSchema = z.object({
      entryId: z.string().min(1).max(SCRATCHLIST_MAX_ENTRY_ID_LENGTH).optional(),
      text: z.string().min(1).max(SCRATCHLIST_MAX_TEXT_LENGTH),
      createdAt: z.number().int().nonnegative().optional()
  })

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed. Prior bot findings remain addressed; one low-severity server-side validation gap remains in the scratchlist create schema.
• Residual risk: no local test run in this review pass.

Testing

• Not run (automation)

HAPI Bot

[github-actions]

Findings

• [Minor] Reloading before dismissing suppresses the migration banner — successful migration writes the migrated flag, but initialization maps a migrated/not-dismissed session to pre-migrated; ScratchlistMigrationBanner only renders for completed, so a user who refreshes before clicking dismiss never sees the banner again and the dismissal flag is never required. Evidence web/src/lib/use-hub-scratchlist.ts:193.
  Suggested fix:

  if (readBannerDismissed(sessionId)) return 'dismissed'
  if (readMigrationFlag(sessionId)) return 'completed'
  
  // No local entries migrated, so opt this session out of the banner.
  if (localEntries.length === 0) {
      writeBannerDismissed(sessionId)
      writeMigrationFlag(sessionId)
      return
  }

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed. The prior entryId validation finding is addressed; one minor banner state-machine issue remains.
• Residual risk: no local test run in this review pass.

Testing

• Not run (automation). Suggested: add a hook test for “migration completed, reload before dismiss” so the banner remains visible until the dismiss flag is written.

HAPI Bot

[github-actions]

Findings

• None.

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed. The previous banner state-machine finding is addressed in the new commits, including coverage for reload-before-dismiss and fresh-session opt-out.
• Residual risk: scratchlist sync now spans hub storage, REST, SSE invalidation, and localStorage migration, so cross-device behavior still depends on the full automation matrix passing.

Testing

• Not run (automation).

HAPI Bot

[github-actions]

Findings

• None.

Questions

• None.

Summary
Review mode: follow-up after new commits
Full current PR diff reviewed, including the prior bot context and the new session-merge scratchlist transfer delta. No high-confidence correctness, security, data-loss, or regression findings to report.
Residual risk: scratchlist sync crosses hub storage, REST, SSE invalidation, migration flags, and offline localStorage cache; local validation could not run in this runner because bun is unavailable.

Testing

• Not run: bun typecheck and bun run test failed because bun was not found in the runner PATH.

HAPI Bot

[heavygee]

Rebased onto upstream/main (was behind 2: #904 service_tier + #886 codex machine binding).

Conflict resolution: upstream took schema V9→V10 for sessions.service_tier; scratchlist v2 now uses V10→V11 for session_scratchlist. Both migrations coexist.

Open review threads: all 4 resolved - fixes were already landed in prior commits:

• Major (migration data loss): 29a0f6ee / follow-ups in use-hub-scratchlist.ts
• Major (cap-before-duplicate): 29a0f6ee in sessions.ts
• Minor (entryId length): dc07b919 in apiTypes.ts
• Minor (banner pre-migrated): 0fab1dfa in use-hub-scratchlist.ts

Latest: 063a1398 (migration rename + rebase fallout fix).

[github-actions]

Findings

• None.

Questions

• None.

Summary
Review mode: follow-up after new commits
Full current PR diff reviewed, including the prior HAPI Bot review, the follow-up compare from 1a58c80cd969c4ee0e8dee2e8b746f330d383dcb to 063a1398574b8e3236aa8a07f0fd93be53aba8da, and surrounding hub/web/shared context. No high-confidence correctness, security, data-loss, performance, or maintainability findings to report.
Residual risk: scratchlist v2 spans SQLite migration, REST, SSE invalidation, session merge transfer, and localStorage migration/offline cache behavior; the diff includes focused coverage for those paths, but this review was static.

Testing

• Not run (automation): static PR review only; did not execute code from PR content.

HAPI Bot

[github-actions]

Findings

• [Major] All-collision scratchlist transfers leave the still-alive source session cache stale — transferScratchlistEntries() deletes colliding rows from oldSessionId, but mergeSessionData() only emits scratchlist invalidations when moved > 0. In the mergeSessionHistory(..., deleteOldSession=false) path, if every old entry collides with the target PK, the old session remains visible while its scratchlist rows were deleted, and any web client on that old session keeps showing entries that now 404 on edit/delete until some unrelated refetch. Evidence: hub/src/sync/sessionCache.ts:781.

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed with prior HAPI Bot review and follow-up context. One sync invalidation gap found in the scratchlist merge path.

Testing

• Not run (automation): static PR review only; did not execute code from PR content. Suggested coverage: add a mergeSessionHistory test where old and new sessions contain only the same entryId, then assert a scratchlist invalidation is emitted for the still-alive old session.

HAPI Bot

[github-actions]

Findings

• [Major] Failed localStorage migrations retry in a tight render loop — queryKeys.scratchlist(sessionId) returns a new array each render, and the migration effect depends on that queryKey. When any POST fails, the effect writes failed rows back, clears migrationAttemptedRef, sets status back to idle, and re-renders; because queryKey identity changed, the effect immediately starts the same failed migration again instead of waiting for a future mount. Persistent 409/offline failures can hammer the hub and keep the UI cycling migrating/idle. Evidence web/src/lib/use-hub-scratchlist.ts:158.
  Suggested fix:

  import { useCallback, useEffect, useMemo, useRef, useState } from 'react'
  
  const queryKey = useMemo(() => queryKeys.scratchlist(sessionId), [sessionId])

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed with prior HAPI Bot review and follow-up compare context. The previous collision invalidation issue is addressed; one web migration retry regression remains.

Testing

• Not run (automation): static PR review only; did not execute code from PR content. Suggested coverage: a migration test where createScratchlistEntry always rejects should assert it is not called again until remount/refetch.

HAPI Bot

[github-actions]

Findings

• [Major] Optimistic add can duplicate the new scratchlist entry when SSE wins the race — the server emits scratchlistUpdatedAt before the POST returns, so another tab or the same tab can invalidate/refetch queryKeys.scratchlist(sessionId) and cache the canonical row while the mutation is still pending. onSuccess then only filters the temporary optimistic id and prepends data.entry, leaving two rows with the same entryId in the UI/offline cache until a later refetch. Evidence web/src/lib/use-hub-scratchlist.ts:353.
  Suggested fix:

  queryClient.setQueryData<ScratchlistResponse>(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] }
  })

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed with prior HAPI Bot review and follow-up compare context. Prior tight-loop retry finding is fixed; one remaining optimistic reconciliation race can leave duplicate scratchlist rows client-side.

Testing

• Not run (automation): static PR review only; did not execute code from PR content. Suggested coverage: seed the scratchlist query with the canonical server row before createScratchlistEntry resolves, then assert add() leaves one row for that entryId.

HAPI Bot

[github-actions]

Findings

• [Major] Missing-entry errors restore deleted scratchlist rows in stale clients — the new optimistic mutations roll back to previousData for every error. When another tab/device has already deleted an entry, this client can still have the old query cache, send DELETE/PUT, receive the route's 404, and then restore the stale row locally; the offline-cache mirror can persist that stale row until a later refetch. Treat 404 as stale cache instead of rollback. Evidence web/src/lib/use-hub-scratchlist.ts:411.
  Suggested fix:

  import { ApiError, type ApiClient } from '@/api/client'
  
  const isNotFound = (error: unknown) => error instanceof ApiError && error.status === 404
  
  onError: (error, variables, context) => {
      if (isNotFound(error)) {
          queryClient.setQueryData<ScratchlistResponse>(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)
      }
  }

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed with prior HAPI Bot review and follow-up compare context. Prior duplicate-row finding is fixed; one remaining stale-cache race can make deleted scratchlist entries reappear client-side after a 404.

Testing

• Not run (automation): static PR review only. Suggested coverage: seed the scratchlist query with an entry, make deleteScratchlistEntry reject with new ApiError(..., 404), and assert the entry stays removed and the query is invalidated.

HAPI Bot

[github-actions]

Findings

• None.

Questions

• None.

Summary

• Review mode: follow-up after new commits
• Full current PR diff reviewed with prior HAPI Bot review and follow-up compare context. The previously reported stale 404 rollback issue is addressed in the latest head. No new Blocker/Major/Minor/Nit findings above the confidence bar.
• Residual risk: review was static only; I did not execute PR code.

Testing

• Not run (automation): static PR review only.

HAPI Bot