Skip to content

fix: steady home balance header - #1729

Merged
ovitrif merged 8 commits into
masterfrom
fix/home-balance-loader
Oct 30, 2026
Merged

ovitrif merged 8 commits into
masterfrom
fix/home-balance-loader

Conversation

@ovitrif

@ovitrif ovitrif commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Twin: synonymdev/bitkit-ios#1168
Refs: #1700

Description

  • Replaces the "Updating balance…" caption under the Home total with a soft shimmer over the headline amount, because the caption pushed Savings, Spending, USDT and the activity list down and back up on every USDT balance or rate refresh.
  • The shimmer covers only the amount's digits, separators and decimal point, or the hidden-amount dots; the ₿/$/€ symbol is never dimmed or swept. The digits dim to 75% and a diagonal band brings them back to full, one pass every 2 s, fading in when a refresh starts and out when it ends, so the cached amount stays readable.
  • With animations off (animator duration scale 0), there is no sweep: the digits only dim while the balance refreshes. Context.areAnimationsDisabled in ext/Context.kt reads that setting.
  • The shimmer wraps the digits text inside LargeRow, so it moves with the BTC/fiat swap animation instead of clipping it. The band has a fixed width and its travel reaches past the text at both ends, so each pass starts and ends off the digits.
  • It takes no layout space: the header, including the eye icon, lays out exactly as on master, and nothing below it moves.
  • While refreshing, the amount carries the TotalBalanceUpdating test tag and "Updating balance…" as its state description.

Question: what should the eye icon do while the balance loads? It keeps its current behavior (shown only while amounts are hidden), and the shimmer runs over the hidden dots.

Out of Scope

Design

N/A — no design available. Figma has no state for this; the shimmer follows the spec agreed in review.

Preview

The "all" regtest wallet, restored on the hosted regtest in a dev debug build (₿ 24 242 671, about $ 18,700), on an API 36 emulator. The stills are full-resolution captures at 1008 × 2244, taken mid-sweep. The recordings are at 2x on the 504 × 1122 emulator and come from a local debug build that toggles the updating state every 7 s, so each shows refresh starts, a few sweeps and the fade-outs. The software-rendered emulator draws the sweep at a lower frame rate than a device.

Mid-sweep Refresh
Bitcoin
and-sh-btc-2x.mp4
Fiat
and-sh-fiat-2x.mp4
Hidden
and-sh-hidden-2x.mp4

Idle headline, master above this PR for each state (Bitcoin, fiat, hidden). Bitcoin and hidden are pixel-identical. For fiat, the rate refreshes on every launch, so the cents differ between the two captures, and the $ 18, part, symbol and comma, is pixel-identical.

QA Notes

Setup

Dev (regtest) debug build with default endpoints. Restore a funded regtest wallet, or create one and enable USDT on the Home intro. For the fallback, set Developer options › Animator duration scale to off. The media came from a 504×1122 AVD at adb shell wm density 210, with stills at wm size 1008x2244 and wm density 420; at the AVD's default 180 dpi the Home top bar overlaps the balance on master too.

Journeys

  • J1: updated usdt-cached-home-balance.md — TotalBalanceUpdating is a shimmer over the headline digits (a slight dim with animations off) while the symbol stays steady, it runs over hidden dots, and nothing below the headline moves when it starts or stops

Manual Tests

N/A

Automated Checks

  • added BalanceHeaderTest.kt — the amount is marked as updating (tag and localized state description) while loading and not otherwise, including over hidden dots with the eye icon kept, and the header's bounds stay the same as updating starts and stops

Evidence

  • On 54940c0, API 36 emulator, "all" wallet: the shimmer runs over the digits or dots and never over the symbol. Idle Home matches master as shown above.
  • On 54940c0: just lint and just test pass, and BalanceHeaderTest.kt (4 cases) passes on an API 36 emulator.

Gaps

  • The recordings stage the updating state; the real USDT refresh drives the same flag.

Models used

  • Planning/scoping: claude-opus-5-5 (reasoning: medium)
  • Implementation: claude-opus-5-5 (reasoning: medium)
  • Review: Not performed

@ovitrif ovitrif self-assigned this Oct 29, 2026
@ovitrif
ovitrif marked this pull request as ready for review October 29, 2026 19:20
@ovitrif
ovitrif requested review from a team, coreyphillips and pwltr and removed request for a team October 29, 2026 19:21
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T01:11:18.145888Z 54940c0 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@ovitrif
ovitrif requested review from ben-kaufman and removed request for pwltr October 29, 2026 19:21
@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no new actionable issues remain.

Summary

Replaces the Home balance’s updating caption with a shimmer, keeping the content below it steady.

  • Home shows balance updates without shifting the content below.

