From 4965dc834aa4100aa42a681af1aecd2cae4fb3fb Mon Sep 17 00:00:00 2001
From: Doyeon Baek <41532591+dosthcpp@users.noreply.github.com>
Date: Sat, 13 Jun 2026 02:05:57 +0900
Subject: [PATCH] fix(ui): don't window-scroll the desktop shell on comment
 submit (#7972) (#8041)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 ui/src/lib/issue-chat-scroll.test.ts | 33 +++++++++++++++++++++++++++
 ui/src/lib/issue-chat-scroll.ts      | 34 ++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/ui/src/lib/issue-chat-scroll.test.ts b/ui/src/lib/issue-chat-scroll.test.ts
index 491369ad..1c23e058 100644
--- a/ui/src/lib/issue-chat-scroll.test.ts
+++ b/ui/src/lib/issue-chat-scroll.test.ts
@@ -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";
diff --git a/ui/src/lib/issue-chat-scroll.ts b/ui/src/lib/issue-chat-scroll.ts
index b70f3d92..6054e079 100644
--- a/ui/src/lib/issue-chat-scroll.ts
+++ b/ui/src/lib/issue-chat-scroll.ts
@@ -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" });
 }