Skip to content

fix: formatting buttons applies to entire paragraph#338

Merged
vitonsky merged 24 commits into
masterfrom
337-formatting-buttons-applies-to-entire-paragraph
Jul 6, 2026
Merged

fix: formatting buttons applies to entire paragraph#338
vitonsky merged 24 commits into
masterfrom
337-formatting-buttons-applies-to-entire-paragraph

Conversation

@vitonsky

@vitonsky vitonsky commented Jul 2, 2026

Copy link
Copy Markdown
Member

Fixes #337

The FormattingNode concept 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

{
  type: 'root',
  children: [
    {
      type: 'paragraph',
      children: [
        {
          type: 'emphasis',
          children: [
            {
              type: 'text',
              value: 'All text can be italic, something additionally can be '
            }
          ]
        },
        {
          type: 'emphasis',
          children: [
            {
              type: 'strong',
              children: [ { type: 'text', value: 'bold' } ]
            }
          ]
        },
        {
          type: 'emphasis',
          children: [ { type: 'text', value: ', ' } ]
        },
        {
          type: 'delete',
          children: [
            {
              type: 'emphasis',
              children: [ { type: 'text', value: 'strikethrough' } ]
            }
          ]
        },
        {
          type: 'emphasis',
          children: [ { type: 'text', value: ', or ' } ]
        },
        {
          type: 'delete',
          children: [
            {
              type: 'emphasis',
              children: [
                {
                  type: 'strong',
                  children: [ { type: 'text', value: 'bold AND strikethrough' } ]
                }
              ]
            }
          ]
        }
      ]
    }
  ]
}

we convert it to

{
  type: 'root',
  children: [
    {
      type: 'paragraph',
      children: [
        {
          type: 'emphasis',
          children: [
            {
              type: 'text',
              value: 'All text can be italic, something additionally can be '
            },
            {
              type: 'strong',
              children: [ { type: 'text', value: 'bold' } ]
            },
            { type: 'text', value: ', ' },
            {
              type: 'delete',
              children: [ { type: 'text', value: 'strikethrough' } ]
            },
            { type: 'text', value: ', or ' },
            {
              type: 'strong',
              children: [
                {
                  type: 'delete',
                  children: [ { type: 'text', value: 'bold AND strikethrough' } ]
                }
              ]
            }
          ]
        }
      ]
    }
  ]
}

That is overhead, but this way build a correct markdown.

Summary by CodeRabbit

  • New Features

    • Enhanced Markdown import/export for rich formatting, improving canonical nesting and round-trip consistency (including idempotent Markdown output).
    • Preserves blank lines in Markdown to maintain spacing.
  • Bug Fixes

    • More predictable bold/italic/strikethrough behavior by normalizing formatting structure and applying it inline for better nested and mixed-content scenarios (lists, quotes, tables).
    • Improved editor rendering so DOM output stays in sync when the editor value changes.

@vitonsky vitonsky linked an issue Jul 2, 2026 that may be closed by this pull request
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 78ef875b-73a5-4e7a-bbaf-41157619145f

📥 Commits

Reviewing files that changed from the base of the PR and between b39d1c4 and 17a409a.

⛔ Files ignored due to path filters (1)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (3)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx
  • 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
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/tests/markdown-roundtrips-consistency.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx

📝 Walkthrough

Walkthrough

Formatting 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.

Changes

RichEditor formatting and Markdown pipeline

