Skip to content

fix: Improve GIF handling and display consistency in chat messages #984

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
+122 −16
Open

fix: Improve GIF handling and display consistency in chat messages #984

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
29751df
fix: Improve GIF URL handling and add debugging logs
KrishnaShuk Feb 12, 2025
f537b90
refactor: Move styles to separate file and improve accessibility in I…
KrishnaShuk Mar 2, 2025
8a7781c
chore: Remove console logs and fix formatting
KrishnaShuk Mar 2, 2025
70ac232
Merge branch 'develop' into fix/improve-gif-url-handling
Spiral-Memory Mar 14, 2025
92a9496
refactor: simplify URL handling for file uploads in Attachment component
KrishnaShuk Mar 15, 2025
eecd589
Merge branch 'fix/improve-gif-url-handling' of https://github.com/Kri…
KrishnaShuk Mar 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions packages/react/src/views/AttachmentHandler/Attachment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,50 @@ const Attachment = ({ attachment, host, type, variantStyles = {}, msg }) => {
/>
);
}
if (attachment && attachment.image_url) {
if (attachment && (attachment.image_url || attachment.title_link)) {
const url = attachment.image_url || attachment.title_link;

const isGif =
url &&
(url.toLowerCase().endsWith('.gif') ||
(attachment.image_type &&
attachment.image_type.toLowerCase() === 'image/gif') ||
(attachment.description &&
attachment.description.toLowerCase().includes('gif')));

const imageUrl = url.startsWith('http') ? url : host + url;

if (url && url.includes('/file-upload/')) {
const fullUrl = url.startsWith('http')
? url
: `${host}/file-upload/${url.split('/file-upload/')[1]}`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the reasoning behind checking whether http is present, then extracting the full URL and everything else
Can't we do it without these manual interventions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here was to handle two possible cases: absolute URLs (like https://example.com/file-upload/123) that are good to go as-is, and relative ones (like /file-upload/123) that need the host tacked on to work. I check for http to figure out which case I’m dealing with, so I don’t accidentally double up the host on an absolute URL or leave a relative one incomplete. The splitting part just grabs the file ID after /file-upload/ to rebuild the URL properly.
we could just assume everything’s relative and put host on front, though that’d break if an absolute URL slips in.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there will be a difference ? I mean once the image is uploaded, it will have any one of these URLs right ? Have you observed anything that in what cases, there could be a chance of absolute / relative URLs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No i haven't checked the cases. I have just guessed it. And you are right that once the image is uploaded it will have any one of these URLs. Should i make a commit in which url is put out directly without any condition?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, check which kind of url is present after the upload and based on it, just use it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The url is of relative type. Sould i totally remove the conditions logic or use this instead -> const fullUrl = ${host}/file-upload/${url.split('/file-upload/')[1]};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, pls go ahead with the new change and once you're done, request a re-review

Thanks a lot for the contribution 😌😊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @Spiral-Memory! These conversations surely help me in leveling up.


return (
<ImageAttachment
attachment={{
...attachment,
image_url: fullUrl,
}}
host={host}
author={author}
variantStyles={variantStyles}
msg={msg}
isGif={isGif}
/>
);
}

return (
<ImageAttachment
attachment={attachment}
attachment={{
...attachment,
image_url: imageUrl,
}}
host={host}
author={author}
variantStyles={variantStyles}
msg={msg}
isGif={isGif}
/>
);
}
Expand Down
79 changes: 65 additions & 14 deletions packages/react/src/views/AttachmentHandler/ImageAttachment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Box, Avatar, useTheme } from '@embeddedchat/ui-elements';
import AttachmentMetadata from './AttachmentMetadata';
import ImageGallery from '../ImageGallery/ImageGallery';
import RCContext from '../../context/RCInstance';
import { imageWrapper, imageStyles } from './ImageAttachment.styles';

