feat(server): ratelimit helpers & plugins - #1183
Conversation
…tash adapter & tests - Add IORedisRatelimiter implementation and full test suite for ioredis adapter. - Normalize RatelimiterLimitResult: rename `reset` -> `resetAtMs` and remove `pending`. - Update Upstash adapter to map Upstash response to the unified result shape and keep invoking waitUtil. - Adjust Upstash tests to expect waitUtil called with a Promise.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an experimental rate-limiting package ( Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Ratelimit Middleware
participant L as Ratelimiter (Memory/Redis/Upstash)
participant H as Handler (business logic)
participant P as Ratelimit Plugin
participant R as HTTP Response
C->>M: Request
M->>M: Resolve limiter & key
alt dedupe enabled
M-->>M: Check per-request dedupe
end
M->>L: limit(key)
alt allowed
L-->>M: {success:true, limit, remaining, reset}
M->>H: Continue with RATELIMIT_MIDDLEWARE_CONTEXT_SYMBOL
H->>P: Response finalization
P->>R: Inject RateLimit-* headers
R-->>C: 200 + RateLimit headers
else denied
L-->>M: {success:false, remaining, reset}
M->>M: Throw TOO_MANY_REQUESTS (with metadata)
P->>R: Add Retry-After when 429 & reset present
R-->>C: 429 + RateLimit + Retry-After
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust and flexible rate limiting solution for oRPC, designed to enhance API stability and prevent abuse. It provides a new experimental package, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-publisher
@orpc/experimental-ratelimit
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new rate limiting package (@orpc/experimental-ratelimit) to the oRPC monorepo, providing flexible rate limiting capabilities with multiple storage backend adapters (memory, Redis, Upstash). The implementation includes middleware for applying rate limits, a handler plugin for adding standard rate limit headers to responses, and comprehensive test coverage.
Key changes:
- New
@orpc/experimental-ratelimitpackage with Memory, Redis, and Upstash adapters - Rate limit middleware with automatic deduplication support
- Handler plugin for automatic rate limit header injection
- Comprehensive documentation and test coverage
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added dependencies for ratelimit package including @upstash/ratelimit and ioredis |
| packages/ratelimit/* | Complete implementation of ratelimit package with adapters, middleware, plugin, types, and tests |
| apps/content/package.json | Added ratelimit package to content app dependencies |
| apps/content/docs/helpers/ratelimit.md | Comprehensive documentation for the ratelimit package |
| apps/content/.vitepress/config.ts | Added ratelimit to documentation navigation |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive rate limiting feature, including helpers and plugins. It adds a new experimental package @orpc/experimental-ratelimit with adapters for in-memory, Redis, and Upstash, along with middleware and a handler plugin for easy integration. The implementation is well-structured with good documentation and extensive tests. I've identified a critical bug in the memory adapter's timestamp cleanup logic and a high-severity issue with context propagation in the middleware. Additionally, there's a minor typo in a comment. After addressing these points, this will be a solid addition to the library.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/ratelimit/tests/e2e.test.ts (1)
8-67: Consider test isolation and additional coverage.The e2e test provides good coverage of the happy path and rate-limiting scenario. Consider these optional enhancements:
- The limiter instance persists across test reruns. For complete isolation, create a fresh limiter per test or reset state.
- Test coverage could be expanded to validate multiple keys/users to ensure per-key isolation.
- The reset time is only checked for type (string), but not validated for reasonable value range.
Example improvement for key isolation testing:
+ // Test different keys are tracked independently + const limiter2 = new MemoryRatelimiter({ + maxRequests: 5, + window: 1000, + }) + const request2 = new Request('https://example.com/login', { + method: 'POST', + body: JSON.stringify({ json: { email: 'other@example.com' } }), + headers: { 'Content-Type': 'application/json' }, + }) + const { response: response2 } = await handler.handle(request2, { + context: { limiter: limiter2 }, + }) + expect(response2?.status).toBe(200)packages/ratelimit/src/adapters/redis.test.ts (1)
173-192: Long timeout for auto-expiration test may mask issues.The 20-second timeout with 1-second polling interval for key expiration is quite generous. If Redis expiration is not working correctly, this test might still pass by waiting unnecessarily long.
Consider reducing the timeout or checking TTL explicitly:
// Wait until the key is auto-expired await vi.waitFor(async () => { const keysAfterExpiry = await redis.keys(`${prefix}${key}`) expect(keysAfterExpiry).toHaveLength(0) - }, { timeout: 20_000, interval: 1000 }) + }, { timeout: 5_000, interval: 500 })Alternatively, check the TTL directly to ensure expiration is configured:
const ttl = await redis.pttl(`${prefix}${key}`) expect(ttl).toBeGreaterThan(0) expect(ttl).toBeLessThanOrEqual(1000)packages/ratelimit/src/handler-plugin.test.ts (1)
1-184: Excellent test coverage!The test suite comprehensively covers all scenarios for the RatelimitHandlerPlugin, including header injection, conditional retry-after logic, edge cases, and context management.
Optional improvement: Consider replacing
anytypes with proper interfaces for better type safety:interface MockOptions { rootInterceptors: Array<(options: any) => Promise<any>> }This is a minor suggestion and doesn't affect functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
apps/content/.vitepress/config.ts(1 hunks)apps/content/docs/helpers/ratelimit.md(1 hunks)apps/content/package.json(1 hunks)packages/ratelimit/.gitignore(1 hunks)packages/ratelimit/README.md(1 hunks)packages/ratelimit/package.json(1 hunks)packages/ratelimit/src/adapters/memory.test.ts(1 hunks)packages/ratelimit/src/adapters/memory.ts(1 hunks)packages/ratelimit/src/adapters/redis.test.ts(1 hunks)packages/ratelimit/src/adapters/redis.ts(1 hunks)packages/ratelimit/src/adapters/upstash-ratelimit.test.ts(1 hunks)packages/ratelimit/src/adapters/upstash-ratelimit.ts(1 hunks)packages/ratelimit/src/handler-plugin.test.ts(1 hunks)packages/ratelimit/src/handler-plugin.ts(1 hunks)packages/ratelimit/src/index.test.ts(1 hunks)packages/ratelimit/src/index.ts(1 hunks)packages/ratelimit/src/middleware.test-d.ts(1 hunks)packages/ratelimit/src/middleware.test.ts(1 hunks)packages/ratelimit/src/middleware.ts(1 hunks)packages/ratelimit/src/types.ts(1 hunks)packages/ratelimit/tests/e2e.test.ts(1 hunks)packages/ratelimit/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
packages/ratelimit/src/middleware.test-d.ts (4)
packages/server/src/builder.ts (1)
os(336-352)packages/ratelimit/src/types.ts (1)
Ratelimiter(20-22)packages/contract/src/schema.ts (1)
type(25-39)packages/ratelimit/src/middleware.ts (1)
createRatelimitMiddleware(45-92)
packages/ratelimit/src/adapters/memory.test.ts (2)
packages/ratelimit/src/adapters/memory.ts (1)
MemoryRatelimiter(24-119)packages/shared/src/index.ts (1)
sleep(22-22)
packages/ratelimit/tests/e2e.test.ts (5)
packages/server/src/builder.ts (2)
os(336-352)handler(273-280)packages/ratelimit/src/types.ts (1)
Ratelimiter(20-22)packages/ratelimit/src/middleware.ts (1)
createRatelimitMiddleware(45-92)packages/ratelimit/src/handler-plugin.ts (1)
RatelimitHandlerPlugin(16-58)packages/ratelimit/src/adapters/memory.ts (1)
MemoryRatelimiter(24-119)
packages/ratelimit/src/middleware.ts (6)
packages/ratelimit/src/types.ts (1)
Ratelimiter(20-22)packages/server/src/context.ts (1)
Context(1-1)packages/shared/src/value.ts (1)
Value(1-1)packages/server/src/middleware.ts (2)
MiddlewareOptions(26-39)Middleware(46-61)packages/ratelimit/src/handler-plugin.ts (2)
RatelimitHandlerPluginContext(7-14)RATELIMIT_HANDLER_CONTEXT_SYMBOL(5-5)packages/shared/src/array.ts (1)
toArray(1-3)
packages/ratelimit/src/handler-plugin.test.ts (3)
packages/ratelimit/src/handler-plugin.ts (2)
RatelimitHandlerPlugin(16-58)RATELIMIT_HANDLER_CONTEXT_SYMBOL(5-5)packages/server/src/adapters/standard/handler.ts (1)
StandardHandler(50-183)packages/server/src/adapters/standard/rpc-matcher.ts (1)
StandardRPCMatcher(24-118)
packages/ratelimit/src/handler-plugin.ts (4)
packages/server/src/context.ts (1)
Context(1-1)packages/ratelimit/src/types.ts (1)
RatelimiterLimitResult(1-18)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(5-8)packages/server/src/adapters/standard/handler.ts (1)
StandardHandlerOptions(27-48)
packages/ratelimit/src/adapters/upstash-ratelimit.test.ts (1)
packages/ratelimit/src/adapters/upstash-ratelimit.ts (1)
UpstashRatelimiter(28-48)
packages/ratelimit/src/middleware.test.ts (5)
packages/ratelimit/src/types.ts (2)
RatelimiterLimitResult(1-18)Ratelimiter(20-22)packages/ratelimit/src/middleware.ts (3)
createRatelimitMiddleware(45-92)RATELIMIT_MIDDLEWARE_CONTEXT_SYMBOL(9-9)RatelimiterMiddlewareContext(11-18)packages/server/src/builder.ts (1)
os(336-352)packages/ratelimit/src/handler-plugin.ts (1)
RATELIMIT_HANDLER_CONTEXT_SYMBOL(5-5)packages/contract/src/schema.ts (1)
type(25-39)
packages/ratelimit/src/adapters/redis.test.ts (1)
packages/ratelimit/src/adapters/redis.ts (2)
RedisRatelimiterOptions(50-77)RedisRatelimiter(79-143)
packages/ratelimit/src/adapters/upstash-ratelimit.ts (1)
packages/ratelimit/src/types.ts (2)
Ratelimiter(20-22)RatelimiterLimitResult(1-18)
packages/ratelimit/src/adapters/memory.ts (1)
packages/ratelimit/src/types.ts (2)
Ratelimiter(20-22)RatelimiterLimitResult(1-18)
packages/ratelimit/src/adapters/redis.ts (3)
packages/ratelimit/src/types.ts (2)
Ratelimiter(20-22)RatelimiterLimitResult(1-18)packages/shared/src/value.ts (1)
fallback(17-19)packages/ratelimit/src/adapters/memory.ts (1)
limit(38-46)
🪛 LanguageTool
apps/content/docs/helpers/ratelimit.md
[grammar] ~123-~123: Use a hyphen to join words.
Context: ...ectly without middleware for custom rate limiting logic: ```ts twoslash import {...
(QB_NEW_EN_HYPHEN)
packages/ratelimit/README.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... your API or implement API contract. - [@orpc/client](https://www.npmjs.com/package/@...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... API on the client with type-safety. - [@orpc/openapi](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...I specs and handle OpenAPI requests. - [@orpc/otel](https://www.npmjs.com/package/@or...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~57-~57: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....io/) integration for observability. - [@orpc/nest](https://www.npmjs.com/package/@or...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... with NestJS. - [@orpc/react](https://www.npmjs.com/package/@o...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...with React and React Server Actions. - [@orpc/tanstack-query](https://www.npmjs.com/p...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~60-~60: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...stack.com/query/latest) integration. - [@orpc/experimental-react-swr](https://www.npm...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ttps://swr.vercel.app/) integration. - [@orpc/vue-colada](https://www.npmjs.com/packa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~62-~62: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ada](https://pinia-colada.esm.dev/). - [@orpc/hey-api](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~63-~63: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...I](https://heyapi.dev/) integration. - [@orpc/zod](https://www.npmjs.com/package/@orp...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~64-~64: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tps://zod.dev/) doesn't support yet. - [@orpc/valibot](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~65-~65: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rom Valibot. - [@orpc/arktype](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~68-~68: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: .../). ## @orpc/experimental-ratelimit Rate Limiting Feature for oRPC ## Sponsors <p align...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: publish-commit
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
packages/ratelimit/.gitignore (1)
1-26: Standard and well‑structured.gitignore.The file follows Node.js/TypeScript best practices with appropriate patterns for:
- Hidden files (with exceptions preserved for
.gitignoreand example files)- Build outputs (
dist/,build/, TypeScript build info)- Test artifacts (
coverage/, vitest temp files)- Development files (
*.local,*.pem)This configuration is solid and aligns well with the package's build tooling and development workflows.
apps/content/package.json (1)
21-21: LGTM!The dependency addition is correctly placed and follows the workspace convention used by other @orpc packages.
packages/ratelimit/src/middleware.test-d.ts (1)
1-44: LGTM!The type test comprehensively validates type inference across the middleware chain, ensuring context, input, and metadata types flow correctly through the
createRatelimitMiddlewareintegration.packages/ratelimit/src/adapters/upstash-ratelimit.ts (1)
28-48: LGTM!The adapter correctly implements the
Ratelimiterinterface and properly handles both blocking and non-blocking rate limit modes. The optionalwaitUntilintegration for edge environments is well-designed.packages/ratelimit/src/handler-plugin.ts (1)
16-58: LGTM!The handler plugin correctly injects standard rate limit headers (
ratelimit-limit,ratelimit-remaining,ratelimit-reset,retry-after) into responses when rate limiting is applied. The retry-after calculation properly converts the reset timestamp to seconds and only applies when the request is rate-limited (status 429).packages/ratelimit/src/index.test.ts (1)
1-3: LGTM!The test correctly verifies that the index module exports the expected middleware function.
apps/content/.vitepress/config.ts (1)
167-167: LGTM!The navigation entry is correctly placed in the Helpers section and follows the existing pattern.
packages/ratelimit/src/index.ts (1)
1-3: LGTM!The barrel export pattern correctly consolidates the public API surface from handler-plugin, middleware, and types modules.
packages/ratelimit/tsconfig.json (1)
1-17: LGTM!The TypeScript configuration is well-structured with appropriate project references and exclusions for a library package.
packages/ratelimit/src/types.ts (1)
1-22: LGTM!The interface design is clean and well-documented. The optional fields provide flexibility for different adapter implementations while maintaining a consistent contract.
packages/ratelimit/src/adapters/redis.test.ts (1)
114-133: Verify timing assertion range.The test expects the blocking call to wait between 500ms and 2000ms, but the window is 2000ms. If the first request was made near the end of a millisecond, the reset could occur almost immediately after the window (e.g., at ~1000ms from the first request).
Consider if the lower bound should account for potential timing variance:
- expect(endTime - startTime).toBeGreaterThanOrEqual(500) // actually waited + expect(endTime - startTime).toBeGreaterThanOrEqual(900) // actually waited close to full windowThis ensures the test validates meaningful blocking behavior rather than edge timing.
packages/ratelimit/src/adapters/upstash-ratelimit.test.ts (1)
44-46: High retry count suggests test instability.The retry count of 5 for the blocking test suggests the test is flaky due to Upstash's rate conditions. Consider if this is acceptable or if the test conditions should be adjusted to be more stable.
Options to improve stability:
- Increase the timeout or window to give Upstash more breathing room
- Add a delay between requests to avoid triggering Upstash's own rate limiting
- Document why retry is needed in a comment
// retry: 5 to handle Upstash's own rate limiting that can affect test timing it('should block when blockingUntilReady is enabled', { retry: 5 }, async () => {packages/ratelimit/src/adapters/memory.test.ts (1)
1-158: Comprehensive test coverage with good practices.The test suite thoroughly validates the MemoryRatelimiter functionality including rate limiting, window expiration, blocking mode, concurrency, and cleanup.
Note: The private property access (lines 145-146, 154-155) is acceptable for testing internal state, though it makes tests dependent on implementation details. This is a reasonable trade-off for validating cleanup behavior.
apps/content/docs/helpers/ratelimit.md (1)
1-229: Excellent documentation with comprehensive coverage!The documentation provides clear guidance on installation, adapter usage, and integration patterns. The code examples are well-structured and cover both common and advanced scenarios.
Note: The static analysis grammar hint about hyphenating "rate limiting" on line 123 can be safely ignored. The phrase "rate limiting logic" is grammatically correct as a noun phrase modifying "logic". Hyphenation would only be needed if it were used as a compound adjective (e.g., "rate-limiting middleware").
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ratelimit/src/adapters/memory.ts (1)
108-120: Consider defensive sleep duration calculation.The blocking loop sleeps until
result.reset, but the durationresult.reset - Date.now()could be negative or very small if time elapses between obtaining the result (line 112) and scheduling the sleep (line 118). While the loop self-corrects quickly, addingMath.max(0, ...)would prevent potential rapid spinning.Apply this diff:
- await new Promise(resolve => setTimeout(resolve, result.reset - Date.now())) + await new Promise(resolve => setTimeout(resolve, Math.max(0, result.reset - Date.now())))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ratelimit/src/adapters/memory.test.ts(1 hunks)packages/ratelimit/src/adapters/memory.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ratelimit/src/adapters/memory.test.ts (2)
packages/ratelimit/src/adapters/memory.ts (1)
MemoryRatelimiter(24-121)packages/shared/src/index.ts (1)
sleep(22-22)
packages/ratelimit/src/adapters/memory.ts (1)
packages/ratelimit/src/types.ts (2)
Ratelimiter(20-22)RatelimiterLimitResult(1-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: publish-commit
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/ratelimit/src/adapters/memory.test.ts (4)
5-68: LGTM! Comprehensive basic rate limiting tests.The helper function and basic test suite thoroughly validate limit enforcement, window expiration, and per-key isolation. The test assertions correctly verify success/failure states, remaining counts, and reset timestamps.
70-108: LGTM! Blocking mode tests validate wait behavior and timeout handling.The tests correctly verify that the limiter blocks until the window resets and respects the configured timeout. The timing assertions account for test environment variability while confirming that actual waiting occurs.
110-139: LGTM! Excellent concurrent request handling test.This test effectively validates race-condition safety by firing multiple concurrent requests across multiple keys in parallel. The assertions confirm correct limit enforcement and consistent remaining counts under concurrent load.
141-201: LGTM! Thorough cleanup verification tests.The cleanup tests effectively exercise both code paths (full and partial expiration) and verify that expired entries are properly removed from the internal store. Accessing private properties for testing purposes is appropriate here and correctly marked with
@ts-expect-error.packages/ratelimit/src/adapters/memory.ts (4)
3-36: LGTM! Clean interface and constructor.The options interface clearly defines the configuration parameters, and the constructor properly initializes all state. The use of readonly fields prevents accidental mutation.
38-46: LGTM! Correctly routes between blocking and non-blocking modes.The public API method appropriately triggers cleanup before evaluating limits and correctly dispatches to blocking or non-blocking implementations based on configuration.
48-68: LGTM! Cleanup logic correctly addresses past feedback.The fixed implementation correctly removes all expired timestamps by finding the first valid (non-expired) entry and splicing everything before it. The once-per-window optimization prevents excessive processing while maintaining reasonable memory hygiene.
70-106: LGTM! Correct sliding window rate limiting implementation.The limit check properly removes expired timestamps, calculates the reset time based on the sliding window (oldest timestamp + window), enforces the maxRequests limit, and records successful requests. The logic is sound and consistent with sliding window semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/ratelimit/src/middleware.ts (1)
80-90: Critical: Preserve existing context when calling next middleware.The current implementation replaces the entire context with a new object containing only the ratelimit symbol. This wipes out all other context values (authentication, database handles, etc.) that were set by upstream middleware. Downstream middleware and handlers will suddenly see missing context, causing runtime failures.
The existing context must be spread first, then the ratelimit symbol added/updated.
Apply this diff to preserve the existing context:
return middlewareOptions.next({ context: { + ...middlewareOptions.context, [RATELIMIT_MIDDLEWARE_CONTEXT_SYMBOL]: { ...middlewareContext, limits: [ ...toArray(middlewareContext?.limits), { limiter, key }, ], }, }, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/content/docs/helpers/ratelimit.md(1 hunks)packages/ratelimit/src/adapters/redis.ts(1 hunks)packages/ratelimit/src/adapters/upstash-ratelimit.ts(1 hunks)packages/ratelimit/src/handler-plugin.ts(1 hunks)packages/ratelimit/src/middleware.ts(1 hunks)packages/ratelimit/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ratelimit/src/adapters/upstash-ratelimit.ts
- packages/ratelimit/src/handler-plugin.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ratelimit/src/adapters/redis.ts (4)
packages/ratelimit/src/types.ts (2)
Ratelimiter(20-22)RatelimiterLimitResult(1-18)packages/shared/src/value.ts (1)
fallback(17-19)packages/ratelimit/src/adapters/upstash-ratelimit.ts (1)
limit(40-47)packages/ratelimit/src/adapters/memory.ts (1)
limit(38-46)
packages/ratelimit/src/middleware.ts (6)
packages/ratelimit/src/types.ts (1)
Ratelimiter(20-22)packages/server/src/context.ts (1)
Context(1-1)packages/shared/src/value.ts (1)
Value(1-1)packages/server/src/middleware.ts (2)
MiddlewareOptions(26-39)Middleware(46-61)packages/ratelimit/src/handler-plugin.ts (2)
RatelimitHandlerPluginContext(7-14)RATELIMIT_HANDLER_CONTEXT_SYMBOL(5-5)packages/shared/src/array.ts (1)
toArray(1-3)
🪛 LanguageTool
apps/content/docs/helpers/ratelimit.md
[uncategorized] ~123-~123: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... directly without middleware for custom rate limiting logic: ```ts twoslash import { MemoryR...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (6)
packages/ratelimit/tsconfig.json (1)
1-18: TypeScript configuration looks good; previous concern addressed.The server package reference (Line 6) has been properly added to the
referencesarray. The configuration correctly extends the root library config, includes all necessary sibling packages (shared, client, server, standard-server), and appropriately excludes test/benchmark artifacts. This resolves the prior flagged concern.packages/ratelimit/src/middleware.ts (5)
1-7: LGTM!Imports are well-organized and all are utilized in the implementation.
9-18: LGTM!The symbol and context interface are correctly defined for storing per-request rate-limit state.
20-43: LGTM!The options interface is well-designed, supporting both static and dynamic limiter/key resolution with clear documentation.
45-61: LGTM!The middleware setup and deduplication logic are correctly implemented. Concurrent resolution with
Promise.allis efficient, and the deduplication check appropriately uses reference equality.
63-78: LGTM!Rate limiting execution and error handling are correctly implemented with appropriate error details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ratelimit/src/adapters/redis.ts (1)
148-148: Clamp the wait duration before sleeping.If
result.resetlands slightly behindDate.now()because of clock jitter, this computes a negative delay and the loop hammers Redis with zero-delay retries. Clamping keeps the wait bounded and avoids tight retry loops.- await new Promise(resolve => setTimeout(resolve, result.reset - Date.now())) + const waitMs = Math.max(result.reset - Date.now(), 0) + await new Promise(resolve => setTimeout(resolve, waitMs))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ratelimit/src/adapters/redis.test.ts(1 hunks)packages/ratelimit/src/adapters/redis.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ratelimit/src/adapters/redis.test.ts (1)
packages/ratelimit/src/adapters/redis.ts (2)
RedisRatelimiterOptions(50-77)RedisRatelimiter(79-151)
packages/ratelimit/src/adapters/redis.ts (4)
packages/ratelimit/src/types.ts (2)
Ratelimiter(20-22)RatelimiterLimitResult(1-18)packages/shared/src/value.ts (1)
fallback(17-19)packages/ratelimit/src/adapters/upstash-ratelimit.ts (1)
limit(40-47)packages/ratelimit/src/adapters/memory.ts (1)
limit(38-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
|
@unnoq I've opened a new pull request, #1186, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/content/docs/helpers/ratelimit.md (1)
159-159: Fix invalid Zod API usage (already flagged in prior review).Line 159 uses the non-existent
z.email()API. This exact issue was flagged in a previous review. The code example will crash if copied.Apply this fix:
- .input(z.object({ email: z.email() })) + .input(z.object({ email: z.string().email() }))packages/ratelimit/src/middleware.ts (1)
86-96: Preserve existing context when calling next middleware.This code replaces the entire context with a new object containing only
RATELIMIT_MIDDLEWARE_CONTEXT_SYMBOL, which wipes out all other context values (auth, database handles, etc.). Downstream middleware and handlers will lose access to the existing context.Apply this diff to preserve the existing context:
return middlewareOptions.next({ context: { + ...middlewareOptions.context, [RATELIMIT_MIDDLEWARE_CONTEXT_SYMBOL]: { ...middlewareContext, limits: [ ...toArray(middlewareContext?.limits), { limiter, key }, ], }, }, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/content/docs/helpers/ratelimit.md(1 hunks)packages/ratelimit/src/handler-plugin.ts(1 hunks)packages/ratelimit/src/middleware.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ratelimit/src/middleware.ts (6)
packages/ratelimit/src/types.ts (1)
Ratelimiter(20-22)packages/server/src/context.ts (1)
Context(1-1)packages/shared/src/value.ts (1)
Value(1-1)packages/server/src/middleware.ts (2)
MiddlewareOptions(26-39)Middleware(46-61)packages/ratelimit/src/handler-plugin.ts (2)
RatelimitHandlerPluginContext(7-14)RATELIMIT_HANDLER_CONTEXT_SYMBOL(5-5)packages/shared/src/array.ts (1)
toArray(1-3)
packages/ratelimit/src/handler-plugin.ts (4)
packages/ratelimit/src/types.ts (1)
RatelimiterLimitResult(1-18)packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(5-8)packages/server/src/adapters/standard/handler.ts (1)
StandardHandlerOptions(27-48)
🪛 LanguageTool
apps/content/docs/helpers/ratelimit.md
[uncategorized] ~123-~123: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... directly without middleware for custom rate limiting logic: ```ts twoslash import { MemoryR...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/ratelimit/src/handler-plugin.test.ts (3)
14-211: Consider extracting common test setup to reduce duplication.Each test follows an identical pattern: creating options, initializing the plugin, creating the handler, and invoking handle. Extracting this into a helper function would improve maintainability and reduce the ~150 lines of repeated boilerplate.
Example helper:
const setupHandlerWithPlugin = ( interceptor: (args: any) => Promise<any> ) => { const options: any = { rootInterceptors: [] } new RatelimitHandlerPlugin().init(options) options.rootInterceptors.push(interceptor) return new StandardHandler({}, new StandardRPCMatcher(), {} as any, options) }Then each test becomes:
it('adds rate limit headers', async () => { const resetTime = Date.now() + 60000 const handler = setupHandlerWithPlugin(async ({ context }: any) => { context[RATELIMIT_HANDLER_CONTEXT_SYMBOL].ratelimitResult = { limit: 100, remaining: 50, reset: resetTime, } return { matched: true as const, response: { status: 200, headers: {}, body: 'ok' }, } }) const result = await handler.handle(createMockRequest('https://example.com/ping'), { context: {} }) // assertions... })
17-17: Consider improving type safety in test setup.While using
anytypes and empty objects is common in tests, adding minimal type definitions would catch potential breaking changes and make tests more robust.Example:
const createMockCodec = () => ({ decode: vi.fn(), encode: vi.fn(), encodeError: vi.fn(), }) const createMockRouter = () => ({}) // Then in tests: const options: Partial<StandardHandlerOptions<any>> = { rootInterceptors: [] } const handler = new StandardHandler( createMockRouter(), new StandardRPCMatcher(), createMockCodec(), options )Also applies to: 33-33
36-37: Consider using vitest assertions instead of throwing errors.The pattern
if (!result.matched) throw new Error(...)works but provides less informative failure messages than vitest's built-in assertions.Apply this pattern:
-if (!result.matched) - throw new Error('request should match') +expect(result.matched).toBe(true)This provides better diff output and integrates with vitest's reporting.
Also applies to: 65-66, 92-93, 117-118, 144-146, 163-164, 186-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ratelimit/src/handler-plugin.test.ts(1 hunks)packages/ratelimit/src/handler-plugin.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ratelimit/src/handler-plugin.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ratelimit/src/handler-plugin.test.ts (3)
packages/ratelimit/src/handler-plugin.ts (2)
RatelimitHandlerPlugin(22-62)RATELIMIT_HANDLER_CONTEXT_SYMBOL(5-5)packages/server/src/adapters/standard/handler.ts (1)
StandardHandler(50-183)packages/server/src/adapters/standard/rpc-matcher.ts (1)
StandardRPCMatcher(24-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: publish-commit
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (2)
packages/ratelimit/src/handler-plugin.test.ts (2)
1-12: LGTM!The imports and mock request helper are well-structured. The helper provides a minimal but sufficient mock for testing the handler plugin's header injection behavior.
14-211: Add explicit test for undefined reset with 429 status and failure condition.The implementation's retry-after logic requires all three conditions:
!success && status === 429 && reset !== undefined. Currently, there's no explicit test covering the case whereresetis undefined whilesuccess: falseandstatus: 429—the retry-after header should not be added in this scenario.Consider adding:
it('does not add retry-after when reset is undefined', async () => { const options: any = { rootInterceptors: [] } new RatelimitHandlerPlugin().init(options) options.rootInterceptors.push(async ({ context }: any) => { context[RATELIMIT_HANDLER_CONTEXT_SYMBOL].ratelimitResult = { success: false, reset: undefined, // explicitly undefined } return { matched: true as const, response: { status: 429, headers: {}, body: 'too many requests' }, } }) const handler = new StandardHandler({}, new StandardRPCMatcher(), {} as any, options) const result = await handler.handle(createMockRequest('https://example.com/ping'), { context: {} }) expect(result.response.headers['retry-after']).toBeUndefined() })
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes: #910
Summary by CodeRabbit
New Features
Documentation
Tests
Chores