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

Code before:
        dispatch(vMakeRequest());
        dispatch(
          showSuccessNotification(
            'Row deleted!',
            'Affected rows: ' + data.affected_rows
          )
        );
      },
      err => {
        dispatch(showErrorNotification('Deleting row failed!', err.error, err));
      }
    );
  };
};

const deleteItems = pkClauses => {
  return (dispatch, getState) => {
    const confirmMessage = 'This will permanently delete rows from this table';
    const isOk = getConfirmation(confirmMessage);
    if (!isOk) {
      return;
    }

    const state = getState();

    const url = Endpoints.query;
    const reqBody = {
      type: 'bulk',
      args: pkClauses.map(pkClause => ({
        type: 'delete',
        args: {
          table: {
            name: state.tables.currentTable,
            schema: state.tables.currentSchema,
          },
          where: pkClause,
        },
      })),
    };
    const options = {
      method: 'POST',
      body: JSON.stringify(reqBody),
      headers: dataHeaders(getState),
      credentials: globalCookiePolicy,
    };
    dispatch(requestAction(url, options)).then(
      data => {
        const affected = data.reduce((acc, d) => acc + d.affected_rows, 0);
        dispatch(vMakeRequest());
        dispatch(
          showSuccessNotification('Rows deleted!', 'Affected rows: ' + affected)

Review comment (suggestion):
There doesn't seem to be a need for this variable. You can directly pass `Endpoints.query` to `requestAction`.

Code after:
      data => {
        dispatch(vMakeRequest());
        dispatch(
          showSuccessNotification(
            'Row deleted!',
            'Affected rows: ' + data.affected_rows
          )
        );
      },
      err => {
        dispatch(showErrorNotification('Deleting row failed!', err.error, err));
      }
    );
  };
};

const deleteItems = pkClauses => {
  return (dispatch, getState) => {
    const confirmMessage = 'This will permanently delete rows from this table';
    const isOk = getConfirmation(confirmMessage);
    if (!isOk) {
      return;
    }

    const state = getState();

    const reqBody = {
      type: 'bulk',
      args: getBulkDeleteQuery(
        pkClauses,
        state.tables.currentTable,
        state.tables.currentSchema
      ),
    };
    const options = {
      method: 'POST',
      body: JSON.stringify(reqBody),
      headers: dataHeaders(getState),
      credentials: globalCookiePolicy,
    };
    dispatch(requestAction(Endpoints.query, options)).then(
      data => {
        const affected = data.reduce((acc, d) => acc + d.affected_rows, 0);
        dispatch(vMakeRequest());
        dispatch(
          showSuccessNotification('Rows deleted!', 'Affected rows: ' + affected)
        );
      },
      err => {
        dispatch(
          showErrorNotification('Deleting rows failed!', err.error, err)
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TableBrowseRows/ViewActions.js (JavaScript)

Code before:
            'Row deleted!',
            'Affected rows: ' + data.affected_rows
          )
        );
      },
      err => {
        dispatch(showErrorNotification('Deleting row failed!', err.error, err));
      }
    );
  };
};

const deleteItems = pkClauses => {
  return (dispatch, getState) => {
    const confirmMessage = 'This will permanently delete rows from this table';
    const isOk = getConfirmation(confirmMessage);
    if (!isOk) {
      return;
    }

    const state = getState();

    const url = Endpoints.query;
    const reqBody = {
      type: 'bulk',
      args: pkClauses.map(pkClause => ({
        type: 'delete',
        args: {
          table: {
            name: state.tables.currentTable,
            schema: state.tables.currentSchema,
          },
          where: pkClause,
        },
      })),
    };
    const options = {
      method: 'POST',
      body: JSON.stringify(reqBody),
      headers: dataHeaders(getState),
      credentials: globalCookiePolicy,
    };
    dispatch(requestAction(url, options)).then(
      data => {
        const affected = data.reduce((acc, d) => acc + d.affected_rows, 0);
        dispatch(vMakeRequest());
        dispatch(
          showSuccessNotification('Rows deleted!', 'Affected rows: ' + affected)
        );
      },
      err => {

Review comment (refactor):
Can you make a new fucntion `getDeleteQuery` in `src/components/Common/utils/v1QueryUtils.js`.
For #DRY purposes. Later, during the rewrite, we can use that function for all delete queries in console.

Code after:
          showSuccessNotification(
            'Row deleted!',
            'Affected rows: ' + data.affected_rows
          )
        );
      },
      err => {
        dispatch(showErrorNotification('Deleting row failed!', err.error, err));
      }
    );
  };
};

