Conversation
📝 WalkthroughWalkthroughRefactors LLMCost Price representation: replaces flat string fields for provider and model with nested structured types (Provider/Model) in the spec and generated API, and updates domain-to-API conversion to populate these nested types with provider display-name formatting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v3/handlers/llmcost/convert.go (1)
16-44: Keep the provider label source out of the handler.
Provider.Nameis public API now, but the canonical names live in a handler-local map. That makes new provider additions easy to miss, and the fallback can send rough labels straight to clients. I’d move the display-name helper next to thellmcost.Providerdefinitions, or at least cover it with a small test there. 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 `@api/v3/handlers/llmcost/convert.go` around lines 16 - 44, Move the providerDisplayNames map and formatProviderName function out of the handler and into the llmcost package alongside the Provider type/definitions so canonical display names are owned by the domain model rather than a handler; update code that calls formatProviderName to import the llmcost symbol, and add a small unit test in the llmcost package that asserts known provider IDs map to their canonical names and that the empty-string and fallback capitalization behavior work as expected (referencing providerDisplayNames and formatProviderName).
🤖 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/src/v3/llmcost/prices.tsp`:
- Around line 50-56: The change nests provider and model into objects for
LLMCostPrice (symbols: provider, model, LLMCostPrice) which breaks wire-format
for existing v3 clients; restore backward compatibility by providing a migration
path: accept both old top-level string fields (provider, model_id/model_name)
and the new nested objects during deserialization in LLMCostPrice, preserve
serializing to the existing format for the migration window or gate the new
nested schema behind an explicit version flag/contract change, and add clear
mapping logic between old field names and the new Provider/Model structures so
both old and new clients remain supported.
- Around line 87-109: The public doc comments for the Provider and Model types
include stray closing parentheses that propagate into generated Go comments;
edit the comments in the Provider and Model definitions (the model named
Provider and the model named Model) and remove the extra ')' characters from the
four field descriptions (Provider.id, Provider.name, Model.id, Model.name) so
the sentences read correctly without the stray punctuation.
---
Nitpick comments:
In `@api/v3/handlers/llmcost/convert.go`:
- Around line 16-44: Move the providerDisplayNames map and formatProviderName
function out of the handler and into the llmcost package alongside the Provider
type/definitions so canonical display names are owned by the domain model rather
than a handler; update code that calls formatProviderName to import the llmcost
symbol, and add a small unit test in the llmcost package that asserts known
provider IDs map to their canonical names and that the empty-string and fallback
capitalization behavior work as expected (referencing providerDisplayNames and
formatProviderName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e5c6e74-3206-4b5d-a8d1-9e7f5be5b208
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (3)
api/spec/src/v3/llmcost/prices.tspapi/v3/api.gen.goapi/v3/handlers/llmcost/convert.go
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/v3/api.gen.go`:
- Around line 2399-2406: The API response shape for LLMCostPrice has changed
from flat fields to nested objects (the struct now uses Model LLMCostModel,
Pricing LLMCostModelPricing, Provider LLMCostProvider in api.gen.go), which is a
breaking change; update all public docs/OpenAPI spec, generated client code,
example responses, and any consumer-facing docs to reflect the new nested
structure and note the breaking change, and add a clear migration note
referencing the conversion helper domainPriceToAPI() so integrators know how
server-side conversion is handled and what to expect in responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90e7f9fe-9fd2-4db8-8529-d2f420094aa4
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (2)
api/spec/src/v3/llmcost/prices.tspapi/v3/api.gen.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/spec/src/v3/llmcost/prices.tsp
Model and Provider objects.
Summary by CodeRabbit