-
Notifications
You must be signed in to change notification settings - Fork 41
fix: combined variants overriding each other#567
Conversation
|
No actionable comments were generated in the recent review. 🎉 i️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe CSS processor now accumulates variant metadata across recursive selector parsing using nullish coalescing assignment, preserving multiple pseudo-class states simultaneously. A new test and a type alignment update validate that combined variants like ChangesCombined variant selector parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (1)
packages/uniwind/tests/native/styles-parsing/meta.test.ts (1)
31-36: ⚡ Quick winConsider expanding test coverage for combined variants.
The test validates the core fix for
dark:active:combination. Consider adding test cases for:
- Triple combinations (
dark:active:focus:bg-...)- Different orderings (
active:dark:bg-...)- Other variant types (
rtl,disabled,dataAttributes)- Verify that unrelated fields remain
null🧪 Example expanded test cases
test('Combined variants', async () => { const { stylesheet } = await compileMetadata() expect(stylesheet['dark:active:bg-purple-700'][0].theme).toBe('dark') expect(stylesheet['dark:active:bg-purple-700'][0].active).toBe(true) + expect(stylesheet['dark:active:bg-purple-700'][0].focus).toBe(null) + expect(stylesheet['dark:active:bg-purple-700'][0].disabled).toBe(null) }) + +test('Triple combined variants', async () => { + const { stylesheet } = await compileMetadata() + + // Assuming this class exists in test.css + expect(stylesheet['dark:active:focus:bg-purple-700'][0].theme).toBe('dark') + expect(stylesheet['dark:active:focus:bg-purple-700'][0].active).toBe(true) + expect(stylesheet['dark:active:focus:bg-purple-700'][0].focus).toBe(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/tests/native/styles-parsing/meta.test.ts` around lines 31 - 36, Expand the 'Combined variants' test in meta.test.ts to cover more variant combinations: add cases that use triple combinations (e.g., 'dark:active:focus:bg-...'), different orderings (e.g., 'active:dark:bg-...'), other variant types like 'rtl', 'disabled' and a data-attribute variant (e.g., 'data-open:bg-...'), and assert via compileMetadata() that for each stylesheet key (use the existing pattern where you access stylesheet['...'][0]) the corresponding flags (theme, active, focus, rtl, disabled, data attributes) are true and unrelated fields remain null; reuse the same test harness (compileMetadata, stylesheet) and add explicit expects for null on fields that should not be set to ensure no bleed between variants.
🤖 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.
Nitpick comments:
In `@packages/uniwind/tests/native/styles-parsing/meta.test.ts`:
- Around line 31-36: Expand the 'Combined variants' test in meta.test.ts to
cover more variant combinations: add cases that use triple combinations (e.g.,
'dark:active:focus:bg-...'), different orderings (e.g., 'active:dark:bg-...'),
other variant types like 'rtl', 'disabled' and a data-attribute variant (e.g.,
'data-open:bg-...'), and assert via compileMetadata() that for each stylesheet
key (use the existing pattern where you access stylesheet['...'][0]) the
corresponding flags (theme, active, focus, rtl, disabled, data attributes) are
true and unrelated fields remain null; reuse the same test harness
(compileMetadata, stylesheet) and add explicit expects for null on fields that
should not be set to ensure no bleed between variants.
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc42cebf-f631-4227-bae4-65a6dcc5c158
📒 Files selected for processing (2)
packages/uniwind/src/bundler/css-processor/processor.tspackages/uniwind/tests/native/styles-parsing/meta.test.ts
🚀 This pull request is included in v1.9.0. See Release v1.9.0 for release notes.
Uh oh!
There was an error while loading. Please reload this page.
fix #566
Summary by CodeRabbit
Bug Fixes
Tests
Refactor