refactor: the new-transaction highlight rail and the wide RHP width model - #98744
refactor: the new-transaction highlight rail and the wide RHP width model#98744TaduJR wants to merge 21 commits into
Conversation
…ts legacy mechanisms
…ports-newTransactionHighlight-and-RHPLayout-followups # Conflicts: # src/selectors/ReportMetaData.ts
…ports-newTransactionHighlight-and-RHPLayout-followups
…ports-newTransactionHighlight-and-RHPLayout-followups
…ports-newTransactionHighlight-and-RHPLayout-followups
…ports-newTransactionHighlight-and-RHPLayout-followups
…ports-newTransactionHighlight-and-RHPLayout-followups # Conflicts: # src/selectors/ReportMetaData.ts
…ports-newTransactionHighlight-and-RHPLayout-followups # Conflicts: # src/components/Search/index.tsx # src/libs/actions/IOU/MoneyRequestBuilder.ts # src/pages/iou/request/step/confirmation/useExpenseSubmission.ts # tests/actions/IOU/GetMoneyRequestInformationTest.ts
|
@abzokhattab 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71b330f5f7
ℹ️ 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. Nice work! 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". |
|
@MelvinBot @codex review the PR and check for regressions, breaking changes or bugs |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
PR Review — regressions, breaking changes & bugsVerdict: Solid, well-tested refactor. No High-severity defects, no crashes, and no data-correctness issues. The Onyx shape change is backward-safe, all changed signatures are fully propagated, and the wide-RHP width derivation reproduces the old hand-synced behavior. Everything I flag below is highlight/flash cosmetics or a deliberate behavior change — nothing blocks correctness. I reviewed the three subsystems (marking, wide-RHP width, animation) against Worth a look before merge1. A deferred flash can be dropped after 4s when a wide RHP covers the report preview (low–med, cosmetic; please verify manually) 2. 60s freshness window silently drops a legitimately-delayed highlight (behavior change vs Minor / suggestions
Claims I verified as SOLID (no defect)Marking / storage shape
Wide-RHP width model
Animation
Automated review — three focused passes over the marking, wide-RHP, and animation subsystems, with adversarial verification. Findings #1 and #3 are worth a quick manual/unit check; the rest is FYI. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d40ac7687
ℹ️ 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".
| selector: pendingNewTransactionIDsSelector, | ||
| }); | ||
| const newTransactions = useNewTransactions(reportLoadingState?.hasOnceLoadedReportActions, reportTransactions, pendingNewTransactionIDs, reportIDFromRoute, isFocused); | ||
| const newTransactions = useNewTransactions(reportLoadingState?.hasOnceLoadedReportActions, reportTransactions, pendingNewTransactionIDs, reportIDFromRoute, isReportVisible); |
There was a problem hiding this comment.
Avoid consuming highlights in covered desktop reports
When a desktop report remains mounted after the user switches to another fullscreen tab, shouldUseNarrowLayout is false, so isReportVisible remains true even though isFocused is false. Passing that value here starts the four-second sweep and the highlight animation while the report is completely covered; returning after the timer therefore shows no highlight. The same regression exists in MoneyRequestReportPreview, so visibility should distinguish a report exposed behind an RHP from one covered by a fullscreen navigator.
Useful? React with 👍 / 👎.
Explanation of Change
Follow-up to #89585, which shipped the duplicate-expense highlight. The behaviour is the same duplicating an expense still highlights the new row and widens the panel but the code behind it is simpler and a few rough edges are fixed.
Marking a new expense. Each mark now records when it was written, so clearing an old mark can't wipe a newer one for the same expense. And only one place decides whether to write a mark, instead of two that could disagree about it.
The wide panel. Which screens show wide is now worked out from the navigation state as it renders, instead of being kept in a second copy that had to be updated by hand. That removes the two functions doing the syncing, plus an API nothing read. A panel that's closing also keeps its width until the animation finishes, rather than snapping narrow straight away.
The animation. The row appears as soon as it's added, and only the flash waits until the screen is in front so an expense added while you're looking at something else is still highlighted when you come back. A highlight now also survives the list re-sorting underneath it.
One thing worth knowing when reviewing: the stored shape of
pendingNewTransactionIDschanged. Marks written by the current production build are ignored rather than read. Nothing needs migrating old entries are cleaned up the first time they're read.Fixed Issues
$ #96327
PROPOSAL:
Tests
**Prerequisite: **
Re-opening a chat you already visited in the same session can make existing expenses flash in that chat's expense cards. That is issue #98308 — already live in production, neither caused nor fixed by this PR, and it reproduces identically without these changes. Please don't file it against this PR, but do mention it if you see it, because it can be mistaken for a failure of test 7.
1. A duplicated expense is highlighted
2. The first expense on a report is not highlighted
3. Editing an existing expense does not highlight it
4. Two additions in a row each get their own highlight
5. A highlight survives the row moving
6. Deleting and re-adding highlights again
7. Switching between expense reports does not highlight
8. Bulk duplicate
9. The panel opens at the right width (web/desktop, maximised)
10. The panel keeps its width while closing (web/desktop, maximised)
11. Closing one screen off a stack keeps the width (web/desktop, maximised)
12. The panel shrinks when the report loses expenses (web/desktop, maximised)
13. Switching tabs while the panel is open (web/desktop, maximised)
14. Moving between expenses with the arrows (web/desktop, maximised)
15. On a wide screen the highlight plays behind a stacked panel (web/desktop, maximised)
16. On mobile the highlight waits for you (iOS, Android, or a narrow browser window)
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Mac-Chrome.mp4