Manager-API: Update refs sequentially in experimental_setFilter#33958
Conversation
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager-api/modules/stories.ts (1)
708-710: Apply the same sequential ref-update strategy in the status-change reapply path.This loop is correctly serialized, but the status-change path still does concurrent
forEachref updates (Line 933-Line 935). That can reintroduce last-write-wins behavior when filters are reapplied after status changes. Consider extracting a shared awaited helper and reusing it in both places.Proposed refactor
+ const updateRefsWithCurrentFilters = async () => { + const refs = await fullAPI.getRefs(); + for (const [refId, { internal_index, ...ref }] of Object.entries(refs)) { + await fullAPI.setRef(refId, { ...ref, storyIndex: internal_index }, true); + } + }; ... - const refs = await fullAPI.getRefs(); - for (const [refId, { internal_index, ...ref }] of Object.entries(refs)) { - await fullAPI.setRef(refId, { ...ref, storyIndex: internal_index }, true); - } + await updateRefsWithCurrentFilters(); ... - const refs = await fullAPI.getRefs(); - Object.entries(refs).forEach(([refId, { internal_index, ...ref }]) => { - fullAPI.setRef(refId, { ...ref, storyIndex: internal_index }, true); - }); + await updateRefsWithCurrentFilters();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/stories.ts` around lines 708 - 710, The status-change reapply path currently updates refs concurrently (forEach at the reapply block) which can cause last-write-wins; extract a small awaited helper (e.g., setRefsSequentially or applyRefsInOrder) that takes the refs object/entries and calls fullAPI.setRef(refId, { ...ref, storyIndex: internal_index }, true) in a for..of loop (as used in the serialized loop that iterates Object.entries(refs)), then replace the concurrent forEach in the status-change reapply path with a call to that helper so both code paths update refs sequentially and await each setRef.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 708-710: The status-change reapply path currently updates refs
concurrently (forEach at the reapply block) which can cause last-write-wins;
extract a small awaited helper (e.g., setRefsSequentially or applyRefsInOrder)
that takes the refs object/entries and calls fullAPI.setRef(refId, { ...ref,
storyIndex: internal_index }, true) in a for..of loop (as used in the serialized
loop that iterates Object.entries(refs)), then replace the concurrent forEach in
the status-change reapply path with a call to that helper so both code paths
update refs sequentially and await each setRef.
Manager-API: Update refs sequentially in experimental_setFilter (cherry picked from commit c4d647b)
Closes #33800
What I did
Change the concurrent
forEachupdates overrefsinexperimental_setFilterto a serialawaitloop to avoid later writes overwriting earlier onesChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
code/.storybook/main.ts, copy threeiconsentries intorefs(pick ones that have docs so they’re easy to filter)code, runyarn storybook:uidocsiconsgroups — they should all contain docs onlyDocumentation
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 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