fix(attachments): rebuild revoked previews and cover the disabled option

A hook hidden by Activity for longer than a tick cannot tell the hide
from an unmount, so useAttachments revokes its previews while the host
still holds the items, and the previews came back blank. Attachment
now rebuilds the picture from the item's file when its url fails,
owning that url and revoking it on unmount or hide. The hook's docs
say the held url itself stays dead, so code that reads it should make
its own from the file or keep the hook above the Activity.

No test passed disabled to useFileDrop, and the test named for it
covered the stray-drop guard instead; both are now tested.

AttachmentPreview takes a label for its open button, as
AttachmentRemove already does. The docs say AttachmentMore renders
last and how to name the drop zone as a region, carriesFiles is no
longer public, and tests exercise the uncontrolled fold.

Author: pratikbuilds

src/hooks/use-attachments.test.ts

@@ -133,6 +133,13 @@ function mountHost({ strict = false, initial = [], ...limits }: Options = {}) {
       act(() => setHidden(true));
       act(() => setHidden(false));
     },
+    /** Hides the hook in its `Activity` for longer than a tick, then shows it. */
+    async hideAndShow() {
+      act(() => setHidden(true));
+      await act(microtasks);
+      act(() => setHidden(false));
+      await act(microtasks);
+    },
     unmount() {
       act(() => root.unmount());
       container.remove();
@@ -249,6 +256,21 @@ describe("useAttachments", () => {
     expect([...revoked].sort()).toEqual([...created].sort());
   });
 
+  // Nothing tells a hide from an unmount, so the previews go and the held
+  // items keep their dead URLs; `Attachment` rebuilds the pictures from
+  // `file` (see attachments.test.tsx).
+  test("a hide longer than a tick revokes the held previews", async () => {
+    const host = mountHost();
+    const [item] = host.drop([file("a.png", 4, "image/png")]).accepted;
+    if (item === undefined) throw new Error("missing");
+
+    await host.hideAndShow();
+    expect(revoked).toEqual([item.url ?? ""]);
+    expect(host.list).toEqual([item]);
+    expect(host.list[0]?.file).toBeInstanceOf(File);
+    host.unmount();
+  });
+
   test("the host's own URLs are never revoked", async () => {
     const uploaded: AttachmentItem = {
       id: "uploaded-1",

src/hooks/use-attachments.ts

@@ -69,8 +69,13 @@ function nextAttachmentId(): string {
  *
  * The unmount revoke waits a microtask and is called off if the hook mounts
  * again, which is what StrictMode's simulated remount does within one
- * commit. A hook hidden by `<Activity mode="hidden">` counts as unmounted,
- * and its previews are revoked.
+ * commit. A hook hidden by `<Activity mode="hidden">` for longer than that
+ * counts as unmounted: its previews are revoked, and once it shows again
+ * the attachments still held keep URLs that no longer load. `Attachment`
+ * rebuilds their pictures from `file`. Anything else that reads `url`
+ * after such a hide, such as a viewer opened from `AttachmentPreview`'s
+ * `onOpen`, should make its own URL from `file`, or the hook should sit
+ * above the `Activity`.
  */
 export function useAttachments({
   attachments,

src/hooks/use-file-drop.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test } from "bun:test";
-import { act, createElement } from "react";
+import { act, createElement, useState } from "react";
 import { createRoot } from "react-dom/client";
 
 import {
@@ -192,16 +192,23 @@ function dropEffectOf(event: Event): unknown {
 
 type Received = { files: readonly File[]; details: FileDropDetails };
 
-function mountTarget({ guard = true }: { guard?: boolean } = {}) {
+function mountTarget({
+  guard = true,
+  disabled: initiallyDisabled = false,
+}: { guard?: boolean; disabled?: boolean } = {}) {
   const container = document.createElement("div");
   document.body.appendChild(container);
   const root = createRoot(container);
   const received: Received[] = [];
   let over = false;
+  let setDisabled: (disabled: boolean) => void = () => {};
 
   function Target() {
+    const [disabled, setTargetDisabled] = useState(initiallyDisabled);
+    setDisabled = setTargetDisabled;
     const drop = useFileDrop<HTMLDivElement>({
       onFiles: (files, details) => received.push({ files, details }),
+      disabled,
     });
     usePreventStrayDrops(guard);
     over = drop.over;
@@ -223,6 +230,9 @@ function mountTarget({ guard = true }: { guard?: boolean } = {}) {
     get over() {
       return over;
     },
+    disable(disabled = true) {
+      act(() => setDisabled(disabled));
+    },
     fire(target: EventTarget, event: Event) {
       act(() => {
         target.dispatchEvent(event);
@@ -243,6 +253,37 @@ function settle(): Promise<void> {
 }
 
 describe("useFileDrop", () => {
+  // The stray-drop guard is off, so only the hook can claim these events.
+  test("takes no drag while disabled", async () => {
+    const target = mountTarget({ guard: false, disabled: true });
+    const files = source([new File(["a"], "a.txt")]);
+    for (const type of ["dragenter", "dragover", "dragleave", "drop"]) {
+      const event = target.fire(target.zone, dragEvent(type, files));
+      expect(event.defaultPrevented).toBe(false);
+      expect(target.over).toBe(false);
+    }
+    await settle();
+    expect(target.received).toEqual([]);
+    target.unmount();
+  });
+
+  test("disabling mid-drag drops over, and enabling takes drags again", () => {
+    const target = mountTarget({ guard: false });
+    const files = source([new File(["a"], "a.txt")]);
+    target.fire(target.zone, dragEvent("dragenter", files));
+    expect(target.over).toBe(true);
+
+    target.disable();
+    expect(target.over).toBe(false);
+    target.disable(false);
+    expect(target.over).toBe(false);
+
+    const enter = target.fire(target.zone, dragEvent("dragenter", files));
+    expect(enter.defaultPrevented).toBe(true);
+    expect(target.over).toBe(true);
+    target.unmount();
+  });
+
   test("a dropped folder reaches onFiles walked to its files", async () => {
     const target = mountTarget();
     const drop = target.fire(target.zone, dragEvent("drop", source([PROJECT])));
@@ -405,7 +446,7 @@ describe("usePreventStrayDrops", () => {
     expect(after.defaultPrevented).toBe(false);
   });
 
-  test("does nothing when disabled", () => {
+  test("stray-drop guard off leaves body drops alone", () => {
     const target = mountTarget({ guard: false });
     const files = source([new File(["a"], "a.txt")]);
     expect(

src/hooks/use-file-drop.ts

@@ -54,9 +54,7 @@ export type ReadDroppedFilesOptions = {
 const DEFAULT_MAX_FOLDER_ENTRIES = 500;
 
 /** Whether a drag carries files, as opposed to text or a link. */
-export function carriesFiles(
-  source: Pick<DropSource, "types"> | null,
-): boolean {
+function carriesFiles(source: Pick<DropSource, "types"> | null): boolean {
   return source !== null && source.types.includes("Files");
 }
 

src/ui/attachment-drop-zone.tsx

@@ -30,7 +30,8 @@ export type AttachmentDropZoneProps = Omit<
  * open the file in place of the page.
  *
  * Dropping needs a pointer, so put a file picker (such as `FileInput`)
- * inside it for keyboard and touch.
+ * inside it for keyboard and touch. The zone is a plain `div`: give it
+ * `role="region"` and an `aria-label` when it should be a landmark.
  */
 export function AttachmentDropZone({
   onFiles,

src/ui/attachments.test.tsx

@@ -1,9 +1,14 @@
-import { act, useRef, useState } from "react";
+import { Activity, act, useRef, useState } from "react";
 import { createRoot } from "react-dom/client";
-import { describe, expect, test } from "bun:test";
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
 
+import {
+  useAttachments,
+  type AttachmentItem,
+} from "../hooks/use-attachments.js";
 import {
   Attachment,
+  AttachmentMore,
   AttachmentPreview,
   AttachmentRemove,
   Attachments,
@@ -114,4 +119,190 @@ describe("Attachments", () => {
     expect(opened).toEqual(["notes.txt"]);
     unmount();
   });
+
+  test("a preview that opens takes its name from label", () => {
+    const { container, unmount } = mount(
+      <Attachments>
+        <Attachment data={NOTES}>
+          <AttachmentPreview label="View the notes" onOpen={() => {}} />
+        </Attachment>
+      </Attachments>,
+    );
+    expect(button(container, "View the notes").dataset["slot"]).toBe(
+      "attachment-preview",
+    );
+    unmount();
+  });
+
+  test("an uncontrolled list folds until More expands it", () => {
+    const files: readonly AttachmentData[] = ["a.txt", "b.txt", "c.txt"].map(
+      (name) => ({ name, mediaType: "text/plain" }),
+    );
+    const { container, unmount } = mount(
+      <Attachments variant="list">
+        {({ expanded }) => {
+          const shown = expanded ? files : files.slice(0, 1);
+          return (
+            <>
+              {shown.map((data) => (
+                <Attachment key={data.name} data={data}>
+                  <AttachmentRemove />
+                </Attachment>
+              ))}
+              {expanded ? null : <AttachmentMore count={files.length - 1} />}
+            </>
+          );
+        }}
+      </Attachments>,
+    );
+    const names = () =>
+      [...container.querySelectorAll("[data-slot=attachment-remove]")].map(
+        (element) => element.getAttribute("aria-label"),
+      );
+    expect(names()).toEqual(["Remove a.txt"]);
+
+    const more = button(container, "Show 2 more");
+    more.focus();
+    act(() => more.click());
+    expect(names()).toEqual(["Remove a.txt", "Remove b.txt", "Remove c.txt"]);
+    expect(container.querySelector("ul")?.dataset["expanded"]).toBe("true");
+    // Focus goes to the first attachment that was folded away.
+    expect(document.activeElement).toBe(button(container, "Remove b.txt"));
+    unmount();
+  });
+
+  test("defaultExpanded starts an uncontrolled list expanded", () => {
+    const { container, unmount } = mount(
+      <Attachments variant="list" defaultExpanded>
+        {({ expanded }) => (expanded ? <li>all</li> : <li>some</li>)}
+      </Attachments>,
+    );
+    expect(container.textContent).toBe("all");
+    unmount();
+  });
+});
+
+describe("a preview held across a hidden Activity", () => {
+  let live = new Set<string>();
+  let made = 0;
+  const realCreate = URL.createObjectURL;
+  const realRevoke = URL.revokeObjectURL;
+
+  beforeEach(() => {
+    live = new Set();
+    made = 0;
+    URL.createObjectURL = () => {
+      made += 1;
+      const url = `blob:test/${made}`;
+      live.add(url);
+      return url;
+    };
+    URL.revokeObjectURL = (url: string) => {
+      live.delete(url);
+    };
+  });
+
+  afterEach(() => {
+    URL.createObjectURL = realCreate;
+    URL.revokeObjectURL = realRevoke;
+  });
+
+  function tick(): Promise<void> {
+    return act(() => new Promise<void>((resolve) => setTimeout(resolve, 0)));
+  }
+
+  test("is rebuilt from its file once its URL stops loading", async () => {
+    let attach: (files: readonly File[]) => void = () => {};
+    let setHidden: (hidden: boolean) => void = () => {};
+
+    function Composer() {
+      const [held, setHeld] = useState<readonly AttachmentItem[]>([]);
+      const { add } = useAttachments({ attachments: held });
+      attach = (files) => {
+        const { accepted } = add(files);
+        setHeld((previous) => [...previous, ...accepted]);
+      };
+      return (
+        <Attachments variant="tiles">
+          {held.map((item) => (
+            <Attachment key={item.id} data={item}>
+              <AttachmentPreview />
+            </Attachment>
+          ))}
+        </Attachments>
+      );
+    }
+
+    function Route() {
+      const [hidden, setRouteHidden] = useState(false);
+      setHidden = setRouteHidden;
+      return (
+        <Activity mode={hidden ? "hidden" : "visible"}>
+          <Composer />
+        </Activity>
+      );
+    }
+
+    const { container, unmount } = mount(<Route />);
+    act(() => attach([new File(["x"], "shot.png", { type: "image/png" })]));
+    const picture = () => container.querySelector("img");
+    const first = picture()?.getAttribute("src") ?? "";
+    expect(live.has(first)).toBe(true);
+
+    // Hidden for longer than a tick: the hook's previews go.
+    act(() => setHidden(true));
+    await tick();
+    act(() => setHidden(false));
+    await tick();
+    expect(live.has(first)).toBe(false);
+
+    // The browser fails to load the revoked URL.
+    act(() => {
+      picture()?.dispatchEvent(new Event("error"));
+    });
+    const rebuilt = picture()?.getAttribute("src") ?? "";
+    expect(rebuilt).not.toBe(first);
+    expect(live.has(rebuilt)).toBe(true);
+
+    // The rebuilt URL follows the same rule: gone while hidden, made again
+    // on show.
+    act(() => setHidden(true));
+    expect(live.has(rebuilt)).toBe(false);
+    act(() => setHidden(false));
+    const again = picture()?.getAttribute("src") ?? "";
+    expect(live.has(again)).toBe(true);
+
+    unmount();
+    await tick();
+    expect([...live]).toEqual([]);
+  });
+
+  test("falls back to the type when the rebuilt URL fails too", () => {
+    const shot = new File(["x"], "shot.png", { type: "image/png" });
+    const { container, unmount } = mount(
+      <Attachments variant="tiles">
+        <Attachment
+          data={{
+            name: "shot.png",
+            mediaType: "image/png",
+            url: "blob:test/gone",
+            file: shot,
+          }}
+        >
+          <AttachmentPreview />
+        </Attachment>
+      </Attachments>,
+    );
+    const fail = () =>
+      act(() => {
+        container.querySelector("img")?.dispatchEvent(new Event("error"));
+      });
+    fail();
+    expect(container.querySelector("img")).not.toBeNull();
+    fail();
+    expect(container.querySelector("img")).toBeNull();
+    expect(container.textContent).toContain("PNG");
+    unmount();
+    expect([...live]).toEqual([]);
+  });
 });