From 62f0b9821239326cc460c0884599e0a610587a32 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 30 Oct 2026 15:02:53 +1100 Subject: [PATCH 1/2] fix(desktop): hold a deep-linked message against bottom settles message-target-highlight.spec.ts:79 is the most frequent unfixed desktop flake. It failed in all 7 main runs whose SHA carries every earlier flake fix (40969965981, 40970005965, 40970053997, 40980258899, 40980404335, 40980513346, 40983029157), and in 26 jobs since those fixes landed, on main and on PRs #8235, #8239, #8241, #8666, #8680, attempts failed. Only the two cases that pick mounted[2] fail: "same channel, row loaded off screen" (15 jobs) and "other channel, row further up" (11 jobs). The target scrolls in and flashes, then the timeline lands on the newest message, so `stillOnScreen` at :179 reads false (twice it was `flashedOnScreen` at :153). Cause The triage note blamed the thread panel opening. The failure videos from run 40983029157 (Smoke shard 6) show that is a precondition, not the trigger: - Other channel: the panel opens at +0.36 s, the target flashes at +0.7 s, the older page prepends by +1.04 s, the target is still on screen at +1.47 s, and the timeline sits at the exact floor by +1.97 s. - Same channel: the panel and the flash land in the same frame. The snap follows about 0.5 s later, with the load-older spinner still showing. The panel opening never moved the target. A later floor report did: TimelineMessageList's onScroll reporting within 32 px of the bottom, during or after the older-page load that only a mounted[2] target sets off. onVirtualizerAtBottomStateChange(true) reset the anchor to at-bottom and set virtualizerAtBottomRef. With the panel open, the split-panel settle, the viewport-resize settle or the next append settle then pinned the timeline to the floor. The exact-floor landing points at our own settleAtBottom rather than restored scroll. Which Virtua layout pass emits the floor report is still unidentified. Change Accepting a virtual route target now sets targetHoldRef in useAnchoredScroll. Only the two targetMessageId paths set it, on first mount and in the later target effect, by passing holdTarget to scrollToMessage. The public scrollToMessage, which MessageTimeline's oldest-unread pill and find-in-channel stepping call, doesn't hold. It also replaces an earlier hold, so a find jump after a deep link follows the floor as on main. While the hold is set: - shouldSettleForSplitPanel and shouldSettleVirtualizedViewportResize return false; - a floor report still updates isAtBottom and the new-message count, but keeps the message anchor, so live appends don't settle either; - a commit that changes the newest message sets isAtBottom false, clears virtualizerAtBottomRef and adds the rows after the previous newest one to the new-message count. Virtua reports bottom state only from scroll events, and an append below the viewport emits none. Without this, a floor report left isAtBottom true, so MessageTimeline hid the Jump to latest / N new pill while rows piled up out of sight (Review-Proven Rule 6). A realistic trigger is a mention or search hit on one of the last few messages in a busy channel: scrollToIndex clamps at the floor and emits that report. Clearing virtualizerAtBottomRef means a later release doesn't snap past the new rows either. The hold only acts on an arrival, so a reader at the floor with nothing new sees no pill. Counting rows rather than the length change matters: a cold target is merged in older than the head window, and the history page that fills the gap keeps both ends of the list. classifyTimelineMessageDelta calls that an append, but nothing arrived below, so it raises no pill. Neither does deleting the newest message; - viewport reflow re-centres the target through virtualScrollToMessage, from the viewport-resize observer and when the split panel opens or closes. Rows rewrap around the target without a scroll event, so the target can drift, and a floor report from before the reflow can keep isAtBottom true while the newest rows drop below the fold. The re-centring scroll refreshes the bottom state. Virtua's scrollToIndex cancels any earlier one still in flight, so a burst of resizes only restarts its measure loop. The split-panel call fires only on a transition, not on mount or when the virtualizer API registers. The hold is released by reader scroll input (a wheel, a touch drag, a pointerdown on the scroller itself, a scroll key aimed at it), Jump to latest, the reader's own send, and a channel change. The input listeners are the ones that already cancel bottom intent in useVirtualizedBottomSettle, now shared as listenForReaderScroll. A release at the floor restores the at-bottom anchor, so the timeline follows live messages again. onTargetReached does not release it. The artifacts never show the panel pushing the target off screen, so skipping the settle is the fix aimed at the CI failure. The re-centring keeps the target's place and the pill right after a reflow. Known limits: - Programmatic scrolls (focus moves, scrollTop writes) don't release the hold. A find or unread-pill jump replaces it. A resize after such a scroll (Tab focus into a lower row, say) re-centres the target, away from the focused row. - ChannelPane's composer-height settle still calls scrollToBottom within 32 px of the floor, which releases the hold. - Live messages that arrive while the hold is set near the floor are not followed until reader input, Jump to latest or the reader's own send. The pill shows them, with their count. - Under the hold, only rows after the previous newest message count. A live message that sorts just ahead of it (a same-second timestamp) raises no pill until a later row lands. Unit before/after useAnchoredScroll.lifecycle.test.mjs replays the CI sequence: accept a route target, clear it, open the split panel, report the floor, resize the viewport and append a live message. It asserts no bottom settle, and that the panel opening and the resize each re-centre the target. Then it asserts the append shows the pill (isAtBottom false, one new message). A scrollbar pointerdown after that releases the hold, and a resize neither settles nor re-centres. One case per release path asserts the resize settle resumes and the re-centring stops. One of them is a find jump: a public scrollToMessage on a held timeline, then a floor report. A descendant pointerdown (row interaction) keeps the hold, and a resize still re-centres. Reader input at the floor resumes following a live append. Under a hold, a history page filling the gap below the target, and then deleting the newest message, leave isAtBottom true and the count at 0. A commit that adds more history and one live row counts 1. The split-panel and viewport-resize policy tables gain a held row. Mutations over the lifecycle file and both policy tables (29 tests, all pass intact): - hold never set on accept (the old behaviour): 12 fail, including the CI-shaped hold case - split-panel settle ignores the hold: 13 fail - viewport-resize settle ignores the hold: 13 fail - floor report resets a held anchor: 12 fail (the reset anchor is never re-centred) - reader input does not release: 6 fail - Jump to latest, own send, channel change don't release: 1 each - release skips the at-bottom resync: 1 fails - a held append doesn't raise the pill: 2 fail - a held append leaves virtualizerAtBottomRef set: 1 fails - every scrollToMessage holds (this commit's first draft): 1 fails - a public jump keeps an earlier hold: 1 fails - the held-append branch as first drafted (any non-prepend growth, counted by length): 1 fails, the gap-fill case - a newest-id gate but a length-delta count: 1 fails - a missing previous newest counts every row: 1 fails - no rowsBelow > 0 guard: 1 fails - rows counted from the previous newest, off by one: 2 fail - no split-panel re-centre: 12 fail - no viewport-resize re-centre: 12 fail - re-centre ignores the hold: 1 fails pnpm typecheck and pnpm check pass. All 46 tests in the files that import the touched modules pass. Only hosted CI can show Playwright was not run locally. Whether the spec now holds in Smoke shard 6 and in Flake Check's repeats is unverified, and so is the unidentified Virtua floor report. scroll-history.spec.ts and the other timeline specs need the full desktop smoke run to show the hold doesn't regress bottom-follow. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matt Toohey --- .../messages/ui/anchoredScrollPolicy.ts | 4 +- .../ui/useAnchoredScroll.lifecycle.test.mjs | 268 ++++++++++++++++++ .../messages/ui/useAnchoredScroll.test.mjs | 25 +- .../features/messages/ui/useAnchoredScroll.ts | 85 +++++- .../messages/ui/useVirtualizedBottomSettle.ts | 86 +++--- .../ui/useVirtualizedViewportResize.test.mjs | 23 +- .../ui/useVirtualizedViewportResize.ts | 21 +- 7 files changed, 446 insertions(+), 66 deletions(-) diff --git a/desktop/src/features/messages/ui/anchoredScrollPolicy.ts b/desktop/src/features/messages/ui/anchoredScrollPolicy.ts index 5dfec80b836..95b7bdd4a6a 100644 --- a/desktop/src/features/messages/ui/anchoredScrollPolicy.ts +++ b/desktop/src/features/messages/ui/anchoredScrollPolicy.ts @@ -45,11 +45,13 @@ export function settleProgrammaticBottomPin( export function shouldSettleForSplitPanel({ isAtBottom, splitPanelOpen, + targetHeld, }: { isAtBottom: boolean; splitPanelOpen: boolean; + targetHeld: boolean; }): boolean { - return isAtBottom && splitPanelOpen; + return isAtBottom && splitPanelOpen && !targetHeld; } export function shouldSettleVirtualizedBottom({ diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs index 044efae607a..1e4cee18f5c 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs +++ b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs @@ -93,6 +93,10 @@ function installDOMShim() { this === node || this.children.some((child) => child.contains(node)) ); } + + closest() { + return null; + } } class DocumentShim extends EventTargetShim { @@ -694,3 +698,267 @@ test("target arriving before the second mount frame supersedes the bottom pin", globalThis.cancelAnimationFrame = previousCancelRaf; } }); + +function TargetHoldHarness({ fake, onState, ...props }) { + onState( + useAnchoredScroll({ + channelId: "conversation", + contentRef: fake.content, + isLoading: false, + messages: fake.messages, + scrollContainerRef: fake.scroller, + virtualizerOwnsPrependAnchoring: true, + virtualScrollToBottom: fake.scrollToBottom, + virtualScrollToMessage: fake.scrollToMessage, + virtualSettleAtBottom: fake.settleAtBottom, + ...props, + }), + ); + return null; +} + +// Accepts a virtual route target, then replays the layout that carried it +// away in CI: the route clears the target, the thread panel opens on it, and a +// later layout pass reports the virtualizer at its floor. Each reflow +// re-centres the held target rather than settling at the bottom. +async function renderHeldTarget() { + const resizeObservers = []; + globalThis.ResizeObserver = class { + constructor(callback) { + this.callback = callback; + this.targets = []; + resizeObservers.push(this); + } + disconnect() {} + observe(target) { + this.targets.push(target); + } + }; + const scroller = document.createElement("div"); + scroller.querySelector = () => null; + const settles = []; + const targetScrolls = []; + const fake = { + content: { current: null }, + messages: [{ id: "selected" }, { id: "latest" }], + scrollToBottom: () => settles.push("jump"), + scrollToMessage: (messageId) => { + targetScrolls.push(messageId); + return true; + }, + scroller: { current: scroller }, + settleAtBottom: () => settles.push("settle"), + }; + let state = null; + let props = { splitPanelOpen: false, targetMessageId: "selected" }; + const root = createRoot(document.createElement("div")); + const render = async (nextProps) => { + props = { ...props, ...nextProps }; + await act(async () => + root.render( + React.createElement(TargetHoldHarness, { + fake, + onState: (next) => { + state = next; + }, + ...props, + }), + ), + ); + }; + const resizeViewport = async () => { + await act(async () => { + for (const observer of resizeObservers) { + if (observer.targets.includes(scroller)) observer.callback(); + } + }); + }; + + await render({}); + assert.equal(state.isAtBottom, false, "the target is accepted"); + assert.deepEqual(targetScrolls, ["selected"]); + await render({ splitPanelOpen: true, targetMessageId: null }); + assert.equal(targetScrolls.length, 2, "the panel opening re-centres it"); + await act(async () => state.onVirtualizerAtBottomStateChange(true)); + await resizeViewport(); + assert.deepEqual(settles, [], "layout cannot settle a held target"); + assert.deepEqual( + targetScrolls, + ["selected", "selected", "selected"], + "viewport reflow re-centres the held target", + ); + return { + fake, + render, + resizeViewport, + root, + scroller, + settles, + state: () => state, + targetScrolls, + }; +} + +test("an accepted virtual target holds through the thread panel opening and floor reports", async () => { + const { + fake, + render, + resizeViewport, + root, + scroller, + settles, + state, + targetScrolls, + } = await renderHeldTarget(); + + // The floor report still drives the affordances. + assert.equal(state().isAtBottom, true); + fake.messages = [...fake.messages, { id: "live" }]; + await render({}); + assert.deepEqual(settles, [], "a live append cannot settle a held target"); + // The append lands below the held row without a scroll event, so the hook + // itself must bring back the Jump to latest / new-message pill. + assert.equal(state().isAtBottom, false, "the pill shows again"); + assert.equal(state().newMessageCount, 1); + // The reader is no longer at the floor, so taking the viewport back must + // not snap past the new row on the next reflow. + await act(async () => + scroller.dispatchEvent({ target: scroller, type: "pointerdown" }), + ); + await resizeViewport(); + assert.deepEqual(settles, [], "release after the append stays put"); + assert.equal(targetScrolls.length, 3, "and is not re-centred"); + await act(async () => root.unmount()); +}); + +const RELEASES = [ + ["wheel", ({ scroller }) => scroller.dispatchEvent({ type: "wheel" })], + [ + "touch drag", + ({ scroller }) => scroller.dispatchEvent({ type: "touchmove" }), + ], + [ + "scrollbar pointerdown", + ({ scroller }) => + scroller.dispatchEvent({ target: scroller, type: "pointerdown" }), + ], + [ + "scroll key", + ({ scroller }) => + globalThis.dispatchEvent({ + altKey: false, + ctrlKey: false, + key: "PageUp", + metaKey: false, + target: scroller, + type: "keydown", + }), + ], + ["Jump to latest", ({ state }) => state().scrollToBottom("auto")], + [ + "own send", + async ({ fake, render, state }) => { + state().scrollToBottomOnNextUpdate(); + fake.messages = [...fake.messages, { id: "sent" }]; + await render({}); + }, + ], + [ + "channel change", + ({ render }) => render({ channelId: "other", isLoading: true }), + ], + // Find-in-channel and the oldest-unread pill jump through the public + // scrollToMessage. Neither holds, so the next floor report follows again. + [ + "find jump", + ({ state }) => { + state().scrollToMessage("selected", { + behavior: "smooth", + highlight: true, + }); + state().onVirtualizerAtBottomStateChange(true); + }, + ], +]; + +for (const [name, release] of RELEASES) { + test(`${name} releases a held virtual target`, async () => { + const held = await renderHeldTarget(); + await act(async () => release(held)); + const before = held.settles.filter((kind) => kind === "settle").length; + const targetScrollsBefore = held.targetScrolls.length; + await held.resizeViewport(); + + assert.equal( + held.settles.filter((kind) => kind === "settle").length, + before + 1, + "the floor follows viewport reflow again", + ); + assert.equal( + held.targetScrolls.length, + targetScrollsBefore, + "a released target is not re-centred", + ); + await act(async () => held.root.unmount()); + }); +} + +test("a descendant pointerdown keeps a virtual target held", async () => { + const held = await renderHeldTarget(); + const row = document.createElement("div"); + held.scroller.appendChild(row); + await act(async () => + held.scroller.dispatchEvent({ target: row, type: "pointerdown" }), + ); + await held.resizeViewport(); + + assert.deepEqual(held.settles, []); + assert.equal(held.targetScrolls.length, 4, "the target is still re-centred"); + await act(async () => held.root.unmount()); +}); + +test("history filling the gap under a held target is not counted as new", async () => { + const held = await renderHeldTarget(); + // A cold target sits older than the head window. The page that fills the + // gap between them keeps both ends of the list, so nothing arrived below. + held.fake.messages = [ + { id: "selected" }, + { id: "gap-2" }, + { id: "gap-3" }, + { id: "latest" }, + ]; + await held.render({}); + assert.equal(held.state().isAtBottom, true, "the floor report stands"); + assert.equal(held.state().newMessageCount, 0); + + // Deleting the newest row changes the last id, but nothing is new. + held.fake.messages = held.fake.messages.slice(0, -1); + await held.render({}); + assert.equal(held.state().isAtBottom, true, "a deletion raises no pill"); + assert.equal(held.state().newMessageCount, 0); + + // More history and a live row in one commit: only the live row is new. + held.fake.messages = [ + { id: "selected" }, + { id: "gap-1" }, + { id: "gap-2" }, + { id: "gap-3" }, + { id: "live" }, + ]; + await held.render({}); + assert.equal(held.state().isAtBottom, false); + assert.equal(held.state().newMessageCount, 1); + assert.deepEqual(held.settles, []); + await act(async () => held.root.unmount()); +}); + +test("reader input at the floor resumes following live messages", async () => { + const held = await renderHeldTarget(); + // A wheel at the floor moves nothing, so no new floor report follows it. + await act(async () => held.scroller.dispatchEvent({ type: "wheel" })); + held.fake.messages = [...held.fake.messages, { id: "live" }]; + await held.render({}); + + assert.deepEqual(held.settles, ["settle"]); + await act(async () => held.root.unmount()); +}); diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.test.mjs b/desktop/src/features/messages/ui/useAnchoredScroll.test.mjs index 28273f08bb8..aa67eca44dd 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.test.mjs +++ b/desktop/src/features/messages/ui/useAnchoredScroll.test.mjs @@ -24,18 +24,19 @@ function fakeContainer({ clientHeight, scrollHeight, scrollTop }) { } test("split panel settles only an already-bottomed timeline", () => { - assert.equal( - shouldSettleForSplitPanel({ isAtBottom: true, splitPanelOpen: true }), - true, - ); - assert.equal( - shouldSettleForSplitPanel({ isAtBottom: false, splitPanelOpen: true }), - false, - ); - assert.equal( - shouldSettleForSplitPanel({ isAtBottom: true, splitPanelOpen: false }), - false, - ); + for (const [isAtBottom, splitPanelOpen, targetHeld, expected] of [ + [true, true, false, true], + [false, true, false, false], + [true, false, false, false], + // A held navigation target keeps the viewport even at the floor. + [true, true, true, false], + ]) { + assert.equal( + shouldSettleForSplitPanel({ isAtBottom, splitPanelOpen, targetHeld }), + expected, + JSON.stringify({ isAtBottom, splitPanelOpen, targetHeld }), + ); + } }); test("virtualized bottom settle arms for pinned appends and replacements", () => { diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.ts b/desktop/src/features/messages/ui/useAnchoredScroll.ts index 41e0b407863..d5e69da9550 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.ts +++ b/desktop/src/features/messages/ui/useAnchoredScroll.ts @@ -8,6 +8,7 @@ import { shouldSettleForSplitPanel, shouldSettleVirtualizedBottom, } from "./anchoredScrollPolicy"; +import { listenForReaderScroll } from "./useVirtualizedBottomSettle"; import { useVirtualizedViewportResize } from "./useVirtualizedViewportResize"; /** @@ -178,9 +179,21 @@ export function useAnchoredScroll({ // restoration). useState would force re-renders we don't want. const anchorRef = React.useRef({ kind: "at-bottom" }); const virtualizerAtBottomRef = React.useRef(true); + // An accepted virtual route target (targetMessageId) owns the viewport until + // the reader scrolls, sends, jumps to latest, or changes channel. Layout + // passes in between (history prepends, row measurement, the thread panel + // opening) can report the floor; none of them may turn that report into a + // bottom settle. The unread pill and find jumps don't hold. + const targetHoldRef = React.useRef(false); const [isAtBottom, setIsAtBottom] = React.useState(true); React.useLayoutEffect(() => { - if (shouldSettleForSplitPanel({ isAtBottom, splitPanelOpen })) { + if ( + shouldSettleForSplitPanel({ + isAtBottom, + splitPanelOpen, + targetHeld: targetHoldRef.current, + }) + ) { virtualSettleAtBottom?.(); } }, [isAtBottom, splitPanelOpen, virtualSettleAtBottom]); @@ -229,6 +242,7 @@ export function useAnchoredScroll({ React.useLayoutEffect(() => { anchorRef.current = { kind: "at-bottom" }; virtualizerAtBottomRef.current = true; + targetHoldRef.current = false; setIsAtBottom(true); setNewMessageCount(0); setHighlightedMessageId(null); @@ -368,6 +382,7 @@ export function useAnchoredScroll({ const container = scrollContainerRef.current; if (!container) return; anchorRef.current = { kind: "at-bottom" }; + targetHoldRef.current = false; // A programmatic jump-to-bottom is not atomic, even for `behavior: "auto"`: // the browser can emit `scroll` while the list is still settling row // measurements. During that window `computeAnchor` may read the transient @@ -443,7 +458,12 @@ export function useAnchoredScroll({ const scrollToMessageImperative = React.useCallback( ( messageId: string, - options: { highlight?: boolean; behavior?: ScrollBehavior } = {}, + options: { + highlight?: boolean; + behavior?: ScrollBehavior; + /** Route targets only: hold the viewport against bottom settles. */ + holdTarget?: boolean; + } = {}, ): boolean => { const container = scrollContainerRef.current; if (!container) return false; @@ -468,9 +488,12 @@ export function useAnchoredScroll({ // A target that is not loaded yet leaves the bottom pin alone. return false; } - // An accepted target supersedes every pending bottom pin. + // An accepted target supersedes every pending bottom pin. A route + // target also holds the viewport until the reader takes it back; any + // other jump replaces an earlier hold. virtualCancelBottomIntent?.(); cancelMountBottomPin(); + targetHoldRef.current = options.holdTarget === true; anchorRef.current = { kind: "message", messageId, topOffset: 0 }; virtualizerAtBottomRef.current = false; setIsAtBottom(false); @@ -640,6 +663,7 @@ export function useAnchoredScroll({ if ( scrollToMessageImperative(targetMessageId, { highlight: highlightTargetMessage, + holdTarget: true, }) ) { handledTargetIdRef.current = targetMessageId; @@ -684,6 +708,7 @@ export function useAnchoredScroll({ if (newLatestArrived && forceBottomOnNextAppendRef.current) { forceBottomOnNextAppendRef.current = false; anchorRef.current = { kind: "at-bottom" }; + targetHoldRef.current = false; settlingRef.current = true; if (virtualizerOwnsPrependAnchoring && virtualScrollToBottom) { virtualScrollToBottom("auto"); @@ -757,6 +782,22 @@ export function useAnchoredScroll({ if (!isPrepend) { setNewMessageCount((current) => current + messagesArrived); } + } else if (newLatestArrived && targetHoldRef.current) { + // A held target keeps its message anchor through floor reports, so + // isAtBottom can still read true when a live row lands below it. Virtua + // reports bottom state only from scroll events, and an append emits + // none, so raise the new-message pill here. Only rows after the previous + // newest one are new. A history page that fills the gap between a cold + // target and the head window keeps both ends: the list grows, but + // nothing arrived below. + const prevLastIndex = messages.findIndex(({ id }) => id === prevLastId); + const rowsBelow = + prevLastIndex < 0 ? 0 : messages.length - 1 - prevLastIndex; + if (rowsBelow > 0) { + virtualizerAtBottomRef.current = false; + setIsAtBottom(false); + setNewMessageCount((current) => current + rowsBelow); + } } prevLastMessageIdRef.current = lastMessage?.id; @@ -819,12 +860,44 @@ export function useAnchoredScroll({ virtualizerOwnsPrependAnchoring, ]); + // Viewport reflow (the thread panel opening or closing, a window resize) + // rewraps rows around a held target without a scroll event, so neither the + // target's place nor the bottom state would follow. Re-centre the target: + // the scroll that follows refreshes the bottom state too. + const reanchorHeldTarget = React.useCallback(() => { + const anchor = anchorRef.current; + if (!targetHoldRef.current || anchor.kind !== "message") return; + virtualScrollToMessage?.(anchor.messageId); + }, [virtualScrollToMessage]); + const splitPanelOpenRef = React.useRef(splitPanelOpen); + React.useLayoutEffect(() => { + if (splitPanelOpenRef.current === splitPanelOpen) return; + splitPanelOpenRef.current = splitPanelOpen; + reanchorHeldTarget(); + }, [reanchorHeldTarget, splitPanelOpen]); useVirtualizedViewportResize( scrollContainerRef, virtualizerAtBottomRef, + targetHoldRef, virtualizerOwnsPrependAnchoring ? virtualSettleAtBottom : undefined, + reanchorHeldTarget, ); + // Reader scroll input releases a held target. The floor reports that + // arrived while it was held then take effect, so a reader already at the + // bottom follows new messages again. + React.useLayoutEffect(() => { + const container = scrollContainerRef.current; + if (!virtualizerOwnsPrependAnchoring || !container) return; + return listenForReaderScroll(container, () => { + if (!targetHoldRef.current) return; + targetHoldRef.current = false; + if (virtualizerAtBottomRef.current) { + anchorRef.current = { kind: "at-bottom" }; + } + }); + }, [scrollContainerRef, virtualizerOwnsPrependAnchoring]); + // Pinned centers survive our own corrections but release as soon as the // reader deliberately takes control of the scroll position or the caller // retires the temporary target after layout settlement. @@ -898,6 +971,7 @@ export function useAnchoredScroll({ if ( !scrollToMessageImperative(targetMessageId, { highlight: highlightTargetMessage, + holdTarget: true, }) ) { // Not loaded yet. A cold deep-link target is fetched by id and spliced @@ -937,7 +1011,10 @@ export function useAnchoredScroll({ if (!virtualizerOwnsPrependAnchoring) return; virtualizerAtBottomRef.current = atBottom; if (atBottom) { - anchorRef.current = { kind: "at-bottom" }; + // A held target keeps its anchor. The report still updates the + // affordances, but must not become bottom-follow ownership. A live + // append under the hold raises them again (restoration effect). + if (!targetHoldRef.current) anchorRef.current = { kind: "at-bottom" }; setNewMessageCount(0); } setIsAtBottom(atBottom); diff --git a/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts b/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts index 8c0535779f1..46c0e68f40c 100644 --- a/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts +++ b/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts @@ -19,6 +19,53 @@ function isEditableKeyboardTarget(target: EventTarget | null) { ); } +/** + * Calls `onReaderScroll` when the reader takes scroll ownership of + * `scroller`: a wheel, a touch drag, a pointerdown on the scroller itself + * (scrollbar or background), or a scroll key aimed at it. Returns the + * unsubscribe function. + */ +export function listenForReaderScroll( + scroller: HTMLElement, + onReaderScroll: () => void, +): () => void { + const onTouchMove = () => onReaderScroll(); + const onPointerDown = (event: PointerEvent) => { + // A descendant pointerdown is ordinary row interaction (link, reaction, + // thread action), not evidence that the reader took scroll ownership. + // Direct scroller hits cover scrollbar/background drag initiation. + if (event.target === scroller) onReaderScroll(); + }; + const onWheel = (event: WheelEvent) => { + // Ctrl+wheel is browser zoom, not reader navigation. The resulting + // viewport/content reflow must still settle at the new physical floor. + if (!event.ctrlKey) onReaderScroll(); + }; + const onKeyDown = (event: KeyboardEvent) => { + if ( + !event.altKey && + !event.ctrlKey && + !event.metaKey && + event.target instanceof Node && + scroller.contains(event.target) && + !isEditableKeyboardTarget(event.target) && + SCROLL_INTENT_KEYS.has(event.key) + ) { + onReaderScroll(); + } + }; + scroller.addEventListener("pointerdown", onPointerDown, { passive: true }); + scroller.addEventListener("touchmove", onTouchMove, { passive: true }); + scroller.addEventListener("wheel", onWheel, { passive: true }); + window.addEventListener("keydown", onKeyDown, true); + return () => { + scroller.removeEventListener("pointerdown", onPointerDown); + scroller.removeEventListener("touchmove", onTouchMove); + scroller.removeEventListener("wheel", onWheel); + window.removeEventListener("keydown", onKeyDown, true); + }; +} + export function useVirtualizedBottomSettle( hostRef: React.RefObject, listRef: React.RefObject, @@ -58,44 +105,7 @@ export function useVirtualizedBottomSettle( React.useLayoutEffect(() => { const scroller = hostRef.current?.firstElementChild; if (!(scroller instanceof HTMLDivElement)) return; - const retire = () => cancel(); - const retireForPointer = (event: PointerEvent) => { - // A descendant pointerdown is ordinary row interaction (link, reaction, - // thread action), not evidence that the reader took scroll ownership. - // Direct scroller hits cover scrollbar/background drag initiation. - if (event.target === scroller) cancel(); - }; - const retireForWheel = (event: WheelEvent) => { - // Ctrl+wheel is browser zoom, not reader navigation. Keep bottom intent - // armed so the resulting viewport/content reflow can settle at the new - // physical floor. - if (!event.ctrlKey) cancel(); - }; - const retireForScrollKey = (event: KeyboardEvent) => { - if ( - !event.altKey && - !event.ctrlKey && - !event.metaKey && - event.target instanceof Node && - scroller.contains(event.target) && - !isEditableKeyboardTarget(event.target) && - SCROLL_INTENT_KEYS.has(event.key) - ) { - cancel(); - } - }; - scroller.addEventListener("pointerdown", retireForPointer, { - passive: true, - }); - scroller.addEventListener("touchmove", retire, { passive: true }); - scroller.addEventListener("wheel", retireForWheel, { passive: true }); - window.addEventListener("keydown", retireForScrollKey, true); - return () => { - scroller.removeEventListener("pointerdown", retireForPointer); - scroller.removeEventListener("touchmove", retire); - scroller.removeEventListener("wheel", retireForWheel); - window.removeEventListener("keydown", retireForScrollKey, true); - }; + return listenForReaderScroll(scroller, cancel); }, [cancel, hostRef]); React.useLayoutEffect(() => { diff --git a/desktop/src/features/messages/ui/useVirtualizedViewportResize.test.mjs b/desktop/src/features/messages/ui/useVirtualizedViewportResize.test.mjs index e284460c172..cabc5dccb55 100644 --- a/desktop/src/features/messages/ui/useVirtualizedViewportResize.test.mjs +++ b/desktop/src/features/messages/ui/useVirtualizedViewportResize.test.mjs @@ -4,12 +4,19 @@ import test from "node:test"; import { shouldSettleVirtualizedViewportResize } from "./useVirtualizedViewportResize.ts"; test("viewport resize follows the virtualizer's explicit bottom state", () => { - assert.equal( - shouldSettleVirtualizedViewportResize({ virtualizerAtBottom: true }), - true, - ); - assert.equal( - shouldSettleVirtualizedViewportResize({ virtualizerAtBottom: false }), - false, - ); + for (const [virtualizerAtBottom, targetHeld, expected] of [ + [true, false, true], + [false, false, false], + // A held navigation target keeps the viewport even at the floor. + [true, true, false], + ]) { + assert.equal( + shouldSettleVirtualizedViewportResize({ + targetHeld, + virtualizerAtBottom, + }), + expected, + JSON.stringify({ targetHeld, virtualizerAtBottom }), + ); + } }); diff --git a/desktop/src/features/messages/ui/useVirtualizedViewportResize.ts b/desktop/src/features/messages/ui/useVirtualizedViewportResize.ts index d3273a09b62..e8d63c55343 100644 --- a/desktop/src/features/messages/ui/useVirtualizedViewportResize.ts +++ b/desktop/src/features/messages/ui/useVirtualizedViewportResize.ts @@ -1,18 +1,24 @@ import * as React from "react"; export function shouldSettleVirtualizedViewportResize({ + targetHeld, virtualizerAtBottom, }: { + targetHeld: boolean; virtualizerAtBottom: boolean; }): boolean { - return virtualizerAtBottom; + return virtualizerAtBottom && !targetHeld; } -/** Re-settles a bottom-pinned virtualized timeline after viewport reflow. */ +/** Re-settles a bottom-pinned virtualized timeline after viewport reflow. + * While an accepted navigation target holds the viewport, it re-anchors + * that target instead. */ export function useVirtualizedViewportResize( scrollContainerRef: React.RefObject, virtualizerAtBottomRef: React.RefObject, + targetHoldRef: React.RefObject, settleAtBottom?: () => void, + reanchorHeldTarget?: () => void, ) { React.useEffect(() => { const container = scrollContainerRef.current; @@ -27,13 +33,22 @@ export function useVirtualizedViewportResize( const observer = new ResizeObserver(() => { if ( shouldSettleVirtualizedViewportResize({ + targetHeld: targetHoldRef.current, virtualizerAtBottom: virtualizerAtBottomRef.current, }) ) { settleAtBottom(); + } else if (targetHoldRef.current) { + reanchorHeldTarget?.(); } }); observer.observe(container); return () => observer.disconnect(); - }, [scrollContainerRef, settleAtBottom, virtualizerAtBottomRef]); + }, [ + reanchorHeldTarget, + scrollContainerRef, + settleAtBottom, + targetHoldRef, + virtualizerAtBottomRef, + ]); } From 89a20681c47e477ab3f41e4940524e1cd47be7eb Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 2 Nov 2026 23:13:27 +1100 Subject: [PATCH 2/2] fix(desktop): keep a held deep link through row Space and same-second arrivals Addresses jedwards27's two review findings on #8742. The Codex Security Review comment raised the same issue as finding 2. Space on a row control released the held target listenForReaderScroll counted Space as reader scroll for any non-editable keydown inside the scroller. Space on a focused row action (Reply, say) activates the button, but it also released targetHoldRef. With a floor report already in, the next panel toggle, resize or append settled the timeline at the bottom and carried the deep-linked message away. Mouse activation never took this path (Review-Proven Rule 8). Space now counts only when it is aimed at something Space scrolls. Buttons, summary, and the button, checkbox, menuitem*, option, radio, switch and tab roles consume it. Editable targets still consume every scroll key, as before. Other scroll keys on a focused control (PageDown on Reply) still release, because the browser scrolls the scroller for them. Any scroll key aimed at the scroller or row text still releases too. A focused link is left out on purpose: links activate on Enter, and Space on one scrolls. useVirtualizedBottomSettle cancels bottom intent from the same listener, so Space on a row control no longer cancels a pending bottom pin either. Same-second arrivals under a hold raised no pill The held-target pill branch ran only when the last id changed, and counted the rows after the previous newest one. A live row stamped in the newest row's second with a larger id sorts ahead of it (compareChronologicalOrder), so the last id stays the same. After a floor report, isAtBottom stayed true and the count stayed 0, so nothing showed the row below the fold. The { id }-only seam can't tell that row from a history page filling the gap below a cold target: both insert ahead of the previous newest row. useAnchoredScroll's messages now carry the optional createdAt and renderKey that MessageTimeline rows already have. While the hold is set, every commit counts the new rows (keyed by renderKey ?? id) stamped in the previous newest row's second or later. Gap fills and older pages are stamped earlier, and a deletion adds nothing. Keying by renderKey stops an optimistic send's ack, which changes its id, from counting twice. The comparison reads the newest row from the previous list, so deleting it in the commit a live row lands in now counts the live row. Agent review had flagged that case, which counted 0. Remaining limits: a live row stamped earlier than the newest row (sender clock skew) still raises no pill, and a gap-fill row would count if the whole head window shared one second. Rows without createdAt never count, but only MessageTimeline holds a target, and its rows always carry one. Tests The lifecycle DOM shim gains a strict matches/closest for tag, [attr] and [attr='value'] selectors that throws on anything else, so the Space cases bind to the real selector list. - Keeps (table, each followed by a resize and a live append on top of the floor report): a descendant pointerdown, Space on a focused button, Space on a focused [role='menuitem']. Nothing settles, the target is re-centred, and the append counts 1. - Releases (table): PageUp and End on the scroller, Space on row text, PageDown on a focused button and Space on a focused link join the existing paths. - A new lifecycle case: held target, floor report, then [selected, same-second, latest] gives isAtBottom false and 1 new. Deleting the previous newest row as a live row lands gives 2. An un-armed send and its id-changing ack give 3. Nothing settles throughout. - The held fixtures carry createdAt. Mutations over the lifecycle, bottom-settle, anchored-scroll policy and viewport-resize tests (44 tests, all pass intact): - Space on any control releases (the old behaviour): 2 fail - every scroll key on a control is kept: 1 fails - Space anywhere in the scroller is kept: 2 fail - "button" dropped from the list: 1 fails - the menuitem role dropped from the list: 1 fails - a[href] added to the list: 1 fails - the old held branch (tail-id gate, rows after the previous newest): 1 fails, the same-second case - the tail-id gate with the new counter: 1 fails - no timestamp filter: 1 fails, the gap-fill case - > instead of >=: 1 fails - keyed by id, not renderKey: 1 fails - the previous newest must survive: 1 fails pnpm typecheck and pnpm check pass. All 62 tests in the files that import the touched modules pass. Playwright was not run locally. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matt Toohey --- .../ui/useAnchoredScroll.lifecycle.test.mjs | 187 +++++++++++++++--- .../features/messages/ui/useAnchoredScroll.ts | 49 ++++- .../messages/ui/useVirtualizedBottomSettle.ts | 35 +++- 3 files changed, 222 insertions(+), 49 deletions(-) diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs index 1e4cee18f5c..dd0f4160d7a 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs +++ b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs @@ -94,7 +94,37 @@ function installDOMShim() { ); } - closest() { + setAttribute(name, value) { + this.attrs ??= new Map(); + this.attrs.set(name, String(value)); + } + + getAttribute(name) { + return this.attrs?.get(name) ?? null; + } + + // Supports `tag`, `[attr]` and `[attr='value']` compounds in a comma list, + // and throws on anything else rather than silently not matching. + matches(selectors) { + const attribute = /\[([\w-]+)(?:='([^']*)')?\]/g; + return selectors.split(",").some((selector) => { + const [, tag, attributes] = /^\s*([\w-]*)(.*?)\s*$/.exec(selector); + if (attributes.replace(attribute, "") !== "") { + throw new Error(`unsupported selector: ${selector}`); + } + if (tag && tag !== this.tagName) return false; + return [...attributes.matchAll(attribute)].every(([, name, value]) => + value === undefined + ? this.getAttribute(name) !== null + : this.getAttribute(name) === value, + ); + }); + } + + closest(selectors) { + for (let node = this; node instanceof NodeShim; node = node.parentNode) { + if (node.matches(selectors)) return node; + } return null; } } @@ -740,7 +770,11 @@ async function renderHeldTarget() { const targetScrolls = []; const fake = { content: { current: null }, - messages: [{ id: "selected" }, { id: "latest" }], + // MessageTimeline rows carry their timestamp (Unix seconds). + messages: [ + { createdAt: 100, id: "selected" }, + { createdAt: 500, id: "latest" }, + ], scrollToBottom: () => settles.push("jump"), scrollToMessage: (messageId) => { targetScrolls.push(messageId); @@ -813,7 +847,7 @@ test("an accepted virtual target holds through the thread panel opening and floo // The floor report still drives the affordances. assert.equal(state().isAtBottom, true); - fake.messages = [...fake.messages, { id: "live" }]; + fake.messages = [...fake.messages, { createdAt: 600, id: "live" }]; await render({}); assert.deepEqual(settles, [], "a live append cannot settle a held target"); // The append lands below the held row without a scroll event, so the hook @@ -831,6 +865,29 @@ test("an accepted virtual target holds through the thread panel opening and floo await act(async () => root.unmount()); }); +// Mounts `tagName` inside a row in the scroller, as a focus or event target. +function mountInRow(scroller, tagName, attributes = {}) { + const row = document.createElement("div"); + const element = document.createElement(tagName); + for (const [name, value] of Object.entries(attributes)) { + element.setAttribute(name, value); + } + row.appendChild(element); + scroller.appendChild(row); + return element; +} + +function pressKey(key, target) { + globalThis.dispatchEvent({ + altKey: false, + ctrlKey: false, + key, + metaKey: false, + target, + type: "keydown", + }); +} + const RELEASES = [ ["wheel", ({ scroller }) => scroller.dispatchEvent({ type: "wheel" })], [ @@ -842,24 +899,27 @@ const RELEASES = [ ({ scroller }) => scroller.dispatchEvent({ target: scroller, type: "pointerdown" }), ], + ["PageUp on the scroller", ({ scroller }) => pressKey("PageUp", scroller)], + ["End on the scroller", ({ scroller }) => pressKey("End", scroller)], [ - "scroll key", - ({ scroller }) => - globalThis.dispatchEvent({ - altKey: false, - ctrlKey: false, - key: "PageUp", - metaKey: false, - target: scroller, - type: "keydown", - }), + "Space on row text", + ({ scroller }) => pressKey(" ", mountInRow(scroller, "p")), + ], + // The browser scrolls the scroller for these, not the focused control. + [ + "PageDown on a focused row action", + ({ scroller }) => pressKey("PageDown", mountInRow(scroller, "button")), + ], + [ + "Space on a focused link", + ({ scroller }) => pressKey(" ", mountInRow(scroller, "a", { href: "#" })), ], ["Jump to latest", ({ state }) => state().scrollToBottom("auto")], [ "own send", async ({ fake, render, state }) => { state().scrollToBottomOnNextUpdate(); - fake.messages = [...fake.messages, { id: "sent" }]; + fake.messages = [...fake.messages, { createdAt: 600, id: "sent" }]; await render({}); }, ], @@ -903,17 +963,80 @@ for (const [name, release] of RELEASES) { }); } -test("a descendant pointerdown keeps a virtual target held", async () => { +// Row interaction, not reader scroll. Space activates these controls. +const KEEPS = [ + [ + "a descendant pointerdown", + ({ scroller }) => + scroller.dispatchEvent({ + target: mountInRow(scroller, "div"), + type: "pointerdown", + }), + ], + [ + "Space on a focused row action", + ({ scroller }) => pressKey(" ", mountInRow(scroller, "button")), + ], + [ + "Space on a focused menu item", + ({ scroller }) => + pressKey(" ", mountInRow(scroller, "div", { role: "menuitem" })), + ], +]; + +for (const [name, interact] of KEEPS) { + test(`${name} keeps a virtual target held`, async () => { + const held = await renderHeldTarget(); + await act(async () => interact(held)); + await held.resizeViewport(); + assert.deepEqual(held.settles, []); + assert.equal(held.targetScrolls.length, 4, "the target is re-centred"); + + held.fake.messages = [ + ...held.fake.messages, + { createdAt: 600, id: "live" }, + ]; + await held.render({}); + assert.deepEqual(held.settles, [], "a live append cannot settle it"); + assert.equal(held.state().newMessageCount, 1); + await act(async () => held.root.unmount()); + }); +} + +test("a same-second live row under a held target raises the pill", async () => { const held = await renderHeldTarget(); - const row = document.createElement("div"); - held.scroller.appendChild(row); - await act(async () => - held.scroller.dispatchEvent({ target: row, type: "pointerdown" }), - ); - await held.resizeViewport(); + // The floor report left isAtBottom true. A row stamped in the newest row's + // second with a larger id sorts ahead of it, so the last id is unchanged. + held.fake.messages = [ + { createdAt: 100, id: "selected" }, + { createdAt: 500, id: "same-second" }, + { createdAt: 500, id: "latest" }, + ]; + await held.render({}); + assert.equal(held.state().isAtBottom, false, "the pill shows"); + assert.equal(held.state().newMessageCount, 1); + // The previous newest row is deleted in the commit a live row lands in. + held.fake.messages = [ + { createdAt: 100, id: "selected" }, + { createdAt: 500, id: "same-second" }, + { createdAt: 600, id: "live" }, + ]; + await held.render({}); + assert.equal(held.state().newMessageCount, 2); + + // A send that doesn't arm the bottom follow (a thread reply also sent to + // the channel) counts once. Its relay ack changes the id, not the row key. + const optimistic = { createdAt: 601, id: "local", renderKey: "local" }; + held.fake.messages = [...held.fake.messages, optimistic]; + await held.render({}); + held.fake.messages = [ + ...held.fake.messages.slice(0, -1), + { ...optimistic, id: "acked" }, + ]; + await held.render({}); + assert.equal(held.state().newMessageCount, 3); assert.deepEqual(held.settles, []); - assert.equal(held.targetScrolls.length, 4, "the target is still re-centred"); await act(async () => held.root.unmount()); }); @@ -922,10 +1045,10 @@ test("history filling the gap under a held target is not counted as new", async // A cold target sits older than the head window. The page that fills the // gap between them keeps both ends of the list, so nothing arrived below. held.fake.messages = [ - { id: "selected" }, - { id: "gap-2" }, - { id: "gap-3" }, - { id: "latest" }, + { createdAt: 100, id: "selected" }, + { createdAt: 200, id: "gap-2" }, + { createdAt: 300, id: "gap-3" }, + { createdAt: 500, id: "latest" }, ]; await held.render({}); assert.equal(held.state().isAtBottom, true, "the floor report stands"); @@ -939,11 +1062,11 @@ test("history filling the gap under a held target is not counted as new", async // More history and a live row in one commit: only the live row is new. held.fake.messages = [ - { id: "selected" }, - { id: "gap-1" }, - { id: "gap-2" }, - { id: "gap-3" }, - { id: "live" }, + { createdAt: 100, id: "selected" }, + { createdAt: 150, id: "gap-1" }, + { createdAt: 200, id: "gap-2" }, + { createdAt: 300, id: "gap-3" }, + { createdAt: 600, id: "live" }, ]; await held.render({}); assert.equal(held.state().isAtBottom, false); @@ -956,7 +1079,7 @@ test("reader input at the floor resumes following live messages", async () => { const held = await renderHeldTarget(); // A wheel at the floor moves nothing, so no new floor report follows it. await act(async () => held.scroller.dispatchEvent({ type: "wheel" })); - held.fake.messages = [...held.fake.messages, { id: "live" }]; + held.fake.messages = [...held.fake.messages, { createdAt: 600, id: "live" }]; await held.render({}); assert.deepEqual(held.settles, ["settle"]); diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.ts b/desktop/src/features/messages/ui/useAnchoredScroll.ts index d5e69da9550..63e67ce763a 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.ts +++ b/desktop/src/features/messages/ui/useAnchoredScroll.ts @@ -24,6 +24,14 @@ type AnchorState = | { kind: "message"; messageId: string; topOffset: number } | { kind: "pinned-center"; messageId: string; contentTop: number }; +type AnchoredMessage = { + id: string; + /** Unix seconds. Lets a held route target count live arrivals. */ + createdAt?: number; + /** Identity that survives an optimistic send's relay ack. */ + renderKey?: string; +}; + type UseAnchoredScrollOptions = { /** Scroll container. Owned by the parent so external refs still compose. */ scrollContainerRef: React.RefObject; @@ -36,7 +44,7 @@ type UseAnchoredScrollOptions = { isLoading: boolean; /** Source of truth for the rendered list. Used to detect new-at-bottom * arrivals and to seed/refresh the anchor pre-render. */ - messages: Array<{ id: string }>; + messages: AnchoredMessage[]; splitPanelOpen?: boolean; /** When set, scroll to this message on mount and on change. */ @@ -154,6 +162,31 @@ function computeAnchor( return { kind: "at-bottom" }; } +/** + * Counts the rows in `current` that arrived live since `previous`: new rows + * stamped in the previous newest row's second or later. A same-second arrival + * can sort ahead of that row and leave the last id unchanged, so position + * can't tell. History only adds older rows, including a page that fills the + * gap between a cold target and the head window. Rows without `createdAt` + * never count, and a live row stamped earlier (sender clock skew) doesn't + * either. + */ +function countLiveArrivals( + previous: readonly AnchoredMessage[], + current: readonly AnchoredMessage[], +) { + const newest = previous[previous.length - 1]?.createdAt; + if (newest === undefined) return 0; + const rowKey = (message: AnchoredMessage) => message.renderKey ?? message.id; + const previousKeys = new Set(previous.map(rowKey)); + return current.filter( + (message) => + message.createdAt !== undefined && + message.createdAt >= newest && + !previousKeys.has(rowKey(message)), + ).length; +} + export function useAnchoredScroll({ scrollContainerRef, contentRef, @@ -206,7 +239,7 @@ export function useAnchoredScroll({ const prevLastMessageIdRef = React.useRef(undefined); const prevFirstMessageIdRef = React.useRef(undefined); const prevMessageCountRef = React.useRef(0); - const prevMessagesRef = React.useRef>([]); + const prevMessagesRef = React.useRef([]); const handledTargetIdRef = React.useRef(null); const highlightTimeoutRef = React.useRef(null); // Tracks a pending rAF queued by pinToBottomOnMount so it can be cancelled @@ -782,17 +815,13 @@ export function useAnchoredScroll({ if (!isPrepend) { setNewMessageCount((current) => current + messagesArrived); } - } else if (newLatestArrived && targetHoldRef.current) { + } else if (targetHoldRef.current && messages !== prevMessages) { // A held target keeps its message anchor through floor reports, so // isAtBottom can still read true when a live row lands below it. Virtua // reports bottom state only from scroll events, and an append emits - // none, so raise the new-message pill here. Only rows after the previous - // newest one are new. A history page that fills the gap between a cold - // target and the head window keeps both ends: the list grows, but - // nothing arrived below. - const prevLastIndex = messages.findIndex(({ id }) => id === prevLastId); - const rowsBelow = - prevLastIndex < 0 ? 0 : messages.length - 1 - prevLastIndex; + // none, so raise the new-message pill here. A same-second arrival can + // land without changing the last id, so count arrivals by timestamp. + const rowsBelow = countLiveArrivals(prevMessages, messages); if (rowsBelow > 0) { virtualizerAtBottomRef.current = false; setIsAtBottom(false); diff --git a/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts b/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts index 46c0e68f40c..926e47704fb 100644 --- a/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts +++ b/desktop/src/features/messages/ui/useVirtualizedBottomSettle.ts @@ -11,19 +11,40 @@ const SCROLL_INTENT_KEYS = new Set([ " ", ]); -function isEditableKeyboardTarget(target: EventTarget | null) { +// Space activates these controls instead of scrolling. A focused link is not +// one: links activate on Enter, and Space scrolls past them. +const SPACE_ACTIVATED_CONTROLS = [ + "button", + "summary", + "[role='button']", + "[role='checkbox']", + "[role='menuitem']", + "[role='menuitemcheckbox']", + "[role='menuitemradio']", + "[role='option']", + "[role='radio']", + "[role='switch']", + "[role='tab']", +].join(", "); + +/** True when the focused control consumes `key` rather than scrolling. */ +function isControlKeyboardTarget(target: EventTarget | null, key: string) { if (!(target instanceof HTMLElement)) return false; if (target.isContentEditable) return true; - return ( + if ( target.closest("input, textarea, select, [contenteditable='true']") !== null - ); + ) { + return true; + } + return key === " " && target.closest(SPACE_ACTIVATED_CONTROLS) !== null; } /** * Calls `onReaderScroll` when the reader takes scroll ownership of * `scroller`: a wheel, a touch drag, a pointerdown on the scroller itself - * (scrollbar or background), or a scroll key aimed at it. Returns the - * unsubscribe function. + * (scrollbar or background), or a scroll key aimed at it or at row content. + * Space on a row control (Reply, a reaction) activates it and doesn't count. + * Returns the unsubscribe function. */ export function listenForReaderScroll( scroller: HTMLElement, @@ -48,8 +69,8 @@ export function listenForReaderScroll( !event.metaKey && event.target instanceof Node && scroller.contains(event.target) && - !isEditableKeyboardTarget(event.target) && - SCROLL_INTENT_KEYS.has(event.key) + SCROLL_INTENT_KEYS.has(event.key) && + !isControlKeyboardTarget(event.target, event.key) ) { onReaderScroll(); }