const ImageAttachment = ({
attachment,
Expand All @@ -13,9 +14,12 @@ const ImageAttachment = ({
author,
variantStyles = {},
msg,
isGif = false,
}) => {
const { RCInstance } = useContext(RCContext);
const [showGallery, setShowGallery] = useState(false);
const [imageError, setImageError] = useState(false);

const getUserAvatarUrl = (icon) => {
const instanceHost = RCInstance.getHost();
const URL = `${instanceHost}${icon}`;
Expand All @@ -35,6 +39,20 @@ const ImageAttachment = ({
setIsExpanded((prevState) => !prevState);
};

const handleImageClick = (e) => {
if (isGif) {
e.stopPropagation();
return;
}
setShowGallery(true);
};

const handleKeyPress = (e) => {
if (e.key === 'Enter' || e.key === ' ') {
handleImageClick(e);
}
};

return (
<Box css={variantStyles.imageAttachmentContainer}>
<Box
Expand Down Expand Up @@ -73,9 +91,7 @@ const ImageAttachment = ({
<Box>@{authorName}</Box>
</Box>
</>
) : (
''
)}
) : null}
<Box
css={css`
align-items: center;
Expand All @@ -94,23 +110,57 @@ const ImageAttachment = ({
/>
</Box>
{isExpanded && (
<Box onClick={() => setShowGallery(true)}>
<button
type="button"
onClick={handleImageClick}
onKeyPress={handleKeyPress}
css={[
imageWrapper({ isGif, theme }),
css`
border: none;
background: none;
padding: 0;
cursor: ${!isGif ? 'pointer' : 'default'};
&:focus {
outline: 2px solid ${theme.colors.primary};
outline-offset: 2px;
}
`,
]}
>
<img
src={host + attachment.image_url}
style={{
maxWidth: '100%',
objectFit: 'contain',
borderBottomLeftRadius: 'inherit',
borderBottomRightRadius: 'inherit',
src={attachment.image_url}
style={imageStyles({ isGif })}
onError={() => {
setImageError(true);
}}
alt={attachment.description || 'Attached image'}
/>
</Box>
{imageError && (
<Box
css={css`
color: ${theme.colors.danger};
padding: 8px;
font-size: 12px;
`}
>
Failed to load image. URL: {host + attachment.image_url}
</Box>
)}
</button>
)}
{attachment.attachments &&
attachment.attachments.map((nestedAttachment, index) => (
<Box css={variantStyles.imageAttachmentContainer} key={index}>
<Box
role="button"
tabIndex={0}
onClick={() => setShowGallery(true)}
onKeyPress={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
setShowGallery(true);
}
}}
css={[
css`
cursor: pointer;
Expand Down Expand Up @@ -148,9 +198,7 @@ const ImageAttachment = ({
<Box>@{nestedAttachment.author_name}</Box>
</Box>
</>
) : (
''
)}
) : null}
<AttachmentMetadata
attachment={nestedAttachment}
url={
Expand All @@ -167,6 +215,9 @@ const ImageAttachment = ({
borderBottomLeftRadius: 'inherit',
borderBottomRightRadius: 'inherit',
}}
alt={
nestedAttachment.description || 'Nested attachment image'
}
/>
</Box>
{showGallery && (
Expand Down
21 changes: 21 additions & 0 deletions packages/react/src/views/AttachmentHandler/ImageAttachment.styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { css } from '@emotion/react';

export const imageWrapper = ({ isGif, theme }) => css`
width: ${isGif ? '200px' : '300px'};
height: ${isGif ? '200px' : 'auto'};
overflow: hidden;
border-radius: inherit;
display: flex;
align-items: center;
justify-content: center;
background-color: ${theme.colors.surface};
`;

export const imageStyles = ({ isGif }) => ({
width: isGif ? '200px' : '100%',
height: isGif ? '200px' : 'auto',
maxHeight: isGif ? '200px' : '200px',
objectFit: isGif ? 'cover' : 'scale-down',
borderRadius: 'inherit',
imageRendering: isGif ? 'auto' : 'inherit',
});