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 @@ -378,10 +378,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 hidden for the report action.
*/
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 @@ -841,6 +851,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
15 changes: 9 additions & 6 deletions src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,10 @@ function ReportActionsList({
const lastVisibleActionCreatedRef = useRef(report.lastVisibleActionCreated);
const lastReadTimeRef = useRef(report.lastReadTime);

const sortedVisibleReportActions = _.filter(sortedReportActions, (s) => isOffline || s.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || s.errors);
const sortedVisibleReportActions = useMemo(
() => _.filter(sortedReportActions, (s) => isOffline || s.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || s.errors),
[sortedReportActions, isOffline],
);
const lastActionIndex = lodashGet(sortedVisibleReportActions, [0, 'reportActionID']);
const reportActionSize = useRef(sortedVisibleReportActions.length);

Expand Down Expand Up @@ -348,9 +351,9 @@ function ReportActionsList({
(reportAction, index) => {
let shouldDisplay = false;
if (!currentUnreadMarker) {
const nextMessage = sortedReportActions[index + 1];
const nextMessage = sortedVisibleReportActions[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 All @@ -367,15 +370,15 @@ function ReportActionsList({

return shouldDisplay;
},
[currentUnreadMarker, sortedReportActions, report.reportID, messageManuallyMarkedUnread],
[currentUnreadMarker, sortedVisibleReportActions, report.reportID, messageManuallyMarkedUnread],
);

useEffect(() => {
// Iterate through the report actions and set appropriate unread marker.
// This is to avoid a warning of:
// Cannot update a component (ReportActionsList) while rendering a different component (CellRenderer).
let markerFound = false;
_.each(sortedReportActions, (reportAction, index) => {
_.each(sortedVisibleReportActions, (reportAction, index) => {
if (!shouldDisplayNewMarker(reportAction, index)) {
return;
}
Expand All @@ -388,7 +391,7 @@ function ReportActionsList({
if (!markerFound) {
setCurrentUnreadMarker(null);
}
}, [sortedReportActions, report.lastReadTime, report.reportID, messageManuallyMarkedUnread, shouldDisplayNewMarker, currentUnreadMarker]);
}, [sortedVisibleReportActions, report.lastReadTime, report.reportID, messageManuallyMarkedUnread, shouldDisplayNewMarker, currentUnreadMarker]);

const renderItem = useCallback(
({item: reportAction, index}) => (
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