1568 persist values in the react encounter search filter form#1588
Conversation
JasonWildMe
left a comment
There was a problem hiding this comment.
Thank you @jbmort!
We're leveraging a pair of bot-overlords for code review right now to help us make sure we don't drop meaningful open source commits like this one. Together they flagged these issues. Would you be able to address them?
🔴 High — deep-link / URL-param searches now run unfiltered
getAllSearchParamsAndParse.js populates only formFilters (via addFilter) for ?username=, ?state=, ?individualIDExact=.
But this PR changed the query binding in EncounterSearch.jsx:72 from queries: store.formFilters → queries:
store.appliedFilters, and the URL path never sets appliedFilters. So a direct link like /search?state=... shows the
filter in the panel but returns all encounters unfiltered. This is a regression vs. main (where the query read
formFilters). (Codex caught this; I confirmed the binding change + that the URL path never applies.)
Fix: after parsing URL filters, call store.applyFilters() (or otherwise sync into appliedFilters) so they actually
run.
🔴 High — applying an empty filter set leaves stale storage
EncounterFormStore.js:266 setFiltersInSessionStorage() only writes when appliedFilters.length > 0. Repro: apply
filters → remove them all → Search (results correctly empty-filter) → refresh → the old filters come back because
storage was never cleared and getFiltersFromStorage() reloads them. Silently reverts user intent. (I rated this
Medium; Codex rated High — I agree it's High because it reverses an explicit user action.)
Fix: in applyFilters()/setFiltersInSessionStorage(), removeItem (or persist []) when appliedFilters is empty.
🟡 Medium — searchQueryId branch is inconsistent with the new split
getAllSearchParamsAndParse.js:32 raw-parses sessionStorage.getItem("formData") (duplicating the magic string instead
of store.FILTER_STORAGE_KEY) and sets only formFilters, and its JSON.parse is unguarded — malformed storage throws
here, outside the store's try/catch loader.
Fix: route through one validated store hydration method; explicitly clear or populate appliedFilters for the
saved-search case.
🟡 Medium — getFiltersFromStorage doesn't check Array.isArray
EncounterFormStore.js:280 guards parsed && parsed.length > 0 but not the type. Stored "abc" (length 3) passes, gets
assigned to the filter arrays, then useFilterEncounters can blow up on queries.filter. Low likelihood (only this code
writes the key) but cheap to harden.
Fix: require Array.isArray(parsed), else ignore + removeItem.
🟢 Low — SideBar.jsx:12 default queryID = [] is truthy
An empty array is truthy, so if the prop is ever omitted, num() returns 1 and the query-ID branch renders. Harmless
today (sole caller passes queryID={false}) but a latent footgun — looks like an accidental leftover from removing the
old tempFormFilters = [] default.
Fix: default to false/null.
🟢 Low — test gaps
Neither test covers the two High bugs. Missing: empty-apply clears storage; resetFilters removes the key;
non-array/malformed storage is ignored; and EncounterSearch.test.js only asserts getFiltersFromStorage is called, not
that restored/URL-parsed filters actually reach useFilterEncounters via appliedFilters.
…ters after parsing
… no queryID is present
|
I believe I've now addressed all those fixes and included test coverage for the major holes the bot caught there. |
|
Thank you @jbmort. I think all 6 were addressed. Awesome! Two issues emerged in a next round review. Issue B might need a runtime test to confirm, but does it look like an issue to you? I can definitely test on one of our QA servers, but I might need instructions from you on how to reproduce it (if it's really an issue) Thank you for helping us! 🔴 B (likely a crash — needs a runtime confirm) — SideBar renders filter objects as React children SideBar.jsx:114 — {store.appliedFilters.map((filter, index) => {filter})}. appliedFilters Why it's new: the base rendered tempFormFilters here, and per your own finding #5 that prop was being retired 🟠 A — loading a non-filter URL param can wipe persisted filters Mount effect order (verified): the helperFunction effect (EncounterSearch.jsx:47) runs before the |
|
I dug into those issues that popped up and I added some protection on A so that applyFilters() only runs if there are changes. B doesn't seem to be an actual issue on my end. The sidebar component passes the objects to the chip component, which utilizes the object properties. It all seems to be running smoothly for me. I also noticed the sidebar is not acknowledging when there is a search query applied. So i made some changes there to bring that back.
The QueryId changes are a little out of the scope of this issue, but will hopefully create a better user experience for those who utilize the sharing of search results through copying the search query ID. Let me know if this should be in a separate issue, and I can undo those last changes to keep this PR more focused on the issue at hand. |
When viewing a result loaded from a shared searchQueryId URL param, the Copy button copied encounterData.searchQueryId — the ID freshly generated by the background filter query running on the viewer's own session filters, not the ID being viewed. This mismatched the "Search Query ID X applied" banner. Copy queryID (the URL-loaded ID) when present, falling back to the generated searchQueryId in normal filter-search mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
I updated the filter, table, and "Applied Filters" sidebar to manage state across page refreshes and reloads.
Values for the applied filters are now stored in session storage to preserve them after the filters are applied, and refreshes the current mobX handling of filter values on reload. I also added a few unit tests to verify the functionality. If a user wants a clear set of filters, then the reset button still works as expected and clears values from storage, so they will not go back to the old values if the page reloads. Filter values will still be lost if the page is closed though.
PR fixes #1568