fix: sort and total Search amounts by the sign the row displays - #99747
Conversation
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.
|
Julesssss
added
Melvin-Test-Web
Android test run: blocked on test dataResult: partial. I reached Search → Expenses on the Android build of this branch ( Your own precondition calls this out: "Needs an account with at least one negative expense (a credit or refund) that is either unreported or on a workspace report whose type is not
To get this verified on Android, the run needs a session seeded with expense data before Melvin starts: at least one +$100 expense and one -$100 credit/refund, with the credit either unreported or on a non- Separately, the open question in your description — whether the backend Search sort is signed — is still unanswered and would need someone with Web-Expensify/Auth access. If pagination sorts by magnitude server-side, the client fix alone won't stop negative rows appearing mid-list as pages arrive. Evidence (1)Search > Expenses view showing 'No expenses yet' empty state after clearing the From filter
Evidence (1)Recordings- [▶ Session recording of the verification run](https://melvinbot-assets.exops.io/session-33195170914-1-cf5bfe81-95aa-49de-ae82-b85823612f26.mp4) |
Web test run: blocked on test data (same as Android)Result: fail. I reached Search → Expenses on web, but the test account has zero expenses — not just no negative one. Every assertion in your Tests section depends on rows existing, so steps 2–9 could not be exercised. This is the identical blocker the Android run hit. Your own precondition names it: "Needs an account with at least one negative expense (a credit or refund) that is either unreported or on a workspace report whose type is not
JS console errors: not captured. The web driver in this session has no console-inspection command, so the "no errors appear in the JS console" checkbox in your PR is still unverified by me. Run note: after reaching the empty state, a stale element reference clicked "Try it out" instead of "Dismiss" on the "Pre-built agents" onboarding tooltip, which opened the full-window Test Drive overlay. The tester stopped there per its own policy rather than trying to recover. That happened after the blocker, so it changed nothing about the outcome. To get this verified, the run needs a session seeded before Melvin starts: at least one +$100 expense and one -$100 credit/refund, with the credit either unreported or on a non- Also still open from your description: whether the backend Search sort is signed. If pagination sorts by magnitude server-side, the client fix alone won't stop negative rows appearing mid-list as pages arrive. That needs someone with Web-Expensify/Auth access. Evidence (4)Global quick-search dialog opened from Home
Quick-search results showing 'Go to Expenses' option
Search > Expenses view showing 'No expenses yet' empty state
Test Drive overlay that ended the run early
Evidence (1)Recordings- [▶ Session recording of the verification run](https://melvinbot-assets.exops.io/session-33195174951-1-d2b0f35a-6dfc-4db8-b5e9-b831078115d8.webm) |
|
@mkhutornyi 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| /** | ||
| * Returns a transaction's amount with the sign it is displayed with. A transaction on an expense report, on a group | ||
| * policy, unreported, or deleted keeps its sign; any other transaction returns its magnitude. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This newly added JSDoc joins two independent clauses with a semicolon. Per the plain-comment-style guidance, prefer two separate sentences over a semicolon so the comment reads like natural prose.
Split the clause into two sentences:
/**
* Returns a transaction's amount with the sign it is displayed with. A transaction on an expense report, on a group
* policy, unreported, or deleted keeps its sign. Any other transaction returns its magnitude.
*/Reviewed at: faed17f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
garrettmknight
left a comment
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
| * policy, unreported, or deleted keeps its sign. Any other transaction returns its magnitude. | ||
| */ | ||
| function getTransactionDisplayAmount(transaction: OnyxInputOrEntry<Transaction>, report: OnyxInputOrEntry<Report>, policy: OnyxInputOrEntry<Policy>): number { | ||
| const isFromExpenseReport = (!isEmptyObject(report) && isExpenseReport(report)) || isGroupPolicyPolicyUtils(policy); |
There was a problem hiding this comment.
This is duplicated with
Line 5073 in 1126a28
Is it worth extracting?
function isTransactionFromExpenseReport(report: OnyxInputOrEntry<Report>, policy: OnyxInputOrEntry<Policy>): boolean {
return isExpenseReport(report) || isGroupPolicyPolicyUtils(policy);
}
There was a problem hiding this comment.
Extracted isTransactionFromExpenseReport and used it in both places, without the !isEmptyObject(report) guard since isExpenseReport already returns false for undefined, null and {}.
| // Sources are expense-signed (the negation of the displayed amount), so callers sum them with `total - source`. | ||
| function getEntrySource(entry: SelectedTransactionInfo): number { | ||
| return entry.groupAmount ?? -Math.abs(entry.amount); | ||
| return entry.groupAmount ?? -(entry.displayAmount ?? Math.abs(entry.amount)); |
There was a problem hiding this comment.
displayAmount? plus a ?? fallback hides the unhandled case.
formattedTotal is required on TransactionListItemType, so that inner fallback is unreachable for every entry mapTransactionItemToSelectedEntry builds.
The only entries that reach it are the ones from mapEmptyReportToSelectedEntry, which is exactly the case that still totals by magnitude and makes mixed selections wrong.
The optional field plus ?? converts an unhandled case into a silently plausible number.
Either populate it in both builders and make the field required, or branch on the report-row case explicitly so it's visible.
There was a problem hiding this comment.
Made displayAmount required and set it in both branches of mapEmptyReportToSelectedEntry, reducing getEntrySource to entry.groupAmount ?? -entry.displayAmount and fixing the Reports view total too, which I can split out if you'd prefer.
|
@codex review |
|
@MelvinBot review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4969f1f6a3
ℹ️ 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".
| return 0; | ||
| } | ||
| const totalAmount = getAmount(t, t.report?.type === CONST.REPORT.TYPE.EXPENSE); | ||
| const totalAmount = getTransactionDisplayAmount(t, t.report, t.policy); |
There was a problem hiding this comment.
For an unreported credit (or a transaction without a report under a group policy), this helper now returns a negative total, but the visible Total per attendee cell is still calculated with getAmount(transactionItem, isExpenseReport(report)) in TransactionItemRow/index.tsx, which returns the positive magnitude in those cases. Consequently, sorting this column orders rows by values with signs opposite to those shown—for example, a displayed $25 tracked credit sorts as -$25. Either update the rendered calculation to use the same display helper or retain the existing calculation here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The cell in TransactionItemRow/index.tsx:250 still uses the narrow calculation, so I reverted the sort change instead. Cell and sort match again like they did before.
The display side is a separate pre-existing issue for tracked credits, and I'd rather not change a rendered value in a shared component here.
|
🤖 Re-review of
What the new commit fixedGroup conversion stamp — Total per attendee — I re-swept for stragglers. The three remaining The excluded-credit case is now resolved, and the author picked the right sideI previously flagged That premise is verifiable from the client alone, and it holds. The footer has two paths that must produce the same number for the same set of rows: the partial-selection path sums The One cosmetic leftover
On the mixed-currency ordering you raised
What actually blocks it is pagination. The backend picks which rows land on each page using the amount as stored, unconverted — the comment at Runtime verification failed again, same wayNobody has runtime evidence on this PR yet, so I retried — deliberately on the positive-amount path, which needs no credit expense. The run died in the same place as last time: during expense creation a referral panel appeared, the UI re-rendered, and a click aimed at that panel's Back button landed on a link to the public marketing site, which navigates the whole window off the app and drops the session unrecoverably. No expenses were created, so none of the four checks ran.
That's twice with the identical cause, so I'd treat automated verification of this PR as unavailable rather than pending. The Static checks: everything passes on |
|
🤖 Web test steps verification results: No regressions found. Amount display, Amount-column sorting both directions, row-level footer totals, report-group footer totals, and amount-edit persistence all behaved correctly on dev NewDot web. Two gaps: the account had no negative-amount expense, so the signed sorting and net-to-zero footer behavior this PR actually fixes was not exercised — steps 3–7 of your Tests section still need a human with a credit or refund. And the literal in-table inline-edit gesture on the Amount cell could not be driven headlessly, so edit-and-persist was verified through the expense detail page reached from a Search row instead.
|
|
🤖 Code quality review of 1. The report view's Amount sort key was left on the old classification
case CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT:
return getTransactionAmount(transaction, isExpenseReport(report), transaction.reportID === CONST.REPORT.UNREPORTED_REPORT_ID);This is the sort key for the report view's Amount column ( return getTransactionDisplayAmount(transaction, report, policy);The divergence is narrow — only deleted transactions and report-less group-policy rows — which is why this isn't blocking. But it's exactly the class of bug this PR exists to fix. 2. The
|
|
@mkhutornyi The review is completed on your side? |
yes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e90b49e92
ℹ️ 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".
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Looks good to me, did not find anything odd, lets give it a go in staging
|
🤖 Web test steps verification results: ❌ fail I could not run the test steps. The browser session died during setup — a stale element ref resolved to the "refer your team / Learn more" link inside the Create Expense panel and navigated the tab off-app to the public marketing site, which is unrecoverable for this session type. No expenses were created, so none of the sorting or footer-total steps were exercised. Treat everything below as static analysis, not verified behavior. Reading the diff instead, I found no regression for ordinary positive amounts, but three things worth your attention. 1. The group-policy half of the fix never runs in Search
But at both Both are
2.
|
|
All of MelvinBot's code findings are already addressed |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 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.4.76-0 🚀
|
Help site review for this PRYes, one article needed updating. Draft PR: #100900 No existing sentence in What changed and why
Everything lands in UI labels I verified, and howI did not take label capitalization on trust:
Written against Two things worth flagging:
@BartekObudzinski, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
7 participants
Explanation of Change
Two Search bugs where a negative amount is handled as its magnitude.
The Amount column's sort key (
formattedTotal) treated a row as an expense only whenreport.type === 'expense', while the cell that renders the amount (TotalCell) also counts group-policy rows and unreported/tracked expenses.getAmount()drops the sign for anything outside that classification, so those rows displayed a negative sign but sorted by magnitude, which is how a -$4,000 credit ends up among the +$4,000 expenses. Both paths now call one helper,getTransactionDisplayAmount()inReportUtils, using the same classificationgetTransactionDetails()already applies everywhere else.formattedTotalis only ever read as a sort key, so no displayed number changes.The selection footer summed
-Math.abs(transaction.amount)per row, so selecting a -$100 credit and a +$100 expense reported $200 instead of $0 while a whole-group selection already netted correctly from the signedgroupAmount. Rows now carry adisplayAmountand the footer sums that; the four copies of the sum expression are collapsed intogetEntrySource().Note for reviewers: the
Math.abs()incompareValues()that looks like the cause is dead code. The Amount column passes the sorting propertyformattedTotal, never'amount', and the Original Amount column has its own branch that passesshouldCompareOriginalValue. I left it alone because removing it means dropping a parameter from five call sites, which is unrelated churn.Deliberately out of scope:
getTotalFormattedAmount()inactions/Search.tsalso usesMath.abs, but it feeds the bulk Pay button label, where a positive sum is intended.One thing I could not check from the App repo: whether the backend Search sort is signed. If it is not, pages can still arrive in an order that differs from the one the client renders, which shows up as rows appearing mid-list while scrolling. That needs someone with Web-Expensify/Auth access.
Draft because automated checks pass but I have not run the manual or on-device steps below.
Fixed Issues
$ #99918
PROPOSAL:
Tests
Needs an account with at least one negative expense (a credit or refund) that is either unreported or on a workspace report whose type is not
expense.Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
Screen.Recording.2026-08-28.at.14.11.46.mov