Skip to content

fix: steady home balance header - #1168

Open
ovitrif wants to merge 19 commits into
masterfrom
fix/home-balance-loader
Open

ovitrif wants to merge 19 commits into
masterfrom
fix/home-balance-loader

Conversation

@ovitrif

@ovitrif ovitrif commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Twin: synonymdev/bitkit-android#1729
Refs: #1131

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 soft diagonal band, drawn as on Android, 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 Reduce Motion on, there is no sweep: the digits only dim while the balance refreshes.
  • MoneyText gains an opt-in shimmer parameter, and MoneyStack an optional isLoading (nil by default) that feeds it; only the Home headline sets it, so every other MoneyStack and MoneyText renders as on master. While it is set, the headline is drawn twice with the same layout: a shimmered copy that shows only the digits and, on top, an unmasked copy that shows only the symbol, so the symbol stays steady however the headline wraps at large text sizes (ZŁ in [Bug]: Home overlaps wallet rows and shimmers wrapped currency symbol at maximum text size #1176). The mask reaches 24 pt past the text frame, because DisplayText draws glyph tops and comma tails outside it.
  • It takes no layout space: the headline lays out exactly as on master, the eye icon stays where it was, and nothing below the header moves.
  • While refreshing, an element over the balance carries the TotalBalanceUpdating identifier and the "Updating balance…" accessibility label.

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

  • HomeWalletView.swift: when the balance counts as updating (isBalanceUpdating) is unchanged.

Design

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

Preview

The recordings are at 2x on an iPhone 17 Pro simulator (iOS 26.5), from a local debug build that toggles the updating state every 7 s and shows a large headline, so each shows refresh starts, a few sweeps and the fade-outs. Fiat and hidden headlines shimmer the same way.

Refresh
Bitcoin, default text size
look-after-2x.mp4
₿ 12 345 / 678 900, largest text size
btcwrap-after-2x.mp4
$ on its own line, largest text size
wrap-after-2x.mp4

Idle headline, master above this PR for each state (Bitcoin, fiat, hidden). Each pair was pixel-identical at 0075dac; since 8df31d0 the symbol is drawn as its own layer and matches to within 1/255 on a few anti-aliased edge pixels.

QA Notes

Setup

Regtest debug build with default endpoints. Restore a funded regtest wallet, or create one and enable USDT on the Home intro. Home refreshes USDT every 30 s while USDT is on. For a fiat headline with a comma, pick a currency such as CLP. For the fallback, turn on Settings › Accessibility › Motion › Reduce Motion.

Journeys

  • J1: updated usdt-cached-home-balance.md — TotalBalanceUpdating is a shimmer over the headline digits (a slight dim with Reduce Motion) 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

  • ran HomeWalletBalanceTests.swift — when the balance counts as updating is unchanged

Evidence

  • On 0075dac, iPhone 17 simulator (iOS 27), "sea" wallet: the shimmer runs over the digits or dots and never over the symbol, and the accessibility tree exposes TotalBalanceUpdating with "Updating balance…" while it runs.
  • On 0075dac, idle Home was pixel-identical to master in the Bitcoin, fiat and hidden states (0 differing pixels across the headline row). On 30a3eb3 the same wallet showed the clipping @coreyphillips found: the ₿ and $ lost their bars and the comma its tail (comparison).
  • On 8df31d0, iPhone 17 Pro simulator (iOS 26.5) at the largest text size: with the headline split as ₿ 12 345 / 678 900 and with $ alone on line 1, every digit dims and sweeps and the symbol stays steady; at 1add21f the split BTC headline swept the ₿, and at 83c10cc the digits under a lone $ never dimmed (recordings, earlier). The idle headline matches 1add21f to within 1/255 on a few ₿ edge pixels.
  • HomeWalletBalanceTests and SwiftFormat lint pass on 0075dac; b8009e5 only makes MoneyStack.isLoading optional and builds clean.

Gaps

  • Reduce Motion fallback not run on the simulator; it follows from the code path, and J1 covers it.
  • No UI test asserts the shimmer; the journey covers it.
  • 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:21
@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:53.969029Z 0075dac 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
@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 synonymdev/bitkit-android#1700 and #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…" caption into a 24 pt ActivityIndicator in a fixed trailing slot of MoneyStack, so the rows below no longer shift. Reviewed 82ac6fe, light tier.

What I checked, and 4 candidates I ruled out

Read in full: MoneyStack.swift, the HomeWalletView.swift hunk, ActivityIndicator.swift, DisplayText in TextStyle.swift
Call sites traced: every MoneyStack( caller; only HomeWalletView passes showEyeIcon: true (Savings and Spending pass false, the rest use the default), so the new fixed slot only affects Home
CI: SwiftFormat and translation validation green; Run Tests, Run Integration Tests and build-local were still running at review time
Not built here: iOS does not build on this reviewer's Linux host, so this was read from the code and CI is the gate

Ruled out

  • Eye button moved or lost: .padding(.leading, 8) moved from eyeIconButton to the slot, so the hidden-balance state keeps the same 8 + 24 pt, and ShowBalance stays on the button.
  • Loader under hidden amounts: the ZStack branches on settings.hideBalance first, so the loader cannot show while amounts are masked. That matches the Android twin's isLoading && !hideBalance.
  • Double fade: ActivityIndicator runs its own 1 s onAppear opacity ramp under the slot's 0.3 s .transition(.opacity), so the appearance is a slower fade-in, not a flash. Removal goes through the transition.
  • Narrower headline when balances are shown: before, the 32 pt slot existed only while hidden; now it is always reserved on Home. DisplayText has no lineLimit, so a very long primary amount on a narrow device wraps 32 pt sooner than before. This is narrow (the hidden state already reserved that width, and the Android row always did), but I could not measure it without a simulator.

Merge confidence: 4/5, no findings, but no test asserts the loader (the PR's own Gaps) and unit and integration tests had not finished at review time.

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

@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.

worth doing, does not block

  • Hidden-balance behaviour of the loader is not covered by J1 or any test (journeys/usdt-cached-home-balance.md:7). The PR says the loader is suppressed while balances are hidden and the eye keeps the slot. The Android twin asserts this in BalanceHeaderTest.keepsEyeIconWhileBalanceIsHidden, but nothing on iOS checks it. Step 3 of usdt-cached-home-balance.md hides balances during the cached-balance run but does not say whether TotalBalanceUpdating should show. That is a change from the old caption, which stayed visible while balances were hidden. Adding the expectation to step 3 would let J1 cover it.

the reviewers disagree, your call

  • Always-reserved trailing slot makes large visible balances wrap to two lines (Bitkit/Components/MoneyStack.swift:163-180). Objection: Real, but non-blocking rather than blocking: The slot reserves 32 pt for every Home headline, including BTC-only wallets, while MoneyText and DisplayText allow wrapping without scaling. This is a non-blocking layout regression; the listed iOS thresholds remain unverified.

Comment thread Bitkit/Components/MoneyStack.swift Resolved
Comment thread journeys/usdt-cached-home-balance.md Resolved
@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed cbf8ba9: the loader is now an overlay on the balance row's trailing edge instead of a reserved 24 pt slot. With the slot, a long headline (₿ 9 999 999 999 on iPhone 17e) wrapped where master keeps it on one line; the overlay leaves the idle row exactly as on master. The overlay also gives the indicator a fixed 24 pt frame, without which it stayed near zero opacity when a refresh started after launch. The description and recording are updated for this head; 82ac6fe added the changelog fragment.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 323827f: journey step 3 now expects TotalBalanceUpdating to stay hidden while balances are hidden, answering @coreyphillips's coverage point. His wrapping point was fixed by the overlay in cbf8ba9.

@ovitrif
ovitrif requested a review from coreyphillips October 29, 2026 19:55
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 (cbf8ba9, 323827f). Nothing blocking remains.

addressed

  • Always-reserved trailing slot makes large visible balances wrap to two lines (Bitkit/Components/MoneyStack.swift:163-180). Fixed: Both headline rows in Bitkit/Components/MoneyStack.swift (62-66 and 110-114) are back to master's layout: the eye shows only while hideBalance is on, and .padding(.leading, 8) sits on eyeIconButton at :191 as on master. The loader is now only an .overlay(alignment: .trailing) at :66 and :114, which takes no layout space. I checked that git diff origin/master...HEAD adds only the overlay line to each row. A SwiftUI layout run with the app's InterTight fonts keeps "₿ 1 000 000 000" on one line on 390 to 402 pt rows.
  • Hidden-balance behaviour of the loader is not covered by J1 or any test (journeys/usdt-cached-home-balance.md:7). Fixed: Step 3 of journeys/usdt-cached-home-balance.md (line 7) now says the eye keeps its place and TotalBalanceUpdating must not show while balances are hidden. That matches the guard showEyeIcon && isLoading && !settings.hideBalance at MoneyStack.swift:170. Step 2 leaves the refresh endpoints down, so the balance is still updating during step 3 and the check means something.

new since the last review

worth doing, does not block

  • Overlay loader covers the last digit of 10+ BTC headlines on 390-393 pt phones (Bitkit/Components/MoneyStack.swift:66). The overlay takes no layout space, so on narrower phones a long headline runs under the loader. At default text size on 390 to 393 pt phones (iPhone 16e/17e, 14 to 16), headlines of 10 BTC or more (1 000 000 000 sats and up) stay on one line but run under the loader at MoneyStack.swift:66 and :114. The loader then sits over the last digit. The loader stays on screen for as long as isBalanceUpdating is true. With a non-zero USDT balance that includes a failed balance read or an expired rate, so offline it can stay indefinitely. How I checked: I measured the headline with CoreText and the app's InterTight Black and ExtraBold at 44 pt, kerning -1. "₿ 1 000 000 000" is about 343 pt wide and "₿ 9 999 999 999" about 339 pt. The loader starts at 334 pt on a 358 pt row and 337 pt on a 361 pt row. A SwiftUI render of the same HStack keeps both on one line at those widths. At 402 pt the text ends about 3 pt short of the loader, and 375 pt phones wrap as on master. The band is wider with larger Dynamic Type, which DisplayText allows up to xxLarge. At xLarge, 1 to 9.99 BTC also run under the loader on 390 to 393 pt phones. This matches the PR's Gaps entry. It is minor and does not need to hold the merge, but the Gaps entry understates it: the overlap starts at 10 BTC on common phones, not only at ₿ 9 999 999 999 on the narrowest one.
  • Long balances collide with the overlaid loader (Bitkit/Components/MoneyStack.swift:66). The spinner can obscure the final digit of a long balance during refresh. An isolated macOS render using the bundled fonts and row modifiers reproduced overlapping pixels for ₿ 1 234 567 890 at 343 pt content width. The overlay occupies space still available to the text; iOS reproduction was blocked by simulator access.

Comment thread Bitkit/Components/MoneyStack.swift Resolved
Comment thread Bitkit/Components/MoneyStack.swift Resolved
@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed f71a9e8: the loader is now 16 pt, smaller than the eye icon it stands in for, and turns once every 2 s instead of 1.2 s, the same speed as on Android. ActivityIndicator gains a rotationDuration parameter whose default keeps every other caller as it was. The Preview has a new recording and a full-resolution still taken mid-refresh.

@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 (f71a9e8). Nothing blocking remains.

still open

  • Overlay loader covers the last digit of 10+ BTC headlines on 390-393 pt phones (Bitkit/Components/MoneyStack.swift:66). The overlay takes no layout space, so on narrower phones a long headline runs under the loader. At default text size on 390 to 393 pt phones (iPhone 16e/17e, 14 to 16), headlines of 10 BTC or more (1 000 000 000 sats and up) stay on one line but run under the loader at MoneyStack.swift:66 and :114. The loader then sits over the last digit. The loader stays on screen for as long as isBalanceUpdating is true. With a non-zero USDT balance that includes a failed balance read or an expired rate, so offline it can stay indefinitely. How I checked: I measured the headline with CoreText and the app's InterTight Black and ExtraBold at 44 pt, kerning -1. "₿ 1 000 000 000" is about 343 pt wide and "₿ 9 999 999 999" about 339 pt. The loader starts at 334 pt on a 358 pt row and 337 pt on a 361 pt row. A SwiftUI render of the same HStack keeps both on one line at those widths. At 402 pt the text ends about 3 pt short of the loader, and 375 pt phones wrap as on master. The band is wider with larger Dynamic Type, which DisplayText allows up to xxLarge. At xLarge, 1 to 9.99 BTC also run under the loader on 390 to 393 pt phones. This matches the PR's Gaps entry. It is minor and does not need to hold the merge, but the Gaps entry understates it: the overlap starts at 10 BTC on common phones, not only at ₿ 9 999 999 999 on the narrowest one. F71a9e87 only shrinks the ring to 16 pt inside the same 24 pt trailing overlay (MoneyStack.swift:171 and :179). Counting the 2.5 pt stroke, the ring now starts about 21 pt from the row's end instead of 24. I measured with CoreText, using the bundled InterTight Black and ExtraBold at 44 pt with kerning -1. The ink of "₿ 1 000 000 000" ends at 342.4 pt, past the ring's left edge at 336.8 pt on a 358 pt row and at 339.8 pt on a 361 pt row. So the overlap shrinks from about 8 pt to about 6 pt but is still there. Your reply agrees and leaves it to a design call. That is a reasonable call, but it is not a rebuttal, so this stays open until a design is chosen.
  • Long balances collide with the overlaid loader (Bitkit/Components/MoneyStack.swift:66). The spinner can obscure the final digit of a long balance during refresh. An isolated macOS render using the bundled fonts and row modifiers reproduced overlapping pixels for ₿ 1 234 567 890 at 343 pt content width. The overlay occupies space still available to the text; iOS reproduction was blocked by simulator access. The loader is still an overlay that takes no width (MoneyStack.swift:66 and :114), so the text can still run under it. Same measurement: on a 343 pt row the ink of "₿ 1 234 567 890" ends at 322.2 pt and the 16 pt ring now starts at 321.75 pt. At default text size they now only just touch. Wider amounts and larger Dynamic Type sizes still overlap. The reply points to the thread for point 1, which defers the fix rather than settling it.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 30a3eb3: the loader is replaced by a soft shimmer over the headline balance. The text dims to 75% and a diagonal band sweeps it back to full every 2 s, fading in and out with the refresh. It runs over hidden-amount dots too, and with Reduce Motion it only dims. It takes no layout space, so the headline lays out as on master and @coreyphillips's overlap case no longer exists. ActivityIndicator is back to its master version. The Preview has a new recording and a full-resolution still.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Back to draft while the loader is replaced with a subtle shimmer over the balance. The current approvals were given on the loader version, and this keeps this PR from merging before the shimmer is pushed and reviewed. It returns to review with a new still, a recording and fresh approvals.

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

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Merged master into this PR (e4f7ae0) to rerun E2E on a fresh head.

The last run failed only @lightning_1, in all three attempts. It fails after the channel close: the app stays on Lightning Connections, which has no HeaderMenu, so doNavigationClose times out. This PR changes only the Home headline (MoneyText, MoneyStack, HomeWalletView), not navigation or channels.

@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

@lightning_1 failed again on e4f7ae0, at the same HeaderMenu step after the channel close. The same failure shows up in today's and yesterday evening's runs on four other branches, so it isn't caused by this PR:

  • fix/paykit-inbox-observation
  • fix/paykit-fixed-request-confirm
  • fix/ordinary-wallet-paykit-waits
  • codex/ios-public-sharing-privacy-20261021

Re-running the failed E2E job.

@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

The rerun failed @lightning_1 again. Cause: #1054 (on master since 29 Oct) returns to Lightning Connections after a channel close, and the E2E spec still expects the wallet drawer button there. It fails on every iOS PR now, not only this one.

synonymdev/bitkit-e2e-tests#288 updates the spec. Once it merges, I'll rerun E2E here.

@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 PR review at e4f7ae08 against merge base 888fce91, covering the complete diff, affected callers, loading transitions, accessibility and test coverage. Compared only the corresponding Home balance behavior in Android dda8e2bc; this is not a full Android review.

No new actionable code findings.

Earlier review concerns are addressed in the pinned source, including glyph clipping, interrupted fades, and unnecessary work outside Home.

Validation: SwiftFormat 0.64.0 passed on all three changed Swift files; git diff --check passed. CI logs confirm all five HomeWalletBalanceTests passed on merge 2d5b0294, whose parents match the supplied head and base. The E2E rerun succeeded after retry, but the inspected suite revision has no shimmer-specific assertion.

Device testing: not performed in this review. This Linux environment has no Xcode; J1, visual mask alignment and the runtime accessibility tree remain unexecuted here.

Suggested additional test cases

  • iOS: on a narrow iPhone at the largest supported text size, use a long grouped balance and a suffix-symbol fiat currency. During refresh, switch units and hide/reveal the amount. Expect intact glyphs, a steady symbol and eye, and no loading-induced layout shift.
  • iOS: with controlled refresh timing, restart loading within the 300 ms fade-out, then leave and return to Home. Expect the active refresh to keep sweeping and the final completion to restore full opacity.

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.

  • Suffix-symbol currencies: the mask covers only the amount span between the leading and trailing text, so a trailing symbol stays outside the shimmer like a leading one. The narrow-device, largest-text case with unit switching was not run on a simulator; it is a good J1 extension for QA.
  • Restart within the fade-out: the changeCount guard drops a stale fade-out completion, so a refresh that restarts during the 300 ms fade keeps sweeping and only the last completion restores full opacity. Not exercised with controlled refresh timing on a device.

@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 e4f7ae087c909a3fa054c441fb3bafe100d8a9ea following the code review and focused checks on an iPhone 17 simulator / iOS 26.0.

Observed: cached offline amounts stayed readable; the symbol stayed steady at normal text size; Reduce Motion produced static dimming; hide/reveal and Home return preserved values; successful balance/rate reads cleared updating despite history failures; balance failures retained cached values; fresh zero without FX showed zero and restored the funding prompt.

Non-blocking Low/P3 follow-up: #1176 records the wrapped PLN symbol participating in the shimmer at maximum accessibility text size, plus the observed USDT label overlapping the Savings amount. Screenshot and two recordings are attached there. The broader layout overlap has not been compared against master and is not established as a regression from this PR.

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. Endpoint-scope rejection and restoration of the original cached balance passed.
  • A measured loading restart within the 300 ms fade-out was not exercised. Ordinary refresh recovery and Home return passed; they do not prove this timing case.

These checks used a locally rebuilt pinned revision, isolated test app/storage, and synthetic read-only RPC/FX display balances. This approval accepts the disclosed visual follow-up and testing gaps for the UI change; it does not claim a complete authored journey pass or funded-transfer testing.

At large text sizes a suffix symbol can wrap below the amount; only the amount's first line shimmers then.

Refs #1176
@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Pushed f48ed5c for #1176: at large text sizes a suffix symbol such as ZŁ can wrap below the amount, and the shimmer swept it too. When the headline wraps, only the amount's first line shimmers now, so the wrapped symbol stays steady; single-line headlines are unchanged.

Checked on an iPhone 17 Pro simulator (iOS 26.5) at the largest accessibility text size with a PLN headline that wraps, from a local build that toggles the updating state every 7 s (2x):

Before (e4f7ae0) After (f48ed5c)
before-1176-2x.mp4
after-1176-2x.mp4

Across the recordings the ZŁ brightness swung between 48% and 68% before, and stays at 64–66% after, while the digits dim and sweep the same way in both. At the default text size the headline still shimmers on one line with a steady ZŁ.

The wallet-row overlap at that text size in #1176 is not part of this change and stays in that issue.

@piotr-iohk this push dismissed your approval; re-requesting your review.

@ovitrif
ovitrif requested a review from piotr-iohk October 30, 2026 15:04
@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Merged master into this PR (83c10cc): the last run's build-local stopped at the stale-checkout guard because master had moved on during the run. No code change; this run also picks up the updated E2E spec from synonymdev/bitkit-e2e-tests#288.

@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: Re-reviewed the full PR at 83c10cc after the merge base changed since the saved assessment. No earlier coverage was inherited.

1 actionable finding — resolve or provide an evidence-backed rebuttal.

Single-line headlines, hidden dots, and a suffix symbol on the following line match the shimmer rules in this diff. A leading symbol that occupies the first line by itself leaves the digits outside the mask. Android dda8e2bc was compared only for this header: its shimmer wraps the amount view and the symbol sits outside that view.

#1176 still tracks the large-text wallet-row overlap, which this pass did not re-establish as a change from the merge base. The wrapped ZŁ sweep in that issue is addressed by the first-line mask at this head. The earlier clipping and fade-restart threads are addressed in the pinned source.

Validation: HomeWalletBalanceTests was inspected and does not cover the mask. SwiftFormat passed on this head. The unit-test job was still pending, and no tests were run in this review. Widths were measured with HarfBuzz on the bundled Inter Tight fonts, not rendered in SwiftUI.

Device testing: not performed in this review.

Suggested additional test cases

  • iOS, on a 402pt-wide phone and a 375pt-wide phone. Set Dynamic Type to the largest size this headline uses (DisplayText caps at xxLarge). Enable USDT and hold refresh pending, with fiat as the primary unit and a leading symbol. Use an amount that fits on one line alone but not beside the symbol, such as $ 12,345,678.00. Expected: the digits dim and the band sweeps them, the symbol stays at full opacity, and Savings and the activity list stay put when the shimmer starts and stops.

Findings

  • [MEDIUM] Leading symbol wrap leaves the digits unshimmered — inline at Bitkit/Components/MoneyText.swift:373.

Reviewer model: grok-4.7
Reasoning effort: high

Comment thread Bitkit/Components/MoneyText.swift Resolved
…nal band

At large text sizes a leading symbol can take a line of its own; the shimmer now follows the amount onto the next line. The band is now drawn like Android's: a soft diagonal sheen across the digits instead of a clipped strip.
@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Pushed 1add21f with two shimmer fixes:

  • Leading symbol on its own line (answering @piotr-iohk's finding): when the $, € or ₿ takes a line of its own at large text sizes, the digits now dim and sweep on their line, and the symbol stays steady. The mask measures whether the leading text wraps and, if so, moves the shimmer to the amount's line with no symbol inset.
  • Band look: the band was a narrow strip as tall as the headline, so its gradient ran almost vertically and its sides showed hard edges. It is now drawn like Android's: a soft diagonal sheen of the same width and slope, travelling fully past the amount at both ends.

Checked on an iPhone 17 Pro simulator (iOS 26.5) with a local build that toggles the updating state every 7 s and shows a large headline (2x):

Before (83c10cc) After (1add21f)
Band, default text size
look-before-2x.mp4
look-after-2x.mp4
$ wrapped, largest text size
wrap-before-2x.mp4
wrap-after-2x.mp4

In the wrapped case the digits' mean brightness stayed flat before (never dimmed) and now drops to about 75% and sweeps while updating; the $ looks the same idle and updating. The wrapped ZŁ case from f48ed5c is unchanged.

@piotr-iohk re-requesting your review.

@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: Follow-up at 1add21f5, descendant of 83c10cc4, with the same merge base 3f14ea05. Baseline: previous review. This pass rechecked the MoneyText amount mask and diagonal sweep, including a leading symbol that wraps, and compared that sweep with the Android header shimmer at dda8e2bc. Inherited: caption removal, the MoneyStack.isLoading opt-in, and unchanged isBalanceUpdating; other MoneyStack callers left at the nil isLoading default; journeys/usdt-cached-home-balance.md steps 2 and 3.

1 actionable finding — resolve or provide an evidence-backed rebuttal.

The new height check treats every taller headline as a symbol-only first line. A space-grouped modern amount that still has digits beside the symbol keeps those digits steady. Android still shimmers the whole amount and draws the symbol as a sibling, so that split does not arise there; the iOS band uses the same 28pt half-width, two-second pass, and 75% floor.

Device testing: not performed in this review. No simulator run and no unit test was executed. HomeWalletBalanceTests covers when the balance counts as updating, which this commit does not change. Wrap examples below are Inter Tight advance sums at the xxLarge-scaled display size, not a captured layout.

Suggested additional test cases

  • iOS, largest text size the headline allows. Modern bitcoin and a balance such as 1 BTC (100 000 000) so the first line contains ₿ and the first groups, with the last group wrapped. While TotalBalanceUpdating is present, every digit group dims and the band sweeps it, and ₿ stays steady.

Earlier findings reconciled at this revision: 2 verified fixed, 1 tracked separately outside this PR.

Tracked separately (outside this PR; unresolved): Large-text wallet rows overlap. Issue #1176 remains open for the USDT label overlapping the Savings amount at the largest text size. The issue states that overlap was not compared with master and is not established as a regression from this PR. The diff from base 3f14ea0 to head 1add21f does not change the wallet-row views. HomeWalletView still places Savings and Spending in the same HStack; the hunk only passes isLoading and removes the updating caption above that row. This is not a runtime verification or merge-readiness declaration.

Findings

  • [LOW] Shimmer digits that share a line with the symbol — inline at Bitkit/Components/MoneyText.swift:151.

Reviewer model: grok-4.7
Reasoning effort: high

Comment thread Bitkit/Components/MoneyText.swift Resolved
Draw the headline twice with the same layout: the shimmered copy shows only the digits and the copy on top only the symbol. This replaces the measured masks, so a line break inside the amount or around the symbol no longer leaves digits steady or sweeps the symbol.
@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Pushed 8df31d0, answering @piotr-iohk's finding on digits sharing a line with the symbol. The headline is now drawn twice with the same layout: the shimmered copy shows only the digits (the symbol drawn clear) and the copy on top only the symbol, unmasked. The measured masks are gone, so a line break inside the amount, before it or after it can no longer leave digits steady or sweep the symbol. The band keeps the Android-style diagonal from 1add21f.

Checked on an iPhone 17 Pro simulator (iOS 26.5) with a local build that toggles the updating state every 7 s and shows ₿ 12 345 678 900; at the largest text size it wraps as ₿ 12 345 / 678 900 (2x):

Before (1add21f) After (8df31d0)
₿ and digits split across lines
btcwrap-before-2x.mp4
btcwrap-after-2x.mp4
  • Split BTC headline: at 1add21f the ₿ brightness swung from 48% to 68% with the sweep; now it stays at 65–66% while both digit lines dim and sweep.
  • $ alone on line 1 and the default-size headline shimmer as at 1add21f; the symbol stays steady.
  • Idle headline matches 1add21f to within 1/255 on a few anti-aliased ₿ edge pixels.
look-after-2x.mp4

@piotr-iohk re-requesting your review.

@ovitrif
ovitrif requested a review from piotr-iohk October 30, 2026 16:58

@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: Follow-up of the full PR at 8df31d0d, since 1add21f5, with inherited baseline review 5715539957. The merge base is still 3f14ea05.

No new actionable code findings.

The delta redraws the headline twice from the same string. The masked copy clears the symbol and prefix, and the overlay clears the digits, so a wrap inside the amount, a leading symbol on its own line, or a wrapped suffix keeps every digit in the sweep and leaves the symbol steady. Hidden dots stay on the masked copy. The band is a 2s diagonal that dims digits to 75%, with a 44pt rise; its travel still crosses a taller wrapped block. Compared with Android dda8e2bc, the symbol stays outside the shimmer. Android masks the digit text in a non-wrapping row; the iOS overlay is what keeps that split when the headline wraps.

Inherited unchanged: Home caption removal, MoneyStack.isLoading opt-in, and isBalanceUpdating; other MoneyStack callers at the nil default; journey usdt-cached-home-balance.md steps 2 and 3.

Unit tests and SwiftFormat passed on this revision. This review did not run those jobs or a simulator. The author reports the split-amount and lone-symbol recordings on an iPhone 17 Pro simulator at this revision.

Device testing: not performed in this review.

Ready for device testing.

Earlier findings reconciled at this revision: 1 verified fixed, 1 tracked separately outside this PR.

Tracked separately (outside this PR; unresolved): Large-text wallet rows overlap. Issue #1176 remains open for the USDT label overlapping the Savings amount at the largest text size. The issue states that overlap was observed on e4f7ae0, was not compared with master, and is not established as a regression from this PR. From base 3f14ea0 to head 8df31d0, HomeWalletView still places Savings and Spending in the same HStack; the hunk only passes isLoading and removes the updating caption above that row. WalletBalanceView and the USDT label are unchanged, and those row amounts do not take the new shimmer path. The same issue's wrapped-suffix shimmer is a different mechanism, already assessed by the frozen discovery at this head, and this disposition does not close #1176. This is not a runtime verification or merge-readiness declaration.

Reviewer model: grok-4.7
Reasoning effort: high

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No findings at 8df31d0.

What I checked:

  • The home header never shows a stale USDT or BTC figure as current, and hidden-balance mode doesn't leak.
  • Switching unit or currency keeps the shimmer, and the layout matches master.
  • The identifiers e2e uses (TotalBalance*, MoneyText, MoneyFiatSymbol) are unchanged. TotalBalanceUpdating is now a clear accessibility overlay with the same label.
  • Parity with synonymdev/bitkit-android#1729: journeys/usdt-cached-home-balance.md is byte-identical to Android, and the constants match (2 s sweep, 0.75 floor, 56 pt band). Only the primary row shimmers on both. iOS keeps the symbol steady with a two-layer draw instead of Android's sibling view.

Perf is relevant. TimelineView(.animation) redraws the mask every frame while the balance is updating, which can last indefinitely offline. Android behaves the same. I'll watch it during the device run.

Device gate (partial): 8df31d0, not run yet. Pending:

  • usdt-cached-home-balance.md J1, with a prepared USDT snapshot and RPC/FX faults.
  • Reduce Motion and the largest Dynamic Type size.
  • CPU while updating offline on Home.

@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): 8df31d0, on the iPhone 17 simulator (iOS 26.5), Debug build. The wallet held 25 USDT on the local Arbitrum fork. To fault the USDT refresh I stopped the fixture gateway; FX stayed live.

  • ✅ Updating with a cached value. A cold launch while offline shows the cached total including USDT $25.00, and TotalBalanceUpdating is present.
    • Only the digits shimmer: a 2 s period, with a floor at 76.5% of the settled luma. The ₿ symbol, the secondary row and the SAVINGS strip stay constant for 50 s.
    • The rows sit at the same y in the settled and updating states, so nothing jumps.
  • ✅ Hidden mode. The dots shimmer and ₿ stays steady. After revealing, USDT still shows $25.00.
  • ✅ Unit switch while updating. BTC → fiat → BTC carries the shimmer straight over, the symbol stays steady, and the rows below don't move.
  • ✅ Reduce Motion. The digits dim with no sweep, and the screen doesn't redraw at all (one frame in 12 s).
  • ✅ Largest Dynamic Type. The shimmer mask clips nothing. The headline is fixed size. A wallet row overlapping the USDT label at this size is [Bug]: Home overlaps wallet rows and shimmers wrapped currency symbol at maximum text size #1176, tracked separately.
  • ✅ Recovery. After restarting the gateway (with +5 USDT funded meanwhile), the digits fade to full brightness by about 30 s, TotalBalanceUpdating goes away, and the header settles at USDT $30.00 with no jump.
  • perf, head only: while updating offline on Home, CPU averaged 9.8% (max 31.7%) over 2 minutes. Settled online, it averaged 1.7% (max 6.9%). The host was heavily loaded during these samples. The master baseline is still running.
[2026-10-30 21:20:11.847 UTC] WARN⚠️: Failed to refresh USDT: Could not update USDT. Check your connection and try again. - UsdtWalletManager [UsdtWalletManager.swift: refresh(includeHistory:) line: 213]

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving at 8df31d0, with one LOW open (perf, inline). Fix it here or move it to a follow-up issue, your choice; reply on the thread with which.

Device gate: 8df31d0 on the iPhone 17 simulator (iOS 26.5), against the local USDT fixture with the gateway stopped and FX live. The balances were real fork balances (25 USDT, then 30).

  • ✅ usdt-cached-home-balance.md J1 steps 2–3 and recovery. While updating, the header shows the cached total and the USDT tile. Only the digits shimmer, with a 2 s sweep and a floor at about 76%.
  • ✅ Frame brightness measurements show the symbol, the secondary row and the rows below never move, through start, stop, hide, a unit swap and a two-line wrap. Master pushes everything down about 25 pt for its caption.
  • ✅ Hidden mode: the dots shimmer and the eye stays fixed. Recovery fades to the live $30 total with no jump.
  • ✅ Reduce Motion: the digits only dim, with no redraws. At the largest Dynamic Type size nothing is clipped. The wallet-row overlap (#1176) and the tight gap above SAVINGS are the same on master.
  • perf: while updating offline on Home, mean CPU was 8.1–11.8% (master 1.6–1.7%). Settled, it was 1.7%. See the inline LOW.
  • ⚠️ Not run: J1 steps 4–6. This PR doesn't touch them.
  • l10n: no UI text changed.

Comment thread Bitkit/Components/MoneyText.swift Resolved
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.

5 participants