Skip to content

Conversation

@JivusAyrus
Copy link
Member

@JivusAyrus JivusAyrus commented Nov 13, 2025

Summary by CodeRabbit

  • New Features

    • Added analytics endpoints to list operation clients and retrieve deprecated fields; new fetch/sort enums and total-count support.
  • UI

    • New Operations page (toolbar, searchable/sortable/paginated list), operation content modal, client-usage and deprecated-fields tables, reusable scroll area, and layout className; metric cards gain display controls.
  • Backend

    • Richer analytics filters (time, clients, deprecated-only), pagination, multiple metric modes, deprecated-field detection, per-operation clients/content, and count endpoints.
  • Tests

    • Comprehensive test suites for operations, clients, and deprecated-fields.
  • Chores

    • Dev tooling update (prettier).

✏️ Tip: You can customize this high-level summary in your review settings.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.
image image image

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 13, 2025

Walkthrough

Adds two new PlatformService RPCs (GetOperationClients, GetOperationDeprecatedFields), expands GetOperations with paging/sorting/metric oneof and deprecated-field support, implements server-side analytics handlers/repositories and tests, and ships a new Operations UI with hooks, components, layout and small UI primitives.

Changes

Cohort / File(s) Change Summary
Proto & generated clients
proto/wg/cosmo/platform/v1/platform.proto, connect/src/wg/cosmo/platform/v1/platform_pb.ts, connect/src/wg/cosmo/platform/v1/platform_connect.ts, connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts
Added enums OperationsFetchBasedOn, SortDirection; expanded GetOperationsRequest/Response (paging, clientNames, fetchBasedOn, sortDirection, includeContent, searchQuery, deprecated-field flags, metric oneof, totalCount); added messages and RPCs for GetOperationClients and GetOperationDeprecatedFields.
Service wiring
controlplane/src/core/bufservices/PlatformService.ts
Registered and exported handlers getOperationClients and getOperationDeprecatedFields; added corresponding imports.
New analytics handlers
controlplane/src/core/bufservices/analytics/getOperationClients.ts, controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts
New router functions implementing auth, analytics-enabled checks, date-range validation, ClickHouse queries (clients) and schema parsing/pruning (deprecated fields), mapping results to protobuf responses and error handling.
Analytics & content handlers
controlplane/src/core/bufservices/analytics/getOperations.ts, controlplane/src/core/bufservices/analytics/getOperationContent.ts
Extended getOperations to support offset/fetchBasedOn/sortDirection/includeContent/includeHasDeprecatedFields/searchQuery/includeTotalCount and deprecated-field filtering; parameterized getOperationContent queries and added optional operationName filter.
Repositories — metrics & usage
controlplane/src/core/repositories/analytics/MetricsRepository.ts, controlplane/src/core/repositories/analytics/UsageRepository.ts
Expanded MetricsRepository.getOperations for pagination/sorting/search/deprecated-fields; added getOperationsCount and getClientsOfOperation. Added UsageRepository.getDeprecatedFieldsUsedInOperation.
Controlplane util
controlplane/src/core/util.ts
validateDateRanges: added end-date guard to reject invalid or too-old end dates.
Tests — analytics
controlplane/test/analytics/get-operations.test.ts, controlplane/test/analytics/get-operation-clients.test.ts, controlplane/test/analytics/get-operation-deprecated-fields.test.ts
New comprehensive tests mocking ClickHouse covering error paths and success cases: analytics-disabled, not-found graphs, invalid date ranges/offsets, escaping/parameterization, deprecated-field flows, sorting, pagination, and content inclusion.
Studio — Operations UI & pages
studio/src/pages/.../operations.tsx, studio/src/components/operations/*
studio/src/components/operations/client-usage-table.tsx, studio/src/components/operations/deprecated-fields-table.tsx, studio/src/components/operations/operation-content-modal.tsx, studio/src/components/operations/operations-list.tsx, studio/src/components/operations/operations-search.tsx
New Operations page and components: searchable/sortable operations list, client-usage and deprecated-fields tables, operation content modal; components call new RPCs and implement selection/search/sort/pagination UX.
Studio — Hooks & filters
studio/src/hooks/use-operations-filters.tsx
Added useOperationsFilters for URL-backed operation filters (includeOperationsWithDeprecatedFieldsOnly, clientNames, searchQuery, fetchBasedOn/sortDirection mapping and actions).
Studio — Layout & UI primitives
studio/src/components/layout/graph-layout.tsx, studio/src/components/ui/scroll-area.tsx
Added "Operations" nav item; GraphPageLayout accepts className; introduced ScrollArea and ScrollBar components.
Studio — Analytics & minor UI changes
studio/src/components/analytics/metrics.tsx, studio/src/components/analytics/field-usage.tsx, studio/src/components/analytics/data-table-primary-filter-menu.tsx, studio/src/components/analytics/filters.tsx
Metrics cards gain showTopList and chartClassName props; FieldUsage no longer clears range URL param on close; primary filter menu gains boolean-option rendering and className plumbing; AnalyticsFilters accepts className.
Studio — Dropdown/Menu
studio/src/components/ui/dropdown-menu.tsx
DropdownMenuCheckboxItem gains `checkboxPosition?: "left"
Misc & deps
package.json
DevDependency bump: prettier ^3.0.3 → ^3.6.2.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing extra attention:
    • ClickHouse query construction, parameterization, CTEs and multi-branch logic in MetricsRepository and UsageRepository.
    • Date-range validation, analytics-retention enforcement, and edge-case guards in validateDateRanges and its callers.
    • Schema retrieval/parsing and SchemaGraphPruner usage for deprecated-field detection and mapping.
    • Proto ↔ server ↔ UI mapping for the metric oneof, OperationsFetchBasedOn/SortDirection, pagination (offset/limit), and hasDeprecatedFields.
    • Tests: ensure mocked ClickHouse responses align with new parameterized queries and branching.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: operations overview' clearly identifies the main change as adding an operations overview feature. It is concise and directly related to the substantial new functionality introduced across the pull request (new operations page, components, API endpoints).
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aad3944 and 1b5d19d.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 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
⏰ 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: build_push_image
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_test
  • GitHub Check: integration_test (./events)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: Analyze (go)

Comment @coderabbitai help to get the list of available commands and usage tips.

@JivusAyrus
Copy link
Member Author

Adding tests

@github-actions
Copy link

github-actions bot commented Nov 13, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-5b678d4d81a164970aab6eb21e4e68c8e92199e7

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 compatibility

The proto file shows a wire-incompatible change. Between commit a494f01b~1 and a494f01b, field numbers in GetOperationsResponse.Operation were reassigned:

  • Field 4: was float latency → now OperationType type (enum)
  • Field 5: was OperationType type → now bool hasDeprecatedFields
  • Fields 6–8: latency moved to 6; requestCount and errorPercentage added at 7–8

Older 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 latency at 4, type at 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: Escape searchQuery before building RegExp to avoid regex injection and runtime errors

highlightText currently does:

const regex = new RegExp(`(${query})`, "gi");

with query coming 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 RegExp entirely), 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 series type.

The series: any[] type could be more specific. Based on the useChartData hook 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 fixing justify-end container

Extending TitleLayoutProps with className and piping it through cn(...) on the header container is a nice touch. One small nit: the items wrapper uses className="justify-end" without a flex or grid display, so that utility currently has no effect. If the intent is to right-align the items content, 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 limit and offset, validating bounds, and rejecting negative offsets is a solid guardrail for the analytics backend.
  • Mapping fetchBasedOnrequests/errors/latency with a default to latency keeps behavior backward compatible if the client doesn’t set the new enum.
  • Using analytics-retention via OrganizationRepository together with validateDateRanges is a good way to cap the lookback window without surprising callers.

Two small follow-ups to consider:

  • Given inputRange falls back to deafultRangeInHoursForGetOperations when both req.range and req.dateRange are unset, the if (!range && !dateRange) branch is effectively unreachable; you could either remove it or make it explicitly guard only truly invalid combinations.
  • When shouldFetchAll is true, you still pass limit: req.limit alongside fetchAll: true. If MetricsRepository.getOperations doesn’t already ignore limit when fetchAll is set, it might be safer to have the service pass limit: 0 or undefined in 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 run

Using the latest valid schema + SchemaGraphPruner.getAllFields({ onlyDeprecated: true }) and then UsageRepository.getOperationsUsingDeprecatedFields to derive a (hash,name) set is a clean way to drive both the hasDeprecatedFields flag and the deprecated-only filtering behavior. The Set-based lookup and in-memory pagination when includeDeprecatedFields is true are also straightforward.

If this endpoint is hit very frequently, you might want to double-check whether you truly need hasDeprecatedFields on 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 like req.includeDeprecatedFields || req.includeOperationsWithDeprecatedFieldsOnly to 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 on

The construction of operationData (name/hash/type/content/hasDeprecatedFields) and the metric oneof based on fetchBasedOn lines 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 propagating undefined if 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 consistent

The new GetOperationClients* and GetOperationDeprecatedFields* messages are shaped consistently with the rest of the analytics API (hash + name plus optional range/dateRange), and the corresponding RPCs on PlatformService clearly separate these concerns from the bulk GetOperations call.

Since these endpoints are strictly read-only analytics, you may want to align them with other analytics RPCs like GetAnalyticsView/GetGraphMetrics by marking them with option 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 helper

The single‑quote escaping for hash and optional name is 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 possible

The deprecated‑fields filter is correctly hooked into AnalyticsFilters via filtersList and handleDeprecatedFieldsFilterSelect. To simplify and avoid the as unknown as string cast, you could pass a plain string operator that matches what optionConstructor expects, 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 for deprecatedFieldsArray

The new methods correctly guard on deprecatedFields.length === 0 and reuse parseTimeFilters, but deprecatedFieldsArray is built via raw string interpolation of field.name and field.typeNames into 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 for fetchBasedOn and sortDirection used in SQL

The getOperations logic is sound, but fetchBasedOn / sortField and sortDirection are plain strings while they directly influence which query branch runs and the ORDER BY clause:

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 BY clause from unexpected tokens if these props are ever passed from less trusted layers.

studio/src/components/operations/operations-list.tsx (1)

23-34: Align selectedOperation typing with null/undefined handling

The selection logic explicitly handles selectedOperation?.name being null or undefined:

selectedOperation?.name === null ||
selectedOperation?.name === undefined
  ? true
  : selectedOperation?.name === operationName;

But OperationsListProps declares:

selectedOperation: {
  hash: string;
  name: string;
} | undefined;

So callers can’t actually pass null for name without 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: Extended GetOperationsRequest filters are wired consistently, but watch default handling

The added pagination/filtering fields (offset, includeDeprecatedFields, fetchBasedOn, sortDirection, includeContent, range, dateRange, searchQuery, includeOperationsWithDeprecatedFieldsOnly) are all present both as properties and in the fields list 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 undefined by 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: 0 and gcTime: 0 means data is immediately stale and garbage collected when unmounted. This can cause:

  1. Excessive refetching when navigating back to this page
  2. Loss of cached data benefits
  3. Potential performance issues

This conflicts with placeholderData: keepPreviousData on 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 query

The query wiring and mapping look correct, but two small points worth tightening:

  • dateRange is dereferenced when range is falsy; if useAnalyticsQueryState ever returns range falsy with an undefined dateRange, this will throw. A defensive null check (or asserting both are always set in the hook) would make this safer.
  • You already destructure error from useQuery, but the table treats “error” and “no data” the same (“No deprecated fields found”). Surfacing an error state (similar to OperationContentModal) would avoid silently hiding backend/transport issues.
studio/src/hooks/use-operations-filters.tsx (1)

28-57: Consider using router.replace and explicit pathname when updating query params

applyNewParams currently 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.replace to avoid bloating browser history.
  • Explicitly setting pathname: router.pathname for 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 validation

This handler is well-aligned with existing analytics patterns:

  • Auth + org scoping is done before repository usage, and FederatedGraphRepository.byName ensures 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 neither range nor dateRange is provided and respects the analytics-retention limit.
  • SQL uses explicit escaping for OperationHash and OperationName, and the result mapping to BigInt requestCount and ISO lastUsed looks correct.

If you want to be extra defensive, you could:

  • Reject or coerce negative/zero range values before calling validateDateRanges, to avoid surprising windows.
  • Document the implicit 24h default (or move it into validateDateRanges for 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

📥 Commits

Reviewing files that changed from the base of the PR and between ac085ab and ec2b42d.

⛔ Files ignored due to path filters (2)
  • connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go is excluded by !**/*.pb.go, !**/gen/**
  • connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.go is 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.ts
  • controlplane/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 className prop is correctly destructured and applied using the cn() utility, allowing external styling while preserving default styles.

Also applies to: 379-379


454-455: LGTM! Well-designed flexibility enhancement.

The addition of showTopList and chartClassName props 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 ErrorMetricsCard and ErrorPercentChart components now follow the same flexibility pattern. The InfoTooltip reformatting 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 patterns

Using useWorkspace to derive namespace.name and passing it both to getFederatedGraphByName and into basePath keeps routing and data-loading in sync with how WorkspaceSelector already models namespaces. No issues spotted here.


132-137: “Operations” nav item wiring looks correct

The new “Operations” entry reuses the basePath, points to /operations, and uses matchExact: 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-structured

The 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 ScrollBar is clean.

studio/src/components/analytics/field-usage.tsx (1)

71-75: Using workspace namespace in FieldUsage keeps analytics links consistent

Pulling namespace.name from useWorkspace and feeding it into the traces link query (namespace param) matches how the rest of the Studio UI models namespaces and avoids relying solely on router.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

getOperationClients and getOperationDeprecatedFields are 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 safe

Auth, 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, the dateRange object is always initialized with a default value ({ start: subHours(new Date(), parsedRange), end: new Date() }) before any conditionals. The logic ensures that:

  • When query.dateRange exists: range becomes undefined, dateRange is set to the parsed value
  • When query.dateRange doesn't exist: range is set to parsedRange, dateRange retains its default

There is no code path where range is falsy and dateRange is undefined. The hook guarantees that dateRange is always defined.

Likely an incorrect or invalid review comment.

connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)

403-427: New OperationsFetchBasedOn enum wiring looks correct

The enum values and setEnumType metadata 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: Optional name on GetOperationContentRequest is correctly modeled

The new name?: string field 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 definitions

The TS definitions for:

  • GetOperationClientsRequest (federatedGraphName, namespace, operationHash, operationName, optional range and dateRange), and
  • GetOperationClientsResponse plus nested Client (name, version, int64 requestCount, lastUsed ISO8601 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 correct

The request/response and nested DeprecatedField are wired as expected:

  • Request: federatedGraphName, namespace, operationHash, optional operationName, range, dateRange.
  • Response: response plus repeated deprecatedFields with fieldName, typeName, path.

Field numbers and optionality match the provided Go definitions, and the TS fields metadata 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 graphContext in this file use optional chaining (?.), which safely handles the nullable GraphContextProps | undefined type. 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 consistently

The additions for GetOperationClients and GetOperationDeprecatedFields (imports plus method descriptors) match the generated message types and follow the existing pattern for unary analytics RPCs on PlatformService. 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

getOperationClients and getOperationDeprecatedFields are exported with the expected localName, RPC name, unary kind, and corresponding request/response types, consistent with the rest of the connect-query surface.

Also applies to: 2619-2650

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec2b42d and 016bf48.

📒 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, not escapeRegExp. 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)

693-707: Minor: subtitle prop is not rendered by GraphPageLayout.

The subtitle prop (line 697) is passed to GraphPageLayout, but as confirmed in past review discussions, while it's declared in TitleLayoutProps, the component doesn't destructure or render it.

Either:

  1. Remove the subtitle prop if it's not needed
  2. Update GraphPageLayout in studio/src/components/layout/graph-layout.tsx to render the subtitle

Based 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 = 7 to sortDirection = 13, then back to includeContent = 8. While protobuf allows non-sequential numbering, this pattern suggests sortDirection may 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?.namespace or graphContext?.graph?.name is undefined during initial render, syncId will be "undefined-undefined". While the query is guarded by enabled: !!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 undefined gracefully 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:

  • applySearchQueryuseCallback([applyNewParams]) (use-operations-filters.tsx:132)
  • applyNewParamsuseCallback([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.push directly or extracting only the push method with useRef:

const routerPushRef = useRef(router.push);
useEffect(() => {
  routerPushRef.current = router.push;
}, [router.push]);

Then update useOperationsFilters to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 016bf48 and a508272.

📒 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 where splice(-1, 1) would remove the last element.


454-525: LGTM: Metric card enhancements are well-designed.

The addition of showTopList and chartClassName props 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 for type.

The type field moved from position 5 to position 4 in GetOperationsResponse.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 metric design is clean and appropriate for the use case where exactly one metric is returned based on fetchBasedOn.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sortDirection as "desc" | "asc" instead of string.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a508272 and b6a5266.

📒 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2f88f5d and 2768c7a.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is 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 parseTimeFilters utility.


347-374: LGTM!

Excellent use of parameterized queries for all filterable parameters. The CTE structure is clean and the conditional handling of optional operationName is well-implemented.


376-388: LGTM!

Parameter object construction and conditional operationName handling are implemented correctly.


390-397: LGTM!

Result processing includes appropriate defensive checks and handles edge cases gracefully.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) when includeContent is 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 that queryPromise was 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2768c7a and 5a593dd.

📒 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
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (2)

756-806: Clarify deprecated_fields CTE construction to reduce string‑templating complexity

The deprecatedFieldsCte / deprecatedFieldsJoin construction 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 WITH clause via overlapping ternaries

makes 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 (...)']) and join(',\n') them, instead of manually juggling commas.
  • Keep the deprecated‑fields CTE text separate from the core ops CTE, 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: getClientsOfOperation query is solid; consider minor robustness tweaks

The new getClientsOfOperation implementation is generally in good shape:

  • Uses parameter binding for all untrusted inputs (org/graph IDs, hash, optional name, timestamps).
  • Aggregates by ClientName/ClientVersion, with sum(TotalRequests) and max(Timestamp) and orders by lastUsed DESC, which matches the intended “most recently active first” semantics.
  • Maps the response to a clean DTO and normalizes lastUsed to ISO via new Date(client.lastUsed + 'Z').toISOString(), which aligns with the expectation that the DB returns a YYYY-MM-DD HH:MM:SS style timestamp. Based on learnings, treating lastUsed as always present without a fallback is acceptable.

Two small follow‑ups you might consider:

  1. Date‑range normalization parity
    Other methods call flipDateRangeValuesIfNeeded (or go through getMetricsProps) to ensure start <= end. If the call‑sites of getClientsOfOperation don’t already enforce this, you may want to guard here too for symmetry and resilience.

  2. Optional server‑side limit
    Depending on expected cardinality, you may want to apply a LIMIT (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

📥 Commits

Reviewing files that changed from the base of the PR and between 5a593dd and ef5191c.

📒 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 and hasDeprecatedFields mapping look consistent

The result typing and mapping in getOperations looks coherent:

  • limitOffsetClause and queryParamsWithPagination push 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 || 0 fallbacks in the mapper.
  • Computing hasDeprecatedFields as IF(dep.operationHash IS NOT NULL AND dep.operationHash != '', 1, 0) and then mapping via op.hasDeprecatedFields === 1 is robust even when the deprecated‑fields CTE is not present (constant 0 path).

No changes needed here from my side.

Also applies to: 820-835, 874-875, 909-910, 947-948, 957-965

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 clientName to repeated 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.Operation includes breaking changes that were previously identified:

  • The type field moved from position 5 to 4
  • Field number 5 is now reused for hasDeprecatedFields

Additionally, field 3 (content) has been made optional, and a new oneof metric structure 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 for type field breaks protobuf compatibility.

This issue was thoroughly investigated in a previous review comment with extensive verification. The type field 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 .proto file (not this generated TypeScript file) by restoring type to field number 5 and reassigning the new fields (hasDeprecatedFields and the metric oneof members) to previously unused field numbers.

controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)

722-737: Sanitize fetchBasedOn / sortDirection before injecting into SQL ORDER BY.

This issue was previously flagged: sortDirection is interpolated directly into ORDER BY clauses 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 using UInt32 type 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

📥 Commits

Reviewing files that changed from the base of the PR and between ef5191c and b3d7500.

⛔ Files ignored due to path filters (1)
  • connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go is 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 fetchBasedOn parameter correctly defaults to LATENCY for 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 EQUALS filters 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 getOperations call 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 metric oneof is correctly structured according to the fetchBasedOn parameter. 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 name field to GetOperationContentRequest is 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 Boolean value to the CustomOptions enum is backwards compatible, as it uses the next sequential value (2).


2883-2892: LGTM: Well-defined new enums.

Both OperationsFetchBasedOn and SortDirection enums 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 GetOperationClients and GetOperationDeprecatedFields message 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 GetOperationClients and GetOperationDeprecatedFields RPCs are added at the end of the PlatformService, 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 = 2 to the CustomOptions enum is correctly structured and backward compatible.


409-453: LGTM: Well-defined enums for operations filtering and sorting.

Both OperationsFetchBasedOn and SortDirection enums are correctly structured with clear semantic values.


4954-4970: LGTM: Backward-compatible optional field addition.

Adding the optional name field to GetOperationContentRequest is correctly implemented.


22628-22730: LGTM: Comprehensive backward-compatible request extension.

The extensive additions to GetOperationsRequest provide rich filtering, pagination, and sorting capabilities. All new fields are properly marked optional or are repeated arrays, maintaining backward compatibility.

Based on learnings, the clientNames repeated field is correctly initialized with an empty array per protobuf conventions.


22760-22779: LGTM: Clean addition of pagination metadata.

Adding optional totalCount field to GetOperationsResponse is backward compatible and useful for pagination.


23034-23362: LGTM: Well-structured new RPC message definitions.

The new message definitions for GetOperationClients and GetOperationDeprecatedFields RPCs 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 getClientsOfOperation method correctly uses parameterized queries throughout, handles the optional operationName parameter safely, and formats the ISO timestamp properly.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 deprecatedFields is empty. When deprecatedFields.length === 0, deprecatedFieldsCte is 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 deprecatedFieldsCte already includes a trailing comma when set (line 792), you only need the comma after endDate.

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 deprecatedFieldsCte is a non-empty string, this works, but the pattern is fragile and produces the same invalid SQL structure as in getOperations.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b3d7500 and cea8593.

📒 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cea8593 and 603e288.

📒 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 getOperations method correctly:

  • Sanitizes sortDirection to 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 getClientsOfOperation method correctly:

  • Uses :UInt32 type hints for timestamp parameters (lines 1165-1166)
  • Parameterizes all query inputs to prevent SQL injection
  • Handles optional operationName parameter conditionally (line 1177, 1189-1191)
  • Converts ClickHouse timestamps to ISO format with UTC timezone (line 1204)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 603e288 and 0cb7d67.

📒 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
Copy link
Contributor

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@ce57515). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2331   +/-   ##
=======================================
  Coverage        ?   31.34%           
=======================================
  Files           ?      336           
  Lines           ?    33612           
  Branches        ?      251           
=======================================
  Hits            ?    10535           
  Misses          ?    22156           
  Partials        ?      921           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1)

340-342: syncId can become "undefined-undefined" when graph context is missing.

If graphContext?.graph?.namespace or graphContext?.graph?.name is undefined, syncId will be the string "undefined-undefined", which could cause unintended chart synchronization across different views that also have undefined context.

Apply this fix to ensure a valid syncId:

  const graphContext = useContext(GraphContext);
- const syncId = `${graphContext?.graph?.namespace}-${graphContext?.graph?.name}`;
+ const syncId = graphContext?.graph?.namespace && graphContext?.graph?.name
+   ? `${graphContext.graph.namespace}-${graphContext.graph.name}`
+   : undefined;

The metric cards should handle undefined gracefully, or you can use a fallback like "default" if a string is always required.

🧹 Nitpick comments (1)
studio/src/components/operations/client-usage-table.tsx (1)

90-101: Consider extracting the column count as a constant.

The table has 5 columns defined in the header (lines 94-98), and colSpan={5} is used in multiple places (lines 107, 114, 159, 173). Extracting this as a constant would improve maintainability if columns are added/removed in the future.

+const COLUMN_COUNT = 5;
+
 export const ClientUsageTable = ({
   // ...
 }) => {
   // ... inside the component
-  <TableCell colSpan={5} className="h-24 text-center">
+  <TableCell colSpan={COLUMN_COUNT} className="h-24 text-center">
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 24b8dda and aad3944.

📒 Files selected for processing (2)
  • studio/src/components/operations/client-usage-table.tsx (1 hunks)
  • studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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
  • studio/src/components/operations/client-usage-table.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
  • studio/src/components/operations/client-usage-table.tsx
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.

Applied to files:

  • studio/src/components/operations/client-usage-table.tsx
🧬 Code graph analysis (2)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (8)
studio/src/components/layout/graph-layout.tsx (2)
  • GraphContext (56-58)
  • getGraphLayout (227-233)
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/hooks/use-workspace.ts (1)
  • useWorkspace (4-11)
studio/src/components/operations/operation-content-modal.tsx (1)
  • OperationContentModal (25-87)
studio/src/components/analytics/field-usage.tsx (1)
  • FieldUsageSheet (345-430)
studio/src/components/operations/client-usage-table.tsx (3)
studio/src/components/analytics/useAnalyticsQueryState.ts (1)
  • useAnalyticsQueryState (50-125)
controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
  • getOperationClients (15-100)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
  • getOperationClients (2624-2633)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
  • GitHub Check: build-router
  • GitHub Check: image_scan
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: integration_test (./events)
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: build_test
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
🔇 Additional comments (8)
studio/src/components/operations/client-usage-table.tsx (2)

35-79: LGTM! Query setup and data processing are well-implemented.

The component properly:

  • Includes namespace in the enabled condition (line 62)
  • Handles both range and dateRange parameters correctly
  • Extracts refetch for the retry functionality
  • Uses appropriate data transformations with safe fallbacks

Based on learnings, the lastUsed field is always present from the API, and BigInt to Number conversion is acceptable for request counts in this codebase.


111-129: Good error handling with retry mechanism.

The error state correctly checks for both network errors and non-OK response codes, and provides a user-friendly retry button that triggers refetch(). The colSpan={5} correctly matches the 5-column table header.

studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (6)

78-157: LGTM! OperationsToolbar implementation is solid.

The toolbar correctly:

  • Handles both range and custom dateRange for date picker changes
  • Includes matching dateRange in the invalidation query key (lines 136-141), addressing the previous review concern
  • Properly manages refresh interval state

205-217: LGTM! Metric extraction from oneof field is handled correctly.

The switch statement properly handles all three metric cases (latency, requestCount, errorPercentage). The requestCount conversion at line 211 correctly uses a ternary operator to avoid the NaN issue that was flagged in a previous review.


711-736: LGTM! Effect correctly validates selected operation against fetched data.

The effect properly:

  • Only runs when operationsData?.operations changes (line 736)
  • Reads selectedOperation and uses applyParams without including them in deps (intentional, per past discussion)
  • Has an explanatory comment (line 711) justifying the eslint-disable

This pattern is appropriate for validation effects that should run only on data fetch, not on every state change.


623-668: LGTM! Query configuration is well-structured.

The getOperations query properly:

  • Uses keepPreviousData to prevent UI flicker during refetches
  • Sets staleTime: 0 and gcTime: 0 to ensure fresh data on filter changes
  • Handles both range and custom dateRange parameters
  • Includes all necessary filter parameters (clientNames, deprecated fields, search, sort)
  • Uses includeTotalCount for pagination

758-792: Clean two-panel layout composition.

The layout properly:

  • Uses flex with responsive breakpoints for mobile/desktop
  • Handles overflow correctly with overflow-hidden and scrollable containers
  • Includes FieldUsageSheet at the page level for field usage overlay

611-615: I'll help you verify this review comment about the applyParams dependency stability. Let me investigate the useApplyParams hook and the actual implementation to determine if this is a valid concern.
<function_calls>


#!/bin/bash

Find the useApplyParams hook file

fd -t f "use-apply-params" --extension tsx --extension ts


</function_calls>


#!/bin/bash

Check the useApplyParams implementation

rg -nP -A20 'export const useApplyParams|function useApplyParams' --type=ts --type=tsx


</function_calls>


#!/bin/bash

Get context around the useEffect in question

sed -n '600,620p' studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx


</function_calls>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants