From 39f20afe4e6603877aa1209cc400b8e577230228 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Wed, 27 May 2026 15:39:57 +0100 Subject: [PATCH 1/3] fix(web): eliminate scroll guard setItem unwrap race (#716) Patch scrollRestorationCache.set to persist only through the guarded sessionStorage wrapper instead of temporarily restoring native setItem. Adds concurrent-write tests for the #707 regression. Co-authored-by: Cursor --- web/src/lib/scrollStorageGuard.test.ts | 138 +++++++++++++------------ web/src/lib/scrollStorageGuard.ts | 68 ++++++++---- 2 files changed, 124 insertions(+), 82 deletions(-) diff --git a/web/src/lib/scrollStorageGuard.test.ts b/web/src/lib/scrollStorageGuard.test.ts index 14d0871b0..6abe909d1 100644 --- a/web/src/lib/scrollStorageGuard.test.ts +++ b/web/src/lib/scrollStorageGuard.test.ts @@ -1,14 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' - -const mockScrollCacheSet = vi.hoisted(() => vi.fn()) - -vi.mock('@tanstack/router-core', () => ({ - scrollRestorationCache: { - state: {}, - set: mockScrollCacheSet, - }, -})) - +import { scrollRestorationCache } from '@tanstack/router-core' import { installScrollRestorationGuard } from './scrollStorageGuard' const STORAGE_KEY = 'tsr-scroll-restoration-v1_3' @@ -37,12 +28,19 @@ function makeMockStorage(): Storage & { _store: Record; _setItem return storage } +function makeFullScrollState(count = 100): Record { + const fullState: Record = {} + for (let i = 0; i < count; i++) { + fullState[`/route/${i}`] = { window: { scrollX: 0, scrollY: i } } + } + return fullState +} + describe('installScrollRestorationGuard', () => { let storage: ReturnType let uninstall: () => void beforeEach(() => { - mockScrollCacheSet.mockClear() storage = makeMockStorage() uninstall = installScrollRestorationGuard(storage) }) @@ -65,11 +63,7 @@ describe('installScrollRestorationGuard', () => { this.name = 'SecurityError' } } - const fullState: Record = {} - for (let i = 0; i < 100; i++) { - fullState[`/route/${i}`] = { window: { scrollX: 0, scrollY: i } } - } - const fullValue = JSON.stringify(fullState) + const fullValue = JSON.stringify(makeFullScrollState()) let call = 0 storage._setItem.mockImplementation((key: string, value: string) => { @@ -91,11 +85,7 @@ describe('installScrollRestorationGuard', () => { name: 'QuotaExceededError', message: "Failed to execute 'setItem' on 'Storage': Setting the value of 'tsr-scroll-restoration-v1_3' exceeded the quota." } - const fullState: Record = {} - for (let i = 0; i < 100; i++) { - fullState[`/route/${i}`] = { window: { scrollX: 0, scrollY: i } } - } - const fullValue = JSON.stringify(fullState) + const fullValue = JSON.stringify(makeFullScrollState()) let call = 0 storage._setItem.mockImplementation((key: string, value: string) => { @@ -119,11 +109,7 @@ describe('installScrollRestorationGuard', () => { }) it('prunes oldest entries to exactly the retain count and retries on quota error', () => { - const fullState: Record = {} - for (let i = 0; i < 100; i++) { - fullState[`/route/${i}`] = { window: { scrollX: 0, scrollY: i } } - } - const fullValue = JSON.stringify(fullState) + const fullValue = JSON.stringify(makeFullScrollState()) let call = 0 storage._setItem.mockImplementation((key: string, value: string) => { @@ -140,11 +126,10 @@ describe('installScrollRestorationGuard', () => { const stored = JSON.parse(storage._store[STORAGE_KEY]) as Record const storedKeys = Object.keys(stored) expect(storedKeys.length).toBe(RETAIN_COUNT) - expect(storedKeys).toContain('/route/99') // newest kept - expect(storedKeys).toContain('/route/50') // boundary kept - expect(storedKeys).not.toContain('/route/49') // boundary dropped - expect(storedKeys).not.toContain('/route/0') // oldest dropped - expect(mockScrollCacheSet).not.toHaveBeenCalled() + expect(storedKeys).toContain('/route/99') + expect(storedKeys).toContain('/route/50') + expect(storedKeys).not.toContain('/route/49') + expect(storedKeys).not.toContain('/route/0') }) it('syncs TanStack in-memory scroll cache after a successful prune on real sessionStorage', () => { @@ -152,12 +137,7 @@ describe('installScrollRestorationGuard', () => { vi.stubGlobal('window', { sessionStorage: realSessionStorage }) const off = installScrollRestorationGuard(realSessionStorage) - - const fullState: Record = {} - for (let i = 0; i < 100; i++) { - fullState[`/route/${i}`] = { window: { scrollX: 0, scrollY: i } } - } - const fullValue = JSON.stringify(fullState) + const fullValue = JSON.stringify(makeFullScrollState()) let call = 0 realSessionStorage._setItem.mockImplementation((key: string, value: string) => { @@ -170,35 +150,66 @@ describe('installScrollRestorationGuard', () => { realSessionStorage.setItem(STORAGE_KEY, fullValue) - expect(mockScrollCacheSet).toHaveBeenCalledTimes(1) - const updater = mockScrollCacheSet.mock.calls[0]![0] as ( - state: Record - ) => Record - const synced = updater(fullState) - expect(Object.keys(synced).length).toBe(RETAIN_COUNT) - expect(Object.keys(synced)).toContain('/route/99') - expect(Object.keys(synced)).not.toContain('/route/0') + let inMemoryKeyCount = 0 + scrollRestorationCache!.set((state) => { + inMemoryKeyCount = Object.keys(state).length + return state + }) + expect(inMemoryKeyCount).toBe(RETAIN_COUNT) off() }) - it('hard reset calls scrollRestorationCache through unwrapped setItem', () => { + it('keeps sessionStorage guard active while syncing in-memory scroll cache', () => { const realSessionStorage = makeMockStorage() vi.stubGlobal('window', { sessionStorage: realSessionStorage }) const off = installScrollRestorationGuard(realSessionStorage) const wrappedSetItem = realSessionStorage.setItem + const fullValue = JSON.stringify(makeFullScrollState()) - let cacheWriteSetItem: Storage['setItem'] | undefined - mockScrollCacheSet.mockImplementationOnce(() => { - cacheWriteSetItem = realSessionStorage.setItem + let call = 0 + let setItemDuringCacheSync: Storage['setItem'] | undefined + realSessionStorage._setItem.mockImplementation((key: string, value: string) => { + call += 1 + if (call === 1) { + throw new QuotaExceededError() + } + realSessionStorage._store[key] = value + if (call === 2) { + setItemDuringCacheSync = realSessionStorage.setItem + } }) - realSessionStorage._setItem.mockImplementationOnce(() => { throw new QuotaExceededError() }) - realSessionStorage.setItem(STORAGE_KEY, 'not json {') + realSessionStorage.setItem(STORAGE_KEY, fullValue) - expect(cacheWriteSetItem).toBeDefined() - expect(cacheWriteSetItem).not.toBe(wrappedSetItem) + expect(setItemDuringCacheSync).toBe(wrappedSetItem) + + off() + }) + + it('recovers concurrent scroll cache writes during guard sync without uncaught throws', () => { + const realSessionStorage = makeMockStorage() + vi.stubGlobal('window', { sessionStorage: realSessionStorage }) + + const off = installScrollRestorationGuard(realSessionStorage) + const fullValue = JSON.stringify(makeFullScrollState()) + + let call = 0 + realSessionStorage._setItem.mockImplementation((key: string, value: string) => { + call += 1 + if (call === 1) { + throw new QuotaExceededError() + } + if (call === 2) { + expect(() => scrollRestorationCache!.set(() => ({ + '/concurrent': { window: { scrollX: 0, scrollY: 1 } }, + }))).not.toThrow() + } + realSessionStorage._store[key] = value + }) + + expect(() => realSessionStorage.setItem(STORAGE_KEY, fullValue)).not.toThrow() off() }) @@ -210,23 +221,20 @@ describe('installScrollRestorationGuard', () => { }) it('removes the key entirely if the retried write also throws', () => { - const fullState: Record = {} - for (let i = 0; i < 100; i++) { - fullState[`/route/${i}`] = { window: { scrollX: 0, scrollY: i } } - } storage._setItem.mockImplementation(() => { throw new QuotaExceededError() }) - storage.setItem(STORAGE_KEY, JSON.stringify(fullState)) + storage.setItem(STORAGE_KEY, JSON.stringify(makeFullScrollState())) expect(storage.removeItem).toHaveBeenCalledWith(STORAGE_KEY) }) it('does not reset TanStack scroll cache when guarding mock storage', () => { + const cacheSetBefore = scrollRestorationCache!.set storage._setItem.mockImplementationOnce(() => { throw new QuotaExceededError() }) storage.setItem(STORAGE_KEY, 'not json {') expect(storage.removeItem).toHaveBeenCalledWith(STORAGE_KEY) - expect(mockScrollCacheSet).not.toHaveBeenCalled() + expect(scrollRestorationCache!.set).toBe(cacheSetBefore) }) it('resets TanStack in-memory scroll cache when hard reset uses real sessionStorage', () => { @@ -239,11 +247,13 @@ describe('installScrollRestorationGuard', () => { realSessionStorage.setItem(STORAGE_KEY, JSON.stringify({ stale: { window: { scrollX: 0, scrollY: 1 } } })) expect(realSessionStorage.removeItem).toHaveBeenCalledWith(STORAGE_KEY) - expect(mockScrollCacheSet).toHaveBeenCalledTimes(1) - const updater = mockScrollCacheSet.mock.calls[0]![0] as ( - state: Record - ) => Record - expect(updater({ stale: { window: { scrollX: 0, scrollY: 1 } } })).toEqual({}) + + let inMemoryKeys: string[] = [] + scrollRestorationCache!.set((state) => { + inMemoryKeys = Object.keys(state) + return state + }) + expect(inMemoryKeys).toEqual([]) off() }) diff --git a/web/src/lib/scrollStorageGuard.ts b/web/src/lib/scrollStorageGuard.ts index 80679fb86..d99f1026e 100644 --- a/web/src/lib/scrollStorageGuard.ts +++ b/web/src/lib/scrollStorageGuard.ts @@ -4,9 +4,10 @@ * the package's public API — update this constant if the library bumps the * suffix on `tsr-scroll-restoration-v1_*`). */ -import { scrollRestorationCache } from '@tanstack/router-core' +import { functionalUpdate, scrollRestorationCache } from '@tanstack/router-core' type ScrollCacheUpdater = NonNullable['set']>[0]> +type ScrollCacheState = Record const STORAGE_KEY = 'tsr-scroll-restoration-v1_3' @@ -18,24 +19,41 @@ interface GuardedStorage extends Storage { [GUARD_MARKER]?: true } -function writeScrollRestorationCache( - storage: Storage, - originalSetItem: Storage['setItem'], - updater: ScrollCacheUpdater, -): void { - const guardedSetItem = storage.setItem - storage.setItem = originalSetItem +function readScrollCacheState(storage: Storage): ScrollCacheState { try { - scrollRestorationCache?.set(updater) - } finally { - storage.setItem = guardedSetItem + const raw = storage.getItem(STORAGE_KEY) + return raw ? JSON.parse(raw) as ScrollCacheState : {} + } catch { + return {} + } +} + +function patchScrollRestorationCacheSet( + storage: Storage, +): () => void { + if (!scrollRestorationCache) { + return () => {} + } + const originalCacheSet = scrollRestorationCache.set.bind(scrollRestorationCache) + let cacheState = readScrollCacheState(storage) + + scrollRestorationCache.set = (updater: ScrollCacheUpdater): void => { + cacheState = (functionalUpdate(updater, cacheState as never) ?? cacheState) as ScrollCacheState + // Always persist through the guarded sessionStorage.setItem — never unwrap + // native setItem (see tiann/hapi#716). + storage.setItem(STORAGE_KEY, JSON.stringify(cacheState)) + } + + return () => { + scrollRestorationCache!.set = originalCacheSet } } function hardResetScrollRestorationPersistedState( storage: Storage, originalSetItem: Storage['setItem'], - isRealSessionStorage: boolean + isRealSessionStorage: boolean, + recoveryDepth: { current: number }, ): void { try { storage.removeItem(STORAGE_KEY) @@ -48,14 +66,25 @@ function hardResetScrollRestorationPersistedState( // TanStack keeps the full scroll map in memory even when setItem fails. // Pruning only the JSON string leaves RAM oversized — the next scroll // write throws again. Clear the library cache so persisted size matches. + if (recoveryDepth.current > 0) { + try { + storage.setItem(STORAGE_KEY, '{}') + } catch { + // nested recovery already in progress + } + return + } + recoveryDepth.current += 1 try { - writeScrollRestorationCache(storage, originalSetItem, () => ({})) + scrollRestorationCache?.set(() => ({})) } catch { try { originalSetItem.call(storage, STORAGE_KEY, '{}') } catch { // last resort: session may be full or private-mode broken } + } finally { + recoveryDepth.current -= 1 } } @@ -85,6 +114,8 @@ export function installScrollRestorationGuard( } const originalSetItem = storage.setItem const isRealSessionStorage = typeof window !== 'undefined' && storage === window.sessionStorage + const recoveryDepth = { current: 0 } + const unpatchScrollCache = isRealSessionStorage ? patchScrollRestorationCacheSet(storage) : () => {} const wrappedSetItem = (key: string, value: string): void => { try { @@ -97,9 +128,9 @@ export function installScrollRestorationGuard( } let trimmed: string - let prunedState: Record + let prunedState: ScrollCacheState try { - const parsed = JSON.parse(value) as Record + const parsed = JSON.parse(value) as ScrollCacheState const keys = Object.keys(parsed) const keepKeys = keys.length > TARGET_ENTRIES_AFTER_PRUNE ? keys.slice(-TARGET_ENTRIES_AFTER_PRUNE) @@ -110,21 +141,22 @@ export function installScrollRestorationGuard( } trimmed = JSON.stringify(prunedState) } catch { - hardResetScrollRestorationPersistedState(storage, originalSetItem, isRealSessionStorage) + hardResetScrollRestorationPersistedState(storage, originalSetItem, isRealSessionStorage, recoveryDepth) return } try { originalSetItem.call(storage, key, trimmed) if (isRealSessionStorage) { - writeScrollRestorationCache(storage, originalSetItem, (() => prunedState) as ScrollCacheUpdater) + scrollRestorationCache?.set((() => prunedState) as ScrollCacheUpdater) } } catch { - hardResetScrollRestorationPersistedState(storage, originalSetItem, isRealSessionStorage) + hardResetScrollRestorationPersistedState(storage, originalSetItem, isRealSessionStorage, recoveryDepth) } } storage.setItem = wrappedSetItem guarded[GUARD_MARKER] = true return () => { + unpatchScrollCache() if (storage.setItem === wrappedSetItem) { storage.setItem = originalSetItem delete guarded[GUARD_MARKER] From da7066b933c078ec2f1026c7837d4832d4911427 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 28 May 2026 04:00:11 +0100 Subject: [PATCH 2/3] test(web): add scroll quota repro tooling and legacy regression test Document how #716 was verified: Vitest positive repro of the #707 unwrap race, plus Playwright dev harnesses for live A/B stress (same pattern as scripts/dev/read-hapi-web.mjs from #544). Co-authored-by: Cursor --- scripts/dev/scroll-quota-ab-compare.mjs | 118 ++++++++ scripts/dev/scroll-quota-prove-positive.mjs | 193 ++++++++++++++ scripts/dev/scroll-quota-repro-playwright.mjs | 251 ++++++++++++++++++ ...rollStorageGuard.legacy-regression.test.ts | 101 +++++++ 4 files changed, 663 insertions(+) create mode 100644 scripts/dev/scroll-quota-ab-compare.mjs create mode 100644 scripts/dev/scroll-quota-prove-positive.mjs create mode 100644 scripts/dev/scroll-quota-repro-playwright.mjs create mode 100644 web/src/lib/scrollStorageGuard.legacy-regression.test.ts diff --git a/scripts/dev/scroll-quota-ab-compare.mjs b/scripts/dev/scroll-quota-ab-compare.mjs new file mode 100644 index 000000000..947e46e53 --- /dev/null +++ b/scripts/dev/scroll-quota-ab-compare.mjs @@ -0,0 +1,118 @@ +#!/usr/bin/env node +/** + * A/B compare scroll quota stress: fixed vs upstream-main (broken #707 unwrap). + * + * Usage: + * node scripts/dev/scroll-quota-ab-compare.mjs --session + * + * Env: + * HAPI_ACCESS_TOKEN — CLI token for auth + * HAPI_URL_FIXED — default http://127.0.0.1:3006 (PR #717 worktree) + * HAPI_URL_BROKEN — default http://127.0.0.1:3007 (upstream/main hub) + */ +import { spawnSync } from 'node:child_process' +import { resolve, dirname } from 'node:path' +import { fileURLToPath } from 'node:url' +import { writeFileSync, mkdirSync } from 'node:fs' + +const __dir = dirname(fileURLToPath(import.meta.url)) +const repro = resolve(__dir, 'scroll-quota-repro-playwright.mjs') + +function parseArgs(argv) { + const args = { sessionId: '', fillMb: 4.5, scrollRoutes: 250, navRounds: 12 } + for (let i = 0; i < argv.length; i += 1) { + const arg = argv[i] + if (arg === '--session') args.sessionId = argv[++i] + else if (arg === '--fill-mb') args.fillMb = Number(argv[++i]) + else if (arg === '--scroll-routes') args.scrollRoutes = Number(argv[++i]) + else if (arg === '--nav-rounds') args.navRounds = Number(argv[++i]) + } + return args +} + +const args = parseArgs(process.argv.slice(2)) +const token = process.env.HAPI_ACCESS_TOKEN ?? '' +const fixedUrl = process.env.HAPI_URL_FIXED ?? 'http://127.0.0.1:3006' +const brokenUrl = process.env.HAPI_URL_BROKEN ?? 'http://127.0.0.1:3007' + +function runVariant(label, baseUrl) { + const env = { + ...process.env, + HAPI_URL: baseUrl, + HAPI_ACCESS_TOKEN: token, + } + const proc = spawnSync( + process.execPath, + [ + repro, + '--session', args.sessionId, + '--fill-mb', String(args.fillMb), + '--scroll-routes', String(args.scrollRoutes), + '--nav-rounds', String(args.navRounds), + ], + { env, encoding: 'utf8', maxBuffer: 20 * 1024 * 1024 }, + ) + let parsed = null + try { + const jsonStart = proc.stdout.lastIndexOf('{') + parsed = JSON.parse(proc.stdout.slice(jsonStart)) + } catch { + parsed = { ok: false, parseError: true, stdout: proc.stdout.slice(-2000), stderr: proc.stderr } + } + return { label, baseUrl, exitCode: proc.status ?? 1, result: parsed } +} + +if (!args.sessionId) { + console.error('Usage: scroll-quota-ab-compare.mjs --session ') + process.exit(2) +} + +console.log('Running A/B scroll quota stress...') +console.log(` FIXED → ${fixedUrl}`) +console.log(` BROKEN → ${brokenUrl}`) +console.log('') + +const broken = runVariant('upstream-main (#707 unwrap)', brokenUrl) +const fixed = runVariant('PR #717 (patched cache.set)', fixedUrl) + +const summary = { + sessionId: args.sessionId, + params: args, + broken, + fixed, + regressionDetected: Boolean(broken.result?.ok === false && (broken.result?.quotaErrors?.length || broken.result?.pageErrors?.length)), + fixVerified: Boolean(fixed.result?.ok === true && !(fixed.result?.quotaErrors?.length)), +} + +mkdirSync(resolve('localdocs/playwright-runs'), { recursive: true }) +const out = resolve('localdocs/playwright-runs', `scroll-quota-ab-${Date.now()}.json`) +writeFileSync(out, JSON.stringify(summary, null, 2)) + +console.log(JSON.stringify({ + broken: { + ok: broken.result?.ok, + quotaErrors: broken.result?.quotaErrors, + pageErrors: broken.result?.pageErrors, + scrollKeyAfter: broken.result?.scrollKeyAfter, + exitCode: broken.exitCode, + }, + fixed: { + ok: fixed.result?.ok, + quotaErrors: fixed.result?.quotaErrors, + pageErrors: fixed.result?.pageErrors, + scrollKeyAfter: fixed.result?.scrollKeyAfter, + exitCode: fixed.exitCode, + }, + regressionDetected: summary.regressionDetected, + fixVerified: summary.fixVerified, + report: out, +}, null, 2)) + +if (!summary.regressionDetected) { + console.error('\nNOTE: upstream-main did NOT fail this harness — may need harsher race trigger or pre-#707 tree.') + process.exitCode = 3 +} else if (!summary.fixVerified) { + process.exitCode = 2 +} else { + process.exitCode = 0 +} diff --git a/scripts/dev/scroll-quota-prove-positive.mjs b/scripts/dev/scroll-quota-prove-positive.mjs new file mode 100644 index 000000000..5cbc40cf1 --- /dev/null +++ b/scripts/dev/scroll-quota-prove-positive.mjs @@ -0,0 +1,193 @@ +#!/usr/bin/env node +/** + * Positive repro: prove upstream scroll guard fails under quota while fixed code survives. + * Runs async scroll-key writes after filling sessionStorage (TanStack-like timing). + */ +import { chromium } from 'playwright' +import { mkdirSync, writeFileSync } from 'node:fs' +import { resolve } from 'node:path' + +const SCROLL_KEY = 'tsr-scroll-restoration-v1_3' +const BASE_URL = process.env.HAPI_URL ?? 'http://127.0.0.1:3007' +const ACCESS_TOKEN = process.env.HAPI_ACCESS_TOKEN ?? '' +const LABEL = process.env.HAPI_VARIANT ?? 'unknown' +const OUT_DIR = resolve('localdocs/playwright-runs') + +function parseArgs(argv) { + const args = { sessionId: '', fillMb: 5.0, routes: 200 } + for (let i = 0; i < argv.length; i += 1) { + if (argv[i] === '--session') args.sessionId = argv[++i] + else if (argv[i] === '--fill-mb') args.fillMb = Number(argv[++i]) + else if (argv[i] === '--routes') args.routes = Number(argv[++i]) + } + return args +} + +const args = parseArgs(process.argv.slice(2)) +mkdirSync(OUT_DIR, { recursive: true }) +const outJson = resolve(OUT_DIR, `scroll-quota-positive-${LABEL}-${Date.now()}.json`) + +const browser = await chromium.launch({ + headless: true, + executablePath: process.env.PLAYWRIGHT_CHROME_PATH ?? '/usr/bin/google-chrome', + args: ['--no-sandbox', '--disable-setuid-sandbox', '--disable-dev-shm-usage'], +}) +const context = await browser.newContext() +if (ACCESS_TOKEN) { + await context.addInitScript(({ token, baseUrl }) => { + localStorage.setItem(`hapi_access_token::${baseUrl}`, token) + }, { token: ACCESS_TOKEN, baseUrl: BASE_URL }) +} + +const page = await context.newPage() +const pageErrors = [] +const consoleErrors = [] +page.on('pageerror', (err) => pageErrors.push(String(err))) +page.on('console', (msg) => { + if (msg.type() === 'error') consoleErrors.push(msg.text()) +}) + +const path = args.sessionId ? `/sessions/${args.sessionId}` : '/sessions' +await page.goto(`${BASE_URL}${path}`, { waitUntil: 'domcontentloaded', timeout: 60000 }) +await page.waitForTimeout(2500) + +const guardInstalled = await page.evaluate(() => Boolean(sessionStorage.__hapiScrollRestorationGuard)) + +// Fill storage until quota, leaving scroll key for last-mile pressure +const fill = await page.evaluate((fillMb) => { + const chunk = 'y'.repeat(256 * 1024) + const target = fillMb * 1024 * 1024 + let added = 0 + let i = 0 + const errors = [] + while (added < target) { + try { + sessionStorage.setItem(`__fill_${i}`, chunk) + added += chunk.length + i += 1 + } catch (e) { + errors.push(String(e)) + break + } + } + return { keys: i, mb: added / (1024 * 1024), errors } +}, args.fillMb) + +function buildPayload(routeCount) { + const state = {} + for (let i = 0; i < routeCount; i += 1) { + state[`/sessions/stress-${i}`] = { + window: { scrollX: 0, scrollY: i * 19 }, + 'main:nth-child(1)': { scrollX: 0, scrollY: i * 13 }, + } + } + return JSON.stringify(state) +} + +// Phase A: TanStack-like async scroll persists +const asyncWrites = await page.evaluate(({ scrollKey, routes }) => { + const payload = (() => { + const state = {} + for (let i = 0; i < routes; i += 1) { + state[`/sessions/stress-${i}`] = { + window: { scrollX: 0, scrollY: i * 19 }, + 'main:nth-child(1)': { scrollX: 0, scrollY: i * 13 }, + } + } + return JSON.stringify(state) + })() + + return new Promise((resolve) => { + const result = { scheduled: 0, syncThrows: [] } + window.__scrollQuotaPositive = { pageErrors: [] } + const onErr = (ev) => { + result.pageErrors = result.pageErrors ?? [] + result.pageErrors.push(String(ev.error ?? ev.message)) + } + window.addEventListener('error', onErr) + + for (let n = 0; n < 25; n += 1) { + result.scheduled += 1 + setTimeout(() => { + try { + sessionStorage.setItem(scrollKey, payload) + } catch (e) { + result.syncThrows.push(String(e)) + } + }, n * 8) + } + + setTimeout(() => { + window.removeEventListener('error', onErr) + resolve(result) + }, 400) + }) +}, { scrollKey: SCROLL_KEY, routes: args.routes }) + +await page.waitForTimeout(300) + +// Phase B: explicit #707 unwrap-window simulation (positive control for broken pattern) +const unwrapRace = await page.evaluate((scrollKey) => { + return new Promise((resolve) => { + const windowErrors = [] + const onErr = (ev) => windowErrors.push(String(ev.error ?? ev.message)) + window.addEventListener('error', onErr) + + const wrapped = sessionStorage.setItem + const native = Storage.prototype.setItem.bind(sessionStorage) + sessionStorage.setItem = native + + for (let i = 0; i < 12; i += 1) { + setTimeout(() => { + try { + const state = {} + for (let r = 0; r < 120; r += 1) { + state[`/race/${i}/${r}`] = { window: { scrollX: 0, scrollY: r } } + } + sessionStorage.setItem(scrollKey, JSON.stringify(state)) + } catch { + // sync catch — window 'error' is what we care about (TanStack async path) + } + }, i * 5) + } + + setTimeout(() => { + sessionStorage.setItem = wrapped + window.removeEventListener('error', onErr) + resolve({ windowErrors }) + }, 250) + }) +}, SCROLL_KEY) + +await page.waitForTimeout(200) + +const scrollKeyAfter = await page.evaluate((scrollKey) => { + const v = sessionStorage.getItem(scrollKey) + return v ? { bytes: v.length, routes: Object.keys(JSON.parse(v)).length } : { bytes: 0, routes: 0 } +}, SCROLL_KEY) + +const quotaSignals = [ + ...pageErrors.filter((t) => /QuotaExceeded|tsr-scroll-restoration/i.test(t)), + ...consoleErrors.filter((t) => /QuotaExceeded|tsr-scroll-restoration/i.test(t)), + ...(asyncWrites.pageErrors ?? []).filter((t) => /QuotaExceeded|tsr-scroll-restoration/i.test(t)), + ...(unwrapRace.windowErrors ?? []).filter((t) => /QuotaExceeded|tsr-scroll-restoration/i.test(t)), +] + +const result = { + label: LABEL, + url: BASE_URL, + guardInstalled, + fill, + asyncWrites, + unwrapRace, + scrollKeyAfter, + pageErrors, + quotaSignals, + bugReproduced: quotaSignals.length > 0, + survived: quotaSignals.length === 0, +} + +writeFileSync(outJson, JSON.stringify(result, null, 2)) +console.log(JSON.stringify(result, null, 2)) +await browser.close() +process.exitCode = result.bugReproduced ? 0 : 1 diff --git a/scripts/dev/scroll-quota-repro-playwright.mjs b/scripts/dev/scroll-quota-repro-playwright.mjs new file mode 100644 index 000000000..eff1fe5b1 --- /dev/null +++ b/scripts/dev/scroll-quota-repro-playwright.mjs @@ -0,0 +1,251 @@ +#!/usr/bin/env node +/** + * Stress-test TanStack scroll restoration against live HAPI. + * Fills sessionStorage toward quota, seeds oversized scroll map, scrolls + navigates. + * + * Usage: + * HAPI_URL=http://127.0.0.1:3006 HAPI_ACCESS_TOKEN=... \ + * node scripts/dev/scroll-quota-repro-playwright.mjs --session --fill-mb 4.5 --scroll-routes 200 + */ +import { chromium } from 'playwright' +import { mkdirSync, writeFileSync } from 'node:fs' +import { resolve } from 'node:path' + +const BASE_URL = process.env.HAPI_URL ?? 'https://hapi.tail9944ee.ts.net' +const ACCESS_TOKEN = process.env.HAPI_ACCESS_TOKEN ?? '' +const OUT_DIR = resolve('localdocs/playwright-runs') +const SCROLL_KEY = 'tsr-scroll-restoration-v1_3' + +function parseArgs(argv) { + const args = { sessionId: '', fillMb: 4.5, scrollRoutes: 200, timeout: 60000, navRounds: 6 } + for (let i = 0; i < argv.length; i += 1) { + const arg = argv[i] + if (arg === '--session') args.sessionId = argv[++i] + else if (arg === '--fill-mb') args.fillMb = Number(argv[++i]) + else if (arg === '--scroll-routes') args.scrollRoutes = Number(argv[++i]) + else if (arg === '--timeout') args.timeout = Number(argv[++i]) + else if (arg === '--nav-rounds') args.navRounds = Number(argv[++i]) + } + return args +} + +const args = parseArgs(process.argv.slice(2)) +mkdirSync(OUT_DIR, { recursive: true }) +const stamp = Date.now() +const outJson = resolve(OUT_DIR, `scroll-quota-${stamp}.json`) +const outPng = resolve(OUT_DIR, `scroll-quota-${stamp}.png`) + +const browser = await chromium.launch({ + headless: true, + executablePath: process.env.PLAYWRIGHT_CHROME_PATH ?? '/usr/bin/google-chrome', + args: ['--no-sandbox', '--disable-setuid-sandbox', '--disable-dev-shm-usage'], +}) +const context = await browser.newContext({ viewport: { width: 1440, height: 1100 } }) + +if (ACCESS_TOKEN) { + await context.addInitScript(({ token, baseUrl }) => { + localStorage.setItem(`hapi_access_token::${baseUrl}`, token) + }, { token: ACCESS_TOKEN, baseUrl: BASE_URL }) +} + +const page = await context.newPage() +const consoleMessages = [] +const pageErrors = [] + +page.on('console', (msg) => { + consoleMessages.push({ type: msg.type(), text: msg.text() }) +}) +page.on('pageerror', (err) => { + pageErrors.push(String(err)) +}) + +const targetPath = args.sessionId ? `/sessions/${args.sessionId}` : '/sessions' +const url = `${BASE_URL}${targetPath}` + +try { + await page.goto(url, { waitUntil: 'domcontentloaded', timeout: args.timeout }) + await page.waitForTimeout(3000) + + const guardInstalled = await page.evaluate(() => { + return Boolean(window.sessionStorage.__hapiScrollRestorationGuard) + }) + + const signedIn = await page.evaluate(() => { + return !document.body.innerText.includes('Sign In') + }) + + // Seed oversized scroll restoration map (simulates long session history) + const scrollSeed = await page.evaluate(({ scrollRoutes, scrollKey }) => { + const state = {} + for (let i = 0; i < scrollRoutes; i += 1) { + state[`/sessions/route-${i}`] = { + window: { scrollX: 0, scrollY: i * 17 }, + 'html:nth-child(1)': { scrollX: 0, scrollY: i * 13 }, + } + } + const json = JSON.stringify(state) + let threw = false + let error = '' + try { + window.sessionStorage.setItem(scrollKey, json) + } catch (e) { + threw = true + error = String(e) + } + const stored = window.sessionStorage.getItem(scrollKey) + return { + threw, + error, + requestedRoutes: scrollRoutes, + storedBytes: stored ? stored.length : 0, + storedRoutes: stored ? Object.keys(JSON.parse(stored)).length : 0, + } + }, { scrollRoutes: args.scrollRoutes, scrollKey: SCROLL_KEY }) + + // Prefill sessionStorage toward quota with unrelated keys + const fillResult = await page.evaluate((fillMb) => { + const chunk = 'x'.repeat(1024 * 256) + const targetBytes = fillMb * 1024 * 1024 + let added = 0 + let i = 0 + const errors = [] + while (added < targetBytes) { + try { + sessionStorage.setItem(`__hapi_fill_${i}`, chunk) + added += chunk.length + i += 1 + } catch (e) { + errors.push(String(e)) + break + } + } + return { keys: i, approxMb: added / (1024 * 1024), errors } + }, args.fillMb) + + // Scroll + navigate to trigger TanStack scrollRestorationCache.set writes + const stressResult = await page.evaluate(async ({ navRounds, sessionId }) => { + const sleep = (ms) => new Promise((r) => setTimeout(r, ms)) + const scrollables = [ + document.scrollingElement, + ...document.querySelectorAll('[data-scroll-restoration-id], .overflow-y-auto, [class*="overflow-y"], main'), + ].filter(Boolean) + + const paths = sessionId + ? [`/sessions/${sessionId}`, '/sessions', `/sessions/${sessionId}?tab=files`, `/sessions/${sessionId}`] + : ['/sessions', '/settings', '/sessions'] + + let navErrors = [] + for (let round = 0; round < navRounds; round += 1) { + for (const el of scrollables) { + if (el instanceof Element) { + el.scrollTop = (round + 1) * 500 + el.scrollLeft = 0 + } + } + window.scrollTo(0, (round + 1) * 400) + await sleep(120) + + const path = paths[round % paths.length] + try { + history.pushState({}, '', path) + window.dispatchEvent(new PopStateEvent('popstate')) + } catch (e) { + navErrors.push(String(e)) + } + await sleep(120) + } + await sleep(600) + return { navErrors, scrollableCount: scrollables.length } + }, { navRounds: args.navRounds, sessionId: args.sessionId }) + + await page.waitForTimeout(1000) + + // Burst scroll-key writes near quota — reproduces #716 unwrap race on upstream #707. + const burstResult = await page.evaluate(async (scrollKey) => { + const buildState = (n) => { + const state = {} + for (let i = 0; i < n; i += 1) { + state[`/burst/${i}`] = { + window: { scrollX: 0, scrollY: i * 11 }, + 'main:nth-child(1)': { scrollX: 0, scrollY: i * 7 }, + } + } + return JSON.stringify(state) + } + const caughtErrors = [] + for (let round = 0; round < 40; round += 1) { + try { + sessionStorage.setItem(scrollKey, buildState(80 + round)) + } catch (e) { + caughtErrors.push(String(e)) + } + await new Promise((r) => setTimeout(r, 5)) + } + return { caughtErrors: caughtErrors.slice(0, 5), caughtCount: caughtErrors.length } + }, SCROLL_KEY) + + await page.waitForTimeout(500) + + const quotaErrors = [ + ...consoleMessages.filter((m) => /QuotaExceededError|tsr-scroll-restoration/.test(m.text)), + ...pageErrors.filter((t) => /QuotaExceededError|tsr-scroll-restoration/.test(t)), + ] + + const scrollKeyAfter = await page.evaluate((scrollKey) => { + const v = window.sessionStorage.getItem(scrollKey) + if (!v) return { bytes: 0, routes: 0 } + try { + return { bytes: v.length, routes: Object.keys(JSON.parse(v)).length } + } catch { + return { bytes: v.length, routes: -1 } + } + }, SCROLL_KEY) + + const errorBoundaryVisible = await page.evaluate(() => { + const text = document.body.innerText + return /Something went wrong|error boundary|QuotaExceededError/i.test(text) + }) + + await page.screenshot({ path: outPng, fullPage: true }) + + const bodySnippet = await page.locator('body').innerText().catch(() => '') + + const result = { + ok: quotaErrors.length === 0 && !errorBoundaryVisible, + url, + signedIn, + guardInstalled, + scrollSeed, + fillResult, + stressResult, + burstResult, + scrollKeyAfter, + quotaErrors, + pageErrors, + errorBoundaryVisible, + relevantConsole: consoleMessages.filter((m) => + /QuotaExceeded|scroll|error/i.test(m.text) + ).slice(-40), + bodySnippet: bodySnippet.slice(0, 800), + screenshot: outPng, + } + + writeFileSync(outJson, JSON.stringify(result, null, 2)) + console.log(JSON.stringify(result, null, 2)) + process.exitCode = result.ok ? 0 : 2 +} catch (error) { + await page.screenshot({ path: outPng, fullPage: true }).catch(() => {}) + const fail = { + ok: false, + error: error instanceof Error ? error.message : String(error), + url, + consoleMessages: consoleMessages.slice(-20), + pageErrors, + screenshot: outPng, + } + writeFileSync(outJson, JSON.stringify(fail, null, 2)) + console.error(JSON.stringify(fail, null, 2)) + process.exitCode = 1 +} finally { + await browser.close() +} diff --git a/web/src/lib/scrollStorageGuard.legacy-regression.test.ts b/web/src/lib/scrollStorageGuard.legacy-regression.test.ts new file mode 100644 index 000000000..b1bc700d1 --- /dev/null +++ b/web/src/lib/scrollStorageGuard.legacy-regression.test.ts @@ -0,0 +1,101 @@ +/** + * Positive regression: legacy #707 unwrap bypasses the guard on quota failures. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const STORAGE_KEY = 'tsr-scroll-restoration-v1_3' + +class QuotaExceededError extends Error { + constructor() { + super('quota') + this.name = 'QuotaExceededError' + } +} + +function legacyWriteScrollRestorationCache( + storage: Storage, + originalSetItem: Storage['setItem'], + scrollCacheSet: (updater: (state: Record) => Record) => void, +): void { + const guardedSetItem = storage.setItem + storage.setItem = originalSetItem + try { + scrollCacheSet(() => ({})) + } finally { + storage.setItem = guardedSetItem + } +} + +function makeMockStorage(): Storage & { _store: Record; _setItem: ReturnType } { + const store: Record = {} + const setItem = vi.fn((key: string, value: string) => { store[key] = value }) + const storage = { + setItem, + getItem: (key: string) => store[key] ?? null, + removeItem: vi.fn((key: string) => { delete store[key] }), + clear: vi.fn(() => { for (const k of Object.keys(store)) delete store[k] }), + key: () => null, + length: 0, + } as unknown as Storage & { _store: Record; _setItem: ReturnType } + storage._store = store + storage._setItem = setItem + return storage +} + +describe('legacy #707 unwrap race (positive repro)', () => { + let storage: ReturnType + let guardedSetItem: Storage['setItem'] + let originalSetItem: Storage['setItem'] + + beforeEach(() => { + storage = makeMockStorage() + originalSetItem = storage._setItem + guardedSetItem = vi.fn((key: string, value: string) => { + try { + originalSetItem.call(storage, key, value) + } catch (err) { + if (key === STORAGE_KEY) { + return + } + throw err + } + }) as Storage['setItem'] + storage.setItem = guardedSetItem + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('legacy unwrap throws QuotaExceededError through native setItem (positive repro)', () => { + originalSetItem.mockImplementation((key: string) => { + if (key === STORAGE_KEY) { + throw new QuotaExceededError() + } + }) + + const tanStackPersist = (updater: (state: Record) => Record) => { + const next = updater({ stale: { window: { scrollX: 0, scrollY: 1 } } }) + storage.setItem(STORAGE_KEY, JSON.stringify(next)) + } + + expect(() => legacyWriteScrollRestorationCache(storage, originalSetItem, tanStackPersist)).toThrow(QuotaExceededError) + expect(guardedSetItem).not.toHaveBeenCalled() + }) + + it('fixed pattern routes cache persist through guarded setItem and survives quota', () => { + originalSetItem.mockImplementation((key: string) => { + if (key === STORAGE_KEY) { + throw new QuotaExceededError() + } + }) + + const tanStackPersist = (updater: (state: Record) => Record) => { + const next = updater({ stale: { window: { scrollX: 0, scrollY: 1 } } }) + storage.setItem(STORAGE_KEY, JSON.stringify(next)) + } + + expect(() => tanStackPersist(() => ({}))).not.toThrow() + expect(guardedSetItem).toHaveBeenCalled() + }) +}) From 6984aca56a57c577073d0c9b637686337b8eb085 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 28 May 2026 04:39:57 +0100 Subject: [PATCH 3/3] fix(web): address PR #717 review findings and CI typecheck Use originalSetItem in nested recovery to avoid re-entering the guard, fix legacy-regression test types, default repro script to localhost, and parse A/B harness stdout correctly. Co-authored-by: Cursor --- scripts/dev/scroll-quota-ab-compare.mjs | 12 +++++++----- scripts/dev/scroll-quota-repro-playwright.mjs | 2 +- ...scrollStorageGuard.legacy-regression.test.ts | 15 +++++++++------ web/src/lib/scrollStorageGuard.test.ts | 17 +++++++++++++++++ web/src/lib/scrollStorageGuard.ts | 2 +- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/scripts/dev/scroll-quota-ab-compare.mjs b/scripts/dev/scroll-quota-ab-compare.mjs index 947e46e53..97a953135 100644 --- a/scripts/dev/scroll-quota-ab-compare.mjs +++ b/scripts/dev/scroll-quota-ab-compare.mjs @@ -7,8 +7,8 @@ * * Env: * HAPI_ACCESS_TOKEN — CLI token for auth - * HAPI_URL_FIXED — default http://127.0.0.1:3006 (PR #717 worktree) - * HAPI_URL_BROKEN — default http://127.0.0.1:3007 (upstream/main hub) + * HAPI_URL_FIXED — default http://127.0.0.1:3006 (fixed build) + * HAPI_URL_BROKEN — default http://127.0.0.1:3007 (upstream/main build) */ import { spawnSync } from 'node:child_process' import { resolve, dirname } from 'node:path' @@ -54,8 +54,10 @@ function runVariant(label, baseUrl) { ) let parsed = null try { - const jsonStart = proc.stdout.lastIndexOf('{') - parsed = JSON.parse(proc.stdout.slice(jsonStart)) + const stdout = proc.stdout.trim() + parsed = stdout + ? JSON.parse(stdout) + : { ok: false, parseError: true, stdout: '', stderr: proc.stderr } } catch { parsed = { ok: false, parseError: true, stdout: proc.stdout.slice(-2000), stderr: proc.stderr } } @@ -72,8 +74,8 @@ console.log(` FIXED → ${fixedUrl}`) console.log(` BROKEN → ${brokenUrl}`) console.log('') +const fixed = runVariant('fixed (patched cache.set)', fixedUrl) const broken = runVariant('upstream-main (#707 unwrap)', brokenUrl) -const fixed = runVariant('PR #717 (patched cache.set)', fixedUrl) const summary = { sessionId: args.sessionId, diff --git a/scripts/dev/scroll-quota-repro-playwright.mjs b/scripts/dev/scroll-quota-repro-playwright.mjs index eff1fe5b1..251ea6907 100644 --- a/scripts/dev/scroll-quota-repro-playwright.mjs +++ b/scripts/dev/scroll-quota-repro-playwright.mjs @@ -11,7 +11,7 @@ import { chromium } from 'playwright' import { mkdirSync, writeFileSync } from 'node:fs' import { resolve } from 'node:path' -const BASE_URL = process.env.HAPI_URL ?? 'https://hapi.tail9944ee.ts.net' +const BASE_URL = process.env.HAPI_URL ?? 'http://127.0.0.1:3006' const ACCESS_TOKEN = process.env.HAPI_ACCESS_TOKEN ?? '' const OUT_DIR = resolve('localdocs/playwright-runs') const SCROLL_KEY = 'tsr-scroll-restoration-v1_3' diff --git a/web/src/lib/scrollStorageGuard.legacy-regression.test.ts b/web/src/lib/scrollStorageGuard.legacy-regression.test.ts index b1bc700d1..f317ed8ef 100644 --- a/web/src/lib/scrollStorageGuard.legacy-regression.test.ts +++ b/web/src/lib/scrollStorageGuard.legacy-regression.test.ts @@ -45,14 +45,13 @@ function makeMockStorage(): Storage & { _store: Record; _setItem describe('legacy #707 unwrap race (positive repro)', () => { let storage: ReturnType let guardedSetItem: Storage['setItem'] - let originalSetItem: Storage['setItem'] beforeEach(() => { storage = makeMockStorage() - originalSetItem = storage._setItem + const nativeSetItem = storage._setItem as unknown as (key: string, value: string) => void guardedSetItem = vi.fn((key: string, value: string) => { try { - originalSetItem.call(storage, key, value) + nativeSetItem(key, value) } catch (err) { if (key === STORAGE_KEY) { return @@ -68,7 +67,7 @@ describe('legacy #707 unwrap race (positive repro)', () => { }) it('legacy unwrap throws QuotaExceededError through native setItem (positive repro)', () => { - originalSetItem.mockImplementation((key: string) => { + storage._setItem.mockImplementation((key: string) => { if (key === STORAGE_KEY) { throw new QuotaExceededError() } @@ -79,12 +78,16 @@ describe('legacy #707 unwrap race (positive repro)', () => { storage.setItem(STORAGE_KEY, JSON.stringify(next)) } - expect(() => legacyWriteScrollRestorationCache(storage, originalSetItem, tanStackPersist)).toThrow(QuotaExceededError) + expect(() => legacyWriteScrollRestorationCache( + storage, + storage._setItem as Storage['setItem'], + tanStackPersist, + )).toThrow(QuotaExceededError) expect(guardedSetItem).not.toHaveBeenCalled() }) it('fixed pattern routes cache persist through guarded setItem and survives quota', () => { - originalSetItem.mockImplementation((key: string) => { + storage._setItem.mockImplementation((key: string) => { if (key === STORAGE_KEY) { throw new QuotaExceededError() } diff --git a/web/src/lib/scrollStorageGuard.test.ts b/web/src/lib/scrollStorageGuard.test.ts index 6abe909d1..fd2fe7a4d 100644 --- a/web/src/lib/scrollStorageGuard.test.ts +++ b/web/src/lib/scrollStorageGuard.test.ts @@ -258,6 +258,23 @@ describe('installScrollRestorationGuard', () => { off() }) + it('does not recurse when nested recovery fallback write also fails on real sessionStorage', () => { + const realSessionStorage = makeMockStorage() + vi.stubGlobal('window', { sessionStorage: realSessionStorage }) + + const off = installScrollRestorationGuard(realSessionStorage) + realSessionStorage._setItem.mockImplementation((key: string) => { + if (key === STORAGE_KEY) { + throw new QuotaExceededError() + } + }) + + expect(() => realSessionStorage.setItem(STORAGE_KEY, JSON.stringify(makeFullScrollState()))).not.toThrow() + expect(realSessionStorage._setItem.mock.calls.length).toBeLessThan(20) + + off() + }) + it('is idempotent — installing twice does not double-wrap', () => { const wrapped1 = storage.setItem const noop = installScrollRestorationGuard(storage) diff --git a/web/src/lib/scrollStorageGuard.ts b/web/src/lib/scrollStorageGuard.ts index d99f1026e..ffaffa4de 100644 --- a/web/src/lib/scrollStorageGuard.ts +++ b/web/src/lib/scrollStorageGuard.ts @@ -68,7 +68,7 @@ function hardResetScrollRestorationPersistedState( // write throws again. Clear the library cache so persisted size matches. if (recoveryDepth.current > 0) { try { - storage.setItem(STORAGE_KEY, '{}') + originalSetItem.call(storage, STORAGE_KEY, '{}') } catch { // nested recovery already in progress }