Conversation
Contributor
WalkthroughIntroduces event throttling for chart interactions via a new Redux slice managing throttle configuration. Updates all chart types and event middleware to support both RAF and setTimeout-based throttling. New ReportEventSettings component propagates throttling settings. Extends all event handlers and chart prop interfaces with throttleDelay and throttledEvents configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Chart as Chart Component
participant RES as ReportEventSettings
participant Redux as Redux Store
participant Middleware as Event Middleware
participant Handler as Event Handler
Chart->>Chart: Mount with throttleDelay, throttledEvents props
Chart->>RES: Render with throttle props
RES->>RES: useEffect on props change
RES->>Redux: Dispatch setEventSettings(props)
Redux->>Redux: Update eventSettings slice
Handler->>Middleware: Event fired (mousemove, etc.)
Middleware->>Redux: Read state.eventSettings
alt throttledEvents includes event
Middleware->>Middleware: Check throttleDelay config
alt throttleDelay === 'raf'
Middleware->>Middleware: Schedule callback via requestAnimationFrame
else throttleDelay is number
Middleware->>Middleware: Schedule callback via setTimeout(delay)
end
Middleware->>Middleware: Execute callback with current state
else event not in throttledEvents
Middleware->>Handler: Execute handler immediately
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/state/externalEventsMiddleware.spec.ts (1)
11-19: Enable fake timers in beforeEach.
Line 18 clears timers but the suite never callsvi.useFakeTimers(). Our test guidelines require fake timers because the store uses timers/RAF.✅ Suggested change
beforeEach(() => { + vi.useFakeTimers(); store = createRechartsStore(); mockHandler = vi.fn(); mockEvent = { type: 'mousemove', persist: vi.fn(), } as unknown as SyntheticEvent; vi.clearAllTimers(); });Based on learnings, use
vi.useFakeTimers()in all tests.
🤖 Fix all issues with AI agents
In `@src/state/touchEventsMiddleware.ts`:
- Around line 17-18: The module-level rafId and timeoutId cause cross-instance
interference; change touchEventMiddleware to keep those timers per store
instance (instead of shared globals) by introducing a WeakMap keyed by the store
(e.g., const timers = new WeakMap<Store, { rafId: number | null; timeoutId:
ReturnType<typeof setTimeout> | null }>()), obtain or create the timers entry
inside the touchEventMiddleware closure when middleware(store) is called, and
replace all uses of rafId/timeoutId with the store-specific entry (clear and set
timers on that entry and clean up on unsubscribe). Apply the same pattern to
mouseEventsMiddleware and keyboardEventsMiddleware to ensure each chart/store
has isolated timer state.
In `@src/util/types.ts`:
- Around line 1430-1453: Update the JSDoc on the EventThrottlingProps interface
to remove the incorrect reference to “container size changes” and clarify that
these props control throttling for chart interaction events; specifically edit
the comments for throttleDelay and throttledEvents (references:
EventThrottlingProps, throttleDelay, throttledEvents) so they state that only
listed interaction events are throttled (or 'all'), that other events run
synchronously, and keep the existing options/defaults unchanged.
In `@storybook/stories/API/arg-types/PieChartArgs.ts`:
- Around line 473-486: The prop description for throttledEvents incorrectly
refers to “container size changes”; update the original source JSDoc/args
generator (the PieChart component prop JSDoc or the args-generator entry that
defines throttledEvents) so the description says it controls throttling of chart
event handlers (and include the existing note about losing sync access to the
event), then re-run the args generator to regenerate
storybook/stories/API/arg-types/PieChartArgs.ts rather than editing that
generated file directly; target the throttledEvents prop definition in the
PieChart source/docs metadata so the regenerated file contains the corrected
wording.
In `@storybook/stories/API/arg-types/RadialBarChartArgs.ts`:
- Around line 459-486: The description for throttledEvents (and the nearby
throttleDelay) incorrectly refers to "container size changes" but these props
throttle user interaction events (mouse/touch/pointer/scroll/wheel) for the
chart; update the generator text that emits the RadialBarChartArgs descriptions
so that throttleDelay's description and throttledEvents' description explicitly
say they control throttling of chart interaction events (e.g.,
mouse/touch/pointer/scroll/wheel) and remove or replace any mention of
"container size changes", ensuring the table summaries/defaults remain unchanged
and referencing the throttleDelay and throttledEvents entries so the correct
wording is emitted into the generated file.
In `@storybook/stories/API/arg-types/ScatterChartArgs.ts`:
- Around line 412-414: The description for the throttledEvents arg is misleading
(it references "container size changes" but actually controls throttling of user
interaction events); update the source TypeScript definition that generates this
auto-generated file to correct the text for the throttledEvents property
(replace "container size changes" with "user interaction events" and keep the
rest about losing synchronous access like preventDefault/stopPropagation), then
re-run the docs/arg-types generation to regenerate
storybook/stories/API/arg-types/ScatterChartArgs.ts so the corrected description
is emitted; locate the original declaration of throttledEvents in the TypeScript
source (the arg-type interface or JSDoc that feeds the generator) and edit
there.
In `@storybook/stories/API/arg-types/TreemapArgs.ts`:
- Around line 289-316: The description for the prop throttledEvents incorrectly
mentions "container size changes"; update the generator text that builds the
TreemapArgs prop descriptions so that the throttledEvents description references
event handlers/listening to events (e.g., "Defines which DOM events should be
throttled when listening for input/mouse/scroll events") instead of container
size changes, then re-run the generator to regenerate this file; locate
references to throttledEvents (and throttleDelay) in the generator/template to
change the wording and ensure the default list and summary remain unchanged.
In `@test/helper/parameterizedTestCases.tsx`:
- Around line 32-50: The CartesianChartTestCase type uses a loose any[] for the
ChartElement `data` prop; update the `data?: any[];` declaration to the correct
readonly chart data type (use `data?: ReadonlyArray<unknown>;`) so it matches
the codebase ChartData shape and removes `any` usage; update the `ChartElement`
prop signature in the CartesianChartTestCase type accordingly and run TS checks
to ensure no further type errors.
In `@test/state/throttling.spec.ts`:
- Around line 44-46: The test currently calls vi.runAllTimers() inside the act
block (see the act(() => { vi.runAllTimers(); }) call) which can trigger
infinite loops with self-rescheduling timers; replace vi.runAllTimers() with
vi.runOnlyPendingTimers() inside that act invocation to only execute pending
timers safely and avoid spinning, keeping the surrounding act(...) and test
logic unchanged.
- Around line 1-2: Import afterEach from 'vitest', replace any vi.runAllTimers()
calls with vi.runOnlyPendingTimers() (e.g., in timer-related tests using act) to
avoid infinite-looping timers, and remove unsafe TypeScript casts by properly
typing the test event variable (replace occurrences of "mockEvent as any" with
the correctly typed "mockEvent" and declare its type where it's created) so
assertions at the places referencing mockEvent use the typed value; ensure these
changes touch the test suite setup (afterEach), the timer invocation sites
(vi.runOnlyPendingTimers), and the mockEvent declaration/uses.
In `@www/src/docs/api/ComposedChartAPI.tsx`:
- Around line 365-385: The doc for the prop object named "throttledEvents"
incorrectly refers to "container size changes"; update the description in
ComposedChartAPI.tsx for the throttledEvents entry so it states that this prop
controls which DOM event handlers (e.g., mousemove, touchmove, pointermove,
scroll, wheel) are throttled, not container resize behavior, and keep the
warning that events listed will lose synchronous access to methods like
e.preventDefault() / e.stopPropagation(); modify the JSX in the desc field for
throttledEvents accordingly to reflect this accurate wording.
In `@www/src/docs/api/FunnelChartAPI.tsx`:
- Around line 305-307: Update the prop description that currently reads "Defines
which events should be throttled when listening to container size changes..."
(the doc for the throttling prop, e.g., throttledEvents) to remove the incorrect
reference to container resizing and instead explain event handling: state that
this prop lists which event types should be throttled when handling events (for
example "mousemove", "scroll"), and that events not included in that list will
not be throttled—ensure the updated text appears where the paragraph for the
throttling prop is defined in FunnelChartAPI.tsx (the paragraph immediately
describing the throttling prop).
In `@www/src/docs/api/SankeyAPI.tsx`:
- Around line 243-248: The doc text incorrectly states "throttled when listening
to container size changes" — update the prop descriptions for throttledEvents
and throttleDelay in SankeyAPI.tsx to clarify they control throttling of user
interaction events (e.g., mouse, touch, keyboard) rather than container size
changes; change both the 'throttledEvents' description around the existing
paragraph and the 'throttleDelay' description at lines referenced so they
explicitly mention user interaction events and the effect of the delay.
In `@www/src/docs/api/SunburstChartAPI.tsx`:
- Around line 254-273: Update the description for the prop named throttledEvents
in SunburstChartAPI.tsx to accurately reflect that it controls throttling of
chart interaction events (e.g., mousemove, touchmove, pointermove, scroll,
wheel) rather than "container size changes"; modify the text inside the desc
('en-US' section) to say these are chart interaction events and keep the note
about losing synchronous access (preventDefault/stopPropagation) when an event
is throttled, preserving the existing examples and defaultVal.
♻️ Duplicate comments (6)
storybook/stories/API/arg-types/BarChartArgs.ts (1)
398-425: Same throttledEvents wording issue as in TreemapArgs.Auto-generated text still mentions “container size changes.” Please fix in the generator and regenerate.
storybook/stories/API/arg-types/RadarChartArgs.ts (1)
459-486: Same throttledEvents wording issue as in TreemapArgs.Auto-generated text still mentions “container size changes.” Please fix in the generator and regenerate.
storybook/stories/API/arg-types/ComposedChartArgs.ts (1)
398-425: Same throttledEvents wording issue as in TreemapArgs.Auto-generated text still mentions “container size changes.” Please fix in the generator and regenerate.
storybook/stories/API/arg-types/SankeyArgs.ts (1)
241-268: Same throttledEvents wording issue as in TreemapArgs.Auto-generated text still mentions “container size changes.” Please fix in the generator and regenerate.
storybook/stories/API/arg-types/FunnelChartArgs.ts (1)
398-425: Same throttledEvents wording issue as in TreemapArgs.Auto-generated text still mentions “container size changes.” Please fix in the generator and regenerate.
www/src/docs/api/AreaChartAPI.tsx (1)
360-380: Same misleading description as in ComposedChartAPI.The
throttledEventsdescription at line 368 mentions "container size changes" which is inaccurate. This should describe event handler throttling instead.
🧹 Nitpick comments (3)
src/state/eventSettingsSlice.ts (1)
12-23: Add explicit state and return types in the reducer.
Make parameter and return types explicit to align with TS typing guidelines. As per coding guidelines.🔧 Suggested fix
- setEventSettings: (state, action: PayloadAction<EventThrottlingProps>) => { + setEventSettings: (state: EventSettingsState, action: PayloadAction<EventThrottlingProps>): void => { if (action.payload.throttleDelay != null) { state.throttleDelay = action.payload.throttleDelay; } if (action.payload.throttledEvents != null) { state.throttledEvents = castDraft(action.payload.throttledEvents); } },src/state/touchEventsMiddleware.ts (1)
44-98: Consider extracting callback to reduce nesting and improve readability.The callback function spans 55 lines with multiple nested conditionals. While the logic is correct, extracting the axis and item handling into separate helper functions would improve maintainability.
storybook/stories/API/arg-types/AreaChartArgs.ts (1)
398-425: Fix the misleading description in the source EventThrottlingProps.This file is auto-generated. The
throttledEventsdescription in the source (EventThrottlingPropsinsrc/util/types.ts) contains misleading wording: "when listening to container size changes" suggests this prop is specific to container resizing, but it applies to event throttling generally. Clarify or remove this phrase in the source JSDoc so the fix propagates to all generated documentation.
Comment on lines
+17
to
+18
| let rafId: number | null = null; | ||
| let timeoutId: ReturnType<typeof setTimeout> | null = null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/state/touchEventsMiddleware.tsRepository: recharts/recharts
Length of output: 5008
🏁 Script executed:
rg -n "touchEventMiddleware" --type ts --type tsxRepository: recharts/recharts
Length of output: 88
🏁 Script executed:
rg -n "touchEventMiddleware" src/ --type typescriptRepository: recharts/recharts
Length of output: 95
🏁 Script executed:
rg -n "touchEventMiddleware" src/Repository: recharts/recharts
Length of output: 406
🏁 Script executed:
cat -n src/state/store.tsRepository: recharts/recharts
Length of output: 5940
🏁 Script executed:
rg -n "let.*Id.*null" src/state/ --type tsRepository: recharts/recharts
Length of output: 573
🏁 Script executed:
cat -n src/state/mouseEventsMiddleware.ts | head -70Repository: recharts/recharts
Length of output: 3636
🏁 Script executed:
rg -n "createRechartsStore" src/ | head -20Repository: recharts/recharts
Length of output: 338
🏁 Script executed:
cat -n src/state/RechartsStoreProvider.tsxRepository: recharts/recharts
Length of output: 2707
Module-level state causes cross-instance interference when multiple charts render.
rafId and timeoutId are shared across all chart instances because touchEventMiddleware is a module-level singleton. When multiple charts are rendered, one chart's touch event cancels another chart's pending throttled callback, leading to missed updates.
Consider storing these variables per-store, either via a WeakMap keyed by store instance or by storing them in Redux state. Note: the same issue exists in mouseEventsMiddleware.ts and keyboardEventsMiddleware.ts and should be fixed consistently.
🤖 Prompt for AI Agents
In `@src/state/touchEventsMiddleware.ts` around lines 17 - 18, The module-level
rafId and timeoutId cause cross-instance interference; change
touchEventMiddleware to keep those timers per store instance (instead of shared
globals) by introducing a WeakMap keyed by the store (e.g., const timers = new
WeakMap<Store, { rafId: number | null; timeoutId: ReturnType<typeof setTimeout>
| null }>()), obtain or create the timers entry inside the touchEventMiddleware
closure when middleware(store) is called, and replace all uses of
rafId/timeoutId with the store-specific entry (clear and set timers on that
entry and clean up on unsubscribe). Apply the same pattern to
mouseEventsMiddleware and keyboardEventsMiddleware to ensure each chart/store
has isolated timer state.
Comment on lines
+473
to
486
| throttledEvents: { | ||
| description: | ||
| 'Defines which events should be throttled when listening to container size changes.\nEvents not in this list will not be throttled.\n\nIf an event is on this list, then you lose the opportunity to access the event synchronously.\nWhich means that if you want to call `e.preventDefault()` or `e.stopPropagation()` inside the event handler,\nthen that event handler must not be in this list.', | ||
| table: { | ||
| type: { | ||
| summary: 'number', | ||
| summary: '"all" | Array<readonly (keyof GlobalEventHandlersEventMap)>', | ||
| }, | ||
| category: 'General', | ||
| defaultValue: { | ||
| summary: '["mousemove","touchmove","pointermove","scroll","wheel"]', | ||
| }, | ||
| }, | ||
| defaultValue: ['mousemove', 'touchmove', 'pointermove', 'scroll', 'wheel'], | ||
| }, |
Contributor
There was a problem hiding this comment.
Fix misleading description for throttledEvents.
Line 474 describes “container size changes,” but this prop controls throttling of chart event handlers. Please update the source JSDoc/args generator and re‑generate this file (don’t edit it directly). As per coding guidelines, update the source of autogenerated files.
🤖 Prompt for AI Agents
In `@storybook/stories/API/arg-types/PieChartArgs.ts` around lines 473 - 486, The
prop description for throttledEvents incorrectly refers to “container size
changes”; update the original source JSDoc/args generator (the PieChart
component prop JSDoc or the args-generator entry that defines throttledEvents)
so the description says it controls throttling of chart event handlers (and
include the existing note about losing sync access to the event), then re-run
the args generator to regenerate storybook/stories/API/arg-types/PieChartArgs.ts
rather than editing that generated file directly; target the throttledEvents
prop definition in the PieChart source/docs metadata so the regenerated file
contains the corrected wording.
Comment on lines
+412
to
+414
| throttledEvents: { | ||
| description: | ||
| 'Defines which events should be throttled when listening to container size changes.\nEvents not in this list will not be throttled.\n\nIf an event is on this list, then you lose the opportunity to access the event synchronously.\nWhich means that if you want to call `e.preventDefault()` or `e.stopPropagation()` inside the event handler,\nthen that event handler must not be in this list.', |
Contributor
There was a problem hiding this comment.
Same misleading description as in API docs.
The description mentions "container size changes" but this is about event throttling for user interactions. Since this file is auto-generated, the fix should be applied to the source TypeScript definitions that generate this file.
🤖 Prompt for AI Agents
In `@storybook/stories/API/arg-types/ScatterChartArgs.ts` around lines 412 - 414,
The description for the throttledEvents arg is misleading (it references
"container size changes" but actually controls throttling of user interaction
events); update the source TypeScript definition that generates this
auto-generated file to correct the text for the throttledEvents property
(replace "container size changes" with "user interaction events" and keep the
rest about losing synchronous access like preventDefault/stopPropagation), then
re-run the docs/arg-types generation to regenerate
storybook/stories/API/arg-types/ScatterChartArgs.ts so the corrected description
is emitted; locate the original declaration of throttledEvents in the TypeScript
source (the arg-type interface or JSDoc that feeds the generator) and edit
there.
Comment on lines
+44
to
+46
| act(() => { | ||
| vi.runAllTimers(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/state/throttling.spec.ts | sed -n '35,55p'Repository: recharts/recharts
Length of output: 1061
🏁 Script executed:
# Check for all usages of vi.runAllTimers() and vi.runOnlyPendingTimers() in this file
grep -n "vi.run.*Timers" test/state/throttling.spec.tsRepository: recharts/recharts
Length of output: 90
🏁 Script executed:
# Get the full test context to understand what's being tested
cat -n test/state/throttling.spec.tsRepository: recharts/recharts
Length of output: 4956
Avoid vi.runAllTimers() here — use vi.runOnlyPendingTimers() instead. This prevents potential infinite loops with self-rescheduling timers, as documented in the testing guidelines.
🔧 Suggested fix
act(() => {
- vi.runAllTimers();
+ vi.runOnlyPendingTimers();
});📝 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.
Suggested change
| act(() => { | |
| vi.runAllTimers(); | |
| }); | |
| act(() => { | |
| vi.runOnlyPendingTimers(); | |
| }); |
🤖 Prompt for AI Agents
In `@test/state/throttling.spec.ts` around lines 44 - 46, The test currently calls
vi.runAllTimers() inside the act block (see the act(() => { vi.runAllTimers();
}) call) which can trigger infinite loops with self-rescheduling timers; replace
vi.runAllTimers() with vi.runOnlyPendingTimers() inside that act invocation to
only execute pending timers safely and avoid spinning, keeping the surrounding
act(...) and test logic unchanged.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6924 +/- ##
==========================================
+ Coverage 94.29% 94.33% +0.04%
==========================================
Files 571 573 +2
Lines 56002 56870 +868
Branches 5222 5253 +31
==========================================
+ Hits 52807 53650 +843
- Misses 3186 3211 +25
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Bundle ReportChanges will increase total bundle size by 25.88kB (0.93%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
Member
|
oops conflicts |
02186a0 to
59ba2f4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/chart/PieChart.spec.tsx (1)
703-705: Avoidanyin theouterRadiuscallback.Please type the parameter explicitly instead of using
any. For example, if the callback needs onlyvalue, a minimal shape works:♻️ Proposed fix
- outerRadius={(element: any) => { - return element.value / 10; - }} + outerRadius={(element: { value: number }): number => { + return element.value / 10; + }}As per coding guidelines, avoid
anyand provide explicit parameter types.
🤖 Fix all issues with AI agents
In `@src/state/keyboardEventsMiddleware.ts`:
- Around line 101-110: The throttling branch fails to run the callback when
isThrottled is true but throttleDelay is undefined; update the logic in the
block around isThrottled/callback and the throttleDelay checks (the code that
assigns rafId via requestAnimationFrame or timeoutId via setTimeout) to treat
undefined throttleDelay as a safe default (e.g., 'raf' or a numeric fallback).
For example, compute a local effectiveDelay = throttleDelay ?? 'raf' (or another
chosen default) before the existing if (effectiveDelay === 'raf') / else if
(typeof effectiveDelay === 'number') branches so the callback always gets
scheduled when throttled; adjust rafId/timeoutId assignment accordingly.
In `@src/state/touchEventsMiddleware.ts`:
- Around line 31-38: The current touch-move handling clears timeoutId on every
event which implements debounce instead of throttle; change the logic so when a
numeric throttleDelay is used you do NOT clear an existing timeoutId on
subsequent touchmove events—only set a timeout if timeoutId is null, and always
capture the latest event in a variable (e.g., latestTouchEvent) to be used by
the trailing update when the timeout fires; keep the rafId cancelation behavior
for animation frame scheduling but ensure the trailing timeout callback uses the
latest stored event to call the existing handler (reference rafId, timeoutId,
throttleDelay and the touchmove handler/dispatch logic), and apply the same
change to the other identical block around lines 105-109.
In `@www/src/docs/api/RadialBarChartAPI.tsx`:
- Around line 359-383: The documented prop type for throttledEvents uses invalid
TypeScript syntax (`Array<readonly (keyof GlobalEventHandlersEventMap)>`);
update the type string for the prop named "throttledEvents" to a valid form such
as `ReadonlyArray<keyof GlobalEventHandlersEventMap>` or `Array<keyof
GlobalEventHandlersEventMap>` in the RadialBarChartAPI prop definition and keep
the rest of the doc/description and defaultVal unchanged so the docs build
cleanly.
In `@www/src/docs/api/ScatterChartAPI.tsx`:
- Around line 364-386: The type string for the prop description
'throttledEvents' should use idiomatic TS readonly array syntax: replace the
current type string '"all" | Array<readonly (keyof
GlobalEventHandlersEventMap)>' with '"all" | ReadonlyArray<keyof
GlobalEventHandlersEventMap>' in the prop definition so the docs reflect the
conventional ReadonlyArray form; update the type value where 'throttledEvents'
is declared to that exact string.
🧹 Nitpick comments (1)
src/state/mouseEventsMiddleware.ts (1)
45-103: Add a fallback whenthrottleDelayis unexpectedIf
throttleDelayis neither'raf'nor a number, the throttled path currently drops the event. A defensive fallback keeps behavior aligned withexternalEventsMiddleware.🔧 Suggested patch
if (throttleDelay === 'raf') { rafId = requestAnimationFrame(callback); } else if (typeof throttleDelay === 'number') { timeoutId = setTimeout(callback, throttleDelay); + } else { + callback(); }
Comment on lines
+101
to
+110
| if (!isThrottled) { | ||
| callback(); | ||
| return; | ||
| } | ||
| const coordinate = selectCoordinateForDefaultIndex(state, 'axis', 'hover', String(nextIndex)); | ||
|
|
||
| listenerApi.dispatch( | ||
| setKeyboardInteraction({ | ||
| active: true, | ||
| activeIndex: nextIndex.toString(), | ||
| activeCoordinate: coordinate, | ||
| }), | ||
| ); | ||
| if (throttleDelay === 'raf') { | ||
| rafId = requestAnimationFrame(callback); | ||
| } else if (typeof throttleDelay === 'number') { | ||
| timeoutId = setTimeout(callback, throttleDelay); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing fallback when throttleDelay is undefined but event is throttled.
If throttleDelay is undefined and the event is in throttledEvents, the callback will never execute because none of the conditions match.
While initialEventSettingsState defaults throttleDelay to 'raf', a consumer could explicitly pass throttleDelay={undefined} with throttledEvents={['keydown']}, causing keyboard events to silently fail.
🛡️ Suggested defensive fix
if (!isThrottled) {
callback();
return;
}
if (throttleDelay === 'raf') {
rafId = requestAnimationFrame(callback);
} else if (typeof throttleDelay === 'number') {
timeoutId = setTimeout(callback, throttleDelay);
+ } else {
+ // Fallback: execute immediately if throttleDelay is invalid
+ callback();
}📝 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.
Suggested change
| if (!isThrottled) { | |
| callback(); | |
| return; | |
| } | |
| const coordinate = selectCoordinateForDefaultIndex(state, 'axis', 'hover', String(nextIndex)); | |
| listenerApi.dispatch( | |
| setKeyboardInteraction({ | |
| active: true, | |
| activeIndex: nextIndex.toString(), | |
| activeCoordinate: coordinate, | |
| }), | |
| ); | |
| if (throttleDelay === 'raf') { | |
| rafId = requestAnimationFrame(callback); | |
| } else if (typeof throttleDelay === 'number') { | |
| timeoutId = setTimeout(callback, throttleDelay); | |
| } | |
| if (!isThrottled) { | |
| callback(); | |
| return; | |
| } | |
| if (throttleDelay === 'raf') { | |
| rafId = requestAnimationFrame(callback); | |
| } else if (typeof throttleDelay === 'number') { | |
| timeoutId = setTimeout(callback, throttleDelay); | |
| } else { | |
| // Fallback: execute immediately if throttleDelay is invalid | |
| callback(); | |
| } |
🤖 Prompt for AI Agents
In `@src/state/keyboardEventsMiddleware.ts` around lines 101 - 110, The throttling
branch fails to run the callback when isThrottled is true but throttleDelay is
undefined; update the logic in the block around isThrottled/callback and the
throttleDelay checks (the code that assigns rafId via requestAnimationFrame or
timeoutId via setTimeout) to treat undefined throttleDelay as a safe default
(e.g., 'raf' or a numeric fallback). For example, compute a local effectiveDelay
= throttleDelay ?? 'raf' (or another chosen default) before the existing if
(effectiveDelay === 'raf') / else if (typeof effectiveDelay === 'number')
branches so the callback always gets scheduled when throttled; adjust
rafId/timeoutId assignment accordingly.
Comment on lines
+31
to
+38
| if (rafId !== null) { | ||
| cancelAnimationFrame(rafId); | ||
| rafId = null; | ||
| } | ||
| if (timeoutId !== null) { | ||
| clearTimeout(timeoutId); | ||
| timeoutId = null; | ||
| } |
Contributor
There was a problem hiding this comment.
Numeric throttleDelay currently behaves like debounce.
Clearing the timeout on every touchmove means the callback only fires after movement stops, so tooltips won’t update during continuous touch. This conflicts with the “throttle” semantics described in the docs. Consider scheduling only when no timer is pending and using the latest event for the trailing update.
🛠️ Example adjustment to achieve true throttling
let rafId: number | null = null;
let timeoutId: ReturnType<typeof setTimeout> | null = null;
+let latestTouchEvent: React.TouchEvent<HTMLDivElement> | null = null;
const touchEvent = action.payload;
if (touchEvent.touches == null || touchEvent.touches.length === 0) {
return;
}
-
- if (rafId !== null) {
- cancelAnimationFrame(rafId);
- rafId = null;
- }
- if (timeoutId !== null) {
- clearTimeout(timeoutId);
- timeoutId = null;
- }
+ latestTouchEvent = touchEvent;
const callback = () => {
+ const event = latestTouchEvent;
+ latestTouchEvent = null;
+ if (event == null) {
+ rafId = null;
+ timeoutId = null;
+ return;
+ }
const currentState = listenerApi.getState();
const tooltipEventType = selectTooltipEventType(currentState, currentState.tooltip.settings.shared);
if (tooltipEventType === 'axis') {
- const touch = touchEvent.touches[0];
+ const touch = event.touches[0];
if (touch == null) {
return;
}
const activeProps = selectActivePropsFromChartPointer(
currentState,
getChartPointer({
clientX: touch.clientX,
clientY: touch.clientY,
- currentTarget: touchEvent.currentTarget,
+ currentTarget: event.currentTarget,
}),
);
...
} else if (tooltipEventType === 'item') {
- const touch = touchEvent.touches[0];
+ const touch = event.touches[0];
...
}
rafId = null;
timeoutId = null;
};
- if (throttleDelay === 'raf') {
- rafId = requestAnimationFrame(callback);
- } else if (typeof throttleDelay === 'number') {
- timeoutId = setTimeout(callback, throttleDelay);
- }
+ if (rafId == null && timeoutId == null) {
+ if (throttleDelay === 'raf') {
+ rafId = requestAnimationFrame(callback);
+ } else if (typeof throttleDelay === 'number') {
+ timeoutId = setTimeout(callback, throttleDelay);
+ }
+ }Also applies to: 105-109
🤖 Prompt for AI Agents
In `@src/state/touchEventsMiddleware.ts` around lines 31 - 38, The current
touch-move handling clears timeoutId on every event which implements debounce
instead of throttle; change the logic so when a numeric throttleDelay is used
you do NOT clear an existing timeoutId on subsequent touchmove events—only set a
timeout if timeoutId is null, and always capture the latest event in a variable
(e.g., latestTouchEvent) to be used by the trailing update when the timeout
fires; keep the rafId cancelation behavior for animation frame scheduling but
ensure the trailing timeout callback uses the latest stored event to call the
existing handler (reference rafId, timeoutId, throttleDelay and the touchmove
handler/dispatch logic), and apply the same change to the other identical block
around lines 105-109.
Comment on lines
+359
to
+383
| { | ||
| name: 'throttledEvents', | ||
| type: '"all" | Array<readonly (keyof GlobalEventHandlersEventMap)>', | ||
| isOptional: true, | ||
| desc: { | ||
| 'en-US': ( | ||
| <section> | ||
| <p>Defines which events should be throttled. Events not in this list will not be throttled.</p> | ||
| <p> | ||
| Use the special value <code>'all'</code> to throttle all events. Empty array means no events are | ||
| throttled. | ||
| </p> | ||
| <p> | ||
| Use the prop <code>throttleDelay</code> to define the throttling interval. | ||
| </p> | ||
| <p> | ||
| If an event is on this list, then you lose the opportunity to access the event synchronously. Which means | ||
| that if you want to call <code>e.preventDefault()</code> or <code>e.stopPropagation()</code> inside the | ||
| event handler, then that event handler must not be in this list. | ||
| </p> | ||
| </section> | ||
| ), | ||
| }, | ||
| defaultVal: ['mousemove', 'touchmove', 'pointermove', 'scroll', 'wheel'], | ||
| }, |
Contributor
There was a problem hiding this comment.
Minor type syntax issue in documentation.
The type string Array<readonly (keyof GlobalEventHandlersEventMap)> has incorrect TypeScript syntax—readonly cannot modify a key type directly. Consider correcting to ReadonlyArray<keyof GlobalEventHandlersEventMap> or Array<keyof GlobalEventHandlersEventMap>.
The description itself is excellent and clearly explains the critical caveat about synchronous event access for preventDefault() and stopPropagation(), which directly addresses the linked issue #6883.
Proposed fix
{
name: 'throttledEvents',
- type: '"all" | Array<readonly (keyof GlobalEventHandlersEventMap)>',
+ type: '"all" | ReadonlyArray<keyof GlobalEventHandlersEventMap>',
isOptional: true,📝 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.
Suggested change
| { | |
| name: 'throttledEvents', | |
| type: '"all" | Array<readonly (keyof GlobalEventHandlersEventMap)>', | |
| isOptional: true, | |
| desc: { | |
| 'en-US': ( | |
| <section> | |
| <p>Defines which events should be throttled. Events not in this list will not be throttled.</p> | |
| <p> | |
| Use the special value <code>'all'</code> to throttle all events. Empty array means no events are | |
| throttled. | |
| </p> | |
| <p> | |
| Use the prop <code>throttleDelay</code> to define the throttling interval. | |
| </p> | |
| <p> | |
| If an event is on this list, then you lose the opportunity to access the event synchronously. Which means | |
| that if you want to call <code>e.preventDefault()</code> or <code>e.stopPropagation()</code> inside the | |
| event handler, then that event handler must not be in this list. | |
| </p> | |
| </section> | |
| ), | |
| }, | |
| defaultVal: ['mousemove', 'touchmove', 'pointermove', 'scroll', 'wheel'], | |
| }, | |
| { | |
| name: 'throttledEvents', | |
| type: '"all" | ReadonlyArray<keyof GlobalEventHandlersEventMap>', | |
| isOptional: true, | |
| desc: { | |
| 'en-US': ( | |
| <section> | |
| <p>Defines which events should be throttled. Events not in this list will not be throttled.</p> | |
| <p> | |
| Use the special value <code>'all'</code> to throttle all events. Empty array means no events are | |
| throttled. | |
| </p> | |
| <p> | |
| Use the prop <code>throttleDelay</code> to define the throttling interval. | |
| </p> | |
| <p> | |
| If an event is on this list, then you lose the opportunity to access the event synchronously. Which means | |
| that if you want to call <code>e.preventDefault()</code> or <code>e.stopPropagation()</code> inside the | |
| event handler, then that event handler must not be in this list. | |
| </p> | |
| </section> | |
| ), | |
| }, | |
| defaultVal: ['mousemove', 'touchmove', 'pointermove', 'scroll', 'wheel'], | |
| }, |
🤖 Prompt for AI Agents
In `@www/src/docs/api/RadialBarChartAPI.tsx` around lines 359 - 383, The
documented prop type for throttledEvents uses invalid TypeScript syntax
(`Array<readonly (keyof GlobalEventHandlersEventMap)>`); update the type string
for the prop named "throttledEvents" to a valid form such as
`ReadonlyArray<keyof GlobalEventHandlersEventMap>` or `Array<keyof
GlobalEventHandlersEventMap>` in the RadialBarChartAPI prop definition and keep
the rest of the doc/description and defaultVal unchanged so the docs build
cleanly.
Comment on lines
+364
to
+386
| name: 'throttledEvents', | ||
| type: '"all" | Array<readonly (keyof GlobalEventHandlersEventMap)>', | ||
| isOptional: true, | ||
| desc: { | ||
| 'en-US': ( | ||
| <section> | ||
| <p>Defines which events should be throttled. Events not in this list will not be throttled.</p> | ||
| <p> | ||
| Use the special value <code>'all'</code> to throttle all events. Empty array means no events are | ||
| throttled. | ||
| </p> | ||
| <p> | ||
| Use the prop <code>throttleDelay</code> to define the throttling interval. | ||
| </p> | ||
| <p> | ||
| If an event is on this list, then you lose the opportunity to access the event synchronously. Which means | ||
| that if you want to call <code>e.preventDefault()</code> or <code>e.stopPropagation()</code> inside the | ||
| event handler, then that event handler must not be in this list. | ||
| </p> | ||
| </section> | ||
| ), | ||
| }, | ||
| defaultVal: ['mousemove', 'touchmove', 'pointermove', 'scroll', 'wheel'], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and examine lines 364-386
head -n 386 www/src/docs/api/ScatterChartAPI.tsx | tail -n 30Repository: recharts/recharts
Length of output: 1218
🏁 Script executed:
# Check for linting configuration
find . -name "eslintrc*" -o -name "tsconfig.json" -o -name ".eslintignore" | head -20Repository: recharts/recharts
Length of output: 143
🏁 Script executed:
# Check the actual TypeScript syntax validity
cd www/src/docs/api && cat ScatterChartAPI.tsx | head -n 386 | tail -n 50Repository: recharts/recharts
Length of output: 1888
🏁 Script executed:
# Look at the beginning of this object and its context
head -n 370 www/src/docs/api/ScatterChartAPI.tsx | tail -n 20Repository: recharts/recharts
Length of output: 688
🏁 Script executed:
# Check if there are other Array<readonly ...> patterns in the file
rg "Array<readonly" www/src/docs/api/ScatterChartAPI.tsxRepository: recharts/recharts
Length of output: 135
🏁 Script executed:
# Look at the structure to understand what this config object is
head -n 300 www/src/docs/api/ScatterChartAPI.tsx | tail -n 50Repository: recharts/recharts
Length of output: 2186
Improve throttledEvents type string idiomaticity.
Array<readonly (keyof GlobalEventHandlersEventMap)> should be ReadonlyArray<keyof GlobalEventHandlersEventMap> for consistency with TypeScript conventions.
🛠️ Proposed fix
- type: '"all" | Array<readonly (keyof GlobalEventHandlersEventMap)>',
+ type: '"all" | ReadonlyArray<keyof GlobalEventHandlersEventMap>',📝 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.
Suggested change
| name: 'throttledEvents', | |
| type: '"all" | Array<readonly (keyof GlobalEventHandlersEventMap)>', | |
| isOptional: true, | |
| desc: { | |
| 'en-US': ( | |
| <section> | |
| <p>Defines which events should be throttled. Events not in this list will not be throttled.</p> | |
| <p> | |
| Use the special value <code>'all'</code> to throttle all events. Empty array means no events are | |
| throttled. | |
| </p> | |
| <p> | |
| Use the prop <code>throttleDelay</code> to define the throttling interval. | |
| </p> | |
| <p> | |
| If an event is on this list, then you lose the opportunity to access the event synchronously. Which means | |
| that if you want to call <code>e.preventDefault()</code> or <code>e.stopPropagation()</code> inside the | |
| event handler, then that event handler must not be in this list. | |
| </p> | |
| </section> | |
| ), | |
| }, | |
| defaultVal: ['mousemove', 'touchmove', 'pointermove', 'scroll', 'wheel'], | |
| name: 'throttledEvents', | |
| type: '"all" | ReadonlyArray<keyof GlobalEventHandlersEventMap>', | |
| isOptional: true, | |
| desc: { | |
| 'en-US': ( | |
| <section> | |
| <p>Defines which events should be throttled. Events not in this list will not be throttled.</p> | |
| <p> | |
| Use the special value <code>'all'</code> to throttle all events. Empty array means no events are | |
| throttled. | |
| </p> | |
| <p> | |
| Use the prop <code>throttleDelay</code> to define the throttling interval. | |
| </p> | |
| <p> | |
| If an event is on this list, then you lose the opportunity to access the event synchronously. Which means | |
| that if you want to call <code>e.preventDefault()</code> or <code>e.stopPropagation()</code> inside the | |
| event handler, then that event handler must not be in this list. | |
| </p> | |
| </section> | |
| ), | |
| }, | |
| defaultVal: ['mousemove', 'touchmove', 'pointermove', 'scroll', 'wheel'], |
🤖 Prompt for AI Agents
In `@www/src/docs/api/ScatterChartAPI.tsx` around lines 364 - 386, The type string
for the prop description 'throttledEvents' should use idiomatic TS readonly
array syntax: replace the current type string '"all" | Array<readonly (keyof
GlobalEventHandlersEventMap)>' with '"all" | ReadonlyArray<keyof
GlobalEventHandlersEventMap>' in the prop definition so the docs reflect the
conventional ReadonlyArray form; update the type value where 'throttledEvents'
is declared to that exact string.
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
7 tasks
ckifer
pushed a commit
that referenced
this pull request
Jan 29, 2026
## Description Followup to the robot review from #6924 which turned out to be correct and useful - the "throttle" implementation was actually "debounce" which doesn't make a whole lot difference if the timeframe is below 4ms but it was wrong so I fixed it - omnidoc interpreted `ReadonlyArray<X>` as `Array<readonly X>` which was wrong so I removed the readonly completely - it's irrelevant on the website anyway - removed a workaround for copilot bug because that bug is fixed already ## Related Issue #6924 ## Types of changes - [x] Bug fix (non-breaking change which fixes an issue) - but we did not release that bug yet - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist: - [ ] My change requires a change to the documentation. - [x] I have updated the documentation accordingly. - [x] I have added tests to cover my changes. - [ ] I have added a storybook story or VR test, or extended an existing story or VR test to show my changes <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Event handling: middlewares now track the latest event payload and apply leading-edge execution with proper cooldowns for mouse, touch, keyboard and external events. * **Documentation** * Removed many `readonly` qualifiers across API docs and Storybook arg types for array/coordinate props. * Removed a guidance line about unsetting NODE_ENV. * **Tests** * Added tests covering throttling behavior for mouse, keyboard and external events. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
throttleDelayprop to also include eventsthrottledEventsfor customizing which events are throttled and which are notRelated Issue
Fixes #6883
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
throttleDelayandthrottledEventsprops for improved performance during high-frequency interactions.refprop to chart components for direct SVG element access.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.