Updating Chat prop to include support for external users. #14711
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe changes in the pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (2)
components/microsoft_teams/microsoft_teams.app.mjs (2)
61-61: Consider enhancing the chat descriptionWhile accurate, the description could be more specific about the types of chats supported (e.g., one-on-one, group chats).
- description: "Team Chat (internal and external contacts)", + description: "Microsoft Teams chat (supports one-on-one and group chats with both internal team members and external contacts)",
73-75: Consider pagination and rate limiting for message fetchingFetching 50 messages per chat could hit API rate limits when there are many chats. Consider implementing rate limiting and pagination controls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/microsoft_teams/microsoft_teams.app.mjs(3 hunks)
🔇 Additional comments (1)
components/microsoft_teams/microsoft_teams.app.mjs (1)
8-8: LGTM: Description accurately reflects new capabilities
The updated description clearly communicates the app's enhanced functionality to support both internal and external communications.
| const members = await Promise.all(chat.members.map(async (member) => { | ||
| let displayName = member.displayName; | ||
|
|
||
| if (!displayName && messages.value.length > 0) { | ||
| const userMessage = messages.value.find((msg) => | ||
| msg.from?.user?.id === member.userId); | ||
| if (userMessage?.from?.user?.displayName) { | ||
| displayName = userMessage.from.user.displayName; | ||
| } | ||
| } | ||
|
|
||
| if (!displayName) { | ||
| try { | ||
| const userDetails = await this.makeRequest({ | ||
| path: `/users/${member.userId}`, | ||
| }); | ||
| displayName = userDetails.displayName; | ||
| } catch (err) { | ||
| displayName = "Unknown User"; | ||
| } | ||
| } | ||
|
|
||
| const isExternal = member.tenantId !== myTenantId || !member.tenantId; | ||
| return isExternal | ||
| ? `${displayName} (External)` | ||
| : displayName; | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and performance
The member resolution logic has several areas for improvement:
- Error handling is generic and could miss specific API error cases
- Multiple sequential API calls could impact performance
- No caching mechanism for user details
Consider implementing these improvements:
const members = await Promise.all(chat.members.map(async (member) => {
- let displayName = member.displayName;
+ // Cache key using userId
+ const cacheKey = `user_${member.userId}`;
+ let displayName = member.displayName || this._userCache?.get(cacheKey);
- if (!displayName && messages.value.length > 0) {
+ if (!displayName) {
const userMessage = messages.value.find((msg) =>
msg.from?.user?.id === member.userId);
if (userMessage?.from?.user?.displayName) {
displayName = userMessage.from.user.displayName;
}
- }
- if (!displayName) {
try {
const userDetails = await this.makeRequest({
path: `/users/${member.userId}`,
});
displayName = userDetails.displayName;
+ // Cache the result
+ this._userCache?.set(cacheKey, displayName);
} catch (err) {
- displayName = "Unknown User";
+ if (err.statusCode === 404) {
+ displayName = "User Not Found";
+ } else if (err.statusCode === 403) {
+ displayName = "Access Denied";
+ } else {
+ displayName = "Unknown User";
+ }
+ console.error(`Failed to fetch user details for ${member.userId}:`, err);
}
}Committable suggestion skipped: line range outside the PR's diff.
| async getAuthenticatedUserTenant() { | ||
| const { value } = await this.client() | ||
| .api("/organization") | ||
| .get(); | ||
| return value[0].id; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to tenant ID retrieval
The method assumes the organization API will always return results. Add proper error handling for edge cases.
async getAuthenticatedUserTenant() {
- const { value } = await this.client()
- .api("/organization")
- .get();
- return value[0].id;
+ try {
+ const { value } = await this.client()
+ .api("/organization")
+ .get();
+
+ if (!value || value.length === 0) {
+ throw new Error("No organization found");
+ }
+
+ return value[0].id;
+ } catch (error) {
+ console.error("Failed to fetch tenant ID:", error);
+ throw new Error("Unable to determine tenant ID");
+ }
},📝 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.
| async getAuthenticatedUserTenant() { | |
| const { value } = await this.client() | |
| .api("/organization") | |
| .get(); | |
| return value[0].id; | |
| }, | |
| async getAuthenticatedUserTenant() { | |
| try { | |
| const { value } = await this.client() | |
| .api("/organization") | |
| .get(); | |
| if (!value || value.length === 0) { | |
| throw new Error("No organization found"); | |
| } | |
| return value[0].id; | |
| } catch (error) { | |
| console.error("Failed to fetch tenant ID:", error); | |
| throw new Error("Unable to determine tenant ID"); | |
| } | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/microsoft_teams/sources/new-chat/new-chat.mjs (1)
Line range hint
20-26: Consider enhancing metadata generation for external usersThe
generateMetamethod might need to be updated to include information about external user participation in chats, which could be valuable for downstream processing.Consider adding external user indicators in the metadata to help consumers of this event distinguish between internal and external chats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
components/microsoft_teams/actions/create-channel/create-channel.mjs(1 hunks)components/microsoft_teams/actions/list-channels/list-channels.mjs(1 hunks)components/microsoft_teams/actions/list-shifts/list-shifts.mjs(1 hunks)components/microsoft_teams/actions/send-channel-message/send-channel-message.mjs(1 hunks)components/microsoft_teams/actions/send-chat-message/send-chat-message.mjs(1 hunks)components/microsoft_teams/package.json(1 hunks)components/microsoft_teams/sources/new-channel-message/new-channel-message.mjs(1 hunks)components/microsoft_teams/sources/new-channel/new-channel.mjs(1 hunks)components/microsoft_teams/sources/new-chat-message/new-chat-message.mjs(1 hunks)components/microsoft_teams/sources/new-chat/new-chat.mjs(1 hunks)components/microsoft_teams/sources/new-team-member/new-team-member.mjs(1 hunks)components/microsoft_teams/sources/new-team/new-team.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- components/microsoft_teams/actions/create-channel/create-channel.mjs
- components/microsoft_teams/actions/list-channels/list-channels.mjs
- components/microsoft_teams/actions/list-shifts/list-shifts.mjs
- components/microsoft_teams/actions/send-channel-message/send-channel-message.mjs
- components/microsoft_teams/actions/send-chat-message/send-chat-message.mjs
- components/microsoft_teams/package.json
- components/microsoft_teams/sources/new-channel-message/new-channel-message.mjs
- components/microsoft_teams/sources/new-channel/new-channel.mjs
- components/microsoft_teams/sources/new-chat-message/new-chat-message.mjs
- components/microsoft_teams/sources/new-team-member/new-team-member.mjs
- components/microsoft_teams/sources/new-team/new-team.mjs
🔇 Additional comments (1)
components/microsoft_teams/sources/new-chat/new-chat.mjs (1)
Line range hint 13-19: Consider enhancing chat resource retrieval for external users
The getResources method currently fetches chats without distinguishing between internal and external users. Based on the PR objectives, this method might need to be updated to handle external user scenarios.
Let's check if there are any existing implementations we can reference:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/microsoft_teams/microsoft_teams.app.mjs (1)
75-77: Consider pagination and limit optimization for message fetching.Fetching 50 messages for each chat could impact performance, especially with many chats. Consider:
- Reducing the initial fetch limit
- Implementing lazy loading
- Adding rate limiting protection
- path: `/chats/${chat.id}/messages?$top=50`, + path: `/chats/${chat.id}/messages?$top=20`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/microsoft_teams/microsoft_teams.app.mjs(3 hunks)
🔇 Additional comments (4)
components/microsoft_teams/microsoft_teams.app.mjs (4)
8-8: LGTM! Description accurately reflects the new functionality.
The updated description clearly communicates the app's enhanced capability to support both internal and external communications.
113-117: LGTM! Robust external user detection.
The implementation correctly identifies external users by comparing tenant IDs and handles the case where tenant ID might be missing. The display name formatting clearly distinguishes external users.
195-210: LGTM! Robust error handling implementation.
The getAuthenticatedUserTenant method includes:
- Proper null/empty checks
- Descriptive error messages
- Error logging
- Clean error propagation
75-77: Verify API usage patterns and rate limits.
The implementation makes multiple API calls for each chat:
- Fetching messages
- Fetching user details
Let's verify the API usage patterns to ensure we're within rate limits:
Also applies to: 94-96
✅ Verification successful
API usage patterns are well-implemented with proper error handling
The implementation shows a well-structured approach to API usage:
- Messages are fetched with a reasonable limit of 50 items per request
- Error handling is implemented for user detail fetches (404, 403 cases)
- The code uses proper pagination through the
nextLinkparameter - API calls are centralized through the
makeRequestmethod - No evidence of aggressive polling or potential rate limit issues
The parallel processing of user details is done through Promise.all with proper error boundaries, and the implementation includes caching of user details which helps reduce API calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check API calls patterns in the codebase
# Check for potential API call patterns that might hit rate limits
echo "Checking API call patterns..."
# Look for message API calls
rg -A 2 "\/messages\?.*top=" --type js
# Look for user detail API calls
rg -A 2 "\/users\/.*" --type js
# Look for organization API calls
rg -A 2 "\/organization" --type js
Length of output: 424
Script:
#!/bin/bash
# Description: Check API call patterns in the codebase with corrected regex
# Look for message API calls
echo "Checking message API calls..."
rg -A 2 "messages\?" --type js
# Look for user detail API calls
echo "Checking user API calls..."
rg -A 2 "/users/" --type js
# Look for chat API calls
echo "Checking chat API calls..."
rg -A 2 "/chats/" --type js
# Look for rate limit handling
echo "Checking rate limit handling..."
rg -A 2 "rate.?limit" -i --type js
Length of output: 776
Script:
#!/bin/bash
# Description: Check API patterns in the Microsoft Teams component
# Look for API calls in the Microsoft Teams component
echo "Checking Microsoft Teams API patterns..."
rg -A 5 "makeRequest" components/microsoft_teams/
# Look for pagination or batch processing
echo "Checking pagination handling..."
rg -A 5 "\$top|\$skip" components/microsoft_teams/
# Look for error handling
echo "Checking error handling..."
rg -A 5 "catch|throw" components/microsoft_teams/
# Look for retry logic
echo "Checking retry mechanisms..."
rg -A 5 "retry|backoff" -i components/microsoft_teams/
Length of output: 10131
| const options = []; | ||
|
|
||
| this._userCache = this._userCache || new Map(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cache implementation.
The current cache implementation could be improved:
- Cache is recreated on each options call
- No cache expiration mechanism
- No cache size limits
Consider implementing a more robust caching solution:
+ _initCache() {
+ if (!this._userCache) {
+ this._userCache = new Map();
+ this._cacheExpiry = new Map();
+ this._maxCacheSize = 1000;
+ this._cacheTimeout = 3600000; // 1 hour
+ }
+ },
+ _getCachedUser(userId) {
+ this._initCache();
+ const now = Date.now();
+ if (this._cacheExpiry.get(userId) < now) {
+ this._userCache.delete(userId);
+ this._cacheExpiry.delete(userId);
+ return null;
+ }
+ return this._userCache.get(userId);
+ },
+ _setCachedUser(userId, displayName) {
+ this._initCache();
+ if (this._userCache.size >= this._maxCacheSize) {
+ const oldestKey = this._cacheExpiry.keys().next().value;
+ this._userCache.delete(oldestKey);
+ this._cacheExpiry.delete(oldestKey);
+ }
+ this._userCache.set(userId, displayName);
+ this._cacheExpiry.set(userId, Date.now() + this._cacheTimeout);
+ },Also applies to: 80-81
| @@ -5,7 +5,7 @@ import constants from "./common/constants.mjs"; | |||
| export default { | |||
| type: "app", | |||
| app: "microsoft_teams", | |||
| description: "**Personal accounts are not currently supported by Microsoft Teams.** Refer to Microsoft's documentation [here](https://learn.microsoft.com/en-us/graph/permissions-reference#remarks-7) to learn more.", | |||
| description: "Connect and interact with Microsoft Teams, supporting both internal and external communications.", | |||
There was a problem hiding this comment.
I assume removing the dcumentation link is intentional here
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/microsoft_teams/microsoft_teams.app.mjs (1)
75-77: Consider pagination impact on performance.The code fetches the top 50 messages for each chat. For chats with many messages, this could impact performance. Consider reducing the message limit or implementing progressive loading.
- path: `/chats/${chat.id}/messages?$top=50`, + path: `/chats/${chat.id}/messages?$top=10`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/microsoft_teams/microsoft_teams.app.mjs(2 hunks)
🔇 Additional comments (5)
components/microsoft_teams/microsoft_teams.app.mjs (5)
61-61: LGTM: Description accurately reflects new external user support.
The updated description clearly communicates the expanded functionality to support both internal and external contacts.
113-116: LGTM: Proper external user detection and labeling.
The implementation correctly:
- Uses tenant ID comparison for external user detection
- Handles cases where tenant ID might be missing
- Clearly labels external users in the display
195-210: LGTM: Robust tenant ID retrieval implementation.
The implementation includes proper error handling with:
- Empty value checks
- Error logging
- Meaningful error messages
72-72: 🛠️ Refactor suggestion
Enhance cache implementation for better performance.
While basic caching is implemented, it could be improved with:
- Cache expiration
- Size limits
- Error state caching to prevent repeated failed requests
This issue was previously identified in a past review comment. The suggested implementation for enhanced caching with expiration and size limits still applies.
Also applies to: 80-81
83-110: Verify external user data access handling.
Let's verify the error handling for external user data access across the codebase.
✅ Verification successful
External user data access is properly secured
The code implements appropriate security measures for handling external user data:
- Proper error handling for 403 (Access Denied) and 404 (User Not Found) status codes
- User data is safely cached with appropriate fallback values
- Clear error logging for troubleshooting
- Consistent error handling patterns across the Microsoft Teams component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper external user error handling patterns
# Look for similar error handling patterns for external user access
rg -A 2 "statusCode === (403|404)" --type js
# Check for potential security-sensitive API calls
ast-grep --pattern 'makeRequest({
path: "/users/$_",
$$$
})'
Length of output: 113
Script:
#!/bin/bash
# Let's try a broader search for error handling patterns and user-related API calls
# Search for error status code handling patterns with more variations
rg -A 3 "err\.statusCode" --type js
# Look for user-related API endpoints and error handling
rg -A 3 "(User Not Found|Access Denied|Unknown User)" --type js
# Check for makeRequest patterns with user endpoints using a more flexible pattern
ast-grep --pattern 'makeRequest({
path: $_,
$$$
})'
Length of output: 161
Script:
#!/bin/bash
# Let's try one final search with different patterns
# Search for error handling in a more generic way
rg -A 3 "catch.*err" --type js
# Look for user-related API calls with different patterns
rg -A 3 "\/users\/.*" --type js
# Search for cache-related operations for user data
rg -A 3 "_userCache" --type js
Length of output: 2218
Script:
#!/bin/bash
# Fix the regex pattern for user-related API calls and try one last time
# Search for user-related API calls with fixed pattern
rg -A 3 '/users/' --type js
# Look for error handling specifically in the Microsoft Teams component
rg -A 3 "catch.*err" components/microsoft_teams/
# Search for user cache operations in Microsoft Teams component
rg -A 3 "_userCache" components/microsoft_teams/
Length of output: 2513
WHY
Updating Chat prop for Microsoft Teams to include external users, and be able to label external users as such.
See example workflow here.
You can test on Microsoft Teams (web) if you don't have the app here:
https://teams.microsoft.com/v2/
Please use the [email protected] account in 1Pass.
Summary by CodeRabbit
Summary by CodeRabbit