Arnike/paperclip
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ui): keep desktop shell pinned when scrollIntoView walks past body (#8071)

Browse Source 
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work
> - The desktop app shell uses a fixed-height `h-dvh` flex column with
`<body>` pinned to `overflow: hidden` so the page itself doesn't scroll
— only `#main-content` does
> - On long threads, submitting a comment fires `scrollIntoView({ block:
"start" })` on the new optimistic message, which by spec walks every
ancestor scroll container and adjusts each
> - With `<body>` (and historically the two shell `<div>`s) marked
`overflow: hidden`, the browser treats them as scroll containers per the
CSS spec; `scrollIntoView` reaches them and scrolls them, visually
shifting the entire shell — sidebar and header included — off the top of
the viewport
> - `overflow: clip` is visually identical to `hidden` but is explicitly
*not* a scroll container, so converting the shell divs and body to
`clip` removes them from `scrollIntoView`'s ancestor walk
> - Chrome additionally drives `document.scrollingElement.scrollTop` via
its internal C++ smooth-scroll algorithm, which bypasses both the JS
`scrollTop` setter and the CSS `overflow` on the root element — so even
with the root viewport clipped, `<html>` still scrolls
> - This PR layers the CSS/JS overflow-clip changes with a capture-phase
`scroll` listener that snaps `documentElement.scrollTop` and
`body.scrollTop` back to 0 whenever they drift, defeating the
root-viewport scroll the browser performs natively

## Linked Issues or Issue Description

Refs #8041 (`fix(ui): don't window-scroll the desktop shell on comment
submit`) — that PR gated the JS-initiated `window.scrollBy(...)` path in
`restoreComposerViewportSnapshot`. The bug this PR fixes is a separate,
browser-internal scroll path triggered by `scrollIntoView`'s ancestor
walk, which the #8041 gate doesn't reach (no JS scroll call to gate).
This PR is complementary, not a revert.

**Bug description (no existing tracked issue):** After submitting a
comment on a long thread in the desktop app, the entire app shell —
sidebar, top nav, everything — visually shifts upward by ~300px, putting
items above "Tasks" in the sidebar off-screen. A soft refresh (Cmd+R)
doesn't fix it; only a hard URL navigation does. Verified the bug
reproduces only on threads long enough that `#main-content.scrollHeight
> clientHeight` by a wide margin.

**Repro:** Open a long-thread issue → submit a one-word comment →
observe sidebar items above the active selection fall off the top of the
viewport.

## What Changed

- `ui/src/components/Layout.tsx`:
- Static desktop-shell `<div>`s: `overflow-hidden` → `overflow-clip` on
both the outer flex column and inner flex row (lines 459, 470 in the
original numbering). Removes them from the scroll-container set.
- JS-applied body overflow: `document.body.style.overflow = isMobile ?
"visible" : "hidden"` → `... : "clip"`. Same rationale, applied to
`<body>`.
- New `useEffect` that registers a capture-phase `scroll` listener on
`window`. On every scroll event, if `documentElement.scrollTop` or
`body.scrollTop` has drifted from 0, it gets snapped back. Gated on
`!isMobile` so the mobile shell (which uses `min-h-dvh` and
intentionally scrolls window) is unaffected.

## Verification

**Reviewer steps:**

1. Open a long-thread issue in the desktop UI (anything where
`#main-content.scrollHeight > clientHeight` substantially — most active
issues qualify).
2. Submit a one-word comment.
3. Confirm the sidebar stays put — items above the active item remain
visible, top nav stays at the top of the viewport.
4. Repeat several times; confirm the shell never drifts upward.

**Tests:** Existing tests for `issue-chat-scroll.ts` and
`IssueChatThread.tsx` still pass (59/59 in
`ui/src/lib/issue-chat-scroll.test.ts` +
`ui/src/components/IssueChatThread.test.tsx`).

**Bisect proof:** During development I confirmed by selectively
reverting the capture-phase scroll listener that the bug returns
immediately when only the listener is removed (with the two
`overflow-clip` changes still in place) — proving the listener is the
decisive fix for the browser-internal root-viewport scroll path. The
probe trace showed `documentElement.scrollTop` animating from 0 →
307.5px over ~400ms during the smooth `scrollIntoView` even with `html {
overflow: clip }` applied in computed style.

