fix: Stop whitespace in prefixes and fixes validation on bytes #2893
Conversation
Prefix allowed for whitespaces - Refined them - Fixed errors - Removed redudant errors - Made all the errors the same
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request updates the validation logic and form handling in several components. The changes enhance the enforcement of constraints on the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as Form Component
participant V as Validation (Zod)
participant A as API Procedure
U->>F: Submit form data
F->>V: Validate input (prefix/defaultPrefix)
alt Validation passes
F->>A: Invoke API procedure
else Validation fails
V->>F: Return error messages
F->>U: Display error messages
end
Possibly related PRs
Suggested labels
Suggested reviewers
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! 🙏 |
Form validation never worked on our default pages. This fixes that and also fixes white spaces anywhere with an error
perkinsjr
force-pushed
the
fix-prefix
branch
from
bba8452 to
2e67791
Compare
perkinsjr
requested review from
chronark,
mcstepp and
ogzhanolguncu
as code owners
perkinsjr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (1)
93-97: Remove unnecessary empty onBlur handler.The onBlur handler doesn't perform any action and can be removed.
Apply this diff:
<Input className="max-w-sm" {...field} - onBlur={(e) => { - if (e.target.value === "") { - return; - } - }} />apps/dashboard/lib/trpc/routers/key/create.ts (1)
13-19: LGTM! Consider additional edge case validation.The validation changes effectively prevent spaces in prefixes and provide clear error messages. However, consider adding validation to prevent empty strings or strings containing only spaces.
Here's a suggested enhancement:
prefix: z .string() .max(8, { message: "Prefixes cannot be longer than 8 characters" }) + .transform((val) => val.trim()) + .refine((val) => val.length > 0, { + message: "Prefix cannot be empty or contain only spaces", + }) .refine((prefix) => !prefix.includes(" "), { message: "Prefixes cannot contain spaces.", }) .optional(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx(4 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2146
File: apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts:80-80
Timestamp: 2024-11-10T16:45:16.994Z
Learning: Ensure that audit log descriptions accurately reflect the action being performed, such as updating the `defaultPrefix`, and avoid incorrect references like 'name' when not applicable.
🪛 Biome (1.9.4)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx
[error] 85-85: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx
[error] 83-83: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
12-17: LGTM! Validation prevents whitespace in prefixes.The validation logic effectively prevents spaces in prefixes and provides clear error messages.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts (1)
15-20: LGTM! Consistent prefix validation.The validation rules and error messages are consistent with the changes in setDefaultPrefix.ts.
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (1)
71-116: LGTM! Improved form structure.The form structure has been enhanced with proper form components and validation handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: A form label must be associated with an input.
Consider adding a
fororhtmlForattribute to the label element or moving the input element to inside the label element.(lint/a11y/noLabelWithoutControl)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (1)
22-27: LGTM! Robust prefix validation.The validation rules effectively prevent spaces and enforce length limits with clear error messages.
apps/dashboard/lib/trpc/routers/key/create.ts (2)
69-72: LGTM! Integration with newKey function is correct.The validated prefix is properly passed to the newKey function, maintaining consistency with the updated validation rules.
13-19: Implementation aligns perfectly with PR objectives.The validation changes successfully prevent whitespace in prefixes and catch the issue before submission, exactly as specified in the PR objectives.
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx
Outdated
Show resolved
Hide resolved
perkinsjr
force-pushed
the
fix-prefix
branch
from
4cbe284 to
2c2726f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (1)
78-129: Enhance error handling for empty defaultBytes.The form structure looks good, but there's a potential edge case in the validation.
Consider adding a clear error message when the user submits without entering a value:
async function onSubmit(values: z.infer<typeof formSchema>) { - if (values.defaultBytes === keyAuth.defaultBytes || !values.defaultBytes) { + if (!values.defaultBytes) { + return toast.error("Please provide a default byte size"); + } + if (values.defaultBytes === keyAuth.defaultBytes) { return toast.error( "Please provide a different byte-size than already existing one as default" ); } await setDefaultBytes.mutateAsync(values); }apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (2)
107-111: Remove empty onBlur handler.The
onBlurhandler is empty and can be removed.- onBlur={(e) => { - if (e.target.value === "") { - return; - } - }}
124-126: Consolidate loading states.The button has both
disabledbased onisSubmittingand a separateloadingprop.<Button variant="primary" - disabled={!form.formState.isValid || form.formState.isSubmitting} + disabled={!form.formState.isValid} type="submit" loading={form.formState.isSubmitting} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (1)
44-55: LGTM: Form configuration enhances validation and user experience.The form configuration improvements with async resolver, immediate validation (
mode: "all"), and focus management enhance the user experience.apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (1)
31-36: LGTM: Robust prefix validation prevents whitespace.The validation logic effectively prevents spaces in prefixes and enforces the 8-character limit, which aligns with the PR objective.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (1)
38-49: Simplify the resolver configuration.The async wrapper around
zodResolveris unnecessary as it already returns a Promise.Apply this diff to simplify the resolver:
const form = useForm<z.infer<typeof formSchema>>({ - resolver: async (data, context, options) => { - return zodResolver(formSchema)(data, context, options); - }, + resolver: zodResolver(formSchema), mode: "all", shouldFocusError: true, delayError: 100,apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (1)
96-100: Remove empty onBlur handler.The onBlur handler is empty and serves no purpose.
Apply this diff to remove the empty handler:
<Input id="defaultPrefix" className="max-w-sm" {...field} - onBlur={(e) => { - if (e.target.value === "") { - return; - } - }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx(4 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts
- apps/dashboard/lib/trpc/routers/key/create.ts
- apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (2)
11-11: LGTM!The form schema validation is well-defined with appropriate constraints and clear error messages.
Also applies to: 20-27
72-119: LGTM!The form structure is well-organized with:
- Proper accessibility through label-input association
- Correct input validation for numeric values
- Clear error message placement
- Appropriate loading states
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (3)
22-27: LGTM!The schema validation effectively addresses the PR objective by:
- Enforcing max length of 8 characters
- Preventing spaces in prefixes
- Providing clear error messages
39-50: Simplify the resolver configuration.The async wrapper around
zodResolveris unnecessary as it already returns a Promise.Apply this diff to simplify the resolver:
const form = useForm<z.infer<typeof formSchema>>({ - resolver: async (data, context, options) => { - return zodResolver(formSchema)(data, context, options); - }, + resolver: zodResolver(formSchema), mode: "all", shouldFocusError: true, delayError: 100,
70-95: LGTM!The form structure is well-organized with:
- Proper accessibility through label-input association
- Clear error message placement
- Appropriate loading states
Also applies to: 101-121
What does this PR do?
Fixes #2297
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Try updating your prefix with spaces in the middle or end. This should error before submission.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit