feat: added global rate limiting#20
Merged
BuckyMcYolo merged 3 commits intomainfrom Mar 18, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Redis-backed rate limiting (API, realtime, auth), message pinning (DB + realtime event + API + UI), new pin-related routes and schemas, client-side pinning UI + hook, permission for pin action, and Redis wiring for auth storage. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as API Server
participant Middleware as Rate Limiter
participant Redis as Redis
participant Handler as Route Handler
Client->>API: HTTP request
API->>Middleware: globalRateLimit middleware
Middleware->>Middleware: determine identifier (IP / keyExtractor)
Middleware->>Redis: INCR ratelimit:api:<prefix>:<id>:<windowNum>
Redis-->>Middleware: count
alt first increment
Middleware->>Redis: EXPIRE key (window + grace)
Redis-->>Middleware: OK
end
alt count <= max
Middleware->>API: next()
API->>Handler: route executes
Handler-->>Client: 200 OK (X-RateLimit headers)
else count > max
Middleware-->>Client: 429 Too Many Requests (Retry-After)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/middleware/rate-limit.ts`:
- Around line 37-73: The rateLimiter middleware currently calls getRedisClient()
and Redis commands (redis.incr, redis.expire) without error handling, which will
cause unhandled exceptions if Redis is down; wrap the Redis calls inside a
try/catch in the rateLimiter function, and on Redis errors either fail-open (log
the error via your logger and call await next() to allow the request through) or
fail-closed (return the 429/appropriate error response) depending on the desired
policy; ensure the catch logs the error and preserves headers behavior when
failing-open (e.g., skip setting X-RateLimit-* when Redis unavailable) and
reference rateLimiter, getRedisClient, redis.incr, and redis.expire so you can
locate and update the logic accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cc05b5a-a4fe-4408-bc30-2235a7af2f2b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/api/src/app.tsapps/api/src/middleware/rate-limit.tsapps/realtime/src/index.tsapps/realtime/src/services/rate-limit.tspackages/auth/package.jsonpackages/auth/src/lib/auth.ts
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR implements a comprehensive global rate-limiting system, message pinning functionality, and authentication enhancements using Redis. However, there is a critical architectural gap in the message pinning feature that would prevent real-time synchronization between clients.
Rate Limiting Implementation
apps/api/src/middleware/rate-limit.tswith configurable limits (100 requests/min per IP by default)writeRateLimit: 30 requests/min per user (for write operations)X-RateLimit-Limit,X-RateLimit-Remaining, andRetry-AfterheadersMessage Pinning Feature
pinned: booleanfield to messages table with default valuefalsePATCH /guilds/{guildSlug}/channels/{channelId}/messages/{messageId}/pin: Toggle pin stateGET /guilds/{guildSlug}/channels/{channelId}/pins: List pinned messages in a channelCritical Issue: Missing Real-Time Event Broadcasting
The message pinning feature has a significant architectural flaw: The REST API endpoint updates the database but does NOT emit WebSocket events to notify other connected clients. This is inconsistent with the established pattern used for other message operations (delete, edit, reaction toggle).
The codebase shows:
message:pin:toggledis defined inServerToClientEvents(packages/realtime-types/src/events.ts)useMessagePinning) listens for this event:socket.on("message:pin:toggled", ...)This means:
Authentication Enhancements
@better-auth/redis-storagewith Redis client initialization usingenv.REDIS_URLDatabase and Schema Changes
Code Quality Observations
Strengths:
Weaknesses:
Confidence Score: 2/5
The PR demonstrates clear intent and has many well-implemented components (rate limiting, UI, permissions), but the missing WebSocket event emission for message pinning represents a critical gap that would cause the feature to malfunction in production. Other clients would not receive real-time updates when messages are pinned/unpinned. The architecture is incomplete and requires: