ronantakizawa/codereview-bench huggingface dataset:
Repository: android/nowinandroid
File: feature/bookmarks/src/test/kotlin/com/google/samples/apps/nowinandroid/feature/bookmarks/BookmarksViewModelTest.kt (Kotlin)

Code before:
        check(itemBeforeViewed is Success)
        check(!itemBeforeViewed.feed.first().hasBeenViewed)

        // When
        viewModel.setNewsResourceViewed(newsResourcesTestData[0].id, true)

        // Then
        val item = viewModel.feedUiState.value
        assertIs<Success>(item)
        assertTrue(item.feed.first().hasBeenViewed)

        collectJob.cancel()
    }

    @Test
    fun whenUndoBookmarkRemoval_thenBookmarkIsRestored() = runTest {
        val collectJob = launch(UnconfinedTestDispatcher()) { viewModel.feedUiState.collect() }

        // Give
        newsRepository.sendNewsResources(newsResourcesTestData)
        userDataRepository.setNewsResourceBookmarked(newsResourcesTestData[0].id, true)
        viewModel.removeFromSavedResources(newsResourcesTestData[0].id)
        check(viewModel.shouldDisplayUndoBookmark)
        val itemBeforeUndo = viewModel.feedUiState.value
        check(itemBeforeUndo is Success)
        check(itemBeforeUndo.feed.isEmpty())

        // When
        viewModel.undoBookmarkRemoval()

        // Then
        assertFalse(viewModel.shouldDisplayUndoBookmark)
        val item = viewModel.feedUiState.value
        assertIs<Success>(item)
        assertEquals(item.feed.size, 1)

        collectJob.cancel()
    }
}


Review comment (suggestion):
```suggestion
        assertTrue(viewModel.shouldDisplayUndoBookmark)
        val itemBeforeUndo = viewModel.feedUiState.value
        assertIs<Success>(itemBeforeUndo)
        assertEquals(0, itemBeforeUndo.feed.size)
```

Also, I'm not sure these assertions in the "given" block are really useful.

Code after:
        viewModel.setNewsResourceViewed(newsResourcesTestData[0].id, true)

        // Then
        val item = viewModel.feedUiState.value
        assertIs<Success>(item)
        assertTrue(item.feed.first().hasBeenViewed)
    }

    @Test
    fun feedUiState_undoneBookmarkRemoval_bookmarkIsRestored() = runTest {
        backgroundScope.launch(UnconfinedTestDispatcher()) { viewModel.feedUiState.collect() }

        // Given
        newsRepository.sendNewsResources(newsResourcesTestData)
        userDataRepository.setNewsResourceBookmarked(newsResourcesTestData[0].id, true)
        viewModel.removeFromSavedResources(newsResourcesTestData[0].id)
        assertTrue(viewModel.shouldDisplayUndoBookmark)
        val itemBeforeUndo = viewModel.feedUiState.value
        assertIs<Success>(itemBeforeUndo)
        assertEquals(0, itemBeforeUndo.feed.size)

        // When
        viewModel.undoBookmarkRemoval()

        // Then
        assertFalse(viewModel.shouldDisplayUndoBookmark)
        val item = viewModel.feedUiState.value
        assertIs<Success>(item)
        assertEquals(1, item.feed.size)
    }
}
Repository: android/nowinandroid
File: core/network/src/main/kotlin/JvmUnitTestDemoAssetManager.kt (Kotlin)

Code before:
import com.google.samples.apps.nowinandroid.core.network.demo.DemoAssetManager
import java.io.File
import java.io.InputStream
import java.nio.charset.StandardCharsets
import java.util.Properties

/**
 * This class helps with loading Android `/assets` files, especially when running JVM unit tests.
 * It must remain on the root package for an easier [Class.getResource] with relative paths.
 * @see <a href="https://developer.android.com/reference/tools/gradle-api/7.3/com/android/build/api/dsl/UnitTestOptions">UnitTestOptions</a>
 */

