Tooltip cursor#338
Conversation
- Replace InteractionEvent with FederatedPointerEvent - Update event API: event.data.X -> event.X, event.data.originalEvent -> event.nativeEvent - Replace deprecated 'transparent' with 'backgroundAlpha' - Cast application.view as HTMLCanvasElement (ICanvas change) - Replace PIXI.AbstractRenderer with PIXI.IRenderer - Fix fontWeight type (string -> 'bold' as const / '900' as const) - Replace buttonMode with cursor='pointer' - Use globalmousemove for reliable stage event delivery in v7 - Add stage.hitArea for full-canvas pointer event coverage - Add pixi-v7-augment.d.ts for DisplayObject event mixin types - Update test snapshots for v7 internals - Cursor now follows mouse position continuously Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
- New TimeGraphChartRichCursor layer that follows mouse position - Draws vertical line, dots on intersecting states, and label tooltips - Operates in screen coordinates (unaffected by viewport scroll/scale) - Added to example application Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Prevents warnings from pixi.js packages missing source maps. Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR migrates timeline-chart to Pixi v7: adds TypeScript augmentations, replaces legacy InteractionEvent handlers with FederatedPointerEvent across components, refactors chart mouse wiring, adds XY-series types/components and XY rendering, implements TimeGraphChartRichCursor with interpolated XY indicators and tooltips, updates container/font/build metadata, and adds tests and example wiring. ChangesPixi v7 Migration and Rich Cursor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
timeline-chart/src/layer/__tests__/time-graph-chart-rich-cursor.test.ts (1)
165-177: ⚡ Quick winConsider asserting the tooltip position for edge-aware behavior.
The test verifies that a tooltip is drawn near the right edge, but doesn't assert that it's actually positioned to the left of the cursor as the test name suggests. Consider adding an assertion on
bg.xto verify the edge-aware positioning logic.📍 Suggested assertion to verify left-side positioning
// Draw near right edge (canvas is 800px wide) (cursor as any).drawCursor(790); const bg = (cursor as any).tooltipContainer.children[0] as PIXI.Graphics; - // The tooltip box should be positioned to the left of cursor - expect(bg).toBeTruthy(); + // The tooltip box should be positioned to the left of cursor + expect(bg).toBeTruthy(); + expect(bg.x).toBeLessThan(790);🤖 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 `@timeline-chart/src/layer/__tests__/time-graph-chart-rich-cursor.test.ts` around lines 165 - 177, The test 'flips tooltip to left when near right edge' currently only checks that the tooltip background exists; update it to assert the tooltip is positioned left of the cursor by comparing the bg.x (from (cursor as any).tooltipContainer.children[0]) to the cursor draw x (use the same 790 value or read a public/accessible cursor property like (cursor as any).lastX if available) and expect bg.x to be less than the cursor x (or less than 790 minus the tooltip width if you can read it) to verify edge-aware flipping logic in drawCursor.timeline-chart/src/layer/time-graph-chart-rich-cursor.ts (1)
93-118: ⚖️ Poor tradeoffConsider object pooling for tooltip elements.
New
PIXI.TextandPIXI.Graphicsobjects are created on everymousemoveevent. With many visible rows, this could cause performance issues or GC pressure. Consider reusing objects or implementing an object pool, especially if users frequently move the mouse across the chart.🤖 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 `@timeline-chart/src/layer/time-graph-chart-rich-cursor.ts` around lines 93 - 118, This code creates new PIXI.Text (textObj) and PIXI.Graphics (bg) on every mousemove and should instead reuse pooled/displayed tooltip objects to avoid GC and allocation spikes: add persistent members (e.g., this.tooltipText and this.tooltipBg) or an object pool used by the mousemove handler to update text/geometry and reposition the existing PIXI.Text and PIXI.Graphics rather than creating new ones, ensure tooltipContainer.addChild is called only once (or check contains) and update textObj.text, textObj.x/y and bg.clear()/bg.drawRoundedRect(...) as needed when showing/hiding the tooltip.
🤖 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 `@timeline-chart/src/layer/time-graph-chart-rich-cursor.ts`:
- Around line 64-65: The code is reaching into a private field by casting
chartLayer to any to access rowComponents; add a public accessor on
TimeGraphChart (e.g., getRowComponent(rowId: string) or a method that returns
the needed position data) that wraps access to the internal rowComponents map,
then replace the cast in time-graph-chart-rich-cursor.ts where it does
(this.chartLayer as any).rowComponents.get(hit.row.id) with a call to the new
public method (e.g., this.chartLayer.getRowComponent(hit.row.id)) and handle
undefined the same way as before.
In `@timeline-chart/src/layer/time-graph-chart.ts`:
- Around line 222-231: The inline mouseover/mouseout handlers on this.stage
(which set triggerKeyEvent and update this.stage.cursor and check
this.mousePanning) must be stored as named functions on the instance (e.g.,
this._onStageMouseOver / this._onStageMouseOut) so they can be removed later,
and then unregistered in the component's destroy/cleanup method using
this.stage.off('mouseover', this._onStageMouseOver) and
this.stage.off('mouseout', this._onStageMouseOut); apply the same change for the
other occurrence of these handlers in the file so re-init won't retain stale
listeners or duplicate behavior.
- Around line 1125-1126: The lookup for a matching state uses an inclusive end
check which can match adjacent states at boundaries; in the row.states.find used
in time-graph-chart (the expression referencing row.states, state.range.start,
state.range.end and timestamp) change the condition from using an inclusive end
(state.range.end >= timestamp) to an end-exclusive check (state.range.end >
timestamp) so it matches the rest of the file's end-exclusive semantics and
prevents boundary collisions; update the predicate accordingly and run unit/edge
tests around timestamp boundaries to confirm behavior.
In `@timeline-chart/src/time-graph-font-controller.ts`:
- Line 9: Two fontWeight values are inconsistent—change the dynamic creation
that currently uses '900' to match the default whose fontWeight is "bold";
update the fontWeight property in the default font object and the fontWeight
used when dynamically creating fonts so both use "bold" (search for occurrences
of fontWeight: '900' and the default fontWeight: "bold" and make them
identical).
---
Nitpick comments:
In `@timeline-chart/src/layer/__tests__/time-graph-chart-rich-cursor.test.ts`:
- Around line 165-177: The test 'flips tooltip to left when near right edge'
currently only checks that the tooltip background exists; update it to assert
the tooltip is positioned left of the cursor by comparing the bg.x (from (cursor
as any).tooltipContainer.children[0]) to the cursor draw x (use the same 790
value or read a public/accessible cursor property like (cursor as any).lastX if
available) and expect bg.x to be less than the cursor x (or less than 790 minus
the tooltip width if you can read it) to verify edge-aware flipping logic in
drawCursor.
In `@timeline-chart/src/layer/time-graph-chart-rich-cursor.ts`:
- Around line 93-118: This code creates new PIXI.Text (textObj) and
PIXI.Graphics (bg) on every mousemove and should instead reuse pooled/displayed
tooltip objects to avoid GC and allocation spikes: add persistent members (e.g.,
this.tooltipText and this.tooltipBg) or an object pool used by the mousemove
handler to update text/geometry and reposition the existing PIXI.Text and
PIXI.Graphics rather than creating new ones, ensure tooltipContainer.addChild is
called only once (or check contains) and update textObj.text, textObj.x/y and
bg.clear()/bg.drawRoundedRect(...) as needed when showing/hiding the tooltip.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f79501a-23d4-4ccb-a501-f3ec6b3abd5f
⛔ Files ignored due to path filters (3)
timeline-chart/src/components/__tests__/__snapshots__/time-graph-arrow.test.ts.snapis excluded by!**/*.snaptimeline-chart/src/components/__tests__/__snapshots__/time-graph-axis-cursor.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
example/src/index.tsexample/webpack.config.jspackage.jsontimeline-chart/package.jsontimeline-chart/src/__tests__/pixi-v7-migration.test.tstimeline-chart/src/components/time-graph-component.tstimeline-chart/src/layer/__tests__/time-graph-chart-rich-cursor.test.tstimeline-chart/src/layer/time-graph-chart-cursors.tstimeline-chart/src/layer/time-graph-chart-rich-cursor.tstimeline-chart/src/layer/time-graph-chart.tstimeline-chart/src/layer/time-graph-viewport-layer.tstimeline-chart/src/pixi-v7-augment.d.tstimeline-chart/src/time-graph-container.tstimeline-chart/src/time-graph-font-controller.ts
- Add TimeGraphChartRichCursor: grey hover cursor with dots and labels - Hide blue cursor lines when not actively selecting a range (start===end) - Remove hover-tracking selectionRange updates from cursors layer - Rich cursor hides during mouse-button drag for clean selection UX - Set eventMode='none' on rich cursor graphics to not block stage events
There was a problem hiding this comment.
🧹 Nitpick comments (1)
timeline-chart/src/layer/time-graph-chart-cursors.ts (1)
110-111: 💤 Low valuePrefer reading modifier keys from the federated event directly.
The mousedown handler at lines 80–84 reads
event.shiftKey,event.ctrlKey, andevent.altKeydirectly from theFederatedPointerEvent. However, the globalmousemove and mouseup handlers cast toevent.nativeEventfor the same reads—an unnecessary inconsistency. PixiJS v7'sFederatedPointerEventAPI exposes these modifier properties directly, eliminating the need for the cast.♻️ Proposed simplification
if ((this.mouseButtons & 1) === 0) { // handle missed button mouseup event this.mouseSelecting = false; - const orig = event.nativeEvent as PointerEvent; - if (!orig.shiftKey || orig.ctrlKey || orig.altKey) { + if (!event.shiftKey || event.ctrlKey || event.altKey) { this.stage.cursor = 'default'; } return; }if (this.mouseSelecting && event.button === 0) { this.mouseSelecting = false; - const orig = event.nativeEvent as PointerEvent; - if (!orig.shiftKey || orig.ctrlKey || orig.altKey) { + if (!event.shiftKey || event.ctrlKey || event.altKey) { this.stage.cursor = 'default'; } }Also applies to: 132-133
🤖 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 `@timeline-chart/src/layer/time-graph-chart-cursors.ts` around lines 110 - 111, The globalmousemove and mouseup handlers currently cast event.nativeEvent to PointerEvent to read modifier keys; change them to read shiftKey/ctrlKey/altKey directly from the FederatedPointerEvent (the event parameter) — i.e., remove the `const orig = event.nativeEvent as PointerEvent` pattern and use `event.shiftKey`, `event.ctrlKey`, `event.altKey` in the globalmousemove and mouseup handlers so they match the mousedown handler and PixiJS v7's FederatedPointerEvent API.
🤖 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.
Nitpick comments:
In `@timeline-chart/src/layer/time-graph-chart-cursors.ts`:
- Around line 110-111: The globalmousemove and mouseup handlers currently cast
event.nativeEvent to PointerEvent to read modifier keys; change them to read
shiftKey/ctrlKey/altKey directly from the FederatedPointerEvent (the event
parameter) — i.e., remove the `const orig = event.nativeEvent as PointerEvent`
pattern and use `event.shiftKey`, `event.ctrlKey`, `event.altKey` in the
globalmousemove and mouseup handlers so they match the mousedown handler and
PixiJS v7's FederatedPointerEvent API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12551653-7d0e-440d-ad92-a088c425cb86
📒 Files selected for processing (2)
timeline-chart/src/layer/time-graph-chart-cursors.tstimeline-chart/src/layer/time-graph-chart-rich-cursor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- timeline-chart/src/layer/time-graph-chart-rich-cursor.ts
|
Without having understood the entire code change, I feel like we are mixing two aspects into one here: Cursors and markers. In the view there is this grey mouse marker with the new overlay label and one or two cursors which are sync across all time-based views. With a mouse click I'd expect a blue cursor is set which does not move along with a mouse move. A second click sets the second blue cursor. Additionally, the grey cursor always moves along with the mouse pointer. Combined with either the shift-modifier or while holding the mouse button down, the range gets selected between the first cursor and the current mouse pointer position. |
- Add TimeGraphXYComponent for rendering line plots within rows - Add XY series model (TimeGraphXYPoint, TimeGraphXYSeries) to row model - Render XY plots only on empty rows (no states) - Rich cursor shows interpolated XY value on hover with tooltip - Example: two EKG-style swimlanes (Heart Rate, Respiration)
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)
timeline-chart/src/layer/time-graph-chart-rich-cursor.ts (1)
47-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing cleanup of
pointerleavehandler indestroy().The
pointerleavehandler registered at line 47 is an anonymous function and is not tracked or removed indestroy(). This creates a potential memory leak if the cursor layer is destroyed while the canvas remains.🔧 Proposed fix
export class TimeGraphChartRichCursor extends TimeGraphChartLayer { private graphics: PIXI.Graphics; private tooltipContainer: PIXI.Container; private _mouseMoveHandler: (event: Event) => void; + private _pointerLeaveHandler: (event: Event) => void; private lastMouseX: number = -1; private isMouseInCanvas: boolean = false;- this.onCanvasEvent('pointerleave', () => { + this._pointerLeaveHandler = () => { this.graphics.clear(); this.removeTooltips(); this.isMouseInCanvas = false; - }); + }; + this.onCanvasEvent('pointerleave', this._pointerLeaveHandler);destroy() { if (this._mouseMoveHandler) { this.removeOnCanvasEvent('pointermove', this._mouseMoveHandler); } + if (this._pointerLeaveHandler) { + this.removeOnCanvasEvent('pointerleave', this._pointerLeaveHandler); + } super.destroy(); }Also applies to: 191-196
🤖 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 `@timeline-chart/src/layer/time-graph-chart-rich-cursor.ts` around lines 47 - 51, The pointerleave handler passed as an anonymous function to this.onCanvasEvent('pointerleave', ...) is never removed in destroy(), causing a leak; capture that handler in a variable (e.g. this._onPointerLeave) when registering and unregister it in destroy() by calling the corresponding remover (e.g. this.offCanvasEvent('pointerleave', this._onPointerLeave) or this.canvas.removeEventListener if onCanvasEvent wraps DOM listeners); do the same for the other pointerleave registration around the other block (lines ~191-196) so both handlers are tracked and cleaned up in destroy().timeline-chart/src/layer/time-graph-chart-cursors.ts (1)
298-300:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncorrect event type in handler cleanup.
_cursorKeyUpHandlerwas registered on'keyup'(line 75) but is being removed from'mousedown'. This means the keyup handler will never be properly removed, causing it to remain attached afterdestroy().🐛 Proposed fix
if (this._cursorKeyUpHandler) { - this.removeOnCanvasEvent('mousedown', this._cursorKeyUpHandler); + this.removeOnCanvasEvent('keyup', this._cursorKeyUpHandler); }🤖 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 `@timeline-chart/src/layer/time-graph-chart-cursors.ts` around lines 298 - 300, The cleanup is removing the wrong event type: the handler stored in _cursorKeyUpHandler was registered for 'keyup' but the code calls removeOnCanvasEvent('mousedown', this._cursorKeyUpHandler); update the call to removeOnCanvasEvent to use the 'keyup' event so _cursorKeyUpHandler is properly removed during destroy() (locate usages in the same file alongside the registration of _cursorKeyUpHandler).
🧹 Nitpick comments (1)
timeline-chart/src/layer/time-graph-chart-rich-cursor.ts (1)
164-170: 💤 Low valueTooltip Y position may overflow canvas bounds.
boxYis computed asy - boxHeight / 2without clamping, which could position tooltips partially or fully outside the visible canvas area when the cursor is near the top or bottom edges.🔧 Suggested clamping
let boxY = y - boxHeight / 2; + const canvasHeight = this.stateController.canvasDisplayHeight; + if (boxY < 0) { + boxY = 0; + } else if (boxY + boxHeight > canvasHeight) { + boxY = canvasHeight - boxHeight; + }🤖 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 `@timeline-chart/src/layer/time-graph-chart-rich-cursor.ts` around lines 164 - 170, Tooltip vertical position (boxY) can overflow the canvas when the cursor is near the top or bottom; clamp the computed boxY (currently set as y - boxHeight / 2) to the visible area by ensuring boxY is no less than 0 and no greater than canvasHeight - boxHeight (use y, boxHeight, canvasHeight and the existing boxY calculation), so the tooltip is always fully inside the canvas while preserving the horizontal flip logic that uses boxX and boxWidth.
🤖 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 `@timeline-chart/src/layer/time-graph-chart-cursors.ts`:
- Around line 298-300: The cleanup is removing the wrong event type: the handler
stored in _cursorKeyUpHandler was registered for 'keyup' but the code calls
removeOnCanvasEvent('mousedown', this._cursorKeyUpHandler); update the call to
removeOnCanvasEvent to use the 'keyup' event so _cursorKeyUpHandler is properly
removed during destroy() (locate usages in the same file alongside the
registration of _cursorKeyUpHandler).
In `@timeline-chart/src/layer/time-graph-chart-rich-cursor.ts`:
- Around line 47-51: The pointerleave handler passed as an anonymous function to
this.onCanvasEvent('pointerleave', ...) is never removed in destroy(), causing a
leak; capture that handler in a variable (e.g. this._onPointerLeave) when
registering and unregister it in destroy() by calling the corresponding remover
(e.g. this.offCanvasEvent('pointerleave', this._onPointerLeave) or
this.canvas.removeEventListener if onCanvasEvent wraps DOM listeners); do the
same for the other pointerleave registration around the other block (lines
~191-196) so both handlers are tracked and cleaned up in destroy().
---
Nitpick comments:
In `@timeline-chart/src/layer/time-graph-chart-rich-cursor.ts`:
- Around line 164-170: Tooltip vertical position (boxY) can overflow the canvas
when the cursor is near the top or bottom; clamp the computed boxY (currently
set as y - boxHeight / 2) to the visible area by ensuring boxY is no less than 0
and no greater than canvasHeight - boxHeight (use y, boxHeight, canvasHeight and
the existing boxY calculation), so the tooltip is always fully inside the canvas
while preserving the horizontal flip logic that uses boxX and boxWidth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c980156d-57be-4afc-92aa-98b834625de6
📒 Files selected for processing (8)
example/src/test-data-provider.tstimeline-chart/src/components/time-graph-component.tstimeline-chart/src/components/time-graph-row.tstimeline-chart/src/components/time-graph-xy.tstimeline-chart/src/layer/time-graph-chart-cursors.tstimeline-chart/src/layer/time-graph-chart-rich-cursor.tstimeline-chart/src/layer/time-graph-chart.tstimeline-chart/src/time-graph-model.ts
The original way of doing it is mouse down == start select, mouse up == end select. The grey follows the mouse as it is a "tooltip cursor" |
MatthewKhouzam
force-pushed
the
tooltip-cursor
branch
from
1dd7c6b to
8506539
Compare
Agree, I think we are aligned on the requirements. In my local tests on Mac it just didn't work exactly like that. And I got confused why mouse move events seem to operate on the cursor positions rather then having this entirely separated. |
please understand I'm in goofing around mode and trying to align with what I understood was a common req too. :) |
2 participants
What it does
How to test
Follow-ups
Review checklist
Summary by CodeRabbit
New Features
Improvements
Tests
Chores