From 412a04c964915fd45e15675f064d24332d50f6cc Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 12 Jun 2026 17:31:16 -0700 Subject: [PATCH] fix(ui): keep desktop shell pinned when scrollIntoView walks past body (#8071) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 `` 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 `` (and historically the two shell `
`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, `` 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 `
`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 ``. - 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 --- ui/src/components/Layout.tsx | 18 +++++- ui/src/lib/pin-document-scroll.test.ts | 76 ++++++++++++++++++++++++++ ui/src/lib/pin-document-scroll.ts | 30 ++++++++++ 3 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 ui/src/lib/pin-document-scroll.test.ts create mode 100644 ui/src/lib/pin-document-scroll.ts diff --git a/ui/src/components/Layout.tsx b/ui/src/components/Layout.tsx index 18fe7cc3..a03400b1 100644 --- a/ui/src/components/Layout.tsx +++ b/ui/src/components/Layout.tsx @@ -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 `` 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() {
-
+
{isMobile && sidebarOpen && (