Open linked message at top - 2#94671
Conversation
|
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@aimane-chnaif 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] |
melvin-bot
Bot
requested review from
a team and
JS00001
and removed request for
a team
![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: 1e99c5b0d9
ℹ️ 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".
| if (initialScrollKey && !isOffline && !reportLoadingState?.hasOnceLoadedReportActions && reportLoadingState?.isLoadingInitialReportActions) { | ||
| return <ReportActionsSkeletonView />; |
There was a problem hiding this comment.
When this loading gate is hit for an online deep link/unread open, listHeaderComponent has already been evaluated above; on the first pass canShowHeader is false and that memo sets hasHeaderRendered.current = true. Returning the skeleton here means the real FlashList has not mounted yet, so the next render mounts it with the real ReportActionsListHeader instead of the intended empty first-render header/spacer. That adds the header to the first measured layout and can shift the initial initialScrollIndex anchor, especially in the linked-message path this gate is trying to stabilize. Move this gate before the header memo or defer flipping the ref until the list actually renders.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There is no visible affection I think, so I'd leave it as is
There was a problem hiding this comment.
This hasHeaderRendered code will be removed soon here: #94149, also ReportActionsListHeader is mostly dead code I remove there so no worries
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
Confirmed #94402 is no longer reproducible |
|
Perf tests are expected to fail, merging |
|
🚧 JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.4.21-2 🚀
|
5 participants
This is second try of the PR.
First PR: #93403
Reverted in #94470 with DB #94402
Explanation of Change
When a chat is opened at a specific message — via a comment deep link or the unread marker — position the target message near the top of the viewport (with a 40px offset) , so the user immediately sees the message in context right away.
FlashList patch 013 —
viewPositionsupport forinitialScrollIndexParamsThe existing slice-based approach (
useFlashListScrollKey, which cut the data at the target so it landed at the bottom edge) is replaced by FlashList's nativeinitialScrollIndexmachinery, extended with a newviewPositionoption (0 = start, 0.5 = center, 1 = end — same semantics asscrollToIndex).ReportActionsList
initialScrollIndex/initialScrollIndexParamsdirectly:targetIndex > 0):{viewPosition: 1, viewOffset: 40}→ target near the top with a 40px offset.shouldFocusToTopOnMount(money/expense report): unchangedReportActionsSkeletonViewwhile a linked message is loading (initialScrollKey && !isOffline && !hasOnceLoadedReportActions): the batch of actions that arrives right after the initial load would otherwise shift the list and break the anchor.actionIndexMapworkaround and revertsdisplayAsGroupto use the FlashList-providedindex— the data is no longer sliced, so the index already matchesrenderedVisibleReportActions.Detailed explanation
Reassure:
ReportActionsList.perf-testrender-count change explainedThe reassure check reports a render-count regression on this test (3 → 6). After investigation, most of this is a measurement artifact, not a real performance regression. Details below.
TL;DR
main's baseline of 3 is artificially low: ~5 of its renders arerequestAnimationFrame-deferred and fall outside reassure's measurement window, so they're never counted.Why the baseline is misleadingly low
Reassure counts every React commit between
render()andcleanup(). The window closes the moment the scenario (await screen.findByTestId('report-actions-list')) resolves — which happens on the first commit, because the list's testID is present immediately. Combined withfakeTimers.enableGlobally: true(sorequestAnimationFrameis faked and only fires when timers advance),main's rAF-driven renders never run before the component is unmounted.Proof — running pure
maincode and only adding a timer/rAF flush inside the scenario:mainSo
maintruly does ~8 renders of work; reassure attributes only 3 to it.What this branch actually changes
The entire +3 comes from the scroll-positioning rewrite — replacing
useFlashListScrollKey(data-slice + 2-frame rAF handoff) with nativeinitialScrollIndex. This shifts work from rAF-deferred (uncounted) to synchronous (counted). Bisecting the +3:initialScrollIndex/initialScrollIndexParams(ReportActionsList.tsx,targetIndex > 0branch) makes FlashList scroll on mount, which firesonContentSizeChange → trackVerticalScrollingsynchronously inside the measurement window. Onmainthis work happened via rAF, outside the window.useFlashListScrollKey. Onmain, wheninitialScrollKeywas set, that hook sliced the data (data.slice(targetIndex)) so the linked action sat at the visual bottom with no native scroll, then ran a controlled 2-frame rAF handoff (deferred, uncounted). The branch deletes the hook and lets the full list mount and settle synchronously instead.Conclusion
No meaningful list-rendering slowdown (durations are flat). The headline +3 is almost entirely a measurement artifact: the scroll-positioning rewrite moves the same work from rAF-deferred (uncounted by reassure) to synchronous (counted). With all deferred renders counted, the real delta is only +1 (8 → 9). Re-baselining is appropriate.
Profiling on web shows no meaningful difference in main vs branch measurements:
Fixed Issues
$ #92152
PROPOSAL: N/A
Tests
On Android DB steps should also be verified: #94402
Offline tests
Same, as in the Tests section
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in the Tests section
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.mp4
android-no-crash.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web.mp4