Wang1rrr opened a new pull request, #4837:
URL: https://github.com/apache/rocketmq-dashboard/pull/4837
<!-- Make sure the base branch is `master`: that is the RocketMQ Studio
trunk. -->
### Which Issue(s) This PR Fixes
Trivial fix, no issue.
### Brief Description
`web/src/utils/apiError.ts` already exists to "extract the server-supplied
message from an API error so callers can surface the concrete rejection reason
instead of a generic fallback", and most of the console uses it -
`pages/instance/index.tsx` (7 call sites), `pages/ops/alerts.tsx`, and the AI
hooks via `describeThrownMessage(error) || t(...)`.
Four pages instead carried their own private extractor. Three of them were
byte-identical copies of each other; the fourth was a divergent sibling that
silently drops the server's answer.
**`pages/cluster/certs.tsx` - the bug this PR is about:**
```ts
const getErrorMessage = (error: unknown): string =>
error instanceof Error && error.message ? error.message : '请求失败，请稍后重试';
```
It never reads `response.data.message`. An axios rejection *is* an `Error`
whose `message` is the transport-level string, so every certificate failure on
this page renders `Request failed with status code 500` and the reason the
backend actually gave is thrown away:
| backend says | certificate page shows |
| --- | --- |
| `k8s cluster unreachable` | `Request failed with status code 500` |
| `certificate PEM does not match the private key` | `Request failed with
status code 400` |
| `k8sId already registered` | `Request failed with status code 409` |
This is the page where the operator is most dependent on the concrete reason
- a failed `createK8sCert` or `deleteK8sCert` gives them nothing actionable
today. The three call sites (list load, create, delete) all funnel through it.
**`pages/instance/message.tsx`, `pages/instance/dlq.tsx`,
`pages/cluster/clients.tsx` - the same helper three more times:**
```ts
type ApiErrorLike = { message?: unknown; response?: { data?: { message?:
unknown } } };
const getErrorMessage = (error: unknown, fallback: string): string => {
const apiError = error as ApiErrorLike;
const responseMessage = apiError.response?.data?.message; // <- unguarded
...
};
```
Identical bodies, identical `ApiErrorLike` declarations (10 call sites
between them). Two problems worth fixing while they are being deleted:
1. `error as ApiErrorLike` then `apiError.response` reads a property of the
thrown value with no guard. A rejection that carries nothing -
`Promise.reject()`, or an `await` on a promise rejected with `undefined` -
makes the extractor itself throw a `TypeError` **inside the catch block**, so
the error escapes the handler: no toast, no `setQueryError`, and `queryLoading`
stays stuck because the `finally` is skipped too. `describeApiError` already
guards this with `(error as {...})?.response?.data?.message`.
2. Three copies of one policy means the next fix has to be found and applied
three times - which is exactly how `certs.tsx` ended up with a fourth, weaker
variant.
**The change**
All four private extractors are deleted and their 15 call sites now use the
shared helper with the idiom the AI pages already established:
```ts
message.error(describeThrownMessage(error) ||
t('messagePage.directConsumeFailed'));
setLoadError(describeThrownMessage(error) || DEFAULT_LOAD_ERROR);
```
Every existing fallback string is preserved verbatim, including the
still-hardcoded Chinese ones (localising `certs.tsx` and `dlq.tsx` is a
separate change; this PR does not touch UI text).
`describeThrownMessage` gained one branch so that it is a strict superset of
the copies it replaces - they accepted a `message` string on *any* thrown
value, not only on `Error` instances:
```ts
const thrownMessage = (error as { message?: unknown } | null |
undefined)?.message;
if (typeof thrownMessage === 'string' && thrownMessage.trim()) return
thrownMessage;
```
Server message first, then `Error.message`, then a bare thrown object, then
`''` so the caller's `||` fallback applies. That matches its own doc comment
("best available machine-supplied message for anything thrown") and keeps the
AI hooks - which already call it - unchanged for every value they can actually
receive.
### How Did You Test This Change?
```
cd web
npx vitest run src/utils/apiError.test.ts
src/pages/cluster/__tests__/K8sCertsPage.test.tsx \
src/pages/cluster/__tests__/ClientsPage.test.tsx
src/pages/instance/__tests__/DLQPage.test.tsx \
src/pages/instance/__tests__/MessagePage.test.tsx
Test Files 5 passed (5)
Tests 73 passed (73)
npx vitest run
Test Files 1 failed | 139 passed (140)
Tests 1 failed | 1289 passed (1290)
the single failure is ConsumerPage > 'keeps the latest client stack when
an older request
resolves last', a timing-sensitive race assertion in a file this PR does
not touch; it passes
36/36 when the file is run on its own.
npx tsc -b (exit 0)
npx eslint <the 7 changed files>
exit 0; one pre-existing react-refresh/only-export-components warning on
dlq.tsx's
`export const formatDateTime`, which is on the trunk revision too and is
not touched here
npx prettier --check --end-of-line auto <same files>
All matched files use Prettier code style!
```
(`--end-of-line auto` because this checkout is CRLF while `.prettierrc` pins
`endOfLine: lf`; untouched trunk files fail a plain `--check` here for the same
reason.)
New unit tests, `web/src/utils/apiError.test.ts` (the helper had none):
- `describeApiError` prefers the server message, falls back on a
blank/non-string server message and on a plain `Error`, and does not throw for
`undefined` / `null`;
- `describeThrownMessage` prefers the server message over the transport
message, falls back to `Error.message`, accepts a bare `{ message: 'rate
limited' }`, and returns `''` for `undefined`, `null`, `{}`, `{ message: 42 }`
and `new Error(' ')`.
New page tests in `K8sCertsPage.test.tsx`:
- `surfaces the server rejection reason when the certificate list cannot be
loaded` - a rejection carrying `response.data.message = 'k8s cluster
unreachable'` shows that text and **not** `Request failed with status code 500`;
- `accepts a rejection reason carried by a bare object`;
- `keeps the generic fallback when the rejection carries nothing usable`.
Both fixes were mutation-checked:
- restoring the old `certs.tsx` one-liner fails exactly the 2 new
behavioural page tests, the 7 pre-existing ones stay green;
- removing the new bare-object branch from `describeThrownMessage` fails
exactly `accepts a bare object thrown without an Error` and the matching page
test.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`feat:` / `fix:` /
`refactor:` / `chore:` / `docs:` / `perf:`)
- [x] Tests added or updated for non-trivial changes, test methods named
`...Test` (frontend; Java test naming does not apply)
- [x] New UI text has both Chinese and English entries under `web/src/i18n/`
(no UI text added or changed - every existing fallback string is preserved
verbatim)
- [x] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks) - no backend change
- [x] New source files carry the ASF license header
(`web/src/utils/apiError.test.ts`)
- [x] Documentation touched where behaviour changed (README / `docs/` /
in-app help) - none needed; `apiError.ts`'s doc comment already describes the
widened contract
--
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]
