diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index bb26f55880..61f77de46c 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -116,6 +116,7 @@ export default defineConfig({ "**/reminder-click-repro.spec.ts", "**/virtualization.spec.ts", "**/scroll-history.spec.ts", + "**/message-target-highlight.spec.ts", "**/channel-dense-second-reach.spec.ts", "**/channel-window-mock-paging.spec.ts", "**/channel-head-restart.spec.ts", diff --git a/desktop/src/features/messages/lib/virtualizedTimelineItems.test.mjs b/desktop/src/features/messages/lib/virtualizedTimelineItems.test.mjs index daae41a786..baebeb1521 100644 --- a/desktop/src/features/messages/lib/virtualizedTimelineItems.test.mjs +++ b/desktop/src/features/messages/lib/virtualizedTimelineItems.test.mjs @@ -206,6 +206,45 @@ test("naive counterexample stays rejected: same key cannot precede prepended row ); }); +test("head trim from a window refresh is admitted as a shift", () => { + const previousKeys = keysOf([ + group("day-Z", DAY_Z, ["z1", "z2"]), + group("day-A", DAY_A, ["a1", "a2", "a3"]), + ]); + // Trimmed inside the oldest day, and exactly at its proven boundary: the + // surviving oldest day loses its divider, so the result is a clean suffix. + for (const keys of [ + keysOf([group("day-A", DAY_A, ["a2", "a3"])]), + keysOf([group("day-A", DAY_A, ["a1", "a2", "a3"])]), + ]) { + assert.equal(didPrependVirtualizedTimeline(previousKeys, keys), true); + const delta = previousKeys.length - keys.length; + keys.forEach((key, index) => { + assert.equal(previousKeys[index + delta], key); + }); + } +}); + +test("shrinks that are not a pure head trim are not shifted", () => { + const previousKeys = keysOf([group("day-A", DAY_A, ["a1", "a2", "a3"])]); + // Trimmed from the head while a new message appended. + assert.equal( + didPrependVirtualizedTimeline( + previousKeys, + keysOf([group("day-A", DAY_A, ["a3", "a4"])]), + ), + false, + ); + // A message removed from the middle. + assert.equal( + didPrependVirtualizedTimeline( + previousKeys, + keysOf([group("day-A", DAY_A, ["a1", "a3"])]), + ), + false, + ); +}); + test("virtualized rows preserve their heterogeneous height estimates", () => { const short = messageItem("short"); short.entry.message.body = "hello"; diff --git a/desktop/src/features/messages/lib/virtualizedTimelineItems.ts b/desktop/src/features/messages/lib/virtualizedTimelineItems.ts index 1ab68af181..8b6393203d 100644 --- a/desktop/src/features/messages/lib/virtualizedTimelineItems.ts +++ b/desktop/src/features/messages/lib/virtualizedTimelineItems.ts @@ -107,11 +107,18 @@ export function didPrependVirtualizedTimeline( previousKeys: readonly string[], keys: readonly string[], ): boolean { - const prependedCount = keys.length - previousKeys.length; + const delta = keys.length - previousKeys.length; // Virtua shifts its positional size cache by the length delta, so enable // `shift` only when every previous slot is the exact suffix it expects. + if (delta > 0) { + return previousKeys.every((key, index) => key === keys[index + delta]); + } + // A channel-window refresh drops older pages from the start of the list. + // Without `shift` the rows above the reader vanish uncompensated and carry + // the reading position away, so a pure head trim shifts too. return ( - prependedCount > 0 && - previousKeys.every((key, index) => key === keys[index + prependedCount]) + delta < 0 && + keys.length > 0 && + keys.every((key, index) => key === previousKeys[index - delta]) ); } diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index cecb291883..3ceeff8f97 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -498,8 +498,6 @@ const MessageTimelineBase = React.forwardRef< [prepareForOwnMessage, scrollToBottom, timelineVirtualizerApi], ); - // Jump-to-message is purely DOM-based now: all loaded rows are mounted, so - // `scrollToMessage` always finds the target row. No virtualizer convergence. const jumpToMessage = React.useCallback( (messageId: string, options?: { behavior?: ScrollBehavior }) => { return scrollToMessage(messageId, { highlight: true, ...options }); @@ -542,8 +540,8 @@ const MessageTimelineBase = React.forwardRef< // Scroll to the active search match when it changes. `jumpToMessage` updates // the scroll anchor (so the post-commit restore won't yank the view back off - // the match) and, when virtualized, converges on the target through the index - // model — the row may be windowed out of the DOM. + // the match) and, when virtualized, scrolls through the index model — the row + // may be windowed out of the DOM. const prevSearchActiveRef = React.useRef(null); const pendingSearchTargetRef = React.useRef(null); React.useEffect(() => { @@ -565,22 +563,10 @@ const MessageTimelineBase = React.forwardRef< } }, [jumpToMessage, searchActiveMessageId, showTimelineSkeleton]); - // biome-ignore lint/correctness/useExhaustiveDependencies: deferredMessages and virtualizerRenderVersion are intentional retry triggers — a search hit may be spliced into messages asynchronously, and in virtualized mode a phase-1 index jump only realizes the row; retry when the rendered range changes so the DOM-visible path can center and highlight it. + // biome-ignore lint/correctness/useExhaustiveDependencies: deferredMessages and virtualizerRenderVersion are intentional retry triggers — a search hit may be spliced into messages asynchronously, so retry until the timeline can scroll to it. React.useEffect(() => { const target = pendingSearchTargetRef.current; if (!target || showTimelineSkeleton) return; - if ( - useTimelineVirtualizer && - !activeScrollContainerRef.current?.querySelector( - `[data-message-id="${CSS.escape(target)}"]`, - ) - ) { - // Phase 1: ask the virtualizer to realize the match's index. The retry effect - // runs again on range change and the DOM-visible path does the actual - // center + highlight once the row exists. - void jumpToMessage(target, { behavior: "auto" }); - return; - } if (jumpToMessage(target, { behavior: "auto" })) { pendingSearchTargetRef.current = null; } diff --git a/desktop/src/features/messages/ui/TimelineMessageList.tsx b/desktop/src/features/messages/ui/TimelineMessageList.tsx index d9f24468b2..c941e2c84a 100644 --- a/desktop/src/features/messages/ui/TimelineMessageList.tsx +++ b/desktop/src/features/messages/ui/TimelineMessageList.tsx @@ -683,11 +683,14 @@ function VirtualizedTimelineRows({ settleAtBottom(); }, settleAtBottom, - scrollToMessage(messageId) { - cancelBottomSettle(); + scrollToMessage(messageId, options) { + const list = listRef.current; const index = messageItemIndexByIdRef.current.get(messageId); - if (index === undefined) return false; - listRef.current?.scrollToIndex(index, { align: "center" }); + if (!list || index === undefined) return false; + list.scrollToIndex(index, { + align: "center", + smooth: options?.behavior === "smooth", + }); return true; }, }; diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs index ee3fec1a98..044efae607 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs +++ b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs @@ -281,7 +281,10 @@ function VirtualTargetHarness({ refs }) { scrollContainerRef: refs.scroller, virtualCancelBottomIntent: bottomApi.cancel, virtualizerOwnsPrependAnchoring: true, - virtualScrollToMessage: () => true, + virtualScrollToMessage: (messageId, options) => { + refs.targetScrolls.push({ messageId, options }); + return refs.targetResolves; + }, }); React.useLayoutEffect(() => { if (didRun.current) return; @@ -522,7 +525,7 @@ test("user interaction releases and retires a pending pinned target", async () = await act(async () => root.unmount()); }); -test("mounted virtual target retires bottom intent before direct centering", async () => { +async function renderVirtualTarget({ targetResolves }) { const resizeObservers = []; globalThis.ResizeObserver = class { constructor(callback) { @@ -571,7 +574,9 @@ test("mounted virtual target retires bottom intent before direct centering", asy }, }, scroller: { current: scroller }, + targetResolves, targetResult: { current: null }, + targetScrolls: [], }; const root = createRoot(document.createElement("div")); await act(async () => { @@ -579,21 +584,113 @@ test("mounted virtual target retires bottom intent before direct centering", asy }); assert.deepEqual(bottomWrites, [{ index: 4, options: { align: "end" } }]); - assert.equal(refs.targetResult.current, true); - assert.equal(row.getBoundingClientRect().top, 180); - const bottomGeometryObserver = resizeObservers.find( - (observer) => - observer.targets.includes(content) && observer.targets.includes(scroller), - ); - assert.ok(bottomGeometryObserver); - bottomGeometryObserver.callback(); - await act(async () => new Promise((resolve) => setTimeout(resolve, 0))); + const settleBottomGeometry = async () => { + const bottomGeometryObserver = resizeObservers.find( + (observer) => + observer.targets.includes(content) && + observer.targets.includes(scroller), + ); + assert.ok(bottomGeometryObserver); + bottomGeometryObserver.callback(); + await act(async () => new Promise((resolve) => setTimeout(resolve, 0))); + }; + return { bottomWrites, refs, root, scroller, settleBottomGeometry }; +} + +test("mounted virtual target retires bottom intent and scrolls through the virtualizer", async () => { + const { bottomWrites, refs, root, scroller, settleBottomGeometry } = + await renderVirtualTarget({ targetResolves: true }); + + assert.equal(refs.targetResult.current, true); + // Even an on-screen row goes through the virtualizer: a direct DOM scroll + // would not supersede a bottom pin the virtualizer still has in flight. + assert.deepEqual(refs.targetScrolls, [ + { messageId: "selected", options: { behavior: "auto" } }, + ]); + assert.equal(scroller.scrollTop, 0, "no direct DOM centering write"); + await settleBottomGeometry(); - assert.equal( - row.getBoundingClientRect().top, - 180, - "target remains centered after later virtual geometry activity", - ); assert.equal(bottomWrites.length, 1, "geometry cannot re-pin to bottom"); await act(async () => root.unmount()); }); + +test("unresolved virtual target keeps the bottom pin", async () => { + const { bottomWrites, refs, root, settleBottomGeometry } = + await renderVirtualTarget({ targetResolves: false }); + + // A target that is not loaded yet retries on later commits. Until it + // resolves, the channel must keep following its floor. + assert.equal(refs.targetResult.current, false); + await settleBottomGeometry(); + + assert.equal(bottomWrites.length, 2, "geometry still re-pins to bottom"); + await act(async () => root.unmount()); +}); + +test("target arriving before the second mount frame supersedes the bottom pin", async () => { + const nodes = makePinnedCenterNodes(); + nodes.container.querySelector = () => null; + const frames = new Map(); + let nextFrameId = 0; + const previousRaf = globalThis.requestAnimationFrame; + const previousCancelRaf = globalThis.cancelAnimationFrame; + globalThis.requestAnimationFrame = (callback) => { + nextFrameId += 1; + frames.set(nextFrameId, callback); + return nextFrameId; + }; + globalThis.cancelAnimationFrame = (id) => frames.delete(id); + const writes = []; + let targetLoaded = false; + const scrollContainerRef = { current: nodes.container }; + const contentRef = { current: nodes.content }; + function RaceHarness({ messages }) { + useAnchoredScroll({ + channelId: "conversation", + contentRef, + isLoading: false, + messages, + scrollContainerRef, + targetMessageId: "selected", + virtualizerOwnsPrependAnchoring: true, + virtualScrollToBottom: () => writes.push("bottom"), + virtualScrollToMessage: () => { + if (!targetLoaded) return false; + writes.push("target"); + return true; + }, + }); + return null; + } + const root = createRoot(document.createElement("div")); + try { + // The target is not loaded on mount, so the hook pins to bottom now and + // queues a second pin for the next frame. + await act(async () => + root.render( + React.createElement(RaceHarness, { messages: [{ id: "tail" }] }), + ), + ); + // The target lands before that frame runs. + targetLoaded = true; + await act(async () => + root.render( + React.createElement(RaceHarness, { + messages: [{ id: "selected" }, { id: "tail" }], + }), + ), + ); + await act(async () => { + for (const [id, callback] of [...frames]) { + frames.delete(id); + callback(); + } + }); + + assert.equal(writes.at(-1), "target", JSON.stringify(writes)); + } finally { + await act(async () => root.unmount()); + globalThis.requestAnimationFrame = previousRaf; + globalThis.cancelAnimationFrame = previousCancelRaf; + } +}); diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.ts b/desktop/src/features/messages/ui/useAnchoredScroll.ts index 0bfcb3b3e2..41e0b40786 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.ts +++ b/desktop/src/features/messages/ui/useAnchoredScroll.ts @@ -101,6 +101,12 @@ function isAtBottomNow( ); } +function isInViewport(el: HTMLElement, container: HTMLElement): boolean { + const rect = el.getBoundingClientRect(); + const containerRect = container.getBoundingClientRect(); + return rect.top >= containerRect.top && rect.bottom <= containerRect.bottom; +} + /** * Pick an anchor for the current scroll position. * @@ -210,6 +216,12 @@ export function useAnchoredScroll({ const programmaticScrollRafRef = React.useRef(null); const targetSettleRafRef = React.useRef(null); + const cancelMountBottomPin = React.useCallback(() => { + if (mountPinRafIdRef.current === null) return; + cancelAnimationFrame(mountPinRafIdRef.current); + mountPinRafIdRef.current = null; + }, []); + // Reset everything when the channel changes — the layout effect that runs // immediately after this reset is responsible for either jumping to bottom // or to the target message for the new channel. @@ -439,48 +451,37 @@ export function useAnchoredScroll({ `[data-message-id="${messageId}"]`, ); if (virtualizerOwnsPrependAnchoring && virtualScrollToMessage) { - // Target navigation owns the viewport before any movement strategy is - // chosen. The already-mounted fast path centers the DOM node directly - // and would otherwise leave durable bottom intent armed. - virtualCancelBottomIntent?.(); - if (el) { - const rect = el.getBoundingClientRect(); - const containerRect = container.getBoundingClientRect(); - const isInViewport = - rect.top >= containerRect.top && - rect.bottom <= containerRect.bottom; - if (!isInViewport) { - if (!virtualScrollToMessage(messageId, { behavior: "auto" })) { - return false; - } - anchorRef.current = { kind: "message", messageId, topOffset: 0 }; - setIsAtBottom(false); - return false; - } - const centeredTop = (container.clientHeight - rect.height) / 2; - container.scrollTo({ - top: Math.max( - 0, - container.scrollTop + - (rect.top - containerRect.top) - - centeredTop, - ), - behavior: options.behavior ?? "auto", - }); - } else if ( + // Virtua owns the viewport, so every target goes through its index + // scroll. A direct DOM scroll would not supersede a bottom pin Virtua + // still has in flight from opening the channel, and that pin would + // carry the target back off screen. + if ( !virtualScrollToMessage(messageId, { - behavior: options.behavior ?? "auto", + // Smooth scrolling across unmeasured rows lands short of the + // target, so only an already-visible row animates. + behavior: + el && isInViewport(el, container) + ? (options.behavior ?? "auto") + : "auto", }) ) { + // A target that is not loaded yet leaves the bottom pin alone. return false; } + // An accepted target supersedes every pending bottom pin. + virtualCancelBottomIntent?.(); + cancelMountBottomPin(); anchorRef.current = { kind: "message", messageId, topOffset: 0 }; + virtualizerAtBottomRef.current = false; setIsAtBottom(false); - if (el && options.highlight) highlightMessage(messageId); - return el !== null; + // The highlight is keyed by id, so a row Virtua mounts after this + // scroll still renders it. + if (options.highlight) highlightMessage(messageId); + return true; } if (!el) return false; + cancelMountBottomPin(); const rect = el.getBoundingClientRect(); const containerRect = container.getBoundingClientRect(); @@ -535,6 +536,7 @@ export function useAnchoredScroll({ return true; }, [ + cancelMountBottomPin, highlightMessage, pinTargetCentered, scrollContainerRef, @@ -893,32 +895,17 @@ export function useAnchoredScroll({ if (!hasInitializedRef.current) return; // initial-mount path will handle. void virtualizerRenderVersion; - const container = scrollContainerRef.current; - if (!container) return; - const el = container.querySelector( - `[data-message-id="${targetMessageId}"]`, - ); - if (!el && virtualizerOwnsPrependAnchoring) { - if ( - scrollToMessageImperative(targetMessageId, { - highlight: highlightTargetMessage, - }) - ) { - handledTargetIdRef.current = targetMessageId; - onTargetReached?.(targetMessageId); - } - return; - } - if (!el) { - // Row not in the DOM yet. A cold deep-link target is fetched by id and - // spliced into `messages` a render or two later; this effect re-runs on - // each `messages` commit and retries until the row exists. + if ( + !scrollToMessageImperative(targetMessageId, { + highlight: highlightTargetMessage, + }) + ) { + // Not loaded yet. A cold deep-link target is fetched by id and spliced + // into `messages` a render or two later; this effect re-runs on each + // `messages` commit and retries until the row can be reached. return; } handledTargetIdRef.current = targetMessageId; - scrollToMessageImperative(targetMessageId, { - highlight: highlightTargetMessage, - }); onTargetReached?.(targetMessageId); }, [ highlightTargetMessage, @@ -926,10 +913,8 @@ export function useAnchoredScroll({ messages, onTargetReached, releasePinnedCenter, - scrollContainerRef, scrollToMessageImperative, targetMessageId, - virtualizerOwnsPrependAnchoring, virtualizerRenderVersion, ]); diff --git a/desktop/tests/e2e/message-target-highlight.spec.ts b/desktop/tests/e2e/message-target-highlight.spec.ts new file mode 100644 index 0000000000..f13e6c3a01 --- /dev/null +++ b/desktop/tests/e2e/message-target-highlight.spec.ts @@ -0,0 +1,181 @@ +import { expect, type Page, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; + +// A message target (deep link, link click, jump) must flash wherever the row +// starts relative to the viewport. The virtualized timeline scrolls by +// different strategies for each starting position; the flash must not depend +// on which one ran. + +const HIGHLIGHT_CLASS = "route-target-highlight-fade"; + +type TargetCase = { + name: string; + /** Navigate from another channel, as a link click from elsewhere does. */ + fromOtherChannel: boolean; + prependCount: number; + pick: (ids: { mounted: string[]; prepended: string[] }) => string; +}; + +const CASES: TargetCase[] = [ + { + name: "same channel, row on screen", + fromOtherChannel: false, + prependCount: 50, + pick: ({ mounted }) => mounted[mounted.length - 3], + }, + { + name: "same channel, row loaded off screen", + fromOtherChannel: false, + prependCount: 50, + pick: ({ mounted }) => mounted[2], + }, + { + name: "same channel, row in unloaded older history", + fromOtherChannel: false, + prependCount: 600, + pick: ({ prepended }) => prepended[Math.floor(prepended.length / 8)], + }, + { + name: "other channel, row near the bottom", + fromOtherChannel: true, + prependCount: 50, + pick: ({ mounted }) => mounted[mounted.length - 3], + }, + { + name: "other channel, row further up", + fromOtherChannel: true, + prependCount: 50, + pick: ({ mounted }) => mounted[2], + }, +]; + +async function seedGeneral(page: Page, prependCount: number) { + await installMockBridge(page); + await page.goto("/"); + await page.waitForFunction( + () => + typeof window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__ === "function" && + typeof window.__BUZZ_E2E_PREPEND_MOCK_HISTORY__ === "function", + ); + return page.evaluate((count) => { + for (let index = 0; index < 40; index += 1) { + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "general", + content: `visible current ${index}\nsecond line ${index}`, + createdAt: 1_700_000_000 + index, + }); + } + const events = window.__BUZZ_E2E_PREPEND_MOCK_HISTORY__?.({ + channelName: "general", + count, + lineCount: 3, + }); + return (events ?? []).map((event) => event.id); + }, prependCount); +} + +for (const targetCase of CASES) { + test(`message target flashes: ${targetCase.name}`, async ({ page }) => { + const prepended = await seedGeneral(page, targetCase.prependCount); + + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + const timeline = page.getByTestId("message-timeline"); + await expect(timeline).toContainText("visible current 39"); + // The second bottom-pin pass runs one frame after mount; let it land so + // "on screen" and "off screen" are measured against the settled view. + await page.evaluate( + () => + new Promise((resolve) => + requestAnimationFrame(() => requestAnimationFrame(resolve)), + ), + ); + const mounted = await timeline.evaluate((el) => + [...el.querySelectorAll("[data-message-id]")].map( + (row) => row.dataset.messageId ?? "", + ), + ); + const targetId = targetCase.pick({ mounted, prepended }); + expect(targetId).toBeTruthy(); + const generalPath = await page.evaluate( + () => (window.location.hash.replace(/^#/, "") || "/").split("?")[0], + ); + + if (targetCase.fromOtherChannel) { + await page.getByTestId("channel-random").click(); + await expect(page.getByTestId("chat-title")).toHaveText("random"); + } + + // The app uses hash history, so the target lives in the hash fragment. + // Watch from the same task that navigates so a short flash is not missed. + const flashedOnScreen = await page.evaluate( + ([targetId, generalPath, highlightClass]) => + new Promise((resolve) => { + const deadline = performance.now() + 3_000; + const tick = () => { + const timelineEl = document.querySelector( + '[data-testid="message-timeline"]', + ); + const row = timelineEl?.querySelector( + `[data-message-id="${CSS.escape(targetId)}"]`, + ); + const flashing = + row?.closest(`[class*="${highlightClass}"]`) ?? + row?.querySelector(`[class*="${highlightClass}"]`); + if (timelineEl && row && flashing) { + const viewport = timelineEl.getBoundingClientRect(); + const rect = row.getBoundingClientRect(); + if (rect.bottom > viewport.top && rect.top < viewport.bottom) { + resolve(true); + return; + } + } + if (performance.now() > deadline) { + resolve(false); + return; + } + requestAnimationFrame(tick); + }; + const params = new URLSearchParams({ messageId: targetId }); + window.history.pushState( + {}, + "", + `${window.location.pathname}#${generalPath}?${params.toString()}`, + ); + window.dispatchEvent(new HashChangeEvent("hashchange")); + window.dispatchEvent(new PopStateEvent("popstate")); + tick(); + }), + [targetId, generalPath, HIGHLIGHT_CLASS] as const, + ); + + expect(flashedOnScreen).toBe(true); + + // The target must still be on screen after the flash ends. Late layout + // passes (the thread panel opening, history loads, bottom settles) must not + // carry it away. + const stillOnScreen = await page.evaluate( + (targetId) => + new Promise((resolve) => { + setTimeout(() => { + const timelineEl = document.querySelector( + '[data-testid="message-timeline"]', + ); + const row = timelineEl?.querySelector( + `[data-message-id="${CSS.escape(targetId)}"]`, + ); + if (!timelineEl || !row) { + resolve(false); + return; + } + const viewport = timelineEl.getBoundingClientRect(); + const rect = row.getBoundingClientRect(); + resolve(rect.bottom > viewport.top && rect.top < viewport.bottom); + }, 2_200); + }), + targetId, + ); + expect(stillOnScreen).toBe(true); + }); +}