fix(ui): finalize reviewer feedback for findings batch filters

- Enforce batch/instant props with discriminated unions in filter components

- Add strict FilterParam typing for findings filter labels

- Simplify apply button label logic and clarify batch hook comments
This commit is contained in:
Alan Buscaglia
2026-03-20 12:53:33 +01:00
parent 1116228fc2
commit 8604215b99
8 changed files with 117 additions and 53 deletions

View File

@@ -50,23 +50,9 @@ const PROVIDER_ICON: Record<ProviderType, ReactNode> = {
openstack: <OpenStackProviderBadge width={18} height={18} />,
};
interface AccountsSelectorProps {
/** Common props shared by both batch and instant modes. */
interface AccountsSelectorBaseProps {
providers: ProviderProps[];
/**
* When provided, called instead of navigating immediately.
* Use this on pages that batch filter changes (e.g. Findings).
* The Overview page omits this prop to keep instant-apply behavior.
*
* @param filterKey - The raw filter key without "filter[]" wrapper, e.g. "provider_id__in"
* @param values - The selected values array
*/
onBatchChange?: (filterKey: string, values: string[]) => void;
/**
* When in batch mode, pass the pending selected values from the parent.
* This allows the component to reflect pending state before Apply is clicked.
* Ignored when `onBatchChange` is not provided (URL-driven mode).
*/
selectedValues?: string[];
/**
* Currently selected provider types (from the pending ProviderTypeSelector state).
* Used only for contextual description/empty-state messaging — does NOT narrow
@@ -75,6 +61,33 @@ interface AccountsSelectorProps {
selectedProviderTypes?: string[];
}
/** Batch mode: caller controls both pending state and notification callback (all-or-nothing). */
interface AccountsSelectorBatchProps extends AccountsSelectorBaseProps {
/**
* Called instead of navigating immediately.
* Use this on pages that batch filter changes (e.g. Findings).
*
* @param filterKey - The raw filter key without "filter[]" wrapper, e.g. "provider_id__in"
* @param values - The selected values array
*/
onBatchChange: (filterKey: string, values: string[]) => void;
/**
* Pending selected values controlled by the parent.
* Reflects pending state before Apply is clicked.
*/
selectedValues: string[];
}
/** Instant mode: URL-driven — neither callback nor controlled value. */
interface AccountsSelectorInstantProps extends AccountsSelectorBaseProps {
onBatchChange?: never;
selectedValues?: never;
}
type AccountsSelectorProps =
| AccountsSelectorBatchProps
| AccountsSelectorInstantProps;
export function AccountsSelector({
providers,
onBatchChange,
@@ -89,7 +102,7 @@ export function AccountsSelector({
const urlSelectedIds = current ? current.split(",").filter(Boolean) : [];
// In batch mode, use the parent-controlled pending values; otherwise, use URL state.
const selectedIds = onBatchChange ? (selectedValues ?? []) : urlSelectedIds;
const selectedIds = onBatchChange ? selectedValues : urlSelectedIds;
const visibleProviders = providers;
// .filter((p) => p.attributes.connection?.connected)

View File

@@ -152,24 +152,38 @@ const PROVIDER_DATA: Record<
},
};
type ProviderTypeSelectorProps = {
/** Common props shared by both batch and instant modes. */
interface ProviderTypeSelectorBaseProps {
providers: ProviderProps[];
}
/** Batch mode: caller controls both pending state and notification callback (all-or-nothing). */
interface ProviderTypeSelectorBatchProps extends ProviderTypeSelectorBaseProps {
/**
* When provided, called instead of navigating immediately.
* Called instead of navigating immediately.
* Use this on pages that batch filter changes (e.g. Findings).
* The Overview page omits this prop to keep instant-apply behavior.
*
* @param filterKey - The raw filter key without "filter[]" wrapper, e.g. "provider_type__in"
* @param values - The selected values array
*/
onBatchChange?: (filterKey: string, values: string[]) => void;
onBatchChange: (filterKey: string, values: string[]) => void;
/**
* When in batch mode, pass the pending selected values from the parent.
* This allows the component to reflect pending state before Apply is clicked.
* Ignored when `onBatchChange` is not provided (URL-driven mode).
* Pending selected values controlled by the parent.
* Reflects pending state before Apply is clicked.
*/
selectedValues?: string[];
};
selectedValues: string[];
}
/** Instant mode: URL-driven — neither callback nor controlled value. */
interface ProviderTypeSelectorInstantProps
extends ProviderTypeSelectorBaseProps {
onBatchChange?: never;
selectedValues?: never;
}
type ProviderTypeSelectorProps =
| ProviderTypeSelectorBatchProps
| ProviderTypeSelectorInstantProps;
export const ProviderTypeSelector = ({
providers,
@@ -185,9 +199,7 @@ export const ProviderTypeSelector = ({
: [];
// In batch mode, use the parent-controlled pending values; otherwise, use URL state.
const selectedTypes = onBatchChange
? (selectedValues ?? [])
: urlSelectedTypes;
const selectedTypes = onBatchChange ? selectedValues : urlSelectedTypes;
const handleMultiValueChange = (values: string[]) => {
if (onBatchChange) {

View File

@@ -33,11 +33,8 @@ export const ApplyFiltersButton = ({
onDiscard,
className,
}: ApplyFiltersButtonProps) => {
const label = hasChanges
? changeCount > 0
? `Apply Filters (${changeCount})`
: "Apply Filters"
: "Apply Filters";
const label =
changeCount > 0 ? `Apply Filters (${changeCount})` : "Apply Filters";
return (
<div className={cn("flex items-center gap-1", className)}>

View File

@@ -11,21 +11,31 @@ const MUTED_FILTER_VALUES = {
INCLUDE: "include",
} as const;
interface CustomCheckboxMutedFindingsProps {
/** Batch mode: caller controls both the checked state and the notification callback (all-or-nothing). */
interface CustomCheckboxMutedFindingsBatchProps {
/**
* Called in batch mode instead of navigating directly.
* Called instead of navigating directly.
* Receives the filter key ("muted") and the string value ("include" or "false").
* When provided, the component does NOT call `navigateWithParams`.
*/
onBatchChange?: (filterKey: string, value: string) => void;
onBatchChange: (filterKey: string, value: string) => void;
/**
* Controlled checked state override for batch mode.
* When provided, this value is used as the checkbox state instead of reading from URL params.
* Controlled checked state from the parent (pending state).
* `true` = include muted, `false` = exclude muted.
* `undefined` defers to URL state while pending state is not yet set.
*/
checked?: boolean;
checked: boolean | undefined;
}
/** Instant mode: URL-driven — neither callback nor controlled value. */
interface CustomCheckboxMutedFindingsInstantProps {
onBatchChange?: never;
checked?: never;
}
type CustomCheckboxMutedFindingsProps =
| CustomCheckboxMutedFindingsBatchProps
| CustomCheckboxMutedFindingsInstantProps;
export const CustomCheckboxMutedFindings = ({
onBatchChange,
checked: checkedProp,

View File

@@ -14,21 +14,30 @@ import {
import { useUrlFilters } from "@/hooks/use-url-filters";
import { cn } from "@/lib/utils";
interface CustomDatePickerProps {
/** Batch mode: caller controls both the pending date value and the notification callback (all-or-nothing). */
interface CustomDatePickerBatchProps {
/**
* Called in batch mode instead of updating the URL directly.
* Receives the filter key and the formatted date string (YYYY-MM-DD).
* When provided, the component does NOT call `updateFilter`.
* Called instead of updating the URL directly.
* Receives the filter key ("inserted_at") and the formatted date string (YYYY-MM-DD).
*/
onBatchChange?: (filterKey: string, value: string) => void;
onBatchChange: (filterKey: string, value: string) => void;
/**
* Controlled value override for batch mode.
* When provided, this value is used as the displayed date instead of reading from URL params.
* Controlled date value from the parent (pending state).
* Expected format: YYYY-MM-DD (or any value parseable by `new Date()`).
*/
value?: string;
value: string | undefined;
}
/** Instant mode: URL-driven — neither callback nor controlled value. */
interface CustomDatePickerInstantProps {
onBatchChange?: never;
value?: never;
}
type CustomDatePickerProps =
| CustomDatePickerBatchProps
| CustomDatePickerInstantProps;
const parseDate = (raw: string | null | undefined): Date | undefined => {
if (!raw) return undefined;
try {

View File

@@ -20,7 +20,7 @@ import { DataTableFilterCustom } from "@/components/ui/table";
import { useFilterBatch } from "@/hooks/use-filter-batch";
import { formatLabel, getCategoryLabel, getGroupLabel } from "@/lib/categories";
import { FilterType, FINDING_STATUS_DISPLAY_NAMES, ScanEntity } from "@/types";
import { DATA_TABLE_FILTER_MODE } from "@/types/filters";
import { DATA_TABLE_FILTER_MODE, FilterParam } from "@/types/filters";
import { getProviderDisplayName, ProviderProps } from "@/types/providers";
import { SEVERITY_DISPLAY_NAMES } from "@/types/severities";
@@ -39,8 +39,10 @@ interface FindingsFiltersProps {
/**
* Maps raw filter param keys (e.g. "filter[severity__in]") to human-readable labels.
* Used to render chips in the FilterSummaryStrip.
* Typed as Record<FilterParam, string> so TypeScript enforces exhaustiveness — any
* addition to FilterParam will cause a compile error here if the label is missing.
*/
const FILTER_KEY_LABELS: Record<string, string> = {
const FILTER_KEY_LABELS: Record<FilterParam, string> = {
"filter[provider_type__in]": "Provider",
"filter[provider_id__in]": "Account",
"filter[severity__in]": "Severity",
@@ -175,7 +177,7 @@ export const FindingsFilters = ({
const filterChips: FilterChip[] = [];
Object.entries(pendingFilters).forEach(([key, values]) => {
if (!values || values.length === 0) return;
const label = FILTER_KEY_LABELS[key] ?? key;
const label = FILTER_KEY_LABELS[key as FilterParam] ?? key;
values.forEach((value) => {
// Do not show a chip for the default muted=false state
if (key === "filter[muted]" && value === "false") return;

View File

@@ -165,7 +165,8 @@ export const useFilterBatch = (
};
const applyAll = () => {
// Start from the current URL params to preserve non-batch params (e.g. filter[search])
// 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.
const params = new URLSearchParams(searchParams.toString());
// Remove all existing batch-managed filter params

View File

@@ -55,3 +55,23 @@ export const DATA_TABLE_FILTER_MODE = {
export type DataTableFilterMode =
(typeof DATA_TABLE_FILTER_MODE)[keyof typeof DATA_TABLE_FILTER_MODE];
/**
* Exhaustive union of all URL filter param keys used in Findings filters.
* Use this instead of `string` to ensure FILTER_KEY_LABELS and other
* param-keyed records stay in sync with the actual filter surface.
*/
export type FilterParam =
| "filter[provider_type__in]"
| "filter[provider_id__in]"
| "filter[severity__in]"
| "filter[status__in]"
| "filter[delta__in]"
| "filter[region__in]"
| "filter[service__in]"
| "filter[resource_type__in]"
| "filter[category__in]"
| "filter[resource_groups__in]"
| "filter[scan__in]"
| "filter[inserted_at]"
| "filter[muted]";