Skip to content

Conversation

@ss-vibe-bot
Copy link

@ss-vibe-bot ss-vibe-bot bot commented Dec 2, 2025

This PR fixes the false positive issue reported in https://sonarsource.atlassian.net/browse/JS-131
Generated by Claude Code.

@jdkandersson
Copy link
Contributor

Have a look at the packages/jsts/src/rules/helpers/ for any helpers that could be used instead of re-implementing. Don't duplicate logic that is there, use the helpers instead.

@ss-vibe-bot
Copy link
Author

ss-vibe-bot bot commented Dec 3, 2025

✅ Addressed review feedback in the latest commit.

@ss-vibe-bot ss-vibe-bot bot requested a review from jdkandersson December 3, 2025 16:41
@ss-vibe-bot
Copy link
Author

ss-vibe-bot bot commented Dec 4, 2025

Quality Gate failed Quality Gate failed

Failed conditions
3 New issues

✅ Addressed in the latest commit.

Fixed all 5 SonarQube quality gate issues in rule.ts:

  • typescript:S6582 (line 55): Used optional chain expression instead of nested if
  • typescript:S7744 (line 112): Removed unnecessary empty object spread
  • typescript:S4204 (line 122): Replaced any type with proper type assertion
  • typescript:S100 (line 170): Renamed inline arrow function to checkForStatement
  • typescript:S4325 (line 243): Removed unnecessary type assertion on BreakStatement

@ss-vibe-bot
Copy link
Author

ss-vibe-bot bot commented Dec 4, 2025

Have a look at the packages/jsts/src/rules/helpers/ for any helpers that could be used instead of re-implementing. Don't duplicate logic that is there, use the helpers instead.

✅ Addressed in the latest commit.

Applied isFunctionNode helper from ast.ts in two additional locations where function type checks were being done manually, reducing code duplication.

Comment on lines 173 to 199
{
code: `
// FP: imported variable should not raise issue (JS-131)
// This should NOT raise an issue because 'imported' could be modified elsewhere
import { imported } from 'module';
while (imported) {
doSomething();
}`,
},
{
code: `
// FP: required variable should not raise issue (JS-131)
// This should NOT raise an issue because 'required' could be modified elsewhere
const required = require('module');
while (required) {
doSomething();
}`,
},
{
code: `
// FP: file-scope variable with function call in loop (JS-131)
// This should NOT raise an issue because someFunction() might modify globalVar
let globalVar = true;
while (globalVar) {
someFunction();
}`,
},
Copy link
Contributor

@jdkandersson jdkandersson Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the unit tests, please make sure that all of the cases from the algorithm in the Jira ticket are covered

The rule was raising issues for imported/required variables in loop conditions.
Since imported variables could be modified by external modules, these are false positives.

Added check to skip reporting for variables that come from import statements or require() calls.
This aligns with the principle of avoiding false positives when cross-procedural modifications are possible.

When unable to determine if a variable is modified across procedures, prefer not raising an issue.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@ss-vibe-bot ss-vibe-bot bot force-pushed the fix/JS-131-S2189-local-vars-only branch from eb95ad7 to 2ac2d0e Compare December 9, 2025 08:50
Added test cases for:
- Required variables (variables from require() calls)
- Variables passed as arguments to functions
- Variables used as method receivers

These test cases cover the key scenarios from the algorithm
described in the Jira ticket to reduce false positives.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@ss-vibe-bot
Copy link
Author

ss-vibe-bot bot commented Dec 9, 2025

For the unit tests, please make sure that all of the cases from the algorithm in the Jira ticket are covered

✅ Addressed in the latest commit.

Added test cases covering key scenarios from the algorithm:

  • Imported variables (import statements) - should NOT raise
  • Required variables (require() calls) - should NOT raise
  • Variables passed as arguments - should NOT raise (may be modified cross-procedurally)
  • Variables used as method receivers - should NOT raise (may be modified cross-procedurally)

These tests validate that the implementation correctly avoids false positives for cases where cross-procedural modifications are possible.

@sonarqube-next
Copy link

sonarqube-next bot commented Dec 9, 2025

Quality Gate failed Quality Gate failed

Failed conditions
1 New issue
66.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE SonarQube for IDE

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant