Core: Prevent navigating to hidden (filtered) item #32715
Conversation
📝 WalkthroughWalkthroughNavigation and selection logic in the stories manager was changed to prefer the store's filteredIndex (and filteredHash) when resolving jumps or selections, expanded selectStory to handle descendants/docs and legacy fallbacks, added lastTrackedStoryId updates, a URL/view navigation helper, and a new public selectFirstStory(). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant API as ManagerAPI.stories
participant S as Store (index / filteredIndex)
participant R as Router
U->>API: selectStory(idOrHash)
API->>S: read index, filteredIndex, filteredHash, current selection
alt filteredIndex applicable (ref or within ref)
API->>S: resolve entry via filteredIndex/filteredHash
else
API->>S: resolve via full index
end
alt entry is leaf
API->>R: navigate to /story/:id or /docs/:id
else entry is component/docs with children
API->>S: find first valid descendant (prefer filteredIndex)
API->>R: navigate to resolved leaf/docs path
end
API->>S: update lastTrackedStoryId and view state
note right of API: Legacy fallback — find child in legacy structure and route to it
U->>API: jumpToStory / jumpToComponent
API->>S: use filteredIndex when present to compute next/prev
API->>R: update URL/hash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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)
446-457: Selecting first visible descendant uses filtered index — add a guard for fully filtered components.The filteredHash-based descendant resolution is correct. If the component itself is filtered out (no entry in filteredHash),
findLeafEntry(filteredHash, entry.id)will throw. Consider a safe fallback.- // If the entry is not a story or docs, find the first descendant entry that is - gotoStory(api.findLeafEntry(filteredHash, entry.id)); + // If the entry is not a story or docs, find the first visible descendant in filtered index + const nextEntry = + filteredHash[entry.id] && api.findLeafEntry(filteredHash, entry.id); + gotoStory(nextEntry);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager-api/modules/stories.ts(4 hunks)code/core/src/manager-api/tests/stories.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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
**/*.{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/tests/stories.test.tscode/core/src/manager-api/modules/stories.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/tests/stories.test.tscode/core/src/manager-api/modules/stories.ts
🧬 Code graph analysis (2)
code/core/src/manager-api/tests/stories.test.ts (2)
code/core/src/manager-api/tests/mockStoriesEntries.ts (1)
navigationEntries(71-120)code/core/src/manager-api/tests/url.test.js (1)
store(143-151)
code/core/src/manager-api/modules/stories.ts (1)
code/core/src/csf/index.ts (2)
sanitize(8-16)toId(27-28)
⏰ 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: Danger JS
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/core/src/manager-api/modules/stories.ts (3)
371-390: Using filteredIndex for component jumps aligns with the fix.Switching to filteredIndex ensures hidden items are skipped when jumping between components. Looks good.
392-411: Using filteredIndex for story jumps is correct.This prevents landing on hidden stories when navigating with next/prev. Good change.
482-484: Prefer first child that exists in provided index — good.This fixes cases where the first child is filtered out. Works as intended.
code/core/src/manager-api/tests/stories.test.ts (1)
1109-1149: Great coverage for filtered selection behavior.The test correctly asserts selecting the first visible child when the first is hidden.
Optionally add a similar test for a component whose first child is a hidden docs page to cover both story/docs variants.
|
View your CI Pipeline Execution ↗ for commit d594c3d
☁️ Nx Cloud last updated this comment at |
|
@ghengeveld will you finish this PR? |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 30.19 MB | 30.19 MB | 🚨 +553 B 🚨 |
| Dependency size | 17.31 MB | 17.36 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 921 KB | 921 KB | 🎉 -84 B 🎉 |
| Dependency size | 79.87 MB | 79.92 MB | 🚨 +54 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 76.30 MB | 76.35 MB | 🚨 +54 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 🚨 +30 B 🚨 |
| Dependency size | 47.50 MB | 47.55 MB | 🚨 +54 KB 🚨 |
| Bundle Size Analyzer | node | node |
Closes #32684
What I did
Use the
filteredIndexrather than fullindexwhen navigating to the next/prev story/component, or when selecting a non-leaf node (e.g. by clicking a component). Only when a hidden/filtered item is explicitly targeted (by id or name) should it be selectable.I've refactored
selectStoryto avoid recursion and simplify it.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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