fix(MembershipsSdkAdapter): added name property to member mapping in getmembers function #344
Conversation
…etmembers function
Parimala032
changed the title
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (3)
src/MembershipsSDKAdapter.test.js (1)
Line range hint
42-82: Consider adding test cases for edge cases in name values.While the current test coverage is good, consider adding test cases for:
- Empty string names
- Null names
- Very long names
- Names with special characters
This would ensure robust handling of the name property across all scenarios.
src/MembershipsSDKAdapter.js (2)
118-118: Fix formatting: Add space after colon in property declaration.The property declaration is missing a space after the colon.
- name:member.name, + name: member.name,🧰 Tools
🪛 eslint
[error] 118-118: Missing space before value for key 'name'.
(key-spacing)
Property inconsistency confirmed between room and meeting members
The verification reveals that:
- Meeting members use
nameproperty frommember.name- Room members don't include any name property in
membershipToMembermapping- Both member types are consumed through
getMembersFromDestination()interface- Test expectations don't verify name-related properties
This inconsistency should be addressed as it could cause issues for consumers expecting consistent member objects.
- In
src/MembershipsSDKAdapter.js:
- Add
personDisplayNameto meeting members (keepingnamefor backward compatibility if needed)- Add
personDisplayNameto room members usingmembership.personDisplayName- Document the naming properties in JSDoc if both are intentionally maintained
🔗 Analysis chain
Line range hint
91-119: Consider standardizing member properties between room and meeting members.The
getMembers()function now includes anameproperty for meeting members, whilegetRoomMembers()usespersonDisplayName. This inconsistency might cause issues for consumers of the adapter. Consider:
- Using the same property name across both member types
- Documenting the difference if it's intentional
- Adding the name property to room members if they should be consistent
Let's verify the usage of these properties in the codebase:
Let me gather more context about the
getRoomMembersfunction to fully understand the inconsistency:
Let me check one more thing to see how these member objects are being consumed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of personDisplayName and name properties echo "Searching for personDisplayName usage:" rg "personDisplayName" --type js echo -e "\nSearching for member.name usage:" rg "member\.name" --type jsLength of output: 948
Script:
#!/bin/bash # Search for getRoomMembers implementation ast-grep --pattern 'function getRoomMembers($_) { $$$ }' # Also search for any consumer code that might be using these member objects rg -A 3 "getRoomMembers|getMembers" --type jsLength of output: 5252
Script:
#!/bin/bash # Search for the membershipToMember implementation in getRoomMembers ast-grep --pattern 'const membershipToMember = $_' # Look for any test expectations that might show the expected shape rg -B 2 -A 5 "expect\(members\)" --type jsLength of output: 1847
🧰 Tools
🪛 eslint
[error] 118-118: Missing space before value for key 'name'.
(key-spacing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/MembershipsSDKAdapter.js(1 hunks)src/MembershipsSDKAdapter.test.js(4 hunks)
🧰 Additional context used
🪛 eslint
src/MembershipsSDKAdapter.js
[error] 118-118: Missing space before value for key 'name'.
(key-spacing)
🔇 Additional comments (4)
src/MembershipsSDKAdapter.test.js (4)
47-47: LGTM! Name property correctly added to basic member object.
The test case properly validates the name property for a standard member with basic privileges.
57-57: LGTM! Name property correctly added to privileged member object.
The test case appropriately validates the name property for a member with full privileges (muted, sharing, host, guest).
67-67: LGTM! Name property correctly added to non-joined member object.
The test case properly validates that the name property is included even for members who haven't joined the meeting.
77-77: Verify if undefined name is the expected behavior for device members.
While the test case properly includes the name property for device members, please confirm if undefined is the intended value. Consider:
- Should devices have a default name?
- Should the name be derived from the device type or ID?
Let's verify the implementation:
✅ Verification successful
These scripts will help us understand:
- How device members are handled in the adapter
- Where and how member objects are created/mapped
- The type definitions for member objects to verify if name is optional
Having undefined name for device members is correct behavior
Based on the code analysis:
- The
membershipToMembertransformation doesn't include a name field in its mapping - The
fromSDKMembershipfunction only maps person-related fields likepersonDisplayName - Device members are handled differently from person members, and the test correctly reflects that devices don't have a name property
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for device member handling in the adapter implementation
rg -l "device.*member" src/
Length of output: 29
Script:
#!/bin/bash
# Let's try a broader search for device-related code and member handling
rg -A 5 "device" src/MembershipsSDKAdapter
Length of output: 108
Script:
#!/bin/bash
# Search for member object construction or mapping
rg -A 5 "mapMember|createMember" src/
Length of output: 39
Script:
#!/bin/bash
# Look for member type definitions or interfaces
ast-grep --pattern 'interface $_Member {
$$$
name?: $_
$$$
}'
Length of output: 69
Script:
#!/bin/bash
# Let's try to find the correct file name first
fd "Membership.*" src/
# Then search for device-related code in test file
rg -A 5 "device" src/MembershipsSDKAdapter.test.js
# Look for member object construction in the test file
rg -A 5 "member" src/MembershipsSDKAdapter.test.js
Length of output: 3069
Script:
#!/bin/bash
# Let's check the implementation file for device member handling
rg -A 5 "device" src/MembershipsSDKAdapter.js
# Look for member mapping or transformation logic
rg -A 10 "transform|map.*Member" src/MembershipsSDKAdapter.js
Length of output: 860
Script:
#!/bin/bash
# Let's look for the membershipToMember and fromSDKMembership transformations
rg -B 5 -A 10 "membershipToMember|fromSDKMembership" src/MembershipsSDKAdapter.js
# Also check for any device-specific member handling in the entire codebase
rg -l "deviceType|isDevice" src/
Length of output: 2177
src/MembershipsSDKAdapter.js
Outdated
| @@ -115,6 +115,7 @@ function getMembers(sdkMembers, meeting) { | |||
| sharing: member.isContentSharing, | |||
| host: !!meeting.meetingInfo.isWebexScheduled && member.isModerator, | |||
| guest: member.isGuest, | |||
| name:member.name, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for member.name property.
The name property is accessed directly from the member object without any validation. Consider adding a null check to maintain consistency with how other properties are handled in the function.
- name:member.name,
+ name: member.name || null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name:member.name, | |
| name: member.name || null, |
🧰 Tools
🪛 eslint
[error] 118-118: Missing space before value for key 'name'.
(key-spacing)
There was a problem hiding this comment.
What happens if this value comes as undefined? Do we have an edge check?
There was a problem hiding this comment.
@adhmenon, if member.name comes as undefined, the name property in the returned object would also be undefined. Currently, there is no edge check for this scenario in the function. Adding a null check like name: member.name || null ensures that when member.name is undefined, the name property is explicitly set to null. This maintains consistency with how other properties are handled and prevents potential issues if other parts of the code expect name to have a defined value.
src/MembershipsSDKAdapter.js
Outdated
| @@ -115,6 +115,7 @@ function getMembers(sdkMembers, meeting) { | |||
| sharing: member.isContentSharing, | |||
| host: !!meeting.meetingInfo.isWebexScheduled && member.isModerator, | |||
| guest: member.isGuest, | |||
| name:member.name, | |||
There was a problem hiding this comment.
What happens if this value comes as undefined? Do we have an edge check?
Yes @adhmenon the check for this is there in the components repo because this function will return the members object that the members object call actually has been there in the components repo so here we just include the name as a property in the members function previously it was not there if we include this property then only the user accepting wished name will be visible on widgets sample app |
|
@adhmenon for you question What happens if this value comes as undefined? Do we have an edge check? below is the answer to this question the logic is there in the components repo here we just passing this property in the return object of |
Parimala032
changed the title
|
🎉 This PR is included in version 1.112.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This pull request addresses
This PR enhances the getMembers function by including the
nameproperty in the returned member objects. By adding thename: member.name, each member object now contains the participant's display name in a local sample app of widgets, improving the comprehensiveness of the member data. If the user wanted to pass thenameaccording to there wish that will be appear on widgets sample app (or) the system default name will be displayedplease do refer to the screenrecording
sdk-component.recording.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Tests