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 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
(cherry picked from commit a2a9767de1b4f01879c652b51e9c10e508678aa6)
This commit is contained in:
mtoohey
2026-11-05 00:28:14 +00:00
parent f2c57afb51
commit c1767345e0
3 changed files with 61 additions and 16 deletions

View File

@@ -140,7 +140,7 @@ afterEach(() => {
container?.remove(); container?.remove();
}); });
async function mountAtFloor() { async function mountAtFloor(initial = history) {
const router = createRouter({ const router = createRouter({
routeTree: createRootRoute(), routeTree: createRootRoute(),
history: createMemoryHistory({ initialEntries: ["/"] }), history: createMemoryHistory({ initialEntries: ["/"] }),
@@ -171,7 +171,7 @@ async function mountAtFloor() {
container = document.createElement("div"); container = document.createElement("div");
document.body.appendChild(container); document.body.appendChild(container);
root = createRoot(container); root = createRoot(container);
await render(history); await render(initial);
const scroller = container.querySelector('[data-testid="message-timeline"]'); const scroller = container.querySelector('[data-testid="message-timeline"]');
act(() => { act(() => {
for (const observer of resizeObservers) { for (const observer of resizeObservers) {
@@ -231,3 +231,35 @@ for (const { name, order } of [
assert.equal(scroller.scrollTop, scroller.scrollHeight - VIEWPORT); 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);
});

View File

@@ -358,7 +358,7 @@ const MessageTimelineBase = React.forwardRef<
onScroll, onScroll,
scrollToBottom, scrollToBottom,
scrollToBottomOnNextUpdate, scrollToBottomOnNextUpdate,
isBottomPinArmed, isOwnSendPending,
scrollToMessage, scrollToMessage,
onVirtualizerAtBottomStateChange, onVirtualizerAtBottomStateChange,
} = useAnchoredScroll({ } = useAnchoredScroll({
@@ -425,17 +425,17 @@ const MessageTimelineBase = React.forwardRef<
} }
} else if (hasConfirmedVirtualizerBottomRef.current) { } else if (hasConfirmedVirtualizerBottomRef.current) {
onVirtualizerAtBottomStateChange(false); onVirtualizerAtBottomStateChange(false);
// An armed own-send pin owns the next append. Freezing the tail first // Until the sent row lands, the own-send pin owns the next append.
// would buffer the sent row, and the pin would never see it arrive. // Freezing the tail first would buffer the sent row, and the pin would
// Virtua reports a stale gap here when a discarded render already // never see it arrive. Virtua reports a stale gap here when a
// counted the sent row in its store. // discarded render already counted the sent row in its store.
if (semanticAtBottomRef.current && !isBottomPinArmed()) { if (semanticAtBottomRef.current && !isOwnSendPending()) {
suppressNextSemanticBottomRef.current = true; suppressNextSemanticBottomRef.current = true;
queueSemanticBottom(false); queueSemanticBottom(false);
} }
} }
}, },
[isBottomPinArmed, onVirtualizerAtBottomStateChange, queueSemanticBottom], [isOwnSendPending, onVirtualizerAtBottomStateChange, queueSemanticBottom],
); );
const hasPendingLiveRows = bufferedTimeline.pendingCount > 0; const hasPendingLiveRows = bufferedTimeline.pendingCount > 0;
React.useEffect(() => { React.useEffect(() => {
@@ -486,12 +486,13 @@ const MessageTimelineBase = React.forwardRef<
// The user's own send is the deliberate Zulip exception: release buffered // 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 // 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 // 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) { if (semanticBottomRafRef.current !== null) {
window.cancelAnimationFrame(semanticBottomRafRef.current); window.cancelAnimationFrame(semanticBottomRafRef.current);
semanticBottomRafRef.current = null; semanticBottomRafRef.current = null;
} }
semanticAtBottomRef.current = true; suppressNextSemanticBottomRef.current = false;
setIsSemanticallyAtBottom(true); setIsSemanticallyAtBottom(true);
scrollToBottomOnNextUpdate(); scrollToBottomOnNextUpdate();
}, [scrollToBottomOnNextUpdate]); }, [scrollToBottomOnNextUpdate]);

View File

@@ -88,8 +88,9 @@ type UseAnchoredScrollResult = {
/** Arm a one-shot scroll-to-bottom that fires on the next appended message /** Arm a one-shot scroll-to-bottom that fires on the next appended message
* (used by the composer's send flow). */ * (used by the composer's send flow). */
scrollToBottomOnNextUpdate: () => void; scrollToBottomOnNextUpdate: () => void;
/** True while that one-shot is armed and its append has not landed. */ /** True from that arming until the first append lands, whether or not the
isBottomPinArmed: () => boolean; * append moves the tail and fires the one-shot. */
isOwnSendPending: () => boolean;
/** Imperative: scroll a specific message into view; optionally pulse it. /** Imperative: scroll a specific message into view; optionally pulse it.
* Returns true if the row was found and scrolled, false otherwise. */ * Returns true if the row was found and scrolled, false otherwise. */
scrollToMessage: ( scrollToMessage: (
@@ -252,6 +253,11 @@ export function useAnchoredScroll({
// appends, we snap to bottom even if they had scrolled up to read history. // 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. // Consumed (and cleared) by the next append in the restoration effect.
const forceBottomOnNextAppendRef = React.useRef(false); 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 // 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` // 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 // ignores transient gaps and keeps chasing the floor. A `ref`, not state — the
@@ -288,6 +294,7 @@ export function useAnchoredScroll({
prevMessagesRef.current = []; prevMessagesRef.current = [];
handledTargetIdRef.current = null; handledTargetIdRef.current = null;
forceBottomOnNextAppendRef.current = false; forceBottomOnNextAppendRef.current = false;
ownSendPendingRef.current = false;
settlingRef.current = false; settlingRef.current = false;
programmaticScrollTopRef.current = null; programmaticScrollTopRef.current = null;
isWritingScrollRef.current = false; isWritingScrollRef.current = false;
@@ -446,9 +453,10 @@ export function useAnchoredScroll({
// outbound message pulls the view down even if they'd scrolled up. // outbound message pulls the view down even if they'd scrolled up.
const scrollToBottomOnNextUpdate = React.useCallback(() => { const scrollToBottomOnNextUpdate = React.useCallback(() => {
forceBottomOnNextAppendRef.current = true; forceBottomOnNextAppendRef.current = true;
ownSendPendingRef.current = true;
}, []); }, []);
const isBottomPinArmed = React.useCallback( const isOwnSendPending = React.useCallback(
() => forceBottomOnNextAppendRef.current, () => ownSendPendingRef.current,
[], [],
); );
@@ -739,6 +747,10 @@ export function useAnchoredScroll({
previous: prevMessages, previous: prevMessages,
}); });
const isPrepend = messageDelta === "prepend"; 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 // One-shot: an outbound send armed `scrollToBottomOnNextUpdate`. When the
// resulting append lands, snap to bottom regardless of the current anchor, // resulting append lands, snap to bottom regardless of the current anchor,
@@ -1065,7 +1077,7 @@ export function useAnchoredScroll({
scrollToBottom: scrollToBottomImperative, scrollToBottom: scrollToBottomImperative,
settleAtBottomAfterLayout, settleAtBottomAfterLayout,
scrollToBottomOnNextUpdate, scrollToBottomOnNextUpdate,
isBottomPinArmed, isOwnSendPending,
scrollToMessage: scrollToMessageImperative, scrollToMessage: scrollToMessageImperative,
onVirtualizerAtBottomStateChange, onVirtualizerAtBottomStateChange,
}; };