ronantakizawa/codereview-bench huggingface dataset:
Repository: appsmithorg/appsmith
File: app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (TypeScript)

Code before:
} as const;

const SCROLL_AREA_STYLE = {
  height: 34,
  top: 1,
};

export const DismissibleTabBar = ({
  children,
  disableAdd = false,
  onTabAdd,
}: DismissibleTabBarProps) => {
  const [isStuck, setIsStuck] = useState(false);

  const containerRef = useRef<HTMLDivElement | null>(null);
  const sentinelRef = useRef<HTMLDivElement | null>(null);

  const handleAdd = disableAdd ? noop : onTabAdd;

  useEffect(function observeSticky() {
    if (!containerRef.current || !sentinelRef.current) return;

    const observer = new IntersectionObserver(
      ([entry]) => {
        if (entry.isIntersecting) {
          setIsStuck(false);
        } else {
          setIsStuck(true);
        }
      },
      {
        root: containerRef.current,
        threshold: 1.0,
      },
    );

    observer.observe(sentinelRef.current);

    return () => observer.disconnect();
  }, []);

  return (
    <Styled.Root>
      <ScrollArea
        data-testid="t--editor-tabs"
        options={SCROLL_AREA_OPTIONS}
        ref={containerRef}
        size="sm"
        style={SCROLL_AREA_STYLE}
      >
        <Styled.TabsContainer data-testid="t--tabs-container" role="tablist">

Review comment (suggestion):
maybe we should rename this to `isOverlapping` or `isIntersecting`
isStuck seems to be derived from `sticky` but the does not convey the meaning well

Code after:
} as const;

const SCROLL_AREA_STYLE = {
  height: 34,
  top: 1,
};

export const DismissibleTabBar = ({
  children,
  disableAdd = false,
  onTabAdd,
}: DismissibleTabBarProps) => {
  const [isLeftIntersecting, setIsLeftIntersecting] = useState(false);
  const [isRightIntersecting, setIsRightIntersecting] = useState(false);

  const containerRef = useRef<HTMLDivElement | null>(null);
  const sentinelLeftRef = useRef<HTMLDivElement | null>(null);
  const sentinelRightRef = useRef<HTMLDivElement | null>(null);

  const handleAdd = disableAdd ? noop : onTabAdd;

  useEffect(function observeSticky() {
    if (
      !containerRef.current ||
      !sentinelLeftRef.current ||
      !sentinelRightRef.current
    )
      return;

    const observer = new IntersectionObserver(
      (entries) => {
        entries.forEach((entry) => {
          if (entry.target === sentinelLeftRef.current) {
            setIsLeftIntersecting(!entry.isIntersecting);
          }

          if (entry.target === sentinelRightRef.current) {
            setIsRightIntersecting(!entry.isIntersecting);
          }
        });
      },
      {
        root: containerRef.current,
        threshold: 1.0,
      },
    );

    observer.observe(sentinelLeftRef.current);
    observer.observe(sentinelRightRef.current);

    return () => observer.disconnect();
Repository: appsmithorg/appsmith
File: app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx (TypeScript)

Code before:
import React from "react";
import { noop } from "lodash";
import { useBoolean } from "usehooks-ts";

import { DismissibleTab } from "../..";
import { EditableEntityName } from "..";

import type { EditableDismissibleTabProps } from "./EditableDismissibleTab.types";

export const EditableDismissibleTab = (props: EditableDismissibleTabProps) => {
  const {
    dataTestId,
    isActive,
    isEditable = true,
    isLoading,
    name,
    onClick,
    onClose,
    onNameSave,
    startIcon,
    validateName,
  } = props;

  const {
    setFalse: exitEditMode,
    setTrue: enterEditMode,
    value: isEditing,
  } = useBoolean(false);

  const handleDoubleClick = isEditable ? enterEditMode : noop;

  return (
    <DismissibleTab
      dataTestId={dataTestId}
      isActive={isActive}
      onClick={onClick}
      onClose={onClose}
      onDoubleClick={handleDoubleClick}
    >
      <EditableEntityName
        isEditing={isEditing}
        isLoading={isLoading}
        name={name}
        onEditComplete={exitEditMode}
        onNameSave={onNameSave}

Review comment (suggestion):
This is an unnecessary and potentially harmful optimization, there is no and will not be an `endIcon`, so it's more confusing than helpful.

Code after:
import React from "react";
import { noop } from "lodash";
import { useBoolean } from "usehooks-ts";

import { DismissibleTab } from "../..";
import { EditableEntityName } from "..";

import type { EditableDismissibleTabProps } from "./EditableDismissibleTab.types";

export const EditableDismissibleTab = (props: EditableDismissibleTabProps) => {
  const {
    canEdit,
    dataTestId,
    icon,
    isActive,
    isEditable = true,
    isLoading,
    name,
    onClick,
    onClose,
    onNameSave,
    validateName,
  } = props;

  const {
    setFalse: exitEditMode,
    setTrue: enterEditMode,
    value: isEditing,
  } = useBoolean(false);

  const handleDoubleClick = isEditable ? enterEditMode : noop;

  return (
    <DismissibleTab
      dataTestId={dataTestId}
      isActive={isActive}
      onClick={onClick}
      onClose={onClose}
      onDoubleClick={handleDoubleClick}
    >
      <EditableEntityName
        canEdit={canEdit}
        icon={icon}
        isEditing={isEditing}
        isLoading={isLoading}
Repository: appsmithorg/appsmith
File: app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx (TypeScript)

Code before:
/* eslint-disable no-console */
import React from "react";
import type { Meta, StoryObj } from "@storybook/react";

import { EditableDismissibleTab } from ".";
import { Icon } from "../..";

const meta: Meta<typeof EditableDismissibleTab> = {
  title: "ADS/Templates/Editable Dismissible Tab",
  component: EditableDismissibleTab,
};

const JSIcon = () => {
  return <Icon name="js-yellow" size="sm" />;
};

export default meta;

type Story = StoryObj<typeof EditableDismissibleTab>;

export const Basic: Story = {
  args: {
    isActive: true,
    dataTestId: "t--dismissible-tab",
    icon: JSIcon(),
    name: "Hello",

    onNameSave: console.log,

Review comment (suggestion):
Edit mode in this story stoped working, the input is not responding to `doubleClick`. Probably because there of the newly introduced `canEdit` prop

Code after:
/* eslint-disable no-console */
import React from "react";
import type { Meta, StoryObj } from "@storybook/react";

import { EditableDismissibleTab } from ".";
import { Icon } from "../..";

const meta: Meta<typeof EditableDismissibleTab> = {
  title: "ADS/Templates/Editable Dismissible Tab",
  component: EditableDismissibleTab,
};

const JSIcon = () => {
  return <Icon name="js-yellow" size="sm" />;
};

export default meta;

type Story = StoryObj<typeof EditableDismissibleTab>;

export const Basic: Story = {
  args: {
    isActive: true,
    dataTestId: "t--dismissible-tab",
    icon: JSIcon(),
    name: "Hello",
    canEdit: true,
Repository: appsmithorg/appsmith
File: app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (TypeScript)

Code before:
      startIcon={datasourceTableIcon}
      title={dbStructure.name}
    />
  );
});

type DatasourceStructureProps = Partial<DatasourceStructureItemProps> & {
  tables: Array<DatasourceTable>;
  step: number;
  datasourceId: string;
  context: DatasourceStructureContext;
  isDefaultOpen?: boolean;
  currentActionId: string;
  showTemplates: boolean;
};

const DatasourceStructure = (props: DatasourceStructureProps) => {
  return (
    <StructureWrapper>
      <StyledList className="t--schema-virtuoso-container">
        {props.tables.map((dbStructure) => (
          <DatasourceStructureItem
            {...omit(props, ["tables"])}
            dbStructure={dbStructure}
            key={`${props.datasourceId}${dbStructure.name}-${props.context}`}
            showTemplates={props.showTemplates}
          />
        ))}
      </StyledList>
    </StructureWrapper>
  );
};

export default DatasourceStructure;


Review comment (performance):
How would we handle the virtual list. This was added in for an important performance problem. We should still keep this in

Code after:
});

type DatasourceStructureProps = Partial<DatasourceStructureItemProps> & {
  tables: Array<DatasourceTable>;
  step: number;
  datasourceId: string;
  context: DatasourceStructureContext;
  isDefaultOpen?: boolean;
  currentActionId: string;
  showTemplates: boolean;
};

const DatasourceStructure = (props: DatasourceStructureProps) => {
  const Row = (index: number) => {
    const structure = props.tables[index];

    return (
      <DatasourceStructureItem
        {...omit(props, ["tables"])}
        dbStructure={structure}
        key={`${props.datasourceId}${structure.name}-${props.context}`}
        showTemplates={props.showTemplates}
      />
    );
  };

  return (
    <StructureWrapper>
      <Virtuoso
        className="t--schema-virtuoso-container"
        itemContent={Row}
        totalCount={props.tables.length}
      />
    </StructureWrapper>
  );
};

export default DatasourceStructure;
Repository: appsmithorg/appsmith
File: app/client/src/widgets/TableWidgetV2/component/header/actions/Download.tsx (TypeScript)

Code before:

            if (
              column.columnProperties?.columnType === "number" &&
              typeof value === "string"
            ) {
              tableRow.push(Number(value) || 0);
            } else {
              tableRow.push(value);
            }
          }
        });

        tableData.push(tableRow);
      }

      // Create workbook and worksheet using the dynamically imported XLSX
      const ws = XLSX.utils.aoa_to_sheet(tableData);
      const wb = XLSX.utils.book_new();

      XLSX.utils.book_append_sheet(wb, ws, "Sheet1");

      // Generate and download file
      XLSX.writeFile(wb, `${props.widgetName}.xlsx`);
    } catch (error) {
      // eslint-disable-next-line no-console
      console.error("Error loading Excel export functionality:", error);
    }
  };

  const downloadTableDataAsCsv = () => {
    selectMenu(true);
    const csvData = transformTableDataIntoCsv({
      columns: props.columns,
      data: props.data,
    });

    downloadDataAsCSV({
      csvData: csvData,
      delimiter: props.delimiter,
      fileName: `${props.widgetName}.csv`,
    });
    selectMenu(false);
  };

  const handleCloseMenu = () => {
    selectMenu(false);
  };

  if (props.columns.length === 0) {
    return (
      <TableIconWrapper disabled>

Review comment (bug):
Can we add a debugger logger instead to provide transparency about the error to the user ? @rahulbarwal 

Code after:
              typeof value === "string"
            ) {
              tableRow.push(Number(value) || 0);
            } else {
              tableRow.push(value);
            }
          }
        });

        tableData.push(tableRow);
      }

      // Create workbook and worksheet using the dynamically imported XLSX
      const ws = XLSX.utils.aoa_to_sheet(tableData);
      const wb = XLSX.utils.book_new();

      XLSX.utils.book_append_sheet(wb, ws, "Sheet1");

      // Generate and download file
      XLSX.writeFile(wb, `${props.widgetName}.xlsx`);
    } catch (error) {
      AppsmithConsole.error({
        text: `Error loading Excel export functionality: ${error}`,
        source: {
          id: props.widgetId,
          name: props.widgetName,
          type: ENTITY_TYPE.WIDGET,
        },
      });
    }
  };

  const downloadTableDataAsCsv = () => {
    selectMenu(true);
    const csvData = transformTableDataIntoCsv({
      columns: props.columns,
      data: props.data,
    });

    downloadDataAsCSV({
      csvData: csvData,
      delimiter: props.delimiter,
      fileName: `${props.widgetName}.csv`,
    });
    selectMenu(false);
  };

  const handleCloseMenu = () => {
    selectMenu(false);
  };
