-
Notifications
You must be signed in to change notification settings - Fork 40
chore(dev): Add a script to analyze cross-team PR reviews #6919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # Label PRs that require cross-team reviews | ||
| # Used by scripts/reports/pr-review-times to generate reports | ||
| name: Label Cross Team | ||
| on: | ||
| pull_request_target: | ||
| types: [review_requested, opened] | ||
|
|
||
| permissions: | ||
| pull-requests: write | ||
|
|
||
| jobs: | ||
| label: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Harden Runner | ||
| uses: step-security/harden-runner@ec9f2d5744a09debf3a187a3f4f675c53b671911 # v2.13.0 | ||
| with: | ||
| egress-policy: audit | ||
|
|
||
| - name: Label Cross Team | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | | ||
| try { | ||
| const { data: pr } = await github.rest.pulls.get({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.issue.number, | ||
| }); | ||
|
|
||
| const teams = pr.requested_teams || []; | ||
| console.log(`Requested teams: ${teams.map((t) => t.slug).join(', ')}`); | ||
|
|
||
| if (teams.length > 1) { | ||
| const hasLabel = pr.labels.some((l) => l.name === 'cross-team'); | ||
| if (hasLabel) { | ||
| console.log(`'cross-team' label already present. Skipping.`); | ||
| } else { | ||
| console.log(`Found ${teams.length} teams requested. Adding 'cross-team' label.`); | ||
| await github.rest.issues.addLabels({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| labels: ['cross-team'], | ||
| }); | ||
| } | ||
| } else { | ||
| console.log(`Only ${teams.length} teams requested. No label needed.`); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error in label-cross-team action:', error); | ||
| core.setFailed(error.message); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| *.json | ||
| *.csv |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,100 @@ | ||||||||||
| # PR Review Time Analysis Tools | ||||||||||
|
|
||||||||||
| A suite of vibe-coded scripts to fetch, analyze, and report on Pull Request review performance and collaboration patterns in the `coveo/ui-kit` repository. | ||||||||||
|
|
||||||||||
| ## Intent | ||||||||||
|
|
||||||||||
| The primary goal of this toolset is to provide visibility into the code review process. It helps answer questions like: | ||||||||||
| - How long does it take for different teams to Pick Up and Approve PRs? | ||||||||||
| - How much "cross-team" collaboration is happening? | ||||||||||
| - Are we improving over time? | ||||||||||
|
|
||||||||||
| By distinguishing between **Business Days** and calendar days, and filtering out holidays, the metrics aim to be fair and accurate representations of team responsiveness. | ||||||||||
|
|
||||||||||
| ## Design Choices | ||||||||||
|
|
||||||||||
| ### 1. Modular Architecture | ||||||||||
| The logic is split into distinct modules to separate concerns: | ||||||||||
| - **`index.mjs`**: The CLI entry point that orchestrates the workflow. | ||||||||||
| - **`fetch-pr.mjs`**: Fetches raw data from GitHub and performs per-PR analysis (calculating durations, identifying teams). | ||||||||||
| - **`aggregate.mjs`**: Consolidates analyzed PRs into high-level metrics (averages, medians, percentiles) and generates reports (JSON, CSV, Console). | ||||||||||
| - **`holidays.mjs`**: Contains the logic for "Business Days". It ignores weekends and fixed/moving holidays (New Year's, Patriot's Day, etc.) to calculate accurate durations. | ||||||||||
| - **`codeowners.mjs`**: Parses the repository's `CODEOWNERS` file to map files to teams. | ||||||||||
|
|
||||||||||
| ### 2. Local Caching & Differential Updates | ||||||||||
| Github API rate limits can be a bottleneck. The script uses a local filesystem cache (`data/*.json`): | ||||||||||
| - When running `fetch`, the script checks the `updated_at` timestamp of the PR. | ||||||||||
| - It only re-fetches data from GitHub if the PR has changed since the last run. | ||||||||||
| - This allows for fast incremental updates and offline reporting. | ||||||||||
|
|
||||||||||
| ### 3. Identifying Cross-Team PRs | ||||||||||
|
|
||||||||||
| A PR is determined to be cross-team if a review is requested from more than one team. | ||||||||||
|
|
||||||||||
| A [GitHub action](../../../.github/workflows/label-cross-team.yml) automatically adds a `cross-team` label for easier filtering. | ||||||||||
|
|
||||||||||
| PRs created after November 1st 2025 were [backfilled](backfill-label.mjs). | ||||||||||
|
|
||||||||||
| ### 4. Author Confidence | ||||||||||
|
|
||||||||||
| The report distinguishes between: | ||||||||||
|
|
||||||||||
| - `high-confidence`: The system is blocking from merging due to CODEOWNERS, but the author rather confident about the change and thinks they should be able to merge without an additional review. | ||||||||||
| - `low-confidence`: The author is not that confident and actually wants another team to review the change before merging. | ||||||||||
|
Comment on lines
+42
to
+43
|
||||||||||
| - `high-confidence`: The system is blocking from merging due to CODEOWNERS, but the author rather confident about the change and thinks they should be able to merge without an additional review. | |
| - `low-confidence`: The author is not that confident and actually wants another team to review the change before merging. | |
| - `high-confidence`: The system is blocking from merging due to CODEOWNERS, and the author is not that confident and actually wants another team to review the change before merging. | |
| - `low-confidence`: The author is rather confident about the change and thinks they should be able to merge without an additional review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'core' object is referenced on line 53 but is never imported or defined in the script. This will cause a ReferenceError at runtime if an error occurs. The correct usage would be to throw the error or simply log it, since 'core.setFailed' is not available in this context.