Repository navigation
Conversation
… xmp Per the WHATWG tree construction rules for "in body", the start tags li, dd, dt, listing, plaintext and xmp close a <p> element in button scope, the same as the block elements already in openImpliesClose. htmlparser2 nested these elements inside the open <p> instead. This also fixes list items that contain a paragraph: `<ul><li><p>a<li>b</ul>` previously nested the second <li> inside the first <p>; it now closes the <p> and the previous <li>, as browsers do.
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main parser fix. It identifies the affected HTML tags and matches the changeset. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks each tag in line
As paragraphs close at the proper time
List items follow, events in view
Description terms close neatly too
The parser hops through markup anew
Comment @coderabbitai help to get the list of available commands.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/Parser.ts">
<violation number="1" location="src/Parser.ts:26">
P2: This is a top-of-stack check, not a button-scope check: `<p><span>x<li>` still nests the list item under the paragraph, while `<button><p>x<li>` closes a paragraph outside button scope. Use a scope-aware close routine for these start tags.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| ["body", new Set(["head", "link", "script"])], | ||
| ["a", new Set(["a"])], | ||
| ["li", new Set(["li"])], | ||
| ["li", new Set(["li", "p"])], |
There was a problem hiding this comment.
P2: This is a top-of-stack check, not a button-scope check: <p><span>x<li> still nests the list item under the paragraph, while <button><p>x<li> closes a paragraph outside button scope. Use a scope-aware close routine for these start tags.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/Parser.ts, line 26:
<comment>This is a top-of-stack check, not a button-scope check: `<p><span>x<li>` still nests the list item under the paragraph, while `<button><p>x<li>` closes a paragraph outside button scope. Use a scope-aware close routine for these start tags.</comment>
<file context>
@@ -23,7 +23,7 @@ const openImpliesClose = new Map<string, Set<string>>([
["body", new Set(["head", "link", "script"])],
["a", new Set(["a"])],
- ["li", new Set(["li"])],
+ ["li", new Set(["li", "p"])],
["p", pTag],
["h1", headingTags],
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
 [Medium impact] The PR appears safe to merge.
|
Follow-up to #2493. In the WHATWG "in body" insertion mode, the
li,dd,dt,listing,plaintextandxmpstart tags also close a<p>in button scope (spec). They were still missing fromopenImpliesClose, so htmlparser2 nested them inside the open<p>.This matters most for list items that contain a paragraph:
<ul><li><p>a<li>b</ul><ul><li><p>a<li>b</li></p></li></ul><ul><li><p>a</p></li><li>b</li></ul><dl><dt><p>a<dd>b</dl><dl><dt><p>a<dd>b</dd></p></dt></dl><dl><dt><p>a</p></dt><dd>b</dd></dl><p>a<xmp>b</xmp><p>a<xmp>b</xmp></p><p>a</p><xmp>b</xmp>Changes:
pto the sets thatli,ddanddtclose, so a<p>on top of the stack is closed first and the previous list item is closed after it;listing,plaintextandxmpwithpTag.Unchanged:
<div><li>and<p><button><li>(the<p>is not in button scope there).Tests: added cases to
src/Parser.spec.tsthat check the order of open and close events.The suite was run:
npx vitest run: 208 passed. Withsrc/Parser.tsreverted tomain, the 7 new tests fail with assertion errors such asexpected [ '<p>', '<li>', '</li>', '</p>' ] to deeply equal [ '<p>', '</p>', '<li>', '</li>' ].npm run lint:es,npm run lint:ts,npm run build: pass.biome check src package.json README.md biome.json: clean. A plainbiome check .processed no files in the local checkout because its path contains a dot-directory, which the!**/.*include pattern excludes.Not run: tests in downstream packages (domhandler, cheerio).
Summary by CodeRabbit
<li>,<dd>,<dt>,<listing>,<plaintext>, and<xmp>elements.<li>or<dt>is now closed first, keeping tag events in the expected order.