ronantakizawa/codereview-bench huggingface dataset:
Repository: angular/angular
File: packages/compiler-cli/src/ngtsc/preprocessor.ts (TypeScript)

Code before:
    });

    const program = this.compiler.getCurrentProgram();

    for (const sf of program.getSourceFiles()) {
      if (sf.isDeclarationFile) {
        continue;
      }

      // If it is a TCB file (ends in .ngtypecheck.ts), we want it as-is, without any transformations.
      if (sf.fileName.endsWith('.ngtypecheck.ts')) {
        const content = printer.printFile(sf);
        result.push({fileName: sf.fileName, content});
        continue;
      }

      if (this.compiler.ignoreForEmit.has(sf)) {
        continue;
      }

      // Manually transform the source file
      const transformationResult = ts.transform(sf, beforeTransformers, this.options);

      if (transformationResult.transformed.length > 0) {
        const transformedSf = transformationResult.transformed[0];
        const content = printer.printFile(transformedSf);
        result.push({fileName: sf.fileName, content});
      }

      transformationResult.dispose();
    }

    return result;
  }
}


Review comment (suggestion):
For incremental, i think we may need to call `recordSuccessfulEmit` on `incrementalCompilation` of the compiler instance.

Code after:
    });

    const program = this.compiler.getCurrentProgram();

    for (const sf of program.getSourceFiles()) {
      if (sf.isDeclarationFile) {
        continue;
      }

      // If it is a TCB file (ends in .ngtypecheck.ts), we want it as-is, without any transformations.
      if (sf.fileName.endsWith('.ngtypecheck.ts')) {
        const content = printer.printFile(sf);
        result.push({fileName: sf.fileName, content});
        continue;
      }

      // Manually transform the source file
      const transformationResult = ts.transform(sf, beforeTransformers, this.options);

      if (transformationResult.transformed.length !== 1) {
        transformationResult.dispose();
        throw new Error(
          `Expected exactly one transformed file, got ${transformationResult.transformed.length}`,
        );
      }

      const transformedSf = transformationResult.transformed[0];

      const content = printer.printFile(transformedSf);
      result.push({fileName: sf.fileName, content});

      transformationResult.dispose();

      this.compiler.incrementalCompilation.recordSuccessfulEmit(sf);
    }

    return result;
  }
}
Repository: angular/angular
File: packages/common/src/pipes/case_conversion_pipes.ts (TypeScript)

Code before:
}

/**
 * Transforms text to all upper case.
 * @see {@link LowerCasePipe}
 * @see {@link TitleCasePipe}
 * @see [Built-in Pipes](guide/templates/pipes#built-in-pipes)
 *
 * @ngModule CommonModule
 * @publicApi
 */
@Pipe({
  name: 'uppercase',
})
export class UpperCasePipe implements PipeTransform {
  /**
   * @param value The string to transform to upper case.
   */
  transform(value: string): string;
  transform(value: null | undefined): null;
  transform(value: string | null | undefined): string | null;
  transform(value: string | null | undefined): string | null {
    if (value == null) return null;
    if (typeof value !== 'string') {
      throw invalidPipeArgumentError(UpperCasePipe, value);
    }
    return value.toUpperCase();
  }
}


Review comment (suggestion):
The `Object` type here is misleading — this function explicitly checks for a string.

