-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add webhook response graph from the last 5 days #7487
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.
PR Summary
This pull request adds a webhook response graph feature to the SettingsDevelopersWebhookDetail component, visualizing data from the last 5 days. Key changes include:
- Implemented a new statistics section in SettingsDevelopersWebhookDetail.tsx using Nivo line charts
- Added analytics tracking for webhook responses in call-webhook.job.ts
- Updated dependencies to include @nivo/line for charting functionality
- Integrated AnalyticsModule into the WorkspaceQueryRunnerJobModule
Potential concerns:
- Hardcoded Tinybird token in the frontend (line 93 of SettingsDevelopersWebhookDetail.tsx) poses a security risk
- Performance impact of fetching and processing webhook data on the client-side
- Lack of error handling for the Tinybird API request in the frontend component
5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
const queryString = new URLSearchParams({ | ||
webhookIdRequest: webhookId, | ||
}).toString(); | ||
const token = ''; //put your tinybird token here |
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: Hardcoding an empty string for the token is a security risk. Consider using an environment variable.
); | ||
setData(graphInput); | ||
} catch (error) { | ||
/* empty todo add error to snackbar*/ |
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: Error handling is incomplete. Implement proper error handling and user feedback.
} catch (err) { | ||
const eventInput = { | ||
action: 'webhook.response', | ||
payload: { | ||
status: err.response.status, | ||
url: data.targetUrl, | ||
webhookId: data.webhookId, | ||
eventName: data.eventName, | ||
}, | ||
}; |
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: Consider wrapping err.response.status in a try-catch block, as err.response might be undefined for network errors
@@ -36,8 +36,8 @@ | |||
"@nestjs/serve-static": "^4.0.1", | |||
"@nestjs/terminus": "^9.2.2", | |||
"@nestjs/typeorm": "^10.0.0", | |||
"@nivo/calendar": "^0.84.0", |
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.
I see a usage in ActivityLog.tsx
package.json
Outdated
"@nivo/calendar": "^0.84.0", | ||
"@nivo/core": "^0.84.0", | ||
"@nivo/core": "^0.87.0", | ||
"@nivo/line": "^0.87.0", |
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.
let's remove it from this package.json and move it to twenty-front/package.json (we are relocating all new dependencies to the packages package.json)
@@ -71,6 +84,78 @@ export const SettingsDevelopersWebhooksDetail = () => { | |||
})), | |||
]; | |||
|
|||
useEffect(() => { |
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 don't put useEffects in component as this is a symptom that the component needs to re-render.
Instead, could you introduce a SettingsDevelopersWebhookUsageGraph.tsx component (that will contain your graph), and a SettingsDevelopersWebhookUsageGraphEffect.tsx (that will contain this useEffect).
You will also need to introduce a recoilState to store your data and so that your Graph can communicate with your GraphEffect
); | ||
setData(graphInput); | ||
} catch (error) { | ||
/* empty todo add error to snackbar*/ |
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.
yes please!
...modules/settings/developers/webhook/components/SettingsDevelopersWebhookUsageGraphEffect.tsx
Dismissed
Show dismissed
Hide dismissed
twentyhq#7346 twentyhq#7343 twentyhq#7342 twentyhq#7344 Before: <img width="799" alt="Screenshot 2024-10-08 at 11 59 37" src="https://github.com/user-attachments/assets/a1cd1714-41ed-4f96-85eb-2861e7a8b2c2"> Now: ![Screenshot 2024-10-07 at 18 56 21](https://github.com/user-attachments/assets/c87ee17a-c6c4-4938-b024-aaa635bab022) In order to test: 1. Set ANALYTICS_ENABLED to true 2. Set TINYBIRD_TOKEN to your token from the workspace _twenty_analytics_playground_ 3. Write your client tinybird token in SettingsDeveloppersWebhookDetail.tsx in line 93 4. Create a Webhook in twenty and set wich events it needs to track 5. Run twenty-worker in order to make the webhooks work. 6. Do your tasks in order to populate the data 7. Enter to settings> webhook>your webhook and the statistics section should be displayed.
#7346 #7343 #7342 #7344
Before:
Now:
In order to test: