Refactor: Modularize scraper pipeline into GitHubService and ScoringEngine#246
Refactor: Modularize scraper pipeline into GitHubService and ScoringEngine#246kunal-10-cloud wants to merge 7 commits into
Conversation
✅ Deploy Preview for cv-community-dashboard ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis PR modularizes the leaderboard pipeline: it extracts types to scripts/types.ts, moves GitHub API interaction into scripts/services/github.service.ts, and centralizes scoring logic into scripts/services/scoring.service.ts. scripts/generateLeaderboard.ts is refactored to orchestrate via those services and no longer declares in-file types. lib/db.ts was refactored to group recent activities by type and the public ActivityGroup type now includes an optional activity_description field. components/home-dashboard.tsx import path for RepoStats was updated. Unit tests for GitHubService and ScoringEngine were added. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generateLeaderboard.ts (1)
204-222: Implement bounded pagination to avoid fetching full closed-PR history.The
fetchPRsMerge()function usesgithub.fetchAll()to fetch all closed PRs for each repo, causing unnecessary API calls and potential rate-limit/timeout issues on large repositories. The codebase already uses bounded pagination infetchPRsOpened()(lines 191–202); apply the same pattern here: paginate manually and exit onceupdated_atfalls beforeprevious_start.🛠️ Proposed fix (bounded pagination)
async function fetchPRsMerge(repo: string, current_start: Date, previous_start: Date, now: Date) { console.log(" 🔎 Fetching PRs merged..."); - const prs = (await github.fetchAll( - `${GITHUB_API}/repos/${ORG}/${repo}/pulls?state=closed&sort=updated&direction=desc` - )) as any[]; - let current = 0; let previous = 0; + let page = 1; + + while (true) { + const res = await github.get( + `${GITHUB_API}/repos/${ORG}/${repo}/pulls?state=closed&sort=updated&direction=desc&per_page=100&page=${page}` + ); + if (!res.ok) break; + const prs = (await res.json()) as any[]; + if (!prs.length) break; - for (const pr of prs) { - if (!pr.merged_at) continue; - if (isBotUser(pr.user)) continue; - const mergedAt = new Date(pr.merged_at); - if (mergedAt >= current_start && mergedAt <= now) current++; - if (mergedAt >= previous_start && mergedAt < current_start) previous++; - } + for (const pr of prs) { + if (!pr.merged_at) continue; + if (isBotUser(pr.user)) continue; + const mergedAt = new Date(pr.merged_at); + if (mergedAt >= current_start && mergedAt <= now) current++; + if (mergedAt >= previous_start && mergedAt < current_start) previous++; + } + + const lastUpdated = prs[prs.length - 1]?.updated_at; + if (lastUpdated && new Date(lastUpdated) < previous_start) break; + if (prs.length < 100) break; + page++; + } return { current, previous }; }
🤖 Fix all issues with AI agents
In `@lib/db.ts`:
- Around line 117-145: Remove the dev-inline comments and the redundant type
cast: delete the checkmark comments (e.g., "// ✅ REAL title", "// ✅ REAL GitHub
link", "// ✅ Extract repo name") inside the activities object pushed to groups,
and simplify the contributor_role assignment by removing "?? null as string |
null" and using user.role directly; keep all other fields and calls (e.g.,
extractRepoFromUrl, slug generation, groups, activities) unchanged.
In `@scripts/services/scoring.service.ts`:
- Around line 89-98: The deduplication key in deduplicateAndRecalculate uses
`${act.type}:${act.occured_at}:${act.link ?? act.title}` which collapses
different activities when both act.link and act.title are null; update the key
construction in the user.raw_activities filter to use a more robust fallback
(e.g., include an explicit unique field if available such as act.id or act.repo,
or append a stable hash/JSON.stringify of the activity when link/title are
missing) or skip deduplication for activities lacking identifying info; ensure
changes reference the deduplicateAndRecalculate function and the
user.raw_activities filter so the key becomes something like
`${act.type}:${act.occured_at}:${act.link ?? act.title ?? act.id ??
JSON.stringify(act)}` or an equivalent safe fallback.
🧹 Nitpick comments (1)
scripts/services/github.service.ts (1)
89-103: fetchOrgRepos silently continues on error, which may return partial results.When a fetch fails mid-pagination (e.g., page 3 of 5), the method logs a warning but returns whatever repos were collected so far. This could lead to incomplete data without clear indication to the caller.
Consider either:
- Throwing an error on failure (consistent with
fetchAll)- Returning a result object that indicates partial success
This is acceptable for now if the caller handles potential partial data, but worth noting for reliability.
482d5eb to
bb012d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/services/github.service.test.ts`:
- Around line 4-16: The test currently assigns global.fetch = vi.fn() at module
scope which isn't restored by vi.restoreAllMocks(); change this to create a spy
in the test lifecycle: remove the module-scope assignment and instead call
vi.spyOn(globalThis, 'fetch') inside beforeEach (and set its
mockImplementation/resolution there as needed), keep vi.clearAllMocks() in
beforeEach and vi.restoreAllMocks() in afterEach so the original global.fetch is
restored between tests; update any references to the mock to use the spy
returned by vi.spyOn.
🧹 Nitpick comments (2)
scripts/services/github.service.ts (1)
224-257: Guard against non‑positivestepDaysto avoid an infinite loop.
IfstepDaysis 0 or negative,cursornever advances and the loop won’t terminate.♻️ Proposed defensive guard
async searchByDateChunks( baseQuery: string, start: Date, end: Date, stepDays = 30, dateField = "created" ): Promise<GitHubSearchItem[]> { + if (stepDays <= 0) { + throw new Error("stepDays must be > 0"); + } const all: GitHubSearchItem[] = []; let cursor = new Date(start);scripts/generateLeaderboard.ts (1)
110-115: UseisBotUserto avoid duplicated bot filtering logic.This keeps bot detection consistent across the file and reduces drift if the rules change.
♻️ Suggested refactor
- if (!review.user?.login) continue; - if (review.user.login.endsWith("[bot]")) continue; - if (review.user.type && review.user.type !== "User") continue; + if (isBotUser(review.user)) continue;
Atharva7126
left a comment
Atharva7126
left a comment
There was a problem hiding this comment.
hey @kunal-10-cloud you need to revert all the json files
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/analytics/analytics.json (1)
743-750: Fix repository field for mobile-app issues.Line 743-750, 977-984, and 1013-1021 list mobile-app issue URLs but set
"repository": "CircuitVerseDocs". This misclassifies analytics and per-repo filters. Update these to"mobile-app"and fix the mapping in the generator.🛠 Proposed fix
- "url": "https://github.com/CircuitVerse/mobile-app/issues/484", - "repository": "CircuitVerseDocs", + "url": "https://github.com/CircuitVerse/mobile-app/issues/484", + "repository": "mobile-app",- "url": "https://github.com/CircuitVerse/mobile-app/issues/482", - "repository": "CircuitVerseDocs", + "url": "https://github.com/CircuitVerse/mobile-app/issues/482", + "repository": "mobile-app",- "url": "https://github.com/CircuitVerse/mobile-app/issues/481", - "repository": "CircuitVerseDocs", + "url": "https://github.com/CircuitVerse/mobile-app/issues/481", + "repository": "mobile-app",Also applies to: 977-984, 1013-1021
🤖 Fix all issues with AI agents
In `@public/leaderboard/recent-activities.json`:
- Around line 4-6: The array of recent-activities date groups in
recent-activities.json is not in strict chronological order (e.g., "2026-01-26"
appears before a later "2026-01-27" group); before writing this file ensure the
top-level array of date-group tuples is sorted in descending (or the
UI-expected) chronological order by the date string key so all date buckets
(e.g., "2026-01-26", "2026-01-27") are contiguous and correctly ordered.
- Changed from module-scope global.fetch assignment to vi.spyOn in beforeEach - Ensures proper cleanup with vi.restoreAllMocks in afterEach - Addresses CodeRabbit review feedback
…Rabbit suggestion
dd824c1 to
589930a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/db.ts (2)
50-96: Remove commented-out code before merging.This large block of commented-out code (the old implementation) should be removed. It creates noise and the git history preserves the original if needed.
98-100: Add derivePeriod call for the 2-month period or remove "2month" from the union type.The function accepts
"2month"as valid input, andpublic/leaderboard/2month.jsonexists. However, the file is not regenerated bygenerateLeaderboard.tssincederivePeriod()is only called forweekandmonth. This means the JSON file will contain stale data over time as activities accumulate.Either add
derivePeriod(yearData, 60, "2month");to keep the 2-month period current, or remove"2month"from the valid union type if it's not intended to be actively maintained.scripts/generateLeaderboard.ts (1)
604-608: AddawaitbeforegenerateRepoOverview()and consider generating2month.json.Two observations:
generateRepoOverview()is an async function but is called withoutawait. While the outercatchwill handle rejection, any errors fromgenerateRepoOverviewwon't interrupt the flow or provide proper sequencing.
lib/db.tsaccepts"2month"as a valid period parameter, but no2month.jsonis generated here. Add aderivePeriodcall for 60 days if this period is needed.🔧 Suggested fix
derivePeriod(yearData, 7, "week"); derivePeriod(yearData, 30, "month"); + derivePeriod(yearData, 60, "2month"); generateRecentActivities(yearData); - await generateRepoOverview() + await generateRepoOverview(); }
🧹 Nitpick comments (4)
scripts/types.ts (2)
67-87: Minor style inconsistency: missing semicolons in nested object types.The nested object types within
RepoStatsare missing semicolons after their closing braces, while other interfaces in this file use semicolons consistently.🔧 Suggested fix
export interface RepoStats { name: string; description: string | null; language: string | null; avatar_url: string; html_url: string; stars: number; forks: number; current: { pr_opened: number; pr_merged: number; issue_created: number; currentTotalContribution: number; - } + }; previous: { pr_merged: number; - } + }; growth: { pr_merged: number; - }; -}; + }; +}
89-105: Consider reusing theContributortype instead of inline definition.The
entriesarray inExistingYearDatahas an inline type definition that nearly duplicates theContributortype. Consider referencingContributordirectly to maintain DRY principles and ensure consistency.♻️ Suggested refactor
export interface ExistingYearData { period: string; updatedAt: number; lastFetchedAt?: number; startDate?: string; endDate?: string; - entries: Array<{ - username: string; - name: string | null; - avatar_url: string | null; - role: string; - total_points: number; - activity_breakdown: Record<string, { count: number; points: number }>; - daily_activity: Array<{ date: string; count: number; points: number }>; - raw_activities: RawActivity[]; - }>; + entries: Contributor[]; }scripts/generateLeaderboard.ts (2)
163-172: Consider adding proper TypeScript types for API responses.Multiple
as any[]type assertions reduce type safety. Consider defining interfaces for GitHub API responses (issues, PRs, etc.) to catch potential issues at compile time.💡 Example type definition
interface GitHubIssue { id: number; pull_request?: object; created_at: string; user: { login: string; type?: string } | null; // ... other fields } // Then use: const issues = await github.fetchAll<GitHubIssue>(...);
534-541: Non-null assertion onpr.closed_atis safe but could use a guard.The search query filters for merged PRs which should always have
closed_at, but a defensive check would prevent potential runtime errors if the API behavior changes.🛡️ Defensive check
for (const pr of await github.searchByDateChunks( `org:${ORG}+is:pr+is:merged`, since, now, 30, "merged" )) { if (isBotUser(pr.user)) continue; + if (!pr.closed_at) continue; ScoringEngine.addActivity( ScoringEngine.ensureUser(users, pr.user), "PR merged", - pr.closed_at!, + pr.closed_at, POINTS["PR merged"], { title: pr.title, link: pr.html_url } ); }
- Remove commented-out code in lib/db.ts - Fix style inconsistencies in scripts/types.ts (missing semicolons) - Reuse Contributor type in scripts/types.ts for DRY - Add await to generateRepoOverview() call - Add derivation for 2-month period in generateLeaderboard.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/db.ts`:
- Around line 65-90: getRecentActivitiesGroupedByType currently only iterates
user.activities so records coming from year.json (which use raw_activities on
Contributor objects) are ignored when valid="year"; update the function (or the
loop processing entries) to check for and prefer user.activities but fall back
to user.raw_activities when activities is undefined, and treat both arrays with
the same shape when building groups (use the same act variable handling and
extractRepoFromUrl). Alternatively, if you intend to drop year support, remove
"year" from the valid union where getRecentActivitiesGroupedByType is
called/validated so callers cannot pass valid="year". Ensure you reference
getRecentActivitiesGroupedByType and the loop that builds groups (places using
user.activities and act) when making the change.
🧹 Nitpick comments (1)
scripts/generateLeaderboard.ts (1)
102-106: Isolate PR review fetch failures per PR.A single review request failure will currently throw and abort the batch. Consider catching errors per PR and continuing with an empty review list so one bad response doesn’t stop the entire repo scan.
Suggested fix
- const reviewResults = await Promise.all( - batch.map(pr => github.fetchPRReviews(ORG, repoName, pr.number).then(reviews => ({ pr, reviews: reviews as GitHubReview[] }))) - ); + const reviewResults = await Promise.all( + batch.map(async pr => { + try { + const reviews = await github.fetchPRReviews(ORG, repoName, pr.number); + return { pr, reviews: reviews as GitHubReview[] }; + } catch (err) { + console.warn(` ⚠️ Failed to fetch reviews for PR #${pr.number}`, err); + return { pr, reviews: [] as GitHubReview[] }; + } + }) + );
…dByType - Resolves CodeRabbit review regarding year.json records being ignored - Ensures backward compatibility with older data files using 'activities' - Fixed unused UserEntry import warning in lib/db.ts
naman79820
left a comment
naman79820
left a comment
There was a problem hiding this comment.
Hey @kunal-10-cloud thanks for the effort on this However the current code is stable, working well in production and we're not actively developing this area anymore. Since everything is functioning fine and there are no planned changes to this part of the codebase so i think there's no need for it. Once again thanksss for the efforts really appreciated :))
3 participants
Description
This PR refactors the monolithic generateLeaderboard.ts script into modular GitHubService and ScoringEngine components to improve maintainability. It also introduces centralized type definitions and comprehensive unit tests (using Vitest) to ensure stability, while preserving all original logic and variable names.
Related Issue
Fixes #245
Type of change
Checklist
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.