fix(core): catch PostCSS parse errors instead of dropping compositions - #3589
Conversation
Invalid CSS in a sub-composition style block made postcss.parse throw
inside scopeCssToComposition. The throw propagated to the composition
loader's catch block, which emptied the host — silently dropping the
entire scene. Lint swallowed the same error via catch { continue },
reporting 0 warnings.
Two fixes:
- Runtime: wrap postcss.parse in try/catch and return the original
(unscoped) CSS on failure, so the composition still mounts
- Lint: emit a css_parse_error finding instead of silently continuing
Fixes #3585.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed the full files at d0daa443, not just the diff. The lint half is a clean win; the core half ships the exact tradeoff #3585 said needed a product call, and it ships the leaky side of it.
Strengths
packages/lint/src/rules/core.ts:315-321is a straight improvement: the oldcatch { continue }discarded aCssSyntaxErrorwith no output at all, and the author now gets acss_parse_errorat severityerror. Thecontinueis correctly retained, so one bad stylesheet still doesn't abort the remaining rules.- The finding's message pins postcss's real
reasonstring. I confirmed against the repo's own postcss (8.5.14) thatMissed semicolonis what it actually emits for the fixture, so the assertion isn't guessing at wording. HyperframeLintFinding.codeis a free-formstring(packages/lint/src/types.ts), so there's no code registry or union that a new finding has to be added to. Nothing was missed on that axis.- The problem being fixed is real, and worse than the title suggests. On the inline-template path,
loadInlineTemplateCompositions(packages/core/src/runtime/compositionLoader.ts:606-630) callsmountCompositionContentwith notry/catcharound it, so a single malformed stylesheet rejected that promise and took down every composition on the page, not just its own.
Blocker — the catch path returns UNSCOPED CSS, and every sub-composition call site injects it into the parent
packages/core/src/compiler/compositionScoping.ts:233-238 returns css verbatim on a parse error. All four sub-composition call sites pass scopeRootSelectors: true specifically to prevent unscoped sub-comp CSS from reaching the host, and the catch bypasses every one of them:
packages/core/src/runtime/compositionLoader.ts:463-473, then:475appends the returned text to the parent document's<head>packages/core/src/compiler/inlineSubCompositions.ts:298-300packages/core/src/compiler/htmlBundler.ts:1014-1016and:1045-1047, which push it intocompStyleChunks— so on the bundler path the leak is baked into the rendered output, not just the live preview
The hazard is documented in the file the change routes into. compositionLoader.ts:468-472: "Sub-comp styles are injected into the PARENT preview document, so remap html/body/:root to the composition box — otherwise a sub-comp body { width/height/overflow } clobbers the host body and clips the preview to the last-mounted sub-comp's size." And :417-420: "mounting the content whole left its CSS unscoped and leaking into the host document."
The reason this is not a narrow escape: postcss aborts the whole stylesheet, so the catch hands back every rule, not just the bad one. Measured with the repo's postcss 8.5.14 on a stylesheet whose only defect is on the last line:
body { margin: 0; overflow: hidden; background: #000; }
:root { --accent: #5ef17c; }
.stage { position: absolute; inset: 0; width: 1920px; height: 1080px; }
.title { font-size: 72px; color: var(--accent); }
.broken { transform: xPercent: -10; }
→ CssSyntaxError: Missed semicolon at line 6, nothing partial returned.
So all five rules are returned unscoped and appended to the parent <head>, where the browser happily applies the four valid ones. That body { overflow: hidden } and that :root custom property now belong to the host preview — precisely the failure the scopeRootSelectors option exists to prevent, and on the bundler path it reaches the final render.
The strict-vs-tolerant product question raised on #3585 is Miguel's call, not mine. But returning unscoped CSS is the wrong implementation of tolerance under either answer, because a one-line change gets the stated goal without the leak: return "" instead of css. The composition still mounts (no more dropped compositions, which is the PR's actual objective), and the stylesheet that could not be parsed is dropped instead of being promoted to global scope.
Important — the runtime half degrades silently, and this is a step back from the previous behaviour
The catch is bare: no diagnostic, no warning. That leaves the runtime with strictly less signal than before the change. loadExternalCompositions previously caught the throw at compositionLoader.ts:733-744 and emitted external_composition_load_failed carrying errorMessage, then reset the host — the composition was dropped, but loudly. Now it mounts with leaked styles and says nothing.
mountCompositionContent already receives an onDiagnostic hook (compositionLoader.ts:409-412) and the codebase already has the code convention for this. A css_parse_error-shaped diagnostic on the runtime path would keep the lint improvement's benefit where the rendering actually happens. As written, the only surface that reports the problem is the linter, which is not in the path for a live preview or a render.
Nit — the test fixture is the one member of its class that cannot show the leak
packages/core/src/compiler/compositionScoping.test.ts uses .stage { transform: xPercent: -10; }, whose only declaration is the malformed one. Leaked or not, the browser drops that declaration and the surviving empty rule paints nothing, so the fixture is inert by construction and the test passes without ever exercising the risk. expect(result).toBe(malformedCss) then pins "return unscoped" as intended behaviour.
A fixture shaped like the block above — valid body/:root rules plus one malformed tail — would make the leak visible, and would fail against the current implementation. That is the test worth having whichever way the product call lands.
Verdict
REQUEST CHANGES. The lint half is ready as-is. The core half needs the catch to stop returning unscoped CSS ("" is sufficient and meets the stated goal) and should emit a diagnostic rather than degrading in silence. The strict-vs-tolerant decision from #3585 is Miguel's to make, and I have not tried to settle it here — but the leak is a defect under either choice.
Footnote: CI was fully concluded at this head before I submitted (single wave, all 8 required contexts green), so nothing was in flight to cancel. Submitting this review may briefly stamp skipped over the green required contexts and flash BLOCKED for a minute; that is the review event, not a regression, and does not need a rerun.
— Rames Jusso
Return "" on PostCSS parse failure so sub-composition stylesheets that cannot be scoped are dropped rather than injected unscoped into the parent document. Updates test fixture to use valid+malformed CSS that demonstrates the leak risk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Addressed the review:
Blocker fixed: scopeCssToComposition now returns "" on parse failure instead of the original unscoped CSS. This prevents sub-composition stylesheets from leaking body/:root rules into the parent document on all four call sites (compositionLoader, inlineSubCompositions, htmlBundler ×2).
Test updated: Fixture now uses valid body/:root/.stage rules with a malformed tail (.broken { transform: xPercent: -10; }), and asserts "" return — directly exercises the leak scenario rather than using an inert single-declaration fixture.
Lint half: Unchanged — css_parse_error finding emission is the right call and stays as-is.
On the diagnostic/onDiagnostic point: You're right that the runtime path currently degrades silently. The lint path catches it for hyperframes check users, but live preview and render get no signal. I'll add that as a follow-up since the diagnostic plumbing needs to thread through scopeCssToComposition's callers — the immediate leak fix shouldn't wait on that.
— Miga
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 989110c6c. The blocker is resolved, and the fix is the minimal one — +1/-1 in source.
Blocker resolved
packages/core/src/compiler/compositionScoping.ts:237 now returns "" instead of the original CSS, so an unparseable stylesheet is dropped rather than promoted to global scope. The four sub-composition call sites that pass scopeRootSelectors: true no longer have their guarantee bypassed on the error path, and the bundler no longer bakes unscoped rules into the render. Nothing else in the file moved.
The new test actually earns its name
I checked this rather than assuming it, because the change implements my own suggestion and that is exactly where a reviewer stops looking. It holds up:
- The fixture carries three valid rules —
body { margin: 0; overflow: hidden; },:root { --accent },.stage { position: absolute; inset: 0; }— plus the malformed.brokentail. - Run through the repo's postcss 8.5.14 it throws
CssSyntaxError: Missed semicolonat line 4, so the catch is genuinely exercised, not bypassed by a fixture that happens to parse. expect(result).toBe("")fails against the previousreturn css, so it discriminates between the two implementations instead of merely passing.- The name changed from "returns original CSS" to "drops the entire stylesheet", which is now what it checks. That was the nit; it is gone.
Those three valid rules are precisely what used to leak into the parent document, so the fixture reproduces the reported scenario rather than gesturing at it.
Residual, and why it is not gating
The catch is still silent — no diagnostic on the runtime or bundler path. My previous verdict line bundled that with the blocker, so to be explicit rather than quietly dropping it: I am downgrading it to a non-gating follow-up. Nothing renders incorrectly, the composition simply loses styles that were never parseable, and hyperframes check does report the problem through the css_parse_error finding this PR added.
What would flip it back to worth-its-own-PR: the first time someone spends real time on "why is my sub-composition unstyled" with nothing in the console pointing at their CSS. mountCompositionContent already takes an onDiagnostic hook (packages/core/src/runtime/compositionLoader.ts:409-412) and the loader already emits external_composition_load_failed, so the wiring exists whenever that becomes worth doing.
The strict-vs-tolerant question from #3585 is still Miguel's to settle, and this change does not foreclose either answer — it just implements tolerance without the leak.
Verdict
APPROVE. Blocker fixed at the root, test now discriminates, lint half unchanged and still a clean improvement. One non-gating follow-up noted above. Not merging — that call is not mine.
Held this submit until the pull_request matrix at this head concluded, for the same reason as last time: a review event can cancel an in-flight matrix in the same concurrency group and nothing re-fires it.
— Rames Jusso
2 participants
What
Invalid CSS in a sub-composition
<style>block drops the entire compositionsilently. Lint reports 0 warnings.
Fixes #3585.
Why
scopeCssToCompositioncallspostcss.parsewith no try/catch. A malformeddeclaration like
transform: xPercent: -10;throwsMissed semicolon. Thethrow propagates to the composition loader's catch block, which calls
resetCompositionHost— emptying the slot. The scene never registers on thetimeline. Meanwhile, lint's own
postcss.parsecalls swallow the same errorvia
catch { continue }, sohyperframes checkreports success.How
Two fixes:
Runtime (
compositionScoping.ts): wrappostcss.parsein try/catch.On failure, return the original (unscoped) CSS. The composition still mounts
with its stylesheet applied browser-like (the browser drops the bad
declaration and keeps the rest).
Lint (
core.ts): emit acss_parse_errorfinding instead of silentlycontinuing.
hyperframes checknow surfaces the offending rule and line.Test plan
compositionScoping.test.ts: new test verifies malformed CSS returnsthe original string instead of throwing (51 tests pass)
core.test.ts: new test verifiescss_parse_errorfinding is emittedwith severity
errorand message containingMissed semicolon(58 tests pass)— Miga