korbit-ai[bot] commented on code in PR #33728:
URL: https://github.com/apache/superset/pull/33728#discussion_r2136669757
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -139,6 +141,20 @@ export default function EchartsTimeseries({
[emitCrossFilters, setDataMask, getCrossFilterDataMask],
);
+ // Helper function to restore legend page position
+ const restoreLegendPagePosition = useCallback(() => {
+ // Use requestAnimationFrame instead of setTimeout for better alignment
with the browser's rendering cycle
+ requestAnimationFrame(() => {
+ const echartInstance = echartRef.current?.getEchartInstance();
+ if (echartInstance) {
+ echartInstance.dispatchAction({
+ type: 'legendScroll',
+ scrollDataIndex: currentLegendPage,
+ });
+ }
+ });
Review Comment:
### Race condition in legend position restoration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The legend position restoration might fail if the chart is not fully
rendered when requestAnimationFrame executes.
###### Why this matters
The chart might not be ready in the next animation frame, causing the scroll
action to have no effect. This can lead to inconsistent legend positioning
behavior.
###### Suggested change ∙ *Feature Preview*
Add a small delay to ensure the chart has time to render before attempting
to restore the legend position:
```typescript
const restoreLegendPagePosition = useCallback(() => {
setTimeout(() => {
requestAnimationFrame(() => {
const echartInstance = echartRef.current?.getEchartInstance();
if (echartInstance) {
echartInstance.dispatchAction({
type: 'legendScroll',
scrollDataIndex: currentLegendPage,
});
}
});
}, 100);
}, [currentLegendPage]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80448e77-f865-4043-aede-69515db557e1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80448e77-f865-4043-aede-69515db557e1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80448e77-f865-4043-aede-69515db557e1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80448e77-f865-4043-aede-69515db557e1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80448e77-f865-4043-aede-69515db557e1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8f1b5214-15aa-4cf8-b2e0-4955858e0ec9 -->
[](8f1b5214-15aa-4cf8-b2e0-4955858e0ec9)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -139,6 +141,20 @@
[emitCrossFilters, setDataMask, getCrossFilterDataMask],
);
+ // Helper function to restore legend page position
+ const restoreLegendPagePosition = useCallback(() => {
+ // Use requestAnimationFrame instead of setTimeout for better alignment
with the browser's rendering cycle
+ requestAnimationFrame(() => {
+ const echartInstance = echartRef.current?.getEchartInstance();
+ if (echartInstance) {
+ echartInstance.dispatchAction({
+ type: 'legendScroll',
+ scrollDataIndex: currentLegendPage,
+ });
+ }
+ });
Review Comment:
### Redundant Legend Position Restorations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The restoreLegendPagePosition function is called multiple times in rapid
succession for each legend event (selectchanged, selectall, inverseselect),
potentially causing unnecessary reflows and rerenders.
###### Why this matters
Multiple rapid-fire calls to requestAnimationFrame with the same
scrollDataIndex can waste CPU cycles and potentially cause visual jank,
especially on lower-end devices.
###### Suggested change ∙ *Feature Preview*
Debounce the restoreLegendPagePosition calls to ensure only one restoration
occurs after a batch of legend changes:
```typescript
const debouncedRestoreLegendPosition = useCallback(
debounce(() => {
requestAnimationFrame(() => {
const echartInstance = echartRef.current?.getEchartInstance();
if (echartInstance) {
echartInstance.dispatchAction({
type: 'legendScroll',
scrollDataIndex: currentLegendPage,
});
}
});
}, 100),
[currentLegendPage]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e212283-d4e2-4735-a977-2b855a14ff97/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e212283-d4e2-4735-a977-2b855a14ff97?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e212283-d4e2-4735-a977-2b855a14ff97?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e212283-d4e2-4735-a977-2b855a14ff97?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e212283-d4e2-4735-a977-2b855a14ff97)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:689f242a-24ae-46ea-bec3-eda4414a0035 -->
[](689f242a-24ae-46ea-bec3-eda4414a0035)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
