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

ctkochan22 · 2022-02-25T05:45:35Z

@Expensify/pullerbear
cc @marcaaron

Details

This was a tricky bug to find. I'm mostly worried about regression so ideally we can really test this thoroughly

Fixed Issues

$ #7810
$ #7811

Tests / QA Steps

Fix tests

Go to Settings -> Preferences. Change the priority mode to "#focus". Verify that the LHN condenses
Then change the priority mode back to "Most Recent". Verify that the LHN expands
Now go back to the chat and type text in a chat. Verify that that user now has a draft "pencil" icon
Go back to Settings -> Preferences. Change the priority mode to "#focus". Verify that the LHN condenses, and the user with the draft still has a pencil.
- This is where the app crashes without this change
Change the priority mode to "Most Recent". Verify that the LHN expands

Regression
Part of the bug was introduced in this PR (#6084), so lets take their tests word for word and test them

Type text in the chat without sending it
- Verify that you see the pencil next to the name and the order does not re-order
Type text in chat and send it
- Verify that the pencil appears and disappears once its sent. The LHN order should change. User that you messaged should go to the top (but below pinned users)
Type text in he chat positioned in the middle. Then click on another contact.
- The pencil icon should remain. User with an unsent draft should move to the top
Go to the user that had the draft message and delete it.
- Pencil icon should disappear and order should re-order
As a different user, message the first user. The message sender should move to the top
- Verify that the message sender moves to the top

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Tested On

Screenshots

Web

Above

ctkochan22 · 2022-02-25T05:46:04Z

Actually, filter is not the solution. We use map because we need to replace old report object with new ones

marcaaron

Changes look good, but I did have a question or two. I found this file really hard to follow and think it could be improved, but no expectations to do that in this PR. 😄

marcaaron · 2022-02-28T23:20:50Z

src/pages/home/sidebar/SidebarLinks.js

            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;


Can we compare this.props.priorityMode with nextProps.priorityMode and leave state out of the equation entirely?

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?

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

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

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

But agree we should just fix this for now :D

src/pages/home/sidebar/SidebarLinks.js

ctkochan22 · 2022-03-01T02:45:51Z

Updated. I tested it on all platforms, videos of the tests above. Definitely encouraged to test though! Added a lot of regression testing as well

LGTM now and agree on the plan forward.

ctkochan22 · 2022-03-01T23:12:50Z

@pecanoro friendly bump!

OSBotify · 2022-03-04T20:54:15Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-03-11T01:00:26Z

🚀 Deployed to staging by @pecanoro in version: 1.1.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-03-15T20:53:09Z

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

ctkochan22 added 2 commits February 23, 2022 23:18

fix crash

b9199e1

always reorder when changing focus

f3e59cb

ctkochan22 requested a review from a team as a code owner February 25, 2022 05:45

ctkochan22 self-assigned this Feb 25, 2022

MelvinBot requested review from pecanoro and removed request for a team February 25, 2022 05:46

ctkochan22 added 5 commits February 26, 2022 15:27

fix fix

89eb09c

add comments because it took me several tries

b88a9fa

revert removing reorder condition

da4dc8a

clean it up a bit

edb7149

factor in priorityMode switching

d69f3bd

ctkochan22 requested a review from marcaaron February 28, 2022 22:19

ctkochan22 changed the title ~~[HOLD] Fix switching Priority Mode with an active draft message from crashing app~~ Fix switching Priority Mode with an active draft message from crashing app Feb 28, 2022

ctkochan22 added 3 commits February 28, 2022 14:20

Merge branch 'main' into ckt_focus_fixDraft

208d137

fix comment

28d0309

clarify

88ea587

marcaaron previously requested changes Feb 28, 2022

View reviewed changes

clarify comment

c9fcea4

pecanoro approved these changes Mar 4, 2022

View reviewed changes

pecanoro merged commit 5ceaf62 into main Mar 4, 2022

pecanoro deleted the ckt_focus_fixDraft branch March 4, 2022 20:45

OSBotify mentioned this pull request Mar 10, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

ctkochan22 mentioned this pull request Mar 11, 2022

[$1000] Desktop app is crashing #7642

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ctkochan22 commented Feb 25, 2022 •

edited

Loading

ctkochan22 commented Feb 25, 2022

marcaaron left a comment •

edited

Loading

marcaaron Feb 28, 2022

ctkochan22 Mar 1, 2022

marcaaron Mar 1, 2022

ctkochan22 Mar 1, 2022

marcaaron Mar 1, 2022

ctkochan22 commented Mar 1, 2022

ctkochan22 commented Mar 1, 2022

OSBotify commented Mar 4, 2022

OSBotify commented Mar 11, 2022

OSBotify commented Mar 15, 2022

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

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

Conversation

ctkochan22 commented Feb 25, 2022 • edited Loading

Details

Fixed Issues

Tests / QA Steps

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Tested On

Screenshots

Web

Desktop

iOS

Android

ctkochan22 commented Feb 25, 2022

marcaaron left a comment • edited Loading

Choose a reason for hiding this comment

marcaaron Feb 28, 2022

Choose a reason for hiding this comment

ctkochan22 Mar 1, 2022

Choose a reason for hiding this comment

marcaaron Mar 1, 2022

Choose a reason for hiding this comment

ctkochan22 Mar 1, 2022

Choose a reason for hiding this comment

marcaaron Mar 1, 2022

Choose a reason for hiding this comment

ctkochan22 commented Mar 1, 2022

ctkochan22 commented Mar 1, 2022

OSBotify commented Mar 4, 2022

OSBotify commented Mar 11, 2022

OSBotify commented Mar 15, 2022

ctkochan22 commented Feb 25, 2022 •

edited

Loading

marcaaron left a comment •

edited

Loading