Fix: Search Bar Reset #9867
Fix: Search Bar Reset #9867
Conversation
WalkthroughThe pull request introduces a modification to the Changes
Assessment against linked issues
Poem
The poem playfully captures the essence of the cache blacklisting approach, suggesting that search terms will now be more transient and less likely to persist across different tabs or navigation actions. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@rithviknishad The search bar should be cleared, and the details should be shown when we switch and come back to the same tab. In this PR, it is just clearing the search fields. Just for clarification, in the screenshot, 'er' details should be shown when we switch and come back to the same tab, and 'er' in the search bar should also be cleared. |
|
The current changes solves the problem about resetting the blacklisted fields. Not sure if I understood your question right? Do you mean when we switch between let's say, from "Name" to "Encounter ID" search, and when we switch back whether the previously searched Name should remain or disappear? Yes, only one of "Name" XOR "Encounter ID" XOR "External ID" should be applied at any given time. Not multiple of them. Hence we can reset the ones as soon as we switch away. |
|
@rithviknishad |
|
Yup
☝🏼 this is the desired behaviour |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/Encounters/EncounterList.tsx (3)
Line range hint
316-332: Consider enhancing the search field clearing UX.The current implementation clears all search fields at once. Consider these improvements:
- Add individual clear buttons for each search field
- Provide visual feedback when fields are cleared due to switching tabs
- Add a "restore previous search" option when switching back to a tab
<Button variant="ghost" size="sm" className="w-full justify-start text-muted-foreground" onClick={() => { updateQuery({ status, encounter_class: encounterClass, priority, - name: undefined, - encounter_id: undefined, - external_identifier: undefined, + [searchOptions.find(opt => opt.value)?.key]: undefined, }); - clearSearch.value = true; + clearSearch.value = { + field: searchOptions.find(opt => opt.value)?.key, + previousValue: searchOptions.find(opt => opt.value)?.value, + }; }} >
Line range hint
234-257: Improve search options accessibility and discoverability.The search options include keyboard shortcuts, but they're not easily discoverable. Consider these improvements:
- Add tooltip to show keyboard shortcuts
- Include aria labels for screen readers
- Add visual indicators for keyboard shortcuts
const searchOptions = [ { key: "name", label: "Patient Name", type: "text" as const, placeholder: "Search by patient name", value: name || "", shortcutKey: "n", + ariaLabel: "Search by patient name, press Alt+N", + tooltip: "Alt+N", }, { key: "encounter_id", label: "Encounter ID", type: "text" as const, placeholder: "Search by encounter ID", value: encounter_id || "", shortcutKey: "i", + ariaLabel: "Search by encounter ID, press Alt+I", + tooltip: "Alt+I", }, { key: "external_identifier", label: "External ID", type: "text" as const, placeholder: "Search by external ID", value: external_identifier || "", shortcutKey: "e", + ariaLabel: "Search by external ID, press Alt+E", + tooltip: "Alt+E", }, ];
Line range hint
182-196: Optimize data fetching for filter changes.The current implementation might trigger multiple API calls when changing filters. Consider these optimizations:
- Debounce filter changes
- Add loading states for individual filters
- Implement request cancellation for superseded queries
const { data: queryEncounters, isLoading } = useQuery< PaginatedResponse<Encounter> >({ queryKey: ["encounters", facilityId, qParams], queryFn: query(routes.encounter.list, { queryParams: { ...buildQueryParams(status, facilityId, encounterClass, priority), name, external_identifier, limit: resultsPerPage, offset: ((qParams.page || 1) - 1) * resultsPerPage, }, }), + staleTime: 30000, // Cache results for 30 seconds + keepPreviousData: true, // Show previous data while fetching + retry: false, // Don't retry failed requests enabled: !propEncounters && !encounter_id, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/EncounterList.tsx(1 hunks)
🔇 Additional comments (2)
src/pages/Encounters/EncounterList.tsx (2)
154-154: LGTM with suggestions for improvement.The core change to prevent caching of search fields is implemented correctly. The suggestions above would enhance the user experience but aren't blocking issues.
154-154: Verify the implementation aligns with the discussed behavior.The addition of
cacheBlacklistprevents caching of search fields, which means they will be cleared when switching between search types. However, based on the PR discussion, there seems to be uncertainty about whether search fields should be retained or cleared when switching tabs.Let's verify the current behavior:
✅ Verification successful
Implementation aligns with established codebase patterns
The
cacheBlacklistimplementation in EncounterList follows the consistent pattern used across the codebase where primary search fields are blacklisted to ensure they're cleared when switching views. This is the standard behavior implemented in other components like OrganizationFacilities, ResourceList, and FacilityUsers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other instances of cacheBlacklist in the codebase # to ensure consistent behavior across components rg "cacheBlacklist" -A 5 -B 5 # Check the useFilters hook implementation to understand # how cacheBlacklist affects the search state ast-grep --pattern 'function useFilters($_) { $$$ }'Length of output: 21316
#9851 is planning to address this in the SearchByMultipleFields btw. cc: @rithviknishad |
|
then this pr can be merged, lgtm |
|
@Jeffrin2005 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
search.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit