fix: trim spaces in strings for names #2527
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
…s://github.com/unkeyed/unkey into eng-1518-i-can-create-a-new-api-without-a-name
MichaelUnkey
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements to the validation logic across multiple components within the dashboard application. Key modifications include the introduction of trimming and minimum length requirements for various fields in form schemas, ensuring that inputs are not just whitespace and meet specified character limits. These changes apply to components such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (13)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (1)
Line range hint
1-95: Consider improving user feedback and error handlingWhile the validation change is a good improvement, consider enhancing the overall user experience:
The current implementation doesn't provide real-time feedback on input validity. Consider adding error messages directly in the form, perhaps using the
FormMessagecomponent from your UI library.The
onSubmitfunction's error handling could be more specific. Instead of a generic "Please provide a valid name" message, it could explain the exact requirements (e.g., "Name must be 3-50 characters and contain only letters, numbers, underscores, hyphens, and periods").The component doesn't handle the case where the new name might already exist. Consider adding a uniqueness check if applicable.
To improve the component's robustness and user-friendliness, consider implementing these suggestions. If you'd like assistance in drafting these improvements, please let me know.
apps/dashboard/app/(app)/apis/[apiId]/settings/update-api-name.tsx (2)
21-24: Improved input validation for API name.The changes enhance the validation for the 'name' field, aligning with the PR objectives. The minimum length requirement and whitespace trimming effectively prevent empty or space-only names.
Consider adding a maximum length constraint to prevent excessively long API names. For example:
name: z .string() .min(3, "Name is required and should be at least 3 characters") + .max(50, "Name should not exceed 50 characters") .refine((v) => v.trim()),
Line range hint
55-57: Enhanced submission validation inonSubmitfunction.The additional checks prevent unnecessary API calls and ensure that only valid names are submitted, improving the component's efficiency and reliability.
Consider using the
trim()method when comparing the new name with the current name to handle cases where only whitespace has been added or removed:- if (values.name === api.name || !values.name) { + if (values.name.trim() === api.name.trim() || !values.name.trim()) { return toast.error("Please provide a valid name before saving."); }apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-name.tsx (1)
28-28: Approve changes with a minor suggestion.The added refinement effectively prevents names consisting solely of whitespace, aligning with the PR objectives. This improvement enhances input validation without altering the component's core functionality.
Consider adding a minimum length requirement to further improve validation. For example:
.refine((v) => !v || v.trim().length >= 1, { message: "Name must not be empty when provided", })This ensures that if a name is provided, it contains at least one non-whitespace character.
apps/dashboard/app/(app)/apis/create-api-button.tsx (1)
26-30: LGTM! Consider adding a custom error message for the trim refinement.The changes to the
formSchemaeffectively address the PR objectives by increasing the minimum length requirement and adding a trim refinement. This aligns well with the updates made in other components.Consider adding a custom error message for the trim refinement to provide more specific feedback to the user. For example:
.refine((v) => v.trim().length > 0, { message: "Name cannot consist only of whitespace", })This would give users clearer feedback if they attempt to submit a name consisting only of spaces.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx (2)
27-28: Approve changes with a minor suggestion.The refinement and transformation added to the
ownerIdfield effectively prevent names consisting solely of spaces, aligning with the PR objectives. This approach maintains backwards compatibility while improving input validation.Consider combining the
.refine()and.transform()methods for better readability:ownerId: z .string() .transform((v) => v.trim()) .refine((v) => v !== "", { message: "Owner ID cannot be empty" }) .optional()This suggestion maintains the same functionality while making the intent clearer.
Line range hint
1-115: Approve overall implementation with a suggestion for improved error messaging.The changes to the
formSchemaeffectively improve input validation without impacting the overall component functionality. The existing error handling and UI feedback mechanisms are sufficient for the new validation rules.Consider adding a custom error message to the
.refine()method to provide more specific feedback to the user:ownerId: z .string() .refine((v) => (v ? v.trim() : undefined), { message: "Owner ID cannot consist only of whitespace" }) .transform((e) => (e === "" ? undefined : e)) .optional(),This will enhance the user experience by providing clearer error messages when input validation fails.
apps/dashboard/app/new/create-workspace.tsx (2)
Line range hint
71-72: Ensure trimmed name value is used when creating the workspace.Currently, the form submission doesn't explicitly trim the name value before sending it to the server. To maintain consistency between validation and stored data, consider modifying the form submission as follows:
onSubmit={form.handleSubmit((values) => createWorkspace.mutate({ ...values, name: values.name.trim() }))}This change ensures that the trimmed name is used when creating the workspace, aligning with the validation logic.
Line range hint
86-86: Update form description to reflect trimming behavior.To improve user understanding and align with the PR objective, consider updating the form description for the name field. Change the following line:
- <FormDescription>What should your workspace be called?</FormDescription> + <FormDescription>What should your workspace be called? Leading and trailing spaces will be removed.</FormDescription>This change informs users about the trimming behavior, potentially reducing confusion and improving the user experience.
apps/dashboard/app/new/create-api.tsx (1)
Line range hint
93-106: Consider adding real-time feedback for input lengthTo improve user experience, consider adding real-time feedback on the input length after trimming. This will help users understand why their input might be rejected even if it visually appears to meet the length requirement.
You can modify the
FormDescriptioncomponent to include this feedback:<FormDescription> <p>What should your api be called?</p> <p>This is just for you, and will not be visible to your customers</p> + <p>Current length: {field.value.trim().length} (min: 3, max: 50)</p> </FormDescription>This change will provide users with immediate feedback on their input length, helping them understand and correct any issues before submission.
apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (1)
40-46: Improved validation for the role name field.The changes enhance the input validation for the role name, which aligns well with the PR objectives. The minimum length requirement and the regex pattern help ensure consistent and meaningful role names.
Consider updating the error message to explicitly mention that spaces are not allowed:
- "Must be at least 3 characters long and only contain alphanumeric, colons, periods, dashes and underscores", + "Must be at least 3 characters long and only contain alphanumeric characters, colons, periods, dashes, and underscores (no spaces allowed)",This change would provide clearer guidance to users about the exact requirements for the role name.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/create-new-override.tsx (1)
31-35: Approved: Improved validation for theidentifierfieldThe changes effectively address the PR objective by preventing names consisting solely of spaces and enhancing the overall validation:
- Increased minimum length requirement from 2 to 3 characters.
- Added refinement to trim whitespace and ensure non-empty strings.
- Updated error message to reflect the new minimum length requirement.
These improvements align with the broader effort to enhance input validation across the application.
Consider updating the error message to explicitly mention the trimming of whitespace:
.min(3, "Name is required, should be at least 3 characters, and cannot consist only of whitespace")This change would provide users with more precise feedback on the validation requirements.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
Line range hint
1-1000: Overall, the changes effectively address the whitespace issue.The modifications to the form schema validation logic successfully prevent names consisting solely of spaces, addressing the main objective of this PR. The changes are focused and minimal, which reduces the risk of unintended side effects.
For consistency, consider applying similar validation to other optional string fields in the form, such as
environment, if appropriate for your use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-name.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/settings/update-api-name.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/create-api-button.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (1 hunks)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/create-new-override.tsx (1 hunks)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (1 hunks)
- apps/dashboard/app/new/create-api.tsx (1 hunks)
- apps/dashboard/app/new/create-workspace.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
apps/dashboard/app/(app)/apis/create-api-button.tsx (1)
Line range hint
32-100: Verify integration of new validation rulesThe changes to the
formSchemaare well-integrated into the existingCreateApiButtoncomponent. The component structure and logic remain unchanged, which is appropriate given the nature of the modifications.To ensure the new validation rules are being applied correctly, please run the following test cases:
- Attempt to submit the form with a 2-character name (should fail).
- Attempt to submit the form with a name consisting only of spaces (should fail).
- Attempt to submit the form with a valid name with leading/trailing spaces (should pass, but trim the spaces).
You can use the following script to search for any hardcoded test cases or snapshots that might need updating:
Please update any affected tests or snapshots to reflect the new validation rules.
apps/dashboard/app/new/create-workspace.tsx (1)
Line range hint
1-114: Summary and Final RecommendationThe changes in this file align with the PR objective of enhancing validation for the workspace name. The addition of the
.refine()method is a good start, but there are opportunities for further improvements:
- Strengthen the name validation to fully prevent names consisting solely of spaces.
- Ensure the trimmed name is used when creating the workspace.
- Update the form description to inform users about the trimming behavior.
These suggestions will help to fully meet the PR objective and improve the overall user experience. Once these changes are implemented, the file should be ready for approval.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
60-69: Improved input validation for prefix, ownerId, and name fields.The addition of
.refine()methods to trim whitespace and ensure non-empty values for theprefix,ownerId, andnamefields is a good improvement. This change aligns with the PR objective of preventing names consisting solely of spaces and enhances the overall input validation.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (1)
21-23: Approve with suggestions: Enhance validation and improve user experienceThe addition of
.trim()to thenamefield validation is a good improvement that prevents submission of names consisting only of whitespace, aligning with the PR objective. However, there are a couple of points to consider:
The trim operation occurs before the
min(3)check. This could lead to valid inputs being rejected. For example, " ab " would be trimmed to "ab" and then fail the minimum length check.The UI doesn't reflect the trimming, which might confuse users.
Consider the following improvements:
- Adjust the minimum length check:
name: z.string().trim().min(1).regex(validCharactersRegex, { message: "Workspace name must contain at least one letter, number, dash, or underscore", })
- Update the UI to reflect the trimmed input:
<Input {...field} className="max-w-sm" defaultValue={workspace.name} onChange={(e) => field.onChange(e.target.value.trim())} />These changes will ensure that the validation is consistent with the user's input and provide a better user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-name.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/settings/update-api-name.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/create-api-button.tsx (1 hunks)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/create-new-override.tsx (1 hunks)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (2 hunks)
- apps/dashboard/app/(app)/ratelimits/create-namespace-button.tsx (1 hunks)
- apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (1 hunks)
- apps/dashboard/app/new/create-api.tsx (1 hunks)
- apps/dashboard/app/new/create-workspace.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-name.tsx
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx
- apps/dashboard/app/(app)/apis/[apiId]/settings/update-api-name.tsx
- apps/dashboard/app/(app)/apis/create-api-button.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/create-new-override.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx
- apps/dashboard/app/new/create-api.tsx
- apps/dashboard/app/new/create-workspace.tsx
🧰 Additional context used
apps/dashboard/app/(app)/ratelimits/create-namespace-button.tsx
Outdated
Show resolved
Hide resolved
…s://github.com/unkeyed/unkey into eng-1518-i-can-create-a-new-api-without-a-name
What does this PR do?
Added extra validation to stop names with only spaces.
Fixes # (issue)
Eng-1518
Type of change
How should this be tested?
Validate changes do not cause issues for some reason.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
name,ownerId, andidentifierrequire a minimum length of 3 characters and cannot consist solely of whitespace.Bug Fixes
Documentation