internal object JvmUnitTestDemoAssetManager : DemoAssetManager {
    private val config =
        requireNotNull(javaClass.getResource("com/android/tools/test_config.properties")) {
            """
            Missing Android resources properties file.
            Did you forget to enable the feature in the gradle build file?
            android.testOptions.unitTests.isIncludeAndroidResources = true
            """.trimIndent()
        }
    private val properties = Properties().apply { config.openStream().use(::load) }
    private val assets = File(properties["android_merged_assets"].toString())

    override fun open(fileName: String): InputStream = File(assets, fileName).inputStream()
    override fun readText(fileName: String): String = File(assets, fileName).readText(StandardCharsets.UTF_8)
}


Review comment (suggestion):
We don't need to expose an additional api in the AssetManager. Simply use the already existing `open` function, and call either `decodeFromStream` and `decodeFromString(input.bufferedReader().readText())`

Code after:
import com.google.samples.apps.nowinandroid.core.network.demo.DemoAssetManager
import java.io.File
import java.io.InputStream
import java.util.Properties

/**
 * This class helps with loading Android `/assets` files, especially when running JVM unit tests.
 * It must remain on the root package for an easier [Class.getResource] with relative paths.
 * @see <a href="https://developer.android.com/reference/tools/gradle-api/7.3/com/android/build/api/dsl/UnitTestOptions">UnitTestOptions</a>
 */

internal object JvmUnitTestDemoAssetManager : DemoAssetManager {
    private val config =
        requireNotNull(javaClass.getResource("com/android/tools/test_config.properties")) {
            """
            Missing Android resources properties file.
            Did you forget to enable the feature in the gradle build file?
            android.testOptions.unitTests.isIncludeAndroidResources = true
            """.trimIndent()
        }
    private val properties = Properties().apply { config.openStream().use(::load) }
    private val assets = File(properties["android_merged_assets"].toString())

    override fun open(fileName: String): InputStream = File(assets, fileName).inputStream()
}
Repository: angular/angular
File: packages/examples/service-worker/push/service_worker_component.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
 */
// tslint:disable: no-duplicate-imports
import {Component} from '@angular/core';
import {ServiceWorkerModule} from '@angular/service-worker';
// #docregion inject-sw-push
import {SwPush} from '@angular/service-worker';
// #enddocregion inject-sw-push
// tslint:enable: no-duplicate-imports

const PUBLIC_VAPID_KEY_OF_SERVER = '...';

@Component({
  selector: 'example-app',
  imports: [ServiceWorkerModule.register('ngsw-worker.js')],
  template: 'SW enabled: {{ swPush.isEnabled }}',
})
// #docregion inject-sw-push
export class AppComponent {
  constructor(readonly swPush: SwPush) {}
  // #enddocregion inject-sw-push

  // #docregion subscribe-to-push
  private async subscribeToPush() {
    try {
      const sub = await this.swPush.requestSubscription({
        serverPublicKey: PUBLIC_VAPID_KEY_OF_SERVER,
      });
      // TODO: Send to server.
    } catch (err) {
      console.error('Could not subscribe due to:', err);
    }
  }
  // #enddocregion subscribe-to-push

