1. Let's rename this to use "sidebar" instead of "thread"
2. Do we have any other functions that parse a pending sidebar ID?

Hmm, I'm not sure this is the right check. After the thread is successfully created, won't threadInfo be created? At that point, it appears that you'll be trying to index into the threadInfo for a role that is actually from the parentThreadInfo

I would think you'd need to do something more like:

threadInfo.roles[messageInfo.newRole]?.name ?? parentThreadInfo.roles[messageInfo.newRole]?.name

That said, I would've figured that my concern would pop up in testing... so maybe I'm missing something?

Call sites:

• lib/selectors/chat-selectors.js --> the root cause of the crash, now addressed
• lib/shared/message-utils.js --> covered this in the diff summary
• lib/shared/notif-utils.js --> testing this now, will update this callsite with parentThreadInfo if I find that we need it, otherwise have it null

I think it would be good to at least pass in null or undefined as parentThreadInfo from the callsites that don't require parentThreadInfo (since they were working fine before). Regardless will test first then update

Not that I'm aware of - the closest I could find was getPendingThreadID, but that just returns pending/sidebar/256|91092 for example, and parsePendingThreadID which returns something else

type PendingThreadIDContents = {
  +threadType: ThreadType,
  +memberIDs: $ReadOnlyArray<string>,
  +sourceMessageID: ?string,
};

I couldn't actually find a function that parses the numeric sidebar ID from the entire string (on a separate note, if I keep this function it'd make sense to move it to thread-utils close to these two)

Yeah you're right, I checked threadStore to confirm and when the sidebar is created, the thread doesn't actually have the role info anymore.

The reason it's not crashing is because of the failsafe of lines 136-137, where if the threadInfo doesn't have the role, we parse it from the DB messageInfo.

Tested the following with notif-utils.js:

• Ran the app on my physical phone
• Ran through several scenarios of robotext notifs (change_role, change_thread_color, etc)
• Confirmed on master that the threadInfo provided is non-null
• Confirmed on this branch that the threadInfo provided is still non-null (this is expected since this was never the root cause of the problem)

The parentThreadInfo that addresses this crash is the 'fallback' option. so it makes sense that the non-problematic callsites will never need to fallback onto parentThreadInfo.

Going to at least update this diff to update non-problematic callsites to pass in a null as a third parameter, rather than not providing one at all.

I am skeptical of this. I get why it's technically okay... we don't need a message preview in this case because sidebars don't show a message preview in the ThreadList.

But think about what you're setting us up for. Let's imagine somebody adds message previewers for sidebars, or maybe somebody introduces a new message spec that relies on the parentThreadInfo parameter. They will have no idea of the complicated "contract" you've introduced here, where this info is only provided for certain cases. And what's worse is that they won't realize there's a problem until somebody coincidentally tries to do something with a CHANGE_ROLE message. Pretty much zero change that they'll think to specifically test that message type.

I don't think passing null in here is okay. Let's make sure we pass in the parentThreadInfo. This really should not be super hard... I see 3 callsites of getMessageTitle, 2 of which already have the parentThreadInfo, and the third is a hook where you can fetch it via useSelector.

Similiar situation here. This is again "technically okay" because we never send a notif for a SIDEBAR_SOURCE, but that could change in the future.

notifRobotextForMessageInfo has 4 callsites, all of which are from the notificationTexts method of a message spec. One of them is changeRoleMessageSpec, which presents some risk.

After taking a look, I think we can add the parentThreadInfo here as well. We'll need to "drill" it through from keyserver/src/push/send.js to notificationTexts and then to this method. keyserver/src/push/send.js has access to the full list of threadInfos, so it should be as easy as threadInfos[threadInfo.parentThreadID].

I've put up D9278, which should make this slightly easier for you