Core: Only show modified/affected icons on components and groups#34429
Core: Only show modified/affected icons on components and groups#34429ghengeveld wants to merge 2 commits into
Conversation
|
View your CI Pipeline Execution ↗ for commit 4b9f32e
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughConsolidated type imports in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
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/components/sidebar/Tree.tsx (1)
265-268: Consider skippinguseContextMenu()for non-internal refs.This now instantiates the full hook for every rendered node and then immediately replaces it with a noop object when
refId !== 'storybook_internal'. SinceuseContextMenu()subscribes to context and sets up several memoized branches, that adds avoidable render work to large composed sidebars.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Tree.tsx` around lines 265 - 268, Currently the component always calls the useContextMenu hook and then overwrites its result for non-internal refs, causing unnecessary hook work; change the API so useContextMenu receives refId (call it from Tree.tsx as useContextMenu(item, statusLinks, api, refId)) and inside the hook immediately return the noop object ({ node: null, onMouseEnter: () => {} }) when refId !== 'storybook_internal' before subscribing to contexts or running memoized logic, so the Tree.tsx code can use the hook directly without creating then replacing its result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/manager/components/sidebar/Tree.tsx`:
- Around line 216-222: The helper getDisplayStatus currently only remaps
HIDDEN_STORY_STATUSES for itemType === 'story', leaving docs able to show
'modified'/'affected'; update the condition to include docs (e.g., itemType ===
'story' || itemType === 'docs') so that when status is in HIDDEN_STORY_STATUSES
you return { status: 'status-value:unknown', label: 'Unknown' for docs as well;
ensure you reference getDisplayStatus, Item['type'], HIDDEN_STORY_STATUSES and
preserve the existing fallback formatting for other types/statuses.
---
Nitpick comments:
In `@code/core/src/manager/components/sidebar/Tree.tsx`:
- Around line 265-268: Currently the component always calls the useContextMenu
hook and then overwrites its result for non-internal refs, causing unnecessary
hook work; change the API so useContextMenu receives refId (call it from
Tree.tsx as useContextMenu(item, statusLinks, api, refId)) and inside the hook
immediately return the noop object ({ node: null, onMouseEnter: () => {} }) when
refId !== 'storybook_internal' before subscribing to contexts or running
memoized logic, so the Tree.tsx code can use the hook directly without creating
then replacing its result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fa00af4-7076-4af8-b3be-c669d529aae8
📒 Files selected for processing (1)
code/core/src/manager/components/sidebar/Tree.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/Tree.tsx (1)
265-272: Good fix for React hooks rules, but consider performance for external refs.The restructuring correctly addresses the React hooks rule violation—hooks are now called unconditionally before any early returns. However,
useContextMenuis now invoked for every node including external-ref items, even though the result is immediately discarded. Per context snippet 4, the hook creates internal state (3×useState), subscribes toStatusContext, and callsapi.getShortcutKeys()on every mount.For storybooks composing large external refs, this could accumulate unnecessary subscriptions and state. Consider passing a
skipflag or similar touseContextMenuto short-circuit internal work when the menu won't be used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Tree.tsx` around lines 265 - 272, The hook useContextMenu is being invoked for every node (including external refs) causing unnecessary state, subscriptions, and api.getShortcutKeys() calls; modify calls in Tree.tsx so you pass a short-circuit flag (e.g., skip or isExternalRef) when refId === 'storybook_internal' is false, and update useContextMenu to early-return a lightweight { node: null, onMouseEnter: () => {} } when skip is true, avoiding creation of useState hooks, StatusContext subscription, and api.getShortcutKeys() work for external-ref items.
🤖 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/components/sidebar/Tree.tsx`:
- Around line 265-272: The hook useContextMenu is being invoked for every node
(including external refs) causing unnecessary state, subscriptions, and
api.getShortcutKeys() calls; modify calls in Tree.tsx so you pass a
short-circuit flag (e.g., skip or isExternalRef) when refId ===
'storybook_internal' is false, and update useContextMenu to early-return a
lightweight { node: null, onMouseEnter: () => {} } when skip is true, avoiding
creation of useState hooks, StatusContext subscription, and
api.getShortcutKeys() work for external-ref items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80dce32b-e377-408c-a351-542eb2307eb8
📒 Files selected for processing (1)
code/core/src/manager/components/sidebar/Tree.tsx
|
Already done in #34346 |
1 participant
What I did
Showing a "modified" or "affected" icon on a story can be confusing, because when only a single story is changed, all siblings will be marked "modified" as well, due to the fact that change detection is file-based. By hiding the status dot on the story level, we avoid this confusion.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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
Bug Fixes
Refactor