[code-simplifier] simplify: reduce duplication in discussion comment handlers#42641
Conversation
- add_workflow_run_comment.cjs parseObject: eliminate nesting and dead return path by collapsing JSON-parse branch to a ternary - add_workflow_run_comment.cjs postDiscussionComment: remove the let-result intermediate; select mutation and variables via ternary (consistent with add_comment.cjs commentOnDiscussion pattern) - add_reaction_and_edit_comment.cjs addCommentWithWorkflowLink: merge the near-identical discussion / discussion_comment branches into one, deriving replyToId conditionally and choosing the mutation accordingly Behavior is unchanged; no interface modifications. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42641 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). The PR only modifies files in .github/skills/, which are not in the default business logic directories (src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR refactors two GitHub Actions JS helpers to reduce duplicated GraphQL discussion-comment posting logic while preserving existing behavior and outputs in the gh-aw setup actions.
Changes:
- Simplified
parseObjectto use early returns and a single conditional return for parsed JSON objects. - Consolidated discussion comment posting logic in
postDiscussionCommentusing a shared mutation/variables construction. - Merged duplicated
discussionvsdiscussion_commentcomment-posting branches inaddCommentWithWorkflowLink, keeping threaded replies fordiscussion_comment.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_workflow_run_comment.cjs | Simplifies object parsing and consolidates GraphQL discussion comment mutation selection without changing output behavior. |
| actions/setup/js/add_reaction_and_edit_comment.cjs | Deduplicates discussion/discussion_comment comment creation, preserving reply threading via resolveTopLevelDiscussionCommentId. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /improve-codebase-architecture, and /tdd — commenting with observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Silent behavior change (
add_reaction_and_edit_comment.cjs): merging thediscussion/discussion_commentbranches introduces a graceful fallback (nullreplyToId→ top-level post instead of GraphQL error) that is untested and undocumented. - Coupled ternaries:
replyToId/replyToNodeIdgates both the mutation string and the variables spread independently in both files. These should be kept co-located to avoid future drift. - No
parseObjectunit tests: the simplified function has no direct test coverage.
Positive Highlights
- ✅ Net −43 LOC with no logic change — excellent signal-to-noise on a cleanup PR
- ✅ Consistent use of the existing ternary + spread pattern from
add_comment.cjs:commentOnDiscussion - ✅ Dead
return nullafter exhaustive try/catch correctly removed inparseObject - ✅ Tests exist for the main happy paths in both changed functions
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 68 AIC · ⌖ 9.58 AIC · ⊞ 6.6K
Comment /matt to run again
| const replyToId = eventName === "discussion_comment" | ||
| ? await resolveTopLevelDiscussionCommentId(github, eventPayload?.comment?.node_id) | ||
| : null; |
There was a problem hiding this comment.
[/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:
- Covered by a test asserting top-level posting when
comment.node_idis absent - Documented with an inline comment explaining the intentional graceful degradation
Without a test, this is an unverified behavior change.
@copilot please address this.
| ); | ||
|
|
||
| }`; | ||
| const result = await github.graphql(mutation, { dId: discussionId, body: commentBody, ...(replyToId ? { replyToId } : {}) }); |
There was a problem hiding this comment.
[/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 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 } : {}) }); |
There was a problem hiding this comment.
[/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.
| if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { | ||
| return parsed; | ||
| } | ||
| return parsed && typeof parsed === "object" && !Array.isArray(parsed) ? parsed : null; |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 79.3 AIC · ⌖ 7.16 AIC · ⊞ 1.6K
Comment /review to run again
| 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" |
There was a problem hiding this comment.
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.
|
@copilot run pr-finisher skill |
PR finisher run complete.
Actions taken: Applied Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, |
|
🎉 This pull request is included in a new release. Release: |
Summary
Refactors discussion comment posting logic in two Action setup scripts to eliminate duplicated GraphQL mutation code. No behaviour changes.
Changes
actions/setup/js/add_reaction_and_edit_comment.cjsaddCommentWithWorkflowLink: Merges separateif (eventName === "discussion")/else if (eventName === "discussion_comment")branches into a single combined branch.discussionNumberparsing,getDiscussionIdcall, and two nearly-identicalgithub.graphql(...)invocations.replyToIdis computed conditionally (resolveTopLevelDiscussionCommentIdcalled only fordiscussion_commentevents,nullotherwise).replyToIdto the variables object.actions/setup/js/add_workflow_run_comment.cjsparseObject: Collapses multi-line guardifblocks to single-line early returns; removes deadreturn nullunreachable after thetry/catchblock.postDiscussionComment: Replaceslet result; if/elsepattern (two separategithub.graphqlcalls with duplicated mutation fields) with a ternary mutation string and a singlegithub.graphqlcall using spread syntax for conditional variables.Behaviour
No runtime behaviour changes. All GraphQL mutations issued are identical to the pre-refactor code paths.
Files Changed
actions/setup/js/add_reaction_and_edit_comment.cjsactions/setup/js/add_workflow_run_comment.cjs