[code-simplifier] refactor(js): derive VALID_REACTIONS from REACTION_MAP to eliminate duplication#43929
Conversation
…ication - add_reaction_and_edit_comment.cjs: remove hardcoded VALID_REACTIONS array and derive it from the exported REACTION_MAP from add_reaction.cjs. This eliminates a duplicate list that could drift out of sync. - add_reaction.cjs: remove transient validReactions local variable; validate directly against REACTION_MAP using hasOwnProperty. Behavior is unchanged — key order is preserved (insertion order matches the original frozen array), and all 105 JS tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43929 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 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 removes duplication in the GitHub Actions JS “reaction” helpers by deriving the set of valid reaction strings from the existing REACTION_MAP, ensuring a single source of truth and reducing drift risk.
Changes:
- Simplified reaction validation in
add_reaction.cjsby checking membership directly againstREACTION_MAP. - Refactored
add_reaction_and_edit_comment.cjsto deriveVALID_REACTIONSfromREACTION_MAPinstead of maintaining a separate hardcoded list.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_reaction.cjs | Uses REACTION_MAP key presence for validation and derives the “valid reactions” list directly from the map for error messaging. |
| actions/setup/js/add_reaction_and_edit_comment.cjs | Removes hardcoded VALID_REACTIONS values and derives them from REACTION_MAP to prevent drift. |
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.
The refactoring is correct and safe. Both changes are semantically equivalent to the originals while eliminating duplication. REACTION_MAP is properly exported and imported, hasOwnProperty.call is a clean validation pattern, and Object.freeze(Object.keys(REACTION_MAP)) preserves the original frozen-array contract. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 20.5 AIC · ⌖ 5.95 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Approved - Clean deduplication with no behavioral changes. Both changes achieve a single source of truth for reaction validation. No correctness, security, or performance issues found.
🔎 Code quality review by PR Code Quality Reviewer · 40.9 AIC · ⌖ 5.08 AIC · ⊞ 5.5K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — two minor suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Style inconsistency:
add_reaction.cjsnow usesObject.prototype.hasOwnProperty.callwhileadd_reaction_and_edit_comment.cjsusesArray.includes.Object.hasOwnwould be the cleaner, consistent choice for the map-based check. - Test still documents old shape: The existing
VALID_REACTIONStest asserts a hard-coded array rather than asserting the derivation relationship introduced by this PR.
Positive Highlights
- ✅ Excellent DRY improvement —
REACTION_MAPis the true source of truth and the PR correctly centralises it. - ✅ Exporting
REACTION_MAPfromadd_reaction.cjsis the right seam; keeps the single source in the module that owns the map. - ✅ 105 tests passing; PR scope is tight and the description is thorough.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 73.3 AIC · ⌖ 6.95 AIC · ⊞ 6.7K
Comment /matt to run again
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for this tidy cleanup from the Code Simplifier! Deriving One small gap to address before merge:
If you'd like a hand, you can assign this prompt to your coding agent: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Refactors reaction validation in two JavaScript CJS modules to derive
VALID_REACTIONSfromREACTION_MAP, eliminating a hardcoded duplicate list.Changes
actions/setup/js/add_reaction.cjsvalidReactionslocal variable.validReactions.includes(reaction)withObject.prototype.hasOwnProperty.call(REACTION_MAP, reaction)— a prototype-safe property lookup.Object.keys(REACTION_MAP).join(", ")directly in the validation error message.actions/setup/js/add_reaction_and_edit_comment.cjsREACTION_MAPfrom./add_reaction.cjs.Impact
REACTION_MAPinadd_reaction.cjsis now the sole authoritative definition; all validation derives from it automatically.Files Changed
actions/setup/js/add_reaction.cjsactions/setup/js/add_reaction_and_edit_comment.cjsCommits
6edbd5457refactor(js): derive VALID_REACTIONS from REACTION_MAP to remove duplication