Fix progress bar regression throughout the app (#9219)

* Fix useSmoothAnimation enablement not working properly by getting rid of it

Passing duration=0 is more logical and less superfluous

* Refactor UploadBar to handle state more correctly

* Change ProgressBar to new useSmoothAnimation signature and default animated to true for consistency

* Add type guard

* Make types stricter

* Write tests for the ProgressBar component

* Make the new test conform to tsc --strict

* Update UploadBar.tsx

* Update UploadBar.tsx

* Update UploadBar.tsx

src/components/structures/UploadBar.tsx

@@ -17,17 +17,19 @@ limitations under the License.
 import React from 'react';
 import { Room } from "matrix-js-sdk/src/models/room";
 import filesize from "filesize";
-import { IEventRelation } from 'matrix-js-sdk/src/matrix';
+import { IAbortablePromise, IEventRelation } from 'matrix-js-sdk/src/matrix';
+import { Optional } from "matrix-events-sdk";
 
 import ContentMessages from '../../ContentMessages';
 import dis from "../../dispatcher/dispatcher";
 import { _t } from '../../languageHandler';
-import { ActionPayload } from "../../dispatcher/payloads";
 import { Action } from "../../dispatcher/actions";
 import ProgressBar from "../views/elements/ProgressBar";
-import AccessibleButton from "../views/elements/AccessibleButton";
+import AccessibleButton, { ButtonEvent } from "../views/elements/AccessibleButton";
 import { IUpload } from "../../models/IUpload";
 import MatrixClientContext from "../../contexts/MatrixClientContext";
+import { ActionPayload } from '../../dispatcher/payloads';
+import { UploadPayload } from "../../dispatcher/payloads/UploadPayload";
 
 interface IProps {
     room: Room;
@@ -35,23 +37,35 @@ interface IProps {
 }
 
 interface IState {
-    currentUpload?: IUpload;
-    uploadsHere: IUpload[];
+    currentFile?: string;
+    currentPromise?: IAbortablePromise<any>;
+    currentLoaded?: number;
+    currentTotal?: number;
+    countFiles: number;
 }
 
-export default class UploadBar extends React.Component<IProps, IState> {
+function isUploadPayload(payload: ActionPayload): payload is UploadPayload {
+    return [
+        Action.UploadStarted,
+        Action.UploadProgress,
+        Action.UploadFailed,
+        Action.UploadFinished,
+        Action.UploadCanceled,
+    ].includes(payload.action as Action);
+}
+
+export default class UploadBar extends React.PureComponent<IProps, IState> {
     static contextType = MatrixClientContext;
 
-    private dispatcherRef: string;
-    private mounted: boolean;
+    private dispatcherRef: Optional<string>;
+    private mounted = false;
 
     constructor(props) {
         super(props);
 
         // Set initial state to any available upload in this room - we might be mounting
         // earlier than the first progress event, so should show something relevant.
-        const uploadsHere = this.getUploadsInRoom();
-        this.state = { currentUpload: uploadsHere[0], uploadsHere };
+        this.state = this.calculateState();
     }
 
     componentDidMount() {
@@ -61,7 +75,7 @@ export default class UploadBar extends React.Component<IProps, IState> {
 
     componentWillUnmount() {
         this.mounted = false;
-        dis.unregister(this.dispatcherRef);
+        dis.unregister(this.dispatcherRef!);
     }
 
     private getUploadsInRoom(): IUpload[] {
@@ -69,45 +83,48 @@ export default class UploadBar extends React.Component<IProps, IState> {
         return uploads.filter(u => u.roomId === this.props.room.roomId);
     }
 
+    private calculateState(): IState {
+        const [currentUpload, ...otherUploads] = this.getUploadsInRoom();
+        return {
+            currentFile: currentUpload?.fileName,
+            currentPromise: currentUpload?.promise,
+            currentLoaded: currentUpload?.loaded,
+            currentTotal: currentUpload?.total,
+            countFiles: otherUploads.length + 1,
+        };
+    }
+
     private onAction = (payload: ActionPayload) => {
-        switch (payload.action) {
-            case Action.UploadStarted:
-            case Action.UploadProgress:
-            case Action.UploadFinished:
-            case Action.UploadCanceled:
-            case Action.UploadFailed: {
-                if (!this.mounted) return;
-                const uploadsHere = this.getUploadsInRoom();
-                this.setState({ currentUpload: uploadsHere[0], uploadsHere });
-                break;
-            }
+        if (!this.mounted) return;
+        if (isUploadPayload(payload)) {
+            this.setState(this.calculateState());
         }
     };
 
-    private onCancelClick = (ev) => {
+    private onCancelClick = (ev: ButtonEvent) => {
         ev.preventDefault();
-        ContentMessages.sharedInstance().cancelUpload(this.state.currentUpload.promise, this.context);
+        ContentMessages.sharedInstance().cancelUpload(this.state.currentPromise!, this.context);
     };
 
     render() {
-        if (!this.state.currentUpload) {
+        if (!this.state.currentFile) {
             return null;
         }
 
         // MUST use var name 'count' for pluralization to kick in
         const uploadText = _t(
             "Uploading %(filename)s and %(count)s others", {
-                filename: this.state.currentUpload.fileName,
-                count: this.state.uploadsHere.length - 1,
+                filename: this.state.currentFile,
+                count: this.state.countFiles - 1,
             },
         );
 
-        const uploadSize = filesize(this.state.currentUpload.total);
+        const uploadSize = filesize(this.state.currentTotal!);
         return (
             <div className="mx_UploadBar">
                 <div className="mx_UploadBar_filename">{ uploadText } ({ uploadSize })</div>
                 <AccessibleButton onClick={this.onCancelClick} className='mx_UploadBar_cancel' />
-                <ProgressBar value={this.state.currentUpload.loaded} max={this.state.currentUpload.total} />
+                <ProgressBar value={this.state.currentLoaded!} max={this.state.currentTotal!} />
             </div>
         );
     }

src/components/views/elements/ProgressBar.tsx

@@ -25,10 +25,10 @@ interface IProps {
 }
 
 const PROGRESS_BAR_ANIMATION_DURATION = 300;
-const ProgressBar: React.FC<IProps> = ({ value, max, animated }) => {
+const ProgressBar: React.FC<IProps> = ({ value, max, animated = true }) => {
     // Animating progress bars via CSS transition isn’t possible in all of our supported browsers yet.
     // As workaround, we’re using animations through JS requestAnimationFrame
-    const currentValue = useSmoothAnimation(0, value, PROGRESS_BAR_ANIMATION_DURATION, animated);
+    const currentValue = useSmoothAnimation(0, value, animated ? PROGRESS_BAR_ANIMATION_DURATION : 0);
     return <progress className="mx_ProgressBar" max={max} value={currentValue} />;
 };
 

src/dispatcher/payloads/UploadPayload.ts

@@ -18,7 +18,7 @@ import { ActionPayload } from "../payloads";
 import { Action } from "../actions";
 import { IUpload } from "../../models/IUpload";
 
-interface UploadPayload extends ActionPayload {
+export interface UploadPayload extends ActionPayload {
     /**
      * The upload with fields representing the new upload state.
      */

src/hooks/useSmoothAnimation.ts

@@ -30,14 +30,12 @@ const debuglog = (...args: any[]) => {
  * Utility function to smoothly animate to a certain target value
  * @param initialValue Initial value to be used as initial starting point
  * @param targetValue Desired value to animate to (can be changed repeatedly to whatever is current at that time)
- * @param duration Duration that each animation should take
- * @param enabled Whether the animation should run or not
+ * @param duration Duration that each animation should take, specify 0 to skip animating
  */
 export function useSmoothAnimation(
     initialValue: number,
     targetValue: number,
     duration: number,
-    enabled: boolean,
 ): number {
     const state = useRef<{ timestamp: DOMHighResTimeStamp | null, value: number }>({
         timestamp: null,
@@ -79,7 +77,7 @@ export function useSmoothAnimation(
         [currentStepSize, targetValue],
     );
 
-    useAnimation(enabled, update);
+    useAnimation(duration > 0, update);
 
-    return currentValue;
+    return duration > 0 ? currentValue : targetValue;
 }

test/components/views/elements/ProgressBar-test.tsx

@@ -0,0 +1,53 @@
+/*
+Copyright 2022 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 { act, render } from "@testing-library/react";
+
+import ProgressBar from "../../../../src/components/views/elements/ProgressBar";
+
+jest.useFakeTimers();
+
+describe("<ProgressBar/>", () => {
+    it("works when animated", () => {
+        const { container, rerender } = render(<ProgressBar max={100} value={50} animated={true} />);
+        const progress = container.querySelector<HTMLProgressElement>("progress")!;
+
+        // The animation always starts from 0
+        expect(progress.value).toBe(0);
+
+        // Await the animation to conclude to our initial value of 50
+        act(() => { jest.runAllTimers(); });
+        expect(progress.position).toBe(0.5);
+
+        // Move the needle to 80%
+        rerender(<ProgressBar max={100} value={80} animated={true} />);
+        expect(progress.position).toBe(0.5);
+
+        // Let the animaiton run a tiny bit, assert it has moved from where it was to where it needs to go
+        act(() => { jest.advanceTimersByTime(150); });
+        expect(progress.position).toBeGreaterThan(0.5);
+        expect(progress.position).toBeLessThan(0.8);
+    });
+
+    it("works when not animated", () => {
+        const { container, rerender } = render(<ProgressBar max={100} value={50} animated={false} />);
+        const progress = container.querySelector<HTMLProgressElement>("progress")!;
+
+        // Without animation all positional updates are immediate, not requiring timers to run
+        expect(progress.position).toBe(0.5);
+        rerender(<ProgressBar max={100} value={80} animated={false} />);
+        expect(progress.position).toBe(0.8);
+    });
+});