fix: resolve missing lead time breakdown when review events are absen…#697
fix: resolve missing lead time breakdown when review events are absen…#697THEAbhishekjoshi wants to merge 2 commits into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR passes bot-filtered events into get_pr_performance and introduces first_response_end_time (first_review.created_at or pr.state_changed_at) to compute first_response_time, rework_time, and merge_time, replacing -1 sentinels and setting merge_time to 0 for merged but unreviewed PRs. Tests updated accordingly. ChangesLead Time Breakdown Components
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py (1)
98-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against negative
rework_timefor parity withmerge_time.The
merge_timebranch on line 119 already clamps negatives to-1to prevent garbage when events arrive out of expected order (e.g., approval recorded after merge, clock skew, post-close reviews). The newrework_time = (pr.state_changed_at - first_response_end_time).total_seconds()path on lines 99–101 has no such guard, so afirst_review.created_atlater thanpr.state_changed_atwould yield a negative rework duration that flows through topr.rework_time(line 33–35 only filters-1, not negatives).🛡️ Suggested guard
if not approved_reviews: - rework_time = ( - pr.state_changed_at - first_response_end_time - ).total_seconds() + rework_time = ( + pr.state_changed_at - first_response_end_time + ).total_seconds() + # Prevent garbage state when review is recorded after state change + rework_time = -1 if rework_time < 0 else rework_time🤖 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 `@backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py` around lines 98 - 108, The computed rework_time in etl_code_analytics.py can be negative when using the branch that sets rework_time = (pr.state_changed_at - first_response_end_time).total_seconds(); update that branch to clamp negative values the same way merge_time does (set rework_time to -1 when the computed value is < 0) so pr.rework_time never receives a raw negative duration. Locate the variables/expressions rework_time, pr.state_changed_at, first_response_end_time, first_review and approved_reviews and apply the guard there to match the merge_time behavior.
🧹 Nitpick comments (1)
backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py (1)
131-138: 💤 Low valueDead fallback branch in
first_review_time.
first_response_end_timeis nowfirst_review.created_at if first_review else pr.state_changed_at(lines 92–96). Sincecreate_pr_metricsreturns early forPullRequestState.OPEN(line 24–25),pr.state_changed_atis always set when this code runs, makingfirst_response_end_timeunconditionally truthy. Theif first_response_end_time else -1ternary on lines 136–137 is therefore unreachable and can be simplified.♻️ Suggested simplification
return PRPerformance( first_review_time=( - ( - first_response_end_time - pull_request_ready_for_review_time - ).total_seconds() - if first_response_end_time - else -1 + first_response_end_time - pull_request_ready_for_review_time + ).total_seconds(), - ), rework_time=rework_time,🤖 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 `@backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py` around lines 131 - 138, The fallback branch in PRPerformance.first_review_time is dead because first_response_end_time is always truthy; simplify the expression by removing the ternary and unconditionally computing (first_response_end_time - pull_request_ready_for_review_time).total_seconds() when constructing PRPerformance in create_pr_metrics (use the existing symbols first_response_end_time and pull_request_ready_for_review_time and the PRPerformance constructor) so the unreachable "else -1" branch is eliminated.
🤖 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.
Outside diff comments:
In `@backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py`:
- Around line 98-108: The computed rework_time in etl_code_analytics.py can be
negative when using the branch that sets rework_time = (pr.state_changed_at -
first_response_end_time).total_seconds(); update that branch to clamp negative
values the same way merge_time does (set rework_time to -1 when the computed
value is < 0) so pr.rework_time never receives a raw negative duration. Locate
the variables/expressions rework_time, pr.state_changed_at,
first_response_end_time, first_review and approved_reviews and apply the guard
there to match the merge_time behavior.
---
Nitpick comments:
In `@backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py`:
- Around line 131-138: The fallback branch in PRPerformance.first_review_time is
dead because first_response_end_time is always truthy; simplify the expression
by removing the ternary and unconditionally computing (first_response_end_time -
pull_request_ready_for_review_time).total_seconds() when constructing
PRPerformance in create_pr_metrics (use the existing symbols
first_response_end_time and pull_request_ready_for_review_time and the
PRPerformance constructor) so the unreachable "else -1" branch is eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f10b82b8-8ce9-4f3e-8c61-2ece778f78ee
📒 Files selected for processing (1)
backend/analytics_server/mhq/service/code/sync/etl_code_analytics.py
There was a problem hiding this comment.
@THEAbhishekjoshi can you please review the linter and breaking unit tests?
|
@dhruvagarwal i'm on it |
|
hey @dhruvagarwal I've pushed the backend formatting fixes (black and trailing whitespace) and the corrected unit tests could you please approve the workflow runs to let the tests execute and re-review the changes when you have a moment :) |
3 participants
Summary
Fixes #696
Fixes incorrect Lead Time breakdown calculations when PRs are merged without review and/or approval events.
Previously, if a PR skipped parts of the review chain, portions of the elapsed lifecycle time were not attributed to any breakdown component, causing the breakdown totals to not reconcile with the overall lead/cycle time.
Problem
The existing implementation assumes the following review chain always exists:
However, some PRs may be:
In those cases:
first_review_timerework_timemerge_timewere set to
-1/None, causing parts of the elapsed time to disappear from the Lead Time breakdown.Example:
As a result, the total cycle time remained correct, but the breakdown components no longer summed to the total.
Solution
Implemented fallback timing behavior so elapsed time is attributed to the next logical stage when review events are missing.
Fallback behavior:
This ensures:
Summary by CodeRabbit