fix: Sender skill closable.onClose not triggered when deleting skill via Backspace#1964
fix: Sender skill closable.onClose not triggered when deleting skill via Backspace#1964nikzart wants to merge 2 commits into
Conversation
…via Backspace Fire closable.onClose in the keyboard deletion branch of handleDeleteOperation, matching the close-button click behavior, so external code can observe skill removal via Backspace. close ant-design#1955, close ant-design#1938
dosubot
Bot
added
bug
📝 WalkthroughWalkthrough更新了 Sender 的 skill 关闭契约与删除流程：Backspace 删除 skill 时会补发 ChangesBackspace 删除 skill 触发 onClose
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 support for triggering the closable.onClose callback when a skill is removed via the Backspace key in the SlotTextArea component, and adds a corresponding unit test. The review feedback points out a type-safety issue where a KeyboardEvent is cast directly to a React.MouseEvent, which could cause runtime errors if mouse-specific properties are accessed. Additionally, it suggests strengthening the unit test assertions by replacing weak toBeDefined() checks with not.toBeNull() and toBeInTheDocument().
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.
| expect(ref.current).toBeDefined(); | ||
| expect(skillDom).toBeDefined(); |
There was a problem hiding this comment.
Using toBeDefined() on ref.current and skillDom is a weak assertion because null is considered defined in Jest. Since querySelector returns null when an element is not found, expect(skillDom).toBeDefined() will pass even if the element does not exist.
We should use not.toBeNull() for the ref and toBeInTheDocument() for the DOM element to ensure they actually exist and are rendered correctly.
| expect(ref.current).toBeDefined(); | |
| expect(skillDom).toBeDefined(); | |
| expect(ref.current).not.toBeNull(); | |
| expect(skillDom).toBeInTheDocument(); |
There was a problem hiding this comment.
Fixed in 523a199 — not.toBeNull() for the ref and toBeInTheDocument() for the skill element.
| removeSkill(); | ||
| // 与点击关闭行为保持一致，删除后补发 closable.onClose | ||
| if (skill?.closable && typeof skill.closable === 'object') { | ||
| skill.closable.onClose?.(e as unknown as React.MouseEvent<HTMLDivElement>); |
There was a problem hiding this comment.
Casting a KeyboardEvent (e) directly to React.MouseEvent is type-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.
Since onClose is defined as a MouseEventHandler, we should consider if we can avoid passing the keyboard event directly, or at least document this behavior. If possible, we could also update the onClose type definition in interface.ts to accept React.MouseEvent | React.KeyboardEvent in a future PR.
There was a problem hiding this comment.
Addressed in 523a199 — took the wider-type route: closable.onClose is now typed (event: React.SyntheticEvent<HTMLDivElement>) => void (interface, local ClosableConfig, and both docs), and the keyboard path passes the real event with no cast.
![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/__tests__/slot.test.tsx`:
- Line 544: The current assertion in the sender slot test uses toBeDefined() on
the result of container.querySelector, which can still pass when the selector
returns null and does not prove the .ant-sender-skill element rendered. Update
the assertion in slot.test.tsx to use a null-safe presence check such as
not.toBeNull(), or match the existing jest-dom style used elsewhere in the same
test file by asserting with toBeInTheDocument() on the skillDom query result.
In `@packages/x/components/sender/components/SlotTextArea.tsx`:
- Around line 525-533: The Backspace delete path in SlotTextArea is forcing a
KeyboardEvent into the closable.onClose callback as a MouseEvent, which does not
match the public SkillType.closable.onClose contract. Update the onClose
signature to accept a more general React.SyntheticEvent<HTMLDivElement> (or
another shared event type) and make the SlotTextArea delete/close flow pass the
real event type through without casting, so both keyboard and mouse close paths
stay aligned.
🪄 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: 0c4a48ae-97f0-4397-8c59-a8ecefdc8195
📒 Files selected for processing (2)
packages/x/components/sender/__tests__/slot.test.tsxpackages/x/components/sender/components/SlotTextArea.tsx
… removal Address review feedback: - Widen closable.onClose (interface, ClosableConfig, docs) from React.MouseEventHandler<HTMLDivElement> to (event: React.SyntheticEvent<HTMLDivElement>) => void, since the callback now also receives the keyboard event on Backspace removal. Removes the KeyboardEvent-to-MouseEvent cast. - Strengthen test assertions (not.toBeNull / toBeInTheDocument).
Bundle ReportChanges will increase total bundle size by 635.41kB (18.29%) ⬆️
Affected Assets, Files, and Routes:view changes for bundle: antdx-array-pushAssets Changed:
view changes for bundle: x-markdown-array-pushAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1964 +/- ##
=======================================
Coverage 97.07% 97.07%
=======================================
Files 159 159
Lines 5748 5750 +2
Branches 1687 1686 -1
=======================================
+ Hits 5580 5582 +2
Misses 166 166
Partials 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
🤔 This is a ...
🔗 Related Issues
close #1955, close #1938
💡 Background and Solution
Deleting an inserted skill tag with Backspace removed the tag but never fired
closable.onClose, so external code could not observe the removal. The close-button click path already fires it (Skill.tsx→handleClose).This PR fires
closable.onClosein theskillKeybranch ofhandleDeleteOperationinSlotTextArea.tsx, right afterremoveSkill(), matching the click path's order (remove, then notify).Notes:
closable.onCloseis now typed(event: React.SyntheticEvent<HTMLDivElement>) => void(previouslyReact.MouseEventHandler<HTMLDivElement>), since the callback now also receives the keyboard event on Backspace removal — no event cast needed. Docs updated in both languages.closable.disabled, as before this PR. If Backspace deletion should also be blocked fordisabled, I'm happy to follow up — kept out of scope here to stay minimal.📝 Change Log
Senderskillclosable.onClosewas not triggered when deleting the skill tag with Backspace. TheonCloseevent parameter is now typed asReact.SyntheticEvent<HTMLDivElement>.Sender使用 Backspace 删除 skill 标签时不触发closable.onClose回调的问题。onClose的事件参数类型调整为React.SyntheticEvent<HTMLDivElement>。Summary by CodeRabbit
onClose回调说明与事件类型，明确其在“点击关闭”及“Backspace 删除”时触发。