Uh oh!
There was an error while loading. Please reload this page.
feat(triggers): modify triggers to use existing subblock system, webhook order of operations improvements - #1774
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. |
…till fix webhook URL on multiselect and add script in text subblock for google form
… better errors for ones that require external connections
…mat from old triggers
…ig field names that were changed
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the trigger system to use the existing subblock architecture and significantly improves webhook lifecycle management.
Key Changes:
Trigger System Refactor: Migrated from modal-based
configFieldsto declarativesubBlocks, making triggers consistent with the rest of the block system. All 16 trigger types updated with new structure including inline instructions and sample payloads.Webhook Order of Operations Fix: External webhook subscriptions (Airtable, Teams, Telegram, Webflow) are now created before saving to the database. This prevents orphaned database records when external API calls fail. Previously, the DB record would be created first, then external setup would fail, leaving orphaned records.
External Connection Cleanup: Added
cleanupExternalWebhook()function that properly deletes external webhook subscriptions when webhooks are deleted. Also integrated into block deletion flow insocket-server/database/operations.tsto prevent orphaned external connections when blocks are removed.Backwards Compatibility: Implemented
populateTriggerFieldsFromConfig()inuse-trigger-config-aggregation.tsto migrate oldtriggerConfigobjects to individual subblock fields. Field name mapping handles renames likecredentialId→triggerCredentials.Dropdown Multi-Select: Enhanced dropdown component with native multi-select support and async option fetching, removing the need for a dedicated multi-select subblock.
New Components: Added
trigger-savesubblock component with validation, error handling, and test URL generation capabilities.
The refactor maintains full backwards compatibility while modernizing the architecture and fixing critical webhook lifecycle issues.
Confidence Score: 4/5
- Safe to merge with minor verification needed for external API error handling edge cases
- The PR implements significant architectural improvements with proper error handling and backwards compatibility. The webhook order of operations fix addresses a real issue with orphaned records. However, the extensive refactoring touches many critical paths (16 trigger types, webhook lifecycle, socket operations) and relies on manual testing. Score is 4 instead of 5 due to the breadth of changes and potential edge cases in external API interactions.
- apps/sim/app/api/webhooks/route.ts - Verify external API error handling covers all failure modes
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/app/api/webhooks/route.ts | 4/5 | Improved webhook creation with external subscription setup before DB save; includes proper error handling and backwards compatibility for credential-based providers |
| apps/sim/app/api/webhooks/[id]/route.ts | 5/5 | Clean webhook deletion with external connection cleanup; well-structured permission checking |
| apps/sim/lib/webhooks/webhook-helpers.ts | 5/5 | Centralized external webhook cleanup logic for Airtable, Teams, and Telegram with proper error handling and fallback mechanisms |
| apps/sim/hooks/use-webhook-management.ts | 4/5 | Comprehensive webhook lifecycle management with backwards compatibility migration and proper state tracking |
| apps/sim/socket-server/database/operations.ts | 5/5 | Added webhook cleanup on block deletion to prevent orphaned external connections |
Sequence Diagram
sequenceDiagram
participant User
participant TriggerSave as Trigger Save Component
participant WebhookMgmt as useWebhookManagement Hook
participant API as POST /api/webhooks
participant ExternalAPI as External Provider API<br/>(Airtable/Teams/Telegram/Webflow)
participant DB as Database
participant Cleanup as Webhook Cleanup
Note over User,Cleanup: Webhook Creation Flow (Order of Operations)
User->>TriggerSave: Click "Save Configuration"
TriggerSave->>TriggerSave: Aggregate trigger config from subblocks
TriggerSave->>TriggerSave: Validate required fields
TriggerSave->>WebhookMgmt: saveConfig()
WebhookMgmt->>API: POST with config
Note over API,ExternalAPI: NEW: External setup BEFORE DB save
alt Provider needs external subscription
API->>ExternalAPI: Create webhook/subscription
ExternalAPI-->>API: Return externalId or error
alt External creation fails
API-->>WebhookMgmt: Return error (no orphan!)
WebhookMgmt-->>TriggerSave: Show error to user
TriggerSave-->>User: Display error message
end
end
Note over API,DB: Only save to DB if external setup succeeded
API->>DB: INSERT/UPDATE webhook record
DB-->>API: Webhook saved
alt Credential-based provider (Gmail/Outlook)
API->>API: Configure polling (post-save)
end
API-->>WebhookMgmt: Success with webhookId
WebhookMgmt->>WebhookMgmt: Update local state
WebhookMgmt-->>TriggerSave: Return success
TriggerSave-->>User: Show "Saved" confirmation
Note over User,Cleanup: Webhook Deletion Flow
User->>TriggerSave: Click delete button
TriggerSave->>TriggerSave: Show confirmation dialog
User->>TriggerSave: Confirm deletion
TriggerSave->>WebhookMgmt: deleteConfig()
WebhookMgmt->>API: DELETE /api/webhooks/{id}
API->>Cleanup: cleanupExternalWebhook()
alt Airtable webhook
Cleanup->>ExternalAPI: DELETE webhook
ExternalAPI-->>Cleanup: Confirmation
else Teams subscription
Cleanup->>ExternalAPI: DELETE subscription
ExternalAPI-->>Cleanup: Confirmation
else Telegram webhook
Cleanup->>ExternalAPI: DELETE webhook
ExternalAPI-->>Cleanup: Confirmation
end
Cleanup-->>API: Cleanup complete (non-blocking)
API->>DB: DELETE webhook record
DB-->>API: Deleted
API-->>WebhookMgmt: Success
WebhookMgmt->>WebhookMgmt: Clear local state
WebhookMgmt-->>TriggerSave: Success
TriggerSave-->>User: Confirmation
Note over User,Cleanup: Block Deletion Flow (NEW)
User->>User: Delete trigger block
User->>DB: Socket operation: remove block
DB->>DB: Find webhooks for blockId
DB->>Cleanup: cleanupExternalWebhook() for each
Cleanup->>ExternalAPI: Delete external connections
ExternalAPI-->>Cleanup: Confirmation
DB->>DB: DELETE webhooks
DB->>DB: DELETE block
DB-->>User: Block deleted
62 files reviewed, 3 comments
| // Create external subscriptions before saving to DB to prevent orphaned records | ||
| let externalSubscriptionId: string | undefined | ||
| const createTempWebhookData = () => ({ | ||
| id: targetWebhookId || nanoid(), | ||
| path: finalPath, | ||
| providerConfig: finalProviderConfig, | ||
| }) | ||
| if (provider === 'airtable') { | ||
| logger.info(`[${requestId}] Creating Airtable subscription before saving to database`) | ||
| try { | ||
| externalSubscriptionId = await createAirtableWebhookSubscription( | ||
| request, | ||
| userId, | ||
| createTempWebhookData(), | ||
| requestId | ||
| ) | ||
| if (externalSubscriptionId) { | ||
| finalProviderConfig.externalId = externalSubscriptionId | ||
| } | ||
| } catch (err) { | ||
| logger.error(`[${requestId}] Error creating Airtable webhook subscription`, err) | ||
| return NextResponse.json( | ||
| { | ||
| error: 'Failed to create webhook in Airtable', | ||
| details: err instanceof Error ? err.message : 'Unknown error', | ||
| }, | ||
| { status: 500 } | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: If external subscription creation fails after creating the subscription but before storing externalId, the external subscription becomes orphaned (no DB record to track it for later cleanup).
Consider wrapping in try-catch and cleaning up the created subscription if DB save fails:
if(externalSubscriptionId){finalProviderConfig.externalId=externalSubscriptionId}Then if DB save fails, call cleanup with the temp webhook data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/api/webhooks/route.ts
Line: 259:290
Comment:
**logic:** If external subscription creation fails after creating the subscription but before storing `externalId`, the external subscription becomes orphaned (no DB record to track it for later cleanup).
Consider wrapping in try-catch and cleaning up the created subscription if DB save fails:
```javascriptif (externalSubscriptionId) {
finalProviderConfig.externalId= externalSubscriptionId
}
```
Then if DB save fails, call cleanup with the temp webhook data.
How can I resolve this? If you propose a fix, please make it concise.| const currentConfig = useSubBlockStore.getState().getValue(blockId, 'triggerConfig') | ||
| if (JSON.stringify(webhook.providerConfig) !== JSON.stringify(currentConfig)) { | ||
| useSubBlockStore | ||
| .getState() | ||
| .setValue(blockId, 'triggerConfig', webhook.providerConfig) | ||
| if (effectiveTriggerId) { | ||
| populateTriggerFieldsFromConfig( | ||
| blockId, | ||
| webhook.providerConfig, | ||
| effectiveTriggerId | ||
| ) | ||
| } else { | ||
| logger.warn('Cannot migrate - triggerId not available', { | ||
| blockId, | ||
| propTriggerId: triggerId, | ||
| providerConfigTriggerId: webhook.providerConfig.triggerId, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: The backwards compatibility migration from triggerConfig to individual subblocks happens during webhook load, but if effectiveTriggerId cannot be determined, the migration is silently skipped with just a warning.
This could leave users with loaded but non-functional trigger configurations. Consider showing a user-facing error or notice when migration fails due to missing triggerId.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/hooks/use-webhook-management.ts
Line: 176:196
Comment:
**logic:** The backwards compatibility migration from `triggerConfig` to individual subblocks happens during webhook load, but if `effectiveTriggerId` cannot be determined, the migration is silently skipped with just a warning.
This could leave users with loaded but non-functional trigger configurations. Consider showing a user-facing error or notice when migration fails due to missing `triggerId`.
How can I resolve this? If you propose a fix, please make it concise.| // Clean up external webhooks before deleting blocks | ||
| try { | ||
| const blockIdsArray = Array.from(blocksToDelete) | ||
| const webhooksToCleanup = await tx | ||
| .select({ | ||
| webhook: webhook, | ||
| workflow: { | ||
| id: workflow.id, | ||
| userId: workflow.userId, | ||
| workspaceId: workflow.workspaceId, | ||
| }, | ||
| }) | ||
| .from(webhook) | ||
| .innerJoin(workflow, eq(webhook.workflowId, workflow.id)) | ||
| .where(and(eq(webhook.workflowId, workflowId), inArray(webhook.blockId, blockIdsArray))) | ||
| if (webhooksToCleanup.length > 0) { | ||
| logger.debug( | ||
| `Found ${webhooksToCleanup.length} webhook(s) to cleanup for blocks: ${blockIdsArray.join(', ')}` | ||
| ) | ||
| const requestId = `socket-${workflowId}-${Date.now()}-${Math.random().toString(36).substring(7)}` | ||
| // Clean up each webhook (don't fail if cleanup fails) | ||
| for (const webhookData of webhooksToCleanup) { | ||
| try { | ||
| await cleanupExternalWebhook(webhookData.webhook, webhookData.workflow, requestId) | ||
| } catch (cleanupError) { | ||
| logger.warn( | ||
| `Failed to cleanup external webhook ${webhookData.webhook.id} during block deletion`, | ||
| cleanupError | ||
| ) | ||
| // Continue with deletion even if cleanup fails | ||
| } | ||
| } | ||
| } | ||
| } catch (webhookCleanupError) { | ||
| logger.warn( | ||
| `Error during webhook cleanup for block deletion (continuing with deletion)`, | ||
| webhookCleanupError |
There was a problem hiding this comment.
style: Webhook cleanup on block deletion is wrapped in try-catch with only error logging, allowing block deletion to proceed even if cleanup fails. This is good for UX but could leave orphaned external subscriptions.
Consider adding monitoring/alerting for cleanup failures, or a background job to retry failed cleanups.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/socket-server/database/operations.ts
Line: 411:450
Comment:
**style:** Webhook cleanup on block deletion is wrapped in try-catch with only error logging, allowing block deletion to proceed even if cleanup fails. This is good for UX but could leave orphaned external subscriptions.
Consider adding monitoring/alerting for cleanup failures, or a background job to retry failed cleanups.
How can I resolve this? If you propose a fix, please make it concise.Uh oh!
There was an error while loading. Please reload this page.
…ook order of operations improvements (#1774) * feat(triggers): make triggers use existing subblock system, need to still fix webhook URL on multiselect and add script in text subblock for google form * minimize added subblocks, cleanup code, make triggers first-class subblock users * remove multi select dropdown and add props to existing dropdown instead * cleanup dropdown * add socket op to delete external webhook connections on block delete * establish external webhook before creating webhook DB record, surface better errors for ones that require external connections * fix copy button in short-input * revert environment.ts, cleanup * add triggers registry, update copilot tool to reflect new trigger setup * update trigger-save subblock * clean * cleanup * remove unused subblock store op, update search modal to reflect list of triggers * add init from workflow to subblock store to populate new subblock format from old triggers * fix mapping of old names to new ones * added debug logging * remove all extraneous debug logging and added mapping for triggerConfig field names that were changed * fix trigger config for triggers w/ multiple triggers * edge cases for effectiveTriggerId * cleaned up * fix dropdown multiselect * fix multiselect * updated short-input copy button * duplicate blocks in trigger mode * ack PR comments
Summary
textandtrigger-savesubblocksType of Change
Testing
Tested extensively manually.
Checklist