Message reactions POC #445
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
vladvelici
requested review from
splindsay-92 and
AndyTWF
and removed request for
splindsay-92
| * (eg. reactions) this is the `serial` of the relevant chat message, usually | ||
| * found under `refSerial`. | ||
| */ | ||
| get messageSerial(): string; |
There was a problem hiding this comment.
I'm not so sure about conflating the two here, unless we decide to call it something like parentSerial ? I'll have more of a think on this, perhaps we can do some funky union type
There was a problem hiding this comment.
I guess the word message has too many meanings. If it was an pubsub message, totally confusing. If we talk about a chat message (my intention), it should be clear it's the serial of the chat message (and for reactions it's the serial of the message it refers to).
The purpose is to be able to get the serial of the chat message for any event easily (no type-casting, no switch statements). Pseudocode:
subscribeAll(event => {
storedMessage = findMessageBySerial(event.messageSerial)
updateMessage(event.messageSerial, storedMessage.apply(event))
});
There was a problem hiding this comment.
Can definitely rename to parentSerial
There was a problem hiding this comment.
I would agree with parentSerial - though does having the convenience method possibly make things a bit obscure?
Would it be more explicit to have the serial on the individual types - so MessageEventPayload simply has messageSerial and the annotation ones has parentSerial?
Im concerned we might be lumping too many principles into one here
There was a problem hiding this comment.
I think it makes development easier if you can get the base message serial without any type-casting in the case of subscribeAll: you can ignore events early on in your listener, you can find the base message, and you can call message.apply() without type-casting as well.
If this isn't a use case we care about, we can just remove this field. The typed events already have either serial or refSerial which also matches realtime.
| @@ -202,6 +202,18 @@ export class ChatApi { | |||
| ); | |||
| } | |||
|
|
|||
| async reactToMessage(roomId: string, serial: string, reaction: string): Promise<void> { | |||
| roomId = encodeURIComponent(roomId); | |||
| return this._makeAuthorizedRequest(`/channels/${roomId}::$chat::$chatMessages/messages`, 'POST', { | |||
There was a problem hiding this comment.
Imagine this is temporary - but we should definitely have a dedicated chat endpoint for this
There was a problem hiding this comment.
It's temporary, I wanted to get something working end-to-end first
| return this._makeAuthorizedRequest(`/channels/${roomId}::$chat::$chatMessages/messages`, 'POST', { | ||
| action: 4, | ||
| data: reaction, | ||
| refType: 'reaction:emoji.v1', |
There was a problem hiding this comment.
We should be using the text reaction reftype - so people aren't limited to just UTF-8 emoji's
| * (eg. reactions) this is the `serial` of the relevant chat message, usually | ||
| * found under `refSerial`. | ||
| */ | ||
| get messageSerial(): string; |
There was a problem hiding this comment.
I would agree with parentSerial - though does having the convenience method possibly make things a bit obscure?
Would it be more explicit to have the serial on the individual types - so MessageEventPayload simply has messageSerial and the annotation ones has parentSerial?
Im concerned we might be lumping too many principles into one here
| /** | ||
| * Payload for a message event. | ||
| */ | ||
| export interface MessageEventPayload extends EventPayload { |
There was a problem hiding this comment.
I was having a think about this. In TypeScript its quite easy for us to say "check the enum, then assume the object shape". In other languages, it's a bit less forgiving / frowned upon. Generally speaking - type safety and not relying on users to do conversions themselves is good.
Should we consider other ways to subscribe to messages, e.g. separate methods for messages themselves and annotations?
I don't have a strong view right now - but just want to make sure we consider type systems outside of TS here
|
|
||
| apply(event: AnyMessageEvent): Message { | ||
| if (event.messageSerial !== this.serial) { | ||
| throw new ErrorInfo('apply(): Cannot apply event to message, serials do not match', 50000, 500); |
There was a problem hiding this comment.
This would be 4xx as would be user-error
|
|
||
| switch (event.type) { | ||
| case MessageEvents.Created: { | ||
| // created events shouldn't get here, we'll treat as no-op |
There was a problem hiding this comment.
I think we should explicitly call this out as a 4xx
| } | ||
|
|
||
| // Clone a message, optionally replace the given fields | ||
| private static clone(source: Message, replace?: Partial<Message>): DefaultMessage { |
There was a problem hiding this comment.
This wouldn't be a straight-up clone, as this message would share reference types such as the reactions and metadata.
For deep cloning, you can do import cloneDeep from 'lodash.clonedeep';
There was a problem hiding this comment.
Yeah. Will use cloneDeep for this and the reactions.
| } | ||
| } | ||
|
|
||
| function cloneReactions(obj: Map<string, MessageReactionSummary>): Map<string, MessageReactionSummary> { |
There was a problem hiding this comment.
Use lodash.clonedeep for this?
| /** | ||
| * Add a reaction to a message. | ||
| */ | ||
| react(message: Message, reaction: string): Promise<void>; |
There was a problem hiding this comment.
I'd suggest addReaction, only because eventually we'll want to remove
There was a problem hiding this comment.
True, will change. addReaction and removeReaction are better than react and removeReaction.
| subscribe(listener: MessageListener): MessageSubscriptionResponse { | ||
| this._logger.trace('Messages.subscribe();'); | ||
| super.on([MessageEvents.Created, MessageEvents.Updated, MessageEvents.Deleted], listener); | ||
| subscribe(listener: MessageListener | MessageListenerObject): MessageSubscriptionResponse { |
There was a problem hiding this comment.
Given we're in the 0.x versions, I'd be comfortable with just making the breaking change - rather than having both versions
There was a problem hiding this comment.
Happy to remove the MessageListener part
Context
Does not yet have:
JIRA:
Checklist
Testing Instructions