Skip to content

JS-1606 Fix/reduce issues#6836

Merged
vdiez merged 5 commits intomasterfrom
fix/reduce-issues
Apr 17, 2026
Merged

JS-1606 Fix/reduce issues#6836
vdiez merged 5 commits intomasterfrom
fix/reduce-issues

Conversation

@guillemsarda
Copy link
Copy Markdown
Contributor

Part of introduction exercise

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title Fix/reduce issues JS-1606 Fix/reduce issues Apr 16, 2026
@hashicorp-vault-sonar-prod
Copy link
Copy Markdown

hashicorp-vault-sonar-prod Bot commented Apr 16, 2026

JS-1606

@sonar-review-alpha
Copy link
Copy Markdown
Contributor

sonar-review-alpha Bot commented Apr 16, 2026

Summary

This PR contains three distinct improvements:

  1. CSS metrics modernization: Replaces .sort() with .toSorted() for array sorting, using the ES2023 immutable sorting method instead of mutating the original arrays.

  2. Rule descriptions in README: Updates 5 security rule descriptions to align with current RSPEC naming conventions. Most changes clarify the description (e.g., "Using unencrypted EFS file systems is security-sensitive" → "Amazon EFS file systems should be encrypted"). Note: os-command also has a status change from ✅ to ❌.

  3. Exception handling in WebSocket: Changes RuntimeException to IllegalStateException with a descriptive message when WebSocket connection is interrupted, providing better semantic clarity and debugging information.

What reviewers should know

Where to start: Review the three files separately—they address different concerns with no interdependencies.

Key observations:

  • The .toSorted() changes in CSS metrics won't affect behavior (both sort numerically), but ensure immutability. Verify the Arrays are used in a context where mutation could cause issues.
  • The README updates are descriptive clarifications; cross-check the linked RSPEC entries if you want to verify the new wording is authoritative.
  • The exception type change in BridgeServerImpl is semantically correct (InterruptedException → IllegalStateException), but ensure any calling code catching RuntimeException doesn't need updating.

Status change: The os-command rule shows both a description change and a status change (✅ → ❌) in the README—confirm this is intentional.


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

sonar-review-alpha[bot]

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 16, 2026

Ruling Report

Code no longer flagged (6 issues)

S3403

