Open
Conversation
Replace 7 refs + 8 useState calls with a single PopoverContextMenuState object. Keep separate isPopoverVisible boolean for animation control: hideContextMenu sets isPopoverVisible=false (triggering animation), onModalHide clears menuState=null (clearing data after animation). Consolidate popoverAnchorPosition + contextMenuDimensions into PopoverPosition within the state object. Eliminate reportActionRef entirely (latent staleness bug -- only set in showDeleteModal, never in showContextMenu). Store only reportActionID in consolidated state. Derive full action from reportActions[reportActionID]. Move onEmojiPickerToggle from ref to state to avoid accessing refs during render. Remove all useCallback wrappers and inline the logic. Made-with: Cursor
…nContextMenu
Remove memo(deepEqual) wrapper and all useMemo calls (5 total).
React Compiler handles memoization automatically. Replace inline
{current: null} with stable nullRef to avoid new object creation
per render. Inline all computed values directly.
Made-with: Cursor
Extract delete confirmation flow from PopoverReportActionContextMenu into a standalone ConfirmDeleteReportActionModal component that mounts via the established global modal system (useModal/ModalProvider). The new component owns all ~16 delete-related Onyx subscriptions, which are only active when the delete modal is actually shown. This eliminates 5 duplicate subscriptions with BaseReportActionContextMenu and defers the remaining ~11 until actually needed. The promise-based API replaces 3 callback refs + 2 state vars from PopoverReportActionContextMenu. The shouldSetModalVisibility parameter is dropped as the global modal system manages visibility independently. Made-with: Cursor
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Create MiniContextMenuProvider with split contexts (Actions/State) to avoid unnecessary re-renders in list items. The provider manages show/hide with 120ms delay, shouldKeepOpen/pendingHide for emoji picker flow, and stable action references via useState lazy init. Rewrite MiniReportActionContextMenu as an animated singleton rendered via createPortal to document.body for reliable position:fixed. Uses Reanimated shared values for animated row-to-row slides with overshoot easing, and CSS transitions for opacity fade. PureReportActionItem now measures its row via getBoundingClientRect on hover and calls showMiniContextMenu instead of rendering its own MiniReportActionContextMenu instance. This eliminates ~1100 Onyx subscriptions (24 per visible item). Made-with: Cursor
In mini mode, BaseReportActionContextMenu now uses the provider's keepOpen()/release() API for emoji picker and overflow menu flows. Non-mini (popover) mode retains local state. This ensures the singleton stays visible when submenus are open. Made-with: Cursor
Add a scroll event listener (capture phase) to dismiss the mini context menu when the list scrolls. The menu reappears at the correct position on the next hover via fresh getBoundingClientRect. Made-with: Cursor
Add ContextMenuPayloadProvider (shared context for all action components), ContextMenuLayout (visibility evaluation, mini-mode truncation, arrow key focus), and actionConfig (shouldShow registry with ordered action IDs). Foundation for converting the config array into individual dot-notation components. Made-with: Cursor
Replace the config array .filter().map() loop in BaseReportActionContextMenu with ContextMenuPayloadProvider, ContextMenuLayout, and individual dot-notation action components. Convert disabledActions (ContextMenuAction[]) to disabledActionIds (Set<string>) throughout the chain: PopoverReportActionContextMenu, MiniContextMenuProvider, ReportActionContextMenu, PureReportActionItem. Use Reanimated .get()/.set() API for shared values. Fix import alias in actionConfig. Made-with: Cursor
Organize component internals: hooks, derived values, callbacks, effects, render. Fix deprecated getReportNameDeprecated usage (add eslint-disable), fix no-default-id-values errors in Debug, Delete, and Explain. Use Reanimated .get()/.set() API. Fix import aliases for @pages prefix. Clean up unused imports. Reduce lint warning budget from 383 to 353 (30 warnings eliminated). Made-with: Cursor
|
npm has a |
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Wire hideDeleteModal to call modalContext.closeModal() so the delete confirmation modal is dismissed when a report action item unmounts while the modal is open (e.g., navigating away). Made-with: Cursor
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
When the delete target is a money request, the effectiveReportID can be the IOU report ID, but the action lives in the chat report's REPORT_ACTIONS collection. Pass actionSourceReportID to the modal and fall back to it when the action isn't found in the primary collection. Made-with: Cursor
The positioning useEffect had no dependency array, causing it to run after every render. Scope it to state changes so animations only trigger when row measurements or visibility actually change. Made-with: Cursor
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
In mini-menu flows the popover menuState is unset, so deriving the source report from menuState?.reportID yields undefined. Pass the source report ID explicitly from Delete.tsx through the showDeleteModal interface so the modal can always resolve the action. Also guard hideDeleteModal with a ref so it only closes the modal when showDeleteModal actually opened one, preventing unrelated modals from being dismissed during navigation/unmount cleanup. Made-with: Cursor
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The scroll handler repeatedly called hideMiniContextMenu() which uses
a 120ms debounced timer, causing the menu to stay visible while
scrolling. Add an {immediate: true} option to bypass the timer for
scroll-triggered hides.
Made-with: Cursor
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The old code passed filteredContextMenuActions as disabledOptions when opening the overflow popover, hiding actions already visible in the mini row. Restore this behavior by passing the current visibleActionIds from ContextMenuLayout as disabledActionIds to the overflow menu. Made-with: Cursor
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 946ac3715d
ℹ️ 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".
src/pages/inbox/report/ContextMenu/PopoverContextMenu/PopoverReportContent.tsx
Outdated
Show resolved
Hide resolved
Contributor
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
Contributor
|
@roryabraham could you please check the Codex comments and fix the failing lint check? |
Made-with: Cursor # Conflicts: # src/pages/inbox/report/ContextMenu/ContextMenuActions.tsx
…eportAction OptionRowLHN passes reportActionID: '-1' for report-level context menus, resolving reportAction to undefined. The guard `&& reportAction` prevented the Mark as Unread item from rendering in report menus. markCommentAsUnread already handles undefined reportAction via optional chaining, so widen the type to match. Made-with: Cursor
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f50d465d37
ℹ️ 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".
src/pages/inbox/report/ContextMenu/actions/DeleteAction/PopoverDeleteItem.tsx
Outdated
Show resolved
Hide resolved
src/pages/inbox/report/ContextMenu/PopoverContextMenu/PopoverReportContent.tsx
Outdated
Show resolved
Hide resolved
1. Wrap delete and flag-as-offensive actions with interceptAnonymousUser so anonymous users are redirected to sign-in instead of executing destructive/reporting actions (P1). 2. Use shouldDisplayContextMenuValue (which excludes Concierge greeting actions) instead of shouldDisplayContextMenu in the hover-in guard of PureReportActionItem, matching the right-click/long-press path (P2). 3. Pass the real currentUserAccountID to PopoverMarkAsUnreadItem in PopoverReportContent instead of hardcoded 0, so markCommentAsUnread correctly excludes the caller's own actions when anchoring (P2). Made-with: Cursor
… markCommentAsUnread Use ?? instead of || for reportAction?.created (prefer-nullish-coalescing). Make reportActionID optional in MarkAsUnreadParams since report-level mark-as-unread doesn't target a specific action. Made-with: Cursor
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Contributor
Screen.Recording.2026-04-09.at.12.33.55.movBUG: Steps to reproduce:
@roryabraham could you check this bug? |
Contributor
Screen.Recording.2026-04-09.at.12.47.30.movBUG: The reaction bar still persists in the thread when replying to a message. |
Contributor
|
@roryabraham Currently, do we have any tools to test performance? If not, I think I will create a new one. |
Contributor
Screen.Recording.2026-04-09.at.13.06.17.movScreen.Recording.2026-04-09.at.13.24.25.movBUG: I noticed that the reaction bar does not appear the first time I hover over a message. It shows normally on the second hover. I was unable to reproduce this issue on staging. |
Contributor
Screen.Recording.2026-04-09.at.13.10.15.movQUESTION: Menu options look different between this branch and staging. Did we change them? |
Contributor
Screen.Recording.2026-04-09.at.13.19.26.movBUG: Accessibility issue. When pressing Tab, I can't focus on the reaction bar. On staging, this works correctly. |
Contributor
Screen.Recording.2026-04-09.at.13.21.46.movBUG: The reaction bar is still hidden when the emoji picker is shown. |
Contributor
Screen.Recording.2026-04-09.at.13.25.54.movBUG: The reaction bar is not hidden when the message loses focus. |
Contributor
Screen.Recording.2026-04-09.at.13.27.22.movBUG: The highlighted report message does not clear when it loses focus. |
Contributor
Screen.Recording.2026-04-09.at.13.29.03.movBUG: When swiping left or right on a report message, the reaction bar becomes hidden. This behavior is different from staging. |
Contributor
Screen.Recording.2026-04-09.at.13.31.32.movBUG: The message remains highlighted when it loses focus. |
Contributor
|
@roryabraham After testing, I found several bugs related to message highlighting, incorrect reaction bar visibility, and UI menu options. Could you please review them? |
Contributor
|
I think we should fix these issues first. After that, I will test edge cases like deep linking and offline or slow network scenarios to avoid the same root cause. |
Contributor
Author
I was using a mix of React Profiler and Claude |
Contributor
Author
|
thanks for finding and reporting all those bugs! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Performance overhaul of the ContextMenu system. Converts per-row
MiniReportActionContextMenuinstances into a singleton rendered via@gorhom/portal, eliminating Onyx subscriptions and reducing the subtree size per visible report action.Architecture changes
ContextMenuActions.tsxwith individual files underactions/, each self-contained with its ownshouldShowpredicate.useRef+ 8useStatecalls inPopoverReportActionContextMenuwith a singlePopoverContextMenuStateobjectConfirmDeleteReportActionModalvia global modal system, so its ~16 Onyx subscriptions are only active when shownmemo/useMemo/useCallbackwrappersPerformance results
Compared via React Profiler traces on the same account with similar interaction patterns (~9K
PureReportActionItemrenders each, ~5s sessions):Overall:
Context menu components:
Non-context-menu render time is mostly flat (4,777ms → 4,823ms).
For context, the original profile that motivated this work (#83774) showed context menu components at 6.6% of total render time.
Fixed Issues
#83774
Tests
Mini context menu (web — singleton behavior)
Full context menu (web right-click, mobile long-press)
Delete action
Emoji reactions (mini menu)
Edit action
Overflow menu
Offline tests
N/A — context menus are UI-only
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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari