ronantakizawa/codereview-bench huggingface dataset:
Repository: freeCodeCamp/freeCodeCamp
File: e2e/map.spec.ts (TypeScript)

Code before:
  ...superBlockOrder[SuperBlockStages.Python],
  ...superBlockOrder[SuperBlockStages.Professional],
  ...superBlockOrder[SuperBlockStages.Extra]
];

test.describe('Map Component E2E Test Suite', () => {
  test('Verifies the headings', async ({ page }) => {
    await expect(
      page.getByText(translations.landing['core-certs-heading'])
    ).toBeVisible();
    await expect(
      page.getByText(translations.landing['professional-certs-heading'])
    ).toBeVisible();
    await expect(
      page.getByText(translations.landing['interview-prep-heading'])
    ).toBeVisible();
  });

  test('verifies all the certifications links rendered correctly', async ({
    page
  }) => {
    const curriculumBtns = page.getByTestId('curriculum-map-button');
    await expect(curriculumBtns).toHaveCount(15);
    for (let i = 0; i < superBlocksWithLinks.length; i++) {
      const superblockLink = page.getByRole('link', {
        name: intro[superBlocksWithLinks[i]].title
      });
      expect(await superblockLink.getAttribute('href')).toBe(
        `/learn/${superBlocksWithLinks[i]}/`
      );
      await superblockLink.click();
    }
  });
});


Review comment (suggestion):
```suggestion
test.describe('Map Component', () => {
  test('should render correctly', async ({ page }) => {
    await expect(
      page.getByText(translations.landing['core-certs-heading'])
    ).toBeVisible();
    await expect(
      page.getByText(translations.landing['professional-certs-heading'])
    ).toBeVisible();
    await expect(
      page.getByText(translations.landing['interview-prep-heading'])
    ).toBeVisible();
    const curriculumBtns = page.getByTestId('curriculum-map-button');
    await expect(curriculumBtns).toHaveCount(15);
    for (let i = 0; i < superBlocksWithLinks.length; i++) {
      const superblockLink = page.getByRole('link', {
        name: intro[superBlocksWithLinks[i]].title
      });
      expect(await superblockLink.getAttribute('href')).toBe(
        `/learn/${superBlocksWithLinks[i]}/`
      );
      await superblockLink.click();
    }
  });
});
```

We could also probably merge these into one test.

Code after:
];

test.describe('Map Component', () => {
  test('should render correctly', async ({ page }) => {
    await expect(
      page.getByText(translations.landing['core-certs-heading'])
    ).toBeVisible();
    await expect(
      page.getByText(translations.landing['professional-certs-heading'])
    ).toBeVisible();
    await expect(
      page.getByText(translations.landing['interview-prep-heading'])
    ).toBeVisible();
    const curriculumBtns = page.getByTestId('curriculum-map-button');
    await expect(curriculumBtns).toHaveCount(15);
    for (let i = 0; i < superBlocksWithLinks.length; i++) {
      const superblockLink = page.getByRole('link', {
        name: intro[superBlocksWithLinks[i]].title
      });
      expect(await superblockLink.getAttribute('href')).toBe(
        `/learn/${superBlocksWithLinks[i]}/`
      );
      await superblockLink.click();
    }
  });
});
Repository: freeCodeCamp/freeCodeCamp
File: client/src/redux/prop-types.ts (TypeScript)

Code before:
    // TODO: make enum like superBlock
    certification: string;
    title: CertTitle;
  };
  headings: [
    {
      depth: number;
      value: string;
      id: string;
    }
  ];
  html: string;
  htmlAst: Record<string, unknown>;
  id: string;
  rawMarkdownBody: string;
  timeToRead: number;
  wordCount: {
    paragraphs: number;
    sentences: number;
    words: number;
  };
};

export type MultipleChoiceAnswer = {
  answer: string;
  feedback: string;
};

export type Question = {
  text: string;
  answers: MultipleChoiceAnswer[];
  solution: number;
};
export type Fields = {
  slug: string;
  blockHashSlug: string;
  blockName: string;
  tests: Test[];
};
type Required = {
  link: string;
  raw: boolean;
  src: string;
  crossDomain?: boolean;
};
export interface BilibiliIds {
  aid: string;
  bvid: string;
  cid: string;
}


Review comment (suggestion):
Is there a particular reason to not do this?

```suggestion
  feedback: string | null;
```

Code after:
    // TODO: make enum like superBlock
    certification: string;
    title: CertTitle;
  };
  headings: [
    {
      depth: number;
      value: string;
      id: string;
    }
  ];
  html: string;
  htmlAst: Record<string, unknown>;
  id: string;
  rawMarkdownBody: string;
  timeToRead: number;
  wordCount: {
    paragraphs: number;
    sentences: number;
    words: number;
  };
};

