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

[CP Staging] Fix/regression unread messages #23729

Merged
merged 9 commits into from
Jul 27, 2023
+67 −38
Merged

[CP Staging] Fix/regression unread messages #23729

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d7ee57c
memoize scrollToBottom function
hannojg Jul 27, 2023
39a6e6a
add dependencies to useEffect
hannojg Jul 27, 2023
5917a4f
fix isFocused
hannojg Jul 27, 2023
df2fc05
fix issue with conflicting subscriptions
hannojg Jul 27, 2023
3cbc752
fix comment
hannojg Jul 27, 2023
6ad9a83
remove reportId from deps
hannojg Jul 27, 2023
890c01d
fix comment
hannojg Jul 27, 2023
5a2656c
fix comment
hannojg Jul 27, 2023
8967d16
remove debug code
hannojg Jul 27, 2023
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
6 changes: 3 additions & 3 deletions src/hooks/useReportScrollManager/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {useContext} from 'react';
import {useContext, useCallback} from 'react';
import ReportScreenContext from '../../pages/home/ReportScreenContext';

function useReportScrollManager() {
Expand All @@ -22,13 +22,13 @@ function useReportScrollManager() {
/**
* Scroll to the bottom of the flatlist.
*/
const scrollToBottom = () => {
const scrollToBottom = useCallback(() => {
if (!flatListRef.current) {
return;
}

flatListRef.current.scrollToOffset({animated: false, offset: 0});
};
}, [flatListRef]);
Copy link
Contributor

Choose a reason for hiding this comment

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

flatListRef would never change. So we can just add [].

Copy link
Contributor Author

@hannojg hannojg Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As flatlist ref comes from the outside eslint doesn't know it's a ref. Thus it will warn us that we need to add it to the dependency array
Then I'd need to add a eslint-ignore line. When it comes to just adding it which is technically correct, or to suppress a warning I'd choose the former.

Also technically it's not correct. Although right now we don't, the flatlistRef could technically change. In this case we need this function to be recreated.


return {ref: flatListRef, scrollToIndex, scrollToBottom};
}
Expand Down
98 changes: 63 additions & 35 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import React, {useRef, useState, useEffect, useContext, useMemo} from 'react';
import React, {useRef, useState, useEffect, useContext, useMemo, useCallback} from 'react';
import PropTypes from 'prop-types';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import lodashCloneDeep from 'lodash/cloneDeep';
import {useIsFocused} from '@react-navigation/native';
import * as Report from '../../../libs/actions/Report';
import reportActionPropTypes from './reportActionPropTypes';
import Visibility from '../../../libs/Visibility';
Expand All @@ -21,7 +22,6 @@ import ReportActionsList from './ReportActionsList';
import * as ReportActionsUtils from '../../../libs/ReportActionsUtils';
import * as ReportUtils from '../../../libs/ReportUtils';
import reportPropTypes from '../../reportPropTypes';
import withNavigationFocus from '../../../components/withNavigationFocus';
import PopoverReactionList from './ReactionList/PopoverReactionList';
import getIsReportFullyVisible from '../../../libs/getIsReportFullyVisible';
import ReportScreenContext from '../ReportScreenContext';
Expand Down Expand Up @@ -58,47 +58,63 @@ const defaultProps = {
policy: null,
};

// In the component we are subscribing to the arrvical of new actions.
// As there is the possibility that there are multiple instances of a ReportScreen
// for the same report, we only ever want one subscription to be active, as
// the subscriptions could otherwise be conflicting.
const newActionUnsubscribeMap = {};

function ReportActionsView(props) {
const context = useContext(ReportScreenContext);
const componentInstanceId = useRef(_.uniqueId('reportActionsView_')).current;

useCopySelectionHelper();

const reportScrollManager = useReportScrollManager();
const {scrollToBottom} = useReportScrollManager();

const didLayout = useRef(false);
const didSubscribeToReportTypingEvents = useRef(false);
const unsubscribeVisibilityListener = useRef(null);
const hasCachedActions = useRef(_.size(props.reportActions) > 0);

const [isFloatingMessageCounterVisible, setIsFloatingMessageCounterVisible] = useState(false);
const [newMarkerReportActionID, setNewMarkerReportActionID] = useState(ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions));

// We use the newMarkerReport ID in a network subscription, we don't want to constantly re-create
// the subscription (as we want to avoid loosing events), so we use a ref to store the value in addition.
// As the value is also needed for UI updates, we also store it in state.
const [newMarkerReportActionID, _setNewMarkerReportActionID] = useState(ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions));
const newMarkerReportActionIDRef = useRef(newMarkerReportActionID);
const setNewMarkerReportActionID = useCallback((value) => {
newMarkerReportActionIDRef.current = value;
_setNewMarkerReportActionID(value);
}, []);

