paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-10-23 04:52:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
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>
This commit is contained in:
Alex Vandiver
2025-05-27 20:39:28 +00:00
committed by Tim Abbott
parent 098228a210
commit c2e0a27d2c
5 changed files with 108 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
package.json
View File

@@ -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",

3
pnpm-lock.yaml generated
View File

@@ -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

2
version.py
View File

@@ -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

87
web/src/upload.ts
View File

@@ -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>;
const upload_objects_by_message_edit_row = new Map<number, Uppy<Meta, TusBody>>();
let compose_upload_object: Uppy<ZulipMeta, 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> {
const uppy = new Uppy<Meta, TusBody>({
export function setup_upload(config: Config): Uppy<ZulipMeta, 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("]", "");
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,
);

44
web/tests/upload.test.cjs
View File

@@ -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");