fix: formatting buttons applies to entire paragraph#338
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughFormatting dispatch now uses Lexical commands, while Markdown parsing and serialization carry text formats through synchronous context and normalization helpers. RichEditor wiring changes and new DOM tests cover formatting, structure, readonly behavior, and editor isolation. ChangesRichEditor formatting and Markdown pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/FormattingPlugin.ts (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the disabled transform instead of leaving it commented out.
If
$convertTextNodeFormattingis no longer needed for this flow, prefer removing the line (and its now-possibly-unused import/helper) rather than commenting it out, to avoid dead code lingering in the codebase.🧹 Suggested cleanup (pending verification above)
- // editor.registerNodeTransform(TextNode, $convertTextNodeFormatting),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/FormattingPlugin.ts` at line 67, Remove the commented-out editor.registerNodeTransform(TextNode, $convertTextNodeFormatting) line from FormattingPlugin and clean up any now-unused imports or helpers tied to $convertTextNodeFormatting. Keep the FormattingPlugin implementation focused on the active flow only, and verify the plugin still compiles after deleting the dead code.packages/app/src/features/NoteEditor/__tests__/interactive/formatting.dom.test.ts (1)
7-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale TODO and leftover debug code.
The TODO at lines 7-8 references removing
.fails, but the test below is already a plaintest(...)— the TODO is now stale and could mislead readers about the test's status. Also, lines 20-21, 23, and 28 contain commented-out/dead assertion code that should be cleaned up.🧹 Suggested cleanup
-// TODO: Remove `.fails` after fixing the formatting implementation -// The assertions below describe the expected behavior and should remain unchanged test('Applies formatting to a selected part of a text node', async () => { const user = userEvent.setup(); const richEditor = await renderRichEditor({ value: 'Hello, my dear friends!' }); const editor = screen.getByRole('textbox'); await user.click(editor); selectText(editor, 'friends'); // Apply formatting await richEditor.format('italic'); - // await act(async () => await user.keyboard('{Ctrl>}b{/Ctrl}')); - // await user.keyboard('{Control>}i{/Control}'); - - // expect(richEditor.container.outerHTML).toBe("") expect( screen.getAllByRole('emphasis').find((el) => within(el).queryByText('friends')) ?.textContent, ).toBe('friends'); - // expect(screen.getByRole('emphasis')).toHaveTextContent('Hello, my dear '); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/__tests__/interactive/formatting.dom.test.ts` around lines 7 - 29, The formatting test has stale cleanup comments and dead debug code: the TODO about removing “.fails” is no longer accurate in the interactive formatting test, and the commented-out keyboard/assertion lines should be removed. Update the test in formatting.dom.test.ts by deleting the obsolete TODO and commented blocks while keeping the existing active assertion and test behavior unchanged.packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCross-feature import into a Markdown plugin file.
Importing
TextFormatfrom@features/NoteEditor/EditorPanelcouples the generic Markdown conversion module to an unrelated feature folder. Consider hoistingTextFormatto a shared/lower-level module (e.g. alongside the Lexical types used here) to keep the dependency direction from Markdown plugin → EditorPanel from growing, and to avoid risk of circular imports if EditorPanel ever depends on Markdown plugin internals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts` at line 18, The Markdown parser is importing TextFormat from the EditorPanel feature, which creates an unnecessary cross-feature dependency from the generic Markdown plugin into a higher-level UI module. Move TextFormat into a shared lower-level location used by markdownParser and the Lexical-related types, then update markdownParser to import it from that shared module instead of `@features/NoteEditor/EditorPanel`.packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts (1)
42-46: 🎯 Functional Correctness | 🔵 TrivialTracked TODO for rich-formatting round-trip support.
The "Rich formatting" interactive case is disabled with a
// TODO: supportnote, consistent with the known canonical-nesting-order limitation inconvertLexicalNodeToMarkdownNode.ts(formats are always re-wrapped in a fixedstrikethrough > italic > boldorder regardless of original nesting). Combined with the inline-code format loss flagged in the other two files, want me to open a tracking issue consolidating these formatting round-trip gaps?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts` around lines 42 - 46, The “Rich formatting” round-trip test case is currently disabled with a TODO, but it should be tied to a single tracked follow-up for the formatting gaps instead of remaining an orphaned note. Update the Markdown round-trips test around markdown-roundtrips-consistency and the related conversion path in convertLexicalNodeToMarkdownNode to reference the consolidated tracking issue covering canonical nesting order and inline-code format loss, and keep the test case clearly marked as pending until that work lands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/FormattingPlugin.ts`:
- Line 67: Restore the TextNode formatting transform in FormattingPlugin by
re-enabling the editor.registerNodeTransform(TextNode,
$convertTextNodeFormatting) registration, since FormattingPlugin relies on
$convertTextNodeFormatting to emit the b/em/del wrappers expected by the
existing DOM behavior. Keep the change localized to the FormattingPlugin setup
so the formatting pipeline continues to run for native text-format commands.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts`:
- Around line 138-145: The text formatting path in convertToMarkdownNode is
applying every entry from textFormatContext with toggleFormat, which can cancel
duplicate formats when the same TextFormat appears more than once. Update the
Markdown parsing flow around textFormatContext and the text node creation in
convertToMarkdownNode to dedupe active formats before applying them, so repeated
nested emphasis entries still result in the format staying enabled instead of
being flipped off.
- Around line 228-232: The inlineCode handling in markdownParser should preserve
the current textFormatContext bits instead of resetting formatting to only code,
so nested bold/italic/strikethrough survives around code spans. Update the
inlineCode branch in markdownParser and the markdown serialization logic in
convertLexicalNodeToMarkdownNode to emit combined code +
bold/italic/strikethrough markers, ensuring cases like code wrapped by bold
round-trip correctly.
---
Nitpick comments:
In
`@packages/app/src/features/NoteEditor/__tests__/interactive/formatting.dom.test.ts`:
- Around line 7-29: The formatting test has stale cleanup comments and dead
debug code: the TODO about removing “.fails” is no longer accurate in the
interactive formatting test, and the commented-out keyboard/assertion lines
should be removed. Update the test in formatting.dom.test.ts by deleting the
obsolete TODO and commented blocks while keeping the existing active assertion
and test behavior unchanged.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/FormattingPlugin.ts`:
- Line 67: Remove the commented-out editor.registerNodeTransform(TextNode,
$convertTextNodeFormatting) line from FormattingPlugin and clean up any
now-unused imports or helpers tied to $convertTextNodeFormatting. Keep the
FormattingPlugin implementation focused on the active flow only, and verify the
plugin still compiles after deleting the dead code.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts`:
- Around line 42-46: The “Rich formatting” round-trip test case is currently
disabled with a TODO, but it should be tied to a single tracked follow-up for
the formatting gaps instead of remaining an orphaned note. Update the Markdown
round-trips test around markdown-roundtrips-consistency and the related
conversion path in convertLexicalNodeToMarkdownNode to reference the
consolidated tracking issue covering canonical nesting order and inline-code
format loss, and keep the test case clearly marked as pending until that work
lands.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts`:
- Line 18: The Markdown parser is importing TextFormat from the EditorPanel
feature, which creates an unnecessary cross-feature dependency from the generic
Markdown plugin into a higher-level UI module. Move TextFormat into a shared
lower-level location used by markdownParser and the Lexical-related types, then
update markdownParser to import it from that shared module instead of
`@features/NoteEditor/EditorPanel`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fa9b365-dcb8-4ae4-8321-b5b6bd8d11b4
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsxpackages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/FormattingPlugin.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.tspackages/app/src/features/NoteEditor/__tests__/interactive/formatting.dom.test.tspackages/app/src/features/NoteEditor/__tests__/interactive/interactive.dom.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts (1)
9-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAmbiguous nested-emphasis delimiters in
formattingInTable.The cell text
*All text can be italic, ... or **~~bold AND strikethrough~~***relies on CommonMark resolving a 3-asterisk closer as "close**then close*" purely via delimiter-run length matching. Since this fixture backs the round-trip AST-equality/idempotence tests for the newremarkLiftFormattingnormalization logic, using the same delimiter character (*) for both italic and bold nesting makes the test harder to reason about and more fragile to any future micromark/remark parsing changes. Using a distinct delimiter for italic (e.g._..._) would remove the ambiguity and make the fixture's expected AST unambiguous by construction.♻️ Suggested clarification
export const formattingInTable = ` | foo | bar | | ---- | ----- | -| *All text can be italic, something additionally can be **bold**, ~~strikethrough~~, or **~~bold AND strikethrough~~*** | plain text and ~~deleted~~ | +| _All text can be italic, something additionally can be **bold**, ~~strikethrough~~, or **~~bold AND strikethrough~~**_ | plain text and ~~deleted~~ | | **bold** | *italic* | `.trim();Since this touches remark/micromark delimiter resolution behavior, worth double-checking against current parser semantics before relying on it for round-trip correctness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts` around lines 9 - 14, The `formattingInTable` fixture uses ambiguous nested emphasis with the same `*` delimiter for both italic and bold, which makes the round-trip markdown AST test harder to reason about. Update the sample in `markdown-samples/index.ts` so the nested italic/bold case uses a distinct italic delimiter such as `_..._`, keeping the intended formatting coverage while making the `remarkLiftFormatting` idempotence expectation unambiguous.packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts (1)
123-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHeadings aren't normalized.
liftFormattingNodesonly visits'paragraph'and'tableCell'. Headings also containPhrasingContentand can carry the same inconsistent/nested mark ordering, but they're excluded from this normalization pass.♻️ Suggested addition
- visit(tree, ['paragraph', 'tableCell'], (node) => { + visit(tree, ['paragraph', 'heading', 'tableCell'], (node) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts` around lines 123 - 130, `liftFormattingNodes` is only normalizing `paragraph` and `tableCell`, so `heading` nodes are left with inconsistent nested formatting. Update the `visit` call in `liftFormattingNodes` to include `heading` alongside the existing node types, and keep using `liftChildren` on any node with `children` so headings are normalized the same way.packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts (2)
6-24: 🚀 Performance & Scalability | 🔵 TrivialConsider restricting insertion to block-level containers.
ignoredNodeTypesexcludes table-related types, but phrasing/inline containers (paragraph,heading,emphasis,strong,delete,link, etc.) aren't excluded from the general traversal. If aPhrasingContent[]array ever produced a line gap between two inline siblings, this would splice a block-levelparagraphnode into what must structurally be inline content, yielding an invalid mdast tree. CommonMark generally prevents a blank line from appearing mid-paragraph, so this is unlikely to trigger today, but there's no explicit guard against it (unlike the table exclusion, which was clearly added deliberately).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts` around lines 6 - 24, Restrict the blank-line preservation logic in remarkPreserveBlankLines to block-level container nodes only, because the current traversal can insert paragraph nodes into inline/phrasing content and produce an invalid mdast tree. Update the visit callback to exclude phrasing/inline parents such as paragraph, heading, emphasis, strong, delete, and link, alongside the existing ignoredNodeTypes table-related exclusions, so only eligible block containers are processed before splicing in blank-line paragraphs.
39-53: 📐 Maintainability & Code Quality | 🔵 TrivialAddress the TODO on placeholder position accuracy.
The synthetic paragraph uses
offset: 0regardless of actual source offset, and the author has flagged this as untested for complex cases. If downstream code (e.g. cursor/position mapping between Markdown and Lexical) relies onoffset, this could produce inaccurate mappings in nested structures (lists, blockquotes).Want me to draft test cases covering blank-line preservation inside nested blocks (list items, blockquotes) to validate position correctness?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts` around lines 39 - 53, The synthetic blank-line paragraph in remarkPreserveBlankLines uses a hardcoded position offset and the placeholder position is not validated in nested Markdown structures. Update the placeholder node creation in remarkPreserveBlankLines so its position metadata is derived consistently with surrounding AST positions instead of always using offset 0, and add tests that cover blank-line preservation inside nested blocks such as list items and blockquotes to verify cursor/position mapping remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts`:
- Around line 79-113: The recursive rebuild in rebuildFromLeaves is skipping
earlier marks for later leaves in a grouped run, which can drop nested
formatting when mixed marks are present. Update the recursion so each grouped
segment is rescanned from the start of MARK_ORDER rather than continuing with
markIndex + 1, and keep the stripping logic in rebuildFromLeaves consistent with
rebuilding nested phrasing content.
---
Nitpick comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts`:
- Around line 9-14: The `formattingInTable` fixture uses ambiguous nested
emphasis with the same `*` delimiter for both italic and bold, which makes the
round-trip markdown AST test harder to reason about. Update the sample in
`markdown-samples/index.ts` so the nested italic/bold case uses a distinct
italic delimiter such as `_..._`, keeping the intended formatting coverage while
making the `remarkLiftFormatting` idempotence expectation unambiguous.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts`:
- Around line 123-130: `liftFormattingNodes` is only normalizing `paragraph` and
`tableCell`, so `heading` nodes are left with inconsistent nested formatting.
Update the `visit` call in `liftFormattingNodes` to include `heading` alongside
the existing node types, and keep using `liftChildren` on any node with
`children` so headings are normalized the same way.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts`:
- Around line 6-24: Restrict the blank-line preservation logic in
remarkPreserveBlankLines to block-level container nodes only, because the
current traversal can insert paragraph nodes into inline/phrasing content and
produce an invalid mdast tree. Update the visit callback to exclude
phrasing/inline parents such as paragraph, heading, emphasis, strong, delete,
and link, alongside the existing ignoredNodeTypes table-related exclusions, so
only eligible block containers are processed before splicing in blank-line
paragraphs.
- Around line 39-53: The synthetic blank-line paragraph in
remarkPreserveBlankLines uses a hardcoded position offset and the placeholder
position is not validated in nested Markdown structures. Update the placeholder
node creation in remarkPreserveBlankLines so its position metadata is derived
consistently with surrounding AST positions instead of always using offset 0,
and add tests that cover blank-line preservation inside nested blocks such as
list items and blockquotes to verify cursor/position mapping remains correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c75434d-85ca-498f-9830-2e79a8ee8011
⛔ Files ignored due to path filters (1)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/tests/markdown-roundtrips-consistency.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts (2)
298-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a negative test case.
All tests here verify that formatting nodes with equal/overlapping formats get merged. There's no test asserting that sibling nodes with genuinely different formats (e.g., emphasis-only vs strong-only, non-overlapping) are left un-merged, which would guard against over-aggressive lifting regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts` around lines 298 - 368, Add a negative test in liftFormattingNodes.test.ts to cover sibling formatting nodes that should not be merged, since the current tests only validate successful grouping of equal/overlapping formats. Use liftFormattingNodes and the existing u/Root helpers to build a case with genuinely different sibling formats (for example emphasis-only vs strong-only, or other non-overlapping formatting) and assert the output preserves them as separate nodes rather than combining them.
6-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent test grouping.
The first two tests are wrapped in
describe('Normalize Lexical tree', ...), while the last two (Line 226, Line 298) are top-leveltest()calls outside anydescribeblock. Consider grouping all four under the samedescribefor consistency and clearer test-report structure.Also applies to: 226-368
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts` around lines 6 - 224, The test file mixes a scoped suite with standalone tests, so move the remaining top-level liftFormattingNodes cases into the existing describe('Normalize Lexical tree') block for consistency. Keep all four scenarios grouped together under that suite, using the same liftFormattingNodes and Root assertions so the test report stays organized and the related normalization cases are easy to find.packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts (1)
9-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated inline-formatting text across fixtures.
The same inline formatting line is repeated verbatim in
formattingInList,formattingInQuote(and previouslysimpleFormatting/richFormatting). Consider extracting the shared inline snippet into a constant and interpolating it into each block wrapper (list/quote/table) to reduce duplication and keep future edits in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts` around lines 9 - 24, The inline formatting sentence is duplicated across the markdown sample fixtures, making them harder to keep in sync. Extract the shared text into a reusable constant in the markdown-samples test fixture module, then interpolate it into each sample such as formattingInList and formattingInQuote (and the other related fixtures) so only the wrapper syntax differs while the content stays centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts`:
- Around line 9-24: The inline formatting sentence is duplicated across the
markdown sample fixtures, making them harder to keep in sync. Extract the shared
text into a reusable constant in the markdown-samples test fixture module, then
interpolate it into each sample such as formattingInList and formattingInQuote
(and the other related fixtures) so only the wrapper syntax differs while the
content stays centralized.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts`:
- Around line 298-368: Add a negative test in liftFormattingNodes.test.ts to
cover sibling formatting nodes that should not be merged, since the current
tests only validate successful grouping of equal/overlapping formats. Use
liftFormattingNodes and the existing u/Root helpers to build a case with
genuinely different sibling formats (for example emphasis-only vs strong-only,
or other non-overlapping formatting) and assert the output preserves them as
separate nodes rather than combining them.
- Around line 6-224: The test file mixes a scoped suite with standalone tests,
so move the remaining top-level liftFormattingNodes cases into the existing
describe('Normalize Lexical tree') block for consistency. Keep all four
scenarios grouped together under that suite, using the same liftFormattingNodes
and Root assertions so the test report stays organized and the related
normalization cases are easy to find.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7bfa78b-14e5-488e-b692-0e05058f3034
⛔ Files ignored due to path filters (1)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/tests/markdown-roundtrips-consistency.test.ts
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts`:
- Around line 116-120: The cancel-formatting test in the RichEditor formatting
spec is asserting the wrong element, since the outer editor container never
reflects inline formatting state. Update the assertion in the test that uses
selectText and user.keyboard(shortcut) to check the selected text/format target
identified by selectionText or the formatted element produced by the helper, so
the expectation verifies that formatClass was actually removed from the content
being toggled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 253de80a-bdef-4355-ae3b-c9e70324b6fb
📒 Files selected for processing (13)
packages/app/src/features/NoteEditor/RichEditor/__tests__/interactions/readonly.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/isolation.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/render.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/resources/example.mdpackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/heading.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/image.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/paragraph.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderRichEditor.tsxpackages/app/src/features/NoteEditor/RichEditor/__tests__/utils/richEditorFixtures.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/utils/utils.tspackages/app/src/features/NoteEditor/__tests__/behavior.dom.test.ts
💤 Files with no reviewable changes (1)
- packages/app/src/features/NoteEditor/tests/behavior.dom.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app/src/features/NoteEditor/RichEditor/tests/utils/richEditorFixtures.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts (1)
112-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep format wrapping in canonical nesting order.
reverse()makes combined formatting serialize asstrong > emphasis > delete > text, while the normalization contract expectsdelete > emphasis > strong > text. Drop the reverse so the converter emits the canonical shape before any later normalization pass.Proposed fix
const formatsOrder = ( ['bold', 'italic', 'strikethrough'] satisfies TextFormatType[] - ).reverse(); + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts` around lines 112 - 124, The formatting wrap order in convertLexicalNodeToMarkdownNode should stay canonical, but formatsOrder is being reversed before wrapping. Update the wrapping logic in convertLexicalNodeToMarkdownNode so the loop applies formats in the original order from the formats array instead of calling reverse(), ensuring combined formatting serializes as delete > emphasis > strong > text. Use the existing symbols formatsOrder, nodesMap, and wrappedNode to make the change in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.test.ts`:
- Around line 51-64: The async assertion in createSyncContext.test.ts is not
being awaited or returned, so the test may finish before the promise settles.
Update the test around ctx.use and getValue to either await the
expect(...).resolves assertion or return it from the test body so the closure
behavior is actually verified.
---
Outside diff comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts`:
- Around line 112-124: The formatting wrap order in
convertLexicalNodeToMarkdownNode should stay canonical, but formatsOrder is
being reversed before wrapping. Update the wrapping logic in
convertLexicalNodeToMarkdownNode so the loop applies formats in the original
order from the formats array instead of calling reverse(), ensuring combined
formatting serializes as delete > emphasis > strong > text. Use the existing
symbols formatsOrder, nodesMap, and wrappedNode to make the change in place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fab04c33-1aa9-423f-b5a9-912f14079535
⛔ Files ignored due to path filters (1)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
packages/app/src/features/NoteEditor/RichEditor/RichEditor.tsxpackages/app/src/features/NoteEditor/RichEditor/RichEditorContent.tsxpackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/utils/format.tsxpackages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/utils.tspackages/app/src/features/NoteEditor/RichEditor/plugins/KeyboardControlsPlugin/KeyboardControlsPlugin.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/utils.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/nodes/FormattingNode.tsx
💤 Files with no reviewable changes (5)
- packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/utils.ts
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/tests/utils.ts
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/nodes/FormattingNode.tsx
- packages/app/src/features/NoteEditor/RichEditor/RichEditor.tsx
- packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/utils/format.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/tests/markdown-roundtrips-consistency.test.ts
- packages/app/src/features/NoteEditor/RichEditor/tests/spec/formatting.dom.test.ts
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts (1)
67-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't assert the rest of the paragraph stays unformatted.
This test only verifies
'friends'gets the italic class after a partial selection, but it never checks that the untouched text ('Hello, my dear ') is not italicized. Since#337is specifically about formatting leaking into the whole paragraph, an explicit negative assertion here would directly validate the fix rather than just showing the target substring is formatted.✅ Suggested addition
selectText(editor, 'friends'); await richEditor.format('italic'); expect(within(editor).getByText('friends')).toHaveClass(textFormatClasses.italic); + expect(within(editor).getByText('Hello, my dear ', { exact: false })).not.toHaveClass( + textFormatClasses.italic, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts` around lines 67 - 79, The partial-formatting test in RichEditor only checks that “friends” becomes italic, but it does not verify that the rest of the paragraph remains unformatted. Update the spec in the formatting DOM test to add an explicit negative assertion around the untouched text segment, using renderRichEditor, selectText, richEditor.format, and within(editor) so the test confirms formatting does not leak beyond the selected substring.packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx (1)
40-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale TODO now contradicts the fix.
The TODO on line 41 says formatting selected text slices is unsupported, but dispatching
FORMAT_TEXT_COMMAND(Lexical's built-in) now formats only the active selection — this is precisely what the PR fixes. Leaving the comment risks confusing future readers into thinking the gap still exists.📝 Suggested cleanup
const cleanupFormatting = onFormatting.watch((format) => { - // TODO: support formatting selected text slices editor.update(() => { editor.dispatchCommand(FORMAT_TEXT_COMMAND, format); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx` around lines 40 - 44, The TODO in EditorPanelPlugin’s onFormatting watcher is now stale because dispatching FORMAT_TEXT_COMMAND already applies formatting to the active selection. Remove the outdated comment in EditorPanelPlugin.tsx around cleanupFormatting so it no longer suggests selected text slice formatting is still unsupported, and keep the logic in editor.update and editor.dispatchCommand(FORMAT_TEXT_COMMAND, format) unchanged.packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts (1)
120-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
headingnodes aren't normalized.
liftFormattingNodesonly visitsparagraphandtableCell, but mdastheadingnodes also carryPhrasingContent[]children (e.g.# **bold** and *italic*). Nested/out-of-order formatting inside headings won't be lifted into canonical order.Proposed fix
- visit(tree, ['paragraph', 'tableCell'], (node) => { + visit(tree, ['paragraph', 'heading', 'tableCell'], (node) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts` around lines 120 - 129, `liftFormattingNodes` is missing mdast `heading` nodes, so nested formatting inside headings is not normalized. Update the `visit` call in `liftFormattingNodes` to include `heading` alongside `paragraph` and `tableCell`, and keep using `liftChildren` on nodes with `children` so heading phrasing content is lifted into the same canonical order.packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts (1)
41-49: 🎯 Functional Correctness | 🔵 TrivialAddress the TODO on synthetic node positions.
The hardcoded
offset: 0combined with the self-acknowledged// TODO: add tests to verify position are correct in complex casessuggests position data for inserted placeholder paragraphs isn't fully verified. If any downstream code relies onoffset(e.g. source mapping, incremental diffing), this could produce subtly wrong results in nested/complex documents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts` around lines 41 - 49, The synthetic placeholder paragraph in remarkPreserveBlankLines should not leave position data unverified with a hardcoded offset of 0. Update the logic that builds the emptyLine RootContent node so its position reflects the real blank-line insertion context consistently, and add coverage for complex/nested cases to validate the generated start/end offsets and line tracking. Focus on the emptyLine construction in remarkPreserveBlankLines and any helper used to derive synthetic node positions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts`:
- Around line 67-79: The partial-formatting test in RichEditor only checks that
“friends” becomes italic, but it does not verify that the rest of the paragraph
remains unformatted. Update the spec in the formatting DOM test to add an
explicit negative assertion around the untouched text segment, using
renderRichEditor, selectText, richEditor.format, and within(editor) so the test
confirms formatting does not leak beyond the selected substring.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx`:
- Around line 40-44: The TODO in EditorPanelPlugin’s onFormatting watcher is now
stale because dispatching FORMAT_TEXT_COMMAND already applies formatting to the
active selection. Remove the outdated comment in EditorPanelPlugin.tsx around
cleanupFormatting so it no longer suggests selected text slice formatting is
still unsupported, and keep the logic in editor.update and
editor.dispatchCommand(FORMAT_TEXT_COMMAND, format) unchanged.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts`:
- Around line 120-129: `liftFormattingNodes` is missing mdast `heading` nodes,
so nested formatting inside headings is not normalized. Update the `visit` call
in `liftFormattingNodes` to include `heading` alongside `paragraph` and
`tableCell`, and keep using `liftChildren` on nodes with `children` so heading
phrasing content is lifted into the same canonical order.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts`:
- Around line 41-49: The synthetic placeholder paragraph in
remarkPreserveBlankLines should not leave position data unverified with a
hardcoded offset of 0. Update the logic that builds the emptyLine RootContent
node so its position reflects the real blank-line insertion context
consistently, and add coverage for complex/nested cases to validate the
generated start/end offsets and line tracking. Focus on the emptyLine
construction in remarkPreserveBlankLines and any helper used to derive synthetic
node positions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4309f4a5-2a23-467d-a4cf-b4a3aafa7d33
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (31)
packages/app/src/features/NoteEditor/RichEditor/RichEditor.tsxpackages/app/src/features/NoteEditor/RichEditor/RichEditorContent.tsxpackages/app/src/features/NoteEditor/RichEditor/__tests__/interactions/readonly.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/isolation.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/render.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/resources/example.mdpackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/heading.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/image.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/spec/paragraph.dom.test.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderRichEditor.tsxpackages/app/src/features/NoteEditor/RichEditor/__tests__/utils/richEditorFixtures.tspackages/app/src/features/NoteEditor/RichEditor/__tests__/utils/utils.tspackages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsxpackages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/utils/format.tsxpackages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/utils.tspackages/app/src/features/NoteEditor/RichEditor/plugins/KeyboardControlsPlugin/KeyboardControlsPlugin.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/utils.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/nodes/FormattingNode.tsxpackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.tspackages/app/src/features/NoteEditor/__tests__/behavior.dom.test.tspackages/app/src/features/NoteEditor/__tests__/interactive.dom.test.ts
💤 Files with no reviewable changes (7)
- packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/utils.ts
- packages/app/src/features/NoteEditor/tests/interactive.dom.test.ts
- packages/app/src/features/NoteEditor/RichEditor/RichEditor.tsx
- packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/utils/format.tsx
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/nodes/FormattingNode.tsx
- packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/tests/utils.ts
- packages/app/src/features/NoteEditor/tests/behavior.dom.test.ts
Fixes #337
The
FormattingNodeconcept is removed. Now we use native Lexical formatting and its hotkeys and commands.Serialization/de-serialization is updated to convert the formatting of Lexical text nodes to MD AST.
To ensure correct markdown we normalize the MD AST while serialization so group siblings with equal formatting.
As example, the raw MD AST after serialization is
we convert it to
That is overhead, but this way build a correct markdown.
Summary by CodeRabbit
New Features
Bug Fixes