Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughAdds a paginated customer credit-transactions endpoint, threads Ledger and AccountResolver through DI and server wiring, makes balance lookups cursor-aware, implements page-based ledger transaction listing and transaction cursors, and propagates per-charge annotations into committed ledger transactions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as ListCreditTransactions Handler
participant Ledger as Ledger Service
participant Historical as Historical Repo
participant DB as Database
participant Balance as CustomerBalance Facade
Client->>Handler: GET /customers/{id}/credits/transactions
Handler->>Handler: parse & validate page + filters
Handler->>Ledger: resolve customer's FBO account
Ledger-->>Handler: accountID
alt account exists
Handler->>Historical: ListTransactionsByPage(page, accountID, currency?, movement?, annotations?)
Historical->>DB: query transactions (account, currency, annotations, movement) with pagination
DB-->>Historical: transactions + total
Historical-->>Handler: paginated results
alt transactions non-empty
Handler->>Balance: GetBalance(ctx, CustomerID, Currency, After=firstTx.Cursor())
Balance->>Ledger: query balance after cursor
Ledger-->>Balance: Balance
Balance-->>Handler: Decimal balance
Handler->>Handler: compute running before/after balances and map items
end
else no account
Handler-->>Client: empty page response
end
Handler-->>Client: PagePaginationResponse[BillingCreditTransaction]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GAlexIHU
force-pushed
the
feat/transaction-listing
branch
from
b425601 to
de598ef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
api/v3/server/server.go (1)
223-223: Consider guarding handler creation with the new dependencies too.Since this constructor now requires
config.Ledgerandconfig.AccountResolver, adding them to theifcondition would make partial wiring fail safer.🔧 Suggested tweak
-if config.CustomerBalanceFacade != nil && config.Credits.Enabled { +if config.CustomerBalanceFacade != nil && config.Credits.Enabled && config.Ledger != nil && config.AccountResolver != nil { customersCreditsHandler = customerscreditshandler.New(resolveNamespace, config.CustomerService, config.CustomerBalanceFacade, config.CreditGrantService, config.Ledger, config.AccountResolver, httptransport.WithErrorHandler(config.ErrorHandler)) }Based on learnings: Ensure
credits.enabledis guarded at multiple layers: ledger-backed customer credit handlers inapi/v3/server, customer ledger hooks, and namespace/default-account provisioning must each be wired separately and stay disabled when credits are off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/server/server.go` at line 223, The customers credit handler is now constructed with additional deps (config.Ledger and config.AccountResolver) but the existing enabled check only verifies credits.enabled; update the guard around the customerscreditshandler.New call so it only constructs customersCreditsHandler when credits are enabled AND both config.Ledger and config.AccountResolver are non-nil/available; reference the customersCreditsHandler variable and the customerscreditshandler.New constructor and short-circuit creation when either config.Ledger or config.AccountResolver is missing to ensure safe wiring when ledger-backed credits are disabled.openmeter/ledger/chargeadapter/creditpurchase.go (1)
81-85: Optional cleanup: extract the repeated input-annotation loop.Same block appears in three places; a small helper would make this easier to maintain.
♻️ Suggested refactor
+func withAnnotationsOnInputs(inputs []ledger.TransactionInput, annotations models.Annotations) []ledger.TransactionInput { + for i, input := range inputs { + if input != nil { + inputs[i] = transactions.WithAnnotations(input, annotations) + } + } + return inputs +} ... - for i, input := range inputs { - if input != nil { - inputs[i] = transactions.WithAnnotations(input, annotations) - } - } + inputs = withAnnotationsOnInputs(inputs, annotations)Also applies to: 135-139, 251-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/creditpurchase.go` around lines 81 - 85, Extract the repeated loop that applies annotations to inputs into a small helper (e.g., annotateInputs or applyAnnotations) that takes the inputs slice and annotations and returns the updated slice; replace the three occurrences of the inline loop (the blocks that iterate over inputs, check for nil, and call transactions.WithAnnotations) with calls to this helper so all sites (the ones using variables inputs and annotations and calling transactions.WithAnnotations) share the same logic and reduce duplication.openmeter/ledger/chargeadapter/usagebased_test.go (1)
324-345: Consider extracting shared test helper.The
transactionAnnotationshelper is identical to the one increditpurchase_test.go. If more charge adapter tests need this pattern, it might be worth extracting toledgertestutils.IntegrationEnvor a shared test helper to reduce duplication.That said, this is a minor point and doesn't block the PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/usagebased_test.go` around lines 324 - 345, The transactionAnnotations helper in usageBasedHandlerTestEnv is duplicated from creditpurchase_test.go; extract it to a shared test helper (e.g., add a method TransactionAnnotations(ctx context.Context, db *DBType, namespace string, groupID string) []models.Annotations on ledgertestutils.IntegrationEnv or a package-level helper in ledgertestutils) and update usageBasedHandlerTestEnv.transactionAnnotations to call that shared helper (or remove the duplicate and call ledgertestutils.TransactionAnnotations directly) so both tests use the single implementation referenced by the unique symbols transactionAnnotations, usageBasedHandlerTestEnv, and ledgertestutils.IntegrationEnv.api/v3/handlers/customers/credits/list_transactions.go (2)
210-218: Verify balance roll-forward direction matches API expectations.The balance calculation walks backwards through time (newest to oldest). Just want to confirm the semantics are intentional:
After= balance immediately after this transaction was appliedBefore= balance immediately before this transaction was appliedThis means for a "consumed" tx with amount
-10andAfter = 100, we getBefore = 110. That reads correctly to me, but worth a quick sanity check that API consumers expect this interpretation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/list_transactions.go` around lines 210 - 218, The current applyCreditTransactionBalances function computes balances by walking from newest to oldest using runningBalance and sets API.AvailableBalance.After = runningBalance and Before = runningBalance.Sub(items[i].Amount); confirm whether API consumers expect After to be the post-transaction balance and Before to be pre-transaction; if they expect the opposite, either reverse the iteration order or swap the assignments (set After = runningBalance.Sub(items[i].Amount) and Before = runningBalance) and update any tests; refer to applyCreditTransactionBalances, mappedCreditTransaction.Amount, and API.AvailableBalance.After/Before when making the change.
280-290: Consider whether missing FBO entry is truly an error condition.The query already filters transactions by the FBO account ID (line 111), so transactions returned should always have an FBO entry. If this error ever fires, it might indicate a data integrity issue or a bug in the query filtering.
A debug log here could help surface such edge cases without failing the request, but the current defensive error is also reasonable. Just noting the trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/list_transactions.go` around lines 280 - 290, The current creditTransactionEntry function treats a missing customer FBO entry as an error; instead, log a debug message with the transaction ID and return (nil, nil) so the request doesn't fail for this edge case, and then update any callers (e.g., the transactions listing path that calls creditTransactionEntry) to treat a nil entry as "skip this transaction" rather than a fatal error; modify creditTransactionEntry to emit a debug log with tx.ID().ID and return nil, nil, and adjust the caller(s) to check for entry == nil and continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/ledger/historical/ledger.go`:
- Around line 61-65: ListTransactionsByPage currently calls
l.repo.ListTransactionsByPage without validating the pagination input; mirror
the validation performed by ListTransactions by validating params (the same
page/pagination checks used in ListTransactions) before calling
l.repo.ListTransactionsByPage, and return an early wrapped error if validation
fails (e.g., return fmt.Errorf("invalid pagination: %w", err)). Ensure the
validation logic references the same validation routine or checks used by
ListTransactions so both entry points enforce the same page constraints.
In `@openmeter/server/router/router.go`:
- Around line 108-109: Config.Validate() must guard the new credits
dependencies: if the credits feature flag (credits.enabled) is true then
validate that the Config fields Ledger and AccountResolver are non-nil and
return a descriptive validation error otherwise; if credits.enabled is false,
allow Ledger and AccountResolver to be nil. Update the Validate method to check
the boolean flag and fail-fast with a clear message referencing Ledger and
AccountResolver so wiring issues surface at startup (this affects the
ledger-backed customer credit handlers, customer ledger hooks, and
namespace/default-account provisioning paths).
---
Nitpick comments:
In `@api/v3/handlers/customers/credits/list_transactions.go`:
- Around line 210-218: The current applyCreditTransactionBalances function
computes balances by walking from newest to oldest using runningBalance and sets
API.AvailableBalance.After = runningBalance and Before =
runningBalance.Sub(items[i].Amount); confirm whether API consumers expect After
to be the post-transaction balance and Before to be pre-transaction; if they
expect the opposite, either reverse the iteration order or swap the assignments
(set After = runningBalance.Sub(items[i].Amount) and Before = runningBalance)
and update any tests; refer to applyCreditTransactionBalances,
mappedCreditTransaction.Amount, and API.AvailableBalance.After/Before when
making the change.
- Around line 280-290: The current creditTransactionEntry function treats a
missing customer FBO entry as an error; instead, log a debug message with the
transaction ID and return (nil, nil) so the request doesn't fail for this edge
case, and then update any callers (e.g., the transactions listing path that
calls creditTransactionEntry) to treat a nil entry as "skip this transaction"
rather than a fatal error; modify creditTransactionEntry to emit a debug log
with tx.ID().ID and return nil, nil, and adjust the caller(s) to check for entry
== nil and continue.
In `@api/v3/server/server.go`:
- Line 223: The customers credit handler is now constructed with additional deps
(config.Ledger and config.AccountResolver) but the existing enabled check only
verifies credits.enabled; update the guard around the
customerscreditshandler.New call so it only constructs customersCreditsHandler
when credits are enabled AND both config.Ledger and config.AccountResolver are
non-nil/available; reference the customersCreditsHandler variable and the
customerscreditshandler.New constructor and short-circuit creation when either
config.Ledger or config.AccountResolver is missing to ensure safe wiring when
ledger-backed credits are disabled.
In `@openmeter/ledger/chargeadapter/creditpurchase.go`:
- Around line 81-85: Extract the repeated loop that applies annotations to
inputs into a small helper (e.g., annotateInputs or applyAnnotations) that takes
the inputs slice and annotations and returns the updated slice; replace the
three occurrences of the inline loop (the blocks that iterate over inputs, check
for nil, and call transactions.WithAnnotations) with calls to this helper so all
sites (the ones using variables inputs and annotations and calling
transactions.WithAnnotations) share the same logic and reduce duplication.
In `@openmeter/ledger/chargeadapter/usagebased_test.go`:
- Around line 324-345: The transactionAnnotations helper in
usageBasedHandlerTestEnv is duplicated from creditpurchase_test.go; extract it
to a shared test helper (e.g., add a method TransactionAnnotations(ctx
context.Context, db *DBType, namespace string, groupID string)
[]models.Annotations on ledgertestutils.IntegrationEnv or a package-level helper
in ledgertestutils) and update usageBasedHandlerTestEnv.transactionAnnotations
to call that shared helper (or remove the duplicate and call
ledgertestutils.TransactionAnnotations directly) so both tests use the single
implementation referenced by the unique symbols transactionAnnotations,
usageBasedHandlerTestEnv, and ledgertestutils.IntegrationEnv.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fd2f635-9370-4888-9442-ed86635a0484
📒 Files selected for processing (39)
api/v3/handlers/customers/credits/handler.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/handlers/customers/credits/list_transactions_test.goapi/v3/server/routes.goapi/v3/server/server.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/ledger/account/account.goopenmeter/ledger/account/subaccount.goopenmeter/ledger/annotations.goopenmeter/ledger/chargeadapter/annotations.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/collector/collect.goopenmeter/ledger/collector/correct.goopenmeter/ledger/collector/service.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/repo.goopenmeter/ledger/historical/transaction.goopenmeter/ledger/ledger_test.goopenmeter/ledger/noop/noop.goopenmeter/ledger/primitives.goopenmeter/ledger/query.goopenmeter/ledger/transactions/correction_test.goopenmeter/server/router/router.goopenmeter/server/server.gotest/credits/sanity_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
openmeter/ledger/historical/ledger.go (1)
61-65:⚠️ Potential issue | 🟠 MajorPlease validate page params before repo call (same as
ListTransactions).At Line 62, this path skips input validation, so invalid pagination can reach the repo/DB. It should mirror the validation guard used in
ListTransactions.Suggested patch
func (l *Ledger) ListTransactionsByPage(ctx context.Context, params ledger.ListTransactionsByPageInput) (pagepagination.Result[ledger.Transaction], error) { + if err := params.Validate(); err != nil { + return pagepagination.Result[ledger.Transaction]{}, fmt.Errorf("failed to validate list transactions by page input: %w", err) + } + res, err := l.repo.ListTransactionsByPage(ctx, params) if err != nil { return pagepagination.Result[ledger.Transaction]{}, fmt.Errorf("list transactions by page: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/historical/ledger.go` around lines 61 - 65, ListTransactionsByPage currently forwards params to l.repo.ListTransactionsByPage without validating pagination, allowing invalid page params to hit the repo; mirror the same validation guard used in ListTransactions by validating the incoming params (the page/pagination fields) before calling l.repo.ListTransactionsByPage and return a wrapped error if validation fails. Locate the ListTransactionsByPage method and add the same param validation logic used in ListTransactions (use the same function/validation call or checks that guard against invalid page size/number), then only call l.repo.ListTransactionsByPage when validation passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/list_transactions.go`:
- Around line 126-133: The page-level running balance is seeded from
items[0].Currency and reused for all items, which produces wrong
AvailableBalance when items contain mixed currencies; update the logic in
list_transactions.go (around customerFBOBalance and
applyCreditTransactionBalances usage) to maintain separate running balances
keyed by item.Currency: either enforce/validate a single-currency filter
earlier, or compute a map[currency]runningBalance by finding the first item for
each distinct currency, calling customerFBOBalance(ctx, request, currency,
&firstCursor) per currency, then pass the correct running balance for each item
(or call applyCreditTransactionBalances per-currency group) so each transaction
uses its currency-specific running balance.
- Around line 163-170: The switch that maps filters currently treats
BillingCreditTransactionTypeAdjusted as
ledger.ListTransactionsCreditMovementUnspecified which causes adjusted queries
to look like empty results; update the handler so that
BillingCreditTransactionTypeAdjusted is not converted into a fake "unspecified"
predicate — either return a 400 Bad Request when an adjusted filter is received
(reject the unsupported filter) or implement and return a real adjusted
predicate from the ledger layer instead of using
ledger.ListTransactionsCreditMovementUnspecified; change the case for
BillingCreditTransactionTypeAdjusted in the same switch (the code around
mapCreditTransaction()/list_transactions.go where
BillingCreditTransactionTypeFunded/Consumed/Adjusted are handled) to perform one
of those two fixes and ensure callers receive an error or a correct predicate
rather than a silent Total=0.
In `@api/v3/server/server.go`:
- Around line 76-77: When credits are enabled, fail fast in Config.Validate():
check cfg.Credits.Enabled (or credits.enabled) and if true ensure the server
dependencies cfg.Server.Ledger and cfg.Server.AccountResolver are non-nil; if
either is nil return a clear validation error indicating that
customersCreditsHandler dependencies are missing so the server cannot start with
credits enabled. Update Config.Validate() to perform these checks (referencing
Config.Validate, customersCreditsHandler, Ledger, AccountResolver, and
credits.enabled) rather than allowing construction to proceed with nil
dependencies.
In `@openmeter/ledger/historical/adapter/ledger.go`:
- Around line 324-325: The current code applies entryPredicates to the
eager-loaded Entries edge via q.Where(entryPredicates...), which trims the
Entries returned on each Transaction; instead, move the predicates produced by
listTransactionsEntryPredicates into the parent transaction query (apply them to
the main transaction query builder) so the transaction selection is scoped but
the eager-loaded Entries edge is loaded completely (remove
q.Where(entryPredicates...) from the Entries eager-load). Update the same
pattern where q.Where(entryPredicates...) is used (the other occurrences noted)
so ListTransactions and related functions select transactions with the
predicates but always preload all Entries for matched transactions.
---
Duplicate comments:
In `@openmeter/ledger/historical/ledger.go`:
- Around line 61-65: ListTransactionsByPage currently forwards params to
l.repo.ListTransactionsByPage without validating pagination, allowing invalid
page params to hit the repo; mirror the same validation guard used in
ListTransactions by validating the incoming params (the page/pagination fields)
before calling l.repo.ListTransactionsByPage and return a wrapped error if
validation fails. Locate the ListTransactionsByPage method and add the same
param validation logic used in ListTransactions (use the same
function/validation call or checks that guard against invalid page size/number),
then only call l.repo.ListTransactionsByPage when validation passes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6771e25-d53b-418d-a1ad-b8d3beefd4b7
📒 Files selected for processing (39)
api/v3/handlers/customers/credits/handler.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/handlers/customers/credits/list_transactions_test.goapi/v3/server/routes.goapi/v3/server/server.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/ledger/account/account.goopenmeter/ledger/account/subaccount.goopenmeter/ledger/annotations.goopenmeter/ledger/chargeadapter/annotations.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/collector/collect.goopenmeter/ledger/collector/correct.goopenmeter/ledger/collector/service.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/repo.goopenmeter/ledger/historical/transaction.goopenmeter/ledger/ledger_test.goopenmeter/ledger/noop/noop.goopenmeter/ledger/primitives.goopenmeter/ledger/query.goopenmeter/ledger/transactions/correction_test.goopenmeter/server/router/router.goopenmeter/server/server.gotest/credits/sanity_test.go
✅ Files skipped from review due to trivial changes (6)
- openmeter/ledger/chargeadapter/usagebased.go
- openmeter/server/router/router.go
- openmeter/ledger/customerbalance/noop.go
- openmeter/ledger/collector/collect.go
- openmeter/ledger/chargeadapter/annotations.go
- openmeter/server/server.go
🚧 Files skipped from review as they are similar to previous changes (18)
- openmeter/ledger/account/subaccount.go
- cmd/server/main.go
- openmeter/ledger/historical/transaction.go
- api/v3/server/routes.go
- openmeter/ledger/transactions/correction_test.go
- openmeter/ledger/historical/repo.go
- openmeter/ledger/collector/service.go
- openmeter/ledger/historical/adapter/sumentries_query.go
- openmeter/ledger/account/account.go
- openmeter/ledger/customerbalance/service.go
- api/v3/handlers/customers/credits/handler.go
- openmeter/ledger/customerbalance/facade_test.go
- openmeter/ledger/historical/adapter/ledger_test.go
- openmeter/ledger/chargeadapter/usagebased_test.go
- openmeter/ledger/annotations.go
- openmeter/ledger/ledger_test.go
- openmeter/ledger/chargeadapter/flatfee.go
- openmeter/ledger/customerbalance/facade.go
Comment on lines
+324
to
+325
| entryPredicates := listTransactionsEntryPredicates(input) | ||
|
|
Contributor
There was a problem hiding this comment.
Don’t trim the transaction’s entry set here.
q.Where(entryPredicates...) narrows the eager-loaded Entries edge itself, so any account/currency-scoped page now returns partial ledger.Transaction objects. That’s a pretty sharp behavior change from ListTransactions, and callers lose the counter-entries they’d normally expect on a transaction. I’d keep the scoping predicate on the parent transaction query and still load all entries for the matched rows.
💡 Safer query shape
func (r *repo) ListTransactionsByPage(ctx context.Context, input ledger.ListTransactionsByPageInput) (pagepagination.Result[*ledgerhistorical.Transaction], error) {
entryPredicates := listTransactionsEntryPredicates(input)
query := r.db.LedgerTransaction.Query().
Where(ledgertransactiondb.Namespace(input.Namespace)).
WithEntries(func(q *db.LedgerEntryQuery) {
- if len(entryPredicates) > 0 {
- q.Where(entryPredicates...)
- }
q.Order(
ledgerentrydb.ByCreatedAt(),
ledgerentrydb.ByID(),
)
q.WithSubAccount(func(sq *db.LedgerSubAccountQuery) {
sq.WithAccount()
sq.WithRoute()
})
}).
Order(
ledgertransactiondb.ByBookedAt(entsql.OrderDesc()),
ledgertransactiondb.ByCreatedAt(entsql.OrderDesc()),
ledgertransactiondb.ByID(entsql.OrderDesc()),
)
- if len(input.AccountIDs) > 0 {
- query = query.Where(
- ledgertransactiondb.HasEntriesWith(
- ledgerentrydb.HasSubAccountWith(
- ledgersubaccountdb.AccountIDIn(input.AccountIDs...),
- ),
- ),
- )
- }
-
- if input.Currency != nil {
- query = query.Where(
- ledgertransactiondb.HasEntriesWith(
- ledgerentrydb.HasSubAccountWith(
- ledgersubaccountdb.HasRouteWith(
- ledgersubaccountroutedb.Currency(string(*input.Currency)),
- ),
- ),
- ),
- )
- }
+ if len(entryPredicates) > 0 {
+ query = query.Where(ledgertransactiondb.HasEntriesWith(entryPredicates...))
+ }Also applies to: 328-339, 347-367, 404-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/historical/adapter/ledger.go` around lines 324 - 325, The
current code applies entryPredicates to the eager-loaded Entries edge via
q.Where(entryPredicates...), which trims the Entries returned on each
Transaction; instead, move the predicates produced by
listTransactionsEntryPredicates into the parent transaction query (apply them to
the main transaction query builder) so the transaction selection is scoped but
the eager-loaded Entries edge is loaded completely (remove
q.Where(entryPredicates...) from the Entries eager-load). Update the same
pattern where q.Where(entryPredicates...) is used (the other occurrences noted)
so ListTransactions and related functions select transactions with the
predicates but always preload all Entries for matched transactions.
turip
previously approved these changes
Apr 15, 2026
| ID: charge.ID, | ||
| }, | ||
| charge.Intent.Subscription, | ||
| ptrIfNotEmpty(charge.State.FeatureID), |
Member
There was a problem hiding this comment.
Suggested change
| ptrIfNotEmpty(charge.State.FeatureID), | |
| lo.EmptyableToPtr(charge.State.FeatureID), |
| return ListCreditTransactionsResponse{}, fmt.Errorf("list transactions: %w", err) | ||
| } | ||
|
|
||
| items, err := mapCreditTransactions(result.Items) |
Member
There was a problem hiding this comment.
I would add some validation here that the list result is single currency only, as single currency is assumed on the rest of execution flow.
| return req, nil | ||
| }, | ||
| func(ctx context.Context, request ListCreditTransactionsRequest) (ListCreditTransactionsResponse, error) { | ||
| creditMovement, empty := creditMovementFromTypeFilter(request.TypeFilter) |
Member
There was a problem hiding this comment.
I think there's way too much logic here inside the http handler. It would be better if we just provide a service call in my opinion so that we are not mixing the http mapping and the actual calculation logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openmeter/ledger/customerbalance/transactions.go (2)
151-165:⚠️ Potential issue | 🟠 MajorDon’t make
adjustedlook like “no data.”
creditTransactionType()can emitCreditTransactionTypeAdjusted, and the API layer exposes that enum too, but this mapping turns anadjustedfilter intoempty=true. Clients end up gettingTotalCount=0instead of either real matches or a clear “unsupported filter” error. Please either rejectadjustedup front or add a real ledger predicate for it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/transactions.go` around lines 151 - 165, The mapping in ledgerCreditMovement treats CreditTransactionTypeAdjusted as "unspecified" (returning ListTransactionsCreditMovementUnspecified, true, nil) which hides adjusted matches; update ledgerCreditMovement (and call sites if needed) to reject adjusted explicitly by returning an error (e.g., fmt.Errorf("adjusted credit transaction filter not supported")) so clients get a clear unsupported-filter error, or alternatively implement a proper ledger predicate for adjusted by returning a distinct ledger.ListTransactionsCreditMovement value and false along with nil error; modify the function ledgerCreditMovement accordingly (change the CreditTransactionTypeAdjusted case) so adjusted is not conflated with unspecified.
127-133:⚠️ Potential issue | 🟠 MajorRunning balances are still wrong on mixed-currency pages.
This seeds one running balance from
items[0].Currencyand reuses it for every row on the page. If the page contains multiple currencies andinput.Currencyis nil, every later currency gets the wrongBefore/Aftervalues. Easiest fix is to require a single-currency query here; otherwise you need separate running balances per currency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/transactions.go` around lines 127 - 133, The code seeds one running balance using items[0].Currency and applies it to all rows, which breaks when the page contains mixed currencies; update the logic in the block that calls GetBalance and applyCreditTransactionBalances so it either (A) enforces a single-currency query by returning an error or requiring input.Currency to be non-nil before proceeding, or (B) computes separate running balances per currency: group items by Currency, call s.GetBalance(ctx, input.CustomerID, routeFilter(<currency>), cursor) for each currency group's first item, then call applyCreditTransactionBalances on each group with its corresponding runningBalance.Settled(). Reference symbols: GetBalance, applyCreditTransactionBalances, input.Currency, items, routeFilter, ListCreditTransactionsResult.
🧹 Nitpick comments (1)
api/v3/handlers/customers/credits/handler.go (1)
34-35: Trim the unused ledger wiring for now.
ListCreditTransactions()goes throughbalanceFacade, and nothing in this handler usesledgeroraccountResolver. Threading both throughNew()makes DI and tests noisier without buying anything yet, so I’d keep the handler boundary on the facade until a call site actually needs the lower-level deps.Also applies to: 44-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/handler.go` around lines 34 - 35, Remove the unused lower-level ledger wiring from the handler by deleting the ledger and accountResolver fields and removing their injection from New(); keep the handler boundary using only balanceFacade which ListCreditTransactions() uses. Update the handler struct to remove ledger and accountResolver, adjust the New(...) constructor signature to stop accepting ledger.AccountResolver and ledger.Ledger, and remove any assignments for those fields in New so DI/tests no longer require those dependencies until a call site needs them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 369-377: The toAPIBillingCreditTransactionType function currently
maps any unknown customerbalance.CreditTransactionType to
BillingCreditTransactionTypeAdjusted; explicitly handle
customerbalance.CreditTransactionTypeAdjusted by returning
api.BillingCreditTransactionTypeAdjusted and change the default branch to
surface unmapped values (e.g., return an error or a sentinel/unmapped value)
instead of silently coercing them so callers can detect unknown/future enum
values; update the function signature and all callers of
toAPIBillingCreditTransactionType accordingly to handle the error or unmapped
result and reference customerbalance.CreditTransactionType and
api.BillingCreditTransactionType in your changes.
In `@openmeter/ledger/customerbalance/transactions.go`:
- Around line 168-183: The helper fboAccountIDFromCustomerAccounts silently
returns "" on unexpected FBO types which masks resolver contract errors; change
fboAccountIDFromCustomerAccounts to return (string, error) and have
customerFBOAccountID propagate that error (returning a descriptive error when
accounts.FBOAccount is not a *ledgeraccount.CustomerFBOAccount), then update
callers such as ListCreditTransactions (and any use of customerFBOAccountID) to
handle the error instead of falling back to emptyCreditTransactions; reference
functions: customerFBOAccountID, fboAccountIDFromCustomerAccounts,
ListCreditTransactions, emptyCreditTransactions, and
ledgeraccount.CustomerFBOAccount.
---
Duplicate comments:
In `@openmeter/ledger/customerbalance/transactions.go`:
- Around line 151-165: The mapping in ledgerCreditMovement treats
CreditTransactionTypeAdjusted as "unspecified" (returning
ListTransactionsCreditMovementUnspecified, true, nil) which hides adjusted
matches; update ledgerCreditMovement (and call sites if needed) to reject
adjusted explicitly by returning an error (e.g., fmt.Errorf("adjusted credit
transaction filter not supported")) so clients get a clear unsupported-filter
error, or alternatively implement a proper ledger predicate for adjusted by
returning a distinct ledger.ListTransactionsCreditMovement value and false along
with nil error; modify the function ledgerCreditMovement accordingly (change the
CreditTransactionTypeAdjusted case) so adjusted is not conflated with
unspecified.
- Around line 127-133: The code seeds one running balance using
items[0].Currency and applies it to all rows, which breaks when the page
contains mixed currencies; update the logic in the block that calls GetBalance
and applyCreditTransactionBalances so it either (A) enforces a single-currency
query by returning an error or requiring input.Currency to be non-nil before
proceeding, or (B) computes separate running balances per currency: group items
by Currency, call s.GetBalance(ctx, input.CustomerID, routeFilter(<currency>),
cursor) for each currency group's first item, then call
applyCreditTransactionBalances on each group with its corresponding
runningBalance.Settled(). Reference symbols: GetBalance,
applyCreditTransactionBalances, input.Currency, items, routeFilter,
ListCreditTransactionsResult.
---
Nitpick comments:
In `@api/v3/handlers/customers/credits/handler.go`:
- Around line 34-35: Remove the unused lower-level ledger wiring from the
handler by deleting the ledger and accountResolver fields and removing their
injection from New(); keep the handler boundary using only balanceFacade which
ListCreditTransactions() uses. Update the handler struct to remove ledger and
accountResolver, adjust the New(...) constructor signature to stop accepting
ledger.AccountResolver and ledger.Ledger, and remove any assignments for those
fields in New so DI/tests no longer require those dependencies until a call site
needs them.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36be7749-67a7-4f9c-ade9-3a3b754838c3
📒 Files selected for processing (17)
api/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/handler.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/handlers/customers/credits/list_transactions_test.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/customerbalance.goopenmeter/billing/creditgrant/noop.goopenmeter/ledger/chargeadapter/annotations.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/customerbalance/transactions_test.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/primitives.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/ledger/customerbalance/service.go
- openmeter/ledger/primitives.go
Comment on lines
+369
to
+377
| func toAPIBillingCreditTransactionType(txType customerbalance.CreditTransactionType) api.BillingCreditTransactionType { | ||
| switch txType { | ||
| case customerbalance.CreditTransactionTypeFunded: | ||
| return api.BillingCreditTransactionTypeFunded | ||
| case customerbalance.CreditTransactionTypeConsumed: | ||
| return api.BillingCreditTransactionTypeConsumed | ||
| default: | ||
| return api.BillingCreditTransactionTypeAdjusted | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t coerce unknown transaction types to adjusted.
The default branch turns any bad or future customerbalance.CreditTransactionType into a valid-looking adjustment, so clients can get silently mislabeled data. Please handle CreditTransactionTypeAdjusted explicitly and leave unknown values unmapped or error out instead.
💡 Minimal safer mapping
func toAPIBillingCreditTransactionType(txType customerbalance.CreditTransactionType) api.BillingCreditTransactionType {
switch txType {
case customerbalance.CreditTransactionTypeFunded:
return api.BillingCreditTransactionTypeFunded
case customerbalance.CreditTransactionTypeConsumed:
return api.BillingCreditTransactionTypeConsumed
+ case customerbalance.CreditTransactionTypeAdjusted:
+ return api.BillingCreditTransactionTypeAdjusted
default:
- return api.BillingCreditTransactionTypeAdjusted
+ return api.BillingCreditTransactionTypeNaked
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/handlers/customers/credits/convert.go` around lines 369 - 377, The
toAPIBillingCreditTransactionType function currently maps any unknown
customerbalance.CreditTransactionType to BillingCreditTransactionTypeAdjusted;
explicitly handle customerbalance.CreditTransactionTypeAdjusted by returning
api.BillingCreditTransactionTypeAdjusted and change the default branch to
surface unmapped values (e.g., return an error or a sentinel/unmapped value)
instead of silently coercing them so callers can detect unknown/future enum
values; update the function signature and all callers of
toAPIBillingCreditTransactionType accordingly to handle the error or unmapped
result and reference customerbalance.CreditTransactionType and
api.BillingCreditTransactionType in your changes.
Comment on lines
+168
to
+183
| func (s *Service) customerFBOAccountID(ctx context.Context, customerID customer.CustomerID) (string, error) { | ||
| accounts, err := s.AccountResolver.GetCustomerAccounts(ctx, customerID) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return fboAccountIDFromCustomerAccounts(accounts), nil | ||
| } | ||
|
|
||
| func fboAccountIDFromCustomerAccounts(accounts ledger.CustomerAccounts) string { | ||
| if fbo, ok := accounts.FBOAccount.(*ledgeraccount.CustomerFBOAccount); ok { | ||
| return fbo.ID().ID | ||
| } | ||
|
|
||
| return "" | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on an unexpected FBO account type.
Returning "" here makes ListCreditTransactions() fall back to emptyCreditTransactions(...), so a broken resolver contract gets surfaced as “no transactions.” It’d be safer to return an error instead of silently converting an internal mismatch into an empty page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/transactions.go` around lines 168 - 183, The
helper fboAccountIDFromCustomerAccounts silently returns "" on unexpected FBO
types which masks resolver contract errors; change
fboAccountIDFromCustomerAccounts to return (string, error) and have
customerFBOAccountID propagate that error (returning a descriptive error when
accounts.FBOAccount is not a *ledgeraccount.CustomerFBOAccount), then update
callers such as ListCreditTransactions (and any use of customerFBOAccountID) to
handle the error instead of falling back to emptyCreditTransactions; reference
functions: customerFBOAccountID, fboAccountIDFromCustomerAccounts,
ListCreditTransactions, emptyCreditTransactions, and
ledgeraccount.CustomerFBOAccount.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/v3/handlers/customers/credits/convert.go (1)
369-377:⚠️ Potential issue | 🟠 MajorDon’t coerce unknown ledger transaction types to
adjustedLine 375/Line 376 currently maps any unknown/future value to
adjusted, which can silently mislabel data. Please handleadjustedexplicitly and keep unknown values detectable (e.g., error/sentinel).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/convert.go` around lines 369 - 377, The switch in toAPIBillingCreditTransactionType currently falls through unknown values to BillingCreditTransactionTypeAdjusted; instead add an explicit case for customerbalance.CreditTransactionTypeAdjusted returning api.BillingCreditTransactionTypeAdjusted, and change the default branch so unknown/future customerbalance.CreditTransactionType values are detectable (for example return a new sentinel api.BillingCreditTransactionTypeUnknown or otherwise surface an error) rather than coercing them to Adjusted; update the api enum (BillingCreditTransactionType) to include the Unknown sentinel if needed and adjust callers to handle it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 312-327: The helper fromAPIBillingCreditTransactionType currently
returns nil for unknown api.BillingCreditTransactionType values which silently
drops the filter; change its signature to return
(*customerbalance.CreditTransactionType, error), map known cases to a pointer to
the corresponding customerbalance.CreditTransactionType, and on the default
branch return a descriptive error (e.g., fmt.Errorf("unknown
BillingCreditTransactionType: %v", *filter)). Update all callers to handle the
error and propagate/return a 400 validation error to the client when the
transaction-type is invalid.
---
Duplicate comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 369-377: The switch in toAPIBillingCreditTransactionType currently
falls through unknown values to BillingCreditTransactionTypeAdjusted; instead
add an explicit case for customerbalance.CreditTransactionTypeAdjusted returning
api.BillingCreditTransactionTypeAdjusted, and change the default branch so
unknown/future customerbalance.CreditTransactionType values are detectable (for
example return a new sentinel api.BillingCreditTransactionTypeUnknown or
otherwise surface an error) rather than coercing them to Adjusted; update the
api enum (BillingCreditTransactionType) to include the Unknown sentinel if
needed and adjust callers to handle it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26905041-5465-4917-868a-d518f56170ba
📒 Files selected for processing (1)
api/v3/handlers/customers/credits/convert.go
Comment on lines
+312
to
+327
| func fromAPIBillingCreditTransactionType(filter *api.BillingCreditTransactionType) *customerbalance.CreditTransactionType { | ||
| if filter == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var txType customerbalance.CreditTransactionType | ||
| switch *filter { | ||
| case api.BillingCreditTransactionTypeFunded: | ||
| txType = customerbalance.CreditTransactionTypeFunded | ||
| case api.BillingCreditTransactionTypeConsumed: | ||
| txType = customerbalance.CreditTransactionTypeConsumed | ||
| case api.BillingCreditTransactionTypeAdjusted: | ||
| txType = customerbalance.CreditTransactionTypeAdjusted | ||
| default: | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Invalid transaction-type filters currently get silently dropped
On Line 326, unknown values return nil, which effectively removes the filter instead of rejecting bad input. That can broaden results unexpectedly for clients.
💡 Suggested fix
-func fromAPIBillingCreditTransactionType(filter *api.BillingCreditTransactionType) *customerbalance.CreditTransactionType {
+func fromAPIBillingCreditTransactionType(filter *api.BillingCreditTransactionType) (*customerbalance.CreditTransactionType, error) {
if filter == nil {
- return nil
+ return nil, nil
}
var txType customerbalance.CreditTransactionType
switch *filter {
case api.BillingCreditTransactionTypeFunded:
txType = customerbalance.CreditTransactionTypeFunded
case api.BillingCreditTransactionTypeConsumed:
txType = customerbalance.CreditTransactionTypeConsumed
case api.BillingCreditTransactionTypeAdjusted:
txType = customerbalance.CreditTransactionTypeAdjusted
default:
- return nil
+ return nil, models.NewGenericValidationError(fmt.Errorf("invalid transaction type: %s", *filter))
}
- return &txType
+ return &txType, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/handlers/customers/credits/convert.go` around lines 312 - 327, The
helper fromAPIBillingCreditTransactionType currently returns nil for unknown
api.BillingCreditTransactionType values which silently drops the filter; change
its signature to return (*customerbalance.CreditTransactionType, error), map
known cases to a pointer to the corresponding
customerbalance.CreditTransactionType, and on the default branch return a
descriptive error (e.g., fmt.Errorf("unknown BillingCreditTransactionType: %v",
*filter)). Update all callers to handle the error and propagate/return a 400
validation error to the client when the transaction-type is invalid.
turip
previously approved these changes
Apr 16, 2026
GAlexIHU
force-pushed
the
feat/transaction-listing
branch
from
9d7bc73 to
a972405
Compare
turip
approved these changes
Apr 16, 2026
This was referenced Apr 20, 2026
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
2 participants
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.
Overview
Notes for reviewer
Summary by CodeRabbit
New Features
Chores