Repository navigation
fix: pin paykit request keypad and ok button - #1758
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Medium impact] The PR appears safe to merge. SummaryUpdates the payment-request amount and sent screens:
No actionable issues were found. Reviews (1) · Last reviewed commit: "fix: pin paykit request keypad and ok bu..." · Reviewed by Greptile |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
the reviewers disagree, your call
- Journey prose now differs from iOS with no twin carrying it (journeys/paykit-amount-layout.xml:5)
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Moves the Paykit request number pad to Send's placement (sized from the available height, directly above Continue, spare space between the divider and the pad) and pins OK to the bottom of the Payment Requested sheet with a scrolling body and the RECIPIENT caption. Reviewed dcab2f0f8, full tier, static: on the Bitkit track an auto review does not build an Android branch, so this was reasoned from the code and from CI, not run.
What I checked, and 4 candidates I ruled out
Read in full: CreatePaymentRequestScreen.kt (both composables), NumberPad.kt (the availableHeight sizing), Spacers.kt (FillHeight(min)), SendAmountScreen.kt (the reference layout).
Call sites traced: PaymentRequestAmountContent is also the Amount step of CreateSubscriptionScreen.kt, so the subscription amount screen gets the same pad placement; J1 already covers it. PaymentRequestSentContent serves the create flow and Receive > Request payment, as the body says.
CI: build, lint, detekt, build-local and Greptile green; the e2e matrix was still pending when this was posted.
Ruled out
- Weights inside a vertical scroll:
FillHeightsits in averticalScrollcolumn, butheightIn(min = maxHeight)gives the column a bounded minimum to distribute, the same construction the screen used before andSendAmountScreenuses for USDT. - Pad taller than the space on a short screen:
NumberPadtakesmaxHeight / ROWSclamped between the min and ideal key height, identical to Send's call; past that the column scrolls, and the short-screen step ofpaykit-amount-layout.xmlcovers reaching every key and Continue. - Sent sheet width: the inner
Columnhas nofillMaxWidth, butBodyMfills the width, so the checkmark'sCenterHorizontallystill centres on the sheet. - New string:
wallet__payment_request_recipientis an existing resource onmaster; nothing new to translate.
Not verified: J1 and J2 on a device, and no Compose test asserts positions (the existing CreatePaymentRequestScreenTest.kt checks only that the nodes are displayed).
Merge confidence: 4/5, no findings, but the layout change has no test that asserts placement and I did not build it.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the run metadata available to this review does not report it)
The keypad and OK button placement is not fixed as requested yet; re-review after the follow-up push.
|
Pushed
The description lists each fix with its status, plus Figma side-by-sides at 360×640, 1080×2410 and 1440×3120 and emulator recordings. |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 1 new commit (71f5cf8). Nothing blocking remains.
still open
- Journey prose now differs from iOS with no twin carrying it (journeys/paykit-amount-layout.xml:5). This PR edits the action prose of two shared journeys, but no bitkit-ios PR carries the same edits. On iOS master,
journeys/paykit-amount-layout.xmlhas blob4053d110e, the same blob Android had before this PR (index 4053d110e..ab80d3521). So this change starts the drift.request-summary.xmlalso gains a RECIPIENT caption and OK-at-bottom assertion. CLAUDE.md asks for identical<action>prose on both platforms, carried by the matching iOS PR. bitkit-ios#1138 tracks the Sent sheet fix, but the PR body has noTwin:line, and nothing tracks the new request-amount keypad assertion on iOS. I did not check whether iOS already places its request keypad directly above Continue. Codex still sees it: The supplied body now links synonymdev/bitkit-ios#1194, matching your response, but its files were inaccessible. I could not compare its action prose against journeys/paykit-amount-layout.xml:5, so the matching port remains unverified.
new since the last review
worth doing, does not block
- Sent checkmark overflows onto the headline when no spare height is left (app/src/main/java/to/bitkit/ui/screens/paymentrequests/CreatePaymentRequestScreen.kt:742-754)
- Checkmark overflows its box when no height remains (app/src/main/java/to/bitkit/ui/screens/paymentrequests/CreatePaymentRequestScreen.kt:751-752)
@coreyphillips The twin synonymdev/bitkit-ios#1194 carries it. On both PR branches, Pushed 561c561: the Payment Requested checkmark now drops out when its slot has no height, instead of overflowing onto the headline at large text sizes. |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 1 new commit (561c561). Nothing blocking remains.
addressed
- Sent checkmark overflows onto the headline when no spare height is left (app/src/main/java/to/bitkit/ui/screens/paymentrequests/CreatePaymentRequestScreen.kt:742-754). Fixed: CreatePaymentRequestScreen.kt:751-752 now uses
fillMaxWidth(0.8f).fillMaxHeight()withContentScale.Fit. The weighted Box passes max height h, the remaining slot, so the Image is exactly 0.8W x h. Fit scales the check to min(0.8W, h), and at h = 0 it draws nothing, so it no longer overflows onto the headline. I checked the modifier chain and the Box/Column constraints by reading the code; I did not run it on a device. - Checkmark overflows its box when no height remains (app/src/main/java/to/bitkit/ui/screens/paymentrequests/CreatePaymentRequestScreen.kt:751-752). Fixed: This is the same fix as point 2, at CreatePaymentRequestScreen.kt:752. With
aspectRatiogone, nothing falls back to a width-sized square. The image is bounded by its slot height and scaled with Fit, the same way SheetIntro bounds its artwork without aspectRatio.
still open
- Journey prose now differs from iOS with no twin carrying it (journeys/paykit-amount-layout.xml:5). This PR edits the action prose of two shared journeys, but no bitkit-ios PR carries the same edits. On iOS master,
journeys/paykit-amount-layout.xmlhas blob4053d110e, the same blob Android had before this PR (index 4053d110e..ab80d3521). So this change starts the drift.request-summary.xmlalso gains a RECIPIENT caption and OK-at-bottom assertion. CLAUDE.md asks for identical<action>prose on both platforms, carried by the matching iOS PR. bitkit-ios#1138 tracks the Sent sheet fix, but the PR body has noTwin:line, and nothing tracks the new request-amount keypad assertion on iOS. I did not check whether iOS already places its request keypad directly above Continue. Codex still sees it: I could not verify the response's byte-identical claim forjourneys/paykit-amount-layout.xml:5against synonymdev/bitkit-ios#1194. GitHub was unreachable, and the twin's head is absent from local refs.
@coreyphillips For a check that doesn't need the twin checked out: |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after replies, with no new commit since 561c561. Nothing blocking remains.
still open
- Journey prose now differs from iOS with no twin carrying it (journeys/paykit-amount-layout.xml:5). This PR edits the action prose of two shared journeys, but no bitkit-ios PR carries the same edits. On iOS master,
journeys/paykit-amount-layout.xmlhas blob4053d110e, the same blob Android had before this PR (index 4053d110e..ab80d3521). So this change starts the drift.request-summary.xmlalso gains a RECIPIENT caption and OK-at-bottom assertion. CLAUDE.md asks for identical<action>prose on both platforms, carried by the matching iOS PR. bitkit-ios#1138 tracks the Sent sheet fix, but the PR body has noTwin:line, and nothing tracks the new request-amount keypad assertion on iOS. I did not check whether iOS already places its request keypad directly above Continue. Codex still sees it:journeys/paykit-amount-layout.xml:5belongs to blobdf74405ae, matching the response, and the description now links the twin. API and browser access failed, so I could not verify that bitkit-ios#1194 carries the identical file.
|
Merging: approved by @coreyphillips on the current head, the screenshots in the description were approved, on-device recordings and Figma side-by-sides are posted, no open threads, CI green. |
Fixes #1705
Fixes #1533
Twin: synonymdev/bitkit-ios#1194
The Paykit request Amount screen and the "Payment Requested" sheet did not follow the layout of Send's amount screen and the other success sheets. This PR makes them match and adapt to the screen height.
The BTC/USD toggle part of #1533 already shipped in #1654; this PR covers that issue's layout part.
Description
Status of each requested fix, at the current head:
Request Amount (
PaymentRequestAmountContent)sheetHeight()).Payment Requested (
PaymentRequestSentContent)RECIPIENTcaption sits above the recipient card, as in Figma.SheetIntroand the "Sent" transaction sheet (SHEET_INTRO_IMAGE_WIDTH_FRACTION, 80% of the sheet width), and it shrinks to fit on short screens, so the details and OK stay visible.Disabled Continue
primaryButtonStyle, so it is tracked in bug: disabled primary button shows a filled pill #1759.Out of Scope
Modifiers.kt: the disabled primary button style, tracked in bug: disabled primary button shows a filled pill #1759.Design
Preview
Request Amount, Figma and the app at three screen sizes. Send's amount screen is the reference row:
Payment Requested:
Payment Requested at 360×640 with 1×, 1.5× and 2× text, then at 1080×2410 and 1440×3120:
Emulator recordings of the three fixed screens (request Amount, Payment Requested and Send amount) at each size, at 2x:
and-small-rec.mp4
and-pixel10pro-rec.mp4
and-tall-rec.mp4
QA Notes
Setup
Two Bitkit instances on regtest with Paykit enabled, each with a Pubky identity, saved as each other's contacts and linked, as in
journeys/payment-requests/README.md. Check on a short phone and on a tall phone, such as a Pixel 10 Pro.Journeys
paykit-amount-layout.xml— the request Amount layout matches Send's amount screen: spare space under the amount, then the controls row, divider, keypad and Continue in the same placesrequest-summary.xml— the Sent receipt shows the RECIPIENT caption, with OK at the bottom and the details directly above itManual Tests
N/A
Automated Checks
PaymentRequestAmountLocaleTest.kt— the controls row sits at least 12 dp below the amount, with the spare space between them, as on SendEvidence
561c5610f:just compile,just lintandjust testpass.PaymentRequestAmountLocaleTest.ktpasses on an Android 16 emulator.71f5cf808, which differs only at large text sizes), at 360×640 (320 dpi), 1080×2410 (420 dpi) and 1440×3120 (560 dpi). A local test harness renders each screen at full sheet size. The Before images are built frommaster.Gaps
CreatePaymentRequestScreenTest.amountShowsNumberPadAndContinuefails on the emulator. It also fails withmaster's screen code, so this PR did not cause it.Models used
claude-opus-5-5(reasoning:medium)claude-opus-5-5(reasoning:medium)