mirror of
https://github.com/block/buzz.git
synced 2026-11-06 09:21:45 +00:00
fix(desktop): keep an own send out of the frozen timeline tail
Problem: `messaging.spec.ts:483` "long autolink wraps without widening the timeline" flaked in Desktop Smoke E2E at :497: the timeline never showed the sent message. The composer was empty, and a "1 new message" pill sat at the bottom of a timeline that was already at the floor. - seiler/BUZZ-517/desktop-upgrade-guidance: https://github.com/block/buzz/actions/runs/41751914119/job/126319694104 (retry 1 passed) The failure video shows the composer filling while the channel still showed its skeleton, the rows rendering under the tall composer, and the send landing about 120 ms later. Cause: The timeline freezes its live tail when Virtua reports that the reader has left the bottom, and later rows wait behind the pill. A send lifts the freeze and arms a one-shot pin for the next append (`prepareForOwnMessage`). Nothing kept a not-at-bottom report between the send and the sent row reaching the deferred snapshot from freezing the tail again. The sent row then waited behind the pill, and the pin, which waits for that append, never fired. Only a later scroll to the floor would have released it. Virtua 0.49.3 makes that report while the reader sits at the floor. It applies a list length change to its store during render (Virtualizer.tsx). React discarded the deferred render that added the sent row, but the store kept the row's estimated height. When the cleared composer shrank the trailing spacer, the next scroll event put the reader 96 px from a floor the DOM didn't have: Virtua read 941 - 479 - 366 = 96, the DOM read 917 - 551 - 366 = 0. The freeze it queued renders at default priority, ahead of the deferred render, which then restarted against the frozen tail. Fix: While the send's pin is armed, a not-at-bottom report doesn't freeze the tail. `useAnchoredScroll` exposes `isBottomPinArmed()`, which reads the pin's own flag, so the hold ends when the pin fires or the channel changes. `prepareForOwnMessage` also cancels a freeze queued before the send, which would otherwise land after its release. Bottom state still comes from Virtua. Reading it from the DOM would add false gaps of its own, because unmeasured rows overflow their slots: a passing run read 51 px from the DOM where Virtua read 0. A scroll up between the send and its row no longer freezes the tail. The pin brings the reader down to the sent row, as it already does when the row lands first. A send that adds no row leaves the pin armed until the next append, as before, and the hold follows it. Validation: - New `MessageTimeline.ownSend.jsdom-test.mjs` renders the real MessageTimeline over Virtua with a modelled layout and manual frames. It sends with a gap reported after the send, and with a freeze already queued at the send. The sent row must render at the floor with no pill. Without the pin guard the first case fails, and without the cancel the second does, each showing "1 new message". - Repro, using temporary, uncommitted changes: the test with a 600 ms channel-head delay, filling during the skeleton, sending once rows render, with CPU throttled 4x over CDP. Without the fix: 3/20 failed with the CI signature, and instrumentation showed the 96 px report above. With the fix: 40/40 passed, and 6 of those runs logged a not-at-bottom report while the pin was armed. - `messaging.spec.ts --grep "long autolink"`: 30/30 passed at 4 workers, 10/10 serially. - messaging, virtualization, scroll-history, stream, smoke, thread-follow, file-attachment and message-target-highlight specs, run twice at 4 workers: 185 passed and 1 failed each time, a different test each run. Repeating those two fails as often without the fix: `:2785` (own avatar) 22/25 vs 20/25 at 4 workers and 8/10 vs 9/10 serially; `:3170` (thread panel width) 4/25 vs 8/25 and 1/10 vs 3/10. `:2785` fails on the avatar image after its message has rendered, and `:3170` never sends, so the change can't reach it. - `tsc --noEmit`, `biome check`, and the messages UI unit (337) and jsdom (9) tests pass. A green run doesn't prove the flake is gone. Hosted CI covers broad validation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> (cherry picked from commit 4c9272a362eb6f01aacb8d0133fbe41066eaf308)
This commit is contained in:
@@ -0,0 +1,233 @@
|
||||
/**
|
||||
* The user's own send reaches the bottom of the timeline even when Virtua
|
||||
* reports a gap from the floor around it.
|
||||
*
|
||||
* MessageTimeline freezes its live tail when Virtua reports that the reader
|
||||
* has left the bottom, and a send arms a one-shot pin for the next append. If
|
||||
* the freeze lands before the sent row reaches the deferred snapshot, the row
|
||||
* waits behind a "1 new message" pill, and the pin never sees its append.
|
||||
* Virtua can report that gap while the reader is at the floor: it counts rows
|
||||
* in its store during render, so a deferred render that React discards leaves
|
||||
* the sent row counted. The next scroll event, from the composer shrinking
|
||||
* after the send, then reads that row's height as distance from the bottom.
|
||||
*/
|
||||
import assert from "node:assert/strict";
|
||||
import { afterEach, test } from "node:test";
|
||||
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import {
|
||||
RouterContextProvider,
|
||||
createMemoryHistory,
|
||||
createRootRoute,
|
||||
createRouter,
|
||||
} from "@tanstack/react-router";
|
||||
import React, { act } from "react";
|
||||
import { createRoot } from "react-dom/client";
|
||||
|
||||
import { MessageTimeline } from "./MessageTimeline.tsx";
|
||||
|
||||
const ROW = 60;
|
||||
const VIEWPORT = 400;
|
||||
|
||||
// jsdom has no layout. Model what the timeline reads: the scroller's offset
|
||||
// is clamped to the height Virtua gives its inner container, and the list
|
||||
// host is as tall as the scroller so Virtua keeps an offscreen buffer.
|
||||
const offsets = new WeakMap();
|
||||
const isScroller = (element) => element?.style?.overflowY === "auto";
|
||||
const maxOffset = (element) =>
|
||||
Math.max(
|
||||
(Number.parseFloat(element.firstElementChild?.style.height) || 0) -
|
||||
VIEWPORT,
|
||||
0,
|
||||
);
|
||||
const clampOffset = (element, value) =>
|
||||
Math.min(Math.max(value, 0), maxOffset(element));
|
||||
Object.defineProperties(window.HTMLElement.prototype, {
|
||||
clientHeight: {
|
||||
configurable: true,
|
||||
get() {
|
||||
return isScroller(this) || isScroller(this.firstElementChild)
|
||||
? VIEWPORT
|
||||
: 0;
|
||||
},
|
||||
},
|
||||
scrollHeight: {
|
||||
configurable: true,
|
||||
get() {
|
||||
return isScroller(this) ? maxOffset(this) + VIEWPORT : 0;
|
||||
},
|
||||
},
|
||||
scrollTop: {
|
||||
configurable: true,
|
||||
get() {
|
||||
const offset = clampOffset(this, offsets.get(this) ?? 0);
|
||||
offsets.set(this, offset);
|
||||
return offset;
|
||||
},
|
||||
set(value) {
|
||||
offsets.set(this, clampOffset(this, value));
|
||||
},
|
||||
},
|
||||
offsetParent: {
|
||||
configurable: true,
|
||||
get() {
|
||||
return this.parentElement;
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const resizeObservers = new Set();
|
||||
globalThis.ResizeObserver = window.ResizeObserver = class {
|
||||
constructor(callback) {
|
||||
this.callback = callback;
|
||||
this.targets = new Set();
|
||||
resizeObservers.add(this);
|
||||
}
|
||||
observe(target) {
|
||||
this.targets.add(target);
|
||||
}
|
||||
unobserve(target) {
|
||||
this.targets.delete(target);
|
||||
}
|
||||
disconnect() {
|
||||
resizeObservers.delete(this);
|
||||
}
|
||||
};
|
||||
|
||||
// Frames run only when the test flushes them, so it decides whether a queued
|
||||
// freeze lands before or after the send.
|
||||
let frames = new Map();
|
||||
let nextFrameId = 1;
|
||||
globalThis.requestAnimationFrame = window.requestAnimationFrame = (
|
||||
callback,
|
||||
) => {
|
||||
const id = nextFrameId++;
|
||||
frames.set(id, callback);
|
||||
return id;
|
||||
};
|
||||
globalThis.cancelAnimationFrame = window.cancelAnimationFrame = (id) => {
|
||||
frames.delete(id);
|
||||
};
|
||||
function flushFrames() {
|
||||
act(() => {
|
||||
const pending = frames;
|
||||
frames = new Map();
|
||||
for (const callback of pending.values()) callback(performance.now());
|
||||
});
|
||||
}
|
||||
|
||||
function message(index, body = `message ${index}`) {
|
||||
return {
|
||||
id: index.toString(16).padStart(64, "0"),
|
||||
createdAt: 1_700_000_000 + index,
|
||||
pubkey: "a".repeat(64),
|
||||
author: "alice",
|
||||
time: "1:00 AM",
|
||||
body,
|
||||
depth: 0,
|
||||
kind: 9,
|
||||
tags: [],
|
||||
};
|
||||
}
|
||||
|
||||
const history = Array.from({ length: 20 }, (_, index) => message(index + 1));
|
||||
const sent = message(21, "my own send");
|
||||
|
||||
let root;
|
||||
let container;
|
||||
afterEach(() => {
|
||||
act(() => root?.unmount());
|
||||
container?.remove();
|
||||
});
|
||||
|
||||
async function mountAtFloor() {
|
||||
const router = createRouter({
|
||||
routeTree: createRootRoute(),
|
||||
history: createMemoryHistory({ initialEntries: ["/"] }),
|
||||
});
|
||||
const queryClient = new QueryClient({
|
||||
defaultOptions: { queries: { enabled: false } },
|
||||
});
|
||||
const timeline = React.createRef();
|
||||
const render = (messages) =>
|
||||
act(async () =>
|
||||
root.render(
|
||||
React.createElement(
|
||||
RouterContextProvider,
|
||||
{ router },
|
||||
React.createElement(
|
||||
QueryClientProvider,
|
||||
{ client: queryClient },
|
||||
React.createElement(MessageTimeline, {
|
||||
channelId: "channel",
|
||||
messages,
|
||||
ref: timeline,
|
||||
}),
|
||||
),
|
||||
),
|
||||
),
|
||||
);
|
||||
|
||||
container = document.createElement("div");
|
||||
document.body.appendChild(container);
|
||||
root = createRoot(container);
|
||||
await render(history);
|
||||
const scroller = container.querySelector('[data-testid="message-timeline"]');
|
||||
act(() => {
|
||||
for (const observer of resizeObservers) {
|
||||
observer.callback(
|
||||
[...observer.targets].map((target) => ({
|
||||
target,
|
||||
contentRect: {
|
||||
width: 600,
|
||||
height: target === scroller ? VIEWPORT : ROW,
|
||||
},
|
||||
})),
|
||||
);
|
||||
}
|
||||
});
|
||||
flushFrames();
|
||||
readerScrollsTo(scroller, scroller.scrollHeight);
|
||||
flushFrames();
|
||||
return { render, scroller, timeline };
|
||||
}
|
||||
|
||||
function readerScrollsTo(scroller, offset) {
|
||||
act(() => {
|
||||
scroller.scrollTop = offset;
|
||||
scroller.dispatchEvent(new window.Event("scroll"));
|
||||
});
|
||||
}
|
||||
|
||||
// The guard cannot tell Virtua's stale gap from a real one, so a real 200px
|
||||
// gap stands in for it. Either way, the send pulls the view to its row.
|
||||
for (const { name, order } of [
|
||||
{ name: "after the send", order: ["send", "gap"] },
|
||||
{ name: "with its freeze still queued at the send", order: ["gap", "send"] },
|
||||
]) {
|
||||
test(`an own send reaches the floor when Virtua reports a gap ${name}`, async () => {
|
||||
const { render, scroller, timeline } = await mountAtFloor();
|
||||
assert.equal(
|
||||
container.querySelector('[data-testid="message-scroll-to-latest"]'),
|
||||
null,
|
||||
);
|
||||
|
||||
for (const step of order) {
|
||||
if (step === "send") {
|
||||
act(() => timeline.current.scrollToBottomOnNextUpdate());
|
||||
} else {
|
||||
readerScrollsTo(scroller, scroller.scrollTop - 200);
|
||||
}
|
||||
}
|
||||
flushFrames();
|
||||
await render([...history, sent]);
|
||||
flushFrames();
|
||||
|
||||
assert.ok(container.querySelector(`[data-message-id="${sent.id}"]`));
|
||||
assert.equal(
|
||||
container.querySelector('[data-testid="message-scroll-to-latest"]'),
|
||||
null,
|
||||
);
|
||||
assert.equal(scroller.scrollTop, scroller.scrollHeight - VIEWPORT);
|
||||
});
|
||||
}
|
||||
@@ -358,6 +358,7 @@ const MessageTimelineBase = React.forwardRef<
|
||||
onScroll,
|
||||
scrollToBottom,
|
||||
scrollToBottomOnNextUpdate,
|
||||
isBottomPinArmed,
|
||||
scrollToMessage,
|
||||
onVirtualizerAtBottomStateChange,
|
||||
} = useAnchoredScroll({
|
||||
@@ -424,13 +425,17 @@ const MessageTimelineBase = React.forwardRef<
|
||||
}
|
||||
} else if (hasConfirmedVirtualizerBottomRef.current) {
|
||||
onVirtualizerAtBottomStateChange(false);
|
||||
if (semanticAtBottomRef.current) {
|
||||
// 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()) {
|
||||
suppressNextSemanticBottomRef.current = true;
|
||||
queueSemanticBottom(false);
|
||||
}
|
||||
}
|
||||
},
|
||||
[onVirtualizerAtBottomStateChange, queueSemanticBottom],
|
||||
[isBottomPinArmed, onVirtualizerAtBottomStateChange, queueSemanticBottom],
|
||||
);
|
||||
const hasPendingLiveRows = bufferedTimeline.pendingCount > 0;
|
||||
React.useEffect(() => {
|
||||
@@ -480,7 +485,13 @@ const MessageTimelineBase = React.forwardRef<
|
||||
const prepareForOwnMessage = React.useCallback(() => {
|
||||
// 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.
|
||||
// Virtua's model and become the new physical floor. Drop a freeze queued
|
||||
// before the send, or it would land after this release.
|
||||
if (semanticBottomRafRef.current !== null) {
|
||||
window.cancelAnimationFrame(semanticBottomRafRef.current);
|
||||
semanticBottomRafRef.current = null;
|
||||
}
|
||||
semanticAtBottomRef.current = true;
|
||||
setIsSemanticallyAtBottom(true);
|
||||
scrollToBottomOnNextUpdate();
|
||||
}, [scrollToBottomOnNextUpdate]);
|
||||
|
||||
@@ -88,6 +88,8 @@ 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;
|
||||
/** Imperative: scroll a specific message into view; optionally pulse it.
|
||||
* Returns true if the row was found and scrolled, false otherwise. */
|
||||
scrollToMessage: (
|
||||
@@ -445,6 +447,10 @@ export function useAnchoredScroll({
|
||||
const scrollToBottomOnNextUpdate = React.useCallback(() => {
|
||||
forceBottomOnNextAppendRef.current = true;
|
||||
}, []);
|
||||
const isBottomPinArmed = React.useCallback(
|
||||
() => forceBottomOnNextAppendRef.current,
|
||||
[],
|
||||
);
|
||||
|
||||
const settleAtBottomAfterLayout = React.useCallback(() => {
|
||||
const container = scrollContainerRef.current;
|
||||
@@ -1059,6 +1065,7 @@ export function useAnchoredScroll({
|
||||
scrollToBottom: scrollToBottomImperative,
|
||||
settleAtBottomAfterLayout,
|
||||
scrollToBottomOnNextUpdate,
|
||||
isBottomPinArmed,
|
||||
scrollToMessage: scrollToMessageImperative,
|
||||
onVirtualizerAtBottomStateChange,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user