Open linked message at top#93403
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.
|
…en-linked-message-centered
|
Three versions of loading on the initial report loading when there is a linked message:
1_full_loading.mp4
2_scroll_correction.mp4
3.center.linked.item.+.loading.mp4 |
|
@Expensify/design What do you think? |
|
Honestly I think I like option 1 the best. There's just so much bouncing around in the other ones. Curious for more thoughts from the team though. |
|
Yeah, that's where I lean towards as well. |
|
Same |
|
@quinthar, are you okay with going with the full-loading option, since it seems to be the most voted? |
|
Agree, perhaps we can fix it in a follow up though? |
|
Would love those little improvements in a follow up! (But def agree they're not blockers here) |
|
@aimane-chnaif Can you complete the checklist please? |
|
Not blocking on flicker issue (also happening on production) but let's try to fix this separately. flicker.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #92152 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
🤖 Code review — the approach (replace the data-slice 1. One patch change is not guarded and affects every - this.layoutManager.recomputeLayouts(0, initialScrollIndex);
+ this.layoutManager.recomputeLayouts(0, this.getDataLength() - 1);This now recomputes all item layouts (was: only up to the target) on initial mount for any list using 2. Magic numbers / hidden coupling in the bottom-crop heuristic. In 3. Test coverage. Codecov flags Things I checked that look fine
These are all NAB-adjacent except possibly #1 — none look like blockers, but #1 is the one I'd want a reviewer to consciously sign off on since it changes behavior for lists beyond this feature's scope. Next Steps: Reply with |
![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: fd247282dd
ℹ️ 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".
| return ( | ||
| <FlashList<T> | ||
| {...restProps} | ||
| {...props} |
There was a problem hiding this comment.
When a linked/unread chat opens near the newest edge and hasNewerActions is true, this now forwards the real onStartReached on the very first FlashList mount; the removed scroll-key hook used to replace it with a no-op during the initial positioning handoff. With onStartReachedThreshold={0.75}, FlashList can immediately call loadNewerChatsAfterTransitions, prepend newer actions, and move the target away from the requested top offset before the initial anchor has settled. Consider deferring/suppressing onStartReached until the initial linked-message positioning has completed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Previously onStartReached was supressed cause with old initialScrollKey logic, the reportActions array was cut off initially, and user always started at bottom -> onStartReached was always triggered by mistake.
Since we have changed the approach, I think we don't need this guard anymore.
|
To know the reason of this change - this.layoutManager.recomputeLayouts(0, initialScrollIndex);
+ this.layoutManager.recomputeLayouts(0, this.getDataLength() - 1);you can check this PR: Shopify/flash-list#2318 |
|
@mountiny are you reviewing this one too? |
|
Failing jest test fails in other PRs too: |
|
Restarting the jest test |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
LGTM, but asked for one WHY comment to be added, thanks!
| const targetIndex = initialScrollKey ? renderedVisibleReportActions.findIndex((item) => keyExtractor(item) === initialScrollKey) : -1; | ||
| let initialScrollIndex: number | undefined; | ||
| let initialScrollIndexParams: {viewPosition?: number; viewOffset?: number} | undefined; | ||
| if (targetIndex > 0) { | ||
| initialScrollIndex = targetIndex; | ||
| initialScrollIndexParams = {viewPosition: 1, viewOffset: CONST.REPORT.ACTIONS.LINKED_MESSAGE_OFFSET}; | ||
| } else if (shouldFocusToTopOnMount) { | ||
| initialScrollIndex = renderedVisibleReportActions.length - 1; | ||
| initialScrollIndexParams = {viewOffset: windowHeight}; | ||
| } |
There was a problem hiding this comment.
could you add plain english explanation of why we doing this so its easier to follow for other devs?
|
Merged through failing perf tests, reason is detailed in the PR description |
|
🚧 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.18-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no documentation changes required. I reviewed the changes in this PR against the help site content under Why no docs are needed:
Since no changes are required, I did not create a draft help site PR. @VickyStash, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #94402 was identified to be related to this PR. |
9 participants
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
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: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web.mp4