fix(conversation): pass actorId to addReaction and deleteReaction #4037
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) WalkthroughThis pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant Actor
participant Conversation
participant ReactionHandler
Actor->>Conversation: addReaction(conversation, displayName, parentActivity, actorId, recipientId)
Conversation->>ReactionHandler: Process reaction with actorId
ReactionHandler-->>Conversation: Reaction added
Actor->>Conversation: deleteReaction(conversation, reactionId, actorId, recipientId)
Conversation->>ReactionHandler: Remove reaction with actorId
ReactionHandler-->>Conversation: Reaction deleted
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (1)
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js (1)
69-79: Consider adding test for null actorId caseWhile the tests cover explicit actorId and undefined cases, consider adding a test for null actorId to ensure complete coverage of the nullish coalescing operator behavior.
it('should use fallback device.userId if actorId is null', async () => { const defaultUserId = 'fallback-id'; webex.internal.device.userId = defaultUserId; await conversation.addReaction({}, 'example-display-name', {}, null, recipientId); assert.equal(conversation.sendReaction.args[0][1].actor.id, defaultUserId); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/internal-plugin-conversation/src/conversation.js(3 hunks)packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js(1 hunks)
🔇 Additional comments (5)
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js (2)
40-68: Well-structured test setup and basic functionality coverage!
The test suite properly verifies the core functionality including recipients handling and HMAC creation behavior.
83-111: Consistent test structure with addReaction!
The deleteReaction test suite maintains consistency with addReaction tests, providing parallel coverage for both functions.
packages/@webex/internal-plugin-conversation/src/conversation.js (3)
Line range hint 306-334: Clean implementation with proper fallback handling!
The implementation correctly:
- Uses nullish coalescing (
??) for actorId fallback - Maintains the existing functionality
- Properly structures the deleteReaction payload
Line range hint 338-376: Clean implementation with consistent async/await usage!
The implementation correctly:
- Uses nullish coalescing (
??) for actorId fallback - Maintains the existing HMAC creation logic
- Properly structures the addReaction payload
- Uses async/await syntax consistently
Line range hint 306-376: Verify impact on existing reaction functionality
While the implementation looks solid, let's verify that existing code paths using these functions continue to work as expected.
✅ Verification successful
Changes are safe and well-tested
The verification shows that the changes are safe because:
- Unit tests explicitly verify both the new
actorIdparameter behavior and fallback todevice.userId - Integration tests demonstrate the functions work in real scenarios
- All existing usages in integration tests don't pass the optional
actorId, relying on the fallback behavior - Test coverage includes verification of both successful paths and edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of addReaction and deleteReaction
# to ensure they're compatible with the new signatures
echo "Searching for addReaction usages..."
rg "addReaction\(" -A 5
echo "Searching for deleteReaction usages..."
rg "deleteReaction\(" -A 5
Length of output: 10867
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
This pull request addresses
in messaging widget iframe, i would like to use conversation functions, without depending on webex.internal.device.userId being set (iframe case, no auth/registration of SDK). number of args to reaction functions has changed
by making the following changes
allow addReaction and deleteReaction to accept an actorId. if present, use it, else fallback to webex.internal.device.userId
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
addReactionanddeleteReactionmethods.Bug Fixes
addReactionanddeleteReactionto ensure correct handling of theactorIdparameter.Documentation
Tests
async/awaitfor better readability and added new test cases foractorIdscenarios.