ronantakizawa/codereview-bench huggingface dataset:
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/Header/components/login.tsx (TypeScript)

Code before:

const Login = ({
  block,
  children,
  'data-test-label': dataTestLabel,
  isSignedIn
}: LoginProps): JSX.Element => {
  const { t } = useTranslation();

  const href = isSignedIn ? `${homeLocation}/learn` : `${apiLocation}/signin`;
  return (
    <a
      className={(block ? 'btn-cta-big btn-block' : '') + ' signup-btn btn-cta'}
      data-test-label={dataTestLabel}
      data-playwright-test-label='header-sign-in-button'
      href={href}
      onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
        // Prevent default to make sure GA is called before the redirect
        e?.preventDefault();
        callGA({
          event: 'sign_in'
        });
        // Ensure async operations are completed
        setTimeout(() => {
          window.location.href = href;
        }, 0); //
      }}
    >
      <span className='login-btn-icon'>
        <FontAwesomeIcon icon={faRightToBracket} />
        <span className='sr-only'> {t('buttons.sign-in')}</span>
      </span>
      <span className='login-btn-text'>{children || t('buttons.sign-in')}</span>
    </a>
  );
};

Login.displayName = 'Login';

export default connect(mapStateToProps)(Login);


Review comment (bug):
I think this is worse. If you really _just_ want to know when the button is clicked, then what you had was fine.

My previous point was, Campers could click the button, fail to be signed in, and you will still get _x number of sign_in events_.



Code after:

const Login = ({
  block,
  children,
  'data-test-label': dataTestLabel,
  isSignedIn
}: LoginProps): JSX.Element => {
  const { t } = useTranslation();

  const href = isSignedIn ? `${homeLocation}/learn` : `${apiLocation}/signin`;
  return (
    <a
      className={(block ? 'btn-cta-big btn-block' : '') + ' signup-btn btn-cta'}
      data-test-label={dataTestLabel}
      data-playwright-test-label='header-sign-in-button'
      href={href}
      onClick={() => {
        callGA({
          event: 'sign_in'
        });
      }}
    >
      <span className='login-btn-icon'>
        <FontAwesomeIcon icon={faRightToBracket} />
        <span className='sr-only'> {t('buttons.sign-in')}</span>
      </span>
      <span className='login-btn-text'>{children || t('buttons.sign-in')}</span>
    </a>
  );
};

Login.displayName = 'Login';

export default connect(mapStateToProps)(Login);
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/Donation/donation-modal.tsx (TypeScript)

Code before:
      callGA({
        event: 'donation_view',
        action: `Displayed ${
          recentlyClaimedBlock !== null ? 'Block' : 'Progress'
        } Donation Modal`
      });
    }
  }, [show, recentlyClaimedBlock]);

  const handleModalHide = () => {
    // If modal is open on a SuperBlock page
    if (isLocationSuperBlock(location)) {
      goToAnchor('claim-cert-block');
    }
  };

  return (
    <Modal
      aria-label='donation-modal'
      open={show}
      onClose={handleModalHide}
      size='large'
      variant='default'
    >
      <Modal.Body>
        <DonationModalBody
          closeDonationModal={closeDonationModal}
          recentlyClaimedBlock={recentlyClaimedBlock}
        />
      </Modal.Body>
    </Modal>
  );
}

DonateModal.displayName = 'DonateModal';

export default connect(mapStateToProps, mapDispatchToProps)(DonateModal);


Review comment (suggestion):
Sem, did you know that you chose the most complicated modal to start the migration? 😄 

We will need to update `DonationModalBody`, too, to use ui-components Modal. `Modal.Body` would be placed in there instead of this file.

We will also need to add `Modal.Header` to the modal, as the header contains a heading (`h2`, to be specific), which is used as the name/title of the modal. The header is needed in order for the modal to be accessible.



Code after:
        event: 'donation_view',
        action: `Displayed ${
          recentlyClaimedBlock !== null ? 'Block' : 'Progress'
        } Donation Modal`
      });
    }
  }, [show, recentlyClaimedBlock]);

  const handleModalHide = () => {
    // If modal is open on a SuperBlock page
    if (isLocationSuperBlock(location)) {
      goToAnchor('claim-cert-block');
    }
  };

  return (
    <Modal
      bsSize='lg'
      className='donation-modal'
      onExited={handleModalHide}
      show={show}
      data-cy='donation-modal'
    >
      <DonationModalBody
        closeDonationModal={closeDonationModal}
        recentlyClaimedBlock={recentlyClaimedBlock}
      />
    </Modal>
  );
}

