lizhimins commented on PR #6134:
URL:
https://github.com/apache/rocketmq-dashboard/pull/6134#issuecomment-6120397343
Thanks — of the three re-entrancy PRs in this series this one has the
strongest test: two `fireEvent.click` calls inside a single tick is exactly
what pins the ref, and the comment explaining why `submitting` alone is not
enough is appreciated. The guard itself is correct.
We are closing rather than requesting changes, because what is left is not a
tweak to this diff.
1. The same window is still open next door. `handleTest`
(`web/src/pages/ops/alerts.tsx:839`) awaits `form.validateFields()` at `:841`
and only sets `setTesting(true)` at `:846`, so the "test rule" button keeps the
identical duplicate-request shape you just fixed — and a rule test is not a
read-only operation. Landing the save path alone leaves the file half-guarded,
which is worse than either extreme, because the next reader will assume the
pattern is applied consistently.
2. The mid-flight cancel assertion passes on the unpatched tip. This Modal
has no `destroyOnClose`, so under antd v5 the `role="dialog"` node stays in the
document after close (only the wrapper gets `display: none`), and
`toBeInTheDocument()` merely checks `document.contains(node)`.
`expect(dialog).toBeVisible()`, or re-querying with
`screen.queryByRole('dialog')`, would actually pin it. The
`toHaveBeenCalledTimes(1)` outside the `waitFor` is also redundant with the one
inside.
So the change we want is one that covers every await-then-mutate path in
`alerts.tsx` together, with an assertion that fails without the lock. If you
would like to take that on we will review it promptly — it is a small step from
what you already have.
Two notes for whoever picks this up. The duplicate *create* cannot land two
rows: the server has `uk_alert_rule_semantic_fingerprint`
(`AlertSchemaMigration.java:134`) plus the semantic dedup at
`AlertService.java:237-247`, so the user-visible win here is avoiding the
contradictory "created" + "save failed" toast pair and, more importantly,
stopping the dialog from being torn down mid-save. And with
`closable/keyboard/maskClosable={!submitting}` the dialog cannot be dismissed
at all if the save request hangs, so it relies on the HTTP timeout to unlock.
--
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]
