From 85e8d276979e2899501738328b513a1dc92ac45b Mon Sep 17 00:00:00 2001
From: Michael Weimann <michaelw@matrix.org>
Date: Thu, 9 Mar 2023 12:48:36 +0100
Subject: [PATCH] Extract avatars from permalink hook (#10328)

---
 src/components/views/elements/Pill.tsx        | 35 +++++++++---
 .../{usePermalink.tsx => usePermalink.ts}     | 57 ++++++++++++-------
 test/components/views/elements/Pill-test.tsx  | 22 ++++++-
 .../elements/__snapshots__/Pill-test.tsx.snap | 40 +++++++++++++
 4 files changed, 126 insertions(+), 28 deletions(-)
 rename src/hooks/{usePermalink.tsx => usePermalink.ts} (76%)

diff --git a/src/components/views/elements/Pill.tsx b/src/components/views/elements/Pill.tsx
index 3a14f7b634..1ccc3f36b5 100644
--- a/src/components/views/elements/Pill.tsx
+++ b/src/components/views/elements/Pill.tsx
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, { useState } from "react";
+import React, { ReactElement, useState } from "react";
 import classNames from "classnames";
 import { Room } from "matrix-js-sdk/src/models/room";
 
@@ -22,6 +22,8 @@ import { MatrixClientPeg } from "../../../MatrixClientPeg";
 import MatrixClientContext from "../../../contexts/MatrixClientContext";
 import Tooltip, { Alignment } from "../elements/Tooltip";
 import { usePermalink } from "../../../hooks/usePermalink";
+import RoomAvatar from "../avatars/RoomAvatar";
+import MemberAvatar from "../avatars/MemberAvatar";
 
 export enum PillType {
     UserMention = "TYPE_USER_MENTION",
@@ -52,13 +54,13 @@ export interface PillProps {
 
 export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room, shouldShowPillAvatar }) => {
     const [hover, setHover] = useState(false);
-    const { avatar, onClick, resourceId, text, type } = usePermalink({
+    const { member, onClick, resourceId, targetRoom, text, type } = usePermalink({
         room,
         type: propType,
         url,
     });
 
-    if (!type) {
+    if (!type || !text) {
         return null;
     }
 
@@ -79,6 +81,27 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
     };
 
     const tip = hover && resourceId ? <Tooltip label={resourceId} alignment={Alignment.Right} /> : null;
+    let avatar: ReactElement | null = null;
+
+    switch (type) {
+        case PillType.AtRoomMention:
+        case PillType.RoomMention:
+        case "space":
+            avatar = targetRoom ? <RoomAvatar room={targetRoom} width={16} height={16} aria-hidden="true" /> : null;
+            break;
+        case PillType.UserMention:
+            avatar = <MemberAvatar member={member} width={16} height={16} aria-hidden="true" hideTitle />;
+            break;
+        default:
+            return null;
+    }
+
+    const content = (
+        <>
+            {shouldShowPillAvatar && avatar}
+            <span className="mx_Pill_linkText">{text}</span>
+        </>
+    );
 
     return (
         <bdi>
@@ -91,14 +114,12 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
                         onMouseOver={onMouseOver}
                         onMouseLeave={onMouseLeave}
                     >
-                        {shouldShowPillAvatar && avatar}
-                        <span className="mx_Pill_linkText">{text}</span>
+                        {content}
                         {tip}
                     </a>
                 ) : (
                     <span className={classes} onMouseOver={onMouseOver} onMouseLeave={onMouseLeave}>
-                        {shouldShowPillAvatar && avatar}
-                        <span className="mx_Pill_linkText">{text}</span>
+                        {content}
                         {tip}
                     </span>
                 )}
diff --git a/src/hooks/usePermalink.tsx b/src/hooks/usePermalink.ts
similarity index 76%
rename from src/hooks/usePermalink.tsx
rename to src/hooks/usePermalink.ts
index b88f05c744..a6b19d7de7 100644
--- a/src/hooks/usePermalink.tsx
+++ b/src/hooks/usePermalink.ts
@@ -16,7 +16,7 @@ limitations under the License.
 
 import { logger } from "matrix-js-sdk/src/logger";
 import { MatrixEvent, Room, RoomMember } from "matrix-js-sdk/src/matrix";
-import React, { ReactElement, useCallback, useMemo, useState } from "react";
+import { useCallback, useMemo, useState } from "react";
 
 import { ButtonEvent } from "../components/views/elements/AccessibleButton";
 import { PillType } from "../components/views/elements/Pill";
@@ -24,8 +24,6 @@ import { MatrixClientPeg } from "../MatrixClientPeg";
 import { parsePermalink } from "../utils/permalinks/Permalinks";
 import dis from "../dispatcher/dispatcher";
 import { Action } from "../dispatcher/actions";
-import RoomAvatar from "../components/views/avatars/RoomAvatar";
-import MemberAvatar from "../components/views/avatars/MemberAvatar";
 
 interface Args {
     /** Room in which the permalink should be displayed. */
@@ -37,13 +35,38 @@ interface Args {
 }
 
 interface HookResult {
-    /** Avatar of the permalinked resource. */
-    avatar: ReactElement | null;
-    /** Displayable text of the permalink resource. Can for instance be a user or room name. */
+    /**
+     * Room member of a user mention permalink.
+     * null for other links, if the profile was not found or not yet loaded.
+     * This can change, for instance, from null to a RoomMember after the profile lookup completed.
+     */
+    member: RoomMember | null;
+    /**
+     * Displayable text of the permalink resource. Can for instance be a user or room name.
+     * null here means that there is nothing to display. Most likely if the URL was not a permalink.
+     */
     text: string | null;
-    onClick: ((e: ButtonEvent) => void) | null;
-    /** This can be for instance a user or room Id. */
+    /**
+     * Should be used for click actions on the permalink.
+     * In case of a user permalink, a view profile action is dispatched.
+     */
+    onClick: (e: ButtonEvent) => void;
+    /**
+     * This can be for instance a user or room Id.
+     * null here means that the resource cannot be detected. Most likely if the URL was not a permalink.
+     */
     resourceId: string | null;
+    /**
+     * Target room of the permalink:
+     * For an @room mention, this is the room where the permalink should be displayed.
+     * For a room permalink, it is the room from the permalink.
+     * null for other links or if the room cannot be found.
+     */
+    targetRoom: Room | null;
+    /**
+     * Type of the pill plus "space" for spaces.
+     * null here means that the type cannot be detected. Most likely if the URL was not a permalink.
+     */
     type: PillType | "space" | null;
 }
 
@@ -53,7 +76,7 @@ interface HookResult {
 export const usePermalink: (args: Args) => HookResult = ({ room, type: argType, url }): HookResult => {
     const [member, setMember] = useState<RoomMember | null>(null);
     // room of the entity this pill points to
-    const [targetRoom, setTargetRoom] = useState<Room | undefined | null>(room);
+    const [targetRoom, setTargetRoom] = useState<Room | null>(room ?? null);
 
     let resourceId: string | null = null;
 
@@ -101,9 +124,6 @@ export const usePermalink: (args: Args) => HookResult = ({ room, type: argType,
 
     useMemo(() => {
         switch (type) {
-            case PillType.AtRoomMention:
-                setTargetRoom(room);
-                break;
             case PillType.UserMention:
                 {
                     if (resourceId) {
@@ -131,23 +151,20 @@ export const usePermalink: (args: Args) => HookResult = ({ room, type: argType,
                                           );
                                       })
                                 : MatrixClientPeg.get().getRoom(resourceId);
-                        setTargetRoom(newRoom);
+                        setTargetRoom(newRoom || null);
                     }
                 }
                 break;
         }
     }, [doProfileLookup, type, resourceId, room]);
 
-    let onClick: ((e: ButtonEvent) => void) | null = null;
-    let avatar: ReactElement | null = null;
+    let onClick: (e: ButtonEvent) => void = () => {};
     let text = resourceId;
 
     if (type === PillType.AtRoomMention && room) {
         text = "@room";
-        avatar = <RoomAvatar room={room} width={16} height={16} aria-hidden="true" />;
     } else if (type === PillType.UserMention && member) {
         text = member.name || resourceId;
-        avatar = <MemberAvatar member={member} width={16} height={16} aria-hidden="true" hideTitle />;
         onClick = (e: ButtonEvent): void => {
             e.preventDefault();
             dis.dispatch({
@@ -158,15 +175,15 @@ export const usePermalink: (args: Args) => HookResult = ({ room, type: argType,
     } else if (type === PillType.RoomMention) {
         if (targetRoom) {
             text = targetRoom.name || resourceId;
-            avatar = <RoomAvatar room={targetRoom} width={16} height={16} aria-hidden="true" />;
         }
     }
 
     return {
-        avatar,
-        text,
+        member,
         onClick,
         resourceId,
+        targetRoom,
+        text,
         type,
     };
 };
diff --git a/test/components/views/elements/Pill-test.tsx b/test/components/views/elements/Pill-test.tsx
index 8659ebb682..72c68e117c 100644
--- a/test/components/views/elements/Pill-test.tsx
+++ b/test/components/views/elements/Pill-test.tsx
@@ -38,6 +38,8 @@ describe("<Pill>", () => {
     const room1Alias = "#room1:example.com";
     const room1Id = "!room1:example.com";
     let room1: Room;
+    const space1Id = "!space1:example.com";
+    let space1: Room;
     const user1Id = "@user1:example.com";
     const user2Id = "@user2:example.com";
     let renderResult: RenderResult;
@@ -70,9 +72,13 @@ describe("<Pill>", () => {
         ]);
         room1.getMember(user1Id)!.setMembershipEvent(user1JoinRoom1Event);
 
-        client.getRooms.mockReturnValue([room1]);
+        space1 = new Room(space1Id, client, client.getSafeUserId());
+        space1.name = "Space 1";
+
+        client.getRooms.mockReturnValue([room1, space1]);
         client.getRoom.mockImplementation((roomId: string) => {
             if (roomId === room1.roomId) return room1;
+            if (roomId === space1.roomId) return space1;
             return null;
         });
 
@@ -116,6 +122,20 @@ describe("<Pill>", () => {
         });
     });
 
+    it("should not render a non-permalink", () => {
+        renderPill({
+            url: "https://example.com/hello",
+        });
+        expect(renderResult.asFragment()).toMatchSnapshot();
+    });
+
+    it("should render the expected pill for a space", () => {
+        renderPill({
+            url: permalinkPrefix + space1Id,
+        });
+        expect(renderResult.asFragment()).toMatchSnapshot();
+    });
+
     it("should render the expected pill for a room alias", () => {
         renderPill({
             url: permalinkPrefix + room1Alias,
diff --git a/test/components/views/elements/__snapshots__/Pill-test.tsx.snap b/test/components/views/elements/__snapshots__/Pill-test.tsx.snap
index 1fd947f8a8..ea973e3686 100644
--- a/test/components/views/elements/__snapshots__/Pill-test.tsx.snap
+++ b/test/components/views/elements/__snapshots__/Pill-test.tsx.snap
@@ -1,5 +1,7 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`<Pill> should not render a non-permalink 1`] = `<DocumentFragment />`;
+
 exports[`<Pill> should not render an avatar or link when called with inMessage = false and shouldShowPillAvatar = false 1`] = `
 <DocumentFragment>
   <bdi>
@@ -91,6 +93,44 @@ exports[`<Pill> should render the expected pill for a room alias 1`] = `
 </DocumentFragment>
 `;
 
+exports[`<Pill> should render the expected pill for a space 1`] = `
+<DocumentFragment>
+  <bdi>
+    <a
+      class="mx_Pill mx_RoomPill"
+      href="https://matrix.to/#/!space1:example.com"
+    >
+      <span
+        aria-hidden="true"
+        class="mx_BaseAvatar"
+        role="presentation"
+      >
+        <span
+          aria-hidden="true"
+          class="mx_BaseAvatar_initial"
+          style="font-size: 10.4px; width: 16px; line-height: 16px;"
+        >
+          S
+        </span>
+        <img
+          alt=""
+          aria-hidden="true"
+          class="mx_BaseAvatar_image"
+          data-testid="avatar-img"
+          src="data:image/png;base64,00"
+          style="width: 16px; height: 16px;"
+        />
+      </span>
+      <span
+        class="mx_Pill_linkText"
+      >
+        Space 1
+      </span>
+    </a>
+  </bdi>
+</DocumentFragment>
+`;
+
 exports[`<Pill> should render the expected pill for a user not in the room 1`] = `
 <DocumentFragment>
   <bdi>