Fix: cursor position after flip (+/-) in new manual expense amount field#94543
Conversation
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
🤖 The failing Failing test: Why it's unrelated:
Recommendation: Re-run The |
|
@MelvinBot Please merge main. After that replace all |
1 similar comment
|
@MelvinBot Please merge main. After that replace all |
|
@MelvinBot Please merge main. After that replace all [ ] characters in PR description by [x]. Finally, open this PR for review |
…orSelection Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@thelullabyy Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-27.at.01.44.18.movScreen.Recording.2026-06-29.at.01.32.42.mov |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f05370d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| setSelection((prevSelection) => ({ | ||
| start: Math.max(prevSelection.start + offset, 0), | ||
| end: Math.max(prevSelection.end + offset, 0), | ||
| })); |
There was a problem hiding this comment.
Preserve manual selection updates on native
On native, the other programmatic update paths set willSelectionBeUpdatedManually (and numberRef.current) before changing selection because the controlled TextInput can emit onSelectionChange while the value update is being applied. This flip path changes the value and then sets the cursor, but leaves that guard unset, so in the new manual expense flow on iOS/Android the selection event can write the old {0,0} selection back after adding -, leaving the cursor before the sign and causing the next digit to be rejected/disappear. Apply the same manual-selection guard used by setFormattedNumber/setNewNumber around this update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid catch — fixed in 6b8c9a5.
On native, shouldIgnoreSelectionWhenUpdatedManually is true, so handleSelectionChange only ignores the spurious onSelectionChange event when willSelectionBeUpdatedManually.current is set. handleFlipPress was changing the value and the selection but never setting that guard (unlike setNewNumber/setFormattedNumber), so the controlled TextInput could write the stale {0,0} selection back and undo the cursor shift.
The flip handler now sets willSelectionBeUpdatedManually.current = true and numberRef.current = newValue before updating the value/selection, matching the other update paths.
|
@MelvinBot Please check this comment and fix if needed #94543 (comment) |
|
@MelvinBot You are using the wrong template for PR description. Let's update to use this https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md Remember to replace all |
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@puneetlath Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.25-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.25-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On the new manual expense flow, tapping the flip (+/-) button in the amount field updated the value but never moved the text cursor (
selection). On an empty field the selection stayed at{0, 0}— i.e. before the prepended-— so the next digit typed produced an invalid string like5-.validateAmountrejects that string,setFormattedNumberreturns early, and the entered number disappears. This reproduced both reported symptoms: "cursor appears before negative sign" and "number disappears after entering a number".handleFlipPressinNumberWithSymbolForm.tsxnow shifts the cursor by the length of the toggled sign (+1 when adding-, -1 clamped to 0 when removing it), keeping it in the same logical position relative to the digits. For the empty-field case the selection becomes{1, 1}, placing the cursor right after the-so subsequent typing stays valid. This mirrors how the imperativeupdateNumberhandler already manages selection.PR
Expensify/App#94134(removing theisNewManualExpenseFlowEnabledbeta) didn't introduce this — it exposed a pre-existing flaw in the new manual flow by enabling it by default. That PR was reverted inExpensify/App#94312to clear the deploy blocker; this PR fixes the underlying bug so the new flow is correct when re-enabled.Fixed Issues
$ #94255
PROPOSAL: #94255 (comment)
Tests
-5).Offline tests
Same as Tests.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari