-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix comment linking bugs #46315
Fix comment linking bugs #46315
Conversation
2b06d82
to
2ee37d8
Compare
0740e00
to
7b35f0f
Compare
7b35f0f
to
85ac515
Compare
85ac515
to
13c9acb
Compare
@ishpaul777 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] |
i'll priortize this today 🙇 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeWhatsApp.Video.2024-08-11.at.1.57.47.AM.mp4Android: mWeb ChromeWhatsApp.Video.2024-08-11.at.1.49.40.AM.mp4iOS: Nativetrim.7408C1CB-CB69-4B81-AAF5-0379D64FC0A8.MOViOS: mWeb SafariScreen.Recording.2024-08-17.at.1.29.52.AM.movMacOS: Chrome / SafariScreen.Recording.2024-08-17.at.1.02.14.AM-2.movMacOS: DesktopScreen.Recording.2024-08-17.at.1.49.13.AM-1.mov |
I still got stuck for few seconds, while scrolling after commentlinking 🤔 (cached messages) Record_2024-08-07-01-13-32.mp4 |
I am not sure if i understand the issue itself, could you please provide a after/before video. is this part of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java code is way over my head, i'll bring in a internal engineer later to help review this. Would be helpful if you can add a before/after video
Do you have to scroll up / down to unblock it or just wait? The rendering of new items is just a bit slow, especially in dev mode so it would be normal if it just stays stuck for a little bit of time. It would be good to profile though to see if it can be optimized.
It is part of
You can kind of see it in |
Is this the jump you are talking about 🤔 (this is on main branch) WhatsApp.Video.2024-08-08.at.2.10.58.AM.mp4 |
i think i have to scroll up down then new message came, i'll ask for a adhoc build (asked here )and test again to see if that makes a difference |
This comment has been minimized.
This comment has been minimized.
@ishpaul777 Would you be able to confirm whether these bugs are also present on main? I think we could address them in a separate PR if so, as this one is already pretty big. |
Not reproducable on main Screen.Recording.2024-08-15.at.11.16.09.PM.mov |
Not able to reproduce on with development build on simulator : \ Screen.Recording.2024-08-16.at.2.50.32.AM.mov |
I am only able to reproduce on real device, will debug it today. |
@ishpaul777 I am able to reproduce the issue when linking to first message on main too, I will have a quick look to see if it would be easy to fix, but I don't think it is a blocker to merge this. |
@ishpaul777 I've been trying to repro the other 2 issues on both main and my branch and wasn't able to. Can you confirm whether it still happens for you? |
I figured out the problem that causes the flickering. It is related to this patch https://github.com/janicduplessis/Expensify-App/blob/c91d1aac8ecbfbfce91b72bde1276aca74961e61/patches/react-native+0.73.4+016+iOSCoreAnimationBorderRendering.patch. It calls |
Checking give me few minutes |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
I pushed the fix for the first issue. Note that the bug only happens on physical devices, a debug build works fine too, as long as it runs on physical device. |
Updated description with the flicker fix
|
I am also not able to reproduce these on latest build 👍 Could be because of something wrong on main branch before |
@ishpaul777 Anything missing on my side or is everything addressed now? |
Addressed I was a bit occupied in other tasks now completing checklist |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #46217 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
discussing in slack here, but let's please be sure to go back and update this PR description with links to all the upstream commits and PRs for all the added patches |
✋ 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/roryabraham in version: 9.0.22-0 🚀
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.0.22-1 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.0.22-9 🚀
|
Details
This includes a few fixes for fixing janky or stuck scroll after comment linking. There were actually multiple different issues all with separate fixes. After all those scroll is now smooth and doesn't get stuck.
Fix onStartReached not called in some cases (15546b6): When scrolling down after comment linking, sometimes the scroll would get stuck. This is because onStartReached was never called. This is caused by the early return in
VirtualizedList.getDerivedStateFromProps
. It compares the number of items withrenderMask.numCells()
, but for some reason sometimes this is different from the previous number of items. So to fix it we now save the previous number of items in state and also compare that. Upstream PR Fix onStartReached not called facebook/react-native#46250Fix maintainVisibleContentPosition on Android (0b214f7): I found a few issues with the maintainVisibleContentPosition implementation on Android, which causes some jumping while scrolling. Part of it was already addressed in Fix maintainVisibleContentPosition on Android during momentum scroll facebook/react-native#43425, but we didn't have a patch for it so this adds it. There is also another issue where the first visible view is sometimes incorrectly updated so it moves the update to when the scroll view scrolls, instead of before views are updated. I also found an issue because we use z-index on list cells which cause views to be re-ordered and the wrong view ends up being selected as the first visible view. Upstream patch maintainVisibleContentPosition fixes on Android facebook/react-native#46247.
Fix newer messages not loading (5f9aede): There is a check in
loadNewerChats
that would sometimes cause scrolling to be stuck because new messages are not loaded. I verified with the author of this code @perunt and also did some testing and we concluded that this code is no longer needed and can be removed.Fix not found view flicker (c804881): Some code relies on the
isLinkingToMessage
variable to not display the not found view, but it is currently updated in auseLayoutEffect
. On web this works fine, but on native it cannot render synchronously so there will be a flicker of the wrong UI. To fix this we can update state in render instead.Fix scrollTo called when not needed on Android (13c9acb): This code is supposed to call scroll to only when the screen is focused, but it was being called more because the callback depended on the scroll position value, so every time scroll ends it would call scroll to. This caused issues sometimes where scroll position would become incorrect because it would scroll to an outdated position and revert the scroll caused by maintainVisibleContentPosition. To fix this we can remove the dependency on the scroll position of the callback by using a ref.
Fix flickering on iOS, this is caused by the
patches/react-native+0.73.4+016+iOSCoreAnimationBorderRendering.patch
patch which adds a CATransaction around mounting phase. This transaction should also include the willMount and didMount observer calls since we use those to adjust the scroll position and we don't want iOS to update the screen before those are called. This moves the CATransaction creation around the code that calls the observers.Known issues left, will address in another PR:
Before
Scrolling after comment linking with cache:
This shows janky scroll and scroll getting stuck.
Screen.Recording.2024-08-01.at.18.22.46.mov
Scrolling after comment linking no cache:
This shows the flicker of not found view as well as scroll being stuck.
Screen.Recording.2024-08-01.at.18.33.31.mov
After
Scrolling after comment linking with cache:
Screen.Recording.2024-08-01.at.14.32.36.mov
Scrolling after comment linking no cache:
Screen.Recording.2024-08-05.at.16.49.22.mov
Fixed Issues
$ #46217
$ #46222
PROPOSAL:
Tests
Offline tests
QA Steps
Comment linking flow:
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-08-01.at.14.32.36.mov
Android: mWeb Chrome
Screen.Recording.2024-08-05.at.17.00.52.mov
iOS: Native
Screen.Recording.2024-08-05.at.17.21.01.mov
iOS: mWeb Safari
Screen.Recording.2024-08-05.at.17.24.16.mov
MacOS: Chrome / Safari
Screen.Recording.2024-08-05.at.16.47.10.mov
MacOS: Desktop
Screen.Recording.2024-08-05.at.17.11.03.mov