[MohamedBassem]

No description provided.

[coderabbitai]

Walkthrough

Rate-limit middleware in both API and tRPC packages is updated to include authenticated user IDs in the rate-limit key when user context is present. Key format changes from IP-only to optional IP+user-ID composition. New test suites added for both implementations.

Changes

Cohort / File(s)	Summary
Rate-Limit Middleware packages/api/middlewares/rateLimit.ts, packages/trpc/lib/rateLimit.ts	Updated rate-limit key generation to conditionally include user ID segment when user?.id is available, changing key from ip:name (or ip:path) to ip:user:<id>:name (or ip:user:<id>:path). Type signature updated to accept optional user context.
Rate-Limit Tests packages/api/middlewares/rateLimit.test.ts, packages/trpc/lib/__tests__/rateLimit.test.ts	New test suites added to validate rate-limit key construction for both authenticated and unauthenticated requests, verifying correct user ID inclusion and fallback to IP-only keys.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Possibly related PRs

• fix: add ratelimiting for the changePassword endpoint #2723 — Adds the rate-limit middleware to the changePassword endpoint, which directly depends on these middleware changes for user-based rate limiting.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No pull request description was provided by the author.	Add a description explaining the rationale for including userId in the ratelimit key and any relevant context about the changes.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: include userId in ratelimit key if the user is authed' directly describes the main change across multiple files: both rateLimit middleware implementations now conditionally include the user ID in the rate-limit lookup key when user context is available.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR fixes the rate limit key to include the authenticated user's ID, giving each user their own rate limit bucket per IP instead of sharing one across all users on the same IP. Both the Hono middleware (packages/api) and the tRPC middleware (packages/trpc) are updated consistently, and new tests cover both the authenticated and unauthenticated key formats.

Confidence Score: 5/5

Safe to merge — minimal, well-tested change with no logic issues.

The change is small and targeted, both implementations are consistent, and new tests verify the expected key format for authenticated and unauthenticated requests. No P0/P1 findings.

No files require special attention.

Important Files Changed

Filename	Overview
packages/api/middlewares/rateLimit.ts	Appends :user:{userId} segment to rate limit key when user is authenticated, keeping IP-only key for unauthenticated requests.
packages/api/middlewares/rateLimit.test.ts	New test file covering both authenticated (user-scoped key) and unauthenticated (IP-only key) middleware behavior.
packages/trpc/lib/rateLimit.ts	Same user-scoped key logic as the Hono middleware; also widens the ctx type to accept an optional user field.
packages/trpc/lib/tests/rateLimit.test.ts	New test file mirroring the Hono tests: verifies user-scoped and IP-only key generation for the tRPC middleware.

_{Reviews (1): Last reviewed commit: "fix: include userId in ratelimit key if ..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (3)

packages/api/middlewares/rateLimit.test.ts (1)

60-89: Good coverage on the key format.

Tests pin down the exact key for both authed and unauthed cases, which is the behavior this PR intends to guarantee.

One optional addition: a test for the rateLimiting.enabled = false path and for a denied (allowed: false) response verifying the 429 is thrown — currently only the happy path with { allowed: true } is exercised.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/api/middlewares/rateLimit.test.ts` around lines 60 - 89, Add two
tests: one that sets rateLimiting.enabled = false in the config passed to
buildApp and asserts client.checkRateLimit is not called (or that request
proceeds with status 200), and another that mocks client.checkRateLimit to
return { allowed: false } and asserts the app responds with HTTP 429; locate and
modify the existing test file around the buildApp usage and
client.checkRateLimit expectations (see tests referencing buildApp,
client.checkRateLimit, and rateLimiting.enabled) to add these cases.

packages/trpc/lib/rateLimit.ts (2)

38-39: Duplicate key-construction logic with packages/api/middlewares/rateLimit.ts.

Both middlewares now compute ${ip}${userSegment}:${suffix} identically. Consider extracting a small helper (e.g. buildRateLimitKey(ip, userId, suffix)) into @karakeep/shared/ratelimiting so the two implementations can't drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/trpc/lib/rateLimit.ts` around lines 38 - 39, The key construction in
this file (userSegment and key using `${ip}${userSegment}:${opts.path}`) is
duplicated with the implementation in the API middleware; extract a shared
helper function like buildRateLimitKey(ip, userId, suffix) into
`@karakeep/shared/ratelimiting` and replace the inline logic in this module (the
userSegment const and key assignment) with a call to
buildRateLimitKey(opts.ctx.ip || "", opts.ctx.user?.id, opts.path) so both
places use the same canonical routine (update imports and remove the duplicated
string assembly).

---

15-18: Consider tightening the ctx type to match the actual tRPC Context.

The declared shape user?: { id?: string | null } | null is looser than the real Context/AuthedContext from @karakeep/trpc (where user is User | null and user.id is non-optional). Making id optional here invites silent fallback to the IP-only key if the shape ever drifts. Since this middleware is only mounted inside tRPC where ctx.user is typed, you could import Context directly (mirroring the API middleware) and drop the local structural type.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/trpc/lib/rateLimit.ts` around lines 15 - 18, The ctx parameter in
packages/trpc/lib/rateLimit.ts uses a loose inline type (ctx: { req: { ip:
string | null }; user?: { id?: string | null } | null }) which allows user.id to
be optional and risks falling back to IP; replace this structural type by
importing and using the actual tRPC Context/AuthedContext type (e.g., import {
Context } from '@karakeep/trpc') for the middleware's ctx parameter (the
middleware/export in this file, e.g., the rateLimit function) so ctx.user and
ctx.user.id match the real types and avoid silent fallbacks to IP-only keys.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/api/middlewares/rateLimit.test.ts`:
- Around line 60-89: Add two tests: one that sets rateLimiting.enabled = false
in the config passed to buildApp and asserts client.checkRateLimit is not called
(or that request proceeds with status 200), and another that mocks
client.checkRateLimit to return { allowed: false } and asserts the app responds
with HTTP 429; locate and modify the existing test file around the buildApp
usage and client.checkRateLimit expectations (see tests referencing buildApp,
client.checkRateLimit, and rateLimiting.enabled) to add these cases.

In `@packages/trpc/lib/rateLimit.ts`:
- Around line 38-39: The key construction in this file (userSegment and key
using `${ip}${userSegment}:${opts.path}`) is duplicated with the implementation
in the API middleware; extract a shared helper function like
buildRateLimitKey(ip, userId, suffix) into `@karakeep/shared/ratelimiting` and
replace the inline logic in this module (the userSegment const and key
assignment) with a call to buildRateLimitKey(opts.ctx.ip || "",
opts.ctx.user?.id, opts.path) so both places use the same canonical routine
(update imports and remove the duplicated string assembly).
- Around line 15-18: The ctx parameter in packages/trpc/lib/rateLimit.ts uses a
loose inline type (ctx: { req: { ip: string | null }; user?: { id?: string |
null } | null }) which allows user.id to be optional and risks falling back to
IP; replace this structural type by importing and using the actual tRPC
Context/AuthedContext type (e.g., import { Context } from '@karakeep/trpc') for
the middleware's ctx parameter (the middleware/export in this file, e.g., the
rateLimit function) so ctx.user and ctx.user.id match the real types and avoid
silent fallbacks to IP-only keys.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e1dfc197-a394-4314-8a38-edc76f636ca7

📥 Commits

Reviewing files that changed from the base of the PR and between 6109588 and 286625a.

📒 Files selected for processing (4)

• packages/api/middlewares/rateLimit.test.ts
• packages/api/middlewares/rateLimit.ts
• packages/trpc/lib/__tests__/rateLimit.test.ts
• packages/trpc/lib/rateLimit.ts