fix: restrict Pagination QuickJumper to numeric input only #662
Conversation
|
@cheeseburgertony is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @cheeseburgertony, 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 addresses a usability issue in the pagination component where the QuickJumper and simple mode input fields accepted non-numeric characters. By implementing robust input filtering, the change ensures that only numeric values can be entered, thereby improving the user experience and preventing unexpected behavior when navigating pages. The update includes real-time filtering and intelligent handling of completely filtered inputs. 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
|
![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 effectively restricts user input to numeric characters in the QuickJumper and simple mode pagination inputs, which improves user experience by preventing invalid entries. The implementation correctly filters non-numeric characters and adds comprehensive test cases to validate the new behavior. However, there are a few areas for improvement regarding React best practices and type safety. The changes introduce direct DOM manipulation for controlled components, which is generally an anti-pattern. Additionally, a new helper function has an incorrect return type, which could lead to runtime errors. My review includes suggestions to address these points for better code maintainability and robustness.
src/Pagination.tsx
Outdated
| function getValidValueForFiltered( | ||
| originalValue: string, | ||
| filteredValue: string, | ||
| ): number { |
There was a problem hiding this comment.
The function's return type is declared as number, but getValidValue on line 156 can return an empty string ('') when filteredValue is empty (e.g., when the user clears the input). This creates a type mismatch that could lead to runtime errors, especially with stricter type checking enabled. To accurately reflect the function's behavior and prevent potential issues in consuming code, the return type should be updated.
| ): number { | |
| ): number | '' { |
src/Options.tsx
Outdated
| if (numericValue !== value) { | ||
| e.target.value = numericValue; | ||
| value = numericValue; |
There was a problem hiding this comment.
While directly manipulating e.target.value provides immediate visual feedback to the user, it's generally considered an anti-pattern for controlled components in React. This approach is often a workaround for the issue where React may not trigger a re-render if setGoInputText is called with the same string value, which would leave an invalid character in the input (e.g., typing 'a' after '1').
Although this works, it can make the component's behavior harder to reason about as it creates a potential mismatch between the DOM and React's state. For future improvements, consider exploring solutions that don't require direct DOM manipulation to keep the state as the single source of truth.
src/Pagination.tsx
Outdated
| if (numericValue !== originalValue) { | ||
| inputElement.value = numericValue; | ||
| } |
There was a problem hiding this comment.
While directly manipulating inputElement.value provides immediate feedback, it's an anti-pattern for controlled components in React because it can cause the view to become out of sync with the component's state. This workaround is likely necessary here because the internalInputVal state is a number, but the input can temporarily contain non-numeric characters. If the numeric value doesn't change (e.g., typing 'a' after '1'), setInternalInputVal isn't called, and the invalid character would remain visible without this direct DOM manipulation.
A more robust, idiomatic React solution would involve using a separate string state for the input's value, similar to goInputText in Options.tsx. The input would be controlled by this string state, and you would parse it into a number only when a page change is triggered (e.g., on Enter or blur). This would make the component state the single source of truth and eliminate the need for this workaround.
|
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. 总体说明快速跳转输入框的 handleChange 函数现已验证输入内容，仅接受数字字符。非数字输入被忽略，状态仅在新值匹配纯数字模式时更新。新增测试用例验证此验证行为。 变更清单
代码审查工作量估计🎯 2 (简单) | ⏱️ ~10 分钟
诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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 (1)
src/Pagination.tsx (1)
162-198: 验证实时范围限制的UX体验过滤非数字字符的逻辑正确实现了PR目标。但173-178行的实时范围限制（clamping）行为需要注意：
当用户输入超出有效范围的数字时（如最大页为10时输入"999"），输入框会立即显示限制后的值（"10"），这可能会打断用户的输入流程。虽然这提供了即时反馈，但在某些场景下用户可能期望先完整输入再验证。
建议确认此实时限制行为是否符合产品设计预期。通常的做法是：
- 允许用户输入任意数字（过滤非数字字符）
- 仅在失焦或回车时验证并限制到有效范围
如果当前行为是预期的，建议添加注释说明设计意图，并考虑简化173-178行的逻辑以提高可读性：
- const displayValue = - parsedValue > allPages || (parsedValue < 1 && numericValue !== '') - ? String(Math.min(Math.max(1, parsedValue), allPages)) - : numericValue; + // Clamp display value to valid range for immediate feedback + const needsClamping = parsedValue > allPages || (parsedValue < 1 && numericValue !== ''); + const displayValue = needsClamping + ? String(Math.min(Math.max(1, parsedValue), allPages)) + : numericValue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Pagination.tsx(7 hunks)
🔇 Additional comments (7)
src/Pagination.tsx (7)
93-99: 状态类型转换实现正确将
internalInputVal改为字符串类型是合理的设计，使得可以更灵活地处理用户输入（如保留前导零、处理空值等）。初始化和同步逻辑都正确地使用了String()转换。
134-144: 验证函数实现正确
getValidValue函数正确处理了各种边界情况：
- 空字符串返回当前页，避免意外跳转
- NaN情况返回当前页，防止错误
- 正常值正确地限制在有效页码范围内
这解决了历史评审中提到的返回类型不匹配问题。
200-205: 失焦处理逻辑正确失焦时的验证流程实现合理：
- 通过
getValidValue获取有效页码- 将验证后的值更新回输入框
- 触发页面变更
这确保了用户在输入完成后总是看到并跳转到有效的页码。
213-213: 类型转换一致正确地将
nextCurrent转换为字符串后更新internalInputVal，保持了类型一致性。
229-229: 类型转换一致正确地将
newPage转换为字符串后更新internalInputVal，保持了类型一致性。
310-315: 跳转按钮处理正确
handleGoTO正确地使用字符串类型的internalInputVal，并通过getValidValue转换为有效页码，逻辑清晰正确。
402-404: 只读模式显示简化合理只读模式直接显示
current而非internalInputVal是合理的简化，避免了不必要的状态管理复杂度，且对用户来说语义更清晰。
import { useState } from "react";
export default function NonNegativeIntegerInput() {
const [value, setValue] = useState("");
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const next = e.target.value;
if (/^\d*$/.test(next)) {
setValue(next);
}
};
return (
<input
type="text"
value={value}
onChange={handleChange}
inputMode="numeric" // 移动端数字键盘
placeholder="请输入非负整数"
/>
);
}Try this way. |
cheeseburgertony
force-pushed
the
fix/numeric-only-input
branch
from
5497b81 to
32d1587
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Options.tsx (1)
162-170: 建议添加inputMode="numeric"以改善移动端体验。根据 PR 目标中 afc163 的建议，应该在 input 元素上添加
inputMode="numeric"属性，以便在移动设备上触发数字键盘，提升用户体验。应用以下 diff：
<input disabled={disabled} type="text" value={goInputText} onChange={handleChange} onKeyUp={go} onBlur={handleBlur} aria-label={locale.page} + inputMode="numeric" />基于 PR 目标中的建议。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Options.tsx(1 hunks)tests/jumper.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/jumper.test.tsx
🔇 Additional comments (1)
src/Options.tsx (1)
62-67: 验证逻辑实现正确！使用
/^\d*$/正则表达式在状态更新前进行验证是正确的做法。这种受控组件模式避免了之前评论中提到的直接操作 DOM 的反模式，使状态成为唯一的数据源。实现简洁且易于维护。
Sorry, I made the problem too complicated. I noticed that the input has already been processed in simple mode, so I only modified the handleChange function in the Options.tsx file. |
cheeseburgertony
changed the title
2 participants
🐛 What problem does this PR solve?
Related to ant-design/ant-design#55637
Currently, the QuickJumper page input fields accept any characters (letters, symbols, etc.), which can cause confusion when users accidentally type non-numeric characters.
✨ What is changed and how it works?
Changes:
Behavior:
Before
After
🔗 Related Issues and PR
Summary by CodeRabbit
发布说明