Skip to content

fix: implement review participation metrics and variables#360

Open
zishanq7861 wants to merge 5 commits into
Priyanshu-byte-coder:mainfrom
zishanq7861:feat-pr-reviews-1
Open

fix: implement review participation metrics and variables#360
zishanq7861 wants to merge 5 commits into
Priyanshu-byte-coder:mainfrom
zishanq7861:feat-pr-reviews-1

Conversation

@zishanq7861
Copy link
Copy Markdown

Summary

Brief description of what this PR does.

Closes #

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Refactor / code cleanup

Changes Made

  • ...
  • ...

How to Test

Steps for the reviewer to verify this works:

  1. ...
  2. ...

Screenshots (if UI change)

Checklist

  • Linked issue in summary
  • npm run lint passes locally
  • No TypeScript errors (npm run type-check)
  • Self-reviewed the diff
  • Added/updated tests if applicable

@vercel
Copy link
Copy Markdown

vercel Bot commented May 19, 2026

@zishanq7861 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.

@Priyanshu-byte-coder
Copy link
Copy Markdown
Owner

Two issues to fix before merge:

1. Multi-account path uses wrong username

const username = req.nextUrl.searchParams.get("username") || session.githubLogin;
// ...
accounts.map((account) => fetchPRMetrics(account.token, username))

In the multi-account flow, every account fetches using the same username (from query param or primary session login). Each account should use its own GitHub login. Pass the login through the account object:

accounts.map((account) => fetchPRMetrics(account.token, account.githubLogin))

2. PRMetrics.tsx not updated

The API now returns reviewsGiven and reviewRatio but PRMetrics.tsx doesn't display them. Add stat cards for both fields (similar to how avgReviewHours and mergeRate are shown).

Fix these two and merge.

Copy link
Copy Markdown
Owner

@Priyanshu-byte-coder Priyanshu-byte-coder left a comment

Choose a reason for hiding this comment

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

Two issues:

1. ?username= param allows querying other users' data

const username = req.nextUrl.searchParams.get("username") || session.githubLogin;

Any authenticated user can request /api/metrics/prs?username=someone-else and get that user's review stats. Either remove the param (always use session.githubLogin) or validate it matches the session login:

const requestedUsername = req.nextUrl.searchParams.get("username");
const username = requestedUsername === session.githubLogin ? requestedUsername : session.githubLogin;

2. Conflicts with merged PR #311
PR #311 (Redis caching) was just merged and modifies prs/route.ts. This PR will now conflict. Rebase on main and resolve conflicts with the caching additions from #311.

The reviewed-by:${githubLogin} search and multi-account account.githubLogin usage are correct. Fix the two issues above and it's ready.

Copy link
Copy Markdown
Owner

@Priyanshu-byte-coder Priyanshu-byte-coder left a comment

Choose a reason for hiding this comment

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

1. Cache removal regression — strips withMetricsCache entirely. Every load now hits the GitHub API with no caching. Restore it.

2. reviewsGiven metric semantics are offreviewed-by: returns the all-time total, not last 30 days. reviewRatio divides by the user's PR total making it incoherent. Scope the query to last 30 days or label it clearly.

3. username query param is unnecessary — the fallback requestedUsername === session.githubLogin ? requestedUsername : session.githubLogin always resolves to session.githubLogin. Just use session.githubLogin directly. Remove the param.

4. Missing EOF newlines on both changed files.

5. Accessibility regression — loading skeleton role="status", aria-live, aria-busy, sr-only text all removed. Restore them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants