[jsweep] Clean add_reaction_and_edit_comment.cjs#30756
Conversation
- Extract large switch statement into resolveEventEndpoints() helper - Move validReactions to module-level VALID_REACTIONS constant - Remove redundant if (commentUpdateEndpoint) guard after switch - Add explicit Record<string, string> type for EVENT_TYPE_DESCRIPTIONS - Export resolveEventEndpoints and VALID_REACTIONS for direct testing - Add 9 new tests for resolveEventEndpoints and VALID_REACTIONS (35 → 44) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors add_reaction_and_edit_comment.cjs to reduce complexity and improve testability as part of the jsweep unbloat initiative, while expanding unit test coverage for the new extraction points.
Changes:
- Extracted event-to-endpoint resolution into
resolveEventEndpoints()and exported it for direct testing. - Moved the reaction allowlist to a module-level exported
VALID_REACTIONSconstant and simplifiedmain()accordingly. - Added targeted unit tests for
resolveEventEndpoints()across supported events and forVALID_REACTIONS.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_reaction_and_edit_comment.cjs | Extracts endpoint resolution into a helper, promotes reaction allowlist to module scope, and exports both for testing. |
| actions/setup/js/add_reaction_and_edit_comment.test.cjs | Adds tests for the new exports and verifies endpoint resolution behavior for multiple event types and error cases. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| }; | ||
|
|
||
| /** Valid GitHub reaction types */ | ||
| const VALID_REACTIONS = ["+1", "-1", "laugh", "confused", "heart", "hooray", "rocket", "eyes"]; |
🧪 Test Quality Sentinel ReportTest Quality Score: 77/100
Test Classification DetailsView all 9 test classifications
Scoring Breakdown
Suggestions for ImprovementThe tests are well-structured behavioral tests — they all assert on observable outputs (return values and
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25502775257
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out — this is a focused refactor/cleanup PR.
Key Themes
- Missing failure-path tests: The extracted
resolveEventEndpoints()has 6+ error branches that have no test coverage. The happy-path tests are solid, but the failure paths (missing IDs forissue_comment,pull_request_review_comment,discussion, and missingnode_idfordiscussion_comment) are untested. - Side-effect in resolver:
resolveEventEndpointscallscore.setFailedinternally, coupling it to the GitHub Actions runtime. This is a minor separation-of-concerns issue — the function is testable today (with the existing mock), but a throw-based approach would make it cleaner.
Positive Highlights
- ✅ Clean extraction of the 90-line switch into a named, independently testable function
- ✅
VALID_REACTIONSpromoted to module scope — correct move - ✅ Removal of the now-redundant
if (commentUpdateEndpoint)guard is well-reasoned - ✅ Good JSDoc on the new function including return type annotation
- ✅ Destructured assignment
const { owner, repo } = invocationContext.eventRepois a nice minor improvement
Verdict
Commenting only — no blocking issues. The failure-path test gap is the most actionable suggestion; the side-effect concern is a design preference to consider for future extractions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 3.8M
| expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining(ERR_VALIDATION)); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/zoom-out] The new resolveEventEndpoints() tests cover happy paths and the most common failure cases, but several error branches remain untested:
issue_commentwith missingcommentId→ERR_VALIDATIONissue_commentwith missingissueNumber→ERR_NOT_FOUNDpull_request_review_commentwith missingreviewCommentIdorprNumberdiscussionwith missingdiscussionNumberdiscussion_commentwith missingnode_id
Since the stated goal of extracting this function is to make the logic independently testable, covering these failure paths would complete the picture:
it('returns null and calls setFailed when issue_comment is missing commentId', async () => {
const { resolveEventEndpoints } = await loadModule();
const result = await resolveEventEndpoints('issue_comment', 'owner', 'repo', { issue: { number: 1 } });
expect(result).toBeNull();
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining(ERR_VALIDATION));
});| if (!issueNumber) { | ||
| core.setFailed(`${ERR_NOT_FOUND}: Issue number not found in event payload`); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
[/zoom-out] resolveEventEndpoints calls core.setFailed directly, coupling a pure endpoint-resolver to the GitHub Actions runtime. This makes the function harder to reuse or test without mocking core. Consider letting it throw a typed error and keeping the core.setFailed call in main():
// resolveEventEndpoints throws on invalid input
// main() catches and calls core.setFailed
try {
const endpoints = await resolveEventEndpoints(eventName, owner, repo, payload);
...
} catch (err) {
core.setFailed(err.message);
return;
}Minor point given the existing pattern in the file — feel free to defer.
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/fb12d090-f298-4a07-b4d3-b03e8e2d7d6a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Applied the freeze suggestion from the review — |
Summary
Cleaned
actions/setup/js/add_reaction_and_edit_comment.cjsas part of the jsweep unbloat initiative.Changes
Context: github-script
This file runs in the
github-scriptaction context with access tocore,github, andcontextglobals.Code Improvements
resolveEventEndpoints()helper — The 90-line switch statement inmain()was extracted into a dedicated async function that returns{reactionEndpoint, commentUpdateEndpoint} | null, makingmain()cleaner and the endpoint logic independently testableVALID_REACTIONSconstant — Moved from insidemain()to module scope where it belongs; it never changes between callsif (commentUpdateEndpoint)guard — After the refactored switch,commentUpdateEndpointis always set (all returning paths are handled byresolveEventEndpointsreturningnull), so the guard was unnecessaryRecord<string, string>type annotation toEVENT_TYPE_DESCRIPTIONSfor clarityresolveEventEndpointsandVALID_REACTIONSto enable direct unit testingTest Improvements
Added 9 new tests (35 → 44 total):
resolveEventEndpoints()forissues,pull_request,issue_comment,pull_request_review_comment,discussion,discussion_commenteventsresolveEventEndpoints()returnsnull+ callssetFailedwhen required fields missingresolveEventEndpoints()returnsnullfor unknown event typeVALID_REACTIONSexports the expected listValidation
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js— 44 tests passed ✓