From 11dc6c4e0cf74f7fbdb2f5887bab2f0c518806da Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Mon, 31 Aug 2026 14:28:39 -0400 Subject: [PATCH] fix(desktop): bind duplicate mention selections to exact recipients Extract behavior B from #7114 after the spacing fix. Preserve ambiguous-name rejection across chat, edits, forum and onboarding; keep exact labels through team selection and automatic addressing. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson (cherry picked from commit 46f9b75bcc3e735d351e0587cefc105e66d6da2c) Signed-off-by: Logan Johnson (cherry picked from commit 86b4e6cef624289a6e2c033327a241c2f6031274) Signed-off-by: Logan Johnson --- AGENTS.md | 6 + desktop/playwright.config.ts | 1 + .../src/features/forum/ui/ForumComposer.tsx | 3 +- .../messages/lib/extractMentionPubkeys.ts | 64 ++++++++- .../messages/lib/mentionCandidates.test.mjs | 15 +- .../messages/lib/mentionCandidates.ts | 9 +- .../messages/lib/useMentions.test.mjs | 82 ++++++++++- .../src/features/messages/lib/useMentions.ts | 28 +++- .../messages/ui/submitMessageEdit.test.mjs | 52 +++++++ .../features/messages/ui/submitMessageEdit.ts | 23 ++- .../ui/useAgentAddressLockPicker.test.mjs | 110 +++++++++++++- .../messages/ui/useAgentAddressLockPicker.ts | 61 ++++---- .../messages/ui/useMentionSendFlow.ts | 4 + desktop/tests/e2e/mention-recipients.spec.ts | 134 ++++++++++++++++++ desktop/tests/e2e/mentions.spec.ts | 4 +- desktop/tests/e2e/onboarding.spec.ts | 42 +++++- docs/mention-editor.md | 23 +++ 17 files changed, 594 insertions(+), 67 deletions(-) create mode 100644 desktop/tests/e2e/mention-recipients.spec.ts diff --git a/AGENTS.md b/AGENTS.md index 6972960f32..4f3ea1aa9e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -738,3 +738,9 @@ Autocomplete inserts a literal full label and a separator, including multi-word names. Only autocomplete settlement may move the caret past that separator; internal label spaces and deliberate ArrowLeft/click movement must be respected. See `docs/mention-editor.md` and `desktop/tests/e2e/mention-spacing.spec.ts`. + +Selected mention labels bind exact keys, including same-name teammates and +persistent automatic addresses. Use the returned label from registration for +insert/restore/remove. Ambiguous manually typed names must fail visibly without +clearing the draft in chat, edit, and standalone forum consumers; never fan out +silently to all identities sharing a name. See `docs/mention-editor.md`. diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index e016c9e3f2..a3d8412de4 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -76,6 +76,7 @@ export default defineConfig({ "**/mentions.spec.ts", "**/remote-owned-mentions.spec.ts", "**/mention-spacing.spec.ts", + "**/mention-recipients.spec.ts", "**/team-mentions.spec.ts", "**/persistent-agent-audience.spec.ts", "**/relay-reconnect.spec.ts", diff --git a/desktop/src/features/forum/ui/ForumComposer.tsx b/desktop/src/features/forum/ui/ForumComposer.tsx index bfb38cd700..d4c61892f8 100644 --- a/desktop/src/features/forum/ui/ForumComposer.tsx +++ b/desktop/src/features/forum/ui/ForumComposer.tsx @@ -280,7 +280,8 @@ export function ForumComposer({ if (compact) setIsCompactExpanded(true); } } catch (error) { - // Fresh authorization failures must be visible; preserve the draft. + // Authorization and ambiguous-name failures must be visible, not a + // silent no-op. This path has not cleared the draft or its selections. toast.error(error instanceof Error ? error.message : String(error)); } finally { isSubmissionPendingRef.current = false; diff --git a/desktop/src/features/messages/lib/extractMentionPubkeys.ts b/desktop/src/features/messages/lib/extractMentionPubkeys.ts index 29193ec30a..9a84f39ce4 100644 --- a/desktop/src/features/messages/lib/extractMentionPubkeys.ts +++ b/desktop/src/features/messages/lib/extractMentionPubkeys.ts @@ -15,6 +15,51 @@ function normalizeDisplayName(name: string): string { return name.trim().toLowerCase(); } +/** Keep a second same-name selection from rebinding text already in the draft. */ +export function selectedMentionLabel( + displayName: string, + pubkey: string, + selectedMentions: ReadonlyMap, +): string { + const bindings = new Map( + [...selectedMentions].map(([label, key]) => [ + normalizeDisplayName(label), + key.toLowerCase(), + ]), + ); + const conflicts = (label: string) => { + const existing = bindings.get(normalizeDisplayName(label)); + return existing !== undefined && existing !== pubkey.toLowerCase(); + }; + if (!conflicts(displayName)) return displayName; + const qualified = `${displayName} (${pubkey.toLowerCase()})`; + let label = qualified; + let suffix = 2; + // A display name may itself look qualified. Never overwrite that binding. + while (conflicts(label)) label = `${qualified} ${suffix++}`; + return label; +} + +/** Reserve each label before binding the next identity in a multi-agent selection. */ +export function selectedMentionLabels< + T extends { displayName: string; pubkey?: string }, +>( + selections: readonly T[], + selectedMentions: ReadonlyMap, +): T[] { + const bindings = new Map(selectedMentions); + return selections.map((selected) => { + if (!selected.pubkey) return selected; + const displayName = selectedMentionLabel( + selected.displayName, + selected.pubkey, + bindings, + ); + bindings.set(displayName, selected.pubkey); + return { ...selected, displayName }; + }); +} + /** * Returns explicit selected mention pubkeys and manually typed channel-member * mentions. At each `@` offset, only the longest valid display name wins so a @@ -71,10 +116,21 @@ export function extractMentionPubkeys({ const longestNameLength = Math.max( ...matches.map((match) => match.displayName.length), ); - for (const match of matches) { - if (match.pubkey && match.displayName.length === longestNameLength) { - winningPubkeys.add(match.pubkey); - } + const winners = matches.filter( + (match) => match.displayName.length === longestNameLength, + ); + const identities = new Set( + winners.flatMap((match) => + match.pubkey ? [match.pubkey.toLowerCase()] : [], + ), + ); + if (identities.size > 1) { + throw new Error( + `The mention @${winners[0].displayName} is ambiguous. Choose a recipient from the mention picker.`, + ); + } + for (const match of winners) { + if (match.pubkey) winningPubkeys.add(match.pubkey); } } diff --git a/desktop/src/features/messages/lib/mentionCandidates.test.mjs b/desktop/src/features/messages/lib/mentionCandidates.test.mjs index 355b56dfea..93402e92ba 100644 --- a/desktop/src/features/messages/lib/mentionCandidates.test.mjs +++ b/desktop/src/features/messages/lib/mentionCandidates.test.mjs @@ -127,7 +127,7 @@ test("only complete, owned teams with mentionable members are suggested", () => ); }); -test("teams with duplicate identity display names are not suggested", () => { +test("teams with duplicate identity display names remain selectable with their exact keys", () => { const personas = [ persona("builder-one", "First"), persona("builder-two", "Second"), @@ -137,13 +137,14 @@ test("teams with duplicate identity display names are not suggested", () => { identity("builder-two", "Builder", { pubkey: "2".repeat(64) }), ]; + const suggestions = buildTeamMentionCandidates( + [team("duplicate-identities", ["builder-one", "builder-two"])], + personas, + candidates, + ); assert.deepEqual( - buildTeamMentionCandidates( - [team("duplicate-identities", ["builder-one", "builder-two"])], - personas, - candidates, - ), - [], + suggestions[0].teamMembers.map((member) => member.pubkey), + ["1".repeat(64), "2".repeat(64)], ); }); diff --git a/desktop/src/features/messages/lib/mentionCandidates.ts b/desktop/src/features/messages/lib/mentionCandidates.ts index 659832dead..2ee46b0141 100644 --- a/desktop/src/features/messages/lib/mentionCandidates.ts +++ b/desktop/src/features/messages/lib/mentionCandidates.ts @@ -128,11 +128,14 @@ export function buildTeamMentionCandidates( .filter((member): member is TeamMentionMember => member !== null); if (teamMembers.length !== resolution.resolvedPersonas.length) return []; - const mentionNames = new Set(); + const mentionNames = new Map(); for (const member of teamMembers) { const mentionName = member.displayName.trim().toLowerCase(); - if (mentionNames.has(mentionName)) return []; - mentionNames.add(mentionName); + const previous = mentionNames.get(mentionName); + // Exact-key members can reserve distinct labels at selection time. A + // persona without a key cannot yet be disambiguated that way. + if (previous && (!previous.pubkey || !member.pubkey)) return []; + mentionNames.set(mentionName, member); } return [ diff --git a/desktop/src/features/messages/lib/useMentions.test.mjs b/desktop/src/features/messages/lib/useMentions.test.mjs index 312285b7b2..eb3e0bbc12 100644 --- a/desktop/src/features/messages/lib/useMentions.test.mjs +++ b/desktop/src/features/messages/lib/useMentions.test.mjs @@ -6,7 +6,11 @@ import { getMentionOffsets, hasMention, } from "./hasMention.ts"; -import { extractMentionPubkeys } from "./extractMentionPubkeys.ts"; +import { + extractMentionPubkeys, + selectedMentionLabel, + selectedMentionLabels, +} from "./extractMentionPubkeys.ts"; // ── Plain @mention ──────────────────────────────────────────────────── @@ -245,3 +249,79 @@ test("requires matching inline-code delimiter lengths", () => { assert.equal(hasMention("`` @Alice ` still code ``", "Alice"), false); assert.equal(hasMention("`` @Alice `", "Alice"), true); }); + +for (const selected of [false, true]) { + test(`duplicate names ${selected ? "stay bound to selection after rename" : "require explicit selection"}`, () => { + const opts = { + text: "@Scout hello", + selectedMentions: new Map(selected ? [["Scout", "first"]] : []), + memberCandidates: [ + { + pubkey: "first", + displayName: selected ? "Renamed" : "Scout", + isMember: true, + }, + { pubkey: "second", displayName: "Scout", isMember: true }, + ], + }; + if (selected) assert.deepEqual(extractMentionPubkeys(opts), ["first"]); + else + assert.throws( + () => extractMentionPubkeys(opts), + /ambiguous.*Choose a recipient/, + ); + }); +} + +test("a second same-name selection cannot redirect the first mention", () => { + const selected = new Map([["Scout", "first"]]); + const secondLabel = selectedMentionLabel("Scout", "second", selected); + selected.set(secondLabel, "second"); + assert.deepEqual( + extractMentionPubkeys({ + text: `@Scout and @${secondLabel}`, + selectedMentions: selected, + memberCandidates: [], + }), + ["first", "second"], + ); +}); + +test("qualified-looking names cannot redirect an existing selection", () => { + const selected = new Map([ + ["Scout", "first"], + ["Scout (second)", "third"], + ]); + const label = selectedMentionLabel("Scout", "second", selected); + assert.notEqual(label.toLowerCase(), "scout (second)"); + selected.set(label, "second"); + assert.deepEqual( + extractMentionPubkeys({ + text: `@Scout and @Scout (second) and @${label}`, + selectedMentions: selected, + memberCandidates: [], + }), + ["first", "third", "second"], + ); + assert.equal(selectedMentionLabel("Scout", "second", selected), label); +}); + +test("same-name teammates are bound sequentially without replacing either recipient", () => { + const selected = selectedMentionLabels( + [ + { displayName: "Scout", pubkey: "first" }, + { displayName: "Scout", pubkey: "second" }, + ], + new Map(), + ); + const bindings = new Map(selected.map((s) => [s.displayName, s.pubkey])); + assert.equal(bindings.size, 2); + assert.deepEqual( + extractMentionPubkeys({ + text: selected.map((s) => `@${s.displayName}`).join(" "), + selectedMentions: bindings, + memberCandidates: [], + }), + ["first", "second"], + ); +}); diff --git a/desktop/src/features/messages/lib/useMentions.ts b/desktop/src/features/messages/lib/useMentions.ts index 03d7d32c5a..754bfe0b43 100644 --- a/desktop/src/features/messages/lib/useMentions.ts +++ b/desktop/src/features/messages/lib/useMentions.ts @@ -36,7 +36,11 @@ import { useActiveAgentPubkeys } from "./useActiveAgentPubkeys"; import { useDefaultAgentSuggestion } from "./useDefaultAgentSuggestion"; import { flushMentionDebounce, isPlainSpace } from "./flushMentionDebounce"; import { useAgentMentionRevalidation } from "./agentMentionRevalidation"; -import { extractMentionPubkeys } from "./extractMentionPubkeys"; +import { + extractMentionPubkeys, + selectedMentionLabel, + selectedMentionLabels, +} from "./extractMentionPubkeys"; import { extractMentionPersonasFromMaps, type PersonaMentionTarget, @@ -448,15 +452,21 @@ export function useMentions( clearTimeout(debounceTimerRef.current); debounceTimerRef.current = null; } - const displayName = suggestion.displayName; + const [boundSuggestion] = selectedMentionLabels( + [suggestion], + mentionMapRef.current, + ); + const displayName = boundSuggestion.displayName; const teamMembers = - suggestion.kind === "team" ? suggestion.teamMembers : null; + suggestion.kind === "team" && suggestion.teamMembers + ? selectedMentionLabels(suggestion.teamMembers, mentionMapRef.current) + : null; const insertText = teamMembers ? formatTeamMention(displayName, teamMembers) : `@${displayName} `; const mentions = mentionMapRef.current; const personaMentions = personaMentionMapRef.current; - const selectedMentions = teamMembers ?? [suggestion]; + const selectedMentions = teamMembers ?? [boundSuggestion]; for (const selected of selectedMentions) { if (selected.kind === "persona" && selected.personaId) { personaMentions.set(selected.displayName, selected.personaId); @@ -518,7 +528,11 @@ export function useMentions( ); const registerMentionPubkey = React.useCallback( (displayName: string, pubkey: string, options?: { isAgent?: boolean }) => { - const trimmedName = displayName.trim(); + const trimmedName = selectedMentionLabel( + displayName.trim(), + pubkey, + mentionMapRef.current, + ); if (!trimmedName) { return; } @@ -555,11 +569,11 @@ export function useMentions( replaceToOffset: number; isAgent?: boolean; }): AutocompleteEdit => { - registerMentionPubkey(displayName, pubkey, { isAgent }); + const label = registerMentionPubkey(displayName, pubkey, { isAgent }); return { replaceFromOffset, replaceToOffset, - insertText: `@${displayName.trim()} `, + insertText: `@${label ?? displayName.trim()} `, }; }, [registerMentionPubkey], diff --git a/desktop/src/features/messages/ui/submitMessageEdit.test.mjs b/desktop/src/features/messages/ui/submitMessageEdit.test.mjs index ffebe51167..5d29996592 100644 --- a/desktop/src/features/messages/ui/submitMessageEdit.test.mjs +++ b/desktop/src/features/messages/ui/submitMessageEdit.test.mjs @@ -149,3 +149,55 @@ test("edit upload pause revalidates revoked mentions only after upload completes ["error", new AgentMentionAuthorizationError().message], ]); }); + +test("ambiguous extractor failure is visible before edit draft clearing or save", async () => { + const calls = []; + const error = + "The mention @Scout is ambiguous. Choose a recipient from the mention picker."; + await submitMessageEdit({ + ...baseOptions(async () => calls.push("save")), + extractMentionPubkeys: () => { + throw new Error(error); + }, + clearComposer: () => calls.push("clear"), + setUploadError: (message) => calls.push(message), + }); + assert.deepEqual(calls, [error]); +}); + +for (const replacement of ["hello", "hello @Alice"]) { + test(`an ambiguous historical mention can be replaced with ${replacement}`, async () => { + const { extractMentionPubkeys } = await import( + "../lib/extractMentionPubkeys.ts" + ); + const alice = "e".repeat(64); + const calls = []; + await submitMessageEdit({ + ...baseOptions(async (_content, _tags, pubkeys) => + calls.push(["save", pubkeys]), + ), + content: replacement, + originalContent: "hello @Scout", + extractMentionPubkeys: (text) => + extractMentionPubkeys({ + text, + selectedMentions: new Map(), + memberCandidates: [ + { displayName: "Scout", pubkey: "c".repeat(64), isMember: true }, + { displayName: "Scout", pubkey: "d".repeat(64), isMember: true }, + { displayName: "Alice", pubkey: alice, isMember: true }, + ], + }), + revalidateMentionPubkeys: async (pubkeys) => { + calls.push(["revalidate", pubkeys]); + return pubkeys; + }, + setUploadError: (error) => calls.push(["error", error]), + }); + const expected = replacement.includes("@Alice") ? [alice] : []; + assert.deepEqual(calls, [ + ["revalidate", expected], + ["save", expected], + ]); + }); +} diff --git a/desktop/src/features/messages/ui/submitMessageEdit.ts b/desktop/src/features/messages/ui/submitMessageEdit.ts index a05daf2a10..db08b923eb 100644 --- a/desktop/src/features/messages/ui/submitMessageEdit.ts +++ b/desktop/src/features/messages/ui/submitMessageEdit.ts @@ -102,11 +102,24 @@ export async function submitMessageEdit({ restoreMentionRefs(draft.mentionRefs); } }; - const addedMentionPubkeys = diffAddedMentionPubkeys( - extractMentionPubkeys(originalContent), - extractMentionPubkeys(content), - ownerPubkey ?? "", - ); + let originalMentionPubkeys: string[] = []; + try { + originalMentionPubkeys = extractMentionPubkeys(originalContent); + } catch { + // An old ambiguous label must not prevent removing it. If the original + // cannot be resolved, conservatively revalidate every current recipient. + } + let addedMentionPubkeys: string[]; + try { + addedMentionPubkeys = diffAddedMentionPubkeys( + originalMentionPubkeys, + extractMentionPubkeys(content), + ownerPubkey ?? "", + ); + } catch (error) { + setUploadError(error instanceof Error ? error.message : String(error)); + return; + } const hasQueuedAttachments = draft.queuedAttachments.length > 0; if (hasQueuedAttachments) setDeferredUploadPending(true); clearComposer(); diff --git a/desktop/src/features/messages/ui/useAgentAddressLockPicker.test.mjs b/desktop/src/features/messages/ui/useAgentAddressLockPicker.test.mjs index 8ed155234f..a01d3694c5 100644 --- a/desktop/src/features/messages/ui/useAgentAddressLockPicker.test.mjs +++ b/desktop/src/features/messages/ui/useAgentAddressLockPicker.test.mjs @@ -400,7 +400,10 @@ test("restoring a multi-word automatic mention into an empty composer focuses af mentions: { getDraftMentionRefs: () => [], getMentionDisplayName: () => "claude code", - registerMentionPubkey: (...args) => registeredMentions.push(args), + registerMentionPubkey: (...args) => { + registeredMentions.push(args); + return args[0]; + }, }, onPulseAddressLock: () => {}, richText: { @@ -491,7 +494,10 @@ test("restoring an existing automatic mention re-registers its agent chip", asyn mentions: { getDraftMentionRefs: () => [], getMentionDisplayName: () => "claude code", - registerMentionPubkey: (...args) => registeredMentions.push(args), + registerMentionPubkey: (...args) => { + registeredMentions.push(args); + return args[0]; + }, }, onPulseAddressLock: () => {}, richText: { @@ -809,7 +815,10 @@ test("restoring after an agent rename keeps the existing automatic mention", asy { displayName: oldName, pubkey: "agent-pubkey", isAgent: true }, ], getMentionDisplayName: () => displayName, - registerMentionPubkey: (...args) => registeredMentions.push(args), + registerMentionPubkey: (...args) => { + registeredMentions.push(args); + return args[0]; + }, }, onPulseAddressLock: () => {}, profiles: {}, @@ -828,7 +837,7 @@ test("restoring after an agent rename keeps the existing automatic mention", asy assert.deepEqual(appliedEdits, []); assert.deepEqual(registeredMentions, [ - [newName, "agent-pubkey", { isAgent: true }], + [oldName, "agent-pubkey", { isAgent: true }], ]); }); @@ -865,3 +874,96 @@ test("an addressed agent keeps its resolved name while mention state clears duri assert.equal(result.current.lockedAgents[0].displayName, "Agent Ada"); }); + +test("automatic mention insertion and restoration use the registered collision-safe label", async () => { + const { act, renderHook } = await import("@testing-library/react"); + const { useAgentAddressLockPicker } = await import( + "./useAgentAddressLockPicker.ts" + ); + const { selectedMentionLabel, extractMentionPubkeys } = await import( + "../lib/extractMentionPubkeys.ts" + ); + const { snapshotDraftMentionRefs } = await import( + "../lib/draftMentionRefs.ts" + ); + const local = "a".repeat(64); + const remote = "e".repeat(64); + const bindings = new Map([["carl", local]]); + let text = "@carl existing"; + const prefixes = []; + const mentions = { + getDraftMentionRefs: (value) => + snapshotDraftMentionRefs(value, bindings, [...bindings.keys()]), + getMentionDisplayName: (pubkey) => + [...bindings].find(([, key]) => key === pubkey)?.[0] ?? "carl", + registerMentionPubkey: (name, pubkey) => { + const label = selectedMentionLabel(name, pubkey, bindings); + bindings.set(label, pubkey); + return label; + }, + isMentionOpen: false, + }; + const { result, rerender } = renderHook( + ({ pubkeys }) => + useAgentAddressLockPicker({ + audience: { pubkeys, addPubkey: () => {}, removePubkey: () => {} }, + audienceScope: "channel", + mentions, + profiles: { [remote]: { displayName: "carl" } }, + onPulseAddressLock: () => {}, + onImplicitPrefixInserted: (value) => prefixes.push(...value), + applyAutocompleteEdit: (edit) => { + text = + text.slice(0, edit.replaceFromOffset) + + edit.insertText + + text.slice(edit.replaceToOffset); + }, + richText: { + getPlainTextAndCursor: () => ({ text, cursor: text.length }), + focusEnd: () => {}, + }, + }), + { initialProps: { pubkeys: [] } }, + ); + act(() => + result.current.toggleAlwaysAddressAgent({ + displayName: "carl", + pubkey: remote, + isAgent: true, + }), + ); + assert.equal(text, `@carl (${remote}) @carl existing`); + assert.deepEqual( + extractMentionPubkeys({ + text, + selectedMentions: bindings, + memberCandidates: [], + }), + [local, remote], + ); + rerender({ pubkeys: [remote] }); + act(() => result.current.restoreAddressedAgentMentions()); + assert.equal( + text, + `@carl (${remote}) @carl existing`, + "restore must not append or rebind the local mention", + ); + act(() => result.current.removeAddressedAgent(remote)); + assert.equal( + text, + "@carl existing", + "unpin removes the qualified prefix only", + ); + text = ""; + act(() => result.current.restoreAddressedAgentMentions([remote], [remote])); + assert.equal(text, `@carl (${remote}) `); + assert.equal(prefixes.at(-1).prefix, `@carl (${remote}) `); + assert.deepEqual( + extractMentionPubkeys({ + text, + selectedMentions: bindings, + memberCandidates: [], + }), + [remote], + ); +}); diff --git a/desktop/src/features/messages/ui/useAgentAddressLockPicker.ts b/desktop/src/features/messages/ui/useAgentAddressLockPicker.ts index 4a9ca5f15c..20ed8be3b1 100644 --- a/desktop/src/features/messages/ui/useAgentAddressLockPicker.ts +++ b/desktop/src/features/messages/ui/useAgentAddressLockPicker.ts @@ -184,9 +184,9 @@ export function useAgentAddressLockPicker({ unpinnedAgentPubkeysRef.current.add(normalized); const excludePubkey = audience.excludePubkey ?? audience.removePubkey; excludePubkey(normalized); - const displayName = lockedAgents.find( - (agent) => agent.pubkey === normalized, - )?.displayName; + const displayName = + mentions.getMentionDisplayName(normalized) ?? + lockedAgents.find((agent) => agent.pubkey === normalized)?.displayName; if (displayName) { const text = richText.getPlainTextAndCursor().text; const implicitPrefix = `@${displayName}${text === `@${displayName}` ? "" : " "}`; @@ -210,6 +210,7 @@ export function useAgentAddressLockPicker({ audience.removePubkey, audienceScope, lockedAgents, + mentions.getMentionDisplayName, onImplicitPrefixRemoved, richText.getPlainTextAndCursor, ], @@ -248,12 +249,13 @@ export function useAgentAddressLockPicker({ ); } else { unpinnedAgentPubkeysRef.current.delete(pubkey); - mentions.registerMentionPubkey(suggestion.displayName, pubkey, { - isAgent: true, - }); + const label = + mentions.registerMentionPubkey(suggestion.displayName, pubkey, { + isAgent: true, + }) ?? suggestion.displayName; const { text } = richText.getPlainTextAndCursor(); - if (getMentionOffsets(text, suggestion.displayName).length === 0) { - const insertedText = `@${suggestion.displayName} `; + if (getMentionOffsets(text, label).length === 0) { + const insertedText = `@${label} `; onImplicitPrefixInserted?.([{ pubkey, prefix: insertedText }]); applyAutocompleteEdit({ replaceFromOffset: 0, @@ -405,37 +407,30 @@ export function useAgentAddressLockPicker({ // refs retain the identity of its already-inserted automatic prefix, so // use that identity as well as the current display name when deciding // whether restoration is needed. - const presentAgentPubkeys = new Set( - mentions - .getDraftMentionRefs(text) - .filter((ref) => ref.isAgent) - .map((ref) => normalizePubkey(ref.pubkey)), - ); + const presentRefs = mentions.getDraftMentionRefs(text); + const missingAgents: Array<{ pubkey: string; displayName: string }> = []; for (const agent of targetAgents) { - if ( - presentAgentPubkeys.has(agent.pubkey) || - getMentionOffsets(text, agent.displayName).length > 0 + const existingRef = presentRefs.find( + (ref) => ref.isAgent && normalizePubkey(ref.pubkey) === agent.pubkey, + ); + // A present exact-key binding wins over renamed or colliding labels. + const displayName = + mentions.registerMentionPubkey( + existingRef?.displayName ?? agent.displayName, + agent.pubkey, + { isAgent: true }, + ) ?? agent.displayName; + if (existingRef || getMentionOffsets(text, displayName).length > 0) { + visibleAgentMentionPubkeysRef.current.add(agent.pubkey); + } else if ( + !unpinnedAgentPubkeysRef.current.has(agent.pubkey) || + allowedUnpinned.has(agent.pubkey) ) { - mentions.registerMentionPubkey(agent.displayName, agent.pubkey, { - isAgent: true, - }); + missingAgents.push({ ...agent, displayName }); visibleAgentMentionPubkeysRef.current.add(agent.pubkey); } } - const missingAgents = targetAgents.filter( - (agent) => - (!unpinnedAgentPubkeysRef.current.has(agent.pubkey) || - allowedUnpinned.has(agent.pubkey)) && - !presentAgentPubkeys.has(agent.pubkey) && - getMentionOffsets(text, agent.displayName).length === 0, - ); if (missingAgents.length === 0) return text; - for (const agent of missingAgents) { - mentions.registerMentionPubkey(agent.displayName, agent.pubkey, { - isAgent: true, - }); - visibleAgentMentionPubkeysRef.current.add(agent.pubkey); - } const insertedText = `${missingAgents .map((agent) => `@${agent.displayName}`) .join(" ")} `; diff --git a/desktop/src/features/messages/ui/useMentionSendFlow.ts b/desktop/src/features/messages/ui/useMentionSendFlow.ts index eda4a07e12..addc6a22eb 100644 --- a/desktop/src/features/messages/ui/useMentionSendFlow.ts +++ b/desktop/src/features/messages/ui/useMentionSendFlow.ts @@ -906,6 +906,10 @@ export function useMentionSendFlow({ } sendPromoted = true; await completeSend(pendingDraft, pubkeys); + } catch (error) { + toast.error( + getErrorMessage(error, "Could not prepare mentions. Please retry."), + ); } finally { if (!sendPromoted) { if (preparedLinkPreviews) { diff --git a/desktop/tests/e2e/mention-recipients.spec.ts b/desktop/tests/e2e/mention-recipients.spec.ts new file mode 100644 index 0000000000..1b5361e22a --- /dev/null +++ b/desktop/tests/e2e/mention-recipients.spec.ts @@ -0,0 +1,134 @@ +import { expect, test, type Page } from "@playwright/test"; +import { waitForAnimations } from "../helpers/animations"; +import { installMockBridge, TEST_IDENTITIES } from "../helpers/bridge"; + +const FIRST = TEST_IDENTITIES.alice.pubkey; +const SECOND = TEST_IDENTITIES.bob.pubkey; +const AMBIGUOUS = + "The mention @Scout is ambiguous. Choose a recipient from the mention picker."; + +async function install(page: Page, channel = "general") { + await installMockBridge(page, { + managedAgents: + channel === "watercooler" + ? ["a".repeat(64), "b".repeat(64)].map((pubkey) => ({ + pubkey, + name: "Scout", + status: "running", + channelNames: ["watercooler"], + })) + : [], + searchProfiles: [FIRST, SECOND].map((pubkey) => ({ + pubkey, + displayName: "Scout", + })), + }); + await page.goto("/"); + await page.getByTestId(`channel-${channel}`).click(); + await expect(page.getByTestId("chat-title")).toHaveText(channel); + if (channel === "watercooler") + await page.getByRole("button", { name: "Start a new post..." }).click(); +} + +async function recipients(page: Page, content: string) { + return page.evaluate( + (content) => + (window.__BUZZ_E2E_SIGNED_EVENTS__ ?? []) + .filter((event) => event.content === content) + .map((event) => + event.tags.filter((tag) => tag[0] === "p").map((tag) => tag[1]), + ), + content, + ); +} + +for (const channel of ["general", "watercooler"]) { + test(`ambiguous typed name is visible and preserves ${channel === "general" ? "chat" : "standalone forum"} draft`, async ({ + page, + }) => { + await install(page, channel); + const input = page.getByTestId("message-input"); + await input.fill("@Scout hello"); + await input.press("Escape"); + await page.getByTestId("send-message").click(); + await expect(page.getByText(AMBIGUOUS, { exact: false })).toBeVisible(); + await expect(input).toHaveText("@Scout hello"); + expect(await recipients(page, "@Scout hello")).toEqual([]); + await waitForAnimations(page); + await page.screenshot({ + path: `test-results/mention-recipients/ambiguous-${channel}.png`, + }); + }); +} + +test("two selected same-name members send both exact identities", async ({ + page, +}) => { + await install(page); + const input = page.getByTestId("message-input"); + await input.fill("@Scout"); + await page.getByTestId(`mention-suggestion-${FIRST}`).click(); + await page.keyboard.type("and @Scout"); + await page.getByTestId(`mention-suggestion-${SECOND}`).click(); + await page.keyboard.type("hello"); + const content = `@Scout and @Scout (${SECOND}) hello`; + await expect(input).toHaveText(content); + await page.getByTestId("send-message").click(); + await expect.poll(() => recipients(page, content)).toEqual([[FIRST, SECOND]]); +}); + +test("ambiguous added mention blocks editing before clearing the draft", async ({ + page, +}) => { + await install(page); + const input = page.getByTestId("message-input"); + await input.fill("original message for ambiguity edit"); + await input.press("Enter"); + const row = page + .getByTestId("message-timeline") + .getByTestId("message-row") + .last(); + await expect(row).toContainText("original message for ambiguity edit"); + await row.hover(); + await row.getByRole("button", { name: "More actions" }).click(); + await page.getByRole("menuitem", { name: "Edit message" }).click(); + await input.fill("edited @Scout hello"); + await page.getByTestId("send-message").click(); + await expect(page.getByText(AMBIGUOUS, { exact: false })).toBeVisible(); + await expect(input).toHaveText("edited @Scout hello"); + await expect(page.getByTestId("edit-target")).toBeVisible(); + expect(await recipients(page, "edited @Scout hello")).toEqual([]); +}); + +test("same-name teammates unfurl into distinct exact-key recipients", async ({ + page, +}) => { + const pubkeys = ["a".repeat(64), "b".repeat(64)]; + await installMockBridge(page, { + personas: pubkeys.map((_, i) => ({ + id: `scout-${i}`, + displayName: "Scout", + systemPrompt: "Help.", + })), + managedAgents: pubkeys.map((pubkey, i) => ({ + pubkey, + personaId: `scout-${i}`, + name: "Scout", + status: "running", + channelNames: ["general"], + })), + teams: [ + { id: "scouts", name: "Scouts", personaIds: ["scout-0", "scout-1"] }, + ], + }); + await page.goto("/"); + await page.getByTestId("channel-general").click(); + const input = page.getByTestId("message-input"); + await input.fill("@Scouts"); + await page.getByTestId("mention-suggestion-team-scouts").click(); + await page.keyboard.type("hello"); + const content = `Scouts(@Scout @Scout (${pubkeys[1]})) hello`; + await expect(input).toHaveText(content); + await page.getByTestId("send-message").click(); + await expect.poll(() => recipients(page, content)).toEqual([pubkeys]); +}); diff --git a/desktop/tests/e2e/mentions.spec.ts b/desktop/tests/e2e/mentions.spec.ts index c17de04554..e392eda5c0 100644 --- a/desktop/tests/e2e/mentions.spec.ts +++ b/desktop/tests/e2e/mentions.spec.ts @@ -472,7 +472,9 @@ test("duplicate owned agents preserve provenance and exact pubkey selection", as // In-channel selections send immediately without opening the prompt. } await expect - .poll(() => readOutgoingMentionPubkeys(page, "@carl remote")) + .poll(() => + readOutgoingMentionPubkeys(page, `@carl (${relayPubkey}) remote`), + ) .toEqual([relayPubkey]); await page.getByTestId("channel-members-trigger").click(); diff --git a/desktop/tests/e2e/onboarding.spec.ts b/desktop/tests/e2e/onboarding.spec.ts index b085e7dab6..9cc17c11ac 100644 --- a/desktop/tests/e2e/onboarding.spec.ts +++ b/desktop/tests/e2e/onboarding.spec.ts @@ -452,8 +452,48 @@ async function expectWelcomeComposerBannerCompletesAfterPersonaMention( throw new Error("Could not measure the Welcome composer"); } - await page.getByTestId("message-input").fill("Thanks @Fizz"); + // The fixture has a seeded Fizz and this new member's starter Fizz. A + // manually typed name cannot choose between them or complete onboarding. + const input = page.getByTestId("message-input"); + const content = "Thanks @Fizz"; + const sentRecipients = () => + page.evaluate( + (content) => + (window.__BUZZ_E2E_SIGNED_EVENTS__ ?? []) + .filter((event) => event.content.trim() === content) + .map((event) => + event.tags.filter((tag) => tag[0] === "p").map((tag) => tag[1]), + ), + content, + ); + await input.fill(content); + await input.press("Escape"); await page.getByTestId("send-message").click(); + await expect( + page.getByText("The mention @Fizz is ambiguous.", { exact: false }), + ).toBeVisible(); + await expect(input).toHaveText(content); + await expect(banner).toHaveAttribute("data-state", "prompt"); + expect(await sentRecipients()).toEqual([]); + + const agents = await invokeMockCommand< + Array<{ pubkey: string; persona_id: string | null; status: string }> + >(page, "list_managed_agents"); + const sameNameAgents = agents.filter( + (agent) => agent.persona_id === "builtin:fizz", + ); + expect(sameNameAgents).toHaveLength(2); + // Onboarding starts the new member's starter; the pre-existing mock stays + // stopped. This identifies the fixture key, not a production routing rule. + const fizz = sameNameAgents.filter((agent) => agent.status === "running"); + expect(fizz).toHaveLength(1); + // Make selection intent explicit; do not remove the colliding fixture or + // relax extraction. The resulting event must tag only our starter identity. + await input.fill(""); + await input.fill(content); + await page.getByTestId(`mention-suggestion-${fizz[0].pubkey}`).click(); + await page.getByTestId("send-message").click(); + await expect.poll(sentRecipients).toEqual([[fizz[0].pubkey]]); await expect(banner).toHaveAttribute("data-state", "complete"); await expect(banner).toHaveAttribute("data-tone", "success"); diff --git a/docs/mention-editor.md b/docs/mention-editor.md index 4bd7d83432..eb6c2932d5 100644 --- a/docs/mention-editor.md +++ b/docs/mention-editor.md @@ -12,3 +12,26 @@ whitespace-run rewrites, internal spaces, and deliberate motion. The ordinary member browser cases in `mention-spacing.spec.ts` test immediate typing and ArrowLeft without requiring remote discovery or invitation; `mentions.spec.ts` also covers clicking chip edges and insertion before existing text. + +## Exact recipient labels + +A selected label is a binding to one exact public key, not a lookup by the latest +profile name. Selecting a second identity with the same name reserves a qualified +label containing its full key (and, if needed, a collision suffix). Team members +reserve labels sequentially. Automatic addressing inserts/restores/removes that +registered label, never a different recipient with the same name. + +Manually typed member names with multiple exact-key matches are rejected with a +visible instruction to use the mention picker. Chat, edits and standalone forum +composition retain their draft and publish nothing on this error. An edit may +remove an ambiguous historical label; when the old content cannot be resolved, +all recipients in the valid replacement are revalidated. Selection stays bound +across profile renames. This does not expand eligibility or change relay +revalidation, invitation or publication authorization. + +Coverage: `useMentions.test.mjs`, `useAgentAddressLockPicker.test.mjs`, +`submitMessageEdit.test.mjs`, `mention-recipients.spec.ts`, and the existing +same-name agent case in `mentions.spec.ts`. The integration-project +`onboarding.spec.ts` checks that an ambiguous Fizz mention cannot complete the +welcome flow, then selects the exact newly started starter and asserts its sole +recipient tag before checking the original completion and layout behavior.