From 21af445290057e9792d0a3f694c5897c5d93693b Mon Sep 17 00:00:00 2001 From: htjulia Date: Mon, 12 Oct 2026 11:22:04 +0900 Subject: [PATCH] fix(settings): pick up changes made in another tab Every pc: key is shared by all tabs of the console, but each tab read them once at mount and never listened for the storage event. Lowering retention in one tab left another pruning the same IndexedDB to the old limit until it was reloaded, so the tab with the higher limit kept counting rows the other had already evicted. - subscribeStored() listens for storage events on given keys (a null key is localStorage.clear()); events from sessionStorage are ignored - useSettings reloads and re-validates all four settings on any of their keys, and keeps the old object when nothing changed so effects keyed on settings do not restart - usePersistentState picks up its key the same way, which covers pc:metricsSort, and useTheme now builds on it for pc:theme. The OS preference is still queried only when nothing is stored - A changed limit takes effect from the next batch: the trace store reads its settings when it evicts, so an eviction already under way finishes on the limit it started with. A new retention-limit scenario pins that - 10 new tests; with the three hooks reverted, 5 of the 9 cross-tab tests fail (the other 4 are guards that pass either way) - Checked with two browser tabs: a write in one switched the other's theme and showed retention 120000 / factor 4x in its settings dialog without a reload - Gates: lint clean (same 7 warnings as main), 212 tests pass Tags: #settings #storage Co-Authored-By: htjulia --- src/hooks/cross-tab-settings.test.ts | 114 +++++++++++++++++++++++++++ src/hooks/retention-limit.test.ts | 21 +++++ src/hooks/use-persistent-state.ts | 40 +++++++++- src/hooks/use-settings.ts | 27 ++++++- src/hooks/use-theme.ts | 15 ++-- src/lib/settings.ts | 8 ++ 6 files changed, 213 insertions(+), 12 deletions(-) create mode 100644 src/hooks/cross-tab-settings.test.ts diff --git a/src/hooks/cross-tab-settings.test.ts b/src/hooks/cross-tab-settings.test.ts new file mode 100644 index 0000000..97a753e --- /dev/null +++ b/src/hooks/cross-tab-settings.test.ts @@ -0,0 +1,114 @@ +import { act, renderHook } from '@testing-library/react' +import { beforeEach, describe, expect, it } from 'vitest' +import { usePersistentState } from '@/hooks/use-persistent-state' +import { useSettings } from '@/hooks/use-settings' +import { useTheme } from '@/hooks/use-theme' +import { DEFAULT_SETTINGS, RETENTION_MIN } from '@/lib/settings' + +/** + * Every `pc:` key is shared by all tabs of the console, and each tab used to read it + * once at mount. Lowering retention in one tab left another pruning the same + * IndexedDB to the old limit until it was reloaded — the tab with the higher limit + * counting rows the other had already evicted. + * + * The browser tells the other tabs through a `storage` event, fired only in tabs + * other than the writer. These scenarios play that tab: write the key the way + * another tab would, then deliver the event the browser would deliver. + */ +function otherTab(key: string | null, value?: unknown, area: Storage = window.localStorage): void { + act(() => { + if (key === null) area.clear() + else area.setItem(key, JSON.stringify(value)) + window.dispatchEvent( + new StorageEvent('storage', { key, newValue: key === null ? null : JSON.stringify(value), storageArea: area }), + ) + }) +} + +beforeEach(() => { + window.localStorage.clear() + window.sessionStorage.clear() + document.documentElement.classList.remove('dark') +}) + +describe('settings changed in another tab', () => { + it('reach this tab without a reload', () => { + const { result } = renderHook(() => useSettings()) + expect(result.current.settings.retention).toBe(RETENTION_MIN) + + otherTab('pc:retention', 200_000) + + expect(result.current.settings.retention).toBe(200_000) + }) + + it('are validated on the way in, like a value read at mount', () => { + // Another tab is just another writer to storage; what it wrote is not trusted + // any more than a hand-edited entry. + const { result } = renderHook(() => useSettings()) + otherTab('pc:retention', 5) + expect(result.current.settings.retention).toBe(DEFAULT_SETTINGS.retention) + }) + + it('arrive whole, one key per event', () => { + // A save writes four keys; this tab may see the events before every write has + // landed. Each event re-reads everything, so the last one leaves the full set. + const { result } = renderHook(() => useSettings()) + otherTab('pc:retention', 80_000) + otherTab('pc:evictionFactor', 4) + otherTab('pc:pageSize', 500) + otherTab('pc:sampleDelayMs', 9_000) + expect(result.current.settings).toEqual({ retention: 80_000, evictionFactor: 4, pageSize: 500, sampleDelayMs: 9_000 }) + }) + + it('leave the settings object alone when nothing changed', () => { + // Consumers key effects on `settings`; a new identity for the same values would + // restart them (the stream's fallback timer among them) for no reason. + const { result } = renderHook(() => useSettings()) + const before = result.current.settings + otherTab('pc:theme', 'dark') + otherTab('pc:retention', RETENTION_MIN) // written, but to the value already held + expect(result.current.settings).toBe(before) + }) + + it('go back to defaults when another tab clears storage', () => { + window.localStorage.setItem('pc:retention', JSON.stringify(90_000)) + const { result } = renderHook(() => useSettings()) + expect(result.current.settings.retention).toBe(90_000) + + otherTab(null) + + expect(result.current.settings).toEqual(DEFAULT_SETTINGS) + }) + + it('ignore the same key in sessionStorage', () => { + const { result } = renderHook(() => useSettings()) + otherTab('pc:retention', 200_000, window.sessionStorage) + expect(result.current.settings.retention).toBe(RETENTION_MIN) + }) +}) + +describe('preferences changed in another tab', () => { + it('reach a persistent state hook', () => { + const { result } = renderHook(() => usePersistentState('pc:metricsSort', 'name')) + otherTab('pc:metricsSort', 'p95') + expect(result.current[0]).toBe('p95') + }) + + it('switch the theme, class and all', () => { + window.localStorage.setItem('pc:theme', JSON.stringify('light')) + const { result } = renderHook(() => useTheme()) + expect(result.current.theme).toBe('light') + + otherTab('pc:theme', 'dark') + + expect(result.current.theme).toBe('dark') + expect(document.documentElement.classList.contains('dark')).toBe(true) + }) + + it('stop arriving once the hook unmounts', () => { + const { result, unmount } = renderHook(() => usePersistentState('pc:metricsSort', 'name')) + unmount() + otherTab('pc:metricsSort', 'p95') + expect(result.current[0]).toBe('name') + }) +}) diff --git a/src/hooks/retention-limit.test.ts b/src/hooks/retention-limit.test.ts index f3145cb..b103f83 100644 --- a/src/hooks/retention-limit.test.ts +++ b/src/hooks/retention-limit.test.ts @@ -198,6 +198,27 @@ describe('the limit the console ships with', () => { expect(DEFAULT_SETTINGS.evictionFactor).toBe(1) }) + it('applies a limit changed mid-session from the next batch', async () => { + // How a save in another tab lands: `useSettings` hands the store new settings + // between batches. The store reads them when it evicts, so the next arrival is + // pruned to the new limit — not the one it connected with. + const hook = renderHook(({ s }) => useConsoleUnderTest(s), { initialProps: { s: withRetention(8) } }) + await act(async () => { + const es = FakeEventSource.current() + es.emit('open') + replay(es, GO_CONNECT) + }) + await waitFor(async () => expect(await db.countTraces()).toBe(ON_CONNECT)) + for (let n = 1; n <= 3; n++) await arrive(n) + expect(await db.countTraces()).toBe(8) + + hook.rerender({ s: withRetention(6) }) + await arrive(4) + + expect(await db.countTraces()).toBe(6) + await waitFor(() => expect(hook.result.current.store.total).toBe(6)) + }) + it('leaves a store well under the limit untouched', async () => { const { result } = await connected(DEFAULT_SETTINGS) await arrive(1) diff --git a/src/hooks/use-persistent-state.ts b/src/hooks/use-persistent-state.ts index c63a790..5793501 100644 --- a/src/hooks/use-persistent-state.ts +++ b/src/hooks/use-persistent-state.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' // All console-owned localStorage keys share the `pc:` prefix so they're easy to // spot and clear as a group. Callers pass the full key, e.g. 'pc:theme'. @@ -22,10 +22,38 @@ export function writeStored(key: string, value: T): void { } } +/** + * Call `onChange` when another tab changes one of `keys`. + * + * Every `pc:` key is shared by all tabs of the console, but each tab read it once at + * mount and never again — so a tab kept acting on a value the user had already + * changed elsewhere. For most keys that only looks stale; for retention it is two + * tabs pruning one IndexedDB to two different limits. + * + * The browser fires `storage` only in the *other* tabs, and only when a value + * actually changes, so writing a value back on receipt cannot echo. A `null` key is + * `localStorage.clear()`, which changed every key at once. + */ +export function subscribeStored(keys: readonly string[], onChange: () => void): () => void { + let area: Storage + try { + area = window.localStorage + } catch { + return () => {} // storage blocked — no other tab can be writing to it either + } + const handler = (e: StorageEvent) => { + if (e.storageArea !== area) return + if (e.key === null || keys.includes(e.key)) onChange() + } + window.addEventListener('storage', handler) + return () => window.removeEventListener('storage', handler) +} + /** * `useState` that mirrors its value to localStorage under `key` (JSON-encoded). * Initial value comes from storage when present, else `initial`. Writes are - * best-effort (private mode / quota failures are swallowed). + * best-effort (private mode / quota failures are swallowed). A change made in + * another tab is picked up as it happens. */ export function usePersistentState(key: string, initial: T): [T, (v: T | ((prev: T) => T)) => void] { const [value, setValue] = useState(() => readStored(key, initial)) @@ -34,6 +62,14 @@ export function usePersistentState(key: string, initial: T): [T, (v: T | ((pr writeStored(key, value) }, [key, value]) + // Read by the listener below, which must not be re-subscribed on every render. + const initialRef = useRef(initial) + useEffect(() => { + initialRef.current = initial + }) + + useEffect(() => subscribeStored([key], () => setValue(readStored(key, initialRef.current))), [key]) + const set = useCallback((v: T | ((prev: T) => T)) => { setValue((prev) => (typeof v === 'function' ? (v as (p: T) => T)(prev) : v)) }, []) diff --git a/src/hooks/use-settings.ts b/src/hooks/use-settings.ts index 999fc1b..b07372c 100644 --- a/src/hooks/use-settings.ts +++ b/src/hooks/use-settings.ts @@ -1,11 +1,19 @@ -import { useCallback, useState } from 'react' -import { loadSettings, saveSettings, type Settings } from '@/lib/settings' +import { useCallback, useEffect, useState } from 'react' +import { subscribeStored } from '@/hooks/use-persistent-state' +import { loadSettings, sameSettings, saveSettings, SETTINGS_KEYS, type Settings } from '@/lib/settings' /** - * User settings, read from localStorage once and written back on save. + * User settings, read from localStorage and written back on save. * * Loaded lazily so a blocked or hand-edited store cannot break the first render — * `loadSettings` validates every field and falls back to its default. + * + * A save in another tab is picked up as it happens. Read once at mount, two tabs + * holding different retention limits pruned the same IndexedDB to both: the tab + * with the higher limit kept counting rows the other had already evicted. The new + * value takes effect from the next decision that reads it — the trace store reads + * its settings when it evicts, so an eviction already under way finishes on the + * limit it started with and the next batch uses the new one. */ export function useSettings(): { settings: Settings; save: (next: Settings) => void } { const [settings, setSettings] = useState(() => loadSettings()) @@ -13,5 +21,18 @@ export function useSettings(): { settings: Settings; save: (next: Settings) => v saveSettings(next) setSettings(next) }, []) + + useEffect( + () => + subscribeStored(SETTINGS_KEYS, () => { + // A save writes four keys and arrives here as up to four events. Each reloads + // all four, so the tab converges on the last; one that changes nothing keeps + // the old identity, or every consumer keyed on `settings` would run again. + const next = loadSettings() + setSettings((prev) => (sameSettings(prev, next) ? prev : next)) + }), + [], + ) + return { settings, save } } diff --git a/src/hooks/use-theme.ts b/src/hooks/use-theme.ts index d307959..3c17f57 100644 --- a/src/hooks/use-theme.ts +++ b/src/hooks/use-theme.ts @@ -1,5 +1,5 @@ import { useEffect, useState } from 'react' -import { readStored, writeStored } from '@/hooks/use-persistent-state' +import { readStored, usePersistentState } from '@/hooks/use-persistent-state' type Theme = 'light' | 'dark' @@ -12,17 +12,18 @@ function prefersDark(): boolean { /** * Persisted light/dark theme. The choice is stored under `pc:theme` and is the * source of truth; the inline bootstrap in index.html applies it to - * before first paint (no flash), and this hook keeps React + storage in sync. - * Falls back to the OS preference when nothing is stored yet. + * before first paint (no flash), and this hook keeps React + storage in sync — + * including a toggle made in another tab. Falls back to the OS preference when + * nothing is stored yet. */ export function useTheme() { - const [theme, setTheme] = useState( - () => readStored(THEME_KEY, null) ?? (prefersDark() ? 'dark' : 'light'), - ) + // Resolved once, and the OS is asked only when nothing is stored — as before. If + // another tab removes the key, this tab keeps the theme it started with. + const [fallback] = useState(() => readStored(THEME_KEY, null) ?? (prefersDark() ? 'dark' : 'light')) + const [theme, setTheme] = usePersistentState(THEME_KEY, fallback) useEffect(() => { document.documentElement.classList.toggle('dark', theme === 'dark') - writeStored(THEME_KEY, theme) }, [theme]) return { diff --git a/src/lib/settings.ts b/src/lib/settings.ts index e4f4f8a..b2f1795 100644 --- a/src/lib/settings.ts +++ b/src/lib/settings.ts @@ -52,6 +52,14 @@ const KEYS = { sampleDelayMs: 'pc:sampleDelayMs', } as const +/** Every storage key a setting lives under — what another tab's change is matched against. */ +export const SETTINGS_KEYS: readonly string[] = Object.values(KEYS) + +/** Two settings say the same thing. */ +export function sameSettings(a: Settings, b: Settings): boolean { + return (Object.keys(KEYS) as (keyof Settings)[]).every((k) => a[k] === b[k]) +} + /** An integer within bounds, or `null` when the input cannot be one. */ function intIn(value: unknown, min: number, max: number): number | null { const n = typeof value === 'number' ? value : typeof value === 'string' ? Number(value) : NaN