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? |
![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: e29132c09a
ℹ️ 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) { | ||
| return <ReportActionsSkeletonView />; |
There was a problem hiding this comment.
When OpenReport fails while opening a linked/unread message, the failure path only clears isLoadingInitialReportActions and does not set hasOnceLoadedReportActions, so this condition stays true forever while the route still has initialScrollKey. In that online failure or timeout scenario, users remain stuck on the skeleton instead of seeing cached actions or any retry/error state; gate this on the request still loading rather than hasOnceLoadedReportActions alone.
Useful? React with 👍 / 👎.
|
🚧 @shawnborton 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! 🧪🧪
|
|
@VickyStash Seems like the reassure failure might be related to these changes |
Yes, please see the PR description, it has a detailed explanation. Let me know if you have any questions. |
|
Ah I see thanks! There are conflicts @aimane-chnaif can you review please? |
# Conflicts: # patches/@shopify/flash-list/details.md
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
@shawnborton are you happy with the behaviour now? |
|
Clicking on action badge pill scrolls up to see the report action at the bottom. Screen.Recording.2026-06-19.at.6.25.35.PM.mov |
|
There's a small flicker when open linked message first time. Screen.Recording.2026-06-19.at.6.28.48.PM.movScreen.Recording.2026-06-19.at.6.31.54.PM.movFlicker also happens in production so let's not block on this. Screen.Recording.2026-06-19.at.6.35.29.PM.mov |
|
The new behavior looks great to me! The only thing I noticed was that if you have a message that is marked as unread that is quite high up in the history... if you scroll even higher up, you see the green pill: Clicking that just brings you to the bottom instead of the message that actually says new. Is that expected? Maybe it is, but just wanted to check! |
|
This is what I mean: CleanShot.2026-06-20.at.07.51.04.mp4 |
|
I would expect that it takes you to the first new message, but not sure if this is something this PR is changing @aimane-chnaif can you please check how this compares with production? thanks! |
I've just check and it's production behaviour too, so not created in this PR: prod_behaviour.mp4Regarding this @aimane-chnaif comment:
I guess I should update to scroll after pill tap to the same position as it would scroll in case of deep linking, right? @mountiny @shawnborton, so two questions:
|
|
I think both of these are NAB for this PR |
|
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 |
| if (targetIndex > 0) { | ||
| initialScrollIndex = targetIndex; | ||
| // top position with 40px offset | ||
| initialScrollIndexParams = {viewPosition: 1, viewOffset: 40}; |
There was a problem hiding this comment.
NAB: Define 40px value in CONST to be reusable.
|
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 |
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 |
| isTrackIntentUser, | ||
| }); | ||
|
|
||
| const targetIndex = initialScrollKey ? renderedVisibleReportActions.findIndex((item) => keyExtractor(item) === initialScrollKey) : -1; |
There was a problem hiding this comment.
NAB, do we need the ternary? Will keyextractor ever return undefined? Can we just use the default behavior of findIndex, since it won't match if initial scroll key doesn't exist?
|
@mountiny are you reviewing this one too? |
7 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