Fix media attachment actions

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]);
+});

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;
+}

desktop/src/features/messages/lib/useMediaUpload.test.mjs

@@ -142,3 +142,221 @@ 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);
+});