Feature: Support for teams in EXCLUDE_USERS/INCLUDE_USERS parameters #76
Conversation
WalkthroughThis pull request extends user inclusion/exclusion filtering to support team-aware logic. Functions across the codebase are updated to accept a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/converters/utils/prepareReviews.ts (1)
16-44: FixteamNamesaccumulation to avoid dropping some team metricsThe new
checkUserInclusive(userLogin, teams)calls look good, but the existingteamNameshandling is problematic:
teamNamesis reassigned on every iteration of thereduce, so after the loop it only contains the teams of the last included reviewer.- When iterating
users, earlier team keys not in that last set fall into the!teamNames.includes(user)branch and end up withuserReviewsfiltered byuserLogin === user, which yields no reviews and effectively drops team-level metrics for those teams.This likely skews
reviewsConductedfor scenarios with multiple distinct teams.A minimal fix is to accumulate all team names instead of overwriting:
- let teamNames: string[] = []; - const users = Object.keys( - reviews?.reduce((acc, review) => { + const teamNamesSet = new Set<string>(); + const users = Object.keys( + reviews?.reduce((acc, review) => { const userLogin = review.user?.login || invalidUserLogin; if (userLogin !== pullRequestLogin && checkUserInclusive(userLogin, teams)) { const teamsNames = (teams[userLogin] || []).reduce( (acc, team) => ({ ...acc, [team]: 1 }), {} ); - teamNames = Object.keys(teamsNames); + Object.keys(teamsNames).forEach((team) => teamNamesSet.add(team)); return { ...acc, [userLogin]: 1, ...teamsNames, total: 1 }; } return acc; }, {}) || {} ); + + const teamNames = Array.from(teamNamesSet);This preserves your existing structure while ensuring all team keys are correctly recognized as teams during the
users.forEachphase.src/converters/utils/calculations/checkUserInclusive.ts (1)
3-27: Team-aware inclusion/exclusion logic is sound; cache inputs for performanceThe extended logic correctly:
- Excludes a user if they or any of their teams are in
EXCLUDE_USERS.- Includes a user only if they or any of their teams are in
INCLUDE_USERSwhen an include list is present, with EXCLUDE taking precedence.Given how often this helper is called, it would be better to avoid recomputing the inputs on every branch:
-export const checkUserInclusive = ( - name: string, - teams: Record<string, string[]> -) => { - if ( - getMultipleValuesInput("EXCLUDE_USERS").length === 0 && - getMultipleValuesInput("INCLUDE_USERS").length === 0 - ) { +export const checkUserInclusive = ( + name: string, + teams: Record<string, string[]> +) => { + const excluded = getMultipleValuesInput("EXCLUDE_USERS"); + const included = getMultipleValuesInput("INCLUDE_USERS"); + + if (excluded.length === 0 && included.length === 0) { return true; } if ( - getMultipleValuesInput("EXCLUDE_USERS").length > 0 && - (getMultipleValuesInput("EXCLUDE_USERS").includes(name) || - teams[name]?.some((team) => - getMultipleValuesInput("EXCLUDE_USERS").includes(team) - )) + excluded.length > 0 && + (excluded.includes(name) || + teams[name]?.some((team) => excluded.includes(team))) ) { return false; } - return getMultipleValuesInput("INCLUDE_USERS").length > 0 - ? getMultipleValuesInput("INCLUDE_USERS").includes(name) || - teams[name]?.some((team) => - getMultipleValuesInput("INCLUDE_USERS").includes(team) - ) + return included.length > 0 + ? included.includes(name) || + teams[name]?.some((team) => included.includes(team)) : true; };Functionally equivalent, but avoids repeated I/O/parsing on every call.
src/converters/utils/prepareResponseTime.ts (1)
101-147: Consider hoisting the inclusion check for efficiency.The
checkUserInclusive(user, teams)call at Line 102 is executed inside theuserKeyloop, resulting in redundant checks with identical arguments. Sinceuser(the reviewer) doesn't change within this loop, consider moving the check outside:["total", dateKey].forEach((key) => { const timeFromInitialRequestToResponse = calcDifferenceInMinutes( responses[0]?.[0], responses[0]?.[1], { endOfWorkingTime: getValueAsIs("CORE_HOURS_END"), startOfWorkingTime: getValueAsIs("CORE_HOURS_START"), }, getMultipleValuesInput("HOLIDAYS") ); const timeFromOpenToResponse = calcDifferenceInMinutes( pullRequest?.created_at, responses[0]?.[1], { endOfWorkingTime: getValueAsIs("CORE_HOURS_END"), startOfWorkingTime: getValueAsIs("CORE_HOURS_START"), }, getMultipleValuesInput("HOLIDAYS") ); const timeFromRepeatedRequestToResponse = responses .filter((el, index) => index > 0) .map((element) => calcDifferenceInMinutes( element?.[0], element?.[1], { endOfWorkingTime: getValueAsIs("CORE_HOURS_END"), startOfWorkingTime: getValueAsIs("CORE_HOURS_START"), }, getMultipleValuesInput("HOLIDAYS") ) ); + if (!checkUserInclusive(user, teams)) { + return; + } + ["total", user, ...(teams[user] || [])].forEach((userKey) => { - if (checkUserInclusive(user, teams)) { set(collection, [userKey, key], { ...get(collection, [userKey, key], {}), timeFromInitialRequestToResponse: typeof timeFromInitialRequestToResponse === "number" ? [ ...get( collection, [userKey, key, "timeFromInitialRequestToResponse"], [] ), timeFromInitialRequestToResponse, ] : get( collection, [userKey, key, "timeFromInitialRequestToResponse"], [] ), timeFromOpenToResponse: typeof timeFromOpenToResponse === "number" ? [ ...get( collection, [userKey, key, "timeFromOpenToResponse"], [] ), timeFromOpenToResponse, ] : get( collection, [userKey, key, "timeFromOpenToResponse"], [] ), timeFromRepeatedRequestToResponse: [ ...get( collection, [userKey, key, "timeFromRepeatedRequestToResponse"], [] ), ...(timeFromRepeatedRequestToResponse.filter( (el) => typeof el === "number" ) as number[]), ], }); - } }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
README.mdis excluded by none and included by nonebuild/index.jsis excluded by!build/**and included by nonepackage-lock.jsonis excluded by!**/package-lock.jsonand included by nonepackage.jsonis excluded by none and included by none
📒 Files selected for processing (11)
src/converters/collectData.ts(3 hunks)src/converters/utils/calculations/checkUserInclusive.ts(2 hunks)src/converters/utils/calculations/getApproveTime.ts(2 hunks)src/converters/utils/calculations/getResponses.ts(3 hunks)src/converters/utils/prepareActionsTime.ts(2 hunks)src/converters/utils/prepareConductedReviews.ts(1 hunks)src/converters/utils/prepareDiscussions.ts(6 hunks)src/converters/utils/preparePullRequestTimeline.ts(1 hunks)src/converters/utils/prepareRequestedReviews.ts(2 hunks)src/converters/utils/prepareResponseTime.ts(3 hunks)src/converters/utils/prepareReviews.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/converters/utils/prepareActionsTime.ts (3)
src/converters/types.ts (1)
Collection(52-124)src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)src/converters/constants.ts (1)
invalidUserLogin(1-1)
src/converters/utils/prepareRequestedReviews.ts (1)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)
src/converters/utils/preparePullRequestTimeline.ts (5)
src/converters/types.ts (1)
Collection(52-124)src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)src/converters/constants.ts (1)
invalidUserLogin(1-1)src/converters/utils/calculations/getApproveTime.ts (1)
getApproveTime(6-58)src/common/utils/index.ts (1)
getValueAsIs(2-2)
src/converters/utils/calculations/getApproveTime.ts (1)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)
src/converters/utils/prepareResponseTime.ts (2)
src/converters/utils/calculations/getResponses.ts (1)
getResponses(9-55)src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)
src/converters/utils/calculations/checkUserInclusive.ts (1)
src/converters/utils/calculations/index.ts (1)
checkUserInclusive(14-14)
src/converters/utils/prepareDiscussions.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)src/converters/constants.ts (1)
invalidUserLogin(1-1)
src/converters/utils/prepareConductedReviews.ts (1)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)
src/converters/utils/calculations/getResponses.ts (3)
src/converters/utils/calculations/index.ts (2)
getResponses(12-12)checkUserInclusive(14-14)src/converters/constants.ts (2)
reviewRequestedTimelineEvent(4-4)invalidUserLogin(1-1)src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)
src/converters/collectData.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)src/converters/utils/calculations/index.ts (1)
checkUserInclusive(14-14)
src/converters/utils/prepareReviews.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(3-28)src/converters/constants.ts (1)
invalidUserLogin(1-1)
🔇 Additional comments (12)
src/converters/utils/calculations/getApproveTime.ts (1)
6-10: Team-aware filtering of approvals looks correctPassing
teamsintocheckUserInclusivebefore updatingstatusesensures excluded users/teams no longer affect approval time, and the updated function signature is consistent with upstream callers. No issues from this change.Also applies to: 17-20
src/converters/utils/prepareConductedReviews.ts (1)
21-23:reviewsConductednow correctly respects team filtersUsing
checkUserInclusive(key, teams)in the loop ensures both per-user and per-teamreviewsConductedentries honor EXCLUDE/INCLUDE team settings without changing existing"total"behavior.src/converters/utils/calculations/getResponses.ts (1)
9-12: Response timelines now correctly honor team-based filtersThreading
teamsintogetResponsesand gating each event type withcheckUserInclusive(user, teams)ensures that:
- Requests to excluded users/teams are not tracked.
- Reviews by excluded users/teams don’t close open request intervals.
- Remove events don’t touch state for excluded users/teams.
The change is consistent with the updated
checkUserInclusivesemantics.Also applies to: 15-17, 25-27, 39-41
src/converters/utils/preparePullRequestTimeline.ts (1)
21-36: Timeline metrics correctly wired to team-aware filtersUsing
checkUserInclusivewithteamsfor the PR author and first reviewer, and passingteamsintogetApproveTime, ensures all time-to-* metrics ignore PRs and approvals from excluded users/teams. The updated signature and call sites are consistent.src/converters/utils/prepareRequestedReviews.ts (1)
14-20: Requested-review stats now fully respect team filtersGating both the construction of
requestedReviewersand the final aggregation intocollectionwithcheckUserInclusive(user, teams)correctly:
- Drops requests made to excluded users/teams.
- Prevents rows for excluded teams/users from being created or incremented.
This aligns prepareRequestedReviews with the new team-aware inclusion logic elsewhere.
Also applies to: 30-33
src/converters/utils/prepareResponseTime.ts (2)
24-24: LGTM! Correct propagation of teams parameter.The
getResponsescall is correctly updated to pass theteamsparameter, enabling team-aware filtering of review responses.
47-47: LGTM! Correct bucket-level filtering.The
checkUserInclusive(userKey, teams)call correctly filters each bucket independently, including whenuserKeyrepresents a team name from the spread...(teams[user] || []).src/converters/collectData.ts (2)
51-56: LGTM! Correct propagation of teams parameter.The
prepareActionsTimecall is correctly updated to pass theteamsparameter, enabling team-aware filtering of action time metrics.
70-95: Verify the intentional asymmetry between preparePullRequestInfo and preparePullRequestTimeline.There's an asymmetry in how these two functions are called:
preparePullRequestInfo(Line 76) is only called whencheckUserInclusive(userKey, teams)returns true (Line 72)preparePullRequestTimeline(Line 86) is always called, though it receives theteamsparameter for internal filteringThis means PR info (opened, closed counts, etc.) is filtered at this level, while timeline data is always collected and presumably filtered internally by
preparePullRequestTimeline.If this is intentional (e.g., timeline needs to aggregate data across all users for dependency tracking), please confirm. Otherwise, consider whether
preparePullRequestTimelineshould also be gated by the same inclusion check for consistency.src/converters/utils/prepareActionsTime.ts (3)
8-15: LGTM! Function signature correctly extended.The function signature is properly updated to include the
teamsparameter, aligning with the broader PR changes to support team-aware filtering.
30-36: LGTM! Correct filtering for reviewer actions.The
checkUserInclusive(user, teams)call correctly filters reviewer actions based on both individual user inclusion and team membership.
38-49: LGTM! Correct filtering for PR author actions.Both
checkUserInclusive(prAuthor, teams)calls correctly filter PR author actions (opening and merging) based on individual user inclusion and team membership.
| const reviewComments = comments[index]?.filter( | ||
| (comment) => | ||
| pullRequestLogin !== (comment.user?.login || invalidUserLogin) && | ||
| checkUserInclusive(comment.user?.login || invalidUserLogin) | ||
| checkUserInclusive(comment.user?.login || invalidUserLogin, teams) | ||
| ); | ||
|
|
||
| const discussions = comments[index]?.filter((comment) => { | ||
| const userLogin = comment.user?.login || invalidUserLogin; | ||
| return ( | ||
| !comment.in_reply_to_id && | ||
| pullRequestLogin !== userLogin && | ||
| checkUserInclusive(userLogin) | ||
| checkUserInclusive(userLogin, teams) | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Team-aware filtering is mostly consistent; consider tightening it for ‘received’ discussion types
Passing teams into all checkUserInclusive calls in this file correctly filters:
- Which commenters’ discussions/review comments are counted.
- Whether the PR author’s own metrics (e.g.,
reviewComments) are updated at all.
One subtle gap remains around received discussion-type metrics:
- In the loop over
[pullRequestLogin, ...(teams[pullRequestLogin] || []), "total"], the guard isif (checkUserInclusive(userLogin, teams)), i.e., it only checks the commenter, not the recipient (pullRequestLoginor its teams). - With
EXCLUDE_USERScontaining a teamT, PRs authored by members ofTcan still contribute rows forTindiscussionsTypes.receivedwhen non-excluded users comment on them, because those comments pass theuserLogincheck.
If the intent of issue #72 is “no rows at all for excluded teams,” you likely want to also gate by the receiver/team key. For example:
- [pullRequestLogin, ...(teams[pullRequestLogin] || []), "total"].forEach(
- (userKey) => {
- if (checkUserInclusive(userLogin, teams)) {
+ [pullRequestLogin, ...(teams[pullRequestLogin] || []), "total"].forEach(
+ (userKey) => {
+ if (
+ checkUserInclusive(userLogin, teams) &&
+ checkUserInclusive(userKey, teams)
+ ) {
set(collection, [userKey, key, "discussionsTypes", type], {
...
});
}
}
);A similar pattern can be applied to other places where you iterate userKey over users/teams/"total" but only check userLogin. That way:
- Excluded commenters don’t generate any metrics.
- Excluded teams/users don’t get rows, even when non-excluded people interact with their PRs.
If the current behavior is intentional (e.g., you do want to see what excluded teams receive, but not what they conduct), it’d be good to document that explicitly.
Also applies to: 33-41, 86-90, 143-146, 162-163, 212-215
🤖 Prompt for AI Agents
In src/converters/utils/prepareDiscussions.ts around lines 18 to 32, the
filtering only validates the commenter via checkUserInclusive(commenter, teams)
but when you later iterate recipient keys (pullRequestLogin,
teams[pullRequestLogin], "total") you must also gate by the recipient/team key
so excluded teams/users do not get rows for received metrics; update the guard
to require checkUserInclusive(commenterLogin, teams) && (userKey === "total" ||
checkUserInclusive(userKey, teams)) (or equivalent logic that treats "total" as
always allowed), and apply the same recipient-aware check to the other noted
ranges (33-41, 86-90, 143-146, 162-163, 212-215).
Pull Request
Description
Adds support for teams in the EXCLUDE_USERS and INCLUDE_USERS parameters. Now, in addition to individual users, you can list teams separated by commas.
Related Issue(s)
Type of Change
How Has This Been Tested?
Run action on a test data.
Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.