Fix - Viewport does not return to highlighted message after returning from thread#67752
Fix - Viewport does not return to highlighted message after returning from thread#67752Valforte merged 11 commits intoExpensify:mainfrom
Conversation
|
@dominictb 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] |
|
@dmkt9 We have conflicts. |
|
Screen.Recording.2025-08-07.at.01.15.56.movSame thing if the message is closer to the bottom. I think for the case of bottom, it already happens with the current inverted flat list so you might not need to fix it. But if you could, it would be much appreciated. |
| renderItem: ListRenderItem<T>; | ||
| }; | ||
|
|
||
| function BaseFlatList<T>(props: BaseFlatListProps<T>, ref: ForwardedRef<RNFlatList>) { |
There was a problem hiding this comment.
| function BaseFlatList<T>(props: BaseFlatListProps<T>, ref: ForwardedRef<RNFlatList>) { | |
| function FlatListWithScrollKey<T>(props: FlatListWithScrollKeyProps<T>, ref: ForwardedRef<RNFlatList>) { |
| shouldEnableAutoScrollToTopThreshold?: boolean; | ||
| renderItem: ListRenderItem<T>; | ||
| }; | ||
|
|
There was a problem hiding this comment.
| /** | |
| * FlatList component that handles initial scroll key. | |
| */ |
src/hooks/useFlatListScrollKey.ts
Outdated
| return data.slice(Math.max(0, currentDataIndex - (isInitialData ? 0 : getInitialPaginationSize))); | ||
| } | ||
| // If data.length > 1 and highlighted item is the last element, there will be a bug that does not trigger the `onStartReached` event. | ||
| // So we will need to return at least the last 2 elements in this case. |
There was a problem hiding this comment.
Yes, the value can be greater than 2. But returning 2 ensures that if the item height is large, the highlighted item remains in the visible viewport and covers the case where the data has only 2 items.
@dominictb This is what I expected. But actually when creating this PR I was quite worried because it creates unexpected jumps. However, this ensures that if App/src/components/FlatList/FlatListWithScrollKey.tsx Lines 46 to 49 in 91bfdba If we could know the exact height of the A video that lacks the logic mentioned above: Screencast_20250807_085700.mp4 |
|
@dominictb I am thinking of a solution that will create a loading state when rendering |
|
The core benefit of |
Thanks for the quick reply. I meant to only display it when |
|
@dominictb Here's a video of me trying out Screencast_20250807_094325.mp4 |
|
@dmkt9 Can you try with Android native? Slower animation might have visualize it better. |
@dominictb On mobile devices, I don't see this issue. It seems that due to the transition animation, the component is fully rendered while the animation is playing. |
|
Screen.Recording.2025-08-12.at.21.06.19-compressed.mov |
|
Screen.Recording.2025-08-12.at.21.13.42-compressed.movFor inverted flat list, the highlighted message is always at the bottom. So I expect normal flat list should pin it as the top message. |
Oh. Thanks. My bad. I refactored |
I just checked on Android but it works fine. I don't have my iOS device with me right now, I will check again tomorrow. |
|
@dmkt9 How's it going? |
@dominictb Thanks for your reminder. My mac has some problems. I will update as soon as possible. |
|
@dominictb Thanks for your patience. I've updated the PR to address the iOS issues. ios.test.result.mp4 |
|
Screen.Recording.2025-08-22.at.07.24.51.mov |
|
Screen.Recording.2025-08-22.at.07.25.26-compressed.mov |
Our PR doesn't affect the inverted FlatList, so it's not the cause of the issue. There's a separate issue tracking it. |
Thanks for your review. This is my solution to address the bug on iOS. The bug is caused by a specific mechanism unique to iOS. |
|
Screen.Recording.2025-09-03.at.07.09.40.mov |
| // eslint-disable-next-line react-compiler/react-compiler | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
| // eslint-disable-next-line react-compiler/react-compiler | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps |
| */ | ||
| const initialNumToRender = useMemo((): number | undefined => { | ||
| const minimumReportActionHeight = styles.chatItem.paddingTop + styles.chatItem.paddingBottom + variables.fontSizeNormalHeight; | ||
| const availableHeight = windowHeight - (CONST.CHAT_FOOTER_MIN_HEIGHT + variables.contentHeaderHeight); |
There was a problem hiding this comment.
This does not account for the list header component here, right?
App/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx
Lines 713 to 719 in 79de25f
There was a problem hiding this comment.
@dominictb Sorry for the delay. Yes. However, since we only render the ListHeaderComponent after the initial render is complete, it's not really necessary to calculate its height.
The method for determining initialNumToRender is based on the approach used in ReportActionsList.
App/src/pages/home/report/ReportActionsList.tsx
Lines 543 to 555 in 79de25f
@dominictb Hi, I’ve tried to reproduce it, but had no luck. However, our PR doesn’t touch the “New message” pill logic, so I don’t think it’s caused by our changes. 2025-09-08.17-52-03.mp4 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-09-09.at.16.49.18-compressed.movAndroid: mWeb ChromeScreen.Recording.2025-09-09.at.16.38.31-compressed.moviOS: HybridAppScreen.Recording.2025-09-09.at.16.30.14-compressed.moviOS: mWeb SafariScreen.Recording.2025-09-09.at.16.32.51-compressed.movMacOS: Chrome / SafariScreen.Recording.2025-09-09.at.16.27.53-compressed.movMacOS: DesktopScreen.Recording.2025-09-09.at.16.31.56-compressed.mov |
|
Tests well. @dmkt9 Can you merge |
@dominictb Sure. I just merged it. |
|
Perf test is not related. |
dominictb
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the hard work but be mindful of potential regressions.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Valforte in version: 9.2.9-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Valforte in version: 9.2.11-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.2.12-4 🚀
|
Explanation of Change
Fixed Issues
$ #61550
PROPOSAL: #61550 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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))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.native.mp4
Android: mWeb Chrome
android.mweb.chrome.mp4
iOS: Native
ios.native.mp4
iOS: mWeb Safari
ios.safari.mp4
MacOS: Chrome / Safari
mac.safari.mp4
MacOS: Desktop
mac.desktop.mp4