From dff0511dcad95814a5a04fa71df6e8ae60846d56 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Wed, 17 Jun 2026 20:33:12 -0400 Subject: [PATCH] fix(desktop): bottom-align short channels via row offset, not spacer padding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The short-channel topPad bottom-align was doubly broken. First, the pad was set as the spacer's paddingTop, but an absolutely-positioned virtual row resolves top:0 against the padding box inner edge — padding inflated the box without moving the rows, so they pinned to the top with dead space below (the inverse of bottom-align). Fold topPad into each row's translateY instead. Second, the pad ignored the non-row scroll chrome (the channel-header padding above the spacer and composer padding below), so the spacer over-padded by exactly that chrome and a short channel became scrollable. Subtract the measured chrome (scrollHeight minus spacer height, which is pad-invariant). Adds a fail-first e2e asserting a short channel bottom-aligns against the content-box floor and is not scrollable. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/playwright.config.ts | 1 + .../features/messages/ui/MessageTimeline.tsx | 2 + .../messages/ui/TimelineMessageList.tsx | 23 ++++--- .../messages/ui/useChatScrollVirtualizer.ts | 34 ++++++++-- .../tests/e2e/timeline-scroll-anchor.spec.ts | 67 +++++++++++++++++++ 5 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 desktop/tests/e2e/timeline-scroll-anchor.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 8256c70c9..a9ccd6e12 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -49,6 +49,7 @@ export default defineConfig({ "**/animated-avatar-screenshots.spec.ts", "**/reminders-screenshots.spec.ts", "**/virtualization-screenshots.spec.ts", + "**/timeline-scroll-anchor.spec.ts", ], use: { ...devices["Desktop Chrome"], diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index 206cd99e8..63c4e9ac4 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -226,6 +226,7 @@ export const MessageTimeline = React.memo(function MessageTimeline({ const { virtualizer, + spacerRef, topPad, isAtBottom, newMessageCount, @@ -619,6 +620,7 @@ export const MessageTimeline = React.memo(function MessageTimeline({ searchQuery={searchQuery} threadUnreadCounts={threadUnreadCounts} topPad={topPad} + spacerRef={spacerRef} unfollowThreadById={unfollowThreadById} virtualizer={virtualizer} /> diff --git a/desktop/src/features/messages/ui/TimelineMessageList.tsx b/desktop/src/features/messages/ui/TimelineMessageList.tsx index fab81c912..af8acffa3 100644 --- a/desktop/src/features/messages/ui/TimelineMessageList.tsx +++ b/desktop/src/features/messages/ui/TimelineMessageList.tsx @@ -85,6 +85,8 @@ type TimelineMessageListProps = { * pushes the absolute rows down without entering the measured total. */ topPad: number; + /** Ref the hook reads to measure chrome around the spacer for `topPad`. */ + spacerRef: React.RefObject; }; export const TimelineMessageList = React.memo(function TimelineMessageList({ @@ -116,6 +118,7 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({ virtualizer, items, topPad, + spacerRef, }: TimelineMessageListProps) { const reviewCommentsByRootId = React.useMemo( () => buildVideoReviewCommentsByRootId(messages), @@ -302,18 +305,18 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({ } }; - // The spacer is `box-sizing: border-box` (Tailwind global), so `topPad` is - // folded into `height` AND set as `paddingTop`: the height keeps the full - // scroll range while the padding pushes the absolute rows (positioned against - // the padding box, `top:0` = inner edge) down to bottom-align a short channel. - // `translateY` then uses the virtualizer's raw `start` — no double offset. + // Bottom-align a short channel by shifting the absolute rows down by + // `topPad`. The pad CANNOT be a `paddingTop` on the spacer: an + // absolutely-positioned child resolves `top:0` against the padding box's + // inner edge, so padding inflates the box without moving the rows — they + // would pin to the top with dead space below. Folding `topPad` into each + // row's `translateY` (and into the spacer height for the matching scroll + // range) is what actually pushes the rows to the floor. return (
0 ? `${topPad}px` : undefined, - }} + ref={spacerRef as React.Ref} + style={{ height: `${virtualizer.getTotalSize() + topPad}px` }} > {virtualizer.getVirtualItems().map((virtualRow) => (
{renderItem(items[virtualRow.index])} diff --git a/desktop/src/features/messages/ui/useChatScrollVirtualizer.ts b/desktop/src/features/messages/ui/useChatScrollVirtualizer.ts index 8369ce860..c906405d8 100644 --- a/desktop/src/features/messages/ui/useChatScrollVirtualizer.ts +++ b/desktop/src/features/messages/ui/useChatScrollVirtualizer.ts @@ -31,9 +31,11 @@ const HIGHLIGHT_DURATION_MS = 2_000; * * - **Short-channel bottom-align pad.** A 2-3 message channel must sit at the * bottom of the viewport. The virtualizer lays rows from the top, so we add - * a top pad of `max(0, viewportHeight - totalSize)`. It is recomputed off - * the LIVE `getTotalSize()` on every measurement pass (via `onChange`) so - * it collapses to 0 the instant content exceeds the viewport — see the + * a top pad of `max(0, viewportHeight - totalSize - chrome)`, where `chrome` + * is the non-row scroll content around the spacer (header/overlay padding + * above, composer padding below). It is recomputed off the LIVE + * `getTotalSize()` on every measurement pass (via `onChange`) so it + * collapses to 0 the instant content exceeds the viewport — see the * ordering note below. * - **The "at bottom" / new-message-count UI state** the scroll-to-latest * pill reads. The library knows the geometry (`isAtEnd`); this hook lifts @@ -68,6 +70,12 @@ export type ChatScrollVirtualizerOptions = { export type ChatScrollVirtualizer = { virtualizer: ChatVirtualizer; + /** + * Ref the surface attaches to the row spacer (the element whose `paddingTop` + * carries `topPad`). The pad math measures the chrome around the spacer off + * it — see `topPad`. + */ + spacerRef: React.RefObject; /** * Top padding (px) that bottom-aligns a channel whose content is shorter * than the viewport. Apply it to the row spacer's `paddingTop`. Always `0` @@ -115,6 +123,16 @@ export function useChatScrollVirtualizer({ string | null >(null); + // The spacer carries `topPad` as its `paddingTop`, so non-row chrome that + // also lives in the scroll content — the channel-header padding above the + // spacer (`contentPadding`, reserving room for the pinned day overlay) and + // the composer padding below it — must be subtracted from the pad, or a + // short channel over-pads by exactly that chrome and becomes scrollable. + // `scrollHeight - spacer.offsetHeight` is that chrome and is pad-invariant: + // both grow by the same delta when the pad changes, so it converges in one + // pass. + const spacerRef = React.useRef(null); + // The bottom-state and pad recompute both read live geometry off the // virtualizer on each `onChange`. virtual-core fires `onChange` directly from // `resizeItem` on any size delta (not only on a visible-range change), which @@ -130,7 +148,14 @@ export function useChatScrollVirtualizer({ if (!scrollEl) { return; } - const pad = Math.max(0, scrollEl.clientHeight - instance.getTotalSize()); + const spacerEl = spacerRef.current; + const chrome = spacerEl + ? Math.max(0, scrollEl.scrollHeight - spacerEl.offsetHeight) + : 0; + const pad = Math.max( + 0, + scrollEl.clientHeight - instance.getTotalSize() - chrome, + ); setTopPad((prev) => (prev === pad ? prev : pad)); // The app's "at bottom" rule is looser than the library's 1px default; @@ -228,6 +253,7 @@ export function useChatScrollVirtualizer({ return { virtualizer, + spacerRef, topPad, isAtBottom, newMessageCount, diff --git a/desktop/tests/e2e/timeline-scroll-anchor.spec.ts b/desktop/tests/e2e/timeline-scroll-anchor.spec.ts new file mode 100644 index 000000000..25df23832 --- /dev/null +++ b/desktop/tests/e2e/timeline-scroll-anchor.spec.ts @@ -0,0 +1,67 @@ +import { expect, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; + +// Scroll-anchoring guards for the virtualized main timeline (PR: virtualize +// timeline). The channel-jump bug was the timeline yanking the viewport when +// the message set changed. These assert geometry via getBoundingClientRect, +// not scrollTop — the bug surfaced as the anchored row visibly jumping even +// while scrollTop looked plausible, so position-on-screen is the real contract. + +test.beforeEach(async ({ page }) => { + await installMockBridge(page); +}); + +// A short channel (#general seeds four backdated messages, far below a full +// viewport) must bottom-align: the rows sit against the bottom of the scroll +// area with a top pad above them, exactly like the legacy spacer. If topPad +// were dropped, the rows would float at the top with empty space below. +test("short channel bottom-aligns its messages against the viewport floor", async ({ + page, +}) => { + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + + const timeline = page.getByTestId("message-timeline"); + const rows = page.getByTestId("message-row"); + await expect(rows.first()).toBeVisible(); + + // Far fewer rows than fill the viewport — the precondition for bottom-align. + const metrics = await timeline.evaluate((element) => ({ + clientHeight: element.clientHeight, + scrollHeight: element.scrollHeight, + })); + expect(metrics.scrollHeight).toBeLessThanOrEqual(metrics.clientHeight + 1); + + // The content bottom-aligns against the scroll container's content-box floor + // (the inner edge above the composer-reserved bottom padding), and the first + // row is pushed well below the top by the pad above it. "Last row" is the + // lowest of any rendered row kind — #general's final seed is a system join + // row, which sits below the last message-row, so anchoring on message-row + // alone would misread the floor. + const geometry = await timeline.evaluate((element) => { + const style = getComputedStyle(element); + const paddingBottom = Number.parseFloat(style.paddingBottom); + const timelineRect = element.getBoundingClientRect(); + const contentRows = Array.from( + element.querySelectorAll( + '[data-testid="message-row"], [data-testid="system-message-row"]', + ), + ).map((row) => row.getBoundingClientRect()); + const firstTop = contentRows[0]?.top ?? Number.NaN; + const lastBottom = Math.max(...contentRows.map((rect) => rect.bottom)); + return { + timelineTop: timelineRect.top, + contentFloor: timelineRect.bottom - paddingBottom, + firstTop, + lastBottom, + }; + }); + + // Bottom-aligned: the last row ends within a small margin of the content + // floor (one row gap of slack). + expect(geometry.contentFloor - geometry.lastBottom).toBeLessThanOrEqual(24); + // Padded from the top: the first row is pushed down, not floating at the top. + expect(geometry.firstTop - geometry.timelineTop).toBeGreaterThan(96); +});