## Risks

Low.

- The two `overflow-hidden` → `overflow-clip` swaps are visually
identical and have well-defined browser support (Chrome 90+, Firefox
81+, Safari 16+). Paperclip's desktop targets are well within those
ranges.
- The capture-phase scroll listener fires only when
`documentElement.scrollTop !== 0` or `body.scrollTop !== 0`. On a
correctly-behaving page neither should ever be non-zero on desktop, so
the listener is effectively a no-op for everything except the bug it's
reverting. It's gated behind `!isMobile`.
- Mobile uses `min-h-dvh` (no overflow set) and intentionally scrolls
window for the standard mobile chrome behavior; the listener is gated
off there so mobile UX is unaffected.

## Model Used

Claude Opus 4.7 (`claude-opus-4-7`), 200k context, extended thinking
enabled. Tool use: file edits, bash for git/test execution. Diagnosis
was driven by an instrumented in-browser probe (built and HMR'd into
`Layout.tsx` during development, removed before this PR) that traced
every `scrollBy`/`scrollTo`/`scrollIntoView` call and the
`Element.prototype.scrollTop` setter; the decisive trace identified the
browser-internal scroll path as the unguarded root cause.

## Checklist

- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have run tests locally and they pass
- [ ] I have added or updated tests where applicable — no new tests; the
bug is a browser-internal scroll behavior that's not exercisable in
jsdom (which doesn't implement the smooth-scroll animation that drives
the bug). Existing snapshot/restore tests in `issue-chat-scroll.test.ts`
cover the JS-initiated paths.
- [ ] If this change affects the UI, I have included before/after
screenshots — see the description text above; the bug manifests as the
entire shell translating up by ~300px on long-thread comment submit
- [x] I have updated relevant documentation to reflect my changes — N/A,
behavior change is non-user-facing CSS/event-handling
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green — will confirm on this PR
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups —
will iterate
- [x] I will address all Greptile and reviewer comments before
requesting merge
This commit is contained in:
Devin Foley
2026-06-12 17:31:16 -07:00
committed by GitHub
parent cd1b4f275d
commit 412a04c964
3 changed files with 121 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/src/components/Layout.tsx
+15 -3
View File
@@ -36,6 +36,7 @@ import {
} from "../lib/navigation-scroll";
import { queryKeys } from "../lib/queryKeys";
import { scheduleMainContentFocus } from "../lib/main-content-focus";
import { pinDocumentScrollToZero } from "../lib/pin-document-scroll";
import { cn } from "../lib/utils";
import { NotFoundPage } from "../pages/NotFound";
import { PluginSlotMount, resolveRouteSidebarSlot, usePluginSlots } from "../plugins/slots";
@@ -424,13 +425,24 @@ export function Layout() {
useEffect(() => {
const previousOverflow = document.body.style.overflow;
document.body.style.overflow = isMobile ? "visible" : "hidden";
document.body.style.overflow = isMobile ? "visible" : "clip";
return () => {
document.body.style.overflow = previousOverflow;
};
}, [isMobile]);
// `scrollIntoView` walks every ancestor scroll container. On a long thread
// the post-submit `scrollIntoView` on the new comment reaches `<html>` and
// animates `documentElement.scrollTop` via the browser's internal scroll
// algorithm, which bypasses the CSS `overflow` on the root element and
// visually shifts the entire shell (sidebar included) off-screen. Pin
// both roots to scrollTop=0 on every scroll tick.
useEffect(() => {
if (isMobile) return;
return pinDocumentScrollToZero();
}, [isMobile]);
useEffect(() => {
if (typeof document === "undefined") return;
const mainContent = mainContentRef.current;
@@ -456,7 +468,7 @@ export function Layout() {
<div
className={cn(
"bg-background text-foreground pt-[env(safe-area-inset-top)]",
isMobile ? "min-h-dvh" : "flex h-dvh flex-col overflow-hidden",
isMobile ? "min-h-dvh" : "flex h-dvh flex-col overflow-clip",
)}
>
<a
@@ -467,7 +479,7 @@ export function Layout() {
</a>
<WorktreeBanner />
<DevRestartBanner devServer={health?.devServer} />
<div className={cn("min-h-0 flex-1", isMobile ? "w-full" : "flex overflow-hidden")}>
<div className={cn("min-h-0 flex-1", isMobile ? "w-full" : "flex overflow-clip")}>
{isMobile && sidebarOpen && (
<button
type="button"
ui/src/lib/pin-document-scroll.test.ts
+76
View File
@@ -0,0 +1,76 @@
// @vitest-environment jsdom
import { afterEach, describe, expect, it, vi } from "vitest";
import { pinDocumentScrollToZero } from "./pin-document-scroll";
describe("pinDocumentScrollToZero", () => {
afterEach(() => {
document.documentElement.scrollTop = 0;
document.body.scrollTop = 0;
});
it("snaps documentElement.scrollTop back to 0 when a scroll event fires after it drifts", () => {
const cleanup = pinDocumentScrollToZero();
document.documentElement.scrollTop = 250;
window.dispatchEvent(new Event("scroll"));
expect(document.documentElement.scrollTop).toBe(0);
cleanup();
});
it("snaps body.scrollTop back to 0 too (some engines target body for the root scroll)", () => {
const cleanup = pinDocumentScrollToZero();
document.body.scrollTop = 120;
window.dispatchEvent(new Event("scroll"));
expect(document.body.scrollTop).toBe(0);
cleanup();
});
it("leaves scrollTop alone when it is already 0 (no thrash on routine scrolls)", () => {
const cleanup = pinDocumentScrollToZero();
const docSetter = vi.spyOn(document.documentElement, "scrollTop", "set");
const bodySetter = vi.spyOn(document.body, "scrollTop", "set");
window.dispatchEvent(new Event("scroll"));
expect(docSetter).not.toHaveBeenCalled();
expect(bodySetter).not.toHaveBeenCalled();
docSetter.mockRestore();
bodySetter.mockRestore();
cleanup();
});
it("registers the listener in capture phase and NOT passive so the reset runs before paint", () => {
const addSpy = vi.spyOn(window, "addEventListener");
const cleanup = pinDocumentScrollToZero();
expect(addSpy).toHaveBeenCalledWith(
"scroll",
expect.any(Function),
// capture: true, no passive flag — a passive listener would let the
// browser paint the shifted frame before the reset runs.
{ capture: true },
);
addSpy.mockRestore();
cleanup();
});
it("cleanup removes the listener — subsequent scroll events do not reset scrollTop", () => {
const cleanup = pinDocumentScrollToZero();
cleanup();
document.documentElement.scrollTop = 75;
window.dispatchEvent(new Event("scroll"));
expect(document.documentElement.scrollTop).toBe(75);
});
});
ui/src/lib/pin-document-scroll.ts
+30
View File
@@ -0,0 +1,30 @@
// Registers a capture-phase `scroll` listener on `win` that snaps the
// `documentElement.scrollTop` and `body.scrollTop` back to 0 whenever they
// drift. The desktop app shell expects neither to ever be non-zero — only
// `#main-content` should scroll — but `scrollIntoView`'s ancestor walk can
// reach `<html>` (which Chrome drives via its internal smooth-scroll
// algorithm, bypassing the JS `scrollTop` setter and ignoring CSS
// `overflow` on the root viewport). Snapping back on every scroll tick
// keeps the shell visually pinned.
//
// Returns a cleanup function that removes the listener.
export function pinDocumentScrollToZero(
doc: Document = document,
win: Window = window,
): () => void {
const onScroll = () => {
if (doc.documentElement.scrollTop !== 0) {
doc.documentElement.scrollTop = 0;
}
if (doc.body.scrollTop !== 0) {
doc.body.scrollTop = 0;
}
};
// Intentionally NOT `passive: true` — a passive listener lets the browser
// paint the scrolled frame before this handler runs, producing a one-frame
// flash of the shifted shell. The handler doesn't call `preventDefault()`
// and `scroll` events aren't cancelable, so dropping the passive flag has
// no performance downside and runs the reset synchronously with dispatch.
win.addEventListener("scroll", onScroll, { capture: true });
return () => win.removeEventListener("scroll", onScroll, { capture: true });
}