fix(desktop): treat a possessive apostrophe as the end of a mention - #5949
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>
themiguelamador
suggested changes
Aug 16, 2026
themiguelamador
left a comment
There was a problem hiding this comment.
The possessive behavior itself is correct: straight and curly apostrophes preserve the p tag, rendered mentions use the same trailing boundary, code masking remains intact, and longest-name selection still prevents a prefix member from being double-tagged.
One minor issue remains in the stacked diff. Formatting walkChildren across multiple lines detached its biome-ignore from the node: any parameter. That creates both a new noExplicitAny warning and an unused-suppression warning in pnpm check. I pushed the cleanup in f43d08c18: use the file's existing structural Node type for the two internal walkers and remove the now-unnecessary suppressions. This should also be applied to/rebased from #5948, where the warning originates.
Verification at the fixed head:
pnpm test— 4,973 passed, 0 failed- focused mention, extraction, highlighter, and rendering tests — 74 passed
pnpm check— pass; only 2 unrelated pre-existing warnings and 2 infos remainpnpm typecheck— passpnpm build— pass
Review: the leading boundary was shared between both prefixes and included
`(`, so `(#general)` became a channel link in a rendered message — more
permissive than the composer's highlighter, which this change set is aligning
the rendered path to.
The composer draws the line in two different places on purpose:
mentions (?:^|(?<=[\s(]))@ — team expansions render as `Team (@ana @bo)`
channels (?:^|(?<=\s))# — channels have no such form
`buildPrefixPattern` now takes `allowOpeningParen`, and only
`buildMentionPattern` opts in. It defaults to off so the stricter rule is what
a new caller gets. Without this, `(#general)` shows no chip while typing and
turns into a link once sent.
- focused `mentionPattern.test.mjs` — 9 passed
Signed-off-by: Taksh <takshkothari09@gmail.com>
Review: reformatting `walkChildren` onto multiple lines detached its `biome-ignore` from the `node: any` parameter it was covering, which cost two diagnostics — a `noExplicitAny` warning on the parameter and an `suppressions/unused` warning on the orphaned comment. `pnpm check` reported 4 warnings on this branch against 2 on main. Both internal walkers take the structural `Node` type the file already defines for built nodes, so no suppression is needed at all. - `pnpm check` — back to 2 warnings and 2 infos, all pre-existing - `pnpm typecheck` — pass Signed-off-by: Taksh <takshkothari09@gmail.com>
Review: the boundary-preservation test asserted through a helper that strips capture group 1 by hand — a re-implementation of what the plugin does, not the plugin. `createRemarkPrefixPlugin` could have swallowed the boundary character instead of emitting it back as text and the test would still have passed. These run both real plugins over a real mdast tree and assert on node types and values, so a dropped space fails the assertion. The pattern-level tests stay where they are — they are about the regex — but the tree behaviour now has its own coverage, including the composer-parity case that `(#general)` stays plain text. - focused `createRemarkPrefixPlugin.test.mjs` — 9 passed 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>
Chessing234
force-pushed
the
fix/mention-possessive
branch
from
August 16, 2026 12:58
363a50b to
ea76a48
Compare
Contributor
Author
|
Fixed by rebasing onto the updated #5948, which is where you correctly said the warning originates — the
Thanks for confirming the possessive behaviour itself — that part is unchanged. Verification at this head:
|
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 mention matcher; no issue filed.
@Alice's PR is readymentions nobody.getMentionOffsetsaccepts a mention only when the name is followed by whitespace or closing punctuation. An apostrophe is in neither set, so the match fails — and that is not just a missed highlight. The send path derives the event'sptags from these same offsets (extractMentionPubkeys→useMentions.extractMentionPubkeysForCurrentMentions), so you can pick Alice out of the autocomplete, type's, and her tag silently disappears: noptag on the wire, no notification, no agent wake-up, with the mention still sitting in the message you sent. The possessive is about the most common way there is to write about a person.This accepts both the straight
'and the curly U+2019 that macOS substitutes as you type, and gives the rendered pattern the same set so a tagged mention is still styled as one.The third commit adds the test file
hasMention.tshas been asking for — its own header says it is "exported separately so it can be unit-tested without importing React", and there was no test. It pins the whole rule, not just the new character: what opens a mention, what closes it, thatbob@Alice.devdoesn't mention Alice, that code spans and fenced blocks aren't mentions, and that matching is case-insensitive.Stacked on #5948 (the leading-boundary fix), which touches the same
boundaryline inmentionPattern.ts. Please merge that one first; this branch contains its two commits. Happy to rebase ontomaininstead if you'd rather take them in the other order or separately.Verified locally at this head:
pnpm test— 4973 passed, 0 failedpnpm 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.