Repository: appsmithorg/appsmith
File: app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetTreeItem.tsx (TypeScript)

Code before:
      validateName,
    ],
  );

  const startIcon = useMemo(
    () => <WidgetTypeIcon type={widget?.type} />,
    [widget?.type],
  );

  const onClick = useCallback(
    (e: React.MouseEvent) => switchToWidget(e, widget),
    [switchToWidget, widget],
  );

  const onDoubleClick = useCallback(
    () => enterEditMode(item.id),
    [enterEditMode, item.id],
  );

  const rightControl = useMemo(
    () => (
      <WidgetContextMenu canManagePages={canManagePages} widgetId={item.id} />
    ),
    [canManagePages, item.id],
  );

  return (
    <EntityItem
      id={item.id}
      isSelected={item.isSelected}
      nameEditorConfig={nameEditorConfig}
      onClick={onClick}
      onDoubleClick={onDoubleClick}
      rightControl={rightControl}
      rightControlVisibility="hover"
      startIcon={startIcon}
      title={item.name}
    />
  );
};


Review comment (suggestion):
no need of this statement anymore?
`if (!widget) return null;`

Code after:
      validateName,
    ],
  );

  const startIcon = useMemo(
    () => <WidgetTypeIcon type={widget?.type} />,
    [widget?.type],
  );

  const onClick = useCallback(
    (e: React.MouseEvent) => switchToWidget(e, widget),
    [switchToWidget, widget],
  );

  const onDoubleClick = useCallback(
    () => enterEditMode(item.id),
    [enterEditMode, item.id],
  );

  const rightControl = useMemo(
    () => (
      <WidgetContextMenu canManagePages={canManagePages} widgetId={item.id} />
    ),
    [canManagePages, item.id],
  );

  if (!widget) return null;

  return (
    <EntityItem
      id={item.id}
      isSelected={item.isSelected}
      nameEditorConfig={nameEditorConfig}
      onClick={onClick}
      onDoubleClick={onDoubleClick}
      rightControl={rightControl}
      rightControlVisibility="hover"
      startIcon={startIcon}
      title={item.name}
    />
  );
};
Repository: appsmithorg/appsmith
File: app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java (Java)

