Repository navigation
fix: usdt amount keypad sits above continue - #1061
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. |
|
Superseded: 5040d45 changed the layout to match the Bitcoin amount screen (see the description's Preview); the checks below covered the earlier layout. QA on an iPhone 17 Pro simulator (iOS 27.0), regtest debug build of this branch:
Not driven: the Paykit request Amount step paid in USDT (see Manual Tests); it uses the same layout code. |
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryMoves the extra space above the USDT keypad into the amount header on both amount screens.
Reviewed the source only; iOS visual checks were not run on Linux. Reviews (1) · Last reviewed commit: "fix: place the usdt amount keypad right ..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Puts the USDT amount keypad edge to edge directly above Continue and centres the amount in the free space above it, on both the Send sheet amount step and USDT Send. Reviewed 7b281b8, light tier.
What I checked, and 3 candidates I ruled out
Read in full: SendAmountView.swift, UsdtSendView.swift (amountContent), Components/NumberPad.swift (fixed key height)
CI: SwiftFormat, validate, build-local green; tests and e2e still running
Build: Bitkit track: the branch was not built here (Android branches are not built on this box, iOS does not build on Linux), so this review is reasoned from the code and from CI.
Ruled out
- Bitcoin amount step changed: every edit is behind
app.paykitUsesUsdt, and the non-USDT keypad padding is 0. - Keypad collapses without
maxHeight:NumberPadsizes itself from a fixedbuttonHeight * 4, so the free space now goes to the amount header instead. - Negative padding clips: both parents pad 16 horizontally, so -16 reaches the sheet edge and no further.
Merge confidence: 4/5, CI had not finished; a layout change no unit test applies to, covered by the updated journey.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 7b281b8: no issues found.
- Layout: the amount is centred between flexible spacers. The keypad's key height comes from the available height, clamped to 50–75, and Continue stays below it.
- Bitcoin path unchanged: on Android the scroll fallback applies only when the Paykit amount step is paid in USDT.
- Edge-to-edge keypad: the 16 dp bleed is not clipped, and the moved modifier body is unchanged.
- Untouched: Back, insets and nav-bar padding.
- Parity: Android adds a scroll fallback for short screens with large text. iOS keeps its fixed layout, which is 32 pt shorter than before, so the SE plus largest Dynamic Type case is no worse than master. I'll raise it only if the device run shows Continue clipped. See synonymdev/bitkit-android#1636.
- Journeys:
usdt-send-review-and-details.xmlupdated, identical across both repos.
Device gate (partial): 7b281b8 — not run yet. Pending:
- keys 1 and 3 reach the screen edges, and the outer 16 dp of key 1 takes taps;
- the gap from the keypad to Continue;
- a short screen with large text, scrolled to Continue;
- the same on the Paykit Amount step paid in USDT.
Device results will follow in a separate review.
jvsena42
left a comment
There was a problem hiding this comment.
Device results, part 1, on the USDT fixture (iPhone 17 simulator). PR 7b281b8 compared with master 5ce560f, on the USDT wallet's Send → Amount screen:
| master | PR | |
|---|---|---|
| Keys 1 / 3 (default text) | x 16–139 / 263–386 | x 0–134 / 268–402, edge to edge |
| Gap from the last key row to Continue | 56 pt | 0 pt; Continue sits right below, as on the Bitcoin screen |
| Largest text (AX5): Continue | bottom 20 pt off screen, label cut | fully on screen, reachable |
- Edge taps: taps at x 2 and x 6 on key 1, and at x 398 on key 3, each registered.
- Largest text: typed 1, tapped Continue and the review opened. The no-scroll concern from my code scan doesn't show up. This PR is better than master here.
- Bitcoin amount screen: unchanged, with the same frames on master and the PR.
- Journey: the changed action in
usdt-send-review-and-details.xmlpassed.
Not from this PR, same on both builds: at AX5 the sheet content is wider than the screen, so the amount header, AVAILABLE and the outer keypad columns are clipped at the sides. No iPhone SE simulator exists, so the large-text checks ran on iPhone 17.
Device gate (partial): 7b281b8 — 5 checks passed, 0 failed. Pending: the Paykit Amount step paid in USDT (the changed paykitUsesUsdt branch). The run needs a fresh USDT request from the Android device.
|
I recommend matching the existing on-chain and Lightning send amount layout rather than introducing a separate layout for USDT. This is the same recommendation as on the Android twin: synonymdev/bitkit-android#1636 (comment) Fixing the floating keypad makes sense, but centring the amount vertically and making the keypad edge-to-edge are not necessary to fix that issue. Please keep the amount placement and 16 pt keypad side insets consistent with the Bitcoin screens. The shared keypad sizing and spacing above Continue can remain as they are on iOS. USDT’s extra controls can wrap where needed without changing the basic layout. If we want to redesign the amount layout, that should be a consistent change across all three flows rather than USDT alone. |
@pwltr done in 5040d45. Both USDT amount screens ( A redesign toward the Figma frame (the amount centred, an edge-to-edge keypad) is listed under Out of Scope as a change for all three amount flows. @jvsena42 your device results here covered the earlier layout: the edge-to-edge keys and the Continue gap now match the Bitcoin screen instead. |
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
worth doing, does not block
- paykit-quoted-usdt.xml still asserts the old USDT amount layout (journeys/paykit-quoted-usdt.xml:19).
journeys/paykit-quoted-usdt.xml:19still says "the balance stays close below the amount and the keypad uses the remaining space without clipping Continue". This PR makes both halves of that false on the route the journey drives. The journey opens a USDT-only contact and reaches Amount. That is theapp.paykitUsesUsdtbranch ofSendAmountView. After this PR, that branch putsSpacer()(SendAmountView.swift:168) between the amount and the Available row. It also drops.frame(maxHeight: app.paykitUsesUsdt ? .infinity : nil)from the keypad. So the free space now sits between the amount and the balance, and the keypad keeps its fixedNumberPad.contentHeight. Anyone who drives this journey will get a failure. AGENTS.md says a PR updates any journey whose route it changes and lists it under#### Journeys. This journey also covers the Paykit USDT Amount step that the PR's Manual Tests says it could not drive. I confirmed this by reading the code and the journey text. I did not drive it on a simulator. The merged Android twin left the same line inbitkit-android/journeys/paykit-quoted-usdt.xml:19. Make the same edit there so the two files stay identical, for example: "the balance, chips and keypad sit at the bottom above Continue, as on direct USDT Send".
the reviewers disagree, your call
- Closes #1056 but the deferred redesign has no tracking issue (general). Objection: The repository guidance requires linked issues for deferred work, but the PR body and #1056 were inaccessible. I cannot independently confirm the claimed tracking omission from the code diff. Answer:
gh pr view 1061showsCloses #1056, and the Out of Scope bullet "A redesign of the amount layout (the amount centred and an edge-to-edge keypad...)" carries no issue link, though.agents/commands/pr.md:112requires one for real work. #1056's Expected behavior items 1 (amount centred) and 3 (no side padding) are exactly what the PR leaves out, it keeps the 16 pt insets, and no open issue tracks them; #974 covers the request-creation amount screen, not the send flows.
|
@coreyphillips thanks.
Right. To keep this PR's approvals, the line changes in #1082 (fbe1d64), and the same words go into the Android file in synonymdev/bitkit-android#1637 (fc46b472b): "the balance, chips and keypad sit at the bottom above Continue, as on direct USDT Send."
Filed #1083 for a shared centred, edge-to-edge layout across all three amount flows, and linked it from the Out of Scope bullet here and on synonymdev/bitkit-android#1636. |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after replies, with no new commit since 5040d45. Nothing blocking remains.
addressed
- Closes #1056 but the deferred redesign has no tracking issue. Fixed:
gh issue view 1083shows an open issue in the 3.2.0 milestone covering the centred amount and edge-to-edge keypad across all three amount flows. This PR's Out of Scope bullet now ends "Tracked in #1083", and the body of Android #1636 links it too.
still open
- paykit-quoted-usdt.xml still asserts the old USDT amount layout (journeys/paykit-quoted-usdt.xml:19).
journeys/paykit-quoted-usdt.xml:19still says "the balance stays close below the amount and the keypad uses the remaining space without clipping Continue". This PR makes both halves of that false on the route the journey drives. The journey opens a USDT-only contact and reaches Amount. That is theapp.paykitUsesUsdtbranch ofSendAmountView. After this PR, that branch putsSpacer()(SendAmountView.swift:168) between the amount and the Available row. It also drops.frame(maxHeight: app.paykitUsesUsdt ? .infinity : nil)from the keypad. So the free space now sits between the amount and the balance, and the keypad keeps its fixedNumberPad.contentHeight. Anyone who drives this journey will get a failure. AGENTS.md says a PR updates any journey whose route it changes and lists it under#### Journeys. This journey also covers the Paykit USDT Amount step that the PR's Manual Tests says it could not drive. I confirmed this by reading the code and the journey text. I did not drive it on a simulator. The merged Android twin left the same line inbitkit-android/journeys/paykit-quoted-usdt.xml:19. Make the same edit there so the two files stay identical, for example: "the balance, chips and keypad sit at the bottom above Continue, as on direct USDT Send".journeys/paykit-quoted-usdt.xml:19is unchanged on this branch and on origin/master. #1082 (fbe1d64) has the right wording, butgh pr view 1082shows it is still OPEN, and this PR's body does not mention it. Merge #1082 first, or link it from this PR's Journeys or Out of Scope section, so whoever drives journeys after this merges is not checking a stale assertion.
Closes #1056
Twin: synonymdev/bitkit-android#1636
This PR moves the USDT Amount step's keypad down so it sits right above Continue, as on the Bitcoin amount screen. The amount stays at the top, the free space sits between the amount and the controls bar, and the keypad keeps its 16 pt side insets.
Description
SendAmountViewdoes, so the controls bar and keypad sit at the bottom.UsdtSendViewand the Paykit request Amount step inSendAmountViewwhen it pays in USDT.usdt-send-review-and-details.xml, which asserted the old 32-point gap.Out of Scope
SendAmountView.swiftbitcoin branch and the other numpad screens: unchanged.Design
Bitkit - Refactor v63 › Send (USDT) › Amount USDT. This PR follows the Bitcoin amount screen's layout rather than the frame's centred amount and edge-to-edge keypad, so the three amount screens stay consistent.
Preview
QA Notes
Journeys
usdt-send-review-and-details.xml— the amount sits at the top, the controls sit right above the keypad and Continue sits right below it, as on the Bitcoin Amount screenManual Tests
journeys/payment-requests/README.mdpublishes Bitcoin endpoints only; a request paid in USDT on Arbitrum needs an issuer that publishes a USDT endpointAutomated Checks
N/A