ronantakizawa/codereview-bench huggingface dataset:
Repository: angular/angular-cli
File: packages/angular/cli/src/commands/mcp/tools/best-practices.ts (TypeScript)

Code before:
  return async (input: BestPracticesInput) => {
    let content: string | undefined;
    let source: string | undefined;

    // First, try to get the version-specific guide.
    if (input.workspacePath) {
      const versionSpecific = await getVersionSpecificBestPractices(input.workspacePath, logger);
      if (versionSpecific) {
        content = versionSpecific.content;
        source = versionSpecific.source;
      }
    }

    // If the version-specific guide was not found for any reason, fall back to the bundled version.
    if (content === undefined) {
      content = await (bundledBestPractices ??= getBundledBestPractices());
      source = `bundled (CLI v${VERSION.full})`;
    }

    return {
      content: [
        {
          type: 'text' as const,
          text: content,
          annotations: {
            audience: ['assistant'],
            priority: 0.9,
            // TODO: Figure out why `source` is not part of the annotations type but added here.
            // @ts-expect-error `source` doesn't appear in the SDK types.
            source,
          },
        },
      ],
    } satisfies CallToolResult;
  };
}


Review comment (suggestion):
```suggestion
            audience: ['assistant' as const],
```
Looks like the audience type is now more specific.

Code after:
    let content: string | undefined;
    let source: string | undefined;

    // First, try to get the version-specific guide.
    if (input.workspacePath) {
      const versionSpecific = await getVersionSpecificBestPractices(input.workspacePath, logger);
      if (versionSpecific) {
        content = versionSpecific.content;
        source = versionSpecific.source;
      }
    }

    // If the version-specific guide was not found for any reason, fall back to the bundled version.
    if (content === undefined) {
      content = await (bundledBestPractices ??= getBundledBestPractices());
      source = `bundled (CLI v${VERSION.full})`;
    }

    return {
      content: [
        {
          type: 'text' as const,
          text: content,
          annotations: {
            audience: ['assistant' as const],
            priority: 0.9,
            source,
          },
        },
      ],
    };
  };
}
Repository: angular/angular.js
File: src/ng/filter/orderBy.js (JavaScript)

Code before:

  function isPrimitive(value) {
    switch (typeof value) {
      case 'number': /* falls through */
      case 'boolean': /* falls through */
      case 'string':
        return true;
      default:
        return false;
    }
  }

  function objectValue(value, index) {
    if (typeof value.valueOf === 'function') {
      value = value.valueOf();
      if (isPrimitive(value)) return value;
    }
    if (hasCustomToString(value)) {
      value = value.toString();
      if (isPrimitive(value)) return value;
    }
    return index;
  }

  function getPredicateValue(value, index) {
    var type = typeof value;
    if (value === null) {
      type = 'string';
      value = 'null';
    } else if (type === 'string') {
      value = value.toLowerCase();
    } else if (type === 'object') {
      value = objectValue(value, index);
    }
    return { value: value, type: type };
  }

  function compare(v1, v2) {
    var result = 0;
    if (v1.type === v2.type) {
      if (v1.value !== v2.value) {
        result = v1.value < v2.value ? -1 : 1;
      }
    } else {
      result = v1.type < v2.type ? -1 : 1;
    }
    return result;
  }
}


Review comment (suggestion):
Actually, this is not a problem. First of all the `orderBy` filter does not mutate the original array. Second if the types of things are different then they are ordered by their type first rather than their value.


