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

[github-actions]

[MAJOR] The migration loop treats every per-entry POST failure as non-fatal and still writes the migrated flag. Because the later offline-cache effect mirrors the initial empty hub response into hapi.scratchlist.v1.<sessionId>, a transient failure or cap rejection can leave entries neither uploaded nor preserved locally. Keep failed entries in localStorage and do not set the migration flag until all intended entries are accepted.

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)

[github-actions]

[MAJOR] This cap check runs before duplicate detection, so a retry for an entry that already exists returns 409 once the session has 200 rows. That breaks the route contract that duplicate entryId POSTs are idempotent for migration retries. Check an existing caller-supplied entryId before enforcing the new-entry cap.

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)
}

[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]

[MINOR] entryId only has .min(1), but POST persists this client-supplied value as part of the session_scratchlist primary key. The new entry-count/text caps do not bound ID size, so an authenticated/direct client can store oversized keys and bloat the SQLite table/index beyond the intended scratchlist limits. Bound it near UUID/fallback ID sizes.

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()
})

[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]

[MINOR] After a successful migration writes hapi.scratchlist.v2.migrated.*, a reload before the operator clicks the banner dismiss button enters this pre-migrated branch. Since ScratchlistMigrationBanner renders only for completed, the banner is then suppressed permanently without the persistent dismissal flag being written. Keep migrated-but-not-dismissed sessions in the banner-visible state, and explicitly opt out only when there were no local entries to migrate.

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
}

[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