mirror of
https://github.com/block/buzz.git
synced 2026-11-06 17:31:19 +00:00
fix(desktop): disarm an own send's pin and hold when the send fails
Problem:
Second review of the branch (warning, regression vs main) on a2a9767de
("fix(desktop): end an own send's freeze hold when its row lands"),
useAnchoredScroll.ts and ChannelPane.tsx. The hold can still go stale
when a send never adds a row to this timeline.
`ChannelPane.handleSendMessage` armed the pin and the hold before it
awaited `onSendMessage`, for every main-composer send. That includes a
DM send that `useMentionSendFlow`'s `onPrepareSendChannel` moves to
another DM (the message mentions an agent who isn't in this one). Its
optimistic row goes to that channel's window, and `goChannel` runs only
after the send succeeds. If the publish is rejected or times out, the
user stays on the original DM with both flags armed. A same-channel send
whose `onMutate` can't resolve the channel also adds no row. Then the
reader scrolls up, the hold skips the freeze, the next arrival from the
other person becomes the tail, and the stale pin moves the reader to the
bottom with no pill. The reviewer's jsdom repro: at HEAD, no pill and
scrollTop 660 -> 956. At the merge base, "1 new message" and 660 stays.
A failed send to the same channel was fine: its optimistic row lands in
`onMutate`, an append that clears the hold, and is then reverted.
Fix:
- `ChannelPane.handleSendMessage` arms only when the send targets this
channel (`!channelId || channelId === activeChannelId`). A send
prepared into another channel lands its row there, and the pane
navigates to it once the send succeeds.
- `MessageTimelineHandle.cancelScrollToBottomOnNextUpdate` disarms a
send. `useAnchoredScroll` clears the pin and the pending-send flag.
The hold skipped every freeze while the send was pending, including
one queued before the send that the arm cancelled. So if the send was
still pending and Virtua's last report has the reader off the floor,
the cancel queues that freeze, as the report would have without the
hold. Later arrivals then wait behind the pill. If the send's row
already landed, the hold had ended and the cancel only clears the
pin.
- `ChannelPane` arms through `armOwnSend`, which returns the disarm. The
main composer calls it when `onSendMessage` rejects, and the Welcome
"create in chat" flow when its `mutateAsync` rejects. Its `beforeSend`
option now returns the disarm. A generation counter keeps a late
failure from disarming a pin that a later send armed.
Validation:
- `MessageTimeline.ownSend.jsdom-test.mjs` adds the reviewer's no-row
case in three orders: fail before the reader scrolls up, after, and
with the freeze queued before the send. Each expects "1 new message"
and the reader's offset unchanged after a new tail arrives. With the
cancel a no-op (HEAD's behavior), all three fail: no pill, and the
reader moves from 660 to 956. With the cancel clearing the flags but
not queueing the freeze, the two orders where the reader left the
floor during the send fail with "Jump to latest". A fourth case, a
same-channel send whose row lands and is then reverted, still shows
"1 new message" and keeps the offset. It passes on HEAD as well,
because it pins behavior that must not change. 7/7 pass.
- `useAnchoredScroll.lifecycle.test.mjs` adds a hook case: arm, cancel,
the reader leaves the floor, a new tail arrives without a jump, and a
later send still pins. It fails if the cancel is a no-op, or if it
clears only the pending flag. useAnchoredScroll unit tests: 45/45.
- Full desktop `pnpm test`: 6934 unit and 183 jsdom tests pass.
- `tsc --noEmit`, `biome check --error-on-warnings` on the changed files
and `git diff --check` are clean.
- No test renders `ChannelPane`, so its arm condition, the rejection
handlers and the generation counter are covered only by tsc and
review. No browser E2E was run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
(cherry picked from commit 159a87b411947e390e39c5bf94376bea9400a4fd)
This commit is contained in:
@@ -280,6 +280,17 @@ export const ChannelPane = React.memo(function ChannelPane({
|
||||
}
|
||||
return pubkeys;
|
||||
}, [activityAgents, agentPubkeys, agentSessionAgents]);
|
||||
// Arms the timeline's own-send pin and returns its disarm for a failed send.
|
||||
// The generation keeps a late failure from disarming a later send's pin.
|
||||
const ownSendGenerationRef = React.useRef(0);
|
||||
const armOwnSend = React.useCallback(() => {
|
||||
const generation = ++ownSendGenerationRef.current;
|
||||
messageTimelineRef.current?.scrollToBottomOnNextUpdate();
|
||||
return () => {
|
||||
if (ownSendGenerationRef.current !== generation) return;
|
||||
messageTimelineRef.current?.cancelScrollToBottomOnNextUpdate();
|
||||
};
|
||||
}, []);
|
||||
const handleSendMessage = React.useCallback(
|
||||
async (
|
||||
content: string,
|
||||
@@ -296,15 +307,24 @@ export const ChannelPane = React.memo(function ChannelPane({
|
||||
isActiveWelcomeChannel &&
|
||||
(containsWelcomePersonaMention(content) ||
|
||||
mentionsKnownAgent(mentionPubkeys, knownAgentPubkeys));
|
||||
messageTimelineRef.current?.scrollToBottomOnNextUpdate();
|
||||
await onSendMessage(
|
||||
content,
|
||||
mentionPubkeys,
|
||||
mediaTags,
|
||||
channelId,
|
||||
threadContext,
|
||||
forceRest,
|
||||
);
|
||||
// A send prepared into another channel (a DM message that mentions an
|
||||
// agent outside the DM) lands its row there, so this timeline never sees
|
||||
// it. The pane navigates there once the send succeeds.
|
||||
const disarmOwnSend =
|
||||
!channelId || channelId === activeChannelId ? armOwnSend() : null;
|
||||
try {
|
||||
await onSendMessage(
|
||||
content,
|
||||
mentionPubkeys,
|
||||
mediaTags,
|
||||
channelId,
|
||||
threadContext,
|
||||
forceRest,
|
||||
);
|
||||
} catch (error) {
|
||||
disarmOwnSend?.();
|
||||
throw error;
|
||||
}
|
||||
if (
|
||||
channelId &&
|
||||
channelId !== activeChannelId &&
|
||||
@@ -319,6 +339,7 @@ export const ChannelPane = React.memo(function ChannelPane({
|
||||
},
|
||||
[
|
||||
activeChannelId,
|
||||
armOwnSend,
|
||||
completeWelcomeComposerBanner,
|
||||
goChannel,
|
||||
isActiveWelcomeChannel,
|
||||
@@ -357,11 +378,8 @@ export const ChannelPane = React.memo(function ChannelPane({
|
||||
[activeChannel, currentPubkey, profiles],
|
||||
);
|
||||
const handleWelcomeAddAgent = React.useCallback(() => {
|
||||
onAddAgent?.({
|
||||
beforeSend: () =>
|
||||
messageTimelineRef.current?.scrollToBottomOnNextUpdate(),
|
||||
});
|
||||
}, [onAddAgent]);
|
||||
onAddAgent?.({ beforeSend: armOwnSend });
|
||||
}, [armOwnSend, onAddAgent]);
|
||||
const standardChannelIntro = useChannelIntro({
|
||||
activeChannel,
|
||||
onAddAgent,
|
||||
|
||||
@@ -96,7 +96,7 @@ export type ChannelPaneProps = {
|
||||
onChannelManagementDeleted?: () => void;
|
||||
onCloseIdleAuxiliaryPanel?: () => void;
|
||||
onCloseProfilePanel: () => void;
|
||||
onAddAgent?: (options?: { beforeSend?: () => void }) => void;
|
||||
onAddAgent?: (options?: { beforeSend?: () => () => void }) => void;
|
||||
onAddFiles?: () => void;
|
||||
onBrowseChannels?: () => void;
|
||||
onCreateChannel?: () => void;
|
||||
|
||||
@@ -559,7 +559,7 @@ export function ChannelScreen({
|
||||
? handleSendVideoReviewComment
|
||||
: undefined;
|
||||
const handleOpenAddBot = React.useCallback(
|
||||
(options?: { beforeSend?: () => void }) =>
|
||||
(options?: { beforeSend?: () => () => void }) =>
|
||||
welcomeAgentCreate.openAddAgent(() => setIsAddBotOpen(true), options),
|
||||
[welcomeAgentCreate],
|
||||
);
|
||||
|
||||
@@ -38,7 +38,7 @@ export function useChannelIntro({
|
||||
onWelcomeAddAgent,
|
||||
}: {
|
||||
activeChannel: Channel | null;
|
||||
onAddAgent?: (options?: { beforeSend?: () => void }) => void;
|
||||
onAddAgent?: (options?: { beforeSend?: () => () => void }) => void;
|
||||
onAddFiles?: () => void;
|
||||
onBrowseChannels?: () => void;
|
||||
onCreateChannel?: () => void;
|
||||
|
||||
@@ -26,10 +26,15 @@ export function useWelcomeAgentCreate({
|
||||
);
|
||||
const [isOpen, setIsOpen] = React.useState(false);
|
||||
const [error, setError] = React.useState<string | null>(null);
|
||||
const beforeSendRef = React.useRef<(() => void) | null>(null);
|
||||
// Runs right before the chat-first send and returns how to undo it if the
|
||||
// send fails.
|
||||
const beforeSendRef = React.useRef<(() => () => void) | null>(null);
|
||||
|
||||
const openAddAgent = React.useCallback(
|
||||
(openRegularPicker: () => void, options?: { beforeSend?: () => void }) => {
|
||||
(
|
||||
openRegularPicker: () => void,
|
||||
options?: { beforeSend?: () => () => void },
|
||||
) => {
|
||||
setError(null);
|
||||
if (isWelcomeExperienceChannel(activeChannel)) {
|
||||
beforeSendRef.current = options?.beforeSend ?? null;
|
||||
@@ -59,8 +64,9 @@ export function useWelcomeAgentCreate({
|
||||
return;
|
||||
}
|
||||
setError(null);
|
||||
let undoBeforeSend: (() => void) | undefined;
|
||||
try {
|
||||
beforeSendRef.current?.();
|
||||
undoBeforeSend = beforeSendRef.current?.();
|
||||
await sendMessageMutation.mutateAsync({
|
||||
channelId: activeChannel.id,
|
||||
content: `@${welcomeGuideAgent.name}, help me create a new agent.`,
|
||||
@@ -68,6 +74,7 @@ export function useWelcomeAgentCreate({
|
||||
});
|
||||
setIsOpen(false);
|
||||
} catch (cause) {
|
||||
undoBeforeSend?.();
|
||||
setError(
|
||||
cause instanceof Error
|
||||
? cause.message
|
||||
|
||||
@@ -263,3 +263,66 @@ test("a reader who scrolls up after a send that lands above the tail keeps their
|
||||
);
|
||||
assert.equal(scroller.scrollTop, readingOffset);
|
||||
});
|
||||
|
||||
// A failed send can leave no row here, as when its optimistic row never
|
||||
// resolved a channel. Its cancel drops the pin and the hold, and freezes the
|
||||
// tail the hold left open if the reader is off the floor, so the next arrival
|
||||
// waits behind the pill instead of firing the pin under the reader.
|
||||
for (const order of [
|
||||
["send", "fail", "gap"],
|
||||
["send", "gap", "fail"],
|
||||
["gap", "send", "fail"],
|
||||
]) {
|
||||
test(`a send that fails without a row leaves the reader in place (${order.join(", ")})`, async () => {
|
||||
const { render, scroller, timeline } = await mountAtFloor();
|
||||
|
||||
for (const step of order) {
|
||||
if (step === "send") {
|
||||
act(() => timeline.current.scrollToBottomOnNextUpdate());
|
||||
} else if (step === "fail") {
|
||||
act(() => timeline.current.cancelScrollToBottomOnNextUpdate());
|
||||
} else {
|
||||
readerScrollsTo(scroller, scroller.scrollTop - 200);
|
||||
}
|
||||
}
|
||||
const readingOffset = scroller.scrollTop;
|
||||
flushFrames();
|
||||
await render([...history, message(22, "from the other person")]);
|
||||
flushFrames();
|
||||
|
||||
assert.equal(
|
||||
container.querySelector('[data-testid="message-scroll-to-latest"]')
|
||||
?.textContent,
|
||||
"1 new message",
|
||||
);
|
||||
assert.equal(scroller.scrollTop, readingOffset);
|
||||
});
|
||||
}
|
||||
|
||||
// A send to this channel lands its optimistic row at once, which is an append
|
||||
// that fires the pin and ends the hold. When the publish then fails, the row
|
||||
// is reverted and the cancel finds nothing pending.
|
||||
test("a reader who scrolls up before a same-channel send fails keeps their place", async () => {
|
||||
const { render, scroller, timeline } = await mountAtFloor();
|
||||
|
||||
act(() => timeline.current.scrollToBottomOnNextUpdate());
|
||||
await render([...history, sent]);
|
||||
flushFrames();
|
||||
assert.equal(scroller.scrollTop, scroller.scrollHeight - VIEWPORT);
|
||||
|
||||
readerScrollsTo(scroller, scroller.scrollTop - 200);
|
||||
const readingOffset = scroller.scrollTop;
|
||||
flushFrames();
|
||||
act(() => timeline.current.cancelScrollToBottomOnNextUpdate());
|
||||
await render(history);
|
||||
flushFrames();
|
||||
await render([...history, message(22, "from the other person")]);
|
||||
flushFrames();
|
||||
|
||||
assert.equal(
|
||||
container.querySelector('[data-testid="message-scroll-to-latest"]')
|
||||
?.textContent,
|
||||
"1 new message",
|
||||
);
|
||||
assert.equal(scroller.scrollTop, readingOffset);
|
||||
});
|
||||
|
||||
@@ -35,6 +35,8 @@ import { useSettleGatedPrependMessages } from "./useSettleGatedPrependMessages";
|
||||
|
||||
export type MessageTimelineHandle = {
|
||||
scrollToBottomOnNextUpdate: () => void;
|
||||
/** Undo `scrollToBottomOnNextUpdate` for a send that failed. */
|
||||
cancelScrollToBottomOnNextUpdate: () => void;
|
||||
settleAtBottom: () => boolean;
|
||||
};
|
||||
|
||||
@@ -359,6 +361,7 @@ const MessageTimelineBase = React.forwardRef<
|
||||
scrollToBottom,
|
||||
scrollToBottomOnNextUpdate,
|
||||
isOwnSendPending,
|
||||
cancelScrollToBottomOnNextUpdate,
|
||||
scrollToMessage,
|
||||
onVirtualizerAtBottomStateChange,
|
||||
} = useAnchoredScroll({
|
||||
@@ -379,6 +382,7 @@ const MessageTimelineBase = React.forwardRef<
|
||||
});
|
||||
|
||||
const hasConfirmedVirtualizerBottomRef = React.useRef(false);
|
||||
const virtualizerAtBottomRef = React.useRef(true);
|
||||
const bottomConfirmationChannelRef = React.useRef(channelId);
|
||||
if (bottomConfirmationChannelRef.current !== channelId) {
|
||||
bottomConfirmationChannelRef.current = channelId;
|
||||
@@ -412,6 +416,7 @@ const MessageTimelineBase = React.forwardRef<
|
||||
// scroll-to-end is still converging. Do not turn that mount transient
|
||||
// into a semantic dataset freeze: wait until this channel has reached a
|
||||
// confirmed bottom once, then track genuine bottom -> history movement.
|
||||
virtualizerAtBottomRef.current = atBottom;
|
||||
if (atBottom) {
|
||||
hasConfirmedVirtualizerBottomRef.current = true;
|
||||
onVirtualizerAtBottomStateChange(true);
|
||||
@@ -496,18 +501,38 @@ const MessageTimelineBase = React.forwardRef<
|
||||
setIsSemanticallyAtBottom(true);
|
||||
scrollToBottomOnNextUpdate();
|
||||
}, [scrollToBottomOnNextUpdate]);
|
||||
const cancelOwnMessage = React.useCallback(() => {
|
||||
// While the send was pending, its hold skipped any freeze, including one
|
||||
// queued before the send. If the reader is off the floor now, freeze as
|
||||
// that report would have, so later arrivals wait behind the pill.
|
||||
const skippedFreeze =
|
||||
isOwnSendPending() &&
|
||||
hasConfirmedVirtualizerBottomRef.current &&
|
||||
!virtualizerAtBottomRef.current;
|
||||
cancelScrollToBottomOnNextUpdate();
|
||||
if (skippedFreeze) {
|
||||
suppressNextSemanticBottomRef.current = true;
|
||||
queueSemanticBottom(false);
|
||||
}
|
||||
}, [cancelScrollToBottomOnNextUpdate, isOwnSendPending, queueSemanticBottom]);
|
||||
|
||||
React.useImperativeHandle(
|
||||
ref,
|
||||
() => ({
|
||||
scrollToBottomOnNextUpdate: prepareForOwnMessage,
|
||||
cancelScrollToBottomOnNextUpdate: cancelOwnMessage,
|
||||
settleAtBottom: () => {
|
||||
if (!timelineVirtualizerApi) return false;
|
||||
scrollToBottom("auto");
|
||||
return true;
|
||||
},
|
||||
}),
|
||||
[prepareForOwnMessage, scrollToBottom, timelineVirtualizerApi],
|
||||
[
|
||||
cancelOwnMessage,
|
||||
prepareForOwnMessage,
|
||||
scrollToBottom,
|
||||
timelineVirtualizerApi,
|
||||
],
|
||||
);
|
||||
|
||||
const jumpToMessage = React.useCallback(
|
||||
|
||||
@@ -1085,3 +1085,58 @@ test("reader input at the floor resumes following live messages", async () => {
|
||||
assert.deepEqual(held.settles, ["settle"]);
|
||||
await act(async () => held.root.unmount());
|
||||
});
|
||||
|
||||
// A failed send may add no row, so its cancel must drop the one-shot and the
|
||||
// pending send. The next tail is then someone else's arrival and must not pull
|
||||
// a reader who left the floor, while a later send still pins.
|
||||
test("a cancelled own send leaves the next tail with the reader", async () => {
|
||||
globalThis.ResizeObserver = class {
|
||||
disconnect() {}
|
||||
observe() {}
|
||||
};
|
||||
const scroller = document.createElement("div");
|
||||
scroller.querySelector = () => null;
|
||||
const settles = [];
|
||||
const fake = {
|
||||
content: { current: null },
|
||||
messages: [{ createdAt: 500, id: "latest" }],
|
||||
scrollToBottom: () => settles.push("jump"),
|
||||
scrollToMessage: () => true,
|
||||
scroller: { current: scroller },
|
||||
settleAtBottom: () => settles.push("settle"),
|
||||
};
|
||||
let state = null;
|
||||
const root = createRoot(document.createElement("div"));
|
||||
const render = () =>
|
||||
act(async () =>
|
||||
root.render(
|
||||
React.createElement(TargetHoldHarness, {
|
||||
fake,
|
||||
onState: (next) => {
|
||||
state = next;
|
||||
},
|
||||
}),
|
||||
),
|
||||
);
|
||||
await render();
|
||||
await act(async () => state.onVirtualizerAtBottomStateChange(true));
|
||||
settles.length = 0; // the mount's own bottom pin
|
||||
|
||||
state.scrollToBottomOnNextUpdate();
|
||||
assert.equal(state.isOwnSendPending(), true);
|
||||
state.cancelScrollToBottomOnNextUpdate();
|
||||
assert.equal(state.isOwnSendPending(), false);
|
||||
|
||||
await act(async () => state.onVirtualizerAtBottomStateChange(false));
|
||||
fake.messages = [...fake.messages, { createdAt: 600, id: "live" }];
|
||||
await render();
|
||||
assert.deepEqual(settles, [], "the arrival does not move the reader");
|
||||
assert.equal(state.isAtBottom, false);
|
||||
|
||||
state.scrollToBottomOnNextUpdate();
|
||||
fake.messages = [...fake.messages, { createdAt: 700, id: "sent" }];
|
||||
await render();
|
||||
assert.deepEqual(settles, ["jump"], "a later send still pins");
|
||||
assert.equal(state.isOwnSendPending(), false);
|
||||
await act(async () => root.unmount());
|
||||
});
|
||||
|
||||
@@ -91,6 +91,9 @@ type UseAnchoredScrollResult = {
|
||||
/** True from that arming until the first append lands, whether or not the
|
||||
* append moves the tail and fires the one-shot. */
|
||||
isOwnSendPending: () => boolean;
|
||||
/** Disarm that one-shot and the pending send without moving the view, for
|
||||
* a send that failed. */
|
||||
cancelScrollToBottomOnNextUpdate: () => void;
|
||||
/** Imperative: scroll a specific message into view; optionally pulse it.
|
||||
* Returns true if the row was found and scrolled, false otherwise. */
|
||||
scrollToMessage: (
|
||||
@@ -459,6 +462,12 @@ export function useAnchoredScroll({
|
||||
() => ownSendPendingRef.current,
|
||||
[],
|
||||
);
|
||||
// A send that fails may never add a row here, so nothing else would clear
|
||||
// these.
|
||||
const cancelScrollToBottomOnNextUpdate = React.useCallback(() => {
|
||||
forceBottomOnNextAppendRef.current = false;
|
||||
ownSendPendingRef.current = false;
|
||||
}, []);
|
||||
|
||||
const settleAtBottomAfterLayout = React.useCallback(() => {
|
||||
const container = scrollContainerRef.current;
|
||||
@@ -1078,6 +1087,7 @@ export function useAnchoredScroll({
|
||||
settleAtBottomAfterLayout,
|
||||
scrollToBottomOnNextUpdate,
|
||||
isOwnSendPending,
|
||||
cancelScrollToBottomOnNextUpdate,
|
||||
scrollToMessage: scrollToMessageImperative,
|
||||
onVirtualizerAtBottomStateChange,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user