export type MultipleChoiceAnswer = {
  answer: string;
  feedback: string | null;
};

export type Question = {
  text: string;
  answers: MultipleChoiceAnswer[];
  solution: number;
};
export type Fields = {
  slug: string;
  blockHashSlug: string;
  blockName: string;
  tests: Test[];
};
type Required = {
  link: string;
  raw: boolean;
  src: string;
  crossDomain?: boolean;
};
export interface BilibiliIds {
  aid: string;
  bvid: string;
  cid: string;
}
Repository: freeCodeCamp/freeCodeCamp
File: tools/challenge-parser/parser/plugins/utils/before-heading.test.js (JavaScript)

Code before:
const isArray = require('lodash/isArray');
const simpleAst = require('../../__fixtures__/ast-simple.json');
const getAllBefore = require('./before-heading');

describe('between-headings', () => {
  it('should return an array', () => {
    expect.assertions(1);
    const actual = getAllBefore(simpleAst, '--hints--');
    expect(isArray(actual)).toBe(true);
  });

  it('should return an empty array if the marker is not present', () => {
    expect.assertions(2);
    const actual = getAllBefore(simpleAst, '--not-a-marker--');
    expect(isArray(actual)).toBe(true);
    expect(actual.length).toBe(0);
  });

  it('should include the whole AST before the marker', () => {
    expect.assertions(1);
    const actual = getAllBefore(simpleAst, '--hints--');
    expect(actual.length === 6).toBe(true);
  });
});


Review comment (bug):
Nit for better assertion when failing

```suggestion
    expect(actual).toHaveLength(6);
```

Code after:
const isArray = require('lodash/isArray');
const simpleAst = require('../../__fixtures__/ast-simple.json');
const getAllBefore = require('./before-heading');

describe('before-headings', () => {
  it('should return an array', () => {
    expect.assertions(1);
    const actual = getAllBefore(simpleAst, '--hints--');
    expect(isArray(actual)).toBe(true);
  });

  it('should return an empty array if the marker is not present', () => {
    expect.assertions(2);
    const actual = getAllBefore(simpleAst, '--not-a-marker--');
    expect(isArray(actual)).toBe(true);
    expect(actual.length).toBe(0);
  });

  it('should include the whole AST before the marker', () => {
    expect.assertions(1);
    const actual = getAllBefore(simpleAst, '--hints--');
    expect(actual).toHaveLength(6);
  });
});
Repository: freeCodeCamp/freeCodeCamp
File: e2e/video-player.spec.ts (TypeScript)

Code before:
import { test, expect } from '@playwright/test';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/scientific-computing-with-python/python-for-everybody/introduction-why-program'
  );
});

test.afterEach(async ({ page }) => {
  await page.close();
});

test.describe('Challenge Video Player Component Tests', () => {
  test('should render video player and play button', async ({ page }) => {
    await expect(
      page.locator('iframe[title="YouTube video player"]')
    ).toBeVisible();
    await expect(
      page
        .frameLocator('.display-youtube-video')
        .getByRole('button', { name: 'Play' })
    ).toBeVisible();
  });
});


Review comment (performance):
Should we add a playright test attribute to the iframe? I think that's more efficient than a `locator` query.

Code after:
import { test, expect } from '@playwright/test';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/scientific-computing-with-python/python-for-everybody/introduction-why-program'
  );
});

test.describe('Challenge Video Player Component Tests', () => {
  test('should render video player and play button', async ({ page }) => {
    await expect(
      page.locator('iframe[title="YouTube video player"]')
    ).toBeVisible();
    await expect(
      page
        .frameLocator('.display-youtube-video')
        .getByRole('button', { name: 'Play' })
    ).toBeVisible();
  });
});
Repository: freeCodeCamp/freeCodeCamp
File: e2e/video-player.spec.ts (TypeScript)

Code before:
import { test, expect } from '@playwright/test';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/scientific-computing-with-python/python-for-everybody/introduction-why-program'
  );
});

test.afterEach(async ({ page }) => {
  await page.close();
});

test.describe('Challenge Video Player Component Tests', () => {
  test('should render video player and play button', async ({ page }) => {
    await expect(
      page.locator('iframe[title="YouTube video player"]')
    ).toBeVisible();
    await expect(
      page
        .frameLocator('.display-youtube-video')
        .getByRole('button', { name: 'Play' })
    ).toBeVisible();
  });
});


Review comment (suggestion):
```suggestion
```

We actually don't need this bit. Playwright does this automatically.

Code after:
import { test, expect } from '@playwright/test';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/scientific-computing-with-python/python-for-everybody/introduction-why-program'
  );
});

