Fix shadow replacer for all <length> types#20067
Conversation
Confidence Score: 4/5Safe to merge for the common cases it fixes, but a regression exists for shadow colors expressed as modern CSS color functions with embedded math calls (e.g. oklch(calc(50%) 0.1 90deg)), where the replacer silently produces wrong output. The change correctly fixes calc() and other math functions in shadow offset/blur positions, and the IS_ZERO regex is sound. The regression is a real output-correctness bug on the color replacement path: any color function that contains a calc() or other math function internally will be misclassified as a length, so the replacer appends currentcolor rather than wrapping the actual color. This pattern is valid CSS and was handled correctly before this PR. packages/tailwindcss/src/utils/replace-shadow-colors.ts — the isLength() call needs to be guarded so that math-function detection only applies to tokens where the math function is at the start of the string, not embedded inside a color function. Reviews (3): Last reviewed commit: "Update changelog" | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughThis PR updates replaceShadowColors to detect shadow offsets using isLength() plus a new IS_ZERO regex (replacing the prior stateful LENGTH regex) and adds Vitest cases that assert exotic zero-like numeric offsets and calc() expressions are preserved while the color portion is wrapped with var(--tw-shadow-color, ...). 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (1)
packages/tailwindcss/src/utils/replace-shadow-colors.ts (1)
25-26: 💤 Low valueMinor: Misleading comment about stateful regex.
The comment states the regex is stateful, but
IS_ZEROis not defined with theg(global) flag, solastIndexis always 0 and resetting it is a no-op. Either add thegflag to make the regex truly stateful, or update the comment to reflect that this is defensive programming for consistency.Option 1: Remove the unnecessary reset
- // Reset index, since the regex is stateful. - IS_ZERO.lastIndex = 0Option 2: Make the regex global and keep the reset
-const IS_ZERO = /^[+-]?0*\.?0+(?:[eE][+-]?\d+)?$/ +const IS_ZERO = /^[+-]?0*\.?0+(?:[eE][+-]?\d+)?$/g🤖 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/tailwindcss/src/utils/replace-shadow-colors.ts` around lines 25 - 26, The comment claiming the regex IS_ZERO is stateful is misleading because IS_ZERO lacks the global flag, so resetting IS_ZERO.lastIndex is a no-op; either make IS_ZERO a global regex (add the 'g' flag where IS_ZERO is defined in replace-shadow-colors.ts) so the trailing IS_ZERO.lastIndex = 0 reset is meaningful, or remove that reset and update the comment to note the code is defensive/ non-stateful; locate the IS_ZERO definition and the reset line to apply one of these two fixes consistently.
🤖 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/tailwindcss/src/utils/replace-shadow-colors.test.ts`:
- Around line 28-31: Add test cases that ensure hex colors containing zeros are
not misclassified as numeric zero offsets: in the existing test file add specs
calling replaceShadowColors with strings like "0 0 `#000`" and "0 0 4px `#f00`"
using the existing replacer and assert the output wraps the color with
var(--tw-shadow-color, <hex>) (e.g., expect "0 0 var(--tw-shadow-color, `#000`)"
and "0 0 4px var(--tw-shadow-color, `#f00`)"). These tests will exercise the
IS_ZERO regex path used inside replaceShadowColors to verify hex values are
treated as colors rather than zero offsets.
In `@packages/tailwindcss/src/utils/replace-shadow-colors.ts`:
- Line 5: The IS_ZERO regex currently matches zero-like substrings anywhere,
causing hex colors with zeros (e.g., "`#000`") to be misdetected as numeric zeros;
update the IS_ZERO pattern in replace-shadow-colors.ts to match the entire token
(anchor with ^ and $) so it only returns true for tokens that are exactly zero
values (e.g., change to a whole-string anchored pattern) and run/adjust any
related logic that uses IS_ZERO.test(...) so color tokens are not treated as
numeric offsets.
---
Nitpick comments:
In `@packages/tailwindcss/src/utils/replace-shadow-colors.ts`:
- Around line 25-26: The comment claiming the regex IS_ZERO is stateful is
misleading because IS_ZERO lacks the global flag, so resetting IS_ZERO.lastIndex
is a no-op; either make IS_ZERO a global regex (add the 'g' flag where IS_ZERO
is defined in replace-shadow-colors.ts) so the trailing IS_ZERO.lastIndex = 0
reset is meaningful, or remove that reset and update the comment to note the
code is defensive/ non-stateful; locate the IS_ZERO definition and the reset
line to apply one of these two fixes consistently.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 574800da-7a5f-4346-be01-93c963dd3c8d
📒 Files selected for processing (2)
packages/tailwindcss/src/utils/replace-shadow-colors.test.tspackages/tailwindcss/src/utils/replace-shadow-colors.ts
| if (KEYWORDS.has(part)) { | ||
| continue | ||
| } else if (LENGTH.test(part)) { | ||
| } else if (isLength(part) || IS_ZERO.test(part)) { |
There was a problem hiding this comment.
isLength misclassifies color functions containing math calls as lengths
isLength delegates to hasMathFn, which returns true for any string that contains a math function call — not only strings that are a math function. Because segment is paren-aware, a color like oklch(calc(50%) 0.1 90deg) arrives as a single token; hasMathFn sees the embedded calc( and returns true, so isLength returns true. With both offsets already filled, the part falls through the length branch silently, color stays null, and the output appends currentcolor instead of wrapping the actual color.
replaceShadowColors('0 0 0 oklch(calc(50%) 0.1 90deg)', replacer) would produce 0 0 0 oklch(calc(50%) 0.1 90deg) var(--tw-shadow-color, currentcolor) rather than the expected 0 0 0 var(--tw-shadow-color, oklch(calc(50%) 0.1 90deg)).
The original LENGTH regex required values to start with a digit, so color function names (which start with letters) could never match. The fix needs to ensure that math-function detection only fires when the math function is at the start of the token, not embedded inside another function.
Summary
Fixes #20065. As commented, the shadow replacer function does not account for all
<length>-type values. This PR uses existing logic to test for<length>values.Test plan
calc()(perhaps could do with more tests for other kind of<length>values).0cases thatisLengthdoes not cover.