♻️ Clean up trpc code and add global client error handling #1549
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Warning Rate limit exceeded@lukevella has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe changes refactor the Changes
Possibly related PRs
Poem
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 (
|
![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/web/src/trpc/trpc.ts (1)
92-97: Consider extracting rate limit configuration to environment variables.The rate limit values (5 requests per minute) are hardcoded. Consider making these configurable through environment variables for better flexibility across different environments.
+const RATE_LIMIT_REQUESTS = process.env.RATE_LIMIT_REQUESTS ? parseInt(process.env.RATE_LIMIT_REQUESTS) : 5; +const RATE_LIMIT_WINDOW = process.env.RATE_LIMIT_WINDOW || "1 m"; const ratelimit = process.env.KV_REST_API_URL ? new Ratelimit({ redis: kv, - limiter: Ratelimit.slidingWindow(5, "1 m"), + limiter: Ratelimit.slidingWindow(RATE_LIMIT_REQUESTS, RATE_LIMIT_WINDOW), }) : null;Also, consider adding type safety:
-const ratelimit = process.env.KV_REST_API_URL +const ratelimit: Ratelimit | null = process.env.KV_REST_API_URL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/trpc/trpc.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Integration tests
- GitHub Check: Type check
- GitHub Check: Linting
🔇 Additional comments (1)
apps/web/src/trpc/trpc.ts (1)
99-121: LGTM! Clean refactor of the rate limit middleware.The changes effectively use the shared rate limit instance while maintaining the existing error handling and rate limiting logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/web/src/trpc/trpc.ts (2)
92-95: Add validation for therequestsparameter.The
requestsparameter should be validated to ensure it's a positive number.export const createRateLimitMiddleware = ( - requests: number, + requests: number & { __brand: "PositiveNumber" }, duration: "1 m" | "1 h", ) => { + if (requests <= 0) { + throw new Error("Number of requests must be positive"); + }
97-99: Consider logging when rate limiting is disabled.When
KV_REST_API_URLis not set, the middleware silently bypasses rate limiting. Consider logging this behavior to help with debugging.if (!process.env.KV_REST_API_URL) { + console.warn("Rate limiting is disabled: KV_REST_API_URL is not set"); return next(); }apps/web/src/trpc/routers/user.ts (2)
56-71: Document rate limit configurations.Consider adding comments explaining the rationale behind each rate limit configuration to help future maintainers understand the choices.
+ // Limit to 5 requests per hour due to destructive nature delete: privateProcedure .use(createRateLimitMiddleware(5, "1 h")) + // Limit to 20 requests per hour for profile updates changeName: privateProcedure .use(createRateLimitMiddleware(20, "1 h")) + // Limit to 30 requests per hour for preference updates updatePreferences: privateProcedure .use(createRateLimitMiddleware(30, "1 h")) + // Limit to 10 requests per hour due to email sending requestEmailChange: privateProcedure .use(createRateLimitMiddleware(10, "1 h")) + // Limit to 20 requests per hour for S3 signed URL generation getAvatarUploadUrl: privateProcedure .use(createRateLimitMiddleware(20, "1 h")) + // Limit to 10 requests per hour due to S3 operations updateAvatar: privateProcedure .input(z.object({ imageKey: z.string().max(255) })) .use(createRateLimitMiddleware(10, "1 h"))Also applies to: 84-86, 101-103, 128-130, 180-182, 225-228
225-228: Maintain consistent middleware ordering.The rate limit middleware is placed after the input validation in
updateAvatar, while in other routes it's placed before. Consider maintaining a consistent order across all routes.updateAvatar: privateProcedure - .input(z.object({ imageKey: z.string().max(255) })) .use(createRateLimitMiddleware(10, "1 h")) + .input(z.object({ imageKey: z.string().max(255) }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/trpc/routers/user.ts(7 hunks)apps/web/src/trpc/trpc.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Integration tests
- GitHub Check: Type check
- GitHub Check: Linting
lukevella
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/web/src/trpc/client.ts (1)
5-14: Consider selective query invalidation
Invalidating all queries on every successful mutation can be unnecessarily broad, leading to extra network requests and potential performance costs. Consider selectively invalidating only the queries relevant to the mutated data.apps/web/src/app/api/trpc/[trpc]/route.ts (1)
34-35: Reevaluate forcing 127.0.0.1 in development
Using "127.0.0.1" during development may mask the real IP address in local testing scenarios. If you need accurate IP-based logic (e.g. rate limiting per IP), consider making the dev IP configurable or preserving the actual IP for more realistic testing.apps/web/src/trpc/client/provider.tsx (2)
22-28: Consider adjusting query client configuration for better performance.The current configuration disables retries and sets infinite cache time which might not be optimal for all scenarios.
Consider these adjustments:
defaultOptions: { queries: { retry: false, - cacheTime: Infinity, + cacheTime: 1000 * 60 * 30, // 30 minutes staleTime: 1000 * 60, }, },
29-50: Enhance error handling with more specific error messages.The error handling could be more informative for users, especially for the
TOO_MANY_REQUESTScase.case "TOO_MANY_REQUESTS": toast({ title: t("tooManyRequests", { defaultValue: "Too many requests", }), - description: error.message, + description: t("tooManyRequestsDescription", { + defaultValue: "Please wait a moment before trying again.", + }), }); break;apps/web/src/trpc/trpc.ts (1)
125-126: Consider documenting rate limit configuration.The example usage would benefit from a comment explaining the rate limit values.
-// Usage example: +// Example: Limit to 5 requests per minute per IP address export const rateLimitMiddleware = createRateLimitMiddleware(5, "1 m");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.env.development(1 hunks)apps/web/package.json(0 hunks)apps/web/src/app/[locale]/poll/[urlId]/duplicate-dialog.tsx(1 hunks)apps/web/src/app/api/trpc/[trpc]/route.ts(1 hunks)apps/web/src/app/providers.tsx(1 hunks)apps/web/src/components/poll/manage-poll.tsx(1 hunks)apps/web/src/components/poll/manage-poll/finalize-poll-dialog.tsx(1 hunks)apps/web/src/trpc/client.ts(1 hunks)apps/web/src/trpc/client/config.ts(0 hunks)apps/web/src/trpc/client/provider.tsx(1 hunks)apps/web/src/trpc/routers/polls.ts(4 hunks)apps/web/src/trpc/routers/user.ts(7 hunks)apps/web/src/trpc/trpc.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/web/package.json
- apps/web/src/trpc/client/config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/trpc/routers/user.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Linting
- GitHub Check: Type check
- GitHub Check: Integration tests
🔇 Additional comments (11)
.env.development (1)
27-27: Review the updated SMTP host setting.
Changing SMTP_HOST from "localhost" to "0.0.0.0" now binds the SMTP service to all interfaces in development. Please verify that this is the intended behavior for your dev environment and that it won’t inadvertently expose the service outside of the trusted network.apps/web/src/app/providers.tsx (2)
8-8: Welcome the introduction of TRPCProvider
The addition of TRPCProvider is a solid approach that aligns with the new TRPC client integration.
16-29: Verify provider layering
Currently, TRPCProvider is nested within PostHogProvider and I18nProvider. If TRPC procedures rely on locale or user data from these providers, this order is correct. Otherwise, consider moving TRPCProvider above them if the TRPC context needs to initialize earlier or remain unaffected by changes in user or locale state.apps/web/src/app/[locale]/poll/[urlId]/duplicate-dialog.tsx (1)
18-18: LGTM!The import path change aligns with the centralized TRPC client configuration.
apps/web/src/trpc/trpc.ts (1)
92-123: Share rate limit instances to improve performance.Creating a new
Ratelimitinstance for each middleware call is inefficient.apps/web/src/components/poll/manage-poll/finalize-poll-dialog.tsx (1)
32-32: LGTM!The import path change aligns with the centralized TRPC client configuration.
apps/web/src/components/poll/manage-poll.tsx (1)
37-37: LGTM!The import path change aligns with the transition from
@trpc/nextto@trpc/react.apps/web/src/trpc/routers/polls.ts (4)
15-15: LGTM!The import of
createRateLimitMiddlewarealigns with the refactoring to share rate limit instances.
133-133: LGTM!The rate limit of 20 requests per hour for poll creation is reasonable to prevent abuse while allowing legitimate use.
236-236: LGTM!The rate limit of 60 requests per hour for poll updates provides flexibility for frequent modifications while maintaining protection.
309-309: LGTM!The rate limit of 30 requests per hour for poll deletion strikes a good balance between allowing cleanup and preventing mass deletions.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores