Handle media download errors better (#12848)

* Handle media download errors better Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Add test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Show error if media download failed Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * More tests Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
tchapgouv · Jul 31, 2024 · f3ac669 · f3ac669
1 parent b55653d
commit f3ac669
Show file tree

Hide file tree

Showing 6 changed files with 140 additions and 8 deletions.
diff --git a/src/components/views/messages/DownloadActionButton.tsx b/src/components/views/messages/DownloadActionButton.tsx
@@ -24,6 +24,8 @@ import { RovingAccessibleButton } from "../../../accessibility/RovingTabIndex";
 import Spinner from "../elements/Spinner";
 import { _t, _td, TranslationKey } from "../../../languageHandler";
 import { FileDownloader } from "../../../utils/FileDownloader";
+import Modal from "../../../Modal";
+import ErrorDialog from "../dialogs/ErrorDialog";
 
 interface IProps {
     mxEvent: MatrixEvent;
@@ -53,6 +55,23 @@ export default class DownloadActionButton extends React.PureComponent<IProps, IS
     }
 
     private onDownloadClick = async (): Promise<void> => {
+        try {
+            await this.doDownload();
+        } catch (e) {
+            Modal.createDialog(ErrorDialog, {
+                title: _t("timeline|download_failed"),
+                description: (
+                    <>
+                        <div>{_t("timeline|download_failed_description")}</div>
+                        <div>{e instanceof Error ? e.toString() : ""}</div>
+                    </>
+                ),
+            });
+            this.setState({ loading: false });
+        }
+    };
+
+    private async doDownload(): Promise<void> {
         const mediaEventHelper = this.props.mediaEventHelperGet();
         if (this.state.loading || !mediaEventHelper) return;
 
@@ -64,15 +83,15 @@ export default class DownloadActionButton extends React.PureComponent<IProps, IS
 
         if (this.state.blob) {
             // Cheat and trigger a download, again.
-            return this.doDownload(this.state.blob);
+            return this.downloadBlob(this.state.blob);
         }
 
         const blob = await mediaEventHelper.sourceBlob.value;
         this.setState({ blob });
-        await this.doDownload(blob);
-    };
+        await this.downloadBlob(blob);
+    }
 
-    private async doDownload(blob: Blob): Promise<void> {
+    private async downloadBlob(blob: Blob): Promise<void> {
         await this.downloader.download({
             blob,
             name: this.props.mediaEventHelperGet()!.fileName,

diff --git a/src/customisations/Media.ts b/src/customisations/Media.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { MatrixClient, ResizeMethod } from "matrix-js-sdk/src/matrix";
+import { MatrixClient, parseErrorResponse, ResizeMethod } from "matrix-js-sdk/src/matrix";
 import { MediaEventContent } from "matrix-js-sdk/src/types";
 import { Optional } from "matrix-events-sdk";
 
@@ -144,12 +144,16 @@ export class Media {
      * Downloads the source media.
      * @returns {Promise<Response>} Resolves to the server's response for chaining.
      */
-    public downloadSource(): Promise<Response> {
+    public async downloadSource(): Promise<Response> {
         const src = this.srcHttp;
         if (!src) {
             throw new UserFriendlyError("error|download_media");
         }
-        return fetch(src);
+        const res = await fetch(src);
+        if (!res.ok) {
+            throw parseErrorResponse(res, await res.text());
+        }
+        return res;
     }
 }
 

diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
@@ -3264,6 +3264,8 @@
         "disambiguated_profile": "%(displayName)s (%(matrixId)s)",
         "download_action_decrypting": "Decrypting",
         "download_action_downloading": "Downloading",
+        "download_failed": "Download failed",
+        "download_failed_description": "An error occurred while downloading this file",
         "edits": {
             "tooltip_label": "Edited at %(date)s. Click to view edits.",
             "tooltip_sub": "Click to view edits",

diff --git a/test/components/views/messages/DownloadActionButton-test.tsx b/test/components/views/messages/DownloadActionButton-test.tsx
@@ -0,0 +1,68 @@
+/*
+Copyright 2024 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import React from "react";
+import { mocked } from "jest-mock";
+import fetchMockJest from "fetch-mock-jest";
+import { fireEvent, render, screen, waitFor } from "@testing-library/react";
+import { MatrixEvent } from "matrix-js-sdk/src/matrix";
+
+import { stubClient } from "../../../test-utils";
+import DownloadActionButton from "../../../../src/components/views/messages/DownloadActionButton";
+import Modal from "../../../../src/Modal";
+import { MediaEventHelper } from "../../../../src/utils/MediaEventHelper";
+import ErrorDialog from "../../../../src/components/views/dialogs/ErrorDialog";
+
+describe("DownloadActionButton", () => {
+    it("should show error if media API returns one", async () => {
+        const cli = stubClient();
+        // eslint-disable-next-line no-restricted-properties
+        mocked(cli.mxcUrlToHttp).mockImplementation(
+            (mxc) => `https://matrix.org/_matrix/media/r0/download/${mxc.slice(6)}`,
+        );
+
+        fetchMockJest.get("https://matrix.org/_matrix/media/r0/download/matrix.org/1234", {
+            status: 404,
+            body: { errcode: "M_NOT_FOUND", error: "Not found" },
+        });
+
+        const event = new MatrixEvent({
+            room_id: "!room:id",
+            sender: "@user:id",
+            type: "m.room.message",
+            content: {
+                body: "test",
+                msgtype: "m.image",
+                url: "mxc://matrix.org/1234",
+            },
+        });
+        const mediaEventHelper = new MediaEventHelper(event);
+
+        render(<DownloadActionButton mxEvent={event} mediaEventHelperGet={() => mediaEventHelper} />);
+
+        const spy = jest.spyOn(Modal, "createDialog");
+
+        fireEvent.click(screen.getByRole("button"));
+        await waitFor(() =>
+            expect(spy).toHaveBeenCalledWith(
+                ErrorDialog,
+                expect.objectContaining({
+                    title: "Download failed",
+                }),
+            ),
+        );
+    });
+});
diff --git a/test/customisations/Media-test.ts b/test/customisations/Media-test.ts
@@ -0,0 +1,39 @@
+/*
+Copyright 2024 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import fetchMockJest from "fetch-mock-jest";
+import { mocked } from "jest-mock";
+
+import { mediaFromMxc } from "../../src/customisations/Media";
+import { stubClient } from "../test-utils";
+
+describe("Media", () => {
+    it("should not download error if server returns one", async () => {
+        const cli = stubClient();
+        // eslint-disable-next-line no-restricted-properties
+        mocked(cli.mxcUrlToHttp).mockImplementation(
+            (mxc) => `https://matrix.org/_matrix/media/r0/download/${mxc.slice(6)}`,
+        );
+
+        fetchMockJest.get("https://matrix.org/_matrix/media/r0/download/matrix.org/1234", {
+            status: 404,
+            body: { errcode: "M_NOT_FOUND", error: "Not found" },
+        });
+
+        const media = mediaFromMxc("mxc://matrix.org/1234");
+        await expect(media.downloadSource()).rejects.toThrow("Not found");
+    });
+});
diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts
@@ -172,7 +172,7 @@ export function createTestClient(): MatrixClient {
                 content: {},
             });
         }),
-        mxcUrlToHttp: (mxc: string) => `http://this.is.a.url/${mxc.substring(6)}`,
+        mxcUrlToHttp: jest.fn().mockImplementation((mxc: string) => `http://this.is.a.url/${mxc.substring(6)}`),
         scheduleAllGroupSessionsForBackup: jest.fn().mockResolvedValue(undefined),
         setAccountData: jest.fn(),
         setRoomAccountData: jest.fn(),