From 80a7de43141b09b2385f2113c9b39b205fa5bf5f Mon Sep 17 00:00:00 2001 From: David Langley Date: Wed, 29 Oct 2025 08:52:17 +0000 Subject: [PATCH] Address code smells from #30453 (#31083) * check recents length * Improve sorting logic in capabilities dialog * Fix allowedHosts regex array check * Remove double negative in filter checks * prettier --- src/components/views/dialogs/InviteDialog.tsx | 2 +- .../WidgetCapabilitiesPromptDialog.tsx | 11 +- .../dialogs/spotlight/SpotlightDialog.tsx | 8 +- src/utils/permalinks/Permalinks.ts | 2 +- .../views/dialogs/SpotlightDialog-test.tsx | 35 +++++ .../WidgetCapabilitiesPromptDialog-test.tsx | 133 ++++++++++++++++++ .../utils/permalinks/Permalinks-test.ts | 27 ++++ 7 files changed, 207 insertions(+), 11 deletions(-) create mode 100644 test/unit-tests/components/views/dialogs/WidgetCapabilitiesPromptDialog-test.tsx diff --git a/src/components/views/dialogs/InviteDialog.tsx b/src/components/views/dialogs/InviteDialog.tsx index afaad55ae3..b0eaf707b2 100644 --- a/src/components/views/dialogs/InviteDialog.tsx +++ b/src/components/views/dialogs/InviteDialog.tsx @@ -415,7 +415,7 @@ export default class InviteDialog extends React.PureComponent b.lastActive - a.lastActive); diff --git a/src/components/views/dialogs/WidgetCapabilitiesPromptDialog.tsx b/src/components/views/dialogs/WidgetCapabilitiesPromptDialog.tsx index 2837701673..3cc54b4499 100644 --- a/src/components/views/dialogs/WidgetCapabilitiesPromptDialog.tsx +++ b/src/components/views/dialogs/WidgetCapabilitiesPromptDialog.tsx @@ -91,12 +91,13 @@ export default class WidgetCapabilitiesPromptDialog extends React.PureComponent< const isTimelineA = isTimelineCapability(capA); const isTimelineB = isTimelineCapability(capB); - if (!isTimelineA && !isTimelineB) return lexicographicCompare(capA, capB); - if (isTimelineA && !isTimelineB) return 1; - if (!isTimelineA && isTimelineB) return -1; - if (isTimelineA && isTimelineB) return lexicographicCompare(capA, capB); + // Sort timeline capabilities to the bottom, non-timeline to the top + if (isTimelineA !== isTimelineB) { + return isTimelineA ? 1 : -1; + } - return 0; + // Both are the same type (both timeline or both non-timeline), sort lexicographically + return lexicographicCompare(capA, capB); }); const checkboxRows = orderedCapabilities.map(([cap, isChecked], i) => { const text = CapabilityText.for(cap, this.props.widgetKind); diff --git a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx index b85c1de29e..d9f2f98390 100644 --- a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx +++ b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx @@ -1144,7 +1144,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n if (rovingContext.state.activeNode && rovingContext.state.nodes.length > 0) { let nodes = rovingContext.state.nodes; - if (!query && !filter !== null) { + if (!query && filter === null) { // If the current selection is not in the recently viewed row then only include the // first recently viewed so that is the target when the user is switching into recently viewed. const keptRecentlyViewedRef = nodeIsForRecentlyViewed(rovingContext.state.activeNode) @@ -1164,7 +1164,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n // only handle these keys when we are in the recently viewed row of options if ( !query && - !filter !== null && + filter === null && rovingContext.state.activeNode && rovingContext.state.nodes.length > 0 && nodeIsForRecentlyViewed(rovingContext.state.activeNode) @@ -1226,8 +1226,8 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n <> ↓ ↑ - {!filter !== null && !query && ←} - {!filter !== null && !query && →} + {filter === null && !query && ←} + {filter === null && !query && →} ), }, diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index 0df4b4beb1..5c5e0b271a 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -217,7 +217,7 @@ export class RoomPermalinkCreator { const allowed = aclEvent.getContent<{ allow: string[] }>().allow; allowedHostsRegexps = []; // we don't want to use the default rule here - if (Array.isArray(denied)) { + if (Array.isArray(allowed)) { allowed.forEach((h) => allowedHostsRegexps.push(getRegex(h))); } } diff --git a/test/unit-tests/components/views/dialogs/SpotlightDialog-test.tsx b/test/unit-tests/components/views/dialogs/SpotlightDialog-test.tsx index 1c343a30a7..2cf9511686 100644 --- a/test/unit-tests/components/views/dialogs/SpotlightDialog-test.tsx +++ b/test/unit-tests/components/views/dialogs/SpotlightDialog-test.tsx @@ -664,4 +664,39 @@ describe("Spotlight Dialog", () => { }), ); }); + + describe("keyboard prompt filter and query checks", () => { + it("should show left and right arrow keys in keyboard hint when filter is null and no query", async () => { + render( null} />); + jest.advanceTimersByTime(200); + await flushPromisesWithFakeTimers(); + + const keyboardPrompt = document.querySelector("#mx_SpotlightDialog_keyboardPrompt"); + expect(keyboardPrompt).toBeInTheDocument(); + expect(keyboardPrompt?.textContent).toContain("←"); + expect(keyboardPrompt?.textContent).toContain("→"); + }); + + it("should not show left and right arrow keys in keyboard hint when filter is set", async () => { + render( null} />); + jest.advanceTimersByTime(200); + await flushPromisesWithFakeTimers(); + + const keyboardPrompt = document.querySelector("#mx_SpotlightDialog_keyboardPrompt"); + expect(keyboardPrompt).toBeInTheDocument(); + expect(keyboardPrompt?.textContent).not.toContain("←"); + expect(keyboardPrompt?.textContent).not.toContain("→"); + }); + + it("should not show left and right arrow keys in keyboard hint when query is present", async () => { + render( null} />); + jest.advanceTimersByTime(200); + await flushPromisesWithFakeTimers(); + + const keyboardPrompt = document.querySelector("#mx_SpotlightDialog_keyboardPrompt"); + expect(keyboardPrompt).toBeInTheDocument(); + expect(keyboardPrompt?.textContent).not.toContain("←"); + expect(keyboardPrompt?.textContent).not.toContain("→"); + }); + }); }); diff --git a/test/unit-tests/components/views/dialogs/WidgetCapabilitiesPromptDialog-test.tsx b/test/unit-tests/components/views/dialogs/WidgetCapabilitiesPromptDialog-test.tsx new file mode 100644 index 0000000000..5abc5ed93b --- /dev/null +++ b/test/unit-tests/components/views/dialogs/WidgetCapabilitiesPromptDialog-test.tsx @@ -0,0 +1,133 @@ +/* +Copyright 2025 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import React from "react"; +import { render, screen } from "jest-matrix-react"; +import { WidgetKind } from "matrix-widget-api"; + +import WidgetCapabilitiesPromptDialog from "../../../../../src/components/views/dialogs/WidgetCapabilitiesPromptDialog"; +import { stubClient } from "../../../../test-utils"; + +describe("WidgetCapabilitiesPromptDialog", () => { + const mockWidget = { + id: "test-widget", + name: "Test Widget", + } as any; + + const onFinished = jest.fn(); + + beforeEach(() => { + stubClient(); + onFinished.mockClear(); + }); + + describe("capability sorting", () => { + it("should sort non-timeline capabilities before timeline capabilities", () => { + // Create a mix of timeline and non-timeline capabilities + const capabilities = new Set([ + "org.matrix.msc2762.timeline:*", + "org.matrix.msc2762.receive.event:m.room.message", + "org.matrix.msc2762.timeline:!room:example.com", + "org.matrix.msc2931.navigate", + "org.matrix.msc2762.receive.state_event:m.room.member#@user:example.com", + ]); + + const { container } = render( + , + ); + + const checkboxes = container.querySelectorAll(".mx_WidgetCapabilitiesPromptDialog_cap"); + + // Verify that we have all the capabilities rendered + expect(checkboxes.length).toBe(5); + + // The dialog should render successfully with the mixed capabilities + expect(screen.getByText("Approve widget permissions")).toBeInTheDocument(); + }); + + it("should sort capabilities lexicographically within the same type", () => { + // Create multiple non-timeline capabilities + const capabilities = new Set([ + "org.matrix.msc2931.navigate", + "org.matrix.msc2762.receive.event:m.room.message", + "org.matrix.msc2762.receive.state_event:m.room.member", + ]); + + render( + , + ); + + // The dialog should render without errors and show the capabilities + expect(screen.getByText("Approve widget permissions")).toBeInTheDocument(); + }); + + it("should handle only timeline capabilities", () => { + const capabilities = new Set([ + "org.matrix.msc2762.timeline:!room1:example.com", + "org.matrix.msc2762.timeline:!room2:example.com", + "org.matrix.msc2762.timeline:*", + ]); + + const { container } = render( + , + ); + + const checkboxes = container.querySelectorAll(".mx_WidgetCapabilitiesPromptDialog_cap"); + expect(checkboxes.length).toBe(3); + }); + + it("should handle only non-timeline capabilities", () => { + const capabilities = new Set([ + "org.matrix.msc2931.navigate", + "org.matrix.msc2762.receive.event:m.room.message", + ]); + + const { container } = render( + , + ); + + const checkboxes = container.querySelectorAll(".mx_WidgetCapabilitiesPromptDialog_cap"); + expect(checkboxes.length).toBe(2); + }); + + it("should handle empty capabilities", () => { + const capabilities = new Set([]); + + const { container } = render( + , + ); + + const checkboxes = container.querySelectorAll(".mx_WidgetCapabilitiesPromptDialog_cap"); + expect(checkboxes.length).toBe(0); + }); + }); +}); diff --git a/test/unit-tests/utils/permalinks/Permalinks-test.ts b/test/unit-tests/utils/permalinks/Permalinks-test.ts index fcb7d3257f..4f96e6c41f 100644 --- a/test/unit-tests/utils/permalinks/Permalinks-test.ts +++ b/test/unit-tests/utils/permalinks/Permalinks-test.ts @@ -334,6 +334,33 @@ describe("Permalinks", function () { expect(creator.serverCandidates![0]).toEqual("evilcorp.com"); }); + it("should handle when ACL allow is not an array", function () { + const roomId = "!fake:example.org"; + const room = mockRoom(roomId, [makeMemberWithPL(roomId, "@alice:goodcorp.com", 100)], { + deny: ["*.evilcorp.com"], + allow: "not-an-array" as any, // Test malformed data + }); + const creator = new RoomPermalinkCreator(room); + creator.load(); + // Should fall back to default behavior (no allowed servers list = allow none) + expect(creator.serverCandidates).toBeTruthy(); + expect(creator.serverCandidates!.length).toBe(0); + }); + + it("should handle when ACL deny is not an array", function () { + const roomId = "!fake:example.org"; + const room = mockRoom(roomId, [makeMemberWithPL(roomId, "@alice:goodcorp.com", 100)], { + deny: "not-an-array" as any, // Test malformed data + allow: ["*"], + }); + const creator = new RoomPermalinkCreator(room); + creator.load(); + // Should not crash and still allow the server + expect(creator.serverCandidates).toBeTruthy(); + expect(creator.serverCandidates!.length).toBe(1); + expect(creator.serverCandidates![0]).toEqual("goodcorp.com"); + }); + it("should generate an event permalink for room IDs with no candidate servers", function () { const room = mockRoom("!somewhere:example.org", []); const creator = new RoomPermalinkCreator(room);