Feature: Time from rerequest to response(PR author perspective) #59
Conversation
WalkthroughThis pull request introduces enhancements to tracking and analytics functionality, focusing on adding new metrics related to GitHub repository and review processes. The changes involve removing validation utilities, adding new properties for tracking GitHub ownership and repository information, and introducing metrics for time spent waiting for repeated reviews. The modifications span multiple files across analytics, converters, view, and utility modules, with a significant refactoring of validation-related code. Changes
Sequence DiagramsequenceDiagram
participant Analytics as SendActionRun
participant Converters as PullRequestStats
participant View as TimelineVisualization
Analytics->>Converters: Track GitHub ownership metrics
Converters->>Converters: Calculate repeated review times
Converters->>View: Prepare timeline data
View->>View: Render chart and table with new metrics
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (10)
README.mdis excluded by none and included by nonebuild/index.jsis excluded by!build/**and included by noneconfigs/lastMonthsReport.ymlis excluded by none and included by noneconfigs/manualTriggerForAllReposOfOrg.ymlis excluded by none and included by noneconfigs/updateReportOnPRClose.ymlis excluded by none and included by noneconfigs/yearReportWithoutDevelopers.ymlis excluded by none and included by noneexamples/collectionExample.jsonis excluded by none and included by noneexamples/nPRsReport.mdis excluded by none and included by noneexamples/periodReport.mdis excluded by none and included by nonepackage.jsonis excluded by none and included by none
📒 Files selected for processing (18)
src/analytics/sendActionRun.ts(1 hunks)src/common/utils/index.ts(0 hunks)src/common/utils/validate.ts(0 hunks)src/common/utils/validators/index.ts(0 hunks)src/common/utils/validators/validateDate.ts(0 hunks)src/common/utils/validators/validateMultipleValues.ts(0 hunks)src/common/utils/validators/validateNumber.ts(0 hunks)src/common/utils/validators/validateRequired.ts(0 hunks)src/common/utils/validators/validateSingleValue.ts(0 hunks)src/converters/types.ts(2 hunks)src/converters/utils/calculations/getApproveTime.spec.ts(2 hunks)src/converters/utils/preparePullRequestStats.ts(3 hunks)src/converters/utils/prepareResponseTime.ts(2 hunks)src/createOutput.ts(1 hunks)src/index.ts(0 hunks)src/view/utils/constants.ts(1 hunks)src/view/utils/createTimelineMonthXYChart.ts(3 hunks)src/view/utils/createTimelineTable.ts(3 hunks)
💤 Files with no reviewable changes (9)
- src/common/utils/index.ts
- src/index.ts
- src/common/utils/validators/validateDate.ts
- src/common/utils/validate.ts
- src/common/utils/validators/validateNumber.ts
- src/common/utils/validators/validateRequired.ts
- src/common/utils/validators/validateSingleValue.ts
- src/common/utils/validators/index.ts
- src/common/utils/validators/validateMultipleValues.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/converters/utils/prepareResponseTime.ts
[error] 29-29: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (11)
src/view/utils/constants.ts (1)
5-6: LGTM! The new constant follows the established pattern.The new header constant is well-named and maintains consistency with existing constants.
src/view/utils/createTimelineTable.ts (3)
3-3: LGTM! Import added correctly.The new header constant is properly imported alongside other constants.
32-34: LGTM! Time metric formatting follows the established pattern.The new time metric is properly integrated with:
- Consistent use of optional chaining
- Same formatting function as other time metrics
53-53: LGTM! Header properly added to table configuration.The new header is correctly integrated into the table headers array.
src/analytics/sendActionRun.ts (1)
12-13: LGTM! Analytics properties added with proper null safety.The new tracking properties are well-integrated with:
- Proper use of optional chaining for null safety
- Consistent pattern of tracking value lengths
src/converters/types.ts (1)
16-16: LGTM! Types properly defined for the new metric.The new property is well-integrated in both types:
- Properly marked as optional with
?- Consistent naming across types
- Correct type definitions (number for TimelinePoints, number[] for Collection)
Also applies to: 74-74
src/converters/utils/preparePullRequestStats.ts (1)
76-78: LGTM! Consistent implementation of the new metric.The new metric
timeWaitingForRepeatedReviewhas been correctly added to all statistical calculations (median, percentile, and average), following the established patterns in the codebase.Also applies to: 95-97, 114-116
src/converters/utils/calculations/getApproveTime.spec.ts (1)
115-131: LGTM! Well-structured test case for dismissed reviews.The new test case thoroughly covers the scenario where a second reviewer dismisses their changes_requested review, ensuring the function returns the correct timestamp.
Also applies to: 170-174
src/converters/utils/prepareResponseTime.ts (1)
21-56: LGTM! Robust implementation with proper error handling.The implementation properly handles:
- Fallback for missing user login
- Working hours and holidays in time calculations
- Team-based aggregation
🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
src/view/utils/createTimelineMonthXYChart.ts (1)
36-36: LGTM! Consistent visualization of the new metric.The new metric has been properly integrated into the chart with:
- Appropriate color scheme (turquoise)
- Consistent time unit conversion (to hours)
- Type-safe implementation
Also applies to: 49-49, 141-155
src/createOutput.ts (1)
111-111: 🧹 Nitpick (assertive)Reconsider the arbitrary threshold for comment creation.
The condition
dates.length < 3seems arbitrary and could lead to loss of information. If there are 1-2 dates with valid markdown content, their comments won't be created even though they might contain valuable insights.Consider the following:
- Document why exactly 3 dates were chosen as the threshold
- Make this threshold configurable through a constant or environment variable
- Alternatively, create comments for all dates that have valid markdown content, regardless of the total number of dates
Would you like me to propose a more flexible implementation that preserves all valid date-specific analytics while still achieving the goal of reducing unnecessary comments?
Pull Request
Description
Type of Change
How Has This Been Tested?
Added unit-tests. Checked it on different repositories.
Checklist:
Summary by CodeRabbit
Analytics
Performance
Visualization
Validation
Chores