Code after:
 * @see {@link LowerCasePipe}
 * @see {@link TitleCasePipe}
 * @see [Built-in Pipes](guide/templates/pipes#built-in-pipes)
 *
 * @ngModule CommonModule
 * @publicApi
 */
@Pipe({
  name: 'uppercase',
})
export class UpperCasePipe implements PipeTransform {
  /**
   * @param value The string to transform to upper case.
   */
  transform(value: string): string;
  transform(value: null | undefined): null;
  transform(value: string | null | undefined): string | null;
  transform(value: string | null | undefined): string | null {
    if (value == null) return null;
    assertPipeArgument(UpperCasePipe, value);
    return value.toUpperCase();
  }
}

function assertPipeArgument(pipe: Type<any>, value: Object): void {
  if (typeof value !== 'string') {
    throw invalidPipeArgumentError(pipe, value);
  }
}
Repository: angular/angular
File: packages/core/src/di/forward_ref.ts (TypeScript)

Code before:
 * })
 * export class ParentComponent {
 *    hideParent = input.required<boolean>();
 * }
 *
 *
 * @Component({
 *   imports: [forwardRef(() => ParentComponent)],
 *   selector: 'app-child',
 *   template: `
 *    @if(!hideParent()) {
 *       <app-parent/>
 *    }
 *  `,
 * })
 * export class ChildComponent {
 *    hideParent = input.required<boolean>();
 * }
 * ```
 * @see [Resolve circular dependencies with a forward reference](guide/di/di-in-action#resolve-circular-dependencies-with-a-forward-reference)
 * @publicApi
 */
export function forwardRef(forwardRefFn: ForwardRefFn): Type<any> {
  (<any>forwardRefFn).__forward_ref__ = forwardRef;
  (<any>forwardRefFn).toString = function () {
    return ngDevMode ? stringify(this()) : '';
  };
  return <Type<any>>(<any>forwardRefFn);
}

/**
 * Lazily retrieves the reference value from a forwardRef.
 *
 * Acts as the identity function when given a non-forward-ref value.
 *
 * @usageNotes
 * ### Example
 *
 * {@example core/di/ts/forward_ref/forward_ref_spec.ts region='resolve_forward_ref'}
 *
 * @see {@link forwardRef}
 * @publicApi
 */
export function resolveForwardRef<T>(type: T): T {
  return isForwardRef(type) ? type() : type;
}

/** Checks whether a function is wrapped by a `forwardRef`. */
export function isForwardRef(fn: any): fn is () => any {
  return (
    typeof fn === 'function' &&

Review comment (bug):
I’d only have a question in the case of toString. From what I reviewed, I couldn’t find any usage other than a possible development-time debug scenario.
Would this be necessary, similar to R3Injector, if we want to remove the stringify function from the final bundle

Code after:
 * })
 * export class ParentComponent {
 *    hideParent = input.required<boolean>();
 * }
 *
 *
 * @Component({
 *   imports: [forwardRef(() => ParentComponent)],
 *   selector: 'app-child',
 *   template: `
 *    @if(!hideParent()) {
 *       <app-parent/>
 *    }
 *  `,
 * })
 * export class ChildComponent {
 *    hideParent = input.required<boolean>();
 * }
 * ```
 * @see [Resolve circular dependencies with a forward reference](guide/di/di-in-action#resolve-circular-dependencies-with-a-forward-reference)
 * @publicApi
 */
export function forwardRef(forwardRefFn: ForwardRefFn): Type<any> {
  (<any>forwardRefFn).__forward_ref__ = forwardRef;
  if (ngDevMode) {
    (<any>forwardRefFn).toString = function () {
      return stringify(this());
    };
  }

  return <Type<any>>(<any>forwardRefFn);
}

/**
 * Lazily retrieves the reference value from a forwardRef.
 *
 * Acts as the identity function when given a non-forward-ref value.
 *
 * @usageNotes
 * ### Example
 *
 * {@example core/di/ts/forward_ref/forward_ref_spec.ts region='resolve_forward_ref'}
 *
 * @see {@link forwardRef}
 * @publicApi
 */
export function resolveForwardRef<T>(type: T): T {
  return isForwardRef(type) ? type() : type;
}

/** Checks whether a function is wrapped by a `forwardRef`. */
Repository: angular/angular
File: devtools/projects/shell-browser/src/app/backend.ts (TypeScript)

Code before:
/**
 * @license
 * Copyright Google LLC All Rights Reserved.
 *
 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.dev/license
 */

import {initializeMessageBus} from '../../../ng-devtools-backend';

import {unHighlight} from '../../../ng-devtools-backend/src/lib/highlighter';

import {initializeExtendedWindowOperations} from './chrome-window-extensions';
import {BACKEND_URI, CONTENT_SCRIPT_URI} from './communication';
import {SamePageMessageBus} from './same-page-message-bus';

const messageBus = new SamePageMessageBus(
  `${BACKEND_URI}-${location.href}`,
  `${CONTENT_SCRIPT_URI}-${location.href}`,
);

let initialized = false;
messageBus.on('handshake', () => {
  if (initialized) {
    return;
  }
  initialized = true;
  initializeMessageBus(messageBus);
  initializeExtendedWindowOperations();

  let inspectorRunning = false;
  messageBus.on('inspectorStart', () => {
    inspectorRunning = true;
  });

  messageBus.on('inspectorEnd', () => {
    inspectorRunning = false;
  });

  // handles case when mouse leaves chrome extension too quickly. unHighlight() is not a very
  // expensive function and has an if check so it's DOM api call is not called more than necessary
  document.addEventListener(
    'mousemove',
    () => {
      if (!inspectorRunning) {

Review comment (bug):
I think we should strip the query parameters from the Href, since they will cause #64723 again.

Here is an [older diff](https://github.com/angular/angular/pull/64806/changes/cbaf69cd6881f520b5ad8e894f6427a8de29df0b#diff-695027f3fe3a70d004bfceaea74039d0b8afa8fce272602f8d953771408ac084) pre-href removal that intended to fix the bug.

Code after:
/**
 * @license
 * Copyright Google LLC All Rights Reserved.
 *
 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.dev/license
 */

import {initializeMessageBus} from '../../../ng-devtools-backend';

import {unHighlight} from '../../../ng-devtools-backend/src/lib/highlighter';

import {initializeExtendedWindowOperations} from './chrome-window-extensions';
import {getBackendUri, getContentScriptUri} from './comm-utils';
import {SamePageMessageBus} from './same-page-message-bus';

const messageBus = new SamePageMessageBus(getBackendUri(), getContentScriptUri());

let initialized = false;
messageBus.on('handshake', () => {
  if (initialized) {
    return;
  }
  initialized = true;
  initializeMessageBus(messageBus);
  initializeExtendedWindowOperations();

  let inspectorRunning = false;
  messageBus.on('inspectorStart', () => {
    inspectorRunning = true;
  });

  messageBus.on('inspectorEnd', () => {
    inspectorRunning = false;
  });

  // handles case when mouse leaves chrome extension too quickly. unHighlight() is not a very
  // expensive function and has an if check so it's DOM api call is not called more than necessary
  document.addEventListener(
    'mousemove',
    () => {
      if (!inspectorRunning) {
        unHighlight();
      }
    },
Repository: angular/angular
File: packages/zone.js/lib/common/promise.ts (TypeScript)

Code before:
      (Ctor as any)[symbolThenPatched] = true;
    }

    api.patchThen = patchThen;

    function zoneify(fn: Function) {
      return function (self: any, args: any[]) {
        let resultPromise = fn.apply(self, args);
        if (resultPromise instanceof ZoneAwarePromise) {
          return resultPromise;
        }
        let ctor = resultPromise.constructor;
        if (!ctor[symbolThenPatched]) {
          patchThen(ctor);
        }
        return resultPromise;
      };
    }

    if (NativePromise) {
      patchThen(NativePromise);
      const nativeTry = (NativePromise as any)['try'];
      if (nativeTry && typeof nativeTry === 'function') {
        (ZoneAwarePromise as any)['try'] = nativeTry;
      }
      patchMethod(global, 'fetch', (delegate) => zoneify(delegate));
    }

    // This is not part of public API, but it is useful for tests, so we expose it.
    (Promise as any)[Zone.__symbol__('uncaughtPromiseErrors')] = _uncaughtPromiseErrors;
    return ZoneAwarePromise;
  });
}


Review comment (performance):
Consider: Would it be worth doing something more generic to propagate any unknown properties on `Promise` to `ZoneAwarePromise`?

That might be more effort than it's worth and comes with a performance cost, but could potentially mean any future additions don't require us to manually plumb through each property.

Code after:
      (Ctor as any)[symbolThenPatched] = true;
    }

    api.patchThen = patchThen;

    function zoneify(fn: Function) {
      return function (self: any, args: any[]) {
        let resultPromise = fn.apply(self, args);
        if (resultPromise instanceof ZoneAwarePromise) {
          return resultPromise;
        }
        let ctor = resultPromise.constructor;
        if (!ctor[symbolThenPatched]) {
          patchThen(ctor);
        }
        return resultPromise;
      };
    }

    if (NativePromise) {
      patchThen(NativePromise);
      // TODO(atscott): Investigate generic to propagate any unknown properties
      const nativeTry = (NativePromise as any)['try'];
      if (nativeTry && typeof nativeTry === 'function') {
        (ZoneAwarePromise as any)['try'] = nativeTry;
      }
      patchMethod(global, 'fetch', (delegate) => zoneify(delegate));
    }

    // This is not part of public API, but it is useful for tests, so we expose it.
    (Promise as any)[Zone.__symbol__('uncaughtPromiseErrors')] = _uncaughtPromiseErrors;
    return ZoneAwarePromise;
  });
}
Repository: angular/angular-cli
File: packages/angular/build/src/utils/load-proxy-config.ts (TypeScript)

Code before:
        }
        throw new Error(errorMessage);
      }

      break;
    }
    default:
      proxyConfiguration = await import(proxyPath);
  }

  if ('default' in proxyConfiguration) {
    proxyConfiguration = proxyConfiguration.default;
  }

  return normalizeProxyConfiguration(proxyConfiguration);
}

