ronantakizawa/codereview-bench huggingface dataset:
Repository: apple/swift-nio
File: IntegrationTests/allocation-counter-tests-framework/run-allocation-counter.sh (Shell)

Code before:
working_dir=$(mktemp -d "$tmp_dir/.nio_alloc_counter_tests_XXXXXX")
echo "Working directory: $working_dir"

selected_hooked_functions="HookedFunctionsDoHook"
selected_bootstrap="bootstrapDoHook"

if ! $do_hooking; then
    selected_hooked_functions=HookedFunctionsDoNotHook
    selected_bootstrap=bootstrapDoNotHook
fi

build_package \
    "$working_dir" \
    "$here/template" \
    "$(realpath "$pkg_root")" \
    "$(find_swiftpm_package_name "$pkg_root")" \
    "$selected_hooked_functions" \
    "$selected_bootstrap" \
    "$extra_dependencies_file" \
    "${shared_files[@]+"${shared_files[@]}"}" \
    -- \
    "${modules[@]}" \
    -- \
    "${files[@]}"
(
set -eu
cd "$working_dir"
swift build "${build_opts[@]}"
bin_path=$(swift build "${build_opts[@]}" --show-bin-path)
if [[ "${NIO_ALLOC_COUNTER_TESTS_PARALLEL:-false}" == "true" ]]; then
    out_dir=$(mktemp -d)
    trap 'rm -rf "$out_dir"' EXIT
    pids=()
    for f in "${files[@]}"; do
        module=$(module_name_from_path "$f")
        "$bin_path/$module" > "$out_dir/$module" &
        pids+=($!)
    done
    all_ok=true
    for i in "${!pids[@]}"; do
        module=$(module_name_from_path "${files[$i]}")
        if wait "${pids[$i]}"; then
            echo "- ${files[$i]}"
            cat "$out_dir/$module"
        else
            all_ok=false
            echo "- ${files[$i]} FAILED" >&2
            cat "$out_dir/$module" >&2
        fi
    done
    "$all_ok"

Review comment (bug):
This used to exit if a `swift run` command failed.

Are we sure it does now since the `wait` is in an `if`

Code after:
working_dir=$(mktemp -d "$tmp_dir/.nio_alloc_counter_tests_XXXXXX")
echo "Working directory: $working_dir"

selected_hooked_functions="HookedFunctionsDoHook"
selected_bootstrap="bootstrapDoHook"

if ! $do_hooking; then
    selected_hooked_functions=HookedFunctionsDoNotHook
    selected_bootstrap=bootstrapDoNotHook
fi

build_package \
    "$working_dir" \
    "$here/template" \
    "$(realpath "$pkg_root")" \
    "$(find_swiftpm_package_name "$pkg_root")" \
    "$selected_hooked_functions" \
    "$selected_bootstrap" \
    "$extra_dependencies_file" \
    "${shared_files[@]+"${shared_files[@]}"}" \
    -- \
    "${modules[@]}" \
    -- \
    "${files[@]}"
(
set -eu
cd "$working_dir"
swift build "${build_opts[@]}"
bin_path=$(swift build "${build_opts[@]}" --show-bin-path)
if [[ "${NIO_ALLOC_COUNTER_TESTS_PARALLEL:-false}" == "true" ]]; then
    out_dir=$(mktemp -d)
    trap 'rm -rf "$out_dir"' EXIT
    for f in "${files[@]}"; do
        module=$(module_name_from_path "$f")
        (
            rc=0
            "$bin_path/$module" > "$out_dir/$module" || rc=$?
            echo $rc > "$out_dir/$module.exit"
        ) &
    done
    wait
    all_ok=true
    for f in "${files[@]}"; do
        module=$(module_name_from_path "$f")
        echo "- $f"
        if [[ "$(cat "$out_dir/$module.exit")" == "0" ]]; then
            cat "$out_dir/$module"
        else
            all_ok=false
            echo "FAILED: $f" >&2
            cat "$out_dir/$module"
Repository: appsmithorg/appsmith
File: app/client/src/sagas/GitSyncSagas.ts (TypeScript)

Code before:
}

function* fetchGitStatusSaga() {
  let response: ApiResponse | undefined;
  try {
    const applicationId: string = yield select(getCurrentApplicationId);
    const gitMetaData: GitApplicationMetadata = yield select(
      getCurrentAppGitMetaData,
    );
    response = yield GitSyncAPI.getGitStatus({
      applicationId,
      branch: gitMetaData?.branchName || "",
    });
    const isValidResponse: boolean = yield validateResponse(
      response,
      false,
      getLogToSentryFromResponse(response),
    );
    if (isValidResponse) {
      yield put(fetchGitStatusSuccess(response?.data));
    }
  } catch (error) {
    yield put({
      type: ReduxActionErrorTypes.FETCH_GIT_STATUS_ERROR,
      payload: { error, show: false },
    });
    // non api error
    if (!response || response?.responseMeta?.success) {
      throw error;
    }
  }
}

function* mergeBranchSaga(
  action: ReduxActionWithCallbacks<MergeBranchPayload, void, void>,
) {
  let response: ApiResponse | undefined;
  try {
    const applicationId: string = yield select(getCurrentApplicationId);

    const { destinationBranch, sourceBranch } = action.payload;

    response = yield GitSyncAPI.merge({
      applicationId,
      sourceBranch,
      destinationBranch,
    });

    const isValidResponse: boolean = yield validateResponse(
      response,
      false,

Review comment (bug):
Can we use `createMessage(ERROR_GIT_AUTH_FAIL)`  instead? The `createMessage` function has been created to deal with i18n in the future. This way, the same message can be shown in different languages based on the user setting.

Code after:
}

function* fetchGitStatusSaga() {
  let response: ApiResponse | undefined;
  try {
    const applicationId: string = yield select(getCurrentApplicationId);
    const gitMetaData: GitApplicationMetadata = yield select(
      getCurrentAppGitMetaData,
    );
    response = yield GitSyncAPI.getGitStatus({
      applicationId,
      branch: gitMetaData?.branchName || "",
    });
    const isValidResponse: boolean = yield validateResponse(
      response,
      false,
      getLogToSentryFromResponse(response),
    );
    if (isValidResponse) {
      yield put(fetchGitStatusSuccess(response?.data));
    }
  } catch (error) {
    const payload = { error, show: true };
    if (error?.message?.includes("Auth fail")) {
      payload.error = new Error(createMessage(ERROR_GIT_AUTH_FAIL));
    } else if (error?.message?.includes("Invalid remote: origin")) {
      payload.error = new Error(createMessage(ERROR_GIT_INVALID_REMOTE));
    }

    yield put({
      type: ReduxActionErrorTypes.FETCH_GIT_STATUS_ERROR,
      payload,
    });

    // non api error
    if (!response || response?.responseMeta?.success) {
      throw error;
    }
  }
}

function* mergeBranchSaga(
  action: ReduxActionWithCallbacks<MergeBranchPayload, void, void>,
) {
  let response: ApiResponse | undefined;
  try {
    const applicationId: string = yield select(getCurrentApplicationId);

    const { destinationBranch, sourceBranch } = action.payload;

    response = yield GitSyncAPI.merge({
Repository: appsmithorg/appsmith
File: app/client/src/sagas/WidgetDeletionSaga.test.ts (TypeScript)

Code before:
  deleteSaga,
  getUpdatedDslAfterDeletingWidget,
  postDelete,
} from "./WidgetDeletionSagas";
import { SelectionRequestType } from "./WidgetSelectUtils";

type GeneratorType = Generator<
  SelectEffect | CallEffect | PutEffect,
  boolean | undefined,
  unknown
>;

describe("WidgetDeletionSaga", () => {
  it("should delete a widget and remove its meta widgets", () => {
    let result;
    const generator: GeneratorType = deleteSaga({
      type: ReduxActionTypes.WIDGET_DELETE,
      payload: {
        widgetId: undefined,
        parentId: undefined,
        disallowUndo: false,
      },
    });

    // Step 1: Check if widgetId is not provided, select the currently selected widget
    result = generator.next();
    expect(result.value).toEqual(select(getSelectedWidget));

    // Step 2: Mock the return value of getSelectedWidget to Widget
    result = generator.next(selectedWidget);
    expect(result.value).toEqual(select(getWidget, selectedWidget.widgetId));

    // Step 3: Mock the return value of getWidget to stateWidget
    result = generator.next(stateWidget);
    expect(result.value).toEqual(
      call(
        getUpdatedDslAfterDeletingWidget,
        selectedWidget.widgetId,
        selectedWidget.parentId,
      ),
    );

    // Step 4: Mock the return value of getUpdatedDslAfterDeletingWidget to updatedDSL
    result = generator.next(updatedDSL);

    // Step 5: Check if the widget has meta widgets, true dispatch DELETE_META_WIDGETS action
    if (stateWidget.hasMetaWidgets) {
      expect(result.value).toEqual({
        "@@redux-saga/IO": true,
        combinator: false,
        type: "PUT",

Review comment (refactor):
@jacquesikot It's great that you add the test in such fine detail but testing a saga in this way make it very brittle to change so in practise its always best to run the saga completely and then assert. Otherwise any small change in the test like moving one yield above another will break the test but in actuality it's just some refactoring and no logical change.

For more context -> https://redux-saga.js.org/docs/advanced/Testing/#testing-the-full-saga

Search for `runSaga` in our codebase and you'll find a lot of examples. You can search in the ee code as well; there you may find much more examples than ce

Code after:

  const deleteAction = {
    type: "DELETE_WIDGET",
    payload: {
      widgetId: undefined,
      parentId: undefined,
      disallowUndo: false,
      isShortcut: false,
    },
  };

  await runSaga(
    {
      dispatch: (action) => dispatched.push(action),
      getState: () => mockStore.getState(),
    },
    deleteSaga,
    deleteAction,
  ).toPromise();

  expect(dispatched).toContainEqual({
    type: ReduxActionTypes.DELETE_META_WIDGETS,
    payload: {
      creatorIds: [listWidget.widgetId],
    },
  });
});
Repository: appsmithorg/appsmith
File: app/client/src/widgets/wds/WDSEmailInputWidget/widget/index.tsx (TypeScript)

Code before:
import { WDSInputWidget } from "widgets/wds/WDSInputWidget";
import { InputIcon, InputThumbnail } from "appsmith-icons";
import { INPUT_TYPES } from "widgets/wds/WDSInputWidget/constants";
import type { WidgetBaseConfiguration } from "WidgetProvider/constants";

class WDSEmailInputWidget extends WDSInputWidget {
  static type = "WDS_EMAIL_INPUT_WIDGET";

  static getConfig(): WidgetBaseConfiguration {
    return {
      ...super.getConfig(),
      displayOrder: undefined,
      name: "Email Input",
    };
  }

  static getDefaults() {
    return {
      ...super.getDefaults(),
      inputType: INPUT_TYPES.EMAIL,
      widgetName: "EmailInput",
    };
  }

  static getMethods() {
    return {
      ...super.getMethods(),
      IconCmp: InputIcon,
      ThumbnailCmp: InputThumbnail,
    };
  }
}

export { WDSEmailInputWidget };


Review comment (suggestion):
displayOrder: undefined is required here as we are inheriting the widgets from input widget. Otherwise, they will all have displayOrder as 1.

Code after:
import { WDSInputWidget } from "widgets/wds/WDSInputWidget";
import { EmailInputIcon, EmailInputThumbnail } from "appsmith-icons";
import { WIDGET_TAGS } from "constants/WidgetConstants";
import { INPUT_TYPES } from "widgets/wds/WDSInputWidget/constants";
import type { WidgetBaseConfiguration } from "WidgetProvider/constants";

class WDSEmailInputWidget extends WDSInputWidget {
  static type = "WDS_EMAIL_INPUT_WIDGET";

  static getConfig(): WidgetBaseConfiguration {
    return {
      ...super.getConfig(),
      displayOrder: undefined,
      tags: [WIDGET_TAGS.INPUTS],
      name: "Email Input",
    };
  }

  static getDefaults() {
    return {
      ...super.getDefaults(),
      inputType: INPUT_TYPES.EMAIL,
      widgetName: "EmailInput",
    };
  }

  static getMethods() {
    return {
      ...super.getMethods(),
      IconCmp: EmailInputIcon,
      ThumbnailCmp: EmailInputThumbnail,
    };
  }
}

export { WDSEmailInputWidget };
Repository: appsmithorg/appsmith
File: app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/index.tsx (TypeScript)

Code before:
            id: Math.random().toString(),
            content: this.props.queryData.choices[0].message.content,
            role: "assistant",
          },
        ],
        isWaitingForResponse: false,
      }));
    }
  };

  handlePromptChange = (prompt: string) => {
    this.setState({ prompt });
  };

  getWidgetView(): ReactNode {
    return (
      <AIChat
        assistantName={this.props.assistantName}
        chatTitle={this.props.chatTitle}
        description={this.props.description}
        isWaitingForResponse={this.state.isWaitingForResponse}
        onPromptChange={this.handlePromptChange}
        onSubmit={this.handleMessageSubmit}
        prompt={this.state.prompt}
        promptInputPlaceholder={this.props.promptInputPlaceholder}
        thread={this.adaptMessages(this.state.messages)}
        username={this.props.username}
      />
    );
  }
}

export default WDSAIChatWidget;


Review comment (bug):
I think it's better to do the check directly in the component, since the consumer does not have to validate edge cases. 

Code after:
            id: Math.random().toString(),
            content: this.props.queryData.choices[0].message.content,
            role: "assistant",
          },
        ],
        isWaitingForResponse: false,
      }));
    }
  };

  handlePromptChange = (prompt: string) => {
    this.setState({ prompt });
  };

  getWidgetView(): ReactNode {
    return (
      <AIChat
        assistantName={this.props.assistantName}
        chatTitle={this.props.chatTitle}
        isWaitingForResponse={this.state.isWaitingForResponse}
        onPromptChange={this.handlePromptChange}
        onSubmit={this.handleMessageSubmit}
        prompt={this.state.prompt}
        promptInputPlaceholder={this.props.promptInputPlaceholder}
        thread={this.adaptMessages(this.state.messages)}
        username={this.props.username || ""}
      />
    );
  }
}

