Merge pull request #6346 from matrix-org/t3chguy/fix/17935

only consider valid & loaded url previews for show N more prompt

src/components/views/rooms/LinkPreviewGroup.tsx

@@ -14,43 +14,57 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, { useEffect } from "react";
+import React, { useContext, useEffect } from "react";
 import { MatrixEvent } from "matrix-js-sdk/src/models/event";
+import { IPreviewUrlResponse } from "matrix-js-sdk/src/client";
 
 import { useStateToggle } from "../../../hooks/useStateToggle";
 import LinkPreviewWidget from "./LinkPreviewWidget";
 import AccessibleButton from "../elements/AccessibleButton";
 import { _t } from "../../../languageHandler";
+import MatrixClientContext from "../../../contexts/MatrixClientContext";
+import { useAsyncMemo } from "../../../hooks/useAsyncMemo";
 
 const INITIAL_NUM_PREVIEWS = 2;
 
 interface IProps {
     links: string[]; // the URLs to be previewed
     mxEvent: MatrixEvent; // the Event associated with the preview
-    onCancelClick?(): void; // called when the preview's cancel ('hide') button is clicked
+    onCancelClick(): void; // called when the preview's cancel ('hide') button is clicked
-    onHeightChanged?(): void; // called when the preview's contents has loaded
+    onHeightChanged(): void; // called when the preview's contents has loaded
 }
 
 const LinkPreviewGroup: React.FC<IProps> = ({ links, mxEvent, onCancelClick, onHeightChanged }) => {
+    const cli = useContext(MatrixClientContext);
     const [expanded, toggleExpanded] = useStateToggle();
+
+    const ts = mxEvent.getTs();
+    const previews = useAsyncMemo<[string, IPreviewUrlResponse][]>(async () => {
+        return Promise.all<[string, IPreviewUrlResponse] | void>(links.map(link => {
+            return cli.getUrlPreview(link, ts).then(preview => [link, preview], error => {
+                console.error("Failed to get URL preview: " + error);
+            });
+        })).then(a => a.filter(Boolean)) as Promise<[string, IPreviewUrlResponse][]>;
+    }, [links, ts], []);
+
     useEffect(() => {
         onHeightChanged();
-    }, [onHeightChanged, expanded]);
+    }, [onHeightChanged, expanded, previews]);
 
-    const shownLinks = expanded ? links : links.slice(0, INITIAL_NUM_PREVIEWS);
+    const showPreviews = expanded ? previews : previews.slice(0, INITIAL_NUM_PREVIEWS);
 
-    let toggleButton;
+    let toggleButton: JSX.Element;
-    if (links.length > INITIAL_NUM_PREVIEWS) {
+    if (previews.length > INITIAL_NUM_PREVIEWS) {
         toggleButton = <AccessibleButton onClick={toggleExpanded}>
             { expanded
                 ? _t("Collapse")
-                : _t("Show %(count)s other previews", { count: links.length - shownLinks.length }) }
+                : _t("Show %(count)s other previews", { count: previews.length - showPreviews.length }) }
         </AccessibleButton>;
     }
 
     return <div className="mx_LinkPreviewGroup">
