feat: implement PR velocity metric in trend analyzer#473
Open
ANU5565 wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds PR velocity metrics: new ChangesPR Velocity Metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/analyzer/trends_test.go`:
- Around line 7-17: The test in trends_test.go is a placeholder and never
exercises the PR velocity logic; replace the dummy assertions with a real unit
test that sets up sample PR/commit data, invokes CalculatePRVelocity (or the
higher-level AnalyzeTrends wrapper) and asserts the expected monthly PR velocity
outputs; locate the test function TestMonthlyMetricCalculation and: create
deterministic input fixtures for PR open/close/merge times, call
CalculatePRVelocity (or AnalyzeTrends) with those fixtures, and assert the
returned metrics (counts/rates) against expected values so the test fails on
regressions.
In `@internal/analyzer/trends.go`:
- Line 67: TrendMetrics.PRVelocity is declared but never set by AnalyzeTrends,
so consumers get 0; update AnalyzeTrends (and any helper e.g.,
computeTrendMetrics or accumulatePRStats) to calculate and assign PRVelocity on
the returned TrendMetrics struct by computing the desired velocity metric from
PR-related data (for example average PRs per period or slope over the window)
using the same source the function uses for PR counts; ensure you set
TrendMetrics.PRVelocity before returning and update any code paths that build
TrendMetrics in lines ~148-157 to also populate PRVelocity consistently.
- Around line 154-156: The loop currently sets every
metrics.MonthlyData[i].PRVelocity to CalculatePRVelocity(prs) using the full PR
list; instead, for each metrics.MonthlyData entry call CalculatePRVelocity with
only the PRs belonging to that month (filter prs by the MonthlyData[i] date
range or month identifier) and assign that result to
metrics.MonthlyData[i].PRVelocity so each month gets a month-specific velocity;
update the loop around metrics.MonthlyData and use the filtered slice when
invoking CalculatePRVelocity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b774672-416f-4996-900a-092afedc48bc
📒 Files selected for processing (2)
internal/analyzer/trends.gointernal/analyzer/trends_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/analyzer/trends.go (1)
67-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTop-level
TrendMetrics.PRVelocityis still never populated.The struct field was added but
AnalyzeTrendsdoes not assign it anywhere (lines 96-178), so consumers always receive the zero value even when monthly PR velocity data exists.🔧 Suggested fix
Add this assignment after line 152 (after PR trend analysis):
// Analyze PR trends metrics.PRTrend, metrics.PRMergeRate = AnalyzePRTrends(prs, metrics.MonthlyData) + metrics.PRVelocity = CalculatePRVelocity(prs) for _, m := range metrics.MonthlyData {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/analyzer/trends.go` at line 67, TrendMetrics.PRVelocity was added but never set in AnalyzeTrends; update AnalyzeTrends (the function that computes trends and calls the PR trend analysis) to assign the computed PR velocity value into the TrendMetrics.PRVelocity field after the PR trend analysis completes (i.e., once you have the monthly PR velocity value from the PR trend computation), ensuring the code that constructs or returns TrendMetrics includes PRVelocity so consumers receive the populated value.
🧹 Nitpick comments (1)
internal/analyzer/trends_test.go (1)
8-23: ⚡ Quick winConsider adding test cases for edge conditions.
The current test covers the basic happy path with a single merged PR. Adding test cases for the following scenarios would improve confidence in the implementation:
- Multiple PRs to verify averaging logic
- Unmerged PRs (nil
MergedAt) to confirm they're excluded- Empty PR slice to verify zero return
- PRs where
CreatedAt >= MergedAtto confirm exclusion📋 Example additional test cases
func TestCalculatePRVelocity_MultiplePRs(t *testing.T) { now := time.Now() prs := []github.PullRequest{ {CreatedAt: now.Add(-24 * time.Hour), MergedAt: &now}, // 24h {CreatedAt: now.Add(-48 * time.Hour), MergedAt: &now}, // 48h } velocity := CalculatePRVelocity(prs) expected := 36.0 // (24 + 48) / 2 if velocity < expected-0.01 || velocity > expected+0.01 { t.Errorf("Expected ~36.0 hours, got %f", velocity) } } func TestCalculatePRVelocity_UnmergedPRs(t *testing.T) { prs := []github.PullRequest{ {CreatedAt: time.Now(), MergedAt: nil}, // unmerged } velocity := CalculatePRVelocity(prs) if velocity != 0 { t.Errorf("Expected 0 for unmerged PRs, got %f", velocity) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/analyzer/trends_test.go` around lines 8 - 23, Add edge-case unit tests for CalculatePRVelocity: (1) a Multiple PRs test that supplies several merged PRs (use differing CreatedAt/MergedAt) and asserts the average hours (allow small float tolerance); (2) an Unmerged PRs test that includes PRs with nil MergedAt and asserts the result is 0; (3) an Empty slice test asserting 0 is returned for no PRs; and (4) a CreatedAfterMerged test that provides PRs where CreatedAt >= MergedAt and asserts those PRs are excluded (result behaves like excluding them). Reference the CalculatePRVelocity function and use github.PullRequest structs with controlled time values in internal/analyzer/trends_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/analyzer/trends_test.go`:
- Around line 20-22: The test in trends_test.go checks float equality with "if
velocity != 24.0" which is fragile; change it to a tolerance-based comparison
using an epsilon (e.g. 1e-9) and fail the test when math.Abs(velocity - 24.0) >
epsilon. Import math if needed and update the error message in the failing
branch to show the expected value, actual value, and the used epsilon for
clarity.
---
Duplicate comments:
In `@internal/analyzer/trends.go`:
- Line 67: TrendMetrics.PRVelocity was added but never set in AnalyzeTrends;
update AnalyzeTrends (the function that computes trends and calls the PR trend
analysis) to assign the computed PR velocity value into the
TrendMetrics.PRVelocity field after the PR trend analysis completes (i.e., once
you have the monthly PR velocity value from the PR trend computation), ensuring
the code that constructs or returns TrendMetrics includes PRVelocity so
consumers receive the populated value.
---
Nitpick comments:
In `@internal/analyzer/trends_test.go`:
- Around line 8-23: Add edge-case unit tests for CalculatePRVelocity: (1) a
Multiple PRs test that supplies several merged PRs (use differing
CreatedAt/MergedAt) and asserts the average hours (allow small float tolerance);
(2) an Unmerged PRs test that includes PRs with nil MergedAt and asserts the
result is 0; (3) an Empty slice test asserting 0 is returned for no PRs; and (4)
a CreatedAfterMerged test that provides PRs where CreatedAt >= MergedAt and
asserts those PRs are excluded (result behaves like excluding them). Reference
the CalculatePRVelocity function and use github.PullRequest structs with
controlled time values in internal/analyzer/trends_test.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ffe9ada-b024-4787-bc58-29658ebf046a
📒 Files selected for processing (2)
internal/analyzer/trends.gointernal/analyzer/trends_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR completes Phase 1 of the Trend Analysis feature.
Added PRVelocity field to MonthlyMetric struct.
Implemented CalculatePRVelocity helper function in trends.go.
Integrated PR velocity calculation into the AnalyzeTrends pipeline.
Added unit test in trends_test.go.
Summary by CodeRabbit
New Features
Tests