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:
📝 WalkthroughWalkthroughAdds scoped theming: introduces UniwindContext and ScopedTheme (web + native), threads the context through style resolution and CSS variable lookups, converts store/config to per-theme vars and caches, replaces function-based CSS visitors with theme-aware visitors used in the transform pipeline, and updates tests to validate scoped behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App/Component
participant ScopedTheme as ScopedTheme
participant Context as UniwindContext
participant Hook as Hook/HOC (useStyle / useResolveClassNames)
participant Store as UniwindStore
participant Vars as Per-Theme Vars
App->>ScopedTheme: render(theme="dark")
ScopedTheme->>Context: Provider({ scopedTheme: "dark" })
App->>Hook: call hook/HOC
Hook->>Context: useUniwindContext() -> { scopedTheme: "dark" }
Hook->>Store: getStyles(className, props, state, uniwindContext)
Store->>Vars: lookup vars["dark"]
Vars-->>Store: return dark variables
Store-->>Hook: return scoped styles
Hook-->>App: apply styles
sequenceDiagram
participant Build as Metro/Vite
participant CSS as Generated CSS
participant Transform as lightningcss.transform
participant Visitor as UniwindCSSVisitor
participant RuleVis as RuleVisitor
participant FuncVis as FunctionVisitor
participant Output as Compiled CSS
Build->>CSS: provide generated CSS
CSS->>Transform: transform(css, visitor=new UniwindCSSVisitor(themes))
Transform->>Visitor: instantiate with themes
Transform->>RuleVis: apply rule transforms (theme/class handling)
Transform->>FuncVis: process function calls (pixelRatio/fontScale/etc.)
Transform-->>Output: emit transformed CSS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/uniwind/src/hooks/useResolveClassNames.ts (1)
9-25:⚠️ Potential issue | 🟠 Major
scopedThemechanges will never re-trigger style resolution on web — scoped theming is effectively broken.
useUniwindContext()subscribes the component to context updates, so it will re-render whenscopedThemechanges. However, theuseLayoutEffectdependency array only contains[className]. WhenscopedThemechanges (butclassNamedoes not), the effect does not re-run,recreate()is never dispatched, and thestylesstate stays frozen at the value computed for the old theme.This matters because
getWebStylesrelies on settingdummyParent's class toscopedThemebefore reading computed styles — it is inherently context-dependent and must be re-invoked on every theme change.🐛 Proposed fix
useLayoutEffect(() => { if (className === '') { return } recreate() const dispose = CSSListener.subscribeToClassName(className, recreate) return dispose - }, [className]) + }, [className, uniwindContext.scopedTheme])The same dependency-array gap exists in the native variant (
useResolveClassNames.native.ts— both of itsuseLayoutEffectcalls only list[className]and[uniwindState.dependencySum, className]). For native, verify whetherUniwindListener's dependency subscription already covers scoped-theme transitions, or whether a similar fix is needed there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/hooks/useResolveClassNames.ts` around lines 9 - 25, The effect in useResolveClassNames.ts only depends on className so it never re-runs when scopedTheme changes; because useUniwindContext() provides scopedTheme to getWebStyles (which uses dummyParent), you must include the context-derived value in the dependency array so recreate() is called on theme changes. Update the useLayoutEffect that calls recreate() and CSSListener.subscribeToClassName(className, recreate) to include the relevant context value (e.g., uniwindContext.scopedTheme or the whole uniwindContext) alongside className; ensure the initial useReducer initializer also re-computes when that context value changes (mirror the dependency change). Also audit the native file useResolveClassNames.native.ts for similar missing dependencies (replace/add uniwindState.scopedTheme or uniwindState/dependencySum as appropriate).packages/uniwind/src/hooks/useResolveClassNames.native.ts (1)
7-25:⚠️ Potential issue | 🟠 MajorStale styles when
ScopedThemetheme changes — missingunifwindContextinuseLayoutEffectdep array.When the enclosing
ScopedThemetheme changes,useContext(UniwindContext)triggers a re-render with the newunifwindContext, butunifwindStatein the reducer stays stale: neitheruseLayoutEffectfires (onlyclassNameanddependencySumare deps), sorecreate()is never dispatched and the component renders with outdated styles untilclassNameindependently changes or the listener fires.The same issue exists in the web variant
useResolveClassNames.ts(referenced in the relevant snippets), where the singleuseLayoutEffect([className])also omitsunifwindContext.🐛 Proposed fix
useLayoutEffect(() => { if (className !== '') { recreate() } -}, [className]) +}, [className, unifwindContext])Note: This fix relies on
unifwindContextbeing a stable reference when the theme hasn't changed. Memoize thevalueobject inScopedTheme(see that file's comment) so context consumers aren't needlessly re-triggered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/hooks/useResolveClassNames.native.ts` around lines 7 - 25, The reducer's state can become stale when the UniWind context changes because uniwindContext (from useUniwindContext) is not included in the useLayoutEffect dependency arrays; update both useLayoutEffect hooks in useResolveClassNames.native (and mirror the same fix in the web variant useResolveClassNames.ts) to include uniwindContext so recreate() is dispatched when the context changes; ensure the UniwindListener.subscribe call still uses uniwindState.dependencies and that dependencySum and className remain in the deps along with uniwindContext so UniwindStore.getStyles (the reducer init) is re-evaluated with the new uniwindContext.
🧹 Nitpick comments (11)
packages/uniwind/tests/web/hoc/withUniwind.test.tsx (1)
97-116: The[manual]color test doesn't verify the context argument passed togetWebStyles.The
[auto]test (line 45) guards thatgetWebStylesis called withUNIWIND_CONTEXT_MOCK, but the equivalent manual test only checks the resolved prop value — it won't catch a regression where the manual code path fails to threaduniwindContext.🧪 Suggested addition
render(<ManualWithUniwind testClassName="fill-red-500" testID="test-component" />) + expect(mockGetWebStyles).toHaveBeenCalledWith('fill-red-500', UNIWIND_CONTEXT_MOCK) + const receivedProps = ComponentWithSpy.mock.calls[0][0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/tests/web/hoc/withUniwind.test.tsx` around lines 97 - 116, The manual color test currently only checks the resolved prop value and misses verifying that withUniwind/threading passes the uniWind context into webCore.getWebStyles; after rendering ManualWithUniwind, add an assertion that the mocked webCore.getWebStyles (mockGetWebStyles) was called with the UNIWIND_CONTEXT_MOCK context (same guard as the [auto] test) so regressions that drop the context are caught—i.e., after render and before inspecting ComponentWithSpy.mock.calls, assert that mockGetWebStyles was invoked with UNIWIND_CONTEXT_MOCK (matching how the [auto] test verifies getWebStyles).packages/uniwind/src/core/context.ts (1)
4-6: Consider settingdisplayNameonUniwindContextfor clearer React DevTools labelling.Without it, DevTools shows the generic
"Context"label, making it harder to identify when debugging nested context trees.✨ Suggested addition
export const UniwindContext = createContext({ scopedTheme: null as ThemeName | null, }) +UniwindContext.displayName = 'UniwindContext'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/context.ts` around lines 4 - 6, Set a displayName on the UniwindContext so React DevTools shows a meaningful label: locate the UniwindContext created with createContext (symbol UniwindContext) and after its creation assign UniwindContext.displayName = "UniwindContext" (or another descriptive name) to improve debugging visibility.packages/uniwind/src/hooks/useResolveClassNames.native.ts (1)
8-11: Eager initial-state evaluation callsgetStyleson every render.The second argument to
useReduceris evaluated on every render even though React only uses it on mount. With the new context parameter, this meansUniwindStore.getStylesis called on every re-render (including context-driven ones) for a result that React silently discards.♻️ Proposed fix — use the 3-arg lazy-initializer form
const [unifwindState, recreate] = useReducer( () => UniwindStore.getStyles(className, undefined, undefined, unifwindContext), - UniwindStore.getStyles(className, undefined, undefined, unifwindContext), + undefined, + () => UniwindStore.getStyles(className, undefined, undefined, unifwindContext), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/hooks/useResolveClassNames.native.ts` around lines 8 - 11, The current useReducer call eagerly evaluates UniwindStore.getStyles on every render; change to the 3-arg lazy-initializer form so getStyles runs only on mount. Replace the two-arg useReducer(...) with useReducer(reducer, initialArg, init) where init uses UniwindStore.getStyles(className, undefined, undefined, uniwindContext) to produce the initial state (keep the existing reducer/dispatch name, e.g., recreate, unchanged) so that UniwindStore.getStyles is invoked only inside the init function and not on every render.packages/uniwind/tests/native/components/scoped-theme.test.tsx (2)
35-37: Useawait act(async () => …)to guard against async subscription flush.The synchronous
act(() => { Uniwind.setTheme('dark') })form only drains synchronously scheduled updates. If any part ofUniwindListener's notification path (or a downstreamuseEffect) is deferred (microtask,setTimeout(0), etc.), those state updates land afteract()returns and produce additional "not wrapped in act" warnings. The async form waits for all pending microtasks and effects.♻️ Proposed change (repeat for every `act` call in the file)
- test('Component styles', () => { + test('Component styles', async () => { // ... - act(() => { + await act(async () => { Uniwind.setTheme('dark') }) // ... })Apply the same
async/await act(async () => …)pattern to every test in this file.Also applies to: 63-65, 92-94, 130-132, 168-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/tests/native/components/scoped-theme.test.tsx` around lines 35 - 37, Replace synchronous act calls that wrap theme changes with the async form to ensure all microtasks and effects flush: change occurrences like act(() => { Uniwind.setTheme('dark') }) to await act(async () => { Uniwind.setTheme('dark') }) (and do the same for every similar act call in this test file, including those around lines where Uniwind.setTheme is invoked), so updates from UniwindListener subscriptions and downstream useEffect handlers are fully resolved before assertions.
6-6: Import from the barrel export instead of the platform-specific file.Importing
ScopedThemedirectly fromScopedTheme.nativebypasses the barrel (ScopedTheme/index.ts) and would break if the component is ever renamed or restructured. In a React Native test environment the platform resolver should pick up.nativeautomatically.♻️ Proposed change
-import { ScopedTheme } from '../../../src/components/ScopedTheme/ScopedTheme.native' +import { ScopedTheme } from '../../../src/components/ScopedTheme'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/tests/native/components/scoped-theme.test.tsx` at line 6, The test imports ScopedTheme from the platform-specific file ScopedTheme.native which bypasses the barrel; update the import to use the barrel export (ScopedTheme index) by importing ScopedTheme from the component directory (e.g., '../../../src/components/ScopedTheme') so the platform resolver and future renames/reshuffles work correctly; adjust the import statement referencing the ScopedTheme symbol accordingly.packages/uniwind/src/hooks/useCSSVariable/useCSSVariable.ts (1)
69-77: Stale closure:uniwindContextcaptured at mount won't reflect a dynamicScopedThemetheme change.
updateValueis defined once (empty deps[]) and permanently captures theuniwindContextfrom the initial render. If a parentScopedThemeever re-renders with a differentthemeprop, the subscription callback keeps using the stale context and returns the wrong variable value. This is latent because themes are currently static, but it is a correctness hole for dynamic scoped themes.The idiomatic fix is a ref that tracks the latest context:
♻️ Proposed fix
export const useCSSVariable: UseCSSVariable = (name: string | Array<string>) => { const uniwindContext = useUniwindContext() + const uniwindContextRef = useRef(uniwindContext) + useLayoutEffect(() => { + uniwindContextRef.current = uniwindContext + }) const [value, setValue] = useState(getValue(name, uniwindContext)) // ... useLayoutEffect(() => { - const updateValue = () => setValue(getValue(nameRef.current, uniwindContext)) + const updateValue = () => setValue(getValue(nameRef.current, uniwindContextRef.current)) const dispose = UniwindListener.subscribe( updateValue, [StyleDependency.Theme, StyleDependency.Variables], ) return dispose }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/hooks/useCSSVariable/useCSSVariable.ts` around lines 69 - 77, The subscription callback updateValue inside useLayoutEffect captures the initial uniwindContext and becomes stale; change the hook to track the latest context with a ref (e.g., contextRef) and update contextRef.current whenever uniwindContext changes, then have updateValue read from contextRef.current (and nameRef.current) before calling setValue; keep the UniwindListener.subscribe call and empty deps if you use the ref so the callback always sees the latest context, and ensure you update the ref where uniwindContext is available (the hook body) so UniwindListener.subscribe uses up-to-date data for StyleDependency.Theme/StyleDependency.Variables.packages/uniwind/src/components/ScopedTheme/ScopedTheme.native.tsx (1)
9-15: Optional: memoize the context value to prevent spurious consumer re-renders.The inline
{{ scopedTheme: theme }}object is recreated on every render. Any context consumer will re-render even whenthemehasn't changed.♻️ Proposed refactor
+import React, { useMemo } from 'react' -import React from 'react' import { UniwindContext } from '../../core/context' import { ThemeName } from '../../core/types' type ScopedThemeProps = { theme: ThemeName } export const ScopedTheme: React.FC<React.PropsWithChildren<ScopedThemeProps>> = ({ theme, children }) => { + const value = useMemo(() => ({ scopedTheme: theme }), [theme]) return ( - <UniwindContext.Provider value={{ scopedTheme: theme }}> + <UniwindContext.Provider value={value}> {children} </UniwindContext.Provider> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/components/ScopedTheme/ScopedTheme.native.tsx` around lines 9 - 15, The provider currently creates a new object inline each render causing consumers to re-render; in ScopedTheme (the ScopedTheme component) memoize the context value by creating it with React.useMemo(() => ({ scopedTheme: theme }), [theme]) and pass that memoized value to UniwindContext.Provider so the reference only changes when theme changes.packages/uniwind/src/css-visitor/rule-visitor.ts (1)
3-8: Typo:LightingRuleVisitors→LightningRuleVisitors.The type name is missing the 'n' in "Lightning", inconsistent with
LightningRuleVisitoron line 3.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/css-visitor/rule-visitor.ts` around lines 3 - 8, The type name has a typo: rename the exported/declared type LightingRuleVisitors to LightningRuleVisitors to match LightningRuleVisitor; update the declaration for the Partial mapped type and any usages/caller code that reference LightingRuleVisitors so they use LightningRuleVisitors (ensure imports/exports and any references to the old identifier are updated as well).packages/uniwind/src/core/web/getWebStyles.ts (1)
63-67: Duplicated scopedTheme DOM manipulation.The same set/remove-class logic for
scopedThemeappears in bothgetWebStylesandgetWebVariable. Consider extracting a small helper to reduce duplication:Suggested helper
const applyScopedTheme = (scopedTheme: string | null) => { if (scopedTheme !== null) { dummyParent?.setAttribute('class', scopedTheme) } else { dummyParent?.removeAttribute('class') } }Also applies to: 93-97
🤖 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 63 - 67, Extract the duplicated DOM manipulation for applying/removing the scoped theme from getWebStyles and getWebVariable into a small helper (e.g., applyScopedTheme) that accepts scopedTheme: string | null and manipulates dummyParent accordingly; replace the inline blocks in getWebStyles and getWebVariable with a call to this helper so both use the same logic for uniwindContext.scopedTheme.packages/uniwind/src/core/native/store.ts (2)
95-98: Non-null assertion onthis.vars[theme]is safe but implicit.The
!on line 97 relies ongetStylesbailing out at line 38 when the theme's cache is missing (andvars/cacheare always populated together inreinit). This coupling is sound but not obvious to future readers.A short inline comment explaining why it's safe would help:
- const theme = uniwindContext.scopedTheme ?? this.runtime.currentThemeName - let vars = this.vars[theme]! + const theme = uniwindContext.scopedTheme ?? this.runtime.currentThemeName + // Safe: getStyles returns early if cache (and thus vars) is missing for this theme + let vars = this.vars[theme]!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/native/store.ts` around lines 95 - 98, Add a short inline comment explaining why the non-null assertion on this.vars[theme] is safe: note that getStyles ensures early exit when a theme's cache is missing and reinit always populates vars and cache together, so when execution reaches the block using const theme = uniwindContext.scopedTheme ?? this.runtime.currentThemeName and let vars = this.vars[theme]!, vars is guaranteed to exist; place the comment next to the this.vars[theme]! usage (and/or above the const originalVars = vars line) referencing getStyles and reinit for future readers.
34-40: Cache key lacks delimiters — low risk of collision but fragile.The cache key is built via raw concatenation:
`${className}${isDisabled}${isFocused}${isPressed}${isScopedTheme}`A className ending in
"false"or"true"could theoretically collide with a different className + state combination. Consider using a delimiter (e.g.,\0or|) between segments.I recognize the concatenation pattern predates this PR; flagging since
isScopedThemeis being added now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/native/store.ts` around lines 34 - 40, The cache key construction in store.ts (variable cacheKey built from className, state flags and isScopedTheme) concatenates segments without delimiters and risks collisions; update the cacheKey formation (referencing cacheKey, className, state?.isDisabled, state?.isFocused, state?.isPressed, isScopedTheme, and uniwindContext/runtime.currentThemeName) to join segments with a clear delimiter (e.g., '|' or '\0') and ensure all places that read/write this.cache use the same delimited key format so lookups remain consistent.
🤖 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/package.json`:
- Line 84: package.json currently pins the lightningcss dependency to "1.30.1",
which removes the Android native binary (lightningcss-android-arm64) and will
break Android/Metro builds; either restore the version to "1.30.2" (or upgrade
to "1.31.1") or, if the downgrade is intentional, add a brief comment in
package.json or the PR describing why Android support was removed; update the
"lightningcss" entry in package.json accordingly and include a short
justification in the PR or package.json comment when keeping "1.30.1".
In `@packages/uniwind/src/components/ScopedTheme/ScopedTheme.tsx`:
- Around line 9-16: The Provider in ScopedTheme creates a new inline object each
render which forces all UniwindContext consumers to re-render; change
ScopedTheme to memoize the context value (e.g., useMemo(() => ({ scopedTheme:
theme }), [theme])) and pass that memoized object to UniwindContext.Provider
instead of the inline literal, and apply the identical change in
ScopedTheme.native.tsx so both components only update the context reference when
the theme actually changes.
In `@packages/uniwind/src/core/config/config.native.ts`:
- Around line 39-43: The code calls Object.defineProperty on
UniwindStore.vars[theme] which can be undefined if __reinit hasn't run; update
the updateCSSVariables path to lazily ensure UniwindStore.vars[theme] is an
object before defining the property (e.g., if (!UniwindStore.vars[theme])
UniwindStore.vars[theme] = {}), then proceed to Object.defineProperty with
getValue and varName; reference UniwindStore.vars, updateCSSVariables, __reinit,
varName, and getValue when making the change so the theme bucket is always
initialized at runtime.
In `@packages/uniwind/src/core/types.ts`:
- Line 1: There is a module cycle between types.ts and context.ts because
types.ts imports UniwindContext from context.ts; remove that cross-file type
import and define the UniwindContextType (the shape previously referenced from
context.ts) inline in types.ts instead — replicate only the needed
properties/types (e.g., theme/ThemeName usage) in a local interface/type named
UniwindContextType so context.ts can still import ThemeName (if required)
without creating the circular dependency; update any references in types.ts from
UniwindContext to the new inline UniwindContextType and remove the type-only
import from context.ts.
In `@packages/uniwind/src/css-visitor/rule-visitor.ts`:
- Around line 17-19: The handler for the 'layer-block' rule sets
this.currentLayerName but never restores it, causing later 'style' rules to
incorrectly think they're inside the layer; modify the 'layer-block' handler (in
the LightningRuleVisitor implementation) to save the previous value (e.g., const
prev = this.currentLayerName), set this.currentLayerName =
layer.value.name?.join('') ?? '', allow child rules to be visited, then restore
this.currentLayerName = prev after children are processed; if the visitor API
offers a post-children hook, use that to restore instead so processThemeStyle
and 'style' rule handling only see the layer name while inside the layer-block.
In `@packages/uniwind/src/css-visitor/visitor.ts`:
- Around line 5-12: The RuleVisitor instance holds stateful Sets
(processedClassNames, processedVariables) and is created once in
UniwindCSSVisitor.constructor, causing cross-file state leakage; fix by ensuring
a fresh RuleVisitor per-file either by moving its construction out of the
constructor into the transform hook (create new RuleVisitor(this.themes) at
start of transform) or by adding a reset() method on RuleVisitor that clears
processedClassNames and processedVariables and invoking ruleVisitor.reset() at
the start of each transform; update any references to
RuleVisitor/UniwindCSSVisitor so transform uses the per-file or reset behavior
and preserve FunctionVisitor creation accordingly.
In `@packages/uniwind/tests/native/components/scoped-theme.test.tsx`:
- Around line 14-16: afterEach currently calls Uniwind.setTheme('light') which
notifies UniwindListener subscribers and causes component setValue updates
outside React's act boundary; wrap the theme reset in React's act so state
updates happen inside act (e.g., call act(() => Uniwind.setTheme('light'))),
ensuring any UniwindListener-driven setValue calls are flushed inside act and
silence the "not wrapped in act" warnings.
In `@packages/uniwind/tests/test.css`:
- Around line 17-23: The Stylelint false positives come from the
scss/at-rule-no-unknown rule not recognizing Tailwind's `@variant` at-rule; update
the Stylelint config to silence these by adding "variant" to the
scss/at-rule-no-unknown ignoreAtRules list (i.e., set scss/at-rule-no-unknown:
[true, { ignoreAtRules: ["variant"] }]) so `@variant` in files like the test.css
snippet is treated as valid and CI linting stops failing.
---
Outside diff comments:
In `@packages/uniwind/src/hooks/useResolveClassNames.native.ts`:
- Around line 7-25: The reducer's state can become stale when the UniWind
context changes because uniwindContext (from useUniwindContext) is not included
in the useLayoutEffect dependency arrays; update both useLayoutEffect hooks in
useResolveClassNames.native (and mirror the same fix in the web variant
useResolveClassNames.ts) to include uniwindContext so recreate() is dispatched
when the context changes; ensure the UniwindListener.subscribe call still uses
uniwindState.dependencies and that dependencySum and className remain in the
deps along with uniwindContext so UniwindStore.getStyles (the reducer init) is
re-evaluated with the new uniwindContext.
In `@packages/uniwind/src/hooks/useResolveClassNames.ts`:
- Around line 9-25: The effect in useResolveClassNames.ts only depends on
className so it never re-runs when scopedTheme changes; because
useUniwindContext() provides scopedTheme to getWebStyles (which uses
dummyParent), you must include the context-derived value in the dependency array
so recreate() is called on theme changes. Update the useLayoutEffect that calls
recreate() and CSSListener.subscribeToClassName(className, recreate) to include
the relevant context value (e.g., uniwindContext.scopedTheme or the whole
uniwindContext) alongside className; ensure the initial useReducer initializer
also re-computes when that context value changes (mirror the dependency change).
Also audit the native file useResolveClassNames.native.ts for similar missing
dependencies (replace/add uniwindState.scopedTheme or uniwindState/dependencySum
as appropriate).
---
Duplicate comments:
In `@packages/uniwind/src/vite/vite.ts`:
- Line 55: The shared visitor instance is created once in config() via new
UniwindCSSVisitor(themes) and thus accumulates state across CSS transforms; to
fix, ensure each transform gets a fresh visitor or that the visitor is reset
per-file: either instantiate a new UniwindCSSVisitor inside the transform hook
instead of reusing the one created in config(), or add and call a reset() method
on UniwindCSSVisitor (and its RuleVisitor Sets) at the start of each transform;
update the config() call site that currently does visitor: new
UniwindCSSVisitor(themes) and the transform pipeline to use the per-transform
instantiation or call the new reset() hook on UniwindCSSVisitor before
processing each file.
---
Nitpick comments:
In `@packages/uniwind/src/components/ScopedTheme/ScopedTheme.native.tsx`:
- Around line 9-15: The provider currently creates a new object inline each
render causing consumers to re-render; in ScopedTheme (the ScopedTheme
component) memoize the context value by creating it with React.useMemo(() => ({
scopedTheme: theme }), [theme]) and pass that memoized value to
UniwindContext.Provider so the reference only changes when theme changes.
In `@packages/uniwind/src/core/context.ts`:
- Around line 4-6: Set a displayName on the UniwindContext so React DevTools
shows a meaningful label: locate the UniwindContext created with createContext
(symbol UniwindContext) and after its creation assign UniwindContext.displayName
= "UniwindContext" (or another descriptive name) to improve debugging
visibility.
In `@packages/uniwind/src/core/native/store.ts`:
- Around line 95-98: Add a short inline comment explaining why the non-null
assertion on this.vars[theme] is safe: note that getStyles ensures early exit
when a theme's cache is missing and reinit always populates vars and cache
together, so when execution reaches the block using const theme =
uniwindContext.scopedTheme ?? this.runtime.currentThemeName and let vars =
this.vars[theme]!, vars is guaranteed to exist; place the comment next to the
this.vars[theme]! usage (and/or above the const originalVars = vars line)
referencing getStyles and reinit for future readers.
- Around line 34-40: The cache key construction in store.ts (variable cacheKey
built from className, state flags and isScopedTheme) concatenates segments
without delimiters and risks collisions; update the cacheKey formation
(referencing cacheKey, className, state?.isDisabled, state?.isFocused,
state?.isPressed, isScopedTheme, and uniwindContext/runtime.currentThemeName) to
join segments with a clear delimiter (e.g., '|' or '\0') and ensure all places
that read/write this.cache use the same delimited key format so lookups remain
consistent.
In `@packages/uniwind/src/core/web/getWebStyles.ts`:
- Around line 63-67: Extract the duplicated DOM manipulation for
applying/removing the scoped theme from getWebStyles and getWebVariable into a
small helper (e.g., applyScopedTheme) that accepts scopedTheme: string | null
and manipulates dummyParent accordingly; replace the inline blocks in
getWebStyles and getWebVariable with a call to this helper so both use the same
logic for uniwindContext.scopedTheme.
In `@packages/uniwind/src/css-visitor/rule-visitor.ts`:
- Around line 3-8: The type name has a typo: rename the exported/declared type
LightingRuleVisitors to LightningRuleVisitors to match LightningRuleVisitor;
update the declaration for the Partial mapped type and any usages/caller code
that reference LightingRuleVisitors so they use LightningRuleVisitors (ensure
imports/exports and any references to the old identifier are updated as well).
In `@packages/uniwind/src/hooks/useCSSVariable/useCSSVariable.ts`:
- Around line 69-77: The subscription callback updateValue inside
useLayoutEffect captures the initial uniwindContext and becomes stale; change
the hook to track the latest context with a ref (e.g., contextRef) and update
contextRef.current whenever uniwindContext changes, then have updateValue read
from contextRef.current (and nameRef.current) before calling setValue; keep the
UniwindListener.subscribe call and empty deps if you use the ref so the callback
always sees the latest context, and ensure you update the ref where
uniwindContext is available (the hook body) so UniwindListener.subscribe uses
up-to-date data for StyleDependency.Theme/StyleDependency.Variables.
In `@packages/uniwind/src/hooks/useResolveClassNames.native.ts`:
- Around line 8-11: The current useReducer call eagerly evaluates
UniwindStore.getStyles on every render; change to the 3-arg lazy-initializer
form so getStyles runs only on mount. Replace the two-arg useReducer(...) with
useReducer(reducer, initialArg, init) where init uses
UniwindStore.getStyles(className, undefined, undefined, uniwindContext) to
produce the initial state (keep the existing reducer/dispatch name, e.g.,
recreate, unchanged) so that UniwindStore.getStyles is invoked only inside the
init function and not on every render.
In `@packages/uniwind/tests/native/components/scoped-theme.test.tsx`:
- Around line 35-37: Replace synchronous act calls that wrap theme changes with
the async form to ensure all microtasks and effects flush: change occurrences
like act(() => { Uniwind.setTheme('dark') }) to await act(async () => {
Uniwind.setTheme('dark') }) (and do the same for every similar act call in this
test file, including those around lines where Uniwind.setTheme is invoked), so
updates from UniwindListener subscriptions and downstream useEffect handlers are
fully resolved before assertions.
- Line 6: The test imports ScopedTheme from the platform-specific file
ScopedTheme.native which bypasses the barrel; update the import to use the
barrel export (ScopedTheme index) by importing ScopedTheme from the component
directory (e.g., '../../../src/components/ScopedTheme') so the platform resolver
and future renames/reshuffles work correctly; adjust the import statement
referencing the ScopedTheme symbol accordingly.
In `@packages/uniwind/tests/web/hoc/withUniwind.test.tsx`:
- Around line 97-116: The manual color test currently only checks the resolved
prop value and misses verifying that withUniwind/threading passes the uniWind
context into webCore.getWebStyles; after rendering ManualWithUniwind, add an
assertion that the mocked webCore.getWebStyles (mockGetWebStyles) was called
with the UNIWIND_CONTEXT_MOCK context (same guard as the [auto] test) so
regressions that drop the context are caught—i.e., after render and before
inspecting ComponentWithSpy.mock.calls, assert that mockGetWebStyles was invoked
with UNIWIND_CONTEXT_MOCK (matching how the [auto] test verifies getWebStyles).
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/uniwind/tests/test.css`:
- Around line 16-25: The CSS custom properties are declared inside `@layer` theme
(symbols: `@layer` theme, :root, `@variant` dark, `@variant` light,
--color-background) but Tailwind v4 assigns `theme` the lowest cascade priority
so these vars can be shadowed by higher layers; fix by moving the :root variable
declarations out of `@layer` theme or into a higher-priority layer such as `@layer`
base (or place them at the top-level :root) and ensure dark/light `@variant`
blocks remain adjacent to those declarations, then audit any component/utility
rules that set background values to use var(--color-background) instead of fixed
colors.
---
Duplicate comments:
In `@packages/uniwind/tests/test.css`:
- Line 18: The Stylelint false positives for the `@variant` at-rule mean the
Stylelint config hasn't been updated to ignore it; update the Stylelint config
used by the project (the rule scss/at-rule-no-unknown or at-rule-no-unknown
settings) to include "variant" in ignoreAtRules (or add "variant" to the scss
plugin's allowed at-rules), then re-run linting so the `@variant` usages (e.g.,
the `@variant` dark block in test.css) no longer trigger errors.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/uniwind/src/components/ScopedTheme/ScopedTheme.native.tsx (1)
3-3: Useimport typefor type-only imports, consistent with the web counterpart.
ThemeNameandUniwindContextTypeare pure type aliases. The web version (ScopedTheme.tsx, line 3) correctly usesimport type, but this file uses a value import. This inconsistency can cause issues withverbatimModuleSyntaxorisolatedModulesand may leave residual imports in the bundle output.♻️ Proposed fix
-import { ThemeName, UniwindContextType } from '../../core/types' +import type { ThemeName, UniwindContextType } from '../../core/types'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/components/ScopedTheme/ScopedTheme.native.tsx` at line 3, The import at the top currently treats ThemeName and UniwindContextType as values; change it to a type-only import so the TS compiler/tree-shaker treats them as types: replace the current import with an `import type { ThemeName, UniwindContextType } from '../../core/types'` in ScopedTheme.native.tsx (matching the web counterpart) to avoid residual runtime imports and issues with isolatedModules/verbatimModuleSyntax.packages/uniwind/src/core/native/store.ts (1)
34-36: Cache key uses string-concatenated booleans without delimiters.The cache key is built by concatenating raw boolean strings without separators. While practical collisions are extremely unlikely with typical Tailwind class names, using a delimiter (e.g.
|) would make the key unambiguous and easier to debug.Proposed fix
- const cacheKey = `${className}${state?.isDisabled ?? false}${state?.isFocused ?? false}${state?.isPressed ?? false}${isScopedTheme}` + const cacheKey = `${className}|${state?.isDisabled ?? false}|${state?.isFocused ?? false}|${state?.isPressed ?? false}|${isScopedTheme}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/native/store.ts` around lines 34 - 36, The cacheKey construction in store.ts concatenates className and boolean state flags without separators (see cacheKey, className, state, isScopedTheme), risking ambiguous keys; update the cacheKey to join parts with a clear delimiter (e.g. '|') between className, state?.isDisabled, state?.isFocused, state?.isPressed and isScopedTheme so each component is unambiguous and easier to debug while leaving the lookup using this.cache[uniwindContext.scopedTheme ?? this.runtime.currentThemeName] unchanged.packages/uniwind/src/hooks/useCSSVariable/useCSSVariable.ts (1)
51-67: MissinguniwindContextin the dependency array may cause a stale read.If the scoped theme changes without
namechanging, this effect won't fire, so thesetValuecalls at lines 57/64 would use a staleuniwindContextcaptured from a previous render ifnamealso happens to change in a subsequent render before the second effect has a chance to update the value.The second
useLayoutEffect(line 69–77) does re-subscribe onuniwindContextchanges and will eventually update the value, so this is unlikely to cause a visible bug in practice. However, addinguniwindContextto the dependency array here would make both effects consistent and eliminate the stale-closure window entirely.Proposed fix
- }, [name]) + }, [name, uniwindContext])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/hooks/useCSSVariable/useCSSVariable.ts` around lines 51 - 67, The effect using useLayoutEffect currently only depends on name which can capture a stale uniwindContext; update the dependency array for that effect to include uniwindContext so the closure sees the latest scoped theme when calling getValue and setValue (i.e., change the dependency list on the useLayoutEffect that inspects Array.isArray(name) and nameRef.current from [name] to [name, uniwindContext]); keep the existing array-equality and nameRef updates logic intact.packages/uniwind/src/css-visitor/rule-visitor.ts (1)
11-12:processedVariablesis a misleading name — it stores theme/variant names, not CSS variables.
processedVariablestracks deduplicated theme names (e.g.'dark','light'), not CSS custom properties. This contradicts the name, especially since the broader CSS theming context does deal with CSS variables. Consider renaming toprocessedThemesorprocessedVariantsfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/css-visitor/rule-visitor.ts` around lines 11 - 12, The variable processedVariables is misleading because it stores theme/variant names; rename it to processedThemes (or processedVariants) across the file to reflect its purpose: update the declaration (processedVariables = new Set<string>() → processedThemes = new Set<string>()), replace every reference to processedVariables with processedThemes in functions/methods inside rule-visitor.ts (e.g., where you check/add to the set), and update any exports, type annotations, or tests that reference processedVariables so all identifiers remain consistent.
🤖 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/css-visitor/rule-visitor.ts`:
- Line 70: The current theme detection uses substring matching: replace the
includes check in the selectedVariant computation (selectedVariant =
this.themes.find(theme => firstSelector.name.includes(`${theme}:`))) with a
prefix check so it only matches theme variants at the start of the selector (use
startsWith on firstSelector.name with `${theme}:`), ensuring correct detection
of theme prefixes when computing selectedVariant from this.themes.
- Around line 78-88: In rule-visitor.ts update the object literals used for
class tokens so they are typed as literal tuples like in processThemeStyle: add
"as const" to the { type: 'class', name: selectedVariant } in scopeStart and to
the { type: 'class', name: theme } inside the .map() for scopeEnd so the
callbacks on scopeStart/scopeEnd infer literal types instead of {type: string;
name: string}; this keeps type-safety and consistency with processThemeStyle.
---
Duplicate comments:
In `@packages/uniwind/src/css-visitor/rule-visitor.ts`:
- Around line 17-19: The 'layer-block' handler currently mutates
this.currentLayerName without restoring it, causing later sibling style rules to
be misattributed to the theme; fix by implementing a stack-based save/restore:
add a this.layerStack: string[] field, in the 'layer-block' handler push the new
name (e.g. const name = layer.value.name?.join('') ?? '';
this.layerStack.push(name); this.currentLayerName = name), and then ensure you
pop the stack when exiting the layer block and restore this.currentLayerName to
the new top (or '' if empty); also update the style handler to read the active
layer from the stack/top-of-stack rather than relying on a single
unreliably-mutated this.currentLayerName so processThemeStyle is only invoked
when the top stack entry === 'theme'.
---
Nitpick comments:
In `@packages/uniwind/src/components/ScopedTheme/ScopedTheme.native.tsx`:
- Line 3: The import at the top currently treats ThemeName and
UniwindContextType as values; change it to a type-only import so the TS
compiler/tree-shaker treats them as types: replace the current import with an
`import type { ThemeName, UniwindContextType } from '../../core/types'` in
ScopedTheme.native.tsx (matching the web counterpart) to avoid residual runtime
imports and issues with isolatedModules/verbatimModuleSyntax.
In `@packages/uniwind/src/core/native/store.ts`:
- Around line 34-36: The cacheKey construction in store.ts concatenates
className and boolean state flags without separators (see cacheKey, className,
state, isScopedTheme), risking ambiguous keys; update the cacheKey to join parts
with a clear delimiter (e.g. '|') between className, state?.isDisabled,
state?.isFocused, state?.isPressed and isScopedTheme so each component is
unambiguous and easier to debug while leaving the lookup using
this.cache[uniwindContext.scopedTheme ?? this.runtime.currentThemeName]
unchanged.
In `@packages/uniwind/src/css-visitor/rule-visitor.ts`:
- Around line 11-12: The variable processedVariables is misleading because it
stores theme/variant names; rename it to processedThemes (or processedVariants)
across the file to reflect its purpose: update the declaration
(processedVariables = new Set<string>() → processedThemes = new Set<string>()),
replace every reference to processedVariables with processedThemes in
functions/methods inside rule-visitor.ts (e.g., where you check/add to the set),
and update any exports, type annotations, or tests that reference
processedVariables so all identifiers remain consistent.
In `@packages/uniwind/src/hooks/useCSSVariable/useCSSVariable.ts`:
- Around line 51-67: The effect using useLayoutEffect currently only depends on
name which can capture a stale uniwindContext; update the dependency array for
that effect to include uniwindContext so the closure sees the latest scoped
theme when calling getValue and setValue (i.e., change the dependency list on
the useLayoutEffect that inspects Array.isArray(name) and nameRef.current from
[name] to [name, uniwindContext]); keep the existing array-equality and nameRef
updates logic intact.
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/hooks/useUniwind.ts (1)
14-21:⚠️ Potential issue | 🟠 MajorSync theme state immediately when exiting a scoped theme context.
When
scopedThemetransitions from non-null to null (e.g., when a<ScopedTheme>unmounts), the local state forthemeandhasAdaptiveThemesmay be stale if they changed while the early return was active. SinceUniwindListener.subscribeonly registers callbacks without emitting immediately, state remains stale until the next theme change notification.Add initial state synchronization before subscribing:
Suggested fix
useLayoutEffect(() => { if (uniwindContext.scopedTheme !== null) { return } + setTheme(Uniwind.currentTheme) + setHasAdaptiveThemes(Uniwind.hasAdaptiveThemes) const dispose = UniwindListener.subscribe(() => { setTheme(Uniwind.currentTheme) setHasAdaptiveThemes(Uniwind.hasAdaptiveThemes) }, [StyleDependency.Theme, StyleDependency.AdaptiveThemes])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/hooks/useUniwind.ts` around lines 14 - 21, When scopedTheme becomes null the hook should synchronously update local state before registering the listener to avoid stale values; in useUniwind.ts, inside the branch where uniwindContext.scopedTheme === null, call setTheme(Uniwind.currentTheme) and setHasAdaptiveThemes(Uniwind.hasAdaptiveThemes) immediately prior to creating the subscription with UniwindListener.subscribe, then proceed to assign the returned dispose; ensure you still return the dispose cleanup as before (references: uniwindContext.scopedTheme, setTheme, setHasAdaptiveThemes, Uniwind.currentTheme, Uniwind.hasAdaptiveThemes, UniwindListener.subscribe).
♻️ Duplicate comments (1)
packages/uniwind/tests/native/components/scoped-theme.test.tsx (1)
14-17:⚠️ Potential issue | 🟠 MajorWrap the theme reset in
act()to avoid state updates outside React’s test boundary.The theme reset triggers subscribed updates; without
act()this can emit warnings or fail tests.🐛 Proposed fix
- afterEach(() => { - Uniwind.setTheme('light') - }) + afterEach(() => { + act(() => { + Uniwind.setTheme('light') + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/tests/native/components/scoped-theme.test.tsx` around lines 14 - 17, The afterEach cleanup is calling Uniwind.setTheme('light') which triggers subscriber updates outside React's test boundary; wrap that call in React's act() (import act from your test utils or react-test-renderer/@testing-library/react as used elsewhere) so the theme reset happens inside act(() => { Uniwind.setTheme('light') }) to avoid warnings/failing tests; update the afterEach block that references Uniwind.setTheme to use act and add the necessary act import if it's not already present.
🧹 Nitpick comments (1)
packages/uniwind/src/hooks/useUniwind.ts (1)
26-26: Narrow the effect dependency to avoid resubscribe churn.The effect only reads
uniwindContext.scopedTheme(checked on line 14 and used in the return values). Depending on the entire context object causes the effect to retrigger whenever the provider re-creates the context value, even ifscopedThemehasn't changed. SincescopedThemeis the only field inUniwindContext, depend only on it.♻️ Suggested refinement
- }, [uniwindContext]) + }, [uniwindContext.scopedTheme])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/hooks/useUniwind.ts` at line 26, The effect in useUniwind currently depends on the entire uniwindContext causing unnecessary re-runs; change the dependency array of the effect inside the useUniwind hook to depend only on uniwindContext.scopedTheme (i.e., replace [uniwindContext] with [uniwindContext.scopedTheme]) so the effect only re-subscribes when scopedTheme changes and avoid churn when the provider recreates the context object.
🤖 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/hooks/useUniwind.ts`:
- Around line 14-21: When scopedTheme becomes null the hook should synchronously
update local state before registering the listener to avoid stale values; in
useUniwind.ts, inside the branch where uniwindContext.scopedTheme === null, call
setTheme(Uniwind.currentTheme) and
setHasAdaptiveThemes(Uniwind.hasAdaptiveThemes) immediately prior to creating
the subscription with UniwindListener.subscribe, then proceed to assign the
returned dispose; ensure you still return the dispose cleanup as before
(references: uniwindContext.scopedTheme, setTheme, setHasAdaptiveThemes,
Uniwind.currentTheme, Uniwind.hasAdaptiveThemes, UniwindListener.subscribe).
---
Duplicate comments:
In `@packages/uniwind/tests/native/components/scoped-theme.test.tsx`:
- Around line 14-17: The afterEach cleanup is calling Uniwind.setTheme('light')
which triggers subscriber updates outside React's test boundary; wrap that call
in React's act() (import act from your test utils or
react-test-renderer/@testing-library/react as used elsewhere) so the theme reset
happens inside act(() => { Uniwind.setTheme('light') }) to avoid
warnings/failing tests; update the afterEach block that references
Uniwind.setTheme to use act and add the necessary act import if it's not already
present.
---
Nitpick comments:
In `@packages/uniwind/src/hooks/useUniwind.ts`:
- Line 26: The effect in useUniwind currently depends on the entire
uniwindContext causing unnecessary re-runs; change the dependency array of the
effect inside the useUniwind hook to depend only on uniwindContext.scopedTheme
(i.e., replace [uniwindContext] with [uniwindContext.scopedTheme]) so the effect
only re-subscribes when scopedTheme changes and avoid churn when the provider
recreates the context object.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/uniwind/src/hooks/useUniwind.tspackages/uniwind/tests/native/components/scoped-theme.test.tsx
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 by CodeRabbit
New Features
Bug Fixes
Tests