Feat/scoped variables - #611
Conversation
Demonstrate per-subtree CSS variable overrides in the expo example app: theme default, scoped override, nested inheritance (nearest wins), and the opt-in cacheKey. Adds --color-primary/--color-surface/--gap theme defaults so the unscoped baseline renders intentionally.
On web, Uniwind passes classes through RNW unchanged, so real elements resolve `var(--name)` from the live CSS cascade. The wrapper only applied its variables to the hidden dummyParent used for JS reads, so scoped overrides never reached descendants — styling stayed at the theme default while useCSSVariable readouts (which use the dummyParent path) looked correct. Set the variables as inline custom properties on the display:contents wrapper so they cascade to children (numbers -> px). Add web regression tests asserting the wrapper carries the overrides inline, nested wrappers only declare their own overrides, and invalid keys are dropped. Rework the expo-example demo with origin pills (default/set here/inherited), swatches, and a gap strip so the override/inherit story is legible.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a cross-platform ChangesScopedVariables feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant ScopedVariables
participant Runtime
participant CSSVariableHook
Component->>ScopedVariables: provide variables
ScopedVariables->>Runtime: merge context and resolve styles
Runtime->>CSSVariableHook: expose scoped variable value
CSSVariableHook-->>Component: return resolved value
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryIntroduces
Confidence Score: 5/5Safe to merge; all correctness and cleanup paths are properly guarded and well-tested. The feature is self-contained and both web and native paths are well-covered by tests. The try/finally cleanup on the DOM side is correctly in place. The two concerns flagged are limited to a DRY violation between two native-only files and a cache-entry accumulation that only materialises under frequent variable cycling with no concurrent global variable updates — neither affects correctness or typical usage. packages/uniwind/src/core/native/store.ts and packages/uniwind/src/hooks/useCSSVariable/getVariableValue.native.ts share identical overlay-construction logic that should be extracted to a shared utility. Important Files Changed
Sequence DiagramsequenceDiagram
participant Parent as Parent Component
participant SV as ScopedVariables
participant Ctx as UniwindContext
participant Child as Child Component
participant Store as UniwindStore (native)
participant DOM as dummyParent (web)
Parent->>SV: render(variables)
SV->>SV: validateVariables(variables)
SV->>SV: "mergedVars = spread parent + own"
SV->>SV: "variablesCacheKey = JSON.stringify(sorted)"
SV->>Ctx: Provider value with variables + variablesCacheKey
Note over SV,DOM: Web only — inline custom props on display:contents div
Child->>Ctx: useUniwindContext()
Ctx-->>Child: ctx with variables set
alt Native getStyles()
Child->>Store: getStyles(className, state, ctx)
Store->>Store: cacheKey includes variablesCacheKey
Store->>Store: "overlay = Object.create(themeVars) + scoped getters"
Store-->>Child: resolved RNStyle
end
alt Web getWebStyles()
Child->>DOM: applyScopedVariables(ctx)
DOM->>DOM: setProperty(name, toWebValue(val))
DOM->>DOM: getComputedStyle → resolve
DOM->>DOM: finally removeProperty(name)
DOM-->>Child: resolved style object
end
alt useCSSVariable
Child->>Child: useState(getCSSVariable(name, ctx))
Child->>Child: useLayoutEffect([ctx]) isMountRef skip on mount
Note over Child: On ctx change — updateValue() + re-subscribe
end
Reviews (4): Last reviewed commit: "fix(ScopedVariables): use JSON.stringify..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
packages/uniwind/src/components/ScopedVariables/utils.ts (1)
24-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInvalid-key filtering is dev-only; downstream consumers diverge in production.
validateVariablesskips filtering entirely when!__DEV__(Line 25-27), butScopedVariables.tsx's inline-style loop always filters non---keys regardless of environment. In a production build this means an invalid key stays in the exposedcontext.variables(read bygetVariableValue.native.tsandapplyScopedVariablesingetWebStyles.ts) while never appearing as an actual inline custom property — a silent, hard-to-diagnose inconsistency between the two paths. Consider always filtering and only gating theLogger.errorcall behind__DEV__.♻️ Proposed fix
const validateVariables = (variables: CSSVariables) => { - if (!__DEV__) { - return variables - } - return Object.fromEntries( Object.entries(variables).filter(([name]) => { if (!name.startsWith('--')) { - Logger.error(`CSS variable name must start with "--", instead got: ${name}`) + if (__DEV__) { + Logger.error(`CSS variable name must start with "--", instead got: ${name}`) + } return false } return true }), ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/uniwind/src/components/ScopedVariables/utils.ts` around lines 24 - 40, Update validateVariables so it always removes variable names that do not start with "--", regardless of __DEV__. Restrict only the Logger.error call to development builds, preserving the filtered result for production consumers such as context.variables and inline-style processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/expo-example/ScopedVariablesDemo.tsx`:
- Around line 124-131: Update the cached subtree example in ScopedVariablesDemo,
specifically the ScopedVariables declaration with cacheKey="demo-amber", to
reuse the exact --color-primary and --gap values from section 2 while leaving
only the cacheKey difference. Keep the surrounding heading and AccentCard usage
unchanged.
In `@packages/uniwind/tests/native/components/scoped-variables.test.tsx`:
- Around line 163-182: Remove the unused `@ts-expect-error` directive from the
ScopedVariables test; the variables object already satisfies
ScopedVariablesProps and should remain unchanged so the test continues
validating the runtime warning and valid --gap styling.
In `@packages/uniwind/tests/web/components/scoped-variables.test.tsx`:
- Around line 8-31: Extend the scoped custom-property test around Probe and
ScopedVariables to update the provider’s variables prop after the initial
render, then assert that the subscribed useCSSVariable result rerenders with the
new DOM-cascaded value. Preserve the existing outside fallback assertion and
verify the inside consumer reflects both the initial and updated values.
- Around line 106-109: Remove the unused `@ts-expect-error` directive from the
invalid-keys test when rendering ScopedVariables. Keep the variables object
unchanged so the runtime filtering of the valid CSSVariables prop continues to
be tested.
---
Nitpick comments:
In `@packages/uniwind/src/components/ScopedVariables/utils.ts`:
- Around line 24-40: Update validateVariables so it always removes variable
names that do not start with "--", regardless of __DEV__. Restrict only the
Logger.error call to development builds, preserving the filtered result for
production consumers such as context.variables and inline-style processing.
🪄 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: bcd8a38c-205e-43b8-9fea-e785e0254907
📒 Files selected for processing (22)
CONTEXT.mdapps/expo-example/App.tsxapps/expo-example/ScopedVariablesDemo.tsxapps/expo-example/global.csspackages/uniwind/src/components/ScopedVariables/ScopedVariables.native.tsxpackages/uniwind/src/components/ScopedVariables/ScopedVariables.tsxpackages/uniwind/src/components/ScopedVariables/index.tspackages/uniwind/src/components/ScopedVariables/utils.tspackages/uniwind/src/core/config/config.common.tspackages/uniwind/src/core/config/config.native.tspackages/uniwind/src/core/config/config.tspackages/uniwind/src/core/context.tspackages/uniwind/src/core/native/native-utils.tspackages/uniwind/src/core/native/store.tspackages/uniwind/src/core/web/getWebStyles.tspackages/uniwind/src/hooks/useCSSVariable/getVariableValue.native.tspackages/uniwind/src/index.tspackages/uniwind/tests/consts.tspackages/uniwind/tests/e2e/getWebStyles.test.tspackages/uniwind/tests/native/components/scoped-variables.test.tsxpackages/uniwind/tests/type-test/theme.tspackages/uniwind/tests/web/components/scoped-variables.test.tsx
| {/* 4. Same as override, plus an opt-in stable cacheKey (native caching) */} | ||
| <SectionHeader | ||
| title="4. Cached subtree (cacheKey)" | ||
| subtitle="Same overrides as #2, but a stable cacheKey re-enables the native style cache." | ||
| /> | ||
| <ScopedVariables variables={{ '--color-primary': '#f59e0b', '--gap': 8 }} cacheKey="demo-amber"> | ||
| <AccentCard title="Amber card" primary="override" gap="override" /> | ||
| </ScopedVariables> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the cache-key example’s overrides identical.
Line 127 says this is the same as section 2 except for cacheKey, but Line 129 changes both values. Reuse section 2’s values so the demo isolates cache behavior.
Proposed fix
- <ScopedVariables variables={{ '--color-primary': '`#f59e0b`', '--gap': 8 }} cacheKey="demo-amber">
- <AccentCard title="Amber card" primary="override" gap="override" />
+ <ScopedVariables variables={{ '--color-primary': '`#e11d48`', '--gap': 16 }} cacheKey="demo-red">
+ <AccentCard title="Cached card" primary="override" gap="override" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* 4. Same as override, plus an opt-in stable cacheKey (native caching) */} | |
| <SectionHeader | |
| title="4. Cached subtree (cacheKey)" | |
| subtitle="Same overrides as #2, but a stable cacheKey re-enables the native style cache." | |
| /> | |
| <ScopedVariables variables={{ '--color-primary': '#f59e0b', '--gap': 8 }} cacheKey="demo-amber"> | |
| <AccentCard title="Amber card" primary="override" gap="override" /> | |
| </ScopedVariables> | |
| {/* 4. Same as override, plus an opt-in stable cacheKey (native caching) */} | |
| <SectionHeader | |
| title="4. Cached subtree (cacheKey)" | |
| subtitle="Same overrides as `#2`, but a stable cacheKey re-enables the native style cache." | |
| /> | |
| <ScopedVariables variables={{ '--color-primary': '`#e11d48`', '--gap': 16 }} cacheKey="demo-red"> | |
| <AccentCard title="Cached card" primary="override" gap="override" /> | |
| </ScopedVariables> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/expo-example/ScopedVariablesDemo.tsx` around lines 124 - 131, Update the
cached subtree example in ScopedVariablesDemo, specifically the ScopedVariables
declaration with cacheKey="demo-amber", to reuse the exact --color-primary and
--gap values from section 2 while leaving only the cacheKey difference. Keep the
surrounding heading and AccentCard usage unchanged.
- useCSSVariable: recompute on context change, not only on global Theme/Variables events — an updated <ScopedVariables> variables prop (or a nearer provider) now surfaces the new value. Adds web + native regression tests for a prop update. - utils: always drop non-`--` keys (not just in dev), gating only the Logger.error behind __DEV__, so invalid keys can't reach the web read helper and corrupt a resolved inheritable property in production. - getWebStyles/getWebVariable: wrap the scoped-variable read in try/finally so the temporary custom properties are always cleared, even if a DOM read throws. - Document the variables prop stability contract (define outside render or useMemo) in JSDoc. - Remove two unused @ts-expect-error directives in tests. - Reword the demo's cacheKey section so it no longer claims parity with section 2.
Brentlok
left a comment
Brentlok
left a comment
There was a problem hiding this comment.
Thanks for PR, I've done initial review for things that I've catched immediately, but I didn't dig any deeper into the logic yet. Since it's a huge PR and touches many core functionalities would you be okay, if I push some of my changes directly into this PR, it would be much faster in some cases than writing a detailed comments?
There was a problem hiding this comment.
We're not really adding any new features into expo-example, it's just a playground when developing new features, we're actually planning to minimize this screen. Our regression tests should take care of any feature
There was a problem hiding this comment.
I'm ok to remove it, just needed some space to visually test. lmk what would you prefer
There was a problem hiding this comment.
We can keep it for now, and remove it in the end, after we're done with this new feature
| ) | ||
|
|
||
| return ( | ||
| <View style={{ display: 'contents' }}> |
There was a problem hiding this comment.
I don't think that we need this in .native
| * Caching only engages when every ancestor `<ScopedVariables>` in the merge | ||
| * chain also opted in; if any ancestor bypasses, this subtree bypasses too. | ||
| */ | ||
| cacheKey?: string |
There was a problem hiding this comment.
I don't really like requesting from the user an additional key that he may not fully understand. We could build our custom cacheKey inside ScopedVariables
const cacheKey = useMemo(
() =>
Object
.entries(variables)
.sort(([a], [b]) => a.localeCompare(b))
.reduce((acc, [key, value]) => `${acc}${key}:${value};`, ''),
[variables],
)There was a problem hiding this comment.
I thought about doing that as well. Reasons why not was just the cost of compute for key - hence opted-in to allow user provide a custom key, where they can do above on their side.
If you prefer that way, this is an easy change.
There was a problem hiding this comment.
I think that the cost of not having a style cache is bigger that computing this cacheKey, so let's generate this cache key
| const names = Object.keys(uniwindContext.variables) | ||
|
|
||
| Object.entries(uniwindContext.variables).forEach(([name, value]) => { | ||
| dummyParent.style.setProperty(name, typeof value === 'number' ? `${value}px` : value) |
There was a problem hiding this comment.
We're using typeof value === 'number' ? ${value}px : value in more and more places, let's move this to some kind of web only util
There was a problem hiding this comment.
Agree. I saw several places, but did not want to touch unrelated code.
| // Recompute on context change too: a new `uniwindContext` (e.g. an | ||
| // updated `<ScopedVariables>` prop or a nearer provider) can resolve to | ||
| // a different value without any global Theme/Variables event firing. | ||
| updateValue() |
There was a problem hiding this comment.
Wouldn't this cause an extra rerender on mount?
There was a problem hiding this comment.
Initially I did not have it but this was flagged by one of auto reviewers as potential staleness.
|
|
||
| // ScopedVariables variables prop | ||
| type ScopedVariablesProp = ComponentProps<typeof ScopedVariables>['variables'] | ||
| type ScopedVariablesPropTest = Expect<Equal<ScopedVariablesProp, Record<string, string | number>>> |
There was a problem hiding this comment.
We don't need this, we're testing here if Theme has correct type light | dark | etc. instead of plain string
This just forces updating tests if the API changes
Brentlok
left a comment
Brentlok
left a comment
There was a problem hiding this comment.
There's too much long, multi line comments everywhere, it's not common in this repo
please go ahead.
|
- Derive the native style cache key from the merged variables map instead of a user-supplied cacheKey prop; removes the cache-bypass path and the stale-key footgun - Drop the display: contents View wrapper on native, render the bare provider like ScopedTheme.native - Remove the expo-example demo (playground only, covered by tests) - useCSSVariable: skip the mount-time recompute, useState already resolved the initial value - Extract toWebValue (number -> px) web util, reuse in config, getWebStyles and the web wrapper - Remove the ScopedVariables prop type test - Trim long comments to match repo style
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/uniwind/src/components/ScopedVariables/utils.ts`:
- Around line 32-35: Update the variablesCacheKey serialization in the
mergedVariables cache-key construction to use an unambiguous encoding that
cannot collide when variable keys or values contain delimiters such as
semicolons or colons. Preserve deterministic ordering of entries so equivalent
variable maps produce the same key, and keep the resulting key compatible with
its use by UniwindStore.getStyles.
🪄 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 Plus
Run ID: cd7d602a-9e01-4195-b622-fe5160ee43d9
📒 Files selected for processing (13)
CONTEXT.mdpackages/uniwind/src/components/ScopedVariables/ScopedVariables.native.tsxpackages/uniwind/src/components/ScopedVariables/ScopedVariables.tsxpackages/uniwind/src/components/ScopedVariables/utils.tspackages/uniwind/src/core/config/config.tspackages/uniwind/src/core/native/native-utils.tspackages/uniwind/src/core/native/store.tspackages/uniwind/src/core/web/getWebStyles.tspackages/uniwind/src/core/web/index.tspackages/uniwind/src/core/web/webUtils.tspackages/uniwind/src/hooks/useCSSVariable/useCSSVariable.tspackages/uniwind/tests/native/components/scoped-variables.test.tsxpackages/uniwind/tests/web/components/scoped-variables.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/uniwind/src/core/native/native-utils.ts
- packages/uniwind/src/core/config/config.ts
- packages/uniwind/src/core/web/getWebStyles.ts
|
@Brentlok |
The key:value; concatenation had no escaping, so values containing
separators could collide ({'--a': '1;--b:2'} vs {'--a': '1', '--b': '2'})
and serve wrong cached styles.
2 participants
@coderabbitai ignore
What
Adds
<ScopedVariables>— a React Context provider that overrides CSS variables for a subtree on both web and native, the per-subtree analogue ofUniwind.updateCSSVariables(which is global-per-theme) but limited to a React tree, instead of global.{ ...inherited, ...own }, nearest wins.display:contentswrapper so the real DOM cascade resolves them.Context
Addresses uni-stack/uniwind#546.
Prior art
In NativeWind
varserve similar purpose. Opted to have a separate explicit component, followingScopedThemeexample:Plain web - this is literally what web portion of PR does:
Summary by CodeRabbit
ScopedVariablesto apply CSS custom-property overrides to a specific component subtree.useCSSVariable.