[jsweep] Clean update_discussion.cjs - #42649
Conversation
- Modernize fetchLabelNodeIds: replace imperative for-loop with flatMap + filter - Add 7 new edge-case tests covering: - Non-discussion context (issues/PRs) rejection - Invalid/negative discussion numbers in target - Existing labels removal before adding new ones - Empty existing labels (no remove mutation) - Unmatched label warning while continuing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42649 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR cleans up the actions/setup/js/update_discussion.cjs github-script helper as part of the jsweep daily maintenance run, and expands its Vitest suite with additional edge-case coverage around discussion targeting and label replacement behavior.
Changes:
- Refactors
fetchLabelNodeIdsto useflatMap+filterinstead of an imperative loop while preserving behavior. - Adds new tests for invalid/unsupported triggering contexts and invalid
targetdiscussion numbers. - Adds tests for label replacement behavior, including removing existing labels and warning on unknown labels.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_discussion.cjs | Refactors label ID lookup logic (fetchLabelNodeIds) to be more declarative while keeping warning/return behavior consistent. |
| actions/setup/js/update_discussion.test.cjs | Adds new edge-case tests for non-discussion triggering contexts, invalid targets, and label replacement scenarios. |
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: 1
- Review effort level: Low
| // Remove mutation should have been called with existing label IDs | ||
| const removeCalls = graphqlCalls.filter(c => c.query.includes("removeLabelsFromLabelable")); | ||
| expect(removeCalls).toHaveLength(1); | ||
| expect(removeCalls[0].variables.labelIds).toEqual(["LA_kwDO_old1", "LA_kwDO_old2"]); | ||
|
|
||
| // Add mutation should have been called with new label ID | ||
| const addCalls = graphqlCalls.filter(c => c.query.includes("addLabelsToLabelable")); | ||
| expect(addCalls).toHaveLength(1); | ||
| expect(addCalls[0].variables.labelIds).toEqual(["LA_kwDO_bug"]); | ||
| }); |
github-actions
Bot
added
the
closed:ci-failure
🧪 Test Quality Sentinel Report
📊 Metrics (40 tests)
|
![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 /codebase-design and /tdd — approving with two minor observations.
📋 Key Themes & Highlights
Observations
- Predicate inconsistency (
update_discussion.cjsline 47):flatMapuses truthiness while theunmatchedfilter useshas()— semantically inconsistent for falsy-but-present IDs. No runtime impact today, but worth aligning. - Fragile mock routing (
update_discussion.test.cjsline 903): query routing byincludes(...)string matching is brittle if query wording changes. Inline comments or shared constants would make the contract explicit.
Positive Highlights
- ✅ Clean single-responsibility refactor —
fetchLabelNodeIdsis now visibly declarative - ✅ Strong test expansion: 7 new edge-case tests, all meaningful (non-discussion fallback, invalid numbers, missing labels, remove-before-add)
- ✅ All 41 tests pass; formatting, linting, and type checking clean
- ✅ PR description is thorough and matches the diff exactly
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 70.5 AIC · ⌖ 7.97 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
| return id ? [id] : []; | ||
| }); | ||
| const unmatched = labelNames.filter(name => !labelMap.has(name.toLowerCase())); |
There was a problem hiding this comment.
[/codebase-design] flatMap filters by truthiness (id ? ...) while unmatched uses labelMap.has() — these predicates are inconsistent for a falsy-but-present value (e.g. an empty-string ID).
💡 Suggested fix
Align both sides on the same undefined check:
const labelIds = labelNames.flatMap(name => {
const id = labelMap.get(name.toLowerCase());
return id !== undefined ? [id] : [];
});
const unmatched = labelNames.filter(
name => labelMap.get(name.toLowerCase()) === undefined
);GitHub label IDs are always non-empty strings today, so this has no runtime impact — but aligning the predicate removes a silent assumption that future readers must re-verify.
@copilot please address this.
| mockGithub.graphql = async (/** @type {string} */ query, /** @type {any} */ variables) => { | ||
| graphqlCalls.push({ query, variables }); | ||
|
|
||
| if (query.includes("discussion(number:")) { |
There was a problem hiding this comment.
[/tdd] The graphql mock routes queries by query.includes(...) string matching — if any query wording changes, this silently routes to the wrong branch with no test failure to signal the mismatch.
💡 Suggested hardening
A renamed or reworded query string can slip past the includes guard and fall through to the throw — but only if no earlier branch accidentally matches. Add a short comment on each branch documenting the expected query operation so the intent is explicit:
// Query: fetchDiscussion — fetches body/labels by discussion number
if (query.includes("discussion(number:")) { ... }
// Query: fetchAllRepoLabels — paginates label list
if (query.includes("labels(first: 100") && query.includes("repository(owner:")) { ... }
// Query: fetchDiscussionLabels — fetches existing labels for a node
if (query.includes("node(id:") && query.includes("on Discussion")) { ... }Alternatively, consider extracting query strings into named constants shared between the implementation and the mock, so a rename breaks both in the same commit.
@copilot please address this.
Summary
Cleans
actions/setup/js/update_discussion.cjsas part of the daily jsweep unbloat run.Context
Execution context: github-script (uses
core,github,contextglobals)Changes
update_discussion.cjsfetchLabelNodeIds: replaced the imperativeforloop that builtlabelIdsandunmatchedarrays withflatMap+filter, making the intent clearer and more idiomatic.@ts-nocheckwas present — file already uses@ts-check.Test Improvements
Added 7 new edge-case tests (41 total, up from 34):
non-discussion context fallback(3 new tests)existing labels removal before adding new ones(4 new tests)removeLabelsFromLabelableis called with correct IDs)Validation
✅ Formatting:
npm run format:cjs— all files unchanged✅ Linting:
npm run lint:cjs— all files use Prettier code style✅ Type checking:
npm run typecheck— no errors inupdate_discussion.cjs(pre-existing unrelated error incopilot_harness.cjs)✅ Tests:
npm run test:js— all 41 tests passWarning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.