Repository navigation
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.
|
|
@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: 44d2d8f387
ℹ️ 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".
|
@ShridharGoel The |
|
@DylanDylann I agree with your approach. Thanks for suggesting it! |
Callers only consume reportID, policyID, and chatType, so the function now returns a minimal LastAccessedReport slice instead of the full Report. Extracts a pure findLastAccessedReportSelector that takes the collections as arguments; the module-scoped findLastAccessedReport wrapper keeps feeding it the intentional module subscriptions (non-UI, no view-based re-renders). navigateAfterOnboarding drops isConciergeChatReport/isSelfDM calls and compares slice fields directly. Refs Expensify#98719 (comment)
Resolve conflicts in DeepLinkHandler.tsx and ReportUtils.ts: - DeepLinkHandler: keep PR's reportNameValuePairs loading guard + arg, adopt main's guidedSetupAndTourStatus refactor (replaces standalone isSelfTourViewed). - ReportUtils: keep the pure findLastAccessedReportSelector + slice return type from this PR, and fold main's new `reports` param into the module-scoped findLastAccessedReport wrapper so callers like ReportRouteParamHandler can pass a subscribed reports collection.
Drop verbose rationale and fix the stale "only takes the name-value pairs" line, since the wrapper now also accepts a `reports` argument.
findLastAccessedReportSelector and FindLastAccessedReportOptions have no external consumers; tests exercise the public wrapper. Keep both module-private (selector still backs the wrapper, type still types its options param).
|
@MelvinBot review |
ReviewThe refactor is correct and the primary path is verified working (see smoke test below). Two things to resolve before this is mergeable — one design, one scope: 1. This still passes large collections — the opposite of what was agreed
2. The
|
|
@MelvinBot review |
Re-reviewThanks for the updates — the main design point from my last review is addressed: you introduced Two things left to sort out before merge: 1.
|
…ast-accessed-report-nvp
|
@DylanDylann Any updates on the final review? |
| }; | ||
|
|
||
| /** Resolves the last accessed report for navigation fallbacks from a view-scoped subscription. */ | ||
| function useFindLastAccessedReport({openOnAdminRoom, excludeReportID, enabled = true}: UseFindLastAccessedReportOptions = {}) { |
There was a problem hiding this comment.
| function useFindLastAccessedReport({openOnAdminRoom, excludeReportID, enabled = true}: UseFindLastAccessedReportOptions = {}) { | |
| function useFindLastAccessedReport({openOnAdminRoom, enabled = true}: UseFindLastAccessedReportOptions = {}) { |
| const shouldResolveReportID = !routeReportID && !reportIdFromPath && !isTransitioning; | ||
|
|
||
| const {lastAccessedReportID} = useFindLastAccessedReport({ | ||
| openOnAdminRoom: isOpenOnAdminRoom, |
There was a problem hiding this comment.
| openOnAdminRoom: isOpenOnAdminRoom, |
|
@ShridharGoel last NITs |
|
@DylanDylann Updated |
|
Merge conflicts 😢. @ShridharGoel can you fix those, and while you're at it can you change the comments to use |
…ccessed-report-nvp # Conflicts: # src/libs/Navigation/AppNavigator/Navigators/ReportsSplitNavigator.tsx # src/pages/DynamicReportDetailsPage.tsx
|
🚧 blimpich 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/blimpich in version: 9.4.92-0 🚀
|
|
🤖 No help site changes are required for this PR, so I did not create a draft docs PR. This is an internal refactor with no user-facing behavior change. It threads the Why nothing in
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.92-8 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Pass report name-value pairs to every
findLastAccessedReport()caller. This lets deep-link, room-leave, group-chat-leave, and task-delete fallback navigation exclude archived reports without reading the collection insideReportUtils.Fixed Issues
$ #66422
PROPOSAL:
Tests
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-16.at.12.17.54.PM.mov
Screen.Recording.2026-08-16.at.12.07.46.PM.mov