lizhimins commented on PR #5800:
URL:
https://github.com/apache/rocketmq-dashboard/pull/5800#issuecomment-6093317276
> We independently re-verified the report against current trunk rather than
taking #4736 at face value: `ToolPlaygroundModal.tsx` still does
`setToolResult(await executeTool(...))` with no generation or ownership check,
and `selectTool` only resets `toolResult` to `undefined` — it cannot cancel an
in-flight promise. So a slow result for tool A does land on tool B's panel (or
on another instance, or on edited arguments, or on a closed modal) together
with a success toast. The `executionRequestRef` + `invalidateExecution` shape
is the right fix, all four context dimensions are hooked, and the success
toast, the error toast and the loading flag are all inside the same guard. > >
The six deferred cases are mutation-sensitive for the right reasons: they
resolve the pending `executeTool` promise *before* asserting that the result
panel and the toasts stayed away, so reverting the `requestId` comparison turns
them red. Reusing `renderPage({ toolsIntent: 'open' })` and `within(dialog)`
from `AiPage.test.tsx` instead of standing up a second harness is also
appreciated — there is no `ToolPlaygroundModal.test.tsx` on trunk today. > >
Two things before we merge. First, a governance point rather than a technical
one: #4736 was closed as `not_planned` by its own reporter on 2026-09-29 and
#4738 was closed unmerged seconds earlier, so this is a third-party revival of
a withdrawn report. We need to decide that explicitly; if we accept it we will
say so on #4736 with the trunk evidence above. Second, the new cases add a
file-level `afterEach(() => vi.restoreAllMocks())` and an `antd`
`App`/`message` import to `AiPage.test.tsx`, which now applies to the 15
pre-existing cases in that file. `restoreAllMocks` only touches `vi.spyOn`
spies and not `vi.mock` module stubs, so the risk is low, but please scope that
`afterEach` to your new `describe` block so the existing cases keep their
current teardown.
---
**Decision**: we are not landing this one. The reporter withdrew #4736 as
`not_planned` and the original #4738 was never merged, so we are respecting
that withdrawal - even though, as noted above, we re-verified the defect and it
does still exist on trunk. If the community still wants it fixed, please reopen
#4736 (or file a fresh issue) and we will pick it up there. Closing.
--
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]
