fix(replay-vision): make scanner and observation order_by a string param#61037
Merged
Conversation
The scanner-list and observation-list `order_by` filters are django-filters `OrderingFilter`s, which drf-spectacular renders as array parameters. The MCP client serializes array query params as a JSON-bracketed string (`order_by=["created_at"]`), which the CSV-style OrderingFilter rejects with "Select a valid choice", so ordering was unusable from the MCP tools. Override the OpenAPI parameter on both list endpoints to a single-value string enum (ascending + `-`-prefixed descending variants), so it serializes as `?order_by=created_at`. The runtime OrderingFilter is unchanged — it already accepts a single value — and the frontend doesn't pass order_by on these endpoints, so the contract change is safe. Multi-field sort via the API is dropped (it was never reachable through the MCP anyway, and the UI sorts one column at a time). The orderable fields are now a single per-filter constant shared by the filter definition and the schema enum so they can't drift. Generated frontend types and MCP tool schemas regenerated via build:openapi. This addresses Replay Vision only; the same MCP-client array-serialization bug affects other products' OrderingFilter tools (e.g. ai_observability) and is tracked separately. Generated-By: PostHog Code Task-Id: fb62321a-5c49-4102-b367-6af893652809
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
assign-reviewers-posthog
Bot
requested review from
a team,
TueHaulund,
arnohillen and
fasyy612
and removed request for
a team
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/replay_vision/backend/api/scanners.py:325
The `_ordering_enum` helper in `observations.py` was introduced specifically to centralise this expansion logic, but `scanners.py` inlines the same comprehension rather than calling it. The two implementations will drift if the pattern ever changes (e.g., adding a `+` ascending prefix). Either move `_ordering_enum` to a shared utilities module, or import and reuse it here.
```suggestion
enum=_ordering_enum(SCANNER_ORDER_FIELDS),
```
Reviews (1): Last reviewed commit: "fix(replay-vision): make scanner and obs..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| str, | ||
| OpenApiParameter.QUERY, | ||
| required=False, | ||
| enum=[value for field in SCANNER_ORDER_FIELDS for value in (field, f"-{field}")], |
Contributor
There was a problem hiding this comment.
_ordering_enum helper in observations.py was introduced specifically to centralise this expansion logic, but scanners.py inlines the same comprehension rather than calling it. The two implementations will drift if the pattern ever changes (e.g., adding a + ascending prefix). Either move _ordering_enum to a shared utilities module, or import and reuse it here.
Suggested change
| enum=[value for field in SCANNER_ORDER_FIELDS for value in (field, f"-{field}")], | |
| enum=_ordering_enum(SCANNER_ORDER_FIELDS), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/replay_vision/backend/api/scanners.py
Line: 325
Comment:
The `_ordering_enum` helper in `observations.py` was introduced specifically to centralise this expansion logic, but `scanners.py` inlines the same comprehension rather than calling it. The two implementations will drift if the pattern ever changes (e.g., adding a `+` ascending prefix). Either move `_ordering_enum` to a shared utilities module, or import and reuse it here.
```suggestion
enum=_ordering_enum(SCANNER_ORDER_FIELDS),
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
|
Size Change: 0 B Total Size: 81 MB ℹ️ View Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No showstoppers. This is a schema fix that corrects order_by from an array to a string type in OpenAPI so the MCP client serializes it correctly. The bot's inline comment about code duplication between the two files is a style nit, not a safety concern.
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
1 participant
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.
Problem
order_byon the Replay Vision scanner-list and observation-list endpoints was unusable from the MCP tools — any value returned a 400:These filters are django-filters
OrderingFilters, which drf-spectacular renders as array parameters. The MCP client serializes array query params as a JSON-bracketed string (order_by=["created_at"]), and the CSV-styleOrderingFilterrejects the bracketed token. (The same MCP-client serialization bug affects other products'OrderingFiltertools, e.g. ai_observability — tracked separately; this PR fixes Replay Vision only.)Changes
Override the OpenAPI
order_byparameter on both list endpoints to a single-value string enum (ascending +--prefixed descending variants) so it serializes as?order_by=created_at:ReplayObservationViewSetandReplayScannerViewSetget an@extend_schema_view(list=...)override.OrderingFilterdefinition and the schema enum, so the two can't drift.OrderingFilterbehavior is unchanged — it already accepts a single value. The frontend doesn't passorder_byon these endpoints (the table sorts client-side), so the contract change is safe. Multi-field sort via the API is dropped — it was never reachable through the MCP, and the UI sorts one column at a time.Generated frontend types and MCP tool schemas were regenerated via
hogli build:openapi(order_byis nowstringinstead ofstring[]).How did you test this code?
I'm an agent (PostHog Code), so automated checks only:
ruff checkon the two backend files — cleanruff, Python format, andty check— all passedhogli build:openapiregenerated cleanly; verifiedorder_byflipped tostringin both the frontendapi.schemas.tsand the MCP zod schemaEnd-to-end MCP verification isn't possible from here — the connected MCP is the deployed production server, which won't carry these changes until released. The fix is the serialization contract; the runtime filter already accepted single-string
order_by.🤖 Agent context
Authored with PostHog Code (Claude) during a Replay Vision dogfooding session. Root cause is a shared MCP-client behavior (
services/mcp/src/api/client.tsJSON-stringifies array query params, added for the logs MCP'sjson.loadsfilters). A systemic client fix was considered but rejected for this PR's scope:OrderingFilter(CSV) vs DRF list filters (repeated keys) need different serializations, so a blanket change is risky and cross-team. The surgical contract change here has zero blast radius to other MCP tools. Requires human review.Created with PostHog Code