fix(weather): date every reading by one rule - #337
Merged
kYem merged 6 commits intoAug 31, 2026
Merged
Conversation
lastFetch was parsed at four sites with three different policies for an unusable stamp. Only the load boundary rejected one from the future, so a clock stepping back mid-session left the reading frozen while the age line called it "Updated just now". weatherAgeMs is now the single answer to how old a reading is: null when the stamp is unparseable or far enough ahead that our clock must have stepped, clamped to zero within the skew tolerance so a fresh write survives jitter. formatWeatherAge, isStale and isTimestamp all collapse onto it. useStaleRefresh keeps its own arithmetic — it is deliberately weather-agnostic — but no longer reads a future stamp as fresh, which is what left the reading frozen rather than merely mislabelled. Fixes ENG-111
The guard added for a backward clock step was defeated by the line under it. retriedRecently subtracted an attempt clock that had taken the same step, so a negative elapsed time read as "retried recently" and stood the refresh down for the length of the step plus a window — 65 minutes for a one-hour step. It went unnoticed because the test rendered a fresh hook, whose attempt clock is zero. useStaleRefresh now dates the reading through weatherAgeMs rather than its own zero-tolerance comparison, so the 60s skew tolerance no longer differs between the hook and the store: a stamp seconds ahead was fresh to one and stale to the other, which is the drift this refactor set out to remove. Also pins the tolerance boundary, which was free to move anywhere in [30s, 30min) with the suite green, and drops a doc claim that every caller went through the shared helper — the hook did not, until now.
A stamp the clock stepped back past left formatWeatherAge returning null, and the popover renders its footer only when it has an age or an error to put there. So the one reading least worth trusting was the one shown with nothing qualifying it at all — temperature, condition and forecast, undated. It now says so instead; null is reserved for having no reading to date. The retry guard added with the future-stamp fix took no skew tolerance, so any sub-minute correction reopened a window meant to stay shut for thirty minutes. It now uses the tolerance the reading already gets, which is what its comment claimed in the first place. Pins the attempt clock re-arming after a backward step: rewriting it as Math.max(now, previous) — the obvious tidy-up — survived all seventeen hook tests while turning one refresh into twelve over ten minutes.
lastAttemptRef only ever measures elapsed time between attempts, and wall time is the wrong instrument for that: a backward step made the gap negative, which first read as "retried recently" and stood the refresh down for the length of the step, then — once guarded — granted a bonus retry instead. Both were the same mistake from opposite ends. performance.now() is monotonic, so the once-per-staleMs bound the docstring advertises now holds unconditionally, in either direction. That deletes the skew tolerance from the hook, the negative branch, and the comment explaining it, and closes a third case: the popover's own Refresh advances lastFetch without stamping the attempt clock, so the two could drift far enough to suppress a genuinely stale reading for a full window. Also logs the clock step where it is detected. The branch that cannot fire in production was instrumented while the one this fix exists for was not, so a step that self-healed left no trace anywhere.
performance.now() does not advance while the machine is suspended on macOS or Linux, so measuring the retry window on it alone withheld the refresh for the whole nap: a lid closed overnight woke to a limiter that thought sixty seconds had passed, and sat on eight-hour-old weather until half an hour of awake time went by. That is the case the hook exists for — its own doc cites sleep. Neither clock measures elapsed time alone. Sleep freezes the monotonic one while wall time runs on; a backward step does the reverse. Recording both and suppressing only when they agree leaves a step unable to buy a retry and sleep unable to withhold one. Also pins the clock-step log, which nothing asserted: removing it, or letting it fire every minute, passed the whole suite. And the two window boundaries, where <= silently became <.
The note on the attempt ref still described the single monotonic value it used to hold. Against the pair, a zero sentinel reads as "just attempted" on one clock and "never attempted" on the other, and since suppression needs both, the window it warned would start spent would in fact start open. The comment on the gates also inverted them: the code suppresses when both clocks call the attempt recent, not when both call the window unspent — a sense of "spent" opposite to the one the line above it used.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
lastFetchwas parsed at four sites with three different policies for an unusable stamp. Only the load boundary rejected one from the future, so a clock stepping back mid-session left the reading frozen while the age line called it "Updated just now".The rule
weatherAgeMs(lastFetch, now?)inpackages/shared/src/weather-utils.ts, besideWEATHER_STALE_MS, is now the single answer to how old a reading is —nullwhen the age is unknowable (no stamp, unparseable, or more than 60s ahead), clamped to0within that tolerance so jitter around a fresh write does not discard the reading.formatWeatherAge,isTimestamp, the store's staleness check anduseStaleRefreshall derive from it. The hook keeps one deliberateDate.parseof its own, purely to split "unparseable" (stand down forever) from "future" (keep retrying) — a distinctionweatherAgeMscollapses into onenull. That exception is documented where it lives.User-visible change
A reading whose age cannot be established now reads "Updated at an unknown time" instead of vanishing. The popover renders its footer only when it has an age or an error, so returning
nullthere left a temperature, condition, hi/lo and forecast strip on screen with nothing dating any of it — the one reading least worth trusting shown with no qualification.nullis now reserved for having no reading at all.Retry window
The staleness hook's rate limiter records both a monotonic and a wall clock, and suppresses only when both agree the last attempt was recent. Neither alone measures elapsed time: system sleep freezes the monotonic clock while wall time runs on, and a backward clock step does the reverse. So a clock step cannot buy a bonus refresh, and a night's sleep cannot withhold one.
Review notes
Five rounds of review ran over this branch, and each round found a defect introduced by the previous round's fix. Worth knowing when reading the history:
performance.now()does not advance while suspended on macOS and Linux. Hence the dual clock.Two tests were deliberately replaced rather than kept:
'treats a future timestamp as just now'asserted the defect as intended behaviour, and the earlier tolerance-boundary test named a tolerance the hook no longer has.Verification
2027 app tests, 460 shared tests, lint and type-check clean. Mutation-checked: reverting the monotonic clock, the dual-clock AND, the log guard, or the attempt-clock re-arm each fails a test.
Deferred
Pre-existing, not introduced here, worth their own ticket:
initializerefreshes on every widget mount with no rate limit (which bypasses this bound on route changes); the hook's refs are per-mount so a remount resets the budget;CalendarStripstill runs its ownlastSyncrule; andisTimestampis now a misleading name, since it rejects valid future timestamps.Fixes ENG-111