RockteMQ-AI commented on code in PR #2629:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2629#discussion_r3862788641
##########
web/src/components/MetricsExplorer.tsx:
##########
@@ -554,12 +563,15 @@ const MetricsExplorer = ({ instanceId }:
MetricsExplorerProps) => {
window.setTimeout(() => {
// Keep the ref in sync with the state; queries read the ref, so a
stale key would
// keep hitting the de-registered data source while the UI shows the
default.
+ dataSourceCredentialsRef.current = null;
dataSourceKeyRef.current = '';
setDataSourceKey('');
setData(null);
+ setPendingDataSource(null);
+ void loadMetrics(selectedMetric, selectedRange);
}, 0);
}
- }, [availableDataSources, dataSourceKey]);
+ }, [availableDataSources, dataSourceKey, loadMetrics, selectedMetric,
selectedRange]);
Review Comment:
**[Warning]** Adding `loadMetrics`, `selectedMetric`, and `selectedRange` to
this effect's dependency array changes the original behavior significantly.
The original effect only ran when `availableDataSources` or `dataSourceKey`
changed (i.e., on instance switch or data source selection). With the new
dependencies, this effect will also fire every time the user selects a
different metric or time range, which will:
1. Reset `dataSourceKeyRef.current` to `''` (clearing the data source
selection)
2. Clear the chart data via `setData(null)`
3. Reload metrics from the default data source
This means every metric/range change would lose the user's selected data
source — likely a regression.
**Suggested fix:** Use refs to capture the current metric/range values
without adding them to the dependency array:
```typescript
const selectedMetricRef = useRef(selectedMetric);
const selectedRangeRef = useRef(selectedRange);
useEffect(() => {
selectedMetricRef.current = selectedMetric;
selectedRangeRef.current = selectedRange;
}, [selectedMetric, selectedRange]);
// Then in the reset effect:
useEffect(() => {
window.setTimeout(() => {
// ... reset logic ...
void loadMetrics(selectedMetricRef.current, selectedRangeRef.current);
}, 0);
}, [availableDataSources, dataSourceKey]); // keep original deps only
```
Alternatively, if the intent is to also reload on metric/range change,
consider separating that into a distinct `useEffect` with a clear comment
explaining the behavior.
--
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]
