Page MenuHomePhabricator

[web] Support chat creation in `ChatMessageListContainer`
ClosedPublic

Authored by jacek on Jul 7 2022, 4:00 AM.
Tags
None
Referenced Files
F3386634: D4465.id14938.diff
Fri, Nov 29, 5:28 AM
F3386570: D4465.id14393.diff
Fri, Nov 29, 5:06 AM
Unknown Object (File)
Mon, Nov 25, 4:41 PM
Unknown Object (File)
Mon, Nov 25, 2:49 PM
Unknown Object (File)
Fri, Nov 15, 12:35 PM
Unknown Object (File)
Tue, Nov 12, 10:20 PM
Unknown Object (File)
Tue, Nov 12, 1:12 AM
Unknown Object (File)
Tue, Nov 12, 1:12 AM

Details

Summary

The diff extends ChatMessageListContainer with chat creation state.
When user enters chat chreation, the component uses useExistingThreadInfoFinder with pending personal private thread set as baseThread - so the behavior is the same as in native app.
Also, in thread creation mode, thread top bar, message list and input bar are displayed only if any users has been selected (so some thread from existingThreadInfoFider could be matched)

Test Plan

Currently there is no way in the app to enter the view by UI. The component can be displayed after entring url http://localhost/comm/chat/thread/new/.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Jul 20 2022, 4:06 AM
tomek added inline comments.
web/chat/chat-message-list-container.react.js
37–54 ↗(On Diff #14393)

A lot of work is done that isn't necessary when isChatCreation is false. Is there a way to avoid this? Maybe we can have a separate component that includes these hooks and is rendered conditionally based on the flag?

87 ↗(On Diff #14393)

What is the scenario in which this effect is needed?

95–96 ↗(On Diff #14393)

The 2nd condition is wider, so maybe we need only that one?

This revision now requires changes to proceed.Jul 20 2022, 4:06 AM
web/chat/chat-message-list-container.react.js
87 ↗(On Diff #14393)

When user enter manually IDs into URL (like http://localhost/comm/chat/thread/new/84072,83850,99999) and some of them are not in userStore.

95–96 ↗(On Diff #14393)

Probably yes, but isn't it faster to enter this branch by just confirming that sizes are different, than creating new set and invoke _isEqual?

Returning to review after discussion with Tomek about other possible solutions

tomek added inline comments.
web/chat/chat-message-list-container.react.js
37–54 ↗(On Diff #14393)

Discussed offline - this approach was selected to avoid renders and possible state loss when switching from one mode to another - just like we do on native.

95–96 ↗(On Diff #14393)

I don't think this optimization is really beneficial. Most of the times, we will still check the 2nd condition - after almost every dependency change. If we want to optimize here, we can create a memo with this Set and use it instead.

This revision is now accepted and ready to land.Jul 25 2022, 4:59 AM
web/chat/chat-message-list-container.react.js
95–96 ↗(On Diff #14393)

Didn't perform any tests with this, but I suppose this effect will usually be executed when adding or removing a user to the list, so in most cases the array length will change and we won't have to compare these arrays in the second condition.
However, this probably doesn't impact performance much. Should I leave it as it is, or simplify the condition?

web/chat/chat-message-list-container.react.js
95–96 ↗(On Diff #14393)

When a performance difference isn't significant, I would prefer the readability. In this case, it would be slightly more readable if we had only one condition. But the readability difference is insignificant, so we can also keep it as it is. Up to you.