fix(lint): parse HTML structure without regex#2223
Conversation
- replace repeated attribute scanner with quoted tag ranges - remove the Fallow complexity finding
Addressed in 60a9b21: removed the complex scanner in favor of quoted tag ranges. |
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: 🟢 LGTM at b114db3b
Clean regex → real-parser refactor. htmlparser2 correctly ignores tag-like content inside quoted attribute values, which was the root cause of the iframe[srcdoc] false-positives — the fix is at the right layer.
Mechanism verified:
parseHtmlStructureinpackages/lint/src/utils.ts— event-based Parser withonopentag/onclosetaghandlers, LIFO-per-name stack for close-index attribution.{ decodeEntities: false, lowerCaseAttributeNames: false, lowerCaseTags: true }— decode-off matches the old regex behavior (both operated on raw source slices), lowercase-tags matches the old(match[1] || "").toLowerCase(), attr-case preserved (unchanged). Void elements like<img>/<link>/<meta>fireonclosetagimmediately in htmlparser2, socloseIndex/endIndexare set for them too — thefindRootTagfilter and SVG-skip logic work unchanged.- Attribute-value tag suppression is a natural property of a real parser:
<iframe srcdoc="<img src=x>">emits exactly oneonopentag("iframe")and never a nestedimg— theimginside quotes is an attribute value, not a token. That's what fixes the false positives across scripts, media URLs, root/composition rules, and variable-declaration collection. findRootTagatutils.ts—bodyTags = tags.filter((tag) => tag.index >= bodyStart && tag.index < bodyEnd)reads absolute indices, so the old{ ...tag, index: tag.index + bodyStart }adjustment goes away cleanly (the old code was compensating for a sliced-view scan; the new code doesn't need to). SVG-skip:skipBefore = tag.endIndex ?? Infinitymatches the oldcloseMatch ? tag.index + closeMatch.index + closeMatch[0].length : Infinityon both the found-close and no-close branches.context.tstemplate-unwrap: iteratestemplateTagsin reverse and space-fills each[index, endIndex)— preserves absolute positions insourceWithoutTemplates(important because downstreamfindRootTagis called on it and needs coherent indices). Unwrap-fallback usestemplateTags[0]and slices the first template's inner content.project.ts— separate parser choice (linkedom.parseHTML) for content-only querying (stylesheets,[style]attributes) makes sense: htmlparser2's position-aware output isn't needed there, and linkedom gives a DOM-shapequerySelectorAllinterface. Both are real parsers, both respect quoted-attribute boundaries — no interop concern.
Test coverage locked at every affected surface:
hyperframeLinter.test.ts:127-134— pins theiframe srcdocnon-fetch onlintMediaUrls.core.test.ts:78-101— pins non-lint onsrcdoc-embedded<script>(noinvalid_inline_script_syntax, nogsap_timeline_not_registered) and onsrcdoc-embedded<video>(no top-level media finding).
Full lint suite green (357/357), and the PR body's list of previously-broken surfaces (JS syntax, missing-timeline, media, visibility, Studio-editability) all route through parseHtmlStructure now.
Minor observations, non-blocking:
- Two-template unwrap behavior narrows. The old greedy regex
<template[^>]*>([\s\S]*)<\/template>on<template>A</template>X<template>B</template>capturedA</template>X<template>B(greedy scan across the middle</template>). The new code slices the FIRST template's inner content only ("A"). This is more principled — a hyperframe should have one composition root, and if the fallback unwrap is triggered the first template is the useful one — but it's a real behavior change if any existing sub-composition file has two templates AND no root outside them AND relied on the greedy junk-inclusive capture. I don't have a reason to think any file does, but worth flagging for grep-verification against your corpus of sub-composition files if you have one handy. attrson self-closing tags keeps a trailing slash.raw.slice(name.length + 1, -1)on<img src="x"/>producessrc="x"/(trailing/). Old regex produced the same shape, andreadAttris regex-based and ignores the slash, so no functional impact — noting for anyone parsingattrsdirectly in the future.decodeEntities: false— attribute values retain&and"unchanged. This matches the raw-source-slice contract the old regex code operated under, so no downstream consumer sees a behavior change. Only worth noting if a future rule ever starts comparing decoded values fromreadAttragainst a decoded literal.
CI: current head b114db3b re-triggered the required run set — CLI smoke (required), Typecheck, Test, Build, and Windows lanes were in-progress at review time (prior Smoke: global install fail was on a superseded run that was cancelled, not a real regression). Verdict conditional on that set landing green; nothing in the diff suggests it wouldn't.
R1 by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at b114db3.
Parser swap is structurally sound and the srcdoc fix is well-motivated — htmlparser2 correctly treats iframe[srcdoc] content as opaque attribute text, not sub-document HTML, so the reported false-positive scripts/timelines/media findings do go away. decodeEntities: false is the right call for keeping script/style/attribute content byte-identical via source.slice(...).
One code-anchored concern in project.ts (see inline on L48) — the DOM-based querySelectorAll path silently narrows coverage vs. the old flat-regex scan for <template>-interior <link> / <style> / [style] elements. Verified empirically against linkedom 0.18.12. Affects template-shell sub-comps specifically; blast radius through lintProject's compositions/${file} iteration is non-trivial. Two nits inline in utils.ts for the attrs-extraction stray / and a let→const micro-simplification.
Non-blocking follow-ups worth flagging:
- Parser divergence. Package now uses BOTH
htmlparser2(utils.ts/context.ts/hyperframeLinter.ts) ANDlinkedom(project.ts). They already disagree on<template>scoping (the blocker above). Any subtle disagreement on malformed-recovery / self-closing / namespaced-tag edge cases could produce inconsistent lint context across rule groups. Longer-term: consolidate. - Redundant reparses.
parseHtmlStructureruns 2–4× per file per lint pass (buildLintContextinitial + template-unwrap re-parse,extractMediaUrls,findRootTagfallback when called withoutparsedTags). Not urgent at typical file sizes, but a shared per-file cache would trim the redundancy — same shape ascontext.externalStylesthreading. - Test-coverage gaps. The added tests cover
iframe[srcdoc]scripts + srcdoc media (the intended fix), but no fixture exercises: (a) template-shell comp with template-interior<link>/<style>(would catch the blocker above), (b) nested<template>(blank-out reverse-iterate handles it, but no test locks that), (c)<div title="a > b">— attribute with literal>(old regex broke, new parser handles; regression test would document the fix).
Nothing I couldn't reason through from the diff — I did not run the lint suite locally.
The new |
What
Prevent HTML embedded in quoted attributes such as
iframe[srcdoc]from producing host-document lint findings. Structural HTML extraction now covers tags, scripts, styles, templates, roots, remote media, variable declarations, and project CSS/link discovery through a real parser.Why
Regex block and tag scans treated nested
srcdoccontent as part of the host composition. That produced false JavaScript syntax and missing-timeline errors, plus false media, visibility, and Studio-editability findings, even when the root composition was valid.How
Use
htmlparser2for HTML structure while preserving exact source positions and raw snippets. Source-semantic checks remain regex-based where appropriate: GSAP/JavaScript patterns, CSS values and selectors, malformed raw markup, and asset paths.The same parser-backed structure is reused across lint context construction, template/root discovery, remote-media validation, variable declarations, and project stylesheet collection.
Test plan
srcdocscript, element, and remote-media regressionsAdditional verification: lint typecheck, format, oxlint, lint build, unchanged 167 KB browser entry, and Fallow audit all pass.