From 31a2ff66e8361b6f2c7d8ddd5fba152f54a4b541 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Fri, 16 Apr 2021 12:11:25 +0200 Subject: [PATCH 01/20] allow marking messages as unread --- src/libs/actions/Report.js | 29 ++- .../home/report/ReportActionContextMenu.js | 176 +++++++++--------- src/pages/home/report/ReportActionItem.js | 5 + src/pages/home/report/ReportActionsView.js | 28 +-- 4 files changed, 124 insertions(+), 114 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 3d91b3551643..cd338725959a 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -49,6 +49,9 @@ const typingWatchTimers = {}; // Keeps track of the max sequence number for each report const reportMaxSequenceNumbers = {}; +// Keeps track of the max sequence number for each report excluding loading actions +const reportMaxSequenceNumbersNotLoading = {}; + // Keeps track of the last read for each report const lastReadSequenceNumbers = {}; @@ -800,21 +803,18 @@ function addAction(reportID, text, file) { * network layer handle the delayed write. * * @param {Number} reportID - * @param {Number} sequenceNumber + * @param {Number} [sequenceNumber] */ function updateLastReadActionID(reportID, sequenceNumber) { - const currentMaxSequenceNumber = reportMaxSequenceNumbers[reportID]; - if (sequenceNumber < currentMaxSequenceNumber) { - return; - } + const lastReadSequenceNumber = sequenceNumber || reportMaxSequenceNumbersNotLoading[reportID]; - setLocalLastRead(reportID, sequenceNumber); + setLocalLastRead(reportID, lastReadSequenceNumber); // Mark the report as not having any unread items API.Report_UpdateLastRead({ accountID: currentUserAccountID, reportID, - sequenceNumber, + sequenceNumber: lastReadSequenceNumber, }); } @@ -875,6 +875,21 @@ function handleReportChanged(report) { // Store the max sequence number for each report reportMaxSequenceNumbers[report.reportID] = report.maxSequenceNumber; + // Store the max sequence number for each report excluding loading actions + Onyx.connect({ + key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, + callback: actions => reportMaxSequenceNumbersNotLoading[report.reportID] = _.chain(actions) + + // We want to avoid marking any pending actions as read since + // 1. Any action ID that hasn't been delivered by the server is a temporary action ID. + // 2. We already set a comment someone has authored as the lastReadActionID_ rNVP on the server + // and should sync it locally when we handle it via Pusher or Airship + .reject(action => action.loading) + .pluck('sequenceNumber') + .max() + .value(), + }); + // Store optimistic actions IDs for each report optimisticReportActionIDs[report.reportID] = report.optimisticReportActionIDs; } diff --git a/src/pages/home/report/ReportActionContextMenu.js b/src/pages/home/report/ReportActionContextMenu.js index 78f9fd8c1066..e7933dd9c387 100644 --- a/src/pages/home/report/ReportActionContextMenu.js +++ b/src/pages/home/report/ReportActionContextMenu.js @@ -12,76 +12,13 @@ import ReportActionPropTypes from './ReportActionPropTypes'; import Clipboard from '../../../libs/Clipboard'; import {isReportMessageAttachment} from '../../../libs/reportUtils'; -/** - * A list of all the context actions in this menu. - */ -const CONTEXT_ACTIONS = [ - // Copy to clipboard - { - text: 'Copy to Clipboard', - icon: ClipboardIcon, - successText: 'Copied!', - successIcon: Checkmark, - shouldShow: true, - - // If return value is true, we switch the `text` and `icon` on - // `ReportActionContextMenuItem` with `successText` and `successIcon` which will fallback to - // the `text` and `icon` - onPress: (action) => { - const message = _.last(lodashGet(action, 'message', null)); - const html = lodashGet(message, 'html', ''); - const text = lodashGet(message, 'text', ''); - const isAttachment = _.has(action, 'isAttachment') - ? action.isAttachment - : isReportMessageAttachment(text); - if (!isAttachment) { - Clipboard.setString(text); - } else { - Clipboard.setString(html); - } - }, - }, - - // Copy chat link - { - text: 'Copy Link', - icon: LinkCopy, - shouldShow: false, - onPress: () => {}, - }, - - // Mark as Unread - { - text: 'Mark as Unread', - icon: Mail, - shouldShow: false, - onPress: () => {}, - }, - - // Edit Comment - { - text: 'Edit Comment', - icon: Pencil, - shouldShow: false, - onPress: () => {}, - }, - - // Delete Comment - { - text: 'Delete Comment', - icon: Trashcan, - shouldShow: false, - onPress: () => {}, - }, -]; - const propTypes = { // The ID of the report this report action is attached to. // eslint-disable-next-line react/no-unused-prop-types reportID: PropTypes.number.isRequired, // The report action this context menu is attached to. - reportAction: PropTypes.shape(ReportActionPropTypes), + reportAction: PropTypes.shape(ReportActionPropTypes).isRequired, // If true, this component will be a small, row-oriented menu that displays icons but not text. // If false, this component will be a larger, column-oriented menu that displays icons alongside text in each row. @@ -89,32 +26,105 @@ const propTypes = { // Controls the visibility of this component. isVisible: PropTypes.bool, + + // Function to trigger when we try to mark a message as unread + onMarkAsUnread: PropTypes.func, }; const defaultProps = { - reportAction: {}, isMini: false, isVisible: false, + onMarkAsUnread: () => {}, }; -const ReportActionContextMenu = (props) => { - const wrapperStyle = getReportActionContextMenuStyles(props.isMini); - return props.isVisible && ( - - {CONTEXT_ACTIONS.map(contextAction => contextAction.shouldShow && ( - contextAction.onPress(props.reportAction)} - key={contextAction.text} - /> - ))} - - ); -}; +class ReportActionContextMenu extends React.Component { + constructor(props) { + super(props); + + /** + * A list of all the context actions in this menu. + */ + this.CONTEXT_ACTIONS = [ + // Copy to clipboard + { + text: 'Copy to Clipboard', + icon: ClipboardIcon, + successText: 'Copied!', + successIcon: Checkmark, + shouldShow: true, + + // If return value is true, we switch the `text` and `icon` on + // `ReportActionContextMenuItem` with `successText` and `successIcon` which will fallback to + // the `text` and `icon` + onPress: () => { + const message = _.last(lodashGet(this.props.reportAction, 'message', null)); + const html = lodashGet(message, 'html', ''); + const text = lodashGet(message, 'text', ''); + const isAttachment = _.has(this.props.reportAction, 'isAttachment') + ? this.props.reportAction.isAttachment + : isReportMessageAttachment(text); + if (!isAttachment) { + Clipboard.setString(text); + } else { + Clipboard.setString(html); + } + }, + }, + + // Copy chat link + { + text: 'Copy Link', + icon: LinkCopy, + shouldShow: false, + onPress: () => {}, + }, + + // Mark as Unread + { + text: 'Mark as Unread', + icon: Mail, + shouldShow: true, + onPress: props.onMarkAsUnread, + }, + + // Edit Comment + { + text: 'Edit Comment', + icon: Pencil, + shouldShow: false, + onPress: () => {}, + }, + + // Delete Comment + { + text: 'Delete Comment', + icon: Trashcan, + shouldShow: true, + onPress: () => {}, + }, + ]; + + this.wrapperStyle = getReportActionContextMenuStyles(this.props.isMini); + } + + render() { + return this.props.isVisible && ( + + {this.CONTEXT_ACTIONS.map(contextAction => contextAction.shouldShow && ( + contextAction.onPress()} + /> + ))} + + ); + } +} ReportActionContextMenu.propTypes = propTypes; ReportActionContextMenu.defaultProps = defaultProps; diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js index 51a48e7dc3ea..6ba90c9ca3bc 100644 --- a/src/pages/home/report/ReportActionItem.js +++ b/src/pages/home/report/ReportActionItem.js @@ -25,6 +25,9 @@ const propTypes = { // Should the comment have the appearance of being grouped with the previous comment? displayAsGroup: PropTypes.bool.isRequired, + // Function to trigger when mark as unread is selected + onMarkAsUnread: PropTypes.func.isRequired, + // Should we display the new indicator on top of the comment? shouldDisplayNewIndicator: PropTypes.bool.isRequired, }; @@ -107,6 +110,7 @@ class ReportActionItem extends Component { && !this.state.isPopoverVisible } isMini + onMarkAsUnread={this.props.onMarkAsUnread} /> diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 347dd3d5ee48..99e84d2123ce 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -68,7 +68,6 @@ class ReportActionsView extends React.Component { this.renderItem = this.renderItem.bind(this); this.renderCell = this.renderCell.bind(this); this.scrollToListBottom = this.scrollToListBottom.bind(this); - this.recordMaxAction = this.recordMaxAction.bind(this); this.onVisibilityChange = this.onVisibilityChange.bind(this); this.loadMoreChats = this.loadMoreChats.bind(this); this.sortedReportActions = []; @@ -89,7 +88,7 @@ class ReportActionsView extends React.Component { AppState.addEventListener('change', this.onVisibilityChange); subscribeToReportTypingEvents(this.props.reportID); this.keyboardEvent = Keyboard.addListener('keyboardDidShow', this.scrollToListBottom); - this.recordMaxAction(); + updateLastReadActionID(this.props.reportID); fetchActions(this.props.reportID); Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD); } @@ -122,7 +121,7 @@ class ReportActionsView extends React.Component { // When the last action changes, wait three seconds, then record the max action // This will make the unread indicator go away if you receive comments in the same chat you're looking at if (Visibility.isVisible()) { - this.timers.push(setTimeout(this.recordMaxAction, 3000)); + this.timers.push(setTimeout(() => updateLastReadActionID(this.props.reportID), 3000)); } } } @@ -143,7 +142,7 @@ class ReportActionsView extends React.Component { */ onVisibilityChange() { if (Visibility.isVisible()) { - this.timers.push(setTimeout(this.recordMaxAction, 3000)); + this.timers.push(setTimeout(() => updateLastReadActionID(this.props.reportID), 3000)); } } @@ -213,25 +212,6 @@ class ReportActionsView extends React.Component { return currentAction.action.actorEmail === previousAction.action.actorEmail; } - /** - * Recorded when the report first opens and when the list is scrolled to the bottom - */ - recordMaxAction() { - const reportActions = lodashGet(this.props, 'reportActions', {}); - const maxVisibleSequenceNumber = _.chain(reportActions) - - // We want to avoid marking any pending actions as read since - // 1. Any action ID that hasn't been delivered by the server is a temporary action ID. - // 2. We already set a comment someone has authored as the lastReadActionID_ rNVP on the server - // and should sync it locally when we handle it via Pusher or Airship - .reject(action => action.loading) - .pluck('sequenceNumber') - .max() - .value(); - - updateLastReadActionID(this.props.reportID, maxVisibleSequenceNumber); - } - /** * This function is triggered from the ref callback for the scrollview. That way it can be scrolled once all the * items have been rendered. If the number of actions has changed since it was last rendered, then @@ -241,7 +221,7 @@ class ReportActionsView extends React.Component { if (this.actionListElement) { this.actionListElement.scrollToIndex({animated: false, index: 0}); } - this.recordMaxAction(); + updateLastReadActionID(this.props.reportID); } /** From b6b9c4e308a83f991c42cc94779bc89ec9c3f862 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Fri, 16 Apr 2021 13:25:36 +0200 Subject: [PATCH 02/20] update the marker when neeeded --- src/libs/actions/Report.js | 2 +- src/pages/home/report/ReportActionsView.js | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index cd338725959a..307ee010426f 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -337,7 +337,7 @@ function setLocalLastRead(reportID, sequenceNumber) { // Update the report optimistically Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: 0, + unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), lastVisitedTimestamp: Date.now(), }); } diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 99e84d2123ce..611e2d96521c 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -73,7 +73,7 @@ class ReportActionsView extends React.Component { this.sortedReportActions = []; this.timers = []; - this.initialNewMarkerPosition = props.report.unreadActionCount === 0 + this.newMarkerPosition = props.report.unreadActionCount === 0 ? 0 : (props.report.maxSequenceNumber + 1) - props.report.unreadActionCount; @@ -97,6 +97,10 @@ class ReportActionsView extends React.Component { if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) { this.updateSortedReportActions(nextProps.reportActions); return true; + } else if (nextProps.report.unreadActionCount !== this.props.report.unreadActionCount + && nextProps.report.unreadActionCount !== 0) { + this.newMarkerPosition = (nextProps.report.maxSequenceNumber + 1) - nextProps.report.unreadActionCount; + return true; } if (nextState.isLoadingMoreChats !== this.state.isLoadingMoreChats) { @@ -264,8 +268,10 @@ class ReportActionsView extends React.Component { reportID={this.props.reportID} action={item.action} displayAsGroup={this.isConsecutiveActionMadeByPreviousActor(index)} - shouldDisplayNewIndicator={this.initialNewMarkerPosition > 0 - && item.action.sequenceNumber === this.initialNewMarkerPosition} + shouldDisplayNewIndicator={this.newMarkerPosition > 0 + && item.action.sequenceNumber === this.newMarkerPosition} + onMarkAsUnread={() => updateLastReadActionID(this.props.reportID, + item.action.sequenceNumber - 1)} /> ); } From e3c61aca5a4470dd92ecc9aab25ea1a792ff3849 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Fri, 16 Apr 2021 13:44:09 +0200 Subject: [PATCH 03/20] saving testing stuff --- src/pages/home/report/ReportActionsView.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 611e2d96521c..97f72860569a 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -94,11 +94,16 @@ class ReportActionsView extends React.Component { } shouldComponentUpdate(nextProps, nextState) { + console.log('called'); if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) { + console.log('this'); this.updateSortedReportActions(nextProps.reportActions); return true; } else if (nextProps.report.unreadActionCount !== this.props.report.unreadActionCount + && nextProps.report.lastMessageTimestamp === this.props.report.lastMessageTimestamp && nextProps.report.unreadActionCount !== 0) { + console.log(this.props); + console.log(nextProps); this.newMarkerPosition = (nextProps.report.maxSequenceNumber + 1) - nextProps.report.unreadActionCount; return true; } From 736eddd30de95d3633b55dd86dc4e9252ddbc0c1 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Fri, 16 Apr 2021 15:23:45 +0200 Subject: [PATCH 04/20] cleanup --- src/pages/home/report/ReportActionsView.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 97f72860569a..66b0cdcafb0a 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -40,6 +40,9 @@ const propTypes = { // The largest sequenceNumber on this report maxSequenceNumber: PropTypes.number, + + // The timestamp of the last message on this report + lastMessageTimestamp: PropTypes.number, }), // Array of report actions for this report @@ -94,16 +97,18 @@ class ReportActionsView extends React.Component { } shouldComponentUpdate(nextProps, nextState) { - console.log('called'); if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) { - console.log('this'); this.updateSortedReportActions(nextProps.reportActions); return true; - } else if (nextProps.report.unreadActionCount !== this.props.report.unreadActionCount + } + if (nextProps.report.unreadActionCount !== this.props.report.unreadActionCount && nextProps.report.lastMessageTimestamp === this.props.report.lastMessageTimestamp + && (!lastItem(nextProps.reportActions) || !lastItem(nextProps.reportActions).loading) && nextProps.report.unreadActionCount !== 0) { - console.log(this.props); - console.log(nextProps); + // We just want to set the marker if a message has been marked as unread manually. To avoid moving the + // marker when new messages come in, we check whether the last action is loading (which would mean the user + // just sent a message themself) and if the timestamps match (which would mean they received a message from + // other chat participant). this.newMarkerPosition = (nextProps.report.maxSequenceNumber + 1) - nextProps.report.unreadActionCount; return true; } From 913f73e8de32e1bc04afd4a1caecd8f0b91f6ca1 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Tue, 20 Apr 2021 14:34:50 +0200 Subject: [PATCH 05/20] move new marker to report --- src/libs/actions/Report.js | 6 ++++-- src/pages/home/report/ReportActionsView.js | 24 +++++++++------------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 307ee010426f..9ea1c9a2562b 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -331,14 +331,16 @@ function fetchChatReportsByIDs(chatList) { * * @param {Number} reportID * @param {Number} sequenceNumber + * @param {Boolean} saveNewMarkerPosition */ -function setLocalLastRead(reportID, sequenceNumber) { +function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerPosition) { lastReadSequenceNumbers[reportID] = sequenceNumber; // Update the report optimistically Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), lastVisitedTimestamp: Date.now(), + saveNewMarkerPosition: saveNewMarkerPosition ? sequenceNumber : 0, }); } @@ -808,7 +810,7 @@ function addAction(reportID, text, file) { function updateLastReadActionID(reportID, sequenceNumber) { const lastReadSequenceNumber = sequenceNumber || reportMaxSequenceNumbersNotLoading[reportID]; - setLocalLastRead(reportID, lastReadSequenceNumber); + setLocalLastRead(reportID, lastReadSequenceNumber, sequenceNumber !== undefined); // Mark the report as not having any unread items API.Report_UpdateLastRead({ diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 66b0cdcafb0a..04461ee9df25 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -26,6 +26,7 @@ import Visibility from '../../../libs/Visibility'; import Timing from '../../../libs/actions/Timing'; import CONST from '../../../CONST'; import themeColors from '../../../styles/themes/default'; +import Onyx from '../../../../../react-native-onyx'; const propTypes = { // The ID of the report actions will be created for @@ -41,8 +42,7 @@ const propTypes = { // The largest sequenceNumber on this report maxSequenceNumber: PropTypes.number, - // The timestamp of the last message on this report - lastMessageTimestamp: PropTypes.number, + newMarkerPosition: PropTypes.number, }), // Array of report actions for this report @@ -76,10 +76,13 @@ class ReportActionsView extends React.Component { this.sortedReportActions = []; this.timers = []; - this.newMarkerPosition = props.report.unreadActionCount === 0 + const newMarkerPosition = props.report.unreadActionCount === 0 ? 0 : (props.report.maxSequenceNumber + 1) - props.report.unreadActionCount; + // Set the new marker + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${props.reportID}`, {newMarkerPosition}); + this.state = { isLoadingMoreChats: false, }; @@ -101,15 +104,8 @@ class ReportActionsView extends React.Component { this.updateSortedReportActions(nextProps.reportActions); return true; } - if (nextProps.report.unreadActionCount !== this.props.report.unreadActionCount - && nextProps.report.lastMessageTimestamp === this.props.report.lastMessageTimestamp - && (!lastItem(nextProps.reportActions) || !lastItem(nextProps.reportActions).loading) - && nextProps.report.unreadActionCount !== 0) { - // We just want to set the marker if a message has been marked as unread manually. To avoid moving the - // marker when new messages come in, we check whether the last action is loading (which would mean the user - // just sent a message themself) and if the timestamps match (which would mean they received a message from - // other chat participant). - this.newMarkerPosition = (nextProps.report.maxSequenceNumber + 1) - nextProps.report.unreadActionCount; + + if (nextProps.report.newMarkerPosition > 0 && nextProps.report.newMarkerPosition !== this.props.report.newMarkerPosition) { return true; } @@ -278,8 +274,8 @@ class ReportActionsView extends React.Component { reportID={this.props.reportID} action={item.action} displayAsGroup={this.isConsecutiveActionMadeByPreviousActor(index)} - shouldDisplayNewIndicator={this.newMarkerPosition > 0 - && item.action.sequenceNumber === this.newMarkerPosition} + shouldDisplayNewIndicator={this.props.report.newMarkerPosition > 0 + && item.action.sequenceNumber === this.props.report.newMarkerPosition} onMarkAsUnread={() => updateLastReadActionID(this.props.reportID, item.action.sequenceNumber - 1)} /> From ff7388710070ce51ebe2f1d8b213b6d754598a88 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Tue, 20 Apr 2021 15:25:59 +0200 Subject: [PATCH 06/20] add onyx calls --- src/libs/actions/Report.js | 18 +++++++++++++----- src/pages/home/report/ReportActionsView.js | 7 +++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 9ea1c9a2562b..d36ec09f901e 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -335,13 +335,21 @@ function fetchChatReportsByIDs(chatList) { */ function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerPosition) { lastReadSequenceNumbers[reportID] = sequenceNumber; + console.log('this got caled'); // Update the report optimistically - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), - lastVisitedTimestamp: Date.now(), - saveNewMarkerPosition: saveNewMarkerPosition ? sequenceNumber : 0, - }); + if (saveNewMarkerPosition) { + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { + unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), + lastVisitedTimestamp: Date.now(), + newMarkerPosition: saveNewMarkerPosition ? sequenceNumber : 0, + }); + } else { + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { + unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), + lastVisitedTimestamp: Date.now(), + }); + } } /** diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 04461ee9df25..d47c7180ffaf 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -8,7 +8,7 @@ import { import PropTypes from 'prop-types'; import _ from 'underscore'; import lodashGet from 'lodash/get'; -import {withOnyx} from 'react-native-onyx'; +import Onyx, {withOnyx} from 'react-native-onyx'; import Text from '../../../components/Text'; import { fetchActions, @@ -26,7 +26,6 @@ import Visibility from '../../../libs/Visibility'; import Timing from '../../../libs/actions/Timing'; import CONST from '../../../CONST'; import themeColors from '../../../styles/themes/default'; -import Onyx from '../../../../../react-native-onyx'; const propTypes = { // The ID of the report actions will be created for @@ -78,7 +77,7 @@ class ReportActionsView extends React.Component { const newMarkerPosition = props.report.unreadActionCount === 0 ? 0 - : (props.report.maxSequenceNumber + 1) - props.report.unreadActionCount; + : props.report.maxSequenceNumber - props.report.unreadActionCount; // Set the new marker Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${props.reportID}`, {newMarkerPosition}); @@ -277,7 +276,7 @@ class ReportActionsView extends React.Component { shouldDisplayNewIndicator={this.props.report.newMarkerPosition > 0 && item.action.sequenceNumber === this.props.report.newMarkerPosition} onMarkAsUnread={() => updateLastReadActionID(this.props.reportID, - item.action.sequenceNumber - 1)} + item.action.sequenceNumber)} /> ); } From 6960577fa798e491959a9abfc0e2e13d866a07ae Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Tue, 20 Apr 2021 16:07:53 +0200 Subject: [PATCH 07/20] cleanup --- src/libs/actions/Report.js | 3 +-- src/pages/home/report/ReportActionsView.js | 9 +++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index d36ec09f901e..f24684722cb6 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -335,7 +335,6 @@ function fetchChatReportsByIDs(chatList) { */ function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerPosition) { lastReadSequenceNumbers[reportID] = sequenceNumber; - console.log('this got caled'); // Update the report optimistically if (saveNewMarkerPosition) { @@ -346,7 +345,7 @@ function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerPosition) { }); } else { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), + unreadActionCount: 0, lastVisitedTimestamp: Date.now(), }); } diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index d47c7180ffaf..392232ea237d 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -41,6 +41,7 @@ const propTypes = { // The largest sequenceNumber on this report maxSequenceNumber: PropTypes.number, + // The current position of the new marker newMarkerPosition: PropTypes.number, }), @@ -74,7 +75,6 @@ class ReportActionsView extends React.Component { this.loadMoreChats = this.loadMoreChats.bind(this); this.sortedReportActions = []; this.timers = []; - const newMarkerPosition = props.report.unreadActionCount === 0 ? 0 : props.report.maxSequenceNumber - props.report.unreadActionCount; @@ -104,7 +104,8 @@ class ReportActionsView extends React.Component { return true; } - if (nextProps.report.newMarkerPosition > 0 && nextProps.report.newMarkerPosition !== this.props.report.newMarkerPosition) { + if (nextProps.report.newMarkerPosition > 0 + && nextProps.report.newMarkerPosition !== this.props.report.newMarkerPosition) { return true; } @@ -274,9 +275,9 @@ class ReportActionsView extends React.Component { action={item.action} displayAsGroup={this.isConsecutiveActionMadeByPreviousActor(index)} shouldDisplayNewIndicator={this.props.report.newMarkerPosition > 0 - && item.action.sequenceNumber === this.props.report.newMarkerPosition} + && item.action.sequenceNumber - 1 === this.props.report.newMarkerPosition} onMarkAsUnread={() => updateLastReadActionID(this.props.reportID, - item.action.sequenceNumber)} + item.action.sequenceNumber - 1)} /> ); } From a1b57a39712d054890b175bd8a259ad68a27bc9f Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Wed, 21 Apr 2021 14:52:11 +0200 Subject: [PATCH 08/20] remove onMarkUnread fonction and view calls to onyx --- src/libs/actions/Report.js | 13 ++++---- .../home/report/ReportActionContextMenu.js | 19 +++-------- src/pages/home/report/ReportActionItem.js | 5 --- src/pages/home/report/ReportActionsView.js | 32 +++++++++++-------- 4 files changed, 30 insertions(+), 39 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index f24684722cb6..7962b1b210d8 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -331,17 +331,17 @@ function fetchChatReportsByIDs(chatList) { * * @param {Number} reportID * @param {Number} sequenceNumber - * @param {Boolean} saveNewMarkerPosition + * @param {Boolean} [saveNewMarkerSequenceNumber] */ -function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerPosition) { +function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerSequenceNumber) { lastReadSequenceNumbers[reportID] = sequenceNumber; // Update the report optimistically - if (saveNewMarkerPosition) { + if (saveNewMarkerSequenceNumber) { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), + unreadActionCount: Math.max(reportMaxSequenceNumbersNotLoading[reportID] - sequenceNumber, 0), lastVisitedTimestamp: Date.now(), - newMarkerPosition: saveNewMarkerPosition ? sequenceNumber : 0, + newMarkerSequenceNumber: sequenceNumber, }); } else { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { @@ -823,7 +823,7 @@ function updateLastReadActionID(reportID, sequenceNumber) { API.Report_UpdateLastRead({ accountID: currentUserAccountID, reportID, - sequenceNumber: lastReadSequenceNumber, + sequenceNumber: lastReadSequenceNumber || 0, }); } @@ -926,6 +926,7 @@ export { fetchOrCreateChatReport, addAction, updateLastReadActionID, + setLocalLastRead, subscribeToReportCommentEvents, subscribeToReportTypingEvents, unsubscribeFromReportChannel, diff --git a/src/pages/home/report/ReportActionContextMenu.js b/src/pages/home/report/ReportActionContextMenu.js index e7933dd9c387..e89d6299d1f1 100644 --- a/src/pages/home/report/ReportActionContextMenu.js +++ b/src/pages/home/report/ReportActionContextMenu.js @@ -7,6 +7,7 @@ import { Clipboard as ClipboardIcon, LinkCopy, Mail, Pencil, Trashcan, Checkmark, } from '../../../components/Icon/Expensicons'; import getReportActionContextMenuStyles from '../../../styles/getReportActionContextMenuStyles'; +import {updateLastReadActionID} from '../../../libs/actions/Report'; import ReportActionContextMenuItem from './ReportActionContextMenuItem'; import ReportActionPropTypes from './ReportActionPropTypes'; import Clipboard from '../../../libs/Clipboard'; @@ -26,24 +27,18 @@ const propTypes = { // Controls the visibility of this component. isVisible: PropTypes.bool, - - // Function to trigger when we try to mark a message as unread - onMarkAsUnread: PropTypes.func, }; const defaultProps = { isMini: false, isVisible: false, - onMarkAsUnread: () => {}, }; class ReportActionContextMenu extends React.Component { constructor(props) { super(props); - /** - * A list of all the context actions in this menu. - */ + // A list of all the context actions in this menu. this.CONTEXT_ACTIONS = [ // Copy to clipboard { @@ -71,7 +66,6 @@ class ReportActionContextMenu extends React.Component { }, }, - // Copy chat link { text: 'Copy Link', icon: LinkCopy, @@ -79,15 +73,13 @@ class ReportActionContextMenu extends React.Component { onPress: () => {}, }, - // Mark as Unread { text: 'Mark as Unread', icon: Mail, shouldShow: true, - onPress: props.onMarkAsUnread, + onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber -1), }, - // Edit Comment { text: 'Edit Comment', icon: Pencil, @@ -95,11 +87,10 @@ class ReportActionContextMenu extends React.Component { onPress: () => {}, }, - // Delete Comment { text: 'Delete Comment', icon: Trashcan, - shouldShow: true, + shouldShow: false, onPress: () => {}, }, ]; @@ -118,7 +109,7 @@ class ReportActionContextMenu extends React.Component { successText={contextAction.successText} isMini={this.props.isMini} key={contextAction.text} - onPress={() => contextAction.onPress()} + onPress={contextAction.onPress} /> ))} diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js index 6ba90c9ca3bc..51a48e7dc3ea 100644 --- a/src/pages/home/report/ReportActionItem.js +++ b/src/pages/home/report/ReportActionItem.js @@ -25,9 +25,6 @@ const propTypes = { // Should the comment have the appearance of being grouped with the previous comment? displayAsGroup: PropTypes.bool.isRequired, - // Function to trigger when mark as unread is selected - onMarkAsUnread: PropTypes.func.isRequired, - // Should we display the new indicator on top of the comment? shouldDisplayNewIndicator: PropTypes.bool.isRequired, }; @@ -110,7 +107,6 @@ class ReportActionItem extends Component { && !this.state.isPopoverVisible } isMini - onMarkAsUnread={this.props.onMarkAsUnread} /> diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 392232ea237d..a96df061cfcd 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -8,11 +8,12 @@ import { import PropTypes from 'prop-types'; import _ from 'underscore'; import lodashGet from 'lodash/get'; -import Onyx, {withOnyx} from 'react-native-onyx'; +import {withOnyx} from 'react-native-onyx'; import Text from '../../../components/Text'; import { fetchActions, updateLastReadActionID, + setLocalLastRead, subscribeToReportTypingEvents, unsubscribeFromReportChannel, } from '../../../libs/actions/Report'; @@ -42,7 +43,7 @@ const propTypes = { maxSequenceNumber: PropTypes.number, // The current position of the new marker - newMarkerPosition: PropTypes.number, + newMarkerSequenceNumber: PropTypes.number, }), // Array of report actions for this report @@ -75,12 +76,6 @@ class ReportActionsView extends React.Component { this.loadMoreChats = this.loadMoreChats.bind(this); this.sortedReportActions = []; this.timers = []; - const newMarkerPosition = props.report.unreadActionCount === 0 - ? 0 - : props.report.maxSequenceNumber - props.report.unreadActionCount; - - // Set the new marker - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${props.reportID}`, {newMarkerPosition}); this.state = { isLoadingMoreChats: false, @@ -93,6 +88,15 @@ class ReportActionsView extends React.Component { AppState.addEventListener('change', this.onVisibilityChange); subscribeToReportTypingEvents(this.props.reportID); this.keyboardEvent = Keyboard.addListener('keyboardDidShow', this.scrollToListBottom); + + // Since we want the New marker to remain in place even if newer messages come in, we set it in the constructor + const newMarkerSequenceNumber = this.props.report.unreadActionCount === 0 + ? 0 + : this.props.report.maxSequenceNumber - this.props.report.unreadActionCount; + + // Set the new marker + setLocalLastRead(this.props.reportID, newMarkerSequenceNumber, true); + updateLastReadActionID(this.props.reportID); fetchActions(this.props.reportID); Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD); @@ -104,8 +108,10 @@ class ReportActionsView extends React.Component { return true; } - if (nextProps.report.newMarkerPosition > 0 - && nextProps.report.newMarkerPosition !== this.props.report.newMarkerPosition) { + // If the new marker has changed places (because the user manually marked a comment as Unread), we have to + // update the component. + if (nextProps.report.newMarkerSequenceNumber > 0 + && nextProps.report.newMarkerSequenceNumber !== this.props.report.newMarkerSequenceNumber) { return true; } @@ -274,10 +280,8 @@ class ReportActionsView extends React.Component { reportID={this.props.reportID} action={item.action} displayAsGroup={this.isConsecutiveActionMadeByPreviousActor(index)} - shouldDisplayNewIndicator={this.props.report.newMarkerPosition > 0 - && item.action.sequenceNumber - 1 === this.props.report.newMarkerPosition} - onMarkAsUnread={() => updateLastReadActionID(this.props.reportID, - item.action.sequenceNumber - 1)} + shouldDisplayNewIndicator={this.props.report.newMarkerSequenceNumber > 0 + && item.action.sequenceNumber - 1 === this.props.report.newMarkerSequenceNumber} /> ); } From 1939856909a514388ed5f0c7e79e84fd4b75e9d5 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Wed, 21 Apr 2021 15:06:19 +0200 Subject: [PATCH 09/20] lint --- src/pages/home/report/ReportActionContextMenu.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionContextMenu.js b/src/pages/home/report/ReportActionContextMenu.js index e89d6299d1f1..421446bd0d4d 100644 --- a/src/pages/home/report/ReportActionContextMenu.js +++ b/src/pages/home/report/ReportActionContextMenu.js @@ -77,7 +77,7 @@ class ReportActionContextMenu extends React.Component { text: 'Mark as Unread', icon: Mail, shouldShow: true, - onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber -1), + onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber - 1), }, { From 7dcc0b74fec528ea6f46f90c853018562086267f Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Fri, 23 Apr 2021 11:00:35 +0200 Subject: [PATCH 10/20] Fix mobile behavior --- src/pages/home/report/ReportActionContextMenu.js | 3 ++- src/pages/home/report/ReportActionItem.js | 2 +- src/pages/home/report/ReportActionsView.js | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/pages/home/report/ReportActionContextMenu.js b/src/pages/home/report/ReportActionContextMenu.js index 421446bd0d4d..f7e79c659856 100644 --- a/src/pages/home/report/ReportActionContextMenu.js +++ b/src/pages/home/report/ReportActionContextMenu.js @@ -76,8 +76,9 @@ class ReportActionContextMenu extends React.Component { { text: 'Mark as Unread', icon: Mail, + successIcon: Checkmark, shouldShow: true, - onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber - 1), + onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber), }, { diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js index 51a48e7dc3ea..33b0c4db8930 100644 --- a/src/pages/home/report/ReportActionItem.js +++ b/src/pages/home/report/ReportActionItem.js @@ -118,7 +118,7 @@ class ReportActionItem extends Component { measureContent={() => ( )} diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index a96df061cfcd..b3193e2bef3b 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -92,7 +92,7 @@ class ReportActionsView extends React.Component { // Since we want the New marker to remain in place even if newer messages come in, we set it in the constructor const newMarkerSequenceNumber = this.props.report.unreadActionCount === 0 ? 0 - : this.props.report.maxSequenceNumber - this.props.report.unreadActionCount; + : (this.props.report.maxSequenceNumber - this.props.report.unreadActionCount) + 1; // Set the new marker setLocalLastRead(this.props.reportID, newMarkerSequenceNumber, true); @@ -281,7 +281,7 @@ class ReportActionsView extends React.Component { action={item.action} displayAsGroup={this.isConsecutiveActionMadeByPreviousActor(index)} shouldDisplayNewIndicator={this.props.report.newMarkerSequenceNumber > 0 - && item.action.sequenceNumber - 1 === this.props.report.newMarkerSequenceNumber} + && item.action.sequenceNumber === this.props.report.newMarkerSequenceNumber} /> ); } From 52c2008817f32bec7131a7935408b495338a1eff Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Tue, 27 Apr 2021 21:20:05 +0200 Subject: [PATCH 11/20] Get rid of ReportsMaxSequenceNumberNotLoading --- src/libs/actions/Report.js | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 7962b1b210d8..f1f50aa4621e 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -49,9 +49,6 @@ const typingWatchTimers = {}; // Keeps track of the max sequence number for each report const reportMaxSequenceNumbers = {}; -// Keeps track of the max sequence number for each report excluding loading actions -const reportMaxSequenceNumbersNotLoading = {}; - // Keeps track of the last read for each report const lastReadSequenceNumbers = {}; @@ -339,7 +336,7 @@ function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerSequenceNumber) // Update the report optimistically if (saveNewMarkerSequenceNumber) { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: Math.max(reportMaxSequenceNumbersNotLoading[reportID] - sequenceNumber, 0), + unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), lastVisitedTimestamp: Date.now(), newMarkerSequenceNumber: sequenceNumber, }); @@ -815,7 +812,7 @@ function addAction(reportID, text, file) { * @param {Number} [sequenceNumber] */ function updateLastReadActionID(reportID, sequenceNumber) { - const lastReadSequenceNumber = sequenceNumber || reportMaxSequenceNumbersNotLoading[reportID]; + const lastReadSequenceNumber = sequenceNumber || reportMaxSequenceNumbers[reportID]; setLocalLastRead(reportID, lastReadSequenceNumber, sequenceNumber !== undefined); @@ -884,21 +881,6 @@ function handleReportChanged(report) { // Store the max sequence number for each report reportMaxSequenceNumbers[report.reportID] = report.maxSequenceNumber; - // Store the max sequence number for each report excluding loading actions - Onyx.connect({ - key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, - callback: actions => reportMaxSequenceNumbersNotLoading[report.reportID] = _.chain(actions) - - // We want to avoid marking any pending actions as read since - // 1. Any action ID that hasn't been delivered by the server is a temporary action ID. - // 2. We already set a comment someone has authored as the lastReadActionID_ rNVP on the server - // and should sync it locally when we handle it via Pusher or Airship - .reject(action => action.loading) - .pluck('sequenceNumber') - .max() - .value(), - }); - // Store optimistic actions IDs for each report optimisticReportActionIDs[report.reportID] = report.optimisticReportActionIDs; } From 769de746ce3e9c4a1fab648731ce3d1ae61bd092 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Tue, 27 Apr 2021 21:39:26 +0200 Subject: [PATCH 12/20] lint --- src/pages/home/report/ReportActionsView.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index cd1435d9dce2..5439f590979a 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -170,7 +170,6 @@ class ReportActionsView extends React.Component { // This will make the unread indicator go away if you receive comments in the same chat you're looking at if (shouldRecordMaxAction) { this.startRecordMaxActionTimer(); - } } From f723644855eb3aac3dc907e41b506933a287931c Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Wed, 28 Apr 2021 15:27:35 +0200 Subject: [PATCH 13/20] refactor some code to make it easier to follow --- src/libs/actions/Report.js | 47 ++++++++++++------- .../home/report/ReportActionContextMenu.js | 2 +- src/pages/home/report/ReportActionsView.js | 37 ++------------- 3 files changed, 37 insertions(+), 49 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 78bfbebf5a3d..7379cac1a7da 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -373,24 +373,30 @@ function setLocalIOUReportData(iouReportObject, chatReportID) { * * @param {Number} reportID * @param {Number} sequenceNumber - * @param {Boolean} [saveNewMarkerSequenceNumber] */ -function setLocalLastRead(reportID, sequenceNumber, saveNewMarkerSequenceNumber) { +function setLocalLastRead(reportID, sequenceNumber) { lastReadSequenceNumbers[reportID] = sequenceNumber; // Update the report optimistically - if (saveNewMarkerSequenceNumber) { - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), - lastVisitedTimestamp: Date.now(), - newMarkerSequenceNumber: sequenceNumber, - }); - } else { - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: 0, - lastVisitedTimestamp: Date.now(), - }); - } + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { + unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), + lastVisitedTimestamp: Date.now(), + }); +} + +/** + * Set the New marker for a given report to the oldest unread message + * + * @param {Number} reportID + */ +function setNewMarkerInitialPosition(reportID) { + const lastReadSequenceNumber = allReports[reportID].unreadActionCount === 0 + ? 0 + : reportMaxSequenceNumbers[reportID] - allReports[reportID].unreadActionCount; + + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { + newMarkerSequenceNumber: lastReadSequenceNumber + 1, + }); } /** @@ -916,9 +922,18 @@ function addAction(reportID, text, file) { * @param {Number} [sequenceNumber] */ function updateLastReadActionID(reportID, sequenceNumber) { + // If we are not specifying the sequence number, let's set it to the max one available const lastReadSequenceNumber = sequenceNumber || reportMaxSequenceNumbers[reportID]; - setLocalLastRead(reportID, lastReadSequenceNumber, sequenceNumber !== undefined); + setLocalLastRead(reportID, lastReadSequenceNumber); + + // If we are specifying the last read sequence number, we are manually marking a comment as unread, so + // let's place new marker in the after the last read sequence. + if (sequenceNumber) { + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { + newMarkerSequenceNumber: sequenceNumber + 1, + }); + } // Mark the report as not having any unread items API.Report_UpdateLastRead({ @@ -1012,7 +1027,7 @@ export { fetchOrCreateChatReport, addAction, updateLastReadActionID, - setLocalLastRead, + setNewMarkerInitialPosition, subscribeToReportTypingEvents, subscribeToUserEvents, unsubscribeFromReportChannel, diff --git a/src/pages/home/report/ReportActionContextMenu.js b/src/pages/home/report/ReportActionContextMenu.js index f7e79c659856..6667e049be3b 100644 --- a/src/pages/home/report/ReportActionContextMenu.js +++ b/src/pages/home/report/ReportActionContextMenu.js @@ -78,7 +78,7 @@ class ReportActionContextMenu extends React.Component { icon: Mail, successIcon: Checkmark, shouldShow: true, - onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber), + onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber - 1), }, { diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 5439f590979a..b31e8ec514c7 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -13,7 +13,7 @@ import Text from '../../../components/Text'; import { fetchActions, updateLastReadActionID, - setLocalLastRead, + setNewMarkerInitialPosition, subscribeToReportTypingEvents, unsubscribeFromReportChannel, } from '../../../libs/actions/Report'; @@ -103,16 +103,10 @@ class ReportActionsView extends React.Component { AppState.addEventListener('change', this.onVisibilityChange); subscribeToReportTypingEvents(this.props.reportID); this.keyboardEvent = Keyboard.addListener('keyboardDidShow', this.scrollToListBottom); - - // Since we want the New marker to remain in place even if newer messages come in, we set it in the constructor - const newMarkerSequenceNumber = this.props.report.unreadActionCount === 0 - ? 0 - : (this.props.report.maxSequenceNumber - this.props.report.unreadActionCount) + 1; - - // Set the new marker - setLocalLastRead(this.props.reportID, newMarkerSequenceNumber, true); - updateLastReadActionID(this.props.reportID); + + // Since we want the New marker to remain in place even if newer messages come in, we set it once on mount + setNewMarkerInitialPosition(this.props.reportID); fetchActions(this.props.reportID); Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD); } @@ -209,7 +203,7 @@ class ReportActionsView extends React.Component { * @memberof ReportActionsView */ startRecordMaxActionTimer() { - this.timers.push(setTimeout(this.recordMaxAction, 3000)); + this.timers.push(setTimeout(() => updateLastReadActionID(this.props.reportID), 3000)); } /** @@ -278,27 +272,6 @@ class ReportActionsView extends React.Component { return currentAction.action.actorEmail === previousAction.action.actorEmail; } - /** -<<<<<<< HEAD -======= - * Recorded when the report first opens and when the list is scrolled to the bottom - */ - recordMaxAction() { - const reportActions = lodashGet(this.props, 'reportActions', {}); - const maxVisibleSequenceNumber = _.chain(reportActions) - - // We want to avoid marking any pending actions as read since - // 1. Any action ID that hasn't been delivered by the server is a temporary action ID. - // 2. We already set a comment someone has authored as the lastReadActionID_ rNVP on the server - // and should sync it locally when we handle it via Pusher or Airship - .reject(action => action.loading) - .pluck('sequenceNumber') - .max() - .value(); - - updateLastReadActionID(this.props.reportID, maxVisibleSequenceNumber); - } - /** * Finds and updates most recent IOU report action number * From 9e4eb50328dca3672c3a27f7f2b0cad35aaff594 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Thu, 29 Apr 2021 13:50:56 +0200 Subject: [PATCH 14/20] add comments and clean up code a bit --- src/libs/actions/Report.js | 26 +++++++++++++++------- src/pages/home/report/ReportActionsView.js | 6 ++--- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 7379cac1a7da..703018fca1b3 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -372,28 +372,37 @@ function setLocalIOUReportData(iouReportObject, chatReportID) { * Update the lastRead actionID and timestamp in local memory and Onyx * * @param {Number} reportID - * @param {Number} sequenceNumber + * @param {Number} lastReadSequenceNumber */ -function setLocalLastRead(reportID, sequenceNumber) { - lastReadSequenceNumbers[reportID] = sequenceNumber; +function setLocalLastRead(reportID, lastReadSequenceNumber) { + lastReadSequenceNumbers[reportID] = lastReadSequenceNumber; + const reportMaxSequenceNumber = reportMaxSequenceNumbers[reportID]; - // Update the report optimistically + // Determine the number of unread actions by deducting the last read sequence from the total. If, for some reason, + // the last read sequence is higher than the actual last sequence, let's just assume all actions are read + const unreadActionCount = Math.max(reportMaxSequenceNumber - lastReadSequenceNumber, 0); + + // Update the report optimistically. Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0), + unreadActionCount, lastVisitedTimestamp: Date.now(), }); } /** - * Set the New marker for a given report to the oldest unread message + * Set the New marker for a given report to the oldest unread sequence * * @param {Number} reportID */ function setNewMarkerInitialPosition(reportID) { + + // If we have any unread actions, we determine the last read action by deduction the number of unread actions + // from the total number of actions in the report const lastReadSequenceNumber = allReports[reportID].unreadActionCount === 0 ? 0 : reportMaxSequenceNumbers[reportID] - allReports[reportID].unreadActionCount; + // Then, we set the New marker on the following action, which is the oldest unread one Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { newMarkerSequenceNumber: lastReadSequenceNumber + 1, }); @@ -922,13 +931,14 @@ function addAction(reportID, text, file) { * @param {Number} [sequenceNumber] */ function updateLastReadActionID(reportID, sequenceNumber) { - // If we are not specifying the sequence number, let's set it to the max one available + // If we are not specifying the sequence number, let's set it to the max one available, so that all actions in the + // report are considered "read". const lastReadSequenceNumber = sequenceNumber || reportMaxSequenceNumbers[reportID]; setLocalLastRead(reportID, lastReadSequenceNumber); // If we are specifying the last read sequence number, we are manually marking a comment as unread, so - // let's place new marker in the after the last read sequence. + // let's place new marker on the oldest unread sequence (after the last read sequence). if (sequenceNumber) { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { newMarkerSequenceNumber: sequenceNumber + 1, diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index b31e8ec514c7..ad6bbe978a76 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -286,7 +286,6 @@ class ReportActionsView extends React.Component { } /** ->>>>>>> main * This function is triggered from the ref callback for the scrollview. That way it can be scrolled once all the * items have been rendered. If the number of actions has changed since it was last rendered, then * scroll the list to the end. As a report can contain non-message actions, we should confirm that list data exists. @@ -333,13 +332,14 @@ class ReportActionsView extends React.Component { item, index, }) { + const shouldDisplayNewIndicator = this.props.report.newMarkerSequenceNumber > 0 + && item.action.sequenceNumber === this.props.report.newMarkerSequenceNumber; return ( 0 - && item.action.sequenceNumber === this.props.report.newMarkerSequenceNumber} + shouldDisplayNewIndicator={shouldDisplayNewIndicator} isMostRecentIOUReportAction={item.action.sequenceNumber === this.mostRecentIOUReportSequenceNumber} iouReportID={this.props.report.iouReportID} hasOutstandingIOU={this.props.report.hasOutstandingIOU} From a26e3ed31783ad5bf462e3b478ebc276f08cc4f7 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Thu, 29 Apr 2021 15:12:46 +0200 Subject: [PATCH 15/20] lint --- src/libs/actions/Report.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 703018fca1b3..765c2375b5d7 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -395,7 +395,6 @@ function setLocalLastRead(reportID, lastReadSequenceNumber) { * @param {Number} reportID */ function setNewMarkerInitialPosition(reportID) { - // If we have any unread actions, we determine the last read action by deduction the number of unread actions // from the total number of actions in the report const lastReadSequenceNumber = allReports[reportID].unreadActionCount === 0 From 9b783b5de9952f69988f1bde7baa47ee3453aec6 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Fri, 30 Apr 2021 12:27:11 +0200 Subject: [PATCH 16/20] remove unneeded method --- src/libs/actions/Report.js | 48 +++++++------------ .../home/report/ReportActionContextMenu.js | 7 ++- src/pages/home/report/ReportActionsView.js | 13 +++-- 3 files changed, 32 insertions(+), 36 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 765c2375b5d7..dde566502b61 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -389,24 +389,6 @@ function setLocalLastRead(reportID, lastReadSequenceNumber) { }); } -/** - * Set the New marker for a given report to the oldest unread sequence - * - * @param {Number} reportID - */ -function setNewMarkerInitialPosition(reportID) { - // If we have any unread actions, we determine the last read action by deduction the number of unread actions - // from the total number of actions in the report - const lastReadSequenceNumber = allReports[reportID].unreadActionCount === 0 - ? 0 - : reportMaxSequenceNumbers[reportID] - allReports[reportID].unreadActionCount; - - // Then, we set the New marker on the following action, which is the oldest unread one - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - newMarkerSequenceNumber: lastReadSequenceNumber + 1, - }); -} - /** * Remove all optimistic actions from report actions and reset the optimisticReportActionsIDs array. We do this * to clear any stuck optimistic actions that have not be updated for whatever reason. @@ -927,23 +909,17 @@ function addAction(reportID, text, file) { * network layer handle the delayed write. * * @param {Number} reportID - * @param {Number} [sequenceNumber] + * @param {Number} [sequenceNumber] This can be used to set the last read actionID to a specific + * spot (eg. mark-as-unread). Otherwise, when this param is omitted, the highest sequence number becomes the one that + * is last read (meaning that the entire report history has been read) */ function updateLastReadActionID(reportID, sequenceNumber) { - // If we are not specifying the sequence number, let's set it to the max one available, so that all actions in the - // report are considered "read". - const lastReadSequenceNumber = sequenceNumber || reportMaxSequenceNumbers[reportID]; + // Need to subtract 1 from sequenceNumber so that the "New" marker appears in the right spot (the last read + // action). If 1 isn't subtracted then the "New" marker appears one row below the action (the first unread action) + const lastReadSequenceNumber = (sequenceNumber - 1) || reportMaxSequenceNumbers[reportID]; setLocalLastRead(reportID, lastReadSequenceNumber); - // If we are specifying the last read sequence number, we are manually marking a comment as unread, so - // let's place new marker on the oldest unread sequence (after the last read sequence). - if (sequenceNumber) { - Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { - newMarkerSequenceNumber: sequenceNumber + 1, - }); - } - // Mark the report as not having any unread items API.Report_UpdateLastRead({ accountID: currentUserAccountID, @@ -952,6 +928,16 @@ function updateLastReadActionID(reportID, sequenceNumber) { }); } +/** + * @param {Number} reportID + * @param {Number} sequenceNumber + */ +function setNewMarkerPosition(reportID, sequenceNumber) { + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { + newMarkerSequenceNumber: sequenceNumber, + }); +} + /** * Toggles the pinned state of the report. * @@ -1036,7 +1022,7 @@ export { fetchOrCreateChatReport, addAction, updateLastReadActionID, - setNewMarkerInitialPosition, + setNewMarkerPosition, subscribeToReportTypingEvents, subscribeToUserEvents, unsubscribeFromReportChannel, diff --git a/src/pages/home/report/ReportActionContextMenu.js b/src/pages/home/report/ReportActionContextMenu.js index 6667e049be3b..5edbd19eff68 100644 --- a/src/pages/home/report/ReportActionContextMenu.js +++ b/src/pages/home/report/ReportActionContextMenu.js @@ -7,7 +7,7 @@ import { Clipboard as ClipboardIcon, LinkCopy, Mail, Pencil, Trashcan, Checkmark, } from '../../../components/Icon/Expensicons'; import getReportActionContextMenuStyles from '../../../styles/getReportActionContextMenuStyles'; -import {updateLastReadActionID} from '../../../libs/actions/Report'; +import {setNewMarkerPosition, updateLastReadActionID} from '../../../libs/actions/Report'; import ReportActionContextMenuItem from './ReportActionContextMenuItem'; import ReportActionPropTypes from './ReportActionPropTypes'; import Clipboard from '../../../libs/Clipboard'; @@ -78,7 +78,10 @@ class ReportActionContextMenu extends React.Component { icon: Mail, successIcon: Checkmark, shouldShow: true, - onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber - 1), + onPress: () => { + updateLastReadActionID(this.props.reportID, this.props.reportAction.sequenceNumber); + setNewMarkerPosition(this.props.reportID, this.props.reportAction.sequenceNumber); + }, }, { diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index ad6bbe978a76..89f98d340f86 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -13,7 +13,7 @@ import Text from '../../../components/Text'; import { fetchActions, updateLastReadActionID, - setNewMarkerInitialPosition, + setNewMarkerPosition, subscribeToReportTypingEvents, unsubscribeFromReportChannel, } from '../../../libs/actions/Report'; @@ -105,8 +105,15 @@ class ReportActionsView extends React.Component { this.keyboardEvent = Keyboard.addListener('keyboardDidShow', this.scrollToListBottom); updateLastReadActionID(this.props.reportID); - // Since we want the New marker to remain in place even if newer messages come in, we set it once on mount - setNewMarkerInitialPosition(this.props.reportID); + // Since we want the New marker to remain in place even if newer messages come in, we set it once on mount. + // We determine the last read action by deducting the number of unread actions from the total number. + const lastReadSequenceNumber = this.props.report.unreadActionCount === 0 + ? 0 + : this.props.report.maxSequenceNumber - this.props.report.unreadActionCount; + + // We are adding 1 to the last read sequence number because we want the New marker displayed over the + // first unread sequence + setNewMarkerPosition(this.props.reportID, lastReadSequenceNumber + 1); fetchActions(this.props.reportID); Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD); } From cc928de3a450313f14ec4b736b91d184a2d59439 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Mon, 3 May 2021 14:02:45 +0200 Subject: [PATCH 17/20] remove zero default --- src/libs/actions/Report.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index dde566502b61..d06e5e0be23d 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -924,7 +924,7 @@ function updateLastReadActionID(reportID, sequenceNumber) { API.Report_UpdateLastRead({ accountID: currentUserAccountID, reportID, - sequenceNumber: lastReadSequenceNumber || 0, + sequenceNumber: lastReadSequenceNumber, }); } From b098d1c94ca5b7a54499529ef7e1fab886e5c4d3 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Wed, 5 May 2021 10:23:27 +0200 Subject: [PATCH 18/20] Do not display the marker at the top of the report --- src/pages/home/report/ReportActionsView.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 89f98d340f86..bea89805a367 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -113,7 +113,9 @@ class ReportActionsView extends React.Component { // We are adding 1 to the last read sequence number because we want the New marker displayed over the // first unread sequence - setNewMarkerPosition(this.props.reportID, lastReadSequenceNumber + 1); + if (lastReadSequenceNumber > 0) { + setNewMarkerPosition(this.props.reportID, lastReadSequenceNumber + 1); + } fetchActions(this.props.reportID); Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD); } From 97869a44dc512c657d8a171212816b6f62883e5d Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Wed, 5 May 2021 10:23:41 +0200 Subject: [PATCH 19/20] add comment --- src/pages/home/report/ReportActionsView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index bea89805a367..0c1b7d1e82bc 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -112,7 +112,7 @@ class ReportActionsView extends React.Component { : this.props.report.maxSequenceNumber - this.props.report.unreadActionCount; // We are adding 1 to the last read sequence number because we want the New marker displayed over the - // first unread sequence + // first unread sequence. If there are no unread actions, there's no need to display it. if (lastReadSequenceNumber > 0) { setNewMarkerPosition(this.props.reportID, lastReadSequenceNumber + 1); } From 0f4c6c81590f9e26e7df9fe15b18a0a64d51f1d6 Mon Sep 17 00:00:00 2001 From: "alberto@expensify.com" Date: Wed, 5 May 2021 19:01:29 +0200 Subject: [PATCH 20/20] Only add +1 when needed --- src/pages/home/report/ReportActionsView.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 0c1b7d1e82bc..f267ef26880c 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -107,15 +107,13 @@ class ReportActionsView extends React.Component { // Since we want the New marker to remain in place even if newer messages come in, we set it once on mount. // We determine the last read action by deducting the number of unread actions from the total number. - const lastReadSequenceNumber = this.props.report.unreadActionCount === 0 + // Then, we add 1 because we want the New marker displayed over the oldest unread sequence. + const oldestUnreadSequenceNumber = this.props.report.unreadActionCount === 0 ? 0 - : this.props.report.maxSequenceNumber - this.props.report.unreadActionCount; + : (this.props.report.maxSequenceNumber - this.props.report.unreadActionCount) + 1; + + setNewMarkerPosition(this.props.reportID, oldestUnreadSequenceNumber); - // We are adding 1 to the last read sequence number because we want the New marker displayed over the - // first unread sequence. If there are no unread actions, there's no need to display it. - if (lastReadSequenceNumber > 0) { - setNewMarkerPosition(this.props.reportID, lastReadSequenceNumber + 1); - } fetchActions(this.props.reportID); Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD); }