Reviews (3) · Last reviewed commit: "fix: shimmer only the balance digits" · Reviewed by Greptile

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Placing this PR in 3.1.0: the "Updating balance…" caption it replaces came in with #1700 and synonymdev/bitkit-ios#1131, which merged after 3.0.1 and ship in 3.1.0. Without this fix, 3.1.0 would ship the Home layout jump on every USDT refresh.

@ovitrif ovitrif added this to the 3.1.0 milestone Oct 29, 2026

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings. Moves the Home "Updating balance…" state from a caption under the total into a 24 dp loader in the eye icon's slot, so the rows below no longer shift. Reviewed 4a32fea, light tier.

What I checked, and 4 candidates I ruled out

Read in full: BalanceHeaderView.kt (the BalanceHeader row), BalanceHeaderTest.kt, the HomeScreen.kt hunk, GradientCircularProgressIndicator.kt, ClickableAlpha.kt
Call sites traced: isLoading is passed only from HomeScreen.kt; every other BalanceHeaderView/BalanceHeader caller keeps the false default, and only Home sets showEyeIcon = true
CI: build-local and Greptile green; build, lint and the e2e shards were still running at review time
Not built here: on this track an unprompted review does not build the branch, so this was read from the code; CI and the PR's instrumented BalanceHeaderTest.kt are the gate

Ruled out

  • Loader covering the eye's tap target: the loader is composed only while loaderAlpha > 0f, and its target alpha is 0f whenever hideBalance is true, so when the eye is visible the loader is either gone or fading out, and it never gets a click handler that would take the tap.
  • Layout shift from the new slot: the Spacer(weight = 1f) plus a fixed 24 dp box were already there (the old AnimatedContent was sized 24 dp even when empty), so the row's width budget is unchanged.
  • Lost hooks for tests and screen readers: TotalBalanceUpdating keeps its tag, and the string moves into contentDescription. No spec in bitkit-e2e-tests main references the tag.
  • Unused imports after removing the caption: Caption13Up import removed; Colors, stringResource and padding still have other uses in HomeScreen.kt.

