-
Notifications
You must be signed in to change notification settings - Fork 67
[BI]Handle special character fields in names #972
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds optional annotationAttachments to the Member interface and introduces an AnnotationAttachment type; updates the AttributeCard UI to extract and use custom display names from member annotations with error handling and a fallback to the original name. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
1461-1479: AlignAnnotationAttachment.propertiestyping with actual runtime shapesThe new
annotationAttachments?: AnnotationAttachment[]onMemberis fine and backward compatible, butAnnotationAttachment.propertiesis strictly typed asProperty[]while the UI helper (getCustomNameFromAnnotationinAttributeCard.tsx) explicitly handles both array and non-array/object-like shapes viaanycasting.If the language server can send non-
Property[]structures here (e.g., nested{ value: { value: string } }objects), consider widening the type to something likeProperty[] | Record<string, unknown>(or evenunknown) and tightening the helper around that shape. Otherwise, if it’s guaranteed to always beProperty[], the extra object-path handling in the helper should be removed to keep types and usage consistent.If you’re unsure about the LS payload shape, please verify against the actual JSON schema or sample responses before deciding which side (type vs. helper) to adjust.
workspaces/ballerina/type-diagram/src/components/entity-relationship/EntityNode/Attribute/AttributeCard.tsx (2)
36-83: Tighten typing and remove noisy logging ingetCustomNameFromAnnotationThe extraction logic for
jsondata:Namelooks robust, but a couple of points:
annotation.propertiesis typed asProperty[], while here you treat it asProperty[] | objectand fall back to(properties as any).value.*. Once the LS payload shape is confirmed, it would be better either to:
- keep only the
Property[]path and delete the object-path fallbacks, or- widen the type on
AnnotationAttachment.propertiesand model the alternative shapes explicitly instead of relying onany.The
console.log(and to a lesser extent the broadconsole.errorincluding the fullmember) will spam the extension’s console in normal use. These should either be removed before merge or guarded behind a debug flag / environment check.Functionally this is OK, but cleaning up the types and logs will make it safer and less noisy.
94-101: Type guard anddisplayNamelogic are fine; consider small simplificationUsing a type guard to gate access to
annotationAttachmentsand preferring the custom name viadisplayNamewhile keeping all IDs/ports based onattribute.nameis a good approach and fits the requirement (“show the jsondata.name if available”).Given that
getCustomNameFromAnnotationalready handles missingannotationAttachments, you could simplify this to always call it withattribute as Memberand avoid the customisMemberguard, but that’s purely a readability preference and not required for correctness.Also applies to: 129-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts(1 hunks)workspaces/ballerina/type-diagram/src/components/entity-relationship/EntityNode/Attribute/AttributeCard.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
workspaces/ballerina/ballerina-core/src/interfaces/bi.ts (1)
Property(124-144)
workspaces/ballerina/type-diagram/src/components/entity-relationship/EntityNode/Attribute/AttributeCard.tsx (1)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (2)
Member(1461-1473)TypeFunctionModel(1424-1437)
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)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
1497-1501: Add JSDoc documentation to the AnnotationAttachment interface.The
AnnotationAttachmentinterface represents a new public type and would benefit from documentation that clarifies:
- The purpose and expected usage pattern
- Examples like how
@json:Namemaps tomodulePrefix: "json"andname: "Name"- Whether
modulePrefixcan be absent (some built-in annotations like@deprecateddon't have a module prefix)- The expected structure of the
propertiesarrayThe reuse of the
Propertytype is reasonable for consistency, though consider documenting which Property fields are semantically relevant for annotation metadata (as some likeeditable,hidden,placeholderare UI-specific).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
workspaces/ballerina/ballerina-core/src/interfaces/bi.ts (1)
Property(176-194)
🔇 Additional comments (1)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
1494-1495: LGTM! Already integrated for handling JSON field names with special characters.The optional
annotationAttachmentsfield addition to theMemberinterface is backward compatible and properly integrated. TheAnnotationAttachmentinterface (with requiredmodulePrefix,name, andpropertiesfields) is already in use inAttributeCard.tsxto extract custom field names from the@jsondata:Nameannotation pattern, enabling mapping of JSON keys with special characters to valid Ballerina identifiers.
Purpose
Fixes: wso2/product-ballerina-integrator#979
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.