From 2b3e0b47ba443c572f980e1b95ac65877700eb29 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 26 Apr 2023 18:35:20 +0100 Subject: [PATCH] Fix all rooms search generating permalinks to wrong room id (#10625) * Fix all rooms search generating permalinks to wrong room id * Iterate * Add comment * Iterate * Add coverage * Iterate * Add comment * Restore src/utils/permalinks/Permalinks.ts * Update src/components/structures/RoomSearchView.tsx Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/components/structures/RoomSearchView.tsx | 21 ++- src/components/structures/RoomView.tsx | 1 - .../structures/RoomSearchView-test.tsx | 151 +++++++++++++++--- 3 files changed, 151 insertions(+), 22 deletions(-) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index 00b9284420..c1faab885a 100644 --- a/src/components/structures/RoomSearchView.tsx +++ b/src/components/structures/RoomSearchView.tsx @@ -50,7 +50,6 @@ interface Props { promise: Promise; abortController?: AbortController; resizeNotifier: ResizeNotifier; - permalinkCreator: RoomPermalinkCreator; className: string; onUpdate(inProgress: boolean, results: ISearchResults | null): void; } @@ -59,7 +58,7 @@ interface Props { // XXX: why doesn't searching on name work? export const RoomSearchView = forwardRef( ( - { term, scope, promise, abortController, resizeNotifier, permalinkCreator, className, onUpdate }: Props, + { term, scope, promise, abortController, resizeNotifier, className, onUpdate }: Props, ref: RefObject, ) => { const client = useContext(MatrixClientContext); @@ -68,6 +67,15 @@ export const RoomSearchView = forwardRef( const [highlights, setHighlights] = useState(null); const [results, setResults] = useState(null); const aborted = useRef(false); + // A map from room ID to permalink creator + const permalinkCreators = useRef(new Map()).current; + + useEffect(() => { + return () => { + permalinkCreators.forEach((pc) => pc.stop()); + permalinkCreators.clear(); + }; + }, [permalinkCreators]); const handleSearchResult = useCallback( (searchPromise: Promise): Promise => { @@ -217,7 +225,7 @@ export const RoomSearchView = forwardRef( const result = results.results[i]; const mxEv = result.context.getEvent(); - const roomId = mxEv.getRoomId(); + const roomId = mxEv.getRoomId()!; const room = client.getRoom(roomId); if (!room) { // if we do not have the room in js-sdk stores then hide it as we cannot easily show it @@ -283,6 +291,13 @@ export const RoomSearchView = forwardRef( ourEventsIndexes.push(result.context.getOurEventIndex()); } + let permalinkCreator = permalinkCreators.get(roomId); + if (!permalinkCreator) { + permalinkCreator = new RoomPermalinkCreator(room); + permalinkCreator.start(); + permalinkCreators.set(roomId, permalinkCreator); + } + ret.push( { promise={this.state.search.promise} abortController={this.state.search.abortController} resizeNotifier={this.props.resizeNotifier} - permalinkCreator={this.permalinkCreator} className={this.messagePanelClassNames} onUpdate={this.onSearchUpdate} /> diff --git a/test/components/structures/RoomSearchView-test.tsx b/test/components/structures/RoomSearchView-test.tsx index 45563f0839..e2a8360d46 100644 --- a/test/components/structures/RoomSearchView-test.tsx +++ b/test/components/structures/RoomSearchView-test.tsx @@ -28,7 +28,6 @@ import { MatrixClient } from "matrix-js-sdk/src/matrix"; import { RoomSearchView } from "../../../src/components/structures/RoomSearchView"; import { SearchScope } from "../../../src/components/views/rooms/SearchBar"; import ResizeNotifier from "../../../src/utils/ResizeNotifier"; -import { RoomPermalinkCreator } from "../../../src/utils/permalinks/Permalinks"; import { stubClient } from "../../test-utils"; import MatrixClientContext from "../../../src/contexts/MatrixClientContext"; import { MatrixClientPeg } from "../../../src/MatrixClientPeg"; @@ -43,15 +42,13 @@ describe("", () => { const resizeNotifier = new ResizeNotifier(); let client: MatrixClient; let room: Room; - let permalinkCreator: RoomPermalinkCreator; beforeEach(async () => { stubClient(); client = MatrixClientPeg.get(); client.supportsThreads = jest.fn().mockReturnValue(true); - room = new Room("!room:server", client, client.getUserId()!); + room = new Room("!room:server", client, client.getSafeUserId()); mocked(client.getRoom).mockReturnValue(room); - permalinkCreator = new RoomPermalinkCreator(room, room.roomId); jest.spyOn(Element.prototype, "clientHeight", "get").mockReturnValue(100); }); @@ -69,7 +66,6 @@ describe("", () => { scope={SearchScope.All} promise={deferred.promise} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} />, @@ -92,7 +88,7 @@ describe("", () => { result: { room_id: room.roomId, event_id: "$2", - sender: client.getUserId()!, + sender: client.getSafeUserId(), origin_server_ts: 1, content: { body: "Foo Test Bar", msgtype: "m.text" }, type: EventType.RoomMessage, @@ -103,7 +99,7 @@ describe("", () => { { room_id: room.roomId, event_id: "$1", - sender: client.getUserId()!, + sender: client.getSafeUserId(), origin_server_ts: 1, content: { body: "Before", msgtype: "m.text" }, type: EventType.RoomMessage, @@ -113,7 +109,7 @@ describe("", () => { { room_id: room.roomId, event_id: "$3", - sender: client.getUserId()!, + sender: client.getSafeUserId(), origin_server_ts: 1, content: { body: "After", msgtype: "m.text" }, type: EventType.RoomMessage, @@ -128,7 +124,6 @@ describe("", () => { count: 1, })} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -154,7 +149,7 @@ describe("", () => { result: { room_id: room.roomId, event_id: "$2", - sender: client.getUserId()!, + sender: client.getSafeUserId(), origin_server_ts: 1, content: { body: "Foo Test Bar", msgtype: "m.text" }, type: EventType.RoomMessage, @@ -172,7 +167,6 @@ describe("", () => { count: 1, })} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -192,7 +186,7 @@ describe("", () => { result: { room_id: room.roomId, event_id: "$2", - sender: client.getUserId()!, + sender: client.getSafeUserId(), origin_server_ts: 1, content: { body: "Foo Test Bar", msgtype: "m.text" }, type: EventType.RoomMessage, @@ -221,7 +215,7 @@ describe("", () => { result: { room_id: room.roomId, event_id: "$4", - sender: client.getUserId()!, + sender: client.getSafeUserId(), origin_server_ts: 4, content: { body: "Potato", msgtype: "m.text" }, type: EventType.RoomMessage, @@ -245,7 +239,6 @@ describe("", () => { scope={SearchScope.All} promise={Promise.resolve(searchResults)} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -267,7 +260,6 @@ describe("", () => { scope={SearchScope.All} promise={deferred.promise} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -291,7 +283,6 @@ describe("", () => { scope={SearchScope.All} promise={deferred.promise} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -315,7 +306,6 @@ describe("", () => { scope={SearchScope.All} promise={deferred.promise} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -417,7 +407,6 @@ describe("", () => { scope={SearchScope.All} promise={Promise.resolve(searchResults)} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -437,4 +426,130 @@ describe("", () => { expect(betweenNode.compareDocumentPosition(foo2Node) == Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); expect(foo2Node.compareDocumentPosition(afterNode) == Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); }); + + it("should pass appropriate permalink creator for all rooms search", async () => { + const room2 = new Room("!room2:server", client, client.getSafeUserId()); + const room3 = new Room("!room3:server", client, client.getSafeUserId()); + mocked(client.getRoom).mockImplementation( + (roomId) => [room, room2, room3].find((r) => r.roomId === roomId) ?? null, + ); + + render( + + ({ + results: [ + SearchResult.fromJson( + { + rank: 1, + result: { + room_id: room.roomId, + event_id: "$2", + sender: client.getSafeUserId(), + origin_server_ts: 1, + content: { body: "Room 1", msgtype: "m.text" }, + type: EventType.RoomMessage, + }, + context: { + profile_info: {}, + events_before: [], + events_after: [], + }, + }, + eventMapper, + ), + SearchResult.fromJson( + { + rank: 2, + result: { + room_id: room2.roomId, + event_id: "$22", + sender: client.getSafeUserId(), + origin_server_ts: 1, + content: { body: "Room 2", msgtype: "m.text" }, + type: EventType.RoomMessage, + }, + context: { + profile_info: {}, + events_before: [], + events_after: [], + }, + }, + eventMapper, + ), + SearchResult.fromJson( + { + rank: 2, + result: { + room_id: room2.roomId, + event_id: "$23", + sender: client.getSafeUserId(), + origin_server_ts: 2, + content: { body: "Room 2 message 2", msgtype: "m.text" }, + type: EventType.RoomMessage, + }, + context: { + profile_info: {}, + events_before: [], + events_after: [], + }, + }, + eventMapper, + ), + SearchResult.fromJson( + { + rank: 3, + result: { + room_id: room3.roomId, + event_id: "$32", + sender: client.getSafeUserId(), + origin_server_ts: 1, + content: { body: "Room 3", msgtype: "m.text" }, + type: EventType.RoomMessage, + }, + context: { + profile_info: {}, + events_before: [], + events_after: [], + }, + }, + eventMapper, + ), + ], + highlights: [], + count: 1, + })} + resizeNotifier={resizeNotifier} + className="someClass" + onUpdate={jest.fn()} + /> + , + ); + + const event1 = await screen.findByText("Room 1"); + expect(event1.closest(".mx_EventTile_line").querySelector("a")).toHaveAttribute( + "href", + `https://matrix.to/#/${room.roomId}/$2`, + ); + + const event2 = await screen.findByText("Room 2"); + expect(event2.closest(".mx_EventTile_line").querySelector("a")).toHaveAttribute( + "href", + `https://matrix.to/#/${room2.roomId}/$22`, + ); + + const event2Message2 = await screen.findByText("Room 2 message 2"); + expect(event2Message2.closest(".mx_EventTile_line").querySelector("a")).toHaveAttribute( + "href", + `https://matrix.to/#/${room2.roomId}/$23`, + ); + + const event3 = await screen.findByText("Room 3"); + expect(event3.closest(".mx_EventTile_line").querySelector("a")).toHaveAttribute( + "href", + `https://matrix.to/#/${room3.roomId}/$32`, + ); + }); });