Preview: Stop mixed CSF3+4 stories getting core annotations injected twice#35094
Conversation
📝 WalkthroughWalkthroughThis PR adds a marker-based deduplication system for core annotations across Storybook's CSF preview initialization. It introduces a non-enumerable Symbol to flag composed preview objects, enabling StoryStore, portable-stories, and CSF factories to conditionally apply core annotations only once. ChangesCore Annotation Deduplication
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs:
✨ Finishing Touches📝 Generate docstrings
Comment |
…a CSF4 preview CSF4 (definePreview) previews already compose the core annotations into their `composed` result, but the StoryStore constructor and the portable `setProjectAnnotations` both unconditionally prepended core again. This doubled core decorators/loaders/beforeEach/beforeAll for non-factory (CSF1/2/3) stories that fall back to `store.projectAnnotations`, and for the addon-vitest setup path. Flag the composed result with a `Symbol.for` marker (stable across the dual ESM/CJS module instances in dev) and skip prepending core when it is present. Co-authored-by: Cursor <cursoragent@cursor.com>
Fix oxfmt formatting in StoryStore.test.ts and align definePreview test fixtures with renderToCanvas typing used elsewhere in the codebase. Co-authored-by: Cursor <cursoragent@cursor.com>
41c39f6 to
c9b1aea
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/csf/core-annotations.ts (1)
51-56: 💤 Low valueConsider hardening the marker property descriptor.
The
configurable: trueandwritable: trueflags allow the marker to be deleted or modified after creation. Since this is an internal marker for deduplication logic, you could prevent tampering by usingconfigurable: false, writable: falseinstead.That said, the current approach is acceptable for controlled internal use and provides flexibility for testing or future adjustments.
🤖 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/core/src/csf/core-annotations.ts` around lines 51 - 56, The property descriptor for the internal deduplication marker (CORE_ANNOTATIONS_COMPOSED) is currently created on the annotations object via Object.defineProperty with configurable: true and writable: true; change this to configurable: false and writable: false to prevent accidental deletion or modification at runtime while still keeping enumerable: false and configurable only during definition; update the Object.defineProperty call that sets CORE_ANNOTATIONS_COMPOSED on annotations accordingly.
🤖 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/core/src/csf/core-annotations.ts`:
- Around line 51-56: The property descriptor for the internal deduplication
marker (CORE_ANNOTATIONS_COMPOSED) is currently created on the annotations
object via Object.defineProperty with configurable: true and writable: true;
change this to configurable: false and writable: false to prevent accidental
deletion or modification at runtime while still keeping enumerable: false and
configurable only during definition; update the Object.defineProperty call that
sets CORE_ANNOTATIONS_COMPOSED on annotations accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88caaeeb-6be0-40b6-b37e-932fbaa46124
📒 Files selected for processing (9)
code/core/src/csf/core-annotations.test.tscode/core/src/csf/core-annotations.tscode/core/src/csf/csf-factories.test.tscode/core/src/csf/csf-factories.tscode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/preview-api/modules/store/StoryStore.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/preview-api/modules/store/csf/processCSFFile.test.ts
✅ Files skipped from review due to trivial changes (1)
- code/core/src/csf/core-annotations.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- code/core/src/csf/csf-factories.test.ts
- code/core/src/preview-api/modules/store/csf/processCSFFile.test.ts
- code/core/src/preview-api/modules/store/csf/portable-stories.ts
- code/core/src/preview-api/modules/store/StoryStore.test.ts
- code/core/src/preview-api/modules/store/StoryStore.ts
- code/core/src/csf/csf-factories.ts
- code/core/src/preview-api/modules/store/csf/portable-stories.test.ts
yannbf
left a comment
There was a problem hiding this comment.
LGTM, though it seems like the logic is a little scattered and might be easy to miss a couple places if we need to modify the core annotations logic. I don't have a good solution though
Closes #
What I did
Core preview annotations (decorators, and by the same mechanism
beforeEach/beforeAll/loaders) were applied twice for a single render of a non-factory (CSF1/2/3) story when the project's.storybook/previewis a CSF4 (definePreview) factory preview.Root cause — core annotations were injected at multiple sites that stacked for a CSF4 preview:
definePreview(...).composedcomposes the core annotations in (csf-factories.ts).composedobject for CSF4 previews — so it already contains core.StoryStore's constructor (and the portablesetProjectAnnotationsused byaddon-vitest) then prepended core again.So
store.projectAnnotationsheld core twice under a CSF4 preview. CSF4 stories bypass it (they usecsfFile.projectAnnotations = preview.composed, core ×1), but non-factory stories fall back tostore.projectAnnotations→ doubled decorators/loaders/etc.defaultDecorateStorydoes not dedupe, so the duplicated entries execute twice.Fix (mark-and-skip)
core-annotations.ts: addmarkAsComposedWithCoreAnnotations()/hasCoreAnnotations(). The marker is a non-enumerableSymbol.forkey, so it resolves across the duplicate ESM/CJScoremodule instances that exist in dev (a plainSymbol()would differ per instance), is collision-proof against user annotation fields, and is hidden from enumeration/JSON/string-spread.csf-factories.ts: flagdefinePreview().composedwith the marker.StoryStore.ts: skip prepending core when the incoming project annotations are already flagged.portable-stories.ts:setProjectAnnotationsskips prepending core when any incoming annotation is flagged (fixes theaddon-vitestsetup path).This makes core inject exactly once for every (preview × story) combination, with no builder changes — both Vite and Webpack hand the store the same flagged
.composedobject.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
The doubling is not visible by default: the affected core annotations are either feature-gated or pass-through, so a duplicated decorator produces no extra DOM on its own. To observe it you need a temporary, always-on, framework-agnostic probe in core. Apply the patch below, which adds a pass-through core decorator and a core
beforeEachthat eachconsole.logwhen they run.Save as
core-probe.patchat the repo root and apply withgit apply core-probe.patch:Then:
.storybook/preview.tsalready ships a CSF4definePreviewpreview, so no preview edits are needed:yarn task sandbox --template react-vite/default-ts --start-from autoyarn nx compile core..storybook/main.ts, point thestoriesglob at the renderer fixtures so you have a non-factory and a factory story side by side:code/renderers/react/template/stories/csf1.stories.tsx(e.g. theHello1story)code/renderers/react/template/stories/csf4.stories.tsxnode_modules/.cachebefore restarting, otherwise Vite serves the stale pre-bundledcore.)next): the probe fires twice for a single render — two[core-probe] decorator #…lines and two[core-probe] beforeEach #…lines per navigation.decoratorline and onebeforeEachline per render.csfFile.projectAnnotations), and that the fix didn't regress them.__STORYBOOK_STORY_STORE__.projectAnnotations.decorators.filter(d => d.name === 'coreProbe').lengthreturns2onnextand1with this PR. (Reference-based dedupe would not catch this — the two copies are distinct function instances from twogetCoreAnnotations()calls — which is why the count, not identity, is the signal.)git apply -R core-probe.patchand rebuild core.Documentation
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 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
Bug Fixes
Tests