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.

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: hide new marker for pending deleted action in online mode #33162

Merged
merged 10 commits into from
Jan 9, 2024
13 changes: 12 additions & 1 deletion src/libs/ReportActionsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,20 @@ function shouldReportActionBeVisible(reportAction: OnyxEntry<ReportAction>, key:

// All other actions are displayed except thread parents, deleted, or non-pending actions
const isDeleted = isDeletedAction(reportAction);
const isPending = !!reportAction.pendingAction && !(!isNetworkOffline && reportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE);
const isPending = !!reportAction.pendingAction;
return !isDeleted || isPending || isDeletedParentAction(reportAction) || isReversedTransaction(reportAction);
}

/**
* Checks if the new marker should be shown for the report action.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Checks if the new marker should be shown for the report action.
* Checks if the new marker should be hidden for the report action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

*/
function shouldHideNewMarker(reportAction: OnyxEntry<ReportAction>): boolean {
if (!reportAction) {
return true;
}
return !isNetworkOffline && reportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
}

/**
* Checks if a reportAction is fit for display as report last action, meaning that
* it satisfies shouldReportActionBeVisible, it's not whisper action and not deleted.
Expand Down Expand Up @@ -816,6 +826,7 @@ export {
isWhisperAction,
isReimbursementQueuedAction,
shouldReportActionBeVisible,
shouldHideNewMarker,
shouldReportActionBeVisibleAsLastAction,
hasRequestFromCurrentAccount,
getFirstVisibleReportActionID,
Expand Down
12 changes: 4 additions & 8 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import withCurrentReportID, {withCurrentReportIDDefaultProps, withCurrentReportI
import withViewportOffsetTop from '@components/withViewportOffsetTop';
import useAppFocusEvent from '@hooks/useAppFocusEvent';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import usePrevious from '@hooks/usePrevious';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
Expand Down Expand Up @@ -159,7 +158,6 @@ function ReportScreen({
const styles = useThemeStyles();
const {translate} = useLocalize();
const {isSmallScreenWidth} = useWindowDimensions();
const {isOffline} = useNetwork();

const firstRenderRef = useRef(true);
const flatListRef = useRef();
Expand All @@ -180,11 +178,8 @@ function ReportScreen({
const {addWorkspaceRoomOrChatPendingAction, addWorkspaceRoomOrChatErrors} = ReportUtils.getReportOfflinePendingActionAndErrors(report);
const screenWrapperStyle = [styles.appContent, styles.flex1, {marginTop: viewportOffsetTop}];

// eslint-disable-next-line react-hooks/exhaustive-deps -- need to re-filter the report actions when network status changes
const filteredReportActions = useMemo(() => ReportActionsUtils.getSortedReportActionsForDisplay(reportActions, true), [isOffline, reportActions]);

// There are no reportActions at all to display and we are still in the process of loading the next set of actions.
const isLoadingInitialReportActions = _.isEmpty(filteredReportActions) && reportMetadata.isLoadingInitialReportActions;
const isLoadingInitialReportActions = _.isEmpty(reportActions) && reportMetadata.isLoadingInitialReportActions;

const isOptimisticDelete = lodashGet(report, 'statusNum') === CONST.REPORT.STATUS.CLOSED;

Expand Down Expand Up @@ -491,7 +486,7 @@ function ReportScreen({
>
{isReportReadyForDisplay && !isLoadingInitialReportActions && !isLoading && (
<ReportActionsView
reportActions={filteredReportActions}
reportActions={reportActions}
report={report}
isLoadingInitialReportActions={reportMetadata.isLoadingInitialReportActions}
isLoadingNewerReportActions={reportMetadata.isLoadingNewerReportActions}
Expand All @@ -509,7 +504,7 @@ function ReportScreen({
{isReportReadyForDisplay ? (
<ReportFooter
pendingAction={addWorkspaceRoomOrChatPendingAction}
reportActions={filteredReportActions}
reportActions={reportActions}
report={report}
isComposerFullSize={isComposerFullSize}
onSubmitComment={onSubmitComment}
Expand Down Expand Up @@ -544,6 +539,7 @@ export default compose(
reportActions: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`,
canEvict: false,
selector: (reportActions) => ReportActionsUtils.getSortedReportActionsForDisplay(reportActions, true),
},
report: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`,
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ function ReportActionsList({
if (!currentUnreadMarker) {
const nextMessage = sortedReportActions[index + 1];
const isCurrentMessageUnread = isMessageUnread(reportAction, lastReadTimeRef.current);
shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, lastReadTimeRef.current));
shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, lastReadTimeRef.current)) && !ReportActionsUtils.shouldHideNewMarker(reportAction);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm:
This PR fully reverts #32474 and this line is the real fix of #31637, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

if (shouldDisplay && !messageManuallyMarkedUnread) {
const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true;
// Prevent displaying a new marker line when report action is of type "REPORTPREVIEW" and last actor is the current user
Expand Down
3 changes: 1 addition & 2 deletions tests/unit/ReportActionsUtilsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ describe('ReportActionsUtils', () => {
expect(result).toStrictEqual(input);
});

it('should filter out deleted and delete-pending comments', () => {
it('should filter out deleted, non-pending comments', () => {
const input = [
{
created: '2022-11-13 22:27:01.825',
Expand All @@ -312,7 +312,6 @@ describe('ReportActionsUtils', () => {
];
const result = ReportActionsUtils.getSortedReportActionsForDisplay(input);
input.pop();
input.pop();
expect(result).toStrictEqual(input);
});
});
Expand Down
Loading