handy search item paths for easier future finding - #431
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new hover-driven tab selection can leave the UI in an invalid or “stuck highlighted” state (and can produce invalid Tabs values), which should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enhances the Navigation search UX by attaching section/context information to search results, displaying a breadcrumb path for each result, and visually linking search results to their parent navigation section via tab highlighting on hover.
Changes:
- Enrich Navigation search entries with
sectionIdand a computed breadcrumb string, and render that breadcrumb in the results list. - Add “hover-to-highlight-tab” behavior by plumbing a new hover callback from
SearchPanelintoNavigation. - Update Storybook sample data to use realistic routes and bump version/changelog.
File summaries
| File | Description |
|---|---|
| src/components/SearchPanel.js | Builds breadcrumbed search entries and renders breadcrumb text; adds hover callbacks on results. |
| src/components/Navigation.js | Tracks hovered section id and uses it to drive Tabs selection while the search panel is open. |
| src/components/Panel.stories.js | Updates sample hrefs to real-ish paths for better Storybook demos. |
| src/components/Navigation.stories.js | Updates sample hrefs to real-ish paths for better Storybook demos. |
| package.json | Bumps package version to 34.1.0. |
| CHANGELOG.md | Adds 34.1.0 entry describing the search path/breadcrumb enhancement. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/performance issues in the new hover + memoized entry logic that should be addressed to avoid sticky tab selection and unnecessary allocations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/components/SearchPanel.js:27
menuEntriescallsmenu.flatMap(...), which will throw ifmenuis evernull/undefined. Since this component already provides a default foronHoverSection, it would be safer to also defaultmenuto an empty array to avoid runtime crashes if the prop is omitted or not yet loaded.
src/components/SearchPanel.js:58uniqueEntriesuses object spread insidereduce, which creates a new object on every iteration and can become unnecessarily expensive as the menu grows. Mutating the accumulator in-place keeps the same behavior (first entry perhref) without the extra allocations.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new hover-to-highlight behavior can leave stale tab selection state and the updated SearchPanel markup has invalid list semantics that impacts accessibility.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
| <Box key={entry.href}> | ||
| <a href={entry.href} style={{ textDecoration: 'none' }}> | ||
| <a | ||
| href={entry.href} | ||
| style={{ textDecoration: 'none' }} | ||
| onMouseEnter={() => onHoverSection(entry.sectionId)} |
| menuEntries.reduce((uniques, entry) => { | ||
| if (!uniques[entry.href]) { | ||
| return { ...uniques, [entry.href]: entry }; | ||
| } | ||
| return uniques; | ||
| }, {}) |
There was a problem hiding this comment.
🔵 Needs a closer look
Search results currently include items regardless of showInMenu, which can expose hidden menu entries and diverges from how Panel renders the same navigation data.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/components/SearchPanel.js:47
- Search results currently include every
category.itemsentry, even ones that are not shown in the navigation menu (showInMenu: false). SincePanelhides non-menu items, the search panel should apply the same filter to avoid exposing hidden routes and inconsistent UX.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
No description provided.