ronantakizawa/codereview-bench huggingface dataset:
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/RawSQL/RawSQL.js (JavaScript)

Code before:
import { CLI_CONSOLE_MODE } from '../../../../constants';
import NotesSection from './molecules/NotesSection';

/**
 * # RawSQL React FC
 * ## renders raw SQL page on route `/data/sql`
 * @component
 *
 * @param sql,
 * @param resultType,
 * @param result,
 * @param resultHeaders,
 * @param dispatch,
 * @param ongoingRequest,
 * @param lastError,
 * @param lastSuccess,
 * @param isModalOpen,
 * @param isCascadeChecked,
 * @param isMigrationChecked,
 * @param isTableTrackChecked,
 * @param migrationMode,
 * @param allSchemas,
 * @param location,
 * @returns
 */
const RawSQL = ({
  sql,
  resultType,
  result,
  resultHeaders,
  dispatch,
  ongoingRequest,
  lastError,
  lastSuccess,
  isModalOpen,
  isCascadeChecked,
  isMigrationChecked,
  isTableTrackChecked,
  migrationMode,
  allSchemas,
}) => {
  const styles = require('../../../Common/TableCommon/Table.scss');

  // local storage key for SQL
  const LS_RAW_SQL_SQL = 'rawSql:sql';

  /* hooks */

  // set up sqlRef to use in unmount
  const sqlRef = useRef(sql);


Review comment (suggestion):
```js
/**
 * @typedef Props
 * @property {<insert type>} sql
 * @property {<insert type>} resultType
 * @property {<insert type>} result
 * @property {<insert type>} resultHeaders
 * @property {<insert type>} dispatch
 * @property {<insert type>} ongoingRequest
 * @property {<insert type>} lastError
 * @property {<insert type>} lastSuccess
 * @property {<insert type>} isModalOpen
 * @property {<insert type>} isCascadeChecked
 * @property {<insert type>} isMigrationChecked
 * @property {<insert type>} isTableTrackChecked
 * @property {<insert type>} migrationMode
 * @property {<insert type>} allSchemas
 * @property {<insert type>} location
 *
 * @param {Props}
 */
```

Code after:
import { CLI_CONSOLE_MODE } from '../../../../constants';
import NotesSection from './molecules/NotesSection';
import Alert from '../../../Common/Alert';

/**
 * # RawSQL React FC
 * ## renders raw SQL page on route `/data/sql`
 *
 * @typedef Props
 * @property {string} sql
 * @property {string} resultType
 * @property {array} result
 * @property {array} resultHeaders
 * @property {function} dispatch
 * @property {boolean} ongoingRequest
 * @property {object} lastError
 * @property {boolean} lastSuccess
 * @property {boolean} isModalOpen
 * @property {boolean} isCascadeChecked
 * @property {boolean} isMigrationChecked
 * @property {boolean} isTableTrackChecked
 * @property {boolean} migrationMode
 * @property {array} allSchemas
 *
 * @param {Props}
 */
const RawSQL = ({
  sql,
  resultType,
  result,
  resultHeaders,
  dispatch,
  ongoingRequest,
  lastError,
  lastSuccess,
  isModalOpen,
  isCascadeChecked,
  isMigrationChecked,
  isTableTrackChecked,
  migrationMode,
  allSchemas,
}) => {
  const styles = require('../../../Common/TableCommon/Table.scss');

  // local storage key for SQL
  const LS_RAW_SQL_SQL = 'rawSql:sql';

  /* hooks */

  // set up sqlRef to use in unmount
  const sqlRef = useRef(sql);
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/RawSQL/RawSQL.js (JavaScript)

Code before:
        <div className="hidden alert alert-warning" role="alert">
          Running...
        </div>
      )}
      {lastError && (
        <div className="hidden alert alert-danger" role="alert">
          Error: {JSON.stringify(lastError)}
        </div>
      )}
      {lastSuccess && (
        <div className="hidden alert alert-success" role="alert">
          Executed Query
        </div>
      )}
      ;
    </div>
  </div>
);
/**
 * #RunButton
 * @component
 *
 * @param {*} { onClick, className }
 * @returns
 */
const RunButton = ({ onClick, className }) => {
  return (
    <Button
      type="submit"
      className={className}
      onClick={onClick}
      color="yellow"
      size="sm"
      data-test="run-sql"
    >
      Run!
    </Button>
  );
};