Merge confidence: 4/5, no findings, but build, lint and e2e had not finished at review time.

Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (the review runner does not report an effort label)

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from dda8e2b (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

coreyphillips
coreyphillips previously approved these changes Oct 29, 2026

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two independent reviews, nothing blocking a merge.

worth doing, does not block

  • Updating indicator vanishes when no exchange rate is loaded (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:104). The indicator now only shows if the header renders, so Home shows no updating state at all when no fiat rate is loaded. Before this PR, HomeScreen drew the TotalBalanceUpdating caption as a sibling after BalanceHeaderView. It now lives inside BalanceHeader. BalanceHeaderView only calls BalanceHeader inside converted?.let { ... } (BalanceHeaderView.kt:104). currency.convert() returns null when CurrencyRepo.getCurrentRate finds no rate for the selected currency (empty rates). In that same state, usdtBalanceUpdating is true whenever USDT is visible. paykitRate is null, so runCatching { currency.paykitRate?.value(now) }.getOrNull() == null holds, and usdt.balance != 0uL is also true for a null balance. Before the PR, Home showed "Updating balance…" under an empty header. Now it shows nothing. Rates are cached in CacheStore, so this is limited to a fresh install or offline restore before the first successful rate fetch, with USDT visible. I confirmed it by reading the code, not on a device. It is a small edge, but it is the case where an updating hint matters most.
  • Tests never exercise the loader clearing after a refresh (app/src/androidTest/java/to/bitkit/ui/components/BalanceHeaderTest.kt:18-40). All three tests in BalanceHeaderTest set isLoading and hideBalance once, at first composition. animateFloatAsState starts at its first target, so the code that removes the loader is never tested. That code is the fade from LOADER_ALPHA to 0 that ends if (loaderAlpha > 0f), both when isLoading turns false and when the balance is hidden mid-load. That path is what the PR and journey step 4 depend on ("the updating indicator must disappear once both are current"). If the animation spec changed so the value never settles at exactly 0f, the node would stay in the tree for good. It would be invisible, but still announced by TalkBack and still found by TotalBalanceUpdating lookups. A cheap fix: hold isLoading/hideBalance in mutableStateOf, flip it after setContent, then waitForIdle() and assert TotalBalanceUpdating no longer exists. The name showsLoaderInEyeSlotWhileLoading also claims more than the test checks. It only asserts that the node exists, not where it is.
  • Loading feedback disappears when the fiat rate is unavailable (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:123). A wallet without a cached fiat rate now loses balance-refresh feedback if the rate endpoint is unavailable. With USDT enabled, open Home in that state: currency.convert() returns null, so converted?.let skips BalanceHeader and its loader despite balanceUpdating being true. The removed caption rendered outside this guard. This follows from the render path; device reproduction was blocked by sandbox permissions.

nits

  • Animated alpha is read in BalanceHeader's own recompose scope (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:280-292). loaderAlpha is read during composition (if (loaderAlpha > 0f) and .alpha(loaderAlpha)). This happens inside the content lambdas of Column, Row and Box, which are inline. So the restart scope is BalanceHeader itself, and the whole header recomposes on every frame of each fade. With frequent USDT and rate refreshes, that happens often. Two alternatives keep the per-frame work in the draw phase: AnimatedVisibility(visible = isLoading && !hideBalance, enter = fadeIn(), exit = fadeOut()) around the indicator, or graphicsLayer { alpha = loaderAlpha } combined with a separate visibility flag. Either would also read more like the eye's AnimatedContent next to it.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed c0cc8d3: documents the loader's opacity constant, as this repository asks for constants; no behavior change. 4a32fea added the changelog fragment.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 16bff47, answering @coreyphillips and Greptile: the loader is its own BalanceLoader with AnimatedVisibility, so the fade no longer recomposes BalanceHeader. BalanceHeaderTest.kt now flips state after composition to check that the loader leaves when loading ends or the balance is hidden, and that the header's bounds don't change (5 cases, passing on an API 36 emulator). The no-fiat-rate gap is filed as #1730. Journey step 3 now expects no loader while balances are hidden.

@ovitrif
ovitrif requested a review from coreyphillips October 29, 2026 19:57
coreyphillips
coreyphillips previously approved these changes Oct 29, 2026

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked after 2 new commits (c0cc8d3, 16bff47). Nothing blocking remains.

addressed

  • Tests never exercise the loader clearing after a refresh (app/src/androidTest/java/to/bitkit/ui/components/BalanceHeaderTest.kt:18-40). Fixed: BalanceHeaderTest.kt:27-28 now holds isLoading and hideBalance in mutableStateOf. loaderClearsWithoutMovingHeader (:57-70) flips loading on, then off, and asserts at :68 that TotalBalanceUpdating is gone. loaderClearsWhenBalanceIsHiddenMidLoad (:72-83) does the same when the balance is hidden, and the first test is renamed showsLoaderWhileLoading. Removal now relies on AnimatedVisibility disposing its content after exit, not on a float reaching 0f. I checked this by reading the code and did not run the instrumented tests. You report them passing on API 36.
  • Animated alpha is read in BalanceHeader's own recompose scope (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:280-292). Fixed: BalanceHeaderView.kt:282 now calls BalanceLoader(visible = isLoading && !hideBalance). BalanceLoader (:289-309) is a separate non-inline composable that wraps AnimatedVisibility(fadeIn(), fadeOut()). BalanceHeader no longer reads an animated value (the animateFloatAsState import is gone), so it recomposes only when isLoading or hideBalance changes.

still open

  • Updating indicator vanishes when no exchange rate is loaded (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:104). The indicator now only shows if the header renders, so Home shows no updating state at all when no fiat rate is loaded. Before this PR, HomeScreen drew the TotalBalanceUpdating caption as a sibling after BalanceHeaderView. It now lives inside BalanceHeader. BalanceHeaderView only calls BalanceHeader inside converted?.let { ... } (BalanceHeaderView.kt:104). currency.convert() returns null when CurrencyRepo.getCurrentRate finds no rate for the selected currency (empty rates). In that same state, usdtBalanceUpdating is true whenever USDT is visible. paykitRate is null, so runCatching { currency.paykitRate?.value(now) }.getOrNull() == null holds, and usdt.balance != 0uL is also true for a null balance. Before the PR, Home showed "Updating balance…" under an empty header. Now it shows nothing. Rates are cached in CacheStore, so this is limited to a fresh install or offline restore before the first successful rate fetch, with USDT visible. I confirmed it by reading the code, not on a device. It is a small edge, but it is the case where an updating hint matters most. BalanceHeaderView.kt:106 still wraps BalanceHeader, and so the loader, in converted?.let, so Home still shows no updating state when no fiat rate is loaded. Filing #1730 (milestone 3.2.0, names the loader explicitly) is the right way to defer a non-blocking point and needn't hold the merge, but it doesn't show the point wrong.
  • Loading feedback disappears when the fiat rate is unavailable (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:123). A wallet without a cached fiat rate now loses balance-refresh feedback if the rate endpoint is unavailable. With USDT enabled, open Home in that state: currency.convert() returns null, so converted?.let skips BalanceHeader and its loader despite balanceUpdating being true. The removed caption rendered outside this guard. This follows from the render path; device reproduction was blocked by sandbox permissions. Same state as point 1: the render path at BalanceHeaderView.kt:106 is unchanged, so with no cached rate the loader is still skipped while balanceUpdating is true. It is acknowledged, tracked in #1730 and listed under Out of Scope, which is fine for a non-blocking point, but the code doesn't answer it yet.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed e97d442: the loader is now 16 dp, smaller than the eye icon it stands in for, centred in the 24 dp slot. At 24 dp the ring filled its box, so half its stroke fell outside and the Home scroll column cut off its right side; it now renders whole. The Preview has a new recording and a full-resolution still taken mid-refresh. BalanceHeaderTest.kt (5 cases) and just lint pass on this head.

@ovitrif
ovitrif requested a review from coreyphillips October 29, 2026 20:29
coreyphillips
coreyphillips previously approved these changes Oct 29, 2026

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked after 1 new commit (e97d442). Nothing blocking remains.

still open

  • Updating indicator vanishes when no exchange rate is loaded (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:104). The indicator now only shows if the header renders, so Home shows no updating state at all when no fiat rate is loaded. Before this PR, HomeScreen drew the TotalBalanceUpdating caption as a sibling after BalanceHeaderView. It now lives inside BalanceHeader. BalanceHeaderView only calls BalanceHeader inside converted?.let { ... } (BalanceHeaderView.kt:104). currency.convert() returns null when CurrencyRepo.getCurrentRate finds no rate for the selected currency (empty rates). In that same state, usdtBalanceUpdating is true whenever USDT is visible. paykitRate is null, so runCatching { currency.paykitRate?.value(now) }.getOrNull() == null holds, and usdt.balance != 0uL is also true for a null balance. Before the PR, Home showed "Updating balance…" under an empty header. Now it shows nothing. Rates are cached in CacheStore, so this is limited to a fresh install or offline restore before the first successful rate fetch, with USDT visible. I confirmed it by reading the code, not on a device. It is a small edge, but it is the case where an updating hint matters most. The code is unchanged for this point. BalanceHeaderView.kt:106 still wraps BalanceHeader, and so BalanceLoader, in converted?.let, and CurrencyRepo.getCurrentRate still throws when no rates are cached. The PR moves this to Out of Scope as #1730, which is open in milestone 3.2.0 and names the loader regression. That defers the point; it does not show it is wrong. It stays non-blocking and need not hold the merge.
  • Loading feedback disappears when the fiat rate is unavailable (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:123). A wallet without a cached fiat rate now loses balance-refresh feedback if the rate endpoint is unavailable. With USDT enabled, open Home in that state: currency.convert() returns null, so converted?.let skips BalanceHeader and its loader despite balanceUpdating being true. The removed caption rendered outside this guard. This follows from the render path; device reproduction was blocked by sandbox permissions. This is the same render path as point 1, and nothing changed for it. The loader in BalanceLoader (BalanceHeaderView.kt:293) is still only reached through the converted?.let guard at line 106, so with no cached rate and a failing rate fetch, Home shows no refresh feedback. #1730 tracks it. The point stands, and it is non-blocking.

coreyphillips
coreyphillips previously approved these changes Oct 29, 2026

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked after 1 new commit (08f1e16). Nothing blocking remains.

still open

  • Updating indicator vanishes when no exchange rate is loaded (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:104). The indicator now only shows if the header renders, so Home shows no updating state at all when no fiat rate is loaded. Before this PR, HomeScreen drew the TotalBalanceUpdating caption as a sibling after BalanceHeaderView. It now lives inside BalanceHeader. BalanceHeaderView only calls BalanceHeader inside converted?.let { ... } (BalanceHeaderView.kt:104). currency.convert() returns null when CurrencyRepo.getCurrentRate finds no rate for the selected currency (empty rates). In that same state, usdtBalanceUpdating is true whenever USDT is visible. paykitRate is null, so runCatching { currency.paykitRate?.value(now) }.getOrNull() == null holds, and usdt.balance != 0uL is also true for a null balance. Before the PR, Home showed "Updating balance…" under an empty header. Now it shows nothing. Rates are cached in CacheStore, so this is limited to a fresh install or offline restore before the first successful rate fetch, with USDT visible. I confirmed it by reading the code, not on a device. It is a small edge, but it is the case where an updating hint matters most. BalanceHeaderView.kt:117 still wraps BalanceHeader in converted?.let, and the shimmer is drawn inside BalanceHeader, so with no fiat rate Home still shows no updating state while master's caption did. The PR now lists this as Out of Scope, tracked in #1730 (open, milestone 3.2.0). That defers the point but does not show it is wrong, and the code is unchanged.
  • Loading feedback disappears when the fiat rate is unavailable (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:123). A wallet without a cached fiat rate now loses balance-refresh feedback if the rate endpoint is unavailable. With USDT enabled, open Home in that state: currency.convert() returns null, so converted?.let skips BalanceHeader and its loader despite balanceUpdating being true. The removed caption rendered outside this guard. This follows from the render path; device reproduction was blocked by sandbox permissions. Same render path as point 1. The converted?.let at BalanceHeaderView.kt:117 still skips BalanceHeader, and with it BalanceShimmer and the TotalBalanceUpdating tag, when convert() returns null. Deferring it to #1730 is a reasonable call for a non-blocking point, but the regression is still in this branch.

new since the last review

worth doing, does not block

  • Shimmer's offscreen layer clips the BTC/fiat swap animation while refreshing (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:324-333). While intensity > 0, BalanceShimmer puts a graphicsLayer with CompositingStrategy.Offscreen on the Box around the swap AnimatedContent. That layer is sized to the Box, so anything drawn outside the Box is cut off. Compose documents the same thing for Modifier.alpha: the offscreen layer is sized to the composable's bounds and content outside them is omitted. The unit swap depends on drawing outside those bounds. swapLargeRowTransition (BalanceAnimations.kt:124) slides rows in and out by their full height, and BalanceHeaderView.kt:247-248 uses SizeTransform(clip = false) so that movement stays visible above the headline. If you tap the header to switch BTC/fiat while the balance is updating, the outgoing row is cut off at the headline's top edge. A wider incoming row is also cut on the right while the size animates. Offline, the updating state can last indefinitely, so every swap in that state looks like this. When idle there is no layer, so idle swaps are unaffected. I found this by reading the code against the documented layer behaviour. I did not reproduce it on a device. One fix is to apply the mask inside the AnimatedContent content lambda, on each LargeRow, so each sliding row carries its own layer.
  • Shimmer band never leaves short headlines, so each loop visibly jumps (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:355-362). shimmerMask moves the band centre from -band to width + band. That range only accounts for the band's horizontal extent. The gradient runs diagonally, from (center - band, 0) to (center + band, height), so at both ends of a pass part of the band is still over the text. Each loop then jumps it from the bottom-right to the top-left. I evaluated the mask formula in a script at both loop ends (sweep 0 and 1) for a 44dp-tall headline: width max alpha area above 0.76 62dp 0.999 48% 120dp 0.925 13% 230dp 0.814 3% A fresh wallet's ₿ 0 headline is about 62dp wide. That is the PR's own demo case, and there about half the text snaps between dim and nearly full brightness every 2 s. For the band to fully clear the box, each end of the travel needs to extend by height² / (2 * band). A band width that does not scale with the headline width would also avoid it.
  • Shimmer clips the balance-unit switch animation (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:324-326). During a USDT refresh, tapping the balance to switch between Bitcoin and fiat clips the sliding headline at the row's top edge. The new enclosing offscreen layer clips drawing to its measured bounds. swapLargeRowTransition moves content above those bounds, so the surrounding SizeTransform(clip = false) cannot preserve the existing animation. Confirmed by tracing the rendering code and documented layer behavior. Device reproduction was blocked by sandbox restrictions.

nits

  • Test hardcodes the English state description (app/src/androidTest/java/to/bitkit/ui/components/BalanceHeaderTest.kt:40). marksBalanceAsUpdatingWhileLoading compares the state description against the literal "Updating balance…", so it fails on a device whose locale is not English. Other tests in androidTest, for example WalletRestoreErrorViewTest.kt and UsdtReceivePrivacyTest.kt, look strings up with targetContext.getString(...). Using R.string.wallet__balance_updating the same way would make this test locale-independent.

@ovitrif
ovitrif dismissed stale reviews from coreyphillips, coreyphillips, and coreyphillips October 29, 2026 21:35

This approval was for the loader version of this PR, which a shimmer over the balance has replaced.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Dismissed the three approvals on the loader commits (4a32fea, 16bff47, e97d442), since the shimmer replaced that version. @coreyphillips's approval of the shimmer head, 08f1e16, stands. Marking this ready for review again.

@ovitrif
ovitrif marked this pull request as ready for review October 29, 2026 21:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08f1e16c67

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

coreyphillips
coreyphillips previously approved these changes Oct 29, 2026

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked after replies, with no new commit since 08f1e16. Nothing blocking remains.

still open

  • Updating indicator vanishes when no exchange rate is loaded (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:104). The indicator now only shows if the header renders, so Home shows no updating state at all when no fiat rate is loaded. Before this PR, HomeScreen drew the TotalBalanceUpdating caption as a sibling after BalanceHeaderView. It now lives inside BalanceHeader. BalanceHeaderView only calls BalanceHeader inside converted?.let { ... } (BalanceHeaderView.kt:104). currency.convert() returns null when CurrencyRepo.getCurrentRate finds no rate for the selected currency (empty rates). In that same state, usdtBalanceUpdating is true whenever USDT is visible. paykitRate is null, so runCatching { currency.paykitRate?.value(now) }.getOrNull() == null holds, and usdt.balance != 0uL is also true for a null balance. Before the PR, Home showed "Updating balance…" under an empty header. Now it shows nothing. Rates are cached in CacheStore, so this is limited to a fresh install or offline restore before the first successful rate fetch, with USDT visible. I confirmed it by reading the code, not on a device. It is a small edge, but it is the case where an updating hint matters most. HEAD is still 08f1e16. BalanceHeader, and with it BalanceShimmer, is still only drawn inside converted?.let at BalanceHeaderView.kt:117. CurrencyViewModel.convert still returns null when CurrencyRepo.getCurrentRate finds no rate. No response addresses this point, and the PR description moves it to #1730, which does not change the code here.
  • Loading feedback disappears when the fiat rate is unavailable (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:123). A wallet without a cached fiat rate now loses balance-refresh feedback if the rate endpoint is unavailable. With USDT enabled, open Home in that state: currency.convert() returns null, so converted?.let skips BalanceHeader and its loader despite balanceUpdating being true. The removed caption rendered outside this guard. This follows from the render path; device reproduction was blocked by sandbox permissions. This is the same render path as point 1, and nothing changed. With no rate loaded, convert() returns null, so the converted?.let at BalanceHeaderView.kt:117 skips the shimmer even when usdtBalanceUpdating is true. The removed caption in HomeScreen was drawn outside that guard. No response rebuts this.
  • Shimmer's offscreen layer clips the BTC/fiat swap animation while refreshing (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:324-333). While intensity > 0, BalanceShimmer puts a graphicsLayer with CompositingStrategy.Offscreen on the Box around the swap AnimatedContent. That layer is sized to the Box, so anything drawn outside the Box is cut off. Compose documents the same thing for Modifier.alpha: the offscreen layer is sized to the composable's bounds and content outside them is omitted. The unit swap depends on drawing outside those bounds. swapLargeRowTransition (BalanceAnimations.kt:124) slides rows in and out by their full height, and BalanceHeaderView.kt:247-248 uses SizeTransform(clip = false) so that movement stays visible above the headline. If you tap the header to switch BTC/fiat while the balance is updating, the outgoing row is cut off at the headline's top edge. A wider incoming row is also cut on the right while the size animates. Offline, the updating state can last indefinitely, so every swap in that state looks like this. When idle there is no layer, so idle swaps are unaffected. I found this by reading the code against the documented layer behaviour. I did not reproduce it on a device. One fix is to apply the mask inside the AnimatedContent content lambda, on each LargeRow, so each sliding row carries its own layer. BalanceHeaderView.kt:324-333 still wraps the whole large-row AnimatedContent in an Offscreen graphicsLayer while intensity > 0. That layer clips the swapLargeRowTransition slide, which SizeTransform(clip = false) at line 248 is there to allow. Greptile's reply on line 326 agrees with the point; the code has not changed.
  • Shimmer band never leaves short headlines, so each loop visibly jumps (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:355-362). shimmerMask moves the band centre from -band to width + band. That range only accounts for the band's horizontal extent. The gradient runs diagonally, from (center - band, 0) to (center + band, height), so at both ends of a pass part of the band is still over the text. Each loop then jumps it from the bottom-right to the top-left. I evaluated the mask formula in a script at both loop ends (sweep 0 and 1) for a 44dp-tall headline: width max alpha area above 0.76 62dp 0.999 48% 120dp 0.925 13% 230dp 0.814 3% A fresh wallet's ₿ 0 headline is about 62dp wide. That is the PR's own demo case, and there about half the text snaps between dim and nearly full brightness every 2 s. For the band to fully clear the box, each end of the travel needs to extend by height² / (2 * band). A band width that does not scale with the headline width would also avoid it. shimmerMask at BalanceHeaderView.kt:352-364 is unchanged. I re-ran the gradient at sweep 0 and 1 for a 44dp-tall headline. A 62dp headline still reaches about 1.0 alpha, with 47% of its area above 0.76, at both loop ends, so each pass still visibly jumps. No response addresses this.
  • Test hardcodes the English state description (app/src/androidTest/java/to/bitkit/ui/components/BalanceHeaderTest.kt:40). marksBalanceAsUpdatingWhileLoading compares the state description against the literal "Updating balance…", so it fails on a device whose locale is not English. Other tests in androidTest, for example WalletRestoreErrorViewTest.kt and UsdtReceivePrivacyTest.kt, look strings up with targetContext.getString(...). Using R.string.wallet__balance_updating the same way would make this test locale-independent. BalanceHeaderTest.kt:40 still compares against the literal "Updating balance…". HiltTestRunner and @ComposeUi do not pin the locale. The Codex and Greptile threads on line 40 agree with the point, and no fix has landed.
  • Shimmer clips the balance-unit switch animation (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:324-326). During a USDT refresh, tapping the balance to switch between Bitcoin and fiat clips the sliding headline at the row's top edge. The new enclosing offscreen layer clips drawing to its measured bounds. swapLargeRowTransition moves content above those bounds, so the surrounding SizeTransform(clip = false) cannot preserve the existing animation. Confirmed by tracing the rendering code and documented layer behavior. Device reproduction was blocked by sandbox restrictions. This is the same offscreen layer as point 3, unchanged at BalanceHeaderView.kt:324-326. Greptile's thread on line 326 restates it. Neither the mask nor the transition moved, so a unit swap during a refresh is still clipped.

@ovitrif
ovitrif marked this pull request as draft October 29, 2026 21:55
@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Back in draft. Pushed 54940c0: the shimmer now covers only the amount's digits (or hidden dots), never the currency symbol. It moves into LargeRow around the digits text, so the BTC/fiat swap no longer passes through its layer. The band has a fixed width and clears the text fully at both ends of each pass. The test reads the state description from resources. Device evidence on a funded wallet follows before this goes back to review.

@ovitrif
ovitrif dismissed stale reviews from coreyphillips and coreyphillips October 30, 2026 01:10

This approval was for an earlier version. The shimmer now covers only the amount's digits, never the symbol, and has new evidence on a funded wallet.

@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Back to ready at 54940c0: the shimmer covers only the digits or hidden dots, never the symbol. The Preview now uses the "all" regtest wallet with real amounts, and idle matches master in the Bitcoin, fiat and hidden states. just lint, just test and BalanceHeaderTest.kt pass. I dismissed the two approvals on 08f1e16 because they predate this change. Re-requesting review.

@ovitrif
ovitrif marked this pull request as ready for review October 30, 2026 01:10
@ovitrif
ovitrif requested a review from coreyphillips October 30, 2026 01:10

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked after 1 new commit (54940c0). Nothing blocking remains.

addressed

  • Shimmer's offscreen layer clips the BTC/fiat swap animation while refreshing (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:324-333). Fixed: The swap AnimatedContent at BalanceHeaderView.kt:243-270 is no longer wrapped. BalanceShimmer now sits inside LargeRow (lines 433-444) around the digits only, so each sliding row carries its own layer. The only other layer on a row is the scale graphicsLayer, which uses the default compositing strategy and does not clip. I confirmed this by reading the code, not on a device.
  • Shimmer band never leaves short headlines, so each loop visibly jumps (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:355-362). Fixed: BalanceHeaderView.kt:354-357 now uses a fixed 28dp half-band and overshoots by band + h²/(2·band). I evaluated the clamped gradient over the whole box at sweep 0 and 1, for widths 26, 62, 120 and 230dp at 44dp height. The maximum alpha was 0.75 (fully dimmed) in every case, so each pass starts and ends off the text and the loop no longer jumps.
  • Test hardcodes the English state description (app/src/androidTest/java/to/bitkit/ui/components/BalanceHeaderTest.kt:40). Fixed: BalanceHeaderTest.kt:32-33 resolves R.string.wallet__balance_updating through the instrumentation target context, and line 45 asserts against it. This matches the pattern in WalletRestoreErrorViewTest.kt. I did not run it on a device.
  • Shimmer clips the balance-unit switch animation (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:324-326). Fixed: Same fix as point 3: the offscreen layer now wraps only the digits inside each LargeRow (BalanceHeaderView.kt:433-444), and nothing encloses the swap AnimatedContent. The slide above the row is no longer clipped. I confirmed this by reading the code.

still open

  • Updating indicator vanishes when no exchange rate is loaded (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:104). The indicator now only shows if the header renders, so Home shows no updating state at all when no fiat rate is loaded. Before this PR, HomeScreen drew the TotalBalanceUpdating caption as a sibling after BalanceHeaderView. It now lives inside BalanceHeader. BalanceHeaderView only calls BalanceHeader inside converted?.let { ... } (BalanceHeaderView.kt:104). currency.convert() returns null when CurrencyRepo.getCurrentRate finds no rate for the selected currency (empty rates). In that same state, usdtBalanceUpdating is true whenever USDT is visible. paykitRate is null, so runCatching { currency.paykitRate?.value(now) }.getOrNull() == null holds, and usdt.balance != 0uL is also true for a null balance. Before the PR, Home showed "Updating balance…" under an empty header. Now it shows nothing. Rates are cached in CacheStore, so this is limited to a fresh install or offline restore before the first successful rate fetch, with USDT visible. I confirmed it by reading the code, not on a device. It is a small edge, but it is the case where an updating hint matters most. The code is unchanged: converted?.let at BalanceHeaderView.kt:117 still skips BalanceHeader and the shimmer when no rate is cached. The PR's Out of Scope note defers this to #1730, which is open in milestone 3.2.0 and describes the regression accurately. That is a reasonable deferral for a non-blocking point, but it neither fixes the point nor shows it is wrong.
  • Loading feedback disappears when the fiat rate is unavailable (app/src/main/java/to/bitkit/ui/components/BalanceHeaderView.kt:123). A wallet without a cached fiat rate now loses balance-refresh feedback if the rate endpoint is unavailable. With USDT enabled, open Home in that state: currency.convert() returns null, so converted?.let skips BalanceHeader and its loader despite balanceUpdating being true. The removed caption rendered outside this guard. This follows from the render path; device reproduction was blocked by sandbox permissions. Same as point 1: the converted?.let guard at BalanceHeaderView.kt:117 is unchanged, so with no cached rate there is still no updating feedback. It is tracked in #1730, which is fine for a non-blocking item, but it is not fixed in this PR.

new since the last review

nits

  • Journey does not say the currency symbol stays out of the shimmer (journeys/usdt-cached-home-balance.md:6). The journey does not check the main behaviour 54940c0 adds, that the symbol is never dimmed or swept. Step 2 of journeys/usdt-cached-home-balance.md still says "a soft shimmer sweeps over the headline", and the PR's J1 summary says "over the headline digits". A device run would pass even if the ₿/$ symbol were shimmered again. Adding "the currency symbol stays steady" to step 2 would cover it. Per the journey rules, the iOS twin needs the same wording.

Comment thread journeys/usdt-cached-home-balance.md Resolved
@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed dda8e2b: journey J1 now requires the currency symbol to stay steady while the digits or hidden dots shimmer, answering @coreyphillips. Docs only; the code and media are unchanged.

@ovitrif
ovitrif requested a review from coreyphillips October 30, 2026 01:21
@piotr-iohk
piotr-iohk self-requested a review October 30, 2026 13:23

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: full Android PR review at dda8e2bc4, against merge base e28307b9d. Covered the complete diff, affected callers, loading transitions, accessibility and tests. Compared the shared loading behavior with iOS e4f7ae08; this was not a full iOS review.

No new actionable code findings.

Existing concern: #1730 remains open and present: without cached fiat rates, Home suppresses both the header and its updating indicator. Tracking this separately has not fixed it; no duplicate finding is added here.

Validation: the matching CI build and unit-test run passed. Locally, diff checks and a standalone Kotlin shimmer-geometry probe passed; app compilation stopped because the Android SDK is unavailable. The Compose instrumentation tests were inspected, not executed. The geometry probe does not establish visual correctness.

Device testing: not performed in this review. J1 still requires a prepared aged balance/rate cache and controlled refresh endpoints.

Suggested additional test cases

  • Android: with cached amounts visible and a USDT refresh held pending, repeatedly tap the total to swap BTC/fiat, navigate away and back, then allow refresh to finish. Expect intact digits and symbols throughout the swap, an updating indicator on return while still pending, and a cleared indicator without a layout jump when refresh completes.

Reviewer model: gpt-6-astra
Reasoning effort: high

@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

@piotr-iohk thanks for the QA pass.

  • bug: home balance header missing without fiat rates #1730 stays tracked in 3.2.0: no cached fiat rate hides the whole header on master too, so it is out of scope for this PR.
  • The swap-while-pending case: the shimmer wraps the digits inside LargeRow's AnimatedContent, so it moves with the BTC/fiat swap, and the symbol is outside it. I have not run a swap during a held refresh on a device; it is a good J1 extension for QA.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the scoped Home balance-header changes at dda8e2bc4284634bf65bb405ce2ee574ad66e33f following the code review and focused device checks on an API 33 arm64 emulator.

Observed: cached offline amounts stayed readable; digits shimmered with a steady symbol; disabling animations produced static dimming; hide/reveal preserved values; BTC/fiat swaps and Home return during a held refresh retained intact glyphs; successful balance/rate reads recovered despite history failures; failed balance reads retained the cached amount.

The missing headline/updating feedback without a fiat rate remains deferred in #1730. Fresh zero correctly restored the USDT zero amount and funding prompt, but the missing BTC headline remains part of that issue.

Remaining testing gaps:

  • Conversion/review requiring fresh FX when only an expired rate is available was not exercised.
  • The exact separate-install/different-credential cache-isolation recipe was not run. Changing endpoint scope rejected the old snapshot, and restoring the original scope restored its cached amount.

These checks used a locally rebuilt pinned revision, isolated test app/storage, and synthetic read-only RPC/FX display balances. Payment and cache-validation logic is unchanged by this PR. This approval accepts the disclosed gaps for the UI change; it does not claim a complete authored journey pass or funded-transfer testing.

@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Merged: approved by @piotr-iohk on dda8e2b after device checks, required checks green, no open threads. The missing header without a cached fiat rate stays in #1730.

@ovitrif
ovitrif merged commit fdc1012 into master Oct 30, 2026
21 checks passed
@ovitrif
ovitrif deleted the fix/home-balance-loader branch October 30, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants