Fix bad save deck state caching on detail pages - #2338
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2338 +/- ##
==========================================
- Coverage 43.57% 43.56% -0.01%
==========================================
Files 601 601
Lines 25812 25827 +15
Branches 8590 7677 -913
==========================================
+ Hits 11248 11252 +4
- Misses 14515 14526 +11
Partials 49 49 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sjd210
left a comment
sjd210
left a comment
There was a problem hiding this comment.
I do need to read up more on RTKQ, but this much makes sense to me and definitely seems to work.
I noticed that subject overview pages were also not showing saved status. This was a slightly different issue (us not loading the gameboards at all), but for the sake of practice I've fixed it myself and committed that. I'd appreciate you having a look in counter-review.
jacbn
left a comment
jacbn
left a comment
There was a problem hiding this comment.
The query is great, just a little point about after it!
| return items[subject].length > 0 ? <ListView className={className} type="gameboard" items={items[subject].map(convertToALVIGameboard)} /> : null; | ||
| const gameboardByIdQuery = useGetGameboardByIdQuery(gameboard_ids[subject]); | ||
|
|
||
| return gameboard_ids[subject] |
There was a problem hiding this comment.
This works great! maintainOnRefetch is proving its value :) probably my favourite addition to the codebase...
The only thing I'd say is that it feels odd to have a query that relies on gameboard_ids[subject] being defined, then afterwards have a conditional where we don't render contents if it is undefined. The query is still being executed and can still error prior.
There are really 3 solutions here, really up to you which you prefer:
-
Have faith in TS type-checking;
subjectmust be a valid key ofgameboard_idsby its type definition. You don't really need the conditional here. -
If we are going to deal with the case where
gameboard_ids[subject]can be undefined, we'll want to make the query parameter deal with this too so it can't error prior;skipTokens are our most common way.
// skipToken needs importing
const gameboardByIdQuery = useGetGameboardByIdQuery(gameboard_ids[subject] ?? skipToken);
return gameboard_ids[subject] ? ...- As an extension to 2, if you'd rather not have two conditionals with the same condition, you can make more extensive use of the tools inside
ShowLoadingQuerywithuninitializedPlaceholder; note though that there isn't a way to automatically reuse the error message, mainly because it being uninitialised isn't always an error state.
const gameboardByIdQuery = useGetGameboardByIdQuery(gameboard_ids[subject] ?? skipToken);
return <ShowLoadingQuery
...
uninitializedPlaceholder={<>...</>}
/>;There was a problem hiding this comment.
Hrmm well looking in comparison to the book/revision detail pages, they fail a lot more gracefully by just not showing the gameboards section at all if they aren't loaded. For those pages and this one, the page as a whole is still functional without that section (unlike e.g. My Assignments where the page is fully broken so there really should be an error), so this feels sensible to me rather than alarming the user with a warning they don't really need to worry about.
It seems like a combination of uninitializedPlaceholder={RenderNothing} and adding ifNotFound={RenderNothing} handles this case best, so I'll go with that
|
LGTM |
2 participants
Uses correct
rtkqcaching tags in book detail and revision detail pages. Paired with the API etag PR, this fixes issues with not appearing to save / unsave decks on these pages.