Skip to content
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.

Sign up for GitHub

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

Merged
merged 17 commits into from
Aug 12, 2024
Merged

Fix the new message button doesn't work on some comment linkings #46627

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
00a7bed
Fix the new message button doesn't work for some comment linkings
tsa321 Aug 1, 2024
69ce490
Merge branch 'main' into fixNwMessageBtnCmntLnkng
tsa321 Aug 1, 2024
4987917
Fix pagination util prev page
tsa321 Aug 1, 2024
76f9489
Using one source of route for report screen, view and list
tsa321 Aug 2, 2024
7036096
using one source of route in report screen, view and list and fixees …
tsa321 Aug 2, 2024
4b4b369
Merge branch 'main' into fixNwMessageBtnCmntLnkng
tsa321 Aug 2, 2024
3235453
revert index of linked action search
tsa321 Aug 2, 2024
ceae85f
Fix typechek error
tsa321 Aug 2, 2024
a9d1374
Fix perf-test error
tsa321 Aug 2, 2024
02fca25
Change method to get last item of result
tsa321 Aug 5, 2024
da00fa5
Fix lint
tsa321 Aug 5, 2024
19844ab
Merge branch 'main' into fixNwMessageBtnCmntLnkng
tsa321 Aug 5, 2024
7553ed6
Revert route scheme
tsa321 Aug 6, 2024
abae889
Revert perf-test
tsa321 Aug 6, 2024
c498c92
Fix commment linking jumping around and new message button in android
tsa321 Aug 8, 2024
0993bd4
Revert changes
tsa321 Aug 10, 2024
c3bc9d1
Merge main
tsa321 Aug 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libs/PaginationUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

@tsa321 tsa321 Aug 5, 2024 •
edited
Loading

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right! my bad


// Current page is inside the previous page, skip
if (page.lastIndex <= prevPage.lastIndex && page.lastID !== CONST.PAGINATION_END_ID) {
Expand Down
9 changes: 6 additions & 3 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {PortalHost} from '@gorhom/portal';
import {useIsFocused} from '@react-navigation/native';
import type {RouteProp} from '@react-navigation/native';
import {useIsFocused, useRoute} from '@react-navigation/native';
import type {StackScreenProps} from '@react-navigation/stack';
import lodashIsEqual from 'lodash/isEqual';
import React, {memo, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState} from 'react';
Expand Down Expand Up @@ -32,14 +33,14 @@ import useViewportOffsetTop from '@hooks/useViewportOffsetTop';
import Timing from '@libs/actions/Timing';
import Log from '@libs/Log';
import Navigation from '@libs/Navigation/Navigation';
import type {AuthScreensParamList} from '@libs/Navigation/types';
import clearReportNotifications from '@libs/Notification/clearReportNotifications';
import Performance from '@libs/Performance';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import shouldFetchReport from '@libs/shouldFetchReport';
import * as ValidationUtils from '@libs/ValidationUtils';
import type {AuthScreensParamList} from '@navigation/types';
import * as ComposerActions from '@userActions/Composer';
import * as Report from '@userActions/Report';
import CONST from '@src/CONST';
Expand Down Expand Up @@ -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>>();
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

@tsa321 tsa321 Aug 5, 2024 •
edited
Loading

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.

Copy link
Contributor

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)

const styles = useThemeStyles();
const {translate} = useLocalize();
const reportIDFromRoute = getReportID(route);
Expand Down Expand Up @@ -782,6 +784,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
<ReportActionsView
reportActions={reportActions}
report={report}
route={route}
parentReportAction={parentReportAction}
isLoadingInitialReportActions={reportMetadata?.isLoadingInitialReportActions}
isLoadingNewerReportActions={reportMetadata?.isLoadingNewerReportActions}
Expand Down
7 changes: 5 additions & 2 deletions src/pages/home/report/ReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {ListRenderItemInfo} from '@react-native/virtualized-lists/Lists/VirtualizedList';
import {useIsFocused, useRoute} from '@react-navigation/native';
import {useIsFocused} from '@react-navigation/native';
import type {RouteProp} from '@react-navigation/native';
import type {DebouncedFunc} from 'lodash';
import React, {memo, useCallback, useEffect, useMemo, useRef, useState} from 'react';
Expand Down Expand Up @@ -43,6 +43,9 @@ type ReportActionsListProps = WithCurrentUserPersonalDetailsProps & {
/** The report currently being looked at */
report: OnyxTypes.Report;

/** The current route of report screen */
route: RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>;

/** The transaction thread report associated with the current report, if any */
transactionThreadReport: OnyxEntry<OnyxTypes.Report>;

Expand Down Expand Up @@ -142,6 +145,7 @@ const onScrollToIndexFailed = () => {};

function ReportActionsList({
report,
route,
transactionThreadReport,
reportActions = [],
parentReportAction,
Expand Down Expand Up @@ -171,7 +175,6 @@ function ReportActionsList({

const {isOffline} = useNetwork();
const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? -1}`);
const route = useRoute<RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>>();
const opacity = useSharedValue(0);
const reportScrollManager = useReportScrollManager();
const userActiveSince = useRef<string | null>(null);
Expand Down
18 changes: 14 additions & 4 deletions src/pages/home/report/ReportActionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {useIsFocused} from '@react-navigation/native';
import type {RouteProp} from '@react-navigation/native';
import {useIsFocused, useRoute} from '@react-navigation/native';
import lodashIsEqual from 'lodash/isEqual';
import React, {useCallback, useContext, useEffect, useLayoutEffect, useMemo, useRef, useState} from 'react';
import {InteractionManager} from 'react-native';
Expand All @@ -13,7 +13,6 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useWindowDimensions from '@hooks/useWindowDimensions';
import DateUtils from '@libs/DateUtils';
import getIsReportFullyVisible from '@libs/getIsReportFullyVisible';
import type {AuthScreensParamList} from '@libs/Navigation/types';
import * as NumberUtils from '@libs/NumberUtils';
import {generateNewRandomInt} from '@libs/NumberUtils';
import Performance from '@libs/Performance';
Expand All @@ -22,6 +21,7 @@ import * as ReportUtils from '@libs/ReportUtils';
import {isUserCreatedPolicyRoom} from '@libs/ReportUtils';
import {didUserLogInDuringSession} from '@libs/SessionUtils';
import shouldFetchReport from '@libs/shouldFetchReport';
import type {AuthScreensParamList} from '@navigation/types';
import {ReactionListContext} from '@pages/home/ReportScreenContext';
import * as Report from '@userActions/Report';
import Timing from '@userActions/Timing';
Expand Down Expand Up @@ -50,6 +50,9 @@ type ReportActionsViewProps = ReportActionsViewOnyxProps & {
/** The report currently being looked at */
report: OnyxTypes.Report;

/** The current route of report screen */
route: RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>;

/** Array of report actions for this report */
reportActions?: OnyxTypes.ReportAction[];

Expand Down Expand Up @@ -85,6 +88,7 @@ const SPACER = 16;
let listOldID = Math.round(Math.random() * 100);

function ReportActionsView({
route,
report,
transactionThreadReport,
session,
Expand All @@ -101,8 +105,8 @@ function ReportActionsView({
}: ReportActionsViewProps) {
useCopySelectionHelper();
const reactionListRef = useContext(ReactionListContext);
const route = useRoute<RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>>();
const reportActionID = route?.params?.reportActionID;
const prevReportActionID = usePrevious(reportActionID);
const didLayout = useRef(false);
const didLoadOlderChats = useRef(false);
const didLoadNewerChats = useRef(false);
Expand Down Expand Up @@ -146,7 +150,7 @@ function ReportActionsView({

// Change the list ID only for comment linking to get the positioning right
const listID = useMemo(() => {
if (!reportActionID) {
if (!reportActionID && !prevReportActionID) {
// Keep the old list ID since we're not in the Comment Linking flow
return listOldID;
}
Expand Down Expand Up @@ -492,6 +496,7 @@ function ReportActionsView({
<>
<ReportActionsList
report={report}
route={route}
transactionThreadReport={transactionThreadReport}
reportActions={reportActions}
parentReportAction={parentReportAction}
Expand Down Expand Up @@ -527,6 +532,11 @@ function arePropsEqual(oldProps: ReportActionsViewProps, newProps: ReportActions
if (!lodashIsEqual(oldProps.isReadyForCommentLinking, newProps.isReadyForCommentLinking)) {
return false;
}

if (oldProps.route !== newProps.route) {
return false;
}

if (!lodashIsEqual(oldProps.reportActions, newProps.reportActions)) {
return false;
}
Expand Down
2 changes: 2 additions & 0 deletions tests/perf-test/ReportActionsList.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ beforeAll(() =>
const mockOnLayout = jest.fn();
const mockOnScroll = jest.fn();
const mockLoadChats = jest.fn();
const mockRoute = {params: {reportID: '1', reportActionID: ''}, key: 'Report', name: 'Report' as const};
const mockRef = {current: null, flatListRef: null, scrollPosition: null, setScrollPosition: () => {}};

beforeEach(() => {
Expand All @@ -91,6 +92,7 @@ function ReportActionsListWrapper() {
parentReportActionForTransactionThread={undefined}
sortedReportActions={ReportTestUtils.getMockedSortedReportActions(500)}
report={LHNTestUtilsModule.getFakeReport()}
route={mockRoute}
onLayout={mockOnLayout}
onScroll={mockOnScroll}
onContentSizeChange={() => {}}
Expand Down
4 changes: 2 additions & 2 deletions tests/perf-test/ReportScreen.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,14 @@ jest.mock('@src/libs/Navigation/Navigation', () => ({
isDisplayedInModal: jest.fn(() => false),
}));

const mockRoute = {params: {reportID: '1', reportActionID: ''}, key: 'Report', name: 'Report' as const};
jest.mock('@react-navigation/native', () => {
const actualNav = jest.requireActual<typeof Navigation>('@react-navigation/native');
return {
...actualNav,
useFocusEffect: jest.fn(),
useIsFocused: () => true,
useRoute: () => jest.fn(),
useRoute: () => mockRoute,
useNavigation: () => ({
navigate: jest.fn(),
addListener: () => jest.fn(),
Expand Down Expand Up @@ -212,7 +213,6 @@ function ReportScreenWrapper(props: ReportScreenWrapperProps) {

const report = {...createRandomReport(1), policyID: '1'};
const reportActions = ReportTestUtils.getMockedReportActionsMap(1000);
const mockRoute = {params: {reportID: '1', reportActionID: ''}, key: 'Report', name: 'Report' as const};

test('[ReportScreen] should render ReportScreen', async () => {
const {addListener} = createAddListenerMock();
Expand Down
Loading