fix(markdown-toolbar): fixed markdown formatting toolbar toggle behaviour and duplicate marker handling#39479
Open
Subhooo5 wants to merge 2 commits intoRocketChat:developfrom
Open
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 46938d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughFixes markdown formatting toolbar toggle behavior by tracking active formatting modes, updating formatting patterns for bold, italic, strikethrough, and code blocks, enhancing text wrapping logic to detect and handle already-wrapped content, and implementing toggle state visualization in the toolbar component. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx (1)
40-59:⚠️ Potential issue | 🟠 MajorCollapsed actions still keep the old one-shot behavior on small screens.
Only
featuredFormattergoes throughtoggleMode().apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/FormattingToolbarDropdown.tsx:14-35still callscomposer.wrapSelection()directly, so the dropdown items do not toggle and never updateactiveModes. That leaves Italic/Strikethrough/Code inconsistent in the small variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx` around lines 40 - 59, The small-variant dropdown items still call composer.wrapSelection() directly (in FormattingToolbarDropdown) so they perform one-shot formatting instead of toggling and updating activeModes; modify FormattingToolbarDropdown to accept and use the same handlers used for featuredFormatter — pass toggleMode and isPromptButton (or a single onItemClick handler) from MessageBoxFormattingToolbar into FormattingToolbarDropdown and, inside FormattingToolbarDropdown, invoke isPromptButton(item) ? item.prompt(composer) : toggleMode(item.pattern, item.label) rather than composer.wrapSelection(); this ensures dropdown items toggle modes and keep activeModes consistent with featuredFormatter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts`:
- Around line 241-287: The current logic using lastIndexOf(startPattern) and
before.endsWith(startPattern) treats symmetric delimiters (startPattern equal to
endPattern like "**", "~~", "`") as interchangeable opener/closer and can pick a
closing marker; replace this with pair-aware matching: implement a helper
(referenced from this block using startPattern, endPattern, left/right, before,
after) that scans backward through before and forward through after counting
delimiter occurrences or tokenizing so you only consider an opener if it pairs
correctly (e.g., for symmetric markers ensure an odd/even parity or find the
nearest delimiter that is not itself a closer by verifying surrounding chars),
then use that matching index instead of lastIndexOf and replace the
before.endsWith(startPattern) check with a call to this helper to decide whether
to unwrap or delete the opener. Ensure setSelectionRange, insert/replace logic
remains unchanged but driven by the new, pair-aware match.
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx`:
- Around line 66-80: The toolbar mapping in MessageBoxFormattingToolbar
currently filters to only formatters with an 'icon' by checking 'icon' in
formatter inside items.map, which drops TextButton entries (e.g., KaTeX) so
their 'link' and prompt paths never run; update the rendering so items without
an 'icon' are not skipped — render a TextButton (or the appropriate button
component) for formatters missing an icon and wire its onClick to the same logic
used for MessageComposerAction (respecting isPromptButton(formatter), the 'link'
in formatter branch that calls window.open(...), and
toggleMode(formatter.pattern, formatter.label)) so TextButton actions become
reachable in the large variant.
- Around line 35-38: toggleMode currently always calls
composer.wrapSelection(pattern) which causes the createComposerAPI unwrap-path
to run when disabling (turning Bold/Italic off) and thus removes formatting from
existing text. Change toggleMode so it checks the current state for the label
and only calls composer.wrapSelection(pattern) when turning the mode on; when
turning the mode off simply update setActiveModes to false without calling
wrapSelection. Use the existing activeModes state (or the prev value in
setActiveModes) to decide enable vs disable, keeping the label/key logic the
same.
---
Outside diff comments:
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx`:
- Around line 40-59: The small-variant dropdown items still call
composer.wrapSelection() directly (in FormattingToolbarDropdown) so they perform
one-shot formatting instead of toggling and updating activeModes; modify
FormattingToolbarDropdown to accept and use the same handlers used for
featuredFormatter — pass toggleMode and isPromptButton (or a single onItemClick
handler) from MessageBoxFormattingToolbar into FormattingToolbarDropdown and,
inside FormattingToolbarDropdown, invoke isPromptButton(item) ?
item.prompt(composer) : toggleMode(item.pattern, item.label) rather than
composer.wrapSelection(); this ensures dropdown items toggle modes and keep
activeModes consistent with featuredFormatter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 139ed68a-6d2a-4701-b59f-63a8b383b3e8
📒 Files selected for processing (4)
.changeset/hip-pans-hug.mdapps/meteor/app/ui-message/client/messageBox/createComposerAPI.tsapps/meteor/app/ui-message/client/messageBox/messageBoxFormatting.tsapps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/ui-message/client/messageBox/createComposerAPI.tsapps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsxapps/meteor/app/ui-message/client/messageBox/messageBoxFormatting.ts
🧠 Learnings (10)
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts.changeset/hip-pans-hug.md
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/ui-message/client/messageBox/createComposerAPI.tsapps/meteor/app/ui-message/client/messageBox/messageBoxFormatting.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/ui-message/client/messageBox/createComposerAPI.tsapps/meteor/app/ui-message/client/messageBox/messageBoxFormatting.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx.changeset/hip-pans-hug.mdapps/meteor/app/ui-message/client/messageBox/messageBoxFormatting.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx
📚 Learning: 2026-02-26T19:22:36.646Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx:40-40
Timestamp: 2026-02-26T19:22:36.646Z
Learning: In packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx, when the media session state is 'unavailable', the voiceCall action is not included in the actions object passed to CallHistoryActions, so it won't appear in the menu at all. The action filtering happens upstream before getItems is called, preventing any tooltip confusion for the unavailable state.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/hip-pans-hug.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/hip-pans-hug.md
📚 Learning: 2026-01-08T15:03:59.621Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
.changeset/hip-pans-hug.md
🪛 LanguageTool
.changeset/hip-pans-hug.md
[uncategorized] ~5-~5: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...- '@rocket.chat/meteor': patch --- Fix markdown formatting toolbar toggle behavior and ...
(MARKDOWN_NNP)
Comment on lines
+241
to
287
| const left = before.lastIndexOf(startPattern); | ||
| const rightRelative = after.indexOf(endPattern); | ||
| const right = rightRelative === -1 ? -1 : end + rightRelative; | ||
|
|
||
| if (endPatternFound) { | ||
| insertText(selectedText); | ||
| input.selectionStart = selectionStart - startPattern.length; | ||
| input.selectionEnd = selectionEnd + endPattern.length; | ||
| const inside = | ||
| left !== -1 && | ||
| right !== -1 && | ||
| left + startPattern.length <= start && | ||
| right >= end; | ||
|
|
||
| if (!document.execCommand?.('insertText', false, selectedText)) { | ||
| input.value = initText.slice(0, initText.length - startPattern.length) + selectedText + finalText.slice(endPattern.length); | ||
| } | ||
| if (inside) { | ||
| const unwrapStart = left; | ||
| const unwrapEnd = right + endPattern.length; | ||
|
|
||
| input.selectionStart = selectionStart - startPattern.length; | ||
| input.selectionEnd = input.selectionStart + selectedText.length; | ||
| triggerEvent(input, 'input'); | ||
| triggerEvent(input, 'change'); | ||
| const inner = text.slice( | ||
| left + startPattern.length, | ||
| right | ||
| ); | ||
|
|
||
| focus(); | ||
| return; | ||
| input.setSelectionRange(unwrapStart, unwrapEnd); | ||
|
|
||
| if (!document.execCommand?.('insertText', false, inner)) { | ||
| input.value = | ||
| text.slice(0, unwrapStart) + | ||
| inner + | ||
| text.slice(unwrapEnd); | ||
| } | ||
|
|
||
| const pos = unwrapStart; | ||
| input.setSelectionRange(pos, pos + inner.length); | ||
|
|
||
| triggerEvent(input, 'input'); | ||
| triggerEvent(input, 'change'); | ||
| focus(); | ||
| return; | ||
| } | ||
|
|
||
| if (!document.execCommand?.('insertText', false, pattern.replace('{{text}}', selectedText))) { | ||
| input.value = initText + pattern.replace('{{text}}', selectedText) + finalText; | ||
| if (!selected && before.endsWith(startPattern)) { | ||
| const newBefore = before.slice(0, before.length - startPattern.length); | ||
| input.value = newBefore + after; | ||
| const pos = newBefore.length; | ||
| input.setSelectionRange(pos, pos); | ||
| triggerEvent(input, 'input'); | ||
| triggerEvent(input, 'change'); | ||
| focus(); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Delimiter matching is not pair-aware for symmetric markers.
For **, ~~, and `, lastIndexOf(startPattern) and before.endsWith(startPattern) can hit a closing delimiter just as easily as an opening one. With the caret after **foo** or between **foo** and **bar**, these branches will strip the closer or unwrap across both spans, producing malformed text. This needs real pair matching instead of nearest-token matching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts` around
lines 241 - 287, The current logic using lastIndexOf(startPattern) and
before.endsWith(startPattern) treats symmetric delimiters (startPattern equal to
endPattern like "**", "~~", "`") as interchangeable opener/closer and can pick a
closing marker; replace this with pair-aware matching: implement a helper
(referenced from this block using startPattern, endPattern, left/right, before,
after) that scans backward through before and forward through after counting
delimiter occurrences or tokenizing so you only consider an opener if it pairs
correctly (e.g., for symmetric markers ensure an odd/even parity or find the
nearest delimiter that is not itself a closer by verifying surrounding chars),
then use that matching index instead of lastIndexOf and replace the
before.endsWith(startPattern) check with a call to this helper to decide whether
to unwrap or delete the opener. Ensure setSelectionRange, insert/replace logic
remains unchanged but driven by the new, pair-aware match.
Comment on lines
+35
to
+38
| const toggleMode = (pattern: string, label: string) => { | ||
| composer.wrapSelection(pattern); | ||
| setActiveModes((prev) => ({ ...prev, [label]: !prev[label] })); | ||
| }; |
Contributor
There was a problem hiding this comment.
toggleMode() still removes the formatted text on the second click.
composer.wrapSelection() runs on both enable and disable. In apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts:251-275, that API unwraps when the caret is already inside a formatted span, so clicking Bold/Italic/etc. again turns **abc** back into abc instead of just ending the typing mode. The on/off path needs separate open/close handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx`
around lines 35 - 38, toggleMode currently always calls
composer.wrapSelection(pattern) which causes the createComposerAPI unwrap-path
to run when disabling (turning Bold/Italic off) and thus removes formatting from
existing text. Change toggleMode so it checks the current state for the label
and only calls composer.wrapSelection(pattern) when turning the mode on; when
turning the mode off simply update setActiveModes to false without calling
wrapSelection. Use the existing activeModes state (or the prev value in
setActiveModes) to decide enable vs disable, keeping the label/key logic the
same.
Comment on lines
66
to
+80
| {items.map((formatter) => | ||
| 'icon' in formatter ? ( | ||
| <MessageComposerAction | ||
| disabled={disabled} | ||
| icon={formatter.icon} | ||
| key={formatter.label} | ||
| data-id={formatter.label} | ||
| icon={formatter.icon} | ||
| title={t(formatter.label)} | ||
| onClick={(): void => { | ||
| if (isPromptButton(formatter)) { | ||
| formatter.prompt(composer); | ||
| return; | ||
| } | ||
| if ('link' in formatter) { | ||
| window.open(formatter.link, '_blank', 'rel=noreferrer noopener'); | ||
| return; | ||
| } | ||
| composer.wrapSelection(formatter.pattern); | ||
| disabled={disabled} | ||
| pressed={!!activeModes[formatter.label]} | ||
| onClick={() => { | ||
| if (isPromptButton(formatter)) return formatter.prompt(composer); | ||
| if ('link' in formatter) return window.open(formatter.link, '_blank', 'rel=noreferrer noopener'); | ||
| toggleMode(formatter.pattern, formatter.label); | ||
| }} | ||
| /> | ||
| ) : ( | ||
| <span key={formatter.label} {...(disabled && { style: { pointerEvents: 'none' } })} title={formatter.label}> | ||
| <a href={formatter.link} target='_blank' rel='noopener noreferrer'> | ||
| {formatter.text()} | ||
| </a> | ||
| </span> | ||
| ), | ||
| ) : null, |
Contributor
There was a problem hiding this comment.
The large toolbar now drops TextButton items entirely.
This branch renders only formatters with an icon, but apps/meteor/app/ui-message/client/messageBox/messageBoxFormatting.ts:84-94 still exposes KaTeX as a TextButton. Returning null here makes that action unreachable in the large variant, and the 'link' in formatter path below never runs for it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx`
around lines 66 - 80, The toolbar mapping in MessageBoxFormattingToolbar
currently filters to only formatters with an 'icon' by checking 'icon' in
formatter inside items.map, which drops TextButton entries (e.g., KaTeX) so
their 'link' and prompt paths never run; update the rendering so items without
an 'icon' are not skipped — render a TextButton (or the appropriate button
component) for formatters missing an icon and wire its onClick to the same logic
used for MessageComposerAction (respecting isPromptButton(formatter), the 'link'
in formatter branch that calls window.open(...), and
toggleMode(formatter.pattern, formatter.label)) so TextButton actions become
reachable in the large variant.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts">
<violation number="1" location="apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts:241">
P1: `lastIndexOf(startPattern)` is not pair-aware: for symmetric delimiters like `**` and `~~`, it can match a *closing* delimiter instead of an opening one. For example, with the cursor between `**foo**` and `**bar**`, `lastIndexOf('**')` in `before` finds the closer of the first span, while `indexOf('**')` in `after` finds the opener of the second span, causing the code to believe the cursor is inside a formatted region and incorrectly unwrap across both spans. This needs balanced pair matching.</violation>
<violation number="2" location="apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts:278">
P1: Formatting toggle can delete the closing marker of an existing valid markdown span when the caret is immediately after it.</violation>
</file>
<file name="apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx">
<violation number="1" location="apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx:35">
P1: `toggleMode` calls `composer.wrapSelection(pattern)` unconditionally on both activation and deactivation. When deactivating, the cursor is inside the formatted span (e.g. `**hello**`), so `wrapSelection`'s unwrap logic strips the formatting the user just typed. The deactivation path should only close the formatting mode (e.g. move the cursor past the closing marker) without calling `wrapSelection`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (startPatternFound) { | ||
| const endPattern = pattern.slice(pattern.indexOf('{{text}}') + '{{text}}'.length); | ||
| const endPatternFound = input.value.slice(selectionEnd, selectionEnd + endPattern.length) === endPattern; | ||
| const left = before.lastIndexOf(startPattern); |
Contributor
There was a problem hiding this comment.
P1: lastIndexOf(startPattern) is not pair-aware: for symmetric delimiters like ** and ~~, it can match a closing delimiter instead of an opening one. For example, with the cursor between **foo** and **bar**, lastIndexOf('**') in before finds the closer of the first span, while indexOf('**') in after finds the opener of the second span, causing the code to believe the cursor is inside a formatted region and incorrectly unwrap across both spans. This needs balanced pair matching.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts, line 241:
<comment>`lastIndexOf(startPattern)` is not pair-aware: for symmetric delimiters like `**` and `~~`, it can match a *closing* delimiter instead of an opening one. For example, with the cursor between `**foo**` and `**bar**`, `lastIndexOf('**')` in `before` finds the closer of the first span, while `indexOf('**')` in `after` finds the opener of the second span, causing the code to believe the cursor is inside a formatted region and incorrectly unwrap across both spans. This needs balanced pair matching.</comment>
<file context>
@@ -223,48 +222,83 @@ export const createComposerAPI = (
- if (startPatternFound) {
- const endPattern = pattern.slice(pattern.indexOf('{{text}}') + '{{text}}'.length);
- const endPatternFound = input.value.slice(selectionEnd, selectionEnd + endPattern.length) === endPattern;
+ const left = before.lastIndexOf(startPattern);
+ const rightRelative = after.indexOf(endPattern);
+ const right = rightRelative === -1 ? -1 : end + rightRelative;
</file context>
|
|
||
| if (!document.execCommand?.('insertText', false, pattern.replace('{{text}}', selectedText))) { | ||
| input.value = initText + pattern.replace('{{text}}', selectedText) + finalText; | ||
| if (!selected && before.endsWith(startPattern)) { |
Contributor
There was a problem hiding this comment.
P1: Formatting toggle can delete the closing marker of an existing valid markdown span when the caret is immediately after it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts, line 278:
<comment>Formatting toggle can delete the closing marker of an existing valid markdown span when the caret is immediately after it.</comment>
<file context>
@@ -223,48 +222,83 @@ export const createComposerAPI = (
- if (!document.execCommand?.('insertText', false, pattern.replace('{{text}}', selectedText))) {
- input.value = initText + pattern.replace('{{text}}', selectedText) + finalText;
+ if (!selected && before.endsWith(startPattern)) {
+ const newBefore = before.slice(0, before.length - startPattern.length);
+ input.value = newBefore + after;
</file context>
| return () => textarea.removeEventListener('input', handleInput); | ||
| }, [composer]); | ||
|
|
||
| const toggleMode = (pattern: string, label: string) => { |
Contributor
There was a problem hiding this comment.
P1: toggleMode calls composer.wrapSelection(pattern) unconditionally on both activation and deactivation. When deactivating, the cursor is inside the formatted span (e.g. **hello**), so wrapSelection's unwrap logic strips the formatting the user just typed. The deactivation path should only close the formatting mode (e.g. move the cursor past the closing marker) without calling wrapSelection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/room/composer/messageBox/MessageBoxFormattingToolbar/MessageBoxFormattingToolbar.tsx, line 35:
<comment>`toggleMode` calls `composer.wrapSelection(pattern)` unconditionally on both activation and deactivation. When deactivating, the cursor is inside the formatted span (e.g. `**hello**`), so `wrapSelection`'s unwrap logic strips the formatting the user just typed. The deactivation path should only close the formatting mode (e.g. move the cursor past the closing marker) without calling `wrapSelection`.</comment>
<file context>
@@ -16,6 +16,26 @@ type MessageBoxFormattingToolbarProps = {
+ return () => textarea.removeEventListener('input', handleInput);
+ }, [composer]);
+
+ const toggleMode = (pattern: string, label: string) => {
+ composer.wrapSelection(pattern);
+ setActiveModes((prev) => ({ ...prev, [label]: !prev[label] }));
</file context>
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
1 participant
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.
Proposed changes (including video)
This PR fixes multiple issues in the Markdown formatting toolbar affecting Bold, Italic, Strikethrough, Inline Code, and Multiline Code actions.
🐛 Problems addressed
1. Button Activation Behavior
Formatting buttons previously behaved as one-time symbol inserters instead of toggle controls.
2. Duplicate / Nested Markdown Symbols
Applying formatting inside already formatted text produced malformed Markdown.
Examples:
✅ Fixes implemented
✔ Correct Markdown Patterns
Updated formatting patterns to follow CommonMark syntax:
**text**(was*text*)~~text~~(was~text~)This ensures consistent Markdown rendering and prevents ambiguity.
✔ Proper Toggle-Based Formatting Mode
Formatting buttons now behave as toggle controls:
This matches expected rich-text editor behavior.
✔ Visual Active State
Formatting buttons now visually indicate active state while formatting mode is enabled.
✔ Robust Formatting Detection
Improved formatting detection logic in
wrapSelection:Screen.Recording.2026-03-09.at.11.19.09.PM.mov
📁 Files modified
messageBoxFormatting.tscreateComposerAPI.tswrapSelectionlogicMessageBoxFormattingToolbar.tsx🎯 Result
Formatting toolbar now:
Issue(s)
Closes #39300
Summary by CodeRabbit
Bug Fixes
strikethrough,code blocks)New Features