Fixes various layout and truncation issues in new pipeline UX#2350
Fixes various layout and truncation issues in new pipeline UX#2350eblairmckee merged 6 commits intomasterfrom
Conversation
de74147 to
455e916
Compare
|
@claude review |
|
Claude finished @eblairmckee's task in 2m 0s —— View job PR Review
Overall this is a solid set of fixes. The layout is well-structured, tests are included, and the approach to auto-collapsing and translate extents is clean. A few issues worth addressing: BugsDuplicate "Start" item when pipeline is STOPPING ( {isStopping ? <DropdownMenuItem onClick={handleStop}>Retry stop</DropdownMenuItem> : null}
{canStart ? <DropdownMenuItem onClick={handleStart}>Start</DropdownMenuItem> : null}
{isStopping ? <DropdownMenuItem onClick={handleStart}>Start</DropdownMenuItem> : null} // ← duplicateIf Minor Issues
{Array.from({ length: 5 }).map(() => (
<TableRow key={crypto.randomUUID()}> // ← new UUID on every renderThis generates a new key on every render, which defeats React's reconciliation. Use the array index instead: Collapse chevron direction is semantically inverted ( {data.collapsed ? <ChevronUp size={12} /> : <ChevronDown size={12} />}Convention for vertical tree expand/collapse: ChevronDown = "click to expand", ChevronUp = "click to collapse". Currently it's the opposite — the node shows ChevronUp when collapsed (hidden children) and ChevronDown when expanded. If this is an intentional design choice, a comment explaining it would help future readers. Horizontal pan condition is a depth proxy, not a width check ( panOnScrollMode: maxDepth > MAX_NESTING_DEPTH ? PanOnScrollMode.Free : PanOnScrollMode.Vertical,The PR description says panning activates "when diagram exceeds width of parent container", but the implementation uses depth as a heuristic. Since auto-collapsed nodes at depth ≥ 5 are invisible, the visible diagram width doesn't grow with depth. Free panning is therefore enabled when nodes are collapsed (and the diagram is narrower). A more precise check would compare Wrong test file extension for
Nits
What's good
|
| <div | ||
| className={cn( | ||
| 'flex flex-col gap-4', | ||
| 'flex max-w-[calc(100dvw-(--sidebar-width))] flex-col gap-4', |
There was a problem hiding this comment.
Why use sidebar width for non-sidebar component?
There was a problem hiding this comment.
basically just trying to prevent the layout from ever having horizontal scroll.
| expect(autoCollapsedGroups.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('returns maxDepth tracking the deepest visual nesting level', () => { |
There was a problem hiding this comment.
What's the max nesting connect supports?
There was a problem hiding this comment.
infinite! that's why I'm adding this guard.
|
2 fixes applied:
Decided to not implement:
|
…urfaces a dot count
c0c849f to
5460413
Compare
Addresses various bug bash reports
Layout and truncation
Pipeline diagram sidebar

View pipeline page

Connect overview page

Create/edit pipeline page

Diagram horizontal pan
Diagram nesting