Layer / File(s) Summary
Dispatch formatting and update editor wiring
packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx, packages/app/src/features/NoteEditor/RichEditor/plugins/KeyboardControlsPlugin/KeyboardControlsPlugin.ts, packages/app/src/features/NoteEditor/RichEditor/RichEditor.tsx, packages/app/src/features/NoteEditor/RichEditor/RichEditorContent.tsx
Formatting requests now dispatch FORMAT_TEXT_COMMAND, the formatting plugin is renamed to KeyboardControlsPlugin, the inline-exit handler is removed, and FormattingNode registration is deleted from editor setup.
Synchronous context helper
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.ts, packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.test.ts
createSyncContext adds get and use, with sync and async tests covering nested overrides and restoration.
Markdown conversion and serialization
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts
Markdown parsing now uses a synchronous text-format context, returns arrays of Lexical nodes, and applies formatting normalization during serialization.
Lexical to Markdown text wrapping
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts
Text conversion now wraps bold, italic, and strikethrough nodes directly from text format flags, and the old formatting-node branch is removed.
Remark formatting normalization
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts, packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts, packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts
New remark plugins normalize nested mark order and preserve blank lines with inserted paragraphs; tests cover grouping and normalized output.
Markdown round-trips and fixtures
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts, packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts, packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/utils.ts
New formatting samples are added, round-trip tests assert AST/string idempotence, and the test Lexical node set drops FormattingNode.
Test selection helper and formatting classes
packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/utils.ts, packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/richEditorFixtures.ts
selectText now spans multiple text nodes and returns a Range, and shared formatting class regexes are added.
Readonly and isolation DOM tests
packages/app/src/features/NoteEditor/RichEditor/__tests__/interactions/readonly.dom.test.ts, packages/app/src/features/NoteEditor/RichEditor/__tests__/isolation.dom.test.ts
New DOM tests verify readonly editors ignore input and formatting stays isolated between editor instances.
Render and heading DOM tests
packages/app/src/features/NoteEditor/RichEditor/__tests__/render.dom.test.ts, packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/heading.dom.test.ts
Rendering assertions switch to class-based formatting checks, and heading updates and reversion are covered by a new spec.
Formatting command DOM spec
packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts
New tests cover formatting toggles, multi-format combinations, partial selection formatting, and keyboard shortcut toggles.
Structural DOM specs
packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/image.dom.test.ts, packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.dom.test.ts, packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/paragraph.dom.test.ts
New specs cover image insertion placement, list and checklist behavior, and paragraph splitting plus blockquote exit behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

  • DeepinkApp/deepink#336: Both PRs modify the same Markdown RichEditor conversion pipeline files, especially markdownParser.ts and the text-format round-tripping path.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main formatting-button fix.
Linked Issues check ✅ Passed The PR routes formatting actions to Lexical's text-format command, so selected text is formatted instead of the whole paragraph.
Out of Scope Changes check ✅ Passed The changes stay focused on the rich-editor formatting pipeline and supporting tests; no clear unrelated code changes are evident.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 337-formatting-buttons-applies-to-entire-paragraph

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Remove the disabled transform instead of leaving it commented out.

If $convertTextNodeFormatting is 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 win

Stale TODO and leftover debug code.

The TODO at lines 7-8 references removing .fails, but the test below is already a plain test(...) — 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 value

Cross-feature import into a Markdown plugin file.

Importing TextFormat from @features/NoteEditor/EditorPanel couples the generic Markdown conversion module to an unrelated feature folder. Consider hoisting TextFormat to 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 | 🔵 Trivial

Tracked TODO for rich-formatting round-trip support.

The "Rich formatting" interactive case is disabled with a // TODO: support note, consistent with the known canonical-nesting-order limitation in convertLexicalNodeToMarkdownNode.ts (formats are always re-wrapped in a fixed strikethrough > italic > bold order 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

📥 Commits

Reviewing files that changed from the base of the PR and between 92f1e59 and 45bc10e.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (9)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/FormattingPlugin.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts
  • packages/app/src/features/NoteEditor/__tests__/interactive/formatting.dom.test.ts
  • packages/app/src/features/NoteEditor/__tests__/interactive/interactive.dom.test.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Ambiguous 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 new remarkLiftFormatting normalization 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 win

Headings aren't normalized.

liftFormattingNodes only visits 'paragraph' and 'tableCell'. Headings also contain PhrasingContent and 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 | 🔵 Trivial

