Search: Add docs headings to search results#33593
Conversation
|
View your CI Pipeline Execution ↗ for commit 77ef62c
☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIndexes document headings from story inputs and threads a new Changes
Sequence Diagram(s)sequenceDiagram
participant IndexGen as Indexing Engine
participant API as Manager API
participant Search as Sidebar Search
participant UI as Sidebar UI
Note over IndexGen,API: IndexGen emits docs entries including headings[]
IndexGen->>API: Emit DocsIndexEntry (includes headings[])
API->>Search: Provide index entries (stories + docs with headings)
Search->>Search: Build Fuse list (keys: name 0.6, headings 0.1, path 0.3)
Search->>Search: Expand docs entries into per-heading items (id#anchor)
Note right of Search: User selects item or item#anchor
Search->>API: call selectStory(id, { viewMode, scrollTo: anchor? })
API->>UI: Navigate to story/docs URL (append `#anchor` if scrollTo)
UI->>UI: Render item and heading icon/type
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/manager/components/sidebar/SearchResults.tsx (1)
181-211: Avoid rendering the entire headings array (and duplicates) in search results.Line 185 uses
item.headingsdirectly, which is likely an array; this renders a comma-joined list and can duplicate text for heading-specific results whereitem.namealready contains the heading. Consider rendering only the matched heading and skipping if already present in the name.🔧 Suggested adjustment
- const [icon] = item.status ? getStatus(theme, item.status) : []; - const heading = item.type === 'docs' && item.headings ? ` ${item.headings}` : undefined; + const [icon] = item.status ? getStatus(theme, item.status) : []; + const headingMatch = matches.find((match: Match) => match.key === 'headings'); + const heading = + item.type === 'docs' && headingMatch?.value && !item.name.includes(headingMatch.value) + ? ` ${headingMatch.value}` + : undefined;
🤖 Fix all issues with AI agents
In `@code/core/src/manager/components/sidebar/Search.tsx`:
- Around line 212-220: Search.tsx is generating heading anchor IDs with
heading.replaceAll(' ', '-').toLowerCase(), which diverges from Heading.tsx's
github-slugger logic and breaks navigation for punctuation/special chars and
duplicate headings; fix by using the same slugger used in Heading.tsx (import
and call slugs.slug(heading.toLowerCase())) when building the id in the
headings.forEach block (or alternatively use a precomputed heading ID from the
indexer), ensuring the id format matches Heading.tsx so anchors resolve
correctly.
🧹 Nitpick comments (1)
code/core/src/manager/utils/tree.ts (1)
67-69: Redundantnameproperty assignment.The
nameproperty is already included in the spread of...item. Addingname: item.nameexplicitly is redundant sinceItemalready containsname.🔧 Suggested simplification
export const searchItem = (item: Item, ref: Parameters<typeof getPath>[1]): SearchItem => { - return { ...item, refId: ref.id, name: item.name, path: getPath(item, ref) }; + return { ...item, refId: ref.id, path: getPath(item, ref) }; };
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 20.52 MB | 20.58 MB | 🚨 +60 KB 🚨 |
| Dependency size | 16.56 MB | 16.56 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 782 KB | 782 KB | 🎉 -84 B 🎉 |
| Dependency size | 68.16 MB | 68.22 MB | 🚨 +60 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 66.68 MB | 66.74 MB | 🚨 +60 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 1.04 MB | 1.04 MB | 0 B |
| Dependency size | 37.07 MB | 37.13 MB | 🚨 +60 KB 🚨 |
| Bundle Size Analyzer | node | node |
|
Hey @kroeder Do you want to schedule a pairing session about this? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-api/modules/stories.ts`:
- Line 444: The current navigate() call encodes the scrollTo anchor into the
path string which gets treated as a query rather than a true hash, breaking
DocsContainer's hash-based scrolling; change the logic around
navigate(`/${entry.type}/${entry.refId ? `${entry.refId}_${entry.id}` :
entry.id}${scrollTo ? `#${scrollTo}` : ''}`) to omit the `#` fragment from the
path and instead pass scrollTo out-of-band (e.g., via navigation state, context,
or a dedicated message) so the preview iframe can read the hash; locate the call
site using symbols navigate, entry.type, entry.refId, entry.id and update
consumers (DocsContainer.tsx and router.tsx behavior) to read the scroll target
from the chosen out-of-band mechanism.
In `@code/core/src/manager/components/sidebar/Search.tsx`:
- Around line 212-219: The code uses heading.replaceAll(' ', '-').toLowerCase()
to build the item id which can diverge from the renderer's real anchor id;
update the indexing to use the canonical renderer heading id (the actual anchor
id emitted by the docs renderer) instead of the local slug. Concretely: change
the headings data to carry the renderer's anchor (e.g., include anchorId
alongside label in datasetValue.headings), then in the loop that builds list
items (where searchItem(datasetValue, dataset.hash[refId]) is spread) set id to
`${datasetValue.id}#${anchorId}` (use the renderer-provided anchor id) and keep
the visible name as `${datasetValue.name} / ${headingLabel}` so IDs match the
real anchors and avoid collisions for duplicates or punctuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30146cb5-aafb-46f3-9e92-d7c91bbc74ae
📒 Files selected for processing (2)
code/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Search.tsx
kroeder
force-pushed
the
kro/add-searchable-headings
branch
from
82c4381 to
4c80aa1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/core-server/utils/StoryIndexGenerator.ts (1)
495-505:⚠️ Potential issue | 🟠 MajorUse the same name fallback for
headingsas story entries.Line 495 currently uses only
input.name, so stories that rely onstoryNameFromExport(...)are dropped from docs headings and won’t be searchable by heading text.Proposed fix
- const headings = indexInputs.map((input) => input.name).filter(Boolean) as string[]; + const headings = indexInputs + .map((input) => + input.name ?? (input.exportName ? storyNameFromExport(input.exportName) : undefined) + ) + .filter((heading): heading is string => Boolean(heading));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/StoryIndexGenerator.ts` around lines 495 - 505, Change the headings calculation to use the same fallback used for story entries so exports that rely on storyNameFromExport(...) are not dropped: when building headings from indexInputs inside StoryIndexGenerator.ts replace the current mapping of indexInputs.map(input => input.name) with a mapping that uses input.name with the same fallback function used elsewhere (e.g. storyNameFromExport or the project’s story-name helper) and keep the filter(Boolean) cast; this will ensure the docsEntry (DocsCacheEntry & { tags: Tag[] }) headings field contains the computed story name for entries missing input.name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/core/src/core-server/utils/StoryIndexGenerator.ts`:
- Around line 495-505: Change the headings calculation to use the same fallback
used for story entries so exports that rely on storyNameFromExport(...) are not
dropped: when building headings from indexInputs inside StoryIndexGenerator.ts
replace the current mapping of indexInputs.map(input => input.name) with a
mapping that uses input.name with the same fallback function used elsewhere
(e.g. storyNameFromExport or the project’s story-name helper) and keep the
filter(Boolean) cast; this will ensure the docsEntry (DocsCacheEntry & { tags:
Tag[] }) headings field contains the computed story name for entries missing
input.name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c63aa6a2-f08e-4dd5-9fd7-f3224b2c2ce6
📒 Files selected for processing (3)
code/core/package.jsoncode/core/src/core-server/utils/StoryIndexGenerator.tscode/core/src/manager-api/modules/stories.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/manager-api/modules/stories.ts
- code/core/package.json
kroeder
force-pushed
the
kro/add-searchable-headings
branch
from
4c80aa1 to
0f2c27a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/Search.tsx (1)
219-219: Consider using nullish coalescing for status fallback.Using
||treats empty string''as falsy. IfmostCriticalStatusValuecould legitimately be an empty string, use??instead:🔧 Suggested fix
- status: mostCriticalStatusValue || groupStatus[datasetValue.id] || null, + status: mostCriticalStatusValue ?? groupStatus[datasetValue.id] ?? null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Search.tsx` at line 219, The status fallback uses the logical OR which treats empty string as falsy; update the expression that sets status (the line using mostCriticalStatusValue, groupStatus, and datasetValue.id) to use nullish coalescing so empty string is preserved—i.e., replace the `||` chain with `??` so it becomes: mostCriticalStatusValue ?? groupStatus[datasetValue.id] ?? null.
🤖 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/Search.tsx`:
- Line 219: The status fallback uses the logical OR which treats empty string as
falsy; update the expression that sets status (the line using
mostCriticalStatusValue, groupStatus, and datasetValue.id) to use nullish
coalescing so empty string is preserved—i.e., replace the `||` chain with `??`
so it becomes: mostCriticalStatusValue ?? groupStatus[datasetValue.id] ?? null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e1ac708-1c57-4a13-8a61-537869a9f9dc
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
code/core/package.jsoncode/core/src/core-server/utils/StoryIndexGenerator.tscode/core/src/manager-api/lib/stories.tscode/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/SearchResults.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/utils/tree.tscode/core/src/types/modules/api-stories.tscode/core/src/types/modules/indexer.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/manager-api/modules/stories.ts
- code/core/src/manager/utils/tree.ts
- code/core/src/types/modules/indexer.ts
- code/core/src/manager/components/sidebar/SearchResults.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/router/router.tsx (1)
1-1: Unused import:useRefis imported but not used.The
useRefhook is imported but doesn't appear to be used anywhere in the file.♻️ Proposed fix
-import React, { useCallback, useRef } from 'react'; +import React, { useCallback } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/router/router.tsx` at line 1, Remove the unused import by deleting useRef from the React import in router.tsx: the import statement currently includes useRef (import React, { useCallback, useRef } from 'react';) but useRef is not referenced anywhere in this file, so change that import to only include used hooks (e.g., useCallback) to eliminate the unused symbol and avoid linter warnings.
🤖 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/router/router.tsx`:
- Line 49: Remove the stray debugger statement in router.tsx (the single
"debugger;" line) before merging; simply delete that line and, if you need
runtime debugging information instead, replace it with a conditional
console.debug guarded by a development flag (e.g., if (process.env.NODE_ENV ===
'development') console.debug(...)) and run lint/tests to confirm no debug
artifacts remain.
---
Nitpick comments:
In `@code/core/src/router/router.tsx`:
- Line 1: Remove the unused import by deleting useRef from the React import in
router.tsx: the import statement currently includes useRef (import React, {
useCallback, useRef } from 'react';) but useRef is not referenced anywhere in
this file, so change that import to only include used hooks (e.g., useCallback)
to eliminate the unused symbol and avoid linter warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8cd28d3-039b-427a-aa4e-1018549a38fd
📒 Files selected for processing (2)
code/core/src/manager-api/modules/stories.tscode/core/src/router/router.tsx
| const navigate = R.useNavigate(); | ||
|
|
||
| return useCallback((to: R.To | number, { plain, ...options } = {} as NavigateOptions) => { | ||
| debugger; |
There was a problem hiding this comment.
Remove debugger statement before merging.
This is a development artifact that will pause JavaScript execution in any browser with developer tools open, breaking the user experience.
🐛 Proposed fix
return useCallback((to: R.To | number, { plain, ...options } = {} as NavigateOptions) => {
- debugger;
if (typeof to === 'string' && to.startsWith('#')) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/router/router.tsx` at line 49, Remove the stray debugger
statement in router.tsx (the single "debugger;" line) before merging; simply
delete that line and, if you need runtime debugging information instead, replace
it with a conditional console.debug guarded by a development flag (e.g., if
(process.env.NODE_ENV === 'development') console.debug(...)) and run lint/tests
to confirm no debug artifacts remain.
|
To run Playwright: |
kroeder
force-pushed
the
kro/add-searchable-headings
branch
from
600b46f to
acb6bd9
Compare
| * | ||
| * @experimental This feature is in early development and may change significantly in future releases. | ||
| */ | ||
| experimentalSearchDocsHeadings?: boolean; |
There was a problem hiding this comment.
since there are e2e tests, do we need to activate this by default in the react vite ts sandbox?
kroeder
force-pushed
the
kro/add-searchable-headings
branch
2 times, most recently
from
c37998c to
3588398
Compare
| for (const datasetValue of datasetValues) { | ||
| const storyStatuses = allStatuses?.[datasetValue.id]; | ||
| const mostCriticalStatusValue = storyStatuses | ||
| ? getMostCriticalStatusValue(Object.values(storyStatuses).map((s) => s.value)) | ||
| : null; | ||
|
|
||
| list.push({ | ||
| ...searchItem(datasetValue, dataset.hash[refId]), | ||
| status: mostCriticalStatusValue ?? groupStatus[datasetValue.id] ?? null, | ||
| }); |
There was a problem hiding this comment.
Mainly did the first refactor commit because it was very frustrating integrating my code additions into this reducer. If the reviewer thinks this is unnecessary, I can try to revert it and apply my additions in-between
| let resultItem = baseItem; | ||
| if (anchor && item.type === 'docs') { | ||
| const matchingHeading = (item as any).headings?.find( | ||
| (h: string) => h.replaceAll(' ', '-').toLowerCase() === anchor |
There was a problem hiding this comment.
probably a better way to do it, CodeRabbit has the same feeling 😄
Is there some kind of "getStorySlug()" or something?
| channel.on(NAVIGATE_URL, (hash: string) => { | ||
| document.querySelector(hash)?.scrollIntoView({ behavior: 'smooth' }); | ||
| }); |
There was a problem hiding this comment.
Kinda feels like I'm misusing this..
the weird thing is that there is already behavior to scrollIntoView() a sub-heading when switching stories. if you search a sub heading on the same docs page you currently are, the scrollIntoView() logic does not get triggerd so I needed this code to fix it
kroeder
force-pushed
the
kro/add-searchable-headings
branch
from
3588398 to
6b7fc7e
Compare
kroeder
force-pushed
the
kro/add-searchable-headings
branch
4 times, most recently
from
4da6a72 to
49425d3
Compare
kroeder
force-pushed
the
kro/add-searchable-headings
branch
from
49425d3 to
190d790
Compare
kroeder
force-pushed
the
kro/add-searchable-headings
branch
from
190d790 to
77ef62c
Compare
kroeder
changed the title
Sidnioulz
changed the title
|
@copilot resolve the merge conflicts in this pull request |
…adings # Conflicts: # code/core/package.json # yarn.lock Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
Resolved. I merged |
Closes N/A
Created from #26531
What I did
I started #26531 from scratch since it was hard to merge next into the original PR.
This PR adds docs-mode headings to the search results and makes it possible to not only search for the main title of a docs story. This aims to improve UX
Example
Given a story with the title "Table" and a docs heading "## Filtering", today it's not possible to search for "Filtering" or "Table Filtering".
With this PR, it should be possible to search for exactly that.
Checklist for Contributors
Todo's
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!
npx playwright test e2e-tests/navigation.spec.ts --uiand run the 4 added tests in this PRDocumentation
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
New Features
Improvements