fix: prevent tooltip flicker in syncMethod="value" with mismatched data arrays#7020
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:
WalkthroughTooltip reducers now clear Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChartA
participant TooltipStore as TooltipSlice
participant SyncBus
participant ChartB as OtherChart
User->>ChartA: hover / click (local interaction)
ChartA->>TooltipStore: dispatch local tooltip action (clears sourceViewBox)
TooltipStore->>SyncBus: emit sync payload (guarded by isReceivingSynchronisation)
SyncBus->>ChartB: deliver sync payload
alt payload contains sourceViewBox
ChartB->>TooltipStore: apply receiving state (sourceViewBox present)
TooltipStore->>ChartB: show synced tooltip
else payload.deactivate or activeTick == null
ChartB->>TooltipStore: clear or mark inactive (may preserve sourceViewBox to indicate receiving)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/component/Tooltip/Tooltip.sync.spec.tsx (1)
562-571: Consider usingexpectLastCalledWithwith the full expected object.The coding guidelines recommend using
expectLastCalledWithoverexpect(spy).toHaveBeenLastCalledWith(...). Here the partial matcherexpect.objectContainingmay not be compatible withexpectLastCalledWith, but sinceselectIsTooltipActivereturns a known shape{ isActive, activeIndex }, you could assert the full object:- expect(spyB).toHaveBeenLastCalledWith(expect.objectContaining({ isActive: true })); + expectLastCalledWith(spyB, { isActive: true, activeIndex: '2' });This would also be a stronger assertion — confirming the correct
activeIndexin addition toisActive. As per coding guidelines: "Use theexpectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/component/Tooltip/Tooltip.sync.spec.tsx` around lines 562 - 571, Replace the partial spy assertion in the test 'chart B sync state should show active tooltip even when chart C has no matching label' by using the expectLastCalledWith helper instead of expect(spyB).toHaveBeenLastCalledWith; call expectLastCalledWith(spyB, selectIsTooltipActive(...)) with the full expected object shape (include both isActive: true and the expected activeIndex returned by the renderThreeChartTestCase interaction) so the assertion verifies both isActive and activeIndex; keep the existing setup using renderThreeChartTestCase, showTooltip, wrapperA and lineChartMouseHoverTooltipSelector and derive the expected activeIndex from the same test interaction to construct the complete expected object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/component/Tooltip/Tooltip.sync.spec.tsx`:
- Around line 562-571: Replace the partial spy assertion in the test 'chart B
sync state should show active tooltip even when chart C has no matching label'
by using the expectLastCalledWith helper instead of
expect(spyB).toHaveBeenLastCalledWith; call expectLastCalledWith(spyB,
selectIsTooltipActive(...)) with the full expected object shape (include both
isActive: true and the expected activeIndex returned by the
renderThreeChartTestCase interaction) so the assertion verifies both isActive
and activeIndex; keep the existing setup using renderThreeChartTestCase,
showTooltip, wrapperA and lineChartMouseHoverTooltipSelector and derive the
expected activeIndex from the same test interaction to construct the complete
expected object.
…ta arrays When synced charts have different data array lengths and use syncMethod="value", a sparse chart that can't match the synced label would emit a counter-sync event with active: false, cascading to clear tooltips on all other synced charts. Fix: separate "tooltip active" from "receiving sync events" by using sourceViewBox as the receiving indicator. When a label doesn't match, hide the tooltip but keep sourceViewBox set to suppress counter-emission. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
534c88f to
8553390
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/synchronisation/useChartSynchronisation.tsx (1)
270-278: Emission guard now correctly suppresses outgoing events for charts still in "receiving" mode.Since
isReceivingSynchronisationis now based onsourceViewBox, a chart with an unmatched tick (tooltip inactive but still receiving) won't re-emit and create a flicker cascade.Minor: the JSDoc block for
useTooltipChartSynchronisation(lines 232–245) could be updated to mention thatsourceViewBox(notactive) drives the receiving check, since this is a material behavioral change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synchronisation/useChartSynchronisation.tsx` around lines 270 - 278, Update the JSDoc for useTooltipChartSynchronisation to reflect the behavioral change: replace references that say the hook checks `active` to determine receiving state with text stating that `sourceViewBox` (used by `isReceivingSynchronisation`) now drives the "receiving" check; mention `isReceivingSynchronisation` and `sourceViewBox` by name and clarify that charts with unmatched ticks (tooltip inactive) can still be considered receiving and thus will suppress outgoing emissions.test/component/Tooltip/Tooltip.sync.spec.tsx (1)
562-572: Consider also testing the mouseLeave (deactivation) flow with sparse data.The two new tests only cover the hover-active path. Adding a test that calls
hideTooltip(wrapperA, ...)after the hover would verify that the source-deactivation early return (lines 51–64 in the hook) correctly clearssourceViewBoxand hides tooltips on all three charts, completing the lifecycle coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/component/Tooltip/Tooltip.sync.spec.tsx` around lines 562 - 572, Add a new test in Tooltip.sync.spec.tsx that follows the existing hover-active test: use renderThreeChartTestCase with selectIsTooltipActive, call showTooltip(wrapperA, lineChartMouseHoverTooltipSelector, debug) then call hideTooltip(wrapperA, lineChartMouseHoverTooltipSelector, debug) and assert that spyB (and other spies if present) were last called with expect.objectContaining({ isActive: false }) and that the hook cleared sourceViewBox (if observable via selectors) to ensure the deactivation early return path in the hook (lines ~51–64) correctly hides tooltips across all three charts; reference the existing helpers showTooltip, hideTooltip, selectIsTooltipActive, spyB, wrapperA and lineChartMouseHoverTooltipSelector when adding the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/synchronisation/useChartSynchronisation.tsx`:
- Around line 270-278: Update the JSDoc for useTooltipChartSynchronisation to
reflect the behavioral change: replace references that say the hook checks
`active` to determine receiving state with text stating that `sourceViewBox`
(used by `isReceivingSynchronisation`) now drives the "receiving" check; mention
`isReceivingSynchronisation` and `sourceViewBox` by name and clarify that charts
with unmatched ticks (tooltip inactive) can still be considered receiving and
thus will suppress outgoing emissions.
In `@test/component/Tooltip/Tooltip.sync.spec.tsx`:
- Around line 562-572: Add a new test in Tooltip.sync.spec.tsx that follows the
existing hover-active test: use renderThreeChartTestCase with
selectIsTooltipActive, call showTooltip(wrapperA,
lineChartMouseHoverTooltipSelector, debug) then call hideTooltip(wrapperA,
lineChartMouseHoverTooltipSelector, debug) and assert that spyB (and other spies
if present) were last called with expect.objectContaining({ isActive: false })
and that the hook cleared sourceViewBox (if observable via selectors) to ensure
the deactivation early return path in the hook (lines ~51–64) correctly hides
tooltips across all three charts; reference the existing helpers showTooltip,
hideTooltip, selectIsTooltipActive, spyB, wrapperA and
lineChartMouseHoverTooltipSelector when adding the assertions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
src/synchronisation/useChartSynchronisation.tsx (1)
78-100:⚠️ Potential issue | 🟡 Minor
elsebranch for'index'method may leavesourceViewBoxundefined, breaking the new receiving guard.The condition on line 79 now requires
action.payload.sourceViewBoxto apply coordinate scaling. When that field is absent (emitting chart'suseViewBox()returnedundefinedduring initialisation), theelsebranch dispatches the rawactionwhosepayload.sourceViewBoxis alsoundefined. With the newisReceivingSynchronisation = tooltipState?.sourceViewBox != null, this setsisReceivingSynchronisation = falseon the receiving chart while it is still being actively synced — the opposite of the intended guard.The old code keyed off
active(a boolean), so dispatchingactive: truevia the else branch was enough to keepisReceivingSynchronisation = true. The newsourceViewBox-based guard loses that fallback.The window is narrow (source chart's viewBox must be undefined), but it is a regression for the
'index'method specifically.🛡️ Proposed defensive patch for the `else` branch
When the condition fails, we still need to signal "receiving" mode. One option is to refuse to dispatch rather than dispatch without a gate, mirroring the
activeTick == nullpattern used for value/function:if (syncMethod === 'index') { if (viewBox && action?.payload?.coordinate && action.payload.sourceViewBox) { // ... existing scaling logic ... dispatch({ ...action, payload: { ...action.payload, coordinate: scaledCoordinate, }, }); } else { - dispatch(action); + // sourceViewBox is absent – cannot scale or gate; dispatch only when + // sourceViewBox is present so we don't lose receiving-mode tracking. + if (action.payload.sourceViewBox != null) { + dispatch(action); + } } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synchronisation/useChartSynchronisation.tsx` around lines 78 - 100, The else branch under the syncMethod === 'index' block currently dispatches the raw action even when action.payload.sourceViewBox is missing, which breaks the receiving guard (isReceivingSynchronisation based on tooltipState?.sourceViewBox). Change the else branch so it does not dispatch when sourceViewBox is absent (i.e., when viewBox, action.payload.coordinate or action.payload.sourceViewBox are falsy) — mirror the defensive pattern used for activeTick by returning early/no-op instead of dispatching the raw action. Update the logic in the syncMethod === 'index' handler (references: syncMethod, viewBox, action.payload.sourceViewBox, tooltipState, isReceivingSynchronisation) to preserve the receiving guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/synchronisation/useChartSynchronisation.tsx`:
- Around line 78-100: The else branch under the syncMethod === 'index' block
currently dispatches the raw action even when action.payload.sourceViewBox is
missing, which breaks the receiving guard (isReceivingSynchronisation based on
tooltipState?.sourceViewBox). Change the else branch so it does not dispatch
when sourceViewBox is absent (i.e., when viewBox, action.payload.coordinate or
action.payload.sourceViewBox are falsy) — mirror the defensive pattern used for
activeTick by returning early/no-op instead of dispatching the raw action.
Update the logic in the syncMethod === 'index' handler (references: syncMethod,
viewBox, action.payload.sourceViewBox, tooltipState, isReceivingSynchronisation)
to preserve the receiving guard.
Claude's reply to this was: I'm going to skip this one. Here's my reasoning:
|
- Use typed expectLastCalledWith helper with full object shape assertion - Add test for deactivation path across all three synced charts - Document sourceViewBox-based receiving guard in useTooltipChartSynchronisation JSDoc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
src/synchronisation/useChartSynchronisation.tsx (1)
341-358:⚠️ Potential issue | 🟠 Major
useBrushChartSynchronisationemits on mount and will reset peer charts' brush positionsThe
useEffectfires whenbrushStartIndex,brushEndIndex,eventEmitterSymbol, orsyncIdchange. When a chart mounts with data,dataEndIndexis initialized todata.length − 1(set bysetChartDatainchartDataSlice), triggering emission of the default brush range[0, N-1]to all synchronized peers. Any peer chart that already had a non-default brush position will receive this emission and update its state viauseBrushSyncEventsListener, resetting the user's zoom to the default range.Unlike
useTooltipChartSynchronisation, which uses anisReceivingSynchronisationguard (line 290) to suppress outgoing events while receiving, the brush emitter has no equivalent guard. The emitter symbol check prevents self-emission loops but does not prevent the cascade: when a receiving chart'sdataStartIndex/dataEndIndexchanges, it re-emits, propagating back to the source.No test coverage exists for multi-chart brush sync scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synchronisation/useChartSynchronisation.tsx` around lines 341 - 358, The hook useBrushChartSynchronisation currently emits brush ranges on mount and when indices change, causing received events to be re-emitted and resetting peer brushes; fix it by reusing the receiving guard pattern used by useTooltipChartSynchronisation: add/select the same isReceivingSynchronisation flag (or equivalent selector) and short-circuit the effect when isReceivingSynchronisation is true, so that updates originating from useBrushSyncEventsListener do not trigger eventCenter.emit; ensure you still check the existing syncId and eventEmitterSymbol guards and only emit in useBrushChartSynchronisation when not receiving to prevent cascading re-emissions.
🧹 Nitpick comments (2)
test/component/Tooltip/Tooltip.sync.spec.tsx (1)
545-560: Consider asserting that Chart C's tooltip is hidden for the unmatchable labelThe test verifies Chart B is not cleared, but does not confirm that Chart C's tooltip is correctly hidden when 'Day 3' is not in
sparseData. Adding that assertion would directly validate the sparse-chart side of the bug fix.✅ Suggested addition
// Chart B should sync and show tooltip — NOT be cleared by Chart C's counter-emission expectTooltipPayload(wrapperB, 'Day 3', ['pv : 350']); + // Chart C has no 'Day 3' entry, so its tooltip should be hidden + expectTooltipNotVisible(wrapperC); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/component/Tooltip/Tooltip.sync.spec.tsx` around lines 545 - 560, Add an assertion that Chart C's tooltip is hidden for the unmatched label by calling expectTooltipNotVisible on wrapperC after showTooltip(wrapperA, ...) (and before/alongside the existing payload assertions); this ensures the sparse-chart side is validated — reference the wrapperC test fixture and the expectTooltipNotVisible helper when adding the new assertion.src/synchronisation/useChartSynchronisation.tsx (1)
40-40:classNameis unused inside the effect but included in the dependency array
classNameis read into a variable and added to theuseEffectdeps, but is never referenced inside the effect body or thelistenerclosure. All semantically meaningful dependencies (myEventEmitter,mySyncId,syncMethod,tooltipTicks,layout,viewBox,dispatch) are already listed. IncludingclassNamecauses the listener to be torn down and re-registered whenever the chart's class name changes, even though the listener's behavior is not affected.🔧 Proposed fix
- const className = useAppSelector(state => state.rootProps.className); useEffect(() => { ... - }, [className, dispatch, myEventEmitter, mySyncId, syncMethod, tooltipTicks, layout, viewBox]); + }, [dispatch, myEventEmitter, mySyncId, syncMethod, tooltipTicks, layout, viewBox]);Also applies to: 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synchronisation/useChartSynchronisation.tsx` at line 40, The variable className (from useAppSelector(state => state.rootProps.className)) is never used inside the effect or its listener but is present in the useEffect dependency array, causing unnecessary teardown/re-registration; remove className from the effect's dependency array and delete the unused className constant if it isn’t used elsewhere in this module (also remove the duplicate/reference at the second occurrence around the listener on line ~197), ensuring the remaining dependencies (myEventEmitter, mySyncId, syncMethod, tooltipTicks, layout, viewBox, dispatch) remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/synchronisation/useChartSynchronisation.tsx`:
- Around line 341-358: The hook useBrushChartSynchronisation currently emits
brush ranges on mount and when indices change, causing received events to be
re-emitted and resetting peer brushes; fix it by reusing the receiving guard
pattern used by useTooltipChartSynchronisation: add/select the same
isReceivingSynchronisation flag (or equivalent selector) and short-circuit the
effect when isReceivingSynchronisation is true, so that updates originating from
useBrushSyncEventsListener do not trigger eventCenter.emit; ensure you still
check the existing syncId and eventEmitterSymbol guards and only emit in
useBrushChartSynchronisation when not receiving to prevent cascading
re-emissions.
---
Nitpick comments:
In `@src/synchronisation/useChartSynchronisation.tsx`:
- Line 40: The variable className (from useAppSelector(state =>
state.rootProps.className)) is never used inside the effect or its listener but
is present in the useEffect dependency array, causing unnecessary
teardown/re-registration; remove className from the effect's dependency array
and delete the unused className constant if it isn’t used elsewhere in this
module (also remove the duplicate/reference at the second occurrence around the
listener on line ~197), ensuring the remaining dependencies (myEventEmitter,
mySyncId, syncMethod, tooltipTicks, layout, viewBox, dispatch) remain intact.
In `@test/component/Tooltip/Tooltip.sync.spec.tsx`:
- Around line 545-560: Add an assertion that Chart C's tooltip is hidden for the
unmatched label by calling expectTooltipNotVisible on wrapperC after
showTooltip(wrapperA, ...) (and before/alongside the existing payload
assertions); this ensures the sparse-chart side is validated — reference the
wrapperC test fixture and the expectTooltipNotVisible helper when adding the new
assertion.
|
Some CodeRabbit nitpicks are outside of the changes made by this PR. I assume I shouldn't force Claude to try and address those, so I'm only revising the PR for the ones that are relevant. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/component/Tooltip/Tooltip.sync.spec.tsx (3)
561-562: PreferassertNotNullfor consistency with the rest of the file.The
expect(wrapperC).not.toBeNull()+wrapperC!pattern diverges from theassertNotNull(wrapperC)style used throughout this file (e.g., lines 350–351).assertNotNullboth fails the test and narrows the type, eliminating the non-null assertion operator.♻️ Suggested refactor
- expect(wrapperC).not.toBeNull(); - expectTooltipNotVisible(wrapperC!); + assertNotNull(wrapperC); + expectTooltipNotVisible(wrapperC);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/component/Tooltip/Tooltip.sync.spec.tsx` around lines 561 - 562, Replace the pattern expect(wrapperC).not.toBeNull(); followed by expectTooltipNotVisible(wrapperC!); with the file's consistent assertion style: call assertNotNull(wrapperC) to both assert and narrow the type, then pass wrapperC (without the non-null assertion) to expectTooltipNotVisible; update references to the symbols wrapperC, assertNotNull, and expectTooltipNotVisible accordingly.
577-594: Intermediate state assertions are incomplete.The "Verify active state before deactivation" comment at line 584 only checks
spyB. AddingspyA(source, should be active) andspyC(sparse, should remain inactive throughout) makes it clear the test covers the full intermediate state and prevents a false-green caused by spyC being{ isActive: false }before and after, with no transition to observe.♻️ Suggested additions
// Verify active state before deactivation + expectLastCalledWith(spyA, { isActive: true, activeIndex: '2' }); expectLastCalledWith(spyB, { isActive: true, activeIndex: '2' }); + // Chart C has no 'Day 3' entry — it should never become active during sync + expectLastCalledWith(spyC, { isActive: false, activeIndex: null });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/component/Tooltip/Tooltip.sync.spec.tsx` around lines 577 - 594, The intermediate state assertions are incomplete: before calling hideTooltip you should assert that spyA (the source chart) is active with the expected payload and spyC (the sparse chart) is still inactive to fully validate the pre-deactivation state; update the test 'all synced charts deactivate when the source chart mouse leaves' to call expectLastCalledWith(spyA, { isActive: true, activeIndex: '2' }) and expectLastCalledWith(spyC, { isActive: false, activeIndex: null }) after showTooltip(wrapperA, lineChartMouseHoverTooltipSelector, debug) and before hideTooltip(wrapperA, lineChartMouseHoverTooltipSelector), using the existing spies and helper functions (showTooltip, hideTooltip, expectLastCalledWith) already present in the test.
545-595: Missing selector call count assertions in new tests.Per the project coding guidelines, spy call counts should be verified to catch unnecessary re-renders. The new suite at lines 565–575 and 577–594 never calls
.toHaveBeenCalledTimes(...)on any spy, unlike the existing adjacent tests (e.g., lines 975, 1002, 1018). Adding call-count assertions forspyB(andspyA/spyCin test 3) after both the hover and deactivation events would surface unexpected extra renders in the cascading sync path.Based on learnings: "Verify the number of selector calls using the spy object from
createSelectorTestCaseto spot unnecessary re-renders and improve performance."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/component/Tooltip/Tooltip.sync.spec.tsx` around lines 545 - 595, Add missing selector spy call-count assertions in the two tests that use renderThreeChartTestCase: after invoking showTooltip(...) assert spyB has the expected call count, and in the third test assert spyA, spyB, and spyC have expected call counts both after the hover and again after hideTooltip(...). Locate the spies (spyA/spyB/spyC) returned from renderThreeChartTestCase and insert .toHaveBeenCalledTimes(...) checks immediately after the hover assertions and immediately after the deactivation assertions to match the style of the existing tests (use the same numeric expectations used elsewhere in the suite).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/component/Tooltip/Tooltip.sync.spec.tsx`:
- Around line 561-562: Replace the pattern expect(wrapperC).not.toBeNull();
followed by expectTooltipNotVisible(wrapperC!); with the file's consistent
assertion style: call assertNotNull(wrapperC) to both assert and narrow the
type, then pass wrapperC (without the non-null assertion) to
expectTooltipNotVisible; update references to the symbols wrapperC,
assertNotNull, and expectTooltipNotVisible accordingly.
- Around line 577-594: The intermediate state assertions are incomplete: before
calling hideTooltip you should assert that spyA (the source chart) is active
with the expected payload and spyC (the sparse chart) is still inactive to fully
validate the pre-deactivation state; update the test 'all synced charts
deactivate when the source chart mouse leaves' to call
expectLastCalledWith(spyA, { isActive: true, activeIndex: '2' }) and
expectLastCalledWith(spyC, { isActive: false, activeIndex: null }) after
showTooltip(wrapperA, lineChartMouseHoverTooltipSelector, debug) and before
hideTooltip(wrapperA, lineChartMouseHoverTooltipSelector), using the existing
spies and helper functions (showTooltip, hideTooltip, expectLastCalledWith)
already present in the test.
- Around line 545-595: Add missing selector spy call-count assertions in the two
tests that use renderThreeChartTestCase: after invoking showTooltip(...) assert
spyB has the expected call count, and in the third test assert spyA, spyB, and
spyC have expected call counts both after the hover and again after
hideTooltip(...). Locate the spies (spyA/spyB/spyC) returned from
renderThreeChartTestCase and insert .toHaveBeenCalledTimes(...) checks
immediately after the hover assertions and immediately after the deactivation
assertions to match the style of the existing tests (use the same numeric
expectations used elsewhere in the suite).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude's view on latest nitpicks.
Applying 1 and 2. |
c056a67 to
c792890
Compare
Bundle ReportChanges will increase total bundle size by 3.79kB (0.13%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7020 +/- ##
==========================================
- Coverage 90.12% 90.09% -0.03%
==========================================
Files 526 526
Lines 39183 39215 +32
Branches 5423 5427 +4
==========================================
+ Hits 35312 35331 +19
- Misses 3862 3875 +13
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
None of the three issues you linked are relevant - they all are older than this code (and some irrelevant completely). Do you have reproduction of what was happening before, and how is it better now? Stackblitz with videos would be perfect. |
Sorry about that. I just let Claude Code roll with writing up the PR itself once we managed to fix the bug. I do have an html file that reproduces it. I've never used Stackblitz but I will look into setting up an account and make a minimalist stand alone demo. |
|
@roy7 stackblitz should work without an account. You can start with an example on our website and click "Open in stackblitz" and make edits there. |
|
Ok. Repro: https://stackblitz.com/edit/vitejs-vite-6negt9nq Video: https://www.loom.com/share/cc8a55927b0941c9879cac52026fa63c Claude had a nice suggestion, since the "flicker off" only happens for 1 pixel it can sometimes be hard to reproduce (thankfully I caught it on video showing total flicker off for a moment), with tooltip animation on you can more easily see that when the tooltip is hidden for 1 refresh it triggered a new animation to happen. So as you slowly scroll in chart A, the chart B tooltip will suddenly re-animate the frame after chart C's tooltip disappears. This is more easy to spot than trying to see it disappear. Hopefully the video makes things obvious. :) I edited my original post to remove the Related links. |
|
I didn't make a video with the bug fix applied, but what will happen in the fixed version is the tooltip in chart B doesn't disappear for 1 pixel (making it flicker if you notice it, and making the tooltip re-animate if animation is on). With the fix, B's tooltip just smoothly behaves as expected. As discussed in the PR there's a difference in how the sparse chart behaves to a mouse in the chart vs a mouse in a linked chart. But that's a different issue so I didn't want to do a single PR for both things, and I'm not even sure if it's a bug per se. But I can submit the sync-value-closest-match PR if you think existing behavior is not ideal. |
|
@PavelVanecek The other issue I mentioned but didn't submit a PR for ("direct hover and synced tooltip behavior on sparse charts"), should I also submit that PR for review or just leave the existing behavior as-is? |
|
@roy7 is it possible to make that configurable? Perhaps with a new prop? |
|
@PavelVanecek I can give it a try however you prefer. To not change default behavior the easiest might be to add a syncMethod="valueClosest". Or if you prefer the prop approach, could do something like syncValueFallback="closest" | "none". |
Summary
Fixes tooltip flickering on synced charts when using
syncMethod="value"with charts that have different data array lengths or starting dates.Root Cause
Cascading counter-emission bug. When synced charts have mismatched data arrays:
label: "2025-06-23"setSyncInteraction({ active: false })— correctly hiding its own tooltipisReceivingSynchronisationfromtruetofalseuseEffectre-fires and emitsactive: falseto all other synced chartsThe flicker only occurs at dates adjacent to the sparse chart's data points — where the exact string match on ticks fails — making it very specific and directional.
Fix
Core change: Separate "tooltip should be visible" (
active) from "this chart is receiving sync events" (sourceViewBox).src/synchronisation/useChartSynchronisation.tsx:active === false) beforesyncMethod-specific branches, clearingsourceViewBoxso charts can resume normal emissionactiveTick == null): dispatchactive: false(hide tooltip) but keepsourceViewBoxset (suppress counter-emission)isReceivingSynchronisationto checksourceViewBox != nullinstead ofactivesrc/state/tooltipSlice.ts:sourceViewBoxin 4 mouse interaction reducers (setActiveMouseOverItemIndex,setActiveClickItemIndex,setMouseOverAxisIndex,setMouseClickAxisIndex) — when a chart starts its own interaction, it exits "receiving" mode.mouseLeaveChartintentionally not modified.Observation / Discussion
There is an asymmetry between direct hover and synced tooltip behavior on sparse charts:
syncMethod="value": exact string match, no tooltip if no matchThis PR preserves the existing behavior — sparse charts hide tooltips for unmatched dates in sync mode. We have a separate branch (
fix/sync-value-closest-match) ready that adds afindClosestTickbinary search fallback, which would make synced behavior match direct-hover behavior. Happy to submit that as a follow-up PR if maintainers feel it should be addressed.Reproduction
Financial dashboard with 6 synced
LineChartcomponents usingsyncId+syncMethod="value":With mismatched arrays, tooltips on non-hovered charts flicker at specific date boundaries. With aligned arrays (null-padding), no flicker.
Test plan
sourceViewBoxbehavior🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests