fix(desktop): don't render a mention inside a word - #5948
Chessing234 wants to merge 5 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
left a comment
There was a problem hiding this comment.
I found one rendered/composer alignment bug and two reviewability regressions that should be fixed before merge.
- Rendered channel links become more permissive than the composer. The shared leading boundary includes
(for both prefixes, so(#general)becomes a channel link in rendered messages. The composer channel highlighter—the source of truth cited by this PR—only accepts start-of-text or whitespace. Parentheses are intentional for mentions because team expansions use them, but channels need the stricter rule. - The refactor introduces two Biome diagnostics. Formatting detaches both
biome-ignore noExplicitAnycomments from thenode: anyparameters, leaving unused suppressions plus twonoExplicitAnyerrors. The existing structuralNodetype works for both helpers and removes the suppressions. - The boundary-preservation test does not exercise the plugin. It manually strips capture group 1, so a regression in
createRemarkPrefixPlugincould still discard whitespace while the test passes. A real remark-tree assertion closes that gap.
I implemented all three corrections in b54de7ad1: mentions retain the opening-parenthesis rule, channels match the composer exactly, the helpers use Node, and the tests exercise the actual remark transformation.
Verification on the fix commit:
- focused mention/composer tests: 32 passed
pnpm typecheck: passedpnpm check: passed with only the two unrelated pre-existing warnings and two infospnpm build: passed
The broader stacked desktop suite was also exercised while reviewing #5949: 4,973 tests passed with this PR's original behavior included.
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>
|
All three were right; each is now its own commit. 1. Channel boundary. Confirmed against 2. Biome diagnostics. Reproduced before fixing — 3. The test. Agreed the helper was asserting against a re-implementation of the plugin's lead-stripping rather than the plugin. The pattern-level tests stay — they're about the regex — but the tree behaviour has its own coverage now. Verification:
|
Found by reading the two mention matchers side by side — no issue filed.
The composer's highlighter (
mentionHighlightExtension.ts) only opens a mention at the start of the input or after whitespace or(. The rendered message (mentionPattern.ts→remarkMentions) has no such rule: any@followed by a p-tagged name becomes a mention chip wherever it sits, including inside a word.So this message:
renders the
@aliceinside the address as a second mention of her — highlighted, styled, and clickable. It's a rendering-only artifact: nothing was tagged for that occurrence, and it looked different while you were typing it. The#channelmatcher shares the same builder and has the same gap (issue-42#general).This makes the rendered path agree with the composer.
Not a lookbehind. #5547 and #3295 established that WebKit before Safari 16.4 fails to parse a lookbehind, which blanks the whole app rather than degrading one pattern. The boundary is therefore a plain capture group, and
createRemarkPrefixPlugingained an opt-inleadGroupindex (first commit) so it re-emits that character as text instead of swallowing it into the chip. Callers that don't opt in — the entity, message and channel deep-link patterns, which have capture groups of their own — are unchanged.Behaviour that deliberately stays as it is:
@alice @bob) both match; the boundary character is not consumed by the preceding match@aliceisn't shadowed by a knownaliVerified locally at this head:
pnpm test— 4962 passed, 0 failed (4954 before, plus the 8 new)pnpm check— clean; the 4 warnings and 2 infos it reports are pre-existing and identical onmain(checked by stashing)pnpm build— succeededNote: I'm an outside contributor, so the workflow runs on this PR will sit at
action_requireduntil a maintainer approves them; only the DCO check reports on its own.