fix: Trigger additional force align for target change #581
Conversation
Walkthrough在 UniqueProvider 中新增两个 useEffect，当 mergedOptions.target 变化时调用 onAlign；其余为格式化调整。测试新增用例验证切换不同 Trigger 的 target 时会触发 onAlign，并添加 afterEach 清理 mock。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant UP as UniqueProvider
participant Eff as useEffect(依赖: mergedOptions.target)
participant Align as onAlign(AlignEngine)
participant Target as Target Element
participant Popup as Popup
App->>UP: 渲染(传入 mergedOptions.target)
UP->>Eff: 注册副作用(依赖 target)
Note over Eff: 初始或 target 变更
Eff->>Align: 调用 onAlign()
Align->>Target: 读取定位信息
Align->>Popup: 计算并更新对齐
Popup-->>App: 渲染已对齐状态
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 resolves an issue where the alignment of components managed by 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #581 +/- ##
==========================================
+ Coverage 95.97% 96.41% +0.43%
==========================================
Files 17 17
Lines 945 948 +3
Branches 278 269 -9
==========================================
+ Hits 907 914 +7
+ Misses 38 34 -4 ☔ 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 correctly identifies the need to re-align the popup when its target changes and implements this using a React.useEffect hook. A corresponding test case has been added to verify this new behavior, which is great. My only suggestion is to make the test's mock implementation more concise for better maintainability. Overall, this is a solid contribution.
| jest.spyOn(useAlignModule, 'default').mockImplementation((...args) => { | ||
| const originalResult = originalUseAlign(...args); | ||
| // Replace onAlign with our mock | ||
| return [ | ||
| originalResult[0], // ready | ||
| originalResult[1], // offsetX | ||
| originalResult[2], // offsetY | ||
| originalResult[3], // offsetR | ||
| originalResult[4], // offsetB | ||
| originalResult[5], // arrowX | ||
| originalResult[6], // arrowY | ||
| originalResult[7], // scaleX | ||
| originalResult[8], // scaleY | ||
| originalResult[9], // alignInfo | ||
| mockOnAlign, // onAlign - our mock function | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
The mock implementation for useAlign is quite verbose. While the comments are helpful, the approach of manually listing each element of the returned array is brittle if the hook's return signature changes. You can make this more concise and maintainable.
jest.spyOn(useAlignModule, 'default').mockImplementation((...args) => {
const result = originalUseAlign(...args);
// The `onAlign` function is the 11th element in the returned array (index 10).
return [...result.slice(0, 10), mockOnAlign];
});
![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 (2)
src/UniqueProvider/index.tsx (1)
150-154: useEffect 依赖与健壮性：建议加入 onAlign 依赖并在无 target 时跳过调用当前未将 onAlign 放入依赖，可能触发 hooks/exhaustive-deps 警告；且当 target 为空时仍调用 onAlign 没必要。建议最小改动如下：
- React.useEffect(() => { - onAlign(); - }, [mergedOptions?.target]); + React.useEffect(() => { + if (!mergedOptions?.target) return; + onAlign(); + }, [mergedOptions?.target, onAlign]);tests/unique.test.tsx (1)
290-366: 对默认导出的 spy 可能未实际接管调用（导入时机/绑定缓存），请验证或改为更稳妥的 mockUniqueProvider 可能在模块加载时已缓存了 useAlign 的函数引用，随后在 it 内对模块 default 的 spy 不一定能替换该引用，导致用例成为“假阳性”或脆弱。建议至少增加一次断言，验证 spy 确实被调用；若失败，请改为在隔离模块中先 mock 再导入被测模块。
最小验证（在本用例内即可落地）：
- const { container } = render(<TestComponent />); + const { container } = render(<TestComponent />); + // 确认我们的 spy 已接管 useAlign（若此处失败，说明当前 spy 未生效） + expect(useAlignModule.default).toHaveBeenCalled();更稳妥的方案（先 mock 再导入，避免导入时机问题；可将该用例改写为 isolateModules 形式）：
// 伪代码示例：在 it 内部使用 isolateModules，先 doMock useAlign，再 require 被测组件 it('should call onAlign when target changes', async () => { const mockOnAlign = jest.fn(); await jest.isolateModulesAsync(async () => { jest.doMock('../src/hooks/useAlign', () => { const actual = jest.requireActual('../src/hooks/useAlign'); return { __esModule: true, default: (...args: any[]) => { const res = actual.default(...args); // 用 mock 替换返回元组中的 onAlign（索引 10） return [...res.slice(0, 10), mockOnAlign]; }, }; }); const React = require('react'); const { render, fireEvent } = require('@testing-library/react'); const { default: Trigger, UniqueProvider } = require('../src'); const { awaitFakeTimer } = require('./util'); // ... 按现有 TestComponent 逻辑渲染与断言 ... }); expect(mockOnAlign).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/UniqueProvider/index.tsx(2 hunks)tests/unique.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unique.test.tsx (1)
tests/util.tsx (1)
awaitFakeTimer(97-104)
🔇 Additional comments (2)
src/UniqueProvider/index.tsx (1)
25-28: 仅格式化变更，语义不变，OKtests/unique.test.tsx (1)
69-69: 测试隔离性提升，赞：恢复所有 mocks
Summary by CodeRabbit