[MM-53081] Redux store refactor #510

streamer45 · 2023-08-30T18:45:54Z

Summary

PR reviews most of our reducers and selectors to try and improve a few things:

Renaming a bunch of very badly thought and/or confusing objects.
- Preferring to be on the verbose side (e.g. profilesInCallInCurrentChannel), clarity over efficiency.
Make selectors more performant through proper use of createSelector.
- Mostly tried to optimize all those instances in which we wanted something for the current call or channel. There may be a couple of additional improvements but nothing major I think.
Trying to find some consistency in how we design reducers.
- On this I am trying to stick to the idea that for anything immutable related to a call (e.g. startAt, threadID) we can use a common call state but if something can change throughout the call then we should have a dedicated reducer (e.g. hostID, screenSharingID).

Notes

E2E passing is the first step to validate this but there's always a chance we will break something untested so we should plan a long enough soaking time on Community.
It would be preferable if any future non urgent PRs touching these files could be based on top of this one to avoid endless conflicts.

Ticket Link

https://mattermost.atlassian.net/browse/MM-53081

streamer45 · 2023-08-30T18:47:10Z

webapp/src/reducers.ts

+    CALL_END,
+    CALL_REC_PROMPT_DISMISSED,


I stripped most of the unnecessary prefixes in this file with the assumption that it's all Calls related. Please let me know if you'd prefer a more verbose take on this side as well.

Definitely this is better, thanks!

codecov-commenter · 2023-08-30T18:54:12Z

Codecov Report

Patch coverage has no change and project coverage change: +0.07% 🎉

Comparison is base (9fdbb65) 5.50% compared to head (e0a6a8c) 5.58%.

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #510      +/-   ##
========================================
+ Coverage   5.50%   5.58%   +0.07%     
========================================
  Files         23      23              
  Lines       4265    4265              
========================================
+ Hits         235     238       +3     
+ Misses      4012    4010       -2     
+ Partials      18      17       -1

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cpoile

Sorry I don't have any more substantive comments, it looks good. Should bring more clarity when you're deep into the code and searching for the right selector.
Thanks!

cpoile · 2023-09-05T19:44:54Z

webapp/src/selectors.ts


-export const connectedChannel: (state: GlobalState) => Channel | null =
+export const channelForCurrentCall: (state: GlobalState) => Channel =


should this be typed to return Channel | undefined?

Yeah, would be safer. I'll update.

cpoile · 2023-09-05T20:02:03Z

webapp/src/reducers.ts

+    CALL_END,
+    CALL_REC_PROMPT_DISMISSED,


Definitely this is better, thanks!

cpoile · 2023-09-05T20:19:32Z

webapp/src/reducers.ts

+// callState should only hold immutable data, meaning those
+// fields that don't change for the whole duration of a call.
+export type callState = {
+    ID: string;
+    startAt: number;
+    channelID: string;
+    threadID: string;
+    ownerID: string;
+}


Maybe we should put this in the types file?

Yeah, I am trying to find a compromise between client side state, server side state and redux state. Not super easy at times. Will look into this.

@cpoile Let me track a proper review of types as a separate effort especially since I am planning some changes on that front.

Created https://mattermost.atlassian.net/browse/MM-54429

cpoile · 2023-09-05T20:20:12Z

webapp/src/reducers.ts

+export type hostsState = {
+    [channelID: string]: {
+        hostID: string;
+        hostChangeAt?: number;
+    };
+}
+


Maybe this too?

cpoile · 2023-09-05T20:21:55Z

webapp/src/reducers.ts

+export type recentlyJoinedUsersState = {
+    [channelID: string]: string[];


I guess this too, if it's being used outside the current file?

cpoile · 2023-09-05T20:23:50Z

webapp/src/selectors.ts

 };

-export const voiceChannelCallHostID = (state: GlobalState, channelID: string) => {
-    return pluginState(state).voiceChannelCalls[channelID]?.hostID;
+const hostsInCalls = (state: GlobalState): hostsState => {


I'm not crazy about the name here... But I can't think of anything better. Something like hostByCall?

It's really just internal given it's the base selector so we may as well just call it hosts or callsHosts. Ultimately it doesn't matter that much I guess.

streamer45 added 20 commits August 30, 2023 11:35

Rename reducers and actions

0d10ec1

Renaming part 2

4abf824

Strip unnecessary prefix

4303190

Consistency with reducers types

0993c3e

More consistency

1eaca6f

Selectors review part 1

e094c94

Add missing dispatch

cf6ea3c

usersStatusesInCurrentCall

4371b73

reactionsInCurrentCall

85fcabe

Reorder

ee28d54

Rename call ID selector

2d773d8

hostIDInCurrentCall & hostChangeAtInCurrentCall

4db12b5

screenSharingIDInCurrentCall

9dd8b66

recordingInCurrentCall & isRecordingInCurrentCall

22d08a9

recentlyJoinedUsersInCurrentCall

e45e875

Better use of connectedChannel

a9869e7

callStartAt

587f68b

More consistency

1c3c755

Revert spurious changes

096a5a9

More revert

90a6fe3

streamer45 added 2: Dev Review Requires review by a core committer Do Not Merge Should not be merged until this label is removed labels Aug 30, 2023

streamer45 requested a review from cpoile August 30, 2023 18:45

streamer45 self-assigned this Aug 30, 2023

streamer45 commented Aug 30, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into MM-53081

8769964

cpoile approved these changes Sep 5, 2023

View reviewed changes

streamer45 added 2 commits September 6, 2023 11:25

Merge remote-tracking branch 'origin/main' into MM-53081

58e7c95

Handle potentially undefined channel

57588a5

streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer Do Not Merge Should not be merged until this label is removed labels Sep 6, 2023

Fix potential undefined team

e0a6a8c

streamer45 added this to the v0.19.0 - MM 9.0 milestone Sep 6, 2023

Merge remote-tracking branch 'origin/main' into MM-53081

2c9d5b8

streamer45 merged commit 7b200f3 into main Sep 7, 2023

streamer45 deleted the MM-53081 branch September 7, 2023 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-53081] Redux store refactor #510

[MM-53081] Redux store refactor #510

streamer45 commented Aug 30, 2023

streamer45 Aug 30, 2023

cpoile Sep 5, 2023

codecov-commenter commented Aug 30, 2023 •

edited

Loading

cpoile left a comment

cpoile Sep 5, 2023

streamer45 Sep 6, 2023

cpoile Sep 5, 2023

cpoile Sep 5, 2023

streamer45 Sep 5, 2023

streamer45 Sep 6, 2023

streamer45 Sep 6, 2023

cpoile Sep 5, 2023

cpoile Sep 5, 2023

cpoile Sep 5, 2023

streamer45 Sep 5, 2023


		export const connectedChannel: (state: GlobalState) => Channel \| null =
		export const channelForCurrentCall: (state: GlobalState) => Channel =

		export type recentlyJoinedUsersState = {
		[channelID: string]: string[];

[MM-53081] Redux store refactor #510

[MM-53081] Redux store refactor #510

Conversation

streamer45 commented Aug 30, 2023

Summary

Ticket Link

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 30, 2023 • edited Loading

Codecov Report

cpoile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 30, 2023 •

edited

Loading