Consider restricting insertion to block-level containers.

ignoredNodeTypes excludes table-related types, but phrasing/inline containers (paragraph, heading, emphasis, strong, delete, link, etc.) aren't excluded from the general traversal. If a PhrasingContent[] array ever produced a line gap between two inline siblings, this would splice a block-level paragraph node 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 | 🔵 Trivial

Address the TODO on placeholder position accuracy.

The synthetic paragraph uses offset: 0 regardless 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 on offset, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 45bc10e and a25ef10.

⛔ Files ignored due to path filters (1)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts
  • 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/remark/remarkLiftFormatting.ts
  • packages/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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts (2)

298-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider 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 value

Inconsistent test grouping.

The first two tests are wrapped in describe('Normalize Lexical tree', ...), while the last two (Line 226, Line 298) are top-level test() calls outside any describe block. Consider grouping all four under the same describe for 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 value

Duplicated inline-formatting text across fixtures.

The same inline formatting line is repeated verbatim in formattingInList, formattingInQuote (and previously simpleFormatting/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

📥 Commits

Reviewing files that changed from the base of the PR and between a25ef10 and b281ecb.

⛔ Files ignored due to path filters (1)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (4)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts
  • packages/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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b281ecb and 76ae1da.

📒 Files selected for processing (13)
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/interactions/readonly.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/isolation.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/render.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/resources/example.md
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/heading.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/image.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/paragraph.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderRichEditor.tsx
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/richEditorFixtures.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/utils.ts
  • packages/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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Keep format wrapping in canonical nesting order.

reverse() makes combined formatting serialize as strong > emphasis > delete > text, while the normalization contract expects delete > 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

📥 Commits

Reviewing files that changed from the base of the PR and between 76ae1da and f2e941b.

⛔ Files ignored due to path filters (1)
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (12)
  • packages/app/src/features/NoteEditor/RichEditor/RichEditor.tsx
  • packages/app/src/features/NoteEditor/RichEditor/RichEditorContent.tsx
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/utils/format.tsx
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/utils.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/KeyboardControlsPlugin/KeyboardControlsPlugin.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/utils.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts
  • packages/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

@vitonsky

vitonsky commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts (1)

67-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Test 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 #337 is 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 win

Stale 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

heading nodes aren't normalized.

liftFormattingNodes only visits paragraph and tableCell, but mdast heading nodes also carry PhrasingContent[] 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 | 🔵 Trivial

Address the TODO on synthetic node positions.

The hardcoded offset: 0 combined with the self-acknowledged // TODO: add tests to verify position are correct in complex cases suggests position data for inserted placeholder paragraphs isn't fully verified. If any downstream code relies on offset (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

📥 Commits

Reviewing files that changed from the base of the PR and between 92f1e59 and b39d1c4.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/__snapshots__/markdown-roundtrips-consistency.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (31)
  • packages/app/src/features/NoteEditor/RichEditor/RichEditor.tsx
  • packages/app/src/features/NoteEditor/RichEditor/RichEditorContent.tsx
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/interactions/readonly.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/isolation.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/render.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/resources/example.md
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/formatting.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/heading.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/image.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/paragraph.dom.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderRichEditor.tsx
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/richEditorFixtures.ts
  • packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/utils.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/EditorPanelPlugin.tsx
  • packages/app/src/features/NoteEditor/RichEditor/plugins/EditorPanelPlugin/utils/format.tsx
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Formatting/utils.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/KeyboardControlsPlugin/KeyboardControlsPlugin.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-roundtrips-consistency.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/markdown-samples/index.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/utils.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/createSyncContext.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/nodes/FormattingNode.tsx
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/liftFormattingNodes.test.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkLiftFormatting.ts
  • packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts
  • packages/app/src/features/NoteEditor/__tests__/behavior.dom.test.ts
  • packages/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

@vitonsky vitonsky merged commit ce713ee into master Jul 6, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Formatting buttons applies to entire paragraph

1 participant