Repository navigation
feat: add repeat policy child runs for the timeline - #2550
Conversation
📝 WalkthroughWalkthrough
ChangesTimeline sub-run expansion and navigation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant TimelineChart
participant TimelineBar
participant onOpenSubRun
TimelineChart->>TimelineBar: Render child run with resolved opener
User->>TimelineBar: Click or press Enter
TimelineBar->>onOpenSubRun: Open sub-DAG name and run ID
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/src/features/dags/components/visualization/TimelineChart.tsx`:
- Around line 303-337: Complete the custom button behavior on the openable
timeline bar: add an aria-label containing the child DAG name and run ID, update
handleKeyDown to activate on both Enter and Space while preventing Space
scrolling, and add a visible focus style when the bar is focused. Extend the
relevant tests to verify the accessible name and Space-key activation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa02467c-5e5e-4757-9ac0-236c7f79508c
📒 Files selected for processing (4)
ui/src/features/dags/components/visualization/TimelineChart.tsxui/src/features/dags/components/visualization/__tests__/TimelineChart.test.tsxui/src/features/dags/components/visualization/__tests__/timelineItems.test.tsui/src/features/dags/components/visualization/timelineItems.ts
| function handleKeyDown(event: KeyboardEvent<HTMLDivElement>) { | ||
| onKeyDown?.(event); | ||
| if (!canOpen || event.key !== 'Enter') { | ||
| return; | ||
| } | ||
| event.preventDefault(); | ||
| if (!openSubRun || !item.dagRunId) { | ||
| return; | ||
| } | ||
| openSubRun({ | ||
| name: item.dagName || item.parentStepName || item.label, | ||
| dagRunId: item.dagRunId, | ||
| }); | ||
| } | ||
|
|
||
| return ( | ||
| <div | ||
| ref={ref} | ||
| data-testid={`timeline-bar-${item.id}`} | ||
| {...props} | ||
| role={canOpen ? 'button' : undefined} | ||
| tabIndex={canOpen ? 0 : undefined} | ||
| className={`absolute h-5 rounded transition-opacity hover:opacity-80 ${ | ||
| canOpen ? 'cursor-pointer' : '' | ||
| } ${isActive ? 'animate-pulse' : ''} ${className ?? ''}`} | ||
| style={{ | ||
| left: `calc(${leftPercent}% + 130px)`, | ||
| width: `calc(${Math.max(widthPercent, 0.5)}% - 130px)`, | ||
| minWidth: '4px', | ||
| backgroundColor: colors.bg, | ||
| borderLeft: `2px solid ${colors.border}`, | ||
| ...style, | ||
| }} | ||
| onClick={canOpen ? handleClick : onClick} | ||
| onKeyDown={canOpen ? handleKeyDown : onKeyDown} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement complete custom button behavior.
The openable sub-DAG bar has no accessible name. Screen readers announce only “button”.
The handler activates only on Enter. A custom role="button" control must also support Space activation and show a visible focus state.
Add an aria-label that includes the child DAG name and run ID. Handle Space according to button behavior. Add tests for the accessible name and Space activation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/features/dags/components/visualization/TimelineChart.tsx` around lines
303 - 337, Complete the custom button behavior on the openable timeline bar: add
an aria-label containing the child DAG name and run ID, update handleKeyDown to
activate on both Enter and Space while preventing Space scrolling, and add a
visible focus style when the bar is focused. Extend the relevant tests to verify
the accessible name and Space-key activation.
| widthPercent={widthPercent} | ||
| colors={colors} | ||
| isActive={isActive} | ||
| openSubRun={openSubRun} |
There was a problem hiding this comment.
useOpenSubRun() can return null here, but the prop does not accept it, so typecheck fails. Passing undefined should do it:
| openSubRun={openSubRun} | |
| openSubRun={openSubRun ?? undefined} |
| const repeated = node.subRunsRepeated || []; | ||
| if (repeated.length === 0) { | ||
| return []; | ||
| } | ||
| return [...repeated, ...(node.subRuns || [])]; |
There was a problem hiding this comment.
I think this misses the first run. subRunsRepeated is empty until run two starts, so if the policy stops after one run, its child never appears. Can we key this off repeatPolicy instead?
| const repeated = node.subRunsRepeated || []; | |
| if (repeated.length === 0) { | |
| return []; | |
| } | |
| return [...repeated, ...(node.subRuns || [])]; | |
| if (!node.step.repeatPolicy?.repeat) { | |
| return []; | |
| } | |
| return [...(node.subRunsRepeated || []), ...(node.subRuns || [])]; |
Please add a test for one current subRuns entry with repeatPolicy set and no subRunsRepeated entries.
| function TimelineChart({ status, onOpenSubRun }: Props) { | ||
| const remoteNode = useRemoteNode(); | ||
| const openSubRunFromContext = useOpenSubRun(); | ||
| const openSubRun = onOpenSubRun ?? openSubRunFromContext; |
There was a problem hiding this comment.
DAGGraph renders this without an opener or provider, so child bars in execution history are not clickable. Can we pass the opener through DAGGraph and test that path?
yohamta0
left a comment
yohamta0
left a comment
There was a problem hiding this comment.
LGTM 🚀 Thanks a lot for adding support for child runs for the timeline!
|
Apologies for delay in PR comment resolution. Glad to see the purple on my PR :)) |
2 participants
Summary
Add repeat policy child runs for the timeline
Changes
Behavior
subRunsRepeatedexpand archived + current children as[...subRunsRepeated, ...subRuns].subRuns; archived parallel batches are ignored even ifsubRunsRepeatedis set.subRuns(nosubRunsRepeated, not parallel) do not expand.onOpenSubRun(orSubRunOpenProvider); Enter works; parent step bars are not buttons.Files
timelineItems.tsgetTimelineSubRuns/hasTimelineSubRunsupdated for the rules aboveTimelineChart.tsxonOpenSubRun; newTimelineBarwith click/keyboard openTimelineChart.test.tsxtimelineItems.test.tsRelated Issues
Closes #2389
Checklist
Summary by cubic
Adds repeat-policy child runs to the timeline and makes sub-DAG bars openable to drill into child runs. Previously only parallel children expanded and bars were non-interactive; now repeat steps expand archived then current children and bars open via click or keyboard.
subRunsRepeated, then currentsubRuns] with numbered labels; if no archived runs exist yet, current runs still expand.subRuns; archived parallel batches are ignored. Ordinary non-parallel calls without a repeat policy remain collapsed even ifsubRunsexist.TimelineChartaccepts optionalonOpenSubRun(defaults to the open-subrun context).DAGGraphforwards this prop, andDAGExecutionHistorywires it to navigate to the selected child run.Written for commit ee8688d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes