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

14893: Edit message arrow up #15207

Merged
merged 17 commits into from
Mar 6, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
85 changes: 83 additions & 2 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import reportPropTypes from '../reportPropTypes';
import FullPageNotFoundView from '../../components/BlockingViews/FullPageNotFoundView';
import ReportHeaderSkeletonView from '../../components/ReportHeaderSkeletonView';
import withViewportOffsetTop, {viewportOffsetTopPropTypes} from '../../components/withViewportOffsetTop';
import * as ReportActionsUtils from '../../libs/ReportActionsUtils';

const propTypes = {
/** Navigation route context info provided by react navigation */
Expand Down Expand Up @@ -111,6 +112,9 @@ class ReportScreen extends React.Component {
this.chatWithAccountManager = this.chatWithAccountManager.bind(this);
this.dismissBanner = this.dismissBanner.bind(this);

// We need this.sortedAndFilteredReportActions to be set before this.state is initialized because the function to calculate the newMarkerReportActionID uses the sorted report actions
this.sortedAndFilteredReportActions = this.getSortedReportActionsForDisplay(props.reportActions);

this.state = {
skeletonViewContainerHeight: reportActionsListViewHeight,
isBannerVisible: true,
Expand All @@ -123,6 +127,60 @@ class ReportScreen extends React.Component {
Navigation.setIsReportScreenIsReady();
}

shouldComponentUpdate(nextProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we return true if this.props.report changed?
I found regression (not refreshing) while adding/removing members.
i.e.

  • Let's say 2 members (including me) in #announce room!
    -This should not show Split bill on + menu.
  • Login same account in another platform
  • Add member to this workspace on that platform
  • Now back to original platform and click + and this still doesn't show Split bill
    This works fine on production

This happens because report.participants updated but not re-rendered

Copy link
Contributor

Choose a reason for hiding this comment

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

can we return true if this.props.report changed?

I am asking this because this already does in main

Copy link
Contributor

Choose a reason for hiding this comment

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

@0xmiroslav so this is not a regression from this PR? I think if not, we can continue here and you can file a bug report for this case

Copy link
Contributor

@0xmiros 0xmiros Feb 24, 2023

Choose a reason for hiding this comment

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

This PR causes that regression.

I am asking this because this already does in main

Here, does means re-renders because ReportScreen doesn't either extend PureComponent or use shouldComponentUpdate

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I don't think we need shouldComponentUpdate in ReportScreen

Copy link
Contributor

Choose a reason for hiding this comment

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

@gedu For what optimization did you introduce shouldComponentUpdate in ReportScreen?

Copy link
Contributor Author

@gedu gedu Feb 27, 2023

Choose a reason for hiding this comment

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

@0xmiroslav Hey, I've been looking at what you wrote and couldn't reproduce it like that, I could, following other steps, and only on web, mobile is working as expected.

Screen.Recording.2023-02-27.at.09.28.55.mp4

Let's say 2 members (including me) in #announce room!
-This should not show Split bill on + menu.

On the other hand, looking at the code, I see #announce and #admins always will show Split Bill on the + menu (if there are 2 members). This was working (above in the video showing the only case isn't working).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the check to the reports as you suggested and is fixing the case I found, doing some round of testing and uploading fix, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@gedu you opened production site on left. please run this PR on both sides.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed it here and we determined that it's ok to filter/sort in render(). I think that we should do that and remove the derived state.

if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) {
this.sortedAndFilteredReportActions = this.getSortedReportActionsForDisplay(nextProps.reportActions);

return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@gedu I am not very happy with moving all of this logic up which mens amore components will have to re-render because of this. Could we keep it in the ReportActionsView and move the getSortedReportActionsForDisplay method to ReportActionsUtils instead since it is itself utils method.

Then we could get the sorted report actions in the ReportScreen, pass it down to the Report screen components and we can call the ReportActionsUtils.getSortedReportActionsForDisplay from the ReportActionsView if needed.

Is there anything I am missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mountiny We needed to move up so the ReportActionsView and ReportFooter receives the same sorted list.

I could move getSortedReportActionsForDisplay into a utils file, but we should keep the sorting at the top.

About the rerender, it will be the same as before, because when ReportScreen needs to rerender when reportActions changes, was rerendering both components ReportActionsView and ReportFooter..., the first one to sort the actions again, and the Footer because of this arrow up feature among others props.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for context.


if (nextProps.report.isLoadingMoreReportActions !== this.props.report.isLoadingMoreReportActions) {
return true;
}

if (nextProps.report.isLoadingReportActions !== this.props.report.isLoadingReportActions) {
return true;
}

if (nextProps.report.lastReadTime !== this.props.report.lastReadTime) {
return true;
}

if (this.props.isSmallScreenWidth !== nextProps.isSmallScreenWidth) {
return true;
}

if (this.props.isDrawerOpen !== nextProps.isDrawerOpen) {
return true;
}

if (lodashGet(this.props.report, 'hasOutstandingIOU') !== lodashGet(nextProps.report, 'hasOutstandingIOU')) {
return true;
}

if (this.props.isComposerFullSize !== nextProps.isComposerFullSize) {
return true;
}

if (this.props.isSidebarLoaded !== nextProps.isSidebarLoaded) {
return true;
}

if (this.props.personalDetails !== nextProps.personalDetails) {
return true;
}

if (this.props.policies !== nextProps.policies) {
return true;
}

if (this.props.betas !== nextProps.betas) {
return true;
}

return !_.isEqual(lodashGet(this.props.report, 'icons', []), lodashGet(nextProps.report, 'icons', []));
}

componentDidUpdate(prevProps) {
if (this.props.route.params.reportID === prevProps.route.params.reportID) {
return;
Expand All @@ -139,6 +197,29 @@ class ReportScreen extends React.Component {
Report.addComment(getReportID(this.props.route), text);
}

/**
* @param {Object} reportActions
* @returns {Array}
*/
getSortedReportActionsForDisplay(reportActions) {
// HACK ALERT: We're temporarily filtering out any reportActions keyed by sequenceNumber
// to prevent bugs during the migration from sequenceNumber -> reportActionID
const filteredReportActions = _.filter(reportActions, (reportAction, key) => {
if (!reportAction) {
return false;
}

if (String(reportAction.sequenceNumber) === key) {
return false;
}

return true;
});

const sortedReportActions = ReportActionsUtils.getSortedReportActions(filteredReportActions, true);
return ReportActionsUtils.filterReportActionsForDisplay(sortedReportActions);
}

/**
* When false the ReportActionsView will completely unmount and we will show a loader until it returns true.
*
Expand Down Expand Up @@ -269,7 +350,7 @@ class ReportScreen extends React.Component {
{(this.isReportReadyForDisplay() && !isLoadingInitialReportActions) && (
<>
<ReportActionsView
reportActions={this.props.reportActions}
reportActions={this.sortedAndFilteredReportActions}
report={this.props.report}
session={this.props.session}
isComposerFullSize={this.props.isComposerFullSize}
Expand All @@ -280,7 +361,7 @@ class ReportScreen extends React.Component {
errors={addWorkspaceRoomOrChatErrors}
pendingAction={addWorkspaceRoomOrChatPendingAction}
isOffline={this.props.network.isOffline}
reportActions={this.props.reportActions}
reportActions={this.sortedAndFilteredReportActions}
report={this.props.report}
isComposerFullSize={this.props.isComposerFullSize}
onSubmitComment={this.onSubmitComment}
Expand Down
15 changes: 7 additions & 8 deletions src/pages/home/report/ReportActionCompose.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const propTypes = {
report: reportPropTypes,

/** Array of report actions for this report */
reportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),
reportActions: PropTypes.arrayOf(PropTypes.shape(reportActionPropTypes)),

/** Is the report view covered by the drawer */
isDrawerOpen: PropTypes.bool.isRequired,
Expand Down Expand Up @@ -110,7 +110,7 @@ const defaultProps = {
comment: '',
modal: {},
report: {},
reportActions: {},
reportActions: [],
blockedFromConcierge: {},
personalDetails: {},
...withCurrentUserPersonalDetailsDefaultProps,
Expand Down Expand Up @@ -453,14 +453,13 @@ class ReportActionCompose extends React.Component {
if (e.key === 'ArrowUp' && this.textInput.selectionStart === 0 && this.state.isCommentEmpty && !ReportUtils.chatIncludesChronos(this.props.report)) {
e.preventDefault();

const reportActionKey = _.find(
_.keys(this.props.reportActions).reverse(),
key => ReportUtils.canEditReportAction(this.props.reportActions[key]),
const lastReportAction = _.find(
this.props.reportActions,
action => ReportUtils.canEditReportAction(action),
);

if (reportActionKey !== -1 && this.props.reportActions[reportActionKey]) {
const {reportActionID, message} = this.props.reportActions[reportActionKey];
Report.saveReportActionDraft(this.props.reportID, reportActionID, _.last(message).html);
if (lastReportAction !== -1 && lastReportAction) {
Report.saveReportActionDraft(this.props.reportID, lastReportAction.reportActionID, _.last(lastReportAction.message).html);
}
}
}
Expand Down
22 changes: 9 additions & 13 deletions src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,9 @@ class ReportActionsView extends React.Component {
this.unsubscribeVisibilityListener = null;
this.hasCachedActions = _.size(props.reportActions) > 0;

// We need this.sortedAndFilteredReportActions to be set before this.state is initialized because the function to calculate the newMarkerReportActionID uses the sorted report actions
this.sortedAndFilteredReportActions = this.getSortedReportActionsForDisplay(props.reportActions);

this.state = {
isFloatingMessageCounterVisible: false,
newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.sortedAndFilteredReportActions),
newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, props.reportActions),
};

this.currentScrollOffset = 0;
Expand Down Expand Up @@ -133,7 +130,6 @@ class ReportActionsView extends React.Component {

shouldComponentUpdate(nextProps, nextState) {
if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) {
this.sortedAndFilteredReportActions = this.getSortedReportActionsForDisplay(nextProps.reportActions);
this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions);
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required in here as well if we have added the logic to the ReportScreen?

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, the mostRecentIOUReportActionID is used just in this component and sends it down, the ReportScreen now just is taking care of the sorting, the logic for mostRecentIOUReportActionID isn't in ReportScreen

Copy link
Contributor

Choose a reason for hiding this comment

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

@gedu sorry I wan not clear, I meant the big chunk of the shouldComponentUpdate is now duplicated, do we need to check for the changes in these reportActionViews? If ReportScreen re-renders than this rerenders too, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhhh got it, yes it is true, my bad, I miss it after some merge conflicts, updating PR

Expand Down Expand Up @@ -204,16 +200,17 @@ class ReportActionsView extends React.Component {
if (didReportBecomeVisible) {
this.setState({
newMarkerReportActionID: ReportUtils.isUnread(this.props.report)
? ReportUtils.getNewMarkerReportActionID(this.props.report, this.sortedAndFilteredReportActions)
? ReportUtils.getNewMarkerReportActionID(this.props.report, this.props.reportActions)
: '',
});
this.openReportIfNecessary();
}

// If the report action marking the unread point is deleted we need to recalculate which action should be the unread marker
if (this.state.newMarkerReportActionID && _.isEmpty(lodashGet(this.props.reportActions[this.state.newMarkerReportActionID], 'message[0].html'))) {
// If the report is unread, we want to check if the number of actions has decreased. If so, then it seems that one of them was deleted. In this case, if the deleted action was the
// one marking the unread point, we need to recalculate which action should be the unread marker.
if (ReportUtils.isUnread(this.props.report) && ReportActionsUtils.filterReportActionsForDisplay(prevProps.reportActions).length > this.props.reportActions.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no filterReportActionsForDisplay function defined in ReportActionsUtils

Copy link
Contributor

Choose a reason for hiding this comment

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

It was removed here c351930

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look at it thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, please remove here as well. App crashes right now when other sends message which makes isUnread true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, cool, I will update it

this.setState({
newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.sortedAndFilteredReportActions),
newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.props.reportActions),
});
}

Expand All @@ -230,7 +227,7 @@ class ReportActionsView extends React.Component {
const didManuallyMarkReportAsUnread = (prevProps.report.lastReadTime !== this.props.report.lastReadTime)
&& ReportUtils.isUnread(this.props.report);
if (didManuallyMarkReportAsUnread) {
this.setState({newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.sortedAndFilteredReportActions)});
this.setState({newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.props.reportActions)});
}

// Ensures subscription event succeeds when the report/workspace room is created optimistically.
Expand Down Expand Up @@ -308,7 +305,7 @@ class ReportActionsView extends React.Component {
return;
}

const oldestReportAction = _.last(this.sortedAndFilteredReportActions);
const oldestReportAction = _.last(this.props.reportActions);

// Don't load more chats if we're already at the beginning of the chat history
if (oldestReportAction.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) {
Expand Down Expand Up @@ -372,7 +369,6 @@ class ReportActionsView extends React.Component {
if (!_.size(this.props.reportActions)) {
return null;
}

return (
<>
{!this.props.isComposerFullSize && (
Expand All @@ -385,7 +381,7 @@ class ReportActionsView extends React.Component {
report={this.props.report}
onScroll={this.trackScroll}
onLayout={this.recordTimeToMeasureItemLayout}
sortedReportActions={this.sortedAndFilteredReportActions}
sortedReportActions={this.props.reportActions}
mostRecentIOUReportActionID={this.mostRecentIOUReportActionID}
isLoadingMoreReportActions={this.props.report.isLoadingMoreReportActions}
loadMoreChats={this.loadMoreChats}
Expand Down
4 changes: 2 additions & 2 deletions src/pages/home/report/ReportFooter.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const propTypes = {
report: reportPropTypes,

/** Report actions for the current report */
reportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),
reportActions: PropTypes.arrayOf(PropTypes.shape(reportActionPropTypes)),

/** Offline status */
isOffline: PropTypes.bool.isRequired,
Expand All @@ -50,7 +50,7 @@ const propTypes = {

const defaultProps = {
report: {reportID: '0'},
reportActions: {},
reportActions: [],
onSubmitComment: () => {},
errors: {},
pendingAction: null,
Expand Down