feat(api): add usage email filters and members endpoint#4211
feat(api): add usage email filters and members endpoint#4211evanjacobson wants to merge 9 commits into
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous Review Summaries (7 snapshots, latest commit e022ad4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e022ad4)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 4db69ed)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (2 files)
Previous review (commit e2e18a2)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (2 files)
Previous review (commit 52c4530)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (2 files)
Previous review (commit ce4959d)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (3 files)
Previous review (commit 62c590f)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (2 files)
Previous review (commit d4343ec)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (6 files)
Reviewed by gpt-5.4-20260305 · Input: 69.1K · Output: 7.2K · Cached: 321.6K Review guidance: REVIEW.md from base branch |
| timeseries: rows.map(row => { | ||
| const rawLabel = toStringValue(row[2]); | ||
| const label = input.splitBy | ||
| ? dimensionDisplayValue(input.splitBy, filters, userEmailsById, rawLabel) |
There was a problem hiding this comment.
Moderate, Functional: The email mapping correctly avoids exposing raw identifiers, but it happens after Snowflake has grouped the timeseries by raw kilo_user_id. If one person has usage under both their canonical ID and a legacy oauth/... ID, the response contains two points with the same timestamp and email instead of one combined point. Clients may render duplicate points or overwrite one of them, which makes the visible total incomplete. Could we join the scoped email map before aggregation and group by the mapped email and time bucket, similar to the breakdown query? A test with canonical and OAuth rows for the same email and timestamp would cover this case.
| const raw = row[dimIndexMap[d]]; | ||
| dimensions[d] = typeof raw === 'string' ? raw : ''; | ||
| const value = toStringValue(raw); | ||
| dimensions[d] = dimensionDisplayValue(d, filters, userEmailsById, value); |
There was a problem hiding this comment.
Moderate, Functional: Mapping the returned user dimension to email avoids exposing raw identifiers, but the table query still groups and applies LIMIT using raw kilo_user_id values. A person with usage under both a canonical ID and a legacy oauth/... ID can therefore produce duplicate rows with the same visible dimensions. Those duplicate identity rows can also consume the limit and exclude rows for other users. Could we join the scoped email map in this query, group by the mapped email with the other requested dimensions, and apply the limit after that aggregation? Tests covering identity aggregation and a constrained limit would help preserve the expected table semantics.
| }, | ||
| }, | ||
| }, | ||
| '500': { |
There was a problem hiding this comment.
Low, Documentation: The shared REST operation documents the common error responses, but this members endpoint can also return HTTP 404. organizations.withMembers raises NOT_FOUND when the organization does not exist, and handleTRPCRequest preserves that status. Without a documented 404, generated clients and API consumers may treat a valid endpoint response as unexpected. Could the REST route metadata support route specific responses and add a 404 using RestErrorResponseSchema for this endpoint? An OpenAPI assertion for the members operation would keep the contract in sync.
|
Why did you not add this as a tRPC endpoint, like the rest? I'm not sure it makes sense to have some endpoints in the API docs as tRPC and then some others as REST. |
Summary
External API consumers previously had to rely on internal user IDs and did not have a documented way to discover organization members for filtering usage analytics. This adds email-oriented usage filtering/display, a public organization members endpoint, and OpenAPI/Swagger coverage so clients can build against the API without depending on private UI assumptions.
Get Members API: /api/v1/organizations//members
Details
Verification
No manual UI verification; this is API/schema behavior with no visual interaction changes.
Visual Changes
N/A
Reviewer Notes
Focus review on email-to-user identity scoping for usage analytics, the public member response PII boundary, and the OpenAPI shape for mixed tRPC/REST paths.