Repository navigation
Conversation
…confirmation mount
![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: 88a6b3aa0d
ℹ️ 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".
|
No product review needed |
|
Hey @FitseTLT, can you take a look at and review if you have a chance? Thanks! |
|
Thx REviewing |
| let participantsKeyByReportID: Record<string, string> = {}; | ||
|
|
||
| /** | ||
| * In index order, so deleting the winner promotes the next one instead of dropping the key. Optimistic chat creation | ||
| * puts two reports on the same participants until the server report replaces the optimistic one, and the confirmation | ||
| * page has to keep resolving a chat throughout. | ||
| */ | ||
| let reportIDsByParticipantsKey: Record<string, string[]> = {}; |
There was a problem hiding this comment.
BTW the reason we are having this moduled scoped variables is for the case where multiple reports can exist with identical key but I checked the case of creation of a new optimistic dm chat or other dm cases but it doesn't happen. But having them causes the rederivation on every reload. Is it worth it? Even if we want to consider the case we can save the derived as reportid array instead.
There was a problem hiding this comment.
Removed the module scoped variables and changed to refilling the key from reports when the indexed report goes away. However, it still rebuilds once on startup, because the persisted index can be stale against reports.
There was a problem hiding this comment.
I really feel like the refill case is irrelevant in any real case but it instead runs when you clear cache for me for some unknown report (with the key change of some key -> undefined key) and when I open the report it shows me not found. Which means the BE is sending some wrong data and clearing it.
From what i understand what would have been a useful circumstance for the refill is if there were two reports with same key and when one is removed we want to recalculate the key with the other report's id because this report might not trigger a recalculation. But for the preexiting report id case the optimsitic first exists then in any order optimistic clearance and BE report writing happens which would trigger a calculation that early returns b/c the optimsitic report is cleared and writing the new BE report id on the key respectively.
But if you still feel safe with it, I am ok.
There was a problem hiding this comment.
It only scans reports for another report with the same key and never writes or clears report data. In your clear cache case it just found nothing and left the key empty, which is correct since that report is gone. The not found is the BE data, like you said.
For the preexisting case, I agree it's fine when the optimistic report is cleared before (or in the same update as) the BE report is written. The order that breaks is the other one: replaceOptimisticReportWithActualReport clears the optimistic report and writes the preexisting one inside runAfterTransitions, so it's deferred. If the real report reaches onyx before that runs, its compute skips it because the key still points to the optimistic report. Then the optimistic report is cleared, the key is deleted, and the real report never triggers another compute, so the key stays empty while the report exists. The next confirmation mount then misses the DM and creates another optimistic one. The refill covers exactly that case, and it only costs one scan when an indexed report loses its key, so I'd prefer to keep it.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| // never show up in a delta, so it is rebuilt rather than trusted. | ||
| // A SESSION change can carry a different account (delegate switching writes it before clearing Onyx), and | ||
| // the index doesn't record which account it was built for. | ||
| if (!currentValue || !sourceValues || hasKeyTriggeredCompute(ONYXKEYS.SESSION, triggeredKeys)) { |
There was a problem hiding this comment.
1.So this will make whole rebuild happen on any prop change on session. We only want if currentUserAccountID changed. So can't we save a unique prop in the derived object to identify to what accountID the derived is calculated for. We will only recalculate when they mismatch.
2. And shouldn't we check for isEmptyObject for currentValue.
or a single check of is it calculated on the currentUserAccountID could suffice.
There was a problem hiding this comment.
- Changed.
- I don't think we need a separate
isEmptyObjectcheck. When there's no accountID or no reports yet we return{reportIDs: {}}without an accountID, so the next compute sees the mismatch and rebuilds. An empty check would also add an extra rebuild on every report update for users with no DMs.
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-09-29.16-41-32.mp4iOS: HybridAppiOS: mWeb Safari2026-09-29.16-40-12.mp4MacOS: Chrome / Safari2026-09-29.16-32-34.mp4 |
|
@OlimpiaZurek Regarding
There is a bug in current branch as it doesn't populate the DM chat properly from a deeplink. In fact if it is immediately after clearing cache it returns not found page. 2026-09-28.19-01-16.mp4 |
@FitseTLT could you share the exact repro steps? From the video it looks like the link was opened in a different env than the one where the flow was started. The selected recipient isn't passed through the URL, which only contains the fixed draft transaction ID ( I see the same behavior on Within the same environment it works as expected for me. |
|
Sorry 🙏 @OlimpiaZurek Spot on! |
How are they going to confirm page directly? |
This point was a bit misleading, updated it in the PR description |
cristipaval
left a comment
cristipaval
left a comment
There was a problem hiding this comment.
Thanks for working on this!
|
🚧 Julesssss 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/Julesssss in version: 9.5.2-0 🚀
|
|
No help site update is needed. This PR is an internal performance change: it finds an existing 1:1 chat with a lookup instead of scanning every report, and expense creation behaves the same for users. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
IOURequestStepConfirmationresolved the existing P2P chat with aCOLLECTION.REPORTselector runninggetChatByParticipants. That helper isObject.values(reports).find(...)with per-report work (anObject.keyson participants, two report-type predicates, a sort and a deep compare), so answering "which chat do I share with this person" walked the entire report collection on the confirmation page's mount path.This adds a
ONE_ON_ONE_CHAT_REPORT_IDSderived value that indexes 1:1 and system chats by participant set, so the page resolves the chat with a lookup instead. Reports are keyed by reportID, so a participant-set lookup can't be answered without walking them, this re-keys the collection rather than duplicating a key it already has.Fixed Issues
$ #101401
PROPOSAL:
Tests
+, chooseCreate expense, enter an amount, and pick that user as the recipient.Offline 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 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari