[lexical-markdown] Bug Fix: Preserve inline formatting when wrapping already-formatted text with matching markers#8728
Conversation
…already-formatted text with matching markers Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
potatowagon
left a comment
There was a problem hiding this comment.
Reviewed by Navi (Tater Thoughts Bobblehead) on behalf of @potatowagon.
Assessment: LGTM ✅ — Clean, well-motivated bug fix.
What I checked:
Logic correctness: The old code had two clear bugs as described in the PR:
nextSelection.hasFormat(format)was checking the selection format (initialized to 0), not the node format — so it always returned false (no-op guard).nextSelection.formatText(format)XOR-toggles the bit, so already-formatted nodes got their format cleared instead of preserved.
The fix correctly iterates over extracted nodes and only toggles the format ON when the node does not already have it. This is a "set ON, never toggle OFF" pattern — exactly right for "wrapping already-formatted text."
Edge cases considered:
- Mixed-format selections (some nodes bold, some not): ✅ Only unformatted nodes get toggled.
- Non-text nodes in selection: ✅
$isTextNode(node)guard skips them. - Multiple formats (e.g.
***= bold+italic): ✅ Outer loop iteratesmatcher.format, inner loop applies per-node — each format applied independently.
Pattern consistency: The PR description notes this mirrors the import-side logic at importTextFormatTransformer.ts#L455-461. I confirmed the same per-node hasFormat/toggleFormat pattern is used there.
Test coverage: No new automated tests added, but the fix is minimal (5 lines changed) and the author verified all inline format markers (**, __, *, _, ~~, ==, ***, ___) in the playground. Given the simplicity of the fix and the clear bug reproduction in #8727, manual verification is adequate here.
CI status: All checks green (CLA ✅, Vercel previews ✅). Note: no unit/e2e CI triggered yet — only CLA and deploy previews ran. The full test suite hasn't run, but this is a one-file, 5-line change with no API surface changes.
www compat: No exports changed, no API surface modified, no type signature changes. Internal consumers (MLCComposer, EPS) are unaffected — this only changes runtime behavior of the markdown shortcut transformer to stop incorrectly clearing format bits.
Risk: Very low. The change is strictly additive in effect (preserves formatting that was previously incorrectly removed). No regressions expected.
|
Nice issue write-up — the reproduction steps and screenshots made it easy to confirm the bug. A regression test would strengthen this fix. One gotcha with headless tests: I also left an inline suggestion for a simpler approach to the fix itself. |
| nextSelection.focus.set(closeNode.__key, newOffset, 'text'); | ||
|
|
||
| // Apply formatting to selected text | ||
| const extractedNodes = nextSelection.extract(); |
There was a problem hiding this comment.
RangeSelection.formatText already has a second alignWithFormat parameter designed for this — it applies the format without toggling it off on nodes that already have it. This avoids reimplementing the split-and-check logic that formatText handles internally:
import {TEXT_TYPE_TO_FORMAT} from 'lexical';
for (const format of matcher.format) {
nextSelection.formatText(format, TEXT_TYPE_TO_FORMAT[format]);
}
etrepum
left a comment
There was a problem hiding this comment.
This should have tests to confirm that it behaves as expected
Closes #8727
Description
The apply-formatting loop in
$runTextFormatTransformers(after the markers are stripped) had two issues:nextSelection.hasFormat(format)guard was a no-op.nextSelectionis a freshly-created$createRangeSelection()whoseformatis initialised to0, so the check always returnedfalseregardless of the actual node format.nextSelection.formatText(format)was called withalignWithFormat = null, which XOR-toggles the bit. For nodes that already had the format, this cleared it.Replaced the loop with a per-node "set ON, never toggle" pass over
nextSelection.extract(): for each node, set the bit only ifhasFormat(format)is false. This mirrors the existing pattern used on the import side (https://github.com/facebook/lexical/blob/main/packages/lexical-markdown/src/importTextFormatTransformer.ts#L455-L461).Test plan
Before
Reproduces via the Steps To Reproduce in issue #8727, or via the minimal reproduction environment:
After
Verified in the Lexical playground that every inline format marker (
**,__,*,_,~~,==,***,___) preserves the formatting when wrapping already-formatted text.