feat(a11y): enhance keyboard interaction in search mode #592
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough此拉取请求对 Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/OptionList.tsx (3)
162-175: 代码实现清晰，建议加强类型安全性递归查找第一个匹配且未禁用的节点的实现逻辑正确。建议对
fieldNames.children的访问添加类型检查，以增强代码的健壮性。建议如下修改：
- if (node[fieldNames.children]) { + const children = node[fieldNames.children]; + if (Array.isArray(children) && children.length > 0) {
193-204: 建议提升代码可读性当前实现正确地解决了搜索模式下的键盘交互问题，但可以通过提取条件判断来提高代码的可维护性。
建议重构如下：
if (searchValue) { const firstMatchNode = getFirstMatchNode(memoTreeData); - if (firstMatchNode) { - if (firstMatchNode.selectable !== false) { - onInternalSelect(null, { - node: { key: firstMatchNode[fieldNames.value] }, - selected: !checkedKeys.includes(firstMatchNode[fieldNames.value]), - }); - } - } + const canSelectNode = firstMatchNode && firstMatchNode.selectable !== false; + if (canSelectNode) { + const nodeValue = firstMatchNode[fieldNames.value]; + onInternalSelect(null, { + node: { key: nodeValue }, + selected: !checkedKeys.includes(nodeValue), + }); + }
226-229: 建议优化 useMemo 的比较逻辑当前的实现在搜索时正确地禁用了数据加载，但比较函数的逻辑可以更清晰。
建议修改如下：
- ([preSearchValue], [nextSearchValue, nextExcludeSearchExpandedKeys]) => - preSearchValue !== nextSearchValue && !!(nextSearchValue || nextExcludeSearchExpandedKeys), + ([prevSearchValue], [nextSearchValue, expandedKeys]) => + prevSearchValue !== nextSearchValue || (!nextSearchValue && !!expandedKeys),tests/Select.SearchInput.spec.js (1)
204-230: 测试用例设计合理，建议补充一个场景！测试用例很好地覆盖了以下场景：
- 按回车键选择第一个匹配的非禁用节点
- 验证禁用节点不会被选中
建议补充测试：当存在多个匹配节点时，验证是否始终选择第一个非禁用节点。
// 建议添加以下测试场景 wrapper.search('2'); // 假设存在多个标签包含 '2' wrapper.find('input').first().simulate('keyDown', { which: KeyCode.ENTER }); expect(onSelect).toHaveBeenCalledWith('第一个非禁用的2', expect.anything());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/OptionList.tsx(3 hunks)tests/Select.SearchInput.spec.js(2 hunks)
🔇 Additional comments (4)
src/OptionList.tsx (1)
193-204: 请确认测试覆盖率
实现已经很好地解决了键盘交互的问题，建议确认测试用例是否覆盖了以下场景：
- 搜索时选择第一个匹配节点
- 处理禁用节点的情况
- 无匹配结果的情况
✅ Verification successful
Let me gather more information about the test coverage.
测试覆盖情况已完全满足要求
测试用例已经全面覆盖了所有关键场景：
- 搜索并选择第一个匹配节点的功能
- 处理禁用节点的情况
- 无搜索值时的处理
- 无匹配结果的处理
- 所有匹配节点都被禁用的情况
测试实现严谨完整，无需额外补充测试用例。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查测试文件中的相关测试用例
rg -A 5 "describe.*keyboard.*search" tests/
rg -A 5 "it.*press.*enter" tests/
Length of output: 898
Script:
#!/bin/bash
# Check test implementation details
cat tests/Select.SearchInput.spec.js
Length of output: 8752
tests/Select.SearchInput.spec.js (3)
5-5: 导入键码常量模块符合最佳实践！
使用 rc-util/lib/KeyCode 模块来处理键盘事件是一个很好的选择，它提供了统一且可维护的键码常量。
232-274: 测试用例完整覆盖了无选择场景！
这两个测试用例很好地验证了以下关键场景：
- 无搜索值时按回车键不应选择任何节点
- 搜索无匹配结果时按回车键不应选择任何节点
测试结构清晰，断言准确。
276-294: 测试用例设计合理，建议验证实现代码！
测试用例很好地覆盖了所有匹配节点都被禁用的场景。为确保实现的完整性，建议验证相关代码。
✅ Verification successful
实现代码已正确处理禁用节点场景，测试用例验证有效！
从源码分析可以看到：
getFirstMatchNode函数在src/OptionList.tsx中明确检查了disabled状态- 搜索匹配时会跳过禁用的节点 (
if (filterTreeNode(node) && !node.disabled)) - 实现逻辑与测试用例的预期行为完全一致
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：验证 OptionList 组件中处理禁用节点的逻辑实现
# 测试：搜索与禁用节点相关的实现代码
rg -A 5 "disabled.*getFirstMatchNode|getFirstMatchNode.*disabled"
Length of output: 67
Script:
#!/bin/bash
# 搜索处理选择和禁用节点的相关代码
rg -A 10 "onSelect.*disabled|disabled.*select" src/
# 搜索处理按键事件的相关代码
ast-grep --pattern 'function $FUNC($_) {
$$$
disabled
$$$
}'
# 搜索处理搜索和匹配节点的代码
rg -A 10 "searchValue.*match|getFirstMatch" src/
Length of output: 1688
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/Select.SearchInput.spec.js (2)
203-230: 键盘事件测试用例设计合理测试用例很好地覆盖了以下场景：
- 按回车键选择第一个匹配的非禁用节点
- 禁用节点不会被选中
建议添加以下场景的测试：
- 多层级树结构中的节点选择
- 异步加载数据时的节点选择
296-317: 搜索时激活首个匹配节点的测试设计合理测试用例很好地验证了：
- 搜索时激活第一个匹配的非禁用节点
- 跳过禁用的节点
建议补充一个测试场景：
- 验证在搜索过程中，当搜索词变化时，激活状态是否正确更新
// 建议添加的测试用例 it('should update active node when search value changes', () => { const wrapper = mount( <TreeSelect showSearch open treeData={[ { value: '1', label: '1' }, { value: '2', label: '2' }, { value: '3', label: '3' }, ]} />, ); // 首次搜索 wrapper.search('1'); expect(wrapper.find('.rc-tree-select-tree-treenode-active').text()).toBe('1'); // 更新搜索词 wrapper.search('2'); expect(wrapper.find('.rc-tree-select-tree-treenode-active').text()).toBe('2'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
tests/__snapshots__/Select.checkable.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Select.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/OptionList.tsx(2 hunks)tests/Select.SearchInput.spec.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OptionList.tsx
🔇 Additional comments (4)
tests/Select.SearchInput.spec.js (4)
5-5: 导入 KeyCode 模块用于键盘事件测试
从 rc-util 库导入 KeyCode 模块来模拟键盘事件，这是一个合理的做法。
232-254: 无搜索值时的回车键处理测试完善
测试用例验证了两个重要场景：
- 无搜索值时按回车键不选择节点
- 有搜索值时按回车键选择第一个匹配节点
逻辑严谨，测试覆盖充分。
256-274: 未找到匹配节点时的处理测试完善
测试用例验证了搜索不存在的值时按回车键不会选择任何节点，这对于提升用户体验很重要。
276-294: 所有匹配节点被禁用时的处理测试完善
测试用例验证了当所有匹配的节点都被禁用时，按回车键不会选择任何节点，这种边界情况的处理很重要。
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/OptionList.tsx (3)
95-109: 实现正确，建议小幅优化递归查找第一个可选节点的实现逻辑正确。不过可以通过以下方式优化性能：
- 使用
Array.prototype.find来简化代码- 添加类型注解提升代码可维护性
建议如下优化：
-const getFirstSelectableNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => { +const getFirstSelectableNode = (nodes: EventDataNode<any>[]): EventDataNode<any> | null => { - for (const node of nodes) { - if (node.selectable !== false) { - return node; - } - if (node[fieldNames.children]) { - const selectableInChildren = getFirstSelectableNode(node[fieldNames.children]); - if (selectableInChildren) { - return selectableInChildren; - } - } - } - return null; + return nodes.find(node => + node.selectable !== false || + (node[fieldNames.children] && getFirstSelectableNode(node[fieldNames.children])) + ) || null; };
179-192: 搜索匹配逻辑正确，建议优化实现递归查找第一个匹配且未禁用的节点的逻辑正确，但可以参考
getFirstSelectableNode的优化方案进行改进。建议如下优化：
-const getFirstMatchNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => { +const getFirstMatchNode = (nodes: EventDataNode<any>[]): EventDataNode<any> | null => { - for (const node of nodes) { - if (filterTreeNode(node) && !node.disabled) { - return node; - } - if (node[fieldNames.children]) { - const matchInChildren = getFirstMatchNode(node[fieldNames.children]); - if (matchInChildren) { - return matchInChildren; - } - } - } - return null; + return nodes.find(node => + (filterTreeNode(node) && !node.disabled) || + (node[fieldNames.children] && getFirstMatchNode(node[fieldNames.children])) + ) || null; };
247-250: 建议优化 loadDataFun 的依赖判断逻辑当前的比较函数逻辑较难理解，建议拆分条件使代码更清晰。
建议如下优化：
const loadDataFun = useMemo( () => (searchValue ? null : (loadData as any)), [searchValue, treeExpandedKeys || expandedKeys], - ([preSearchValue], [nextSearchValue, nextExcludeSearchExpandedKeys]) => - preSearchValue !== nextSearchValue && !!(nextSearchValue || nextExcludeSearchExpandedKeys), + ([prevSearchValue], [nextSearchValue, nextExpandedKeys]) => { + const hasSearchValueChanged = prevSearchValue !== nextSearchValue; + const shouldUpdateOnExpandedKeys = !nextSearchValue && !!nextExpandedKeys; + return hasSearchValueChanged || shouldUpdateOnExpandedKeys; + }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
tests/__snapshots__/Select.checkable.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Select.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/OptionList.tsx(4 hunks)tests/Select.SearchInput.spec.js(2 hunks)tests/Select.spec.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Select.SearchInput.spec.js
🔇 Additional comments (6)
src/OptionList.tsx (4)
113-124: 下拉框打开时的焦点处理逻辑合理
当下拉框打开时：
- 单选模式下，正确滚动到已选中项
- 多选模式下，正确激活第一个可选项
- 使用
getFirstSelectableNode确保激活项是可选的
134-148: 选择处理逻辑完善且符合无障碍标准
- 正确处理了可选和禁用状态
- 单选模式下自动关闭下拉框
- 支持多选模式
194-207: 搜索时的状态处理逻辑完善
- 搜索时自动展开所有节点，提升用户体验
- 正确设置第一个匹配项为活动项
- 无匹配项时正确清空活动项
226-227: 键盘交互逻辑符合无障碍要求
回车键处理逻辑正确：
- 检查节点是否可选
- 检查节点是否禁用
- 仅在节点可用时触发选择
tests/Select.spec.tsx (2)
441-447: 代码逻辑正确
在键盘交互测试中，matchValue 函数正确地验证了选中的值，符合预期的选中顺序。
470-475: 验证不可选择的节点不会触发 onChange 事件
测试成功验证了当尝试通过键盘操作选择不可选择的节点时，onChange 事件不会被触发。而当选择可选择的父节点时，onChange 正确地被调用。
tests/Select.spec.tsx
Outdated
| // it('should not add new tag when key enter is pressed if nothing is active', () => { | ||
| // const onSelect = jest.fn(); | ||
|
|
||
| // const wrapper = mount( | ||
| // <TreeSelect open treeDefaultExpandAll multiple onSelect={onSelect}> | ||
| // <TreeNode value="parent 1-0" title="parent 1-0"> | ||
| // <TreeNode value="leaf1" title="my leaf" disabled /> | ||
| // <TreeNode value="leaf2" title="your leaf" disabled /> | ||
| // </TreeNode> | ||
| // </TreeSelect>, | ||
| // ); | ||
|
|
||
| // wrapper.find('input').first().simulate('keydown', { which: KeyCode.ENTER }); | ||
| // expect(onSelect).not.toHaveBeenCalled(); | ||
| // }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
避免注释掉测试用例
发现测试用例 should not add new tag when key enter is pressed if nothing is active 被注释掉了。这可能会导致相关功能缺乏测试覆盖，潜在的问题无法被及时发现。
建议重新启用并完善该测试用例，或者如果该测试不再适用，提供详细的注释说明原因，并考虑安全地删除此冗余代码。
tests/Select.spec.tsx
Outdated
|
|
||
| it('should active first option when dropdown is opened', () => { | ||
| const treeData = [ | ||
| { key: '0', value: '0', title: '0 label', disabled: true }, | ||
| { key: '1', value: '1', title: '1 label' }, | ||
| { key: '2', value: '2', title: '2 label' }, | ||
| ]; | ||
|
|
||
| const wrapper = mount(<TreeSelect treeData={treeData} />); | ||
|
|
||
| expect(wrapper.find('.rc-tree-select-tree-treenode-active')).toHaveLength(0); | ||
|
|
||
| wrapper.openSelect(); | ||
|
|
||
| const activeNode = wrapper.find('.rc-tree-select-tree-treenode-active'); | ||
| expect(activeNode).toHaveLength(1); | ||
| expect(activeNode.text()).toBe('0 label'); | ||
| }); |
There was a problem hiding this comment.
默认激活的节点是禁用的节点
在测试 should active first option when dropdown is opened 中，下拉菜单打开时默认激活了第一个禁用的节点 '0 label'。根据用户体验和可访问性的最佳实践，应该跳过禁用的节点，激活第一个可选择的未禁用节点 '1 label'。
建议修改测试用例，使其在打开下拉菜单时，默认激活第一个未禁用的节点。可以修改如下代码：
- expect(activeNode.text()).toBe('0 label');
+ expect(activeNode.text()).toBe('1 label');同时，需要检查组件逻辑，确保在实际使用中，下拉菜单打开时不会默认聚焦到禁用的节点。
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/OptionList.tsx (4)
244-247: 优化 loadDataFun 的依赖项
useMemo的依赖数组中使用treeExpandedKeys || expandedKeys可能会导致不必要的重新计算。建议将条件判断移到回调函数中。const loadDataFun = useMemo( () => (searchValue ? null : (loadData as any)), - [searchValue, treeExpandedKeys || expandedKeys], + [searchValue, treeExpandedKeys, expandedKeys], ([preSearchValue], [nextSearchValue, nextExpandedKeys]) => preSearchValue !== nextSearchValue && !!(nextSearchValue || nextExpandedKeys), );
184-204: 优化 Active Key Effect 的性能当前实现在每次
open或searchValue变化时都会重新计算firstSelectableNode。建议将getFirstSelectableNode的结果进行缓存。+ const firstSelectableNode = React.useMemo( + () => getFirstSelectableNode(memoTreeData), + [memoTreeData] + ); React.useEffect(() => { if (searchValue) { const firstMatchNode = getFirstMatchNode(memoTreeData); setActiveKey(firstMatchNode ? firstMatchNode[fieldNames.value] : null); return; } if (open) { if (!multiple && checkedKeys.length) { setActiveKey(checkedKeys[0]); } else { - const firstSelectableNode = getFirstSelectableNode(memoTreeData); if (firstSelectableNode) { setActiveKey(firstSelectableNode[fieldNames.value]); } } return; } setActiveKey(null); }, [open, searchValue]);
223-224: 优化 ENTER 键处理逻辑的可读性建议将选择逻辑抽取为一个独立的函数，使代码更易理解和维护。
+ const isNodeSelectable = (node: EventDataNode<any>) => + node?.selectable !== false && !node?.disabled; case KeyCode.ENTER: { if (activeEntity) { - const { selectable, value, disabled } = activeEntity?.node || {}; - if (selectable !== false && !disabled) { + if (isNodeSelectable(activeEntity.node)) { onInternalSelect(null, { node: { key: activeKey }, - selected: !checkedKeys.includes(value), + selected: !checkedKeys.includes(activeEntity.node.value), }); } } break; }
152-157: 优化搜索时的展开逻辑当前实现在搜索时展开所有节点，这对于大型树结构可能会影响性能。建议只展开包含匹配节点的路径。
React.useEffect(() => { if (searchValue) { - setSearchExpandedKeys(getAllKeys(treeData, fieldNames)); + // 建议实现一个函数，只返回包含匹配节点的路径的键 + setSearchExpandedKeys(getMatchedNodePaths(treeData, fieldNames, searchValue)); } }, [searchValue]);您需要我帮助实现
getMatchedNodePaths函数吗？
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/OptionList.tsx(5 hunks)tests/Select.spec.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Select.spec.tsx
src/OptionList.tsx
Outdated
| // ========================== Get First Selectable Node ========================== | ||
| const getFirstMatchingNode = ( | ||
| nodes: EventDataNode<any>, | ||
| predicate: (node: EventDataNode<any>) => boolean, | ||
| ): EventDataNode<any> | null => { | ||
| for (const node of nodes) { | ||
| if (predicate(node)) { | ||
| return node; | ||
| } | ||
| if (node[fieldNames.children]) { | ||
| const matchInChildren = getFirstMatchingNode(node[fieldNames.children], predicate); | ||
| if (matchInChildren) { | ||
| return matchInChildren; | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| }; | ||
|
|
||
| const onInternalSelect = (__: Key[], info: TreeEventInfo) => { | ||
| const { node } = info; | ||
| const getFirstSelectableNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => | ||
| getFirstMatchingNode(nodes, node => node.selectable !== false && !node.disabled); | ||
|
|
||
| if (checkable && isCheckDisabled(node)) { | ||
| const getFirstMatchNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => | ||
| getFirstMatchingNode(nodes, node => filterTreeNode(node) && !node.disabled); | ||
|
|
There was a problem hiding this comment.
修复 getFirstMatchingNode 的类型定义
nodes 参数应该是数组类型，但当前的类型定义是单个节点。
const getFirstMatchingNode = (
- nodes: EventDataNode<any>,
+ nodes: EventDataNode<any>[],
predicate: (node: EventDataNode<any>) => boolean,
): EventDataNode<any> | null => {📝 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.
| // ========================== Get First Selectable Node ========================== | |
| const getFirstMatchingNode = ( | |
| nodes: EventDataNode<any>, | |
| predicate: (node: EventDataNode<any>) => boolean, | |
| ): EventDataNode<any> | null => { | |
| for (const node of nodes) { | |
| if (predicate(node)) { | |
| return node; | |
| } | |
| if (node[fieldNames.children]) { | |
| const matchInChildren = getFirstMatchingNode(node[fieldNames.children], predicate); | |
| if (matchInChildren) { | |
| return matchInChildren; | |
| } | |
| } | |
| } | |
| return null; | |
| }; | |
| const onInternalSelect = (__: Key[], info: TreeEventInfo) => { | |
| const { node } = info; | |
| const getFirstSelectableNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => | |
| getFirstMatchingNode(nodes, node => node.selectable !== false && !node.disabled); | |
| if (checkable && isCheckDisabled(node)) { | |
| const getFirstMatchNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => | |
| getFirstMatchingNode(nodes, node => filterTreeNode(node) && !node.disabled); | |
| // ========================== Get First Selectable Node ========================== | |
| const getFirstMatchingNode = ( | |
| nodes: EventDataNode<any>[], | |
| predicate: (node: EventDataNode<any>) => boolean, | |
| ): EventDataNode<any> | null => { | |
| for (const node of nodes) { | |
| if (predicate(node)) { | |
| return node; | |
| } | |
| if (node[fieldNames.children]) { | |
| const matchInChildren = getFirstMatchingNode(node[fieldNames.children], predicate); | |
| if (matchInChildren) { | |
| return matchInChildren; | |
| } | |
| } | |
| } | |
| return null; | |
| }; | |
| const getFirstSelectableNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => | |
| getFirstMatchingNode(nodes, node => node.selectable !== false && !node.disabled); | |
| const getFirstMatchNode = (nodes: EventDataNode<any>): EventDataNode<any> | null => | |
| getFirstMatchingNode(nodes, node => filterTreeNode(node) && !node.disabled); |
src/OptionList.tsx
Outdated
| // ========================== Active Key Effect ========================== | ||
| React.useEffect(() => { | ||
| if (searchValue) { | ||
| const firstMatchNode = getFirstMatchNode(memoTreeData); |
There was a problem hiding this comment.
有没有 searchValue 你的 matchNode 逻辑应该都是一样的。要么都是 selectable 的，要么都是非 disabled 的。可以考虑直接从 virtualList 里获取，这样也不需要递归，并且给出来的 TreeNode 本来就是过滤完的。
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/OptionList.tsx (2)
105-117: 建议优化选择逻辑的实现当前实现正确但可以更简洁。建议合并条件判断，减少代码嵌套。
- const onInternalSelect = (__: Key[], info: TreeEventInfo) => { - const { node } = info; - - if (checkable && isCheckDisabled(node)) { - return; - } - - onSelect(node.key, { - selected: !checkedKeys.includes(node.key), - }); - - if (!multiple) { - toggleOpen(false); - } - }; + const onInternalSelect = (__: Key[], { node }: TreeEventInfo) => { + if (checkable && isCheckDisabled(node)) return; + + onSelect(node.key, { selected: !checkedKeys.includes(node.key) }); + !multiple && toggleOpen(false); + };
239-242: 建议简化 useMemo 的条件判断当前的条件判断逻辑较复杂，可以简化为更直观的形式。
const loadDataFun = useMemo( () => (searchValue ? null : (loadData as any)), [searchValue, treeExpandedKeys || expandedKeys], - ([preSearchValue], [nextSearchValue, nextExcludeSearchExpandedKeys]) => - preSearchValue !== nextSearchValue && !!(nextSearchValue || nextExcludeSearchExpandedKeys), + ([prevSearch], [nextSearch, nextKeys]) => prevSearch !== nextSearch || !!nextKeys, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/OptionList.tsx(4 hunks)
🔇 Additional comments (3)
src/OptionList.tsx (3)
101-103: 事件处理实现正确！
preventDefault 的使用可以防止失去焦点，这对于键盘交互很重要。
Line range hint 218-223: 键盘交互逻辑完善！
新增的选择条件检查（selectable !== false && !disabled）确保了更好的可访问性体验。
158-172: 🛠️ Refactor suggestion
建议优化节点匹配逻辑
根据之前的讨论，OptionList 接收的 treeData 已经是过滤后的节点，不需要重复过滤。建议简化实现：
- const getFirstMatchingNode = (nodes: EventDataNode<any>[]): EventDataNode<any> | null => {
- for (const node of nodes) {
- if (!node.disabled && node.selectable !== false) {
- return node;
- }
-
- if (node[fieldNames.children]) {
- const matchInChildren = getFirstMatchingNode(node[fieldNames.children]);
- if (matchInChildren) {
- return matchInChildren;
- }
- }
- }
- return null;
- };
+ const getFirstMatchingNode = (nodes: EventDataNode<any>[]): EventDataNode<any> | null =>
+ nodes.find(node => !node.disabled && node.selectable !== false) || null;Likely invalid or redundant comment.
| React.useEffect(() => { | ||
| if (searchValue) { | ||
| setSearchExpandedKeys(getAllKeys(treeData, fieldNames)); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [searchValue]); |
There was a problem hiding this comment.
需要补充 useEffect 的依赖项
当前的依赖数组缺少 treeData 和 fieldNames，这可能导致搜索结果不准确。
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
- }, [searchValue]);
+ }, [searchValue, treeData, fieldNames]);📝 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.
| React.useEffect(() => { | |
| if (searchValue) { | |
| setSearchExpandedKeys(getAllKeys(treeData, fieldNames)); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [searchValue]); | |
| React.useEffect(() => { | |
| if (searchValue) { | |
| setSearchExpandedKeys(getAllKeys(treeData, fieldNames)); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [searchValue, treeData, fieldNames]); |
src/OptionList.tsx
Outdated
| React.useEffect(() => { | ||
| if (!open) { | ||
| return; | ||
| } | ||
|
|
||
| onSelect(node.key, { | ||
| selected: !checkedKeys.includes(node.key), | ||
| }); | ||
| // // Prioritize activating the searched node | ||
| if (searchValue) { | ||
| const firstNode = getFirstMatchingNode(memoTreeData); | ||
| setActiveKey(firstNode ? firstNode[fieldNames.value] : null); | ||
| return; | ||
| } | ||
|
|
||
| if (!multiple) { | ||
| toggleOpen(false); | ||
| // If no search value, activate the first checked node | ||
| if (!multiple && checkedKeys.length) { | ||
| setActiveKey(checkedKeys[0]); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // If no search value and no checked nodes, activate the first node | ||
| const firstNode = getFirstMatchingNode(memoTreeData); | ||
| setActiveKey(firstNode ? firstNode[fieldNames.value] : null); | ||
| }, [open, searchValue]); |
There was a problem hiding this comment.
需要完善 useEffect 依赖项
当前实现存在以下问题：
- 缺少重要的依赖项：
multiple、checkedKeys、memoTreeData和fieldNames - 根据之前的讨论，关闭时不需要设置
activeKey为null，这会影响动画效果
React.useEffect(() => {
- if (!open) {
- return;
- }
+ if (!open) return;
if (searchValue) {
const firstNode = getFirstMatchingNode(memoTreeData);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
return;
}
if (!multiple && checkedKeys.length) {
setActiveKey(checkedKeys[0]);
return;
}
const firstNode = getFirstMatchingNode(memoTreeData);
setActiveKey(firstNode ? firstNode[fieldNames.value] : null);
- }, [open, searchValue]);
+ }, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]);📝 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.
| React.useEffect(() => { | |
| if (!open) { | |
| return; | |
| } | |
| onSelect(node.key, { | |
| selected: !checkedKeys.includes(node.key), | |
| }); | |
| // // Prioritize activating the searched node | |
| if (searchValue) { | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| setActiveKey(firstNode ? firstNode[fieldNames.value] : null); | |
| return; | |
| } | |
| if (!multiple) { | |
| toggleOpen(false); | |
| // If no search value, activate the first checked node | |
| if (!multiple && checkedKeys.length) { | |
| setActiveKey(checkedKeys[0]); | |
| return; | |
| } | |
| }; | |
| // If no search value and no checked nodes, activate the first node | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| setActiveKey(firstNode ? firstNode[fieldNames.value] : null); | |
| }, [open, searchValue]); | |
| React.useEffect(() => { | |
| if (!open) return; | |
| // // Prioritize activating the searched node | |
| if (searchValue) { | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| setActiveKey(firstNode ? firstNode[fieldNames.value] : null); | |
| return; | |
| } | |
| // If no search value, activate the first checked node | |
| if (!multiple && checkedKeys.length) { | |
| setActiveKey(checkedKeys[0]); | |
| return; | |
| } | |
| // If no search value and no checked nodes, activate the first node | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| setActiveKey(firstNode ? firstNode[fieldNames.value] : null); | |
| }, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/OptionList.tsx (1)
105-117: 选择逻辑处理合理，建议小幅优化选择逻辑的实现考虑了禁用状态和多选模式，处理得当。不过建议将选择和关闭下拉框的逻辑分开处理，以提高代码的可维护性。
const onInternalSelect = (__: Key[], info: TreeEventInfo) => { const { node } = info; if (checkable && isCheckDisabled(node)) { return; } onSelect(node.key, { selected: !checkedKeys.includes(node.key), }); - if (!multiple) { - toggleOpen(false); - } + // 将关闭逻辑提取到单独的副作用中 + React.useEffect(() => { + if (!multiple && checkedKeys.length) { + toggleOpen(false); + } + }, [checkedKeys, multiple]); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/OptionList.tsx(4 hunks)
🔇 Additional comments (5)
src/OptionList.tsx (5)
101-103: 事件处理实现正确！
preventDefault的使用可以防止不必要的文本选择，这对于下拉列表的用户体验很重要。
222-223: 键盘交互处理得当！
选择逻辑正确考虑了节点的可选状态和禁用状态，增强了键盘访问的可用性。
243-246: 数据加载逻辑优化合理！
loadDataFun的记忆化处理考虑了搜索状态的变化，比较函数的实现准确。
141-155:
搜索功能需要完善依赖项
搜索功能的实现基本正确，但useEffect的依赖数组缺少了重要的依赖项：
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
-}, [searchValue]);
+}, [searchValue, treeData, fieldNames]);Likely invalid or redundant comment.
157-172: 🛠️ Refactor suggestion
建议优化节点查找逻辑
当前的递归实现虽然正确，但考虑到性能和已有的virtualList功能，建议重构此方法：
-const getFirstMatchingNode = (nodes: EventDataNode<any>[]): EventDataNode<any> | null => {
- for (const node of nodes) {
- if (!node.disabled && node.selectable !== false) {
- return node;
- }
-
- if (node[fieldNames.children]) {
- const matchInChildren = getFirstMatchingNode(node[fieldNames.children]);
- if (matchInChildren) {
- return matchInChildren;
- }
- }
- }
- return null;
-};
+const getFirstMatchingNode = (nodes: EventDataNode<any>[]): EventDataNode<any> | null => {
+ // 直接从virtualList中获取第一个可用节点
+ return nodes.find(node => !node.disabled && node.selectable !== false) || null;
+};Likely invalid or redundant comment.
src/OptionList.tsx
Outdated
| // ========================== Active ========================== | ||
| const [activeKey, setActiveKey] = React.useState<Key>(null); | ||
| const activeEntity = keyEntities[activeKey as SafeKey]; | ||
|
|
||
| React.useEffect(() => { | ||
| if (!open) { | ||
| return; | ||
| } | ||
| let nextActiveKey = null; | ||
|
|
||
| onSelect(node.key, { | ||
| selected: !checkedKeys.includes(node.key), | ||
| }); | ||
| const getFirstNode = () => { | ||
| const firstNode = getFirstMatchingNode(memoTreeData); | ||
| return firstNode ? firstNode[fieldNames.value] : null; | ||
| }; | ||
|
|
||
| if (!multiple) { | ||
| toggleOpen(false); | ||
| // search mode active first node | ||
| if (searchValue) { | ||
| nextActiveKey = getFirstNode(); | ||
| } | ||
| }; | ||
| // single mode active first checked node | ||
| else if (!multiple && checkedKeys.length) { | ||
| nextActiveKey = checkedKeys[0]; | ||
| } | ||
| // default active first node | ||
| else { | ||
| nextActiveKey = getFirstNode(); | ||
| } | ||
|
|
||
| setActiveKey(nextActiveKey); | ||
| }, [open, searchValue]); |
There was a problem hiding this comment.
活动键管理需要补充依赖项
活动键管理的逻辑符合需求，但useEffect缺少了关键依赖：
React.useEffect(() => {
if (!open) {
return;
}
// ...
setActiveKey(nextActiveKey);
-}, [open, searchValue]);
+}, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]);📝 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.
| // ========================== Active ========================== | |
| const [activeKey, setActiveKey] = React.useState<Key>(null); | |
| const activeEntity = keyEntities[activeKey as SafeKey]; | |
| React.useEffect(() => { | |
| if (!open) { | |
| return; | |
| } | |
| let nextActiveKey = null; | |
| onSelect(node.key, { | |
| selected: !checkedKeys.includes(node.key), | |
| }); | |
| const getFirstNode = () => { | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| return firstNode ? firstNode[fieldNames.value] : null; | |
| }; | |
| if (!multiple) { | |
| toggleOpen(false); | |
| // search mode active first node | |
| if (searchValue) { | |
| nextActiveKey = getFirstNode(); | |
| } | |
| }; | |
| // single mode active first checked node | |
| else if (!multiple && checkedKeys.length) { | |
| nextActiveKey = checkedKeys[0]; | |
| } | |
| // default active first node | |
| else { | |
| nextActiveKey = getFirstNode(); | |
| } | |
| setActiveKey(nextActiveKey); | |
| }, [open, searchValue]); | |
| // ========================== Active ========================== | |
| const [activeKey, setActiveKey] = React.useState<Key>(null); | |
| const activeEntity = keyEntities[activeKey as SafeKey]; | |
| React.useEffect(() => { | |
| if (!open) { | |
| return; | |
| } | |
| let nextActiveKey = null; | |
| const getFirstNode = () => { | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| return firstNode ? firstNode[fieldNames.value] : null; | |
| }; | |
| // search mode active first node | |
| if (searchValue) { | |
| nextActiveKey = getFirstNode(); | |
| } | |
| // single mode active first checked node | |
| else if (!multiple && checkedKeys.length) { | |
| nextActiveKey = checkedKeys[0]; | |
| } | |
| // default active first node | |
| else { | |
| nextActiveKey = getFirstNode(); | |
| } | |
| setActiveKey(nextActiveKey); | |
| }, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/OptionList.tsx (2)
189-193: 建议简化活动键设置逻辑当前的条件判断结构可以更简洁。根据之前的讨论，可以先检查单选模式下的选中项，其他情况统一处理。
建议修改如下：
- if (!multiple && checkedKeys.length && !searchValue) { - nextActiveKey = checkedKeys[0]; - } else { - nextActiveKey = getFirstNode(); - } + nextActiveKey = (!multiple && checkedKeys.length && !searchValue) + ? checkedKeys[0] + : getFirstNode();
237-240: 建议优化 memoization 逻辑的可读性当前的实现逻辑较为复杂，可以通过拆分条件提高可读性。
建议修改如下：
useMemo( () => (searchValue ? null : (loadData as any)), [searchValue, treeExpandedKeys || expandedKeys], - ([preSearchValue], [nextSearchValue, nextExcludeSearchExpandedKeys]) => - preSearchValue !== nextSearchValue && !!(nextSearchValue || nextExcludeSearchExpandedKeys), + ([preSearchValue], [nextSearchValue, nextExpandedKeys]) => { + const hasSearchValueChanged = preSearchValue !== nextSearchValue; + const hasValidNextState = !!(nextSearchValue || nextExpandedKeys); + return hasSearchValueChanged && hasValidNextState; + }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/OptionList.tsx(4 hunks)
🔇 Additional comments (4)
src/OptionList.tsx (4)
101-103: 事件处理符合预期
preventDefault的使用正确，可以防止失去焦点，有助于维持键盘导航状态。
105-117: 选择逻辑处理完善
选择处理逻辑考虑周全：
- 正确处理了禁用状态的检查
- 适当处理了单选模式下的下拉框关闭
- 维护了选中状态的更新
216-217: 键盘交互处理得当
Enter键的处理逻辑完善：
- 正确检查了可选择状态
- 适当处理了禁用状态
- 符合无障碍访问要求
150-155:
需要补充 useEffect 依赖项
当前的依赖数组缺少了 treeData 和 fieldNames，这可能导致在这些值发生变化时不会触发预期的更新。
建议修改如下：
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
- }, [searchValue]);
+ }, [searchValue, treeData, fieldNames]);Likely invalid or redundant comment.
| if (node[fieldNames.children]) { | ||
| const matchInChildren = getFirstMatchingNode(node[fieldNames.children]); |
There was a problem hiding this comment.
需要增加空值检查
访问 node[fieldNames.children] 时缺少必要的空值检查，可能导致运行时错误。
建议修改如下：
- if (node[fieldNames.children]) {
+ if (node?.[fieldNames.children]?.length) {📝 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.
| if (node[fieldNames.children]) { | |
| const matchInChildren = getFirstMatchingNode(node[fieldNames.children]); | |
| if (node?.[fieldNames.children]?.length) { | |
| const matchInChildren = getFirstMatchingNode(node[fieldNames.children]); |
tests/Select.SearchInput.spec.js
Outdated
| describe('keyboard events', () => { | ||
| it('should select first matched node when press enter', () => { | ||
| const onSelect = jest.fn(); | ||
| const wrapper = mount( |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #592 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 520 545 +25
Branches 151 163 +12
=========================================
+ Hits 520 545 +25 ☔ View full report in Codecov by Sentry. |
|
覆盖率掉了 |
我改一下 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/OptionList.tsx(4 hunks)tests/Select.SearchInput.spec.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Select.SearchInput.spec.js
🔇 Additional comments (6)
src/OptionList.tsx (6)
100-103: 事件处理实现正确！
onListMouseDown 的实现简洁且有效，正确阻止了默认行为。
105-117: 选择处理逻辑完善！
选择处理器正确实现了：
- 禁用节点的检查
- 单选模式下的下拉框关闭
224-225: 键盘事件处理完善！
Enter 键处理逻辑正确实现了：
- 可选状态检查
- 禁用状态检查
245-248: 数据加载函数优化得当！
loadDataFun 的记忆化逻辑正确处理了：
- 搜索状态变化
- 展开键变化
172-173:
需要添加空值检查！
访问 node[fieldNames.children] 时没有进行必要的空值检查，可能导致运行时错误。建议修改如下：
- if (node[fieldNames.children]) {
+ if (node?.[fieldNames.children]?.length) {Likely invalid or redundant comment.
150-155:
需要补充 useEffect 依赖项！
当前实现缺少了 treeData 和 fieldNames 作为依赖项，这可能导致搜索结果不准确。建议修改如下：
React.useEffect(() => {
if (searchValue) {
setSearchExpandedKeys(getAllKeys(treeData, fieldNames));
}
- }, [searchValue]);
+ }, [searchValue, treeData, fieldNames]);Likely invalid or redundant comment.
| React.useEffect(() => { | ||
| if (!open) { | ||
| return; | ||
| } | ||
| let nextActiveKey = null; | ||
|
|
||
| onSelect(node.key, { | ||
| selected: !checkedKeys.includes(node.key), | ||
| }); | ||
| const getFirstNode = () => { | ||
| const firstNode = getFirstMatchingNode(memoTreeData); | ||
| return firstNode ? firstNode[fieldNames.value] : null; | ||
| }; | ||
|
|
||
| if (!multiple) { | ||
| toggleOpen(false); | ||
| // single mode active first checked node | ||
| if (!multiple && checkedKeys.length && !searchValue) { | ||
| nextActiveKey = checkedKeys[0]; | ||
| } else { | ||
| nextActiveKey = getFirstNode(); | ||
| } | ||
| }; | ||
|
|
||
| setActiveKey(nextActiveKey); | ||
| }, [open, searchValue]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议优化活动键管理逻辑并补充依赖项！
- 逻辑可以简化：先检查单选模式下的选中键，其他情况都使用
getFirstNode - 需要补充依赖项
React.useEffect(() => {
if (!open) {
return;
}
let nextActiveKey = null;
- if (!multiple && checkedKeys.length && !searchValue) {
+ if (!multiple && checkedKeys.length) {
nextActiveKey = checkedKeys[0];
} else {
nextActiveKey = getFirstNode();
}
setActiveKey(nextActiveKey);
- }, [open, searchValue]);
+ }, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]);📝 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.
| React.useEffect(() => { | |
| if (!open) { | |
| return; | |
| } | |
| let nextActiveKey = null; | |
| onSelect(node.key, { | |
| selected: !checkedKeys.includes(node.key), | |
| }); | |
| const getFirstNode = () => { | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| return firstNode ? firstNode[fieldNames.value] : null; | |
| }; | |
| if (!multiple) { | |
| toggleOpen(false); | |
| // single mode active first checked node | |
| if (!multiple && checkedKeys.length && !searchValue) { | |
| nextActiveKey = checkedKeys[0]; | |
| } else { | |
| nextActiveKey = getFirstNode(); | |
| } | |
| }; | |
| setActiveKey(nextActiveKey); | |
| }, [open, searchValue]); | |
| React.useEffect(() => { | |
| if (!open) { | |
| return; | |
| } | |
| let nextActiveKey = null; | |
| const getFirstNode = () => { | |
| const firstNode = getFirstMatchingNode(memoTreeData); | |
| return firstNode ? firstNode[fieldNames.value] : null; | |
| }; | |
| // single mode active first checked node | |
| if (!multiple && checkedKeys.length) { | |
| nextActiveKey = checkedKeys[0]; | |
| } else { | |
| nextActiveKey = getFirstNode(); | |
| } | |
| setActiveKey(nextActiveKey); | |
| }, [open, searchValue, multiple, checkedKeys, memoTreeData, fieldNames]); |
🚀 Feature Description
Enhanced keyboard interaction in search mode to improve accessibility.
Changes
🎯 Motivation
Improve accessibility for keyboard users by providing a more intuitive way to select nodes when searching. Previously, pressing enter would select the active node regardless of search context, which could lead to unexpected selections.
📝 Implementation Details
Modified the keyboard event handling in
OptionList.tsxto:🧪 Testing
Added comprehensive test cases in
Select.SearchInput.spec.jscovering:📋 Checklist
📸 Screenshots/GIFs
N/A
📚 Related Issues
Summary by CodeRabbit
OptionList组件的搜索功能，新增了根据搜索条件识别首个可选节点的方法。.gitignore文件，以忽略更多的配置文件和锁文件。