refactor: hookify min zoom, use in all charts - #446
Conversation
|
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:
📝 WalkthroughWalkthroughTimeline charts now share minimum zoom-span calculation, enforce the minimum during shift-wheel zooming, support horizontal wheel panning, and clean up chart listeners through a reusable navigation hook. Timeline and operator Gantt chart integrations apply the shared span to ECharts dataZoom controls. ChangesTimeline zoom navigation
Estimated code review effort: 4 (Complex) | ~45 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.
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/lib/useTimelineWheelNavigation.ts-31-38 (1)
31-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
atZoomLimitgoes stale whenminZoomSpanPctchanges.
attachWheelNavigationis memoized with empty deps and only called fromonChartReady, soupdateZoomLimitre-evaluates exclusively on EChartsdatazoomevents. IfdurationSeconds(and thereforeminZoomSpanPct) changes while the chart instance is alive and already zoomed to the floor, the cached flag keeps blocking Shift+scroll zoom-in until the nextdatazoomevent — but such an event can no longer be produced, since zoom-in is what's being blocked.Reading the span at wheel time removes the cache, the listener, and the staleness in one go; ECharts' own
minSpanstill provides the hard floor.🐛 Proposed fix: evaluate the limit lazily
- let atZoomLimit = false; - const updateZoomLimit = () => { - if (instance.isDisposed?.()) return; - const dataZoom = getDataZoomState(instance); - if (!dataZoom) return; - const spanPct = (dataZoom.end ?? 100) - (dataZoom.start ?? 0); - atZoomLimit = spanPct <= minZoomSpanPctRef.current * 1.01; - }; + const isAtZoomLimit = () => { + if (instance.isDisposed?.()) return false; + const dataZoom = getDataZoomState(instance); + if (!dataZoom) return false; + const spanPct = (dataZoom.end ?? 100) - (dataZoom.start ?? 0); + return spanPct <= minZoomSpanPctRef.current * 1.01; + }; const handleWheel = (event: WheelEvent) => { if (event.shiftKey) { const zoomDelta = event.deltaY || event.deltaX; - if (zoomDelta < 0 && atZoomLimit) { + if (zoomDelta < 0 && isAtZoomLimit()) { event.preventDefault(); event.stopPropagation(); } return; }- updateZoomLimit(); - instance.on('datazoom', updateZoomLimit); wheelTarget.addEventListener('wheel', handleWheel, { capture: true, passive: false }); cleanupRef.current = () => { wheelTarget.removeEventListener('wheel', handleWheel, { capture: true }); - if (!instance.isDisposed?.()) instance.off('datazoom', updateZoomLimit); };Note:
useTimelineWheelNavigation.test.tswould need itsemitDataZoom/offassertions updated accordingly.🤖 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/lib/useTimelineWheelNavigation.ts around lines 31 - 38, Update attachWheelNavigation and its wheel-handler path to evaluate the current zoom span lazily when handling Shift+scroll, rather than caching atZoomLimit through updateZoomLimit or datazoom listeners. Remove the stale flag and related listener bookkeeping while preserving ECharts minSpan as the hard limit; adjust useTimelineWheelNavigation.test.ts expectations for the removed emitDataZoom/off behavior.
🧹 Nitpick comments (3)
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.ts (2)
114-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the pan clamp boundaries.
newStartis clamped byMath.max(0, Math.min(100 - spanPct, …))inuseTimelineWheelNavigation.ts(Line 67), and no test exercises either bound. Two cases — panning left atstart: 0and panning right atend: 100— pin the window to the range edges instead of drifting out of bounds.As per path instructions, tests should cover "observable behavior, fallback/unknown inputs, empty/error states" and "meaningful boundary coverage".
💚 Proposed additional cases
it.each([ { dataZoom: { start: 0, end: 50 }, deltaX: -100, expected: 0 }, { dataZoom: { start: 50, end: 100 }, deltaX: 100, expected: 50 }, ])('clamps panning at the range edges', ({ dataZoom, deltaX, expected }) => { const chart = createChart(dataZoom); const { result } = renderHook(() => useTimelineWheelNavigation(10)); act(() => result.current(chart.instance)); act(() => { chart.dom.dispatchEvent(new WheelEvent('wheel', { deltaX, bubbles: true, cancelable: true })); }); expect(chart.dispatchAction).toHaveBeenCalledWith( expect.objectContaining({ start: expected, end: expected + 50 }) ); });🤖 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/lib/useTimelineWheelNavigation.test.ts around lines 114 - 128, Add boundary coverage to the useTimelineWheelNavigation tests by parameterizing cases for panning left from start 0 and right from end 100. Dispatch the corresponding wheel events and assert chart.dispatchAction receives a window clamped to the range edges, preserving the existing span.Source: Path instructions
41-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpected
start: 30silently encodesTIMELINE_SPACING.The 1010px mock width and the expected 5% shift only line up because
TIMELINE_SPACING.left + right === 10. Importing the constant and computing the expected delta makes the assertion self-explanatory and immune to spacing changes.🤖 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/lib/useTimelineWheelNavigation.test.ts around lines 41 - 60, Update the horizontal wheel navigation test around useTimelineWheelNavigation to import and use the TIMELINE_SPACING constant when calculating the expected dataZoom start/end shift, rather than hardcoding start: 30 and end: 80. Preserve the existing event prevention and dispatch assertions while deriving the delta from the mock chart width and configured spacing.ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
1.01tolerance.The magic factor absorbs float drift between the dispatched span and ECharts' clamped span; a terse comment (or a named constant) keeps the intent from being read as an arbitrary fudge.
🤖 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/lib/useTimelineWheelNavigation.ts at line 37, Document the purpose of the 1.01 tolerance in the atZoomLimit calculation within useTimelineWheelNavigation, explaining that it compensates for floating-point drift between the dispatched span and ECharts’ clamped span. Use a concise inline comment or a clearly named constant without changing the existing behavior.
🤖 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.
Other comments:
In `@ui/packages/`@quent/components/src/lib/useTimelineWheelNavigation.ts:
- Around line 31-38: Update attachWheelNavigation and its wheel-handler path to
evaluate the current zoom span lazily when handling Shift+scroll, rather than
caching atZoomLimit through updateZoomLimit or datazoom listeners. Remove the
stale flag and related listener bookkeeping while preserving ECharts minSpan as
the hard limit; adjust useTimelineWheelNavigation.test.ts expectations for the
removed emitDataZoom/off behavior.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/lib/useTimelineWheelNavigation.test.ts:
- Around line 114-128: Add boundary coverage to the useTimelineWheelNavigation
tests by parameterizing cases for panning left from start 0 and right from end
100. Dispatch the corresponding wheel events and assert chart.dispatchAction
receives a window clamped to the range edges, preserving the existing span.
- Around line 41-60: Update the horizontal wheel navigation test around
useTimelineWheelNavigation to import and use the TIMELINE_SPACING constant when
calculating the expected dataZoom start/end shift, rather than hardcoding start:
30 and end: 80. Preserve the existing event prevention and dispatch assertions
while deriving the delta from the mock chart width and configured spacing.
In `@ui/packages/`@quent/components/src/lib/useTimelineWheelNavigation.ts:
- Line 37: Document the purpose of the 1.01 tolerance in the atZoomLimit
calculation within useTimelineWheelNavigation, explaining that it compensates
for floating-point drift between the dispatched span and ECharts’ clamped span.
Use a concise inline comment or a clearly named constant without changing the
existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 2b96eff3-6605-4fc9-adea-3edad0cba177
📒 Files selected for processing (7)
ui/packages/@quent/components/src/lib/useMinZoomSpanPct.test.tsui/packages/@quent/components/src/lib/useMinZoomSpanPct.tsui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.tsui/packages/@quent/components/src/lib/useTimelineWheelNavigation.tsui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsxui/packages/@quent/components/src/timeline/Timeline.tsxui/packages/@quent/components/src/timeline/TimelineController.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts (2)
27-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecompute the zoom-limit state when the minimum changes.
atZoomLimitis cached only on attachment anddatazoomevents. IfminZoomSpanPctchanges, the cached value can remain permanently stale—for example, a lowered minimum can remain blocked indefinitely, while a raised minimum may allow another zoom step. Derive the limit from the currentdataZoomstate in the wheel handler or synchronize it whenever the minimum changes.🤖 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/lib/useTimelineWheelNavigation.ts around lines 27 - 34, Update the zoom-limit handling in useTimelineWheelNavigation so changes to minZoomSpanPctRef.current are reflected before processing each wheel action. Recompute atZoomLimit from the current getDataZoomState(instance) and latest minimum, or synchronize it when the minimum changes, while preserving the existing disposed-instance and missing-data guards.
46-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not consume wheel input unless navigation succeeds.
The handler currently swallows events that it cannot handle, which breaks parent/native scrolling. Validate the gesture, chart state, and resulting range before calling
stopPropagation()orpreventDefault().
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts#L46-L70: defer event consumption until after confirming horizontaldataZoomnavigation and a changed range.ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.ts#L40-L76: add vertical propagation and boundary/no-dataZoomcases.🤖 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/lib/useTimelineWheelNavigation.ts around lines 46 - 70, Defer event consumption in the wheel handler of useTimelineWheelNavigation until after validating a horizontal gesture, a live chart with dataZoom state, and a changed clamped range; only then call stopPropagation, preventDefault, and dispatch dataZoom. In ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts lines 46-70, preserve propagation for vertical, boundary, disposed, missing-dataZoom, and no-op navigation cases. In ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.ts lines 40-76, add coverage confirming vertical gestures and boundary/no-dataZoom cases are not consumed.
🧹 Nitpick comments (1)
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.ts (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the ECharts mock structurally typed.
as unknown as EChartsTypebypasses checks on the mockedgetOption,on,off,dispatchAction, andisDisposedsurface. Use aPick<EChartsType, ...>/Partial<EChartsType>fixture withsatisfies, or a shared typed builder instead.🤖 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/lib/useTimelineWheelNavigation.test.ts at line 35, Update the ECharts mock in the useTimelineWheelNavigation test to use a structurally typed fixture, such as Pick or Partial of EChartsType with satisfies, covering getOption, on, off, dispatchAction, and isDisposed. Remove the as unknown as EChartsType cast while preserving the mock 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.
Outside diff comments:
In `@ui/packages/`@quent/components/src/lib/useTimelineWheelNavigation.ts:
- Around line 27-34: Update the zoom-limit handling in
useTimelineWheelNavigation so changes to minZoomSpanPctRef.current are reflected
before processing each wheel action. Recompute atZoomLimit from the current
getDataZoomState(instance) and latest minimum, or synchronize it when the
minimum changes, while preserving the existing disposed-instance and
missing-data guards.
- Around line 46-70: Defer event consumption in the wheel handler of
useTimelineWheelNavigation until after validating a horizontal gesture, a live
chart with dataZoom state, and a changed clamped range; only then call
stopPropagation, preventDefault, and dispatch dataZoom. In
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts lines 46-70,
preserve propagation for vertical, boundary, disposed, missing-dataZoom, and
no-op navigation cases. In
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.ts lines
40-76, add coverage confirming vertical gestures and boundary/no-dataZoom cases
are not consumed.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/lib/useTimelineWheelNavigation.test.ts:
- Line 35: Update the ECharts mock in the useTimelineWheelNavigation test to use
a structurally typed fixture, such as Pick or Partial of EChartsType with
satisfies, covering getOption, on, off, dispatchAction, and isDisposed. Remove
the as unknown as EChartsType cast while preserving the mock behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 41e819e6-fc3d-4928-9399-3400e4958198
📒 Files selected for processing (2)
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.tsui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts
There was a problem hiding this comment.
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/lib/useTimelineWheelNavigation.ts (1)
37-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not intercept vertical wheel events.
Line 47 stops propagation before Line 48 confirms horizontal dominance. Vertical/diagonal wheel events therefore cannot reach parent wheel handlers despite being otherwise ignored. Move
stopPropagation()below the guard and assert this in the vertical-wheel test.Proposed fix
- event.stopPropagation(); if (event.deltaX === 0 || Math.abs(event.deltaX) <= Math.abs(event.deltaY)) return; + event.stopPropagation();🤖 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/lib/useTimelineWheelNavigation.ts around lines 37 - 48, Update handleWheel so the event.stopPropagation() call occurs only after the event.deltaX zero/vertical-dominance guard, allowing vertical and non-horizontal-dominant wheel events to reach parent handlers. Preserve horizontal handling and add or update the vertical-wheel test to assert propagation is not intercepted.
🧹 Nitpick comments (1)
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.ts (1)
12-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the ECharts fixture from its canonical API.
as unknown as EChartsTypelets this mock drift from the methods the hook consumes. Define it withPick<EChartsType, ...>orPartial<EChartsType>before the final narrow cast. As per coding guidelines, “Build test fixtures from canonical production or generated types usingPick,Partial, or shared builders instead of 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/lib/useTimelineWheelNavigation.test.ts around lines 12 - 32, Update the createChart fixture to derive its type from the canonical EChartsType API using Pick or Partial for the methods it supplies, then apply only the necessary narrow cast to EChartsType. Replace the current unknown-to-EChartsType cast while preserving the existing getDom, getOption, dispatchAction, and isDisposed behavior.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.
Outside diff comments:
In `@ui/packages/`@quent/components/src/lib/useTimelineWheelNavigation.ts:
- Around line 37-48: Update handleWheel so the event.stopPropagation() call
occurs only after the event.deltaX zero/vertical-dominance guard, allowing
vertical and non-horizontal-dominant wheel events to reach parent handlers.
Preserve horizontal handling and add or update the vertical-wheel test to assert
propagation is not intercepted.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/lib/useTimelineWheelNavigation.test.ts:
- Around line 12-32: Update the createChart fixture to derive its type from the
canonical EChartsType API using Pick or Partial for the methods it supplies,
then apply only the necessary narrow cast to EChartsType. Replace the current
unknown-to-EChartsType cast while preserving the existing getDom, getOption,
dispatchAction, and isDisposed behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: f30b5e39-48d3-4d81-a050-a3ead258bb6a
📒 Files selected for processing (2)
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.tsui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts
cmatzenbach
left a comment
cmatzenbach
left a comment
There was a problem hiding this comment.
Two comments, nothing critical though. Works really well, great job solving the infinite scrolling issue.
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/packages/`@quent/components/src/lib/useTimelineWheelNavigation.ts:
- Around line 41-47: Shifted vertical wheel input currently reaches ECharts
instead of panning. In useTimelineWheelNavigation, convert shifted vertical
deltaY into the horizontal pan delta and intercept the event before ECharts
handling; update useTimelineWheelNavigation.test.ts ranges 88-110 and 128-163 to
assert shifted input pans, does not invoke the ECharts wheel handler, and
preserves minimum-span behavior through panning.
🪄 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: e4769929-49d4-4312-a24e-707ee238999e
📒 Files selected for processing (2)
ui/packages/@quent/components/src/lib/useTimelineWheelNavigation.test.tsui/packages/@quent/components/src/lib/useTimelineWheelNavigation.ts
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/packages/`@quent/components/src/operator-timeline/OperatorGanttChart.tsx:
- Around line 343-346: The OperatorGanttChart cleanup must explicitly detach
listeners when operators becomes empty or the chart instance is replaced. Update
the onChartReady/instance lifecycle around registerAxisPointerSync and
attachWheelNavigation to retain the cleanup handles and invoke them before
rendering the empty state and before attaching a new instance, rather than
relying on instanceRef.current changes or final unmount cleanup.
🪄 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: 504205c7-6252-4dca-91a7-b1ac45eed534
📒 Files selected for processing (1)
ui/packages/@quent/components/src/operator-timeline/OperatorGanttChart.tsx
|
/merge |
# Description * Upgrades echarts to 6.x * In order to do this we had to ditch the decimal millisecond timestamps which did not render correctly in 6. This now uses value type axes and use the "relative to start" seconds for the axes values (not timestamps). * This also allows us to have more precise bins, i've made them 10ns (previously 250) * Removes the `startTime` param since everything uses the relative seconds value now, big win IMO * Removes ignored vulnerability ## Related Issues ## Testing * Timelines zoom as before (can zoom in further now but should still limit zoom as before) * DAG data flow views work as before Note: #445 Exists still, so the operator timeline won't stop you at the max zoom level and will break things, but will tackle that bug separately in #446 ## Screenshots Authors: - Joe O'Hallaron (https://github.com/johallar) Approvers: - Chris Matzenbach (https://github.com/cmatzenbach) URL: #447
2 participants
Description
Related Issues
Fixes #445
Testing
Screenshots
Screen.Recording.2026-07-27.at.12.54.20.PM.mov