Code before:
package com.appsmith.server.configurations;

import io.micrometer.core.instrument.Meter;
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.config.MeterFilter;

import java.util.List;

public class NoTagsMeterFilter implements MeterFilter {
    private static final List<String> seriesExceptionList = List.of(
            "appsmith.total.plugin.execution", "appsmith.total.server.execution", "appsmith.get.datasource.context");

    @Override
    public Meter.Id map(Meter.Id id) {
        // Remove all tags from the metric
        if (id.getName().startsWith("appsmith") && !startsWithPrefix(id.getName())) {
            return id.replaceTags(Tags.empty());
        }
        return id;
    }

    private boolean startsWithPrefix(String metricName) {
        for (String prefix : seriesExceptionList) {
            if (metricName.startsWith(prefix)) {
                return true;
            }
        }
        return false;
    }
}


Review comment (suggestion):
@ApekshaBhosale 

We can check for metric name from the keys of the `seriesExceptionMap` map to make the function generic

Code after:
package com.appsmith.server.configurations;

import io.micrometer.core.instrument.Meter;
import io.micrometer.core.instrument.Tag;
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.config.MeterFilter;

import java.util.List;
import java.util.Map;

public class NoTagsMeterFilter implements MeterFilter {
    private static final Map<String, List<String>> seriesExceptionMap = Map.of(
            "appsmith.total.plugin.execution", List.of("plugin"),
            "appsmith.total.server.execution", List.of("plugin"),
            "appsmith.get.datasource.context", List.of("plugin"));

