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 dynamic dates for webhookGraphDataUsage #7720

Merged
merged 8 commits into from
Oct 18, 2024
Merged

Conversation

anamarn
Copy link
Contributor

@anamarn anamarn commented Oct 15, 2024

Before:
Only last 5 days where displayed on Developers Settings Webhook Usage Graph.
image

Now
Added component where you can select the time range where you want to view the webhook usage. To do better the styling and content depassing .

Screenshot 2024-10-15 at 16 56 45

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.
  8. Select the desired time range in the dropdown

To do list

  • Tooltip is truncated when accessing values at the right end of the graph
  • DateTicks needs to follow a more clear standard
  • Update this PR with more representative images

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 dynamic date selection for the webhook usage graph in the Developers Settings, enhancing the visualization of webhook activity over time.

  • Introduced SettingsDevelopersWebhookUsageGraph component with a time range selector for 7 days, 1 day, 12 hours, and 4 hours views
  • Added WEBHOOK_GRAPH_API_OPTIONS_MAP in /constants/WebhookGraphApiOptionsMap.ts to define time intervals and tick sizes for different ranges
  • Created fetchGraphData function in /utils/fetchGraphData.ts to retrieve and transform webhook usage data for selected time ranges
  • Implemented SettingsDevelopersWebhookTooltip component in /components/SettingsDeveloperSliceTooltip.tsx for displaying detailed information on data points
  • Updated SettingsDevelopersWebhookDetail component to integrate new graph features and time range selection

7 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 60 to 61
point,
}: any): ReactElement => {
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 defining a proper type for the 'point' prop instead of using 'any'

Copy link
Member

Choose a reason for hiding this comment

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

I agree with greptile, you should shouldn't use any here :)

return (
<StyledTooltipContainer>
<StyledTooltipDateContainer>
{format(newDate, 'MMM d · HH:mm')}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Use a consistent date format across the application. Consider using a constant for the date format string

Copy link
Member

Choose a reason for hiding this comment

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

Agree with Greptile again, maybe rely on existing formatting options? or add a new helper similar to formatToHumanReadableTime, etc.

@@ -1,8 +1,14 @@
import { SettingsDevelopersWebhookTooltip } from '@/settings/developers/webhook/components/SettingsDevelopersliceTooltip';
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: SettingsDevelopersliceTooltip is misspelled

Copy link
Member

Choose a reason for hiding this comment

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

Agree with greptile, the filename should match the component

});
return;
}
// try {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Remove commented-out code before merging

// });
// }
// };
// fetchData();
}, [enqueueSnackBar, setWebhookGraphData, webhookId]);
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Ensure fetchGraphData is included in the dependency array if it's defined outside this component

@@ -0,0 +1,6 @@
export const WEBHOOK_GRAPH_API_OPTIONS_MAP = {
'7D': { startIntervalHours: '168', tickSizeMinutes: '420' },
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: 420 minutes is 7 hours, which seems like an unusual tick size for a 7-day view

@@ -0,0 +1,8 @@
import { createState } from 'twenty-ui';

export type WebhookGraphOptions = '7D' | '1D' | '12H' | '4H';
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 adding a more descriptive type name, such as 'WebhookGraphTimeRange'

}
// Next steps: separate the map logic to a different component (response.data, {id:str, color:str}[])=>NivoLineInput[]

const graphInput = result.data
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Complex data transformation. Consider breaking this into smaller, more manageable functions

@@ -0,0 +1,6 @@
export const WEBHOOK_GRAPH_API_OPTIONS_MAP = {
'7D': { startIntervalHours: '168', tickSizeMinutes: '420' },
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could rename startIntervalHours? It wasn't very clear for me
windowInHours ; tickIntervalInMinutes ?


export const fetchGraphData = async ({
webhookId,
enqueueSnackBar,
Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit strange to pass enqueueSnackBar to a util. Maybe just throw errors here and put your try catch at a high level? You can create a dedicated hook probably?

import { WEBHOOK_GRAPH_API_OPTIONS_MAP } from '@/settings/developers/webhook/constants/WebhookGraphAPIOptionsMap';
import { SnackBarVariant } from '@/ui/feedback/snack-bar-manager/components/SnackBar';

type fetchGraphDataProps = {
Copy link
Member

Choose a reason for hiding this comment

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

feels a bit strange to have webhookId and webhookOptions = 7D... next to another. WebhookOptions feels very generic so I would have expected an object.
Maybe you can just rename that param to windowLength or something like that?

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Great!

@anamarn anamarn mentioned this pull request Oct 18, 2024
Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Great!

@FelixMalfait FelixMalfait merged commit 8cadcdf into main Oct 18, 2024
13 checks passed
@FelixMalfait FelixMalfait deleted the feat/webhooks-graph-v2 branch October 18, 2024 09:00
Copy link

Fails
🚫

node failed.

Log

�[31mError: �[39m RequestError [HttpError]: You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 7FC7:1484C5:640BB:C349F:671223D6.
    at /home/runner/work/twenty/twenty/node_modules/�[4m@octokit�[24m/request/dist-node/index.js:86:21
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m {
  status: �[33m403�[39m,
  response: {
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Aanamarn%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    status: �[33m403�[39m,
    headers: {
      �[32m'access-control-allow-origin'�[39m: �[32m'*'�[39m,
      �[32m'access-control-expose-headers'�[39m: �[32m'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'�[39m,
      connection: �[32m'close'�[39m,
      �[32m'content-encoding'�[39m: �[32m'gzip'�[39m,
      �[32m'content-security-policy'�[39m: �[32m"default-src 'none'"�[39m,
      �[32m'content-type'�[39m: �[32m'application/json; charset=utf-8'�[39m,
      date: �[32m'Fri, 18 Oct 2024 09:01:10 GMT'�[39m,
      �[32m'referrer-policy'�[39m: �[32m'origin-when-cross-origin, strict-origin-when-cross-origin'�[39m,
      server: �[32m'github.com'�[39m,
      �[32m'strict-transport-security'�[39m: �[32m'max-age=31536000; includeSubdomains; preload'�[39m,
      �[32m'transfer-encoding'�[39m: �[32m'chunked'�[39m,
      vary: �[32m'Accept-Encoding, Accept, X-Requested-With'�[39m,
      �[32m'x-content-type-options'�[39m: �[32m'nosniff'�[39m,
      �[32m'x-frame-options'�[39m: �[32m'deny'�[39m,
      �[32m'x-github-api-version-selected'�[39m: �[32m'2022-11-28'�[39m,
      �[32m'x-github-media-type'�[39m: �[32m'github.v3; format=json'�[39m,
      �[32m'x-github-request-id'�[39m: �[32m'7FC7:1484C5:640BB:C349F:671223D6'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1729242130'�[39m,
      �[32m'x-ratelimit-resource'�[39m: �[32m'search'�[39m,
      �[32m'x-ratelimit-used'�[39m: �[32m'1'�[39m,
      �[32m'x-xss-protection'�[39m: �[32m'0'�[39m
    },
    data: {
      documentation_url: �[32m'https://docs.github.com/free-pro-team@latest/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits'�[39m,
      message: �[32m'You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 7FC7:1484C5:640BB:C349F:671223D6.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Aanamarn%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    headers: {
      accept: �[32m'application/vnd.github.v3+json'�[39m,
      �[32m'user-agent'�[39m: �[32m'octokit-rest.js/18.12.0 octokit-core.js/3.6.0 Node.js/18.20.4 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-6b76929e.json

Generated by 🚫 dangerJS against e4eb06c

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