chore: add IME lock to prevent unintended close #34
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough将 Changes
Sequence Diagram(s)（无序列图：变更为局部事件与内部栈/监听器重构，不满足生成序列图的条件） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello @aojunhao123, 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! This pull request enhances the 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
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 94.36% 94.57% +0.21%
==========================================
Files 7 7
Lines 142 166 +24
Branches 49 53 +4
==========================================
+ Hits 134 157 +23
- Misses 8 9 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 an IME lock to prevent the onEsc callback from firing unintentionally when the Escape key is used to interact with an IME. The implementation is clean and includes a new test case that validates the behavior correctly. I have a couple of suggestions to improve the reliability of the timing mechanism by using performance.now() instead of Date.now(). Additionally, I noticed a potential pre-existing issue in useEscKeyDown.ts where useMemo is used for side effects and logic is duplicated with useEffect, which could lead to bugs. I recommend addressing this in a follow-up to improve the hook's correctness.
src/useEscKeyDown.ts
Outdated
| const now = Date.now(); | ||
| if (now - compositionEndTimeRef.current < IME_LOCK_DURATION) { |
There was a problem hiding this comment.
For measuring time intervals, performance.now() is generally preferred over Date.now(). performance.now() provides a monotonically increasing timestamp that is not affected by system time changes, making it more reliable for calculating durations.
| const now = Date.now(); | |
| if (now - compositionEndTimeRef.current < IME_LOCK_DURATION) { | |
| const now = performance.now(); | |
| if (now - compositionEndTimeRef.current < IME_LOCK_DURATION) { |
src/useEscKeyDown.ts
Outdated
| }); | ||
|
|
||
| const handleCompositionEnd = useEvent(() => { | ||
| compositionEndTimeRef.current = Date.now(); |
There was a problem hiding this comment.
For consistency and reliability in measuring time intervals, it's better to use performance.now() here as well, to match the usage in handleEscKeyDown.
| compositionEndTimeRef.current = Date.now(); | |
| compositionEndTimeRef.current = performance.now(); |
![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
🧹 Nitpick comments (1)
tests/index.test.tsx (1)
373-397: 测试逻辑正确，建议考虑导入常量以提高可维护性。测试覆盖了 IME 锁定窗口的行为：在
compositionEnd后的 200ms 内按下 Escape 不会触发onEsc，超过 200ms 后才会触发。逻辑清晰正确。不过，测试中的时间值（100ms、150ms）与实现中的
IME_LOCK_DURATION（200ms）紧密耦合。如果将来调整锁定时长，测试也需要同步修改。可以考虑从实现文件导出该常量以提高可维护性。🔎 可选优化方案
- import { stack } from '../src/useEscKeyDown'; + import { stack, IME_LOCK_DURATION } from '../src/useEscKeyDown';然后在测试中使用：
jest.advanceTimersByTime(IME_LOCK_DURATION - 100); // ... jest.advanceTimersByTime(100 + 50); // 超过 IME_LOCK_DURATION
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/useEscKeyDown.tstests/index.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/useEscKeyDown.ts (1)
src/Portal.tsx (1)
EscCallback(24-30)
🔇 Additional comments (4)
src/useEscKeyDown.ts (4)
8-9: 常量定义合理。200ms 的锁定时长对于处理 IME 组合结束后的误触场景是合适的。如果需要在测试中使用该值，可以考虑导出此常量。
13-29: IME 锁定逻辑实现正确。
compositionEndTimeRef初始值为 0 确保首次 Escape 不会被误阻止- 使用
useEvent包装handleCompositionEnd保持回调引用稳定，避免重复注册监听器- 时间差判断逻辑清晰，
Date.now() - compositionEndTimeRef.current < IME_LOCK_DURATION能有效过滤 IME 结束后的短时间内的 Escape 按键
48-55: 事件监听器管理正确。
compositionend监听器在 Portal 打开时注册，关闭时清理，与keydown监听器保持一致的模式。确保了不会产生内存泄漏。
10-57: 整体实现审查通过。该实现有效防止了 IME 组合结束后立即按下 Escape 键导致的意外关闭行为：
- 每个 Portal 实例独立维护
compositionEndTimeRefcompositionend事件在window上监听，能捕获所有输入框的 IME 结束事件- 锁定窗口（200ms）足够覆盖用户完成 IME 输入后按 Escape 的常见场景
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/useEscKeyDown.ts (1)
26-27: 建议使用performance.now()替代Date.now()进行时间间隔测量。
performance.now()提供单调递增的时间戳，不受系统时间调整的影响，对于计算时间间隔更加可靠。此外，这也与之前的评审建议一致。🔎 建议的修复方案
const onGlobalKeyDown = (event: KeyboardEvent) => { if (event.key === 'Escape' && !event.isComposing) { - const now = Date.now(); + const now = performance.now(); if (now - lastCompositionEndTime < IME_LOCK_DURATION) { return; }const onGlobalCompositionEnd = () => { - lastCompositionEndTime = Date.now(); + lastCompositionEndTime = performance.now(); };Also applies to: 41-43
🧹 Nitpick comments (1)
src/useEscKeyDown.ts (1)
72-78:useMemo用于执行副作用不符合 React 最佳实践。
useMemo的设计目的是缓存计算值，而非执行副作用。React 文档明确指出useMemo的回调可能在某些情况下被跳过或重复执行。当前代码已经在useEffect中有相同的逻辑，这里的useMemo用于在渲染期间同步更新 stack，但这种模式不够清晰。建议考虑使用
useLayoutEffect或重构逻辑，使 stack 管理完全在 effect 中完成。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/useEscKeyDown.tstests/index.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
tests/index.test.tsx (1)
src/useEscKeyDown.ts (1)
_test(12-21)
src/useEscKeyDown.ts (1)
src/Portal.tsx (1)
EscCallback(24-30)
🔇 Additional comments (5)
src/useEscKeyDown.ts (2)
45-55: 全局事件监听器的附加逻辑是正确的。
addEventListener在使用相同函数引用时不会重复添加，所以多次调用attachGlobalEventListeners是安全的。detachGlobalEventListeners在 stack 为空时正确移除监听器。这种实现避免了为每个 Portal 实例添加独立监听器的问题，符合之前评审中 zombieJ 提出的建议。
62-66: 此 concern 是不必要的 - 代码已通过useEvent正确处理了回调更新。
useEvent从@rc-component/util实现了"稳定身份+最新回调"模式：它返回一个保持恒定引用的包装函数，该函数在内部通过 ref 跟踪最新的回调，并在每次调用时都执行最新的回调。因此，即使 stack 中的条目未被更新，当事件触发时（第 33 行），调用的仍然是最新的onEsc回调，而非旧的回调。依赖数组[open]是正确的，因为onEventEsc本身是稳定的引用。tests/index.test.tsx (3)
301-309: 测试设置正确，使用_test().reset()和 fake timers。在
beforeEach中重置lastCompositionEndTime并启用 fake timers，在afterEach中正确清理。这确保了测试之间的隔离性。如果采纳
performance.now()的建议，Jest 27+ 的 modern fake timers 默认会 mockperformance.now()，所以无需额外配置。
383-406: IME 锁定时长测试逻辑正确。测试验证了：
compositionEnd后立即按 Escape 被阻止 (t=0)- 100ms 后按 Escape 仍被阻止 (t=100 < 200ms)
- 250ms 后按 Escape 成功触发 (t=250 >= 200ms)
时间计算正确，覆盖了 IME 锁定功能的核心行为。
415-418: 使用_test().stack验证 unmount 后 stack 清理是正确的测试方式。确保了组件卸载时正确清理 stack 条目，避免内存泄漏和状态污染。
2 participants
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.