diff --git a/apps/web/src/components/ChatView.logic.test.ts b/apps/web/src/components/ChatView.logic.test.ts index a2897ce3ddb0..a2708de8bda3 100644 --- a/apps/web/src/components/ChatView.logic.test.ts +++ b/apps/web/src/components/ChatView.logic.test.ts @@ -12,7 +12,7 @@ import { TurnId, type WorktreeSetupSnapshot, } from "@t3tools/contracts"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; +import { afterEach, describe, expect, it, vi } from "vite-plus/test"; import { Atom, AsyncResult } from "effect/unstable/reactivity"; import { appAtomRegistry } from "../rpc/atomRegistry"; import { environmentThreadDetails } from "../state/threads"; @@ -86,7 +86,6 @@ import { shouldShowBranchMismatchBanner, shouldShowPlanFollowUpPrompt, shouldWriteThreadErrorToCurrentServerThread, - toolGroupConsumesUpwardNavigation, waitForRevertedMessage, prepareRevertedMessageAttachments, } from "./ChatView.logic"; @@ -446,134 +445,6 @@ describe("proactive panels", () => { }); }); -describe("toolGroupConsumesUpwardNavigation", () => { - class ScrollElement extends EventTarget { - scrollTop = 0; - scrollHeight = 100; - clientHeight = 100; - overflowY = "visible"; - - constructor( - readonly parentElement: ScrollElement | null = null, - readonly isToolGroup = false, - ) { - super(); - } - - closest(selector: string): ScrollElement | null { - if (selector !== "[data-tool-group-scroll]") return null; - return this.isToolGroup ? this : (this.parentElement?.closest(selector) ?? null); - } - } - - beforeEach(() => { - vi.stubGlobal("Element", ScrollElement); - vi.stubGlobal("getComputedStyle", (element: ScrollElement) => ({ - overflowY: element.overflowY, - })); - }); - afterEach(() => vi.unstubAllGlobals()); - - it("releases upward navigation when an overflowing group is at the top", () => { - const group = Object.assign(new ScrollElement(null, true), { - overflowY: "auto", - scrollHeight: 300, - }); - - expect(toolGroupConsumesUpwardNavigation(new ScrollElement(group))).toBe(false); - }); - - it.each([ - { overflowY: "auto", scrollTop: 1 }, - { overflowY: "auto", scrollTop: 0.25 }, - { overflowY: "scroll", scrollTop: 80 }, - ])("consumes upward navigation within a scrolled group: %j", (scroll) => { - const group = Object.assign(new ScrollElement(null, true), { - scrollHeight: 300, - ...scroll, - }); - - expect(toolGroupConsumesUpwardNavigation(group)).toBe(true); - }); - - it.each([100, 300])( - "consumes scrolling in a nested result with a group content height of %i", - (scrollHeight) => { - const group = Object.assign(new ScrollElement(null, true), { - overflowY: "auto", - scrollHeight, - }); - const result = Object.assign(new ScrollElement(group), { - overflowY: "auto", - scrollHeight: 300, - scrollTop: 0.25, - }); - - expect(toolGroupConsumesUpwardNavigation(new ScrollElement(result))).toBe(true); - }, - ); - - it("releases upward navigation when the group and nested result are both at the top", () => { - const group = Object.assign(new ScrollElement(null, true), { - overflowY: "auto", - scrollHeight: 300, - }); - const result = Object.assign(new ScrollElement(group), { - overflowY: "scroll", - scrollHeight: 300, - }); - - expect(toolGroupConsumesUpwardNavigation(new ScrollElement(result))).toBe(false); - }); - - it("ignores targets outside a tool group and non-element targets", () => { - const outside = Object.assign(new ScrollElement(), { - overflowY: "auto", - scrollHeight: 300, - scrollTop: 40, - }); - - expect(toolGroupConsumesUpwardNavigation(outside)).toBe(false); - expect(toolGroupConsumesUpwardNavigation(new EventTarget())).toBe(false); - expect(toolGroupConsumesUpwardNavigation(null)).toBe(false); - }); - - it("does not consume scrolling from an ancestor beyond the tool group", () => { - const timeline = Object.assign(new ScrollElement(), { - overflowY: "auto", - scrollHeight: 300, - scrollTop: 40, - }); - const group = new ScrollElement(timeline, true); - - expect(toolGroupConsumesUpwardNavigation(new ScrollElement(group))).toBe(false); - }); - - it.each(["hidden", "clip", "visible"])( - "ignores a non-scrollable child with overflow-y %s", - (overflowY) => { - const group = new ScrollElement(null, true); - const result = Object.assign(new ScrollElement(group), { - overflowY, - scrollHeight: 300, - scrollTop: 40, - }); - - expect(toolGroupConsumesUpwardNavigation(new ScrollElement(result))).toBe(false); - }, - ); - - it("does not consume programmatic scrolling on an overflow-hidden group", () => { - const group = Object.assign(new ScrollElement(null, true), { - overflowY: "hidden", - scrollHeight: 300, - scrollTop: 40, - }); - - expect(toolGroupConsumesUpwardNavigation(group)).toBe(false); - }); -}); - const environmentId = EnvironmentId.make("environment-local"); const projectId = ProjectId.make("project-1"); const threadId = ThreadId.make("thread-1"); diff --git a/apps/web/src/components/ChatView.logic.ts b/apps/web/src/components/ChatView.logic.ts index 928d99c549a6..528024de4393 100644 --- a/apps/web/src/components/ChatView.logic.ts +++ b/apps/web/src/components/ChatView.logic.ts @@ -239,22 +239,6 @@ export function shouldReleaseTimelineAnchorForToolActivity(input: { }); } -export function toolGroupConsumesUpwardNavigation(target: EventTarget | null): boolean { - const elementTarget = target instanceof Element ? target : null; - const group = elementTarget?.closest("[data-tool-group-scroll]"); - if (!group) return false; - - // A nested result or the group itself can consume an upward scroll. - for (let element = elementTarget; element; element = element.parentElement) { - if (element.scrollTop > 0) { - const overflowY = getComputedStyle(element).overflowY; - if (overflowY === "auto" || overflowY === "scroll") return true; - } - if (element === group) break; - } - return false; -} - export { findRecordedWorktreeSetup, resolveVisibleWorktreeSetup, diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index b97411e7a44b..3ce6566c041d 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -365,6 +365,7 @@ import { import { environmentShell } from "../state/shell"; import { ChatComposer, type ChatComposerHandle } from "./chat/ChatComposer"; import { createPageScrollController, type PageScrollKey } from "./chat/pageScrollController"; +import { isTimelineScrollTarget } from "./chat/timelineScrollTarget"; import { DraftHeroHeadline } from "./chat/DraftHeroHeadline"; import { ExpandedImageDialog } from "./chat/ExpandedImageDialog"; import { PullRequestThreadDialog } from "./PullRequestThreadDialog"; @@ -474,7 +475,6 @@ import { shouldWriteThreadErrorToCurrentServerThread, startNewThreadForProject, codexArtifactTemplatePromptToAppend, - toolGroupConsumesUpwardNavigation, waitForStartedServerThread, shouldRefocusComposerOnWindowFocus, } from "./ChatView.logic"; @@ -5466,6 +5466,8 @@ export default function ChatView(props: ChatViewProps) { // Only an upward wheel is a navigation intent; wheeling down while // following either does nothing (at the end) or moves toward it. const handleWheel = (event: WheelEvent) => { + if (event.ctrlKey || !isTimelineScrollTarget(event.target, scrollNode, event.deltaY)) + return; if (event.deltaY > 0) { timelineScrollIntentRef.current = "toward-end"; if (isAtEndRef.current) { @@ -5474,11 +5476,7 @@ export default function ChatView(props: ChatViewProps) { } else if (event.deltaY < 0) { timelineScrollIntentRef.current = "away-from-end"; } - if ( - event.deltaY < 0 && - contentScrollsUp() && - !toolGroupConsumesUpwardNavigation(event.target) - ) { + if (event.deltaY < 0 && contentScrollsUp()) { handleManualNavigation(); } }; @@ -5528,12 +5526,20 @@ export default function ChatView(props: ChatViewProps) { ) { return; } + if (!["PageUp", "Home", "ArrowUp", "PageDown", "End", "ArrowDown"].includes(event.key)) + return; + const scrollDirection = ["PageUp", "Home", "ArrowUp"].includes(event.key) ? -1 : 1; + if ( + scrollNode.contains(event.target) && + !isTimelineScrollTarget(event.target, scrollNode, scrollDirection) + ) + return; switch (event.key) { case "PageUp": case "Home": case "ArrowUp": timelineScrollIntentRef.current = "away-from-end"; - if (contentScrollsUp() && !toolGroupConsumesUpwardNavigation(event.target)) { + if (contentScrollsUp()) { handleManualNavigation(); composerRef.current?.collapseForTimelineScrollKey(event.key); } diff --git a/apps/web/src/components/chat/ChatComposer.tsx b/apps/web/src/components/chat/ChatComposer.tsx index e258c995e573..7955e1e02b25 100644 --- a/apps/web/src/components/chat/ChatComposer.tsx +++ b/apps/web/src/components/chat/ChatComposer.tsx @@ -310,6 +310,7 @@ import { import { ComposerPromptLengthValidation } from "./ComposerPromptLengthValidation"; import { PierreEntryIcon } from "./PierreEntryIcon"; import { pendingDraftWork } from "./pendingDraftWork"; +import { isTimelineScrollTarget } from "./timelineScrollTarget"; import { createComposerScrollGestureState, recordComposerScrollGestureEvent, @@ -4894,8 +4895,12 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) const scrollNode = getTimelineScrollableNode(); if (!scrollNode) return; - const targetsTimeline = scrollNode.contains(event.target); - if (!targetsTimeline && !composerScrollGestureRef.current.collapseSuppressed) return; + const targetsTimeline = isTimelineScrollTarget(event.target, scrollNode, event.deltaY); + if ( + !scrollNode.contains(event.target) && + !composerScrollGestureRef.current.collapseSuppressed + ) + return; if (composerScrollCollapseTimeoutRef.current !== null) { window.clearTimeout(composerScrollCollapseTimeoutRef.current); diff --git a/apps/web/src/components/chat/timelineScrollTarget.test.ts b/apps/web/src/components/chat/timelineScrollTarget.test.ts new file mode 100644 index 000000000000..051f7785f5a3 --- /dev/null +++ b/apps/web/src/components/chat/timelineScrollTarget.test.ts @@ -0,0 +1,142 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +import { + createComposerScrollGestureState, + recordComposerScrollGestureEvent, +} from "./composerScrollGesture"; +import { isTimelineScrollTarget } from "./timelineScrollTarget"; + +class ScrollElement extends EventTarget { + scrollTop = 0; + scrollHeight = 100; + clientHeight = 100; + overflowY = "visible"; + overscrollBehaviorY = "auto"; + + constructor(readonly parentElement: ScrollElement | null = null) { + super(); + } + + contains(target: ScrollElement): boolean { + return ( + target === this || (target.parentElement !== null && this.contains(target.parentElement)) + ); + } +} + +function targetsTimeline(target: EventTarget | null, timeline: ScrollElement, deltaY: number) { + return isTimelineScrollTarget(target, timeline as unknown as HTMLElement, deltaY); +} + +function setup() { + const timeline = Object.assign(new ScrollElement(), { + overflowY: "auto", + scrollHeight: 1500, + clientHeight: 500, + scrollTop: 1000, + }); + const group = Object.assign(new ScrollElement(timeline), { + overflowY: "auto", + scrollHeight: 300, + scrollTop: 80, + }); + return { timeline, group, content: new ScrollElement(group) }; +} + +beforeEach(() => { + vi.stubGlobal("Element", ScrollElement); + vi.stubGlobal("getComputedStyle", (element: ScrollElement) => element); +}); +afterEach(() => vi.unstubAllGlobals()); + +describe("timeline scroll targets", () => { + it.each([-30, 30])("keeps a nested tool group's scroll out of the timeline: %i", (deltaY) => { + const { timeline, group, content } = setup(); + expect(targetsTimeline(content, timeline, deltaY)).toBe(false); + expect(targetsTimeline(group, timeline, deltaY)).toBe(false); + }); + + it.each([ + { scrollTop: 0, deltaY: -30 }, + { scrollTop: 200, deltaY: 30 }, + ])("allows chaining only past the matching edge: %j", ({ scrollTop, deltaY }) => { + const { timeline, group, content } = setup(); + group.scrollTop = scrollTop; + expect(targetsTimeline(content, timeline, deltaY)).toBe(true); + expect(targetsTimeline(content, timeline, -deltaY)).toBe(false); + }); + + it.each(["contain", "none"])("respects overscroll-y %s at either edge", (overscrollBehaviorY) => { + const { timeline, group, content } = setup(); + group.overscrollBehaviorY = overscrollBehaviorY; + group.scrollTop = 0; + expect(targetsTimeline(content, timeline, -30)).toBe(false); + group.scrollTop = 200; + expect(targetsTimeline(content, timeline, 30)).toBe(false); + group.scrollTop = 0; + group.scrollHeight = group.clientHeight; + expect(targetsTimeline(content, timeline, 30)).toBe(false); + }); + + it("checks nested results even when the tool group cannot scroll", () => { + const { timeline, group } = setup(); + group.scrollTop = 0; + group.scrollHeight = group.clientHeight; + const result = Object.assign(new ScrollElement(group), { + overflowY: "scroll", + scrollHeight: 300, + scrollTop: 0.25, + }); + expect(targetsTimeline(new ScrollElement(result), timeline, -30)).toBe(false); + result.scrollTop = 0; + expect(targetsTimeline(result, timeline, -30)).toBe(true); + }); + + it("checks an outer group when an inner result reaches its edge", () => { + const { timeline, group } = setup(); + const result = Object.assign(new ScrollElement(group), { overflowY: "auto" }); + expect(targetsTimeline(result, timeline, -30)).toBe(false); + group.scrollTop = 0; + expect(targetsTimeline(result, timeline, -30)).toBe(true); + }); + + it.each(["visible", "hidden", "clip"])("ignores overflow-y %s", (overflowY) => { + const { timeline, group, content } = setup(); + group.overflowY = overflowY; + expect(targetsTimeline(content, timeline, -30)).toBe(true); + }); + + it("allows ordinary message content and the outer viewport", () => { + const { timeline } = setup(); + expect(targetsTimeline(new ScrollElement(timeline), timeline, -30)).toBe(true); + expect(targetsTimeline(timeline, timeline, 30)).toBe(true); + }); + + it("rejects outside targets, non-elements, and horizontal-only scrolling", () => { + const { timeline, content } = setup(); + expect(targetsTimeline(new ScrollElement(), timeline, -30)).toBe(false); + expect(targetsTimeline(new EventTarget(), timeline, -30)).toBe(false); + expect(targetsTimeline(null, timeline, -30)).toBe(false); + expect(targetsTimeline(content, timeline, 0)).toBe(false); + }); + + it("does not accumulate nested scrolling toward composer collapse", () => { + const { timeline, group, content } = setup(); + const state = createComposerScrollGestureState(); + const record = (target: ScrollElement, now: number, deltaPx: number) => + recordComposerScrollGestureEvent(state, { + now, + deltaPx, + collapseThresholdPx: 24, + collapseEligible: targetsTimeline(target, timeline, -deltaPx), + canScrollInGestureDirection: timeline.scrollTop > 0, + scrollsTowardLogicalEnd: false, + }); + + expect(record(timeline, 0, 20)).toBe(false); + expect(record(content, 20, 30)).toBe(false); + group.scrollTop = 0; + expect(record(content, 40, 10)).toBe(false); + expect(record(content, 60, 14)).toBe(true); + }); +}); diff --git a/apps/web/src/components/chat/timelineScrollTarget.ts b/apps/web/src/components/chat/timelineScrollTarget.ts new file mode 100644 index 000000000000..da87e9a7036d --- /dev/null +++ b/apps/web/src/components/chat/timelineScrollTarget.ts @@ -0,0 +1,31 @@ +// A gesture inside the timeline may belong to a nested tool result or code +// block. Only treat it as timeline navigation if it can chain to the outer list. +export function isTimelineScrollTarget( + target: EventTarget | null, + timeline: HTMLElement, + deltaY: number, +): boolean { + if (!(target instanceof Element) || !timeline.contains(target) || deltaY === 0) return false; + + for ( + let element: Element | null = target; + element && element !== timeline; + element = element.parentElement + ) { + const style = getComputedStyle(element); + if (style.overflowY !== "auto" && style.overflowY !== "scroll") continue; + + const canScroll = + deltaY < 0 + ? element.scrollTop > 0 + : element.scrollTop < element.scrollHeight - element.clientHeight; + if ( + canScroll || + style.overscrollBehaviorY === "contain" || + style.overscrollBehaviorY === "none" + ) { + return false; + } + } + return true; +}