/**
 * Converts glob patterns to regular expressions to support Vite's proxy option.
 * Also converts the Webpack supported array form to an object form supported by both.
 *
 * @param proxy A proxy configuration object.
 */
function normalizeProxyConfiguration(
  proxy: Record<string, object> | object[],
): Record<string, object> {
  let normalizedProxy: Record<string, object> | undefined;

  if (Array.isArray(proxy)) {
    // Construct an object-form proxy configuration from the array
    normalizedProxy = {};
    for (const proxyEntry of proxy) {
      if (!('context' in proxyEntry)) {
        continue;
      }
      if (!Array.isArray(proxyEntry.context)) {
        continue;
      }

      // Array-form entries contain a context string array with the path(s)
      // to use for the configuration entry.
      const context = proxyEntry.context;
      delete proxyEntry.context;
      for (const contextEntry of context) {
        if (typeof contextEntry !== 'string') {
          continue;
        }

        normalizedProxy[contextEntry] = proxyEntry;
      }
    }

Review comment (suggestion):
We should still handle the async case here (top-level await in the config file or its dependencies). Unfortunately, we would still need the workaround for this one case.

Code after:
          const { line, column } = getJsonErrorLineColumn(parseError.offset, content);
          errorMessage += `\n[${line}, ${column}] ${printParseErrorCode(parseError.error)}`;
        }
        throw new Error(errorMessage);
      }

      break;
    }
    default: {
      try {
        proxyConfiguration = await import(proxyPath);
      } catch (e) {
        assertIsError(e);
        if (e.code !== 'ERR_REQUIRE_ASYNC_MODULE') {
          throw e;
        }

        proxyConfiguration = await loadEsmModule<{ default: unknown }>(pathToFileURL(proxyPath));
      }

      break;
    }
  }

  if ('default' in proxyConfiguration) {
    proxyConfiguration = proxyConfiguration.default;
  }

  return normalizeProxyConfiguration(proxyConfiguration);
}

