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

Code before:
      // Wait for the new page to load.
      const newPage = await context.waitForEvent('page');
      await newPage.waitForLoadState();

      expect(newPage.url()).toBe(
        'https://www.freecodecamp.org/news/search/?query=test'
      );
    }
  });

  test('searching with search button click', async ({
    context,
    page,
    isMobile
  }) => {
    const searchInput = page.getByLabel(translations.search.label);

    if (isMobile) {
      await expect(searchInput).not.toBeVisible();
    } else {
      await expect(searchInput).toBeVisible();
      await searchInput.fill('test');
      await page.getByTestId(searchElements.searchButton).click();

      // wait for results to open in new window
      await page.waitForTimeout(1000);

      const url = context.pages()[1].url();
      expect(url).toBe('https://www.freecodecamp.org/news/search/?query=test');
    }
  });

  test('clearing search with clear button', async ({ page, isMobile }) => {
    const searchInput = page.getByLabel(translations.search.label);

    if (isMobile) {
      await expect(searchInput).not.toBeVisible();
    } else {
      await expect(searchInput).toBeVisible();
      await searchInput.fill('test');
      await page.getByTestId(searchElements.searchClear).click();

      await expect(searchInput).toHaveValue('');
    }
  });
});


Review comment (suggestion):
Changed this to:

```ts
const newPage = await context.waitForEvent('page');
await newPage.waitForLoadState();
```

to follow the doc: https://playwright.dev/docs/pages#handling-new-pages.

