perf: stop WorkspacesListPage re-renders triggered by policy isLoading merges#91316
perf: stop WorkspacesListPage re-renders triggered by policy isLoading merges#91316WojtekBoman wants to merge 3 commits into
Conversation
![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: ff884e7611
ℹ️ 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".
| brickRoadIndicator={item.brickRoadIndicator} | ||
| shouldDisableThreeDotsMenu={item.disabled} | ||
| style={[item.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ? styles.offlineFeedbackDeleted : {}]} | ||
| style={item.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ? styles.offlineFeedbackDeleted : undefined} |
There was a problem hiding this comment.
Passing styles.offlineFeedbackDeleted directly here changes style from an array to an object, but WorkspacesListRow derives isDeleted using Array.isArray(style) && style.includes(styles.offlineFeedbackDeleted). In the pending-delete case this now evaluates false, so the row’s text no longer gets the deleted strike-through treatment even though the workspace is pending deletion. This regresses the deleted-row visual state in Workspaces list flows.
Useful? React with 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
melvin-bot
Bot
requested review from
abzokhattab and
joekaufmanexpensify
and removed request for
a team
|
@abzokhattab 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] |
|
@codex review the PR |
|
@MelvinBot review the PR |
PR ReviewOverall: Solid perf improvement. The selector approach is well-proven in this codebase (mirrors Issues
Looks Good
|
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ 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". |
| // (e.g. openPolicyWorkflowsPage's optimistic isLoading=true/false). Consumers that render the | ||
| // policy collection but don't care about those flags can pass this as a selector so useOnyx's | ||
| // deepEqual check elides re-renders while another screen flips the flags. | ||
| const policyCollectionWithoutLoadingFlagsSelector = (policies: OnyxCollection<Policy>): OnyxCollection<Policy> => { |
There was a problem hiding this comment.
Can hou add a test for this?
|
|
||
| function usePoliciesWithCardFeedErrors() { | ||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); | ||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {selector: policyCollectionWithoutLoadingFlagsSelector}); |
There was a problem hiding this comment.
uhh I think this will be a big perf regression for a customer with 5k policies, as the entire object will go through deepEqual inside useOnyx 😬. Can we verify it?
There was a problem hiding this comment.
I'll check it out 🕵️
There was a problem hiding this comment.
Hi, I checked how the performance behaves with 5k workspaces in the workspace list. To test this, I used the Aplause heavy account and multiplied their workspaces by 50x.
In the scenario with the selector enabled, the multiplication was done inside the selector itself. In the scenario without the selector, the multiplication was applied directly to the result returned from useOnyx, and then I measured rendering only.
I ran several profiler measurements, and the screenshot above shows the average results. From what I can see, we’re not really dealing with a performance issue here even with 5k workspaces, the difference is only a few milliseconds. In my opinion, with such a large dataset, this kind of variance is likely within the margin of measurement error.
It’s also worth noting that the selector helps reduce unnecessary re-renders caused by changes in loading fields, not only in the specific scenario related to this issue.
What do you think?
There was a problem hiding this comment.
I’ll also prepare tests using the file from you, but it’s possible the results will be similar.
There was a problem hiding this comment.
I did a measurement on iOS and for 5k policies I got ~650ms spent on deepEqual when I manually sent a merge to policies compared to ~1ms on main, here are the steps I followed
Screen.Recording.2026-05-26.at.09.52.56.mov
And the output:
Trace-20260526T095323.json
There was a problem hiding this comment.
So multiplying in the selector is probably not the best solution after all 😅 . In that case, I’ll try to come up with a better solution/workaround for this problem.
|
I believe the goal was also to undo the freezewrapper #90970 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Is it not ready for review? should i halt? |
|
No product review needed |
WojtekBoman
force-pushed
the
perf/fix-workspaces-list-rerenders
branch
from
b509f56 to
b5605f0
Compare
|
Let me know when its ready for a review :D |
7 participants
Explanation of Change
Adds a
policyCollectionWithoutLoadingFlagsSelectorthat strips transientisLoading*flags from the policy collection, and applies it toWorkspacesListPageandusePoliciesWithCardFeedErrors. Also replaces an unstable inlinestylearray literal with a stableundefinedso the shallow compare on the prop can hold.Why this selector is safe?
The three flags this selector strips (isLoading, isLoadingReceiptPartners, isLoadingWorkspaceReimbursement) are only ever read off a single policy entry — every consumer subscribes via useOnyx(${ONYXKEYS.COLLECTION.POLICY}${policyID}) (directly or through usePolicy(policyID)), never off the policy collection. Since Onyx single-entry subscriptions are independent from collection subscriptions and the selector only filters what it returns (it doesn't mutate stored data), the two collection consumers using this selector (WorkspacesListPage and usePoliciesWithCardFeedErrors, neither of which reads these flags) stop re-rendering on loading-flag flips, while every spinner that depends on those flags keeps working unchanged.
Main:
Screen.Recording.2026-05-22.at.10.10.53.mov
The optimized version:
Important
Rows still re-render when the page re-renders, because menuItems and onPress are built inline per row and defeat React.memo; this is orthogonal to the loading-flag churn fixed here and should be addressed in a follow-up perf PR.
Screen.Recording.2026-05-22.at.10.08.33.mov
Fixed Issues
$ #91175
PROPOSAL:
Tests
Sign in to an account with several workspaces (a High Traffic account gives the clearest signal).
Open Settings → Workspaces and wait for the list to settle.
Open the React DevTools Profiler (web) and start recording.
Tap any workspace to navigate into it, then open a tab that calls
openPolicyWorkflowsPage(e.g. Workflows).Stop recording without tapping back.
Verify that
WorkspacesListPagedoes not commit while the workspace screen is foregrounded, apart from the expected singleisFocusedblur transition. Before this PR, the page would re-render twice per optimisticisLoadingflip on the policy entry (and the cascade re-rendered every row).Sign in to an account with several workspaces.
Open Settings → Workspaces, scroll partway down, and tap one workspace to open it.
Navigate through a few workspace tabs (Workflows, Members, etc.) and then tap back to the workspaces list.
Verify that scroll position is preserved, the row that was tapped is not stuck in a hovered/pressed state, and the list renders normally.
Sign in to an account that both owns at least one workspace and is a member of another workspace it doesn't own.
Open Settings → Workspaces and tap the three-dots menu on each kind of workspace row.
Verify the expected entries appear based on role (Go to workspace / Leave / Duplicate / Set as default / Delete / Transfer owner) and that selecting each performs its action without errors.
Offline tests
N/A — this change only affects which fields on the policy collection trigger re-renders. It does not add API calls and does not change offline persistence behaviour.
QA Steps
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