fix(core): categorize UniversalStore internal errors#34597
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a UniversalStore-specific error hierarchy and replaces previous generic TypeError throws with the new error classes; the errors are defined in a new module and re-exported via the manager-errors enum/category. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/manager-errors.ts`:
- Around line 51-57: The re-export of UniversalStoreFollowerTimeoutError,
UniversalStoreIdRequiredError, UniversalStoreMissingSubscribeArgumentError,
UniversalStoreNotConstructableError, and UniversalStoreNotReadyError currently
imports from a relative module without an explicit extension; update the export
source string './shared/universal-store/errors' to include the explicit
TypeScript extension (i.e., './shared/universal-store/errors.ts') so the
re-export uses the project's explicit-extension convention.
In `@code/core/src/shared/universal-store/errors.ts`:
- Line 1: The import of StorybookError should use an explicit .ts extension;
update the import statement that references StorybookError (symbol:
StorybookError) in errors.ts to import from './storybook-error.ts' instead of
'./storybook-error' so the relative TypeScript module import includes the
explicit file extension.
- Around line 42-48: The error message in UniversalStoreNotReadyError
(constructor in class UniversalStoreNotReadyError) references a nonexistent
readyPromise; update the message string to reference the correct readiness API
untilReady() and adjust any inline snapshot tests that contain the old message
so they assert the new text; ensure the message still includes the store id,
action, and guidance like "You can get the current status with store.status, or
await store.untilReady() to wait for the store to be ready before sending
events."
🪄 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: 049ec030-44ca-48dd-b3d9-bb2950132f1e
📒 Files selected for processing (4)
code/core/src/manager-errors.tscode/core/src/shared/universal-store/errors.tscode/core/src/shared/universal-store/index.test.tscode/core/src/shared/universal-store/index.ts
|
Superseded by #34592 |
Closes #34566
What I did
Categorized internal
UniversalStorefailures by replacing genericTypeErroroccurrences with a granularUniversalStoreErrorhierarchy.Currently, when a
UniversalStorefollower times out or is misused, it throws a plainTypeError. These are caught byprepareForTelemetry.tsand bucketed generically asSB_MANAGER_UNCAUGHT_0001, making it impossible to distinguish store-specific failures from other uncaught manager errors in telemetry.Specifically, I:
UniversalStoreErrorand 5 specialized subclasses in theshared/universal-storelayer.UniversalStoreto throw these specific errors with codes1and1001through1004(Timeout, NotConstructable, IdRequired, NotReady, MissingSubscribeArgument).MANAGER_UNIVERSAL-STOREcategory inmanager-errors.tsand re-exported the errors to completely avoid circular dependencies between thesharedandmanagerbundles.
index.test.ts.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!
No manual testing is necessary.
This PR fixes a telemetry categorization bug where internal
UniversalStorefailures were being generically reported as uncaught manager errors. It does not alter any user-facing features, UI components, or public APIs.Because manually reproducing these specific store initialization and timeout failures in a browser requires intentionally breaking core configurations, manual testing provides no practical diagnostic value. Instead, the updated
index.test.tssuite exhaustively simulates these edge cases and verifies that the new error classes are thrown with the correct category, code, andfromStorybookflag required by the telemetry handler.Documentation
MIGRATION.MD
(Note: No public documentation changes were required as this is an internal telemetry/error architecture improvement).
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 sandboxescan be found in
code/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