fix(sender): trigger onClose when removing skill via Backspace - #1984
fix(sender): trigger onClose when removing skill via Backspace#1984wenzeyu8888-rgb wants to merge 2 commits into
Conversation
…#1949 Add unit tests verifying streamStatus is isolated per component instance: - Renderer-level: different/same tag names, interleaved, nested, self-closing - Full pipeline: Parser + DOMPurify + html-react-parser - XMarkdown component: streaming loading/done transitions Add demo showing multiple custom components with independent loading states. Closes ant-design#1949
In handleDeleteOperation, the Backspace key deletion branch for skill nodes only called removeSkill() without firing the closable.onClose callback. This was inconsistent with the click-close path in Skill.tsx which fires both removeSkill() and config.onClose?.(event). Fix: fire closable.onClose after removeSkill() in the keyboard deletion branch to match the click-close behavior. Closes ant-design#1955
📝 WalkthroughWalkthrough新增 XMarkdown 多组件流式加载隔离测试与演示文档，确保各组件实例独立维护 ChangesMarkdown 流式加载隔离
Sender Skill 删除回调
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant XMarkdown
participant Parser
participant Renderer
participant CustomComponents
XMarkdown->>Parser: 解析流式 Markdown
Parser->>Renderer: 传递解析后的 HTML
Renderer->>CustomComponents: 为各组件实例传递 streamStatus
CustomComponents-->>XMarkdown: 渲染 loading 或 done 状态
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces loading isolation tests for XMarkdown to ensure stream status is isolated per component instance, adds a multi-component loading demo, and triggers the onClose callback when a skill is removed via Backspace in SlotTextArea. Feedback was provided to improve test robustness by refactoring manual spy cleanup to lifecycle hooks, using safer assertions instead of toBeDefined(), and addressing an unsafe event type cast in SlotTextArea.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| it('should mark only the unclosed instance as loading (different tag names)', () => { | ||
| const components = { 'comp-a': MockComponent, 'comp-b': MockComponent }; | ||
| const renderer = new Renderer({ components }); | ||
| const spy = jest.spyOn(React, 'createElement'); |
There was a problem hiding this comment.
Creating and restoring the spy manually inside each test case can lead to mock leakage. If any assertion (expect) fails, the test execution will halt, and spy.mockRestore() will never be called. This pollutes the global React.createElement for subsequent tests, potentially causing cascading failures.\n\nIt is highly recommended to refactor this by moving the spy setup and teardown to beforeEach and afterEach hooks at the describe block level. This ensures that the mock is always cleaned up properly, even if a test fails, and reduces code duplication across all test cases in this file.
| expect(ref.current).toBeDefined(); | ||
| expect(skillDom).toBeDefined(); |
There was a problem hiding this comment.
Using toBeDefined() to assert the presence of ref.current and skillDom is unsafe because null is considered a defined value in JavaScript/Jest. If ref.current is null or skillDom is not found (returning null), these assertions will still pass.\n\nUse not.toBeNull() or toBeTruthy() to ensure that these values are actually present and not null.
expect(ref.current).not.toBeNull();\n expect(skillDom).not.toBeNull();
| removeSkill(); | ||
| // Fire onClose callback to match click-close behavior (Skill.tsx) | ||
| const closableConfig = typeof skill?.closable === 'boolean' ? {} : skill?.closable; | ||
| closableConfig?.onClose?.(e as unknown as React.MouseEvent<HTMLDivElement>); |
There was a problem hiding this comment.
Casting a KeyboardEvent or ClipboardEvent directly to a MouseEvent (e as unknown as React.MouseEvent<HTMLDivElement>) is unsafe. If the consumer's onClose callback accesses mouse-specific properties (such as clientX, clientY, or button), it will result in undefined values or potential runtime errors.\n\nConsider typing the event parameter in onClose more broadly (e.g., allowing React.SyntheticEvent or union types) if possible, or document this behavior clearly so consumers know that onClose can be triggered by keyboard events with a different event payload.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/x/components/sender/components/SlotTextArea.tsx`:
- Around line 526-530: 在 Backspace 删除处理逻辑中，基于 closableConfig.disabled 检查 skill
是否禁用；禁用时阻止默认行为并立即返回，不执行 removeSkill() 或 onClose?.( )，同时保留启用状态下现有的删除与回调行为。
In `@packages/x/docs/x-markdown/demo/streaming/multi-component-loading.tsx`:
- Line 77: Update the Component B info-panel text to say “loads independently”
instead of “downloads independently,” preserving the rest of the message and the
streaming demo behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1f9b807-b6a1-4dd8-b58c-84406392f56e
📒 Files selected for processing (6)
packages/x-markdown/src/XMarkdown/__tests__/loading-isolation.test.tsxpackages/x/components/sender/__tests__/slot.test.tsxpackages/x/components/sender/components/SlotTextArea.tsxpackages/x/docs/x-markdown/demo/streaming/multi-component-loading.tsxpackages/x/docs/x-markdown/streaming.en-US.mdpackages/x/docs/x-markdown/streaming.zh-CN.md
| e.preventDefault(); | ||
| removeSkill(); | ||
| // Fire onClose callback to match click-close behavior (Skill.tsx) | ||
| const closableConfig = typeof skill?.closable === 'boolean' ? {} : skill?.closable; | ||
| closableConfig?.onClose?.(e as unknown as React.MouseEvent<HTMLDivElement>); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Backspace 删除前需要遵守 closable.disabled
点击关闭路径在 Skill.tsx 中会阻止禁用 skill 的删除，但这里无条件执行 removeSkill() 和 onClose。因此 closable={{ disabled: true }} 的 skill 仍可通过 Backspace 被删除。
建议在删除前读取 closableConfig.disabled，禁用时阻止默认行为并直接返回。
建议修复
if (skillKey) {
e.preventDefault();
- removeSkill();
const closableConfig = typeof skill?.closable === 'boolean' ? {} : skill?.closable;
+ if (closableConfig?.disabled) {
+ return true;
+ }
+ removeSkill();
closableConfig?.onClose?.(e as unknown as React.MouseEvent<HTMLDivElement>);
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| e.preventDefault(); | |
| removeSkill(); | |
| // Fire onClose callback to match click-close behavior (Skill.tsx) | |
| const closableConfig = typeof skill?.closable === 'boolean' ? {} : skill?.closable; | |
| closableConfig?.onClose?.(e as unknown as React.MouseEvent<HTMLDivElement>); | |
| e.preventDefault(); | |
| const closableConfig = typeof skill?.closable === 'boolean' ? {} : skill?.closable; | |
| if (closableConfig?.disabled) { | |
| return true; | |
| } | |
| removeSkill(); | |
| // Fire onClose callback to match click-close behavior (Skill.tsx) | |
| closableConfig?.onClose?.(e as unknown as React.MouseEvent<HTMLDivElement>); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/x/components/sender/components/SlotTextArea.tsx` around lines 526 -
530, 在 Backspace 删除处理逻辑中，基于 closableConfig.disabled 检查 skill
是否禁用；禁用时阻止默认行为并立即返回，不执行 removeSkill() 或 onClose?.( )，同时保留启用状态下现有的删除与回调行为。
|
|
||
| Some text between components. | ||
|
|
||
| <info-panel>Component B: This info panel downloads independently.</info-panel> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
将 downloads independently 修正为 loads independently。
当前文案误指“独立下载”，与组件独立加载状态的演示含义不符。
建议修改
-<info-panel>Component B: This info panel downloads independently.</info-panel>
+<info-panel>Component B: This info panel loads independently.</info-panel>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <info-panel>Component B: This info panel downloads independently.</info-panel> | |
| <info-panel>Component B: This info panel loads independently.</info-panel> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/x/docs/x-markdown/demo/streaming/multi-component-loading.tsx` at
line 77, Update the Component B info-panel text to say “loads independently”
instead of “downloads independently,” preserving the rest of the message and the
streaming demo behavior.
kimteayon
left a comment
kimteayon
left a comment
There was a problem hiding this comment.
审核结论
不建议当前状态合并，有两个需要修复的行为问题：
-
[P1] Backspace 未遵守
closable.disabled
[SlotTextArea.tsx:525](https://github.com/ant-design/x/pull/1984/files/794a77ba745dace4ec9e722e4435e713bfab93f9#diff-1227930f6b2e9e6ced1d046c7de3ce83cb7fd0e546d8499ef112edce86b97d57R525) 无条件调用removeSkill()和onClose，而点击关闭路径会在disabled时直接返回。
我补充临时回归用例验证：设置closable={{ disabled: true, onClose }}后按 Backspace，回调仍被调用 1 次。应在删除前检查disabled，同时阻止浏览器默认删除行为。 -
[P2]
onClose的事件类型契约被破坏
[SlotTextArea.tsx:530](https://github.com/ant-design/x/pull/1984/files/794a77ba745dace4ec9e722e4435e713bfab93f9#diff-1227930f6b2e9e6ced1d046c7de3ce83cb7fd0e546d8499ef112edce86b97d57R530) 把KeyboardEvent强制转换成MouseEvent，但公开 API 仍声明为React.MouseEventHandler。实际回调收到的是type: "keydown"，不存在可靠的button、clientX等鼠标属性。应把接口、Skill.tsx和文档统一调整为React.SyntheticEvent<HTMLDivElement>，或显式定义鼠标/键盘事件联合类型。
链接中的测试文件还有两个非阻塞问题：
- [P3] Spy 清理可能泄漏：[loading-isolation.test.tsx:48](https://github.com/ant-design/x/pull/1984/files/794a77ba745dace4ec9e722e4435e713bfab93f9#diff-88080e70d0b3fc0a75077a92cd61b3667e3502a268e23bc1a75dac84a62172c7R48) 在断言之后手动
mockRestore()；断言失败时会污染后续用例。建议统一在afterEach(() => jest.restoreAllMocks())清理。 - 9 个 Renderer 层用例大部分已由 #1590 加入现有 Renderer.test.ts。建议只保留真正新增的组件级
rerender覆盖，并将 #1949 的测试/演示从 Sender 修复 PR 中拆出。
验证结果：PR head 的 loading isolation 测试 13/13 通过，TypeScript 和 Biome 通过；disabled 回归用例稳定失败。
2 participants
📝 Problem
Closes #1955
Related: #1938
In the Sender component, when removing an inserted skill tag via the Backspace key, the
closable.onClosecallback was not fired. External code could not detect skill removal via keyboard.🔍 Root Cause
In
SlotTextArea.tsx, thehandleDeleteOperationfunction handles Backspace deletion. TheskillKeybranch only calledremoveSkill()without firingclosable.onClose, which was inconsistent with the click-close path inSkill.tsxthat fires bothremoveSkill()andconfig.onClose?.(event).✅ Fix
Fire
closable.onCloseafterremoveSkill()in the keyboard deletion branch, matching the click-close behavior.🧪 Test
Added a unit test that:
closable.onClosecallbackonCloseis called exactly onceAll existing tests continue to pass.
Summary by CodeRabbit
新功能
问题修复
文档