Code after:
    isMobile
  }) => {
    if (isMobile) {
      const menuButton = page.getByRole('button', {
        name: translations.buttons.menu
      });
      await menuButton.click();

      const searchInput = page.getByLabel(translations.search.label);
      await expect(searchInput).toBeVisible();

      await searchInput.fill('test');
      await page.keyboard.press('Enter');

      // Wait for the new page to load.
      const newPage = await context.waitForEvent('page');
      await newPage.waitForLoadState();

      expect(newPage.url()).toBe(
        'https://www.freecodecamp.org/news/search/?query=test'
      );
    } else {
      const searchInput = page.getByLabel(translations.search.label);
      await expect(searchInput).toBeVisible();

      await searchInput.fill('test');
      await page.keyboard.press('Enter');

      // Wait for the new page to load.
      const newPage = await context.waitForEvent('page');
      await newPage.waitForLoadState();

      expect(newPage.url()).toBe(
        'https://www.freecodecamp.org/news/search/?query=test'
      );
    }
  });

  test('should return the search results when the user clicks the search button', async ({
    context,
    page,
    isMobile
  }) => {
    if (isMobile) {
      const menuButton = page.getByRole('button', {
        name: translations.buttons.menu
      });
      await menuButton.click();

      const searchInput = page.getByLabel(translations.search.label);
      await expect(searchInput).toBeVisible();
Repository: freeCodeCamp/freeCodeCamp
File: e2e/profile.spec.ts (TypeScript)

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

import translations from '../client/i18n/locales/english/translations.json';

const certs = [
  {
    name: 'Responsive Web Design',
    url: '/certification/certifieduser/responsive-web-design'
  },
  {
    name: 'JavaScript Algorithms and Data Structures',
    url: '/certification/certifieduser/javascript-algorithms-and-data-structures-v8'
  },
  {
    name: 'Front End Development Libraries',
    url: '/certification/certifieduser/front-end-development-libraries'
  },
  {
    name: 'Data Visualization',
    url: '/certification/certifieduser/data-visualization'
  },
  {
    name: 'Relational Database',
    url: '/certification/certifieduser/relational-database-v8'
  },
  {
    name: 'Back End Development and APIs',
    url: '/certification/certifieduser/back-end-development-and-apis'
  },
  {
    name: 'Quality Assurance',
    url: '/certification/certifieduser/quality-assurance-v7'
  },
  {
    name: 'Scientific Computing with Python',
    url: '/certification/certifieduser/scientific-computing-with-python-v7'

Review comment (suggestion):
We should rename this to `JavaScript Algorithms and Data Structures (Beta)` to match the actual certification name. The tests shouldn't be affected by this change, as I believe they are checking if the element on the page _contains_ the `name` rather than exactly _matches_ it.

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

import translations from '../client/i18n/locales/english/translations.json';

const certs = [
  {
    name: 'Responsive Web Design',
    url: '/certification/certifieduser/responsive-web-design'
  },
  {
    name: 'JavaScript Algorithms and Data Structures (Beta)',
    url: '/certification/certifieduser/javascript-algorithms-and-data-structures-v8'
  },
  {
    name: 'Front End Development Libraries',
    url: '/certification/certifieduser/front-end-development-libraries'
  },
  {
    name: 'Data Visualization',
    url: '/certification/certifieduser/data-visualization'
  },
  {
    name: 'Relational Database',
    url: '/certification/certifieduser/relational-database-v8'
  },
  {
    name: 'Back End Development and APIs',
    url: '/certification/certifieduser/back-end-development-and-apis'
  },
  {
    name: 'Quality Assurance',
    url: '/certification/certifieduser/quality-assurance-v7'
  },
  {
    name: 'Scientific Computing with Python',
    url: '/certification/certifieduser/scientific-computing-with-python-v7'
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/formHelpers/form-validators.tsx (TypeScript)

Code before:
import React from 'react';
import { Trans } from 'react-i18next';

// Matches editor links for: Replit, Glitch, CodeSandbox, GitHub. NOT Codespaces, and NOT Gitpod yet
// Once safari allows negative lookbehinds, this can be used:
// |(?<!https:\/\/\d+-[\w.-]+)\.gitpod\.io
// |(?<!\.app)\.github\.dev
const editorRegex =
  /repl\.?it(\.com)?\/(@|join\/)|glitch\.com\/edit\/#!|codesandbox\.io\/s\/|github\.com/;
const fCCRegex =
  /codepen\.io\/freecodecamp|freecodecamp\.rocks|github\.com\/freecodecamp|\.freecodecamp\.org/i;
const localhostRegex = /localhost:/;
const httpRegex = /http(?!s|([^s]+?localhost))/;

function isPathRoot(urlString: string): boolean {
  try {
    return new URL(urlString).pathname !== '/';
  } catch {
    return false;
  }
}

type Validator = (value: string) => React.ReactElement | null;

export const editorValidator: Validator = value =>
  editorRegex.test(value) ? <Trans>validation.editor-url</Trans> : null;

export const fCCValidator: Validator = value =>
  fCCRegex.test(value) ? <Trans>validation.own-work-url</Trans> : null;

export const localhostValidator: Validator = value =>

Review comment (suggestion):
```suggestion
```
Unfortunately not, because https://github.com/3000 exists. Unless we know the structure of Gitpod's links, we can't reliably tell the difference between 3000's editor url and app url.

Code after:
import React from 'react';
import { Trans } from 'react-i18next';

// Matches editor links for: Replit, Glitch, CodeSandbox, GitHub. NOT Codespaces, and NOT Gitpod yet
// Once safari allows negative lookbehinds, this can be used:
// |(?<!\.app)\.github\.dev
const editorRegex =
  /repl\.?it(\.com)?\/(@|join\/)|glitch\.com\/edit\/#!|codesandbox\.io\/s\/|github\.com/;
const fCCRegex =
  /codepen\.io\/freecodecamp|freecodecamp\.rocks|github\.com\/freecodecamp|\.freecodecamp\.org/i;
const localhostRegex = /localhost:/;
const httpRegex = /http(?!s|([^s]+?localhost))/;

function isPathRoot(urlString: string): boolean {
  try {
    return new URL(urlString).pathname !== '/';
  } catch {
    return false;
  }
}

type Validator = (value: string) => React.ReactElement | null;

export const editorValidator: Validator = value =>
  editorRegex.test(value) ? <Trans>validation.editor-url</Trans> : null;

export const fCCValidator: Validator = value =>
  fCCRegex.test(value) ? <Trans>validation.own-work-url</Trans> : null;

export const localhostValidator: Validator = value =>
  localhostRegex.test(value) ? (
Repository: freeCodeCamp/freeCodeCamp
File: curriculum-server/source-curriculum.ts (TypeScript)

Code before:
import fs from 'fs/promises';

import curriculum from '../shared/config/curriculum.json';

interface Curriculum {
  [key: string]: unknown;
}
const patchedCurriculum: Curriculum = {};

for (const key in curriculum) {
  patchedCurriculum[key.replace('/', '-')] = (curriculum as Curriculum)[key];
}

void fs
  .mkdir('data', { recursive: true })
  .then(() =>
    fs.writeFile('./data/curriculum.json', JSON.stringify(patchedCurriculum))
  );


Review comment (suggestion):
```suggestion
const typedCurriculum = curriculum as Curriculum;
```
Just to avoid further type casts.

Code after:
import fs from 'fs/promises';

import curriculum from '../shared/config/curriculum.json';

interface Curriculum {
  [key: string]: unknown;
}
const typedCurriculum = curriculum as Curriculum;

const patchedCurriculum = Object.keys(typedCurriculum).reduce((acc, key) => {
  return { ...acc, [key.replace(/\//g, '-')]: typedCurriculum[key] };
}, {});

void fs
  .mkdir('data', { recursive: true })
  .then(() =>
    fs.writeFile('./data/curriculum.json', JSON.stringify(patchedCurriculum))
  );
Repository: freeCodeCamp/freeCodeCamp
File: curriculum-server/source-curriculum.ts (TypeScript)

Code before:
import fs from 'fs/promises';

import curriculum from '../shared/config/curriculum.json';

interface Curriculum {
  [key: string]: unknown;
}
const patchedCurriculum: Curriculum = {};

for (const key in curriculum) {
  patchedCurriculum[key.replace('/', '-')] = (curriculum as Curriculum)[key];
}

void fs
  .mkdir('data', { recursive: true })
  .then(() =>
    fs.writeFile('./data/curriculum.json', JSON.stringify(patchedCurriculum))
  );


Review comment (suggestion):
```suggestion
const patchedCurriculum = Object.keys(typedCurriculum).reduce((acc, key) => {
  return { ...acc, [key.replace(/\//g, '-')]: typedCurriculum[key] };
}, {});
```
Your code is a massive improvement on what I'd put together (using `split` and `join` instead of replace was particularly silly). However, I'd like to keep it functional - it doesn't hugely matter in this instance, but the more instances of side-effect free code, the better. 

Also, `replace` with a string pattern only replaces the first match, so it should use a regex.

Code after:
import fs from 'fs/promises';

import curriculum from '../shared/config/curriculum.json';

interface Curriculum {
  [key: string]: unknown;
}
const typedCurriculum = curriculum as Curriculum;

const patchedCurriculum = Object.keys(typedCurriculum).reduce((acc, key) => {
  return { ...acc, [key.replace(/\//g, '-')]: typedCurriculum[key] };
}, {});

void fs
  .mkdir('data', { recursive: true })
  .then(() =>
    fs.writeFile('./data/curriculum.json', JSON.stringify(patchedCurriculum))
  );
Repository: freeCodeCamp/freeCodeCamp
File: client/src/templates/Challenges/components/scene/scene.tsx (TypeScript)

Code before:

  const audioRef = useRef<HTMLAudioElement>(
    new Audio(`${sounds}/${audio.filename}${audioTimestamp}`)
  );

  const loadImage = (src: string) => {
    const img = new Image();
    img.src = src;
  };

  // on mount
  useEffect(() => {
    const { current } = audioRef;
    current.addEventListener('canplaythrough', audioLoaded);

    // preload images
    loadImage(`${backgrounds}/${setup.background}`);

    setup.characters.forEach(character => {
      const characterImages = characterAssets[character.character];
      Object.values(characterImages).forEach(src => {
        if (src) {
          loadImage(src);
        }
      });
    });

    commands.forEach(command => {
      if (command.background) loadImage(`${backgrounds}/${command.background}`);
    });

    // on unmount
    return () => {
      const { current } = audioRef;

      current.pause();
      current.currentTime = 0;
      current.removeEventListener('canplaythrough', audioLoaded);
    };
  }, [audioRef, setup.background, setup.characters, commands]);

  const audioLoaded = () => {
    setSceneIsReady(true);
  };

  const initBackground = setup.background;
  const initDialogue = { label: '', text: '', align: 'left' };
  const initCharacters = setup.characters.map(character => {
    return {
      ...character,
      opacity: character.opacity ?? 1,

Review comment (suggestion):
```suggestion
    setup.characters
      .map(({ character }) => Object.values(characterAssets[character]))
      .flat()
      .forEach(loadImage);
```
The linear pipeline is easier to read, imo.

Code after:

  const audioRef = useRef<HTMLAudioElement>(
    new Audio(`${sounds}/${audio.filename}${audioTimestamp}`)
  );

  const loadImage = (src: string | null) => {
    if (src) new Image().src = src;
  };

  // on mount
  useEffect(() => {
    const { current } = audioRef;
    current.addEventListener('canplaythrough', audioLoaded);

    // preload images
    loadImage(`${backgrounds}/${setup.background}`);

    setup.characters
      .map(({ character }) => Object.values(characterAssets[character]))
      .flat()
      .forEach(loadImage);

    commands
      .map(({ background }) =>
        background ? `${backgrounds}/${background}` : null
      )
      .forEach(loadImage);

    // on unmount
    return () => {
      const { current } = audioRef;

      current.pause();
      current.currentTime = 0;
      current.removeEventListener('canplaythrough', audioLoaded);
    };
  }, [audioRef, setup.background, setup.characters, commands]);

  const audioLoaded = () => {
    setSceneIsReady(true);
  };

  const initBackground = setup.background;
  const initDialogue = { label: '', text: '', align: 'left' };
  const initCharacters = setup.characters.map(character => {
    return {
      ...character,
      opacity: character.opacity ?? 1,
      isTalking: false
    };
  });
Repository: freeCodeCamp/freeCodeCamp
File: client/src/templates/Challenges/components/scene/scene.tsx (TypeScript)

Code before:

  const loadImage = (src: string) => {
    const img = new Image();
    img.src = src;
  };

  // on mount
  useEffect(() => {
    const { current } = audioRef;
    current.addEventListener('canplaythrough', audioLoaded);

    // preload images
    loadImage(`${backgrounds}/${setup.background}`);

    setup.characters.forEach(character => {
      const characterImages = characterAssets[character.character];
      Object.values(characterImages).forEach(src => {
        if (src) {
          loadImage(src);
        }
      });
    });

    commands.forEach(command => {
      if (command.background) loadImage(`${backgrounds}/${command.background}`);
    });

    // on unmount
    return () => {
      const { current } = audioRef;

      current.pause();
      current.currentTime = 0;
      current.removeEventListener('canplaythrough', audioLoaded);
    };
  }, [audioRef, setup.background, setup.characters, commands]);

  const audioLoaded = () => {
    setSceneIsReady(true);
  };

  const initBackground = setup.background;
  const initDialogue = { label: '', text: '', align: 'left' };
  const initCharacters = setup.characters.map(character => {
    return {
      ...character,
      opacity: character.opacity ?? 1,
      isTalking: false
    };
  });


Review comment (suggestion):
```suggestion
    commands
      .map(({ background }) =>
        background ? `${backgrounds}/${background}` : null
      )
      .forEach(loadImage);
```
Again, this simplifies the logical flow.

Code after:

  const loadImage = (src: string | null) => {
    if (src) new Image().src = src;
  };

  // on mount
  useEffect(() => {
    const { current } = audioRef;
    current.addEventListener('canplaythrough', audioLoaded);

    // preload images
    loadImage(`${backgrounds}/${setup.background}`);

    setup.characters
      .map(({ character }) => Object.values(characterAssets[character]))
      .flat()
      .forEach(loadImage);

    commands
      .map(({ background }) =>
        background ? `${backgrounds}/${background}` : null
      )
      .forEach(loadImage);

    // on unmount
    return () => {
      const { current } = audioRef;

      current.pause();
      current.currentTime = 0;
      current.removeEventListener('canplaythrough', audioLoaded);
    };
  }, [audioRef, setup.background, setup.characters, commands]);

  const audioLoaded = () => {
    setSceneIsReady(true);
  };

  const initBackground = setup.background;
  const initDialogue = { label: '', text: '', align: 'left' };
  const initCharacters = setup.characters.map(character => {
    return {
      ...character,
      opacity: character.opacity ?? 1,
      isTalking: false
    };
  });

  const [isPlaying, setIsPlaying] = useState(false);
  const [sceneIsReady, setSceneIsReady] = useState(true);
  const [showDialogue, setShowDialogue] = useState(false);
Repository: freeCodeCamp/freeCodeCamp
File: client/src/templates/Introduction/components/challenges.tsx (TypeScript)

Code before:
import { Link } from 'gatsby';
import React from 'react';
import { withTranslation, useTranslation } from 'react-i18next';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import type { Dispatch } from 'redux';

import GreenNotCompleted from '../../../assets/icons/green-not-completed';
import GreenPass from '../../../assets/icons/green-pass';
import { executeGA } from '../../../redux/actions';
import { ChallengeWithCompletedNode } from '../../../redux/prop-types';
import { SuperBlocks } from '../../../../../shared/config/superblocks';
import { challengeTypes } from '../../../../../shared/config/challenge-types';

const mapDispatchToProps = (dispatch: Dispatch) =>
  bindActionCreators({ executeGA }, dispatch);

interface Challenges {
  challengesWithCompleted: ChallengeWithCompletedNode[];
  isProjectBlock: boolean;
  isGridMap?: boolean;
  blockTitle?: string | null;
}

const CheckMark = ({ isCompleted }: { isCompleted: boolean }) =>
  isCompleted ? <GreenPass /> : <GreenNotCompleted />;

const Challenge = ({
  challenge
}: {
  challenge: ChallengeWithCompletedNode;
}) => (
  <Link to={challenge.fields.slug}>
    <span className='badge map-badge'>
      <CheckMark isCompleted={challenge.isCompleted} />
    </span>
    {challenge.title}
  </Link>

Review comment (suggestion):
```suggestion
import { challengeTypes } from '../../../../../shared/config/challenge-types';

const getStepNumber = (dashedName: string) => {
  // dashedName should be in the format 'step-1' or 'task-1'
  const match = dashedName.match(/-(\d+)/);
  return match ? match[1] : '';
};
```
It's a bit verbose, but I think it's clearer as a result.

Code after:
import { Link } from 'gatsby';
import React from 'react';
import { withTranslation, useTranslation } from 'react-i18next';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import type { Dispatch } from 'redux';

import GreenNotCompleted from '../../../assets/icons/green-not-completed';
import GreenPass from '../../../assets/icons/green-pass';
import { executeGA } from '../../../redux/actions';
import { ChallengeWithCompletedNode } from '../../../redux/prop-types';
import { SuperBlocks } from '../../../../../shared/config/superblocks';
import { challengeTypes } from '../../../../../shared/config/challenge-types';

const getStepNumber = (dashedName: string) => {
  // dashedName should be in the format 'step-1' or 'task-1'
  const match = dashedName.match(/-(\d+)/);
  return match ? match[1] : '';
};

const mapDispatchToProps = (dispatch: Dispatch) =>
  bindActionCreators({ executeGA }, dispatch);

interface Challenges {
  challengesWithCompleted: ChallengeWithCompletedNode[];
  isProjectBlock: boolean;
  isGridMap?: boolean;
  blockTitle?: string | null;
}

const CheckMark = ({ isCompleted }: { isCompleted: boolean }) =>
  isCompleted ? <GreenPass /> : <GreenNotCompleted />;

const Challenge = ({
  challenge
}: {
  challenge: ChallengeWithCompletedNode;
}) => (
Repository: freeCodeCamp/freeCodeCamp
File: client/src/templates/Introduction/components/help-translate.tsx (TypeScript)

Code before:

  if (
    !isAuditedSuperBlock(clientLocale, superBlock, {
      showNewCurriculum,
      showUpcomingChanges
    })
  ) {
    return (
      <>
        <div style={{ textAlign: 'center' }}>
          <Spacer size='medium' />
          <p style={{ marginBottom: 0 }}>{t('learn.help-translate')} </p>
          <Link
            external={true}
            sameTab={false}
            to={t('links:help-translate-link-url')}
          >
            {t('learn.help-translate-link')}
          </Link>
        </div>
      </>
    );
  } else {
    return null;
  }
}

export default HelpTranslate;


Review comment (suggestion):
```suggestion
function HelpTranslate({ superBlock }: HelpTranslateProps): JSX.Element | null {
  const { t } = useTranslation();

  if (isAuditedSuperBlock(clientLocale, superBlock, {
    showNewCurriculum,
    showUpcomingChanges
  })) {
    return null;
  }

  return (
    <div style={{ textAlign: 'center' }}>
      <Spacer size='medium' />
      <p style={{ marginBottom: 0 }}>{t('learn.help-translate')} </p>
      <Link
        external={true}
        sameTab={false}
        to={t('links:help-translate-link-url')}
      >
        {t('learn.help-translate-link')}
      Link>
    </div>
  );
}

```

Code after:

  if (
    isAuditedSuperBlock(clientLocale, superBlock, {
      showNewCurriculum,
      showUpcomingChanges
    })
  ) {
    return null;
  }

  return (
    <div style={{ textAlign: 'center' }}>
      <Spacer size='medium' />
      <p style={{ marginBottom: 0 }}>{t('learn.help-translate')} </p>
      <Link
        external={true}
        sameTab={false}
        to={t('links:help-translate-link-url')}
      >
        {t('learn.help-translate-link')}
      </Link>
    </div>
  );
}

export default HelpTranslate;
Repository: freeCodeCamp/freeCodeCamp
File: client/src/templates/Introduction/components/legacy-links.tsx (TypeScript)

Code before:
            </Link>
          </p>
        </Alert>
      </>
    );
  } else if (isRelationalDbCert(superBlock)) {
    return (
      <>
        <CodeAllyDown />
        {clientLocale != 'english' && (
          <Alert variant='info'>
            <p>{t('intro:misc-text.english-only')}</p>
          </Alert>
        )}
      </>
    );
  } else if (isExamCert(superBlock)) {
    return (
      <>
        {clientLocale != 'english' && (
          <Alert variant='info'>
            <p>{t('intro:misc-text.exam-english-only')}</p>
          </Alert>
        )}
      </>
    );
  } else {
    return (
      <>
        <GitpodNote superBlock={superBlock} />
      </>
    );
  }
}

export default LegacyLinks;


Review comment (suggestion):
Could we move the condition to the `else if` statement? We can also remove the fragment (`<>`) since we are only displaying one element (`<Alert>`).

```suggestion
  } else if (isExamCert(superBlock) && clientLocale != 'english') {
    return (
      <Alert variant='info'>
        <p>{t('intro:misc-text.exam-english-only')}</p>
      </Alert>
    );
```

Code after:
            </Link>
          </p>
        </Alert>
      </>
    );
  } else if (isRelationalDbCert(superBlock)) {
    return (
      <>
        <CodeAllyDown />
        {clientLocale != 'english' && (
          <Alert variant='info'>
            <p>{t('intro:misc-text.english-only')}</p>
          </Alert>
        )}
      </>
    );
  } else if (isExamCert(superBlock) && clientLocale != 'english') {
    return (
      <Alert variant='info'>
        <p>{t('intro:misc-text.exam-english-only')}</p>
      </Alert>
    );
  } else {
    return (
      <>
        <GitpodNote superBlock={superBlock} />
      </>
    );
  }
}

export default LegacyLinks;