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 GetOperations with paging/sorting/metric oneof and deprecated-field support, implements server handlers/repositories and tests, and ships a new Operations UI (hooks, components, page) plus UI primitives and layout updates. 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
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-10-01T20:39:16.113ZApplied to files:
📚 Learning: 2025-11-19T11:26:26.657ZApplied to files:
📚 Learning: 2025-11-19T11:44:31.726ZApplied to files:
⏰ 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). (17)
🔇 Additional comments (7)
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: 3
♻️ Duplicate comments (2)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
327-327: Potential invalid syncId construction.This issue was previously raised: If
graphContext?.graph?.namespaceorgraphContext?.graph?.nameis undefined, thesyncIdwill be"undefined-undefined", which could cause issues with chart synchronization. While the query is guarded, this component still renders and constructs the syncId.Consider adding a fallback:
- const syncId = `${graphContext?.graph?.namespace}-${graphContext?.graph?.name}`; + const syncId = graphContext?.graph?.namespace && graphContext?.graph?.name + ? `${graphContext.graph.namespace}-${graphContext.graph.name}` + : 'default-sync-id';
799-799: Unused subtitle prop.This issue was previously raised and extensively discussed: The
subtitleprop is passed toGraphPageLayout, but the component doesn't destructure or render it (seegraph-layout.tsxlines 246-296).Either remove the prop or update
GraphPageLayoutto support and render the subtitle.<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" >
🧹 Nitpick comments (4)
controlplane/test/analytics/get-operations.test.ts (4)
13-31: ClickHouseClient mocking and lifecycle hooks are fine; afterEach clearAllMocks is redundantThe ClickHouseClient mock is wired correctly and shared across instances, and the DB setup/teardown via
beforeAllSetup/afterAllSetuplooks good. You can drop the extravi.clearAllMocks()inafterEach, since it already runs inbeforeEach, to avoid redundant work per test.- afterEach(() => { - vi.clearAllMocks(); - }); + afterEach(() => { + // no-op for now; kept for potential per-test teardown + });
84-111: Strengthen validation tests by asserting ClickHouse is never queried for invalid requestsThe four validation tests (invalid
limitlow/high, negativeoffset, invaliddateRange) correctly assert error codes/messages and emptyoperations. To fully capture the desired behavior (reject invalid input before touching ClickHouse), you can also assert thatchClient.queryPromisewas never called in these cases.- expect(response.operations).toEqual([]); - - await server.close(); + expect(response.operations).toEqual([]); + expect(chClient.queryPromise).not.toHaveBeenCalled(); + + await server.close(); @@ - expect(response.operations).toEqual([]); - - await server.close(); + expect(response.operations).toEqual([]); + expect(chClient.queryPromise).not.toHaveBeenCalled(); + + await server.close(); @@ - expect(response.operations).toEqual([]); - - await server.close(); + expect(response.operations).toEqual([]); + expect(chClient.queryPromise).not.toHaveBeenCalled(); + + await server.close(); @@ - expect(response.operations).toEqual([]); - - await server.close(); + expect(response.operations).toEqual([]); + expect(chClient.queryPromise).not.toHaveBeenCalled(); + + await server.close();Also applies to: 113-140, 142-169, 171-202
392-431: Client-name filter test is fine; consider a stronger differential caseThis test at least ensures that passing
clientNamesdoesn’t break the endpoint and returns operations. For even higher signal, you might later extend it to a differential scenario (e.g., stub multiple operations but only one matching client name, similar to your deprecated-fields-only test) so the assertion proves the filter actually affects which rows are returned.
528-583: Deprecated-fields handling and filtering are well coveredThese two tests together do a good job: the first asserts
hasDeprecatedFieldsis set whenincludeDeprecatedFieldsis true, and the second verifies thatincludeOperationsWithDeprecatedFieldsOnlyfilters the list down to only those operations. If you want to go further, you could add an assertion in the second test that operations not inmockDeprecatedFieldswould havehasDeprecatedFields === falsewhenincludeOperationsWithDeprecatedFieldsOnlyis false, but what you have already exercises the key behavior.Also applies to: 585-649
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/test/analytics/get-operations.test.ts(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/hooks/use-operations-filters.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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.tsxcontrolplane/test/analytics/get-operations.test.ts
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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
📚 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 (2)
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/components/analytics/use-apply-params.tsx (1)
useApplyParams(4-25)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(22724-22762)AnalyticsFilter(5960-6010)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(6-88)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(49-253)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)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/operations/operation-content-modal.tsx (1)
OperationContentModal(25-87)controlplane/src/core/bufservices/analytics/getOperations.ts (1)
getOperations(27-287)controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
getOperations(722-864)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
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)
⏰ 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). (3)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
🔇 Additional comments (13)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (6)
76-155: LGTM - Well-implemented toolbar with proper query invalidation.The date range handling and query invalidation logic correctly includes both
rangeanddateRangeparameters, matching the query call pattern. Good use ofisFetchingfor loading states.
193-230: Excellent metric handling with proper oneof field mapping.The
computedOperationsmemo correctly handles the backend's oneof metric field structure, mapping each case (latency, requestCount, errorPercentage) to the appropriate type. The requestCount conversion at line 207 properly prevents NaN by checking for a value before callingNumber().Based on learnings
383-423: Well-structured query and error handling.The
getGraphMetricsquery is properly configured withplaceholderData: keepPreviousDatafor smooth transitions, appropriate refetch settings, and comprehensive error handling with retry functionality. Good UX patterns.
584-604: Excellent debounced search implementation.The search implementation uses a clean pattern:
- Local state (
localSearchQuery) for immediate input feedback- Debounced value (
debouncedSearchQuery) to avoid excessive URL updates- Bidirectional sync: URL → local (lines 588-590) and debounced → URL (lines 600-604)
- Automatic page reset on search change (line 602)
This provides great UX while keeping the URL as the single source of truth.
660-672: Smart optimistic pagination approach.The
noOfPagescalculation implements a clever optimistic pagination strategy: if a full page is returned, it assumes there might be more data and showspageNumber + 1. This works well for infinite data without requiring a total count from the backend.
713-737: Validation effect with clear intent.The useEffect correctly validates that the selected operation still exists in the filtered results, clearing the selection if not. The effect intentionally only depends on
operationsData?.operations(notselectedOperationorapplyParams) to run only when operations data changes, as explained by the comment at line 712.controlplane/test/analytics/get-operations.test.ts (7)
41-66: ERR_ANALYTICS_DISABLED path is covered cleanlyThis test nicely exercises the “analytics disabled” branch by omitting
chClientfromSetupTestand asserting both the status code and emptyoperationslist; the setup with a real federated graph ensures you’re hitting the intended path.
68-82: Missing federated graph (ERR_NOT_FOUND) behavior looks correctThe NOT_FOUND scenario validates code, error details, and that
operationsis empty for a non-existent graph name; this is a good high-signal check of the lookup failure path.
204-232: Empty-operations case is well coveredMocking
queryPromiseto return an empty array and assertingOKplus an emptyoperationslist is a straightforward check that the happy path behaves sensibly when ClickHouse has no rows.
286-336: Request-count metric test aligns with BigInt expectationsThis test nicely verifies both the sort-by-requests behavior and that
metric.case === 'requestCount'withmetric.valueasBigInt(1000). That matches the generated proto’s 64‑bit integer semantics and the existing studio convention of converting BigInt tonumberonly in the UI layer.Based on learnings
338-390: Error-percentage metric coverage looks solidThe error-percentage test checks the correct winner (
Operation1), validates the metric oneof (case === 'errorPercentage') and the numeric value10, giving good confidence in the error-based ranking path.
651-691: searchQuery handling test is straightforward and adequateStubbing a matching operation and asserting a single result with the expected name validates that passing
searchQuerydoesn’t regress the success path. Given the other coverage you have, this seems sufficient here.
693-750: Operation-type mapping test nicely exercises enum conversionThis test cleanly covers mapping of
'query' | 'mutation' | 'subscription'strings from ClickHouse into theGetOperationsResponse_OperationTypeenum values, which is an easy place for regressions; the assertions on all three entries look spot on.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/analytics/get-operations.test.ts (1)
41-219: Consider extracting a shared helper for graph + subgraph test setupMost tests repeat the same sequence:
SetupTest,createThenPublishSubgraph, andcreateFederatedGraphwith a generated name/label. Pulling this into a small local helper (e.g.,setupFederatedGraphWithSubgraph(dbname, chClient)) would reduce duplication, keep individual tests focused on the behavior under test, and make future changes to the setup (extra labels, schema tweaks, etc.) easier to apply in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/analytics/get-operations.test.ts(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 (1)
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)
⏰ 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). (16)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
controlplane/test/analytics/get-operations.test.ts (1)
750-828: Stronger default paging assertions look goodThe default limit/offset tests now assert concrete expectations (exact length and first operation name), which properly exercise the intended defaults instead of allowing trivially passing results. This is a solid improvement in test strength and documentation of behavior.
…eckbox position and update related components
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
studio/src/components/analytics/data-table-primary-filter-menu.tsx (1)
46-64: Boolean filters may not render correctly on mobile.The mobile view (Lines 46-64) uses
DataTableFilterCommandsfor all filters, including those withCustomOptions.Boolean. However, the desktop view (Lines 68-85) has special handling for Boolean filters usingDropdownMenuCheckboxItem. This inconsistency could cause Boolean filters to render incorrectly or not function properly on mobile devices.Consider adding a conditional check in the mobile view similar to the desktop implementation:
{filters.map((filter, index) => { + if (filter.customOptions === CustomOptions.Boolean) { + return ( + <div key={index.toString()} className="p-2"> + <label className="flex items-center space-x-2"> + <input + type="checkbox" + checked={filter.selectedOptions && filter.selectedOptions.length > 0} + onChange={(e) => { + filter.onSelect?.(e.target.checked ? ["true"] : undefined); + }} + /> + <span>{filter.title}</span> + </label> + </div> + ); + } return ( <AccordionItem value={index.toString()} key={index.toString()}>Alternatively, enhance
DataTableFilterCommandsto handleCustomOptions.Booleaninternally.
🧹 Nitpick comments (1)
studio/src/components/operations/operations-search.tsx (1)
66-75: Consider adding error handling for the clients query.The
useQueryhook doesn't handle potential errors. If the query fails, the component silently falls back to an empty clients array, leaving users unaware of why the Clients filter is empty.Consider destructuring
isErrororerrorfrom the query result and conditionally rendering a message or disabling the filter:- const { data: clientsData } = useQuery( + const { data: clientsData, isError } = useQuery( getClientsFromAnalytics, { namespace: graphContext?.graph?.namespace, federatedGraphName: graphContext?.graph?.name, }, { enabled: !!graphContext?.graph?.name, }, );Then use
isErrorin the filter rendering to show a placeholder or disable the Clients filter when the query fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
studio/src/components/analytics/data-table-primary-filter-menu.tsx(4 hunks)studio/src/components/operations/operations-search.tsx(1 hunks)studio/src/components/ui/dropdown-menu.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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/components/operations/operations-search.tsx
🧬 Code graph analysis (3)
studio/src/components/operations/operations-search.tsx (7)
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)studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)studio/src/components/analytics/use-apply-params.tsx (1)
useApplyParams(4-25)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(6-88)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getClientsFromAnalytics(2608-2617)studio/src/components/analytics/filters.tsx (2)
AnalyticsFilter(9-19)AnalyticsFilters(26-34)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
AnalyticsFilter(5960-6010)
studio/src/components/analytics/data-table-primary-filter-menu.tsx (3)
studio/src/components/ui/dropdown-menu.tsx (2)
DropdownMenuContent(205-205)DropdownMenuCheckboxItem(207-207)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)
studio/src/components/ui/dropdown-menu.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). (17)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
studio/src/components/operations/operations-search.tsx (3)
88-171: LGTM! Well-structured filter and handler logic.The handlers properly manage state synchronization with URL params. Good defensive checks like
pageNumber !== 1 ? "1" : nullprevent unnecessary URL updates. ThehandleResetFilterscorrectly clears all related state including operation selection.
173-252: LGTM! Accessible and well-structured UI.The rendering logic is clean with proper accessibility attributes. Past review feedback has been addressed (clear button and sort toggle now have proper sizing and aria-labels).
1-47: LGTM! Clean imports and type definitions.The component interface is well-defined with proper TypeScript types. The
SORT_OPTIONSconstant provides a clean mapping between the proto enum and user-facing labels.studio/src/components/analytics/data-table-primary-filter-menu.tsx (2)
26-44: LGTM! Clean className composition.The optional
classNameprop is properly typed and composed using thecnutility, allowing external styling while preserving the defaultw-56width.
68-85: LGTM! Boolean filter correctly implemented for desktop.The conditional rendering for
CustomOptions.Booleanproperly usesDropdownMenuCheckboxItemwith right-positioned checkbox and correct state handling. ThepreventDefaulton Line 80 correctly prevents the menu from closing on toggle.studio/src/components/ui/dropdown-menu.tsx (1)
99-133: LGTM! Well-implemented checkbox positioning feature.The
checkboxPositionprop is correctly implemented with:
- Proper TypeScript typing (Lines 101-103)
- Backward-compatible default value of
"left"(Line 106)- Consistent padding adjustments (Line 113)
- Correct conditional rendering of children and indicator (Lines 119-130)
- Proper absolute positioning of the checkbox indicator (Lines 121-124)
The implementation maintains existing behavior by default while enabling flexible checkbox placement for new use cases.
…Name in OperationsRightPanel
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)
314-571: Right panel is well-implemented with proper state handling.The component correctly handles loading, error, and empty states. The metrics filtering logic and chart rendering are solid. The modal integration and navigation links work well.
Line 328: The
syncIdconstruction could produce"undefined-undefined"ifgraphContextis missing, though the query is guarded by theenabledflag elsewhere. Consider adding a fallback or conditional construction if chart sync reliability is critical.
📜 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:44:31.726Z
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.726Z
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
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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
🧬 Code graph analysis (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (9)
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/components/analytics/use-apply-params.tsx (1)
useApplyParams(4-25)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(6-88)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)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/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). (16)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (4)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (4)
77-156: LGTM! Toolbar component is well-structured.The date range handling, refresh invalidation, and interval controls are implemented correctly. The invalidation logic properly matches the query shape with both
rangeanddateRangeparameters.
158-312: LGTM! Left panel implementation is solid.The operations mapping correctly handles the oneof metric field, and the BigInt to Number conversion at line 208 properly prevents NaN values. The mobile sheet pattern provides good UX across device sizes.
Based on learnings
573-796: Excellent page architecture with clean state management.The URL-driven state approach with debounced search is well-implemented. The effect at lines 603-607 correctly syncs search state to URL with page reset. The operation validation effect at lines 716-740 appropriately runs only when operations data changes, as discussed in previous reviews.
Pagination logic, operation selection, and error handling are all solid.
798-812: Layout configuration is correct.The
getLayoutpattern properly wraps the page withGraphPageLayout.Note: The
subtitleprop at line 802 is passed but not rendered byGraphPageLayout(it's in the interface but not destructured in the component implementation). This was previously discussed—if the subtitle display is desired, the component would need an update to actually render it.
| const operationNameFilter = | ||
| req.operationName === undefined ? '' : `AND OperationName = '${req.operationName.replace(/'/g, "''")}'`; | ||
|
|
||
| const query = ` |
There was a problem hiding this comment.
Please abstract business logic into repositories so the handler remains primarily free of it.
| optional int32 offset = 5; // pagination offset | ||
| optional bool includeDeprecatedFields = 6; | ||
| optional OperationsFetchBasedOn fetchBasedOn = 7; // determines which metric to fetch and sort by | ||
| optional string sortDirection = 13; // asc, desc - defaults to desc |
There was a problem hiding this comment.
Should we use an enum here?
…to include hasDeprecatedFields and sortDirection properties
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
proto/wg/cosmo/platform/v1/platform.proto (2)
2894-2908:⚠️ CRITICAL: Confirm status of breaking field change at line 2897 (duplicate issue).The earlier review flagged that field 3 changed from
optional string clientNametorepeated string clientNames. This is a breaking protobuf wire-format change. Old clients will fail to parse new messages.Action required: Either:
- Revert field 3 to
optional string clientName, addrepeated string clientNamesat a new field number, and mark the old field as[deprecated=true]- Confirm this is an intentional breaking change requiring coordinated migration and document it in release notes
All other new fields (offset, includeHasDeprecatedFields, fetchBasedOn, etc.) use new field numbers and are safe additions.
2910-2931:⚠️ CRITICAL: Confirm status of field reassignment in Operation (line 2921) (duplicate issue).The earlier review flagged that the
typefield was moved from field number 5 to field number 4. This is a breaking protobuf wire-format change; old clients will misinterpret the new message structure.Recommended remediation:
- Restore
typeto field number 5 and mark with[deprecated=true]- Move
hasDeprecatedFields(currently at 5) to a new field number (e.g., 14)- OR document this as a major version-bump breaking change requiring coordinated client/server deployment
The new
OperationTypeenum andoneof metricstructure are well-designed and pose no issues.connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
22825-22860:GetOperationsResponse.Operationmetric oneof is well-shaped; re‑confirmtypetag for protobuf compatibilityThe new shape of
GetOperationsResponse_Operationis internally consistent:
contentis optional string at tag3,typeis a non-optional enum at tag4,hasDeprecatedFieldsis optional at tag5,latency,requestCount, anderrorPercentageform a propermetriconeof at tags6–8, with the union property correctly initialized.However, there is already an earlier review comment pointing out that
typeappears at tag4here but historically used a different tag number in this message, which would be a wire-level breaking change for existing clients if true. Please make sure the underlying.protokeeps the original field number fortypeand only assigns new, previously unused tag numbers tocontent,hasDeprecatedFields, and the metric fields, then regenerate this file.Also applies to: 22872-22877
🧹 Nitpick comments (8)
controlplane/src/core/bufservices/analytics/getOperations.ts (1)
124-148: Schema parsing during request could impact performance with large schemas.The deprecated fields logic parses and builds the GraphQL schema synchronously during the request. For federated graphs with large schemas, this could increase response time.
Since this is opt-in (only when
includeHasDeprecatedFieldsis true) and has proper error handling, it's acceptable for now. If performance becomes an issue, consider caching the parsed schema/deprecated fields list or moving this computation to a background job.controlplane/src/core/repositories/analytics/MetricsRepository.ts (2)
744-749: Case-insensitive search prevents index usage.The search filter uses
lower(OperationName) LIKE '%...'andlower(OperationHash) LIKE '%...', which prevents ClickHouse from using indexes on these columns. For analytics queries over aggregated metrics tables, this is likely acceptable, but if search becomes slow, consider exact-match search or prefix-only patterns for better performance.
835-952: Consider extracting common query building logic.The three query branches (latency, requests, errors) have significant structural duplication:
- Similar WITH clause structure
- Similar deprecated fields CTE and join logic
- Similar final SELECT and ORDER BY patterns
While the different data sources (operation_latency_metrics_5_30 vs operation_request_metrics_5_30) make some duplication unavoidable, consider extracting helper functions for:
- Building the deprecated fields CTE
- Building the final SELECT with join
- Building the WHERE clauses
This would improve maintainability and reduce the risk of inconsistencies when making future changes.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (3)
4954-4957:GetOperationContentRequest.nameaddition is correct; consider clarifying selection rulesAdding
nameas an optional string at tag4with matching field metadata is wire-safe and follows the generated pattern.If the intent is that callers provide either
hashor(federatedGraphName, namespace, name)(but not arbitrary combinations), it might be worth tightening this at the proto level (e.g., aoneofgrouping) or at least documenting which combination is expected so server-side resolution is unambiguous.Also applies to: 4967-4971
22631-22699: ExpandedGetOperationsRequestfilters look coherent; double‑check time range semanticsThe new fields (
clientNames,limit,offset,includeHasDeprecatedFields,fetchBasedOn,includeContent,range,dateRange,searchQuery,includeOperationsWithDeprecatedFieldsOnly,sortDirection) are all mapped with consistent tags and correct scalar/enum/message types in the field list, which matches the intended feature set.If
range(hours) anddateRangeare intended as alternative ways to specify the time window, you may want to model them as aoneofin the proto or clearly document which takes precedence when both are set; the current shape allows both to be populated simultaneously.Also applies to: 22711-22721
23020-23348: New GetOperationClients/GetOperationDeprecatedFields messages align with usage; verify a couple of semantics**The new request/response pairs and nested types are structurally sound and match the provided Go protobuf definitions (field numbers, types, and comments all line up), including:
GetOperationClientsRequestwithfederatedGraphName,namespace,operationHash,operationName, plus optionalrangeanddateRange.GetOperationClientsResponseand nestedClientwithrequestCountasint64(backed byprotoInt64.zero), andlastUseddocumented as ISO 8601.GetOperationDeprecatedFieldsRequest/Responseand nestedDeprecatedFieldwith the expected string fields.A few semantics to double‑check before relying on these in clients:
- In all three request types (
GetOperationsRequest,GetOperationClientsRequest,GetOperationDeprecatedFieldsRequest),rangeanddateRangeare independent optionals. If they’re meant to be mutually exclusive ways of specifying the time window, consider modelling them as aoneofor clearly documenting precedence when both are set.operationNameis required (non‑optional) inGetOperationClientsRequestbut optional inGetOperationDeprecatedFieldsRequest. If that asymmetry isn’t intentional, you may want to align them at the proto level and regenerate.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
333-333: Consider guarding against undefined values in syncId.Line 333 constructs
syncIddirectly from nullable properties. IfgraphContext?.graph?.namespaceorgraphContext?.graph?.nameis undefined, the result will be"undefined-undefined", which might create an invalid Recharts sync group.Apply this diff to use a fallback or conditional construction:
- 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 already accept
syncId?: string, so passingundefinedwhen the graph context isn't available is safe and prevents creating an invalid sync ID.
807-821: Remove unusedsubtitleprop or update GraphPageLayout to render it.Line 811 passes
subtitle="Comprehensive view into GraphQL Operations Performance"toGraphPageLayout, but based on the component definition ingraph-layout.tsx(lines 246-296), thesubtitleprop is not destructured or rendered. While it's declared inTitleLayoutProps, the implementation ignores it.Choose one of these options:
Option 1: Remove the unused prop (simpler, if subtitle isn't needed):
getGraphLayout( <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" >Option 2: Update GraphPageLayout to render subtitle (if the subtitle is needed):
Modifystudio/src/components/layout/graph-layout.tsxto destructure and render thesubtitleprop.Based on past review comments.
📜 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 (9)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(9 hunks)controlplane/src/core/bufservices/analytics/getOperationClients.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperations.ts(5 hunks)controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)controlplane/test/analytics/get-operations.test.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(6 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/bufservices/analytics/getOperationClients.ts
🧰 Additional context used
🧠 Learnings (6)
📚 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
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
controlplane/test/analytics/get-operations.test.ts
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
connect/src/wg/cosmo/platform/v1/platform_pb.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:
controlplane/src/core/bufservices/analytics/getOperations.ts
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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.tsxcontrolplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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.tsxcontrolplane/src/core/repositories/analytics/MetricsRepository.ts
🧬 Code graph analysis (7)
studio/src/components/operations/operations-search.tsx (5)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)studio/src/components/analytics/use-apply-params.tsx (1)
useApplyParams(4-25)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(6-89)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getClientsFromAnalytics(2608-2617)studio/src/components/analytics/filters.tsx (2)
AnalyticsFilter(9-19)AnalyticsFilters(26-34)
studio/src/hooks/use-operations-filters.tsx (1)
studio/src/components/analytics/use-apply-params.tsx (1)
useApplyParams(4-25)
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)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (36)
CustomOptions(174-174)CustomOptions(206-208)CustomOptions(210-212)CustomOptions(219-221)OperationsFetchBasedOn(657-657)OperationsFetchBasedOn(689-691)OperationsFetchBasedOn(693-695)OperationsFetchBasedOn(702-704)SortDirection(706-706)SortDirection(735-737)SortDirection(739-741)SortDirection(748-750)DateRange(7299-7308)DateRange(7323-7323)DateRange(7338-7340)GetOperationClientsRequest(27871-27882)GetOperationClientsRequest(27897-27897)GetOperationClientsRequest(27912-27914)GetOperationClientsResponse(27958-27965)GetOperationClientsResponse(27980-27980)GetOperationClientsResponse(27995-27997)Response(856-864)Response(879-879)Response(894-896)GetOperationClientsResponse_Client(30530-30539)GetOperationClientsResponse_Client(30554-30554)GetOperationClientsResponse_Client(30569-30571)GetOperationDeprecatedFieldsRequest(28013-28024)GetOperationDeprecatedFieldsRequest(28039-28039)GetOperationDeprecatedFieldsRequest(28054-28056)GetOperationDeprecatedFieldsResponse(28100-28107)GetOperationDeprecatedFieldsResponse(28122-28122)GetOperationDeprecatedFieldsResponse(28137-28139)GetOperationDeprecatedFieldsResponse_DeprecatedField(30601-30609)GetOperationDeprecatedFieldsResponse_DeprecatedField(30624-30624)GetOperationDeprecatedFieldsResponse_DeprecatedField(30639-30641)
controlplane/src/core/bufservices/analytics/getOperations.ts (8)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
SubgraphRepository(80-2135)controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
UsageRepository(12-482)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/core/util.ts (1)
validateDateRanges(346-380)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 (2)
AnalyticsFilter(5980-6030)GetOperationsResponse_Operation(22813-22895)
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/components/analytics/use-apply-params.tsx (1)
useApplyParams(4-25)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(6-89)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(49-261)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)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)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
controlplane/src/core/clickhouse/client/ClickHouseClient.ts (1)
query(268-272)
⏰ 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). (16)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
997-1063: LGTM! Well-structured client lookup with proper parameterization.The
getClientsOfOperationmethod correctly:
- Uses parameterized queries to prevent SQL injection
- Conditionally includes operationName filter when provided
- Aggregates request counts by client/version
- Converts timestamps to ISO format
- Returns consistent result shape
proto/wg/cosmo/platform/v1/platform.proto (5)
636-641: Backward-compatible addition of optional name field.The new
namefield at line 640 uses a previously-unused field number and is correctly marked optional, maintaining compatibility with existing clients.
819-823: Safe enum extension for Boolean options.The new
Boolean = 2enum value is a straightforward, backward-compatible addition to theCustomOptionsenum used for filter configuration.
2883-2892: Well-designed new enums follow proto best practices.The
OperationsFetchBasedOnandSortDirectionenums properly encapsulate metric and sort options, replacing what could have been string literals. This addresses the earlier review feedback to use enums.
2946-2983: Well-structured new messages for operation insights.The new
GetOperationClientsRequest/ResponseandGetOperationDeprecatedFieldsRequest/Responsemessages are cleanly designed with proper use of proto3 conventions:
- Clear field numbering and no collisions
- Appropriate use of
optionalfor conditional fields- Helpful inline comments (e.g., "ISO 8601 timestamp", "range in hours")
- Nested message types (Client, DeprecatedField) follow idiomatic naming
No backward-compatibility concerns.
3390-3397: Well-formed new RPCs with clear semantics.The two new RPC methods follow proto conventions:
- Correct message type mapping (request → response)
- Clear documentation comments describing purpose
- Proper placement within PlatformService alongside related operations
Ensure corresponding server handlers exist and include proper authorization/authentication checks for these new analytics endpoints.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
104-114: NewCustomOptions.Booleanenum entry is consistent and wire-safe
Boolean = 2is appended with a unique tag and the enum metadata table is updated accordingly, so this addition is backward compatible and matches the existing generated style.
409-453:OperationsFetchBasedOnandSortDirectionenums are correctly declared and registeredThe new enums use contiguous numeric values (
REQUESTS/LATENCY/ERRORSandASC/DESC) and are properly wired viaproto3.util.setEnumType, which aligns with how enums are used later inGetOperationsRequest.studio/src/hooks/use-operations-filters.tsx (1)
1-89: LGTM! Hook implementation is clean and follows best practices.The hook properly manages URL-driven operations filters with:
- Memoized callbacks (applyDeprecatedFieldsFilter, applySorting)
- Safe enum conversion with defaults
- Clear separation of concerns
Past review concerns about filter manipulation have been addressed.
studio/src/components/operations/operations-search.tsx (1)
49-260: LGTM! Component implementation follows best practices.The OperationsSearch component properly:
- Fetches and displays client filter options
- Implements accessible controls (aria-labels on icon buttons)
- Handles pagination reset when filters change
- Memoizes expensive computations
All past review concerns have been addressed.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (3)
78-157: LGTM! Toolbar implementation is correct.The refresh invalidation properly includes both
rangeanddateRangewith correct conditional logic, matching the query shape used elsewhere. This ensures reliable cache invalidation.
159-317: LGTM! Left panel implementation handles data normalization correctly.The metric handling properly:
- Uses oneof field switching for different metric types
- Converts BigInt to Number safely (acceptable per team decision)
- Provides sensible defaults (0) for missing values
- Implements mobile-responsive design with sheet
578-805: LGTM! Main page logic is well-structured.The OperationsPage component properly:
- Manages URL-driven state with debounced search
- Syncs local input with URL for responsive UX
- Handles pagination with offset calculation
- Validates selected operation when data changes
- Implements proper loading and error states
- Uses placeholderData: keepPreviousData for smooth transitions
The intentional useEffect dependency choices (lines 725-749) are justified per past discussions and align with the intended behavior of validating operations only on data fetch.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
336-345: Add input validation for field names and type names.Good job adding single-quote escaping! However, the previous review requested input validation to ensure field names and type names match GraphQL identifier rules. As a defense-in-depth measure, validate each field name and type name against the GraphQL spec pattern before building the array.
Add validation before the array construction:
// Validate field names and type names match GraphQL identifier rules const graphQLIdentifierPattern = /^[_A-Za-z][_0-9A-Za-z]*$/; for (const field of deprecatedFields) { if (!graphQLIdentifierPattern.test(field.name)) { throw new Error(`Invalid field name: ${field.name}`); } for (const typeName of field.typeNames) { if (!graphQLIdentifierPattern.test(typeName)) { throw new Error(`Invalid type name: ${typeName}`); } } } // Build the deprecated fields array // Each deprecated field is represented as a tuple: (name, typeNames_array) // Escape single quotes const deprecatedFieldsArray = deprecatedFields .map((field) => { const escapedName = field.name.replace(/'/g, "''"); const quotedTypeNames = field.typeNames.map((tn) => `'${tn.replace(/'/g, "''")}'`).join(', '); return `('${escapedName}', [${quotedTypeNames}])`; }) .join(', ');Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/UsageRepository.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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/src/core/repositories/analytics/UsageRepository.ts
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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:
controlplane/src/core/repositories/analytics/UsageRepository.ts
🧬 Code graph analysis (1)
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)
⏰ 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). (2)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
controlplane/src/core/repositories/analytics/UsageRepository.ts (5)
306-330: LGTM!The method signature is well-typed, and the early return for empty deprecated fields is a good optimization.
332-334: LGTM!Time filter parsing correctly uses the
parseTimeFiltersutility.
347-374: LGTM!Excellent use of parameterized queries for all filterable parameters. The CTE structure is clean and the conditional handling of optional
operationNameis well-implemented.
376-388: LGTM!Parameter object construction and conditional
operationNamehandling are implemented correctly.
390-397: LGTM!Result processing includes appropriate defensive checks and handles edge cases gracefully.
…rgraph/cosmo into suvij/eng-5256-operations-overview
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/analytics/get-operations.test.ts (1)
479-528: Consider asserting the ClickHouse query call count for consistency.The test at line 530 asserts
expect(chClient.queryPromise).toHaveBeenCalledTimes(1)whenincludeContentis false, which verifies the implementation optimization of skipping the content query. For symmetry and better documentation of the expected behavior, this test could similarly assert thatqueryPromisewas called exactly twice (once for operations, once for content).Add this assertion after line 525:
expect(response.response?.code).toBe(EnumStatusCode.OK); expect(response.operations).toHaveLength(1); expect(response.operations[0]?.content).toBe('query { hello }'); + +// Should call ClickHouse twice: once for operations, once for content +expect(chClient.queryPromise).toHaveBeenCalledTimes(2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/analytics/get-operations.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
controlplane/test/analytics/get-operations.test.ts
🧬 Code graph analysis (1)
controlplane/test/analytics/get-operations.test.ts (4)
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)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (8)
OperationsFetchBasedOn(657-657)OperationsFetchBasedOn(689-691)OperationsFetchBasedOn(693-695)OperationsFetchBasedOn(702-704)GetOperationsResponse_OperationType(752-752)GetOperationsResponse_OperationType(784-786)GetOperationsResponse_OperationType(788-790)GetOperationsResponse_OperationType(797-799)
⏰ 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). (16)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (1)
controlplane/test/analytics/get-operations.test.ts (1)
1-906: Excellent test coverage and structure!This test suite is comprehensive and well-organized, covering:
- Error conditions (analytics disabled, not found, invalid parameters, invalid date ranges)
- Core functionality (sorting by latency/requests/errors, filtering, search, pagination)
- Optional features (content inclusion, deprecated fields handling)
- Edge cases (empty results, different operation types, default parameters)
The tests demonstrate good practices:
- Proper test isolation using unique IDs and mocking
- Clear, descriptive test names that document expected behavior
- Consistent patterns across related tests
- Appropriate cleanup with
server.close()- Strong assertions that verify specific values rather than loose checks
The resolution of past review comments (stronger assertions for default pagination, accurate "(default)" labeling) shows attention to test quality.
…arameterized values for organizationId and federatedGraphId, and improve search query handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (2)
756-806: Clarifydeprecated_fieldsCTE construction to reduce string‑templating complexityThe
deprecatedFieldsCte/deprecatedFieldsJoinconstruction is functionally sound (names and type names are escaped, and joins are conditionally applied), but the combination of:
- building a multi‑line CTE string with a trailing comma, and
- weaving it into the
WITHclause via overlapping ternariesmakes the final SQL a bit hard to reason about and fragile to future edits.
Consider refactoring along these lines:
- Build an array of CTE fragments (
['deprecated_fields AS (...)', 'operations_with_deprecated_fields AS (...)', 'ops AS (...)']) andjoin(',\n')them, instead of manually juggling commas.- Keep the deprecated‑fields CTE text separate from the core
opsCTE, so you don’t need${deprecatedFieldsCte ? ',' : ''}/${deprecatedFieldsCte ? '' : ','}tricks.That would preserve behavior but make it much easier to verify (and adjust) the CTE chain when you touch this query again.
Also applies to: 829-835
999-1065:getClientsOfOperationquery is solid; consider minor robustness tweaksThe new
getClientsOfOperationimplementation is generally in good shape:
- Uses parameter binding for all untrusted inputs (org/graph IDs, hash, optional name, timestamps).
- Aggregates by
ClientName/ClientVersion, withsum(TotalRequests)andmax(Timestamp)and orders bylastUsed DESC, which matches the intended “most recently active first” semantics.- Maps the response to a clean DTO and normalizes
lastUsedto ISO vianew Date(client.lastUsed + 'Z').toISOString(), which aligns with the expectation that the DB returns aYYYY-MM-DD HH:MM:SSstyle timestamp. Based on learnings, treatinglastUsedas always present without a fallback is acceptable.Two small follow‑ups you might consider:
Date‑range normalization parity
Other methods callflipDateRangeValuesIfNeeded(or go throughgetMetricsProps) to ensurestart <= end. If the call‑sites ofgetClientsOfOperationdon’t already enforce this, you may want to guard here too for symmetry and resilience.Optional server‑side limit
Depending on expected cardinality, you may want to apply aLIMIT(or pagination) to avoid accidentally returning an unbounded number of client rows for very hot operations on long ranges.Neither is blocking, but worth considering to keep this in line with the rest of the repository’s patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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/src/core/repositories/analytics/MetricsRepository.ts
⏰ 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). (17)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
808-818: Result shape andhasDeprecatedFieldsmapping look consistentThe result typing and mapping in
getOperationslooks coherent:
limitOffsetClauseandqueryParamsWithPaginationpush pagination and filtering fully into ClickHouse.- Each branch (latency / requests / errors) returns a consistent row shape, with unused numeric fields set to
0, which aligns with the|| 0fallbacks in the mapper.- Computing
hasDeprecatedFieldsasIF(dep.operationHash IS NOT NULL AND dep.operationHash != '', 1, 0)and then mapping viaop.hasDeprecatedFields === 1is robust even when the deprecated‑fields CTE is not present (constant0path).No changes needed here from my side.
Also applies to: 820-835, 874-875, 909-910, 947-948, 957-965
…ationsResponse, and implement total count retrieval in getOperations function
…onsistent casing and improve SQL query ordering
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
proto/wg/cosmo/platform/v1/platform.proto (2)
2897-2908: Breaking change already flagged in previous review.Field 3 was changed from
optional string clientNametorepeated string clientNames, which is a breaking protobuf change. This issue was already identified in previous review comments.The new optional fields (offset, includeHasDeprecatedFields, fetchBasedOn, includeContent, range, dateRange, searchQuery, includeOperationsWithDeprecatedFieldsOnly, sortDirection, includeTotalCount) are backwards compatible additions.
2918-2932: Breaking changes already flagged in previous review.The restructuring of
GetOperationsResponse.Operationincludes breaking changes that were previously identified:
- The
typefield moved from position 5 to 4- Field number 5 is now reused for
hasDeprecatedFieldsAdditionally, field 3 (
content) has been made optional, and a newoneof metricstructure has been added. These changes break wire compatibility with existing clients.connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
22844-22892: Duplicate: Field number change fortypefield breaks protobuf compatibility.This issue was thoroughly investigated in a previous review comment with extensive verification. The
typefield assignment to field number 4 represents a breaking protobuf change (it was previously at field number 5 per the earlier analysis).As noted in the previous review, the fix must be applied to the source
.protofile (not this generated TypeScript file) by restoringtypeto field number 5 and reassigning the new fields (hasDeprecatedFieldsand the metric oneof members) to previously unused field numbers.controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
722-737: SanitizefetchBasedOn/sortDirectionbefore injecting into SQL ORDER BY.This issue was previously flagged:
sortDirectionis interpolated directly intoORDER BYclauses without validation, creating a SQL injection vector.Per the previous review, constrain both parameters to known unions:
fetchBasedOn:'latency' | 'requests' | 'errors'(default'latency')sortDirection:'ASC' | 'DESC'(default'DESC')Map and validate them before building SQL to prevent injection and ensure type safety at call sites.
Also applies to: 877-880, 912-915, 950-953
🧹 Nitpick comments (2)
controlplane/src/core/bufservices/analytics/getOperations.ts (1)
124-148: Acceptable implementation with graceful error handling.The deprecated fields detection logic is only executed when explicitly requested and includes error handling. If schema parsing fails, the operation continues with an empty deprecated fields list, which is an acceptable graceful degradation.
Note: For federated graphs with very large schemas, the schema parsing and field extraction (lines 133-138) could be computationally expensive. Consider caching the deprecated fields list if this endpoint experiences performance issues.
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
1165-1166: Consider usingUInt32type hint for timestamp parameters.The query uses
{startTimestamp:String}and{endTimestamp:String}but passes numeric timestamp values (lines 1182-1183). While ClickHouse will coerce these correctly, using{startTimestamp:UInt32}and{endTimestamp:UInt32}would be more direct and consistent with the other queries in this file (e.g., lines 842, 886, 922, 1034).WITH - toDateTime({startTimestamp:String}) AS startDate, - toDateTime({endTimestamp:String}) AS endDate + toDateTime({startTimestamp:UInt32}) AS startDate, + toDateTime({endTimestamp:UInt32}) AS endDate
📜 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 (5)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(11 hunks)controlplane/src/core/bufservices/analytics/getOperations.ts(5 hunks)controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(6 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/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
connect/src/wg/cosmo/platform/v1/platform_pb.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:
controlplane/src/core/bufservices/analytics/getOperations.ts
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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/src/core/repositories/analytics/MetricsRepository.ts
🧬 Code graph analysis (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (7)
OperationsFetchBasedOn(657-657)OperationsFetchBasedOn(689-691)OperationsFetchBasedOn(693-695)OperationsFetchBasedOn(702-704)DateRange(7299-7308)DateRange(7323-7323)DateRange(7338-7340)controlplane/src/types/index.ts (1)
DateRange(630-633)
controlplane/src/core/bufservices/analytics/getOperations.ts (5)
controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/core/constants.ts (1)
deafultRangeInHoursForGetOperations(11-11)controlplane/src/core/util.ts (1)
validateDateRanges(346-380)composition/src/ast/utils.ts (1)
parse(272-274)composition/src/buildASTSchema/buildASTSchema.ts (1)
buildASTSchema(26-72)
⏰ 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). (16)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (23)
controlplane/src/core/bufservices/analytics/getOperations.ts (11)
12-26: LGTM: Imports are appropriate for the new features.The new imports support the extended functionality for operations retrieval: enum types for sorting/fetching, schema parsing for deprecated fields detection, additional repositories for analytics retention and usage data, and the validation utility for date ranges.
50-52: LGTM: Repository initialization follows established patterns.The new repository instances are initialized consistently with the existing code and have access to the necessary dependencies from
opts.
66-87: LGTM: Offset validation is correct.The offset is properly defaulted to 0 and validated to ensure it's non-negative, with appropriate error handling for invalid values.
89-96: LGTM: Backwards-compatible enum mapping.The
fetchBasedOnparameter correctly defaults toLATENCYfor backwards compatibility, and the enum-to-string mapping is clear and correct.
98-119: LGTM: Date range validation with organization-specific retention.The code correctly retrieves the organization's analytics retention limit and validates the requested date range against it, with appropriate fallback to defaults and error handling for invalid ranges.
150-163: LGTM: Client name filtering with OR semantics.The code correctly creates multiple
EQUALSfilters for each client name, which will be combined with OR logic. The comment clearly documents this behavior.
165-183: LGTM: Sort direction and repository call are correct.The sort direction is properly mapped with a sensible default (descending), and the
getOperationscall includes all the new parameters for the extended functionality.
194-205: LGTM: Efficient conditional content fetching.Operation content is only fetched when explicitly requested, avoiding unnecessary database queries. The operation hashes are extracted from the already-retrieved operations for efficient lookup.
208-253: LGTM: Operation objects correctly structured with oneof metric.The operation objects are built with conditional fields based on request flags, and the
metriconeof is correctly structured according to thefetchBasedOnparameter. This follows the protobuf pattern for oneof fields.
255-267: LGTM: Conditional total count with consistent filters.The total count is only computed when explicitly requested, using the same filter criteria as the main operations query. This separate count query is the expected pattern for pagination support.
269-276: LGTM: Response correctly structured.The response correctly includes the operations array and conditionally includes the total count when requested, matching the protobuf schema definition.
proto/wg/cosmo/platform/v1/platform.proto (5)
640-640: LGTM: Backwards-compatible field addition.Adding the optional
namefield toGetOperationContentRequestis backwards compatible in protobuf, as it uses a new field number (4) and is optional.
822-822: LGTM: Backwards-compatible enum value addition.Adding the
Booleanvalue to theCustomOptionsenum is backwards compatible, as it uses the next sequential value (2).
2883-2892: LGTM: Well-defined new enums.Both
OperationsFetchBasedOnandSortDirectionenums are properly structured with clear, descriptive values starting at 0, which is correct for proto3.
2948-2985: LGTM: Well-structured new message definitions.The new
GetOperationClientsandGetOperationDeprecatedFieldsmessage pairs are well-designed with proper field numbering, consistent structure, and appropriate use of optional fields. The nested message definitions follow protobuf best practices.
3396-3399: LGTM: Backwards-compatible RPC additions.The new
GetOperationClientsandGetOperationDeprecatedFieldsRPCs are added at the end of thePlatformService, which is good practice for backwards compatibility. They properly reference the new request/response message types.connect/src/wg/cosmo/platform/v1/platform_pb.ts (6)
101-115: LGTM: Clean enum extension.The addition of
Boolean = 2to theCustomOptionsenum is correctly structured and backward compatible.
409-453: LGTM: Well-defined enums for operations filtering and sorting.Both
OperationsFetchBasedOnandSortDirectionenums are correctly structured with clear semantic values.
4954-4970: LGTM: Backward-compatible optional field addition.Adding the optional
namefield toGetOperationContentRequestis correctly implemented.
22628-22730: LGTM: Comprehensive backward-compatible request extension.The extensive additions to
GetOperationsRequestprovide rich filtering, pagination, and sorting capabilities. All new fields are properly marked optional or are repeated arrays, maintaining backward compatibility.Based on learnings, the
clientNamesrepeated field is correctly initialized with an empty array per protobuf conventions.
22760-22779: LGTM: Clean addition of pagination metadata.Adding optional
totalCountfield toGetOperationsResponseis backward compatible and useful for pagination.
23034-23362: LGTM: Well-structured new RPC message definitions.The new message definitions for
GetOperationClientsandGetOperationDeprecatedFieldsRPCs are properly structured with:
- Consistent request/response patterns
- Appropriate field types and numbering
- Clear field documentation (e.g., ISO 8601 timestamp for
lastUsed)- Correct protobuf boilerplate generation
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
1140-1206: Well-structured method with proper parameterization.The
getClientsOfOperationmethod correctly uses parameterized queries throughout, handles the optionaloperationNameparameter safely, and formats the ISO timestamp properly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (2)
843-844: Critical CTE syntax error remains unresolved.The conditional comma logic still produces invalid SQL when
deprecatedFieldsis empty. WhendeprecatedFields.length === 0,deprecatedFieldsCteis an empty string, which causes:
- Line 843:
endDate(no trailing comma, since empty string is falsy)- Line 844:
,(standalone comma, since empty string is falsy)This generates:
WITH toDateTime(...) AS startDate, toDateTime(...) AS endDate , ops AS (The standalone comma creates a syntax error.
Apply this fix to all three query branches (lines 843-844, 887-888, 923-924):
- toDateTime({endDate:UInt32}) AS endDate${deprecatedFieldsCte ? ',' : ''} - ${deprecatedFieldsCte}${deprecatedFieldsCte ? '' : ','} + toDateTime({endDate:UInt32}) AS endDate, + ${deprecatedFieldsCte}Since
deprecatedFieldsCtealready includes a trailing comma when set (line 792), you only need the comma afterendDate.Also applies to: 887-888, 923-924
1035-1036: Critical CTE syntax error (same as in getOperations).The same conditional comma logic error exists here. When
deprecatedFieldsCteis a non-empty string, this works, but the pattern is fragile and produces the same invalid SQL structure as ingetOperations.Apply the same fix as in
getOperations:- toDateTime({endDate:UInt32}) AS endDate${deprecatedFieldsCte ? ',' : ''} - ${deprecatedFieldsCte}${deprecatedFieldsCte ? '' : ','} + toDateTime({endDate:UInt32}) AS endDate, + ${deprecatedFieldsCte}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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/src/core/repositories/analytics/MetricsRepository.ts
controlplane/src/core/repositories/analytics/MetricsRepository.ts
Outdated
Show resolved
Hide resolved
…mproved readability and consistency
…rgraph/cosmo into suvij/eng-5256-operations-overview
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)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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/src/core/repositories/analytics/MetricsRepository.ts
⏰ 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). (17)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (2)
722-971: Well-structured query with proper parameterization.The
getOperationsmethod correctly:
- Sanitizes
sortDirectionto only'ASC' | 'DESC'(line 735)- Uses parameterized queries throughout to prevent SQL injection
- Properly handles the deprecated fields CTE with correct comma placement
- Escapes single quotes in deprecated field names (line 763-764)
The three-branch structure (latency/requests/errors) has some duplication but maintains clarity.
1140-1206: Clean implementation with proper parameterization.The
getClientsOfOperationmethod correctly:
- Uses
:UInt32type hints for timestamp parameters (lines 1165-1166)- Parameterizes all query inputs to prevent SQL injection
- Handles optional
operationNameparameter conditionally (line 1177, 1189-1191)- Converts ClickHouse timestamps to ISO format with UTC timezone (line 1204)
…oper comma placement for improved clarity
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
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.657Z
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:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
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.726Z
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/src/core/repositories/analytics/MetricsRepository.ts
⏰ 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_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_test
…revent unnecessary queries and improve performance
Summary by CodeRabbit
New Features
UI
Backend
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist