Feature: Exclude/Include users from report #70
Conversation
WalkthroughUser filtering logic was introduced across multiple analytics and aggregation functions, using a new Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant checkUserInclusive
participant AnalyticsFunction
participant User
Config->>checkUserInclusive: Provide INCLUDE_USERS, EXCLUDE_USERS
AnalyticsFunction->>User: For each user in data
AnalyticsFunction->>checkUserInclusive: Is User allowed?
alt User is allowed
checkUserInclusive-->>AnalyticsFunction: true
AnalyticsFunction->>AnalyticsFunction: Process user data
else User is not allowed
checkUserInclusive-->>AnalyticsFunction: false
AnalyticsFunction->>AnalyticsFunction: Skip user
end
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (4)
src/converters/collectData.ts (1)
71-92:⚠️ Potential issueTimeline still processed for excluded users – metrics become inconsistent
checkUserInclusiveguards thepreparePullRequestInfocall but the subsequent
preparePullRequestTimelineinvocation is unconditional, so:
- an excluded user does not affect counters (
opened,merged, …)- yet their entire timeline is merged into
collectionThis leads to impossible states (e.g. 0 opened PRs but non-empty timeline
arrays) and leaks excluded users into the final report.Suggested fix – move the timeline logic under the same guard:
- if (checkUserInclusive(userKey)) { - set( - collection, - [key, innerKey], - preparePullRequestInfo( - pullRequest, - get(collection, [key, innerKey], {}) - ) - ); - } - - set( + if (checkUserInclusive(userKey)) { + set( collection, [key, innerKey], - preparePullRequestTimeline( - pullRequest, - reviews, - reviewRequests?.[0], - statuses, - get(collection, [key, innerKey], {}) - ) + preparePullRequestInfo( + pullRequest, + get(collection, [key, innerKey], {}) + ) ); + set( + collection, + [key, innerKey], + preparePullRequestTimeline( + pullRequest, + reviews, + reviewRequests?.[0], + statuses, + get(collection, [key, innerKey], {}) + ) + ); + }src/converters/utils/prepareRequestedReviews.ts (1)
14-20: 🛠️ Refactor suggestionAccumulator spread causes O(n²) – replace with direct mutation
Using
{ ...acc, [user]: 1 }reallocates and copies the accumulator on every iteration.
Adopt in-place mutation to keep the loop linear:- const requestedReviewers = requests.reduce((acc, request) => { + const requestedReviewers = requests.reduce((acc, request) => { const user = request.requested_reviewer ? request.requested_reviewer?.login || invalidUserLogin : request.requested_team?.name || "Invalid Team"; - if(!checkUserInclusive(user)) return acc; - return { ...acc, [user]: 1 }; + if (!checkUserInclusive(user)) return acc; + acc[user] = 1; + return acc; }, {});This eliminates the quadratic copy cost flagged by static-analysis tools.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: 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/converters/utils/calculations/getResponses.ts (1)
35-48:⚠️ Potential issuePotential crash when
review_request_removedarrives before a matching request
acc[user]may beundefined, leading toacc[user].map(...)throwing.
Guard against this:- return { - ...acc, - [user]: acc[user].map( + if (!acc[user]) return acc; // nothing to adjust + acc[user] = acc[user].map( (el: [string | undefined | null, string | undefined | null]) => [ el[0], null, ] - ), - }; + ); + return acc;🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: 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/converters/utils/prepareReviews.ts (1)
18-29: 🛠️ Refactor suggestionLogical bug: only the last reviewer’s teams are recorded
Inside
reduce,teamNamesis reassigned each iteration, so teams from earlier reviewers are overwritten.
Consequences:
• Some team aggregates are never built.
• Subsequent filtering withteamNames.includes(user)misses teams dropped earlier.At the same time, the spread operator on every accumulation causes O(n²) copies – Biome flagged this.
Fix both in one go:
- let teamNames: string[] = []; + const teamNamesSet = new Set<string>(); const users = Object.keys( reviews?.reduce((acc, review) => { const userLogin = review.user?.login || invalidUserLogin; if (userLogin !== pullRequestLogin && checkUserInclusive(userLogin)) { - const teamsNames = (teams[userLogin] || []).reduce( - (acc, team) => ({ ...acc, [team]: 1 }), - {} - ); - teamNames = Object.keys(teamsNames); - return { ...acc, [userLogin]: 1, ...teamsNames, total: 1 }; + (teams[userLogin] || []).forEach((t) => teamNamesSet.add(t)); + acc[userLogin] = 1; + acc.total = 1; + return acc; } return acc; }, {}) || {} ); + + const teamNames = [...teamNamesSet];• All teams accumulate correctly.
• No expensive object spreads on every iteration.🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: 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)
[error] 28-28: 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)
📜 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 noneaction.ymlis excluded by none and included by nonebuild/index.jsis excluded by!build/**and included by nonepackage.jsonis excluded by none and included by none
📒 Files selected for processing (14)
src/converters/collectData.ts(2 hunks)src/converters/utils/calculations/checkUserInclusive.ts(1 hunks)src/converters/utils/calculations/getApproveTime.ts(2 hunks)src/converters/utils/calculations/getResponses.ts(2 hunks)src/converters/utils/calculations/index.ts(1 hunks)src/converters/utils/prepareActionsTime.ts(2 hunks)src/converters/utils/prepareConductedReviews.ts(2 hunks)src/converters/utils/prepareDiscussions.ts(6 hunks)src/converters/utils/preparePullRequestTimeline.ts(2 hunks)src/converters/utils/prepareRequestedReviews.ts(3 hunks)src/converters/utils/prepareResponseTime.ts(3 hunks)src/converters/utils/prepareReviews.ts(2 hunks)src/view/createMarkdown.ts(1 hunks)src/view/utils/createConfigParamsCode.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/converters/collectData.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/converters/utils/preparePullRequestInfo.ts (1)
preparePullRequestInfo(6-46)
src/converters/utils/calculations/getApproveTime.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/converters/utils/calculations/index.ts (1)
checkUserInclusive(14-14)
src/converters/utils/prepareActionsTime.ts (2)
src/converters/constants.ts (1)
invalidUserLogin(1-1)src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)
src/converters/utils/preparePullRequestTimeline.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/converters/constants.ts (1)
invalidUserLogin(1-1)
src/converters/utils/prepareConductedReviews.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/converters/utils/calculations/index.ts (1)
checkUserInclusive(14-14)
src/converters/utils/calculations/getResponses.ts (2)
src/converters/constants.ts (3)
reviewRequestedTimelineEvent(4-4)invalidUserLogin(1-1)reviewedTimelineEvent(7-7)src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)
src/converters/utils/prepareRequestedReviews.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/converters/utils/calculations/index.ts (1)
checkUserInclusive(14-14)
src/converters/utils/prepareReviews.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/converters/constants.ts (1)
invalidUserLogin(1-1)
🪛 Biome (1.9.4)
src/converters/utils/calculations/getResponses.ts
[error] 17-17: 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)
src/converters/utils/prepareReviews.ts
[error] 24-24: 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)
[error] 28-28: 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 (6)
src/converters/utils/prepareConductedReviews.ts (2)
2-2: Import path is fineThe new helper is correctly re-exported from
calculations/index.ts, so this import remains stable even if the file is relocated later.
23-23:totalbucket may disappear whenINCLUDE_USERSis set
checkUserInclusive("total")returnsfalsewhenever anINCLUDE_USERS
list is configured (because"total"is never in that list).
This skips aggregation for the global bucket, so reports will lack overall
numbers in “conducted reviews”.Verify whether that behaviour is intended; if not, special-case
"total":-if (!checkUserInclusive(key)) return; +if (key !== "total" && !checkUserInclusive(key)) return;src/converters/utils/calculations/getApproveTime.ts (2)
4-4: Good to see filter applied earlyEarly-exit in the reducer avoids unnecessary work and keeps approval
calculations consistent with inclusion/exclusion rules.
16-18: 🧹 Nitpick (assertive)Minor micro-optimisation
checkUserInclusive(user)is called for every review.
Caching the boolean outside the heavy reducer body saves redundant list
look-ups:-const user = review.user?.login || invalidUserLogin; -if (!checkUserInclusive(user)) { +const user = review.user?.login || invalidUserLogin; +const include = checkUserInclusive(user); +if (!include) { return acc; }Likely an incorrect or invalid review comment.
src/view/createMarkdown.ts (1)
43-44:Details
❓ Verification inconclusive
Closed-PR sentence dropped – verify that stakeholders still get the “sample size” context
The count of processed PRs is no longer surfaced. If teams relied on that number to gauge how representative the report is, consider adding an alternative (e.g. “Total PRs analysed: X”) elsewhere in the markdown, or confirm the removal is intentional.
🏁 Script executed:
#!/bin/bash set -e # Locate the createMarkdown.ts file file=$(fd createMarkdown.ts | head -n1) echo "Inspecting file: $file" # Show context around the issueDescription rg -n "issueDescription" -C3 "$file" # Search for any mentions of PR counts or sample-size-related keywords rg -nE "PRs?|Total|analys(ed|yse)|count" -C1 "$file" || echo "No sample-size or PR-count mentions found"Length of output: 917
🏁 Script executed:
#!/bin/bash set -e # Find where createConfigParamsCode is defined rg -n "createConfigParamsCode" -C3Length of output: 1982
Verify sample-size context after PR count removal
The generated markdown no longer includes the total number of PRs processed, which teams may have used to assess how representative the report is. Please confirm this omission is intentional, or consider adding an explicit “Total PRs analysed: X” line.
Locations to review:
- src/view/createMarkdown.ts around the
issueDescriptiontemplate (lines 43–44)- src/view/utils/createConfigParamsCode.ts where report settings are listed
Example addition:
Total PRs analysed: ${prCount}src/converters/utils/prepareActionsTime.ts (1)
28-34: Good defensive default for unknown reviewer loginUsing
invalidUserLoginwhenloginis absent prevents runtime errors.
Implementation looks correct.
| "INCLUDE_USERS", | ||
| "EXCLUDE_USERS", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Keep configuration keys ordered for readability
All other keys appear to follow alphabetical order. Inserting INCLUDE_USERS / EXCLUDE_USERS breaks that pattern, making future diffs harder to scan.
- "INCLUDE_USERS",
- "EXCLUDE_USERS",
+ "EXCLUDE_USERS",
+ "INCLUDE_USERS",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "INCLUDE_USERS", | |
| "EXCLUDE_USERS", | |
| "EXCLUDE_USERS", | |
| "INCLUDE_USERS", |
🤖 Prompt for AI Agents
In src/view/utils/createConfigParamsCode.ts around lines 38 to 39, the
configuration keys INCLUDE_USERS and EXCLUDE_USERS are not in alphabetical
order, breaking the existing pattern. Reorder these keys to maintain
alphabetical order with the other keys to improve readability and make future
diffs easier to scan.
| export { calcDraftTime } from "./calcDraftTime"; | ||
| export { getResponses } from "./getResponses"; | ||
| export { deletionCoefficient } from "./constants"; | ||
| export { checkUserInclusive } from "./checkUserInclusive"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Alphabetise export list
For quicker look-ups the exports are roughly alphabetical. checkUserInclusive should be placed under calcWeekendMinutes to preserve that ordering.
🤖 Prompt for AI Agents
In src/converters/utils/calculations/index.ts at line 14, the export statement
for checkUserInclusive is not in alphabetical order. Move the export of
checkUserInclusive so that it appears after calcWeekendMinutes to maintain the
alphabetical ordering of exports for easier look-ups.
| convertToDraftTimelineEvent, | ||
| } from "./constants"; | ||
| import { getPullRequestSize } from "./utils/calculations"; | ||
| import { checkUserInclusive, getPullRequestSize } from "./utils/calculations"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Import now pulls two heavy modules every call-site
getPullRequestSize is used rarely whereas checkUserInclusive is hot-path inside the main loop.
Consider splitting the import to avoid bundling unused code when only one helper is required.
🤖 Prompt for AI Agents
In src/converters/collectData.ts at line 25, the import statement brings in both
checkUserInclusive and getPullRequestSize together, causing unnecessary bundling
of the heavier getPullRequestSize module even when only checkUserInclusive is
frequently used. To fix this, split the import so that checkUserInclusive is
imported separately and getPullRequestSize is imported only where needed,
reducing the bundle size and improving performance.
| if (!checkUserInclusive(pullRequestInfo?.user?.login || invalidUserLogin)) { | ||
| return collection; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Early-exit returns the original object reference – risk of silent mutation later
return collection leaves the same mutable instance in circulation, whereas the normal path clones via { ...collection }.
Downstream code might mutate the object assuming a fresh copy. Align behaviour by returning a shallow clone as well:
- if (!checkUserInclusive(pullRequestInfo?.user?.login || invalidUserLogin)) {
- return collection;
- }
+ if (!checkUserInclusive(pullRequestInfo?.user?.login || invalidUserLogin)) {
+ // maintain API symmetry with the non-skipped branch
+ return { ...collection };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!checkUserInclusive(pullRequestInfo?.user?.login || invalidUserLogin)) { | |
| return collection; | |
| } | |
| if (!checkUserInclusive(pullRequestInfo?.user?.login || invalidUserLogin)) { | |
| // maintain API symmetry with the non-skipped branch | |
| return { ...collection }; | |
| } |
🤖 Prompt for AI Agents
In src/converters/utils/preparePullRequestTimeline.ts around lines 23 to 25, the
early return returns the original collection object, which is mutable and can
lead to silent mutations downstream. To fix this, return a shallow clone of the
collection object instead of the original by using object spread syntax,
ensuring consistent behavior with the normal path that returns a cloned object.
| const firstReview = pullRequestReviews?.find( | ||
| (review) => review.user?.login !== pullRequestInfo?.user?.login | ||
| (review) => | ||
| review.user?.login !== pullRequestInfo?.user?.login && | ||
| checkUserInclusive(review.user?.login || invalidUserLogin) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor efficiency – avoid repeated checkUserInclusive inside .find
checkUserInclusive is invoked once per review during the search.
Compute it once per iteration:
- const firstReview = pullRequestReviews?.find(
- (review) =>
- review.user?.login !== pullRequestInfo?.user?.login &&
- checkUserInclusive(review.user?.login || invalidUserLogin)
- );
+ const firstReview = pullRequestReviews?.find((review) => {
+ const reviewer = review.user?.login || invalidUserLogin;
+ return reviewer !== pullRequestInfo?.user?.login && checkUserInclusive(reviewer);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const firstReview = pullRequestReviews?.find( | |
| (review) => review.user?.login !== pullRequestInfo?.user?.login | |
| (review) => | |
| review.user?.login !== pullRequestInfo?.user?.login && | |
| checkUserInclusive(review.user?.login || invalidUserLogin) | |
| ); | |
| const firstReview = pullRequestReviews?.find((review) => { | |
| const reviewer = review.user?.login || invalidUserLogin; | |
| return reviewer !== pullRequestInfo?.user?.login && checkUserInclusive(reviewer); | |
| }); |
🤖 Prompt for AI Agents
In src/converters/utils/preparePullRequestTimeline.ts around lines 26 to 30, the
checkUserInclusive function is called repeatedly inside the find callback for
each review. To improve efficiency, assign the result of
checkUserInclusive(review.user?.login || invalidUserLogin) to a variable within
the iteration and then use that variable in the conditional check. This avoids
redundant calls to checkUserInclusive for the same review.
| if (!checkUserInclusive(user)) { | ||
| return acc; | ||
| } | ||
| return { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid costly accumulator spread inside .reduce
Each branch spreads acc, yielding O(n²) behaviour. Mutate in place instead:
- return {
- ...acc,
- [user]: [...(acc?.[user] || []), [(event as any)?.created_at]],
- };
+ (acc[user] ||= []).push([(event as any)?.created_at]);
+ return acc;Same pattern applies to the other two branches.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!checkUserInclusive(user)) { | |
| return acc; | |
| } | |
| return { | |
| if (!checkUserInclusive(user)) { | |
| return acc; | |
| } | |
| (acc[user] ||= []).push([(event as any)?.created_at]); | |
| return acc; |
🤖 Prompt for AI Agents
In src/converters/utils/calculations/getResponses.ts around lines 13 to 16, the
accumulator object is being spread inside the reduce function, causing O(n²)
complexity. To fix this, avoid spreading the accumulator and instead mutate it
directly in place before returning it. Apply this in-place mutation approach
consistently to all similar branches within the reduce function to improve
performance.
| if ( | ||
| getMultipleValuesInput("EXCLUDE_USERS").length === 0 && | ||
| getMultipleValuesInput("INCLUDE_USERS").length === 0 | ||
| ) { | ||
| return true; | ||
| } | ||
| if ( | ||
| getMultipleValuesInput("EXCLUDE_USERS").length > 0 && | ||
| getMultipleValuesInput("EXCLUDE_USERS").includes(name) | ||
| ) { | ||
| return false; | ||
| } | ||
| return getMultipleValuesInput("INCLUDE_USERS").length > 0 | ||
| ? getMultipleValuesInput("INCLUDE_USERS").includes(name) | ||
| : true; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid repeated ENV parsing & cut redundant look-ups
getMultipleValuesInput is invoked up to six times on every call, parsing the same two variables each time.
This is on the hot-path now used throughout all aggregation utilities, so the overhead will be noticeable when thousands of events are processed.
-export const checkUserInclusive = (name: string) => {
- if (
- getMultipleValuesInput("EXCLUDE_USERS").length === 0 &&
- getMultipleValuesInput("INCLUDE_USERS").length === 0
- ) {
- return true;
- }
- if (
- getMultipleValuesInput("EXCLUDE_USERS").length > 0 &&
- getMultipleValuesInput("EXCLUDE_USERS").includes(name)
- ) {
- return false;
- }
- return getMultipleValuesInput("INCLUDE_USERS").length > 0
- ? getMultipleValuesInput("INCLUDE_USERS").includes(name)
- : true;
-};
+// cache lists once – closure keeps them immutable
+export const checkUserInclusive = (() => {
+ const exclude = getMultipleValuesInput("EXCLUDE_USERS");
+ const include = getMultipleValuesInput("INCLUDE_USERS");
+
+ return (name: string): boolean => {
+ if (exclude.length === 0 && include.length === 0) return true;
+ if (exclude.length && exclude.includes(name)) return false;
+ return include.length ? include.includes(name) : true;
+ };
+})();Cuts down duplicated parsing, keeps semantics intact.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| getMultipleValuesInput("EXCLUDE_USERS").length === 0 && | |
| getMultipleValuesInput("INCLUDE_USERS").length === 0 | |
| ) { | |
| return true; | |
| } | |
| if ( | |
| getMultipleValuesInput("EXCLUDE_USERS").length > 0 && | |
| getMultipleValuesInput("EXCLUDE_USERS").includes(name) | |
| ) { | |
| return false; | |
| } | |
| return getMultipleValuesInput("INCLUDE_USERS").length > 0 | |
| ? getMultipleValuesInput("INCLUDE_USERS").includes(name) | |
| : true; | |
| // cache lists once – closure keeps them immutable | |
| export const checkUserInclusive = (() => { | |
| const exclude = getMultipleValuesInput("EXCLUDE_USERS"); | |
| const include = getMultipleValuesInput("INCLUDE_USERS"); | |
| return (name: string): boolean => { | |
| if (exclude.length === 0 && include.length === 0) return true; | |
| if (exclude.length && exclude.includes(name)) return false; | |
| return include.length ? include.includes(name) : true; | |
| }; | |
| })(); |
🤖 Prompt for AI Agents
In src/converters/utils/calculations/checkUserInclusive.ts lines 3 to 17, the
function calls getMultipleValuesInput multiple times for the same environment
variables, causing redundant parsing and performance overhead. To fix this,
store the results of getMultipleValuesInput("EXCLUDE_USERS") and
getMultipleValuesInput("INCLUDE_USERS") in local variables once at the start of
the function, then use these variables throughout the logic to avoid repeated
look-ups and maintain the same behavior.
| if (checkUserInclusive(userKey)) { | ||
| set( | ||
| collection, | ||
| [userKey, key, "timeWaitingForRepeatedReview"], | ||
| [ | ||
| ...get( | ||
| collection, | ||
| [userKey, key, "timeWaitingForRepeatedReview"], | ||
| [] | ||
| ), | ||
| ...awaitingResponse, | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
total & team aggregates disappear when INCLUDE_USERS is set
checkUserInclusive is called on userKey, which can be "total" or a team name.
These pseudo-users will never be present in INCLUDE_USERS, so the new guard silently skips writing aggregated stats once any inclusion list is provided.
Result: the report loses overall / per-team numbers – only per-user lines survive.
Quick fix:
- if (checkUserInclusive(userKey)) {
+ if (userKey === "total" || (teams[user] || []).includes(userKey) || checkUserInclusive(userKey)) {
set(
collection,Apply the same condition in the block starting at line 101.
Without this, the new feature breaks existing summary rows.
Also applies to: 101-146
🤖 Prompt for AI Agents
In src/converters/utils/prepareResponseTime.ts between lines 47-60 and 101-146,
the checkUserInclusive function is used on userKey, which can be "total" or a
team name, causing aggregated stats to be skipped when INCLUDE_USERS is set. To
fix this, modify the condition to also allow these pseudo-users ("total" and
team names) to pass the guard so that overall and per-team aggregates are
preserved. Apply this updated condition in both code blocks to ensure summary
rows are not lost.
| const reviewComments = comments[index]?.filter( | ||
| (comment) => pullRequestLogin !== comment.user?.login | ||
| (comment) => | ||
| pullRequestLogin !== (comment.user?.login || invalidUserLogin) && | ||
| checkUserInclusive(comment.user?.login || invalidUserLogin) | ||
| ); | ||
|
|
||
| const discussions = comments[index]?.filter((comment) => { | ||
| const userLogin = comment.user?.login || invalidUserLogin; | ||
| return !comment.in_reply_to_id && pullRequestLogin !== userLogin; | ||
| return ( | ||
| !comment.in_reply_to_id && | ||
| pullRequestLogin !== userLogin && | ||
| checkUserInclusive(userLogin) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: double evaluation of checkUserInclusive
checkUserInclusive is executed in both the reviewComments and discussions filters.
Given each comment is iterated only once, this is fine, but you can cache the login’s inclusion result inside the callback to avoid two extra look-ups per comment.
🤖 Prompt for AI Agents
In src/converters/utils/prepareDiscussions.ts around lines 18 to 31, the
function checkUserInclusive is called twice for each comment in separate
filters, causing redundant evaluations. Refactor the code to call
checkUserInclusive once per comment by caching its result in a variable inside
the filter callback, then reuse that cached value in the conditional checks for
both reviewComments and discussions filters to avoid extra look-ups.
| ? reviews?.filter((review) => { | ||
| const userLogin = review.user?.login || invalidUserLogin; | ||
| return userLogin === user; | ||
| return userLogin === user && checkUserInclusive(userLogin); | ||
| }) | ||
| : reviews; | ||
| : reviews?.filter((review) => { | ||
| const userLogin = review.user?.login || invalidUserLogin; | ||
| return checkUserInclusive(userLogin); | ||
| }); | ||
| [dateKey, "total"].forEach((key) => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Micro-perf: duplicate predicate
Both filter branches compute checkUserInclusive(userLogin).
You can DRY by extracting the predicate once; improves readability.
🤖 Prompt for AI Agents
In src/converters/utils/prepareReviews.ts around lines 37 to 45, the filter
logic duplicates the predicate checkUserInclusive(userLogin) in both branches.
Refactor by extracting the predicate function or the userLogin computation
outside the conditional, then apply a single filter call that uses the combined
condition to avoid duplication and improve readability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/converters/utils/preparePullRequestTimeline.ts (2)
23-25: Early-exit still leaks the original mutable object
Previous feedback already highlighted the risk of returning the samecollectionreference. The code path is unchanged and therefore the risk persists. Return a shallow clone to preserve immutability.
26-30: RedundantcheckUserInclusivecall inside.find
Each iteration recomputescheckUserInclusive. Caching the result per review (as suggested earlier) avoids unnecessary work and keeps the predicate readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
build/index.jsis excluded by!build/**and included by none
📒 Files selected for processing (1)
src/converters/utils/preparePullRequestTimeline.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/converters/utils/preparePullRequestTimeline.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/converters/constants.ts (1)
invalidUserLogin(1-1)
| unreviewed: | ||
| timeToReview !== null | ||
| ? collection?.unreviewed || 0 | ||
| : (collection?.unreviewed || 0) + 1, | ||
| unapproved: | ||
| timeToApprove !== null | ||
| ? collection?.unapproved || 0 | ||
| : (collection?.unapproved || 0) + 1, |
There was a problem hiding this comment.
Incorrect null-check increments ‘unreviewed’ / ‘unapproved’ counters
timeToReview and timeToApprove can be undefined, yet the condition only guards against null.
When they are undefined, the counters falsely treat the PR as reviewed/approved.
Fix:
- unreviewed:
- timeToReview !== null
+ unreviewed:
+ typeof timeToReview === "number"
? collection?.unreviewed || 0
: (collection?.unreviewed || 0) + 1,
- unapproved:
- timeToApprove !== null
+ unapproved:
+ typeof timeToApprove === "number"
? collection?.unapproved || 0
: (collection?.unapproved || 0) + 1,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unreviewed: | |
| timeToReview !== null | |
| ? collection?.unreviewed || 0 | |
| : (collection?.unreviewed || 0) + 1, | |
| unapproved: | |
| timeToApprove !== null | |
| ? collection?.unapproved || 0 | |
| : (collection?.unapproved || 0) + 1, | |
| unreviewed: | |
| typeof timeToReview === "number" | |
| ? collection?.unreviewed || 0 | |
| : (collection?.unreviewed || 0) + 1, | |
| unapproved: | |
| typeof timeToApprove === "number" | |
| ? collection?.unapproved || 0 | |
| : (collection?.unapproved || 0) + 1, |
🤖 Prompt for AI Agents
In src/converters/utils/preparePullRequestTimeline.ts around lines 119 to 126,
the conditions checking timeToReview and timeToApprove only guard against null
but not undefined, causing incorrect increments of unreviewed and unapproved
counters. Update the conditions to check for both null and undefined (e.g.,
using a nullish check or explicit undefined check) so that the counters
increment correctly when these values are either null or undefined.
Pull Request
Description
Excludes data of mentioned users from report. Or allows you to include only data from mentioned users
Fixed unreviewed and unapproved PRs calculations
Related Issue(s)
Type of Change
How Has This Been Tested?
Run action on different periods
Checklist:
Summary by CodeRabbit