Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a working search functionality in the header navigation bar that allows users to search for events. When a user enters a search query and presses Enter (or clicks the search icon), they are navigated to the events page with matching results displayed based on the search query passed through URL parameters.
Key Changes:
- Added form submission handling to the Header search bar with URL navigation
- Integrated URL search parameters in EventsPage to initialize and sync search state
- Enhanced search TextField with submit button functionality and keyboard shortcuts
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
frontend/my-react-app/src/components/Header.tsx |
Converted search bar to a controlled form component with submit handler that navigates to /events with encoded search parameter; added state management for search input and Escape key handler to clear search |
frontend/my-react-app/src/pages/EventsPage.tsx |
Added useSearchParams hook to read/write URL search parameters; implemented useEffect to initialize search query from URL; updated search input onChange to sync search state with URL parameters in real-time |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const val = e.target.value; | ||
| setSearchQuery(val); | ||
| if (val.trim().length > 0) { | ||
| setSearchParams({ search: val }); | ||
| } else { | ||
| // Remove param if empty | ||
| searchParams.delete('search'); | ||
| setSearchParams(searchParams); | ||
| } | ||
| }} | ||
| placeholder="Search by title, description, or location..." | ||
| /> |
There was a problem hiding this comment.
The search input updates URL parameters on every keystroke, which creates a new browser history entry for each character typed. This makes it difficult for users to use the browser's back button effectively and can cause performance issues.
Consider updating the URL only when the user finishes typing (using a debounce) or when they submit the search (blur event or Enter key). The Header.tsx implementation handles this better by only updating on form submission.
| const val = e.target.value; | |
| setSearchQuery(val); | |
| if (val.trim().length > 0) { | |
| setSearchParams({ search: val }); | |
| } else { | |
| // Remove param if empty | |
| searchParams.delete('search'); | |
| setSearchParams(searchParams); | |
| } | |
| }} | |
| placeholder="Search by title, description, or location..." | |
| /> | |
| setSearchQuery(e.target.value); | |
| }} | |
| onBlur={() => { | |
| if (searchQuery.trim().length > 0) { | |
| setSearchParams({ search: searchQuery }); | |
| } else { | |
| // Remove param if empty | |
| searchParams.delete('search'); | |
| setSearchParams(searchParams); | |
| } | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter') { | |
| if (searchQuery.trim().length > 0) { | |
| setSearchParams({ search: searchQuery }); | |
| } else { | |
| searchParams.delete('search'); | |
| setSearchParams(searchParams); | |
| } | |
| } | |
| }} | |
| placeholder="Search by title, description, or location..." |
| const val = e.target.value; | ||
| setSearchQuery(val); | ||
| if (val.trim().length > 0) { | ||
| setSearchParams({ search: val }); |
There was a problem hiding this comment.
There's an inconsistency in how search parameters are handled between Header and EventsPage. The Header trims the search value before encoding it in the URL (trimmed = headerSearch.trim()), but EventsPage sets the URL parameter with the untrimmed value (setSearchParams({ search: val })).
This inconsistency could lead to unexpected behavior. For example, if a user types " test " (with spaces) in EventsPage, the URL will contain the spaces, but if they search from the Header with " test ", the URL will contain "test" without spaces.
Consider trimming the value before setting the search params: setSearchParams({ search: val.trim() })
| setSearchParams({ search: val }); | |
| setSearchParams({ search: val.trim() }); |
| onChange={(e) => setSearchQuery(e.target.value)} | ||
| onChange={(e) => { | ||
| const val = e.target.value; | ||
| setSearchQuery(val); |
There was a problem hiding this comment.
The onChange handler calls setSearchQuery(val) and then updates the URL params with setSearchParams. However, updating searchParams triggers the useEffect on line 128-131, which also calls setSearchQuery. This causes the search query state to be set twice for every keystroke, leading to redundant state updates and re-renders.
Consider removing the setSearchQuery(val) call from the onChange handler and letting the useEffect handle all search query updates from URL params. This maintains a single source of truth (the URL) for the search state.
| setSearchQuery(val); |
| searchParams.delete('search'); | ||
| setSearchParams(searchParams); |
There was a problem hiding this comment.
When clearing the search parameter, the code mutates the existing searchParams object by calling searchParams.delete('search') before passing it to setSearchParams. While this works, it's modifying a URLSearchParams object that React manages.
A cleaner approach would be to create a new URLSearchParams object:
const newParams = new URLSearchParams(searchParams);
newParams.delete('search');
setSearchParams(newParams);or simply use setSearchParams({}) to clear all params, or avoid the mutation pattern.
| searchParams.delete('search'); | |
| setSearchParams(searchParams); | |
| const newParams = new URLSearchParams(searchParams); | |
| newParams.delete('search'); | |
| setSearchParams(newParams); |
| const open = Boolean(anchorEl); | ||
| const navigate = useNavigate(); | ||
| const { isAuthenticated, logout, user } = useAuth(); | ||
| const [headerSearch, setHeaderSearch] = useState<string>(""); |
There was a problem hiding this comment.
The Header's headerSearch state is not synchronized with the URL search parameter. When a user navigates to the events page with a search query (e.g., from a bookmark or direct link to /events?search=test), the Header's search input will be empty instead of showing "test".
Consider using useSearchParams and a useEffect in the Header component to sync the headerSearch state with the URL parameter when the location changes, similar to how it's implemented in EventsPage.tsx (lines 125-131).
daniel-buta
left a comment
There was a problem hiding this comment.
The header search bar works well now and gives the right events based on the search.
Great work.
The search bar in the header now works properly. When you search for an event and hit 'enter', it sends you to the events page and shows you events that match with your search.
closes #145