Add frontend for search #46
Conversation
Summary of ChangesHello @bniladridas, 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 integrates a new frontend component to enable task searching within the application. It provides an interactive search bar and dynamically displays search results, significantly enhancing the application's usability by allowing users to efficiently find specific tasks. 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
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. This stack of pull requests is managed by Graphite. Learn more about stacking. |
bniladridas
force-pushed
the
01-11-demo_22d42e7a_add_frontend_for_search
branch
from
e981492 to
2e95e47
Compare
bniladridas
force-pushed
the
01-11-demo_97ac36a2_add_server_api
branch
from
769dc13 to
34fe4df
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a frontend component for searching tasks. The implementation is a good starting point, but it has significant issues regarding correctness and performance. Specifically, the data fetching logic is prone to race conditions, which can lead to displaying incorrect data. Additionally, API requests are made on every keystroke, which is inefficient and can degrade user experience. I've provided detailed feedback and suggestions to address these high-severity issues. I also included a medium-severity suggestion to make the component more robust by validating API responses.
| useEffect(() => { | ||
| setLoading(true); | ||
| fetch(`/search?query=${encodeURIComponent(searchQuery)}`) | ||
| .then(response => { | ||
| if (!response.ok) { | ||
| throw new Error('Network response was not ok'); | ||
| } | ||
| return response.json(); | ||
| }) | ||
| .then(data => { | ||
| setTasks(data); | ||
| setLoading(false); | ||
| }) | ||
| .catch(error => { | ||
| setError(error.message); | ||
| setLoading(false); | ||
| }); | ||
| }, [searchQuery]); // Depend on searchQuery |
There was a problem hiding this comment.
The current data fetching logic is susceptible to race conditions. If a user types quickly, multiple requests are sent, but there's no guarantee responses will arrive in the same order. This could result in older data overwriting newer data in the UI. To prevent this, you should cancel the previous request when a new one is initiated. The standard approach in React is to use an AbortController within the useEffect hook and call its abort() method in the cleanup function.
useEffect(() => {
const controller = new AbortController();
setLoading(true);
fetch(`/search?query=${encodeURIComponent(searchQuery)}`, { signal: controller.signal })
.then(response => {
if (!response.ok) {
throw new Error('Network response was not ok');
}
return response.json();
})
.then(data => {
setTasks(data);
setLoading(false);
})
.catch(error => {
if (error.name !== 'AbortError') {
setError(error.message);
setLoading(false);
}
});
return () => {
controller.abort();
};
}, [searchQuery]); // Depend on searchQuery| type="text" | ||
| placeholder="Search tasks..." | ||
| value={searchQuery} | ||
| onChange={(e) => setSearchQuery(e.target.value)} |
There was a problem hiding this comment.
The current implementation triggers a search API call on every keystroke because searchQuery is updated directly and is a dependency of the data-fetching useEffect. This can lead to excessive network requests and a poor user experience, especially on slower connections. It's a best practice to debounce the input. This means waiting for the user to stop typing for a short period (e.g., 300-500ms) before triggering the search. You can achieve this by introducing an intermediate state for the input value and using a useEffect with setTimeout to update the searchQuery state that triggers the fetch.
| return response.json(); | ||
| }) | ||
| .then(data => { | ||
| setTasks(data); |
There was a problem hiding this comment.
The code assumes that the data received from the API is always an array. If the API returns something else (e.g., null or an error object with a 200 status code), the call to tasks.map on line 46 will throw an error and crash the component. It's safer to validate the data before setting the state to ensure it's an array.
setTasks(Array.isArray(data) ? data : []);
Summary[Write a one-sentence high-level summary of the change.]
Changes
👉 Use GitHub permalinks (commit-based links).
[Add any closing statement, e.g.:]
All tests pass locally and in CI, improving reliability and maintainability.