refactor(payments): Removed payments from clickhouse - #43
Conversation
📝 WalkthroughWalkthroughThis PR removes ChangesRemove Payment Event Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/http/createdCheckout.ts (1)
140-146:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake session update, user update, and payment insert atomic.
This flow can commit
sessions.processed=trueand user updates before payment insert succeeds. If insert fails, retries can be ignored and the payment event is permanently lost. Wrap all three writes in a single transaction and only markprocessedwhen payment persistence succeeds.As per coding guidelines, "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly".
🤖 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 140 - 146, Wrap the three DB writes (update usersTable.last_billed_timestamp, update sessionsTable.processed, and the call to storePaymentEvent) in a Drizzle ORM transaction so they succeed or roll back together: start a transaction, validate inputs (userId, checkout_session_id, payment_id, creditAmount, apiKeyId) before any DB calls, call storePaymentEvent inside the transaction (or perform its inserts/updates via the transaction-bound DB instance), and only set sessionsTable.processed = true after the payment insert succeeds; additionally catch and explicitly handle unique constraint violations from the payment insert (treat as idempotent or return existing) and ensure builder.setUser/builder.setPaymentContext are used with the same validated values inside the transactional flow to keep state consistent.
🤖 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/db/postgres/helpers/payments.ts`:
- Around line 12-21: The current guard for creditAmount in payments.ts allows
zero even though the error message and contract require a positive finite
number; update the conditional that currently checks `creditAmount < 0` to
`creditAmount <= 0` so zero is rejected, keeping the existing type and
Number.isFinite checks intact, and ensure the thrown StorageError.invalidData
message remains accurate for values <= 0.
- Around line 44-49: Replace the fragile shape-check on the caught error with a
concrete type guard by using instanceof StorageError (remove the `(e as any)`
cast and the `"type" in e` shape checks) so existing StorageError instances are
not re-wrapped as insertFailed; update the catch block in the helper where `e`
is inspected (the postgresql payments helper) to import/ reference StorageError
and perform `if (e instanceof StorageError) { ... }` and handle other errors
separately.
---
Outside diff comments:
In `@src/routes/http/createdCheckout.ts`:
- Around line 140-146: Wrap the three DB writes (update
usersTable.last_billed_timestamp, update sessionsTable.processed, and the call
to storePaymentEvent) in a Drizzle ORM transaction so they succeed or roll back
together: start a transaction, validate inputs (userId, checkout_session_id,
payment_id, creditAmount, apiKeyId) before any DB calls, call storePaymentEvent
inside the transaction (or perform its inserts/updates via the transaction-bound
DB instance), and only set sessionsTable.processed = true after the payment
insert succeeds; additionally catch and explicitly handle unique constraint
violations from the payment insert (treat as idempotent or return existing) and
ensure builder.setUser/builder.setPaymentContext are used with the same
validated values inside the transactional flow to keep state consistent.
🪄 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: e800c643-467e-49c5-a8b6-c482259edb03
📒 Files selected for processing (15)
src/events/Payment.tssrc/factory/EventStorageAdapterFactory.tssrc/interface/event/Event.tssrc/routes/http/createdCheckout.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/addPayment.tssrc/storage/adapter/clickhouse/handlers/index.tssrc/storage/adapter/clickhouse/handlers/queryEvents.tssrc/storage/adapter/clickhouse/schema.tssrc/storage/adapter/common/queryEventsBase.tssrc/storage/adapter/postgres/handlers/addPayment.tssrc/storage/adapter/postgres/handlers/index.tssrc/storage/adapter/postgres/handlers/queryEvents.tssrc/storage/adapter/postgres/postgres.tssrc/storage/db/postgres/helpers/payments.ts
💤 Files with no reviewable changes (8)
- src/storage/adapter/clickhouse/handlers/index.ts
- src/events/Payment.ts
- src/storage/adapter/clickhouse/handlers/addPayment.ts
- src/storage/adapter/clickhouse/schema.ts
- src/storage/adapter/postgres/handlers/index.ts
- src/storage/adapter/clickhouse/ClickHouseAdapter.ts
- src/storage/adapter/postgres/postgres.ts
- src/storage/adapter/postgres/handlers/addPayment.ts
Summary by CodeRabbit
Refactor
Improvements