fix: move css cache onto container to allow parallel runs#252
fix: move css cache onto container to allow parallel runs#252danielroe merged 14 commits intodanielroe:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPer-container DOM caches added: Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
siimsams
changed the title
…ncurrent process() calls `classCache` and `idCache` in `dom.ts` were module-level globals shared across all `process()` invocations. When two `process()` calls overlap (e.g. Next.js static export processing multiple pages concurrently), the second call's `createDocument()` → `buildCache()` overwrites the caches while the first call is suspended at `await Promise.all(fetchStylesheet(...))`. When the first call resumes, it reads the wrong cache, finds no matching selectors, and silently skips CSS inlining entirely. Fix by storing `_classCache` and `_idCache` on the container node itself (via the domhandler `Node` module augmentation) instead of in module-level variables. Each `process()` call now has its own isolated cache through its own document/container, eliminating the race condition.
This reverts commit 6dec22a.
siimsams
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/beasties/src/dom.ts (1)
348-367: Add defensive null checks before accessing cache properties.The
cachedQuerySelectorfunction uses non-null assertions when accessingnode._classCacheandnode._idCache, but these properties are typed as optional. Althoughexists()is currently only called onbeastiesContainer(which has caches attached viabuildCache), the method is defined globally onElement.prototypeanddocument. Ifexistswere called on a different node in future code, the non-null assertions would throw aTypeError.Consider adding a guard to check cache existence before using them, falling back to
selectOne:Suggested defensive check
function cachedQuerySelector(sel: string, node: Node) { let selectorTokens = selectorTokensCache.get(sel) if (selectorTokens === undefined) { selectorTokens = parseRelevantSelectors(sel) selectorTokensCache.set(sel, selectorTokens) } - if (selectorTokens) { + if (selectorTokens && node._classCache && node._idCache) { for (const token of selectorTokens) { if (token.name === 'class') { - return node._classCache!.has(token.value) + return node._classCache.has(token.value) } if (token.name === 'id') { - return node._idCache!.has(token.value) + return node._idCache.has(token.value) } } } return !!selectOne(sel, node) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/beasties/src/dom.ts` around lines 348 - 367, The cachedQuerySelector function currently uses non-null assertions on node._classCache and node._idCache which can throw if those optional caches are missing; update cachedQuerySelector to defensively check that node._classCache and node._idCache exist before calling .has (and avoid the ! operator), and if a required cache is absent for a given token (token.name === 'class' or 'id') fall back to using selectOne(sel, node); keep the existing selectorTokensCache and parseRelevantSelectors logic but only short-circuit when the corresponding cache is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/beasties/src/dom.ts`:
- Around line 348-367: The cachedQuerySelector function currently uses non-null
assertions on node._classCache and node._idCache which can throw if those
optional caches are missing; update cachedQuerySelector to defensively check
that node._classCache and node._idCache exist before calling .has (and avoid the
! operator), and if a required cache is absent for a given token (token.name ===
'class' or 'id') fall back to using selectOne(sel, node); keep the existing
selectorTokensCache and parseRelevantSelectors logic but only short-circuit when
the corresponding cache is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83edc04a-a697-463f-8016-83e6c22eda0c
📒 Files selected for processing (2)
packages/beasties/src/dom.tspackages/beasties/test/beasties.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/beasties/src/dom.ts`:
- Around line 359-366: The current loop in the selector cache check (inside the
function handling selectorTokens on node with _classCache/_idCache) returns on
the first token check causing false negatives; change the logic so you do not
return false for a non-matching token — instead iterate all selectorTokens,
return true immediately if any token matches (e.g., token.name === 'class' and
node._classCache.has(token.value') or token.name === 'id' and
node._idCache.has(token.value')), and only allow the function to fall through
(no false return) when no positive match is found so the selectOne fallback can
run; apply the same fix to the equivalent block that skips partial token-groups
(the code referenced at the other occurrence around lines 380–387).
🪄 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
Run ID: 909fa5a9-9597-4e8a-b7ee-71777e90e893
📒 Files selected for processing (1)
packages/beasties/src/dom.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 84.81% 85.39% +0.57%
==========================================
Files 8 8
Lines 843 842 -1
Branches 235 235
==========================================
+ Hits 715 719 +4
+ Misses 115 111 -4
+ Partials 13 12 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@danielroe Hey! You are probably really busy but can you take a look? If there is anything needed let me know and I'll fix it. |
| const tokenGroup = tokens[i] | ||
| if (tokenGroup?.length !== 1) { | ||
| continue | ||
| return null |
There was a problem hiding this comment.
can you tell me why we change this?
There was a problem hiding this comment.
Hey, thank you for checking this so fast.
I changed this because continue could return the wrong result when a selector list contains both simple and complex selectors.
For example, with this HTML:
<div class="parent">
<span class="child">text</span>
</div>and this selector:
'.nonexistent, .parent .child'the correct result is true, because .parent .child matches.
The problem with continue was that it skipped complex groups like .parent .child, then kept evaluating only the simple ones. So in this case it would only check .nonexistent, find nothing, and return false, even though the full selector list should match.
Changing this to return null makes the cache logic stop and fall back to selectOne, which performs a full DOM query and handles complex selectors correctly.
I also added a test for this case to make sure the fallback returns the correct result.
| } | ||
| if (token.name === 'id') { | ||
| return idCache!.has(token.value) | ||
| if (token.name === 'id' && !node._idCache.has(token.value)) { |
There was a problem hiding this comment.
also here: is there a reason we're changing the logic?
There was a problem hiding this comment.
This was changed because the previous logic could return different results for the same selector list depending on the order.
With the old code, we returned immediately on the first selector that was checked. So for:
exists('.present, .absent')it checked .present first, found it, and returned true.
But for:
exists('.absent, .present')it checked .absent first, did not find it, and returned false.
That is incorrect, because selector lists separated by commas use OR logic. These two selectors are equivalent and should always produce the same result.
The updated logic checks all simple selector groups before deciding. This avoids order-dependent behavior and makes the result consistent.
I also added a test to verify that:
exists('.present, .absent') === exists('.absent, .present')so both orders behave the same way.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/beasties/test/dom.test.ts (1)
38-39: Consider adding an explicit expected value assertion.The test only verifies that both selector orders return the same result, but doesn't assert what that result should be. If both returned
falsedue to a bug, this test would still pass. Since.presentexists, the expected result istrue.Suggested improvement
- expect(container.exists('.present, .absent')) - .toBe(container.exists('.absent, .present')) + const result1 = container.exists('.present, .absent') + const result2 = container.exists('.absent, .present') + expect(result1).toBe(true) + expect(result2).toBe(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/beasties/test/dom.test.ts` around lines 38 - 39, The test currently only checks that container.exists('.present, .absent') equals container.exists('.absent, .present') which can mask failures; change the assertion to assert the explicit expected boolean (true) since the .present element exists—e.g., replace the equality assertion with an explicit expect(container.exists('.present, .absent')).toBe(true) (and optionally also expect(container.exists('.absent, .present')).toBe(true)) so both selector orders are asserted to be true; refer to the container.exists call and the selectors '.present, .absent' and '.absent, .present' to locate the code to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/beasties/test/dom.test.ts`:
- Around line 38-39: The test currently only checks that
container.exists('.present, .absent') equals container.exists('.absent,
.present') which can mask failures; change the assertion to assert the explicit
expected boolean (true) since the .present element exists—e.g., replace the
equality assertion with an explicit expect(container.exists('.present,
.absent')).toBe(true) (and optionally also expect(container.exists('.absent,
.present')).toBe(true)) so both selector orders are asserted to be true; refer
to the container.exists call and the selectors '.present, .absent' and '.absent,
.present' to locate the code to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aae1a614-a84a-4804-bb43-8d2a6fc84de1
⛔ Files ignored due to path filters (1)
packages/beasties-webpack-plugin/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/beasties/test/dom.test.ts
|
Please let me know if there’s anything else I should clarify or fix, and I’ll take a look as soon as I can. Thank you again, and I hope you have a peaceful Easter! |
danielroe
changed the title
|
Hey! Thank you again for taking a look and merging this. Is it possible to publish this version so I can migrate NextJs to the new version? Or is there something blocking the release? My open PR related to these changes. |
|
@danielroe is it possible to publish the version that's got this fix? Or is there some beta tag I'm not aware of? |
|
apologies for that! now released ✅ |
3 participants
Summary
other's caches across the await boundary in process(). This resulted in CSS selectors not matching and inlining
being silently skipped.
How it happened
Any framework that calls process() concurrently hits this (e.g. Next.js next build processing index, 404, 500 pages in the same worker).
Test plan
Related issue:
resolves #251
Related PR in Next.js repository:
vercel/next.js#88640