-        { shownLinks.map((link, i) => (
+        { showPreviews.map(([link, preview], i) => (
-            <LinkPreviewWidget key={link} link={link} mxEvent={mxEvent} onHeightChanged={onHeightChanged}>
+            <LinkPreviewWidget key={link} link={link} preview={preview} mxEvent={mxEvent}>
                 { i === 0 ? (
                     <AccessibleButton
                         className="mx_LinkPreviewGroup_hide"

src/components/views/rooms/LinkPreviewWidget.tsx

@@ -21,7 +21,6 @@ import { IPreviewUrlResponse } from 'matrix-js-sdk/src/client';
 
 import { linkifyElement } from '../../../HtmlUtils';
 import SettingsStore from "../../../settings/SettingsStore";
-import { MatrixClientPeg } from "../../../MatrixClientPeg";
 import Modal from "../../../Modal";
 import * as ImageUtils from "../../../ImageUtils";
 import { replaceableComponent } from "../../../utils/replaceableComponent";
@@ -29,37 +28,15 @@ import { mediaFromMxc } from "../../../customisations/Media";
 import ImageView from '../elements/ImageView';
 
 interface IProps {
-    link: string; // the URL being previewed
+    link: string;
+    preview: IPreviewUrlResponse;
     mxEvent: MatrixEvent; // the Event associated with the preview
-    onHeightChanged(): void; // called when the preview's contents has loaded
-}
-
-interface IState {
-    preview?: IPreviewUrlResponse;
 }
 
 @replaceableComponent("views.rooms.LinkPreviewWidget")
-export default class LinkPreviewWidget extends React.Component<IProps, IState> {
+export default class LinkPreviewWidget extends React.Component<IProps> {
-    private unmounted = false;
     private readonly description = createRef<HTMLDivElement>();
 
-    constructor(props) {
-        super(props);
-
-        this.state = {
-            preview: null,
-        };
-
-        MatrixClientPeg.get().getUrlPreview(this.props.link, this.props.mxEvent.getTs()).then((preview) => {
-            if (this.unmounted) {
-                return;
-            }
-            this.setState({ preview }, this.props.onHeightChanged);
-        }, (error) => {
-            console.error("Failed to get URL preview: " + error);
-        });
-    }
-
     componentDidMount() {
         if (this.description.current) {
             linkifyElement(this.description.current);
@@ -72,12 +49,8 @@ export default class LinkPreviewWidget extends React.Component<IProps, IState> {
         }
     }
 
-    componentWillUnmount() {
-        this.unmounted = true;
-    }
-
     private onImageClick = ev => {
-        const p = this.state.preview;
+        const p = this.props.preview;
         if (ev.button != 0 || ev.metaKey) return;
         ev.preventDefault();
 
@@ -99,7 +72,7 @@ export default class LinkPreviewWidget extends React.Component<IProps, IState> {
     };
 
     render() {
-        const p = this.state.preview;
+        const p = this.props.preview;
         if (!p || Object.keys(p).length === 0) {
             return <div />;
         }

test/components/views/messages/TextualBody-test.js

@@ -22,8 +22,10 @@ import sdk from "../../../skinned-sdk";
 import { mkEvent, mkStubRoom } from "../../../test-utils";
 import { MatrixClientPeg } from "../../../../src/MatrixClientPeg";
 import * as languageHandler from "../../../../src/languageHandler";
+import * as TestUtils from "../../../test-utils";
 
-const TextualBody = sdk.getComponent("views.messages.TextualBody");
+const _TextualBody = sdk.getComponent("views.messages.TextualBody");
+const TextualBody = TestUtils.wrapInMatrixClientContext(_TextualBody);
 
 configure({ adapter: new Adapter() });
 
@@ -305,10 +307,9 @@ describe("<TextualBody />", () => {
         const wrapper = mount(<TextualBody mxEvent={ev} showUrlPreview={true} onHeightChanged={() => {}} />);
         expect(wrapper.text()).toBe(ev.getContent().body);
 
-        let widgets = wrapper.find("LinkPreviewWidget");
+        let widgets = wrapper.find("LinkPreviewGroup");
-        // at this point we should have exactly one widget
+        // at this point we should have exactly one link
-        expect(widgets.length).toBe(1);
+        expect(widgets.at(0).prop("links")).toEqual(["https://matrix.org/"]);
-        expect(widgets.at(0).prop("link")).toBe("https://matrix.org/");
 
         // simulate an event edit and check the transition from the old URL preview to the new one
         const ev2 = mkEvent({
@@ -333,11 +334,9 @@ describe("<TextualBody />", () => {
 
             // XXX: this is to give TextualBody enough time for state to settle
             wrapper.setState({}, () => {
-                widgets = wrapper.find("LinkPreviewWidget");
+                widgets = wrapper.find("LinkPreviewGroup");
-                // at this point we should have exactly two widgets (not the matrix.org one anymore)
+                // at this point we should have exactly two links (not the matrix.org one anymore)
-                expect(widgets.length).toBe(2);
+                expect(widgets.at(0).prop("links")).toEqual(["https://vector.im/", "https://riot.im/"]);
-                expect(widgets.at(0).prop("link")).toBe("https://vector.im/");
-                expect(widgets.at(1).prop("link")).toBe("https://riot.im/");
             });
         });
     });