Skip to content

[code-simplifier] simplify: reduce duplication in discussion comment handlers #42641

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

Merged
pelikhan merged 1 commit into from
Jul 1, 2026
+26 −69
Merged

[code-simplifier] simplify: reduce duplication in discussion comment handlers #42641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 17 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 @@ -274,50 +274,27 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName, invocatio
];
const commentBody = commentParts.join("\n\n");

if (eventName === "discussion") {
// Parse discussion number from special format: "discussion:NUMBER"
if (eventName === "discussion" || eventName === "discussion_comment") {
// Parse discussion number from special format: "discussion:NUMBER" or "discussion_comment:NUMBER:COMMENT_ID"
const discussionNumber = parseInt(endpoint.split(":")[1], 10);
const { id: discussionId } = await getDiscussionId(eventRepo.owner, eventRepo.repo, discussionNumber);

const result = await github.graphql(
`
mutation($dId: ID!, $body: String!) {
addDiscussionComment(input: { discussionId: $dId, body: $body }) {
comment {
id
url
// For discussion_comment events, thread the reply under the triggering comment.
// GitHub Discussions only supports two nesting levels, so resolve the top-level parent node ID.
const replyToId = eventName === "discussion_comment"

@github-actions github-actions Bot Jul 1, 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.

Silent degradation to top-level comment when comment.node_id is absent: if resolveTopLevelDiscussionCommentId returns a falsy value, the new ternary silently posts a top-level comment rather than failing — a behavior change the PR description does not acknowledge.

💡 Details

resolveTopLevelDiscussionCommentId in github_api_helpers.cjs early-returns its input unchanged when that input is falsy (if (!commentNodeId) return commentNodeId). So when eventPayload?.comment?.node_id is absent, replyToId is undefined.

Old behavior: undefined was passed as replyToId to a mutation declaring $replyToId: ID! (non-null required) → GraphQL validation error, loud failure.

New behavior: replyToId is falsy, the ternary at line 286 picks the no-replyTo mutation, and the comment is silently posted as a top-level discussion item with no error or warning.

In practice comment.node_id is always present in real GitHub discussion_comment payloads, so this is unlikely to trigger in the field. But silent success-with-wrong-threading replaces a detectable error, and the PR description's claim of "behavior is unchanged" is inaccurate for this case.

If the degradation is intentional, add a core.warning() here so callers can observe the fallback, and update the PR description accordingly.

? await resolveTopLevelDiscussionCommentId(github, eventPayload?.comment?.node_id)
: null;
Comment on lines +283 to +285

@github-actions github-actions Bot Jul 1, 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.

[/codebase-design] Silent fallback to top-level comment when comment.node_id is missing — this is an undocumented behavior change.

If eventPayload?.comment?.node_id is null/undefined, resolveTopLevelDiscussionCommentId returns null, and the code silently posts a top-level comment. The original code would have passed replyToId: null to the non-nullable $replyToId: ID! mutation, surfacing the missing payload as a GraphQL error.

💡 Recommendation

The silent fallback is likely the better UX, but should be:

  1. Covered by a test asserting top-level posting when comment.node_id is absent
  2. Documented with an inline comment explaining the intentional graceful degradation

Without a test, this is an unverified behavior change.

@copilot please address this.

const mutation = replyToId
? `mutation($dId: ID!, $body: String!, $replyToId: ID!) {
addDiscussionComment(input: { discussionId: $dId, body: $body, replyToId: $replyToId }) {
comment { id url }
}
}
}`,
{ dId: discussionId, body: commentBody }
);

const comment = result.addDiscussionComment.comment;
setCommentOutputs(comment.id, comment.url, eventRepo);
return;
} else if (eventName === "discussion_comment") {
// Parse discussion number from special format: "discussion_comment:NUMBER:COMMENT_ID"
const discussionNumber = parseInt(endpoint.split(":")[1], 10);
const { id: discussionId } = await getDiscussionId(eventRepo.owner, eventRepo.repo, discussionNumber);

// Get the comment node ID to use as the parent for threading.
// GitHub Discussions only supports two nesting levels, so if the triggering comment is
// itself a reply, we resolve the top-level parent's node ID.
const commentNodeId = await resolveTopLevelDiscussionCommentId(github, eventPayload?.comment?.node_id);

const result = await github.graphql(
`
mutation($dId: ID!, $body: String!, $replyToId: ID!) {
addDiscussionComment(input: { discussionId: $dId, body: $body, replyToId: $replyToId }) {
comment {
id
url
}`
: `mutation($dId: ID!, $body: String!) {
addDiscussionComment(input: { discussionId: $dId, body: $body }) {
comment { id url }
}
}
}`,
{ dId: discussionId, body: commentBody, replyToId: commentNodeId }
);

}`;
const result = await github.graphql(mutation, { dId: discussionId, body: commentBody, ...(replyToId ? { replyToId } : {}) });

@github-actions github-actions Bot Jul 1, 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.

[/codebase-design] Coupled ternaries: replyToId gates both the mutation string and the variables spread independently.

If these two checks ever drift out of sync (e.g., mutation uses $replyToId but spread doesn't include it), you'll get a silent GraphQL variable-not-provided error at runtime.

💡 Suggested extraction

Consider extracting a helper that returns both at once:

function buildDiscussionCommentArgs(discussionId, body, replyToId) {
  if (replyToId) {
    return {
      mutation: `mutation($dId: ID!, $body: String!, $replyToId: ID!) {
        addDiscussionComment(input: { discussionId: $dId, body: $body, replyToId: $replyToId }) {
          comment { id url }
        }
      }`,
      variables: { dId: discussionId, body, replyToId },
    };
  }
  return {
    mutation: `mutation($dId: ID!, $body: String!) {
      addDiscussionComment(input: { discussionId: $dId, body: $body }) {
        comment { id url }
      }
    }`,
    variables: { dId: discussionId, body },
  };
}

This pattern is used twice (here and in add_workflow_run_comment.cjs) — sharing it would remove the coupling entirely.

@copilot please address this.

const comment = result.addDiscussionComment.comment;
setCommentOutputs(comment.id, comment.url, eventRepo);
return;
Expand Down
38 changes: 9 additions & 29 deletions actions/setup/js/add_workflow_run_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,23 +84,16 @@ function setCommentOutputs(commentId, commentUrl, eventRepo = context.repo, opti
* @returns {Record<string, any>|null}
*/
function parseObject(value) {
if (!value) {
return null;
}
if (!value) return null;
if (typeof value === "string") {
const trimmed = value.trim();
if (!trimmed) {
return null;
}
if (!trimmed) return null;
try {
const parsed = JSON.parse(trimmed);
if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) {
return parsed;
}
return parsed && typeof parsed === "object" && !Array.isArray(parsed) ? parsed : null;

@github-actions github-actions Bot Jul 1, 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.

[/tdd] parseObject was simplified but has no unit tests — the refactored ternary path is unverified.

The test file (add_workflow_run_comment.test.cjs) has no parseObject describe block. Edge cases like parseObject(0), parseObject(false), parseObject([]), and parseObject('{}') are untested.

💡 Suggested tests 
describe('parseObject()', () => {
  it('returns null for falsy input', () => {
    expect(parseObject(null)).toBeNull();
    expect(parseObject(undefined)).toBeNull();
    expect(parseObject('')).toBeNull();
    expect(parseObject(0)).toBeNull();
    expect(parseObject(false)).toBeNull();
  });
  it('returns null for arrays', () => {
    expect(parseObject([])).toBeNull();
    expect(parseObject('[1,2]')).toBeNull();
  });
  it('returns null for whitespace-only strings', () => {
    expect(parseObject('   ')).toBeNull();
  });
  it('parses valid JSON objects', () => {
    expect(parseObject('{"a":1}')).toEqual({ a: 1 });
  });
  it('returns object references unchanged', () => {
    const obj = { x: 1 };
    expect(parseObject(obj)).toBe(obj);
  });
});

@copilot please address this.

} catch {
return null;
}
return null;
}
if (typeof value === "object" && !Array.isArray(value)) {
return /** @type {Record<string, any>} */ value;
Expand Down Expand Up @@ -396,31 +389,18 @@ function buildCommentBody(eventName, runUrl, workflowNameOverride) {
*/
async function postDiscussionComment(discussionNumber, commentBody, replyToNodeId = null, eventRepo = context.repo) {
const discussionId = await getDiscussionNodeId(discussionNumber, eventRepo);

/** @type {any} */
let result;
if (replyToNodeId) {
result = await github.graphql(
`
mutation($dId: ID!, $body: String!, $replyToId: ID!) {
const mutation = replyToNodeId
? `mutation($dId: ID!, $body: String!, $replyToId: ID!) {
addDiscussionComment(input: { discussionId: $dId, body: $body, replyToId: $replyToId }) {
comment { id url }
}
}`,
{ dId: discussionId, body: commentBody, replyToId: replyToNodeId }
);
} else {
result = await github.graphql(
`
mutation($dId: ID!, $body: String!) {
}`
: `mutation($dId: ID!, $body: String!) {
addDiscussionComment(input: { discussionId: $dId, body: $body }) {
comment { id url }
}
}`,
{ dId: discussionId, body: commentBody }
);
}

}`;
const result = await github.graphql(mutation, { dId: discussionId, body: commentBody, ...(replyToNodeId ? { replyToId: replyToNodeId } : {}) });
Comment on lines +392 to +403

@github-actions github-actions Bot Jul 1, 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.

[/improve-codebase-architecture] Same coupled-ternary pattern as in add_reaction_and_edit_comment.cjs — the mutation string and variables spread are gated by separate replyToNodeId checks.

Since this exact pattern now appears in both files, it's a prime candidate for a shared helper (e.g., buildDiscussionCommentRequest(discussionId, body, replyToId)) to keep the two checks co-located and remove the duplication across files.

@copilot please address this.

const comment = result.addDiscussionComment.comment;
return setCommentOutputs(comment.id, comment.url, eventRepo);
}
Expand Down
Loading