fix: prevent @tailwindcss/upgrade from rewriting node_modules when run from subdirectory#20341
fix: prevent @tailwindcss/upgrade from rewriting node_modules when run from subdirectory#20341deepshekhardas wants to merge 6 commits into
Conversation
…les` when run from a workspace subpackage
Notes by Robin Malfait: Split the commit in 2, because I want to be able to see the test fail _before_ we apply the fix.
Confidence Score: 3/5The CSS analysis path is correctly fixed, but the template migration path in index.ts uses the same unfixed pattern and could rewrite files inside ignored directories when the tool is run from a subdirectory. The fix addresses the reported scenario but leaves an identical isGitIgnored call in index.ts at line 46 unfixed, which guards template file migration with the same potentially broken predicate when run from a subdirectory. The new integration test uses source(none) so this gap is not caught. packages/@tailwindcss-upgrade/src/index.ts — the isGitIgnored call at line 46 needs the same gitRoot treatment applied in analyze.ts. Reviews (1): Last reviewed commit: "update CHANGELOG" | Re-trigger Greptile |
WalkthroughThe upgrade flow now passes its base directory into stylesheet analysis. Analysis resolves the Git repository root with 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/@tailwindcss-upgrade/src/index.ts (2)
44-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRespect the repo-root
.gitignorefor template files.The PR successfully updates
analyze.tsto usegitRoot(base)so that the repository root's.gitignorerules are applied. However,isIgnoredin this file is still initialized with{ cwd: base }.This means that when upgrading from a subpackage, template files that should be ignored by the root
.gitignore(e.g., build outputs in adist/folder) will still be scanned and incorrectly migrated.Update the initialization to match
analyze.ts(and remember to addimport { gitRoot } from './utils/git'to your imports):🐛 Proposed fix
async function run() { let base = process.cwd() - let isIgnored = await isGitIgnored({ cwd: base }) + let isIgnored = await isGitIgnored({ cwd: gitRoot(base) ?? base })
105-106: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter discovered CSS files to respect repo-root ignores.
The
globbycall above usesgitignore: true, which resolves.gitignorefiles starting fromcwd(base). Whenbaseis a subpackage,globbywill not see the repository root's.gitignore.Because
analyze.tsseeds these initial stylesheets directly into its processing map without verifying them againstisIgnored, any git-ignored CSS files (likedist/out.css) discovered here will be incorrectly mutated.Filter the discovered files using the properly initialized
isIgnoredhelper. It's best to apply this filter inside theif (files.length === 0)block to ensure that files explicitly passed as CLI arguments are still upgraded.🐛 Proposed fix
// gitignore: true will first search for all .gitignore including node_modules folders, this makes the initial search much faster ignore: ['**/node_modules/**'], }) + + files = files.filter((file) => { + try { + return !isIgnored(file) + } catch { + return false + } + }) }
🧹 Nitpick comments (1)
integrations/upgrade/index.test.ts (1)
3405-3410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for non-
node_modulesignored files.This test successfully verifies that
@imported files insidenode_modulesare ignored. However, becausenode_modulesis already hardcoded in theglobbyignore list inindex.ts(ignore: ['**/node_modules/**']), this test setup doesn't catch cases where other git-ignored files (like adist/folder containing generated CSS or HTML templates) are incorrectly discovered and processed when upgrading from a nested directory.To ensure comprehensive coverage of the root
.gitignorelogic across both CSS discovery and template scanning, consider adding a dummy file in adist/directory, ignoringdist/in this.gitignore, and asserting that thedist/file remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c5d8ca8-d57a-443e-9993-c4886d7f58bf
📒 Files selected for processing (5)
CHANGELOG.mdintegrations/upgrade/index.test.tspackages/@tailwindcss-upgrade/src/codemods/css/analyze.tspackages/@tailwindcss-upgrade/src/index.tspackages/@tailwindcss-upgrade/src/utils/git.ts
Cherry-pick of PR #20329