Skip to content
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

Merged
merged 7 commits into from
Oct 9, 2024

Conversation

anamarn
Copy link
Contributor

@anamarn anamarn commented Oct 8, 2024

#7346 #7343 #7342 #7344

Before:

Screenshot 2024-10-08 at 11 59 37

Now:

Screenshot 2024-10-07 at 18 56 21

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.

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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
Copy link
Contributor

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*/
Copy link
Contributor

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.

Comment on lines 45 to +54
} catch (err) {
const eventInput = {
action: 'webhook.response',
payload: {
status: err.response.status,
url: data.targetUrl,
webhookId: data.webhookId,
eventName: data.eventName,
},
};
Copy link
Contributor

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",
Copy link
Member

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",
Copy link
Member

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(() => {
Copy link
Member

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*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please!

@charlesBochet charlesBochet merged commit f901512 into main Oct 9, 2024
17 of 18 checks passed
@charlesBochet charlesBochet deleted the feat/webhooks-graphs branch October 9, 2024 13:41
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants