Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix switching Priority Mode with an active draft message from crashing app #7907

Merged
merged 11 commits into from
Mar 4, 2022
26 changes: 19 additions & 7 deletions src/pages/home/sidebar/SidebarLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class SidebarLinks extends React.Component {
return true;
}

// Do not re-order if the active report has a draft and vice versa.
// Do not re-order if the active report has a draft
if (nextProps.currentlyViewedReportID) {
const hasActiveReportDraft = lodashGet(nextProps.reportsWithDraft, `${ONYXKEYS.COLLECTION.REPORTS_WITH_DRAFT}${nextProps.currentlyViewedReportID}`, false);
return !hasActiveReportDraft;
Expand All @@ -158,23 +158,35 @@ class SidebarLinks extends React.Component {
this.state = {
currentlyViewedReportID: props.currentlyViewedReportID,
orderedReports: [],
priorityMode: props.priorityMode,
unreadReports: SidebarLinks.getUnreadReports(props.reports || {}),
};
}

static getDerivedStateFromProps(nextProps, prevState) {
const shouldReorder = SidebarLinks.shouldReorder(nextProps, prevState.orderedReports, prevState.currentlyViewedReportID, prevState.unreadReports);
const switchingPriorityModes = nextProps.priorityMode !== prevState.priorityMode;
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 compare this.props.priorityMode with nextProps.priorityMode and leave state out of the equation entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.props isn't available within getDerivedStateFromProps for some reason.

Out of curiosity, why do you want to leave state out of the equation? Bc we are not using state.priorityMode anywhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bc we are not using state.priorityMode anywhere else

That, and we're just setting whatever is in props directly to state which usually means it does not need to be there. If we need to track the updates of a prop then we can just look at the prop.

Anyway, I think we are basically stuck at this point and would need to refactor the entire component to fix this. I'm remembering now that this is a static method and can't access the component instance at all.

There are some good explanations here that go into why we should not use this here (but hopefully the explanation I just gave also makes sense).

https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

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 would actually be super interested in refactoring the entire component

However, should I scale back some of the changes then to get somethign merged now? Right now the undefineds are causing a crash, or requires a hard refresh

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah! I created this issue here - feel free to steal it from me.

But agree we should just fix this for now :D


// Pull the reports we want to show on the left hand nav, ordered by priority
ctkochan22 marked this conversation as resolved.
Show resolved Hide resolved
const recentReports = SidebarLinks.getRecentReports(nextProps);
const orderedReports = shouldReorder

// Determine whether we need to keep the previous LHN order
const orderedReports = shouldReorder || switchingPriorityModes
? recentReports
: _.map(prevState.orderedReports,
orderedReport => _.chain(recentReports)
.filter(recentReport => orderedReport.reportID === recentReport.reportID)
.first()
.value());
: _.chain(prevState.orderedReports)

// To preserve the order of the conversations, we map over the previous state's order of reports.
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
// Then match and replace older reports with the newer report conversations from recentReports
.map(orderedReport => _.find(recentReports, recentReport => orderedReport.reportID === recentReport.reportID))
marcaaron marked this conversation as resolved.
Show resolved Hide resolved

// Because we are using map, we have to filter out any undefined reports. This happens if recentReports
// does not have all the conversations in prevState.orderedReports
.filter(orderedReport => orderedReport !== undefined)
.value();

return {
orderedReports,
priorityMode: nextProps.priorityMode,
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
currentlyViewedReportID: nextProps.currentlyViewedReportID,
unreadReports: SidebarLinks.getUnreadReports(nextProps.reports || {}),
};
Expand Down