  private subscribeToNotificationClicks() {
    // #docregion subscribe-to-notification-clicks
    this.swPush.notificationClicks.subscribe(({action, notification}) => {
      // TODO: Do something in response to notification click.
    });

Review comment (suggestion):
This isn't correct. You're supposed to provide it at the root level (in `bootstrapApplication` as `importProvidersFrom(ServiceWorkerModule.register('ngsw-worker.js'))`

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
 */
// tslint:disable: no-duplicate-imports
import {Component} from '@angular/core';

// #docregion inject-sw-push
import {SwPush} from '@angular/service-worker';
// #enddocregion inject-sw-push
// tslint:enable: no-duplicate-imports

const PUBLIC_VAPID_KEY_OF_SERVER = '...';

@Component({
  selector: 'example-app',
  template: 'SW enabled: {{ swPush.isEnabled }}',
})
// #docregion inject-sw-push
export class AppComponent {
  constructor(readonly swPush: SwPush) {}
  // #enddocregion inject-sw-push

  // #docregion subscribe-to-push
  private async subscribeToPush() {
    try {
      const sub = await this.swPush.requestSubscription({
        serverPublicKey: PUBLIC_VAPID_KEY_OF_SERVER,
      });
      // TODO: Send to server.
    } catch (err) {
      console.error('Could not subscribe due to:', err);
    }
  }
  // #enddocregion subscribe-to-push

  private subscribeToNotificationClicks() {
    // #docregion subscribe-to-notification-clicks
    this.swPush.notificationClicks.subscribe(({action, notification}) => {
      // TODO: Do something in response to notification click.
    });
    // #enddocregion subscribe-to-notification-clicks
Repository: angular/angular
File: packages/compiler-cli/src/ngtsc/typecheck/extended/api/format-extended-error.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 {ErrorCode} from '../../../diagnostics';
import {EXTENDED_ERROR_DETAILS_PAGE_BASE_URL} from '../../../diagnostics/src/error_details_base_url';

export function formatExtendedError(code: ErrorCode, message: null | false | string): string {
  const fullCode = `NG${Math.abs(code)}`;
  const errorMessage = `${fullCode}${message ? ': ' + message : ''}`;
  const addPeriodSeparator = !errorMessage.match(/[.,;!?\n]$/);
  const separator = addPeriodSeparator ? '.' : '';

  return `${errorMessage}${separator} Find more at ${EXTENDED_ERROR_DETAILS_PAGE_BASE_URL}/${fullCode}`;
}


Review comment (suggestion):
Can we add a short comment saying that, despite the compiler error number overlaping with runtime errors, they are distinct because runtime error codes are prefixed with 0 

Code after:
 * 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 {VERSION} from '@angular/compiler';

import {ErrorCode} from '../../../diagnostics';

/**
 * Base URL for the extended error details page.
 *
 * Keep the files below in full sync:
 *  - packages/compiler-cli/src/ngtsc/diagnostics/src/error_details_base_url.ts
 *  - packages/core/src/error_details_base_url.ts
 */
export const EXTENDED_ERROR_DETAILS_PAGE_BASE_URL: string = (() => {
  const versionSubDomain = VERSION.major !== '0' ? `v${VERSION.major}.` : '';
  return `https://${versionSubDomain}angular.dev/extended-diagnostics`;
})();

export function formatExtendedError(code: ErrorCode, message: null | false | string): string {
  // Note: Runtime error codes are prefixed with 0 (e.g., NG0100-999) while compiler errors
  // use plain numbers (e.g., NG1001), keeping them distinct despite numerical overlap.
  const fullCode = `NG${Math.abs(code)}`;
  const errorMessage = `${fullCode}${message ? ': ' + message : ''}`;
  const addPeriodSeparator = !errorMessage.match(/[.,;!?\n]$/);
  const separator = addPeriodSeparator ? '.' : '';

  return `${errorMessage}${separator} Find more at ${EXTENDED_ERROR_DETAILS_PAGE_BASE_URL}/${fullCode}`;
}
Repository: angular/angular
File: adev/src/app/editor/code-editor/workers/factory-provider.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 {InjectionToken, Provider} from '@angular/core';

export const TYPESCRIPT_VFS_WORKER = new InjectionToken<Worker>('TYPESCRIPT_VFS_WORKER');

export function createTypescriptVfsWorker(): Worker {
  return new Worker(new URL('./typescript-vfs.worker.ts', import.meta.url), {
    type: 'module',
  });
}

export const TYPESCRIPT_VFS_WORKER_PROVIDER: Provider = {
  provide: TYPESCRIPT_VFS_WORKER,
  useFactory: createTypescriptVfsWorker,
};


Review comment (suggestion):
We probably don't want to create the instance directly (so we can init later). 
What I would suggest is that we provide a factory of worker that we can invoke where it was intialy created 

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 {InjectionToken, Provider} from '@angular/core';

export type TypescriptVfsWorkerFactory = () => Worker;

export const TYPESCRIPT_VFS_WORKER_FACTORY = new InjectionToken<TypescriptVfsWorkerFactory>(
  'TYPESCRIPT_VFS_WORKER_FACTORY',
);

export function createTypescriptVfsWorker(): Worker {
  return new Worker(new URL('./typescript-vfs.worker.ts', import.meta.url), {
    type: 'module',
  });
}

export const TYPESCRIPT_VFS_WORKER_PROVIDER: Provider = {
  provide: TYPESCRIPT_VFS_WORKER_FACTORY,
  useValue: createTypescriptVfsWorker,
};
Repository: angular/angular
File: adev/src/app/editor/code-editor/workers/factory-provider.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 {InjectionToken, Provider} from '@angular/core';

export type TypescriptVfsWorkerFactory = () => Worker;

export const TYPESCRIPT_VFS_WORKER_FACTORY = new InjectionToken<TypescriptVfsWorkerFactory>(
  'TYPESCRIPT_VFS_WORKER_FACTORY',
);

export function createTypescriptVfsWorker(): Worker {
  return new Worker(new URL('./typescript-vfs.worker.ts', import.meta.url), {
    type: 'module',
  });
}

export const TYPESCRIPT_VFS_WORKER_PROVIDER: Provider = {
  provide: TYPESCRIPT_VFS_WORKER_FACTORY,
  useValue: () => createTypescriptVfsWorker(),
};


Review comment (suggestion):
Here you're creating the worker on token instanciation. 
It should create the factory, which can later be used to create the worker. 

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 {InjectionToken, Provider} from '@angular/core';

export type TypescriptVfsWorkerFactory = () => Worker;

export const TYPESCRIPT_VFS_WORKER_FACTORY = new InjectionToken<TypescriptVfsWorkerFactory>(
  'TYPESCRIPT_VFS_WORKER_FACTORY',
);

export function createTypescriptVfsWorker(): Worker {
  return new Worker(new URL('./typescript-vfs.worker.ts', import.meta.url), {
    type: 'module',
  });
}

export const TYPESCRIPT_VFS_WORKER_PROVIDER: Provider = {
  provide: TYPESCRIPT_VFS_WORKER_FACTORY,
  useValue: createTypescriptVfsWorker,
};
Repository: angular/angular
File: adev/shared-docs/pipeline/api-gen/rendering/transforms/type-alias-transforms.mts (TypeScript)

Code before:
 * found in the LICENSE file at https://angular.dev/license
 */

import {TypeAliasEntry} from '../entities.mjs';
import {TypeAliasEntryRenderable} from '../entities/renderables.mjs';
import {addRenderableCodeToc} from './code-transforms.mjs';
import {
  addHtmlAdditionalLinks,
  addHtmlDescription,
  addHtmlJsDocTagComments,
  addHtmlUsageNotes,
  setEntryFlags,
} from './jsdoc-transforms.mjs';
import {addRenderableMembers} from './member-transforms.mjs';
import {addModuleName} from './module-name.mjs';
import {addRepo} from './repo.mjs';

/** Given an unprocessed type alias entry, get the fully renderable type alias entry. */
export async function getTypeAliasRenderable(
  typeAliasEntry: TypeAliasEntry,
  moduleName: string,
  repo: string,
): Promise<TypeAliasEntryRenderable> {
  const entryWithModuleAndRepo = addRepo(addModuleName(typeAliasEntry, moduleName), repo);

  const renderableMembers = (typeAliasEntry as any).members
    ? (await addRenderableMembers(entryWithModuleAndRepo as any)).members
    : undefined;

  return setEntryFlags(
    await addRenderableCodeToc({
      ...addHtmlAdditionalLinks(
        addHtmlUsageNotes(addHtmlJsDocTagComments(addHtmlDescription(entryWithModuleAndRepo))),
      ),
      members: renderableMembers,
    }),
  );
}


Review comment (suggestion):
is that `as any` assertion necessary ? (looks like `TypeAliasEntry` has that property) 

Code after:
 * found in the LICENSE file at https://angular.dev/license
 */

import {TypeAliasEntry} from '../entities.mjs';
import {TypeAliasEntryRenderable} from '../entities/renderables.mjs';
import type {HasMembers} from '../entities/traits.mjs';
import {addRenderableCodeToc} from './code-transforms.mjs';
import {
  addHtmlAdditionalLinks,
  addHtmlDescription,
  addHtmlJsDocTagComments,
  addHtmlUsageNotes,
  setEntryFlags,
} from './jsdoc-transforms.mjs';
import {addRenderableMembers} from './member-transforms.mjs';
import {addModuleName} from './module-name.mjs';
import {addRepo} from './repo.mjs';

/** Given an unprocessed type alias entry, get the fully renderable type alias entry. */
export async function getTypeAliasRenderable(
  typeAliasEntry: TypeAliasEntry,
  moduleName: string,
  repo: string,
): Promise<TypeAliasEntryRenderable> {
  const entryWithModuleAndRepo = addRepo(addModuleName(typeAliasEntry, moduleName), repo);

  const renderableMembers = hasMembers(entryWithModuleAndRepo)
    ? (await addRenderableMembers(entryWithModuleAndRepo)).members
    : undefined;

  return setEntryFlags(
    await addRenderableCodeToc({
      ...addHtmlAdditionalLinks(
        addHtmlUsageNotes(addHtmlJsDocTagComments(addHtmlDescription(entryWithModuleAndRepo))),
      ),
      members: renderableMembers,
    }),
  );
}

function hasMembers(typeAlias: TypeAliasEntry): typeAlias is TypeAliasEntry & HasMembers {
  return typeAlias.members !== undefined;
}
Repository: angular/angular
File: packages/core/src/render3/instructions/animation.ts (TypeScript)

Code before:
  }

  ngDevMode && assertAnimationTypes(value, 'animate.leave');

  const lView = getLView();
  const animationsDisabled = areAnimationsDisabled(lView);
  if (animationsDisabled) {
    return ɵɵanimateLeave;
  }

  const tNode = getCurrentTNode()!;
  cancelLeavingNodes(tNode, lView);

  addAnimationToLView(getLViewLeaveAnimations(lView), tNode, () =>
    runLeaveAnimations(lView, tNode, value),
  );

  initializeAnimationQueueScheduler(lView[INJECTOR]);

  return ɵɵanimateLeave; // For chaining
}

function runLeaveAnimations(
  lView: LView,
  tNode: TNode,
  value: string | (() => string | string[]),
): {promise: Promise<void>; resolve: VoidFunction} {
  const {promise, resolve} = promiseWithResolvers<void>();
  const nativeElement = getNativeByTNode(tNode, lView) as Element;

  ngDevMode && assertElementNodes(nativeElement, 'animate.leave');

  const renderer = lView[RENDERER];
  const ngZone = lView[INJECTOR].get(NgZone);
  allLeavingAnimations.add(lView);
  (getLViewLeaveAnimations(lView).get(tNode.index)!.resolvers ??= []).push(resolve);

  const activeClasses = getClassListFromValue(value);
  if (activeClasses && activeClasses.length > 0) {
    animateLeaveClassRunner(
      nativeElement as HTMLElement,
      tNode,
      lView,
      activeClasses,
      renderer,
      ngZone,
    );
  } else {
    resolve();
  }


Review comment (suggestion):
```suggestion
  value: string | AnimationClassBindingFn,
```

Code after:
  }

  ngDevMode && assertAnimationTypes(value, 'animate.leave');

  const lView = getLView();
  const animationsDisabled = areAnimationsDisabled(lView);
  if (animationsDisabled) {
    return ɵɵanimateLeave;
  }

  const tNode = getCurrentTNode()!;
  cancelLeavingNodes(tNode, lView);

  addAnimationToLView(getLViewLeaveAnimations(lView), tNode, () =>
    runLeaveAnimations(lView, tNode, value),
  );

  initializeAnimationQueueScheduler(lView[INJECTOR]);

  return ɵɵanimateLeave; // For chaining
}

function runLeaveAnimations(
  lView: LView,
  tNode: TNode,
  value: string | AnimationClassBindingFn,
): {promise: Promise<void>; resolve: VoidFunction} {
  const {promise, resolve} = promiseWithResolvers<void>();
  const nativeElement = getNativeByTNode(tNode, lView) as Element;

  ngDevMode && assertElementNodes(nativeElement, 'animate.leave');

  const renderer = lView[RENDERER];
  const ngZone = lView[INJECTOR].get(NgZone);
  allLeavingAnimations.add(lView);
  (getLViewLeaveAnimations(lView).get(tNode.index)!.resolvers ??= []).push(resolve);

  const activeClasses = getClassListFromValue(value);
  if (activeClasses && activeClasses.length > 0) {
    animateLeaveClassRunner(
      nativeElement as HTMLElement,
      tNode,
      lView,
      activeClasses,
      renderer,
      ngZone,
    );
  } else {
    resolve();
  }
Repository: angular/angular
File: packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts (TypeScript)

Code before:
        ts.isIdentifier(member.name.expression) &&
        member.name.expression.text === 'ɵCONTROL',
    )
  );
}

function hasModel(name: string, meta: TypeCheckableDirectiveMeta): boolean {
  return (
    !!meta.inputs.getByBindingPropertyName(name)?.some((v) => v.isSignal) &&
    meta.outputs.hasBindingPropertyName(name + 'Change')
  );
}

/** Names of the input fields on custom controls. */
const formControlInputFields = [
  // Should be kept in sync with the `FormUiControl` bindings,
  // defined in `packages/forms/signals/src/api/control.ts`.
  'errors',
  'invalid',
  'disabled',
  'disabledReasons',
  'name',
  'readonly',
  'touched',
  'max',
  'maxlength',
  'maxLength',
  'min',
  'minLength',
  'minlength',
  'pattern',
  'required',
] as const;

function getCustomFieldDirectiveType(
  meta: TypeCheckableDirectiveMeta,
): 'value' | 'checkbox' | null {
  if (hasModel('value', meta)) {
    return 'value';
  } else if (hasModel('checked', meta)) {
    return 'checkbox';
  }

  return null;
}


Review comment (suggestion):
The lowercase variants (`maxlength`, `minlength`) that were included to catch unsupported attributes should not be included in this set of inputs. If a user's custom form control had a required `maxlength` input we wouldn't want to pretend it would be set by the `[field]` binding.

I think we can simply move them to the `unsupportedBindingFields` set.

Code after:
        ts.isPropertyDeclaration(member) &&
        ts.isComputedPropertyName(member.name) &&
        ts.isIdentifier(member.name.expression) &&
        member.name.expression.text === 'ɵCONTROL',
    )
  );
}

function hasModel(name: string, meta: TypeCheckableDirectiveMeta): boolean {
  return (
    !!meta.inputs.getByBindingPropertyName(name)?.some((v) => v.isSignal) &&
    meta.outputs.hasBindingPropertyName(name + 'Change')
  );
}

/** Names of the input fields on custom controls. */
const formControlInputFields = [
  // Should be kept in sync with the `FormUiControl` bindings,
  // defined in `packages/forms/signals/src/api/control.ts`.
  'errors',
  'invalid',
  'disabled',
  'disabledReasons',
  'name',
  'readonly',
  'touched',
  'max',
  'maxLength',
  'min',
  'minLength',
  'pattern',
  'required',
] as const;

function getCustomFieldDirectiveType(
  meta: TypeCheckableDirectiveMeta,
): 'value' | 'checkbox' | null {
  if (hasModel('value', meta)) {
    return 'value';
  } else if (hasModel('checked', meta)) {
    return 'checkbox';
  }

  return null;
}