Details

Reviewers

atul
inka
ashoat

Commits

rCOMM6412c3e55793: [lib] fix @-mentioning in a pending GENESIS sidebar crashes app

Summary

@-mentioning in a pending GENSIS sidebar would crash the app. This was because in useMentionTypeaheadChatSuggestions we were attempting to get the chat mention search index from the communityThreadIDForGenesisThreads with a pending thread info id. The reason this is an issue is that we build the communityThreadIDForGenesisThreads is built with resolvedThreadInfos. This meant that pending sidebar thread ids (which look like this pending/sidebar/256|145027) weren't included in the communityThreadIDForGenesisThreads object, and was returning an undefined chatSearchIndex.

The solution I've come up with is to add a check if the thread is pending and then if it is, use the resolved containing threadID of the pending sidebar since that will have the same chat mention candidates as the sidebar thread if it was resolved.

Linear task: https://linear.app/comm/issue/ENG-6640/mentioning-a-user-in-a-pending-genesis-sidebar-causes-the-app-to-crash

Test Plan

Please see the demo video below

Here are the @ mentioning candidates for a resolved sidebar:

Here are the @ mentioning candidates for a pending sidebar:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Feb 5 2024, 2:01 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptFeb 5 2024, 2:01 PM

ginsu edited the summary of this revision. (Show Details)Feb 5 2024, 2:09 PM

ginsu added reviewers: atul, inka.

Harbormaster completed remote builds in B26521: Diff 36656.Feb 5 2024, 2:19 PM

ginsu requested review of this revision.Feb 5 2024, 2:19 PM

ginsu edited the summary of this revision. (Show Details)Feb 5 2024, 2:33 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F1117812: Screen Recording 2024-02-05 at 5.22.25 PM.mov. (Show Details)

ginsu edited the summary of this revision. (Show Details)

ginsu attached a referenced file: F1117841: Screen Recording 2024-01-31 at 8.04.22 PM.mov. (Show Details)

ginsu retitled this revision from [web] fix @-mentioning in a pending GENESIS sidebar crashes app to [lib] fix @-mentioning in a pending GENESIS sidebar crashes app.Feb 5 2024, 2:54 PM

ginsu mentioned this in D10957: [lib/native] fix @-mentioning in a create new pending GENSIS chat crashes app.Feb 5 2024, 3:02 PM

ginsu added a child revision: D10957: [lib/native] fix @-mentioning in a create new pending GENSIS chat crashes app.Feb 5 2024, 3:17 PM

use the resolved containing threadID of the pending sidebar since that will have the same chat mention candidates as the sidebar thread if it was resolved

~~This makes sense, but it's working confirming that pending sidebars are the only kind of pending threads where this can happen.~~

~~I think we can also have pending threads that are channels of GENESIS. Will this change behavior in that case? If so, is that change what we want?~~

(Looks like this is addressed in the next diff in the stack)

lib/components/chat-mention-provider.react.js
44 ↗	(On Diff #36656)	Would it be better to fix `communityThreadIDForGenesisThreads` to make it impossible to make this mistake? For instance, imagine `communityThreadIDForGenesisThreads` was a function that took a `ThreadInfo`. Then the check you're doing could be made inside of it.

This revision now requires changes to proceed.Feb 5 2024, 6:17 PM

address comments

make getCommunityThreadIDForGenesisThreads type read only

ginsu added inline comments.Feb 6 2024, 1:19 PM

lib/components/chat-mention-provider.react.js
217–219 ↗	(On Diff #36747)	This was being returned, but not used...

Harbormaster completed remote builds in B26590: Diff 36746.Feb 6 2024, 1:26 PM

Harbormaster completed remote builds in B26591: Diff 36747.Feb 6 2024, 1:28 PM

ashoat accepted this revision.Feb 7 2024, 10:57 AM

ashoat added inline comments.

lib/components/chat-mention-provider.react.js
235–238 ↗	(On Diff #36747)	Wondering why you replaced the ternary expression with this. In general I think avoiding the use of `let` in favor of `const` is good practice, but maybe the ternary expression was getting formatted poorly?

This revision is now accepted and ready to land.Feb 7 2024, 10:57 AM

ginsu added inline comments.Feb 7 2024, 10:59 AM

lib/components/chat-mention-provider.react.js
235–238 ↗	(On Diff #36747)	Personally I thought this was easier to read, but happy to change it back a ternary expression + use const if that is better practice

ashoat added inline comments.Feb 7 2024, 11:01 AM

lib/components/chat-mention-provider.react.js
235–238 ↗	(On Diff #36747)	Yeah I think that would be better, unless it breaks one of the rules here

address feedback

This revision was landed with ongoing or failed builds.Feb 7 2024, 11:11 AM

Closed by commit rCOMM6412c3e55793: [lib] fix @-mentioning in a pending GENESIS sidebar crashes app (authored by ginsu). · Explain Why

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM6412c3e55793: [lib] fix @-mentioning in a pending GENESIS sidebar crashes app.

Harbormaster completed remote builds in B26626: Diff 36792.Feb 7 2024, 11:24 AM

[lib] fix @-mentioning in a pending GENESIS sidebar crashes app
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 36794

lib/components/chat-mention-provider.react.js

lib/hooks/chat-mention-hooks.js

[lib] fix @-mentioning in a pending GENESIS sidebar crashes appClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 36794

lib/components/chat-mention-provider.react.js

lib/hooks/chat-mention-hooks.js

[lib] fix @-mentioning in a pending GENESIS sidebar crashes app
ClosedPublic
Actions

Revision Contents
Changeset List