Refactor/readthrough - #44
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThis PR narrows AuthContext.mode to non-null, threads a unified AuthContext through storage adapters and handlers, updates DB schema/helpers for transaction participation, refactors payment webhook handling into a single DB transaction, and converts several schemas to enum-backed Zod literals. ChangesAuth Mode Narrowing and Storage Adapter Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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
🧹 Nitpick comments (2)
src/storage/adapter/postgres/handlers/addAiTokenUsage.ts (1)
141-167: ⚡ Quick winSame ordering concern: await user-existence checks before insert.
Similar to
addBasicUsage.ts, theensurePromisesarray is created but awaited after theaiTokenUsageEventsTableinsert. For explicit ordering and code clarity, consider awaitingPromise.all(ensurePromises)before the insert.Proposed fix
const uniqueUserIds = Array.from( new Set(aggregatedEvents.map((event) => event.userId)) ); - const ensurePromises = uniqueUserIds.map((userId) => - ensureUserExists(userId, txn) - ); + try { + await Promise.all( + uniqueUserIds.map((userId) => ensureUserExists(userId, txn)) + ); + } catch (e) { + throw StorageError.insertFailed( + "Failed to ensure users exist for AI token usage events", + e instanceof Error ? e : new Error(String(e)) + ); + } try { const aiTokenUsageValues = buildAiTokenInsertValues(aggregatedEvents, auth); const inserted = await txn .insert(aiTokenUsageEventsTable) .values(aiTokenUsageValues) .returning({ id: aiTokenUsageEventsTable.id }); if (!inserted[0] || !inserted[0].id) { throw StorageError.insertFailed( "Missing or invalid ID for the first inserted event", new Error(`Invalid first event ID: ${JSON.stringify(inserted[0])}`) ); } - try { - await Promise.all(ensurePromises); - } catch (e) { - throw StorageError.insertFailed( - "Failed to ensure users exist for AI token usage events", - e instanceof Error ? e : new Error(String(e)) - ); - } - return { id: inserted[0].id };🤖 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 141 - 167, The ensureUserExists promises (ensurePromises) must be awaited before inserting into aiTokenUsageEventsTable to guarantee users exist prior to the insert; move the await Promise.all(ensurePromises) so it runs before calling buildAiTokenInsertValues and txn.insert, keeping the existing error handling (wrap Promise.all in its try/catch and throw StorageError.insertFailed on failure), and ensure references to ensureUserExists, ensurePromises, buildAiTokenInsertValues, txn.insert and aiTokenUsageEventsTable are used to locate and update the logic.src/storage/adapter/postgres/handlers/addBasicUsage.ts (1)
31-63: ⚡ Quick winAwait
ensureUserExistsbefore the insert for clearer ordering.The
ensurePromiseis created on line 31 but not awaited until line 57, after thebasicUsageEventsTableinsert. While this likely works due to pg driver serializing queries on the same transaction connection, the code structure suggests the user-existence check happens after the FK-dependent insert.Awaiting
ensureUserExistsbefore the insert would make the execution order explicit and less reliant on driver implementation details.Proposed fix
return await executeInTransaction( connectionObject, "storing BASIC_USAGE event", async (txn) => { - const ensurePromise = ensureUserExists(event_data.userId, txn); + try { + await ensureUserExists(event_data.userId, txn); + } catch (e) { + throw StorageError.insertFailed( + "Failed to ensure user exists for basic usage event", + e instanceof Error ? e : new Error(String(e)) + ); + } const reportedTimestamp = await validateAndPrepareTimestamp( event_data.reported_timestamp ); try { const [result] = await txn .insert(basicUsageEventsTable) .values({ reportedTimestamp, ingestedTimestamp: DateTime.utc().toString(), userId: event_data.userId, apiKeyId: auth.apiKeyId, mode: auth.mode, type: event_data.data.basicUsageType, debitAmount: event_data.data.debitAmount, metadata: event_data.data.metadata ?? null, }) .returning({ id: basicUsageEventsTable.id }); if (!result) { throw StorageError.emptyResult("Basic usage event insert returned no ID"); } - try { - await ensurePromise; - } catch (e) { - throw StorageError.insertFailed( - "Failed to ensure user exists for basic usage event", - e instanceof Error ? e : new Error(String(e)) - ); - } - return { id: result.id }; } catch (e) { throw StorageError.insertFailed( "Failed to insert basic usage event", e instanceof Error ? e : new Error(String(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/postgres/handlers/addBasicUsage.ts` around lines 31 - 63, The ensureUserExists call is started but not awaited until after inserting into basicUsageEventsTable; to make ordering explicit await the ensureUserExists promise before performing txn.insert: replace the pre-created ensurePromise with an awaited call to ensureUserExists(event_data.userId, txn) (or await the existing ensurePromise) immediately after validateAndPrepareTimestamp and before the txn.insert block so the FK-dependent insert in the insert handler (txn.insert(basicUsageEventsTable)...) only runs after the user existence is guaranteed.
🤖 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/registerWebhookRoutes.ts`:
- Around line 10-18: The extractHeaderValue function currently returns the first
element of string[] inputs which allows ambiguous duplicate auth headers; change
it to fail closed by returning undefined for multi-value arrays (and for empty
arrays), only returning the value when the header is a single string or a
single-element array; update the same logic used in the other occurrence around
the webhook auth header handling (the code paths that process
"webhook-signature", "webhook-timestamp", "webhook-id") so any header array with
length !== 1 is treated as missing/invalid.
In `@src/storage/adapter/clickhouse/handlers/addBasicUsage.ts`:
- Around line 30-31: The current flow starts ensureUserExists() (ensurePromise)
and only awaits it after the ClickHouse insert, risking persisted data if
ensureUserExists fails; change the flow so ensureUserExists is awaited before
performing the ClickHouse insert to fail-fast: locate the ensureUserExists
call/variable (ensurePromise) in addBasicUsage handler and move or replace it
with an awaited call (await ensureUserExists(event_data.userId)) before the
function that inserts into ClickHouse (the ClickHouse insert/insertEvent call),
so insertion only runs after user existence is confirmed; alternatively, if you
prefer to keep the insert as primary, catch errors from await ensureUserExists
and log a warning via the same logger used in this file and still return success
when the ClickHouse insert succeeded (i.e., do not rethrow) — pick one approach
and apply it consistently where ensurePromise is currently awaited.
---
Nitpick comments:
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Around line 141-167: The ensureUserExists promises (ensurePromises) must be
awaited before inserting into aiTokenUsageEventsTable to guarantee users exist
prior to the insert; move the await Promise.all(ensurePromises) so it runs
before calling buildAiTokenInsertValues and txn.insert, keeping the existing
error handling (wrap Promise.all in its try/catch and throw
StorageError.insertFailed on failure), and ensure references to
ensureUserExists, ensurePromises, buildAiTokenInsertValues, txn.insert and
aiTokenUsageEventsTable are used to locate and update the logic.
In `@src/storage/adapter/postgres/handlers/addBasicUsage.ts`:
- Around line 31-63: The ensureUserExists call is started but not awaited until
after inserting into basicUsageEventsTable; to make ordering explicit await the
ensureUserExists promise before performing txn.insert: replace the pre-created
ensurePromise with an awaited call to ensureUserExists(event_data.userId, txn)
(or await the existing ensurePromise) immediately after
validateAndPrepareTimestamp and before the txn.insert block so the FK-dependent
insert in the insert handler (txn.insert(basicUsageEventsTable)...) only runs
after the user existence is guaranteed.
🪄 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: 1ced9dfb-270b-4911-895d-b9532c57fca5
📒 Files selected for processing (21)
src/context/auth.tssrc/interceptors/auth.tssrc/interface/event/Event.tssrc/interface/storage/Storage.tssrc/routes/http/checkoutRedirect.tssrc/routes/http/createdCheckout.tssrc/routes/http/registerWebhookRoutes.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/addBasicUsage.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addBasicUsage.tssrc/storage/adapter/postgres/handlers/addEventUtils.tssrc/storage/adapter/postgres/postgres.tssrc/storage/db/postgres/helpers/payments.tssrc/storage/db/postgres/helpers/sessions.tssrc/storage/db/postgres/helpers/users.tssrc/storage/db/postgres/schema.tssrc/utils/apiKeyCache.tssrc/utils/authenticateHttpApiKey.tssrc/utils/eventHelpers.ts
💤 Files with no reviewable changes (1)
- src/storage/adapter/postgres/handlers/addEventUtils.ts
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/zod/event.ts`:
- Line 18: The transform callback passed to the Zod schema's .transform call
uses an untyped parameter `ts`; update the lambda used with .transform (the
`(ts) => DateTime.fromSeconds(...)` callback) to annotate the parameter as a
number (i.e., `(ts: number)`) so it satisfies strict TypeScript; keep the body
using DateTime.fromSeconds(ts, { zone: "utc" }) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| .number() | ||
| .int() | ||
| .transform((ts) => DateTime.fromSeconds(ts, { zone: 'utc' })), | ||
| .transform((ts) => DateTime.fromSeconds(ts, { zone: "utc" })), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify project strictness and locate untyped transform params in this file.
fd -a 'tsconfig.*\.json$'
rg -n 'transform\(\((ts|v)\)\s*=>' src/zod/event.tsRepository: ScrawnDotDev/Scrawn
Length of output: 166
🏁 Script executed:
cat -n src/zod/event.ts | head -30Repository: ScrawnDotDev/Scrawn
Length of output: 1314
🏁 Script executed:
cat /home/jailuser/git/tsconfig.json | jq '.'Repository: ScrawnDotDev/Scrawn
Length of output: 120
🏁 Script executed:
rg -B5 -A2 'transform.*DateTime.fromSeconds' src/zod/event.tsRepository: ScrawnDotDev/Scrawn
Length of output: 239
🏁 Script executed:
cat /home/jailuser/git/tsconfig.jsonRepository: ScrawnDotDev/Scrawn
Length of output: 810
🏁 Script executed:
rg -n '\.transform\(\(' --type ts --type tsx | head -20Repository: ScrawnDotDev/Scrawn
Length of output: 91
🏁 Script executed:
rg -B2 -A1 'transform.*\(' src/zod/event.tsRepository: ScrawnDotDev/Scrawn
Length of output: 1259
Add an explicit type for the transform callback parameter.
Line 18 uses an untyped callback parameter (ts). The TypeScript configuration enforces strict: true, which requires explicit parameter types. Annotate the parameter as (ts: number) since it's transforming a .number().int() schema value.
.transform((ts: number) => DateTime.fromSeconds(ts, { zone: "utc" })),
🤖 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/event.ts` at line 18, The transform callback passed to the Zod
schema's .transform call uses an untyped parameter `ts`; update the lambda used
with .transform (the `(ts) => DateTime.fromSeconds(...)` callback) to annotate
the parameter as a number (i.e., `(ts: number)`) so it satisfies strict
TypeScript; keep the body using DateTime.fromSeconds(ts, { zone: "utc" })
unchanged.
Comment on lines
+42
to
+48
| return { | ||
| basicUsageType: v.basicusagetype, | ||
| debitAmount, | ||
| metadata: v.metadata | ||
| ? (JSON.parse(v.metadata) as Record<string, unknown>) | ||
| : undefined, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant lines
if [ -f "src/zod/event.ts" ]; then
echo "=== Lines 35-55 (covering line 42-48) ==="
sed -n '35,55p' src/zod/event.ts
echo ""
echo "=== Lines 120-135 (covering line 127-129) ==="
sed -n '120,135p' src/zod/event.ts
else
echo "File not found at src/zod/event.ts"
git ls-files | grep -i event
fiRepository: ScrawnDotDev/Scrawn
Length of output: 1291
🏁 Script executed:
# Search for other .transform() calls with JSON.parse to find similar patterns
rg "JSON\.parse" src/zod/event.ts -B 5 -A 5Repository: ScrawnDotDev/Scrawn
Length of output: 624
🏁 Script executed:
# Get more context around the .transform() calls to see the full method signatures
echo "=== Context around line 42 (first occurrence) ==="
sed -n '30,55p' src/zod/event.ts
echo ""
echo "=== Context around line 127 (second occurrence) ==="
sed -n '115,135p' src/zod/event.tsRepository: ScrawnDotDev/Scrawn
Length of output: 1540
Guard metadata JSON parsing to prevent non-Zod runtime failures.
JSON.parse(v.metadata) can throw SyntaxError inside .transform(...), which escapes as a non-Zod error path. Convert invalid metadata into a Zod issue instead by accepting the ctx parameter and using ctx.addIssue().
Suggested fix
- .transform(async (v): Promise<BasicUsageEventData> => {
+ .transform(async (v, ctx): Promise<BasicUsageEventData> => {
@@
- return {
+ let metadata: Record<string, unknown> | undefined;
+ if (v.metadata) {
+ try {
+ metadata = JSON.parse(v.metadata) as Record<string, unknown>;
+ } catch {
+ ctx.addIssue({ code: "custom", message: "metadata must be valid JSON" });
+ metadata = undefined;
+ }
+ }
+
+ return {
basicUsageType: v.basicusagetype,
debitAmount,
- metadata: v.metadata
- ? (JSON.parse(v.metadata) as Record<string, unknown>)
- : undefined,
+ metadata,
};
});
@@
- .transform(async (v): Promise<AITokenUsageEventData> => {
+ .transform(async (v, ctx): Promise<AITokenUsageEventData> => {
@@
- return {
+ let metadata: Record<string, unknown> | undefined;
+ if (v.metadata) {
+ try {
+ metadata = JSON.parse(v.metadata) as Record<string, unknown>;
+ } catch {
+ ctx.addIssue({ code: "custom", message: "metadata must be valid JSON" });
+ metadata = undefined;
+ }
+ }
+
+ return {
model: v.model,
@@
- metadata: v.metadata
- ? (JSON.parse(v.metadata) as Record<string, unknown>)
- : undefined,
+ metadata,
};
});This aligns with the coding guideline: "Use Zod schemas for all request validation; catch ZodError and convert to domain errors". The fix ensures unguarded exceptions are converted to proper Zod validation errors.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/storage/adapter/clickhouse/handlers/addBasicUsage.ts (1)
30-31: 💤 Low valueConsider wrapping
ensureUserExistserrors inStorageErrorfor consistent error handling.Unlike the ClickHouse insert which wraps errors in
StorageError.insertFailed, failures fromensureUserExistspropagate unwrapped. This creates inconsistent error types for callers.♻️ Proposed fix
- await ensureUserExists(event_data.userId); + try { + await ensureUserExists(event_data.userId); + } catch (e) { + throw StorageError.insertFailed( + `Failed to ensure user ${event_data.userId} exists`, + e instanceof Error ? e : new Error(String(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/handlers/addBasicUsage.ts` around lines 30 - 31, The call to ensureUserExists in addBasicUsage can throw unwrapped errors; catch errors from ensureUserExists(event_data.userId) and rethrow a StorageError (e.g., StorageError.insertFailed) so callers get consistent error types; include the original error message or error as the cause when constructing the StorageError to preserve context and mirror how the ClickHouse insert error is handled.
🤖 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/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts`:
- Around line 147-150: The current handler calls ensureUserExists only for
firstEvent and swallows errors with .catch(() => {}), causing inconsistency and
hidden failures; change this to iterate over the unique userIds from events
(derive a Set from events.map(e => e.userId)) and for each id call await
ensureUserExists(userId) without silently catching errors so failures propagate
(matching the Postgres handler) — reference the ensureUserExists function, the
events array, and firstEvent variable in addAiTokenUsage handler and replace the
single swallowed call with a loop over unique userIds that awaits
ensureUserExists and lets errors surface.
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Line 132: The code currently calls ensureUserExists only for firstEvent.userId
in addAiTokenUsage, which misses other users in the events batch; change the
logic to collect all unique userIds from the events array and call
ensureUserExists for each unique id (i.e., iterate over new Set(events.map(e =>
e.userId)) and await ensureUserExists for each) so every user referenced by the
batch is ensured to exist; apply the same fix for the similar calls around the
ensureUserExists / firstEvent usage referenced near lines 138-140.
---
Nitpick comments:
In `@src/storage/adapter/clickhouse/handlers/addBasicUsage.ts`:
- Around line 30-31: The call to ensureUserExists in addBasicUsage can throw
unwrapped errors; catch errors from ensureUserExists(event_data.userId) and
rethrow a StorageError (e.g., StorageError.insertFailed) so callers get
consistent error types; include the original error message or error as the cause
when constructing the StorageError to preserve context and mirror how the
ClickHouse insert error is handled.
🪄 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: 568e2bbb-2b76-4826-a917-4fdafa5e904d
📒 Files selected for processing (5)
src/routes/http/registerWebhookRoutes.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/addBasicUsage.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/zod/event.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/http/registerWebhookRoutes.ts
- src/zod/event.ts
Comment on lines
+147
to
+150
| const firstEvent = events[0]; | ||
| if (firstEvent) { | ||
| await ensureUserExists(firstEvent.userId).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
Silent .catch(() => {}) hides failures and creates inconsistency with Postgres handler.
Swallowing ensureUserExists errors means:
- ClickHouse data may reference users that don't exist in Postgres (data inconsistency)
- DB connectivity or other serious errors are silently ignored
- Behavior differs from the Postgres handler which propagates errors
If user-ensure is truly best-effort for ClickHouse, at minimum log the error. Otherwise, consider failing fast to maintain consistency.
Additionally, similar to the Postgres handler, only the first event's userId is ensured—other users in the batch are skipped.
🛠️ Proposed fix (at minimum, log the error)
+import { WideEventLogger } from "../../../../errors/logger";
+
const firstEvent = events[0];
- if (firstEvent) {
- await ensureUserExists(firstEvent.userId).catch(() => {});
- }
+ const uniqueUserIds = [...new Set(events.map((e) => e.userId))];
+ for (const userId of uniqueUserIds) {
+ await ensureUserExists(userId).catch((err) => {
+ const logger = new WideEventLogger();
+ logger.lifecycleWarning(`Failed to ensure user ${userId} exists`, err);
+ });
+ }🤖 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 147
- 150, The current handler calls ensureUserExists only for firstEvent and
swallows errors with .catch(() => {}), causing inconsistency and hidden
failures; change this to iterate over the unique userIds from events (derive a
Set from events.map(e => e.userId)) and for each id call await
ensureUserExists(userId) without silently catching errors so failures propagate
(matching the Postgres handler) — reference the ensureUserExists function, the
events array, and firstEvent variable in addAiTokenUsage handler and replace the
single swallowed call with a loop over unique userIds that awaits
ensureUserExists and lets errors surface.
| } | ||
|
|
||
| const aggregatedEvents = await aggregateAiTokenEvents(events); | ||
| const firstEvent = events[0]; |
There was a problem hiding this comment.
ensureUserExists only called for first event's userId may skip other users.
Events in the batch may belong to different users (aggregation key is userId:model). Calling ensureUserExists only for firstEvent.userId means other users in the batch won't be ensured to exist, potentially causing foreign key violations or orphaned records.
Consider ensuring all unique userIds:
🛠️ Proposed fix
- const firstEvent = events[0];
+ const uniqueUserIds = [...new Set(events.map((e) => e.userId))];
return await executeInTransaction(
connectionObject,
`storing ${events.length} AI_TOKEN_USAGE event(s)`,
async (txn) => {
- if (firstEvent) {
- await ensureUserExists(firstEvent.userId, txn);
- }
+ for (const userId of uniqueUserIds) {
+ await ensureUserExists(userId, txn);
+ }Also applies to: 138-140
🤖 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` at line 132, The
code currently calls ensureUserExists only for firstEvent.userId in
addAiTokenUsage, which misses other users in the events batch; change the logic
to collect all unique userIds from the events array and call ensureUserExists
for each unique id (i.e., iterate over new Set(events.map(e => e.userId)) and
await ensureUserExists for each) so every user referenced by the batch is
ensured to exist; apply the same fix for the similar calls around the
ensureUserExists / firstEvent usage referenced near lines 138-140.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts (1)
147-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure all users in the batch, not only the first event user.
Current logic still only validates one
userId, so mixed-user batches can insert rows for users never ensured in Postgres.Proposed fix
- const firstEvent = events[0]; - if (firstEvent) { - await ensureUserExists(firstEvent.userId); - } + const uniqueUserIds = [...new Set(events.map((eventData) => eventData.userId))]; + for (const userId of uniqueUserIds) { + await ensureUserExists(userId); + }🤖 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 147 - 150, The current addAiTokenUsage handler only calls ensureUserExists for firstEvent.userId, letting mixed-user batches skip Postgres user creation; update the logic in addAiTokenUsage (where ensureUserExists is called) to collect all unique userIds from the events array and ensure each one exists (e.g., map unique IDs to ensureUserExists and await them, using Promise.all) before proceeding with inserts so every user in the batch is validated/created.
🧹 Nitpick comments (1)
src/storage/db/postgres/helpers/sessions.ts (1)
9-26: ⚡ Quick winConsider verifying at least one row was updated.
The update succeeds silently if no session matches
checkoutSessionId(0 rows affected). In payment flows, this could mask issues where a session is expected to exist but doesn't.♻️ Suggested fix to verify update affected a row
export async function markSessionProcessed( checkoutSessionId: string, txn?: PgTransaction<any, any, any> -): Promise<void> { +): Promise<boolean> { const db = txn ?? getPostgresDB(); try { - await db + const result = await db .update(sessionsTable) .set({ processed: true }) - .where(eq(sessionsTable.sessionId, checkoutSessionId)); + .where(eq(sessionsTable.sessionId, checkoutSessionId)) + .returning({ id: sessionsTable.id }); + + return result.length > 0; } catch (e) { throw StorageError.queryFailed( "Failed to mark session as processed", e instanceof Error ? e : new Error(String(e)) ); } }Alternatively, if the session must exist, throw
StorageError.emptyResultwhen no rows are affected.🤖 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/db/postgres/helpers/sessions.ts` around lines 9 - 26, The markSessionProcessed function currently performs an update that may affect 0 rows silently; change it to inspect the update result from db.update(sessionsTable)...where(eq(sessionsTable.sessionId, checkoutSessionId)) and ensure at least one row was affected—if zero rows are affected, throw StorageError.emptyResult (or an appropriate StorageError) so missing sessions aren’t masked; keep the try/catch but base the empty-result check on the update response returned by the db call (use the same markSessionProcessed, sessionsTable, eq(...) and txn/getPostgresDB() symbols to locate and modify the code).
🤖 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 24-60: The code always wraps price calculation in a Postgres
transaction via executeInTransaction(getPostgresDB(), ...) which breaks
ClickHouse-only modes; change calculatePaymentPrice to detect which storage
adapters were returned (inspect sdkAdapter and aiAdapter or consult
StorageAdapterFactory/adapter.type) and only call executeInTransaction when the
adapters actually require Postgres; if adapters are ClickHouse (or not
Postgres), invoke sdkAdapter.price and aiAdapter.price directly (or use the
adapters' own transaction APIs) passing no Postgres txn so you avoid
cross-backend coupling; keep the existing validation/error handling
(StorageError.priceCalculationFailed) and ensure you reference the same symbols
sdkAdapter, aiAdapter, executeInTransaction, getPostgresDB, and
StorageAdapterFactory when making the conditional branching.
In `@src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts`:
- Line 149: The call to ensureUserExists has a stray extra parenthesis causing a
syntax error; update the invocation in addAiTokenUsage handler to call await
ensureUserExists(firstEvent.userId); (remove the extra ')' so the function name
ensureUserExists and argument firstEvent.userId are properly formed and
awaited).
In `@src/storage/adapter/postgres/handlers/priceRequest.ts`:
- Around line 24-26: The function uses a broad PgTransaction<any, any, any> type
for txn; replace this with a concrete transaction alias (e.g., PostgresTxn)
defined in the Postgres adapter types and use that alias everywhere instead of
any. Add a typed alias export (matching the actual generic parameters used by
your Postgres client) and update the function signature to use that alias for
the txn parameter, and replace other occurrences (helpers/handlers) that
currently import or declare PgTransaction<any, any, any> so they all reference
the new PostgresTxn alias; ensure getPostgresDB() return type aligns with the
alias as well.
In `@src/storage/adapter/postgres/postgres.ts`:
- Around line 85-87: The method blindly casts txn?: unknown to
PgTransaction<any, any, any> which breaks type safety; either change the
function signature to accept txn?: PgTransaction<TRecord, TArgs, TResult> with
appropriate generics (replace PgTransaction<any, any, any> with concrete generic
parameters) or add a runtime guard before casting (e.g., if (txn &&
isValidTransaction(txn)) throw or use it) — implement a small type guard
function isValidTransaction to verify required transaction shape before using
txn and update all usages of txn in the method (and the local variable tx) to
rely on the tightened signature or guarded/validated value.
---
Duplicate comments:
In `@src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts`:
- Around line 147-150: The current addAiTokenUsage handler only calls
ensureUserExists for firstEvent.userId, letting mixed-user batches skip Postgres
user creation; update the logic in addAiTokenUsage (where ensureUserExists is
called) to collect all unique userIds from the events array and ensure each one
exists (e.g., map unique IDs to ensureUserExists and await them, using
Promise.all) before proceeding with inserts so every user in the batch is
validated/created.
---
Nitpick comments:
In `@src/storage/db/postgres/helpers/sessions.ts`:
- Around line 9-26: The markSessionProcessed function currently performs an
update that may affect 0 rows silently; change it to inspect the update result
from db.update(sessionsTable)...where(eq(sessionsTable.sessionId,
checkoutSessionId)) and ensure at least one row was affected—if zero rows are
affected, throw StorageError.emptyResult (or an appropriate StorageError) so
missing sessions aren’t masked; keep the try/catch but base the empty-result
check on the update response returned by the db call (use the same
markSessionProcessed, sessionsTable, eq(...) and txn/getPostgresDB() symbols to
locate and modify the code).
🪄 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: 2ea66ba4-5ffe-4f95-a18c-55f2b9983e36
📒 Files selected for processing (12)
AGENTS.mdsrc/interface/storage/Storage.tssrc/services/pricingService.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/priceRequest.tssrc/storage/adapter/postgres/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/postgres/handlers/priceRequestBasicUsage.tssrc/storage/adapter/postgres/postgres.tssrc/storage/db/postgres/helpers/apiKeys.tssrc/storage/db/postgres/helpers/metadata.tssrc/storage/db/postgres/helpers/sessions.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/interface/storage/Storage.ts
- src/storage/adapter/clickhouse/ClickHouseAdapter.ts
Comment on lines
+24
to
+60
| return await executeInTransaction( | ||
| getPostgresDB(), | ||
| "calculating payment price", | ||
| async (txn) => { | ||
| const sdkPrice = await sdkAdapter.price( | ||
| userId, | ||
| "BASIC_USAGE", | ||
| beforeTimestampUtc, | ||
| mode, | ||
| txn | ||
| ); | ||
|
|
||
| if (typeof sdkPrice !== "number" || isNaN(sdkPrice)) { | ||
| throw StorageError.priceCalculationFailed( | ||
| userId, | ||
| new Error(`Invalid SDK price value returned: ${sdkPrice}`) | ||
| ); | ||
| } | ||
|
|
||
| const aiPrice = await aiAdapter.price( | ||
| userId, | ||
| "AI_TOKEN_USAGE", | ||
| beforeTimestampUtc, | ||
| mode, | ||
| txn | ||
| ); | ||
|
|
||
| const totalPrice = sdkPrice + aiPrice; | ||
| return totalPrice; | ||
| if (typeof aiPrice !== "number" || isNaN(aiPrice)) { | ||
| throw StorageError.priceCalculationFailed( | ||
| userId, | ||
| new Error(`Invalid AI price value returned: ${aiPrice}`) | ||
| ); | ||
| } | ||
|
|
||
| return sdkPrice + aiPrice; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Postgres-only transaction wrapper can break single-adapter ClickHouse mode.
calculatePaymentPrice now always opens a Postgres transaction even though pricing adapters come from StorageAdapterFactory. If the runtime adapter is ClickHouse, this introduces cross-backend coupling and can fail before pricing executes.
Based on learnings: "The Storage Factory can only return 1 adapter for ALL events, so either all events use Postgres or all events use Clickhouse".
🤖 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 24 - 60, The code always wraps
price calculation in a Postgres transaction via
executeInTransaction(getPostgresDB(), ...) which breaks ClickHouse-only modes;
change calculatePaymentPrice to detect which storage adapters were returned
(inspect sdkAdapter and aiAdapter or consult StorageAdapterFactory/adapter.type)
and only call executeInTransaction when the adapters actually require Postgres;
if adapters are ClickHouse (or not Postgres), invoke sdkAdapter.price and
aiAdapter.price directly (or use the adapters' own transaction APIs) passing no
Postgres txn so you avoid cross-backend coupling; keep the existing
validation/error handling (StorageError.priceCalculationFailed) and ensure you
reference the same symbols sdkAdapter, aiAdapter, executeInTransaction,
getPostgresDB, and StorageAdapterFactory when making the conditional branching.
Comment on lines
+24
to
+26
| txn?: PgTransaction<any, any, any> | ||
| ): Promise<number> { | ||
| const db = getPostgresDB(); | ||
| const db = txn ?? getPostgresDB(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify breadth of unsafe txn typing usage
rg -nP --type=ts 'PgTransaction<any,\s*any,\s*any>'Repository: ScrawnDotDev/Scrawn
Length of output: 1066
🏁 Script executed:
# Search for transaction type definitions and aliases
rg -nP --type=ts 'type.*Transaction|interface.*Transaction'Repository: ScrawnDotDev/Scrawn
Length of output: 1148
🏁 Script executed:
# Examine the postgres.ts setup to understand the DB schema typing
fd postgres.ts --type=ts | head -1 | xargs cat -n | head -100Repository: ScrawnDotDev/Scrawn
Length of output: 233
🏁 Script executed:
# Check if there's a database schema file that would inform typing
fd -e ts schema | head -5Repository: ScrawnDotDev/Scrawn
Length of output: 138
🏁 Script executed:
# Look for any typed transaction helper patterns
rg -nP --type=ts 'PgTransaction<' -B 2 -A 2 | head -50Repository: ScrawnDotDev/Scrawn
Length of output: 3430
🏁 Script executed:
# View the TransactionFn definition in addEventUtils.ts
cat -n src/storage/adapter/postgres/handlers/addEventUtils.ts | head -30Repository: ScrawnDotDev/Scrawn
Length of output: 1094
🏁 Script executed:
# Check the postgres schema to understand the proper type parameters
cat -n src/storage/db/postgres/schema.ts | head -80Repository: ScrawnDotDev/Scrawn
Length of output: 2823
🏁 Script executed:
# Search for actual usage of TransactionFn in the codebase
rg -nP 'TransactionFn' --type=tsRepository: ScrawnDotDev/Scrawn
Length of output: 235
Replace PgTransaction<any, any, any> with a concrete typed transaction alias.
Using any for transaction type parameters violates the repository's TypeScript guidelines. This pattern appears across multiple files (helpers and handlers). Define a typed transaction alias that captures the proper generic parameters and apply it consistently throughout the codebase.
Per coding guidelines: "Always use explicit types for function parameters and return values; avoid any".
🤖 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/priceRequest.ts` around lines 24 - 26,
The function uses a broad PgTransaction<any, any, any> type for txn; replace
this with a concrete transaction alias (e.g., PostgresTxn) defined in the
Postgres adapter types and use that alias everywhere instead of any. Add a typed
alias export (matching the actual generic parameters used by your Postgres
client) and update the function signature to use that alias for the txn
parameter, and replace other occurrences (helpers/handlers) that currently
import or declare PgTransaction<any, any, any> so they all reference the new
PostgresTxn alias; ensure getPostgresDB() return type aligns with the alias as
well.
Comment on lines
+85
to
+87
| txn?: unknown | ||
| ): Promise<number> { | ||
| const tx = txn as PgTransaction<any, any, any> | undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -100 src/storage/adapter/postgres/postgres.ts | tail -30Repository: ScrawnDotDev/Scrawn
Length of output: 786
🏁 Script executed:
sed -n '70,110p' src/storage/adapter/postgres/postgres.tsRepository: ScrawnDotDev/Scrawn
Length of output: 1002
🏁 Script executed:
sed -n '75,150p' src/storage/adapter/postgres/postgres.tsRepository: ScrawnDotDev/Scrawn
Length of output: 904
🏁 Script executed:
rg "handlePriceRequestBasicUsage|handlePriceRequestAiTokenUsage" -A 5Repository: ScrawnDotDev/Scrawn
Length of output: 5891
🏁 Script executed:
rg "PgTransaction" src/storage/ -A 2Repository: ScrawnDotDev/Scrawn
Length of output: 5032
Avoid blind-casting txn from unknown to PgTransaction.
This bypasses type safety and can propagate invalid objects into DB handlers, causing runtime failures deep in the query stack. Either add a runtime guard before casting (e.g., if (txn && isValidTransaction(txn))) or tighten the method signature to accept txn?: PgTransaction<any, any, any> directly.
Additionally, replace PgTransaction<any, any, any> with properly typed generic parameters per coding guidelines.
🤖 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/postgres.ts` around lines 85 - 87, The method
blindly casts txn?: unknown to PgTransaction<any, any, any> which breaks type
safety; either change the function signature to accept txn?:
PgTransaction<TRecord, TArgs, TResult> with appropriate generics (replace
PgTransaction<any, any, any> with concrete generic parameters) or add a runtime
guard before casting (e.g., if (txn && isValidTransaction(txn)) throw or use it)
— implement a small type guard function isValidTransaction to verify required
transaction shape before using txn and update all usages of txn in the method
(and the local variable tx) to rely on the tightened signature or
guarded/validated value.
SteakFisher
force-pushed
the
refactor/readthrough
branch
from
May 18, 2026 13:40
1449c30 to
806ec9b
Compare
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
Bug Fixes
Refactor
Chores
Documentation