fix(desktop): tag and render a mention picked after a bracket - #5957
Open
Chessing234 wants to merge 7 commits into
Open
Chessing234 wants to merge 7 commits into
Chessing234 wants to merge 7 commits into
Conversation
…ndary `createRemarkPrefixPlugin` treats the whole match as the node's text, so a pattern that needs to assert what precedes it has nowhere to put that character. Lookbehind is not an option here: WebKit before Safari 16.4 fails to *parse* a lookbehind, which blanks the entire app rather than degrading one pattern (block#5547). Add an opt-in `leadGroup` capture index. When set, that group's text is emitted back as plain text and the node is built from the rest of the match. Callers that don't pass it — the entity, message and channel deep-link patterns, which have capture groups of their own — are untouched. No behaviour change on its own. Signed-off-by: Taksh <takshkothari09@gmail.com>
The composer highlights `@name` only at the start of the input or after whitespace or `(`. The rendered message applied no such rule: any occurrence of a p-tagged name after `@` became a mention chip, wherever it sat. So a message that mentions @alice and also gives out an address — @alice can you mail me at bob@alice.dev? renders `@alice` inside the address as a second mention of her. The two halves of the same feature disagreed about what a mention is, and the rendered half was the wrong one: nothing was ever tagged for that occurrence. Require the same leading boundary in the rendered path, using the capture group added in the previous commit rather than a lookbehind. The channel pattern gets the same treatment for the same reason — it shares the builder, and `issue-42#general` is no more a channel link than the address is a mention. Signed-off-by: Taksh <takshkothari09@gmail.com>
`getMentionOffsets` accepted a mention only when it was followed by whitespace or closing punctuation. An apostrophe was in neither set, so `@Alice's PR is ready` found no mention of Alice at all. That is not just a highlighting miss. The send path derives the event's `p` tags from these same offsets (`extractMentionPubkeys` → `useMentions.extractMentionPubkeysForCurrentMentions`), so picking Alice from the autocomplete and then typing `'s` silently dropped her tag: no `p` tag on the wire, no notification, no agent wake-up — with the mention still sitting there in the message the sender wrote. Accept both the straight `'` and the curly U+2019 that macOS substitutes as you type, and give the rendered pattern the same set so a tagged mention is still styled as one. Signed-off-by: Taksh <takshkothari09@gmail.com>
`hasMention.ts` says it is "exported separately so it can be unit-tested without importing React" and then had no test file — for the function that decides who gets a `p` tag and therefore who gets notified. Pin the whole rule, not just the possessive: start-of-text, whitespace and `(` open a mention; whitespace, closing punctuation, spoiler delimiters and an apostrophe close it; an email address does not mention its domain; code spans and fenced blocks are not mentions; matching is case-insensitive; and every occurrence gets its own offset. Signed-off-by: Taksh <takshkothari09@gmail.com>
`detectPrefixQuery` opens an autocomplete after whitespace, `(`, `[` or `{` —
brackets were added deliberately so `(#channel` and `[@name` behave like
` @name`. `getMentionOffsets` accepted only `(`, so a name picked from the
autocomplete after `[` or `{` produced no `p` tag at all: the composer
offered Alice, you chose her, and the event went out mentioning nobody.
Accept the same bracket set the autocomplete does.
Signed-off-by: Taksh <takshkothari09@gmail.com>
Same boundary, the render side. With `getMentionOffsets` now tagging `[@alice`, the rendered message still had to agree or the message would carry a `p` tag for a name shown as plain text. `detectPrefixQuery` is the source of truth for what opens a mention; all three paths — autocomplete, tagging, rendering — now use the same set. Signed-off-by: Taksh <takshkothari09@gmail.com>
themiguelamador
suggested changes
Aug 16, 2026
themiguelamador
left a comment
There was a problem hiding this comment.
The mention-boundary behavior itself looks correct: autocomplete, p-tag extraction, and remark rendering agree for start/whitespace/open-bracket boundaries; possessives work; glued-to-word cases remain excluded; and the shared splitter preserves consumed boundaries across adjacent matches.
One minor PR-local lint issue remains: moving walkChildren to a multiline declaration detached the biome-ignore from its node: any parameter. pnpm check therefore reports both a new noExplicitAny warning and an unused-suppression warning. The suppression should sit immediately above the parameter.
I prepared that one-line fix in Complear/buzz:review/pr-5957-fix, commit 3883828eac9a0f8009f5322389f479aff4a8efc4.
Verification after the fix:
- focused mention/autocomplete contracts: 38 passed
- direct remark-tree boundary/adjacency probes: passed
- complete Desktop unit suite: passed
pnpm check: passed with only the 2 pre-existing warnings and 2 infos outside this PRpnpm typecheck: passedpnpm build: passedgit diff --check: passed
Review feedback: moving `walkChildren` to a multiline declaration left the `biome-ignore` above the function instead of above the `node: any` it suppresses, so `pnpm check` reported both a new `noExplicitAny` warning and an unused-suppression warning. Put it back on the parameter. Signed-off-by: Taksh <takshkothari09@gmail.com>
Contributor
Author
|
pushed 6d24654 — the
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by probing the three mention paths against each other; no issue filed.
detectPrefixQueryopens an autocomplete query after whitespace,(,[or{. The brackets are there on purpose — its comment says they were added because "a prefix glued to a(never fired". But the other two paths never learned about[and{:ptag@Alice(@Alice[@Alice{@AliceSo the composer offers Alice, you pick her, and the message goes out mentioning nobody — no
ptag, no notification, no agent wake-up, and the name rendered as plain text. Silent, and the composer actively led you into it.Both paths now accept the same bracket set
detectPrefixQuerydoes, in that order: tagging first, then rendering, so neither commit leaves the tree in a state where a tagged mention renders unstyled.x@Alicestill opens nothing, and a test pins that.Stacked on #5949 (which is stacked on #5948) — the same two boundary definitions. This finishes the set: #5948 gave the render path a leading boundary at all, #5949 fixed the possessive on both sides, this one aligns the opener with what the autocomplete already accepts. Happy to squash the three or rebase them differently if you'd rather.
Verified locally at this head:
pnpm test— 4976 passed, 0 failed (4973 on the parent branch, plus the 3 new)pnpm check— clean; its 4 warnings and 2 infos are pre-existing and identical onmainpnpm build— succeededNote: I'm an outside contributor, so the workflow runs here sit at
action_requireduntil a maintainer approves them; only the DCO check reports on its own.