diff --git a/desktop/src/features/messages/lib/imetaSlots.test.mjs b/desktop/src/features/messages/lib/imetaSlots.test.mjs new file mode 100644 index 0000000000..55307f08c4 --- /dev/null +++ b/desktop/src/features/messages/lib/imetaSlots.test.mjs @@ -0,0 +1,100 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { applyImetaUpdate, compactImetaSlots } from "./imetaSlots.ts"; + +// An immediate upload reserves a `null` placeholder and later fills it *by +// index*. Updates written against the compacted list must therefore be mapped +// back onto the slot layout: replacing the array renumbers it under an +// in-flight upload, whose fillSlot would then overwrite an unrelated +// attachment. + +const SNAPSHOT = { url: "snapshot.png", sha256: "5555" }; +const append = (descriptor) => (current) => [...current, descriptor]; + +test("compaction hides in-flight placeholders", () => { + const only = { url: "only.png", sha256: "1111" }; + assert.deepEqual(compactImetaSlots([null, only, null]), [only]); + assert.deepEqual(compactImetaSlots([]), []); +}); + +test("a snapshot paste during an in-flight upload does not take its slot", () => { + // Repro: attach a photo (slot 0 reserved, still uploading), then paste an + // agent snapshot. The snapshot must land after the placeholder so the + // photo's fillSlot(0, ...) cannot overwrite it. + const slots = applyImetaUpdate([null], append(SNAPSHOT)); + assert.deepEqual(slots, [null, SNAPSHOT]); + + // The upload completes and fills its own reserved index. + const photo = { url: "photo.png", sha256: "aaaa" }; + const filled = [...slots]; + filled[0] = photo; + assert.deepEqual(filled, [photo, SNAPSHOT]); +}); + +test("an append keeps already-filled attachments at their own indexes", () => { + const first = { url: "first.png", sha256: "1111" }; + assert.deepEqual(applyImetaUpdate([first, null], append(SNAPSHOT)), [ + first, + null, + SNAPSHOT, + ]); +}); + +test("an updater returning its input leaves the slots untouched", () => { + // handleSnapshotPaste returns `current` unchanged when the snapshot is + // already attached; that must not disturb a reserved placeholder. + const existing = [SNAPSHOT, null]; + const slots = applyImetaUpdate(existing, (current) => current); + assert.equal(slots, existing, "same array identity, no re-render churn"); +}); + +test("a removal nulls its slot instead of renumbering", () => { + // Removing an attachment must not shift the index a pending upload holds. + const keep = { url: "keep.png", sha256: "1111" }; + const drop = { url: "drop.png", sha256: "2222" }; + const slots = applyImetaUpdate([keep, drop, null], (current) => + current.filter((d) => d.url !== "drop.png"), + ); + assert.deepEqual(slots, [keep, null, null]); +}); + +test("clearing every attachment keeps the reserved placeholders", () => { + const one = { url: "one.png", sha256: "1111" }; + assert.deepEqual( + applyImetaUpdate([one, null], () => []), + [null, null], + ); +}); + +test("the updater only ever sees real attachments", () => { + const only = { url: "only.png", sha256: "1111" }; + let seen = null; + applyImetaUpdate([null, only, null], (current) => { + seen = current; + return current; + }); + assert.deepEqual(seen, [only]); +}); + +test("descriptors are matched on url and digest together", () => { + // Same url, different bytes: the new descriptor is an append, not a survivor. + const original = { url: "same.png", sha256: "1111" }; + const reuploaded = { url: "same.png", sha256: "2222" }; + assert.deepEqual(applyImetaUpdate([original, null], append(reuploaded)), [ + original, + null, + reuploaded, + ]); +}); + +test("a reorder does not move descriptors out of their slots", () => { + // Reordering cannot be honored while an upload holds an index; keeping the + // existing positions is what protects the pending fillSlot. + const a = { url: "a.png", sha256: "1111" }; + const b = { url: "b.png", sha256: "2222" }; + const slots = applyImetaUpdate([a, b, null], (current) => + [...current].reverse(), + ); + assert.deepEqual(slots, [a, b, null]); +}); diff --git a/desktop/src/features/messages/lib/imetaSlots.ts b/desktop/src/features/messages/lib/imetaSlots.ts new file mode 100644 index 0000000000..d8aa10e49f --- /dev/null +++ b/desktop/src/features/messages/lib/imetaSlots.ts @@ -0,0 +1,65 @@ +import type { BlobDescriptor } from "@/shared/api/tauri"; + +/** + * Slot bookkeeping for composer attachments. + * + * Attachments live in a sparse array: an immediate upload calls `reserveSlots` + * to claim an index up front and fills it by that index when it completes, so + * concurrent uploads publish in the order they were attached. A `null` is a + * placeholder for an upload still in flight. + * + * Consumers of the composer only ever see the compacted list of real + * attachments, so any update expressed against that view has to be mapped back + * onto the slot layout — never applied to it directly. + */ + +/** + * Identity of a descriptor. `url` alone can repeat across re-uploads of + * identical bytes, so pair it with the digest. + */ +function descriptorKey(descriptor: BlobDescriptor): string { + return `${descriptor.url}\u0000${descriptor.sha256 ?? ""}`; +} + +/** The real attachments, in order, with in-flight placeholders dropped. */ +export function compactImetaSlots( + slots: (BlobDescriptor | null)[], +): BlobDescriptor[] { + return slots.filter((d): d is BlobDescriptor => d !== null); +} + +/** + * Apply an updater written against the compacted list back onto `slots`. + * + * Replacing the array with the updater's result would renumber it while an + * in-flight upload still holds an index from `reserveSlots`, so that upload's + * `fillSlot` would overwrite an unrelated attachment. Instead: + * + * - survivors stay at the index they already occupy; + * - removals become `null` rather than shifting their neighbours; + * - genuinely new descriptors append after the reserved tail, where no pending + * `fillSlot` can reach them. + * + * An updater that returns its input unchanged (e.g. the snapshot-paste dedupe) + * leaves `slots` exactly as it was, identity included. + */ +export function applyImetaUpdate( + slots: (BlobDescriptor | null)[], + update: (current: BlobDescriptor[]) => BlobDescriptor[], +): (BlobDescriptor | null)[] { + const current = compactImetaSlots(slots); + const next = update(current); + if (next === current) return slots; + + const survivingKeys = new Set(next.map(descriptorKey)); + const preserved = slots.map((descriptor) => + descriptor === null || survivingKeys.has(descriptorKey(descriptor)) + ? descriptor + : null, + ); + const presentKeys = new Set(current.map(descriptorKey)); + const appended = next.filter( + (descriptor) => !presentKeys.has(descriptorKey(descriptor)), + ); + return appended.length > 0 ? [...preserved, ...appended] : preserved; +} diff --git a/desktop/src/features/messages/lib/useMediaUpload.test.mjs b/desktop/src/features/messages/lib/useMediaUpload.test.mjs index 674cce5ffd..f4ced7c600 100644 --- a/desktop/src/features/messages/lib/useMediaUpload.test.mjs +++ b/desktop/src/features/messages/lib/useMediaUpload.test.mjs @@ -142,3 +142,280 @@ test("reserveSlots pads if slots array is shorter than expected start index", () assert.equal(next[3], null); // reserved assert.equal(next[4], null); // reserved }); + +// ── Draft-boundary epoch guard (pure logic) ─────────────────────────── +// Photos/files upload immediately, so an upload can still be in flight when +// the composer swaps drafts (channel switch, post-send clear, edit restore). +// Every wholesale `setPendingImeta` replacement bumps an epoch; uploads pin +// the epoch at start and discard their descriptor if it no longer matches, so +// one draft's attachment can never land in — or overwrite a slot reserved by — +// another draft. Mirrors `isUploadStale` + `fillSlot`/`onUploaded`. + +function fillSlotIfCurrent(slots, index, descriptor, epoch, currentEpoch) { + if (epoch !== currentEpoch) return slots; + const next = [...slots]; + next[index] = descriptor; + return next; +} + +test("upload completing in the same draft fills its slot", () => { + const a = { url: "a.png", sha256: "aaaa" }; + const next = fillSlotIfCurrent([null], 0, a, 0, 0); + assert.deepEqual(next, [a]); +}); + +test("upload completing after a draft switch is discarded", () => { + // Draft A reserves slot 0 at epoch 0, user switches channels (epoch → 1), + // then the upload resolves. It must not write into draft B's slots. + const a = { url: "a.png", sha256: "aaaa" }; + const draftBSlots = [null]; + const next = fillSlotIfCurrent(draftBSlots, 0, a, 0, 1); + assert.deepEqual(next, [null]); + assert.equal(next, draftBSlots); +}); + +test("stale upload cannot overwrite a slot the new draft already filled", () => { + // Draft B has its own attachment in slot 0; draft A's late upload targets + // the same index and must leave B's descriptor intact. + const stale = { url: "stale.png", sha256: "aaaa" }; + const current = { url: "current.png", sha256: "bbbb" }; + const next = fillSlotIfCurrent([current], 0, stale, 0, 2); + assert.deepEqual(next, [current]); +}); + +test("appending to the current draft does not bump the epoch", () => { + // Only wholesale replacement (`setPendingImeta(array)`) is a draft boundary. + // The updater form appends within the current draft, so in-flight uploads + // for that same draft must still be considered current. + let epoch = 0; + const bumpIfReplacement = (action) => { + if (typeof action !== "function") epoch += 1; + }; + bumpIfReplacement((current) => [...current, { url: "pasted.png" }]); + assert.equal(epoch, 0); + bumpIfReplacement([]); + assert.equal(epoch, 1); +}); + +// ── Cancel guard for stale previews (pure logic) ─────────────────────── +// The epoch bump makes completions discard their descriptors, but the old +// preview row (and its cancel button) can still be on screen. Cancelling it +// must not null a slot in the draft now on screen, because the preview carries +// the *previous* draft's slotIndex. Mirrors `cancelUpload`'s `isStalePreview`. + +function cancelSlotIndex(preview, currentEpoch) { + if (preview?.slotIndex === undefined) return undefined; + const isStale = + preview.uploadEpoch !== undefined && preview.uploadEpoch !== currentEpoch; + return isStale ? undefined : preview.slotIndex; +} + +test("cancelling a preview from the current draft nulls its slot", () => { + assert.equal(cancelSlotIndex({ slotIndex: 1, uploadEpoch: 3 }, 3), 1); +}); + +test("cancelling a stale preview does not null the new draft's slot", () => { + // Draft A reserved slot 0 at epoch 0; draft B now owns slot 0. Cancelling + // A's leftover preview must leave B's attachment intact. + assert.equal(cancelSlotIndex({ slotIndex: 0, uploadEpoch: 0 }, 1), undefined); +}); + +test("cancelling a preview with no slot is a no-op for slots", () => { + // `handlePaperclip`'s native-picker preview has no reserved slot. + assert.equal(cancelSlotIndex({ uploadEpoch: 0 }, 0), undefined); +}); + +// ── Retiring in-flight uploads at a draft boundary (pure logic) ──────── +// Bumping the epoch alone discards descriptors but leaves the previous draft's +// preview rows on screen and its uploads counted, which keeps `isUploading` +// true and holds the *new* draft's send gate closed. A wholesale replacement +// must therefore retire those uploads outright. Mirrors `beginNewDraftEpoch`. + +function beginNewDraftEpoch(state) { + const next = { + epoch: state.epoch + 1, + active: new Set(state.active), + canceled: new Set(state.canceled), + previews: state.previews, + uploadingCount: state.uploadingCount, + }; + if (next.active.size === 0) return next; + // Mirrors the real callback: snapshot, clear the live set, then schedule the + // updaters. `applyUpdates` below runs them afterwards, the way React does. + const retiredIds = new Set(next.active); + const retiredCount = retiredIds.size; + next.active.clear(); + for (const id of retiredIds) next.canceled.add(id); + next.pendingUpdates = [ + (s) => { + s.previews = s.previews.filter((preview) => !retiredIds.has(preview.id)); + }, + (s) => { + s.uploadingCount = Math.max(0, s.uploadingCount - retiredCount); + }, + ]; + return next; +} + +/** Run the scheduled state updaters, as React does after the event handler. */ +function applyUpdates(state) { + for (const update of state.pendingUpdates ?? []) update(state); + state.pendingUpdates = []; + return state; +} + +test("a draft boundary retires in-flight uploads so the new draft can send", () => { + // Draft A has one upload in flight; switching to draft B must leave B with + // no previews and nothing counted as uploading. + const after = applyUpdates( + beginNewDraftEpoch({ + epoch: 0, + active: new Set([1]), + canceled: new Set(), + previews: [{ id: 1, slotIndex: 0, uploadEpoch: 0 }], + uploadingCount: 1, + }), + ); + assert.equal(after.epoch, 1); + assert.deepEqual(after.previews, []); + assert.equal(after.uploadingCount, 0); + assert.equal(after.active.size, 0); + // Canceled so the late completion/error paths stay silent in the new draft. + assert.ok(after.canceled.has(1)); +}); + +test("retiring several concurrent uploads clears the count exactly once each", () => { + const after = applyUpdates( + beginNewDraftEpoch({ + epoch: 4, + active: new Set([7, 8, 9]), + canceled: new Set(), + previews: [{ id: 7 }, { id: 8 }, { id: 9 }], + uploadingCount: 3, + }), + ); + assert.equal(after.uploadingCount, 0); + assert.deepEqual(after.previews, []); +}); + +test("a draft boundary with no uploads in flight still advances the epoch", () => { + const after = applyUpdates( + beginNewDraftEpoch({ + epoch: 2, + active: new Set(), + canceled: new Set(), + previews: [], + uploadingCount: 0, + }), + ); + assert.equal(after.epoch, 3); + assert.equal(after.uploadingCount, 0); +}); + +test("the retired count never drives uploadingCount negative", () => { + // Defensive: a preview already settled by finishUpload must not be + // double-decremented into a negative count that would wedge the gate. + const after = applyUpdates( + beginNewDraftEpoch({ + epoch: 0, + active: new Set([1, 2]), + canceled: new Set(), + previews: [{ id: 1 }, { id: 2 }], + uploadingCount: 1, + }), + ); + assert.equal(after.uploadingCount, 0); +}); + +test("retirement holds even though the live active set is cleared first", () => { + // Regression: the updaters must not read the live `active` set, which is + // emptied before React runs them. Closing over it filtered against an empty + // set and subtracted 0, leaving the stale preview and a stuck send gate. + const state = beginNewDraftEpoch({ + epoch: 0, + active: new Set([1]), + canceled: new Set(), + previews: [{ id: 1 }], + uploadingCount: 1, + }); + assert.equal(state.active.size, 0, "live set is cleared before updates run"); + // Updates land only now — after the clear — exactly as React schedules them. + applyUpdates(state); + assert.deepEqual(state.previews, []); + assert.equal(state.uploadingCount, 0); +}); + +test("replayed updaters stay idempotent", () => { + // React may invoke an updater more than once (StrictMode double-render). + const state = beginNewDraftEpoch({ + epoch: 0, + active: new Set([1]), + canceled: new Set(), + previews: [{ id: 1 }], + uploadingCount: 1, + }); + const updates = state.pendingUpdates; + for (const update of updates) update(state); + for (const update of updates) update(state); + assert.deepEqual(state.previews, []); + assert.equal(state.uploadingCount, 0); +}); + +// ── Edit mode while an upload is in flight ──────────────────────────── +// Immediate photo/file uploads reserve null slots that are absent from the +// compacted `pendingImeta` snapshot. MessageComposer therefore rejects edit +// entry while an upload is active, leaving the current draft and upload epoch +// untouched. Once the upload settles, normal edit snapshot/restore proceeds. + +function attemptEditModeRoundTrip({ isUploading, draft = [] }) { + const uploaded = { sha256: "ffff", url: "in-flight.png" }; + const editTargetImeta = [{ sha256: "eeee", url: "edit-target.png" }]; + let slots = [...draft]; + let epoch = 0; + + if (isUploading) { + slots = [...slots, null]; + return { + editEntered: false, + epoch, + restoredDraft: slots, + }; + } + + const snapshot = [...slots]; + epoch += 1; + slots = editTargetImeta; + epoch += 1; + slots = snapshot; + + return { + editEntered: true, + epoch, + restoredDraft: slots, + uploaded, + }; +} + +test("edit entry is rejected without replacing a draft that is uploading", () => { + const existing = { sha256: "aaaa", url: "already-there.png" }; + const result = attemptEditModeRoundTrip({ + draft: [existing], + isUploading: true, + }); + + assert.equal(result.editEntered, false); + assert.equal(result.epoch, 0, "the current draft epoch must not be retired"); + assert.deepEqual(result.restoredDraft, [existing, null]); +}); + +test("edit entry proceeds normally after uploads settle", () => { + const existing = { sha256: "aaaa", url: "already-there.png" }; + const result = attemptEditModeRoundTrip({ + draft: [existing], + isUploading: false, + }); + + assert.equal(result.editEntered, true); + assert.equal(result.epoch, 2); + assert.deepEqual(result.restoredDraft, [existing]); +}); diff --git a/desktop/src/features/messages/lib/useMediaUpload.ts b/desktop/src/features/messages/lib/useMediaUpload.ts index b4c3cae44f..374d392818 100644 --- a/desktop/src/features/messages/lib/useMediaUpload.ts +++ b/desktop/src/features/messages/lib/useMediaUpload.ts @@ -5,7 +5,10 @@ import { pickAndUploadMedia, uploadMediaBytes, } from "@/shared/api/tauri"; +import { uploadMediaFile } from "@/shared/api/tauriMedia"; import type { QueuedMediaAttachment } from "./backgroundMediaUploadStore"; +import { applyImetaUpdate, compactImetaSlots } from "./imetaSlots"; +import { isVideoFile, videoMimeForFile } from "./videoFileType"; /** * First 4 hex chars of the sha256 — used as a short display name. @@ -33,6 +36,12 @@ export type UploadingAttachmentPreview = { slotIndex?: number; spoilered?: boolean; type?: string; + /** + * Upload epoch this preview was created in. Cancel handling compares it + * against the current epoch so a preview left over from a replaced draft + * cannot null a slot belonging to the draft now on screen. + */ + uploadEpoch?: number; }; /** Correlation id for the Rust `media-upload-progress` events. */ @@ -85,9 +94,16 @@ type CapturedVideoPoster = { async function captureVideoPosterFrame( file: File, ): Promise { - if (!file.type.startsWith("video/")) return null; - - const objectUrl = URL.createObjectURL(file); + const videoMime = videoMimeForFile(file); + if (!videoMime) return null; + + // A blob URL inherits the File's own MIME type, so a video whose type is + // empty or `application/octet-stream` would be rejected by the