Conversation
MichaelUnkey
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request updates several validation schemas across the API and dashboard codebases. The changes modify the constraint on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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 (3)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-ratelimit.tsx (1)
47-47: Validation updated to allow zero rate limit.The validation for the
ratelimitLimitfield has been changed to accept non-negative values (including zero) instead of strictly positive values. However, the error message still mentions "positive number" while adding "or 0" which could be slightly confusing.Consider updating the error message to be more precise:
- .nonnegative({ message: "This refill limit must be a positive number or 0." }) + .nonnegative({ message: "This refill limit must be a non-negative number." })apps/api/src/routes/v1_keys_createKey.ts (2)
168-168: Update to allow zero-value rate limitsThe validation has been changed from requiring a minimum of 1 to allowing any non-negative integer (including 0) for the rate limit. This change aligns with the PR objective to "fix: allow 0 for ratelimit limit".
While the implementation is correct, consider adding documentation to clarify what a rate limit of 0 means in this context (e.g., does it mean unlimited requests or no requests allowed?).
limit: z.number().int().nonnegative().openapi({ description: "The total amount of requests in a given interval.", + example: 10, + // A rate limit of 0 means [explain behavior here] }),
311-316:Details
❓ Verification inconclusive
Consider aligning validation logic for consistency
The code allows
ratelimit.limitto be 0 but explicitly disallowsremainingto be 0. This inconsistency could be confusing for API users.Consider either:
- Aligning the validation to allow 0 for both properties
- Adding a comment explaining why these properties have different validation rules
🏁 Script executed:
#!/bin/bash # Search for any other instances where similar validation happens for ratelimit properties rg -A 3 "if \(.*ratelimit.*limit.*===.*0\)" --glob "*.ts" rg -A 3 "if \(.*remaining.*===.*0\)" --glob "*.ts"Length of output: 422
Action Required: Clarify/Align Validation for
remainingvsratelimit.limitIt appears that while the code enforces a strict rule for
req.remaining(rejecting a value of 0), there is no similar validation forratelimit.limit(which implicitly allows 0). This inconsistency could lead to confusion for API users regarding what constitutes valid input.Please consider either of the following:
- Align the validation logic: Adjust the code so that both
remainingandratelimit.limitare treated consistently (e.g., allowing 0 values if that’s the intended behavior).- Document the divergence: Add an inline comment explaining why 0 is disallowed for
remainingbut permitted forratelimit.limitto clarify the design intent for future maintainers and API consumers.File:
apps/api/src/routes/v1_keys_createKey.ts(Lines 311-316)if (req.remaining === 0) { throw new UnkeyApiError({ code: "BAD_REQUEST", message: "remaining must be greater than 0.", }); }Please verify that the current behavior is intentional and update either the validation logic or documentation as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/routes/schema.ts(1 hunks)apps/api/src/routes/v1_keys_createKey.ts(1 hunks)apps/api/src/routes/v1_keys_updateKey.ts(1 hunks)apps/api/src/routes/v1_ratelimits_limit.ts(1 hunks)apps/api/src/routes/v1_ratelimits_setOverride.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-ratelimit.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/api/src/routes/v1_ratelimits_setOverride.ts (1)
38-38: Correct implementation for allowing zero as a valid rate limit.The change from
positive()tononnegative()properly allows the rate limit to be set to 0, aligning with the PR objective to fix issue Eng-1661.apps/dashboard/lib/trpc/routers/key/create.ts (1)
42-42: LGTM - Validation properly updated to allow zero rate limits.The schema validation now correctly accepts zero as a valid rate limit value while maintaining type safety.
apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (1)
14-14: Validation correctly updated to allow zero rate limits.The change to use
nonnegative()instead ofpositive()appropriately allows the rate limit to be set to 0, while maintaining the optional nature of the field.apps/api/src/routes/schema.ts (1)
107-107: Schema correctly updated to allow zero rate limits.Good change from
min(1)tononnegative()in the keySchema. This change is consistent with the other files and correctly implements the fix for Eng-1661.apps/api/src/routes/v1_ratelimits_limit.ts (1)
35-35: Validation updated to allow zero rate limit.The validation for the
limitfield now allows zero as a valid value, which is consistent with the PR objective of fixing issue Eng-1661. This change provides greater flexibility by enabling users to set a rate limit of 0, effectively creating a way to block all requests in a namespace when needed.apps/api/src/routes/v1_keys_updateKey.ts (1)
94-94: Validation updated to allow zero rate limit.The validation for the
limitproperty within theratelimitobject now accepts non-negative integers (including zero) instead of requiring strictly positive values. This change is consistent with the PR objective and with similar changes across other route handlers.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-ratelimit.tsx (2)
40-43: Error message updated to reflect new validation constraint.The error message has been updated to indicate that the limit value must be greater than or equal to zero, which aligns with the changed validation logic allowing zero as a valid input.
127-127: UI input correctly configured for new validation.The
minattribute on the Input component is already set to 0, which is consistent with the updated validation constraint allowing zero as a valid value.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts (2)
108-110: Error message updated to reflect new validation constraint.The error message has been updated to indicate that the limit value must be greater than or equal to zero, which aligns with the changed validation logic allowing zero as a valid input.
113-113: Validation updated to allow zero rate limit.The validation for the
limitfield has been changed to accept non-negative values (including zero) instead of strictly positive values. The error message has been appropriately updated to reflect this change.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🛑 Comments failed to post (1)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
42-42: 💡 Verification agent
🧩 Analysis chain
Critical issue: Code change contradicts PR objective
The PR objective is to "fix: allow 0 for ratelimit limit", but this change does the opposite. Changing from
z.number().nonnegative()(which allows 0) toz.number().int().positive()(which requires values > 0) actually prevents using 0 as a valid limit value.If the intent is to allow 0 as stated in the PR objectives, this should be:
- limit: z.number().int().positive(), + limit: z.number().int().nonnegative(),
🏁 Script executed:
#!/bin/bash # Check if PR title or description confirms the intended behavior for ratelimit limit echo "Checking PR description to confirm intent..." gh pr view 2948 --json title,body | jq -r '.title, .body' # Search for related issue Eng-1661 mentioned in PR echo "Searching for related issue..." gh issue view Eng-1661 --json title,body 2>/dev/null || echo "Issue not accessible or found"Length of output: 2774
Action Required: Correct the ratelimit validation to allow 0
The PR is intended to allow a ratelimit limit of 0 or greater, but the current validation using
z.number().int().positive()only permits values greater than 0. To align with the PR objective, please update the validation to usenonnegative().
File:
apps/dashboard/lib/trpc/routers/key/create.ts(Line 42)Issue: The current Zod schema disallows 0, contradicting the intended behavior as confirmed in the PR description.
Proposed diff:
- limit: z.number().int().positive(), + limit: z.number().int().nonnegative(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.limit: z.number().int().nonnegative(),
chronark
deleted the
eng-1661-allow-ratelimit-overrides-to-have-a-limit-of-0-effectively
branch
2 participants
What does this PR do?
Fixes # (issue)
Eng-1661
Type of change
How should this be tested?
Ensure ratelimit limit can be 0 or greater.
Ratelimit still functions as it should.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit