Repository navigation
feat(core): Show sent chat messages right away, with new status attribute and resend function
#1497
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| '@livekit/components-core': minor | ||
| '@livekit/components-react': minor | ||
| --- | ||
|
|
||
| Show sent chat messages right away. `send` now adds the message as soon as it's called, with a | ||
| `status` of `sending`, then `sent` or `failed`, and keeps a failed message in place. Use the new | ||
| `resend` from `useChat` or `useSessionMessages` to send it again. Messages sent from this client now | ||
| keep a local id for their whole lifetime instead of taking the text stream id once sent. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* eslint-disable camelcase */ | ||
| import type { Room, SendTextOptions } from 'livekit-client'; | ||
| import type { Room, SendTextOptions, TextStreamInfo } from 'livekit-client'; | ||
| import { compareVersions, RoomEvent } from 'livekit-client'; | ||
| import { | ||
| BehaviorSubject, | ||
|
|
@@ -69,8 +69,24 @@ export type ChatOptions = { | |
| type ChatTopicState = { | ||
| messageSubject: Subject<ReceivedChatMessage>; | ||
| onDestroyObservable: Subject<void>; | ||
| /** Messages sent from this client that haven't been sent successfully yet, kept for `resend`. */ | ||
| unsentMessages: Map< | ||
| string, | ||
| { message: ReceivedChatMessage; options: SendTextOptions; isSending: boolean } | ||
| >; | ||
| }; | ||
|
|
||
| let localMessageCount = 0; | ||
|
|
||
| /** | ||
| * `sendText` only returns the stream id once sending finishes, so a message sent from this client | ||
| * gets its own id up front and keeps it, whether it's sending, sent, or failed. | ||
| */ | ||
| function createLocalMessageId() { | ||
| localMessageCount += 1; | ||
| return `local-${Date.now().toString(36)}-${localMessageCount}`; | ||
| } | ||
|
|
||
| const topicSubjectMap: WeakMap<Room, Map<string, ChatTopicState>> = new WeakMap(); | ||
| const streamIdToAttachments = new Map< | ||
| string /* stream id */, | ||
|
|
@@ -112,8 +128,9 @@ export function setupChat(room: Room, options?: ChatOptions) { | |
| const topicState = topicMap.get(topic) ?? { | ||
| messageSubject: new Subject<ReceivedChatMessage>(), | ||
| onDestroyObservable: new Subject<void>(), | ||
| unsentMessages: new Map(), | ||
| }; | ||
| const { messageSubject, onDestroyObservable } = topicState; | ||
| const { messageSubject, onDestroyObservable, unsentMessages } = topicState; | ||
| topicMap.set(topic, topicState); | ||
| topicSubjectMap.set(room, topicMap); | ||
|
|
||
|
|
@@ -254,11 +271,14 @@ export function setupChat(room: Room, options?: ChatOptions) { | |
| const replaceIndex = acc.findIndex((msg) => msg.id === value.id); | ||
| if (replaceIndex > -1) { | ||
| const originalMsg = acc[replaceIndex]; | ||
| acc[replaceIndex] = { | ||
| ...value, | ||
| timestamp: originalMsg.timestamp, | ||
| editTimestamp: value.timestamp, | ||
| }; | ||
| // A status change on a message sent from this client isn't an edit | ||
| acc[replaceIndex] = value.status | ||
| ? value | ||
| : { | ||
| ...value, | ||
| timestamp: originalMsg.timestamp, | ||
| editTimestamp: value.timestamp, | ||
| }; | ||
| } | ||
| return [...acc]; | ||
| } | ||
|
|
@@ -271,37 +291,36 @@ export function setupChat(room: Room, options?: ChatOptions) { | |
| let pendingSends = 0; | ||
| const finalMessageEncoder = options?.messageEncoder ?? encodeLegacyMsg; | ||
|
|
||
| const send = async (message: string, options?: SendTextOptions) => { | ||
| if (!options) { | ||
| options = {}; | ||
| } | ||
| options.topic ??= topic; | ||
| /** | ||
| * Adds the message as `sending`, then marks it `sent` or `failed`. Rejects if sending fails, | ||
| * leaving the failed message in place for `resend`. | ||
| */ | ||
| const deliver = async (unsentMessage: ReceivedChatMessage, options: SendTextOptions) => { | ||
| const { id, message } = unsentMessage; | ||
| unsentMessages.set(id, { message: unsentMessage, options, isSending: true }); | ||
| pendingSends += 1; | ||
| isSending$.next(true); | ||
| messageSubject.next({ ...unsentMessage, status: 'sending' }); | ||
|
|
||
| try { | ||
| const info = await room.localParticipant.sendText(message, options); | ||
| let info: TextStreamInfo; | ||
| try { | ||
| info = await room.localParticipant.sendText(message, options); | ||
| } catch (error) { | ||
| unsentMessages.set(id, { message: unsentMessage, options, isSending: false }); | ||
| messageSubject.next({ ...unsentMessage, status: 'failed' }); | ||
| throw error; | ||
| } | ||
|
Comment on lines
+294
to
+313
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Just generally, I'm curious if you have any data around how often chat message sending fails. Even anecdotally is fine - I've yet to see one fail in local development. But it's possible I just have been treating it with kid gloves, and I haven't deeply tested cases like switching wifi networks / etc Broader context: It's definitely possible for a data stream to fail but the bar for that has gotten a lot higher now with data streams v2. I'm actually going to be doing an EPD all hands presentation about this next month (and can send you all over the slides early if you'd be interested in learning more) but the TLDR is data streams are now over 50% smaller than they used to be, which means that on phones and poor bandwidth network connections it becomes substantially harder for them to drop (though not impossible). I bring it up because while I think this is cool and lets a user implement a pretty nice UX, I also wonder if the complexity is really warranted if a data stream delivery failure is that much harder now. Something like interface pattern that the existing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No data on my end, just seemed like something to think through, even if rare (and even less rare with data streams v2 — nice). I'm fine keeping it out for now. If it becomes an issue we can always take care of it then. |
||
|
|
||
| unsentMessages.delete(id); | ||
| const receivedChatMsg: ReceivedChatMessage = { ...unsentMessage, status: 'sent' }; | ||
| messageSubject.next(receivedChatMsg); | ||
|
|
||
| const legacyChatMsg: LegacyChatMessage = { | ||
| id: info.id, | ||
| timestamp: Date.now(), | ||
| timestamp: unsentMessage.timestamp, | ||
| message, | ||
| }; | ||
|
|
||
| const chatMsg: ChatMessage = { | ||
| ...legacyChatMsg, | ||
| attachedFiles: options.attachments, | ||
| }; | ||
|
|
||
| const receivedChatMsg: ReceivedChatMessage = { | ||
| ...chatMsg, | ||
| type: 'chatMessage', | ||
| from: room.localParticipant, | ||
| attributes: options.attributes, | ||
| }; | ||
|
|
||
| messageSubject.next(receivedChatMsg); | ||
|
|
||
| const encodedLegacyMsg = finalMessageEncoder({ | ||
| ...legacyChatMsg, | ||
| ignoreLegacy: serverSupportsDataStreams(), | ||
|
|
@@ -323,9 +342,43 @@ export function setupChat(room: Room, options?: ChatOptions) { | |
| } | ||
| }; | ||
|
|
||
| /** Sends a message. It appears in the messages right away with a `sending` status. */ | ||
| const send = async (message: string, options?: SendTextOptions) => { | ||
| if (!options) { | ||
| options = {}; | ||
| } | ||
| options.topic ??= topic; | ||
|
|
||
| return deliver( | ||
| { | ||
| id: createLocalMessageId(), | ||
| timestamp: Date.now(), | ||
| message, | ||
| attachedFiles: options.attachments, | ||
| type: 'chatMessage', | ||
| from: room.localParticipant, | ||
| attributes: options.attributes, | ||
| }, | ||
| options, | ||
| ); | ||
| }; | ||
|
|
||
| /** Sends a failed message again, keeping its id and place in the messages. */ | ||
| const resend = async (messageId: string) => { | ||
| const unsent = unsentMessages.get(messageId); | ||
| if (!unsent) { | ||
| throw new Error(`No failed message with id ${messageId} to resend`); | ||
| } | ||
| if (unsent.isSending) { | ||
| throw new Error(`Message ${messageId} is already sending`); | ||
| } | ||
| return deliver(unsent.message, unsent.options); | ||
| }; | ||
|
|
||
| return { | ||
| messageObservable: messagesObservable, | ||
| isSendingObservable: isSending$, | ||
| send, | ||
| resend, | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,12 +15,21 @@ type ReceivedMessageWithType<Type extends string, Metadata extends object = obje | |||||||||||||||||||||||||||||||||||||||||||
| attributes?: Record<string, string>; | ||||||||||||||||||||||||||||||||||||||||||||
| } & Metadata; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||
| * Where a message sent from this client is in delivery. `sent` means the LiveKit server accepted | ||||||||||||||||||||||||||||||||||||||||||||
| * it, not that other participants have read it. | ||||||||||||||||||||||||||||||||||||||||||||
| * @public | ||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||
| export type ChatMessageStatus = 'sending' | 'sent' | 'failed'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /** @private */ | ||||||||||||||||||||||||||||||||||||||||||||
| type ReceivedChatMessageWithRequiredType = ReceivedMessageWithType< | ||||||||||||||||||||||||||||||||||||||||||||
| 'chatMessage', | ||||||||||||||||||||||||||||||||||||||||||||
| ChatMessage & { | ||||||||||||||||||||||||||||||||||||||||||||
| from?: Participant; | ||||||||||||||||||||||||||||||||||||||||||||
| attributes?: Record<string, string>; | ||||||||||||||||||||||||||||||||||||||||||||
| /** Set only on messages sent from this client. */ | ||||||||||||||||||||||||||||||||||||||||||||
| status?: ChatMessageStatus; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| >; | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
26
to
34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I don't necessarily like some messages having a field and some not having a different field without it being clear in what cases that field can be set. What if you were to switch this to be some sort of discriminated union based on
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree the type should make it clear when Something to settle in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This was what I was trying to accomplish with |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
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.
suggestion: Today, stream ids are uuids: https://github.com/livekit/client-sdk-js/blob/4721d094b2062b08b12093f7eb8f5597bbff235c/src/room/data-stream/outgoing/OutgoingDataStreamManager.ts#L84. If you made this a uuid then from the perspective of a downstream user, the api interface would appear exactly the same.