Conversation
WalkthroughThis pull request introduces a configurable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
commit: |
![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 (2)
src/component/shared.ts (1)
40-40: Consider adding validation forrateLimitPerSecondbounds.The validator accepts any number, including zero, negative values, or extremely large values that could cause issues. In
lib.ts,Math.floor(1000 / rateLimitPerSecond)would result inInfinityfor zero, negative period for negative values, or a 0ms period for very large values.Consider adding bounds validation here or in the client constructor:
- rateLimitPerSecond: v.optional(v.number()), + rateLimitPerSecond: v.optional(v.number()),And validate in the client:
if (rateLimitPerSecond !== undefined && rateLimitPerSecond <= 0) { throw new Error("rateLimitPerSecond must be a positive number"); }src/client/index.ts (1)
238-239: Consider adding input validation in the constructor.This is where invalid values (≤0) should be caught before they propagate to the rate limiter.
rateLimitPerSecond: options?.rateLimitPerSecond ?? defaultConfig.rateLimitPerSecond, }; + if (this.config.rateLimitPerSecond <= 0) { + throw new Error("rateLimitPerSecond must be a positive number"); + } if (options?.onEmailEvent) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)src/client/index.ts(4 hunks)src/component/lib.ts(5 hunks)src/component/setup.test.ts(1 hunks)src/component/shared.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/component/lib.ts (2)
src/component/_generated/api.ts (1)
components(54-55)src/component/shared.ts (2)
RunMutationCtx(173-175)RunQueryCtx(170-172)
🔇 Additional comments (8)
src/component/setup.test.ts (1)
153-159: LGTM!The test configuration correctly includes the new
rateLimitPerSecondfield with the default value of 2, maintaining consistency with the production default configuration.src/component/lib.ts (4)
40-40: LGTM!Good addition of the default constant. This centralizes the default value and provides clear documentation of Resend's default rate limit.
83-84: LGTM!The rate limiter initialization is simplified by removing hardcoded configuration, now relying on dynamic configuration passed to
getDelay.
439-444: Good backward compatibility handling.The fallback to
DEFAULT_RATE_LIMIT_PER_SECONDensures existing database records withoutrateLimitPerSecondcontinue to work correctly during migration.
708-729: Rate limiting implementation is correct and library supports this pattern.The
@convex-dev/rate-limiterv0.3.x fully supports thefixed windowconfig with dynamic period values as implemented here. The code correctly usesMath.max(1, ...)to prevent zero periods, and the dynamic period calculation properly maps rate limits to millisecond intervals (e.g., 100 req/s → 10ms, 2 req/s → 500ms). The configuration withrate: 1and dynamicperiodmatches the documented API exactly, and omitting the optionalcapacityfield allows sensible defaults. No edge case issues.src/client/index.ts (2)
42-51: LGTM!The default configuration correctly includes
rateLimitPerSecond: 2, matching Resend's default rate limit.
98-106: Good documentation.The JSDoc clearly explains the option's purpose, default value, and use case for accounts with higher rate limits.
README.md (1)
166-174: LGTM!Clear documentation with a practical code example. The explanation of the default value and when to change it is helpful for users.
|
Damn, code rabbit loves you
…On Thu, Dec 4, 2025 at 9:33 PM coderabbitai[bot] ***@***.***> wrote:
***@***.***[bot]* commented on this pull request.
*Actionable comments posted: 0*
🧹 Nitpick comments (2)
src/component/shared.ts (1)
40-40: *Consider adding validation for rateLimitPerSecond bounds.*
The validator accepts any number, including zero, negative values, or
extremely large values that could cause issues. In lib.ts, Math.floor(1000
/ rateLimitPerSecond) would result in Infinity for zero, negative period
for negative values, or a 0ms period for very large values.
Consider adding bounds validation here or in the client constructor:
- rateLimitPerSecond: v.optional(v.number()),+ rateLimitPerSecond: v.optional(v.number()),
And validate in the client:
if (rateLimitPerSecond !== undefined && rateLimitPerSecond <= 0) {
throw new Error("rateLimitPerSecond must be a positive number");}
src/client/index.ts (1)
238-239: *Consider adding input validation in the constructor.*
This is where invalid values (≤0) should be caught before they propagate
to the rate limiter.
rateLimitPerSecond:
options?.rateLimitPerSecond ?? defaultConfig.rateLimitPerSecond,
};+ if (this.config.rateLimitPerSecond <= 0) {+ throw new Error("rateLimitPerSecond must be a positive number");+ }
if (options?.onEmailEvent) {
📜 Review details
*Configuration used*: CodeRabbit UI
*Review profile*: CHILL
*Plan*: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 9c380e6
<9c380e6>
and 8981cd1
<8981cd1>
.
📒 Files selected for processing (5)
- README.md (1 hunks)
- src/client/index.ts (4 hunks)
- src/component/lib.ts (5 hunks)
- src/component/setup.test.ts (1 hunks)
- src/component/shared.ts (1 hunks)
🧰 Additional context used 🧬 Code graph analysis (1) src/component/lib.ts
(2)
src/component/_generated/api.ts (1)
- components (54-55)
src/component/shared.ts (2)
- RunMutationCtx (173-175)
- RunQueryCtx (170-172)
🔇 Additional comments (8)
src/component/setup.test.ts (1)
153-159: *LGTM!*
The test configuration correctly includes the new rateLimitPerSecond
field with the default value of 2, maintaining consistency with the
production default configuration.
src/component/lib.ts (4)
40-40: *LGTM!*
Good addition of the default constant. This centralizes the default value
and provides clear documentation of Resend's default rate limit.
------------------------------
83-84: *LGTM!*
The rate limiter initialization is simplified by removing hardcoded
configuration, now relying on dynamic configuration passed to getDelay.
------------------------------
439-444: *Good backward compatibility handling.*
The fallback to DEFAULT_RATE_LIMIT_PER_SECOND ensures existing database
records without rateLimitPerSecond continue to work correctly during
migration.
------------------------------
708-729: *Rate limiting implementation is correct and library supports
this pattern.*
The @convex-dev/rate-limiter v0.3.x fully supports the fixed window
config with dynamic period values as implemented here. The code correctly
uses Math.max(1, ...) to prevent zero periods, and the dynamic period
calculation properly maps rate limits to millisecond intervals (e.g., 100
req/s → 10ms, 2 req/s → 500ms). The configuration with rate: 1 and
dynamic period matches the documented API exactly, and omitting the
optional capacity field allows sensible defaults. No edge case issues.
src/client/index.ts (2)
42-51: *LGTM!*
The default configuration correctly includes rateLimitPerSecond: 2,
matching Resend's default rate limit.
------------------------------
98-106: *Good documentation.*
The JSDoc clearly explains the option's purpose, default value, and use
case for accounts with higher rate limits.
README.md (1)
166-174: *LGTM!*
Clear documentation with a practical code example. The explanation of the
default value and when to change it is helpful for users.
—
Reply to this email directly, view it on GitHub
<#53 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACZQW7IOJKNYYNPDQDFJDL4AEKKFAVCNFSM6AAAAACODWK5G2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNBTGE3TAOBUGY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
| const EMAIL_POOL_SIZE = 4; | ||
| const CALLBACK_POOL_SIZE = 4; |
There was a problem hiding this comment.
These might prove insufficient if folks are trying to get 100RPS - I think a heuristic might be that the workpool can do at most 10x the pool size per second, but could be lower like 4x.. but I think we wait to get asked to raise this, at which point hopefully we can have a shared workpool with different queues for email sending & receiving..
| period: periodMs, | ||
| rate: 1, |
There was a problem hiding this comment.
This will effectively cause serialization of these mutations, making it impossible to do more than ~10 per second, and could get much slower. Let's do something like
const shards = Math.ceil(rateLimitPerSecond / 6);
const config = {
shards,
rate: rateLimitPerSecond,
period: SECOND,
}rationale:
- higher rate is helpful when dividing amongst shards
- this way each shard will have 5-6 capacity per shard, which makes it more likely you'll land on a shard with capacity (esp. with the choose two logic)
- their enforcement period is a second, so let's just leverage that instead of having a tiny period
ianmacartney
left a comment
ianmacartney
left a comment
There was a problem hiding this comment.
needs a bit of work to actually achieve the advertised parallelism
2 participants
Fixes #38
Allows users to specify if Resend has increased their rate limit.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.