Repository navigation
Conversation
|
@codex review |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18f415b433
ℹ️ 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".
JakubKorytko
left a comment
There was a problem hiding this comment.
I tested it on the web and couldn't find any problems. Some minor comments, but otherwise LGTM
|
@MelvinBot review |
ReviewThe refactor is clean, and the Enter, Escape and blur paths are safe. 1. Touch wide layouts can no longer flip the sign (agrees with the Codex P2). 2. Replacing the whole number of a negative credit now saves a positive amount. Your existing inline comments still apply: the per-row |
aaa303d to
e49fb2d
Compare
e49fb2d to
7c25f28
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 251b1ceb1e
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@bernhardoj 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 / Safariw.mp4a.mp4 |
grgia
left a comment
There was a problem hiding this comment.
One blocker from running the tests on this branch merged into current main, plus one question on the sign behavior. Details inline.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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/mountiny in version: 9.5.27-0 🚀
|
|
No help site update needed. This PR swaps the input behind inline amount editing, and the existing steps ("hover over amount and click the pencil icon") in Edit Expenses and Getting Started with the Spend Page are still accurate. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.27-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Previously,
TotalCellrelied onMoneyRequestAmountInputfor inline amount editing in transaction table rows (a legacy carryover fromNumberWithSymbolForm-based amount entry). BecauseMoneyRequestAmountInputis designed as a broad form adapter, using it inside a compact table cell required heavy styling overrides to suppress its outer wrappers, borders, and scroll containers. Furthermore, editing negative amounts (such as Split Bills or credits) risked sign desynchronization during mid-edit re-renders.TotalCellis the only inline context in the app requiring a detached minus sign placed outside the currency symbol (-$12.34). Because standard form inputs likeNumericFieldencapsulate the entire signed value inside a single text field,TotalCellcannot useNumericField. Instead, it shares the underlying detached sign controller (useDetachedSignEditingController) and selection mechanics poweringNumericInput, tailored specifically for an inline table cell.This PR replaces
MoneyRequestAmountInputinTotalCellwith a dedicated, lightweightEditableTotalAmountInput:EditableTotalAmountInput):useDetachedSignEditingControllerto render the minus sign, the currency symbol, and the right-aligned amount input as separate visual elements in the row (-$12.34).hasFixedNegativeSignvs.canFlipSign), preventing mid-edit updates from stripping, doubling, or flipping the negative sign on Split Bills.aria-hiddenand prepends unicode minus\u2212toaccessibilityLabelwhen negative so screen readers announce "minus".NumericFlipButtonsupport for touchscreen devices where soft decimal keyboards lack a minus key.useNumericPressSelectionto ensure accurate caret placement without losing focus when clicking the empty cell area to the left of the input.TotalCellLogic:isNegativestate and delegates signed value normalization directly tooriginalValueandgetNormalizedValue.10.00in-$10.00and typing5), treating the edit as a fresh positive entry (+$5.00) to matchNumericInputand the full-screen amount editor.hasFixedNegativeSignpreserves the sign even when the entire magnitude is replaced.useDetachedSignEditingControllercovering standalone table cell usage, sign locking, and full-magnitude replacement.TotalCellTest.tsxto verify inline editing flows, minus sign preservation for Split Bills, whole-range selection replacements, long amount boundaries, and discarding draft edits onEscape.NumericFlipButtonand touchscreen vs. non-touchscreen rendering (TotalCellWithoutTouchScreenTest.tsx).Fixed Issues
$ #98171
PROPOSAL: #97970
Tests
A. Wide Layout
Tabto focus the edit button and hitEnter).10.00to25.50) and click outside the cell (or pressEnter):-$12.34).12.34with50.00).-$50.00).10.00in-$10.00with5).+$5.00), matching full-screen amount editor parity.9999999999.99):Escape:B. Narrow Layout
-$prefix for negative amounts).Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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 (wide layout)
Before
before_wide.mov
After
after_wide.mov
MacOS: Chrome / Safari
web.mov