Fix CSS variable updates for scoped / non-global themes#498
Fix CSS variable updates for scoped / non-global themes#498sascha-kaleidoscode wants to merge 2 commits intouni-stack:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds theme-scoped CSS variable notification and filtering: a new UniwindListener.notifyVariables(theme) path, a shouldNotifyVariables subscription option, runtime CSS variable lookup method, and changes to subscriptions so variable updates only trigger listeners whose scoped theme matches the updated theme. Changes
Sequence DiagramsequenceDiagram
participant Component as Component/Hook
participant Config as UniwindConfig
participant Listener as UniwindListener
participant Context as UniwindContext
participant Callback as Subscription Callback
Component->>Config: updateCSSVariables(theme='custom', vars)
Config->>Listener: notifyVariables('custom')
Listener->>Listener: set notifyingVariablesTheme = 'custom'
Listener->>Callback: invoke Variables dependency handlers
Callback->>Context: read scopedTheme / effectiveTheme
Callback->>Listener: shouldNotifyVariables('custom')?
alt Theme matches scoped theme
Listener-->>Callback: true
Callback->>Component: execute callback (rerender)
else Theme does not match
Listener-->>Callback: false
Callback->>Component: skip execution
end
Listener->>Listener: clear notifyingVariablesTheme
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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: 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/hoc/withUniwind.native.tsx`:
- Around line 76-78: The subscription's shouldNotifyVariables closure captures
the current effective theme (uniwindContext.scopedTheme ??
UniwindStore.runtime.currentThemeName) but the effect is only keyed by
dependencySum, so change the effect(s) that call UniwindListener.subscribe
(search for UniwindListener.subscribe, rerender, dependencySum,
shouldNotifyVariables, uniwindContext.scopedTheme,
UniwindStore.runtime.currentThemeName) to also include the effective theme value
in their dependency array (e.g. compute const effectiveTheme =
uniwindContext.scopedTheme ?? UniwindStore.runtime.currentThemeName and add
effectiveTheme to the dependency list) so the subscription is torn down and
recreated whenever the scoped/global theme changes; apply the same change to the
other occurrences noted (lines around the other subscribe calls).
In `@packages/uniwind/src/hooks/useResolveClassNames.native.ts`:
- Around line 22-24: The subscription's shouldNotifyVariables closure captures
the current effective theme (uniwindContext.scopedTheme ??
UniwindStore.runtime.currentThemeName) but the subscribe call only depends on
uniwindState.dependencies, so the listener never re-subscribes when the
effective theme changes; update the subscribe invocation in
useResolveClassNames.native (and the analogous location around line 28) to
include the computed effective theme as part of the dependencies passed to
UniwindListener.subscribe (or otherwise trigger recreate when it changes) so
that recreate is re-registered whenever uniwindContext.scopedTheme or
UniwindStore.runtime.currentThemeName changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24d63485-2ec9-4110-80f4-a7c7ff023634
📒 Files selected for processing (12)
packages/uniwind/src/components/native/useStyle.tspackages/uniwind/src/core/config/config.native.tspackages/uniwind/src/core/config/config.tspackages/uniwind/src/core/listener.tspackages/uniwind/src/core/native/store.tspackages/uniwind/src/core/web/getWebStyles.tspackages/uniwind/src/hoc/withUniwind.native.tsxpackages/uniwind/src/hooks/useCSSVariable/useCSSVariable.tspackages/uniwind/src/hooks/useResolveClassNames.native.tspackages/uniwind/tests/native/components/scoped-theme.test.tsxpackages/uniwind/tests/setup.native.tspackages/uniwind/tests/web/scoped-theme-css-variables.test.tsx
|
Thank you for taking the time to investigate this and for putting together a PR. I really appreciate your effort to improve Uniwind and document the issue with That said, for future reference, please always open an Issue to discuss a bug before jumping into a PR. This helps us align on the problem and the best approach before anyone spends time writing code. Looking at the proposed solution, this touches on several core pieces of Uniwind's logic. Because these systems are highly interconnected and complex, I prefer to handle modifications to the core architecture myself to ensure edge cases are covered and the broader system remains stable. We've totally missed updateCSSVariables while working on ScopedThemes I noticed during testing that even with this PR, the basic case on the web is still not functioning correctly. <ScopedTheme theme="premium">
<View className="bg-background size-20" />
</ScopedTheme>
<Button
title="Update"
onPress={() =>
Uniwind.updateCSSVariables('dark', {
'--color-background': '#ff0000',
})}
/>Because of the architectural changes required, I'm pretty sure that I would start new PR from scratch. However, your PR description explains the problem perfectly. Could you copy your "Problem" section into a new Issue so we can track the bug properly? Thanks again for bringing this to my attention and for your support of the project! |
|
Thanks for taking the time to check the PR and your explanation. Maybe it's just a copy/paste mistake, but the case you've described has the wrong theme name in |
Yup, it should be premium |
2 participants
Problem
Uniwind.updateCSSVariables(theme, variables)did not reliably trigger UI updates for components rendered under<ScopedTheme>when the updatedthemediffered from the globally active theme.On web, runtime variable overrides for non-global themes were also not consistently observable via
getWebVariable/useCSSVariable, because resolution depended ondocument.documentElementand computed styles without consulting per-theme runtime state.Solution
scopedThemewhen present, otherwise the global theme).getWebVariableagainst the per-theme runtime map for the effective theme before falling back togetComputedStyle.Testing
Summary by CodeRabbit
New Features
Tests