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

Code before:
import { test, expect } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/2022/responsive-web-design/learn-html-by-building-a-cat-photo-app/step-2'
  );
});

test.describe('Challenge Preview Component Tests', () => {
  test('should render correct ouptput of default code', async ({
    page,
    isMobile
  }) => {
    if (isMobile) {
      await page
        .getByRole('tab', { name: translations.learn['editor-tabs'].preview })
        .click();
    }
    await expect(
      page
        .frameLocator('.challenge-preview-frame')
        .first()
        .getByRole('heading', { name: 'CatPhotoApp' })
    ).toBeVisible();
  });

  test('should render correct output of changed code', async ({
    page,
    isMobile
  }) => {
    await page.getByLabel('Editor content').click();
    await page.keyboard.insertText('<h1>FreeCodeCamp</h1>');
    if (isMobile) {
      await page

Review comment (suggestion):
Nit: Since this is a test suite, we don't need to have "Tests" in its title

```suggestion
test.describe('Challenge Preview Component', () => {
```

Code after:
import { test, expect } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/2022/responsive-web-design/learn-html-by-building-a-cat-photo-app/step-2'
  );
});

test.describe('Challenge Preview Component', () => {
  test('should render correct output of default code', async ({
    page,
    isMobile
  }) => {
    if (isMobile) {
      await page
        .getByRole('tab', { name: translations.learn['editor-tabs'].preview })
        .click();
    }
    await expect(
      page
        .frameLocator('.challenge-preview-frame')
        .first()
        .getByRole('heading', { name: 'CatPhotoApp' })
    ).toBeVisible();
  });

  test('should render correct output of changed code', async ({
    page,
    isMobile
  }) => {
    await page.getByLabel('Editor content').click();
    await page.keyboard.insertText('<h1>FreeCodeCamp</h1>');
    if (isMobile) {
      await page
Repository: freeCodeCamp/freeCodeCamp
File: e2e/preview.spec.ts (TypeScript)

Code before:
import { test, expect } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/2022/responsive-web-design/learn-html-by-building-a-cat-photo-app/step-2'
  );
});

test.describe('Challenge Preview Component Tests', () => {
  test('should render correct ouptput of default code', async ({
    page,
    isMobile
  }) => {
    if (isMobile) {
      await page
        .getByRole('tab', { name: translations.learn['editor-tabs'].preview })
        .click();
    }
    await expect(
      page
        .frameLocator('.challenge-preview-frame')
        .first()
        .getByRole('heading', { name: 'CatPhotoApp' })
    ).toBeVisible();
  });

  test('should render correct output of changed code', async ({
    page,
    isMobile
  }) => {
    await page.getByLabel('Editor content').click();
    await page.keyboard.insertText('<h1>FreeCodeCamp</h1>');
    if (isMobile) {
      await page
        .getByRole('tab', { name: translations.learn['editor-tabs'].preview })

Review comment (suggestion):
```suggestion
  test('should render correct output of default code', async ({
```

Code after:
import { test, expect } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/2022/responsive-web-design/learn-html-by-building-a-cat-photo-app/step-2'
  );
});

test.describe('Challenge Preview Component', () => {
  test('should render correct output of default code', async ({
    page,
    isMobile
  }) => {
    if (isMobile) {
      await page
        .getByRole('tab', { name: translations.learn['editor-tabs'].preview })
        .click();
    }
    await expect(
      page
        .frameLocator('.challenge-preview-frame')
        .first()
        .getByRole('heading', { name: 'CatPhotoApp' })
    ).toBeVisible();
  });

  test('should render correct output of changed code', async ({
    page,
    isMobile
  }) => {
    await page.getByLabel('Editor content').click();
    await page.keyboard.insertText('<h1>FreeCodeCamp</h1>');
    if (isMobile) {
      await page
        .getByRole('tab', { name: translations.learn['editor-tabs'].preview })
Repository: freeCodeCamp/freeCodeCamp
File: e2e/lower-jaw.spec.ts (TypeScript)

Code before:

  await checkButton.click();

  const failing = page.getByTestId('lowerJaw-failing-test-feedback');
  const hint = page.getByTestId('lowerJaw-failing-hint');
  await expect(failing).toBeVisible();
  await expect(hint).toBeVisible();
});

test('Click on the "Reset" button', async ({ page }) => {
  const resetButton = page.getByTestId('lowerJaw-reset-button');
  await resetButton.click();

  const resetModal = page.getByTestId('reset-modal');

  await expect(resetModal).toBeVisible();
});

test('Should render UI correctly', async ({ page }) => {
  const codeCheckButton = page.getByRole('button', {
    name: 'Check Your Code'
  });
  const lowerJawTips = page.getByTestId('failing-test-feedback');
  await expect(codeCheckButton).toHaveText('Check Your Code (Ctrl + Enter)');
  await expect(lowerJawTips).toHaveCount(0);
  await codeCheckButton.click();
});

test('Should display full button text on desktop but hide (Ctrl + Enter on mobile)', async ({
  page,
  isMobile
}) => {
  const codeCheckButton = page.getByRole('button', {
    name: 'Check Your Code'
  });
  await expect(codeCheckButton).toHaveText('Check Your Code (Ctrl + Enter)');

  if (isMobile) {
    await expect(codeCheckButton).toHaveText('Check Your Code');
  }
});


Review comment (suggestion):
Since the test case is for ensuring the UI is rendered correctly, we don't need to perform a click action here.

```suggestion
```

Code after:

  await checkButton.click();

  const failing = page.getByTestId('lowerJaw-failing-test-feedback');
  const hint = page.getByTestId('lowerJaw-failing-hint');
  await expect(failing).toBeVisible();
  await expect(hint).toBeVisible();
});

test('Click on the "Reset" button', async ({ page }) => {
  const resetButton = page.getByTestId('lowerJaw-reset-button');
  await resetButton.click();

  const resetModal = page.getByTestId('reset-modal');

  await expect(resetModal).toBeVisible();
});

test('Should render UI correctly', async ({ page }) => {
  const codeCheckButton = page.getByRole('button', {
    name: 'Check Your Code'
  });
  const lowerJawTips = page.getByTestId('failing-test-feedback');
  await expect(codeCheckButton).toHaveText('Check Your Code (Ctrl + Enter)');
  await expect(lowerJawTips).toHaveCount(0);
});

test('Should display full button text on desktop but hide (Ctrl + Enter on mobile)', async ({
  page,
  isMobile
}) => {
  const codeCheckButton = page.getByRole('button', {
    name: 'Check Your Code'
  });
  await expect(codeCheckButton).toHaveText('Check Your Code (Ctrl + Enter)');

  if (isMobile) {
    await expect(codeCheckButton).toHaveText('Check Your Code');
  }
});
Repository: freeCodeCamp/freeCodeCamp
File: e2e/side-panel.spec.ts (TypeScript)

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

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/javascript-algorithms-and-data-structures/basic-javascript/declare-javascript-variables'
  );
});

test.describe('Challenge Side Panel Component Tests', () => {
  test('should render correctly', async ({ page }) => {
    await expect(page.getByTestId('challenge-title')).toBeVisible();
    await expect(page.getByTestId('challenge-description')).toBeVisible();
    await expect(page.getByTestId('get-help-dropdown')).toBeVisible();
    await expect(page.getByTestId('test-result')).toBeVisible();
  });
});


Review comment (suggestion):
Nit: Since this is a test suite, we don't need to have "Tests" in the suite title.

```suggestion
test.describe('Challenge Side Panel Component', () => {
```

Code after:
import { test, expect } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/javascript-algorithms-and-data-structures/basic-javascript/declare-javascript-variables'
  );
});

test.describe('Challenge Side Panel Component', () => {
  test('should render correctly', async ({ page, isMobile }) => {
    const toolPanelItem = page.getByText(translations.buttons['get-help']);
    if (isMobile) {
      await expect(toolPanelItem).not.toBeVisible();
    } else {
      await expect(toolPanelItem).toBeVisible();
    }
    await expect(page.getByTestId('challenge-title')).toBeVisible();
    await expect(page.getByTestId('challenge-description')).toBeVisible();
    await expect(page.getByTestId('test-result')).toBeVisible();
  });
});
Repository: freeCodeCamp/freeCodeCamp
File: curriculum/test/test-challenges.js (JavaScript)

Code before:
  }
  return oldRunnerFail.call(this, test, err);
};

