Repository navigation
Angular: Fix zone.js drop in angular-to-angular-vite migration + schema sync + transform widening - #35386
Merged
valentinpalkovic merged 14 commits intoJul 9, 2026
Conversation
…emas, widen preview transform match The angular-to-angular-vite automigration never re-added zone.js after moving projects off the webpack builder's implicit polyfill loading, breaking zone-based Angular apps (e.g. Bitwarden clients) after migrating to @storybook/angular-vite. Detection reads the flag-only signal (storybook builder target options.experimentalZoneless) across every angular.json/Nx project.json storybook target, injects import 'zone.js' into the preview when needed, and renames any leftover experimentalZoneless key to zoneless. Also fixes two adjacent bugs so the zoneless story lands coherently: - angular-vite's published start-schema.json/build-schema.json still declared the stale experimentalZoneless option instead of zoneless, so the correct zoneless: false setting failed schema validation while the stale key passed silently. - preset.ts's transform hook only matched a literal preview.ts, so .tsx (and other) preview files never received the runtime zone.js injection. It now resolves the preview path once via findConfigFile (core's own resolution list), cached in the plugin's config() hook.
Dropping the standalone schema-consistency test file per request; schema correctness for zoneless/experimentalZoneless is still exercised indirectly through the migration test suite and manual verification.
valentinpalkovic
changed the title
valentinpalkovic
added
bug
angular
ci:normal
valentinpalkovic
commented
Jul 7, 2026
valentinpalkovic
marked this pull request as ready for review
July 7, 2026 16:31
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames Angular Vite zoneless schema fields, adds format-preserving Angular JSON editing, caches preview path resolution, and updates the Angular-to-Angular Vite migration to rewrite Storybook targets, normalize zoneless flags, and inject ChangesZoneless schema, preset, and migration updates
Estimated code review effort: 4 (Complex) | ~60 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts (1)
647-651: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate
eslint-disablecomment and fix the indentation.There are now two consecutive
// eslint-disable-next-line depend/ban-dependenciescomments (lines 647 and 649), and theconst { globby }declaration is dedented to column 0 inside therun()body, which will fail Prettier/lint formatting checks.♻️ Proposed cleanup
- // eslint-disable-next-line depend/ban-dependencies - -// eslint-disable-next-line depend/ban-dependencies -const { globby } = await import('globby'); + // eslint-disable-next-line depend/ban-dependencies + const { globby } = await import('globby');🤖 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/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts` around lines 647 - 651, Remove the duplicated `eslint-disable-next-line depend/ban-dependencies` comment in `run()` and restore the `const { globby } = await import('globby');` indentation so it matches the surrounding block. Keep only one disable comment immediately before the import and align the following `projectJsonFiles` logic with the existing formatting in `angular-to-angular-vite.ts`.code/frameworks/angular-vite/src/preset.test.ts (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock setup violates repo Vitest spy-mocking conventions.
vi.mock()is missing thespy: trueoption, andmockFindConfigFile.mockReturnValue(...)is set inline within eachit()body instead of a sharedbeforeEach.♻️ Suggested refactor
vi.mock('storybook/internal/common', async (importOriginal) => ({ ...(await importOriginal<typeof import('storybook/internal/common')>()), findConfigFile: vi.fn(), -})); +}), { spy: true }); const mockFindConfigFile = vi.mocked(findConfigFile); const normalizePath = (p: string) => p.replace(/\\/g, '/'); const baseOptions = { configDir: '.storybook' } as unknown as StandaloneOptions; describe('angularOptionsPlugin', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + const extensions = ['ts', 'js', 'tsx', 'jsx', 'mjs', 'cjs']; it.each(extensions)( "injects `import 'zone.js';` for a resolved preview.%s when zoneless is false", async (ext) => { const resolvedPreviewPath = `.storybook/preview.${ext}`; - mockFindConfigFile.mockReturnValue(resolvedPreviewPath); + mockFindConfigFile.mockReturnValue(resolvedPreviewPath); // move to beforeEach if shared across cases ...As per coding guidelines,
**/*.{test,spec}.{ts,tsx}: "Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest", and**/*.test.{ts,tsx,js,jsx}: "Implement mock behaviors inbeforeEachblocks in Vitest tests" / "Avoid mocking without thespy: trueoption in Vitest tests" / "Avoid inline mock implementations within test cases in Vitest tests".Also applies to: 24-85
🤖 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 8 - 11, The Vitest mock setup for `findConfigFile` in `preset.test.ts` should follow the repo spy-mocking convention. Update the `vi.mock('storybook/internal/common', ...)` setup to use the `spy: true` option, and move any `mockFindConfigFile.mockReturnValue(...)` behavior out of individual `it()` blocks into a shared `beforeEach` so the tests use consistent mock initialization. Use the existing `mockFindConfigFile` reference and the `preset.test.ts` suite to keep the changes localized.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.
Nitpick comments:
In `@code/frameworks/angular-vite/src/preset.test.ts`:
- Around line 8-11: The Vitest mock setup for `findConfigFile` in
`preset.test.ts` should follow the repo spy-mocking convention. Update the
`vi.mock('storybook/internal/common', ...)` setup to use the `spy: true` option,
and move any `mockFindConfigFile.mockReturnValue(...)` behavior out of
individual `it()` blocks into a shared `beforeEach` so the tests use consistent
mock initialization. Use the existing `mockFindConfigFile` reference and the
`preset.test.ts` suite to keep the changes localized.
In `@code/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts`:
- Around line 647-651: Remove the duplicated `eslint-disable-next-line
depend/ban-dependencies` comment in `run()` and restore the `const { globby } =
await import('globby');` indentation so it matches the surrounding block. Keep
only one disable comment immediately before the import and align the following
`projectJsonFiles` logic with the existing formatting in
`angular-to-angular-vite.ts`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4af4cbf6-9ea9-47ef-a232-bd1b7e1d83b0
📒 Files selected for processing (6)
code/frameworks/angular-vite/build-schema.jsoncode/frameworks/angular-vite/src/preset.test.tscode/frameworks/angular-vite/src/preset.tscode/frameworks/angular-vite/start-schema.jsoncode/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.test.tscode/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts
A direct edit on the PR branch introduced a duplicated eslint-disable comment and mis-indented the globby dynamic import, tripping CI's format-check and eslint jobs. Restores the original correctly formatted line.
Sidnioulz
requested changes
Jul 8, 2026
Sidnioulz
left a comment
Sidnioulz
left a comment
Contributor
There was a problem hiding this comment.
I'm concerned with maintainability over this one. We should already have easier tools to manipulate both JSON and preview.
…rJSON path-configurable The angular.json/project.json rename-and-rewrite logic previously hand-rolled a string-literal-aware bracket-matching scanner to relocate targets for a scoped string replace, guarding against risks (same-named storybook targets across projects, brace characters inside string values) entirely by construction rather than by using a real JSON editor. Replaces it with jsonc-parser's modify/applyEdits: edits are addressed by structural JSON path, so the same-key-collision problem the scanner defended against becomes structurally impossible, and the diff stays minimal (verified against a real-world angular.json: only the touched lines change, compact arrays and trailing newline preserved) unlike a parse-mutate-JSON.stringify round trip, which was empirically shown to reformat the whole document. AngularJSON (used by `init` today) gains an optional path parameter and switches its own writes to the same format-preserving edits, so the automigrate fix can reuse it directly for angular.json instead of re-implementing JSON I/O. Nx project.json's flatter shape (no `projects.<name>` nesting) doesn't fit AngularJSON's model, so it uses the same underlying editJsonText primitive directly. jsonc-parser is added as a direct dependency (not bundled) rather than a devDependency, since esbuild's bundling of its CJS/UMD build broke on its internal relative requires.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts`:
- Around line 203-207: Guard the iteration in angular-to-angular-vite’s
automigration flow against a missing angularJSON.projects object. In the loop
that starts over Object.entries(angularJSON.projects), add an early check before
iterating so the code skips the file or returns safely when projects is
undefined or not an object, instead of letting Object.entries throw. Use the
existing angularJSON variable and the surrounding automigration logic in
angular-to-angular-vite to place the guard close to the iteration point.
🪄 Autofix (Beta)
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
Run ID: 3a607809-b5e1-4efe-a24c-6c007712ec2c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
code/core/package.jsoncode/core/src/cli/angular/helpers.tscode/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.test.tscode/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.test.ts
Contributor
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 73 | 🚨 +1 🚨 |
| Self size | 21.95 MB | 21.95 MB | 🚨 +2 KB 🚨 |
| Dependency size | 36.44 MB | 36.65 MB | 🚨 +213 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 32 | 33 | 🚨 +1 🚨 |
| Self size | 22.38 MB | 22.48 MB | 🚨 +94 KB 🚨 |
| Dependency size | 20.07 MB | 20.07 MB | 🚨 +497 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 205 | 🚨 +1 🚨 |
| Self size | 821 KB | 825 KB | 🚨 +4 KB 🚨 |
| Dependency size | 91.94 MB | 92.15 MB | 🚨 +216 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 198 | 🚨 +1 🚨 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 90.42 MB | 90.63 MB | 🚨 +216 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 74 | 🚨 +1 🚨 |
| Self size | 1.09 MB | 1.09 MB | 0 B |
| Dependency size | 58.39 MB | 58.60 MB | 🚨 +215 KB 🚨 |
| Bundle Size Analyzer | node | node |
…son target walk isStorybookTarget now narrows its argument to a StorybookBuilderTarget type instead of accepting any, letting the two call sites drop their manual typeof/any-cast checks. transformAngularJson and transformProjectJson shared the same per-target walk (detect, snapshot builder/executor + compodoc + zoneless, decide edits) and only differed in how an edit gets applied (in-place via AngularJSON vs. accumulated via editJsonText) and how targets are grouped (nested projects.<name>.architect vs. a single flat targets object). Factored the shared walk into processStorybookTargets, parameterized by a small TargetEditor interface that AngularJSON already satisfies and a new TextJsonEditor implements for the string-accumulating project.json case.
… import Pruned the comments this PR added down to ones that explain non-obvious behavior (edge cases, snapshot-before-edit reasoning, why a separate function exists) rather than narrating what the code already says. Also replaced addZoneJsPreviewImport's regex-based content scan/prepend with ConfigFile (readConfig/setImport), the same AST-based utility already used elsewhere in this file for main.ts. setImport(null, 'zone.js') is already idempotent (no-ops if the import exists) and inserts at the top of the file; the existing-import check is widened to also treat zone.js/* subpath imports as already-satisfied, matching prior regex behavior. Output is piped through formatFileContent so the written quote style matches the project's own Prettier config instead of whatever babel/recast prints by default.
Both describe the generic shape of an angular.json/project.json storybook builder target, not anything specific to the angular-to-angular-vite migration, so they belong alongside AngularJSON and editJsonText in code/core/src/cli/angular/helpers.ts and are re-exported via storybook/internal/cli like the rest of that module.
valentinpalkovic
commented
Jul 8, 2026
valentinpalkovic
commented
Jul 8, 2026
…rating-an-ngular-21-pro
….com:storybookjs/storybook into task/ffd6b664-Automigrating-an-ngular-21-pro
Sidnioulz
approved these changes
Jul 9, 2026
This was referenced Jul 14, 2026
Merged
Open
1 of 6 tasks
JReinhold
added
qa:success
Contributor
LogDetails |
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
3 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.
Closes #
What I did
Fixed the
angular-to-angular-viteautomigration droppingzone.jsfor zone-based Angular projects (e.g. Bitwarden clients), plus two adjacent bugs so the zoneless story lands coherently in one PR:options.experimentalZoneless !== true) across every:start-storybook/:build-storybooktarget inangular.json/Nxproject.json(skip injection only if EVERY storybook target explicitly sets the flagtrue). When needed, it idempotently prependsimport 'zone.js';to the Storybook preview file, and renames any leftoverexperimentalZonelesskey tozonelessin the migrated targets.@storybook/angular-vite's publishedstart-schema.json/build-schema.jsonstill declared the staleexperimentalZonelessoption instead ofzoneless, so the correctzoneless: falsesetting failed Angular CLI schema validation while the stale key passed silently. Both schemas now declarezonelessand no longer declareexperimentalZoneless.preset.ts's runtime transform hook only matched a literalpreview.ts, so.tsx(and other) preview files never received the zone.js injection at build time. It now resolves the preview path once viafindConfigFile(core's own preview-resolution list), cached in the plugin'sconfig()hook to avoid repeated disk I/O per transformed module.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This bug reproduces on any real Angular >= 21 project using zone-based change detection whose Storybook builder target does not explicitly set
experimentalZoneless: true(the common case, since the flag defaults to unset). Bitwarden'sclientsrepo is a good real-world reproduction:https://github.com/bitwarden/clientsandnpm install(node >= 22.12, npm ~10, per itsenginesfield).npx storybook@10.5.0-alpha.11 automigrate angular-to-angular-vite --yes --config-dir .storybook. Note: the checkout's.storybook/main.tshas awebpackFinalhook, which the automigration warns about and auto-declines continuing when--yesis passed — temporarily remove that hook frommain.tsbefore running the migration so it can proceed. Once migrated, open.storybook/preview.tsxand confirmimport 'zone.js';was not added, even though no storybook target inangular.jsonsetsexperimentalZoneless: true. This is the bug:@storybook/angular-vitedefaults to zoneless outsideng run, so zone-based apps loseNgZone-driven change detection after migrating.git checkout -- .to reset the checkout back to the pre-migration state (remove thewebpackFinalhook again), then run this branch's local build instead:node <path-to-this-checkout>/code/lib/cli-storybook/dist/bin/index.js automigrate angular-to-angular-vite --yes --skip-install --config-dir .storybook(buildcli-storybookandangular-vitefirst, e.g.yarn nx compile addon-vitest/ relevantnx compiletargets, oryarn buildinside those packages). Confirm the CLI logsAdded a zone.js import to .storybook/preview.tsxand thatimport 'zone.js';is now the first line of the file."zoneless": falseexplicitly on thecomponentsproject's storybook target inangular.jsonand confirmng run components:storybookpasses Angular CLI schema validation (noadditionalProperties/unknown-property rejection) — this exercises the fixedstart-schema.json/build-schema.json.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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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 pull request has been released as version
0.0.0-pr-35386-sha-531bdf43. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35386-sha-531bdf43 sandboxor in an existing project withnpx storybook@0.0.0-pr-35386-sha-531bdf43 upgrade.More information
0.0.0-pr-35386-sha-531bdf43task/ffd6b664-Automigrating-an-ngular-21-pro531bdf431783539260)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=35386Summary by CodeRabbit
New Features
zone.jsimport in the preview config when applicable.zonelessoption (defaulting to enabled).Bug Fixes
angular.json/project.jsonedits for more reliable workspace updates..ts/.tsx, includes dry-run handling) and better handling of preview-config path resolution.