Skip to content

Commit 87fd279

Browse files
t3chguyt3chguy
authored
Fix sort order in space hierarchy (#30975)
* Fix sort order in space hierarchy To match spec and not add unexpected sorting by space vs room * Update SpaceHierarchy.tsx * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Update snapshot Signed-off-by: Michael Telatynski <[email protected]> * Add test Signed-off-by: Michael Telatynski <[email protected]> * Update snapshot Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]>
1 parent 77c41d6 commit 87fd279

File tree

3 files changed

+514
-118
lines changed

3 files changed

+514
-118
lines changed

‎src/components/structures/SpaceHierarchy.tsx‎

Lines changed: 56 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ import {
3232
RoomType,
3333
GuestAccess,
3434
HistoryVisibility,
35-
type HierarchyRelation,
3635
type HierarchyRoom,
3736
JoinRule,
3837
} from "matrix-js-sdk/src/matrix";
@@ -71,6 +70,7 @@ import { getTopic } from "../../hooks/room/useTopic";
7170
import { SdkContextClass } from "../../contexts/SDKContext";
7271
import { getDisplayAliasForAliasSet } from "../../Rooms";
7372
import SettingsStore from "../../settings/SettingsStore";
73+
import { filterBoolean } from "../../utils/arrays.ts";
7474

