-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix: expose all database event properties in workflow triggers #16722
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
Fixes twentyhq#16716 Previously, database event triggers only exposed properties.after fields. This fix exposes all relevant properties based on event type (before, after, diff, updatedFields) for complete workflow context.
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
| return { | ||
| ...baseFields, | ||
| ...(afterRecord.fields as BaseOutputSchema), | ||
| }; |
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.
Bug: The generateFakeObjectRecordEvent function's return structure changed, but the frontend was not updated. This will cause runtime errors when the frontend tries to access now-missing properties like object and fields.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The backend function generateFakeObjectRecordEvent changed its return type from a nested structure (RecordOutputSchema) to a flat structure (BaseOutputSchema). The frontend, particularly the workflow builder, still expects the old structure, which included object, fields, and _outputSchemaType properties. The frontend type guard isRecordOutputSchemaV2 will fail because _outputSchemaType is missing. Subsequent code that attempts to access recordSchema.object.label or recordSchema.fields will throw runtime errors, breaking the display and parsing of database event trigger variables in workflows.
💡 Suggested Fix
Update the generateFakeObjectRecordEvent function to return the nested RecordOutputSchemaV2 structure that the frontend expects. This involves wrapping the fields in a fields property and adding the object and _outputSchemaType: 'RECORD' properties to the returned object.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/modules/workflow/workflow-builder/workflow-schema/utils/generate-fake-object-record-event.ts#L62-L65
Potential issue: The backend function `generateFakeObjectRecordEvent` changed its return
type from a nested structure (`RecordOutputSchema`) to a flat structure
(`BaseOutputSchema`). The frontend, particularly the workflow builder, still expects the
old structure, which included `object`, `fields`, and `_outputSchemaType` properties.
The frontend type guard `isRecordOutputSchemaV2` will fail because `_outputSchemaType`
is missing. Subsequent code that attempts to access `recordSchema.object.label` or
`recordSchema.fields` will throw runtime errors, breaking the display and parsing of
database event trigger variables in workflows.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7774484
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.
No issues found across 2 files
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:32786 This environment will automatically shut down when the PR is closed or after 5 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.
Hey @alexanderkochux,
Thank you so much for your contribution and sorry for the time it took to review it.
I'll close this PR because, after testing, I'm not sure the change you made fixes the raised issue.
Please don't hesitate to contact me on Discord or comment if you have any questions or if you want to brainstorm an implementation strategy together. Thanks 🙏
Fixes #16716
Previously, database event triggers only exposed properties.after fields. This fix exposes all relevant properties based on event type (before, after, diff, updatedFields) for complete workflow context.