    @Override
    public Meter.Id map(Meter.Id id) {
        // Remove all tags from the metric
        if (id.getName().startsWith("appsmith")) {
            List<String> allowedTags = seriesExceptionMap.get(id.getName());

            if (allowedTags != null) {
                Tags filteredTags = Tags.empty();
                for (Tag tag : id.getTags()) {
                    if (allowedTags.contains(tag.getKey())) {
                        filteredTags = filteredTags.and(tag);
                    }
                }
                return id.replaceTags(filteredTags);
            } else {
                // Remove all tags for other metrics
                return id.replaceTags(Tags.empty());
            }
        } else {
            return id;
        }
    }
}
Repository: appsmithorg/appsmith
File: app/client/src/sagas/FocusRetentionSaga.ts (TypeScript)

Code before:
      for (const storePath of toStore) {
        yield call(this.storeStateOfPath, storePath, previousPath);
      }
    }

    this.updateFocusStrategy(currentPath);
    /* RESTORE THE UI STATE OF THE NEW URL */
    yield call(this.focusStrategy.waitForPathLoad, currentPath, previousPath);
    const setPaths: Array<FocusPath> = yield call(
      this.focusStrategy.getEntitiesForSet,
      previousPath,
      currentPath,
      state,
    );

    for (const setPath of setPaths) {
      yield call(this.setStateOfPath, setPath.key, setPath.entityInfo);
    }
  }

  public *handleRemoveFocusHistory(url: string) {
    const removeKeys: string[] = [];
    const entityKey: string = yield call(this.focusStrategy.getUrlKey, url);
    const focusEntityInfo = identifyEntityFromPath(url);

    removeKeys.push(`${entityKey}`);

    const parentElement = FocusStoreHierarchy[focusEntityInfo.entity];

    if (parentElement) {
      const parentUrl: string = yield call(
        this.focusStrategy.getEntityParentUrl,
        focusEntityInfo,
        parentElement,
      );

      const parentEntityKey: string = yield call(
        this.focusStrategy.getUrlKey,
        parentUrl,
      );

      removeKeys.push(parentEntityKey);
    }

    for (const key of removeKeys) {
      yield put(removeFocusHistory(key));
    }
  }

  private updateFocusStrategy(currentPath: string) {
    const ideType = getIDETypeByUrl(currentPath);

Review comment (suggestion):
```suggestion
    removeKeys.push(entityKey);
```

Code after:
      for (const storePath of toStore) {
        yield call(this.storeStateOfPath, storePath, previousPath);
      }
    }

    this.updateFocusStrategy(currentPath);
    /* RESTORE THE UI STATE OF THE NEW URL */
    yield call(this.focusStrategy.waitForPathLoad, currentPath, previousPath);
    const setPaths: Array<FocusPath> = yield call(
      this.focusStrategy.getEntitiesForSet,
      previousPath,
      currentPath,
      state,
    );

    for (const setPath of setPaths) {
      yield call(this.setStateOfPath, setPath.key, setPath.entityInfo);
    }
  }

  public *handleRemoveFocusHistory(url: string) {
    const removeKeys: string[] = [];
    const entityKey: string = yield call(this.focusStrategy.getUrlKey, url);
    const focusEntityInfo = identifyEntityFromPath(url);

    removeKeys.push(entityKey);

    const parentElement = FocusStoreHierarchy[focusEntityInfo.entity];

    if (parentElement) {
      const parentUrl: string = yield call(
        this.focusStrategy.getEntityParentUrl,
        focusEntityInfo,
        parentElement,
      );

      const parentEntityKey: string = yield call(
        this.focusStrategy.getUrlKey,
        parentUrl,
      );

      removeKeys.push(parentEntityKey);
    }

    for (const key of removeKeys) {
      yield put(removeFocusHistory(key));
    }
  }

  private updateFocusStrategy(currentPath: string) {
    const ideType = getIDETypeByUrl(currentPath);
Repository: appsmithorg/appsmith
File: app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java (Java)

Code before:

        return redisOperations
                .execute(connection -> {
                    Flux<ByteBuffer> scanFlux = connection
                            .keyCommands()
                            .scan(ScanOptions.scanOptions()
                                    .match(SPRING_SESSION_PATTERN)
                                    .count(1000)
                                    .build());

                    return scanFlux.flatMap(scannedKey -> {
                                return connection.keyCommands().del(scannedKey).doOnSuccess(result -> {
                                    int count = deletedKeysCount.incrementAndGet();
                                    if (count % 10000 == 0) {
                                        log.info("Processed {} Redis keys", count);
                                    }
                                });
                            })
                            .then()
                            .doOnSuccess(v -> log.info("Total Redis keys processed: {}", deletedKeysCount.get()))
                            .doOnError(error -> log.error("Redis key deletion error: {}", error.getMessage()));
                })
                .then();
    }
}


