Refactor/cleanup - #41
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConsolidates error enums and removes many static error factories; introduces calculatePaymentPrice and routes checkout pricing to it; removes common PAYMENT pricing module and related adapter exports; refactors ClickHouse/Postgres pricing utilities, AI token usage aggregation, query→proto helpers, webhook handling, and Zod filter-group schemas. ChangesError Type Consolidation and Pricing Service Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/pricingService.ts`:
- Around line 1-2: Add operation-level logging around all storage adapter
interactions and error paths in the PricingService: import logger from
"errors/logger" and call logger.logOperationInfo(...) before each
StorageAdapterFactory usage (include operation name like
"PricingService.storeEvent", and context object with userId, mode, and
eventType) and call logger.logOperationError(...) in each catch or when
returning StorageError (include same context, the caught error, and operation
name). Ensure every adapter call site created via StorageAdapterFactory is
bracketed by a logOperationInfo before/after and any error branch or thrown
StorageError uses logOperationError with the operation name and the context keys
userId, mode, and eventType.
- Around line 8-9: Normalize beforeTimestamp to UTC at the service boundary by
creating a single normalized variable (e.g., beforeTimestampUtc =
beforeTimestamp.toUTC()) inside the entry function that receives
beforeTimestamp, and use that normalized value for all downstream adapter calls
instead of the original beforeTimestamp; update the places in this file that
call adapters (references to beforeTimestamp in adapter method invocations) to
pass beforeTimestampUtc so both adapter calls use the same UTC-normalized
timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db338341-8a8e-4b3a-b44d-70acaf6c061f
📒 Files selected for processing (19)
.fallowrc.jsonpackage.jsonsrc/errors/apikey.tssrc/errors/auth.tssrc/errors/event.tssrc/errors/payment.tssrc/errors/storage.tssrc/interceptors/auth.tssrc/routes/gRPC/payment/createCheckoutLink.tssrc/services/pricingService.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/index.tssrc/storage/adapter/clickhouse/handlers/priceRequestPayment.tssrc/storage/adapter/common/priceRequestPayment.tssrc/storage/adapter/common/queryEventsBase.tssrc/storage/adapter/postgres/handlers/index.tssrc/storage/adapter/postgres/handlers/priceRequestPayment.tssrc/storage/adapter/postgres/postgres.tssrc/storage/db/postgres/helpers/users.ts
💤 Files with no reviewable changes (15)
- src/storage/adapter/common/priceRequestPayment.ts
- src/storage/adapter/clickhouse/handlers/priceRequestPayment.ts
- src/storage/adapter/postgres/handlers/priceRequestPayment.ts
- src/storage/db/postgres/helpers/users.ts
- package.json
- src/interceptors/auth.ts
- src/storage/adapter/postgres/postgres.ts
- src/storage/adapter/clickhouse/handlers/index.ts
- src/storage/adapter/clickhouse/ClickHouseAdapter.ts
- src/errors/auth.ts
- src/storage/adapter/postgres/handlers/index.ts
- src/errors/apikey.ts
- src/errors/event.ts
- src/errors/payment.ts
- src/errors/storage.ts
Comment on lines
+1
to
+2
| import { StorageAdapterFactory } from "../factory/EventStorageAdapterFactory"; | ||
| import { StorageError } from "../errors/storage"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add operation-level logging in this new pricing flow.
This service is a key billing path and currently has no logOperationInfo / logOperationError calls. Please add operation/context logging (at least userId, mode, and event type) around adapter calls and error branches.
As per coding guidelines, "Use logger from errors/logger with logOperationInfo and logOperationError; include operation name and context".
Also applies to: 6-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/pricingService.ts` around lines 1 - 2, Add operation-level
logging around all storage adapter interactions and error paths in the
PricingService: import logger from "errors/logger" and call
logger.logOperationInfo(...) before each StorageAdapterFactory usage (include
operation name like "PricingService.storeEvent", and context object with userId,
mode, and eventType) and call logger.logOperationError(...) in each catch or
when returning StorageError (include same context, the caught error, and
operation name). Ensure every adapter call site created via
StorageAdapterFactory is bracketed by a logOperationInfo before/after and any
error branch or thrown StorageError uses logOperationError with the operation
name and the context keys userId, mode, and eventType.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/storage/adapter/clickhouse/utils.ts (2)
83-84: 💤 Low valueAdd explicit radix to
parseInt.While modern JavaScript defaults to base 10 for numeric strings, explicitly passing the radix improves clarity and avoids edge cases with unexpected input formats.
♻️ Proposed fix
- const parsed = parseInt(data[0].total); + const parsed = parseInt(data[0].total, 10);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/adapter/clickhouse/utils.ts` around lines 83 - 84, The parseInt call that converts data[0].total to a number should include an explicit radix to avoid ambiguous parsing; update the parseInt usage that assigns parsed (from data[0].total) to call parseInt(data[0].total, 10) so the code returns the intended base-10 integer before the isNaN check.
23-25: 💤 Low valueConsider logging errors instead of silently swallowing them.
The empty catch block discards all error information, which could mask database connectivity issues or other problems during debugging. Consider at minimum logging the error, even if you still return
nullfor graceful degradation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/adapter/clickhouse/utils.ts` around lines 23 - 25, The catch block in src/storage/adapter/clickhouse/utils.ts that currently just returns null should capture and log the error before returning to avoid swallowing failures; change the empty catch to catch (err) (or e) and call the project's logging facility (e.g., logger.error or processLogger.error) with a clear message like "ClickHouse utility error" plus the caught error details, then return null to preserve graceful degradation; if a project logger isn't available in this module, use console.error with the same contextual message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/http/createdCheckout.ts`:
- Around line 61-81: The function resolvePaymentSession is defined but unused;
replace the duplicate manual session lookup and validation in the handler (where
getSessionByCheckoutId is called and fields checked) by calling
resolvePaymentSession(checkoutSessionId) and using its returned ResolvedSession
(or handle null) to simplify the flow, or remove resolvePaymentSession entirely
if you prefer not to refactor; ensure any references to session.userId,
session.billed_upto, session.apiKeyId, and session.mode are updated to use the
ResolvedSession properties userId, billedUpto, apiKeyId, and mode when adopting
the refactor.
- Around line 163-171: Wrap the two DB updates and the payment event creation in
a single Drizzle transaction so they succeed or roll back atomically: use
getPostgresDB()'s transaction API to execute the updates to usersTable and
sessionsTable together and call storePaymentEvent (and builder.setUser /
builder.setPaymentContext) inside that transaction, ensuring either both updates
and the payment record persist or none do to prevent duplicate events.
In `@src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts`:
- Around line 22-29: The validateNonNegative function currently allows NaN and
Infinity; update its checks to reject non-finite numbers by verifying typeof
value === "number" && !Number.isFinite(value) (or using Number.isFinite(value)
in the positive branch) and throw StorageError.insertFailed with the same
descriptive message (e.g., `Negative ${label} not allowed...` or for non-finite:
`${label} ${value} is not a finite number`) so NaN/Infinity are treated as
invalid; apply the same finite-number validation fix to the other similar helper
used at the 92-103 region (the alternate validation function referenced there)
so neither path accepts non-finite numeric values.
In `@src/storage/adapter/clickhouse/utils.ts`:
- Around line 86-93: Replace the duck-typing check that inspects e.name and
casts to any with a direct instanceof check against StorageError: remove the
object/type/name checks and use "if (e instanceof StorageError) throw e;".
Ensure StorageError is imported/visible in the module so the instanceof check is
type-safe and delete the unnecessary any cast and property access.
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Around line 88-92: The helper buildAiTokenInsertValues currently relies on
inference for its return type; annotate its signature with an explicit return
type (e.g., a specific array type like AiTokenInsertRow[] or an existing type
such as AiTokenUsageInsert[]) and, if needed, declare a small interface/type
alias (e.g., AiTokenInsertRow) describing the shape returned (fields derived
from AggregatedEvent + apiKeyId and mode). Update the function signature for
buildAiTokenInsertValues(aggregatedEvents: AggregatedEvent[], apiKeyId: string,
mode: "production" | "test"): AiTokenInsertRow[] and ensure any callers/imports
use the same type; create the new type near the top of the file or in the module
types file if one exists.
In `@src/zod/internals.ts`:
- Around line 16-21: The current logical schema (the "logical" zod entry using
logicalMap) silently defaults unknown numeric values to "AND"; change it to
reject unknown values instead: validate that the incoming number exists in
logicalMap (e.g., via .refine or a transform that throws when logicalMap[v] is
undefined) so validation fails for invalid numerics rather than mapping them to
"AND"; keep the symbol names logical and logicalMap so you locate and update
that schema entry.
---
Nitpick comments:
In `@src/storage/adapter/clickhouse/utils.ts`:
- Around line 83-84: The parseInt call that converts data[0].total to a number
should include an explicit radix to avoid ambiguous parsing; update the parseInt
usage that assigns parsed (from data[0].total) to call parseInt(data[0].total,
10) so the code returns the intended base-10 integer before the isNaN check.
- Around line 23-25: The catch block in src/storage/adapter/clickhouse/utils.ts
that currently just returns null should capture and log the error before
returning to avoid swallowing failures; change the empty catch to catch (err)
(or e) and call the project's logging facility (e.g., logger.error or
processLogger.error) with a clear message like "ClickHouse utility error" plus
the caught error details, then return null to preserve graceful degradation; if
a project logger isn't available in this module, use console.error with the same
contextual message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2764feea-b7bb-48e3-948f-17fc022c3f9b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
AGENTS.mdsrc/routes/gRPC/query/queryEvents.tssrc/routes/http/createdCheckout.tssrc/services/pricingService.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/priceRequestSdkCall.tssrc/storage/adapter/clickhouse/utils.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/zod/data.tssrc/zod/internals.tssrc/zod/query.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/pricingService.ts
Comment on lines
+163
to
+171
| const db = getPostgresDB(); | ||
|
|
||
| await db.update(usersTable).set({ last_billed_timestamp: billedUpto }).where(eq(usersTable.id, userId)); | ||
| await db.update(sessionsTable).set({ processed: true }).where(eq(sessionsTable.sessionId, checkout_session_id)); | ||
|
|
||
| builder.setUser(userId); | ||
| builder.setPaymentContext({ creditAmount }); | ||
|
|
||
| try { | ||
| const paymentEvent = new Payment(userId, { creditAmount }); | ||
| const adapter = | ||
| await StorageAdapterFactory.getEventStorageAdapter("PAYMENT"); | ||
|
|
||
| await adapter.add(paymentEvent.serialize(), session.apiKeyId, session.mode); | ||
|
|
||
| builder.setSuccess(200); | ||
| return { | ||
| statusCode: 200, | ||
| body: { message: "Webhook processed successfully" }, | ||
| }; | ||
| } catch (dbError) { | ||
| Sentry.captureException(dbError, { | ||
| extra: { | ||
| context: "payment event storage", | ||
| checkoutSessionId: checkout_session_id, | ||
| paymentId: payment_id, | ||
| }, | ||
| }); | ||
| const errorMessage = | ||
| dbError instanceof Error ? dbError.message : String(dbError); | ||
| builder.setError(500, { | ||
| type: "DatabaseError", | ||
| message: `Failed to store payment event: ${errorMessage}`, | ||
| cause: dbError instanceof Error ? dbError.message : undefined, | ||
| stack: isDev && dbError instanceof Error ? dbError.stack : undefined, | ||
| }); | ||
| return { statusCode: 500, body: { error: "Database error" } }; | ||
| } | ||
| return await storePaymentEvent(userId, creditAmount, apiKeyId, mode, checkout_session_id, payment_id, builder); |
There was a problem hiding this comment.
Wrap database updates in a transaction to prevent duplicate payment events.
Lines 165-166 perform two separate updates without atomicity. If the usersTable update succeeds but the sessionsTable update fails, a webhook retry would:
- Pass the
session.processedcheck (line 146) since it remainsfalse - Execute
storePaymentEventagain, creating a duplicate payment record
This is a data integrity risk for financial operations.
🐛 Proposed fix using a transaction
const db = getPostgresDB();
- await db.update(usersTable).set({ last_billed_timestamp: billedUpto }).where(eq(usersTable.id, userId));
- await db.update(sessionsTable).set({ processed: true }).where(eq(sessionsTable.sessionId, checkout_session_id));
+ await db.transaction(async (tx) => {
+ await tx.update(usersTable).set({ last_billed_timestamp: billedUpto }).where(eq(usersTable.id, userId));
+ await tx.update(sessionsTable).set({ processed: true }).where(eq(sessionsTable.sessionId, checkout_session_id));
+ });
builder.setUser(userId);
builder.setPaymentContext({ creditAmount });As per coding guidelines: "Use Drizzle ORM with transactions".
📝 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.
Suggested change
| const db = getPostgresDB(); | |
| await db.update(usersTable).set({ last_billed_timestamp: billedUpto }).where(eq(usersTable.id, userId)); | |
| await db.update(sessionsTable).set({ processed: true }).where(eq(sessionsTable.sessionId, checkout_session_id)); | |
| builder.setUser(userId); | |
| builder.setPaymentContext({ creditAmount }); | |
| try { | |
| const paymentEvent = new Payment(userId, { creditAmount }); | |
| const adapter = | |
| await StorageAdapterFactory.getEventStorageAdapter("PAYMENT"); | |
| await adapter.add(paymentEvent.serialize(), session.apiKeyId, session.mode); | |
| builder.setSuccess(200); | |
| return { | |
| statusCode: 200, | |
| body: { message: "Webhook processed successfully" }, | |
| }; | |
| } catch (dbError) { | |
| Sentry.captureException(dbError, { | |
| extra: { | |
| context: "payment event storage", | |
| checkoutSessionId: checkout_session_id, | |
| paymentId: payment_id, | |
| }, | |
| }); | |
| const errorMessage = | |
| dbError instanceof Error ? dbError.message : String(dbError); | |
| builder.setError(500, { | |
| type: "DatabaseError", | |
| message: `Failed to store payment event: ${errorMessage}`, | |
| cause: dbError instanceof Error ? dbError.message : undefined, | |
| stack: isDev && dbError instanceof Error ? dbError.stack : undefined, | |
| }); | |
| return { statusCode: 500, body: { error: "Database error" } }; | |
| } | |
| return await storePaymentEvent(userId, creditAmount, apiKeyId, mode, checkout_session_id, payment_id, builder); | |
| const db = getPostgresDB(); | |
| await db.transaction(async (tx) => { | |
| await tx.update(usersTable).set({ last_billed_timestamp: billedUpto }).where(eq(usersTable.id, userId)); | |
| await tx.update(sessionsTable).set({ processed: true }).where(eq(sessionsTable.sessionId, checkout_session_id)); | |
| }); | |
| builder.setUser(userId); | |
| builder.setPaymentContext({ creditAmount }); | |
| return await storePaymentEvent(userId, creditAmount, apiKeyId, mode, checkout_session_id, payment_id, builder); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/http/createdCheckout.ts` around lines 163 - 171, Wrap the two DB
updates and the payment event creation in a single Drizzle transaction so they
succeed or roll back atomically: use getPostgresDB()'s transaction API to
execute the updates to usersTable and sessionsTable together and call
storePaymentEvent (and builder.setUser / builder.setPaymentContext) inside that
transaction, ensuring either both updates and the payment record persist or none
do to prevent duplicate events.
Comment on lines
+22
to
+29
| function validateNonNegative(value: unknown, label: string, userId: UserId): void { | ||
| if (typeof value === "number" && value < 0) { | ||
| throw StorageError.insertFailed( | ||
| `Negative ${label} not allowed for AI token usage for user ${userId}`, | ||
| new Error(`${label} ${value} is negative`) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateNonNegative allows non-finite numbers, which can serialize to null metrics.
NaN/Infinity currently pass validation and are converted to null by JSON.stringify, which can silently corrupt billing data.
💡 Proposed fix
function validateNonNegative(value: unknown, label: string, userId: UserId): void {
- if (typeof value === "number" && value < 0) {
+ if (typeof value !== "number" || !Number.isFinite(value) || value < 0) {
throw StorageError.insertFailed(
- `Negative ${label} not allowed for AI token usage for user ${userId}`,
- new Error(`${label} ${value} is negative`)
+ `Invalid ${label} for AI token usage for user ${userId}`,
+ new Error(
+ `${label} must be a finite non-negative number, received ${String(value)}`
+ )
);
}
}Also applies to: 92-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts` around lines 22 -
29, The validateNonNegative function currently allows NaN and Infinity; update
its checks to reject non-finite numbers by verifying typeof value === "number"
&& !Number.isFinite(value) (or using Number.isFinite(value) in the positive
branch) and throw StorageError.insertFailed with the same descriptive message
(e.g., `Negative ${label} not allowed...` or for non-finite: `${label} ${value}
is not a finite number`) so NaN/Infinity are treated as invalid; apply the same
finite-number validation fix to the other similar helper used at the 92-103
region (the alternate validation function referenced there) so neither path
accepts non-finite numeric values.
Comment on lines
+86
to
+93
| if ( | ||
| e && | ||
| typeof e === "object" && | ||
| "type" in e && | ||
| (e as any).name === "StorageError" | ||
| ) { | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use instanceof instead of duck-typing with any.
The current approach casts to any (discouraged per coding guidelines) and relies on the name property which could be minified or unreliable. Use instanceof StorageError for type-safe error identification.
♻️ Proposed fix
- if (
- e &&
- typeof e === "object" &&
- "type" in e &&
- (e as any).name === "StorageError"
- ) {
+ if (e instanceof StorageError) {
throw e;
}📝 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.
Suggested change
| if ( | |
| e && | |
| typeof e === "object" && | |
| "type" in e && | |
| (e as any).name === "StorageError" | |
| ) { | |
| throw e; | |
| } | |
| if (e instanceof StorageError) { | |
| throw e; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/adapter/clickhouse/utils.ts` around lines 86 - 93, Replace the
duck-typing check that inspects e.name and casts to any with a direct instanceof
check against StorageError: remove the object/type/name checks and use "if (e
instanceof StorageError) throw e;". Ensure StorageError is imported/visible in
the module so the instanceof check is type-safe and delete the unnecessary any
cast and property access.
Comment on lines
+88
to
+92
| function buildAiTokenInsertValues( | ||
| aggregatedEvents: AggregatedEvent[], | ||
| apiKeyId: string, | ||
| mode: "production" | "test" | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add an explicit return type to buildAiTokenInsertValues.
This helper currently relies on inference for its return type.
💡 Proposed fix
function buildAiTokenInsertValues(
aggregatedEvents: AggregatedEvent[],
apiKeyId: string,
mode: "production" | "test"
-) {
+): Array<Record<string, unknown>> {As per coding guidelines: "Always use explicit types for function parameters and return values; avoid any".
📝 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.
Suggested change
| function buildAiTokenInsertValues( | |
| aggregatedEvents: AggregatedEvent[], | |
| apiKeyId: string, | |
| mode: "production" | "test" | |
| ) { | |
| function buildAiTokenInsertValues( | |
| aggregatedEvents: AggregatedEvent[], | |
| apiKeyId: string, | |
| mode: "production" | "test" | |
| ): Array<Record<string, unknown>> { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts` around lines 88 -
92, The helper buildAiTokenInsertValues currently relies on inference for its
return type; annotate its signature with an explicit return type (e.g., a
specific array type like AiTokenInsertRow[] or an existing type such as
AiTokenUsageInsert[]) and, if needed, declare a small interface/type alias
(e.g., AiTokenInsertRow) describing the shape returned (fields derived from
AggregatedEvent + apiKeyId and mode). Update the function signature for
buildAiTokenInsertValues(aggregatedEvents: AggregatedEvent[], apiKeyId: string,
mode: "production" | "test"): AiTokenInsertRow[] and ensure any callers/imports
use the same type; create the new type near the top of the file or in the module
types file if one exists.
Comment on lines
+16
to
+21
| logical: z | ||
| .number() | ||
| .int() | ||
| .min(0) | ||
| .max(2) | ||
| .transform((v) => (logicalMap[v] ?? "AND") as "AND" | "OR"), |
There was a problem hiding this comment.
Reject unknown logical operator values instead of defaulting to AND.
Invalid logical values currently pass validation and silently become AND, which can alter query behavior instead of failing fast.
💡 Proposed fix
- logical: z
- .number()
- .int()
- .min(0)
- .max(2)
- .transform((v) => (logicalMap[v] ?? "AND") as "AND" | "OR"),
+ logical: z
+ .number()
+ .int()
+ .refine((v) => logicalMap[v] !== undefined, "Invalid logical operator")
+ .transform((v) => logicalMap[v]),📝 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.
Suggested change
| logical: z | |
| .number() | |
| .int() | |
| .min(0) | |
| .max(2) | |
| .transform((v) => (logicalMap[v] ?? "AND") as "AND" | "OR"), | |
| logical: z | |
| .number() | |
| .int() | |
| .refine((v) => logicalMap[v] !== undefined, "Invalid logical operator") | |
| .transform((v) => logicalMap[v]), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/zod/internals.ts` around lines 16 - 21, The current logical schema (the
"logical" zod entry using logicalMap) silently defaults unknown numeric values
to "AND"; change it to reject unknown values instead: validate that the incoming
number exists in logicalMap (e.g., via .refine or a transform that throws when
logicalMap[v] is undefined) so validation fails for invalid numerics rather than
mapping them to "AND"; keep the symbol names logical and logicalMap so you
locate and update that schema entry.
This was referenced May 17, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Chores