feat(api): better transaction list API#4209
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR migrates credit transaction listing from page-based to cursor-based pagination. It removes the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as ListTransactions Handler
participant Service as Customer Balance Service
participant Loaders as Transaction Loaders<br/>(Funded + Consumed)
participant Merge as Merge Engine
participant CursorCodec as Cursor Codec
Client->>Handler: List transactions (after cursor, limit)
Handler->>CursorCodec: Decode opaque cursor token
CursorCodec->>Handler: TransactionCursor
Handler->>Service: ListCreditTransactions(after, before, limit)
Service->>Loaders: Load funded transactions
Loaders->>Service: [CreditTransaction...]
Service->>Loaders: Load consumed transactions
Loaders->>Service: [CreditTransaction...]
Service->>Merge: Merge sorted lists (cursor ordering)
Merge->>Service: Merged items + hasMore flag
Service->>Handler: ListCreditTransactionsResult<br/>(items, nextCursor)
Handler->>CursorCodec: Encode nextCursor to token
CursorCodec->>Handler: Opaque cursor string
Handler->>Client: CreditTransactionPaginatedResponse<br/>(data, meta with cursor)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
refactor/transaction-list-api
branch
from
4e859b0 to
99ff004
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/v3/handlers/customers/credits/list_transactions.go (1)
37-51:⚠️ Potential issue | 🟠 MajorKeep a runtime max for
page.size.Right now
?page[size]=1000000passes validation and goes straight into the balance/ledger loaders. Please preserve the shared cursor pagination max at runtime, not just in the schema.As per coding guidelines, “Performance should be a priority in critical code paths. Anything related to ... database operations ... should be vetted for potential performance bottlenecks.”
🤖 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 37 - 51, The handler currently only validates page.size > 0 but allows arbitrarily large values; enforce a runtime maximum by comparing the computed size (the local variable size derived from args.Params.Page.Size) against the shared pagination max (introduce or use an existing constant like sharedCursorPaginationMax or MAX_PAGE_SIZE) and if it exceeds the max return the same apierrors.NewBadRequestError pattern with Field "page.size" and Reason "must be <= {max}". Update the logic in the function that builds the ListCreditTransactionsRequest so size is clamped or rejected before it is used by the balance/ledger loaders (reference the local size variable, args.Params.Page.Size, and the ListCreditTransactionsRequest creation point).openmeter/ledger/historical/adapter/ledger.go (1)
261-304:⚠️ Potential issue | 🟠 MajorApply account and currency filters to the same entry.
Tiny gotcha here: the two transaction-level
HasEntriesWithclauses can be satisfied by different entries. For example, an account-scoped USD entry and a separate EUR entry could match anaccount + EURquery even though no single entry matches both. SinceWithEntriesalready uses the combined predicate, the top-level query should use that same combined predicate too.Proposed fix
entryPredicates := listTransactionsEntryPredicates(input.AccountIDs, input.Currency) query := r.db.LedgerTransaction.Query(). Where(ledgertransactiondb.Namespace(input.Namespace)). @@ }) @@ - // Scope to specific accounts. - 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)), - ), - ), - ), - ) + // Scope to entries matching all requested entry-level filters. + if len(entryPredicates) > 0 { + query = query.Where(ledgertransactiondb.HasEntriesWith(entryPredicates...)) }🤖 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 261 - 304, The transaction-level filters use two separate HasEntriesWith calls so account and currency predicates can match different entries; instead use the same combined entry predicate slice produced by listTransactionsEntryPredicates (entryPredicates) when scoping transactions so both account and currency must apply to the same entry. Concretely, replace the separate ledgertransactiondb.HasEntriesWith(...) blocks with a single ledgertransactiondb.HasEntriesWith(...) that applies entryPredicates (or wraps them into the ledgerentrydb predicate form expected) so the top-level query uses the exact same entry-level predicates as the WithEntries call.
🧹 Nitpick comments (7)
openmeter/billing/charges/service/funded_credit_activity.go (1)
10-18: Optional:transaction.Runfor a read-only list may be overkill.
ListFundedCreditActivitiesis a pure read — wrapping it intransaction.Runopens (or joins) a tx, issues aSAVEPOINT, and thenCOMMITs, which is unnecessary bookkeeping for a single SELECT. If a tx is already on the context it'll just nest, so no harm, but when called outside one (e.g., from the funded loader) it spins one up just for this read.If the intent is purely to join an existing tx when present, consider calling the adapter directly and relying on
entutils.TransactingRepoin the adapter to pick up the driver from context — that's the common pattern for read paths. Happy to keep it as-is for consistency if that's the preference though.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/funded_credit_activity.go` around lines 10 - 18, The call to transaction.Run in ListFundedCreditActivities causes an unnecessary transaction for a read-only list; change the implementation to validate the input then call s.adapter.ListFundedCreditActivities(ctx, input) directly (removing transaction.Run) so the adapter/entutils.TransactingRepo can pick up any existing transaction from context without starting a new one; keep the input.Validate() check and return signature unchanged, and ensure references to ListFundedCreditActivities and transaction.Run are updated accordingly.AGENTS.md (1)
92-92: Tiny wording nit — consider generalizing to "HTTP decorators"The note is helpful, but the same import-and-
usingrequirement applies to any@typespec/httpdecorator (e.g.,@route,@get,@header,@path), not just@query. A small rewording would keep this durable guidance as other decorators get introduced:📝 Suggested tweak
-When adding query decorators (for example `@query`) to a TypeSpec file that does not already use HTTP decorators, import `@typespec/http` and add `using TypeSpec.Http;` in that file; otherwise compilation fails with `Unknown decorator `@query``. +When adding HTTP decorators (for example `@query`, `@route`, `@header`) to a TypeSpec file that does not already use any, import `@typespec/http` and add `using TypeSpec.Http;` in that file; otherwise compilation fails with `Unknown decorator @<name>`.As per coding guidelines: "Treat AGENTS.md as long-lived project guidance; prefer durable wording... keep entries actionable and specific".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 92, Update the wording to generalize the guidance from just `@query` to all HTTP decorators: state that when adding any `@typespec/http` decorator (e.g., `@query`, `@route`, `@get`, `@header`, `@path`) to a TypeSpec file that does not already use HTTP decorators you must import `@typespec/http` and add `using TypeSpec.Http;` in that file to avoid the `Unknown decorator` compilation error; replace the current sentence referencing only `@query` with this generalized wording.api/v3/handlers/customers/credits/list_transactions_test.go (1)
64-92: Add a small backward-pagination coverage case.Nice round-trip coverage here. Since this PR changes cursor semantics, it’d be great to also cover
page.beforebehavior and the “bothafterandbefore” case so cursor-direction regressions are caught early.As per coding guidelines,
**/*_test.go: “Make sure the tests are comprehensive and cover the changes.”🤖 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_test.go` around lines 64 - 92, Add two unit tests to cover backward-pagination and the invalid "both after and before" case: create a test (e.g., TestCreditTransactionCursorConversion_Before) that builds a ledger.TransactionCursor, uses encodeBillingCreditTransactionCursor, then decodes it via decodeBillingCreditTransactionCursor while simulating a page.before usage and asserts BookedAt, CreatedAt and ID namespace/ID match; and add a test (e.g., TestCreditTransactionCursorConversion_BothAfterAndBefore) that passes both an encoded "after" cursor and a "before" cursor together (or otherwise simulates both parameters) and asserts decode/handler returns an error to catch the illegal both-directions case. Ensure you reference the existing encodeBillingCreditTransactionCursor and decodeBillingCreditTransactionCursor helpers and mirror the style of TestCreditTransactionCursorConversion.openmeter/billing/charges/adapter/search_test.go (1)
337-337: Use the test-scoped context in the new tests.These new suite tests can use
s.T().Context()instead ofcontext.Background()so cancellation follows the test lifecycle.Suggested cleanup
- ctx := context.Background() + ctx := s.T().Context()As per coding guidelines, “In tests, prefer
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background()to keep cancellation and test-scoped lifecycle tied to the test harness”.Also applies to: 396-396, 498-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/adapter/search_test.go` at line 337, Replace test-scoped use of context.Background() with the suite's test context so cancellation follows the test lifecycle: locate the tests in search_test.go that set ctx := context.Background() (the occurrences in the suite methods using receiver s) and change them to use s.T().Context() instead (i.e., obtain context from s.T()). Ensure every occurrence in the file (including the ones noted around lines 337, 396, 498) is updated so subsequent test operations use the test-scoped ctx variable.openmeter/ledger/primitives.go (3)
254-261: Nice use of the zero-value sentinel — consider a tiny helper for symmetry.
ListTransactionsCreditMovementUnspecifiedbeing the iota-zero value means forgetting to setCreditMovementsilently validates as "no filter", which is fine and matches the loader's explicit-pass pattern. Purely optional: a smallfunc (m ListTransactionsCreditMovement) Validate() error(mirroringTransactionCursor.Validateandcurrencyx.Code.Validate) would keepListTransactionsInput.Validate()uniform and make the enum self-checking if it's reused elsewhere later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/primitives.go` around lines 254 - 261, Add a Validate method on the ListTransactionsCreditMovement type (e.g. func (m ListTransactionsCreditMovement) Validate() error) that mirrors the pattern used by TransactionCursor.Validate and currencyx.Code.Validate: return nil for ListTransactionsCreditMovementUnspecified, ListTransactionsCreditMovementPositive, and ListTransactionsCreditMovementNegative and return ErrListTransactionsInputInvalid.WithAttrs(...) for other values; then replace the switch in ListTransactionsInput.Validate() with a call to i.CreditMovement.Validate() to keep validation symmetry and make the enum self-validating for future reuse.
171-175: Minor: the TODO aboveListTransactionsis partially stale.The concern about
wall_clockvsbooked_atis now largely addressed byTransactionCursorbeing a composite of(BookedAt, CreatedAt, ID)with a deterministicCompare. If the API is considered settled after this PR, it'd be nice to either delete the TODO or narrow it to whatever is still open (e.g. ordering guarantees across sources) so future readers don't chase a ghost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/primitives.go` around lines 171 - 175, The TODO above the ListTransactions method is outdated because TransactionCursor (composed of BookedAt, CreatedAt, ID) with its Compare provides deterministic ordering; remove or update that TODO: either delete the comment entirely or replace it with a concise note about any remaining open ordering concerns (for example: "Note: ordering across heterogeneous sources may still require additional guarantees" or similar). Update the comment near ListTransactions and ensure any mention references TransactionCursor and its Compare so future readers understand why the original wall_clock vs booked_at concern was resolved.
179-232: Consider renamingCursor→Afterfor symmetry withBefore.The new field is semantically an "after" bound (the caller in
openmeter/ledger/customerbalance/ledger_loader.goeven assignsCursor: input.After, and your own validation error literal says"after_before_both_set"). Keeping the field calledCursorwhile its sibling isBeforemakes the API a bit harder to read and invites mis-pairing at call sites. A straight rename would make the pair self-documenting and align the struct with the error messages you already picked.♻️ Proposed rename
type ListTransactionsInput struct { Namespace string - Cursor *TransactionCursor + After *TransactionCursor Before *TransactionCursor Limit int @@ - if i.Cursor != nil { - if err := i.Cursor.Validate(); err != nil { + if i.After != nil { + if err := i.After.Validate(); err != nil { return ErrListTransactionsInputInvalid.WithAttrs(models.Attributes{ - "reason": "cursor_invalid", - "cursor": i.Cursor, - "error": err, + "reason": "after_invalid", + "after": i.After, + "error": err, }) } } @@ - if i.Cursor != nil && i.Before != nil { + if i.After != nil && i.Before != nil { return ErrListTransactionsInputInvalid.WithAttrs(models.Attributes{ "reason": "after_before_both_set", }) }Don't forget to update the
ledger_loader.gocall site (and any other callers) at the same time.As per coding guidelines: "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/primitives.go` around lines 179 - 232, The ListTransactionsInput field named Cursor is semantically an "after" bound and should be renamed to After for symmetry with Before; update the struct field in ListTransactionsInput (replace Cursor *TransactionCursor with After *TransactionCursor), adjust all references in the Validate() method (checks, calls to Cursor.Validate(), and the mutual-exclusion check that uses "after_before_both_set"), and update every call site (e.g., customerbalance/ledger_loader.go where Caller: input.After was previously assigned to Cursor) to use After; ensure tests and any JSON/tags or marshaling code are updated accordingly to preserve compatibility.
🤖 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/spec/packages/aip/src/customers/credits/ledger.tsp`:
- Around line 24-27: Update the documentation note in ledger.tsp to remove
references to `first` and `last` since the API only exposes `next` and
`previous`; edit the comment block (the note starting "We intentionally return
opaque cursor tokens...") to state that only `next` and `previous` opaque cursor
tokens are returned and avoid implying `first`/`last` exist, keeping the
rationale about opaque tokens and proxy/gateway coupling intact.
In `@api/v3/api.gen.go`:
- Around line 2176-2179: The generated comment still references response fields
`first`/`last` even though the model only exposes `next`, `previous`, and
`size`; fix this by updating the TypeSpec/OpenAPI source to remove
`first`/`last` from the response documentation (and ensure the spec describes
`next`, `previous`, `size` only), then regenerate the OpenAPI and client
artifacts using the project's API generation target (make gen-api); do not edit
the generated file directly—change the TypeSpec source and re-run generation so
the comment in the generated code is corrected.
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 260-278: The cursor encoding/decoding uses base64.StdEncoding
which can produce '+' characters that break query params; update
encodeBillingCreditTransactionCursor and decodeBillingCreditTransactionCursor to
use URL-safe base64 without padding by replacing
base64.StdEncoding.EncodeToString/DecodeString with
base64.RawURLEncoding.EncodeToString and base64.RawURLEncoding.DecodeString
respectively so generated cursors use '-' and '_' and are safe in URL query
parameters.
In `@api/v3/handlers/customers/credits/list_transactions.go`:
- Around line 61-90: Check for the ambiguous case where both
args.Params.Page.After and args.Params.Page.Before are provided before decoding:
inside the handler that builds the ListCreditTransactionsRequest (where req is
populated and decodeBillingCreditTransactionCursor is used), if both After and
Before are non-nil immediately return a 400 using
apierrors.NewBadRequestError(ctx, errors.New("cannot set both page.after and
page.before"), apierrors.InvalidParameters{{Field: "page", Reason: "cannot set
both page.after and page.before", Source: apierrors.InvalidParamSourceQuery}})
so we reject requests that supply both cursors and avoid setting req.After and
req.Before together.
In `@openmeter/billing/charges/adapter/search.go`:
- Around line 171-214: The code currently only sets nextCursor when hasMore is
true, which loses the forward cursor for requests using input.Before; change the
nextCursor creation condition so a next cursor is produced whenever items were
returned and either hasMore is true OR input.Before is set (i.e., replace "if
hasMore && len(items) > 0" with "if len(items) > 0 && (hasMore || input.Before
!= nil)"), keeping the construction of charges.FundedCreditActivityCursor the
same and leaving the slices.Reverse(items) and hasPrevious logic unchanged
unless you want to tighten it further.
In `@openmeter/ledger/customerbalance/funded_loader.go`:
- Around line 20-54: The loader currently always sets HasMore using
result.NextCursor which is wrong for backward pagination; in
fundedCreditTransactionLoader.Load check whether input.Before is non-nil and, if
so, set creditTransactionLoaderResult.HasMore = result.PrevCursor != nil,
otherwise set HasMore = result.NextCursor != nil; update the return to use this
direction-aware logic (referencing fundedCreditTransactionLoader.Load,
creditTransactionLoaderInput.input.Before,
creditTransactionLoaderResult.HasMore, result.NextCursor and result.PrevCursor).
In `@openmeter/ledger/customerbalance/ledger_loader.go`:
- Around line 21-43: The HasMore value returned by
ledgerCreditTransactionLoader.Load is not direction-aware: change how
creditTransactionLoaderResult.HasMore is computed in Load
(ledgerCreditTransactionLoader.Load) to reflect the requested pagination
direction by inspecting the input (input.Before vs input.After); when a backward
(before) request is made use result.PrevCursor != nil for HasMore, otherwise use
result.NextCursor != nil for forward pagination, so the loader's HasMore matches
transactions.go expectations.
In `@openmeter/ledger/customerbalance/transactions.go`:
- Around line 164-170: The code assumes a single running balance by using
items[0].Currency and applying it to all credit transactions, which is incorrect
when input.Currency is nil and items contain multiple currencies; update the
logic to handle balances per currency: when input.Currency is nil, group items
by their Currency, for each currency compute runningBalance by calling
s.GetBalance(ctx, input.CustomerID, currency,
lo.ToPtr(creditTransactionCursor(firstItemInGroup))) and then call
applyCreditTransactionBalances on that group's slice using the corresponding
runningBalance.Settled(); apply the same per-currency grouping/fix to the other
occurrence that uses applyCreditTransactionBalances (the block around the second
instance mentioned).
---
Outside diff comments:
In `@api/v3/handlers/customers/credits/list_transactions.go`:
- Around line 37-51: The handler currently only validates page.size > 0 but
allows arbitrarily large values; enforce a runtime maximum by comparing the
computed size (the local variable size derived from args.Params.Page.Size)
against the shared pagination max (introduce or use an existing constant like
sharedCursorPaginationMax or MAX_PAGE_SIZE) and if it exceeds the max return the
same apierrors.NewBadRequestError pattern with Field "page.size" and Reason
"must be <= {max}". Update the logic in the function that builds the
ListCreditTransactionsRequest so size is clamped or rejected before it is used
by the balance/ledger loaders (reference the local size variable,
args.Params.Page.Size, and the ListCreditTransactionsRequest creation point).
In `@openmeter/ledger/historical/adapter/ledger.go`:
- Around line 261-304: The transaction-level filters use two separate
HasEntriesWith calls so account and currency predicates can match different
entries; instead use the same combined entry predicate slice produced by
listTransactionsEntryPredicates (entryPredicates) when scoping transactions so
both account and currency must apply to the same entry. Concretely, replace the
separate ledgertransactiondb.HasEntriesWith(...) blocks with a single
ledgertransactiondb.HasEntriesWith(...) that applies entryPredicates (or wraps
them into the ledgerentrydb predicate form expected) so the top-level query uses
the exact same entry-level predicates as the WithEntries call.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 92: Update the wording to generalize the guidance from just `@query` to
all HTTP decorators: state that when adding any `@typespec/http` decorator
(e.g., `@query`, `@route`, `@get`, `@header`, `@path`) to a TypeSpec file that
does not already use HTTP decorators you must import `@typespec/http` and add
`using TypeSpec.Http;` in that file to avoid the `Unknown decorator` compilation
error; replace the current sentence referencing only `@query` with this
generalized wording.
In `@api/v3/handlers/customers/credits/list_transactions_test.go`:
- Around line 64-92: Add two unit tests to cover backward-pagination and the
invalid "both after and before" case: create a test (e.g.,
TestCreditTransactionCursorConversion_Before) that builds a
ledger.TransactionCursor, uses encodeBillingCreditTransactionCursor, then
decodes it via decodeBillingCreditTransactionCursor while simulating a
page.before usage and asserts BookedAt, CreatedAt and ID namespace/ID match; and
add a test (e.g., TestCreditTransactionCursorConversion_BothAfterAndBefore) that
passes both an encoded "after" cursor and a "before" cursor together (or
otherwise simulates both parameters) and asserts decode/handler returns an error
to catch the illegal both-directions case. Ensure you reference the existing
encodeBillingCreditTransactionCursor and decodeBillingCreditTransactionCursor
helpers and mirror the style of TestCreditTransactionCursorConversion.
In `@openmeter/billing/charges/adapter/search_test.go`:
- Line 337: Replace test-scoped use of context.Background() with the suite's
test context so cancellation follows the test lifecycle: locate the tests in
search_test.go that set ctx := context.Background() (the occurrences in the
suite methods using receiver s) and change them to use s.T().Context() instead
(i.e., obtain context from s.T()). Ensure every occurrence in the file
(including the ones noted around lines 337, 396, 498) is updated so subsequent
test operations use the test-scoped ctx variable.
In `@openmeter/billing/charges/service/funded_credit_activity.go`:
- Around line 10-18: The call to transaction.Run in ListFundedCreditActivities
causes an unnecessary transaction for a read-only list; change the
implementation to validate the input then call
s.adapter.ListFundedCreditActivities(ctx, input) directly (removing
transaction.Run) so the adapter/entutils.TransactingRepo can pick up any
existing transaction from context without starting a new one; keep the
input.Validate() check and return signature unchanged, and ensure references to
ListFundedCreditActivities and transaction.Run are updated accordingly.
In `@openmeter/ledger/primitives.go`:
- Around line 254-261: Add a Validate method on the
ListTransactionsCreditMovement type (e.g. func (m
ListTransactionsCreditMovement) Validate() error) that mirrors the pattern used
by TransactionCursor.Validate and currencyx.Code.Validate: return nil for
ListTransactionsCreditMovementUnspecified,
ListTransactionsCreditMovementPositive, and
ListTransactionsCreditMovementNegative and return
ErrListTransactionsInputInvalid.WithAttrs(...) for other values; then replace
the switch in ListTransactionsInput.Validate() with a call to
i.CreditMovement.Validate() to keep validation symmetry and make the enum
self-validating for future reuse.
- Around line 171-175: The TODO above the ListTransactions method is outdated
because TransactionCursor (composed of BookedAt, CreatedAt, ID) with its Compare
provides deterministic ordering; remove or update that TODO: either delete the
comment entirely or replace it with a concise note about any remaining open
ordering concerns (for example: "Note: ordering across heterogeneous sources may
still require additional guarantees" or similar). Update the comment near
ListTransactions and ensure any mention references TransactionCursor and its
Compare so future readers understand why the original wall_clock vs booked_at
concern was resolved.
- Around line 179-232: The ListTransactionsInput field named Cursor is
semantically an "after" bound and should be renamed to After for symmetry with
Before; update the struct field in ListTransactionsInput (replace Cursor
*TransactionCursor with After *TransactionCursor), adjust all references in the
Validate() method (checks, calls to Cursor.Validate(), and the mutual-exclusion
check that uses "after_before_both_set"), and update every call site (e.g.,
customerbalance/ledger_loader.go where Caller: input.After was previously
assigned to Cursor) to use After; ensure tests and any JSON/tags or marshaling
code are updated accordingly to preserve compatibility.
🪄 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: 8cd05479-df7b-402a-867d-d997b1819be3
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (28)
AGENTS.mdapi/spec/packages/aip/src/customers/credits/ledger.tspapi/spec/packages/aip/src/customers/credits/operations.tspapi/v3/api.gen.goapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/handlers/customers/credits/list_transactions_test.goopenmeter/billing/charges/adapter.goopenmeter/billing/charges/adapter/search.goopenmeter/billing/charges/adapter/search_test.goopenmeter/billing/charges/funded_credit_activity.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/funded_credit_activity.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/ledger_loader.goopenmeter/ledger/customerbalance/loaders.goopenmeter/ledger/customerbalance/merge.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/customerbalance/transactions_test.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/repo.goopenmeter/ledger/noop/noop.goopenmeter/ledger/primitives.go
Comment on lines
+24
to
+27
| * NOTE: We intentionally return opaque cursor tokens (not URI links) in | ||
| * `first`, `last`, `next`, and `previous`. This matches existing OpenMeter | ||
| * cursor implementations and avoids coupling response construction to | ||
| * externally visible URL shape behind proxies/gateways. |
Contributor
There was a problem hiding this comment.
Drop the stale first/last wording.
The model only exposes next and previous, and the PR intentionally removes first/last, so this note would make the generated docs confusing.
Suggested wording
- * `first`, `last`, `next`, and `previous`. This matches existing OpenMeter
+ * `next` and `previous`. This matches existing OpenMeterAs per coding guidelines, “The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * NOTE: We intentionally return opaque cursor tokens (not URI links) in | |
| * `first`, `last`, `next`, and `previous`. This matches existing OpenMeter | |
| * cursor implementations and avoids coupling response construction to | |
| * externally visible URL shape behind proxies/gateways. | |
| * NOTE: We intentionally return opaque cursor tokens (not URI links) in | |
| * `next` and `previous`. This matches existing OpenMeter | |
| * cursor implementations and avoids coupling response construction to | |
| * externally visible URL shape behind proxies/gateways. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/packages/aip/src/customers/credits/ledger.tsp` around lines 24 - 27,
Update the documentation note in ledger.tsp to remove references to `first` and
`last` since the API only exposes `next` and `previous`; edit the comment block
(the note starting "We intentionally return opaque cursor tokens...") to state
that only `next` and `previous` opaque cursor tokens are returned and avoid
implying `first`/`last` exist, keeping the rationale about opaque tokens and
proxy/gateway coupling intact.
Comment on lines
+2176
to
+2179
| // NOTE: We intentionally return opaque cursor tokens (not URI links) in | ||
| // `first`, `last`, `next`, and `previous`. This matches existing OpenMeter | ||
| // cursor implementations and avoids coupling response construction to | ||
| // externally visible URL shape behind proxies/gateways. |
Contributor
There was a problem hiding this comment.
Small docs mismatch: first/last are no longer returned.
This comment still says the response contains first and last, but the generated model only has next, previous, and size. Please update the TypeSpec/OpenAPI source text and regenerate instead of editing this generated file directly.
Based on learnings: Never manually edit files with // Code generated by X, DO NOT EDIT. headers; edit TypeSpec files in api/spec/ to change the API; regenerate api/openapi.yaml, api/openapi.cloud.yaml, and API client code using make gen-api after editing TypeSpec files in api/spec/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/api.gen.go` around lines 2176 - 2179, The generated comment still
references response fields `first`/`last` even though the model only exposes
`next`, `previous`, and `size`; fix this by updating the TypeSpec/OpenAPI source
to remove `first`/`last` from the response documentation (and ensure the spec
describes `next`, `previous`, `size` only), then regenerate the OpenAPI and
client artifacts using the project's API generation target (make gen-api); do
not edit the generated file directly—change the TypeSpec source and re-run
generation so the comment in the generated code is corrected.
Comment on lines
+260
to
+278
| func encodeBillingCreditTransactionCursor(cursor ledger.TransactionCursor) (string, error) { | ||
| payload := billingCreditTransactionCursorPayload{ | ||
| BookedAt: cursor.BookedAt, | ||
| CreatedAt: cursor.CreatedAt, | ||
| ID: cursor.ID.ID, | ||
| } | ||
|
|
||
| raw, err := json.Marshal(payload) | ||
| if err != nil { | ||
| return "", fmt.Errorf("encode cursor payload: %w", err) | ||
| } | ||
|
|
||
| return base64.StdEncoding.EncodeToString(raw), nil | ||
| } | ||
|
|
||
| func decodeBillingCreditTransactionCursor(cursor string, namespace string) (*ledger.TransactionCursor, error) { | ||
| raw, err := base64.StdEncoding.DecodeString(cursor) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode cursor: %w", err) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from urllib.parse import parse_qs
token = "abc+def=="
parsed = parse_qs("after=" + token)["after"][0]
print("raw token: ", token)
print("parsed token:", parsed)
print("unchanged: ", parsed == token)
PYRepository: openmeterio/openmeter
Length of output: 132
🏁 Script executed:
rg "encodeBillingCreditTransactionCursor|decodeBillingCreditTransactionCursor" --type go -A 5 -B 5Repository: openmeterio/openmeter
Length of output: 7124
🏁 Script executed:
rg "after|before" api/v3/handlers/customers/credits/convert.go --type go -C 3Repository: openmeterio/openmeter
Length of output: 305
Switch to URL-safe base64 for cursor tokens.
These cursors get passed as page.after and page.before query parameters, and base64.StdEncoding can emit + characters. When clients paste the cursor raw into a query string, that + gets decoded as a space by URL parsers, breaking pagination. base64.RawURLEncoding sidesteps this by using - and _ instead.
Suggested fix
- return base64.StdEncoding.EncodeToString(raw), nil
+ return base64.RawURLEncoding.EncodeToString(raw), nil
}
func decodeBillingCreditTransactionCursor(cursor string, namespace string) (*ledger.TransactionCursor, error) {
- raw, err := base64.StdEncoding.DecodeString(cursor)
+ raw, err := base64.RawURLEncoding.DecodeString(cursor)
if err != nil {
return nil, fmt.Errorf("decode cursor: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func encodeBillingCreditTransactionCursor(cursor ledger.TransactionCursor) (string, error) { | |
| payload := billingCreditTransactionCursorPayload{ | |
| BookedAt: cursor.BookedAt, | |
| CreatedAt: cursor.CreatedAt, | |
| ID: cursor.ID.ID, | |
| } | |
| raw, err := json.Marshal(payload) | |
| if err != nil { | |
| return "", fmt.Errorf("encode cursor payload: %w", err) | |
| } | |
| return base64.StdEncoding.EncodeToString(raw), nil | |
| } | |
| func decodeBillingCreditTransactionCursor(cursor string, namespace string) (*ledger.TransactionCursor, error) { | |
| raw, err := base64.StdEncoding.DecodeString(cursor) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode cursor: %w", err) | |
| func encodeBillingCreditTransactionCursor(cursor ledger.TransactionCursor) (string, error) { | |
| payload := billingCreditTransactionCursorPayload{ | |
| BookedAt: cursor.BookedAt, | |
| CreatedAt: cursor.CreatedAt, | |
| ID: cursor.ID.ID, | |
| } | |
| raw, err := json.Marshal(payload) | |
| if err != nil { | |
| return "", fmt.Errorf("encode cursor payload: %w", err) | |
| } | |
| return base64.RawURLEncoding.EncodeToString(raw), nil | |
| } | |
| func decodeBillingCreditTransactionCursor(cursor string, namespace string) (*ledger.TransactionCursor, error) { | |
| raw, err := base64.RawURLEncoding.DecodeString(cursor) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode cursor: %w", err) |
🤖 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 260 - 278, The
cursor encoding/decoding uses base64.StdEncoding which can produce '+'
characters that break query params; update encodeBillingCreditTransactionCursor
and decodeBillingCreditTransactionCursor to use URL-safe base64 without padding
by replacing base64.StdEncoding.EncodeToString/DecodeString with
base64.RawURLEncoding.EncodeToString and base64.RawURLEncoding.DecodeString
respectively so generated cursors use '-' and '_' and are safe in URL query
parameters.
Comment on lines
+61
to
+90
| if args.Params.Page != nil { | ||
| if args.Params.Page.After != nil { | ||
| after, err := decodeBillingCreditTransactionCursor(*args.Params.Page.After, ns) | ||
| if err != nil { | ||
| return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ | ||
| { | ||
| Field: "page.after", | ||
| Reason: err.Error(), | ||
| Source: apierrors.InvalidParamSourceQuery, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| req.After = after | ||
| } | ||
|
|
||
| if args.Params.Page.Before != nil { | ||
| before, err := decodeBillingCreditTransactionCursor(*args.Params.Page.Before, ns) | ||
| if err != nil { | ||
| return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ | ||
| { | ||
| Field: "page.before", | ||
| Reason: err.Error(), | ||
| Source: apierrors.InvalidParamSourceQuery, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| req.Before = before | ||
| } |
Contributor
There was a problem hiding this comment.
Reject requests that send both page.after and page.before.
This accepts both cursors and sets both on the service input, which makes the requested pagination direction ambiguous. A clear 400 keeps cursor metadata predictable.
🐛 Proposed validation
if args.Params.Page != nil {
+ if args.Params.Page.After != nil && args.Params.Page.Before != nil {
+ err := fmt.Errorf("page.after and page.before cannot both be set")
+ return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{
+ {
+ Field: "page",
+ Reason: "page.after and page.before cannot both be set",
+ Source: apierrors.InvalidParamSourceQuery,
+ },
+ })
+ }
+
if args.Params.Page.After != nil {
after, err := decodeBillingCreditTransactionCursor(*args.Params.Page.After, ns)
if err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if args.Params.Page != nil { | |
| if args.Params.Page.After != nil { | |
| after, err := decodeBillingCreditTransactionCursor(*args.Params.Page.After, ns) | |
| if err != nil { | |
| return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ | |
| { | |
| Field: "page.after", | |
| Reason: err.Error(), | |
| Source: apierrors.InvalidParamSourceQuery, | |
| }, | |
| }) | |
| } | |
| req.After = after | |
| } | |
| if args.Params.Page.Before != nil { | |
| before, err := decodeBillingCreditTransactionCursor(*args.Params.Page.Before, ns) | |
| if err != nil { | |
| return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ | |
| { | |
| Field: "page.before", | |
| Reason: err.Error(), | |
| Source: apierrors.InvalidParamSourceQuery, | |
| }, | |
| }) | |
| } | |
| req.Before = before | |
| } | |
| if args.Params.Page != nil { | |
| if args.Params.Page.After != nil && args.Params.Page.Before != nil { | |
| err := fmt.Errorf("page.after and page.before cannot both be set") | |
| return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ | |
| { | |
| Field: "page", | |
| Reason: "page.after and page.before cannot both be set", | |
| Source: apierrors.InvalidParamSourceQuery, | |
| }, | |
| }) | |
| } | |
| if args.Params.Page.After != nil { | |
| after, err := decodeBillingCreditTransactionCursor(*args.Params.Page.After, ns) | |
| if err != nil { | |
| return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ | |
| { | |
| Field: "page.after", | |
| Reason: err.Error(), | |
| Source: apierrors.InvalidParamSourceQuery, | |
| }, | |
| }) | |
| } | |
| req.After = after | |
| } | |
| if args.Params.Page.Before != nil { | |
| before, err := decodeBillingCreditTransactionCursor(*args.Params.Page.Before, ns) | |
| if err != nil { | |
| return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ | |
| { | |
| Field: "page.before", | |
| Reason: err.Error(), | |
| Source: apierrors.InvalidParamSourceQuery, | |
| }, | |
| }) | |
| } | |
| req.Before = before | |
| } |
🤖 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 61 - 90,
Check for the ambiguous case where both args.Params.Page.After and
args.Params.Page.Before are provided before decoding: inside the handler that
builds the ListCreditTransactionsRequest (where req is populated and
decodeBillingCreditTransactionCursor is used), if both After and Before are
non-nil immediately return a 400 using apierrors.NewBadRequestError(ctx,
errors.New("cannot set both page.after and page.before"),
apierrors.InvalidParameters{{Field: "page", Reason: "cannot set both page.after
and page.before", Source: apierrors.InvalidParamSourceQuery}}) so we reject
requests that supply both cursors and avoid setting req.After and req.Before
together.
Comment on lines
+171
to
+214
| hasMore := len(entities) > input.Limit | ||
| if hasMore { | ||
| entities = entities[:input.Limit] | ||
| } | ||
|
|
||
| items := make([]charges.FundedCreditActivity, 0, len(entities)) | ||
| for _, entity := range entities { | ||
| creditPurchase, err := entity.Edges.CreditPurchaseOrErr() | ||
| if err != nil { | ||
| return charges.ListFundedCreditActivitiesResult{}, fmt.Errorf("credit purchase not loaded for grant %s: %w", entity.ID, err) | ||
| } | ||
|
|
||
| items = append(items, charges.FundedCreditActivity{ | ||
| ChargeID: meta.ChargeID{ | ||
| Namespace: creditPurchase.Namespace, | ||
| ID: creditPurchase.ID, | ||
| }, | ||
| ChargeCreatedAt: creditPurchase.CreatedAt, | ||
| FundedAt: entity.GrantedAt, | ||
| TransactionGroupID: entity.TransactionGroupID, | ||
| Currency: creditPurchase.Currency, | ||
| Amount: creditPurchase.CreditAmount, | ||
| Name: creditPurchase.Name, | ||
| Description: creditPurchase.Description, | ||
| }) | ||
| } | ||
|
|
||
| if input.Before != nil { | ||
| slices.Reverse(items) | ||
| } | ||
|
|
||
| var nextCursor *charges.FundedCreditActivityCursor | ||
| if hasMore && len(items) > 0 { | ||
| next := items[len(items)-1] | ||
| nextCursor = &charges.FundedCreditActivityCursor{ | ||
| FundedAt: next.FundedAt, | ||
| ChargeCreatedAt: next.ChargeCreatedAt, | ||
| ChargeID: next.ChargeID, | ||
| } | ||
| } | ||
|
|
||
| hasPrevious := input.After != nil | ||
| if input.Before != nil { | ||
| hasPrevious = hasMore |
Contributor
There was a problem hiding this comment.
Don’t gate the forward cursor for before pages on hasMore.
For Before requests, hasMore means “more newer rows exist”, but NextCursor is needed to resume toward older rows from the last returned item. A before-page with fewer than Limit + 1 newer rows can still need a next cursor to get back to the older page.
Minimal direction-preserving fix
- if hasMore && len(items) > 0 {
+ if len(items) > 0 && (hasMore || input.Before != nil) {
next := items[len(items)-1]
nextCursor = &charges.FundedCreditActivityCursor{
FundedAt: next.FundedAt,
ChargeCreatedAt: next.ChargeCreatedAt,
ChargeID: next.ChargeID,
}
}If you want exact metadata for arbitrary cursors, an older-row existence check from the returned tail would be even tighter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/adapter/search.go` around lines 171 - 214, The code
currently only sets nextCursor when hasMore is true, which loses the forward
cursor for requests using input.Before; change the nextCursor creation condition
so a next cursor is produced whenever items were returned and either hasMore is
true OR input.Before is set (i.e., replace "if hasMore && len(items) > 0" with
"if len(items) > 0 && (hasMore || input.Before != nil)"), keeping the
construction of charges.FundedCreditActivityCursor the same and leaving the
slices.Reverse(items) and hasPrevious logic unchanged unless you want to tighten
it further.
Comment on lines
+20
to
+54
| func (l *fundedCreditTransactionLoader) Load(ctx context.Context, input creditTransactionLoaderInput) (creditTransactionLoaderResult, error) { | ||
| result, err := l.service.ChargesService.ListFundedCreditActivities(ctx, charges.ListFundedCreditActivitiesInput{ | ||
| Customer: input.CustomerID, | ||
| Limit: input.Limit, | ||
| After: toFundedCreditActivityCursor(input.After), | ||
| Before: toFundedCreditActivityCursor(input.Before), | ||
| Currency: input.Currency, | ||
| }) | ||
| if err != nil { | ||
| return creditTransactionLoaderResult{}, err | ||
| } | ||
|
|
||
| items := make([]CreditTransaction, 0, len(result.Items)) | ||
| for _, activity := range result.Items { | ||
| annotations := models.Annotations{ | ||
| ledger.AnnotationChargeID: activity.ChargeID.ID, | ||
| } | ||
|
|
||
| items = append(items, CreditTransaction{ | ||
| ID: models.NamespacedID(activity.ChargeID), | ||
| CreatedAt: activity.ChargeCreatedAt, | ||
| BookedAt: activity.FundedAt, | ||
| Type: CreditTransactionTypeFunded, | ||
| Currency: activity.Currency, | ||
| Amount: activity.Amount, | ||
| Name: activity.Name, | ||
| Description: activity.Description, | ||
| Annotations: annotations, | ||
| }) | ||
| } | ||
|
|
||
| return creditTransactionLoaderResult{ | ||
| Items: items, | ||
| HasMore: result.NextCursor != nil, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Use the backward cursor when loading with before.
For before pagination, HasMore needs to reflect whether more funded activities exist in the backward direction. Using result.NextCursor only can make the merged response drop previous even when older records exist.
🐛 Proposed direction-aware fix
- return creditTransactionLoaderResult{
+ hasMore := result.NextCursor != nil
+ if input.Before != nil {
+ hasMore = result.PreviousCursor != nil
+ }
+
+ return creditTransactionLoaderResult{
Items: items,
- HasMore: result.NextCursor != nil,
+ HasMore: hasMore,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (l *fundedCreditTransactionLoader) Load(ctx context.Context, input creditTransactionLoaderInput) (creditTransactionLoaderResult, error) { | |
| result, err := l.service.ChargesService.ListFundedCreditActivities(ctx, charges.ListFundedCreditActivitiesInput{ | |
| Customer: input.CustomerID, | |
| Limit: input.Limit, | |
| After: toFundedCreditActivityCursor(input.After), | |
| Before: toFundedCreditActivityCursor(input.Before), | |
| Currency: input.Currency, | |
| }) | |
| if err != nil { | |
| return creditTransactionLoaderResult{}, err | |
| } | |
| items := make([]CreditTransaction, 0, len(result.Items)) | |
| for _, activity := range result.Items { | |
| annotations := models.Annotations{ | |
| ledger.AnnotationChargeID: activity.ChargeID.ID, | |
| } | |
| items = append(items, CreditTransaction{ | |
| ID: models.NamespacedID(activity.ChargeID), | |
| CreatedAt: activity.ChargeCreatedAt, | |
| BookedAt: activity.FundedAt, | |
| Type: CreditTransactionTypeFunded, | |
| Currency: activity.Currency, | |
| Amount: activity.Amount, | |
| Name: activity.Name, | |
| Description: activity.Description, | |
| Annotations: annotations, | |
| }) | |
| } | |
| return creditTransactionLoaderResult{ | |
| Items: items, | |
| HasMore: result.NextCursor != nil, | |
| }, nil | |
| func (l *fundedCreditTransactionLoader) Load(ctx context.Context, input creditTransactionLoaderInput) (creditTransactionLoaderResult, error) { | |
| result, err := l.service.ChargesService.ListFundedCreditActivities(ctx, charges.ListFundedCreditActivitiesInput{ | |
| Customer: input.CustomerID, | |
| Limit: input.Limit, | |
| After: toFundedCreditActivityCursor(input.After), | |
| Before: toFundedCreditActivityCursor(input.Before), | |
| Currency: input.Currency, | |
| }) | |
| if err != nil { | |
| return creditTransactionLoaderResult{}, err | |
| } | |
| items := make([]CreditTransaction, 0, len(result.Items)) | |
| for _, activity := range result.Items { | |
| annotations := models.Annotations{ | |
| ledger.AnnotationChargeID: activity.ChargeID.ID, | |
| } | |
| items = append(items, CreditTransaction{ | |
| ID: models.NamespacedID(activity.ChargeID), | |
| CreatedAt: activity.ChargeCreatedAt, | |
| BookedAt: activity.FundedAt, | |
| Type: CreditTransactionTypeFunded, | |
| Currency: activity.Currency, | |
| Amount: activity.Amount, | |
| Name: activity.Name, | |
| Description: activity.Description, | |
| Annotations: annotations, | |
| }) | |
| } | |
| hasMore := result.NextCursor != nil | |
| if input.Before != nil { | |
| hasMore = result.PreviousCursor != nil | |
| } | |
| return creditTransactionLoaderResult{ | |
| Items: items, | |
| HasMore: hasMore, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/funded_loader.go` around lines 20 - 54, The
loader currently always sets HasMore using result.NextCursor which is wrong for
backward pagination; in fundedCreditTransactionLoader.Load check whether
input.Before is non-nil and, if so, set creditTransactionLoaderResult.HasMore =
result.PrevCursor != nil, otherwise set HasMore = result.NextCursor != nil;
update the return to use this direction-aware logic (referencing
fundedCreditTransactionLoader.Load, creditTransactionLoaderInput.input.Before,
creditTransactionLoaderResult.HasMore, result.NextCursor and result.PrevCursor).
Comment on lines
+21
to
+43
| func (l *ledgerCreditTransactionLoader) Load(ctx context.Context, input creditTransactionLoaderInput) (creditTransactionLoaderResult, error) { | ||
| result, err := l.service.Ledger.ListTransactions(ctx, ledger.ListTransactionsInput{ | ||
| Namespace: input.CustomerID.Namespace, | ||
| Cursor: input.After, | ||
| Before: input.Before, | ||
| Limit: input.Limit, | ||
| AccountIDs: []string{input.AccountID}, | ||
| Currency: input.Currency, | ||
| CreditMovement: l.movement, | ||
| }) | ||
| if err != nil { | ||
| return creditTransactionLoaderResult{}, err | ||
| } | ||
|
|
||
| items, err := creditTransactionsFromLedgerTransactions(result.Items) | ||
| if err != nil { | ||
| return creditTransactionLoaderResult{}, err | ||
| } | ||
|
|
||
| return creditTransactionLoaderResult{ | ||
| Items: items, | ||
| HasMore: result.NextCursor != nil, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Make HasMore direction-aware.
transactions.go treats loader HasMore as “more items in the requested direction.” For before requests, checking only result.NextCursor can lose the backward-pagination signal.
🐛 Proposed direction-aware fix
items, err := creditTransactionsFromLedgerTransactions(result.Items)
if err != nil {
return creditTransactionLoaderResult{}, err
}
+ hasMore := result.NextCursor != nil
+ if input.Before != nil {
+ hasMore = result.PreviousCursor != nil
+ }
+
return creditTransactionLoaderResult{
Items: items,
- HasMore: result.NextCursor != nil,
+ HasMore: hasMore,
}, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/ledger_loader.go` around lines 21 - 43, The
HasMore value returned by ledgerCreditTransactionLoader.Load is not
direction-aware: change how creditTransactionLoaderResult.HasMore is computed in
Load (ledgerCreditTransactionLoader.Load) to reflect the requested pagination
direction by inspecting the input (input.Before vs input.After); when a backward
(before) request is made use result.PrevCursor != nil for HasMore, otherwise use
result.NextCursor != nil for forward pagination, so the loader's HasMore matches
transactions.go expectations.
Comment on lines
164
to
170
| if len(items) > 0 { | ||
| runningBalance, err := s.GetBalance(ctx, input.CustomerID, items[0].Currency, lo.ToPtr(result.Items[0].Cursor())) | ||
| runningBalance, err := s.GetBalance(ctx, input.CustomerID, items[0].Currency, lo.ToPtr(creditTransactionCursor(items[0]))) | ||
| if err != nil { | ||
| return ListCreditTransactionsResult{}, fmt.Errorf("get FBO balance after transaction %s: %w", result.Items[0].ID().ID, err) | ||
| return ListCreditTransactionsResult{}, fmt.Errorf("get FBO balance after transaction %s: %w", items[0].ID.ID, err) | ||
| } | ||
|
|
||
| applyCreditTransactionBalances(items, runningBalance.Settled()) |
Contributor
There was a problem hiding this comment.
Keep running balances separated by currency.
When input.Currency is nil, the merged list can contain multiple currencies, but this starts from items[0].Currency and applies that one running balance to every item. That can show incorrect balances for any later item in a different currency. Either require Currency, or compute balances per currency.
Proposed fix direction
- if len(items) > 0 {
- runningBalance, err := s.GetBalance(ctx, input.CustomerID, items[0].Currency, lo.ToPtr(creditTransactionCursor(items[0])))
- if err != nil {
- return ListCreditTransactionsResult{}, fmt.Errorf("get FBO balance after transaction %s: %w", items[0].ID.ID, err)
- }
-
- applyCreditTransactionBalances(items, runningBalance.Settled())
+ if err := s.applyCreditTransactionBalances(ctx, input.CustomerID, items); err != nil {
+ return ListCreditTransactionsResult{}, err
}-func applyCreditTransactionBalances(items []CreditTransaction, after alpacadecimal.Decimal) {
- runningBalance := after
-
- for i := range items {
- items[i].Balance.After = runningBalance
- items[i].Balance.Before = runningBalance.Sub(items[i].Amount)
- runningBalance = runningBalance.Sub(items[i].Amount)
- }
+func (s *service) applyCreditTransactionBalances(ctx context.Context, customerID customer.CustomerID, items []CreditTransaction) error {
+ runningBalances := make(map[currencyx.Code]alpacadecimal.Decimal)
+
+ for i := range items {
+ runningBalance, ok := runningBalances[items[i].Currency]
+ if !ok {
+ balance, err := s.GetBalance(ctx, customerID, items[i].Currency, lo.ToPtr(creditTransactionCursor(items[i])))
+ if err != nil {
+ return fmt.Errorf("get FBO balance after transaction %s: %w", items[i].ID.ID, err)
+ }
+
+ runningBalance = balance.Settled()
+ }
+
+ items[i].Balance.After = runningBalance
+ items[i].Balance.Before = runningBalance.Sub(items[i].Amount)
+ runningBalances[items[i].Currency] = runningBalance.Sub(items[i].Amount)
+ }
+
+ return nil
}Also applies to: 267-274
🤖 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 164 - 170, The
code assumes a single running balance by using items[0].Currency and applying it
to all credit transactions, which is incorrect when input.Currency is nil and
items contain multiple currencies; update the logic to handle balances per
currency: when input.Currency is nil, group items by their Currency, for each
currency compute runningBalance by calling s.GetBalance(ctx, input.CustomerID,
currency, lo.ToPtr(creditTransactionCursor(firstItemInGroup))) and then call
applyCreditTransactionBalances on that group's slice using the corresponding
runningBalance.Settled(); apply the same per-currency grouping/fix to the other
occurrence that uses applyCreditTransactionBalances (the block around the second
instance mentioned).
turip
reviewed
Apr 22, 2026
| }) | ||
| } | ||
|
|
||
| func (a *adapter) ListFundedCreditActivities(ctx context.Context, input charges.ListFundedCreditActivitiesInput) (charges.ListFundedCreditActivitiesResult, error) { |
Member
There was a problem hiding this comment.
Do you foresee that this will be needed for other types of charges or just credit purchases?
If it's for credit purchase putting it into creditpurchase might make sense, esp that there you can reuse the adapter methods for testing.
Contributor
Author
There was a problem hiding this comment.
just for creditpurchase so the "+" values show up as a single row. i'll move it there
Contributor
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 (2)
api/v3/handlers/customers/credits/list_transactions.go (2)
37-51:⚠️ Potential issue | 🟡 MinorConsider capping
page.sizewith an upper bound.Right now any positive
sizeis accepted and passed straight through asLimit, so a client can request e.g.page.size=1000000and force the loaders + merge step to materialize a huge batch. A small cap (say 100 or 500) would protect the endpoint without changing normal client behavior.🛡️ Suggested guard
- if size < 1 { - err := fmt.Errorf("must be greater than 0") + const maxPageSize = 100 + if size < 1 || size > maxPageSize { + err := fmt.Errorf("must be between 1 and %d", maxPageSize) return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ { Field: "page.size", - Reason: "must be greater than 0", + Reason: fmt.Sprintf("must be between 1 and %d", maxPageSize), Source: apierrors.InvalidParamSourceQuery, }, }) }🤖 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 37 - 51, The page.size value parsed via lo.FromPtrOr into the local variable size should be clamped to a sane maximum (e.g., 100 or 500) to prevent huge requests; update the validation block in list_transactions.go where size is set (using args.Params.Page.Size and lo.FromPtrOr) to enforce size = min(size, MAX_PAGE_SIZE) and if the incoming value exceeds MAX_PAGE_SIZE return an apierrors.NewBadRequestError with an InvalidParameters entry for "page.size" (reason like "must be <= <MAX_PAGE_SIZE>"); ensure the final Limit passed downstream (where ListCreditTransactionsRequest uses size) uses the clamped value.
93-100:⚠️ Potential issue | 🟡 MinorUnknown
filter.typeis silently dropped.
fromAPIBillingCreditTransactionTypereturnsnilfor any value it doesn’t recognize (e.g., a legacyadjustedor a typo), so the request proceeds as if no type filter was given — the user thinks they filtered but gets everything back. Validating explicitly here (or in the converter) and returning a 400 would be much kinder.🐛 Rough sketch
if args.Params.Filter != nil { - req.Type = fromAPIBillingCreditTransactionType(args.Params.Filter.Type) + if args.Params.Filter.Type != nil { + txType := fromAPIBillingCreditTransactionType(args.Params.Filter.Type) + if txType == nil { + err := fmt.Errorf("unsupported type: %s", *args.Params.Filter.Type) + return ListCreditTransactionsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ + { + Field: "filter.type", + Reason: err.Error(), + Source: apierrors.InvalidParamSourceQuery, + }, + }) + } + req.Type = txType + }🤖 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 93 - 100, The handler currently calls fromAPIBillingCreditTransactionType(args.Params.Filter.Type) and silently treats a nil return as "no filter"; change this to validate the converter's result and return a 400 Bad Request when the converter returns nil for a provided filter value. Specifically, after calling fromAPIBillingCreditTransactionType, if the result is nil and args.Params.Filter.Type is non-empty, return an HTTP 400 with a clear message that the provided filter.type is invalid; keep setting req.Currency as-is (currencyx.Code) and include the offending value in the error message for easier debugging.
♻️ Duplicate comments (1)
openmeter/ledger/customerbalance/ledger_loader.go (1)
21-43:⚠️ Potential issue | 🟠 MajorBackward pagination can’t signal
HasMorewith the current ledger result shape.
transactions.gotreats loaderHasMoreas “are there more items in the requested direction.” For forward (After) paginationresult.NextCursor != nilis right, but for backward (Before) pagination we need aPreviousCursorsignal — andledger.ListTransactionsResult(inopenmeter/ledger/primitives.go) only carriesNextCursor. So abeforepage that drains the window will still look like “no more older/newer items” even when more exist.Two ways to close this:
- Add
PreviousCursor *TransactionCursortoListTransactionsResult(and have the historical adapter populate it), then makeHasMoredirection-aware here (if input.Before != nil { hasMore = result.PreviousCursor != nil }).- Or, as a stopgap, have the ledger adapter overfetch by one in whichever direction is requested and surface that via
NextCursorsemantically, with a clear comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/ledger_loader.go` around lines 21 - 43, The loader's HasMore only checks result.NextCursor which is correct for forward (After) pagination but fails for backward (Before) pagination; update ledgerCreditTransactionLoader.Load to set HasMore based on the requested direction: if input.Before != nil use result.PreviousCursor != nil, otherwise use result.NextCursor != nil, and add PreviousCursor *TransactionCursor to the ListTransactionsResult type (in the ledger.ListTransactionsResult / primitives.go) and ensure the ledger adapter populates PreviousCursor (or alternatively implement directional overfetching in the adapter and document it) so the loader can make a direction-aware HasMore decision.
🧹 Nitpick comments (2)
api/v3/handlers/customers/credits/convert.go (1)
452-459: Default branch silently maps unknown internal types toconsumed.If a new
customerbalance.CreditTransactionTypeever lands (e.g., a reintroducedadjusted, or a future refund/expiry), it will surface over the API asconsumedwith no error — a subtle data-correctness footgun. Making the default explicit would turn such a bug into something you notice at code-review/test time.♻️ Suggested refactor
func toAPIBillingCreditTransactionType(txType customerbalance.CreditTransactionType) api.BillingCreditTransactionType { switch txType { case customerbalance.CreditTransactionTypeFunded: return api.BillingCreditTransactionTypeFunded - default: + case customerbalance.CreditTransactionTypeConsumed: return api.BillingCreditTransactionTypeConsumed + default: + // Fall back to consumed to avoid breaking the API contract, but + // surface it so unmapped types are caught early. + return api.BillingCreditTransactionTypeConsumed } }🤖 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 452 - 459, The mapping in toAPIBillingCreditTransactionType currently falls back to BillingCreditTransactionTypeConsumed for unknown customerbalance.CreditTransactionType values; change it to fail loudly instead: update toAPIBillingCreditTransactionType to explicitly handle all known customerbalance.CreditTransactionType cases and in the default branch either return a clear "unspecified"/unknown API enum (if one exists) or raise an error/panic/log.Fatalf with the unexpected txType value so new internal types (e.g., adjusted/refund/expiry) won't be silently mapped to "consumed". Ensure you reference customerbalance.CreditTransactionType constants in the switch and include the unexpected txType value in the error message for debugging.openmeter/ledger/customerbalance/transactions_test.go (1)
124-161: Nice coverage for the merge step.The interleaved funded/consumed setup exercises the heap merge cleanly. If you fancy it later, consider a small table-driven expansion (equal cursors / single-list / empty-list / limit ≥ total) to lock down the edge cases around
hasMoreand tie-breaks, but nothing blocking here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/transactions_test.go` around lines 124 - 161, TestMergeSortedLists_ByCursorDesc correctly covers the interleaved merge behavior using mergeSortedLists and compareCreditTransactionsByCursor; no functional change required, but optionally add table-driven cases exercising equal cursors, single-list, empty-list and limit >= total to validate hasMore and tie-break behavior across mergeSortedLists and compareCreditTransactionsByCursor.
🤖 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/adapter/ledger.go`:
- Around line 9-10: Remove the duplicate import alias for entgo by keeping a
single alias (use entsql) and update all call sites that use the other alias
(sql) to entsql; specifically, remove the import line that defines sql and
replace every usage of sql. with entsql. in the cursor predicate helper
functions (the cursor predicate helpers around the ~400–427 region) and in
ledgerTransactionBeforeCursorPredicate so all ent SQL calls consistently use
entsql.
In `@openmeter/ledger/primitives.go`:
- Around line 195-198: ListTransactionsResult currently only contains NextCursor
which prevents callers using ListTransactionsInput.Before from detecting
backward pagination; add a PreviousCursor *TransactionCursor field to the
ListTransactionsResult struct and update all adapters/implementations that build
ListTransactionsResult (e.g., the historical adapter) to populate PreviousCursor
symmetrically to how NextCursor is set so callers (like functions in
customerbalance/ledger_loader.go) can compute HasMore when paging backwards;
ensure TransactionCursor is reused for the new field and tests/consumers that
inspect ListTransactionsResult are updated accordingly.
---
Outside diff comments:
In `@api/v3/handlers/customers/credits/list_transactions.go`:
- Around line 37-51: The page.size value parsed via lo.FromPtrOr into the local
variable size should be clamped to a sane maximum (e.g., 100 or 500) to prevent
huge requests; update the validation block in list_transactions.go where size is
set (using args.Params.Page.Size and lo.FromPtrOr) to enforce size = min(size,
MAX_PAGE_SIZE) and if the incoming value exceeds MAX_PAGE_SIZE return an
apierrors.NewBadRequestError with an InvalidParameters entry for "page.size"
(reason like "must be <= <MAX_PAGE_SIZE>"); ensure the final Limit passed
downstream (where ListCreditTransactionsRequest uses size) uses the clamped
value.
- Around line 93-100: The handler currently calls
fromAPIBillingCreditTransactionType(args.Params.Filter.Type) and silently treats
a nil return as "no filter"; change this to validate the converter's result and
return a 400 Bad Request when the converter returns nil for a provided filter
value. Specifically, after calling fromAPIBillingCreditTransactionType, if the
result is nil and args.Params.Filter.Type is non-empty, return an HTTP 400 with
a clear message that the provided filter.type is invalid; keep setting
req.Currency as-is (currencyx.Code) and include the offending value in the error
message for easier debugging.
---
Duplicate comments:
In `@openmeter/ledger/customerbalance/ledger_loader.go`:
- Around line 21-43: The loader's HasMore only checks result.NextCursor which is
correct for forward (After) pagination but fails for backward (Before)
pagination; update ledgerCreditTransactionLoader.Load to set HasMore based on
the requested direction: if input.Before != nil use result.PreviousCursor !=
nil, otherwise use result.NextCursor != nil, and add PreviousCursor
*TransactionCursor to the ListTransactionsResult type (in the
ledger.ListTransactionsResult / primitives.go) and ensure the ledger adapter
populates PreviousCursor (or alternatively implement directional overfetching in
the adapter and document it) so the loader can make a direction-aware HasMore
decision.
---
Nitpick comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 452-459: The mapping in toAPIBillingCreditTransactionType
currently falls back to BillingCreditTransactionTypeConsumed for unknown
customerbalance.CreditTransactionType values; change it to fail loudly instead:
update toAPIBillingCreditTransactionType to explicitly handle all known
customerbalance.CreditTransactionType cases and in the default branch either
return a clear "unspecified"/unknown API enum (if one exists) or raise an
error/panic/log.Fatalf with the unexpected txType value so new internal types
(e.g., adjusted/refund/expiry) won't be silently mapped to "consumed". Ensure
you reference customerbalance.CreditTransactionType constants in the switch and
include the unexpected txType value in the error message for debugging.
In `@openmeter/ledger/customerbalance/transactions_test.go`:
- Around line 124-161: TestMergeSortedLists_ByCursorDesc correctly covers the
interleaved merge behavior using mergeSortedLists and
compareCreditTransactionsByCursor; no functional change required, but optionally
add table-driven cases exercising equal cursors, single-list, empty-list and
limit >= total to validate hasMore and tie-break behavior across
mergeSortedLists and compareCreditTransactionsByCursor.
🪄 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: db089d82-4cd5-4668-ba8e-f83114d48f28
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (28)
AGENTS.mdapi/spec/packages/aip/src/customers/credits/ledger.tspapi/spec/packages/aip/src/customers/credits/operations.tspapi/v3/api.gen.goapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/handlers/customers/credits/list_transactions_test.goopenmeter/billing/charges/adapter.goopenmeter/billing/charges/adapter/search.goopenmeter/billing/charges/adapter/search_test.goopenmeter/billing/charges/funded_credit_activity.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/funded_credit_activity.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/ledger_loader.goopenmeter/ledger/customerbalance/loaders.goopenmeter/ledger/customerbalance/merge.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/customerbalance/transactions_test.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/repo.goopenmeter/ledger/noop/noop.goopenmeter/ledger/primitives.go
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- openmeter/ledger/customerbalance/merge.go
- openmeter/billing/charges/funded_credit_activity.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/ledger/customerbalance/facade_test.go
- openmeter/ledger/customerbalance/service.go
- openmeter/billing/charges/service/funded_credit_activity.go
- openmeter/ledger/historical/repo.go
- api/v3/handlers/customers/credits/list_transactions_test.go
- openmeter/billing/charges/adapter.go
- openmeter/ledger/noop/noop.go
- api/spec/packages/aip/src/customers/credits/operations.tsp
- openmeter/billing/charges/service.go
- openmeter/ledger/customerbalance/loaders.go
- openmeter/ledger/historical/ledger.go
Comment on lines
+195
to
+198
| type ListTransactionsResult struct { | ||
| Items []Transaction | ||
| NextCursor *TransactionCursor | ||
| } |
Contributor
There was a problem hiding this comment.
ListTransactionsResult needs a PreviousCursor for backward pagination to work.
ListTransactionsInput now accepts both Cursor (after) and Before, but the result only exposes NextCursor. Callers that page backward with Before have no way to know whether more items exist in the backward direction — and that’s exactly what openmeter/ledger/customerbalance/ledger_loader.go runs into when computing HasMore. Adding a PreviousCursor *TransactionCursor (and having the historical adapter populate it symmetrically with NextCursor) would close the gap.
🐛 Suggested shape
type ListTransactionsResult struct {
Items []Transaction
NextCursor *TransactionCursor
+ // PreviousCursor points at the window boundary for backward pagination;
+ // it is set when more items exist before the returned page.
+ PreviousCursor *TransactionCursor
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/primitives.go` around lines 195 - 198,
ListTransactionsResult currently only contains NextCursor which prevents callers
using ListTransactionsInput.Before from detecting backward pagination; add a
PreviousCursor *TransactionCursor field to the ListTransactionsResult struct and
update all adapters/implementations that build ListTransactionsResult (e.g., the
historical adapter) to populate PreviousCursor symmetrically to how NextCursor
is set so callers (like functions in customerbalance/ledger_loader.go) can
compute HasMore when paging backwards; ensure TransactionCursor is reused for
the new field and tests/consumers that inspect ListTransactionsResult are
updated accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
openmeter/ledger/customerbalance/funded_loader.go (1)
69-74: Tiny nit — the helper can just be a type conversion.
meta.ChargeIDandmodels.NamespacedIDshare the same field shape (you already rely on that at Line 39 for the reverse direction), so the helper adds indirection without pulling its weight.♻️ Suggested simplification
-func toFundedCreditActivityCursor(cursor *ledger.TransactionCursor) *creditpurchase.FundedCreditActivityCursor { - if cursor == nil { - return nil - } - - return &creditpurchase.FundedCreditActivityCursor{ - FundedAt: cursor.BookedAt, - ChargeCreatedAt: cursor.CreatedAt, - ChargeID: chargesFundedCursorChargeID(cursor.ID), - } -} - -func chargesFundedCursorChargeID(id models.NamespacedID) meta.ChargeID { - return meta.ChargeID{ - Namespace: id.Namespace, - ID: id.ID, - } -} +func toFundedCreditActivityCursor(cursor *ledger.TransactionCursor) *creditpurchase.FundedCreditActivityCursor { + if cursor == nil { + return nil + } + + return &creditpurchase.FundedCreditActivityCursor{ + FundedAt: cursor.BookedAt, + ChargeCreatedAt: cursor.CreatedAt, + ChargeID: meta.ChargeID(cursor.ID), + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/funded_loader.go` around lines 69 - 74, The helper chargesFundedCursorChargeID unnecessarily re-constructs meta.ChargeID; replace its body with a direct type conversion from models.NamespacedID to meta.ChargeID (i.e., return meta.ChargeID(id)) so it relies on the identical field shape (matching the reverse conversion used elsewhere) — alternatively remove the helper and use meta.ChargeID(...) at call sites.openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.go (1)
120-277: Solid pagination coverage — really nice scenarios.The
TestPaginatesWithBeforecase in particular exercises the non-trivial bit (forward → forward → backward viaBefore→ forward again) and catches ordering correctness of the reverse step. Consider a follow-up adding a test for theAfter+Beforemutual-exclusivity validation, just so the input contract is pinned down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.go` around lines 120 - 277, Add a test that asserts the API rejects simultaneous After and Before inputs: in the test file add a new case (e.g., TestAfterBeforeMutualExclusivity) that calls ListFundedCreditActivities with both After (use a NextCursor from a prior call) and Before (construct a creditpurchase.FundedCreditActivityCursor from an existing item) and assert it returns an error (or validation failure) rather than a page; this verifies the mutual-exclusivity contract for the ListFundedCreditActivities input.openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.go (1)
41-69: Heads-up on predicate/order layering — works today, worth a sanity check.The cursor predicates (
After/Before) and the currency filter are appended afterLimit(input.Limit + 1)is already set. Ent is tolerant of this ordering today, but a defensive reorder (apply predicates → order → limit) would make intent clearer and match the pattern used inopenmeter/ledger/historical/adapter/ledger.goaround lines 322–365. Purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.go` around lines 41 - 69, The predicates and currency filter should be applied before ordering and limiting: move the query.Where calls that use fundedCreditActivityAfterPredicate, fundedCreditActivityBeforePredicate and the dbchargecreditpurchase.CurrencyEQ filter (currently gated by input.After, input.Before, input.Currency) to run before you call query.Order (dbchargecreditpurchasecreditgrant.ByGrantedAt/ByCreditPurchaseField/ByChargeID) and before Limit; then apply Order and finally Limit(input.Limit + 1). This keeps the intended predicate→order→limit sequence and aligns with the pattern used in ledger.go.
🤖 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/billing/charges/creditpurchase/adapter/funded_credit_activity_test.go`:
- Around line 56-66: The tests use context.Background() which detaches DB
operations from the test lifecycle; update createCustomer and the three test
cases (TestPaginatesWithAfter, TestPaginatesWithBefore, TestFiltersByCurrency)
to use s.T().Context() instead of context.Background() so database calls are
tied to the test's context (match the seed helper which already uses
s.T().Context()); locate calls in createCustomer and the three test methods and
replace context.Background() with s.T().Context().
In `@openmeter/ledger/historical/adapter/ledger.go`:
- Around line 356-365: When paginating with input.Before set the current code
picks nextCursor = items[len(items)-1] after possibly reversing items, which
yields the item closest to the Before cursor instead of the newest item in the
returned page; change the logic in ledger.go so that when input.Before != nil
(or after slices.Reverse(items) has run) you set nextCursor = &items[0].Cursor()
(with the same hasMore and len(items) checks and preserving
ledger.TransactionCursor type), keeping the existing behavior for the forward
case (items[len(items)-1]) so consumers like ledgerCreditTransactionLoader that
only nil-check NextCursor still see a correct continuation cursor for backward
pagination.
---
Nitpick comments:
In
`@openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.go`:
- Around line 120-277: Add a test that asserts the API rejects simultaneous
After and Before inputs: in the test file add a new case (e.g.,
TestAfterBeforeMutualExclusivity) that calls ListFundedCreditActivities with
both After (use a NextCursor from a prior call) and Before (construct a
creditpurchase.FundedCreditActivityCursor from an existing item) and assert it
returns an error (or validation failure) rather than a page; this verifies the
mutual-exclusivity contract for the ListFundedCreditActivities input.
In `@openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.go`:
- Around line 41-69: The predicates and currency filter should be applied before
ordering and limiting: move the query.Where calls that use
fundedCreditActivityAfterPredicate, fundedCreditActivityBeforePredicate and the
dbchargecreditpurchase.CurrencyEQ filter (currently gated by input.After,
input.Before, input.Currency) to run before you call query.Order
(dbchargecreditpurchasecreditgrant.ByGrantedAt/ByCreditPurchaseField/ByChargeID)
and before Limit; then apply Order and finally Limit(input.Limit + 1). This
keeps the intended predicate→order→limit sequence and aligns with the pattern
used in ledger.go.
In `@openmeter/ledger/customerbalance/funded_loader.go`:
- Around line 69-74: The helper chargesFundedCursorChargeID unnecessarily
re-constructs meta.ChargeID; replace its body with a direct type conversion from
models.NamespacedID to meta.ChargeID (i.e., return meta.ChargeID(id)) so it
relies on the identical field shape (matching the reverse conversion used
elsewhere) — alternatively remove the helper and use meta.ChargeID(...) at call
sites.
🪄 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: 8cd34fb6-2d55-4bba-8f50-f0b97c224559
📒 Files selected for processing (12)
app/common/customerbalance.goopenmeter/billing/charges/creditpurchase/adapter.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.goopenmeter/billing/charges/creditpurchase/funded_credit_activity.goopenmeter/billing/charges/creditpurchase/service.goopenmeter/billing/charges/creditpurchase/service/funded_credit_activity.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions_test.goopenmeter/ledger/historical/adapter/ledger.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/ledger/customerbalance/transactions_test.go
- openmeter/ledger/customerbalance/testenv_test.go
- openmeter/ledger/customerbalance/service.go
Comment on lines
+56
to
+66
| func (s *ListFundedCreditActivitiesSuite) createCustomer(namespace string) string { | ||
| s.T().Helper() | ||
|
|
||
| c, err := s.dbClient.Customer.Create(). | ||
| SetNamespace(namespace). | ||
| SetName("test-customer"). | ||
| Save(context.Background()) | ||
| s.Require().NoError(err) | ||
|
|
||
| return c.ID | ||
| } |
Contributor
There was a problem hiding this comment.
Prefer s.T().Context() over context.Background() in the test helpers and cases.
The seed helper already uses s.T().Context() at Lines 100 and 111, but createCustomer and the three test methods fall back to context.Background() (Lines 62, 121, 180, 280). Switching these to s.T().Context() ties DB operations to the test's lifecycle so a test cancellation actually cancels in-flight queries.
🔧 Suggested change (repeat the same edit in `TestPaginatesWithAfter`, `TestPaginatesWithBefore`, and `TestFiltersByCurrency`)
func (s *ListFundedCreditActivitiesSuite) createCustomer(namespace string) string {
s.T().Helper()
c, err := s.dbClient.Customer.Create().
SetNamespace(namespace).
SetName("test-customer").
- Save(context.Background())
+ Save(s.T().Context())
s.Require().NoError(err) func (s *ListFundedCreditActivitiesSuite) TestPaginatesWithAfter() {
- ctx := context.Background()
+ ctx := s.T().Context()As per coding guidelines: "In tests, prefer t.Context() when a testing.T or testing.TB is available instead of introducing context.Background() to keep cancellation and lifecycle tied to the test harness".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *ListFundedCreditActivitiesSuite) createCustomer(namespace string) string { | |
| s.T().Helper() | |
| c, err := s.dbClient.Customer.Create(). | |
| SetNamespace(namespace). | |
| SetName("test-customer"). | |
| Save(context.Background()) | |
| s.Require().NoError(err) | |
| return c.ID | |
| } | |
| func (s *ListFundedCreditActivitiesSuite) createCustomer(namespace string) string { | |
| s.T().Helper() | |
| c, err := s.dbClient.Customer.Create(). | |
| SetNamespace(namespace). | |
| SetName("test-customer"). | |
| Save(s.T().Context()) | |
| s.Require().NoError(err) | |
| return c.ID | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.go`
around lines 56 - 66, The tests use context.Background() which detaches DB
operations from the test lifecycle; update createCustomer and the three test
cases (TestPaginatesWithAfter, TestPaginatesWithBefore, TestFiltersByCurrency)
to use s.T().Context() instead of context.Background() so database calls are
tied to the test's context (match the seed helper which already uses
s.T().Context()); locate calls in createCustomer and the three test methods and
replace context.Background() with s.T().Context().
Comment on lines
+18
to
+39
| func (a *adapter) ListFundedCreditActivities(ctx context.Context, input creditpurchase.ListFundedCreditActivitiesInput) (creditpurchase.ListFundedCreditActivitiesResult, error) { | ||
| return ListFundedCreditActivities(ctx, a.db, input) | ||
| } | ||
|
|
||
| func ListFundedCreditActivities(ctx context.Context, dbClient *db.Client, input creditpurchase.ListFundedCreditActivitiesInput) (creditpurchase.ListFundedCreditActivitiesResult, error) { | ||
| query := dbClient.ChargeCreditPurchaseCreditGrant.Query(). | ||
| Where( | ||
| dbchargecreditpurchasecreditgrant.Namespace(input.Customer.Namespace), | ||
| dbchargecreditpurchasecreditgrant.DeletedAtIsNil(), | ||
| dbchargecreditpurchasecreditgrant.HasCreditPurchaseWith( | ||
| dbchargecreditpurchase.Namespace(input.Customer.Namespace), | ||
| dbchargecreditpurchase.CustomerIDEQ(input.Customer.ID), | ||
| dbchargecreditpurchase.DeletedAtIsNil(), | ||
| ), | ||
| ). | ||
| WithCreditPurchase(func(q *db.ChargeCreditPurchaseQuery) { | ||
| q.Where( | ||
| dbchargecreditpurchase.Namespace(input.Customer.Namespace), | ||
| dbchargecreditpurchase.DeletedAtIsNil(), | ||
| ) | ||
| }). | ||
| Limit(input.Limit + 1) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how other charges adapters wrap Ent calls to confirm the expected pattern in this module.
rg -nP --type=go -C3 'entutils\.(TransactingRepo|TransactingRepoWithNoValue)\b' openmeter/billing/chargesRepository: openmeterio/openmeter
Length of output: 32821
🏁 Script executed:
cat -n openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.goRepository: openmeterio/openmeter
Length of output: 7138
🏁 Script executed:
rg -n "ListFundedCreditActivities" openmeter/billing/charges/creditpurchase/service/Repository: openmeterio/openmeter
Length of output: 802
Wrap Ent access with entutils.TransactingRepo() to honor the transaction context.
ListFundedCreditActivities is called inside transaction.Run(...) in the service layer, which puts a tx driver on ctx. Your implementation queries a.db / dbClient directly, bypassing that context-driven transaction. Without wrapping with entutils.TransactingRepo(), the query executes outside the active transaction and won't see uncommitted writes from the same unit of work.
This pattern is used throughout the charges adapter layer (usagebased, flatfee, meta, lineage, etc.), so keep it consistent here too:
Fix sketch
func (a *adapter) ListFundedCreditActivities(ctx context.Context, input creditpurchase.ListFundedCreditActivitiesInput) (creditpurchase.ListFundedCreditActivitiesResult, error) {
- return ListFundedCreditActivities(ctx, a.db, input)
+ return entutils.TransactingRepo(ctx, a, func(ctx context.Context, tx *adapter) (creditpurchase.ListFundedCreditActivitiesResult, error) {
+ return listFundedCreditActivities(ctx, tx.db, input)
+ })
}(then rename the free function to lowercase, or inline it).
Per coding guidelines: "In openmeter/billing/charges/.../adapter, keep Ent access transaction-aware by wrapping with entutils.TransactingRepo() / TransactingRepoWithNoValue()"
Comment on lines
+356
to
+365
| if input.Before != nil { | ||
| slices.Reverse(items) | ||
| } | ||
|
|
||
| var nextCursor *ledger.TransactionCursor | ||
| if hasMore && len(items) > 0 { | ||
| nextItem := items[len(items)-1] | ||
| cursor := nextItem.Cursor() | ||
| nextCursor = &cursor | ||
| } |
Contributor
There was a problem hiding this comment.
NextCursor value looks off when paginating with Before.
Nice work on the fetch-Limit+1 + hasMore pattern — the detection side is solid. One thing to sanity-check though: when input.Before != nil, items are fetched in ASC order, truncated to Limit (dropping the furthest/newest neighbor), then reversed so the slice is returned in DESC display order [newest-in-set, …, just-after-cursor]. At that point items[len(items)-1] is the item closest to the original Before cursor, not the newest in the returned set.
For any caller that wants to continue paginating backward with Before = result.NextCursor, that's the wrong end — the continuation cursor for backward pagination should be items[0] (the newest item in the current page). The current ledgerCreditTransactionLoader in openmeter/ledger/customerbalance/ledger_loader.go only checks NextCursor != nil as a "has more" signal, so this doesn't blow up today, but it's a latent footgun the next consumer could trip on.
Worth either: (a) flipping NextCursor to items[0].Cursor() when input.Before != nil, or (b) documenting that NextCursor is always "forward-continuation only" and returning nil (or a separate PreviousCursor) in the Before case.
🛠️ Quick patch idea (option a)
var nextCursor *ledger.TransactionCursor
if hasMore && len(items) > 0 {
- nextItem := items[len(items)-1]
- cursor := nextItem.Cursor()
+ var nextItem *ledgerhistorical.Transaction
+ if input.Before != nil {
+ nextItem = items[0]
+ } else {
+ nextItem = items[len(items)-1]
+ }
+ cursor := nextItem.Cursor()
nextCursor = &cursor
}Want to double-check whether any other caller of ListTransactions relies on NextCursor's value (vs. just nil-check) after a Before query:
#!/bin/bash
# Find every consumer of ListTransactionsResult.NextCursor and inspect how the value is used.
rg -nP --type=go -C5 '\.NextCursor\b'
# And any call site that passes Before, to see if they subsequently thread NextCursor back in.
rg -nP --type=go -C3 'ListTransactionsInput\{' 🤖 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 356 - 365, When
paginating with input.Before set the current code picks nextCursor =
items[len(items)-1] after possibly reversing items, which yields the item
closest to the Before cursor instead of the newest item in the returned page;
change the logic in ledger.go so that when input.Before != nil (or after
slices.Reverse(items) has run) you set nextCursor = &items[0].Cursor() (with the
same hasMore and len(items) checks and preserving ledger.TransactionCursor
type), keeping the existing behavior for the forward case (items[len(items)-1])
so consumers like ledgerCreditTransactionLoader that only nil-check NextCursor
still see a correct continuation cursor for backward pagination.
turip
approved these changes
Apr 23, 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.
Summary
This PR refactors customer credit transaction listing to return user-meaningful activity while moving the endpoint to cursor pagination semantics that are consistent with our current implementation approach (opaque tokens).
Why
The old transaction list exposed low-level ledger rows 1:1, which is confusing for users (especially for funded credits where one purchase may map to multiple ledger entries).
We now present activity aligned with what users expect to see: funded and consumed movements.
What Changed
Reworked credit transaction domain semantics:
fundedandconsumedadjustedfrom this listing surface for nowIntroduced funded activity source from Charges:
Kept consumed activity source from Ledger:
Added loader-based aggregation in customer balance:
booked_at,created_at,id)Cursor pagination updates:
Common.CursorPaginationQuerybooked_at,created_at,id)before/afterhandling through the stack for this use casefirst/lastcursors from API response meta (keptnext/previous)Summary by CodeRabbit
Release Notes
New Features
Changes