Conversation
- Refactor: tighten try/catch scope - discussion cases now have their own try/catch blocks instead of being wrapped in one giant try/catch - Refactor: extract shared error handling into handleReactionError() helper to eliminate duplicated locked-error logic - Fix: properly type reactionEndpoint as string | undefined with explicit guard instead of an inline JSDoc cast (/** @type {string} */) - Tests: add 7 new test cases covering repository.discussion=null, locked errors for discussion/discussion_comment events Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors the add_reaction github-script to reduce unnecessary try/catch scope, centralize error handling, and improve type-check friendliness while expanding test coverage for discussion-related failure/locked scenarios.
Changes:
- Refactored
add_reaction.cjscontrol flow to use targeted try/catch blocks and introduced a sharedhandleReactionError()helper. - Improved typing for
reactionEndpointand added a guard before REST reaction calls. - Added new tests covering discussion “not found” variants and locked-resource behavior for discussions and discussion comments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| actions/setup/js/add_reaction.cjs | Refactors endpoint selection and error handling; introduces handleReactionError and tighter try/catch scope. |
| actions/setup/js/add_reaction.test.cjs | Adds discussion/discussion_comment test cases for null GraphQL paths and locked-resource behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| core.setFailed(`${ERR_API}: Failed to add reaction: ${errorMessage}`); | ||
| // Add reaction using REST API (for non-discussion events) | ||
| // reactionEndpoint is always defined here - all other cases return early | ||
| if (!reactionEndpoint) return; |
There was a problem hiding this comment.
The if (!reactionEndpoint) return; guard can cause a silent no-op if a new switch branch is added later that forgets to set reactionEndpoint (or changes control flow), making failures harder to detect. Consider failing explicitly here (e.g., core.setFailed with an internal error) so unexpected states don’t pass quietly.
| if (!reactionEndpoint) return; | |
| if (!reactionEndpoint) { | |
| core.setFailed(`${ERR_API}: Internal error: reaction endpoint not set for event: ${eventName}`); | |
| return; | |
| } |
Summary
Cleaned
actions/setup/js/add_reaction.cjsas part of the jsweep JavaScript unbloating effort.Context type: github-script (uses
core,github,contextglobals)Changes
add_reaction.cjsTightened try/catch scope
try/catchwrapping the entireswitchstatement with targetedtry/catchblocks only around the async GraphQL calls in thediscussionanddiscussion_commentcasestry/catchreactionEndpoint) no longer run inside a try/catch unnecessarilyExtracted
handleReactionError()helpercore.setFailedpattern was duplicated (now needed in 3 places after the refactor)handleReactionError(error)functionFixed
reactionEndpointtypinglet reactionEndpoint;(untyped) used with an inline/**@type{string} */cast/**@type{string | undefined} */ let reactionEndpoint;with an explicitif (!reactionEndpoint) return;guard before useadd_reaction.test.cjsAdded 7 new test cases:
should handle discussion not found error when repository is null— existing test renamed for clarityshould handle discussion not found error when discussion is null— NEW: tests{ repository: { discussion: null } }path that was previously uncoveredshould silently ignore locked discussion errors— NEW: locked error handling fordiscussioneventsshould silently ignore locked discussion comment errors— NEW: locked error handling fordiscussion_commenteventsTest count: 23 → 30 tests
Validation ✅
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js -- add_reaction→ 30 tests passed ✓