Fix generic qf not showing qs from user context on load#1936
Conversation
Also adds a few tests to check this in future.
| const searchAndUpdateURL = useCallback(() => { | ||
| setPageCount(1); | ||
|
|
||
| debouncedSearch.cancel(); |
There was a problem hiding this comment.
This isn't strictly part of the fix for this PR, but this kills off stale requests before they update state later which fixes a lot of random inconsistency with tests! :)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1936 +/- ##
==========================================
+ Coverage 42.23% 42.47% +0.24%
==========================================
Files 575 575
Lines 24479 24500 +21
Branches 8087 8083 -4
==========================================
+ Hits 10339 10407 +68
- Misses 13474 14047 +573
+ Partials 666 46 -620 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const initialSearchStages = params.stages | ||
| ? arrayFromPossibleCsv(params.stages) as STAGE[] | ||
| : isAda || isSubjectSpecificQF | ||
| ? getSearchStagesFromAccountSettings(user, pageContext) | ||
| : []; | ||
|
|
||
| const initialExamBoards = params.examBoards | ||
| ? arrayFromPossibleCsv(params.examBoards) as ExamBoard[] | ||
| : isAda | ||
| ? getSearchExamBoardsFromAccountSettings(user) | ||
| : []; |
There was a problem hiding this comment.
These consts are being recalculated on every re-render, so when a filter is changed, the params change and so do these values. In turn, this retriggers the updateFiltersFromAccountSettings useEffect. This is creating some weird effects such as clicking "Clear all filters" resetting filters to account settings rather than actually clearing them.
These should probably be using either a useRef or useMemo so it's truly just initial values on mount (I don't have a strong preference for which - annoyingly we seem to be slightly inconsistent in situations like this across the codebase).
There was a problem hiding this comment.
Ah, yeah, I moved the initial ones into the updateFilters useEffect deps to stop the exhaustive deps warning, but that wasn't stable :/
I'm not sure I ever looked much into when to use which. This was a good read. I think in this case, these values:
- should exist only as the initial ones; never update
- are never used in rendering
so ref makes sense here. A useful side-effect of this is no longer having exhaustive deps warnings around this, which we wouldn't get if we used useMemo (since initialSearchStages is dependent on params.stages, which we would necessarily need to ignore).
Also adds a few tests to check this in future.
In as brief as I can, the
userobject is sometimes available on load (if you came via soft redirect), and sometimes not (if not). This means our originalpopulateFiltersFromAccountSettingsfunction was not running on soft reload, because theuserobject had not changed (thus not triggering the useEffect).I have fixed this by pulling the logic from
populateFiltersFromAccountSettingsinto external functions, which are also used in the initial values forsearchStagesandsearchExamBoardsprovided the user is available then. This means that no matter when the user is available, we always run the same logic.