TypeScript/lib/tsc.js:6973

  6971 |     var indentStrings = ["", "    "];
  6972 |     function getIndentString(level) {
> 6973 |         if (indentStrings[level] === undefined) {
  6974 |             indentStrings[level] = getIndentString(level - 1) + indentStrings[1];
  6975 |         }

react-cloud-music/src/api/utils.js:59

    57 |   };
    58 |   for (let key in transformNames) {
>   59 |     if (elementStyle[transformNames[key]] !== undefined) {
    60 |       return key;
    61 |     }

S1154

ace/lib/ace/mode/xml/dom-parser.js:243

   241 |  *  #unparsedEntityDecl(name, publicId, systemId, notationName) {};
   242 |  */
>  243 | "endDTD,startEntity,endEntity,attributeDecl,elementDecl,externalEntityDecl,internalEntityDecl,resolveEntity,getExternalSubset,notationDecl,unparsedEntityDecl".replace(/\w+/g,function(key){
   244 | 	DOMHandler.prototype[key] = function(){return null}
   245 | })

es5-shim/es5-shim.js:1865

  1863 |                         if (!compliantExecNpcg && match.length > 1) {
  1864 |                             /* eslint-disable no-loop-func */
> 1865 |                             match[0].replace(separator2, function () {
  1866 |                                 for (var i = 1; i < arguments.length - 2; i++) {
  1867 |                                     if (typeof arguments[i] === 'undefined') {

es5-shim/es5-shim.js:1917

  1915 |     var replaceReportsGroupsCorrectly = (function () {
  1916 |         var groups = [];
> 1917 |         'x'.replace(/x(.)?/g, function (match, group) {
  1918 |             pushCall(groups, group);
  1919 |         });

paper.js/src/core/PaperScope.js:89

    87 |             // here: { chrome: true, webkit: false }, Mozilla missing is the
    88 |             // only difference to jQuery.browser
>   89 |             user.replace(
    90 |                 /(opera|chrome|safari|webkit|firefox|msie|trident|atom|node|jsdom)\/?\s*([.\d]+)(?:.*version\/([.\d]+))?(?:.*rv\:v?([.\d]+))?/g,
    91 |                 function(match, n, v1, v2, rv) {

New issues flagged (5 issues)

S6582

TypeScript/lib/tsc.js:44761

  44759 |             table.forEach(function (labelMarker, labelText) {
  44760 |                 var statements = [];
> 44761 |                 if (!outerLoop || (outerLoop.labels && outerLoop.labels.get(labelText))) {
  44762 |                     var label = ts.createIdentifier(labelText);
  44763 |                     statements.push(isBreak ? ts.createBreak(label) : ts.createContinue(label));

desktop/app/src/ui/toolbar/branch-dropdown.tsx:330

   328 |       target instanceof Node &&
   329 |       ((prBadgeElem !== null && prBadgeElem.contains(target)) ||
>  330 |         (rerunDialog !== null && rerunDialog.contains(target)))
   331 |     ) {
   332 |       return

knockout/src/binding/editDetection/arrayToDomNodeChildren.js:89

    87 |             ko.utils.arrayForEach(array, itemAdded);
    88 |         } else {
>   89 |             if (!editScript || (lastMappingResult && lastMappingResult['_countWaitingForRemove'])) {
    90 |                 // Compare the provided array against the previous one
    91 |                 var lastArray = ko.utils.arrayMap(lastMappingResult, function (x) { return x.arrayEntry; }),

paper.js/src/event/Key.js:145

   143 |             // Chrome doesn't fire keypress events for command and alt keys,
   144 |             // so we need to handle this in a way that works across all OSes.
>  145 |             if (key.length > 1 || agent && (agent.chrome && (event.altKey
   146 |                         || agent.mac && event.metaKey
   147 |                         || !agent.mac && event.ctrlKey))) {

paper.js/src/path/PathItem.Boolean.js:442

   440 |                     // This is a new curve, update clearHandles setting.
   441 |                     clearHandles = !curve.hasHandles()
>  442 |                             || clearLookup && clearLookup[getId(curve)];
   443 |                     // Keep track of locations for later curve-time
   444 |                     // renormalization within the curve.
📋 View full report

Code no longer flagged (6)

S3403

S1154

New issues flagged (5)

S6582

@zglicz
Copy link
Copy Markdown
Contributor

zglicz commented Apr 16, 2026

I have a PR to remove the flaky ESlint8 Node 16 job #6837
Once its merged, you can rebase and it should be green

sonar-review-alpha[bot]

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 16, 2026

README Freshness Check

The rules README is out of date.

A fix PR has been created: #6869

Please review and merge it into your branch.

guillemsarda pushed a commit that referenced this pull request Apr 16, 2026
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sonar-review-alpha[bot]

This comment was marked as resolved.

Comment thread packages/analysis/src/css/analysis/metrics.ts Outdated
sonar-review-alpha[bot]

This comment was marked as outdated.

guillemsarda pushed a commit that referenced this pull request Apr 17, 2026
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
We removed the TypeScript rule S4043 warning in the metric.ts file from the CSS analyzer.
Taking as a proxy the file Http.java file in the same directory, we have replaced the RuntimeException class for an IllegalStateException containing a descriptive message.
sonar-review-alpha[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@zglicz zglicz left a comment

Choose a reason for hiding this comment

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

Good job! First one down.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sonar-review-alpha[bot]

This comment was marked as outdated.

@vdiez vdiez enabled auto-merge (squash) April 17, 2026 17:20
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@sonar-review-alpha sonar-review-alpha Bot left a comment

Choose a reason for hiding this comment

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

The CSS metrics and Java exception changes are clean and correct. There is one issue in the README that needs fixing before merge.

🗣️ Give feedback

| [object-alt-content](https://sonarsource.github.io/rspec/#/rspec/S5264/javascript) | "<object>" tags should provide an alternative content | ✅ | | | | |
| [operation-returning-nan](https://sonarsource.github.io/rspec/#/rspec/S3757/javascript) | Arithmetic operations should not result in "NaN" | | | | 💭 | |
| [os-command](https://sonarsource.github.io/rspec/#/rspec/S4721/javascript) | Using shell interpreter when executing OS commands is security-sensitive | ✅ | | | | |
| [os-command](https://sonarsource.github.io/rspec/#/rspec/S4721/javascript) | OS commands should not be executed using a shell interpreter | | | | | ❌ |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The os-command entry has two independent changes bundled together:

  1. Description — updated from "Using shell interpreter when executing OS commands is security-sensitive" to "OS commands should not be executed using a shell interpreter". Fine on its own.
  2. Status — removed from the recommended configuration (✅ dropped) and marked as deprecated (❌ added).

The second change is not backed by the code. packages/analysis/src/jsts/rules/S4721/rule.ts is fully active — there is no deprecated marker in the rule metadata, and there is no CHANGELOG entry for this deprecation. The other rules marked ❌ in this file (e.g. aws-s3-bucket-server-encryption, standard-input) have corresponding deprecation markers in their implementations.

The README table is also flagged as auto-generated, so a manual status change here will be overwritten the next time the generator runs.

If the intent is just a description update, revert the status columns to match the original: in the first column and nothing in the column. If os-command is genuinely being deprecated, the rule implementation and CHANGELOG also need to be updated.

Suggested change
| [os-command](https://sonarsource.github.io/rspec/#/rspec/S4721/javascript) | OS commands should not be executed using a shell interpreter | | | | | |
| [os-command](https://sonarsource.github.io/rspec/#/rspec/S4721/javascript) | OS commands should not be executed using a shell interpreter | | | | | |
  • Mark as noise

@sonarqube-next
Copy link
Copy Markdown

@vdiez vdiez merged commit 7d5dfa9 into master Apr 17, 2026
43 checks passed
@vdiez vdiez deleted the fix/reduce-issues branch April 17, 2026 17:44
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.

3 participants