fix(api): v3 sort by fields - #4578
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a shared unsupported-sort-field API error, per-resource sort-field converters in v3 handlers, updated list-handler wiring, and matching spec/doc updates for snake_case sort fields. ChangesAPI v3 sort-field handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
tothandras
force-pushed
the
fix/api-sort-by
branch
from
June 26, 2026 10:10
b9c7fa7 to
25b22d4
Compare
Contributor
Greptile SummaryThis PR fixes v3 sort-field handling across the API. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(sdk): generator" | 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.
🧹 Nitpick comments (1)
api/v3/handlers/llmcost/convert.go (1)
207-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNice clean conversion — one tiny dedup idea.
Logic looks spot on, and the docstring explaining why the field is returned unchanged is a lovely touch. 🙂
One small thing: the supported-field list lives in two places (the
caselabels and the variadic args toNewUnsupportedSortFieldError). If someone adds a field later, it's easy to update one and forget the other, leading to a misleading error message. Totally optional, but you could hoist it to a package-level slice and feed both:♻️ Optional dedup
var llmCostSortFields = []string{"id", "provider.id", "model.id", "effective_from", "effective_to"} func FromAPILlmCostSortField(ctx context.Context, field string) (string, error) { if slices.Contains(llmCostSortFields, field) { return field, nil } return "", apierrors.NewUnsupportedSortFieldError(ctx, field, llmCostSortFields...) }Same pattern shows up in the sibling converters, so if you like it, it travels well.
🤖 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/v3/handlers/llmcost/convert.go` around lines 207 - 216, The supported llm cost sort fields are duplicated in FromAPILlmCostSortField, which can cause the switch cases and NewUnsupportedSortFieldError arguments to drift apart. Hoist the supported values into a shared package-level slice or constant used by FromAPILlmCostSortField, then use that same source for both the membership check and the unsupported-field error so the allowed list stays consistent.
🤖 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 `@api/v3/handlers/llmcost/convert.go`:
- Around line 207-216: The supported llm cost sort fields are duplicated in
FromAPILlmCostSortField, which can cause the switch cases and
NewUnsupportedSortFieldError arguments to drift apart. Hoist the supported
values into a shared package-level slice or constant used by
FromAPILlmCostSortField, then use that same source for both the membership check
and the unsupported-field error so the allowed list stays consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64ddb61a-e672-4fcc-a3d2-fc7a651f8695
📒 Files selected for processing (22)
api/v3/apierrors/errors_ctors.goapi/v3/handlers/addons/convert.goapi/v3/handlers/addons/list.goapi/v3/handlers/currencies/convert.goapi/v3/handlers/currencies/list.goapi/v3/handlers/customers/charges/convert.goapi/v3/handlers/customers/charges/list.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/list.goapi/v3/handlers/events/convert.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/list.goapi/v3/handlers/llmcost/convert.goapi/v3/handlers/llmcost/list_prices.goapi/v3/handlers/meters/convert.goapi/v3/handlers/meters/list.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/list.goapi/v3/handlers/subscriptions/convert.goapi/v3/handlers/subscriptions/list.goapi/v3/handlers/subscriptions/subscriptionaddons/convert.goapi/v3/handlers/subscriptions/subscriptionaddons/list.go
turip
previously approved these changes
Jun 26, 2026
turip
previously approved these changes
Jun 26, 2026
tothandras
force-pushed
the
fix/api-sort-by
branch
from
June 26, 2026 11:44
a978009 to
0aeb652
Compare
tothandras
enabled auto-merge (squash)
June 26, 2026 11:44
turip
approved these changes
Jun 26, 2026
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/spec/packages/typespec-typescript/src/utils.tsx (1)
501-520: 🩺 Stability & Availability | 🔵 TrivialHeads up: a
*/inside a doc string will break the emitted comment block.Nice helper! One edge case worth guarding: if a TypeSpec
@docvalue contains*/, it'll close the JSDoc block early and produce invalid TypeScript in the generated SDK. Since this text comes straight from author-controlled docs, escaping it keeps codegen robust.🛡️ One way to escape the sequence
if (!doc) { return undefined } - const trimmed = doc.trim() + const trimmed = doc.trim().replace(/\*\//g, '*\\/') if (!trimmed.includes('\n')) {Just checked the repo—no current
@docstrings contain*/, so this isn't urgent. It's a low-effort defensive tweak to keep things bulletproof for the future.🤖 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/typespec-typescript/src/utils.tsx` around lines 501 - 520, The jsdoc helper can emit invalid TypeScript when a doc string contains the closing sequence */ because it is inserted directly into the JSDoc block. Update jsdoc in utils.tsx to escape or neutralize that sequence before building the comment, including both the single-line and multi-line branches, so generated output from doc values cannot accidentally terminate the block early.
🤖 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 `@api/spec/packages/typespec-typescript/src/utils.tsx`:
- Around line 501-520: The jsdoc helper can emit invalid TypeScript when a doc
string contains the closing sequence */ because it is inserted directly into the
JSDoc block. Update jsdoc in utils.tsx to escape or neutralize that sequence
before building the comment, including both the single-line and multi-line
branches, so generated output from doc values cannot accidentally terminate the
block early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc51414c-16bc-47ee-a607-ea1ccc41abfe
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (38)
api/spec/packages/aip-client-javascript/src/models/operations/addons.tsapi/spec/packages/aip-client-javascript/src/models/operations/currencies.tsapi/spec/packages/aip-client-javascript/src/models/operations/customers.tsapi/spec/packages/aip-client-javascript/src/models/operations/events.tsapi/spec/packages/aip-client-javascript/src/models/operations/features.tsapi/spec/packages/aip-client-javascript/src/models/operations/llmCost.tsapi/spec/packages/aip-client-javascript/src/models/operations/meters.tsapi/spec/packages/aip-client-javascript/src/models/operations/plans.tsapi/spec/packages/aip-client-javascript/src/models/operations/subscriptions.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/meters/operations.tspapi/spec/packages/aip/src/subscriptions/operations.tspapi/spec/packages/typespec-typescript/src/interface-types.tsapi/spec/packages/typespec-typescript/src/request-types.tsapi/spec/packages/typespec-typescript/src/utils.tsxapi/v3/api.gen.goapi/v3/apierrors/errors_ctors.goapi/v3/handlers/addons/convert.goapi/v3/handlers/addons/list.goapi/v3/handlers/currencies/convert.goapi/v3/handlers/currencies/list.goapi/v3/handlers/customers/charges/convert.goapi/v3/handlers/customers/charges/list.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/list.goapi/v3/handlers/events/convert.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/list.goapi/v3/handlers/llmcost/convert.goapi/v3/handlers/llmcost/list_prices.goapi/v3/handlers/meters/convert.goapi/v3/handlers/meters/list.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/list.goapi/v3/handlers/subscriptions/convert.goapi/v3/handlers/subscriptions/list.goapi/v3/handlers/subscriptions/subscriptionaddons/convert.goapi/v3/handlers/subscriptions/subscriptionaddons/list.go
💤 Files with no reviewable changes (1)
- api/v3/api.gen.go
✅ Files skipped from review due to trivial changes (11)
- api/spec/packages/aip-client-javascript/src/models/operations/addons.ts
- api/spec/packages/aip-client-javascript/src/models/operations/plans.ts
- api/spec/packages/aip-client-javascript/src/models/operations/currencies.ts
- api/spec/packages/aip-client-javascript/src/models/operations/meters.ts
- api/spec/packages/aip-client-javascript/src/models/operations/features.ts
- api/spec/packages/aip-client-javascript/src/models/operations/subscriptions.ts
- api/v3/handlers/llmcost/convert.go
- api/spec/packages/aip/src/meters/operations.tsp
- api/spec/packages/aip-client-javascript/src/models/operations/llmCost.ts
- api/spec/packages/aip-client-javascript/src/models/operations/events.ts
- api/spec/packages/aip-client-javascript/src/models/operations/customers.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- api/v3/handlers/features/list.go
- api/v3/handlers/llmcost/list_prices.go
- api/v3/apierrors/errors_ctors.go
- api/v3/handlers/meters/list.go
- api/v3/handlers/subscriptions/convert.go
- api/v3/handlers/customers/charges/convert.go
- api/v3/handlers/customers/list.go
- api/v3/handlers/addons/convert.go
- api/v3/handlers/subscriptions/list.go
- api/v3/handlers/addons/list.go
- api/v3/handlers/meters/convert.go
- api/spec/packages/aip/src/subscriptions/operations.tsp
- api/v3/handlers/currencies/convert.go
- api/v3/handlers/subscriptions/subscriptionaddons/convert.go
- api/v3/handlers/subscriptions/subscriptionaddons/list.go
- api/v3/handlers/currencies/list.go
- api/v3/handlers/customers/charges/list.go
- api/v3/handlers/events/convert.go
- api/v3/handlers/plans/convert.go
- api/v3/handlers/features/convert.go
- api/v3/handlers/plans/list.go
- api/v3/handlers/customers/convert.go
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 by CodeRabbit
sortvalues now return consistent “unsupported sort field” errors that include the supported options, instead of being inconsistently validated or handled.snake_casetimestamp sort attributes (for example,created_at,updated_at).