Skip to content

Conversation

Gautam-Arora24
Copy link
Contributor

@Gautam-Arora24 Gautam-Arora24 commented Feb 12, 2021

What was the error?
The unformattedMessage in getUserFromMention was giving undefined which was the cause of the crash.

How I solved this issue?
Since on mentioning the group in the ComposeBox was providing the same error. I used a constant called splittedMessage which splits the string through "*". This computation can provide me an array of length either 3 or 5. Thus, according to the size, I specified which index's value is further needed in the code. Hence, the bug was solved.

Before
Screenshot-2021-02-05-at-10 52 28-AM

After changes
No error
Screenshot 2021-02-13 at 1 58 09 AM

Fixes: #4460

@Gautam-Arora24 Gautam-Arora24 force-pushed the composebox_crash branch 2 times, most recently from 09a99b2 to fb0b1c9 Compare February 12, 2021 20:34
@Gautam-Arora24 Gautam-Arora24 changed the title crash: Fix crash in getUserFromMention as unformattedMessage gives un… crash: Fix crash in getUserFromMention as unformattedMessage gives undefined. Feb 12, 2021
@Gautam-Arora24 Gautam-Arora24 marked this pull request as draft February 13, 2021 04:17
@Gautam-Arora24 Gautam-Arora24 marked this pull request as ready for review February 13, 2021 04:18
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Feb 15, 2021

Hi, @Gautam-Arora24! I've commented on the issue (here) with a potentially cleaner way of handling this.

In particular, I get the feeling that future readers of the code might prefer not to have to read and understand exactly what it means if completion.split('**') has a length of 3 or not. And if that did turn out to be necessary, I'd like to have a very clear code comment (with //), so a future reader doing some debugging could understand the code just as clearly as you do. 🙂

@chrisbobbe
Copy link
Contributor

Thanks, @Gautam-Arora24!

In the summary line, let's use MentionWarnings: for the prefix, instead of crash:. We tend to use the prefix to help specify "what part of the code is affected". And the rest of the summary line makes it clear that a crash is being fixed, as well as narrowing it down to a particular function that can be found in the MentionWarnings component/file. 🙂

In the commit-message body, let's say "Use a conditional" instead of "Used a conditional". The mobile-app project differs from the main project in explicitly preferring the first form in the commit message body, not just the summary line, to explain the "what" of the commit message (i.e., what work is being done). We should get around to documenting this, but I usually refer to Greg's comment at #4079 (comment) to explain our reasoning. 🙂

I notice that the line lengths are well within bounds; thanks! 🙂 I do notice that the lines of the body are a bit uneven, though; the second is quite a lot shorter than the third. Do you mean the third line to begin a new paragraph? I tend to separate paragraphs with an extra line break, e.g.,

Use a conditional to check whether the unformattedMessage
is undefined or not.

Now, there is no error related to unformattedMessage as
undefined.

@Gautam-Arora24
Copy link
Contributor Author

Gautam-Arora24 commented Feb 15, 2021

@chrisbobbe I hope that the commit is fine now.

Use a conditional to check whether the unformattedMessage
is undefined or not.

Now, there is no error related to unformattedMessage as
undefined.

Fixes: zulip#4460
@chrisbobbe chrisbobbe merged commit 706ac55 into zulip:master Feb 15, 2021
@chrisbobbe
Copy link
Contributor

Merged, thanks!

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.

Uncaught error in getUserFromMention

2 participants