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 loading skeleton appears in the workspace expense thread after sending an IOU expense request #46864

Merged
merged 5 commits into from
Aug 9, 2024
Merged

Fix loading skeleton appears in the workspace expense thread after sending an IOU expense request #46864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb048dc
Fix ws report chat blank
tsa321 Aug 4, 2024
d52cc1b
Merge branch 'main' into blankRaList
tsa321 Aug 6, 2024
3f24ce0
Fix lint error
tsa321 Aug 6, 2024
3d1d57a
Fix newer messages are not loaded after click comment link
tsa321 Aug 8, 2024
3ffeeb4
Revert some changes
tsa321 Aug 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
8 changes: 1 addition & 7 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,12 +402,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const isLoading = isLoadingApp || !reportIDFromRoute || (!isSidebarLoaded && !isInNarrowPaneModal) || PersonalDetailsUtils.isPersonalDetailsEmpty();
const shouldShowSkeleton =
(isLinkingToMessage && !isLinkedMessagePageReady) ||
(!!reportActionIDFromRoute && !!reportMetadata?.isLoadingInitialReportActions) ||
(!isLinkingToMessage && !isInitialPageReady) ||
isLoadingReportOnyx ||
!isCurrentReportLoadedFromOnyx ||
isLoading;
(isLinkingToMessage && !isLinkedMessagePageReady) || (!isLinkingToMessage && !isInitialPageReady) || isLoadingReportOnyx || !isCurrentReportLoadedFromOnyx || isLoading;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain a bit more why you made this change? I see you mentioned it here, but since it was never part of your proposal, I'd like to get a better understanding of why the change is needed.

Copy link
Contributor Author

@tsa321 tsa321 Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jjcoffee I remove this (!!reportActionIDFromRoute && !!reportMetadata?.isLoadingInitialReportActions) || condition.

After the changes made here in my previous PR:

App/src/pages/home/ReportScreen.tsx

Lines 781 to 782 in 934797e

{!shouldShowSkeleton && (
<ReportActionsView

the blank list will no longer be displayed; instead, the loading skeleton will appear. I updated the code so that the ReportActionsView depends on shouldShowSkeleton rather than shouldShowReportActionsList. The previous code can be found here:

App/src/pages/home/ReportScreen.tsx

Lines 748 to 766 in 7557e45

{shouldShowReportActionList && (
<ReportActionsView
reportActions={reportActions}
report={report}
parentReportAction={parentReportAction}
isLoadingInitialReportActions={reportMetadata?.isLoadingInitialReportActions}
isLoadingNewerReportActions={reportMetadata?.isLoadingNewerReportActions}
hasLoadingNewerReportActionsError={reportMetadata?.hasLoadingNewerReportActionsError}
isLoadingOlderReportActions={reportMetadata?.isLoadingOlderReportActions}
hasLoadingOlderReportActionsError={reportMetadata?.hasLoadingOlderReportActionsError}
isReadyForCommentLinking={!shouldShowSkeleton}
transactionThreadReportID={transactionThreadReportID}
/>
)}
{/* Note: The ReportActionsSkeletonView should be allowed to mount even if the initial report actions are not loaded.
If we prevent rendering the report while they are loading then
we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */}
{shouldShowSkeleton && <ReportActionsSkeletonView />}

Before my previous PR, both shouldShowSkeleton and shouldShowReportActionsList were true, but ReportSkeletonView was rendered below ReportActionsView. This meant that ReportSkeletonView was hidden behind ReportActionsView. In my previous PR, I removed shouldShowReportActionsList and made the display of ReportActionsView depend on shouldShowSkeleton.


To verify you could try to reproduce the issue in staging. It will display loading skeleton instead of blank list.


// eslint-disable-next-line rulesdir/no-negated-variables
const shouldShowNotFoundLinkedAction =
Expand Down Expand Up @@ -788,7 +783,6 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
hasLoadingNewerReportActionsError={reportMetadata?.hasLoadingNewerReportActionsError}
isLoadingOlderReportActions={reportMetadata?.isLoadingOlderReportActions}
hasLoadingOlderReportActionsError={reportMetadata?.hasLoadingOlderReportActionsError}
isReadyForCommentLinking={!shouldShowSkeleton}
transactionThreadReportID={transactionThreadReportID}
/>
)}
Expand Down
16 changes: 4 additions & 12 deletions src/pages/home/report/ReportActionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ type ReportActionsViewProps = ReportActionsViewOnyxProps & {
/** There an error when loading newer report actions */
hasLoadingNewerReportActionsError?: boolean;

/** Whether the report is ready for comment linking */
isReadyForCommentLinking?: boolean;

/** The reportID of the transaction thread report associated with this current report, if any */
// eslint-disable-next-line react/no-unused-prop-types
transactionThreadReportID?: string | null;
Expand All @@ -96,7 +93,6 @@ function ReportActionsView({
hasLoadingOlderReportActionsError = false,
isLoadingNewerReportActions = false,
hasLoadingNewerReportActionsError = false,
isReadyForCommentLinking = false,
transactionThreadReportID,
}: ReportActionsViewProps) {
useCopySelectionHelper();
Expand All @@ -122,7 +118,6 @@ function ReportActionsView({
const [isNavigatingToLinkedMessage, setNavigatingToLinkedMessage] = useState(!!reportActionID);
const prevShouldUseNarrowLayoutRef = useRef(shouldUseNarrowLayout);
const reportID = report.reportID;
const isLoading = (!!reportActionID && isLoadingInitialReportActions) || !isReadyForCommentLinking;
const isReportFullyVisible = useMemo((): boolean => getIsReportFullyVisible(isFocused), [isFocused]);
const openReportIfNecessary = () => {
if (!shouldFetchReport(report)) {
Expand Down Expand Up @@ -156,7 +151,7 @@ function ReportActionsView({
listOldID = newID;
return newID;
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [route, isLoadingInitialReportActions, reportActionID]);
}, [route, reportActionID]);

// When we are offline before opening an IOU/Expense report,
// the total of the report and sometimes the expense aren't displayed because these actions aren't returned until `OpenReport` API is complete.
Expand Down Expand Up @@ -244,7 +239,7 @@ function ReportActionsView({
if (!reportActionID) {
return combinedReportActions;
}
if (isLoading || indexOfLinkedAction === -1) {
if (indexOfLinkedAction === -1) {
return [];
}

Expand All @@ -256,7 +251,7 @@ function ReportActionsView({

// currentReportActionID is needed to trigger batching once the report action has been positioned
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [reportActionID, combinedReportActions, indexOfLinkedAction, isLoading, currentReportActionID]);
}, [reportActionID, combinedReportActions, indexOfLinkedAction, currentReportActionID]);

const reportActionIDMap = useMemo(() => {
const reportActionIDs = allReportActions.map((action) => action.reportActionID);
Expand Down Expand Up @@ -404,7 +399,7 @@ function ReportActionsView({
!force &&
(!reportActionID ||
!isFocused ||
isLoadingInitialReportActions ||
(isLoadingInitialReportActions && !hasMoreCached) ||
isLoadingNewerReportActions ||
// If there was an error only try again once on initial mount. We should also still load
// more in case we have cached messages.
Expand Down Expand Up @@ -523,9 +518,6 @@ function arePropsEqual(oldProps: ReportActionsViewProps, newProps: ReportActions
return false;
}

if (!lodashIsEqual(oldProps.isReadyForCommentLinking, newProps.isReadyForCommentLinking)) {
return false;
}
if (!lodashIsEqual(oldProps.reportActions, newProps.reportActions)) {
return false;
}
Expand Down
Loading