From c2e0a27d2c096aa8f70e95abd07811a2e8dfbec6 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Tue, 27 May 2025 20:39:28 +0000 Subject: [PATCH] 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 --- package.json | 1 + pnpm-lock.yaml | 3 ++ version.py | 2 +- web/src/upload.ts | 87 ++++++++++++++++++++++++++++----------- web/tests/upload.test.cjs | 44 +++++++++++++++++--- 5 files changed, 108 insertions(+), 29 deletions(-) diff --git a/package.json b/package.json index dd49c3a007..62bda2f822 100644 --- a/package.json +++ b/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.3", "@zxcvbn-ts/core": "^3.0.1", "@zxcvbn-ts/language-common": "^3.0.2", "@zxcvbn-ts/language-en": "^3.0.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b198535ab1..9d3664637b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -76,6 +76,9 @@ importers: '@uppy/tus': specifier: ^4.1.5 version: 4.2.2(@uppy/core@4.4.4) + '@uppy/utils': + specifier: ^6.1.3 + version: 6.1.3 '@zxcvbn-ts/core': specifier: ^3.0.1 version: 3.0.4 diff --git a/version.py b/version.py index 1fe8743b08..ad47bfeef5 100644 --- a/version.py +++ b/version.py @@ -49,4 +49,4 @@ API_FEATURE_LEVEL = 390 # historical commits sharing the same major version, in which case a # minor version bump suffices. -PROVISION_VERSION = (326, 7) # bumped 2025-05-23 to add @types/winchan +PROVISION_VERSION = (326, 8) # bumped 2025-05-27 to add @uppy/utils diff --git a/web/src/upload.ts b/web/src/upload.ts index 8e9824bd95..744a93b7f6 100644 --- a/web/src/upload.ts +++ b/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; -const upload_objects_by_message_edit_row = new Map>(); +let compose_upload_object: Uppy; +const upload_objects_by_message_edit_row = new Map>(); 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, + uppy: Uppy, config: Config, file_id: string, delay = 0, @@ -193,7 +198,7 @@ export function show_error_message( } export let upload_files = ( - uppy: Uppy, + uppy: Uppy, 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 { - const uppy = new Uppy({ +export function setup_upload(config: Config): Uppy { + const uppy = new Uppy({ debug: false, autoProceed: true, restrictions: { @@ -355,7 +360,20 @@ export function setup_upload(config: Config): Uppy { }, 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 { }); 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("]", ""); - const syntax_to_insert = "[" + filtered_filename + "](" + url + ")"; + // We do not receive response text if the file has already + // 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, ); diff --git a/web/tests/upload.test.cjs b/web/tests/upload.test.cjs index c23acee165..4309ba0db1 100644 --- a/web/tests/upload.test.cjs +++ b/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");