refactor(popover): v16适配#3492
Conversation
github-actions
Bot
added
the
action:review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
代码摘要对 Popover 组件进行 v4 重构：新增 变更内容Popover v4 重构
Tour 与 Button 联动更新
估算代码审查工作量🎯 4 (Complex) | ⏱️ ~60 minutes 可能相关的 PR
建议审查者
小诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
升级后
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 110-111: The version specification strategy for `@nutui/icons-react`
and `@nutui/icons-react-taro` is inconsistent: `@nutui/icons-react` uses a caret
range (^3.0.2-beta.6) allowing minor version updates, while
`@nutui/icons-react-taro` uses an exact version (3.0.2-beta.6) with the caret
removed. Align these two icon package dependencies by applying the same
versioning strategy to both, either by adding the caret prefix to
`@nutui/icons-react-taro` to match `@nutui/icons-react`, or removing the caret from
`@nutui/icons-react` to match `@nutui/icons-react-taro`, ensuring consistency across
H5 and Taro icon dependencies.
In `@src/packages/popover/__tests__/popover.spec.tsx`:
- Around line 207-226: The test "should auto close after duration" has two
timer-related issues that need fixing. First, add a try-finally block to ensure
timers are properly cleaned up even if assertions fail early; in the finally
block, call both vi.clearAllTimers() and vi.useRealTimers() to completely reset
the timer state. Second, wrap the vi.advanceTimersByTime() call with the act()
function from React testing library to properly handle React state updates
triggered by timer advancement, which prevents "not wrapped in act" warnings and
potential race conditions.
In `@src/packages/popover/popover.taro.tsx`:
- Around line 188-192: The skew positioning logic in lines 188-192 consistently
uses styles.left for both left and right skew cases, but the arrow positioning
logic in lines 218-228 has already been updated to handle RTL by switching
between left and right sides. Apply the same RTL-aware strategy used in the
arrow positioning section to the content positioning in the skew branch. Instead
of always assigning to styles.left, conditionally assign to either styles.left
or styles.right based on the RTL direction check, ensuring the content anchor
positioning aligns correctly with the arrow positioning in RTL mode.
In `@src/packages/popover/popover.tsx`:
- Around line 210-215: The content positioning logic in the skew === 'left' and
skew === 'right' blocks always assigns to styles.left, but the arrow positioning
code (referenced around lines 240-250) already implements proper RTL handling by
switching between left and right. This inconsistency causes the content
container and arrow anchor to be positioned on different sides in RTL mode.
Apply the same RTL-aware logic to the content positioning by checking the RTL
status and conditionally assigning to either styles.left or styles.right
(similar to how the arrow positioning handles this), ensuring the content
container and arrow anchor remain aligned in both LTR and RTL modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5578985a-6c74-4463-860f-5b9f624b4d81
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
package.jsonsrc/config.jsonsrc/packages/configprovider/types.tssrc/packages/popover/__tests__/popover.spec.tsxsrc/packages/popover/demo.taro.tsxsrc/packages/popover/demo.tsxsrc/packages/popover/demos/h5/demo1.tsxsrc/packages/popover/demos/h5/demo2.tsxsrc/packages/popover/demos/h5/demo3.tsxsrc/packages/popover/demos/h5/demo4-1.tsxsrc/packages/popover/demos/h5/demo4.tsxsrc/packages/popover/demos/h5/demo5.tsxsrc/packages/popover/demos/h5/demo7.tsxsrc/packages/popover/demos/h5/demo8.tsxsrc/packages/popover/demos/taro/demo1.tsxsrc/packages/popover/demos/taro/demo2.tsxsrc/packages/popover/demos/taro/demo3.tsxsrc/packages/popover/demos/taro/demo4-1.tsxsrc/packages/popover/demos/taro/demo4.tsxsrc/packages/popover/demos/taro/demo5.tsxsrc/packages/popover/doc.en-US.mdsrc/packages/popover/doc.mdsrc/packages/popover/doc.taro.mdsrc/packages/popover/doc.zh-TW.mdsrc/packages/popover/popover.scsssrc/packages/popover/popover.taro.tsxsrc/packages/popover/popover.tsxsrc/sites/sites-react/doc/docs/react/migrate-from-v3.en-US.mdsrc/sites/sites-react/doc/docs/react/migrate-from-v3.mdsrc/sites/sites-react/doc/docs/taro/migrate-from-v3.en-US.mdsrc/sites/sites-react/doc/docs/taro/migrate-from-v3.mdsrc/sites/theme/components/header/header.scsssrc/styles/variables-daojia.scsssrc/styles/variables-jmapp.scsssrc/styles/variables-jrkf.scsssrc/styles/variables.scsssrc/types/spec/popover/base.ts
💤 Files with no reviewable changes (8)
- src/packages/popover/demos/h5/demo5.tsx
- src/packages/popover/demos/taro/demo5.tsx
- src/packages/popover/demos/h5/demo4-1.tsx
- src/packages/popover/demos/taro/demo2.tsx
- src/packages/popover/demos/h5/demo8.tsx
- src/packages/popover/demos/taro/demo4-1.tsx
- src/packages/popover/demos/h5/demo7.tsx
- src/packages/popover/demos/h5/demo2.tsx
| "@nutui/icons-react": "^3.0.2-beta.6", | ||
| "@nutui/icons-react-taro": "3.0.2-beta.6", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
版本锁定策略不一致
@nutui/icons-react 保持范围符 ^3.0.2-beta.6（允许小版本更新），而 @nutui/icons-react-taro 改为精确版本 3.0.2-beta.6（移除了 ^）。
这样的不一致可能导致 H5 和 Taro 版本的图标依赖在不同环境中出现版本分歧。请说明为什么需要这样的差异，或统一为相同的版本策略。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 110 - 111, The version specification strategy for
`@nutui/icons-react` and `@nutui/icons-react-taro` is inconsistent:
`@nutui/icons-react` uses a caret range (^3.0.2-beta.6) allowing minor version
updates, while `@nutui/icons-react-taro` uses an exact version (3.0.2-beta.6) with
the caret removed. Align these two icon package dependencies by applying the
same versioning strategy to both, either by adding the caret prefix to
`@nutui/icons-react-taro` to match `@nutui/icons-react`, or removing the caret from
`@nutui/icons-react` to match `@nutui/icons-react-taro`, ensuring consistency across
H5 and Taro icon dependencies.
| test('should auto close after duration', () => { | ||
| vi.useFakeTimers() | ||
| const close = vi.fn() | ||
| const click = vi.fn() | ||
| render( | ||
| <Popover | ||
| visible | ||
| duration={5000} | ||
| list={itemList} | ||
| onClose={close} | ||
| onClick={click} | ||
| > | ||
| <Button type="primary">自动关闭</Button> | ||
| </Popover> | ||
| ) | ||
| vi.advanceTimersByTime(5000) | ||
| expect(close).toBeCalled() | ||
| expect(click).toBeCalled() | ||
| vi.useRealTimers() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file structure
cat -n src/packages/popover/__tests__/popover.spec.tsx | sed -n '207,226p'Repository: jdf2e/nutui-react
Length of output: 1917
🏁 Script executed:
# Also check the Popover component implementation to understand how duration works
fd -t f "popover" --type f -e ts -e tsx src/packages/popover/ | grep -v "spec\|test" | head -5Repository: jdf2e/nutui-react
Length of output: 1917
🏁 Script executed:
# Let's look at the broader test file context
wc -l src/packages/popover/__tests__/popover.spec.tsxRepository: jdf2e/nutui-react
Length of output: 1917
🌐 Web query:
Vitest vi.useFakeTimers try-finally cleanup pattern best practices
💡 Result:
Best-practice cleanup pattern for Vitest fake timers (vi.useFakeTimers) is: always restore real timers in a finally/afterEach, and clear any scheduled timers before switching back. Recommended pattern (per test) try { vi.useFakeTimers //...test code that schedules timers // optionally: vi.advanceTimersByTime(...), vi.runAllTimers, etc. } finally { vi.clearAllTimers vi.useRealTimers } Why this pattern 1) vi.useFakeTimers keeps faking timers until you call vi.useRealTimers. [1] 2) vi.clearAllTimers “removes all timers that are scheduled to run” so they will never run later (prevents leakage into subsequent tests). [1] 3) vi.useRealTimers restores mocked timers back to their original implementations; timers scheduled before switching back are discarded. [2] Notes / variations - If you only need a subset of timer APIs, pass options like vi.useFakeTimers({ toFake: [...] }) or vi.useFakeTimers({ toNotFake: [...] }). [1] - Dates: with fake timers, Date.now is mocked, and to control Date you can use vi.setSystemTime; when done you should restore with vi.useRealTimers in afterEach/cleanup. [3] Example using beforeEach/afterEach instead of per-test try/finally beforeEach( => { vi.useFakeTimers }) afterEach( => { vi.clearAllTimers vi.useRealTimers }) This is consistent with Vitest’s timer APIs: useFakeTimers wraps timer calls until useRealTimers, and clearAllTimers drops queued timers so they don’t affect later tests. [1][2]
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/vi.md
- 2: https://github.com/vitest-dev/vitest/blob/206e8cff/packages/vitest/src/integrations/vi.ts
- 3: https://vitest.dev/guide/mocking/dates
🌐 Web query:
React Testing Library act() wrapping vi.advanceTimersByTime Vitest integration
💡 Result:
When using Vitest fake timers with React Testing Library (RTL), you should generally wrap calls to vi.advanceTimersByTime in act [1][2][3]. This ensures that any React state updates triggered by the timer advancement are processed and flushed, preventing "not wrapped in act" warnings and ensuring your assertions reflect the updated component state [2][3]. Why act is required React processes state updates in batches [4]. When you manually advance timers using vi.advanceTimersByTime, the callbacks associated with those timers (such as setTimeout or setInterval) execute outside of React's standard update cycle [3]. Without act, React may not be aware of the state updates triggered by these callbacks, leading to warnings or race conditions where your assertions run before the component has finished re-rendering [2][3]. Recommended Usage Pattern To correctly advance timers and update the component, wrap the call in act: import { act, render, screen } from '@testing-library/react'; import { vi } from 'vitest'; test('advances timer correctly', => { vi.useFakeTimers; render(); act( => { vi.advanceTimersByTime(1000); }); expect(screen.getByText('Updated')).toBeInTheDocument; }); Handling user-event with Fake Timers If you use user-event in combination with fake timers, it is important to configure it to use your test runner's timer functions [5][6]. This prevents compatibility issues and test timeouts [6]: const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime.bind(vi), }); Best Practices Always restore timers: Use afterEach to clear timers and restore real timers to prevent side effects in other tests [6]. import { vi } from 'vitest'; afterEach( => { vi.runOnlyPendingTimers; vi.useRealTimers; }); Warning about "not wrapped in act": If you continue to see act warnings despite wrapping, ensure that your environment is properly configured for React 18+ and that you are not running into issues with asynchronous timer flushing (where you might need vi.runAllTimersAsync instead) [7][8]. Additionally, some newer versions of testing libraries are working toward better integration to minimize the need for manual act wrapping [9].
Citations:
- 1: https://stackoverflow.com/questions/73609501/cant-test-timer-in-react-using-vitest-jest
- 2: https://stackoverflow.com/questions/78541663/when-do-i-need-to-use-act-when-tests-cause-react-state-updates
- 3: https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning
- 4: why can my code work normally when the code (vi.advanceTimersByTime) is not wrapped in act function? testing-library/react-testing-library#1183
- 5:
userEvent.click()fails when used withvi.useFakeTimers(), all available solutions are not working testing-library/user-event#1115 - 6: https://testing-library.com/docs/using-fake-timers/
- 7: Async fake timers generate "not wrapped in act()" React warnings vitest-dev/vitest#7196
- 8: RTL + Vitest issue with patching asyncWrapper/eventWrapper (with act()) for userEvent APIs testing-library/react-testing-library#1338
- 9: feat: add advanceTimers option testing-library/react-testing-library#1402
在 Line 207-226 增强定时器用例隔离，避免测试串扰
当前测试缺少两个关键的最佳实践：
- 定时器清理不可靠：
vi.useRealTimers()仅在成功路径执行；若断言提前失败，伪计时器会污染后续测试。 - 状态更新未包裹 act()：
vi.advanceTimersByTime()触发的 React 状态更新需要在act()中执行，否则可能出现"not wrapped in act"警告或竞态条件。
建议使用 try-finally 结构并在 finally 块中同时调用 vi.clearAllTimers() 和 vi.useRealTimers() 来完全清理定时器。
建议修改
test('should auto close after duration', () => {
vi.useFakeTimers()
const close = vi.fn()
const click = vi.fn()
- render(
- <Popover
- visible
- duration={5000}
- list={itemList}
- onClose={close}
- onClick={click}
- >
- <Button type="primary">自动关闭</Button>
- </Popover>
- )
- vi.advanceTimersByTime(5000)
- expect(close).toBeCalled()
- expect(click).toBeCalled()
- vi.useRealTimers()
+ try {
+ render(
+ <Popover
+ visible
+ duration={5000}
+ list={itemList}
+ onClose={close}
+ onClick={click}
+ >
+ <Button type="primary">自动关闭</Button>
+ </Popover>
+ )
+ act(() => {
+ vi.advanceTimersByTime(5000)
+ })
+ expect(close).toBeCalled()
+ expect(click).toBeCalled()
+ } finally {
+ vi.clearAllTimers()
+ vi.useRealTimers()
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should auto close after duration', () => { | |
| vi.useFakeTimers() | |
| const close = vi.fn() | |
| const click = vi.fn() | |
| render( | |
| <Popover | |
| visible | |
| duration={5000} | |
| list={itemList} | |
| onClose={close} | |
| onClick={click} | |
| > | |
| <Button type="primary">自动关闭</Button> | |
| </Popover> | |
| ) | |
| vi.advanceTimersByTime(5000) | |
| expect(close).toBeCalled() | |
| expect(click).toBeCalled() | |
| vi.useRealTimers() | |
| }) | |
| test('should auto close after duration', () => { | |
| vi.useFakeTimers() | |
| const close = vi.fn() | |
| const click = vi.fn() | |
| try { | |
| render( | |
| <Popover | |
| visible | |
| duration={5000} | |
| list={itemList} | |
| onClose={close} | |
| onClick={click} | |
| > | |
| <Button type="primary">自动关闭</Button> | |
| </Popover> | |
| ) | |
| act(() => { | |
| vi.advanceTimersByTime(5000) | |
| }) | |
| expect(close).toBeCalled() | |
| expect(click).toBeCalled() | |
| } finally { | |
| vi.clearAllTimers() | |
| vi.useRealTimers() | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/packages/popover/__tests__/popover.spec.tsx` around lines 207 - 226, The
test "should auto close after duration" has two timer-related issues that need
fixing. First, add a try-finally block to ensure timers are properly cleaned up
even if assertions fail early; in the finally block, call both
vi.clearAllTimers() and vi.useRealTimers() to completely reset the timer state.
Second, wrap the vi.advanceTimersByTime() call with the act() function from
React testing library to properly handle React state updates triggered by timer
advancement, which prevents "not wrapped in act" warnings and potential race
conditions.
| styles.left = pxTransform(left + width / 2 - edgeOffset + parallel) | ||
| } | ||
| if (skew === 'right') { | ||
| styles.left = pxTransform(right + parallel) | ||
| styles.left = pxTransform(left + width / 2 + edgeOffset + parallel) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
同步修复 Taro 端 RTL skew 锚点错位。
Line 188-192 的内容定位在 skew 分支固定走 styles.left，而 Line 218-228 的箭头已按 RTL 在左右侧切换。该不一致会在 RTL 下造成箭头与内容对齐偏差，需与 H5 端同策略修复。
Also applies to: 218-228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/packages/popover/popover.taro.tsx` around lines 188 - 192, The skew
positioning logic in lines 188-192 consistently uses styles.left for both left
and right skew cases, but the arrow positioning logic in lines 218-228 has
already been updated to handle RTL by switching between left and right sides.
Apply the same RTL-aware strategy used in the arrow positioning section to the
content positioning in the skew branch. Instead of always assigning to
styles.left, conditionally assign to either styles.left or styles.right based on
the RTL direction check, ensuring the content anchor positioning aligns
correctly with the arrow positioning in RTL mode.
| if (skew === 'left') { | ||
| styles.left = `${left + parallel}px` | ||
| styles.left = `${left + width / 2 - edgeOffset + parallel}px` | ||
| } | ||
| if (skew === 'right') { | ||
| styles.left = `${right + parallel}px` | ||
| styles.left = `${left + width / 2 + edgeOffset + parallel}px` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
修复 RTL 下内容定位与箭头锚点不一致问题。
Line 211-214 在 skew === left/right 时始终写入 styles.left，但 Line 240-250 的箭头定位已经按 RTL 在 left/right 间切换。这样会导致 RTL 场景下内容容器与箭头锚点不在同一侧，出现偏移错位。
建议修复（示意）
- if (skew === 'left') {
- styles.left = `${left + width / 2 - edgeOffset + parallel}px`
- }
- if (skew === 'right') {
- styles.left = `${left + width / 2 + edgeOffset + parallel}px`
- }
+ if (skew === 'left') {
+ styles[dir] = `${wrapperPosition[dir] + width / 2 - edgeOffset + parallel}px`
+ }
+ if (skew === 'right') {
+ styles[dir2] = `${wrapperPosition[dir2] + width / 2 + edgeOffset + parallel}px`
+ }Also applies to: 240-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/packages/popover/popover.tsx` around lines 210 - 215, The content
positioning logic in the skew === 'left' and skew === 'right' blocks always
assigns to styles.left, but the arrow positioning code (referenced around lines
240-250) already implements proper RTL handling by switching between left and
right. This inconsistency causes the content container and arrow anchor to be
positioned on different sides in RTL mode. Apply the same RTL-aware logic to the
content positioning by checking the RTL status and conditionally assigning to
either styles.left or styles.right (similar to how the arrow positioning handles
this), ensuring the content container and arrow anchor remain aligned in both
LTR and RTL modes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat_v4.x #3492 +/- ##
=============================================
+ Coverage 88.33% 88.43% +0.10%
=============================================
Files 295 296 +1
Lines 19747 19818 +71
Branches 3117 3136 +19
=============================================
+ Hits 17443 17527 +84
+ Misses 2298 2285 -13
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
🤔 这个变动的性质是？
🔗 相关 Issue
💡 需求背景和解决方案
☑️ 请求合并前的自查清单
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
Documentation