mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-11 05:31:46 +02:00
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>
105 lines
3.5 KiB
TypeScript
105 lines
3.5 KiB
TypeScript
export type IssueChatScrollTarget =
|
|
| { type: "element"; element: HTMLElement }
|
|
| { type: "window" };
|
|
|
|
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,
|
|
): IssueChatScrollTarget {
|
|
const mainContent = doc.getElementById("main-content");
|
|
|
|
if (mainContent instanceof HTMLElement) {
|
|
const overflowY = win.getComputedStyle(mainContent).overflowY;
|
|
const usesOwnScroll =
|
|
(overflowY === "auto" || overflowY === "scroll" || overflowY === "overlay")
|
|
&& mainContent.scrollHeight > mainContent.clientHeight + 1;
|
|
|
|
if (usesOwnScroll) {
|
|
return { type: "element", element: mainContent };
|
|
}
|
|
}
|
|
|
|
return { type: "window" };
|
|
}
|
|
|
|
export function captureComposerViewportSnapshot(
|
|
composerElement: HTMLElement | null,
|
|
): ComposerViewportSnapshot | null {
|
|
if (!composerElement) return null;
|
|
|
|
return {
|
|
composerViewportTop: composerElement.getBoundingClientRect().top,
|
|
};
|
|
}
|
|
|
|
export function shouldPreserveComposerViewport(
|
|
composerElement: HTMLElement | null,
|
|
doc: Document = document,
|
|
) {
|
|
if (!composerElement) return false;
|
|
|
|
const activeElement = doc.activeElement;
|
|
if (activeElement instanceof Node && composerElement.contains(activeElement)) {
|
|
return true;
|
|
}
|
|
return false;
|
|
}
|
|
|
|
export function restoreComposerViewportSnapshot(
|
|
snapshot: ComposerViewportSnapshot | null,
|
|
composerElement: HTMLElement | null,
|
|
doc: Document = document,
|
|
win: Window = window,
|
|
) {
|
|
if (!snapshot || !composerElement) return;
|
|
|
|
const delta = composerElement.getBoundingClientRect().top - snapshot.composerViewportTop;
|
|
if (!Number.isFinite(delta) || Math.abs(delta) < 1) return;
|
|
|
|
const target = resolveIssueChatScrollTarget(doc, win);
|
|
if (target.type === "element") {
|
|
target.element.scrollTop += delta;
|
|
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" });
|
|
}
|