Fix duplicate-name user search results - #6483
wesbillman wants to merge 7 commits into
Conversation
Preserve pubkey-distinct people in mention autocomplete and paginate selected-people agent access search. Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed exact head 9c0eed1234f904d121adce1842b215820d3a0e83 against base 4e3c9e619c93dd26677b392ad1f8cf0d12c8f855.
Two blocking correctness gaps remain:
-
Preserving duplicate rows does not preserve identity after selection.
insertMentionstill recordsMap<displayName, pubkey>atdesktop/src/features/messages/lib/useMentions.ts:607-617. Select two different people namedWilland the secondMap.set("Will", ...)overwrites the first;extractMentionPubkeysthen applies that surviving pubkey to every matching@Willoccurrence (desktop/src/features/messages/lib/extractMentionPubkeys.ts:52-88). Deleting the second visible token can therefore leave the first token notifying the second person. Draft/edit restoration has the same name-keyed shape. The new E2E selects only one duplicate in isolation, so it misses the broken lifecycle. Keep occurrence-bound identity (or insert stable disambiguated labels) and cover select-both, delete-one, send, draft restore, and edit across duplicate person/member/agent/persona labels. -
Selected-people pagination dead-ends when client filtering leaves no scrollable first page.
RespondToField.tsx:136-143removes already selected and archived users, but the onlyfetchNextPagetrigger is attached to the results scroll container, which renders only when filtered results are nonempty (RespondToField.tsx:417-448;features/profile/hooks.ts:481-501). If the raw first 50 matches are filtered out, or leave too few rows to overflow,nextCursorexists but page 2 is unreachable and a valid later person cannot be selected. Prefetch until the visible list can scroll or the query is exhausted (or provide an explicit load-more path), and test both an entirely filtered first page and a non-overflowing first page.
CI was still in progress when this review was submitted; these defects are source-traced and independently reproduced, not CI-derived.
Verify that selecting either same-name search result sends only the pubkey attached to that row. Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Paul, an automated reviewer, commenting via Will's GitHub account. Consolidated feedback from three independent passes (two source reviews, one live E2E run) at head 484b1651e619b829f10548feda8e1acf5ff6908e.
Both source reviews independently reproduced the same two blockers Carl already left above, so I'm confirming rather than restating them in full:
-
Same-name selection still collapses identity after the isolated happy path.
insertMentionkeys the mention map by display name (desktop/src/features/messages/lib/useMentions.ts), so selecting a second distinct "Will" overwrites the first, andextractMentionPubkeysresolves every visible@Willtoken to the survivor. The new E2E tests select each duplicate in separate tests, so they can't expose the collision. Draft/edit restoration shares the name-keyed shape. -
Selected-people pagination can dead-end.
RespondToFieldfilters selected/archived users from each raw page, but the onlyfetchNextPagetrigger isonScrollon a container that renders only when filtered results are nonempty. A fully filtered or non-overflowing first page makes page 2 unreachable even whennextCursorexists — and the allowlist filter grows with every selection. Prefetch until the list can scroll or the query is exhausted, or add an explicit load-more.
Live E2E note: a 51-person adversarial run of the selected-people flow passed — page-2 fetch fired on scroll and respondToAllowlist preserved both same-name pubkeys. That path stores pubkeys directly, so it doesn't refute either blocker: 1 lives in the mention-map lifecycle, and 2 requires a filtered-empty/non-overflowing first page that a scroll can't reach.
Everything else checked out clean: removal of coalesceAutocompleteCandidatesByKey/globalSearchIdentityKey leaves no dangling references, the infinite-query usage matches the PersonaShareRecipients/MembersSidebar pattern, and the same-name coalescing unit test is correct.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: PR base ref 4e3c9e619c93dd26677b392ad1f8cf0d12c8f855 through exact head 484b1651e619b829f10548feda8e1acf5ff6908e
Risk: high — this changes messaging recipient identity and agent-access authorization selection.
Three blocking correctness/trust gaps remain:
-
Selecting two different people with the same display name in one message drops the first identity.
desktop/src/features/messages/lib/useMentions.ts:84,607-616still stores selections asMap<displayName, pubkey>, so the secondWilloverwrites the first.extractMentionPubkeys.ts:30,34-53,81-84can consequently emit only the surviving pubkey. An adversarial E2E selected both duplicate-name rows and sent one composition: expected both pubkeys, observed only the second. The shipped tests select each duplicate only in isolation, so they do not guard the failing lifecycle. Preserve identity by occurrence (or another structure that can hold colliding display names), then add a regression selecting both same-name people in one composition and asserting both outgoingptags. -
The “Selected people” access picker can render two distinct pubkeys with identical visible and accessible identity.
RespondToField.tsx:62-77,422-446uses display name plus NIP-05 and omits the pubkey whenever NIP-05 exists. Two profiles claimingWill/will@example.comtherefore expose identical buttons even though granting access affects files, accounts, and connected tools. The NIP-05 is copied from self-authored kind-0 JSON without verification state (desktop/src-tauri/src/nostr_convert/user_search.rs:11-25), so it is not a unique or trustworthy discriminator. The new E2E covers only duplicate names without NIP-05 (agent-access-warning.spec.ts:131-174). Collision-detect the rendered tuple and include a pubkey-derived discriminator in both the row and accessible name; make the full key keyboard-accessible in line withshared/ui/PubKey.tsx:18-29. Test two pubkeys sharing both display name and NIP-05, keyboard-select each, and assert the exact persisted pubkey. -
Selected-people pagination can dead-end after client filtering.
RespondToField.tsx:136-145filters already-selected and archived people after each server page, while the only next-page trigger isonScrollon the results container (RespondToField.tsx:417-448;features/profile/hooks.ts:481-501). If page 1 is fully filtered, the container is not rendered; if too few rows remain to overflow, no scroll event reaches the threshold. A valid person on page 2 is then unreachable despitehasNextPage. Prefetch until the visible list can scroll or the query is exhausted, or provide an explicit load-more action; cover both a fully filtered first page and a non-overflowing first page.
Validation at exact clean head:
pnpm build:e2e: PASS.- Focused same-name Playwright cases: 3/3 PASS.
just desktop-ci: PASS.- Mutation removing pubkey-based duplicate preservation made the new single-selection tests fail, confirming those tests protect duplicate-row visibility but not the select-both lifecycle.
- Source traced search/coalescing through access mutation payloads; no additional confirmed tenancy/cache or stale-response contamination in the inspected paths.
- GitHub CI at submission: macOS build and Desktop integration green; Desktop Core and smoke shards 1/2/4 still running.
Manual/native evidence: no native capture; these blockers are renderer-contained and were established by executable E2E/source behavior. Residual risk: narrow-layout/theme behavior was not separately captured, and pending CI cannot resolve these untested identity/access paths.
Fetch another user-search page when selected or archived results leave the visible result viewport underfilled. Cover fully filtered and non-scrollable first pages, and capture duplicate-name UI evidence. Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Always expose the canonical pubkey for access-picker search results and include the full key in each Add action accessible name. Verify keyboard selection persists only the chosen identity. Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 4e3c9e619c93dd26677b392ad1f8cf0d12c8f855..206e6074407ebe47cebd69220a5d615afc98b4a3 (exact head 206e6074407ebe47cebd69220a5d615afc98b4a3)
Risk: high — recipient identity and agent-access identity must remain exact when display metadata collides.
Behavior/contracts traced: search pagination/filtering, duplicate-name rendering and accessible disambiguation, keyboard access selection/persistence, mention selection state, and outgoing pubkey extraction.
Blocking finding
- Two distinct same-name recipients still cannot be preserved in one composition.
desktop/src/features/messages/lib/useMentions.ts:84stores selections inMap<displayName, pubkey>;useMentions.ts:607-616sets by display name, so selecting a secondWilloverwrites the first;useMentions.ts:782-789passes the collapsed map into extraction. A clean exact-head adversarial Playwright probe selected pubkeys111…111and222…222as two@Willmentions, expected both outgoing pubkeys, and received only222…222. The shipped cases atdesktop/tests/e2e/mentions.spec.ts:690-735select either duplicate alone and therefore miss the failure. Represent mention identity by occurrence/pubkey rather than display-name key, and add an E2E selecting both identities in one composition, deleting either occurrence, and asserting the exact remaining/sent pubkeys.
Resolved prior findings: the refreshed head now handles filtered/non-scrollable pagination and always exposes pubkey-based visible/accessibility disambiguation in the agent-access picker; the hostile identical-name/NIP-05 keyboard persistence case passes.
Validation at matching clean HEAD: pnpm build:e2e PASS; focused shipped Playwright 5/5 PASS; access collision/pagination 3/3 PASS; same-name single-selection mentions 2/2 PASS; the temporary select-both probe failed causally as described and was removed. One combined Playwright attempt lost its local server and was discarded as infra-invalid.
Manual/native evidence: not run; both the resolved behaviors and remaining failure are renderer-contained and executable/source-deterministic.
Residual risk: Desktop Core and smoke shards were still pending at lane refresh. Their completion cannot repair the reproduced untested recipient-loss path.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 4e3c9e619c93dd26677b392ad1f8cf0d12c8f855..206e6074407ebe47cebd69220a5d615afc98b4a3 (exact head 206e6074407ebe47cebd69220a5d615afc98b4a3)
Risk: high — this changes messaging recipient identity and selected-people access behavior.
The new head resolves the previous pagination and access-picker ambiguity: filtered non-scrollable results continue fetching, each access result visibly exposes its pubkey, and the Add button's accessible name includes the full key. The hostile same-name/same-NIP-05 keyboard case correctly selects the intended pubkey.
Blocking finding
desktop/src/features/messages/lib/useMentions.ts:84,607-616,782-789still keys selected mentions bydisplayName. Selecting pubkey111…111as@Willand then pubkey222…222as another@Willoverwrites the first map entry. The composer can display both tokens, but the outgoing mention list contains only222…222; the first intended recipient is silently lost. This violates the PR's core identity-preservation contract. The added cases indesktop/tests/e2e/mentions.spec.ts:691-735select either identity separately, so they do not cover the failing two-recipient composition.
Please preserve mention identity by occurrence/pubkey rather than display-name key, then add an E2E that selects both same-name identities in one composition, verifies deletion of either occurrence leaves the correct identity, and asserts both exact outgoing pubkeys when both remain.
Validation at matching clean HEAD:
cd desktop && pnpm build:e2e— PASS independently.- Focused shipped same-name and filtered/non-scrollable pagination Playwright coverage — PASS (5/5 in one run; independently repeated as 3/3 access/pagination and 2/2 mention cases).
- Temporary adversarial Playwright selecting both same-name rows — FAIL as expected: expected
[111…111, 222…222], received[222…222]; probe removed and clean head restored. - One earlier combined Playwright attempt lost its local HTTP server after a timeout; treated as infrastructure-invalid and replaced by clean passing focused runs.
Manual/native evidence: not run. The fixed access semantics and remaining blocker are renderer-contained and established directly by source plus executable E2E evidence.
Residual risk: CI was still in progress at review time (Desktop Core/smoke jobs pending), but green CI cannot cover the missing multi-recipient case.
— :bot: Jude’s code review agent
The PR intentionally preserves pubkey-distinct same-name people and already covers selecting each exact row. Remove the superseded E2E that still expects those identities to collapse. Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 4e3c9e619c93dd26677b392ad1f8cf0d12c8f855..1fa8916992c8164ea028cb128aa24f5b4f4fa9cc (exact head 1fa8916992c8164ea028cb128aa24f5b4f4fa9cc)
Risk: high — this changes message recipient identity and access-selection behavior.
Blocking finding: two distinct people with the same display name still cannot both survive one composer send. desktop/src/features/messages/lib/useMentions.ts:84 stores selected mentions as Map<displayName, pubkey>, and useMentions.ts:607-616 writes with mentions.set(selected.displayName, selected.pubkey). Selecting a second Will overwrites the first before the map reaches extractMentionPubkeys (useMentions.ts:782-789; desktop/src/features/messages/lib/extractMentionPubkeys.ts:52-54,81-84). The UI exposes both identities as independently selectable, but serialization emits only the final identity.
This was reproduced through the composer-to-outgoing-event E2E boundary at this exact head: selecting 111…111 and 222…222 as two visible @Will tokens produced only [222…222]. The temporary causal probe was removed afterward and the tree restored clean. The shipped duplicate-name cases in desktop/tests/e2e/mentions.spec.ts:690-735 select each duplicate only in separate compositions, so green CI does not exercise the failing lifecycle. The delta from the previously reviewed head 206e6074407ebe47cebd69220a5d615afc98b4a3 only deletes the obsolete collapse-expectation E2E; the affected production source is unchanged.
Author action: preserve mention identity by token/occurrence/pubkey rather than display-name key. Add a causal E2E that selects both same-name identities in one composition, asserts both exact outgoing pubkeys, then deletes each occurrence in turn and asserts the exact survivor. Cover draft/edit restoration if it rehydrates through the same name-keyed representation.
Verification owner: author for the fix and regression; reviewer will rerun the adversarial composer/serialization path at the next immutable head.
Other reviewed behavior: selected-people pagination now prefetches for non-overflowing and heavily filtered pages; duplicate access rows retain exact pubkeys and expose NIP-05/truncated-pubkey labels with full-key accessible names. No remaining author-actionable pagination, keyboard, accessibility, or label defect was found in the reviewed paths.
Validation: pnpm build:e2e passed at matching HEAD; the exact-head temporary Playwright recipient probe failed 1/1 with the missing-first-pubkey diff; the five relevant shipped same-name/selected-people tests passed; GitHub's exact-head check rollup is green, including Desktop Core, four Desktop Smoke E2E shards, Desktop E2E Integration, macOS Desktop Build, Security, and DCO.
Manual/native evidence: no native run. The blocker is renderer-contained and was reproduced through the real composer-to-signed-event mock bridge.
Residual risk: draft/edit mention restoration was not dynamically exercised and may share the same name-keyed collision.
— :bot: Jude’s code review agent
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
CHANGES REQUESTED. GitHub forbids requesting changes on Wes’s own PR, so this blocking verdict is published as a comment.
Reviewed exact head 1fa8916992c8164ea028cb128aa24f5b4f4fa9cc. Two blocking identity/state defects remain:
-
Two same-name recipients still collapse to one identity in a single composition.
desktop/src/features/messages/lib/useMentions.ts:84,607-616stores selected mentions asMap<displayName, pubkey>, so selecting a second distinctWilloverwrites the first.extractMentionPubkeys.ts:52-54,69-84receives only the surviving map entry and can emit only that pubkey even though the composer shows two@Willtokens. The new E2E atdesktop/tests/e2e/mentions.spec.ts:690-735selects either duplicate in separate compositions, never both together. Preserve identity per token/occurrence (or make inserted text uniquely distinguishable), then cover select-both, exact outgoing pubkeys, delete-either, draft restoration, and edit. -
Selected-people pagination can dead-end when the allowlist changes under an active query. The completeness effect at
desktop/src/features/agents/ui/RespondToField.tsx:149-165checks whether the filtered viewport is non-scrollable, but its dependencies omitsearchResults/allowlist. With a scrollable first page andhasNextPage === true, adding enough matching pubkeys through the adjacent paste flow (RespondToField.tsx:189-192) filters the active result set down without changing the query. The effect does not rerun for that content transition, and once the list no longer overflows the scroll-only fetch trigger cannot fire. The added coverage atdesktop/tests/e2e/agent-access-warning.spec.ts:221-269starts already filtered and does not exercise this lifecycle. Re-run the completeness check when filtered viewport content changes and cover an active query crossing from scrollable to non-scrollable while a valid later page remains.
The access picker’s pubkey labels/accessibility and initially filtered/non-scrollable pagination paths are otherwise resolved. GitHub’s exact-head rollup is green, but it does not cover either lifecycle above.
Track mention identity by plain-text occurrence through compose, draft, edit, and send lifecycles. Fail closed when editor transactions cannot be projected safely, and refill filtered selected-people search pages when the visible result set changes. Co-authored-by: Carl <9d00794d3df50972eb8b615511783cab12a77a8fd5dd5edd58073ec73b54bd8b@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…r-search-completeness Co-authored-by: Carl <9d00794d3df50972eb8b615511783cab12a77a8fd5dd5edd58073ec73b54bd8b@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Implemented at
Validation at |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Paul here, reviewing via Will's account, with Thufir's independent source review folded in.
Thufir traced the identity lifecycle end to end (compose, draft restore, edit reopen/resubmit, delayed and failed sends, edited-message overlays, agent revalidation, and the selected-people pagination) and came back with no findings; he also independently verified typecheck and the full desktop JS suite (5,318/5,318) at cf9f306. The duplicate-name collapse and the allowlist pagination dead-end both look genuinely fixed, and the occurrence-rebase design reads well.
One correctness issue did turn up in my pass — left inline on MessageComposer.tsx. It's the same silent-mention-loss class this PR is fixing, so I'd rather see it handled before merge than approve around it.
Separate note: the branch currently reports conflicting with main, so it needs a merge regardless.
| await mentionSendFlow.sendMessageWithMentionFlow({ | ||
| capturedChannelId: channelId, | ||
| capturedThreadContext, | ||
| mentionText: plainTextRef.current.trim(), |
There was a problem hiding this comment.
🤖 plainTextRef.current.trim() breaks the occurrence-offset coordinate space whenever the live projection has whitespace at both ends, and the result is a silently dropped mention.
extractMentionPubkeys(mentionText) first runs reconcileMentionIdentities(mentionText) while mentionTextRef.current still holds the untrimmed projection, with no supplied textChange, so inferSingleTextChange kicks in. A leading-only or trailing-only trim infers a clean deletion and the refs survive, but when both ends get trimmed the common prefix and suffix are both zero, so the inferred change is “the entire text was replaced” — every occurrence-bound identity intersects it and gets dropped. And because those display names are in occurrenceManagedNamesRef, the name-based fallback in extractMentionPubkeysForCurrentMentions is filtered out too, so the message sends with no p tag at all — for that state it's a regression vs the pre-PR name-map path.
Repro against this branch's module:
const ref = { displayName: "Will", pubkey: "1".repeat(64), isAgent: false, offset: 1 };
reconcileMentionIdentityRefs([ref], " @Will hi ", "@Will hi"); // => [] (identity dropped)
reconcileMentionIdentityRefs([ref], " @Will hi", "@Will hi"); // => [{ ...ref, offset: 0 }] (leading-only trim is fine)@Will hi is reachable: an accidental leading space or newline plus the trailing space the autocomplete insert always appends. The same trim feeds the edit path above and ForumComposer, and since reconcileMentionIdentities mutates the ref state, the subsequent getDraftMentionRefs(mentionText) snapshot loses the refs too — so a draft restored after a cancelled/failed send comes back without them.
I think the fix is to stop trimming mentionText at all three call sites (here, the edit submit above, and ForumComposer) — the offset coordinate space should just be the live projection. The trimmed string is only needed for the outgoing content, which is built separately; everything mentionText feeds (extraction, the DM-thread guard, draft snapshots) works fine untrimmed. Worth a regression case covering the both-ends-trim shape alongside the existing mentionIdentityRefs tests.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
CHANGES REQUESTED. GitHub may forbid a changes-requested state on Wes’s own PR; if so, this remains an unmistakably blocking comment.
P1: trimming the mention projection can silently remove the selected recipient from send, edit, and forum events. Occurrence refs use offsets in the live untrimmed plain-text coordinate space, but MessageComposer passes plainTextRef.current.trim() into both edit submit and normal send (desktop/src/features/messages/ui/MessageComposer.tsx:534,602), and ForumComposer does the same (desktop/src/features/forum/ui/ForumComposer.tsx:242-244). Extraction then reconciles that trimmed string against the stored untrimmed projection (desktop/src/features/messages/lib/useMentions.ts:860-899). With whitespace on both ends, such as @Will hi, the inferred single replacement has no common prefix or suffix, intersects the occurrence, and drops its identity (mentionIdentityRefs.ts:34-65,97-144). Name fallback is intentionally suppressed for occurrence-managed names, so the outgoing event omits the intended p tag. The mutation occurs before recovery snapshots too, so failed/cancelled send or failed edit can restore without the exact identity.
Keep mention-coordinate input untrimmed at all three call sites while trimming outgoing content separately. Add regressions for both-end whitespace through primary send, edit failure/retry, forum send, and draft/cancelled-send recovery. The prior duplicate-name collapse and active-query pagination blockers are fixed at this head.
Review was read-only against exact head cf9f3064293adc10e0cba0699e43e6070af0a838; I did not check out or execute PR code. GitHub’s only current exact-head status is DCO green; the review-maintenance workflow failed independently.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated multi-lane review at head cf9f306. Two independent code-review lanes plus a compiled-behavior E2E verification lane; verdict: changes requested — one IMPORTANT correctness finding, independently reproduced by both code lanes.
IMPORTANT — send-time trim silently drops mention recipients
All three submission paths pass a trimmed plain-text projection into mention extraction, but occurrence identity offsets were recorded against the untrimmed editor text:
- normal send:
desktop/src/features/messages/ui/MessageComposer.tsx(mentionText: plainTextRef.current.trim()) - edit: same file, edit submit options
- forum send:
desktop/src/features/forum/ui/ForumComposer.tsx(getPlainTextAndCursor().text.trim())
reconcileMentionIdentityRefs() infers a single text change between the stored projection and the supplied text (mentionIdentityRefs.ts). With BOTH leading and trailing whitespace present — trailing is near-universal since autocomplete inserts "@Name " — the inferred change spans the entire string (prefix=0, suffix=0). Every occurrence ref then intersects the replacement range and is dropped: send-time allowedOverlappingIds is empty, and isMentionAt checks the stale offset against the trimmed text. The display-name fallback cannot recover because extractMentionPubkeysForCurrentMentions deliberately suppresses occurrence-managed names from both selectedMentions and memberCandidates.
Net effect: a message like " @Alice hi " sends with no p tag for Alice — silent notification loss on ordinary sends, edits, and forum posts, affecting unique names as well as duplicates. Both code lanes reproduced the arithmetic independently (one via re-implementation of inferSingleTextChange + the rebase branches: leading-only trim survives, leading+trailing drops the ref).
Suggested fix: keep the text supplied to identity reconciliation untrimmed at all three call sites and trim outgoing content only after occurrence identities are resolved (or supply the trim explicitly as MentionTextChanges). Add regression coverage with leading+trailing whitespace across normal send, edit retry, forum send, and draft recovery.
MINOR
registerMentionPubkey dedupes an existing identity by (offset, name, pubkey), so a differing-pubkey ref at the same offset accumulates alongside the old one. Benign under current callers (restore clears first) — flagged for tidiness.
What holds up
- Root cause correctly fixed: name+secondary-label coalescing (
globalSearchIdentityKey) removed in favor of pubkey identity; same-name candidates survive autocomplete. - Occurrence-offset model is well contained in
mentionIdentityRefs.tswith fail-closednull-change semantics; ProseMirror StepMap projection correctly refuses multi-step transactions. RespondToFieldselected-people search now paginates (50/page, scroll + dead-end autofetch), matching add-member completeness; rows expose pubkey identity.- Draft schema extends
offsetas optional with validation and equality updated; edit path diffs added mentions against the edit target's own refs. - Live E2E lane (compiled behavior, headless, mock bridge): duplicate-name identities survive draft switch + reload and send with both exact
ptags in selection order; mutation control confirmed the probe detects regression.
Operational note
GitHub reports this branch as conflicting with current main — needs a rebase independent of the finding above.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 4e3c9e619c93dd26677b392ad1f8cf0d12c8f855..cf9f3064293adc10e0cba0699e43e6070af0a838 (exact head cf9f3064293adc10e0cba0699e43e6070af0a838)
Risk: high — this changes recipient identity tracking across composer state, edit/draft restoration, outgoing Nostr p tags, and access-control selection.
Blocking finding
-
[P2] Keep mention identity offsets in the untrimmed text coordinate space —
desktop/src/features/messages/ui/MessageComposer.tsx:602passesplainTextRef.current.trim()asmentionTexton send, and line 534 does the same for edits. The occurrence refs were recorded against the untrimmed projection. When whitespace exists at both ends (reachable via a leading space/newline plus autocomplete's trailing space), reconciliation infers replacement of the whole string and drops all intersecting refs. Because those names are occurrence-managed, the legacy name fallback is excluded too. The message can therefore retain visible duplicate-name mentions while emitting no recipientptags; failed-send recovery snapshots and edit submit inherit the same mismatch.Exact-head causal E2E probe: composing
" @Will @Will hello both "while expecting the normalized outgoing message to preserve both selected pubkeys failed 1/1: expected[111…111, 222…222], actual[]. The probe was reverted and the tree rechecked clean. This independently reproduces the unresolved defect documented at #6483 (comment).
Author action: pass the live untrimmed plain-text projection as mentionText for normal send and edit, while trimming only the separate outgoing content. Add a causal regression with whitespace at both ends that asserts exact recipients survive normal send and edit/recovery.
Verification owner: author for the fix and regression coverage; reviewer for rerunning the composer→recipient and restoration probes on the next immutable head.
Cleared behavior at this head
The original duplicate-display-name collision is fixed in the reviewed paths. Exact-head tests demonstrated independent selection of both identities, outgoing tags with both exact pubkeys, deletion survivor behavior, duplicate-name edit restoration, independently selectable access rows, and filtered/non-overflow pagination with active-page refill. Source review also covered offset rebasing, stale restoration failing closed, tag/occurrence-order edit reconstruction, and stale identity removal. Same-name accessible labels are disambiguated while preserving visible display names and existing keyboard interaction.
Validation at matching clean head:
- Desktop full package suite: 5318/5318 passed.
pnpm typecheck: passed.pnpm check: passed.- E2E app build: passed.
- Targeted exact-head Playwright product/UI suite: 8/8 passed.
- Core identity/edit/access E2E: 3/3 passed; pagination-refill: 1/1 passed.
- Mutation proof replacing occurrence recipient extraction with
[]: causal composer E2E failed 1/1, then source was restored clean. - Final preflight: local/live head
cf9f3064293adc10e0cba0699e43e6070af0a838, requested merge-base matched, working tree clean, DCO passed.
Integration state: GitHub currently reports CONFLICTING / DIRTY; Mark Previous Review Stale is red. Those states also prevent merge, but they are separate from the author-actionable recipient defect above.
Manual/native evidence: browser E2E only; no physical assistive-technology or native Tauri/WebView observation.
Residual risk: native WebView and physical AT behavior remain unobserved. That is a release/accessibility verification gap, not additional author rework by itself.
🤖 ## Summary This PR finishes the change on the remaining mobile screens. After it, every place that names a person or agent uses a contextual name: - The channel list, DM labels, and the new-DM picker - The channel header and details, and the members and add-members sheets - Huddle avatars, spotlight, and overlay - The Activity inbox - Search results for people and messages - Pulse notes, agent cards, and reply previews Screens that belong to a channel compare names against its members. Search, Pulse, and the new-DM picker have no channel, so they compare the identities shown together. In the example below, Search showed four results all named "murderbot". Now the human keeps the plain name, and baxen's three agents are labeled by owner and key suffix. Part 3 of 3: #7894 (resolver) → #7895 (conversations) → #7896 (lists, Search, Pulse). ### Related issue Related to #2910. The desktop search change in #6483 addresses the same problem in a different app. ### Testing - Widget tests cover the Activity inbox, Search, and Pulse notes with colliding names. - `flutter analyze` is clean, and the full `flutter test` suite passes. - Checked on an Android emulator against the live relay by searching for "murderbot". The after image also shows "classy-murderbot", a new account that was created between the two captures. | Before | After | | --- | --- | | <img width="300" alt="Before: Search shows four people all named murderbot" src="https://github.com/user-attachments/assets/21bbcc52-fea8-4b31-ab8c-ff3ec9b32a39" /> | <img width="300" alt="After: three results show as baxen's murderbot with a key suffix, and the human shows as murderbot" src="https://github.com/user-attachments/assets/d59584d9-4a35-46da-b39a-83addfad3cd2" /> | --------- Signed-off-by: Logan Johnson <loganj@squareup.com> Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>


Summary
Testing
cd desktop && pnpm build:e2ecd desktop && pnpm exec playwright test tests/e2e/mentions.spec.ts tests/e2e/agent-access-warning.spec.ts --project=smoke --grep "same-name"