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

Revert "Fix the loading skeleton behavior when opening a report" #45914

Closed
wants to merge 1 commit into from
Closed

Revert "Fix the loading skeleton behavior when opening a report" #45914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,6 @@ const CONST = {
MEMBER: 'member',
},
MAX_COUNT_BEFORE_FOCUS_UPDATE: 30,
MIN_INITIAL_REPORT_ACTION_COUNT: 15,
SPLIT_REPORTID: '-2',
ACTIONS: {
LIMIT: 50,
Expand Down
1 change: 0 additions & 1 deletion src/hooks/usePaginatedReportActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ function usePaginatedReportActions(reportID?: string, reportActionID?: string) {
return {
reportActions,
linkedAction,
sortedAllReportActions,
};
}

Expand Down
65 changes: 14 additions & 51 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ function ReportScreen({
const [isLinkingToMessage, setIsLinkingToMessage] = useState(!!reportActionIDFromRoute);

const [currentUserAccountID = -1] = useOnyx(ONYXKEYS.SESSION, {selector: (value) => value?.accountID});
const {reportActions, linkedAction, sortedAllReportActions} = usePaginatedReportActions(report.reportID, reportActionIDFromRoute);
const {reportActions, linkedAction} = usePaginatedReportActions(report.reportID, reportActionIDFromRoute);

// Define here because reportActions are recalculated before mount, allowing data to display faster than useEffect can trigger.
// If we have cached reportActions, they will be shown immediately.
Expand All @@ -310,19 +310,6 @@ function ReportScreen({
const screenWrapperStyle: ViewStyle[] = [styles.appContent, styles.flex1, {marginTop: viewportOffsetTop}];
const isEmptyChat = useMemo(() => ReportUtils.isEmptyReport(report), [report]);
const isOptimisticDelete = report.statusNum === CONST.REPORT.STATUS_NUM.CLOSED;
const indexOfLinkedMessage = useMemo(
(): number => reportActions.findIndex((obj) => String(obj.reportActionID) === String(reportActionIDFromRoute)),
[reportActions, reportActionIDFromRoute],
);

const isPendingActionExist = !!reportActions.at(0)?.pendingAction;
const doesCreatedActionExists = useCallback(() => !!sortedAllReportActions?.findLast((action) => ReportActionsUtils.isCreatedAction(action)), [sortedAllReportActions]);
const isLinkedMessageAvailable = useMemo(() => indexOfLinkedMessage > -1, [indexOfLinkedMessage]);

// The linked report actions should have at least 15 messages (counting as 1 page) above them to fill the screen.
// If the count is too high (equal to or exceeds the web pagination size / 50) and there are no cached messages in the report,
// OpenReport will be called each time the user scrolls up the report a bit, clicks on report preview, and then goes back."
const isLinkedMessagePageReady = isLinkedMessageAvailable && (reportActions.length - indexOfLinkedMessage >= CONST.REPORT.MIN_INITIAL_REPORT_ACTION_COUNT || doesCreatedActionExists());

// If there's a non-404 error for the report we should show it instead of blocking the screen
const hasHelpfulErrors = Object.keys(report?.errorFields ?? {}).some((key) => key !== 'notFound');
Expand Down Expand Up @@ -412,10 +399,6 @@ function ReportScreen({
return reportIDFromRoute !== '' && !!report.reportID && !isTransitioning;
}, [report, reportIDFromRoute]);

const isInitialPageReady = isOffline
? reportActions.length > 0
: reportActions.length >= CONST.REPORT.MIN_INITIAL_REPORT_ACTION_COUNT || isPendingActionExist || (doesCreatedActionExists() && reportActions.length > 0);

/**
* Using logical OR operator because with nullish coalescing operator, when `isLoadingApp` is false, the right hand side of the operator
* is not evaluated. This causes issues where we have `isLoading` set to false and later set to true and then set to false again.
Expand All @@ -424,13 +407,13 @@ function ReportScreen({
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const isLoading = isLoadingApp || !reportIDFromRoute || (!isSidebarLoaded && !isReportOpenInRHP) || PersonalDetailsUtils.isPersonalDetailsEmpty();
const shouldShowSkeleton =
(isLinkingToMessage && !isLinkedMessagePageReady) ||
(!!reportActionIDFromRoute && !!reportMetadata?.isLoadingInitialReportActions) ||
(!isLinkingToMessage && !isInitialPageReady) ||
isLoadingReportOnyx ||
!isCurrentReportLoadedFromOnyx ||
isLoading;

!linkedAction &&
(isLinkingToMessage ||
!isCurrentReportLoadedFromOnyx ||
(reportActions.length === 0 && !!reportMetadata?.isLoadingInitialReportActions) ||
isLoading ||
(!!reportActionIDFromRoute && reportMetadata?.isLoadingInitialReportActions));
const shouldShowReportActionList = isCurrentReportLoadedFromOnyx && !isLoading;
const currentReportIDFormRoute = route.params?.reportID;

// eslint-disable-next-line rulesdir/no-negated-variables
Expand All @@ -453,6 +436,7 @@ function ReportScreen({
if (shouldHideReport) {
return true;
}

return !!currentReportIDFormRoute && !ReportUtils.isValidReportIDFromPath(currentReportIDFormRoute);
}, [isLoadingApp, finishedLoadingApp, report.reportID, isOptimisticDelete, reportMetadata?.isLoadingInitialReportActions, userLeavingStatus, shouldHideReport, currentReportIDFormRoute]);

Expand Down Expand Up @@ -486,12 +470,12 @@ function ReportScreen({
return;
}

if (!shouldFetchReport(report) && (isInitialPageReady || isLinkedMessagePageReady)) {
if (!shouldFetchReport(report)) {
return;
}

fetchReport();
}, [report, fetchReport, reportIDFromRoute, isLoadingApp, isInitialPageReady, isLinkedMessagePageReady]);
}, [report, fetchReport, reportIDFromRoute, isLoadingApp]);

const dismissBanner = useCallback(() => {
setIsBannerVisible(false);
Expand Down Expand Up @@ -536,26 +520,14 @@ function ReportScreen({

useEffect(() => {
// Call OpenReport only if we are not linking to a message or the report is not available yet
if (isLoadingReportOnyx || (reportActionIDFromRoute && report.reportID && isLinkedMessagePageReady)) {
if (isLoadingReportOnyx || (reportActionIDFromRoute && report.reportID)) {
return;
}

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

useEffect(() => {
if (isLoadingReportOnyx || !reportActionIDFromRoute || isLinkedMessagePageReady) {
return;
}

// This function is triggered when a user clicks on a link to navigate to a report.
// For each link click, we retrieve the report data again, even though it may already be cached.
// There should be only one openReport execution per page start or navigating
fetchReportIfNeeded();
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [route, isLinkedMessagePageReady, isLoadingReportOnyx, reportActionIDFromRoute]);

// If a user has chosen to leave a thread, and then returns to it (e.g. with the back button), we need to call `openReport` again in order to allow the user to rejoin and to receive real-time updates
useEffect(() => {
if (!shouldUseNarrowLayout || !isFocused || prevIsFocused || !ReportUtils.isChatThread(report) || report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
Expand Down Expand Up @@ -733,16 +705,7 @@ function ReportScreen({
Report.readNewestAction(report.reportID);
}, [report]);

if (
(!isLinkedActionInaccessibleWhisper && isLinkedActionDeleted) ||
(shouldShowSkeleton &&
!reportMetadata.isLoadingInitialReportActions &&
reportActionIDFromRoute &&
sortedAllReportActions &&
sortedAllReportActions?.length > 0 &&
reportActions.length === 0 &&
!isLinkingToMessage)
) {
if ((!isLinkedActionInaccessibleWhisper && isLinkedActionDeleted) ?? (!shouldShowSkeleton && reportActionIDFromRoute && reportActions?.length === 0 && !isLinkingToMessage)) {
return (
<BlockingView
icon={Illustrations.ToddBehindCloud}
Expand Down Expand Up @@ -805,7 +768,7 @@ function ReportScreen({
onLayout={onListLayout}
testID="report-actions-view-wrapper"
>
{!shouldShowSkeleton && (
{shouldShowReportActionList && (
<ReportActionsView
reportActions={reportActions}
report={report}
Expand Down
2 changes: 1 addition & 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 @@ -220,7 +220,7 @@ function ReportActionsList({
const previousLastIndex = useRef(lastActionIndex);

const isLastPendingActionIsDelete = sortedReportActions?.[0]?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
const linkedReportActionID = route?.params?.reportActionID ?? '-1';
const linkedReportActionID = route.params?.reportActionID ?? '-1';

// This state is used to force a re-render when the user manually marks a message as unread
// by using a timestamp you can force re-renders without having to worry about if another message was marked as unread before
Expand Down
12 changes: 12 additions & 0 deletions src/pages/home/report/ReportActionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,18 @@ function ReportActionsView({
const oldestReportAction = useMemo(() => reportActions?.at(-1), [reportActions]);
const hasCreatedAction = oldestReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED;

useEffect(() => {
if (!reportActionID || indexOfLinkedAction > -1) {
return;
}

// This function is triggered when a user clicks on a link to navigate to a report.
// For each link click, we retrieve the report data again, even though it may already be cached.
// There should be only one openReport execution per page start or navigating
Report.openReport(reportID, reportActionID);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [route, indexOfLinkedAction]);

useEffect(() => {
const wasLoginChangedDetected = prevAuthTokenType === CONST.AUTH_TOKEN_TYPES.ANONYMOUS && !session?.authTokenType;
if (wasLoginChangedDetected && didUserLogInDuringSession() && isUserCreatedPolicyRoom(report)) {
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/PaginationTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,13 +281,13 @@ describe('Pagination', () => {
});

it('opens a chat and load older messages', async () => {
mockOpenReport(CONST.REPORT.MIN_INITIAL_REPORT_ACTION_COUNT, '18');
mockOpenReport(5, '8');
mockGetOlderActions(5);

await signInAndGetApp();
await navigateToSidebarOption(REPORT_ID);

expect(getReportActions()).toHaveLength(CONST.REPORT.MIN_INITIAL_REPORT_ACTION_COUNT);
expect(getReportActions()).toHaveLength(5);
TestHelper.expectAPICommandToHaveBeenCalled('OpenReport', 1);
TestHelper.expectAPICommandToHaveBeenCalledWith('OpenReport', 0, {reportID: REPORT_ID, reportActionID: ''});
TestHelper.expectAPICommandToHaveBeenCalled('GetOlderActions', 0);
Expand All @@ -304,9 +304,9 @@ describe('Pagination', () => {

await waitForBatchedUpdatesWithAct();

// We now have 18 messages. 15 (MIN_INITIAL_REPORT_ACTION_COUNT) from the initial OpenReport and 3 from GetOlderActions.
// We now have 8 messages. 5 from the initial OpenReport and 3 from GetOlderActions.
// GetOlderActions only returns 3 actions since it reaches id '1', which is the created action.
expect(getReportActions()).toHaveLength(18);
expect(getReportActions()).toHaveLength(8);
});

// Currently broken on main by https://github.com/Expensify/App/pull/42582.
Expand Down
Loading