-
Notifications
You must be signed in to change notification settings - Fork 99
feat: add maintainer day-over-day stat deltas #522
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,11 @@ import { profiles, xpEvents, pullRequests } from '@/lib/db/schema'; | |
| import { eq, inArray, sum, desc, and, count } from 'drizzle-orm'; | ||
| import { cacheGet, cacheSet } from '@/lib/cache'; | ||
| import type { MaintainerAnalyticsTrends } from '@/lib/maintainer/analytics'; | ||
| import { | ||
| buildDayOverDayStats, | ||
| buildMaintainerAnalyticsTrends, | ||
| emptyMaintainerDayOverDayStats, | ||
| } from '@/lib/maintainer/analytics'; | ||
| import { | ||
| comparePrRows, | ||
| validateFilters, | ||
|
|
@@ -182,57 +187,91 @@ export async function getMaintainerAnalyticsTrends(args: { | |
|
|
||
| const repos = await listMaintainerRepos(user.id, args.installationId); | ||
| if (repos.length === 0) { | ||
| return ok({ weekly: [], levelDistribution: [], avgReviewTimeHours: null }); | ||
| return ok({ | ||
| weekly: [], | ||
| levelDistribution: [], | ||
| avgReviewTimeHours: null, | ||
| dayOverDay: emptyMaintainerDayOverDayStats(), | ||
| }); | ||
| } | ||
|
|
||
| const cacheKey = `maint:analytics-trends:${user.id}:${args.installationId}`; | ||
| const cached = await cacheGet<MaintainerAnalyticsTrends>(cacheKey); | ||
| if (cached) return ok(cached); | ||
| if (cached?.dayOverDay) return ok(cached); | ||
|
|
||
| const { data, error } = await service.rpc('maintainer_analytics_trends', { | ||
| repo_names: repos, | ||
| }); | ||
|
|
||
| if (error) return err('query_failed', error.message); | ||
|
|
||
| // Fetch average review time from pull_requests | ||
| const yesterdayStart = new Date(); | ||
| yesterdayStart.setDate(yesterdayStart.getDate() - 1); | ||
|
|
||
| // Fetch review stats and day-over-day deltas from timestamped PR rows. | ||
| const { data: prs } = await service | ||
| .from('pull_requests') | ||
| .select('github_created_at, mentor_review_at') | ||
| .select('github_created_at, merged_at, mentor_review_at') | ||
| .in('repo_full_name', repos) | ||
| .eq('mentor_verified', true) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this filter means
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should i remove the merged for true check or add another similar block without check
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a second query without the filter for opened/merged counts. keep the existing query (with mentor_verified = true) only for calculating review time. removing the filter entirely would break the review-time calculation. |
||
| .not('mentor_review_at', 'is', null); | ||
| .not('github_created_at', 'is', null) | ||
| .gte('github_created_at', yesterdayStart.toISOString()); | ||
|
|
||
| let avgReviewTimeHours = null; | ||
| if (prs && prs.length > 0) { | ||
| const reviewRows = ( | ||
| (prs ?? []) as { | ||
| github_created_at: string; | ||
| merged_at: string | null; | ||
| mentor_review_at: string | null; | ||
| }[] | ||
| ).filter((pr) => pr.mentor_review_at); | ||
|
|
||
| if (reviewRows.length > 0) { | ||
| const totalSeconds = ( | ||
| prs as { github_created_at: string; mentor_review_at: string | null }[] | ||
| reviewRows as { github_created_at: string; mentor_review_at: string | null }[] | ||
| ).reduce((sum: number, pr) => { | ||
| const created = new Date(pr.github_created_at).getTime(); | ||
| const reviewed = new Date(pr.mentor_review_at!).getTime(); | ||
| return sum + (reviewed - created) / 1000; | ||
| }, 0); | ||
| avgReviewTimeHours = totalSeconds / prs.length / 3600; | ||
| avgReviewTimeHours = totalSeconds / reviewRows.length / 3600; | ||
| } | ||
|
|
||
| const trends = normaliseAnalyticsTrends(data, avgReviewTimeHours); | ||
| const dayOverDay = buildDayOverDayStats( | ||
| new Date(), | ||
| ( | ||
| (prs ?? []) as { | ||
| github_created_at: string | null; | ||
| merged_at: string | null; | ||
| mentor_review_at: string | null; | ||
| }[] | ||
| ).map((pr) => ({ | ||
| githubCreatedAt: pr.github_created_at, | ||
| mergedAt: pr.merged_at, | ||
| mentorReviewAt: pr.mentor_review_at, | ||
| })), | ||
| ); | ||
|
|
||
| const trends = normaliseAnalyticsTrends(data, avgReviewTimeHours, dayOverDay); | ||
| await cacheSet(cacheKey, trends, 30 * 60); | ||
| return ok(trends); | ||
| } | ||
|
|
||
| function normaliseAnalyticsTrends( | ||
| value: unknown, | ||
| avgReviewTimeHours: number | null, | ||
| dayOverDay = emptyMaintainerDayOverDayStats(), | ||
| ): MaintainerAnalyticsTrends { | ||
| if (!value || typeof value !== 'object') { | ||
| return { weekly: [], levelDistribution: [], avgReviewTimeHours: null }; | ||
| return { weekly: [], levelDistribution: [], avgReviewTimeHours: null, dayOverDay }; | ||
| } | ||
|
|
||
| const data = value as Partial<MaintainerAnalyticsTrends>; | ||
| return { | ||
| weekly: Array.isArray(data.weekly) ? data.weekly : [], | ||
| levelDistribution: Array.isArray(data.levelDistribution) ? data.levelDistribution : [], | ||
| avgReviewTimeHours, | ||
| dayOverDay, | ||
| }; | ||
| } | ||
|
|
||
|
|
||
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.
setDateuses local server time butbuildDayOverDayStatsuses UTC day boundaries - they can disagree near UTC midnight. useDate.UTC(...)instead to match.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.
ahh i see the dates are conflicting for the time zone ..
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.
@GhanshyamJha05 are u working on this??