It looks like this diff "pushes" the Modal title to the right. This probably makes sense when there is an icon, but it looks strange when there isn't.

We should either:

A. Decide that we do want to display an icon for every modal, in which case we should go through them and decide which ones
B. Justify the title to the left or center it if there's no icon

Otherwise modals (eg ThreadSettingsModal) end up looking pretty strange.

Feedback to @benschac about regressions

Disappointing to see a regression after I took the time to type this out on the corresponding Linear task two weeks ago:

@benschac, regarding the continued shipping of regressions... would be great if you could try integrating some of the feedback I've given to you about this previously, such as to make sure you have a theoretical understanding of the code changes you are making, not just a practical one. Your test plan shouldn't just be "test to see if it looks good" – it should be fully and thoroughly understanding every single code point this effects, and the side effects your change can have. That involves a LOT of git grep. One way to think about this: if you're not git greping somewhere in the course of writing a diff like this, then you probably aren't understanding your changes well enough.

Feedback to @atul about leaving comments requiring follow-ups without creating tasks

See this comment. Leaving a comment on a landed diff like this without a follow-up task is extra bad. Even if the diff is live, you should never be discussing a follow-up without linking a task for that follow-up. Please consider this to be a hard rule.

Feedback to @palys-swm about being stricter on test plan

In the review here, we should've been more strict with @benschac about testing every single codepoint his change effects. Especially as this is an issue that has come up, please make sure you bring this up repeatedly in every review. Every single codepoint should be checked and tested, and we should be very sensitive to potential regressions.

Feedback to @palys-swm about being stricter on test plan

In the review here, we should've been more strict with @benschac about testing every single codepoint his change effects. Especially as this is an issue that has come up, please make sure you bring this up repeatedly in every review. Every single codepoint should be checked and tested, and we should be very sensitive to potential regressions.

Thanks for this feedback - it makes sense! Going to do that during following reviews.