fix: getWebStyles not return default html styles#440
Merged
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:
📝 WalkthroughWalkthroughRegisters and tracks active CSSStyleRule objects (including media-query membership), prunes stale rules, listens for media-query changes, and switches getWebStyles to derive styles by scanning CSSListener.activeRules and safe selector-matching against a dummy element; tests updated for color and typography cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant GW as getWebStyles
participant CL as CSSListener
participant Dummy as DummyElem
participant MQL as MediaQueryList
Caller->>GW: request styles for className
GW->>CL: read activeRules
CL->>CL: pruneStaleRules() (on init)
loop for each CSSStyleRule in activeRules
GW->>Dummy: set selector (strip pseudos) & apply className
Dummy-->>GW: matches? (try/catch)
alt match
GW->>GW: collect rule.declarations -> parse to RN-style
end
end
par media-query monitoring
CL->>MQL: subscribe to MediaQueryList events
MQL-->>CL: change -> toggleRule(mqList, rule)
end
GW-->>Caller: combined RN-style object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/uniwind/src/core/web/cssListener.ts`:
- Around line 5-8: registeredRules is being used as an append-only cache which
can contain stale rules; update the implementation so the active-rule source of
truth reflects rule liveness and media state instead of blindly iterating
registeredRules. Either (A) clear and rebuild registeredRules during
re-initialization (and on stylesheet mutations) by scanning document.styleSheets
and repopulating only currently applicable CSSStyleRule entries, or (B) change
registeredRules into a Map<CSSStyleRule, MediaQueryList|boolean> (or use the
existing registeredRulesMediaQueries map) and update entries when listeners (the
listeners Map and MediaQueryList callbacks) fire or when stylesheets are removed
so iteration in getWebStyles uses only entries whose media match and whose
stylesheet is still in document.styleSheets; update code paths that add rules
(e.g., where lines 143-151 register rules) to set the liveness/media state
instead of appending blindly.
In `@packages/uniwind/src/core/web/getWebStyles.ts`:
- Around line 26-35: The comparison against rule.selectorText is using raw class
tokens and misses CSSOM-escaped selectors; in the CSSListener.registeredRules
loop update the mightMatch logic in getWebStyles.ts to escape each class token
(use CSS.escape on values from classNames derived from className) before
checking selector.includes(`.${escapedToken}`) so variant/escaped Tailwind
tokens (e.g. sm:text-base, w-1/2) are detected prior to calling dummy.matches();
modify the block that computes mightMatch (and any upstream uses of the raw
token) to use the escaped token instead.
- Around line 19-20: The variable extractedStyles in getActiveStylesForClass is
typed as CSSStyleDeclaration which lacks a string index signature and prevents
dynamic assignment like extractedStyles[camelCaseName] = propertyValue; change
extractedStyles to a plain string-keyed map (e.g., Record<string, string>) so
you can assign dynamic properties, update any related type annotations/usages in
getActiveStylesForClass to use the new map type, and ensure returns and
consumers expect a Record<string, string> instead of CSSStyleDeclaration.
In `@packages/uniwind/tests/e2e/getWebStyles.test.ts`:
- Around line 74-77: Update the test named "text-base -> fontSize 16px" (the
test function calling getWebStyles(page, 'text-base')) to also assert the
computed lineHeight; after obtaining styles from getWebStyles, add an assertion
that styles.lineHeight equals the expected value (e.g., '24px' for text-base at
16px fontSize) so both fontSize and lineHeight regressions are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13c658ac-ee32-4ba2-9630-d05c09d3e936
📒 Files selected for processing (3)
packages/uniwind/src/core/web/cssListener.tspackages/uniwind/src/core/web/getWebStyles.tspackages/uniwind/tests/e2e/getWebStyles.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/uniwind/src/core/web/getWebStyles.ts (2)
43-48: Redundant camelCase conversion.The property name is converted to camelCase at line 45, then the same conversion is applied again at lines 80-82 in
getWebStyles. Since keys are already camelCased, the second pass is a no-op. Consider removing one of the conversions.♻️ Option A: Remove conversion in getActiveStylesForClass (simpler helper)
for (const propertyName of rule.style) { const propertyValue = computedStyles.getPropertyValue(propertyName) - const camelCaseName = propertyName.replace(/-([a-z])/g, (g) => (g[1] ?? '').toUpperCase()) - extractedStyles[camelCaseName] = propertyValue + extractedStyles[propertyName] = propertyValue }♻️ Option B: Remove conversion in getWebStyles (avoid redundant iteration)
- return Object.fromEntries( - Object.entries(computedStyles) - .map(([key, value]) => { - const parsedKey = key[0] === '-' - ? key - : key.replace(/-([a-z])/g, (_, letter) => letter.toUpperCase()) - - return [ - parsedKey, - parseCSSValue(value), - ] - }), - ) + return Object.fromEntries( + Object.entries(computedStyles) + .map(([key, value]) => [key, parseCSSValue(value)]), + )Also applies to: 77-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/web/getWebStyles.ts` around lines 43 - 48, There’s a duplicated camelCase conversion of CSS property names between getWebStyles and getActiveStylesForClass; remove one conversion to avoid redundant work — either stop converting to camelCase in the first loop inside getWebStyles (remove creation/assignment of camelCaseName and assign extractedStyles[propertyName] = propertyValue) or stop converting in getActiveStylesForClass and rely on getWebStyles to emit camelCased keys; update usages that expect camelCased keys accordingly (look for variables propertyName, camelCaseName, extractedStyles, and the functions getWebStyles and getActiveStylesForClass) so keys remain consistent across the codebase.
19-20: Consider usingRecord<string, string>for type safety.Since
getPropertyValue()always returns a string, usingRecord<string, string>would be more precise thanRecord<string, any>and improve type safety downstream ingetWebStyles.♻️ Suggested fix
const getActiveStylesForClass = (className: string) => { - const extractedStyles = {} as Record<string, any> + const extractedStyles: Record<string, string> = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/web/getWebStyles.ts` around lines 19 - 20, The extractedStyles object in getActiveStylesForClass is typed too loosely as Record<string, any>; change its type to Record<string, string> to reflect that window.getComputedStyle(...).getPropertyValue(...) returns strings, and update any assignments in getActiveStylesForClass (and usages in getWebStyles) to ensure values are string (e.g., cast/convert results of getPropertyValue to string if necessary) so downstream consumers have correct, stricter typing.
🤖 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/uniwind/src/core/web/getWebStyles.ts`:
- Around line 43-48: There’s a duplicated camelCase conversion of CSS property
names between getWebStyles and getActiveStylesForClass; remove one conversion to
avoid redundant work — either stop converting to camelCase in the first loop
inside getWebStyles (remove creation/assignment of camelCaseName and assign
extractedStyles[propertyName] = propertyValue) or stop converting in
getActiveStylesForClass and rely on getWebStyles to emit camelCased keys; update
usages that expect camelCased keys accordingly (look for variables propertyName,
camelCaseName, extractedStyles, and the functions getWebStyles and
getActiveStylesForClass) so keys remain consistent across the codebase.
- Around line 19-20: The extractedStyles object in getActiveStylesForClass is
typed too loosely as Record<string, any>; change its type to Record<string,
string> to reflect that window.getComputedStyle(...).getPropertyValue(...)
returns strings, and update any assignments in getActiveStylesForClass (and
usages in getWebStyles) to ensure values are string (e.g., cast/convert results
of getPropertyValue to string if necessary) so downstream consumers have
correct, stricter typing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 889a49f3-dd22-4c0f-b1c0-325909bc9f4c
📒 Files selected for processing (1)
packages/uniwind/src/core/web/getWebStyles.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/uniwind/src/core/web/cssListener.ts (1)
182-186:⚠️ Potential issue | 🟠 MajorRules reusing a cached media query bypass match checks and change listeners.
When a rule shares its media query with a previously-processed rule, this early return skips:
- The initial match check (lines 190-194) — the rule stays in
registeredRulesregardless of whether the media query matches- Registering a change listener (lines 200-208) — the rule won't be added/removed when the media query state changes
For example, with two rules under the same
@media:`@media` (max-width: 600px) { .foo { color: red; } } `@media` (max-width: 600px) { .bar { color: blue; } }The
.barrule will remain inregisteredRuleseven when the viewport exceeds 600px.🔧 Suggested fix
if (cachedMediaQueryList) { this.classNameMediaQueryListeners.set(parsedClassName, cachedMediaQueryList) + if (cachedMediaQueryList.matches) { + this.registeredRules.add(rule) + } else { + this.registeredRules.delete(rule) + } + + cachedMediaQueryList.addEventListener('change', () => { + if (cachedMediaQueryList.matches) { + this.registeredRules.add(rule) + } else { + this.registeredRules.delete(rule) + } + }) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/web/cssListener.ts` around lines 182 - 186, The early return when cachedMediaQueryList exists (referencing classNameMediaQueryListeners, parsedClassName and cachedMediaQueryList) skips the per-rule match check and listener registration so subsequent rules sharing the same media query are never evaluated or toggled; fix by removing the early return and instead reuse the cached MediaQueryList object but continue to perform the initial match check (the logic that updates registeredRules) and attach the change listener for this parsedClassName just like the non-cached path — ensure the mapping classNameMediaQueryListeners.set(parsedClassName, cachedMediaQueryList) remains but follow with the same match and listener registration code that handles adding/removing entries in registeredRules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/uniwind/src/core/web/cssListener.ts`:
- Around line 182-186: The early return when cachedMediaQueryList exists
(referencing classNameMediaQueryListeners, parsedClassName and
cachedMediaQueryList) skips the per-rule match check and listener registration
so subsequent rules sharing the same media query are never evaluated or toggled;
fix by removing the early return and instead reuse the cached MediaQueryList
object but continue to perform the initial match check (the logic that updates
registeredRules) and attach the change listener for this parsedClassName just
like the non-cached path — ensure the mapping
classNameMediaQueryListeners.set(parsedClassName, cachedMediaQueryList) remains
but follow with the same match and listener registration code that handles
adding/removing entries in registeredRules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 888d3b29-c97a-4960-b2b3-6833200ee2e2
📒 Files selected for processing (3)
packages/uniwind/src/core/web/cssListener.tspackages/uniwind/src/core/web/getWebStyles.tspackages/uniwind/tests/e2e/getWebStyles.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uniwind/tests/e2e/getWebStyles.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/uniwind/src/core/web/cssListener.ts (1)
194-203: Consider: Event listeners accumulate for rules sharing the same media query.Each rule with the same media query condition adds a new listener to the cached
MediaQueryList. While functionally correct (each toggles its specific rule), these listeners persist even afterpruneStaleRules()removes the corresponding rules fromactiveRules.For now this is likely acceptable—listeners are lightweight and
toggleRuleon a removed rule is harmless. If memory becomes a concern with large stylesheets, consider tracking listener references for cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/web/cssListener.ts` around lines 194 - 203, Event listeners are added to the same cached MediaQueryList for each rule but never removed, so update the logic in cssListener.ts to track and remove listeners when rules are pruned: when you call cachedMediaQueryList.addEventListener('change', ...) (inside the branch where classNameMediaQueryListeners is set for parsedClassName), capture the listener callback and store it in a new map (e.g., mediaQueryListeners or entries on classNameMediaQueryListeners keyed by parsedClassName or the media query string) alongside the cachedMediaQueryList; then update pruneStaleRules() to look up and call cachedMediaQueryList.removeEventListener('change', storedCallback) for any removed parsedClassName/rule and delete the tracking entry, while keeping toggleRule(cachedMediaQueryList, rule) behavior unchanged.
🤖 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/uniwind/src/core/web/cssListener.ts`:
- Around line 212-215: The forEach callback for mediaQueryList currently uses a
concise arrow (listener => listener()) which returns the listener's result and
triggers the lint warning; change the callback to an explicit block body (e.g.,
listener => { listener(); }) or otherwise void the call so it returns undefined,
updating the call site where mediaQueryList.addEventListener('change', ...) uses
this.listeners.get(mediaQueryList)!.forEach(...) and then ensure
toggleRule(mediaQueryList, rule) remains invoked as before.
- Around line 172-178: When handling a CSSSupportsRule in addMediaQueriesDeep,
the branch that checks isSupportsRule(rule) and calls
addMediaQueriesDeep(rule.cssRules) must return control immediately to avoid
falling through into the later "'cssRules' in rule" branch and double-processing
the same nested rules; update addMediaQueriesDeep so that after calling
addMediaQueriesDeep(rule.cssRules) (when CSS.supports(rule.conditionText) is
true) you use continue/return to skip the subsequent "'cssRules' in rule" check
for that rule.
---
Nitpick comments:
In `@packages/uniwind/src/core/web/cssListener.ts`:
- Around line 194-203: Event listeners are added to the same cached
MediaQueryList for each rule but never removed, so update the logic in
cssListener.ts to track and remove listeners when rules are pruned: when you
call cachedMediaQueryList.addEventListener('change', ...) (inside the branch
where classNameMediaQueryListeners is set for parsedClassName), capture the
listener callback and store it in a new map (e.g., mediaQueryListeners or
entries on classNameMediaQueryListeners keyed by parsedClassName or the media
query string) alongside the cachedMediaQueryList; then update pruneStaleRules()
to look up and call cachedMediaQueryList.removeEventListener('change',
storedCallback) for any removed parsedClassName/rule and delete the tracking
entry, while keeping toggleRule(cachedMediaQueryList, rule) behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7da91943-3b07-449a-87fe-c6be253e98f5
📒 Files selected for processing (2)
packages/uniwind/src/core/web/cssListener.tspackages/uniwind/src/core/web/getWebStyles.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/uniwind/src/core/web/cssListener.ts`:
- Around line 19-20: Attribute-change mutations (handled in
scheduleInitialization/initialize) currently skip already-seen sheets using
processedStyleSheets, so toggling attributes like disabled/media/href leaves the
rule index stale; update initialize (or the attribute-mutation path in
scheduleInitialization) to invalidate or remove affected entries from
processedStyleSheets (or clear the cache) when the mutation.type is 'attributes'
for elements that can change stylesheet applicability, and then rebuild the
active rule index by re-scanning document.styleSheets (the same logic used by
getWebStyles.ts) so the toggled/modified <link> or <style> is re-evaluated;
ensure you reference processedStyleSheets, scheduleInitialization, and
initialize when making the change.
- Around line 87-95: pruneStaleRules currently removes dead CSSStyleRule objects
from activeRules but leaves MediaQueryList listeners (created in addMediaQuery)
holding references, letting toggleRule resurrect detached rules; fix by (a)
updating toggleRule to first verify rule.parentStyleSheet exists and is present
in document.styleSheets before toggling, and/or (b) during pruneStaleRules
iterate any stored MediaQueryList listeners (created in addMediaQuery) and call
removeEventListener / removeListener for those whose associated
rule.parentStyleSheet is missing, then clear their references from whatever
listener registry you use so they cannot re-add rules; reference
pruneStaleRules, toggleRule, and addMediaQuery to locate changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3ba442a-4fcb-4664-909d-58c6f63cd65d
📒 Files selected for processing (1)
packages/uniwind/src/core/web/cssListener.ts
Contributor
|
🚀 This pull request is included in v1.6.0. See Release v1.6.0 for release notes. |
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.
fixes #411
Summary by CodeRabbit
Refactor
Bug Fixes
Tests