Bug Fix: Fixed field should not allow only whitespace values issue #2318
Conversation
Vinod-Mane3021
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@vinod-techstax is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request enhance the validation process for 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 (
|
|
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
![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-prefix.tsx (2)
56-58: Approved: Good addition of whitespace validationThe new validation check effectively addresses the issue of allowing only whitespace values in the default prefix. The use of
trim()is appropriate, and the error message is clear.For consistency with other error messages in this function, consider using the
toast.error()method instead of returning the result. This would allow the function to continue checking other conditions if needed in the future.Here's a suggested minor refactor for consistency:
- if (values.defaultPrefix.trim() === '') { - return toast.error("Default prefix cannot be empty or contain only spaces."); - } + if (values.defaultPrefix.trim() === '') { + toast.error("Default prefix cannot be empty or contain only spaces."); + return; + }
Line range hint
56-65: Consider refactoring validation logic for better maintainabilityWhile the current implementation works correctly, the multiple validation checks in the
onSubmitfunction could be refactored for better maintainability. Consider extracting these checks into a separate validation function or using Zod's custom refinements.Here's a suggested refactor using Zod's refinements:
const formSchema = z.object({ keyAuthId: z.string(), workspaceId: z.string(), defaultPrefix: z.string() .refine(value => value.trim() !== '', { message: "Default prefix cannot be empty or contain only spaces." }) .refine(value => value.length <= 8, { message: "Default prefix is too long, maximum length is 8 characters." }) }); // In the component: const form = useForm<z.infer<typeof formSchema>>({ resolver: zodResolver(formSchema), // ... other options }); async function onSubmit(values: z.infer<typeof formSchema>) { if (values.defaultPrefix === keyAuth.defaultPrefix) { toast.error("Please provide a different prefix than the already existing one as default"); return; } await setDefaultPrefix.mutateAsync(values); }This approach moves most of the validation logic into the schema definition, making it easier to maintain and modify in the future. It also ensures that these validations are applied consistently, both in the form and during submission.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (1)
Line range hint
1-105: Overall, well-implemented component with good validationThe
DefaultPrefixcomponent is well-structured and implements proper form handling, validation, and error management. The recent changes to prevent whitespace-only inputs effectively address the reported issue.Key strengths:
- Use of React Hook Form with Zod for robust form management and validation.
- Clear separation of concerns between UI components and logic.
- Proper error handling and user feedback through toast notifications.
- Appropriate use of TRPC for API interactions.
The added validation for whitespace-only inputs enhances the overall robustness of the form. With the suggested minor improvements (consistent error handling and potential refactoring of validation logic), this component will be even more maintainable and aligned with best practices.
|
/assign |
|
Assigned to @DetrojaRadhey! Please open a draft PR linking this issue within 48h |
|
prefixes must not include any whitespace at all |
|
I have applied validation for only Do you mean validation for below also |
|
yes,the validation must ensure there is no whitespace in the input field |
|
@Vinod-Mane3021 I am not the maintainer of this repo. But like I commented on the issue. This issue also exists for the
Currently your code only solves it for the default prefix. cc: @chronark |
in addition to that, we must also validate it on the serverside in the trpc handler. |
|
ill make all changes |
|
I have added validation for both
in both client and server side can you review the changes @chronark |
|
Looks good, please sign the CLA and fix the title |
What does this PR do?
Added validation to check the Default Prefix is empty or contain whitespaces using .trim() method.
Fixes #2297
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes