Yes, I know we consider leaving TODO comments in the code an anti-pattern, but I think it's the best way forward in this case.

The "ref" is currently "within" the class component and can't be accessed from the outer functional component.

"Why not pull the input refs out from the class component first then?"

I tried sequencing the "lifting of refs" before this and found it very frustrating... particularly with regard to flow issues.

I think the best bet is to leave a reminder here that we want to focus that input, and then once we've "turned ThreadSettingsModal into a functional component," we can introduce replacement refs with React.useRef and bring back this focus behavior.

Ah thanks for the heads up. It looks that stack requires revision before it can be landed.

I think the best bet is for me to

1. Land this diff as is
2. Commandeer D3665
3. Rebase and make the necessary changes in ThreadSettingsModal
4. Hand the revision back to Jacek

That way I should be able to move forward with the ThreadSettingsModal refactor without breaking Jacek's existing work?

cc @def-au1t

Do you think it is a good idea to create a linear task for it?

Sure, don't think it would hurt: https://linear.app/comm/issue/ENG-1028/[web]-reintroduce-focusing-of-inputs-in-threadsettingsmodal

Yup, will make that change

Hm, when I modify the deplist I get the following:

React Hook React.useCallback has a missing dependency: 'threadInfo'. Either include it or remove the dependency array.

Going to land as is, open to making any changes in a subsequent diff if necessary

Up to you, but usually the diff author should be responsible for rebasing and resolving conflicts. It's a part of the process that we're introducing conflicting changes.

That's due to invariant using threadInfo. We can change that and use invariant(threadInfo.id, ... or, if that doesn't work, introduce a new variable threadID before the callback and use it as a dependency and in the invariant.

Overall, it is a bit wasteful to create a new callback every time a threadInfo changes, when we only use id part of it. This improvement should be introduced, but may be easier when invariants are gone.

Makes sense, fwiw I think I remember trying to change the invariant to be threadInfo.id and a few other approaches but still ran into the issue

Regardless, the invariant in deleteThreadAction is removed altogether as of D3837

Sometimes that happens, in which case you can just pull threadInfo.id out into its own variable outside of the block