Repository navigation
Conversation
|
@codex review |
|
@MelvinBot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ReviewThe logic looks correct. I found no blocking issues. Before merge, you need to fill in the Tests, Offline tests and QA sections; QA still has the I checked whether anything still reads the fields that
All required CI checks pass. Only the checklist and independent-approval checks fail. Suggestions (non-blocking)
Notes on the fallback in useReportPreviewSenderID
Only previews with a pending scan subscribe on every message ( Suggested test steps for the empty sections:
|
|
Codex Review: Didn't find any major issues. Hooray! 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". |
dylanexpensify
left a comment
There was a problem hiding this comment.
Performance-related, no product input required.
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp103976-android-hybrid-001.mp4Android: mWeb Chrome103976-mweb-chrome-001.mp4iOS: HybridApp103976-ios-hybrid-001.mp4iOS: mWeb Safari103976-mweb-safari-001.mp4MacOS: Chrome / Safari103976-web-chrome-002.mp4 |
rojiphil
left a comment
There was a problem hiding this comment.
@cristipaval @Julesssss Changes LGTM.
Over to you for internal review. Thanks.
|
🚧 Julesssss 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.5.15-1 🚀
|
|
No help site update is needed: this PR is a performance change that stops chat avatars from re-rendering on every sent message, with no change to what users see or do. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
useReportActionAvatarsnow reads the chat report through the existinggetStableReportSelector, which drops the 10last*and read-state fields. An audit of every path that receives the chat report found only one read of a dropped field:lastActorAccountID, used by the pending scan sender lookup on report previews.useReportPreviewSenderIDnow subscribes tolastActorAccountIDon its own, and only while a transaction has a pending scan with an unknown amount direction. The puregetReportPreviewSenderIDtakes it as the newchatReportLastActorAccountIDparam instead of reading it offchatReport. It falls back tochatReport.lastActorAccountIDwhen the report is not in Onyx (for example a Search snapshot).Avg iOS ManualSendMessageVisible, scenario with all non-grouped items (worst): 646.8 ms -> 542.0 ms −16.2%
Fixed Issues
$ #103609
PROPOSAL:
Tests
Test 1: Smoke, message avatars in chats
Test 2: Avatars still update on real report changes
Prerequisite: A is an admin of the workspace.
Test 3: Report preview sender while a receipt is scanning
Prerequisite: account A has a DM with account B.
Offline tests
QA Steps
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.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios.web.mov
MacOS: Chrome / Safari
web.mov