Assignment/issue 21524#34899
Conversation
chore: adding the report for issue 34258
fix(core): categorize UniversalStore internal errors
|
📝 WalkthroughWalkthroughThis PR implements UniversalStore error categorization for telemetry tracking and extends configuration support for async story entry resolution. It adds a dedicated error hierarchy in the universal-store module, integrates those errors into the manager error taxonomy, updates UniversalStore to throw them instead of generic TypeErrors, and broadens the stories configuration type to accept async functions. ChangesUniversalStore Error Categorization
Async Stories Configuration Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches⚔️ Resolve merge conflicts
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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/lib/cli-storybook/src/util.ts (1)
802-813:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the
stories.lengthempty-check to after resolvingstories
code/lib/cli-storybook/src/util.tscurrently returns[]based onstories.lengthbefore thetypeof stories === 'function' ? await stories() : storiesresolution runs; whenstoriesis a function,stories.lengthis the function arity, so a zero-arg resolver gets skipped.Suggested fix
- if (stories.length === 0) { - return []; - } - -const resolvedStories = typeof stories === 'function' - ? await stories() - : stories; +const resolvedStories = typeof stories === 'function' + ? await stories() + : stories; + + if (resolvedStories.length === 0) { + return []; + } const normalizedStories = normalizeStories(resolvedStories, { configDir, workingDir, });🤖 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/util.ts` around lines 802 - 813, The empty-check currently uses stories.length before resolving when stories may be a resolver function; move the guard to after resolving (i.e., after computing resolvedStories via const resolvedStories = typeof stories === 'function' ? await stories() : stories) and check resolvedStories.length instead; then return [] if resolvedStories.length === 0 and proceed to call normalizeStories(resolvedStories, { configDir, workingDir }) so normalizeStories always receives the actual resolved array.
🧹 Nitpick comments (3)
code/core/src/shared/universal-store/index.ts (1)
4-10: ⚡ Quick winUse explicit
.tsextension for the new relative import.Please update this import path to follow repository conventions.
Proposed change
} from './errors'; +} from './errors.ts';As per coding guidelines,
code/**/*.{ts,tsx}: "Prefer explicit file extensions for relative code imports and exports such as ./foo.ts or ./bar.tsx when the target is another TS/JS module in this repository; keep framework-specific component imports like .vue and .svelte in their expected form".🤖 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/shared/universal-store/index.ts` around lines 4 - 10, The import of error symbols (UniversalStoreFollowerTimeoutError, UniversalStoreIdRequiredError, UniversalStoreMissingSubscribeArgumentError, UniversalStoreNotConstructableError, UniversalStoreNotReadyError) is using a bare relative path; update the import source from './errors' to './errors.ts' to follow the repository convention of explicit .ts extensions for relative TypeScript module imports, keeping the imported symbol list unchanged in the import statement in index.ts.code/core/src/shared/universal-store/errors.ts (1)
1-1: ⚡ Quick winUse explicit
.tsextension for this relative import.Please align this import with the repository rule for explicit relative module extensions.
Proposed change
-import { StorybookError } from '../../storybook-error'; +import { StorybookError } from '../../storybook-error.ts';As per coding guidelines,
code/**/*.{ts,tsx}: "Prefer explicit file extensions for relative code imports and exports such as ./foo.ts or ./bar.tsx when the target is another TS/JS module in this repository; keep framework-specific component imports like .vue and .svelte in their expected form".🤖 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/shared/universal-store/errors.ts` at line 1, Update the relative import for StorybookError in errors.ts to use an explicit .ts extension; locate the import statement "import { StorybookError } from '../../storybook-error';" and change it to reference '../../storybook-error.ts' so it follows the repository rule for explicit relative TypeScript module extensions.code/core/src/manager-errors.ts (1)
51-57: ⚡ Quick winUse explicit
.tsextension in this relative re-export path.Please make this re-export path explicit to match repository TS import/export conventions.
Proposed change
} from './shared/universal-store/errors'; +} from './shared/universal-store/errors.ts';As per coding guidelines,
code/**/*.{ts,tsx}: "Prefer explicit file extensions for relative code imports and exports such as ./foo.ts or ./bar.tsx when the target is another TS/JS module in this repository; keep framework-specific component imports like .vue and .svelte in their expected form".🤖 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/manager-errors.ts` around lines 51 - 57, The re-export currently omits the TypeScript file extension; update the export statement that re-exports UniversalStoreFollowerTimeoutError, UniversalStoreIdRequiredError, UniversalStoreMissingSubscribeArgumentError, UniversalStoreNotConstructableError, and UniversalStoreNotReadyError so that the module path includes the explicit .ts extension (replace './shared/universal-store/errors' with './shared/universal-store/errors.ts') and keep the exported symbol list 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/core/src/shared/universal-store/errors.ts`:
- Around line 43-48: The error message in UniversalStoreNotReadyError's
constructor references a non-existent API (readyPromise); update the message to
point users to the correct readiness API (untilReady). Modify the message string
in the UniversalStoreNotReadyError constructor so it suggests checking
store.status or awaiting store.untilReady() (e.g., "You can get the current
status with store.status, or await store.untilReady() to wait for the store to
be ready before sending events."), leaving the rest of the error object (name,
code, data) unchanged.
In `@code/frameworks/nextjs-vite/src/index.ts`:
- Line 8: The import of storybookNextJsPlugin in index.ts uses a relative path
without an explicit extension; update the import statement for
storybookNextJsPlugin to reference the TypeScript file with its .ts extension
(e.g., './vite-plugin.ts') so the module resolver and coding guidelines are
satisfied and the symbol storybookNextJsPlugin is imported from the explicit
file.
---
Outside diff comments:
In `@code/lib/cli-storybook/src/util.ts`:
- Around line 802-813: The empty-check currently uses stories.length before
resolving when stories may be a resolver function; move the guard to after
resolving (i.e., after computing resolvedStories via const resolvedStories =
typeof stories === 'function' ? await stories() : stories) and check
resolvedStories.length instead; then return [] if resolvedStories.length === 0
and proceed to call normalizeStories(resolvedStories, { configDir, workingDir })
so normalizeStories always receives the actual resolved array.
---
Nitpick comments:
In `@code/core/src/manager-errors.ts`:
- Around line 51-57: The re-export currently omits the TypeScript file
extension; update the export statement that re-exports
UniversalStoreFollowerTimeoutError, UniversalStoreIdRequiredError,
UniversalStoreMissingSubscribeArgumentError,
UniversalStoreNotConstructableError, and UniversalStoreNotReadyError so that the
module path includes the explicit .ts extension (replace
'./shared/universal-store/errors' with './shared/universal-store/errors.ts') and
keep the exported symbol list unchanged.
In `@code/core/src/shared/universal-store/errors.ts`:
- Line 1: Update the relative import for StorybookError in errors.ts to use an
explicit .ts extension; locate the import statement "import { StorybookError }
from '../../storybook-error';" and change it to reference
'../../storybook-error.ts' so it follows the repository rule for explicit
relative TypeScript module extensions.
In `@code/core/src/shared/universal-store/index.ts`:
- Around line 4-10: The import of error symbols
(UniversalStoreFollowerTimeoutError, UniversalStoreIdRequiredError,
UniversalStoreMissingSubscribeArgumentError,
UniversalStoreNotConstructableError, UniversalStoreNotReadyError) is using a
bare relative path; update the import source from './errors' to './errors.ts' to
follow the repository convention of explicit .ts extensions for relative
TypeScript module imports, keeping the imported symbol list unchanged in the
import statement in index.ts.
🪄 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: aebb4891-af73-4ef7-a30a-65d79dca9b28
📒 Files selected for processing (12)
README.mdcode/core/src/core-server/presets/common-override-preset.tscode/core/src/manager-errors.tscode/core/src/manager/globals/exports.tscode/core/src/shared/universal-store/errors.tscode/core/src/shared/universal-store/index.test.tscode/core/src/shared/universal-store/index.tscode/core/src/types/modules/core-common.tscode/frameworks/nextjs-vite/src/index.tscode/lib/cli-storybook/src/util.tsissue-34258-report.mdissue-34566-report.md
| constructor(public data: { id: string; action: 'set state' | 'send event' }) { | ||
| super({ | ||
| name: 'UniversalStoreNotReadyError', | ||
| code: 1003, | ||
| message: `Cannot ${data.action} before store with id '${data.id}' is ready. You can get the current status with store.status, or await store.readyPromise to wait for the store to be ready before sending events.`, | ||
| }); |
There was a problem hiding this comment.
Error message points to a non-existent readiness API.
UniversalStore exposes untilReady(), but this message tells users to await store.readyPromise, which is misleading and not available.
Proposed change
- message: `Cannot ${data.action} before store with id '${data.id}' is ready. You can get the current status with store.status, or await store.readyPromise to wait for the store to be ready before sending events.`,
+ message: `Cannot ${data.action} before store with id '${data.id}' is ready. You can get the current status with store.status, or await store.untilReady() to wait for the store to be ready before sending events.`,📝 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.
| constructor(public data: { id: string; action: 'set state' | 'send event' }) { | |
| super({ | |
| name: 'UniversalStoreNotReadyError', | |
| code: 1003, | |
| message: `Cannot ${data.action} before store with id '${data.id}' is ready. You can get the current status with store.status, or await store.readyPromise to wait for the store to be ready before sending events.`, | |
| }); | |
| constructor(public data: { id: string; action: 'set state' | 'send event' }) { | |
| super({ | |
| name: 'UniversalStoreNotReadyError', | |
| code: 1003, | |
| message: `Cannot ${data.action} before store with id '${data.id}' is ready. You can get the current status with store.status, or await store.untilReady() to wait for the store to be ready before sending events.`, | |
| }); |
🤖 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/shared/universal-store/errors.ts` around lines 43 - 48, The
error message in UniversalStoreNotReadyError's constructor references a
non-existent API (readyPromise); update the message to point users to the
correct readiness API (untilReady). Modify the message string in the
UniversalStoreNotReadyError constructor so it suggests checking store.status or
awaiting store.untilReady() (e.g., "You can get the current status with
store.status, or await store.untilReady() to wait for the store to be ready
before sending events."), leaving the rest of the error object (name, code,
data) unchanged.
| import type { ReactTypes } from '@storybook/react'; | ||
|
|
||
| import type vitePluginStorybookNextJs from 'vite-plugin-storybook-nextjs'; | ||
| import type { storybookNextJsPlugin } from './vite-plugin'; |
There was a problem hiding this comment.
Add explicit file extension to relative import.
The import path should include an explicit .ts extension. As per coding guidelines, relative code imports should use explicit file extensions like ./foo.ts or ./bar.tsx.
📝 Proposed fix
-import type { storybookNextJsPlugin } from './vite-plugin';
+import type { storybookNextJsPlugin } from './vite-plugin/index.ts';🤖 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/nextjs-vite/src/index.ts` at line 8, The import of
storybookNextJsPlugin in index.ts uses a relative path without an explicit
extension; update the import statement for storybookNextJsPlugin to reference
the TypeScript file with its .ts extension (e.g., './vite-plugin.ts') so the
module resolver and coding guidelines are satisfied and the symbol
storybookNextJsPlugin is imported from the explicit file.
3 participants
Closes #
What I did
The
storiesfield in.storybook/main.tsaccepts custom resolver functions for advanced story discovery. When this issue was filed, Storybook already ran async resolvers correctly at runtime — the problem was that the TypeScript definition ofStoriesEntrydidn't allow them.So the type system and the actual runtime were out of sync: async functions worked fine when executed, but TypeScript flagged them as errors. Users had to work around it with type assertions or just ignore the compiler noise.
This is a type system/API consistency bug, not a runtime issue.
code/core/src/types/modules/core-common.tsThis file contains Storybook's shared configuration type definitions, including the
StoriesEntrytype used by.storybook/main.ts.Changes made:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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.tsMake 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
Release Notes
New Features
Documentation
Improvements