DonateModal.displayName = 'DonateModal';

export default connect(mapStateToProps, mapDispatchToProps)(DonateModal);
Repository: freeCodeCamp/freeCodeCamp
File: e2e/update-email.spec.ts (TypeScript)

Code before:
    page
  }) => {
    const emailInput = page.getByLabel(translations.misc.email);
    const submitButton = page.getByRole('button', { name: 'Update my Email' });

    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123');
    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123@gmail.com');
    await expect(submitButton).toBeEnabled();
  });
});

test.describe('The update-email page when the user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

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

  test('should redirect to the signin page', async ({ page }) => {
    if (process.env.CI) {
      await page.waitForURL('**/learn/');
    } else {
      await page.waitForURL('**/learn');
    }

    await expect(
      page.getByRole('heading', { name: 'Welcome back, Full Stack User' })
    ).toBeVisible();
  });
});


Review comment (bug):
Initially I had:

```ts
await page.waitForURL('**/learn');
```

But the test failed in CI while passed locally.

It took me a while to figure this out, and a trailing slash is what makes the difference. I tried adding:

```ts
await expect(page).toHaveURL('/learn');
```

And CI failed with:

<img width="359" alt="Screenshot 2024-06-18 at 14 22 41" src="https://github.com/freeCodeCamp/freeCodeCamp/assets/25715018/7730d630-eaca-4539-92e4-c1d2ebf31d5c">


Code after:
    page
  }) => {
    const emailInput = page.getByLabel(translations.misc.email);
    const submitButton = page.getByRole('button', { name: 'Update my Email' });

    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123');
    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123@gmail.com');
    await expect(submitButton).toBeEnabled();
  });
});

test.describe('The update-email page when the user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

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

  test('should sign the user in and redirect them to /learn', async ({
    page,
    browserName
  }) => {
    // The signin step involves multiple navigations, which results a network error in Firefox.
    // The error is harmless but Playwright doesn't suppress it, causing the test to fail.
    // Ref: https://github.com/microsoft/playwright/issues/20749
    test.skip(browserName === 'firefox');

    await page.waitForURL(/\/learn\/?/);

    await expect(
      page.getByRole('heading', { name: 'Welcome back, Full Stack User' })
    ).toBeVisible();
  });
});
Repository: freeCodeCamp/freeCodeCamp
File: client/src/client-only-routes/show-update-email.tsx (TypeScript)

Code before:
import { Link } from 'gatsby';
import { isString } from 'lodash-es';
import React, { useState, type FormEvent, type ChangeEvent } from 'react';
import Helmet from 'react-helmet';
import { useTranslation } from 'react-i18next';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import type { Dispatch } from 'redux';
import { createSelector } from 'reselect';
import isEmail from 'validator/lib/isEmail';
import {
  Container,
  FormGroup,
  FormControl,
  ControlLabel,
  Col,
  Row,
  Button
} from '@freecodecamp/ui';

import envData from '../../config/env.json';
import { Spacer, Loader } from '../components/helpers';
import './show-update-email.css';
import { isSignedInSelector, userSelector } from '../redux/selectors';
import { hardGoTo as navigate } from '../redux/actions';
import { updateMyEmail } from '../redux/settings/actions';
import { maybeEmailRE } from '../utils';


Review comment (refactor):
```suggestion
import React, {
  useState,
  useEffect,
  type FormEvent,
  type ChangeEvent
} from 'react';
```
I know it's unrelated, but a little refactoring would be good.

Code after:
import { Link } from 'gatsby';
import React, {
  useState,
  useEffect,
  type FormEvent,
  type ChangeEvent
} from 'react';
import Helmet from 'react-helmet';
import { useTranslation } from 'react-i18next';
import { connect } from 'react-redux';
import { createSelector } from 'reselect';
import isEmail from 'validator/lib/isEmail';
import {
  Container,
  FormGroup,
  FormControl,
  ControlLabel,
  Col,
  Row,
  Button
} from '@freecodecamp/ui';

import envData from '../../config/env.json';
import { Spacer, Loader } from '../components/helpers';
import './show-update-email.css';
import { isSignedInSelector, userSelector } from '../redux/selectors';
import { hardGoTo as navigate } from '../redux/actions';
import { updateMyEmail } from '../redux/settings/actions';
Repository: freeCodeCamp/freeCodeCamp
File: client/src/client-only-routes/show-update-email.tsx (TypeScript)

