feat(ui): gantt chart implementation, entities swimlanes 2 - #496
Conversation
8e67edc to
34107cb
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a reusable generic Gantt chart with typed options, ECharts rendering, hover tracking, zoom navigation, scrolling, and empty-state handling. ChangesGantt chart extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx (1)
52-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd test coverage for the shared
GanttChartcomponent.
GanttChartcentralizes zoom sync, wheel navigation, hover tracking, and empty-state handling for multiple consumers (operator Gantt now, entities Gantt planned). Unlikeutils.tsandoptions.ts, this file has no colocated test. Add aGanttChart.test.tsxcovering the empty-state render, therenderTooltiphover wiring, and theonChartReady/cleanup lifecycle with a mockedEChartsInstance.🤖 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/packages/`@quent/components/src/gantt-chart/GanttChart.tsx around lines 52 - 166, Add a colocated GanttChart.test.tsx covering empty-state rendering, renderTooltip hover updates through observeGanttHover, and onChartReady cleanup using a mocked EChartsInstance. Exercise the component with the required props and verify axis-pointer synchronization, wheel-navigation attachment, hover teardown, and unmount cleanup without changing GanttChart behavior.Source: Path instructions
ui/packages/@quent/components/src/gantt-chart/options.test.ts (1)
7-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the default
gridSpacingfallback and the wheel/pandataZoomentries.The single test case always passes an explicit
gridSpacingand only assertsdataZoom[0]. Add a case that omitsgridSpacingto lock in theTIMELINE_SPACINGdefault, and assert ondataZoom[1]anddataZoom[2]to guard the pan/zoom wheel behavior contract.🧪 Suggested additional test cases
+ it('falls back to TIMELINE_SPACING when gridSpacing is omitted', () => { + const option = buildGanttOption({ + data: [], + durationSeconds: 1, + yAxisCategories: [], + seriesName: 'test-series', + renderItem: vi.fn(() => null) as GanttRenderItem, + minZoomSpanPct: 2, + }); + expect(option.grid).toMatchObject(TIMELINE_SPACING); + }); + + it('configures the pan and shift-zoom dataZoom entries', () => { + const option = buildGanttOption({ + data: [], + durationSeconds: 1, + yAxisCategories: [], + seriesName: 'test-series', + renderItem: vi.fn(() => null) as GanttRenderItem, + minZoomSpanPct: 5, + }); + expect((option.dataZoom as object[])[1]).toMatchObject({ type: 'inside', zoomOnMouseWheel: false }); + expect((option.dataZoom as object[])[2]).toMatchObject({ type: 'inside', zoomOnMouseWheel: 'shift', minSpan: 5 }); + });🤖 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/packages/`@quent/components/src/gantt-chart/options.test.ts around lines 7 - 40, Extend the buildGanttOption tests with a case that omits gridSpacing and verifies the grid uses the TIMELINE_SPACING defaults. In the existing or added assertions, also validate dataZoom[1] and dataZoom[2] to cover the expected pan and wheel-zoom configuration while preserving the current slider assertions.Source: Path instructions
🤖 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/packages/`@quent/components/src/gantt-chart/GanttChart.tsx:
- Around line 74-81: Update the useMemo calculation in GanttChart to determine
maxRow without spreading data.map results into Math.max; use a loop or reduce
over data while preserving the empty-data result and existing yAxisCategories
and rowCount outputs.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/gantt-chart/GanttChart.tsx:
- Around line 52-166: Add a colocated GanttChart.test.tsx covering empty-state
rendering, renderTooltip hover updates through observeGanttHover, and
onChartReady cleanup using a mocked EChartsInstance. Exercise the component with
the required props and verify axis-pointer synchronization, wheel-navigation
attachment, hover teardown, and unmount cleanup without changing GanttChart
behavior.
In `@ui/packages/`@quent/components/src/gantt-chart/options.test.ts:
- Around line 7-40: Extend the buildGanttOption tests with a case that omits
gridSpacing and verifies the grid uses the TIMELINE_SPACING defaults. In the
existing or added assertions, also validate dataZoom[1] and dataZoom[2] to cover
the expected pan and wheel-zoom configuration while preserving the current
slider assertions.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 319a7a6f-bee3-4913-a901-277d75012ea8
📒 Files selected for processing (10)
ui/packages/@quent/components/src/gantt-chart/GanttChart.tsxui/packages/@quent/components/src/gantt-chart/hover.tsui/packages/@quent/components/src/gantt-chart/options.test.tsui/packages/@quent/components/src/gantt-chart/options.tsui/packages/@quent/components/src/gantt-chart/utils.test.tsui/packages/@quent/components/src/gantt-chart/utils.tsui/packages/@quent/components/src/index.tsui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsxui/packages/@quent/components/src/operator-timeline/utils.test.tsui/packages/@quent/components/src/operator-timeline/utils.ts
💤 Files with no reviewable changes (1)
- ui/packages/@quent/components/src/operator-timeline/utils.test.ts
cmatzenbach
left a comment
cmatzenbach
left a comment
There was a problem hiding this comment.
A few general questions for you to consider, but other than that looks good!
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| chartCleanupRef.current?.(); |
There was a problem hiding this comment.
In OperatorGantChart.tsx there was a comment ("empty data replaces the chart without unmounting"). Do we need to do something similar here or are we fine unmounting each time? i.e.
return () => {
if (data.length > 0) return;
chartCleanupRef.current?.():
....
There was a problem hiding this comment.
That effect would run the cleanup function when there was 0 operators, but I think now we want to keep the xaxis pointer and mouse wheel effects even when there is no data so it's goneeee. This one will run the cleanup function when the component is unmounted so we don't leak mouse wheel listeners.
ea1e716 to
bb8f59d
Compare
Centralize timeline chart behavior so the operator Gantt only owns its domain-specific rendering and interactions.
bb8f59d to
60f1863
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx-109-127 (1)
109-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRebind hover listeners when
renderTooltipchanges.
observeGanttHoverruns only fromonChartReady.useChartConnectupdates its callback ref but does not invoke it whenrenderTooltipchanges. Toggle the observer in an effect keyed by the ready chart instance andrenderTooltip != null; clear hover when disabled and clean up on chart replacement or unmount. Add a prop-toggle test.🤖 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/packages/`@quent/components/src/gantt-chart/GanttChart.tsx around lines 109 - 127, The hover observer currently only binds during onChartReady, so changing renderTooltip leaves stale listeners. Add an effect keyed by the ready chart instance and renderTooltip state that attaches or removes observeGanttHover, clears hover when disabled, and cleans up on chart replacement or unmount; avoid duplicating hover cleanup in onChartReady, and add a test covering the prop toggle.Sources: Coding guidelines, Path instructions
🤖 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/packages/`@quent/components/src/gantt-chart/options.test.ts:
- Around line 12-21: Add a test case for buildGanttOption that omits
gridSpacing, then assert the resulting option uses the TIMELINE_SPACING
defaults. Keep the existing explicit-gridSpacing case unchanged and ensure the
new test covers the fallback path.
---
Other comments:
In `@ui/packages/`@quent/components/src/gantt-chart/GanttChart.tsx:
- Around line 109-127: The hover observer currently only binds during
onChartReady, so changing renderTooltip leaves stale listeners. Add an effect
keyed by the ready chart instance and renderTooltip state that attaches or
removes observeGanttHover, clears hover when disabled, and cleans up on chart
replacement or unmount; avoid duplicating hover cleanup in onChartReady, and add
a test covering the prop toggle.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 67e7d127-a2fe-4ed7-97b7-f08845153668
📒 Files selected for processing (10)
ui/packages/@quent/components/src/gantt-chart/GanttChart.tsxui/packages/@quent/components/src/gantt-chart/hover.tsui/packages/@quent/components/src/gantt-chart/options.test.tsui/packages/@quent/components/src/gantt-chart/options.tsui/packages/@quent/components/src/gantt-chart/utils.test.tsui/packages/@quent/components/src/gantt-chart/utils.tsui/packages/@quent/components/src/index.tsui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsxui/packages/@quent/components/src/operator-timeline/utils.test.tsui/packages/@quent/components/src/operator-timeline/utils.ts
💤 Files with no reviewable changes (1)
- ui/packages/@quent/components/src/operator-timeline/utils.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/packages/@quent/components/src/index.ts (1)
242-260: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExport
GanttChartfrom the package root.The barrel exports the new utilities but not the reusable
GanttChartcomponent. External package consumers cannot use the shared component through@quent/components.Export
GanttChart,GanttChartProps, and the public Gantt contracts from this barrel.As per path instructions, each package must expose its public API through
src/index.ts.Proposed fix
// ─── Operator-timeline components ──────────────────────────────────────────── +export { GanttChart } from './gantt-chart/GanttChart'; +export type { + GanttChartProps, + GanttDatum, + GanttGridSpacing, + GanttRenderItem, +} from './gantt-chart/GanttChart'; +export type { GanttHover } from './gantt-chart/hover'; export { OperatorGanttChart } from './operator-timeline/OperatorGanttChart';🤖 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/packages/`@quent/components/src/index.ts around lines 242 - 260, Update the package-root barrel in src/index.ts to export the reusable GanttChart component, its GanttChartProps type, and the public Gantt contracts from their existing modules. Keep the current operator-timeline and utility exports unchanged, and ensure these APIs are accessible to external consumers through the package root.Source: Path instructions
🟡 Other comments (1)
ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx-157-163 (1)
157-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear stale hover state when data disappears.
Line 163 hides the tooltip but retains
hover. If data becomes empty and later returns, the previous tooltip can render before the next pointer event.Reset
hoverwhendata.lengthbecomes zero.As per path instructions, clear synchronized visualization state when data disappears, even if the component remains mounted.
Proposed fix
+ useEffect(() => { + if (data.length === 0) { + setHover(null); + } + }, [data.length]); + return (🤖 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/packages/`@quent/components/src/gantt-chart/GanttChart.tsx around lines 157 - 163, Reset the GanttChart hover state when data becomes empty, rather than only hiding the tooltip. Add an effect tied to data length that clears hover when data.length reaches zero, while preserving the existing renderTooltip behavior for non-empty data.Source: Path instructions
🤖 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/packages/`@quent/components/src/gantt-chart/GanttChart.tsx:
- Line 21: Update the CHART_GROUP import in GanttChart.tsx to use its exported
module, ../timeline/types, instead of ../timeline/Timeline; leave the remaining
chart logic unchanged so the TypeScript build passes.
---
Outside diff comments:
In `@ui/packages/`@quent/components/src/index.ts:
- Around line 242-260: Update the package-root barrel in src/index.ts to export
the reusable GanttChart component, its GanttChartProps type, and the public
Gantt contracts from their existing modules. Keep the current operator-timeline
and utility exports unchanged, and ensure these APIs are accessible to external
consumers through the package root.
---
Other comments:
In `@ui/packages/`@quent/components/src/gantt-chart/GanttChart.tsx:
- Around line 157-163: Reset the GanttChart hover state when data becomes empty,
rather than only hiding the tooltip. Add an effect tied to data length that
clears hover when data.length reaches zero, while preserving the existing
renderTooltip behavior for non-empty data.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: a5192793-871b-4d57-99db-e2053c3ea729
📒 Files selected for processing (10)
ui/packages/@quent/components/src/gantt-chart/GanttChart.tsxui/packages/@quent/components/src/gantt-chart/hover.tsui/packages/@quent/components/src/gantt-chart/options.test.tsui/packages/@quent/components/src/gantt-chart/options.tsui/packages/@quent/components/src/gantt-chart/utils.test.tsui/packages/@quent/components/src/gantt-chart/utils.tsui/packages/@quent/components/src/index.tsui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsxui/packages/@quent/components/src/operator-timeline/utils.test.tsui/packages/@quent/components/src/operator-timeline/utils.ts
💤 Files with no reviewable changes (1)
- ui/packages/@quent/components/src/operator-timeline/utils.test.ts
|
/merge |
2 participants
Description
Captures common quent functionality (data zoom sync, xaxis pointer, etc) in a GanttChart component, much like Timeline. This implementation will be used to implement the operators gantt chart and the entities gantt chart. This PR only updates the OperatorGantt to the new base implementation.
Related Issues
relates to #215
Testing
Operators gantt chart functions as previously
Screenshots