-
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 the new message button doesn't work on some comment linkings #46627
Conversation
Signed-off-by: Tsaqif <[email protected]>
@s77rt I am not sure if I should include my proposed fix for |
@tsa321 Is the pagination bug still reproducible? If so can you please add reproduction steps? |
@tsa321 It seems that something broke Screen.Recording.2024-08-01.at.8.31.50.AM.mov |
Signed-off-by: Tsaqif <[email protected]>
@s77rt Thanks for the review. For the last part of the video, where the page doesn’t scroll to the last message, we need to include the Regarding the flicker of the loading skeleton, could you provide more details about the linked message, such as how many positions up from the last message it is? Also, could you reproduce the issue in the main branch? macos-web-d.mp4I will try to submit new commit tomorrow. |
The latest commit didn't fix the reported but and now the "New message" button is not visible Screen.Recording.2024-08-01.at.6.16.02.PM.mov |
@s77rt, since the current commit is minimal, could you check if the following three bugs are also present in the main branch by manually reverting my changes?
Also, is it possible for me to get the exported Onyx data? |
Signed-off-by: Tsaqif <[email protected]>
@tsa321 Those bugs appear to be on |
…types and lints Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
@s77rt I have tried to fix the bugs in the last commit. Although there is a GitHub check error, could you please re-test it |
Signed-off-by: Tsaqif <[email protected]>
src/pages/home/ReportScreen.tsx
Outdated
@@ -95,7 +96,8 @@ function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportAc | |||
return parentReportActions[parentReportActionID ?? '0']; | |||
} | |||
|
|||
function ReportScreen({route, currentReportID = '', navigation}: ReportScreenProps) { | |||
function ReportScreen({currentReportID = '', navigation}: ReportScreenProps) { | |||
const route = useRoute<RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>>(); |
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.
Why is this change necessary?
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.
@s77rt When a user clicks the same linked message twice (when the current URL path and the clicked link are the same), the useRoute
returns a different object (different reference to object), but it has the same content as the previous one. However, the route
property of the ReportScreen
remains the same in both content and object reference. Consequently, the second click does not work (won't re-render) using route property of ReportScreen
. Here is the video:
using route property of report screen:
usginRouteProperty.mp4
using useRoute:
usingUseRoute.mp4
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.
But this is already working on main
, what broke it?
Screen.Recording.2024-08-04.at.5.14.09.PM.mov
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.
@s77rt Because I am fixing this bug, I am using the same source of route in ReportScreen
, ReportActionsView
, and ReportActionsList
.
In your video, the bug does not appear in the main branch because ReportActionsView
is using useRoute
.
You can reproduce this bug by reverting these two lines in my changes, meaning by using the route
property of ReportScreen
instead of useRoute
.
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.
@tsa321 Can you please clarify the solution independently for those two bugs:
- Not showing "new message" button
- Not navigating on clicking same report action link
(Also please merge main
)
src/libs/PaginationUtils.ts
Outdated
@@ -124,7 +124,7 @@ function mergeAndSortContinuousPages<TResource>(sortedItems: TResource[], pages: | |||
const result = [sortedPages[0]]; | |||
for (let i = 1; i < sortedPages.length; i++) { | |||
const page = sortedPages[i]; | |||
const prevPage = sortedPages[i - 1]; | |||
const prevPage = result?.at(-1) ?? sortedPages[i - 1]; |
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.
Why do we fallback to sortedPages[i - 1]
?
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.
Actually I am trying to fix a type-checking error. result?.at(-1)
gives a type-checking error. Should I use result[result?.length - 1]
instead?
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.
prevPage
is expected to be undefined
at first, no need for the fallback
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.
Alright, since result?.at(-1)
fails the type check, I will use result[result?.length - 1]
instead.
Signed-off-by: Tsaqif <[email protected]>
I missed this comment. To reproduce the issue, the steps are:
|
Signed-off-by: Tsaqif <[email protected]>
@@ -124,7 +124,7 @@ function mergeAndSortContinuousPages<TResource>(sortedItems: TResource[], pages: | |||
const result = [sortedPages[0]]; | |||
for (let i = 1; i < sortedPages.length; i++) { | |||
const page = sortedPages[i]; | |||
const prevPage = sortedPages[i - 1]; | |||
const prevPage = result[result.length - 1]; |
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.
prepage (result[result.length - 1]
) could be same as page if the result contains only one page
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.
@s77rt What? I don't understand. Could you elaborate please?
Do you mean when the sortedPages.length is 1. If it is 1 it won't even enter the for loop.
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.
If result length is 1, then:
page = sortedPages[0]
and
prevPage = sortedPages[0]
page is same as prevPage which is misleading and could cause bugs
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.
@s77rt Maybe I still don't understand.
The i
in the for
loop starts at 1, not 0, so page = sortedPages[0]
is not possible. Consequently, page
and prevPage
won't be the same.
Am I missing something?
const prevPage = result[result.length - 1];
Here I am using the last updated page of result and not the sortedPages.
The end of result
could be only 1 continuous page for for example 10 pages
in sortedPages
. Because the function merge the pages into one continuous page.
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.
Ah right! my bad
src/pages/home/ReportScreen.tsx
Outdated
@@ -95,7 +96,8 @@ function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportAc | |||
return parentReportActions[parentReportActionID ?? '0']; | |||
} | |||
|
|||
function ReportScreen({route, currentReportID = '', navigation}: ReportScreenProps) { | |||
function ReportScreen({currentReportID = '', navigation}: ReportScreenProps) { | |||
const route = useRoute<RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>>(); |
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.
@tsa321 Can you please clarify the solution independently for those two bugs:
- Not showing "new message" button
- Not navigating on clicking same report action link
(Also please merge main
)
@s77rt for bug:
|
@s77rt Is it also reproducible in main? |
@tsa321 I can reproduce on |
@s77rt Should I fix that issue as well? I need more information to reproduce the bug. |
@tsa321 It would be better if we can get this fixed or at least find what went wrong. Can't you reproduce the bug on |
@s77rt I cannot reproduce the issue, so it will be really difficult to figure out. Is your connection slow? It might be related to I'm just guessing here because I can't reproduce the issue. Could you provide more information: which counts from the last message are the first and second linked messages, and how many messages are in the report? Or if you could invite me to the room or make it public report... Alternatively, we could address this in a follow-up PR. |
@tsa321 Let's not block on that.
This should refer to step 3 |
@s77rt I have edited the test step. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
On Android the new message button does not scroll to the bottom android-bug.mov |
@s77rt, on Android, this issue occurs in the main branch on many comment linkings. Even without clearing the Onyx cache (cached messages exist) , the Before merging the main branch, everything worked as expected, as shown in the Android video test I uploaded. However, after merging, the same link I used for testing fails to display the newest message. |
@s77rt update: Removing this useEffect: App/src/components/FlatList/index.android.tsx Lines 28 to 32 in 65bca91
works as expected for me. |
@tsa321 That code was there for a while, it can't be the culprit. Can you try find the offending PR or the root cause. Also try disable strict mode and test on production |
Signed-off-by: Tsaqif <[email protected]>
setScrollPosition(undefined); | ||
return () => setScrollPosition(undefined); | ||
},[]) | ||
|
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.
The CustomFlatList
component tries to scroll the flat list back using useFocusEffect
below. However, when we open or click on a comment link, we don't need to restore the scroll position. Therefore, we must reset the last scroll position to undefined
on mount and unmount.
import type {FlatListProps, NativeScrollEvent, NativeSyntheticEvent} from 'react-native'; | ||
import {FlatList} from 'react-native'; | ||
import {ActionListContext} from '@pages/home/ReportScreenContext'; | ||
|
||
// FlatList wrapped with the freeze component will lose its scroll state when frozen (only for Android). | ||
// CustomFlatList saves the offset and use it for scrollToOffset() when unfrozen. | ||
function CustomFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<FlatList>) { | ||
const {scrollPosition, setScrollPosition} = useContext(ActionListContext); | ||
const {setScrollPosition, getScrollPosition} = useContext(ActionListContext); | ||
|
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.
I am replacing scrollPosition
with getScrollPosition
because scrollPosition
is a state in ReportScreen
and does not represent the last value here. Using useState
for scroll position also causes the ReportScreen
to re-render unnecessarily when user stop scrolling the flat list.
I have verified that scrollPosition
is only used in here.
For more details, see ReportScreen
.
src/pages/home/ReportScreen.tsx
Outdated
|
||
const getScrollPosition = useCallback(() => { | ||
return scrollPosition; | ||
}, []); | ||
|
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.
I am replacing scrollPosition with getScrollPosition because scrollPosition is a state in ReportScreen and the setScrollPosition will cause re-render of reportscreen unnecessarily and cause the flatlist to jumping around. Using useState for scroll position also causes the ReportScreen to re-render unnecessarily when user stop scrolling the flat list.
@s77rt I have fixed the issue please review before I am fixing the type or lint errors. |
@tsa321 Before going with a solution, we need to find the root cause and since this broke recently it's better to find the offending PR first |
I haven’t been able to find it. Should we wait for it to be fixed in the main branch, or do you have another suggestion? I'm fine with either option. |
@tsa321 Can you revert the latest changes and merge |
Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
@s77rt I have reverted the last changes and merged main |
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.
on Android it's partially working as expected but that seems due to another cause. Let's not block on that
android.mov
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
FYI I believe this was deployed to prod yesterday, from this checklist - #47219 |
Details
Fixed Issues
$ #45004
PROPOSAL: #45004 (comment)
Tests
Offline tests
QA Steps
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
android-native-d.mp4
Android: mWeb Chrome
android-mweb_d.mp4
iOS: Native
ios-native_d.mp4
iOS: mWeb Safari
ios-msfari_d.mp4
MacOS: Chrome / Safari
macos-web-d.mp4
MacOS: Desktop
macos-desktop_d.mp4