const currentScrollOffset = useRef(0);
const mostRecentIOUReportActionID = useRef(ReportActionsUtils.getMostRecentIOURequestActionID(props.reportActions));

const unsubscribeFromNewActionEvent = useRef(null);

const prevReportActionsRef = useRef(props.reportActions);
const prevReportRef = useRef(props.report);
const prevNetworkRef = useRef(props.network);
const prevIsSmallScreenWidthRef = useRef(props.isSmallScreenWidth);

const isFocused = useIsFocused();
const reportID = props.report.reportID;

/**
* @returns {Boolean}
*/
const isReportFullyVisible = useMemo(() => getIsReportFullyVisible(props.isFocused), [props.isFocused]);
const isReportFullyVisible = useMemo(() => getIsReportFullyVisible(isFocused), [isFocused]);

const openReportIfNecessary = () => {
// If the report is optimistic (AKA not yet created) we don't need to call openReport again
if (props.report.isOptimisticReport) {
return;
}

Report.openReport(props.report.reportID);
Report.openReport(reportID);
};

useEffect(() => {
unsubscribeVisibilityListener.current = Visibility.onVisibilityChange(() => {
const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
if (!isReportFullyVisible) {
return;
}
Expand All @@ -110,37 +126,46 @@ function ReportActionsView(props) {
}
});
return () => {
if (!unsubscribeVisibilityListener.current) {
if (!unsubscribeVisibilityListener) {
return;
}
unsubscribeVisibilityListener.current();
unsubscribeVisibilityListener();
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
}, [isReportFullyVisible, isFocused, props.report, reportID, setNewMarkerReportActionID]);

useEffect(() => {
openReportIfNecessary();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
// Why ware we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function?
// Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted,
// meaning that the cleanup might not get called. When we then open a report we had open again, a new
// ReportScreen will get created. Thus, we have to cancel the earlier subscription of the previous screen,
// because the two subscriptions could conflict!
const previousSubUnsubscribe = newActionUnsubscribeMap[reportID];
if (previousSubUnsubscribe) {
previousSubUnsubscribe();
}

// This callback is triggered when a new action arrives via Pusher and the event is emitted from Report.js. This allows us to maintain
// a single source of truth for the "new action" event instead of trying to derive that a new action has appeared from looking at props.
unsubscribeFromNewActionEvent.current = Report.subscribeToNewActionEvent(props.report.reportID, (isFromCurrentUser, newActionID) => {
const isNewMarkerReportActionIDSet = !_.isEmpty(newMarkerReportActionID);
const unsubscribe = Report.subscribeToNewActionEvent(reportID, (isFromCurrentUser, newActionID) => {
const isNewMarkerReportActionIDSet = !_.isEmpty(newMarkerReportActionIDRef.current);

// If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
// they are now in the list.
if (isFromCurrentUser) {
reportScrollManager.scrollToBottom();
scrollToBottom();
// If the current user sends a new message in the chat we clear the new marker since they have "read" the report
setNewMarkerReportActionID('');
} else if (isReportFullyVisible) {
// We use the scroll position to determine whether the report should be marked as read and the new line indicator reset.
// If the user is scrolled up and no new line marker is set we will set it otherwise we will do nothing so the new marker
// stays in it's previous position.
if (currentScrollOffset.current === 0) {
Report.readNewestAction(props.report.reportID);
Report.readNewestAction(reportID);
setNewMarkerReportActionID('');
} else if (!isNewMarkerReportActionIDSet) {
// The report is not in view and we received a comment from another user while the new marker is not set
Expand All @@ -151,16 +176,19 @@ function ReportActionsView(props) {
setNewMarkerReportActionID(newActionID);
}
});

return () => {
if (unsubscribeFromNewActionEvent.current) {
unsubscribeFromNewActionEvent.current();
const cleanup = () => {
if (unsubscribe) {
unsubscribe();
}
Report.unsubscribeFromReportChannel(reportID);
};

Report.unsubscribeFromReportChannel(props.report.reportID);
newActionUnsubscribeMap[reportID] = cleanup;

return () => {
cleanup();
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
}, [componentInstanceId, isReportFullyVisible, reportID, scrollToBottom, setNewMarkerReportActionID]);

useEffect(() => {
const prevNetwork = prevNetworkRef.current;
Expand All @@ -172,7 +200,7 @@ function ReportActionsView(props) {
if (isReportFullyVisible) {
openReportIfNecessary();
} else {
Report.reconnect(props.report.reportID);
Report.reconnect(reportID);
}
}
// update ref with current network state
Expand Down Expand Up @@ -203,7 +231,7 @@ function ReportActionsView(props) {
setNewMarkerReportActionID(ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions));

prevReportActionsRef.current = props.reportActions;
}, [props.report, props.reportActions]);
}, [props.report, props.reportActions, setNewMarkerReportActionID]);

useEffect(() => {
// If the last unread message was deleted, remove the *New* green marker and the *New Messages* notification at scroll just as the deletion starts.
Expand All @@ -222,7 +250,7 @@ function ReportActionsView(props) {
if (newMarkerReportActionID !== ReportUtils.getNewMarkerReportActionID(props.report, reportActionsWithoutPendingOne)) {
setNewMarkerReportActionID(ReportUtils.getNewMarkerReportActionID(props.report, reportActionsWithoutPendingOne));
}
}, [props.report, props.reportActions, props.network, newMarkerReportActionID]);
}, [props.report, props.reportActions, props.network, newMarkerReportActionID, setNewMarkerReportActionID]);

useEffect(() => {
const prevReport = prevReportRef.current;
Expand All @@ -234,7 +262,7 @@ function ReportActionsView(props) {
}
// update ref with current report
prevReportRef.current = props.report;
}, [props.report, props.reportActions]);
}, [props.report, props.reportActions, setNewMarkerReportActionID]);

useEffect(() => {
// Ensures subscription event succeeds when the report/workspace room is created optimistically.
Expand All @@ -243,10 +271,10 @@ function ReportActionsView(props) {
// Existing reports created will have empty fields for `pendingFields`.
const didCreateReportSuccessfully = !props.report.pendingFields || (!props.report.pendingFields.addWorkspaceRoom && !props.report.pendingFields.createChat);
if (!didSubscribeToReportTypingEvents.current && didCreateReportSuccessfully) {
Report.subscribeToReportTypingEvents(props.report.reportID);
Report.subscribeToReportTypingEvents(reportID);
didSubscribeToReportTypingEvents.current = true;
}
}, [props.report, didSubscribeToReportTypingEvents]);
}, [props.report, didSubscribeToReportTypingEvents, reportID]);

/**
* Retrieves the next set of report actions for the chat once we are nearing the end of what we are currently
Expand All @@ -266,12 +294,12 @@ function ReportActionsView(props) {
}

// Retrieve the next REPORT.ACTIONS.LIMIT sized page of comments
Report.readOldestAction(props.report.reportID, oldestReportAction.reportActionID);
Report.readOldestAction(reportID, oldestReportAction.reportActionID);
};

const scrollToBottomAndMarkReportAsRead = () => {
reportScrollManager.scrollToBottom();
Report.readNewestAction(props.report.reportID);
scrollToBottom();
Report.readNewestAction(reportID);
};

/**
Expand Down Expand Up @@ -413,4 +441,4 @@ function arePropsEqual(oldProps, newProps) {

const MemoizedReportActionsView = React.memo(ReportActionsView, arePropsEqual);

export default compose(Performance.withRenderTrace({id: '<ReportActionsView> rendering'}), withWindowDimensions, withNavigationFocus, withLocalize, withNetwork())(MemoizedReportActionsView);
export default compose(Performance.withRenderTrace({id: '<ReportActionsView> rendering'}), withWindowDimensions, withLocalize, withNetwork())(MemoizedReportActionsView);