feat: add review cycle time metric with weekly trend chart and slowes…#357
feat: add review cycle time metric with weekly trend chart and slowes…#357YuktiNandwana wants to merge 5 commits into
Conversation
|
@YuktiNandwana is attempting to deploy a commit to the PRIYANSHU DOSHI's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thanks for your first PR on DevTrack! 🎉
A maintainer will review it within 48 hours. While you wait:
- Make sure CI is passing (type-check + lint)
- Double-check the PR description is filled out and the issue is linked
- Feel free to ask questions in Discussions if you need help
|
Hi @Priyanshu-byte-coder! I've resolved the merge conflicts with upstream/main. PR is ready for review |
|
Good idea, but several issues block merge: 1. Reverts the merged PR #197 fix (critical) PR #197 was merged specifically to fix merged vs closed counting. Your PR removes that fix: // Your code (wrong — counts rejected/abandoned PRs as merged):
const merged = data.items.filter((pr) => pr.state === "closed").length;
// Correct (already in main from PR #197):
const merged = data.items.filter(
(pr) => pr.pull_request?.merged_at != null
).length;Restore the 2. Removes combined-account support (regression) Your PR deletes the 3. GraphQL uses
search(query: "type:pr reviewed-by:@me created:>2026-02-18", type: ISSUE, first: 100)Then you don't need the 4. Recharts version bump is out of scope
Fix these four, and the cycle time feature is solid. |
…ccounts and query optimization
|
Hi @Priyanshu-byte-coder! Thanks for the constructive feedback. I have resolved all the noted issues and pushed the fixes: Restored the correct pull_request?.merged_at != null check from PR #197 and optimized avgReviewMs to only track explicitly merged PRs. Re-implemented full multi-account aggregation support inside the accountId === "combined" evaluation block for all the new variables (avgCycleTime, weeklyTrend, slowestRepos). Updated the GraphQL search query to use reviewed-by:@me, making it cleaner and removing the need for post-fetch filters. Reverted the recharts version bump back to ^2.12.7 inside package.json. Everything is fully resolved and ready for your final review and merge! |
Priyanshu-byte-coder
left a comment
There was a problem hiding this comment.
1. Cache removal regression — removes withMetricsCache entirely. Every load now makes two HTTP requests (REST + GraphQL) to GitHub with no caching. Restore the cache wrapper.
2. Wrong mergeRate denominator — uses data.total_count (all-time GitHub total) which gives near-zero rates for active users. The prior code correctly used sampleTotal (items actually fetched). Revert this change.
3. Pervasive any types — GraphQL result typed as any throughout. Add proper interfaces.
4. Broken combined-account path — Promise.all instead of Promise.allSettled means one failed account token fetch crashes the entire response. Use Promise.allSettled.
5. Missing EOF newlines on both changed files.
6. Leading whitespace on import lines — spurious diff noise, remove.
Summary
Added review cycle time metric to PR Analytics — tracks how long PRs sit before getting their first review (DORA metric).
Closes #255
Type of Change
Changes Made
submittedAtfirst_review_at - created_atper PRavgCycleTimestat card in PRMetrics componentHow to Test
Checklist