Skip to content

Improve advanced filter UX: always-visible X buttons, click-to-edit, …

19df7d2
Select commit
Loading
Failed to load commit list.
Open

feat: Add advanced filters panel to incident list #126

Improve advanced filter UX: always-visible X buttons, click-to-edit, …
19df7d2
Select commit
Loading
Failed to load commit list.
@sentry/warden / warden: code-review completed Apr 9, 2026 in 5m 39s

2 issues

code-review: Found 2 issues (2 medium)

Medium

"Clear all filters" clears status filter which is not reflected in the filter count badge - `frontend/src/routes/components/filters/FilterTrigger.tsx:17-23`

The activeCount badge only counts advanced filters (severity, service_tier, etc. plus date filters) but excludes the status filter. However, the "Clear all filters" button uses search: {} which clears ALL URL params including status. This creates a UX inconsistency: if a user is viewing "Closed" incidents with severity=P0, the badge shows "1" but clicking "Clear all filters" also resets the status view from "Closed" to "Active" (the default). Users may not expect their status selection to be reset when the badge doesn't indicate it as an active filter.

Missing dependency in useEffect causes stale closure - `frontend/src/routes/components/filters/useFilterEditor.ts:83-94`

The useEffect on line 83-94 that handles the Escape key listener includes close in its dependency array. However, close is created with useCallback which includes onClose in its dependency array (line 68). If onClose changes (e.g., a new callback is passed), close will be recreated, causing this useEffect to re-register the event listener on every render where onClose changes. This could lead to memory leaks or multiple handlers if the parent component doesn't memoize onClose.


Duration: 5m 37s · Tokens: 1.1M in / 15.1k out · Cost: $2.24 (+merge: $0.00)

Annotations

Check warning on line 23 in frontend/src/routes/components/filters/FilterTrigger.tsx

See this annotation in the file changed.

@sentry-warden sentry-warden / warden: code-review

"Clear all filters" clears status filter which is not reflected in the filter count badge

The `activeCount` badge only counts advanced filters (severity, service_tier, etc. plus date filters) but excludes the status filter. However, the "Clear all filters" button uses `search: {}` which clears ALL URL params including `status`. This creates a UX inconsistency: if a user is viewing "Closed" incidents with severity=P0, the badge shows "1" but clicking "Clear all filters" also resets the status view from "Closed" to "Active" (the default). Users may not expect their status selection to be reset when the badge doesn't indicate it as an active filter.

Check warning on line 94 in frontend/src/routes/components/filters/useFilterEditor.ts

See this annotation in the file changed.

@sentry-warden sentry-warden / warden: code-review

Missing dependency in useEffect causes stale closure

The useEffect on line 83-94 that handles the Escape key listener includes `close` in its dependency array. However, `close` is created with `useCallback` which includes `onClose` in its dependency array (line 68). If `onClose` changes (e.g., a new callback is passed), `close` will be recreated, causing this useEffect to re-register the event listener on every render where `onClose` changes. This could lead to memory leaks or multiple handlers if the parent component doesn't memoize `onClose`.