feat(parser): add openImpliesClose option to disable implicit HTML changes#2462
feat(parser): add openImpliesClose option to disable implicit HTML changes#2462Gravirei wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new optional ChangesopenImpliesClose Option
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/Parser.ts (1)
415-506: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test appears to cover
openImpliesClose: false.The existing "Implicit open p and br tags" and "Implicit close tags" cases in
src/Parser.events.spec.tsrun with defaults, so all four newly gated branches (form suppression,impliesClosemap,</p>implicit open,</br>implicit open) are only exercised in the enabled state. Adding a case with{ openImpliesClose: false }asserting the preserved structure would lock in the new contract.Do you want me to draft these test cases?
#!/bin/bash # Confirm no existing test references the new option. rg -nP --type=ts '\bopenImpliesClose\b' -g '*spec*' -g '*test*'🤖 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 `@src/Parser.ts` around lines 415 - 506, The parser branches in emitOpenTag and onclosetag are only tested with openImpliesClose enabled, so add coverage in Parser.events.spec.ts for the false case. Create a test that instantiates the parser with { openImpliesClose: false } and verifies the tag stack and emitted events stay unchanged for the affected paths (form suppression, implied-close mapping, and the implicit p/br close handling). Use the existing Parser, emitOpenTag, and onclosetag behavior as the reference points when asserting the preserved structure.
🤖 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 `@src/Parser.ts`:
- Around line 425-430: Rename the module-level Map currently called
openImpliesClose in Parser.ts to a clearer internal name such as
impliesCloseMap, and update the lookup in the Parser logic that uses this map so
it no longer reads ambiguously alongside the public this.openImpliesClose field.
Keep the public option/property name unchanged, and adjust any related
references or imports in the Parser class to use the new map identifier
consistently.
---
Outside diff comments:
In `@src/Parser.ts`:
- Around line 415-506: The parser branches in emitOpenTag and onclosetag are
only tested with openImpliesClose enabled, so add coverage in
Parser.events.spec.ts for the false case. Create a test that instantiates the
parser with { openImpliesClose: false } and verifies the tag stack and emitted
events stay unchanged for the affected paths (form suppression, implied-close
mapping, and the implicit p/br close handling). Use the existing Parser,
emitOpenTag, and onclosetag behavior as the reference points when asserting the
preserved structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…add tests for openImpliesClose:false
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Parser.ts (1)
425-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated
this.htmlMode && this.openImpliesClosegate — extract to a single field.The compound condition appears four times (Lines 425, 430, 496, 501). Since both fields are
readonlyand set once in the constructor, compute the combined flag once to reduce duplication and make future changes to the gating logic less error-prone.♻️ Proposed refactor
private readonly htmlMode: boolean; private readonly openImpliesClose: boolean; + private readonly impliesCloseEnabled: boolean; private readonly tokenizer: Tokenizer; ... this.openImpliesClose = options.openImpliesClose ?? this.htmlMode; + this.impliesCloseEnabled = this.htmlMode && this.openImpliesClose;Then replace each
this.htmlMode && this.openImpliesCloseoccurrence withthis.impliesCloseEnabled.Note: since
this.openImpliesClosealready defaults tothis.htmlModewhen unset, the extrathis.htmlMode &&guard only changes behavior when a caller explicitly setsopenImpliesClose: truetogether withxmlMode: true— in that case the option silently has no effect. Worth confirming this is intentional and documenting it on theParserOptions.openImpliesCloseJSDoc.Also applies to: 496-506
🤖 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 `@src/Parser.ts` around lines 425 - 436, The repeated html/open-implies-close gating in Parser should be centralized into a single computed field. Add a readonly parser flag (for example, `impliesCloseEnabled`) in `Parser`’s constructor and use it everywhere the compound `this.htmlMode && this.openImpliesClose` check appears, including the logic around the form guard and the implied-close map handling. Also review `ParserOptions.openImpliesClose` documentation to note that the extra HTML-mode gate makes the option ineffective in XML mode when explicitly enabled.
🤖 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.
Outside diff comments:
In `@src/Parser.ts`:
- Around line 425-436: The repeated html/open-implies-close gating in Parser
should be centralized into a single computed field. Add a readonly parser flag
(for example, `impliesCloseEnabled`) in `Parser`’s constructor and use it
everywhere the compound `this.htmlMode && this.openImpliesClose` check appears,
including the logic around the form guard and the implied-close map handling.
Also review `ParserOptions.openImpliesClose` documentation to note that the
extra HTML-mode gate makes the option ineffective in XML mode when explicitly
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14fb5a0d-eacf-45c4-a8d8-dd5633af758c
⛔ Files ignored due to path filters (1)
src/__snapshots__/Parser.events.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/Parser.events.spec.tssrc/Parser.ts
…sCloseEnabled field
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 `@src/Parser.ts`:
- Around line 295-297: The Parser class has a redundant `openImpliesClose` field
that is only used to initialize `impliesCloseEnabled`. Simplify the constructor
and class state by removing the intermediate `openImpliesClose` property and
deriving `impliesCloseEnabled` directly from the incoming options where it is
currently assigned, keeping the existing HTML-mode gating logic 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
…liesCloseEnabled directly
1 participant
Summary
Adds a new
openImpliesCloseoption toParserOptionsthat controls whether the parser performs implicit HTML tag open/close operations per the HTML5 spec. When set tofalse, the parser preserves the original document structure without adding or removing tags that weren't explicitly in the source.This is useful for users who want to parse and serialize HTML without having the parser alter invalid or non-standard markup.
Changes
openImpliesCloseoption toParserOptions(default:!xmlMode, i.e.truein HTML mode)openImpliesClosemap (e.g.<p>closing another open<p>)<form>suppression<p>open before</p>close<br>open before</br>closeRelated
Closes #2428
Summary by CodeRabbit
openImpliesClose).openImpliesClose: falseto confirm implicit close/open sequences are suppressed.