RockteMQ-AI commented on PR #1506:
URL:
https://github.com/apache/rocketmq-dashboard/pull/1506#issuecomment-5245724870
The repo isn't checked out locally, so I'm reviewing from the diff provided.
The core fix uses a standard React `active`-flag pattern to ignore stale
catalog responses — let me verify the implementation and test coverage
carefully against the PR's stated goals.
## Review Summary
The stale-response guard (the `active` flag in each catalog `useEffect`) is
implemented correctly and is the right pattern. The two regression tests
genuinely exercise the dangerous out-of-order resolution order. However,
`handleVendorChange` resets option arrays and form values but **not the loading
flags**, which can orphan a spinner when the user switches vendor while a
catalog request is in flight — directly contradicting the PR's claim that
"loading state [is] owned by the latest active request." There's also no test
for the vendor-switch path the PR explicitly claims to fix.
## Findings
### [Medium] `handleVendorChange` does not reset loading flags, leaving
spinners stuck
- File: `web/src/pages/instance/index.tsx` — `handleVendorChange`
- Description: The new handler calls `setCredentials([])`, `setRegions([])`,
`setCloudInstances([])`, and `addForm.setFieldsValue({ credentialId: undefined,
regionId: undefined, cloudInstanceId: undefined })`, but never touches
`setCredentialsLoading` / `setRegionsLoading` / `setCloudInstancesLoading`.
Trace the regions/instances effects after a vendor switch: their cleanup sets
`active = false` (so the in-flight request's `.finally` won't fire
`setRegionsLoading(false)`), and the re-run **early-returns** because
`addCredentialId` is now `undefined` — so nothing ever clears the loading flag.
Result: if the user switches vendor (e.g. Aliyun → Tencent) while regions or
instances are still loading, the region/instance `Select` shows a perpetual
spinner with no options until the user picks a credential and region on the new
vendor. This is different from `handleCredentialChange`, which doesn't have the
problem because its effect re-runs (non-early-returning) and re-sets then clear
s the flag.
- Suggestion: Reset the loading flags in `handleVendorChange`:
```tsx
const handleVendorChange = (nextVendor: string) => {
setVendor(nextVendor as InstanceVendor);
setCredentials([]);
setRegions([]);
setCloudInstances([]);
setCredentialsLoading(false);
setRegionsLoading(false);
setCloudInstancesLoading(false);
addForm.setFieldsValue({ credentialId: undefined, regionId: undefined,
cloudInstanceId: undefined });
};
```
Optionally also guard each effect's early-return path to reset its own
loading flag (e.g. `if (!cloudVendor || !addCredentialId) {
setRegionsLoading(false); return; }`) as defense-in-depth.
### [Low] No test coverage for the vendor-switch path
- File: `web/src/pages/instance/__tests__/InstancePage.test.tsx`
- Description: The PR description lists "clear cloud-specific form values
when switching between Apache, Aliyun, and Tencent vendors" as a goal, and
`handleVendorChange` is the single most substantial behavioral addition in the
diff. Yet neither new test exercises a vendor tab switch — they only cover
credential-change (stale regions) and region-change (stale instances). The
loading-flag bug above would not be caught by the current suite, and neither
would a regression where form values leak across vendors.
- Suggestion: Add a test that selects a credential + region on Aliyun,
switches to the Tencent tab, and asserts (a)
`credentialId`/`regionId`/`cloudInstanceId` form values are cleared, (b) the
region/instance option lists are empty, and (c) no loading spinner remains.
Also consider a variant that switches vendor *while* a region request is
pending to lock in the loading-state fix.
### [Low] Inconsistent stale-guard style across the three effects
- File: `web/src/pages/instance/index.tsx`
- Description: The credentials and instances effects wrap their `.then`
bodies in `if (active) { ... }`, but the regions effect uses an early `if
(!active) { return; }` inside `.then`. Both are correct; the inconsistency just
makes the three near-identical blocks harder to scan.
- Suggestion: Pick one style (the early-return form is slightly cleaner
since the `.then` has multiple statements) and apply it uniformly.
## Verdict
REQUEST_CHANGES — The core stale-response fix is sound and the ordering
tests are well constructed, but `handleVendorChange` leaves loading flags
un-reset, which can orphan a spinner and undermines the PR's stated goal of
keeping loading state owned by the latest request. Adding the three
`set*Loading(false)` calls (plus a vendor-switch test) closes the gap.
--
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]
