Skip to content

Fix crash on new chat creation due to undefined sequenceNumber#2721

Merged
marcaaron merged 3 commits into
mainfrom
marcaaron-updateLastRead
May 7, 2021
Merged

Fix crash on new chat creation due to undefined sequenceNumber#2721
marcaaron merged 3 commits into
mainfrom
marcaaron-updateLastRead

Conversation

@marcaaron

@marcaaron marcaaron commented May 6, 2021

Copy link
Copy Markdown
Contributor

cc @tgolen @Gonals I think this change is safe but need a second opinion. another option could be to not mark the report as "read" until the reportMaxSequenceNumber[reportID] exists, but that's difficult since there's no way to know this has happened because of the async Onyx callbacks etc.

Details

  • When a chat is first created it does not yet have a reportMaxSequenceNumbers[reportID]
  • We are still trying to mark it as "read" but since it has no history yet it crashes and leads to a white screen of death.

Bonus: The rest of the code changes are related to propTypes and thought I'd sneak them in here.

Fixed Issues

Fixes #2704

Tests

QA Steps

  1. Create a new chat with a user that you have no existing chat with via New Group, New Chat, or Search flows
  2. Verify you do not experience a white screen and are brought to the chat.
  3. Test "new marker" and unread behavior for regressions

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron requested a review from a team as a code owner May 6, 2021 19:24
@marcaaron marcaaron self-assigned this May 6, 2021
@marcaaron marcaaron requested review from a team and removed request for a team May 6, 2021 20:31
@MelvinBot MelvinBot requested review from stitesExpensify and removed request for a team May 6, 2021 20:31
Comment thread src/libs/actions/Report.js Outdated
// If we don't have a maxSequenceNumber for this report then we will assume that the report has just been created
// and use 0 for the maxSequenceNumber. Otherwise, we'll have an undefined sequenceNumber in cases where no
// sequenceNumber is explicitly passed.
const maxSequenceNumber = !_.isUndefined(reportMaxSequenceNumbers[reportID])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be an early return instead? Just thinking that if the maxSequenceNumber is 0, then it doesn't really need to call setLocalLastRead() and make an API call.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh hmm yeah I guess that would make sense wouldn't it 😄

@marcaaron marcaaron May 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably a safer change as well since my comment above could end up being wrong at some point ...

{option.hasDraftComment && (
<View style={styles.ml2}>
<Icon src={Pencil} height="16" width="16" />
<Icon src={Pencil} height={16} width={16} />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this :D It must have been causing a propType warning? I'm ashamed I didn't notice that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yes forgot to mention that I fixed a few random propTypes things in this.

@marcaaron

Copy link
Copy Markdown
Contributor Author

Updated.

@Gonals

Gonals commented May 7, 2021

Copy link
Copy Markdown
Contributor

Ahh. Dammit. I thought this was no longer happening

@Gonals

Gonals commented May 7, 2021

Copy link
Copy Markdown
Contributor

All yours, @stitesExpensify!

@marcaaron

Copy link
Copy Markdown
Contributor Author

Gonna merge this since it's the last deploy blocker I think

@marcaaron marcaaron merged commit 4253f79 into main May 7, 2021
@marcaaron marcaaron deleted the marcaaron-updateLastRead branch May 7, 2021 19:37
@OSBotify

OSBotify commented May 7, 2021

Copy link
Copy Markdown
Contributor

✋ 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

OSBotify commented May 8, 2021

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging in version: 1.0.41-0🚀

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

@roryabraham

roryabraham commented May 8, 2021

Copy link
Copy Markdown
Contributor

I verified that this worked on desktop 👍 1.0.41-0

@roryabraham

Copy link
Copy Markdown
Contributor

Also verified this on iOS 👍

@isagoico

isagoico commented May 8, 2021

Copy link
Copy Markdown

Android, web and mweb were a pass too 🎉

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production in version: 1.0.44-0🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to create new group

6 participants