Repository navigation
Fix 85553 rbr carousel sorting - #86477
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const [reportDetailsColumns] = useOnyx(ONYXKEYS.NVP_REPORT_DETAILS_COLUMNS); | ||
| const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED); | ||
| const [draftTransactionIDs] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {selector: validTransactionDraftIDsSelector}); | ||
| const [allTransactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS); |
There was a problem hiding this comment.
❌ PERF-11
useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS) subscribes to the entire transaction violations collection without a selector. This component will re-render whenever any transaction violation changes across the entire app, not just those belonging to the transactions in this report.
Consider using a selector that filters to only the relevant transaction IDs, or compute violations per-transaction inside the useMemo using the transaction IDs already available from the transactions prop.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
✅ This is fine, we want to rerender whenever any violation changes across the app, not just those belonging to the transactions in this report.
| }, [sortBy, sortOrder, transactions, localeCompare, report]); | ||
| return [...transactions].sort((a, b) => { | ||
| // When on default sort (Date/ASC), prioritize RBR-flagged transactions | ||
| if (isDefaultSort && allTransactionViolations) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3
The RBR-aware sort comparator logic (violation lookup + transactionHasRBR comparison) is duplicated nearly identically in both MoneyRequestReportTransactionList.tsx and MoneyRequestReportPreviewContent.tsx.
Extract a shared compareByRBR(a, b, violations) comparator into TransactionPreviewUtils.ts next to transactionHasRBR so both call sites can reuse it.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This makes sense, repeated code should be extracted to a shared utility function in TransactionPreviewUtils.ts.
There was a problem hiding this comment.
@abelhailefen Mind addressing this valid refactoring concern ?
There was a problem hiding this comment.
This is important, we should use the existing logic
|
I have read the CLA Document and I hereby sign the CLA |
![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
https://github.com/Expensify/App/blob/56394089d33b3c6b6d81eb52acc8fd8cfc555d2e/mkcert#L1
This commit adds a 4.7MB ELF executable at the repo root (mkcert), which is unrelated to the sorting fix and introduces an opaque binary artifact into source control. Keeping committed executables like this increases clone size, complicates audits/reproducibility, and can trigger security/compliance concerns in CI pipelines; this should be removed from the commit.
ℹ️ 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".
| }, [sortBy, sortOrder, transactions, localeCompare, report]); | ||
| return [...transactions].sort((a, b) => { | ||
| // When on default sort (Date/ASC), prioritize RBR-flagged transactions | ||
| if (isDefaultSort && allTransactionViolations) { |
There was a problem hiding this comment.
The RBR prioritization is currently gated by allTransactionViolations, so if that collection is unavailable/undefined, the comparator skips transactionHasRBR() entirely and falls back to plain date sorting. In that state, transactions that should still be considered RBR via non-violation signals (hold, missing merchant/amount, receipt error) are not surfaced to the top, so the new behavior silently fails for reports without loaded violation entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the new behavior silently fails for reports without loaded violation entries.
This is expected.
This comment was marked as resolved.
This comment was marked as resolved.
|
I have updated the PR to follow the Author Checklist |
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
@abelhailefen Thanks for the update! Before I can start reviewing, can you please add the (After only) video proof for all other platforms (iOS/Android Native and mWeb - 4 videos in total) at the bottom of the PR Author Checklist ? This is an absolute requitement for any contributor PR in order to prove you actually tested on all platforms - if you don't have that proof and did not test on those platforms, there could be regressions that are never caught on Web only (big layout) 🙌 |
There was a problem hiding this comment.
This file should be removed from the PR.
There was a problem hiding this comment.
@abelhailefen Mind removing this file ? It's a new file being commited to the codebase which shouldn't.
|
@abelhailefen How is it going with the PR Author Checklist completion (videos for native platforms) ? Asking since that's the only blocker left for me to be able to proceed with review and approve. ℹ️ Note that's been about 1 week since my last comment without a response from you and this can be seen as abandoning the issue / PR, per our guidelines:
|
|
@ikevin127 hello sorry for the late response but since i don't have a mac i'm unable to build for ios native ive been trying to find a laptop i can work on but i was unsuccessful. I'm currently building for android native both my pc and phone are low spec so thats been a challenge but i will make that available as soon i can and ill let you know. |
|
@ikevin127 hello I've just added the android native after video proof which is the 4th one. The only one missing now is the ios native I am still unable to provide the ios native one because i dont have my own iphone and mac. |
|
Understood, I'll pick things up from here and make sure to test on all platforms as part of the review ✅ |
|
thank you for your understanding i wont apply for issues that require an ios and mac from here on out until i acquire those platforms |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios.moviOS: mWeb Safariios-mweb.movMacOS: Chrome / Safariweb.mov |
🔄 Review summary🟢 Reviewer checklist completed.
Recommendation: This PR is well-architected and solves a real user problem. The implementation is clean, well-tested, and follows Expensify patterns. Address the HIGH priority item (DRY violation) before approval. The DRY refactoring and formatting fixes can be addressed either before merge or as fast-follow items. @abelhailefen Before I can approve please address the following issues:
Once you completed the remaining tasks, please tag me and I'll do a final pass before approval 👍 |
|
@ikevin127 i have addressed the issues i have merged with main, i have removed the mkcert file and i have made the RBR-aware sort comparator logic dry. I've push all the changes to the branch. I'd like to point out that once i merged there were some squiggly line errors but they weren't related to my code plus i verified its running and the sorting works and i have ran the tests as well |
|
Thanks for the updates! I'll do a final check on the PR soon. You'll probably need to merge main again once CME allows the workflows to actually run (they require approval for first time contributors) which could reveal some failing workflows. @abelhailefen In the meantime could you please remove the Also please try to not force-push changes in PRs as it messes with the branch history - it's mentioned in guidelines here:
|
|
okay ive been i using rebase to merge the branches thats why i needed to force push ill just pull from main and merge from here on out but i do need to force push the revert for package-lock.json |
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.
|
|
@abelhailefen Looks like the workflows were approved and we can see which ones are failing, all must pass before moving forward:
basically all your commits need to be verified otherwise merge cannot occur - so for this one you'll need to force push again, I recommend:
|
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
@ikevin127 would you please final review
|
@ikevin127 @grgia hello I'm concerned another merge conflicts might happen if we wait too long if everything is good can it be merged to main? |
|
@grgia Approved once again, everything seems 🟢 on workflows - could you please merge ? 🙌 |
|
Yep, running now |
|
🚧 @grgia 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! 🧪🧪
|
|
@ikevin127 I haven't gotten an invite on upwork. I've sent a proposal but it hasn't been accepted |
|
Don't worry about it, as I mentioned in the issue: after merge, PR must deploy to stging first, then production, and 7 day after production deploy the payment is issued if no regressions occured in the meantime as a result of the PR. Automations will add payment due date on the issue once PR is deployed to production - when payment is due, you will get the offer and get paid. This being your first PR, it took unexpectedly long to merge - but next ones should go smoothly. |
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.3.69-0 🚀
Bundle Size Analysis (Sentry): |
|
Yes, help site changes are needed. The RBR sorting prioritization introduced in this PR affects two documented features:
I've created a draft PR with the documentation updates: #90016 |
|
Deploy Blocker ##90063 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.69-18 🚀
|
|
@ikevin127 hello do i need to do anything? |
|
@abelhailefen Yes, the initial PR caused a regression issue here, please review the comments I posted there regarding next steps and proceed with another follow-up PR to fix the regression issue: |
|
@ikevin127 I'm going to fix the regression issue I wasn't getting the notification till now |
|
@ikevin127 do I open a new PR how do I proceed? |
|
@abelhailefen Yes, you go to the regression issue here, checkout the issue to understand the bug that was missed in the initial PR and note the clarifying solution comment (at bottom) and proceed to implement a fix and when ready open a PR for that issue like you did for the initial issue. |
|
@ikevin127 okay thank you |
Explanation of Change
Prioritize RBR-flagged transactions in carousel and transaction list sorting
When an expense report contains transactions that require user attention (violations, holds, missing fields, receipt errors), those actionable items were buried in the middle of the list based on their creation date. Users had to scroll through all transactions to find the ones needing action.
This PR introduces RBR-aware sorting so that transactions with Red Brick Road (RBR) indicators are automatically surfaced to the top of both the carousel preview and the transaction list when using the default Date sort. This ensures users see actionable items first without additional clicks or scrolling.
How it works:
transactionHasRBR()helper checks transaction-level signals (violations, hold status, missing merchant/amount, receipt errors) without requiring heavy report/policy context.MoneyRequestReportPreviewContent) and the transaction list (MoneyRequestReportTransactionList) sorting logic now prioritize RBR-flagged transactions on the default sort only (Date/ASC). When the user explicitly changes the sort column, standard sorting applies — the RBR pinning deactivates to respect the user's intent.Fixed Issues
$ #85553
PROPOSAL: #85553 (comment)
Tests
Precondition: Log in with an account that has a workspace. If you don't have one, click + Create > New Workspace and follow the setup flow.
Web test
Mobile test
Same as web tests.
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
video_2026-04-09_13-40-57.mp4
Android: mWeb Chrome
Screen_Recording_20260330_134802_Samsung.Internet.mp4
iOS: Native
iOS: mWeb Safari
video_2026-04-05_21-59-21.mp4
MacOS: Chrome / Safari
Screen.Recording.2026-03-30.at.15.28.00.mp4