From 8eb3c0ceba97c2e41abd5acf1e2e800ff8cdbe24 Mon Sep 17 00:00:00 2001 From: Luke Parker <10430890+Hona@users.noreply.github.com> Date: Wed, 4 Nov 2026 16:43:13 -0600 Subject: [PATCH] fix(app): flush pending scroll positions when the layout unmounts (#61805) --- .../src/shell/state/scroll-machine.test.ts | 109 +++++++++++++++ .../app/src/shell/state/scroll-machine.ts | 94 +++++++++++++ packages/app/src/shell/state/scroll.test.ts | 48 ++++--- packages/app/src/shell/state/scroll.ts | 127 ++++++++---------- 4 files changed, 292 insertions(+), 86 deletions(-) create mode 100644 packages/app/src/shell/state/scroll-machine.test.ts create mode 100644 packages/app/src/shell/state/scroll-machine.ts diff --git a/packages/app/src/shell/state/scroll-machine.test.ts b/packages/app/src/shell/state/scroll-machine.test.ts new file mode 100644 index 00000000000..68eb3c2c9bd --- /dev/null +++ b/packages/app/src/shell/state/scroll-machine.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, test } from "bun:test" +import { + INITIAL_SCROLL_PERSISTENCE_STATE, + transitionScrollPersistence, + type ScrollPersistenceEvent, + type ScrollPersistenceState, +} from "./scroll-machine" + +function replay(events: ScrollPersistenceEvent[], state: ScrollPersistenceState = INITIAL_SCROLL_PERSISTENCE_STATE) { + return events.reduce<{ state: ScrollPersistenceState; effects: unknown[][] }>( + (acc, event) => { + const result = transitionScrollPersistence(acc.state, event) + + return { state: result.state, effects: [...acc.effects, [...result.effects]] } + }, + { state, effects: [] }, + ) +} + +describe("scroll persistence", () => { + test("disposing writes the positions still waiting in the debounce", () => { + const result = replay([ + { type: "edited", sessionKey: "first" }, + { type: "edited", sessionKey: "second" }, + { type: "disposed" }, + ]) + + expect(result.effects.at(-1)).toEqual([ + { type: "timer.clear", sessionKeys: ["first", "second"] }, + { type: "flush", sessionKeys: ["first", "second"] }, + ]) + expect(result.state).toEqual({ status: "disposed" }) + }) + + test("further scroll frames in a debounce window keep the state and reschedule the write", () => { + const first = transitionScrollPersistence(INITIAL_SCROLL_PERSISTENCE_STATE, { type: "edited", sessionKey: "s" }) + const second = transitionScrollPersistence(first.state, { type: "edited", sessionKey: "s" }) + + expect(second.state).toBe(first.state) + expect(second.effects).toEqual([{ type: "timer.schedule", sessionKey: "s" }]) + }) + + test("a session writes once per debounce window", () => { + const result = replay([ + { type: "edited", sessionKey: "s" }, + { type: "flushed", sessionKey: "s" }, + { type: "flushed", sessionKey: "s" }, + ]) + + expect(result.effects.slice(1)).toEqual([ + [ + { type: "timer.clear", sessionKeys: ["s"] }, + { type: "flush", sessionKeys: ["s"] }, + ], + [], + ]) + expect(result.state).toBe(INITIAL_SCROLL_PERSISTENCE_STATE) + }) + + test("flushing all writes every waiting session at once", () => { + const result = replay([ + { type: "edited", sessionKey: "first" }, + { type: "edited", sessionKey: "second" }, + { type: "flush-all" }, + { type: "flush-all" }, + ]) + + expect(result.effects.slice(2)).toEqual([ + [ + { type: "timer.clear", sessionKeys: ["first", "second"] }, + { type: "flush", sessionKeys: ["first", "second"] }, + ], + [], + ]) + }) + + test("a dropped session discards its waiting write", () => { + const result = replay([ + { type: "edited", sessionKey: "dirty" }, + { type: "dropped", sessionKeys: ["dirty", "clean"] }, + { type: "dropped", sessionKeys: [] }, + ]) + + expect(result.effects.slice(1)).toEqual([ + [ + { type: "timer.clear", sessionKeys: ["dirty"] }, + { type: "cache.drop", sessionKeys: ["dirty", "clean"] }, + ], + [], + ]) + expect(result.state).toBe(INITIAL_SCROLL_PERSISTENCE_STATE) + }) + + test("a disposed store ignores later events", () => { + const disposed = replay([{ type: "disposed" }]).state + + expect( + replay( + [ + { type: "edited", sessionKey: "s" }, + { type: "flushed", sessionKey: "s" }, + { type: "flush-all" }, + { type: "disposed" }, + ], + disposed, + ), + ).toEqual({ state: disposed, effects: [[], [], [], []] }) + }) +}) diff --git a/packages/app/src/shell/state/scroll-machine.ts b/packages/app/src/shell/state/scroll-machine.ts new file mode 100644 index 00000000000..5f3be4f1116 --- /dev/null +++ b/packages/app/src/shell/state/scroll-machine.ts @@ -0,0 +1,94 @@ +// Session scroll positions wait in a debounce before they are written to the layout store. `dirty` holds the sessions +// with a write still waiting. +export type ScrollPersistenceState = + | { readonly status: "active"; readonly dirty: ReadonlySet } + | { readonly status: "disposed" } + +export type ScrollPersistenceEvent = + | { readonly type: "edited"; readonly sessionKey: string } + // The session's debounce ended, or something asked for its write now. + | { readonly type: "flushed"; readonly sessionKey: string } + | { readonly type: "flush-all" } + | { readonly type: "dropped"; readonly sessionKeys: readonly string[] } + | { readonly type: "disposed" } + +export type ScrollPersistenceEffect = + | { readonly type: "timer.schedule"; readonly sessionKey: string } + | { readonly type: "timer.clear"; readonly sessionKeys: readonly string[] } + | { readonly type: "flush"; readonly sessionKeys: readonly string[] } + | { readonly type: "cache.drop"; readonly sessionKeys: readonly string[] } + +export type ScrollPersistenceTransition = { + readonly state: ScrollPersistenceState + readonly effects: readonly ScrollPersistenceEffect[] +} + +export const INITIAL_SCROLL_PERSISTENCE_STATE: ScrollPersistenceState = Object.freeze({ + status: "active", + dirty: new Set(), +}) + +const DISPOSED_SCROLL_PERSISTENCE_STATE: ScrollPersistenceState = Object.freeze({ status: "disposed" }) + +const NO_EFFECTS: readonly ScrollPersistenceEffect[] = Object.freeze([]) + +export function transitionScrollPersistence( + state: ScrollPersistenceState, + event: ScrollPersistenceEvent, +): ScrollPersistenceTransition { + if (state.status === "disposed") return { state, effects: NO_EFFECTS } + + if (event.type === "edited") { + const effects = [{ type: "timer.schedule", sessionKey: event.sessionKey }] as const + + // Every scroll frame reschedules the debounce; only the first one in a window changes the state. + if (state.dirty.has(event.sessionKey)) return { state, effects } + + return { state: { status: "active", dirty: new Set(state.dirty).add(event.sessionKey) }, effects } + } + + // The last positions are written on the way out instead of waiting for a debounce that will never fire. + if (event.type === "disposed") return { state: DISPOSED_SCROLL_PERSISTENCE_STATE, effects: write([...state.dirty]) } + + if (event.type === "flushed") { + if (!state.dirty.has(event.sessionKey)) return { state, effects: NO_EFFECTS } + + return { state: without(state, [event.sessionKey]), effects: write([event.sessionKey]) } + } + + if (event.type === "flush-all") { + if (state.dirty.size === 0) return { state, effects: NO_EFFECTS } + + return { state: INITIAL_SCROLL_PERSISTENCE_STATE, effects: write([...state.dirty]) } + } + + if (event.sessionKeys.length === 0) return { state, effects: NO_EFFECTS } + const dirty = event.sessionKeys.filter((key) => state.dirty.has(key)) + + return { + state: dirty.length === 0 ? state : without(state, dirty), + effects: [ + ...(dirty.length === 0 ? [] : [{ type: "timer.clear" as const, sessionKeys: dirty }]), + { type: "cache.drop", sessionKeys: event.sessionKeys }, + ], + } +} + +export function isScrollPersistenceDisposed(state: ScrollPersistenceState) { + return state.status === "disposed" +} + +function write(sessionKeys: readonly string[]): readonly ScrollPersistenceEffect[] { + if (sessionKeys.length === 0) return NO_EFFECTS + + return [ + { type: "timer.clear", sessionKeys }, + { type: "flush", sessionKeys }, + ] +} + +function without(state: Extract, sessionKeys: readonly string[]) { + const dirty = new Set([...state.dirty].filter((key) => !sessionKeys.includes(key))) + + return dirty.size === 0 ? INITIAL_SCROLL_PERSISTENCE_STATE : { status: "active" as const, dirty } +} diff --git a/packages/app/src/shell/state/scroll.test.ts b/packages/app/src/shell/state/scroll.test.ts index 6c07e608224..f5849dd6e5c 100644 --- a/packages/app/src/shell/state/scroll.test.ts +++ b/packages/app/src/shell/state/scroll.test.ts @@ -1,24 +1,20 @@ import { describe, expect, test, vi } from "bun:test" -import { createScrollPersistence } from "./scroll" +import { createScrollPersistence, type SessionScroll } from "./scroll" describe("createScrollPersistence", () => { test("debounces persisted scroll writes", () => { vi.useFakeTimers() try { - const snapshot = { - session: { - review: { x: 0, y: 0 }, - }, - } as Record> + const snapshot = new Map>([["session", { review: { x: 0, y: 0 } }]]) - const writes: Array> = [] + const writes: Array> = [] const scroll = createScrollPersistence({ debounceMs: 10, - getSnapshot: (sessionKey) => snapshot[sessionKey], + getSnapshot: (sessionKey) => snapshot.get(sessionKey), onFlush: (sessionKey, next) => { - snapshot[sessionKey] = next + snapshot.set(sessionKey, next) writes.push(next) }, }) @@ -45,21 +41,41 @@ describe("createScrollPersistence", () => { } }) + test("disposing writes the last position instead of dropping its pending debounce", () => { + vi.useFakeTimers() + + try { + const writes: Array<[string, Record]> = [] + + const scroll = createScrollPersistence({ + debounceMs: 250, + getSnapshot: () => undefined, + onFlush: (sessionKey, next) => writes.push([sessionKey, next]), + }) + + scroll.setScroll("session", "review", { x: 0, y: 120 }) + scroll.dispose() + vi.advanceTimersByTime(250) + scroll.setScroll("session", "review", { x: 0, y: 240 }) + vi.advanceTimersByTime(250) + + expect(writes).toEqual([["session", { review: { x: 0, y: 120 } }]]) + } finally { + vi.useRealTimers() + } + }) + test("reseeds empty cache after persisted snapshot loads", () => { - const snapshot = { - session: {}, - } as Record> + const snapshot = new Map>([["session", {}]]) const scroll = createScrollPersistence({ - getSnapshot: (sessionKey) => snapshot[sessionKey], + getSnapshot: (sessionKey) => snapshot.get(sessionKey), onFlush: () => {}, }) expect(scroll.scroll("session", "review")).toBeUndefined() - snapshot.session = { - review: { x: 12, y: 34 }, - } + snapshot.set("session", { review: { x: 12, y: 34 } }) expect(scroll.scroll("session", "review")).toEqual({ x: 12, y: 34 }) scroll.dispose() diff --git a/packages/app/src/shell/state/scroll.ts b/packages/app/src/shell/state/scroll.ts index 110d8da6544..83ca5809fdf 100644 --- a/packages/app/src/shell/state/scroll.ts +++ b/packages/app/src/shell/state/scroll.ts @@ -1,4 +1,11 @@ import { createStore, produce } from "solid-js/store" +import { + INITIAL_SCROLL_PERSISTENCE_STATE, + isScrollPersistenceDisposed, + transitionScrollPersistence, + type ScrollPersistenceEffect, + type ScrollPersistenceEvent, +} from "./scroll-machine" export type SessionScroll = { x: number @@ -16,8 +23,8 @@ type Options = { export function createScrollPersistence(opts: Options) { const wait = opts.debounceMs ?? 200 const [cache, setCache] = createStore>({}) - const dirty = new Set() const timers = new Map>() + let persistence = INITIAL_SCROLL_PERSISTENCE_STATE function clone(input?: ScrollMap) { const out: ScrollMap = {} @@ -34,19 +41,55 @@ export function createScrollPersistence(opts: Options) { return out } - function seed(sessionKey: string) { - const next = clone(opts.getSnapshot(sessionKey)) - const current = cache[sessionKey] + function dispatch(event: ScrollPersistenceEvent) { + const result = transitionScrollPersistence(persistence, event) + persistence = result.state + result.effects.forEach(run) + } - if (!current) { - setCache(sessionKey, next) + function run(effect: ScrollPersistenceEffect) { + if (effect.type === "timer.schedule") { + clearTimeout(timers.get(effect.sessionKey)) + timers.set( + effect.sessionKey, + setTimeout(() => dispatch({ type: "flushed", sessionKey: effect.sessionKey }), wait), + ) return } - if (Object.keys(current).length > 0) return + if (effect.type === "timer.clear") { + for (const key of effect.sessionKeys) { + clearTimeout(timers.get(key)) + timers.delete(key) + } - if (Object.keys(next).length === 0) return + return + } + + if (effect.type === "flush") { + for (const key of effect.sessionKeys) opts.onFlush(key, clone(cache[key])) + + return + } + + setCache( + produce((draft) => { + for (const key of effect.sessionKeys) { + delete draft[key] + } + }), + ) + } + + function seed(sessionKey: string) { + const current = cache[sessionKey] + + // Scroll frames reach here first; a seeded session skips reading and copying its snapshot. + if (current && Object.keys(current).length > 0) return + const next = clone(opts.getSnapshot(sessionKey)) + + if (current && Object.keys(next).length === 0) return setCache(sessionKey, next) } @@ -56,17 +99,8 @@ export function createScrollPersistence(opts: Options) { return cache[sessionKey]?.[tab] ?? opts.getSnapshot(sessionKey)?.[tab] } - function schedule(sessionKey: string) { - const prev = timers.get(sessionKey) - - if (prev) clearTimeout(prev) - timers.set( - sessionKey, - setTimeout(() => flush(sessionKey), wait), - ) - } - function setScroll(sessionKey: string, tab: string, pos: SessionScroll) { + if (isScrollPersistenceDisposed(persistence)) return seed(sessionKey) const prev = cache[sessionKey]?.[tab] @@ -74,64 +108,17 @@ export function createScrollPersistence(opts: Options) { if (prev?.x === pos.x && prev?.y === pos.y) return setCache(sessionKey, tab, { x: pos.x, y: pos.y }) - dirty.add(sessionKey) - schedule(sessionKey) - } - - function flush(sessionKey: string) { - const timer = timers.get(sessionKey) - - if (timer) clearTimeout(timer) - timers.delete(sessionKey) - - if (!dirty.has(sessionKey)) return - dirty.delete(sessionKey) - - opts.onFlush(sessionKey, clone(cache[sessionKey])) - } - - function flushAll() { - const keys = Array.from(dirty) - - if (keys.length === 0) return - - for (const key of keys) { - flush(key) - } - } - - function drop(keys: string[]) { - if (keys.length === 0) return - - for (const key of keys) { - const timer = timers.get(key) - - if (timer) clearTimeout(timer) - timers.delete(key) - dirty.delete(key) - } - - setCache( - produce((draft) => { - for (const key of keys) { - delete draft[key] - } - }), - ) - } - - function dispose() { - drop(Array.from(timers.keys())) + dispatch({ type: "edited", sessionKey }) } return { cache, - drop, - flush, - flushAll, + drop: (sessionKeys: string[]) => dispatch({ type: "dropped", sessionKeys }), + flush: (sessionKey: string) => dispatch({ type: "flushed", sessionKey }), + flushAll: () => dispatch({ type: "flush-all" }), scroll, seed, setScroll, - dispose, + dispose: () => dispatch({ type: "disposed" }), } }