Sidebar: Prevent updating non-existent stories #33037
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/manager-api/modules/stories.ts (1)
602-602: Update or remove the now-addressed FIXME comment.This FIXME describes the exact issue being fixed in this PR. Consider updating it to document the fix or removing it entirely.
-// FIXME: is there a bug where filtered stories get added back in on updateStory??? +// updateStory only updates entries that already exist to prevent corruption of filtered indexes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager-api/modules/stories.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/manager-api/modules/stories.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/manager-api/modules/stories.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/manager-api/modules/stories.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager-api/modules/stories.ts
🧠 Learnings (3)
📓 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/manager-api/modules/stories.ts
📚 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: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager-api/modules/stories.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). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/manager-api/modules/stories.ts (1)
608-624: LGTM! Defensive guards prevent index corruption.The existence checks ensure that
updateStoryonly modifies entries that already exist in bothindexandfilteredIndex, preventing the creation of incomplete entries for filtered-out stories. This directly addresses the root cause described in the PR objectives.
| } else { | ||
| const { id: refId, index, filteredIndex }: any = ref; | ||
| index[storyId] = { | ||
| ...index[storyId], | ||
| ...update, | ||
| } as API_StoryEntry; | ||
| filteredIndex[storyId] = { | ||
| ...filteredIndex[storyId], | ||
| ...update, | ||
| } as API_StoryEntry; | ||
| if (index && index[storyId]) { | ||
| index[storyId] = { | ||
| ...index[storyId], | ||
| ...update, | ||
| } as API_StoryEntry; | ||
| } | ||
| if (filteredIndex && filteredIndex[storyId]) { | ||
| filteredIndex[storyId] = { | ||
| ...filteredIndex[storyId], | ||
| ...update, | ||
| } as API_StoryEntry; | ||
| } | ||
| await fullAPI.updateRef(refId, { index, filteredIndex }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Consistent defensive guards across both branches.
The ref branch now has the same existence checks as the no-ref branch, ensuring consistent behavior. However, the updateDocs function (lines 642-676) still performs unconditional updates without existence checks.
Verify whether updateDocs can encounter the same filtered-entry scenario and needs similar defensive guards for consistency:
🏁 Script executed:
#!/bin/bash
# Description: Check how updateDocs is called and whether it can receive updates for filtered-out docs entries
# Search for calls to updateDocs to understand the calling patterns
rg -nP --type=ts -C3 '\bupdateDocs\s*\('
# Search for DOCS_PREPARED event handling to see if similar filtering scenarios apply
rg -nP --type=ts -C5 'DOCS_PREPARED'Length of output: 19635
Wait, this will need cloning. Let me revise:
🏁 Script executed:
#!/bin/bash
# Search for updateDocs function definition and all its usages
rg -nP --type=ts 'updateDocs' code/core/src/manager-api/modules/stories.tsLength of output: 125
🏁 Script executed:
#!/bin/bash
# Search for updateDocs function definition and all its usages in the stories module
rg -nP --type=ts 'updateDocs'Length of output: 215
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract the updateDocs implementation
sed -n '642,676p' code/core/src/manager-api/modules/stories.ts
Length of output: 997
Add defensive existence checks to updateDocs ref branch for consistency.
The updateStory fix correctly adds existence checks in the ref branch. However, updateDocs's ref branch (lines 664-669) unconditionally updates index[docsId] and filteredIndex[docsId] without verifying those entries exist, mirroring updateStory's pre-fix vulnerability. Apply the same defensive pattern:
} else {
const { id: refId, index, filteredIndex }: any = ref;
if (index && index[docsId]) {
index[docsId] = { ...index[docsId], ...update } as API_DocsEntry;
}
if (filteredIndex && filteredIndex[docsId]) {
filteredIndex[docsId] = { ...filteredIndex[docsId], ...update } as API_DocsEntry;
}
await fullAPI.updateRef(refId, { index, filteredIndex });
}
🤖 Prompt for AI Agents
In code/core/src/manager-api/modules/stories.ts around lines 625 to 640, the ref
branch of updateDocs unconditionally mutates index[docsId] and
filteredIndex[docsId]; add the same defensive existence checks used in
updateStory: guard with if (index && index[docsId]) before merging update into
index[docsId], and if (filteredIndex && filteredIndex[docsId]) before merging
into filteredIndex[docsId], then call await fullAPI.updateRef(refId, { index,
filteredIndex }) as before.
There was a problem hiding this comment.
Currently, I haven’t noticed any errors caused by it, but it might still be necessary. Awaiting suggestions.
|
View your CI Pipeline Execution ↗ for commit aed740f ☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
valentinpalkovic
changed the title
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Code LGTM but I don't feel like I understand why this would fix the reported issue.
I also near systematically get Error 327 on your branch (and on next). So I'm thinking the PR addresses a symptom and not a root cause of the bugs related to switching between mobile and desktop layouts.
@ia319 could you please share more details on why this code change fixes the issue? How have you tested this? Did you systematically see the issue on next and is it systematically fixed? Or does it occur less often?
Regarding why you still get the error on this branch, I suspect it’s an Nx caching issue. I tried multiple times, and on this branch the problem no longer occurs. The error only reappears when I switch back to Previously, I found that the crash was caused by the To diagnose the issue, I added logs in the const item = collapsedData[itemId];
console.log('Item passed to Node:', item);When the bug occurred, the log showed that an incomplete data object (missing the This incomplete data caused the |
In |
|
Thanks for clarifying @ia319! I suspect that we should not try to fix build caching issues with changes to our code. To the best of my understanding, those build caching issues only occur in our monorepo (and contributors usually learn that they will want to purge dependencies and build cache if their branch has significantly diverged from next; I personally use I'm gonna pass this PR on to someone more experienced to see if they agree, but my recommendation would be to close the PR. My reasoning is that adding conditions could end up making it harder to diagnose future bugs, as we'd get missing stories instead of early errors during index computation. |
Thanks for the detailed explanation. I'll go ahead and close this PR, and hope we can identify the root cause. |
|
@Sidnioulz Regarding the “cache issue” I mentioned: I wasn't saying that what I fixed was the cache issue. As for why I made this change, I want to apologize first. Root cause When the user uses tag filtering, stories that do not match (such as Where the bug happens When a background incremental update is triggered for that story, the current filteredIndex[id] = { ...filteredIndex[id], ...update };Since This story, which should have been hidden, gets added back into the index in a corrupted form. What this fix does My change adds a check: Clarification about the reproduction steps You must first select the stories that are about to be filtered, and then perform the reproduction steps. After filtering, you can also reproduce the issue by modifying the label of the stories that still remain on the page, which triggers a hot update. Why selecting first is necessary,I believe the reason is here: code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx#L202 At the moment the update is triggered, it is still referencing the stories before filtering, and when those filtered-out stories receive an update, the update logic breaks. |
|
Thank you so much for the explanation, @ia319! I'll reopen the PR and do a new review. I want to emphasize the importance of following the PR template. There is a section asking you what automatic tests you wrote, and a mandatory section explaining how to manually test the fix. The manual testing section makes things a lot clearer for reviewers, and it avoids this kind of misunderstanding. It's the most important part of the template :) Also, you are now allowed to create branches in the |
Thank you for your reply. I did not intend to cause any misunderstanding regarding the manual testing section in the template. The part mentioned above,
is something I discovered after revisiting it in my research yesterday. |
4 participants
Closes #32997
What I did
After identifying that the error was caused by
LeafNodebecomingundefined, I added logs incode/core/src/manager/components/sidebar/Tree.tsx.The logs show:
These logs indicate that incomplete data was passed to the
Nodecomponent — theitemobject was missing itstypeproperty.As a result,
const LeafNode = ...[item.type]evaluated toundefined, causing the render error.Root Cause
The
updateStoryfunction incorrectly created an incomplete entry in the index when handling an update event for a story that had been filtered out.Fix
Added an existence check in
updateStoryso it only updates entries that already exist.Testing
Manual testing
(added by @Sidnioulz)
Without the PR, I confirm a React crash occurs. With the PR, the sidebar opens (which incidentally clears filters due to #32997 which I'm working on).
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
The error described in the issue has disappeared, and no new errors have been found.
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 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
Release Notes