ronantakizawa/codereview-bench huggingface dataset:
Repository: hasura/graphql-engine
File: console/src/helpers/versionUtils.ts (TypeScript)

Code before:
import globals from '../Globals';

const semver = require('semver');

export const READ_ONLY_RUN_SQL_QUERIES = 'readOnlyRunSqlQueries';

// list of feature launch versions
const featureLaunchVersions = {
  // feature: 'v1.0.1'
  [READ_ONLY_RUN_SQL_QUERIES]: 'v1.1.0',
};

type IFeature = keyof typeof featureLaunchVersions;

type IFeaturesCompatibility = {
  [key in IFeature]?: boolean;
};

export const checkValidServerVersion = (version: string) => {
  return semver.valid(version) !== null;
};

export const getFeaturesCompatibility = (serverVersion: string) => {
  const featuresCompatibility: IFeaturesCompatibility = {};

  const isValidServerVersion = checkValidServerVersion(serverVersion);

  Object.keys(featureLaunchVersions).forEach(_feature => {
    const feature = _feature as IFeature;
    featuresCompatibility[feature] = isValidServerVersion
      ? semver.satisfies(serverVersion, `>= ${featureLaunchVersions[feature]}`)
      : true;
  });

  return featuresCompatibility;
};

export const versionGT = (version1: string, version2: string) => {
  try {
    return semver.gt(version1, version2);

Review comment (suggestion):
```suggestion
type FeaturesCompatibility = {
```

Code after:
import globals from '../Globals';

const semver = require('semver');

export const READ_ONLY_RUN_SQL_QUERIES = 'readOnlyRunSqlQueries';

// list of feature launch versions
const featureLaunchVersions = {
  // feature: 'v1.0.1'
  [READ_ONLY_RUN_SQL_QUERIES]: 'v1.1.0',
};

type Feature = keyof typeof featureLaunchVersions;

type FeaturesCompatibility = {
  [key in Feature]?: boolean;
};

export const checkValidServerVersion = (version: string) => {
  return semver.valid(version) !== null;
};

export const getFeaturesCompatibility = (serverVersion: string) => {
  const featuresCompatibility: FeaturesCompatibility = {};

  const isValidServerVersion = checkValidServerVersion(serverVersion);

  Object.keys(featureLaunchVersions).forEach(_feature => {
    const feature = _feature as Feature;
    featuresCompatibility[feature] = isValidServerVersion
      ? semver.satisfies(serverVersion, `>= ${featureLaunchVersions[feature]}`)
      : true;
  });

  return featuresCompatibility;
};

export const versionGT = (version1: string, version2: string) => {
  try {
    return semver.gt(version1, version2);
Repository: hasura/graphql-engine
File: console/src/helpers/Html.tsx (TypeScript)

Code before:
import { env } from './localDev';
/**
 * Wrapper component containing HTML metadata and boilerplate tags.
 * Used in server-side code only to wrap the string output of the
 * rendered route component.
 *
 * The only thing this component doesn't (and can't) include is the
 * HTML doctype declaration, which is added to the rendered output
 * by the server.js file.
 */

type IAssets = {
  [key in 'styles' | 'javascript']: {
    [key: string]: string;
  };
};

type HtmlProps = {
  assets: IAssets;
  component: React.ReactNode;
  baseDomain: string;
};

export default function Html(props: HtmlProps) {
  const { assets } = props;
  const head = Helmet.rewind();
  return (
    <html lang="en-US">
      <head>
        <link rel="icon" type="image/png" href="/rstatic/favicon_green.png" />
        {Object.keys(assets.styles).map((style, key) => (
          <link
            href={assets.styles[style]}
            // eslint-disable-next-line react/no-array-index-key
            key={key}
            media="screen, projection"
            rel="stylesheet"
            type="text/css"
          />
        ))}

        <script
          dangerouslySetInnerHTML={{
            __html: env,
          }}
        />
      </head>
      <body>
        <style
          dangerouslySetInnerHTML={{
            __html: `

Review comment (suggestion):
This constant seems not to be used anywhere.
```suggestion
```

Code after:
/**
 * Wrapper component containing HTML metadata and boilerplate tags.
 * Used in server-side code only to wrap the string output of the
 * rendered route component.
 *
 * The only thing this component doesn't (and can't) include is the
 * HTML doctype declaration, which is added to the rendered output
 * by the server.js file.
 */

type Assets = Record<'styles' | 'javascript', Record<string, string>>;

type HtmlProps = {
  assets: Assets;
  component: React.ReactNode;
  baseDomain: string;
};

const Html: React.FC<HtmlProps> = props => {
  const { assets } = props;
  return (
    <html lang="en-US">
      <head>
        <link rel="icon" type="image/png" href="/rstatic/favicon_green.png" />
        {Object.keys(assets.styles).map((style, key) => (
          <link
            href={assets.styles[style]}
            key={key}
            media="screen, projection"
            rel="stylesheet"
            type="text/css"
          />
        ))}

        <script
          dangerouslySetInnerHTML={{
            __html: env,
          }}
        />
      </head>
      <body>
        <style
          dangerouslySetInnerHTML={{
            __html: `
          .content {
            display: 'none';
            opacity: 0;
            transition: opacity .20s linear;
          }
          .content.show {
            display: 'block';
Repository: hasura/graphql-engine
File: console/src/helpers/Html.tsx (TypeScript)

Code before:
import React from 'react';
import Helmet from 'react-helmet';
import { env } from './localDev';
/**
 * Wrapper component containing HTML metadata and boilerplate tags.
 * Used in server-side code only to wrap the string output of the
 * rendered route component.
 *
 * The only thing this component doesn't (and can't) include is the
 * HTML doctype declaration, which is added to the rendered output
 * by the server.js file.
 */

type IAssets = {
  [key in 'styles' | 'javascript']: {
    [key: string]: string;
  };
};

type HtmlProps = {
  assets: IAssets;
  component: React.ReactNode;
  baseDomain: string;
};

export default function Html(props: HtmlProps) {
  const { assets } = props;
  const head = Helmet.rewind();
  return (
    <html lang="en-US">
      <head>
        <link rel="icon" type="image/png" href="/rstatic/favicon_green.png" />
        {Object.keys(assets.styles).map((style, key) => (
          <link
            href={assets.styles[style]}
            // eslint-disable-next-line react/no-array-index-key
            key={key}
            media="screen, projection"
            rel="stylesheet"
            type="text/css"
          />
        ))}

        <script
          dangerouslySetInnerHTML={{
            __html: env,
          }}
        />
      </head>
      <body>
        <style

Review comment (suggestion):
We usually try use `const` instead `function` (in newer code), so to keep it consistent, could you change it to:
```ts
const Html = (...) => { ... }

export default Html;
```

And also as it's function component we can do:
`export const Html: React.FC<HtmlProps> = props => { ... }`

Code after:
import React from 'react';
import { env } from './localDev';
/**
 * Wrapper component containing HTML metadata and boilerplate tags.
 * Used in server-side code only to wrap the string output of the
 * rendered route component.
 *
 * The only thing this component doesn't (and can't) include is the
 * HTML doctype declaration, which is added to the rendered output
 * by the server.js file.
 */

type Assets = Record<'styles' | 'javascript', Record<string, string>>;

type HtmlProps = {
  assets: Assets;
  component: React.ReactNode;
  baseDomain: string;
};

const Html: React.FC<HtmlProps> = props => {
  const { assets } = props;
  return (
    <html lang="en-US">
      <head>
        <link rel="icon" type="image/png" href="/rstatic/favicon_green.png" />
        {Object.keys(assets.styles).map((style, key) => (
          <link
            href={assets.styles[style]}
            key={key}
            media="screen, projection"
            rel="stylesheet"
            type="text/css"
          />
        ))}

        <script
          dangerouslySetInnerHTML={{
            __html: env,
          }}
        />
      </head>
      <body>
        <style
          dangerouslySetInnerHTML={{
            __html: `
          .content {
            display: 'none';
            opacity: 0;
            transition: opacity .20s linear;
          }
Repository: hasura/graphql-engine
File: console/src/helpers/makeRouteHooksSafe.ts (TypeScript)

Code before:
import { createRoutes } from 'react-router/lib/RouteUtils';
import { PlainRoute, EnterHook } from 'react-router';

// Wrap the hooks so they don't fire if they're called before
// the store is initialised. This only happens when doing the first
// client render of a route that has an onEnter hook

function makeHooksSafe(routes: PlainRoute[] | PlainRoute, store: any): any {
  if (Array.isArray(routes)) {
    return routes.map(route => makeHooksSafe(route, store));
  }

  const onEnter = routes.onEnter;

  if (onEnter) {
    // eslint-disable-next-line no-param-reassign
    routes.onEnter = function safeOnEnter(...args: Parameters<EnterHook>) {
      try {
        store.getState();
      } catch (err) {
        if (onEnter.length === 3 && args[2]) {
          args[2]();
        }

        // There's no store yet so ignore the hook
        return;
      }

      // eslint-disable-next-line prefer-spread
      onEnter.apply(null, args);
    };
  }


Review comment (suggestion):
```suggestion
function makeHooksSafe(routes: PlainRoute[] | PlainRoute, store: unknown): unknown {
```

Code after:
import { createRoutes } from 'react-router/lib/RouteUtils';
import { PlainRoute, EnterHook } from 'react-router';

// Wrap the hooks so they don't fire if they're called before
// the store is initialised. This only happens when doing the first
// client render of a route that has an onEnter hook

function makeHooksSafe(routes: PlainRoute[] | PlainRoute, store: any): unknown {
  if (Array.isArray(routes)) {
    return routes.map(route => makeHooksSafe(route, store));
  }

  const onEnter = routes.onEnter;

  if (onEnter) {
    // eslint-disable-next-line no-param-reassign
    routes.onEnter = function safeOnEnter(...args: Parameters<EnterHook>) {
      try {
        store.getState();
      } catch (err) {
        if (onEnter.length === 3 && args[2]) {
          args[2]();
        }

        // There's no store yet so ignore the hook
        return;
      }

      // eslint-disable-next-line prefer-spread
      onEnter.apply(null, args);
    };
  }
Repository: hasura/graphql-engine
File: console/src/helpers/makeRouteHooksSafe.ts (TypeScript)

Code before:
import { createRoutes } from 'react-router/lib/RouteUtils';
import { PlainRoute, EnterHook } from 'react-router';

// Wrap the hooks so they don't fire if they're called before
// the store is initialised. This only happens when doing the first
// client render of a route that has an onEnter hook

function makeHooksSafe(routes: PlainRoute[] | PlainRoute, store: unknown): unknown {
  if (Array.isArray(routes)) {
    return routes.map(route => makeHooksSafe(route, store));
  }

  const onEnter = routes.onEnter;

  if (onEnter) {
    // eslint-disable-next-line no-param-reassign
    routes.onEnter = function safeOnEnter(...args: Parameters<EnterHook>) {
      try {
        store.getState();
      } catch (err) {
        if (onEnter.length === 3 && args[2]) {
          args[2]();
        }

        // There's no store yet so ignore the hook
        return;
      }

      // eslint-disable-next-line prefer-spread
      onEnter.apply(null, args);
    };
  }


Review comment (suggestion):
```suggestion
function makeHooksSafe(routes: PlainRoute[] | PlainRoute, store: any): unknown {
```

Code after:
import { createRoutes } from 'react-router/lib/RouteUtils';
import { PlainRoute, EnterHook } from 'react-router';

// Wrap the hooks so they don't fire if they're called before
// the store is initialised. This only happens when doing the first
// client render of a route that has an onEnter hook

function makeHooksSafe(routes: PlainRoute[] | PlainRoute, store: any): unknown {
  if (Array.isArray(routes)) {
    return routes.map(route => makeHooksSafe(route, store));
  }

  const onEnter = routes.onEnter;

  if (onEnter) {
    // eslint-disable-next-line no-param-reassign
    routes.onEnter = function safeOnEnter(...args: Parameters<EnterHook>) {
      try {
        store.getState();
      } catch (err) {
        if (onEnter.length === 3 && args[2]) {
          args[2]();
        }

        // There's no store yet so ignore the hook
        return;
      }

      // eslint-disable-next-line prefer-spread
      onEnter.apply(null, args);
    };
  }
Repository: hasura/graphql-engine
File: console/src/components/Services/Settings/utils.ts (TypeScript)

Code before:
];

export type TableDef = {
  name: string;
};

export type MetadataObject = {
  name: string;
  function_name: string;
  type: 'table' | 'array_relation' | 'object_relation' | 'function' | 'event_trigger' | 'remote_schema' | 'action';
  definition: {
    name: string;
    role: string;
    table: {
      name: string;
    }
    configuration: {
      name: string;
    }
  };
  table_name: string;
  relationships: Array<{ rel_name: string }>;
  permissions: Array<{ role_name: string }>
};

export const getTableNameFromDef = (def: TableDef|string): string => {
    if (typeof def === 'string') {
      return def;
    }
    return def.name;
};

const filterInconsistentMetadataObject = (
  objects: MetadataObject[],
  inconsistentObject: MetadataObject,
  type: string
): MetadataObject[] => {
  switch (type) {
    case 'tables':
      const schemas = objects;

      if (inconsistentObject.type === 'table') {
        const tableName = getTableNameFromDef(inconsistentObject.definition);
        return schemas.filter(s => s.table_name !== tableName);
      }

      if (
        inconsistentObject.type === 'array_relation' ||
        inconsistentObject.type === 'object_relation'
      ) {
        const { table } = inconsistentObject.definition;

Review comment (suggestion):
As TypeScript can infer this.
```suggestion
export const getTableNameFromDef = (def: TableDef | string) => {
```

Code after:
];

export type MetadataObject = {
  name: string;
  function_name: string;
  type:
    | 'table'
    | 'array_relation'
    | 'object_relation'
    | 'function'
    | 'event_trigger'
    | 'remote_schema'
    | 'action';
  definition: {
    name: string;
    role: string;
    table: {
      name: string;
    };
    configuration: {
      name: string;
    };
  };
  table_name: string;
  relationships: Array<{ rel_name: string }>;
  permissions: Array<{ role_name: string }>;
};

export const getTableNameFromDef = (
  def:
    | {
        name: string;
      }
    | string
) => {
  if (typeof def === 'string') {
    return def;
  }
  return def.name;
};

const filterInconsistentMetadataObject = (
  objects: MetadataObject[],
  inconsistentObject: MetadataObject,
  type: string
): MetadataObject[] => {
  switch (type) {
    case 'tables':
      const schemas = objects;

      if (inconsistentObject.type === 'table') {
Repository: hasura/graphql-engine
File: console/src/components/Services/Settings/utils.ts (TypeScript)

Code before:
export const permissionTypes = [
  'select_permission',
  'update_permission',
  'insert_permission',
  'delete_permission',
];

export type TableDef = {
  name: string;
};

export type MetadataObject = {
  name: string;
  function_name: string;
  type: 'table' | 'array_relation' | 'object_relation' | 'function' | 'event_trigger' | 'remote_schema' | 'action';
  definition: {
    name: string;
    role: string;
    table: {
      name: string;
    }
    configuration: {
      name: string;
    }
  };
  table_name: string;
  relationships: Array<{ rel_name: string }>;
  permissions: Array<{ role_name: string }>
};

export const getTableNameFromDef = (def: TableDef|string): string => {
    if (typeof def === 'string') {
      return def;

Review comment (suggestion):
It looks like something used only in this particular use case (for `getTableNameFromDef`). And as in some other places, we have table definition represented as `{ table_name: string }` or `{ tableName: string }` or `{ name: string, other_attribute: Type }`etc, I wouldn't export this type. (At least not until we have all of these types unified).

What's more, I would just inline this type in the function definition:
```ts
export const getTableNameFromDef = (def: { name: string } | string) => {
```

Code after:
export const permissionTypes = [
  'select_permission',
  'update_permission',
  'insert_permission',
  'delete_permission',
];

export type MetadataObject = {
  name: string;
  function_name: string;
  type:
    | 'table'
    | 'array_relation'
    | 'object_relation'
    | 'function'
    | 'event_trigger'
    | 'remote_schema'
    | 'action';
  definition: {
    name: string;
    role: string;
    table: {
      name: string;
    };
    configuration: {
      name: string;
    };
  };
  table_name: string;
  relationships: Array<{ rel_name: string }>;
  permissions: Array<{ role_name: string }>;
};
Repository: hasura/graphql-engine
File: console/src/components/Services/Settings/utils.ts (TypeScript)

Code before:
  table_name: string;
  relationships: Array<{ rel_name: string }>;
  permissions: Array<{ role_name: string }>
};

export const getTableNameFromDef = (def: TableDef|string): string => {
    if (typeof def === 'string') {
      return def;
    }
    return def.name;
};

const filterInconsistentMetadataObject = (
  objects: MetadataObject[],
  inconsistentObject: MetadataObject,
  type: string
): MetadataObject[] => {
  switch (type) {
    case 'tables':
      const schemas = objects;

      if (inconsistentObject.type === 'table') {
        const tableName = getTableNameFromDef(inconsistentObject.definition);
        return schemas.filter(s => s.table_name !== tableName);
      }

      if (
        inconsistentObject.type === 'array_relation' ||
        inconsistentObject.type === 'object_relation'
      ) {
        const { table } = inconsistentObject.definition;
        return schemas.map(schema => {
          if (schema.table_name === getTableNameFromDef(table)) {
            return {
              ...schema,
              relationships: schema.relationships.filter(r => {
                return r.rel_name !== inconsistentObject.definition.name;
              }),
            };
          }
          return schema;
        });
      }

      if (permissionTypes.includes(inconsistentObject.type)) {
        const { table } = inconsistentObject.definition;
        return schemas.map(schema => {
          if (schema.table_name === getTableNameFromDef(table)) {
            return {
              ...schema,
              permissions: schema.permissions.filter(p => {

Review comment (suggestion):
TypeScript can infer this properly.
```suggestion
) => {
```

Code after:
      name: string;
    };
  };
  table_name: string;
  relationships: Array<{ rel_name: string }>;
  permissions: Array<{ role_name: string }>;
};

export const getTableNameFromDef = (
  def:
    | {
        name: string;
      }
    | string
) => {
  if (typeof def === 'string') {
    return def;
  }
  return def.name;
};

const filterInconsistentMetadataObject = (
  objects: MetadataObject[],
  inconsistentObject: MetadataObject,
  type: string
): MetadataObject[] => {
  switch (type) {
    case 'tables':
      const schemas = objects;

      if (inconsistentObject.type === 'table') {
        const tableName = getTableNameFromDef(inconsistentObject.definition);
        return schemas.filter(s => s.table_name !== tableName);
      }

      if (
        inconsistentObject.type === 'array_relation' ||
        inconsistentObject.type === 'object_relation'
      ) {
        const { table } = inconsistentObject.definition;
        return schemas.map(schema => {
          if (schema.table_name === getTableNameFromDef(table)) {
            return {
              ...schema,
              relationships: schema.relationships.filter(r => {
                return r.rel_name !== inconsistentObject.definition.name;
              }),
            };
          }
          return schema;
        });
Repository: hasura/graphql-engine
File: console/src/components/Services/Settings/Sidebar.tsx (TypeScript)

Code before:
import styles from '../../Common/TableCommon/Table.scss';

interface Metadata {
  inconsistentObjects: object[];
}

type SidebarProps = {
  location: Location;
  metadata: Metadata;
};

type SectionDataKey = 'actions' | 'status' | 'allowed-queries' | 'logout';

interface SectionData {
  key: SectionDataKey;
  link: string;
  dataTestVal: string;
  title: string | JSX.Element;
}

const Sidebar: React.FC<SidebarProps> = ({ location, metadata }) => {
  const sectionsData: SectionData[] = [];

  sectionsData.push({
    key: 'actions',
    link: `/settings/metadata-actions`,
    dataTestVal: 'metadata-actions-link',
    title: 'Metadata Actions',
  });

  const consistentIcon =
    metadata.inconsistentObjects.length === 0 ? <CheckIcon /> : <CrossIcon />;

  sectionsData.push({
    key: 'status',
    link: `/settings/metadata-status`,
    dataTestVal: 'metadata-status-link',
    title: (
      <div className={styles.display_flex}>
        Metadata Status
        <span className={styles.add_mar_left}>{consistentIcon}</span>
      </div>
    ),
  });

  sectionsData.push({
    key: 'allowed-queries',
    link: `/settings/allowed-queries`,
    dataTestVal: 'allowed-queries-link',
    title: 'Allowed Queries',
  });

Review comment (suggestion):
```suggestion
    link: '/settings/metadata-actions',
```

Code after:
import styles from '../../Common/TableCommon/Table.scss';

interface Metadata {
  inconsistentObjects: object[];
}

type SidebarProps = {
  location: RouteComponentProps<{}, {}>['location'];
  metadata: Metadata;
};

type SectionDataKey =
  | 'actions'
  | 'status'
  | 'allowed-queries'
  | 'logout'
  | 'about';

interface SectionData {
  key: SectionDataKey;
  link: string;
  dataTestVal: string;
  title: string | JSX.Element;
}

const Sidebar: React.FC<SidebarProps> = ({ location, metadata }) => {
  const sectionsData: SectionData[] = [];

  sectionsData.push({
    key: 'actions',
    link: '/settings/metadata-actions',
    dataTestVal: 'metadata-actions-link',
    title: 'Metadata Actions',
  });

  const consistentIcon =
    metadata.inconsistentObjects.length === 0 ? <CheckIcon /> : <CrossIcon />;

  sectionsData.push({
    key: 'status',
    link: '/settings/metadata-status',
    dataTestVal: 'metadata-status-link',
    title: (
      <div className={styles.display_flex}>
        Metadata Status
        <span className={styles.add_mar_left}>{consistentIcon}</span>
      </div>
    ),
  });

  sectionsData.push({