Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
3948493 to
2e7566c
Compare
2e7566c to
727d764
Compare
| } | ||
|
|
||
| return ( | ||
| <NavigationDeferredMount |
There was a problem hiding this comment.
@BartekObudzinski Let's put ReportActionsWithInboxTabDeferredMount into src/pages/inbox/ReportActions.tsx
|
@BartekObudzinski I love the idea. The transition is now smoother since we no longer perform the navigate function. I also noticed that the change fixes some flickers during the transition that happen when using the navigate function Screen.Recording.2026-07-18.at.17.37.25.mov |
| // Deep links and REPORT_WITH_ID navigation pass the reportID in nested params, | ||
| // which lets us skip the O(n) findLastAccessedReport scan over all reports. | ||
| if (route.params?.screen === SCREENS.REPORT && route.params.params?.reportID) { | ||
| if (route.params && 'screen' in route.params && route.params.screen === SCREENS.REPORT && route.params.params?.reportID) { |
There was a problem hiding this comment.
I'm not sure we need to change this if condition. It felt a little easier to read before
There was a problem hiding this comment.
route.params can now be either NavigatorScreenParams or the marker-only {shouldDeferInitialReportActions} object. The marker branch has no screen property, so the in check is needed to narrow the union before reading nested params.
|
|
||
| type RootNavigationState = NavigationState | PartialState<NavigationState> | undefined; | ||
|
|
||
| function isRecord(value: unknown): value is Record<string, unknown> { |
There was a problem hiding this comment.
It looks like we already have an isRecord utility in @libs/ObjectUtils.ts:41. That version is actually a bit more robust since it also checks for !Array.isArray. Could we just import that one here instead of creating a new copy?
| return getTabNavigatorRoute(rootState)?.state?.key; | ||
| } | ||
|
|
||
| function getReusableReportsTabStateKey( |
There was a problem hiding this comment.
I wonder if we could reuse or extend getTopmostReportParams here? It might help us avoid manually repeating the REPORTS_SPLIT_NAVIGATOR -> REPORT traversal and reduce duplication in the route-finding logic
There was a problem hiding this comment.
getTopmostReportParams can prefer a top-level Reports split and only returns report params. This helper needs the specific Reports tab instance, its initialized nested state, and the tab state key used as the jump target, so reusing it would still require the same traversal and could validate a different navigator.
|
@DylanDylann 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] |
![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: 468d2dea06
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
🤖 I verified the Tests steps on web (dev NewDot, wide layout). Result: passed — the Inbox report renders fully with no persistent skeleton across repeated Home↔Inbox navigations, the URL stays
Evidence & caveats
Screenshots
▶ Session recordings (1) |
Tests verification — wide web (dev build @
|
| # | Step | Result |
|---|---|---|
| 1 | Home shows no central report | ✅ Pass |
| 2 | Refresh → Inbox shows skeleton once, then cached report | |
| 3 | Home → Inbox again shows same report immediately, no skeleton | ✅ Pass |
| 4 | Repeat Home → Inbox, URL stays /r/:reportID |
✅ Pass (/r/6423160280876552) |
| 5 | Browser back/forward restores Home + same report | |
| 6 | Open another report from LHN, no defer skeleton | ✅ Pass |
| 9 | No JS console errors | ✅ No errors detected |
Overall: No regressions found — repeated Home → Inbox navigation consistently restores the same cached report immediately, the URL stays /r/:reportID, and opening other reports from the LHN renders without a defer skeleton. All API calls returned 200 and no console errors surfaced.
Caveats on steps 2 and 5
- Step 2 (skeleton): The cached Concierge report rendered immediately on every Inbox open, and I never captured the one-time loading skeleton. This is expected when report data is already warm in the Onyx cache (content paints synchronously). The test harness also can't reproduce a session-preserving
F5refresh — itsopenalways creates a fresh browser context — so the exact "skeleton appears once after refresh" moment couldn't be isolated. This is a harness limitation, not evidence of a defect; the "no skeleton on subsequent visits" behavior (steps 3–5) was confirmed. - Step 5 (browser back/forward): Programmatic browser back/forward isn't supported for web in the test harness. URL transitions between
/homeand/r/6423160280876552were confirmed correct via network referers, but history-based restoration wasn't directly exercised.
These two steps would benefit from a manual pass by the author/reviewer.
Evidence
Step 2 — Inbox opened after fresh load, cached report renders (no skeleton captured):
Step 3 — same report shown immediately on repeat navigation:
Step 4 — URL stays /r/:reportID on repeat navigation:
Step 5 — Inbox report consistently restored:
Step 6 — different report (Melvin Bot DM) opened from LHN, no defer skeleton:
|
PR doesn’t need product input as a perf refactor PR. Unassigning and unsubscribing myself. |
| return rootState?.routes.findLast((route) => route.name === NAVIGATORS.TAB_NAVIGATOR); | ||
| } | ||
|
|
||
| function getReportsTabStateKey(rootState: RootNavigationState): string | undefined { |
There was a problem hiding this comment.
NAB: I wonder if the function name could be adjusted as we return here TabNavigator's state key
There was a problem hiding this comment.
Good call, renamed it to getTabNavigatorStateKey.
sumo-slonik
left a comment
sumo-slonik
left a comment
There was a problem hiding this comment.
Looks good. I went through the spots that seemed risky (react-freeze, the deferred-mount lifecycle, the raw jumpTo dispatches) and didn't find any bugs. Just one small inline NAB. Nice work! 🎉
| const tabNavigatorRoute = getTabNavigatorRoute(rootState); | ||
| const tabState = getTabState(tabNavigatorRoute); | ||
| const reportsSplitRoute = tabState?.routes.findLast((route) => route.name === NAVIGATORS.REPORTS_SPLIT_NAVIGATOR); | ||
| const preservedReportRoute = reportsSplitRoute?.state?.routes.findLast((route) => route.name === SCREENS.REPORT); |
There was a problem hiding this comment.
NAB: Worth adding a quick comment here. preservedReportID comes from the current navigation state, while reportID comes from the preserved snapshot (which may lag while the Reports tab is frozen). A mismatch is expected and intentionally triggers a full navigation. The preserved* naming is also a bit misleading since these variables actually represent the live route state.
There was a problem hiding this comment.
done, thank you
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-27.at.17.06.21.movAndroid: mWeb ChromeScreen.Recording.2026-07-27.at.17.05.07.moviOS: HybridAppiOS: mWeb SafariScreen.Recording.2026-07-27.at.17.04.42.movMacOS: Chrome / SafariScreen.Recording.2026-07-27.at.17.02.04.mov |
| @@ -0,0 +1,11 @@ | |||
| import {isRecord} from '@libs/ObjectUtils'; | |||
|
|
|||
| function getStringParam(params: unknown, key: string): string | undefined { | |||
There was a problem hiding this comment.
nab can we move this to ObjectUtils, this method isnt related to the tab bar
|
Going to go ahead to merge to get this in |
|
🚧 JS00001 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/JS00001 in version: 9.4.45-0 🚀
|
|
Reviewed the changes in this PR against the help site articles under Why: This is a pure internal performance optimization. It changes how the Inbox tab restores the last-opened report — focusing the already-mounted All changed files are navigation internals and their unit tests:
The help site documents user-facing product behavior, and none of that behavior changes here, so there is nothing to update. @BartekObudzinski, since no user-facing behavior changed, there is no linked help site PR to review here. If you believe a docs update is warranted, let me know and I'll create one. |
|
Deploy Blocker #97154 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.45-14 🚀
Bundle Size Analysis (Sentry): |
11 participants
Explanation of Change
On wide layout, tapping the Inbox tab navigates to the last opened report. The current URL-based navigation rebuilds the nested report route, so React remounts the central
ReportScreenand its heavy report actions list in the same commit as the LHN. TheManualNavigateToInboxTabspan ends when the LHN lays out, but that layout cannot paint until the report list commit finishes.This PR follows the cached navigation architecture used by the Search and Spend tab:
TabActions.jumpToaction. Omitting nested params preserves the mountedReportScreen, report list, route key, report params, and URL.ReportActionsbehindNavigationDeferredMount, allowing the LHN to paint before the heavy list hydrates.ReportScreenslot. After the first hydration,react-freezepreserves that state. Subsequent Home to Inbox navigations reuse the same report list and do not show the skeleton again.The deferral remains scoped to an in-flight
ManualNavigateToInboxTabspan. Opening a report from the LHN, a deep link, or Search does not start that span and renders the report immediately.Benchmark results
Wide web with warm cached data.
First warm Inbox mount, 8 measured runs:
Subsequent cached Inbox visits, 12 measured runs:
The skeleton appears only during the first mount. Subsequent Home to Inbox navigations preserve the same
ReportScreenand report list with no skeleton.Fixed Issues
$ #96398
PROPOSAL:
Tests
/r/:reportID.Offline tests
QA Steps
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
Screen.Recording.2026-07-21.at.11.42.58.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-07-21.at.11.35.17.mov