ronantakizawa/codereview-bench huggingface dataset:
Repository: freeCodeCamp/freeCodeCamp
File: api/src/server.ts (TypeScript)

Code before:
// We need to use the triple-slash directive to ensure that ts-node uses the
// reset.d.ts file. It's not possible to import the file directly because it
// is not included in the build (it's a dev dependency).
// eslint-disable-next-line @typescript-eslint/triple-slash-reference
/// <reference path="./reset.d.ts" />
import { randomBytes } from 'crypto';
import { FastifyRequest } from 'fastify';
import { build } from './app';
import {
  FREECODECAMP_NODE_ENV,
  FCC_API_LOG_LEVEL,
  HOST,
  PORT
} from './utils/env';

const requestSerializer = (request: FastifyRequest) => ({
  method: request.method,
  url: request.url,
  ip: request.headers['x-forwarded-for']?.toString() || request.ip,
  hostname: request.hostname,
  remoteAddress: request.headers['x-forwarded-for']?.toString() || request.ip,
  remotePort: request.socket.remotePort
});

const envToLogger = {
  development: {
    transport: {
      target: 'pino-pretty',
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
  },
  production: {
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
  }
};

Review comment (suggestion):
```suggestion
  remoteAddress: request.headers['x-forwarded-for'] || request.ip,
```
Same again.

Code after:
// We need to use the triple-slash directive to ensure that ts-node uses the
// reset.d.ts file. It's not possible to import the file directly because it
// is not included in the build (it's a dev dependency).
// eslint-disable-next-line @typescript-eslint/triple-slash-reference
/// <reference path="./reset.d.ts" />
import { randomBytes } from 'crypto';
import { FastifyRequest } from 'fastify';
import { build } from './app';
import {
  FREECODECAMP_NODE_ENV,
  FCC_API_LOG_LEVEL,
  HOST,
  PORT
} from './utils/env';

const requestSerializer = (request: FastifyRequest) => ({
  method: request.method,
  url: request.url,
  ip: request.headers['x-forwarded-for'] || request.ip,
  hostname: request.hostname,
  remoteAddress: Array.isArray(request.headers['x-forwarded-for'])
    ? request.headers['x-forwarded-for'][0]
    : request.headers['x-forwarded-for'] || request.ip,
  remotePort: request.socket.remotePort
});

const envToLogger = {
  development: {
    transport: {
      target: 'pino-pretty',
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
  },
  production: {
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/profile/components/internet.tsx (TypeScript)

Code before:

function Info({ message }: { message: string }) {
  return message ? <HelpBlock>{message}</HelpBlock> : null;
}

const InternetSettings = ({
  githubProfile = '',
  linkedin = '',
  twitter = '',
  website = '',
  t,
  updateSocials,
  setIsEditing
}: InternetProps) => {
  const [formValues, setFormValues] = useState<Socials>({
    githubProfile,
    linkedin,
    twitter,
    website
  });
  const [originalValues, setOriginalValues] = useState<Socials>({
    githubProfile,
    linkedin,
    twitter,
    website
  });

  useEffect(() => {
    if (
      githubProfile !== originalValues.githubProfile ||
      linkedin !== originalValues.linkedin ||
      twitter !== originalValues.twitter ||
      website !== originalValues.website
    ) {
      setOriginalValues({ githubProfile, linkedin, twitter, website });
    }
  }, [githubProfile, linkedin, twitter, website, originalValues]);

  const getValidationStateFor = (maybeURl = ''): URLValidation => {
    if (!maybeURl || !maybeUrlRE.test(maybeURl)) {
      return {
        state: null,
        message: ''
      };
    }
    if (isURL(maybeURl)) {
      return {
        state: 'success',
        message: ''
      };
    }

Review comment (suggestion):
```suggestion
```
This does faithfully preserve the original implementation, but it was flawed. We don't need to create more state to keep track of the props. We have the props!

Code after:

function Info({ message }: { message: string }) {
  return message ? <HelpBlock>{message}</HelpBlock> : null;
}

const InternetSettings = ({
  githubProfile = '',
  linkedin = '',
  twitter = '',
  website = '',
  t,
  updateSocials,
  setIsEditing
}: InternetProps) => {
  const [formValues, setFormValues] = useState<Socials>({
    githubProfile,
    linkedin,
    twitter,
    website
  });

  const getValidationStateFor = (maybeURl = ''): URLValidation => {
    if (!maybeURl || !maybeUrlRE.test(maybeURl)) {
      return {
        state: null,
        message: ''
      };
    }
    if (isURL(maybeURl)) {
      return {
        state: 'success',
        message: ''
      };
    }
    return {
      state: 'error',
      message: t('validation.invalid-url')
    };
  };

  const createHandleChange =
    (key: keyof Socials) => (e: React.FormEvent<HTMLInputElement>) => {
      const value = (e.target as HTMLInputElement).value.slice(0);
      setFormValues({
        ...formValues,
        [key]: value
      });
    };

  const isFormPristine = () => {
    const originalValues = { githubProfile, linkedin, twitter, website };
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/profile/components/internet.tsx (TypeScript)

Code before:
    website
  });

  useEffect(() => {
    if (
      githubProfile !== originalValues.githubProfile ||
      linkedin !== originalValues.linkedin ||
      twitter !== originalValues.twitter ||
      website !== originalValues.website
    ) {
      setOriginalValues({ githubProfile, linkedin, twitter, website });
    }
  }, [githubProfile, linkedin, twitter, website, originalValues]);

  const getValidationStateFor = (maybeURl = ''): URLValidation => {
    if (!maybeURl || !maybeUrlRE.test(maybeURl)) {
      return {
        state: null,
        message: ''
      };
    }
    if (isURL(maybeURl)) {
      return {
        state: 'success',
        message: ''
      };
    }
    return {
      state: 'error',
      message: t('validation.invalid-url')
    };
  };

  const createHandleChange =
    (key: keyof Socials) => (e: React.FormEvent<HTMLInputElement>) => {
      const value = (e.target as HTMLInputElement).value.slice(0);
      setFormValues({
        ...formValues,
        [key]: value
      });
    };

  const isFormPristine = () => {
    return (Object.keys(originalValues) as Array<keyof Socials>).every(
      key => originalValues[key] === formValues[key]
    );
  };

  const isFormValid = (): boolean =>
    Object.values(formValues).every(
      (value: string) => value === '' || isURL(value)

Review comment (suggestion):
```suggestion
  const isFormPristine = () => {
    const originalValues = { githubProfile, linkedin, twitter, website };
```
This is all we need.

Code after:
        state: null,
        message: ''
      };
    }
    if (isURL(maybeURl)) {
      return {
        state: 'success',
        message: ''
      };
    }
    return {
      state: 'error',
      message: t('validation.invalid-url')
    };
  };

  const createHandleChange =
    (key: keyof Socials) => (e: React.FormEvent<HTMLInputElement>) => {
      const value = (e.target as HTMLInputElement).value.slice(0);
      setFormValues({
        ...formValues,
        [key]: value
      });
    };

  const isFormPristine = () => {
    const originalValues = { githubProfile, linkedin, twitter, website };

    return (Object.keys(originalValues) as Array<keyof Socials>).every(
      key => originalValues[key] === formValues[key]
    );
  };

  const isFormValid = (): boolean =>
    Object.values(formValues).every(
      (value: string) => value === '' || isURL(value)
    );

  const handleSubmit = (e: React.FormEvent) => {
    e.preventDefault();
    if (!isFormPristine() && isFormValid()) {
      // Only submit the form if is has changed, and if it is valid
      updateSocials({ ...formValues });
    }
    setIsEditing(false);
  };

  const {
    state: githubProfileValidation,
    message: githubProfileValidationMessage
  } = getValidationStateFor(formValues.githubProfile);
Repository: freeCodeCamp/freeCodeCamp
File: client/src/components/profile/components/portfolio.tsx (TypeScript)

Code before:
  t: TFunction;
  updatePortfolio: (obj: { portfolio: PortfolioProjectData[] }) => void;
  username?: string;
  setIsEditing: (isEditing: boolean) => void;
};

interface ProfileValidation {
  state: FormGroupProps['validationState'];
  message: string;
}

function createEmptyPortfolioItem(): PortfolioProjectData {
  return {
    id: nanoid(),
    title: '',
    description: '',
    url: '',
    image: ''
  };
}

function createFindById(id: string) {
  return (p: PortfolioProjectData) => p.id === id;
}

const PortfolioSettings: React.FC<PortfolioProps> = props => {
  const {
    t,
    portfolio: initialPortfolio = [],
    setIsEditing,
    updatePortfolio
  } = props;
  const [portfolio, setPortfolio] = useState([...initialPortfolio]);
  const [unsavedItemId, setUnsavedItemId] = useState<string | null>(null);
  const [isImageValid, setIsImageValid] = useState<ProfileValidation>({
    state: 'success',
    message: ''
  });
  const validationImageRef = useRef<HTMLImageElement>(new Image());

  const validateImageLoad = async (
    image: string
  ): Promise<ProfileValidation> => {
    return new Promise(resolve => {
      validationImageRef.current.src = encodeURI(image);
      validationImageRef.current.onload = () => {
        resolve({ state: 'success', message: '' });
      };
      validationImageRef.current.onerror = () => {
        resolve({ state: 'error', message: t('validation.url-not-image') });
      };

Review comment (bug):
```suggestion
const PortfolioSettings = (props: PortfolioProps) => {
```
Nit: this is a little cleaner. Also, React.FC does some stuff implicitly https://stackoverflow.com/a/59991281 and is probably best avoided.


Code after:
  t: TFunction;
  updatePortfolio: (obj: { portfolio: PortfolioProjectData[] }) => void;
  username?: string;
  setIsEditing: (isEditing: boolean) => void;
};

interface ProfileValidation {
  state: FormGroupProps['validationState'];
  message: string;
}

function createEmptyPortfolioItem(): PortfolioProjectData {
  return {
    id: nanoid(),
    title: '',
    description: '',
    url: '',
    image: ''
  };
}

function createFindById(id: string) {
  return (p: PortfolioProjectData) => p.id === id;
}

const PortfolioSettings = (props: PortfolioProps) => {
  const {
    t,
    portfolio: initialPortfolio = [],
    setIsEditing,
    updatePortfolio
  } = props;
  const [portfolio, setPortfolio] = useState(initialPortfolio);
  const [unsavedItemId, setUnsavedItemId] = useState<string | null>(null);
  const [imageValidation, setImageValid] = useState<ProfileValidation>({
    state: 'success',
    message: ''
  });

  const checkIfValidImage = (url: string): Promise<ProfileValidation> => {
    const img = new Image();

    return new Promise(resolve => {
      img.onerror = () =>
        resolve({ state: 'error', message: t('validation.url-not-image') });
      img.onload = () => resolve({ state: 'success', message: '' });
      img.src = url;
    });
  };

  const createOnChangeHandler =
Repository: freeCodeCamp/freeCodeCamp
File: api/src/server.ts (TypeScript)

Code before:
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
  },
  production: {
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
  }
};

const start = async () => {
  const fastify = await build({
    logger: envToLogger[FREECODECAMP_NODE_ENV] ?? true,
    genReqId: () => randomBytes(8).toString('hex'),
    disableRequestLogging: true
  });
  fastify.addHook('onRequest', (req, _reply, done) => {
    req.log.debug({ req }, 'incoming request');
    done();
  });
  try {
    const port = Number(PORT);
    fastify.log.info(`Starting server on port ${port}`);
    await fastify.listen({ port, host: HOST });
  } catch (err) {
    fastify.log.error(err);
    process.exit(1);
  }
};

void start();


Review comment (bug):
This reduces the granular control on 'specific' requests that we may want to log - example: I always want the ping endpoint, but never the others unless the 'debug' filter is on.

I see your point about single source of truth -- but this makes it "too noisy" even when we turn on "debug" filters.

I would like more control.

Code after:
  const remotePort = req.socket.remotePort || 'REMOTE_PORT not found';

  return {
    REQ_ID: req.id,
    METHOD: method,
    URL: url,
    IP: ip,
    HOSTNAME: hostname,
    REMOTE_PORT: remotePort,
    QUERY: query,
    HEADERS: headers
  };
};

const envToLogger = {
  development: {
    transport: {
      target: 'pino-pretty',
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: (req: FastifyRequest) => {
        return {
          method: req.method,
          url: req.url
        };
      }
    }
    // No need to redact in development
  },
  production: {
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    },
    redact: ['req.HEADERS.cookie']
  }
};

const start = async () => {
  const fastify = await build({
    logger: envToLogger[FREECODECAMP_NODE_ENV] ?? true,
    genReqId: () => randomBytes(8).toString('hex'),
    disableRequestLogging: true
  });
  try {
Repository: freeCodeCamp/freeCodeCamp
File: api/src/routes/public/status.ts (TypeScript)

Code before:
import { type FastifyPluginCallbackTypebox } from '@fastify/type-provider-typebox';

/**
 * Plugin for the health check endpoint.
 *
 * @param fastify The Fastify instance.
 * @param _options Options passed to the plugin via `fastify.register(plugin,
 * options)`.
 * @param done The callback to signal that the plugin is ready.
 */
export const statusRoute: FastifyPluginCallbackTypebox = (
  fastify,
  _options,
  done
) => {
  fastify.get('/status/ping', async (req, _reply) => {
    req.log.debug('returning a ping');
    return { msg: 'pong' };
  });

  done();
};


Review comment (bug):
I do want to debug some of these information from time to time. So a child logger (which adds the request specific annotations automatically) will be needed here in addition to logging the data.

You are welcome to simplify it.

Code after:
import { type FastifyPluginCallbackTypebox } from '@fastify/type-provider-typebox';

/**
 * Plugin for the health check endpoint.
 *
 * @param fastify The Fastify instance.
 * @param _options Options passed to the plugin via `fastify.register(plugin,
 * options)`.
 * @param done The callback to signal that the plugin is ready.
 */
export const statusRoute: FastifyPluginCallbackTypebox = (
  fastify,
  _options,
  done
) => {
  fastify.get('/status/ping', async (req, _reply) => {
    fastify.log.child({ req }).debug('returning a ping');
    return { msg: 'pong' };
  });

  done();
};
Repository: freeCodeCamp/freeCodeCamp
File: api/src/server.ts (TypeScript)

Code before:

import { FastifyRequest } from 'fastify';
import { isEmpty } from 'lodash';

import { build } from './app';
import {
  FREECODECAMP_NODE_ENV,
  FCC_API_LOG_LEVEL,
  HOST,
  PORT
} from './utils/env';

const requestSerializer = (req: FastifyRequest) => {
  const method = req.method || 'METHOD not found';
  const url = req.url || 'URL not found';
  const headers = req.headers || 'HEADERS not found';
  const xForwardedFor = Array.isArray(req.headers['x-forwarded-for'])
    ? req.headers['x-forwarded-for'][0]
    : req.headers['x-forwarded-for'];
  const ip =
    xForwardedFor || req.headers['x-real-ip'] || req.ip || 'IP not found';
  const query = isEmpty(req.query) ? 'QUERY not found' : req.query;
  const hostname = req.hostname || 'HOSTNAME not found';
  const remotePort = req.socket.remotePort || 'REMOTE_PORT not found';

  return {
    METHOD: method,
    URL: url,
    IP: ip,
    HOSTNAME: hostname,
    REMOTE_PORT: remotePort,
    QUERY: query,
    HEADERS: headers
  };
};

const envToLogger = {
  development: {
    transport: {
      target: 'pino-pretty',
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
    // No need to redact in development

Review comment (suggestion):
```suggestion
  return {
    REQ_ID: req.id,
```

Include the request ID

Code after:

import { FastifyRequest } from 'fastify';
import { isEmpty } from 'lodash';

import { build } from './app';
import {
  FREECODECAMP_NODE_ENV,
  FCC_API_LOG_LEVEL,
  HOST,
  PORT
} from './utils/env';

const requestSerializer = (req: FastifyRequest) => {
  const method = req.method || 'METHOD not found';
  const url = req.url || 'URL not found';
  const headers = req.headers || 'HEADERS not found';
  const xForwardedFor = Array.isArray(req.headers['x-forwarded-for'])
    ? req.headers['x-forwarded-for'][0]
    : req.headers['x-forwarded-for'];
  const ip =
    xForwardedFor || req.headers['x-real-ip'] || req.ip || 'IP not found';
  const query = isEmpty(req.query) ? 'QUERY not found' : req.query;
  const hostname = req.hostname || 'HOSTNAME not found';
  const remotePort = req.socket.remotePort || 'REMOTE_PORT not found';

  return {
    REQ_ID: req.id,
    METHOD: method,
    URL: url,
    IP: ip,
    HOSTNAME: hostname,
    REMOTE_PORT: remotePort,
    QUERY: query,
    HEADERS: headers
  };
};

const envToLogger = {
  development: {
    transport: {
      target: 'pino-pretty',
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: (req: FastifyRequest) => {
        return {
Repository: freeCodeCamp/freeCodeCamp
File: api/src/server.ts (TypeScript)

Code before:
  const remotePort = req.socket.remotePort || 'REMOTE_PORT not found';

  return {
    METHOD: method,
    URL: url,
    IP: ip,
    HOSTNAME: hostname,
    REMOTE_PORT: remotePort,
    QUERY: query,
    HEADERS: headers
  };
};

const envToLogger = {
  development: {
    transport: {
      target: 'pino-pretty',
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    }
    // No need to redact in development
  },
  production: {
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    },
    redact: ['req.HEADERS.cookie']
  }
};

const start = async () => {
  const fastify = await build({
    logger: envToLogger[FREECODECAMP_NODE_ENV] ?? true,
    genReqId: () => randomBytes(8).toString('hex'),
    disableRequestLogging: true
  });
  try {
    const port = Number(PORT);
    fastify.log.info(`Starting server on port ${port}`);
    await fastify.listen({ port, host: HOST });
  } catch (err) {
    fastify.log.error(err);
    process.exit(1);

Review comment (suggestion):
```suggestion
      req: (req: FastifyRequest) => {
        return {
          method: req.method,
          url: req.url
        };
      }
```

The `requestSerializer` is way too noisy for local development. Parity is not a concern here.

Code after:
  const remotePort = req.socket.remotePort || 'REMOTE_PORT not found';

  return {
    REQ_ID: req.id,
    METHOD: method,
    URL: url,
    IP: ip,
    HOSTNAME: hostname,
    REMOTE_PORT: remotePort,
    QUERY: query,
    HEADERS: headers
  };
};

const envToLogger = {
  development: {
    transport: {
      target: 'pino-pretty',
      options: {
        singleLine: true,
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname'
      }
    },
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: (req: FastifyRequest) => {
        return {
          method: req.method,
          url: req.url
        };
      }
    }
    // No need to redact in development
  },
  production: {
    level: FCC_API_LOG_LEVEL || 'info',
    serializers: {
      req: requestSerializer
    },
    redact: ['req.HEADERS.cookie']
  }
};

const start = async () => {
  const fastify = await build({
    logger: envToLogger[FREECODECAMP_NODE_ENV] ?? true,
    genReqId: () => randomBytes(8).toString('hex'),
    disableRequestLogging: true
  });
  try {
Repository: freeCodeCamp/freeCodeCamp
File: api/src/routes/public/status.ts (TypeScript)

Code before:
import { type FastifyPluginCallbackTypebox } from '@fastify/type-provider-typebox';

/**
 * Plugin for the health check endpoint.
 *
 * @param fastify The Fastify instance.
 * @param _options Options passed to the plugin via `fastify.register(plugin,
 * options)`.
 * @param done The callback to signal that the plugin is ready.
 */
export const statusRoute: FastifyPluginCallbackTypebox = (
  fastify,
  _options,
  done
) => {
  fastify.get('/status/ping', async (req, _reply) => {
    req.log.child({ req }).debug('returning a ping');
    return { msg: 'pong' };
  });

  done();
};


Review comment (bug):
```suggestion
    fastify.log.child({ req }).debug('returning a ping');
```

This is not better, but is a clearer mental model. Use `fastify.log` and `fastify.log.child` everywhere.

Code after:
import { type FastifyPluginCallbackTypebox } from '@fastify/type-provider-typebox';

/**
 * Plugin for the health check endpoint.
 *
 * @param fastify The Fastify instance.
 * @param _options Options passed to the plugin via `fastify.register(plugin,
 * options)`.
 * @param done The callback to signal that the plugin is ready.
 */
export const statusRoute: FastifyPluginCallbackTypebox = (
  fastify,
  _options,
  done
) => {
  fastify.get('/status/ping', async (req, _reply) => {
    fastify.log.child({ req }).debug('returning a ping');
    return { msg: 'pong' };
  });

  done();
};