fix(ui): create independent play-head indicator - #528
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:
📝 WalkthroughWalkthroughThis PR replaces synced-pointer playhead synchronization with shared playhead-line timestamp state, adds pixel conversion for timeline overlays, renders independent vertical playhead lines in timeline views, and updates playback tests to validate the new playhead-line notifications. ChangesIndependent playhead indicator
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
🤖 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/usePlayheadLinePixel.ts:
- Around line 19-33: The usePlayheadLinePixel subscription only updates on
playhead broadcasts, leaving the overlay stale after chart mapping or size
changes. Update usePlayheadLinePixel to retain the latest timestamp and
recompute the pixel after chart readiness, dataZoom, and resize events, while
preserving null/error handling and cleaning up every added listener.
🪄 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: 0cb594c6-619f-4958-b792-214835a3244e
📒 Files selected for processing (5)
ui/packages/@quent/components/src/dag/DagPlayhead.tsxui/packages/@quent/components/src/lib/timeline.utils.tsui/packages/@quent/components/src/lib/usePlayheadLinePixel.tsui/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 (1)
ui/packages/@quent/components/src/lib/timeline.utils.ts (1)
515-535: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReplay the current playhead state for late subscribers.
If a timeline subscribes after
broadcastPlayheadLineemits a timestamp and beforehidePlayheadLine,usePlayheadLinePixelretains its initialnulland hides the overlay until the next broadcast. Store the latestnumber | nullvalue and deliver it during subscription.🤖 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/timeline.utils.ts around lines 515 - 535, Update subscribePlayheadLine and the shared playhead state so the latest number or null value is stored whenever broadcastPlayheadLine or hidePlayheadLine runs, then immediately invoke each newly subscribed listener with that stored value. Preserve existing listener registration and unsubscription 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.
Outside diff comments:
In `@ui/packages/`@quent/components/src/lib/timeline.utils.ts:
- Around line 515-535: Update subscribePlayheadLine and the shared playhead
state so the latest number or null value is stored whenever
broadcastPlayheadLine or hidePlayheadLine runs, then immediately invoke each
newly subscribed listener with that stored value. Preserve existing listener
registration and unsubscription behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ad74c9f2-2cd7-45a6-937c-1ebb833e320a
📒 Files selected for processing (4)
ui/packages/@quent/components/src/lib/timeline.utils.tsui/packages/@quent/components/src/lib/usePlayheadLinePixel.tsui/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 (3)
ui/packages/@quent/components/src/lib/timeline.utils.ts (1)
529-535: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBroadcast every playhead state update.
usePlayheadLinePixel.tsupdates only fromsubscribePlayheadLine. ThehandleKeyDownand restart paths inDagPlayhead.tsxupdateplayheadTimeSwithout callingbroadcastPlayheadLine. The chart line can remain stale or hidden after keyboard input. Broadcast each state change or centralize the state update and broadcast in one helper.Based on the downstream
DagPlayhead.tsxandusePlayheadLinePixel.tspaths.🤖 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/timeline.utils.ts around lines 529 - 535, Ensure every playheadTimeS state update, including keyboard handling and restart paths in DagPlayhead, broadcasts the new value through broadcastPlayheadLine so usePlayheadLinePixel stays synchronized. Prefer centralizing the state update and broadcast in one helper, and preserve hidePlayheadLine behavior when the playhead is cleared.ui/packages/@quent/components/src/index.ts (1)
167-171: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the package-root
Timelineexport.
Timelineremains a named component and supported examples import it from@quent/components. Removing the export breaks those consumers. Keep the export or provide an intentional migration and release note.🤖 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 167 - 171, Restore the package-root named Timeline export in the index.ts export list, using the existing Timeline component symbol and preserving the current timeline type exports so consumers can continue importing Timeline from `@quent/components`.ui/packages/@quent/components/src/timeline/Timeline.tsx (1)
379-386: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBroadcast every playhead state change.
usePlayheadLinePixelupdates only fromsubscribePlayheadLine. Inui/packages/@quent/components/src/dag/DagPlayhead.tsx, the Arrow/Home/End handlers and the restart path updateplayheadTimeSwithout callingbroadcastPlayheadLine. The playhead control can therefore move while this line remains stale. Broadcast those updates or subscribe the overlay to the canonical playhead 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 `@ui/packages/`@quent/components/src/timeline/Timeline.tsx around lines 379 - 386, The playhead overlay can become stale because usePlayheadLinePixel depends on broadcast updates that are missing from DagPlayhead state changes. Update the Arrow/Home/End handlers and restart path in DagPlayhead to call broadcastPlayheadLine whenever playheadTimeS changes, or make the overlay subscribe directly to the canonical playhead state while preserving existing chart-ready behavior.
🧹 Nitpick comments (1)
ui/src/components/DataFlowOverlay.test.tsx (1)
267-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused chart-level test for playhead conversion.
DataFlowOverlay.test.tsxmountsDagPlayhead, notTimelineorTimelineController. Its DOM assertions do not coverusePlayheadLinePixelor coexistence of the solid playhead line with the dotted ECharts pointer. Keep the producer assertion here and cover those behaviors in a Timeline-focused 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/src/components/DataFlowOverlay.test.tsx` around lines 267 - 295, Add a focused Timeline-level test covering playhead line pixel conversion and coexistence of the solid playhead line with the dotted ECharts pointer. Keep the existing producer/broadcast assertions in the DataFlowOverlay test, and mount the Timeline or TimelineController path so usePlayheadLinePixel and both rendered indicators are exercised.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/index.ts:
- Around line 167-171: Restore the package-root named Timeline export in the
index.ts export list, using the existing Timeline component symbol and
preserving the current timeline type exports so consumers can continue importing
Timeline from `@quent/components`.
In `@ui/packages/`@quent/components/src/lib/timeline.utils.ts:
- Around line 529-535: Ensure every playheadTimeS state update, including
keyboard handling and restart paths in DagPlayhead, broadcasts the new value
through broadcastPlayheadLine so usePlayheadLinePixel stays synchronized. Prefer
centralizing the state update and broadcast in one helper, and preserve
hidePlayheadLine behavior when the playhead is cleared.
In `@ui/packages/`@quent/components/src/timeline/Timeline.tsx:
- Around line 379-386: The playhead overlay can become stale because
usePlayheadLinePixel depends on broadcast updates that are missing from
DagPlayhead state changes. Update the Arrow/Home/End handlers and restart path
in DagPlayhead to call broadcastPlayheadLine whenever playheadTimeS changes, or
make the overlay subscribe directly to the canonical playhead state while
preserving existing chart-ready behavior.
---
Nitpick comments:
In `@ui/src/components/DataFlowOverlay.test.tsx`:
- Around line 267-295: Add a focused Timeline-level test covering playhead line
pixel conversion and coexistence of the solid playhead line with the dotted
ECharts pointer. Keep the existing producer/broadcast assertions in the
DataFlowOverlay test, and mount the Timeline or TimelineController path so
usePlayheadLinePixel and both rendered indicators are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 664dfcb4-346e-44b5-a93b-b1b04b64b7b3
📒 Files selected for processing (4)
ui/packages/@quent/components/src/index.tsui/packages/@quent/components/src/lib/timeline.utils.tsui/packages/@quent/components/src/timeline/Timeline.tsxui/src/components/DataFlowOverlay.test.tsx
| broadcastHidePointer(null); | ||
| } | ||
|
|
||
| type PlayheadListener = (timestampMs: number | null) => void; |
There was a problem hiding this comment.
Could we use an atom for this to stay reactive without setting up a custom pub/sub system?
There was a problem hiding this comment.
Yes, we can go ahead and move that to an atom instead of keeping the pub/sub block, that should make it cleaner.
One caveat though - the ECharts event listeners (dataZoom, finished) in usePlayheadLinePixel would still be necessary regardless, since the pixel position goes stale on zoom/resize even when the timestamp hasn't changed - that part of the complexity doesn't go away with the atom approach.
There was a problem hiding this comment.
Makes sense, I just didn't want to set up the custom pub/sub + global to track if atoms do work for our purposes
| onReady: onChartReady, | ||
| }); | ||
|
|
||
| const playheadPixelX = usePlayheadLinePixel(instanceRef, 0, readyTick); |
There was a problem hiding this comment.
I'm on the fence about this living in Timeline. To keep this component as generic as possible, I think i'd prefer this stay in ResourceTimeline.
Worth a quick experiment I think. If it's too much plumbing then I could see a playhead being part of this component, but we should confirm that this fails gracefully if the Timeline component is used elsewhere without any kind of playhead hooked up to it.
There was a problem hiding this comment.
No, that's a good idea, and ended up being a fairly easy move. All we needed to do was add an optional onReady prop to Timeline, so anyone using it without playhead hooked up is unaffected.
There was a problem hiding this comment.
Actionable comments posted: 1
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/dag/DagPlayhead.tsx (1)
87-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCancel queued pointer work before clearing the playhead line.
If
pointermovequeues the animation frame andpointerupoccurs before it runs, the callback callsapplyClientXafter Line 100 clears the state. The overlay line then remains visible after the interaction ends.Cancel
rafRef.currentand clearpendingClientXRef.currentinhandlePointerEndbefore callingsetPlayheadLineTimeMs(null).Proposed fix
const handlePointerEnd = useCallback((event: React.PointerEvent<HTMLDivElement>) => { if (event.currentTarget.hasPointerCapture(event.pointerId)) { event.currentTarget.releasePointerCapture(event.pointerId); } + if (rafRef.current != null) { + cancelAnimationFrame(rafRef.current); + rafRef.current = null; + } + pendingClientXRef.current = null; setPlayheadLineTimeMs(null); }, [setPlayheadLineTimeMs]);🤖 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/dag/DagPlayhead.tsx around lines 87 - 101, Update handlePointerEnd to cancel any queued animation frame via rafRef and clear pendingClientXRef before calling setPlayheadLineTimeMs(null), preventing deferred applyClientX work after pointer interaction ends.
🤖 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/usePlayheadLinePixel.ts:
- Around line 31-32: Update the convertToPixel result handling in the playhead
pixel calculation to first require typeof pixel === 'number', then retain the
finite-number check before passing it to setPixelX; continue assigning null for
arrays, invalid numbers, or other unsupported results.
---
Outside diff comments:
In `@ui/packages/`@quent/components/src/dag/DagPlayhead.tsx:
- Around line 87-101: Update handlePointerEnd to cancel any queued animation
frame via rafRef and clear pendingClientXRef before calling
setPlayheadLineTimeMs(null), preventing deferred applyClientX work after pointer
interaction ends.
🪄 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: cb4f9a7e-06a4-4d17-a7a5-b1bf6234630d
📒 Files selected for processing (7)
ui/packages/@quent/components/src/dag/DagPlayhead.tsxui/packages/@quent/components/src/lib/timeline.utils.tsui/packages/@quent/components/src/lib/usePlayheadLinePixel.tsui/packages/@quent/hooks/src/atoms/dataFlow.tsui/packages/@quent/hooks/src/dataFlow/dataFlowSelectors.tsui/packages/@quent/hooks/src/index.tsui/src/components/DataFlowOverlay.test.tsx
💤 Files with no reviewable changes (1)
- ui/packages/@quent/components/src/lib/timeline.utils.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| }, [gridOptions, minZoomSpanPct, xAxisOptions, yAxisOptions, seriesOptions]); | ||
|
|
||
| const isDraggingRef = useRef(false); | ||
| const onReadyRef = useRef(onReady); |
There was a problem hiding this comment.
Why store this in a ref? Can we just call onReady(instance) below on L366?
There was a problem hiding this comment.
I was thinking onChartReady only runs once per chart instance, but looking at it further, you're right, it's recreated on each render, so the ref is unnecessary.
johallar
left a comment
johallar
left a comment
There was a problem hiding this comment.
🙇 ty for all the updates
|
/merge |
2 participants
Description
Creates a separate play-head line indicator so that it does not interfere with the current x-axis pointer that shows where the user is currently focused on the timelines.
Related Issues
Closes #487
Testing
Screenshots
play-head-indicator.mp4