Fix: Select first visible story when clicking component with tag filters applied #32688
Conversation
📝 WalkthroughWalkthroughselectStory now reads and uses refs[ref].filteredIndex (instead of index) when resolving a target story and computing the navigation hash; tests were added to ensure selecting a component navigates to the first visible (non-filtered) story when earlier stories are filtered out. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ManagerAPI as Manager API (selectStory)
participant State as Manager State (refs.*.filteredIndex)
participant Router
User->>ManagerAPI: selectStory({ ref, storyId/component })
ManagerAPI->>State: Read refs[ref].filteredIndex
Note right of State #DDEEFF: filteredIndex contains only visible entries
ManagerAPI->>ManagerAPI: Resolve target (use first visible story if component)
ManagerAPI->>ManagerAPI: Compute navigation hash from filteredIndex
ManagerAPI->>Router: navigate(hash)
Router-->>User: URL and view updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
Files:
🧬 Code graph analysis (2)code/core/src/manager-api/modules/stories.ts (1)
code/core/src/manager-api/tests/stories.test.ts (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
429-431: LGTM — filteredIndex for sidebar selection
The change ensures sidebar clicks respect active filters. For consistency, evaluate whetherjumpToComponentandjumpToStoryshould also usefilteredIndexinstead ofindex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager-api/modules/stories.ts(1 hunks)code/core/src/manager-api/tests/stories.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/manager-api/modules/stories.tscode/core/src/manager-api/tests/stories.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/manager-api/modules/stories.tscode/core/src/manager-api/tests/stories.test.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/core/src/manager-api/tests/stories.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/core/src/manager-api/tests/stories.test.ts
🧬 Code graph analysis (2)
code/core/src/manager-api/modules/stories.ts (1)
code/core/src/manager-api/tests/url.test.js (1)
store(143-151)
code/core/src/manager-api/tests/stories.test.ts (1)
code/core/src/types/modules/indexer.ts (1)
StoryIndex(174-177)
🔇 Additional comments (1)
code/core/src/manager-api/tests/stories.test.ts (1)
31-31: LGTM! Type import is correct.The
StoryIndextype import is properly used to type the test entries and follows TypeScript best practices.
| it('navigates to first visible story when component is selected and first story is filtered out', async () => { | ||
| const initialState = { path: '/story/a--1', storyId: 'a--1', viewMode: 'story' }; | ||
| const moduleArgs = createMockModuleArgs({ initialState }); | ||
| const { api } = initStories(moduleArgs as unknown as ModuleArgs); | ||
| const { navigate, store } = moduleArgs; | ||
|
|
||
| const entries: StoryIndex['entries'] = { | ||
| 'component-a--story-1': { | ||
| type: 'story', | ||
| subtype: 'story', | ||
| id: 'component-a--story-1', | ||
| title: 'Component A', | ||
| name: 'Story 1', | ||
| tags: ['dev'], | ||
| importPath: './component-a.ts', | ||
| }, | ||
| 'component-a--story-2': { | ||
| type: 'story', | ||
| subtype: 'story', | ||
| id: 'component-a--story-2', | ||
| title: 'Component A', | ||
| name: 'Story 2', | ||
| tags: [], | ||
| importPath: './component-a.ts', | ||
| }, | ||
| 'component-a--story-3': { | ||
| type: 'story', | ||
| subtype: 'story', | ||
| id: 'component-a--story-3', | ||
| title: 'Component A', | ||
| name: 'Story 3', | ||
| tags: [], | ||
| importPath: './component-a.ts', | ||
| }, | ||
| }; | ||
|
|
||
| await store.setState({ | ||
| filters: { | ||
| 'test-filter': (entry: any) => !entry.tags?.includes('dev'), | ||
| }, | ||
| }); | ||
|
|
||
| await api.setIndex({ v: 5, entries }); | ||
|
|
||
| api.selectStory('component-a'); | ||
|
|
||
| expect(navigate).toHaveBeenCalledWith('/story/component-a--story-2'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move test inside the selectStory describe block for consistency.
The test logic is correct and validates the fix well. However, it's placed outside the describe('selectStory', ...) block (which ends at line 1214), breaking the test organization pattern. All other selectStory tests are grouped within that describe block.
Apply this diff to move the test inside the describe block:
- });
it('navigates to first visible story when component is selected and first story is filtered out', async () => {
const initialState = { path: '/story/a--1', storyId: 'a--1', viewMode: 'story' };
const moduleArgs = createMockModuleArgs({ initialState });
const { api } = initStories(moduleArgs as unknown as ModuleArgs);
const { navigate, store } = moduleArgs;
const entries: StoryIndex['entries'] = {
'component-a--story-1': {
type: 'story',
subtype: 'story',
id: 'component-a--story-1',
title: 'Component A',
name: 'Story 1',
tags: ['dev'],
importPath: './component-a.ts',
},
'component-a--story-2': {
type: 'story',
subtype: 'story',
id: 'component-a--story-2',
title: 'Component A',
name: 'Story 2',
tags: [],
importPath: './component-a.ts',
},
'component-a--story-3': {
type: 'story',
subtype: 'story',
id: 'component-a--story-3',
title: 'Component A',
name: 'Story 3',
tags: [],
importPath: './component-a.ts',
},
};
await store.setState({
filters: {
'test-filter': (entry: any) => !entry.tags?.includes('dev'),
},
});
await api.setIndex({ v: 5, entries });
api.selectStory('component-a');
expect(navigate).toHaveBeenCalledWith('/story/component-a--story-2');
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('navigates to first visible story when component is selected and first story is filtered out', async () => { | |
| const initialState = { path: '/story/a--1', storyId: 'a--1', viewMode: 'story' }; | |
| const moduleArgs = createMockModuleArgs({ initialState }); | |
| const { api } = initStories(moduleArgs as unknown as ModuleArgs); | |
| const { navigate, store } = moduleArgs; | |
| const entries: StoryIndex['entries'] = { | |
| 'component-a--story-1': { | |
| type: 'story', | |
| subtype: 'story', | |
| id: 'component-a--story-1', | |
| title: 'Component A', | |
| name: 'Story 1', | |
| tags: ['dev'], | |
| importPath: './component-a.ts', | |
| }, | |
| 'component-a--story-2': { | |
| type: 'story', | |
| subtype: 'story', | |
| id: 'component-a--story-2', | |
| title: 'Component A', | |
| name: 'Story 2', | |
| tags: [], | |
| importPath: './component-a.ts', | |
| }, | |
| 'component-a--story-3': { | |
| type: 'story', | |
| subtype: 'story', | |
| id: 'component-a--story-3', | |
| title: 'Component A', | |
| name: 'Story 3', | |
| tags: [], | |
| importPath: './component-a.ts', | |
| }, | |
| }; | |
| await store.setState({ | |
| filters: { | |
| 'test-filter': (entry: any) => !entry.tags?.includes('dev'), | |
| }, | |
| }); | |
| await api.setIndex({ v: 5, entries }); | |
| api.selectStory('component-a'); | |
| expect(navigate).toHaveBeenCalledWith('/story/component-a--story-2'); | |
| }); | |
| }); | |
| it('navigates to first visible story when component is selected and first story is filtered out', async () => { | |
| const initialState = { path: '/story/a--1', storyId: 'a--1', viewMode: 'story' }; | |
| const moduleArgs = createMockModuleArgs({ initialState }); | |
| const { api } = initStories(moduleArgs as unknown as ModuleArgs); | |
| const { navigate, store } = moduleArgs; | |
| const entries: StoryIndex['entries'] = { | |
| 'component-a--story-1': { | |
| type: 'story', | |
| subtype: 'story', | |
| id: 'component-a--story-1', | |
| title: 'Component A', | |
| name: 'Story 1', | |
| tags: ['dev'], | |
| importPath: './component-a.ts', | |
| }, | |
| 'component-a--story-2': { | |
| type: 'story', | |
| subtype: 'story', | |
| id: 'component-a--story-2', | |
| title: 'Component A', | |
| name: 'Story 2', | |
| tags: [], | |
| importPath: './component-a.ts', | |
| }, | |
| 'component-a--story-3': { | |
| type: 'story', | |
| subtype: 'story', | |
| id: 'component-a--story-3', | |
| title: 'Component A', | |
| name: 'Story 3', | |
| tags: [], | |
| importPath: './component-a.ts', | |
| }, | |
| }; | |
| await store.setState({ | |
| filters: { | |
| 'test-filter': (entry: any) => !entry.tags?.includes('dev'), | |
| }, | |
| }); | |
| await api.setIndex({ v: 5, entries }); | |
| api.selectStory('component-a'); | |
| expect(navigate).toHaveBeenCalledWith('/story/component-a--story-2'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In code/core/src/manager-api/tests/stories.test.ts around lines 1215 to 1263,
the test "navigates to first visible story when component is selected and first
story is filtered out" is located outside the describe('selectStory', ...) block
(which ends at line 1214); move the entire it(...) block so it sits inside that
describe('selectStory', ...) block to match the other selectStory tests and
maintain test organization and scoping. Ensure you place it before the closing
brace of the describe, preserve the test body unchanged, and update any
surrounding indentation if needed.
There was a problem hiding this comment.
@kamal0620040, thank you for moving the test into the selectStory describe block! The test organization is now consistent with the rest of the test suite. 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
kamal0620040
force-pushed
the
fix/filtered-story-selection
branch
from
1f1d954 to
f2336c5
Compare
|
Thank you for your contribution. We have already worked on it internally. Superseded by #32715. |
2 participants
Closes #32684
What I did
Fixed a bug where clicking a component in the sidebar would select a hidden first item instead of the first visible item when tag filtering is applied.
Problem
When using tag filters to hide certain stories, clicking on a component in the sidebar would still navigate to the first story in the component's children array, even if that story was filtered out. This resulted in showing a filtered/hidden story instead of the first visible one.
Solution
Modified the
selectStoryfunction incode/core/src/manager-api/modules/stories.tsto usefilteredIndexinstead ofindexwhen finding the leaf entry to navigate to. This ensures that only visible (non-filtered) stories are considered when selecting the first story of a component.Changes Made
selectStoryto usefilteredIndexfor both main stories and refsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
The new unit test (
navigates to first visible story when component is selected and first story is filtered out) verifies:'dev'tag'dev'tagselectStory('component-a')to select the componentAll existing
selectStorytests continue to pass (14 tests total).Manual testing
To manually test this fix:
Run a sandbox template:
Create or find a component with multiple stories where the first story has a tag (e.g.,
dev)Open Storybook and use the tag filter icon in the sidebar to filter out stories with that tag
Click on the component name in the sidebar (not individual stories)
Expected behavior: The first visible/unfiltered story should be selected and displayed
Before this fix: The hidden first story would be selected, showing nothing or wrong content
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
Tests