Copilot commented on code in PR #36133:
URL: https://github.com/apache/superset/pull/36133#discussion_r2543128460
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -67,8 +67,32 @@ export default function EchartsTimeseries({
const extraControlRef = useRef<HTMLDivElement>(null);
const [extraControlHeight, setExtraControlHeight] = useState(0);
useEffect(() => {
- const updatedHeight = extraControlRef.current?.offsetHeight || 0;
- setExtraControlHeight(updatedHeight);
+ const element = extraControlRef.current;
+ if (!element) {
+ setExtraControlHeight(0);
+ return;
+ }
+
+ const updateHeight = () => {
+ setExtraControlHeight(element.offsetHeight || 0);
+ };
+
+ updateHeight();
+
+ if (typeof ResizeObserver === 'function') {
+ const resizeObserver = new ResizeObserver(() => {
+ updateHeight();
+ });
+ resizeObserver.observe(element);
+ return () => {
+ resizeObserver.disconnect();
+ };
+ }
+
+ window.addEventListener('resize', updateHeight);
+ return () => {
+ window.removeEventListener('resize', updateHeight);
+ };
Review Comment:
[nitpick] The cleanup function for the window resize listener will always be
registered even when ResizeObserver is used. When ResizeObserver is available
(line 82-89), the function returns early at line 89, which means lines 92-95
are never executed. However, if ResizeObserver becomes unavailable during a
re-render or due to any condition change, the window event listener won't be
properly cleaned up. Consider refactoring to ensure only one event listener
type is active at a time, or ensure both paths have proper cleanup. The current
implementation is correct but could be more explicit. Actually, upon closer
inspection, the early return at line 89 prevents the window listener from being
registered when ResizeObserver is available, which is the correct behavior.
However, the code structure makes this non-obvious. Consider restructuring with
an else clause for better readability: `if (typeof ResizeObserver ===
'function') { ... } else { window.addEventListener('resize', updateHeigh
t); return () => { window.removeEventListener('resize', updateHeight); }; }`
```suggestion
} else {
window.addEventListener('resize', updateHeight);
return () => {
window.removeEventListener('resize', updateHeight);
};
}
```
--
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]
