[native] fix missing threadInfo in thread settings
ClosedPublic
Actions

Authored by michal on Sep 15 2022, 2:29 AM.

Details

Reviewers

tomek
atul
• abosh
ashoat

Commits

rCOMMb2b658d80025: [native] fix missing threadInfo in thread settings

Summary

Logging out while sidebar settings are opened causes a crash.
https://linear.app/comm/issue/ENG-1721/

After logging out, we lose redux state and crash because threadInfo is undefined. In this case we should fall back onto threadInfo in the navigation params.

Test Plan

Check if the app doesn't crash after logging out with sidebar settings open.
Check if the app doesn't crash with non-sidebar thread settings open.
Check that if you change some value in settings (e.g. color) and log out, the settings screen that "blinks" for a moment has an updated value.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Sep 15 2022, 2:29 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 15 2022, 2:29 AM

michal added a child revision: D5143: [native] move threadInfo caching logic.Sep 15 2022, 2:32 AM

Harbormaster completed remote builds in B12162: Diff 16690.Sep 15 2022, 2:38 AM

michal requested review of this revision.Sep 15 2022, 2:38 AM

I can review this one since I have the most context. Overall, great work, but some changes requested inline

native/chat/settings/thread-settings.react.js
242–243 ↗	(On Diff #16690)	Don't think this needs to be removed. It's correct – when `ThreadSettings` is opened, `threadInfo` should exist
294–296 ↗	(On Diff #16690)	If you want to move `getThreadInfo` to `ConnectedThreadSettings`, then I think this logic for updating `props.route.params.threadInfo` should've been moved to `ConnectedThreadSettings` too. That way you can keep looking at `reduxThreadInfo` specifically (which I think is more clear), and I also think this sort of logic is more readable as a `React.useEffect` anyways (Separately, there is no need to check if `newThreadInfo` is truthy here... your changes should guarantee that it is always truthy)
298–305 ↗	(On Diff #16690)	These changes make sense, but I would also consider moving them to a `React.useEffect` in `ConnectedThreadSettings`... feels a little cleaner that way
1132 ↗	(On Diff #16690)	(This is the actual line that was causing problems)

This revision now requires changes to proceed.Sep 16 2022, 7:06 AM

ashoat added inline comments.Sep 16 2022, 7:06 AM

native/chat/settings/thread-settings.react.js
242–243 ↗	(On Diff #16690)	Should be moved to `ConnectedThreadSettings` though, and should be checking `reduxThreadInfo` specifically

ashoat added inline comments.Sep 16 2022, 7:09 AM

native/chat/settings/thread-settings.react.js
242–243 ↗	(On Diff #16690)	Only thing to do in this diff now is just to move this `invariant` to `ConnectedThreadSettings` and have it check `reduxThreadInfo`
294–296 ↗	(On Diff #16690)	Ah, I see you did this in D5143!! Disregard here
298–305 ↗	(On Diff #16690)	Since D5143 handles the above, we can address this there

Update

Harbormaster completed remote builds in B12250: Diff 16811.Sep 19 2022, 2:10 AM

Please address the inline comment before landing!

native/chat/settings/thread-settings.react.js
5 ↗	(On Diff #16811)	Nit: we usually just specify `React.useEffect` on the team, rather than separately importing it. Would you mind removing this import?