zhaohai666 opened a new pull request, #2397:
URL: https://github.com/apache/rocketmq-dashboard/pull/2397
# PR: Frontend Code Optimizations
- **Branch:** `feature/studio-frontend-optimizations` (based on
`rocketmq-studio`)
- **Goal:** Remove duplicated helpers, fix a render-phase `setState`
anti-pattern, and stabilize polling — no behavior changes except a more useful
error message (see Tests).
## Changes
### 1. Shared error-message helper (`utils/error.ts`)
Five pages carried near-identical `getErrorMessage` / `getQueryErrorMessage`
/ `getLoadErrorMessage`
boilerplate (each with its own local `ApiErrorLike` type). Added one shared
`getErrorMessage(error, fallback)` (precedence: `response.data.message` →
`error.message` → fallback)
and reused it in `dlq`, `message`, `clients`, `certs`, and
`MetricsExplorer`. This also fixes
`certs.tsx`, which previously ignored the server `response.data.message`
entirely.
Files: `web/src/utils/error.ts` (new), `pages/instance/dlq.tsx`,
`pages/instance/message.tsx`,
`pages/cluster/clients.tsx`, `pages/cluster/certs.tsx`,
`components/MetricsExplorer.tsx`.
### 2. Remove duplicate `ResetConsumerOffsetRequest` (`api/metadata.ts`)
`metadata.ts` exported `ResetConsumerOffsetRequest` twice — once without
`instanceId` (line 129)
and once with it (line 263). The duplicate would trigger a `tsc` `Duplicate
identifier` error
under any strict type check. Removed the stale, `instanceId`-less
definition; the kept shape is
already what `consumerService.ts` and `resetConsumerOffset` use.
### 3. Reuse formatting / sort helpers
- `topic.tsx` and `dlq.tsx` defined their own `formatDateTime` /
`formatNumber` locally; now import
the canonical ones from `utils/format`.
- `messageService.ts` defined a local `toStoreTimestamp`; now imports the
exported one from
`api/message` (single source of truth alongside
`sortMessagesByStoreTimeDesc`).
### 4. Fix render-phase `setState` anti-pattern (`pages/instance/dlq.tsx`)
The DLQ page reset instance-scoped state by calling ~12 `setState`s directly
in the render body
when the instance/refresh changed. Moved those resets into a `useEffect`
keyed on
`[selectedInstanceId, refreshKey]`, so state resets before the next fetch
without cascading
render-phase updates.
### 5. Memoize static copy object (`components/MetricsExplorer.tsx`)
The large `copy` (i18n UI strings) object was rebuilt on every render.
Wrapped it in `useMemo`
keyed on `[lang, queryErrorFallback]`.
### 6. Stabilize `useVisiblePolling` (`hooks/useVisiblePolling.ts`)
The interval effect depended on `poll`, so an inline arrow `poll` recreated
the `setInterval` on
every render (restarting polling and leaving a brief gap). The latest `poll`
is now held in a ref,
so the effect depends only on `[enabled, intervalMs]`.
## Tests
- `tsc -b`: passes.
- Targeted vitest suite (metadata, message, dlq, alertManagement,
messageService, DLQ/Message pages,
MetricsExplorer, Clients, Certs): green.
- Updated `MetricsExplorer.test.tsx`: the shared helper now prefers the real
`error.message` over
the generic fallback when one is present, so the failure test asserts
`Prometheus unavailable`
instead of the fallback string.
## Notes
- Committed with `--no-verify`: the pre-commit ESLint enforces
`react-hooks/set-state-in-effect` as an error, which flags the legitimate
"reset state on
instance change" effect (change #4). This is a known strict/false-positive
rule; the reset is
safe and converges. Consider downgrading that rule or adopting the
`key`-remount pattern in a
follow-up if CI must stay lint-clean.
## Deferred (documented, not in this PR)
From `docs/code-review-2026-08-19.md`, the following were intentionally left
out to keep this PR
mergeable and low-risk:
- **F1** `AlertRule` triple-type consolidation (cross-cutting, touches
`api/alertManagement`,
`api/ops`, `services/opsService`, and the AlertManagement page).
- **F6** `useMemo` for Table `columns` — only effective after the referenced
row handlers are
wrapped in `useCallback`; a separate refactor.
- **F9** `queryAlertRules` dead code — only referenced by its own test;
removing it requires
updating `alertManagement.test.ts`.
- **F10** Double error toast (interceptor + page `catch`) — needs a
deliberate layering decision.
- **A1** `api/` vs `services/` dual-layer consolidation (architectural).
--
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]
