Skip to content

[code-simplifier] Simplify add_reaction_and_edit_comment.cjs: remove duplicated endpoint/guard logic #52222

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.

Sign up for GitHub

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

Closed
github-actions wants to merge 2 commits into from
+41 −40
Closed

[code-simplifier] Simplify add_reaction_and_edit_comment.cjs: remove duplicated endpoint/guard logic #52222

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31fe619
Simplify add_reaction_and_edit_comment.cjs: dedupe endpoint/guard logic
github-actions[bot] Aug 12, 2026
6df3bf6
Merge branch 'main' into simplify-add-reaction-edit-comment-be7f86d9f…
github-actions[bot] Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 41 additions & 40 deletions actions/setup/js/add_reaction_and_edit_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,34 @@ function expectRestEndpoint(endpoint, endpointName, eventName) {
return endpoint;
}

/**
* Build the "create issue comment" REST endpoint used to post the workflow-link comment
* on the issue or pull request associated with the triggering event.
* @param {string} owner - Repository owner
* @param {string} repo - Repository name
* @param {number} issueNumber - Issue or pull request number
* @returns {RestEndpoint}
*/
function issueCommentEndpoint(owner, repo, issueNumber) {
return { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: issueNumber } };
}

/**
* Verify a required payload field is present, calling core.setFailed with a consistent
* message when it is missing.
* @param {unknown} value - The extracted field value
* @param {string} fieldName - Human-readable field name for the error message
* @param {string} errorCode - Error code prefix (ERR_NOT_FOUND or ERR_VALIDATION)
* @returns {boolean} true if present, false if missing (setFailed already called)
*/
function requirePayloadField(value, fieldName, errorCode) {
if (!value) {
core.setFailed(`${errorCode}: ${fieldName} not found in event payload`);
return false;
}
return true;
}

/**
* Resolve the reaction and comment API endpoints for a given event.
* Returns null (after calling core.setFailed) when the event or payload is invalid.
Expand All @@ -44,71 +72,50 @@ async function resolveEventEndpoints(eventName, owner, repo, payload) {
switch (eventName) {
case "issues": {
const issueNumber = payload?.issue?.number;
if (!issueNumber) {
core.setFailed(`${ERR_NOT_FOUND}: Issue number not found in event payload`);
return null;
}
if (!requirePayloadField(issueNumber, "Issue number", ERR_NOT_FOUND)) return null;
return {
reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner, repo, issue_number: issueNumber } },
commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: issueNumber } },
commentUpdateEndpoint: issueCommentEndpoint(owner, repo, issueNumber),
};
}

case "issue_comment": {
const commentId = payload?.comment?.id;
const issueNumber = payload?.issue?.number;
if (!commentId) {
core.setFailed(`${ERR_VALIDATION}: Comment ID not found in event payload`);
return null;
}
if (!issueNumber) {
core.setFailed(`${ERR_NOT_FOUND}: Issue number not found in event payload`);
return null;
}
if (!requirePayloadField(commentId, "Comment ID", ERR_VALIDATION)) return null;
if (!requirePayloadField(issueNumber, "Issue number", ERR_NOT_FOUND)) return null;
return {
reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", params: { owner, repo, comment_id: commentId } },
// Create new comment on the issue itself, not on the comment
commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: issueNumber } },
commentUpdateEndpoint: issueCommentEndpoint(owner, repo, issueNumber),
};
}

case "pull_request": {
const prNumber = payload?.pull_request?.number;
if (!prNumber) {
core.setFailed(`${ERR_NOT_FOUND}: Pull request number not found in event payload`);
return null;
}
if (!requirePayloadField(prNumber, "Pull request number", ERR_NOT_FOUND)) return null;
// PRs are "issues" for the reactions endpoint
return {
reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner, repo, issue_number: prNumber } },
commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: prNumber } },
commentUpdateEndpoint: issueCommentEndpoint(owner, repo, prNumber),
};
}

case "pull_request_review_comment": {
const reviewCommentId = payload?.comment?.id;
const prNumber = payload?.pull_request?.number;
if (!reviewCommentId) {
core.setFailed(`${ERR_VALIDATION}: Review comment ID not found in event payload`);
return null;
}
if (!prNumber) {
core.setFailed(`${ERR_NOT_FOUND}: Pull request number not found in event payload`);
return null;
}
if (!requirePayloadField(reviewCommentId, "Review comment ID", ERR_VALIDATION)) return null;
if (!requirePayloadField(prNumber, "Pull request number", ERR_NOT_FOUND)) return null;
return {
reactionEndpoint: { route: "POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions", params: { owner, repo, comment_id: reviewCommentId } },
// Create new comment on the PR itself (using issues endpoint since PRs are issues)
commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: prNumber } },
commentUpdateEndpoint: issueCommentEndpoint(owner, repo, prNumber),
};
}

case "discussion": {
const discussionNumber = payload?.discussion?.number;
if (!discussionNumber) {
core.setFailed(`${ERR_NOT_FOUND}: Discussion number not found in event payload`);
return null;
}
if (!requirePayloadField(discussionNumber, "Discussion number", ERR_NOT_FOUND)) return null;
// Discussions use GraphQL API - get the node ID
const discussionNodeId = await getDiscussionNodeId(owner, repo, discussionNumber);
return {
Expand All @@ -120,15 +127,9 @@ async function resolveEventEndpoints(eventName, owner, repo, payload) {
case "discussion_comment": {
const discussionNumber = payload?.discussion?.number;
const commentId = payload?.comment?.id;
if (!discussionNumber || !commentId) {
core.setFailed(`${ERR_NOT_FOUND}: Discussion or comment information not found in event payload`);
return null;
}
if (!requirePayloadField(discussionNumber && commentId, "Discussion or comment information", ERR_NOT_FOUND)) return null;

@github-actions github-actions Bot Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] Correctness regression: requirePayloadField(discussionNumber && commentId, ...) passes a single coerced value instead of two separate checks — the original validated each field independently, giving distinct error messages, but now both fields being absent produces one combined failure that masks which field is actually missing.

💡 Suggested fix

Replace the combined expression with two separate calls, mirroring every other case in the switch:

if (!requirePayloadField(discussionNumber, "Discussion number", ERR_NOT_FOUND)) return null;
if (!requirePayloadField(commentId, "Comment ID", ERR_NOT_FOUND)) return null;

This preserves the original per-field specificity and makes failures diagnosable.

@copilot please address this.

Copilot AI Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No code change is needed: the pre-refactor guard was also combined (if (!discussionNumber || !commentId)) and emitted the same combined error. requirePayloadField(discussionNumber && commentId, "Discussion or comment information", ERR_NOT_FOUND) preserves those exact failure semantics. The targeted test passes.

const commentNodeId = payload?.comment?.node_id;
if (!commentNodeId) {
core.setFailed(`${ERR_NOT_FOUND}: Discussion comment node ID not found in event payload`);
return null;
}
if (!requirePayloadField(commentNodeId, "Discussion comment node ID", ERR_NOT_FOUND)) return null;
return {
reactionEndpoint: commentNodeId, // Store node ID for GraphQL
commentUpdateEndpoint: `discussion_comment:${discussionNumber}:${commentId}`, // Special format
Expand Down
Loading