-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Core: Prevent BAIL state from showing in interactions panel when switching stories
#32172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…hen switching stories, in order to prevent BAIL from briefly appearing
…d (terminal phases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, no comments
|
View your CI Pipeline Execution ↗ for commit 72b63e9
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 41.36 MB | 41.37 MB | 🚨 +16 KB 🚨 |
| Dependency size | 18.13 MB | 18.13 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
sb
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 52 | 52 | 0 |
| Self size | 1 KB | 1 KB | 0 B |
| Dependency size | 59.49 MB | 59.51 MB | 🚨 +16 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 220 | 220 | 0 |
| Self size | 585 KB | 585 KB | 0 B |
| Dependency size | 103.95 MB | 103.97 MB | 🚨 +16 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 189 | 189 | 0 |
| Self size | 31 KB | 31 KB | 0 B |
| Dependency size | 88.04 MB | 88.06 MB | 🚨 +16 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
|
@ghengeveld please fill in the manual testing section for us to do QA on this PR |
Closes #
What I did
Originally fixed for #32163 but regressed when another (more severe) issue was fixed. So this is the 2nd attempt at fixing the problem, in a way that avoids reintroducing the more severe problem.
The source of these bugs is overlapping
STORY_RENDER_PHASE_CHANGEDevents originating from different render cycles and/or stories. The two issues are:preparingphase for render cycles that never actually render, causing the logs to stay empty. This is the most severe issue and was resolved by ignoring thepreparingandloadingphases and only clear the log on therenderingphase.BAILwhen switching stories. This happens when the previous story's render cycle is aborted. The render cycle for the new story would only kick in at therenderingphase, and so ifabortedarrives before rendering starts, you'd see theBAILlabel on the new story even though that update was intended for the previous story.That's why in this PR, I have extended the condition to ignore
preparing/loadingonly when we're not switching stories. If we are switching stories, thepreparingphase will updatelastRenderId, thereby causing theabortedupdate to be ignored (as it uses an olderrenderId).Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Switch between two stories which have (long-running) play functions. In the interactions panel, there should not be a purple
BAILlabel showing when switching stories. When you HMR while the play function is running, theBAILlabel should show.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 pull request has been released as version
0.0.0-pr-32172-sha-72b63e9f. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-32172-sha-72b63e9freset-on-change-story72b63e9f1754046408)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=32172Greptile Summary
This PR fixes a race condition issue that causes the interactions panel to incorrectly display a "BAIL" status when switching between stories in Storybook. The root cause stems from overlapping
STORY_RENDER_PHASE_CHANGEDevents from different render cycles, where an aborted render cycle from the previous story can interfere with the new story's render phases.The solution involves two key changes to the event handling logic:
Smart phase filtering in Panel.tsx: Instead of blindly ignoring
preparingandloadingphases (which caused the BAIL issue), the code now tracks the current story ID using alastStoryIdref. It only ignores early phases when re-rendering the same story, allowing story switches to proceed normally while still protecting against premature log clearing during rerenders.Terminal state protection in StoryRender.ts: The
checkIfAbortedmethod now prevents emittingSTORY_RENDER_PHASE_CHANGEDevents with 'aborted' phase when the render is already in a terminal state (['finished', 'aborted', 'errored']). This prevents stale abort events from previous render cycles from affecting new stories.The changes integrate well with Storybook's existing render phase management system, building on the existing
STORY_RENDER_PHASE_CHANGEDevent infrastructure while adding safeguards against race conditions. Additionally, the PR includes a minor test refactoring inSaveStory.stories.tsxthat switches fromuserEventtofireEventfor simpler input testing.Confidence score: 4/5
• This PR addresses a well-documented race condition with a targeted solution that maintains existing functionality while fixing the specific BAIL state issue
• The logic appears sound but deals with complex async event timing that could have edge cases not immediately apparent
• The StoryRender.ts and Panel.tsx files need careful review as they handle critical render phase state management