feat(runtime-tags): report HTML the parser will restructure, in development - #3686
feat(runtime-tags): report HTML the parser will restructure, in development#3686DylanPiercey wants to merge 8 commits into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds a development-only HTML structure validator that detects browser parser interventions and reports template source locations. Runtime chunks now preserve sourced, raw, and injected HTML segments through writes, chunk consumption, reordering, and flushing. Translators record native, dynamic, show, placeholder, and raw HTML sources for validation. New fixtures and tests cover parser restructuring, source attribution, segment handling, and diagnostic deduplication. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
🦋 Changeset detectedLatest commit: 9065acd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3686 +/- ##
==========================================
- Coverage 90.35% 90.32% -0.04%
==========================================
Files 414 417 +3
Lines 19737 20004 +267
Branches 3609 3713 +104
==========================================
+ Hits 17834 18069 +235
- Misses 1366 1376 +10
- Partials 537 559 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
15ebded to
849569c
Compare
…opment The parser silently relocates, drops or re-nests markup it considers invalid, which leaves the resume walk pointing at nodes the browser never built. Streamed HTML is now scanned in development and any such construct reported with the originating template position (file:line:col). It detects divergence rather than emulating the parser: table and select insertion contexts, missing table ancestors, implied end tags that reparent (including blocks closing a p through phrasing), nested a/button/headings/form, and unbalanced tags. Open-tag sources ride a dev-only channel beside each write; raw markup and runtime wrappers (including show's hidden t) keep their own attribution so opens stay ordered. Page asset flush preserves body sources when it only prepends, and show rejects illegal table/select contexts through transparent control flow.
e3122ac to
6005811
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/runtime-tags/src/html/validate-structure.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment blocks exceed the two-line limit in both files. Shared root cause: multi-line explanatory comments where two intent-capturing lines suffice.
packages/runtime-tags/src/html/validate-structure.ts#L1-L4: trim the header block (and the similar 3-4 line blocks at Lines 33-35, 62-64, 281-284) to two lines.packages/runtime-tags/src/__tests__/fixtures/html-structure-invalid-div-in-table/test.ts#L3-L6: condense the four-line rationale to two lines stating whyequivalent: falseandskip_optimize: trueare needed.As per coding guidelines: "Keep comments to two lines or fewer, use them only as a last resort, and make them capture intent rather than describe existing or removed code."
🤖 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 `@packages/runtime-tags/src/html/validate-structure.ts` around lines 1 - 4, Shorten the explanatory comment blocks in packages/runtime-tags/src/html/validate-structure.ts at lines 1-4, 33-35, 62-64, and 281-284 to no more than two intent-focused lines each. Also condense the rationale in packages/runtime-tags/src/__tests__/fixtures/html-structure-invalid-div-in-table/test.ts at lines 3-6 to two lines explaining why equivalent: false and skip_optimize: true are required; do not alter behavior.Source: Coding guidelines
packages/runtime-tags/src/__tests__/validate-structure.test.ts (1)
279-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test for state carried across two validator invocations.
In
writer.tsthe same validator instance is reused for every flush (state.validateStructure ||= createStructureValidator()), so theopenstack andreportedset persist between calls. A case that opens<table><tbody>in one call and emits the offending child in a second call would pin that contract.🤖 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 `@packages/runtime-tags/src/__tests__/validate-structure.test.ts` around lines 279 - 326, Add a test in the “segment boundaries” suite that invokes the same structure validator twice: first with a sourced segment opening `<table><tbody>` and then with a segment containing an invalid child such as `<div>`. Assert the second invocation reports the expected "`<div>` is not allowed in `<tbody>`" message, covering persistence of the open stack and reported state across validation calls.
🤖 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/runtime-tags/src/html/validate-structure.ts`:
- Line 356: Update the closing logic in validate-structure around the
voidElements and self-closing-attribute check so /> is accepted only for void
HTML elements or elements within foreign-content svg/math subtrees. Do not treat
/> as closing ordinary HTML elements such as div; preserve the parser-aligned
open-element stack and existing void-element behavior.
- Around line 294-305: Update the interactive lookup in the validation flow
around the a/button branch and its findLast call so it stops at template
boundaries, matching the existing select-scope behavior. Ensure nested
interactive elements inside template content do not report as closing the outer
element, either by adding the template stop condition to the lookup or by
removing template from phrasingThrough.
In `@packages/runtime-tags/src/translator/core/show.ts`:
- Around line 234-237: Condense the comment immediately above the runtime calls
to no more than two lines, preserving only the intent that the calls bracket
statements for readable declarations and that debug-only source keeps discarded
hidden wrappers associated with the show.
---
Nitpick comments:
In `@packages/runtime-tags/src/__tests__/validate-structure.test.ts`:
- Around line 279-326: Add a test in the “segment boundaries” suite that invokes
the same structure validator twice: first with a sourced segment opening
`<table><tbody>` and then with a segment containing an invalid child such as
`<div>`. Assert the second invocation reports the expected "`<div>` is not
allowed in `<tbody>`" message, covering persistence of the open stack and
reported state across validation calls.
In `@packages/runtime-tags/src/html/validate-structure.ts`:
- Around line 1-4: Shorten the explanatory comment blocks in
packages/runtime-tags/src/html/validate-structure.ts at lines 1-4, 33-35, 62-64,
and 281-284 to no more than two intent-focused lines each. Also condense the
rationale in
packages/runtime-tags/src/__tests__/fixtures/html-structure-invalid-div-in-table/test.ts
at lines 3-6 to two lines explaining why equivalent: false and skip_optimize:
true are required; do not alter behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| } else if (name === "a" || name === "button") { | ||
| closed = findLast( | ||
| open, | ||
| (el) => closes.includes(el.name), | ||
| phrasingThrough, | ||
| ); | ||
| if (closed) { | ||
| report( | ||
| `\`<${name}>\` closes the open \`<${closed.name}>\` early.`, | ||
| source, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
<template> in phrasingThrough yields a false nested-<a>/<button> report.
Template contents parse into a separate tree, so <a><template><a>… does not close the outer <a>. The select-scope check at Line 254 already stops at template; the interactive lookup should too.
🔧 Possible fix
} else if (name === "a" || name === "button") {
closed = findLast(
open,
- (el) => closes.includes(el.name),
+ (el) => el.name !== "template" && closes.includes(el.name),
phrasingThrough,
);Or drop template from phrasingThrough so the walk stops 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 `@packages/runtime-tags/src/html/validate-structure.ts` around lines 294 - 305,
Update the interactive lookup in the validation flow around the a/button branch
and its findLast call so it stops at template boundaries, matching the existing
select-scope behavior. Ensure nested interactive elements inside template
content do not report as closing the outer element, either by adding the
template stop condition to the lookup or by removing template from
phrasingThrough.
| ); | ||
| } | ||
|
|
||
| if (voidElements.has(name) || /(^|\s)\/$/.test(attrs)) continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
/> on a non-void HTML element does not actually close it.
The parser ignores the self-closing flag for HTML elements, so <div/> (reachable via raw markup) stays open. Treating it as closed here drops the element from open and can misattribute or suppress later reports. Restricting the /> allowance to foreign-content (svg/math subtrees) would keep the stack aligned with the parser.
🤖 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 `@packages/runtime-tags/src/html/validate-structure.ts` at line 356, Update the
closing logic in validate-structure around the voidElements and
self-closing-attribute check so /> is accepted only for void HTML elements or
elements within foreign-content svg/math subtrees. Do not treat /> as closing
ordinary HTML elements such as div; preserve the parser-aligned open-element
stack and existing void-element behavior.
| // The runtime calls bracket the body's statements (rather than taking a | ||
| // callback) so declarations in them stay readable by later statements. | ||
| // Source is only emitted in debug HTML so discarded `<t hidden>` wrappers | ||
| // still point at this `<show>`; omit unused trailing args when clean. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment exceeds the 2-line limit.
This 4-line block should be condensed to 2 lines or fewer.
As per coding guidelines, **/*.{ts,tsx,js,jsx,marko}: "Keep comments to two lines or fewer, use them only as a last resort, and make them capture intent rather than describe existing or removed code."
✏️ Suggested condensed comment
- // The runtime calls bracket the body's statements (rather than taking a
- // callback) so declarations in them stay readable by later statements.
- // Source is only emitted in debug HTML so discarded `<t hidden>` wrappers
- // still point at this `<show>`; omit unused trailing args when clean.
+ // Bracketing (not a callback) keeps body declarations readable; source is
+ // debug-only so discarded `<t hidden>` wrappers still point here.📝 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.
| // The runtime calls bracket the body's statements (rather than taking a | |
| // callback) so declarations in them stay readable by later statements. | |
| // Source is only emitted in debug HTML so discarded `<t hidden>` wrappers | |
| // still point at this `<show>`; omit unused trailing args when clean. | |
| // Bracketing (not a callback) keeps body declarations readable; source is | |
| // debug-only so discarded `<t hidden>` wrappers still point here. |
🤖 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 `@packages/runtime-tags/src/translator/core/show.ts` around lines 234 - 237,
Condense the comment immediately above the runtime calls to no more than two
lines, preserving only the intent that the calls bracket statements for readable
declarations and that debug-only source keeps discarded hidden wrappers
associated with the show.
Source: Coding guidelines
1 participant
The browser's parser silently relocates, drops or re-nests markup it considers invalid, which leaves the resume walk pointing at nodes it never built — the
<show>-in-a-table and reordered-rows bugs were both instances of this.The streamed HTML is now scanned in development and any such construct reported. It detects divergence rather than emulating the parser, so it covers only what actually changes the tree: table and select insertion contexts, table markup missing a required ancestor, implied end tags, and unbalanced tags.