fix(ui): don't window-scroll the desktop shell on comment submit (#7972) (#8041)

Fixes #7972.

## Thinking Path
- The reporter pinned the post-submit composer-viewport restore in
`ui/src/lib/issue-chat-scroll.ts`, which falls back to
`window.scrollBy(...)` when `#main-content` is not independently
scrollable — exactly the short-thread repro case.
- In the desktop shell the body is `overflow: hidden` (set in
`Layout.tsx`) inside a fixed-height `h-dvh` flex column, so a window
scroll never moves content: it translates the entire shell (sidebar
included) off-screen, and a plain reload does not restore it. On mobile
(`min-h-dvh`, `body { overflow: visible }`) and the auth-free perf
fixture the page genuinely scrolls, so the window IS the correct target
there.
- A prior attempt forced `resolveIssueChatScrollTarget` to always use
`#main-content`; that path is a no-op on a non-overflowing container and
is sensitive to a stale `ui/dist`/`.vite` cache, which likely masked the
result. Gating the window-scroll itself is the precise root-cause fix
and covers both restore call sites (`queueViewportRestore` and the
`[messages]` layout effect) since both route through one function.

## What Changed
- Added `isWindowScrollable(doc, win)` to `issue-chat-scroll.ts`: the
window is a valid scroll target only when the document body is not
clipped. It checks both the `overflow` shorthand and the `overflow-y`
longhand (some engines, incl. jsdom, do not derive the longhand from the
shorthand in computed style).
- Gated the `window.scrollBy` fallback in
`restoreComposerViewportSnapshot` behind `isWindowScrollable`; on the
desktop shell there is nothing to restore, so the scroll position is
left untouched.
- Added unit tests for the desktop-shell (no window scroll) case and for
`isWindowScrollable`.

## Verification
- `ui $ vitest run src/lib/issue-chat-scroll.test.ts` → 6 passed (2 new
+ existing window/element restore tests still green).
- `ui $ vitest run src/components/IssueChatThread.test.tsx` → 55 passed
(consumer regression).

## Risks
- Low. Behaviour only changes when the resolved target is `window` AND
the document body is clipped — i.e. the desktop shell, where the
previous behaviour was the bug. Mobile and the perf fixture keep window
scrolling unchanged (body not clipped → `isWindowScrollable` true).

## Model Used
claude-opus-4-8

---

- [x] I searched the GitHub PRs for similar or duplicate PRs and
confirmed this is not a duplicate.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Doyeon Baek
2026-06-13 02:05:57 +09:00
committed by GitHub
parent 3fbab2e6db
commit 4965dc834a
2 changed files with 67 additions and 0 deletions
+33
View File
@@ -3,6 +3,7 @@
import { describe, expect, it, vi } from "vitest";
import {
captureComposerViewportSnapshot,
isWindowScrollable,
restoreComposerViewportSnapshot,
shouldPreserveComposerViewport,
} from "./issue-chat-scroll";
@@ -39,6 +40,38 @@ describe("issue-chat-scroll", () => {
composer.remove();
});
it("does not scroll the window when the document body is overflow:hidden (desktop shell)", () => {
// The desktop app shell pins the body to overflow:hidden inside a fixed
// h-dvh flex column, so a window scroll would translate the whole shell
// (sidebar included) off-screen — paperclipai/paperclip#7972.
const composer = document.createElement("div");
document.body.appendChild(composer);
document.body.style.overflow = "hidden";
const scrollByMock = vi.spyOn(window, "scrollBy").mockImplementation(() => {});
mockTop(composer, 420);
const snapshot = captureComposerViewportSnapshot(composer);
mockTop(composer, 560);
restoreComposerViewportSnapshot(snapshot, composer);
expect(scrollByMock).not.toHaveBeenCalled();
scrollByMock.mockRestore();
document.body.style.overflow = "";
composer.remove();
});
it("reports the document as scrollable only when the body is not clipped", () => {
document.body.style.overflow = "";
expect(isWindowScrollable()).toBe(true);
document.body.style.overflow = "hidden";
expect(isWindowScrollable()).toBe(false);
document.body.style.overflow = "";
});
it("restores main-content scroll when the layout uses an internal scroller", () => {
const mainContent = document.createElement("main");
mainContent.id = "main-content";
+34
View File
@@ -6,6 +6,34 @@ export interface ComposerViewportSnapshot {
composerViewportTop: number;
}
/**
* The page itself is only a usable scroll target when the document can actually
* scroll. The desktop app shell pins the body to `overflow: hidden` and renders
* a fixed-height (`h-dvh`) flex column, so scrolling the window there does not
* move content — it translates the entire shell (sidebar included) out of the
* viewport, the regression reported in paperclipai/paperclip#7972. The mobile
* shell and the auth-free perf fixture leave the body scrollable, where window
* scrolling is the correct behaviour.
*/
export function isWindowScrollable(
doc: Document = document,
win: Window = window,
): boolean {
const candidates = [doc.scrollingElement, doc.documentElement, doc.body];
for (const element of candidates) {
if (!(element instanceof HTMLElement)) continue;
const style = win.getComputedStyle(element);
// Check both the `overflow-y` longhand and the `overflow` shorthand: the
// shell sets `body.style.overflow` (the shorthand) and some engines (incl.
// jsdom) do not derive the longhand from it in computed style.
const clipped = (value: string) => value === "hidden" || value === "clip";
if (clipped(style.overflowY) || clipped(style.overflow)) {
return false;
}
}
return true;
}
export function resolveIssueChatScrollTarget(
doc: Document = document,
win: Window = window,
@@ -66,5 +94,11 @@ export function restoreComposerViewportSnapshot(
return;
}
// Falling back to the window is only safe when the page itself scrolls. In
// the fixed-height desktop shell the body is `overflow: hidden`, so a window
// scroll would shift the whole app shell — sidebar included — off-screen
// (paperclipai/paperclip#7972). There is nothing to restore in that case.
if (!isWindowScrollable(doc, win)) return;
win.scrollBy({ top: delta, left: 0, behavior: "auto" });
}