fix(css): terminate a string or url() the tokenizer closed at EOF - #21624
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21624 +/- ##
==========================================
- Coverage 94.23% 94.22% -0.01%
==========================================
Files 628 628
Lines 79388 79590 +202
Branches 23456 23548 +92
==========================================
+ Hits 74810 74997 +187
- Misses 4578 4593 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
This PR is packaged and the instant preview is available (21af4f5). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@21af4f5
yarn add -D webpack@https://pkg.pr.new/webpack@21af4f5
pnpm add -D webpack@https://pkg.pr.new/webpack@21af4f5 |
| expect(min('@unknown [foo="bar\\"')).toBe('@unknown [foo="bar\\""];'); | ||
| // A `\` left dangling at EOF contributes nothing, so it goes rather than | ||
| // escaping the quote written back after it. | ||
| expect(min('@unknown [foo="bar\\')).toBe('@unknown [foo="bar"];'); |
There was a problem hiding this comment.
Config snapshots retain broken output
Low Severity
The direct expectations reflect the added closing quote, but both minimize-strings snapshots still expect the unterminated output. The corresponding ConfigTestCases and ConfigCacheTestCases snapshot assertions will fail.
Reviewed by Cursor Bugbot for commit b5f46f2. Configure here.
🦋 Changeset detectedLatest commit: 1dedc3b 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 |
Merging this PR will degrade performance by 11.39%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "devtool-eval", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
514 KB | 950.1 KB | -45.9% |
| ❌ | Memory | benchmark "future-defaults", scenario '{"name":"mode-production","mode":"production"}' |
8.4 MB | 10.7 MB | -21.29% |
| ⚡ | Memory | benchmark "three-long", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
1,158.6 KB | 709.2 KB | +63.36% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/css-printer-equivalence-defects (1dedc3b) with main (57177cc)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
There are 5 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 96113ad. Configure here.
| ); | ||
| if (extra.hasExtra) return CUSTOM_MEDIA_UNSUPPORTED; | ||
| if (nested !== undefined) return { kind: "alias", name: nested }; | ||
| return { kind: "condition", text: valueText }; |
There was a problem hiding this comment.
Compound aliases discard remaining media conditions
Medium Severity
classifyCustomMedia treats a leading (--name) as a complete alias without checking for subsequent tokens, silently discarding the rest of a compound media condition.
Reviewed by Cursor Bugbot for commit 96113ad. Configure here.
| )};`, | ||
| `${declare} removed = surplus(oldChildren, newChildren);`, | ||
| `${declare} added = surplus(newChildren, oldChildren);`, | ||
| "if (removed.length === 0 && added.length === 0) return true;", |
There was a problem hiding this comment.
Head patches do not preserve element order
Medium Severity
The multiset diff treats reorder-only updates as unchanged and appends every added node at the end. Head ordering therefore diverges from the updated HTML, affecting stylesheet cascade and script ordering.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 96113ad. Configure here.
|
You have used all Bugbot PR reviews included in your team's free trial on GitHub. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
1 similar comment
|
You have used all Bugbot PR reviews included in your team's free trial on GitHub. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
…light A declaration carrying `transition-*` started a CSS transition on the shared probe, so `getComputedStyle` returned a value interpolating away from the previous rule's rather than the one the declaration sets. The walk order is deterministic, so the two arms read different points of the same animation and one fixture was filed as a printer defect it never had.
CSS Syntax 4.3.5 and 4.3.6 end a string and a url-token at EOF, so their
content stops there. The printer re-emitted the token as written and then
wrote the enclosing `}`, which the still-open token took into its content:
`.a{content:"abc` printed as `.a{content:"abc}`. Write the terminator back
so the token ends where it ended for the tokenizer. A terminator that is
itself escaped (`"x\\"`) does not close the token, so the check counts
trailing backslashes rather than reading the last character.
A module that records one could not be written to the persistent cache — "No serializer registered for ModuleDependencyError" — so the pack was invalidated and the module rebuilt on every run. Its structurally identical sibling ModuleDependencyWarning was already registered.
Deserialization constructs the class with no arguments, so `err.message` threw and every restore of a cached compilation error failed. With the guard the `html/srcset` case restores from the persistent cache and its `test.filter.js` skip can go.
webpack always rebuilds a module that carries a build error, so the pack is written to on every run. That is expected, and the repo already states it with an `infrastructure-log.js` expectation; these three cases skipped the whole cache suite instead.
A dangling `\` is dropped when a string is closed at EOF (4.3.5 has it contribute nothing) but not when a url is: 4.3.6 reads it as an escape and 4.3.7 ends one at EOF with U+FFFD, so dropping it changed the url's value. Write that code point back instead.
Media Queries 5 3 defines `true` and `false` as always- and never-matching, and lets a value name another custom media. Both were classified unsupported and left as written. No `<media-in-parens>` is unconditionally true on every engine, so a constant is folded into the query around it instead of written out: it drops out of an `and` / `or` chain, takes the whole query with it when it decides one, and a list with nothing left prints `not all`. A name standing for another resolves transitively, with a cycle falling back to the old leave-as-written warning. Definitions now hold their classification rather than their tokens — node ids are recycled per top-level rule, so an at-rule's AST cannot outlive the walk that reaches it, and a name may be defined below its use.
Any `<head>` edit beyond `<title>` reloaded the page, because replacing `document.head.innerHTML` would tear down the runtime scripts and stylesheet links webpack injects there on load. Applying only the delta between the two authored heads cannot: neither string contains an injected node. A `<script>` in the delta still reloads — one parsed out of a string carries the already-started flag, so inserting it would never run it — as does a removal whose element is no longer in the head. FakeDocument now parses an `innerHTML` fragment (through webpack's own HTML parser) and exposes `children` / `outerHTML`, which the reconciliation walks.
`Pack got invalid because of write to: X` names something that was rebuilt rather than restored — work a user's incremental build redoes every time. The guide had no rule for it, so the reflex was to drop the case from the cache suite with a `test.filter.js`, which removes every cache assertion the case had. Name the diagnostics that point at the real cause, allow exactly the one expected write webpack ships (a module carrying a build error is rebuilt unconditionally), and require any other expectation to carry its reason. `define-plugin-runtime-value-errors` was skipping the cache suite for that one expected write; it states it and runs cached now. The other two opt-outs assert a throw from `webpack()` itself, which the pre-compile runs have nowhere to report — they keep the filter, with the reason written down.
Capturing walked node arrays, so every level sliced a fresh one and every `@media` in a file that defines a custom media built a tree even with no reference in it. Walk index ranges instead, and check for a reference first — `childAt` reaches the nested blocks without materializing a child list. On a stylesheet with one `@custom-media` and 200 unrelated `@media` rules the capture allocates 1 node instead of 801 and 0 arrays instead of 1604. Emitted bytes and warnings are unchanged on that file and on both custom-media cases.
An element parsed out of a string carries the already-started flag, so appending it never runs it — the reconciliation reloaded on any <script> in the delta. Building a fresh element from its attributes and body does run it, which is what the reload it replaces would have done. Removing one still reloads: a script that has run cannot be un-run. FakeDocument grew what that walks — attributes off the parsed fragment, an outerHTML serialized from the element's own state rather than the source that spelled it, and a load that fires only for a script that has a src, matching when a browser fetches one.
A miss in the memory cache returns `null`, and `null` bails `Cache.hooks.get` — so it is not "ask the next stage", it is "definitively not cached", and the file cache is never consulted. Etags were compared by identity, so any asset whose `Source` is rebuilt each compilation missed on an etag that stringifies identically to the stored one, then recomputed and stored: re-minified every build, and the pack invalidated with it. Three cases already recorded the symptom as unavoidable — "each time returns different OriginalSource … this prevents hit in inmemory cache". Identity still decides a hit first, so a warm hit costs no hashing; only a mismatch falls back to the value the rest of the cache compares etags by. Ten `infrastructure-log.js` expectations go with it, and stand as the regression test: they fail again the moment the pack is written to.
…g a miss Comparing the stringified etags was treating the symptom. A lazy etag is interned per source object (`getLazyHashedEtag` keys a WeakMap by it), so equal content reached through a second object is a different etag by construction — and hashing to tell those apart would cost every hit exactly what laziness saves. The defect is that the memory layer answered `null` there. `hooks.get` is an `AsyncSeriesBailHook`, so `null` bails the chain and the file cache — which compares etags by value — is never asked. It also buys the caller nothing: `Cache.get` maps `null` to `undefined` either way. So a mismatch now falls through, and only an identifier nothing was known about records a miss: an entry reached with a different etag still answers its own. Measured on a two-run build of `benchmarkCases/cache-filesystem`: 1029 identity hits out of 2556 gets and zero fall-throughs, so the added stage costs nothing outside the pattern it fixes.
…in order Two review findings, both real. `classifyCustomMedia` read a leading `(--name)` as the whole value, so `@custom-media --a (--b) and (min-width: 1px)` resolved to `--b` and dropped the rest. Only a block that *is* the whole value is an alias now; a reference anywhere else makes the value unresolvable, because nothing outside a `@media` prelude scans for one — the same hole was open in `not (--b)` and in a comma segment, and both are closed too. The head reconciliation appended every addition, so an element authored between two others landed after them and changed the cascade. Retained elements are now paired with their live nodes scanning only forward, which places each addition before the element it precedes and turns a reordering the delta cannot express into the reload it needs. FakeDocument's `insertBefore` ignored its reference argument, and parsing into a detached container registered its children document-wide.
alexander-akait
force-pushed
the
fix/css-printer-equivalence-defects
branch
from
August 7, 2026 13:05
28305e1 to
1dedc3b
Compare
|
You have used all Bugbot PR reviews included in your team's free trial on GitHub. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Contributor
Contributor
Generated code sizeComparing
8 asset(s) changed size
No runtime gained or lost a runtime module. Built |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three defects found while auditing what the browser equivalence suite had filed. Printer: CSS Syntax §4.3.5 and §4.3.6 end a string and a
url()at EOF, so their content stops there; the printer re-emitted such a token as written and then wrote the enclosing}, which the still-open token took into its content —.a{content:"abcprinted as.a{content:"abc}. The terminator is now written back, and a\left dangling at EOF is dropped rather than escaping it. Test suite: every declaration was applied to one shared probe, so a declaration carryingtransition-*animated the probe away from the previous rule's value andgetComputedStyleread the interpolation —color: limegreenread back asrgb(0, 0, 0)on one side andrgb(0, 128, 0)on the other. Cancelling running animations before the read fixes it, andat-rule-value.module.cssleavesFILED_CSS_DEFECTSwith the printer untouched, since its CSS and CSSOM were identical on both sides all along. Persistent cache:ModuleDependencyErrorwas never registered withmakeSerializable, though its structurally identical siblingModuleDependencyWarningwas, so any module recording one was skipped by the pack ("No serializer registered for ModuleDependencyError") and rebuilt on every run. Refs #21608.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
For the printer and the suite, yes: two fixtures leave
FILED_CSS_DEFECTSintest/SyntaxBrowserEquivalence.unittest.js, whose assertion is exact in both directions — it fails if a defect returns and also if an entry is left behind after a fix — andtest/CssSyntax.unittest.jscovers the printer directly, including the dangling\at EOF. Three assertions there were updated because they encoded the old output, which re-parsed to a different string than the source held. The serializer registration has no test of its own yet: its natural one is un-skipping the fourconfigCases/htmlcases thattest.filter.jsexcludes from the cache suite, and those still fail on further unrelatedRestoring failedgaps, so the filters stay for now.Does this PR introduce a breaking change?
No. The printer path only runs for a string or
url()left unterminated at end of file; well-formed input is byte-identical.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
AI was used. Claude Code reduced each filed defect to a minimal reproduction, proved the second was a measurement artifact rather than a printer bug by logging identical helper input against differing output, traced the third from a
PackFileCacheStrategyinfrastructure log to the unregistered class, and verified each fix in both directions. A dangling-backslash case reported by Bugbot on an earlier revision was reproduced and fixed here. All changes were reviewed before committing.Note
Medium Risk
Substantial changes to CSS media parsing and dev-server HTML HMR runtime, plus cache serialization for a common error type; mitigated by broad config/hot-case coverage but HMR head patching has edge cases (duplicate tags, script removal).
Overview
CSS
@custom-medianow resolves chained names, treatstrue/falseas boolean constants (folding whole@mediapreludes via a capturedMediaNodetree), and inlines condition/type values with clearer warnings for cycles and invalid uses.CSS printing writes closing quotes/
)for strings andurl()the tokenizer left open at EOF, using_terminateso a trailing\does not escape the added terminator (including U+FFFD for unterminatedurl()).HTML HMR reconciles authored
<head>changes as a multiset delta onouterHTMLinstead of reloading when only safe edits occur: injected webpack runtime tags stay put, new<script>nodes are recreated so inline code runs, and removing a<script>still forceslocation.reload().FakeDocumentparsesinnerHTMLfor children/outerHTMLto exercise this in hot cases.Persistent cache:
ModuleDependencyErroris registered withmakeSerializableand its constructor tolerates deserialization with noerr.AGENTS.mddocuments cache invalidation causes and discouragestest.filter.jscache exclusions; several HTML/error config cases now expect pack writes viainfrastructure-log.jsinstead of being dropped from the cache suite.Tests: custom-media cases and warnings updated; browser-equivalence probe cancels animations before
getComputedStyle; CSS syntax unit tests cover EOF behavior.Reviewed by Cursor Bugbot for commit 97a00f7. Bugbot is set up for automated code reviews on this repo. Configure here.