Fix flashing messages on iOS #2252

Maftalion · 2021-04-07T00:46:03Z

Details

Fixes weird behavior when messages flash upon sending a new message

Fixed Issues

Fixes #2205

Tests

Open any chat and send a message
Test both chats with full page of messages and newer chats

QA Steps

same as above

Tested On

Screenshots

Web

Screen.Recording.2021-04-05.at.12.44.36.PM.mov

Mobile Web

Screen.Recording.2021-04-05.at.12.49.42.PM.mov

Desktop

Screen.Recording.2021-04-05.at.12.50.51.PM.mov

iOS

Screen.Recording.2021-04-02.at.3.57.13.PM.mov

Android

Screen.Recording.2021-04-06.at.5.35.52.PM.mov

src/pages/home/report/ReportActionCompose.js

Beamanator

A few small comments with lots of explanation (sorry) - let me know if you agree / disagree / have questions!

src/pages/home/report/ReportActionsView.js

Beamanator · 2021-04-07T10:11:38Z

src/pages/home/report/ReportActionsView.js

@@ -373,7 +373,7 @@ class ReportActionsView extends React.Component {
                renderItem={this.renderItem}
                CellRendererComponent={this.renderCell}
                contentContainerStyle={[styles.chatContentScrollView]}
-                keyExtractor={item => `${item.action.sequenceNumber}`}
+                keyExtractor={item => `${item.action.timestamp}`}


Sadly, this isn't actually going to work out. It turns out, it's quite easy to hit a problem if you send messages quickly (more than one per second), since moment().unix() (the code creating this timestamp, in src/libs/actions/Report.js) easily becomes non-unique.

Instead, I'm thinking we can change this to item.action.clientID since this comes from optimisticReportActionID in Report.js, which is created with Date.now() (more digits than moment().unix() plus a random number (see https://github.com/Expensify/Expensify.cash/pull/1394/files where this was added).

From my investigation, this does still a slight issue where .clientID doesn't exist when initially merging the new action into Onyx (in Report.js -> addAction) since we don't set clientID there -> clientID is added in the server code later, which is how it comes back to the front-end code.

To fix this, I think we can just add clientID: optimisticReportActionID, when updating Onyx locally. Please let me know your thoughts on this 👍

Ah, yeah it seems to work with clientID as well. updating the pr

Maftalion · 2021-04-07T22:44:40Z

Hmm not sure if I'm running into a weird caching issue, but I can't seem to reproduce the "flashing" on latest master.

deetergp

Looks good!

Beamanator · 2021-04-08T08:02:32Z

Hmm not sure if I'm running into a weird caching issue, but I can't seem to reproduce the "flashing" on latest master.

@Maftalion Me either - I also tested master on my physical android device (which i hadn't done while testing your changes before) and I don't experience the flashing either... I'm going to ask a few co-workers to check as well

I wonder if it's the recent Onyx upgrades that fixed this 🤔 or something related to sequenceNumber

Beamanator · 2021-04-08T10:52:05Z

Alright it looks like we're good to close this, no changes needed since the error isn't occurring on the latest version (dev, not production) - tested by you & me + a few of my colleagues

The base branch was changed.

Maftalion · 2021-04-27T21:05:09Z

@Beamanator it looks like these changes still get rid of the "flashing" that reappeared. There is some new complexity with the IOU items, they don't come with a action.clientID so I can either fallback to sequenceNumber if clientID doesn't exist or I can check if actionName === 'IOU' and use actionName === 'IOU'. what are your thoughts?

Also would still need the change to scrollToListBottom to remove the bouncing that appears. Should I open a separate ticket for that as a followup?

Beamanator · 2021-04-29T14:53:50Z

Hey @Maftalion sorry for the delay!

I like the idea of falling back to sequenceNumber so we (hopefully) never get undefined / duplicate keys in the keyExtractor... I think we have to do a decent amount of testing to make sure the change does actually fix the problem - Currently I still can't even reproduce the issue on android or iOS 🤷‍♂️ were you ever able to consistently reproduce on the latest dev?

Also would still need the change to scrollToListBottom to remove the bouncing that appears. Should I open a separate ticket for that as a followup?

Yes please create another issue for this 👍

deetergp · 2021-04-30T22:37:51Z

Can confirm that the screen no longer flashes, but now we have a different behavior: newly-typed text appears in the chat history, then the entire chat history shifts down about half the font height:
https://user-images.githubusercontent.com/766197/116760418-b8dddb00-a9c9-11eb-81dd-213b1c1f59e4.mp4

deetergp · 2021-04-30T22:41:21Z

Neither behavior is happening in the main branch right now:
https://user-images.githubusercontent.com/766197/116760699-7a94eb80-a9ca-11eb-9ea2-2a9cee3187e4.mp4

Beamanator · 2021-05-05T11:37:20Z

I'm also not getting any flashing in the main branch... I guess we can close this again?

trjExpensify · 2021-05-05T14:41:06Z

The flashing is still happening on staging v1.0.37-0 (iOS). Here's another screen recording from today to confirm.

Image.from.iOS.2.MP4

Also, looks like another issue for this was created last night, so I've closed that dupe to focus our efforts here: #2693

deetergp · 2021-05-05T21:06:30Z

Hookay! I spent WAY too much time getting myself setup to test this on a physical device and then building several branches to test if the bug IS definitely still there in 1.0.37-0 (and -1), as well as in the main branch:
https://user-images.githubusercontent.com/766197/117208966-9b828580-adaa-11eb-9d8e-047e027d29ad.mp4

And while @Maftalion's branch does get rid of the flashing, it introduces a regression that I'll call the Donky Kong Effect that makes the chat history shudder like there's a large gorilla stomping at the top:
https://user-images.githubusercontent.com/766197/117209170-da184000-adaa-11eb-955d-aba5e149fcde.mp4

As such, testing on a physical device is required, and no regressions should be included.

Maftalion · 2021-05-07T15:15:09Z

@deetergp does this mean it's only reproducible on a physical device? also I refer to the regression issue as "bouncing" above but totally like your terminology better 😆 . It can be fixed by updating scrollToListBottom to use this.actionListElement.scrollToOffset({animated: false, offset: 0}); but was told to followup with that change in a different issue/pr

deetergp · 2021-05-07T17:40:43Z

@Maftalion It may well only be reproducible on a physical device--all the times I tried testing any branch in the simulator, I haven't seen the flashing occur, and your branch definitely makes it go away.

but was told to followup with that change in a different issue/pr

Ahh, that's what that was about. Has that fix already been merged into the main branch? If it has, can you pull main back into your branch and then we'll be good to go!

Beamanator · 2021-05-07T21:32:22Z

Hey guys, yeah that is probably my bad - I thought the ~~bouncing~~ Donkey Kong Effect was separate from the flashing issue, which is why I thought it best to separate the issues. If Matt's fix causes the Donkey Kong regression, which is easily fixed (with the fix he mentioned multiple times), let's combine them here and see if we can crush this issue once and for all :D

Maftalion · 2021-05-07T23:49:43Z

Awesome, sounds good! Just pushed up that change @Beamanator @deetergp

deetergp

Tests pass and code looks good! Thanks for sticking with it 👍

RPReplay_Final1620669015.mp4

Beamanator

Sorry to do this to you, but can you merge master here & fix the merge conflict before I test? :D

# Conflicts: # src/pages/home/report/ReportActionsView.js

Maftalion · 2021-05-11T16:34:35Z

@Beamanator fixed!

deetergp

Re-tested and it still works like a champ. Code looks good so let's get this merged ASAP, before more conflicts pop up!

Beamanator

Tests well for me too! LGTM :D

Thanks @deetergp & @Maftalion !

OSBotify · 2021-05-12T21:15:21Z

🚀 Deployed to staging in version: 1.0.42-5🚀

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

OSBotify · 2021-05-13T23:05:15Z

🚀 Deployed to production in version: 1.0.44-0🚀

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

fix flashing messages

1f0a223

Maftalion requested a review from a team as a code owner April 7, 2021 00:46

MelvinBot requested review from deetergp and removed request for a team April 7, 2021 00:46

Maftalion commented Apr 7, 2021

View reviewed changes

src/pages/home/report/ReportActionCompose.js Show resolved Hide resolved

Beamanator self-requested a review April 7, 2021 09:16

Beamanator requested changes Apr 7, 2021

View reviewed changes

use clientID, revert scrollToBottom

34a082b

deetergp previously approved these changes Apr 7, 2021

View reviewed changes

Beamanator closed this Apr 8, 2021

trjExpensify reopened this Apr 27, 2021

Maftalion changed the base branch from master to main April 27, 2021 17:18

Merge branch 'main' into matt-2205-fix-flashing-messages

08db220

use sequenceNumber as fallback

fb8c844

trjExpensify mentioned this pull request May 5, 2021

iOS - Chat - Messages flicker once after message is sent #2693

Closed

Merge branch 'main' into matt-2205-fix-flashing-messages

8399514

use scrollToOffset instead of scrollToIndex

87e8073

deetergp previously approved these changes May 10, 2021

View reviewed changes

Beamanator self-requested a review May 11, 2021 11:44

Beamanator reviewed May 11, 2021

View reviewed changes

Merge branch 'main' into matt-2205-fix-flashing-messages

a8ad6ec

# Conflicts: # src/pages/home/report/ReportActionsView.js

Maftalion dismissed deetergp’s stale review via a8ad6ec May 11, 2021 16:34

deetergp approved these changes May 11, 2021

View reviewed changes

Beamanator self-requested a review May 12, 2021 20:38

Beamanator approved these changes May 12, 2021

View reviewed changes

Beamanator merged commit ad2a39c into Expensify:main May 12, 2021

OSBotify mentioned this pull request May 12, 2021

Deploy Checklist: Expensify.cash 2021-05-08 #2751

Closed

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flashing messages on iOS #2252

Fix flashing messages on iOS #2252

Maftalion commented Apr 7, 2021 •

edited

Loading

Beamanator left a comment

Beamanator Apr 7, 2021

Maftalion Apr 7, 2021

Maftalion commented Apr 7, 2021

deetergp left a comment

Beamanator commented Apr 8, 2021 •

edited

Loading

Beamanator commented Apr 8, 2021

Maftalion commented Apr 27, 2021

Beamanator commented Apr 29, 2021 •

edited

Loading

deetergp commented Apr 30, 2021

deetergp commented Apr 30, 2021

Beamanator commented May 5, 2021

trjExpensify commented May 5, 2021

deetergp commented May 5, 2021

Maftalion commented May 7, 2021

deetergp commented May 7, 2021

Beamanator commented May 7, 2021

Maftalion commented May 7, 2021

deetergp left a comment

Beamanator left a comment

Maftalion commented May 11, 2021

deetergp left a comment

Beamanator left a comment

OSBotify commented May 12, 2021

OSBotify commented May 13, 2021

Fix flashing messages on iOS #2252

Fix flashing messages on iOS #2252

Conversation

Maftalion commented Apr 7, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Beamanator left a comment

Choose a reason for hiding this comment

Beamanator Apr 7, 2021

Choose a reason for hiding this comment

Maftalion Apr 7, 2021

Choose a reason for hiding this comment

Maftalion commented Apr 7, 2021

deetergp left a comment

Choose a reason for hiding this comment

Beamanator commented Apr 8, 2021 • edited Loading

Beamanator commented Apr 8, 2021

Maftalion commented Apr 27, 2021

Beamanator commented Apr 29, 2021 • edited Loading

deetergp commented Apr 30, 2021

deetergp commented Apr 30, 2021

Beamanator commented May 5, 2021

trjExpensify commented May 5, 2021

deetergp commented May 5, 2021

Maftalion commented May 7, 2021

deetergp commented May 7, 2021

Beamanator commented May 7, 2021

Maftalion commented May 7, 2021

deetergp left a comment

Choose a reason for hiding this comment

Beamanator left a comment

Choose a reason for hiding this comment

Maftalion commented May 11, 2021

deetergp left a comment

Choose a reason for hiding this comment

Beamanator left a comment

Choose a reason for hiding this comment

OSBotify commented May 12, 2021

OSBotify commented May 13, 2021

Maftalion commented Apr 7, 2021 •

edited

Loading

Beamanator commented Apr 8, 2021 •

edited

Loading

Beamanator commented Apr 29, 2021 •

edited

Loading