const deleteItems = pkClauses => {
  return (dispatch, getState) => {
    const confirmMessage = 'This will permanently delete rows from this table';
    const isOk = getConfirmation(confirmMessage);
    if (!isOk) {
      return;
    }

    const state = getState();

    const reqBody = {
      type: 'bulk',
      args: getBulkDeleteQuery(
        pkClauses,
        state.tables.currentTable,
        state.tables.currentSchema
      ),
    };
    const options = {
      method: 'POST',
      body: JSON.stringify(reqBody),
      headers: dataHeaders(getState),
      credentials: globalCookiePolicy,
    };
    dispatch(requestAction(Endpoints.query, options)).then(
      data => {
        const affected = data.reduce((acc, d) => acc + d.affected_rows, 0);
        dispatch(vMakeRequest());
        dispatch(
          showSuccessNotification('Rows deleted!', 'Affected rows: ' + affected)
        );
      },
      err => {
        dispatch(
          showErrorNotification('Deleting rows failed!', err.error, err)
        );
      }
    );
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TableBrowseRows/ViewRows.js (JavaScript)

Code before:
      }

      const maxContentCellWidth = maxContentWidth + CONTENT_PADDING;

      const headerCellWidth =
        getTextWidth(header, HEADER_FONT) + HEADER_PADDING;

      return Math.min(
        MAX_WIDTH,
        Math.max(maxContentCellWidth, headerCellWidth)
      );
    };

    _gridHeadings.push({
      Header: '',
      accessor: 'tableRowActionButtons',
      id: 'tableRowActionButtons',
      width: 152,
    });

    _gridHeadings.push({
      Header: (
        <div className={styles.tableCenterContent}>
          <input
            className={styles.inputCheckbox}
            style={{ marginBottom: '2px' }}
            checked={
              curRows.length > 0 && selectedRows.length === curRows.length
            }
            type="checkbox"
            onChange={handleAllCheckboxChange}
          />
        </div>
      ),
      accessor: 'tableRowSelectAction',
      id: 'tableRowSelectAction',
      width: 60,
    });

    _columns.map(col => {
      const columnName = col.column_name;

      let sortIcon = 'fa-sort';
      if (curQuery.order_by && curQuery.order_by.length) {
        sortIcon = '';

        curQuery.order_by.forEach(orderBy => {
          if (orderBy.column === columnName) {
            sortIcon = orderBy.type === 'asc' ? 'fa-caret-up' : 'fa-caret-down';
          }
        });

Review comment (suggestion):
Please avoid using `style` prop unless absolutely necessary. Things  could easily get inconsistent if the style prop is used at a multiple places. 
You can just write a tiny-winy CSS class for this :) 

Code after:
          }
        }
      }

      const maxContentCellWidth = maxContentWidth + CONTENT_PADDING;

      const headerCellWidth =
        getTextWidth(header, HEADER_FONT) + HEADER_PADDING;

      return Math.min(
        MAX_WIDTH,
        Math.max(maxContentCellWidth, headerCellWidth)
      );
    };

    _gridHeadings.push({
      Header: '',
      accessor: 'tableRowActionButtons',
      id: 'tableRowActionButtons',
      width: 152,
    });

    _gridHeadings.push({
      Header: (
        <div className={styles.tableCenterContent}>
          <input
            className={`${styles.inputCheckbox} ${styles.headerInputCheckbox}`}
            checked={
              curRows.length > 0 && selectedRows.length === curRows.length
            }
            disabled={_disableBulkSelect}
            title={_disableBulkSelect ? 'No primary key to identify row' : ''}
            type="checkbox"
            onChange={handleAllCheckboxChange}
          />
        </div>
      ),
      accessor: 'tableRowSelectAction',
      id: 'tableRowSelectAction',
      width: 60,
    });

    _columns.map(col => {
      const columnName = col.column_name;

      let sortIcon = 'fa-sort';
      if (curQuery.order_by && curQuery.order_by.length) {
        sortIcon = '';

        curQuery.order_by.forEach(orderBy => {
          if (orderBy.column === columnName) {
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TableBrowseRows/ViewRows.js (JavaScript)

Code before:
        onPageChange={handlePageChange}
        onPageSizeChange={handlePageSizeChange}
        page={Math.floor(curFilter.offset / curFilter.limit)}
      />
    );
  };

  // Is this ViewRows visible
  let isVisible = false;
  if (!curRelName || curRelName === activePath[curDepth]) {
    isVisible = true;
  }

  return (
    <div className={isVisible ? '' : 'hide '}>
      {getFilterQuery()}
      <div className={`row ${styles.add_mar_top}`}>
        {selectedRows.length > 0 && (
          <div
            className={`${styles.display_flex}`}
            style={{ paddingLeft: '18px' }}
          >
            <b className={styles.padd_small_right}>Selected:</b>
            {selectedRows.length}
            <button
              className={`add_mar_right_small___3slZA btn btn-xs btn-default ${styles.bulkDeleteButton}`}
              title="Delete selected rows"
              onClick={handleDeleteItems}
            >
              <i className="fa fa-trash" />
            </button>
          </div>
        )}
        <div className="col-xs-12">
          <div className={styles.tableContainer}>{renderTableBody()}</div>
          <br />
          <br />
          <div>{getChildComponent()}</div>
        </div>
      </div>
    </div>
  );
};

export default ViewRows;


Review comment (suggestion):
You have probably copied this classname from inspect-element devtools. It won't work because these names are autogenerated on the fly while building.

You can replace `add_mar_right_small__3slZA` with `${styles.add_mar_right_small}`. It is the same classname.

Code after:
          sortByColumn(column.id, !e.shiftKey);
        }

        disableSortColumn = false;
      },
    });

    const getResizerProps = (finalState, none, column, ctx) => ({
      onMouseDown: e => {
        disableSortColumn = true;
        ctx.resizeColumnStart(e, column, false);
      },
    });

    const handlePageChange = page => {
      if (curFilter.offset !== page * curFilter.limit) {
        dispatch(setOffset(page * curFilter.limit));
        dispatch(runQuery(tableSchema));
      }
    };

    const handlePageSizeChange = size => {
      if (curFilter.size !== size) {
        dispatch(setLimit(size));
        dispatch(setOffset(0));
        dispatch(runQuery(tableSchema));
      }
    };

    return (
      <DragFoldTable
        className="-highlight -fit-content"
        data={_gridRows}
        columns={_gridHeadings}
        resizable
        manual
        sortable={false}
        minRows={0}
        getTheadThProps={getTheadThProps}
        getResizerProps={getResizerProps}
        showPagination={!isSingleRow}
        pageSize={curFilter.limit}
        pages={Math.ceil(count / curFilter.limit)}
        onPageChange={handlePageChange}
        onPageSizeChange={handlePageSizeChange}
        page={Math.floor(curFilter.offset / curFilter.limit)}
      />
    );
  };

  // Is this ViewRows visible
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/Add/CheckConstraints.js (JavaScript)

