refactor: remove personalDetails from ReportActionsList dependencies#91416
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ 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". |
…-personal-details-v2
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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". |
|
@Eskalifer1 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] |
Eskalifer1
left a comment
There was a problem hiding this comment.
What do you think about removing personalDetailsList from here:
We only use this value in one place:
And the shouldShowReportRecipientLocalTime value is used in only one place:
To add the paddingBottom: 16px style.
It doesn't seem very practical to load an entire data set—which could be enormous—just to determine whether or not to indent the text
I think we can create a new component where we move the shouldShowReportRecipientLocalTime logic, thereby completely removing personalDetailsList from ReportActionsList, which will give us a performance boost. And within this component, we’ll retrieve personalDetailsList, and we’ll be able to pass hideComposer and reportActionsListFSClass to it as props
|
Please add indentation before video links so that GitHub can automatically detect them and display the videos instead of just the links |
|
Also, if you have any steps or, for example, a value that can be set in Onyx to trigger a Report action for the fourth text, I think it’s worth adding that to the tests, since QA will definitely ask how to trigger that message |
|
And let's merge main! I will complete checklist tomorrow |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp91609-android-native.movAndroid: mWeb Chrome91609-android-web.moviOS: HybridApp91609-ios-native-1.mov91609-ios-native-2.moviOS: mWeb Safari91609-ios-web.movMacOS: Chrome / Safari91609-web.mov |
|
Hi @LukasMod Seems like we have some eslint and ts problems after merging main |
|
@codex review |
|
Looking into that now |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ 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". |
|
@Eskalifer1 all green now 🎉 |
…-personal-details-v2
|
New conflicts resolved |
|
🚧 @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! 🧪🧪
|
|
✋ 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/mountiny in version: 9.3.90-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site articles under Why: This is a pure performance/architecture refactor — it stops Evidence
Help site articles document user-facing product behavior, features, and copy. Since none of those changed here, there is nothing to update. Since no documentation changes are required, I did not create a draft help site PR. If you believe a specific behavior described in the docs has actually changed, reply with |
|
This PR failing because of the issue #92225 Bug7168482_1780314053297.Chat_Tests.mp4 |
|
@jponikarchuk Is that only in staging and stemming from this PR? |
|
@mountiny Issue says it's repro in prod as well. |
|
I don't think this applies to our PR, since the bug occurs on both stage and prod, which means it's not due to our changes. Perhaps QA didn't quite understand the last test correctly; specifically, they expect this text to always be there (which isn't the case), which is why the issue was created. I just logged in and checked myself, and indeed, this text isn’t displayed in all chats, which is to be expected, and the same behavior is observed on prod. In those chats where this text is present on prod, it’s also displayed on stage... |
|
Cool, I missed that, thanks |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.90-3 🚀
|

Explanation of Change
This PR stops the report-actions list from re-rendering when unrelated users' personal details change. Previously, the full
PERSONAL_DETAILS_LISTcollection was subscribed at the list level and passed down as a prop; any profile update invalidated memoization for every visible row.The fix has two parts:
personalDetailsprop-drilling. Each row (or small subtree) subscribes only to the account IDs it needs viauseOnyx+ selectors.PERSONAL_DETAILS_LISTusage out ofReportActionsListinto a thin wrapper and shared hook so the heavy list no longer subscribes to the full collection.Problem
ReportActionsListsubscribed to the fullPERSONAL_DETAILS_LISTOnyx key (viausePersonalDetails()) and passedpersonalDetailsthroughReportActionsListItemRenderer→ReportActionItem→PureReportActionItem(and related children). It also used that collection to decide composer/list padding for recipient local time.Any update to any user's profile caused the entire report-actions list and its memoized rows to re-render.
Solution
Stop prop-drilling the full collection
personalDetailswas removed from props across the report-action tree (ReportActionsListItemRenderer,ReportActionItem,ActionContentRouter,MoneyRequestReportActionsList, SearchChatListItem, debug pages, etc.).Each component that needs a name now reads only the specific account it cares about via
useOnyxwith a selector:PureReportActionItempersonalDetailsDisplayNameSelector(actorAccountID)for message accessibility labelsReimbursementQueuedContentpersonalDetailsDisplayNameSelector(ownerAccountID)for submitter copy in system messagesAncestorReportActionItempersonalDetailByAccountIDSelector(report.ownerAccountID)for thread ancestor navigationOnly rows whose actor (or owner, for reimbursement messages) actually changed will re-render.
Isolate recipient local time from
ReportActionsListThe list still needed personal-details logic for bottom padding (
pb4) when the recipient local-time row is not shown, but that must not live onReportActionsListitself.ReportActionsListPaddingView(new) — wrapsInvertedFlashList. Owns list padding, FullstoryfsClass, and composer visibility (canUserPerformWriteAction+useShouldShowComposerForActiveEditDraft). Subscribes to local-time visibility via the hook below instead of pulling the full collection into the list.useReportRecipientLocalTime(new) — shared hook that subscribes toPERSONAL_DETAILS_LISTwithcanShowReportRecipientLocalTimeSelector(report, currentUserAccountID)and returns a boolean (not the full list), so Onyx only re-renders subscribers when eligibility changes.canShowReportRecipientLocalTimeSelector(in@selectors/Report) — curriescanShowReportRecipientLocalTimefromReportUtilsfor use withuseOnyx.ComposerLocalTime— usesuseReportRecipientLocalTimefor eligibility; loads the recipient record withpersonalDetailByAccountIDSelectoronly whenshouldShowis true (canShow && !isComposerFullSize). Avoids a second full-list subscription for display data when the row is hidden.ReportActionsListno longer callsusePersonalDetails()orcanShowReportRecipientLocalTimedirectly. The deadisComposerFullSizeprop on the list was removed (it was never passed fromReportActionsView).User-visible impact
None expected. Chat layout, local time, and list padding should behave the same as before.
Note: List padding does not account for expanded composer (
isComposerFullSize); that matches prior behavior because the list never received that prop. OnlyComposerLocalTimehides the row when the composer is full size.Fixed Issues
$ #91609
PROPOSAL:
Tests
1. Chat — report actions list
2. Accessibility — message labels (web)
aria-label/accessibilityLabel).{sender}, {timestamp}, {message}; sender not empty for known users, no console errors.3. Threads — ancestors & navigation
4. Reimbursement queued
Setup (payer account A, recipient account B):
Test:
5. Expense report — actions list
6. Search — chat row preview
7 Composer, recipient local time
Pass: 1:1 shows "It's {time} for {name}" above the composer; group has no local-time row; expand hides it, collapse brings it back; spacing above composer looks normal.
Offline tests
QA Steps
Same as tests
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Test 1:
scroll.web.1.mov
Test 2:
aria.label.web.2.mov
Test3:
threads.-.3.mov
Test4:
Test5:
expenses.5.mov
Test 6:
search.mov
Test 7:
Screen.Recording.2026-05-27.at.13.40.58.mov