From D5142:

These changes make sense, but I would also consider moving them to a React.useEffect in ConnectedThreadSettings... feels a little cleaner that way

Nit

This code can also be moved to a React.useEffect now, but I think that could be a separate diff

Nit: we usually just specify React.useEffect on the team, rather than separately importing it. Would you mind making this change?

This is the same logic as before, but we can actually improve it now.

The intention here is: "If the thread's not in the chat list, make sure it's watched. If the thread joins the chat list, or if the threadID changes, or if the component is unmounted, make sure we stop watching the thread."

We can handle that with fewer wasted operations this way:

React.useEffect(() => {
  if (threadInChatList(threadInfo)) {
    return undefined;
  }
  threadWatcher.watchID(threadInfo.id);
  return () => {
    threadWatcher.removeID(threadInfo.id);
  };
}, [threadInfo]);

This code is better because:

1. Each removeID call is paired 1-1 with a watchID call. Previously, a permissions change could cause a weird effect where we call watchID without calling removeID. (This was the case before your diff as well.)
2. We exit early