feat: feature filters - #4536
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (46)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (44)
📝 WalkthroughWalkthroughAdds an optional ChangesFeature-key filtering for credit balance and transactions
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Facade
participant Service
participant HistoricalLedger
participant BreakageSvc
participant CreditPurchaseSvc
Client->>Handler: GET /credits/balance?filter[feature_key][eq]=api-calls
Handler->>Handler: fromAPICustomerCreditFeatureFilter → FeatureFilter
Handler->>Facade: GetBalances(GetBalancesInput{FeatureFilter})
Facade->>Service: GetBalance(GetBalanceServiceInput{FeatureFilter})
Service->>Service: featureFilterRoute → RouteFilter{MatchFeature}
Service->>HistoricalLedger: GetBalance(bookedRoute)
Service->>HistoricalLedger: GetBalance(advanceRoute)
Service->>Service: getChargePendingBalanceImpacts(featureFilter)
Service-->>Facade: ledger.Balance
Facade-->>Handler: balance response
Handler-->>Client: 200 OK
Client->>Handler: GET /credits/transactions?filter[feature_key][eq]=api-calls
Handler->>Facade: ListCreditTransactions(ListCreditTransactionsInput{FeatureFilter})
Facade->>CreditPurchaseSvc: ListFundedCreditActivities(FeatureFilter)
Facade->>HistoricalLedger: ListTransactions(Route{MatchFeature})
Facade->>BreakageSvc: ListExpiredBreakageImpacts(Route{MatchFeature})
Facade-->>Handler: merged transactions
Handler-->>Client: 200 OK
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 |
Contributor
Greptile SummaryThis PR implements optional
Confidence Score: 5/5Safe to merge; the feature filter logic is correctly implemented and consistently threaded across all layers with good test coverage. The tri-state filter semantics are consistently applied from the API handler through service, routing, breakage, and funded-credit adapters. SQL predicates use correct @> array-containment. The two flagged items are a latent silent-match-all edge case unreachable through current validation, and an O(n) DB-call pattern in the funded loader that could affect high-traffic customers but does not produce incorrect results. openmeter/ledger/customerbalance/funded_loader.go warrants a second look on the per-activity DB lookup before this hits high-volume customers. Important Files Changed
Reviews (5): Last reviewed commit: "chore: review comments" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 3
🧹 Nitpick comments (6)
api/spec/packages/aip/src/customers/credits/operations.tsp (1)
155-160: ⚡ Quick winClarify the exact supported
feature_keyoperators in the API docs.Nice addition overall. One small contract-clarity tweak: these fields use
Common.StringFieldFilter, but downstream handling only supportseq,oeq, andexists=false(whileexists=true/other operators are rejected). Calling that out in the doc comments here would make the API behavior much clearer for SDK/users.Suggested doc tweak
/** * Filter credit balance by feature key. Omit to return the total portfolio value. - * Use `exists=false` to return only unrestricted balance. + * Supported operators: `eq`, `oeq`, and `exists=false`. + * Use `exists=false` to return only unrestricted balance. */ feature_key?: Common.StringFieldFilter;/** * Filter credit transactions by feature key. Omit to return all credit - * transactions. Use `exists=false` to return only unrestricted credit + * transactions. Supported operators: `eq`, `oeq`, and `exists=false`. + * Use `exists=false` to return only unrestricted credit * transactions. */ feature_key?: Common.StringFieldFilter;As per coding guidelines, the declared API should be accurate and easy to understand for users.
Also applies to: 207-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/spec/packages/aip/src/customers/credits/operations.tsp` around lines 155 - 160, Update the JSDoc comments for the `feature_key` parameter field to clarify which operators are actually supported by the downstream implementation. Add a note specifying that only `eq`, `oeq`, and `exists=false` operators are supported for this `Common.StringFieldFilter`, while other operators like `exists=true` are rejected. Apply this same clarification to the similar field at line 207-213 to ensure API documentation accurately reflects the actual supported behavior.Source: Coding guidelines
test/credits/creditgrant_test.go (1)
190-193: ⚡ Quick winUse test-scoped context in this updated test.
Small tweak: switch to
s.T().Context()here so cancellation/lifecycle stays tied to the test harness.Suggested patch
func (s *CreditGrantTestSuite) TestCreateFeatureFilteredGrant() { - ctx := context.Background() + ctx := s.T().Context() ns := s.GetUniqueNamespace("creditgrant-service-feature-filters") s.ProvisionDefaultTaxCodes(ctx, ns)As per coding guidelines: "Use
t.Context()in Go tests when atesting.Tortesting.TBis available instead of introducingcontext.Background()to keep cancellation tied to the test harness."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/credits/creditgrant_test.go` around lines 190 - 193, In the TestCreateFeatureFilteredGrant method, replace the context initialization from context.Background() to s.T().Context() so that the test's cancellation and lifecycle are properly tied to the test harness. Change the line that declares ctx to use s.T().Context() instead of context.Background().Source: Coding guidelines
openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.go (1)
136-155: 💤 Low valueAccessing
features[0]relies on upstream validation ensuring exactly one feature.The code accesses
features[0]directly after normalization. This is safe becauseValidateAsFeatureFilter()enforces exactly one feature per the upstream contract, but adding a defensive length check would make this more resilient to future changes in validation logic.That said, validation happens in
ListFundedCreditActivitiesInput.Validate()before this code runs, so this should be fine in practice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.go` around lines 136 - 155, In the fundedCreditActivityFeatureFilterPredicate function, add a defensive length check on the features slice before accessing features[0]. After normalizing features, verify that the slice has exactly one element before attempting to access features[0] in the predicate; if the length check fails, return an appropriate default predicate (such as nil or dbchargecreditpurchase.FeatureFiltersIsNil()) to prevent potential index out of bounds errors.openmeter/ledger/historical/adapter/ledger.go (3)
609-636: ⚖️ Poor tradeoffConsider extracting shared Postgres array predicate types to a common package.
These types (
postgresArrayRouteOperator,postgresQualifiedColumn,postgresArrayRouteExpression) are duplicated inopenmeter/ledger/breakage/adapter/query.go. Extracting them to a shared internal package would reduce duplication and make future maintenance easier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/historical/adapter/ledger.go` around lines 609 - 636, The types postgresArrayRouteOperator, postgresQualifiedColumn, and postgresArrayRouteExpression along with their methods (Ident and Predicate) are currently defined in openmeter/ledger/historical/adapter/ledger.go but are duplicated in openmeter/ledger/breakage/adapter/query.go. Create a new shared internal package (e.g., openmeter/internal/postgres or similar) and move these type definitions and their associated methods to this common location. Then update both the historical and breakage adapter files to import and use these types from the new shared package instead of maintaining duplicate definitions.
420-429: 💤 Low valuePotential duplicate currency predicate when both
currencyandroute.Currencyare set.If both the
currencyparameter androute.Currencyare populated, this function will append two separate currency equality predicates. While they'd both match the same rows if identical, it adds redundant conditions to the query. Consider checking if they're the same value before appending:🔧 Suggested fix
func listTransactionsRoutePredicates(currency *currencyx.Code, route ledger.RouteFilter) []predicate.LedgerSubAccountRoute { routePredicates := make([]predicate.LedgerSubAccountRoute, 0, 3) if currency != nil { routePredicates = append(routePredicates, ledgersubaccountroutedb.Currency(string(*currency))) } - if route.Currency != "" { + if route.Currency != "" && (currency == nil || route.Currency != *currency) { routePredicates = append(routePredicates, ledgersubaccountroutedb.Currency(string(route.Currency))) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/historical/adapter/ledger.go` around lines 420 - 429, The listTransactionsRoutePredicates function appends duplicate currency predicates when both the currency parameter and route.Currency field are populated, creating redundant query conditions. Remove the second currency predicate append that checks route.Currency, and instead consolidate the logic so that only one currency predicate is added. Either prioritize the currency parameter if it's not nil, or use route.Currency as a fallback when currency is nil, ensuring only a single ledgersubaccountroutedb.Currency predicate is appended to the routePredicates slice.
564-573: 💤 Low valueSame potential duplicate currency predicate issue in the scoped route selector.
This helper has the same pattern where both
currencyandroute.Currencycould add separate predicates for the same filter. Keep it consistent with whatever approach you take inlistTransactionsRoutePredicates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/historical/adapter/ledger.go` around lines 564 - 573, The scopedRouteSelectorPredicates function has duplicate currency predicate logic where both the currency parameter check and the route.Currency check independently add predicates for the same FieldCurrency field. Refactor the function to consolidate these two conditions into a single logic block that adds only one currency predicate, prioritizing the appropriate source (either currency parameter or route.Currency field) to match the approach used in listTransactionsRoutePredicates. Remove the separate if blocks for currency and route.Currency, and replace them with unified logic that prevents duplicate predicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v3/handlers/customers/credits/feature_filter_test.go`:
- Around line 15-61: The TestFromAPICustomerCreditFeatureFilter table test is
missing coverage for unsupported operators that are handled by
unsupportedCustomerCreditFeatureKeyOperator. Add six new test cases to the tests
slice to cover the neq, ocontains, gt, gte, lt, and lte operators in the
api.StringFieldFilter. Each test case should set the corresponding field on the
StringFieldFilter to an appropriate test value and expect wantErr to be true,
ensuring that all unsupported operator branches are tested and
regression-protected.
In `@openmeter/ledger/breakage/adapter/record_test.go`:
- Around line 39-87: Add a new test case to the tests slice that covers the
scenario where MatchFeature is set but Route.Currency is left empty or unset,
allowing the currency value to come from the top-level context. Create this test
case similar to the existing "feature match route includes unrestricted and
containing features" test case but ensure Route.Currency is not populated in the
ledger.RouteFilter struct, then define the appropriate want slice with the
expected matching records based on feature matching behavior.
In `@openmeter/ledger/customerbalance/service_test.go`:
- Around line 87-95: The test case "after and as of both set" is using an empty
ledger.TransactionCursor{} on line 92, which causes an earlier validation error
and masks the mutual-exclusion check between After and AsOf that the test is
meant to verify. Replace the empty ledger.TransactionCursor{} with a properly
initialized valid cursor (e.g., one that would normally pass cursor validation)
so the test can isolate and validate the specific rule that After and AsOf
cannot both be set simultaneously.
---
Nitpick comments:
In `@api/spec/packages/aip/src/customers/credits/operations.tsp`:
- Around line 155-160: Update the JSDoc comments for the `feature_key` parameter
field to clarify which operators are actually supported by the downstream
implementation. Add a note specifying that only `eq`, `oeq`, and `exists=false`
operators are supported for this `Common.StringFieldFilter`, while other
operators like `exists=true` are rejected. Apply this same clarification to the
similar field at line 207-213 to ensure API documentation accurately reflects
the actual supported behavior.
In `@openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.go`:
- Around line 136-155: In the fundedCreditActivityFeatureFilterPredicate
function, add a defensive length check on the features slice before accessing
features[0]. After normalizing features, verify that the slice has exactly one
element before attempting to access features[0] in the predicate; if the length
check fails, return an appropriate default predicate (such as nil or
dbchargecreditpurchase.FeatureFiltersIsNil()) to prevent potential index out of
bounds errors.
In `@openmeter/ledger/historical/adapter/ledger.go`:
- Around line 609-636: The types postgresArrayRouteOperator,
postgresQualifiedColumn, and postgresArrayRouteExpression along with their
methods (Ident and Predicate) are currently defined in
openmeter/ledger/historical/adapter/ledger.go but are duplicated in
openmeter/ledger/breakage/adapter/query.go. Create a new shared internal package
(e.g., openmeter/internal/postgres or similar) and move these type definitions
and their associated methods to this common location. Then update both the
historical and breakage adapter files to import and use these types from the new
shared package instead of maintaining duplicate definitions.
- Around line 420-429: The listTransactionsRoutePredicates function appends
duplicate currency predicates when both the currency parameter and
route.Currency field are populated, creating redundant query conditions. Remove
the second currency predicate append that checks route.Currency, and instead
consolidate the logic so that only one currency predicate is added. Either
prioritize the currency parameter if it's not nil, or use route.Currency as a
fallback when currency is nil, ensuring only a single
ledgersubaccountroutedb.Currency predicate is appended to the routePredicates
slice.
- Around line 564-573: The scopedRouteSelectorPredicates function has duplicate
currency predicate logic where both the currency parameter check and the
route.Currency check independently add predicates for the same FieldCurrency
field. Refactor the function to consolidate these two conditions into a single
logic block that adds only one currency predicate, prioritizing the appropriate
source (either currency parameter or route.Currency field) to match the approach
used in listTransactionsRoutePredicates. Remove the separate if blocks for
currency and route.Currency, and replace them with unified logic that prevents
duplicate predicates.
In `@test/credits/creditgrant_test.go`:
- Around line 190-193: In the TestCreateFeatureFilteredGrant method, replace the
context initialization from context.Background() to s.T().Context() so that the
test's cancellation and lifecycle are properly tied to the test harness. Change
the line that declares ctx to use s.T().Context() instead of
context.Background().
🪄 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: 167a43ca-5865-4e80-8c3d-d7d8f0cbb189
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (45)
api/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/customers/credits/operations.tspapi/v3/api.gen.goapi/v3/filters/parse.goapi/v3/filters/parse_test.goapi/v3/handlers/customers/credits/feature_filter.goapi/v3/handlers/customers/credits/feature_filter_test.goapi/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/test/filters_test.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.goopenmeter/billing/charges/creditpurchase/charge_test.goopenmeter/billing/charges/creditpurchase/featurefilters.goopenmeter/billing/charges/creditpurchase/funded_credit_activity.goopenmeter/billing/creditgrant/errors.goopenmeter/billing/creditgrant/service.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/breakage/adapter/query.goopenmeter/ledger/breakage/adapter/query_test.goopenmeter/ledger/breakage/adapter/record.goopenmeter/ledger/breakage/adapter/record_test.goopenmeter/ledger/breakage/breakage_impacts.goopenmeter/ledger/breakage/service.goopenmeter/ledger/breakage/types.goopenmeter/ledger/customerbalance/expired_loader.goopenmeter/ledger/customerbalance/expired_loader_test.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/feature_filter.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/ledger_loader.goopenmeter/ledger/customerbalance/loaders.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.gotest/credits/creditgrant_test.go
💤 Files with no reviewable changes (2)
- openmeter/billing/creditgrant/service.go
- openmeter/billing/creditgrant/errors.go
GAlexIHU
force-pushed
the
feat/complete-feature-filters
branch
from
June 17, 2026 17:16
8ae6eea to
6cea320
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
openmeter/ledger/breakage/adapter/query.go (2)
85-97: ⚡ Quick winConsider adding a comment explaining MatchFeature semantics.
The OR logic here implements "match unrestricted routes (NULL features) or routes containing this single feature key." While correct, the business intent isn't immediately clear from the code. A brief comment would help future readers understand why we OR with NULL.
📝 Suggested comment
if q.Route.MatchFeature != "" { + // MatchFeature matches unrestricted routes (NULL features) or routes containing the specified feature. predicates = append(predicates, sql.Or( sql.IsNull(routeColumn(ledgersubaccountroutedb.FieldFeatures)),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/breakage/adapter/query.go` around lines 85 - 97, Add a comment above the if statement that checks q.Route.MatchFeature to explain the OR logic semantics. The comment should clarify that the condition matches both unrestricted routes (where FieldFeatures is NULL) and routes that contain the specified MatchFeature value. This will help future readers understand why we OR the IsNull check with the postgresArrayRouteExpression containing the feature key lookup.
68-83: ⚡ Quick winConsider documenting the array equality sorting contract.
The exact-match filter relies on Postgres array equality (
=), which is order-sensitive. The code correctly sorts features here, but this creates an implicit contract that stored route features must also be canonically sorted. A brief comment noting this dependency would help future maintainers understand why the sorting step is essential.📝 Suggested comment
if q.Route.Features.IsPresent() { features, _ := q.Route.Features.Get() + // Sort features for canonical ordering required by Postgres array equality. + // Stored route features must also be sorted for exact-match semantics. features = ledger.SortedFeatures(features)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/breakage/adapter/query.go` around lines 68 - 83, The code sorts features using ledger.SortedFeatures() before constructing a postgresArrayRouteExpression with postgresArrayRouteOperatorEqual, but this critical ordering dependency is not documented. Add a comment above the sorting step explaining that Postgres array equality is order-sensitive and therefore this sorting creates an implicit contract requiring both the query features and stored route features to be canonically sorted in the same order. This will help future maintainers understand why the sorting is essential for correctness.openmeter/ledger/customerbalance/funded_loader.go (2)
95-95: 💤 Low valueError message could include match criteria.
Consider including the currency and amount in this error message to make debugging easier when this condition occurs.
💬 Suggested improvement
- return nil, fmt.Errorf("funded credit transaction group %s has no matching customer FBO transaction", activity.TransactionGroupID) + return nil, fmt.Errorf("funded credit transaction group %s has no matching customer FBO transaction with currency %s and amount %s", activity.TransactionGroupID, activity.Currency, activity.Amount)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/customerbalance/funded_loader.go` at line 95, The error message in the funded_loader.go file that returns an error about "funded credit transaction group has no matching customer FBO transaction" only includes the transaction group ID. Enhance this error message by adding the currency and amount information to the fmt.Errorf call to provide more context for debugging. Include the relevant currency and amount values from the activity object or its associated transaction data in the error message string.
66-96: ⚖️ Poor tradeoffNice cursor resolution logic, with a minor optimization opportunity.
The helper correctly handles the three scenarios (empty TransactionGroupID, matching transaction found, no match). The scanning logic that continues on non-FBO transactions (lines 85-87) is exactly right.
One thing to note: this creates an N+1 query pattern—one
GetTransactionGroupcall per funded activity. Since theLoadmethod is paginated (line 24), you're typically making 20–50 queries per page, which is probably fine for most use cases. If you ever see performance issues with large page sizes, batching the transaction group lookups could be a nice optimization, but it's definitely not blocking anything now.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/customerbalance/funded_loader.go` around lines 66 - 96, The balanceCursorForFundedActivity method calls GetTransactionGroup once per funded activity, creating an N+1 query pattern. To optimize this, consider batching the transaction group lookups when processing multiple funded activities in the Load method. Instead of calling GetTransactionGroup individually for each activity through balanceCursorForFundedActivity, collect all the unique TransactionGroupIDs first, fetch them in a single batch operation (if the Ledger service supports batch retrieval), and then match the results to the activities. This optimization becomes important for larger page sizes and can be implemented when performance becomes a concern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/ledger/breakage/adapter/query.go`:
- Around line 85-97: Add a comment above the if statement that checks
q.Route.MatchFeature to explain the OR logic semantics. The comment should
clarify that the condition matches both unrestricted routes (where FieldFeatures
is NULL) and routes that contain the specified MatchFeature value. This will
help future readers understand why we OR the IsNull check with the
postgresArrayRouteExpression containing the feature key lookup.
- Around line 68-83: The code sorts features using ledger.SortedFeatures()
before constructing a postgresArrayRouteExpression with
postgresArrayRouteOperatorEqual, but this critical ordering dependency is not
documented. Add a comment above the sorting step explaining that Postgres array
equality is order-sensitive and therefore this sorting creates an implicit
contract requiring both the query features and stored route features to be
canonically sorted in the same order. This will help future maintainers
understand why the sorting is essential for correctness.
In `@openmeter/ledger/customerbalance/funded_loader.go`:
- Line 95: The error message in the funded_loader.go file that returns an error
about "funded credit transaction group has no matching customer FBO transaction"
only includes the transaction group ID. Enhance this error message by adding the
currency and amount information to the fmt.Errorf call to provide more context
for debugging. Include the relevant currency and amount values from the activity
object or its associated transaction data in the error message string.
- Around line 66-96: The balanceCursorForFundedActivity method calls
GetTransactionGroup once per funded activity, creating an N+1 query pattern. To
optimize this, consider batching the transaction group lookups when processing
multiple funded activities in the Load method. Instead of calling
GetTransactionGroup individually for each activity through
balanceCursorForFundedActivity, collect all the unique TransactionGroupIDs
first, fetch them in a single batch operation (if the Ledger service supports
batch retrieval), and then match the results to the activities. This
optimization becomes important for larger page sizes and can be implemented when
performance becomes a concern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7565c255-b58a-4a66-8d5a-fa98d04545ab
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (46)
api/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/customers/credits/operations.tspapi/v3/api.gen.goapi/v3/filters/parse.goapi/v3/filters/parse_test.goapi/v3/handlers/customers/credits/feature_filter.goapi/v3/handlers/customers/credits/feature_filter_test.goapi/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/test/filters_test.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.goopenmeter/billing/charges/creditpurchase/charge_test.goopenmeter/billing/charges/creditpurchase/featurefilters.goopenmeter/billing/charges/creditpurchase/funded_credit_activity.goopenmeter/billing/creditgrant/errors.goopenmeter/billing/creditgrant/service.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/breakage/adapter/query.goopenmeter/ledger/breakage/adapter/query_test.goopenmeter/ledger/breakage/adapter/record.goopenmeter/ledger/breakage/adapter/record_test.goopenmeter/ledger/breakage/breakage_impacts.goopenmeter/ledger/breakage/service.goopenmeter/ledger/breakage/types.goopenmeter/ledger/customerbalance/expired_loader.goopenmeter/ledger/customerbalance/expired_loader_test.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/feature_filter.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/ledger_loader.goopenmeter/ledger/customerbalance/loaders.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.goopenmeter/ledger/validations_test.gotest/credits/creditgrant_test.go
💤 Files with no reviewable changes (3)
- openmeter/billing/creditgrant/service.go
- openmeter/billing/creditgrant/errors.go
- api/v3/api.gen.go
🚧 Files skipped from review as they are similar to previous changes (39)
- openmeter/ledger/breakage/adapter/record.go
- openmeter/ledger/customerbalance/ledger_loader.go
- openmeter/ledger/historical/adapter/sumentries_query.go
- openmeter/ledger/breakage/service.go
- openmeter/ledger/breakage/types.go
- api/v3/filters/parse_test.go
- api/v3/handlers/customers/credits/get_balance.go
- api/spec/packages/aip/src/customers/credits/operations.tsp
- openmeter/ledger/account/adapter/subaccount.go
- openmeter/ledger/routing_test.go
- api/spec/packages/aip-client-javascript/src/models/schemas.ts
- openmeter/ledger/breakage/adapter/query_test.go
- openmeter/ledger/customerbalance/noop.go
- api/v3/handlers/customers/credits/list_transactions.go
- openmeter/ledger/customerbalance/expired_loader.go
- api/v3/filters/parse.go
- openmeter/ledger/breakage/breakage_impacts.go
- api/v3/handlers/customers/credits/feature_filter_test.go
- api/v3/test/filters_test.go
- openmeter/billing/charges/creditpurchase/funded_credit_activity.go
- openmeter/billing/charges/creditpurchase/charge_test.go
- openmeter/billing/charges/creditpurchase/featurefilters.go
- openmeter/ledger/primitives.go
- openmeter/ledger/routing.go
- openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.go
- openmeter/ledger/customerbalance/loaders.go
- openmeter/ledger/customerbalance/feature_filter.go
- api/spec/packages/aip-client-javascript/src/models/types.ts
- openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.go
- openmeter/ledger/customerbalance/facade.go
- openmeter/ledger/breakage/adapter/record_test.go
- api/v3/handlers/customers/credits/feature_filter.go
- openmeter/ledger/customerbalance/expired_loader_test.go
- openmeter/ledger/customerbalance/service.go
- openmeter/ledger/customerbalance/testenv_test.go
- openmeter/ledger/customerbalance/service_test.go
- test/credits/creditgrant_test.go
- openmeter/ledger/historical/adapter/ledger_test.go
- openmeter/ledger/historical/adapter/ledger.go
GAlexIHU
force-pushed
the
feat/complete-feature-filters
branch
from
June 17, 2026 17:34
6cea320 to
d4a411a
Compare
turip
previously approved these changes
Jun 19, 2026
| // ValidateAsFeatureFilter validates the singular customer-facing filter form. | ||
| // Credit routes may be restricted to multiple features, but a spendability | ||
| // query can only ask for one feature at a time. | ||
| func (f FeatureFilters) ValidateAsFeatureFilter() error { |
Member
There was a problem hiding this comment.
ValidateAsSingularSpendabilityFilter might be a better name
GAlexIHU
force-pushed
the
feat/complete-feature-filters
branch
from
June 22, 2026 11:34
2a92f27 to
fdc4743
Compare
turip
approved these changes
Jun 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.
Overview
Summary by CodeRabbit
Release Notes
New Features
feature_keyfiltering for customer credit balance and credit transaction listing.MatchFeaturerouting across ledger transactions, expired breakage, and customer balance calculations.Improvements
exists/nexistsfilter parsing to accept explicit boolean values (including empty values) consistently.Bug Fixes
exists=falsebehavior to return only unrestricted results where applicable.Other