test.describe('Challenge Video Player Component Tests', () => {
  test('should render video player and play button', async ({ page }) => {
    await expect(
      page.locator('iframe[title="YouTube video player"]')
    ).toBeVisible();
    await expect(
      page
        .frameLocator('.display-youtube-video')
        .getByRole('button', { name: 'Play' })
    ).toBeVisible();
  });
});
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/Flash/index.tsx (TypeScript)

Code before:
import { Alert, CloseButton, type AlertProps } from '@freecodecamp/ui';
import React from 'react';
import { useTranslation } from 'react-i18next';
import { TransitionGroup, CSSTransition } from 'react-transition-group';
import { FlashState } from '../../redux/types';
import { removeFlashMessage } from './redux';

import './flash.css';

type FlashProps = {
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // The flash type returned from the API is 'error' and needs to be mapped to 'danger'.
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert variant={flashStyle} className='flash-message'>
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
          />
        </Alert>
      </CSSTransition>
    </TransitionGroup>
  );
}

Review comment (bug):
Give `variables` a default value as it can be `undefined` and fail typecheck:

<img width="955" alt="Screenshot 2023-10-13 at 16 01 50" src="https://github.com/freeCodeCamp/freeCodeCamp/assets/25715018/2750acd4-601d-46ab-91e7-be1d74456ec0">


Code after:
import { Alert, CloseButton, type AlertProps } from '@freecodecamp/ui';
import React from 'react';
import { useTranslation } from 'react-i18next';
import { TransitionGroup, CSSTransition } from 'react-transition-group';
import { FlashState } from '../../redux/types';
import { removeFlashMessage } from './redux';

import './flash.css';

type FlashProps = {
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // Some APIs are returning 'error' as a flash type, and it needs to be mapped to 'danger'.
  // TODO: Standardize the value of `type`.
  // Tracking issue: https://github.com/freeCodeCamp/freeCodeCamp/issues/50184
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert
          variant={flashStyle}
          className='flash-message'
          data-playwright-test-label='flash-message'
        >
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/Flash/index.tsx (TypeScript)

Code before:
import { Alert, CloseButton, type AlertProps } from '@freecodecamp/ui';
import React from 'react';
import { useTranslation } from 'react-i18next';
import { TransitionGroup, CSSTransition } from 'react-transition-group';
import { FlashState } from '../../redux/types';
import { removeFlashMessage } from './redux';

import './flash.css';

type FlashProps = {
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // The flash type returned from the API is 'error' and needs to be mapped to 'danger'.
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert variant={flashStyle} className='flash-message'>
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
          />
        </Alert>
      </CSSTransition>
    </TransitionGroup>
  );
}

Flash.displayName = 'FlashMessages';

export default Flash;


Review comment (suggestion):
Add a note to prevent this logic from being removed.

Ref: https://github.com/freeCodeCamp/freeCodeCamp/pull/51892/files#r1355610305

Code after:
import { Alert, CloseButton, type AlertProps } from '@freecodecamp/ui';
import React from 'react';
import { useTranslation } from 'react-i18next';
import { TransitionGroup, CSSTransition } from 'react-transition-group';
import { FlashState } from '../../redux/types';
import { removeFlashMessage } from './redux';

import './flash.css';

type FlashProps = {
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // Some APIs are returning 'error' as a flash type, and it needs to be mapped to 'danger'.
  // TODO: Standardize the value of `type`.
  // Tracking issue: https://github.com/freeCodeCamp/freeCodeCamp/issues/50184
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert
          variant={flashStyle}
          className='flash-message'
          data-playwright-test-label='flash-message'
        >
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
          />
        </Alert>
      </CSSTransition>
    </TransitionGroup>
  );
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/Flash/index.tsx (TypeScript)

Code before:
import { useTranslation } from 'react-i18next';
import { TransitionGroup, CSSTransition } from 'react-transition-group';
import { FlashState } from '../../redux/types';
import { removeFlashMessage } from './redux';

import './flash.css';

type FlashProps = {
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // The flash type returned from the API is 'error' and needs to be mapped to 'danger'.
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert variant={flashStyle} className='flash-message'>
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
          />
        </Alert>
      </CSSTransition>
    </TransitionGroup>
  );
}

Flash.displayName = 'FlashMessages';

export default Flash;


Review comment (suggestion):
Assigning class name to `CSSTransition` has no effect. The class name also collides with the one we pass to Alert (line 29).

Code after:
import { useTranslation } from 'react-i18next';
import { TransitionGroup, CSSTransition } from 'react-transition-group';
import { FlashState } from '../../redux/types';
import { removeFlashMessage } from './redux';

import './flash.css';

type FlashProps = {
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // Some APIs are returning 'error' as a flash type, and it needs to be mapped to 'danger'.
  // TODO: Standardize the value of `type`.
  // Tracking issue: https://github.com/freeCodeCamp/freeCodeCamp/issues/50184
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert
          variant={flashStyle}
          className='flash-message'
          data-playwright-test-label='flash-message'
        >
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
          />
        </Alert>
      </CSSTransition>
    </TransitionGroup>
  );
}

Flash.displayName = 'FlashMessages';

export default Flash;
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/Flash/index.tsx (TypeScript)

Code before:
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // The flash type returned from the API is 'error' and needs to be mapped to 'danger'.
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert variant={flashStyle} className='flash-message'>
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
          />
        </Alert>
      </CSSTransition>
    </TransitionGroup>
  );
}

Flash.displayName = 'FlashMessages';

export default Flash;


Review comment (suggestion):
We actually have a usage issue here. In the documentation, we say that Alert shouldn't be dismissible, but we are using it with a close button.

https://github.com/freeCodeCamp/freeCodeCamp/blob/d52aedc046edd4f207507e7627c333013e137041/tools/ui-components/src/alert/alert.tsx#L19

IMHO, dismissible alert is an acceptable pattern, and we probably should:
- Update the documentation
- Build the dismissibility into the component

Code after:
  flashMessage: FlashState['message'];
  removeFlashMessage: typeof removeFlashMessage;
};

function Flash({ flashMessage, removeFlashMessage }: FlashProps): JSX.Element {
  const { type, message, id, variables = {} } = flashMessage;
  const { t } = useTranslation();

  // Some APIs are returning 'error' as a flash type, and it needs to be mapped to 'danger'.
  // TODO: Standardize the value of `type`.
  // Tracking issue: https://github.com/freeCodeCamp/freeCodeCamp/issues/50184
  const flashStyle =
    type === 'error' ? 'danger' : (type as AlertProps['variant']);

  function handleClose() {
    removeFlashMessage();
  }

  return (
    <TransitionGroup>
      <CSSTransition key={id} timeout={500}>
        <Alert
          variant={flashStyle}
          className='flash-message'
          data-playwright-test-label='flash-message'
        >
          {t(message, variables)}
          <CloseButton
            onClick={handleClose}
            label={t('buttons.close')}
            className='close'
          />
        </Alert>
      </CSSTransition>
    </TransitionGroup>
  );
}

Flash.displayName = 'FlashMessages';

export default Flash;
Repository: freeCodeCamp/freeCodeCamp
File: e2e/seo.spec.ts (TypeScript)

Code before:
import { test, expect } from '@playwright/test';
import { SuperBlocks } from '../shared/config/superblocks';
import type { ListItem } from '../client/src/components/seo/';
import metaTags from '../client/i18n/locales/english/meta-tags.json';

test.beforeEach(async ({ page }) => {
  await page.goto('/');
});

test.afterEach(async ({ page }) => {
  await page.close();
});

test('should set the correct title', async ({ page }) => {
  await expect(page).toHaveTitle(metaTags.title);
});

test('should inject structured data (JSON-LD) into the page', async ({
  page
}) => {
  const structuredData = await page
    .getByTestId('structured-data')
    .textContent();

  expect(structuredData).toBeTruthy();

  const parsedData = JSON.parse(structuredData ?? '');

  expect(parsedData?.['@context']).toBe('https://schema.org');
  expect(parsedData['@type']).toBe('ItemList');
  expect(parsedData.itemListElement).toBeInstanceOf(Array);

  expect(parsedData.itemListElement.length).toBe(
    Object.values(SuperBlocks).length
  );

  parsedData.itemListElement.forEach((listItem: ListItem, index: number) => {
    expect(listItem['@type']).toBe('ListItem');

Review comment (suggestion):
```suggestion
```

We actually don't need this. Playwright will close the page automatically.

Code after:
import { test, expect } from '@playwright/test';
import { SuperBlocks } from '../shared/config/superblocks';
import type { ListItem } from '../client/src/components/seo/';
import metaTags from '../client/i18n/locales/english/meta-tags.json';

test.beforeEach(async ({ page }) => {
  await page.goto('/');
});

test('should set the correct title', async ({ page }) => {
  await expect(page).toHaveTitle(metaTags.title);
});

test('should inject structured data (JSON-LD) into the page', async ({
  page
}) => {
  const structuredData = await page
    .getByTestId('structured-data')
    .textContent();

  expect(structuredData).toBeTruthy();

  const parsedData = JSON.parse(structuredData ?? '');

  expect(parsedData?.['@context']).toBe('https://schema.org');
  expect(parsedData['@type']).toBe('ItemList');
  expect(parsedData.itemListElement).toBeInstanceOf(Array);

  expect(parsedData.itemListElement.length).toBe(
    Object.values(SuperBlocks).length
  );

  parsedData.itemListElement.forEach((listItem: ListItem, index: number) => {
    expect(listItem['@type']).toBe('ListItem');
    expect(listItem.position).toBe(index + 1);

    const item = listItem.item;
    expect(item['@type']).toBe('Course');