feat: add deprecation reason to deprecated fields table in op view#2386
feat: add deprecation reason to deprecated fields table in op view#2386JivusAyrus merged 4 commits intomainfrom
Conversation
JivusAyrus
requested review from
Aenimus,
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
WalkthroughThis pull request extends the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2386 +/- ##
=======================================
Coverage ? 34.66%
=======================================
Files ? 340
Lines ? 33837
Branches ? 251
=======================================
Hits ? 11728
Misses ? 21081
Partials ? 1028 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
controlplane/src/core/services/SchemaGraphPruner.ts (1)
67-67: Consider consistency with response mapping.The use of
|| undefinedhere contrasts with|| ''used in the API response mapping (line 122 of getOperationDeprecatedFields.ts). While both approaches work, using|| undefinedis cleaner for optional fields and aligns better with the TypeScript type definition.Consider applying the same pattern in the response mapping for consistency:
- deprecationReason: - deprecatedFields.find( - (f) => f.name === field.deprecatedFieldName && f.typeName === field.deprecatedFieldTypeNames[0], - )?.deprecationReason || '', + deprecationReason: + deprecatedFields.find( + (f) => f.name === field.deprecatedFieldName && f.typeName === field.deprecatedFieldTypeNames[0], + )?.deprecationReason || undefined,studio/src/components/operations/deprecated-fields-table.tsx (1)
124-128: Minor redundancy in fallback logic.The fallback
|| "-"on line 126 is technically redundant since the data mapping at line 68 already ensuresdeprecationReasonis never empty. However, this defensive coding approach is acceptable and doesn't cause any issues.Optionally simplify to rely on the mapping:
- <span className="text-sm text-muted-foreground"> - {field.deprecationReason || "-"} - </span> + <span className="text-sm text-muted-foreground"> + {field.deprecationReason} + </span>controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts (1)
112-123: Consider consolidating duplicate field lookups.The response mapping performs two separate
.find()operations with identical predicates (lines 116-118 and 120-122). While functionally correct, this could be optimized for better performance, especially if thedeprecatedFieldsarray is large.Apply this diff to consolidate the lookups:
deprecatedFields: deprecatedFieldsUsedInOperation.map((field) => { + const matchingField = deprecatedFields.find( + (f) => f.name === field.deprecatedFieldName && f.typeName === field.deprecatedFieldTypeNames[0], + ); + return { fieldName: field.deprecatedFieldName, typeName: field.deprecatedFieldTypeNames[0] || '', - path: - deprecatedFields.find( - (f) => f.name === field.deprecatedFieldName && f.typeName === field.deprecatedFieldTypeNames[0], - )?.path || '', - deprecationReason: - deprecatedFields.find( - (f) => f.name === field.deprecatedFieldName && f.typeName === field.deprecatedFieldTypeNames[0], - )?.deprecationReason || '', + path: matchingField?.path || '', + deprecationReason: matchingField?.deprecationReason || '', + }; }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (7)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(2 hunks)controlplane/src/core/bufservices/analytics/getOperationDeprecatedFields.ts(1 hunks)controlplane/src/core/services/SchemaGraphPruner.ts(1 hunks)controlplane/src/types/index.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(1 hunks)studio/src/components/operations/deprecated-fields-table.tsx(6 hunks)studio/src/components/operations/operation-content-modal.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T11:44:31.726Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:66-72
Timestamp: 2025-11-19T11:44:31.726Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), the `lastUsed` field from the `getOperationClients` API response is always present, so no fallback value is needed when mapping client data.
Applied to files:
studio/src/components/operations/deprecated-fields-table.tsx
🧬 Code graph analysis (1)
studio/src/components/operations/operation-content-modal.tsx (1)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)
⏰ 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
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
proto/wg/cosmo/platform/v1/platform.proto (1)
3020-3020: LGTM! Clean proto field addition.The
deprecationReasonfield is correctly added as field number 4 with an appropriate string type, following the existing field numbering convention.controlplane/src/types/index.ts (1)
749-749: LGTM! Type definition aligns with the feature.The optional
deprecationReasonfield is correctly added to the Field interface, complementing the existingisDeprecatedboolean flag.studio/src/components/operations/deprecated-fields-table.tsx (4)
21-26: LGTM! Interface correctly defines the new field.The
deprecationReasonfield is added as a required string, which is appropriate since the mapping at line 68 ensures a fallback value is always provided.
63-69: LGTM! Clean data mapping with user-friendly fallback.The mapping correctly extracts
deprecationReasonfrom the API response with a sensible fallback to "-" for missing or empty values.
101-103: LGTM! Well-proportioned table layout.The column width distribution (30% / 50% / 20%) appropriately allocates more space to the deprecation reason, which is likely to contain longer descriptive text.
112-112: LGTM! ColSpan values correctly updated.All
colSpanattributes are properly updated from 2 to 3 to accommodate the new Deprecation Reason column.Also applies to: 143-143, 157-157
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
23614-23617: LGTM!The new
deprecationReasonfield is correctly added to the generated protobuf TypeScript bindings. The field number (4) is unique, the type (STRING/T: 9) is appropriate, and the implementation follows the established pattern for existing fields (fieldName,typeName,path).Also applies to: 23630-23630
JivusAyrus
deleted the
suvij/add-reason-to-deprecated-fields-in-op-view
branch
2 participants
@coderabbitai summary
Checklist
COMPLETES ENG-8616