-
Notifications
You must be signed in to change notification settings - Fork 202
feat: operations overview #2331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ate related logic in analytics
… states in client usage and deprecated fields tables
WalkthroughAdds 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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-19T11:44:31.726ZApplied to files:
📚 Learning: 2025-11-19T11:26:26.657ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Comment |
|
Adding tests |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this 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 compatibilityThe proto file shows a wire-incompatible change. Between commit
a494f01b~1anda494f01b, field numbers inGetOperationsResponse.Operationwere reassigned:
- Field 4: was
float latency→ nowOperationType type(enum)- Field 5: was
OperationType type→ nowbool hasDeprecatedFields- Fields 6–8:
latencymoved to 6;requestCountanderrorPercentageadded at 7–8Older clients compiled against the previous proto will receive field 4 as an enum but expect it to be a float (latency). This causes data misinterpretation on the wire and breaks compatibility. Protobuf best practice requires never reassigning field numbers—instead, new fields must use fresh, previously unused tags, and deprecated fields should be marked as reserved rather than reused.
If this has been released, affected clients will experience silent deserialization errors or data corruption. Restore the old field numbers (e.g., keep
latencyat 4,typeat 5) and place new fields at previously unused numbers, or issue a breaking change notice if reverting is not viable.
♻️ Duplicate comments (1)
studio/src/components/operations/operations-list.tsx (1)
77-92: EscapesearchQuerybefore buildingRegExpto avoid regex injection and runtime errors
highlightTextcurrently does:const regex = new RegExp(`(${query})`, "gi");with
querycoming from user input. This can:
- Throw on invalid patterns (e.g. unmatched
(,\).- Enable user-controlled regex features (potential ReDoS).
- Matches the CodeQL “regular expression injection” finding.
If you only need literal substring highlighting, escape the query (or avoid
RegExpentirely), e.g.:- const highlightText = (text: string, query: string) => { - if (!query) return text; - - const regex = new RegExp(`(${query})`, "gi"); + const highlightText = (text: string, query: string) => { + if (!query) return text; + + const escapeRegExp = (value: string) => + value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const regex = new RegExp(`(${escapeRegExp(query)})`, "gi");This keeps the UX while removing the injection vector and hardening against malformed input.
🧹 Nitpick comments (17)
studio/src/components/analytics/metrics.tsx (1)
868-891: LGTM! Consistency refactor.The refactoring to destructured props with explicit types improves consistency with other components in the file and enhances maintainability.
Optional: Consider tightening the
seriestype.The
series: any[]type could be more specific. Based on theuseChartDatahook usage and the data structure (containing p50, p90, p99 properties), consider defining a more specific interface for the series items.interface LatencySeriesItem { timestamp: number | string | Date; p50?: number | string; p90?: number | string; p99?: number | string; }Then use:
series: LatencySeriesItem[]This would improve type safety and make the expected data structure clearer.
studio/src/components/layout/graph-layout.tsx (1)
235-245: GraphPageLayout className extension is good; consider fixingjustify-endcontainerExtending
TitleLayoutPropswithclassNameand piping it throughcn(...)on the header container is a nice touch. One small nit: the items wrapper usesclassName="justify-end"without a flex or grid display, so that utility currently has no effect. If the intent is to right-align theitemscontent, consider making it a flex container:- <div className="justify-end">{items}</div> + <div className="flex justify-end">{items}</div>This keeps behavior explicit without affecting existing layouts.
Also applies to: 247-283
controlplane/src/core/bufservices/analytics/getOperations.ts (3)
49-52: Pagination, sorting, and date-range handling look good; a couple of minor nits
- Defaulting
limitandoffset, validating bounds, and rejecting negative offsets is a solid guardrail for the analytics backend.- Mapping
fetchBasedOn→requests/errors/latencywith a default to latency keeps behavior backward compatible if the client doesn’t set the new enum.- Using
analytics-retentionviaOrganizationRepositorytogether withvalidateDateRangesis a good way to cap the lookback window without surprising callers.Two small follow-ups to consider:
- Given
inputRangefalls back todeafultRangeInHoursForGetOperationswhen bothreq.rangeandreq.dateRangeare unset, theif (!range && !dateRange)branch is effectively unreachable; you could either remove it or make it explicitly guard only truly invalid combinations.- When
shouldFetchAllis true, you still passlimit: req.limitalongsidefetchAll: true. IfMetricsRepository.getOperationsdoesn’t already ignorelimitwhenfetchAllis set, it might be safer to have the service passlimit: 0orundefinedin that case to avoid accidentally truncating results before the in-memory filtering/pagination.Also applies to: 64-87, 88-119, 156-176
123-151: Deprecated-fields computation and filtering are coherent; consider when it should runUsing the latest valid schema +
SchemaGraphPruner.getAllFields({ onlyDeprecated: true })and thenUsageRepository.getOperationsUsingDeprecatedFieldsto derive a(hash,name)set is a clean way to drive both thehasDeprecatedFieldsflag and the deprecated-only filtering behavior. The Set-based lookup and in-memory pagination whenincludeDeprecatedFieldsis true are also straightforward.If this endpoint is hit very frequently, you might want to double-check whether you truly need
hasDeprecatedFieldson every response or only when the client explicitly asks for deprecated-field information. If it’s the latter, you could gate the schema build and usage query behind something likereq.includeDeprecatedFields || req.includeOperationsWithDeprecatedFieldsOnlyto avoid extra work on simple “top operations” views.Also applies to: 187-215, 233-235
217-270: Response mapping to GetOperationsResponse.Operation is mostly spot onThe construction of
operationData(name/hash/type/content/hasDeprecatedFields) and the metriconeofbased onfetchBasedOnlines up with the updated response shape and keeps only the relevant metric populated, which should simplify consumers.One minor robustness tweak you could make is to default the latency value in the fallback branch, e.g.
value: operation.latency ?? 0, to avoid propagatingundefinedif the repository ever omits latency for a row.proto/wg/cosmo/platform/v1/platform.proto (1)
2941-2959: New per-operation clients/deprecated-fields RPC surface looks consistentThe new
GetOperationClients*andGetOperationDeprecatedFields*messages are shaped consistently with the rest of the analytics API (hash + name plus optional range/dateRange), and the corresponding RPCs onPlatformServiceclearly separate these concerns from the bulkGetOperationscall.Since these endpoints are strictly read-only analytics, you may want to align them with other analytics RPCs like
GetAnalyticsView/GetGraphMetricsby marking them withoption idempotency_level = NO_SIDE_EFFECTS;to give clients and intermediaries a stronger correctness signal about caching/retries.Also applies to: 2961-2978, 3389-3392
controlplane/src/core/bufservices/analytics/getOperationContent.ts (1)
46-49: Escaping hash and name looks good; consider centralizing this helperThe single‑quote escaping for
hashand optionalnameis correct and consistent with other analytics queries. Since this pattern is repeated across handlers, you might later extract a small shared helper to keep the escaping logic in one place.Also applies to: 55-56
studio/src/components/operations/operations-search.tsx (1)
70-99: Deprecated‑fields filter wiring is correct; small type/ergonomic tweaks possibleThe deprecated‑fields filter is correctly hooked into
AnalyticsFiltersviafiltersListandhandleDeprecatedFieldsFilterSelect. To simplify and avoid theas unknown as stringcast, you could pass a plain string operator that matches whatoptionConstructorexpects, e.g.:- operator: AnalyticsViewFilterOperator.EQUALS as unknown as string, + operator: "EQUALS",This keeps the filter semantics the same while avoiding a noisy cast.
controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
305-454: Avoid ad‑hoc SQL string concatenation fordeprecatedFieldsArrayThe new methods correctly guard on
deprecatedFields.length === 0and reuseparseTimeFilters, butdeprecatedFieldsArrayis built via raw string interpolation offield.nameandfield.typeNamesinto the query. If any of these values ever contain a'or other odd characters (or become more user-controlled over time), this risks malformed SQL and makes the query harder to reason about.Consider centralizing escaping or parameterization here, similar to how other analytics code uses query parameters:
- const deprecatedFieldsArray = deprecatedFields - .map((field) => `('${field.name}', [${field.typeNames.map((tn) => `'${tn}'`).join(', ')}])`) - .join(', '); + const escape = (v: string) => v.replace(/'/g, "''"); + const deprecatedFieldsArray = deprecatedFields + .map( + (field) => + `('${escape(field.name)}', [${field.typeNames + .map((tn) => `'${escape(tn)}'`) + .join(', ')}])`, + ) + .join(', ');Or, if feasible, move this into a shared helper and/or use ClickHouse parameters instead of interpolated literals.
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
722-860: Tighten typing/validation forfetchBasedOnandsortDirectionused in SQLThe
getOperationslogic is sound, butfetchBasedOn/sortFieldandsortDirectionare plainstrings while they directly influence which query branch runs and theORDER BYclause:const sortField = props.fetchBasedOn || 'latency'; const sortDirection = props.sortDirection || 'desc'; … ORDER BY latency ${sortDirection.toUpperCase()} …To prevent accidental invalid values and keep the SQL predictable, it would be safer to narrow these at the type and runtime level, e.g.:
- fetchBasedOn?: string; // 'requests', 'latency', or 'errors' - sortDirection?: string; // 'asc' or 'desc' + fetchBasedOn?: 'requests' | 'latency' | 'errors'; + sortDirection?: 'asc' | 'desc'; … - const sortField = props.fetchBasedOn || 'latency'; - const sortDirection = props.sortDirection || 'desc'; + const sortField: 'requests' | 'latency' | 'errors' = + props.fetchBasedOn ?? 'latency'; + const sortDirection: 'asc' | 'desc' = + props.sortDirection === 'asc' ? 'asc' : 'desc';This keeps the branching explicit and guards the
ORDER BYclause from unexpected tokens if these props are ever passed from less trusted layers.studio/src/components/operations/operations-list.tsx (1)
23-34: AlignselectedOperationtyping with null/undefined handlingThe selection logic explicitly handles
selectedOperation?.namebeingnullorundefined:selectedOperation?.name === null || selectedOperation?.name === undefined ? true : selectedOperation?.name === operationName;But
OperationsListPropsdeclares:selectedOperation: { hash: string; name: string; } | undefined;So callers can’t actually pass
nullfornamewithout breaking the type. To make this consistent and self-documenting, either:
- Allow nullish in the type:
- selectedOperation: { - hash: string; - name: string; - } | undefined; + selectedOperation: + | { + hash: string; + name: string | null | undefined; + } + | undefined;or
- Simplify the selection logic to treat
''as the “unnamed” case and remove the null checks, depending on how upstream data is modeled.This avoids surprises at call sites and keeps the selection semantics explicit.
Also applies to: 255-268
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
22614-22671: ExtendedGetOperationsRequestfilters are wired consistently, but watch default handlingThe added pagination/filtering fields (
offset,includeDeprecatedFields,fetchBasedOn,sortDirection,includeContent,range,dateRange,searchQuery,includeOperationsWithDeprecatedFieldsOnly) are all present both as properties and in thefieldslist with matching numbers and kinds, which looks correct.One thing to double‑check: comments like “defaults to true/desc” aren’t enforced in this generated class (the properties are
undefinedby default). Make sure callers or the server-side logic consistently apply those defaults when these fields are omitted.Also applies to: 22682-22693
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
534-535: Review query cache configuration.Setting
staleTime: 0andgcTime: 0means data is immediately stale and garbage collected when unmounted. This can cause:
- Excessive refetching when navigating back to this page
- Loss of cached data benefits
- Potential performance issues
This conflicts with
placeholderData: keepPreviousDataon line 530, which tries to keep previous data visible during refetches.Consider adjusting these values unless immediate staleness is required:
- staleTime: 0, - gcTime: 0, + staleTime: 30_000, // 30 seconds + gcTime: 5 * 60_000, // 5 minutes
520-521: Consider implementing pagination.The TODO comment indicates pagination support is needed. With a fixed
limit: 100, users cannot view more than 100 operations. This could be limiting for large-scale applications.Would you like me to help implement pagination with URL-based state management that aligns with the existing analytics query state pattern?
studio/src/components/operations/deprecated-fields-table.tsx (1)
38-69: Handle error state and dateRange robustness in deprecated fields queryThe query wiring and mapping look correct, but two small points worth tightening:
dateRangeis dereferenced whenrangeis falsy; ifuseAnalyticsQueryStateever returnsrangefalsy with an undefineddateRange, this will throw. A defensive null check (or asserting both are always set in the hook) would make this safer.- You already destructure
errorfromuseQuery, but the table treats “error” and “no data” the same (“No deprecated fields found”). Surfacing an error state (similar toOperationContentModal) would avoid silently hiding backend/transport issues.studio/src/hooks/use-operations-filters.tsx (1)
28-57: Consider usingrouter.replaceand explicit pathname when updating query params
applyNewParamscurrently does:router.push({ query: { ...q, ...cleanedNewParams, }, });This implicitly reuses the current pathname and adds a new history entry for every filter/sort change. For an analytics view with lots of filter tweaks, you might prefer:
- Using
router.replaceto avoid bloating browser history.- Explicitly setting
pathname: router.pathnamefor clarity.This is optional, but can improve UX when users navigate back/forward through pages.
Also applies to: 166-199
controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
21-71: Server-side operation client analytics look sound; optionally tighten range validationThis handler is well-aligned with existing analytics patterns:
- Auth + org scoping is done before repository usage, and
FederatedGraphRepository.byNameensures graph access is organization-scoped (good fit with previous service-layer auth learnings). Based on learnings.- Date range handling (
inputRange,validateDateRanges,isoDateRangeToTimestamps,getDateRange) gives a sensible default (24h) when neitherrangenordateRangeis provided and respects theanalytics-retentionlimit.- SQL uses explicit escaping for
OperationHashandOperationName, and the result mapping toBigIntrequestCountand ISOlastUsedlooks correct.If you want to be extra defensive, you could:
- Reject or coerce negative/zero
rangevalues before callingvalidateDateRanges, to avoid surprising windows.- Document the implicit 24h default (or move it into
validateDateRangesfor centralization).These are optional hardening tweaks; the current implementation is functionally solid.
Also applies to: 73-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (22)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_connect.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(8 hunks)controlplane/src/core/bufservices/PlatformService.ts(2 hunks)controlplane/src/core/bufservices/analytics/getOperationClients.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperationContent.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts(1 hunks)controlplane/src/core/bufservices/analytics/getOperations.ts(6 hunks)controlplane/src/core/repositories/analytics/MetricsRepository.ts(1 hunks)controlplane/src/core/repositories/analytics/UsageRepository.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(5 hunks)studio/src/components/analytics/field-usage.tsx(1 hunks)studio/src/components/analytics/metrics.tsx(15 hunks)studio/src/components/layout/graph-layout.tsx(6 hunks)studio/src/components/operations/client-usage-table.tsx(1 hunks)studio/src/components/operations/deprecated-fields-table.tsx(1 hunks)studio/src/components/operations/operation-content-modal.tsx(1 hunks)studio/src/components/operations/operations-list.tsx(1 hunks)studio/src/components/operations/operations-search.tsx(1 hunks)studio/src/components/ui/scroll-area.tsx(1 hunks)studio/src/hooks/use-operations-filters.tsx(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
controlplane/src/core/bufservices/analytics/getOperationContent.ts
📚 Learning: 2025-07-30T09:29:04.257Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
Applied to files:
controlplane/src/core/bufservices/analytics/getOperationContent.ts
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
controlplane/src/core/bufservices/analytics/getOperationContent.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tscontrolplane/src/core/bufservices/analytics/getOperations.ts
🧬 Code graph analysis (19)
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (9)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationDeprecatedFields(2640-2649)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
GetOperationDeprecatedFieldsRequest(23164-23228)GetOperationDeprecatedFieldsResponse(23233-23271)controlplane/src/core/util.ts (4)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)validateDateRanges(346-373)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(96-1835)controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
UsageRepository(12-455)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/types/index.ts (1)
Field(741-752)controlplane/src/core/services/SchemaGraphPruner.ts (1)
SchemaGraphPruner(21-404)
studio/src/components/operations/deprecated-fields-table.tsx (4)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationDeprecatedFields(2640-2649)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
getOperationDeprecatedFields(19-120)
connect/src/wg/cosmo/platform/v1/platform_connect.ts (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
GetOperationClientsRequest(22995-23059)GetOperationClientsResponse(23064-23102)GetOperationDeprecatedFieldsRequest(23164-23228)GetOperationDeprecatedFieldsResponse(23233-23271)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (12)
GetOperationClientsRequest(27822-27833)GetOperationClientsRequest(27848-27848)GetOperationClientsRequest(27863-27865)GetOperationClientsResponse(27909-27916)GetOperationClientsResponse(27931-27931)GetOperationClientsResponse(27946-27948)GetOperationDeprecatedFieldsRequest(27964-27975)GetOperationDeprecatedFieldsRequest(27990-27990)GetOperationDeprecatedFieldsRequest(28005-28007)GetOperationDeprecatedFieldsResponse(28051-28058)GetOperationDeprecatedFieldsResponse(28073-28073)GetOperationDeprecatedFieldsResponse(28088-28090)
studio/src/components/operations/operations-search.tsx (3)
studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
AnalyticsFilter(5954-6004)studio/src/components/analytics/filters.tsx (1)
AnalyticsFilters(25-33)
controlplane/src/core/bufservices/PlatformService.ts (2)
controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
getOperationClients(14-115)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
getOperationDeprecatedFields(19-120)
studio/src/hooks/use-operations-filters.tsx (3)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
AnalyticsViewResultFilter(6233-6283)AnalyticsFilter(5954-6004)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (14)
AnalyticsViewResultFilter(7536-7545)AnalyticsViewResultFilter(7560-7560)AnalyticsViewResultFilter(7575-7577)AnalyticsViewFilterOperator(287-287)AnalyticsViewFilterOperator(352-354)AnalyticsViewFilterOperator(356-358)AnalyticsViewFilterOperator(365-367)AnalyticsFilter(7184-7195)AnalyticsFilter(7210-7210)AnalyticsFilter(7225-7227)OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)
studio/src/components/operations/operation-content-modal.tsx (4)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationContent(918-927)controlplane/src/core/bufservices/analytics/getOperationContent.ts (1)
getOperationContent(14-79)studio/src/components/code-viewer.tsx (1)
CodeViewer(82-176)
studio/src/components/ui/scroll-area.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
controlplane/src/core/repositories/analytics/UsageRepository.ts (2)
controlplane/src/types/index.ts (1)
DateRange(630-633)controlplane/src/core/repositories/analytics/util.ts (1)
parseTimeFilters(417-429)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (32)
OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)DateRange(7250-7259)DateRange(7274-7274)DateRange(7289-7291)GetOperationsResponse_OperationType(703-703)GetOperationsResponse_OperationType(735-737)GetOperationsResponse_OperationType(739-741)GetOperationsResponse_OperationType(748-750)GetOperationClientsRequest(27822-27833)GetOperationClientsRequest(27848-27848)GetOperationClientsRequest(27863-27865)GetOperationClientsResponse(27909-27916)GetOperationClientsResponse(27931-27931)GetOperationClientsResponse(27946-27948)Response(807-815)Response(830-830)Response(845-847)GetOperationClientsResponse_Client(30481-30490)GetOperationClientsResponse_Client(30505-30505)GetOperationClientsResponse_Client(30520-30522)GetOperationDeprecatedFieldsRequest(27964-27975)GetOperationDeprecatedFieldsRequest(27990-27990)GetOperationDeprecatedFieldsRequest(28005-28007)GetOperationDeprecatedFieldsResponse(28051-28058)GetOperationDeprecatedFieldsResponse(28073-28073)GetOperationDeprecatedFieldsResponse(28088-28090)GetOperationDeprecatedFieldsResponse_DeprecatedField(30552-30560)GetOperationDeprecatedFieldsResponse_DeprecatedField(30575-30575)GetOperationDeprecatedFieldsResponse_DeprecatedField(30590-30592)connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
Response(683-691)Response(706-706)Response(721-723)
studio/src/components/layout/graph-layout.tsx (2)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/components/dashboard/workspace-selector.tsx (1)
WorkspaceSelector(16-58)
controlplane/src/core/bufservices/analytics/getOperationClients.ts (5)
controlplane/src/core/routes.ts (1)
RouterOptions(25-54)controlplane/src/core/util.ts (4)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)validateDateRanges(346-373)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(96-1835)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/core/repositories/analytics/util.ts (2)
isoDateRangeToTimestamps(353-366)getDateRange(404-415)
studio/src/components/operations/client-usage-table.tsx (4)
studio/src/components/layout/graph-layout.tsx (1)
GraphContext(56-58)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
getOperationClients(14-115)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
getOperationClients(2624-2633)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (3)
controlplane/src/core/bufservices/analytics/getOperationClients.ts (1)
getOperationClients(14-115)connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
GetOperationClientsRequest(22995-23059)GetOperationClientsResponse(23064-23102)GetOperationDeprecatedFieldsRequest(23164-23228)GetOperationDeprecatedFieldsResponse(23233-23271)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
getOperationDeprecatedFields(19-120)
studio/src/components/operations/operations-list.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (11)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-200)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(43-191)studio/src/components/operations/operations-list.tsx (1)
OperationsList(218-286)studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(33-156)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(450-521)LatencyMetricsCard(523-581)ErrorMetricsCard(583-642)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-80)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
studio/src/components/analytics/field-usage.tsx (1)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)
controlplane/src/core/bufservices/analytics/getOperations.ts (7)
controlplane/src/core/repositories/analytics/UsageRepository.ts (1)
UsageRepository(12-455)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/core/util.ts (1)
validateDateRanges(346-373)composition/src/ast/utils.ts (1)
parse(272-274)composition/src/buildASTSchema/buildASTSchema.ts (1)
buildASTSchema(26-72)controlplane/src/core/services/SchemaGraphPruner.ts (1)
SchemaGraphPruner(21-404)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
GetOperationsResponse_Operation(22785-22867)
studio/src/components/analytics/metrics.tsx (3)
studio/src/lib/utils.ts (1)
cn(6-8)studio/src/components/info-tooltip.tsx (1)
InfoTooltip(16-37)studio/src/lib/insights-helpers.ts (1)
useChartData(66-151)
🪛 GitHub Check: CodeQL
studio/src/components/operations/operations-list.tsx
[failure] 80-80: Regular expression injection
This regular expression is constructed from a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (21)
studio/src/components/analytics/metrics.tsx (6)
46-52: LGTM! Import organization improved.The expanded import statement with explicit hooks improves readability while maintaining all necessary dependencies.
83-87: LGTM! Formatting improved.The reformatting enhances readability without changing the logic.
363-368: LGTM! Proper className propagation.The
classNameprop is correctly destructured and applied using thecn()utility, allowing external styling while preserving default styles.Also applies to: 379-379
454-455: LGTM! Well-designed flexibility enhancement.The addition of
showTopListandchartClassNameprops enables component reuse in different contexts while maintaining backward compatibility through sensible defaults.Also applies to: 458-458, 507-507, 510-518
527-528: LGTM! Consistent pattern applied.The same flexibility enhancement pattern is consistently applied, which improves maintainability across metric card components.
Also applies to: 531-531, 567-567, 570-578
587-591: LGTM! Complete pattern implementation.The
ErrorMetricsCardandErrorPercentChartcomponents now follow the same flexibility pattern. TheInfoTooltipreformatting is a minor cosmetic improvement.Also applies to: 606-608, 624-624, 627-639, 645-645, 653-653
studio/src/components/layout/graph-layout.tsx (2)
60-76: Namespace from workspace context fits existing patternsUsing
useWorkspaceto derivenamespace.nameand passing it both togetFederatedGraphByNameand intobasePathkeeps routing and data-loading in sync with howWorkspaceSelectoralready models namespaces. No issues spotted here.
132-137: “Operations” nav item wiring looks correctThe new “Operations” entry reuses the
basePath, points to/operations, and usesmatchExact: false, which should make nested operation views behave like Analytics/Routers. The icon sizing is consistent with the rest of the menu.studio/src/components/ui/scroll-area.tsx (1)
1-37: ScrollArea/ScrollBar wrapper is well-structuredThe forwardRef wrappers, type inference from the underlying primitives, and class composition all look solid. The component tree (Root → Viewport + ScrollBar + Corner) is coherent, and orientation-specific styling in
ScrollBaris clean.studio/src/components/analytics/field-usage.tsx (1)
71-75: Using workspace namespace in FieldUsage keeps analytics links consistentPulling
namespace.namefromuseWorkspaceand feeding it into the traces link query (namespaceparam) matches how the rest of the Studio UI models namespaces and avoids relying solely onrouter.query. This should make navigation to the traces view more robust as routing evolves.Also applies to: 251-271
controlplane/src/core/bufservices/PlatformService.ts (1)
167-168: New analytics RPCs correctly wired into PlatformService
getOperationClientsandgetOperationDeprecatedFieldsare imported and exposed using the same pattern as existing analytics methods, so routing from the platform service to the new handlers looks correct.Also applies to: 849-855
controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
19-119: End‑to‑end deprecated‑fields flow looks consistent and safeAuth, analytics‑retention/date‑range validation, schema loading/pruning, and ClickHouse usage query are all wired consistently with existing analytics handlers. Returning an OK response with an empty list when there is no valid schema or no usages is a reasonable choice here, given the error logging in the catch block.
studio/src/components/operations/client-usage-table.tsx (1)
41-55: The review comment is incorrect; the guard is unnecessary.Looking at
useAnalyticsQueryState, thedateRangeobject is always initialized with a default value ({ start: subHours(new Date(), parsedRange), end: new Date() }) before any conditionals. The logic ensures that:
- When
query.dateRangeexists:rangebecomesundefined,dateRangeis set to the parsed value- When
query.dateRangedoesn't exist:rangeis set toparsedRange,dateRangeretains its defaultThere is no code path where
rangeis falsy anddateRangeis undefined. The hook guarantees thatdateRangeis always defined.Likely an incorrect or invalid review comment.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
403-427: NewOperationsFetchBasedOnenum wiring looks correctThe enum values and
setEnumTypemetadata are consistent and give a clear, extensible basis for selecting the metric (requests/latency/errors). No issues from a TS/proto wiring perspective.
4928-4931: OptionalnameonGetOperationContentRequestis correctly modeledThe new
name?: stringfield and its metadata entry{ no: 4, name: "name", ..., opt: true }correctly reflect an optional string at field number 4. This keeps the message backward compatible while allowing lookups by name.Also applies to: 4941-4945
22992-23159:GetOperationClients*messages align with Go definitionsThe TS definitions for:
GetOperationClientsRequest(federatedGraphName,namespace,operationHash,operationName, optionalrangeanddateRange), andGetOperationClientsResponseplus nestedClient(name,version, int64requestCount,lastUsedISO8601 string),are consistent with the Go snippets (field names, numbers, and types), including int64 handling via
protoInt64. No structural issues here.
23161-23320:GetOperationDeprecatedFields*message modeling looks correctThe request/response and nested
DeprecatedFieldare wired as expected:
- Request:
federatedGraphName,namespace,operationHash, optionaloperationName,range,dateRange.- Response:
responseplus repeateddeprecatedFieldswithfieldName,typeName,path.Field numbers and optionality match the provided Go definitions, and the TS
fieldsmetadata is coherent.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
638-640: LGTM: Conditional loader prevents layout shift.Good practice to only show the fullscreen loader on initial load (
!operationsData) and not during refetches, which prevents unnecessary layout shifts and improves UX.
67-67: Withdraw concern — graphContext is properly guarded throughout.All accesses to
graphContextin this file use optional chaining (?.), which safely handles the nullableGraphContextProps | undefinedtype. Query invalidation at lines 977–984 and all other usages correctly chain properties with?., preventing undefined errors.Likely an incorrect or invalid review comment.
connect/src/wg/cosmo/platform/v1/platform_connect.ts (1)
8-8: New PlatformService RPCs are wired consistentlyThe additions for
GetOperationClientsandGetOperationDeprecatedFields(imports plus method descriptors) match the generated message types and follow the existing pattern for unary analytics RPCs onPlatformService. No issues from an API‑surface perspective.Also applies to: 1800-1821
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
9-9: Connect-query bindings for new RPCs look correct
getOperationClientsandgetOperationDeprecatedFieldsare exported with the expectedlocalName, RPCname, unarykind, and corresponding request/response types, consistent with the rest of the connect-query surface.Also applies to: 2619-2650
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
studio/src/components/operations/operations-list.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/components/operations/operations-list.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
studio/src/components/operations/operations-list.tsx (5)
96-106: LGTM!The cleanup effect properly clears both timeout references on unmount, preventing memory leaks.
108-133: LGTM!The hash click handler properly manages the copy-to-clipboard flow with appropriate timeout sequencing for the "Copied!" feedback and tooltip state transitions.
135-150: LGTM!The metric selection logic correctly handles different sort fields with appropriate formatting and null checks.
254-287: LGTM!The list rendering and selection logic is well-implemented with:
- Clear loading and empty states
- Thoughtful selection matching that handles both hash-only and hash+name scenarios
- Proper key generation for list items
The detailed inline comments explaining the selection logic are particularly helpful for maintainability.
81-82: Based on my verification of the codebase and security advisories:Lodash version 4.17.21 is already patched and secure.
The
_.escapeRegExp()method properly escapes regex metacharacters (^, $, , ., *, +, ?, (, ), [, ], {, }, |), which prevents regex injection. The known ReDoS vulnerabilities in lodash affect the toNumber, trim, and trimEnd functions, notescapeRegExp. The project's lodash version (^4.17.21) is the patched version that mitigates ReDoS vulnerabilities.The code pattern in question is secure: escaping user input before constructing a regex prevents special characters from being interpreted as regex operators. Wrapping the escaped query in a capturing group
(${safeQuery})maintains this safety because the input has already been neutralized.
…ations components
There was a problem hiding this comment.
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:subtitleprop is not rendered by GraphPageLayout.The
subtitleprop (line 697) is passed toGraphPageLayout, but as confirmed in past review discussions, while it's declared inTitleLayoutProps, the component doesn't destructure or render it.Either:
- Remove the
subtitleprop if it's not needed- Update
GraphPageLayoutinstudio/src/components/layout/graph-layout.tsxto render the subtitleBased on past review comments.
🧹 Nitpick comments (3)
proto/wg/cosmo/platform/v1/platform.proto (1)
2888-2902: Minor: Non-sequential field numbering in GetOperationsRequest.Field numbers jump from
fetchBasedOn = 7tosortDirection = 13, then back toincludeContent = 8. While protobuf allows non-sequential numbering, this pattern suggestssortDirectionmay have been added later or moved. This is not incorrect but can be confusing.Consider documenting the reason for the jump or renumbering fields sequentially if this is still under active development.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (2)
250-252: Minor: Potential invalid syncId during initial load.If
graphContext?.graph?.namespaceorgraphContext?.graph?.nameis undefined during initial render,syncIdwill be"undefined-undefined". While the query is guarded byenabled: !!graphContext?.graph?.name, the syncId is constructed before this check.Consider adding a guard:
- const syncId = `${graphContext?.graph?.namespace}-${graphContext?.graph?.name}`; + const syncId = graphContext?.graph?.namespace && graphContext?.graph?.name + ? `${graphContext.graph.namespace}-${graphContext.graph.name}` + : undefined;Then handle
undefinedgracefully in metric cards or use it as-is if your sync logic already handles this case.Based on past review comments.
492-496: Recommend: Stabilize applySearchQuery to prevent unnecessary effect runs.The effect depends on
applySearchQuery, which indirectly depends on[router]through the dependency chain:
applySearchQuery→useCallback([applyNewParams])(use-operations-filters.tsx:132)applyNewParams→useCallback([router])(use-operations-filters.tsx:28)Since Next.js
useRouter()returns a non-stable object reference, this effect re-runs on every router change (even same-page navigations), not just when search actually changes.Consider using
router.pushdirectly or extracting only thepushmethod withuseRef:const routerPushRef = useRef(router.push); useEffect(() => { routerPushRef.current = router.push; }, [router.push]);Then update
useOperationsFiltersto use the ref instead of[router]dependency. This will stabilize the callback chain.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
proto/wg/cosmo/platform/v1/platform.proto(5 hunks)studio/src/components/analytics/metrics.tsx(17 hunks)studio/src/components/operations/operation-content-modal.tsx(1 hunks)studio/src/components/operations/operations-list.tsx(1 hunks)studio/src/hooks/use-operations-filters.tsx(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- studio/src/components/operations/operation-content-modal.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
studio/src/hooks/use-operations-filters.tsx (3)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
AnalyticsViewResultFilter(6233-6283)AnalyticsFilter(5954-6004)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (14)
AnalyticsViewResultFilter(7536-7545)AnalyticsViewResultFilter(7560-7560)AnalyticsViewResultFilter(7575-7577)AnalyticsViewFilterOperator(287-287)AnalyticsViewFilterOperator(352-354)AnalyticsViewFilterOperator(356-358)AnalyticsViewFilterOperator(365-367)AnalyticsFilter(7184-7195)AnalyticsFilter(7210-7210)AnalyticsFilter(7225-7227)OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)
studio/src/components/analytics/metrics.tsx (4)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/lib/utils.ts (1)
cn(6-8)studio/src/components/info-tooltip.tsx (1)
InfoTooltip(16-37)studio/src/lib/insights-helpers.ts (1)
useChartData(66-151)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx (13)
studio/src/components/layout/graph-layout.tsx (3)
GraphContext(56-58)getGraphLayout(227-233)GraphPageLayout(247-297)studio/src/components/analytics/useAnalyticsQueryState.ts (1)
useAnalyticsQueryState(50-125)studio/src/hooks/use-feature-limit.ts (1)
useFeatureLimit(10-17)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
getGraphMetrics(1923-1933)getOperations(2592-2601)studio/src/hooks/use-operations-filters.tsx (1)
useOperationsFilters(25-202)studio/src/components/operations/operations-search.tsx (1)
OperationsSearch(43-191)studio/src/components/operations/operations-list.tsx (1)
OperationsList(223-291)studio/src/components/operations/client-usage-table.tsx (1)
ClientUsageTable(33-156)studio/src/components/operations/deprecated-fields-table.tsx (1)
DeprecatedFieldsTable(33-144)studio/src/components/analytics/metrics.tsx (3)
RequestMetricsCard(454-525)LatencyMetricsCard(527-585)ErrorMetricsCard(587-646)studio/src/components/operations/operation-content-modal.tsx (1)
OperationContentModal(25-87)controlplane/src/core/bufservices/analytics/getOperations.ts (1)
getOperations(27-280)studio/src/components/analytics/field-usage.tsx (1)
FieldUsageSheet(345-430)
studio/src/components/operations/operations-list.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-router
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
🔇 Additional comments (4)
studio/src/components/analytics/metrics.tsx (2)
70-81: LGTM: Filter removal logic is now safe.The guard
if (index !== -1)at line 71 correctly prevents the splice bug identified in previous reviews wheresplice(-1, 1)would remove the last element.
454-525: LGTM: Metric card enhancements are well-designed.The addition of
showTopListandchartClassNameprops provides good flexibility for customizing metric card display. Defaults are sensible (showTopList = true), and the pattern is consistent across all metric cards.studio/src/components/operations/operations-list.tsx (1)
80-97: LGTM: Regex highlighting bug has been fixed.The previous issue with
regex.test()and the global flag has been correctly addressed. The code now uses a simple string comparison (part.toLowerCase() === queryLower) instead of the stateful regex test, preventing intermittent highlighting failures.proto/wg/cosmo/platform/v1/platform.proto (1)
2911-2922: Verify compatibility: Field number changed fortype.The
typefield moved from position 5 to position 4 inGetOperationsResponse.Operation. If this is a breaking change from a previous proto version, ensure all clients are updated or use field number preservation for backward compatibility.The new
oneof metricdesign is clean and appropriate for the use case where exactly one metric is returned based onfetchBasedOn.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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
sortDirectionas"desc" | "asc"instead ofstring.Apply this diff for stricter typing:
sortDirection: string; - onSortDirectionChange: (direction: string) => void; + sortDirection: "desc" | "asc"; + onSortDirectionChange: (direction: "desc" | "asc") => void;
150-166: Consider simplifying enum conversion pattern.The current pattern converts the enum to a string and back to a number. While functionally correct, this could be streamlined.
Consider using enum values directly if the Select component supports non-string values, or create a helper function to reduce repetition:
// Helper function approach const enumToSelectValue = (value: OperationsFetchBasedOn) => value.toString(); const selectValueToEnum = (value: string) => Number(value) as OperationsFetchBasedOn; // Usage <Select value={enumToSelectValue(fetchBasedOn)} onValueChange={(value) => onFetchBasedOnChange(selectValueToEnum(value))} >Alternatively, if this pattern is used elsewhere, consider creating shared utility functions in the analytics utilities file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/components/operations/operations-search.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/components/operations/operations-search.tsx (3)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (11)
OperationsFetchBasedOn(654-654)OperationsFetchBasedOn(686-688)OperationsFetchBasedOn(690-692)OperationsFetchBasedOn(699-701)AnalyticsViewFilterOperator(287-287)AnalyticsViewFilterOperator(352-354)AnalyticsViewFilterOperator(356-358)AnalyticsViewFilterOperator(365-367)AnalyticsFilter(7184-7195)AnalyticsFilter(7210-7210)AnalyticsFilter(7225-7227)studio/src/components/analytics/getDataTableFilters.ts (1)
optionConstructor(5-23)studio/src/components/analytics/filters.tsx (1)
AnalyticsFilters(25-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (3)
studio/src/components/operations/operations-search.tsx (3)
1-23: LGTM!Imports are well-organized and appropriate for the component's functionality.
54-68: LGTM!All callback handlers are correctly implemented with proper memoization and dependency tracking.
81-104: LGTM!The selected options derivation and filter list construction are correctly implemented with proper memoization.
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/UsageRepository.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:44:31.726Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.726Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
controlplane/src/core/repositories/analytics/UsageRepository.ts
📚 Learning: 2025-11-19T11:26:26.657Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.657Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
Applied to files:
controlplane/src/core/repositories/analytics/UsageRepository.ts
🧬 Code graph analysis (1)
controlplane/src/core/repositories/analytics/UsageRepository.ts (2)
controlplane/src/types/index.ts (1)
DateRange(630-633)controlplane/src/core/repositories/analytics/util.ts (1)
parseTimeFilters(417-429)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
controlplane/src/core/repositories/analytics/UsageRepository.ts (5)
306-330: LGTM!The method signature is well-typed, and the early return for empty deprecated fields is a good optimization.
332-334: LGTM!Time filter parsing correctly uses the
parseTimeFiltersutility.
347-374: LGTM!Excellent use of parameterized queries for all filterable parameters. The CTE structure is clean and the conditional handling of optional
operationNameis well-implemented.
376-388: LGTM!Parameter object construction and conditional
operationNamehandling are implemented correctly.
390-397: LGTM!Result processing includes appropriate defensive checks and handles edge cases gracefully.
…rgraph/cosmo into suvij/eng-5256-operations-overview
There was a problem hiding this comment.
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)whenincludeContentis false, which verifies the implementation optimization of skipping the content query. For symmetry and better documentation of the expected behavior, this test could similarly assert thatqueryPromisewas called exactly twice (once for operations, once for content).Add this assertion after line 525:
expect(response.response?.code).toBe(EnumStatusCode.OK); expect(response.operations).toHaveLength(1); expect(response.operations[0]?.content).toBe('query { hello }'); + +// Should call ClickHouse twice: once for operations, once for content +expect(chClient.queryPromise).toHaveBeenCalledTimes(2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/analytics/get-operations.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
controlplane/test/analytics/get-operations.test.ts
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
controlplane/test/analytics/get-operations.test.ts
🧬 Code graph analysis (1)
controlplane/test/analytics/get-operations.test.ts (4)
controlplane/src/core/test-util.ts (4)
beforeAllSetup(38-45)afterAllSetup(47-51)genID(53-55)genUniqueLabel(57-59)controlplane/test/test-util.ts (3)
SetupTest(66-417)createThenPublishSubgraph(550-573)DEFAULT_NAMESPACE(52-52)shared/src/utils/util.ts (1)
joinLabel(17-19)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (8)
OperationsFetchBasedOn(657-657)OperationsFetchBasedOn(689-691)OperationsFetchBasedOn(693-695)OperationsFetchBasedOn(702-704)GetOperationsResponse_OperationType(752-752)GetOperationsResponse_OperationType(784-786)GetOperationsResponse_OperationType(788-790)GetOperationsResponse_OperationType(797-799)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (1)
controlplane/test/analytics/get-operations.test.ts (1)
1-906: Excellent test coverage and structure!This test suite is comprehensive and well-organized, covering:
- Error conditions (analytics disabled, not found, invalid parameters, invalid date ranges)
- Core functionality (sorting by latency/requests/errors, filtering, search, pagination)
- Optional features (content inclusion, deprecated fields handling)
- Edge cases (empty results, different operation types, default parameters)
The tests demonstrate good practices:
- Proper test isolation using unique IDs and mocking
- Clear, descriptive test names that document expected behavior
- Consistent patterns across related tests
- Appropriate cleanup with
server.close()- Strong assertions that verify specific values rather than loose checks
The resolution of past review comments (stronger assertions for default pagination, accurate "(default)" labeling) shows attention to test quality.
…arameterized values for organizationId and federatedGraphId, and improve search query handling
There was a problem hiding this comment.
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: Clarifydeprecated_fieldsCTE construction to reduce string‑templating complexityThe
deprecatedFieldsCte/deprecatedFieldsJoinconstruction is functionally sound (names and type names are escaped, and joins are conditionally applied), but the combination of:
- building a multi‑line CTE string with a trailing comma, and
- weaving it into the
WITHclause via overlapping ternariesmakes the final SQL a bit hard to reason about and fragile to future edits.
Consider refactoring along these lines:
- Build an array of CTE fragments (
['deprecated_fields AS (...)', 'operations_with_deprecated_fields AS (...)', 'ops AS (...)']) andjoin(',\n')them, instead of manually juggling commas.- Keep the deprecated‑fields CTE text separate from the core
opsCTE, so you don’t need${deprecatedFieldsCte ? ',' : ''}/${deprecatedFieldsCte ? '' : ','}tricks.That would preserve behavior but make it much easier to verify (and adjust) the CTE chain when you touch this query again.
Also applies to: 829-835
999-1065:getClientsOfOperationquery is solid; consider minor robustness tweaksThe new
getClientsOfOperationimplementation is generally in good shape:
- Uses parameter binding for all untrusted inputs (org/graph IDs, hash, optional name, timestamps).
- Aggregates by
ClientName/ClientVersion, withsum(TotalRequests)andmax(Timestamp)and orders bylastUsed DESC, which matches the intended “most recently active first” semantics.- Maps the response to a clean DTO and normalizes
lastUsedto ISO vianew Date(client.lastUsed + 'Z').toISOString(), which aligns with the expectation that the DB returns aYYYY-MM-DD HH:MM:SSstyle timestamp. Based on learnings, treatinglastUsedas always present without a fallback is acceptable.Two small follow‑ups you might consider:
Date‑range normalization parity
Other methods callflipDateRangeValuesIfNeeded(or go throughgetMetricsProps) to ensurestart <= end. If the call‑sites ofgetClientsOfOperationdon’t already enforce this, you may want to guard here too for symmetry and resilience.Optional server‑side limit
Depending on expected cardinality, you may want to apply aLIMIT(or pagination) to avoid accidentally returning an unbounded number of client rows for very hot operations on long ranges.Neither is blocking, but worth considering to keep this in line with the rest of the repository’s patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.657Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.726Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
808-818: Result shape andhasDeprecatedFieldsmapping look consistentThe result typing and mapping in
getOperationslooks coherent:
limitOffsetClauseandqueryParamsWithPaginationpush pagination and filtering fully into ClickHouse.- Each branch (latency / requests / errors) returns a consistent row shape, with unused numeric fields set to
0, which aligns with the|| 0fallbacks in the mapper.- Computing
hasDeprecatedFieldsasIF(dep.operationHash IS NOT NULL AND dep.operationHash != '', 1, 0)and then mapping viaop.hasDeprecatedFields === 1is robust even when the deprecated‑fields CTE is not present (constant0path).No changes needed here from my side.
Also applies to: 820-835, 874-875, 909-910, 947-948, 957-965
…ationsResponse, and implement total count retrieval in getOperations function
…onsistent casing and improve SQL query ordering
There was a problem hiding this comment.
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 clientNametorepeated string clientNames, which is a breaking protobuf change. This issue was already identified in previous review comments.The new optional fields (offset, includeHasDeprecatedFields, fetchBasedOn, includeContent, range, dateRange, searchQuery, includeOperationsWithDeprecatedFieldsOnly, sortDirection, includeTotalCount) are backwards compatible additions.
2918-2932: Breaking changes already flagged in previous review.The restructuring of
GetOperationsResponse.Operationincludes breaking changes that were previously identified:
- The
typefield moved from position 5 to 4- Field number 5 is now reused for
hasDeprecatedFieldsAdditionally, field 3 (
content) has been made optional, and a newoneof metricstructure has been added. These changes break wire compatibility with existing clients.connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
22844-22892: Duplicate: Field number change fortypefield breaks protobuf compatibility.This issue was thoroughly investigated in a previous review comment with extensive verification. The
typefield assignment to field number 4 represents a breaking protobuf change (it was previously at field number 5 per the earlier analysis).As noted in the previous review, the fix must be applied to the source
.protofile (not this generated TypeScript file) by restoringtypeto field number 5 and reassigning the new fields (hasDeprecatedFieldsand the metric oneof members) to previously unused field numbers.controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
722-737: SanitizefetchBasedOn/sortDirectionbefore injecting into SQL ORDER BY.This issue was previously flagged:
sortDirectionis interpolated directly intoORDER BYclauses without validation, creating a SQL injection vector.Per the previous review, constrain both parameters to known unions:
fetchBasedOn:'latency' | 'requests' | 'errors'(default'latency')sortDirection:'ASC' | 'DESC'(default'DESC')Map and validate them before building SQL to prevent injection and ensure type safety at call sites.
Also applies to: 877-880, 912-915, 950-953
🧹 Nitpick comments (2)
controlplane/src/core/bufservices/analytics/getOperations.ts (1)
124-148: Acceptable implementation with graceful error handling.The deprecated fields detection logic is only executed when explicitly requested and includes error handling. If schema parsing fails, the operation continues with an empty deprecated fields list, which is an acceptable graceful degradation.
Note: For federated graphs with very large schemas, the schema parsing and field extraction (lines 133-138) could be computationally expensive. Consider caching the deprecated fields list if this endpoint experiences performance issues.
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
1165-1166: Consider usingUInt32type hint for timestamp parameters.The query uses
{startTimestamp:String}and{endTimestamp:String}but passes numeric timestamp values (lines 1182-1183). While ClickHouse will coerce these correctly, using{startTimestamp:UInt32}and{endTimestamp:UInt32}would be more direct and consistent with the other queries in this file (e.g., lines 842, 886, 922, 1034).WITH - toDateTime({startTimestamp:String}) AS startDate, - toDateTime({endTimestamp:String}) AS endDate + toDateTime({startTimestamp:UInt32}) AS startDate, + toDateTime({endTimestamp:UInt32}) AS endDate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (5)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(11 hunks)controlplane/src/core/bufservices/analytics/getOperations.ts(5 hunks)controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(6 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
connect/src/wg/cosmo/platform/v1/platform_pb.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/analytics/getOperations.ts
📚 Learning: 2025-11-19T11:26:26.657Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.657Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.726Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
🧬 Code graph analysis (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (7)
OperationsFetchBasedOn(657-657)OperationsFetchBasedOn(689-691)OperationsFetchBasedOn(693-695)OperationsFetchBasedOn(702-704)DateRange(7299-7308)DateRange(7323-7323)DateRange(7338-7340)controlplane/src/types/index.ts (1)
DateRange(630-633)
controlplane/src/core/bufservices/analytics/getOperations.ts (5)
controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1661)controlplane/src/core/constants.ts (1)
deafultRangeInHoursForGetOperations(11-11)controlplane/src/core/util.ts (1)
validateDateRanges(346-380)composition/src/ast/utils.ts (1)
parse(272-274)composition/src/buildASTSchema/buildASTSchema.ts (1)
buildASTSchema(26-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (23)
controlplane/src/core/bufservices/analytics/getOperations.ts (11)
12-26: LGTM: Imports are appropriate for the new features.The new imports support the extended functionality for operations retrieval: enum types for sorting/fetching, schema parsing for deprecated fields detection, additional repositories for analytics retention and usage data, and the validation utility for date ranges.
50-52: LGTM: Repository initialization follows established patterns.The new repository instances are initialized consistently with the existing code and have access to the necessary dependencies from
opts.
66-87: LGTM: Offset validation is correct.The offset is properly defaulted to 0 and validated to ensure it's non-negative, with appropriate error handling for invalid values.
89-96: LGTM: Backwards-compatible enum mapping.The
fetchBasedOnparameter correctly defaults toLATENCYfor backwards compatibility, and the enum-to-string mapping is clear and correct.
98-119: LGTM: Date range validation with organization-specific retention.The code correctly retrieves the organization's analytics retention limit and validates the requested date range against it, with appropriate fallback to defaults and error handling for invalid ranges.
150-163: LGTM: Client name filtering with OR semantics.The code correctly creates multiple
EQUALSfilters for each client name, which will be combined with OR logic. The comment clearly documents this behavior.
165-183: LGTM: Sort direction and repository call are correct.The sort direction is properly mapped with a sensible default (descending), and the
getOperationscall includes all the new parameters for the extended functionality.
194-205: LGTM: Efficient conditional content fetching.Operation content is only fetched when explicitly requested, avoiding unnecessary database queries. The operation hashes are extracted from the already-retrieved operations for efficient lookup.
208-253: LGTM: Operation objects correctly structured with oneof metric.The operation objects are built with conditional fields based on request flags, and the
metriconeof is correctly structured according to thefetchBasedOnparameter. This follows the protobuf pattern for oneof fields.
255-267: LGTM: Conditional total count with consistent filters.The total count is only computed when explicitly requested, using the same filter criteria as the main operations query. This separate count query is the expected pattern for pagination support.
269-276: LGTM: Response correctly structured.The response correctly includes the operations array and conditionally includes the total count when requested, matching the protobuf schema definition.
proto/wg/cosmo/platform/v1/platform.proto (5)
640-640: LGTM: Backwards-compatible field addition.Adding the optional
namefield toGetOperationContentRequestis backwards compatible in protobuf, as it uses a new field number (4) and is optional.
822-822: LGTM: Backwards-compatible enum value addition.Adding the
Booleanvalue to theCustomOptionsenum is backwards compatible, as it uses the next sequential value (2).
2883-2892: LGTM: Well-defined new enums.Both
OperationsFetchBasedOnandSortDirectionenums are properly structured with clear, descriptive values starting at 0, which is correct for proto3.
2948-2985: LGTM: Well-structured new message definitions.The new
GetOperationClientsandGetOperationDeprecatedFieldsmessage pairs are well-designed with proper field numbering, consistent structure, and appropriate use of optional fields. The nested message definitions follow protobuf best practices.
3396-3399: LGTM: Backwards-compatible RPC additions.The new
GetOperationClientsandGetOperationDeprecatedFieldsRPCs are added at the end of thePlatformService, which is good practice for backwards compatibility. They properly reference the new request/response message types.connect/src/wg/cosmo/platform/v1/platform_pb.ts (6)
101-115: LGTM: Clean enum extension.The addition of
Boolean = 2to theCustomOptionsenum is correctly structured and backward compatible.
409-453: LGTM: Well-defined enums for operations filtering and sorting.Both
OperationsFetchBasedOnandSortDirectionenums are correctly structured with clear semantic values.
4954-4970: LGTM: Backward-compatible optional field addition.Adding the optional
namefield toGetOperationContentRequestis correctly implemented.
22628-22730: LGTM: Comprehensive backward-compatible request extension.The extensive additions to
GetOperationsRequestprovide rich filtering, pagination, and sorting capabilities. All new fields are properly marked optional or are repeated arrays, maintaining backward compatibility.Based on learnings, the
clientNamesrepeated field is correctly initialized with an empty array per protobuf conventions.
22760-22779: LGTM: Clean addition of pagination metadata.Adding optional
totalCountfield toGetOperationsResponseis backward compatible and useful for pagination.
23034-23362: LGTM: Well-structured new RPC message definitions.The new message definitions for
GetOperationClientsandGetOperationDeprecatedFieldsRPCs are properly structured with:
- Consistent request/response patterns
- Appropriate field types and numbering
- Clear field documentation (e.g., ISO 8601 timestamp for
lastUsed)- Correct protobuf boilerplate generation
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
1140-1206: Well-structured method with proper parameterization.The
getClientsOfOperationmethod correctly uses parameterized queries throughout, handles the optionaloperationNameparameter safely, and formats the ISO timestamp properly.
There was a problem hiding this comment.
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
deprecatedFieldsis empty. WhendeprecatedFields.length === 0,deprecatedFieldsCteis an empty string, which causes:
- Line 843:
endDate(no trailing comma, since empty string is falsy)- Line 844:
,(standalone comma, since empty string is falsy)This generates:
WITH toDateTime(...) AS startDate, toDateTime(...) AS endDate , ops AS (The standalone comma creates a syntax error.
Apply this fix to all three query branches (lines 843-844, 887-888, 923-924):
- toDateTime({endDate:UInt32}) AS endDate${deprecatedFieldsCte ? ',' : ''} - ${deprecatedFieldsCte}${deprecatedFieldsCte ? '' : ','} + toDateTime({endDate:UInt32}) AS endDate, + ${deprecatedFieldsCte}Since
deprecatedFieldsCtealready includes a trailing comma when set (line 792), you only need the comma afterendDate.Also applies to: 887-888, 923-924
1035-1036: Critical CTE syntax error (same as in getOperations).The same conditional comma logic error exists here. When
deprecatedFieldsCteis a non-empty string, this works, but the pattern is fragile and produces the same invalid SQL structure as ingetOperations.Apply the same fix as in
getOperations:- toDateTime({endDate:UInt32}) AS endDate${deprecatedFieldsCte ? ',' : ''} - ${deprecatedFieldsCte}${deprecatedFieldsCte ? '' : ','} + toDateTime({endDate:UInt32}) AS endDate, + ${deprecatedFieldsCte}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.657Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.726Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
controlplane/src/core/repositories/analytics/MetricsRepository.ts
Outdated
Show resolved
Hide resolved
…mproved readability and consistency
…rgraph/cosmo into suvij/eng-5256-operations-overview
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.657Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
📚 Learning: 2025-11-19T11:44:31.726Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.726Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
controlplane/src/core/repositories/analytics/MetricsRepository.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (2)
722-971: Well-structured query with proper parameterization.The
getOperationsmethod correctly:
- Sanitizes
sortDirectionto only'ASC' | 'DESC'(line 735)- Uses parameterized queries throughout to prevent SQL injection
- Properly handles the deprecated fields CTE with correct comma placement
- Escapes single quotes in deprecated field names (line 763-764)
The three-branch structure (latency/requests/errors) has some duplication but maintains clarity.
1140-1206: Clean implementation with proper parameterization.The
getClientsOfOperationmethod correctly:
- Uses
:UInt32type hints for timestamp parameters (lines 1165-1166)- Parameterizes all query inputs to prevent SQL injection
- Handles optional
operationNameparameter conditionally (line 1177, 1189-1191)- Converts ClickHouse timestamps to ISO format with UTC timezone (line 1204)
…oper comma placement for improved clarity
There was a problem hiding this comment.
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
📒 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
StarpTech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ns total count message
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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?.namespaceorgraphContext?.graph?.nameis undefined,syncIdwill 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
undefinedgracefully, 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
📒 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.tsxstudio/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.tsxstudio/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
enabledcondition (line 62)- Handles both
rangeanddateRangeparameters correctly- Extracts
refetchfor the retry functionality- Uses appropriate data transformations with safe fallbacks
Based on learnings, the
lastUsedfield is always present from the API, andBigInttoNumberconversion 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(). ThecolSpan={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
rangeand customdateRangefor date picker changes- Includes matching
dateRangein 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). TherequestCountconversion at line 211 correctly uses a ternary operator to avoid theNaNissue 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?.operationschanges (line 736)- Reads
selectedOperationand usesapplyParamswithout 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
getOperationsquery properly:
- Uses
keepPreviousDatato prevent UI flicker during refetches- Sets
staleTime: 0andgcTime: 0to ensure fresh data on filter changes- Handles both
rangeand customdateRangeparameters- Includes all necessary filter parameters (clientNames, deprecated fields, search, sort)
- Uses
includeTotalCountfor pagination
758-792: Clean two-panel layout composition.The layout properly:
- Uses flex with responsive breakpoints for mobile/desktop
- Handles overflow correctly with
overflow-hiddenand scrollable containers- Includes
FieldUsageSheetat the page level for field usage overlay
611-615: I'll help you verify this review comment about theapplyParamsdependency stability. Let me investigate theuseApplyParamshook and the actual implementation to determine if this is a valid concern.
<function_calls>
#!/bin/bashFind the useApplyParams hook file
fd -t f "use-apply-params" --extension tsx --extension ts
</function_calls>
#!/bin/bashCheck the useApplyParams implementation
rg -nP -A20 'export const useApplyParams|function useApplyParams' --type=ts --type=tsx
</function_calls>
#!/bin/bashGet context around the useEffect in question
sed -n '600,620p' studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/operations.tsx
</function_calls>
Summary by CodeRabbit
New Features
UI
Backend
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist