Repository navigation
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe Angular Vite plugin now derives a workspace root during configuration, resolves SCSS and preview style paths from that root, supports Angular style objects, filters ChangesAngular Vite preview style handling
Possibly related PRs
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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: 1
🧹 Nitpick comments (1)
code/frameworks/angular-vite/src/preset.test.ts (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a spy mock with behavior configured in
beforeEach.The factory mock bypasses
spy: trueand does not usevi.mocked(). The helper changes its behavior through shared state. Use a top-levelvi.mock(import('storybook/internal/common'), { spy: true }), then configurevi.mocked(findConfigFile)inbeforeEach.
code/frameworks/angular-vite/src/preset.test.ts#L15-L19: Replace the factory mock with a spy mock.code/frameworks/angular-vite/src/preset.test.ts#L158-L179: Set and resetfindConfigFilemock behavior inbeforeEachandafterEach.As per coding guidelines, use
vi.mock()with thespy: trueoption, usevi.mocked(), and implement mock behaviors inbeforeEachblocks.🤖 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 `@code/frameworks/angular-vite/src/preset.test.ts` around lines 15 - 19, Replace the factory mock in code/frameworks/angular-vite/src/preset.test.ts lines 15-19 with a top-level spy mock for storybook/internal/common. In the beforeEach block at lines 158-179, configure vi.mocked(findConfigFile) with the required previewPath behavior, and reset or restore that mock in afterEach; remove the shared mocks state and use vi.mocked() consistently.Source: Coding guidelines
🤖 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 `@code/frameworks/angular-vite/src/preset.test.ts`:
- Around line 187-189: Update the expected global-style import strings in the
angularOptionsPlugin tests to wrap each resolve(WORKSPACE_ROOT, ...) result with
normalizePath, covering the indicated assertions. Keep the existing import paths
and expectations unchanged apart from applying the same normalization used by
the implementation.
---
Nitpick comments:
In `@code/frameworks/angular-vite/src/preset.test.ts`:
- Around line 15-19: Replace the factory mock in
code/frameworks/angular-vite/src/preset.test.ts lines 15-19 with a top-level spy
mock for storybook/internal/common. In the beforeEach block at lines 158-179,
configure vi.mocked(findConfigFile) with the required previewPath behavior, and
reset or restore that mock in afterEach; remove the shared mocks state and use
vi.mocked() consistently.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69dca22e-e4de-4310-8a67-6e0d1a56831c
📒 Files selected for processing (2)
code/frameworks/angular-vite/src/preset.test.tscode/frameworks/angular-vite/src/preset.ts
b374582 to
58de2dd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Closing this as superseded. The bug was real: This PR still does the unconditional join, so merging it would undo #35998. Object |
Closes #
What I did
angularOptionsPluginresolves two path options, and it was using a different root for each one.stylePreprocessorOptions.includePathsusesangularBuilderContext?.workspaceRoot ?? userConfig?.root ?? process.cwd(), which is right. Thestylestransform a few lines further down usedresolvedConfig?.root ?? process.cwd(). That isthe same Vite config object, but it skips the builder context. So when a builder context is
present, SCSS include paths resolved against the workspace root while global styles resolved
against the project directory.
In any workspace where those two are not the same directory, the configured stylesheet never
loads. Nothing fails, the styles are just missing.
There were three problems in that transform:
stylesentries are workspace-relative, same as inangular.json.{ input, bundleName, inject }, but entries werepushed through as-is and then called with
.startsWith(...), so an object entry gaveTypeError: extraImport.startsWith is not a function.startsWith('.') || startsWith('src').That gets both directions wrong.
theme/global.csscame out as a bare import and neverresolved, and
srcery/index.csswas treated as a path.The transform now turns every entry into its object form, resolves all of them against
workspaceRoot, and drops the guessing. The schema already tells us what each entry is, so thereis nothing to infer. Entries with
inject: falseare skipped, because Angular emits those asstandalone bundles instead of injecting them into the page.
zone.jsis appended as its ownimport rather than going into the same list and relying on the old check to leave it alone.
workspaceRootis now worked out once inconfig()and shared, which is what stops the twooptions from disagreeing again.
Behaviour change
If you are in a multi-project workspace and worked around the old behaviour by writing
project-relative
stylespaths, those need to become workspace-relative, matching the schema,angular.json, andstylePreprocessorOptions.includePaths. Single-project workspaces are notaffected, because there the two roots are the same directory.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Seven cases added to
preset.test.ts: workspace-relative paths,src-prefixed and dot-relativepaths, the expanded
{ input }form,inject: false, the fallback to the Vite root when there isno builder context,
zone.jsonly showing up when not zoneless, and modules other than the previewbeing left alone. They fail on the old code.
Manual testing
yarn task --task sandbox --start-from auto --template angular-vite/default-ts"root": "projects/app"inangular.json, movesrc/toprojects/app/src/, and updatesourceRoot.theme/global.cssat the workspace root withbody { background: deeppink }."styles": ["theme/global.css"]to thestorybooktarget.ng run app:storybookOn the old code the background does not change. The entry starts with neither
.norsrc, so itcomes out as
import 'theme/global.css'and never resolves. With this change the background ispink.
Then change the entry to
[{ "input": "theme/global.css" }]and run it again. On the old code thatthrows
TypeError: extraImport.startsWith is not a function. With this change it does the samething as the string form.
Documentation
MIGRATION.MD