Home updating-veil rework: change-triggered, settle-driven, with refresh feedback #114

Merged

bvandeusen merged 4 commits from dev into main 2026-07-31 23:32:06 -04:00

Conversation 0 Commits 4 Files Changed 9

+1059 -121

bvandeusen commented 2026-07-31 23:31:54 -04:00

Owner

Copy Link

Copy Source

Android-only. No server, web, workflow or schema changes — android/ plus one build.gradle.kts test-logging tweak. Closes issue #2327 (arose from #1486).

Operator report: the "Updating your mixes…" veil "swipes on and off before the update completes in most cases", and "there's still a number of other times where the contents will visibly update without the veil triggering."

Why it lowered early — three independent causes

1. The 500ms delay measured the wrong thing. refreshIndex() writes only the section id lists; each tile's AlbumRef/ArtistRef hydrates afterward via MetadataProvider (null → skeleton → album), and cover art loads after that. The veil's refresh().join() + 500ms expired before the visible work began.
2. Overlapping triggers cleared a shared Boolean. Reconnect and playlist.system_rebuilt routinely arrive together (SSE reconnects, then delivers the rebuild). Whichever pull finished first flipped updatingInternal false in its finally while the other was still running.
3. Failure was invisible. refresh() swallowed everything in runCatching, so join() returned "fine" after a failed pull — no retry, no notice.

Why content changed unveiled

replaceSection was delete-then-insert, per section, un-transacted — so observeBySection genuinely emitted emptyList() (a visible collapse) before the new ids landed, seven times in sequence. Even unchanged tiles flickered. Plus init { refresh() }, manual pull, and scan.run_finished were never veiled at all.

What changed

Stop generating the churn — the biggest win, and it means the veil now has far less to hide:

• One @Transaction swap across all seven sections. Room notifies observers only on commit, so the empty gap is never observed. Same fix CachedQuarantineDao already carried for the same stated reason.
• distinctUntilChanged on the id list ahead of flatMapLatest, so an unmoved section no longer tears down and rebuilds every tile's hydration flow. Compares ids, not rows — fetchedAt is restamped on every write.

A veil driven by the screen, not a timer — new UpdateVeilController: raise → work (with retries) → hold until the content signature has been quiet for a window and nothing is still loading; floored so it can't flash, ceilinged so it can't strand (the ceiling caps the veil, never the work). One conflated-channel consumer, so overlapping triggers extend a single session instead of racing.

Raised only when content actually changed. A refresh returning what's already cached raises nothing — no more veil on every launch. Baseline for "changed" 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. Trade-off: the veil arrives one emission after the change, so a single atomic swap shows through — everything messier behind it (hydration, then artwork) stays covered.

Artwork, the operator's headline case: ServerImage — the one choke point behind every album/artist/playlist cover — reports in-flight loads to an ArtSettleTracker the veil waits on. Art also crossfades app-wide now (set once on the ImageLoader; Coil skips it for memory-cache hits), with the placeholder fading out over the same window.

Refresh feedback. Since an unchanged refresh raises no veil, a manual pull that changed nothing would produce no visible response — indistinguishable from broken. Sessions now report CHANGED / UNCHANGED / FAILED; Home surfaces "Already up to date" or "Couldn't check for updates", only for refreshes a person asked for. Background outcomes stay silent — "Already up to date" on every launch would be worse than silence.

Triggers widened to scan.run_finished (Home never reacted to it before), the playlist.* kinds, manual pull, and the initial load over a warm cache.

Also

• testLogging { exceptionFormat = FULL } on the test task. Gradle was printing coroutine-test failures as bare AssertionError at Foo.kt:12 pointing at the runTest line — assertions inside the lambda live in a generated class it filters out — which left nothing to debug from in CI.
• refreshError is cleared on success rather than at the start of each attempt; with retries, clearing up front made a failing cold start flash the "Welcome to Minstrel" empty state between attempts.
• HomeViewModel.refresh() removed in favour of retry() + refreshFromPull(). Other screens' refresh() are different ViewModels, untouched.

Verify on device

Android-only and almost entirely about feel, so this is the part CI can't sign off:

• Launch over a warm cache → content appears immediately, no veil, no flicker. (Previously: sections collapsed and refilled.)
• Pull to refresh, nothing changed → "Already up to date".
• Pull to refresh after the 03:00 rebuild → veil wipes on, holds until tiles and artwork have landed, then wipes off. No pop-in after it lifts.
• Pull with the server unreachable → "Couldn't check for updates" after the retries; cached content stays put.
• Cold start, empty cache → skeleton as before, no veil.

Known, deliberate

FreshnessSweeper re-fetches stale metadata in the background, so one tile can still change unannounced. Left unveiled on purpose — a full-screen panel for a single tile would be worse than the pop, and the new crossfade softens it.

10 unit tests on the controller. dev CI green on 3acac985 (run 3177).

🤖 Generated with Claude Code