RawSQL.propTypes = {
  sql: PropTypes.string.isRequired,
  resultType: PropTypes.string.isRequired,
  result: PropTypes.array.isRequired,
  resultHeaders: PropTypes.array.isRequired,
  allSchemas: PropTypes.array.isRequired,
  dispatch: PropTypes.func.isRequired,
  ongoingRequest: PropTypes.bool.isRequired,
  lastError: PropTypes.object.isRequired,
  lastSuccess: PropTypes.bool.isRequired,
  isModalOpen: PropTypes.bool.isRequired,

Review comment (suggestion):
Is a separate component needed? There is no additional logic in `RunButton`, we can just use `Button` instead. JSX will be a couple of lines bigger, but there will be fewer components. 

Code after:
      {getMigrationWarningModal()}

      <div className={styles.add_mar_bottom}>{getResultTable()}</div>
    </div>
  );
};

RawSQL.propTypes = {
  sql: PropTypes.string.isRequired,
  resultType: PropTypes.string.isRequired,
  result: PropTypes.array.isRequired,
  resultHeaders: PropTypes.array.isRequired,
  allSchemas: PropTypes.array.isRequired,
  dispatch: PropTypes.func.isRequired,
  ongoingRequest: PropTypes.bool.isRequired,
  lastError: PropTypes.object.isRequired,
  lastSuccess: PropTypes.bool.isRequired,
  isModalOpen: PropTypes.bool.isRequired,
  isMigrationChecked: PropTypes.bool.isRequired,
  isTableTrackChecked: PropTypes.bool.isRequired,
  migrationMode: PropTypes.bool.isRequired,
  currentSchema: PropTypes.string.isRequired,
};

const mapStateToProps = state => ({
  ...state.rawSQL,
  migrationMode: state.main.migrationMode,
  currentSchema: state.tables.currentSchema,
  allSchemas: state.tables.allSchemas,
  serverVersion: state.main.serverVersion ? state.main.serverVersion : '',
});

const rawSQLConnector = connect => connect(mapStateToProps)(RawSQL);

export default rawSQLConnector;
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TablePermissions/JSONEditor.tsx (TypeScript)

Code before:
import React, { useState, useEffect } from 'react';
import AceEditor, { IAnnotation } from 'react-ace';
import { isJsonString } from '../../../Common/utils/jsUtils';

export interface JSONEditorProps {
  initData: string;
  onChange: (v: string) => void;
  data: string;
}

