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:
📝 WalkthroughWalkthroughRequest parsing and conversion now distinguish unset vs JSON null for feature.unit_cost using nullable tri-state semantics; handlers, converters, validation, and DB adapter were updated to propagate IsNull/IsSpecified behavior and tests were added to cover value/null/omitted cases. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as API Handler
participant Converter as Request Converter
participant Validator as Domain Validator
participant Adapter as DB Adapter
Client->>Handler: PATCH /features/:id (body with unit_cost:null)
Handler->>Handler: json.Decoder.Decode -> preserve nullable unit_cost
Handler->>Converter: convertUpdateRequestToDomain(request)
Converter->>Converter: interpret nullable.UnitCost (IsNull / IsSpecified / Get)
Converter-->>Handler: UpdateFeatureInputs (nullable UnitCost)
Handler->>Validator: inputs.Validate() (may call Get())
Validator-->>Handler: validation result
Handler->>Adapter: UpdateFeature(UpdateFeatureInputs)
Adapter->>Adapter: if UnitCost.IsNull(): clear unit-cost columns
Adapter->>Adapter: if UnitCost.IsSpecified(): set relevant unit-cost columns
Adapter-->>Handler: updated feature
Handler-->>Client: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
openmeter/llmcost/service/service_test.go (2)
33-35: Minor: Tx discards input context.The mock returns a fresh
context.Background()instead of forwarding the input. For these tests it's fine, but if you ever need to test context propagation or cancellation, you'd want to return the input context instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/llmcost/service/service_test.go` around lines 33 - 35, The mockAdapter.Tx implementation discards the incoming context by returning context.Background(); update the Tx method to return the provided ctx parameter instead so context propagation and cancellation are preserved (i.e., change mockAdapter.Tx to return the input context along with noopDriver{} and nil). Ensure you modify the Tx function signature's use of the context parameter so callers receive the original context.
71-85: Consider using a fixed time for determinism.
time.Now()makes tests technically non-deterministic. If timestamp comparisons ever become relevant (e.g., sorting byEffectiveFrom), this could cause flaky tests. A fixed time liketime.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)would make things more predictable.Not urgent since current tests don't depend on time ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/llmcost/service/service_test.go` around lines 71 - 85, In makeTestPrice replace the non-deterministic time.Now() used for EffectiveFrom with a fixed timestamp (e.g., time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)) so tests are deterministic; update the makeTestPrice function in service_test.go to set EffectiveFrom to a constant time value to avoid flakiness when comparing or sorting by EffectiveFrom.openmeter/productcatalog/adapter/feature.go (1)
107-118: Please centralize the unit-cost reset fields.This new clear branch now owns the full unit-cost column list, while the type-switch branches below still own partial clears. A small helper for “clear all unit-cost columns” would make future schema additions much harder to miss.
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/productcatalog/adapter/feature.go` around lines 107 - 118, The clear logic for unit-cost columns is duplicated and scattered; add a single helper that encapsulates all ClearUnitCost* calls (e.g., a method ClearAllUnitCost on the query builder or a local function clearAllUnitCostFields) that chains ClearUnitCostType, ClearUnitCostManualAmount, ClearUnitCostLlmProviderProperty, ClearUnitCostLlmProvider, ClearUnitCostLlmModelProperty, ClearUnitCostLlmModel, ClearUnitCostLlmTokenTypeProperty, and ClearUnitCostLlmTokenType; then replace the long ClearUnitCost branch and any partial clears in the type-switch branches to call that helper (look for the ClearUnitCost sequence in the function containing the input.ClearUnitCost check and the subsequent type-switch) so future schema additions require updating only one place.api/v3/handlers/features/convert_test.go (1)
359-391: Please add one raw-handler test forunit_cost: null.These cases start after
clearUnitCostis already computed, so they won’t catch a regression inapi/v3/handlers/features/update.gowhere omitted vs explicitnullis detected from the raw JSON body. A small handler test with both payloads would lock down the actual bug-fix path.As per coding guidelines, "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert_test.go` around lines 359 - 391, Add a raw-handler test that posts JSON directly to the feature update HTTP handler (the endpoint that ultimately calls convertUpdateRequestToDomain) to verify explicit null vs omitted unit_cost are handled differently: send two requests—one with {"unit_cost": null} and one with an otherwise-identical body that omits unit_cost—and assert the handler/converted domain result sets ClearUnitCost true and UnitCost nil for the explicit null case, and ClearUnitCost false (UnitCost nil) for the omitted case; use the same test harness used by existing handler tests to construct requests and inspect the resulting domain object so this regression is exercised end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/features/convert_test.go`:
- Around line 359-391: Add a raw-handler test that posts JSON directly to the
feature update HTTP handler (the endpoint that ultimately calls
convertUpdateRequestToDomain) to verify explicit null vs omitted unit_cost are
handled differently: send two requests—one with {"unit_cost": null} and one with
an otherwise-identical body that omits unit_cost—and assert the
handler/converted domain result sets ClearUnitCost true and UnitCost nil for the
explicit null case, and ClearUnitCost false (UnitCost nil) for the omitted case;
use the same test harness used by existing handler tests to construct requests
and inspect the resulting domain object so this regression is exercised
end-to-end.
In `@openmeter/llmcost/service/service_test.go`:
- Around line 33-35: The mockAdapter.Tx implementation discards the incoming
context by returning context.Background(); update the Tx method to return the
provided ctx parameter instead so context propagation and cancellation are
preserved (i.e., change mockAdapter.Tx to return the input context along with
noopDriver{} and nil). Ensure you modify the Tx function signature's use of the
context parameter so callers receive the original context.
- Around line 71-85: In makeTestPrice replace the non-deterministic time.Now()
used for EffectiveFrom with a fixed timestamp (e.g., time.Date(2024, 1, 1, 0, 0,
0, 0, time.UTC)) so tests are deterministic; update the makeTestPrice function
in service_test.go to set EffectiveFrom to a constant time value to avoid
flakiness when comparing or sorting by EffectiveFrom.
In `@openmeter/productcatalog/adapter/feature.go`:
- Around line 107-118: The clear logic for unit-cost columns is duplicated and
scattered; add a single helper that encapsulates all ClearUnitCost* calls (e.g.,
a method ClearAllUnitCost on the query builder or a local function
clearAllUnitCostFields) that chains ClearUnitCostType,
ClearUnitCostManualAmount, ClearUnitCostLlmProviderProperty,
ClearUnitCostLlmProvider, ClearUnitCostLlmModelProperty, ClearUnitCostLlmModel,
ClearUnitCostLlmTokenTypeProperty, and ClearUnitCostLlmTokenType; then replace
the long ClearUnitCost branch and any partial clears in the type-switch branches
to call that helper (look for the ClearUnitCost sequence in the function
containing the input.ClearUnitCost check and the subsequent type-switch) so
future schema additions require updating only one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65309550-a879-4406-8756-b013d15f12e8
📒 Files selected for processing (9)
api/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goapi/v3/handlers/features/update.goopenmeter/llmcost/service/service.goopenmeter/llmcost/service/service_test.goopenmeter/productcatalog/adapter/feature.goopenmeter/productcatalog/adapter/feature_test.goopenmeter/productcatalog/feature/connector.goopenmeter/productcatalog/feature/connector_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/spec/packages/aip/src/features/feature.tsp (1)
60-61: Please document thenull-clears behavior explicitly.Line 61 now supports an important PATCH semantic, but the field docs still read like a normal optional field. A one-liner saying "
unit_cost: nullclears existing unit cost" would make the contract much clearer for clients.Suggested doc tweak
model FeatureUpdateRequest { /** * Optional per-unit cost configuration. * Use "manual" for a fixed per-unit cost, or "llm" to look up cost * from the LLM cost database based on meter group-by properties. + * Set to `null` to clear an existing unit cost. */ `#suppress` "@openmeter/api-spec-aip/no-nullable" "PATCH operation" `@summary`("Unit cost") unit_cost?: FeatureUnitCost | null; }As per coding guidelines, "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/features/feature.tsp` around lines 60 - 61, The field documentation for unit_cost (type FeatureUnitCost | null) lacks the PATCH semantic that sending unit_cost: null clears the existing unit cost; update the JSDoc/summary for unit_cost to explicitly state that setting unit_cost to null in a PATCH request will clear/remove the stored unit cost so clients understand the null-clears behavior (refer to the unit_cost field and FeatureUnitCost type to ensure wording matches existing types).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/spec/packages/aip/src/features/feature.tsp`:
- Around line 60-61: The field documentation for unit_cost (type FeatureUnitCost
| null) lacks the PATCH semantic that sending unit_cost: null clears the
existing unit cost; update the JSDoc/summary for unit_cost to explicitly state
that setting unit_cost to null in a PATCH request will clear/remove the stored
unit cost so clients understand the null-clears behavior (refer to the unit_cost
field and FeatureUnitCost type to ensure wording matches existing types).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12fbf636-238d-44dd-8dfd-032176a1d432
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (2)
api/spec/packages/aip/src/features/feature.tspapi/v3/api.gen.go
tothandras
requested changes
Mar 31, 2026
tothandras
approved these changes
Apr 1, 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.
?sourcefilter on ListPrices API, the namespace override overlay was unconditionally replacing global prices with manual overrides, even when the caller filtered by?source=system. Now the overlay is skipped when the source filter excludes manual prices (e.g.source=systemorsource≠manual).Summary by CodeRabbit
New Features
Improvements
Tests