[code-simplifier] Simplify null check idiom and update stale comment in add_comment.cjs#35118
Conversation
Replace explicit '=== undefined || === null' with idiomatic '== null' for the isLockedByKnownMessageWithoutStatus check. In JavaScript, == null correctly matches both undefined and null without matching 0 or other falsy values. Also update stale comment 'For non-404 errors, fail as before' to 'For all other errors, propagate the failure' now that locked errors are also handled separately above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35118 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (well below the 100-line threshold). |
|
✅ PR Code Quality Reviewer completed the code quality review. No action needed: PR #35118 is already merged. Both changes are correct — error?.status == null is semantically equivalent to the explicit triple-equals checks, and the comment update accurately reflects current control flow. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The only changed file is actions/setup/js/add_comment.cjs (2 additions, 2 deletions). Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
This PR simplifies error classification logic in the add_comment safe-output handler by using an idiomatic null/undefined check, and updates an adjacent comment to better reflect the current control flow introduced in #35064.
Changes:
- Replace an explicit
undefined/nulldisjunction witherror?.status == nullfor the “missing status” lock-detection path. - Update a stale inline comment describing how non-404 errors are handled.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_comment.cjs | Simplifies the “missing status” lock detection check and updates an error-handling comment for accuracy. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| const isHttp423Locked = error?.status === 423; | ||
| const isHttp403WithLockedMessage = error?.status === 403 && normalizedErrorMessage.includes("locked"); | ||
| const isLockedByKnownMessageWithoutStatus = (error?.status === undefined || error?.status === null) && hasKnownLockPhrase; | ||
| const isLockedByKnownMessageWithoutStatus = error?.status == null && hasKnownLockPhrase; |
![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 /zoom-out — changes are correct and well-reasoned. Approving.
📋 Analysis
Semantic Equivalence ✅
error?.status == null is identical in behaviour to (error?.status === undefined || error?.status === null) — the abstract equality check with null matches exactly null and undefined, and nothing else. The simplification is safe.
Comment Update ✅
The updated comment // For all other errors, propagate the failure is more accurate: by this point in the control flow, 404, 423, and 403-locked cases have all been handled, so "non-404" was indeed stale and misleading.
Positive Highlights
- ✅ Idiomatic JavaScript —
== nullis the canonical null/undefined guard - ✅ Comment accurately reflects the current control flow
- ✅ Zero functional change — a pure readability win
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 861.3K
Summary
Simplifies a null-check idiom and refreshes a stale inline comment in
add_comment.cjs.Changes
actions/setup/js/add_comment.cjs(modified — low impact, non-breaking)=== undefined(or equivalent) guard onerror?.statuswith the== nullidiom, which correctly covers bothnullandundefinedin a single expression.Impact
Commit
d699fccsimplify: use == null idiom and update stale comment in add_comment.cjs