Repository navigation
fix(id-web): validate block/inline placement in prosemirror_to_markdown (#1043) - #1089
Conversation
prosemirror_to_markdown placed any known PM node type anywhere, so a
misplaced node (text/image/hard_break at doc level, a block inside a
paragraph) built an ill-formed comrak AST: the formatter panics in
debug builds and silently emits invalid markdown in release builds.
Add a PM-level placement walk (validate_placement) ahead of
json_to_ast: doc and block containers take blocks only, paragraph /
heading take inline only, lists/tables enforce their item/row/cell
structure, code_block takes text only. Violations return the new
ConversionError::MisplacedNode { parent, child } — the save path
already propagates ConversionError as a save failure. Unknown types
still fall through to UnknownType; footnote definitions and table
cells accept both shapes they legitimately carry (#945 / unwrapping).
Tests: test_misplaced_nodes_return_error_not_panic covers the five
verified panic shapes plus blockquote, table, and task_list cases,
unknown-type precedence, and a well-formed control. Verified with the
verbatim-codec probe crate: suite 71/71, all five shapes now Err.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 124700032d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "bullet_list" | "ordered_list" => child == Placement::ListItem, | ||
| "task_list" => child == Placement::TaskListItem, |
There was a problem hiding this comment.
For Markdown mixing checkbox and ordinary items, such as - [ ] todo\n- plain\n, this rejects the document produced by markdown_to_prosemirror itself. convert_node chooses the list type based only on the first child, while collect_list_items preserves both list_item and task_list_item children. Consequently, mixed lists in either order now return MisplacedNode instead of round-tripping successfully as before. Allow both item kinds under these list containers, or normalize the parser output before enforcing this restriction.
Useful? React with 👍 / 👎.
Fixes #1043 · Queue task 1232 (decision from triage task 1229)
Problem
json_to_astplaced any known PM node type anywhere. A misplaced node —text/image/hard_breakat doc level, a block inside aparagraph— produced an ill-formed comrak AST: the commonmark formatter panics in debug builds (comrak cm.rs InvalidChildType validation) and silently emits invalid markdown in release builds (validation isdebug_assertions-gated). All five shapes were reproduced against main in triage.Change (codec only,
pkgs/id/src/web/markdown.rs)validate_placementwalk over the PM document, run beforejson_to_ast: doc and block containers (blockquote,list_item,task_list_item) take blocks only;paragraph/headingtake inline only; lists enforce item types;table→table_row→ cells;code_blocktakes text only.ConversionError::MisplacedNode { parent, child }. The save path already propagatesConversionErroras a save failure (convert_doc_to_bytes, routes.rs:1986) — no caller changes.footnote_definitionaccept block or inline children (cell paragraph-unwrapping; id(web): footnotes are backslash-escaped on save — [^1] syntax mangled, definition orphaned #945 inline re-wrap); unknown node types fall through to the existingUnknownTypeerror, never masked by validation.Tests
New
test_misplaced_nodes_return_error_not_panic: the five verified panic shapes (text/image/hard_break at top level, paragraph-in-paragraph, heading-in-paragraph) plus inline-in-blockquote, paragraph-directly-in-table, and list_item-in-task_list — all must returnMisplacedNode; unknown-type precedence and a well-formed control asserted.Verified with the verbatim-codec probe crate (sandbox in-crate link OOMs, per prior tasks): suite 71/71, all five shapes now
Err, controls unchanged. Full CI is the remaining gate.Need help on this PR? Tag
@codesmith-botwith what you need.