Skip to content

[code-simplifier] Simplify locked resource error check logic#15977

Closed
github-actions[bot] wants to merge 1 commit intomainfrom
copilot/simplify-locked-check-logic-0935351de8db3d6f
Closed

[code-simplifier] Simplify locked resource error check logic#15977
github-actions[bot] wants to merge 1 commit intomainfrom
copilot/simplify-locked-check-logic-0935351de8db3d6f

Conversation

@github-actions
Copy link
Contributor

Summary

This PR simplifies the boolean logic for checking locked resource errors in the add_reaction handlers, improving code clarity while preserving all functionality.

Files Simplified

  • actions/setup/js/add_reaction.cjs - Simplified locked resource check
  • actions/setup/js/add_reaction_and_edit_comment.cjs - Simplified locked resource check

Improvements Made

Reduced Complexity

  • Simplified redundant boolean expression: (is403Error && hasLockedMessage) || (!is403Error && hasLockedMessage)hasLockedMessage
  • Removed unused is403Error variable
  • Applied boolean algebra: (A && B) || (!A && B) = B

Enhanced Clarity

  • Updated comment to accurately reflect that status code is not checked
  • Reduced conditional logic from 2 branches to 1
  • Made the code intention more explicit

Applied Project Standards

  • Maintained ES module conventions
  • Preserved all error handling behavior
  • Followed existing code patterns

Changes Based On

Recent changes from:

  • Commit ee42fff - "fix: Simplify locked message check in addReaction functions"

This PR further simplifies the logic introduced in that commit.

Testing

  • ✅ All JavaScript tests pass (make test-js)
    • add_reaction.test.cjs: 27 tests passing
    • add_reaction_and_edit_comment.test.cjs: 19 tests passing
  • ✅ Linting passes (make lint-cjs)
  • ✅ Formatting validated (make fmt-cjs)
  • ✅ No functional changes - behavior is identical

Review Focus

Please verify:

  • Boolean simplification is logically correct
  • Functionality is preserved (all tests pass)
  • Comment accurately describes the new behavior
  • No unintended side effects

Diff Summary

-    const is403Error = error && typeof error === "object" && "status" in error && error.status === 403;
     const hasLockedMessage = errorMessage && (errorMessage.includes("locked") || errorMessage.includes("Lock conversation"));

-    // Only ignore the error if it's a 403 AND mentions locked, or if the message mentions locked
-    if ((is403Error && hasLockedMessage) || (!is403Error && hasLockedMessage)) {
-      // Silently ignore locked resource errors - just log for debugging
+    // Silently ignore locked resource errors regardless of status code
+    if (hasLockedMessage) {
       core.info(`Cannot add reaction: resource is locked (this is expected and not an error)`);
       return;
     }

Net change: -4 lines, +2 lines (2 lines removed per file)


References:

AI generated by Code Simplifier

  • expires on Feb 16, 2026, 7:02 PM UTC

Simplify the boolean expression for checking locked resource errors
by removing redundant status code check.

The condition (A && B) || (!A && B) is logically equivalent to just B.

Changes:
- Remove unused is403Error variable
- Simplify conditional from 2 branches to 1
- Update comment to clarify behavior
- Preserve all functionality (tests pass)

Files:
- actions/setup/js/add_reaction.cjs
- actions/setup/js/add_reaction_and_edit_comment.cjs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@pelikhan
Copy link
Contributor

/cloclo it should be 403 && locked

github-actions bot added a commit that referenced this pull request Feb 15, 2026
This fixes the locked resource error check logic to properly require BOTH
a 403 status code AND a locked message before suppressing the error.

The previous logic was incorrectly simplified to ignore any error with a
locked message, regardless of status code. This could mask legitimate
errors that mention "locked" but aren't actually resource-locked errors.

Changes:
- Updated error check from `(is403Error && hasLockedMessage) || (!is403Error && hasLockedMessage)`
  to `is403Error && hasLockedMessage`
- Updated comments to clarify that BOTH conditions are required
- Updated tests to verify non-403 errors with locked messages are not suppressed

Files modified:
- actions/setup/js/add_reaction.cjs
- actions/setup/js/add_reaction.test.cjs
- actions/setup/js/add_reaction_and_edit_comment.cjs
- actions/setup/js/add_reaction_and_edit_comment.test.cjs

Fixes #15977

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor Author

✨ Claude Response via /cloclo

Summary

You're absolutely right! 🎯 The logic should check for BOTH 403 status code AND locked message, not just the locked message alone. I've fixed the error handling logic and created a new PR with the corrected implementation.

Problem Identified

The previous "simplification" was actually incorrect. The boolean expression:

(is403Error && hasLockedMessage) || (!is403Error && hasLockedMessage)

simplifies to just hasLockedMessage, which means any error mentioning "locked" would be suppressed, regardless of status code. This could mask legitimate errors like:

  • 500 errors: "Database locked"
  • 502 errors: "Upstream service locked"
  • Other unexpected errors mentioning "locked"

Solution Implemented ✅

The correct logic requires BOTH conditions:

if (is403Error && hasLockedMessage) {
  // Only suppress 403 errors specifically about locked resources
}

Changes Made 📝

  1. Fixed error check logic in both files:

    • actions/setup/js/add_reaction.cjs
    • actions/setup/js/add_reaction_and_edit_comment.cjs
  2. Updated comments to clarify BOTH conditions are required

  3. Updated tests to verify non-403 errors with locked messages are NOT suppressed:

    • add_reaction.test.cjs
    • add_reaction_and_edit_comment.test.cjs

Test Results ✅

All 46 tests passing:

  • add_reaction.test.cjs: 27 tests passed
  • add_reaction_and_edit_comment.test.cjs: 19 tests passed

Next Steps 🚀

I've created a new pull request with these fixes. The PR includes:

  • Corrected error handling logic
  • Updated test coverage
  • Detailed explanation of why this matters for proper error handling

Merci for catching this logical error! Proper error handling is critical for reliability.

🤖 Generated with [Claude Code]((claude.com/redacted)

🎤 Magnifique! Performance by /cloclo for issue #15977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant