fix: defaultPopupVisible should work #592
Conversation
|
Caution Review failedThe pull request is closed. 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将 Trigger 组件的内部 open 状态从 useState 迁移到 useControlledState，移除 popupVisible 到内部状态的同步副作用，统一使用 internalOpen 管理弹窗可见性；新增 defaultPopupVisible 的测试覆盖并调整示例导入方式。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Consumer as 使用方
participant Trigger as Trigger 组件
participant useControlled as useControlledState
Note over Consumer,Trigger: 初始化
Consumer->>Trigger: 提供 props (maybe popupVisible / defaultPopupVisible)
Trigger->>useControlled: 初始化 internalOpen (依据 props)
useControlled-->>Trigger: internalOpen state
alt 用户/事件 触发打开/关闭
Consumer->>Trigger: click / hover / internal event
Trigger->>useControlled: setInternalOpen(nextOpen)
useControlled-->>Trigger: internalOpen 更新
Trigger->>Consumer: render popup (based on internalOpen)
end
Note over Trigger: 不再同步外部 popupVisible 到 internalOpen（无后续 effect）
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟 可能需额外关注：
Possibly related PRs
Suggested reviewers
诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 @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! This pull request refactors the internal state management of the Highlights
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
|
![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 addresses an issue with defaultPopupVisible by refactoring the component's state management. The introduction of the useControlledState hook is a significant improvement, simplifying the logic for handling both controlled and uncontrolled states of the popup's visibility. This change removes complex manual state synchronization, making the code cleaner and more robust. The addition of a specific test case for defaultPopupVisible ensures the fix is effective and prevents future regressions. The changes are well-implemented, with one minor point of feedback regarding an unused import.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #592 +/- ##
==========================================
- Coverage 96.42% 96.40% -0.02%
==========================================
Files 17 17
Lines 952 947 -5
Branches 277 279 +2
==========================================
- Hits 918 913 -5
Misses 34 34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/basic.test.jsx (1)
994-1002:defaultPopupVisible用例覆盖到关键行为，建议补一个最小popup（可选）这个测试很好地验证了：在不触发任何事件的情况下，
defaultPopupVisible为true时会默认渲染.rc-trigger-popup，能防止这类回归再次出现。唯一的小建议是：
TriggerProps中popup是必填属性，示例代码里省略了它，在类型层面略微违背组件约定。可以考虑补一个空的节点，使测试更贴近真实用法，例如：- render( - <Trigger defaultPopupVisible> - <div className="target" /> - </Trigger>, - ); + render( + <Trigger defaultPopupVisible popup={<span />}> + <div className="target" /> + </Trigger>, + );这不会改变断言逻辑，但能让用例在语义上更一致。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.tsx(4 hunks)tests/basic.test.jsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/basic.test.jsx (2)
docs/examples/simple.tsx (1)
render(193-382)docs/examples/point.tsx (1)
render(34-86)
🪛 GitHub Actions: ✅ test
src/index.tsx
[error] 10-10: useLayoutUpdateEffect is defined but never used. (no-unused-vars) ESLint: @typescript-eslint/no-unused-vars
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
tests/basic.test.jsx (1)
371-377: children renderProps 用例调整合理这里将
Trigger写成单行并显式传入popupVisible={true}，依然能够让 renderProps 拿到{ open: true }并渲染"true"，测试逻辑清晰且不依赖异步行为，看起来是一次纯风格层面的改动，可以保持。src/index.tsx (1)
309-315: open 状态经internalOpen/mergedOpen统一后逻辑更清晰、行为集中
- 通过
internalOpen与mergedOpen = internalOpen || false把「可能为undefined」的状态统一成布尔值，后续 renderProps、对齐、滚动关闭、ResizeObserver等逻辑只依赖mergedOpen，读起来更直观。internalTriggerOpen内部使用lastTriggerRef只在实际开关状态变化时才触发onOpenChange/onPopupVisibleChange，可以有效避免 pointer + mouse 事件组合触发两次回调的问题。- 结合新增的
defaultPopupVisible测试以及已有的「popupVisible在undefined与false间切换」测试，这一段重构与当前用例预期是一致的，没有明显的行为回归风险。总体来看，这部分状态管理改动可以保留。
Also applies to: 380-392
Summary by CodeRabbit
发布说明
错误修复
测试
✏️ Tip: You can customize this high-level summary in your review settings.