Skip to content

Conversation

@Sidnioulz
Copy link
Member

@Sidnioulz Sidnioulz commented Dec 17, 2025

Closes #32834

What I did

Forced a reset of lastRenderId when switching stories, to ensure we account for the fact that Date.now() can be mocked in a story.

Checklist for Contributors

Testing

Manual testing

  1. Build a canary for this PR
  2. Install it on https://github.com/Sidnioulz/storybook-play-function-error-state-persistence-repro
  3. Launch Storybook
  4. Go to http://localhost:6006/?path=/story/example-frog--late which triggers a caught exception state
  5. Navigate to http://localhost:6006/?path=/story/example-frog--on-time-too and see the state is cleared

Without the canary, the state will not be properly cleared.

Documentation

ø

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 publish.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

  • Bug Fixes
    • Improved render tracking so component testing more reliably handles switching between stories, preventing stale or concurrent render updates.
    • Added per-story update logic and an early bailout to ensure only the relevant render advances, reducing incorrect state transitions during rapid story changes or mocked-time scenarios.

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

@Sidnioulz Sidnioulz requested a review from ghengeveld December 17, 2025 16:59
@Sidnioulz Sidnioulz added bug addon: interactions ci:normal needs qa Indicates that this needs manual QA during the upcoming minor/major release labels Dec 17, 2025
@nx-cloud
Copy link

nx-cloud bot commented Dec 17, 2025

View your CI Pipeline Execution ↗ for commit aef650d

Command Status Duration Result
nx run-many -t compile,check,knip,test,pretty-d... ✅ Succeeded 8m 23s View ↗

☁️ Nx Cloud last updated this comment at 2025-12-19 10:12:30 UTC

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 17, 2025

📝 Walkthrough

Walkthrough

Reworks the STORY_RENDER_PHASE_CHANGED handler in Panel.tsx: renames lastRenderId to latestRenderId, tracks per-story render IDs (reset latestRenderId on story switch, max it for same-story arrivals), updates lastStoryId, and early-bails if latestRenderId does not equal the incoming event.renderId before proceeding with phase/state updates.

Changes

Cohort / File(s) Change Summary
STORY_RENDER_PHASE_CHANGED handler / Panel component
code/core/src/component-testing/components/Panel.tsx
Rename lastRenderIdlatestRenderId; introduce lastStoryId tracking; on event arrival: if storyId changed, set latestRenderId = event.renderId and update lastStoryId; if same story, set latestRenderId = max(latestRenderId, event.renderId); early return if latestRenderId !== event.renderId; preserve existing phase/state transitions gated by this logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Inspect Panel.tsx handler for correct rename and state updates (latestRenderId, lastStoryId).
  • Verify the early-bail condition doesn't drop valid out-of-order events unintentionally.
  • Check existing tests or add tests for same-story vs story-switch event sequences and mocked time scenarios.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb3fbb1 and aef650d.

📒 Files selected for processing (1)
  • code/core/src/component-testing/components/Panel.tsx (2 hunks)
🧰 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/component-testing/components/Panel.tsx
**/*.{ts,tsx}

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

Enable TypeScript strict mode

Files:

  • code/core/src/component-testing/components/Panel.tsx
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/component-testing/components/Panel.tsx
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/component-testing/components/Panel.tsx
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/component-testing/components/Panel.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/component-testing/components/Panel.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/core/src/component-testing/components/Panel.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/component-testing/components/Panel.tsx
⏰ 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). (3)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
🔇 Additional comments (2)
code/core/src/component-testing/components/Panel.tsx (2)

253-253: LGTM! Clear and appropriate rename.

Renaming lastRenderId to latestRenderId improves clarity and distinguishes it from lastStoryId.


276-289: LGTM! Correctly fixes the state-reset bug.

The logic correctly handles story switches and concurrent renders:

  1. Same story: latestRenderId tracks the highest render ID via Math.max, ensuring only the latest render updates state.
  2. Story switch: Resets latestRenderId and updates lastStoryId, which handles cases where Date.now() is mocked and render IDs might decrease.
  3. Bailout: Prevents stale or concurrent renders from updating state by returning early if latestRenderId !== event.renderId.

This fixes the reported bug where play-function errors persisted after navigating to new stories.


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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c60f2d8 and bb3fbb1.

📒 Files selected for processing (1)
  • code/core/src/component-testing/components/Panel.tsx (1 hunks)
🧰 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/component-testing/components/Panel.tsx
**/*.{ts,tsx}

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

Enable TypeScript strict mode

Files:

  • code/core/src/component-testing/components/Panel.tsx
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/component-testing/components/Panel.tsx
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/component-testing/components/Panel.tsx
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/component-testing/components/Panel.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/component-testing/components/Panel.tsx
⏰ 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). (5)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: nx
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest

if (lastStoryId.current === event.storyId) {
lastRenderId.current = Math.max(lastRenderId.current, event.renderId || 0);
} else {
lastRenderId.current = event.renderId || 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering whether this should always be 0 rather than event.renderId, or would that break things? Not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't initialise it to the id for the current event, then we'll hit the code below that bails out when multiple concurrent renders exist.

@ghengeveld
Copy link
Member

@Sidnioulz is this a sufficient fix or do we still need to revisit the use of dates for this?

@Sidnioulz
Copy link
Member Author

@Sidnioulz is this a sufficient fix or do we still need to revisit the use of dates for this?

It seems sufficient. I think we can leave it at that, and if we see more issues / next time we build an ID system, we can use a global sequential counter instead.

@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-32834 branch from d5be5e7 to aef650d Compare December 19, 2025 10:02
@ghengeveld ghengeveld merged commit b6b9160 into next Dec 22, 2025
69 checks passed
@ghengeveld ghengeveld deleted the sidnioulz/issue-32834 branch December 22, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addon: interactions bug ci:normal needs qa Indicates that this needs manual QA during the upcoming minor/major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: SB10 Play function error state persists for a bit after visiting new stories

3 participants