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.
|
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 a cross-platform ChangesScopedVariables feature
Pre-commit output mode
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant ScopedVariables
participant UniwindContext
participant Runtime
participant useCSSVariable
Component->>ScopedVariables: provide variables
ScopedVariables->>UniwindContext: merge scoped values and cache key
UniwindContext->>Runtime: resolve scoped styles
Runtime->>useCSSVariable: evaluate variable overlay
useCSSVariable-->>Component: return scoped value
Possibly related PRs
Suggested reviewers: 🚥 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 SummaryThis PR introduces
Confidence Score: 4/5The feature implementation is solid; the one concrete defect is a stale The Files Needing Attention: packages/uniwind/tests/web/components/scoped-variables.test.tsx — the
|
| Filename | Overview |
|---|---|
| packages/uniwind/src/components/ScopedVariables/utils.ts | Shared merge/validate logic; invalid keys are always filtered (not just in DEV), cache key computed over sorted entries after stripping the sentinel, merging correctly preserves ancestor vars with nearest-wins semantics. |
| packages/uniwind/src/core/web/getWebStyles.ts | Scoped variables applied/disposed around DOM reads with try/finally. Minor issue: applyScopedVariables iterates __uniwindVariablesCacheKey alongside real CSS vars, causing harmless but impure setProperty calls. |
| packages/uniwind/src/core/native/store.ts | Cache key extended with __uniwindVariablesCacheKey; scoped vars overlaid via prototype chain onto theme vars, preventing mutation of global state. |
| packages/uniwind/src/core/native/native-utils.ts | Extracts createVarGetter and adds getScopedVars with a WeakMap cache keyed on the variables reference; normalisation logic (culori color → hex) reused from config. |
| packages/uniwind/tests/web/components/scoped-variables.test.tsx | Comprehensive coverage of web scoping, nesting, and disposal, but the last test passes variablesCacheKey: null — an excess property not in UniwindContextType — which will fail TypeScript compilation. |
| packages/uniwind/tests/native/components/scoped-variables.test.tsx | Well-structured native tests covering override scope, nesting, numeric pass-through, color normalization, composition with ScopedTheme, cache key derivation, and stale-cache prevention. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["ScopedVariables variables={...}"] --> B["buildScopedVariablesContext"]
B --> C["validateVariables (drops non -- keys)"]
C --> D["merge: ...parent.variables, ...own"]
D --> E["delete __uniwindVariablesCacheKey\nJSON.stringify(sorted entries)\nset __uniwindVariablesCacheKey"]
E --> F["UniwindContext.Provider (updated variables)"]
F --> G{Platform}
G -->|Web| H["div display:contents with inline --vars\nchildren"]
G -->|Native| I["Children with updated context"]
H --> J["useCSSVariable / getWebStyles"]
I --> K["useCSSVariable / UniwindStore.getStyles"]
J --> L["applyScopedVariables setProperty on dummyParent\ntry { read } finally { dispose }"]
K --> M["Object.create(themeVars) + getScopedVars\nprototype-chained overlay"]
L --> N["parseCSSValue(computed)"]
M --> O["vars[name](vars)"]
Reviews (6): Last reviewed commit: "Merge branch 'main' into feat/scoped-var..." | 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
- 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?
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.
|
@dlebedynskyi I've pushed some minor changes, I need to do some more testing as this is quite a big feature, but overall great contribution! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/components/ScopedVariables/utils.ts (1)
4-6: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftPreserve the optional native no-cache path.
buildScopedVariablesContextalways setsvariables.__uniwindVariablesCacheKey, and nativegetStylesuses that value in its cache key. As a result, everyScopedVariablessubtree becomes cacheable even without an explicit cache key. Carry an optional key through the context and let native styles opt out of caching when it is absent.🤖 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 4 - 6, Update ScopedVariablesProps and buildScopedVariablesContext to carry an optional cache key instead of always assigning variables.__uniwindVariablesCacheKey. Ensure native getStyles uses the key when provided but preserves the no-cache path when it is absent.
🤖 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.
Outside diff comments:
In `@packages/uniwind/src/components/ScopedVariables/utils.ts`:
- Around line 4-6: Update ScopedVariablesProps and buildScopedVariablesContext
to carry an optional cache key instead of always assigning
variables.__uniwindVariablesCacheKey. Ensure native getStyles uses the key when
provided but preserves the no-cache path when it is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a48f6b06-3ac7-4bea-a1d6-119243b948d8
📒 Files selected for processing (19)
.husky/pre-commitpackages/uniwind/src/components/ScopedVariables/ScopedVariables.tsxpackages/uniwind/src/components/ScopedVariables/utils.tspackages/uniwind/src/components/web/useUniwindAccent.tspackages/uniwind/src/core/config/config.common.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/types.tspackages/uniwind/src/core/web/getWebStyles.tspackages/uniwind/src/core/web/index.tspackages/uniwind/src/core/web/parseCSSValue.tspackages/uniwind/src/core/web/webUtils.tspackages/uniwind/src/hooks/useCSSVariable/getVariableValue.native.tspackages/uniwind/src/hooks/useCSSVariable/useCSSVariable.tspackages/uniwind/tests/consts.tspackages/uniwind/tests/e2e/getWebStyles.test.tspackages/uniwind/tests/native/components/scoped-variables.test.tsx
💤 Files with no reviewable changes (3)
- packages/uniwind/src/core/web/parseCSSValue.ts
- packages/uniwind/tests/consts.ts
- packages/uniwind/src/core/web/index.ts
dlebedynskyi
left a comment
dlebedynskyi
left a comment
There was a problem hiding this comment.
I've looked over changes. All make sense to me.
2 participants
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
Summary by CodeRabbit
New Features
ScopedVariablesto override CSS custom properties for a component subtree.useCSSVariableon both native and web.Bug Fixes
Tests