Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a ProductCatalog TypeSpec module (pricing primitives, price discriminated unions and tiers, payment terms, spend commitments) and unit conversion/rounding models with invoice quantity detail. The OpenMeter entrypoint now imports the ProductCatalog module so its types are included in the generated API spec. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
rolosp
force-pushed
the
feat/productcatalog-v3-price
branch
from
d0a15f5 to
3c90261
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: 1
🤖 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/productcatalog/price.tsp`:
- Around line 190-214: The PriceTier TypeSpec model currently allows
up_to_amount, flat_price, and unit_price to all be optional even though the
server enforces "At least one price component (flat_price or unit_price) must be
set" in Validate() (openmeter/productcatalog/price.go::Validate); update the
PriceTier doc comment to explicitly state this invariant is enforced server-side
and must be validated by the service (mentioning Validate()), and if desired add
a custom TypeSpec decorator or flag to surface this constraint in generated
schemas; ensure no code changes remove the existing Validate() logic and that
consumers are directed to rely on server-side validation for the one-of
requirement between flat_price and unit_price.
🪄 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: 4434bf59-5cf6-46b2-abe4-4151107a12fc
📒 Files selected for processing (4)
api/spec/packages/aip/src/openmeter.tspapi/spec/packages/aip/src/productcatalog/index.tspapi/spec/packages/aip/src/productcatalog/price.tspapi/spec/packages/aip/src/productcatalog/unitconfig.tsp
Comment on lines
+190
to
+214
| model PriceTier { | ||
| /** | ||
| * Up to and including this quantity will be contained in the tier. | ||
| * If undefined, the tier is open-ended (the last tier). | ||
| */ | ||
| @visibility(Lifecycle.Read, Lifecycle.Create, Lifecycle.Update) | ||
| @summary("Up to quantity") | ||
| up_to_amount?: Shared.Numeric; | ||
|
|
||
| /** | ||
| * The flat price component of the tier. | ||
| * Charged once when the tier is entered. | ||
| */ | ||
| @visibility(Lifecycle.Read, Lifecycle.Create, Lifecycle.Update) | ||
| @summary("Flat price component") | ||
| flat_price?: PriceFlat; | ||
|
|
||
| /** | ||
| * The unit price component of the tier. | ||
| * Charged per billing unit within the tier. | ||
| */ | ||
| @visibility(Lifecycle.Read, Lifecycle.Create, Lifecycle.Update) | ||
| @summary("Unit price component") | ||
| unit_price?: PriceUnit; | ||
| } |
Contributor
There was a problem hiding this comment.
PriceTier validation rule not enforced in schema.
The doc comment on line 185 correctly states "At least one price component (flat_price or unit_price) must be set", and the Go implementation (openmeter/productcatalog/price.go:749-750) enforces this in Validate(). However, the TypeSpec model allows all three fields to be optional without any constraint.
TypeSpec doesn't have a native "oneOf required" decorator, so this invariant will need server-side validation. Just flagging so it's on the radar - might be worth adding a more prominent note in the doc string that this is a server-enforced constraint, or exploring if a custom decorator could help here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/packages/aip/src/productcatalog/price.tsp` around lines 190 - 214,
The PriceTier TypeSpec model currently allows up_to_amount, flat_price, and
unit_price to all be optional even though the server enforces "At least one
price component (flat_price or unit_price) must be set" in Validate()
(openmeter/productcatalog/price.go::Validate); update the PriceTier doc comment
to explicitly state this invariant is enforced server-side and must be validated
by the service (mentioning Validate()), and if desired add a custom TypeSpec
decorator or flag to surface this constraint in generated schemas; ensure no
code changes remove the existing Validate() logic and that consumers are
directed to rely on server-side validation for the one-of requirement between
flat_price and unit_price.
tothandras
requested changes
Mar 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/spec/packages/aip/src/productcatalog/price.tsp (1)
52-56: Consider adding@summarydecorators for consistency.The
Priceunion above has@summarydecorators on each member (lines 28-41), butPriceUsageBaseddoesn't. Adding them would keep the generated docs consistent across both unions.✨ Suggested improvement
union PriceUsageBased { + `@summary`("Unit price") unit: PriceUnit, + `@summary`("Graduated price") graduated: PriceGraduated, + `@summary`("Volume price") volume: PriceVolume, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/productcatalog/price.tsp` around lines 52 - 56, The PriceUsageBased union is missing `@summary` decorators on its members (unit, graduated, volume); add matching `@summary` annotations for each variant in the PriceUsageBased union (same style/phrasing used in the Price union's members) so generated docs remain consistent across both unions.
🤖 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/productcatalog/price.tsp`:
- Around line 52-56: The PriceUsageBased union is missing `@summary` decorators on
its members (unit, graduated, volume); add matching `@summary` annotations for
each variant in the PriceUsageBased union (same style/phrasing used in the Price
union's members) so generated docs remain consistent across both unions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd60ebed-beef-45ab-bbcd-c24f7d6a2558
📒 Files selected for processing (1)
api/spec/packages/aip/src/productcatalog/price.tsp
rolosp
force-pushed
the
feat/productcatalog-v3-price
branch
from
f66d7f2 to
3a2f4a6
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/spec/packages/aip/src/productcatalog/price.tsp (2)
52-56: Consider adding@summarydecorators for consistency.The
Priceunion (lines 28-41) includes@summarydecorators on each variant, butPriceUsageBaseddoesn't. For consistent API documentation, you might want to add summaries here too:📝 Suggested improvement
union PriceUsageBased { + `@summary`("Unit price") unit: PriceUnit, + `@summary`("Graduated price") graduated: PriceGraduated, + `@summary`("Volume price") volume: PriceVolume, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/productcatalog/price.tsp` around lines 52 - 56, Add `@summary` decorators to each variant of the PriceUsageBased union to match the existing style used in the Price union: annotate the unit, graduated, and volume variants in PriceUsageBased with concise `@summary` strings describing each variant (e.g., "Per-unit pricing", "Graduated pricing by tiers", "Volume-based pricing") so documentation is consistent; update the PriceUsageBased union declaration to include these `@summary` decorators for unit, graduated, and volume.
221-240: Consider documenting the min ≤ max constraint.When both
minimum_amountandmaximum_amountare set, there's an implicit business rule that minimum should not exceed maximum. This will need server-side validation - might be worth mentioning in the doc comment so API consumers know what to expect.📝 Suggested doc improvement
/** * Spend commitments for a rate card. * The customer is committed to spend at least the minimum amount and at most the maximum amount. + * + * When both are specified, minimum_amount must not exceed maximum_amount (validated server-side). */ `@friendlyName`("BillingSpendCommitments") model SpendCommitments {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/productcatalog/price.tsp` around lines 221 - 240, Add a clear note in the SpendCommitments model doc comment that when both minimum_amount and maximum_amount are provided the invariant minimum_amount ≤ maximum_amount must hold, and implement server-side validation enforcing this rule (e.g., in the create/update handlers or validation layer that processes SpendCommitments) to return a descriptive error when the constraint is violated; reference the SpendCommitments model and the minimum_amount and maximum_amount fields so consumers and implementers see the requirement.
🤖 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/productcatalog/price.tsp`:
- Around line 52-56: Add `@summary` decorators to each variant of the
PriceUsageBased union to match the existing style used in the Price union:
annotate the unit, graduated, and volume variants in PriceUsageBased with
concise `@summary` strings describing each variant (e.g., "Per-unit pricing",
"Graduated pricing by tiers", "Volume-based pricing") so documentation is
consistent; update the PriceUsageBased union declaration to include these
`@summary` decorators for unit, graduated, and volume.
- Around line 221-240: Add a clear note in the SpendCommitments model doc
comment that when both minimum_amount and maximum_amount are provided the
invariant minimum_amount ≤ maximum_amount must hold, and implement server-side
validation enforcing this rule (e.g., in the create/update handlers or
validation layer that processes SpendCommitments) to return a descriptive error
when the constraint is violated; reference the SpendCommitments model and the
minimum_amount and maximum_amount fields so consumers and implementers see the
requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2531349-76bd-4049-b498-0cbc61dfac91
📒 Files selected for processing (4)
api/spec/packages/aip/src/openmeter.tspapi/spec/packages/aip/src/productcatalog/index.tspapi/spec/packages/aip/src/productcatalog/price.tspapi/spec/packages/aip/src/productcatalog/unitconfig.tsp
✅ Files skipped from review due to trivial changes (2)
- api/spec/packages/aip/src/openmeter.tsp
- api/spec/packages/aip/src/productcatalog/index.tsp
🚧 Files skipped from review as they are similar to previous changes (1)
- api/spec/packages/aip/src/productcatalog/unitconfig.tsp
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
Adds price and unit-config types to the productcatalog package
as a preparatory step toward the full product catalog API (plans, addons, rate
cards).
No new API surface is introduced (the types are not yet reachable)
Summary by CodeRabbit