-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Feat(microsoftteams-file): new trigger + file upload #1590
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
Conversation
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.
Greptile Overview
Summary
Added Microsoft Teams chat subscription trigger using Microsoft Graph API change notifications, enabling workflows to react to new chat messages with automatic file attachment downloads.
Major Changes:
- Implemented new
microsoftteams_chat_subscriptiontrigger that subscribes to Teams chat messages via Graph API - Added automatic file download and S3 upload for message attachments from SharePoint/OneDrive URLs with multiple fallback strategies
- Created background job to renew Teams subscriptions every 2 days before they expire (max lifetime ~3 days)
- Added validation token handling for Graph API subscription verification
- Integrated idempotency protection using
x-teams-notification-idheader to prevent duplicate message processing - Extended
read_channelandread_chattools with optionalincludeAttachmentsparameter for hosted contents
Implementation Details:
- Subscription lifecycle: create → validate → receive notifications → auto-renew → expire
- File attachments are fetched from various sources (SharePoint, OneDrive, direct URLs) and uploaded to S3 with presigned URLs
- Changed Teams webhook response from JSON to 202 Accepted for proper async processing
Confidence Score: 3/5
- PR has solid architecture but contains a critical bug that will break multi-workflow Teams setups
- The subscription cleanup logic in
teams-subscriptions.ts:46-48deletes ALL chat subscriptions for a credential instead of just the one being replaced, which will break other workflows using Teams triggers. The file attachment download logic is complex with many silent error handlers that could hide issues. The renewal timing (check 24h, run every 2 days) has a potential race condition if jobs are delayed. - Pay close attention to
apps/sim/lib/webhooks/teams-subscriptions.ts(subscription cleanup bug) andapps/sim/lib/webhooks/utils.ts(complex file download logic with silent errors)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/lib/webhooks/utils.ts | 3/5 | Added complex Teams Graph notification formatting with file download/upload logic for attachments from SharePoint/OneDrive URLs. File download logic has extensive fallback mechanisms but silently continues on errors. |
| apps/sim/triggers/microsoftteams/chat_webhook.ts | 5/5 | New trigger configuration for Microsoft Teams chat subscriptions. Clean configuration with appropriate scopes and output definitions. |
| apps/sim/lib/webhooks/teams-subscriptions.ts | 4/5 | Creates Microsoft Teams Graph subscriptions and cleans up old subscriptions. Logic appears sound but deletes ALL chat subscriptions matching URL pattern, not just those for this webhook. |
| apps/sim/background/teams-subscription-renewal.ts | 4/5 | Background job for renewing Teams subscriptions before expiry. Includes fallback to recreate if subscription not found. Well-structured with proper error handling. |
| apps/sim/lib/webhooks/processor.ts | 4/5 | Added Teams notification idempotency support and improved inactive webhook logging. Changed response from JSON to 202 for Teams webhooks. |
Sequence Diagram
sequenceDiagram
participant User
participant TeamsClient as Microsoft Teams
participant GraphAPI as MS Graph API
participant Webhook as /api/webhooks/trigger/[path]
participant Processor as Webhook Processor
participant Queue as Background Queue
participant Executor as Webhook Execution
participant S3 as S3 Storage
participant Workflow as Workflow Engine
Note over User,Workflow: Setup Phase
User->>Webhook: POST /api/webhooks (create subscription)
Webhook->>GraphAPI: POST /subscriptions (create chat subscription)
GraphAPI-->>Webhook: Subscription ID + expiration
Webhook->>Webhook: Store subscription ID in providerConfig
Webhook-->>User: Success
Note over User,Workflow: Message Received
TeamsClient->>GraphAPI: User sends message in chat
GraphAPI->>Webhook: POST notification (changeType: created)
Webhook->>Webhook: Validate notification token
Webhook->>Processor: findWebhookAndWorkflow(path)
Processor->>Processor: Check idempotency (x-teams-notification-id)
alt First time processing
Processor->>GraphAPI: GET /chats/{chatId}/messages/{messageId}
GraphAPI-->>Processor: Message details
opt includeAttachments=true
Processor->>GraphAPI: GET hostedContents
GraphAPI-->>Processor: Base64 content
loop For each attachment
Processor->>GraphAPI: Fetch from SharePoint/OneDrive URL
GraphAPI-->>Processor: File buffer
Processor->>S3: Upload file
S3-->>Processor: S3 key + presigned URL
end
end
Processor->>Queue: Enqueue webhook execution
Queue-->>Webhook: 202 Accepted
Queue->>Executor: Execute webhook job
Executor->>Workflow: Trigger workflow with message + attachments
Workflow-->>Executor: Execution complete
else Duplicate notification
Processor-->>Webhook: 202 Accepted (idempotent)
end
Note over GraphAPI,Webhook: Subscription Renewal (every 2 days)
GraphAPI->>GraphAPI: Check subscriptions expiring < 24h
GraphAPI->>GraphAPI: PATCH /subscriptions/{id} (renew)
GraphAPI->>GraphAPI: Update expiration in DB
22 files reviewed, 4 comments
| const chatSubscriptions = allSubscriptions.filter((sub: any) => | ||
| sub.resource?.includes('/chats/') | ||
| ) |
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.
logic: deletes ALL chat subscriptions for this credential that match the URL pattern, not just subscriptions for this specific webhook. if the user has multiple workflows with Teams chat triggers, this will break other workflows' subscriptions.
| const chatSubscriptions = allSubscriptions.filter((sub: any) => | |
| sub.resource?.includes('/chats/') | |
| ) | |
| const chatSubscriptions = allSubscriptions.filter((sub: any) => | |
| sub.resource?.includes('/chats/') && sub.clientState === webhookData.id | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/lib/webhooks/teams-subscriptions.ts
Line: 46:48
Comment:
**logic:** deletes ALL chat subscriptions for this credential that match the URL pattern, not just subscriptions for this specific webhook. if the user has multiple workflows with Teams chat triggers, this will break other workflows' subscriptions.
```suggestion
const chatSubscriptions = allSubscriptions.filter((sub: any) =>
sub.resource?.includes('/chats/') && sub.clientState === webhookData.id
)
```
How can I resolve this? If you propose a fix, please make it concise.
| const encodedUrl = Buffer.from(contentUrl) | ||
| .toString('base64') | ||
| .replace(/\+/g, '-') | ||
| .replace(/\//g, '_') | ||
| .replace(/=+$/, '') |
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.
style: base64 encoding should use 'utf-8' encoding explicitly, or better yet use encodeURIComponent for URL encoding instead of base64
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/lib/webhooks/utils.ts
Line: 296:300
Comment:
**style:** base64 encoding should use 'utf-8' encoding explicitly, or better yet use `encodeURIComponent` for URL encoding instead of base64
How can I resolve this? If you propose a fix, please make it concise.
| url, | ||
| sourceUrl: contentUrl, | ||
| }) | ||
| } catch {} |
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.
style: empty catch blocks swallow all errors including critical ones like S3 upload failures. at minimum, log the error
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/lib/webhooks/utils.ts
Line: 466:466
Comment:
**style:** empty catch blocks swallow all errors including critical ones like S3 upload failures. at minimum, log the error
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| try { | ||
| // Find all Microsoft Teams webhooks with chat subscriptions that expire soon | ||
| // Check for subscriptions expiring within the next 24 hours |
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.
style: checking for subscriptions expiring within 24 hours but job runs every 2 days according to comment on line 16. if the job is delayed, subscriptions may expire before renewal
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/background/teams-subscription-renewal.ts
Line: 23:23
Comment:
**style:** checking for subscriptions expiring within 24 hours but job runs every 2 days according to comment on line 16. if the job is delayed, subscriptions may expire before renewal
How can I resolve this? If you propose a fix, please make it concise.
3c012ed to
507790b
Compare
6e3b8d8 to
66b4143
Compare
| } | ||
| } | ||
|
|
||
| // If it's a Microsoft Teams webhook with a subscription, delete the Graph subscription |
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.
why are we doing this....
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.
we want to remove any external Graph subscription as well, and since we run the renewal subscription we need to delete it on the graph end too.
| // If path is missing | ||
| if (!finalPath || finalPath.trim() === '') { | ||
| if (isCredentialBased) { | ||
| if (isCredentialBased || isMicrosoftTeamsChatSubscription) { |
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.
why are we treating a subscription to a chat as credential based?
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.
bc we need the credential to create the graph subscription, we create the callback when we create this subscription
| const [copied, setCopied] = useState<string | null>(null) | ||
| const accessiblePrefixes = useAccessibleReferencePrefixes(blockId) | ||
|
|
||
| // Sync credential field values from subblock store to config |
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.
Is this intended -- this runs on all triggers right?
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.
it returns early when there are no credential fields set, supposed to target the credential-based triggers like teams chat
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.
fix bc if we open the modal, we screw up the other guys stuf.
| * Background job to renew Microsoft Teams Graph API subscriptions before they expire. | ||
| * Runs periodically to check for subscriptions expiring soon and renews them. | ||
| */ | ||
| export const renewTeamsSubscriptions = task({ |
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.
you made a new trigger.dev task to renew chat subscriptions?
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.
use verifyCronAuth (verifyCronSecret)
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.
confirm:
availableTriggers={availableTriggers}
selectedTriggerId={selectedTriggerId}
onTriggerChange={(newTriggerId) => {
setStoredTriggerId(newTriggerId)
// Clear config when changing trigger type
setTriggerConfig({})
}}
* adding file logic and chat trigger * working trig * teams specific logic * greptile comments * lint * cleaned up * save modal changes * created a interface for subscriptions * removed trigger task * reduce comments * removed trig task * removed comment * simplified * added tele logic back * addressed some more comments * simplified db call * cleaned up utils * helper telegram * removed fallback * removed scope * simplify to use helpers * fix credential resolution * add logs * fix * fix attachment case --------- Co-authored-by: Adam Gough <adamgough@Mac.attlocal.net> Co-authored-by: Adam Gough <adamgough@Adams-MacBook-Pro.local> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
Summary
Added new teams trigger with file upload so users can access their files from teams chat.
Type of Change
Testing
Tested with uploading files and the new trigger system.
Checklist