[CP Staging] Fix QA guide avatar disappearing when reopening admins room#88571
Conversation
Use the action's actual index in sortedVisibleReportActions for displayAsGroup computation instead of the FlashList-provided index, which may be offset when useFlashListScrollKey slices the data array for deep-link scroll positioning. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
| // Use the action's actual index in sortedVisibleReportActions rather than the FlashList-provided index, | ||
| // because useFlashListScrollKey may slice the data for deep-link scroll positioning, making the | ||
| // FlashList index offset from the full array and causing wrong displayAsGroup computation. | ||
| const actionIndex = sortedVisibleReportActions.indexOf(reportAction); |
There was a problem hiding this comment.
@MelvinBot In favor of this line, no need to pass index anymore here:
App/src/pages/inbox/report/ReportActionsList.tsx
Lines 848 to 851 in c70c52b
There was a problem hiding this comment.
Good catch! Since renderItem now computes the real index via indexOf internally, the duplicate indexOf call in renderTopReportActions was redundant. Removed it — now just using the map index as the parameter (it only serves as a fallback). Fixed in f20ecda.
Since renderItem now computes the real index via indexOf internally, the call site in renderTopReportActions no longer needs to compute it. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
@MelvinBot add Tests/QA Steps, check off all items in author checklist |
|
Updated the PR body: added Tests/QA steps and checked off all author checklist items. Since this is a logic-only change in |
|
@MelvinBot add this test case as Case 2 in Tests/QA Steps, as this is reproducible in production. |
|
Added the production repro steps as Case 2 in both the Tests and QA Steps sections of the PR body. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chromemchrome.moviOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
@MelvinBot mark as ready for review |
|
I'm unable to mark this PR as ready for review due to environment permission restrictions. Please click the Ready for review button on the PR page to convert it from draft. |
|
@MelvinBot mark as ready for review |
|
✅ PR marked as ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f20ecdaac6
ℹ️ 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".
| const actionIndex = sortedVisibleReportActions.indexOf(reportAction); | ||
| const safeIndex = actionIndex >= 0 ? actionIndex : index; |
There was a problem hiding this comment.
Remove linear lookup from the item render hot path
Computing actionIndex via sortedVisibleReportActions.indexOf(reportAction) inside renderItem makes each row render O(n) in the total action count, so list updates become O(visibleRows × totalRows). In large chats/high-traffic accounts this runs on every scroll/render cycle and can introduce noticeable UI jank compared to the previous O(1) per-item path. Please precompute an index map (e.g., by reportActionID) once per data change or otherwise avoid a full-array scan in the per-item renderer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done — replaced the indexOf call with a precomputed Map<reportActionID, index> via useMemo, so index lookup in renderItem is now O(1). Fixed in b9d902d.
Avoids O(n) array scan per rendered row by precomputing a reportActionID-to-index Map once per data change. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
Code ReviewThe fix is correct and well-structured. A few notes: What's good:
No issues found. The fix correctly addresses the root cause: FlashList indices are relative to sliced |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9d902d46a
ℹ️ 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".
| item: action, | ||
| index: sortedVisibleReportActions.indexOf(action), | ||
| index, | ||
| } as ListRenderItemInfo<OnyxTypes.ReportAction>)} |
There was a problem hiding this comment.
Preserve global action index in static preview
renderTopReportActions() now passes the preview-local index instead of the action’s index in sortedVisibleReportActions, but this value is forwarded as index to ReportActionsListItemRenderer and ultimately used by ReportActionItemMessageEdit (e.g., deleteDraft treats index === 0 as a special “latest message” case when scrolling). In StaticReportActionsPreview (shown while shouldScrollToEndAfterLayout is true), the sliced/reversed preview indices do not match global list indices, so dismissing/saving a draft can scroll to the wrong item or skip the expected scroll behavior.
Useful? React with 👍 / 👎.
| {renderItem({ | ||
| item: action, | ||
| index: sortedVisibleReportActions.indexOf(action), | ||
| index, |
There was a problem hiding this comment.
Reverted — renderTopReportActions now passes the global index via actionIndexMap instead of the preview-local map index. Fixed in cceddfb.
The preview-local map index doesn't match the global list index, which can cause wrong scroll behavior for draft dismiss/save. Use actionIndexMap to pass the correct global index. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
Code Review (updated)The fix is correct. The current diff uses a Summary of changes:
No issues found. The fix correctly addresses the root cause: FlashList indices are relative to sliced |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
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". |
|
Ready for final approval and merge 🎉 |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🧪🧪 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. |
…pearing [CP Staging] Fix QA guide avatar disappearing when reopening admins room (cherry picked from commit c27efa4) (cherry-picked to staging by mountiny)
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.61-4 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is an internal rendering bug fix — it corrects how |
|
CP is passed by the QA team, comment |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.3.61-4 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.62-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.64-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a bug fix that corrects an internal rendering index mismatch in |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.3.64-31 🚀
|
Explanation of Change
When navigating to the #admins room with a
linkedReportActionID(fromactionTargetReportActionIDinSidebarLinks),useFlashListScrollKeyslices the data array on the initial render to position the linked action at the visual bottom. FlashList then passesindexvalues relative to this sliceddisplayedData, butrenderItemuses those indices against the fullsortedVisibleReportActionsarray for thedisplayAsGroupcomputation. This index mismatch causesisConsecutiveActionMadeByPreviousActorto compare the wrong adjacent actions, returningtrueincorrectly and hiding the QA guide's avatar/name.This fix resolves the mismatch by looking up the action's real index in
sortedVisibleReportActions(viaindexOf) instead of relying on the FlashList-provided index.Fixed Issues
$ #88544
PROPOSAL: #88544 (comment)
Tests
Case 1: Deep-link to a specific message
linkedReportActionID)displayAsGroupcorrectly groups consecutive messages from the same actorCase 2: Navigate back from task detail (reproducible in production)
Offline tests
This change only affects how list indices are resolved for
displayAsGroupcomputation. Offline behavior is unchanged — the samesortedVisibleReportActionsarray is used regardless of network state.QA Steps
Case 1: Deep-link to a specific message
Case 2: Navigate back from task detail (reproducible in production)
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