/**
 * Converts glob patterns to regular expressions to support Vite's proxy option.
 * Also converts the Webpack supported array form to an object form supported by both.
 *
 * @param proxy A proxy configuration object.
 */
function normalizeProxyConfiguration(
  proxy: Record<string, object> | object[],
): Record<string, object> {
  let normalizedProxy: Record<string, object> | undefined;

  if (Array.isArray(proxy)) {
    // Construct an object-form proxy configuration from the array
    normalizedProxy = {};
    for (const proxyEntry of proxy) {
      if (!('context' in proxyEntry)) {
        continue;
      }
      if (!Array.isArray(proxyEntry.context)) {
        continue;
Repository: angular/angular-cli
File: packages/angular/build/src/builders/unit-test/runners/vitest/executor.ts (TypeScript)

Code before:
    return startVitest(
      'test',
      undefined,
      {
        config: externalConfigPath,
        root: workspaceRoot,
        project: projectName,
        outputFile,
        testNamePattern: this.options.filter,
        watch,
        ui,
        ...debugOptions,
      },
      {
        server: {
          // Disable the actual file watcher. The boolean watch option above should still
          // be enabled as it controls other internal behavior related to rerunning tests.
          watch: null,
        },
        plugins: [
          await createVitestConfigPlugin({
            browser: browserOptions.browser,
            coverage,
            projectName,
            projectSourceRoot: this.options.projectSourceRoot,
            optimizeDepsInclude: this.externalMetadata.explicitBrowser.filter(
              (entry) => entry !== 'vitest',
            ),
            reporters,
            setupFiles: testSetupFiles,
            projectPlugins,
            include: [...this.testFileToEntryPoint.keys()].filter(
              // Filter internal entries
              (entry) => !entry.startsWith('angular:'),
            ),
          }),
        ],
      },
    );
  }
}


Review comment (suggestion):
Should I add a comment here?

I think that ideally, we should filter out all entrypoints from `explicitBrowser` but I can't imagine any valid scenario other then the `vitest` package import.

Code after:
    return startVitest(
      'test',
      undefined,
      {
        config: externalConfigPath,
        root: workspaceRoot,
        project: projectName,
        outputFile,
        testNamePattern: this.options.filter,
        watch,
        ui,
        ...debugOptions,
      },
      {
        server: {
          // Disable the actual file watcher. The boolean watch option above should still
          // be enabled as it controls other internal behavior related to rerunning tests.
          watch: null,
        },
        plugins: [
          await createVitestConfigPlugin({
            browser: browserOptions.browser,
            coverage,
            projectName,
            projectSourceRoot: this.options.projectSourceRoot,
            optimizeDepsInclude: this.externalMetadata.implicitBrowser,
            reporters,
            setupFiles: testSetupFiles,
            projectPlugins,
            include: [...this.testFileToEntryPoint.keys()].filter(
              // Filter internal entries
              (entry) => !entry.startsWith('angular:'),
            ),
          }),
        ],
      },
    );
  }
}
Repository: angular/angular-cli
File: packages/schematics/angular/migrations/use-application-builder/migration.ts (TypeScript)

