-
Notifications
You must be signed in to change notification settings - Fork 125
feat: Workflow History V2 - Ungrouped Event Table #1109
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
feat: Workflow History V2 - Ungrouped Event Table #1109
Conversation
Signed-off-by: Adhitya Mamallan <[email protected]>
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
This PR implements the basic infrastructure for an ungrouped event table view in Workflow History V2. It creates a new table component that flattens event groups into individual events, sorts them appropriately (non-pending events by ID, pending events by time), and provides the foundation for displaying workflow history events in an ungrouped format.
Key changes:
- Added
WorkflowHistoryUngroupedTablecomponent with virtualized scrolling support - Implemented event sorting logic that handles both pending and non-pending events
- Created placeholder
WorkflowHistoryUngroupedEventcomponent for individual event rendering
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
workflow-history-v2.tsx |
Integrated ungrouped table component with conditional rendering based on view mode |
workflow-history-ungrouped-table.tsx |
Main table component with event flattening, sorting, and virtualized list rendering |
workflow-history-ungrouped-table.types.ts |
Type definitions for table props and ungrouped event information |
workflow-history-ungrouped-table.styles.ts |
Responsive grid-based table header styling |
workflow-history-ungrouped-table.constants.ts |
Grid template columns configuration for 8-column layout |
compare-ungrouped-events.ts |
Sorting logic for ordering non-pending events by ID and pending events by timestamp |
compare-ungrouped-events.test.ts |
Comprehensive test coverage for the sorting function |
workflow-history-ungrouped-table.test.tsx |
Integration tests for the table component with various scenarios |
workflow-history-ungrouped-event.tsx |
Placeholder component that currently renders event data as JSON |
workflow-history-ungrouped-event.types.ts |
Type definitions for individual event props |
workflow-history-ungrouped-event.styles.ts |
Temporary styling for the placeholder event component |
workflow-history-v2.test.tsx |
Updated tests to verify ungrouped table rendering conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { styled } from './workflow-history-ungrouped-event.styles'; | ||
| import { type Props } from './workflow-history-ungrouped-event.types'; | ||
|
|
||
| export default function WorkflowHistoryUngroupedEvent({ eventInfo }: Props) { |
Copilot
AI
Dec 8, 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.
[nitpick] The component receives multiple props (workflowStartTimeMs, decodedPageUrlParams, isExpanded, toggleIsExpanded, animateOnEnter, onReset) but only destructures and uses eventInfo. While this is a placeholder implementation, consider at least destructuring the unused props to avoid ESLint warnings, or using a rest parameter pattern if they're genuinely not needed yet.
| export default function WorkflowHistoryUngroupedEvent({ eventInfo }: Props) { | |
| export default function WorkflowHistoryUngroupedEvent({ | |
| workflowStartTimeMs, | |
| decodedPageUrlParams, | |
| isExpanded, | |
| toggleIsExpanded, | |
| animateOnEnter, | |
| onReset, | |
| eventInfo, | |
| }: Props) { |
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.
Interesting, does this shows a warning ?
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.
Nope, it would show a warning if I implemented Copilot's suggestion though.
I think this is a good argument in favour of having inline "Props" instead of keeping them in a separate file.
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.
How does inline props help here ?
...ws/workflow-history-v2/workflow-history-ungrouped-table/workflow-history-ungrouped-table.tsx
Show resolved
Hide resolved
...ws/workflow-history-v2/workflow-history-ungrouped-table/workflow-history-ungrouped-table.tsx
Show resolved
Hide resolved
...ow-history-v2/workflow-history-ungrouped-table/workflow-history-ungrouped-table.constants.ts
Show resolved
Hide resolved
...kflow-history-v2/workflow-history-ungrouped-table/workflow-history-ungrouped-table.styles.ts
Show resolved
Hide resolved
Signed-off-by: Adhitya Mamallan <[email protected]>
Signed-off-by: Adhitya Mamallan <[email protected]>
| import { styled } from './workflow-history-ungrouped-event.styles'; | ||
| import { type Props } from './workflow-history-ungrouped-event.types'; | ||
|
|
||
| export default function WorkflowHistoryUngroupedEvent({ eventInfo }: Props) { |
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.
Interesting, does this shows a warning ?
| {isUngroupedHistoryViewEnabled ? ( | ||
| <div>WIP: ungrouped table</div> | ||
| <WorkflowHistoryUngroupedTable | ||
| eventGroupsById={filteredEventGroupsById} |
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.
Nit: the name eventGroupsById got me for a second thinking it is a map.
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.
True, I see that we call the Array<[string, Type]> type "Entries" in some other places
Summary
Implement basic ungrouped table with placeholders for ungrouped events
Test plan
Unit tests + ran locally.