Skip to content

Fix media attachment actions #4849

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
klopez4212 wants to merge 10 commits into
base: main
Choose a base branch
from
+1,167 −133
Open

Fix media attachment actions #4849

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4f789f4
Fix media attachment actions
klopez4212 Aug 5, 2026
d90da28
Block send while an immediate attachment upload is in flight
Aug 5, 2026
ed8e7fd
Discard immediate uploads that outlive their originating draft
Aug 5, 2026
1e4e250
Guard stale upload previews and queue videos with no MIME type
Aug 5, 2026
51ced80
Retire in-flight uploads when the composer swaps drafts
Aug 5, 2026
6b7d5d9
Snapshot retired upload ids before clearing the active set
Aug 5, 2026
6103b24
Preserve reserved upload slots across updater appends
Aug 5, 2026
3d868b6
Keep composer media actions reachable without a mouse
Aug 5, 2026
0bcc614
Name the composer remove buttons for screen readers
Aug 5, 2026
b95a5e9
Pin edit-mode behaviour for in-flight draft uploads
Aug 5, 2026
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
143 changes: 66 additions & 77 deletions desktop/src/features/messages/lib/useMediaUpload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ async function captureVideoPosterFrame(
}

type UseMediaUploadOptions = {
/** Keep newly selected files local until the message is submitted. */
/** Keep newly selected videos local until the message is submitted. */
deferUploadsUntilSend?: boolean;
};

