messere1 commented on PR #6454:
URL:
https://github.com/apache/rocketmq-dashboard/pull/6454#issuecomment-6196406475
Thanks @lizhimins for the review and for confirming the core problem and the
#6543 false positive. All three items are done in e2d3464b (also rebased onto
the current tip a92c2314 — the branch had drifted ~100 commits behind; the
tip's own alerts.tsx/test changes merge cleanly and are covered by the run
below):
1. **`handleBulkDelete` race** — the `onOk` now re-checks
`actionInFlightRef.current` and returns *before* acquiring, exactly as
prescribed (kept the cheap outer guard for the common path). I kept the shared
lock rather than a bulk-only ref: the row and bulk actions already share
`setActionId`/`setBulkAction` rendered state, so letting them run concurrently
would reintroduce the same interleaving one level up. The new test reproduces
your scenario end to end: select → bulk delete → toggle a row while the confirm
is up → press OK → the bulk delete stands down (`bulkDeleteAlertRules` not
called); after the toggle settles, a re-confirmed bulk delete goes through.
2. **Split out the modal-teardown change** — `closeRuleModal`, the shared
cancel handler, and `closable/keyboard/maskClosable={!submitting}` are all
removed from this PR; the dialog teardown is back to the base behavior. The
mid-save teardown guard will come back as its own follow-up once this lands,
built the way you sketched: the three props reading the same ref as the close
guard, plus a visible "saving, dialog locked (up to 30s)" affordance instead of
a swallowed click.
3. **Lock-release coverage** —
- Save path: the first test now resolves the first update, then re-opens
the same rule and saves again, asserting `updateAlertRule` is called twice (the
second save would be silently swallowed if the `finally` never released the
ref).
- Bulk path: the new test's later phases re-confirm a bulk delete after
the toggle resolves and bulk-delete a second selection after the first one
succeeds.
- Both were mutation-verified: commenting out `submitInFlightRef.current
= false` fails the save test's second-save phase, and commenting out the `onOk`
re-check fails the bulk test's stand-down assertion.
Two jsdom findings baked into the tests, for the record: antd's loading-icon
leave motion never completes in jsdom, so after any loading phase the bulk
button's accessible name permanently gains a "loading" prefix — the later
lookups use a regex for that button; and the re-selection waits for the toolbar
to settle (checkbox re-enabled) before driving the next confirmation.
Evidence: full `AlertsPage.test.tsx` **39/39** green on the rebased head (35
tests from the current tip + the three from this PR + the new bulk race test);
eslint 0 errors (the 5 warnings are pre-existing on untouched lines); Prettier
clean; `tsc -b` clean.
--
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]
