feat!: move send with pending attachments logic to LLC - #3821
Conversation
SDK Size
|
isekovanic
left a comment
There was a problem hiding this comment.
Looks great ! Thanks for taking this up 🤝
Have a few nitpicky comments but other than that LGTM
| const localId = attachment.custom?.localId; | ||
| const localId = isLocalUploadAttachment(attachment) ? attachment.localMetadata.id : undefined; | ||
|
|
||
| const defaultOnPress = () => openUrlSafely(attachment.asset_url); |
There was a problem hiding this comment.
This still reads attachment.asset_url, but after this PR a file attachment whose upload hasn't
settled has no asset_url if I understand correctly. Since the URL lives in localMetadata and openUrlSafely doesn't guard
undefined, this ends up at 'http://' + undefined, and thencanOpenURL('http://undefined') returns
true and tapping the attachment opens the browser at http://undefined.
I guess should be reproducible by just clicking on a file attachment whose upload hasn't finished yet (with async uploads enabled).
I think guarding openUrlSafely would be great (in general too) and would resolve this
There was a problem hiding this comment.
Thanks for catching this, fixed in 4245ca5
There was a problem hiding this comment.
| totalBytes={resolveAttachmentFullByteSize(attachment)} |
I'm guessing this also needs to match how FileAttachment handles it ? Otherwise it would just be a spinner (with no max size)
There was a problem hiding this comment.
It works because custom.file_size is already set here: https://github.com/GetStream/stream-chat-js/blob/release-v10/src/messageComposer/attachmentManager.ts#L502 - so for RN both works, but I wanted to use the method defined by stream-chat to futureproof us, so changed it everywhere for resolveAttachmentFullByteSize in 173ca72
There was a problem hiding this comment.
| totalBytes={resolveAttachmentFullByteSize(attachment)} |
same here I think
There was a problem hiding this comment.
| totalBytes={resolveAttachmentFullByteSize(attachment)} |
same here
| * The value this hook wrote is remembered, so a later `enableOfflineSupport` change moves the | ||
| * default along with it, while a value registered by anyone else is never overwritten. | ||
| */ | ||
| export const usePendingUploadsDefault = (client: StreamChat, enableOfflineSupport: boolean) => { |
There was a problem hiding this comment.
I get what the hook is trying to do, but I think it can be simplified as:
| export const usePendingUploadsDefault = (client: StreamChat, enableOfflineSupport: boolean) => { | |
| export const usePendingUploadsDefault = (client: StreamChat, enableOfflineSupport: boolean) => { | |
| useEffect(() => { | |
| if (!client || !enableOfflineSupport) { | |
| return; | |
| } | |
| const registered = | |
| client.config.getConfig('messageComposer')?.attachments?.pendingUploadsEnabled; | |
| if (registered !== undefined) { | |
| return; | |
| } | |
| client.config.setConfig('messageComposer', { | |
| attachments: { pendingUploadsEnabled: true }, | |
| }); | |
| // Deliberately no teardown, for the same reason as the network reporter in `useIsOnline`: the | |
| // configuration lives as long as the client, which outlives `<Chat>`. | |
| }, [client, enableOfflineSupport]); | |
| }; |
the reason being, if the following chain of events occurs:
- we enable offline support
- we mount
Chat(and the hook runs) - after this, we manually set the config
pendingUploadsconfig totrue(but the hook is unaware of this yet) - further down the line, we disable offline support
- the hook now runs again and sets the config to
false(even though we've already set it totrueourselves)
with the above hook, the only downside is that if we leave it to the defaults, setting offline to true and then to false after that will keep pending uploads to true. But this is kinda expected behaviour perhaps.
I know it's a weird edge case but probably safer.
What're your thoughts here ?
There was a problem hiding this comment.
I agree with only setting if the integrators haven't set anything, fixed here: b8e11b8
isekovanic
left a comment
There was a problem hiding this comment.
Cool ! Please update the AI migration guide when you get the chance as well so it reflects the changes
Moves the logic for sending a message with pending (in progress or failed) attachments to the LLC.
Breaking changes
ChanneldoSendMessageRequestis removed, useclient.config.setto register a channel request handlerallowSendBeforeAttachmentsUploadis removed:pendingUploadsEnabledat the client level totrueif offline support is enabled and integrators didn't already set a value forpendingUploadsEnabledThe previous solution stored pending attachment metadata in
attachment.customandattachment.image_url/attachment.asset_urlthe new shape:The easiest way to display attachments is to use the two new helper methods:
getAttachmentPreviewUrl-> defined bystream-chat-jsgetPlayableVideoUrl-> defined by RN SDK, used when we want to play a video (in this case we can't return the thumb URL - that the SDK creates)I also checked this comment: GetStream/stream-chat-js#1845 (comment) -> it doesn't cause any issues because the SDK always reads upload state from
uploadManager, so a staleuploadingstate won't cause any issues. That being said typing wise it's not really developer friendly that we emit this data sincelocalMetadatais never refreshed after the message is sent, so even in React integrators can think it's ok to read these fields, when it's not. But that issue is not RN specific.