async function newPageContext(browser) {
  const page = await browser.newPage();
  // it's needed for workers as context.
  await page.goto('http://127.0.0.1:8080/index.html');
  return page;
}

spinner.start();
spinner.text = 'Populate tests.';

let browser;
let page;

setup()
  .then(runTests)
  .catch(err => handleRejection(err));

async function setup() {
  if (
    [
      process.env.FCC_BLOCK,
      process.env.FCC_ID,
      process.env.FCC_SUPERBLOCK
    ].filter(Boolean).length > 1
  ) {
    throw new Error(
      `Please use at most single input from: block, id, superblock.`
    );
  }

  // liveServer starts synchronously
  liveServer.start({
    host: '127.0.0.1',
    port: '8080',
    root: path.resolve(__dirname, 'stubs'),
    mount: [['/js', path.join(clientPath, 'static/js')]],
    open: false,
    logLevel: 0
  });
  browser = await puppeteer.launch({
    args: [
      // Required for Docker version of Puppeteer
      '--no-sandbox',
      '--disable-setuid-sandbox',
      // This will write shared memory files into /tmp instead of /dev/shm,
      // because Docker’s default for /dev/shm is 64MB
      '--disable-dev-shm-usage'

Review comment (suggestion):
Let's rename this to `FCC_CHALLENGE_ID`. It's more specific that way.

Code after:
  }
  return oldRunnerFail.call(this, test, err);
};

async function newPageContext(browser) {
  const page = await browser.newPage();
  // it's needed for workers as context.
  await page.goto('http://127.0.0.1:8080/index.html');
  return page;
}

spinner.start();
spinner.text = 'Populate tests.';

let browser;
let page;

setup()
  .then(runTests)
  .catch(err => handleRejection(err));

async function setup() {
  if (
    [
      process.env.FCC_BLOCK,
      process.env.FCC_CHALLENGE_ID,
      process.env.FCC_SUPERBLOCK
    ].filter(Boolean).length > 1
  ) {
    throw new Error(
      `Please use at most single input from: block, challenge id, superblock.`
    );
  }

  // liveServer starts synchronously
  liveServer.start({
    host: '127.0.0.1',
    port: '8080',
    root: path.resolve(__dirname, 'stubs'),
    mount: [['/js', path.join(clientPath, 'static/js')]],
    open: false,
    logLevel: 0
  });
  browser = await puppeteer.launch({
    args: [
      // Required for Docker version of Puppeteer
      '--no-sandbox',
      '--disable-setuid-sandbox',
      // This will write shared memory files into /tmp instead of /dev/shm,
      // because Docker’s default for /dev/shm is 64MB
      '--disable-dev-shm-usage'
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/button/button.tsx (TypeScript)

Code before:
import { ButtonProps, ButtonSize, ButtonVariant } from './types';

const defaultClassNames = [
  // Positioning and spacing
  'relative',
  'inline-block',
  'mt-[0.5px]',
  // Border
  'border-solid',
  'border-3',
  // Active state
  'active:before:w-full',
  'active:before:h-full',
  'active:before:absolute',
  'active:before:inset-0',
  'active:before:border-3',
  'active:before:border-transparent',
  'active:before:bg-gray-900',
  'active:before:opacity-20',
  // Disabled state
  'aria-disabled:cursor-not-allowed',
  'aria-disabled:opacity-50',
  // Focus state
  'focus:outline-none', // Hide the default browser outline
  'focus-visible:ring',
  'focus-visible:ring-focus-outline-color',
  // Misc
  'text-center',
  'cursor-pointer',
  'no-underline' // For link
];

const computeClassNames = ({
  size,
  variant,
  disabled,
  block
}: {
  size?: ButtonSize;
  variant?: ButtonVariant;
  disabled?: boolean;
  block?: boolean;
}) => {
  const classNames = [...defaultClassNames];

  if (block) {
    classNames.push('block', 'w-full');
  }

  switch (variant) {
    case 'danger':

Review comment (suggestion):
Use `focus-visible` in place of `focus` to remove the focus outline when the button is clicked (as Bruce recommended in https://github.com/freeCodeCamp/freeCodeCamp/pull/50465#issuecomment-1585033555).

This change addresses the issue with focus outline on disabled button, but it actually applies to all button states and variants.

Code after:
import { ButtonProps, ButtonSize, ButtonVariant } from './types';

const defaultClassNames = [
  // Positioning
  'relative',
  'inline-block',
  'mt-[0.5px]',
  // Border
  'border-solid',
  'border-3',
  // Active state
  'active:before:w-full',
  'active:before:h-full',
  'active:before:absolute',
  'active:before:inset-0',
  'active:before:border-3',
  'active:before:border-transparent',
  'active:before:bg-gray-900',
  'active:before:opacity-20',
  // Disabled state
  'aria-disabled:cursor-not-allowed',
  'aria-disabled:opacity-50',
  // Focus state
  'focus:outline-none', // Hide the default browser outline
  'focus-visible:ring',
  'focus-visible:ring-focus-outline-color',
  // Misc
  'text-center',
  'cursor-pointer',
  'no-underline' // For link
];

const computeClassNames = ({
  size,
  variant,
  disabled,
  block
}: {
  size?: ButtonSize;
  variant?: ButtonVariant;
  disabled?: boolean;
  block?: boolean;
}) => {
  const classNames = [...defaultClassNames];

  if (block) {
    classNames.push('block', 'w-full');
  }

  switch (variant) {
    case 'danger':
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/button/button.tsx (TypeScript)

Code before:
      classNames.push('px-4 py-2.5 text-lg');
      break;
    case 'small':
      classNames.push('px-2.5 py-1 text-sm');
      break;
    // default size is 'medium'
    default:
      classNames.push('px-3 py-1.5 text-md');
  }

  return classNames.join(' ');
};

const StylessButton = React.forwardRef<React.ElementRef<'button'>, ButtonProps>(
  (
    { className, onClick, disabled, children, type = 'button', ...rest },
    ref
  ) => {
    // Manually prevent the click event if the button is disabled
    // as `aria-disabled` marks the element disabled but still registers the click event.
    // Ref: https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled
    const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
      if (!disabled && onClick) {
        onClick(event);
      }
    };

    return (
      <button
        className={className}
        onClick={handleClick}
        aria-disabled={disabled}
        ref={ref}
        type={type}
        {...rest}
      >
        {children}
      </button>
    );
  }
);

const Link = React.forwardRef<React.ElementRef<'a'>, ButtonProps>(
  ({ className, href, download, target, children, ...rest }, ref) => {
    return (
      <a
        className={className}
        download={download}
        target={target}
        ref={ref}
        href={href}

Review comment (suggestion):
This is real minor, but assigning `'button'` here implies that it is the fallback value while we actually want it to be the _default_ value, which is usually set in the props destructuring.

The result is the same but I would like to make the code communicate the intend a little more accurately.

Code after:
      classNames.push('px-4 py-2.5 text-lg');
      break;
    case 'small':
      classNames.push('px-2.5 py-1 text-sm');
      break;
    // default size is 'medium'
    default:
      classNames.push('px-3 py-1.5 text-md');
  }

  return classNames.join(' ');
};

const StylessButton = React.forwardRef<React.ElementRef<'button'>, ButtonProps>(
  (
    { className, onClick, disabled, children, type = 'button', ...rest },
    ref
  ) => {
    // Manually prevent the click event if the button is disabled
    // as `aria-disabled` marks the element disabled but still registers the click event.
    // Ref: https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled
    const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
      if (disabled) {
        return;
      }

      if (onClick) {
        onClick(event);
      }
    };

    return (
      <button
        className={className}
        onClick={handleClick}
        aria-disabled={disabled}
        ref={ref}
        type={type}
        {...rest}
      >
        {children}
      </button>
    );
  }
);

const Link = React.forwardRef<React.ElementRef<'a'>, ButtonProps>(
  ({ className, href, download, target, children, ...rest }, ref) => {
    return (
      <a
        className={className}
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/button/button.tsx (TypeScript)

Code before:
          : [
              'hover:bg-foreground-primary',
              'hover:text-background-primary',
              'dark:hover:bg-background-primary',
              'dark:hover:text-foreground-primary'
            ])
      );
  }

  switch (size) {
    case 'large':
      classNames.push('px-4 py-2.5 text-lg');
      break;
    case 'small':
      classNames.push('px-2.5 py-1 text-sm');
      break;
    // default size is 'medium'
    default:
      classNames.push('px-3 py-1.5 text-md');
  }

  return classNames.join(' ');
};

const StylessButton = React.forwardRef<React.ElementRef<'button'>, ButtonProps>(
  (
    { className, onClick, disabled, children, type = 'button', ...rest },
    ref
  ) => {
    // Manually prevent the click event if the button is disabled
    // as `aria-disabled` marks the element disabled but still registers the click event.
    // Ref: https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled
    const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
      if (!disabled && onClick) {
        onClick(event);
      }
    };

    return (
      <button
        className={className}
        onClick={handleClick}
        aria-disabled={disabled}
        ref={ref}
        type={type}
        {...rest}
      >
        {children}
      </button>
    );
  }

Review comment (refactor):
I'm just simplifying this a bit. The `aria-disabled` attribute and the `disabled` prop are equivalent in this computation, but using props is more appropriate in React world.

Code after:
          : [
              'hover:bg-foreground-primary',
              'hover:text-background-primary',
              'dark:hover:bg-background-primary',
              'dark:hover:text-foreground-primary'
            ])
      );
  }

  switch (size) {
    case 'large':
      classNames.push('px-4 py-2.5 text-lg');
      break;
    case 'small':
      classNames.push('px-2.5 py-1 text-sm');
      break;
    // default size is 'medium'
    default:
      classNames.push('px-3 py-1.5 text-md');
  }

  return classNames.join(' ');
};

const StylessButton = React.forwardRef<React.ElementRef<'button'>, ButtonProps>(
  (
    { className, onClick, disabled, children, type = 'button', ...rest },
    ref
  ) => {
    // Manually prevent the click event if the button is disabled
    // as `aria-disabled` marks the element disabled but still registers the click event.
    // Ref: https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled
    const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
      if (disabled) {
        return;
      }

      if (onClick) {
        onClick(event);
      }
    };

    return (
      <button
        className={className}
        onClick={handleClick}
        aria-disabled={disabled}
        ref={ref}
        type={type}
        {...rest}
      >
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/button/button.tsx (TypeScript)

Code before:
  ) => {
    // Manually prevent the click event if the button is disabled
    // as `aria-disabled` marks the element disabled but still registers the click event.
    // Ref: https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled
    const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
      if (!disabled && onClick) {
        onClick(event);
      }
    };

    return (
      <button
        className={className}
        onClick={handleClick}
        aria-disabled={disabled}
        ref={ref}
        type={type}
        {...rest}
      >
        {children}
      </button>
    );
  }
);

const Link = React.forwardRef<React.ElementRef<'a'>, ButtonProps>(
  ({ className, href, download, target, children, ...rest }, ref) => {
    return (
      <a
        className={className}
        download={download}
        target={target}
        ref={ref}
        href={href}
        {...rest}
      >
        {children}
      </a>
    );
  }
);

export const HeadlessButton = React.forwardRef<
  React.ElementRef<'button' | 'a'>,
  ButtonProps
>(
  (
    { onClick, className, children, disabled, href, download, target, ...rest },
    ref
  ) => {
    if (href && !disabled) {

Review comment (refactor):
We needed this fallback value because the type of `href` is currently:

https://github.com/freeCodeCamp/freeCodeCamp/blob/46375c6dd60a6b1880347b7823c89b864fdb57eb/tools/ui-components/src/button/types.ts#L16

But the `a` element only accepts `href` as either `string` or `undefined`:

<img width="880" alt="Screenshot 2023-10-26 at 15 13 25" src="https://github.com/freeCodeCamp/freeCodeCamp/assets/25715018/0e1fa600-6c69-4c5b-b27a-92905d22284e">

I resolved the issue by removing `null` from the interface, which also helps simplify the value type.

Code after:
  ) => {
    // Manually prevent the click event if the button is disabled
    // as `aria-disabled` marks the element disabled but still registers the click event.
    // Ref: https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled
    const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
      if (disabled) {
        return;
      }

      if (onClick) {
        onClick(event);
      }
    };

    return (
      <button
        className={className}
        onClick={handleClick}
        aria-disabled={disabled}
        ref={ref}
        type={type}
        {...rest}
      >
        {children}
      </button>
    );
  }
);

const Link = React.forwardRef<React.ElementRef<'a'>, ButtonProps>(
  ({ className, href, download, target, children, ...rest }, ref) => {
    return (
      <a
        className={className}
        download={download}
        target={target}
        ref={ref}
        href={href}
        {...rest}
      >
        {children}
      </a>
    );
  }
);

export const HeadlessButton = React.forwardRef<
  React.ElementRef<'button' | 'a'>,
  ButtonProps
>(
  (