Feature: Configure approvals number #71
Conversation
WalkthroughThe changes update analytics event payloads to include counts and raw values for user inclusion/exclusion and required approvals. The approval calculation logic now dynamically uses the required approvals parameter, determining approval status and timestamp based on this value. Configuration reporting also now includes the required approvals parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getApproveTime
participant getValueAsIs
Caller->>getApproveTime: Call with PR reviews and requiredApprovals
getApproveTime->>getApproveTime: Count approvals, compare to requiredApprovals
getApproveTime-->>Caller: Return approval status and timestamp (Nth approval)
Assessment against linked issues
Possibly related PRs
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: 3
🔭 Outside diff range comments (1)
src/converters/utils/calculations/getApproveTime.ts (1)
22-27: Add error handling for invalid REQUIRED_APPROVALS values.The current implementation doesn't handle cases where
REQUIRED_APPROVALSis undefined, null, or contains invalid data. This could lead toNaNvalues breaking the approval logic.- const isApproved = - statusesEntries.filter((user) => acc[user].state === "approved") - .length >= parseInt(getValueAsIs("REQUIRED_APPROVALS")) && + const requiredApprovals = Number.parseInt(getValueAsIs("REQUIRED_APPROVALS") || "1", 10); + const isApproved = + statusesEntries.filter((user) => acc[user].state === "approved") + .length >= requiredApprovals &&
📜 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 (3)
src/analytics/sendActionRun.ts(1 hunks)src/converters/utils/calculations/getApproveTime.ts(3 hunks)src/view/utils/createConfigParamsCode.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/analytics/sendActionRun.ts (1)
src/common/utils/index.ts (2)
getMultipleValuesInput(4-4)getValueAsIs(2-2)
src/converters/utils/calculations/getApproveTime.ts (2)
src/converters/utils/calculations/checkUserInclusive.ts (1)
checkUserInclusive(2-18)src/common/utils/index.ts (1)
getValueAsIs(2-2)
🪛 Biome (1.9.4)
src/converters/utils/calculations/getApproveTime.ts
[error] 23-23: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 48-48: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 54-54: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (2)
src/view/utils/createConfigParamsCode.ts (1)
22-22: LGTM! Clean integration of the new parameter.The addition of
"REQUIRED_APPROVALS"to the configuration parameters list is consistent with the existing pattern and correctly integrates the new feature into the report settings output.src/analytics/sendActionRun.ts (1)
32-36: LGTM! Consistent analytics tracking implementation.The new analytics properties follow the established patterns:
INCLUDE_USERSandEXCLUDE_USERScorrectly track array lengthsREQUIRED_APPROVALSappropriately captures the raw configuration valueThis provides good visibility into how the new feature is being used.
| const isApproved = | ||
| statuses.some((status) => status.state === "approved") && | ||
| statuses.filter((status) => status.state === "approved").length >= | ||
| parseInt(getValueAsIs("REQUIRED_APPROVALS")) && | ||
| !statuses.some((status) => status.state === "changes_requested"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix parseInt usage and add error handling.
Static analysis correctly identifies that Number.parseInt should be used instead of global parseInt. Also add proper error handling for invalid values.
const isApproved =
statuses.filter((status) => status.state === "approved").length >=
- parseInt(getValueAsIs("REQUIRED_APPROVALS")) &&
+ Number.parseInt(getValueAsIs("REQUIRED_APPROVALS") || "1", 10) &&
!statuses.some((status) => status.state === "changes_requested");🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🤖 Prompt for AI Agents
In src/converters/utils/calculations/getApproveTime.ts around lines 46 to 49,
replace the global parseInt with Number.parseInt for better clarity and
consistency. Additionally, add error handling to check if the parsed value is a
valid number before using it in the comparison. If the value is invalid, handle
the error appropriately, such as by setting a default value or throwing an
error.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/converters/utils/calculations/getApproveTime.spec.ts (1)
135-175: LGTM! Consider adding tests for different required approval counts.The test updates correctly match the new function signature. However, since the PR introduces configurable approval requirements, consider adding test cases that verify the behavior with different
requiredApprovalsvalues (e.g., 2, 3).Consider adding tests like:
+ it("Check PR with 2 approvals required - should return null with only 1 approval", () => { + expect(getApproveTime(approvedReview, 2)).toBe(null); + }); + it("Check PR with 2 approvals required - should return second approval time", () => { + expect(getApproveTime(approvedTwiceReview, 2)).toBe("2024-01-11T09:00:00Z"); + });
♻️ Duplicate comments (1)
src/converters/utils/calculations/getApproveTime.ts (1)
51-56: Add bounds checking for array access.The array indexing could fail if
requiredApprovalsis less than 1 or greater than the number of available approvals. This issue was identified in past reviews and should be addressed.Apply this diff to add safe array access:
+ const approvalIndex = Math.max(0, requiredApprovals - 1); + const sortedStatuses = statuses.sort((a, b) => + isBefore(parseISO(a.submittedAt), parseISO(b.submittedAt)) ? 1 : -1 + ); + return isApproved - ? statuses.sort((a, b) => - isBefore(parseISO(a.submittedAt), parseISO(b.submittedAt)) ? 1 : -1 - )[requiredApprovals - 1]?.submittedAt + ? sortedStatuses[approvalIndex]?.submittedAt : null;
📜 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 (3)
src/converters/utils/calculations/getApproveTime.spec.ts(1 hunks)src/converters/utils/calculations/getApproveTime.ts(3 hunks)src/converters/utils/preparePullRequestTimeline.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/converters/utils/calculations/getApproveTime.spec.ts (1)
src/converters/utils/calculations/getApproveTime.ts (1)
getApproveTime(6-56)
🪛 Biome (1.9.4)
src/converters/utils/preparePullRequestTimeline.ts
[error] 33-33: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (2)
src/converters/utils/calculations/getApproveTime.ts (2)
6-9: LGTM! Parameterization approach improves function design.Moving the configuration parsing to the caller is a good architectural decision that makes the function more testable and reusable.
17-17: LGTM! Logic correctly uses the requiredApprovals parameter.The approval counting logic properly uses the
requiredApprovalsparameter instead of parsing configuration internally.Also applies to: 22-23, 47-48
| const approveTime = getApproveTime( | ||
| pullRequestReviews, | ||
| parseInt(getValueAsIs("REQUIRED_APPROVALS")) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix parseInt usage and add error handling for the config value.
The logic correctly passes the required approvals parameter, but there are two improvements needed:
- Static analysis correctly identifies that
Number.parseIntshould be used - Add error handling for invalid or missing config values
Apply this diff:
const approveTime = getApproveTime(
pullRequestReviews,
- parseInt(getValueAsIs("REQUIRED_APPROVALS"))
+ Number.parseInt(getValueAsIs("REQUIRED_APPROVALS") || "1", 10)
);📝 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 approveTime = getApproveTime( | |
| pullRequestReviews, | |
| parseInt(getValueAsIs("REQUIRED_APPROVALS")) | |
| ); | |
| const approveTime = getApproveTime( | |
| pullRequestReviews, | |
| Number.parseInt(getValueAsIs("REQUIRED_APPROVALS") || "1", 10) | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🤖 Prompt for AI Agents
In src/converters/utils/preparePullRequestTimeline.ts around lines 31 to 34,
replace the use of parseInt with Number.parseInt for better clarity and static
analysis compliance. Additionally, add error handling to check if the config
value retrieved by getValueAsIs("REQUIRED_APPROVALS") is valid and a proper
number before parsing; if invalid or missing, handle the error gracefully, such
as by throwing an informative error or using a default value.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/converters/utils/calculations/getApproveTime.ts (2)
60-64: Fix return logic to return the Nth approval timestamp.The current implementation always returns the first approval timestamp (
[0]), but it should return the timestamp of the Nth approval where N equalsrequiredApprovals. This breaks the expected behavior for multiple required approvals.Based on the test case expecting the 2nd approval timestamp when
requiredApprovals=2, the logic should be:return isApproved ? statuses.sort((a, b) => isBefore(parseISO(a.submittedAt), parseISO(b.submittedAt)) ? 1 : -1 - )[0]?.submittedAt + )[requiredApprovals - 1]?.submittedAt : null;
21-30: Refine early-exit logic in getApproveTime.ts to only trigger on new approvalsThe current
isApprovedcheck (src/converters/utils/calculations/getApproveTime.ts, ~lines 21–30) will short-circuit on any review that isn’t a change request (e.g. comments), causing subsequent change requests or state flips to be ignored. To ensure we never miss a “changes_requested” after the threshold is reached, the early exit should only fire on additional approvals, not on arbitrary review events:• File: src/converters/utils/calculations/getApproveTime.ts
Lines: ~21–30Suggested diff:
- const isApproved = - statusesEntries.filter((u) => acc[u].state === "approved").length >= requiredApprovals && - !statusesEntries.some((u) => acc[u].state === "changes_requested") && - review.state !== "changes_requested"; + const isApproved = + statusesEntries.filter((u) => acc[u].state === "approved").length >= requiredApprovals && + !statusesEntries.some((u) => acc[u].state === "changes_requested") && + review.state === "approved";Additionally, please add unit tests to cover:
- A review switching from approved → changes_requested after the threshold is reached
- A comment or other non-approval event occurring after threshold, followed by a changes_requested
- Multiple reviewers flipping statuses in sequence (approve → request changes → approve → request changes)
📜 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 (2)
src/converters/utils/calculations/getApproveTime.spec.ts(1 hunks)src/converters/utils/calculations/getApproveTime.ts(3 hunks)
🔇 Additional comments (3)
src/converters/utils/calculations/getApproveTime.spec.ts (2)
135-174: LGTM! Test updates correctly maintain existing behavior.All existing test calls have been properly updated to include the new
requiredApprovalsparameter with a value of1, which maintains the original behavior while adapting to the new function signature.
175-180: Well-designed test cases for multiple approval requirements.The new test cases appropriately verify the function's behavior when requiring multiple approvals:
- Testing that the second approval timestamp is returned when 2 approvals are required
- Testing that
nullis returned when requesting more approvals than availableThese test cases ensure the new functionality works as expected.
src/converters/utils/calculations/getApproveTime.ts (1)
7-8: Excellent approach - parameter-based design improves testability.The updated function signature accepting
requiredApprovalsas a parameter is much better than the previous approach of callinggetValueAsIsinternally. This improves testability, performance, and separation of concerns.
| console.log(statuses); | ||
|
|
There was a problem hiding this comment.
Remove debug console.log statements.
Console.log statements should not be included in production code as they can impact performance and clutter logs.
- console.log(statuses);
-- console.log(
- isApproved
- ? statuses.sort((a, b) =>
- isBefore(parseISO(a.submittedAt), parseISO(b.submittedAt)) ? 1 : -1
- )
- : null
- );Also applies to: 53-59
🤖 Prompt for AI Agents
In src/converters/utils/calculations/getApproveTime.ts at lines 46-47 and also
lines 53-59, remove all console.log statements used for debugging. These debug
logs should be eliminated from production code to avoid performance issues and
unnecessary log clutter.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/converters/utils/calculations/getApproveTime.ts (1)
55-55: Fix the approval timestamp logic to return the correct approval time.The current implementation returns the latest approval timestamp (
[0]after descending sort), but it should return the timestamp of the approval that reached the required threshold. For multiple required approvals, this should be the nth approval chronologically.return isApproved ? statuses.sort((a, b) => - isBefore(parseISO(a.submittedAt), parseISO(b.submittedAt)) ? 1 : -1 - )[0]?.submittedAt + isBefore(parseISO(a.submittedAt), parseISO(b.submittedAt)) ? -1 : 1 + )[requiredApprovals - 1]?.submittedAt : null;This change:
- Sorts in ascending chronological order (earliest first)
- Returns the timestamp of the final required approval (when the PR actually became approved)
📜 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/calculations/getApproveTime.ts(3 hunks)
🔇 Additional comments (3)
src/converters/utils/calculations/getApproveTime.ts (3)
6-8: LGTM: Function signature updated correctly.The addition of the
requiredApprovalsparameter enables the configurable approval feature as intended.
22-23: LGTM: Approval counting logic updated correctly.The change from
some()tofilter().length >= requiredApprovalsproperly implements the configurable approval threshold check.
48-49: LGTM: Final approval status check updated correctly.Consistent with the reducer logic, this properly checks if the required number of approvals is met.
| export const getApproveTime = ( | ||
| reviews: Awaited<ReturnType<typeof makeComplexRequest>>["events"][number] | ||
| reviews: Awaited<ReturnType<typeof makeComplexRequest>>["events"][number], | ||
| requiredApprovals: number |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the requiredApprovals parameter.
Consider adding validation to ensure requiredApprovals is a positive integer to prevent unexpected behavior.
export const getApproveTime = (
reviews: Awaited<ReturnType<typeof makeComplexRequest>>["events"][number],
requiredApprovals: number
) => {
+ if (requiredApprovals <= 0 || !Number.isInteger(requiredApprovals)) {
+ throw new Error("requiredApprovals must be a positive integer");
+ }
+
const statuses = Object.values(🤖 Prompt for AI Agents
In src/converters/utils/calculations/getApproveTime.ts at line 8, the
requiredApprovals parameter lacks validation. Add a check to ensure
requiredApprovals is a positive integer before using it, throwing an error or
handling invalid values appropriately to prevent unexpected behavior.
Pull Request
Description
Added new parameter
REQUIRED_APPROVALS. It allows to configure amount of approvals required for PRs.Related Issue(s)
Type of Change
How Has This Been Tested?
Tested it in my project in PRs with 2 or more approvals.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests