Honor {fetch: false} in dataPool.getDaily (#1087) - #1088
Open
gangster wants to merge 1 commit into
Open
Conversation
The Favorites tab passes {fetch: false} to getDaily, intending to read
from the in-memory cache only, but the guard tested for === false and
so ran the fetch in exactly the case it was asked not to. Because the
call sits inside an awaited for loop over every stored favorite, that
produced one serialised events.php request per favorite on each visit.
Inverting the comparison makes a cache miss return undefined, which
favorites.js already handles: the stored copy is left alone and the
list renders from local storage.
Note this also removes the incidental refresh those requests were
doing. Responses are filtered through pick() either way, so no field
is lost, but values saved alongside a favorite are no longer updated
in the background. That matches the documented intent: pick() notes
the data "might be stale", and CalVue.md still lists a refresh helper
and a staleness disclaimer as open items.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1087.
favorites.jspasses{fetch: false}todataPool.getDailyto read from the in-memory cache only, but the guard tested=== false, so it ran the fetch in exactly the case it was asked not to:Because that call sits inside an awaited
forloop over every stored key, the result was one serialisedevents.phprequest per favorite, every time the Favorites tab was opened.Verification
Favorited 4 rides through the UI, then loaded
/events/favoritesfresh so the in-memorycaldaily_mapstarted empty (an in-session cache hit masks the bug), withfetchinstrumented to record every API call.events.php?id=requestsBefore:
?id=6,?id=1,?id=2,?id=8taking 11, 2.1, 1.9 and 1.9 ms, starting at +0, +11.3, +13.5 and +15.5 ms. Each start lines up with the previous request finishing, confirming they were serialised rather than just issued in quick succession.After: no requests, and all four favorites still render with the correct titles, dates and times from local storage — 4 stored keys producing 4 cards across 4 distinct dates, including two occurrences of the same series.
What this changes
Worth stating plainly, since it is the one judgement call in here: those requests were not doing nothing.
updateStorageruns the response back throughpick(), so the fetch could never add a field the stored copy lacked, but it did refresh values — a ride cancelled or retimed after being favorited was picked up on the next visit. After this change a favorite shows what it showed when it was saved, until you open it.That reads as the intended design rather than a regression:
pick()'s comment: "doesn't store newsflash: there's no fast refresh; it might be stale."docs/CalVue.mdstill lists "a disclaimer about opening each favorite to see the latest information" and "future: server helper to quick update favorite status" as open items.If you would rather keep refreshing, this fix is still the right starting point — the refresh then wants to become deliberate and batched, instead of one awaited request per favorite on the critical path.
Also worth knowing
getDaily(id, {fetch: true})previously matched neither branch and returnedundefined. Nothing calls it that way, so it was latent; this change makes it behave as the name suggests.No tests here — there is no frontend test harness in the repo. Verified manually as described above;
npm test(backend) is unchanged at 53/53.