Details

• benschac retitled this revision from [lib, native] move,replace canPromote thread logic to lib hook to [lib, native] [move] [ENG-991] replace canPromote thread logic to lib hook.

ashoat added inline comments.Apr 22 2022, 6:18 AM

lib/hooks/promote-sidebar.react.js
30 ↗	(On Diff #11728)	I think this variable could be named better. What does it represent? If it's the `parentThreadID` in all cases, maybe we can name it that, and we could just always use it instead of ever checking `threadInfo.parentThreadID`

• benschac added inline comments.Apr 22 2022, 10:55 AM

lib/hooks/promote-sidebar.react.js
30 ↗	(On Diff #11728)	agreed, that makes sense.

ashoat review changes

Harbormaster completed remote builds in B8468: Diff 11807.Apr 22 2022, 11:07 AM

• benschac added a child revision: D3817: [lib] [refactor] [ENG-991] use nullish coalescing operator (?.) to simplify code.Apr 22 2022, 11:08 AM

Whoopsy! didn't implement the entire change.

I looked at this again, maybe for a bit too long and I'm having trouble figuring out exactly what you mean by @ashoat:

maybe we can name it that, and we could just always use it instead of ever checking threadInfo.parentThreadID

From what I understand about the feature, I think all we'd ever want is the threadInfo.parentID because a sidebar will always have a parentThread to be promoted from. I'm honestly not sure why this check was here in the first place.

If you have a minute today could we chat about it IRL.

lib/hooks/promote-sidebar.react.js
30 ↗	(On Diff #11728)

Putting this back as needs review to get it back into @ashoat's queue to make sure he'll see my comment.

ashoat requested changes to this revision.Apr 27 2022, 2:27 PM

ashoat added inline comments.

lib/hooks/promote-sidebar.react.js
30 ↗	(On Diff #11807)	I think all we'd ever want is the `threadInfo.parentID` because a sidebar will always have a `parentThread` to be promoted from. I'm honestly not sure why this check was here in the first place. Assuming you mean `threadInfo.parentThreadID`... it seems like you're saying we don't need this parameter here, since we can get it from `threadInfo.parentThreadID`? In that case, let's simplify
40 ↗	(On Diff #11807)	My feedback from the last review cycle is basically that I don't think we should have two distinct places we are getting `parentThreadID` from. If there is some reason we can't trust `threadInfo.parentThreadID`, then I think this line should be handled by whoever calls `usePromoteSidebar`. That way, the third parameter to `usePromoteSidebar` will always be valid, and we never need to check `threadInfo.parentThreadID`. We could simplify lift this line to the caller and that would address my feedback (That said, see my comment above – if we can trust `threadInfo.parentThreadID`, it seems cleaner to remove the third parameter here and just use that)

This revision now requires changes to proceed.Apr 27 2022, 2:27 PM

remove route / parent thread param

I need to test the functionality a bit before passing this over for review.

lib/hooks/promote-sidebar.react.js
30 ↗	(On Diff #11807)	yes, that's the assumption! Awesome, thanks.

Tested promote thread functionality on both iOS and web and it's working as expected.

Harbormaster completed remote builds in B8645: Diff 12036.Apr 28 2022, 6:58 AM

• benschac added inline comments.Apr 28 2022, 7:03 AM

lib/hooks/promote-sidebar.react.js
40 ↗	(On Diff #12036)	This is a copy/pasta from: https://github.com/CommE2E/comm/blob/master/native/chat/settings/thread-settings.react.js#L1119 It's not that obvious from the diff because we still need it in `thread-settings.react` / I didn't remove it in that file.

ashoat requested changes to this revision.Apr 28 2022, 11:43 AM

ashoat added inline comments.

lib/hooks/promote-sidebar.react.js
36 ↗	(On Diff #12036)	What's the reason for this added newline?
39 ↗	(On Diff #12036)	Can we git rid of `?? false`? It's best for variables to have clean types, eg. `?string` rather than `?string \| boolean`
39 ↗	(On Diff #12036)	I think it's better to use the consistent name `parentThreadID` that we use everywhere in the codebase
39 ↗	(On Diff #12036)	Can you remind me why you were passing this in as a third parameter? Are you 100% sure we can always trust `threadInfo.parentThreadID`?
39 ↗	(On Diff #12036)	In what scenario will `threadInfo` be falsey? It's typed as `ThreadInfo`, so I don't understand why you need the question mark in `threadInfo?.parentThreadID`

This revision now requires changes to proceed.Apr 28 2022, 11:43 AM

diff review

lib/hooks/promote-sidebar.react.js
36 ↗	(On Diff #12114)	going to get rid of this one too.
39 ↗	(On Diff #12036)	yeah, makes sense.
39 ↗	(On Diff #12036)	I'd say I'm about 90%. Here are the following steps I took to confirm that we can trust `threadInfo.parentThreadID` I looked at the `keyserver` code. I grepped on `parentThreadID` and it looks like it's always set on new threads/sidebars. https://github.com/CommE2E/comm/blob/master/keyserver/src/creators/thread-creator.js#L87 when they're created. I looked at how the route is being set via the `navInfoFromURL` function: https://github.com/CommE2E/comm/blob/master/web/app.react.js#L109 and it _looks_ like it's just reading the same data in the server.

remove line

Harbormaster completed remote builds in B8695: Diff 12114.Apr 29 2022, 6:34 AM

• benschac planned changes to this revision.Apr 29 2022, 6:36 AM

Harbormaster completed remote builds in B8696: Diff 12115.Apr 29 2022, 6:38 AM

parentThreadID

Harbormaster completed remote builds in B8697: Diff 12116.Apr 29 2022, 6:43 AM

Sending to your queue so we can discuss offline (see inline)

lib/hooks/promote-sidebar.react.js
38 ↗	(On Diff #12116)	Hmm... RE trusting `parentThreadID`, the only reason I suspect we might have for not trusting `parentThreadID` is if it was a pending thread. Can you check with me in person / schedule some time on my Calendly so we can look into this together? I also want to explain the concept of a pending thread to you a bit