-
Notifications
You must be signed in to change notification settings - Fork 126
feat: History Page V2 - event group row in history grouped table #1095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: History Page V2 - event group row in history grouped table #1095
Conversation
f772407 to
b89db0f
Compare
Signed-off-by: Adhitya Mamallan <[email protected]>
Signed-off-by: Adhitya Mamallan <[email protected]>
Signed-off-by: Adhitya Mamallan <[email protected]>
Signed-off-by: Adhitya Mamallan <[email protected]>
Signed-off-by: Adhitya Mamallan <[email protected]>
Signed-off-by: Adhitya Mamallan <[email protected]>
b89db0f to
74fe264
Compare
There was a problem hiding this 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 implements the grouped event view for History v2, introducing a comprehensive event grouping interface with expansion/collapse functionality, duration tracking, and reset capabilities. The changes establish a foundation for displaying workflow history events in a more organized, grouped format.
Key changes:
- Added event group rendering with Panel-based UI including status badges, timestamps, and durations
- Implemented expansion/collapse state management using a custom hook for individual event groups
- Integrated workflow reset functionality with modal support for decision events
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/views/workflow-history-v2/workflow-history-v2.tsx |
Added expansion toggle state management, workflow close time computation, and modal integration for reset functionality |
src/views/workflow-history-v2/workflow-history-grouped-table/workflow-history-grouped-table.types.ts |
Extended props to include workflow execution state, event expansion handlers, and reset callbacks |
src/views/workflow-history-v2/workflow-history-grouped-table/workflow-history-grouped-table.tsx |
Replaced placeholder with WorkflowHistoryEventGroup component and updated default item height for virtualized rendering |
src/views/workflow-history-v2/workflow-history-grouped-table/workflow-history-grouped-table.styles.ts |
Adjusted padding to remove space for expand icon (now hidden) and reduced accordion panel padding |
src/views/workflow-history-v2/workflow-history-grouped-table/workflow-history-grouped-table.constants.ts |
Updated grid template columns to accommodate icon, adjusted column proportions for better layout |
src/views/workflow-history-v2/workflow-history-grouped-table/__tests__/workflow-history-grouped-table.test.tsx |
Updated tests to mock WorkflowHistoryEventGroup, removed grid layout test, added new required props to setup |
src/views/workflow-history-v2/workflow-history-event-group/workflow-history-event-group.types.ts |
Defined props interface for event group component with workflow state and interaction handlers |
src/views/workflow-history-v2/workflow-history-event-group/workflow-history-event-group.tsx |
Created event group component with header displaying status, time, duration, and reset button; panel expansion based on event state |
src/views/workflow-history-v2/workflow-history-event-group/workflow-history-event-group.styles.ts |
Defined styled components and overrides for event group panel with grid layout and animation support |
src/views/workflow-history-v2/workflow-history-event-group/helpers/get-event-group-filtering-type.ts |
Implemented helper to determine filtering type based on event group type and attributes |
src/views/workflow-history-v2/workflow-history-event-group/helpers/__tests__/get-event-group-filtering-type.test.ts |
Added comprehensive test coverage for filtering type determination logic |
src/views/workflow-history-v2/workflow-history-event-group/__tests__/workflow-history-event-group.test.tsx |
Created tests for event group rendering, expansion, reset functionality, and loading states |
src/views/workflow-history-v2/workflow-history-event-group-duration/workflow-history-event-group-duration.types.ts |
Defined props for duration component including start/close times and workflow state |
src/views/workflow-history-v2/workflow-history-event-group-duration/workflow-history-event-group-duration.tsx |
Implemented duration component with live updates for ongoing events and conditional rendering |
src/views/workflow-history-v2/workflow-history-event-group-duration/helpers/get-formatted-events-duration.ts |
Created helper to format duration between timestamps with configurable millisecond display |
src/views/workflow-history-v2/workflow-history-event-group-duration/helpers/__tests__/get-formatted-events-duration.test.ts |
Added tests for duration formatting with various time scenarios |
src/views/workflow-history-v2/workflow-history-event-group-duration/__tests__/workflow-history-event-group-duration.test.tsx |
Created tests for duration component behavior including live updates and conditional rendering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/views/workflow-history-v2/workflow-history-event-group/workflow-history-event-group.tsx
Outdated
Show resolved
Hide resolved
src/views/workflow-history-v2/workflow-history-event-group/workflow-history-event-group.tsx
Outdated
Show resolved
Hide resolved
...views/workflow-history-v2/workflow-history-event-group/workflow-history-event-group.types.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Adhitya Mamallan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| .flat(1) | ||
| .map(({ history }) => history?.events || []) | ||
| .flat(1) |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The allEventIds computation uses multiple .flat() and .map() operations which creates intermediate arrays. Consider using .flatMap() to reduce iterations: (result?.pages || []).flatMap(({ history }) => history?.events || []).map(({ eventId }) => eventId).
| .flat(1) | |
| .map(({ history }) => history?.events || []) | |
| .flat(1) | |
| .flatMap(({ history }) => history?.events || []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that useExpansionToggle needs the a list of ids but this can influence the API to change from accepting array of ids to accepting items as is with a key for getting the identifier, this can save the events computation that runs frequently after receiving event pages.
| itemContent={(_, [__, group]) => <div>{JSON.stringify(group)}</div>} | ||
| itemContent={(_, [groupId, group]) => ( | ||
| <WorkflowHistoryEventGroup | ||
| key={groupId} |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key prop on line 56 is unnecessary within the itemContent function. React Virtuoso manages keys internally for virtualized items. This prop has no effect and should be removed.
| key={groupId} |
| statusReady={!showLoadingMoreEvents} | ||
| size="small" | ||
| /> | ||
| {eventsMetadata.at(-1)?.label} |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing eventsMetadata.at(-1) assumes the last metadata entry is always relevant. Consider adding a comment explaining why the last entry is used, or use a more descriptive variable name like lastEventLabel to clarify intent.
| const workflowEnded = | ||
| workflowIsArchived || | ||
| workflowCloseStatus !== 'WORKFLOW_EXECUTION_CLOSE_STATUS_INVALID'; |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison against the literal string 'WORKFLOW_EXECUTION_CLOSE_STATUS_INVALID' is fragile. Consider importing and using the enum value: WorkflowExecutionCloseStatus.WORKFLOW_EXECUTION_CLOSE_STATUS_INVALID to ensure type safety and catch refactoring issues.
| const duration = formatDuration( | ||
| { | ||
| seconds: seconds.toString(), | ||
| nanos: (durationObj.asMilliseconds() - seconds * 1000) * 1000000, |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 1000000 (conversion from milliseconds to nanoseconds) should be extracted to a named constant like MS_TO_NANOS = 1_000_000 for better readability and maintainability.
|
I have few questions on UI:
|
| }); | ||
|
|
||
| it('should render event groups data', () => { | ||
| it('should render event group labels', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing the mock, the removed test was testing that group was passed to the component which makes more sense.
| import getFormattedEventsDuration from './helpers/get-formatted-events-duration'; | ||
| import { type Props } from './workflow-history-event-group-duration.types'; | ||
|
|
||
| export default function WorkflowHistoryEventGroupDuration({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to duplicate this component? I expect a single flag returning the value without the badge (only text) would be sufficient.
| .flat(1) | ||
| .map(({ history }) => history?.events || []) | ||
| .flat(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that useExpansionToggle needs the a list of ids but this can influence the API to change from accepting array of ids to accepting items as is with a key for getting the identifier, this can save the events computation that runs frequently after receiving event pages.
| (typeof filterConfig === 'function' && filterConfig(group)) || | ||
| group.groupType === filterConfig | ||
| ) { | ||
| return eventGroupFilterType as WorkflowHistoryEventFilteringType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we casting here?
| } | ||
| } | ||
|
|
||
| return 'WORKFLOW'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are is this filtering by WORKFLOW by default ? shouldn't it be null?
Summary
useExpansionTogglehookTest plan
Added unit tests + ran locally.
Basic table
With hover
Expanded
Reset Workflow form