Home updating-veil rework: change-triggered, settle-driven, with refresh feedback #114
Merged
bvandeusen
merged 4 commits from 2026-07-31 23:32:06 -04:00
dev into main
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bvandeusenandClaude Opus 5
|
3acac985cd |
feat(home): veil only when content changed; tell the user when it didn't — #2327
android / Build + lint + test (push) Successful in 4m8s
The veil raised eagerly: any trigger over a warm cache put it up before knowing whether the refresh would change anything. So every launch cost ~1-2s of opaque panel even when the pull returned exactly what was already cached — which, now that the section swap is atomic and the index flow dedups on ids, produces no visible churn to hide at all. The veil was covering nothing and only delaying first paint. The raise is now reactive: it fires when the content key actually differs from what was already on screen, and never for a no-op refresh. The baseline is the first state that HAS content, not the first state at all — over a warm cache the cached rows paint a moment after the session starts, and counting that first paint as "a change" would veil every launch, which is the thing being fixed. Cost of reacting rather than anticipating: the veil arrives one emission after the change, so a single atomic swap shows through. Everything messier that follows it — tile hydration, then artwork — still lands behind it. That leaves a hole this closes too: a manual pull where nothing changed would now produce no veil, no movement, nothing whatsoever, which reads as broken. So sessions report an outcome — CHANGED / UNCHANGED / FAILED — and Home surfaces it as "Already up to date" or "Couldn't check for updates". Only for refreshes a person actually asked for. "Already up to date" on every launch, every 03:00 rebuild and every reconnect would be worse than silence, so VeilSessionResult carries a userInitiated bit and background sessions stay quiet. The bit is tracked separately from the request token because the request channel is CONFLATED: coalescing drops the older token, and a user's pull must not be swallowed by a background trigger arriving on its heels. The surfaced failure is a deliberate narrowing of the earlier "silent on give up" call, which is now read as being about background refreshes: for a pull the user deliberately triggered, silence looks broken, and staying silent while the success case speaks would be incoherent. Recovery is unaffected either way. Pull-to-refresh now waits for whichever successor actually arrives — the veil, or the snackbar — via finishedSessions, instead of only ever waiting on the veil and timing out for 2s on an unchanged pull. The Error-state Retry goes through the controller as well, so it gets the retries and reports its outcome; over an empty cache there's no content to protect, so no veil appears. HomeViewModel.refresh() is gone, replaced by retry() and refreshFromPull() — the two things that actually exist. Tests: two changed meaning and are rewritten rather than patched. A failed pull writes nothing, so the veil no longer stands over the retries — it goes up when a retry finally lands. And "waits for content to paint" became "cached content painting is not mistaken for a change", which is the baseline subtlety above. Added coverage for UNCHANGED, FAILED, the cold-load CHANGED case, and the conflation of a user request with a background one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bvandeusenandClaude Opus 5
|
d3b40342b4 |
test(home): drive the veil tests' clock explicitly, not advanceUntilIdle
android / Build + lint + test (push) Successful in 3m54s
All seven new UpdateVeilController tests failed in CI run 3163, and the one test that passed is the tell: it was the only one that never called advanceUntilIdle(). advanceUntilIdle() advances only while *foreground* work remains. Every coroutine this controller owns lives in backgroundScope — it has to, because its consumer loop runs forever and would otherwise stop runTest from completing — so advanceUntilIdle() returned having run nothing at all, and the assertions landed on a session that never started. Hence "exhausts its attempts. Expected <3>, actual <0>" and, where an earlier advanceTimeBy had got a session partway, "retries until the pull succeeds. Expected <3>, actual <2>". Each wait is now an explicit advanceTimeBy sized for what that test still has pending, and the class KDoc says why so nobody folds them back. The drains stay deliberately under maxHoldMs. If a drain overshot the ceiling, "the veil lowered" would stop distinguishing "it settled" from "it gave up" — which is exactly what these tests exist to tell apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bvandeusenandClaude Opus 5
|
4f99b42844 |
ci(android): print full assertion messages for failing tests
android / Build + lint + test (push) Failing after 3m11s
CI run 3161 reported seven failures as bare "java.lang.AssertionError at
UpdateVeilControllerTest.kt:87" — and line 87 is the test's own `fun ... =
runTest {` line, not the assertion. Gradle picks the first stack frame
belonging to the test class, and assertions inside a `runTest { }` lambda
live in a generated suspend-lambda class that gets filtered out, so every
failure in a coroutine test collapses to the function declaration. With the
HTML report unreachable from CI, that leaves nothing to debug from.
testLogging with exceptionFormat = FULL prints the assertion message and the
whole stack trace for failures, which is what makes a coroutine-test failure
diagnosable at all here.
Also drop the NonCancellable floor-join from UpdateVeilController's finally.
Honouring the minimum hold while the scope is being torn down is pointless —
nothing is left to render the veil — and a finally that suspends is a finally
that can resist cancellation. The floor is now awaited in the try instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
bvandeusenandClaude Opus 5
|
5044e7a055 |
fix(home): hold the updating veil until Home actually settles — #2327
android / Build + lint + test (push) Failing after 3m7s
The "Updating your mixes…" veil wiped on and straight back off before the update finished, and a number of churn paths never raised it at all. Three reasons it lowered early. refreshBehindVeil held it for refresh().join() + a flat 500ms, but finishing the network pull is nowhere near the end of the visible work: refreshIndex writes only the section id lists, then each tile hydrates through MetadataProvider (null → skeleton → album), and only then does the cover art load. Second, updatingInternal was a plain Boolean cleared in a finally — reconnect and playlist.system_rebuilt routinely arrive together, so whichever pull finished first wiped the veil off while the other was still running. Third, refresh() swallowed every failure in runCatching, so join() returned "fine" after a failed pull: veil off, content unchanged, no retry. So the veil's lifetime is now driven by watching the screen instead of by a guess. UpdateVeilController raises, runs the work (retrying behind the veil), then holds until the content signature has been unchanged for a quiet window AND nothing is still loading — floored by a minimum hold so it cannot flash, capped by a hard ceiling so it cannot strand, and with overlapping triggers folded into one session rather than racing it. Giving up is silent and sets no latch: the reconnect-driven recovery and the freshness sweeper keep retrying afterwards exactly as before. Cover art was the most visible pop-in and the refresh coroutine cannot see it, so the composition reports it upward: ServerImage — the single choke point behind CoverTile for every album/artist/playlist cover — counts its in-flight loads into an ArtSettleTracker the veil waits on. Art also crossfades now (set once on the ImageLoader, so it applies app-wide) with the placeholder fading out over the same window, which softens the pop everywhere the veil isn't involved. Underneath all of it, the churn is largely no longer generated. replaceSection was delete-then-insert per section, un-transacted, so observeBySection emitted emptyList() — a visible collapse — before refilling, seven times in sequence. It is now one @Transaction across all sections (Room notifies once, on commit, so the empty gap is never observed), and the index flow dedups on the id list, so a section whose contents did not move no longer tears down and rebuilds every tile's hydration flow. fetchedAt is restamped on every write, which is why the dedup compares ids rather than rows. Same fix CachedQuarantineDao already carried for the same reason. Trigger set widened per the operator's call: the initial load over a warm cache (a full re-pull that churned every section completely unveiled), manual pull-to-refresh, scan.run_finished (Home never reacted to it at all), and the playlist.created/updated/deleted/tracks_changed kinds. The veil waits for content to be on screen before raising, so a genuinely cold load still gets its skeleton rather than an opaque panel over nothing. refreshError is now cleared on success rather than at the start of each attempt — with retries, clearing it up front made a failing cold start flash the "Welcome to Minstrel" empty state between attempts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |