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该 PR 在 Changes
Sequence Diagram(s)（此次变更为局部事件/钩子调整，未生成序列图） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/index.test.tsx (1)
🔇 Additional comments (3)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 94.36% 94.77% +0.40%
==========================================
Files 7 7
Lines 142 153 +11
Branches 49 50 +1
==========================================
+ Hits 134 145 +11
Misses 8 8 ☔ 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 的常见场景
2 participants
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.