Skip to content

feat: force useId #741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yoyo837 wants to merge 5 commits into from
Closed

feat: force useId #741

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6446f2c
feat: forceReactUseId
yoyo837 Mar 18, 2026
fe17cb0
feat: force useId
yoyo837 Mar 18, 2026
5b67b9e
comment
yoyo837 Mar 18, 2026
02f938e
test: cover forceUseId behavior in react18 and react17 paths
yoyo837 Mar 18, 2026
ddecc5e
change name
yoyo837 Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/hooks/useId.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,23 @@ const useOriginId = getUseId();

export default useOriginId
? // Use React `useId`
function useId(id?: string) {
function useId(id?: string, forceUseId?: boolean) {
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

const reactId = useOriginId();

// Developer passed id is single source of truth
if (id) {
return id;
}

// Test env always return mock id
if (process.env.NODE_ENV === 'test') {
// 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') {
return 'test-id';
}
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.


return reactId;
}
: // Use compatible of `useId`
function useCompatId(id?: string) {
function useCompatId(id?: string, forceUseId?: boolean) {
// Inner id for accessibility usage. Only work in client side
const [innerId, setInnerId] = React.useState<string>('ssr-id');

Expand All @@ -72,8 +72,8 @@ export default useOriginId
return id;
}

// Test env always return mock id
if (process.env.NODE_ENV === 'test') {
// 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') {
return 'test-id';
}

Expand Down
Loading