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.
Here is everything you need to know about this update. Please take a good look at what changed and the test results before merging this pull request. ### What changed? #### ✳️ eslint (9.36.0 → 9.37.0) · [Repo](https://github.com/eslint/eslint) · [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) <details> <summary>Release Notes</summary> <h4><a href="https://github.com/eslint/eslint/releases/tag/v9.37.0">9.37.0</a></h4> <blockquote><h2 dir="auto">Features</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/39f7fb493a6924ff7dc638fd4d6e7b3d8eb95383"><code class="notranslate">39f7fb4</code></a> feat: <code class="notranslate">preserve-caught-error</code> should recognize all static "cause" keys (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20163">#20163</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/f81eabc5849ece98b8ca054f96b29f038a69bcf8"><code class="notranslate">f81eabc</code></a> feat: support TS syntax in <code class="notranslate">no-restricted-imports</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/19562">#19562</a>) (Nitin Kumar)</li> </ul> <h2 dir="auto">Bug Fixes</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/a129cced7a86ea2518eb9be6990fa18af39694ca"><code class="notranslate">a129cce</code></a> fix: correct <code class="notranslate">no-loss-of-precision</code> false positives for leading zeros (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20164">#20164</a>) (Francesco Trotta)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/09e04fcc3f4cc963eea7c9c579391de5e231595b"><code class="notranslate">09e04fc</code></a> fix: add missing AST token types (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20172">#20172</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/861c6da2bd2796414e6eed782155ec34e2ed6344"><code class="notranslate">861c6da</code></a> fix: correct <code class="notranslate">ESLint</code> typings (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20122">#20122</a>) (Pixel998)</li> </ul> <h2 dir="auto">Documentation</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/b950359c5f39085483c3137a6a160e582ef32007"><code class="notranslate">b950359</code></a> docs: fix typos across the docs (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20182">#20182</a>) (루밀LuMir)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/42498a27981d50750dd15ae8660dbe85c4f4587c"><code class="notranslate">42498a2</code></a> docs: improve ToC accessibility by hiding non-semantic character (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20181">#20181</a>) (Percy Ma)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/29ea092b93608756350b1e9c5a4f29c8a49264ab"><code class="notranslate">29ea092</code></a> docs: Update README (GitHub Actions Bot)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/5c97a04578e6280c2395f642c2d8d6bdf30eec18"><code class="notranslate">5c97a04</code></a> docs: show <code class="notranslate">availableUntil</code> in deprecated rule banner (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20170">#20170</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/90a71bf5024a86fc232cd2e05f96811e2a18fd0f"><code class="notranslate">90a71bf</code></a> docs: update <code class="notranslate">README</code> files to add badge and instructions (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20115">#20115</a>) (루밀LuMir)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/1603ae1526d9b6f557c7d5534a4f40f46842edd6"><code class="notranslate">1603ae1</code></a> docs: update references from <code class="notranslate">master</code> to <code class="notranslate">main</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20153">#20153</a>) (루밀LuMir)</li> </ul> <h2 dir="auto">Chores</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/afe8a1346958242031fea66fdfbb239e8bf408b7"><code class="notranslate">afe8a13</code></a> chore: update <code class="notranslate">@eslint/js</code> dependency to version 9.37.0 (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20183">#20183</a>) (Francesco Trotta)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/abee4ca1fa10da733b1cc4a7d5e765b912a9de82"><code class="notranslate">abee4ca</code></a> chore: package.json update for @eslint/js release (Jenkins)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/fc9381f6ca57b824e82d118c14631c17bea79d7e"><code class="notranslate">fc9381f</code></a> chore: fix typos in comments (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20175">#20175</a>) (overlookmotel)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/e1574a22d38fd7e1891f86f8db0b09053f8963cb"><code class="notranslate">e1574a2</code></a> chore: unpin jiti (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20173">#20173</a>) (renovate[bot])</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/e1ac05e2fae779e738f85bd47dda1cc2b7099346"><code class="notranslate">e1ac05e</code></a> refactor: mark <code class="notranslate">ESLint.findConfigFile()</code> as <code class="notranslate">async</code>, add missing docs (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20157">#20157</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/347906d627c53bf45d63ba831d2fd2b83fb0a749"><code class="notranslate">347906d</code></a> chore: update eslint (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20149">#20149</a>) (renovate[bot])</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/0cb5897e24059bacadb8d2e6458184904759fda1"><code class="notranslate">0cb5897</code></a> test: remove tmp dir created for circular fixes in multithread mode test (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20146">#20146</a>) (Milos Djermanovic)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/bb995665e32b3a958e78006c9fd75744c5604f1b"><code class="notranslate">bb99566</code></a> ci: pin <code class="notranslate">jiti</code> to version 2.5.1 (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20151">#20151</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/177f669adc0f96d14ae1a71cde7786f327515863"><code class="notranslate">177f669</code></a> perf: improve worker count calculation for <code class="notranslate">"auto"</code> concurrency (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20067">#20067</a>) (Francesco Trotta)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/448b57bca3406ee12c4e44e9298fc0c99d3ee10c"><code class="notranslate">448b57b</code></a> chore: Mark deprecated formatting rules as available until v11.0.0 (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20144">#20144</a>) (Milos Djermanovic)</li> </ul></blockquote> <p><em>Does any of this look wrong? <a href="https://depfu.com/packages/npm/eslint/feedback">Please let us know.</a></em></p> </details> <details> <summary>Commits</summary> <p><a href="https://github.com/eslint/eslint/compare/b4857e54e54b5dba96d156cd8d8b4d42dc5a3bf4...d5d1bdf5fdfad75197aadd3e894182135158c3b1">See the full diff on Github</a>. The new version differs by 23 commits:</p> <ul> <li><a href="https://github.com/eslint/eslint/commit/d5d1bdf5fdfad75197aadd3e894182135158c3b1"><code>9.37.0</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/94865ff41cdc14b90ecd325926b78c2ffc6a5206"><code>Build: changelog update for 9.37.0</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/afe8a1346958242031fea66fdfbb239e8bf408b7"><code>chore: update `@eslint/js` dependency to version 9.37.0 (#20183)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/abee4ca1fa10da733b1cc4a7d5e765b912a9de82"><code>chore: package.json update for @eslint/js release</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/b950359c5f39085483c3137a6a160e582ef32007"><code>docs: fix typos across the docs (#20182)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/42498a27981d50750dd15ae8660dbe85c4f4587c"><code>docs: improve ToC accessibility by hiding non-semantic character (#20181)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/fc9381f6ca57b824e82d118c14631c17bea79d7e"><code>chore: fix typos in comments (#20175)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/e1574a22d38fd7e1891f86f8db0b09053f8963cb"><code>chore: unpin jiti (#20173)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/29ea092b93608756350b1e9c5a4f29c8a49264ab"><code>docs: Update README</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/a129cced7a86ea2518eb9be6990fa18af39694ca"><code>fix: correct `no-loss-of-precision` false positives for leading zeros (#20164)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/09e04fcc3f4cc963eea7c9c579391de5e231595b"><code>fix: add missing AST token types (#20172)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/5c97a04578e6280c2395f642c2d8d6bdf30eec18"><code>docs: show `availableUntil` in deprecated rule banner (#20170)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/39f7fb493a6924ff7dc638fd4d6e7b3d8eb95383"><code>feat: `preserve-caught-error` should recognize all static "cause" keys (#20163)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/f81eabc5849ece98b8ca054f96b29f038a69bcf8"><code>feat: support TS syntax in `no-restricted-imports` (tailwindlabs#19562)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/e1ac05e2fae779e738f85bd47dda1cc2b7099346"><code>refactor: mark `ESLint.findConfigFile()` as `async`, add missing docs (#20157)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/90a71bf5024a86fc232cd2e05f96811e2a18fd0f"><code>docs: update `README` files to add badge and instructions (#20115)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/861c6da2bd2796414e6eed782155ec34e2ed6344"><code>fix: correct `ESLint` typings (#20122)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/347906d627c53bf45d63ba831d2fd2b83fb0a749"><code>chore: update eslint (#20149)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/1603ae1526d9b6f557c7d5534a4f40f46842edd6"><code>docs: update references from `master` to `main` (#20153)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/0cb5897e24059bacadb8d2e6458184904759fda1"><code>test: remove tmp dir created for circular fixes in multithread mode test (#20146)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/bb995665e32b3a958e78006c9fd75744c5604f1b"><code>ci: pin `jiti` to version 2.5.1 (#20151)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/177f669adc0f96d14ae1a71cde7786f327515863"><code>perf: improve worker count calculation for `"auto"` concurrency (tailwindlabs#20067)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/448b57bca3406ee12c4e44e9298fc0c99d3ee10c"><code>chore: Mark deprecated formatting rules as available until v11.0.0 (#20144)</code></a></li> </ul> </details> ---  [Depfu](https://depfu.com) will automatically keep this PR conflict-free, as long as you don't add any commits to this branch yourself. You can also trigger a rebase manually by commenting with `@depfu rebase`. <details><summary>All Depfu comment commands</summary> <blockquote><dl> <dt>@depfu rebase</dt><dd>Rebases against your default branch and redoes this update</dd> <dt>@depfu recreate</dt><dd>Recreates this PR, overwriting any edits that you've made to it</dd> <dt>@depfu merge</dt><dd>Merges this PR once your tests are passing and conflicts are resolved</dd> <dt>@depfu cancel merge</dt><dd>Cancels automatic merging of this PR</dd> <dt>@depfu close</dt><dd>Closes this PR and deletes the branch</dd> <dt>@depfu reopen</dt><dd>Restores the branch and reopens this PR (if it's closed)</dd> <dt>@depfu pause</dt><dd>Ignores all future updates for this dependency and closes this PR</dd> <dt>@depfu pause [minor|major]</dt><dd>Ignores all future minor/major updates for this dependency and closes this PR</dd> <dt>@depfu resume</dt><dd>Future versions of this dependency will create PRs again (leaves this PR as is)</dd> </dl></blockquote> </details> Co-authored-by: depfu[bot] <23717796+depfu[bot]@users.noreply.github.com>
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.