Code before:
      ctx.logger.info(`Skipping updating '${path}' as it does not exist.`);
    }
  };
}

/**
 * Migration main entrypoint
 */
export default function (): Rule {
  return chain([
    updateProjects,
    // Delete package.json helper scripts
    updateJsonFile('package.json', (pkgJson) =>
      ['build:ssr', 'dev:ssr', 'serve:ssr', 'prerender'].forEach((s) =>
        pkgJson.remove(['scripts', s]),
      ),
    ),
    // Update main tsconfig
    updateJsonFile('tsconfig.json', (rootJson) => {
      let hasPreserveModule = false;
      const compilerOptions = rootJson.get(['compilerOptions']);

      if (compilerOptions && typeof compilerOptions === 'object') {
        const { module } = compilerOptions as JsonObject;
        hasPreserveModule = typeof module === 'string' && module.toLowerCase() === 'preserve';
      }

      if (!hasPreserveModule) {
        rootJson.modify(['compilerOptions', 'esModuleInterop'], true);
        rootJson.modify(['compilerOptions', 'moduleResolution'], 'bundler');
      }

      rootJson.modify(['compilerOptions', 'downlevelIteration'], undefined);
      rootJson.modify(['compilerOptions', 'allowSyntheticDefaultImports'], undefined);
    }),
  ]);
}


