Skip to content

Commit aa73295

Browse files
authored
fix(review): work around Safari scroll-reset bug with shadow DOM diffs (#540) (#547)
Safari's compositor resets scrollTop to 0 when momentum-scrolling ends inside a container whose child is a web-component shadow DOM (the @pierre/diffs `<diffs-container>`). The reset bypasses all JavaScript APIs — neither scrollTo nor the scrollTop setter fires. Add a WebKit-only scroll event listener that detects the bogus reset (scrollTop jumping from >200 to 0 in a single event, which cannot happen during normal scrolling) and synchronously restores the last known good position before the browser paints the wrong frame. Closes #540 For provenance purposes, this commit was AI assisted.
1 parent 8ee7c6b commit aa73295

1 file changed

Lines changed: 40 additions & 0 deletions

File tree

packages/review-editor/components/DiffViewer.tsx

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,46 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
323323
}
324324
}, [filePath, onLineSelection]);
325325

326+
// Safari scroll-position guardian. Safari has a compositor bug where
327+
// scrollTop resets to 0 (sometimes multiple times in quick succession)
328+
// when momentum-scrolling ends inside a container whose child is a
329+
// web-component shadow DOM (@pierre/diffs `<diffs-container>`). The reset
330+
// bypasses JavaScript entirely — no scrollTo / scrollTop setter fires.
331+
// Detect the bogus resets and restore the last known good position.
332+
// Only active in WebKit — Chrome / Firefox / Edge are unaffected.
333+
//
334+
// filePath is in the dep array so the guardian resets when the user
335+
// switches files (the file-switch useLayoutEffect legitimately scrolls
336+
// to 0 — without resetting here the guardian would fight it).
337+
useEffect(() => {
338+
if (!viewport) return;
339+
const ua = navigator.userAgent;
340+
const isWebKit = ua.includes('Safari') && !ua.includes('Chrome');
341+
if (!isWebKit) return;
342+
343+
let lastGoodST = 0;
344+
345+
const onScroll = () => {
346+
const st = viewport.scrollTop;
347+
if (st > 0) {
348+
lastGoodST = st;
349+
} else if (lastGoodST > 200) {
350+
// scrollTop jumped from a distant position to 0 — Safari compositor bug.
351+
// A legitimate scroll-to-top always has intermediate events that bring
352+
// lastGoodST down to a small value before reaching 0. A jump from >200
353+
// to 0 in a single event can only be the bug. Restore synchronously so
354+
// the browser never paints the wrong frame.
355+
viewport.scrollTop = lastGoodST;
356+
} else {
357+
// Near the top already (lastGoodST ≤ 200) — legitimate scroll to top
358+
lastGoodST = 0;
359+
}
360+
};
361+
362+
viewport.addEventListener('scroll', onScroll, { passive: true });
363+
return () => viewport.removeEventListener('scroll', onScroll);
364+
}, [viewport, filePath]);
365+
326366
// Scroll to selected annotation when it changes
327367
useEffect(() => {
328368
if (!selectedAnnotationId || !containerRef.current) return;

0 commit comments

Comments
 (0)