From d5921b9f34ac5e3b70da482f47fd573992609264 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Thu, 10 Sep 2026 14:53:03 -0300 Subject: [PATCH] refactor(ui): drop the devices list's unreachable status filter The list hid every status tab but accepted and an effect forced the parameter back, so the pending and rejected column sets, their tabs and the accept/reject/remove dialog could not be reached by any URL. Acceptance is reviewed on the install keys list, so the tab that used to filter is one link out to it. --- .../pages/devices/__tests__/Devices.test.tsx | 58 +--- ui/apps/console/src/pages/devices/index.tsx | 284 +++++------------- 2 files changed, 82 insertions(+), 260 deletions(-) diff --git a/ui/apps/console/src/pages/devices/__tests__/Devices.test.tsx b/ui/apps/console/src/pages/devices/__tests__/Devices.test.tsx index d88977fd8cc..d5c62227df7 100644 --- a/ui/apps/console/src/pages/devices/__tests__/Devices.test.tsx +++ b/ui/apps/console/src/pages/devices/__tests__/Devices.test.tsx @@ -2,7 +2,6 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { render, screen, act, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import React from "react"; -import type { UseActionDialogResult } from "@/hooks/useActionDialog"; import { createTestWrapper } from "@/tests/wrapper"; import { mockDevice, mockNamespace } from "@/tests/factories"; import { mockSdkResponse, paginatedResponse } from "@/tests/sdk"; @@ -88,22 +87,6 @@ vi.mock("@/components/common/TagsPopover", () => ({ ), })); -vi.mock("@/components/common/ActionDialog", () => ({ - default: () => null, -})); - -const mockRequestAction = vi.fn(); -const mockDeviceActionsController: UseActionDialogResult = { - action: undefined, - actionKey: "closed", - requestAction: mockRequestAction, - close: vi.fn(), - handleSuccess: vi.fn(), -}; -vi.mock("@/hooks/useActionDialog", () => ({ - useActionDialog: vi.fn(() => mockDeviceActionsController), -})); - vi.mock("@/components/common/RestrictedAction", () => ({ default: ({ children }: { children: React.ReactNode }) => <>{children}, })); @@ -133,7 +116,6 @@ beforeEach(() => { sdk.pullTagFromDevice.mockResolvedValue(mockSdkResponse(undefined)); mockNavigate.mockReset(); mockManageTagsDrawer.mockReset(); - mockRequestAction.mockReset(); }); describe("Devices list", () => { @@ -145,22 +127,23 @@ describe("Devices list", () => { ).toBeInTheDocument(); }); - it("renders the Accepted tab and the Install Keys link, not the pending/rejected tabs", async () => { + it("shows the list is the accepted devices, with no status to switch", async () => { renderPage(); - expect( - await screen.findByRole("button", { name: "Accepted" }), - ).toBeInTheDocument(); - expect( - screen.getByRole("link", { name: "Install Keys" }), - ).toBeInTheDocument(); - expect( - screen.queryByRole("button", { name: "Pending" }), - ).not.toBeInTheDocument(); + + expect(await screen.findByText("Accepted")).toBeInTheDocument(); expect( screen.queryByRole("button", { name: "Rejected" }), ).not.toBeInTheDocument(); }); + it("links out to the install keys", async () => { + renderPage(); + + expect( + await screen.findByRole("link", { name: "Install Keys" }), + ).toHaveAttribute("href", "/install-keys"); + }); + it("renders the search input", async () => { renderPage(); expect( @@ -278,12 +261,12 @@ describe("Devices list", () => { }); describe("URL hydration — URL params seed page state on mount", () => { - it("passes status from URL to the SDK", async () => { - renderPage(["/?status=pending&tags=a&tags=b&page=2"]); + it("always asks for accepted devices, whatever the URL says", async () => { + renderPage(["/?status=pending"]); await waitFor(() => { expect(sdk.getDevices).toHaveBeenCalledWith( expect.objectContaining({ - query: expect.objectContaining({ status: "pending" }), + query: expect.objectContaining({ status: "accepted" }), }), ); }); @@ -312,7 +295,7 @@ describe("Devices list", () => { }); }); - it("falls back to status=accepted and page=1 when URL has no params", async () => { + it("defaults to accepted devices and page 1 when the URL has no params", async () => { renderPage(["/"]); await waitFor(() => { expect(sdk.getDevices).toHaveBeenCalledWith( @@ -323,17 +306,6 @@ describe("Devices list", () => { }); }); - it("falls back to status=accepted for an invalid status value", async () => { - renderPage(["/?status=invalid"]); - await waitFor(() => { - expect(sdk.getDevices).toHaveBeenCalledWith( - expect.objectContaining({ - query: expect.objectContaining({ status: "accepted" }), - }), - ); - }); - }); - it("passes no tag filter when no tags param is present", async () => { renderPage(["/"]); await waitFor(() => { diff --git a/ui/apps/console/src/pages/devices/index.tsx b/ui/apps/console/src/pages/devices/index.tsx index 4b7f85ce798..6bc3a92809a 100644 --- a/ui/apps/console/src/pages/devices/index.tsx +++ b/ui/apps/console/src/pages/devices/index.tsx @@ -1,4 +1,4 @@ -import { useState, useMemo, useCallback, useEffect } from "react"; +import { useState, useMemo, useCallback } from "react"; import { useNavigate, Link } from "react-router-dom"; import { useDevices, type NormalizedDevice } from "@/hooks/useDevices"; import { useDebouncedValue } from "@/hooks/useDebouncedValue"; @@ -7,7 +7,6 @@ import { usePaginatedListState } from "@/hooks/usePaginatedListState"; import { useNamespace } from "@/hooks/useNamespaces"; import { useAuthStore } from "@/stores/authStore"; import { useTerminalStore } from "@/stores/terminalStore"; -import { useActionDialog } from "@/hooks/useActionDialog"; import PageHeader from "@/components/common/PageHeader"; import ConnectDrawer from "@/components/ConnectDrawer"; import ManageTagsDrawer from "@/components/ManageTagsDrawer"; @@ -24,8 +23,6 @@ import { useAddDeviceTag, useRemoveDeviceTag, } from "@/hooks/useDeviceMutations"; -import ActionDialog from "@/components/common/ActionDialog"; -import { useDeviceActionRunner } from "@/hooks/useDeviceActionRunner"; import { PlusIcon, TagIcon, @@ -39,38 +36,21 @@ import { Callout, IconButton, } from "@shellhub/design-system/primitives"; -import { cn } from "@shellhub/design-system/cn"; import RestrictedAction from "@/components/common/RestrictedAction"; import { apiErrorMessage } from "@/api/errors"; import { PER_PAGE, pageCount } from "@/utils/pagination"; const SEARCH_DEBOUNCE_MS = 300; -const VALID_STATUSES = ["accepted", "pending", "rejected"] as const; - -/** Stable module-level constant — avoids a new object identity every render, - * which would invalidate the `update` useCallback in usePaginatedListState. */ -const CONSTRAINTS = { status: VALID_STATUSES } as const; - -type ValidStatus = (typeof VALID_STATUSES)[number]; - -const statusTabs: { label: string; value: ValidStatus }[] = [ - { label: "Accepted", value: "accepted" }, - { label: "Pending", value: "pending" }, - { label: "Rejected", value: "rejected" }, -]; - type DevicesParams = { page: number; search: string; - status: ValidStatus; tags: string[]; }; const DEFAULTS: DevicesParams = { page: 1, search: "", - status: "accepted", tags: [], }; @@ -81,17 +61,10 @@ type SortField = "name" | "last_seen"; * filter state held in the URL so a view can be shared. */ export default function Devices() { - const { - params, - setPage, - setSearch, - setFilter, - setArrayFilter, - mapArrayFilter, - } = usePaginatedListState({ - defaults: DEFAULTS, - constraints: CONSTRAINTS, - }); + const { params, setPage, setSearch, setArrayFilter, mapArrayFilter } = + usePaginatedListState({ + defaults: DEFAULTS, + }); const debouncedSearch = useDebouncedValue( params.search.trim(), @@ -100,9 +73,6 @@ export default function Devices() { const addDeviceTag = useAddDeviceTag(); const removeDeviceTag = useRemoveDeviceTag(); - const runDeviceAction = useDeviceActionRunner(); - const deviceActions = useActionDialog(); - const { requestAction: requestDeviceAction } = deviceActions; const [connectTarget, setConnectTarget] = useState<{ uid: string; name: string; @@ -117,7 +87,7 @@ export default function Devices() { const { devices, totalCount, isLoading, error, refetch } = useDevices({ page: params.page, perPage: PER_PAGE, - status: params.status, + status: "accepted", search: debouncedSearch, filterTags: params.tags, sortBy, @@ -131,18 +101,6 @@ export default function Devices() { const totalPages = pageCount(totalCount); const nsName = currentNamespace?.name ?? ""; - const visibleTabs = statusTabs.filter((tab) => tab.value === "accepted"); - - const handleStatusChange = (newStatus: ValidStatus) => { - setFilter("status", newStatus); - }; - - useEffect(() => { - if (params.status !== "accepted") { - setFilter("status", "accepted"); - } - }, [params.status, setFilter]); - const addFilterTag = useCallback( (tag: string) => { mapArrayFilter("tags", (tags) => @@ -161,7 +119,7 @@ export default function Devices() { }; const columns = useMemo[]>(() => { - const baseColumns: Column[] = [ + const [hostnameColumn, ...detailColumns]: Column[] = [ { key: "name", header: "Hostname", @@ -207,163 +165,80 @@ export default function Devices() { }, ]; - if (params.status === "accepted") { - return [ - { - key: "online", - header: "", - headerClassName: "w-12", - render: (device) => , - }, - baseColumns[0], // hostname - { - key: "sshid", - header: "SSHID", - render: (device) => { - const sshid = nsName - ? buildSshid(nsName, device.name) - : device.uid.substring(0, 8); - return ( -
- - {sshid} - - -
- ); - }, - }, - ...baseColumns.slice(1), // os, tags, last_seen - { - key: "connect", - header: "", - headerClassName: "w-20", - render: (device) => - device.online ? ( - - - - ) : ( - - Offline - - ), - }, - ]; - } - - if (params.status === "pending") { - return [ - ...baseColumns, - { - key: "actions", - header: "Actions", - headerClassName: "text-right", - render: (device) => ( -
- - - - - - + return [ + { + key: "online", + header: "", + headerClassName: "w-12", + render: (device) => , + }, + hostnameColumn, + { + key: "sshid", + header: "SSHID", + render: (device) => { + const sshid = nsName + ? buildSshid(nsName, device.name) + : device.uid.substring(0, 8); + return ( +
+ + {sshid} + +
- ), + ); }, - ]; - } - - return [ - ...baseColumns, + }, + ...detailColumns, { - key: "actions", - header: "Actions", - headerClassName: "text-right", - render: (device) => ( -
- + key: "connect", + header: "", + headerClassName: "w-20", + render: (device) => + device.online ? ( + - - - -
- ), + ) : ( + + Offline + + ), }, ]; }, [ - params.status, nsName, addFilterTag, - requestDeviceAction, addDeviceTag.mutateAsync, removeDeviceTag.mutateAsync, ]); @@ -391,23 +266,9 @@ export default function Devices() {
- {visibleTabs.map((tab) => ( - - ))} - {/* Acceptance moved to the install-keys area, so the tab that used to be Pending now - navigates there instead of filtering (the arrow signals it leaves the list). */} + + Accepted + - {deviceActions.action && ( - - )} - setConnectTarget(null)}