-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor reactions / hand raised to use rxjs and start ordering tiles based on hand raised. #2885
Conversation
src/useReactions.tsx
Outdated
@@ -92,19 +99,38 @@ export const ReactionsProvider = ({ | |||
clientState?.state === "valid" && clientState.supportedFeatures.reactions; | |||
const room = rtcSession.room; | |||
const myUserId = room.client.getUserId(); | |||
const myDeviceId = room.client.getDeviceId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially this hook could be split up, and listening for incoming reactions could be moved somewhere else (maybe even MatrixRTCSession?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split the sending and receiving logic pieces into a provider and a producer-style hook, using the call view model for state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useReactionsReader
refactor looks like a good step forward. It's converted the data sources into observables which can now affect the view model, though while staying within React code.
I'm wondering if you feel up to the extra step of moving that hook's code fully out of React and into the view model. It would be a more suitable environment for creating these observables (as the class body runs once, rather than on every render), and it also gets us closer to a world in which there's a clear stratification between Model (matrix-js-sdk, LiveKit) → View Model (RxJS) → View (React), so that the View Models can become entirely external to React, with a unidirectional flow of data.
(That's something I haven't been very explicit about: that the current approach of letting a React effect create the CallViewModel is a bit of a hack, necessary because the app didn't start out with a view model architecture.)
I've half gone with your suggestion, but I factored the reaction logic into it's own class which exports it's own observables. My rationale is the code for doing reactions handling isn't trivial, and I find it easier to parse when you have an explicit class to handle and test, which then passes it's observables into the call view model. I think we could, if you felt very strongly here, port the code it the CVM but I worry about the size of that class.. Anyway! No more react which is the main thing :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! Splitting call logic across multiple files is a good call.
src/reactions/ReactionsReader.ts
Outdated
this.reactionsSubject$ | ||
.pipe(delay(REACTION_ACTIVE_TIME_MS)) | ||
.subscribe((reactions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This subscription should be bound to some ObservableScope
so that it doesn't run forever
916009d
to
20f2ca4
Compare
Fixes #2715
Fixes #2843
This change will adjust our code so that consumers of reactions and hand raised state use the CallViewModel as much as possible, which allows us to finally do fun stuff like order tiles by hand raised.
The changes are as follows:
useReactions
is no more. There is nowuseReactionsReader
which consumes reaction/HR events, and emits them inBehaviourSubjects
. The sending code has been refactored into aReactionsSenderProvider
.CallViewModel
now consumes these two subjects, and includes a range of observables for our presentation components. The presentation components now contain very little selection logic, and focus on...presenting :)And, dear reviewer, I'm very sorry for the diff :(. I hope it's worth it.