RockteMQ-AI commented on PR #4189:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4189#issuecomment-5597639650
Verification complete. My applied diff matched the PR exactly; the tree is
restored to clean state. Here's my review:
---
## Code Review: PR #4189 — fix(metrics): keep the selected range when the
instance changes
### 1. Summary
This PR fixes the Metrics Explorer on the home dashboard so that switching
the 实例 (instance) selector reloads panels with the user's currently selected
time range instead of silently resetting to the 1h default while the Segmented
control still shows the picked range. The fix adds a `rangeIdRef` (synced via
effect) and resolves the reload range from it, plus a regression test.
### 2. Verification Performed
I independently reproduced the bug and verified the fix (applied the diff to
a clean checkout at `3612602`):
- **Premise confirmed**: `dashboard.tsx:318` renders `<MetricsExplorer
instanceId={selectedInstanceId} />` with no remount key, and the re-run chain
holds: `runQuery` depends on `instanceId` (`MetricsExplorer.tsx:480`) →
`loadAll` depends on `runQuery` (line 515) → the profiles effect depends on
`loadAll` (line 540), so it re-fires on every instance switch.
- **Red state confirmed**: with only the test applied, the new test fails
exactly as described (`start 179996400, step '30s'` — the 1h default).
- **Green state confirmed**: 20/20 pass, stable across 3 consecutive
full-file runs.
- `tsc --noEmit` clean; `eslint` on both files 0 errors/warnings.
- Full web suite: only the pre-existing `ConsumerPage.test.tsx` flake fails
(passes 29/29 in isolation with this change applied) — untouched file,
consistent with the PR's disclosure.
### 3. Issues Found
- **[SUGGESTION]** Custom query panel is not refreshed on instance switch.
The reload path (`MetricsExplorer.tsx:531-541`) calls `loadAll` but not
`runCustomQuery`, so a previously applied custom PromQL result keeps showing
the *old* instance's data until manually re-run — the same class of staleness
this PR fixes for panels. Pre-existing behavior, out of scope here, but a
natural follow-up (note the refresh button at line 776-779 does re-run it).
- **[SUGGESTION]** PR description says the file suite is "21/21 passed"; the
file at this base contains 20 tests (including the new one). Likewise
full-suite counts differ slightly (945 claimed vs 923 here) — likely a
different sandbox base. Cosmetic discrepancy only; the failure pattern (single
unrelated ConsumerPage flake) matches.
- **[SUGGESTION]** The `?? RANGE_OPTIONS[0]` fallback in the new lookup is
technically unreachable (`rangeIdRef` is typed to `RANGE_OPTIONS` values), but
it's harmless defensive parity with `handleRangeChange` (line 551) and
`selectedRange` (line 436). No change needed.
No critical or warning-level issues found. Specifically checked:
- **Mount path unchanged**: `rangeId` initializes to `'1h'`, so `rangeIdRef`
starts at `'1h'` and the first load still uses `RANGE_OPTIONS[0]` semantics.
- **No race**: the reload reads `rangeIdRef.current` inside the `.then()` of
`listMetricProfiles()`, i.e., after React has committed and the sync effect has
run — the ref is always current at read time.
- **Ref design is right**: adding `rangeId` to the effect deps would
re-trigger profile fetching on every range change (duplicate requests); the ref
avoids that. The dedicated sync effect is arguably more robust than the
imperative sync used for `dataSourceKeyRef`.
- **Test determinism**: waiting for `toHaveBeenCalledTimes(3)` before
asserting avoids asserting before the reload lands; `localStorage` is cleared
per-test in `src/test/setup.ts`; no cross-test leakage.
- **Security**: no API, auth, or query-surface changes — only the
`start`/`step` values of existing `queryMetrics`/`queryByDataSource` calls
change.
### 4. Overall Assessment
**APPROVE**
Minimal, correct fix with a verified red→green regression test. The behavior
now matches the refresh-all button (line 776), which already uses the selected
range. The only notes are a pre-existing adjacent gap (custom panel staleness
on instance switch) worth a follow-up issue, and trivial PR-description count
discrepancies.
--
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]
