-
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 dynamic dates for webhookGraphDataUsage #7720
Conversation
packages/twenty-front/src/modules/settings/developers/webhook/utils/fetchGraphData.ts
Fixed
Show fixed
Hide fixed
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 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
point, | ||
}: any): ReactElement => { |
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 defining a proper type for the 'point' prop instead of using 'any'
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 agree with greptile, you should shouldn't use any here :)
return ( | ||
<StyledTooltipContainer> | ||
<StyledTooltipDateContainer> | ||
{format(newDate, 'MMM d · HH:mm')} |
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: Use a consistent date format across the application. Consider using a constant for the date format string
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.
Agree with Greptile again, maybe rely on existing formatting options? or add a new helper similar to formatToHumanReadableTime, etc.
...y-front/src/modules/settings/developers/webhook/components/SettingsDeveloperSliceTooltip.tsx
Outdated
Show resolved
Hide resolved
...y-front/src/modules/settings/developers/webhook/components/SettingsDeveloperSliceTooltip.tsx
Outdated
Show resolved
Hide resolved
@@ -1,8 +1,14 @@ | |||
import { SettingsDevelopersWebhookTooltip } from '@/settings/developers/webhook/components/SettingsDevelopersliceTooltip'; |
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.
syntax: SettingsDevelopersliceTooltip is misspelled
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.
Agree with greptile, the filename should match the component
}); | ||
return; | ||
} | ||
// try { |
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: Remove commented-out code before merging
// }); | ||
// } | ||
// }; | ||
// fetchData(); | ||
}, [enqueueSnackBar, setWebhookGraphData, webhookId]); |
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: 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' }, |
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: 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'; |
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 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 |
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: 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' }, |
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.
Maybe we could rename startIntervalHours? It wasn't very clear for me
windowInHours
; tickIntervalInMinutes
?
|
||
export const fetchGraphData = async ({ | ||
webhookId, | ||
enqueueSnackBar, |
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.
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 = { |
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.
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?
packages/twenty-front/src/modules/settings/developers/webhook/utils/fetchGraphDataOrThrow.ts
Dismissed
Show dismissed
Hide dismissed
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.
Great!
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.
Great!
Log
|
Before:
Only last 5 days where displayed on Developers Settings Webhook Usage Graph.
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 .
In order to test
To do list