[jsweep] Clean add_labels.cjs#33208
Conversation
- Replace isNaN() with Number.isNaN(Number()) for better type safety - Improve code formatting consistency - Add 2 new test cases for numeric string validation (40 total tests) - All validation checks pass: format ✓, lint ✓, tests ✓ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR modernizes the add_labels safe-output handler’s issue/PR number validation (notably for values coming from the GitHub Actions context.payload) and adds tests for numeric-string and invalid-string payload cases.
Changes:
- Updated unresolved-temp-id error fallback to use nullish coalescing (
??) inadd_labels.cjs. - Replaced
isNaN()validation withNumber.isNaN(Number(...))when deriving the issue/PR number fromcontext.payload. - Added 2 new tests covering numeric-string and non-numeric string values coming from
context.payload.issue.number.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_labels.cjs | Adjusts item number validation logic and minor formatting/fallback handling. |
| actions/setup/js/add_labels.test.cjs | Adds tests for string-based context.payload issue numbers (valid numeric string + invalid string). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/add_labels.cjs:108
itemNumbercan come fromcontext.payloadas a string (as the new tests demonstrate), but it's passed through toissues.addLabels({ issue_number: itemNumber })and returned inresult.numberwithout normalization. That can result inissue_number/numberbeing a string and diverging from theManifestEntry.numbercontract (documented asnumber) and Octokit expectations. Consider coercing once (e.g.,const n = Number(itemNumber)) and validatingNumber.isInteger(n) && n > 0, then use the numericnfor API calls and returned results.
itemNumber = context.payload?.issue?.number ?? context.payload?.pull_request?.number;
}
if (!itemNumber || Number.isNaN(Number(itemNumber))) {
const error = "No issue/PR number available";
core.warning(error);
return { success: false, error };
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
| ); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(addLabelsCalls).toHaveLength(1); |
![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 /tdd (test-driven development) and /zoom-out (architectural consistency) based on this refactor + test enhancement PR.
Key Themes
- Test assertion depth: New tests verify behavior but could validate type coercion more explicitly
- Operator consistency: Mixed use of
??vs||for default values across the file - Boundary case coverage: Missing test for
itemNumber = 0edge case
Positive Highlights
- ✅ Excellent proactive test coverage — 40 comprehensive test cases total
- ✅ Modernized type-safe number validation with
Number.isNaN(Number()) - ✅ Added tests for numeric string handling from context payload
- ✅ Clean formatting improvements throughout
Verdict
This is solid modernization work with strong test coverage. The inline comments are minor suggestions for improved test quality and consistency — not blocking issues. Nice cleanup! 🎯
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 3M
| success: false, | ||
| deferred: true, | ||
| error: resolvedTarget.errorMessage || `Unresolved temporary ID: ${explicitItemNumber}`, | ||
| error: resolvedTarget.errorMessage ?? `Unresolved temporary ID: ${explicitItemNumber}`, |
There was a problem hiding this comment.
[/zoom-out] Inconsistent nullish coalescing operator usage. Line 104 uses || but this line uses ??. These operators have different semantics:
??- nullish coalescing (onlynull/undefined)||- logical OR (any falsy value)
For consistency and correctness with the numeric validation below, consider using ?? throughout:
resolvedTarget.errorMessage ?? `Unresolved temporary ID: ${explicitItemNumber}`This aligns with the pattern of explicit null/undefined checks in this codebase.
| expect(result.labelsAdded[0].length).toBe(64); | ||
| }); | ||
|
|
||
| it("should handle numeric string from context payload correctly", async () => { |
There was a problem hiding this comment.
[/tdd] This test verifies numeric strings are accepted, but doesn't validate that the type coercion actually works. Consider asserting the converted number is used:
expect(addLabelsCalls[0].issue_number).toBe(123); // Verify number, not stringThis ensures Number.isNaN(Number(itemNumber)) correctly converts the string to a number before validation.
| expect(addLabelsCalls).toHaveLength(1); | ||
| }); | ||
|
|
||
| it("should reject invalid non-numeric value from context", async () => { |
There was a problem hiding this comment.
[/tdd] Missing boundary test for itemNumber = 0. GitHub issue #0 doesn't exist, but the current logic would reject it as falsy (!itemNumber).
Consider adding a test case:
it("should reject issue number 0 as invalid", async () => {
mockContext.payload = { issue: { number: 0 } };
const result = await handler({ labels: ["bug"] }, {});
expect(result.success).toBe(false);
expect(result.error).toContain("No issue/PR number available");
});This documents the intended behavior for edge case numeric values.
🧪 Test Quality Sentinel ReportTest Quality Score: 75/100
Test Classification Details
🚨 Test Inflation WarningThe test file grew significantly faster than the production code:
While both new tests provide valuable behavioral coverage, the disproportionate growth suggests that the tests may be more verbose than necessary. The production changes are minimal:
These small changes resulted in 46 lines of test code because:
Recommendation: Consider extracting common setup patterns into shared helper functions to reduce boilerplate in future tests. Analysis SummaryBoth new tests verify behavioral contracts rather than implementation details:
Both tests align with the code change: the switch from Language SupportTests analyzed:
Verdict
Score breakdown:
Why not higher? The score is 75/100 primarily due to:
Both penalties are minor concerns that don't affect the core quality of the tests. 📖 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.
|
Summary
Code modernization and type safety improvements for the
add_labels.cjsmodule, replacing legacyisNaN()withNumber.isNaN(Number())for more precise numeric validation and adding test coverage for numeric string handling.Changes
Modified Files
actions/setup/js/add_labels.cjs(Modified, 7 lines: +5/-2)isNaN(itemNumber)withNumber.isNaN(Number(itemNumber))for better type coercion behaviorisNaN()coerces non-numeric strings to NaN (e.g.,isNaN("abc")returns true)Number.isNaN(Number())provides explicit type conversion and safer validationactions/setup/js/add_labels.test.cjs(Modified, 46 lines: +46/-0)"123"from webhook data)"not-a-number")Number.isNaN(Number())validation correctly handles edge casesImpact
isNaN()type coercionTesting
Technical Context
This is part of a broader code modernization effort (
jsweep) to replace legacy JavaScript validation patterns with modern, type-safe alternatives. The change aligns with best practices for numeric validation in Node.js environments where webhook payloads may contain numeric values as strings.