Feat/environments - #37
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR threads explicit "production" | "test" mode through payment provider clients, session creation, pricing and event storage (ClickHouse/Postgres), updates DB schemas to persist mode and checkout URLs, and adds a proxied checkout redirect route. ChangesMode-aware payment and storage layer
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/storage/adapter/postgres/handlers/addEventUtils.ts (1)
60-66:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
modecan be dropped before insert despite a required DB columnLine 65 makes
modeoptional, and Line 81 conditionally omits it. SinceeventsTable.modeis required, this path can fail inserts at runtime.Suggested fix
export type EventInsertValues = { reported_timestamp: string; ingested_timestamp: string; userId: string; api_keyId: string | undefined; - mode?: "production" | "test"; + mode: "production" | "test"; }; @@ .values({ reported_timestamp: values.reported_timestamp, ingested_timestamp: values.ingested_timestamp, userId: values.userId, api_keyId: values.api_keyId, - ...(values.mode ? { mode: values.mode } : {}), + mode: values.mode, })Also applies to: 76-82
🤖 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/addEventUtils.ts` around lines 60 - 66, EventInsertValues declares mode as optional and the insert logic conditionally omits it, but eventsTable.mode is a required DB column so this can cause runtime insert failures; make mode required in EventInsertValues (remove the ?), and update the insert construction (the code that references eventsTable.mode / the conditional omit) to always include mode—either pass through the provided value or apply a sensible default (e.g., "production") before building the insert so every insert includes eventsTable.mode.src/routes/gRPC/payment/createCheckoutLink.ts (1)
67-82:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMake provider checkout creation and session persistence recoverable.
createProviderCheckouthappens beforehandleAddSession. If the DB write fails after the provider call succeeds, this RPC returns an error but leaves a live checkout session with no stored redirect record. A retry can then mint duplicate payment sessions. Add a compensating cancel/update path or persist a pending session before calling the provider.🤖 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/gRPC/payment/createCheckoutLink.ts` around lines 67 - 82, The current flow calls createCheckoutSession (provider creation) before handleAddSession (DB persistence), which can leave a live provider session if the DB write fails; make this recoverable by either persisting a pending session record first or adding a compensating cancel/update after a failed DB write. Concretely: create a pending session entry in the DB (e.g., status="pending") for validatedData.userId before calling createCheckoutSession, then call createCheckoutSession, and on success update that pending record with sessionId and checkoutUrl via handleAddSession (or a dedicated update method); alternatively, if you keep provider-first, catch DB errors and call the provider cancel function (or update via createProviderCheckout cancellation API) to clean up the remote session and surface a clear error. Ensure changes reference createCheckoutSession and handleAddSession (or the provider cancel API) so retries do not create duplicate live sessions.
🤖 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/gRPC/payment/createCheckoutLink.ts`:
- Around line 85-88: Validate process.env.APP_URL before using it to build the
proxy link: in the createCheckoutLink handler check that APP_URL is present and
a valid URL (parse it with new URL(APP_URL) in a try/catch), construct the
checkout URL using new URL(`/checkout/${sessionResult.id}`,
parsedAppUrl).toString() instead of string concatenation, and handle/throw a
clear error or log and return a gRPC error if APP_URL is missing or malformed;
then set that validated URL on CreateCheckoutLinkResponse via
response.setCheckoutlink(...).
- Line 53: The code currently uses a non-null assertion on auth.mode (const mode
= auth.mode!) which bypasses type checks; instead check auth.mode explicitly and
fail fast if it's null by throwing or returning a domain error before calling
calculatePrice() or createCheckoutSession(); replace the non-null assertion with
an early guard that validates auth.mode is "production" | "test" and then pass
that validated mode to calculatePrice() and createCheckoutSession().
In `@src/routes/gRPC/payment/paymentProvider.ts`:
- Line 20: The webhookKey property is reading
process.env.DODO_PAYMENTS_WEBHOOK_SIGNING_SECRET which doesn't match the
documented .env.example variable; update the usages in paymentProvider.ts (the
webhookKey assignments at the occurrences around the webhookKey lines) to read
process.env.DODO_PAYMENTS_WEBHOOK_SECRET instead of
DODO_PAYMENTS_WEBHOOK_SIGNING_SECRET so the env value from .env.example is
picked up (update both occurrences).
In `@src/routes/http/checkoutRedirect.ts`:
- Around line 4-10: In handleCheckoutRedirect, validate request.params.sessionId
with a Zod schema before calling getCheckoutUrl: create a Zod schema for the
params (including sessionId), parse request.params and catch ZodError, then
convert that ZodError into the appropriate domain/HTTP error (e.g., bad request)
and reply accordingly instead of proceeding to call getCheckoutUrl; ensure the
Zod validation and error conversion happen at the top of handleCheckoutRedirect
so getCheckoutUrl only runs with a validated sessionId.
In `@src/storage/adapter/clickhouse/ClickHouseAdapter.ts`:
- Line 28: The add(serialized: SerializedEvent, apiKeyId?: string, mode?:
"production" | "test") method currently accepts an optional mode which allows
callers to omit environment scoping; change the signature to require mode
(remove the optional ?), validate that mode is present at the start of add and
throw/reject if missing, and propagate the non-optional mode to any internal
handlers or DB calls; apply the same required-mode change and pre-DB validation
to the other write/pricing entry points in this adapter so no write or pricing
path can be invoked without an explicit "production" | "test" mode.
In `@src/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.ts`:
- Around line 48-49: The query currently omits the mode predicate when mode is
falsy so test and production data can mix; change the code to resolve a
defaultMode = mode || "production" (or similar) and always build the predicate
using that resolvedMode (e.g., const modeFilter = " AND mode = {mode:String}"),
then pass resolvedMode into the query parameters where the current mode would be
used (update any occurrences that build the query or parameters around
modeFilter and the places noted at lines 51-55 and 57-59 so they use
resolvedMode instead of possibly undefined mode).
In `@src/storage/adapter/clickhouse/handlers/priceRequestSdkCall.ts`:
- Around line 46-47: The optional mode filter currently allows requests without
mode which can mix production/test billing; change the logic in
priceRequestSdkCall so mode defaults to "production" and always append the
filter expression (use the existing modeFilter variable and the query
construction that references it) instead of conditionally omitting it; update
all related occurrences where modeFilter is built/used (including the blocks
around the existing modeFilter declaration and the other uses at the sections
corresponding to the other occurrences) to pass or bind the default "production"
value and keep the SQL fragment "AND mode = {mode:String}" in every query.
In `@src/storage/adapter/clickhouse/schema.ts`:
- Line 9: Existing tables will not get the new mode column because CREATE TABLE
IF NOT EXISTS does not alter schemas; before each of the three CREATE TABLE
statements in src/storage/adapter/clickhouse/schema.ts, add an ALTER TABLE for
that table to add the mode column if missing (use ADD COLUMN IF NOT EXISTS with
a sensible default such as 'production') so pre-existing tables gain the mode
String column prior to any inserts/queries that reference it.
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Around line 26-30: The batch insert path in handleAddAiTokenUsage may omit the
required eventsTable.mode when mode param is undefined; update the batch insert
to always supply a valid mode for each row by populating eventsTable.mode with
the provided mode param or a safe default (e.g., 'production') before calling
the insert, or validate/throw if mode must be explicit; change the conditional
exclusion logic so the insert payload always includes eventsTable.mode (e.g.,
eventsTable.mode: mode ?? 'production') to prevent DB constraint errors.
In `@src/storage/db/postgres/helpers/sessions.ts`:
- Line 13: The session model currently treats checkoutUrl as optional and
inserts it without validation; update the code that constructs and inserts
sessions to require and validate checkoutUrl (e.g., non-empty string and valid
URL format using a URL constructor or regex) before any DB operation, and wrap
the insert in a Drizzle ORM transaction (use db.transaction or the project's
transaction helper around sessions.insert/insertSession) so the validation runs
inside the transactional flow; additionally catch DB unique-constraint errors
(Postgres code 23505) around the insert and handle them explicitly (translate to
a clear conflict error or retry logic) instead of letting raw DB errors bubble
up.
---
Outside diff comments:
In `@src/routes/gRPC/payment/createCheckoutLink.ts`:
- Around line 67-82: The current flow calls createCheckoutSession (provider
creation) before handleAddSession (DB persistence), which can leave a live
provider session if the DB write fails; make this recoverable by either
persisting a pending session record first or adding a compensating cancel/update
after a failed DB write. Concretely: create a pending session entry in the DB
(e.g., status="pending") for validatedData.userId before calling
createCheckoutSession, then call createCheckoutSession, and on success update
that pending record with sessionId and checkoutUrl via handleAddSession (or a
dedicated update method); alternatively, if you keep provider-first, catch DB
errors and call the provider cancel function (or update via
createProviderCheckout cancellation API) to clean up the remote session and
surface a clear error. Ensure changes reference createCheckoutSession and
handleAddSession (or the provider cancel API) so retries do not create duplicate
live sessions.
In `@src/storage/adapter/postgres/handlers/addEventUtils.ts`:
- Around line 60-66: EventInsertValues declares mode as optional and the insert
logic conditionally omits it, but eventsTable.mode is a required DB column so
this can cause runtime insert failures; make mode required in EventInsertValues
(remove the ?), and update the insert construction (the code that references
eventsTable.mode / the conditional omit) to always include mode—either pass
through the provided value or apply a sensible default (e.g., "production")
before building the insert so every insert includes eventsTable.mode.
🪄 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: f4100d0c-c88a-4dc1-bf34-5a03b049e279
📒 Files selected for processing (27)
.env.examplesrc/interface/storage/Storage.tssrc/routes/gRPC/payment/createCheckoutLink.tssrc/routes/gRPC/payment/paymentProvider.tssrc/routes/http/checkoutRedirect.tssrc/routes/http/createdCheckout.tssrc/servers/fastifyServer.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/addPayment.tssrc/storage/adapter/clickhouse/handlers/addSdkCall.tssrc/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/priceRequestSdkCall.tssrc/storage/adapter/clickhouse/schema.tssrc/storage/adapter/common/priceRequestPayment.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addEventUtils.tssrc/storage/adapter/postgres/handlers/addPayment.tssrc/storage/adapter/postgres/handlers/addSdkCall.tssrc/storage/adapter/postgres/handlers/index.tssrc/storage/adapter/postgres/handlers/priceRequest.tssrc/storage/adapter/postgres/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/postgres/handlers/priceRequestSdkCall.tssrc/storage/adapter/postgres/postgres.tssrc/storage/db/postgres/helpers/sessions.tssrc/storage/db/postgres/schema.tssrc/utils/eventHelpers.ts
💤 Files with no reviewable changes (1)
- src/storage/adapter/postgres/handlers/index.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/eventHelpers.ts (1)
31-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNon-null assertion masks a real type mismatch — dashboard roles can have
auth.mode = null.The
auth.mode!assertion bypasses type safety for a legitimate case. ThegetModeForRole()function returnsnullwhen the API key role is "dashboard", makingauth.modenullable. However,adapter.add()requiresmode: "production" | "test"(non-nullable). Passingnullviolates the storage contract. As per coding guidelines, validate all inputs before DB operations.🛡️ Proposed fix with runtime validation
export async function storeEvent( event: Event, auth: AuthContext ): Promise<void> { const adapter = await StorageAdapterFactory.getEventStorageAdapter( event.type ); + if (!auth.mode) { + throw new Error("auth.mode is required for event storage"); + } await adapter.add(event.serialize(), auth.apiKeyId, auth.mode); }🤖 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/utils/eventHelpers.ts` around lines 31 - 39, storeEvent currently uses a non-null assertion on auth.mode which can be null for dashboard roles; update storeEvent to validate auth.mode (from getModeForRole) before calling StorageAdapterFactory.getEventStorageAdapter/add: if auth.mode is null either throw a clear error or map dashboard role to an explicit allowed mode (or refuse the operation), and pass the validated non-null "production" | "test" value into adapter.add; reference the storeEvent function, StorageAdapterFactory.getEventStorageAdapter, and adapter.add when making the change.
🤖 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.
Outside diff comments:
In `@src/utils/eventHelpers.ts`:
- Around line 31-39: storeEvent currently uses a non-null assertion on auth.mode
which can be null for dashboard roles; update storeEvent to validate auth.mode
(from getModeForRole) before calling
StorageAdapterFactory.getEventStorageAdapter/add: if auth.mode is null either
throw a clear error or map dashboard role to an explicit allowed mode (or refuse
the operation), and pass the validated non-null "production" | "test" value into
adapter.add; reference the storeEvent function,
StorageAdapterFactory.getEventStorageAdapter, and adapter.add when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79b503fc-38be-493a-aa27-6cbac97bb5cb
📒 Files selected for processing (21)
src/interface/storage/Storage.tssrc/routes/gRPC/payment/createCheckoutLink.tssrc/routes/http/createdCheckout.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/addPayment.tssrc/storage/adapter/clickhouse/handlers/addSdkCall.tssrc/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/priceRequestSdkCall.tssrc/storage/adapter/common/priceRequestPayment.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addEventUtils.tssrc/storage/adapter/postgres/handlers/addPayment.tssrc/storage/adapter/postgres/handlers/addSdkCall.tssrc/storage/adapter/postgres/handlers/priceRequest.tssrc/storage/adapter/postgres/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/postgres/handlers/priceRequestSdkCall.tssrc/storage/adapter/postgres/postgres.tssrc/storage/db/postgres/helpers/sessions.tssrc/storage/db/postgres/schema.tssrc/utils/eventHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/storage/adapter/postgres/handlers/addSdkCall.ts
- src/storage/adapter/common/priceRequestPayment.ts
- src/storage/db/postgres/schema.ts
- src/routes/gRPC/payment/createCheckoutLink.ts
- src/storage/db/postgres/helpers/sessions.ts
This was referenced May 17, 2026
Merged
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
New Features
Bug Fixes
Chores
Other