7575
interface IProps {
7676
space: Room;
@@ -504,68 +504,67 @@ export const HierarchyLevel: React.FC<IHierarchyLevelProps> = ({
504504
const space = cli.getRoom(root.room_id);
505505
const hasPermissions = space?.currentState.maySendStateEvent(EventType.SpaceChild, cli.getSafeUserId());
506506

507-
const sortedChildren = sortBy(root.children_state, (ev) => {
508-
return getChildOrder(ev.content.order, ev.origin_server_ts, ev.state_key);
509-
});
510-
511-
const [subspaces, childRooms] = sortedChildren.reduce(
512-
(result, ev: HierarchyRelation) => {
513-
const room = hierarchy.roomMap.get(ev.state_key);
514-
if (room && roomSet.has(room)) {
515-
result[room.room_type === RoomType.Space ? 0 : 1].push(toLocalRoom(cli, room, hierarchy));
516-
}
517-
return result;
518-
},
519-
[[] as HierarchyRoom[], [] as HierarchyRoom[]],
507+
const sortedChildren = filterBoolean(
508+
sortBy(root.children_state, (ev) => {
509+
return getChildOrder(ev.content.order, ev.origin_server_ts, ev.state_key);
510+
}).map((ev) => {
511+
const hierarchyRoom = hierarchy.roomMap.get(ev.state_key);
512+
if (!hierarchyRoom || !roomSet.has(hierarchyRoom)) return null;
513+
// Find the most up-to-date info for this room, if it has been upgraded and we know about it.
514+
return toLocalRoom(cli, hierarchyRoom, hierarchy);
515+
}),
520516
);
521517

522518
const newParents = new Set(parents).add(root.room_id);
523519
return (
524520
<React.Fragment>
525-
{uniqBy(childRooms, "room_id").map((room) => (
526-
<Tile
527-
key={room.room_id}
528-
room={room}
529-
suggested={hierarchy.isSuggested(root.room_id, room.room_id)}
530-
selected={selectedMap?.get(root.room_id)?.has(room.room_id)}
531-
onViewRoomClick={() => onViewRoomClick(room.room_id, room.room_type as RoomType)}
532-
onJoinRoomClick={() => onJoinRoomClick(room.room_id, newParents)}
533-
hasPermissions={hasPermissions}
534-
onToggleClick={onToggleClick ? () => onToggleClick(root.room_id, room.room_id) : undefined}
535-
/>
536-
))}
537-
538-
{subspaces
539-
.filter((room) => !newParents.has(room.room_id))
540-
.map((space) => (
541-
<Tile
542-
key={space.room_id}
543-
room={space}
544-
numChildRooms={
545-
space.children_state.filter((ev) => {
546-
const room = hierarchy.roomMap.get(ev.state_key);
547-
return room && roomSet.has(room) && !room.room_type;
548-
}).length
549-
}
550-
suggested={hierarchy.isSuggested(root.room_id, space.room_id)}
551-
selected={selectedMap?.get(root.room_id)?.has(space.room_id)}
552-
onViewRoomClick={() => onViewRoomClick(space.room_id, RoomType.Space)}
553-
onJoinRoomClick={() => onJoinRoomClick(space.room_id, newParents)}
554-
hasPermissions={hasPermissions}
555-
onToggleClick={onToggleClick ? () => onToggleClick(root.room_id, space.room_id) : undefined}
556-
>
557-
<HierarchyLevel
558-
root={space}
559-
roomSet={roomSet}
560-
hierarchy={hierarchy}
561-
parents={newParents}
562-
selectedMap={selectedMap}
563-
onViewRoomClick={onViewRoomClick}
564-
onJoinRoomClick={onJoinRoomClick}
565-
onToggleClick={onToggleClick}
521+
{uniqBy(sortedChildren, "room_id").map((room) => {
522+
if (room.room_type !== RoomType.Space) {
523+
return (
524+
<Tile
525+
key={room.room_id}
526+
room={room}
527+
suggested={hierarchy.isSuggested(root.room_id, room.room_id)}
528+
selected={selectedMap?.get(root.room_id)?.has(room.room_id)}
529+
onViewRoomClick={() => onViewRoomClick(room.room_id, room.room_type as RoomType)}
530+
onJoinRoomClick={() => onJoinRoomClick(room.room_id, newParents)}
531+
hasPermissions={hasPermissions}
532+
onToggleClick={onToggleClick ? () => onToggleClick(root.room_id, room.room_id) : undefined}
566533
/>
567-
</Tile>
568-
))}
534+
);
535+
} else {
536+
if (newParents.has(room.room_id)) return null; // prevent cycles
537+
return (
538+
<Tile
539+
key={room.room_id}
540+
room={room}
541+
numChildRooms={
542+
room.children_state.filter((ev) => {
543+
const child = hierarchy.roomMap.get(ev.state_key);
544+
return child && roomSet.has(child) && !child.room_type;
545+
}).length
546+
}
547+
suggested={hierarchy.isSuggested(root.room_id, room.room_id)}
548+
selected={selectedMap?.get(root.room_id)?.has(room.room_id)}
549+
onViewRoomClick={() => onViewRoomClick(room.room_id, RoomType.Space)}
550+
onJoinRoomClick={() => onJoinRoomClick(room.room_id, newParents)}
551+
hasPermissions={hasPermissions}
552+
onToggleClick={onToggleClick ? () => onToggleClick(root.room_id, room.room_id) : undefined}
553+
>
554+
<HierarchyLevel
555+
root={room}
556+
roomSet={roomSet}
557+
hierarchy={hierarchy}
558+
parents={newParents}
559+
selectedMap={selectedMap}
560+
onViewRoomClick={onViewRoomClick}
561+
onJoinRoomClick={onJoinRoomClick}
562+
onToggleClick={onToggleClick}
563+
/>
564+
</Tile>
565+
);
566+
}
567+
})}
569568
</React.Fragment>
570569
);
571570
};

‎test/unit-tests/components/structures/SpaceHierarchy-test.tsx‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,14 @@ describe("SpaceHierarchy", () => {
169169
const room2 = mkStubRoom("room-id-3", "Room 2", client);
170170
const space1 = mkStubRoom("space-id-4", "Space 2", client);
171171
const room3 = mkStubRoom("room-id-5", "Room 3", client);
172+
const space2 = mkStubRoom("space-id-6", "Space 3", client);
172173
mocked(client.getRooms).mockReturnValue([root]);
173174
mocked(client.getRoom).mockImplementation(
174175
(roomId) => client.getRooms().find((room) => room.roomId === roomId) ?? null,
175176
);
176-
[room1, room2, space1, room3].forEach((r) => mocked(r.getMyMembership).mockReturnValue(KnownMembership.Leave));
177+
[room1, room2, space1, room3, space2].forEach((r) =>
178+
mocked(r.getMyMembership).mockReturnValue(KnownMembership.Leave),
179+
);
177180

178181
const hierarchyRoot: HierarchyRoom = {
179182
room_id: root.roomId,
@@ -324,5 +327,36 @@ describe("SpaceHierarchy", () => {
324327
undefined,
325328
);
326329
});
330+
331+
it("should not render cycles", async () => {
332+
const hierarchySpace2: HierarchyRoom = {
333+
room_id: space2.roomId,
334+
name: "Space with cycle",
335+
num_joined_members: 1,
336+
room_type: "m.space",
337+
children_state: [
338+
{
339+
state_key: root.roomId,
340+
content: { order: "1" },
341+
origin_server_ts: 111,
342+
type: "m.space.child",
343+
sender: "@other:server",
344+
},
345+
],
346+
world_readable: true,
347+
guest_can_join: true,
348+
};
349+
350+
mocked(client.getRoomHierarchy).mockResolvedValue({
351+
rooms: [hierarchyRoot, hierarchyRoom1, hierarchyRoom2, hierarchySpace1, hierarchySpace2],
352+
});
353+
354+
const { getAllByText, queryByText, asFragment } = render(getComponent());
355+
// Wait for spinners to go away
356+
await waitForElementToBeRemoved(screen.getAllByRole("progressbar"));
357+
expect(getAllByText("Nested space")).toHaveLength(1);
358+
expect(queryByText("Space 1")).not.toBeInTheDocument();
359+
expect(asFragment()).toMatchSnapshot();
360+
});
327361
});
328362
});

0 commit comments

Comments
 (0)