From 1bf13bf567b4a4830a575bb411d5edada40406a3 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Mon, 30 Mar 2026 15:45:01 +0200 Subject: [PATCH] fix(ui): code quality improvements for findings grouped view - Replace `any` with `unknown` + type guards in finding-groups and findings-by-resource adapters (4 adapter functions hardened) - Add unmount cleanup for AbortController in use-resource-detail-drawer to prevent memory leaks on mid-flight unmount - Add try/catch/finally error handling for onBeforeOpen in floating mute button to prevent permanent spinner state on network errors - Replace hardcoded colors (border-gray-300, bg-white, text-slate-950) with semantic tokens (border-default-200, bg-background, text-foreground) for dark mode compatibility - Change non-accessible `

` to ` ); }, diff --git a/ui/components/findings/table/resource-detail-drawer/resource-detail-drawer-content.test.tsx b/ui/components/findings/table/resource-detail-drawer/resource-detail-drawer-content.test.tsx new file mode 100644 index 0000000000..69003bcbc5 --- /dev/null +++ b/ui/components/findings/table/resource-detail-drawer/resource-detail-drawer-content.test.tsx @@ -0,0 +1,357 @@ +import { render } from "@testing-library/react"; +import type { ButtonHTMLAttributes, HTMLAttributes, ReactNode } from "react"; +import { describe, expect, it, vi } from "vitest"; + +// --------------------------------------------------------------------------- +// Hoist mocks for components that pull in next-auth transitively +// --------------------------------------------------------------------------- + +vi.mock("next/navigation", () => ({ + useRouter: () => ({ refresh: vi.fn() }), + usePathname: () => "/findings", + useSearchParams: () => new URLSearchParams(), + redirect: vi.fn(), +})); + +vi.mock("next/image", () => ({ + default: ({ alt }: { alt: string }) => , +})); + +vi.mock("next/link", () => ({ + default: ({ children, href }: { children: ReactNode; href: string }) => ( + {children} + ), +})); + +// Mock the entire shadcn barrel to avoid auth import chain +vi.mock("@/components/shadcn", () => { + const Passthrough = ({ children }: { children?: ReactNode }) => ( + <>{children} + ); + return { + Badge: ({ + children, + className, + }: { + children: ReactNode; + className?: string; + }) => {children}, + Button: ({ + children, + ...props + }: ButtonHTMLAttributes & { + variant?: string; + size?: string; + asChild?: boolean; + }) => , + InfoField: ({ + children, + label, + }: { + children: ReactNode; + label: string; + variant?: string; + }) => ( +

+ {label} + {children} +
+ ), + Tabs: Passthrough, + TabsContent: ({ + children, + value, + }: { + children: ReactNode; + value: string; + }) =>
{children}
, + TabsList: Passthrough, + TabsTrigger: ({ + children, + value, + }: { + children: ReactNode; + value: string; + }) => , + Tooltip: Passthrough, + TooltipContent: Passthrough, + TooltipTrigger: Passthrough, + }; +}); + +vi.mock("@/components/shadcn/card/card", () => ({ + Card: ({ children }: { children: ReactNode }) =>
{children}
, +})); + +vi.mock("@/components/shadcn/dropdown", () => ({ + ActionDropdown: ({ children }: { children: ReactNode }) => ( +
{children}
+ ), + ActionDropdownItem: () => null, +})); + +vi.mock("@/components/shadcn/skeleton/skeleton", () => ({ + Skeleton: () =>
, +})); + +vi.mock("@/components/shadcn/spinner/spinner", () => ({ + Spinner: () =>
, +})); + +vi.mock("@/components/shadcn/tooltip", () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children}, +})); + +vi.mock("@/components/findings/mute-findings-modal", () => ({ + MuteFindingsModal: () => null, +})); + +vi.mock("@/components/findings/send-to-jira-modal", () => ({ + SendToJiraModal: () => null, +})); + +vi.mock("@/components/findings/markdown-container", () => ({ + MarkdownContainer: ({ children }: { children: ReactNode }) => children, +})); + +vi.mock("@/components/icons", () => ({ + getComplianceIcon: vi.fn(() => null), +})); + +vi.mock("@/components/icons/services/IconServices", () => ({ + JiraIcon: () => null, +})); + +vi.mock("@/components/ui/code-snippet/code-snippet", () => ({ + CodeSnippet: ({ value }: { value: string }) => {value}, +})); + +vi.mock("@/components/ui/custom/custom-link", () => ({ + CustomLink: ({ children, href }: { children: ReactNode; href: string }) => ( + {children} + ), +})); + +vi.mock("@/components/ui/entities/date-with-time", () => ({ + DateWithTime: ({ dateTime }: { dateTime: string }) => {dateTime}, +})); + +vi.mock("@/components/ui/entities/entity-info", () => ({ + EntityInfo: () => null, +})); + +vi.mock("@/components/ui/table", () => ({ + Table: ({ children }: { children: ReactNode }) => {children}
, + TableBody: ({ children }: { children: ReactNode }) => ( + {children} + ), + TableCell: ({ children }: { children: ReactNode }) => {children}, + TableHead: ({ children }: { children: ReactNode }) => {children}, + TableHeader: ({ children }: { children: ReactNode }) => ( + {children} + ), + TableRow: ({ children, ...props }: HTMLAttributes) => ( + {children} + ), +})); + +vi.mock("@/components/ui/table/severity-badge", () => ({ + SeverityBadge: ({ severity }: { severity: string }) => ( + {severity} + ), +})); + +vi.mock("@/components/ui/table/status-finding-badge", () => ({ + FindingStatus: {}, + StatusFindingBadge: ({ status }: { status: string }) => {status}, +})); + +vi.mock("@/components/shared/events-timeline/events-timeline", () => ({ + EventsTimeline: () => null, +})); + +vi.mock("@/lib/region-flags", () => ({ + getRegionFlag: vi.fn(() => "🇺🇸"), +})); + +vi.mock("@/lib/date-utils", () => ({ + getFailingForLabel: vi.fn(() => "2 days"), + formatDuration: vi.fn(() => "5m"), +})); + +vi.mock("@/lib/utils", () => ({ + cn: (...args: (string | undefined | false | null)[]) => + args.filter(Boolean).join(" "), +})); + +vi.mock("../delta-indicator", () => ({ + DeltaIndicator: () => null, +})); + +vi.mock("../notification-indicator", () => ({ + NotificationIndicator: () => null, +})); + +vi.mock("./resource-detail-skeleton", () => ({ + ResourceDetailSkeleton: () =>
, +})); + +vi.mock("../../muted", () => ({ + Muted: () => null, +})); + +// --------------------------------------------------------------------------- +// Import after mocks +// --------------------------------------------------------------------------- + +import type { ResourceDrawerFinding } from "@/actions/findings"; + +import { ResourceDetailDrawerContent } from "./resource-detail-drawer-content"; +import type { CheckMeta } from "./use-resource-detail-drawer"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +const mockCheckMeta: CheckMeta = { + checkId: "s3_check", + checkTitle: "S3 Check", + risk: "High", + description: "S3 description", + complianceFrameworks: ["CIS-1.4", "PCI-DSS"], + categories: ["security"], + remediation: { + recommendation: { text: "Fix it", url: "https://example.com" }, + code: { cli: "", other: "", nativeiac: "", terraform: "" }, + }, + additionalUrls: [], +}; + +const mockFinding: ResourceDrawerFinding = { + id: "finding-1", + uid: "uid-1", + checkId: "s3_check", + checkTitle: "S3 Check", + status: "FAIL", + severity: "critical", + delta: null, + isMuted: false, + mutedReason: null, + firstSeenAt: null, + updatedAt: null, + resourceId: "res-1", + resourceUid: "arn:aws:s3:::bucket", + resourceName: "my-bucket", + resourceService: "s3", + resourceRegion: "us-east-1", + resourceType: "Bucket", + resourceGroup: "default", + providerType: "aws", + providerAlias: "prod", + providerUid: "123456789", + risk: "High", + description: "Description", + statusExtended: "Status extended", + complianceFrameworks: [], + categories: [], + remediation: { + recommendation: { text: "Fix", url: "" }, + code: { cli: "", other: "", nativeiac: "", terraform: "" }, + }, + additionalUrls: [], + scan: null, +}; + +// --------------------------------------------------------------------------- +// Fix 4: Dark mode — no hardcoded color classes +// --------------------------------------------------------------------------- + +describe("ResourceDetailDrawerContent — dark mode token classes", () => { + it("should NOT use hardcoded bg-white class anywhere in the component", () => { + // Given + const { container } = render( + , + ); + + // When — collect class strings from HTML elements only (skip SVG) + const allElements = container.querySelectorAll("[class]"); + const classStrings = Array.from(allElements) + .map((el) => (typeof el.className === "string" ? el.className : "")) + .filter(Boolean); + const hasBgWhite = classStrings.some((c) => c.includes("bg-white")); + + // Then — no hardcoded bg-white + expect(hasBgWhite).toBe(false); + }); + + it("should NOT use hardcoded border-gray-300 class anywhere in the component", () => { + // Given + const { container } = render( + , + ); + + // When + const allElements = container.querySelectorAll("[class]"); + const classStrings = Array.from(allElements) + .map((el) => (typeof el.className === "string" ? el.className : "")) + .filter(Boolean); + const hasBorderGray = classStrings.some((c) => + c.includes("border-gray-300"), + ); + + // Then + expect(hasBorderGray).toBe(false); + }); + + it("should NOT use hardcoded text-slate-950 class anywhere", () => { + // Given + const { container } = render( + , + ); + + // When + const allElements = container.querySelectorAll("[class]"); + const classStrings = Array.from(allElements) + .map((el) => (typeof el.className === "string" ? el.className : "")) + .filter(Boolean); + const hasTextSlate = classStrings.some((c) => c.includes("text-slate-950")); + + // Then + expect(hasTextSlate).toBe(false); + }); +}); diff --git a/ui/components/findings/table/resource-detail-drawer/resource-detail-drawer-content.tsx b/ui/components/findings/table/resource-detail-drawer/resource-detail-drawer-content.tsx index e54839ea54..878f1e973b 100644 --- a/ui/components/findings/table/resource-detail-drawer/resource-detail-drawer-content.tsx +++ b/ui/components/findings/table/resource-detail-drawer/resource-detail-drawer-content.tsx @@ -200,7 +200,7 @@ export function ResourceDetailDrawerContent({ return icon ? ( -
+
{framework} - + {framework} @@ -684,7 +684,7 @@ export function ResourceDetailDrawerContent({ {/* Lighthouse AI button */} ({ + getLatestFindingsByResourceUidMock: vi.fn(), + adaptFindingsByResourceResponseMock: vi.fn(), +})); + +vi.mock("@/actions/findings", () => ({ + getLatestFindingsByResourceUid: getLatestFindingsByResourceUidMock, + adaptFindingsByResourceResponse: adaptFindingsByResourceResponseMock, +})); + +vi.mock("next/navigation", () => ({ + redirect: vi.fn(), +})); + +// --------------------------------------------------------------------------- +// Import after mocks +// --------------------------------------------------------------------------- + +import type { FindingResourceRow } from "@/types"; + +import { useResourceDetailDrawer } from "./use-resource-detail-drawer"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function makeResource( + overrides?: Partial, +): FindingResourceRow { + return { + id: "row-1", + rowType: "resource" as const, + findingId: "finding-1", + checkId: "s3_check", + providerType: "aws", + providerAlias: "prod", + providerUid: "123", + resourceName: "my-bucket", + resourceGroup: "default", + resourceUid: "arn:aws:s3:::my-bucket", + service: "s3", + region: "us-east-1", + severity: "critical", + status: "FAIL", + isMuted: false, + mutedReason: undefined, + firstSeenAt: null, + lastSeenAt: null, + ...overrides, + } as FindingResourceRow; +} + +// --------------------------------------------------------------------------- +// Fix 2: AbortController cleanup on unmount +// --------------------------------------------------------------------------- + +describe("useResourceDetailDrawer — unmount cleanup", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.restoreAllMocks(); + }); + + it("should abort the in-flight fetch controller when the hook unmounts", async () => { + // Given — spy on AbortController.prototype.abort to detect abort calls + const abortSpy = vi.spyOn(AbortController.prototype, "abort"); + + // never-resolving fetch to simulate in-flight request + getLatestFindingsByResourceUidMock.mockImplementation( + () => new Promise(() => {}), + ); + adaptFindingsByResourceResponseMock.mockReturnValue([]); + + const resources = [makeResource()]; + + const { result, unmount } = renderHook(() => + useResourceDetailDrawer({ + resources, + checkId: "s3_check", + }), + ); + + // When — trigger a fetch by opening the drawer + act(() => { + result.current.openDrawer(0); + }); + + // Verify a fetch was started + expect(getLatestFindingsByResourceUidMock).toHaveBeenCalledTimes(1); + + // Reset spy count to detect only the unmount abort + abortSpy.mockClear(); + + // Then — unmount while fetch is in flight + unmount(); + + // The abort should have been called on unmount + expect(abortSpy).toHaveBeenCalledTimes(1); + }); + + it("should not abort when no fetch has been started yet", () => { + // Given — spy on abort + const abortSpy = vi.spyOn(AbortController.prototype, "abort"); + + const resources = [makeResource()]; + + // When — render without opening drawer (no fetch started) + const { unmount } = renderHook(() => + useResourceDetailDrawer({ + resources, + checkId: "s3_check", + }), + ); + + // Then — unmount without any fetch + unmount(); + + // abort should NOT have been called (fetchControllerRef.current is null) + expect(abortSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/components/findings/table/resource-detail-drawer/use-resource-detail-drawer.ts b/ui/components/findings/table/resource-detail-drawer/use-resource-detail-drawer.ts index bd39b3d63c..d1d9eb96d3 100644 --- a/ui/components/findings/table/resource-detail-drawer/use-resource-detail-drawer.ts +++ b/ui/components/findings/table/resource-detail-drawer/use-resource-detail-drawer.ts @@ -1,6 +1,6 @@ "use client"; -import { useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { adaptFindingsByResourceResponse, @@ -84,6 +84,14 @@ export function useResourceDetailDrawer({ const checkMetaRef = useRef(null); const fetchControllerRef = useRef(null); + // Abort any in-flight request on unmount to prevent state updates + // on an already-unmounted component. + useEffect(() => { + return () => { + fetchControllerRef.current?.abort(); + }; + }, []); + const fetchFindings = async (resourceUid: string) => { // Abort any in-flight request to prevent stale data from out-of-order responses fetchControllerRef.current?.abort(); diff --git a/ui/hooks/use-infinite-resources.ts b/ui/hooks/use-infinite-resources.ts index 63a86aec3a..fc720dd9f7 100644 --- a/ui/hooks/use-infinite-resources.ts +++ b/ui/hooks/use-infinite-resources.ts @@ -202,6 +202,7 @@ export function useInfiniteResources({ const controller = new AbortController(); controllerRef.current = controller; + pageRef.current = 1; hasMoreRef.current = true; isLoadingRef.current = false; diff --git a/ui/lib/region-flags.test.ts b/ui/lib/region-flags.test.ts new file mode 100644 index 0000000000..8a2211b96d --- /dev/null +++ b/ui/lib/region-flags.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; + +import { getRegionFlag } from "./region-flags"; + +// --------------------------------------------------------------------------- +// Fix 6: Taiwan (asia-east1) mapped correctly vs Hong Kong (asia-east2) +// --------------------------------------------------------------------------- + +describe("getRegionFlag — Taiwan vs Hong Kong disambiguation", () => { + it("should return 🇹🇼 for GCP asia-east1 (Taiwan)", () => { + // Given/When + const result = getRegionFlag("asia-east1"); + + // Then + expect(result).toBe("🇹🇼"); + }); + + it("should return 🇭🇰 for GCP asia-east2 (Hong Kong)", () => { + // Given/When + const result = getRegionFlag("asia-east2"); + + // Then + expect(result).toBe("🇭🇰"); + }); + + it("should return 🇭🇰 for regions containing 'hongkong'", () => { + // Given/When + const result = getRegionFlag("hongkong"); + + // Then + expect(result).toBe("🇭🇰"); + }); + + it("should NOT return 🇭🇰 for asia-east1", () => { + // Given/When + const result = getRegionFlag("asia-east1"); + + // Then — confirm it's not Hong Kong flag + expect(result).not.toBe("🇭🇰"); + }); +}); + +describe("getRegionFlag — existing regions not broken", () => { + it("should return 🇺🇸 for us-east-1 (AWS)", () => { + expect(getRegionFlag("us-east-1")).toBe("🇺🇸"); + }); + + it("should return 🇪🇺 for eu-west-1 (AWS)", () => { + expect(getRegionFlag("eu-west-1")).toBe("🇪🇺"); + }); + + it("should return 🇯🇵 for ap-northeast-1 (Japan)", () => { + expect(getRegionFlag("ap-northeast-1")).toBe("🇯🇵"); + }); + + it("should return 🇦🇺 for ap-southeast-2 (Australia)", () => { + expect(getRegionFlag("ap-southeast-2")).toBe("🇦🇺"); + }); + + it("should return 🇸🇬 for ap-southeast-1 (Singapore)", () => { + expect(getRegionFlag("ap-southeast-1")).toBe("🇸🇬"); + }); + + it("should return empty string for '-' (unknown/no region)", () => { + expect(getRegionFlag("-")).toBe(""); + }); + + it("should return empty string for empty string", () => { + expect(getRegionFlag("")).toBe(""); + }); +}); diff --git a/ui/lib/region-flags.ts b/ui/lib/region-flags.ts index 1b844ffcf0..26b1ea23b0 100644 --- a/ui/lib/region-flags.ts +++ b/ui/lib/region-flags.ts @@ -56,8 +56,10 @@ const REGION_FLAG_RULES: [RegExp, string][] = [ /\bap[-_]?south[-_]?1|india|centralindia|southindia|westindia|mumbai|hyderabad/i, "🇮🇳", ], - // Hong Kong - [/\bap[-_]?east[-_]?1|hongkong/i, "🇭🇰"], + // Taiwan — GCP asia-east1 (must come BEFORE Hong Kong rule) + [/\basia[-_]east[-_]?1\b/i, "🇹🇼"], + // Hong Kong — GCP asia-east2 (ap-east-1 is AWS HK) + [/\bap[-_]?east[-_]?1|\basia[-_]east[-_]?2\b|hongkong/i, "🇭🇰"], // Indonesia [/\bap[-_]?southeast[-_]?3|indonesia|jakarta/i, "🇮🇩"], // China