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

Jump to bottom

14893: Edit message arrow up #15207

Merged
merged 17 commits into from
Mar 6, 2023
Merged

14893: Edit message arrow up #15207

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4ef5645
Moving up the reportActions sorting
gedu Feb 15, 2023
0c821ff
Merge branch 'main' into 14893/edit_message_arrow_up
gedu Feb 16, 2023
0d1f768
Updated props and removed shouldComponentUpdate
gedu Feb 16, 2023
0d91a37
Fixed isPinned
gedu Feb 16, 2023
17dc211
Merge branch 'main' into 14893/edit_message_arrow_up
gedu Feb 23, 2023
f54539a
replace local function for utils
gedu Feb 23, 2023
6091178
Merge branch 'main' into 14893/edit_message_arrow_up
gedu Feb 24, 2023
e1834f1
removed call to unexisting function
gedu Feb 24, 2023
5ec7c39
added participants check to rerender the report screen component
gedu Feb 27, 2023
8114e7d
removed shouldComponentUpdate and sort the reports on componentDidUpdate
gedu Feb 28, 2023
112ce8a
added the sorted report actions into the state and moved back the sho…
gedu Mar 1, 2023
8118c20
Merge branch 'main' into 14893/edit_message_arrow_up
gedu Mar 1, 2023
9444c98
moved up the getLastClosedAction function to handle archived room rea…
gedu Mar 1, 2023
2b1a943
Changed the sorting to be in Onyx select
gedu Mar 3, 2023
42ce483
ArchivedReportFooter listening directly the reportsAction and getting…
gedu Mar 6, 2023
6784b28
Merge branch 'main' into 14893/edit_message_arrow_up
gedu Mar 6, 2023
ab82acd
Updated prop type from reportActions
gedu Mar 6, 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
66 changes: 64 additions & 2 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
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 = ReportActionsUtils.getSortedReportActionsForDisplay(props.reportActions);

this.state = {
skeletonViewContainerHeight: reportActionsListViewHeight,
isBannerVisible: true,
Expand All @@ -123,6 +127,64 @@ 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 •
edited
Loading

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 •
edited
Loading

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 = ReportActionsUtils.getSortedReportActionsForDisplay(nextProps.reportActions);

return true;
}

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

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

if (nextProps.report.isPinned !== this.props.report.isPinned) {
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 Down Expand Up @@ -269,7 +331,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 +342,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,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 @@ -108,7 +108,7 @@ const defaultProps = {
comment: '',
modal: {},
report: {},
reportActions: {},
reportActions: [],
blockedFromConcierge: {},
personalDetails: {},
...withCurrentUserPersonalDetailsDefaultProps,
Expand Down Expand Up @@ -431,14 +431,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
75 changes: 11 additions & 64 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const propTypes = {
report: reportPropTypes.isRequired,

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

/** The session of the logged in person */
session: PropTypes.shape({
Expand All @@ -49,7 +49,7 @@ const propTypes = {
};

const defaultProps = {
reportActions: {},
reportActions: [],
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
session: {},
};

Expand All @@ -62,12 +62,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 = ReportActionsUtils.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 @@ -130,56 +127,6 @@ class ReportActionsView extends React.Component {
});
}

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

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

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 (nextState.isFloatingMessageCounterVisible !== this.state.isFloatingMessageCounterVisible) {
return true;
}

if (nextState.newMarkerReportActionID !== this.state.newMarkerReportActionID) {
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;
}

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

componentDidUpdate(prevProps) {
const isReportFullyVisible = this.getIsReportFullyVisible();

Expand All @@ -203,16 +150,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 @@ -229,7 +177,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 @@ -283,7 +231,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 @@ -347,7 +295,6 @@ class ReportActionsView extends React.Component {
if (!_.size(this.props.reportActions)) {
return null;
}

return (
<>
{!this.props.isComposerFullSize && (
Expand All @@ -360,7 +307,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
View file
Open in desktop
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