fix(attachments): take a nested picker's drop once and drop late reads

A drop on a FileInput inside the drop zone reached both the picker and
the zone, so the file was attached and then refused as already there,
or attached twice by a host that does not de-duplicate. The zone now
leaves a drop a child already handled to that child.

A folder walk could also outlive the zone: it still delivered after
the zone was disabled, and after unmount it added files whose preview
URLs nothing would revoke. A read that finishes once the zone is
disabled or unmounted is now dropped, and useAttachments makes no
preview URL once it has unmounted.

A nameless file was checked for duplicates under its empty name but
held as "Untitled", so two of them could slip in and a real Untitled
was refused. The check now uses the name the file is held under.

Author: pratikbuilds

e2e/attachments.spec.ts

@@ -262,6 +262,28 @@ test("a dropped folder is walked to the files inside it", async ({ page }) => {
   }
 });
 
+test("a drop on the picker inside the zone is taken once", async ({ page }) => {
+  await page.goto("/attachments");
+  await page.locator("[data-testid=zone] label").evaluate((picker) => {
+    const data = new DataTransfer();
+    data.items.add(new File(["hi"], "notes.txt", { type: "text/plain" }));
+    for (const type of ["dragenter", "dragover", "drop"]) {
+      picker.dispatchEvent(
+        new DragEvent(type, {
+          dataTransfer: data,
+          bubbles: true,
+          cancelable: true,
+        }),
+      );
+    }
+  });
+  await expect(count(page)).toHaveText("1");
+  await expect(page.getByRole("status", { name: "Refused" })).toHaveText("");
+  await expect(
+    page.locator("[data-slot=attachment-drop-zone-overlay]"),
+  ).toHaveCount(0);
+});
+
 test("files dropped outside the zone do not open in place of the page", async ({
   page,
 }) => {

e2e/fixtures.tsx

@@ -68,6 +68,7 @@ import {
   Attachment,
   AttachmentDropZone,
   AttachmentInfo,
+  FileInput,
   AttachmentMore,
   AttachmentPreview,
   AttachmentRemove,
@@ -1368,6 +1369,11 @@ function AttachmentsScenario() {
   });
   const folded = !expanded && attachments.length > 5;
   const shown = folded ? attachments.slice(0, 4) : attachments;
+  const take = (files: Iterable<File> | ArrayLike<File>) => {
+    const { accepted, rejected } = add(files);
+    setAttachments((previous) => [...previous, ...accepted]);
+    setRefused(refusals(rejected));
+  };
   return (
     <div style={{ display: "flex", flexDirection: "column", gap: 24 }}>
       <div data-testid="outside" style={{ height: 80 }}>
@@ -1376,9 +1382,7 @@ function AttachmentsScenario() {
       <AttachmentDropZone
         data-testid="zone"
         onFiles={(files, details) => {
-          const { accepted, rejected } = add(files);
-          setAttachments((previous) => [...previous, ...accepted]);
-          setRefused(refusals(rejected));
+          take(files);
           setFolders(details.folders.join(","));
         }}
         style={{ padding: "96px 16px 16px", width: 640 }}
@@ -1421,6 +1425,7 @@ function AttachmentsScenario() {
           </Button>
         ) : null}
         <textarea ref={message} aria-label="Message" />
+        <FileInput multiple onFiles={take} />
       </AttachmentDropZone>
       <output aria-label="Count">{attachments.length}</output>
       <output aria-label="Refused">{refused}</output>

src/hooks/use-attachments.test.ts

@@ -178,6 +178,24 @@ describe("useAttachments", () => {
     host.unmount();
   });
 
+  test("a nameless file is checked as the Untitled it is held as", () => {
+    const host = mountHost();
+    const [item] = host.drop([file("")]).accepted;
+    if (item === undefined) throw new Error("missing");
+    expect(item.name).toBe("Untitled");
+
+    const nameless = file("");
+    const named = file("Untitled");
+    expect(host.drop([nameless]).rejected).toEqual([
+      { file: nameless, reason: "duplicate" },
+    ]);
+    expect(host.drop([named]).rejected).toEqual([
+      { file: named, reason: "duplicate" },
+    ]);
+    expect(host.list).toEqual([item]);
+    host.unmount();
+  });
+
   test("two adds before the list updates still respect the limits", () => {
     const host = mountHost({ maxFiles: 2 });
     let second: ReturnType<UseAttachmentsResult["add"]> | undefined;
@@ -242,6 +260,17 @@ describe("useAttachments", () => {
   // StrictMode replays effects only at mount, before any URL is made, so
   // the replay here comes after: the effects are torn down and set up again
   // while the previews are on screen.
+  // A drop still being read, or any other late caller.
+  test("an add after unmounting makes no preview URL", async () => {
+    const host = mountHost();
+    host.unmount();
+    await microtasks();
+    const [item] = host.add([file("a.txt")]).accepted;
+    expect(item?.url).toBeUndefined();
+    expect(item?.file).toBeInstanceOf(File);
+    expect(created).toEqual([]);
+  });
+
   test("an effect replay while previews are held revokes nothing", async () => {
     const host = mountHost({ strict: true });
     host.drop([file("a.txt"), file("b.txt")]);

src/hooks/use-attachments.ts

@@ -45,6 +45,11 @@ export type UseAttachmentsResult = {
 
 let lastAttachmentId = 0;
 
+/** The name a file is held under. */
+function nameOf(file: File): string {
+  return file.name === "" ? "Untitled" : file.name;
+}
+
 function nextAttachmentId(): string {
   lastAttachmentId += 1;
   return `attachment-${lastAttachmentId}`;
@@ -62,7 +67,7 @@ function nextAttachmentId(): string {
  * Every URL the hook makes is revoked at the first commit whose list does
  * not hold its attachment: once the host removes it, however it does, or
  * right away if the host never appended it. The rest go when the hook
- * unmounts. Only
+ * unmounts, and an `add` after that makes no URL. Only
  * URLs the hook made are revoked; the host's own are left alone. To keep an
  * attachment on screen after it leaves the list (sending moves it into a
  * message), `release` it first.
@@ -86,7 +91,9 @@ export function useAttachments({
   const pending = useRef<AttachmentItem[]>([]);
   const latest = useRef({ attachments, maxFiles, maxFileBytes });
   latest.current = { attachments, maxFiles, maxFileBytes };
-  const mounted = useRef(false);
+  // True until the hook unmounts, so an `add` before its effects run still
+  // makes a URL.
+  const mounted = useRef(true);
 
   // After every commit, not only when the list changes: an accepted file the
   // host did not append must neither keep its URL nor count as a duplicate
@@ -124,26 +131,40 @@ export function useAttachments({
       ...(max === undefined ? {} : { maxFiles: max }),
       ...(maxBytes === undefined ? {} : { maxFileBytes: maxBytes }),
     };
+    // Checked under the name each file will be held under, so a nameless
+    // file is a duplicate of the "Untitled" one already held.
     const { accepted, rejected } = checkAttachments(
       [...held, ...pending.current],
-      Array.from(files),
+      Array.from(files, (file) => ({
+        name: nameOf(file),
+        size: file.size,
+        file,
+      })),
       limits,
     );
-    const items = accepted.map((file): AttachmentItem => {
+    const items = accepted.map(({ name, file }): AttachmentItem => {
       const id = nextAttachmentId();
-      const url = URL.createObjectURL(file);
-      urls.current.set(id, url);
-      return {
+      const item = {
         id,
-        name: file.name === "" ? "Untitled" : file.name,
+        name,
         mediaType: file.type === "" ? "application/octet-stream" : file.type,
         size: file.size,
-        url,
         file,
       };
+      // Past unmount nothing would revoke it.
+      if (!mounted.current) return item;
+      const url = URL.createObjectURL(file);
+      urls.current.set(id, url);
+      return { ...item, url };
     });
     pending.current = [...pending.current, ...items];
-    return { accepted: items, rejected };
+    return {
+      accepted: items,
+      rejected: rejected.map(({ file, reason }) => ({
+        file: file.file,
+        reason,
+      })),
+    };
   }, []);
 
   const release = useCallback((ids: Iterable<string>) => {

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

@@ -1,5 +1,6 @@
 import { describe, expect, test } from "bun:test";
 import { act, createElement, useState } from "react";
+import type { DragEvent as ReactDragEvent } from "react";
 import { createRoot } from "react-dom/client";
 
 import {
@@ -59,6 +60,27 @@ function folderEntry(
   };
 }
 
+/** A folder whose listing waits until `release` is called. */
+function heldFolder(name: string, entries: readonly DroppedEntry[]) {
+  let release = () => {};
+  const listed = new Promise<void>((resolve) => {
+    release = resolve;
+  });
+  const folder = folderEntry(name, entries);
+  const entry: DroppedEntry = {
+    ...folder,
+    createReader: () => {
+      const reader = folder.createReader?.();
+      return {
+        readEntries: (success, failure) => {
+          void listed.then(() => reader?.readEntries(success, failure));
+        },
+      };
+    },
+  };
+  return { entry, release };
+}
+
 function source(
   dropped: readonly (DroppedEntry | File)[],
   types: readonly string[] = ["Files"],
@@ -195,7 +217,8 @@ type Received = { files: readonly File[]; details: FileDropDetails };
 function mountTarget({
   guard = true,
   disabled: initiallyDisabled = false,
-}: { guard?: boolean; disabled?: boolean } = {}) {
+  childTakesDrops = false,
+}: { guard?: boolean; disabled?: boolean; childTakesDrops?: boolean } = {}) {
   const container = document.createElement("div");
   document.body.appendChild(container);
   const root = createRoot(container);
@@ -215,7 +238,13 @@ function mountTarget({
     return createElement(
       "div",
       { "data-testid": "zone", ...drop.dropProps },
-      createElement("span", null, "Drop here"),
+      createElement(
+        "span",
+        childTakesDrops
+          ? { onDrop: (event: ReactDragEvent) => event.preventDefault() }
+          : null,
+        "Drop here",
+      ),
     );
   }
 
@@ -385,6 +414,41 @@ describe("useFileDrop", () => {
     target.unmount();
   });
 
+  // A picker such as FileInput inside the zone reads its own drops.
+  test("a drop a child took itself is left to it", async () => {
+    const target = mountTarget({ childTakesDrops: true });
+    const files = source([new File(["a"], "a.txt")]);
+    target.fire(target.child, dragEvent("dragenter", files));
+    expect(target.over).toBe(true);
+    const drop = target.fire(target.child, dragEvent("drop", files));
+    expect(drop.defaultPrevented).toBe(true);
+    expect(target.over).toBe(false);
+    await settle();
+    expect(target.received).toEqual([]);
+    target.unmount();
+  });
+
+  test("a drop still being read when the target is disabled is dropped", async () => {
+    const target = mountTarget();
+    const folder = heldFolder("photos", [fileEntry("a.jpg")]);
+    target.fire(target.zone, dragEvent("drop", source([folder.entry])));
+    target.disable();
+    folder.release();
+    await settle();
+    expect(target.received).toEqual([]);
+    target.unmount();
+  });
+
+  test("a drop still being read when the target unmounts is dropped", async () => {
+    const target = mountTarget();
+    const folder = heldFolder("photos", [fileEntry("a.jpg")]);
+    target.fire(target.zone, dragEvent("drop", source([folder.entry])));
+    target.unmount();
+    folder.release();
+    await settle();
+    expect(target.received).toEqual([]);
+  });
+
   test("a drag without files is left alone", async () => {
     const target = mountTarget();
     const text = source([], ["text/plain"]);

src/hooks/use-file-drop.ts

@@ -1,4 +1,4 @@
-import { useEffect, useRef, useState } from "react";
+import { useEffect, useLayoutEffect, useRef, useState } from "react";
 import type { DragEvent as ReactDragEvent } from "react";
 
 /**
@@ -230,7 +230,10 @@ export type UseFileDropOptions = ReadDroppedFilesOptions & {
    * what it held. A drop of only empty folders calls it with no files.
    */
   readonly onFiles: (files: readonly File[], details: FileDropDetails) => void;
-  /** While true, drags are ignored and the browser's default stands. */
+  /**
+   * While true, drags are ignored and the browser's default stands. A drop
+   * still being read when it turns true is dropped, errors and all.
+   */
   readonly disabled?: boolean;
   /**
    * Called when a drop could not be read in full: a folder that could not
@@ -262,7 +265,10 @@ function report(error: unknown) {
 
 /**
  * Makes an element a target for dropped files and folders. Only drags that
- * carry files are taken; text and links dragged over it behave as usual.
+ * carry files are taken; text and links dragged over it behave as usual. A
+ * drop that a child handles itself (calls `preventDefault`), as `FileInput`
+ * does, is left to the child. A drop still being read when the target
+ * unmounts is dropped.
  *
  * `over` is true while files are held over the target, including over its
  * children. It turns false when the drag leaves the target, is dropped, or
@@ -278,8 +284,18 @@ export function useFileDrop<T extends Element = HTMLElement>({
   onError,
 }: UseFileDropOptions): UseFileDropResult<T> {
   const [over, setOver] = useState(false);
-  const latest = useRef({ onFiles, onError, maxFolderEntries });
-  latest.current = { onFiles, onError, maxFolderEntries };
+  // The latest options, for a drop still being read under an earlier render.
+  const latest = useRef({ onFiles, onError, maxFolderEntries, disabled });
+  useLayoutEffect(() => {
+    latest.current = { onFiles, onError, maxFolderEntries, disabled };
+  });
+  const mounted = useRef(false);
+  useEffect(() => {
+    mounted.current = true;
+    return () => {
+      mounted.current = false;
+    };
+  }, []);
   // Counted rather than read from `relatedTarget`, which WebKit leaves null:
   // moving between two children enters one before leaving the other, so the
   // count only reaches zero once the drag has left the target.
@@ -343,22 +359,29 @@ export function useFileDrop<T extends Element = HTMLElement>({
       },
       onDrop: (event) => {
         if (!takes(event)) return;
-        event.preventDefault();
         depth.current = 0;
         setOver(false);
+        // A child that takes its own drops, such as `FileInput`, took it.
+        if (event.isDefaultPrevented()) return;
+        event.preventDefault();
         const options = latest.current.maxFolderEntries;
+        // Disabled or unmounted while the drop was being read.
+        const stale = () => latest.current.disabled || !mounted.current;
         readDroppedFiles(
           event.dataTransfer,
           options === undefined ? {} : { maxFolderEntries: options },
         ).then(
           ({ files, ...details }) => {
+            if (stale()) return;
             const { onFiles: deliver, onError: fail } = latest.current;
             for (const error of details.errors) (fail ?? report)(error);
             if (files.length > 0 || details.folders.length > 0) {
               deliver(files, details);
             }
           },
-          (error: unknown) => (latest.current.onError ?? report)(error),
+          (error: unknown) => {
+            if (!stale()) (latest.current.onError ?? report)(error);
+          },
         );
       },
     },

src/ui/attachment-drop-zone.tsx

@@ -30,8 +30,11 @@ 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. The zone is a plain `div`: give it
- * `role="region"` and an `aria-label` when it should be a landmark.
+ * inside it for keyboard and touch. A child that takes its own drops keeps
+ * them: a drop right on `FileInput` reaches only the picker's `onFiles`,
+ * which reads loose files and does not walk folders. The zone is a plain
+ * `div`: give it `role="region"` and an `aria-label` when it should be a
+ * landmark.
  */
 export function AttachmentDropZone({
   onFiles,