Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions apps/web/src/components/ChatView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,11 @@ 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 {
createTimelineWheelLatch,
isTimelineScrollTarget,
latchTimelineWheelTarget,
} from "./chat/timelineScrollTarget";
import { DraftHeroHeadline } from "./chat/DraftHeroHeadline";
import { ExpandedImageDialog } from "./chat/ExpandedImageDialog";
import { PullRequestThreadDialog } from "./PullRequestThreadDialog";
Expand Down Expand Up @@ -5431,6 +5435,12 @@ export default function ChatView(props: ChatViewProps) {
timelineEntries,
timelineLiveFollowEnabled,
]);
// Outlives the listener effect below, which reattaches whenever the
// composer inset changes, possibly in the middle of a wheel run.
const wheelLatchRef = useRef(createTimelineWheelLatch());
useEffect(() => {
wheelLatchRef.current = createTimelineWheelLatch();
}, [activeThread?.id]);
useEffect(() => {
let removeListeners: (() => void) | null = null;
let frame: number | null = null;
Expand Down Expand Up @@ -5466,7 +5476,7 @@ 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))
if (event.ctrlKey || !latchTimelineWheelTarget(wheelLatchRef.current, event, scrollNode))
return;
if (event.deltaY > 0) {
timelineScrollIntentRef.current = "toward-end";
Expand Down
15 changes: 8 additions & 7 deletions apps/web/src/components/chat/ChatComposer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ import {
import { ComposerPromptLengthValidation } from "./ComposerPromptLengthValidation";
import { PierreEntryIcon } from "./PierreEntryIcon";
import { pendingDraftWork } from "./pendingDraftWork";
import { isTimelineScrollTarget } from "./timelineScrollTarget";
import { createTimelineWheelLatch, latchTimelineWheelTarget } from "./timelineScrollTarget";
import {
createComposerScrollGestureState,
recordComposerScrollGestureEvent,
Expand Down Expand Up @@ -4881,19 +4881,20 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps)
composerScrollCollapseTimeoutRef.current = null;
resetComposerScrollGesture(composerScrollGestureRef.current);
};
const wheelLatch = createTimelineWheelLatch();
const handleTimelineWheel = (event: WheelEvent) => {
if (event.ctrlKey || !(event.target instanceof Element)) {
return;
}

const scrollNode = getTimelineScrollableNode();
if (!scrollNode) return;
const targetsTimeline = isTimelineScrollTarget(event.target, scrollNode, event.deltaY);
if (
!scrollNode.contains(event.target) &&
!composerScrollGestureRef.current.collapseSuppressed
)
return;
// Only timeline events may start or extend a run, as in ChatView, whose
// listener sits on the timeline itself.
const insideTimeline = scrollNode.contains(event.target);
if (!insideTimeline && !composerScrollGestureRef.current.collapseSuppressed) return;
const targetsTimeline =
insideTimeline && latchTimelineWheelTarget(wheelLatch, event, scrollNode);

if (composerScrollCollapseTimeoutRef.current !== null) {
window.clearTimeout(composerScrollCollapseTimeoutRef.current);
Expand Down
67 changes: 62 additions & 5 deletions apps/web/src/components/chat/timelineScrollTarget.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ import {
createComposerScrollGestureState,
recordComposerScrollGestureEvent,
} from "./composerScrollGesture";
import { isTimelineScrollTarget } from "./timelineScrollTarget";
import {
TIMELINE_WHEEL_RUN_GAP_MS,
createTimelineWheelLatch,
isTimelineScrollTarget,
latchTimelineWheelTarget,
} from "./timelineScrollTarget";

class ScrollElement extends EventTarget {
scrollTop = 0;
Expand All @@ -28,6 +33,20 @@ function targetsTimeline(target: EventTarget | null, timeline: ScrollElement, de
return isTimelineScrollTarget(target, timeline as unknown as HTMLElement, deltaY);
}

function latchedTargetsTimeline(
latch: ReturnType<typeof createTimelineWheelLatch>,
target: EventTarget | null,
timeline: ScrollElement,
deltaY: number,
timeStamp: number,
) {
return latchTimelineWheelTarget(
latch,
{ target, deltaY, timeStamp },
timeline as unknown as HTMLElement,
);
}

function setup() {
const timeline = Object.assign(new ScrollElement(), {
overflowY: "auto",
Expand Down Expand Up @@ -122,21 +141,59 @@ describe("timeline scroll targets", () => {

it("does not accumulate nested scrolling toward composer collapse", () => {
const { timeline, group, content } = setup();
const latch = createTimelineWheelLatch();
const state = createComposerScrollGestureState();
const record = (target: ScrollElement, now: number, deltaPx: number) =>
recordComposerScrollGestureEvent(state, {
now,
deltaPx,
collapseThresholdPx: 24,
collapseEligible: targetsTimeline(target, timeline, -deltaPx),
collapseEligible: latchedTargetsTimeline(latch, target, timeline, -deltaPx, now),
canScrollInGestureDirection: timeline.scrollTop > 0,
scrollsTowardLogicalEnd: false,
});

expect(record(timeline, 0, 20)).toBe(false);
expect(record(content, 0, 30)).toBe(false);
group.scrollTop = 0;
expect(record(content, 20, 30)).toBe(false);
expect(record(content, 40, 30)).toBe(false);
expect(record(content, 40 + TIMELINE_WHEEL_RUN_GAP_MS + 1, 30)).toBe(true);
});
});

describe("timeline wheel runs", () => {
it("keeps a run on a nested group after the group reaches its edge", () => {
const { timeline, group, content } = setup();
const latch = createTimelineWheelLatch();

expect(latchedTargetsTimeline(latch, content, timeline, -100, 0)).toBe(false);
group.scrollTop = 0;
// A slow wheel still counts as one run while clicks keep arriving.
expect(latchedTargetsTimeline(latch, content, timeline, -100, 400)).toBe(false);
expect(latchedTargetsTimeline(latch, content, timeline, -100, 800)).toBe(false);
expect(
latchedTargetsTimeline(latch, content, timeline, -100, 800 + TIMELINE_WHEEL_RUN_GAP_MS + 1),
).toBe(true);
});

it("keeps a timeline run on the timeline when a nested group slides under the pointer", () => {
const { timeline, content } = setup();
const latch = createTimelineWheelLatch();

expect(latchedTargetsTimeline(latch, new ScrollElement(timeline), timeline, -100, 0)).toBe(
true,
);
expect(latchedTargetsTimeline(latch, content, timeline, -100, 30)).toBe(true);
});

it("does not let a horizontal-only event decide or end a run", () => {
const { timeline, group, content } = setup();
const latch = createTimelineWheelLatch();

expect(latchedTargetsTimeline(latch, content, timeline, 0, 0)).toBe(false);
group.scrollTop = 0;
expect(record(content, 40, 10)).toBe(false);
expect(record(content, 60, 14)).toBe(true);
expect(latchedTargetsTimeline(latch, content, timeline, -100, 10)).toBe(true);
expect(latchedTargetsTimeline(latch, content, timeline, 0, 20)).toBe(false);
expect(latchedTargetsTimeline(latch, content, timeline, -100, 30)).toBe(true);
});
});
31 changes: 31 additions & 0 deletions apps/web/src/components/chat/timelineScrollTarget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,34 @@ export function isTimelineScrollTarget(
}
return true;
}

// Chromium keeps a run of wheel events on the scroller the run started on: a
// nested group that reaches its edge mid-run swallows the rest of the run, and
// the timeline only moves on a run that starts at that edge. With OS wheel input
// in Edge, clicks 400 ms apart stayed on the group and 800 ms apart chained;
// Chromium's own wheel transaction timeout is 500 ms.
export const TIMELINE_WHEEL_RUN_GAP_MS = 500;

export type TimelineWheelLatch = {
targetsTimeline: boolean;
lastEventAt: number;
};

export function createTimelineWheelLatch(): TimelineWheelLatch {
return { targetsTimeline: false, lastEventAt: Number.NEGATIVE_INFINITY };
}

// Whether a wheel event scrolls the timeline, decided once per run by
// isTimelineScrollTarget on the run's first vertical event.
export function latchTimelineWheelTarget(
latch: TimelineWheelLatch,
event: Pick<WheelEvent, "target" | "deltaY" | "timeStamp">,
timeline: HTMLElement,
): boolean {
if (event.deltaY === 0) return false;
if (event.timeStamp - latch.lastEventAt > TIMELINE_WHEEL_RUN_GAP_MS) {
latch.targetsTimeline = isTimelineScrollTarget(event.target, timeline, event.deltaY);
}
latch.lastEventAt = event.timeStamp;
return latch.targetsTimeline;
}
Loading