From d369ca9df1248e2ee16a40b2a193bf08dd8126c4 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Thu, 2 Jul 2026 16:15:44 -0400 Subject: [PATCH] fix(desktop): bind channel and thread context at compose time to prevent wrong-channel send (#1472) Signed-off-by: Will Pfleger Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 --- desktop/playwright.config.ts | 1 + desktop/src/features/agents/hooks.ts | 34 +- desktop/src/features/channels/hooks.ts | 19 +- .../src/features/channels/ui/ChannelPane.tsx | 3 +- .../features/channels/ui/ChannelPane.types.ts | 6 + .../channels/useChannelPaneHandlers.ts | 35 ++- desktop/src/features/messages/hooks.ts | 123 +++++++- .../messages/lib/sendChannelBinding.test.mjs | 290 ++++++++++++++++++ .../features/messages/ui/MessageComposer.tsx | 30 ++ .../messages/ui/MessageThreadPanel.tsx | 19 ++ .../messages/ui/useMentionSendFlow.ts | 73 ++++- desktop/src/testing/e2eBridge.ts | 7 + .../tests/e2e/send-channel-binding.spec.ts | 189 ++++++++++++ desktop/tests/helpers/bridge.ts | 1 + 14 files changed, 781 insertions(+), 49 deletions(-) create mode 100644 desktop/src/features/messages/lib/sendChannelBinding.test.mjs create mode 100644 desktop/tests/e2e/send-channel-binding.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 46c8bd9d8..3b5f35d99 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -64,6 +64,7 @@ export default defineConfig({ "**/timeline-no-shift.spec.ts", "**/human-edit-agent-content.spec.ts", "**/reaction-order.spec.ts", + "**/send-channel-binding.spec.ts", ], use: { ...devices["Desktop Chrome"], diff --git a/desktop/src/features/agents/hooks.ts b/desktop/src/features/agents/hooks.ts index 413086969..567c5b576 100644 --- a/desktop/src/features/agents/hooks.ts +++ b/desktop/src/features/agents/hooks.ts @@ -432,15 +432,24 @@ export function useAttachManagedAgentToChannelMutation( const queryClient = useQueryClient(); return useMutation({ - mutationFn: async (input: AttachManagedAgentToChannelInput) => { - if (!channelId) { + mutationFn: async ( + input: AttachManagedAgentToChannelInput & { channelId?: string }, + ) => { + const { channelId: capturedChannelId, ...rest } = input; + const effectiveChannelId = capturedChannelId ?? channelId; + if (!effectiveChannelId) { throw new Error("No channel selected."); } - return attachManagedAgentToChannel(channelId, input); + return attachManagedAgentToChannel(effectiveChannelId, rest); }, - onSettled: () => { - invalidateAgentQueriesInBackground(queryClient, channelId); + onSettled: (_data, _err, variables) => { + // Invalidate the effective channel (the one the server actually mutated) + // so its membership/agent state stays fresh. Invalidating the live + // hook-closure channelId when the user has already switched away would + // leave the compose-time channel stale. + const effectiveChannelId = variables?.channelId ?? channelId; + invalidateAgentQueriesInBackground(queryClient, effectiveChannelId); }, }); } @@ -469,13 +478,17 @@ export function useCreateChannelManagedAgentMutation(channelId: string | null) { return useMutation({ mutationFn: async ( - input: CreateChannelManagedAgentInput, + input: CreateChannelManagedAgentInput & { channelId?: string }, ): Promise => { - if (!channelId) { + const { channelId: capturedChannelId, ...rest } = input; + const effectiveChannelId = capturedChannelId ?? channelId; + if (!effectiveChannelId) { throw new Error("No channel selected."); } - const result = await createChannelManagedAgents(channelId, [input]); + const result = await createChannelManagedAgents(effectiveChannelId, [ + rest, + ]); const success = result.successes[0]; if (success) { return success; @@ -484,8 +497,9 @@ export function useCreateChannelManagedAgentMutation(channelId: string | null) { const failure = result.failures[0]; throw new Error(failure?.error ?? "Could not create agent."); }, - onSettled: () => { - invalidateAgentQueriesInBackground(queryClient, channelId); + onSettled: (_data, _err, variables) => { + const effectiveChannelId = variables?.channelId ?? channelId; + invalidateAgentQueriesInBackground(queryClient, effectiveChannelId); }, }); } diff --git a/desktop/src/features/channels/hooks.ts b/desktop/src/features/channels/hooks.ts index c5cb3572b..91e6aebd6 100644 --- a/desktop/src/features/channels/hooks.ts +++ b/desktop/src/features/channels/hooks.ts @@ -396,15 +396,24 @@ export function useAddChannelMembersMutation(channelId: string | null) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: (input: Omit) => { - if (!channelId) { + mutationFn: ( + input: Omit & { + channelId?: string; + }, + ) => { + const { channelId: capturedChannelId, ...rest } = input; + const effectiveChannelId = capturedChannelId ?? channelId; + if (!effectiveChannelId) { throw new Error("No channel selected."); } - return addChannelMembers({ ...input, channelId }); + return addChannelMembers({ ...rest, channelId: effectiveChannelId }); }, - onSettled: async () => { - await invalidateChannelState(queryClient, channelId); + onSettled: async (_data, _err, variables) => { + // Invalidate the effective channel (the one actually mutated) not the + // live hook-closure channel, which may have changed mid-send. + const effectiveChannelId = variables?.channelId ?? channelId; + await invalidateChannelState(queryClient, effectiveChannelId); }, }); } diff --git a/desktop/src/features/channels/ui/ChannelPane.tsx b/desktop/src/features/channels/ui/ChannelPane.tsx index c6c4ea58a..7145507fa 100644 --- a/desktop/src/features/channels/ui/ChannelPane.tsx +++ b/desktop/src/features/channels/ui/ChannelPane.tsx @@ -297,6 +297,7 @@ export const ChannelPane = React.memo(function ChannelPane({ content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, ) => { const shouldCompleteWelcomeBanner = isActiveWelcomeChannel && @@ -304,7 +305,7 @@ export const ChannelPane = React.memo(function ChannelPane({ mentionsKnownAgent(mentionPubkeys, knownAgentPubkeys)); messageTimelineRef.current?.scrollToBottomOnNextUpdate(); - await onSendMessage(content, mentionPubkeys, mediaTags); + await onSendMessage(content, mentionPubkeys, mediaTags, channelId); if (shouldCompleteWelcomeBanner) { completeWelcomeComposerBanner(); diff --git a/desktop/src/features/channels/ui/ChannelPane.types.ts b/desktop/src/features/channels/ui/ChannelPane.types.ts index 02b441ff6..db0ffaba2 100644 --- a/desktop/src/features/channels/ui/ChannelPane.types.ts +++ b/desktop/src/features/channels/ui/ChannelPane.types.ts @@ -67,6 +67,7 @@ export type ChannelPaneProps = { content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, ) => Promise; onSendVideoReviewComment?: ( message: TimelineMessage, @@ -79,6 +80,11 @@ export type ChannelPaneProps = { content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, + threadContext?: { + parentEventId: string | null; + threadHeadId: string | null; + } | null, ) => Promise; onTargetReached?: (messageId: string) => void; onToggleReaction?: ( diff --git a/desktop/src/features/channels/useChannelPaneHandlers.ts b/desktop/src/features/channels/useChannelPaneHandlers.ts index 0b77a3d42..efe36c70b 100644 --- a/desktop/src/features/channels/useChannelPaneHandlers.ts +++ b/desktop/src/features/channels/useChannelPaneHandlers.ts @@ -6,6 +6,7 @@ import type { useSendMessageMutation, useToggleReactionMutation, } from "@/features/messages/hooks"; +import { resolveThreadReplyTarget } from "@/features/messages/hooks"; /** * Stable callback references for ChannelPane so that keystroke-driven @@ -230,11 +231,13 @@ export function useChannelPaneHandlers({ content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, ) => { await sendMutateRef.current({ content, mentionPubkeys, mediaTags, + channelId: channelId ?? undefined, }); }, [], @@ -245,13 +248,24 @@ export function useChannelPaneHandlers({ content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, + threadContext?: { + parentEventId: string | null; + threadHeadId: string | null; + } | null, ) => { - const activeThreadHeadId = openThreadHeadIdRef.current; - const parentEventId = - threadReplyTargetIdRef.current ?? activeThreadHeadId; - if (!parentEventId) { + // Resolve target using captured submit-time context (race-free) or live + // refs (legacy path). When threadContext is supplied, no live-ref reads + // occur after the mention-flow awaits; the resolution is purely data. + const target = resolveThreadReplyTarget( + threadContext, + threadReplyTargetIdRef.current, + openThreadHeadIdRef.current, + ); + if (!target) { return; } + const { parentEventId, threadHeadId: activeThreadHeadId } = target; if ( activeThreadHeadId && @@ -270,10 +284,17 @@ export function useChannelPaneHandlers({ mentionPubkeys, parentEventId, mediaTags, + channelId: channelId ?? undefined, }); - setThreadReplyTargetId(activeThreadHeadId); - if (activeThreadHeadId && parentEventId !== activeThreadHeadId) { - setThreadScrollTargetId(sentMessage.id); + + // Only update thread UI state if the user is still viewing the same + // thread. If they navigated away during the async send, don't disrupt + // the thread they are currently viewing. + if (openThreadHeadIdRef.current === activeThreadHeadId) { + setThreadReplyTargetId(activeThreadHeadId); + if (activeThreadHeadId && parentEventId !== activeThreadHeadId) { + setThreadScrollTargetId(sentMessage.id); + } } }, [ diff --git a/desktop/src/features/messages/hooks.ts b/desktop/src/features/messages/hooks.ts index 81feccd0c..2209f24fa 100644 --- a/desktop/src/features/messages/hooks.ts +++ b/desktop/src/features/messages/hooks.ts @@ -18,6 +18,7 @@ import { import { splitOutgoingTags } from "@/features/messages/lib/imetaMediaMarkdown"; import { relayClient } from "@/shared/api/relayClient"; import { customEmojiQueryKey } from "@/features/custom-emoji/hooks"; +import { channelsQueryKey } from "@/features/channels/hooks"; import { reactionEmojiUrl } from "@/shared/api/customEmoji"; import type { CustomEmoji } from "@/shared/lib/remarkCustomEmoji"; import { @@ -174,6 +175,71 @@ export function createOptimisticMessage( }; } +/** + * Resolves the effective target channel for a send operation. + * + * When `capturedChannelId` is supplied (non-null), the target is looked up from + * `channelsCache` — this pins the send to the compose-time channel regardless + * of any subsequent navigation. If the id is supplied but resolves to nothing, + * returns `null` (caller should throw — don't silently fall back to the live + * channel). When `capturedChannelId` is null, the caller didn't capture one and + * the closed-over `fallbackChannel` is the intended target. + * + * Exported for unit testing. + */ +export function resolveEffectiveChannel( + capturedChannelId: string | null | undefined, + channelsCache: Channel[] | undefined, + fallbackChannel: Channel | null, +): Channel | null { + if (capturedChannelId == null) { + return fallbackChannel; + } + return channelsCache?.find((c) => c.id === capturedChannelId) ?? null; +} + +/** + * Resolves the thread reply target from a submit-time captured context or, + * for callers that predate the capture pattern, from live refs. + * + * When `threadContext` is supplied (non-null), its values are used exclusively + * — no live-ref reads occur. This is the race-free path: the context was + * captured synchronously at submit time before any async awaits. + * + * When `threadContext` is null/undefined (legacy callers), falls back to + * `liveReplyTargetId ?? liveThreadHeadId`. + * + * Returns null when no parentEventId can be resolved (caller should bail). + */ +export function resolveThreadReplyTarget( + threadContext: + | { parentEventId: string | null; threadHeadId: string | null } + | null + | undefined, + liveReplyTargetId: string | null | undefined, + liveThreadHeadId: string | null | undefined, +): { parentEventId: string; threadHeadId: string | null } | null { + if (threadContext != null) { + // Captured context: use exclusively — no ?? fallback to live refs. + if (!threadContext.parentEventId) { + return null; + } + return { + parentEventId: threadContext.parentEventId, + threadHeadId: threadContext.threadHeadId, + }; + } + // Legacy path: read from live refs. + const parentEventId = liveReplyTargetId ?? liveThreadHeadId ?? null; + if (!parentEventId) { + return null; + } + return { + parentEventId, + threadHeadId: liveThreadHeadId ?? null, + }; +} + export function useChannelMessagesQuery(channel: Channel | null) { const queryClient = useQueryClient(); const queryKey = channelMessagesKey(channel?.id ?? "none"); @@ -385,6 +451,7 @@ export function useSendMessageMutation( RelayEvent, Error, { + channelId?: string; content: string; mentionPubkeys?: string[]; parentEventId?: string | null; @@ -393,12 +460,28 @@ export function useSendMessageMutation( MessageQueryContext | undefined >({ mutationFn: async ({ + channelId: capturedChannelId, content, mentionPubkeys, parentEventId, mediaTags, }) => { - if (!channel || channel.channelType === "forum") { + // Resolve the target channel from the compose-time id when provided, so + // a channel switch mid-send does not redirect the message. Fall back to + // the closed-over `channel` for callers that don't supply a capturedId. + // A supplied-but-unresolvable id throws rather than silently falling back + // to the live channel (silent misdelivery is the failure mode we're fixing). + const effectiveChannel = resolveEffectiveChannel( + capturedChannelId, + queryClient.getQueryData(channelsQueryKey), + channel, + ); + + if (capturedChannelId != null && effectiveChannel == null) { + throw new Error("Channel is no longer available."); + } + + if (!effectiveChannel || effectiveChannel.channelType === "forum") { throw new Error("This channel does not support message sending yet."); } @@ -422,10 +505,10 @@ export function useSendMessageMutation( if (parentEventId || imetaTags.length > 0 || emojiTags.length > 0) { const cachedMessages = queryClient.getQueryData( - channelMessagesKey(channel.id), + channelMessagesKey(effectiveChannel.id), ) ?? []; const result = await sendChannelMessage( - channel.id, + effectiveChannel.id, content, parentEventId ?? null, imetaTags, @@ -440,7 +523,7 @@ export function useSendMessageMutation( // For non-replies (media-only), we add them ourselves. const replyTags = parentEventId ? buildReplyTags( - channel.id, + effectiveChannel.id, identity.pubkey, parentEventId, resolveReplyRootId(parentEventId, cachedMessages), @@ -450,7 +533,7 @@ export function useSendMessageMutation( const baseTags = parentEventId ? replyTags // buildReplyTags includes h + author p + mention ps : [ - ["h", channel.id], + ["h", effectiveChannel.id], ["p", identity.pubkey], ]; // non-reply: add ourselves @@ -478,24 +561,44 @@ export function useSendMessageMutation( } return relayClient.sendMessage( - channel.id, + effectiveChannel.id, content, mentionPubkeys ?? [], mentionTags, ); }, - onMutate: async ({ content, mentionPubkeys, parentEventId, mediaTags }) => { - if (!channel || !identity || channel.channelType === "forum") { + onMutate: async ({ + channelId: capturedChannelId, + content, + mentionPubkeys, + parentEventId, + mediaTags, + }) => { + // Mirror the mutationFn channel resolution so the optimistic message + // lands in the same cache key the real send will eventually populate. + // A supplied-but-unresolvable id returns undefined (skips optimistic write) + // rather than silently writing to the live channel. + const effectiveChannel = resolveEffectiveChannel( + capturedChannelId, + queryClient.getQueryData(channelsQueryKey), + channel, + ); + + if ( + !effectiveChannel || + !identity || + effectiveChannel.channelType === "forum" + ) { return undefined; } - const queryKey = channelMessagesKey(channel.id); + const queryKey = channelMessagesKey(effectiveChannel.id); await queryClient.cancelQueries({ queryKey }); const previousMessages = queryClient.getQueryData(queryKey) ?? []; const optimisticMessage = createOptimisticMessage( - channel.id, + effectiveChannel.id, content.trim(), identity, previousMessages, diff --git a/desktop/src/features/messages/lib/sendChannelBinding.test.mjs b/desktop/src/features/messages/lib/sendChannelBinding.test.mjs new file mode 100644 index 000000000..a038e8827 --- /dev/null +++ b/desktop/src/features/messages/lib/sendChannelBinding.test.mjs @@ -0,0 +1,290 @@ +/** + * Regression tests for wrong-channel send bug. + * + * The bug: when a channel switch happens mid-send (during the async agent-prep + * await in useMentionSendFlow), the "latest-value" onSendRef and sendMutateRef + * would already point at the new channel. The fix threads capturedChannelId as + * data through the entire pipeline so the mutation always targets the + * compose-time channel regardless of navigation. + * + * Test coverage: + * 1. createOptimisticMessage uses the supplied channelId for the h-tag. + * 2. resolveEffectiveChannel pins the send to the captured channel even when + * the closed-over channel is different (the core invariant). + * 3. resolveEffectiveChannel returns null for a supplied-but-unresolvable id + * so the caller can throw rather than silently misdeliver. + * 4. resolveEffectiveChannel falls back to the closed-over channel when no + * capturedChannelId was supplied (legacy-caller path). + */ + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + createOptimisticMessage, + resolveEffectiveChannel, + resolveThreadReplyTarget, +} from "../hooks.ts"; + +// --------------------------------------------------------------------------- +// Minimal stubs +// --------------------------------------------------------------------------- +const IDENTITY = { + pubkey: "aaaa1111bbbb2222cccc3333dddd4444eeee5555ffff6666aaaa1111bbbb2222", +}; + +function makeChannel(id) { + return { + id, + name: id, + channelType: "channel", + // Only id and channelType are required by the resolution logic. + }; +} + +// --------------------------------------------------------------------------- +// createOptimisticMessage — h-tag carries the compose-time channelId +// --------------------------------------------------------------------------- + +test("createOptimisticMessage_composedChannelId_hTagMatchesComposedChannel", () => { + const composeChannelId = "channel-A"; + const msg = createOptimisticMessage( + composeChannelId, + "hello", + IDENTITY, + [], // currentMessages + [], // mentionPubkeys + null, // parentEventId + [], // mediaTags + ); + + const hTag = msg.tags.find(([name]) => name === "h"); + assert.ok(hTag, "message must have an h-tag"); + assert.equal( + hTag[1], + composeChannelId, + "h-tag must match the compose-time channelId, not any other channel", + ); + assert.equal(msg.content, "hello"); + assert.equal(msg.pending, true); +}); + +test("createOptimisticMessage_differentChannelIds_hTagsAreIndependent", () => { + // Simulate two messages composed in two different channels. + // If a channel switch had corrupted channelId, both would carry the same tag. + const msgA = createOptimisticMessage( + "channel-A", + "msg A", + IDENTITY, + [], + [], + null, + [], + ); + const msgB = createOptimisticMessage( + "channel-B", + "msg B", + IDENTITY, + [], + [], + null, + [], + ); + + const hTagA = msgA.tags.find(([n]) => n === "h"); + const hTagB = msgB.tags.find(([n]) => n === "h"); + + assert.equal(hTagA[1], "channel-A", "message A must target channel-A"); + assert.equal(hTagB[1], "channel-B", "message B must target channel-B"); + assert.notEqual( + hTagA[1], + hTagB[1], + "compose-time channel isolation: the two h-tags must differ", + ); +}); + +test("createOptimisticMessage_withReply_hTagStillCarriesSuppliedChannelId", () => { + // Thread replies also carry the h-tag via buildReplyTags. + // Verify the channel id flows through when a parentEventId is set. + const composeChannelId = "channel-A"; + const parentEvent = createOptimisticMessage( + "channel-A", + "parent", + IDENTITY, + [], + [], + null, + [], + ); + const replyMsg = createOptimisticMessage( + composeChannelId, + "reply", + IDENTITY, + [parentEvent], + [], + parentEvent.id, + [], + ); + + const hTag = replyMsg.tags.find(([name]) => name === "h"); + assert.ok(hTag, "reply must have an h-tag"); + assert.equal( + hTag[1], + composeChannelId, + "reply h-tag must match the compose-time channelId", + ); +}); + +// --------------------------------------------------------------------------- +// resolveEffectiveChannel — the channel-binding invariant +// --------------------------------------------------------------------------- + +test("resolveEffectiveChannel_capturedIdPresentInCache_returnsComposeTimeChannel", () => { + // Core invariant: closure channel is B, variables carry channel A. + // The mutation must target A regardless of what the closure says. + const channelA = makeChannel("channel-A"); + const channelB = makeChannel("channel-B"); + const cache = [channelA, channelB]; + + const result = resolveEffectiveChannel("channel-A", cache, channelB); + + assert.strictEqual( + result?.id, + "channel-A", + "must return the compose-time channel even when the closed-over channel is B", + ); +}); + +test("resolveEffectiveChannel_capturedIdNotInCache_returnsNull", () => { + // F3 invariant: a supplied-but-unresolvable id must not fall back to the + // live channel — the caller is expected to throw "channel no longer available". + const channelB = makeChannel("channel-B"); + const cache = [channelB]; // channel-A is absent (e.g. new channel, cache miss) + + const result = resolveEffectiveChannel("channel-A", cache, channelB); + + assert.strictEqual( + result, + null, + "a supplied-but-unresolvable capturedChannelId must return null, not the live channel", + ); +}); + +test("resolveEffectiveChannel_capturedIdNull_returnsFallbackChannel", () => { + // Legacy callers (thread reply, InboxDetailPane) don't supply a capturedId. + // They rely on the closed-over channel being correct for other reasons. + const channelB = makeChannel("channel-B"); + const cache = [channelB]; + + const result = resolveEffectiveChannel(null, cache, channelB); + + assert.strictEqual( + result?.id, + "channel-B", + "null capturedChannelId must fall through to the closed-over channel", + ); +}); + +test("resolveEffectiveChannel_capturedIdUndefined_returnsFallbackChannel", () => { + // Same as null — undefined means the caller didn't capture an id. + const channelB = makeChannel("channel-B"); + + const result = resolveEffectiveChannel(undefined, [channelB], channelB); + + assert.strictEqual(result?.id, "channel-B"); +}); + +test("resolveEffectiveChannel_emptyCache_capturedIdPresent_returnsNull", () => { + // Cache was wiped (e.g. sign-out race). Must not fall back to live channel. + const channelB = makeChannel("channel-B"); + + const result = resolveEffectiveChannel("channel-A", [], channelB); + + assert.strictEqual(result, null); +}); + +// --------------------------------------------------------------------------- +// resolveThreadReplyTarget — flush-time resolution for handleSendThreadReply +// +// These tests exercise the production resolveThreadReplyTarget function. +// Key invariant (the race): when a captured context is provided, the live +// ref values (liveReplyTargetId, liveThreadHeadId) must be IGNORED even if +// they point at a different thread — they represent post-navigation state. +// --------------------------------------------------------------------------- + +test("resolveThreadReplyTarget_capturedContext_ignoresLiveRefs", () => { + // The race scenario: compose-time context captured A, live refs now point + // at B (user switched threads mid-send). + const result = resolveThreadReplyTarget( + { parentEventId: "parent-A", threadHeadId: "head-A" }, + /* liveReplyTargetId = */ "parent-B", + /* liveThreadHeadId = */ "head-B", + ); + + assert.deepStrictEqual(result, { + parentEventId: "parent-A", + threadHeadId: "head-A", + }); +}); + +test("resolveThreadReplyTarget_capturedContextNullParent_returnsNull", () => { + // Captured context has no parentEventId — bail before any await fires. + const result = resolveThreadReplyTarget( + { parentEventId: null, threadHeadId: "head-A" }, + "live-parent", + "live-head", + ); + + assert.strictEqual(result, null); +}); + +test("resolveThreadReplyTarget_capturedContextNullThreadHead_usesItNotLiveRef", () => { + // F7 degenerate case: threadContext is non-null but threadHeadId is null. + // Must not fall through to the live ref — use null from the context itself. + const result = resolveThreadReplyTarget( + { parentEventId: "parent-A", threadHeadId: null }, + "live-parent", + "live-head", + ); + + assert.deepStrictEqual(result, { + parentEventId: "parent-A", + threadHeadId: null, + }); +}); + +test("resolveThreadReplyTarget_nullContext_fallsBackToLiveRefs", () => { + // Legacy path: no captured context — fall back to live refs. + const result = resolveThreadReplyTarget( + null, + /* liveReplyTargetId = */ "live-parent", + /* liveThreadHeadId = */ "live-head", + ); + + assert.deepStrictEqual(result, { + parentEventId: "live-parent", + threadHeadId: "live-head", + }); +}); + +test("resolveThreadReplyTarget_nullContext_noLiveReplyTarget_fallsBackToThreadHead", () => { + // When there is no specific reply target (just the thread head), parentEventId + // equals the thread head. + const result = resolveThreadReplyTarget( + null, + /* liveReplyTargetId = */ null, + /* liveThreadHeadId = */ "head-only", + ); + + assert.deepStrictEqual(result, { + parentEventId: "head-only", + threadHeadId: "head-only", + }); +}); + +test("resolveThreadReplyTarget_nullContext_noLiveRefs_returnsNull", () => { + // No context and no live refs — bail. + const result = resolveThreadReplyTarget(null, null, null); + + assert.strictEqual(result, null); +}); diff --git a/desktop/src/features/messages/ui/MessageComposer.tsx b/desktop/src/features/messages/ui/MessageComposer.tsx index 130c55613..f30d039ff 100644 --- a/desktop/src/features/messages/ui/MessageComposer.tsx +++ b/desktop/src/features/messages/ui/MessageComposer.tsx @@ -86,10 +86,25 @@ type MessageComposerProps = { */ onEditLastOwnMessage?: () => boolean; onEditSave?: (content: string, mediaTags?: string[][]) => Promise; + /** + * Called synchronously at the start of `submitMessage`, before any awaits, + * to capture context that must be stable throughout the async send pipeline. + * Used by the thread-reply composer to capture the current reply target before + * the mention-flow awaits can change navigation state. + */ + onCaptureSendContext?: () => { + parentEventId: string | null; + threadHeadId: string | null; + } | null; onSend: ( content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, + threadContext?: { + parentEventId: string | null; + threadHeadId: string | null; + } | null, ) => Promise; placeholder?: string; profiles?: UserProfileLookup; @@ -115,6 +130,7 @@ function MessageComposerImpl({ isSending = false, onCancelEdit, onCancelReply, + onCaptureSendContext, onEditLastOwnMessage, onEditSave, onSend, @@ -570,13 +586,26 @@ function MessageComposerImpl({ return; } + const capturedThreadContext = onCaptureSendContext?.() ?? null; + // If a thread-reply composer reported no reply target at submit time, + // bail here rather than discovering the null later after async awaits. + if ( + capturedThreadContext !== null && + !capturedThreadContext.parentEventId + ) { + return; + } + await mentionSendFlow.sendMessageWithMentionFlow({ + capturedChannelId: channelId, + capturedThreadContext, pendingImeta: currentPendingImeta, sentDraftKey: effectiveDraftKeyRef.current, spoileredAttachmentUrls, trimmed, }); }, [ + channelId, channelLinks.clearChannels, customEmoji, emojiAutocomplete.clearEmojis, @@ -590,6 +619,7 @@ function MessageComposerImpl({ setComposerContent, spoileredAttachmentUrls, syncComposerContentFromEditor, + onCaptureSendContext, ]); submitMessageRef.current = submitMessage; diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index cabe05845..09041e5e2 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -69,6 +69,11 @@ type MessageThreadPanelProps = { content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, + threadContext?: { + parentEventId: string | null; + threadHeadId: string | null; + } | null, ) => Promise; onToggleReaction?: ( message: TimelineMessage, @@ -340,6 +345,19 @@ export function MessageThreadPanel({ isSinglePanelView, ); + // Live ref so onCaptureSendContext can read reply state at submit time + // (before any async mention-flow awaits change navigation state). + const replyTargetMessageRef = React.useRef(replyTargetMessage); + replyTargetMessageRef.current = replyTargetMessage; + + const onCaptureSendContext = React.useCallback( + () => ({ + parentEventId: replyTargetMessageRef.current?.id ?? threadHeadId, + threadHeadId, + }), + [threadHeadId], + ); + const collapseThreadHeadReplies = React.useCallback(() => { if (!threadHeadId) { return; @@ -849,6 +867,7 @@ export function MessageThreadPanel({ isSending={isSending} onCancelEdit={onCancelEdit} onCancelReply={composerReplyTarget ? onCancelReply : undefined} + onCaptureSendContext={onCaptureSendContext} onEditLastOwnMessage={onEditLastOwnMessage} onEditSave={onEditSave} onSend={onSend} diff --git a/desktop/src/features/messages/ui/useMentionSendFlow.ts b/desktop/src/features/messages/ui/useMentionSendFlow.ts index daf63e50f..ca457ecfb 100644 --- a/desktop/src/features/messages/ui/useMentionSendFlow.ts +++ b/desktop/src/features/messages/ui/useMentionSendFlow.ts @@ -27,6 +27,12 @@ import { MENTION_REFERENCE_TAG } from "@/shared/lib/resolveMentionNames"; import { buildCustomEmojiTags } from "@/shared/lib/customEmojiTags"; type PendingNonMemberMentionSend = { + capturedChannelId: string | null; + /** Thread context captured at submit time — null for main-timeline sends. */ + capturedThreadContext: { + parentEventId: string | null; + threadHeadId: string | null; + } | null; finalContent: string; mentionPubkeys: string[]; nonMemberPubkeys: string[]; @@ -39,6 +45,12 @@ type PendingNonMemberMentionSend = { }; type SendMessageWithMentionFlowInput = { + capturedChannelId: string | null; + /** Thread context captured at submit time — null for main-timeline sends. */ + capturedThreadContext?: { + parentEventId: string | null; + threadHeadId: string | null; + } | null; pendingImeta: ImetaMedia[]; sentDraftKey: string | null | undefined; spoileredAttachmentUrls?: ReadonlySet; @@ -59,6 +71,11 @@ type UseMentionSendFlowOptions = { content: string, mentionPubkeys: string[], mediaTags?: string[][], + channelId?: string | null, + threadContext?: { + parentEventId: string | null; + threadHeadId: string | null; + } | null, ) => Promise >; richText: Pick; @@ -128,6 +145,10 @@ export function useMentionSendFlow({ const isMentionSendPendingRef = React.useRef(false); const isCompleteSendPendingRef = React.useRef(false); const previousChannelIdRef = React.useRef(channelId); + // Tracks the live channel so completeSend can ask "is the user still here?" + // without being frozen to the compose-time closure. + const channelIdRef = React.useRef(channelId); + channelIdRef.current = channelId; const addMembersMutation = useAddChannelMembersMutation(channelId); const attachAgentMutation = useAttachManagedAgentToChannelMutation(channelId); @@ -170,8 +191,8 @@ export function useMentionSendFlow({ ]); const ensureManagedAgentMentionsReady = React.useCallback( - async (mentionPubkeys: string[]) => { - if (!channelId || mentionPubkeys.length === 0) { + async (mentionPubkeys: string[], capturedChannelId: string) => { + if (!capturedChannelId || mentionPubkeys.length === 0) { return []; } @@ -195,6 +216,7 @@ export function useMentionSendFlow({ } } else { await attachAgentMutation.mutateAsync({ + channelId: capturedChannelId, agent, role: "bot", }); @@ -213,7 +235,6 @@ export function useMentionSendFlow({ }, [ attachAgentMutation, - channelId, getManagedAgentsByPubkey, mentions.memberPubkeys, startAgentMutation, @@ -221,9 +242,9 @@ export function useMentionSendFlow({ ); const createMentionedPersonaAgents = React.useCallback( - async (trimmed: string) => { + async (trimmed: string, capturedChannelId: string) => { const personaMentions = mentions.extractMentionPersonas(trimmed); - if (!channelId || personaMentions.length === 0) { + if (!capturedChannelId || personaMentions.length === 0) { return { errors: [] as string[], pubkeys: [] as string[], @@ -254,6 +275,7 @@ export function useMentionSendFlow({ try { const result = await createPersonaAgentMutation.mutateAsync({ + channelId: capturedChannelId, runtime, name: persona.displayName, personaId: persona.id, @@ -284,7 +306,6 @@ export function useMentionSendFlow({ }; }, [ - channelId, createPersonaAgentMutation, getAvailableRuntimes, mentions.extractMentionPersonas, @@ -346,6 +367,7 @@ export function useMentionSendFlow({ mentionPubkeys.filter( (pubkey) => !readyAgentPubkeys.has(normalizePubkey(pubkey)), ), + draft.capturedChannelId ?? "", ); if (agentReadinessErrors.length > 0) { const message = @@ -359,25 +381,36 @@ export function useMentionSendFlow({ return; } - clearComposer(); + // Only clear the composer if the user has not switched channels since + // submit. If they have, the composer they see belongs to the new channel + // and we must not wipe it. + if (draft.capturedChannelId === channelIdRef.current) { + clearComposer(); + } try { await onSendRef.current( draft.finalContent, mentionPubkeys, outgoingTags, + draft.capturedChannelId, + draft.capturedThreadContext, ); if (draft.sentDraftKey) { drafts.clearDraft(draft.sentDraftKey); } } catch { - setContent(draft.savedContent); - contentRef.current = draft.savedContent; - richText.setContent(draft.savedContent); - setPendingImeta(draft.savedImeta); - setSpoileredAttachmentUrls?.( - new Set(draft.savedSpoileredAttachmentUrls), - ); + // Only restore the composer content if the user is still on the + // channel that originated the send. + if (draft.capturedChannelId === channelIdRef.current) { + setContent(draft.savedContent); + contentRef.current = draft.savedContent; + richText.setContent(draft.savedContent); + setPendingImeta(draft.savedImeta); + setSpoileredAttachmentUrls?.( + new Set(draft.savedSpoileredAttachmentUrls), + ); + } } } finally { isCompleteSendPendingRef.current = false; @@ -416,6 +449,8 @@ export function useMentionSendFlow({ const sendMessageWithMentionFlow = React.useCallback( async ({ + capturedChannelId, + capturedThreadContext = null, pendingImeta, sentDraftKey, spoileredAttachmentUrls = new Set(), @@ -428,8 +463,10 @@ export function useMentionSendFlow({ isMentionSendPendingRef.current = true; setIsMentionSendPending(true); try { - const personaMentionResult = - await createMentionedPersonaAgents(trimmed); + const personaMentionResult = await createMentionedPersonaAgents( + trimmed, + capturedChannelId ?? "", + ); if (personaMentionResult.errors.length > 0) { const message = personaMentionResult.errors.length === 1 @@ -479,6 +516,8 @@ export function useMentionSendFlow({ } const pendingDraft: PendingNonMemberMentionSend = { + capturedChannelId, + capturedThreadContext, finalContent, mentionPubkeys: pubkeys, nonMemberPubkeys: promptNonMemberPubkeys, @@ -578,6 +617,7 @@ export function useMentionSendFlow({ const errors: string[] = []; if (peoplePubkeys.length > 0) { const result = await addMembersMutation.mutateAsync({ + channelId: pendingNonMemberSend.capturedChannelId ?? undefined, pubkeys: peoplePubkeys, role: "member", }); @@ -586,6 +626,7 @@ export function useMentionSendFlow({ if (relayAgentPubkeys.length > 0) { const result = await addMembersMutation.mutateAsync({ + channelId: pendingNonMemberSend.capturedChannelId ?? undefined, pubkeys: relayAgentPubkeys, role: "bot", }); diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 7e5ab1183..5008656f7 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -91,6 +91,7 @@ type E2eConfig = { relayAgents?: MockRelayAgentSeed[]; agentListDelayMs?: number; agentMemory?: RawAgentMemoryListing | Record; + addChannelMembersDelayMs?: number; createManagedAgentDelayMs?: number; channelsReadError?: string; feedReadError?: string; @@ -4962,6 +4963,12 @@ async function handleAddChannelMembers( }, config: E2eConfig | undefined, ): Promise { + const addChannelMembersDelayMs = config?.mock?.addChannelMembersDelayMs ?? 0; + if (addChannelMembersDelayMs > 0) { + await new Promise((resolve) => + window.setTimeout(resolve, addChannelMembersDelayMs), + ); + } const identity = getIdentity(config); if (!identity) { const channel = getMockChannel(args.channelId); diff --git a/desktop/tests/e2e/send-channel-binding.spec.ts b/desktop/tests/e2e/send-channel-binding.spec.ts new file mode 100644 index 000000000..c1b518192 --- /dev/null +++ b/desktop/tests/e2e/send-channel-binding.spec.ts @@ -0,0 +1,189 @@ +/** + * E2E regression for the wrong-channel send bug. + * + * Repro: compose a message in channel A that tags a non-member managed agent + * (which forces the slow `add_channel_members` path), submit, then immediately + * switch to channel B before the agent-attach await resolves. Without the fix, + * the message lands in B's timeline; with the fix it must land in A's. + * + * The `addChannelMembersDelayMs` bridge knob holds the `add_channel_members` + * handler open long enough for the channel click to race the in-flight send. + */ + +import { expect, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; + +// A managed agent that is NOT a member of any channel in the seed data. +const OUT_OF_CHANNEL_BOT_PUBKEY = + "ee00000000000000000000000000000000000000000000000000000000000001"; + +/** Locator scoped to the mention autocomplete dropdown inside the composer. */ +function autocomplete(page: import("@playwright/test").Page) { + return page + .getByTestId("message-composer") + .getByTestId("mention-autocomplete"); +} + +async function readCommandLog(page: import("@playwright/test").Page) { + return page.evaluate(() => { + return ( + (window as Window & { __BUZZ_E2E_COMMANDS__?: string[] }) + .__BUZZ_E2E_COMMANDS__ ?? [] + ); + }); +} + +function commandCount(commands: string[], command: string) { + return commands.filter((c) => c === command).length; +} + +// The channel timeline renders off a `useDeferredValue` snapshot; poll for the +// pending marker to clear before asserting on freshly-sent content. +async function waitForTimelineSettled(page: import("@playwright/test").Page) { + await expect(page.locator("[data-render-pending]")).toHaveCount(0); +} + +// --------------------------------------------------------------------------- +// Main regression: message always lands in the compose-time channel +// --------------------------------------------------------------------------- + +test("message with agent mention lands in compose-time channel despite mid-send navigation", async ({ + page, +}) => { + const MESSAGE_TEXT = `send-binding-repro-${Date.now()}`; + + // Install bridge with: + // - a managed agent that is NOT in general (forces add_channel_members path) + // - a 500ms delay on add_channel_members to open the race window + await installMockBridge(page, { + addChannelMembersDelayMs: 500, + managedAgents: [ + { + pubkey: OUT_OF_CHANNEL_BOT_PUBKEY, + name: "BotA", + status: "running", + // No channelNames → agent is not a member of any channel + }, + ], + }); + + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + + // Type a message that mentions the out-of-channel agent + const input = page.getByTestId("message-input"); + await input.fill("@BotA"); + + const dropdown = autocomplete(page); + const botRow = dropdown.locator("button", { hasText: "BotA" }); + await expect(botRow).toBeVisible(); + await expect(botRow.getByText("not in channel")).toBeVisible(); + // Select BotA from the autocomplete + await input.press("Enter"); + await page.keyboard.type(` ${MESSAGE_TEXT}`); + + // Verify the mention chip is present before submitting + const composerChip = input.locator(".agent-mention-highlight", { + hasText: "BotA", + }); + await expect(composerChip).toBeVisible(); + + // Snapshot the baseline command count before sending + const baselineCommands = await readCommandLog(page); + const baselineAddCount = commandCount( + baselineCommands, + "add_channel_members", + ); + + // Submit the message — this triggers the async add_channel_members path + await page.getByTestId("send-message").click(); + + // Immediately switch to channel-agents BEFORE the 500ms delay resolves. + // This is the race the fix closes. + await page.getByTestId("channel-agents").click(); + await expect(page.getByTestId("chat-title")).toHaveText("agents"); + + // Wait for add_channel_members to fire (confirms the race window opened and + // the fix's captured channel id was used for the agent-attach call). + await expect + .poll(async () => + commandCount(await readCommandLog(page), "add_channel_members"), + ) + .toBeGreaterThan(baselineAddCount); + + // Let the in-flight send finish (500ms delay + buffer). + await page.waitForTimeout(800); + + // --- Assert message landed in general (compose-time channel) --- + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + await waitForTimelineSettled(page); + + // The message must appear in general's timeline (not the switched-to channel). + await expect(page.getByTestId("message-timeline")).toContainText( + MESSAGE_TEXT, + ); + + // --- Assert message did NOT land in agents (switched-to channel) --- + await page.getByTestId("channel-agents").click(); + await expect(page.getByTestId("chat-title")).toHaveText("agents"); + await waitForTimelineSettled(page); + + await expect(page.getByTestId("message-timeline")).not.toContainText( + MESSAGE_TEXT, + ); +}); + +// --------------------------------------------------------------------------- +// Invariant: without mid-send navigation, normal agent-mention send still works +// --------------------------------------------------------------------------- + +test("message with agent mention delivers correctly when no channel switch occurs", async ({ + page, +}) => { + const MESSAGE_TEXT = `no-switch-verify-${Date.now()}`; + + await installMockBridge(page, { + addChannelMembersDelayMs: 0, + managedAgents: [ + { + pubkey: OUT_OF_CHANNEL_BOT_PUBKEY, + name: "BotA", + status: "running", + }, + ], + }); + + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + + const input = page.getByTestId("message-input"); + await input.fill("@BotA"); + const dropdown = autocomplete(page); + await expect(dropdown.locator("button", { hasText: "BotA" })).toBeVisible(); + await input.press("Enter"); + await page.keyboard.type(` ${MESSAGE_TEXT}`); + + const baselineCommands = await readCommandLog(page); + const baselineAddCount = commandCount( + baselineCommands, + "add_channel_members", + ); + + await page.getByTestId("send-message").click(); + + // Wait for the agent-attach step to complete before asserting the timeline. + await expect + .poll(async () => + commandCount(await readCommandLog(page), "add_channel_members"), + ) + .toBeGreaterThan(baselineAddCount); + + await waitForTimelineSettled(page); + await expect(page.getByTestId("message-timeline")).toContainText( + MESSAGE_TEXT, + ); +}); diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index eef98632b..1073d7bea 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -110,6 +110,7 @@ type MockBridgeOptions = { relayAgents?: MockRelayAgentSeed[]; agentListDelayMs?: number; createManagedAgentDelayMs?: number; + addChannelMembersDelayMs?: number; channelsReadError?: string; feedReadError?: string; canvasReadError?: string;