add media sharing document #2683
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 ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -0,0 +1,160 @@ | |||
| --- | |||
There was a problem hiding this comment.
I think these pages should go in the main docs section, rather than the guides - as the intention is to cover off features that are trivial to implement rather than a use-case guide (it's slightly misleading but the guides folder is for wider chat-use cases :) )
| title: "Sharing media with Ably Chat" | ||
| meta_description: "How to share media like images, videos, or files with Ably Chat" | ||
| meta_keywords: "media, sharing, Ably Chat, chat SDK, realtime messaging, dependability, cost optimisation" | ||
| --- |
There was a problem hiding this comment.
The page needs to be added to src/data/nav/chat.ts for it to show up
|
|
||
| We'll build a simple chat room where users can share images with each other. | ||
|
|
||
| We also assume that `room` is an Ably chat room that is attached. Please refer to our getting started guide for step-by-step instructions on how to set up Ably Chat and get a chat room. |
There was a problem hiding this comment.
I would suggest we include a link here to the JS GS guide
|
|
||
| We also assume that `room` is an Ably chat room that is attached. Please refer to our getting started guide for step-by-step instructions on how to set up Ably Chat and get a chat room. | ||
|
|
||
| We assume that a function called `uploadImage() : Promise<string>` exists and handles the whole image upload flow: asks users to pick a picture, uploads it, and returns a unique identifier for this image. |
There was a problem hiding this comment.
The one above says "also assume", should it be the other way around?
| let imageId = 'abcd123abcd'; // assume this is the image we want to remove | ||
| let message = ...; // assume this is the message we want to edit | ||
|
|
||
| if (!message.metadata.images || message.metadata.images.length === 0) { return; /* do nothing if no images */} |
There was a problem hiding this comment.
I think we should put the return on a new line, looks cleaner
|
|
||
| Our moderation integration is currently limited to text moderation. To add automatic or human moderation for images (and other attachments) you must implement moderation on the server side. A recommended flow is: | ||
|
|
||
| 1. Upload the media to your storage service (can be S3 or others) |
There was a problem hiding this comment.
I would suggest making this a synchronous process - avoids race conditions, and usually uploading images you'd accept that it takes a bit longer
There was a problem hiding this comment.
Send message and then upload pictures? Or start both and edit the message when the pictures are ready?
I wanted to avoid the edit-loop where you need to edit the message to attach images. It is a valid flow especially for moderation-before-publish, where image metadata can be sent without image (so UI can display a "processing" message or something) and an edit adds the image links post-moderation. The initial message gets sent before uploads finish. Potentially start the uploads after message is sent so you have a serial and the edit happens server-side?
|
Generally speaking, I would suggest that instead of saying we/our, say "Ably Chat" |
| ```javascript | ||
| const imageIdRegex = /^[a-z0-9]{10,15}$/; | ||
|
|
||
| function getValidImages(message) { |
There was a problem hiding this comment.
Should we include the type here for clarity?
There was a problem hiding this comment.
I went for JS instead of TS but happy to swap and add types
There was a problem hiding this comment.
Ah yeh, if the tag is js then that's fine :)
| ```javascript | ||
| const imageIdRegex = /^[a-z0-9]{10,15}$/; | ||
|
|
||
| function getValidImages(message) { |
There was a problem hiding this comment.
Is it worth adding the expected type here so users know this is for a Message?
|
|
||
| Instead, add an authetnication layer on the server side to control access to the media. This allows you full control over who can access the files. | ||
|
|
||
| If you must serve the files directly form a service such as AWS S3 consider saving a file name in the metadata of the message, and have a mechanism for the user to request a signed URL to the file. |
There was a problem hiding this comment.
| If you must serve the files directly form a service such as AWS S3 consider saving a file name in the metadata of the message, and have a mechanism for the user to request a signed URL to the file. | |
| If you must serve the files directly form a service, such as AWS S3, consider saving a file name in the metadata of the message, and have a mechanism for the user to request a signed URL to the file. |
vladvelici
force-pushed
the
chat-image-sharing-doc-CHA-965-CHA-966
branch
from
1444c1d to
6a80a18
Compare
|
Thanks @splindsay-92 and @AndyTWF for the review. I've made all the changes (one question remaining). I'll rebase the commits before merging. Would you be able to have another look please? |
There was a problem hiding this comment.
Happy with the overall content - as this is the first guide of this type, would be good to get some @m-hulbert feedback
vladvelici
force-pushed
the
chat-image-sharing-doc-CHA-965-CHA-966
branch
from
6a80a18 to
fb214ba
Compare
src/data/nav/chat.ts
Outdated
| @@ -84,6 +84,10 @@ export default { | |||
| name: 'Room reactions', | |||
| link: '/docs/chat/rooms/reactions', | |||
| }, | |||
| { | |||
| name: 'Media sharing', | |||
| link: '/docs/chat/rooms/media-sharing', | |||
There was a problem hiding this comment.
Should this really go at the rooms level? Do we think its likely we may have other docs, perhaps we should group them under some directory?
There was a problem hiding this comment.
I also find it odd tbh.
Just synced with @AndyTWF on this, and the idea is to leave it here for now, but we'll probably move it later as we'll reorganise the docs a little bit.
|
|
||
| When the user initiates the upload flow (for example by clicking a button called "attach"), we use the `uploadImage()` flow and save the resulting ID, like this: | ||
|
|
||
| ```javascript |
There was a problem hiding this comment.
This doesn't look right in the guide, I think you need to wrap this with tags
There was a problem hiding this comment.
<Code> tags needed here, yep.
|
|
||
| Ably Chat currently does not support media sharing out of the box, but it is very straightforward to link media and metadata to messages, and this can be used to implement media sharing features. | ||
|
|
||
| ## Architecture Overview |
There was a problem hiding this comment.
I think these headings need the new <a id="architecture-overview"/> wrapping?
|
|
||
| A common requirement for chat applications is the ability to share media like images, videos, or files with other participants. | ||
|
|
||
| Ably Chat currently does not support media sharing out of the box, but it is very straightforward to link media and metadata to messages, and this can be used to implement media sharing features. |
There was a problem hiding this comment.
This starts out a bit negative. Do we really need to state that we don't have a dedicated API for it?
There was a problem hiding this comment.
Changed it to a more positive tone, not mentioning we don't support it
| meta_keywords: "image, file, media, sharing, Ably Chat, chat SDK, realtime messaging, dependability, cost optimisation" | ||
| --- | ||
|
|
||
| A common requirement for chat applications is the ability to share media like images, videos, or files with other participants. |
There was a problem hiding this comment.
| A common requirement for chat applications is the ability to share media like images, videos, or files with other participants. | |
| A common requirement for chat applications is the ability to share media such as images, videos, or files. |
'with others' is implicit in the sharing.
| @@ -0,0 +1,165 @@ | |||
| --- | |||
| title: "Sharing media with Ably Chat" | |||
There was a problem hiding this comment.
I think this should be "Media sharing" to match with the nav and the format of other pages?
|
|
||
| Ably Chat currently does not support media sharing out of the box, but it is very straightforward to link media and metadata to messages, and this can be used to implement media sharing features. | ||
|
|
||
| ## Architecture Overview |
There was a problem hiding this comment.
Sentence case for headings please.
|
|
||
| The general approach for implementing media sharing in Ably Chat is: | ||
|
|
||
| 1. Upload media to your own storage service (your own servers, or a third-party service like AWS S3) |
There was a problem hiding this comment.
| 1. Upload media to your own storage service (your own servers, or a third-party service like AWS S3) | |
| 1. Upload the media to your own storage service. This can be your own servers, or a third-party service like AWS S3. |
| } | ||
| ``` | ||
|
|
||
| In the UI, the `imagesToAttach` array is displayed so the users know what images will be attached to their message. The UI can allow users to remove or sort the images. |
There was a problem hiding this comment.
| In the UI, the `imagesToAttach` array is displayed so the users know what images will be attached to their message. The UI can allow users to remove or sort the images. | |
| In your UI, the `imagesToAttach` array should be displayed so that users know which images will be attached to their message. It can allow users to remove or sort the images. |
There was a problem hiding this comment.
applied your version, thanks!
| ### Metadata for each image | ||
|
|
||
| In the example above we used a simple string array to store image IDs. We can change this to be an object where each image holds extra information that can be displayed to the user, such as a title or description, or the expected size in pixels so that the UI can allocate a placeholder loading box of the correct proportions. |
There was a problem hiding this comment.
Should this be titled differently and provide an example?
There was a problem hiding this comment.
I've removed this section and instead made it the default.
|
|
||
| ## Privacy and security considerations | ||
|
|
||
| The message metadata is not validated by the server. Always treat it as untrusted user input. The same applies to the text and headers of a chat message. |
There was a problem hiding this comment.
This could be a note when we first mention using the metadata field.
|
|
||
| ### Validate metadata on the receiving end | ||
|
|
||
| You must always validate the metadata before displaying any media to the user. Treat the metadata as untrusted user input. If you are using IDs, validate they are of the correct format. If you are using URLs, validate the schema, domain, path and query parameters are what you expect. Do not display images or other media directly from the metadata of a message. |
There was a problem hiding this comment.
This also seems like something to mention within the "Display images/attachments" section.
|
|
||
| You must always validate the metadata before displaying any media to the user. Treat the metadata as untrusted user input. If you are using IDs, validate they are of the correct format. If you are using URLs, validate the schema, domain, path and query parameters are what you expect. Do not display images or other media directly from the metadata of a message. | ||
|
|
||
| ### Use authentication server-side to control access |
There was a problem hiding this comment.
Should this be higher up the page? Seems important to understand the access controls before setting it up.
There was a problem hiding this comment.
I think it's fine to leave it towards the end since we don't really talk at all about the server-side stuff. I can't find a way to fit it in nicely with the rest of the article.
vladvelici
force-pushed
the
chat-image-sharing-doc-CHA-965-CHA-966
branch
from
fb214ba to
e755f90
Compare
|
Thanks @m-hulbert for the feedback, very useful, I've changed it quite a bit now. |
There was a problem hiding this comment.
Nice article, I find it a bit complex, but I think it can be valuable. Compare this complexity with how you send a file in Sendbird:
const {
requestId,
url,
} = await channel.uploadFile({
file,
uploadStartedHandler: (requestId: string) => {
// upload got started
},
progressHandler: (requestId: string, progress: number, total: number) => {
// progress in percentage = progress / total
},
});
channel.sendFileMessage({
fileUrl: url,
...
})
...
.onSucceeded((message: FileMessage) => {
// message is sent successfully
...
})
In my opinion, sending files is a basic feature for 1-1 and group chat we should offer in our SDK, because it adds a lot of value.
Description
Add an article showing how to add image (or other files) sharing in Ably Chat. It's a high-level overview, I don't think we should go into more details.
The URL is
/docs/guides/chat/media-sharing. We previously discussed not to put it as a guide so I'll move it where we think it should be and add a link in the side menu.Checklist