Code before:
import { maybeEmailRE } from '../utils';

const { apiLocation } = envData;

interface ShowUpdateEmailProps {
  isNewEmail: boolean;
  updateMyEmail: (e: string) => void;
  path?: string;
  isSignedIn: boolean;
  navigate: (location: string) => void;
}

const mapStateToProps = createSelector(
  userSelector,
  isSignedInSelector,
  (
    { email, emailVerified }: { email: string; emailVerified: boolean },
    isSignedIn
  ) => ({
    isNewEmail: !email || emailVerified,
    isSignedIn
  })
);

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

function ShowUpdateEmail({
  isNewEmail,
  updateMyEmail,
  isSignedIn,
  navigate
}: ShowUpdateEmailProps) {
  const { t } = useTranslation();
  const [emailValue, setEmailValue] = useState('');

  if (!isSignedIn) {
    navigate(`${apiLocation}/signin`);
    return <Loader fullScreen={true} />;
  }

  function handleSubmit(event: FormEvent) {
    event.preventDefault();
    updateMyEmail(emailValue);
  }

  function onChange(event: ChangeEvent<HTMLInputElement>) {
    const newEmailValue = event.target.value;
    if (!isString(newEmailValue)) {
      return null;
    }

Review comment (suggestion):
```suggestion
const mapDispatchToProps = { updateMyEmail, navigate };
```
Same thing, but less boilerplate: https://react-redux.js.org/using-react-redux/connect-mapdispatch#defining-mapdispatchtoprops-as-an-object

Code after:
import { hardGoTo as navigate } from '../redux/actions';
import { updateMyEmail } from '../redux/settings/actions';
import { maybeEmailRE } from '../utils';

const { apiLocation } = envData;

interface ShowUpdateEmailProps {
  isNewEmail: boolean;
  updateMyEmail: (e: string) => void;
  path?: string;
  isSignedIn: boolean;
  navigate: (location: string) => void;
}

const mapStateToProps = createSelector(
  userSelector,
  isSignedInSelector,
  (
    { email, emailVerified }: { email: string; emailVerified: boolean },
    isSignedIn
  ) => ({
    isNewEmail: !email || emailVerified,
    isSignedIn
  })
);

const mapDispatchToProps = { updateMyEmail, navigate };

function ShowUpdateEmail({
  isNewEmail,
  updateMyEmail,
  isSignedIn,
  navigate
}: ShowUpdateEmailProps) {
  const { t } = useTranslation();
  const [emailValue, setEmailValue] = useState('');

  useEffect(() => {
    if (!isSignedIn) navigate(`${apiLocation}/signin`);
  }, [isSignedIn, navigate]);
  if (!isSignedIn) return <Loader fullScreen={true} />;

  function handleSubmit(event: FormEvent) {
    event.preventDefault();
    updateMyEmail(emailValue);
  }

  function onChange(event: ChangeEvent<HTMLInputElement>) {
    setEmailValue(event.target.value);
    return null;
  }
Repository: freeCodeCamp/freeCodeCamp
File: client/src/client-only-routes/show-update-email.tsx (TypeScript)

Code before:
  isSignedInSelector,
  (
    { email, emailVerified }: { email: string; emailVerified: boolean },
    isSignedIn
  ) => ({
    isNewEmail: !email || emailVerified,
    isSignedIn
  })
);

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

function ShowUpdateEmail({
  isNewEmail,
  updateMyEmail,
  isSignedIn,
  navigate
}: ShowUpdateEmailProps) {
  const { t } = useTranslation();
  const [emailValue, setEmailValue] = useState('');

  if (!isSignedIn) {
    navigate(`${apiLocation}/signin`);
    return <Loader fullScreen={true} />;
  }

  function handleSubmit(event: FormEvent) {
    event.preventDefault();
    updateMyEmail(emailValue);
  }

  function onChange(event: ChangeEvent<HTMLInputElement>) {
    const newEmailValue = event.target.value;
    if (!isString(newEmailValue)) {
      return null;
    }
    setEmailValue(newEmailValue);
    return null;
  }

  function getEmailValidationState() {
    if (maybeEmailRE.test(emailValue)) {
      return isEmail(emailValue) ? 'success' : 'error';
    }
    return null;
  }

  return (
    <>
      <Helmet>

Review comment (bug):
```suggestion
  useEffect(() => {
    if (!isSignedIn) navigate(`${apiLocation}/signin`);
  }, [isSignedIn, navigate]);
  if (!isSignedIn) return <Loader fullScreen={true} />;
```
While this isn't broken, it's a good practice to confine side effects to useEffect hooks.

Code after:
const mapStateToProps = createSelector(
  userSelector,
  isSignedInSelector,
  (
    { email, emailVerified }: { email: string; emailVerified: boolean },
    isSignedIn
  ) => ({
    isNewEmail: !email || emailVerified,
    isSignedIn
  })
);

const mapDispatchToProps = { updateMyEmail, navigate };

function ShowUpdateEmail({
  isNewEmail,
  updateMyEmail,
  isSignedIn,
  navigate
}: ShowUpdateEmailProps) {
  const { t } = useTranslation();
  const [emailValue, setEmailValue] = useState('');

  useEffect(() => {
    if (!isSignedIn) navigate(`${apiLocation}/signin`);
  }, [isSignedIn, navigate]);
  if (!isSignedIn) return <Loader fullScreen={true} />;

  function handleSubmit(event: FormEvent) {
    event.preventDefault();
    updateMyEmail(emailValue);
  }

  function onChange(event: ChangeEvent<HTMLInputElement>) {
    setEmailValue(event.target.value);
    return null;
  }

  const emailValidationState = maybeEmailRE.test(emailValue)
    ? isEmail(emailValue)
      ? 'success'
      : 'error'
    : null;

  return (
    <>
      <Helmet>
        <title>{t('misc.update-email-1')} | freeCodeCamp.org</title>
      </Helmet>
      <Container>
        <Spacer size='medium' />
Repository: freeCodeCamp/freeCodeCamp
File: client/src/client-only-routes/show-update-email.tsx (TypeScript)

Code before:

function ShowUpdateEmail({
  isNewEmail,
  updateMyEmail,
  isSignedIn,
  navigate
}: ShowUpdateEmailProps) {
  const { t } = useTranslation();
  const [emailValue, setEmailValue] = useState('');

  if (!isSignedIn) {
    navigate(`${apiLocation}/signin`);
    return <Loader fullScreen={true} />;
  }

  function handleSubmit(event: FormEvent) {
    event.preventDefault();
    updateMyEmail(emailValue);
  }

  function onChange(event: ChangeEvent<HTMLInputElement>) {
    const newEmailValue = event.target.value;
    if (!isString(newEmailValue)) {
      return null;
    }
    setEmailValue(newEmailValue);
    return null;
  }

  function getEmailValidationState() {
    if (maybeEmailRE.test(emailValue)) {
      return isEmail(emailValue) ? 'success' : 'error';
    }
    return null;
  }

  return (
    <>
      <Helmet>
        <title>{t('misc.update-email-1')} | freeCodeCamp.org</title>
      </Helmet>
      <Container>
        <Spacer size='medium' />
        <h2 className='text-center'>{t('misc.update-email-2')}</h2>
        <Row>
          <Col sm={6} smOffset={3}>
            <Row>
              <form
                onSubmit={handleSubmit}
                data-playwright-test-label='update-email-form'
              >

Review comment (suggestion):
```suggestion
    setEmailValue(event.target.value);
```
It is a string, so we can keep this simple.

Code after:
const mapDispatchToProps = { updateMyEmail, navigate };

function ShowUpdateEmail({
  isNewEmail,
  updateMyEmail,
  isSignedIn,
  navigate
}: ShowUpdateEmailProps) {
  const { t } = useTranslation();
  const [emailValue, setEmailValue] = useState('');

  useEffect(() => {
    if (!isSignedIn) navigate(`${apiLocation}/signin`);
  }, [isSignedIn, navigate]);
  if (!isSignedIn) return <Loader fullScreen={true} />;

  function handleSubmit(event: FormEvent) {
    event.preventDefault();
    updateMyEmail(emailValue);
  }

  function onChange(event: ChangeEvent<HTMLInputElement>) {
    setEmailValue(event.target.value);
    return null;
  }

  const emailValidationState = maybeEmailRE.test(emailValue)
    ? isEmail(emailValue)
      ? 'success'
      : 'error'
    : null;

  return (
    <>
      <Helmet>
        <title>{t('misc.update-email-1')} | freeCodeCamp.org</title>
      </Helmet>
      <Container>
        <Spacer size='medium' />
        <h2 className='text-center'>{t('misc.update-email-2')}</h2>
        <Row>
          <Col sm={6} smOffset={3}>
            <Row>
              <form
                onSubmit={handleSubmit}
                data-playwright-test-label='update-email-form'
              >
                <FormGroup
                  className='update-email-field'
                  validationState={emailValidationState}
                >
Repository: freeCodeCamp/freeCodeCamp
File: client/src/client-only-routes/show-update-email.tsx (TypeScript)

Code before:
          <Col sm={6} smOffset={3}>
            <Row>
              <form
                onSubmit={handleSubmit}
                data-playwright-test-label='update-email-form'
              >
                <FormGroup
                  className='update-email-field'
                  validationState={getEmailValidationState()}
                >
                  <ControlLabel htmlFor='emailInput'>
                    {t('misc.email')}
                  </ControlLabel>
                  <FormControl
                    id='emailInput'
                    onChange={onChange}
                    placeholder='camperbot@example.com'
                    required={true}
                    type='email'
                  />
                </FormGroup>
                <Button
                  block={true}
                  size='large'
                  variant='primary'
                  disabled={getEmailValidationState() !== 'success'}
                  type='submit'
                >
                  {isNewEmail
                    ? t('buttons.update-email')
                    : t('buttons.verify-email')}
                </Button>
              </form>
              <p className='text-center'>
                <Link to='/signout'>{t('buttons.sign-out')}</Link>
              </p>
            </Row>
          </Col>
        </Row>
      </Container>
    </>
  );
}

ShowUpdateEmail.displayName = 'ShowUpdateEmail';

export default connect(mapStateToProps, mapDispatchToProps)(ShowUpdateEmail);


Review comment (suggestion):
```suggestion
                  disabled={emailValidationState !== 'success'}
```

Code after:
                data-playwright-test-label='update-email-form'
              >
                <FormGroup
                  className='update-email-field'
                  validationState={emailValidationState}
                >
                  <ControlLabel htmlFor='emailInput'>
                    {t('misc.email')}
                  </ControlLabel>
                  <FormControl
                    id='emailInput'
                    onChange={onChange}
                    placeholder='camperbot@example.com'
                    required={true}
                    type='email'
                  />
                </FormGroup>
                <Button
                  block={true}
                  size='large'
                  variant='primary'
                  disabled={emailValidationState !== 'success'}
                  type='submit'
                >
                  {isNewEmail
                    ? t('buttons.update-email')
                    : t('buttons.verify-email')}
                </Button>
              </form>
              <p className='text-center'>
                <Link to='/signout'>{t('buttons.sign-out')}</Link>
              </p>
            </Row>
          </Col>
        </Row>
      </Container>
    </>
  );
}

ShowUpdateEmail.displayName = 'ShowUpdateEmail';

export default connect(mapStateToProps, mapDispatchToProps)(ShowUpdateEmail);
Repository: freeCodeCamp/freeCodeCamp
File: e2e/update-email.spec.ts (TypeScript)

Code before:
    page
  }) => {
    const emailInput = page.getByLabel(translations.misc.email);
    const submitButton = page.getByRole('button', { name: 'Update my Email' });

    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123');
    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123@gmail.com');
    await expect(submitButton).toBeEnabled();
  });
});

test.describe('The update-email page when the user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

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

  test('should redirect to the signin page', async ({ page }) => {
    if (process.env.CI) {
      await page.waitForURL('**/learn/');
    } else {
      await page.waitForURL('**/learn');
    }

    await expect(
      page.getByRole('heading', { name: 'Welcome back, Full Stack User' })
    ).toBeVisible();
  });
});


Review comment (suggestion):
```suggestion
    await page.waitForURL(/\/learn\/?/);
```
I'm in two minds about this if this is an improvement, but it is a little more concise :shrug: 

Code after:
    page
  }) => {
    const emailInput = page.getByLabel(translations.misc.email);
    const submitButton = page.getByRole('button', { name: 'Update my Email' });

    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123');
    await expect(submitButton).toBeDisabled();
    await emailInput.fill('123@gmail.com');
    await expect(submitButton).toBeEnabled();
  });
});

test.describe('The update-email page when the user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

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

  test('should sign the user in and redirect them to /learn', async ({
    page,
    browserName
  }) => {
    // The signin step involves multiple navigations, which results a network error in Firefox.
    // The error is harmless but Playwright doesn't suppress it, causing the test to fail.
    // Ref: https://github.com/microsoft/playwright/issues/20749
    test.skip(browserName === 'firefox');

    await page.waitForURL(/\/learn\/?/);

    await expect(
      page.getByRole('heading', { name: 'Welcome back, Full Stack User' })
    ).toBeVisible();
  });
});