-
-
Notifications
You must be signed in to change notification settings - Fork 486
enhance: support filter not change #1193
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
base: master
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @zombieJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在解决 Ant Design Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
概览此更改为 变更
序列图sequenceDiagram
participant User as 用户
participant BaseSelect as BaseSelect 组件
participant useOpen as useOpen 钩子
participant OptionList as OptionList 组件
participant Context as BaseSelectContext
User->>BaseSelect: 点击触发打开选择器
BaseSelect->>useOpen: 调用 toggleOpen()
activate useOpen
Note over useOpen: setLock(true)<br/>计算 nextOpenVal
useOpen->>useOpen: 锁定启用,阻止选项更新
useOpen->>BaseSelect: 返回 [open, toggleOpen, lockOptions=true]
deactivate useOpen
BaseSelect->>Context: 更新上下文 lockOptions=true
OptionList->>Context: 读取 lockOptions=true
Note over OptionList: flattenOptions 记忆化跳过<br/>(lockOptions=true 时不重新计算)
useOpen->>useOpen: triggerEvent() 后清除锁
useOpen->>BaseSelect: 返回 lockOptions=false
BaseSelect->>Context: 更新上下文 lockOptions=false
OptionList->>Context: 读取 lockOptions=false
Note over OptionList: flattenOptions 可以安全<br/>重新计算和渲染
预估代码审查工作量🎯 3 (中等) | ⏱️ ~25 分钟 可能相关的 PR
诗句
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1193 +/- ##
=======================================
Coverage 99.42% 99.42%
=======================================
Files 31 31
Lines 1219 1222 +3
Branches 412 412
=======================================
+ Hits 1212 1215 +3
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
您好,感谢您的贡献。这个 PR 旨在通过引入一个锁来解决关闭 Select 时选项列表闪烁的问题,这个思路是正确的。useOpen hook 中的锁状态管理看起来没有问题。
然而,在 OptionList.tsx 中使用这个锁的方式似乎引入了一个严重的问题:它可能会导致搜索过滤功能失效。我在代码中留下了具体的审查意见和修改建议。请您关注一下这个 useMemo 的实现,确保在修复闪烁问题的同时,不会影响到现有的过滤功能。
| const memoFlattenOptions = useMemo( | ||
| () => flattenOptions, | ||
| [open, flattenOptions], | ||
| (prev, next) => next[0] && prev[1] !== next[1], | ||
| [open, lockOptions], | ||
| (prev, next) => next[0] && !next[1], | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个 useMemo 的实现似乎会破坏搜索过滤功能。当前的逻辑是当下拉框打开且未锁定时(open && !lockOptions),memoFlattenOptions 将不会更新。这会导致用户在搜索时,选项列表不会刷新。
为了解决这个问题,同时保留修复闪烁的逻辑,我们可以修改 useMemo 的依赖和比较函数。我们希望仅在 lockOptions 为 true 时“冻结”选项列表,或者在 flattenOptions 没有实际变化时避免不必要的重算。
以下是一个建议的修改,它能正确处理过滤和锁定:
const memoFlattenOptions = useMemo(
() => flattenOptions,
[lockOptions, flattenOptions],
(prev, next) => next[0] || prev[1] === next[1],
);
There was a problem hiding this 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useOpen.ts (1)
82-91: 当cancelFun返回true时 lock 状态未被重置。在
triggerUpdate函数中,当cancelFun?.()返回true时,整个 if 块被跳过,导致第 90 行的setLock(false)不被执行。此时 lock 会保持在第 79 行设置的true状态。虽然当前 BaseSelect 的使用场景中,
onRootBlur中的 cancel 会紧接着被onRootMouseDown的triggerOpen(true)调用重置,但这种依赖于后续操作的重置方式存在风险。如果未来有其他调用者以不同的方式使用triggerOpen(false, { cancelFun })但未紧跟toggleOpen(true)调用,lock 将长期处于true状态,可能影响lockOptions相关的 UI 更新逻辑(如 OptionList 的 memoization)。建议在 cancel 发生时也重置 lock 状态,或在 API 层面明确文档化这一行为约束。
🧹 Nitpick comments (1)
src/OptionList.tsx (1)
70-74: 建议添加注释说明比较器逻辑。当前的 comparator 逻辑比较隐晦:
next[0] && !next[1]即open && !lockOptions- 返回
true时更新 options,返回false时使用缓存这意味着:当下拉框正在关闭时(
lockOptions=true),保持 options 缓存不变,防止 UI 闪烁。建议添加注释帮助后续维护者理解这段逻辑。建议的改进
const memoFlattenOptions = useMemo( () => flattenOptions, [open, lockOptions], + // Only update options when: + // 1. Dropdown is open (next[0] = true) + // 2. Options are not locked during close transition (next[1] = false) + // This prevents UI flicker when closing the dropdown (prev, next) => next[0] && !next[1], );
|
如果jack不在新的列表里了呢 |
lock 是无论如何在一个 macro task 后都会释放的,就会更新到同步的状态 |
ref ant-design/ant-design#56049
当设置 open false 的时候,设置一个临时锁。在 open 生效后解锁,以保持 OptionList 渲染同步。以解决闪烁问题:
PS: jsdom 无法模拟,sad
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.