Repository navigation
fix: improve usdt payment feedback and layout - #1564
ben-kaufman wants to merge 12 commits into
Conversation
 [Medium impact] The changes affect several payment and USDT screens within the wallet app. The PR appears safe to merge, with non-blocking improvements needed for accessible loading feedback and regression coverage. FindingsSummaryThis PR improves USDT payment feedback, request-card layout, and receive sharing.
Two non-blocking improvements remain: expose loading to TalkBack and test the new receive-amount conversion. Reviews (1) · Last reviewed commit: "fix: improve usdt payment feedback and 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. Shows per-contact progress while a contact payment resolves and blocks a second selection, gives the request card's amount/status column bounded width, pads USDT confirmation and incoming activity actions above the navigation bar, and shares the direct-receive amount from its parsed atomic value. Reviewed 9112565, full tier, reasoned from the code and CI: Bitkit Android branches are not built here.
What I checked, and 6 candidates I ruled out
Read in full: SendContactSelectViewModel.kt, SendContactSelectScreen.kt, PubkyContactRow.kt, PaykitAmountView.kt, Money.kt (MoneyCell), PaymentRequestCard in PaymentRequestsScreen.kt, PaykitUsdtReview.kt, UsdtActivityDetail.kt, the ADDRESS page of UsdtDepositReceiveScreen.kt, UsdtReceiveContent.kt
Call sites traced: PubkyContactRow (send contact select, create request, create subscription), PaykitAmountCell / MoneyCell (request card, create request, subscription row), UsdtActivityDetail (wallet screen route, send review), PaykitUsdtReview (SendRoute.UsdtReview)
CI: lint, detekt, build-local and Greptile green; build and the local e2e matrix still running when this was posted
Ruled out
- Double tap starts two resolutions: the null check and
resolvingPublicKeyupdate happen synchronously beforelaunch, so a second tap returns early;finallyclears it on success, failure and cancellation. - Selection stays locked after navigating to the amount screen: the state clears in
finallyright after the effect is emitted, so returning to the list allows another pick, which the new view model test asserts. - Other
PaykitAmountCellcallers change width:modifierdefaults toModifier, and only the request card passesweight(1f);TextAlign.EndinMoneyCellonly changes wrapped lines. - Double navigation-bar inset: neither
BottomSheetnor the nav host applies navigation-bar insets, and sibling send screens in the same sheet (SendAmountScreen,SendAddressScreen) pad themselves the same way. - Share text drops a typed amount: an unparsable amount now shares no amount, but the QR then carries no
uint256either, so share and QR agree. - Contact rows gain a role they should not have: rows with
isSelected == nullare clickable, soRole.Buttondescribes them; radio semantics are unchanged whereisSelectedis set.
Merge confidence: 4/5, the navigation-bar fixes are covered only by device journeys, and CI's build and e2e had not finished.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 💬 Comment. Two MEDIUM findings: the contact picker unlocks before the amount screen opens, and payment request rows now cut off contact names. Test 4 did not run before 95f57d4 was pushed.
The PR moved to 95f57d4 after this run; these results are for 9112565.
Findings: 2 MEDIUM, 1 LOW. 95f57d4 does not touch these lines.
-
MEDIUM
app/src/main/java/to/bitkit/ui/screens/wallets/send/SendContactSelectViewModel.kt:98: the picker unlocks before the payment opens.finallyclearsresolvingPublicKeyas soon asOpenPaymentis emitted.openContactPaymentthen runs a scan job that decodes the endpoints and refreshes USDT before it navigates.- In Test 1 the spinner disappeared about 0.5 s before the amount screen slid in. Per the code, every row is enabled again in that gap.
- #1525 asks to keep selection locked until navigation.
-
MEDIUM
app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt:628: the secondweight(1f)splits every request row 50/50, even for a short amount with no long status.- Contact names and subtitles that fit next to a short amount can now end in "…" on every
PaymentRequestCardcaller. - At 130% text on a Pixel 7, "Alexandra Payer" shows as "Alexandra P…" beside
$2.00 pending, with half the row empty.
- Contact names and subtitles that fit next to a short amount can now end in "…" on every
-
LOW
app/src/main/java/to/bitkit/ui/screens/wallets/usdt/UsdtActivityDetail.kt:182: only the footer gets the navigation-bar inset.- The summary view still scrolls to the bottom of the screen.
- On the compact emulator at 200% text with three-button navigation, Details stays behind the bar when scrolled to the end.
QA for 9112565: two Android 16 emulators, a compact Small Phone (360dp) as payer and a Pixel 7 as requester, on a local USDT stack (Arbitrum fork and bundler).
Test 1: Contact picker progress and retry ✅ The tapped contact shows a spinner while the other rows are disabled. A failed lookup re-enables them, and a retry opens the amount screen.
- A contact with no payment endpoint:
- The spinner showed on its row, and both rows were disabled.
- After about 12 s the lookup failed with "No compatible payment endpoint is available." Both rows were then enabled.
- Bartholomew Requester then showed the spinner and opened the amount screen.
- Back from the amount screen returned to the picker with both rows enabled. Nothing was paid.
- I read the spinner and the rows' enabled state from UI dumps taken during resolution.
test1-contact-picker.2x.mp4
Test 2: USDT receive of 3.50 ✅ The QR keeps uint256=3500000. Both Shares send 3.5 USDT.
- After entering 3.50, the QR encodes
ethereum:0xFd08…@42161/transfer?address=0xe72D…eD9c&uint256=3500000. - Share on the QR and Share in Show Details both preview "Network: Arbitrum One", the address and "3.5 USDT".
- The QR content comes from its content description on screen. The share text comes from the system share sheet's preview.
test2-receive-share.2x.mp4
Test 3: USDT confirmations and incoming Details above system navigation ✅ Swipe To Pay and Cross-Network Deposits stay above the bar in three-button and gesture modes.
- All on the compact emulator, in three-button and gesture modes:
- the contact USDT payment, with details collapsed and expanded
- a one-time $2 USDT request, with details collapsed and expanded
- the first installment of a $3 monthly USDT subscription, collapsed and expanded in three-button mode and expanded in gesture mode
- In received USDT activity Details, Cross-Network Deposits sits above the bar in both modes.
- Each payment was cancelled without paying.
test3-confirmations-and-details.2x.mp4
Test 4: Payments list with a long verified-USDT status ⏭️ Not run: 95f57d4 was pushed while Test 3 ran.
- It needs a USD request and a BTC request, each paid in USDT and verified, then the Payments list in Polish at 130% text.
Not tested on device:
- The iOS-only actions added to
usdt-bridge-details.xmlandusdt-payment-links-and-scans.xml. - The long verified-USDT status from #1537 (Test 4 did not run).
Reviews I relied on:
- Greptile raised the TalkBack loading state in this comment. 95f57d4 addresses it, so it is not repeated here.
- Talos's review found no findings. The fix asked for in the first finding keeps the behaviour that review checked: the picker unlocks again after Back from the amount screen.
Scope: a full review of diff, 19 files.
No pair PR: synonymdev/bitkit-ios#1002 is a separate iOS accessibility fix. iOS already shows the contact spinner and shares 3.5 USDT.
There was a problem hiding this comment.
Verdict: ⛔️ Request changes. Request cards now cut a long amount to "…" (Test 5). The three findings from our last review are fixed.
Findings: 1 MEDIUM, 1 LOW.
-
MEDIUM
app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt:631: request cards cut a long amount to "…".- The amount column is capped at half of the free width, and its amount is held to one line with an ellipsis.
- Before this PR the amount took the width it needed.
- On a 360dp phone at 130% text, the incoming 1,000,000-sat request reads "₿ 1 000 0…" next to Pay. At 100% the same amount just fits.
- The cap applies to every
PaymentRequestCard: the Payment Requests sheet, the Payments tab, a subscription's payment history and the Sent receipt.
-
LOW
app/src/main/java/to/bitkit/ui/components/Money.kt:99: the status that replaces a BTC request's fiat amount breaks words.- With fiat as the primary unit, the receipt status takes the fiat amount's bold style and wraps inside the capped column.
- On a Pixel 7 in Polish at 130% text, the 5,000-sat request paid in USDT reads "Płatność / zweryfikowan / a · 4,12485 / USDT".
- With Bitcoin as the primary unit, the same status is the smaller second line and wraps between words.
Earlier findings, checked at fccb1f9: all three are fixed. The replies are in their threads.
- The contact picker keeps the spinner until the amount screen slides in (Test 1).
- A short amount leaves the rest of the row to the contact: "Alexandra Payer" shows in full beside "$2,00" at 130% text (Test 4).
- Activity details get the navigation-bar inset in the summary view too: Details clears three-button navigation at 200% text (Test 3).
QA for fccb1f9: two Android 16 emulators, a compact Small Phone (360dp) as payer and a Pixel 7 as requester, on a local USDT stack (Arbitrum fork and bundler).
Test 1: Contact picker progress and retry ✅ The tapped contact keeps its spinner until the amount screen slides over it. A failed lookup re-enables the rows for a retry.
- A contact with no payment endpoint:
- The spinner showed on its row and both rows were disabled. A tap on the other row did nothing.
- After about 10 s "Unable To Pay Contact" showed, and both rows were enabled.
- Bartholomew Requester, three times:
- The spinner stayed on until the amount screen started sliding in. No frame shows the picker idle in between.
- A tap on the other row during resolution started nothing.
- Back returned to the picker with both rows enabled. Nothing was paid.
- I read the rows' state from UI dumps and the handoff from 10 fps frames of the recording. The still shows four frames 0.1 s apart.
test1-contact-picker.2x.mp4
Test 2: USDT receive of 3.50 ✅ The QR keeps uint256=3500000. Both Shares send 3.5 USDT.
- After entering $ 3.50, the QR encodes
ethereum:0xFd08…@42161/transfer?address=0xB771…963b&uint256=3500000. - Share on the QR and Share in Show Details both preview "Network: Arbitrum One", the address and "3.5 USDT".
- The QR content comes from its content description on screen. The share text comes from the system share sheet's preview.
test2-receive-share.2x.mp4
Test 3: USDT activity Details above system navigation ✅ In both navigation modes and both hosts, the last button sits above the bar, also at 200% text.
- The received $1,500 USDT, opened from the USDT wallet on the compact phone:
- Summary scrolled to the end: Details clears the bar in gesture and three-button modes, at 100% and 200% text.
- Expanded: Cross-Network Deposits clears the bar in both modes, at 100% and 200% text.
- The sent $5 payment, opened from Details on "USDT Sent" in the send sheet:
- Details and Open Block Explorer clear the bar in both modes.
- The gap under the last button is 16dp at most, so neither host gets the inset twice.
- I read the button and bar edges from full-resolution screenshots.
test3-activity-details.2x.mp4
Test 4: Payments list with a long verified-USDT status ✅ In Polish at 130% text, both verified statuses wrap in full inside the amount column and the contact stays readable.
- On the requester (Pixel 7), with a $5 request and a 5,000-sat request, both paid in USDT:
- "Płatność zweryfikowana · 5,00 USDT" and "Płatność zweryfikowana · 4,12485 USDT" wrap in full under "+$5,00" and "+₿ 5 000".
- The contact shows as "Alexandra P…".
- The pending "$2,00" and "₿ 1 000 000" requests show "Alexandra Payer" in full.
- With fiat as the primary unit, the BTC request's status breaks "zweryfikowana" mid-word (the LOW finding).
test4-payments-list-b.2x.mp4
Test 5: Request card amounts at large text on a compact phone ❌ The 1,000,000-sat request card shows "₿ 1 000 0…" at 130% text.
- On the payer (360dp) at 130% text, in English:
- The $2.00 card shows its amount in full.
- The 1,000,000-sat card shows "₿ 1 000 0…" above "$ 824.70", next to Pay.
- At 100% text the same amount just fits.
- I added this case for the new one-line amount cap, which no journey covers.
test5-request-cards-a.2x.mp4
Not tested on device:
- USDT confirmations (Swipe To Pay) above system navigation: our earlier review proved them at 9112565, and
PaykitUsdtReview.ktis unchanged since. - The TalkBack progress state on the loading contact: checked in
PubkyContactRow.ktandSendContactSelectScreenTest.kt. - The iOS-only actions added to
usdt-bridge-details.xmlandusdt-payment-links-and-scans.xml.
Scope: a delta review of 9112565...fccb1f9, 12 files, plus the three findings our last review left open.
No twin PR: synonymdev/bitkit-ios#1002 is the related iOS accessibility and journey follow-up, not a port of these fixes.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no HIGH/MEDIUM. One LOW (journey preconditions) is inline.
Checked and clean:
- The receive share text and the QR
uint256now come from the same parsedatomic, so they always agree. maxAmountLines = 1is passed only byPaymentRequestCard, never on review or confirm screens.OpenPaymentcarries the resolvedpublicKey, so no other contact can be paid, and every exit ofopenContactPaymentreleases the row lock.PaykitUsdtReviewonly adds a nav-bar inset; the auth gate and Swipe To Pay order are unchanged.- The hidden-balance path in
UsdtReceivePrivacyTestis untouched.
Perf-relevant: BoxWithConstraints now wraps every PaymentRequestCard in lazy lists. I'll measure Payments-tab scroll with a long list against master.
The six journey file names and the added action prose match the twin synonymdev/bitkit-ios#1002.
Device gate (partial): not run yet. Pending: the six changed journeys, a perf check and Check 5. 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 402aecb, at 79d2089.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Contact selection stays locked until the scan job from openContactPayment finishes, including the gap after OpenPayment, and unlocks on failure, cancellation, and completion. USDT confirmation and incoming activity details pad the outer column for the navigation bar. Arbitrum receive sharing formats the parsed atomic amount with usdt_format_amount from bitkit-core v0.8.0-rc6, so an entered 3.50 shares as 3.5 while the QR keeps uint256. The amount line is no longer held to half the card, so the truncation in the earlier amount comment does not apply at this revision.
iOS contact loading was compared at 7b3f093: the spinner lasts through scan and navigation, and a second tap is ignored. Android also disables the other rows. bitkit-ios#1002 is the related accessibility follow-up, not a port of these fixes, and was not reviewed in full.
CI build, lint, and detekt succeeded on this head. This review did not run unit or Compose tests and did not start a device.
Device testing: not performed in this review.
Recommended before device testing: The new steps in request-summary.xml, usdt-send-review-and-details.xml, and contact-request-or-pay.xml depend on wallet state their descriptions never create. See the open journey comment.
Suggested additional test cases
- Android, 360dp-wide display, text size 130%, Polish. Open Payments on a verified USDT receipt whose status contains "Płatność zweryfikowana". Expected: "zweryfikowana" is fully visible, the status wraps only between words, and the contact name stays readable. Repeat with Bitcoin as the primary unit for a BTC request paid in USDT.
Findings
- [LOW] Long verification words still clip in the amount column — inline at
app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt:602.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up at 595951a0 since 79d2089e, using the inherited baseline previous QA review. The merge base is unchanged at 402aecb6. This pass inspected receipt-status sizing, the payment-request layout assertions added in this commit, and the journey text updates. Inherited coverage is contact selection through cancellation, contact-row disabled and progress state, USDT navigation-bar insets, Arbitrum share text versus the QR amount, contact tests and journey steps unchanged in this delta, and the iOS contact-loading comparison at 7b3f093be6643a3075634f57d5d980f2c58912c2.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Device testing: not performed in this review. The Compose payment-request tests were not executed. Widths below were measured offline from the repo's Inter Tight fonts. iOS receipt-card layout was not compared in this pass.
Suggested additional test cases
- Android. Use a 320dp-wide window, font scale 1.3, and a USD-denominated payment-request history row whose verified receipt is "Płatność zweryfikowana · 1,234567 USDT" and whose contact is named "Maria". Open that history. The contact name is fully visible, and each word of the receipt status stays on one line.
Earlier findings reconciled at this revision: 9 verified fixed.
Findings
- [MEDIUM] Keep a readable contact name beside long receipt words — inline at
app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt:631.
|
Resolved the master conflict in 43be1a9, preserving the existing layout/contact fixes and master’s locale-aware amount formatting and wraparound Receive swipes. Independent review confirmed both parent changes are retained. Compile and all 4,375 unit tests pass. Complete Detekt and Android Lint reports have no findings, with the lint baseline removed by master. All ten PR commits verify locally and on GitHub. No funded/device flows were rerun for this integration. Re-requesting review; #1565 remains stacked on this PR and its resulting import conflict is being handled separately. |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of PR 1564 at 43be1a92, because the comparison base changed and no coverage baseline was available to inherit. The diff is from merge base 99497260.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
Contact resolution can leave the contact list disabled when the open-payment event is dropped, and a long USDT receipt can still ellipsize a multi-word name whose longest word is short. USDT confirmation and incoming activity details now pad for system navigation the same way Bitcoin confirmation already does. An entered Arbitrum amount is shared from its parsed atomic value, while the QR uint256 value is unchanged.
Validation: this review did not run unit tests, Compose tests, or a device. Lint and detekt succeeded for this head. Contact loading was compared with bitkit-ios SendContactSelectView, and the payment row with PaymentRequestsView, at edd752ac from merged bitkit-ios#1002. That was not a full iOS review.
Device testing: not performed in this review.
Suggested additional test cases
- Android, Send → Contacts, with a saved contact whose payment resolution takes several seconds. Start resolution, then rotate the device or change the system font scale before the amount screen appears. Expected: the amount screen opens for that contact, or the contact list returns with no spinner and another contact can be selected.
Findings
- [MEDIUM] Contact list stays disabled when the open-payment event is dropped — inline at
app/src/main/java/to/bitkit/ui/screens/wallets/send/SendContactSelectViewModel.kt:78. - [LOW] Long USDT receipt still ellipsizes a short-word contact name — inline at
app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt:611.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta at afcf6ff: no issues found. The 43be1a9 merge resolves an import-only conflict in PaykitAmountView.kt, and the kept imports are used.
Checked and clean:
SendContactSelectViewModel: the one-shot open event is now a nullableStateFlowbehindfilterNotNull()(:40-41). It carries the sameOpenPaymentpayload (:77), so the amount, recipient and endpoints handed toopenContactPaymentare unchanged.- No double open:
onPaymentOpeningclears the event synchronously (:107) right after the non-suspendingonOpenPayment, so a recreated screen can't open the same payment twice. - Back, dismiss and failures: the ViewModel lives in the
ContactSelectdestination (SendSheet.kt:330), and NoEndpoint, NotOpened and Waiting never store a payment. None of these paths leaves one to resurface later. PaymentRequestsScreen.kt:611:IntrinsicSize.Maxis layout only. The name and subtitle are reserved first, as on iOS.- The new tests cover a late collector, a replacement collector, and "Ann Lee" with a receipt at 407/412 dp.
- CI green.
Perf: not relevant.
Device gate (partial): afcf6ff — not run yet. Pending:
payment-requests/request-summary.xmlwith a short multi-word name, a 6-decimal USDT receipt and a short subtitle;payment-requests/contact-request-or-pay.xmlwith a rotation or font-scale change while the contact resolves.
Device results will follow in a separate review.
|
Device gate (partial):
The detail of the BTC request paid in USDT shows "pending · 2.46375 USDT", not clipped. Still running: |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving again. The afcf6ff delta is clean (code trace in my comment above), and the device re-check passes.
Device gate: afcf6ff — journeys: payment-requests/request-summary.xml row-layout action passed at 411 dp and 320 dp with font 1.0 and 1.3; payment-requests/contact-request-or-pay.xml 13/14 passed, step 7 over its 3 s budget (staging latency, same on master); perf: not relevant; Figma: n/a; l10n: no UI text changed.
Ran on Pixel_9 (emulator).
- Send → Contact (the path
afcf6ffchanges), run with no perturbation, then rotation twice and a font-scale change twice while the contact resolves. The font changes recreate the activity.- Every run logs exactly one
Opened private Paykit paymentand oneStarting scan from 'scan result'. - One amount screen opens, and nothing reopens within 15 s of Back.
- Every run logs exactly one
- "Ann Lee" with USDT receipts (2.46375 / 3.00 USDT): the name stays whole and the receipts aren't clipped, beside the contact or wrapped below it.
⚠️ Not checked: a "verified" status with a long translated verification string, which needs a second instance paying on the USDT fixture.
|
Outside this PR's scope; no change needed here. Found while running the device gate. Contact Detail: a font-scale change while Pay is resolving drops the payment silently.
Suggested follow-up issue in bitkit-android: keep the pending open-payment request across activity recreation on Contact Detail, as |
|
Out of scope: tracked in #1617 for 3.1.0, because Contact Detail uses a separate screen/ViewModel unchanged by this PR. I included your two font-scale reproductions and the retained-handoff investigation, without changing the validated Send → Contact fix. |
There was a problem hiding this comment.
Verdict: ✅ Approve.
Findings: None. The retained-opening finding and contact-name finding hold fixed.
QA for afcf6ff: Android 16 emulator; app and instrumentation built at this head. Recordings play at 2×. Receipt-layout checks use deterministic fixtures with the production Payments list, card and formatter. No funds were sent.
Test 1: Request cards and contact selection ✅ The rendered regression assertions pass.
Instrumentation checks the full “Ann Lee” name with an English verified receipt at padded 407dp and 412dp, compact translated receipts, whole amounts, and contact progress and disabled-row semantics.
card-regressions.2x.mp4
Test 2: Verified Polish receipts ✅ Ann Lee and every verification word remain complete at 320dp with enlarged text.
The production Payments list retains its horizontal padding. USD-, USDT- and BTC-denominated fixture requests show the exact 1,234567 USDT receipt at 130% and 200% text in both primary unit settings. Instrumentation checks overflow and word boundaries; I also checked the recording. Short default-size content remains beside the contact.
verified-receipts.2x.mp4
Test 3: Whole amounts and contact space ✅ Long amounts stay whole, and Alexandra Payer fits beside $2.00 pending.
At padded 320dp and 130% text, ₿ 1 000 000 and $10,000.00 each fit on one line. The short-amount check uses padded 375dp. Instrumentation also confirms Pay and Dismiss invoke their supplied callbacks once each; no payment is dispatched.
amount-contact-space.2x.mp4
Test 4: Receive tab wraparound ✅ Edge-tab swipes wrap without dismissing Receive; a QR tap still copies.
On a fresh wallet with Savings, Spending and USDT, right from Savings selects USDT and left from USDT selects Savings. Ordinary swipes traverse Spending. Diagonal swipes released above the sheet also wrap and keep it open. The final QR tap shows the copy tooltip. This verifies the wraparound behavior from synonymdev/bitkit-android#1593 fix: wrap receive tab swipes around.
receive-wraparound.2x.mp4
The retained-payment ViewModel regressions also pass: a late collector receives the pending opening, a replacement collector cannot reopen it, and completion or cancellation releases selection.
Reports retained for unchanged behavior:
- The current-head contact device report proves one amount screen opens during rotation and font-scale recreation, with no reopening after Back.
- The 371478a device report proves receive sharing/QR amounts, genuine USD/BTC receipt comparisons, and contact confirmation above both navigation modes.
- The activity-details report proves activity actions clear system navigation.
- The confirmation report proves one-time and recurring USDT confirmation padding in the unchanged component.
Scope: the contiguous 371478a…afcf6ff delta, including the master merge, plus answered findings. Other merged UI changes and resource cleanup were inspected in source; complete unchanged journey sequences and iOS-only bridge/scanner actions were not rerun. No Figma comparison was performed.
Related synonymdev/bitkit-ios#1002 fix: improve usdt deposit accessibility, merged at edd752ac, reserves the full ideal contact width before stacking the receipt, matching this fix. iOS device QA is outside this Android run.
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 pull request diff against merge base 9949726, at afcf6ff. Explicit full review; no inherited coverage.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Contact selection stays locked on the chosen contact through resolution and the scan job, keeps one resolved open across a gap before the screen collects it, and unlocks after success, cancellation, or a non-open result. USDT confirmation and incoming activity detail add navigation-bar padding on the edge-to-edge sheet and wallet screen. Payment cards wrap the amount and receipt when they do not fit beside the contact; Compose foundation-layout 1.11.4 includes the contact text in that wrap decision. Arbitrum share text formats the entered atomic amount with usdtFormatAmount, and the QR still carries that same uint256.
Compared with iOS contact loading at 59278ad and the payment-row ViewThatFits layout at edd752ac in bitkit-ios#1002. Android disables every contact row while resolving; iOS disables the selected row and ignores another tap in the model. That was a scoped comparison, not a review of the iOS pull request.
Validation was source and caller inspection only. Unit tests, Compose tests, and device journeys were not run. Device testing: not performed. Ready for device testing of the updated journeys.
Suggested additional test cases
- Android, with a saved payable contact: open Send, choose Contacts, tap the contact, then change the system font scale before the amount screen appears. The same row keeps its progress indicator, other contacts stay disabled, and the amount screen opens once. Returning to the list allows another selection.
Earlier findings reconciled at this revision: 1 still present, 9 verified fixed.
Findings
- [MEDIUM] Contact Detail cancels Pay when the screen is recreated — Contact Detail cancels Pay when the screen is recreated
Raised earlier in review discussion; still present at afcf6ff: A font-scale recreation still drops Contact Detail Pay. ContactDetailScreen's DisposableEffect calls dismissRequestOrPaySheet on dispose. While isPayLoading is true, that cancels payJob and clears the sheet without an error. The ViewModel survives the recreation, and the new screen has no retained opening to collect. The reviewer reports this on Pixel_9 at afcf6ff, with no Opened log, and says rotation did not recreate the activity in that run. That report is attributed, not a device run from this reconciliation. The author tracked it in #1617; that is not a fix. These two files are unchanged by the pull request diff.
Source. Summary placement: ContactDetailScreen.kt and ContactDetailViewModel.kt are not in the pull request diff, so the dispose cancellation has no diff line. Still present from an earlier unpublished review.
|
Out of scope: tracked in #1617 for 3.1.0, because this is the separate Contact Detail flow covered by John’s original follow-up report. I agree it remains unfixed; tracking it is not a fix. I verified the reported path: disposal calls |
There was a problem hiding this comment.
Verdict: ⛔️ Request changes. Two device cases fail in code inherited unchanged from master; I found no new defect on this PR’s changed lines.
Findings: None in the PR diff. The inherited failures are detailed in Tests 1 and 6.
QA for c1f1c0a: Android 16 emulators, with the app and instrumentation built at this exact head. Layout checks used 320×640dp at 100–200% text; contact routing also used a 411dp display. USDT funding and quotes used an isolated Arbitrum fork, and contact linking used disposable staging profiles. No payment was submitted. Videos play at 2× speed.
Test 1: Receive pager wraparound ❌ The documented right diagonal swipe from Savings stays on Savings.
On the default 1080×2400 display, input swipe 100 720 950 200 1200 starts inside the QR but leaves Savings selected. The same action fails at compact width. The sheet remains open. Horizontal wraparound works, the opposite diagonal reaches Savings from USDT, and tapping the QR copies with feedback. The recording shows the failed diagonal followed by the successful horizontal control and copy.
ReceiveQrScreen.kt is identical to master, outside this PR’s final diff.
receive-wraparound.2x.mp4
Test 2: Direct USDT amount and review ✅ Scrolling exposes the exact amount, keypad, Continue and fee note; Swipe To Pay clears navigation.
The 1.234567 link amount survives keypad interaction and is readable at 100% and 200% text, checked in the UI hierarchy. Short downward drags reveal the header; upward drags reach Continue. A real fixture quote opens collapsed and expanded review. The fee note scrolls below Hide Details while Swipe To Pay stays pinned above gesture and three-button navigation. Nothing was paid.
| Direct review | Enlarged text and three-button navigation |
|---|---|
direct-review.2x.mp4 |
direct-large.2x.mp4 |
Test 3: Contact and request USDT confirmation — J4 ✅ All three presentation types keep their swipe control above navigation, verified by instrumentation.
I rendered the production PaykitUsdtReview with contact, one-time request and recurring request presentation fixtures on the real activity. At 100%, 130% and 200% text, collapsed and expanded views retain a visible swipe control; the fee note is reachable. Instrumentation compared its bounds with the actual system navigation inset in both modes. These are presentation checks; no request was paid. The production APK remained unchanged.
| Three-button navigation | Gesture navigation |
|---|---|
paykit-threebutton.2x.mp4 |
paykit-gesture.2x.mp4 |
Test 4: Incoming activity and transaction copy — J4 ✅ The activity actions clear navigation, and transaction-ID copy preserves the full hash.
A confirmed 25 USDT fixture receipt shows Details above navigation in summary view and Cross-Network Deposits above it in expanded view. Explorer and Hide Details remain reachable at 200% text. Both navigation modes were checked; enlarged-text gesture bounds were also checked in the UI hierarchy. Tapping the transaction ID copies the full 66-character hash, independently verified in Android’s clipboard editor.
| Copy and three-button navigation | Enlarged text | Gesture navigation |
|---|---|---|
activity-details.2x.mp4 |
activity-large.2x.mp4 |
activity-gesture.2x.mp4 |
Test 5: Receive amount and sharing — J3 ✅ Both shares show 3.5 USDT and their actual QR images retain uint256=3500000.
After entering 3.50, both Android share previews show 3.5 USDT. I decoded both shared PNG files independently and confirmed uint256=3500000. Cancelling an edited amount leaves the committed QR unchanged. QR-copy feedback is visible in Test 1.
receive-sharing.2x.mp4
Test 6: Contact Detail Request or Pay — J1 ❌ The route works, but Pay exceeds three seconds and the compact sheet hides its buttons.
With two mutually saved, linked profiles on the 411dp display, Pay shows progress and disables Request until Amount opens. A capture started 3.001 seconds after the tap still shows the spinner; the hierarchy completed at 5.893 seconds still contains Request or Pay. Back restores enabled Contact Pay, and Request opens PaymentRequestAmount. No payment or request was submitted.
On 320×640dp at default text, the fixed-height Request or Pay sheet places its action buttons below the visible area. The compact recording shows that limit; I completed the route on the normal-height display.
ContactDetailScreen.kt and its ViewModel are identical to master. The earlier Contact Detail report already recorded the timing failure on unchanged code.
| Completed route | Compact sheet |
|---|---|
contact-detail.2x.mp4 |
contact-compact.2x.mp4 |
Coverage retained: The afcf6ff review proves the unchanged J2 translated receipt/card layouts and contact-picker state. The recreation report proves the unchanged Send→Contact handoff/retry path. The genuine receipt comparison remains applicable to unchanged receipt formatting.
Scope: contiguous delta afcf6ff…c1f1c0a, including the master merge and confirmation-padding resolution. Other merged features received source review only; full legacy journey sequences and the full unit/lint suites were not rerun. J5/J6’s added actions are iOS-only. No Figma comparison was performed.
Related iOS follow-up: synonymdev/bitkit-ios#1002. Its source likewise preserves the contact’s full width and stacks receipt content when needed; iOS device behavior was outside this Android run.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Checked the new device review against the PR base and current master
I added the exact-head device evidence to the PR's QA notes. The direct review, navigation-inset/activity controls, exact amount sharing and QR decoding checks passed in that review. The broader journeys remain unchecked where the report did not establish a complete pass; no payment was submitted. No source changes or review-thread resolutions were made for these inherited findings. |
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 from merge base 22a5cb561, at c1f1c0a78. No earlier baseline was supplied.
3 actionable findings — resolve or provide an evidence-backed rebuttal.
Contact selection stays on the tapped contact until resolution fails or the scan job returned by openContactPayment finishes, retains that open across a gap between screen collectors, and consumes it once. Failure and job cancellation clear the busy state. Payment-request rows keep the contact beside the amount when both fit and place the amount and receipt below the contact when they do not; BTC receipt status uses caption styling for either unit setting. Arbitrum receive sharing formats the committed amount from its atomic value, so 3.50 shares as 3.5 USDT, while the QR uint256 still uses that atomic value. Incoming USDT activity detail is padded above the system navigation bar. Contact loading was compared with bitkit-ios 2464865: iOS disables the loading row and ignores other taps, and Android disables every row; both hold the busy state through payment opening.
Device testing: not performed in this review. The new unit and Compose tests were inspected and not executed here.
Ready for device testing.
Suggested additional test cases
- Android, devDebug, USDT enabled, with a linked payable contact. Open Send → Contacts, tap the contact, and while its spinner is visible recreate the activity. The same row should stay busy, other rows should stay disabled, and the amount screen should open once.
Earlier findings reconciled at this revision: 3 still present, 1 tracked separately outside this PR. Tracked separately (outside this PR; unresolved): Documented Savings diagonal swipe was reported to leave Savings selected. The reported Savings diagonal-swipe failure is tracked in #1667 as a pre-existing issue outside this PR. ReceiveQrScreen.kt and SwipeToChangeTabModifier.kt are byte-identical at the reviewed base and head. The gesture cause remains unestablished, and this review does not claim a passing device run. This is not a runtime verification or merge-readiness declaration.
Findings
- [MEDIUM] Contact Detail cancels Pay when the screen is recreated — Contact Detail cancels Pay when the screen is recreated
Raised earlier in review discussion.
Still present at c1f1c0a (first reported at afcf6ff): A font-scale recreation still cancels Contact Detail Pay at c1f1c0a. MainActivity locks portrait and declares no configChanges, so a fontScale change recreates the activity. ContactDetailScreen's DisposableEffect calls dismissRequestOrPaySheet on dispose. While isPayLoading is true, that cancels payJob and paymentScanJob and clears the sheet without an error. The screen takes its Hilt ViewModel from the navigation back stack, so the ViewModel survives recreation. effects is a MutableSharedFlow created with extraBufferCapacity 1 and the default replay of 0, so the replacement collector has no retained opening, and a payJob still inside beginSavedContactPayment is cancelled before it emits. ContactDetailScreen.kt and ContactDetailViewModel.kt are absent from the pull request diff. This check did not run a device.
Source. Summary placement: ContactDetailScreen.kt and ContactDetailViewModel.kt are not in the pull request diff, so the dispose cancellation has no diff line. Still present from the earlier review.
- [MEDIUM] Request or Pay actions are clipped on a 320×640dp screen — Request or Pay actions are clipped on a 320×640dp screen
Raised earlier in review discussion.
Still present at c1f1c0a (first reported at c1f1c0a): Request or Pay still places its actions past the bottom of a 320×640dp sheet at c1f1c0a. The sheet column is a fixed SheetSize.MEDIUM height with isModal true and has no vertical scroll. MEDIUM height is max(preferred, min(600.dp, screen − topPadding)), and topPadding is the status-bar inset plus twice the navigation-bar inset plus TopBarHeight plus 16.dp. On a 640.dp window that remainder is under 600.dp, so the column gets only that remainder. navigationBarsPadding then removes the navigation inset again inside it. The non-weighted content is the 54.dp SheetTopBar minimum, a 256.dp image, a two-line 44.sp headline because the source string contains a newline, at least one 22.sp body line, 12+24+16.dp spacers, and a 56.dp button row, totaling 528.dp. Child space is 560.dp minus the status inset minus three navigation insets. A 24.dp status bar and 24.dp navigation bar leave 464.dp, 64.dp short of the button row; a 48.dp navigation bar leaves the buttons and description past the bottom. FillHeight only takes leftover weight and does not shrink the image. MainActivity enables edge-to-edge, so those insets apply. ContactDetailScreen.kt and SheetHeight.kt match the merge base. This check did not render the sheet.
Source. Summary placement: ContactDetailScreen.kt and SheetHeight.kt are not in the pull request diff, so the compact sheet clip has no diff line. Still present from an earlier unpublished review.
- [LOW] Contact Detail Pay can stay on the sheet past the 3 second journey bound — Contact Detail Pay can stay on the sheet past the 3 second journey bound
Raised earlier in review discussion.
Still present at c1f1c0a (first reported at c1f1c0a): Contact Detail Pay can still remain on the sheet past the journey's 3 second bound at c1f1c0a. payContact sets isPayLoading and calls openPayment, which waits on beginSavedContactPayment. When that result is WaitingForUpdatedPaymentList, beginSavedContactPaymentWithRetry delays 1 second, then 3 seconds, then 8 seconds before returning. The sheet stays up through those delays and, after a successful open, until onPaymentOpening joins the scan job. The contact-request-or-pay journey requires the amount screen within 3 seconds, and the 1 second plus 3 second retries already pass that bound while the spinner is showing. ContactDetailViewModel.kt is not in the pull request diff. This check did not run the journey.
Source. Summary placement: ContactDetailViewModel.kt is not in the pull request diff, so the unbounded sheet Pay wait has no diff line. Still present from an earlier unpublished review.
Reviewer model: grok-4.7
Reasoning effort: xhigh
|
Rechecked the three findings in this review against
The previous disposition and device evidence are recorded here. None is claimed fixed by this PR, and no new device run was performed for this recheck. This PR's retained Send → Contacts handoff is a different entry point. The iOS all-row disabling counterpart is the still-open iOS #1003; comparing against iOS master does not include that fix. |
This branch has not been deployed
5 participants
Fixes #1523
Fixes #1525
Fixes #1537
Fixes #1561
Related iOS accessibility and shared-journey follow-up: synonymdev/bitkit-ios#1002
Description
3.50shares as3.5 USDT. QR amounts and selected bridge deposit addresses remain unchanged.Out of Scope
Design
Uses the existing payment request layouts and shared contact spinner. No redesign. The latest handoff page reference for these small fixes is not confirmed.
Preview
The linked issues show the affected screens. The focused Compose device tests exercise contact loading and a long translated receipt at 320dp including the list’s outer padding, at 130% and 200% text; no screenshot attachment is available yet.
QA Notes
Setup
Use app revision
c1f1c0a78and its pinned dependencies; build the devDebug app withjust run. Enable USDT for these cases. Use existing linked test contacts and incoming request/receipt fixtures; the failure/retry contact must have no usable payment endpoints. Use a 320dp emulator at 130% and 200% text and repeat the navigation cases with gesture and three-button navigation. Funded USDT and controlled provider cases require the separate local USDT fixture setup; Bitcoin regtest alone does not supply it. Preserve existing wallet data.Journeys
contact-request-or-pay.xml— selected-contact progress, duplicate-selection prevention and retry after failure.request-summary.xml— readable contact and exact amount with a long translated verification status.usdt-receive-amount-and-units.xml— enter3.50, share3.5, and retainuint256=3500000in the QR.usdt-send-review-and-details.xml— confirmation and incoming-activity actions stay above gesture and three-button navigation.usdt-bridge-details.xml— shared iOS accessibility checks; Android behavior is unchanged by that step.usdt-payment-links-and-scans.xml— shared iOS scanner-scroll check; Android behavior is unchanged by that step.Journey prerequisites include completed USDT receipts, linked payment contacts and unpaid requests. USD receipts are compared with their request amount; BTC-denominated receipts keep up to six decimals and are compared with the rounded Payment field in the payer’s activity detail, excluding fees. Layout checks use iOS 375 points / Android 320 dp. The failure/retry step uses a contact without payment endpoints.
Manual Tests
N/A
Automated Checks
SendContactSelectViewModelTest.kt— resolution and navigation keep the selected contact locked, including an absent screen collector and a new collector after navigation starts; completion/cancellation/failure allow retry.SendContactSelectScreenTest.kt— progress, disabled contact semantics and selection after resolution.PaymentRequestsScreenTest.kt— contact/status text remains fully laid out, with no split words, at narrow width with enlarged text for USDT-, USD- and BTC-denominated requests; short amounts leave room for longer contact names, and a long USDT receipt wraps before truncating a short-word name such as Ann Lee. Regressions cover a 1,000,000-sat amount, a large USD amount, and a Polish BTC-request status in both unit settings.PaymentRequestsScreenTest.kt— all 15 payment-list Compose tests and the contact-selection component test pass on the dedicated API 36 emulator, including actual padded 320dp cards at 130% and 200% text. Existing contact, bridge and receive-sharing checks also passed. No funds were sent.Validation at
c1f1c0a78: integrated master22a5cb561and preserved its system-navigation padding.just compile, all 4,406 unit tests andjust lintpassed. Complete Detekt and Android Lint reports contain zero findings, with no lint baseline applied. All 12 PR commits are cryptographically verified locally and by GitHub. A bounded independent review found no merge-introduced issues. The previously recorded device-test results above remain evidence for their original revision; the full three-button/gesture journeys and live activity-recreation timing still need reviewer device QA.External device review at
c1f1c0a78: review and recordings verify direct USDT exact-amount review, contact/request swipe placement, incoming activity controls at 100–200% text under both navigation modes, and shared3.5 USDTwith independently decodeduint256=3500000QR images. No payment was submitted. The same report identifies inherited Contact Detail compact-layout and receive-diagonal failures (#1666/#1667), so it is not a pass of every listed journey. Its retained contact-picker/receipt coverage remains attributed to the earlier tested revisions. Source checks confirm the affected inherited files match current masterd5526d409.Models used
gpt-6-astra(reasoning:xhigh).gpt-6-astra(reasoning:xhigh).gpt-6-astra(reasoning:xhigh).