Review comment (bug):
If an error occurs, some keys may remain in Redis. Does it make sense to implement a retry mechanism here? I get the sense that the cleanup is intended to be a binary operation, either fully succeeding or fully failing.

Code after:
     * This would be required for whenever any attribute related to sessions becomes invalid at a systemic level.
     * Use with caution, every user will be logged out.
     */
    public Mono<Void> deleteAllSessionsIncludingCurrentUser() {
        AtomicInteger deletedKeysCount = new AtomicInteger(0);

        return redisOperations
                .execute(connection -> {
                    Flux<ByteBuffer> scanFlux = connection
                            .keyCommands()
                            .scan(ScanOptions.scanOptions()
                                    .match(SPRING_SESSION_PATTERN)
                                    .count(1000)
                                    .build());

                    return scanFlux.flatMap(scannedKey -> {
                                return connection.keyCommands().del(scannedKey).doOnSuccess(result -> {
                                    int count = deletedKeysCount.incrementAndGet();
                                    if (count % 10000 == 0) {
                                        log.info("Processed {} Redis keys", count);
                                    }
                                });
                            })
                            .then()
                            .doOnSuccess(v -> log.info("Total Redis keys processed: {}", deletedKeysCount.get()))
                            .doOnError(error -> log.error("Redis key deletion error: {}", error.getMessage()));
                })
                .then();
    }
}
Repository: appsmithorg/appsmith
File: app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/useInfiniteScroll.tsx (TypeScript)