Code before:
        ) : null;
      }

      return (
        <div>
          <b>{existingConstraintName}</b>
        </div>
      );
    };

    const expandedLabel = () => {
      if (isLast) {
        return null;
      }

      return (
        <div>
          <b>{existingConstraintName}</b>
        </div>
      );
    };

    const expandButtonText = isLast ? 'Add a check constraint' : 'Edit';

    const expandedContent = () => {
      return (
        <div>
          <div className={styles.add_mar_bottom}>
            <div className={styles.add_mar_bottom_mid}>
              <b>Constraint Name:</b>
            </div>
            <input
              type="text"
              value={name}
              onChange={onChangeName}
              className={`form-control ${styles.wd50percent}`}
            />
          </div>
          <div>
            <div className={styles.add_mar_bottom_mid}>
              <b>Check Expression: </b>
              <ToolTip message="Boolean expression that must be satisfied for all rows in the table. e.g. min_price >= 0 AND max_price >= min_price" />
              &nbsp;&nbsp;
              <KnowMoreLink href="https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-CHECK-CONSTRAINTS" />
            </div>
            <AceEditor
              mode="sql"
              theme="github"
              name={existingConstraintName}
              onChange={onChangeCheck}
              value={check}

Review comment (refactor):
We should pull this out into a reusable component. Because the same UI is replicated on the `Modify` page as well. What do you think?

Code after:
      return (
        <div>
          <b>{existingConstraintName}</b>
        </div>
      );
    };

    const expandedLabel = () => {
      if (isLast) {
        return null;
      }

      return (
        <div>
          <b>{existingConstraintName}</b>
        </div>
      );
    };

    const expandButtonText = isLast ? 'Add a check constraint' : 'Edit';

    let saveFunc;
    if (name && check) {
      saveFunc = toggle => {
        dispatch(
          setCheckConstraints(
            addConstraintsState.filter(c => c.name && c.check)
          )
        );
        toggle();
      };
    }

    let removeFunc;
    if (!isLast) {
      removeFunc = toggle => {
        dispatch(removeCheckConstraint(i));
        setAddConstraintsState(prev => prev.filter((_, idx) => idx !== i));
        toggle();
      };
    }
    const expandedContent = () => (
      <ConstraintExpandedContent
        nameOnChange={onChangeName}
        constraintName={existingConstraintName}
        name={name}
        checkOnChange={onChangeCheck}
        check={check}
      />
    );
Repository: hasura/graphql-engine
File: scripts/tag-release.sh (Shell)

Code before:
ROOT="$(readlink -f ${BASH_SOURCE[0]%/*}/../)"

# check if required argument is set
if [ -z "$1" ]; then
    echo "Usage: ./tag-release.sh <tag> [<optional-tag-message>]"
    exit 1
fi

# assign arguments to variables
TAG=$1
MESSAGE=$2

# default message to tag
if [ -z "$MESSAGE" ]; then
    MESSAGE="$TAG"
fi

# replace the image version with latest tag for all references in install-manifests
find "$ROOT/install-manifests" \
     "$ROOT/scripts/cli-migrations" \
     -type f -exec sed -i -E \
     's#(hasura/graphql-engine:)v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(\-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(.*)*$#\1'"${TAG}"'\9#' {} \;

# add the latest tag to the catalog_versions file
[ -n "$(tail -c1 catalog_versions)" ] && echo >> catalog_versions 
echo $TAG $(cat catalog_version) >> catalog_versions

git add "$ROOT/install-manifests" \
        "$ROOT/scripts/cli-migrations"

git commit -m "update manifests to $TAG"

git tag -a "$TAG" -m "$MESSAGE"

echo "tagged $TAG"


Review comment (suggestion):
I think you want to use `$ROOT` to build the paths here. (Also, the first line seems needlessly defensive to me, but I guess it’s fine if you want to keep it.)

Code after:
ROOT="$(readlink -f ${BASH_SOURCE[0]%/*}/../)"

# check if required argument is set
if [ -z "$1" ]; then
    echo "Usage: ./tag-release.sh <tag> [<optional-tag-message>]"
    exit 1
fi

# assign arguments to variables
TAG=$1
MESSAGE=$2

# default message to tag
if [ -z "$MESSAGE" ]; then
    MESSAGE="$TAG"
fi

# replace the image version with latest tag for all references in install-manifests
find "$ROOT/install-manifests" \
     "$ROOT/scripts/cli-migrations" \
     -type f -exec sed -i -E \
     's#(hasura/graphql-engine:)v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(\-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(.*)*$#\1'"${TAG}"'\9#' {} \;

# add the latest tag to the catalog_versions file
[ -n "$(tail -c1 "$ROOT/src-rsr/catalog_versions.txt")" ] && echo >> "$ROOT/src-rsr/catalog_versions.txt"
echo $TAG $(cat "$ROOT/src-rsr/catalog_version.txt") >> "$ROOT/src-rsr/catalog_versions.txt"

git add "$ROOT/install-manifests" \
        "$ROOT/scripts/cli-migrations"

git commit -m "update manifests to $TAG"

git tag -a "$TAG" -m "$MESSAGE"

echo "tagged $TAG"
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TableBrowseRows/ViewActions.js (JavaScript)

Code before:
const UPDATE_TRIGGER_ROW = 'ViewTable/UPDATE_TRIGGER_ROW';
const UPDATE_TRIGGER_FUNCTION = 'ViewTable/UPDATE_TRIGGER_FUNCTION';

// const V_ADD_WHERE;
// const V_REMOVE_WHERE;
// const V_SET_LIMIT;
// const V_SET_OFFSET;
// const V_ADD_SORT;
// const V_REMOVE_SORT;

/* ****************** action creators *************/

const vExpandRow = rowKey => ({
  type: V_EXPAND_ROW,
  data: rowKey,
});

const vCollapseRow = () => ({
  type: V_COLLAPSE_ROW,
});

const vSetDefaults = () => ({ type: V_SET_DEFAULTS });

const vMakeRequest = () => {
  return (dispatch, getState) => {
    const { tables } = getState();
    const url = Endpoints.query;
    const originalTable = getState().tables.currentTable;
    dispatch({ type: V_REQUEST_PROGRESS, data: true });

    const requestBody = {
      type: 'bulk',
      args: [
        generateSelectQuery(
          'select',
          generateTableDef(tables.currentTable, tables.currentSchema),
          tables.view.query
        ),
        getRunSqlQuery(
          getEstimateCountQuery(tables.currentSchema, tables.currentTable)
        ),
      ],
    };
    const options = {
      method: 'POST',
      body: JSON.stringify(requestBody),
      headers: dataHeaders(getState),
      credentials: globalCookiePolicy,
    };
    return dispatch(requestAction(url, options)).then(
      data => {

Review comment (refactor):
### Suggestion

I think it would make sense to extract the relevant parts here itself rather than rocking the `tables` object everywhere. This is because, in large schemas, the `tables` object could be **huge**.

You can just:

```js
const {
  currentTable: originalTable,
  currentSchema
} = getState().tables;
```

Code after:
const UPDATE_TRIGGER_ROW = 'ViewTable/UPDATE_TRIGGER_ROW';
const UPDATE_TRIGGER_FUNCTION = 'ViewTable/UPDATE_TRIGGER_FUNCTION';

// const V_ADD_WHERE;
// const V_REMOVE_WHERE;
// const V_SET_LIMIT;
// const V_SET_OFFSET;
// const V_ADD_SORT;
// const V_REMOVE_SORT;

/* ****************** action creators *************/

const vExpandRow = rowKey => ({
  type: V_EXPAND_ROW,
  data: rowKey,
});

const vCollapseRow = () => ({
  type: V_COLLAPSE_ROW,
});

const vSetDefaults = () => ({ type: V_SET_DEFAULTS });

const vMakeRowsRequest = () => {
  return (dispatch, getState) => {
    const {
      currentTable: originalTable,
      currentSchema,
      view,
    } = getState().tables;

    const url = Endpoints.query;
    dispatch({ type: V_REQUEST_PROGRESS, data: true });

    const requestBody = {
      type: 'bulk',
      args: [
        generateSelectQuery(
          'select',
          generateTableDef(originalTable, currentSchema),
          view.query
        ),
        getRunSqlQuery(getEstimateCountQuery(currentSchema, originalTable)),
      ],
    };
    const options = {
      method: 'POST',
      body: JSON.stringify(requestBody),
      headers: dataHeaders(getState),
      credentials: globalCookiePolicy,
    };
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TableBrowseRows/localStorageUtils.ts (TypeScript)

Code before:
const setLSState = (key: string, data: any) => {
  window.localStorage.setItem(key, JSON.stringify(data));
};

const getLSState = (key: string, defaultValue: null | {} = {}) => {
  try {
    const p = window.localStorage.getItem(key);
    if (p) {
      return JSON.parse(p);
    }
    return defaultValue;
  } catch (e) {
    console.error(e);
    setLSState(key, defaultValue);
    return defaultValue;
  }
};

const columnsCollapsedKey = 'data:collapsed';

type CollapesEntry = Record<string, boolean>;
type CollapesState = Record<string, CollapesEntry>;

export const handleCollapseChange = (
  tableName: string,
  schemaName: string,
  collapsedData: CollapesEntry
) => {
  const currentCollapsed = getLSState(columnsCollapsedKey);
  const newCollapsed = {
    ...currentCollapsed,
    [`${schemaName}.${tableName}`]: collapsedData,
  };

  setLSState(columnsCollapsedKey, newCollapsed);
};

export const getCollapsedColumns = (tableName: string, schemaName: string) => {
  const collapsedData = getLSState(columnsCollapsedKey) as CollapesState;
  return collapsedData[`${schemaName}.${tableName}`];
};

const columnsOrderKey = 'data:order';

type OrderEntry = { newOrder: number; defaultOrder: number };
type OrderState = Record<string, OrderEntry[]>;

const compareReorderItems = (item1: OrderEntry) => (item2: OrderEntry) =>
  item1.newOrder === item2.newOrder &&

Review comment (suggestion):
```suggestion
export const persistColumnCollapseChange = (
```

Code after:
export const setLSState = (key: string, data: string) => {
  window.localStorage.setItem(key, data);
};

export const getLSState = (key: string) => {
  return window.localStorage.getItem(key);
};

export const parseLSState = (data: any) => {
  try {
    if (data) {
      return JSON.parse(data);
    }
    return null;
  } catch {
    return null;
  }
};

const columnsCollapsedKey = 'data:collapsed';

type CollapesEntry = Record<string, boolean>;

export const persistColumnCollapseChange = (
  tableName: string,
  schemaName: string,
  collapsedData: CollapesEntry
) => {
  const state = getLSState(columnsCollapsedKey);
  const currentCollapsed = parseLSState(state) || {};
  const newCollapsed = {
    ...currentCollapsed,
    [`${schemaName}.${tableName}`]: collapsedData,
  };

  setLSState(columnsCollapsedKey, JSON.stringify(newCollapsed));
};

export const getPersistedCollapsedColumns = (
  tableName: string,
  schemaName: string
) => {
  const collapsedData = parseLSState(getLSState(columnsCollapsedKey)) || {};
  return collapsedData[`${schemaName}.${tableName}`];
};

const columnsOrderKey = 'data:order';

type OrderEntry = { newOrder: number; defaultOrder: number };