Review comment (suggestion):
This can be simplified a bit.

```suggestion
   const module = rootJson.get(['compilerOptions', 'module']);
   const hasPreserveModule = typeof module === 'string' && module.toLowerCase() === 'preserve';
```

Code after:
    }
  };
}

/**
 * Migration main entrypoint
 */
export default function (): Rule {
  return chain([
    updateProjects,
    // Delete package.json helper scripts
    updateJsonFile('package.json', (pkgJson) =>
      ['build:ssr', 'dev:ssr', 'serve:ssr', 'prerender'].forEach((s) =>
        pkgJson.remove(['scripts', s]),
      ),
    ),
    // Update main tsconfig
    updateJsonFile('tsconfig.json', (rootJson) => {
      const module = rootJson.get(['compilerOptions', 'module']);
      const hasPreserveModule = typeof module === 'string' && module.toLowerCase() === 'preserve';

      if (!hasPreserveModule) {
        rootJson.modify(['compilerOptions', 'esModuleInterop'], true);
        rootJson.modify(['compilerOptions', 'moduleResolution'], 'bundler');
      }

      rootJson.modify(['compilerOptions', 'downlevelIteration'], undefined);
      rootJson.modify(['compilerOptions', 'allowSyntheticDefaultImports'], undefined);
    }),
  ]);
}
Repository: angular/angular-cli
File: packages/angular/build/src/tools/vite/middlewares/host-check-middleware.ts (TypeScript)

Code before:
/**
 * @license
 * Copyright Google LLC All Rights Reserved.
 *
 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.dev/license
 */

import type { ServerResponse } from 'node:http';
import type { Connect, ViteDevServer } from 'vite';

function extractHostname(hostHeader: string | undefined): string | undefined {
  if (!hostHeader) {
    return undefined;
  }

  // Remove port if present (e.g., example.com:4200)
  const idx = hostHeader.lastIndexOf(':');
  if (idx > -1 && hostHeader.indexOf(']') === -1) {
    // Skip IPv6 addresses that include ':' within brackets
    return hostHeader.slice(0, idx).toLowerCase();
  }

  return hostHeader.toLowerCase();
}


Review comment (suggestion):
This seems to change the behaviour for a number of valid cases. 

Example: 
- `http://foo.localhost` will now be marked as invalid
-  When adding allowed hosts, all sub-domains will also be marked as valid, but with this change they will be marked as invalid. 
- Accessing the app with an external IP, will now be marked as invalid.


My suggestion would be that instead of recreating the middleware, just replace the response.

```ts

const middlewareStack = server.middlewares.stack;
const idx = middlewareStack.findIndex(({handle}) =>
  handle.name.startsWith('hostValidationMiddleware'),
);

if (idx !== -1) {
  const middleware = middlewareStack[idx];
  const originalHandle = middleware.handle;
  middleware.handle = function angularHostValidationMiddleware(req, res, next) {
    originalHandle(
      req,
      {
        writeHead: res.writeHead.bind(res),
        end: () => res.end('Error message.'),
      },
      next,
    );
  };
}

```



Code after:
/**
 * @license
 * Copyright Google LLC All Rights Reserved.
 *
 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.dev/license
 */

import type { IncomingMessage, ServerResponse } from 'node:http';
import type { Connect } from 'vite';

export function patchHostValidationMiddleware(middlewares: Connect.Server): void {
  const entry = middlewares.stack.find(
    ({ handle }) =>
      typeof handle === 'function' && handle.name.startsWith('hostValidationMiddleware'),
  );

  if (typeof entry?.handle !== 'function') {
    return;
  }

  const originalHandle = entry.handle as Connect.NextHandleFunction;

  entry.handle = function angularHostValidationMiddleware(
    req: IncomingMessage,
    res: ServerResponse,