feat: operations overview #2331
Conversation
…ate related logic in analytics
… states in client usage and deprecated fields tables
JivusAyrus
requested review from
Aenimus,
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
WalkthroughAdds two RPCs (GetOperationClients, GetOperationDeprecatedFields), extends operations proto with paging/sorting/filtering and a metric oneof, implements server analytics handlers and repository queries, adds tests, and ships a new Operations UI (hooks, components, page) plus a ScrollArea UI primitive and layout wiring. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
|
Adding tests |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proto/wg/cosmo/platform/v1/platform.proto (1)
2882-2926: Critical: Field number reassignments in GetOperationsResponse.Operation break wire compatibilityThe proto file shows a wire-incompatible change. Between commit
a494f01b~1anda494f01b, field numbers inGetOperationsResponse.Operationwere reassigned:
- Field 4: was
float latency→ nowOperationType type(enum)- Field 5: was
OperationType type→ nowbool hasDeprecatedFields- Fields 6–8:
latencymoved to 6;requestCountanderrorPercentageadded at 7–8Older clients compiled against the previous proto will receive field 4 as an enum but expect it to be a float (latency). This causes data misinterpretation on the wire and breaks compatibility. Protobuf best practice requires never reassigning field numbers—instead, new fields must use fresh, previously unused tags, and deprecated fields should be marked as reserved rather than reused.
If this has been released, affected clients will experience silent deserialization errors or data corruption. Restore the old field numbers (e.g., keep
latencyat 4,typeat 5) and place new fields at previously unused numbers, or issue a breaking change notice if reverting is not viable.
♻️ Duplicate comments (1)
studio/src/components/operations/operations-list.tsx (1)
77-92: EscapesearchQuerybefore buildingRegExpto avoid regex injection and runtime errors
highlightTextcurrently does:const regex = new RegExp(`(${query})`, "gi");with
querycoming from user input. This can:
- Throw on invalid patterns (e.g. unmatched
(,\).- Enable user-controlled regex features (potential ReDoS).
- Matches the CodeQL “regular expression injection” finding.
If you only need literal substring highlighting, escape the query (or avoid
RegExpentirely), e.g.:- const highlightText = (text: string, query: string) => { - if (!query) return text; - - const regex = new RegExp(`(${query})`, "gi"); + const highlightText = (text: string, query: string) => { + if (!query) return text; + + const escapeRegExp = (value: string) => + value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const regex = new RegExp(`(${escapeRegExp(query)})`, "gi");This keeps the UX while removing the injection vector and hardening against malformed input.
🧹 Nitpick comments (17)
studio/src/components/analytics/metrics.tsx (1)
868-891: LGTM! Consistency refactor.The refactoring to destructured props with explicit types improves consistency with other components in the file and enhances maintainability.
Optional: Consider tightening the
seriestype.The
series: any[]type could be more specific. Based on theuseChartDatahook usage and the data structure (containing p50, p90, p99 properties), consider defining a more specific interface for the series items.interface LatencySeriesItem { timestamp: number | string | Date; p50?: number | string; p90?: number | string; p99?: number | string; }Then use:
series: LatencySeriesItem[]This would improve type safety and make the expected data structure clearer.
studio/src/components/layout/graph-layout.tsx (1)
235-245: GraphPageLayout className extension is good; consider fixingjustify-endcontainerExtending
TitleLayoutPropswithclassNameand piping it throughcn(...)on the header container is a nice touch. One small nit: the items wrapper usesclassName="justify-end"without a flex or grid display, so that utility currently has no effect. If the intent is to right-align theitemscontent, consider making it a flex container:- <div className="justify-end">{items}</div> + <div className="flex justify-end">{items}</div>This keeps behavior explicit without affecting existing layouts.
Also applies to: 247-283
controlplane/src/core/bufservices/analytics/getOperations.ts (3)
49-52: Pagination, sorting, and date-range handling look good; a couple of minor nits
- Defaulting
limitandoffset, validating bounds, and rejecting negative offsets is a solid guardrail for the analytics backend.- Mapping
fetchBasedOn→requests/errors/latencywith a default to latency keeps behavior backward compatible if the client doesn’t set the new enum.- Using
analytics-retentionviaOrganizationRepositorytogether withvalidateDateRangesis a good way to cap the lookback window without surprising callers.Two small follow-ups to consider:
- Given
inputRangefalls back todeafultRangeInHoursForGetOperationswhen bothreq.rangeandreq.dateRangeare unset, theif (!range && !dateRange)branch is effectively unreachable; you could either remove it or make it explicitly guard only truly invalid combinations.- When
shouldFetchAllis true, you still passlimit: req.limitalongsidefetchAll: true. IfMetricsRepository.getOperationsdoesn’t already ignorelimitwhenfetchAllis set, it might be safer to have the service passlimit: 0orundefinedin that case to avoid accidentally truncating results before the in-memory filtering/pagination.Also applies to: 64-87, 88-119, 156-176
123-151: Deprecated-fields computation and filtering are coherent; consider when it should runUsing the latest valid schema +
SchemaGraphPruner.getAllFields({ onlyDeprecated: true })and thenUsageRepository.getOperationsUsingDeprecatedFieldsto derive a(hash,name)set is a clean way to drive both thehasDeprecatedFieldsflag and the deprecated-only filtering behavior. The Set-based lookup and in-memory pagination whenincludeDeprecatedFieldsis true are also straightforward.If this endpoint is hit very frequently, you might want to double-check whether you truly need
hasDeprecatedFieldson every response or only when the client explicitly asks for deprecated-field information. If it’s the latter, you could gate the schema build and usage query behind something likereq.includeDeprecatedFields || req.includeOperationsWithDeprecatedFieldsOnlyto avoid extra work on simple “top operations” views.Also applies to: 187-215, 233-235
217-270: Response mapping to GetOperationsResponse.Operation is mostly spot onThe construction of
operationData(name/hash/type/content/hasDeprecatedFields) and the metriconeofbased onfetchBasedOnlines up with the updated response shape and keeps only the relevant metric populated, which should simplify consumers.One minor robustness tweak you could make is to default the latency value in the fallback branch, e.g.
value: operation.latency ?? 0, to avoid propagatingundefinedif the repository ever omits latency for a row.proto/wg/cosmo/platform/v1/platform.proto (1)
2941-2959: New per-operation clients/deprecated-fields RPC surface looks consistentThe new
GetOperationClients*andGetOperationDeprecatedFields*messages are shaped consistently with the rest of the analytics API (hash + name plus optional range/dateRange), and the corresponding RPCs onPlatformServiceclearly separate these concerns from the bulkGetOperationscall.Since these endpoints are strictly read-only analytics, you may want to align them with other analytics RPCs like
GetAnalyticsView/GetGraphMetricsby marking them withoption idempotency_level = NO_SIDE_EFFECTS;to give clients and intermediaries a stronger correctness signal about caching/retries.Also applies to: 2961-2978, 3389-3392
controlplane/src/core/bufservices/analytics/getOperationContent.ts (1)
46-49: Escaping hash and name looks good; consider centralizing this helperThe single‑quote escaping for
hashand optionalnameis correct and consistent with other analytics queries. Since this pattern is repeated across handlers, you might later extract a small shared helper to keep the escaping logic in one place.Also applies to: 55-56
studio/src/components/operations/operations-search.tsx (1)
70-99: Deprecated‑fields filter wiring is correct; small type/ergonomic tweaks possibleThe deprecated‑fields filter is correctly hooked into
AnalyticsFiltersviafiltersListandhandleDeprecatedFieldsFilterSelect. To simplify and avoid theas unknown as stringcast, you could pass a plain string operator that matches whatoptionConstructorexpects, e.g.:- operator: AnalyticsViewFilterOperator.EQUALS as unknown as string, + operator: "EQUALS",This keeps the filter semantics the same while avoiding a noisy cast.
controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
305-454: Avoid ad‑hoc SQL string concatenation fordeprecatedFieldsArrayThe new methods correctly guard on
deprecatedFields.length === 0and reuseparseTimeFilters, butdeprecatedFieldsArrayis built via raw string interpolation offield.nameandfield.typeNamesinto the query. If any of these values ever contain a'or other odd characters (or become more user-controlled over time), this risks malformed SQL and makes the query harder to reason about.Consider centralizing escaping or parameterization here, similar to how other analytics code uses query parameters:
- const deprecatedFieldsArray = deprecatedFields - .map((field) => `('${field.name}', [${field.typeNames.map((tn) => `'${tn}'`).join(', ')}])`) - .join(', '); + const escape = (v: string) => v.replace(/'/g, "''"); + const deprecatedFieldsArray = deprecatedFields + .map( + (field) => + `('${escape(field.name)}', [${field.typeNames + .map((tn) => `'${escape(tn)}'`) + .join(', ')}])`, + ) + .join(', ');Or, if feasible, move this into a shared helper and/or use ClickHouse parameters instead of interpolated literals.
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
722-860: Tighten typing/validation forfetchBasedOnandsortDirectionused in SQLThe
getOperationslogic is sound, butfetchBasedOn/sortFieldandsortDirectionare plainstrings while they directly influence which query branch runs and theORDER BYclause:const sortField = props.fetchBasedOn || 'latency'; const sortDirection = props.sortDirection || 'desc'; … ORDER BY latency ${sortDirection.toUpperCase()} …To prevent accidental invalid values and keep the SQL predictable, it would be safer to narrow these at the type and runtime level, e.g.:
- fetchBasedOn?: string; // 'requests', 'latency', or 'errors' - sortDirection?: string; // 'asc' or 'desc' + fetchBasedOn?: 'requests' | 'latency' | 'errors'; + sortDirection?: 'asc' | 'desc'; … - const sortField = props.fetchBasedOn || 'latency'; - const sortDirection = props.sortDirection || 'desc'; + const sortField: 'requests' | 'latency' | 'errors' = + props.fetchBasedOn ?? 'latency'; + const sortDirection: 'asc' | 'desc' = + props.sortDirection === 'asc' ? 'asc' : 'desc';This keeps the branching explicit and guards the
ORDER BYclause from unexpected tokens if these props are ever passed from less trusted layers.studio/src/components/operations/operations-list.tsx (1)
23-34: AlignselectedOperationtyping with null/undefined handlingThe selection logic explicitly handles
selectedOperation?.namebeingnullorundefined:selectedOperation?.name === null || selectedOperation?.name === undefined ? true : selectedOperation?.name === operationName;But
OperationsListPropsdeclares:selectedOperation: { hash: string; name: string; } | undefined;So callers can’t actually pass
nullfornamewithout breaking the type. To make this consistent and self-documenting, either:
- Allow nullish in the type:
- selectedOperation: { - hash: string; - name: string; - } | undefined; + selectedOperation: + | { + hash: string; + name: string | null | undefined; + } + | undefined;or
- Simplify the selection logic to treat
''as the “unnamed” case and remove the null checks, depending on how upstream data is modeled.This avoids surprises at call sites and keeps the selection semantics explicit.
Also applies to: 255-268
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
22614-22671: ExtendedGetOperationsRequestfilters are wired consistently, but watch default handlingThe added pagination/filtering fields (
offset,includeDeprecatedFields,fetchBasedOn,sortDirection,includeContent,range,dateRange,searchQuery,includeOperationsWithDeprecatedFieldsOnly) are all present both as properties and in thefieldslist with matching numbers and kinds, which looks correct.One thing to double‑check: comments like “defaults to true/desc” aren’t enforced in this generated class (the properties are
undefinedby default). Make sure callers or the server-side logic consistently apply those defaults when these fields are omitted.Also applies to: 22682-22693
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
534-535: Review query cache configuration.Setting
staleTime: 0andgcTime: 0means data is immediately stale and garbage collected when unmounted. This can cause:
- Excessive refetching when navigating back to this page
- Loss of cached data benefits
- Potential performance issues
This conflicts with
placeholderData: keepPreviousDataon line 530, which tries to keep previous data visible during refetches.Consider adjusting these values unless immediate staleness is required:
- staleTime: 0, - gcTime: 0, + staleTime: 30_000, // 30 seconds + gcTime: 5 * 60_000, // 5 minutes
520-521: Consider implementing pagination.The TODO comment indicates pagination support is needed. With a fixed
limit: 100, users cannot view more than 100 operations. This could be limiting for large-scale applications.Would you like me to help implement pagination with URL-based state management that aligns with the existing analytics query state pattern?
studio/src/components/operations/deprecated-fields-table.tsx (1)
38-69: Handle error state and dateRange robustness in deprecated fields queryThe query wiring and mapping look correct, but two small points worth tightening:
dateRangeis dereferenced whenrangeis falsy; ifuseAnalyticsQueryStateever returnsrangefalsy with an undefineddateRange, this will throw. A defensive null check (or asserting both are always set in the hook) would make this safer.- You already destructure
errorfromuseQuery, but the table treats “error” and “no data” the same (“No deprecated fields found”). Surfacing an error state (similar toOperationContentModal) would avoid silently hiding backend/transport issues.studio/src/hooks/use-operations-filters.tsx (1)
28-57: Consider usingrouter.replaceand explicit pathname when updating query params
applyNewParamscurrently does:router.push({ query: { ...q, ...cleanedNewParams, }, });This implicitly reuses the current pathname and adds a new history entry for every filter/sort change. For an analytics view with lots of filter tweaks, you might prefer:
- Using
router.replaceto avoid bloating browser history.- Explicitly setting
pathname: router.pathnamefor clarity.This is optional, but can improve UX when users navigate back/forward through pages.
Also applies to: 166-199
controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
21-71: Server-side operation client analytics look sound; optionally tighten range validationThis handler is well-aligned with existing analytics patterns:
- Auth + org scoping is done before repository usage, and
FederatedGraphRepository.byNameensures graph access is organization-scoped (good fit with previous service-layer auth learnings). Based on learnings.- Date range handling (
inputRange,validateDateRanges,isoDateRangeToTimestamps,getDateRange) gives a sensible default (24h) when neitherrangenordateRangeis provided and respects theanalytics-retentionlimit.- SQL uses explicit escaping for
OperationHashandOperationName, and the result mapping toBigIntrequestCountand ISOlastUsedlooks correct.If you want to be extra defensive, you could:
- Reject or coerce negative/zero
rangevalues before callingvalidateDateRanges, to avoid surprising windows.- Document the implicit 24h default (or move it into
validateDateRangesfor centralization).These are optional hardening tweaks; the current implementation is functionally solid.
Also applies to: 73-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (22)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_connect.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(8 hunks)controlplane/src/core/bufservices/PlatformService.ts(2 hunks)controlplane/src/core/bufservices/analytics/getOperationClients.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperationContent.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperations.ts(6 hunks)controlplane/src/core/repositories/analytics/MetricsRepository.ts(1 hunks)controlplane/src/core/repositories/analytics/UsageRepository.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(5 hunks)studio/src/components/analytics/field-usage.tsx(1 hunks)studio/src/components/analytics/metrics.tsx(15 hunks)studio/src/components/layout/graph-layout.tsx(6 hunks)studio/src/components/operations/client-usage-table.tsx(1 hunks)studio/src/components/operations/deprecated-fields-table.tsx(1 hunks)studio/src/components/operations/operation-content-modal.tsx(1 hunks)studio/src/components/operations/operations-list.tsx(1 hunks)studio/src/components/operations/operations-search.tsx(1 hunks)studio/src/components/ui/scroll-area.tsx(1 hunks)studio/src/hooks/use-operations-filters.tsx(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
controlplane/src/core/bufservices/analytics/getOperationContent.ts
📚 Learning: 2025-07-30T09:29:04.257Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
Applied to files:
controlplane/src/core/bufservices/analytics/getOperationContent.ts
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
controlplane/src/core/bufservices/analytics/getOperationContent.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tscontrolplane/src/core/bufservices/analytics/getOperations.ts
🧬 Code graph analysis (19)
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (9)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationDeprecatedFields(2640-2649)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
GetOperationDeprecatedFieldsRequest(23164-23228)GetOperationDeprecatedFieldsResponse(23233-23271)controlplane/src/core/util.ts (4)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)validateDateRanges(346-373)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(96-1835)controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
UsageRepository(12-455)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/types/index.ts (1)
Field(741-752)controlplane/src/core/services/SchemaGraphPruner.ts (1)
SchemaGraphPruner(21-404)
studio/src/components/operations/deprecated-fields-table.tsx (4)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationDeprecatedFields(2640-2649)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
getOperationDeprecatedFields(19-120)
connect/src/wg/cosmo/platform/v1/platform_connect.ts (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
GetOperationClientsRequest(22995-23059)GetOperationClientsResponse(23064-23102)GetOperationDeprecatedFieldsRequest(23164-23228)GetOperationDeprecatedFieldsResponse(23233-23271)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (12)
GetOperationClientsRequest(27822-27833)GetOperationClientsRequest(27848-27848)GetOperationClientsRequest(27863-27865)GetOperationClientsResponse(27909-27916)GetOperationClientsResponse(27931-27931)GetOperationClientsResponse(27946-27948)GetOperationDeprecatedFieldsRequest(27964-27975)GetOperationDeprecatedFieldsRequest(27990-27990)GetOperationDeprecatedFieldsRequest(28005-28007)GetOperationDeprecatedFieldsResponse(28051-28058)GetOperationDeprecatedFieldsResponse(28073-28073)GetOperationDeprecatedFieldsResponse(28088-28090)
studio/src/components/operations/operations-search.tsx (3)
studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
AnalyticsFilter(5954-6004)studio/src/components/analytics/filters.tsx (1)
AnalyticsFilters(25-33)
controlplane/src/core/bufservices/PlatformService.ts (2)
controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
getOperationClients(14-115)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
getOperationDeprecatedFields(19-120)
studio/src/hooks/use-operations-filters.tsx (3)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
AnalyticsViewResultFilter(6233-6283)AnalyticsFilter(5954-6004)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (14)
AnalyticsViewResultFilter(7536-7545)AnalyticsViewResultFilter(7560-7560)AnalyticsViewResultFilter(7575-7577)AnalyticsViewFilterOperator(287-287)AnalyticsViewFilterOperator(352-354)AnalyticsViewFilterOperator(356-358)AnalyticsViewFilterOperator(365-367)AnalyticsFilter(7184-7195)AnalyticsFilter(7210-7210)AnalyticsFilter(7225-7227)OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)
studio/src/components/operations/operation-content-modal.tsx (4)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationContent(918-927)controlplane/src/core/bufservices/analytics/getOperationContent.ts (1)
getOperationContent(14-79)studio/src/components/code-viewer.tsx (1)
CodeViewer(82-176)
studio/src/components/ui/scroll-area.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
controlplane/src/core/repositories/analytics/UsageRepository.ts (2)
controlplane/src/types/index.ts (1)
DateRange(630-633)controlplane/src/core/repositories/analytics/util.ts (1)
parseTimeFilters(417-429)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (32)
OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)DateRange(7250-7259)DateRange(7274-7274)DateRange(7289-7291)GetOperationsResponse_OperationType(703-703)GetOperationsResponse_OperationType(735-737)GetOperationsResponse_OperationType(739-741)GetOperationsResponse_OperationType(748-750)GetOperationClientsRequest(27822-27833)GetOperationClientsRequest(27848-27848)GetOperationClientsRequest(27863-27865)GetOperationClientsResponse(27909-27916)GetOperationClientsResponse(27931-27931)GetOperationClientsResponse(27946-27948)Response(807-815)Response(830-830)Response(845-847)GetOperationClientsResponse_Client(30481-30490)GetOperationClientsResponse_Client(30505-30505)GetOperationClientsResponse_Client(30520-30522)GetOperationDeprecatedFieldsRequest(27964-27975)GetOperationDeprecatedFieldsRequest(27990-27990)GetOperationDeprecatedFieldsRequest(28005-28007)GetOperationDeprecatedFieldsResponse(28051-28058)GetOperationDeprecatedFieldsResponse(28073-28073)GetOperationDeprecatedFieldsResponse(28088-28090)GetOperationDeprecatedFieldsResponse_DeprecatedField(30552-30560)GetOperationDeprecatedFieldsResponse_DeprecatedField(30575-30575)GetOperationDeprecatedFieldsResponse_DeprecatedField(30590-30592)connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
Response(683-691)Response(706-706)Response(721-723)
studio/src/components/layout/graph-layout.tsx (2)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/components/dashboard/workspace-selector.tsx (1)
WorkspaceSelector(16-58)
controlplane/src/core/bufservices/analytics/getOperationClients.ts (5)
controlplane/src/core/routes.ts (1)
RouterOptions(25-54)controlplane/src/core/util.ts (4)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)validateDateRanges(346-373)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(96-1835)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/core/repositories/analytics/util.ts (2)
isoDateRangeToTimestamps(353-366)getDateRange(404-415)
studio/src/components/operations/client-usage-table.tsx (4)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
getOperationClients(14-115)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationClients(2624-2633)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (3)
controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
getOperationClients(14-115)connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
GetOperationClientsRequest(22995-23059)GetOperationClientsResponse(23064-23102)GetOperationDeprecatedFieldsRequest(23164-23228)GetOperationDeprecatedFieldsResponse(23233-23271)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
getOperationDeprecatedFields(19-120)
studio/src/components/operations/operations-list.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (11)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-200)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(43-191)studio/src/components/operations/operations-list.tsx (1)
OperationsList(218-286)studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(33-156)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(450-521)LatencyMetricsCard(523-581)ErrorMetricsCard(583-642)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-80)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
studio/src/components/analytics/field-usage.tsx (1)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)
controlplane/src/core/bufservices/analytics/getOperations.ts (7)
controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
UsageRepository(12-455)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/core/util.ts (1)
validateDateRanges(346-373)composition/src/ast/utils.ts (1)
parse(272-274)composition/src/buildASTSchema/buildASTSchema.ts (1)
buildASTSchema(26-72)controlplane/src/core/services/SchemaGraphPruner.ts (1)
SchemaGraphPruner(21-404)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
GetOperationsResponse_Operation(22785-22867)
studio/src/components/analytics/metrics.tsx (3)
studio/src/lib/utils.ts (1)
cn(6-8)studio/src/components/info-tooltip.tsx (1)
InfoTooltip(16-37)studio/src/lib/insights-helpers.ts (1)
useChartData(66-151)
🪛 GitHub Check: CodeQL
studio/src/components/operations/operations-list.tsx
[failure] 80-80: Regular expression injection
This regular expression is constructed from a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (21)
studio/src/components/analytics/metrics.tsx (6)
46-52: LGTM! Import organization improved.The expanded import statement with explicit hooks improves readability while maintaining all necessary dependencies.
83-87: LGTM! Formatting improved.The reformatting enhances readability without changing the logic.
363-368: LGTM! Proper className propagation.The
classNameprop is correctly destructured and applied using thecn()utility, allowing external styling while preserving default styles.Also applies to: 379-379
454-455: LGTM! Well-designed flexibility enhancement.The addition of
showTopListandchartClassNameprops enables component reuse in different contexts while maintaining backward compatibility through sensible defaults.Also applies to: 458-458, 507-507, 510-518
527-528: LGTM! Consistent pattern applied.The same flexibility enhancement pattern is consistently applied, which improves maintainability across metric card components.
Also applies to: 531-531, 567-567, 570-578
587-591: LGTM! Complete pattern implementation.The
ErrorMetricsCardandErrorPercentChartcomponents now follow the same flexibility pattern. TheInfoTooltipreformatting is a minor cosmetic improvement.Also applies to: 606-608, 624-624, 627-639, 645-645, 653-653
studio/src/components/layout/graph-layout.tsx (2)
60-76: Namespace from workspace context fits existing patternsUsing
useWorkspaceto derivenamespace.nameand passing it both togetFederatedGraphByNameand intobasePathkeeps routing and data-loading in sync with howWorkspaceSelectoralready models namespaces. No issues spotted here.
132-137: “Operations” nav item wiring looks correctThe new “Operations” entry reuses the
basePath, points to/operations, and usesmatchExact: false, which should make nested operation views behave like Analytics/Routers. The icon sizing is consistent with the rest of the menu.studio/src/components/ui/scroll-area.tsx (1)
1-37: ScrollArea/ScrollBar wrapper is well-structuredThe forwardRef wrappers, type inference from the underlying primitives, and class composition all look solid. The component tree (Root → Viewport + ScrollBar + Corner) is coherent, and orientation-specific styling in
ScrollBaris clean.studio/src/components/analytics/field-usage.tsx (1)
71-75: Using workspace namespace in FieldUsage keeps analytics links consistentPulling
namespace.namefromuseWorkspaceand feeding it into the traces link query (namespaceparam) matches how the rest of the Studio UI models namespaces and avoids relying solely onrouter.query. This should make navigation to the traces view more robust as routing evolves.Also applies to: 251-271
controlplane/src/core/bufservices/PlatformService.ts (1)
167-168: New analytics RPCs correctly wired into PlatformService
getOperationClientsandgetOperationDeprecatedFieldsare imported and exposed using the same pattern as existing analytics methods, so routing from the platform service to the new handlers looks correct.Also applies to: 849-855
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
19-119: End‑to‑end deprecated‑fields flow looks consistent and safeAuth, analytics‑retention/date‑range validation, schema loading/pruning, and ClickHouse usage query are all wired consistently with existing analytics handlers. Returning an OK response with an empty list when there is no valid schema or no usages is a reasonable choice here, given the error logging in the catch block.
studio/src/components/operations/client-usage-table.tsx (1)
41-55: The review comment is incorrect; the guard is unnecessary.Looking at
useAnalyticsQueryState, thedateRangeobject is always initialized with a default value ({ start: subHours(new Date(), parsedRange), end: new Date() }) before any conditionals. The logic ensures that:
- When
query.dateRangeexists:rangebecomesundefined,dateRangeis set to the parsed value- When
query.dateRangedoesn't exist:rangeis set toparsedRange,dateRangeretains its defaultThere is no code path where
rangeis falsy anddateRangeis undefined. The hook guarantees thatdateRangeis always defined.Likely an incorrect or invalid review comment.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
403-427: NewOperationsFetchBasedOnenum wiring looks correctThe enum values and
setEnumTypemetadata are consistent and give a clear, extensible basis for selecting the metric (requests/latency/errors). No issues from a TS/proto wiring perspective.
4928-4931: OptionalnameonGetOperationContentRequestis correctly modeledThe new
name?: stringfield and its metadata entry{ no: 4, name: "name", ..., opt: true }correctly reflect an optional string at field number 4. This keeps the message backward compatible while allowing lookups by name.Also applies to: 4941-4945
22992-23159:GetOperationClients*messages align with Go definitionsThe TS definitions for:
GetOperationClientsRequest(federatedGraphName,namespace,operationHash,operationName, optionalrangeanddateRange), andGetOperationClientsResponseplus nestedClient(name,version, int64requestCount,lastUsedISO8601 string),are consistent with the Go snippets (field names, numbers, and types), including int64 handling via
protoInt64. No structural issues here.
23161-23320:GetOperationDeprecatedFields*message modeling looks correctThe request/response and nested
DeprecatedFieldare wired as expected:
- Request:
federatedGraphName,namespace,operationHash, optionaloperationName,range,dateRange.- Response:
responseplus repeateddeprecatedFieldswithfieldName,typeName,path.Field numbers and optionality match the provided Go definitions, and the TS
fieldsmetadata is coherent.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
638-640: LGTM: Conditional loader prevents layout shift.Good practice to only show the fullscreen loader on initial load (
!operationsData) and not during refetches, which prevents unnecessary layout shifts and improves UX.
67-67: Withdraw concern — graphContext is properly guarded throughout.All accesses to
graphContextin this file use optional chaining (?.), which safely handles the nullableGraphContextProps | undefinedtype. Query invalidation at lines 977–984 and all other usages correctly chain properties with?., preventing undefined errors.Likely an incorrect or invalid review comment.
connect/src/wg/cosmo/platform/v1/platform_connect.ts (1)
8-8: New PlatformService RPCs are wired consistentlyThe additions for
GetOperationClientsandGetOperationDeprecatedFields(imports plus method descriptors) match the generated message types and follow the existing pattern for unary analytics RPCs onPlatformService. No issues from an API‑surface perspective.Also applies to: 1800-1821
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
9-9: Connect-query bindings for new RPCs look correct
getOperationClientsandgetOperationDeprecatedFieldsare exported with the expectedlocalName, RPCname, unarykind, and corresponding request/response types, consistent with the rest of the connect-query surface.Also applies to: 2619-2650
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/components/operations/operations-list.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/components/operations/operations-list.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
studio/src/components/operations/operations-list.tsx (5)
96-106: LGTM!The cleanup effect properly clears both timeout references on unmount, preventing memory leaks.
108-133: LGTM!The hash click handler properly manages the copy-to-clipboard flow with appropriate timeout sequencing for the "Copied!" feedback and tooltip state transitions.
135-150: LGTM!The metric selection logic correctly handles different sort fields with appropriate formatting and null checks.
254-287: LGTM!The list rendering and selection logic is well-implemented with:
- Clear loading and empty states
- Thoughtful selection matching that handles both hash-only and hash+name scenarios
- Proper key generation for list items
The detailed inline comments explaining the selection logic are particularly helpful for maintainability.
81-82: Based on my verification of the codebase and security advisories:Lodash version 4.17.21 is already patched and secure.
The
_.escapeRegExp()method properly escapes regex metacharacters (^, $, , ., *, +, ?, (, ), [, ], {, }, |), which prevents regex injection. The known ReDoS vulnerabilities in lodash affect the toNumber, trim, and trimEnd functions, notescapeRegExp. The project's lodash version (^4.17.21) is the patched version that mitigates ReDoS vulnerabilities.The code pattern in question is secure: escaping user input before constructing a regex prevents special characters from being interpreted as regex operators. Wrapping the escaped query in a capturing group
(${safeQuery})maintains this safety because the input has already been neutralized.
…ations components
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)
693-707: Minor:subtitleprop is not rendered by GraphPageLayout.The
subtitleprop (line 697) is passed toGraphPageLayout, but as confirmed in past review discussions, while it's declared inTitleLayoutProps, the component doesn't destructure or render it.Either:
- Remove the
subtitleprop if it's not needed- Update
GraphPageLayoutinstudio/src/components/layout/graph-layout.tsxto render the subtitleBased on past review comments.
🧹 Nitpick comments (3)
proto/wg/cosmo/platform/v1/platform.proto (1)
2888-2902: Minor: Non-sequential field numbering in GetOperationsRequest.Field numbers jump from
fetchBasedOn = 7tosortDirection = 13, then back toincludeContent = 8. While protobuf allows non-sequential numbering, this pattern suggestssortDirectionmay have been added later or moved. This is not incorrect but can be confusing.Consider documenting the reason for the jump or renumbering fields sequentially if this is still under active development.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
250-252: Minor: Potential invalid syncId during initial load.If
graphContext?.graph?.namespaceorgraphContext?.graph?.nameis undefined during initial render,syncIdwill be"undefined-undefined". While the query is guarded byenabled: !!graphContext?.graph?.name, the syncId is constructed before this check.Consider adding a guard:
- const syncId = `${graphContext?.graph?.namespace}-${graphContext?.graph?.name}`; + const syncId = graphContext?.graph?.namespace && graphContext?.graph?.name + ? `${graphContext.graph.namespace}-${graphContext.graph.name}` + : undefined;Then handle
undefinedgracefully in metric cards or use it as-is if your sync logic already handles this case.Based on past review comments.
492-496: Recommend: Stabilize applySearchQuery to prevent unnecessary effect runs.The effect depends on
applySearchQuery, which indirectly depends on[router]through the dependency chain:
applySearchQuery→useCallback([applyNewParams])(use-operations-filters.tsx:132)applyNewParams→useCallback([router])(use-operations-filters.tsx:28)Since Next.js
useRouter()returns a non-stable object reference, this effect re-runs on every router change (even same-page navigations), not just when search actually changes.Consider using
router.pushdirectly or extracting only thepushmethod withuseRef:const routerPushRef = useRef(router.push); useEffect(() => { routerPushRef.current = router.push; }, [router.push]);Then update
useOperationsFiltersto use the ref instead of[router]dependency. This will stabilize the callback chain.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
proto/wg/cosmo/platform/v1/platform.proto(5 hunks)studio/src/components/analytics/metrics.tsx(17 hunks)studio/src/components/operations/operation-content-modal.tsx(1 hunks)studio/src/components/operations/operations-list.tsx(1 hunks)studio/src/hooks/use-operations-filters.tsx(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- studio/src/components/operations/operation-content-modal.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
studio/src/hooks/use-operations-filters.tsx (3)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
AnalyticsViewResultFilter(6233-6283)AnalyticsFilter(5954-6004)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (14)
AnalyticsViewResultFilter(7536-7545)AnalyticsViewResultFilter(7560-7560)AnalyticsViewResultFilter(7575-7577)AnalyticsViewFilterOperator(287-287)AnalyticsViewFilterOperator(352-354)AnalyticsViewFilterOperator(356-358)AnalyticsViewFilterOperator(365-367)AnalyticsFilter(7184-7195)AnalyticsFilter(7210-7210)AnalyticsFilter(7225-7227)OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)
studio/src/components/analytics/metrics.tsx (4)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/lib/utils.ts (1)
cn(6-8)studio/src/components/info-tooltip.tsx (1)
InfoTooltip(16-37)studio/src/lib/insights-helpers.ts (1)
useChartData(66-151)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (13)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)studio/src/hooks/use-feature-limit.ts (1)
useFeatureLimit(10-17)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
getGraphMetrics(1923-1933)getOperations(2592-2601)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-202)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(43-191)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(33-156)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(454-525)LatencyMetricsCard(527-585)ErrorMetricsCard(587-646)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-87)controlplane/src/core/bufservices/analytics/getOperations.ts (1)
getOperations(27-280)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
studio/src/components/operations/operations-list.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-router
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
🔇 Additional comments (4)
studio/src/components/analytics/metrics.tsx (2)
70-81: LGTM: Filter removal logic is now safe.The guard
if (index !== -1)at line 71 correctly prevents the splice bug identified in previous reviews wheresplice(-1, 1)would remove the last element.
454-525: LGTM: Metric card enhancements are well-designed.The addition of
showTopListandchartClassNameprops provides good flexibility for customizing metric card display. Defaults are sensible (showTopList = true), and the pattern is consistent across all metric cards.studio/src/components/operations/operations-list.tsx (1)
80-97: LGTM: Regex highlighting bug has been fixed.The previous issue with
regex.test()and the global flag has been correctly addressed. The code now uses a simple string comparison (part.toLowerCase() === queryLower) instead of the stateful regex test, preventing intermittent highlighting failures.proto/wg/cosmo/platform/v1/platform.proto (1)
2911-2922: Verify compatibility: Field number changed fortype.The
typefield moved from position 5 to position 4 inGetOperationsResponse.Operation. If this is a breaking change from a previous proto version, ensure all clients are updated or use field number preservation for backward compatibility.The new
oneof metricdesign is clean and appropriate for the use case where exactly one metric is returned based onfetchBasedOn.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
studio/src/components/operations/operations-search.tsx (2)
25-41: Consider stricter typing for sortDirection.The interface and constants are well-defined. For enhanced type safety, consider typing
sortDirectionas"desc" | "asc"instead ofstring.Apply this diff for stricter typing:
sortDirection: string; - onSortDirectionChange: (direction: string) => void; + sortDirection: "desc" | "asc"; + onSortDirectionChange: (direction: "desc" | "asc") => void;
150-166: Consider simplifying enum conversion pattern.The current pattern converts the enum to a string and back to a number. While functionally correct, this could be streamlined.
Consider using enum values directly if the Select component supports non-string values, or create a helper function to reduce repetition:
// Helper function approach const enumToSelectValue = (value: OperationsFetchBasedOn) => value.toString(); const selectValueToEnum = (value: string) => Number(value) as OperationsFetchBasedOn; // Usage <Select value={enumToSelectValue(fetchBasedOn)} onValueChange={(value) => onFetchBasedOnChange(selectValueToEnum(value))} >Alternatively, if this pattern is used elsewhere, consider creating shared utility functions in the analytics utilities file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/components/operations/operations-search.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/components/operations/operations-search.tsx (3)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (11)
OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)AnalyticsViewFilterOperator(287-287)AnalyticsViewFilterOperator(352-354)AnalyticsViewFilterOperator(356-358)AnalyticsViewFilterOperator(365-367)AnalyticsFilter(7184-7195)AnalyticsFilter(7210-7210)AnalyticsFilter(7225-7227)studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)studio/src/components/analytics/filters.tsx (1)
AnalyticsFilters(25-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (3)
studio/src/components/operations/operations-search.tsx (3)
1-23: LGTM!Imports are well-organized and appropriate for the component's functionality.
54-68: LGTM!All callback handlers are correctly implemented with proper memoization and dependency tracking.
81-104: LGTM!The selected options derivation and filter list construction are correctly implemented with proper memoization.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
22805-22856: Field number change fortyperemains a breaking protobuf change.This issue was previously flagged. The
typefield at field number 4 represents a wire-level breaking change from its previous position at field 5. This must be corrected in the source proto file before regenerating this TypeScript code.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)
180-217: Revisit requestCount conversion to Number for int64 metrics
requestCountis derived as:case "requestCount": requestCount = Number(op.metric.value) ?? 0; break;Given the proto defines
requestCountasint64, the generated TypeScript field may bebigintor a string representation. Converting blindly toNumberrisks precision loss for very large counts and can cause type friction if the generator usesbigint.A safer pattern would be to keep the underlying type (e.g.,
bigintor string) and only convert to string for display, or, if you truly need anumber, ensure the generator is configured to emit numbers for this field and/or clamp/validate againstNumber.MAX_SAFE_INTEGER.#!/bin/bash # Verify the TypeScript type of GetOperationsResponse.Operation.metric.requestCount rg -n "class GetOperationsResponse" connect/src/wg/cosmo/platform/v1/platform_pb.ts -n -A40 | sed -n '1,200p'
🧹 Nitpick comments (2)
studio/src/components/operations/operations-search.tsx (1)
85-152: Prefercnfor composing className to avoid stray"undefined"
<div className={w-full space-y-4 ${className}}>will render a literalundefinedclass whenclassNameis not passed. Consider using the sharedcnhelper for consistency with the rest of the codebase:-import { Input } from "@/components/ui/input"; +import { Input } from "@/components/ui/input"; +import { cn } from "@/lib/utils"; ... - return ( - <div className={`w-full space-y-4 ${className}`}> + return ( + <div className={cn("w-full space-y-4", className)}>studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)
588-602: Pagination heuristic can strand users on an empty page without controls
noOfPagesis computed optimistically:if (!operationsData?.operations) return 0; const operationsCount = operationsData.operations.length; if (operationsCount < pageSize) { return pageNumber; } return pageNumber + 1;And
Paginationis only rendered whenoperations.length > 0in the left panel:{operations.length > 0 && ( <Pagination limit={pageSize} noOfPages={noOfPages} pageNumber={pageNumber} /> )}Scenario: if there are exactly
pageSizeoperations on page N,noOfPagesbecomesN + 1. Navigating to pageN + 1will likely returnoperations.length === 0, causing thePaginationcomponent to disappear, leaving the user on a blank page with no pagination controls.Consider a safer strategy, for example:
- Track
totalCountfrom the backend (preferred), or- Only add the extra page when you actually observe
operationsCount === pageSizeon page 1, and keep showingPaginationeven when the current page has 0 items, so users can navigate back.Also applies to: 247-255
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (7)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(9 hunks)proto/wg/cosmo/platform/v1/platform.proto(6 hunks)studio/src/components/analytics/data-table-primary-filter-menu.tsx(4 hunks)studio/src/components/analytics/filters.tsx(1 hunks)studio/src/components/operations/operations-search.tsx(1 hunks)studio/src/hooks/use-operations-filters.tsx(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
🧬 Code graph analysis (6)
studio/src/components/operations/operations-search.tsx (3)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (11)
OperationsFetchBasedOn(657-657)OperationsFetchBasedOn(689-691)OperationsFetchBasedOn(693-695)OperationsFetchBasedOn(702-704)AnalyticsFilter(7187-7198)AnalyticsFilter(7213-7213)AnalyticsFilter(7228-7230)CustomOptions(174-174)CustomOptions(206-208)CustomOptions(210-212)CustomOptions(219-221)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
AnalyticsFilter(5960-6010)studio/src/components/analytics/filters.tsx (2)
AnalyticsFilter(9-19)AnalyticsFilters(26-34)
studio/src/components/analytics/data-table-primary-filter-menu.tsx (3)
studio/src/components/ui/dropdown-menu.tsx (2)
DropdownMenuContent(191-191)DropdownMenuCheckboxItem(193-193)studio/src/lib/utils.ts (1)
cn(6-8)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
CustomOptions(174-174)CustomOptions(206-208)CustomOptions(210-212)CustomOptions(219-221)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (32)
OperationsFetchBasedOn(657-657)OperationsFetchBasedOn(689-691)OperationsFetchBasedOn(693-695)OperationsFetchBasedOn(702-704)DateRange(7253-7262)DateRange(7277-7277)DateRange(7292-7294)GetOperationsResponse_OperationType(706-706)GetOperationsResponse_OperationType(738-740)GetOperationsResponse_OperationType(742-744)GetOperationsResponse_OperationType(751-753)GetOperationClientsRequest(27825-27836)GetOperationClientsRequest(27851-27851)GetOperationClientsRequest(27866-27868)GetOperationClientsResponse(27912-27919)GetOperationClientsResponse(27934-27934)GetOperationClientsResponse(27949-27951)Response(810-818)Response(833-833)Response(848-850)GetOperationClientsResponse_Client(30484-30493)GetOperationClientsResponse_Client(30508-30508)GetOperationClientsResponse_Client(30523-30525)GetOperationDeprecatedFieldsRequest(27967-27978)GetOperationDeprecatedFieldsRequest(27993-27993)GetOperationDeprecatedFieldsRequest(28008-28010)GetOperationDeprecatedFieldsResponse(28054-28061)GetOperationDeprecatedFieldsResponse(28076-28076)GetOperationDeprecatedFieldsResponse(28091-28093)GetOperationDeprecatedFieldsResponse_DeprecatedField(30555-30563)GetOperationDeprecatedFieldsResponse_DeprecatedField(30578-30578)GetOperationDeprecatedFieldsResponse_DeprecatedField(30593-30595)
studio/src/components/analytics/filters.tsx (1)
studio/src/components/analytics/data-table-primary-filter-menu.tsx (1)
DataTablePrimaryFilterMenu(26-103)
studio/src/hooks/use-operations-filters.tsx (3)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
AnalyticsViewResultFilter(6239-6289)AnalyticsFilter(5960-6010)studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)studio/src/components/analytics/filters.tsx (1)
AnalyticsFilter(9-19)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (13)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)studio/src/hooks/use-feature-limit.ts (1)
useFeatureLimit(10-17)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
getGraphMetrics(1923-1933)getOperations(2592-2601)connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
GetOperationsResponse(22722-22760)Pagination(5813-5851)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-229)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(44-154)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(33-156)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(454-525)LatencyMetricsCard(527-585)ErrorMetricsCard(587-646)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-87)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (5)
101-115: LGTM: Safe enum extension.The addition of
Boolean = 2to theCustomOptionsenum is a backward-compatible change.
409-433: LGTM: New enum addition.The
OperationsFetchBasedOnenum is appropriately structured for operation metric filtering.
4931-4950: LGTM: Safe optional field addition.The optional
namefield at position 4 is a backward-compatible addition toGetOperationContentRequest.
22617-22700: LGTM: Comprehensive request parameter additions.The new optional fields enable pagination (offset), filtering (includeDeprecatedFields, includeOperationsWithDeprecatedFieldsOnly, searchQuery), sorting (fetchBasedOn, sortDirection), and date range selection. All are backward-compatible additions.
Note: Field 13 (sortDirection) appears out of sequential order but this is acceptable in protobuf if it reflects the source proto structure.
22998-23326: LGTM: Well-structured new message types.The new
GetOperationClientsandGetOperationDeprecatedFieldsrequest/response message pairs are properly structured with consistent field numbering and appropriate nesting for client and deprecated field details.studio/src/components/analytics/filters.tsx (1)
21-33: className passthrough is correctly wired and non-breakingAdding
className?: stringand forwarding it toDataTablePrimaryFilterMenupreserves existing behavior while enabling external styling; no issues spotted.studio/src/components/operations/operations-search.tsx (1)
38-83: Filters and sorting integration looks solidThe boolean deprecated-fields filter is correctly modeled via
AnalyticsFilterswithCustomOptions.Boolean, and sort state is cleanly driven offOperationsFetchBasedOnandsortDirection. Memoization and callbacks have appropriate dependency lists.studio/src/hooks/use-operations-filters.tsx (1)
11-105: Filter state management and splice guard look correct
useSelectedFiltersandapplyNewParamsgive you robust, URL-driven filter state, and the updatedonSelectlogic correctly guardsindex !== -1before splicing or updating, avoiding the previoussplice(-1, 1)bug. Enum-to-string conversion forOperationsFetchBasedOnis also straightforward and centralized here.studio/src/components/analytics/data-table-primary-filter-menu.tsx (1)
23-45: Boolean filter support and styling hook are well implementedUsing
CustomOptions.Booleanto render aDropdownMenuCheckboxItemwithcheckedbound toselectedOptions.length > 0and delegating viaonSelect?.(checked ? ["true"] : undefined)is a clean, minimal extension. PassingclassNamethroughcn("w-56", className)onDropdownMenuContentgives flexible styling without breaking defaults.Also applies to: 68-84
proto/wg/cosmo/platform/v1/platform.proto (2)
819-830: CustomOptions.Boolean cleanly extends analytics filter configurationAdding
Boolean = 2toCustomOptionsand wiring it intoAnalyticsViewResultFilter.custom_optionsmatches the new boolean filter path used in the UI; existing numeric values (Text = 0, Range = 1) are preserved, so this is wire-compatible.
2883-2903: Confirm: Field number migration from field 5→4 fortypefield is a breaking changeYour concern is valid. Git history confirms that in commit
f2607e6e(feat: refactor GetOperationsResponse to use oneof for metrics), theOperationType typefield was moved from field number 5 to field number 4, andhasDeprecatedFieldswas moved from 8 to 5. The previous structure used individual fields (latency, requestCount, errorCount) at positions 4–7; the refactored version reorganizes these into aoneof metricblock.Before refactor:
- float latency = 4
- OperationType type = 5
- int64 requestCount = 6
- int64 errorCount = 7
- bool hasDeprecatedFields = 8
After refactor (current):
- OperationType type = 4
- bool hasDeprecatedFields = 5
- oneof metric { float latency = 6; int64 requestCount = 7; float errorPercentage = 8; }
This renumbering breaks wire-format compatibility with clients compiled against the previous schema. Confirm this breaking change is intentional and acceptable for your versioning strategy. If backward compatibility is required, consider reserving old field numbers or implementing a versioning/migration strategy.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
491-535: Search URL syncing and debounce logic looks coherentUsing
localSearchQueryas the input state, debounced viauseDebounce, and only callingapplySearchQuerywhendebouncedSearchQuery !== urlSearchQuerykeeps the URL as the single source of truth while avoiding unnecessary router pushes. Resettingpageto"1"on search changes is also a nice touch.
729-762: Two-panel layout and layout integration are cohesiveThe split between
OperationsLeftPanel(list/search/pagination) andOperationsRightPanel(details/metrics) is clear, and composing the page viaGraphPageLayout+getGraphLayoutmatches existing layout patterns in the repo. PassingOperationsToolbarintoitemskeeps the toolbar aligned with other analytics pages.Also applies to: 765-779
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (3)
771-777: Minor:subtitleprop is not rendered byGraphPageLayout.The
subtitleprop is passed toGraphPageLayout(line 773), but based on the component implementation ingraph-layout.tsx(lines 246-296),GraphPageLayoutdoes not destructure or render thesubtitleproperty, even though it's declared in theTitleLayoutPropsinterface.Options:
- Remove the unused
subtitleprop from this usage if it's not essential- Update
GraphPageLayoutcomponent to destructure and render thesubtitleif you need it displayedIf the subtitle is not needed:
<GraphPageLayout title="Operations" - subtitle="Comprehensive view into GraphQL Operations Performance" items={<OperationsToolbar />} noPadding className="md:w-full lg:pl-6 lg:pr-5 xl:pl-8 xl:pr-5" >If you need the subtitle displayed, update the
GraphPageLayoutcomponent ingraph-layout.tsxto render it.Based on past review comments.
676-711: Major: useEffect readscurrentFilterStatewithout declaring it in dependencies.The effect at lines 676-711 reads
currentFilterState(line 694) but doesn't include it in the dependencies array (line 711). The ESLint rule is disabled (line 710) to suppress this warning.Issues:
- Effect may not run when
filterStatechanges externally (e.g., browser back/forward navigation)- Reading reactive values without declaring them as dependencies is an anti-pattern that can lead to stale closures
- The disabled ESLint rule hides this architectural problem
Recommended fix:
useEffect(() => { if (!selectedOperation || !operationsData?.operations) { return; } const operationExists = operationsData.operations.some((op) => { const matchesHash = op.hash === selectedOperation.hash; if (!matchesHash) return false; const opName = op.name || ""; const selectedName = selectedOperation.name || ""; return opName === selectedName; }); if (!operationExists) { const hasOperationFilters = currentFilterState.some( (f: { id: string }) => f.id === "operationHash" || f.id === "operationName", ); if (hasOperationFilters) { const filteredState = currentFilterState.filter( (f: { id: string }) => f.id !== "operationHash" && f.id !== "operationName", ); applyParams({ filterState: JSON.stringify(filteredState), }); } } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [operationsData?.operations, selectedOperation]); + }, [operationsData?.operations, selectedOperation?.hash, selectedOperation?.name, currentFilterState, applyParams]);Use stable primitives (
selectedOperation?.hash,selectedOperation?.name) instead of the full object to prevent unnecessary re-runs, and includecurrentFilterStateandapplyParamsto satisfy the exhaustive-deps rule.
278-278: Add guard clause for syncId construction to prevent invalid "undefined-undefined" value.Line 278 constructs
syncIdfrom potentially undefinedgraphContext?.graph?.namespaceand.namefields without guards. This inconsistency is problematic because:
- Line 580 of the same file guards with
!!graphContext?.graph?.name, proving these fields can be undefined- If either field is undefined, the template literal produces "undefined-undefined", breaking chart synchronization
- No early return or fallback handles the case when context data isn't available
Add a guard clause to handle undefined values:
- const syncId = `${graphContext?.graph?.namespace}-${graphContext?.graph?.name}`; + const syncId = graphContext?.graph?.namespace && graphContext?.graph?.name + ? `${graphContext.graph.namespace}-${graphContext.graph.name}` + : "default-sync";Alternatively, return early if context is unavailable to prevent rendering components that depend on valid syncId.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
🧬 Code graph analysis (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (11)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
getGraphMetrics(1923-1933)getOperations(2592-2601)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-229)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(44-154)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(33-156)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(454-525)LatencyMetricsCard(527-585)ErrorMetricsCard(587-646)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-87)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Show resolved
Hide resolved
…ehensive tests for operations retrieval
…ed readability and responsiveness
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)
275-275: Potential issue: syncId can be "undefined-undefined".If
graphContext?.graph?.namespaceorgraphContext?.graph?.nameis undefined during initial render, thesyncIdwill be"undefined-undefined", which could cause issues with chart synchronization across the metric cards.Apply this fix to ensure a valid syncId:
- const syncId = `${graphContext?.graph?.namespace}-${graphContext?.graph?.name}`; + const syncId = graphContext?.graph?.namespace && graphContext?.graph?.name + ? `${graphContext.graph.namespace}-${graphContext.graph.name}` + : undefined;The metric cards should handle
undefinedsyncId gracefully (charts without sync), or you can provide a fallback like'default'.
🧹 Nitpick comments (4)
studio/src/components/operations/client-usage-table.tsx (2)
106-130: Avoid using index in React keys.Line 107 uses
indexin the key, which is generally not recommended in React. Since the SQL query groups byClientNameandClientVersion, the combination of${client.name}-${client.version}should be unique. Consider removing the index from the key.clientUsageData.map((client, index) => ( - <TableRow key={`${client.name}-${client.version}-${index}`}> + <TableRow key={`${client.name}-${client.version}`}> <TableCell className="w-[35%]">
86-93: Clarify the purpose of the empty 5% column.Line 91 defines an empty column with 5% width and no header text. If this is intentional spacing, consider using CSS padding or gap instead. If it serves a specific purpose (e.g., future action buttons), consider adding a comment.
controlplane/test/analytics/get-operations.test.ts (1)
21-28: Optional: Redundant mock clearing in afterEach.Both
beforeEach(line 23) andafterEach(line 27) callvi.clearAllMocks(). SincebeforeEachalready clears mocks before each test, theafterEachclearing is redundant.You can simplify by removing the redundant
afterEach:beforeEach(() => { chClient = new ClickHouseClient(); vi.clearAllMocks(); }); - afterEach(() => { - vi.clearAllMocks(); - });studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)
770-770: Note: subtitle prop is declared but not rendered.Based on previous review discussion, while
subtitleis declared in theTitleLayoutPropsinterface, theGraphPageLayoutcomponent (studio/src/components/layout/graph-layout.tsx lines 246-296) does not destructure or render it.If you need the subtitle to appear, you'll need to update
GraphPageLayoutto extract and render the subtitle prop. Otherwise, this line can be removed as the prop currently has no effect.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controlplane/src/core/util.ts(1 hunks)controlplane/test/analytics/get-operations.test.ts(1 hunks)studio/src/components/operations/client-usage-table.tsx(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
controlplane/test/analytics/get-operations.test.ts
🧬 Code graph analysis (3)
controlplane/test/analytics/get-operations.test.ts (4)
controlplane/src/core/clickhouse/client/ClickHouseClient.ts (1)
ClickHouseClient(19-380)controlplane/src/core/test-util.ts (4)
beforeAllSetup(38-45)afterAllSetup(47-51)genID(53-55)genUniqueLabel(57-59)controlplane/test/test-util.ts (3)
SetupTest(66-417)createThenPublishSubgraph(550-573)DEFAULT_NAMESPACE(52-52)shared/src/utils/util.ts (1)
joinLabel(17-19)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (11)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)studio/src/hooks/use-feature-limit.ts (1)
useFeatureLimit(10-17)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
getGraphMetrics(1923-1933)getOperations(2592-2601)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-229)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(33-164)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(454-525)LatencyMetricsCard(527-585)ErrorMetricsCard(587-646)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-87)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
studio/src/components/operations/client-usage-table.tsx (4)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
getOperationClients(14-115)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationClients(2624-2633)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
studio/src/components/operations/client-usage-table.tsx (2)
1-18: LGTM!All imports are properly used throughout the component.
20-31: LGTM!Type definitions are clear and appropriate.
controlplane/src/core/util.ts (1)
368-374: LGTM! Date range validation guard is correct.The guard clause properly validates that the start date is not after the end date and returns with
dateRange: undefinedwhen invalid. ISO 8601 string comparison works correctly for this validation, and the early return pattern is appropriate.controlplane/test/analytics/get-operations.test.ts (3)
38-199: LGTM! Comprehensive error path coverage.The test suite thoroughly validates all error scenarios:
- Analytics disabled when ClickHouse unavailable
- Graph not found
- Invalid limit (< 1 or > 1000)
- Negative offset
- Invalid date range (start after end)
All assertions properly verify response codes and error details.
231-387: LGTM! Sorting tests properly validate fetchBasedOn parameter.The tests correctly verify that operations can be sorted by:
- Latency (lines 231-281) - validates
metric.case === 'latency'and value- Request count (lines 283-333) - validates
metric.case === 'requestCount'and BigInt value- Error percentage (lines 335-387) - validates
metric.case === 'errorPercentage'and valueThe mock data structure and assertions align with the oneof metric field design in the proto.
480-523: LGTM! Content inclusion logic properly tested.The test correctly verifies that when
includeContent: false:
- Operation content is not fetched (line 520:
expect(chClient.queryPromise).toHaveBeenCalledTimes(1))- The
contentfield is empty string (line 517)This confirms the conditional content fetching optimization works as expected.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
67-144: LGTM! Toolbar properly handles date range and refresh.The toolbar implementation correctly:
- Manages date range selection with proper URL param updates
- Invalidates graph metrics with matching query keys including both
rangeanddateRange(lines 119-131)- Applies analytics retention limits via feature flags (line 105)
The invalidation key fix (including dateRange when range is undefined) addresses the previous review concern.
181-218: LGTM! Operation metrics extraction is correct.The
computedOperationsmemo properly:
- Handles the oneof
metricfield by switching onmetric.case- Safely converts BigInt to Number with null check:
op.metric.value ? Number(op.metric.value) : 0(line 195)- Falls back to 0 for undefined metrics (lines 212-214)
- Has correct dependencies:
[operations]The BigInt conversion is acceptable per previous discussion - precision loss is not a concern for this use case.
| useEffect(() => { | ||
| if (debouncedSearchQuery !== urlSearchQuery) { | ||
| applySearchQuery(debouncedSearchQuery); | ||
| // Reset to page 1 when search changes | ||
| if (pageNumber !== 1) { | ||
| applyParams({ page: "1" }); | ||
| } | ||
| } | ||
| }, [ | ||
| debouncedSearchQuery, | ||
| urlSearchQuery, | ||
| applySearchQuery, | ||
| pageNumber, | ||
| applyParams, | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify stability of callback dependencies.
This useEffect includes applySearchQuery and applyParams in its dependencies. Based on the hook chain:
applySearchQuerycomes fromuseOperationsFilters(use-operations-filters.tsx)- Which uses
useCallbackdepending onapplyNewParams - Which depends on
[router]fromuseRouter()
Since Next.js's useRouter() returns a new object on each navigation (not referentially stable), this could cause unnecessary effect runs whenever the router object changes, even if the actual route hasn't changed.
Verify the stability of these callbacks:
If router instability is causing issues, consider using specific router properties like router.pathname or extract stable methods (like router.push) into refs.
🏁 Script executed:
#!/bin/bash
# Check the dependency chains in useOperationsFilters and useApplyParams
rg -nP -A10 'export const useOperationsFilters' studio/src/hooks/use-operations-filters.tsx
rg -nP -A10 'export const useApplyParams' studio/src/components/analytics/use-apply-paramsLength of output: 615
🏁 Script executed:
#!/bin/bash
# Get full useOperationsFilters implementation to see what's exported and dependency chains
rg -nP -A50 'export const useOperationsFilters' studio/src/hooks/use-operations-filters.tsx
# Also search for where applySearchQuery might be defined
rg -nP 'applySearchQuery' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsx | head -20
# Find applySearchQuery definition in the hooks
rg -nP 'applySearchQuery' studio/src/hooks/use-operations-filters.tsx | head -20Length of output: 1913
🏁 Script executed:
#!/bin/bash
# Get full definition of applySearchQuery in operations.tsx to see its dependencies
rg -nP -A20 'const applySearchQuery = useCallback' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsxLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get context around line 159 where applySearchQuery is defined
sed -n '155,180p' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsx
# Also get the full useEffect at lines 522-536 with more context
sed -n '515,545p' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsxLength of output: 1531
🏁 Script executed:
#!/bin/bash
# Find the full destructuring of useOperationsFilters call
sed -n '173,195p' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsx
# Search for where applySearchQuery is actually assigned/defined in operations.tsx
rg -nP 'applySearchQuery|applyParams' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsx | head -30Length of output: 1079
🏁 Script executed:
#!/bin/bash
# Search broadly for where applySearchQuery comes from - import, const assignment, etc.
rg -nP -B5 'applySearchQuery' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsx | grep -E '(import|const|=)' | head -20
# Get context around line 502 where applySearchQuery is in the useEffect dependency
sed -n '495,515p' studio/src/pages/\[organizationSlug\]/\[namespace\]/graph/\[slug\]/operations.tsx
# Find useApplyParams definition
rg -nP 'export.*useApplyParams|const useApplyParams' studio/src --type=tsx --type=ts | head -5Length of output: 866
🏁 Script executed:
#!/bin/bash
# Get full useOperationsFilters implementation - search for the return statement and all exports
rg -nP -A150 'export const useOperationsFilters' studio/src/hooks/use-operations-filters.tsx | tail -100
# Search for useApplyParams in all files
find studio/src -name "*.ts" -o -name "*.tsx" | xargs grep -l "useApplyParams" | head -5Length of output: 3419
🏁 Script executed:
#!/bin/bash
# Get useApplyParams implementation
rg -nP -A30 'export.*useApplyParams|const useApplyParams' studio/src/components/analytics/use-apply-params.tsx | head -50Length of output: 635
Both callback dependencies have unstable dependency chains rooted in useRouter().
The concern is valid:
applySearchQuerydepends onapplyNewParams(use-operations-filters.tsx:159-166), which depends on[router](use-operations-filters.tsx:56)applyParamsdepends on the result ofuseApplyParams(), which also depends on[router](use-apply-params.tsx:23)- In both cases,
routerfromuseRouter()is recreated on each render, causing these callbacks to be recreated unnecessarily
This can trigger the effect on every render even when search query and page number haven't changed, potentially causing redundant router.push() calls.
Consider extracting router.push into a useRef or memoizing the router's push method to break the dependency chain on the router object itself.
🤖 Prompt for AI Agents
In studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
around lines 522 to 536, the effect depends on two callbacks that transitively
depend on useRouter() (causing router to be a new object each render) which
recreates those callbacks and may retrigger the effect; to fix, create a stable
reference to router.push (e.g., capture router.push in a useRef or wrap it in a
useCallback with minimal deps) and update use-operations-filters.tsx and
use-apply-params.tsx so their returned callbacks call that stable push ref
instead of the raw router object, then adjust the effect dependencies to use
those now-stable callbacks (or the debouncedSearchQuery/pageNumber) so the
effect only runs when the actual inputs change.
…covering various scenarios including error handling and client retrieval
…ensive tests for its functionality, including error handling and date range validation
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
controlplane/test/analytics/get-operation-clients.test.ts (1)
20-27: Consider removing redundant mock clearing.Both
beforeEachandafterEachcallvi.clearAllMocks(). SincebeforeEachruns before each test and already clears mocks, theafterEachclearing is redundant.Apply this diff if you prefer to keep only one:
beforeEach(() => { chClient = new ClickHouseClient(); vi.clearAllMocks(); }); afterEach(() => { - vi.clearAllMocks(); });controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
77-107: Schema/usage failures are silently converted into “no deprecated fields” responsesIf
getLatestValidSchemaVersionreturnsundefinedor any error occurs while parsing the schema, building the GraphQL schema, or querying usage (the entire block aroundparse,SchemaGraphPruner, andgetDeprecatedFieldsUsedInOperation), the catch block only logs and you still returnEnumStatusCode.OKwith an emptydeprecatedFieldsarray.This conflates “no deprecated usage” with “backend failed to analyze schema/usage”, which can mislead both UI and operators.
Consider making failures explicit, for example:
- Return a distinct error code (e.g.
ERRorERR_NOT_FOUND) when no valid schema version exists.- In the
catch, either rethrow to lethandleErrorconvert to an error response, or return anERRresponse with a diagnosticdetailsmessage while still logging.That would keep the endpoint semantics sharper and make operational issues observable instead of silently downgraded to “no results”.
controlplane/test/analytics/get-operation-deprecated-fields.test.ts (2)
151-201: Strengthen assertions for deprecated-fields responsesThe tests:
test('Should return deprecated fields used in operation', ...)currently only checksresponse.deprecatedFields.length).toBeGreaterThanOrEqual(0), which is always true.test('Should return deprecated fields with correct field name and type name', ...)only asserts thatfieldNameandtypeNameare defined, and only ifdeprecatedFields.length > 0.Given you fully control:
- The schema (with known deprecated fields),
- The mocked ClickHouse result (
mockDeprecatedFields),you can assert the exact contents of the response: number of entries, specific
fieldName,typeName, andpathvalues. That would quickly reveal regressions in howSchemaGraphPrunerfields are mapped throughUsageRepositoryinto the RPC response.For example, in the “used in operation” test, you could assert something along the lines of:
- expect(response.response?.code).toBe(EnumStatusCode.OK); - expect(response.deprecatedFields.length).toBeGreaterThanOrEqual(0); - // The response should contain deprecated fields if they exist in the schema + expect(response.response?.code).toBe(EnumStatusCode.OK); + expect(response.deprecatedFields).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + fieldName: 'hello', + typeName: 'Query', + path: 'Query.hello', + }), + expect.objectContaining({ + fieldName: 'oldName', + typeName: 'User', + path: 'User.oldName', + }), + ]), + );And in the “correct field name and type name” test, instead of only checking for defined-ness behind an
if (length > 0), directly assert the expected single element:- expect(response.response?.code).toBe(EnumStatusCode.OK); - // If deprecated fields are found, they should have correct structure - if (response.deprecatedFields.length > 0) { - expect(response.deprecatedFields[0]?.fieldName).toBeDefined(); - expect(response.deprecatedFields[0]?.typeName).toBeDefined(); - } + expect(response.response?.code).toBe(EnumStatusCode.OK); + expect(response.deprecatedFields).toEqual([ + expect.objectContaining({ + fieldName: 'hello', + typeName: 'Query', + path: 'Query.hello', + }), + ]);This makes the suite much more effective at guarding the new analytics behavior.
Also applies to: 376-423
239-303: Special-character tests don’t actually cover query/escaping behaviorThe tests:
test('Should handle operation name with special characters', ...)test('Should handle operation hash with special characters', ...)intend to guard behavior when
operationName/operationHashcontain single quotes. However, becauseClickHouseClientis fully mocked andqueryPromiseis stubbed to simplymockResolvedValue([]), these tests only assert that the RPC returnsEnumStatusCode.OK; they never exercise or validate:
- How
UsageRepository.getDeprecatedFieldsUsedInOperationbuilds the ClickHouse query string.- Whether quotes and other special characters are escaped safely.
As a result, these tests may give a false sense of safety around SQL/ClickHouse injection and escaping.
Consider either:
- Moving this concern into focused tests for
UsageRepository.getDeprecatedFieldsUsedInOperationthat:
- Use a real
ClickHouseClientconfigured with a harmless DSN, or- Use a more sophisticated mock that records the last query string so you can assert that
'is correctly escaped / parameterized.- Or, if you keep these RPC-level tests, add expectations that inspect how
UsageRepositoryis called (e.g., via a spy) to ensure the inputs are sanitized before reaching the query builder.That will make the “special characters” coverage actually exercise the risky area instead of just the happy-path response shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts(1 hunks)controlplane/src/core/util.ts(1 hunks)controlplane/test/analytics/get-operation-clients.test.ts(1 hunks)controlplane/test/analytics/get-operation-deprecated-fields.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/util.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
controlplane/test/analytics/get-operation-clients.test.tscontrolplane/test/analytics/get-operation-deprecated-fields.test.ts
🧬 Code graph analysis (3)
controlplane/test/analytics/get-operation-clients.test.ts (4)
controlplane/src/core/clickhouse/client/ClickHouseClient.ts (1)
ClickHouseClient(19-380)controlplane/src/core/test-util.ts (4)
beforeAllSetup(38-45)afterAllSetup(47-51)genID(53-55)genUniqueLabel(57-59)controlplane/test/test-util.ts (3)
SetupTest(66-417)createThenPublishSubgraph(550-573)DEFAULT_NAMESPACE(52-52)shared/src/utils/util.ts (1)
joinLabel(17-19)
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (5)
controlplane/src/core/routes.ts (1)
RouterOptions(25-54)controlplane/src/core/util.ts (4)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)validateDateRanges(346-380)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(96-1835)controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
UsageRepository(12-455)controlplane/src/core/services/SchemaGraphPruner.ts (1)
SchemaGraphPruner(21-404)
controlplane/test/analytics/get-operation-deprecated-fields.test.ts (4)
controlplane/src/core/clickhouse/client/ClickHouseClient.ts (1)
ClickHouseClient(19-380)controlplane/src/core/test-util.ts (4)
beforeAllSetup(38-45)afterAllSetup(47-51)genID(53-55)genUniqueLabel(57-59)controlplane/test/test-util.ts (3)
SetupTest(66-417)createThenPublishSubgraph(550-573)DEFAULT_NAMESPACE(52-52)shared/src/utils/util.ts (1)
joinLabel(17-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
controlplane/test/analytics/get-operation-clients.test.ts (2)
1-14: LGTM!The imports and mock setup are well-structured. The ClickHouseClient mock correctly targets the prototype's queryPromise method, which will be available for all test cases.
16-496: This review comment is incorrect—there are no pagination or sorting parameters in the GetOperationClientsRequest RPC.The proto definition at
proto/wg/cosmo/platform/v1/platform.proto:2941shows GetOperationClientsRequest has only these fields:federatedGraphName,namespace,operationHash,operationName,range(hours), anddateRange. There are no pagination or sorting parameters defined in the API.The test file comprehensively covers all actual supported parameters: date ranges, time ranges, special characters in operation hash/name, null values, and zero request counts. The suggestion to add pagination/sorting tests is based on a misunderstanding of the actual RPC interface capabilities.
Likely an incorrect or invalid review comment.
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts
Show resolved
Hide resolved
…onality for client data loading
…peration hash and name in analytics functions
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
100-101: SQL injection risk persists despite quote escaping.While escaping single quotes (
'→'') provides partial mitigation, the underlyingusageRepo.getDeprecatedFieldsUsedInOperationstill constructs SQL via string concatenation. This approach remains vulnerable to injection attacks because:
- Only single quotes are escaped; other ClickHouse-specific characters may enable injection.
- String concatenation inherently cannot guarantee safe interpolation of untrusted input.
The proper fix requires refactoring
UsageRepository.getDeprecatedFieldsUsedInOperationto use ClickHouse query parameters (param_*) for all dynamic values:organizationId,federatedGraphId,operationHash,operationName, and deprecated field names/type names.Based on previous review feedback.
controlplane/test/analytics/get-operation-clients.test.ts (1)
363-452: Special-character tests look good; optionally also assert ClickHouse paramsThe operationHash/operationName special-character tests (Lines 363-452) now verify that the raw values with single quotes do not appear in the SQL string, which is a solid safeguard against naive string interpolation.
To go one step further, you could also assert that the second argument to
queryPromisecontains the raw values, confirming parameterized usage:- const calledQuery = (chClient.queryPromise as Mock).mock.calls[0][0]; - // Verify proper escaping or parameterization - expect(calledQuery).not.toContain("test'hash"); // Should be escaped/parameterized + const [calledQuery, calledParams] = (chClient.queryPromise as Mock).mock.calls[0]; + // Verify proper escaping or parameterization + expect(calledQuery).not.toContain("test'hash"); + expect(calledParams.operationHash).toBe("test'hash");and similarly for
operationName. The current assertions are already valuable; this would just tighten them further.
🧹 Nitpick comments (3)
controlplane/test/analytics/get-operation-clients.test.ts (3)
20-27: Avoid redundantvi.clearAllMockscallsYou call
vi.clearAllMocks()in bothbeforeEach(Line 22) andafterEach(Line 26). Clearing once per test is enough and slightly easier to reason about.Consider keeping it only in
afterEach:- beforeEach(() => { - chClient = new ClickHouseClient(); - vi.clearAllMocks(); - }); + beforeEach(() => { + chClient = new ClickHouseClient(); + });
147-238: Core client-shape tests are strong; optional extra check forlastUsedThe tests for “clients for an operation”, empty name/version, and zero requestCount (Lines 147-238, 240-278) nicely cover mapping of
name,version, andrequestCount(including BigInt conversion and null → empty string).Given the Studio UI assumes
lastUsedis always present, you might optionally assert that at least one returned client has a defined/truthylastUsedto guard against regressions:expect(response.clients[0]?.requestCount).toBe(BigInt(100)); + expect(response.clients[0]?.lastUsed).toBeTruthy();This is a nice-to-have only; current coverage is already useful. Based on learnings
280-361: Range/dateRange success cases covered; could also assert query usage if desiredThe “date range correctly” and “range parameter correctly” tests (Lines 280-321, 323-361) confirm that providing either
dateRangeorrangeyields anOKresponse and non-empty clients, which is good functional coverage.If you ever want to enforce that these parameters actually influence the ClickHouse query, you could mirror the special-character tests and inspect
chClient.queryPromise.mock.calls[0][0]for the appropriate date predicates/placeholders—but this is not strictly necessary for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/src/core/bufservices/analytics/getOperationContent.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts(1 hunks)controlplane/test/analytics/get-operation-clients.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/bufservices/analytics/getOperationContent.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
controlplane/test/analytics/get-operation-clients.test.ts
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
controlplane/test/analytics/get-operation-clients.test.ts
📚 Learning: 2025-07-30T09:29:04.257Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
Applied to files:
controlplane/test/analytics/get-operation-clients.test.ts
📚 Learning: 2025-11-19T11:44:31.709Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.709Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
controlplane/test/analytics/get-operation-clients.test.ts
🧬 Code graph analysis (2)
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (8)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationDeprecatedFields(2640-2649)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
GetOperationDeprecatedFieldsRequest(23170-23234)GetOperationDeprecatedFieldsResponse(23239-23277)controlplane/src/core/util.ts (4)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)validateDateRanges(346-380)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(96-1835)controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
UsageRepository(12-455)controlplane/src/types/index.ts (1)
Field(741-752)controlplane/src/core/services/SchemaGraphPruner.ts (1)
SchemaGraphPruner(21-404)
controlplane/test/analytics/get-operation-clients.test.ts (3)
controlplane/src/core/clickhouse/client/ClickHouseClient.ts (1)
ClickHouseClient(19-380)controlplane/test/test-util.ts (3)
SetupTest(66-417)createThenPublishSubgraph(550-573)DEFAULT_NAMESPACE(52-52)shared/src/utils/util.ts (1)
joinLabel(17-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (5)
1-18: LGTM!Imports are well-organized and all dependencies are properly declared.
27-34: LGTM!Appropriate early return when analytics is disabled, preventing downstream errors.
54-75: LGTM!Date range validation properly enforces retention limits and provides sensible defaults, with appropriate error handling for invalid ranges.
104-106: Graceful degradation is appropriate here.The broad catch gracefully handles schema parsing or retrieval errors by returning empty deprecated fields. Since this is an analytics query, failing silently with logging is acceptable.
112-119: LGTM!Response mapping correctly transforms the deprecated fields data with appropriate fallbacks for missing values.
controlplane/test/analytics/get-operation-clients.test.ts (4)
1-14: Mocking ClickHouseClient at module level looks solidThe vi.mock setup cleanly replaces ClickHouseClient and exposes queryPromise for inspection, which is exactly what you need for these analytics tests. No issues here.
37-80: Error handling scenarios are well coveredThe tests for
ERR_ANALYTICS_DISABLED(Lines 37-63) andERR_NOT_FOUND(Lines 65-80) exercise the important negative paths: missing ClickHouse client and non-existent federated graph, including checkingdetailscontent and emptyclients. This looks good.
82-145: Date-range validation and empty-result behavior are exercised correctlyThe invalid dateRange test (Lines 82-114) checks both status code and error message, and the “no clients exist” test (Lines 116-145) verifies the handler gracefully returns
OKwith an empty list when ClickHouse yields no rows. Both behaviors match expected semantics and should be stable.
454-507: Multi-client grouping test is precise and aligned with expectationsThe “multiple clients with same name but different versions” test (Lines 454-507) precisely asserts
response.clients.length === 3and checks that both versions fortest-clientare present, matching the intended grouping by(name, version). This closes the earlier imprecision around client count nicely.
…rgraph/cosmo into suvij/eng-5256-operations-overview
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
193-230: Operation metric mapping looks good; tiny enum‑mapping nit.The oneof handling for latency/requestCount/errorPercentage and the
0fallbacks (including the guarded BigInt→Number conversion) look correct and avoid the earlierNaNissue; given request counts stay within JS safe integer bounds, this is fine. One small improvement: defaulting any unknownGetOperationsResponse_OperationTypeto"subscription"could be confusing if new enum values are added later—consider an explicitswitchwith a more neutral fallback like"query"or"unknown".Based on learnings
567-732: URL‑driven search/pagination and selection logic are solid; optional hash‑only selection support.Debounced local search synced back to the URL (with the inequality guard and page reset) plus the page/pageSize/offset derivation all look robust and avoid feedback loops. One minor nit:
selectedOperationis only populated when bothoperationHashandoperationNamefilters exist; if you want to support legacy or manually crafted URLs that only includeoperationHash, you could relax thisuseMemoto treatoperationNameas optional (null/undefined) and let the downstream matching logic—which already handles null/undefined names—take care of it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.640Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.640Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
Applied to files:
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
📚 Learning: 2025-11-19T11:44:31.709Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.709Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
🧬 Code graph analysis (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (14)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)studio/src/hooks/use-feature-limit.ts (1)
useFeatureLimit(10-17)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
getGraphMetrics(1923-1933)getOperations(2592-2601)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
GetOperationsResponse(22722-22760)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-229)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(44-154)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(35-188)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(454-525)LatencyMetricsCard(527-585)ErrorMetricsCard(587-646)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-87)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (3)
80-152: Toolbar date/range handling and metrics refresh are consistent.Date range and refresh interval updates are correctly funneled through
applyParams, and the manual refresh invalidatesgetGraphMetricswith the samenamespace/federatedGraphName/range/dateRangeshape used in the query, so the active metrics query will be refreshed reliably.
315-550: Right panel composition and per‑operation wiring look coherent.Resolving
selectedOperationDataby hash plus name (with the empty‑name handling), reusingcreateFilterStateand the currentrange/dateRangefor the “View Traces” link, and layering client usage, deprecated fields, and per‑operation vs global metrics based on selection all look consistent and correct; nothing blocking here.
734-770: Selection‑clearing effect matches the “only when ops change” intent.The effect correctly checks whether the selected operation still exists in
operationsData.operationsand, if not, stripsoperationHash/operationNameentries fromfilterState; scoping the dependency array tooperationsData?.operationsand documenting the rationale keeps it from running on every filter‑state change while still reading the latestcurrentFilterState.
Summary by CodeRabbit
New Features
UI
Tests
Checklist