Skip to content

Fix potential raceCondition in SET_CONFIG hook#35401

Open
mrginglymus wants to merge 1 commit into
nextfrom
bill/racey-setstate
Open

Fix potential raceCondition in SET_CONFIG hook#35401
mrginglymus wants to merge 1 commit into
nextfrom
bill/racey-setstate

Conversation

@mrginglymus

@mrginglymus mrginglymus commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What I did

Found while sniffing for a related issue, this is a potential race condition in setState if filters are updated between the call to getState and the asynchronous setState

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

none required

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

  • Declare whether manual QA will be needed for this PR during the next release, through qa:needed or qa:skip

  • 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 how story filters are updated so the latest filter settings are preserved more reliably.
    • Fixed cases where filter changes could be applied out of order, helping ensure sidebar and managed filters behave consistently.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor
Fails
🚫 PR title must be in the format of "Area: Summary", With both Area and Summary starting with a capital letter Good examples: - "Docs: Describe Canvas Doc Block" - "Svelte: Support Svelte v4" Bad examples: - "add new api docs" - "fix: Svelte 4 support" - "Vue: improve docs"

Generated by 🚫 dangerJS against d20469e

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The SET_CONFIG handler in stories.ts now updates the filters state using a functional store.setState callback that reads current filters from the updater argument, replacing the prior approach of reading store.getState().filters directly. Merge precedence of filters remains unchanged.

Changes

SET_CONFIG Filters Update

Layer / File(s) Summary
Functional setState for filters merge
code/core/src/manager-api/modules/stories.ts
Filters update in the SET_CONFIG handler switches to a functional store.setState updater, spreading current filters from the callback argument, then merging configFilters and recomputed STATIC_FILTER, TAGS_FILTER, STATUS_FILTER entries with unchanged precedence.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Related Issues: Not specified in provided context.

Related PRs: Not specified in provided context.

Suggested labels: manager-api, bug

Suggested reviewers: Not specified in provided context.


🐰 A tiny tweak inside the state,
setState now reads what's fresh, not stale,
filters merge just the same as before,
just a subtler path through the store's front door.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
code/core/src/manager-api/modules/stories.ts (1)

1252-1272: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Read all dependent filter state inside the updater.

filters now comes from the functional updater, but includedTagFilters, excludedTagFilters, includedStatusFilters, excludedStatusFilters, and tagPresets are still captured from store.getState() before the async setState runs. If any of those change before the updater executes, STATIC_FILTER/TAGS_FILTER/STATUS_FILTER will still be built from stale values. Move those reads into the updater argument too.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/core/src/manager-api/modules/stories.ts` around lines 1252 - 1272, The
SET_CONFIG handler in stories.ts is still building STATIC_FILTER, TAGS_FILTER,
and STATUS_FILTER from values read via store.getState() before the functional
update runs, so those dependent filter values can go stale. Move the reads for
includedTagFilters, excludedTagFilters, includedStatusFilters,
excludedStatusFilters, and tagPresets inside the store.setState updater
alongside filters, and compute the managed filter functions from that updater
state so all filter values are derived from the same snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 1252-1272: The SET_CONFIG handler in stories.ts is still building
STATIC_FILTER, TAGS_FILTER, and STATUS_FILTER from values read via
store.getState() before the functional update runs, so those dependent filter
values can go stale. Move the reads for includedTagFilters, excludedTagFilters,
includedStatusFilters, excludedStatusFilters, and tagPresets inside the
store.setState updater alongside filters, and compute the managed filter
functions from that updater state so all filter values are derived from the same
snapshot.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 57d055fb-fee8-4404-a4d3-5d204f234d61

📥 Commits

Reviewing files that changed from the base of the PR and between 696e724 and d20469e.

📒 Files selected for processing (1)
  • code/core/src/manager-api/modules/stories.ts

@Sidnioulz Sidnioulz added bug ci:daily Run the CI jobs that normally run in the daily job. qa:skip Pull Requests that do not need any QA. labels Jul 7, 2026
@Sidnioulz Sidnioulz self-assigned this Jul 7, 2026
@Sidnioulz Sidnioulz moved this to In Progress in Core Team Projects Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:daily Run the CI jobs that normally run in the daily job. qa:skip Pull Requests that do not need any QA.

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants