feat(ui): entities chart implementation, entities swimlanes 2.5 - #497
Conversation
johallar
changed the title
226678c to
00e8c6d
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds a long-entity Gantt component, exported long-entity types, and utilities that convert FSM transitions into row-packed chart entries. It also adds tests for entry building and a chart option default. ChangesGantt timeline components
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
ui/packages/@quent/components/src/gantt-chart/hover.ts-26-36 (1)
26-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the hover when the pixel conversion yields no finite value.
At Line 28 the handler returns without calling
onChange. The consumer keeps the previous hover, so the tooltip shows a stale timestamp while the pointer continues to move. All other failure branches clear the hover.🐛 Proposed fix
try { const value = instance.convertFromPixel({ xAxisIndex: 0 }, point[0]); - if (value == null || !Number.isFinite(value as number)) return; + if (value == null || !Number.isFinite(value as number)) { + onChange(null); + 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/hover.ts around lines 26 - 36, Update the pixel-conversion validation in the hover handler to call onChange(null) before returning when convertFromPixel produces null or a non-finite value. Preserve the existing onChange timestamp path for valid finite values and the catch-block clearing behavior.ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx-74-81 (1)
74-81: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid the spread over the full datum array.
Math.max(...data.map(...))passes one argument per datum.LongEntitiesGanttemits one datum per state segment, so the argument count grows with the total number of transitions in the trace. A large trace can throwRangeError: Maximum call stack size exceeded. Use a reduction instead.🐛 Proposed fix
const { yAxisCategories, rowCount } = useMemo(() => { if (data.length === 0) return { yAxisCategories: [] as number[], rowCount: 0 }; - const maxRow = Math.max(...data.map(datum => datum.value[2])); + const maxRow = data.reduce((max, datum) => Math.max(max, datum.value[2]), 0); 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 74 - 81, Replace the spread-based Math.max call in the yAxisCategories/rowCount useMemo with a reduction over data that computes the maximum datum.value[2] without expanding the array into function arguments. Preserve the existing empty-data result and category/rowCount calculations.Source: Path instructions
🧹 Nitpick comments (6)
ui/packages/@quent/components/src/index.ts (1)
263-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark the
stackOperatorsIntoRowsalias as deprecated.The barrel now exposes one function under two public names. Keep the alias for compatibility, but state that
stackIntervalsIntoRowsis the supported name so the alias can be removed after consumers migrate.♻️ Proposed change
export { clipRectByRect, stackIntervalsIntoRows, + /** `@deprecated` Use `stackIntervalsIntoRows`. */ stackIntervalsIntoRows as stackOperatorsIntoRows, } from './gantt-chart/utils';🤖 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 263 - 267, Add a deprecation annotation for the exported stackOperatorsIntoRows alias in the barrel export, directing consumers to use stackIntervalsIntoRows as the supported name while preserving the alias for compatibility.ui/packages/@quent/components/src/long-entities/utils.test.ts (1)
104-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the attribute fixture instead of casting to
never.
as unknown as neverremoves the only check that would catch aDynamicAttributeshape change in the ts-bindings. Build the fixture from the canonical type.💚 Proposed fix
+import type { DynamicAttribute } from '`@quent/utils`'; + +const bytesAttribute: DynamicAttribute = { key: 'bytes', value: { Int: 42 } }; + it('carries transition attributes onto segments', () => { const fsm = makeFsm('e1', [ - transition('a', 0, { - attributes: [{ key: 'bytes', value: { Int: 42 } } as unknown as never], - }), + transition('a', 0, { attributes: [bytesAttribute] }), transition('exit', 1), ]);Adjust the literal to the exact
DynamicAttributevariant shape if the binding differs.Based on path instructions: "Build fixtures from canonical production or ts-binding types (
Pick,Partial, builders/factories) instead of brittle hand-written lookalike interfaces."🤖 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/long-entities/utils.test.ts around lines 104 - 113, Update the attribute fixture in the “carries transition attributes onto segments” test to use the canonical DynamicAttribute type or an existing production/ts-binding builder, removing the `as unknown as never` cast. Match the exact bound variant shape while preserving the current bytes value and assertion.Source: Path instructions
ui/packages/@quent/components/src/gantt-chart/utils.ts (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the mutation contract in the JSDoc.
stackIntervalsIntoRowsmutatesrowIndexon the caller's objects and returns the same array reference in input order, while sorting only an internal copy. This function is public API through the package barrel, so state both facts in the doc comment.♻️ Proposed doc update
-/** Greedily pack intervals into non-overlapping rows. */ +/** + * Greedily pack intervals into non-overlapping rows. + * Assigns `rowIndex` in place and returns the same array in input order. + */ export function stackIntervalsIntoRows<🤖 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/utils.ts around lines 23 - 29, Update the JSDoc for stackIntervalsIntoRows to document that it mutates each caller-provided object's rowIndex and returns the original entries array in its input order, while sorting only an internal copy for packing.ui/packages/@quent/components/src/long-entities/utils.ts (1)
9-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese row-id helpers have no consumer and are not exported.
LONG_ENTITIES_ROW_TYPE,longEntitiesRowId, andresourceIdFromLongEntitiesRowIdare not re-exported fromui/packages/@quent/components/src/index.ts(lines 243-247), and no file in this PR calls them. The operator counterparts are exported at lines 253-262 of the same barrel. Either export them now for the follow-up integration or move them to that PR.Also note the collision risk: a resource id that itself starts with
__long_entities__is misclassified byresourceIdFromLongEntitiesRowId. If the row-id space stays string based, consider a typed row variant instead of a string prefix.Based on coding guidelines: "Avoid sentinel strings that can collide with real identifiers; use typed variants, opaque IDs, or another collision-free representation."
🤖 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/long-entities/utils.ts around lines 9 - 23, Export LONG_ENTITIES_ROW_TYPE, longEntitiesRowId, and resourceIdFromLongEntitiesRowId through the components barrel alongside the operator helpers so follow-up consumers can use them. Also replace the collision-prone string-prefix encoding in longEntitiesRowId/resourceIdFromLongEntitiesRowId with the project’s typed or otherwise collision-free row-ID representation, preserving correct round-tripping of resource IDs that begin with __long_entities__.Source: Coding guidelines
ui/packages/@quent/components/src/long-entities/LongEntitiesGantt.tsx (1)
66-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow
datumonce and drop the non-null assertions.The current guard relies on
entry?.short-circuiting so thatdatum!at Line 68 is never evaluated whendatumis undefined. Line 103 repeats the assertion. Guarddatumfirst; both assertions then become unnecessary and the code stays correct after future edits.♻️ Proposed refactor
const datum = customSeriesData[params.dataIndex]; - const entry = datum ? entries[datum.entryIndex] : undefined; - const segment = entry?.segments[datum!.segmentIndex]; - if (!entry || !segment) return null; + if (!datum) return null; + const entry = entries[datum.entryIndex]; + const segment = entry?.segments[datum.segmentIndex]; + if (!entry || !segment) return null;Then change Line 103 to
datum.segmentIndex === 0.🤖 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/long-entities/LongEntitiesGantt.tsx around lines 66 - 69, In the relevant render logic, guard that datum exists before accessing its segmentIndex, then derive entry and segment only after the guard. Remove the non-null assertions and update the later segment-index check to use datum.segmentIndex directly, preserving the existing null return behavior when datum, entry, or segment is absent.ui/packages/@quent/components/src/gantt-chart/options.test.ts (1)
7-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
gridSpacingfallback.
buildGanttOptiondefaultsgridSpacingtoTIMELINE_SPACING(options.tsLine 36). That branch is the only fallback in the builder and no test exercises it. Add a case that omitsgridSpacingandcursor.💚 Proposed additional test
+ 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: 1, + }); + + expect(option.grid).toMatchObject({ ...TIMELINE_SPACING, width: undefined, height: undefined }); + expect((option.series as { cursor?: unknown }[])[0]?.cursor).toBeUndefined(); + });Import the constant:
import { buildGanttOption, type GanttRenderItem } from './options'; +import { TIMELINE_SPACING } from '../timeline/types';Based on path instructions: "Cover observable behavior, fallback/unknown inputs, empty/error states".
🤖 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 - 39, Extend the buildGanttOption tests with a case that omits gridSpacing and cursor, then assert the resulting grid uses TIMELINE_SPACING. Import and reference the existing TIMELINE_SPACING constant, while preserving assertions for the other observable default behavior.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 109-140: Update the GanttChart hover wiring around onChartReady
and useChartConnect so the chart instance is also stored in reactive state, then
manage observeGanttHover through an effect keyed by that instance,
renderTooltip, and data presence. Remove the hover subscription and clear hover
whenever any condition is false, while preserving the existing chart cleanup and
wheel-navigation behavior.
---
Other comments:
In `@ui/packages/`@quent/components/src/gantt-chart/GanttChart.tsx:
- Around line 74-81: Replace the spread-based Math.max call in the
yAxisCategories/rowCount useMemo with a reduction over data that computes the
maximum datum.value[2] without expanding the array into function arguments.
Preserve the existing empty-data result and category/rowCount calculations.
In `@ui/packages/`@quent/components/src/gantt-chart/hover.ts:
- Around line 26-36: Update the pixel-conversion validation in the hover handler
to call onChange(null) before returning when convertFromPixel produces null or a
non-finite value. Preserve the existing onChange timestamp path for valid finite
values and the catch-block clearing behavior.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/gantt-chart/options.test.ts:
- Around line 7-39: Extend the buildGanttOption tests with a case that omits
gridSpacing and cursor, then assert the resulting grid uses TIMELINE_SPACING.
Import and reference the existing TIMELINE_SPACING constant, while preserving
assertions for the other observable default behavior.
In `@ui/packages/`@quent/components/src/gantt-chart/utils.ts:
- Around line 23-29: Update the JSDoc for stackIntervalsIntoRows to document
that it mutates each caller-provided object's rowIndex and returns the original
entries array in its input order, while sorting only an internal copy for
packing.
In `@ui/packages/`@quent/components/src/index.ts:
- Around line 263-267: Add a deprecation annotation for the exported
stackOperatorsIntoRows alias in the barrel export, directing consumers to use
stackIntervalsIntoRows as the supported name while preserving the alias for
compatibility.
In `@ui/packages/`@quent/components/src/long-entities/LongEntitiesGantt.tsx:
- Around line 66-69: In the relevant render logic, guard that datum exists
before accessing its segmentIndex, then derive entry and segment only after the
guard. Remove the non-null assertions and update the later segment-index check
to use datum.segmentIndex directly, preserving the existing null return behavior
when datum, entry, or segment is absent.
In `@ui/packages/`@quent/components/src/long-entities/utils.test.ts:
- Around line 104-113: Update the attribute fixture in the “carries transition
attributes onto segments” test to use the canonical DynamicAttribute type or an
existing production/ts-binding builder, removing the `as unknown as never` cast.
Match the exact bound variant shape while preserving the current bytes value and
assertion.
In `@ui/packages/`@quent/components/src/long-entities/utils.ts:
- Around line 9-23: Export LONG_ENTITIES_ROW_TYPE, longEntitiesRowId, and
resourceIdFromLongEntitiesRowId through the components barrel alongside the
operator helpers so follow-up consumers can use them. Also replace the
collision-prone string-prefix encoding in
longEntitiesRowId/resourceIdFromLongEntitiesRowId with the project’s typed or
otherwise collision-free row-ID representation, preserving correct
round-tripping of resource IDs that begin with __long_entities__.
🪄 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: 5ff978b3-e856-4bfb-b53d-c8cf4c19fad9
📒 Files selected for processing (14)
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/long-entities/LongEntitiesGantt.tsxui/packages/@quent/components/src/long-entities/types.tsui/packages/@quent/components/src/long-entities/utils.test.tsui/packages/@quent/components/src/long-entities/utils.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
| const onChartReady = useCallback( | ||
| (instance: EChartsInstance) => { | ||
| chartCleanupRef.current?.(); | ||
| registerAxisPointerSync(instance, 0, { receiveShowTip: false }); | ||
| const detachWheelNavigation = attachWheelNavigation( | ||
| instance, | ||
| wrapperRef.current ?? undefined | ||
| ); | ||
| const detachHover = renderTooltip ? observeGanttHover(instance, setHover) : undefined; | ||
| const cleanup = () => { | ||
| unregisterAxisPointerSync(instance); | ||
| detachWheelNavigation(); | ||
| detachHover?.(); | ||
| if (chartCleanupRef.current === cleanup) chartCleanupRef.current = null; | ||
| }; | ||
| chartCleanupRef.current = cleanup; | ||
| }, | ||
| [attachWheelNavigation, renderTooltip] | ||
| ); | ||
|
|
||
| const { handleChartReady, instanceRef } = useChartConnect({ | ||
| durationSeconds, | ||
| chartGroup: CHART_GROUP, | ||
| onReady: onChartReady, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| chartCleanupRef.current?.(); | ||
| instanceRef.current = null; | ||
| }; | ||
| }, [instanceRef]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect useChartConnect to confirm when onReady runs and whether the instance is exposed reactively.
fd -t f 'useChartConnect.*' ui/packages/@quent/components/src/lib --exec cat -n {}Repository: rapidsai/quent
Length of output: 3799
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- GanttChart outline ---'
ast-grep outline ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx --view compact
printf '%s\n' '--- GanttChart relevant sections ---'
cat -n ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx | sed -n '1,210p'
printf '%s\n' '--- chart connection and resize usage ---'
rg -n -C 8 'useChartConnect|useChartResize|observeGanttHover|setHover|onChartReady' ui/packages/@quent/components/srcRepository: rapidsai/quent
Length of output: 330
🏁 Script executed:
#!/bin/bash
set -e
cat -n ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx | sed -n '1,210p'
fd -t f 'useChartResize.*' ui/packages/@quent/components/src --exec cat -n {}Repository: rapidsai/quent
Length of output: 8281
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- hover implementation ---'
fd -t f 'hover.*' ui/packages/@quent/components/src/gantt-chart --exec cat -n {}
printf '%s\n' '--- GanttChart consumers and conditional tooltip props ---'
rg -n -C 5 'renderTooltip=|renderTooltip:|<GanttChart|GanttChart\(' uiRepository: rapidsai/quent
Length of output: 5451
🏁 Script executed:
#!/bin/bash
set -e
fd -t f 'hover.*' ui/packages/@quent/components/src/gantt-chart --exec cat -n {}
rg -n -C 5 'renderTooltip=|renderTooltip:|<GanttChart|GanttChart\(' uiRepository: rapidsai/quent
Length of output: 5363
Make the hover subscription reactive.
useChartConnect invokes onReady only from handleChartReady; changing renderTooltip does not invoke it again for the current chart. Attach observeGanttHover in an effect keyed by the chart instance, renderTooltip, and data presence. Clear hover and remove the pointer listeners when any condition becomes false. Expose the chart instance through reactive state because instanceRef.current changes do not rerun effects.
🤖 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 - 140, Update the GanttChart hover wiring around onChartReady and
useChartConnect so the chart instance is also stored in reactive state, then
manage observeGanttHover through an effect keyed by that instance,
renderTooltip, and data presence. Remove the hover subscription and clear hover
whenever any condition is false, while preserving the existing chart cleanup and
wheel-navigation behavior.
Source: Path instructions
cmatzenbach
left a comment
cmatzenbach
left a comment
There was a problem hiding this comment.
New additions look good!
1c87da3 to
495ffd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
ui/packages/@quent/components/src/long-entities/utils.test.ts-104-113 (1)
104-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winType the attribute fixture.
as unknown as neverdisables validation againstDynamicAttribute. A schema change can leave this test compiling and passing with a fixture that no longer represents production data. Build this fixture withDynamicAttributeor a shared canonical builder instead.As per coding guidelines and path instructions, build fixtures from canonical production or ts-binding types instead of hand-written lookalikes.
🤖 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/long-entities/utils.test.ts around lines 104 - 113, Update the attribute fixture in the “carries transition attributes onto segments” test to use the production DynamicAttribute type or the shared canonical attribute builder. Remove the `as unknown as never` cast while preserving the existing bytes attribute value and test behavior.Sources: Coding guidelines, Path instructions
ui/packages/@quent/components/src/gantt-chart/hover.ts-19-28 (1)
19-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear hover state when timestamp conversion fails.
If the chart is disposed or
convertFromPixelreturnsnullor a non-finite value, this handler keeps the previousGanttHover. The tooltip can then remain at an obsolete position. CallonChange(null)before both returns.Proposed fix
- if (instance.isDisposed?.()) return; + if (instance.isDisposed?.()) { + onChange(null); + return; + } @@ - if (value == null || !Number.isFinite(value as number)) return; + if (value == null || !Number.isFinite(value as number)) { + onChange(null); + 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/hover.ts around lines 19 - 28, Update the hover handler around the disposed-instance check and the convertFromPixel validation so it calls onChange(null) before returning when the chart is disposed or timestamp conversion yields null or a non-finite value. Preserve the existing behavior for valid conversions and out-of-grid points.
🧹 Nitpick comments (1)
ui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsx (1)
209-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the remaining operator-timeline contracts.
Existing tests cover generic interval packing, including touching endpoints, and overlapping operator spans. Add tests for returned order, worker-filtered touching spans, and
OperatorGanttChartempty-state and click behavior.🤖 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/operator-timeline/OperatorGanttChart.tsx around lines 209 - 221, The operator-timeline contracts lack focused test coverage. Add tests for returned ordering and worker-filtered touching spans in ui/packages/@quent/components/src/operator-timeline/utils.ts at lines 112 and 141, and add OperatorGanttChart tests covering the empty state and click behavior around ui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsx lines 209-221; these sites require test coverage rather than production changes.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/long-entities/utils.ts:
- Around line 10-22: Replace the prefix-based identity in longEntitiesRowId and
resourceIdFromLongEntitiesRowId with collision-free metadata: store the
synthetic row kind separately from resourceId, or use an opaque tagged
representation outside the resource-ID namespace. Ensure
resourceIdFromLongEntitiesRowId only recognizes values explicitly marked as
long-entities rows and never reinterprets a real resource ID.
In `@ui/packages/`@quent/components/src/operator-timeline/OperatorGanttChart.tsx:
- Around line 219-220: Update the Gantt bar interaction around
onEvents={handleClick} to provide a semantic, focusable keyboard-operable
control for each operator bar. Ensure Enter and Space trigger the same selection
transition as the existing click handler, including selected-node detail
updates, while preserving pointer selection behavior.
---
Other comments:
In `@ui/packages/`@quent/components/src/gantt-chart/hover.ts:
- Around line 19-28: Update the hover handler around the disposed-instance check
and the convertFromPixel validation so it calls onChange(null) before returning
when the chart is disposed or timestamp conversion yields null or a non-finite
value. Preserve the existing behavior for valid conversions and out-of-grid
points.
In `@ui/packages/`@quent/components/src/long-entities/utils.test.ts:
- Around line 104-113: Update the attribute fixture in the “carries transition
attributes onto segments” test to use the production DynamicAttribute type or
the shared canonical attribute builder. Remove the `as unknown as never` cast
while preserving the existing bytes attribute value and test behavior.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/operator-timeline/OperatorGanttChart.tsx:
- Around line 209-221: The operator-timeline contracts lack focused test
coverage. Add tests for returned ordering and worker-filtered touching spans in
ui/packages/@quent/components/src/operator-timeline/utils.ts at lines 112 and
141, and add OperatorGanttChart tests covering the empty state and click
behavior around
ui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsx lines
209-221; these sites require test coverage rather than production changes.
🪄 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: 45154aae-457e-4d7d-8ac5-277553d427a1
📒 Files selected for processing (14)
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/long-entities/LongEntitiesGantt.tsxui/packages/@quent/components/src/long-entities/types.tsui/packages/@quent/components/src/long-entities/utils.test.tsui/packages/@quent/components/src/long-entities/utils.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
| export const LONG_ENTITIES_ROW_TYPE = 'long-entities'; | ||
| const LONG_ENTITIES_ROW_ID_PREFIX = '__long_entities__'; | ||
|
|
||
| /** Id used for the synthetic long-entities row under a resource. */ | ||
| export function longEntitiesRowId(resourceId: string): string { | ||
| return `${LONG_ENTITIES_ROW_ID_PREFIX}${resourceId}`; | ||
| } | ||
|
|
||
| /** Extract the resource id from a long-entities row id, or null if it is not one. */ | ||
| export function resourceIdFromLongEntitiesRowId(id: string): string | null { | ||
| return id.startsWith(LONG_ENTITIES_ROW_ID_PREFIX) | ||
| ? id.slice(LONG_ENTITIES_ROW_ID_PREFIX.length) | ||
| : null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use a collision-free long-entities row identity.
resourceIdFromLongEntitiesRowId() classifies every ID with the __long_entities__ prefix as synthetic. If a real resource ID uses that prefix, this function strips it and returns a different resource ID. Store the row kind separately from resourceId, or use a tagged opaque value that cannot share the resource-ID namespace.
As per coding guidelines and path instructions, avoid sentinel strings that can collide with real identifiers.
🤖 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/long-entities/utils.ts around lines 10 -
22, Replace the prefix-based identity in longEntitiesRowId and
resourceIdFromLongEntitiesRowId with collision-free metadata: store the
synthetic row kind separately from resourceId, or use an opaque tagged
representation outside the resource-ID namespace. Ensure
resourceIdFromLongEntitiesRowId only recognizes values explicitly marked as
long-entities rows and never reinterprets a real resource ID.
Sources: Coding guidelines, Path instructions
| cursor="pointer" | ||
| onEvents={handleClick} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Provide keyboard access for operator selection.
cursor="pointer" and onEvents.click expose selection only through the ECharts chart. The supplied GanttChart contract has no focusable control or keyboard event for an individual bar. Keyboard users cannot select an operator or update selected-node details. Add a semantic keyboard-operable equivalent that performs the same selection transition.
As per coding guidelines, interactive affordances must be clickable and keyboard-operable; path instructions require semantic interactive elements and keyboard behavior.
🤖 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/operator-timeline/OperatorGanttChart.tsx
around lines 219 - 220, Update the Gantt bar interaction around
onEvents={handleClick} to provide a semantic, focusable keyboard-operable
control for each operator bar. Ensure Enter and Space trigger the same selection
transition as the existing click handler, including selected-node detail
updates, while preserving pointer selection behavior.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
Addressing here: #538, i think this is impacts multiple charts, we could handle better/consistently
There was a problem hiding this comment.
@johallar, agreed. Issue #538 is the correct place to implement a shared, consistent keyboard-accessibility solution for the Gantt chart consumers. This operator-specific finding can be handled there.
You are interacting with an AI system.
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
ui/packages/@quent/components/src/gantt-chart/options.test.ts-7-40 (1)
7-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the default and empty-data contracts.
Lines 12-21 supply non-empty
data,cursor, andgridSpacing. Add a case withdata: []and omitted optional values. Assert the default grid spacing, empty series data, and valid zoom configuration.As per path instructions, cover fallback inputs and empty states in UI tests.
🤖 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 using data: [] and omitting optional cursor and gridSpacing inputs. Assert the documented default grid spacing, empty series data, and a valid dataZoom configuration, while retaining the existing non-empty custom-series coverage.Sources: Coding guidelines, Path instructions
ui/packages/@quent/components/src/long-entities/utils.test.ts-104-113 (1)
104-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a canonical
DynamicAttributefixture.Line 107 casts a hand-written payload through
unknowntonever. This removes type validation for the wire payload and can hide a binding-schema change. Construct the attribute withDynamicAttributeor the element type ofFsmTransition['attributes']instead.As per coding guidelines, build fixtures from canonical production or generated types instead of hand-written lookalike payloads.
🤖 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/long-entities/utils.test.ts around lines 104 - 113, The transition fixture in “carries transition attributes onto segments” bypasses type validation with an unknown-to-never cast. Replace the hand-written attribute payload with a canonical DynamicAttribute fixture or the element type from FsmTransition['attributes'], preserving the existing bytes value and assertion.Sources: Coding guidelines, Path instructions
ui/packages/@quent/components/src/gantt-chart/hover.ts-26-30 (1)
26-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear hover state when coordinate conversion is invalid.
Line 28 returns without calling
onChange(null). IfcontainPixelsucceeds butconvertFromPixelreturnsnull,NaN, or another non-finite value, the previous tooltip remains visible with stale coordinates.Proposed fix
- if (value == null || !Number.isFinite(value as number)) return; + if (value == null || !Number.isFinite(value as number)) { + onChange(null); + 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/hover.ts around lines 26 - 30, Update the hover handling around instance.convertFromPixel so invalid conversion results (null, NaN, or other non-finite values) call onChange(null) before returning, clearing any stale tooltip state; preserve the existing onChange payload for valid timestamps.
🤖 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: Resolve the TS2459 failure involving CHART_GROUP: ensure the symbol
is exported from its current owner in ../timeline/Timeline, or move it to an
exported leaf module and update the GanttChart import accordingly. Preserve a
single shared constant and verify pnpm --dir ui typecheck succeeds.
In `@ui/packages/`@quent/components/src/gantt-chart/options.ts:
- Around line 38-111: The Gantt chart options returned by the chart-options
factory lack an accessible summary for non-empty data. Add an explicit semantic
description or equivalent accessible table covering each interval; if using
ECharts ARIA, register AriaComponent, include its option type in EChartsOption,
and enable aria.enabled with the explicit description while preserving the
existing chart configuration.
In `@ui/packages/`@quent/components/src/gantt-chart/utils.ts:
- Around line 32-41: Replace the linear row scan in the interval-packing helper
with an active-row min-heap keyed by end time and a reusable-row-index min-heap,
preserving the lowest available row index when rows are released out of end-time
order. Update the helper used by buildLongEntityEntries() and both operator-span
builders, and add a regression test covering out-of-order row release while
verifying row assignments remain lowest-index first.
---
Other comments:
In `@ui/packages/`@quent/components/src/gantt-chart/hover.ts:
- Around line 26-30: Update the hover handling around instance.convertFromPixel
so invalid conversion results (null, NaN, or other non-finite values) call
onChange(null) before returning, clearing any stale tooltip state; preserve the
existing onChange payload for valid timestamps.
In `@ui/packages/`@quent/components/src/gantt-chart/options.test.ts:
- Around line 7-40: Extend the buildGanttOption tests with a case using data: []
and omitting optional cursor and gridSpacing inputs. Assert the documented
default grid spacing, empty series data, and a valid dataZoom configuration,
while retaining the existing non-empty custom-series coverage.
In `@ui/packages/`@quent/components/src/long-entities/utils.test.ts:
- Around line 104-113: The transition fixture in “carries transition attributes
onto segments” bypasses type validation with an unknown-to-never cast. Replace
the hand-written attribute payload with a canonical DynamicAttribute fixture or
the element type from FsmTransition['attributes'], preserving the existing bytes
value and assertion.
🪄 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: 4f2e12a2-6a22-44f3-85d5-6939d5d64dbc
📒 Files selected for processing (14)
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/long-entities/LongEntitiesGantt.tsxui/packages/@quent/components/src/long-entities/types.tsui/packages/@quent/components/src/long-entities/utils.test.tsui/packages/@quent/components/src/long-entities/utils.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
| for (const entry of sorted) { | ||
| let row = 0; | ||
| while (row < rowEndMs.length && entry.startMs < rowEndMs[row]) { | ||
| row++; | ||
| } | ||
| if (row === rowEndMs.length) { | ||
| rowEndMs.push(entry.endMs); | ||
| } else { | ||
| rowEndMs[row] = Math.max(rowEndMs[row], entry.endMs); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Prevent quadratic interval packing.
Line 34 scans every active row for each interval. For N mutually overlapping entries, this creates O(N²) work before the chart renders. buildLongEntityEntries() and both operator-span builders use this helper. Use an active-row min-heap and a reusable-row-index min-heap. Preserve lowest-index row assignment with a regression test for rows released out of end-time order. As per path instructions, “this UI renders big traces.”
🤖 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/utils.ts around lines 32 - 41,
Replace the linear row scan in the interval-packing helper with an active-row
min-heap keyed by end time and a reusable-row-index min-heap, preserving the
lowest available row index when rows are released out of end-time order. Update
the helper used by buildLongEntityEntries() and both operator-span builders, and
add a regression test covering out-of-order row release while verifying row
assignments remain lowest-index first.
Source: Path instructions
There was a problem hiding this comment.
These are limited to 100 at a time so deferring this
There was a problem hiding this comment.
@johallar, acknowledged. The 100-entry limit bounds the current cost, so deferring the heap-based optimization is reasonable for this pull request.
Would you like me to create a follow-up GitHub issue for the interval-packing optimization?
🐇
You are interacting with an AI system.
495ffd3 to
6cc1728
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
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/hover.ts-26-35 (1)
26-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear stale hover state when coordinate conversion fails.
When
convertFromPixelreturnsnullor a non-finite value, callonChange(null)before returning. Otherwise,GanttChartretains the previous hover until pointer leave. Add a regression 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/hover.ts around lines 26 - 35, Update the coordinate validation in the hover conversion flow to call onChange(null) before returning when convertFromPixel produces null or a non-finite value, matching the existing catch behavior. Add a regression test covering both invalid conversion outcomes and verifying the stale hover state is cleared.
🧹 Nitpick comments (4)
ui/packages/@quent/components/src/index.ts (2)
242-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the shared Gantt surface as well.
This diff adds
LongEntitiesGanttandOperatorGanttChartexports, butGanttChart,GanttChartProps,GanttDatum, andGanttRenderItemstay internal. A consumer outside@quent/componentscannot build a Gantt on the shared foundation or type arenderItemcallback. The stack describes this layer as the shared Gantt foundation, so the primitive belongs in the barrel with its own section.♻️ Proposed addition
+// ─── Gantt chart ────────────────────────────────────────────────────────────── +export { GanttChart } from './gantt-chart/GanttChart'; +export type { GanttChartProps, GanttRenderItem } from './gantt-chart/GanttChart'; + // ─── Long-entities components ───────────────────────────────────────────────── export { LongEntitiesGantt } from './long-entities/LongEntitiesGantt';🤖 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 - 246, Update the components barrel near the Long-entities exports to add a dedicated shared Gantt foundation section, exporting GanttChart and its public types GanttChartProps, GanttDatum, and GanttRenderItem from their existing module. Keep the current LongEntitiesGantt exports unchanged.Source: Path instructions
262-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the shared Gantt utilities out of the operator-timeline section and mark the alias.
These three exports come from
./gantt-chart/utils, but they sit under theOperator-timeline componentsheader at line 248.clipRectByRectandstackIntervalsIntoRowsare shared Gantt utilities, not operator-timeline utilities.stackOperatorsIntoRowsis a compatibility alias only. Group the shared exports under a Gantt section and add a deprecation note on the alias so callers migrate to the shared name.🤖 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 262 - 266, Reorganize the exports in the index around clipRectByRect and stackIntervalsIntoRows so they appear under a dedicated Gantt utilities section rather than the Operator-timeline components section. Keep stackOperatorsIntoRows as an alias for compatibility, add a deprecation annotation directing callers to stackIntervalsIntoRows, and preserve all existing export behavior.ui/packages/@quent/components/src/gantt-chart/utils.test.ts (1)
48-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for row reuse behind a long interval.
The suite never exercises the case where an early long interval holds row 0 while later short intervals must reuse row 1. That path decides whether packing keeps the lowest free row index. Both operator span builders and
buildLongEntityEntries()now depend on this single helper, so the boundary deserves a fixed test.As per path instructions, retain "meaningful boundary coverage".
💚 Proposed additional test
it('handles unsorted input and mutates the original entries', () => { const later = span(10, 20); const earlier = span(0, 5); const entries = [later, earlier]; expect(stackIntervalsIntoRows(entries)).toBe(entries); expect(entries.map(entry => entry.rowIndex)).toEqual([0, 0]); }); + + it('reuses the lowest free row behind a long interval', () => { + const long = span(0, 100); + const first = span(10, 20); + const second = span(30, 40); + stackIntervalsIntoRows([long, first, second]); + expect([long.rowIndex, first.rowIndex, second.rowIndex]).toEqual([0, 1, 1]); + }); });🤖 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/utils.test.ts around lines 48 - 74, Add a focused test in the stackIntervalsIntoRows suite where an early long interval occupies row 0 and later intervals overlap it but can reuse row 1, asserting the helper assigns the lowest available row index. Keep the test meaningful by verifying the resulting rowIndex values and covering the row-reuse boundary relied on by the span builders and buildLongEntityEntries().Source: Path instructions
ui/packages/@quent/components/src/long-entities/utils.ts (1)
74-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive entry bounds from all segments, not from positions.
buildSegments()preserves transition order, and no ascending-timestamp guarantee exists forFiniteStateMachine.transitions. Inverted entry bounds can causestackIntervalsIntoRows()to place overlapping segments on the same row. ComputestartMswith the minimum segment start andendMswith the maximum segment end.🤖 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/long-entities/utils.ts around lines 74 - 87, Update the entry-bound calculation in the loop using buildSegments so startMs is the minimum startMs across all segments and endMs is the maximum endMs across all segments, rather than relying on the first and last segment positions. Keep the existing empty-segments skip and entry construction unchanged.
🤖 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/long-entities/LongEntitiesGantt.tsx:
- Around line 84-85: Update the segment-rendering logic around clippedShape in
LongEntitiesGantt so the entity label is rendered on the first segment that
remains visible after horizontal clipping, rather than only on the original
first segment. Preserve label visibility when navigating horizontally, and add
regression coverage for an offscreen first segment followed by a visible
segment.
In `@ui/packages/`@quent/components/src/long-entities/utils.test.ts:
- Around line 104-113: Update the “carries transition attributes onto segments”
test to construct its transition metadata with the canonical production or
ts-binding DynamicAttribute fixture/builder, removing the unknown-to-never cast.
Include both regular and derived attributes in the fixture, then assert the
segment preserves both attributes and derivedAttributes outputs.
- Around line 115-120: Update the “stacks non-overlapping entities onto the same
row” test to use adjacent intervals whose end and start timestamps are equal,
such as ending the first FSM at 1 and starting the second at 1. Keep the
expected rowIndex values as [0, 0] to verify boundary-touching entities reuse
the same row.
---
Other comments:
In `@ui/packages/`@quent/components/src/gantt-chart/hover.ts:
- Around line 26-35: Update the coordinate validation in the hover conversion
flow to call onChange(null) before returning when convertFromPixel produces null
or a non-finite value, matching the existing catch behavior. Add a regression
test covering both invalid conversion outcomes and verifying the stale hover
state is cleared.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/gantt-chart/utils.test.ts:
- Around line 48-74: Add a focused test in the stackIntervalsIntoRows suite
where an early long interval occupies row 0 and later intervals overlap it but
can reuse row 1, asserting the helper assigns the lowest available row index.
Keep the test meaningful by verifying the resulting rowIndex values and covering
the row-reuse boundary relied on by the span builders and
buildLongEntityEntries().
In `@ui/packages/`@quent/components/src/index.ts:
- Around line 242-246: Update the components barrel near the Long-entities
exports to add a dedicated shared Gantt foundation section, exporting GanttChart
and its public types GanttChartProps, GanttDatum, and GanttRenderItem from their
existing module. Keep the current LongEntitiesGantt exports unchanged.
- Around line 262-266: Reorganize the exports in the index around clipRectByRect
and stackIntervalsIntoRows so they appear under a dedicated Gantt utilities
section rather than the Operator-timeline components section. Keep
stackOperatorsIntoRows as an alias for compatibility, add a deprecation
annotation directing callers to stackIntervalsIntoRows, and preserve all
existing export behavior.
In `@ui/packages/`@quent/components/src/long-entities/utils.ts:
- Around line 74-87: Update the entry-bound calculation in the loop using
buildSegments so startMs is the minimum startMs across all segments and endMs is
the maximum endMs across all segments, rather than relying on the first and last
segment positions. Keep the existing empty-segments skip and entry construction
unchanged.
🪄 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: 47a3be8e-6b8a-41d6-b6ab-63d6b04cdd95
📒 Files selected for processing (14)
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/long-entities/LongEntitiesGantt.tsxui/packages/@quent/components/src/long-entities/types.tsui/packages/@quent/components/src/long-entities/utils.test.tsui/packages/@quent/components/src/long-entities/utils.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
| const clippedShape = clipBound ? clipRectByRect(rectShape, clipBound) : rectShape; | ||
| if (!clippedShape) return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- LongEntitiesGantt outline ---'
ast-grep outline ui/packages/@quent/components/src/long-entities/LongEntitiesGantt.tsx --match 'LongEntitiesGantt' --view expanded || true
printf '%s\n' '--- LongEntitiesGantt relevant lines ---'
sed -n '1,180p' ui/packages/@quent/components/src/long-entities/LongEntitiesGantt.tsx
printf '%s\n' '--- Gantt options relevant lines ---'
sed -n '1,220p' ui/packages/@quent/components/src/gantt-chart/options.ts
printf '%s\n' '--- related tests and call sites ---'
rg -n -S 'LongEntitiesGantt|segmentIndex|clipRectByRect|entity label|long entities' ui/packages/@quent/components ui/srcRepository: rapidsai/quent
Length of output: 12302
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- clipping utility ---'
sed -n '1,100p' ui/packages/@quent/components/src/gantt-chart/utils.ts
printf '%s\n' '--- GanttChart implementation ---'
ast-grep outline ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx --match 'GanttChart' --view expanded || true
sed -n '1,260p' ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx
printf '%s\n' '--- LongEntityEntry types and component tests ---'
sed -n '1,180p' ui/packages/@quent/components/src/long-entities/types.ts
fd -i 'LongEntitiesGantt' ui/packages/@quent/components ui/src || true
rg -n -S 'buildGanttOption|tooltip|dataZoom|LongEntityEntry' ui/packages/@quent/components/src/long-entities ui/packages/@quent/components/src/gantt-chart ui/srcRepository: rapidsai/quent
Length of output: 9751
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
gantt = Path("ui/packages/@quent/components/src/long-entities/LongEntitiesGantt.tsx").read_text()
options = Path("ui/packages/@quent/components/src/gantt-chart/options.ts").read_text()
chart = Path("ui/packages/@quent/components/src/gantt-chart/GanttChart.tsx").read_text()
clip_source = Path("ui/packages/@quent/components/src/gantt-chart/utils.ts").read_text()
assert "datum!.segmentIndex === 0" in gantt
assert "if (!clippedShape) return null;" in gantt
assert "axisLabel: { show: false }" in options
assert "renderTooltip" in chart
assert "renderTooltip={renderTooltip}" not in gantt
assert "return undefined;" in clip_source
# Model two segments for one entity. The first is fully outside the viewport;
# the second is visible. The render path therefore reaches the label condition
# with segmentIndex 1, which produces no label child.
bounds = {"x": 100, "y": 0, "width": 200, "height": 100}
segments = [
{"index": 0, "x": 0, "width": 50},
{"index": 1, "x": 120, "width": 50},
]
def clip(rect, bounds):
x = max(rect["x"], bounds["x"])
x2 = min(rect["x"] + rect["width"], bounds["x"] + bounds["width"])
y = max(rect.get("y", 0), bounds["y"])
y2 = min(rect.get("y", 0) + rect.get("height", 10), bounds["y"] + bounds["height"])
return None if x2 < x or y2 < y else {"x": x, "width": x2 - x}
visible = []
for segment in segments:
clipped = clip(segment, bounds)
if clipped is not None:
visible.append((segment["index"], segment["index"] == 0))
assert visible == [(1, False)]
print("PASS: the first segment is discarded outside the viewport, and the visible later segment has no label.")
print("PASS: the chart hides y-axis labels and LongEntitiesGantt does not provide renderTooltip.")
PYRepository: rapidsai/quent
Length of output: 345
Keep the entity label visible after horizontal navigation.
When the first segment is outside the zoom window, later visible segments do not render entry.label. Render the label on the first visible segment or provide a persistent row or tooltip label. Add regression coverage.
🤖 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/long-entities/LongEntitiesGantt.tsx around
lines 84 - 85, Update the segment-rendering logic around clippedShape in
LongEntitiesGantt so the entity label is rendered on the first segment that
remains visible after horizontal clipping, rather than only on the original
first segment. Preserve label visibility when navigating horizontally, and add
regression coverage for an offscreen first segment followed by a visible
segment.
There was a problem hiding this comment.
I'm going to include this in part 3, other refactors happen there
There was a problem hiding this comment.
@johallar, acknowledged. Keep this finding for part 3 with the related refactors. The current PR can remain focused on the initial Gantt rendering work.
🐇
You are interacting with an AI system.
There was a problem hiding this comment.
Also handle in part 3
There was a problem hiding this comment.
@johallar, acknowledged. Handle the entity-label visibility case and regression coverage in part 3.
🐇
You are interacting with an AI system.
| it('carries transition attributes onto segments', () => { | ||
| const fsm = makeFsm('e1', [ | ||
| transition('a', 0, { | ||
| attributes: [{ key: 'bytes', value: { Int: 42 } } as unknown as never], | ||
| }), | ||
| transition('exit', 1), | ||
| ]); | ||
| const [entry] = buildLongEntityEntries([fsm], {}, 'light'); | ||
| expect(entry.segments[0].attributes).toHaveLength(1); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a canonical metadata fixture and cover both attribute fields.
Line 107 casts a hand-written value through unknown to never. This disables the FsmTransition attribute contract. The test also omits derived_attributes, so a regression that drops derivedAttributes still passes. Build a typed DynamicAttribute fixture or use a production builder, then assert both output fields.
As per coding guidelines, fixtures must use canonical production or generated types. As per path instructions, build fixtures from canonical production or ts-binding types instead of brittle lookalike interfaces.
🤖 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/long-entities/utils.test.ts around lines
104 - 113, Update the “carries transition attributes onto segments” test to
construct its transition metadata with the canonical production or ts-binding
DynamicAttribute fixture/builder, removing the unknown-to-never cast. Include
both regular and derived attributes in the fixture, then assert the segment
preserves both attributes and derivedAttributes outputs.
Sources: Coding guidelines, Path instructions
| it('stacks non-overlapping entities onto the same row', () => { | ||
| const a = makeFsm('a', [transition('s', 0), transition('exit', 1)]); | ||
| const b = makeFsm('b', [transition('s', 2), transition('exit', 3)]); | ||
| const entries = buildLongEntityEntries([a, b], {}, 'light'); | ||
| expect(entries.map(e => e.rowIndex).sort()).toEqual([0, 0]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test adjacent intervals without a gap.
The current case leaves a one-second gap. Consecutive FSM state spans meet at the transition timestamp, so an entity ending at 1 and another starting at 1 must reuse the same row. Test that boundary to catch a < versus <= error in row packing.
Proposed test change
- const b = makeFsm('b', [transition('s', 2), transition('exit', 3)]);
+ const b = makeFsm('b', [transition('s', 1), transition('exit', 2)]);As per coding guidelines, tests must cover meaningful boundaries. As per path instructions, tests must cover meaningful boundary behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('stacks non-overlapping entities onto the same row', () => { | |
| const a = makeFsm('a', [transition('s', 0), transition('exit', 1)]); | |
| const b = makeFsm('b', [transition('s', 2), transition('exit', 3)]); | |
| const entries = buildLongEntityEntries([a, b], {}, 'light'); | |
| expect(entries.map(e => e.rowIndex).sort()).toEqual([0, 0]); | |
| }); | |
| it('stacks non-overlapping entities onto the same row', () => { | |
| const a = makeFsm('a', [transition('s', 0), transition('exit', 1)]); | |
| const b = makeFsm('b', [transition('s', 1), transition('exit', 2)]); | |
| const entries = buildLongEntityEntries([a, b], {}, 'light'); | |
| expect(entries.map(e => e.rowIndex).sort()).toEqual([0, 0]); | |
| }); |
🤖 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/long-entities/utils.test.ts around lines
115 - 120, Update the “stacks non-overlapping entities onto the same row” test
to use adjacent intervals whose end and start timestamps are equal, such as
ending the first FSM at 1 and starting the second at 1. Keep the expected
rowIndex values as [0, 0] to verify boundary-touching entities reuse the same
row.
Sources: Coding guidelines, Path instructions
Build the entity-specific visualization on the shared Gantt foundation so the common refactor remains independently reviewable.
Keep the new component files compliant with the repository copyright hook.
6cc1728 to
f208b00
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/merge |
2 participants
Description
Related Issues
relates to #215
Testing
N/A
Screenshots
N/A