fix: inconsistent markdown conversion - #336
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRefactors Markdown parsing and serialization into exported helpers, moves Lexical-to-mdast conversion into a standalone module, and adds round-trip consistency tests with expanded fixtures. ChangesMarkdown AST conversion refactor and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🤖 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/__tests__/markdown-roundtrips-consistency.test.ts`:
- Around line 21-72: The Markdown round-trip test cases use the wrong task-list
syntax and never cover the mutation path that reproduces the bug. Update the
fixtures in markdown-roundtrips-consistency.test.ts to use GFM task-list markup
with spaced checkboxes, and extend the test around createLexicalEditorInstance,
updateEditorState, and $convertFromMarkdownString so it also performs the editor
edit sequence that triggers the nested checklist regression before asserting the
Markdown AST round-trip.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/utils.ts`:
- Around line 71-80: The test helper’s cleanup in the Markdown editor setup
leaves the appended root element in document.body, so update the destroy() logic
in the create test utility to fully remove the rootElement after calling
editor.setRootElement(null). Use the unique symbols createEditor/create test
helper and destroy() to locate the cleanup path, and ensure each test detaches
the Lexical root and then removes the DOM node from jsdom to avoid leaking state
between tests.
In
`@packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.ts`:
- Around line 133-146: The list item conversion in
convertLexicalNodeToMarkdownNode currently wraps each inline child from
$isListItemNode into its own paragraph, which breaks mixed inline content into
multiple blocks. Update the listItem handling to buffer consecutive inline
markdown nodes into a single paragraph and only flush/start a new block when a
non-inline child appears, so mixed content like text plus links stays in one
paragraph while nested lists still become separate blocks.
- Around line 150-157: The Link conversion in convertLexicalNodeToMarkdownNode
is using the wrong mdast field for the link title, so update the
$isLinkNode(node) branch to pass title from node.getTitle() instead of alt, and
make sure it uses a null fallback with title: node.getTitle() ?? null. Keep the
rest of the u('link', ...) mapping unchanged so the link children and URL
handling remain intact.
🪄 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: 53bac22c-e859-42e0-80a7-161cfdecee30
⛔ 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 (5)
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/__tests__/utils.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/convertLexicalNodeToMarkdownNode.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.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/plugins/Markdown/markdownParser.ts`:
- Around line 35-82: Do not inject blank-line paragraph nodes into list children
in remarkPreserveBlankLines, since the tree walk also visits list parents and
can turn list.children into an invalid mix of listItem and paragraph nodes.
Update the plugin to skip list nodes (or preserve spacing via
spread/listItem.spread instead) so markdownParser.ts keeps the mdast/Lexical
list shape intact during $convertFromMarkdownString.
🪄 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: 745bd7d4-ff17-43e9-aaea-bed0f550e36c
⛔ 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/convertLexicalNodeToMarkdownNode.tspackages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Fix #334
Summary by CodeRabbit