improvement(webhooks): lifecycle management with external providers, remove save configuration #2831
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Greptile SummaryRefactors webhook lifecycle management to occur during workflow deployment rather than requiring a separate "Save Configuration" button. This significantly improves UX by removing a manual step. Key Changes
Implementation DetailsThe new flow:
Webhook recreation is intelligently triggered when relevant config changes are detected (via Issues Found
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant UI as Deploy Modal
participant API as Deploy API
participant Deploy as saveTriggerWebhooksForDeploy
participant Provider as Provider Subscriptions
participant External as External Provider APIs
participant DB as Database
User->>UI: Click Deploy
UI->>API: POST /api/workflows/{id}/deploy
API->>API: Validate permissions & schedules
API->>Deploy: saveTriggerWebhooksForDeploy()
loop For each trigger block
Deploy->>Deploy: Resolve trigger ID & build config
Deploy->>Deploy: Validate required fields
alt Credential Set
Deploy->>DB: syncWebhooksForCredentialSet()
DB->>DB: Create webhooks for each credential
else Single Credential
Deploy->>DB: Query existing webhook
alt Webhook exists
Deploy->>Provider: shouldRecreateExternalWebhookSubscription()
alt Config changed
Provider->>External: cleanupExternalWebhook()
Provider->>External: createExternalWebhookSubscription()
External-->>Provider: Return external ID
end
Deploy->>DB: Update webhook record
else New webhook
Provider->>External: createExternalWebhookSubscription()
External-->>Provider: Return external ID
Deploy->>DB: Insert webhook record
end
end
end
Deploy-->>API: Success/Error
API->>DB: deployWorkflow()
API->>DB: createSchedulesForDeploy()
API-->>UI: Deployment result
UI-->>User: Show success or error
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+359
to
+368
| const finalProviderConfig = { | ||
| ...nextProviderConfig, | ||
| credentialId: existingConfig.credentialId, | ||
| credentialSetId: existingConfig.credentialSetId, | ||
| userId: existingConfig.userId, | ||
| historyId: existingConfig.historyId, | ||
| lastCheckedTimestamp: existingConfig.lastCheckedTimestamp, | ||
| setupCompleted: existingConfig.setupCompleted, | ||
| externalId: nextProviderConfig.externalId ?? existingConfig.externalId, | ||
| } |
Contributor
There was a problem hiding this comment.
logic: Credential fields from existingConfig always override nextProviderConfig, even when new credentials are provided
The merge logic preserves credentialId and credentialSetId from the existing config unconditionally. If a user updates credentials during redeployment, the new values in providerConfig will be overwritten with old values.
Consider checking if new credential values are present in providerConfig before falling back to existing ones:
Suggested change
| const finalProviderConfig = { | |
| ...nextProviderConfig, | |
| credentialId: existingConfig.credentialId, | |
| credentialSetId: existingConfig.credentialSetId, | |
| userId: existingConfig.userId, | |
| historyId: existingConfig.historyId, | |
| lastCheckedTimestamp: existingConfig.lastCheckedTimestamp, | |
| setupCompleted: existingConfig.setupCompleted, | |
| externalId: nextProviderConfig.externalId ?? existingConfig.externalId, | |
| } | |
| const finalProviderConfig = { | |
| ...nextProviderConfig, | |
| credentialId: providerConfig.credentialId ?? existingConfig.credentialId, | |
| credentialSetId: providerConfig.credentialSetId ?? existingConfig.credentialSetId, | |
| userId: existingConfig.userId, | |
| historyId: existingConfig.historyId, | |
| lastCheckedTimestamp: existingConfig.lastCheckedTimestamp, | |
| setupCompleted: existingConfig.setupCompleted, | |
| externalId: nextProviderConfig.externalId ?? existingConfig.externalId, | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/lib/webhooks/deploy.ts
Line: 359:368
Comment:
**logic:** Credential fields from `existingConfig` always override `nextProviderConfig`, even when new credentials are provided
The merge logic preserves `credentialId` and `credentialSetId` from the existing config unconditionally. If a user updates credentials during redeployment, the new values in `providerConfig` will be overwritten with old values.
Consider checking if new credential values are present in `providerConfig` before falling back to existing ones:
```suggestion
const finalProviderConfig = {
...nextProviderConfig,
credentialId: providerConfig.credentialId ?? existingConfig.credentialId,
credentialSetId: providerConfig.credentialSetId ?? existingConfig.credentialSetId,
userId: existingConfig.userId,
historyId: existingConfig.historyId,
lastCheckedTimestamp: existingConfig.lastCheckedTimestamp,
setupCompleted: existingConfig.setupCompleted,
externalId: nextProviderConfig.externalId ?? existingConfig.externalId,
}
```
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Will do webhook lifecycle management as part of deploy process.
Type of Change
Testing
Tested manually
Checklist