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>
📝 WalkthroughWalkthroughThis PR migrates timeline-chart to Pixi v7: adds type augmentations, changes interaction handlers to PIXI.FederatedPointerEvent, refactors global mouse wiring, adjusts container/font code, implements TimeGraphChartRichCursor (vertical cursor, per-row dots, tooltips), adds tests, updates example wiring, and updates build/dependency metadata. 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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
| const rowComponent = (this.chartLayer as any).rowComponents.get(hit.row.id); | ||
| if (!rowComponent) return; |
There was a problem hiding this comment.
Avoid accessing private members via type assertion.
Line 64 breaks encapsulation by casting chartLayer to any to access the private rowComponents field. This creates tight coupling and will break if the internal structure of TimeGraphChart changes.
Consider adding a public method to TimeGraphChart to retrieve row components by ID, or expose a method that returns the necessary position data for a given row.
🔧 Suggested approach
Add a public method to TimeGraphChart:
// In TimeGraphChart class
public getRowComponent(rowId: string): { position: { x: number, y: number } } | undefined {
return this.rowComponents.get(rowId);
}Then update this file:
-const rowComponent = (this.chartLayer as any).rowComponents.get(hit.row.id);
+const rowComponent = this.chartLayer.getRowComponent(hit.row.id);🤖 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 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.
| this.stage.on('mouseover', () => { | ||
| triggerKeyEvent = true; | ||
| }); | ||
|
|
||
| this.stage.addListener('mouseout', (event: MouseEvent) => { | ||
| this.stage.on('mouseout', () => { | ||
| triggerKeyEvent = false; | ||
| if (this.stage.cursor === 'grabbing' && !this.mousePanning) { | ||
| this.stage.cursor = 'default'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Persist stage hover handlers and unregister them on destroy.
mouseover/mouseout are registered with inline functions, so they cannot be removed later. This can retain stale chart instances and duplicate behavior after re-init.
🔧 Suggested fix
@@
- private _stageMouseDownHandler: Function;
- private _stageMouseMoveHandler: Function;
- private _stageMouseUpHandler: Function;
+ private _stageMouseDownHandler: Function;
+ private _stageMouseMoveHandler: Function;
+ private _stageMouseUpHandler: Function;
+ private _stageMouseOverHandler: Function;
+ private _stageMouseOutHandler: Function;
@@
- this.stage.on('mouseover', () => {
+ this._stageMouseOverHandler = () => {
triggerKeyEvent = true;
- });
+ };
+ this.stage.on('mouseover', this._stageMouseOverHandler);
- this.stage.on('mouseout', () => {
+ this._stageMouseOutHandler = () => {
triggerKeyEvent = false;
if (this.stage.cursor === 'grabbing' && !this.mousePanning) {
this.stage.cursor = 'default';
}
- });
+ };
+ this.stage.on('mouseout', this._stageMouseOutHandler);
@@
if (this.stage) {
+ this.stage.off('mouseover', this._stageMouseOverHandler);
+ this.stage.off('mouseout', this._stageMouseOutHandler);
this.stage.off('mousedown', this._stageMouseDownHandler);
this.stage.off('globalmousemove', this._stageMouseMoveHandler);
this.stage.off('mouseup', this._stageMouseUpHandler);
this.stage.off('mouseupoutside', this._stageMouseUpHandler);
}Also applies to: 466-470
🤖 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.ts` around lines 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.
| const state = row.states.find(s => s.range.start <= timestamp && s.range.end >= timestamp && s.data?.style); | ||
| if (state) { |
There was a problem hiding this comment.
Use end-exclusive range check for timestamp lookup consistency.
range.end >= timestamp can match two adjacent states at a boundary. Other selection logic in this file treats end as exclusive.
🔧 Suggested fix
- const state = row.states.find(s => s.range.start <= timestamp && s.range.end >= timestamp && s.data?.style);
+ const state = row.states.find(s => s.range.start <= timestamp && s.range.end > timestamp && s.data?.style);📝 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.
| const state = row.states.find(s => s.range.start <= timestamp && s.range.end >= timestamp && s.data?.style); | |
| if (state) { | |
| const state = row.states.find(s => s.range.start <= timestamp && s.range.end > timestamp && s.data?.style); | |
| if (state) { |
🤖 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.ts` around lines 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.
| fontSize: 8, | ||
| fill: "white", | ||
| fontWeight: "bold" | ||
| fontWeight: "bold" as const |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Pixi v7's accepted fontWeight values and whether "bold" vs '900' causes issues
# Search for other fontWeight usages in the codebase
rg -n "fontWeight" --type=ts -C2Repository: eclipse-cdt-cloud/timeline-chart
Length of output: 897
🏁 Script executed:
cat -n timeline-chart/src/time-graph-font-controller.tsRepository: eclipse-cdt-cloud/timeline-chart
Length of output: 5126
🏁 Script executed:
# Check if there are any type definitions or comments about fontWeight
rg -n "fontWeight|BitmapFont" timeline-chart/src/time-graph-font-controller.ts -B2 -A2Repository: eclipse-cdt-cloud/timeline-chart
Length of output: 650
🏁 Script executed:
# Check what version of Pixi.js is being used
rg -n "pixi" timeline-chart/package.jsonRepository: eclipse-cdt-cloud/timeline-chart
Length of output: 110
🌐 Web query:
Pixi.js BitmapFont fontWeight "bold" vs '900' accepted values CSS font weight
💡 Result:
Pixi.js BitmapFont (used via BitmapFont/BitmapText) accepts the same TextStyle.fontWeight values as Pixi’s text system: 'normal' | 'bold' | 'bolder' | 'lighter' | '100'…'900' (string literals). Specifically, both 'bold' and '900' are valid accepted inputs: 'bold' corresponds to 700, while '900' corresponds to heavy/black. [1][2] So for Pixi: use either - fontWeight: 'bold' (equivalent to weight 700) [1] - fontWeight: '900' (heavy/black) [1] CSS font-weight: numeric values range 1–1000 and bold is equivalent to 700. [3] Practical note for BitmapFont generation: Pixi’s BitmapFont generation uses style properties to build the bitmap glyphs; fontWeight is one of the style-based properties respected when generating a BitmapFont. [4][5]
Citations:
- 1: https://pixijs.download/dev/docs/text.TextStyleFontWeight.html
- 2: https://pixijs.download/v8.9.0/docs/text.TextStyle.html
- 3: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/font-weight
- 4: https://pixijs.download/v6.0.4/docs/PIXI.BitmapFont.html
- 5: Properties from PIXI.TextStyle aren't all used in PIXI.bitmapText / PIXI.bitmapFont pixijs/pixijs#6789
Inconsistent fontWeight values in default vs. dynamically created fonts.
Line 9 uses "bold" (weight 700) while line 40 uses '900' (ultra-bold) for fontWeight. These represent different font weights and will cause visual inconsistency—the default font will appear noticeably lighter than dynamically created fonts. Both values are valid in Pixi.js v7, but they should be standardized for visual consistency.
Standardize to either "bold" or '900' throughout the file.
Suggested fixes
Use "bold" everywhere (weight 700):
fontWeight: '900' as const
+ fontWeight: "bold" as constOr use '900' everywhere (ultra-bold):
fontWeight: "bold" as const
+ fontWeight: '900' as constAlso applies to: 40-40
🤖 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/time-graph-font-controller.ts` at 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).
- 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. |
2 participants
What it does
How to test
Follow-ups
Review checklist
Summary by CodeRabbit
New Features
Improvements
Tests
Chores