Repository navigation
Vue3: Only reload the preview when docgen changes on hot update - #35705
huang-julien merged 3 commits into
Conversation
The vue-component-meta plugin's handleHotUpdate sent a Vite full-reload on every file change, so editing a template, style or any component internal reloaded the whole preview iframe and lost transient story state such as open dialogs, form values, focus and scroll position. Cache the docgen emitted for each module in the transform hook and, on hot update, recompute it after refreshing the checker. When the docgen is unchanged, return undefined so Vue's own HMR handles the edit. The reload is kept for changes that actually alter the generated docs, and for files that have no docgen yet.
|
Danger is failing on the required labels ( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe Vue component metadata plugin caches serialized metadata during transforms and recomputes metadata for dependent modules during hot updates. Unchanged metadata preserves normal Vue HMR. Changed or unavailable metadata triggers a full reload. ChangesVue component metadata flow
Sequence Diagram(s)sequenceDiagram
participant Vite
participant vueComponentMeta
participant Checker
participant WebSocket
Vite->>vueComponentMeta: process hot update
vueComponentMeta->>Checker: synchronize checker
vueComponentMeta->>vueComponentMeta: recompute dependent metadata
alt Metadata unchanged
vueComponentMeta-->>Vite: return undefined
else Metadata changed or unavailable
vueComponentMeta->>WebSocket: send full-reload
vueComponentMeta-->>Vite: return empty module list
end
Priority: ➖ Normal — Impact reflects medium issue severity. Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Vue metadata hot updates now preserve story state when generated metadata is unchanged while retaining full reloads when metadata changes or has no cached baseline. No actionable current-head merge risk remains. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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: 1
🧹 Nitpick comments (3)
code/frameworks/vue3-vite/src/plugins/vue-component-meta.test.ts (2)
249-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfigure the changed-docgen mock in
beforeEach.Line 253 changes mock behavior inside the test case and accesses the mock without
vi.mocked(). Move this setup into a nesteddescribeblock with abeforeEach, and configure it throughvi.mocked(mockChecker.getComponentMeta).As per coding guidelines, “Use
vi.mocked()to type and access the mocked functions in Vitest tests” and “Implement mock behaviors inbeforeEachblocks in Vitest tests.”🤖 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/vue3-vite/src/plugins/vue-component-meta.test.ts` around lines 249 - 256, Move the changed-docgen setup from the test body into a nested describe block’s beforeEach, configuring mockChecker.getComponentMeta through vi.mocked(). Keep the test focused on reload behavior while preserving the existing returned metadata with the newProp entry.Source: Coding guidelines
39-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the Vite hook contract in the test helper.
Line 41 casts
handleHotUpdateto acceptunknown. This prevents TypeScript from validating thatmakeHotUpdateContextmatches the Vite HMR context. Type the handler andctxfrom the plugin hook signature so changes to required fields, such asmodulesorserver, fail at compile time.As per coding guidelines, “Encode invariants with TypeScript types and existing lint rules first.”
🤖 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/vue3-vite/src/plugins/vue-component-meta.test.ts` around lines 39 - 65, Update makeHotUpdateContext and the plugin wrapper to derive the handleHotUpdate parameter type from the Vite hook signature instead of using unknown and a manually shaped context. Type the returned ctx and helper callback against that inferred hook parameter so required fields such as modules and server are checked by TypeScript.Source: Coding guidelines
code/frameworks/vue3-vite/src/plugins/vue-component-meta.ts (1)
36-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional:
computeMetaSourcesmixes several concerns in one function.The function gathers export names, calls
applyTempFixForEventDescriptions, filters empty/unknown meta, removes nested schemas, and de-duplicatesexposedentries, all inline in oneforEach. Extracting the per-component normalization (lines 59-113) into a small helper (for examplenormalizeComponentMeta(meta, exportName, id)) would let each concern be unit-tested and read independently, without changing behavior.This is optional. The current extraction from the old inline transform logic is already a net improvement.
🤖 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/vue3-vite/src/plugins/vue-component-meta.ts` around lines 36 - 117, Optionally refactor computeMetaSources by extracting the per-component normalization logic into a helper such as normalizeComponentMeta(meta, exportName, id). Keep export collection, applyTempFixForEventDescriptions, empty/unknown filtering, nested-schema removal, exposed-entry de-duplication, and generated MetaSource values behaviorally unchanged.
🤖 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/vue3-vite/src/plugins/vue-component-meta.ts`:
- Around line 129-130: In the transform flow around computeMetaSources, check
metaSources.length before caching the result in emittedMeta. Only call
emittedMeta.set for non-empty metadata, while preserving the existing undefined
return for empty docgen so handleHotUpdate retains full-reload behavior.
---
Nitpick comments:
In `@code/frameworks/vue3-vite/src/plugins/vue-component-meta.test.ts`:
- Around line 249-256: Move the changed-docgen setup from the test body into a
nested describe block’s beforeEach, configuring mockChecker.getComponentMeta
through vi.mocked(). Keep the test focused on reload behavior while preserving
the existing returned metadata with the newProp entry.
- Around line 39-65: Update makeHotUpdateContext and the plugin wrapper to
derive the handleHotUpdate parameter type from the Vite hook signature instead
of using unknown and a manually shaped context. Type the returned ctx and helper
callback against that inferred hook parameter so required fields such as modules
and server are checked by TypeScript.
In `@code/frameworks/vue3-vite/src/plugins/vue-component-meta.ts`:
- Around line 36-117: Optionally refactor computeMetaSources by extracting the
per-component normalization logic into a helper such as
normalizeComponentMeta(meta, exportName, id). Keep export collection,
applyTempFixForEventDescriptions, empty/unknown filtering, nested-schema
removal, exposed-entry de-duplication, and generated MetaSource values
behaviorally unchanged.
🪄 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 Plus
Run ID: 754a8f8e-235b-450d-8328-6038e7bf5692
📒 Files selected for processing (2)
code/frameworks/vue3-vite/src/plugins/vue-component-meta.test.tscode/frameworks/vue3-vite/src/plugins/vue-component-meta.ts
valentinpalkovic
added
bug
vue
vue3
docgen
hmr
sev:S2
![storybook-contribution-review[bot]](https://avatars.githubusercontent.com/u/22632046?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot; a human maintainer approved this message before it was posted.
Review phase: Architecture review (stage 1 of 3)We are trying out something new: a three-stage automated review - architecture, then code quality, then runtime verification - that runs on a PR before a human maintainer looks at it closely. A maintainer still reads this and still makes the call; the aim is that by the time they do, the reachability and evidence work is already done. Tell us how it lands for you, that feedback is genuinely useful to us right now.
Hi @alliasgher,
Thank you for tackling the Vue HMR state-loss problem. Keeping this decision inside the @storybook/vue3-vite plugin is the right ownership boundary, and preserving open dialogs, form state, focus, and scroll position during style-only edits is a valuable improvement.
I am requesting changes because the cache currently compares only the file passed to handleHotUpdate. Vue component metadata is whole-program output, so a dependency can change an already-emitted component's docgen even when that dependency has no docgen of its own.
There is a concrete path in the existing Vue fixture: component.vue imports the Severity enum from severity.ts. After both files have been transformed, severity.ts is cached as []. If that enum changes, checker.updateFile refreshes the program, but the hook recomputes only severity.ts, compares [] with [], and returns undefined. That suppresses the full reload even though component.vue's injected prop metadata changed, which can leave Controls and docs stale. The three new tests cover same-file changes and an uncached file, but not this dependency case.
To make this acceptable:
- Rebase onto current
nextand keep normalized extraction in the existingcollectComponentMetaSourcespath exposed throughexperimental_vueDocgenEngine; please do not restore a secondcomputeMetaSourcesimplementation invue3-vite. - Cache only component module IDs whose transform actually emitted
__docgenInfo. - After
checker.updateFile, recompute every potentially affected cached component. Recomputing all cached emitted component IDs is a safe first version; a reverse-dependency index is also fine if it includes type-only imports that Vite may erase. - When a change is detected, reload without advancing the cached baseline first. Let the subsequent transform store the metadata that was actually emitted.
- Add a regression test where a cached component imports a type or enum from another
.tsfile and editing that dependency changes the component metadata and triggers a reload. Please keep the unchanged style/template case too.
This review is against head 927706f90c35311717406869cedadcabfb1cd5f4. Once the dependency-aware invalidation path is covered, the architecture gate can run again.
|
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. |
storybook-contribution-review
Bot
added
the
review:changes-requested
huang-julien
added
the
ci:normal
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
storybook-contribution-review
Bot
added
sev:S3
huang-julien
added
the
qa:needed
3 participants
Closes #35653
What I did
The
vue-component-metaplugin'shandleHotUpdatesent a Vitefull-reloadon every file change. The reload only exists to get fresh docgen into the preview, but it fired even when the generated meta was identical, so editing a template, a style block or any component internal reloaded the whole preview iframe and lost transient story state (open dialogs, form values, focus, scroll position).This PR makes the reload conditional:
transformhook now caches the docgen it emits for each module (emittedMeta).handleHotUpdatestill refreshes the checker with the new content (so the checker never goes stale), then recomputes the docgen and compares it to what was last emitted.undefinedand Vue's own HMR handles the edit, preserving story state.full-reloadbehaviour is kept unchanged.The meta extraction that
transformalready performed is pulled out into acomputeMetaSourceshelper so both hooks share one implementation. Most of the diff is that move; the behavioural change is confined tohandleHotUpdate.This addresses the first half of the issue (preserve Vue HMR when docgen is unchanged). Updating the metadata without a reload when it does change is a bigger change and I left it out of scope here.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Three cases were added to
vue-component-meta.test.ts: docgen unchanged (no reload, and the checker is still updated), docgen changed (reload), and a file with no docgen yet (reload). I confirmed the first test fails without the production change.Manual testing
App.stories.ts.DialogContent(a style-only change) and save. Expected: the style updates via Vue HMR and the dialog stays open. Onnextthe preview full-reloads and the dialog closes.Note
I was not able to run the repo's full Vitest suite locally (the monorepo install did not complete in my environment), so I ran
vue-component-meta.test.tsagainst the real plugin source in an isolated Vitest project withstorybook/internal/commonandstorybook/internal/oxc-parserstubbed. All 15 tests pass there (12 existing + 3 new). Please let CI confirm on the real harness.