Preview: Treat canceled animations as finished

[bawjensen]

Closes #32400

What I did

Add a try/catch around animation.finished so we can treat canceled promises as finished, just the same as we'd treat successfully completed animations.

Switch from all to allSettled so errored animations are treated as finished, just like successfully completed ones.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

I believe this is simple enough to not warrant manual testing (also didn't do automated tests, above), given the trickiness of setting up a repro. But I'm certainly lacking context on storybook's testing setup, so I could easily be wrong.

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

• Make 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/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

• Bug Fixes
  • Prevent animation cancellation errors from interrupting animation sequences — abort-related cancellations are treated as completed so they no longer surface as errors, while other errors still propagate.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[Sidnioulz]

Thanks for your first PR, @bawjensen! Happy to see new faces!

You might be able to test this by adding an end-to-end test in test-storybooks/portable-stories-kitchen-sink, that runs tests on a story with a cancelled animation. You likely have a use case in your own code that prompted you to fix this. This use case could be transformed into the story for E2E testing.

Feel free to come find me on Discord if you want a showcase of how to run common commands, and how to set up those end-to-end tests. Any feedback you have on the documentation around contributing is also more than welcome.

[Sidnioulz]

@jonniebigodes I'm not super familiar with animations and interactions tests but I'm not finding relevant doc with naive searches. Should we have a brief mention of how animations are waited for in the interaction tests doc?

[jonniebigodes]

@Sidnioulz more than glad to get this documented as well, we can sync up on it and if @bawjensen has any questions, here to help him out and get him unblocked.

[Sidnioulz]

@bawjensen would you like to try and add an end-to-end test? The process to run E2E tests should look like that, roughly:

yarn task compile
cd test-storybooks/portable-stories-kitchen-sink/react/
yarn
# ensure localhost:6006 is up
yarn storybook
# in another tab
yarn playwright-e2e --headed

You can run Playwright in VSCode to record a new test, see https://playwright.dev/docs/codegen

[bawjensen]

@Sidnioulz yeah my intent is to try to create that test, just slow going as this hasn't been my main priority and then I went on vacation. Can't promise when I'll find time for it - but I appreciate the guidance!

[Sidnioulz]

@Sidnioulz yeah my intent is to try to create that test, just slow going as this hasn't been my main priority and then I went on vacation. Can't promise when I'll find time for it - but I appreciate the guidance!

No worries! Contributions can spread over the course of a few months, we expect that and it's nothing to be ashamed of.

[ghengeveld]

I think Promise.all should just be replaced with Promise.allSettled, as that is what we are trying to achieve here anyway (wait for every animation to finish, not just the first one to throw). Alternatively, we could change a.finished to a.finished.catch(() => {}).

I don't think we need to re-throw in case of errors other than AbortError, it's fine to just catch any kind of error (realistically I don't expect anything other than AbortError).

[coderabbitai]

📝 Walkthrough

Walkthrough

waitForAnimations was changed to await each animation's finished promise individually, catching exceptions per-animation and treating AbortError as a non-fatal/ignored cancellation while rethrowing other errors.

Changes

Cohort / File(s)	Summary
Animation utility error handling code/core/src/preview-api/modules/preview-web/render/animation-utils.ts	waitForAnimations now awaits each animation's finished promise inside per-animation try/catch. AbortError exceptions are caught and ignored; other errors are rethrown.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Inspect waitForAnimations logic to ensure AbortError is correctly identified (e.g., name/type checks).
• Confirm no other error types are unintentionally suppressed.
• Verify behavior in environments with React.StrictMode (where AbortError was observed) and that caller expectations for cancellations remain valid.

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Sidnioulz]

I think Promise.all should just be replaced with Promise.allSettled, as that is what we are trying to achieve here anyway (wait for every animation to finish, not just the first one to throw). Alternatively, we could change a.finished to a.finished.catch(() => {}).

I don't think we need to re-throw in case of errors other than AbortError, it's fine to just catch any kind of error (realistically I don't expect anything other than AbortError).

I think it's ok as-is. Sure, the code would be simpler, but at the end of the day if there actually is another error, we'd be making it invisible by catching it and doing nothing with it. All we need is a test and we can merge this PR :)

[SimenB]

Any news on this one? Just stumbled across it, and it's quite annoying 😅

[Sidnioulz]

Any news on this one? Just stumbled across it, and it's quite annoying 😅

We're waiting on an update to E2E tests from @bawjensen. Feel free to fork their PR and write the test though :) It'll speed things up!

[Sidnioulz]

I've spent a considerable amount of time reproducing this issue in our React sandboxes. It turns out it only occurs with CRA, and only when React.StrictMode wraps the code being run.

We don't currently have a CI pipeline for CRA, and our CRA sandbox is in a delicate state. Rebuilding CI for CRA is way out of scope for this PR and likely not something we want to consume CI credits for anyway.

In parallel, a user has independently confirmed the fix works, and so have I. So, I'm comfortable with not adding non-regression tests and merging as-is to unblock our CRA users.

@bawjensen thanks again for digging into this and finding a solution!

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

code/core/src/preview-api/modules/preview-web/render/animation-utils.ts (1)

86-87: Clarify AbortError occurrence in comment.

When an animation's playState is anything but "idle" when cancelled, the finished promise is rejected with an AbortError. The current comment states this occurs "exclusively on CRA with React.StrictMode enabled," but this is standard Web Animations API behavior that can occur whenever animations are canceled, not limited to that environment. Consider revising to:

// Ignore AbortError from canceled animations, treat as "finished"
// Per Web Animations spec, AbortError occurs when Animation.cancel() is called on active animations

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 761e02e and e8c29b6.

📒 Files selected for processing (1)

• code/core/src/preview-api/modules/preview-web/render/animation-utils.ts

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use ESLint and Prettier configurations that are enforced in the codebase

Files:

• code/core/src/preview-api/modules/preview-web/render/animation-utils.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode

Files:

• code/core/src/preview-api/modules/preview-web/render/animation-utils.ts

code/**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

• code/core/src/preview-api/modules/preview-web/render/animation-utils.ts

code/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Export functions that need to be tested from their modules

Files:

• code/core/src/preview-api/modules/preview-web/render/animation-utils.ts

code/**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing

Files:

• code/core/src/preview-api/modules/preview-web/render/animation-utils.ts

🧠 Learnings (2)

📓 Common learnings

Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.512Z
Learning: When reviewing async functions that perform side effects like opening browsers, `.catch()` is preferred over `await` if the operation should be non-blocking. This prevents unhandled Promise rejections while keeping the operation as fire-and-forget.

📚 Learning: 2025-09-17T07:32:14.512Z

Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.512Z
Learning: When reviewing async functions that perform side effects like opening browsers, `.catch()` is preferred over `await` if the operation should be non-blocking. This prevents unhandled Promise rejections while keeping the operation as fire-and-forget.

Applied to files:

• code/core/src/preview-api/modules/preview-web/render/animation-utils.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: normal

🔇 Additional comments (2)

code/core/src/preview-api/modules/preview-web/render/animation-utils.ts (2)

81-93: Solid error handling for canceled animations.

The logic correctly catches AbortError from canceled animations and treats them as finished while preserving other errors. This approach ensures unhandled promise rejections are prevented without masking genuine animation failures.

---

81-93: E2E test required before merge.

The maintainers have requested an end-to-end test for this change before merging, as noted in the PR comments and the "needs tests" label. Given that this handles a timing-sensitive edge case with canceled animations, test coverage would significantly reduce regression risk.

As suggested by Sidnioulz, consider adding a test under test-storybooks/portable-stories-kitchen-sink that exercises a story with a canceled animation.

[bawjensen]

I've spent a considerable amount of time reproducing this issue in our React sandboxes. It turns out it only occurs with CRA, and only when React.StrictMode wraps the code being run.

[...]

@Sidnioulz fwiw my original repro is a quite complex web app unrelated to CRA. Our setup does have <StrictMode /> in storybook though

[Sidnioulz]

I've spent a considerable amount of time reproducing this issue in our React sandboxes. It turns out it only occurs with CRA, and only when React.StrictMode wraps the code being run.
[...]

@Sidnioulz fwiw my original repro is a quite complex web app unrelated to CRA. Our setup does have <StrictMode /> in storybook though

Were you using Webpack as well?

I tried NextJS with Webpack and Vite and could only repro on CRA. I haven't tried rspack though.

[nx-cloud]

View your CI Pipeline Execution ↗ for commit b3531d7

---

☁️ Nx Cloud last updated this comment at 2025-12-24 18:48:19 UTC

[bawjensen]

Were you using Webpack as well?

I tried NextJS with Webpack and Vite and could only repro on CRA. I haven't tried rspack though.

webpack, yeah. Not next.js, though

sadly I don't know that I can set up a minimal repro, it'd take a decent amount of effort to pare down our webpack setup to figure out what's causing it to happen when it didn't with your testing

[Sidnioulz]

Were you using Webpack as well?
I tried NextJS with Webpack and Vite and could only repro on CRA. I haven't tried rspack though.

webpack, yeah. Not next.js, though

sadly I don't know that I can set up a minimal repro, it'd take a decent amount of effort to pare down our webpack setup to figure out what's causing it to happen when it didn't with your testing

It's okay. The environment where we reliably see the bug is outside the scope of our CI anyway! Thanks again!

[ndelangen]

I agree that Promise.allSettled use would be a bit better, but I find the PR mergable as-is.

If @bawjensen (or someone else) wants to improve things further with the above suggestion, that would be very welcome.

[bawjensen]

I agree that Promise.allSettled use would be a bit better, but I find the PR mergable as-is.

If @bawjensen (or someone else) wants to improve things further with the above suggestion, that would be very welcome.

@ndelangen done ✅