Skip to content

Rewrite history page filters to act after event grouping#937

Merged
adhityamamallan merged 4 commits intocadence-workflow:masterfrom
adhityamamallan:filter-only-event-groups
Jul 2, 2025
Merged

Rewrite history page filters to act after event grouping#937
adhityamamallan merged 4 commits intocadence-workflow:masterfrom
adhityamamallan:filter-only-event-groups

Conversation

@adhityamamallan
Copy link
Member

@adhityamamallan adhityamamallan commented Jul 2, 2025

Summary

  • Rewrite workflow-history to allow filtering only event groups, to avoid the (expensive) event grouping operation every time filters are applied.
  • Modify existing filter functions to work with event groups
  • Add unit test coverage for filtering event groups by type
  • Misc:
    • Add fixtures for more types of event groups
    • Change Virtuoso scroll-to-event behaviour to automatically snap to a selected event instead of scrolling to it
    • Fix typo in fixture file name

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

@adhityamamallan adhityamamallan requested a review from Copilot July 2, 2025 11:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 filterGroupsByGroupType and filterGroupsByGroupStatus helpers 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

getIsEventExpanded,
} = useEventExpansionToggle({
visibleEvents: filteredEvents,
visibleEvents: events,
Copy link

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +388 to +394
behavior: 'auto',
});

timelineSectionListRef.current?.scrollToIndex({
index: eventGroupIndex,
align: 'start',
behavior: 'smooth',
behavior: 'auto',
Copy link

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copilot uses AI. Check for mistakes.
@adhityamamallan adhityamamallan merged commit 1b18bc4 into cadence-workflow:master Jul 2, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants