-
Notifications
You must be signed in to change notification settings - Fork 24
Fix: Sidebar filter produces duplicate results #2914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where items matching a filter by both title and alias were being duplicated in the filtered navigation results. The fix adds test coverage for the getFilteredNavItems helper function and refactors the implementation to use modern JavaScript patterns like in operator for type narrowing and for...of loops for clearer control flow.
Key changes:
- Adds comprehensive unit tests for the
getFilteredNavItemshelper - Fixes duplication bug by using
continueto prevent items from being added twice when both title and alias match - Refactors to use
inoperator instead ofhasOwnPropertyfor better type safety
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/sidebar/helpers/get-filtered-nav-items.ts | Refactored filtering logic to prevent duplicate items and improved code maintainability with clearer control flow |
| src/components/sidebar/helpers/tests/get-filtered-nav-items.test.ts | Added comprehensive test suite covering edge cases including the title+alias duplication bug |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/sidebar/helpers/__tests__/get-filtered-nav-items.test.ts
Outdated
Show resolved
Hide resolved
π¦ Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action π€ This PR introduced no changes to the javascript bundle π |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
RubenSandwich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for adding tests.
π Relevant links
ποΈ What
This PR:
In the first commit, I only add tests and specifically add a test to reproduce the bug.
In the second I fix the bug.
And while I am already here, I decided to propose a few opinionated changes:
'routes' in itemrather than item.hasOwnProperty('routes'), because it allows for Typescript narrowing, and enables us to remove all casts ("as")π€· Why
@l2fprod found a bug:

and created a ticket for us to fix it. https://hashicorp.atlassian.net/browse/TF-32415
π οΈ How
πΈ Design Screenshots
π§ͺ Testing
This PR adds unit tests for the helper function.
π Anything else?
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.