From ecd2a8e981093bcce28376d84422862460ae405f Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 17 Apr 2023 08:51:10 +0100 Subject: [PATCH 1/9] Fix all rooms search generating permalinks to wrong room id --- src/components/structures/RoomSearchView.tsx | 22 +++++++++++++++++++- src/utils/permalinks/Permalinks.ts | 2 +- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index 00b92844209..53567575c0f 100644 --- a/src/components/structures/RoomSearchView.tsx +++ b/src/components/structures/RoomSearchView.tsx @@ -59,7 +59,16 @@ 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, + permalinkCreator: _permalinkCreator, + className, + onUpdate, + }: Props, ref: RefObject, ) => { const client = useContext(MatrixClientContext); @@ -68,6 +77,7 @@ export const RoomSearchView = forwardRef( const [highlights, setHighlights] = useState(null); const [results, setResults] = useState(null); const aborted = useRef(false); + const permalinkCreators = useRef(new Map()).current; const handleSearchResult = useCallback( (searchPromise: Promise): Promise => { @@ -283,6 +293,16 @@ export const RoomSearchView = forwardRef( ourEventsIndexes.push(result.context.getOurEventIndex()); } + let permalinkCreator = _permalinkCreator; + if (roomId !== permalinkCreator.roomId) { + if (permalinkCreators.has(roomId)) { + permalinkCreator = permalinkCreators.get(roomId); + } else { + permalinkCreator = new RoomPermalinkCreator(client.getRoom(roomId), roomId); + permalinkCreators.set(roomId, permalinkCreator); + } + } + ret.push( Date: Mon, 17 Apr 2023 09:08:08 +0100 Subject: [PATCH 2/9] Iterate --- src/components/structures/RoomSearchView.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index 53567575c0f..28bd7562bdc 100644 --- a/src/components/structures/RoomSearchView.tsx +++ b/src/components/structures/RoomSearchView.tsx @@ -227,7 +227,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 @@ -296,7 +296,7 @@ export const RoomSearchView = forwardRef( let permalinkCreator = _permalinkCreator; if (roomId !== permalinkCreator.roomId) { if (permalinkCreators.has(roomId)) { - permalinkCreator = permalinkCreators.get(roomId); + permalinkCreator = permalinkCreators.get(roomId)!; } else { permalinkCreator = new RoomPermalinkCreator(client.getRoom(roomId), roomId); permalinkCreators.set(roomId, permalinkCreator); From 64bf46e2acbf5f54cdc81c983ca08c2166bbc971 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 17 Apr 2023 10:28:07 +0100 Subject: [PATCH 3/9] Add comment --- src/components/structures/RoomSearchView.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index 28bd7562bdc..056308e534c 100644 --- a/src/components/structures/RoomSearchView.tsx +++ b/src/components/structures/RoomSearchView.tsx @@ -77,6 +77,9 @@ export const RoomSearchView = forwardRef( const [highlights, setHighlights] = useState(null); const [results, setResults] = useState(null); const aborted = useRef(false); + // The permalinkCreator prop we are passed is only for the room the user was viewing + // So we will need to create additional room permalink creators for "All rooms" mode results + // to be able to generate share permalinks for results from other rooms const permalinkCreators = useRef(new Map()).current; const handleSearchResult = useCallback( From 7f07110898b767e0b37b63f707d01b6f732b0dd6 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 17 Apr 2023 10:34:14 +0100 Subject: [PATCH 4/9] Iterate --- src/components/structures/RoomSearchView.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index 056308e534c..deb4ec514b6 100644 --- a/src/components/structures/RoomSearchView.tsx +++ b/src/components/structures/RoomSearchView.tsx @@ -82,6 +82,13 @@ export const RoomSearchView = forwardRef( // to be able to generate share permalinks for results from other rooms const permalinkCreators = useRef(new Map()).current; + useEffect(() => { + return () => { + permalinkCreators.forEach((pc) => pc.stop()); + permalinkCreators.clear(); + }; + }, [permalinkCreators]); + const handleSearchResult = useCallback( (searchPromise: Promise): Promise => { setInProgress(true); @@ -302,6 +309,7 @@ export const RoomSearchView = forwardRef( permalinkCreator = permalinkCreators.get(roomId)!; } else { permalinkCreator = new RoomPermalinkCreator(client.getRoom(roomId), roomId); + permalinkCreator.start(); permalinkCreators.set(roomId, permalinkCreator); } } From 6540bb3e7bc00ce3e04280f12a77dd863cd2f987 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Apr 2023 09:49:23 +0100 Subject: [PATCH 5/9] Add coverage --- .../structures/RoomSearchView-test.tsx | 141 +++++++++++++++++- 1 file changed, 134 insertions(+), 7 deletions(-) diff --git a/test/components/structures/RoomSearchView-test.tsx b/test/components/structures/RoomSearchView-test.tsx index 45563f0839b..5ee977fa5e9 100644 --- a/test/components/structures/RoomSearchView-test.tsx +++ b/test/components/structures/RoomSearchView-test.tsx @@ -49,7 +49,7 @@ describe("", () => { 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); @@ -92,7 +92,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 +103,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 +113,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, @@ -154,7 +154,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, @@ -192,7 +192,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 +221,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, @@ -437,4 +437,131 @@ 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} + permalinkCreator={permalinkCreator} + 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`, + ); + }); }); From 3c022370c89f3ee7744670b715159f63597ce567 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Apr 2023 13:41:50 +0100 Subject: [PATCH 6/9] Iterate --- src/components/structures/RoomSearchView.tsx | 26 +++++-------------- src/components/structures/RoomView.tsx | 1 - .../structures/RoomSearchView-test.tsx | 12 --------- 3 files changed, 6 insertions(+), 33 deletions(-) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index deb4ec514b6..462856e66bd 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,16 +58,7 @@ interface Props { // XXX: why doesn't searching on name work? export const RoomSearchView = forwardRef( ( - { - term, - scope, - promise, - abortController, - resizeNotifier, - permalinkCreator: _permalinkCreator, - className, - onUpdate, - }: Props, + { term, scope, promise, abortController, resizeNotifier, className, onUpdate }: Props, ref: RefObject, ) => { const client = useContext(MatrixClientContext); @@ -303,15 +293,11 @@ export const RoomSearchView = forwardRef( ourEventsIndexes.push(result.context.getOurEventIndex()); } - let permalinkCreator = _permalinkCreator; - if (roomId !== permalinkCreator.roomId) { - if (permalinkCreators.has(roomId)) { - permalinkCreator = permalinkCreators.get(roomId)!; - } else { - permalinkCreator = new RoomPermalinkCreator(client.getRoom(roomId), roomId); - permalinkCreator.start(); - permalinkCreators.set(roomId, permalinkCreator); - } + let permalinkCreator = permalinkCreators.get(roomId); + if (!permalinkCreator) { + permalinkCreator = new RoomPermalinkCreator(client.getRoom(roomId), roomId); + permalinkCreator.start(); + permalinkCreators.set(roomId, permalinkCreator); } ret.push( diff --git a/src/components/structures/RoomView.tsx b/src/components/structures/RoomView.tsx index 1eaa2cf16e3..a37cf390c20 100644 --- a/src/components/structures/RoomView.tsx +++ b/src/components/structures/RoomView.tsx @@ -2261,7 +2261,6 @@ export class RoomView extends React.Component { 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 5ee977fa5e9..e2a8360d46d 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,7 +42,6 @@ describe("", () => { const resizeNotifier = new ResizeNotifier(); let client: MatrixClient; let room: Room; - let permalinkCreator: RoomPermalinkCreator; beforeEach(async () => { stubClient(); @@ -51,7 +49,6 @@ describe("", () => { client.supportsThreads = jest.fn().mockReturnValue(true); 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()} />, @@ -128,7 +124,6 @@ describe("", () => { count: 1, })} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -172,7 +167,6 @@ describe("", () => { count: 1, })} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> @@ -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()} /> @@ -533,7 +522,6 @@ describe("", () => { count: 1, })} resizeNotifier={resizeNotifier} - permalinkCreator={permalinkCreator} className="someClass" onUpdate={jest.fn()} /> From dadd7e61c6609efcfb2e09f964dab35631177431 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Apr 2023 13:42:27 +0100 Subject: [PATCH 7/9] Add comment --- src/components/structures/RoomSearchView.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index 462856e66bd..5b957ba6a40 100644 --- a/src/components/structures/RoomSearchView.tsx +++ b/src/components/structures/RoomSearchView.tsx @@ -67,9 +67,7 @@ export const RoomSearchView = forwardRef( const [highlights, setHighlights] = useState(null); const [results, setResults] = useState(null); const aborted = useRef(false); - // The permalinkCreator prop we are passed is only for the room the user was viewing - // So we will need to create additional room permalink creators for "All rooms" mode results - // to be able to generate share permalinks for results from other rooms + // A map from room ID to permalink creator const permalinkCreators = useRef(new Map()).current; useEffect(() => { From 81c1a8d841e914533b1479a98bcde86a6adffd3d Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Apr 2023 14:30:02 +0100 Subject: [PATCH 8/9] Restore src/utils/permalinks/Permalinks.ts --- src/utils/permalinks/Permalinks.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index 5243cd5f3a0..c4e15f3918f 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -82,7 +82,7 @@ const ANY_REGEX = /.*/; // the list and magically have the link work. export class RoomPermalinkCreator { - public readonly roomId: string; + private roomId: string; private highestPlUserId: string | null = null; private populationMap: { [serverName: string]: number } = {}; private bannedHostsRegexps: RegExp[] = []; From c583f0acbd388f131a5da7fd316606b5672b1ccf Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Apr 2023 14:30:11 +0100 Subject: [PATCH 9/9] Update src/components/structures/RoomSearchView.tsx Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/components/structures/RoomSearchView.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/RoomSearchView.tsx b/src/components/structures/RoomSearchView.tsx index 5b957ba6a40..c1faab885ac 100644 --- a/src/components/structures/RoomSearchView.tsx +++ b/src/components/structures/RoomSearchView.tsx @@ -293,7 +293,7 @@ export const RoomSearchView = forwardRef( let permalinkCreator = permalinkCreators.get(roomId); if (!permalinkCreator) { - permalinkCreator = new RoomPermalinkCreator(client.getRoom(roomId), roomId); + permalinkCreator = new RoomPermalinkCreator(room); permalinkCreator.start(); permalinkCreators.set(roomId, permalinkCreator); }