Android-only. No server, web, workflow or schema changes — `android/` plus one `build.gradle.kts` test-logging tweak. Closes issue #2327 (arose from #1486). Operator report: the "Updating your mixes…" veil *"swipes on and off before the update completes in most cases"*, and *"there's still a number of other times where the contents will visibly update without the veil triggering."* ## Why it lowered early — three independent causes 1. **The 500ms delay measured the wrong thing.** `refreshIndex()` writes only the section *id lists*; each tile's `AlbumRef`/`ArtistRef` hydrates afterward via `MetadataProvider` (null → skeleton → album), and cover art loads after *that*. The veil's `refresh().join() + 500ms` expired before the visible work began. 2. **Overlapping triggers cleared a shared Boolean.** Reconnect and `playlist.system_rebuilt` routinely arrive together (SSE reconnects, then delivers the rebuild). Whichever pull finished first flipped `updatingInternal` false in its `finally` while the other was still running. 3. **Failure was invisible.** `refresh()` swallowed everything in `runCatching`, so `join()` returned "fine" after a failed pull — no retry, no notice. ## Why content changed unveiled `replaceSection` was delete-then-insert, **per section, un-transacted** — so `observeBySection` genuinely emitted `emptyList()` (a visible collapse) before the new ids landed, seven times in sequence. Even *unchanged* tiles flickered. Plus `init { refresh() }`, manual pull, and `scan.run_finished` were never veiled at all. ## What changed **Stop generating the churn** — the biggest win, and it means the veil now has far less to hide: - One `@Transaction` swap across all seven sections. Room notifies observers only on commit, so the empty gap is never observed. Same fix `CachedQuarantineDao` already carried for the same stated reason. - `distinctUntilChanged` on the id list ahead of `flatMapLatest`, so an unmoved section no longer tears down and rebuilds every tile's hydration flow. Compares ids, not rows — `fetchedAt` is restamped on every write. **A veil driven by the screen, not a timer** — new `UpdateVeilController`: raise → work (with retries) → hold until the content signature has been quiet for a window *and* nothing is still loading; floored so it can't flash, ceilinged so it can't strand (the ceiling caps the veil, never the work). One conflated-channel consumer, so overlapping triggers extend a single session instead of racing. **Raised only when content actually changed.** A refresh returning what's already cached raises nothing — no more veil on every launch. Baseline for "changed" 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. Trade-off: the veil arrives one emission after the change, so a single atomic swap shows through — everything messier behind it (hydration, then artwork) stays covered. **Artwork**, the operator's headline case: `ServerImage` — the one choke point behind every album/artist/playlist cover — reports in-flight loads to an `ArtSettleTracker` the veil waits on. Art also crossfades app-wide now (set once on the `ImageLoader`; Coil skips it for memory-cache hits), with the placeholder fading out over the same window. **Refresh feedback.** Since an unchanged refresh raises no veil, a manual pull that changed nothing would produce *no* visible response — indistinguishable from broken. Sessions now report CHANGED / UNCHANGED / FAILED; Home surfaces "Already up to date" or "Couldn't check for updates", **only** for refreshes a person asked for. Background outcomes stay silent — "Already up to date" on every launch would be worse than silence. **Triggers widened** to `scan.run_finished` (Home never reacted to it before), the `playlist.*` kinds, manual pull, and the initial load over a warm cache. ## Also - `testLogging { exceptionFormat = FULL }` on the test task. Gradle was printing coroutine-test failures as bare `AssertionError at Foo.kt:12` pointing at the `runTest` line — assertions inside the lambda live in a generated class it filters out — which left nothing to debug from in CI. - `refreshError` is cleared on success rather than at the start of each attempt; with retries, clearing up front made a failing cold start flash the "Welcome to Minstrel" empty state between attempts. - `HomeViewModel.refresh()` removed in favour of `retry()` + `refreshFromPull()`. Other screens' `refresh()` are different ViewModels, untouched. ## Verify on device Android-only and almost entirely about *feel*, so this is the part CI can't sign off: - **Launch over a warm cache** → content appears immediately, no veil, no flicker. (Previously: sections collapsed and refilled.) - **Pull to refresh, nothing changed** → "Already up to date". - **Pull to refresh after the 03:00 rebuild** → veil wipes on, holds until tiles *and* artwork have landed, then wipes off. No pop-in after it lifts. - **Pull with the server unreachable** → "Couldn't check for updates" after the retries; cached content stays put. - **Cold start, empty cache** → skeleton as before, no veil. ## Known, deliberate `FreshnessSweeper` re-fetches stale metadata in the background, so one tile can still change unannounced. Left unveiled on purpose — a full-screen panel for a single tile would be worse than the pop, and the new crossfade softens it. 10 unit tests on the controller. `dev` CI green on `3acac985` (run 3177). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

bvandeusen added 4 commits 2026-07-31 23:31:54 -04:00

fix(home): hold the updating veil until Home actually settles — #2327 ... 5044e7a055

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>

ci(android): print full assertion messages for failing tests ... 4f99b42844

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>

test(home): drive the veil tests' clock explicitly, not advanceUntilIdle ... d3b40342b4

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>

feat(home): veil only when content changed; tell the user when it didn't — #2327 ... 3acac985cd

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>

bvandeusen merged commit 0cea82984c into main 2026-07-31 23:32:06 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

deps

Dependency update (Renovate)

deps:major

Major dependency bump — held for manual review

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate-bot bvandeusen

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bvandeusen/minstrel#114