Repository navigation
Conversation
Menu item children (9 components with ~46 Onyx subscriptions total) were always mounted even when the popover was closed. Now they mount only when the menu opens, eliminating idle subscription cost on the main screen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep children mounted during the close animation by deferring unmount to onModalHide instead of gating on isVisible directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
leshniak
changed the title
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb Chromemchrome.moviOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
![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: c851447b7f
ℹ️ 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".
|
Nice idea — using The issue: if (prevProps.isVisible === nextProps.isVisible && nextProps.isVisible === false) {
return true; // skip re-render
}When the popover closes:
So after the first open, Activity is stuck at Suggested fix: Derive mode directly from - const [contentActivityMode, setContentActivityMode] = useState<ActivityProps['mode']>(isVisible ? 'visible' : 'hidden');
...
- onModalWillShow={() => setContentActivityMode('visible')}
- onModalHide={() => setContentActivityMode('hidden')}
...
- <Activity mode={contentActivityMode}>
+ <Activity mode={isVisible ? 'visible' : 'hidden'}>This way Everything else looks good — correct |
|
No product review needed |
| <View style={shouldUseNarrowLayout ? styles.flexGrow1 : [styles.createMenuContainer, styles.pv0, styles.flex1]}> | ||
| <View style={styles.pv4}>{children}</View> | ||
| </View> | ||
| <Activity mode={contentActivityMode}> |
There was a problem hiding this comment.
We're not using isVisible ? 'visible' : 'hidden' directly here because of modal animation, right?
There was a problem hiding this comment.
We're not using
isVisible ? 'visible' : 'hidden'directly here because of modal animation, right?
correct.
|
Please complete author checklist |
|
@leshniak I think you are missing some checks, can you use the most up to date template please? https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md |
@rlinoz Updated |
|
🚧 @rlinoz 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/rlinoz in version: 9.3.59-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes required. This PR is a purely internal performance optimization — it wraps the FAB popover content in React 19's |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.3.59-4 🚀
|
Explanation of Change
The FAB popover menu tree contains 9 menu item components with 56
useOnyxcalls (~95 active subscribers at runtime). These subscribers fire on every Pusher event even when the popover is closed, which is the vast majority of the user's session.This PR wraps the popover content in React 19's
<Activity>component:mode="hidden"when the popover is closed — React deprioritizes updates, so menu item subscribers stop processing Onyx changesmode="visible"when the popover opens — React flushes any pending state before painting, so items always show current dataThe mode transitions are tied to
onModalWillShow/onModalHidecallbacks so the content stays visible during the fade-out animation.Measured results (perf-test, 500 reports):
Fixed Issues
$ #87158
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests above.
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
xd.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
xd.mp4