Page MenuHomePhabricator
Differential D13260

Implement joining DM thread
ClosedPublic
Actions

Authored by marcin on Fri, Sep 6, 8:45 AM.
Tags
None
Referenced Files
F2715719: D13260.id44016.diff
Mon, Sep 16, 4:19 AM
F2711362: D13260.id44015.diff
Sun, Sep 15, 11:11 PM
F2709042: D13260.id44072.diff
Sun, Sep 15, 5:46 PM
Unknown Object (File)
Sun, Sep 15, 6:10 AM
Unknown Object (File)
Sat, Sep 14, 4:59 PM
Unknown Object (File)
Sat, Sep 14, 9:47 AM
Unknown Object (File)
Sat, Sep 14, 4:58 AM
Unknown Object (File)
Fri, Sep 13, 8:52 AM
View All 13 Files
Subscribers
ashoat

Details

Reviewers
kamil
tomek
inka
ashoat
Commits
rCOMMa91bc4cfd0f7: Implement joining DM thread
Summary

This differential refactors useJoinThread hook so that it handles DM threads as well.

Test Plan

TODO. Once Inka implements creating thick sidebars testing will be easy and smooth.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Fri, Sep 6, 8:45 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFri, Sep 6, 8:45 AM
Harbormaster completed remote builds in B31463: Diff 43941.Fri, Sep 6, 9:17 AM
marcin requested review of this revision.Fri, Sep 6, 9:17 AM
ashoat requested changes to this revision.Mon, Sep 9, 12:32 PM
ashoat added inline comments.
lib/actions/thread-actions.js
354–356 ↗(On Diff #43941)

This API is confusing because it's unclear when rawThreadInfo needs to be specified.

One possibility would be to split into two hooks: useJoinThinThread and useJoinThickThread. However, all the callsites would have to call each hook separately, and the ChatInputBars would need some additional code to decide which to use.

Another option would be to replace the input here. Combined with the above feedback, I think we could do an API like this:

type UseJoinThreadInput =
  | $ReadOnly<{
      +thick: false,
      ...ClientThreadJoinRequest,
    }>
  | {
      +thick: true,
      +rawThreadInfo: RawThreadInfo,
    };
function useJoinThread(): (
  input: ClientThreadJoinRequest,
) => Promise<ThreadJoinPayload> {
  ...
}
355 ↗(On Diff #43941)

There's no reason for this to be a parameter to the hook instead of part of input. It's more flexible to move it to input, so let's do that

native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

Do we want to use joinThreadActionTypes for thick threads?

Does processAndSendDMOperation dispatch a Redux action, in which case we're dispatching an action from another action? That's generally an anti-pattern, and makes me wonder if we really need to be dispatching a Redux action at all.

If we take the approach of splitting into useJoinThinThread and useJoinThickThread, then we could document this difference in behavior (useJoinThickThread dispatches an action and useJoinThinThread doesn't), and make sure that ChatInputBar only wraps useJoinThinThread in dispatchActionPromise.

This revision now requires changes to proceed.Mon, Sep 9, 12:32 PM
ashoat mentioned this in D13236: Implement updating DM thread subscription.Mon, Sep 9, 12:56 PM
ashoat mentioned this in D13238: Implement maunally marking DM thread as read/unread.Mon, Sep 9, 2:13 PM
ashoat mentioned this in D13246: Implement DMActivityHandler.Mon, Sep 9, 2:17 PM
marcin added inline comments.Tue, Sep 10, 2:48 AM
lib/actions/thread-actions.js
354–356 ↗(On Diff #43941)

I think this solution has the same disadvantage as separate hooks since the component has to know whether it is running for thick thread to device on the input structure.

Is there anything that I am missing? Perhaps you are aware of this but don't consider it a problem since we can use simple ternary statement to choose the right input?

marcin added inline comments.Tue, Sep 10, 3:02 AM
native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

I think the reason we want to use dispatchActionPromise in thick threads is to be able to call createLodaingStatusSelector. processAndSendDMOperation alone doesn't let us track the status of an action.

cc @tomek can you confirm?

ashoat added inline comments.Tue, Sep 10, 3:03 AM
lib/actions/thread-actions.js
354–356 ↗(On Diff #43941)

Yeah, that makes sense. I think we should probably go with separate hooks... it seems beneficial also for the reasons described in the inline comment I left on lines 1139-1142

marcin added inline comments.Tue, Sep 10, 3:06 AM
lib/actions/thread-actions.js
354–356 ↗(On Diff #43941)

Have you seen my comment there: https://phab.comm.dev/D13260#inline-76487? I think it could affect your judgement.

ashoat added a comment.Tue, Sep 10, 3:13 AM

I still think the API is pretty confusing, but it's probably too much work here to address the Redux-action-wrapping-another-Redux-action issue.

Could you:

  1. Create a follow-up task for that?
  2. Improve the API through either of the suggestions from my comment on lines 354-356?
native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

Ah, that's helpful context. I can see now why dispatching a Redux action from another action is the most expedient solution here.

The alternative would be to track multiple loading statuses from each component. If processAndSendDMOperation dispatches via dispatchActionPromise, this wouldn't be too hard... we could do this with a combination of customKeyName and combineLoadingStatuses. But I suspect processAndSendDMOperation doesn't dispatche via dispatchActionPromise.

marcin added inline comments.Tue, Sep 10, 3:21 AM
native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

The useProcessAndSendDMOperation hook indeed does use dispatchActionPromise: https://github.com/CommE2E/comm/blob/d1ef059b2362927a7f646a7616b1c70e4e31d0d1/lib/shared/dm-ops/process-dm-ops.js#L335

this wouldn't be too hard... we could do this with a combination of customKeyName and combineLoadingStatuses.

  1. Can this approach be implemented as of this diff or does it require separate diff/stack?
  2. If we take that approach , do we still want to proceed with separate hooks for each type of thread or does something change here?
ashoat added inline comments.Tue, Sep 10, 3:33 AM
native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

The useProcessAndSendDMOperation hook indeed does use dispatchActionPromise: https://github.com/CommE2E/comm/blob/d1ef059b2362927a7f646a7616b1c70e4e31d0d1/lib/shared/dm-ops/process-dm-ops.js#L335

That's great! Let me know if you have any questions about how to use customKeyName and combineLoadingStatuses here. I think you'll need to change the useProcessAndSendDMOperation API so that the function returned there takes the customKeyName as input. I can share some more pointers if helpful.

Can this approach be implemented as of this diff or does it require separate diff/stack?

Defer to you, but please prioritize readability!

If we take that approach , do we still want to proceed with separate hooks for each type of thread or does something change here?

I think the API should make it clear when rawThreadInfo needs to be specified.

I suggest two approaches to this: either implementing separate useJoinThinThread and useJoinThickThread hooks, or by having a single useJoinThread with an input similar to the UseJoinThreadInput type I proposed above.

I'm open to either solution, or an alternative that make it clear when rawThreadInfo needs to be specified.

marcin added inline comments.Tue, Sep 10, 3:44 AM
native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

I think the API should make it clear when rawThreadInfo needs to be specified. I suggest two approaches to this: either implementing separate useJoinThinThread and useJoinThickThread hooks, or by having a single useJoinThread with an input similar to the UseJoinThreadInput type I proposed above.

If we proceed with one hook with union of inputs, then why can't we use dispatchActionPromise as it is used in this diff?

ashoat added inline comments.Tue, Sep 10, 4:43 AM
native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

I'm not sure I understand the question.

Are you confused about why it's bad practice to dispatch an action from another action?

kamil added inline comments.Tue, Sep 10, 5:36 AM
lib/actions/thread-actions.js
354–356 ↗(On Diff #43941)

I described some different possible approaches here - we can discuss what is better there.

native/chat/chat-input-bar.react.js
1139–1142 ↗(On Diff #43941)

The useProcessAndSendDMOperation hook indeed does use dispatchActionPromise: https://github.com/CommE2E/comm/blob/d1ef059b2362927a7f646a7616b1c70e4e31d0d1/lib/shared/dm-ops/process-dm-ops.js#L335

The code you linked is only for non auto retriable messages, here, to use dispatchActionPromise you'll some additional changes to support that.

marcin updated this revision to Diff 44014.Tue, Sep 10, 1:36 PM

Implement union of types approach

Harbormaster failed remote builds in B31513: Diff 44014!Tue, Sep 10, 1:41 PM
marcin updated this revision to Diff 44015.Tue, Sep 10, 2:04 PM

Improve types

Harbormaster completed remote builds in B31514: Diff 44015.Tue, Sep 10, 2:22 PM
marcin added a child revision: D13287: Update input type of useChangeThreadSettings.Wed, Sep 11, 2:44 AM
ashoat accepted this revision.Wed, Sep 11, 10:15 AM
ashoat added inline comments.
lib/actions/thread-actions.js
355 ↗(On Diff #44015)

You implemented most of my suggestion, but skipped the $ReadOnly wrapper. Can you make sure to add that before landing?

This revision is now accepted and ready to land.Wed, Sep 11, 10:15 AM
marcin updated this revision to Diff 44071.Thu, Sep 12, 4:03 AM

Make input type read only

Harbormaster completed remote builds in B31554: Diff 44071.Thu, Sep 12, 4:41 AM
marcin updated this revision to Diff 44097.Thu, Sep 12, 9:51 AM

rebase before landing

Harbormaster completed remote builds in B31569: Diff 44097.Thu, Sep 12, 10:08 AM
Closed by commit rCOMMa91bc4cfd0f7: Implement joining DM thread (authored by marcin). · Explain WhyThu, Sep 12, 10:23 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMa91bc4cfd0f7: Implement joining DM thread.

Revision Contents
Changeset List

PathSize
lib/
actions/
72 lines
shared/
42 lines
dm-ops/
6 lines
types/
2 lines
native/
chat/
45 lines
web/
chat/
53 lines

Diff 44103

View Options

lib/actions/thread-actions.js

Loading...
View Options

lib/shared/community-utils.js

Loading...
View Options

lib/shared/dm-ops/dm-op-utils.js

Loading...
View Options

lib/types/thread-types.js

Loading...
View Options

native/chat/chat-input-bar.react.js

Loading...
View Options

web/chat/chat-input-bar.react.js

Loading...