Maintenance: Extract createPreviewAnnotations factory to internal/common#34394
Maintenance: Extract createPreviewAnnotations factory to internal/common#34394mixelburg wants to merge 3 commits into
Conversation
…rnal/common The previewAnnotations preset function was copy-pasted identically across 5 renderer preset files (html, preact, svelte, vue3, web-components). The only difference was the package name string and, for web-components, an extra entry-preview-argtypes entry. Extract a createPreviewAnnotations(packageName, extraEntries?) factory to storybook/internal/common and use it in all 5 renderers. This eliminates the duplication and ensures future changes to the docsEnabled check or the result-building logic only need to be made in one place.
📝 WalkthroughWalkthroughA shared factory Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/core/src/common/utils/preview-annotations.ts (2)
11-25: Add a focused regression test around this factory.This helper now fans out to five renderer presets, so one small test for docs on/off plus
extraEntriesordering would catch behavior drift without duplicating coverage in every preset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/common/utils/preview-annotations.ts` around lines 11 - 25, Add a focused regression test for createPreviewAnnotations that verifies behavior when docs are enabled and disabled and confirms extraEntries ordering: call createPreviewAnnotations(packageName, extraEntries) to get the returned async preset function, provide a mocked options object whose presets.apply('docs', {}, options) returns either an empty object (docs disabled) or non-empty (docs enabled), await the preset function (passing an initial input array) and assert the resulting string array includes fileURLToPath(import.meta.resolve(`${packageName}/entry-preview`)) first, then the fileURLToPath(...) results for each extraEntries in order, and finally includes fileURLToPath(import.meta.resolve(`${packageName}/entry-preview-docs`)) only when docs are enabled; also include a test for empty extraEntries and for non-empty to catch ordering regressions.
18-23: Drop thestring[]cast oninput.
previewAnnotationsaccepts object-form entries as well as strings, so this cast narrows away valid inputs and makes future string-only assumptions easier to miss. Please double-check the inferred type here against theEntry[]preset contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/common/utils/preview-annotations.ts` around lines 18 - 23, Remove the "(input as string[])" cast in previewAnnotations so we don't narrow Entry[] to strings; instead concatenate the original input array directly (or start with ([] as Entry[]).concat(input)) and ensure the function signature accepts Entry[] per the preset contract. Keep the fileURLToPath + import.meta.resolve calls for package-based strings (extraEntries, entry-preview, entry-preview-docs) but do not assume the existing input entries are strings—preserve object-form entries when concatenating and only pass actual strings into fileURLToPath/resolve paths.
🤖 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/core/src/common/utils/preview-annotations.ts`:
- Around line 11-25: Add a focused regression test for createPreviewAnnotations
that verifies behavior when docs are enabled and disabled and confirms
extraEntries ordering: call createPreviewAnnotations(packageName, extraEntries)
to get the returned async preset function, provide a mocked options object whose
presets.apply('docs', {}, options) returns either an empty object (docs
disabled) or non-empty (docs enabled), await the preset function (passing an
initial input array) and assert the resulting string array includes
fileURLToPath(import.meta.resolve(`${packageName}/entry-preview`)) first, then
the fileURLToPath(...) results for each extraEntries in order, and finally
includes fileURLToPath(import.meta.resolve(`${packageName}/entry-preview-docs`))
only when docs are enabled; also include a test for empty extraEntries and for
non-empty to catch ordering regressions.
- Around line 18-23: Remove the "(input as string[])" cast in previewAnnotations
so we don't narrow Entry[] to strings; instead concatenate the original input
array directly (or start with ([] as Entry[]).concat(input)) and ensure the
function signature accepts Entry[] per the preset contract. Keep the
fileURLToPath + import.meta.resolve calls for package-based strings
(extraEntries, entry-preview, entry-preview-docs) but do not assume the existing
input entries are strings—preserve object-form entries when concatenating and
only pass actual strings into fileURLToPath/resolve paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82093879-03e5-46ec-a464-a927d3dfc318
📒 Files selected for processing (7)
code/core/src/common/index.tscode/core/src/common/utils/preview-annotations.tscode/renderers/html/src/preset.tscode/renderers/preact/src/preset.tscode/renderers/svelte/src/preset.tscode/renderers/vue3/src/preset.tscode/renderers/web-components/src/preset.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
valentinpalkovic
changed the title
valentinpalkovic
changed the title
|
Hi @mixelburg Thank you for your contribution! Please fix the formatting issues so that we can proceed with merging the PR |
valentinpalkovic
moved this from Empathy Queue (prioritized)
to On Hold
in Core Team Projects
valentinpalkovic
left a comment
valentinpalkovic
left a comment
There was a problem hiding this comment.
Unfortunately, this refactoring will cause issues in package managers with strict dependency resolution. Some package managers like pnpm will complain because they will try to e.g. resolve @storybook/react from the storybook package, which doesn't have @storybook/react defined as an entry point. Therefore, the import.meta.resolve calls has to remain in their renderer packages.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/common/utils/preview-annotations.ts`:
- Line 16: The expression computing docsEnabled calls Object.keys on the result
of presets.apply('docs', {}, options) which can be null/undefined; change the
expression that assigns docsEnabled so it treats the apply result as
(presets.apply('docs', {}, options) ?? {}) before calling Object.keys (i.e., use
nullish coalescing) to avoid TypeError when a preset returns a nullish value;
update the assignment where docsEnabled is defined to apply this coalescing.
🪄 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: f6fe8206-5de0-48f9-93c3-1d1f32d374c1
📒 Files selected for processing (1)
code/core/src/common/utils/preview-annotations.ts
| extraEntries: string[] = [] | ||
| ): PresetProperty<'previewAnnotations'> { | ||
| return async (input = [], options) => { | ||
| const docsEnabled = Object.keys(await options.presets.apply('docs', {}, options)).length > 0; |
There was a problem hiding this comment.
Add defensive nullish coalescing for presets.apply() result.
Per the codebase pattern shown in common-preset.ts and server-statics.ts, presets.apply() can return null/undefined if a preset's extension function returns a nullish value. Calling Object.keys() on null/undefined throws a TypeError.
Proposed fix
- const docsEnabled = Object.keys(await options.presets.apply('docs', {}, options)).length > 0;
+ const docsEnabled =
+ Object.keys((await options.presets.apply('docs', {}, options)) ?? {}).length > 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const docsEnabled = Object.keys(await options.presets.apply('docs', {}, options)).length > 0; | |
| const docsEnabled = | |
| Object.keys((await options.presets.apply('docs', {}, options)) ?? {}).length > 0; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/common/utils/preview-annotations.ts` at line 16, The expression
computing docsEnabled calls Object.keys on the result of presets.apply('docs',
{}, options) which can be null/undefined; change the expression that assigns
docsEnabled so it treats the apply result as (presets.apply('docs', {}, options)
?? {}) before calling Object.keys (i.e., use nullish coalescing) to avoid
TypeError when a preset returns a nullish value; update the assignment where
docsEnabled is defined to apply this coalescing.
2 participants
What
Extracts a
createPreviewAnnotations(packageName, extraEntries?)factory function tostorybook/internal/commonand uses it in all 5 renderer preset files.Closes #34389
Why
The
previewAnnotationsexport was copy-pasted identically across 5 files (html,preact,svelte,vue3,web-components), differing only in the package name string and — forweb-components— an extraentry-preview-argtypesannotation. This means any change to thedocsEnabledcheck or result-building logic requires updating 5 files, with real risk of inconsistencies.Changes
code/core/src/common/utils/preview-annotations.ts—createPreviewAnnotations(packageName, extraEntries?)factorycode/core/src/common/index.ts— re-exports the new factorycode/renderers/{html,preact,svelte,vue3,web-components}/src/preset.ts— replaced inlined function withcreatePreviewAnnotations()callThe
web-componentsrenderer passes['entry-preview-argtypes']as the second argument; all others use the default (no extra entries). Behaviour is identical to before.Summary by CodeRabbit