Forward/Back navigation buttons disappear after submitting a report#94928
Conversation
…f allReports and improve component stability
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f41e89c72
ℹ️ 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".
|
@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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd53844ef9
ℹ️ 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".
mkhutornyi
left a comment
There was a problem hiding this comment.
Looks good but please check above comment and fix conflict
ReviewThe root-cause analysis is correct and the fix is sound. The bug was a component-subtree swap: the old parent rendered either Two things worth addressing before merge: 1. Performance regression:
|
|
@mkhutornyi I fixed conflicts & resolved codex comments. Kindly check again |
|
@codex review |
|
🤖 Regression scan — the fix itself is sound (one stable
|
There was a problem hiding this comment.
💡 Codex Review
App/src/hooks/useSearchSections.ts
Line 54 in ce54998
When contextReports.length > 0 && !isSearchLoading, this guard only skips getSections; the hook has already run the unconditional useOnyx subscriptions above for card feeds, report NVPs, bank accounts, and useReportAttributes. Since MoneyRequestReportNavigation now always calls this hook, opening a report from Search with precomputed sorted IDs no longer uses the previous lightweight fast path and will re-render on those large collections even though it ultimately returns contextReports. Please keep the heavy fallback in a separate component/hook or otherwise avoid mounting these subscriptions for the context path.
ℹ️ 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".
|
@lorretheboy please check ^ |
|
@TMisiukiewicz you wanna do secondary review? |
| // All Onyx subscriptions via useSearchSections. Mounts if there are no sorted report IDs in the context. | ||
| function MoneyRequestReportNavigationStandalone({reportID, shouldDisplayNarrowVersion}: MoneyRequestReportNavigationProps) { | ||
| const {allReports, isSearchLoading, lastSearchQuery} = useSearchSections(); | ||
|
|
||
| return ( | ||
| <MoneyRequestReportNavigationContent | ||
| reportID={reportID} | ||
| shouldDisplayNarrowVersion={shouldDisplayNarrowVersion} | ||
| allReports={allReports} | ||
| isSearchLoading={isSearchLoading} | ||
| lastSearchQuery={lastSearchQuery} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
@MelvinBot #86238 was partially reverted. Example: this code was removed.
Won't it cause performance regression back (what #86238 fixed)?
|
Good question — and the distinction matters, so let me be precise. Short answer: No, this does not bring back the specific regression What the removed fast-path branch did cost: the old code skipped So to your worry directly: the expensive part #86238 removed stays removed; what came back is the lighter subscription cost the fast path used to avoid. If you want to close even that gap without giving up the always-mounted structure the bug fix needs, the cleanest option is the one from my earlier comment / codex's: lift the Why the sort frequency is unchanged even during SubmitSubmitting toggles |
|
@mkhutornyi I updated code to address this perf issue #94928 (review) |
|
🤖 Regression scan — PR #94928 The change is well-scoped and the core fix (keeping one stable 1. (Highest priority) The unit test no longer tests the component it's protecting. The updated test defines a local 2. New one-render lag on the slow path. Previously the slow path read 3. Stale What I checked and why the rest looks safe
I was not able to run the jest suite or a full typecheck in this environment (sandbox blocked Next Steps: Reply with |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
|
@lorretheboy can you update the PR body with an explanation of your changes, please? |
|
✋ 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! 🧪🧪
|
|
@mountiny I updated the summary of changes |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.32-0 🚀
|
Help site review — no changes requiredI reviewed the changes in this PR and no updates to the help site ( Why:
Since there is nothing user-facing to document, there is no linked help site PR to open, label, or assign.
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.32-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
I moved the cache out of the swapping children and into the stable parent.
MoneyRequestReportNavigationitself never unmounts once shouldKeepMounted is set, so the lastValidReports/effectiveAllReports logic that currently lives in MoneyRequestReportNavigationContent should be lifted there:App/src/components/MoneyRequestReportView/MoneyRequestReportNavigation.tsx
Lines 95 to 100 in ec76752
Compute the allReports source (fast-path context list vs. useSearchSections) in the parent, keep the "last list containing the report" cache there, and pass the resolved effectiveAllReports down to a single presentational content component. That way toggling isSearchLoading only changes which data feeds the cache - it no longer destroys it - so the cached list (still containing the just-submitted report) keeps the arrows visible.
Fixed Issues
$ #93750
PROPOSAL: #93750 (comment)
Tests
Offline 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
aw.mov
Android: mWeb Chrome
a.mov
iOS: Native
ia.mov
iOS: mWeb Safari
i.mov
MacOS: Chrome / Safari
w.mov