Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, 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. Footnotes
|
Walkthrough在 Changes
Estimated code review effort🎯 2 (简单) | ⏱️ ~8 分钟 诗句
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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 a forceUseId parameter to bypass mock ID generation in test environments, which is a useful addition. My review has identified two main areas for improvement. First, the new functionality lacks corresponding tests. It is important to add tests to cover both the modern useId and the legacy useCompatId paths to ensure correctness and prevent regressions. Second, the name forceUseId could be clearer; I've suggested alternatives to improve code readability and maintainability. Addressing these points will strengthen the quality of this contribution.
src/hooks/useId.ts
Outdated
| if (!forceUseId && process.env.NODE_ENV === 'test') { | ||
| return 'test-id'; | ||
| } |
There was a problem hiding this comment.
This new logic is not covered by tests. Please add tests to verify that forceUseId correctly bypasses the mock ID generation in a test environment. This is important to ensure the feature works as expected and to prevent future regressions. The tests should cover both the useId and useCompatId implementations.
src/hooks/useId.ts
Outdated
| export default useOriginId | ||
| ? // Use React `useId` | ||
| function useId(id?: string) { | ||
| function useId(id?: string, forceUseId?: boolean) { |
There was a problem hiding this comment.
The parameter name forceUseId can be a bit confusing. Its purpose is to bypass the mock ID in test environments, but the name might suggest it's forcing the use of the useId hook itself. A more descriptive name like forceActualId or skipMock would improve clarity. This change should be applied consistently to both useId and useCompatId and their usages.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in mechanism to bypass the test-id shortcut in useId, allowing callers to get a “real” generated id even when NODE_ENV === 'test'.
Changes:
- Extend
useId/ compatuseIdto accept an optionalforceUseIdflag. - Skip returning
'test-id'in test env when the flag is enabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
src/hooks/useId.ts
Outdated
| if (!forceUseId && process.env.NODE_ENV === 'test') { | ||
| return 'test-id'; |
src/hooks/useId.ts
Outdated
| function useCompatId(id?: string, forceUseId?: boolean) { | ||
| // Inner id for accessibility usage. Only work in client side |
src/hooks/useId.ts
Outdated
| // By default, test env always return mock id, but also allow force use id for test case which need to test id logic. | ||
| if (!forceUseId && process.env.NODE_ENV === 'test') { |
src/hooks/useId.ts
Outdated
| export default useOriginId | ||
| ? // Use React `useId` | ||
| function useId(id?: string) { | ||
| function useId(id?: string, forceUseId?: boolean) { |
src/hooks/useId.ts
Outdated
| // By default, test env always return mock id, but also allow force use id for test case which need to test id logic. | ||
| if (!forceUseId && process.env.NODE_ENV === 'test') { |
![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)
src/hooks/useId.ts (1)
52-53: 建议补齐forceUseId=true的分支测试Line 52 和 Line 76 新增了
!forceUseId条件，但现有用例主要覆盖默认行为。建议新增测试：在NODE_ENV='test'下传forceUseId=true时，不应返回'test-id'（React 18 分支和 compat 分支都应覆盖）。Also applies to: 76-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useId.ts` around lines 52 - 53, Add tests covering the forceUseId=true branch so that when process.env.NODE_ENV === 'test' and the hook/useId (function useId) is called with forceUseId set to true it does NOT return the special 'test-id' value; add cases for both the React 18 implementation branch and the compat branch to assert a real/generated id is returned (or not equal to 'test-id') and ensure NODE_ENV is set to 'test' only for the test scope and restored afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useId.ts`:
- Line 43: The exposed parameter forceUseId in useId is not being used by
internal callers, causing test runs to reuse the default 'test-id' and collide
keys in ignoredElementMap; update the internal calls to useId inside
useLockFocus (and the other occurrence noted) to explicitly pass true for
forceUseId so each instance generates/uses a unique id for keys stored in
ignoredElementMap, ensuring the new parameter actually takes effect.
---
Nitpick comments:
In `@src/hooks/useId.ts`:
- Around line 52-53: Add tests covering the forceUseId=true branch so that when
process.env.NODE_ENV === 'test' and the hook/useId (function useId) is called
with forceUseId set to true it does NOT return the special 'test-id' value; add
cases for both the React 18 implementation branch and the compat branch to
assert a real/generated id is returned (or not equal to 'test-id') and ensure
NODE_ENV is set to 'test' only for the test scope and restored afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
src/hooks/useId.ts
Outdated
| export default useOriginId | ||
| ? // Use React `useId` | ||
| function useId(id?: string) { | ||
| function useId(id?: string, forceUseId?: boolean) { |
There was a problem hiding this comment.
forceUseId 已暴露但内部关键调用路径未接入，测试环境下仍可能发生 ID 冲突
src/Dom/focus.ts 的 useLockFocus（Line 224）仍是 useId() 调用，测试环境下默认仍会返回 'test-id'，而它在 Line 238 用该值作为 ignoredElementMap 的 key，多个实例会互相覆盖。建议在该内部场景显式传入 true，让新参数真正生效。
建议修复（跨文件）
--- a/src/Dom/focus.ts
+++ b/src/Dom/focus.ts
@@
- const id = useId();
+ const id = useId(undefined, true);Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useId.ts` at line 43, The exposed parameter forceUseId in useId is
not being used by internal callers, causing test runs to reuse the default
'test-id' and collide keys in ignoredElementMap; update the internal calls to
useId inside useLockFocus (and the other occurrence noted) to explicitly pass
true for forceUseId so each instance generates/uses a unique id for keys stored
in ignoredElementMap, ensuring the new parameter actually takes effect.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/hooks-17.test.tsx (1)
60-72: 建议强化断言，直接覆盖“唯一 ID”目标。Line 70 和 Line 71 目前只校验“不是
test-id”，若回归为其它固定值（例如同一个常量）也会误通过。建议一次渲染两个DemoForce，再断言两个 id 不相等，这样更贴合本次修复“避免 id 冲突”的目标。可参考的断言增强
it('force use compat id in test env', () => { const DemoForce: React.FC<{ id?: string }> = ({ id }) => { const mergedId = useId(id, true); return <div id={mergedId} className="target-force" />; }; resetUuid(); - const { container } = render(<DemoForce />); - const ele = container.querySelector('.target-force'); - - expect(ele.id).toBeTruthy(); - expect(ele.id).not.toEqual('test-id'); + const { container } = render( + <> + <DemoForce /> + <DemoForce /> + </>, + ); + const elements = container.querySelectorAll('.target-force'); + + expect(elements[0].id).toBeTruthy(); + expect(elements[0].id).not.toEqual('test-id'); + expect(elements[1].id).toBeTruthy(); + expect(elements[1].id).not.toEqual('test-id'); + expect(elements[0].id).not.toEqual(elements[1].id); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks-17.test.tsx` around lines 60 - 72, The test currently only asserts the generated id is not 'test-id' which can miss regressions; update the test for DemoForce (which uses useId(id, true)) to render two instances (after resetUuid()), query both elements via '.target-force', assert each element has a truthy id and then assert the two ids are not equal to guarantee uniqueness; keep the existing resetUuid() call and retain the force-compat usage by calling useId with true.tests/hooks.test.tsx (1)
645-655: 建议把断言从“非 test-id”升级为“唯一且非 test-id”。Line 653 和 Line 654 的覆盖面偏弱：如果实现错误地返回固定字符串（但不是
test-id），测试仍会通过。建议和上面的 compat 场景一致，验证两个实例的 id 不相等。可参考的断言增强
it('force use react useId in test env', () => { const DemoForce: React.FC<Readonly<{ id?: string }>> = ({ id }) => { const mergedId = useId(id, true); return <div id={mergedId} className="target-force" />; }; - const { container } = render(<DemoForce />); - const ele = container.querySelector('.target-force'); - expect(ele.id).toBeTruthy(); - expect(ele.id).not.toEqual('test-id'); + const { container } = render( + <> + <DemoForce /> + <DemoForce /> + </>, + ); + const elements = container.querySelectorAll('.target-force'); + expect(elements[0].id).toBeTruthy(); + expect(elements[0].id).not.toEqual('test-id'); + expect(elements[1].id).toBeTruthy(); + expect(elements[1].id).not.toEqual('test-id'); + expect(elements[0].id).not.toEqual(elements[1].id); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks.test.tsx` around lines 645 - 655, The test currently only asserts the generated id is truthy and not equal to 'test-id'; update the DemoForce/useId test to create two rendered instances (e.g., render two <DemoForce /> or render twice) and assert both elements have ids that are non-empty, not equal to 'test-id', and not equal to each other to ensure uniqueness; locate the DemoForce component and the assertions around useId and replace the single-instance checks with the pairwise uniqueness checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/hooks-17.test.tsx`:
- Around line 60-72: The test currently only asserts the generated id is not
'test-id' which can miss regressions; update the test for DemoForce (which uses
useId(id, true)) to render two instances (after resetUuid()), query both
elements via '.target-force', assert each element has a truthy id and then
assert the two ids are not equal to guarantee uniqueness; keep the existing
resetUuid() call and retain the force-compat usage by calling useId with true.
In `@tests/hooks.test.tsx`:
- Around line 645-655: The test currently only asserts the generated id is
truthy and not equal to 'test-id'; update the DemoForce/useId test to create two
rendered instances (e.g., render two <DemoForce /> or render twice) and assert
both elements have ids that are non-empty, not equal to 'test-id', and not equal
to each other to ensure uniqueness; locate the DemoForce component and the
assertions around useId and replace the single-instance checks with the pairwise
uniqueness checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7a4ab77-eda8-4958-8d9e-c3d123935e96
📒 Files selected for processing (3)
src/hooks/useId.tstests/hooks-17.test.tsxtests/hooks.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useId.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #741 +/- ##
=======================================
Coverage 86.20% 86.20%
=======================================
Files 38 38
Lines 1044 1044
Branches 385 385
=======================================
Hits 900 900
Misses 142 142
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
forceUseIdcapability touseIdso callers can opt out of the fixed'test-id'behavior in test env.useId()in test env still returns'test-id') to avoid breaking current snapshots.Motivation
Recent upstream changes in Portal (global keydown listener + stack dedupe by id) make some nested-portal tests sensitive to id collisions.
When every hook call returns
'test-id'in test env, multiple portals can share the same id in test runtime, which may hide or distort real behavior in stack-based logic.forceUseIdis introduced as an opt-in escape hatch for these specific scenarios:Backward Compatibility
forceUseId === true.Tests
Added/updated tests to verify:
useIdpath: in test env withforceUseId=true, id is not'test-id'.useId): in test env withforceUseId=true, id is not'test-id'.Summary by CodeRabbit
发布说明
新特性
测试