NextJS-Vite: Automatically fix bad PostCSS configuration #32691
Conversation
- Added `lilconfig` dependency to manage PostCSS configurations. - Introduced `find-postcss-config.ts` to locate and load PostCSS config files with a fallback mechanism. - Updated `preset.ts` to utilize the new PostCSS loading functionality, improving error handling for incompatible configurations.
📝 WalkthroughWalkthroughAdds devDependency Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Preset caller
participant Preset as preset.ts
participant Finder as find-postcss-config.ts
participant Lil as lilconfig("postcss")
participant Loader as postcss config loader
participant FS as File System
Caller->>Preset: initialize preset (config, options)
Preset->>Preset: compute searchPath
alt searchPath exists
Preset->>Finder: normalizePostCssConfig(searchPath)
Finder->>Lil: search(searchPath) with custom loaders
Lil-->>Finder: configPath or null
alt configPath found
Finder->>Loader: load(configPath)
alt load succeeds
Loader-->>Finder: config
Finder-->>Preset: return true
else Invalid PostCSS Plugin error
Finder->>FS: read configPath
Finder->>FS: rewrite Tailwind plugin entry (array -> object)
Finder->>Loader: retry load(configPath)
alt retry succeeds
Loader-->>Finder: config
Finder-->>Preset: return true
else
Finder-->>Preset: throw IncompatiblePostCssConfigError
end
else other load error
Finder-->>Preset: return false
end
else no config found
Finder-->>Preset: return true
end
else no searchPath
Preset->>Preset: skip PostCSS processing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (1)
1-1: Remove redundant directive.The
@ts-checkcomment is unnecessary in TypeScript files as type checking is enabled by default.Apply this diff:
-// @ts-check import { readFile, writeFile } from 'node:fs/promises';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs-vite/src/find-postcss-config.ts(1 hunks)code/frameworks/nextjs-vite/src/preset.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite/src/preset.tscode/frameworks/nextjs-vite/src/find-postcss-config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite/src/preset.tscode/frameworks/nextjs-vite/src/find-postcss-config.ts
🧬 Code graph analysis (2)
code/frameworks/nextjs-vite/src/preset.ts (1)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (1)
loadPostCssConfigWithFallback(79-120)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
code/core/src/common/utils/paths.ts (1)
getProjectRoot(11-57)code/core/src/server-errors.ts (1)
IncompatiblePostCssConfigError(498-526)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/frameworks/nextjs-vite/package.json (1)
90-90: LGTM!The addition of
lilconfigas a devDependency is appropriate for the new PostCSS config discovery functionality.code/frameworks/nextjs-vite/src/preset.ts (1)
64-69: LGTM!The refactored PostCSS config loading logic correctly determines the search path and delegates to the new utility function, simplifying error handling.
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
24-61: LGTM!The
withLoadersfunction correctly configures lilconfig with comprehensive file extension support and search locations for PostCSS configs.
70-74: LGTM!The
postCssFindConfigfunction correctly uses lilconfig to locate PostCSS configuration files.
|
View your CI Pipeline Execution ↗ for commit 857bfc4
☁️ Nx Cloud last updated this comment at |
| const modifiedContent = originalContent.replace( | ||
| 'plugins: ["@tailwindcss/postcss"]', | ||
| 'plugins: { "@tailwindcss/postcss": {} }' | ||
| ); |
There was a problem hiding this comment.
This approach will not work for yml files.
There was a problem hiding this comment.
I do not think we need it.
I had to copy code from here:
https://github.com/postcss/postcss-load-config/blob/main/src/index.js
And i wanted to keep it close to the original code.
yaml is already included in the bundle.
There was a problem hiding this comment.
If we don't support modifying YAML-based PostCSS files, we should clean up the code above (searching for YAML files, registering a YAML config loader,...). Then we can also actually remove the yaml dependency.
There was a problem hiding this comment.
You would rather not keep the code similar to where we got it from? Okay.
Their code seems to prefer yaml config files, so our code skipping/not looking for them, might cause their code to find different files, if the user has many different ones, possibly on multiple levels.
The replacement code, also does not "work" for JSON files, should I remove handling for those as well?
- Refactored PostCSS configuration loading by renaming `loadPostCssConfigWithFallback` to `normalizePostCssConfig` for clarity. - Enhanced error handling in `normalizePostCssConfig` to manage incompatible configurations more effectively. - Added `yaml` dependency version `2.8.1` to `package.json` and updated `yarn.lock` accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (1)
146-146: Unhandled errors should be thrown, not silently returned asfalse.Returning
falsefor unexpected errors masks real failures. If an error occurs that doesn't match the expected patterns ("No PostCSS Config found" or "Invalid PostCSS Plugin found"), it should be thrown to surface the issue to the caller.Apply this diff to throw unhandled errors:
- return false; + // Unhandled error - rethrow to surface the issue + throw error; };
🧹 Nitpick comments (2)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
1-1: Remove redundant@ts-checkdirective in TypeScript file.The
@ts-checkcomment is unnecessary in.tsfiles where TypeScript type checking is already enabled by default. This directive is typically used in.jsfiles to opt into TypeScript checking.Apply this diff:
-// @ts-check import { readFile, writeFile } from 'node:fs/promises';
70-74: Consider documenting whypostCssFindConfigis not exported.Based on past review comments, this function was intentionally kept internal. However, since it has comprehensive JSDoc documentation suggesting it's part of the public API, consider either:
- Adding a note in the JSDoc explaining it's internal-only, or
- Removing the JSDoc if it's truly internal
This will prevent confusion about the module's public API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs-vite/src/find-postcss-config.ts(1 hunks)code/frameworks/nextjs-vite/src/preset.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/frameworks/nextjs-vite/src/preset.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite/src/find-postcss-config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite/src/find-postcss-config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:21:27.284Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.284Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/frameworks/nextjs-vite/src/find-postcss-config.ts
🧬 Code graph analysis (1)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
code/core/src/common/utils/paths.ts (1)
getProjectRoot(11-57)code/core/src/server-errors.ts (1)
IncompatiblePostCssConfigError(498-526)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
16-22: LGTM: Loaders are correctly implemented.The custom loaders for JavaScript/TypeScript modules and YAML files are properly implemented. The use of
createRequirefor ES module compatibility is appropriate.
129-132: Robustify PostCSS plugin replacement
The currentreplace('plugins: ["@tailwindcss/postcss"]', …)only matches the exact default format and will break on formatting changes, multiple plugins, or plugin options—either document this limitation (in a comment or the error message) or switch to a regex/AST-based transform.code/frameworks/nextjs-vite/package.json (1)
90-90: Dependencies correctly added.The
lilconfigandyamldevDependencies are now properly declared, resolving the previous review concern about undeclared dependencies.Note: A past review comment questioned whether
yamlsupport adds unnecessary bundle size, since the primary use case is handling Next.js's default JavaScript config. This is a maintainer decision, but consider measuring the bundle size impact if this becomes a concern.Based on past review comments.
Also applies to: 95-95
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (3)
76-76: Reconsider this re-export to keep the API surface minimal.This export might not be necessary if callers only need
normalizePostCssConfig. Evaluate whether any code directly importspostCssLoadConfigfrom this module.Based on learnings from past review.
140-143: Fix: Useunknowninstead ofanyfor error variable.Line 140 uses
anytype for the caught error, which bypasses TypeScript's type safety. This violates the coding guidelines requiring strict mode compliance.As per coding guidelines.
Apply this diff:
- } catch (e: any) { + } catch (e: unknown) { // We were unable to fix the config, so we throw an error throw new IncompatiblePostCssConfigError({ error }); }Note: Since you're only using the original
errorvariable (note) when constructingIncompatiblePostCssConfigError, the type change won't affect the throw statement.
146-146: Fix: Throw unhandled errors instead of returning false.Returning
falsefor unexpected errors silently masks real failures. If an error doesn't match the known patterns (e.g., neither "No PostCSS Config found" nor "Invalid PostCSS Plugin found"), it should be thrown to surface the issue.Apply this diff:
- return false; + // Unhandled error case - rethrow to surface the issue + throw error; };This ensures that unexpected errors during PostCSS config loading are properly propagated to the caller.
🧹 Nitpick comments (2)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
20-22: Consider renaming unused parameter for clarity.The first parameter of
yamlLoaderis unused. Convention is to name it with a leading underscore (which you did), but consider naming it descriptively (e.g.,_filepath) for better self-documentation.-async function yamlLoader(_: string, content: string) { +async function yamlLoader(_filepath: string, content: string) { return yaml.parse(content); }
40-59: Potential duplication in searchPlaces array.Line 41 spreads
options.searchPlaces ?? []before the explicit list. If callers provide overlapping search places, this could lead to duplicate searches. Consider whether the spread should come after the explicit list or if deduplication is needed.If you want caller-provided paths to take precedence:
searchPlaces: [ - ...(options.searchPlaces ?? []), 'package.json', `.${moduleName}rc`, // ... rest of the list `${moduleName}.config.mjs`, + ...(options.searchPlaces ?? []), ],Or deduplicate using a Set if order and uniqueness both matter:
searchPlaces: Array.from(new Set([ ...(options.searchPlaces ?? []), 'package.json', // ... rest ])),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs-vite/src/find-postcss-config.ts(1 hunks)code/frameworks/nextjs-vite/src/preset.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/frameworks/nextjs-vite/package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite/src/find-postcss-config.tscode/frameworks/nextjs-vite/src/preset.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite/src/find-postcss-config.tscode/frameworks/nextjs-vite/src/preset.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:21:27.284Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.284Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/frameworks/nextjs-vite/src/find-postcss-config.ts
🧬 Code graph analysis (2)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
code/core/src/common/utils/paths.ts (1)
getProjectRoot(11-57)code/core/src/server-errors.ts (1)
IncompatiblePostCssConfigError(498-526)
code/frameworks/nextjs-vite/src/preset.ts (1)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (1)
normalizePostCssConfig(97-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
63-74: LGTM!The
postCssFindConfighelper is well-implemented with clear JSDoc and appropriate encapsulation as a private function.
129-132: Ensure plugin replacement handles all PostCSS config variants
The current replace only matches the exact stringplugins: ["@tailwindcss/postcss"]and will miss different spacing, quotes, or multiple plugins—either document that Next.js always scaffolds this exact format or switch to a regex/AST-based approach to cover all variants.code/frameworks/nextjs-vite/src/preset.ts (1)
64-69: No additional guard required for undefined searchPath
The existingif (searchPath)check already prevents callingnormalizePostCssConfigwhen bothconfig.css?.postcssandconfig.rootare undefined.
ndelangen
changed the title
- Eliminated the YAML loader and related search places from the PostCSS configuration handling. - Simplified the code structure in `find-postcss-config.ts` for better maintainability. - Ensured compatibility with existing PostCSS configurations while enhancing error handling.
…date yarn.lock accordingly
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
138-138: Unhandled errors should be thrown, not silently returned.Returning
falsefor unexpected errors masks real failures. Although a previous review marked this as "addressed in commit c874228", the code still returnsfalseinstead of throwing.Apply this diff to throw unhandled errors:
- return false; + // Unhandled error - rethrow to surface the issue + throw error; };
66-66: Remove unused export ofpostCssLoadConfig.
postCssLoadConfigisn’t referenced anywhere else—remove this export to keep the module’s API surface clean.- export { postCssLoadConfig };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs-vite/src/find-postcss-config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/frameworks/nextjs-vite/package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite/src/find-postcss-config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite/src/find-postcss-config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:21:27.284Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.284Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/frameworks/nextjs-vite/src/find-postcss-config.ts
🧬 Code graph analysis (1)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (2)
code/core/src/common/utils/paths.ts (1)
getProjectRoot(11-57)code/core/src/server-errors.ts (1)
IncompatiblePostCssConfigError(498-526)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/frameworks/nextjs-vite/src/find-postcss-config.ts (4)
1-12: LGTM! Clean imports.The imports are well-organized and appropriate for the PostCSS config handling functionality.
14-51: LGTM! Appropriate loader configuration.The loader setup correctly handles JS/TS variants without adding yaml complexity, aligning with the decision to keep the implementation simple.
60-64: LGTM! Clean helper function.The function appropriately remains unexported, keeping the module's API surface minimal.
93-103: LGTM! Proper error handling.The error handling correctly uses
Error | undefinedand properly type-guards withinstanceof Error, addressing previous review feedback.
shilman
changed the title
shilman
added
the
patch:yes
shilman
changed the title
…ification NextJS: Enhance PostCSS configuration handling (cherry picked from commit e9a2317)
github-actions
bot
added
the
patch:done
ndelangen
removed
the
patch:yes
What I did
NextJS ships with a invalid postcss config file.
Storybook then fails to run, the solution is a simple change to the postcss config file.
We've asked users to make this change manually, but that breaks the onboarding flow, so this PR attempts to fix the postcss config file automatically.
We make a string-replacement on the contents of the file, and if that does not solve the issue, we revert the file and throw the error we did before.
Related to this PR/Issue:
vercel/next.js#79949
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
I've manually tested this on my machine using a sandbox.
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Refactor
Chores