fix: Default Bytes should only allow numbers#2380
Conversation
harshsbhat
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (2)
87-89: Excellent changes to restrict input to numbers only!The modifications effectively address the issue of allowing only numeric input for the Default Bytes field:
- Adding
type="number"to theInputcomponent restricts user input to numbers only.- Converting the input value to a number in the
onChangehandler ensures that the form state always contains a numeric value.These changes align perfectly with the PR objectives and resolve the issue mentioned in #2359.
Consider adding a
minandmaxattribute to theInputcomponent to match the zod schema constraints:<Input className="max-w-sm" type="number" + min={8} + max={255} autoComplete="off" onChange={(e) => field.onChange(Number(e.target.value))} />This will provide an additional layer of input validation directly in the browser.
Line range hint
52-57: Consider improving the error handling logicThe current implementation prevents submission when the
defaultBytesvalue is unchanged or undefined. While this is a good check, it might be more user-friendly to handle this case differently.Consider the following improvements:
- Move this check to the form validation level by extending the zod schema:
const formSchema = z.object({ // ... other fields defaultBytes: z .number() .min(8, "Byte size needs to be at least 8") .max(255, "Byte size cannot exceed 255") .refine((val) => val !== keyAuth.defaultBytes, { message: "Please provide a different byte-size than the existing one", }) .optional(), });
- Remove the manual check in the
onSubmitfunction:async function onSubmit(values: z.infer<typeof formSchema>) { - if (values.defaultBytes === keyAuth.defaultBytes || !values.defaultBytes) { - return toast.error( - "Please provide a different byte-size than already existing one as default", - ); - } await setDefaultBytes.mutateAsync(values); }These changes will provide more consistent error handling and improve the user experience by showing validation errors directly in the form.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (1)
Line range hint
1-105: Summary of the reviewThe changes implemented in this PR effectively address the issue of allowing only numeric input for the Default Bytes field. The addition of
type="number"and the conversion of input to a number in theonChangehandler ensure that only valid numeric values are accepted and stored.The overall implementation of the
DefaultBytescomponent is solid, with proper form validation using zod and appropriate error handling. Two minor improvements were suggested:
- Adding
minandmaxattributes to theInputcomponent for additional browser-level validation.- Refining the error handling logic for unchanged values by moving the check to the zod schema and simplifying the
onSubmitfunction.These suggestions aim to further enhance the user experience and maintain consistent validation throughout the component.
|
/assign |
|
The /assign command can only be used on issues, not on pull requests. |
|
|
@perkinsjr using This will just make sure that |
|
Awarding harshsbhat: 50 points 🕹️ Well done! Check out your new contribution on oss.gg/harshsbhat |
3 participants
What does this PR do?
It makes sure that the user cannot type in "non number" characters for default bytes
Fixes #2359
Type of change
How should this be tested?
Test A:
Go to API settings -> enter a string as a default byte. It won't allow you to do so.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Improvements