upload: Fix uploading the same file twice in the same session.

This commit fixes a bug where uploading the same file a second time
in the same browser session would appear to the user to stall with
`Uploading [filename]...` in the composebox.  This is because
`tus-js-client` makes a HEAD request to check for already-uploaded
files -- and, if found, that request is used in the `upload-success`
callback.  That left the callback with no response body to parse, to
know what URL to insert.

Store the `/user_uploads/...` URL in the file metadata after a
successful upload, and if the fingerprint matches a previous upload,
pull that URL (and filename, as it may have changed server-side) out
of the previous upload's metadata.

Co-authored-by: Shubham Padia <shubham@zulip.com>
(cherry picked from commit c2e0a27d2c)

package.json

@@ -18,6 +18,7 @@
     "@uppy/drag-drop": "^4.0.2",
     "@uppy/progress-bar": "^4.0.0",
     "@uppy/tus": "^4.1.5",
+    "@uppy/utils": "^6.1.2",
     "@zxcvbn-ts/core": "^3.0.1",
     "@zxcvbn-ts/language-common": "^3.0.2",
     "@zxcvbn-ts/language-en": "^3.0.1",

pnpm-lock.yaml

@@ -76,6 +76,9 @@ importers:
       '@uppy/tus':
         specifier: ^4.1.5
         version: 4.2.2(@uppy/core@4.4.3)
+      '@uppy/utils':
+        specifier: ^6.1.2
+        version: 6.1.2
       '@zxcvbn-ts/core':
         specifier: ^3.0.1
         version: 3.0.4

version.py

@@ -49,4 +49,4 @@ API_FEATURE_LEVEL = 372  # Last bumped to interpret "(no topic)" as empty string
 #   historical commits sharing the same major version, in which case a
 #   minor version bump suffices.
 
-PROVISION_VERSION = (323, 0)  # bumped 2025-05-04 to upgrade Python requirements
+PROVISION_VERSION = (323, 1)  # bumped 2025-06-25 to add @uppy/utils

web/src/upload.ts

@@ -1,6 +1,7 @@
 import type {Meta} from "@uppy/core";
 import {Uppy} from "@uppy/core";
 import Tus, {type TusBody} from "@uppy/tus";
+import {getSafeFileId} from "@uppy/utils/lib/generateFileID";
 import $ from "jquery";
 import assert from "minimalistic-assert";
 import {z} from "zod";
@@ -19,9 +20,13 @@ import * as message_lists from "./message_lists.ts";
 import * as rows from "./rows.ts";
 import {realm} from "./state_data.ts";
 
+type ZulipMeta = {
+    zulip_url: string;
+} & Meta;
+
 let drag_drop_img: HTMLElement | null = null;