Code before:
import type { Row } from "react-table";
import { useEffect } from "react";

export interface UseInfiniteScrollProps {
  loadMore: () => void;
  rows: Row<Record<string, unknown>>[];
  pageSize: number;
}

export const useInfiniteScroll = ({
  loadMore,
  rows,
  pageSize,
}: UseInfiniteScrollProps) => {
  useEffect(() => {
    // If cachedRows is just a single page, call loadMore to fetch the next page
    if (rows.length > 0 && rows.length <= pageSize) {
      loadMore();
    }
  }, [rows.length, pageSize, loadMore]);

  return;
};


Review comment (suggestion):
I thought we took lot of effort with `endOfData` and `hasMoreData` variables?
Lets stick to one approach: either we use those variables or we calculate them separately.
cc: @AmanAgarwal041 

Code after:
import type { Row } from "react-table";
import { useEffect } from "react";

export interface UseInfiniteScrollProps {
  loadMore: () => void;
  rows: Row<Record<string, unknown>>[];
  pageSize: number;
}

export const useInfiniteScroll = ({
  loadMore,
  pageSize,
  rows,
}: UseInfiniteScrollProps) => {
  useEffect(() => {
    // If cachedRows is just a single page, call loadMore to fetch the next page
    if (rows.length > 0 && rows.length <= pageSize) {
      loadMore();
    }
  }, [rows.length, pageSize, loadMore]);

  return;
};