Builder-Vite: Fix cold-cache vitest failures for story paths containing glob special characters#34044
Conversation
…t cold-cache failures Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
|
View your CI Pipeline Execution ↗ for commit 5aac90b
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughA new utility function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts (2)
28-31: Consider adding test coverage for remaining escaped characters.The
escapeGlobPathregex escapes!,|,+, and@in addition to the characters currently tested. While these are less common in file paths, adding coverage would ensure the implementation is fully verified.Example additions:
it('should escape extglob prefix characters', () => { expect(escapeGlobPath('./src/!important/file.ts')).toBe('./src/\\!important/file.ts'); expect(escapeGlobPath('./src/a+b/file.ts')).toBe('./src/a\\+b/file.ts'); expect(escapeGlobPath('./src/@scope/file.ts')).toBe('./src/\\@scope/file.ts'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts` around lines 28 - 31, Add tests to cover the remaining characters escaped by escapeGlobPath (specifically '!', '|', '+', and '@'): update storybook-optimize-deps-plugin.test.ts to include a new it block (or extend the existing cases) that calls escapeGlobPath with paths containing each of these characters and asserts they are escaped with a backslash (e.g. './src/!important/file.ts' -> './src/\\!important/file.ts', './src/a+b/file.ts' -> './src/a\\+b/file.ts', './src/@scope/file.ts' -> './src/\\@scope/file.ts', and a case for '|' similarly). Ensure the test references the escapeGlobPath function and uses expect(...).toBe(...) for each input/expected pair so the regex coverage is validated.
39-43: Redundant test case.This test is functionally identical to the first test case on lines 6-8. Both verify that paths without special glob characters remain unchanged. Consider removing one or differentiating them (e.g., test with different path structures like absolute paths or paths with hyphens/underscores).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts` around lines 39 - 43, Remove or modify the redundant test with description "should not modify paths that contain no special glob characters" which duplicates the earlier test that asserts unchanged behavior for simple paths; either delete this second it(...) block or change it to cover a different case (e.g., use an absolute path, different separators, or names with hyphens/underscores) so the test `escapeGlobPath('./src/my-component/Button.stories.tsx')` provides unique coverage compared to the first test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts`:
- Around line 28-31: Add tests to cover the remaining characters escaped by
escapeGlobPath (specifically '!', '|', '+', and '@'): update
storybook-optimize-deps-plugin.test.ts to include a new it block (or extend the
existing cases) that calls escapeGlobPath with paths containing each of these
characters and asserts they are escaped with a backslash (e.g.
'./src/!important/file.ts' -> './src/\\!important/file.ts', './src/a+b/file.ts'
-> './src/a\\+b/file.ts', './src/@scope/file.ts' -> './src/\\@scope/file.ts',
and a case for '|' similarly). Ensure the test references the escapeGlobPath
function and uses expect(...).toBe(...) for each input/expected pair so the
regex coverage is validated.
- Around line 39-43: Remove or modify the redundant test with description
"should not modify paths that contain no special glob characters" which
duplicates the earlier test that asserts unchanged behavior for simple paths;
either delete this second it(...) block or change it to cover a different case
(e.g., use an absolute path, different separators, or names with
hyphens/underscores) so the test
`escapeGlobPath('./src/my-component/Button.stories.tsx')` provides unique
coverage compared to the first test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f2eef27-eeb3-4218-8630-ae950bbccb0f
📒 Files selected for processing (2)
code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.tscode/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.ts
When story files live inside directories with parentheses (e.g. Next.js route group dirs like
src/(group)/...), the first cold-cache run fails becauseoptimizeDeps.entriespaths are passed raw to Vite, which processes them viafast-glob— interpreting(group)as an extglob pattern rather than a literal path segment. This causes the story file to be missed during the initial dep scan, triggering a runtime re-optimization and the "Vite unexpectedly reloaded a test" failure.Manual testing instructions:
Changes
storybook-optimize-deps-plugin.ts: AddedescapeGlobPath()that backslash-escapes glob special characters (()[]{}!*?|+@) in file paths. Applied to both story import paths and preview annotation paths before they're spread intooptimizeDeps.entries.storybook-optimize-deps-plugin.test.ts: New test file coveringescapeGlobPathfor parentheses, brackets, braces, wildcards, and plain paths.Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit
Release Notes
New Features
Tests