-
-
Notifications
You must be signed in to change notification settings - Fork 23.1k
fix: apply search filter to tools list view (#5469) #5487
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Wanyi9988, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where the search functionality for the tools overview was non-operational in the list view. By integrating the existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request fixes an issue where the search filter was not applied to the tools list view. The change correctly adds the filtering logic, making the list view consistent with the card view. My review points out a significant issue with the current search implementation: it's performed client-side on paginated data, which leads to an incorrect search experience as it only searches the current page. I've recommended moving the filtering logic to the server for correct behavior. Additionally, I've suggested using useMemo to optimize performance and reduce code duplication for the filtering logic.
| </Box> | ||
| ) : ( | ||
| <ToolsTable data={getAllToolsApi.data.data} isLoading={isLoading} onSelect={edit} /> | ||
| <ToolsTable data={getAllToolsApi.data.data.filter(filterTools)} isLoading={isLoading} onSelect={edit} /> |
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.
While this change correctly applies the filter to the list view, making it consistent with the card view, there are some improvements to consider for the overall filtering implementation:
-
Client-side filtering on paginated data: The most significant issue is that filtering is performed on the client-side on paginated data. This means the search only applies to the tools on the currently displayed page. If a user searches for a tool that exists but is on a different page, it will not be found. This can be very confusing. For the search to work as expected, it should be implemented on the server-side. The search term should be passed to the API, which would then return the correctly filtered and paginated results.
-
Performance and Code Duplication: The
.filter()operation is executed on every render, and the same logic is duplicated for the card view on line 243. To improve performance and keep the code DRY, you can memoize the filtered data usinguseMemoand reuse it.
Here's an example of how you could use useMemo:
const filteredData = useMemo(() => {
if (!getAllToolsApi.data?.data) return [];
return getAllToolsApi.data.data.filter(filterTools);
}, [getAllToolsApi.data?.data, search]);
// Then use `filteredData` for both card and list views.While the server-side filtering is a larger change, applying useMemo is a smaller refactor that would improve the current implementation.
Description
This PR fixes the issue where the search box in the tools overview was not working in list view.
Fixes #5469
Before
After
Notes