Code after:
  }

  function isPrimitive(value) {
    switch (typeof value) {
      case 'number': /* falls through */
      case 'boolean': /* falls through */
      case 'string':
        return true;
      default:
        return false;
    }
  }

  function objectValue(value, index) {
    // If `valueOf` is a valid function use that
    if (typeof value.valueOf === 'function') {
      value = value.valueOf();
      if (isPrimitive(value)) return value;
    }
    // If `toString` is a valid function and not the one from `Object.prototype` use that
    if (hasCustomToString(value)) {
      value = value.toString();
      if (isPrimitive(value)) return value;
    }
    // We have a basic object so we use the position of the object in the collection
    return index;
  }

  function getPredicateValue(value, index) {
    var type = typeof value;
    if (value === null) {
      type = 'string';
      value = 'null';
    } else if (type === 'string') {
      value = value.toLowerCase();
    } else if (type === 'object') {
      value = objectValue(value, index);
    }
    return { value: value, type: type };
  }

  function compare(v1, v2) {
    var result = 0;
    if (v1.type === v2.type) {
      if (v1.value !== v2.value) {
        result = v1.value < v2.value ? -1 : 1;
      }
    } else {
      result = v1.type < v2.type ? -1 : 1;
    }
    return result;
Repository: angular/angular.js
File: src/ng/animateRunner.js (JavaScript)

Code before:
    AnimateRunner.all = function(runners, callback) {
      var count = 0;
      var status = true;
      forEach(runners, function(runner) {
        runner.done(onProgress);
      });

      function onProgress(response) {
        status = status && response;
        if (++count === runners.length) {
          callback(status);
        }
      }
    };

    function AnimateRunner(host) {
      this.setHost(host);

      var rafTick = $$animateAsyncRun();
      var timeoutTick = function(fn) {
        $timeout(fn, 0, false);
      };

      this._doneCallbacks = [];
      this._tick = function(fn) {
        var doc = $document[0];

        // the document may not be ready or attached
        // to the module for some internal tests
        if ($$isDocumentHidden()) {
          timeoutTick(fn);
        } else {
          rafTick(fn);
        }
      };
      this._state = 0;
    }

    AnimateRunner.prototype = {
      setHost: function(host) {
        this.host = host || {};
      },

      done: function(fn) {
        if (this._state === DONE_COMPLETE_STATE) {
          fn();
        } else {
          this._doneCallbacks.push(fn);
        }
      },


Review comment (suggestion):
Ah, I confused this with the `$$rAF`'s tick. Since it's indeed called one, I guess it's fine as it is.


Code after:
    AnimateRunner.all = function(runners, callback) {
      var count = 0;
      var status = true;
      forEach(runners, function(runner) {
        runner.done(onProgress);
      });

      function onProgress(response) {
        status = status && response;
        if (++count === runners.length) {
          callback(status);
        }
      }
    };

    function AnimateRunner(host) {
      this.setHost(host);

      var rafTick = $$animateAsyncRun();
      var timeoutTick = function(fn) {
        $timeout(fn, 0, false);
      };

      this._doneCallbacks = [];
      this._tick = function(fn) {
        if ($$isDocumentHidden()) {
          timeoutTick(fn);
        } else {
          rafTick(fn);
        }
      };
      this._state = 0;
    }

    AnimateRunner.prototype = {
      setHost: function(host) {
        this.host = host || {};
      },

      done: function(fn) {
        if (this._state === DONE_COMPLETE_STATE) {
          fn();
        } else {
          this._doneCallbacks.push(fn);
        }
      },

      progress: noop,

      getPromise: function() {
        if (!this.promise) {
Repository: angular/angular.js
File: src/ngMock/angular-mocks.js (JavaScript)

Code before:
    return header;
  };

  this.getAllResponseHeaders = function() {
    var lines = [];

    angular.forEach(this.$$respHeaders, function(value, key) {
      lines.push(key + ': ' + value);
    });
    return lines.join('\n');
  };

  this.abort = angular.noop;

  this.$$events = {};
  this.addEventListener = function(name, listener) {
    if (angular.isUndefined(this.$$events[name])) this.$$events[name] = [];
    this.$$events[name].push(listener);
  };

  this.upload = {
    $$events: {},
    addEventListener: this.addEventListener
  };
}


/**
 * @ngdoc service
 * @name $timeout
 * @description
 *
 * This service is just a simple decorator for {@link ng.$timeout $timeout} service
 * that adds a "flush" and "verifyNoPendingTasks" methods.
 */

angular.mock.$TimeoutDecorator = ['$delegate', '$browser', function($delegate, $browser) {

  /**
   * @ngdoc method
   * @name $timeout#flush
   * @description
   *
   * Flushes the queue of pending tasks.
   *
   * @param {number=} delay maximum timeout amount to flush up until
   */
  $delegate.flush = function(delay) {
    $browser.defer.flush(delay);
  };


Review comment (bug):
I am kind of confused with this snippet:

When we create a `MockXhr`, we set `this.$$events = {}`.
Then inside the mock `$httpBackend`, right after we create `xhr = new MockXhr()`, we overwrite `xhr.$$events = eventHandlers` (where `eventHandlers` is of type `{[key: string]: function}`).
Then, when `addEventListener` is called on a `MockXhr` instance, we check create an array on `this.$$events[name]` and push the listener.

I would expect the tests to fail, but they obviously don't, so what do I miss ?


Code after:
    header = undefined;
    angular.forEach(this.$$respHeaders, function(headerVal, headerName) {
      if (!header && angular.lowercase(headerName) == name) header = headerVal;
    });
    return header;
  };

  this.getAllResponseHeaders = function() {
    var lines = [];

    angular.forEach(this.$$respHeaders, function(value, key) {
      lines.push(key + ': ' + value);
    });
    return lines.join('\n');
  };

  this.abort = angular.noop;

  // This section simulates the events on a real XHR object (and the upload object)
  // When we are testing $httpBackend (inside the angular project) we make partial use of this
  // but store the events directly ourselves on `$$events`, instead of going through the `addEventListener`
  this.$$events = {};
  this.addEventListener = function(name, listener) {
    if (angular.isUndefined(this.$$events[name])) this.$$events[name] = [];
    this.$$events[name].push(listener);
  };

  this.upload = {
    $$events: {},
    addEventListener: this.addEventListener
  };
}


/**
 * @ngdoc service
 * @name $timeout
 * @description
 *
 * This service is just a simple decorator for {@link ng.$timeout $timeout} service
 * that adds a "flush" and "verifyNoPendingTasks" methods.
 */

angular.mock.$TimeoutDecorator = ['$delegate', '$browser', function($delegate, $browser) {

  /**
   * @ngdoc method
   * @name $timeout#flush
   * @description
   *
   * Flushes the queue of pending tasks.
Repository: angular/angular.js
File: src/jqLite.js (JavaScript)

Code before:
  },

  attr: function(element, name, value) {
    var ret;
    var nodeType = element.nodeType;
    if (nodeType === NODE_TYPE_TEXT || nodeType === NODE_TYPE_ATTRIBUTE || nodeType === NODE_TYPE_COMMENT) {
      return;
    }

    var lowercasedName = lowercase(name);
    var isBooleanAttr = BOOLEAN_ATTR[lowercasedName];

    if (isDefined(value)) {
      // setter

      if (value === null || (value === false && isBooleanAttr)) {
        element.removeAttribute(name);
      } else {
        element.setAttribute(name, isBooleanAttr ? lowercasedName : value);
      }
    } else {
      // getter

      ret = element.getAttribute(name);

      if (isBooleanAttr && ret !== null) {
        ret = lowercasedName;
      }
      // Normalize non-existing attributes to undefined (as jQuery).
      return ret === null ? undefined : ret;
    }
  },

  prop: function(element, name, value) {
    if (isDefined(value)) {
      element[name] = value;
    } else {
      return element[name];
    }
  },

  text: (function() {
    getText.$dv = '';
    return getText;

    function getText(element, value) {
      if (isUndefined(value)) {
        var nodeType = element.nodeType;
        return (nodeType === NODE_TYPE_ELEMENT || nodeType === NODE_TYPE_TEXT) ? element.textContent : '';
      }
      element.textContent = value;

Review comment (suggestion):
Previously, there was this `element.getAttribute` check, which is now removed.
Was it necessary for IE<9 browsers only? It would be useful to have a comment (e.g. in the PR) explaining why it is not necessary any more.


Code after:
  },

  attr: function(element, name, value) {
    var ret;
    var nodeType = element.nodeType;
    if (nodeType === NODE_TYPE_TEXT || nodeType === NODE_TYPE_ATTRIBUTE || nodeType === NODE_TYPE_COMMENT ||
      !element.getAttribute) {
      return;
    }

    var lowercasedName = lowercase(name);
    var isBooleanAttr = BOOLEAN_ATTR[lowercasedName];

    if (isDefined(value)) {
      // setter

      if (value === null || (value === false && isBooleanAttr)) {
        element.removeAttribute(name);
      } else {
        element.setAttribute(name, isBooleanAttr ? lowercasedName : value);
      }
    } else {
      // getter

      ret = element.getAttribute(name);

      if (isBooleanAttr && ret !== null) {
        ret = lowercasedName;
      }
      // Normalize non-existing attributes to undefined (as jQuery).
      return ret === null ? undefined : ret;
    }
  },

  prop: function(element, name, value) {
    if (isDefined(value)) {
      element[name] = value;
    } else {
      return element[name];
    }
  },

  text: (function() {
    getText.$dv = '';
    return getText;

    function getText(element, value) {
      if (isUndefined(value)) {
        var nodeType = element.nodeType;
        return (nodeType === NODE_TYPE_ELEMENT || nodeType === NODE_TYPE_TEXT) ? element.textContent : '';
      }
Repository: angular/angular.js
File: test/ng/directive/selectSpec.js (JavaScript)

Code before:
            expect(optionElements[2].selected).toBe(true);
            expect(scope.obj.value).toEqual(['B', 'C']);
        });

      });

      it('should keep the ngModel value when the selected option is recreated by ngRepeat', function() {
          scope.options = [{ name: 'A'}, { name: 'B'}, { name: 'C'}];
          scope.obj = {
            value: 'B'
          };

          compile(
            '<select ng-model="obj.value">' +
              '<option ng-repeat="option in options" value="{{option.name}}">{{option.name}}</option>' +
            '</select>'
          );

          var optionElements = element.find('option');
          expect(optionElements.length).toEqual(3);
          expect(optionElements[0].value).toBe('A');
          expect(optionElements[1]).toBeMarkedAsSelected();
          expect(scope.obj.value).toBe('B');

          scope.$apply(function() {
            // Only when new objects are used, ngRepeat re-creates the element from scratch
            scope.options = [{ name: 'B'}, { name: 'C'}, { name: 'D'}];
          });

          optionElements = element.find('option');
          expect(optionElements.length).toEqual(3);
          expect(optionElements[0].value).toBe('B');
          expect(optionElements[0]).toBeMarkedAsSelected();
          expect(scope.obj.value).toBe('B');
      });

    });


  });
});


Review comment (suggestion):
Just to make it more explicit, I would add an expactation that `optionElements[1] (before) !== optionElements[0] (after)`.

Code after:
            expect(optionElements[2].selected).toBe(true);
            expect(scope.obj.value).toEqual(['B', 'C']);
        });

      });

      it('should keep the ngModel value when the selected option is recreated by ngRepeat', function() {
          scope.options = [{ name: 'A'}, { name: 'B'}, { name: 'C'}];
          scope.obj = {
            value: 'B'
          };

          compile(
            '<select ng-model="obj.value">' +
              '<option ng-repeat="option in options" value="{{option.name}}">{{option.name}}</option>' +
            '</select>'
          );

          var optionElements = element.find('option');
          expect(optionElements.length).toEqual(3);
          expect(optionElements[0].value).toBe('A');
          expect(optionElements[1]).toBeMarkedAsSelected();
          expect(scope.obj.value).toBe('B');

          scope.$apply(function() {
            // Only when new objects are used, ngRepeat re-creates the element from scratch
            scope.options = [{ name: 'B'}, { name: 'C'}, { name: 'D'}];
          });

          var previouslySelectedOptionElement = optionElements[1];
          optionElements = element.find('option');

          expect(optionElements.length).toEqual(3);
          expect(optionElements[0].value).toBe('B');
          expect(optionElements[0]).toBeMarkedAsSelected();
          expect(scope.obj.value).toBe('B');
          // Ensure the assumption that the element is re-created is true
          expect(previouslySelectedOptionElement).not.toBe(optionElements[0]);
      });

    });


  });
});
Repository: angular/angular.js
File: src/ng/sanitizeUri.js (JavaScript)

Code before:
'use strict';

/**
 * @this
 * @description
 * Private service to sanitize uris for links and images. Used by $compile and $sanitize.
 */
function $$SanitizeUriProvider() {
  var aHrefSanitizationWhitelist = /^\s*(https?|s?ftp|mailto|tel|file):/,
    imgSrcSanitizationWhitelist = /^\s*((https?|s?ftp|file|blob):|data:image\/)/;

  /**
   * @description
   * Retrieves or overrides the default regular expression that is used for whitelisting of safe
   * urls during a[href] sanitization.
   *
   * The sanitization is a security measure aimed at prevent XSS attacks via html links.
   *
   * Any url about to be assigned to a[href] via data-binding is first normalized and turned into
   * an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist`
   * regular expression. If a match is found, the original url is written into the dom. Otherwise,
   * the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
   *
   * @param {RegExp=} regexp New regexp to whitelist urls with.
   * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
   *    chaining otherwise.
   */
  this.aHrefSanitizationWhitelist = function(regexp) {
    if (isDefined(regexp)) {
      aHrefSanitizationWhitelist = regexp;
      return this;
    }
    return aHrefSanitizationWhitelist;
  };


Review comment (bug):
Oops, I missed this, sorry: is there a reason you're updating imgSrc too? I can see how navigational links could work, but if browsers don't support the protocol, images and such wouldn't work at all (Chrome seems to complain with `Failed to load resource: net::ERR_UNKNOWN_URL_SCHEME` at least).

Code after:
'use strict';

/**
 * @this
 * @description
 * Private service to sanitize uris for links and images. Used by $compile and $sanitize.
 */
function $$SanitizeUriProvider() {
  var aHrefSanitizationWhitelist = /^\s*(https?|s?ftp|mailto|tel|file):/,
    imgSrcSanitizationWhitelist = /^\s*((https?|ftp|file|blob):|data:image\/)/;

  /**
   * @description
   * Retrieves or overrides the default regular expression that is used for whitelisting of safe
   * urls during a[href] sanitization.
   *
   * The sanitization is a security measure aimed at prevent XSS attacks via html links.
   *
   * Any url about to be assigned to a[href] via data-binding is first normalized and turned into
   * an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist`
   * regular expression. If a match is found, the original url is written into the dom. Otherwise,
   * the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
   *
   * @param {RegExp=} regexp New regexp to whitelist urls with.
   * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
   *    chaining otherwise.
   */
  this.aHrefSanitizationWhitelist = function(regexp) {
    if (isDefined(regexp)) {
      aHrefSanitizationWhitelist = regexp;
      return this;
    }
    return aHrefSanitizationWhitelist;
  };
Repository: angular/angular.js
File: docs/config/processors/versions-data.js (JavaScript)

Code before:
        var url = 'https://code.angularjs.org/' + version.version + '/docs';
        // Versions before 1.0.2 had a different docs folder name
        if (version.major === 1 && version.minor === 0 && version.patch < 2) {
          url += '-' + version.version;
        }
        return url;
      }

      function reverse(fn) {
        return function(left, right) { return -fn(left, right); };
      }

      function sortObject(obj, cmp) {
        return Object.keys(obj).map(function(key) { return obj[key]; }).sort(cmp);
      }

      // https://github.com/kaelzhang/node-semver-stable/blob/34dd29842409295d49889d45871bec55a992b7f6/index.js#L25
      function semverIsStable(version) {
        var semverObj = semver.parse(version);
        return semverObj === null ? false : !semverObj.prerelease.length;
      }

      function createSnapshotStableLabel(version) {
        var semverObj = semver.parse(version);

        var label = 'v' + semverObj.inc('patch').version + '-snapshot';

        return label;
      }
    }
  };
};


Review comment (suggestion):
It would be nice if we showed `v1.6.x-snapshot` (instead of `v1.6.7-snapshot`, which is kind of unclear).

Code after:
        var url = 'https://code.angularjs.org/' + version.version + '/docs';
        // Versions before 1.0.2 had a different docs folder name
        if (version.major === 1 && version.minor === 0 && version.patch < 2) {
          url += '-' + version.version;
        }
        return url;
      }

      function reverse(fn) {
        return function(left, right) { return -fn(left, right); };
      }

      function sortObject(obj, cmp) {
        return Object.keys(obj).map(function(key) { return obj[key]; }).sort(cmp);
      }

      // https://github.com/kaelzhang/node-semver-stable/blob/34dd29842409295d49889d45871bec55a992b7f6/index.js#L25
      function semverIsStable(version) {
        var semverObj = semver.parse(version);
        return semverObj === null ? false : !semverObj.prerelease.length;
      }

      function createSnapshotStableLabel(version) {
        var label = 'v' + version.replace(/.$/, 'x') + '-snapshot';

        return label;
      }
    }
  };
};