fix: Fix Popup localization issue when NeedConfirm=false #941
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough新增一个 RangePicker 文档示例（time、needConfirm=false、uncontrolled），并在 Popup 内部新增可选 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as 用户
participant RP as Range/SinglePicker
participant P as Popup
participant DOM as 浏览器布局
U->>RP: 聚焦或切换面板 (activeIndex 变化)
RP->>P: 渲染 Popup(index=activeIndex)
note right of P: useEffect 依赖包含 index
P->>DOM: 读取 wrapper.getBoundingClientRect()
alt rect 无效或越界
P->>P: retryTimes -= (index + 1)
P->>DOM: 延后重算布局并重试
else rect 有效
P->>DOM: 应用定位样式
end
note over RP,P: activeIndex 变化会触发 Popup 重新评估定位
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @qwer5230, 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!
此 PR 旨在解决 Popup 组件在 needConfirm 设置为 false 时出现的特定本地化问题。该修复方案是对 React 18 中 useEffect 行为不一致性的一种规避措施，这种不一致性影响了弹窗定位的重试机制。通过引入输入框索引，组件能够更好地调整其重试行为，从而确保弹窗的正确显示。
Highlights
- 修复 Popup 本地化问题: 修复了 Popup 组件在 needConfirm=false 时出现的本地化问题，该问题与 React 18 的 useEffect 行为有关。
- 引入 index 属性: 为 Popup 组件引入了一个新的 index 属性，用于跟踪当前聚焦的输入框。
- 调整重试逻辑: 修改了 Popup 组件内部 useEffect 钩子中的重试逻辑，通过结合 index 值来兼容/规避 React 18/19 useEffect 的不一致性问题。
- 新增示例: 在 docs/examples/range.tsx 中添加了一个新的 RangePicker 示例，用于展示 needConfirm=false 时的修复效果。
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.
Pull Request Overview
This PR fixes a localization issue in the Popup component when needConfirm is set to false. The fix addresses a React 18 useEffect inconsistency by using the active input index to adjust retry timing logic.
- Passes the active index as a prop from RangePicker to Popup
- Modifies the retry logic to account for different input focus orders
- Adds a test case demonstrating the fix with
needConfirm=false
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/PickerInput/RangePicker.tsx | Passes activeIndex as index prop to Popup component |
| src/PickerInput/Popup/index.tsx | Adds index prop and uses it in retry timing calculation |
| docs/examples/range.tsx | Adds test case for needConfirm=false scenario |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/PickerInput/Popup/index.tsx
Outdated
| @@ -118,7 +119,8 @@ export default function Popup<DateType extends object = any>(props: PopupProps<D | |||
| // Arrow Offset | |||
| const wrapperRect = wrapperRef.current.getBoundingClientRect(); | |||
| if (!wrapperRect.height || wrapperRect.right < 0) { | |||
| setRetryTimes((times) => Math.max(0, times - 1)); | |||
| // Index is designed to be compatible with the bug of inconsistency between React 18 useEffect and React 19 | |||
There was a problem hiding this comment.
The comment mentions React 19 but the description indicates this is addressing React 18 useEffect issues. The comment should be corrected to accurately reflect that this is a workaround for React 18 useEffect behavior.
| // Index is designed to be compatible with the bug of inconsistency between React 18 useEffect and React 19 | |
| // Index is designed to be compatible with the bug of inconsistency in React 18 useEffect behavior |
Copilot uses AI. Check for mistakes.
src/PickerInput/Popup/index.tsx
Outdated
| // Index is designed to be compatible with the bug of inconsistency between React 18 useEffect and React 19 | ||
| setRetryTimes((times) => Math.max(0, times - index - 1)); |
There was a problem hiding this comment.
当前解释此变通方案的注释（“Index is designed to be compatible with the bug of inconsistency between React 18 useEffect and React 19”）有些模糊，并且提到了 React 19，可能会引起困惑。为了提高代码的可维护性，建议提供更详细的说明，解释为什么在 React 18 中 useEffect 的行为不一致，以及为何减去 index 可以解决这个问题。这将帮助未来的开发者更好地理解这段代码的意图。
// 这是一个绕过 React 18 中 useEffect 行为不一致问题的变通方案。
// 在某些情况下（例如切换输入框后），此 useEffect 在 React 18 中可能会比在 React 17 中多执行一次，
// 导致在布局稳定前重试次数就已耗尽。
// 通过减去当前激活的输入框索引 `index` (0 或 1) 来补偿这次额外的执行，
// 确保定位逻辑有足够的时间在 `wrapperRef` 可用后执行。
// 关联 issue: https://github.com/ant-design/ant-design/issues/54885
setRetryTimes((times) => Math.max(0, times - index - 1));
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/PickerInput/Popup/index.tsx (1)
108-111: 重置重试计数的触发条件应包含 index（以及 right 边界）目前仅在
activeInputLeft变化时重置retryTimes=10。切换焦点（left→right）或仅右侧偏移变化时可能不会重置，导致重算预算不足。建议把activeInputRight与index一并纳入依赖。应用如下 diff：
- React.useEffect(() => { - setRetryTimes(10); - }, [activeInputLeft]); + React.useEffect(() => { + setRetryTimes(10); + }, [activeInputLeft, activeInputRight, index]);
🧹 Nitpick comments (4)
src/PickerInput/Popup/index.tsx (2)
191-205: 向子组件透传整个 props 的同时带上新增字段，注意类型漂移
<PopupPanel {...props} />与<Footer {...props} />会拿到index。虽然运行时无碍，但请确保PopupPanelProps与FooterProps接口未对未知属性做限制（例如ExactProps），否则可能在严格项目中触发类型告警。若有顾虑，可改为显式挑选：- <PopupPanel {...props} value={popupPanelValue} /> + <PopupPanel + {...((({ index, ...rest }) => rest)(props))} + value={popupPanelValue} + />（Footer 同理）
Also applies to: 201-205
247-259: 对 wrapper 与箭头的测量与 ResizeObserver 结合良好，但建议为测量失败添加节流连续 10 次同步重渲染可能造成轻微卡顿。可考虑在
retryTimes分支里用requestAnimationFrame包裹一次，以把“读布局→写 state”拉到下一帧，减少布局抖动。src/PickerInput/RangePicker.tsx (1)
614-617: defaultOpenValue 取值与 activeIndex 强相关，建议加越界保护当外部误传
activeIndex为非常规值（如运行时态变更）时，toArray(showTime?.defaultOpenValue)[activeIndex]可能为undefined。可在Popup内部对defaultOpenValue做一次兜底，或这里 clamp 到[0,1]。docs/examples/range.tsx (1)
216-228: 示例建议使用已推荐的 presets API 替代已标记弃用的 ranges文档示例应引导用户使用推荐用法，避免新示例继续依赖
ranges。应用如下 diff：
- <RangePicker<Moment> + <RangePicker<Moment> {...sharedProps} value={undefined} locale={zhCN} needConfirm={false} picker="time" - ranges={{ - test: [moment(), moment().add(1, 'hour')], - }} + presets={[ + { label: 'test', value: [moment(), moment().add(1, 'hour')] }, + ]} />如需，我可以顺便在 docs 中补一段说明文字，解释
needConfirm={false}下的交互与本次修复点。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/examples/range.tsx(1 hunks)src/PickerInput/Popup/index.tsx(4 hunks)src/PickerInput/RangePicker.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
docs/examples/range.tsx (3)
docs/examples/time.tsx (1)
div(17-70)docs/examples/uncontrolled.tsx (1)
div(8-44)src/index.tsx (1)
RangePicker(35-35)
🔇 Additional comments (5)
src/PickerInput/Popup/index.tsx (4)
122-124: 关于按index+1递减的稳健性与可预测性按
index + 1递减会在index=1时每次扣 2，可能过快耗尽重试预算，尤其当上面的重置未发生时更明显。建议：
- 若采纳上条“包含 index 的重置”，这里维持
-1即可，保证每帧稳定推进；- 或至少保证下限，比如当
times <= 1时改为0，以避免负跳变。需要你确认该策略对修复的场景是否仍然稳定。如果需要，我可以补充一个基于 JSDOM 的回归用例来验证不同浏览器/RTL 情况下的布局重算次数。
143-152: 将 index 加入效果依赖是合理的当活动输入切换时强制触发布局重算，符合预期。
219-235: 容器偏移样式写法 LGTMRTL/LTR 下使用动态 key 设置
marginLeft/marginRight清晰且无副作用。
85-87: 上下文读取位置整理小改动 OK只读
prefixCls，无额外副作用。src/PickerInput/RangePicker.tsx (1)
595-596: 转发 activeIndex 给 Popup 的改动合理这能让 Popup 在面板切换时参与布局重算，符合本 PR 目标。
src/PickerInput/Popup/index.tsx
Outdated
| // focus index | ||
| index: number; |
There was a problem hiding this comment.
将新增的 index 改为可选并标注为内部属性，避免破坏性变更
把 index 作为必选公开属性会对所有直接使用 Popup 的调用方造成 TS 编译破坏。建议将其设为可选并标注内部使用，运行时仍保持解构默认值为 0，不影响现有逻辑。
应用如下 diff：
- // focus index
- index: number;
+ /** @internal: active input index for layout retry. Not part of public API. */
+ index?: number;📝 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.
| // focus index | |
| index: number; | |
| /** @internal: active input index for layout retry. Not part of public API. */ | |
| index?: number; |
🤖 Prompt for AI Agents
In src/PickerInput/Popup/index.tsx around lines 37-38, make the newly added
property `index` optional and mark it as internal to avoid TypeScript breaking
changes; change its type to `index?: number` in the props/interface and add an
internal JSDoc comment (e.g. @internal) to indicate it's for internal use only,
while keeping the runtime destructuring default to 0 (e.g. const { index = 0 } =
props) so existing behavior remains unchanged.
…18 中 useEffect 行为的不一致性
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/PickerInput/Popup/index.tsx (1)
37-38: index 设为可选并标注内部用途：已对齐建议，兼容性良好将 index 设为可选并用内部注释标识，避免对外部调用方产生破坏性变更，做得好。
🧹 Nitpick comments (3)
src/PickerInput/Popup/index.tsx (3)
108-111: index 变更时也应重置重试次数activeInputLeft 改变会重置重试计数，但切换左右输入可能只改变 index 而不改变 activeInputLeft。建议一并监听 index，避免未重置导致的布局对齐失败。
- React.useEffect(() => { - setRetryTimes(10); - }, [activeInputLeft]); + React.useEffect(() => { + setRetryTimes(10); + }, [activeInputLeft, index]);
121-129: 重试分支中的注释可再收敛措辞，避免“判定外部为 bug”表述建议改为中性、可维护的“WORKAROUND”说明，聚焦现象与补偿策略，并保留引用链接。
- // This is a workaround to bypass the inconsistent useEffect behavior in React 18. - // When wrapperRef.current.getBoundingClientRect() fails to calculate the position correctly, it enters the retry logic. - // Under normal circumstances, retryTimes - 1 should equal 9, and useEffect would re-execute the side effect if dependencies change. - // However, in React 18, the side effect is no longer re-executed in such cases. - // By subtracting the index of the currently active input field (index, which is either 0 or 1), we compensate for this additional execution. + // WORKAROUND: mitigate layout timing inconsistencies observed under React 18. + // When wrapperRef.current.getBoundingClientRect() is not yet reliable, enter retry logic. + // To compensate extra effect runs, subtract (index + 1) so the retry budget aligns with actual executions. // Related issue: https://github.com/ant-design/ant-design/issues/54885
148-157: 在重试耗尽时尽早退出，避免无效 setState当 retryTimes 已为 0 时继续进入分支会产生一次 no-op setState 调用。可在 effect 顶部短路返回，微幅减少渲染抖动。
React.useEffect(() => { + if (retryTimes <= 0) { + return; + } // `activeOffset` is always align with the active input element
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/PickerInput/Popup/index.tsx(4 hunks)src/PickerInput/SinglePicker.tsx(1 hunks)
🔇 Additional comments (1)
src/PickerInput/SinglePicker.tsx (1)
516-516: 向 Popup 透传 activeIndex：与新重试逻辑对齐，OKSinglePicker 下 activeIndex 恒为 0，传入后不影响行为但保持一致性。
src/PickerInput/Popup/index.tsx
Outdated
| index = 0, | ||
| // Presets |
There was a problem hiding this comment.
🛠️ Refactor suggestion
避免将内部属性 index 透传到子组件/DOM
当前仍以 {...props} 透传给 <PopupPanel> 与 <Footer>，可能让未知 prop（index）一路落到 DOM，触发 React “Unknown prop” 警告或影响 SSR 对齐。建议在本组件消费掉 index，并显式排除后再下发。
在本段解构保持不变的前提下，新增一个“去除 index 的转发 props”，并替换两处传参：
// 紧跟解构之后添加
const forwardProps = React.useMemo(() => {
const { index: _internalIndex, ...rest } = props;
return rest as Omit<PopupProps<DateType>, 'index'>;
}, [props]);然后将下方两处替换为：
- <PopupPanel {...props} value={popupPanelValue} />
+ <PopupPanel {...forwardProps} value={popupPanelValue} />
- <Footer
- {...props}
+ <Footer
+ {...forwardProps}
showNow={multiple ? false : showNow}
invalid={disableSubmit}
onSubmit={onFooterSubmit}
/>🤖 Prompt for AI Agents
In src/PickerInput/Popup/index.tsx around lines 63-64, avoid forwarding the
internal prop `index` to child components: consume `index` locally and create a
memoized `forwardProps` that omits `index` (e.g. using object rest to strip
`index` and casting to the appropriate Omit type), then replace `{...props}`
with `{...forwardProps}` when rendering <PopupPanel> and <Footer> so the
internal `index` does not propagate to DOM/children.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #941 +/- ##
=======================================
Coverage 98.80% 98.80%
=======================================
Files 65 65
Lines 2676 2680 +4
Branches 718 745 +27
=======================================
+ Hits 2644 2648 +4
Misses 29 29
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
关联issue：ant-design/ant-design#54885
通过当前聚焦input的顺序，在进入到重试逻辑时相减的次数不同，来兼容/绕过react 18 useEffect的问题；
本质原因我认为是react 18的原因，但因为我提供不了最小复现路径所以还没有给react 提issue
Summary by CodeRabbit
文档
Bug 修复