Show selected mentions as inline identity chips - #147
Conversation
Use shared inert chips in the composer and distinguish selected namesakes without changing authored text or notification identity. Preserve draft editing and add clipboard, motion, and responsive coverage. Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Signed-off-by: morgmart <98432065+morgmart@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 head 9d7c63c84ee81c9d8f443eb9a9b9ba7536d54ea5 against base c5762f47830cac9b29a6bd5eaf3d28e02312a209.
Changes requested
P2: Selected mentions inside Markdown literals lose all notification disclosure
Removing RecipientAvatars makes the inline decoration the only visible record of an explicit recipient. However, RichComposerInput still suppresses that decoration when it overlaps an inline-code/code/image/HTML literal (src/features/messages/RichComposerInput.tsx:91-137). Inserting a selected mention in such a range is still accepted, retained in the draft, and sent as an exact recipient.
Verified reproduction with the actual MessageComposer, MentionPicker, and createMessages implementations in both Chromium and WebKit:
- Enter a backtick, a space, and a closing backtick; place the caret immediately after the opening backtick.
- Open Mention a member and select Honey.
- The draft becomes
`@Honey `. There is no inline chip and no Notification recipients disclosure. - Send. The emitted kind-9 event still contains
["p", "aaaaaaaa…"]for the selected identity.
The same focused comparison against the base retains the separate recipient disclosure for this draft. This is therefore a disclosure regression introduced by its removal, not a new claim that the underlying Markdown suppression is itself new. Namesakes are particularly problematic: the composer can no longer show which exact identity will be notified.
Required outcome: every retained explicit recipient remains visibly identifiable before send, including recipients whose normal decoration is suppressed. Preserve the inline-only design if desired, but reconcile the literal/decoration filter with explicit recipient intent rather than allowing invisible recipients. Add regression coverage for insertion inside a literal, restoration of that draft, and the resulting channel/reply recipient keys. Do not silently change existing notification semantics merely to hide the inconsistency.
Other reviewed lanes found no additional material blocker: scoped identity presentation, suffix collision handling, shared chip accessibility/motion, editor source serialization, and the migrated tests. Hosted CI is green. Focused probes used synthetic session/outbox data and existing dependencies; no live notifications or native packaged-app validation, and no broad CI-equivalent suites rerun.
Give explicit recipients precedence over inferred decorations without changing source or notification semantics. Cover literal and depth fallback disclosure, restored drafts, copy, and exact channel/reply publication. Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
|
Carl, an automated reviewer, commenting via Morgan’s GitHub account. Fixed in dfc5a1f. Explicit selected recipients now take precedence over inferred decorations and remain visible inside Markdown literals or parser-depth fallback. Ordinary typed mentions still do not become notification recipients. No source, picker, or recipient-publication semantics changed. Nine unit regressions failed on the prior code and pass with the fix: inline/fenced/indented code, images, image references, definitions, HTML, link labels, and deep Markdown. They check both namesake identities before/after draft restoration, unchanged source, plain typed mentions, and exact recipients. At the new commit, the complete unit suite passed (2,148 tests, four workers). Both full affected browser files passed in Chromium and WebKit (6 cases), including real picker insertion inside code, restored channel/reply drafts, exact copy text, and published channel/reply recipient keys. Mandatory hooks passed. Independent scoped review found no remaining blocker. New-head CI and reviewer recheck remain pending. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: prior blocker resolved; no remaining material findings
Reviewed head dfc5a1f3f3a12b8249acfed6a0bfe95c6eb267a3 against base c5762f47830cac9b29a6bd5eaf3d28e02312a209, concentrating on the three-file follow-up since reviewed head 9d7c63c84ee81c9d8f443eb9a9b9ba7536d54ea5.
The previous P2 recipient-disclosure blocker is resolved. RichComposerInput now gives explicit recipient spans precedence over inferred decorations and does not suppress them inside Markdown literals or the depth fallback. It leaves authored text, recipient metadata, persistence and send routing under their existing owners. Typed names still do not acquire notification intent.
Validation:
- Traced decoration precedence through the editor's source-backed tokens, copied text, draft restoration and channel/reply submission. Reviewed the nine literal/depth unit regressions; an independent coverage audit found no material gap.
- On the clean exact head, ran both complete affected browser files,
mentions.spec.mjsandcomposer-links.spec.mjs, in Chromium and WebKit: 6/6 passed. The new real-picker journey verifies both visible namesake chips inside code, channel/reply draft restoration, exact copied/sent source without visual qualifiers, exact selectedpkeys, channelh, and reply-onlyerouting. Existing link-editing and recipient journeys passed alongside it. - Hosted automatic checks are green at this head, including all four browser shards, JavaScript, Rust/tool integration and
CI required; DCO and security checks are green. Full unit/broad validation relies on hosted CI, not a duplicate local run.
No additional material defect found in this bounded re-review. Synthetic browser fixtures only: no live relay notifications, native packaged-app, physical IME or screen-reader acceptance. This is a COMMENT review, not approval or permission to merge.
What this does
Selected people and agents now appear as shared inline chips directly in the composer, replacing the separate recipient-avatar row. When two selected identities share a name, both chips show a short public-key ending, such as
@Honey · npub…4hu, so the person writing can tell them apart.The earlier chip gently reveals its qualifier when ambiguity first appears. Reduced motion keeps the information without animation. The playground includes two Alice people and two Honey agents to try it.
Why it matters
The mention and its notification intent are visible in one place. Deleting or editing the mention removes that intent through the existing editor behavior, rather than a separate recipient control.
The qualifier is display-only: copied text, saved drafts, source positions, and published recipient keys stay unchanged. Chips inside the editor have no extra buttons, focus stops, hover previews, or click actions.
How it works
The composer supplies its own identity labels to the existing design-system chip instead of consulting a global identity registry. A shared public-key formatter lengthens suffixes when necessary to distinguish a namesake group. The composer owns ambiguity and reveal history; the shared chip owns presentation and motion tokens.
This builds on #119 and preserves #137’s exact-recipient, remembered-agent, channel/reply, and restored-draft behavior. It does not redesign the picker or prevent selecting the same agent twice; duplicate prevention remains BOT-2033. Coordinate with #142 before merging both: its recipient presentation overlaps this replacement, while its agent-directory changes are intentionally outside this PR.
Visual evidence
Synthetic playground, one selected agent:
Adding a different Honey qualifies both:
Narrow layout, dark mode:
Verification
At
9d7c63c84ee81c9d8f443eb9a9b9ba7536d54ea5, after integrating current main:bin/pnpm exec vitest run --maxWorkers=4.Browser coverage adds one playground case per engine and extends the existing namesake journey. Existing recipient-control assertions now exercise inline chips and authored-span deletion while retaining exact identity assertions. Browser coverage is necessary for native selection/copy, noneditable token geometry, wrapping, and animation; group/collision and lifecycle matrices remain in unit tests.
Earlier validation on the pre-rebase tree: seven changed browser files passed 63/64, with an intermittent WebKit media-review handoff failure; the entire affected file then passed 8/8 on rerun. The default-worker unit run hit two timeouts in page activation and read-state growth; both files passed unchanged together, and the complete four-worker run passed. These results do not establish that the original failures were pre-existing or fixed.
CI and native desktop validation remain pending/not performed for this frontend-only change. Non-blocking review follow-ups: synthetic composition-event remounts can replay a reveal (not reproduced through realistic IME cancellation), and qualifier changes have correct accessible names but no separate live announcement. Neither expands the approved behavior in this PR.
Review follow-up: visible recipients inside Markdown
At
dfc5a1f3f3a12b8249acfed6a0bfe95c6eb267a3, selected mentions stay visibly identifiable inside code, images, HTML, link labels, and Markdown depth fallback. Explicit recipient chips take precedence over inferred decoration; typed prose remains non-notifying.Nine new unit regressions failed before the fix. The complete unit suite now passes 2,148 tests with four workers; both affected browser files pass 6/6 across Chromium/WebKit. One added journey per engine verifies real picker insertion into code, restored channel/reply drafts, copy text, and exact publication keys. Mandatory hooks passed; new-head CI and reviewer recheck are pending. Earlier results above remain historical, not new-head claims.