You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "unbridled-41 (via GitHub)" <gi...@apache.org> on 2026/09/20 07:13:45 UTC
[PR] fix(settings): keep unsaved edits when the display language changes [rocketmq-dashboard]
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(settings): keep unsaved edits when the display language changes [rocketmq-dashboard]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins merged PR #4588:
URL: https://github.com/apache/rocketmq-dashboard/pull/4588
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(settings): keep unsaved edits when the display language changes [rocketmq-dashboard]
Posted by "unbridled-41 (via GitHub)" <gi...@apache.org>.
unbridled-41 commented on PR #4588:
URL: https://github.com/apache/rocketmq-dashboard/pull/4588#issuecomment-5748334908
### Verification evidence
**CI** — this PR's head is `898f68334d757fac1a84e3202ead3f1b9b251ce0`. GitHub reports exactly one workflow run for it: `35496305423` `CI` (`pull_request`) → `startup_failure` (2026-09-20T07:13:46Z); `check-runs` = 0 and the combined status is `pending` with 0 statuses. The repository's workflow fails to start on every branch, including `master`'s own recent runs (the same result appears for this repository's other PRs opened today), so the checks below were executed locally.
```
$ gh api "repos/apache/rocketmq-dashboard/actions/runs?head_sha=898f6833..." --jq '.workflow_runs[] | "\(.id) \(.name) \(.event) \(.conclusion)"'
35496305423 CI pull_request startup_failure
$ gh api "repos/apache/rocketmq-dashboard/commits/898f6833.../check-runs" --jq .total_count
0
```
**Red-light (base source + new test)**
```
$ git checkout origin/master -- web/src/pages/settings/GeneralSettingsTab.tsx # d50ffecc
$ 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)
```
**Module tests (with the fix)**
```
$ 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 + baseline**
```
$ npx vitest run --maxWorkers=4 # branch head
Test Files 123 passed (123)
Tests 1035 passed (1035)
```
`1035 = 1034 (pristine origin/master) + 1 new test` — the same suite reported `1038` earlier in this session while three other in-progress regression files were in the tree, which cross-checks the 1034 baseline. 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); the `--maxWorkers=4` run above is the contention-free result, and none of those files is touched by this PR.
**Build**
```
$ 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 self-check**
```
$ git show --numstat --format="" HEAD
9 2 web/src/pages/settings/GeneralSettingsTab.tsx
110 0 web/src/pages/settings/__tests__/GeneralSettingsFormResidue.test.tsx
```
One commit, two files, no unrelated changes: the source diff adds the `translationRef` declaration plus its sync effect, routes the load-failure message through the ref, and drops `t` from the load effect's dependency list. The ref is written from an effect, so `react-hooks/set-state-in-effect` remains satisfied (eslint reports no findings on the file).
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org