unbridled-41 opened a new pull request, #4588:
URL: https://github.com/apache/rocketmq-dashboard/pull/4588
Fixes #4587.
## Problem / Evidence
`GeneralSettingsTab` loads the settings record once and fills the
notification and security forms from the response:
```tsx
useEffect(() => {
let cancelled = false;
void getGeneralSettings()
.then((loaded) => {
if (cancelled) return;
setSettings(loaded);
securityForm.setFieldsValue({ sessionTimeout: loaded.sessionTimeout
});
notifyForm.setFieldsValue({ dingtalkWebhook: …, emailRecipients: …,
smsWebhook: … });
})
…
}, [message, notifyForm, securityForm, t]); // :100
```
`message` (`App.useApp()`), `securityForm`/`notifyForm` (`Form.useForm()`)
are stable; `t` is recreated on every display-language change
(`web/src/i18n/LangContext.tsx`). So switching the language re-runs the effect
and overwrites both forms from the server snapshot — the operator's unsaved
webhook / recipient / session-timeout edits silently disappear. Reproduced by
the regression test added here
(`web/src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx`), which
switches the language through the real `LangProvider.setLang` and then asserts
the field value.
## Root cause / Fix
A load-once effect whose dependency list contains the translation function:
the effect's job (fetch on mount, seed the forms) is unrelated to rendering
translated text, but the dependency made the language toggle a re-seed trigger.
The fix follows the pattern this repository already uses for the same problem
(`web/src/pages/studio/LiteTopic.tsx:126-161`, `messageRef`/`translationRef`):
```diff
const securityInFlightRef = useRef(false);
const notifyInFlightRef = useRef(false);
+ const translationRef = useRef(t);
const [securityForm] = Form.useForm();
const [notifyForm] = Form.useForm();
+ // The load effect below fills both forms from the server snapshot and
must therefore run only
+ // once per mount, while the translation function changes identity with
the display language.
+ useEffect(() => {
+ translationRef.current = t;
+ }, [t]);
+
useEffect(() => {
…
- if (!cancelled) message.error(t('settings.loadFailed'));
+ if (!cancelled)
message.error(translationRef.current('settings.loadFailed'));
…
- }, [message, notifyForm, securityForm, t]);
+ }, [message, notifyForm, securityForm]);
```
Refs are written in an effect (not in render), so the repository's
`react-hooks/set-state-in-effect` rule stays satisfied; the error message still
follows the current language.
## Priority & scoring
- PRIORITY **70** = impact 28 + blast radius 8 + reproducibility 20 +
maintenance value 14
- impact 28 — silent loss of user input on a settings form (webhook URL,
signing secret, recipient list, session timeout); the operator only notices
when the saved value turns out to be the old one.
- blast radius 8 — one tab, but it is the tab where notification channel
configuration lives, and the trigger (top-bar language toggle) is one click
away.
- reproducibility 20 — deterministic; the regression test drives the real
language switch.
- maintenance value 14 — removes a trap that recurs across this codebase
(the same dependency shape was already fixed in `LiteTopic`/`#2200`); the ref
pattern is the established remedy.
- FIX_CONFIDENCE **92** — the intent is unambiguous (only `t` is unstable,
and the effect's whole purpose is a one-time seed), the fix mirrors an existing
in-repo solution, and the full suite stays green.
## Tests
Environment: Node 24.20.0, `web/` at the PR head.
Red — base source (`d50ffecc`) with the new test:
```
$ git checkout origin/master -- web/src/pages/settings/GeneralSettingsTab.tsx
$ npx vitest run
src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx
FAIL src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx >
GeneralSettingsTab unsaved input > keeps unsaved notification edits when the
display language changes
Error: expect(element).toHaveValue(…)
Expected the element to have value:
https://oapi.dingtalk.com/robot/send?access_token=edited
Received:
https://oapi.dingtalk.com/robot/send?access_token=server
❯ src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx:108:21
Test Files 1 failed (1)
Tests 1 failed (1)
```
Green — with the fix, including the tab's existing suites:
```
$ npx vitest run
src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx \
src/pages/settings/__tests__/GeneralSettingsTab.test.tsx \
src/pages/settings/__tests__/SettingsPage.test.tsx
Test Files 3 passed (3)
Tests 9 passed (9)
```
Full suite:
```
$ npx vitest run --maxWorkers=4
Test Files 123 passed (123)
Tests 1035 passed (1035)
```
1035 = 1034 (`origin/master`) + 1 new test. (The default-parallel `npx
vitest run` on this repository intermittently reports failures in the
load-fragile files — MetricsExplorer, ConsumerPage, ClusterPage, TopicPage,
InstancePage, AlertsPage, AuditPage, ClientsPage,
MessagePage/MessagePageAsyncState — on pristine `master` as well; the
limited-parallelism run above is the contention-free result, and the touched
files pass in both.)
Static checks:
```
$ npx tsc -b # clean, exit 0
$ npx eslint src/pages/settings/GeneralSettingsTab.tsx
src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx
# no output: 0 errors, 0 warnings
$ npm run build # ✓ built in 11.82s
```
Diff: `web/src/pages/settings/GeneralSettingsTab.tsx` +9/−2,
`web/src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx` +110/−0
(`git show --numstat`).
## Risk
- The load effect no longer re-fetches on a language change. That request
only re-seeded the forms and the `settings` snapshot; translated labels are
re-rendered from `t` as before, and the tab still re-reads the record before
every save (`loadFreshSettings`), so a save cannot write a stale snapshot.
- If the record changes elsewhere while this tab is open, the previous
behaviour would not have refreshed it either (the same effect only ran on mount
and on language changes).
- No API, contract or translation-key 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]
