unbridled-41 opened a new pull request, #4586:
URL: https://github.com/apache/rocketmq-dashboard/pull/4586
Fixes #4585.
## Problem / Evidence
`TopicConfigComparisonDrawer` starts a comparison with a request token and
only renders the response when the token is still current:
```tsx
const requestId = ++requestIdRef.current; // :128
if (requestId === requestIdRef.current) { … } // :135 / :141 / :143
```
But the three paths that change the compared pair clear the result without
invalidating the in-flight request — 源实例 `onChange` (`:211`), 目标实例 `onChange`
(`:224`) and `swapInstances` (`:147`, used by the swap button). A response that
is already in flight therefore repopulates the result after the user switched
the pair.
The rendered output is then mislabelled, because the labels come from live
state: the expanded field table columns are titled with
`sourceInstanceId`/`targetInstanceId` (`:313`), and 导出结果 writes
`rocketmq-topic-config-<source>-vs-<target>.csv` (`:164`) — both reflect the
*new* pair, while `result.rows` belong to the old one. The user can click 开始对比,
switch instances while it loads, and read/export a drift report for a pair they
did not select.
Regression test added in this PR: the `discards a comparison that resolves
after the instance pair changed` case in
`web/src/components/__tests__/TopicConfigComparisonDrawer.test.tsx` (two
`listAllTopics` promises are held open, the 源实例 is switched, then both promises
resolve).
## Root cause / Fix
Request ownership was attached to the *action* (starting a comparison)
instead of the *pair being compared*: the guard is only as good as the set of
paths that bump it, and every path that changes the pair was left out. The fix
introduces one helper for "the pair changed" and calls it from all three paths:
```diff
+ // Any change of the compared pair invalidates a comparison that is still
loading, so a
+ // response for the previous pair can never be rendered under the new
selection.
+ const invalidateComparison = () => {
+ requestIdRef.current += 1;
+ setResult(null);
+ setLoading(false);
+ };
+
const swapInstances = () => {
setSourceInstanceId(targetInstanceId);
setTargetInstanceId(sourceInstanceId);
- setResult(null);
+ invalidateComparison();
};
```
…and the same replacement in both `onChange` handlers. Clearing `loading` in
the helper matters: without it the button would keep spinning, because the
superseded response's `finally` block no longer resets it.
## Priority & scoring
- PRIORITY **75** = impact 28 + blast radius 12 + reproducibility 20 +
maintenance value 15
- impact 28 — a wrong cross-instance drift report is attributed to the
wrong instance pair, including in the CSV export; the user acts on data from a
different pair than the one displayed.
- blast radius 12 — reachable for every instance pair from the topic page,
and the wrong labels travel off-page through the export.
- reproducibility 20 — deterministic; captured by one regression test with
two held-open requests.
- maintenance value 15 — restores the request-ownership contract this repo
already standardised in #3154/#4166 (merged) and #4331 (merged refactor); one
shared helper prevents the omission from recurring at the next pair-changing
path.
- FIX_CONFIDENCE **92** — the guard already exists and is used by
`runComparison`; the fix only adds the missing invalidation paths, and the
sibling pages define the intended semantics.
## Tests
Environment: Node 24.20.0, `web/` at the PR head.
Red — with the base source and the new test:
```
$ git checkout origin/master --
web/src/components/TopicConfigComparisonDrawer.tsx
$ npx vitest run
src/components/__tests__/TopicConfigComparisonDrawer.test.tsx
FAIL src/components/__tests__/TopicConfigComparisonDrawer.test.tsx >
TopicConfigComparisonDrawer > discards a comparison that resolves after the
instance pair changed
Error: expect(element).not.toBeInTheDocument()
expected document not to contain element, found <div
class="ant-statistic-title">配置一致</div> instead
❯ src/components/__tests__/TopicConfigComparisonDrawer.test.tsx:243:44
Test Files 1 failed (1)
Tests 1 failed | 7 passed (8)
```
Green — with the fix:
```
$ npx vitest run
src/components/__tests__/TopicConfigComparisonDrawer.test.tsx
Test Files 1 passed (1)
Tests 8 passed (8)
$ npx vitest run src/pages/instance/__tests__/TopicPage.test.tsx # the
page that mounts the drawer
Test Files 1 passed (1)
Tests 26 passed (26)
```
Full suite:
```
$ npx vitest run --maxWorkers=4
Test Files 122 passed (122)
Tests 1035 passed (1035)
```
1035 = 1034 (`origin/master`) + 1 new test. The default-parallel `npx vitest
run` on this same head reported 23 failures across 10 files, all of them files
this PR does not touch (MetricsExplorer, ClientsPage, ClusterPage,
ConsumerPage, InstancePage, MessagePage, MessagePageAsyncState, TopicPage,
AlertsPage, AuditPage) — the load-fragile set that also fails intermittently on
pristine `master` when the whole suite runs in parallel. Isolating exactly
those 10 files passes:
```
$ npx vitest run <those 10 files>
Test Files 10 passed (10)
Tests 226 passed (226)
```
Static checks:
```
$ npx tsc -b # clean, exit 0
$ npx eslint src/components/TopicConfigComparisonDrawer.tsx
src/components/__tests__/TopicConfigComparisonDrawer.test.tsx
# no problems (0 errors, 0 warnings)
$ npm run build # ✓ built in 14.80s
```
Diff: `web/src/components/TopicConfigComparisonDrawer.tsx` +11/−3,
`web/src/components/__tests__/TopicConfigComparisonDrawer.test.tsx` +29/−1
(`git show --numstat`).
## Risk
- The helper only ever discards a comparison the user has already moved away
from; the flow for a stable pair (start → rows → expand → export) is unchanged,
and the existing 7 tests of the file, including export and search/filter tests,
still pass.
- `setLoading(false)` in the helper is what keeps the 开始对比 button usable
after a pair change; without it the superseded response's `finally` would no
longer clear it.
- No API, contract or translation changes.
--
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]
