feat: track rejected trigger invocations with error details #1570
Conversation
- Add 'rejected' status to TriggerInvocationStatusEnum - Add errorCode (int) and respondedAt (timestamp) columns to triggerInvocations - Refactor processWebhook to create rejected invocations at each validation failure point: - Trigger not found (404) - Trigger disabled (404) - Auth failure (401/403) - Signature verification failure (403/500) - Payload validation failure (400) - Transform failure (422) - Update webhooks route to return invocationId in error responses - Update UI to show rejected status, error codes, and response time - Add comprehensive tests for rejected invocation tracking
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Claude finished @amikofalvy's task —— View job Code Review: Track Rejected Trigger InvocationsReview Checklist:
SummaryThis is a well-architected feature that adds comprehensive tracking for rejected trigger invocations. The implementation demonstrates solid engineering practices with proper error handling, database design, and test coverage. However, there are several areas for improvement around API consistency, performance optimization, and system design considerations. Critical Issues🚨 API Contract Inconsistency (High Priority)Problem: The webhook API now sometimes returns Current OpenAPI spec: 400: { schema: z.object({ error: z.string(), validationErrors: z.array(z.string()).optional() }) }
404: { schema: z.object({ error: z.string() }) }Actual response: Impact: This is a breaking change that could cause client applications to fail or behave unexpectedly. Solution: Update the OpenAPI specs in 🔍 Race Condition Risk (Medium Priority)Location: const respondedAt = new Date().toISOString(); // Generated before DB write
await createTriggerInvocation(runDbClient)({ ... });Problem: The Solution: Consider setting Architectural & Design Feedback✅ Excellent Patterns
🎯 System Design Considerations1. Performance & ScalabilityThe current approach creates database records for every failed request, including malicious traffic: // This runs for ANY failed request, even obvious attacks
const invocationId = await createRejectedInvocation({ ... });Consider:
2. Observability EnhancementThe current error categorization could be more granular: Suggestion: Add rejectionReason: 'TRIGGER_NOT_FOUND' | 'TRIGGER_DISABLED' | 'AUTH_MISSING' | 'AUTH_INVALID' | 'SIGNATURE_INVALID' | 'PAYLOAD_INVALID' | 'TRANSFORM_FAILED'This would enable better metrics and alerting. 3. JSON Parsing ResilienceThe function safeParseJson(rawBody: string): Record<string, unknown> {
try {
return JSON.parse(rawBody) as Record<string, unknown>;
} catch {
return {}; // Information loss
}
}Issue: This loses information about what the client actually sent. Better approach: function safeParseJson(rawBody: string): { parsed: Record<string, unknown>; raw?: string } {
try {
return { parsed: JSON.parse(rawBody) };
} catch {
return { parsed: {}, raw: rawBody.slice(0, 1000) }; // Preserve original for debugging
}
}Technical Implementation ReviewDatabase Schema ✅The migration is well-designed:
UI Implementation ✅
Type Safety ✅Schema updates properly flow through the type system:
Minor Improvements1. Error Message ConsistencyDifferent failure modes use inconsistent error messages: "Trigger ${triggerId} not found" // Template literal
"Trigger is disabled" // Static stringSuggestion: Standardize error message format for better user experience. 2. Security ConsiderationThe 404 responses for both "not found" and "disabled" are good for security (no information leakage), but consider if disabled triggers should have different internal tracking. 3. Code OrganizationThe RecommendationsImmediate Actions
Future Enhancements
Monitoring & AlertingThis feature enables powerful observability. Consider setting up alerts for:
Overall AssessmentThis is a production-ready feature that significantly improves the platform's observability and debugging capabilities. The implementation follows sound engineering principles and maintains system consistency. The identified issues are addressable and don't block the core functionality. Recommendation: Approve with minor fixes (primarily the API contract update). |
- Replace custom error response schemas with commonGetErrorResponses - Use createApiError to throw proper Problem Details format errors - Include invocationId and validationErrors as extensions - Update tests to use 'detail' field instead of 'error' for message assertions - Update OpenAPI snapshot to reflect the new error response schema This ensures the webhook API contract is consistent with the rest of the API and follows the application/problem+json specification.
Summary
This PR adds tracking for trigger invocations that fail validation, storing error details for debugging and monitoring purposes.
Changes
Schema
'rejected'status toTriggerInvocationStatusEnumerrorCodecolumn (integer, nullable) to store HTTP status coderespondedAtcolumn (timestamp, nullable) to track when response was sentBackend
processWebhookto create rejected invocations at each validation failure point:invocationIdin error responsessafeParseJsonhelper for graceful JSON parsingUI
respondedAt - createdAt)Tests
Added 8 new tests covering rejected invocation tracking for all failure scenarios.
Migration
Migration
0010_eminent_ronan.sqladds the new columns to thetrigger_invocationstable.