Repository navigation
fix: stabilize usdt wallet and payment layouts - #1565
ben-kaufman wants to merge 11 commits into
Conversation
 [Medium impact] The changes affect several wallet and payment screens within the Android app. The PR appears safe to merge, with a non-blocking layout issue on shorter amount sheets. Findings
SummaryThis PR makes the USDT wallet background opaque and shares hardware vendor labels across Receive tabs.
Reviews (1) · Last reviewed commit: "fix: stabilize usdt wallet and payment l..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Gives the USDT wallet an opaque background so the default slide no longer shows Home through it, keeps the Receive footer and hardware tab label stable on the USDT tab, and moves the request/subscription BTC/USD and unit controls into one trailing FlowRow above the divider. Reviewed 6da6a73, full tier, reasoned from the code and CI; the branch was not built here.
What I checked, and 5 candidates I ruled out
Read in full: CreatePaymentRequestScreen.kt (amount content and asset selector), UsdtDepositReceiveScreen.kt, the USDT branch and tab row of ReceiveQrScreen.kt, UsdtWalletContent in UsdtWalletScreen.kt, NumberPad.kt sizing, FillHeight in Spacers.kt
Call sites traced: PaymentRequestAmountContent (Receive sheet request flow and CreateSubscriptionScreen), UsdtDepositReceiveScreen (only ReceiveQrScreen), the Routes.UsdtWallet entry (deepLinkableComposable, default slide transitions)
CI: lint, detekt, build-local green; build and the local e2e shards still running at review time
Ruled out
- USDT wallet still fades: the route already uses the default slide transitions; the fade in #1519 was the transparent root
Boxshowing Home underneath, andColors.BlackmatchesSavingsWalletScreenandSpendingWalletScreen. - Hardware label lost on other USDT pages: the tab row only renders on
ReceivePage.ADDRESS, and the same rememberedtabLabelsmap now feeds both tab rows. - Extra 16dp bottom spacer shifting the USDT sub-pages: it sits after every
UsdtDepositReceiveScreenpage equally and mirrors theVerticalSpacer(16.dp)under the Bitcoin footer button. - Wrapped controls breaking the new test's edge assertions: with
Arrangement.spacedBy(8.dp, Alignment.End)every wrapped line ends at the row's right edge, sodivider.right == control.rightholds whether or not the BTC row wraps. - Continue clipped on short sheets: the column does not scroll and
NumberPadsizes from the whole box height, so the two newFillHeightminimums add 28dp to the USD layout's fixed height (the BTC layout nets 28dp minus the unit row it dropped). Already raised in Greptile's thread on the Continue spacer, so not repeated here; the test only covers a 640dp box.
Merge confidence: 4/5, not built here and CI build/e2e still running; the short-sheet Continue question is open in the existing thread.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ⛔️ Request changes: the amount keypad becomes unreadable on short screens with large text.
The PR moved to df4d002 after this run; these results are for b4b8d2a.
Findings:
- MEDIUM
app/src/main/java/to/bitkit/ui/screens/paymentrequests/CreatePaymentRequestScreen.kt:255: the request and subscription keypad shrinks with no floor.heightIn(max = 300.dp).fillMaxHeight()givesNumberPada fixed height, so it splits it into four rows with no minimum (NumberPad.kt:117). The otherNumberPadcall sites passavailableHeightand get 50–75dp keys.- The column does not scroll, so the keypad absorbs every dp that the larger amount and the wrapped BTC/USD row take.
- On device, BTC digits are clipped at 360×640dp with 3-button navigation and 2.0× text (25dp keys), at 320×640dp with 2.0× and at 320×568dp with 1.6×. At 320×568dp with 2.0× the keypad is 6dp tall.
- Continue stays fully visible in all of them, so the point in Greptile's thread is fixed, but the shortfall moved into the keys. A key floor plus a scrollable column, as synonymdev/bitkit-ios#1003 does with its
ScrollView, keeps both.
- LOW
app/src/main/java/to/bitkit/ui/screens/wallets/usdt/UsdtActivityDetail.kt:119: two USDT routes still show Home through their slide, the #1519 symptom.- A USDT row tapped on Home opens
Routes.UsdtActivity, which rendersUsdtActivityDetailwith a rootColumnthat has no background. - Cross-Network Deposits (
Routes.UsdtDeposits,UsdtDepositsScreen.kt:223) has the same gap. - This PR fixes it only for
UsdtWalletContent. On device both slide in with Home's balance and activity rows showing through, while Bitcoin activity detail is opaque.
- A USDT row tapped on Home opens
QA for b4b8d2a: 2 of 3 tests passed, on two Pixel 9 emulators (API 36) with a paired Trezor (emulator). Test 1 failed on its compact, enlarged-text step.
Test 1: paykit amount layout ❌ Layout and 1.50 USD pass at full size, but the keypad digits are clipped on short screens with large text.
- At 411×923dp the request Amount (Receive › contacts › Bob) and the subscription Amount match the journey in BTC and USD:
- the amount is leading-aligned
- BTC/USD and the unit button are trailing-aligned below it
- a divider sits above the keypad, with Continue below
- 1.50 USD stays 1.50 USD on request details. A USD subscription keeps only USDT selected, with Savings showing +. Nothing was sent.
- I opened a fresh sheet for each size. Continue was fully visible at every size. Key heights by size:
- 320×640dp, 1.6×: 35dp, legible
- 360×640dp, 3-button, 2.0×: 25dp, clipped
- 320×640dp, 2.0×: 20dp, clipped, and the BTC/USD row wraps to two lines
- 320×568dp, 1.6×: 18dp, clipped
- 320×568dp, 2.0×: the keypad is 6dp tall
- The still shows 320×640dp/1.6× and 360×640dp/2.0× on top, and 320×568dp at 1.6× and 2.0× below. Taps on the clipped keys still register: 1000 sats entered at 360×640dp/2.0×.
case1-request-amount-2x.2x.mp4
case1b-subscription-amount-2x.2x.mp4
case1c-compact-keypad-2x.2x.mp4
Test 2: usdt wallet receive layout ✅ The USDT wallet slides like Savings, the Receive footer holds on every tab, and the Trezor label holds on USDT.
- Animations were slowed 5× to show the slide. USDT opens and closes over Home as opaquely as Savings.
- Show Details kept the same on-screen bounds, read from the layout dump, on Trezor, Savings, Spending and USDT across 10 switches.
- USDT shows Network: Arbitrum One. The network list opens and returns with the network still visible, and Show Details opens details.
- The hardware tab reads "Trezor" on every tab, USDT included.
- The same holds at 320×640dp with 1.6× text.
- Side check for the LOW finding: the still shows USDT activity detail and Cross-Network Deposits mid-slide over Home. The third video shows Bitcoin activity detail sliding in opaque for comparison.
case2-wallet-receive-2x.2x.mp4
case2c-compact-receive-2x.2x.mp4
case2b-usdt-siblings-2x.2x.mp4
Test 3: contact request or pay ✅ Only the selected row loads, all rows are disabled, a second tap opens nothing, and a failure re-enables selection.
- Contact Detail Pay opens Request or Pay. Pay shows a spinner with Request disabled, and the send amount sheet opens about 2.5 s after the tap. Request opens the Payment Request amount screen.
- In Send › Contact, tapping Bob puts the spinner on Bob's row only, and both rows are disabled. Tapping the other contact meanwhile changes nothing, and one amount sheet opens for Bob.
- A contact with no published endpoints shows "Unable To Pay Contact" after about 15 s of row spinner, and both rows are enabled again. Nothing was paid.
case3-contact-select-2x.2x.mp4
Coverage:
- The review covered
fccb1f9..b4b8d2ain full, and every changed screen was driven. - 3c1adce, pushed during this run, changes the amount layout and is not covered here.
PaymentRequestAmountLocaleTestpasses at this head because it checks that the keys are displayed, not their size.
Twin: synonymdev/bitkit-ios#1003 makes the same layout changes, and its three journeys match this PR's. It scrolls the amount screen at large text; Android has no scroll here (finding 1).
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- Amount entry:
PaykitAmount.parseInput, the decimal key and the separator display are unchanged, and the typed amount is the amount that reaches Continue. PaymentRequestDenominationLayoutmeasures both denominations and places only the active one. The hidden one has its semantics cleared and is not hit-testable.- The
BringIntoViewSpecoverride only stops auto-scroll while the keypad has focus, so scrolling to Continue still works. ReceiveQrScreentab labels are shared, and no address or URI state is touched.- The USDT wallet, activity and deposit changes are background-only.
- No state is cleared, so there are no Back or cancel paths to check.
The two new journeys are byte-identical to synonymdev/bitkit-ios#1003. contact-request-or-pay.xml differs only in platform tags.
Device gate (partial): not run yet. Pending: paykit-amount-layout.xml, usdt-wallet-receive-layout.xml and contact-request-or-pay.xml, plus a Figma check of the changed screens. Perf: not relevant. The device result follows in a second review.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete PR diff against merge base fccb1f9a, at 2291408. No earlier assessment baseline was available.
No new actionable code findings.
The amount sheet scrolls and keeps the keypad on its normal 50–75dp key height, so a short screen reaches Continue without shrinking the keys. USDT wallet, activity, and deposit details use an opaque black root under the same slide as the other wallets. The USDT Receive tab keeps the hardware vendor label and places Show Details on the same 16dp bottom inset as the Bitcoin tabs. The contact journey text matches the existing selected-row loading and duplicate-payment guard. Compared with bitkit-ios#1003 at b087c501 for this amount and Receive layout.
The keypad-clipping comments on b4b8d2a are addressed at this revision (thread). Build succeeded at this head. This review inspected PaymentRequestAmountLocaleTest and did not run it or a device pass; the author reports those checks passed. Device testing: not performed in this review. Ready for device testing of the three journeys in the PR.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
@ben-kaufman please resolve the merge conflicts. QA review has not been performed for this request.
2291408 to
03e1952
Compare
|
Rebased onto the updated #1564 and resolved the shared journey conflict, preserving both the duplicate-selection check and explicit failure/retry setup. All five rebased commits are signed; the existing layout changes are preserved. Compile, 4,334 unit tests and complete lint pass with no introduced findings. The combined request-card and amount-layout device suite also passes (16 tests, including all 24 amount-layout configurations). Ready for another QA pass. |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the complete PR diff against merge base 595951a0, at 03e1952. The comparison base changed since the saved assessment, so no earlier coverage was inherited.
No new actionable code findings.
The amount sheet scrolls and keeps keypad keys on the component's normal 50–75dp height, so a short screen can reach Continue without shrinking the keys. Focusing the keypad does not scroll the amount off screen. BTC and USD are measured together and only the active one is placed, so switching denomination does not move the controls, divider, or keypad. Continue submits the active BTC view-model amount or the USD string. USDT wallet, activity, and deposit roots are opaque black under the same horizontal slide as the other wallets. USDT Receive uses the shared hardware-tab label and the same 16dp bottom inset as the Bitcoin tabs. The updated contact journey matches the existing selected-row spinner, disabled rows, and endpointless retry.
Compared with bitkit-ios#1003 at b087c501: the amount screen also splits leftover height above and below the keypad, and paykit-amount-layout.xml and usdt-wallet-receive-layout.xml match. The contact journey differs only by platform identifiers and an explicit endpointless retry. On a tall sheet that split leaves the keypad lower than the cited amount frame, which keeps the keypad against the divider; short screens have no leftover space. The keypad-clipping comments on older commits are outdated at this revision (thread).
Build and lint succeeded at this head. This review inspected PaymentRequestAmountLocaleTest and did not run it. Device testing: not performed in this review. Ready for device testing of paykit-amount-layout.xml, usdt-wallet-receive-layout.xml, and contact-request-or-pay.xml.
There was a problem hiding this comment.
Verdict: ⛔️ Request changes: the new compact Receive QR scan step fails.
Findings: No new code findings. Both earlier findings are fixed: the keypad keeps its normal key height and scrolls, and the USDT activity/deposit roots are opaque. The compact QR failure below also occurs with the new Receive bottom inset removed; I am not attributing it to that change.
QA for 03e1952: Android 16 emulators, regtest and test USDT. I built the app at this head. No payment or request was sent.
Test 1: request and subscription amount layout ✅ BTC/USD keeps the controls and keypad stationary, 1.50 USD reaches details, and compact enlarged-text entry scrolls to Continue.
- Both live callers retain the amount. Subscription details select USDT and deselect Savings.
- At 320×568dp with 200% system text, the initial amount remains visible. The wrapped controls and divider keep their positions across BTC/USD. Scrolling exposes the full keypad and Continue; the live keys measure 75dp high.
PaymentRequestAmountLocaleTestpassed on device. Its assertions check every key's minimum height and vertical text fit, stable denomination positions and scrolling across 24 layout configurations, plus exact comma-decimal submission.- Positions and sizes were checked from UI bounds and rendered-text assertions. Both forms were closed without publishing.
amount-normal.2x.mp4
amount-compact.2x.mp4
amount-regression.2x.mp4
Test 2: opaque USDT activity and deposit routes ✅ Received-USDT details and Cross-Network Deposits slide over Home without showing Home through the destination.
I opened the received 25 test-USDT activity from Home and opened Cross-Network Deposits through Receive. Slowed transition frames show an opaque destination in both directions. Deposit history was empty; there was no saved provider-deposit detail to open. History and detail share the changed root background.
opaque-routes.2x.mp4
Test 3: compact Receive Copy and displayed QR ❌ Copy works, but the displayed USDT QR cannot be scanned at 320×640dp with 160% text.
- QR, Edit/Copy/Share, Network and Show Details remain separate. Copy leaves Receive open and puts the exact plain address on the clipboard, verified by pasting it into the contact input.
- Bitkit's gallery scanner rejects the captured display with
No QR code found in the image. Core Image and ZXing also return no decoded code. - At normal size, ZXing decodes the head's captured display to the correct Arbitrum URI and the same receiving address.
- A comparison build removing only this PR's new 16dp Receive bottom inset still produces a compact QR that ZXing cannot decode. The QR renderer is unchanged in this PR, so this is an existing limitation exposed by the new journey step.
compact-usdt.2x.mp4
compact-scan.2x.mp4
Normal-size control:
normal-usdt-control.2x.mp4
Comparison without the new bottom inset:
receive-before-inset.2x.mp4
Retained passing evidence for unchanged paths:
- The earlier review proves the original wallet slide, Receive footer stability, paired Trezor label, contact-detail flow and contact-row loading/failure behavior.
- The stacked PR's contact report proves endpointless failure followed by payable-contact retry. The contact runtime is unchanged here.
No planned case is unrun. A fresh Figma comparison was unavailable. The original hardware-label and footer checks were not repeated because their runtime code is unchanged.
Scope: inspected the requested b4b8d2a..03e1952 tree delta and both earlier findings. Because the branch was rebased, I also re-reviewed the full PR against 595951a0. Test, journey and changelog edits were source-reviewed; the live changed amount and background components were driven.
Twin: synonymdev/bitkit-ios#1003 fix: stabilize contact selection and amount layouts matches the amount stability, scrolling and shared journey intent. Its compact QR fix scales the logo; Android's QR renderer is unchanged. iOS was not rebuilt in this run.
|
Fixed the compact QR scanning failure in 47ffa20. The center logo now scales with the displayed QR bounds, matching iOS #1003. A real composable capture/decode test reproduced the previous ChecksumException and now passes for Bitcoin and USDT (with and without an amount) at 120, 160, 240 and 320dp. QR contents and copy/share payloads are unchanged. All 4,334 unit tests pass, the rendered QR test passes, and the full detekt report has no new findings. Independent review found no remaining issue in this fix. |
|
@ben-kaufman one small addition for |
|
Integrated the updated #1564 parent while keeping this PR stacked on it. The compact amount layout now retains master’s Validation: |
|
Device gate (partial):
Unconfirmed, not findings:
|
|
The Settings shard failed before running tests: Docker could not bind Bitcoin's port |
There was a problem hiding this comment.
Verdict: ⛔️ Request changes: a fresh USD subscription amount ignores digit entry.
Findings:
- MEDIUM
app/src/main/java/to/bitkit/ui/screens/paymentrequests/CreatePaymentRequestScreen.kt:168: choosing USDT on a fresh subscription before opening Edit Amount initializes$0.00. Digit taps leave it unchanged and Continue disabled becauseNumberPadInputHandlerrejects input with both decimal places already filled. Initialize zero drafts with an empty string, matching the existingonConvertedguard. This regression comes from the updated parent integration; the live flow and existing comma-decimal device test both reproduce it.
QA for c735d84: Built at this head and tested on Android 16 emulators with regtest and test USDT. Four cases passed; the amount case failed. No payment, request or subscription was sent.
Test 1: rendered QR decoding ✅ The rendered Bitcoin and USDT codes decode with their logos at compact and normal sizes.
QrCodeImageTest captures the actual composable and asserts exact decoded contents for Bitcoin, plain USDT and amount-bearing USDT at 120, 160, 240 and 320dp.
qr-regression-2x.2x.mp4
Test 2: compact Receive Copy and scan ✅ The previously failing compact USDT display now scans to the matching Arbitrum recipient.
- At 320×640dp and 160% text, QR, Edit/Copy/Share, Network and Show Details remain separate.
- Copy leaves Receive open and pastes the plain address. The contact input lowercases pasted text; its value matches the QR recipient ignoring address letter case.
- Core Image decodes the normal and compact screenshots to the same Arbitrum URI. Bitkit's gallery scan opens USDT amount entry on Arbitrum One; Back exposes the matching recipient address.
- Network selection returns to the selected network, and Show Details opens the address.
receive-normal-2x.2x.mp4
receive-compact-2x.2x.mp4
compact-copy-2x.2x.mp4
compact-scan-2x.2x.mp4
Test 3: USDT wallet slide and taps ✅ The wallet remains opaque and empty-area taps leave USDT in place.
Savings and USDT open with opaque slides. Taps over the underlying Savings card after opening, and Spending during the slowed USDT slide, do not navigate Home. Send opens its scanner. Route and tap results were checked from the recording and UI bounds.
wallet-2x.2x.mp4
Test 4: request and subscription amount entry ❌ Fresh zero-USD entry ignores digits; the layout checks and nonzero value preservation pass.
- Fresh subscription → USDT → Edit Amount displays
$0.00. Tapping 1, 2 and 5 leaves it unchanged and Continue disabled. - Removing the existing decimal zeros permits entry. Both live callers retain
1.50 USDin details; the subscription selects USDT and deselects Savings. - BTC/USD keeps the divider and keypad positions. At 320×568dp and 200% text, the initial amount is visible, keys retain normal height and scrolling reaches the full Continue button.
- The existing device regression passes its layout assertions across all 24 configurations, including key height and rendered-text fit. Its comma-decimal entry test fails at the first entered-value assertion because the initial zero draft ignores input.
subscription-zero-2x.2x.mp4
request-normal-2x.2x.mp4
subscription-normal-2x.2x.mp4
amount-compact-2x.2x.mp4
amount-compact-tail-2x.2x.mp4
amount-regression-2x.2x.mp4
Test 5: contact progress and failure retry ✅ Only the selected contact loads, duplicate taps are ignored, and an endpointless failure allows a payable retry.
- Contact Detail offers Request or Pay. Pay keeps its progress while Request is disabled, then opens amount entry in about three seconds, read from the recording. Request opens request amount entry.
- On the contact list, only the selected row shows progress and both rows disable; their disabled states were checked in the UI hierarchy.
- Tapping the other row during resolution opens no second flow.
- The endpointless contact shows “Unable To Pay Contact”. Both rows re-enable, and selecting the payable contact again opens amount entry. All flows were canceled before payment.
contacts-normal-2x.2x.mp4
contacts-rows-2x.2x.mp4
contacts-retry-2x.2x.mp4
Retained evidence for unchanged paths:
- The earlier review proves the paired Trezor vendor label; the shared label map and tab logic are unchanged.
- The previous delta review proves opaque USDT activity and deposit roots; those background fixes remain unchanged.
No planned case is unrun. A fresh Figma comparison was unavailable. Reviewed 03e1952..c735d84, including parent integration and the earlier QR, keypad and background findings; parent flows outside these journeys and documentation/test edits were source-reviewed.
Parent integration:
- synonymdev/bitkit-android#1564 fix: improve usdt payment feedback and layout supplies the
inputValuechange that triggers the finding above.
Twin:
- synonymdev/bitkit-ios#1003 fix: stabilize contact selection and amount layouts matches the QR logo scaling, stable amount controls, scrolling and shared layout journeys. The pointer modifier is Android-specific. iOS was not rebuilt here.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the complete PR diff against merge base 371478a, at a4ce71c. The comparison base changed since the saved assessment, so no earlier coverage was inherited.
No new actionable code findings.
A fresh non-BTC amount with atomic zero now starts from an empty string, so the first digit is accepted. A zero USD amount's inputValue is 0.00, and NumberPadInputHandler rejects another digit once both decimal places are filled; a nonzero amount still restores inputValue (1.50 for 150 cents). Continue submits that parsed amount. The sheet scrolls, keypad keys stay on the component's 50–75dp height, and leftover height is still shared above and below the keypad. BTC and USD are measured together and only the active one is placed. The USDT wallet blocks taps and, with activity and deposit details, uses an opaque black root under the same slide. USDT Receive keeps the hardware vendor label and the same 16dp bottom inset as the Bitcoin tabs. The contact journey matches the existing selected-row spinner, disabled rows, and endpointless retry.
The same zero-amount initialization is in bitkit-ios#1003 at 1780ff2; that check was limited to this amount contract. Build, lint, and local e2e succeeded at this head. Those e2e shards do not run the new journeys. PaymentRequestAmountLocaleTest and QrCodeImageTest were inspected and not executed here. Device testing: not performed in this review. Ready for device testing of paykit-amount-layout.xml, usdt-wallet-receive-layout.xml, and contact-request-or-pay.xml.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. The earlier zero-dollar entry failure is fixed; fresh USD input accepts digits and existing cents survive reopening.
QA for a4ce71c: Built at this head and tested on Android 16 emulators with regtest and test USDT. No payment, request or subscription was sent.
Test 1: actual amount component regression ✅ Fresh zero-USD entry accepts digits, exact comma-decimal cents submit, and compact layout assertions pass.
PaymentRequestAmountLocaleTest passes, including the previously failing decimal-entry test. Its assertions check initial amount visibility, hardware input, stable BTC/USD positions, keypad height and text fit, and scrolling to Continue across the compact/enlarged-text matrix.
amount-regression-2x.2x.mp4
Test 2: live request and subscription entry ✅ Both callers retain 1.50 USD through details, reopening and BTC/USD switching.
- A fresh subscription selects USDT before Edit Amount and accepts
1.50without deleting first. Its details retain USDT as the selected method. - The request editor also accepts
1.50from zero. Both editors reopen with the cents intact and preserve them through a BTC/USD round trip. - At 320×568dp with 200% text, the subscription's initial amount remains visible; scrolling reaches the keypad and full Continue button. Both forms were canceled before publishing.
amount-live-2x.2x.mp4
Test 3: Receive layout, QR and swipe integration ✅ Compact Copy and scan retain the Arbitrum recipient, footer bounds match, and QR swipes change tabs without copying.
- At normal size and 320×640dp with 160% text, Savings and USDT keep Show Details at the same position. Compact button bounds were checked in the UI hierarchy. QR, Edit/Copy/Share, Network and Show Details remain separate.
- Core Image decodes both captured displays to the same Arbitrum payment URI. Copy keeps Receive open and pastes the matching plain address; comparison ignores the contact input's lowercasing.
- Bitkit's gallery scanner accepts the compact capture, opens USDT entry on Arbitrum One, and Back shows the matching recipient. Network selection and Show Details also work.
- Swiping right over the USDT QR selects Spending without a copied tooltip. Swiping back returns to USDT; a left swipe on the last tab stays there without copying. A single QR tap still copies.
receive-standard-2x.2x.mp4
receive-compact-qa-2x.2x.mp4
Retained passing evidence for unchanged paths:
- The previous review proves rendered QR decoding, wallet opacity/tap protection and contact progress/failure/retry. Those runtime paths are unchanged.
- The earlier hardware check proves the Trezor vendor label; its shared label map and binding are unchanged.
- The activity/deposit check proves the unchanged opaque route backgrounds.
No planned case is unrun. A fresh Figma comparison was unavailable. Reviewed the contiguous c735d84..a4ce71c delta and the earlier amount finding; integrated parent behavior outside these journeys and documentation/test edits were source-reviewed.
Twin: synonymdev/bitkit-ios#1003 fix: stabilize contact selection and amount layouts at 1780ff2 uses the same zero-dollar guard and identical amount journey. iOS was not rebuilt here.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at a4ce71c: no issues found. I reviewed 47ffa20 and a08ca54, plus the two merges with --remerge-diff; the rebased commits match what I reviewed before.
Checked and clean:
- Amount: the amount shown before Continue is the amount submitted. BTC uses the view-model sats and USD uses
parseInput(USD, dollars). Continue is disabled at zero and submitsinitialDraft.copy(amount = it). - Zero draft: a zero USD draft starts empty, so the first digit is accepted (
CreatePaymentRequestScreen.kt:167-175). A nonzero draft keeps its cents, and only the display uses the locale separator. c735d84merge: keeps master'sinputValueinonConvertedand this PR'sNumberPadAmountText.- QR logo:
min(68dp, 0.2 × min(maxWidth, maxHeight)). QR content, copied text, shared bitmap and tooltip are unchanged. NoQrCodeImagecaller sits underIntrinsicSize, andProfileScreen/AddressViewerScreenpass no logo. blockPointerInputPassthrough(): observes without consuming, as on Savings and Spending, so the screen's own controls still get taps.- Back: no state is cleared, so there are no new Back or cancel paths.
- Parity: the zero-draft guard and the logo scaling match synonymdev/bitkit-ios#1003. The two new journeys are byte-identical.
Perf: not relevant. l10n: no strings added or changed. The longest labels on these screens (common__show_details in el, wallet__payment_request_rate_unavailable in el/ru/ar) wrap, and the unit controls sit in a FlowRow.
Device gate (partial): a4ce71c — not run yet. Pending:
paykit-amount-layout.xml,usdt-wallet-receive-layout.xmland the updatedpayment-requests/contact-request-or-pay.xml;- comparing these Figma frames with the running screens: Payment Request › Request payment enter amount, Receive Lightning Active › Receive Auto (Unified QR), Savings, Spending, Hardware, USDT Balances › Wallet USDT (funds & activity).
Device results will follow in a separate review.
|
Device gate (partial):
Notes on step 2: from Home, the USDT activity detail slides in. Inside the USDT wallet list it is an in-screen state swap ( Notes on step 7, both on master and not touched by this PR:
Still running: step 5, |
|
Integrated the updated #1564 parent in 83d34cd, resolving the stacked-branch conflict while keeping this PR based on #1564. The integration preserves master's app-locale handling and Receive wraparound gestures alongside the amount and footer layouts. Validation: required compilation, all 4,375 unit tests, zero Detekt findings and zero Android Lint issues. Both locale/layout component tests pass on the isolated emulator, including all 24 compact/enlarged-text configurations. The German-locale fixture now supplies the app configuration that production uses. No payment or wallet reset was performed. |
|
Device gate (partial 2):
Seen while testing, not from a journey action: changing the display size or font scale while the Payment Request sheet is open recreates it at Amount with 0 BTC, which loses the entered 1.50 and the details step. Still running: step 14, step 5 of |
|
Device gate (partial 3):
Figma, with frames from the Refactor v63 page (
No delta comes from this PR. Still running: step 14 of |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the complete PR diff against merge base 43be1a92, at 83d34cd. The comparison base changed since the saved assessment, so no earlier coverage was inherited.
No new actionable code findings.
A zero non-BTC amount starts from an empty input, so the first digit is accepted, while a positive amount still restores its input text. Both denomination rows are measured and only the active one is placed, with semantics cleared on the hidden row. The keypad keeps its normal key height inside a scrolling column, and keypad focus does not pull the amount off screen. Compact QR logos scale to 20% of the displayed code, capped at 68dp. The USDT wallet, activity, and deposit screens are opaque, the wallet blocks touch passthrough, and USDT Receive keeps the hardware vendor label and the same 16dp footer inset as Bitcoin.
Validation: PaymentRequestAmountLocaleTest and QrCodeImageTest were inspected at this revision and not executed. Detekt and Android Lint succeeded on this commit. Device testing: not performed in this review.
Compared with bitkit-ios 56635438: a zero USD amount there is still initialized from its formatted input, and the QR logo stays a fixed 68pt.
Ready for device testing.
There was a problem hiding this comment.
Approving. No issues found; the code-scan trace is in my earlier review today. The 83d34cd / 43be1a9 merges resolve import-only conflicts, so the device results from a4ce71c carry over. contact-request-or-pay.xml ran on 83d34cd.
Device gate: 83d34cd — journeys: 27 passed, 1 failed on environment (usdt-wallet-receive-layout.xml 7/7, paykit-amount-layout.xml 7/7, payment-requests/contact-request-or-pay.xml 13/14, step 7 over its 3 s budget on master too); perf: contact → amount screen 3.7-7.1 s vs master 4.4-6.6 s; Figma: 3 frames compared (Request payment enter amount, Unified QR, Wallet USDT), no delta from this PR; l10n: no UI text changed, checked at 360×640 dp with font 1.3 and 2.0; iOS: synonymdev/bitkit-ios#1003 has one LOW (QR offset) open.
Ran on Pixel_9 (emulator), with the Trezor emulator paired over Bridge for the hardware-tab step.
- Step 7 of
contact-request-or-pay.xml("amount screen within 3 s") misses its 3 s on master too. All the time sits beforeOpened private Paykit payment…, which is the private payment-list fetch from the staging homeserver. That makes it environment latency, not this PR; the journey's 3 s budget doesn't hold on staging. - Step 14: a second tap while one contact loads starts no second flow. The no-endpoints contact shows "Unable To Pay Contact" after about 31 s of public resolution (
PrivatePaykitRepo.kt:1172→:866), and the list is selectable again afterwards. - Seen while testing, not from this PR: changing the font scale or display size with the Payment Request sheet open recreates it at Amount with 0.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at aba8c0f after no verified baseline coverage was supplied and earlier reviews used different merge bases. The complete diff against merge base afcf6ff was inspected, including amount entry, compact QR logos, USDT opacity and tap blocking, Receive footer alignment, and the contact-selection journey wording.
No new actionable code findings.
Fresh zero-dollar request and subscription amounts start empty so the first digits register, while a nonzero amount still restores inputValue and its cents. The keypad keeps its normal key height in a scrolling body, and keypad focus does not scroll the amount away. The center logo is 20% of the displayed QR, capped at 68dp. USDT wallet, activity, and deposit roots are opaque black, and the wallet blocks tap passthrough the same way as Savings and Spending. The USDT Receive tab keeps the hardware vendor label and the same 16dp footer inset as Bitcoin. The updated contact journey matches the existing single-resolution guard.
This review did not execute tests or a device. The layout and journey sources match 83d34cd, where an earlier device pass reported the new amount and Receive journeys passing; that run was not repeated here. The final parent merge only brings contact and receipt files that match the base. iOS default bcaa461 was compared for QR overlay size and zero-dollar seeding only; bitkit-ios#1003 was not reviewed.
Device testing: not performed in this review.
Ready for device testing.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Approve at aba8c0f. Current-head code review found no actionable issues. Earlier device coverage remains applicable to the unchanged amount and Receive layouts, including fresh USD entry, preserved cents, compact scrolling, QR decoding and hardware labels.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for aba8c0f: Android 16 emulators (Pixel 9), with the app and instrumentation built at this head. Recordings play at 2×. No payment, request or subscription was sent.
Test 1: amount locale and compact layout ✅ Fresh comma-decimal input submits exact cents, and the layout regression passes.
PaymentRequestAmountLocaleTest exercises the production amount component with the app's German configuration. Its assertions verify exact 150.50 USD submission, initial amount visibility, hardware-key input, stable BTC/USD controls, keypad height and text fit, and scrolling to Continue across compact sizes and enlarged text.
Test 2: Contact Detail preparation timing ✅ Pay shows progress and opens the amount screen within the three-second target.
On warmed, mutually linked test wallets with regtest funds, Pay shows its spinner while Request is disabled. A timestamped full-screen capture shows Bitcoin Amount fully visible by 2.94 seconds after the touch began; the recording shows the same transition. Back cancels without paying. This proves the target for this run; earlier staging runs reported slower preparation.
Retained passing evidence for unchanged paths:
- The earlier amount and Receive review proves live fresh request/subscription entry, preserved cents, compact Copy and displayed-QR scanning. The locale integration is rechecked above.
- The completed layout journeys prove wallet/activity opacity, Receive footer and network controls, and the paired Trezor vendor label. Those implementations are unchanged since 83d34cd.
- The parent's contact device report proves the merged contact handoff survives rotation and font-scale recreation without reopening after Back. Its report also covers duplicate-selection prevention and endpointless failure/retry.
- The parent's device review proves Receive edge-tab wraparound and QR tap behavior in the exact code integrated here.
Scope: contiguous a4ce71c to aba8c0f delta, including both parent merges and the earlier findings. Other merged-base UI, resource and lint changes were source-reviewed only; no fresh Figma comparison was performed.
Pair PR: synonymdev/bitkit-ios#1003 fix: stabilize contact selection and amount layouts at 17e6533 matches the zero-dollar guard, proportional QR logo and shared layout-journey intent. iOS was source-compared only.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
This branch has not been deployed
5 participants
Fixes #1519 and #1535. Refs #1533. Stacked on #1564.
Description
Out of Scope
Design
Request amount — Handoff v62 and amount layout — Refactor v63. Reuses the existing wallet navigation, typography, buttons and divider. The selector styling remains unchanged pending design confirmation.
Preview
Before/after captures were taken at 320/375dp and normal/1.6× text. Live Receive captures show matching footer bounds for Bitcoin and USDT at both text sizes and the visible Arbitrum network. Screenshots are available in the author’s local review gallery.
QA Notes
Journeys
paykit-amount-layout.xml— request/subscription alignment, fresh USDT amount entry, preserved cents, compact controls, divider and enlarged-text reachability.usdt-wallet-receive-layout.xml— wallet slide, Receive footer stability and hardware vendor label.contact-request-or-pay.xml— consistent selected-row progress and duplicate-payment guard, with an explicit endpointless-contact failure and retry.Manual Tests
N/A
Automated Checks
QrCodeImageTest.kt— decodes the actual rendered QR and logo for USDT, an amount-bearing USDT request and Bitcoin at 120/160/240/320dp. All 12 cases pass on the emulator; the compact case failed before the fix.PaymentRequestAmountLocaleTest.kt— checks amount/control edges, compact gaps, stable BTC/USD positions, every keypad key’s minimum height and vertical text fit, and scrolling to Continue across 24 combinations of 320–375dp widths, 468–640dp content heights and 1×/1.6×/2× text. Checks initial amount visibility and hardware-key input before scrolling; exact decimal input coverage remains.UsdtBridgeDetailsTest.ktcases covering activity/deposit summaries, details and enlarged-text refund input.Live wallet open/back and Receive switching passed on the dedicated empty emulator. Hardware label propagation was source-reviewed; a paired-hardware tab-switching journey was not rerun. With the updated #1564 parent integrated, required compilation and all 4,375 unit tests pass. Detekt and Android Lint both report zero findings, with master's baseline removal retained. Both
PaymentRequestAmountLocaleTest.ktcases pass on the dedicated API 36 emulator, including all 24 compact/enlarged-text configurations, exact decimal entry and hardware-key input. The fixture supplies the German app configuration used by master's locale handling. Existing numeric amounts retain their cents.The current parent integration preserves the stack and changes only the four contact/receipt files from #1564; their content matches the independently reviewed parent exactly. Compilation, all 4,375 unit tests, Detekt and Android Lint pass on this final integration, with zero lint findings. The parent’s 16 payment-list/contact component tests pass, including both reproduced regressions. The amount and Receive layout implementations are unchanged by this integration.