[jsweep] Clean resolve_mentions_from_payload.cjs#26809
Merged
Conversation
Extract pushNonBotUser, pushNonBotAssignees, and extractKnownAuthorsFromPayload helpers to eliminate the repetitive switch-case pattern that appeared in 4-5 case blocks. Reduces ~100 lines of repetitive code to clean, tested helpers. Also exports the new helpers for direct testing and reuse. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closed
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors resolve_mentions_from_payload.cjs to reduce repetitive payload parsing logic by extracting helper functions, and adds a dedicated Vitest suite to cover payload-author extraction and mention-resolution behavior.
Changes:
- Extracted helper functions to push non-bot users/assignees and to extract known authors by event type.
- Simplified
resolveAllowedMentionsFromPayloadby delegating payload parsing to the new helper. - Added a new Vitest test file covering the new helpers and key
resolveAllowedMentionsFromPayloadbehaviors.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/resolve_mentions_from_payload.cjs | Extracts reusable helpers for payload author extraction and integrates them into mention resolution. |
| actions/setup/js/resolve_mentions_from_payload.test.cjs | Adds targeted unit tests for the new helpers and the main resolver function. |
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: 3
Comment on lines
+40
to
+47
| function extractKnownAuthorsFromPayload(context) { | ||
| const users = /** @type {string[]} */ []; | ||
| const { eventName, payload } = context; | ||
|
|
||
| switch (eventName) { | ||
| case "issues": | ||
| pushNonBotUser(users, payload.issue?.user); | ||
| pushNonBotAssignees(users, payload.issue?.assignees); |
There was a problem hiding this comment.
extractKnownAuthorsFromPayload assumes context.payload is always defined (const { eventName, payload } = context;), but then dereferences payload.* in every case. Since this helper is now exported, calling it with a partial/undefined context will throw a TypeError. Consider making it resilient by defaulting payload to {} (and optionally early-returning [] if !context or typeof context !== "object").
| break; | ||
|
|
||
| case "workflow_dispatch": | ||
| users.push(context.actor); |
There was a problem hiding this comment.
In the workflow_dispatch case, users.push(context.actor) can push a non-string/undefined value into an array documented as string[]. This can later produce @undefined in fakeText and trigger an unnecessary API lookup, or return undefined when allowTeamMembers is false. Recommend guarding with typeof context.actor === "string" && context.actor.length > 0 (and/or filtering falsy entries before returning).
Suggested change
| users.push(context.actor); | |
| if (typeof context.actor === "string" && context.actor.length > 0) { | |
| users.push(context.actor); | |
| } |
| @@ -0,0 +1,353 @@ | |||
| // @ts-check | |||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | |||
There was a problem hiding this comment.
afterEach is imported from vitest but never used in this test file. Please remove the unused import to keep the test clean and avoid unused-import lint failures if the config tightens.
Suggested change
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | |
| import { describe, it, expect, beforeEach, vi } from "vitest"; |
Contributor
Author
🧪 Test Quality Sentinel ReportTest Quality Score: 75/100
Scoring Breakdown
Test Classification DetailsView all 30 test classifications
Flagged Tests — Requires ReviewNo tests were flagged. All 30 tests verify observable behavioral contracts. On mocking approach: 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: §24548646480
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
✅ Test Quality Sentinel: 75/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 30 new tests verify behavioral contracts; 15 cover error paths and edge cases. No coding-guideline violations detected.
Collaborator
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3a569e35-f41d-4528-b103-c394dc16c9a0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3a569e35-f41d-4528-b103-c394dc16c9a0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3a569e35-f41d-4528-b103-c394dc16c9a0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Addressed all actionable review comments in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
pelikhan
deleted the
jsweep/clean-resolve-mentions-from-payload-f36980010f05d1b7
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleaned
actions/setup/js/resolve_mentions_from_payload.cjsby extracting repetitive helpers and adding comprehensive test coverage.Context type: github-script (uses
core,github,contextglobals)Changes
Code improvements
The switch statement in
resolveAllowedMentionsFromPayloadhad ~100 lines of repetitive user/assignee extraction logic repeated across 4-5 cases. Extracted three focused helpers:pushNonBotUser(users, user)— pushes a non-bot user's login to an array; handles null/undefined safelypushNonBotAssignees(users, assignees)— iterates assignees array, delegating topushNonBotUserextractKnownAuthorsFromPayload(context)— encapsulates the full switch statement cleanlyThe main
resolveAllowedMentionsFromPayloadfunction now delegates extraction toextractKnownAuthorsFromPayloadand is easier to follow. The redundantallowAllMentionsvariable (assigned but never used) was also removed.All three helpers are exported for direct testing and potential reuse.
Test coverage (new file:
resolve_mentions_from_payload.test.cjs)Added 30 tests covering:
pushNonBotUser: regular user, bot user, null, missing loginpushNonBotAssignees: mixed bots/humans, null, empty arrayextractKnownAuthorsFromPayload: all 9 event types (issues, pull_request, pull_request_target, issue_comment, pull_request_review_comment, pull_request_review, discussion, discussion_comment, release, workflow_dispatch) + unknown event fallback + bot filteringresolveAllowedMentionsFromPayload: null guards, disabled mentions, allowContext:false, team members mode, extra authors, max limit, limit warning, error recovery, allowed list configValidation ✅
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js -- --no-file-parallelism✓ (30/30 new tests pass; 4 pre-existing unrelated failures unchanged)Warning
The following domain was blocked by the firewall during workflow execution:
invalid.example.invalidTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.