Assignment/issue 21524#34677
Closed
beoziel wants to merge 7 commits into
Closed
Conversation
chore: adding the report for issue 34258
fix(core): categorize UniversalStore internal errors
Contributor
📝 WalkthroughWalkthroughThis change introduces dedicated error types for UniversalStore failures, replaces generic TypeErrors with structured error classes, enables asynchronous story entry computation in Storybook configuration, and updates type exports for the NextJS Vite plugin. Changes
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/lib/cli-storybook/src/util.ts (1)
802-813:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve
storiesbefore the empty-checkLine 802 checks
.lengthon the union before resolving function-based stories. Withstories: () => Promise<...>, this returns the function arity (typically0) and exits early, so no stories are discovered.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, + const normalizedStories = normalizeStories(resolvedStories, { + configDir, workingDir, -}); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/cli-storybook/src/util.ts` around lines 802 - 813, Resolve the potentially functional "stories" before checking its length: compute resolvedStories using the existing ternary (const resolvedStories = typeof stories === 'function' ? await stories() : stories) and then perform the empty-check against resolvedStories (e.g., if (!resolvedStories || resolvedStories.length === 0) return []); then pass resolvedStories into normalizeStories (normalizeStories(resolvedStories, { configDir, workingDir })), updating any references to use resolvedStories instead of the original stories variable.code/frameworks/nextjs-vite/src/index.ts (1)
8-21:⚠️ Potential issue | 🟠 MajorPublic plugin type degrades to
anydue to untypedrequire(); add explicit import extensionLine 21 derives
StorybookNextJsPluginfromstorybookNextJsPlugin, which is sourced from an untypedrequire('vite-plugin-storybook-nextjs')invite-plugin/index.ts. Without type annotations on the require result, this export has typeany, causing the public type alias to degrade as well. Additionally, line 8's import lacks an explicit.tsextension per repository guidelines.Fixes:
- Add an explicit type annotation to
storybookNextJsPlugininvite-plugin/index.ts, or importvite-plugin-storybook-nextjstypes if available- Change line 8 to
import type { storybookNextJsPlugin } from './vite-plugin.ts';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/frameworks/nextjs-vite/src/index.ts` around lines 8 - 21, The public type StorybookNextJsPlugin is degrading to any because storybookNextJsPlugin is imported from an untyped require in vite-plugin/index.ts and the current import lacks a .ts extension; update the import to use the explicit module extension (change import type { storybookNextJsPlugin } from './vite-plugin.ts') and add an explicit type annotation or exported declaration for storybookNextJsPlugin in vite-plugin/index.ts (or import/types from 'vite-plugin-storybook-nextjs' if available) so storybookNextJsPlugin has a concrete type and StorybookNextJsPlugin preserves that type.
🧹 Nitpick comments (3)
code/core/src/shared/universal-store/index.ts (1)
4-10: ⚡ Quick winUse an explicit
.tsextension in the new relative import.The newly added relative import should follow the repo TS import guideline.
Proposed patch
import { instances } from './instances'; import { UniversalStoreFollowerTimeoutError, UniversalStoreIdRequiredError, UniversalStoreMissingSubscribeArgumentError, UniversalStoreNotConstructableError, UniversalStoreNotReadyError, -} from './errors'; +} from './errors.ts';As per coding guidelines:
code/**/*.{ts,tsx,js,jsx}should prefer explicit file extensions for relative TS/JS imports/exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/shared/universal-store/index.ts` around lines 4 - 10, The import of errors in index.ts is missing the explicit TypeScript file extension; update the relative import from './errors' to './errors.ts' so the module import follows the repo guideline—locate the import that brings in UniversalStoreFollowerTimeoutError, UniversalStoreIdRequiredError, UniversalStoreMissingSubscribeArgumentError, UniversalStoreNotConstructableError, and UniversalStoreNotReadyError and change its path to include the .ts extension.code/core/src/shared/universal-store/errors.ts (1)
1-1: ⚡ Quick winUse explicit
.tsextension for the relative import.Please align this new import with the TS import convention used by the repo guidelines.
Proposed patch
-import { StorybookError } from '../../storybook-error'; +import { StorybookError } from '../../storybook-error.ts';As per coding guidelines:
code/**/*.{ts,tsx,js,jsx}should prefer explicit file extensions for relative TS/JS imports/exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/shared/universal-store/errors.ts` at line 1, The import for StorybookError should use an explicit .ts extension; update the import statement that currently references '../../storybook-error' to include the .ts file extension (i.e., '../../storybook-error.ts') so the module import in errors.ts follows the repository's TypeScript import convention.code/frameworks/nextjs-vite/src/index.ts (1)
8-8: ⚡ Quick winUse explicit extension for relative TS import
Line 8 should use an explicit TS/JS module path (for example
./vite-plugin/index.ts) to match repo import conventions.As per coding guidelines, “For TypeScript source in the repo, prefer explicit file extensions for relative code imports and exports such as
./foo.tsor./bar.tsxwhen the target is another TS/JS module”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/frameworks/nextjs-vite/src/index.ts` at line 8, Update the relative TypeScript import so it uses an explicit file extension: change the import of the type symbol storybookNextJsPlugin from './vite-plugin' to the module's explicit TS path (e.g., './vite-plugin/index.ts') to follow repo conventions for TypeScript imports; update the import statement in index.ts accordingly so tooling and bundlers resolve the module with the explicit extension.
🤖 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/shared/universal-store/errors.ts`:
- Around line 42-48: The error message in UniversalStoreNotReadyError
(constructor of class UniversalStoreNotReadyError extending UniversalStoreError)
references the outdated API store.readyPromise; update the message string to
reference the correct readiness API untilReady() instead (e.g., "await
store.untilReady()") so that the message guides users to use untilReady();
ensure you only change the message text in the constructor while keeping name,
code, and the rest of the template intact.
In `@issue-34566-report.md`:
- Line 175: Update the PR reference text on line 175 in issue-34566-report.md by
replacing the outdated "storybook!34597" token with the correct submission
identifier for this PR ("storybook!34677" or "#34677" as used elsewhere) so the
link and displayed PR number match the current submission.
In `@README.md`:
- Around line 1-17: The README currently exposes assignment-specific PII in the
"Group Members" table (student names and IDs); remove or anonymize the ID column
and any student identifiers from the root README and instead relocate full
member details to a non-public/internal metadata file or private storage; update
the "Group Members" section to use initials or first names only (or simply list
roles) and keep the Issues table intact; ensure references to the removed info
are not left in other top-level docs and commit the change with a clear message.
---
Outside diff comments:
In `@code/frameworks/nextjs-vite/src/index.ts`:
- Around line 8-21: The public type StorybookNextJsPlugin is degrading to any
because storybookNextJsPlugin is imported from an untyped require in
vite-plugin/index.ts and the current import lacks a .ts extension; update the
import to use the explicit module extension (change import type {
storybookNextJsPlugin } from './vite-plugin.ts') and add an explicit type
annotation or exported declaration for storybookNextJsPlugin in
vite-plugin/index.ts (or import/types from 'vite-plugin-storybook-nextjs' if
available) so storybookNextJsPlugin has a concrete type and
StorybookNextJsPlugin preserves that type.
In `@code/lib/cli-storybook/src/util.ts`:
- Around line 802-813: Resolve the potentially functional "stories" before
checking its length: compute resolvedStories using the existing ternary (const
resolvedStories = typeof stories === 'function' ? await stories() : stories) and
then perform the empty-check against resolvedStories (e.g., if (!resolvedStories
|| resolvedStories.length === 0) return []); then pass resolvedStories into
normalizeStories (normalizeStories(resolvedStories, { configDir, workingDir })),
updating any references to use resolvedStories instead of the original stories
variable.
---
Nitpick comments:
In `@code/core/src/shared/universal-store/errors.ts`:
- Line 1: The import for StorybookError should use an explicit .ts extension;
update the import statement that currently references '../../storybook-error' to
include the .ts file extension (i.e., '../../storybook-error.ts') so the module
import in errors.ts follows the repository's TypeScript import convention.
In `@code/core/src/shared/universal-store/index.ts`:
- Around line 4-10: The import of errors in index.ts is missing the explicit
TypeScript file extension; update the relative import from './errors' to
'./errors.ts' so the module import follows the repo guideline—locate the import
that brings in UniversalStoreFollowerTimeoutError,
UniversalStoreIdRequiredError, UniversalStoreMissingSubscribeArgumentError,
UniversalStoreNotConstructableError, and UniversalStoreNotReadyError and change
its path to include the .ts extension.
In `@code/frameworks/nextjs-vite/src/index.ts`:
- Line 8: Update the relative TypeScript import so it uses an explicit file
extension: change the import of the type symbol storybookNextJsPlugin from
'./vite-plugin' to the module's explicit TS path (e.g.,
'./vite-plugin/index.ts') to follow repo conventions for TypeScript imports;
update the import statement in index.ts accordingly so tooling and bundlers
resolve the module with the explicit extension.
🪄 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: ab76f71c-c4d3-4379-85c7-20699c4ae513
📒 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
Comment on lines
+42
to
+48
| export class UniversalStoreNotReadyError extends UniversalStoreError { | ||
| 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.`, | ||
| }); |
Contributor
There was a problem hiding this comment.
store.readyPromise in the error text is outdated; use untilReady().
The message currently points to a non-existent/publicly incorrect readiness API. UniversalStore exposes untilReady().
Proposed patch
export class UniversalStoreNotReadyError extends UniversalStoreError {
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.`,
+ 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 the current code and only fix it if needed.
In `@code/core/src/shared/universal-store/errors.ts` around lines 42 - 48, The
error message in UniversalStoreNotReadyError (constructor of class
UniversalStoreNotReadyError extending UniversalStoreError) references the
outdated API store.readyPromise; update the message string to reference the
correct readiness API untilReady() instead (e.g., "await store.untilReady()") so
that the message guides users to use untilReady(); ensure you only change the
message text in the constructor while keeping name, code, and the rest of the
template intact.
|
|
||
| ## Submit the Fix | ||
|
|
||
| This Pull Request [storybook!34597](https://github.com/storybookjs/storybook/pull/34597) was submitted to Storybook project's Github. |
Contributor
There was a problem hiding this comment.
Update the PR link to match this submission.
Line 175 points to storybook!34597, but this submission is PR #34677. Keeping this aligned avoids stale traceability in the report.
Proposed patch
-This Pull Request [storybook!34597](https://github.com/storybookjs/storybook/pull/34597) was submitted to Storybook project's Github.
+This Pull Request [storybook!34677](https://github.com/storybookjs/storybook/pull/34677) was submitted to Storybook project's GitHub.📝 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.
Suggested change
| This Pull Request [storybook!34597](https://github.com/storybookjs/storybook/pull/34597) was submitted to Storybook project's Github. | |
| This Pull Request [storybook!34677](https://github.com/storybookjs/storybook/pull/34677) was submitted to Storybook project's GitHub. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@issue-34566-report.md` at line 175, Update the PR reference text on line 175
in issue-34566-report.md by replacing the outdated "storybook!34597" token with
the correct submission identifier for this PR ("storybook!34677" or "#34677" as
used elsewhere) so the link and displayed PR number match the current
submission.
Comment on lines
+1
to
+17
| ## CES Info | ||
|
|
||
| ### Group Members | ||
|
|
||
| | Student | ID | | ||
| | --------------------- | --------- | | ||
| | Beatriz Oziel de Lima | 202510621 | | ||
| | Rubens Brock Silva | 202510624 | | ||
| | Matvii Suk | 202514197 | | ||
|
|
||
| ### Issues | ||
|
|
||
| | Issue | Resources | | ||
| | --------------------------------------------------------------- | ----------------------------------- | | ||
| | [#34258](https://github.com/storybookjs/storybook/issues/34258) | [Report doc](issue-34258-report.md) | | ||
| | [#34566](https://github.com/storybookjs/storybook/issues/34566) | [Report doc](issue-34566-report.md) | | ||
|
|
Contributor
There was a problem hiding this comment.
Remove assignment-specific PII from the root README
Line 5–9 publishes personal student identifiers in a project-wide document. This should not live in the public repository README; keep assignment metadata outside the product docs.
Suggested change
-## CES Info
-
-### Group Members
-
-| Student | ID |
-| --------------------- | --------- |
-| Beatriz Oziel de Lima | 202510621 |
-| Rubens Brock Silva | 202510624 |
-| Matvii Suk | 202514197 |
-
-### Issues
-
-| Issue | Resources |
-| --------------------------------------------------------------- | ----------------------------------- |
-| [`#34258`](https://github.com/storybookjs/storybook/issues/34258) | [Report doc](issue-34258-report.md) |
-| [`#34566`](https://github.com/storybookjs/storybook/issues/34566) | [Report doc](issue-34566-report.md) |
-
-## Rest of the Original README
-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 1 - 17, The README currently exposes
assignment-specific PII in the "Group Members" table (student names and IDs);
remove or anonymize the ID column and any student identifiers from the root
README and instead relocate full member details to a non-public/internal
metadata file or private storage; update the "Group Members" section to use
initials or first names only (or simply list roles) and keep the Issues table
intact; ensure references to the removed info are not left in other top-level
docs and commit the change with a clear message.
Contributor
|
Closing, since the PR doesn't contain a proper title or description. |
github-project-automation
Bot
moved this from Empathy Queue (prioritized)
to Done
in Core Team Projects
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
The stories field in .storybook/main.ts accepts 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 of StoriesEntry didn't allow them.
stories: async (list) => {
const extraStories = await findStories();
return [...list, ...extraStories];
}
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.
Keep existing synchronous stories implementations working.
Allow async resolvers that return promises.
Not break any existing consumers of StoriesEntry.
Leave runtime behavior exactly as is.
Validate that the async usage actually compiles.
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
Bug Fixes
Documentation