fix(mdxish): preserve <style> blocks, style objects, and .map() JSX carried over from MDX#1532
Conversation
carried over from MDX
…style evaluation - Make brace-depth tracking in protectNestedHtmlBlankLines quote-aware so a literal brace inside a string/template literal (e.g. a URL or JSON example) no longer desyncs depth tracking and silently leaves a blank line unprotected - Generalize tag-depth tracking to count opens/closes anywhere on a line instead of requiring one tag alone per line, so compacted JSX (e.g. `<div><span>a</span>` sharing a line) is tracked correctly - Use the canonical evalExpression() helper in evaluateStyleBlockExpressions instead of the lower-level evaluate(), matching every other mdxish expression-evaluation call site Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@coderabbitai review full |
|
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR adds new mdxish transforms and helpers: Sequence Diagram(s)sequenceDiagram
participant mdxish
participant protectNestedHtmlBlankLines
participant evaluateStyleBlockExpressions
participant resolveDeferredAttributeExpressionProps
participant styleObjectToCssText
mdxish->>protectNestedHtmlBlankLines: preprocess raw content
protectNestedHtmlBlankLines-->>mdxish: normalized content
mdxish->>evaluateStyleBlockExpressions: evaluate style block expressions (safeMode off)
evaluateStyleBlockExpressions-->>mdxish: rewritten style html node
mdxish->>resolveDeferredAttributeExpressionProps: resolve deferred attribute expressions
resolveDeferredAttributeExpressionProps->>styleObjectToCssText: serialize style object (if plain object)
styleObjectToCssText-->>resolveDeferredAttributeExpressionProps: CSS text
resolveDeferredAttributeExpressionProps-->>mdxish: node.properties updated
Related PRs: None identified. Suggested labels: enhancement, bug Suggested reviewers: None identified. Compact metadata:
🐰 A rabbit hops through braces and style, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/transformers/protect-nested-html-blank-lines.test.ts (1)
1-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for self-closing block tags and tag-like text mid-line.
Given the bugs identified in
protect-nested-html-blank-lines.ts(missing^anchor and no self-closing exclusion inSTANDALONE_OPEN_TAG_RE), consider adding cases such as:
- A standalone self-closing block tag (e.g.
<div className="icon" />) followed by unrelated later content with blank lines, assertingactiveTagdoesn't leak past it.- A prose line containing
<div>mid-sentence (not standalone), asserting it isn't treated as an open tag.🤖 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 `@__tests__/transformers/protect-nested-html-blank-lines.test.ts` around lines 1 - 85, Add regression tests for protectNestedHtmlBlankLines to cover two parsing edge cases: a self-closing standalone block tag like a <div /> followed by later blank lines, and prose containing tag-like text such as <div> mid-sentence. Update the tests around protectNestedHtmlBlankLines so they verify activeTag does not leak past self-closing tags and that STANDALONE_OPEN_TAG_RE only matches real standalone opening tags, not inline tag-like text.
🤖 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 `@processor/transform/mdxish/components/mdx-blocks.ts`:
- Around line 12-14: The nested-attribute detection in mdx-blocks is too naive
because NESTED_ATTR_EXPRESSION_RE.test(contentAfterTag) scans raw text and can
treat JSX-like examples inside code or strings as real attributes. Update the
logic around the nested-attribute check in mdx-blocks.ts to protect
fenced/inline code before evaluating the regex, following the same pattern used
by protect-nested-html-blank-lines.ts. Make sure the code path that decides
whether a <div> block is promoted into JSX/expression processing only sees
unprotected content, so literal docs examples like <Foo key={i} /> stay as
HTML/text.
In `@processor/transform/mdxish/protect-nested-html-blank-lines.ts`:
- Line 14: The STANDALONE_OPEN_TAG_RE in protectNestedHtmlBlankLines is too
permissive: it can match tag text inside prose and it incorrectly treats
self-closing tags as standalone open tags. Update the regex in
protect-nested-html-blank-lines.ts to require a line start anchor and to reject
self-closing forms, then verify the behavior in protectNestedHtmlBlankLines so
only true standalone opening tags set activeTag/depth. Also add or update tests
in protect-nested-html-blank-lines.test.ts for prose lines and self-closing tags
to prevent regressions.
---
Nitpick comments:
In `@__tests__/transformers/protect-nested-html-blank-lines.test.ts`:
- Around line 1-85: Add regression tests for protectNestedHtmlBlankLines to
cover two parsing edge cases: a self-closing standalone block tag like a <div />
followed by later blank lines, and prose containing tag-like text such as <div>
mid-sentence. Update the tests around protectNestedHtmlBlankLines so they verify
activeTag does not leak past self-closing tags and that STANDALONE_OPEN_TAG_RE
only matches real standalone opening tags, not inline tag-like text.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e29cf1c-5d6b-45bc-981c-d4bfcd22090d
📒 Files selected for processing (8)
__tests__/lib/mdxish/cx-3646-style-and-jsx-expressions.test.ts__tests__/transformers/protect-nested-html-blank-lines.test.tslib/mdxish.tsprocessor/transform/mdxish/components/mdx-blocks.tsprocessor/transform/mdxish/evaluate-expressions.tsprocessor/transform/mdxish/evaluate-style-block-expressions.tsprocessor/transform/mdxish/protect-nested-html-blank-lines.tsprocessor/transform/mdxish/resolve-deferred-attribute-expression-props.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
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)
processor/transform/mdxish/protect-nested-html-blank-lines.ts (1)
143-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep blank lines inside template literals
Atprocessor/transform/mdxish/protect-nested-html-blank-lines.ts:143-149, dropping blank lines wheneverbraceDepth > 0also rewrites multiline backtick strings inside JSX expressions, changing content like<style>{...}</style>. Only strip these blanks whenquoteChar === null, and add a regression test for a blank line inside a template literal.🤖 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 `@processor/transform/mdxish/protect-nested-html-blank-lines.ts` around lines 143 - 149, The blank-line stripping logic in protectNestedHtmlBlankLines is too broad because it removes empty lines whenever braceDepth > 0, which also affects template literals inside JSX expressions. Update the branch in the main scan loop to only splice blank lines when quoteChar is null so multiline backtick strings (for example in style/content expressions) keep their internal blank lines, and add a regression test covering a blank line inside a template literal to verify the behavior.
🧹 Nitpick comments (2)
__tests__/transformers/protect-nested-html-blank-lines.test.ts (2)
86-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM, with room for one more scenario.
Correctly validates that tag-depth tracking now accounts for content sharing a line with the opening tag. For fuller coverage of the "detect HTML tags anywhere on a line" fix mentioned in the commit message, consider also adding a case with two same-name tags fully on one line (e.g.
<div>...</div><div>...</div>), which more directly exercises the multi-open/close-per-line depth counting rather than a single wrapper tag sharing a line with differently-named nested tags.🤖 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 `@__tests__/transformers/protect-nested-html-blank-lines.test.ts` around lines 86 - 95, Add one more test in protect-nested-html-blank-lines.test.ts to cover multiple same-name tags on a single line, such as two div blocks written back-to-back, so the protectNestedHtmlBlankLines behavior exercises same-tag open/close depth counting on one line. Keep the existing wrapper-tag scenario, but extend the test coverage in the protectNestedHtmlBlankLines suite to verify the blank-line protection still works when multiple matching tags share a line.
97-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen assertions and add a template-literal case.
Two gaps relative to the commit's stated intent ("quote-aware so braces inside strings or template literals do not break brace-depth tracking"):
- This test only exercises a double-quoted string; template literals (backticks), which can also embed
${...}expressions with their own braces, aren't covered despite being explicitly named in the fix.- Unlike the analogous test at Line 46 (
expect(result).not.toContain('<!---->')), this test doesn't assert that no marker was inserted — it only checksnot.toContain('\n\n'). Adding that assertion (and ideally a fulltoBe(...)equality check like the test at Lines 40-45) would give stronger confidence that the blank line was deleted rather than replaced.Suggested additions
const result = protectNestedHtmlBlankLines(input); expect(result).not.toContain('\n\n'); + expect(result).not.toContain('<!---->'); + }); + + it('does not desync brace depth when a template literal contains a stray brace', () => { + const input = `<div className="grid"> + {(() => { + const s = \`}}\`; + return 1; + + return 2; + })()} +</div>`; + + const result = protectNestedHtmlBlankLines(input); + + expect(result).not.toContain('\n\n'); + expect(result).not.toContain('<!---->'); });🤖 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 `@__tests__/transformers/protect-nested-html-blank-lines.test.ts` around lines 97 - 110, Strengthen the protectNestedHtmlBlankLines test by expanding the existing brace-depth case in protect-nested-html-blank-lines.test.ts to also cover a template literal with embedded ${...} braces, since the fix is meant to be quote-aware for both strings and backticks. In the current test near the existing protectNestedHtmlBlankLines call, add an assertion that the output does not contain the marker inserted by the transformer (matching the pattern used in the other tests), and prefer a full expected-output equality check if practical so the test verifies the blank line was removed rather than replaced.
🤖 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 `@processor/transform/mdxish/protect-nested-html-blank-lines.ts`:
- Around line 127-137: The same-line handling in
protect-nested-html-blank-lines.ts only updates tag nesting via tagDepthDelta,
but it does not carry forward brace state for trailing opening-tag content, so
expressions like `<div>{items.map(...)` can lose track of JSX braces and insert
`<!---->` into JavaScript. Update the activeTag initialization path to also scan
the remainder of the line after OPEN_TAG_LEADING_RE for `{` and `}` and adjust
braceDepth accordingly, alongside the existing depth logic, so later blank lines
inside the expression are treated as JS content rather than HTML sibling gaps.
---
Outside diff comments:
In `@processor/transform/mdxish/protect-nested-html-blank-lines.ts`:
- Around line 143-149: The blank-line stripping logic in
protectNestedHtmlBlankLines is too broad because it removes empty lines whenever
braceDepth > 0, which also affects template literals inside JSX expressions.
Update the branch in the main scan loop to only splice blank lines when
quoteChar is null so multiline backtick strings (for example in style/content
expressions) keep their internal blank lines, and add a regression test covering
a blank line inside a template literal to verify the behavior.
---
Nitpick comments:
In `@__tests__/transformers/protect-nested-html-blank-lines.test.ts`:
- Around line 86-95: Add one more test in
protect-nested-html-blank-lines.test.ts to cover multiple same-name tags on a
single line, such as two div blocks written back-to-back, so the
protectNestedHtmlBlankLines behavior exercises same-tag open/close depth
counting on one line. Keep the existing wrapper-tag scenario, but extend the
test coverage in the protectNestedHtmlBlankLines suite to verify the blank-line
protection still works when multiple matching tags share a line.
- Around line 97-110: Strengthen the protectNestedHtmlBlankLines test by
expanding the existing brace-depth case in
protect-nested-html-blank-lines.test.ts to also cover a template literal with
embedded ${...} braces, since the fix is meant to be quote-aware for both
strings and backticks. In the current test near the existing
protectNestedHtmlBlankLines call, add an assertion that the output does not
contain the marker inserted by the transformer (matching the pattern used in the
other tests), and prefer a full expected-output equality check if practical so
the test verifies the blank line was removed rather than replaced.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c2da092-2da4-4db6-b1bc-5e05c3d3ce66
📒 Files selected for processing (3)
__tests__/transformers/protect-nested-html-blank-lines.test.tsprocessor/transform/mdxish/evaluate-style-block-expressions.tsprocessor/transform/mdxish/protect-nested-html-blank-lines.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- processor/transform/mdxish/evaluate-style-block-expressions.ts
| if (!activeTag) { | ||
| const openMatch = line.match(OPEN_TAG_LEADING_RE); | ||
| if (openMatch && BLOCK_TAG_NAMES.has(openMatch[1].toLowerCase())) { | ||
| activeTag = openMatch[1]; | ||
| depth = 1; | ||
| braceDepth = 0; | ||
| quoteChar = null; | ||
| // Account for any further opens/closes of this tag later on the same line (e.g. a | ||
| // compacted `<div><div>` or a fully self-contained `<div>...</div>` one-liner). | ||
| depth += tagDepthDelta(line.slice(openMatch[0].length), activeTag); | ||
| if (depth <= 0) activeTag = null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Track brace state in trailing opening-tag content.
Line 136 accounts for same-line tag depth, but same-line expressions like <div>{items.map(...) never update braceDepth. A later blank line inside that JSX expression can be treated as a sibling gap and get <!----> inserted into JS syntax.
🐛 Proposed fix
- depth += tagDepthDelta(line.slice(openMatch[0].length), activeTag);
- if (depth <= 0) activeTag = null;
+ const remainder = line.slice(openMatch[0].length);
+ const brace = braceDeltaAndQuote(remainder, quoteChar);
+ quoteChar = brace.quoteChar;
+ braceDepth = Math.max(0, braceDepth + brace.delta);
+ depth += tagDepthDelta(remainder, activeTag);
+ if (depth <= 0) {
+ activeTag = null;
+ braceDepth = 0;
+ quoteChar = null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!activeTag) { | |
| const openMatch = line.match(OPEN_TAG_LEADING_RE); | |
| if (openMatch && BLOCK_TAG_NAMES.has(openMatch[1].toLowerCase())) { | |
| activeTag = openMatch[1]; | |
| depth = 1; | |
| braceDepth = 0; | |
| quoteChar = null; | |
| // Account for any further opens/closes of this tag later on the same line (e.g. a | |
| // compacted `<div><div>` or a fully self-contained `<div>...</div>` one-liner). | |
| depth += tagDepthDelta(line.slice(openMatch[0].length), activeTag); | |
| if (depth <= 0) activeTag = null; | |
| if (!activeTag) { | |
| const openMatch = line.match(OPEN_TAG_LEADING_RE); | |
| if (openMatch && BLOCK_TAG_NAMES.has(openMatch[1].toLowerCase())) { | |
| activeTag = openMatch[1]; | |
| depth = 1; | |
| braceDepth = 0; | |
| quoteChar = null; | |
| // Account for any further opens/closes of this tag later on the same line (e.g. a | |
| // compacted `<div><div>` or a fully self-contained `<div>...</div>` one-liner). | |
| const remainder = line.slice(openMatch[0].length); | |
| const brace = braceDeltaAndQuote(remainder, quoteChar); | |
| quoteChar = brace.quoteChar; | |
| braceDepth = Math.max(0, braceDepth + brace.delta); | |
| depth += tagDepthDelta(remainder, activeTag); | |
| if (depth <= 0) { | |
| activeTag = null; | |
| braceDepth = 0; | |
| quoteChar = null; | |
| } |
🤖 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 `@processor/transform/mdxish/protect-nested-html-blank-lines.ts` around lines
127 - 137, The same-line handling in protect-nested-html-blank-lines.ts only
updates tag nesting via tagDepthDelta, but it does not carry forward brace state
for trailing opening-tag content, so expressions like `<div>{items.map(...)` can
lose track of JSX braces and insert `<!---->` into JavaScript. Update the
activeTag initialization path to also scan the remainder of the line after
OPEN_TAG_LEADING_RE for `{` and `}` and adjust braceDepth accordingly, alongside
the existing depth logic, so later blank lines inside the expression are treated
as JS content rather than HTML sibling gaps.
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
processor/transform/mdxish/protect-nested-html-blank-lines.ts (1)
127-141: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame-line brace/quote state after the opening tag is still not tracked (unresolved from a prior review round).
This branch computes
tagDepthDeltafor the remainder of the line but resetsbraceDepth/quoteCharto0/nullunconditionally — it never scans the remainder withbraceDeltaAndQuote. For a wrapper whose opening tag shares a line with the start of a brace expression, e.g.<div>{items.map(item => {, the two open braces are lost. A blank line right after is then evaluated withbraceDepth === 0, so it's treated as a JSX-sibling gap instead of "inside an open expression" — iflastNonBlankTrimmeddoesn't happen to end with>, the blank line is left completely unprotected.This matches a gap flagged by coderabbitai in a previous review round ("Track brace state in trailing opening-tag content") that doesn't appear to have a corresponding "Fixed" confirmation, and the code still lacks the fix.
🐛 Proposed fix
- activeTag = openMatch[1]; - depth = 1; - braceDepth = 0; - quoteChar = null; - // Account for any further opens/closes of this tag later on the same line (e.g. a - // compacted `<div><div>` or a fully self-contained `<div>...</div>` one-liner). - depth += tagDepthDelta(line.slice(openMatch[0].length), activeTag); - if (depth <= 0) activeTag = null; + activeTag = openMatch[1]; + depth = 1; + const remainder = line.slice(openMatch[0].length); + const brace = braceDeltaAndQuote(remainder, null); + quoteChar = brace.quoteChar; + braceDepth = Math.max(0, brace.delta); + // Account for any further opens/closes of this tag later on the same line (e.g. a + // compacted `<div><div>` or a fully self-contained `<div>...</div>` one-liner). + depth += tagDepthDelta(remainder, activeTag); + if (depth <= 0) { + activeTag = null; + braceDepth = 0; + quoteChar = null; + }🤖 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 `@processor/transform/mdxish/protect-nested-html-blank-lines.ts` around lines 127 - 141, The same-line state after an opening block tag is still not carried forward in protect-nested-html-blank-lines.ts. In the branch that initializes activeTag, depth, braceDepth, and quoteChar, also scan the remainder of the line after the opening tag using the existing brace/quote tracking helper so any trailing "{", "}", and quote state are preserved. Update the activeTag setup logic to combine the tag-depth handling with the trailing expression state instead of resetting braceDepth and quoteChar unconditionally.
🧹 Nitpick comments (1)
__tests__/transformers/protect-nested-html-blank-lines.test.ts (1)
1-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for two gaps still present in
protectNestedHtmlBlankLines.Good coverage of the previously-fixed brace/quote-desync and same-line-tag cases. Two related gaps found in
protect-nested-html-blank-lines.tsaren't yet covered:
- A blank line following an expression that opens on the same line as the wrapper's opening tag (e.g.
<div>{items.map(item => {) — braces opened there aren't tracked.- A tag-like string inside an expression for the active tag name (e.g.
{"</div>"}) desyncingtagDepthDeltathe same way stray braces in strings desyncedbraceDeltaAndQuote(test at lines 97-110 covers only the brace case).Adding tests for both alongside the existing suite would lock in fixes for the issues raised in the main file review.
🤖 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 `@__tests__/transformers/protect-nested-html-blank-lines.test.ts` around lines 1 - 111, Add regression tests in protectNestedHtmlBlankLines for two missing edge cases: a blank line after an expression that opens on the same line as the wrapper tag, and a tag-like string inside an expression that can desync tagDepthDelta. Extend the protectNestedHtmlBlankLines suite with cases using the existing function name so it verifies same-line tag/expression openings are tracked correctly and that strings like "</div>" inside braces do not corrupt tag-depth handling; assert the blank line is protected or removed as appropriate without breaking nested HTML processing.
🤖 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 `@processor/transform/mdxish/protect-nested-html-blank-lines.ts`:
- Around line 23-27: `tagDepthDelta` currently scans raw lines, so tag-like text
inside quoted/template literal content can incorrectly change nesting depth and
clear `activeTag` too early. Update the `protect-nested-html-blank-lines`
transform to make `tagDepthDelta` reuse the same quote-aware parsing approach as
`braceDeltaAndQuote`, ignoring `<tag>` and `</tag>` sequences that appear inside
strings/expressions. Add a regression test near the existing stray-brace case
covering a stray closing tag inside a string (for example in a wrapped `<div>`
expression) to verify blank lines still get protected until the real wrapper
closes.
---
Duplicate comments:
In `@processor/transform/mdxish/protect-nested-html-blank-lines.ts`:
- Around line 127-141: The same-line state after an opening block tag is still
not carried forward in protect-nested-html-blank-lines.ts. In the branch that
initializes activeTag, depth, braceDepth, and quoteChar, also scan the remainder
of the line after the opening tag using the existing brace/quote tracking helper
so any trailing "{", "}", and quote state are preserved. Update the activeTag
setup logic to combine the tag-depth handling with the trailing expression state
instead of resetting braceDepth and quoteChar unconditionally.
---
Nitpick comments:
In `@__tests__/transformers/protect-nested-html-blank-lines.test.ts`:
- Around line 1-111: Add regression tests in protectNestedHtmlBlankLines for two
missing edge cases: a blank line after an expression that opens on the same line
as the wrapper tag, and a tag-like string inside an expression that can desync
tagDepthDelta. Extend the protectNestedHtmlBlankLines suite with cases using the
existing function name so it verifies same-line tag/expression openings are
tracked correctly and that strings like "</div>" inside braces do not corrupt
tag-depth handling; assert the blank line is protected or removed as appropriate
without breaking nested HTML processing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ece3b770-0b21-46ea-b881-2523a2d844cb
📒 Files selected for processing (8)
__tests__/lib/mdxish/cx-3646-style-and-jsx-expressions.test.ts__tests__/transformers/protect-nested-html-blank-lines.test.tslib/mdxish.tsprocessor/transform/mdxish/components/mdx-blocks.tsprocessor/transform/mdxish/evaluate-expressions.tsprocessor/transform/mdxish/evaluate-style-block-expressions.tsprocessor/transform/mdxish/protect-nested-html-blank-lines.tsprocessor/transform/mdxish/resolve-deferred-attribute-expression-props.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
processor/transform/mdxish/protect-nested-html-blank-lines.ts (2)
127-141: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrailing brace state on the opening-tag line still isn't tracked.
Same gap flagged in a prior round on this segment (127-137) with no follow-up "Fixed in ..." reply. When
activeTagis first set, onlytagDepthDeltais applied to the same-line remainder (line 136) —braceDeltaAndQuoteis never called against it. A same-line expression like<div>{items.map(item => {never updatesbraceDepth/quoteChar, so a later blank line inside that JSX expression body is treated as a sibling gap and gets<!---->inserted into JS syntax instead of being safely dropped.🐛 Suggested fix
if (openMatch && BLOCK_TAG_NAMES.has(openMatch[1].toLowerCase())) { activeTag = openMatch[1]; depth = 1; braceDepth = 0; quoteChar = null; // Account for any further opens/closes of this tag later on the same line (e.g. a // compacted `<div><div>` or a fully self-contained `<div>...</div>` one-liner). - depth += tagDepthDelta(line.slice(openMatch[0].length), activeTag); - if (depth <= 0) activeTag = null; + const remainder = line.slice(openMatch[0].length); + const brace = braceDeltaAndQuote(remainder, quoteChar); + quoteChar = brace.quoteChar; + braceDepth = Math.max(0, braceDepth + brace.delta); + depth += tagDepthDelta(remainder, activeTag); + if (depth <= 0) { + activeTag = null; + braceDepth = 0; + quoteChar = null; + } }🤖 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 `@processor/transform/mdxish/protect-nested-html-blank-lines.ts` around lines 127 - 141, Same-line JSX expression state is not being tracked when a block tag first becomes active, so blank-line protection can misclassify later lines. In protect-nested-html-blank-lines.ts, update the activeTag initialization path to also run the remaining part of the opening-tag line through the brace/quote parser used elsewhere, alongside tagDepthDelta, so braceDepth and quoteChar are initialized correctly for cases like `<div>{items.map(item => {`. Use the existing helpers in this flow, especially activeTag, tagDepthDelta, and braceDeltaAndQuote, to keep same-line nested JS/JSX state consistent.
23-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
tagDepthDeltastill not quote-aware — tag-like text inside strings can desync nesting depth.This was already flagged in a prior review round on this exact function (lines 23-27) without a corresponding "Fixed in ..." resolution reply (unlike the sibling fixes for
STANDALONE_OPEN_TAG_REandbraceDeltaAndQuote). The regex still scans the raw line unconditionally, so a wrapped expression containing tag-like text for the active tag (e.g.{"</div>"}whileactiveTag === 'div') will prematurely zerodepthand clearactiveTag, silently un-protecting a real blank line later in the document — reproducing the exact bug this file exists to fix.🐛 Suggested fix (reuse quote-tracking from braceDeltaAndQuote)
-function tagDepthDelta(line: string, tag: string): number { - const opens = line.match(new RegExp(`<${tag}(?:\\s[^<>]*)?(?<!/)>`, 'g'))?.length ?? 0; - const closes = line.match(new RegExp(`</${tag}>`, 'g'))?.length ?? 0; - return opens - closes; -} +function stripQuotedSegments(line: string, quoteChar: string | null): string { + let out = ''; + let quote = quoteChar; + for (let i = 0; i < line.length; i += 1) { + const char = line[i]; + if (quote) { + if (char === '\\') i += 1; + else if (char === quote) quote = null; + continue; // eslint-disable-line no-continue + } + if (char === '"' || char === "'" || char === '`') quote = char; + else out += char; + } + return out; +} + +function tagDepthDelta(line: string, tag: string, quoteChar: string | null = null): number { + const sanitized = stripQuotedSegments(line, quoteChar); + const opens = sanitized.match(new RegExp(`<${tag}(?:\\s[^<>]*)?(?<!/)>`, 'g'))?.length ?? 0; + const closes = sanitized.match(new RegExp(`</${tag}>`, 'g'))?.length ?? 0; + return opens - closes; +}The static-analysis
regexp-from-variablewarning on thenew RegExpcalls at lines 24-25 is a low-risk false positive here (tagis bounded to[a-z][\w-]*captured fromOPEN_TAG_LEADING_RE, not arbitrary attacker input of unbounded structure), so no ReDoS action needed there.🤖 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 `@processor/transform/mdxish/protect-nested-html-blank-lines.ts` around lines 23 - 27, `tagDepthDelta` is still scanning raw lines and can miscount when tag-like text appears inside quoted strings, which can desync `depth` and `activeTag`. Update the `tagDepthDelta` logic in `protect-nested-html-blank-lines` to be quote-aware by reusing the same quote-tracking approach already used in `braceDeltaAndQuote`, so `<tag>` and `</tag>` are only counted when they are not inside strings. Keep the existing `tagDepthDelta`/`braceDeltaAndQuote` flow intact and ensure wrapped expressions like `{"</div>"}` do not affect nesting.
🧹 Nitpick comments (2)
processor/transform/mdxish/resolve-deferred-attribute-expression-props.ts (1)
20-21: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
isPlainObjecttreats any non-array, non-element object as a plain object.
Date,Map,RegExp, or class instances passed asstylewould pass this check and silently produce an empty/garbage CSS string viaObject.entries. Low practical likelihood for astyleprop specifically, so not blocking, but worth a tighter check (e.g.Object.getPrototypeOf(value) === Object.prototype) if this is reused for other props later.🤖 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 `@processor/transform/mdxish/resolve-deferred-attribute-expression-props.ts` around lines 20 - 21, The isPlainObject helper in resolveDeferredAttributeExpressionProps is too permissive because it accepts any non-array, non-React object, including Date, Map, RegExp, and class instances. Tighten the check in isPlainObject to only accept true plain objects (for example by verifying the prototype is Object.prototype or null) so downstream Object.entries usage only runs on valid object literals.__tests__/transformers/protect-nested-html-blank-lines.test.ts (1)
1-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage for the fixed bugs, but two previously-fixed regressions aren't locked in by a test here.
The prior review round's two "Fixed in 840938e" items — (1) a standalone self-closing block tag (e.g.
<div className="icon" />) incorrectly settingactiveTagwith no matching close, and (2) a tag mention inside prose (e.g."See the <div> element") incorrectly matchingOPEN_TAG_LEADING_RE— don't appear to have corresponding test cases in this file. Since the underlying regex (OPEN_TAG_LEADING_RE) is exactly what was patched for both, adding two small cases would guard against future regressions on this specific fix.it('does not treat a standalone self-closing tag as an unclosed wrapper', () => { const input = `<div className="icon" /> More text. <div className="icon" />`; expect(protectNestedHtmlBlankLines(input)).toBe(input); }); it('does not match a tag mentioned in prose text', () => { const input = `See the <div> element. More text.`; expect(protectNestedHtmlBlankLines(input)).toBe(input); });🤖 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 `@__tests__/transformers/protect-nested-html-blank-lines.test.ts` around lines 1 - 111, Add two regression tests to protectNestedHtmlBlankLines in the test suite: one for a standalone self-closing wrapper like <div className="icon" /> so it does not leave activeTag open or alter surrounding blank lines, and one for prose containing a literal tag mention like “See the <div> element” so OPEN_TAG_LEADING_RE does not treat it as markup. Place them alongside the existing protectNestedHtmlBlankLines cases and assert the input is preserved unchanged.
🤖 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 `@processor/transform/mdxish/resolve-deferred-attribute-expression-props.ts`:
- Around line 11-28: The `styleObjectToCssText` helper in
`resolve-deferred-attribute-expression-props.ts` currently serializes numeric
style values without CSS units, so dimensional properties like `fontSize` become
invalid; update `styleObjectToCssText` (and any supporting helpers such as
`cssPropertyName`/`isPlainObject` if needed) to append `px` for bare numeric
values on CSS length-like properties while leaving unitless or already-string
values unchanged. Also adjust the relevant regression test to assert that
`fontSize: 12` serializes to `font-size:12px`.
---
Duplicate comments:
In `@processor/transform/mdxish/protect-nested-html-blank-lines.ts`:
- Around line 127-141: Same-line JSX expression state is not being tracked when
a block tag first becomes active, so blank-line protection can misclassify later
lines. In protect-nested-html-blank-lines.ts, update the activeTag
initialization path to also run the remaining part of the opening-tag line
through the brace/quote parser used elsewhere, alongside tagDepthDelta, so
braceDepth and quoteChar are initialized correctly for cases like
`<div>{items.map(item => {`. Use the existing helpers in this flow, especially
activeTag, tagDepthDelta, and braceDeltaAndQuote, to keep same-line nested
JS/JSX state consistent.
- Around line 23-27: `tagDepthDelta` is still scanning raw lines and can
miscount when tag-like text appears inside quoted strings, which can desync
`depth` and `activeTag`. Update the `tagDepthDelta` logic in
`protect-nested-html-blank-lines` to be quote-aware by reusing the same
quote-tracking approach already used in `braceDeltaAndQuote`, so `<tag>` and
`</tag>` are only counted when they are not inside strings. Keep the existing
`tagDepthDelta`/`braceDeltaAndQuote` flow intact and ensure wrapped expressions
like `{"</div>"}` do not affect nesting.
---
Nitpick comments:
In `@__tests__/transformers/protect-nested-html-blank-lines.test.ts`:
- Around line 1-111: Add two regression tests to protectNestedHtmlBlankLines in
the test suite: one for a standalone self-closing wrapper like <div
className="icon" /> so it does not leave activeTag open or alter surrounding
blank lines, and one for prose containing a literal tag mention like “See the
<div> element” so OPEN_TAG_LEADING_RE does not treat it as markup. Place them
alongside the existing protectNestedHtmlBlankLines cases and assert the input is
preserved unchanged.
In `@processor/transform/mdxish/resolve-deferred-attribute-expression-props.ts`:
- Around line 20-21: The isPlainObject helper in
resolveDeferredAttributeExpressionProps is too permissive because it accepts any
non-array, non-React object, including Date, Map, RegExp, and class instances.
Tighten the check in isPlainObject to only accept true plain objects (for
example by verifying the prototype is Object.prototype or null) so downstream
Object.entries usage only runs on valid object literals.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ece3b770-0b21-46ea-b881-2523a2d844cb
📒 Files selected for processing (8)
__tests__/lib/mdxish/cx-3646-style-and-jsx-expressions.test.ts__tests__/transformers/protect-nested-html-blank-lines.test.tslib/mdxish.tsprocessor/transform/mdxish/components/mdx-blocks.tsprocessor/transform/mdxish/evaluate-expressions.tsprocessor/transform/mdxish/evaluate-style-block-expressions.tsprocessor/transform/mdxish/protect-nested-html-blank-lines.tsprocessor/transform/mdxish/resolve-deferred-attribute-expression-props.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
| /** Convert a camelCase (or CSS custom property) key into its kebab-case CSS name. */ | ||
| const cssPropertyName = (key: string): string => (key.startsWith('--') ? key : key.replace(/[A-Z]/g, m => `-${m.toLowerCase()}`)); | ||
|
|
||
| /** | ||
| * Serialize a React-style inline style object (`{ color: "red", fontSize: 12 }`) into a | ||
| * CSS declaration string (`color:red;font-size:12`), the shape hast/HTML expects for a | ||
| * `style` attribute. Non-plain-object values (already a string, `undefined`, etc.) pass through | ||
| * unchanged so only the object case introduced by evaluated `style={{...}}` expressions is affected. | ||
| */ | ||
| const isPlainObject = (value: unknown): value is Record<string, unknown> => | ||
| typeof value === 'object' && value !== null && !Array.isArray(value) && !React.isValidElement(value); | ||
|
|
||
| const styleObjectToCssText = (style: Record<string, unknown>): string => | ||
| Object.entries(style) | ||
| .filter(([, value]) => value !== undefined && value !== null && value !== '') | ||
| .map(([key, value]) => `${cssPropertyName(key)}:${value}`) | ||
| .join(';'); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does React append px to all numeric CSS style values except a specific unitless list, and what's the current unitless list in React 18?
💡 Result:
Yes, React automatically appends the string "px" to numeric values provided in the style prop, unless the property is explicitly included in a specific unitless list [1][2]. If you provide a value as a string (e.g., style={{ width: '200px' }} or style={{ width: '200%' }}), React will not append "px" [3]. Regarding the unitless list, while React historically maintained a hardcoded whitelist of unitless properties in its source code (such as in CSSProperty.js) [4][5], this list was refactored and removed from the core package's main runtime logic in recent versions (e.g., around React 18/19 transitions) [6]. In older versions, the list included properties like animationIterationCount, flex, flexGrow, fontWeight, lineHeight, opacity, order, zIndex, and zoom, among others [1][4]. Current React versions rely on standard CSS behavior and have moved away from this historical automatic pixel-appending feature, encouraging developers to provide explicit units or values as strings [7][6]. If you are working in a modern codebase, you should generally provide units (like 'px', 'rem', or '%') with your values rather than relying on React's automatic "px" suffixing [7].
Citations:
- 1: https://github.com/facebook/react/blob/8cac523beaaacfeae179ca14a1d8a46d82892016/docs/tips/06-style-props-value-px.md
- 2: https://legacy.reactjs.org/docs/dom-elements.html
- 3: CSS property values that are strings shouldn't have 'px' appended react/react#1357
- 4: https://github.com/facebook/react/blob/afea1d0c536e0336735b0ea5c74f635527b65785/packages/react-dom-bindings/src/shared/CSSProperty.js
- 5: https://github.com/facebook/react/blob/c78464f8ea9a5b00ec80252d20a71a1482210e57/src/renderers/dom/shared/CSSProperty.js
- 6: react/react@73deff0
- 7: Add missing unitless CSS properties react/react#19286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby implementation
ast-grep outline processor/transform/mdxish/resolve-deferred-attribute-expression-props.ts --view expanded || true
cat -n processor/transform/mdxish/resolve-deferred-attribute-expression-props.ts | sed -n '1,220p'
# Find related style handling and tests
rg -n "styleObjectToCssText|resolve-deferred-attribute-expression-props|style=\\{\\{|font-size:12|px" processor __tests__ test . || trueRepository: readmeio/markdown
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the regression test that exercises style={{...}} serialization.
cat -n __tests__/lib/mdxish/cx-3646-style-and-jsx-expressions.test.ts | sed -n '1,170p'
# Inspect a few nearby tests that exercise style props and CSS strings.
sed -n '1,220p' __tests__/components/Image.test.tsx | cat -n | sed -n '250,360p'
sed -n '1,220p' __tests__/components/Cards.test.tsx | cat -n | sed -n '1,160p'Repository: readmeio/markdown
Length of output: 12048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the regression test that exercises style={{...}} serialization.
cat -n __tests__/lib/mdxish/cx-3646-style-and-jsx-expressions.test.ts | sed -n '1,170p'
# Inspect a few nearby tests that exercise style props and CSS strings.
cat -n __tests__/components/Image.test.tsx | sed -n '250,360p'
cat -n __tests__/components/Cards.test.tsx | sed -n '1,160p'Repository: readmeio/markdown
Length of output: 16988
Append px for numeric style values
styleObjectToCssText needs CSS-unit handling for bare numbers on dimensional properties; fontSize: 12 currently becomes font-size:12, which browsers treat as invalid CSS instead of the 12px output MDX/React-style content expects. Update the regression test to assert the unitized form.
🤖 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 `@processor/transform/mdxish/resolve-deferred-attribute-expression-props.ts`
around lines 11 - 28, The `styleObjectToCssText` helper in
`resolve-deferred-attribute-expression-props.ts` currently serializes numeric
style values without CSS units, so dimensional properties like `fontSize` become
invalid; update `styleObjectToCssText` (and any supporting helpers such as
`cssPropertyName`/`isPlainObject` if needed) to append `px` for bare numeric
values on CSS length-like properties while leaving unitless or already-string
values unchanged. Also adjust the relevant regression test to assert that
`fontSize: 12` serializes to `font-size:12px`.
There was a problem hiding this comment.
Tested and the changes fixes the document, so approving for now even though I think we can avoid creating the preprocessing protect-nested-html-blank-lines.ts step to solve the CommonMark fragmentation issue. I have a PR for that pointing to this here: #1533!
The style blocks & object fixes look good.
2 participants
/prep-pr🎯 What does this PR do?
Docs that were force-migrated from the MDX renderer to mdxish silently broke content that relied on MDX-only evaluation:
<style>{...}</style>template literals rendered as literal, invalid CSS text instead of being evaluatedstyle={{...}}object expressions rendered as the literal string"[object Object]".map()blocks returning JSX were swallowed or JSON-stringified instead of rendered<div>caused CommonMark to fragment the block, leaking indented content as stray code blocksThis PR:
<style>{...}</style>expressions into plain CSS textstyle={{...}}objects into CSS attribute strings.map()expressions that return arrays of JSX elements🧪 QA tips
<style>{.foo { color: red; }}</style>— confirm the<style>tag contains plain CSS, not the literal{`/`}wrapper<div style={{ color: "red", fontSize: 12 }}>— confirmstyle="color:red;font-size:12", not[object Object]<div>wrapping a.map()that returns JSX (e.g. a card grid withkey={i}) — confirm each item renders instead of leaking[object Object]/_ownerJSON<div className="...">with blank lines between nested JSX siblings — confirm the block isn't fragmented into stray<pre>/code blocks__tests__/lib/mdxish/cx-3646-style-and-jsx-expressions.test.tsand__tests__/transformers/protect-nested-html-blank-lines.test.tsfor automated coverage📸 Screenshot or Loom