- Notifications
You must be signed in to change notification settings - Fork 191
feat: Support usePropState #673
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.
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
Conversation
Walkthrough在 hooks 中新增 Changes
Sequence Diagram(s)sequenceDiagram autonumber participant C as Component participant H as usePropState participant RS as React State(innerValue) participant LE as useLayoutEffect C->>H: call(defaultStateValue, value?) H->>RS: init innerValue <- defaultStateValue (lazy allowed) H->>H: mergedValue = value ?? innerValue H-->>C: return [mergedValue, setInnerValue] Note over C,H: 受控:value 已定义时以其为准 rect rgba(200,230,255,0.25) C->>LE: value changed after mount LE->>RS: setInnerValue(value) RS-->>H: update innerValue H-->>C: mergedValue reflects value end rect rgba(220,255,220,0.25) C->>H: setInnerValue(next or updater) RS-->>H: update innerValue H-->>C: mergedValue updates (若未受控) end Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File ( |
| The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @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!
此拉取请求引入了一个新的 React Hook usePropState,旨在为 React 18 及更高版本提供一个更精简、性能更优的状态管理方案。它通过移除在 React 18 中不再需要的防御性编程和复杂状态逻辑,简化了 useMergedState 的功能,从而提升了性能,特别是在底层调度和复杂组件场景下。
Highlights
- usePropState 新增: 引入了一个新的 React Hook
usePropState,旨在提供一个更精简、性能更优的状态管理方案。 - useMergedState 弃用: 对于 React 18 及更高版本,
useMergedState已被标记为弃用,并建议使用usePropState。 - 逻辑简化与性能提升: 新的
usePropState移除了onChange和postState逻辑,因为 React 18 后不再需要对卸载组件更新状态进行防御性编程,从而优化了性能。在 MAC M4 环境下,usePropState比useMergedState可节省约 0.01ms 的性能开销。
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review | Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary | Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help | Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
这个 PR 引入了一个新的 usePropState hook,作为 useMergedState 的一个更轻量、更高性能的替代品,适用于 React 18+ 的环境,移除了不常用的 onChange 和 postState 逻辑。这是一个很好的优化。
我在代码审查中发现 usePropState 的实现在处理受控和非受控模式切换时存在一些问题,可能导致状态意外重置或跳变。我提供了一些具体的代码建议来修复这些问题,使其行为更健壮和可预测。此外,相关的单元测试中有一个断言似乎与期望的行为不符,我也提出了修改建议。
| defaultStateValue: T | (() => T), | ||
| value?: T, | ||
| ): [T, Updater<T>] { | ||
| const [innerValue, setInnerValue] = useState<T>(defaultStateValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
innerValue 的初始状态没有考虑到组件从一开始就是受控的情况。它被初始化为 defaultStateValue,但 hook 返回的是 value prop。如果组件之后切换到非受控模式,它会回退到这个初始的 defaultStateValue,而不是最后一个有效的 prop 值,这可能会导致意外的 UI 跳变。更好的做法是,如果 value prop 存在,就用它来初始化 state。
const [innerValue, setInnerValue] = useState<T>(() => { if (value !== undefined) { return value; } if (typeof defaultStateValue === 'function') { return (defaultStateValue as () => T)(); } return defaultStateValue; });| useLayoutEffect( | ||
| mount => { | ||
| if (!mount) { | ||
| setInnerValue(value); | ||
| } | ||
| }, | ||
| [value], | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | ||
| rerender(<FC value={undefined} />); | ||
| expect(container.querySelector('input').value).toEqual('test'); | ||
| expect(container.querySelector('.txt').textContent).toEqual(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个断言似乎不正确。val 同时用于 input 的 value 和 span 的内容。如果 input.value 期望是 'test',那么 span 的 textContent 也应该是 'test'。在 usePropState 修复为保留值的行为后,val 会是 'test'。这个断言 toEqual('') 可能是从 useMergedState 的测试中错误地复制过来的,建议将其修改为 toEqual('test') 以反映正确的预期行为。
| expect(container.querySelector('.txt').textContent).toEqual(''); | |
| expect(container.querySelector('.txt').textContent).toEqual('test'); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## master #673 +/- ## ========================================== + Coverage 90.11% 90.23% +0.12% ========================================== Files 36 37 +1 Lines 880 891 +11 Branches 288 297 +9 ========================================== + Hits 793 804 +11 Misses 85 85 Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @gemini-code-assist @copilot @coderabbitai 给一下这个 hook 的更好的命名建议。 |
| 你好 @afc163! 关于
综合来看, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new useControlledState hook as a performance-optimized alternative to useMergedState, leveraging React 18+ capabilities. The hook simplifies state management by removing defensive programming and unused features.
- Adds
useControlledStatehook that eliminatesonChangeandpostStatelogic for better performance (~0.01ms improvement) - Deprecates
useMergedStatewith recommendation to use the new hook for React 18+ environments - Includes comprehensive test coverage for the new hook's controlled/uncontrolled behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/hooks/useControlledState.ts | New hook implementation with simplified state merging logic |
| src/hooks/useMergedState.ts | Adds deprecation notice directing users to the new hook |
| tests/hooks.test.tsx | Comprehensive test suite covering controlled/uncontrolled scenarios and edge cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| | ||
| rerender(<FC value={undefined} />); | ||
| expect(container.querySelector('input').value).toEqual('test'); | ||
| expect(container.querySelector('.txt').textContent).toEqual(''); |
Copilot AI Sep 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion expects an empty string, but based on line 350 the input value remains 'test' when value becomes undefined. The text content should likely be 'test' to match the actual behavior, or there's a bug in the hook implementation.
| expect(container.querySelector('.txt').textContent).toEqual(''); | |
| expect(container.querySelector('.txt').textContent).toEqual('test'); |
| defaultStateValue: T | (() => T), | ||
| value?: T, | ||
| ): [T, Updater<T>] { | ||
| const [innerValue, setInnerValue] = useState<T>(defaultStateValue); |
Copilot AI Sep 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When value is undefined and defaultStateValue is a function, the function will be called on every render instead of only on initialization. This could cause performance issues and unexpected behavior. Consider using a lazy initial state pattern or memoization.
| const [innerValue, setInnerValue] = useState<T>(defaultStateValue); | |
| const [innerValue, setInnerValue] = useState<T>( | |
| typeof defaultStateValue === 'function' | |
| // @ts-ignore | |
| ? () => (defaultStateValue as () => T)() | |
| : () => defaultStateValue | |
| ); |
| @coderabbitai 给一下这个 hook 的更好的命名建议。 |
| Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@afc163 基于我对 推荐命名(按优先级排序)1. |
| Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@afc163 我来分析一下 基于 PR 的描述和目标,这个 hook 的主要特点是:
命名建议:1. |
| Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@afc163 基于您的 推荐命名(按优先级排序)1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/hooks/useMergedState.ts (1)
16-19: 弃用注释措辞改进(更清晰的 React 版本条件)当前文案略显别扭:“if not need support < React 18”。建议调整为更清晰的表述。
- * @deprecated Please use `useControlledState` instead if not need support < React 18. + * @deprecated If you do not need to support React < 18, please use `useControlledState` instead.src/hooks/useControlledState.ts (2)
6-10: 命名建议:更贴近社区心智与 rc- 生态*可选命名(按推荐度排序):
- useControllableState(社区广泛使用,语义直观)
- useControllableValue(rc-* 家族常见“Value”表述,易迁移)
- usePropState(呼应 PR 文案,强调由 props 驱动)
- useMergedPropState(强调合并外部/内部)
如需我批量替换命名并更新测试、文档,可直接提示我生成对应变更。
6-10: JSDoc 英文小修(语法与措辞)更自然的英文表述如下:
/** * Similar to `useState` but will use props value if provided. - * From React 18, we do not need safe `useState` since it will not throw for unmounted update. - * This hooks remove the `onChange` & `postState` logic since we only need basic merged state logic. + * Since React 18, a "safe" `useState` is no longer required because setState after unmount no longer warns. + * This hook removes the `onChange` and `postState` logic, keeping only the basic merged state semantics. */tests/hooks.test.tsx (1)
441-462: 拼写修正:alway -> always(测试名称)建议修正测试标题中的小拼写错误,便于搜索与一致性。
- it('should alway use option value', () => { + it('should always use option value', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/hooks/useControlledState.ts(1 hunks)src/hooks/useMergedState.ts(1 hunks)tests/hooks.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/hooks.test.tsx (1)
src/hooks/useControlledState.ts (1)
useControlledState(11-34)
🪛 Biome (2.1.2)
tests/hooks.test.tsx
[error] 468-468: Unexpected empty array pattern.
(lint/correctness/noEmptyPattern)
🔇 Additional comments (1)
src/hooks/useControlledState.ts (1)
15-18: 核心逻辑 LGTM内部状态与受控值合并逻辑简单直接,默认值懒初始化也符合预期。
| const [] = useControlledState(undefined); | ||
| count += 1; | ||
| return null; | ||
| }; | ||
| | ||
| render(<Demo />); | ||
| expect(count).toBe(1); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修复 Biome 报错:Unexpected empty array pattern
空数组解构会触发 lint/correctness/noEmptyPattern。这里无需接收返回值,直接调用即可。
- const [] = useControlledState(undefined); + useControlledState(undefined);📝 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.
| const [] = useControlledState(undefined); | |
| count += 1; | |
| return null; | |
| }; | |
| render(<Demo />); | |
| expect(count).toBe(1); | |
| }); | |
| useControlledState(undefined); | |
| count += 1; | |
| return null; | |
| }; | |
| render(<Demo />); | |
| expect(count).toBe(1); | |
| }); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 468-468: Unexpected empty array pattern.
(lint/correctness/noEmptyPattern)
🤖 Prompt for AI Agents
In tests/hooks.test.tsx around lines 468 to 475, the line using an empty array destructuring "const [] = useControlledState(undefined);" triggers Biome lint rule noEmptyPattern; remove the empty destructuring and simply call useControlledState(undefined) directly (i.e., replace the const [] = ... with a plain call), so the hook is invoked without assigning its return value. There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/hooks.test.tsx (1)
465-465: 修复空数组模式的静态分析警告静态分析工具检测到空数组解构模式。虽然这在 JavaScript 中是有效的,但可以通过添加注释来抑制警告。
- const [] = usePropState(undefined); + const [] = usePropState(undefined); // eslint-disable-line @typescript-eslint/no-unused-vars或者使用更明确的变量名:
- const [] = usePropState(undefined); + const [_unused] = usePropState(undefined);src/hooks/useMergedState.ts (1)
16-16: 弃用说明英文表述不清,建议润色当前文案有语法问题,且“< React 18”支持条件表述不够自然。建议修改为更清晰的英文说明,便于用户理解迁移条件。
- * @deprecated Please use `usePropState` instead if not need support < React 18. + * @deprecated Prefer `usePropState` if you do not need to support React < 18.src/hooks/usePropState.ts (2)
4-4: 复用 React 内置类型,避免自定义重复定义
Updater<T>与React.Dispatch<React.SetStateAction<T>>等价。为减少重复定义与未来维护成本,建议直接使用 React 内置类型。-import { useState } from 'react'; +import { useState, type Dispatch, type SetStateAction } from 'react'; -type Updater<T> = (updater: T | ((origin: T) => T)) => void; +type Updater<T> = Dispatch<SetStateAction<T>>;
7-10: JSDoc 英文措辞改进(准确性与语法)
- “hooks remove” 单复数不一致;
- React 18 的行为是“不再警告(warn)”,而非“不再抛错(throw)”。
/** - * Similar to `useState` but will use props value if provided. - * From React 18, we do not need safe `useState` since it will not throw for unmounted update. - * This hooks remove the `onChange` & `postState` logic since we only need basic merged state logic. + * Similar to `useState` but prefers the prop `value` when provided. + * Since React 18, unmounted state updates no longer warn, so extra safety guards are unnecessary. + * This hook removes the `onChange` & `postState` logic and keeps a minimal merged-state behavior. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/hooks/useMergedState.ts(1 hunks)src/hooks/usePropState.ts(1 hunks)tests/hooks.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/hooks.test.tsx (1)
src/hooks/usePropState.ts (1)
usePropState(11-34)
🪛 Biome (2.1.2)
tests/hooks.test.tsx
[error] 465-465: Unexpected empty array pattern.
(lint/correctness/noEmptyPattern)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: test / react component workflow
🔇 Additional comments (13)
tests/hooks.test.tsx (9)
11-11: 导入添加正确正确导入新的
usePropState钩子用于测试。
321-339: 测试组件结构符合预期测试组件正确使用了
usePropState的新 API,传递defaultValue ?? null作为第一个参数,value作为第二个参数。
341-350: 控制状态转换测试覆盖全面测试正确验证了从受控状态转为非受控状态时的行为,这是状态管理钩子的关键场景。
352-364: 默认值测试覆盖完整测试覆盖了字符串和函数两种形式的默认值,确保两种情况都能正常工作。
366-384: 依赖稳定性测试重要这个测试确保
setVal函数在依赖数组中时不会引起无限重渲染,这是状态钩子的关键特性。
386-400: React 18 严格模式测试验证了核心改进测试验证了在 React 18 严格模式下,传递
undefined作为value不会重置为undefined,这正是新钩子相比旧钩子的改进之处。
402-436: 状态转换测试逻辑正确测试正确验证了非受控到受控再到非受控的状态转换,包括事件处理和状态更新。
438-459: 受控模式优先级测试重要测试验证了当提供
value时,内部状态更新不会影响显示的值,这确保了受控模式的正确性。
461-472: 渲染次数测试验证性能测试确保钩子只渲染一次,这验证了性能优化的效果。
src/hooks/usePropState.ts (4)
15-16: 初始化逻辑合理(懒初始化保留)直接将
defaultStateValue传入 React 的useState,可在为函数时获得懒初始化语义,符合预期。
17-18: 值合并策略仅以undefined判空,请确认与历史语义一致这里将
null视作“有值”(受控)且仅把undefined视作“空”。若迁移自useMergedState,请确保测试覆盖value=null情形,并确认与既有组件约定一致。建议补充/验证用例要点:
- 受控
value=null时应优先返回null;value从null/具体值切到undefined时的行为(是否重置)符合预期。
28-33: 返回值形态与语义清晰返回
[mergedValue, setInnerValue]符合使用预期,便于直接替换useState。
1-2: 确认 useLayoutEffect 已默认导出且签名为(mount: boolean) => void | VoidFunction,与调用处一致。
| useLayoutEffect( | ||
| mount => { | ||
| if (!mount) { | ||
| setInnerValue(value); | ||
| } | ||
| }, | ||
| [value], | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
严重:当 T 为函数类型时,setInnerValue(value) 会被当作 updater 调用,导致状态错误
React setState 将“函数”参数解释为 updater。若 T 本身是函数(少见但可能,例如存储回调),当前实现会把 value 当 updater 执行。应使用函数式 set 包装,以将函数“作为值”写入。同时可顺便避免相等值的无效更新。
useLayoutEffect( mount => { if (!mount) { - setInnerValue(value); + // 避免函数类型 T 被当作 updater;并跳过相等写入 + setInnerValue(prev => + Object.is(prev, value as T) ? prev : (value as T), + ); } }, [value], );📝 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.
| useLayoutEffect( | |
| mount => { | |
| if (!mount) { | |
| setInnerValue(value); | |
| } | |
| }, | |
| [value], | |
| ); | |
| useLayoutEffect( | |
| mount => { | |
| if (!mount) { | |
| // 避免函数类型 T 被当作 updater;并跳过相等写入 | |
| setInnerValue(prev => | |
| Object.is(prev, value as T) ? prev : (value as T), | |
| ); | |
| } | |
| }, | |
| [value], | |
| ); |
🤖 Prompt for AI Agents
In src/hooks/usePropState.ts around lines 19–26, the useLayoutEffect calls setInnerValue(value) which treats a function-typed T as a state updater; change it to call the functional setter and return the incoming value while avoiding unnecessary updates — e.g. setInnerValue(prev => Object.is(prev, value) ? prev : value) — so function values are stored as values and equal values don’t trigger state updates. | 标题没改,不叫 usePropState |
useState更新 unmount state 不再会有 warning。onChange和postState逻辑,代码中大部分情况都没有使用这两个特性但是却额外消耗了性能。MAC M4 下,usePropState 会比 useMergedState 节约 ~0.01ms 性能。考虑到偏底层调度,对于 Table 嵌套子组件的场景会有一定客观的效果。
Summary by CodeRabbit