fix: join borderWidth styles if duplicated#285
Conversation
📝 WalkthroughWalkthroughCollapses identical per-side border width properties into a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/uniwind/src/metro/processor/rn.ts:
- Around line 281-298: The joinStyles method erroneously checks every key in the
styles object instead of only border-width keys, so replace the condition that
uses keys.every(...) with a check that iterates over BORDER_WIDTH_KEYS (e.g.,
BORDER_WIDTH_KEYS.every(k => styles[k] === borderWidth)) to ensure you only
compare the four border widths; keep the existing removeKeys(styles,
BORDER_WIDTH_KEYS) and returned shape unchanged and use the same borderWidth
value from styles.borderTopWidth.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/uniwind/src/metro/processor/rn.tspackages/uniwind/src/metro/utils/common.tspackages/uniwind/tests/styles-parsing/borders.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/uniwind/src/metro/processor/rn.ts (1)
packages/uniwind/src/metro/utils/common.ts (2)
isDefined(1-1)removeKeys(107-110)
packages/uniwind/tests/styles-parsing/borders.test.tsx (1)
apps/expo-router/app/sections/border.tsx (1)
BorderScreen(7-271)
🔇 Additional comments (5)
packages/uniwind/tests/styles-parsing/borders.test.tsx (1)
23-27: LGTM! Test updates correctly reflect the border width consolidation.The test assertions now properly validate that uniform border widths are consolidated into a single
borderWidthproperty instead of four directional properties. This aligns with the implementation changes inrn.ts.packages/uniwind/src/metro/utils/common.ts (1)
107-110: LGTM! Clean and type-safe utility function.The implementation correctly removes specified keys from an object using
Object.entriesandObject.fromEntries, with proper TypeScript typing that accurately represents the operation.packages/uniwind/src/metro/processor/rn.ts (3)
1-1: LGTM! Import correctly added.The
removeKeysimport is properly added to support the border width consolidation logic.
167-167: LGTM! Border width keys correctly defined.The constant properly defines all four directional border width properties that should be consolidated.
192-194: LGTM! Integration of joinStyles is clean.The
cssToRNmethod correctly callsjoinStylesto consolidate border widths before returning the final style entries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/uniwind/src/metro/processor/rn.ts (1)
167-167: Consider making the constant readonly for type safety.For better type safety and to prevent accidental mutation, consider making
BORDER_WIDTH_KEYSreadonly:const BORDER_WIDTH_KEYS = ['borderTopWidth', 'borderRightWidth', 'borderBottomWidth', 'borderLeftWidth'] as constOr:
const BORDER_WIDTH_KEYS: readonly string[] = ['borderTopWidth', 'borderRightWidth', 'borderBottomWidth', 'borderLeftWidth']
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/uniwind/src/metro/processor/rn.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/uniwind/src/metro/processor/rn.ts (1)
packages/uniwind/src/metro/utils/common.ts (2)
isDefined(1-1)removeKeys(107-110)
🔇 Additional comments (3)
packages/uniwind/src/metro/processor/rn.ts (3)
1-1: LGTM!The
removeKeysimport is correctly added and used in the newjoinStylesmethod.
192-194: LGTM!The integration of
joinStylesinto thecssToRNflow is correct. Border width consolidation now happens before the final style entries are returned.
281-298: LGTM! Critical bug from previous review has been fixed.The implementation correctly consolidates border width properties when all four sides have identical values. The critical bug identified in the previous review (using
keys.everyinstead ofBORDER_WIDTH_KEYS.everyat line 289) has been properly fixed.Edge case consideration: If a
borderWidthproperty already exists alongside the four specific border width properties, it will be replaced by the consolidated value. This appears intentional but verify this behavior aligns with expectations.#!/bin/bash # Verify that the joinStyles consolidation works correctly with test cases # Search for test files related to RN processor fd -e test.ts -e test.tsx -e spec.ts . packages/uniwind/src/metro/processor/ # Look for existing tests of border width handling rg -n -C3 --type=ts "borderWidth|border.*Width" packages/uniwind/src/metro/processor/ -g "*test*" -g "*spec*"
Summary by CodeRabbit
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.