const JSONEditor: React.FC<JSONEditorProps> = ({
  initData,
  onChange,
  data,
}) => {
  const [value, setValue] = useState(initData || data || '');
  const [annotations, setAnnotations] = useState([] as IAnnotation[]);
  useEffect(() => {
    if (value !== data) onChange(value);
  }, [value]);

  // check and set error message
  useEffect(() => {
    if (isJsonString(value)) {
      setAnnotations([]);
    } else {
      setAnnotations([
        { row: 0, column: 0, text: 'Invalid JSON', type: 'error' },
      ]);
    }
    return () => {
      setAnnotations([]);
    };
  }, [value]);

  useEffect(() => {
    // set data to editor only if the prop has a valid json string
    // setting value from query editor will always have a valid json
    // any invalid json means, the value is set from this component so no need to set that again
    if (isJsonString(data)) setValue(data);
  }, [data]);

  return (
    <AceEditor
      mode="json"

Review comment (suggestion):
This hook also relies on `data`, right?
```suggestion
  }, [value, data]);
```

Code after:
import React, { useState, useEffect, useCallback } from 'react';
import AceEditor, { IAnnotation } from 'react-ace';
import { isJsonString } from '../../../Common/utils/jsUtils';
import { usePrevious } from '../../../../hooks/usePrevious';

export interface JSONEditorProps {
  initData: string;
  onChange: (v: string) => void;
  data: string;
}

const JSONEditor: React.FC<JSONEditorProps> = ({
  initData,
  onChange,
  data,
}) => {
  const [value, setValue] = useState(initData || data || '');
  const [annotations, setAnnotations] = useState<IAnnotation[]>([]);
  const prevData = usePrevious<string>(data);

  useEffect(() => {
    // if the data prop is changed do nothing
    if (prevData !== data) return;
    // when state gets new data, trigger parent callback
    if (value !== data) onChange(value);
  }, [value, data, prevData]);

  // check and set error message
  useEffect(() => {
    if (isJsonString(value)) {
      setAnnotations([]);
    } else {
      setAnnotations([
        { row: 0, column: 0, text: 'Invalid JSON', type: 'error' },
      ]);
    }
    return () => {
      setAnnotations([]);
    };
  }, [value]);

  useEffect(() => {
    // set data to editor only if the prop has a valid json string
    // setting value from query editor will always have a valid json
    // any invalid json means, the value is set from this component so no need to set that again
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TablePermissions/JSONEditor.tsx (TypeScript)

Code before:
import React, { useState, useEffect } from 'react';
import AceEditor, { IAnnotation } from 'react-ace';
import { isJsonString } from '../../../Common/utils/jsUtils';

export interface JSONEditorProps {
  initData: string;
  onChange: (v: string) => void;
  data: string;
}

const JSONEditor: React.FC<JSONEditorProps> = ({
  initData,
  onChange,
  data,
}) => {
  const [value, setValue] = useState(initData || data || '');
  const [annotations, setAnnotations] = useState([] as IAnnotation[]);
  useEffect(() => {
    if (value !== data) onChange(value);
  }, [value]);

  // check and set error message
  useEffect(() => {
    if (isJsonString(value)) {
      setAnnotations([]);
    } else {
      setAnnotations([
        { row: 0, column: 0, text: 'Invalid JSON', type: 'error' },
      ]);
    }
    return () => {
      setAnnotations([]);
    };
  }, [value]);

  useEffect(() => {
    // set data to editor only if the prop has a valid json string
    // setting value from query editor will always have a valid json
    // any invalid json means, the value is set from this component so no need to set that again
    if (isJsonString(data)) setValue(data);
  }, [data]);


Review comment (suggestion):
Let's do as little type assertion as possible. 
```suggestion
  const [annotations, setAnnotations] = useState<IAnnotation[]>([]);
```

Code after:
import React, { useState, useEffect, useCallback } from 'react';
import AceEditor, { IAnnotation } from 'react-ace';
import { isJsonString } from '../../../Common/utils/jsUtils';
import { usePrevious } from '../../../../hooks/usePrevious';

export interface JSONEditorProps {
  initData: string;
  onChange: (v: string) => void;
  data: string;
}

const JSONEditor: React.FC<JSONEditorProps> = ({
  initData,
  onChange,
  data,
}) => {
  const [value, setValue] = useState(initData || data || '');
  const [annotations, setAnnotations] = useState<IAnnotation[]>([]);
  const prevData = usePrevious<string>(data);

  useEffect(() => {
    // if the data prop is changed do nothing
    if (prevData !== data) return;
    // when state gets new data, trigger parent callback
    if (value !== data) onChange(value);
  }, [value, data, prevData]);

  // check and set error message
  useEffect(() => {
    if (isJsonString(value)) {
      setAnnotations([]);
    } else {
      setAnnotations([
        { row: 0, column: 0, text: 'Invalid JSON', type: 'error' },
      ]);
    }
    return () => {
      setAnnotations([]);
    };
  }, [value]);

  useEffect(() => {
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TablePermissions/JSONEditor.tsx (TypeScript)

Code before:

  // check and set error message
  useEffect(() => {
    if (isJsonString(value)) {
      setAnnotations([]);
    } else {
      setAnnotations([
        { row: 0, column: 0, text: 'Invalid JSON', type: 'error' },
      ]);
    }
    return () => {
      setAnnotations([]);
    };
  }, [value]);

  useEffect(() => {
    // set data to editor only if the prop has a valid json string
    // setting value from query editor will always have a valid json
    // any invalid json means, the value is set from this component so no need to set that again
    if (isJsonString(data)) setValue(data);
  }, [data]);

  return (
    <AceEditor
      mode="json"
      onChange={setValue}
      theme="github"
      height="5em"
      maxLines={15}
      width="100%"
      showPrintMargin={false}
      value={value}
      annotations={annotations}
    />
  );
};

export default JSONEditor;


Review comment (suggestion):
It causes console error when typing in the JSON row editor:
<img width="683" alt="Screenshot 2020-06-03 at 17 11 44" src="https://user-images.githubusercontent.com/9019397/83654376-50218f00-a5bd-11ea-824a-5e22136d387b.png">
```suggestion
      onChange={v => setValue(v)}
```

Code after:

  // check and set error message
  useEffect(() => {
    if (isJsonString(value)) {
      setAnnotations([]);
    } else {
      setAnnotations([
        { row: 0, column: 0, text: 'Invalid JSON', type: 'error' },
      ]);
    }
    return () => {
      setAnnotations([]);
    };
  }, [value]);

  useEffect(() => {
    // set data to editor only if the prop has a valid json string
    // setting value from query editor will always have a valid json
    // any invalid json means, the value is set from this component so no need to set that again
    if (isJsonString(data)) setValue(data);
  }, [data]);

  const onEditorValueChange = useCallback(newVal => setValue(newVal), [
    setValue,
  ]);

  return (
    <AceEditor
      mode="json"
      onChange={onEditorValueChange}
      theme="github"
      height="5em"
      maxLines={15}
      width="100%"
      showPrintMargin={false}
      value={value}
      annotations={annotations}
    />
  );
};

export default JSONEditor;
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/constants.js (JavaScript)

Code before:
  'integer',
  'bigserial',
  'smallint',
  'bigint',
];

export const COUNT_LIMIT = 100000;

export const Reals = ['float4', 'float8', 'numeric'];

export const Numerics = [...Integers, ...Reals];

export const defaultDataTypeToCast = 'text';

export const ERROR_CODES = {
  postgresError: {
    ERRORCODE: 'postgres-error',
    ERRORS: [
      'Not-NULL violation. null value in column <column-name> violates not-null constraint',
      'query execution failed',
      'postgres transaction error',
      'connection error',
      'postgres query error',
    ],
    HTTPCODE: '500',
  },
  permissionDenied: {
    ERRORCODE: 'permission-denied',
    ERRORS: [
      'select on <column/table> for role <role-name> is not allowed.',
      '<permission-type> on <table-name> for role <role-name> is not allowed',
      'role <role-name> does not have permission to <permission-type> column <column-name>',
    ],
    HTTPCODE: '400',
  },
  notExists: {
    ERRORCODE: 'not-exists',
    ERRORS: [
      'table <table-name> does not exist',
      'no such table/view exists in postgres : <table-name>',
      '<field-name> does not exist',
      '<column-name> does not exist',
      'tx does not exists',
      'no such column exists : <column-name>',
    ],
    HTTPCODE: '400',
  },
  alreadyTracked: {
    ERRORCODE: 'already-tracked',
    ERRORS: ['view/table already tracked : <table-name>'],
    HTTPCODE: '400',

Review comment (suggestion):
`code` for simplicity. Also, let's stick to either upper case or lower case. This isn't comfortable to read:
```
ERROR_CODES.dependencyError.ERRORCODE
```

Code after:
  'integer',
  'bigserial',
  'smallint',
  'bigint',
];

export const COUNT_LIMIT = 100000;

export const Reals = ['float4', 'float8', 'numeric'];

export const Numerics = [...Integers, ...Reals];

export const defaultDataTypeToCast = 'text';

// https://hasura.io/docs/1.0/graphql/manual/api-reference/schema-metadata-api/index.html#error-codes
export const ERROR_CODES = {
  postgresError: {
    code: 'postgres-error',
    httpCode: '500',
  },
  permissionDenied: {
    code: 'permission-denied',
    httpCode: '400',
  },
  notExists: {
    code: 'not-exists',

    httpCode: '400',
  },
  alreadyTracked: {
    code: 'already-tracked',
    httpCode: '400',
  },
  accessDenied: {
    code: 'access-denied',

    httpCode: '400',
  },
  notSupported: {
    code: 'not-supported',
    httpCode: '400',
  },
  alreadyExists: {
    code: 'already-exists',
    httpCode: '400',
  },
  invalidJson: {
    code: 'invalid-json',
    httpCode: '400',
  },
  invalidHeaders: {
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/Function/Modify/ModifyCustomFunction.js (JavaScript)

Code before:
          appPrefix={appPrefix}
          currentTab="modify"
          heading={functionName}
          tabsInfo={tabInfo}
          breadCrumbs={breadCrumbs}
          baseUrl={functionBaseUrl}
          showLoader={isFetching}
          testPrefix={'functions'}
        />
        <br />
        {/*
        <h4>Function Definition:</h4>
        */}
        <div className={styles.sqlBlock}>
          <TextAreaWithCopy
            copyText={functionDefinition}
            textLanguage={'sql'}
            id={'copyCustomFunctionSQL'}
          />
        </div>
        <SessionVarSection
          key={functionName}
          functionName={functionName}
          dispatch={this.props.dispatch}
          configuration={configuration}
          loading={isRequesting || isDeleting || isUntracking || isFetching}
          onSessVarUpdate={this.onSessVarUpdate}
        />
        {migrationMode
          ? [<hr key="modify-custom-function-divider" />, generateMigrateBtns()]
          : null}
      </div>
    );
  }
}

ModifyCustomFunction.propTypes = {
  functions: PropTypes.array.isRequired,
};

export default ModifyCustomFunction;


Review comment (refactor):
I'm wondering how we can simplify this and have fewer variables in state.
Would:
```ts
loading = null | {
   isRequesting?: boolean,
   isDeleting?: boolean, 
   isUntracking?: boolean,
   isFetching?: boolean,
}
```
work?

Most places check if any of the loading states is true. So we can just pass `loading`. 
Otherwise do `loading.isUntracking`, etc.

Code after:
              <RawSqlButton
                className={styles.add_mar_right}
                sql={functionDefinition}
                dispatch={dispatch}
                data-test="modify-view"
              >
                Modify
              </RawSqlButton>
            </span>
          </h4>
        </div>

        <div className={styles.sqlBlock}>
          <TextAreaWithCopy
            copyText={functionDefinition}
            textLanguage={'sql'}
            id="copyCustomFunctionSQL"
          />
        </div>
        <SessionVarSection
          key={functionName}
          functionName={functionName}
          configuration={configuration}
          loading={loading}
          onSessVarUpdate={this.onSessVarUpdate}
        />
        {migrationMode
          ? [<hr key="modify-custom-function-divider" />, generateMigrateBtns()]
          : null}
      </div>
    );
  }
}

ModifyCustomFunction.propTypes = {
  functions: PropTypes.array.isRequired,
};

export default ModifyCustomFunction;
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/Function/customFunctionReducer.js (JavaScript)

Code before:
    case DELETE_CUSTOM_FUNCTION_FAIL:
      return {
        ...state,
        isDeleting: false,
        isError: action.data,
      };
    case DELETING_CUSTOM_FUNCTION:
      return {
        ...state,
        isDeleting: true,
        isError: null,
      };

    case UNTRACK_CUSTOM_FUNCTION_FAIL:
      return {
        ...state,
        isUntracking: false,
        isError: action.data,
      };
    case UNTRACKING_CUSTOM_FUNCTION:
      return {
        ...state,
        isUntracking: true,
        isError: null,
      };
    case SESSVAR_CUSTOM_FUNCTION_ADDING:
      return {
        ...state,
        isUpdating: true,
        isError: null,
      };
    case SESSVAR_CUSTOM_FUNCTION_ADD_FAIL:
      return {
        ...state,
        isUpdating: false,
        isError: action.data,
      };
    case SESSVAR_CUSTOM_FUNCTION_ADD_SUCCESS:
      return {
        ...state,
        isUpdating: false,
        isError: null,
      };
    default:
      return {
        ...state,
      };
  }
};

/* End of it */

Review comment (suggestion):
```suggestion
    case SESSVAR_CUSTOM_FUNCTION_REQUEST:
```

Code after:
    case CUSTOM_FUNCTION_FETCH_FAIL:
      return {
        ...state,
        isFetching: false,
        isFetchError: action.data,
      };
    case DELETE_CUSTOM_FUNCTION_FAIL:
      return {
        ...state,
        isDeleting: false,
        isError: action.data,
      };
    case DELETING_CUSTOM_FUNCTION:
      return {
        ...state,
        isDeleting: true,
        isError: null,
      };

    case UNTRACK_CUSTOM_FUNCTION_FAIL:
      return {
        ...state,
        isUntracking: false,
        isError: action.data,
      };
    case UNTRACKING_CUSTOM_FUNCTION:
      return {
        ...state,
        isUntracking: true,
        isError: null,
      };
    case SESSVAR_CUSTOM_FUNCTION_REQUEST:
      return {
        ...state,
        isUpdating: true,
        isError: null,
      };
    case SESSVAR_CUSTOM_FUNCTION_ADD_FAIL:
      return {
        ...state,
        isUpdating: false,
        isError: action.data,
      };
    case SESSVAR_CUSTOM_FUNCTION_ADD_SUCCESS:
      return {
        ...state,
        isUpdating: false,
        isError: null,
      };
    default:
      return {