From c257ff1690f634c8fa8c409158730108692d383e Mon Sep 17 00:00:00 2001 From: mtoohey Date: Thu, 5 Nov 2026 02:00:15 +0000 Subject: [PATCH] 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 Signed-off-by: mtoohey (cherry picked from commit 159a87b411947e390e39c5bf94376bea9400a4fd) --- .../src/features/channels/ui/ChannelPane.tsx | 46 +++++++++----- .../features/channels/ui/ChannelPane.types.ts | 2 +- .../features/channels/ui/ChannelScreen.tsx | 2 +- .../features/channels/ui/useChannelIntro.tsx | 2 +- .../channels/useWelcomeAgentCreate.ts | 13 +++- .../ui/MessageTimeline.ownSend.jsdom-test.mjs | 63 +++++++++++++++++++ .../features/messages/ui/MessageTimeline.tsx | 27 +++++++- .../ui/useAnchoredScroll.lifecycle.test.mjs | 55 ++++++++++++++++ .../features/messages/ui/useAnchoredScroll.ts | 10 +++ 9 files changed, 199 insertions(+), 21 deletions(-) diff --git a/desktop/src/features/channels/ui/ChannelPane.tsx b/desktop/src/features/channels/ui/ChannelPane.tsx index 9b384e1a0f..cc84d45c74 100644 --- a/desktop/src/features/channels/ui/ChannelPane.tsx +++ b/desktop/src/features/channels/ui/ChannelPane.tsx @@ -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, diff --git a/desktop/src/features/channels/ui/ChannelPane.types.ts b/desktop/src/features/channels/ui/ChannelPane.types.ts index 1fe5bf751b..e4f987cbd1 100644 --- a/desktop/src/features/channels/ui/ChannelPane.types.ts +++ b/desktop/src/features/channels/ui/ChannelPane.types.ts @@ -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; diff --git a/desktop/src/features/channels/ui/ChannelScreen.tsx b/desktop/src/features/channels/ui/ChannelScreen.tsx index c6a8907896..51568d514f 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.tsx +++ b/desktop/src/features/channels/ui/ChannelScreen.tsx @@ -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], ); diff --git a/desktop/src/features/channels/ui/useChannelIntro.tsx b/desktop/src/features/channels/ui/useChannelIntro.tsx index 3374fb7a65..53b380126e 100644 --- a/desktop/src/features/channels/ui/useChannelIntro.tsx +++ b/desktop/src/features/channels/ui/useChannelIntro.tsx @@ -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; diff --git a/desktop/src/features/channels/useWelcomeAgentCreate.ts b/desktop/src/features/channels/useWelcomeAgentCreate.ts index cf102b9fbc..1cb79e4905 100644 --- a/desktop/src/features/channels/useWelcomeAgentCreate.ts +++ b/desktop/src/features/channels/useWelcomeAgentCreate.ts @@ -26,10 +26,15 @@ export function useWelcomeAgentCreate({ ); const [isOpen, setIsOpen] = React.useState(false); const [error, setError] = React.useState(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 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 82b06cf34b..779621864a 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.ownSend.jsdom-test.mjs +++ b/desktop/src/features/messages/ui/MessageTimeline.ownSend.jsdom-test.mjs @@ -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); +}); diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index 28cdcd63ad..b4f0c18551 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -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( diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs index dd0f4160d7..8f3523a19a 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs +++ b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs @@ -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()); +}); diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.ts b/desktop/src/features/messages/ui/useAnchoredScroll.ts index e6df41134f..73dd4dfa46 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.ts +++ b/desktop/src/features/messages/ui/useAnchoredScroll.ts @@ -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, };