Expand All @@ -151,6 +151,10 @@ export function useMediaUpload({
const queueUntilSend =
deferUploadsUntilSend &&
(!e2eConfig || e2eConfig.mock?.deferredComposerUploads === true);
const shouldQueueFile = React.useCallback(
(file: File) => queueUntilSend && file.type.startsWith("video/"),
Comment thread
klopez4212 marked this conversation as resolved.
Outdated
[queueUntilSend],
);
const [uploadState, setUploadState] = React.useState<UploadState>({
status: "idle",
});
Expand Down Expand Up @@ -481,14 +485,55 @@ export function useMediaUpload({
[finishUpload, isUploadCanceled],
);

const uploadFiles = React.useCallback(
(files: File[]) => {
if (files.length === 0) return;

setUploadingCount((count) => count + files.length);
const baseIndex = reserveSlots(files.length);

for (let index = 0; index < files.length; index++) {
const file = files[index];
const slotIndex = baseIndex + index;
const previewId = reserveUploadingPreview(file, slotIndex);
// Fire-and-forget each upload concurrently — slot preserves order.
void (async () => {
try {
const buffer = await file.arrayBuffer();
if (isUploadCanceled(previewId)) return;
const descriptor = await uploadMediaBytes(

@wesbillman wesbillman Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Keep immediate File uploads off JSON IPC

This PR changes the normal deferred composer so photos and generic files selected by paperclip/drop/paste take uploadFiles, which reads the entire File, expands it into a JS number[], and then JSON-serializes it through uploadMediaBytes. Before this change those files stayed queued and the send path used uploadMediaFile, whose raw IPC body exists specifically to avoid JSON expansion (desktop/src/shared/api/tauriMedia.ts:15-34). For a large allowed PDF/photo, the ArrayBuffer + expanded JS array + JSON payload can multiply renderer memory and stall or fail the composer before Rust-side size/upload handling runs. Please use uploadMediaFile(file, progressId) for these immediate browser File paths (while preserving the epoch/cancel behavior), rather than materializing a number[].

[...new Uint8Array(buffer)],
Comment on lines +605 to +608

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid JSON-expanding immediate uploads

When deferred composer uploads are enabled in the normal MessageComposer, the paperclip path now routes non-video selections through this helper, so a selected large PDF/photo is read into the renderer and then spread into a JSON number[] for uploadMediaBytes. That multiplies the memory/IPC payload and can freeze or fail the desktop renderer before Rust-side upload handling can help; the background upload path already uses the raw uploadMediaFile IPC helper to avoid this expansion. Please route these immediate File uploads through the raw helper, or keep them queued until send, instead of JSON-serializing the bytes.

Useful? React with 👍 / 👎.

file.name,
uploadProgressId(previewId),
);
fillSlot(slotIndex, descriptor, previewId);
} catch (err) {
onUploadError(err, previewId);
}
})();
}
},
[
fillSlot,
isUploadCanceled,
onUploadError,
reserveSlots,
reserveUploadingPreview,
],
);

const handlePaperclip = React.useCallback(async () => {
if (queueUntilSend) {
const input = document.createElement("input");
input.type = "file";
input.multiple = true;
input.addEventListener(
"change",
() => queueFiles(Array.from(input.files ?? [])),
() => {
const files = Array.from(input.files ?? []);
queueFiles(files.filter(shouldQueueFile));
uploadFiles(files.filter((file) => !shouldQueueFile(file)));
Comment thread
klopez4212 marked this conversation as resolved.
Comment thread
klopez4212 marked this conversation as resolved.
Comment thread
klopez4212 marked this conversation as resolved.
},
{ once: true },
);
input.click();
Expand Down Expand Up @@ -520,6 +565,8 @@ export function useMediaUpload({
onUploadError,
queueFiles,
reserveUploadingPreview,
shouldQueueFile,
uploadFiles,
]);

const handleDrop = React.useCallback(
Expand All @@ -534,44 +581,10 @@ export function useMediaUpload({
// (active-content + executables) and size caps; everything else uploads.
const validFiles = files;

if (queueUntilSend) {
queueFiles(validFiles);
return;
}

setUploadingCount((c) => c + validFiles.length);
const baseIndex = reserveSlots(validFiles.length);

for (let i = 0; i < validFiles.length; i++) {
const file = validFiles[i];
const slotIndex = baseIndex + i;
const previewId = reserveUploadingPreview(file, slotIndex);
// Fire-and-forget each upload concurrently — slot preserves order
(async () => {
try {
const buffer = await file.arrayBuffer();
if (isUploadCanceled(previewId)) return;
const descriptor = await uploadMediaBytes(
[...new Uint8Array(buffer)],
file.name,
uploadProgressId(previewId),
);
fillSlot(slotIndex, descriptor, previewId);
} catch (err) {
onUploadError(err, previewId);
}
})();
}
queueFiles(validFiles.filter(shouldQueueFile));
uploadFiles(validFiles.filter((file) => !shouldQueueFile(file)));
},
[
reserveSlots,
queueUntilSend,
fillSlot,
isUploadCanceled,
onUploadError,
queueFiles,
reserveUploadingPreview,
],
[queueFiles, shouldQueueFile, uploadFiles],
);

const handleDragEnter = React.useCallback(
Expand Down Expand Up @@ -639,49 +652,16 @@ export function useMediaUpload({

event.preventDefault();

if (queueUntilSend) {
queueFiles(mediaFiles);
return;
}

setUploadingCount((c) => c + mediaFiles.length);
const baseIndex = reserveSlots(mediaFiles.length);

for (let i = 0; i < mediaFiles.length; i++) {
const file = mediaFiles[i];
const slotIndex = baseIndex + i;
const previewId = reserveUploadingPreview(file, slotIndex);
(async () => {
try {
const buffer = await file.arrayBuffer();
if (isUploadCanceled(previewId)) return;
const descriptor = await uploadMediaBytes(
[...new Uint8Array(buffer)],
file.name,
uploadProgressId(previewId),
);
fillSlot(slotIndex, descriptor, previewId);
} catch (err) {
onUploadError(err, previewId);
}
})();
}
queueFiles(mediaFiles.filter(shouldQueueFile));
uploadFiles(mediaFiles.filter((file) => !shouldQueueFile(file)));
},
[
reserveSlots,
queueUntilSend,
fillSlot,
isUploadCanceled,
onUploadError,
queueFiles,
reserveUploadingPreview,
],
[queueFiles, shouldQueueFile, uploadFiles],
);

/** Upload a File directly — used by Tiptap's editorProps.handlePaste. */
const uploadFile = React.useCallback(
async (file: File) => {
if (queueUntilSend) {
if (shouldQueueFile(file)) {
queueFiles([file]);
return;
}
Expand All @@ -701,12 +681,12 @@ export function useMediaUpload({
}
},
[
queueUntilSend,
isUploadCanceled,
onUploaded,
onUploadError,
queueFiles,
reserveUploadingPreview,
shouldQueueFile,
],
);

Expand Down Expand Up @@ -801,6 +781,15 @@ export function useMediaUpload({
[],
);

/**
* True while any attachment upload is in flight.
*
* Send paths must gate on this: with `deferUploadsUntilSend`, only videos
* are queued locally, so an in-flight photo/file is in neither
* `pendingImeta` nor `queuedAttachments`. Sending mid-flight would publish
* the message without that attachment and land the descriptor in an
* already-cleared composer.
*/
const isUploading = uploadingCount > 0;
const queuedPreviews = React.useMemo<UploadingAttachmentPreview[]>(
() =>
Expand Down
78 changes: 57 additions & 21 deletions desktop/src/features/messages/ui/ComposerAttachments.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
Bot,
FileText,
HatGlasses,
LineSquiggle,
Pencil,
Play,
UploadCloud,
Expand Down Expand Up @@ -36,6 +37,9 @@ import { Toggle } from "@/shared/ui/toggle";
import { Tooltip, TooltipContent, TooltipTrigger } from "@/shared/ui/tooltip";
import { ComposerImageEditor } from "./ComposerImageEditor";

const COMPOSER_MEDIA_HOVER_ACTION_CLASS =
"absolute inset-0 z-[1] hidden items-center justify-center rounded-2xl bg-black/35 text-white backdrop-blur-[1px] hover:bg-black/45 group-hover:flex";

/** Dashed-border overlay shown when a file is dragged over the composer form. */
export function DropZoneOverlay({ className }: { className?: string }) {
return (
Expand Down Expand Up @@ -63,7 +67,7 @@ type ComposerAttachmentsProps = {
onCancelUpload?: (previewId: number) => void;
/** Remove a local attachment that has not started uploading yet. */
onRemoveQueued?: (previewId: number) => void;
/** Toggle spoiler state for a local attachment before it receives a URL. */
/** Toggle spoiler state for a queued video before it receives a URL. */
onToggleQueuedSpoiler?: (previewId: number) => void;
/** Local previews that are queued for upload when the message is sent. */
queuedPreviews?: UploadingAttachmentPreview[];
Expand Down Expand Up @@ -293,9 +297,13 @@ const MediaAttachmentItem = React.forwardRef<
const handleRevert = React.useCallback(() => {
onRevert?.(attachment.url);
}, [attachment.url, onRevert]);
const handleOpenLightbox = React.useCallback(() => {
setOpen(true);
}, []);

return (
<motion.div
data-testid="composer-media-attachment"
ref={ref}
layout
initial={false}
Expand Down Expand Up @@ -441,12 +449,6 @@ const MediaAttachmentItem = React.forwardRef<
className={cn(
LIGHTBOX_BUTTON_CLASS,
"h-auto min-w-0",
// Active state driven by component state, not
// Radix's data-state: the TooltipTrigger clobbers
// the Toggle's data-state attribute. Swap the
// circular pill for the shared button radius with
// a visible ring so a spoilered attachment reads
// as "selected" on the dark lightbox backdrop.
isSpoilered &&
"rounded-lg bg-white/25 text-white ring-2 ring-white",
)}
Expand Down Expand Up @@ -494,13 +496,48 @@ const MediaAttachmentItem = React.forwardRef<
<button
type="button"
onClick={() => onRemove(attachment.url)}
className="absolute -right-1 -top-1 hidden h-4 w-4 items-center justify-center rounded-full bg-foreground text-background group-hover:flex"
className="absolute -right-1 -top-1 z-10 hidden h-4 w-4 items-center justify-center rounded-full bg-foreground text-background group-hover:flex"
>
<X className="h-2.5 w-2.5" />
</button>
</TooltipTrigger>
<TooltipContent>Remove attachment</TooltipContent>
</Tooltip>
{canEdit ? (
<Tooltip disableHoverableContent>
<TooltipTrigger asChild>
<button
className={COMPOSER_MEDIA_HOVER_ACTION_CLASS}
data-testid="composer-attachment-annotate"
onClick={handleOpenLightbox}
type="button"
>
<LineSquiggle className="h-5 w-5" />
<span className="sr-only">Draw on image</span>
</button>
</TooltipTrigger>
<TooltipContent>Draw on image</TooltipContent>
</Tooltip>
) : null}
{isVideo && onToggleSpoiler ? (
<Tooltip disableHoverableContent>
<TooltipTrigger asChild>
<button
aria-label={isSpoilered ? "Remove spoiler" : "Mark as spoiler"}
aria-pressed={isSpoilered}
className={COMPOSER_MEDIA_HOVER_ACTION_CLASS}
data-testid="composer-video-spoiler"
onClick={() => onToggleSpoiler(attachment.url)}
Comment thread
klopez4212 marked this conversation as resolved.

@wesbillman wesbillman Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Preserve a mouse path into the video lightbox

Carl reviewing on Wes’s behalf. Mongo independently traced this after my changes-requested review, and I agree the earlier automated finding should not have been dismissed as intentional. This full-tile inset-0 spoiler button becomes pointer-events-auto on hover and sits above the dialog trigger, so an uploaded video thumbnail cannot be opened/played with a mouse: every click toggles spoiler instead. This is reachable when editing a message with an existing video; the lightbox already has its own spoiler control, but this overlay blocks the mouse path into that lightbox. Please preserve a click-through region, use a smaller spoiler affordance, or remove the redundant inline toggle, and add coverage that hovers then opens an uploaded video preview.

type="button"
>
<HatGlasses className="h-5 w-5" />
</button>
</TooltipTrigger>
<TooltipContent>
{isSpoilered ? "Remove spoiler" : "Mark as spoiler"}
</TooltipContent>
</Tooltip>
) : null}
</div>
</motion.div>
);
Expand Down Expand Up @@ -620,13 +657,13 @@ export const ComposerAttachments = React.memo(function ComposerAttachments({
);
})}
{queuedPreviews.map((preview) => {
const isMedia =
preview.type?.startsWith("image/") ||
preview.type?.startsWith("video/");
const isVideo = preview.type?.startsWith("video/") ?? false;
const isMedia = preview.type?.startsWith("image/") || isVideo;
return (
<motion.div
animate={{ opacity: 1, scale: 1 }}
className="group relative"
data-testid="composer-queued-media-attachment"
exit={{ opacity: 0, scale: 0.8 }}
initial={{ opacity: 0, scale: 0.8 }}
key={`queued-attachment-${preview.id}`}
Expand Down Expand Up @@ -670,7 +707,7 @@ export const ComposerAttachments = React.memo(function ComposerAttachments({
<TooltipTrigger asChild>
<button
aria-label="Remove attachment"
className="absolute -right-1 -top-1 z-10 flex h-4 w-4 items-center justify-center rounded-full bg-foreground text-background"
className="absolute -right-1 -top-1 z-10 hidden h-4 w-4 items-center justify-center rounded-full bg-foreground text-background group-hover:flex"
Comment thread
klopez4212 marked this conversation as resolved.
Outdated
onClick={() => onRemoveQueued(preview.id)}
type="button"
>
Expand All @@ -680,24 +717,23 @@ export const ComposerAttachments = React.memo(function ComposerAttachments({
<TooltipContent>Remove attachment</TooltipContent>
</Tooltip>
) : null}
{isMedia && onToggleQueuedSpoiler ? (
{isVideo && onToggleQueuedSpoiler ? (
<Tooltip disableHoverableContent>
<TooltipTrigger asChild>
<Toggle
<button
aria-label={
preview.spoilered
? "Remove spoiler"
: "Mark as spoiler"
}
className="absolute -bottom-1 -left-1 z-10 h-4 w-4 rounded-full bg-foreground text-background hover:bg-foreground"
onPressedChange={() =>
onToggleQueuedSpoiler(preview.id)
}
pressed={preview.spoilered}
aria-pressed={preview.spoilered}
className={COMPOSER_MEDIA_HOVER_ACTION_CLASS}
data-testid="composer-queued-video-spoiler"
onClick={() => onToggleQueuedSpoiler(preview.id)}
type="button"
>
<HatGlasses className="h-2.5 w-2.5" />
</Toggle>
<HatGlasses className="h-5 w-5" />
</button>
</TooltipTrigger>
<TooltipContent>
{preview.spoilered ? "Remove spoiler" : "Mark as spoiler"}
Expand Down
Loading
Loading