iOS - Previous conversation is briefly visible when opening a new chat

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open iOS e.cash app and log in
2. Navigate to a conversation
3. Go back to LHN
4. Open a different conversation

Expected Result:

New chat should be displayed.

Actual Result:

Previous conversation is briefly visible before displaying the correct chat.

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.73-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:
Issue not reproducible in the Android app.

Image.from.iOS.2.MP4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

---

From @puneetlath https://expensify.slack.com/archives/C01GTK53T8Q/p1624380109498500

ISSUE: on iOS staging:
tap into a chat
tap back to the home screen
tap into a different chat
the last chat you were in will flash on the screen before the new chat shows

[MelvinBot]

Triggered auto assignment to @roryabraham (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[roryabraham]

Sorry for the delay here, let's get this on Upwork!

[rdjuric]

For posterity's sake: We had another issue #2154 open for this before, and a PR #2221 was made to try and fix it.

I think one of the problems with our current fix is that this code block is never executed the way we intended
https://github.com/Expensify/Expensify.cash/blob/c5707430dff9ecfb631012bb0ba25357c7f367e5/src/pages/home/ReportScreen.js#L37-L44
That's because our component is remounted whenever we change our reportID and the ComponentDidUpdate lifecycle isn't called on the first render. This means that in our implementation, every time ComponentDidUpdate is called our component will have updated its props already (prevProps === this.props) and our reportChanged check will always return false.

Proposal

1. Delete the ComponentDidUpdate lifecycle
2. Conditionally render our ReportView here, something like
   {!this.shouldShowLoader() && <ReportView reportID={this.getReportID()} />}
3. This gives us the behavior below

Simulator.Screen.Recording.-.iPhone.SE.14.5.-.2021-07-01.at.02.21.29.mp4

[roryabraham]

Thanks for the explanation @rdjuric!

our component is remounted whenever we change our reportID

It seems like this was not the intention when this PR was written, and I wonder if this is the real source of the issue. Do you know why the ReportScreen remounts entirely when the reportID changes? I see that the parent component is only passing a reportID as a route param, but why would changing the route prop trigger a full remount of the component?

[rdjuric]

@roryabraham Good questions!

I don't think the remount is the cause since our ComponentDidUpdate would do the same thing as ComponentDidMount does (in case our reportChanged check ever returned true).

i.e remounting is also calling this.prepareTransition() correctly when we change reports, and adding a few logs shows that our FullScreenLoadingIndicator is also being set to visible as it should.

I think the problem is really that we're trying to render both the FullScreenLoadingIndicator and the ReportView while loading. So if our ReportView takes 300ms or more to be painted, our LoadingIndicator isn't shown at all and we're stuck on the old ReportView during the whole transition.

I'll talk a bit about the remounts in the next comment.

[rdjuric]

Do you know why the ReportScreen remounts entirely when the reportID changes? I see that the parent component is only passing a reportID as a route param, but why would changing the route prop trigger a full remount of the component?

It's not the prop change that is triggering the remount (the parent is also being remounted), but the way we navigate in our drawer navigator. This reset here causes the remounts.
https://github.com/Expensify/Expensify.cash/blob/870c3db1675bb294c8961a8db88bc1ffa276dea9/src/libs/Navigation/CustomActions.js#L27-L35

I modified it a bit so that it would stop remounting our routes and this issue kept happening

iOSNoRemount.mp4

[MitchExpensify]

My bad for the delay! Missed the "external" assignment on this one, here's the Upwork job.

[roryabraham]

Hmmm okay thanks for digging into this some more @rdjuric. I think that good enough for me to agree that we should just take the path of least resistance and move forward with your original proposed solution. Feel free to submit a PR once you're been hired on Upwork.

@MitchExpensify feel free to hire @rdjuric if/when he applies on Upwork.

[MitchExpensify]

Hired!

[rdjuric]

Sorry for the delay here @roryabraham! PR is up at #3908

[roryabraham]

Merged!

[mallenexpensify]

Shouldn't this issue have been closed once the PR was merged or when it hit production?
@MitchExpensify , can you review, pay in Upwork then close this issue?
I went to do it in Upwork but saw this message from you and got confused

[rdjuric]

Shouldn't this issue have been closed once the PR was merged or when it hit production?
@MitchExpensify , can you review, pay in Upwork then close this issue?
I went to do it in Upwork but saw this message from you and got confused

I think this is the message I sent to mitch on Upwork 😅

[MitchExpensify]

Paid! Apologies for the delay @rdjuric

[mallenexpensify]

@rdjuric thanks for comment, makes sense now.
@arielgreen going back to 'shouldn't this issue have been closed' - Our new process will NOT close the issue so what I should have said was 'shouldn't an automated comment have been added stating that the PR had been merged?

Also, our new process will be adding Awaiting payment and Weekly labels to issue once the PR is merged so that contributor managers will know to pay in a week

[arielgreen]

@mallenexpensify ah, yeah I think you're correct; might be due to what @MonilBhavsar was saying elsewhere, that since this is an older issue it wasn't in the DB so it didn't get the automated comment.