From b01fcc6cb2a09cc634517921e49e0cd3371aa085 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Wed, 25 Mar 2026 13:20:10 +0100 Subject: [PATCH] fix(ui): refine filter clear and undo behavior in Findings page (#10446) --- ui/CHANGELOG.md | 8 + .../filters/apply-filters-button.test.tsx | 13 +- .../filters/apply-filters-button.tsx | 12 +- .../filters/clear-filters-button.tsx | 5 +- ui/components/findings/findings-filters.tsx | 21 +- ui/hooks/use-filter-batch.test.ts | 260 ++++++++++++++++++ ui/hooks/use-filter-batch.ts | 63 ++++- 7 files changed, 357 insertions(+), 25 deletions(-) diff --git a/ui/CHANGELOG.md b/ui/CHANGELOG.md index 58394198ed..74441a54a1 100644 --- a/ui/CHANGELOG.md +++ b/ui/CHANGELOG.md @@ -2,6 +2,14 @@ All notable changes to the **Prowler UI** are documented in this file. +## [1.23.0] (Prowler UNRELEASED) + +### 🐞 Fixed + +- Clear Filters now resets all filters including muted findings and auto-applies, Clear all in pills only removes pill-visible sub-filters, and the discard icon is now an Undo text button [(#10446)](https://github.com/prowler-cloud/prowler/pull/10446) + +--- + ## [1.22.0] (Prowler v5.22.0) ### 🚀 Added diff --git a/ui/components/filters/apply-filters-button.test.tsx b/ui/components/filters/apply-filters-button.test.tsx index 31f601c0de..ed28f23c56 100644 --- a/ui/components/filters/apply-filters-button.test.tsx +++ b/ui/components/filters/apply-filters-button.test.tsx @@ -5,7 +5,6 @@ import { describe, expect, it, vi } from "vitest"; // Mock lucide-react to avoid SVG rendering issues in jsdom vi.mock("lucide-react", () => ({ Check: () => , - X: () => , })); // Mock @/components/shadcn to avoid next-auth import chain @@ -74,7 +73,7 @@ describe("ApplyFiltersButton", () => { expect(applyButton).toBeDisabled(); }); - it("should NOT render the discard (X) button when there are no changes", () => { + it("should NOT render the Undo button when there are no changes", () => { // Given / When render( { // Then expect( screen.queryByRole("button", { - name: /discard/i, + name: /undo/i, }), ).not.toBeInTheDocument(); }); @@ -166,7 +165,7 @@ describe("ApplyFiltersButton", () => { ).toBeInTheDocument(); }); - it("should render the discard (X) button", () => { + it("should render the Undo button", () => { // Given / When render( { // Then expect( - screen.getByRole("button", { name: /discard pending filter changes/i }), + screen.getByRole("button", { name: /undo pending filter changes/i }), ).toBeInTheDocument(); }); }); @@ -237,7 +236,7 @@ describe("ApplyFiltersButton", () => { // ── onDiscard interaction ──────────────────────────────────────────────── describe("onDiscard", () => { - it("should call onDiscard when the Discard button is clicked", async () => { + it("should call onDiscard when the Undo button is clicked", async () => { // Given const user = userEvent.setup(); const onApply = vi.fn(); @@ -254,7 +253,7 @@ describe("ApplyFiltersButton", () => { // When await user.click( - screen.getByRole("button", { name: /discard pending filter changes/i }), + screen.getByRole("button", { name: /undo pending filter changes/i }), ); // Then diff --git a/ui/components/filters/apply-filters-button.tsx b/ui/components/filters/apply-filters-button.tsx index be0fa183a7..8a338f8620 100644 --- a/ui/components/filters/apply-filters-button.tsx +++ b/ui/components/filters/apply-filters-button.tsx @@ -1,6 +1,6 @@ "use client"; -import { Check, X } from "lucide-react"; +import { Check } from "lucide-react"; import { Button } from "@/components/shadcn"; import { cn } from "@/lib/utils"; @@ -12,7 +12,7 @@ export interface ApplyFiltersButtonProps { changeCount: number; /** Called when the user clicks "Apply Filters" */ onApply: () => void; - /** Called when the user clicks the discard (X) action */ + /** Called when the user clicks the discard (Undo) action */ onDiscard: () => void; /** Optional extra class names for the outer wrapper */ className?: string; @@ -23,7 +23,7 @@ export interface ApplyFiltersButtonProps { * * - Shows the count of pending changes when `hasChanges` is true. * - The apply button is disabled (and visually muted) when there are no changes. - * - The discard (X) button only appears when there are pending changes. + * - The Undo button only appears when there are pending changes. * - Uses Prowler's shadcn `Button` component. */ export const ApplyFiltersButton = ({ @@ -52,11 +52,11 @@ export const ApplyFiltersButton = ({ {hasChanges && ( )} diff --git a/ui/components/filters/clear-filters-button.tsx b/ui/components/filters/clear-filters-button.tsx index 0cc586cc70..550a6b6711 100644 --- a/ui/components/filters/clear-filters-button.tsx +++ b/ui/components/filters/clear-filters-button.tsx @@ -2,7 +2,6 @@ import { XCircle } from "lucide-react"; import { usePathname, useRouter, useSearchParams } from "next/navigation"; -import { useCallback } from "react"; import { Button } from "../shadcn"; @@ -52,7 +51,7 @@ export const ClearFiltersButton = ({ const filterCount = activeFilters.length; // Clear all filters except excluded ones (muted, search) - const clearFiltersPreservingExcluded = useCallback(() => { + const clearFiltersPreservingExcluded = () => { const params = new URLSearchParams(searchParams.toString()); Array.from(params.keys()).forEach((key) => { if ( @@ -64,7 +63,7 @@ export const ClearFiltersButton = ({ }); params.delete("page"); router.push(`${pathname}?${params.toString()}`, { scroll: false }); - }, [router, searchParams, pathname]); + }; // In batch mode: use pendingCount if provided; otherwise fall back to URL count. // In instant mode: always use URL count. diff --git a/ui/components/findings/findings-filters.tsx b/ui/components/findings/findings-filters.tsx index 9d7a7225ba..36a3c523a9 100644 --- a/ui/components/findings/findings-filters.tsx +++ b/ui/components/findings/findings-filters.tsx @@ -118,7 +118,8 @@ export const FindingsFilters = ({ setPending, applyAll, discardAll, - clearAll, + clearAndApply, + clearKeys, hasChanges, changeCount, getFilterValue, @@ -198,6 +199,20 @@ export const FindingsFilters = ({ setPending(filterKey, nextValues); }; + // Handler for clearing all chips: clears only the filter keys visible as chips, + // without touching provider/account selectors. + const PROVIDER_KEYS = new Set([ + "filter[provider_type__in]", + "filter[provider_id__in]", + ]); + + const handleClearAllChips = () => { + const chipKeys = Array.from(new Set(filterChips.map((c) => c.key))).filter( + (k) => !PROVIDER_KEYS.has(k), + ); + clearKeys(chipKeys); + }; + // Derive pending muted state for the checkbox. // Note: "filter[muted]" participates in batch mode — applyAll includes it // when present in pending state, and the defaultParams option ensures @@ -252,7 +267,7 @@ export const FindingsFilters = ({ )} { if (!values || values.length === 0) return false; @@ -279,7 +294,7 @@ export const FindingsFilters = ({ {/* Expandable filters section */} diff --git a/ui/hooks/use-filter-batch.test.ts b/ui/hooks/use-filter-batch.test.ts index 00ac517719..d921b0c4e8 100644 --- a/ui/hooks/use-filter-batch.test.ts +++ b/ui/hooks/use-filter-batch.test.ts @@ -585,4 +585,264 @@ describe("useFilterBatch", () => { ]); }); }); + + // ── clearAndApply ────────────────────────────────────────────────────────── + + describe("clearAndApply", () => { + it("should clear all batch-managed filters and push URL immediately", () => { + // Given + setSearchParams({ + "filter[severity__in]": "critical", + "filter[status__in]": "FAIL", + }); + const { result } = renderHook(() => useFilterBatch()); + + // Pre-condition — pending is loaded from URL + expect(result.current.pendingFilters["filter[severity__in]"]).toEqual([ + "critical", + ]); + expect(result.current.pendingFilters["filter[status__in]"]).toEqual([ + "FAIL", + ]); + + // When + act(() => { + result.current.clearAndApply(); + }); + + // Then — pending is empty + expect(result.current.pendingFilters).toEqual({}); + + // And router.push was called + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + + // The pushed URL must NOT contain severity or status + expect(calledUrl).not.toContain("severity"); + expect(calledUrl).not.toContain("status"); + }); + + it("should apply defaultParams when clearing", () => { + // Given + setSearchParams({ "filter[severity__in]": "critical" }); + const { result } = renderHook(() => + useFilterBatch({ defaultParams: { "filter[muted]": "false" } }), + ); + + // When + act(() => { + result.current.clearAndApply(); + }); + + // Then — pushed URL contains the defaultParam + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("filter%5Bmuted%5D=false"); + }); + + it("should preserve filter[search] (excluded from batch)", () => { + // Given — URL has both a search param (excluded) and a batch filter + mockSearchParamsValue = new URLSearchParams({ + "filter[search]": "test", + "filter[severity__in]": "critical", + }); + const { result } = renderHook(() => useFilterBatch()); + + // When + act(() => { + result.current.clearAndApply(); + }); + + // Then — search param is preserved; severity is gone + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("filter%5Bsearch%5D=test"); + expect(calledUrl).not.toContain("severity"); + }); + + it("should reset pagination to page 1", () => { + // Given — URL already has a page param + mockSearchParamsValue = new URLSearchParams({ + "filter[severity__in]": "critical", + page: "3", + }); + const { result } = renderHook(() => useFilterBatch()); + + // When + act(() => { + result.current.clearAndApply(); + }); + + // Then — page is reset to 1 + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("page=1"); + }); + }); + + // ── clearKeys ───────────────────────────────────────────────────────────── + + describe("clearKeys", () => { + it("should remove only specified keys and push URL", () => { + // Given + setSearchParams({}); + const { result } = renderHook(() => useFilterBatch()); + + act(() => { + result.current.setPending("filter[severity__in]", ["critical"]); + result.current.setPending("filter[status__in]", ["FAIL"]); + result.current.setPending("filter[region__in]", ["us-east-1"]); + }); + + // When + act(() => { + result.current.clearKeys(["filter[severity__in]"]); + }); + + // Then — severity is gone; status and region remain + expect( + result.current.pendingFilters["filter[severity__in]"], + ).toBeUndefined(); + expect(result.current.pendingFilters["filter[status__in]"]).toEqual([ + "FAIL", + ]); + expect(result.current.pendingFilters["filter[region__in]"]).toEqual([ + "us-east-1", + ]); + + // And the pushed URL contains the remaining keys but not severity + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("status"); + expect(calledUrl).toContain("region"); + expect(calledUrl).not.toContain("severity"); + }); + + it("should accept keys without 'filter[' prefix", () => { + // Given + setSearchParams({}); + const { result } = renderHook(() => useFilterBatch()); + + act(() => { + result.current.setPending("filter[severity__in]", ["critical"]); + }); + + // When — pass key without filter[] wrapper + act(() => { + result.current.clearKeys(["severity__in"]); + }); + + // Then — severity is cleared + expect( + result.current.pendingFilters["filter[severity__in]"], + ).toBeUndefined(); + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).not.toContain("severity"); + }); + + it("should preserve provider/account keys not in the cleared list", () => { + // Given + setSearchParams({}); + const { result } = renderHook(() => useFilterBatch()); + + act(() => { + result.current.setPending("filter[provider_type__in]", ["aws"]); + result.current.setPending("filter[severity__in]", ["critical"]); + result.current.setPending("filter[status__in]", ["FAIL"]); + }); + + // When — clear only severity and status; leave provider untouched + act(() => { + result.current.clearKeys([ + "filter[severity__in]", + "filter[status__in]", + ]); + }); + + // Then — provider_type__in is still in pending + expect( + result.current.pendingFilters["filter[provider_type__in]"], + ).toEqual(["aws"]); + expect( + result.current.pendingFilters["filter[severity__in]"], + ).toBeUndefined(); + expect( + result.current.pendingFilters["filter[status__in]"], + ).toBeUndefined(); + + // And the pushed URL retains provider but not severity/status + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("provider_type__in"); + expect(calledUrl).not.toContain("severity"); + expect(calledUrl).not.toContain("status__in"); + }); + + it("should apply defaultParams after clearing", () => { + // Given + setSearchParams({}); + const { result } = renderHook(() => + useFilterBatch({ defaultParams: { "filter[muted]": "false" } }), + ); + + act(() => { + result.current.setPending("filter[severity__in]", ["critical"]); + }); + + // When + act(() => { + result.current.clearKeys(["filter[severity__in]"]); + }); + + // Then — defaultParam is present in the pushed URL + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("filter%5Bmuted%5D=false"); + }); + + it("should reset pagination to page 1", () => { + // Given — URL already has a page param + mockSearchParamsValue = new URLSearchParams({ + "filter[severity__in]": "critical", + page: "5", + }); + const { result } = renderHook(() => useFilterBatch()); + + // When + act(() => { + result.current.clearKeys(["filter[severity__in]"]); + }); + + // Then — page is reset to 1 + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("page=1"); + }); + + it("should handle empty keys array gracefully", () => { + // Given + setSearchParams({}); + const { result } = renderHook(() => useFilterBatch()); + + act(() => { + result.current.setPending("filter[severity__in]", ["critical"]); + }); + + // When — clear no keys at all + act(() => { + result.current.clearKeys([]); + }); + + // Then — pending is unchanged + expect(result.current.pendingFilters["filter[severity__in]"]).toEqual([ + "critical", + ]); + + // And router.push was still called (navigates with current state) + expect(mockPush).toHaveBeenCalledTimes(1); + const calledUrl: string = mockPush.mock.calls[0][0]; + expect(calledUrl).toContain("severity"); + }); + }); }); diff --git a/ui/hooks/use-filter-batch.ts b/ui/hooks/use-filter-batch.ts index 45cfe4eae0..b777d6dbdd 100644 --- a/ui/hooks/use-filter-batch.ts +++ b/ui/hooks/use-filter-batch.ts @@ -32,6 +32,19 @@ export interface UseFilterBatchReturn { * Includes provider/account keys and all batch-managed filter keys. */ clearAll: () => void; + /** + * Clear all batch-managed filters and immediately navigate (router.push) + * with defaultParams applied. Equivalent to clearAll() + applyAll() but + * avoids the async state gap between the two calls. + */ + clearAndApply: () => void; + /** + * Clear only the specified filter keys from pending state and immediately + * navigate (router.push) with the remaining pending filters + defaultParams. + * Used by "Clear all" in the pills strip to remove only pill-visible filters + * without touching provider/account selectors. + */ + clearKeys: (keys: string[]) => void; /** Remove a single filter key from pending state */ removePending: (key: string) => void; /** Whether pending state differs from the current URL */ @@ -164,20 +177,19 @@ export const useFilterBatch = ( }); }; - const applyAll = () => { - // Start from the current URL params to preserve non-batch params. - // Only filter[search] is excluded from batch management and preserved from the URL as-is. + /** Private helper — builds URLSearchParams from a pending state and pushes. */ + const buildAndPush = (nextPending: PendingFilters) => { const params = new URLSearchParams(searchParams.toString()); - // Remove all existing batch-managed filter params + // Remove all batch-managed filter params Array.from(params.keys()).forEach((key) => { if (key.startsWith("filter[") && !EXCLUDED_FROM_BATCH.includes(key)) { params.delete(key); } }); - // Write the pending state - Object.entries(pendingFilters).forEach(([key, values]) => { + // Re-apply the given pending filters + Object.entries(nextPending).forEach(([key, values]) => { const nonEmpty = values.filter(Boolean); if (nonEmpty.length > 0) { params.set(key, nonEmpty.join(",")); @@ -203,6 +215,10 @@ export const useFilterBatch = ( router.push(targetUrl, { scroll: false }); }; + const applyAll = () => { + buildAndPush(pendingFilters); + }; + const discardAll = () => { const applied = deriveAppliedFromUrl( new URLSearchParams(searchParams.toString()), @@ -230,6 +246,39 @@ export const useFilterBatch = ( setPendingFilters({}); }; + /** + * Clears ALL batch-managed filters and immediately navigates (router.push). + * + * Works around the async gap between clearAll() + applyAll(): instead of + * setting pending to `{}` and then calling applyAll() (which would still + * read the old pendingFilters from the closure), this function builds the + * target URL directly from an empty pending state and pushes it in one step. + * defaultParams (e.g. filter[muted]=false) are applied as usual. + */ + const clearAndApply = () => { + setPendingFilters({}); + buildAndPush({}); + }; + + /** + * Removes only the specified filter keys from pending state and immediately + * navigates (router.push) with the remaining filters + defaultParams. + * + * Used by the pills strip "Clear all" to remove pill-visible filters (severity, + * status, delta, region, service, etc.) without touching provider/account selectors. + */ + const clearKeys = (keys: string[]) => { + const normalizedKeys = keys.map((k) => + k.startsWith("filter[") ? k : `filter[${k}]`, + ); + const nextPending: PendingFilters = { ...pendingFilters }; + normalizedKeys.forEach((k) => { + delete nextPending[k]; + }); + setPendingFilters(nextPending); + buildAndPush(nextPending); + }; + const getFilterValue = (key: string): string[] => { const filterKey = key.startsWith("filter[") ? key : `filter[${key}]`; return pendingFilters[filterKey] ?? []; @@ -249,6 +298,8 @@ export const useFilterBatch = ( applyAll, discardAll, clearAll, + clearAndApply, + clearKeys, removePending, hasChanges, changeCount,