From b749fa90f23d187d5428de1a2d321cc6497b6667 Mon Sep 17 00:00:00 2001 From: Adam <2363879+adamdotdevin@users.noreply.github.com> Date: Mon, 9 Mar 2026 10:43:56 -0500 Subject: [PATCH] fix(app): scroll jitter/loop --- packages/app/src/pages/session.tsx | 78 ++--- .../src/pages/session/message-timeline.tsx | 11 +- .../app/src/pages/session/scroll-spy.test.ts | 127 -------- packages/app/src/pages/session/scroll-spy.ts | 275 ------------------ .../pages/session/use-session-hash-scroll.ts | 86 +++--- .../ui/src/components/scroll-view.test.ts | 19 ++ packages/ui/src/components/scroll-view.tsx | 36 ++- 7 files changed, 144 insertions(+), 488 deletions(-) delete mode 100644 packages/app/src/pages/session/scroll-spy.test.ts delete mode 100644 packages/app/src/pages/session/scroll-spy.ts create mode 100644 packages/ui/src/components/scroll-view.test.ts diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 35e89eabe..c1552ad02 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -37,7 +37,6 @@ import { createOpenReviewFile, createSizing } from "@/pages/session/helpers" import { MessageTimeline } from "@/pages/session/message-timeline" import { type DiffStyle, SessionReviewTab, type SessionReviewTabProps } from "@/pages/session/review-tab" import { resetSessionModel, syncSessionModel } from "@/pages/session/session-model-helpers" -import { createScrollSpy } from "@/pages/session/scroll-spy" import { SessionMobileTabs } from "@/pages/session/session-mobile-tabs" import { SessionSidePanel } from "@/pages/session/session-side-panel" import { TerminalPanel } from "@/pages/session/terminal-panel" @@ -486,20 +485,49 @@ export default function Page() { return "main" }) - const activeMessage = createMemo(() => { - if (!store.messageId) return lastUserMessage() - const found = visibleUserMessages()?.find((m) => m.id === store.messageId) - return found ?? lastUserMessage() - }) const setActiveMessage = (message: UserMessage | undefined) => { + messageMark = scrollMark setStore("messageId", message?.id) } + const anchor = (id: string) => `message-${id}` + + const cursor = () => { + const root = scroller + if (!root) return store.messageId + + const box = root.getBoundingClientRect() + const line = box.top + 100 + const list = [...root.querySelectorAll("[data-message-id]")] + .map((el) => { + const id = el.dataset.messageId + if (!id) return + + const rect = el.getBoundingClientRect() + return { id, top: rect.top, bottom: rect.bottom } + }) + .filter((item): item is { id: string; top: number; bottom: number } => !!item) + + const shown = list.filter((item) => item.bottom > box.top && item.top < box.bottom) + const hit = shown.find((item) => item.top <= line && item.bottom >= line) + if (hit) return hit.id + + const near = [...shown].sort((a, b) => { + const da = Math.abs(a.top - line) + const db = Math.abs(b.top - line) + if (da !== db) return da - db + return a.top - b.top + })[0] + if (near) return near.id + + return list.filter((item) => item.top <= line).at(-1)?.id ?? list[0]?.id ?? store.messageId + } + function navigateMessageByOffset(offset: number) { const msgs = visibleUserMessages() if (msgs.length === 0) return - const current = store.messageId + const current = store.messageId && messageMark === scrollMark ? store.messageId : cursor() const base = current ? msgs.findIndex((m) => m.id === current) : msgs.length const currentIndex = base === -1 ? msgs.length : base const targetIndex = currentIndex + offset @@ -572,6 +600,8 @@ export default function Page() { let dockHeight = 0 let scroller: HTMLDivElement | undefined let content: HTMLDivElement | undefined + let scrollMark = 0 + let messageMark = 0 const scrollGestureWindowMs = 250 @@ -616,6 +646,7 @@ export default function Page() { () => { setStore("messageId", undefined) setStore("changes", "session") + setUi("pendingMessage", undefined) }, { defer: true }, ), @@ -1110,12 +1141,6 @@ export default function Page() { let scrollStateFrame: number | undefined let scrollStateTarget: HTMLDivElement | undefined - const scrollSpy = createScrollSpy({ - onActive: (id) => { - if (id === store.messageId) return - setStore("messageId", id) - }, - }) const updateScrollState = (el: HTMLDivElement) => { const max = el.scrollHeight - el.clientHeight @@ -1163,31 +1188,21 @@ export default function Page() { ), ) - createEffect( - on( - sessionKey, - () => { - scrollSpy.clear() - }, - { defer: true }, - ), - ) - - const anchor = (id: string) => `message-${id}` - const setScrollRef = (el: HTMLDivElement | undefined) => { scroller = el autoScroll.scrollRef(el) - scrollSpy.setContainer(el) if (el) scheduleScrollState(el) } + const markUserScroll = () => { + scrollMark += 1 + } + createResizeObserver( () => content, () => { const el = scroller if (el) scheduleScrollState(el) - scrollSpy.markDirty() }, ) @@ -1220,7 +1235,6 @@ export default function Page() { if (stick) autoScroll.forceScrollToBottom() if (el) scheduleScrollState(el) - scrollSpy.markDirty() }, ) @@ -1248,7 +1262,6 @@ export default function Page() { onCleanup(() => { document.removeEventListener("keydown", handleKeyDown) - scrollSpy.destroy() if (reviewFrame !== undefined) cancelAnimationFrame(reviewFrame) if (scrollStateFrame !== undefined) cancelAnimationFrame(scrollStateFrame) }) @@ -1280,7 +1293,7 @@ export default function Page() {
- + diff --git a/packages/app/src/pages/session/message-timeline.tsx b/packages/app/src/pages/session/message-timeline.tsx index 4060e5e5c..6463e7cbb 100644 --- a/packages/app/src/pages/session/message-timeline.tsx +++ b/packages/app/src/pages/session/message-timeline.tsx @@ -193,8 +193,7 @@ export function MessageTimeline(props: { onAutoScrollHandleScroll: () => void onMarkScrollGesture: (target?: EventTarget | null) => void hasScrollGesture: () => boolean - isDesktop: boolean - onScrollSpyScroll: () => void + onUserScroll: () => void onTurnBackfillScroll: () => void onAutoScrollInteraction: (event: MouseEvent) => void centered: boolean @@ -205,8 +204,6 @@ export function MessageTimeline(props: { onLoadEarlier: () => void renderedUserMessages: UserMessage[] anchor: (id: string) => string - onRegisterMessage: (el: HTMLDivElement, id: string) => void - onUnregisterMessage: (id: string) => void }) { let touchGesture: number | undefined @@ -574,9 +571,9 @@ export function MessageTimeline(props: { props.onScheduleScrollState(e.currentTarget) props.onTurnBackfillScroll() if (!props.hasScrollGesture()) return + props.onUserScroll() props.onAutoScrollHandleScroll() props.onMarkScrollGesture(e.currentTarget) - if (props.isDesktop) props.onScrollSpyScroll() }} onClick={props.onAutoScrollInteraction} class="relative min-w-0 w-full h-full" @@ -763,10 +760,6 @@ export function MessageTimeline(props: {
{ - props.onRegisterMessage(el, messageID) - onCleanup(() => props.onUnregisterMessage(messageID)) - }} classList={{ "min-w-0 w-full max-w-full": true, "md:max-w-200 2xl:max-w-[1000px]": props.centered, diff --git a/packages/app/src/pages/session/scroll-spy.test.ts b/packages/app/src/pages/session/scroll-spy.test.ts deleted file mode 100644 index f3e6775cb..000000000 --- a/packages/app/src/pages/session/scroll-spy.test.ts +++ /dev/null @@ -1,127 +0,0 @@ -import { describe, expect, test } from "bun:test" -import { createScrollSpy, pickOffsetId, pickVisibleId } from "./scroll-spy" - -const rect = (top: number, height = 80): DOMRect => - ({ - x: 0, - y: top, - top, - left: 0, - right: 800, - bottom: top + height, - width: 800, - height, - toJSON: () => ({}), - }) as DOMRect - -const setRect = (el: Element, top: number, height = 80) => { - Object.defineProperty(el, "getBoundingClientRect", { - configurable: true, - value: () => rect(top, height), - }) -} - -describe("pickVisibleId", () => { - test("prefers higher intersection ratio", () => { - const id = pickVisibleId( - [ - { id: "a", ratio: 0.2, top: 100 }, - { id: "b", ratio: 0.8, top: 300 }, - ], - 120, - ) - - expect(id).toBe("b") - }) - - test("breaks ratio ties by nearest line", () => { - const id = pickVisibleId( - [ - { id: "a", ratio: 0.5, top: 90 }, - { id: "b", ratio: 0.5, top: 140 }, - ], - 130, - ) - - expect(id).toBe("b") - }) -}) - -describe("pickOffsetId", () => { - test("uses binary search cutoff", () => { - const id = pickOffsetId( - [ - { id: "a", top: 0 }, - { id: "b", top: 200 }, - { id: "c", top: 400 }, - ], - 350, - ) - - expect(id).toBe("b") - }) -}) - -describe("createScrollSpy fallback", () => { - test("tracks active id from offsets and dirty refresh", () => { - const active: string[] = [] - const root = document.createElement("div") as HTMLDivElement - const one = document.createElement("div") - const two = document.createElement("div") - const three = document.createElement("div") - - root.append(one, two, three) - document.body.append(root) - - Object.defineProperty(root, "scrollTop", { configurable: true, writable: true, value: 250 }) - setRect(root, 0, 800) - setRect(one, -250) - setRect(two, -50) - setRect(three, 150) - - const queue: FrameRequestCallback[] = [] - const flush = () => { - const run = [...queue] - queue.length = 0 - for (const cb of run) cb(0) - } - - const spy = createScrollSpy({ - onActive: (id) => active.push(id), - raf: (cb) => (queue.push(cb), queue.length), - caf: () => {}, - IntersectionObserver: undefined, - ResizeObserver: undefined, - MutationObserver: undefined, - }) - - spy.setContainer(root) - spy.register(one, "a") - spy.register(two, "b") - spy.register(three, "c") - spy.onScroll() - flush() - - expect(spy.getActiveId()).toBe("b") - expect(active.at(-1)).toBe("b") - - root.scrollTop = 450 - setRect(one, -450) - setRect(two, -250) - setRect(three, -50) - spy.onScroll() - flush() - expect(spy.getActiveId()).toBe("c") - - root.scrollTop = 250 - setRect(one, -250) - setRect(two, 250) - setRect(three, 150) - spy.markDirty() - spy.onScroll() - flush() - expect(spy.getActiveId()).toBe("a") - - spy.destroy() - }) -}) diff --git a/packages/app/src/pages/session/scroll-spy.ts b/packages/app/src/pages/session/scroll-spy.ts deleted file mode 100644 index 6ef4c844c..000000000 --- a/packages/app/src/pages/session/scroll-spy.ts +++ /dev/null @@ -1,275 +0,0 @@ -type Visible = { - id: string - ratio: number - top: number -} - -type Offset = { - id: string - top: number -} - -type Input = { - onActive: (id: string) => void - raf?: (cb: FrameRequestCallback) => number - caf?: (id: number) => void - IntersectionObserver?: typeof globalThis.IntersectionObserver - ResizeObserver?: typeof globalThis.ResizeObserver - MutationObserver?: typeof globalThis.MutationObserver -} - -export const pickVisibleId = (list: Visible[], line: number) => { - if (list.length === 0) return - - const sorted = [...list].sort((a, b) => { - if (b.ratio !== a.ratio) return b.ratio - a.ratio - - const da = Math.abs(a.top - line) - const db = Math.abs(b.top - line) - if (da !== db) return da - db - - return a.top - b.top - }) - - return sorted[0]?.id -} - -export const pickOffsetId = (list: Offset[], cutoff: number) => { - if (list.length === 0) return - - let lo = 0 - let hi = list.length - 1 - let out = 0 - - while (lo <= hi) { - const mid = (lo + hi) >> 1 - const top = list[mid]?.top - if (top === undefined) break - - if (top <= cutoff) { - out = mid - lo = mid + 1 - continue - } - - hi = mid - 1 - } - - return list[out]?.id -} - -export const createScrollSpy = (input: Input) => { - const raf = input.raf ?? requestAnimationFrame - const caf = input.caf ?? cancelAnimationFrame - const CtorIO = input.IntersectionObserver ?? globalThis.IntersectionObserver - const CtorRO = input.ResizeObserver ?? globalThis.ResizeObserver - const CtorMO = input.MutationObserver ?? globalThis.MutationObserver - - let root: HTMLDivElement | undefined - let io: IntersectionObserver | undefined - let ro: ResizeObserver | undefined - let mo: MutationObserver | undefined - let frame: number | undefined - let active: string | undefined - let dirty = true - - const node = new Map() - const id = new WeakMap() - const visible = new Map() - let offset: Offset[] = [] - - const schedule = () => { - if (frame !== undefined) return - frame = raf(() => { - frame = undefined - update() - }) - } - - const refreshOffset = () => { - const el = root - if (!el) { - offset = [] - dirty = false - return - } - - const base = el.getBoundingClientRect().top - offset = [...node].map(([next, item]) => ({ - id: next, - top: item.getBoundingClientRect().top - base + el.scrollTop, - })) - offset.sort((a, b) => a.top - b.top) - dirty = false - } - - const update = () => { - const el = root - if (!el) return - - const line = el.getBoundingClientRect().top + 100 - const next = - pickVisibleId( - [...visible].map(([k, v]) => ({ - id: k, - ratio: v.ratio, - top: v.top, - })), - line, - ) ?? - (() => { - if (dirty) refreshOffset() - return pickOffsetId(offset, el.scrollTop + 100) - })() - - if (!next || next === active) return - active = next - input.onActive(next) - } - - const observe = () => { - const el = root - if (!el) return - - io?.disconnect() - io = undefined - if (CtorIO) { - try { - io = new CtorIO( - (entries) => { - for (const entry of entries) { - const item = entry.target - if (!(item instanceof HTMLElement)) continue - const key = id.get(item) - if (!key) continue - - if (!entry.isIntersecting || entry.intersectionRatio <= 0) { - visible.delete(key) - continue - } - - visible.set(key, { - ratio: entry.intersectionRatio, - top: entry.boundingClientRect.top, - }) - } - - schedule() - }, - { - root: el, - threshold: [0, 0.25, 0.5, 0.75, 1], - }, - ) - } catch { - io = undefined - } - } - - if (io) { - for (const item of node.values()) io.observe(item) - } - - ro?.disconnect() - ro = undefined - if (CtorRO) { - ro = new CtorRO(() => { - dirty = true - schedule() - }) - ro.observe(el) - for (const item of node.values()) ro.observe(item) - } - - mo?.disconnect() - mo = undefined - if (CtorMO) { - mo = new CtorMO(() => { - dirty = true - schedule() - }) - mo.observe(el, { subtree: true, childList: true, characterData: true }) - } - - dirty = true - schedule() - } - - const setContainer = (el?: HTMLDivElement) => { - if (root === el) return - - root = el - visible.clear() - active = undefined - observe() - } - - const register = (el: HTMLElement, key: string) => { - const prev = node.get(key) - if (prev && prev !== el) { - io?.unobserve(prev) - ro?.unobserve(prev) - } - - node.set(key, el) - id.set(el, key) - if (io) io.observe(el) - if (ro) ro.observe(el) - dirty = true - schedule() - } - - const unregister = (key: string) => { - const item = node.get(key) - if (!item) return - - io?.unobserve(item) - ro?.unobserve(item) - node.delete(key) - visible.delete(key) - dirty = true - schedule() - } - - const markDirty = () => { - dirty = true - schedule() - } - - const clear = () => { - for (const item of node.values()) { - io?.unobserve(item) - ro?.unobserve(item) - } - - node.clear() - visible.clear() - offset = [] - active = undefined - dirty = true - } - - const destroy = () => { - if (frame !== undefined) caf(frame) - frame = undefined - clear() - io?.disconnect() - ro?.disconnect() - mo?.disconnect() - io = undefined - ro = undefined - mo = undefined - root = undefined - } - - return { - setContainer, - register, - unregister, - onScroll: schedule, - markDirty, - clear, - destroy, - getActiveId: () => active, - } -} diff --git a/packages/app/src/pages/session/use-session-hash-scroll.ts b/packages/app/src/pages/session/use-session-hash-scroll.ts index 20e88a3ea..1ea6a302b 100644 --- a/packages/app/src/pages/session/use-session-hash-scroll.ts +++ b/packages/app/src/pages/session/use-session-hash-scroll.ts @@ -1,6 +1,6 @@ import type { UserMessage } from "@opencode-ai/sdk/v2" import { useLocation, useNavigate } from "@solidjs/router" -import { createEffect, createMemo, onMount } from "solid-js" +import { createEffect, createMemo, onCleanup, onMount } from "solid-js" import { messageIdFromHash } from "./message-id-from-hash" export { messageIdFromHash } from "./message-id-from-hash" @@ -26,17 +26,38 @@ export const useSessionHashScroll = (input: { const messageById = createMemo(() => new Map(visibleUserMessages().map((m) => [m.id, m]))) const messageIndex = createMemo(() => new Map(visibleUserMessages().map((m, i) => [m.id, i]))) let pendingKey = "" + let clearing = false const location = useLocation() const navigate = useNavigate() + const frames = new Set() + const queue = (fn: () => void) => { + const id = requestAnimationFrame(() => { + frames.delete(id) + fn() + }) + frames.add(id) + } + const cancel = () => { + for (const id of frames) cancelAnimationFrame(id) + frames.clear() + } + const clearMessageHash = () => { + cancel() + input.consumePendingMessage(input.sessionKey()) + if (input.pendingMessage()) input.setPendingMessage(undefined) if (!location.hash) return + clearing = true navigate(location.pathname + location.search, { replace: true }) } const updateHash = (id: string) => { - navigate(location.pathname + location.search + `#${input.anchor(id)}`, { + const hash = `#${input.anchor(id)}` + if (location.hash === hash) return + clearing = false + navigate(location.pathname + location.search + hash, { replace: true, }) } @@ -54,51 +75,37 @@ export const useSessionHashScroll = (input: { return true } + const seek = (id: string, behavior: ScrollBehavior, left = 4): boolean => { + const el = document.getElementById(input.anchor(id)) + if (el) return scrollToElement(el, behavior) + if (left <= 0) return false + queue(() => { + seek(id, behavior, left - 1) + }) + return false + } + const scrollToMessage = (message: UserMessage, behavior: ScrollBehavior = "smooth") => { - console.log({ message, behavior }) + cancel() if (input.currentMessageId() !== message.id) input.setActiveMessage(message) const index = messageIndex().get(message.id) ?? -1 if (index !== -1 && index < input.turnStart()) { input.setTurnStart(index) - requestAnimationFrame(() => { - const el = document.getElementById(input.anchor(message.id)) - if (!el) { - requestAnimationFrame(() => { - const next = document.getElementById(input.anchor(message.id)) - if (!next) return - scrollToElement(next, behavior) - }) - return - } - scrollToElement(el, behavior) + queue(() => { + seek(message.id, behavior) }) updateHash(message.id) return } - const el = document.getElementById(input.anchor(message.id)) - if (!el) { - updateHash(message.id) - requestAnimationFrame(() => { - const next = document.getElementById(input.anchor(message.id)) - if (!next) return - if (!scrollToElement(next, behavior)) return - }) - return - } - if (scrollToElement(el, behavior)) { + if (seek(message.id, behavior)) { updateHash(message.id) return } - requestAnimationFrame(() => { - const next = document.getElementById(input.anchor(message.id)) - if (!next) return - if (!scrollToElement(next, behavior)) return - }) updateHash(message.id) } @@ -135,9 +142,11 @@ export const useSessionHashScroll = (input: { } createEffect(() => { - location.hash + const hash = location.hash + if (!hash) clearing = false if (!input.sessionID() || !input.messagesReady()) return - requestAnimationFrame(() => applyHash("auto")) + cancel() + queue(() => applyHash("auto")) }) createEffect(() => { @@ -159,16 +168,19 @@ export const useSessionHashScroll = (input: { } } - if (!targetId) targetId = messageIdFromHash(location.hash) + if (!targetId && !clearing) targetId = messageIdFromHash(location.hash) if (!targetId) return - if (input.currentMessageId() === targetId) return + const pending = input.pendingMessage() === targetId const msg = messageById().get(targetId) if (!msg) return - if (input.pendingMessage() === targetId) input.setPendingMessage(undefined) + if (pending) input.setPendingMessage(undefined) + if (input.currentMessageId() === targetId && !pending) return + input.autoScroll.pause() - requestAnimationFrame(() => scrollToMessage(msg, "auto")) + cancel() + queue(() => scrollToMessage(msg, "auto")) }) onMount(() => { @@ -177,6 +189,8 @@ export const useSessionHashScroll = (input: { } }) + onCleanup(cancel) + return { clearMessageHash, scrollToMessage, diff --git a/packages/ui/src/components/scroll-view.test.ts b/packages/ui/src/components/scroll-view.test.ts new file mode 100644 index 000000000..d28b51fea --- /dev/null +++ b/packages/ui/src/components/scroll-view.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, test } from "bun:test" +import { scrollKey } from "./scroll-view" + +describe("scrollKey", () => { + test("maps plain navigation keys", () => { + expect(scrollKey({ key: "PageDown", altKey: false, ctrlKey: false, metaKey: false, shiftKey: false })).toBe( + "page-down", + ) + expect(scrollKey({ key: "ArrowUp", altKey: false, ctrlKey: false, metaKey: false, shiftKey: false })).toBe("up") + }) + + test("ignores modified keybinds", () => { + expect( + scrollKey({ key: "ArrowDown", altKey: false, ctrlKey: false, metaKey: true, shiftKey: false }), + ).toBeUndefined() + expect(scrollKey({ key: "PageUp", altKey: false, ctrlKey: true, metaKey: false, shiftKey: false })).toBeUndefined() + expect(scrollKey({ key: "End", altKey: false, ctrlKey: false, metaKey: false, shiftKey: true })).toBeUndefined() + }) +}) diff --git a/packages/ui/src/components/scroll-view.tsx b/packages/ui/src/components/scroll-view.tsx index 52ed39a46..c3d878af6 100644 --- a/packages/ui/src/components/scroll-view.tsx +++ b/packages/ui/src/components/scroll-view.tsx @@ -6,6 +6,25 @@ export interface ScrollViewProps extends ComponentProps<"div"> { orientation?: "vertical" | "horizontal" // currently only vertical is fully implemented for thumb } +export const scrollKey = (event: Pick) => { + if (event.altKey || event.ctrlKey || event.metaKey || event.shiftKey) return + + switch (event.key) { + case "PageDown": + return "page-down" + case "PageUp": + return "page-up" + case "Home": + return "home" + case "End": + return "end" + case "ArrowUp": + return "up" + case "ArrowDown": + return "down" + } +} + export function ScrollView(props: ScrollViewProps) { const i18n = useI18n() const merged = mergeProps({ orientation: "vertical" }, props) @@ -133,31 +152,34 @@ export function ScrollView(props: ScrollViewProps) { return } + const next = scrollKey(e) + if (!next) return + const scrollAmount = viewportRef.clientHeight * 0.8 const lineAmount = 40 - switch (e.key) { - case "PageDown": + switch (next) { + case "page-down": e.preventDefault() viewportRef.scrollBy({ top: scrollAmount, behavior: "smooth" }) break - case "PageUp": + case "page-up": e.preventDefault() viewportRef.scrollBy({ top: -scrollAmount, behavior: "smooth" }) break - case "Home": + case "home": e.preventDefault() viewportRef.scrollTo({ top: 0, behavior: "smooth" }) break - case "End": + case "end": e.preventDefault() viewportRef.scrollTo({ top: viewportRef.scrollHeight, behavior: "smooth" }) break - case "ArrowUp": + case "up": e.preventDefault() viewportRef.scrollBy({ top: -lineAmount, behavior: "smooth" }) break - case "ArrowDown": + case "down": e.preventDefault() viewportRef.scrollBy({ top: lineAmount, behavior: "smooth" }) break