export default WDSAIChatWidget;
Repository: appsmithorg/appsmith
File: app/client/src/workers/Evaluation/evaluate.ts (TypeScript)

Code before:
  isDataField: boolean;
  isTriggerBased: boolean;
}) {
  self["$isDataField"] = isDataField;

  const evalContext = createEvaluationContext({
    dataTree,
    configTree,
    context,
    evalArguments,
    isTriggerBased,
  });

  Object.assign(self, evalContext);
}

export function evaluateSync(
  userScript: string,
  dataTree: DataTree,
  isJSCollection: boolean,
  context?: EvaluateContext,
  // TODO: Fix this the next time the file is edited
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  evalArguments?: Array<any>,
  configTree: ConfigTree = {},
  scopeCache?: EvalContext,
): EvalResult {
  return (function () {
    const errors: EvaluationError[] = [];
    let result;

    const { script } = getUserScriptToEvaluate(
      userScript,
      false,
      evalArguments,
    );

    // If nothing is present to evaluate, return instead of evaluating
    if (!script.length) {
      return {
        errors: [],
        result: undefined,
        triggers: [],
      };
    }

    self["$isDataField"] = true;
    const EVAL_CONTEXT: EvalContext = {};

    ///// Adding callback data
    EVAL_CONTEXT.ARGUMENTS = evalArguments;

Review comment (performance):
Can we name the variable to be able to explain it's usecase. 
```suggestion
  evalContextCache?: EvalContext,
```

Code after:
  isDataField: boolean;
  isTriggerBased: boolean;
}) {
  self["$isDataField"] = isDataField;

  const evalContext = createEvaluationContext({
    dataTree,
    configTree,
    context,
    evalArguments,
    isTriggerBased,
  });

  Object.assign(self, evalContext);
}

export function evaluateSync(
  userScript: string,
  dataTree: DataTree,
  isJSCollection: boolean,
  context?: EvaluateContext,
  // TODO: Fix this the next time the file is edited
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  evalArguments?: Array<any>,
  configTree: ConfigTree = {},
  evalContextCache?: EvalContext,
): EvalResult {
  return (function () {
    const errors: EvaluationError[] = [];
    let result;

    const { script } = getUserScriptToEvaluate(
      userScript,
      false,
      evalArguments,
    );

    // If nothing is present to evaluate, return instead of evaluating
    if (!script.length) {
      return {
        errors: [],
        result: undefined,
        triggers: [],
      };
    }

    self["$isDataField"] = true;
    const EVAL_CONTEXT: EvalContext = {};

    ///// Adding callback data
    EVAL_CONTEXT.ARGUMENTS = evalArguments;
Repository: appsmithorg/appsmith
File: app/client/packages/design-system/ads/src/Templates/EntityExplorer/EditorSegments/EditorSegments.tsx (TypeScript)

Code before:
import React from "react";
import styled from "styled-components";
import {
  SegmentedControl,
  type SegmentedControlOption,
} from "../../../SegmentedControl";
import { Flex } from "../../../Flex";

interface Props {
  selectedSegment: string;
  onSegmentChange: (value: string) => void;
  options: SegmentedControlOption[];
  children?: React.ReactNode | React.ReactNode[];
}

const Container = styled(Flex)`
  #editor-pane-segment-control {
    max-width: 247px;
  }
`;

export const EditorSegments = (props: Props) => {
  const { children, onSegmentChange, options, selectedSegment } = props;

  return (
    <Container
      alignItems="center"
      backgroundColor="var(--ads-v2-colors-control-track-default-bg)"
      className="ide-editor-left-pane__header"
      gap="spaces-2"
      justifyContent="space-between"
      padding="spaces-2"
    >
      <SegmentedControl
        id="editor-pane-segment-control"
        onChange={onSegmentChange}
        options={options}
        value={selectedSegment}
      />
      {children}
    </Container>
  );

Review comment (suggestion):
This styling pattern should be avoided whenever possible, but in case you cannot get to that component to style it directly, you should consider using a class name instead of an id (just in case this component is used more that once on the same page). 

Code after:
import React from "react";
import { SegmentedControl } from "../../../SegmentedControl";
import { Container } from "./EditorSegments.styles";
import type { EditorSegmentsProps } from "./EditorSegments.types";

const EditorSegments = (props: EditorSegmentsProps) => {
  const { children, onSegmentChange, options, selectedSegment } = props;

  return (
    <Container
      alignItems="center"
      backgroundColor="var(--ads-v2-colors-control-track-default-bg)"
      className="ide-editor-left-pane__header"
      gap="spaces-2"
      justifyContent="space-between"
      padding="spaces-2"
    >
      <SegmentedControl
        className="editor-pane-segment-control"
        onChange={onSegmentChange}
        options={options}
        value={selectedSegment}
      />
      {children}
    </Container>
  );
};

EditorSegments.displayName = "EditorSegments";

EditorSegments.defaultProps = {};

export { EditorSegments };
Repository: appsmithorg/appsmith
File: app/client/packages/design-system/ads/src/Templates/EntityExplorer/EditorSegments/EditorSegments.stories.tsx (TypeScript)

Code before:
import React from "react";
import { EditorSegments } from "./EditorSegments";
import type { EditorSegmentsProps } from "./EditorSegments.types";
import type { StoryObj } from "@storybook/react";
import { Button } from "../../../Button";

export default {
  title: "ADS/Templates/Editor Segments",
  component: EditorSegments,
};

// eslint-disable-next-line react/function-component-definition
const Template = (args: EditorSegmentsProps) => {
  return <EditorSegments {...args}>{args.children}</EditorSegments>;
};

export const EditorSegmentsStory = Template.bind({}) as StoryObj;
EditorSegmentsStory.storyName = "Default";
EditorSegmentsStory.args = {
  options: [
    {
      value: "queries",
      label: "Queries",
      startIcon: "queries-v3",
    },
    {
      value: "js",
      label: "JS",
      startIcon: "content-type-json",
    },
    {
      value: "ui",
      label: "UI",

Review comment (suggestion):
Please use `ADS/Templates/EntityExplorer/EditorSegments` to group these basic components under EntityExplorer branch in Storybook (this will also be consistent with the folder path).

Code after:
import React from "react";
import { EditorSegments } from "./EditorSegments";
import type { EditorSegmentsProps } from "./EditorSegments.types";
import type { StoryObj } from "@storybook/react";
import { Button } from "../../../Button";

export default {
  title: "ADS/Templates/Entity Explorer/Editor Segments",
  component: EditorSegments,
};

// eslint-disable-next-line react/function-component-definition
const Template = (args: EditorSegmentsProps) => {
  return <EditorSegments {...args}>{args.children}</EditorSegments>;
};

export const EditorSegmentsStory = Template.bind({}) as StoryObj;
EditorSegmentsStory.storyName = "Default";
EditorSegmentsStory.args = {
  options: [
    {
      value: "queries",
      label: "Queries",
      startIcon: "queries-v3",
    },
    {
      value: "js",
      label: "JS",
      startIcon: "content-type-json",
    },
    {
      value: "ui",
      label: "UI",
Repository: appsmithorg/appsmith
File: app/client/packages/design-system/ads/src/Templates/EntityExplorer/SearchAndAdd/SearchAndAdd.stories.tsx (TypeScript)

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

import { SearchAndAdd, type SearchAndAddProps } from ".";

const meta: Meta<typeof SearchAndAdd> = {
  title: "ADS/Templates/EntityExplorer/SearchAndAdd",
  component: SearchAndAdd,
};

export default meta;

const Template = (props: SearchAndAddProps) => {
  const { onAdd, onSearch, placeholder, showAddButton = true } = props;

  return (
    <div style={{ width: 250 }}>
      <SearchAndAdd
        {...{
          onAdd,
          onSearch,
          showAddButton,
          placeholder,
        }}
      />
    </div>
  );
};

export const Basic = Template.bind({}) as StoryObj;

Basic.args = {
  onAdd: () => console.log("Add clicked"),
  onSearch: (searchTerm: string) => console.log(searchTerm),
  placeholder: "Search",

Review comment (suggestion):
We don't need this story as such, as we plan to add story for the Entity explorer as a whole, showing different variations for the same.

Refer `IDEHeader` template on [design-system.appsmith.com](https://design-system.appsmith.com) for context.

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

import { SearchAndAdd, type SearchAndAddProps } from ".";

const meta: Meta<typeof SearchAndAdd> = {
  title: "ADS/Templates/Entity Explorer/Search And Add",
  component: SearchAndAdd,
};

export default meta;

const Template = (props: SearchAndAddProps) => {
  const { onAdd, onSearch, placeholder, showAddButton = true } = props;

  return (
    <div style={{ width: 250 }}>
      <SearchAndAdd
        {...{
          onAdd,
          onSearch,
          showAddButton,
          placeholder,
        }}
      />
    </div>
  );
};

export const Basic = Template.bind({}) as StoryObj;

Basic.args = {
  onAdd: () => console.log("Add clicked"),
  onSearch: (searchTerm: string) => console.log(searchTerm),
  placeholder: "Search",