Repository navigation
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
peachbits
left a comment
peachbits
left a comment
There was a problem hiding this comment.
The dock is the right fix. The old rem(5) reserve was a constant that never matched the slider's footprint (the rem(2) gap plus the 55pt slider), and the content now pads by the measured dock height instead. The slider and padding code is the same in v4.51.3 and develop, so Stealth Send exposed the shortfall rather than causing it: its rows push the scene past one screen, and its warning and error cards render last.
Checked locally at 06c61ee: tsc is clean, the SendScene2 suite passes, and eslint reports the same warnings on the file as on develop. The snapshot churn is re-indentation from the new wrapper View plus the slider moving into the dock, and the develop snapshot still passes against the develop source.
Outside the diff, the comment above needsScrollToEnd in the broadcast-failure path (lines 3401-3403) still says the slider floats over the bottom of the scroll view, which stops being true with this change.
The evidence comes from one large simulator. Since the slider now rides the PIN keyboard, one look on a small phone and on Android is worth doing before this goes to staging.
|
Updated the comment above |
The slider floated as an absolute view while the scroll content padded its end by a fixed rem(5). That constant never included the safe-area inset, so the slider (inset + rem(2) gap + 55pt) covered the bottom of the last card. Stealth Send made this visible: its cards push the scene past one screen and put text-heavy warning and error cards last, right under the slider. Render the slider through SceneWrapper dockProps instead, using the same insetStyle/undoInsetStyle pairing as CreateWalletSelectCryptoScene: the scroll view undoes the scene insets so it runs to the screen edge, and its content pads by insetStyle, whose bottom now includes the dock's measured height. The end of the content always clears the slider, whatever the safe area or notification state. The dock rides the keyboard, so the scroll view's extraHeight adds the bottom inset and a focused PIN row lands above the slider. The content padding already reserves the dock, so it stays out of extraScrollHeight, which would also grow the keyboard inset and count the dock twice. This also drops the stale hasNotifications selector, unused since the scene stopped passing hasNotifications to SceneWrapper.
965042f to
30e6d1a
Compare
Each point is answered on the PR. The dock no longer counts twice with the keyboard up (it moved into extraHeight), a new test fires a layout on the dock and checks the content padding, and the comment above needsScrollToEnd now describes the docked slider. The small-phone look (iPhone SE frames 10 and 11) and the Android look (emulator frames 12 to 14) both show every bottom card ending above the slider. The land is approved through the task's Force Land field.
|
@peachbits I dismissed your changes-requested review too early, and GitHub has no way to restore a dismissed review. Please take another look at the current head (30e6d1a). If it still needs changes, a new "Request changes" review puts the block back. This PR will not land until you have re-reviewed it. |
2 participants
Description
On the Send scene, the floating "Enter an Amount" slider covered the last lines of the bottom card (the Stealth Send warning, the Stealth minimum Exchange Error) when scrolled to the end.
The slider was an absolute view, and the scroll content reserved a fixed
rem(5)for it. That constant has left out the safe-area inset since a March 2024 padding change (0ab5b5b). The slider always sat a few points into the content. Stealth Send (0f67bbb) made it visible: its cards push the scene past one screen and render text cards last, right under the slider.The scene now uses SceneWrapper's own bottom-overscroll support:
dockProps, which makes its measured height part of the scene's bottom inset.undoInsetStyle/insetStylepairing from CreateWalletSelectCryptoScene. The view runs to the screen edge and its content pads by the inset, endingrem(0.5)above the slider whatever the safe area or notification state.extraHeight={75 + insetStyle.paddingBottom}lifts a focused PIN row above the slider (75 is the library default).extraScrollHeightstays atrem(2.75): the content padding already reserves the dock, and adding it there would also grow the keyboard inset and count the dock twice.The stale
hasNotificationsselector goes away with the old absolute positioning.A new SendScene2 UI test fires a layout on the dock and checks that the scroll content's bottom padding grows to at least 100.
Tested on iPhone 16 Pro Max and iPhone SE (3rd generation) simulators. Both task repro states scroll to an end with about 20pt between the card and the slider, against about 6pt before. On the SE the Stealth warning and the below-minimum Exchange Error both end fully above the slider. The PIN case was checked with the spending-limit row forced on; with the PIN keyboard up and the scene scrolled to the end, the empty space between the last card and the slider dropped from about 245pt (dock in
extraScrollHeight) to about 113pt (dock inextraHeight).Tested on an Android 16 emulator (1080x2400, 420dpi) at the same head (frames 12-14). With Stealth Send on and the scene scrolled to the end, the Stealth warning ends about 18dp above the slider (12), and the below-minimum Exchange Error ends the same distance above it (13). With the spending-limit PIN row forced on and the PIN keyboard up, the scene scrolls to an end where the PIN row and the Exchange Error both sit above the slider, about 30dp clear, and the slider rides above the keyboard (14).
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Requirements
If you have made any visual changes to the GUI. Make sure you have:
Test evidence
06c61eeDock the Send slider so content clears it
2026-10-16
🪓 forced the spending-limit PIN row on in SendScene2 renderAuthentication to check the PIN keyboard path
1. before stealth on
2. before below minimum
3. after stealth on
4. after below minimum
5. 🪓 before pin keyboard
6. 🪓 after pin keyboard
965042famend! Dock the Send slider so content clears it
2026-10-19
🪓 forced the spending-limit PIN row on in SendScene2; 08 also put the dock back in extraScrollHeight to show the old split; forced the spending-limit PIN row on in SendScene2 renderAuthentication (frame 14 only)
8. 🪓 before pin keyboard scrolled end
9. 🪓 after pin keyboard scrolled end
10. after iphone se stealth on
11. after iphone se below minimum
12. after android stealth on scrolled end
13. after android below minimum scrolled end
14. 🪓 after android pin keyboard scrolled end
Note
Low Risk
Send-scene layout and keyboard scroll behavior only; no changes to signing, balances, or network logic.
Overview
Fixes the Send scene so bottom warning cards (e.g. Stealth Send, exchange errors) are not covered by the confirmation slider when scrolled to the end.
SendScene2 stops using an absolutely positioned slider with a fixed bottom padding (
rem(5)). The slider now mounts throughSceneWrapperdockProps(keyboardVisibleOnly: false), so its measured height feeds the scene bottom inset. The scroll view uses theundoInsetStyle/insetStylepattern so content pads above the dock (including safe area and notifications) instead of relying onhasNotifications-based offsets. Keyboard handling shifts dock clearance intoextraHeight(PIN row above the docked slider) whileextraScrollHeightstays unchanged to avoid double-counting the dock.Adds a SendScene2 UI test that simulates dock layout and asserts scroll
paddingBottomclears the slider height. CHANGELOG notes the fix.Reviewed by Cursor Bugbot for commit 30e6d1a. Bugbot is set up for automated code reviews on this repo. Configure here.