Conversation
The glass background sometimes never appeared until the tab was refreshed. Three things conspired, all on a slow link. Every fresh pick carried a t=Date.now() cache-buster, a leftover from source.unsplash.com which returned a random photo per request. The curated ids that replaced it never change, so the param only made each pick a URL nobody had ever requested: a full CDN miss and an origin transform, measured at three times the latency of the cached form. A timeout was treated as a dead image. The stored photo was re-verified with an 8s limit, cancelled if it missed, and replaced by up to three more cold picks at 8s each — then the page's own preload gave up at 5s. A good image that would have landed at 9s was thrown away at every step, and the tab sat on the gradient with nothing left to retry. A refresh worked because the first attempt had warmed the CDN. Both total-failure paths logged at warn, which is invisible at the shipped level, so none of this left a trace. A timeout now means slow, not dead: the request is left running, the stored photo is kept, no rival download is started, and the page waits it out — the reveal deadline already unblocks the UI. Only an error replaces an image. Fixes ENG-116
Returning a still-loading URL from loadImageWithFallback on timeout broke a contract three callers relied on. refreshBackground persisted it and the refresh button applied it with no preload, so a slow pick blanked the current photo to the gradient and credited a photographer nobody could see. A pick that was slow to fail was persisted for the day and then kept on every tab, because timeouts now meant keep. Focus mode queued it as if it were cache-warm. The keep-on-timeout rule was right for the stored photo, which was verified when stored, and wrong for a fresh one, which was not. A fresh pick now gets one generous budget and is handed back only once it loads; a timeout ends the search without starting a rival beside it, and the download continues to warm the cache for the next tab. Also guards the final apply against a slow load landing on top of a later refresh — a window the longer timeout had widened from nil to a minute — and stops logging a failure for a load the user had already switched away from.
The apply guard compared URLs claimed after each writer's own await, so a request that started during that await could not supersede it, and each one overwrote whatever the other had claimed meanwhile. A refresh in flight when the category changed showed the old category's photo under the new setting and suppressed the new load's own failure log — a case main got right by letting the last write win. A monotonic id claimed before the first await settles it. A fresh pick that merely outlasted its budget threw "all image sources failed", which every downstream log repeated and which pointed diagnosis at a blocked CDN. It now says the pick is still loading, with the timeout as its cause, and a genuine total failure names every pick it tried. Focus mode's own image layer kept the previous photo on error without a word, which the stored-keep rule made reachable; it says so now.
The onerror log added to focus mode's image layer wrote the URL as given, and in focus mode that URL is the user's own picture as a data URL whenever a custom background is set — a corrupted stored value would have put its base64 in the console at the shipped level. App.tsx already masked the same case two files away; both now go through one helper so they cannot drift apart. A refresh that interrupted an effect load and then came back empty kept the claim, so the load it displaced was dropped on success and unlogged on failure. It hands the claim back now. The abandoned-pick error names the URL, so a slow-link report can say which photo, and the docs drop a "null only when every source fails" that stopped being true once a single timeout ended the search.
Gating on "has a newer request started" dropped a load that finished while a refresh was still out, then handed the claim back to a load that had already gone. Gating on "has a newer request already applied" lets the earlier result land and the later one overwrite it, which is what the screen should show in either order, and needs no handback. Leaving glass counts as a request that applied nothing, so a refresh still out cannot land after it.
Every refresh test had another request bump the counter first, so a refresh that borrowed the previous id instead of claiming its own was dropped by the gate on an idle page — the button silently did nothing — and the whole suite stayed green. Also pins the one landing order the overlap tests had left out.
The 8s on a stored photo no longer decides anything — a timeout keeps it — so it bounds how long callers wait before the URL is handed back still loading. A named predicate says so and removes a catch whose tail was deliberately empty. The fresh-pick loop drops a Math.min against a catalog shape that does not exist. The overlap tests advanced fake timers by thirty to forty seconds, firing a few hundred component interval ticks per test for no reason; the delays only have to be ordered. Both unsplash mocks spread the real module instead of hand-copying exports, which also retires four isUnsplashUrl lines that no longer reached the code under test.
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.
The glass background sometimes never appeared until the tab was refreshed. Reported as "maybe a slow server"; the slow part is real but was largely self-inflicted.
Root cause
Three things conspired on a slow link:
Every fresh pick carried a
t=Date.now()cache-buster — a leftover fromsource.unsplash.com, which returned a random photo per request. The curated ids that replaced it in 2024 never change, so the param only made each pick a URL nobody had ever requested. Measured against the CDN:x-cachet=t=A timeout was treated as a dead image. The stored photo was re-verified with an 8s limit, cancelled if it missed (
img.src = ''), and replaced by up to three more cold picks at 8s each — then the page's own preload gave up at 5s. A good image that would have landed at 9s was thrown away at every step, and the tab sat on the gradient with nothing left to retry. A refresh worked because the first attempt had warmed the CDN.Both total-failure paths logged at
warn, invisible at the shipped level, so none of this left a trace.Fix
The rule: a timeout means slow, an error means dead; only dead images are replaced — but it applies differently to the two kinds of photo.
preloadImageleaves the request running on timeout and rejects with a distinguishableImageLoadTimeoutError.App.tsxwaits up to 60s for the final apply — the reveal deadline already unblocks the page at 1.5s, so this only has to outlast the link — and a result applies only if nothing newer has already applied, so a refresh and an effect load can overlap in either order.error, name the URL, and never log a custom background (a data URL of the user's own picture) — through one helper shared by both sites.Review
Five rounds ran, and each found a hole in the previous round's fix — including a regression vs
mainin round 2 and a privacy leak I introduced in round 3. The commits tell the story; the branch is squash-merged, so the history is here rather than inmain.Two tests were deliberately replaced rather than kept: one asserted the defect as intended behaviour, and one had put the slow part of a race in the wrong place and passed for the wrong reason.
Verification
2049 app tests, lint and type-check clean. Mutation-checked: reverting the cache-buster, the no-cancel timeout, the stored-keep rule, the verified-or-throw contract, the apply gate, the custom-background masking, or the plain-refresh increment each fails a test.
Not in this PR
Pre-existing, noted for follow-up: the first tab of the day on a link slower than ~10KB/s still cannot converge on a stored photo (the picker is random per tab); a toast on refresh failure; four surviving mutants on guards that are currently double-covered.
Fixes ENG-116