Rewrite history page filters to act after event grouping#937
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the history page filters to run on already-grouped events, improving performance by avoiding repeated grouping. It replaces event-level filters with group-level filters, adds new helper functions and tests for filtering by group type/status, and updates fixtures and scroll behavior.
- Apply filters after grouping (group-level filtering)
- Remove old event-level filter logic and associated types
- Introduce
filterGroupsByGroupTypeandfilterGroupsByGroupStatushelpers with unit tests
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/views/workflow-history/workflow-history.types.ts | Simplified WorkflowHistoryFilterConfig to only group filters |
| src/views/workflow-history/workflow-history.tsx | Removed event-level filtering hooks and updated scrolling behavior |
| src/views/workflow-history/workflow-history-filters-type | Added group-type mapping and helper; removed deprecated event helper |
| src/views/workflow-history/workflow-history-filters-status | Renamed status filter helper and updated imports/tests |
| src/views/workflow-history/config/workflow-history-filters.config.ts | Switched config to use group-level filter functions |
| src/views/workflow-history/fixtures/workflow-history-event-groups.ts | Added new event-group fixtures |
src/views/workflow-history/__fixtures__/workflow-history-event-groups.ts
Outdated
Show resolved
Hide resolved
| getIsEventExpanded, | ||
| } = useEventExpansionToggle({ | ||
| visibleEvents: filteredEvents, | ||
| visibleEvents: events, |
There was a problem hiding this comment.
The useEventExpansionToggle hook now receives the unfiltered events array instead of filtered events/groups. This may cause toggles to include events that should be hidden by the active filters.
There was a problem hiding this comment.
This is intentional; it's so that Expand All applies to events that are also being filtered out. Therefore, if the filter is removed, the events will show up again but in the expanded state.
| behavior: 'auto', | ||
| }); | ||
|
|
||
| timelineSectionListRef.current?.scrollToIndex({ | ||
| index: eventGroupIndex, | ||
| align: 'start', | ||
| behavior: 'smooth', | ||
| behavior: 'auto', |
There was a problem hiding this comment.
[nitpick] Multiple scrollToIndex calls inline the string 'auto' for behavior. Consider extracting this into a constant or shared function to reduce duplication and ease future behavior changes.
Summary
Test plan
Updated unit tests + ran locally.
Before:
Screen.Recording.2025-07-02.at.12.34.24.PM.mov
After:
Screen.Recording.2025-07-02.at.12.35.38.PM.mov