Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 29 additions & 14 deletions src/script/components/Image/Image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ interface BaseImageProps extends React.HTMLProps<HTMLDivElement> {
interface RemoteDataImageProps extends BaseImageProps {
image: AssetRemoteData;
imageSizes?: {width: string; height: string; ratio: number};
fileType?: string;
}
interface AssetImageProps extends BaseImageProps {
image: MediumImage;
Expand All @@ -56,12 +57,13 @@ interface AssetImageProps extends BaseImageProps {
export const AssetImage = ({image, alt, ...props}: AssetImageProps) => {
const {resource} = useKoSubscribableChildren(image, ['resource']);

return <Image image={resource} imageSizes={image} alt={alt} {...props} />;
return <Image image={resource} imageSizes={image} fileType={image.file_type} alt={alt} {...props} />;
};

export const Image = ({
image,
imageSizes,
fileType,
onClick,
className,
isQuote = false,
Expand All @@ -88,7 +90,7 @@ export const Image = ({
'application/octet-stream', // Octet-stream is required to paste images from clipboard
...Config.getConfig().ALLOWED_IMAGE_TYPES,
];
const url = await getAssetUrl(image, allowedImageTypes);
const url = await getAssetUrl(image, allowedImageTypes, fileType);
if (isUnmouted.current) {
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The variable name contains a typo - 'isUnmouted' should be 'isUnmounted'. While this is a pre-existing typo, it reduces code readability and should be corrected.

Copilot uses AI. Check for mistakes.
// Avoid re-rendering a component that is umounted
return;
Expand All @@ -115,27 +117,40 @@ export const Image = ({
const dummyImageUrl = `data:image/svg+xml;utf8,<svg aria-hidden="true" xmlns='http://www.w3.org/2000/svg' viewBox='0 0 1 1' width='${imageSizes?.width}' height='${imageSizes?.height}'></svg>`;
const assetUrl = imageUrl?.url || dummyImageUrl;
const isLoading = !imageUrl;
const ImageElement = ({src}: {src: string}) => (
<img
css={{...getImageStyle(imageSizes), ...imageStyles}}
src={src}
role="presentation"
alt={alt}
data-uie-name={isLoading ? 'image-loader' : 'image-asset-img'}
onClick={event => {
if (!isLoading) {
onClick?.(event);
}
}}
/>
);

return (
<InViewport
onVisible={() => setIsInViewport(true)}
css={getWrapperStyles(!!onClick)}
className={cx(className, {'loading-dots image-asset--no-image': isLoading})}
onClick={event => {
if (!isLoading) {
onClick?.(event);
}
}}
data-uie-status={isLoading ? 'loading' : 'loaded'}
{...props}
>
<img
css={{...getImageStyle(imageSizes), ...imageStyles}}
src={assetUrl}
role="presentation"
alt={alt}
data-uie-name={isLoading ? 'image-loader' : 'image-asset-img'}
/>
{imageUrl?.pauseFrameUrl ? (
<div className="image-asset--animated-wrapper">
<ImageElement src={imageUrl?.pauseFrameUrl} />
<details open className="image-asset--animated">
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The details element is set to open by default, which means GIFs will always start playing automatically. This contradicts the pause/play feature's purpose and may be jarring for users as noted in the PR description. Consider removing the open attribute so GIFs start paused, or make this configurable based on user preference.

Suggested change
<details open className="image-asset--animated">
<details className="image-asset--animated">

Copilot uses AI. Check for mistakes.
<summary role="button" aria-label="Toggle animation playback"></summary>
<ImageElement src={assetUrl} />
</details>
Comment on lines +146 to +149
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Using the HTML details/summary elements for a play/pause toggle is semantically incorrect. These elements are designed for expandable/collapsible content disclosure, not for media controls. Consider using a standard button element with proper ARIA attributes like aria-pressed to indicate the play/pause state, which would better convey the control's purpose to assistive technologies.

Copilot uses AI. Check for mistakes.
</div>
) : (
<ImageElement src={assetUrl} />
)}
</InViewport>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import {useEffect, useState} from 'react';

import {GifUtil} from 'gifwrap';
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The 'gifwrap' library is imported but is not listed as a dependency in package.json. This will cause runtime errors when the code tries to process GIF files. Add 'gifwrap' to the dependencies section of package.json.

Copilot uses AI. Check for mistakes.
import {container} from 'tsyringe';

import {AssetRemoteData} from 'Repositories/assets/AssetRemoteData';
Expand All @@ -30,6 +31,7 @@ import {useKoSubscribableChildren} from 'Util/ComponentUtil';

export type AssetUrl = {
url: string;
pauseFrameUrl?: string;
dispose: () => void;
};

Expand Down Expand Up @@ -59,7 +61,11 @@ export const useAssetTransfer = (message?: ContentMessage, assetRepository = con
isPendingUpload: transferState === AssetTransferState.UPLOAD_PENDING,
isUploaded: transferState === AssetTransferState.UPLOADED,
isUploading: transferState === AssetTransferState.UPLOADING,
getAssetUrl: async (resource: AssetRemoteData, acceptedMimeTypes?: string[]): Promise<AssetUrl> => {
getAssetUrl: async (
resource: AssetRemoteData,
acceptedMimeTypes?: string[],
fileType?: string,
): Promise<AssetUrl> => {
Comment on lines +64 to +68
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The fileType parameter is optional but there's no documentation explaining when it should be provided or how it relates to blob.type. The condition checks both blob.type === 'image/gif' OR fileType === 'image/gif', which suggests they might differ in some cases. Add a JSDoc comment explaining when and why fileType should be passed as a parameter.

Copilot uses AI. Check for mistakes.
const blob = await assetRepository.load(resource);
if (!blob) {
throw new Error(`Asset could not be loaded`);
Expand All @@ -68,9 +74,55 @@ export const useAssetTransfer = (message?: ContentMessage, assetRepository = con
throw new Error(`Mime type not accepted "${blob.type}"`);
}
const url = URL.createObjectURL(blob);

const pauseFrameUrl = await (async () => {
if (blob.type === 'image/gif' || fileType === 'image/gif') {
try {
const gifArrayBuffer = await blob.arrayBuffer();
const gifBuffer = Buffer.from(gifArrayBuffer);
const frame0 = await GifUtil.read(gifBuffer).then(gifFile => gifFile.frames[0]);
const canvas = document.createElement('canvas');
const ctx = canvas.getContext('2d');
if (!ctx) {
return undefined;
}
canvas.width = frame0.bitmap.width;
canvas.height = frame0.bitmap.height;

const imageData = ctx.createImageData(frame0.bitmap.width, frame0.bitmap.height);
imageData.data.set(frame0.bitmap.data);
ctx.putImageData(imageData, 0, 0);

Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Typo in comment: "umounted" should be "unmounted"

Copilot uses AI. Check for mistakes.
const pauseImageBlob: Blob | null = await new Promise(resolve => {
canvas.toBlob(
blob => {
resolve(blob);
},
'image/jpeg',
0.9,
);
});

if (pauseImageBlob) {
return URL.createObjectURL(pauseImageBlob);
}
return undefined;
} catch (_error) {
return undefined;
}
}
return undefined;
})();

return {
dispose: () => URL.revokeObjectURL(url),
dispose: () => {
URL.revokeObjectURL(url);
if (pauseFrameUrl) {
URL.revokeObjectURL(pauseFrameUrl);
}
},
url,
pauseFrameUrl,
};
},
transferState,
Expand Down
1 change: 1 addition & 0 deletions src/script/repositories/entity/message/FileAsset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
public readonly downloadProgress: ko.PureComputed<number | undefined>;
public readonly cancelDownload: () => void;
public file_size: number;
public readonly file_type: string;

Check failure on line 43 in src/script/repositories/entity/message/FileAsset.ts

View workflow job for this annotation

GitHub Actions / test

Property 'file_type' will overwrite the base property in 'Asset'. If this is intentional, add an initializer. Otherwise, add a 'declare' modifier or remove the redundant declaration.
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The field is declared as 'readonly' but initialized to an empty string in the constructor, which contradicts the readonly semantics. Either remove 'readonly' to allow modification, or ensure file_type is set at construction time via a constructor parameter.

Suggested change
public readonly file_type: string;
public file_type: string;

Copilot uses AI. Check for mistakes.
public meta: Partial<AssetMetaData>;
public readonly status: ko.Observable<AssetTransferState>;
public readonly upload_failed_reason: ko.Observable<ProtobufAsset.NotUploaded>;
Expand Down
55 changes: 55 additions & 0 deletions src/style/components/image.less
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,58 @@
line-height: 1.3em;
}
}

.image-asset--animated-wrapper {
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The class name 'image-asset--animated-wrapper' doesn't follow a consistent naming pattern with existing BEM-style classes in the codebase. Consider using a more descriptive name that indicates this is specifically for GIF controls, such as 'image-asset--gif-controls-wrapper' or following the existing pattern more closely.

Suggested change
.image-asset--animated-wrapper {
.image-asset--gif-controls-wrapper {

Copilot uses AI. Check for mistakes.
position: relative;
display: inline-block;

summary {
position: absolute;
z-index: 2;
top: 0.5rem;
right: 0.5rem;
width: 0;
height: 2em;
border-width: 1em 0 1em 2em;
border-style: solid;

border-color: transparent transparent transparent #202020;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The play/pause button uses a hardcoded color (#202020). This may not respect user theme preferences (light/dark mode) or brand color schemes. Consider using a CSS variable or theme token to ensure consistency across the application and proper contrast in different themes.

Suggested change
border-color: transparent transparent transparent #202020;
border-color: transparent transparent transparent var(--gif-play-button-color, #202020);

Copilot uses AI. Check for mistakes.
background: transparent;
cursor: pointer;
opacity: 0.1;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The opacity value of 0.1 makes the play/pause button nearly invisible by default. This could make it difficult for users to discover this functionality. Consider using a higher opacity value (e.g., 0.5-0.7) or adding a subtle background to ensure the button is more discoverable while still being unobtrusive.

Suggested change
opacity: 0.1;
opacity: 0.6;

Copilot uses AI. Check for mistakes.
transition: 100ms all ease;

&:focus,
&:focus-visible {
opacity: 1;
outline: 2px solid var(--accent-color, #ffffff);
outline-offset: 2px;
}
}

&:hover summary {
opacity: 1;
}

Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The play/pause button lacks visible focus styling. Users navigating by keyboard won't have a visual indication when the button is focused, making it difficult to know where they are on the page. Add focus styles to the summary element.

Suggested change
summary:focus,
summary:focus-visible {
outline: 2px solid #ffffff;
outline-offset: 2px;
}

Copilot uses AI. Check for mistakes.
[open] summary {
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The CSS selector [open] on line 79 is missing the element type qualifier. It should be details[open] to be more specific and match the pattern used elsewhere in the file (e.g., details summary on lines 85 and 89). This improves specificity and makes the selector's intent clearer.

Suggested change
[open] summary {
details[open] summary {

Copilot uses AI. Check for mistakes.
border-width: 0 0 0 2em;
border-style: double;
}

/* for blink/webkit */
details summary::-webkit-details-marker {
display: none;
}
/* for firefox */
details > summary:first-of-type {
list-style: none;
}

.image-asset--animated img {
position: absolute;
top: 0;
left: 0;
display: inline-block;
Comment on lines +93 to +97
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The image uses absolute positioning which removes it from normal document flow. Both images (pause frame and animated) are absolutely positioned at the same location, which could cause layout issues or z-index conflicts. Ensure the positioning strategy doesn't break the layout in edge cases.

Suggested change
.image-asset--animated img {
position: absolute;
top: 0;
left: 0;
display: inline-block;
.image-asset--animated {
position: relative;
display: inline-block;
}
.image-asset--animated img {
display: block;
width: 100%;
height: auto;

Copilot uses AI. Check for mistakes.
overflow: visible;
}
}
Loading