feat(ui): add batch apply pattern to Findings filters#10388
feat(ui): add batch apply pattern to Findings filters#10388Alan-TheGentleman wants to merge 7 commits intomasterfrom
Conversation
- Add useFilterBatch hook for two-state filter management (pending vs applied) - Add FilterSummaryStrip component showing pending selections as removable chips - Add ApplyFiltersButton with change detection and discard action - Add batch mode to DataTableFilterCustom with backward-compatible mode prop - Wire all Findings filters (including provider/account) through batch mode - Remove cross-filter dependency logic between provider type and accounts - Remove scan option narrowing from useRelatedFilters in Findings view - Add clearAll function for complete pending state reset - Add 47 unit tests covering hook, components, and batch integration
|
✅ All necessary |
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
🔒 Container Security ScanImage: ✅ No Vulnerabilities DetectedThe container image passed all security checks. No known CVEs were found.📋 Resources:
|
- Remove dead props from FindingsFiltersProps (providerIds, providerDetails, completedScans) - Make muted filter fully participate in batch flow (remove from EXCLUDED_FROM_BATCH) - Remove eslint-disable and use searchParams as proper useEffect dependency - Hide ClearFiltersButton when both pending and URL filter counts are zero - Simplify handleChipRemove by removing redundant key normalization - Make useFilterBatch reusable with defaultParams option (remove hardcoded FINDINGS_PATH) - Simplify CustomDatePicker by deriving date from valueProp in batch mode - Extract STATUS_DISPLAY as top-level const alongside PROVIDER_TYPE_DISPLAY - Use const object pattern for DataTableFilterMode type (DATA_TABLE_FILTER_MODE)
There was a problem hiding this comment.
Code Review
Nice work on the overall architecture. The useFilterBatch hook design with pending/applied separation is clean, the tests are thorough (588 lines for the hook alone with great edge case coverage), and backward compatibility with instant mode is well preserved.
That said, there are a few issues that should be addressed before merging:
1. PROVIDER_TYPE_DISPLAY duplicates the existing PROVIDER_DISPLAY_NAMES
findings-filters.tsx introduces a new PROVIDER_TYPE_DISPLAY mapping, but ui/types/providers.ts already exports PROVIDER_DISPLAY_NAMES and getProviderDisplayName().
Worse, the values diverge:
- PR:
gcp: "GCP"/iac: "IaC" - Existing:
gcp: "Google Cloud"/iac: "Infrastructure as Code"
This will cause the filter summary chips to show different names than the rest of the app (provider selectors, badges, etc.).
Fix: Use getProviderDisplayName() from @/types/providers instead of the local map.
prowler/ui/components/findings/findings-filters.tsx
Lines 62 to 81 in 9574390
2. STATUS_DISPLAY duplicates existing status patterns
findings-filters.tsx introduces a STATUS_DISPLAY map (PASS → "Pass", etc.), but status values and display logic already exist in:
FINDING_STATUSinui/types/components.tsFindingStatusValuesinui/components/ui/table/status-finding-badge.tsx
For consistency, a centralized STATUS_DISPLAY_NAMES should be created (similar to how SEVERITY_DISPLAY_NAMES works in ui/types/severities.ts) and reused here.
prowler/ui/components/findings/findings-filters.tsx
Lines 55 to 59 in 9574390
3. formatFilterValue ignores existing formatting utilities
The formatFilterValue function does a generic "capitalize first letter" fallback, but this produces incorrect results for many filter values (e.g., "iam" → "Iam" instead of "IAM").
The codebase already has proper formatting for each filter type:
getProviderDisplayName()for provider types (@/types/providers)SEVERITY_DISPLAY_NAMESfor severities (@/types/severities) — not handled at all currentlygetCategoryLabel()/getGroupLabel()for categories and groups (@/lib/categories)formatLabel()for generic delimiter-based formatting (@/lib/categories)
The function should dispatch to these existing utilities per filter key, and only fall back to formatLabel() for unknown keys.
prowler/ui/components/findings/findings-filters.tsx
Lines 88 to 100 in 9574390
4. AccountsSelector loses contextual information (UX regression)
The PR removes the dynamic description and empty-state messages:
- const filterDescription =
- selectedTypesList.length > 0
- ? `Showing accounts for ${selectedTypesList.join(", ")} providers`
- : "All connected cloud provider accounts";
+ const filterDescription = "All connected cloud provider accounts";- {selectedTypesList.length > 0
- ? "No accounts available for selected providers"
- : "No connected accounts available"}
+ No connected accounts availableEven in batch mode, the selector has access to selectedValues — it could derive which provider types are selected and show context-appropriate messages. Users losing this feedback is a UX regression.
prowler/ui/app/(prowler)/_overview/_components/accounts-selector.tsx
Lines 108 to 111 in 9574390
5. Misleading comment in applyAll about filter[muted]
In use-filter-batch.ts, the comment in applyAll says:
"Start from the current URL params to preserve non-batch params (search, muted, etc.)"
But filter[muted] is now batch-managed (it's not in EXCLUDED_FROM_BATCH). The "etc." is also misleading since only filter[search] is excluded. This will confuse future developers about which filters are batch-managed vs excluded.
prowler/ui/hooks/use-filter-batch.ts
Line 161 in 9574390
6. Minor: FilterSummaryStrip should use aria-live for dynamic updates
The role="region" is good, but adding aria-live="polite" would ensure screen readers announce when chips are added or removed. Without it, the dynamic chip changes are invisible to assistive technology.
prowler/ui/components/filters/filter-summary-strip.tsx
Lines 48 to 51 in 9574390
- Replace local provider and status label maps with shared utilities - Improve filter value formatting with category/group/provider helpers - Restore contextual account selector messaging without narrowing options - Add aria-live polite announcements to the filter summary strip - Clarify applyAll comment for non-batch URL params
|
@alejandrobailo @pfe-nazaries I addressed all requested review points in the latest commits ( and ).\n\n✅ Done:\n- Replaced local provider/status label maps with shared utilities\n- Updated filter value formatting to use existing helpers (, , category/group formatters)\n- Restored contextual account selector messaging without reintroducing dependent filtering\n- Fixed misleading comment about excluded params\n- Added to summary strip for accessibility\n\nAlso reran lint/tests for touched areas and everything is green.\n\nCould you please take another look? Thanks! |
|
@alejandrobailo @pfe-nazaries quick follow-up (previous comment had formatting issues from shell escaping):\n\nI addressed all requested review points in the latest commits.\n\nDone:\n- Replaced local provider/status label maps with shared utilities\n- Updated filter value formatting to use existing helpers from shared modules\n- Restored contextual account selector messaging without reintroducing dependent filtering\n- Fixed misleading applyAll comment about excluded params\n- Added aria-live="polite" to summary strip for accessibility\n\nI also reran lint/tests for touched areas and everything is green.\n\nCould you please re-review when you have a moment? Thanks! |
- Hide default muted=false from summary chips - Exclude muted=false from pending clear-filters count - Keep inserted_at chip value as ISO date string
|
@pfe-nazaries totally agree in general — I also avoid global barrel files because they can hurt tree-shaking and make ownership less explicit. In this PR I kept the existing domain-scoped barrel under ui/components/filters (it already existed before this change) and only added exports for the new filter components.\n\nSo my approach is: use barrel files only where they are truly needed and make sense (shared domain folders), and avoid broad/global barrels. If you want, we can align on a stricter project-wide rule in a separate refactor. |
|
@alejandrobailo @pfe-nazaries quick update: I pushed another pass addressing the remaining review-thread items.\n\nDone in latest changes:\n- Enforced all-or-nothing batch props with discriminated unions in AccountsSelector, ProviderTypeSelector, CustomDatePicker, and CustomCheckboxMutedFindings\n- Tightened findings filter key typing with an explicit FilterParam union and strict key record typing\n- Simplified ApplyFiltersButton label logic\n- Clarified applyAll comment about excluded params\n\nAlso handled minor UX polish:\n- hide default muted false chip\n- exclude default muted from pending count\n- keep inserted_at chip display as ISO date\n\nLint and tests are green on touched areas.\n\nCould you please re-review when you have a moment? Thanks! |
- 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
- Add batch/instant API contract and derived-state guardrails to prowler-ui - Add coupled optional props rule to TypeScript skill - Add pre-re-review thread hygiene checklist to prowler-pr
|
All inline review threads have now been addressed and resolved with follow-up replies (including the remaining type-safety and API-shape items). Ready for final re-review when you have a moment 🙌 |
Context
Screen.Recording.2026-03-19.at.12.21.12.mov
Implements PROWLER-1228 — Findings Filters Batch Apply with Selection Summary.
Currently, every filter selection in the Findings view triggers an immediate API call to the backend. This causes excessive API calls when configuring multiple filters, poor UX as the table reloads on every change, and unnecessary backend load for intermediate filter states.
Description
Introduces a batch apply pattern for the Findings view filters: users select all desired filters freely without triggering API calls, see a summary of pending selections, and explicitly apply them with a single "Apply Filters" button.
New components:
useFilterBatchhook — manages two-state model (pending vs applied/URL) withsetPending,applyAll,discardAll,clearAll,hasChanges,changeCount,getFilterValueFilterSummaryStrip— horizontal strip below filter bar showing pending selections as removable chips with human-readable labelsApplyFiltersButton— Apply/Discard button pair with change detectionModified components (backward compatible):
DataTableFilterCustom— newmodeprop ("instant"|"batch", default"instant") for opt-in batch behavior. All other views (Scans, Providers, Compliance, Users) continue using instant mode unchangedProviderTypeSelector/AccountsSelector— new optionalonBatchChange+selectedValuesprops for batch participation. Overview page keeps instant-applyClearFiltersButton— newonClear+pendingCountprops for batch-aware clearingCustomDatePicker/CustomCheckboxMutedFindings— batch support via optional propsRemoved:
useRelatedFiltersin Findings viewTests: 47 new unit tests (175 total, all passing)
Steps to review
Provider: AWS,Severity: Critical)Checklist
Community Checklist
UI (if applicable)
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.