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

Handle pagination errors in chats #40610

Merged
merged 4 commits into from
May 13, 2024
Merged

Handle pagination errors in chats #40610

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c95398c
Handle pagination errors in chats
janicduplessis Apr 19, 2024
ff7c706
Merge remote-tracking branch 'upstream/main' into @janic/pagination-e…
janicduplessis May 6, 2024
47be44a
Fix incorrect change
janicduplessis May 6, 2024
350c7e2
Always render header / footer on error, address review comments
janicduplessis May 9, 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
4 changes: 4 additions & 0 deletions src/languages/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2938,4 +2938,8 @@ export default {
offline:
"You appear to be offline. Unfortunately, Expensify Classic doesn't work offline, but New Expensify does. If you prefer to use Expensify Classic, try again when you have an internet connection.",
},
listBoundary: {
errorMessage: 'There was an error loading more messages.',
tryAgain: 'Try again',
},
} satisfies TranslationBase;
4 changes: 4 additions & 0 deletions src/languages/es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3434,4 +3434,8 @@ export default {
offline:
'Parece que estás desconectado. Desafortunadamente, Expensify Classic no funciona sin conexión, pero New Expensify sí. Si prefieres utilizar Expensify Classic, inténtalo de nuevo cuando tengas conexión a internet.',
},
listBoundary: {
errorMessage: 'Se produjo un error al cargar más mensajes.',
tryAgain: 'Inténtalo de nuevo',
},
} satisfies EnglishTranslation;
2 changes: 1 addition & 1 deletion src/libs/API/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ const WRITE_COMMANDS = {
ACCEPT_JOIN_REQUEST: 'AcceptJoinRequest',
DECLINE_JOIN_REQUEST: 'DeclineJoinRequest',
CREATE_POLICY_TAX: 'CreatePolicyTax',
UPDATE_POLICY_CONNECTION_CONFIG: 'UpdatePolicyConnectionConfiguration',
UPDATE_POLICY_CONNECTION_CONFIG: 'UpdatePolicyConnectionConfig',
REMOVE_POLICY_CONNECTION: 'RemovePolicyConnection',
SET_POLICY_TAXES_ENABLED: 'SetPolicyTaxesEnabled',
DELETE_POLICY_TAXES: 'DeletePolicyTaxes',
Expand Down
6 changes: 6 additions & 0 deletions src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,9 @@ function openReport(
value: {
isLoadingInitialReportActions: true,
isLoadingOlderReportActions: false,
hasLoadingOlderReportActionsError: false,
isLoadingNewerReportActions: false,
hasLoadingNewerReportActionsError: false,
lastVisitTime: DateUtils.getDBTime(),
},
},
Expand Down Expand Up @@ -988,6 +990,7 @@ function getOlderActions(reportID: string, reportActionID: string) {
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingOlderReportActions: true,
hasLoadingOlderReportActionsError: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

And we may want to do the same for getOlderActions here

},
},
];
Expand All @@ -1008,6 +1011,7 @@ function getOlderActions(reportID: string, reportActionID: string) {
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingOlderReportActions: false,
hasLoadingOlderReportActionsError: true,
},
},
];
Expand All @@ -1031,6 +1035,7 @@ function getNewerActions(reportID: string, reportActionID: string) {
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingNewerReportActions: true,
hasLoadingNewerReportActionsError: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

We can prevent setting hasLoadingNewerReportActionsError to false optimistically on retry of the failed request. Since we are forcefully getting newer actions on retry, we can do something like this here:
...(!force ? {hasLoadingNewerReportActionsError: false} : {}),
And when the API request succeeds, we can set it to false in successData

},
},
];
Expand All @@ -1051,6 +1056,7 @@ function getNewerActions(reportID: string, reportActionID: string) {
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingNewerReportActions: false,
hasLoadingNewerReportActionsError: true,
},
},
];
Expand Down
4 changes: 4 additions & 0 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,9 @@ function ReportScreen({
reportMetadata = {
isLoadingInitialReportActions: true,
isLoadingOlderReportActions: false,
hasLoadingOlderReportActionsError: false,
isLoadingNewerReportActions: false,
hasLoadingNewerReportActionsError: false,
janicduplessis marked this conversation as resolved.
Show resolved Hide resolved
},
parentReportActions,
accountManagerReportID,
Expand Down Expand Up @@ -696,7 +698,9 @@ function ReportScreen({
parentReportAction={parentReportAction}
isLoadingInitialReportActions={reportMetadata?.isLoadingInitialReportActions}
isLoadingNewerReportActions={reportMetadata?.isLoadingNewerReportActions}
hasLoadingNewerReportActionsError={reportMetadata?.hasLoadingNewerReportActionsError}
isLoadingOlderReportActions={reportMetadata?.isLoadingOlderReportActions}
hasLoadingOlderReportActionsError={reportMetadata?.hasLoadingOlderReportActionsError}
isReadyForCommentLinking={!shouldShowSkeleton}
transactionThreadReportID={transactionThreadReportID}
/>
Expand Down
49 changes: 47 additions & 2 deletions src/pages/home/report/ListBoundaryLoader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import React from 'react';
import React, {useEffect} from 'react';
import {ActivityIndicator, View} from 'react-native';
import type {ValueOf} from 'type-fest';
import Button from '@components/Button';
import ReportActionsSkeletonView from '@components/ReportActionsSkeletonView';
import Text from '@components/Text';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -22,6 +25,12 @@ type ListBoundaryLoaderProps = {

/** Name of the last report action */
lastReportActionName?: string;

/** Shows if there was an error when loading report actions */
hasError?: boolean;

/** Function to retry if there was an error */
onRetry?: () => void;
};

function ListBoundaryLoader({
Expand All @@ -30,11 +39,47 @@ function ListBoundaryLoader({
isLoadingInitialReportActions = false,
lastReportActionName = '',
isLoadingNewerReportActions = false,
hasError = false,
onRetry,
}: ListBoundaryLoaderProps) {
const theme = useTheme();
const styles = useThemeStyles();
const {isOffline} = useNetwork();
const {translate} = useLocalize();

// When retrying we want to show the loading state in the retry button so we
// have this separate state to handle that.
const [isRetrying, setIsRetrying] = React.useState(false);

const retry = () => {
setIsRetrying(true);
onRetry?.();
};

// Reset the retrying state once loading is done.
useEffect(() => {
if (isLoadingNewerReportActions || isLoadingOlderReportActions) {
return;
}

setIsRetrying(false);
}, [isLoadingNewerReportActions, isLoadingOlderReportActions]);

if (hasError || isRetrying) {
return (
<View style={[styles.alignItemsCenter, styles.justifyContentCenter, styles.listBoundaryError]}>
<Text style={styles.listBoundaryErrorText}>{translate('listBoundary.errorMessage')}</Text>
{!isOffline && (
<Button
small
onPress={retry}
text={translate('listBoundary.tryAgain')}
isLoading={isRetrying}
/>
)}
</View>
);
}
// We use two different loading components for the header and footer
// to reduce the jumping effect when the user is scrolling to the newer report actions
if (type === CONST.LIST_COMPONENTS.FOOTER) {
Expand All @@ -55,7 +100,7 @@ function ListBoundaryLoader({
// applied for a header of the list, i.e. when you scroll to the bottom of the list
// the styles for android and the rest components are different that's why we use two different components
return (
<View style={[styles.alignItemsCenter, styles.justifyContentCenter, styles.chatBottomLoader]}>
<View style={[styles.alignItemsCenter, styles.justifyContentCenter, styles.listBoundaryLoader]}>
<ActivityIndicator
color={theme.spinner}
size="small"
Expand Down
44 changes: 36 additions & 8 deletions src/pages/home/report/ReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,20 +65,26 @@ type ReportActionsListProps = WithCurrentUserPersonalDetailsProps & {
/** Are we loading more report actions? */
isLoadingOlderReportActions?: boolean;

/** Was there an error when loading older report actions? */
hasLoadingOlderReportActionsError?: boolean;

/** Are we loading newer report actions? */
isLoadingNewerReportActions?: boolean;

/** Was there an error when loading newer report actions? */
hasLoadingNewerReportActionsError?: boolean;

/** Callback executed on list layout */
onLayout: (event: LayoutChangeEvent) => void;

/** Callback executed on scroll */
onScroll?: (event: NativeSyntheticEvent<NativeScrollEvent>) => void;

/** Function to load more chats */
loadOlderChats: () => void;
loadOlderChats: (force?: boolean) => void;

/** Function to load newer chats */
loadNewerChats: () => void;
loadNewerChats: (force?: boolean) => void;

/** Whether the composer is in full size */
isComposerFullSize?: boolean;
Expand Down Expand Up @@ -139,7 +145,9 @@ function ReportActionsList({
parentReportAction,
isLoadingInitialReportActions = false,
isLoadingOlderReportActions = false,
hasLoadingOlderReportActionsError = false,
isLoadingNewerReportActions = false,
hasLoadingNewerReportActionsError = false,
sortedReportActions,
onScroll,
mostRecentIOUReportActionID = '',
Expand Down Expand Up @@ -569,7 +577,11 @@ function ReportActionsList({

const lastReportAction: OnyxTypes.ReportAction | EmptyObject = useMemo(() => sortedReportActions.at(-1) ?? {}, [sortedReportActions]);

const listFooterComponent = useCallback(() => {
const retryLoadOlderChatsError = useCallback(() => {
loadOlderChats(true);
}, [loadOlderChats]);

const listFooterComponent = useMemo(() => {
// Skip this hook on the first render (when online), as we are not sure if more actions are going to be loaded,
// Therefore showing the skeleton on footer might be misleading.
// When offline, there should be no second render, so we should show the skeleton if the corresponding loading prop is present
Expand All @@ -584,9 +596,11 @@ function ReportActionsList({
isLoadingOlderReportActions={isLoadingOlderReportActions}
isLoadingInitialReportActions={isLoadingInitialReportActions}
lastReportActionName={lastReportAction.actionName}
hasError={hasLoadingOlderReportActionsError}
onRetry={retryLoadOlderChatsError}
/>
);
}, [isLoadingInitialReportActions, isLoadingOlderReportActions, lastReportAction.actionName, isOffline]);
}, [isLoadingInitialReportActions, isLoadingOlderReportActions, lastReportAction.actionName, isOffline, hasLoadingOlderReportActionsError, retryLoadOlderChatsError]);

const onLayoutInner = useCallback(
(event: LayoutChangeEvent) => {
Expand All @@ -601,7 +615,11 @@ function ReportActionsList({
[onContentSizeChange],
);

const listHeaderComponent = useCallback(() => {
const retryLoadNewerChatsError = useCallback(() => {
loadNewerChats(true);
}, [loadNewerChats]);

const listHeaderComponent = useMemo(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this so we pass a react element instead of a function. When passing a function it causes the component to be unmounted and remounted every time one of the dependencies of useCallback changes. This caused flickers in the loading component as well as resetting any internal state so this fixes it.

if (!canShowHeader) {
hasHeaderRendered.current = true;
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

When GetNewerActions API fails, we set hasLoadingNewerReportActionsError to true. But there is a possibility that the error UI is not displayed if there is an early return here due to unfavorable canShowHeader. And I think without the error UI we may not be able to retry GetNewerActions again during the ongoing session. Here is a test video demonstrating the same.

40610-newer-actions-issue.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am curious about what this canShowHeader is for, meant to raise this in the review but probably forgot.

Maybe I should just ignore canShowHeader if there’s an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

am curious about what this canShowHeader is for, meant to raise this in the review but probably forgot.

I think the same reasoning for listFooterComponent here holds good for listHeaderComponent. But, canShowHeader also considers scrollingVerticalOffset here which seems to make this more complicated.

Maybe I should just ignore canShowHeader if there’s an error?

Hmm. Interesting. If there is an error then we may as well show the error UI even during first render.
Also, we may want to do the same thing for ListFooterComponent here.
@Gonals What are your thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if there is a bug with that logic but when testing comment linking I never see the loading indicator for newer messages, maybe I should make an issue for that?. Anyway I think it makes sense to always render the header / footer state if there's an error.

Expand All @@ -611,9 +629,19 @@ function ReportActionsList({
<ListBoundaryLoader
type={CONST.LIST_COMPONENTS.HEADER}
isLoadingNewerReportActions={isLoadingNewerReportActions}
hasError={hasLoadingNewerReportActionsError}
onRetry={retryLoadNewerChatsError}
/>
);
}, [isLoadingNewerReportActions, canShowHeader]);
}, [isLoadingNewerReportActions, canShowHeader, hasLoadingNewerReportActionsError, retryLoadNewerChatsError]);

const onStartReached = useCallback(() => {
loadNewerChats(false);
}, [loadNewerChats]);

const onEndReached = useCallback(() => {
loadOlderChats(false);
}, [loadOlderChats]);

// When performing comment linking, initially 25 items are added to the list. Subsequent fetches add 15 items from the cache or 50 items from the server.
// This is to ensure that the user is able to see the 'scroll to newer comments' button when they do comment linking and have not reached the end of the list yet.
Expand All @@ -635,9 +663,9 @@ function ReportActionsList({
contentContainerStyle={contentContainerStyle}
keyExtractor={keyExtractor}
initialNumToRender={initialNumToRender}
onEndReached={loadOlderChats}
onEndReached={onEndReached}
onEndReachedThreshold={0.75}
onStartReached={loadNewerChats}
onStartReached={onStartReached}
onStartReachedThreshold={0.75}
ListFooterComponent={listFooterComponent}
ListHeaderComponent={listHeaderComponent}
Expand Down
Loading
Loading