Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: supplement maxCount logic for complicated cases #602

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aojunhao123
Copy link
Contributor

@aojunhao123 aojunhao123 commented Dec 5, 2024

TreeSelect maxCount 功能优化

修复了 TreeSelect 组件在设置 maxCount 时对树形结构处理的问题,使其能够正确处理父子节点关系和树的传导规则。

主要改动

  • 修复了设置 maxCount 时父节点选择的问题
  • 优化了节点禁用状态的计算逻辑,当选中会导致超出 maxCount 限制时正确禁用节点
  • 改进了对禁用节点的处理,正确实现树的传导规则
  • 根据不同的 showCheckedStrategy 策略准确计算显示值数量

具体改进

  1. 父节点选中时会考虑其子节点数量,提前禁用可能导致超出 maxCount 的节点
  2. 遵循树的传导规则计算可选节点
  3. 在节点遍历时正确处理禁用状态
  4. 保持已选中节点始终可选
  5. 针对不同的显示策略准确计算显示值数量:
    • SHOW_ALL:计算所有选中节点
    • SHOW_CHILD:仅计算叶子节点

相关问题

修复了以下场景的问题:

  1. 当父节点包含大量子节点时,选中父节点可能导致超出 maxCount 限制
  2. 在不同显示策略下,maxCount 的计算不准确
  3. 已选中节点被错误禁用的问题

Summary by CodeRabbit

Summary by CodeRabbit

  • 新功能

    • TreeSelect 组件中引入了最大选择数量 (maxCount) 和显示选中策略 (showCheckedStrategy) 属性。
    • 更新了 OptionList 组件的节点选择逻辑,增强了对最大选择数量的管理。
    • 在示例中新增了禁用节点的支持。
    • 新增了针对复杂树结构的测试用例,以验证最大选择数量的处理。
  • bug 修复

    • 改进了选项选择逻辑,以确保在多选模式下不会超过最大选择数量。

Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tree-select ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 5:39am

Copy link

coderabbitai bot commented Dec 5, 2024

Walkthrough

此次更改涉及多个组件的修改,主要集中在 TreeSelectOptionListTreeSelectContext 中。引入了 maxCountshowCheckedStrategy 两个新属性,以增强选择逻辑和上下文的可配置性。OptionList 组件的节点选择逻辑经过改进,确保在选择过程中遵循最大选择数量的限制。同时,treeData 结构在示例文件中也进行了调整,增加了禁用节点的功能。

Changes

文件路径 更改摘要
examples/mutiple-with-maxCount.tsx treeData 结构中添加了禁用节点,更新了节点的子项。保留了 showCheckedStrategy 的注释。
src/OptionList.tsx 引入了新导入,增强了选择逻辑,更新了 nodeDisabled 函数以支持最大选择数量,更新了 useEffect 钩子。
src/TreeSelect.tsx 更新了 treeSelectContext,添加了 maxCountshowCheckedStrategy,改进了选择逻辑和状态管理。
src/TreeSelectContext.ts TreeSelectContextProps 接口中添加了 maxCountshowCheckedStrategy 属性,增强了上下文的可配置性。

Possibly related PRs

Suggested reviewers

  • zombieJ

兔子欢快跳跃,代码更新真妙,
树形选择更灵活,禁用节点不再烦恼。
最大选择数把控,策略显示更明了,
让我们一起欢庆,代码世界更美好! 🐰✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a1284ba) to head (8f47675).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #602   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines          561       586   +25     
  Branches       167       168    +1     
=========================================
+ Hits           561       586   +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/OptionList.tsx (1)

171-221: 建议优化 nodeDisabled 函数的可读性

当前的 nodeDisabled 函数逻辑较为复杂,嵌套深度较大,可能影响代码的可读性和维护性。建议将复杂的逻辑拆分为多个小函数,或在关键部分添加注释,以提高代码的可读性。

examples/mutiple-with-maxCount.tsx (1)

87-87: 考虑清理已注释的 showCheckedStrategy 属性

在第 87 行,showCheckedStrategy="SHOW_PARENT" 被注释掉。若不再需要,建议删除该注释代码;若需保留,建议添加说明以解释注释原因。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a1284ba and 6193365.

📒 Files selected for processing (4)
  • examples/mutiple-with-maxCount.tsx (2 hunks)
  • src/OptionList.tsx (3 hunks)
  • src/TreeSelect.tsx (2 hunks)
  • src/TreeSelectContext.ts (2 hunks)
🔇 Additional comments (4)
src/TreeSelect.tsx (2)

628-629: 确保在上下文中正确传递 maxCountshowCheckedStrategy

maxCountmergedShowCheckedStrategy 添加到 treeSelectContext 中,确保子组件能够访问并使用这些新添加的配置属性。


646-646: 在依赖项数组中包含 mergedShowCheckedStrategy

useMemo 的依赖项数组中添加 mergedShowCheckedStrategy,确保当该策略更改时,treeSelectContext 能够正确地重新计算和更新。

src/TreeSelectContext.ts (2)

4-4: 引入 CheckedStrategy 类型

'./utils/strategyUtil' 导入 CheckedStrategy,以便在接口中使用该类型,增强类型检查和代码可读性。


20-21: TreeSelectContextProps 接口中添加 maxCountshowCheckedStrategy 属性

TreeSelectContextProps 接口新增可选的 maxCountshowCheckedStrategy 属性,使上下文配置更灵活,支持最大选择数量和选择显示策略的定制。

// Simulate checked state after selecting current node
const simulatedCheckedKeys = [...checkedKeys, ...selectableNodeValues];

const { checkedKeys: conductedKeys } = conductCheck(simulatedCheckedKeys, true, keyEntities);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个对于没有开启 virtual-list 的场景,会雪崩。

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
tests/Select.maxCount.spec.tsx (1)

449-470: 建议将代码注释统一为英文

为了保持代码库的一致性和方便全球开发者理解,建议将测试代码中的中文注释翻译为英文。

Also applies to: 493-502

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c96aee2 and c8b2c92.

📒 Files selected for processing (5)
  • examples/mutiple-with-maxCount.tsx (2 hunks)
  • src/OptionList.tsx (3 hunks)
  • src/TreeSelect.tsx (3 hunks)
  • src/TreeSelectContext.ts (2 hunks)
  • tests/Select.maxCount.spec.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/TreeSelectContext.ts
  • examples/mutiple-with-maxCount.tsx
🧰 Additional context used
🪛 eslint
src/OptionList.tsx

[error] 178-178: 'fieldNames' is already declared in the upper scope on line 46 column 5.

(@typescript-eslint/no-shadow)

🔇 Additional comments (1)
tests/Select.maxCount.spec.tsx (1)

375-503: 测试用例正确地覆盖了复杂场景

新增的测试用例有效地验证了在复杂树结构下 maxCount 的行为,逻辑清晰,符合预期。

src/OptionList.tsx Outdated Show resolved Hide resolved
src/TreeSelect.tsx Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
src/OptionList.tsx (1)

164-176: 建议优化缓存重置逻辑

当前实现在 checkedKeysmaxCount 变化时会清空整个缓存。考虑以下优化建议:

  1. 可以只清除受影响的缓存项,而不是清空整个 Map
  2. 考虑添加缓存大小限制,避免在大型树结构中内存占用过大
  const resetCache = React.useCallback(() => {
-   disabledCacheRef.current.clear();
+   // 只清除受影响的缓存项
+   const affectedPrefix = `${checkedKeys.join(',')}-${maxCount}`;
+   for (const key of disabledCacheRef.current.keys()) {
+     if (key.includes(affectedPrefix)) {
+       disabledCacheRef.current.delete(key);
+     }
+   }
    lastCheckedKeysRef.current = [...checkedKeys];
    lastMaxCountRef.current = maxCount;
  }, [checkedKeys, maxCount]);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c8b2c92 and ffe87e5.

📒 Files selected for processing (2)
  • src/OptionList.tsx (3 hunks)
  • src/TreeSelectContext.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/TreeSelectContext.ts
🔇 Additional comments (2)
src/OptionList.tsx (2)

12-16: 导入和上下文变更看起来不错!

新增的导入和上下文值很好地支持了 maxCount 和 showCheckedStrategy 的功能实现。

Also applies to: 53-54


192-226: ⚠️ Potential issue

性能优化警告:需要处理非虚拟列表场景

根据之前 @zombieJ 的评论,当前实现在未启用虚拟列表时可能会导致性能雪崩。建议:

  1. 在非虚拟列表场景下,实现节点懒加载机制
  2. 考虑使用 worker 线程处理复杂计算
  3. 实现节点计算结果的批量更新
  const nodeDisabled = useEvent((node: DataNode) => {
+   // 非虚拟列表场景下的性能优化
+   if (!virtual) {
+     const batchSize = 100;
+     const throttleMs = 16;
+     
+     // 实现节点计算的批量处理逻辑
+     // TODO: 添加具体实现
+   }
    
    // 原有逻辑保持不变
    const nodeValue = node[fieldNames.value];
    // ...
  });

src/OptionList.tsx Show resolved Hide resolved
@@ -163,8 +161,69 @@ const OptionList: React.ForwardRefRenderFunction<ReviseRefOptionListProps> = (_,
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchValue]);

const disabledCacheRef = React.useRef(new Map<Key, boolean>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个 cache 用处不大,如果是 defaultExpandedAll + 关闭 virtual。初始化阶段没有 cache 会直接炸死。

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/TreeSelect.tsx (2)

Line range hint 89-89: 建议为 maxCount 属性添加 JSDoc 注释

为了提高代码的可维护性和使用体验,建议添加 JSDoc 注释来说明 maxCount 属性的用途、限制条件和使用示例。

建议添加如下注释:

+ /** 
+  * 最大可选择的节点数量
+  * @default undefined
+  */
  maxCount?: number;

Line range hint 391-476: 建议重构 onOptionSelect 函数以提高可维护性

当前的 onOptionSelect 函数职责过重,包含了选择逻辑、树形结构处理和 maxCount 限制等多个功能。建议将其拆分为更小的、职责单一的函数。

建议按照以下方式重构:

const canSelectMore = (newValue: SafeKey) => {
  if (!mergedMultiple || maxCount === undefined) {
    return true;
  }
  
  const newNodeCount = getNodeCount(newValue, keyEntities);
  return calculateSelectedCount + newNodeCount <= maxCount;
};

const handleTreeConduction = (newRawValues: SafeKey[], selected: boolean) => {
  const { missingRawValues, existRawValues } = splitRawValues(newRawValues);
  const keyList = existRawValues.map(val => valueEntities.get(val).key);
  
  const { checkedKeys } = selected
    ? conductCheck(keyList, true, keyEntities)
    : conductCheck(keyList, { checked: false, halfCheckedKeys: rawHalfCheckedValues }, keyEntities);
    
  return [...missingRawValues, ...checkedKeys.map(key => 
    keyEntities[key as SafeKey].node[mergedFieldNames.value])];
};

const onOptionSelect = (selectedKey: SafeKey, { selected, source }) => {
  const entity = keyEntities[selectedKey];
  const node = entity?.node;
  const selectedValue = node?.[mergedFieldNames.value] ?? selectedKey;

  if (!mergedMultiple) {
    triggerChange([selectedValue], { selected: true, triggerValue: selectedValue }, 'option');
    onSelect?.(selectedValue, fillLegacyProps(node));
    return;
  }

  if (selected && !canSelectMore(selectedValue)) {
    return;
  }

  const newRawValues = selected
    ? [...rawValues, selectedValue]
    : rawCheckedValues.filter(v => v !== selectedValue);

  const finalValues = treeConduction 
    ? handleTreeConduction(newRawValues, selected)
    : newRawValues;

  triggerChange(finalValues, { selected, triggerValue: selectedValue }, source || 'option');

  if (selected) {
    onSelect?.(selectedValue, fillLegacyProps(node));
  } else {
    onDeselect?.(selectedValue, fillLegacyProps(node));
  }
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ffe87e5 and 8f47675.

📒 Files selected for processing (2)
  • src/OptionList.tsx (3 hunks)
  • src/TreeSelect.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/OptionList.tsx
🔇 Additional comments (1)
src/TreeSelect.tsx (1)

Line range hint 4-4: 建议补充 maxCount 功能的测试用例

为确保 maxCount 功能在各种场景下的正确性,建议补充以下测试场景:

  1. 不同 showCheckedStrategy 策略下的 maxCount 限制
  2. 父子节点选择时的 maxCount 计数
  3. 取消选择时的状态恢复
  4. 边界条件测试(maxCount 为 0、1 等)

src/TreeSelect.tsx Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants