Optimization #5
Optimization #5
Conversation
…ed base adapter insert
WalkthroughRemoves the legacy monolithic adapter; introduces a Redis-backed UsageCache with Lua script, UsageTracker and WebSocket server; refactors adapters into query helpers and resolvers; updates endpoints/types/schemas and adds an async init() for optional cache/realtime wiring; adds extensive tests and build/config updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Endpoint as Consume Endpoint
participant Resolver as resolveInsertUsage
participant Cache as UsageCache
participant Redis
participant Tracker as UsageTracker
participant WSServer as WebSocket Server
Client->>Endpoint: POST /consume (referenceId, feature, amount)
Endpoint->>Resolver: resolveInsertUsage(referenceId, amount, feature)
Resolver->>Cache: insertEvent(referenceId, feature, amount)
Cache->>Redis: EVAL increment.lua (usageKey, limitKey, amount, now)
Redis-->>Cache: [newAmount, resetAt]
Cache-->>Resolver: cached_usageEvent
Resolver->>Tracker: publishUpdate(cached_usageEvent)
Tracker->>Redis: PUBLISH usage:updates:{feature}:{referenceId}
Tracker->>WSServer: broadcastUpdate(cached_usageEvent)
WSServer->>Client: emit usage:updated
sequenceDiagram
autonumber
participant Plugin
participant Cache as UsageCache
participant IO as Socket.IO Server
participant Tracker as UsageTracker
participant Redis
Plugin->>Cache: new UsageCache(redisUrl)
Plugin->>IO: create Socket.IO server (if enabled)
Plugin->>Tracker: new UsageTracker(redisUrl, IO, cache)
Tracker->>Redis: SUBSCRIBE usage:updates:*
IO->>Client: handle subscribe/get/unsubscribe via UsageWebSocketServer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
|
Note Docstrings generation - SUCCESS |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
5 similar comments
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Docstrings generation was requested by @EggerMarc. * #5 (comment) The following files were modified: * `package/adapters/queries/get-usage.ts` * `package/adapters/queries/insert-usage.ts` * `package/adapters/queries/reset-usage.ts` * `package/endpoints/consume-feature.ts` * `package/index.ts` * `package/resolvers/reset-usage.ts` * `package/utils.ts`
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…cumentation and package configuration
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Docstrings generation was requested by @coderabbitai[bot]. * #5 (comment) The following files were modified: * `package/adapters/queries/get-usage.ts` * `package/adapters/queries/insert-usage.ts` * `package/adapters/queries/reset-usage.ts` * `package/endpoints/consume-feature.ts` * `package/index.ts` * `package/resolvers/reset-usage.ts` * `package/utils.ts`
CodeRabbit Generated Unit Tests: Add comprehensive test suite with documentation and package configuration
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (9)
package/__tests__/utils.test.ts (2)
267-271: Potentially flaky test: time-dependent logic.This test uses
new Date()which makes it time-dependent. IfshouldResethas time-of-day boundaries (e.g., midnight), this test could fail intermittently.Consider using a fixed timestamp:
it("should not reset for daily when last reset was today", () => { const now = new Date(); now.setHours(12, 0, 0, 0); const result = shouldReset(now, "daily"); expect(result.shouldReset).toBe(false); });
261-265: Fix incorrect time calculation.The calculation uses 25 hours instead of 24 hours for "yesterday," making the test semantically incorrect.
- const yesterday = new Date(Date.now() - 25 * 60 * 60 * 1000); + const yesterday = new Date(Date.now() - 24 * 60 * 60 * 1000);package/__tests__/adapters/cache.test.ts (1)
17-22: Mockevalreturns hardcoded values, limiting test effectiveness.The mock always returns
[100, Date.now() + 86400000]regardless of inputs, so tests at lines 101-141 cannot verify that usage increments correctly.Consider simulating the Lua script behavior:
async eval(script: string, numKeys: number, ...args: any[]) { - const newAmount = 100; - const resetAt = Date.now() + 86400000; - return [newAmount, resetAt]; + const [usageKey, limitKey, amount] = args; + const currentUsage = JSON.parse(this.data.get(usageKey) || '{"current": 0}'); + const newAmount = currentUsage.current + Number(amount); + const resetAt = Date.now() + 86400000; + this.data.set(usageKey, JSON.stringify({ ...currentUsage, current: newAmount })); + return [newAmount, resetAt]; }package/__tests__/realtime/usage-tracker.test.ts (5)
12-30: Tests create external Redis dependency.The
UsageTrackerconstructor instantiates real Redis clients, requiring a running Redis server. This breaks test isolation and will fail in environments without Redis.Mock the Redis clients or inject mock clients:
beforeEach(() => { const mockPubClient = { connect: mock(async () => {}), publish: mock(async () => {}), quit: mock(async () => {}), psubscribe: mock(() => {}), on: mock(() => {}) }; const mockSubClient = { ...mockPubClient }; // Inject mocks into UsageTracker constructor or mock Redis module });
80-92: Test lacks assertion for channel name.The test is titled "should format channel name correctly" but performs no verification of the channel format.
Once Redis is mocked, add assertions:
await tracker.publishUpdate(update); - // Channel should be: usage:updates:storage:org-456 + expect(mockPubClient.publish).toHaveBeenCalledWith( + "usage:updates:storage:org-456", + expect.stringContaining('"feature":"storage"') + );
169-184: Test lacks assertions for Socket.IO broadcast.The test claims to verify "correct room format" but only triggers the event without verifying
mockIo.tooremitcalls.Add assertions:
tracker.emit("usage:update", update); - // Room should be: usage:api-calls:user-123 + expect(mockIo.to).toHaveBeenCalledWith("usage:api-calls:user-123"); + const roomMock = mockIo.to.mock.results[0].value; + expect(roomMock.emit).toHaveBeenCalledWith("usage:updated", update);
186-198: Test lacks assertion for event name.The test has a comment indicating the expected event name but doesn't verify it.
Add assertion:
tracker.emit("usage:update", update); - // Should emit "usage:updated" event + const roomMock = mockIo.to.mock.results[0].value; + expect(roomMock.emit).toHaveBeenCalledWith("usage:updated", expect.any(Object));
291-354: Channel naming test suite has no assertions.All three tests in this suite only have comments without any verification, providing no test value.
Once Redis is mocked, add assertions to verify channel names. For example:
await tracker.publishUpdate(update); await tracker.publishUpdate(update); - // Both should use same channel: usage:updates:api-calls:user-123 + expect(mockPubClient.publish).toHaveBeenCalledTimes(2); + expect(mockPubClient.publish).toHaveBeenNthCalledWith( + 1, + "usage:updates:api-calls:user-123", + expect.any(String) + );package/__tests__/realtime/websocket-server.test.ts (1)
124-143: Critical production bug revealed by test.The test correctly verifies error emission for nonexistent features. However, the production code (
websocket-server.tslines ~23-26) emits the error but lacks acontinuestatement, so execution continues and attempts to accessfeature.authorizeReferenceon undefined, causing a TypeError.The production code needs:
if (!feature) { socket.emit("error", { message: `Feature ${sub.feature} not found` }); continue; // Missing statement }Also enhance this test to verify the loop continues properly:
expect(mockSocket.emit).toHaveBeenCalledWith("error", { message: "Feature nonexistent not found" }); + expect(mockSocket.join).not.toHaveBeenCalled();
🧹 Nitpick comments (2)
package.json (2)
13-19: Verify bun is configured as a global tool or add to devDependencies.The package.json defines test scripts that invoke
bun test, but bun is not listed in devDependencies—only@types/bunis present. This could cause CI/CD pipelines or developers using npm/yarn to fail when running tests, unless bun is expected to be pre-installed globally.Clarify the intent:
- If bun should be a project dependency, add it to devDependencies.
- If bun is intentionally a global tool, document this requirement in a README or CONTRIBUTING guide.
If bun should be managed as a project dependency:
"devDependencies": { + "bun": "^1.1.14", "@types/bun": "^1.1.14",
22-27: Zod version is slightly behind but acceptable.The latest version of zod is 4.1.12. The package.json specifies
^4.1.9, which is behind by a few patch versions but within the acceptable range. Consider updating to^4.1.12to benefit from the latest bug fixes and improvements.Update zod to the latest version:
- "zod": "^4.1.9" + "zod": "^4.1.12"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
TESTING.md(1 hunks)package.json(2 hunks)package/__tests__/adapters/cache.test.ts(1 hunks)package/__tests__/adapters/index.test.ts(1 hunks)package/__tests__/realtime/usage-tracker.test.ts(1 hunks)package/__tests__/realtime/websocket-server.test.ts(1 hunks)package/__tests__/utils.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
package/__tests__/utils.test.ts (1)
package/utils.ts (3)
tryCatch(110-119)checkLimit(9-17)shouldReset(19-39)
package/__tests__/adapters/cache.test.ts (1)
package/adapters/cache.ts (1)
UsageCache(13-85)
package/__tests__/realtime/usage-tracker.test.ts (2)
package/realtime/usage-tracker.ts (2)
UsageTracker(15-97)UsageUpdate(6-13)package/adapters/cache.ts (1)
UsageCache(13-85)
package/__tests__/adapters/index.test.ts (2)
package/types.ts (2)
Usage(10-10)Customer(34-34)package/adapters/index.ts (1)
getUsageAdapter(7-149)
package/__tests__/realtime/websocket-server.test.ts (3)
package/realtime/websocket-server.ts (1)
UsageWebSocketServer(14-95)package/realtime/usage-tracker.ts (1)
UsageTracker(15-97)package/types.ts (1)
UsageOptions(197-209)
🪛 Biome (2.1.2)
package/__tests__/utils.test.ts
[error] 60-60: Expected a statement but instead found '})'.
Expected a statement here.
(parse)
[error] 69-69: Expected a statement but instead found '})'.
Expected a statement here.
(parse)
[error] 122-122: Expected a statement but instead found '})'.
Expected a statement here.
(parse)
[error] 201-202: Expected a statement but instead found '})'.
Expected a statement here.
(parse)
package/__tests__/adapters/cache.test.ts
[error] 269-269: expected } but instead the file ends
the file ends here
(parse)
package/__tests__/realtime/websocket-server.test.ts
[error] 443-444: Expected a statement but instead found '})'.
Expected a statement here.
(parse)
🔇 Additional comments (9)
package.json (4)
3-3: Version bump and new dependencies align well with PR objectives.The version bump to 0.1.17 is appropriate for these feature additions. The new dependencies—ioredis, socket.io, zod, and events—directly support the Redis-backed cache and real-time Socket.IO integration described in the PR summary. The added metadata (description, author, license, homepage) improves package discoverability and maintainability.
Also applies to: 8-8, 10-12, 22-27
20-21: Module configuration and exports look correct.The
"type": "module"declaration,typesfield, and export entries for the main entry point and client are well-structured and follow modern ES module conventions.Also applies to: 36-45
53-64: Keywords comprehensively cover the feature set.The expanded keywords—including "consumption", "consumption-tracking", "credits", "better-auth-plugin", and "credits-tracking"—provide good coverage for discoverability and accurately reflect the package's expanded functionality.
22-27: Update zod to version 4.1.12, the latest stable release.The latest stable Zod release is 4.1.12, but the dependency is pinned to
^4.1.9. Consider updating to4.1.12to stay current with patch fixes and improvements.TESTING.md (1)
1-60: Excellent testing documentation!The testing guide is well-structured and comprehensive, providing clear instructions for running tests, coverage commands, and a complete overview of the test suite.
package/__tests__/adapters/index.test.ts (1)
1-264: Well-structured adapter tests.The test suite effectively covers the adapter's API surface with appropriate mocks and assertions. The use of transactions and mock context is handled correctly.
package/__tests__/utils.test.ts (3)
13-13: Critical syntax error: Statement outside test block.This
expectstatement appears outside any test function and will cause a parse error. Remove this orphaned line.- expect(result).toBe("below-min-limit");Likely an incorrect or invalid review comment.
23-23: Critical syntax error: Statement outside test block.This
expectstatement appears outside any test function and will cause a parse error. Remove this orphaned line.- expect(result).toBe("in-limit");Likely an incorrect or invalid review comment.
59-59: Critical syntax error: Statement outside test block.This
expectstatement appears outside any test function and will cause a parse error. Remove this orphaned line.- expect(result).toBe("in-limit");Likely an incorrect or invalid review comment.
📝 Add docstrings to `optimization`
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
package/adapters/queries/reset-usage.ts (1)
28-80: Reset records omitreferenceTypeEvery usage record in the schema requires
referenceType, but none of the reset inserts include it and the function doesn’t accept it. This breaks parity withinsertUsageQueryand will either fail validation or leave inconsistent data. PassreferenceTypeinto the query and set it on everycreatepayload.export async function resetUsageQuery({ adapter, referenceId, + referenceType, curr, feature, }: { adapter: Adapter, referenceId: string, + referenceType: string, curr?: number, feature: Omit<Feature, "hooks"> }) { @@ - const usage = await adapter.create<Usage>({ + const usage = await adapter.create<Usage>({ model: "usage", data: { amount: feature.resetValue - curr, feature: feature.key, referenceId, + referenceType, event: "reset", @@ - const usage = await tx.create<Usage>({ + const usage = await tx.create<Usage>({ model: "usage", data: { amount: feature.resetValue!, feature: feature.key, event: "reset", referenceId, + referenceType, @@ - const usage = await tx.create<Usage>({ + const usage = await tx.create<Usage>({ model: "usage", data: { amount: feature.resetValue! - total, feature: feature.key, event: "reset", referenceId, + referenceType,package/endpoints/consume-feature.ts (1)
84-104: Fix tracker lookup and default to zero usageTwo blockers here: (1)
tracker.getUsagetakes(referenceId, feature)but the arguments are reversed, so every call hits the wrong Redis key and throws. (2) Both tracker and DB paths leavecurrentasnull/undefinedwhen nothing exists, so later arithmetic producesNaN, and the tracker path crashes on first use because the thrownNOT_FOUNDisn’t caught. Initializecurrentto0, call the tracker with(customer.referenceId, feature.key), and fall back to zero when nothing is stored.- let current = null; - if (tracker) { - const trackerData = await tracker.getUsage(feature.key, customer?.referenceId); - current = trackerData.current - } else { - const dbData = await adapter.getUsage({ - referenceId: customer.referenceId, - feature - }) - current = dbData?.amount - } + let current = 0; + if (tracker) { + try { + const trackerData = await tracker.getUsage(customer.referenceId, feature.key); + current = trackerData.current ?? 0; + } catch (error) { + if (error instanceof APIError && error.code === "NOT_FOUND") { + current = 0; + } else { + throw error; + } + } + } else { + const dbData = await adapter.getUsage({ + referenceId: customer.referenceId, + feature + }); + current = dbData?.amount ?? 0; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package/adapters/queries/get-usage.ts(1 hunks)package/adapters/queries/insert-usage.ts(1 hunks)package/adapters/queries/reset-usage.ts(1 hunks)package/endpoints/consume-feature.ts(3 hunks)package/index.ts(3 hunks)package/resolvers/reset-usage.ts(1 hunks)package/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package/adapters/queries/get-usage.ts
- package/adapters/queries/insert-usage.ts
- package/utils.ts
🧰 Additional context used
🧬 Code graph analysis (4)
package/index.ts (7)
package/types.ts (1)
UsageOptions(197-209)package/adapters/cache.ts (1)
UsageCache(13-85)package/realtime/usage-tracker.ts (1)
UsageTracker(15-97)package/realtime/websocket-server.ts (1)
UsageWebSocketServer(14-95)package/endpoints/consume-feature.ts (1)
getConsumeEndpoint(17-129)package/endpoints/check-usage.ts (1)
getCheckEndpoint(16-85)package/endpoints/sync-usage.ts (1)
getSyncEndpoint(18-72)
package/endpoints/consume-feature.ts (1)
package/types.ts (1)
UsageOptionsWithCache(215-218)
package/adapters/queries/reset-usage.ts (3)
package/types.ts (2)
Feature(61-131)Usage(10-10)package/index.ts (1)
usage(24-144)package/resolvers/reset-usage.ts (2)
ResetError(75-75)ResetSuccess(76-76)
package/resolvers/reset-usage.ts (3)
package/types.ts (2)
Feature(61-131)Usage(10-10)package/index.ts (1)
usage(24-144)package/adapters/queries/reset-usage.ts (2)
ResetError(85-85)ResetSuccess(86-86)
🪛 Biome (2.1.2)
package/adapters/queries/reset-usage.ts
[error] 86-86: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
package/resolvers/reset-usage.ts
[error] 76-76: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
package/adapters/redis.ts (3)
4-4: Remove unused import.The
SocketServerimport is not used; all Socket.IO references in the class are commented out.Apply this diff:
-import { Server as SocketServer } from "socket.io";
15-17: Remove or complete commented-out code.Multiple commented-out features (pub/sub clients, Socket.IO integration) clutter the code. Either complete these features or remove the comments.
Also applies to: 22-22, 26-29
74-76: LGTM with minor suggestion.The factory function correctly instantiates the cache. However, consider adding JSDoc to document the expected Redis URL format and usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/adapters/redis.ts(1 hunks)
🔇 Additional comments (1)
package/adapters/redis.ts (1)
58-71: LGTM!The key resolution helpers are well-structured and follow a clear naming convention for Redis keys.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
package/realtime/__tests__/usage-tracker.test.ts (5)
12-30: Critical: Redis clients must be mocked to eliminate external dependency.The
UsageTrackerconstructor instantiates real Redis clients vianew Redis(testRedisUrl)on line 29, creating an external dependency on a running Redis server atlocalhost:6379. This breaks test isolation, causes failures in environments without Redis, and prevents verification of Redis interactions.Mock the Redis constructor using Bun's module mocking. Apply this approach:
+import Redis from "ioredis"; + describe("UsageTracker", () => { let tracker: UsageTracker; let mockIo: SocketServer; let mockCache: UsageCache; + let mockPubClient: any; + let mockSubClient: any; const testRedisUrl = "redis://localhost:6379"; beforeEach(() => { + mockPubClient = { + connect: mock(async () => {}), + publish: mock(async (channel: string, message: string) => {}), + quit: mock(async () => {}) + }; + + mockSubClient = { + connect: mock(async () => {}), + psubscribe: mock((pattern: string) => {}), + on: mock((event: string, handler: Function) => {}), + quit: mock(async () => {}) + }; + + let callCount = 0; + mock.module("ioredis", () => ({ + default: mock(() => { + // Return pubClient on first call, subClient on second + return callCount++ === 0 ? mockPubClient : mockSubClient; + }) + })); + mockIo = {This enables assertion of Redis calls and removes the external service dependency.
80-92: Missing assertion: verify the Redis publish channel name.The test title claims to verify channel formatting, and line 91 documents the expected channel
usage:updates:storage:org-456, but the test contains no assertion to validate this behavior. Currently it only checks that the operation doesn't throw.Once Redis is mocked (see previous comment), add this assertion:
await tracker.publishUpdate(update); - // Channel should be: usage:updates:storage:org-456 + expect(mockPubClient.publish).toHaveBeenCalledWith( + "usage:updates:storage:org-456", + expect.stringContaining('"feature":"storage"') + );
169-184: Missing assertions: verify Socket.IO room and event emission.The test claims to verify room format (documented on line 183 as
usage:api-calls:user-123), but contains no assertions after triggering the event on line 181. Currently it only verifies no exception is thrown.Add assertions to verify the mock Socket.IO interactions:
// Trigger internal broadcast by emitting to tracker tracker.emit("usage:update", update); - // Room should be: usage:api-calls:user-123 + expect(mockIo.to).toHaveBeenCalledWith("usage:api-calls:user-123"); + const roomMock = (mockIo.to as any).mock.results[0].value; + expect(roomMock.emit).toHaveBeenCalledWith("usage:updated", update);
186-198: Missing assertion: verify the broadcasted event name.The test claims to verify the event name (documented on line 197 as
usage:updated), but performs no validation after triggering the event on line 196.Verify the correct event name is emitted:
tracker.emit("usage:update", update); - // Should emit "usage:updated" event + const roomMock = (mockIo.to as any).mock.results[0].value; + expect(roomMock.emit).toHaveBeenCalledWith("usage:updated", expect.any(Object));
291-354: Missing assertions: entire channel naming test suite has no verification.All three tests in this suite only contain comments describing expected behavior (lines 304, 328, 352) without any assertions. These tests provide no validation of channel naming logic.
Once Redis is mocked, add assertions to each test. For example, in the first test:
await tracker.publishUpdate(update); await tracker.publishUpdate(update); - // Both should use same channel: usage:updates:api-calls:user-123 + expect(mockPubClient.publish).toHaveBeenCalledTimes(2); + expect(mockPubClient.publish).toHaveBeenNthCalledWith( + 1, + "usage:updates:api-calls:user-123", + expect.any(String) + ); + expect(mockPubClient.publish).toHaveBeenNthCalledWith( + 2, + "usage:updates:api-calls:user-123", + expect.any(String) + );Apply similar verification to the other two tests, asserting distinct channel names for different features (test 2) and different references (test 3).
package/realtime/__tests__/websocket-server.test.ts (3)
124-143: Test incomplete: Should verify join not called and no TypeError thrown.This test correctly verifies error emission for a nonexistent feature, but as noted in previous reviews, the production code has a critical bug: after emitting the error at lines 23-26 in
websocket-server.ts, there's nocontinuestatement. Execution proceeds to accessfeature.authorizeReferenceon an undefined value, causing a runtime TypeError.The test passes only because mocks don't throw. To properly validate the fix, add assertions that verify the loop continues safely without attempting to join:
await mockSocket._["subscribe:usage"](subscribeData); expect(mockSocket.emit).toHaveBeenCalledWith("error", { message: "Feature nonexistent not found" }); + expect(mockSocket.join).not.toHaveBeenCalled(); + expect(mockSocket.emit).toHaveBeenCalledWith("subscribed", { + subscriptions: subscribeData.subscriptions + });This ensures the handler emits the error, skips the invalid subscription, and still completes successfully.
Based on learnings
420-442: Critical structural error: Test outside describe block.This test is currently outside the "edge cases"
describeblock, which closed at line 418. It should be moved inside that block to maintain proper test organization.Move the test inside the "edge cases" describe block:
expect(mockSocket.join).toHaveBeenCalledTimes(1); expect(mockSocket.join).toHaveBeenCalledWith("usage:api-calls:user-123"); - }); -}); - - it("should handle concurrent subscriptions", async () => { + }); + + it("should handle concurrent subscriptions", async () => { const handler = (mockIo as any)._connectionHandler; handler(mockSocket); const subscribeData1 = { subscriptions: [ { referenceId: "user-123", feature: "api-calls", referenceType: "user" } ] }; const subscribeData2 = { subscriptions: [ { referenceId: "user-456", feature: "storage", referenceType: "user" } ] }; await Promise.all([ mockSocket._["subscribe:usage"](subscribeData1), mockSocket._["subscribe:usage"](subscribeData2) ]); expect(mockSocket.join).toHaveBeenCalledTimes(2); }); + });
444-444: Critical syntax error: Extra closing brace.This line contains an extra closing brace that causes a parse error. After moving the concurrent subscriptions test inside the "edge cases" describe block (per the previous comment), this brace becomes redundant.
Remove the extra closing brace:
expect(mockSocket.join).toHaveBeenCalledTimes(2); }); }); -});
🧹 Nitpick comments (2)
package/realtime/__tests__/usage-tracker.test.ts (2)
200-221: Consider adding assertions to verify feature-specific routing.This test creates updates for different features but doesn't verify that each feature triggered a broadcast to its own distinct room. Adding assertions would validate the feature-specific routing logic.
Once Socket.IO mocking is enhanced, consider adding:
tracker.emit("usage:update", update1); tracker.emit("usage:update", update2); + + expect(mockIo.to).toHaveBeenCalledTimes(2); + expect(mockIo.to).toHaveBeenNthCalledWith(1, "usage:api-calls:user-123"); + expect(mockIo.to).toHaveBeenNthCalledWith(2, "usage:storage:user-123");
356-409: Optional: enhance edge case tests with behavior verification.The edge case tests verify resilience (operations don't throw) but don't validate how edge cases are handled. Consider adding lightweight assertions to verify behavior, especially for the empty feature name test which could produce invalid channel names.
For example, once Redis is mocked, you could verify the empty feature name behavior:
await expect(tracker.publishUpdate(update)).resolves.not.toThrow(); + // Optionally verify the channel format for empty feature + expect(mockPubClient.publish).toHaveBeenCalledWith( + expect.stringMatching(/^usage:updates::user-123$/), + expect.any(String) + );Similarly, you could verify that large numbers and dates are properly serialized in the Redis payload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/realtime/__tests__/usage-tracker.test.ts(1 hunks)package/realtime/__tests__/websocket-server.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
package/realtime/__tests__/usage-tracker.test.ts (2)
package/realtime/usage-tracker.ts (2)
UsageTracker(15-97)UsageUpdate(6-13)package/adapters/cache.ts (1)
UsageCache(13-85)
package/realtime/__tests__/websocket-server.test.ts (3)
package/realtime/websocket-server.ts (1)
UsageWebSocketServer(14-95)package/realtime/usage-tracker.ts (1)
UsageTracker(15-97)package/types.ts (1)
UsageOptions(197-209)
🪛 Biome (2.1.2)
package/realtime/__tests__/websocket-server.test.ts
[error] 443-444: Expected a statement but instead found '})'.
Expected a statement here.
(parse)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package/adapters/cache.ts(1 hunks)package/adapters/index.ts(1 hunks)package/adapters/lua/increment.lua(1 hunks)package/endpoints/consume-feature.ts(4 hunks)package/resolvers/get-usage.ts(1 hunks)package/schema.ts(1 hunks)package/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- package/adapters/cache.ts
- package/types.ts
- package/adapters/index.ts
- package/adapters/lua/increment.lua
- package/schema.ts
🧰 Additional context used
🧬 Code graph analysis (2)
package/resolvers/get-usage.ts (2)
package/types.ts (4)
Feature(62-132)UsageOptionsWithCache(216-220)cached_Usage(225-225)Usage(11-11)package/utils.ts (1)
tryCatch(122-131)
package/endpoints/consume-feature.ts (3)
package/types.ts (1)
UsageOptionsWithCache(216-220)package/middlewares/usage.ts (1)
usageMiddleware(14-33)package/resolvers/get-usage.ts (1)
resolveGetUsage(11-37)
🔇 Additional comments (6)
package/resolvers/get-usage.ts (1)
1-9: LGTM!The imports and interface definition are clean and appropriate for the resolver's purpose.
package/endpoints/consume-feature.ts (5)
1-1: LGTM!The refactoring to accept a unified
optionsobject instead of destructured parameters is a clean improvement that aligns with the new cache/tracker architecture.Also applies to: 5-5, 16-16, 23-23
65-67: LGTM!The adapter and feature resolution correctly use the unified
optionsobject.Also applies to: 73-78
80-84: Addresses past NaN issue, but first-time usage will fail.The new approach using
resolveGetUsagecorrectly eliminates the NaN arithmetic issue flagged in the past review. However, as noted inpackage/resolvers/get-usage.ts(lines 30-34), the resolver throwsNOT_FOUNDwhen no usage record exists, which will cause the endpoint to fail with 404 on the first consumption attempt.Once the resolver is updated to return a default zero-usage object for missing records, this endpoint will function correctly for first-time usage.
86-96: LGTM!The
hooks.beforecall correctly usescurrent.amountfor safe arithmetic, and the usage object shape appears consistent with the expectedUsageDatatype.
98-103: LGTM!The
insertUsagecall correctly uses theoptions.adapterand passes the appropriate parameters.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
package/__tests__/utils.test.ts (1)
263-267: Fix potentially flaky test.Using
new Date()makes this test time-dependent and could cause intermittent failures near reset boundaries (e.g., midnight for daily resets). Use a fixed timestamp instead.Apply this diff:
it("should not reset for daily when last reset was today", () => { - const today = new Date(); + const today = new Date(); + today.setHours(12, 0, 0, 0); // Use noon to avoid boundary conditions const result = shouldReset(today, "daily"); expect(result.shouldReset).toBe(false); });package/resolvers/get-usage.ts (1)
33-41: RestorelastResetAtin the default Usage payload.The fallback branch builds an object typed as
Usagebut omitslastResetAt.usageSchemastill markslastResetAtas a requiredz.date(), so any consumer that trusts the type and dereferencesusage.lastResetAt(e.g., calling.getTime()inside reset logic) will hitundefinedat runtime. Please initialize the field when synthesizing the first-usage payload.Apply this diff:
if (!data) { return { referenceId, feature: feature.key, amount: 0, event: undefined, createdAt: new Date(), + lastResetAt: new Date(), } as Usage; }
🧹 Nitpick comments (2)
package/resolvers/get-usage.ts (1)
27-30: Improve the retrieval error message.
Failed to getgives no actionable context. Including the feature key and referenceId (and ideally the original error message) will make on-call debugging far easier, especially now that multiple data sources exist.package/resolvers/reset-usage.ts (1)
77-78: Satisfy Biome’snoEmptyInterfacerule.
ResetErrorextendsAPIErrorwithout adding members, so Biome flags it as an error. Switching to a type alias keeps the same surface while clearing the lint violation.Apply this diff:
-export interface ResetError extends APIError { }; +export type ResetError = APIError;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
package/__tests__/schema.test.ts(1 hunks)package/__tests__/utils.test.ts(1 hunks)package/endpoints/consume-feature.ts(4 hunks)package/index.ts(3 hunks)package/realtime/__tests__/usage-tracker.test.ts(1 hunks)package/realtime/__tests__/websocket-server.test.ts(1 hunks)package/realtime/usage-tracker.ts(1 hunks)package/realtime/websocket-server.ts(1 hunks)package/resolvers/get-usage.ts(1 hunks)package/resolvers/reset-usage.ts(1 hunks)package/schema.ts(1 hunks)package/types.ts(3 hunks)tsup.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package/tests/schema.test.ts
- package/realtime/tests/usage-tracker.test.ts
- package/realtime/usage-tracker.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-30T15:09:50.082Z
Learnt from: EggerMarc
PR: EggerMarc/better-auth-usage#5
File: package/adapters/index.ts:63-89
Timestamp: 2025-10-30T15:09:50.082Z
Learning: In the better-auth-usage plugin, the `referenceType` field has been removed from the usage schema. Usage records now only track `referenceId` and `feature`, not `referenceType`.
Applied to files:
package/types.tspackage/schema.tspackage/resolvers/get-usage.tspackage/resolvers/reset-usage.ts
🧬 Code graph analysis (8)
package/types.ts (3)
package/adapters/cache.ts (1)
UsageCache(22-122)package/realtime/usage-tracker.ts (1)
UsageTracker(18-107)package/schema.ts (2)
cached_usageSchema(36-44)cached_usageEventSchema(54-58)
package/realtime/websocket-server.ts (4)
package/realtime/usage-tracker.ts (1)
UsageTracker(18-107)package/types.ts (1)
UsageOptions(198-210)package/index.ts (1)
usage(24-134)package/utils.ts (1)
tryCatch(122-131)
package/index.ts (5)
package/types.ts (1)
UsageOptions(198-210)package/adapters/cache.ts (1)
UsageCache(22-122)package/realtime/usage-tracker.ts (1)
UsageTracker(18-107)package/realtime/websocket-server.ts (1)
UsageWebSocketServer(14-97)package/endpoints/consume-feature.ts (1)
getConsumeEndpoint(17-124)
package/endpoints/consume-feature.ts (3)
package/types.ts (1)
UsageOptionsWithCache(216-219)package/middlewares/usage.ts (1)
usageMiddleware(14-33)package/resolvers/get-usage.ts (1)
resolveGetUsage(13-44)
package/resolvers/get-usage.ts (3)
package/types.ts (4)
Feature(62-132)UsageOptionsWithCache(216-219)Usage(11-11)cached_Usage(225-225)package/adapters/index.ts (1)
UsageAdapter(152-152)package/utils.ts (1)
tryCatch(122-131)
package/resolvers/reset-usage.ts (2)
package/types.ts (2)
Feature(62-132)Usage(11-11)package/adapters/queries/reset-usage.ts (2)
ResetError(85-85)ResetSuccess(86-86)
package/realtime/__tests__/websocket-server.test.ts (3)
package/realtime/websocket-server.ts (1)
UsageWebSocketServer(14-97)package/realtime/usage-tracker.ts (1)
UsageTracker(18-107)package/types.ts (1)
UsageOptions(198-210)
package/__tests__/utils.test.ts (1)
package/utils.ts (3)
tryCatch(122-131)checkLimit(9-17)shouldReset(19-39)
🪛 Biome (2.1.2)
package/resolvers/reset-usage.ts
[error] 78-78: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
package/__tests__/utils.test.ts
[error] 197-198: Expected a statement but instead found '})'.
Expected a statement here.
(parse)
🔇 Additional comments (2)
package/__tests__/utils.test.ts (2)
25-67: LGTM!Comprehensive test coverage for
tryCatchwith various data types (objects, arrays, null, undefined, boolean). All assertions are correct.
69-118: LGTM!Excellent test coverage across multiple dimensions:
- Error handling (Error objects, strings, custom types, undefined/null)
- Async operations (delayed success/failure)
- Type safety and inference
- Edge cases (large data, immediate resolution)
- Boundary conditions for
checkLimit(max/min limits, undefined limits)- Reset logic for various scenarios (never, null, yesterday)
All assertions are correct and comprehensive.
Also applies to: 120-144, 146-172, 174-196, 200-243, 245-262
| describe("get:usage event", () => { | ||
| it("should return current usage", async () => { | ||
| const handler = (mockIo as any)._connectionHandler; | ||
| handler(mockSocket); | ||
|
|
||
| const getData = { | ||
| referenceId: "user-123", | ||
| feature: "api-calls" | ||
| }; | ||
|
|
||
| await mockSocket._["get:usage"](getData); | ||
|
|
||
| expect(mockTracker.getUsage).toHaveBeenCalledWith("user-123", "api-calls"); | ||
| expect(mockSocket.emit).toHaveBeenCalledWith("usage:current", expect.any(Object)); | ||
| }); | ||
|
|
||
| it("should emit error when tracker fails", async () => { | ||
| mockTracker.getUsage = mock(async () => { | ||
| throw new Error("Tracker error"); | ||
| }); | ||
|
|
||
| const handler = (mockIo as any)._connectionHandler; | ||
| handler(mockSocket); | ||
|
|
||
| const getData = { | ||
| referenceId: "user-123", | ||
| feature: "api-calls" | ||
| }; | ||
|
|
||
| await mockSocket._["get:usage"](getData); | ||
|
|
||
| expect(mockSocket.emit).toHaveBeenCalledWith("usage:error", { | ||
| error: "Failed to fetch usage" | ||
| }); | ||
| }); | ||
|
|
||
| it("should handle requests for different features", async () => { | ||
| const handler = (mockIo as any)._connectionHandler; | ||
| handler(mockSocket); | ||
|
|
||
| await mockSocket._["get:usage"]({ referenceId: "user-123", feature: "api-calls" }); | ||
| await mockSocket._["get:usage"]({ referenceId: "user-123", feature: "storage" }); | ||
|
|
||
| expect(mockTracker.getUsage).toHaveBeenCalledTimes(2); | ||
| }); |
There was a problem hiding this comment.
Make the tracker mock enforce the real feature contract.
The production tracker requires a Feature object (it dereferences feature.key). This test (and the mockTracker above) accepts a plain string, so it can’t catch the runtime crash fixed in the server file. Update the mock and expectations to require the feature object.
- mockTracker = {
- getUsage: mock(async (referenceId: string, feature: string) => ({
- referenceId,
- feature,
+ mockTracker = {
+ getUsage: mock(async (referenceId: string, feature: { key: string }) => ({
+ referenceId,
+ feature: feature.key,
@@
- expect(mockTracker.getUsage).toHaveBeenCalledWith("user-123", "api-calls");
+ expect(mockTracker.getUsage).toHaveBeenCalledWith(
+ "user-123",
+ mockOptions.features["api-calls"]
+ );This will ensure the test suite fails if the handler ever regresses to passing the bare feature key again.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package/realtime/__tests__/websocket-server.test.ts around lines 271 to 315,
the test currently passes a plain string for the feature but the real tracker
expects a Feature object (accessing feature.key); update the test to pass
objects like { key: "api-calls" } and { key: "storage" }, update the
mockTracker.getUsage signature/expectations to assert it was called with
("user-123", { key: "api-calls" }) (and similarly for storage), and change the
error-emission expectation to match the new shape only if needed; ensure all
calls to mockSocket._["get:usage"] in these tests supply feature objects instead
of strings so the mock enforces the real contract.
| socket.on("get:usage", async (data: { | ||
| referenceId: string, | ||
| feature: string | ||
| }) => { | ||
|
|
||
| const { data: usage, error } = await tryCatch(this.tracker.getUsage(data.referenceId, data.feature)) | ||
| if (error) { | ||
| socket.emit("usage:error", { | ||
| error: "Failed to fetch usage" | ||
| }); | ||
| return; | ||
| } | ||
| socket.emit("usage:current", usage); | ||
| }); |
There was a problem hiding this comment.
Pass the full feature into tracker.getUsage.
UsageTracker.getUsage (and the underlying UsageCache.getUsage) expect a feature object so they can read feature.key. Passing the raw string forces feature.key to be undefined, which will throw as soon as realtime mode is enabled. Guard the feature lookup and forward the resolved Feature instead.
socket.on("get:usage", async (data: {
referenceId: string,
feature: string
}) => {
-
- const { data: usage, error } = await tryCatch(this.tracker.getUsage(data.referenceId, data.feature))
+ const feature = this.options.features[data.feature];
+ if (!feature) {
+ socket.emit("usage:error", {
+ error: `Feature ${data.feature} not found`
+ });
+ return;
+ }
+
+ const { data: usage, error } = await tryCatch(
+ this.tracker.getUsage(data.referenceId, feature)
+ );
if (error) {
socket.emit("usage:error", {
error: "Failed to fetch usage"
});
return;
}
socket.emit("usage:current", usage);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| socket.on("get:usage", async (data: { | |
| referenceId: string, | |
| feature: string | |
| }) => { | |
| const { data: usage, error } = await tryCatch(this.tracker.getUsage(data.referenceId, data.feature)) | |
| if (error) { | |
| socket.emit("usage:error", { | |
| error: "Failed to fetch usage" | |
| }); | |
| return; | |
| } | |
| socket.emit("usage:current", usage); | |
| }); | |
| socket.on("get:usage", async (data: { | |
| referenceId: string, | |
| feature: string | |
| }) => { | |
| const feature = this.options.features[data.feature]; | |
| if (!feature) { | |
| socket.emit("usage:error", { | |
| error: `Feature ${data.feature} not found` | |
| }); | |
| return; | |
| } | |
| const { data: usage, error } = await tryCatch( | |
| this.tracker.getUsage(data.referenceId, feature) | |
| ); | |
| if (error) { | |
| socket.emit("usage:error", { | |
| error: "Failed to fetch usage" | |
| }); | |
| return; | |
| } | |
| socket.emit("usage:current", usage); | |
| }); |
🤖 Prompt for AI Agents
In package/realtime/websocket-server.ts around lines 77–90, the code passes
data.feature (a string) into tracker.getUsage, but getUsage expects a Feature
object with a key property; resolve the Feature object first and pass that
instead. Modify the handler to look up the feature by key (e.g. via the server's
feature store/registry or a helper like getFeatureByKey(data.feature)), check
for a missing feature and emit a clear "usage:error" and return if not found,
then call this.tracker.getUsage(data.referenceId, feature) with the resolved
Feature object and keep the existing try/catch and emits for success/error.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
package/__tests__/utils.test.ts(1 hunks)package/adapters/cache.ts(1 hunks)package/adapters/index.ts(1 hunks)package/endpoints/consume-feature.ts(4 hunks)package/endpoints/upsert-customer.ts(3 hunks)package/index.ts(3 hunks)package/realtime/__tests__/usage-tracker.test.ts(1 hunks)package/realtime/websocket-server.ts(1 hunks)package/resolvers/get-usage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package/tests/utils.test.ts
- package/realtime/websocket-server.ts
- package/adapters/index.ts
- package/endpoints/upsert-customer.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-30T15:09:50.082Z
Learnt from: EggerMarc
PR: EggerMarc/better-auth-usage#5
File: package/adapters/index.ts:63-89
Timestamp: 2025-10-30T15:09:50.082Z
Learning: In the better-auth-usage plugin, the `referenceType` field has been removed from the usage schema. Usage records now only track `referenceId` and `feature`, not `referenceType`.
Applied to files:
package/resolvers/get-usage.ts
🧬 Code graph analysis (5)
package/realtime/__tests__/usage-tracker.test.ts (2)
package/realtime/usage-tracker.ts (2)
UsageTracker(18-107)UsageUpdate(9-16)package/adapters/cache.ts (1)
UsageCache(24-138)
package/endpoints/consume-feature.ts (3)
package/types.ts (1)
UsageOptionsWithCache(216-219)package/middlewares/usage.ts (1)
usageMiddleware(14-33)package/resolvers/get-usage.ts (1)
resolveGetUsage(13-68)
package/adapters/cache.ts (3)
package/utils.ts (1)
tryCatch(122-131)package/types.ts (3)
Feature(62-132)Usage(11-11)Customer(35-35)package/schema.ts (2)
usageSchema(27-34)customerSchema(3-18)
package/resolvers/get-usage.ts (3)
package/types.ts (4)
Feature(62-132)UsageOptionsWithCache(216-219)Usage(11-11)cached_Usage(225-225)package/adapters/index.ts (1)
UsageAdapter(170-170)package/utils.ts (1)
tryCatch(122-131)
package/index.ts (8)
package/types.ts (2)
UsageOptions(198-210)UsageOptionsWithCache(216-219)package/adapters/cache.ts (1)
UsageCache(24-138)package/realtime/usage-tracker.ts (1)
UsageTracker(18-107)package/realtime/websocket-server.ts (1)
UsageWebSocketServer(14-97)package/endpoints/consume-feature.ts (1)
getConsumeEndpoint(17-125)package/endpoints/check-usage.ts (1)
getCheckEndpoint(16-85)package/endpoints/upsert-customer.ts (1)
getUpsertCustomerEndpoint(14-51)package/endpoints/sync-usage.ts (1)
getSyncEndpoint(18-72)
| async getCustomer(referenceId: string): Promise<Customer> { | ||
| const { data, error } = await tryCatch(this.cache.get(`customer:${referenceId}`)) | ||
|
|
||
| if (error) { | ||
| throw new APIError("INTERNAL_SERVER_ERROR", { message: `[ERROR][USAGE] Failed to get customer from cache for ${referenceId}` }) | ||
| } | ||
|
|
||
| return customerSchema.parse(data) | ||
| } |
There was a problem hiding this comment.
Cached customer reads always fail
Redis#get returns a JSON string, but customerSchema.parse is being fed that raw string. The schema expects an object, so every cached customer lookup now throws and the cache becomes useless. Please JSON.parse the value (and handle parse errors) before validation.
🤖 Prompt for AI Agents
In package/adapters/cache.ts around lines 125 to 133, the code passes the raw
Redis string to customerSchema.parse which causes validation to always fail;
instead, JSON.parse the returned data before validating and handle parse
failures: if data is null/undefined keep current behavior or throw a suitable
APIError, otherwise wrap JSON.parse in try/catch and on parse error throw an
APIError("INTERNAL_SERVER_ERROR") that includes a clear message with the
referenceId and the parse error, then pass the parsed object to
customerSchema.parse.
| getFeature: getFeatureEndpoint(options), | ||
| /** | ||
| * Consume (meter) a feature for a given referenceId. | ||
| * - runs before hook | ||
| * - inserts usage row (adapter) | ||
| * - runs after hook | ||
| */ | ||
| consumeFeature: getConsumeEndpoint(options), | ||
|
|
||
| consumeFeature: getConsumeEndpoint({ | ||
| ...options, | ||
| cache, | ||
| tracker, | ||
| }), | ||
|
|
||
| listFeatures: getFeaturesEndpoint(options), | ||
| /** | ||
| * Check usage limit for a feature for a specific reference. | ||
| * Returns a small enum ("in-limit"|"above-limit"|"below-limit") based on checkLimit util. | ||
| */ | ||
| checkUsage: getCheckEndpoint(options), | ||
|
|
||
| checkUsage: getCheckEndpoint({ | ||
| ...options, | ||
| cache | ||
| }), | ||
|
|
||
| upsertCustomer: getUpsertCustomerEndpoint(), | ||
| /** | ||
| * Sync usage according to feature.reset rules. | ||
| * This will insert a reset event row with zeroed usage if the feature requires it. | ||
| * | ||
| * Note: you might prefer running this as a background job for many customers, | ||
| * rather than via an endpoint. | ||
| */ | ||
| syncUsage: getSyncEndpoint(options) | ||
| } | ||
|
|
||
| syncUsage: getSyncEndpoint({ | ||
| ...options, | ||
| cache, | ||
| tracker | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Endpoints still see undefined cache/tracker
Even though you introduced runtimeOptions, the endpoints are instantiated with { ...options, cache, tracker } while both variables are still undefined. Those values are captured at construction time and never updated, so every endpoint keeps running in DB-only mode. Please wire the endpoints to runtimeOptions (or similar mutable reference) so they observe the initialized cache/tracker instances.
🤖 Prompt for AI Agents
In package/index.ts around lines 112 to 134 the endpoints are being instantiated
with "{ ...options, cache, tracker }" where cache and tracker are still
undefined at construction time; this locks every endpoint into DB-only mode.
Replace those direct undefined captures by wiring the endpoints to the mutable
runtimeOptions reference (or a getter) so they access runtimeOptions.cache and
runtimeOptions.tracker at runtime; specifically, call getConsumeEndpoint,
getCheckEndpoint and getSyncEndpoint with an object that reads from
runtimeOptions (or pass runtimeOptions itself) so the endpoints observe the
initialized cache/tracker instances rather than the undefined locals.
| // Mock Redis constructor | ||
| mock.module("ioredis", () => ({ | ||
| default: mock(() => mockPubClient) // alternate returning pub/sub clients as needed | ||
| })); | ||
| mockIo = { | ||
| to: mock((room: string) => ({ | ||
| emit: mock((event: string, data: any) => { }) | ||
| })) |
There was a problem hiding this comment.
Redis mock never provides a sub client
mock.module("ioredis", …) always hands back mockPubClient, which lacks psubscribe/on. When UsageTracker constructs the second client it still receives mockPubClient, so tracker.connect() blows up. You likely meant to return mockPubClient for the first instantiation and mockSubClient for the second. Please adjust the mock so the tracker gets distinct pub/sub clients with the expected APIs.
🤖 Prompt for AI Agents
In package/realtime/__tests__/usage-tracker.test.ts around lines 28–35, the
ioredis mock always returns mockPubClient so the second Redis instantiation
(subscriber) never receives a client with psubscribe/on and tracker.connect()
fails; change the mock.module("ioredis", ...) to return distinct pub and sub
clients (e.g., have the mocked default constructor be a factory that returns
mockPubClient on first call and mockSubClient on second, or use sequential mock
implementations like mockImplementationOnce) so the UsageTracker gets a pub
client with emit and a sub client with psubscribe/on.
package/resolvers/get-usage.ts
Outdated
| return { | ||
| referenceId, | ||
| feature: feature.key, | ||
| amount: 0, | ||
| event: undefined, | ||
| createdAt: new Date(), | ||
| } as Usage; |
There was a problem hiding this comment.
Default usage object violates schema contract
The fallback usage you return is missing lastResetAt, yet usageSchema still requires that field. Downstream code (hooks, serializers, schema validation) will blow up on the first read. Please populate lastResetAt with an appropriate default (e.g. null if the schema allows it, otherwise a sentinel date) instead of returning an incomplete Usage.
🤖 Prompt for AI Agents
In package/resolvers/get-usage.ts around lines 58 to 64, the fallback Usage
returned omits the required lastResetAt field; update the returned object to
include lastResetAt set to an appropriate default (use null if the usageSchema
allows null, otherwise use a sentinel Date like new Date(0) or createdAt) so the
object fully satisfies the schema and downstream validators/hooks won't fail.
…e actions in their named scope
…olvers and adapters
Docstrings generation was requested by @EggerMarc. * #10 (comment) The following files were modified: * `package/adapters/queries/get-usage.ts` * `package/adapters/queries/insert-usage.ts` * `package/adapters/queries/reset-usage.ts` * `package/endpoints/check-usage.ts` * `package/endpoints/consume-feature.ts` * `package/endpoints/sync-usage.ts` * `package/resolvers/get-customer.ts` * `package/resolvers/get-usage.ts` * `package/resolvers/reset-usage.ts` * `package/resolvers/sync-usage.ts` * `package/utils.ts`
📝 Add docstrings to `optimization-refactor`
CodeRabbit Generated Unit Tests: Add comprehensive test suite for resolvers and adapters
…endpoints. multiple error handling fixes
Set out scope of control for each layer of abstraction
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (4)
package/adapters/queries/reset-usage.ts (1)
27-82: Handle zero reset values correctly.Line 27’s truthy guard exits when
feature.resetValueis0, so features that legitimately reset to zero never record a reset entry. Similarly, Line 31 skips the direct path whencurris0. Both are valid inputs and should be processed.- if (!feature.resetValue) { + if (feature.resetValue === undefined || feature.resetValue === null) { return } - if (curr) { + const resetValue = feature.resetValue; + + if (curr !== undefined) { const usage = await adapter.create<Usage>({ model: "usage", data: { - amount: feature.resetValue - curr, + amount: resetValue - curr, feature: feature.key, referenceId, event: "reset", lastResetAt: new Date(), createdAt: new Date() } }) return usage } const transaction = await adapter.transaction(async (tx) => { const currentUsage = await tx.findMany<Usage>({ model: "usage", where: [ { field: "referenceId", value: referenceId }, { field: "feature", value: feature.key }, ], sortBy: { field: "createdAt", direction: "desc" } }) if (currentUsage.length === 0) { const usage = await tx.create<Usage>({ model: "usage", data: { - amount: feature.resetValue!, + amount: resetValue, feature: feature.key, event: "reset", referenceId, lastResetAt: new Date(), createdAt: new Date() } }) return usage } - const total = currentUsage.reduce((curr, { amount }) => amount + curr, 0) + const total = currentUsage.reduce((sum, { amount }) => sum + amount, 0) const usage = await tx.create<Usage>({ model: "usage", data: { - amount: feature.resetValue! - total, + amount: resetValue - total, feature: feature.key, event: "reset", referenceId, lastResetAt: new Date(), createdAt: new Date() } }) return usage }) return transaction }package/index.ts (1)
119-135: Critical: Endpoints still receive undefined cache/tracker.Despite the past review comment being marked as "Addressed in commit d29897f", the issue persists. The endpoints at lines 119-135 are constructed with the local
cacheandtrackervariables (which areundefinedat plugin construction time) instead of reading fromruntimeOptionswhere they are set duringinit().Result: All endpoints run in DB-only mode even when cache/realtime are configured.
Apply this fix:
endpoints: { getFeature: getFeatureEndpoint(options), consumeFeature: getConsumeEndpoint({ - ...options, - cache, - tracker, + options: runtimeOptions, + adapter: serverAdapter }), listFeatures: getFeaturesEndpoint(options), checkUsage: getCheckEndpoint({ - ...options, - cache + options: runtimeOptions, + adapter: serverAdapter }), upsertCustomer: getUpsertCustomerEndpoint(options), syncUsage: getSyncEndpoint({ - ...options, - cache, - tracker + options: runtimeOptions, + adapter: serverAdapter }) },Note: This assumes the endpoint signatures have been updated to accept
{ options, adapter }(as shown in the AI summary for consume-feature.ts). If not, the endpoint signatures must also be updated to matchEndpointParams.package/adapters/cache.ts (2)
131-139: Parse and validate cached customers correctly.Redis returns a JSON string (or null). Passing that raw string to
customerSchema.parsealways throws, so every cached lookup fails. Also, a missing key becomes a bare ZodError. Parse the JSON, handle the null case, and wrap parse failures in anAPIError.- const { data, error } = await tryCatch(this.cache.get(`customer:${referenceId}`)) - - if (error) { - throw new APIError("INTERNAL_SERVER_ERROR", { message: `[ERROR][USAGE] Failed to get customer from cache for ${referenceId}` }) - } - - return customerSchema.parse(data) + const { data, error } = await tryCatch(this.cache.get(`customer:${referenceId}`)); + + if (error) { + throw new APIError("INTERNAL_SERVER_ERROR", { + message: `[ERROR][USAGE] Failed to get customer from cache for ${referenceId}`, + }); + } + + if (!data) { + throw new APIError("NOT_FOUND", { + message: `[ERROR][USAGE] Customer ${referenceId} not found in cache`, + }); + } + + try { + return customerSchema.parse(JSON.parse(data)); + } catch (err) { + throw new APIError("INTERNAL_SERVER_ERROR", { + message: `[ERROR][USAGE] Corrupted customer cache entry for ${referenceId}: ${(err as Error).message ?? err}`, + }); + }
34-38: Don’t throw from the Redis error event.Throwing an
APIErrorinside the'error'listener runs outside the call stack and will crash the process. Emit/log the failure instead so callers can decide how to react.- this.cache.on('error', (err) => { - throw new APIError("INTERNAL_SERVER_ERROR", { - message: `Failed to initialize cache with error: ${err}` - }) - }); + this.cache.on("error", (err) => { + console.error("[ERROR][USAGE] Redis connection error:", err); + this.emit("error", err); + });
🧹 Nitpick comments (3)
package/types.ts (1)
201-209: Consider making redisUrl and port required when enableRealtime is true.The current structure allows
enableRealtime: truewithout providingredisUrlorport, which would cause runtime errors. Consider using discriminated unions or validation to enforce this constraint.Apply this approach:
- cacheOptions?: { - enableRealtime?: boolean, - redisUrl: string; - port: number; + cacheOptions?: + | { + enableRealtime: true; + redisUrl: string; + port: number; cors?: { origin: string | string[]; credentials?: boolean; }; - }, + } + | { + enableRealtime?: false; + redisUrl: string; + port: number; + cors?: { + origin: string | string[]; + credentials?: boolean; + }; + },package/adapters/queries/get-usage.ts (1)
54-54: Simplify the reduce operation.The current reduce can be made more concise and clearer.
Apply this diff:
- const current = usage.reduce((value, { amount }) => amount + value, 0) + const current = usage.reduce((sum, { amount }) => sum + amount, 0)package/resolvers/get-customer.ts (1)
49-53: Consider using structured logging instead of console.log.The cache write error is only logged to console. Consider using a structured logger for better observability in production environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
package/adapters/__tests__/index.test.ts(1 hunks)package/adapters/cache.ts(1 hunks)package/adapters/index.ts(1 hunks)package/adapters/lua/set-limit.lua(1 hunks)package/adapters/queries/get-customer.ts(1 hunks)package/adapters/queries/get-latest-usage.ts(1 hunks)package/adapters/queries/get-usage.ts(1 hunks)package/adapters/queries/insert-usage.ts(1 hunks)package/adapters/queries/reset-usage.ts(1 hunks)package/adapters/queries/upsert-customer.ts(1 hunks)package/endpoints/check-usage.ts(3 hunks)package/endpoints/consume-feature.ts(2 hunks)package/endpoints/sync-usage.ts(3 hunks)package/endpoints/upsert-customer.ts(3 hunks)package/index.ts(3 hunks)package/middlewares/customer.ts(1 hunks)package/middlewares/usage.ts(1 hunks)package/realtime/usage-tracker.ts(1 hunks)package/resolvers/__tests__/get-customer.test.ts(1 hunks)package/resolvers/__tests__/get-usage.test.ts(1 hunks)package/resolvers/__tests__/upsert-customer.test.ts(1 hunks)package/resolvers/get-customer.ts(1 hunks)package/resolvers/get-usage.ts(1 hunks)package/resolvers/insert-usage.ts(1 hunks)package/resolvers/reset-usage.ts(1 hunks)package/resolvers/sync-usage.ts(1 hunks)package/resolvers/upsert-customer.ts(1 hunks)package/schema.ts(1 hunks)package/types.ts(3 hunks)package/utils.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- package/adapters/lua/set-limit.lua
🚧 Files skipped from review as they are similar to previous changes (3)
- package/schema.ts
- package/resolvers/get-usage.ts
- package/adapters/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-30T15:09:50.082Z
Learnt from: EggerMarc
Repo: EggerMarc/better-auth-usage PR: 5
File: package/adapters/index.ts:63-89
Timestamp: 2025-10-30T15:09:50.082Z
Learning: In the better-auth-usage plugin, the `referenceType` field has been removed from the usage schema. Usage records now only track `referenceId` and `feature`, not `referenceType`.
Applied to files:
package/middlewares/usage.tspackage/adapters/queries/get-usage.tspackage/adapters/queries/get-latest-usage.tspackage/adapters/queries/reset-usage.tspackage/types.tspackage/resolvers/sync-usage.tspackage/adapters/queries/insert-usage.ts
🧬 Code graph analysis (26)
package/middlewares/usage.ts (1)
package/types.ts (1)
UsageOptions(198-210)
package/adapters/__tests__/index.test.ts (2)
package/adapters/index.ts (1)
getUsageAdapter(10-68)package/types.ts (2)
Feature(62-132)Customer(35-35)
package/adapters/queries/upsert-customer.ts (1)
package/types.ts (1)
Customer(35-35)
package/resolvers/upsert-customer.ts (3)
package/adapters/index.ts (1)
UsageAdapter(70-70)package/types.ts (2)
UsageOptionsWithCache(216-219)Customer(35-35)package/utils.ts (1)
tryCatch(122-131)
package/resolvers/__tests__/get-usage.test.ts (3)
package/adapters/index.ts (1)
UsageAdapter(70-70)package/types.ts (3)
UsageOptionsWithCache(216-219)Feature(62-132)Usage(11-11)package/resolvers/get-usage.ts (1)
resolveGetUsage(26-88)
package/endpoints/upsert-customer.ts (4)
package/types.ts (1)
UsageOptionsWithCache(216-219)package/adapters/index.ts (1)
getUsageAdapter(10-68)package/utils.ts (1)
tryCatch(122-131)package/resolvers/upsert-customer.ts (1)
resolveUpsertCustomer(12-27)
package/adapters/queries/get-usage.ts (3)
package/types.ts (2)
Feature(62-132)Usage(11-11)package/index.ts (1)
usage(25-137)package/utils.ts (1)
tryCatch(122-131)
package/resolvers/get-customer.ts (3)
package/adapters/index.ts (1)
UsageAdapter(70-70)package/types.ts (2)
UsageOptionsWithCache(216-219)Customer(35-35)package/utils.ts (1)
tryCatch(122-131)
package/resolvers/insert-usage.ts (6)
package/types.ts (2)
Feature(62-132)UsageOptionsWithCache(216-219)package/adapters/index.ts (1)
UsageAdapter(70-70)package/utils.ts (1)
tryCatch(122-131)package/resolvers/get-usage.ts (1)
resolveGetUsage(26-88)package/resolvers/get-customer.ts (1)
resolveGetCustomer(22-56)package/resolvers/sync-usage.ts (1)
resolveSyncUsage(23-76)
package/adapters/queries/get-latest-usage.ts (1)
package/types.ts (2)
Feature(62-132)Usage(11-11)
package/resolvers/reset-usage.ts (3)
package/types.ts (2)
Feature(62-132)UsageOptionsWithCache(216-219)package/resolvers/get-usage.ts (1)
resolveGetUsage(26-88)package/utils.ts (1)
shouldReset(19-39)
package/middlewares/customer.ts (3)
package/types.ts (1)
UsageOptionsWithCache(216-219)package/adapters/index.ts (1)
UsageAdapter(70-70)package/resolvers/get-customer.ts (1)
resolveGetCustomer(22-56)
package/endpoints/consume-feature.ts (5)
package/types.ts (1)
EndpointParams(224-227)package/middlewares/usage.ts (1)
getUsageMiddleware(14-33)package/middlewares/customer.ts (1)
getCustomerMiddleware(18-29)package/utils.ts (1)
tryCatch(122-131)package/resolvers/insert-usage.ts (1)
resolveInsertUsage(18-112)
package/adapters/queries/reset-usage.ts (2)
package/types.ts (2)
Feature(62-132)Usage(11-11)package/index.ts (1)
usage(25-137)
package/resolvers/__tests__/get-customer.test.ts (3)
package/adapters/index.ts (1)
UsageAdapter(70-70)package/types.ts (2)
UsageOptionsWithCache(216-219)Customer(35-35)package/resolvers/get-customer.ts (1)
resolveGetCustomer(22-56)
package/endpoints/sync-usage.ts (5)
package/types.ts (1)
EndpointParams(224-227)package/middlewares/customer.ts (1)
getCustomerMiddleware(18-29)package/index.ts (1)
usage(25-137)package/utils.ts (1)
tryCatch(122-131)package/resolvers/sync-usage.ts (1)
resolveSyncUsage(23-76)
package/types.ts (4)
package/adapters/cache.ts (1)
UsageCache(25-153)package/realtime/usage-tracker.ts (1)
UsageTracker(10-84)package/adapters/index.ts (1)
UsageAdapter(70-70)package/schema.ts (3)
cached_usageSchema(36-44)cached_usageEventSchema(56-61)cached_limitsSchema(46-54)
package/resolvers/sync-usage.ts (4)
package/types.ts (2)
Feature(62-132)UsageOptionsWithCache(216-219)package/adapters/index.ts (1)
UsageAdapter(70-70)package/resolvers/get-usage.ts (1)
resolveGetUsage(26-88)package/utils.ts (1)
shouldReset(19-39)
package/index.ts (9)
package/types.ts (2)
UsageOptions(198-210)UsageOptionsWithCache(216-219)package/adapters/cache.ts (1)
UsageCache(25-153)package/realtime/usage-tracker.ts (1)
UsageTracker(10-84)package/realtime/websocket-server.ts (1)
UsageWebSocketServer(14-97)package/adapters/index.ts (2)
UsageAdapter(70-70)getUsageAdapter(10-68)package/endpoints/consume-feature.ts (1)
getConsumeEndpoint(17-93)package/endpoints/check-usage.ts (1)
getCheckEndpoint(17-93)package/endpoints/upsert-customer.ts (1)
getUpsertCustomerEndpoint(16-71)package/endpoints/sync-usage.ts (1)
getSyncEndpoint(16-72)
package/adapters/queries/get-customer.ts (1)
package/types.ts (1)
Customer(35-35)
package/endpoints/check-usage.ts (6)
package/types.ts (1)
EndpointParams(224-227)package/middlewares/usage.ts (1)
getUsageMiddleware(14-33)package/middlewares/customer.ts (1)
getCustomerMiddleware(18-29)package/index.ts (1)
usage(25-137)package/utils.ts (2)
tryCatch(122-131)checkLimit(9-17)package/resolvers/get-usage.ts (1)
resolveGetUsage(26-88)
package/adapters/queries/insert-usage.ts (2)
package/types.ts (2)
Feature(62-132)Usage(11-11)package/adapters/queries/get-usage.ts (1)
getUsageQuery(11-59)
package/realtime/usage-tracker.ts (4)
package/adapters/cache.ts (1)
UsageCache(25-153)package/schema.ts (1)
cached_usageEventSchema(56-61)package/types.ts (1)
cached_UsageEvent(233-233)package/utils.ts (1)
tryCatch(122-131)
package/adapters/cache.ts (3)
package/utils.ts (1)
tryCatch(122-131)package/types.ts (4)
Feature(62-132)Usage(11-11)Customer(35-35)cached_Limits(234-234)package/schema.ts (2)
usageSchema(27-34)customerSchema(3-18)
package/resolvers/__tests__/upsert-customer.test.ts (3)
package/adapters/index.ts (1)
UsageAdapter(70-70)package/types.ts (2)
UsageOptionsWithCache(216-219)Customer(35-35)package/resolvers/upsert-customer.ts (1)
resolveUpsertCustomer(12-27)
package/utils.ts (1)
package/types.ts (2)
cached_Usage(232-232)Usage(11-11)
🔇 Additional comments (10)
package/adapters/queries/get-customer.ts (1)
5-18: Query implementation looks solid.Line 12 cleanly wraps the adapter lookup with the expected selector, and the return value is passed through without extra work.
package/adapters/queries/get-latest-usage.ts (1)
11-43: Helper reads cleanly.Line 33 correctly orders results before returning the first entry, keeping the query helper straightforward.
package/types.ts (1)
229-234: LGTM!The cached type aliases properly derive from their corresponding Zod schemas and provide a clean API surface for cached data structures.
package/resolvers/__tests__/get-customer.test.ts (1)
1-211: Excellent test coverage!The test suite comprehensively covers:
- Cache hit/miss scenarios
- Error handling for both cache and DB layers
- Edge cases with special characters and minimal data
- Graceful handling of cache write failures
The use of mocks and beforeEach setup is clean and maintainable.
package/adapters/queries/get-usage.ts (1)
35-50: Consider the implications of silently failing initial usage creation.When no usage records exist, the function creates an initial usage record but does so non-blocking (lines 46-48). If this creation fails, the returned
initialUsagewon't exist in the database, potentially causing inconsistencies in subsequent queries.Consider whether this should be a blocking operation or if there should be additional retry/recovery logic.
package/resolvers/get-customer.ts (1)
22-56: Well-structured resolver with proper error handling!The resolver correctly:
- Attempts cache read first when available
- Falls back to DB on cache miss
- Handles errors with appropriate APIError types
- Performs best-effort cache write after DB read
package/adapters/__tests__/index.test.ts (1)
1-342: Comprehensive adapter test suite!The tests effectively cover:
- All adapter methods (getLatestUsage, resetUsage, insertUsage, getCustomer, upsertCustomer, getUsage)
- Happy paths with valid data
- Edge cases (empty data, special parameters)
- Error scenarios (transaction rollback, DB failures)
- Concurrent operations
The mock setup and assertions are well-structured.
package/endpoints/upsert-customer.ts (2)
53-68: Well-refactored endpoint using the resolver pattern!The endpoint now properly:
- Uses the resolver abstraction layer
- Wraps calls in tryCatch for consistent error handling
- Structures the customer payload explicitly
- Provides detailed error messages
This aligns well with the broader PR refactoring towards a resolver-based architecture.
44-46: LGTM on OpenAPI metadata update.The addition of the 500 error response properly documents the error contract for API consumers.
package/middlewares/usage.ts (1)
14-14: All imports have been correctly updated to the new function name.The verification confirms no remaining references to the old
usageMiddlewarename exist in the codebase. The newgetUsageMiddlewarefunction is properly imported and used in bothcheck-usage.tsandconsume-feature.ts.
| if (existingCustomer) { | ||
| await tx.update<Customer>({ | ||
| model: "customer", | ||
| where: [{ field: "referenceId", value: customer.referenceId }], | ||
| update: customer, | ||
| }); | ||
| } else { | ||
| await tx.create<Customer>({ | ||
| model: "customer", | ||
| data: customer, | ||
| }); | ||
| } | ||
| return customer | ||
| }); | ||
| return upsertedCustomer; |
There was a problem hiding this comment.
Return the persisted customer record from the transaction.
Lines 28-29 return the inbound customer, so generated fields (IDs, timestamps, defaults) or DB-side mutations never reach callers. Fetch and return the record actually stored in the database before leaving the transaction.
- if (existingCustomer) {
- await tx.update<Customer>({
- model: "customer",
- where: [{ field: "referenceId", value: customer.referenceId }],
- update: customer,
- });
- } else {
- await tx.create<Customer>({
- model: "customer",
- data: customer,
- });
- }
- return customer
+ if (existingCustomer) {
+ await tx.update<Customer>({
+ model: "customer",
+ where: [{ field: "referenceId", value: customer.referenceId }],
+ update: customer,
+ });
+ } else {
+ await tx.create<Customer>({
+ model: "customer",
+ data: customer,
+ });
+ }
+
+ const persisted = await tx.findOne<Customer>({
+ model: "customer",
+ where: [{ field: "referenceId", value: customer.referenceId }],
+ });
+
+ return persisted📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (existingCustomer) { | |
| await tx.update<Customer>({ | |
| model: "customer", | |
| where: [{ field: "referenceId", value: customer.referenceId }], | |
| update: customer, | |
| }); | |
| } else { | |
| await tx.create<Customer>({ | |
| model: "customer", | |
| data: customer, | |
| }); | |
| } | |
| return customer | |
| }); | |
| return upsertedCustomer; | |
| if (existingCustomer) { | |
| await tx.update<Customer>({ | |
| model: "customer", | |
| where: [{ field: "referenceId", value: customer.referenceId }], | |
| update: customer, | |
| }); | |
| } else { | |
| await tx.create<Customer>({ | |
| model: "customer", | |
| data: customer, | |
| }); | |
| } | |
| const persisted = await tx.findOne<Customer>({ | |
| model: "customer", | |
| where: [{ field: "referenceId", value: customer.referenceId }], | |
| }); | |
| return persisted | |
| }); | |
| return upsertedCustomer; |
🤖 Prompt for AI Agents
In package/adapters/queries/upsert-customer.ts around lines 16 to 30, the
function currently returns the inbound customer object instead of the record
persisted by the transaction; change the code to query the DB within the same
transaction after the update/create (e.g., use tx.findUnique/findFirst by
referenceId or the primary key) and assign that fetched record to the
upsertedCustomer variable, then return that persisted record so generated IDs,
timestamps and DB-side defaults/mutations are returned to callers.
| referenceId: { type: "string" }, | ||
| featureKey: { type: "string" }, | ||
| overrideKey: { type: "string" }, | ||
| amount: { type: "string" } | ||
| }, |
There was a problem hiding this comment.
Fix OpenAPI schema type for amount.
amount is validated as a number in Zod, but the OpenAPI metadata advertises it as a string. Update the schema to keep generated clients in sync.
- amount: { type: "string" }
+ amount: { type: "number" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| referenceId: { type: "string" }, | |
| featureKey: { type: "string" }, | |
| overrideKey: { type: "string" }, | |
| amount: { type: "string" } | |
| }, | |
| referenceId: { type: "string" }, | |
| featureKey: { type: "string" }, | |
| overrideKey: { type: "string" }, | |
| amount: { type: "number" } | |
| }, |
🤖 Prompt for AI Agents
In package/endpoints/check-usage.ts around lines 42 to 46, the OpenAPI schema
incorrectly declares amount as type "string" while Zod validates it as a number;
change the amount property in the schema to type "number" so generated clients
match the server validation and update any related examples or docs accordingly.
| const { data: usage, error } = await tryCatch( | ||
| resolveGetUsage({ | ||
| referenceId: ctx.body.referenceId, | ||
| adapter, options, feature | ||
| }) | ||
| ) | ||
|
|
||
| if (error) { | ||
| throw new APIError("INTERNAL_SERVER_ERROR", { | ||
| message: `Internal error getting usage for feature ${feature.key}, ${error.message}` | ||
| }) | ||
| } |
There was a problem hiding this comment.
Don't mask resolver APIError as 500.
resolveGetUsage already throws APIError (e.g. NOT_FOUND) when the usage row is missing. Wrapping it in tryCatch and rethrowing INTERNAL_SERVER_ERROR converts every expected 4xx into a 500, so a missing usage now surfaces as “internal error.” Preserve the original APIError and only wrap unexpected failures.
- const { data: usage, error } = await tryCatch(
- resolveGetUsage({
- referenceId: ctx.body.referenceId,
- adapter, options, feature
- })
- )
-
- if (error) {
- throw new APIError("INTERNAL_SERVER_ERROR", {
- message: `Internal error getting usage for feature ${feature.key}, ${error.message}`
- })
- }
+ const { data: usage, error } = await tryCatch(
+ resolveGetUsage({
+ referenceId: ctx.body.referenceId,
+ adapter,
+ options,
+ feature,
+ }),
+ );
+
+ if (error) {
+ if (error instanceof APIError) {
+ throw error;
+ }
+ throw new APIError("INTERNAL_SERVER_ERROR", {
+ message: `Internal error getting usage for feature ${feature.key}, ${(error as Error).message ?? error}`,
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data: usage, error } = await tryCatch( | |
| resolveGetUsage({ | |
| referenceId: ctx.body.referenceId, | |
| adapter, options, feature | |
| }) | |
| ) | |
| if (error) { | |
| throw new APIError("INTERNAL_SERVER_ERROR", { | |
| message: `Internal error getting usage for feature ${feature.key}, ${error.message}` | |
| }) | |
| } | |
| const { data: usage, error } = await tryCatch( | |
| resolveGetUsage({ | |
| referenceId: ctx.body.referenceId, | |
| adapter, | |
| options, | |
| feature, | |
| }), | |
| ); | |
| if (error) { | |
| if (error instanceof APIError) { | |
| throw error; | |
| } | |
| throw new APIError("INTERNAL_SERVER_ERROR", { | |
| message: `Internal error getting usage for feature ${feature.key}, ${(error as Error).message ?? error}`, | |
| }); | |
| } |
| const { data: usage, error } = await tryCatch( | ||
| resolveSyncUsage({ adapter, feature, referenceId: ctx.body.referenceId, options }) | ||
| ); | ||
| if (error || !usage) { | ||
| throw new APIError("INTERNAL_SERVER_ERROR", { | ||
| message: `Failed to sync usage on feature ${feature.key}, ${error ? error.message : 'usage not found'}` | ||
| }) | ||
| } |
There was a problem hiding this comment.
Error handling treats valid no-op response as failure.
resolveSyncUsage returns undefined when feature.reset is falsy (see package/resolvers/sync-usage.ts line 23), which is a valid no-op case. However, line 64 treats !usage as an error, causing the endpoint to throw INTERNAL_SERVER_ERROR for features without reset configuration.
Apply this fix:
const { data: usage, error } = await tryCatch(
resolveSyncUsage({ adapter, feature, referenceId: ctx.body.referenceId, options })
);
- if (error || !usage) {
+ if (error) {
throw new APIError("INTERNAL_SERVER_ERROR", {
message: `Failed to sync usage on feature ${feature.key}, ${error ? error.message : 'usage not found'}`
})
}
return usageAlternatively, document that the endpoint returns undefined for features without reset configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data: usage, error } = await tryCatch( | |
| resolveSyncUsage({ adapter, feature, referenceId: ctx.body.referenceId, options }) | |
| ); | |
| if (error || !usage) { | |
| throw new APIError("INTERNAL_SERVER_ERROR", { | |
| message: `Failed to sync usage on feature ${feature.key}, ${error ? error.message : 'usage not found'}` | |
| }) | |
| } | |
| const { data: usage, error } = await tryCatch( | |
| resolveSyncUsage({ adapter, feature, referenceId: ctx.body.referenceId, options }) | |
| ); | |
| if (error) { | |
| throw new APIError("INTERNAL_SERVER_ERROR", { | |
| message: `Failed to sync usage on feature ${feature.key}, ${error ? error.message : 'usage not found'}` | |
| }) | |
| } |
🤖 Prompt for AI Agents
In package/endpoints/sync-usage.ts around lines 61 to 68, the handler treats a
valid no-op (resolveSyncUsage returning undefined when feature.reset is falsy)
as an error; change the logic to only throw when error is present (remove the
!usage check) and handle undefined as a valid no-op response (e.g., return a
successful 200/204 response or an explicit message indicating no-op) so the
endpoint does not return INTERNAL_SERVER_ERROR for features without reset
configuration.
| /** | ||
| * Creates an authentication middleware that authorizes a reference against a resolved feature. | ||
| * | ||
| * @param options - Configuration for the middleware | ||
| * @param adapter - Server adapter to access DB by resolver | ||
| * @throws APIError with type `"UNAUTHORIZED"` if the resolved feature's `authorizeReference` returns `false` |
There was a problem hiding this comment.
Update the JSDoc description.
The JSDoc comment mentions "authorizes a reference against a resolved feature," which is incorrect for customer middleware. It should describe customer retrieval and validation.
Apply this diff:
/**
- * Creates an authentication middleware that authorizes a reference against a resolved feature.
+ * Creates an authentication middleware that retrieves and validates a customer by referenceId.
*
* @param options - Configuration for the middleware
* @param adapter - Server adapter to access DB by resolver
- * @throws APIError with type `"UNAUTHORIZED"` if the resolved feature's `authorizeReference` returns `false`
+ * @throws APIError with type `"NOT_FOUND"` if the customer does not exist
*/🤖 Prompt for AI Agents
In package/middlewares/customer.ts around lines 11 to 16, the JSDoc comment
incorrectly describes the middleware as "authoriz[ing] a reference against a
resolved feature"; update the JSDoc to accurately describe this middleware's
purpose: retrieving a customer from the adapter/request, validating the customer
(e.g., existence, status), and throwing an APIError with type "UNAUTHORIZED"
when validation fails; change parameter descriptions to reflect any adapter or
options used for customer lookup and validation and remove references to
"feature" or "authorizeReference."
| // Non blocking | ||
| adapter.resetUsage({ | ||
| referenceId, | ||
| feature, | ||
| curr: data.amount, | ||
| }).catch() |
There was a problem hiding this comment.
Handle adapter.resetUsage rejections.
Calling .catch() without a handler leaves the rejection unhandled; Node treats that as a fatal unhandled rejection. Either await the promise or add a real catch handler to log/emit the failure.
- adapter.resetUsage({
- referenceId,
- feature,
- curr: data.amount,
- }).catch()
+ void adapter.resetUsage({
+ referenceId,
+ feature,
+ curr: data.amount,
+ }).catch((err) => {
+ console.error("[ERROR][USAGE] Failed to reset usage via adapter:", err);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Non blocking | |
| adapter.resetUsage({ | |
| referenceId, | |
| feature, | |
| curr: data.amount, | |
| }).catch() | |
| // Non blocking | |
| void adapter.resetUsage({ | |
| referenceId, | |
| feature, | |
| curr: data.amount, | |
| }).catch((err) => { | |
| console.error("[ERROR][USAGE] Failed to reset usage via adapter:", err); | |
| }); |
🤖 Prompt for AI Agents
In package/resolvers/sync-usage.ts around lines 52-57, adapter.resetUsage(...)
is called with a bare .catch() which leaves rejections unhandled; change this to
either await the call inside a try/catch or attach a real rejection handler that
logs or emits the error (e.g., .catch(err => processLogger.error('resetUsage
failed', { referenceId, feature, err }) or forward to your monitoring), ensuring
the promise rejection is not left unhandled.
| referenceId, | ||
| feature: feature.key, | ||
| amount: (feature.resetValue ?? 0 - data.amount), | ||
| event: "reset" | ||
| }) |
There was a problem hiding this comment.
Fix reset delta calculation when resetValue exists.
Operator precedence makes feature.resetValue ?? 0 - data.amount evaluate as feature.resetValue ?? (0 - data.amount). When resetValue is defined, you now publish that raw value instead of the intended difference. Parenthesize so the amount reflects (resetValue ?? 0) - current.
- amount: (feature.resetValue ?? 0 - data.amount),
+ amount: (feature.resetValue ?? 0) - data.amount,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| referenceId, | |
| feature: feature.key, | |
| amount: (feature.resetValue ?? 0 - data.amount), | |
| event: "reset" | |
| }) | |
| referenceId, | |
| feature: feature.key, | |
| amount: (feature.resetValue ?? 0) - data.amount, | |
| event: "reset" | |
| }) |
🤖 Prompt for AI Agents
In package/resolvers/sync-usage.ts around lines 60 to 64, the amount passed for
the reset event uses wrong operator precedence: feature.resetValue ?? 0 -
data.amount evaluates as feature.resetValue ?? (0 - data.amount) so when
resetValue exists you publish the raw resetValue. Fix by parenthesizing the
nullish coalescing result so the amount is (feature.resetValue ?? 0) -
data.amount, ensuring the published amount is the difference between resetValue
(or 0) and the current data.amount.
| options.cache && options.cache.setCustomer(customer).catch(() => { | ||
| console.log("[ERROR][CUSTOMER] Cache failed to insert customer") | ||
| }) |
There was a problem hiding this comment.
Cache should store the DB result, not the inbound payload.
Line 23 writes the original customer object into cache, so any defaults or normalization applied by the adapter (e.g., trimmed fields, generated IDs) are lost and subsequent reads serve stale data. Cache the data returned from the adapter instead.
- options.cache && options.cache.setCustomer(customer).catch(() => {
- console.log("[ERROR][CUSTOMER] Cache failed to insert customer")
- })
+ if (options.cache) {
+ options.cache.setCustomer(data).catch(() => {
+ console.log("[ERROR][CUSTOMER] Cache failed to insert customer")
+ })
+ }🤖 Prompt for AI Agents
In package/resolvers/upsert-customer.ts around lines 23 to 25, the code
currently writes the inbound `customer` object into the cache which loses
adapter-applied defaults/normalization; change the cache call to store the
adapter's returned result (e.g., `data` or `result.data`) instead of `customer`,
and keep the existing `.catch` logging so: call options.cache.setCustomer(with
the adapter-returned data) and handle the promise rejection as before.
| authorizeReference?: (params: { | ||
| feature: string; | ||
| referenceId: string; | ||
| referenceType: string; | ||
| incomingId: string; | ||
| }) => Promise<boolean> | boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that all authorizeReference implementations match the new signature.
The signature of authorizeReference has changed from accepting body and customer parameters to a structured params object. This is a breaking change that requires all implementations to be updated.
Run the following script to find all implementations and usages:
🏁 Script executed:
#!/bin/bash
# Search for authorizeReference implementations
rg -nP --type=ts 'authorizeReference\s*[=:]' -A5Length of output: 1884
I need to search for actual invocations of authorizeReference to verify if the new signature is being used. Let me also check for any old-style calls with body and customer parameters.
🏁 Script executed:
#!/bin/bash
# Find all call sites where authorizeReference is invoked
rg -nP --type=ts 'authorizeReference\s*\(' -B2 -A3Length of output: 542
🏁 Script executed:
#!/bin/bash
# Search for any remaining usage of body and customer parameters near authorizeReference
rg -nP --type=ts 'authorizeReference.*(?:body|customer)' -B2 -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find implementations outside test files
rg -nP --type=ts 'authorizeReference\s*[=:]' -g '!**/__tests__/**' -g '!**/*.test.ts' -A5Length of output: 0
Let me verify the complete call site and check for any other invocations:
🏁 Script executed:
#!/bin/bash
# Get the complete authorizeReference call with all parameters
sed -n '42,50p' package/realtime/websocket-server.tsLength of output: 445
🏁 Script executed:
#!/bin/bash
# Count all authorizeReference occurrences to ensure we haven't missed any call sites
rg -c --type=ts 'authorizeReference' || echo "done"Length of output: 176
🏁 Script executed:
#!/bin/bash
# Show all lines with authorizeReference across the codebase with context
rg --type=ts 'authorizeReference' -nLength of output: 1713
I found another call site in package/middlewares/usage.ts. Let me verify it matches the new signature:
🏁 Script executed:
#!/bin/bash
# Get the complete authorizeReference call in usage.ts with full context
sed -n '20,35p' package/middlewares/usage.tsLength of output: 411
| if (source === "cache") { | ||
| const d = (data as cached_Usage) | ||
|
|
||
| return { | ||
| referenceId: d.referenceId, | ||
| feature: d.feature, | ||
| amount: d.current, | ||
| event: undefined, | ||
| createdAt: d.updatedAt, | ||
| lastResetAt: d.lastResetAt | ||
| } as Usage | ||
| } |
There was a problem hiding this comment.
Event field set to undefined violates Usage schema.
Line 156 sets event: undefined, but the usage schema at package/index.ts line 102 declares event as a required string field. This will cause schema validation failures or runtime issues.
Either:
- Set a default event value (e.g.,
"cache"or"sync") - Update the Usage schema to make event optional
Apply this fix (option 1):
return {
referenceId: d.referenceId,
feature: d.feature,
amount: d.current,
- event: undefined,
+ event: "cache",
createdAt: d.updatedAt,
lastResetAt: d.lastResetAt
} as Usage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (source === "cache") { | |
| const d = (data as cached_Usage) | |
| return { | |
| referenceId: d.referenceId, | |
| feature: d.feature, | |
| amount: d.current, | |
| event: undefined, | |
| createdAt: d.updatedAt, | |
| lastResetAt: d.lastResetAt | |
| } as Usage | |
| } | |
| if (source === "cache") { | |
| const d = (data as cached_Usage) | |
| return { | |
| referenceId: d.referenceId, | |
| feature: d.feature, | |
| amount: d.current, | |
| event: "cache", | |
| createdAt: d.updatedAt, | |
| lastResetAt: d.lastResetAt | |
| } as Usage | |
| } |
🤖 Prompt for AI Agents
In package/utils.ts around lines 149 to 160, the returned Usage object from the
cache branch sets event: undefined which violates the Usage schema requiring a
string; change this to a default string (e.g., "cache") so the object conforms
to the schema — update the returned object to set event: "cache" and ensure any
typing reflects that default.
1 participant
Summary by CodeRabbit