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

Jump to bottom

Fix "New messages" banner shows up every time sending a message in a chat #44724

Merged
merged 18 commits into from
Jul 16, 2024
Merged

Fix "New messages" banner shows up every time sending a message in a chat #44724

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
eebf281
replace withOnyx with useOnyx
bernhardoj Jun 22, 2024
391e4f6
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 2, 2024
4e24f27
replace withOnyx with useOnyx hook
bernhardoj Jul 2, 2024
57edf6c
reuse variable
bernhardoj Jul 2, 2024
fc6420e
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 2, 2024
2762037
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 3, 2024
7a29255
update test
bernhardoj Jul 3, 2024
8c427a2
only clears the referrer when it's open from notification
bernhardoj Jul 3, 2024
8acb474
prettier
bernhardoj Jul 3, 2024
1d6de85
fix extra render
bernhardoj Jul 4, 2024
f93774e
prettier
bernhardoj Jul 4, 2024
d0fddfe
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 5, 2024
62cf228
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 8, 2024
0608ee8
prettier
bernhardoj Jul 8, 2024
126d3ce
update test
bernhardoj Jul 8, 2024
76d15b5
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 8, 2024
f7ec477
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 11, 2024
c923fac
Merge branch 'main' into fix/43486-new-message-shows-every-time-add-n…
bernhardoj Jul 16, 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
8 changes: 4 additions & 4 deletions src/components/PopoverMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ function PopoverMenu({
const selectedItemIndex = useRef<number | null>(null);

const [currentMenuItems, setCurrentMenuItems] = useState(menuItems);
const [enteredSubMenuIndexes, setEnteredSubMenuIndexes] = useState<number[]>([]);
const [enteredSubMenuIndexes, setEnteredSubMenuIndexes] = useState<readonly number[]>(CONST.EMPTY_ARRAY);

const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({initialFocusedIndex: -1, maxIndex: currentMenuItems.length - 1, isActive: isVisible});

Expand Down Expand Up @@ -156,7 +156,7 @@ function PopoverMenu({
onPress={() => {
setCurrentMenuItems(previousMenuItems);
setFocusedIndex(-1);
enteredSubMenuIndexes.splice(-1);
setEnteredSubMenuIndexes(enteredSubMenuIndexes.slice(0, -1));
}}
/>
);
Expand Down Expand Up @@ -186,7 +186,7 @@ function PopoverMenu({
if (menuItems.length === 0) {
return;
}
setEnteredSubMenuIndexes([]);
setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY);
setCurrentMenuItems(menuItems);
}, [menuItems]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the reassure extra render count. This effect is run on mount and on menuItems prop update. enteredSubMenuIndexes initial value is [] and we update it to [] which has different references. Using CONST.EMPTY_ARRAY so it has stable references.


Expand All @@ -197,7 +197,7 @@ function PopoverMenu({
anchorAlignment={anchorAlignment}
onClose={() => {
setCurrentMenuItems(menuItems);
setEnteredSubMenuIndexes([]);
setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY);
onClose();
}}
isVisible={isVisible}
Expand Down
137 changes: 26 additions & 111 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import lodashIsEqual from 'lodash/isEqual';
import React, {memo, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState} from 'react';
import type {FlatList, ViewStyle} from 'react-native';
import {InteractionManager, View} from 'react-native';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import {useOnyx, withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import {useOnyx} from 'react-native-onyx';
import Banner from '@components/Banner';
import BlockingView from '@components/BlockingViews/BlockingView';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
Expand Down Expand Up @@ -58,34 +58,9 @@ import ReportFooter from './report/ReportFooter';
import type {ActionListContextType, ReactionListRef, ScrollPosition} from './ReportScreenContext';
import {ActionListContext, ReactionListContext} from './ReportScreenContext';

type ReportScreenOnyxProps = {
/** Tells us if the sidebar has rendered */
isSidebarLoaded: OnyxEntry<boolean>;

/** Beta features list */
betas: OnyxEntry<OnyxTypes.Beta[]>;

/** The policies which the user has access to */
policies: OnyxCollection<OnyxTypes.Policy>;

/** An array containing all report actions related to this report, sorted based on a date criterion */
sortedAllReportActions: OnyxTypes.ReportAction[];

/** Additional report details */
reportNameValuePairs: OnyxEntry<OnyxTypes.ReportNameValuePairs>;

/** The report metadata loading states */
reportMetadata: OnyxEntry<OnyxTypes.ReportMetadata>;
};

type OnyxHOCProps = {
/** Onyx function that marks the component ready for hydration */
markReadyForHydration?: () => void;
};

type ReportScreenNavigationProps = StackScreenProps<AuthScreensParamList, typeof SCREENS.REPORT>;

type ReportScreenProps = OnyxHOCProps & CurrentReportIDContextValue & ReportScreenOnyxProps & ReportScreenNavigationProps;
type ReportScreenProps = CurrentReportIDContextValue & ReportScreenNavigationProps;

/** Get the currently viewed report ID as number */
function getReportID(route: ReportScreenNavigationProps['route']): string {
Expand Down Expand Up @@ -115,24 +90,7 @@ function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportAc
return parentReportActions[parentReportActionID ?? '0'];
}

function ReportScreen({
betas = [],
route,
reportNameValuePairs,
sortedAllReportActions,
reportMetadata = {
isLoadingInitialReportActions: true,
isLoadingOlderReportActions: false,
hasLoadingOlderReportActionsError: false,
isLoadingNewerReportActions: false,
hasLoadingNewerReportActionsError: false,
},
markReadyForHydration,
policies = {},
isSidebarLoaded = false,
currentReportID = '',
navigation,
}: ReportScreenProps) {
function ReportScreen({route, currentReportID = '', navigation}: ReportScreenProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const reportIDFromRoute = getReportID(route);
Expand All @@ -148,15 +106,32 @@ function ReportScreen({
const shouldUseNarrowLayout = isSmallScreenWidth || isReportOpenInRHP;

const [modal] = useOnyx(ONYXKEYS.MODAL);
const [isComposerFullSize] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${getReportID(route)}`, {initialValue: false});
const [isComposerFullSize] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${reportIDFromRoute}`, {initialValue: false});
const [accountManagerReportID] = useOnyx(ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID, {initialValue: ''});
const [userLeavingStatus] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_USER_IS_LEAVING_ROOM}${getReportID(route)}`, {initialValue: false});
const [reportOnyx, reportResult] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`, {allowStaleData: true});
const [userLeavingStatus] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_USER_IS_LEAVING_ROOM}${reportIDFromRoute}`, {initialValue: false});
const [reportOnyx, reportResult] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportIDFromRoute}`, {allowStaleData: true});
const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportIDFromRoute}`, {allowStaleData: true});
const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportIDFromRoute}`, {
initialValue: {
isLoadingInitialReportActions: true,
isLoadingOlderReportActions: false,
hasLoadingOlderReportActionsError: false,
isLoadingNewerReportActions: false,
hasLoadingNewerReportActionsError: false,
},
});
const [isSidebarLoaded] = useOnyx(ONYXKEYS.IS_SIDEBAR_LOADED, {initialValue: false});
const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {allowStaleData: true, initialValue: {}});
const [betas] = useOnyx(ONYXKEYS.BETAS);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const [parentReportAction] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportOnyx?.parentReportID || 0}`, {
canEvict: false,
selector: (parentReportActions) => getParentReportAction(parentReportActions, reportOnyx?.parentReportActionID ?? ''),
});
const [sortedAllReportActions = []] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportIDFromRoute}`, {
canEvict: false,
selector: (allReportActions) => ReportActionsUtils.getSortedReportActionsForDisplay(allReportActions, true),
});
const [isLoadingApp] = useOnyx(ONYXKEYS.IS_LOADING_APP);
const wasLoadingApp = usePrevious(isLoadingApp);
const finishedLoadingApp = wasLoadingApp && !isLoadingApp;
Expand Down Expand Up @@ -643,15 +618,6 @@ function ReportScreen({
};
}, [report, didSubscribeToReportLeavingEvents, reportIDFromRoute]);

const onListLayout = useCallback(() => {
if (!markReadyForHydration) {
return;
}

markReadyForHydration();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const actionListValue = useMemo((): ActionListContextType => ({flatListRef, scrollPosition, setScrollPosition}), [flatListRef, scrollPosition, setScrollPosition]);

// This helps in tracking from the moment 'route' triggers useMemo until isLoadingInitialReportActions becomes true. It prevents blinking when loading reportActions from cache.
Expand Down Expand Up @@ -767,10 +733,7 @@ function ReportScreen({
/>
)}
<DragAndDropProvider isDisabled={!isCurrentReportLoadedFromOnyx || !ReportUtils.canUserPerformWriteAction(report)}>
<View
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]}
onLayout={onListLayout}
>
<View style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]}>
{shouldShowReportActionList && (
<ReportActionsView
reportActions={reportActions}
Expand Down Expand Up @@ -816,52 +779,4 @@ function ReportScreen({
}

ReportScreen.displayName = 'ReportScreen';

export default withCurrentReportID(
withOnyx<ReportScreenProps, ReportScreenOnyxProps>(
{
isSidebarLoaded: {
key: ONYXKEYS.IS_SIDEBAR_LOADED,
},
sortedAllReportActions: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`,
canEvict: false,
selector: (allReportActions: OnyxEntry<OnyxTypes.ReportActions>) => ReportActionsUtils.getSortedReportActionsForDisplay(allReportActions, true),
},
reportNameValuePairs: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${getReportID(route)}`,
allowStaleData: true,
},
reportMetadata: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_METADATA}${getReportID(route)}`,
initialValue: {
isLoadingInitialReportActions: true,
isLoadingOlderReportActions: false,
hasLoadingOlderReportActionsError: false,
isLoadingNewerReportActions: false,
hasLoadingNewerReportActionsError: false,
},
},
betas: {
key: ONYXKEYS.BETAS,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
allowStaleData: true,
},
},
true,
)(
memo(
ReportScreen,
(prevProps, nextProps) =>
prevProps.isSidebarLoaded === nextProps.isSidebarLoaded &&
lodashIsEqual(prevProps.sortedAllReportActions, nextProps.sortedAllReportActions) &&
lodashIsEqual(prevProps.reportMetadata, nextProps.reportMetadata) &&
lodashIsEqual(prevProps.betas, nextProps.betas) &&
lodashIsEqual(prevProps.policies, nextProps.policies) &&
prevProps.currentReportID === nextProps.currentReportID &&
lodashIsEqual(prevProps.route, nextProps.route),
),
),
);
export default withCurrentReportID(memo(ReportScreen, (prevProps, nextProps) => prevProps.currentReportID === nextProps.currentReportID && lodashIsEqual(prevProps.route, nextProps.route)));
4 changes: 3 additions & 1 deletion src/pages/home/report/ReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,9 @@ function ReportActionsList({
const isFromNotification = route?.params?.referrer === CONST.REFERRER.NOTIFICATION;
if ((Visibility.isVisible() || isFromNotification) && scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD) {
Report.readNewestAction(report.reportID);
Navigation.setParams({referrer: undefined});
if (isFromNotification) {
Navigation.setParams({referrer: undefined});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also fixes the test of UnreadIndicatorsTest. When the test runner opens the report screen, if it's unread (which is true), it will call both functions above to mark the report as read and remove the referrer param.

When the referrer param is updated, the report screen is re-mounted (on test only), but the readNewestAction optimisticData isn't completed yet and triggers the above logic again, repeatedly, infinitely.

This doesn't happen when we still use withOnyx, specifically the reportMetadata. Using withOnyx, when the report screen is re-mounted, it's not immediately rendered because reportMetadata has a pending merge, giving optimisticData of readNewestAction time to complete.

I think the best solution is to clear the referrer param after the readNewestAction optimisticData is applied, but I'm not sure how we can do that without using a timeout.

Given the re-mount behavior when updating param is only occurs on test, I updated the code to only clears it when referrer exists.

} else {
readActionSkipped.current = true;
}
Expand Down
16 changes: 16 additions & 0 deletions tests/ui/UnreadIndicatorsTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,22 @@ describe('Unread Indicators', () => {

// Leave a comment as the current user and verify the indicator is removed
Report.addComment(REPORT_ID, 'Current User Comment 1');
const newActionID: string = await new Promise((resolve) => {
const connectionID = Onyx.connect({
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
callback: (reportActions) => {
Onyx.disconnect(connectionID);
const sortedActions = Object.values(reportActions ?? {}).toSorted((a, b) => (a.created < b.created ? -1 : 1));
resolve(sortedActions.pop()?.reportActionID ?? '');
},
});
});
expect(newActionID).toBeTruthy();
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, {
[newActionID]: {
previousReportActionID: '9',
},
});
bernhardoj marked this conversation as resolved.
Show resolved Hide resolved
return waitForBatchedUpdates();
})
.then(() => {
Expand Down
Loading