Rate Limiting Feature #2
Conversation
…organize recommended additions
|
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 a cache-backed windowed RateLimiter (Memory/Redis) with atomic incr/TTL, integrates rate limiting into ApiKeyManager.verify (returns RATE_LIMIT_EXCEEDED and rateLimit metadata), exports RateLimiter and related types, extends Cache API with incr, updates tests, README, and feature suggestion ordering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ApiKeyManager
participant RateLimiter
participant Cache
Client->>ApiKeyManager: verify(apiKey, options)
ApiKeyManager->>RateLimiter: check(apiKeyRecord, options?) [if configured]
alt increment = true
Note right of RateLimiter #D6EAF8: compute windowStart, windowKey, ttl
RateLimiter->>Cache: incr(windowKey, ttl)
Cache-->>RateLimiter: newCount
else increment = false
RateLimiter->>Cache: get(windowKey)
Cache-->>RateLimiter: currentCount
end
alt newCount ≤ maxRequests
RateLimiter-->>ApiKeyManager: allowed + rateLimit metadata
ApiKeyManager-->>Client: success + rateLimit info
else newCount > maxRequests
RateLimiter-->>ApiKeyManager: blocked + rateLimit metadata
ApiKeyManager-->>Client: error (RATE_LIMIT_EXCEEDED) + rateLimit info
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
README.md
Outdated
| }) | ||
|
|
||
| // Check rate limit | ||
| const result = await keys.verify(headers) |
There was a problem hiding this comment.
Looks good, but would be better DX to include the ratelimit check into the verify itself, and that can simplify the flow
There was a problem hiding this comment.
Ah, I forgot to use the ratelimiter in the manager itself
Btw should it be opt-in with a boolean option?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
FEATURE_SUGGESTIONS.md (1)
8-8: Keep priorities consistent with “Currently Implemented”.Rate limiting and “Update last used timestamp” are already implemented, yet still listed under High Priority. Remove or move them to a “Completed”/“Shipped” section to avoid confusion.
Apply this diff to the Priority section:
**High Priority:** -- Rate limiting (security) -- Update last used timestamp (already added) - Audit logging (compliance)Also applies to: 111-126
src/types/error-types.ts (1)
28-31: Add structured rate-limit details for better client UX.New RATE_LIMIT_EXCEEDED looks good. Consider including retry metadata (e.g., resetMs/resetAt) in the error’s details when you surface this code so callers can set Retry-After headers without re-computing.
Also applies to: 48-48
src/core/rate-limiter.test.ts (3)
1-2: Prefer Bun’s built‑in test runner and Redis client.Guidelines require bun test + Bun’s Redis. Replace Vitest/ioredis with Bun equivalents to reduce deps and speed up CI.
Minimal migration sketch:
-import Redis from "ioredis"; -import { beforeEach, describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it } from "bun:test"; +import { redis } from "bun"; // Bun’s native Redis client
- Switch CI to
bun test.- Replace ioredis usage with Bun’s
redisAPI.Docs:
- Bun test runner usage, import from "bun:test". (bun.com)
- Bun Redis client (import { redis } from "bun"). (bun.com)
59-79: Avoid real sleeps; use fake timers or inject a time source.The tests wait with
setTimeout, slowing CI and adding flakiness. Prefer fake timers (advance time) or inject a clock into RateLimiter and mock it in tests.
- With Vitest:
vi.useFakeTimers(),vi.advanceTimersByTime(...),vi.setSystemTime(...). (main.vitest.dev)- Alternative: RateLimiter accepts a
now: () => numberin config for deterministic windows (no sleeps).Also applies to: 98-109, 111-127, 129-141, 142-162
180-183: Replace console.warn with project logger or test skip messaging.Guidelines disallow console usage. Use the shared logger utility or rely on the test runner’s skip messaging.
src/manager.ts (1)
730-757: Provide a sensible default keyPrefix to avoid cache key collisions.If callers omit
config.keyPrefix, consider defaulting it to a namespaced value (e.g.,keypal:rl:${this.config.prefix ?? ""}) so multiple apps/environments sharing Redis don’t clash.Example:
- createRateLimiter(config: RateLimitConfig): RateLimiter { + createRateLimiter(config: RateLimitConfig): RateLimiter { if (!this.cache) { throw new Error( "[keypal] Cache is required for rate limiting. Enable cache in ApiKeyManager config." ); } - return new RateLimiter(this.cache, config); + const keyPrefix = + config.keyPrefix ?? `keypal:rl:${this.config.prefix ?? ""}`; + return new RateLimiter(this.cache, { ...config, keyPrefix }); }README.md (1)
176-217: Document the race condition behavior for concurrent requests.The fixed-window rate limiter has an inherent race condition: when multiple requests arrive concurrently, they may all read the same count before any increments are persisted, potentially exceeding the limit slightly. This is a known limitation of non-atomic fixed-window implementations.
Consider adding a brief note in the documentation:
Protect your API from abuse with built-in rate limiting. Uses the same cache infrastructure (memory or Redis) for high-performance request tracking. +**Note:** This implementation uses a fixed-window algorithm without atomic operations. Under high concurrency, the limit may be slightly exceeded due to race conditions between read and write operations. For strict enforcement, consider using Redis with atomic operations or a token bucket algorithm. + **Note:** Cache must be enabled to use rate limiting.src/core/rate-limiter.ts (2)
49-50: Add validation for parsed cache values.If the cache returns corrupted data,
Number.parseIntcould returnNaN, leading to incorrect limit enforcement. While unlikely in normal operation, defensive coding would prevent cache corruption from breaking rate limiting.Apply this diff to add NaN validation:
const currentValue = await this.cache.get(key); -const current = currentValue ? Number.parseInt(currentValue, 10) : 0; +const parsedValue = currentValue ? Number.parseInt(currentValue, 10) : 0; +const current = Number.isNaN(parsedValue) ? 0 : parsedValue;
88-91: Add validation for parsed cache values.Same issue as in the
checkmethod: if the cache returns corrupted data,Number.parseIntcould returnNaN. This would propagate to the caller.Apply this diff to add NaN validation:
const value = await this.cache.get(key); -return value ? Number.parseInt(value, 10) : 0; +const parsedValue = value ? Number.parseInt(value, 10) : 0; +return Number.isNaN(parsedValue) ? 0 : parsedValue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
FEATURE_SUGGESTIONS.md(8 hunks)README.md(3 hunks)src/core/rate-limiter.test.ts(1 hunks)src/core/rate-limiter.ts(1 hunks)src/index.ts(2 hunks)src/manager.ts(3 hunks)src/types/error-types.ts(2 hunks)src/types/rate-limit-types.ts(1 hunks)src/utils/constants.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/types/error-types.tssrc/types/rate-limit-types.tssrc/core/rate-limiter.test.tssrc/manager.tssrc/utils/constants.tssrc/core/rate-limiter.tssrc/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/types/error-types.tssrc/types/rate-limit-types.tssrc/core/rate-limiter.test.tssrc/manager.tssrc/utils/constants.tssrc/core/rate-limiter.tssrc/index.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/types/error-types.tssrc/types/rate-limit-types.tssrc/core/rate-limiter.test.tssrc/manager.tssrc/utils/constants.tssrc/core/rate-limiter.tssrc/index.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/types/error-types.tssrc/types/rate-limit-types.tssrc/core/rate-limiter.test.tssrc/manager.tssrc/utils/constants.tssrc/core/rate-limiter.tssrc/index.ts
{**/*.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.{ts,tsx,js,jsx},package.json}: Usebun <file>instead ofnode <file>orts-node <file>
UseBun.serve()for HTTP/WebSocket servers; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use the built-inWebSocket; do not usews
UseBun.$shell (e.g., Bun.$ls) instead ofexeca
Files:
src/types/error-types.tssrc/types/rate-limit-types.tssrc/core/rate-limiter.test.tssrc/manager.tssrc/utils/constants.tssrc/core/rate-limiter.tssrc/index.ts
{**/*.{html,ts,css},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuild
Files:
src/types/error-types.tssrc/types/rate-limit-types.tssrc/core/rate-limiter.test.tssrc/manager.tssrc/utils/constants.tssrc/core/rate-limiter.tssrc/index.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{test,spec}.{js,jsx,ts,tsx}: Don't nest describe() blocks too deeply in test files
Don't use callbacks in asynchronous tests and hooks
Don't have duplicate hooks in describe blocks
Don't use export or module.exports in test files
Don't use focused tests
Ensure assertions (e.g., expect) are placed inside an it() call
Don't use disabled tests
Files:
src/core/rate-limiter.test.ts
{**/*.test.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.test.{ts,tsx,js,jsx},package.json}: Usebun testinstead ofjestorvitest
Usebun testto run tests
Files:
src/core/rate-limiter.test.ts
🧬 Code graph analysis (5)
src/types/error-types.ts (1)
src/index.ts (1)
ApiKeyErrorCode(20-20)
src/types/rate-limit-types.ts (1)
src/index.ts (3)
RateLimitConfig(27-27)RateLimitResult(28-28)RateLimitCheckOptions(26-26)
src/core/rate-limiter.test.ts (2)
src/manager.ts (1)
verify(244-358)src/utils/logger.ts (1)
error(59-64)
src/manager.ts (1)
src/index.ts (2)
RateLimitConfig(27-27)RateLimiter(7-7)
src/core/rate-limiter.ts (3)
src/core/cache.ts (1)
Cache(3-7)src/types/rate-limit-types.ts (3)
RateLimitConfig(4-11)RateLimitCheckOptions(34-39)RateLimitResult(16-29)src/utils/constants.ts (1)
MILLISECONDS_PER_SECOND(1-1)
🔇 Additional comments (8)
src/index.ts (2)
7-7: LGTM: public RateLimiter value re-export.Clean, tree‑shake friendly barrel re-export. Matches public API direction.
25-29: LGTM: type-only re-exports.export type used correctly for RateLimit types; no runtime impact.
src/utils/constants.ts (1)
1-1: LGTM: shared time constant.Clear name, avoids magic numbers across modules.
FEATURE_SUGGESTIONS.md (1)
12-107: No dangling references found—removal is clean.Verification confirms no references to the removed "Rate Limiting Helper" section exist in README.md or other documentation files in the repository. The removal has no broken cross-links or anchors.
README.md (2)
14-14: LGTM!The feature description accurately describes the fixed window algorithm implementation.
506-524: LGTM!The TypeScript type definitions accurately reflect the implementation and provide clear documentation for the rate limiting API.
src/core/rate-limiter.ts (2)
21-30: LGTM!The window calculation correctly implements a fixed-window algorithm aligned to epoch time, ensuring consistent behavior across distributed systems.
78-82: LGTM!The reset method correctly clears the current window's counter. The scoped deletion (only current window) is the right behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="src/core/rate-limiter.test.ts">
<violation number="1" location="src/core/rate-limiter.test.ts:183">
Rethrowing the Redis connection error causes this test to fail whenever Redis isn’t available, despite the intent to skip; return early instead so the suite can proceed when Redis is offline.</violation>
</file>
<file name="src/core/rate-limiter.ts">
<violation number="1" location="src/core/rate-limiter.ts:65">
Updating the rate limit counter with separate get/set calls is not atomic, so concurrent requests can read the same count and each increment it, allowing the limit to be exceeded. Please use an atomic increment (e.g., INCR/SETEX transaction or Lua script) when incrementing the window counter.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/core/cache.ts (1)
55-59: Use the existingset()method to avoid code duplication.Lines 55-59 duplicate the TTL calculation logic from the
set()method. While this works, it creates maintenance burden if the TTL logic ever changes.Consider refactoring to use the existing method (after addressing the TTL reset issue):
- this.cache.set(key, { - value: String(newValue), - // biome-ignore lint/style/noMagicNumbers: 1000ms to seconds - expires: Date.now() + ttl * 1000, - }); + this.set(key, String(newValue), ttl);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/cache.test.ts(3 hunks)src/core/cache.ts(3 hunks)src/core/rate-limiter.test.ts(1 hunks)src/core/rate-limiter.ts(1 hunks)src/types/rate-limit-types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/rate-limiter.test.ts
- src/types/rate-limit-types.ts
- src/core/rate-limiter.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/core/cache.tssrc/core/cache.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/core/cache.tssrc/core/cache.test.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/core/cache.tssrc/core/cache.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/core/cache.tssrc/core/cache.test.ts
{**/*.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.{ts,tsx,js,jsx},package.json}: Usebun <file>instead ofnode <file>orts-node <file>
UseBun.serve()for HTTP/WebSocket servers; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use the built-inWebSocket; do not usews
UseBun.$shell (e.g., Bun.$ls) instead ofexeca
Files:
src/core/cache.tssrc/core/cache.test.ts
{**/*.{html,ts,css},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuild
Files:
src/core/cache.tssrc/core/cache.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{test,spec}.{js,jsx,ts,tsx}: Don't nest describe() blocks too deeply in test files
Don't use callbacks in asynchronous tests and hooks
Don't have duplicate hooks in describe blocks
Don't use export or module.exports in test files
Don't use focused tests
Ensure assertions (e.g., expect) are placed inside an it() call
Don't use disabled tests
Files:
src/core/cache.test.ts
{**/*.test.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.test.{ts,tsx,js,jsx},package.json}: Usebun testinstead ofjestorvitest
Usebun testto run tests
Files:
src/core/cache.test.ts
🧬 Code graph analysis (1)
src/core/cache.ts (1)
src/storage/redis.ts (1)
key(14-16)
🔇 Additional comments (2)
src/core/cache.test.ts (2)
62-104: Add tests for edge cases and TTL behavior.The tests cover basic functionality well, but are missing coverage for:
- NaN handling: What happens when you increment a key that contains a non-numeric value?
- TTL reset behavior: Do multiple increments reset the TTL or preserve the original expiration?
Consider adding these test cases:
it("should handle non-numeric values gracefully", () => { cache.set("counter", "not-a-number", 60); const result = cache.incr("counter", 60); expect(result).toBe(1); // Should treat as new key }); it("should verify TTL behavior on multiple increments", () => { const firstIncr = cache.incr("counter", 1); // 1 second TTL const item = cache['cache'].get("counter"); const firstExpiry = item?.expires; // Increment again immediately const secondIncr = cache.incr("counter", 1); const secondItem = cache['cache'].get("counter"); const secondExpiry = secondItem?.expires; // Verify whether TTL was reset or preserved // (behavior depends on implementation choice) });
150-174: LGTM! Redis incr tests are well structured.The tests appropriately verify:
- The Lua script is invoked via
eval- The script contains the expected INCR command
- The return value is correctly typed as a number
- The correct parameters (key count, key name, TTL) are passed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/types/config-types.ts (1)
122-129: Document/cache-validate the rateLimit option.JSDoc is clear. Two follow-ups:
- Add this field to any runtime validation schema you use (e.g., TypeBox) so misconfigurations surface early.
- Clarify the cache requirement inline: “Requires cache: true | 'redis' | Cache instance.”
Optional JSDoc tweak:
- * Requires cache to be enabled + * Requires cache to be enabled (cache: true | 'redis' | Cache instance)src/manager.test.ts (2)
1124-1131: Avoid await inside loops where feasible.Sequential calls are required here, but the guideline discourages
awaitin loops. Since the count is small and fixed, unroll for clarity:- for (let i = 0; i < ALLOWED_REQUESTS; i++) { - const result = await keysWithRateLimit.verify(key); - expect(result.valid).toBe(true); - expect(result.rateLimit).toBeDefined(); - expect(result.rateLimit?.limit).toBe(ALLOWED_REQUESTS); - expect(result.rateLimit?.remaining).toBe(ALLOWED_REQUESTS - (i + 1)); - } + const r1 = await keysWithRateLimit.verify(key); + expect(r1.valid).toBe(true); + expect(r1.rateLimit?.limit).toBe(ALLOWED_REQUESTS); + expect(r1.rateLimit?.remaining).toBe(ALLOWED_REQUESTS - 1); + const r2 = await keysWithRateLimit.verify(key); + expect(r2.valid).toBe(true); + expect(r2.rateLimit?.remaining).toBe(ALLOWED_REQUESTS - 2); + const r3 = await keysWithRateLimit.verify(key); + expect(r3.valid).toBe(true); + expect(r3.rateLimit?.remaining).toBe(0);
1239-1247: Add resetAt assertion on cache‑hit path (optional).Strengthen the cache‑path test by validating the ISO format for
resetAtas well:- const result3 = await keysWithRateLimit.verify(key); + const result3 = await keysWithRateLimit.verify(key); expect(result3.valid).toBe(false); expect(result3.errorCode).toBe("RATE_LIMIT_EXCEEDED"); + expect(result3.rateLimit?.resetAt).toMatch(ISO_DATE_REGEX);src/manager.ts (3)
314-350: Also invalidate cache when key is disabled (parity with expired/revoked).On cache‑hit you delete for expired/revoked but not disabled. For consistency and to avoid stale entries if a key is disabled out‑of‑band, delete here too.
- if (record.metadata.enabled === false) { - return createErrorResult(ApiKeyErrorCode.DISABLED); - } + if (record.metadata.enabled === false) { + await this.cache?.del(`apikey:${keyHash}`); + return createErrorResult(ApiKeyErrorCode.DISABLED); + }
361-366: Await cache deletion on corruption (consistency).You
awaitcache deletions elsewhere; here it’s fire‑and‑forget. Prefer awaiting with try/catch to avoid silent failures.- this.cache.del(`apikey:${keyHash}`); + try { + await this.cache?.del(`apikey:${keyHash}`); + } catch (e) { + logger.error("Failed to invalidate corrupt cache entry:", e); + }
395-443: Deduplicate rate‑limit result shaping.The allowed/blocked branches here and in the cache‑hit path duplicate structure. Extract a tiny helper to format the
rateLimitpayload and/or a small internalapplyRateLimit()that returns either an error result or the shaped data.Benefit: reduces drift if fields change.
README.md (2)
185-227: Add a note about epoch‑aligned windows.Mention that windows align to epoch boundaries to explain occasional shorter initial windows after reset (useful for operators).
- Protect your API from abuse with built-in rate limiting. Uses the same cache infrastructure (memory or Redis) for high-performance request tracking. + Protect your API from abuse with built-in rate limiting. Uses the same cache infrastructure (memory or Redis) for high-performance request tracking. Windows are aligned to epoch time for consistent behavior in distributed systems.
255-303: Tiny editorial nit.Hyphenate the heading for consistency with style guides.
- #### Manual Rate Limiting (Advanced) + #### Manual rate limiting (advanced)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(5 hunks)src/core/cache.ts(3 hunks)src/manager.test.ts(1 hunks)src/manager.ts(8 hunks)src/types/config-types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/cache.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/types/config-types.tssrc/manager.test.tssrc/manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/types/config-types.tssrc/manager.test.tssrc/manager.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/types/config-types.tssrc/manager.test.tssrc/manager.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/types/config-types.tssrc/manager.test.tssrc/manager.ts
{**/*.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.{ts,tsx,js,jsx},package.json}: Usebun <file>instead ofnode <file>orts-node <file>
UseBun.serve()for HTTP/WebSocket servers; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use the built-inWebSocket; do not usews
UseBun.$shell (e.g., Bun.$ls) instead ofexeca
Files:
src/types/config-types.tssrc/manager.test.tssrc/manager.ts
{**/*.{html,ts,css},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuild
Files:
src/types/config-types.tssrc/manager.test.tssrc/manager.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{test,spec}.{js,jsx,ts,tsx}: Don't nest describe() blocks too deeply in test files
Don't use callbacks in asynchronous tests and hooks
Don't have duplicate hooks in describe blocks
Don't use export or module.exports in test files
Don't use focused tests
Ensure assertions (e.g., expect) are placed inside an it() call
Don't use disabled tests
Files:
src/manager.test.ts
{**/*.test.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.test.{ts,tsx,js,jsx},package.json}: Usebun testinstead ofjestorvitest
Usebun testto run tests
Files:
src/manager.test.ts
🧬 Code graph analysis (3)
src/types/config-types.ts (1)
src/index.ts (1)
RateLimitConfig(27-27)
src/manager.test.ts (3)
src/manager.ts (1)
createKeys(893-895)src/index.ts (1)
createKeys(10-10)src/storage/redis.ts (1)
key(14-16)
src/manager.ts (3)
src/index.ts (3)
RateLimiter(7-7)ApiKeyErrorCode(20-20)RateLimitConfig(27-27)src/types/error-types.ts (2)
ApiKeyErrorCode(4-31)ApiKeyErrorCode(33-34)src/utils/logger.ts (2)
logger(71-71)error(59-64)
🪛 LanguageTool
README.md
[uncategorized] ~257-~257: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...al Rate Limiting (Advanced) For custom rate limiting scenarios (e.g., different limits per e...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (9)
src/types/config-types.ts (1)
4-4: Type-only import is correct and lean.Keeps runtime bundle clean and aligns with TS best practices.
src/manager.test.ts (1)
1109-1119: Test runner consistency with repo standards.These tests use Vitest. If the project standard is
bun test, consider aligning in a follow-up PR; otherwise, confirm Vitest remains the chosen runner. As per coding guidelines.src/manager.ts (6)
11-11: Import looks good.Dependency is scoped to the manager and keeps layering clean.
25-25: Type-only import is correct.Avoids runtime cost and clarifies intent.
41-54: VerifyResult extension is well-shaped.Payload is minimal and sufficient for headers and UX. Naming is clear.
104-104: Private rateLimiter field addition is appropriate.Optional, readonly, and initialized via config—good encapsulation.
160-164: Constructor wiring is safe and explicit.Initialization happens after cache setup, and throws when cache is missing—good fail‑fast behavior.
856-864: Factory method is clear and enforces prerequisites.Good API surface for custom scenarios; error message is actionable.
README.md (1)
14-14: Nice: concise feature bullet.Matches implementation and sets expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/manager.test.ts(1 hunks)src/manager.ts(8 hunks)src/types/error-types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/types/error-types.tssrc/manager.test.tssrc/manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/types/error-types.tssrc/manager.test.tssrc/manager.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/types/error-types.tssrc/manager.test.tssrc/manager.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/types/error-types.tssrc/manager.test.tssrc/manager.ts
{**/*.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.{ts,tsx,js,jsx},package.json}: Usebun <file>instead ofnode <file>orts-node <file>
UseBun.serve()for HTTP/WebSocket servers; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use the built-inWebSocket; do not usews
UseBun.$shell (e.g., Bun.$ls) instead ofexeca
Files:
src/types/error-types.tssrc/manager.test.tssrc/manager.ts
{**/*.{html,ts,css},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuild
Files:
src/types/error-types.tssrc/manager.test.tssrc/manager.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{test,spec}.{js,jsx,ts,tsx}: Don't nest describe() blocks too deeply in test files
Don't use callbacks in asynchronous tests and hooks
Don't have duplicate hooks in describe blocks
Don't use export or module.exports in test files
Don't use focused tests
Ensure assertions (e.g., expect) are placed inside an it() call
Don't use disabled tests
Files:
src/manager.test.ts
{**/*.test.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.test.{ts,tsx,js,jsx},package.json}: Usebun testinstead ofjestorvitest
Usebun testto run tests
Files:
src/manager.test.ts
🧬 Code graph analysis (3)
src/types/error-types.ts (1)
src/index.ts (1)
ApiKeyErrorCode(24-24)
src/manager.test.ts (1)
src/manager.ts (1)
createKeys(1131-1133)
src/manager.ts (3)
src/index.ts (3)
RateLimiter(6-6)ApiKeyErrorCode(24-24)RateLimitConfig(31-31)src/types/error-types.ts (2)
ApiKeyErrorCode(4-52)ApiKeyErrorCode(54-55)src/utils/logger.ts (2)
logger(71-71)error(59-64)
🔇 Additional comments (6)
src/types/error-types.ts (1)
29-30: LGTM! Clean addition of rate limit error code.The new error code follows the established pattern and integrates cleanly with the existing error handling infrastructure.
Also applies to: 69-69
src/manager.test.ts (1)
1362-1501: LGTM! Comprehensive rate limiting test coverage.The test suite thoroughly covers:
- Rate limit enforcement across multiple requests
- Per-key isolation of limits
- Behavior when rate limiting is disabled
- Error handling when cache is not configured
- Rate limit metadata inclusion in responses
- Both cache hit and cache miss scenarios
The tests are well-structured and use appropriate constants for clarity.
src/manager.ts (4)
11-11: LGTM! Clean type definitions and imports.The rate limit types are properly integrated with optional fields, ensuring backward compatibility. The structure provides all necessary information for clients to handle rate limiting appropriately.
Also applies to: 36-36, 52-64
115-115: LGTM! Proper initialization order.The rate limiter is correctly initialized after the cache, and the error handling for missing cache is appropriate.
Also applies to: 175-178
329-365: LGTM! Cache path rate limiting is correctly implemented.The rate limit check is properly placed after validation checks, and the response includes appropriate rate limit metadata for both allowed and blocked requests.
1075-1102: LGTM! Clean rate limiter factory method.The method properly validates that caching is enabled before creating a rate limiter, and provides a clear error message to guide users toward the correct configuration.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Redis is being mocked in the tests, should be used directly like we do in the other tests |
|
@izadoesdev should be put in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/core/cache.test.ts (3)
1-1: Consider Bun.redis over ioredis (non-blocking).Repo guidelines prefer Bun.redis; tests currently use ioredis. Fine for now, but consider aligning in a follow-up.
21-29: Use fake timers for TTL tests to avoid flakiness.Real sleeps (100–150ms) are brittle in CI. Prefer vi.useFakeTimers() + advanceTimersByTimeAsync.
it("should expire values after TTL", async () => { - // biome-ignore lint/style/noMagicNumbers: 100ms - cache.set("key1", "value1", 0.1); - expect(cache.get("key1")).toBe("value1"); - - // biome-ignore lint/style/noMagicNumbers: 150ms - await new Promise((resolve) => setTimeout(resolve, 150)); - expect(cache.get("key1")).toBeNull(); + vi.useFakeTimers(); + cache.set("key1", "value1", 0.1); + expect(cache.get("key1")).toBe("value1"); + await vi.advanceTimersByTimeAsync(150); + expect(cache.get("key1")).toBeNull(); + vi.useRealTimers(); });it("should handle increment with TTL", async () => { - // biome-ignore lint/style/noMagicNumbers: 100ms - cache.incr("counter", 0.1); - expect(cache.get("counter")).toBe("1"); - - // biome-ignore lint/style/noMagicNumbers: 150ms - await new Promise((resolve) => setTimeout(resolve, 150)); - expect(cache.get("counter")).toBeNull(); + vi.useFakeTimers(); + cache.incr("counter", 0.1); + expect(cache.get("counter")).toBe("1"); + await vi.advanceTimersByTimeAsync(150); + expect(cache.get("counter")).toBeNull(); + vi.useRealTimers(); });Also applies to: 85-105
170-186: Add TTL assertions for incr script and ensure TTL isn’t reset on subsequent increments.Strengthen the EVAL path test by checking TTL semantics of the Lua script (EXPIRE only on first increment).
const result = await cache.incr("counter", 120); // biome-ignore lint/style/noMagicNumbers: 5 as the incremented value expect(result).toBe(5); expect(spyRedisEval).toHaveBeenCalledWith( expect.stringContaining("INCR"), 1, "counter", 120 ); +// TTL should be set (and positive) +const ttlAfterFirst = await redis.ttl("counter"); +expect(ttlAfterFirst).toBeGreaterThan(0); + +// Next incr should NOT reset TTL +await cache.incr("counter", 120); +const ttlAfterSecond = await redis.ttl("counter"); +expect(ttlAfterSecond).toBeLessThanOrEqual(ttlAfterFirst);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/cache.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/core/cache.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/core/cache.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{test,spec}.{js,jsx,ts,tsx}: Don't nest describe() blocks too deeply in test files
Don't use callbacks in asynchronous tests and hooks
Don't have duplicate hooks in describe blocks
Don't use export or module.exports in test files
Don't use focused tests
Ensure assertions (e.g., expect) are placed inside an it() call
Don't use disabled tests
Files:
src/core/cache.test.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/core/cache.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/core/cache.test.ts
{**/*.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.{ts,tsx,js,jsx},package.json}: Usebun <file>instead ofnode <file>orts-node <file>
UseBun.serve()for HTTP/WebSocket servers; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use the built-inWebSocket; do not usews
UseBun.$shell (e.g., Bun.$ls) instead ofexeca
Files:
src/core/cache.test.ts
{**/*.test.{ts,tsx,js,jsx},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
{**/*.test.{ts,tsx,js,jsx},package.json}: Usebun testinstead ofjestorvitest
Usebun testto run tests
Files:
src/core/cache.test.ts
{**/*.{html,ts,css},package.json}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuild
Files:
src/core/cache.test.ts
🧬 Code graph analysis (1)
src/core/cache.test.ts (1)
src/core/cache.ts (1)
RedisCache(79-112)
🔇 Additional comments (4)
src/core/cache.test.ts (4)
139-146: LGTM: verifies get path and spies correctly.
149-156: LGTM: asserts setex called with TTL.
159-163: LGTM: verifies del invocation.
188-194: LGTM: validates numeric return type for new key.
|
please resolve the merge conflict pending PR |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/manager.ts (1)
458-471: Use the error helper for consistency.The error result is manually constructed here, but the cache path at line 368 uses the
createErrorResulthelper with spread syntax. For consistency and to leverage the centralized error message mapping, use the same pattern.Apply this diff:
const rateLimitResult = await this.rateLimiter.check(record); if (!rateLimitResult.allowed) { return { - valid: false, - error: "Rate limit exceeded", - errorCode: ApiKeyErrorCode.RATE_LIMIT_EXCEEDED, + ...createErrorResult(ApiKeyErrorCode.RATE_LIMIT_EXCEEDED), rateLimit: { current: rateLimitResult.current, limit: rateLimitResult.limit, remaining: rateLimitResult.remaining, resetMs: rateLimitResult.resetMs, resetAt: rateLimitResult.resetAt, }, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
FEATURE_SUGGESTIONS.md(6 hunks)biome.jsonc(1 hunks)src/manager.test.ts(1 hunks)src/manager.ts(8 hunks)src/storage/kysely.test.ts(3 hunks)src/storage/kysely.ts(4 hunks)src/types/error-types.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/storage/kysely.test.ts
- src/storage/kysely.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/manager.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/types/error-types.tssrc/manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/types/error-types.tssrc/manager.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/types/error-types.tssrc/manager.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/types/error-types.tssrc/manager.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Prefer
Bun.fileovernode:fsreadFile/writeFile
Files:
src/types/error-types.tssrc/manager.ts
🧬 Code graph analysis (2)
src/types/error-types.ts (1)
src/index.ts (1)
ApiKeyErrorCode(24-24)
src/manager.ts (5)
src/types/audit-log-types.ts (1)
ActionContext(14-26)src/types/config-types.ts (1)
ConfigInput(36-143)src/core/hash.ts (1)
hashKey(10-14)src/types/error-types.ts (3)
createErrorResult(109-123)ApiKeyErrorCode(4-52)ApiKeyErrorCode(54-55)src/utils/logger.ts (2)
logger(71-71)error(59-64)
🔇 Additional comments (13)
biome.jsonc (1)
14-20: Verify the justification for doubling the complexity threshold.The
maxAllowedComplexityhas been increased from 50 to 100—a 2x increase. This is a significant threshold change that could mask future complexity drift or code that genuinely needs refactoring.Please verify that:
- This increase is the minimal necessary amount to accommodate the rate limiting feature.
- The RateLimiter class (and any related code) genuinely warrants this higher threshold and couldn't be simplified instead.
- This change is intentional and not a temporary workaround for complexity that should be addressed in the code itself.
FEATURE_SUGGESTIONS.md (1)
1-114: LGTM! Documentation updates are clear and accurate.The reordering of features and addition of rate limiting to the completed features list appropriately reflects the implementation added in this PR. The new webhook events section provides a useful blueprint for future enhancements.
src/types/error-types.ts (3)
29-30: LGTM! Rate limit error code added correctly.The new
RATE_LIMIT_EXCEEDEDerror code follows the existing pattern and integrates properly with the rate limiting implementation insrc/manager.ts.
69-69: LGTM! Error message is clear and consistent.The error message matches the pattern of other error messages in the codebase.
112-116: LGTM! Return type formatting improved.The multi-line formatting improves readability without changing functionality.
src/manager.ts (8)
11-11: LGTM! Rate limiting imports added correctly.The imports for
RateLimiterandRateLimitConfigare properly structured and align with the new rate limiting functionality.Also applies to: 36-36
52-64: LGTM! Rate limit metadata properly typed.The
rateLimitfield addition toVerifyResultis well-structured with clear documentation for each field. The optional nature allows backward compatibility.
70-75: LGTM! Type formatting improved.The multi-line formatting of
CacheRecordenhances readability. The type correctly defines the minimal data cached for security.
130-132: LGTM! Salt preprocessing improves security.Hashing the salt before use is a good security practice that normalizes the input and strengthens the key derivation. The deterministic hashing ensures existing installations won't break.
Also applies to: 140-140
189-192: LGTM! Rate limiter initialization is clean.The conditional initialization of the rate limiter based on configuration is properly structured and uses the new
createRateLimitermethod.
363-397: LGTM! Cache path rate limiting implemented correctly.The rate limiting logic in the cache hit path properly:
- Checks limits after validating expiry/revoked/disabled states
- Returns appropriate rate limit metadata in both allowed and exceeded cases
- Tracks usage only after successful rate limit validation
- Uses the helper function for error result construction
473-491: LGTM! Success case properly returns rate limit metadata.The successful verification returns appropriate rate limit information alongside the valid record. The structure is consistent with the cache path implementation.
1114-1141: LGTM! Rate limiter factory method is well-implemented.The
createRateLimitermethod properly:
- Validates that cache is enabled (required for rate limiting)
- Provides a clear, actionable error message when cache is missing
- Returns a properly configured
RateLimiterinstance- Includes comprehensive documentation with usage examples
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/manager.ts (1)
130-132: Consider documenting the salt hashing approach.The salt is always hashed using SHA-256, regardless of the configured key hashing algorithm. While this may be intentional for consistency, adding a comment explaining this design decision would improve maintainability.
Example:
+ // Hash the salt with SHA-256 for consistent derivation across all algorithm choices const salt = config.salt ? hashKey(config.salt, { algorithm: "sha256" }) : "";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/manager.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/manager.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/manager.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/manager.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Prefer
Bun.fileovernode:fsreadFile/writeFile
Files:
src/manager.ts
🧬 Code graph analysis (1)
src/manager.ts (3)
src/types/audit-log-types.ts (1)
ActionContext(14-26)src/types/config-types.ts (1)
ConfigInput(36-143)src/types/error-types.ts (3)
createErrorResult(109-123)ApiKeyErrorCode(4-52)ApiKeyErrorCode(54-55)
🔇 Additional comments (7)
src/manager.ts (7)
11-11: LGTM: Correct import statements.The imports correctly distinguish between value imports (
RateLimiter) and type-only imports (RateLimitConfig), following TypeScript best practices.Also applies to: 36-36
52-64: LGTM: Well-structured rate limit metadata.The
rateLimitfield is correctly optional and includes all necessary information for clients to handle rate limiting (current count, limit, remaining requests, and reset timing).
125-125: LGTM: Properly declared rate limiter field.The field is correctly marked as
private readonlyand optional, ensuring immutability and proper encapsulation.
188-192: LGTM: Correct initialization order.The rate limiter is properly initialized after the cache, ensuring dependencies are satisfied.
363-397: LGTM: Correct rate limiting implementation in cache path.The rate limit check is properly positioned after validation and before usage tracking. The error response correctly includes rate limit metadata using
createErrorResult, and usage is only tracked when the rate limit check passes.
444-461: Excellent fix: Addresses both previous review concerns.The code now correctly:
- Caches the record before the rate limit check (preventing redundant storage hits on subsequent rate-limited requests)
- Caches only the minimal
CacheRecordstructure (avoiding exposure of sensitive metadata)This resolves the issues raised in the previous review.
1120-1147: LGTM: Well-implemented factory method.The
createRateLimitermethod correctly validates that cache is enabled before creating aRateLimiterinstance, with a clear error message guiding users to enable caching in the configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/manager.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use consecutive spaces in regular expression literals
Don't use the comma operator
Avoid functions exceeding configured Cognitive Complexity
Don't use unnecessary boolean casts
Don't use unnecessary callbacks with flatMap
Prefer for...of statements over Array.forEach
Don't create classes that only have static members
Don't use this and super in static contexts
Don't use unnecessary catch clauses
Don't use unnecessary constructors
Don't use unnecessary continue statements
Don't export empty modules
Don't use unnecessary escape sequences in regex literals
Don't use unnecessary labels
Don't use unnecessary nested block statements
Don't rename imports/exports/destructured assignments to the same name
Avoid unnecessary string or template literal concatenation
Don't use String.raw in template literals without escapes
Don't use useless case statements in switch
Avoid ternary operators when simpler alternatives exist
Don't use useless this aliasing
Don't initialize variables to undefined
Don't use the void operator
Prefer arrow functions over function expressions
Use Date.now() for epoch milliseconds
Prefer .flatMap() over map().flat() when possible
Use literal property access instead of computed when possible
Don't use parseInt/Number.parseInt for binary, octal, or hex literals
Use optional chaining instead of chained logical expressions
Prefer regex literals over RegExp constructor when possible
Don't use non-decimal or underscored number literal member names
Remove redundant terms from logical expressions
Use while loops when for initial/update are not needed
Don't reassign const variables
Don't use constant expressions in conditions
Avoid Math.min/Math.max to clamp when result is constant
Don't return a value from a constructor
Don't use empty character classes in regex literals
Don't use empty destructuring patterns
Don't call global object properties as functions
Don't declare functions/vars that escape their block (use block scoping prop...
Files:
src/manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use the arguments object
Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use any or unknown as type constraints
Don't return a value from a function with return type void
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables/params/class properties initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with ! postfix
Don't use parameter properties in class constructors
Don't use user-defined types (type aliases) where discouraged
Prefer as const over literal types/type annotations for literals
Use either T[] or Array consistently
Initialize each enum member value explicitly
Use export type for types
Use import type for types
Ensure all enum members are literal values
Don't use const enum
Don't declare empty interfaces
Don't let variables evolve into any through reassignments
Don't use the any type
Don't misuse non-null assertion operator in TS files
Don't use implicit any in variable declarations
Don't merge interfaces and classes unsafely
Place overload signatures adjacent to each other
Use namespace keyword instead of module to declare TS namespaces
Don't use void type outside generics or return types
Files:
src/manager.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx,js,ts}: Ensure preconnect is used with Google Fonts
Use a recommended display strategy with Google Fonts
Files:
src/manager.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx
Files:
src/manager.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Prefer
Bun.fileovernode:fsreadFile/writeFile
Files:
src/manager.ts
🧬 Code graph analysis (1)
src/manager.ts (5)
src/types/audit-log-types.ts (1)
ActionContext(14-26)src/types/config-types.ts (1)
ConfigInput(36-143)src/core/hash.ts (1)
hashKey(10-14)src/types/error-types.ts (3)
createErrorResult(109-123)ApiKeyErrorCode(4-52)ApiKeyErrorCode(54-55)src/utils/logger.ts (2)
logger(71-71)error(59-64)
🔇 Additional comments (3)
src/manager.ts (3)
359-397: Cache path rate limiting looks correct.The rate limit check is properly placed after validation checks, uses the centralized
createErrorResulthelper, and returns consistent rate limit metadata in both success and failure cases.
442-495: Storage path rate limiting implementation is correct.The implementation properly addresses the previous review concerns:
- Caches the minimal
CacheRecordbefore checking rate limits (lines 444-461), ensuring subsequent requests use the cache path- Uses the centralized
createErrorResulthelper for consistent error handling (line 466)- Returns rate limit metadata in both success and failure responses
The flow is consistent with the cache path and maintains the existing caching behavior.
1118-1145: Rate limiter factory method is well-implemented.The
createRateLimitermethod correctly validates that caching is enabled (required for rate limiting) and provides a clear error message. The documentation and example usage are helpful.
| const salt = config.salt | ||
| ? hashKey(config.salt, { algorithm: "sha256" }) | ||
| : ""; |
There was a problem hiding this comment.
Breaking change: Salt hashing breaks existing keys.
Hashing the salt before storing it (line 131) changes the key hash computation from hash(key + rawSalt) to hash(key + hash(rawSalt)). This breaks all existing API keys for deployments that have config.salt configured, with no migration path.
If this change is intentional (e.g., to normalize salt length/format), provide a migration guide or version detection. Otherwise, revert to the previous behavior.
Apply this diff to preserve backward compatibility:
- const salt = config.salt
- ? hashKey(config.salt, { algorithm: "sha256" })
- : "";
+ const salt = config.salt ?? "";
this.config = {
prefix: config.prefix,
// biome-ignore lint/style/noMagicNumbers: 32 characters default
length: config.length ?? 32,
algorithm: config.algorithm ?? "sha256",
alphabet: config.alphabet,
- salt,
+ salt: salt,
};Also applies to: 140-140
🤖 Prompt for AI Agents
In src/manager.ts around lines 130-132 and also at line 140, the code currently
hashes config.salt before using it which changes the computed key hash and
breaks existing keys; revert this change so the raw salt string is used exactly
as stored (use config.salt or "" when absent) in both places to preserve
backward compatibility, and if you need normalization instead implement explicit
version detection/migration rather than silently hashing the salt.
There was a problem hiding this comment.
This change exists in the upstream branch
@izadoesdev do you want this to be changed or is this intentional?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
This PR adds rate limiting support
What's New
RateLimiterclass with fixed window algorithmHow It Works
Rate limiting uses the existing cache layer to track request counts within fixed time windows. Windows are aligned to epoch time for consistent behavior across distributed systems. When the limit is reached, subsequent requests are blocked until the window resets.
Example Usage
Files Changed
src/core/rate-limiter.ts- Core rate limiter implementationsrc/types/rate-limit-types.ts- TypeScript typessrc/manager.ts- AddedcreateRateLimiter()methodsrc/index.ts- Exported rate limiter typesREADME.md- Comprehensive documentation with examplesSummary by cubic
Adds built-in rate limiting using a fixed window algorithm backed by the existing cache to prevent API abuse. Introduces a RateLimiter and a simple way to enforce per-key or custom limits with clear reset info.
New Features
Migration
Summary by CodeRabbit
New Features
Documentation
Tests
Chores