fix: check whether it's undefined #2817
Conversation
chronark
requested review from
perkinsjr,
mcstepp and
ogzhanolguncu
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThe pull request introduces changes to the validation logic in the Changes
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! 🙏 |
![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)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx (1)
111-115: LGTM! Consider enhancing type safety further.The addition of the
undefinedcheck improves the robustness of the validation logic.For even better type safety, consider using a type guard or the nullish coalescing operator:
- if ( - values.refill?.interval !== undefined && - values.refill?.interval !== "none" && - !values.refill?.amount - ) { + if (values.refill?.interval && values.refill.interval !== "none" && !values.refill.amount) {This approach is more concise and ensures that
intervalis both defined and not "none" in a single check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
This adds correct validation to the form, it also fixes an issue where we don't set interval back to undefined if it set to none. So the form is always refilling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
122-125: Use explicit undefined check for more precise logic.The current condition might incorrectly handle edge cases where refillDay is 0. Use an explicit undefined check for more precise control.
Apply this diff to improve the logic:
if (refill?.interval === "daily") { refill.refillDay = undefined; } - if (refill?.interval === "monthly" && !refill.refillDay) { + if (refill?.interval === "monthly" && refill.refillDay === undefined) { refill.refillDay = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
🔇 Additional comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
71-78: LGTM! Well-structured form initialization.The default values are properly structured with appropriate initial states for all fields.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx
Outdated
Show resolved
Hide resolved
perkinsjr
force-pushed
the
update-remaining-form
branch
from
7ee51dc to
ba24aaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
Line range hint
744-748: Fix incorrect error message in metadata section.The metadata card is incorrectly showing rate limit errors. This appears to be copy-pasted error handling that should be specific to metadata validation.
- {form.formState.errors.ratelimit && ( - <p className="text-xs text-center text-content-alert"> - {form.formState.errors.ratelimit.message} - </p> - )} + {form.formState.errors.meta && ( + <p className="text-xs text-center text-content-alert"> + {form.formState.errors.meta.message} + </p> + )}
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
122-125: Add validation for monthly refill day.While the logic for handling refill days is correct, consider adding validation to ensure the refill day is between 1 and 31 when the interval is monthly.
if (refill?.interval === "monthly" && !refill.refillDay) { refill.refillDay = 1; } + if (refill?.interval === "monthly" && (refill.refillDay < 1 || refill.refillDay > 31)) { + form.setError("limit.refill.refillDay", { + type: "manual", + message: "Refill day must be between 1 and 31", + }); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Test Packages / Test ./packages/hono
- 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: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (2)
71-78: Well-structured initialization of the limit configuration!The nested structure provides good type safety and proper initialization of all fields.
528-528: Properly controlled Select component.The removal of defaultValue in favor of value prop aligns with React's controlled component pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
122-124: Consider a more explicit check for the monthly refill day.The condition
!refill.refillDaywould treat 0 as falsy. Consider usingrefill.refillDay === undefinedfor a more explicit check.- if (refill?.interval === "monthly" && !refill.refillDay) { + if (refill?.interval === "monthly" && refill.refillDay === undefined) { refill.refillDay = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Test Packages / Test ./packages/nextjs
- 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 API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (2)
71-78: LGTM! Well-structured default values for the limit configuration.The default values are properly structured with appropriate initial values for optional fields.
526-526: LGTM! Properly implemented controlled component pattern.The redundant defaultValue prop has been removed, and the component is now properly controlled via the value prop.
Summary by CodeRabbit