fix(app): flush pending scroll positions when the layout unmounts (#61805)

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: [[], [], [], []] })
+  })
+})

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<string> }
+  | { 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<string>(),
+})
+
+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<ScrollPersistenceState, { status: "active" }>, 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 }
+}

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<string, Record<string, { x: number; y: number }>>
+      const snapshot = new Map<string, Record<string, SessionScroll>>([["session", { review: { x: 0, y: 0 } }]])
 
-      const writes: Array<Record<string, { x: number; y: number }>> = []
+      const writes: Array<Record<string, SessionScroll>> = []
 
       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<string, SessionScroll>]> = []
+
+      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<string, Record<string, { x: number; y: number }>>
+    const snapshot = new Map<string, Record<string, SessionScroll>>([["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()

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<Record<string, ScrollMap>>({})
-  const dirty = new Set<string>()
   const timers = new Map<string, ReturnType<typeof setTimeout>>()
+  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" }),
   }
 }