From c1767345e0f942e24b7bac27684e2bb0e66d386b Mon Sep 17 00:00:00 2001 From: mtoohey Date: Thu, 5 Nov 2026 00:28:14 +0000 Subject: [PATCH] fix(desktop): end an own send's freeze hold when its row lands Problem: Review of 3c47951dc ("fix(desktop): keep an own send out of the frozen timeline tail") raised two findings in MessageTimeline.tsx. - Warning: the hold on freezing the tail lasted as long as the send's bottom pin, not as long as the send. The pin fires only when the tail id changes (`newLatestArrived` in useAnchoredScroll), and only firing or a channel change cleared it. The window cache sorts rows by created_at, so the sent row doesn't become the tail when the last row has a later created_at (a sender whose clock runs ahead) or a same-second row sorts after it. The pin then stayed armed after the sent row rendered, and a reader who scrolled up never froze the tail. The next arrival to become the tail fired the pin and pulled them to the bottom instead of showing the pill. Before 3c47951dc the freeze buffered that arrival, so a stale pin could not fire under a reader. - Nit: `prepareForOwnMessage` set `semanticAtBottomRef.current = true`, which the hold already made redundant, and cancelling the queued freeze left behind the `suppressNextSemanticBottomRef` it had set. Fix: `useAnchoredScroll` keeps a pending-send flag next to the pin. `scrollToBottomOnNextUpdate` sets both. The restoration effect clears the flag on the first append after arming, whether or not it moves the tail (`newLatestArrived` or an "append" delta), and the channel reset clears it. `isBottomPinArmed` becomes `isOwnSendPending`, which reads the flag, and MessageTimeline holds off freezing only while it is set. The pin itself is unchanged and still waits for a new tail. Once the hold ends, a reader who scrolls up freezes the tail as before, so later arrivals wait behind the pill and a stale pin can't fire under them. This keeps arming and disarming in the hook. Disarming from ChannelPane after `onSendMessage` settles would need a second call site (the welcome add-agent flow also arms the pin), and the send's promise isn't tied to when the sent row reaches the deferred snapshot. `prepareForOwnMessage` now clears `suppressNextSemanticBottomRef` instead of setting the semantic ref, so dropping the queued freeze also drops the suppression it armed. No test covers this: a leftover suppression only swallowed a bottom report while the send's release was already making the timeline semantically at the bottom. Validation: - New case in `MessageTimeline.ownSend.jsdom-test.mjs`: the tail's createdAt is an hour ahead, the send lands above it, the reader scrolls up 200 px, and a row from the same fast clock arrives and becomes the tail. It expects "1 new message" and the reader's offset unchanged. On 3c47951dc it fails: no pill, and the pin moved the reader from 756 to the floor at 1052 and rendered the arrival. With the fix the reader stays at 756 and the arrival waits behind the pill. - Mutations: clearing the flag only on a new tail, or never on an append, fails only the new case. Removing the hold check fails the original "gap after the send" case, as before. - Messages UI jsdom tests (10) and `src/features/messages` unit tests (1530) pass. Full desktop `pnpm test`: 6933 unit and 179 jsdom tests pass. - `tsc --noEmit`, `biome check` on the changed files, `check:file-sizes` and `git diff --check` are clean. No browser E2E was run for this follow-up. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit a2a9767de1b4f01879c652b51e9c10e508678aa6) --- .../ui/MessageTimeline.ownSend.jsdom-test.mjs | 36 +++++++++++++++++-- .../features/messages/ui/MessageTimeline.tsx | 19 +++++----- .../features/messages/ui/useAnchoredScroll.ts | 22 +++++++++--- 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageTimeline.ownSend.jsdom-test.mjs b/desktop/src/features/messages/ui/MessageTimeline.ownSend.jsdom-test.mjs index 7784bea4b8..82b06cf34b 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.ownSend.jsdom-test.mjs +++ b/desktop/src/features/messages/ui/MessageTimeline.ownSend.jsdom-test.mjs @@ -140,7 +140,7 @@ afterEach(() => { container?.remove(); }); -async function mountAtFloor() { +async function mountAtFloor(initial = history) { const router = createRouter({ routeTree: createRootRoute(), history: createMemoryHistory({ initialEntries: ["/"] }), @@ -171,7 +171,7 @@ async function mountAtFloor() { container = document.createElement("div"); document.body.appendChild(container); root = createRoot(container); - await render(history); + await render(initial); const scroller = container.querySelector('[data-testid="message-timeline"]'); act(() => { for (const observer of resizeObservers) { @@ -231,3 +231,35 @@ for (const { name, order } of [ assert.equal(scroller.scrollTop, scroller.scrollHeight - VIEWPORT); }); } + +// Rows arrive sorted by created_at, so a tail from a sender whose clock runs +// ahead stays the tail when the send lands above it, and the pin stays armed. +// The send's row still ends its hold on freezing: a reader who then scrolls up +// gets the next arrival behind the pill, even one that becomes the tail. +test("a reader who scrolls up after a send that lands above the tail keeps their place", async () => { + const earlier = history.slice(0, -1); + const aheadTail = { ...history.at(-1), createdAt: sent.createdAt + 3600 }; + const aheadNext = { ...message(22), createdAt: aheadTail.createdAt + 1 }; + const { render, scroller, timeline } = await mountAtFloor([ + ...earlier, + aheadTail, + ]); + + act(() => timeline.current.scrollToBottomOnNextUpdate()); + await render([...earlier, sent, aheadTail]); + flushFrames(); + assert.ok(container.querySelector(`[data-message-id="${sent.id}"]`)); + + readerScrollsTo(scroller, scroller.scrollTop - 200); + const readingOffset = scroller.scrollTop; + flushFrames(); + await render([...earlier, sent, aheadTail, aheadNext]); + flushFrames(); + + assert.equal( + container.querySelector('[data-testid="message-scroll-to-latest"]') + ?.textContent, + "1 new message", + ); + assert.equal(scroller.scrollTop, readingOffset); +}); diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index 2bb13b01d8..28cdcd63ad 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -358,7 +358,7 @@ const MessageTimelineBase = React.forwardRef< onScroll, scrollToBottom, scrollToBottomOnNextUpdate, - isBottomPinArmed, + isOwnSendPending, scrollToMessage, onVirtualizerAtBottomStateChange, } = useAnchoredScroll({ @@ -425,17 +425,17 @@ const MessageTimelineBase = React.forwardRef< } } else if (hasConfirmedVirtualizerBottomRef.current) { onVirtualizerAtBottomStateChange(false); - // An armed own-send pin owns the next append. Freezing the tail first - // would buffer the sent row, and the pin would never see it arrive. - // Virtua reports a stale gap here when a discarded render already - // counted the sent row in its store. - if (semanticAtBottomRef.current && !isBottomPinArmed()) { + // Until the sent row lands, the own-send pin owns the next append. + // Freezing the tail first would buffer the sent row, and the pin would + // never see it arrive. Virtua reports a stale gap here when a + // discarded render already counted the sent row in its store. + if (semanticAtBottomRef.current && !isOwnSendPending()) { suppressNextSemanticBottomRef.current = true; queueSemanticBottom(false); } } }, - [isBottomPinArmed, onVirtualizerAtBottomStateChange, queueSemanticBottom], + [isOwnSendPending, onVirtualizerAtBottomStateChange, queueSemanticBottom], ); const hasPendingLiveRows = bufferedTimeline.pendingCount > 0; React.useEffect(() => { @@ -486,12 +486,13 @@ const MessageTimelineBase = React.forwardRef< // The user's own send is the deliberate Zulip exception: release buffered // output before arming the next-append bottom pin so the sent row can enter // Virtua's model and become the new physical floor. Drop a freeze queued - // before the send, or it would land after this release. + // before the send, or it would land after this release, and the bottom + // report suppression that freeze armed. if (semanticBottomRafRef.current !== null) { window.cancelAnimationFrame(semanticBottomRafRef.current); semanticBottomRafRef.current = null; } - semanticAtBottomRef.current = true; + suppressNextSemanticBottomRef.current = false; setIsSemanticallyAtBottom(true); scrollToBottomOnNextUpdate(); }, [scrollToBottomOnNextUpdate]); diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.ts b/desktop/src/features/messages/ui/useAnchoredScroll.ts index 8310ac5c9d..e6df41134f 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.ts +++ b/desktop/src/features/messages/ui/useAnchoredScroll.ts @@ -88,8 +88,9 @@ type UseAnchoredScrollResult = { /** Arm a one-shot scroll-to-bottom that fires on the next appended message * (used by the composer's send flow). */ scrollToBottomOnNextUpdate: () => void; - /** True while that one-shot is armed and its append has not landed. */ - isBottomPinArmed: () => boolean; + /** True from that arming until the first append lands, whether or not the + * append moves the tail and fires the one-shot. */ + isOwnSendPending: () => boolean; /** Imperative: scroll a specific message into view; optionally pulse it. * Returns true if the row was found and scrolled, false otherwise. */ scrollToMessage: ( @@ -252,6 +253,11 @@ export function useAnchoredScroll({ // appends, we snap to bottom even if they had scrolled up to read history. // Consumed (and cleared) by the next append in the restoration effect. const forceBottomOnNextAppendRef = React.useRef(false); + // Set with the one-shot, but any append clears it, including one that sorts + // above the current tail (a sender whose clock runs ahead, a same-second + // row) and so leaves the one-shot armed for a later tail. MessageTimeline + // holds its tail open only while this is set. + const ownSendPendingRef = React.useRef(false); // True from a programmatic bottom pin until the list's row measurement settles // and the view reaches a true physical bottom. During this window `onScroll` // ignores transient gaps and keeps chasing the floor. A `ref`, not state — the @@ -288,6 +294,7 @@ export function useAnchoredScroll({ prevMessagesRef.current = []; handledTargetIdRef.current = null; forceBottomOnNextAppendRef.current = false; + ownSendPendingRef.current = false; settlingRef.current = false; programmaticScrollTopRef.current = null; isWritingScrollRef.current = false; @@ -446,9 +453,10 @@ export function useAnchoredScroll({ // outbound message pulls the view down even if they'd scrolled up. const scrollToBottomOnNextUpdate = React.useCallback(() => { forceBottomOnNextAppendRef.current = true; + ownSendPendingRef.current = true; }, []); - const isBottomPinArmed = React.useCallback( - () => forceBottomOnNextAppendRef.current, + const isOwnSendPending = React.useCallback( + () => ownSendPendingRef.current, [], ); @@ -739,6 +747,10 @@ export function useAnchoredScroll({ previous: prevMessages, }); const isPrepend = messageDelta === "prepend"; + // The first append ends a pending send, wherever its row sorts. + if (newLatestArrived || messageDelta === "append") { + ownSendPendingRef.current = false; + } // One-shot: an outbound send armed `scrollToBottomOnNextUpdate`. When the // resulting append lands, snap to bottom regardless of the current anchor, @@ -1065,7 +1077,7 @@ export function useAnchoredScroll({ scrollToBottom: scrollToBottomImperative, settleAtBottomAfterLayout, scrollToBottomOnNextUpdate, - isBottomPinArmed, + isOwnSendPending, scrollToMessage: scrollToMessageImperative, onVirtualizerAtBottomStateChange, };