Make upgrade tooling more stable#19846
Merged
RobinMalfait merged 20 commits intomainfrom Mar 24, 2026
Merged
Conversation
…skip writes when content is unchanged When migrating from v3 to v4, the CSS entrypoint has no @source directive yet, leaving compiler.root as null. The fallback to '**/*' caused the tool to scan every file in the project (PHP migrations, config files, etc.) and unconditionally write them all back to disk. - Use the v3 JS config's content patterns (config.sources) as the scan scope when compiler.root is null, falling back to '**/*' only when no config sources are available - Move config lookup before the sources IIFE to avoid a TDZ error - Skip fs.writeFile when the migrated content is identical to the original
In Tailwind CSS v3, the `config.content` was required. So if we are sure that we are coming from Tailwind CSS v3, then we can use that. This will already be part of the `compiler.sources`, so we can bail early. We don't need to return the `config.content` because it's already part of `compiler.sources` resulting in duplicated sources and therefore more work.
We always follow the `.gitignore` rules by default, and most people will ignore `.env` files. But just in case they don't, we will explicitly ignore these files.
This is just something I noticed during debugging. The `cwd` is already set to `process.cwd()` and our `base` is also just `process.cwd()`. But in the event that we change the `base` variable, then this will be up to date. This doesn't make a functional difference.
Contributor
|
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 as they are similar to previous changes (1)
WalkthroughAdded 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/@tailwindcss-upgrade/src/index.ts (1)
251-255:⚠️ Potential issue | 🟠 MajorSkip byte-identical stylesheet writes.
Line 247 already uses
originalsto skip no-op formatting, but Lines 251-255 still replace every discovered CSS file unconditionally. That keeps touching unchanged stylesheets on no-op runs.🛠️ Suggested fix
// Write all files to disk for (let sheet of stylesheets) { if (!sheet.file) continue - await writeFileSafely(sheet.file, sheet.root.toString()) + let next = sheet.root.toString() + if (originals.get(sheet) === next) continue + await writeFileSafely(sheet.file, next) if (sheet.isTailwindRoot) { success(`Migrated stylesheet: ${highlight(relative(sheet.file, base))}`, { prefix: '↳ ' }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@tailwindcss-upgrade/src/index.ts around lines 251 - 255, The loop that writes stylesheets unconditionally (iterating stylesheets and calling writeFileSafely for sheet.file with sheet.root.toString()) should skip writing when the generated content is byte-identical to the original; use the existing originals collection (referencing originals and stylesheets) to compare originals.get(sheet.file) (or the stored original content) against sheet.root.toString() and only call writeFileSafely(sheet.file, ...) when the contents differ, preserving timestamps for no-op runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integrations/upgrade/index.test.ts`:
- Around line 3148-3197: The fs.writeFile override uses
file.includes('src/templates') and a generic '__TRUNCATED_TARGET__' marker which
fails on Windows paths and makes the test assert a fixed template file; change
the check to normalize the incoming path (e.g., const normalized =
String(file).replace(/\\/g, '/'); if (!normalized.includes('src/templates'))
...) so backslashes are handled, and emit a file-specific marker (e.g.,
`__TRUNCATED_TARGET__:${path.basename(normalized)}`) when truncating; update the
process.onStderr listener to match the emitted marker (capture the filename from
stderr) and assert the preserved contents against the same specific file rather
than always template-0.php (adjust references to fs.writeFile override,
process.onStderr, and the originalTemplate/template variable usage accordingly).
In `@packages/`@tailwindcss-upgrade/src/utils/write-file-safely.ts:
- Around line 19-40: The current write flow writes to a temp file
(temporaryFile) then calls fs.rename(temporaryFile, file), which will replace a
symlink itself instead of updating its target; update write-file-safely to
detect symlinks before renaming: use fs.lstat(file) and if
lstat.isSymbolicLink() then resolve the link target (await fs.readlink(file) and
path.resolve(path.dirname(file), target)) and rename the temporaryFile into that
resolved target path instead of into file; if the file doesn't exist or isn't a
symlink, fall back to the existing fs.rename(temporaryFile, file) behavior, and
ensure error paths still clean up temporaryFile.
---
Outside diff comments:
In `@packages/`@tailwindcss-upgrade/src/index.ts:
- Around line 251-255: The loop that writes stylesheets unconditionally
(iterating stylesheets and calling writeFileSafely for sheet.file with
sheet.root.toString()) should skip writing when the generated content is
byte-identical to the original; use the existing originals collection
(referencing originals and stylesheets) to compare originals.get(sheet.file) (or
the stored original content) against sheet.root.toString() and only call
writeFileSafely(sheet.file, ...) when the contents differ, preserving timestamps
for no-op runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8a238fd2-cc29-409b-98c8-db0b203b366e
📒 Files selected for processing (7)
crates/oxide/src/scanner/fixtures/ignored-files.txtintegrations/upgrade/index.test.tsintegrations/utils.tspackages/@tailwindcss-upgrade/src/codemods/config/migrate-postcss.tspackages/@tailwindcss-upgrade/src/codemods/template/migrate.tspackages/@tailwindcss-upgrade/src/index.tspackages/@tailwindcss-upgrade/src/utils/write-file-safely.ts
We needed to update this because we want to ignore the git ignored files. But we also don't want to use auto content detection otherwise we don't really test the explicit content array. So with this relatively hacky way of tackling it, we _do_ scan files that are not ignored and not picked up by auto content detection.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/@tailwindcss-upgrade/src/index.ts (2)
252-261: Consider skipping writes for unchanged stylesheets.The
writeFileSafelycall writes all stylesheets regardless of whether they changed. While the safe-write utility prevents truncation on interruption, writing unchanged files is unnecessary I/O and could touch file timestamps without actual changes.The PR objectives mention "skip writing if migrated contents are unchanged" - this appears to be implemented for templates (via
migrateTemplatereturning a boolean) but not for stylesheets.♻️ Proposed fix to skip unchanged stylesheets
// Write all files to disk for (let sheet of stylesheets) { if (!sheet.file) continue + + // Skip writing if the stylesheet hasn't changed + if (originals.get(sheet) === sheet.root.toString()) continue await writeFileSafely(sheet.file, sheet.root.toString()) if (sheet.isTailwindRoot) { success(`Migrated stylesheet: ${highlight(relative(sheet.file, base))}`, { prefix: '↳ ' }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@tailwindcss-upgrade/src/index.ts around lines 252 - 261, The loop currently calls writeFileSafely unconditionally for each stylesheet (iterating over stylesheets, using sheet.file and sheet.root.toString()), which can unnecessarily update files; change it to first read the existing file contents (if present) and compare with sheet.root.toString(), and only call writeFileSafely when contents differ. Keep the existing checks for sheet.file and sheet.isTailwindRoot and only emit the success(...) message (which uses relative(sheet.file, base)) when a write actually occurs; this mirrors the migrateTemplate boolean-return behavior by skipping writes and timestamp changes for unchanged stylesheets.
359-371: Consider adding an explicit loop termination condition.The
do...while (parent)loop relies onisIgnored()throwing an error when escaping the repository to terminate. Sincepath.dirname()on a root path returns the same root path (e.g.,path.dirname('/') === '/'), this could theoretically loop indefinitely ifisIgnored()doesn't throw.In practice, this likely works because
isIgnoredthrows for paths outside the git repo, but an explicit termination condition would be more robust.♻️ Proposed improvement for loop robustness
let parent = path.dirname(file) + let previousParent = '' do { try { if (isIgnored(parent)) { culprit = parent } } catch { // Escaping the current repo break } + previousParent = parent parent = path.dirname(parent) - } while (parent) + } while (parent && parent !== previousParent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@tailwindcss-upgrade/src/index.ts around lines 359 - 371, The loop that walks up parent directories (using parent and path.dirname) currently relies on isIgnored() throwing to break out; change it to include an explicit termination check so it cannot loop forever: inside the do/while that updates parent, compare the newly computed parent to the previous value (or check for path.dirname(parent) === parent / root) and break when they are equal or when you reach the FS root, while still preserving the existing isIgnored() handling and setting culprit when isIgnored(parent) returns true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/`@tailwindcss-upgrade/src/index.ts:
- Around line 336-339: Wrap the fs.realpath(file) call in a try/catch to avoid
unhandled exceptions when resolving symlinks (e.g., broken symlink or deleted
file); on error, skip this file and continue the loop (optionally log or collect
the error) instead of letting it bubble. Update the loop that iterates
scanner.files and the realpath usage so that any exception from fs.realpath is
caught before calling file.endsWith or isIgnored; keep the existing isIgnored
error handling separate. Ensure you reference the existing scanner.files
iteration and the fs.realpath and isIgnored calls when making the change.
---
Nitpick comments:
In `@packages/`@tailwindcss-upgrade/src/index.ts:
- Around line 252-261: The loop currently calls writeFileSafely unconditionally
for each stylesheet (iterating over stylesheets, using sheet.file and
sheet.root.toString()), which can unnecessarily update files; change it to first
read the existing file contents (if present) and compare with
sheet.root.toString(), and only call writeFileSafely when contents differ. Keep
the existing checks for sheet.file and sheet.isTailwindRoot and only emit the
success(...) message (which uses relative(sheet.file, base)) when a write
actually occurs; this mirrors the migrateTemplate boolean-return behavior by
skipping writes and timestamp changes for unchanged stylesheets.
- Around line 359-371: The loop that walks up parent directories (using parent
and path.dirname) currently relies on isIgnored() throwing to break out; change
it to include an explicit termination check so it cannot loop forever: inside
the do/while that updates parent, compare the newly computed parent to the
previous value (or check for path.dirname(parent) === parent / root) and break
when they are equal or when you reach the FS root, while still preserving the
existing isIgnored() handling and setting culprit when isIgnored(parent) returns
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09de03ca-78e1-46ba-bf8d-2d2a2645955f
📒 Files selected for processing (6)
CHANGELOG.mdintegrations/upgrade/index.test.tsintegrations/upgrade/js-config.test.tspackages/@tailwindcss-upgrade/src/codemods/template/migrate.tspackages/@tailwindcss-upgrade/src/index.tspackages/@tailwindcss-upgrade/src/utils/write-file-safely.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/@tailwindcss-upgrade/src/codemods/template/migrate.ts
- packages/@tailwindcss-upgrade/src/utils/write-file-safely.ts
- integrations/upgrade/index.test.ts
No need for this additional `.kill()`
Member
Author
|
Update: I was finally able to reproduce it when running with While watching whenever the git status changed to files I knew should not be touched, I tried to quickly ctrl+c and eventually got it. I also added logging to ensure that the file in question didn't actually change (so no migration) but it was still written to disk, which was cancelled and ended up emptying the file. 
Good news is, I can't reproduce this now anymore, at all, with the changes implemented in this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is an attempt to make the upgrade tooling more stable.
TL;DR
config.contentinstead of relying on v4's auto content detection featureconfig.contentfile.envand.env.*files when scanning for files. Most people will have this in their.gitignorefile, but if not, then this is a fallback mechanism.Looking at the #18972 issue, it looks like some people are running into weird situations where some of the contents is just gone.
I have never been able to reproduce this on my own devices and in my own projects unfortunately. But there is definitely something happening that's not right that people are running into.
Therefore, this PR is an attempt to fix what I think might be wrong, but I'm not 100% sure if these fixes are enough, or if something else is still happening here.
This builds on top of the #19779 PR which has some small fixes, but is incomplete to make this work.
What's happening
Looking at some of the comments, it looks like a few things are happening such as:
Fixes
Emptying out files
The files being emptied looks like it's because how
fs.writeFilebehaves by default. It opens the file handle with thewflag, which will first truncate the file before writing the new contents. This is not a single atomic operation, so a killed process in the middle will cause invalid state.When we migrate your template files, everything is happening in promises to migrate things at the same time. When a lot of files are being scanned, truncating might have happened already before we write the new content. Since we migrate a bunch of files in parallel, a ctrl+c could cause data loss in multiple files.
To mitigate this, I switched to an alternative way of writing files.
First, we do some quick checks where if the contents didn't change we just bail out immediately. Files that don't include Tailwind CSS classes won't change, and therefore we don't need to override these files with the same contents.
When the migrated contents is empty, we bail out as well. I'm 100% sure that this is not the spot where the "emptying out" happens, I still believe it happens in the
writeFileitself, but added it just in case.Next, I introduced a safe write, where we first write to a temporary file in the same folder. We could write it to
/tmp, but then we can't guarantee that we are on the same file system.If we ctrl+c at this stage, then the worst case scenario is that you have additional temporary files in your project, but your original files are still there.
Once that file was written, we will use the atomic
fs.rename. This should be atomic as long as we are on the same file system, so either the rename didn't happen yet, or it completed.I added an integration test for this, but I had to change the
writeFileimplementation slightly. In the test, we will truncate the file first, after that we will write the new contents. This is so that we have enough time to kill the current process and allows us to verify that we didn't clear out the file. Again, this is a hacky way of testing this, just because I can't reproduce this issue myself, let alone reproduce it reliable in a CI environment.Note: we are also using
realpathto make sure that we are updating the real file. Otherwise, if we were dealing with a symlinked file, we would override the symlink with a "hard" copy instead.Touching files that should not be touched
During the migration, we rely on the Tailwind CSS v4 auto detection logic which means that it will scan any plain text file that is not git ignored. Therefore changes to php files could happen because in theory they could contain Tailwind CSS classes.
To solve this, when migrating from Tailwind CSS v3 to Tailwind CSS v4, we will only take the sources into account that were listed in the
config.contentarray. Since this was a requirement in Tailwind CSS v3, it should be safe to rely on this array.Additionally, this will make sure that we are dealing with way fewer files to migrate as well.
On top of that, files that match the patterns in the content array that are git ignored will also be skipped. This is to prevent that we mutate files in
node_modulesfor example.In one of the comments I read that
.envfiles were emptied out. In most cases people will have these files gitignored but I explicitly added.envand.env.*as files to never ever touch by default when scanning.Last but not least, this also updates the output a little bit of the upgrade tool in case we skip content files (because of git ignore) and if we changed a file.
./node_modules": this is because the content array looks like this while thenode_modulesare being ignored:./resources/views/vendor/filament-panels/components/logo.blade.php": this is because I made a change to showcase this feature.Fixes: #18972
Closes: #19779
Test plan
config.contentinto account when migrating from Tailwind CSS v3 to Tailwind CSS v4 projects.config.contentthat are also git ignored, will still be skipped.writeFileis cancelled mid-write that our old files are still present..envand.env.*files even if you didn't git ignore them.[ci-all] To verify on Windows