-let compose_upload_object: Uppy<Meta, TusBody>;
+let compose_upload_object: Uppy<ZulipMeta, TusBody>;
-const upload_objects_by_message_edit_row = new Map<number, Uppy<Meta, TusBody>>();
+const upload_objects_by_message_edit_row = new Map<number, Uppy<ZulipMeta, TusBody>>();
 
 export function compose_upload_cancel(): void {
     compose_upload_object.cancelAll();
@@ -126,7 +131,7 @@ export function edit_config(row: number): Config {
 }
 
 export let hide_upload_banner = (
-    uppy: Uppy<Meta, TusBody>,
+    uppy: Uppy<ZulipMeta, TusBody>,
     config: Config,
     file_id: string,
     delay = 0,
@@ -193,7 +198,7 @@ export function show_error_message(
 }
 
 export let upload_files = (
-    uppy: Uppy<Meta, TusBody>,
+    uppy: Uppy<ZulipMeta, TusBody>,
     config: Config,
     files: File[] | FileList,
 ): void => {
@@ -335,8 +340,8 @@ const zulip_upload_response_schema = z.object({
     filename: z.string(),
 });
 
-export function setup_upload(config: Config): Uppy<Meta, TusBody> {
+export function setup_upload(config: Config): Uppy<ZulipMeta, TusBody> {
-    const uppy = new Uppy<Meta, TusBody>({
+    const uppy = new Uppy<ZulipMeta, TusBody>({
         debug: false,
         autoProceed: true,
         restrictions: {
@@ -355,7 +360,20 @@ export function setup_upload(config: Config): Uppy<Meta, TusBody> {
             },
             pluralize: (_n) => 0,
         },
-        onBeforeFileAdded: () => true, // Allow duplicate file uploads
+        onBeforeFileAdded(file, files) {
+            const file_id = getSafeFileId(file, uppy.getID());
+
+            if (files[file_id]) {
+                // We have a duplicate file upload on our hands.
+                // Since we don't get a response with a body back from
+                // the server, pull the values that we got the last
+                // time around.
+                file.meta.zulip_url = files[file_id].meta.zulip_url!;
+                file.name = files[file_id].name!;
+            }
+
+            return file;
+        }, // Allow duplicate file uploads
     });
     uppy.use(Tus, {
         // https://uppy.io/docs/tus/#options
@@ -470,25 +488,48 @@ export function setup_upload(config: Config): Uppy<Meta, TusBody> {
             });
             return;
         }
-        let upload_response;
-        try {
-            upload_response = zulip_upload_response_schema.parse(
-                JSON.parse(response.body!.xhr.responseText),
-            );
-        } catch {
-            blueslip.warn("Invalid JSON response from tus server", {
-                body: response.body!.xhr.responseText,
-            });
-            return;
-        }
-        const filename = upload_response.filename;
-        const url = upload_response.url;
 
-        const filtered_filename = filename.replaceAll("[", "").replaceAll("]", "");
+        // We do not receive response text if the file has already
-        const syntax_to_insert = "[" + filtered_filename + "](" + url + ")";
+        // been uploaded. For an existing upload, TUS js client sends
+        // a HEAD request to the TUS server to check `Upload-Offset`
+        // and if some part of the upload is left to be done -- and
+        // when the upload offset is the same as the file length, it
+        // will not send any further requests, meaning we will not
+        // have a response body.  See the beforeUpload hook, above.
+        if (response.body!.xhr.responseText === "") {
+            if (!file.meta.zulip_url) {
+                blueslip.warn("No zulip_url retrieved from previous upload", {file});
+                return;
+            }
+        } else {
+            try {
+                const upload_response = zulip_upload_response_schema.parse(
+                    JSON.parse(response.body!.xhr.responseText),
+                );
+                uppy.setFileState(file.id, {
+                    name: upload_response.filename,
+                });
+                uppy.setFileMeta(file.id, {
+                    zulip_url: upload_response.url,
+                });
+                file = uppy.getFile(file.id);
+            } catch {
+                blueslip.warn("Invalid JSON response from the tus server", {
+                    body: response.body!.xhr.responseText,
+                });
+                return;
+            }
+        }
+
+        const filtered_filename = file.name!.replaceAll("[", "").replaceAll("]", "");
+        const syntax_to_insert = "[" + filtered_filename + "](" + file.meta.zulip_url + ")";
         const $text_area = config.textarea();
         const replacement_successful = compose_ui.replace_syntax(
-            get_translated_status(file.name!),
+            // We need to replace the original file name, and not the
+            // possibly modified filename returned in the response by
+            // the server. file.meta.name remains unchanged by us
+            // unlike file.name
+            get_translated_status(file.meta.name),
             syntax_to_insert,
             $text_area,
         );

web/tests/upload.test.cjs

@@ -454,6 +454,14 @@ test("uppy_events", ({override_rewire, mock_template}) => {
 
     const callbacks = {};
     let state = {};
+    const file = {
+        name: "copenhagen.png",
+        meta: {
+            name: "copenhagen.png",
+        },
+    };
+    let uppy_set_file_state_called = false;
+    let uppy_set_file_meta_called = false;
 
     uppy_stub = function () {
         return {
@@ -466,6 +474,21 @@ test("uppy_events", ({override_rewire, mock_template}) => {
             getFiles() {
                 return [];
             },
+            // This is currently only called in
+            // on_upload_success_callback, we return the modified name
+            // keeping in mind only that case. Although this isn't
+            // ideal, it seems better than the alternative of creating
+            // a file store in the tests.
+            getFile() {
+                return {
+                    ...file,
+                    name: "modified-name-copenhagen.png",
+                    meta: {
+                        ...file.meta,
+                        zulip_url: "/user_uploads/4/cb/rue1c-MlMUjDAUdkRrEM4BTJ/copenhagen.png",
+                    },
+                };
+            },
             getState: () => ({
                 info: [
                     {
@@ -475,21 +498,30 @@ test("uppy_events", ({override_rewire, mock_template}) => {
                     },
                 ],
             }),
+            setFileState(_file_id, {name}) {
+                uppy_set_file_state_called = true;
+                assert.equal(name, "modified-name-copenhagen.png");
+            },
+            setFileMeta(_file_id, {zulip_url}) {
+                uppy_set_file_meta_called = true;
+                assert.equal(
+                    zulip_url,
+                    "/user_uploads/4/cb/rue1c-MlMUjDAUdkRrEM4BTJ/copenhagen.png",
+                );
+            },
         };
     };
     upload.setup_upload(upload.compose_config);
     assert.equal(Object.keys(callbacks).length, 6);
 
     const on_upload_success_callback = callbacks["upload-success"];
-    const file = {
-        name: "copenhagen.png",
-    };
     let response = {
+        status: 200,
         body: {
             xhr: {
                 responseText: JSON.stringify({
                     url: "/user_uploads/4/cb/rue1c-MlMUjDAUdkRrEM4BTJ/copenhagen.png",
-                    filename: "copenhagen.png",
+                    filename: "modified-name-copenhagen.png",
                 }),
             },
         },
@@ -501,7 +533,7 @@ test("uppy_events", ({override_rewire, mock_template}) => {
         assert.equal(old_syntax, "[translated: Uploading copenhagen.png…]()");
         assert.equal(
             new_syntax,
-            "[copenhagen.png](/user_uploads/4/cb/rue1c-MlMUjDAUdkRrEM4BTJ/copenhagen.png)",
+            "[modified-name-copenhagen.png](/user_uploads/4/cb/rue1c-MlMUjDAUdkRrEM4BTJ/copenhagen.png)",
         );
         assert.equal($textarea, $("textarea#compose-textarea"));
     });
@@ -513,6 +545,8 @@ test("uppy_events", ({override_rewire, mock_template}) => {
 
     assert.ok(compose_ui_replace_syntax_called);
     assert.ok(compose_ui_autosize_textarea_called);
+    assert.ok(uppy_set_file_state_called);
+    assert.ok(uppy_set_file_meta_called);
 
     mock_template("compose_banner/upload_banner.hbs", false, (data) => {
         assert.equal(data.banner_type, "error");