ronantakizawa/codereview-bench huggingface dataset:
Repository: mui/material-ui
File: packages/mui-material/src/ButtonBase/ButtonBase.js (JavaScript)

Code before:
  verticalAlign: 'middle',
  MozAppearance: 'none', // Reset
  WebkitAppearance: 'none', // Reset
  textDecoration: 'none',
  // So we take precedent over the style of a native <a /> element.
  color: 'inherit',
  '&::-moz-focus-inner': {
    borderStyle: 'none', // Remove Firefox dotted outline.
  },
  [`&.${buttonBaseClasses.disabled}`]: {
    pointerEvents: 'none', // Disable link interactions
    cursor: 'default',
  },
  '@media print': {
    colorAdjust: 'exact',
  },
});

function useRippleHandler(ripple, rippleAction, eventCallback, skipRippleAction = false) {
  return useEventCallback((event) => {
    if (eventCallback) {
      eventCallback(event);
    }

    const ignore = skipRippleAction;
    if (!ignore) {
      ripple[rippleAction](event);
    }

    return true;
  });
}

/**
 * `ButtonBase` contains as few styles as possible.
 * It aims to be a simple building block for creating a button.
 * It contains a load of style reset and some focus/ripple logic.
 */
const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
  const props = useDefaultProps({ props: inProps, name: 'MuiButtonBase' });
  const {
    action,
    centerRipple = false,
    children,
    className,
    component = 'button',
    disabled = false,
    disableRipple = false,
    disableTouchRipple = false,
    focusRipple = false,
    focusVisibleClassName,

Review comment (suggestion):
While you're touching this, you could also remove that variable, the first name is already semantically correct.

Code after:
  verticalAlign: 'middle',
  MozAppearance: 'none', // Reset
  WebkitAppearance: 'none', // Reset
  textDecoration: 'none',
  // So we take precedent over the style of a native <a /> element.
  color: 'inherit',
  '&::-moz-focus-inner': {
    borderStyle: 'none', // Remove Firefox dotted outline.
  },
  [`&.${buttonBaseClasses.disabled}`]: {
    pointerEvents: 'none', // Disable link interactions
    cursor: 'default',
  },
  '@media print': {
    colorAdjust: 'exact',
  },
});

/**
 * `ButtonBase` contains as few styles as possible.
 * It aims to be a simple building block for creating a button.
 * It contains a load of style reset and some focus/ripple logic.
 */
const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
  const props = useDefaultProps({ props: inProps, name: 'MuiButtonBase' });
  const {
    action,
    centerRipple = false,
    children,
    className,
    component = 'button',
    disabled = false,
    disableRipple = false,
    disableTouchRipple = false,
    focusRipple = false,
    focusVisibleClassName,
    LinkComponent = 'a',
    onBlur,
    onClick,
    onContextMenu,
    onDragLeave,
    onFocus,
    onFocusVisible,
    onKeyDown,
    onKeyUp,
    onMouseDown,
    onMouseLeave,
    onMouseUp,
    onTouchEnd,
    onTouchMove,
    onTouchStart,
Repository: mui/material-ui
File: scripts/utils.mjs (JavaScript)

Code before:
import path from 'path';
import url from 'url';
import fs from 'fs';

function findUpFile(fileName, cwd = process.cwd(), maxIterations = 5) {
  const pathName = path.join(cwd, fileName);
  if (fs.existsSync(pathName)) {
    return pathName;
  }
  if (maxIterations === 0) {
    return null;
  }
  return findUpFile(fileName, path.dirname(cwd), maxIterations - 1);
}

/**
 * Returns the full path of the root directory of the monorepo.
 */
// eslint-disable-next-line import/prefer-default-export
export function getWorkspaceRoot() {
  // Short circuit when available
  if (process.env.NX_WORKSPACE_ROOT) {
    return process.env.NX_WORKSPACE_ROOT;
  }

  const workspaceFilePath = findUpFile('pnpm-workspace.yaml', process.cwd());
  if (workspaceFilePath) {
    return path.dirname(workspaceFilePath);
  }


Review comment (suggestion):
Previously I was using `find-up` package but later realised that this won't work with the `useReactVersion` script if we were to import `getWorkspaceRoot` in that file since it runs before `pnpm install`.

Code after:
import path from 'path';
import url from 'url';
import fs from 'fs';

function findUpFile(fileName, cwd = process.cwd(), maxIterations = 5) {
  const pathName = path.join(cwd, fileName);
  if (fs.existsSync(pathName)) {
    return pathName;
  }
  if (maxIterations === 0) {
    return null;
  }
  return findUpFile(fileName, path.dirname(cwd), maxIterations - 1);
}

/**
 * Returns the full path of the root directory of the monorepo.
 */
// eslint-disable-next-line import/prefer-default-export
export function getWorkspaceRoot() {
  // Use this when available. Avoids the need to check for the workspace file.
  if (process.env.NX_WORKSPACE_ROOT) {
    return process.env.NX_WORKSPACE_ROOT;
  }

  const workspaceFilePath = findUpFile('pnpm-workspace.yaml', process.cwd());
  if (workspaceFilePath) {
    return path.dirname(workspaceFilePath);
  }
Repository: mui/material-ui
File: packages/mui-material/src/internal/SwitchBase.js (JavaScript)

Code before:
    ref,
    elementType: SwitchBaseRoot,
    className: classes.root,
    shouldForwardComponentProp: true,
    externalForwardedProps: {
      ...externalForwardedProps,
      component: 'span',
      ...other,
    },
    getSlotProps: (handlers) => ({
      ...handlers,
      onFocus: (event) => {
        handlers.onFocus?.(event);
        handleFocus(event);
      },
      onBlur: (event) => {
        handlers.onBlur?.(event);
        handleBlur(event);
      },
    }),
    ownerState,
    additionalProps: {
      centerRipple: true,
      focusRipple: !disableFocusRipple,
      disabled,
      role: 'switch',
      tabIndex: null,
    },
  });

  const [InputSlot, inputSlotProps] = useSlot('input', {
    ref: inputRef,
    elementType: SwitchBaseInput,
    className: classes.input,
    externalForwardedProps,
    getSlotProps: (handlers) => ({
      ...handlers,
      onChange: (event) => {
        handlers.onChange?.(event);
        handleInputChange(event);
      },
    }),
    ownerState,
    additionalProps: {
      autoFocus,
      checked: checkedProp,
      defaultChecked,
      disabled,
      id: hasLabelFor ? id : undefined,
      name,
      readOnly,

Review comment (suggestion):
I think the explicit `role: undefined` exist for specific fix.

The proper fix is to add `role: 'switch'` to `Switch` component. @sai6855 Would you mind checking on this?

This PR requires a test to ensure that the issue is fixed without introducing any regression.

Code after:
    ref,
    elementType: SwitchBaseRoot,
    className: classes.root,
    shouldForwardComponentProp: true,
    externalForwardedProps: {
      ...externalForwardedProps,
      component: 'span',
      ...other,
    },
    getSlotProps: (handlers) => ({
      ...handlers,
      onFocus: (event) => {
        handlers.onFocus?.(event);
        handleFocus(event);
      },
      onBlur: (event) => {
        handlers.onBlur?.(event);
        handleBlur(event);
      },
    }),
    ownerState,
    additionalProps: {
      centerRipple: true,
      focusRipple: !disableFocusRipple,
      role: undefined,
      tabIndex: null,
    },
  });

  const [InputSlot, inputSlotProps] = useSlot('input', {
    ref: inputRef,
    elementType: SwitchBaseInput,
    className: classes.input,
    externalForwardedProps,
    getSlotProps: (handlers) => ({
      ...handlers,
      onChange: (event) => {
        handlers.onChange?.(event);
        handleInputChange(event);
      },
    }),
    ownerState,
    additionalProps: {
      autoFocus,
      checked: checkedProp,
      defaultChecked,
      disabled,
      id: hasLabelFor ? id : undefined,
      name,
      readOnly,
      required,
Repository: mui/material-ui
File: packages/mui-joy/src/Autocomplete/AutocompleteProps.ts (TypeScript)

Code before:
import * as React from 'react';
import {
  AutocompleteChangeDetails,
  AutocompleteChangeReason,
  AutocompleteCloseReason,
  AutocompleteInputChangeReason,
  AutocompleteValue,
  AutocompleteValueOrFreeSoloValueMapping,
  UseAutocompleteProps,
} from '@mui/base/useAutocomplete';
import { PopperOwnProps } from '@mui/base/Popper';
import { OverridableStringUnion } from '@mui/types';
import { ColorPaletteProp, SxProps, VariantProp, ApplyColorInversion } from '../styles/types';
import { CreateSlotsAndSlotProps, SlotProps } from '../utils/types';

export type AutocompleteSlot =
  | 'root'
  | 'wrapper'
  | 'input'
  | 'startDecorator'
  | 'endDecorator'
  | 'clearIndicator'
  | 'popupIndicator'
  | 'listbox'
  | 'option'
  | 'loading'
  | 'noOptions'
  | 'limitTag';

export interface AutocompleteSlots {
  /**
   * The component that renders the root.
   * @default 'div'

Review comment (suggestion):
This import references `AutocompleteValueOrFreeSoloValueMapping` from the external `@mui/base` package. Please verify that this type has been added to `@mui/base` (version 7.0.0-beta.4 or the version being used) before merging this PR. If this type doesn't exist in the current version of `@mui/base`, this will cause a TypeScript compilation error.

Code after:
import * as React from 'react';
import {
  AutocompleteChangeDetails,
  AutocompleteChangeReason,
  AutocompleteCloseReason,
  AutocompleteFreeSoloValueMapping,
  AutocompleteInputChangeReason,
  AutocompleteValue,
  UseAutocompleteProps,
} from '@mui/base/useAutocomplete';
import { PopperOwnProps } from '@mui/base/Popper';
import { OverridableStringUnion } from '@mui/types';
import { ColorPaletteProp, SxProps, VariantProp, ApplyColorInversion } from '../styles/types';
import { CreateSlotsAndSlotProps, SlotProps } from '../utils/types';

export type AutocompleteSlot =
  | 'root'
  | 'wrapper'
  | 'input'
  | 'startDecorator'
  | 'endDecorator'
  | 'clearIndicator'
  | 'popupIndicator'
  | 'listbox'
  | 'option'
  | 'loading'
  | 'noOptions'
  | 'limitTag';

export interface AutocompleteSlots {
  /**
   * The component that renders the root.
   * @default 'div'
Repository: mui/material-ui
File: packages/mui-joy/src/Autocomplete/AutocompleteProps.ts (TypeScript)

Code before:
export interface AutocompleteProps<
  T,
  Multiple extends boolean | undefined,
  DisableClearable extends boolean | undefined,
  FreeSolo extends boolean | undefined,
>
  extends
    AutocompleteOwnProps<T, Multiple, DisableClearable, FreeSolo>,
    Omit<React.HTMLAttributes<HTMLDivElement>, 'defaultValue' | 'onChange' | 'children' | 'color'> {
  /**
   * Type of the `input` element. It should be [a valid HTML5 input type](https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input#input_types).
   */
  type?: string;
  onKeyDown?: (
    event: React.KeyboardEvent<HTMLDivElement> & { defaultMuiPrevented?: boolean },
  ) => void;
}

export interface AutocompleteOwnerState<
  T,
  Multiple extends boolean | undefined,
  DisableClearable extends boolean | undefined,
  FreeSolo extends boolean | undefined,
> extends ApplyColorInversion<AutocompleteOwnProps<T, Multiple, DisableClearable, FreeSolo>> {
  focused?: boolean;
  getOptionLabel: (option: AutocompleteValueOrFreeSoloValueMapping<T, FreeSolo>) => string;
  hasClearIcon?: boolean;
  hasPopupIcon?: boolean;
  hasOptions?: boolean;
  inputFocused?: boolean;
  popupOpen?: boolean;
}


Review comment (suggestion):
Let's not update the types in Joy UI. It is no longer maintained and we won't release a new version for it. 

Code after:
export interface AutocompleteProps<
  T,
  Multiple extends boolean | undefined,
  DisableClearable extends boolean | undefined,
  FreeSolo extends boolean | undefined,
>
  extends
    AutocompleteOwnProps<T, Multiple, DisableClearable, FreeSolo>,
    Omit<React.HTMLAttributes<HTMLDivElement>, 'defaultValue' | 'onChange' | 'children' | 'color'> {
  /**
   * Type of the `input` element. It should be [a valid HTML5 input type](https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input#input_types).
   */
  type?: string;
  onKeyDown?: (
    event: React.KeyboardEvent<HTMLDivElement> & { defaultMuiPrevented?: boolean },
  ) => void;
}

export interface AutocompleteOwnerState<
  T,
  Multiple extends boolean | undefined,
  DisableClearable extends boolean | undefined,
  FreeSolo extends boolean | undefined,
> extends ApplyColorInversion<AutocompleteOwnProps<T, Multiple, DisableClearable, FreeSolo>> {
  focused?: boolean;
  getOptionLabel: (option: T | AutocompleteFreeSoloValueMapping<FreeSolo>) => string;
  hasClearIcon?: boolean;
  hasPopupIcon?: boolean;
  hasOptions?: boolean;
  inputFocused?: boolean;
  popupOpen?: boolean;
}
Repository: n8n-io/n8n
File: packages/cli/src/modules/workflow-index/workflow-index.service.ts (TypeScript)

Code before:
			dependencyKey: webhookPath,
			dependencyInfo: { id: node.id, version: node.typeVersion },
		});
	}

	private getCalledWorkflowIdFrom(node: INode): string | undefined {
		if (node.parameters?.['source'] === 'parameter') {
			return undefined; // The sub-workflow is provided directly in the parameters, so no dependency to track.
		}
		if (node.parameters?.['source'] === 'localFile') {
			return undefined; // The sub-workflow is provided via a local file, so no dependency to track.
		}
		if (node.parameters?.['source'] === 'url') {
			return undefined; // The sub-workflow is provided via a URL, so no dependency to track.
		}
		// If it's none of those sources, it must be 'workflowId'. This might be either directly as a string, or an object.
		if (typeof node.parameters?.['workflowId'] === 'string') {
			return node.parameters?.['workflowId'];
		}
		if (
			node.parameters &&
			typeof node.parameters['workflowId'] === 'object' &&
			node.parameters['workflowId'] !== null &&
			'value' in node.parameters['workflowId'] &&
			typeof node.parameters['workflowId']['value'] === 'string'
		) {
			return node.parameters['workflowId']['value'];
		}
		this.errorReporter.warn(
			`While indexing, could not determine called workflow ID from executeWorkflow node ${node.id}`,
		);
		return undefined;
	}
}


Review comment (security):
Could we include the node parameters here as extra? It shouldn't contain anything sensitive 🤔 

```ts
.warn(`...`, { extra: node.parameters })
```

Code after:

	private getCalledWorkflowIdFrom(node: INode): string | undefined {
		if (node.parameters?.['source'] === 'parameter') {
			return undefined; // The sub-workflow is provided directly in the parameters, so no dependency to track.
		}
		if (node.parameters?.['source'] === 'localFile') {
			return undefined; // The sub-workflow is provided via a local file, so no dependency to track.
		}
		if (node.parameters?.['source'] === 'url') {
			return undefined; // The sub-workflow is provided via a URL, so no dependency to track.
		}
		// If it's none of those sources, it must be 'workflowId'. This might be either directly as a string, or an object.
		if (typeof node.parameters?.['workflowId'] === 'string') {
			return node.parameters?.['workflowId'];
		}
		if (
			node.parameters &&
			typeof node.parameters['workflowId'] === 'object' &&
			node.parameters['workflowId'] !== null &&
			'value' in node.parameters['workflowId'] &&
			typeof node.parameters['workflowId']['value'] === 'string'
		) {
			return node.parameters['workflowId']['value'];
		}
		this.errorReporter.warn(
			`While indexing, could not determine called workflow ID from executeWorkflow node ${node.id}`,
			{ extra: node.parameters },
		);
		return undefined;
	}
}
Repository: n8n-io/n8n
File: packages/@n8n/task-runner-python/src/pipe_reader.py (Python)

Code before:
            result[offset : offset + len(chunk)] = chunk
            offset += len(chunk)
        return bytes(result)

    def _validate_pipe_message(self, msg) -> PipeMessage:
        if not isinstance(msg, dict):
            raise InvalidPipeMsgContentError(f"Expected dict, got {type(msg).__name__}")

        if "print_args" not in msg:
            raise InvalidPipeMsgContentError("Message missing 'print_args' key")

        if not isinstance(msg["print_args"], list):
            raise InvalidPipeMsgContentError("'print_args' must be a list")

        has_result = "result" in msg
        has_error = "error" in msg

        if not has_result and not has_error:
            raise InvalidPipeMsgContentError("Msg is missing 'result' or 'error' key")

        if has_result and has_error:
            raise InvalidPipeMsgContentError("Msg has both 'result' and 'error' keys")

        if has_result and not isinstance(msg["result"], list):
            raise InvalidPipeMsgContentError("'result' must be a list")

        if has_error and not isinstance(msg["error"], dict):
            raise InvalidPipeMsgContentError("'error' must be a dict")

        return cast(PipeMessage, msg)


Review comment (suggestion):
The result for a code tool call is a plain string rather than items, so two options:

1. Introduce a `tool` mode for the runner and conditionally skip list validation in the runner.
2. Skip list validation in the runner and rely on the TypeScript side validating items via `validateRunCodeAllItems` and `validateRunCodeEachItem` in the Code node. 

Opted for the second. At #18928 we centralized all validation n8n-side, so this lingering runner-side check is redundant and couples the runner to n8n more than needed.

Code after:
            result[offset : offset + len(chunk)] = chunk
            offset += len(chunk)
        return bytes(result)

    def _validate_pipe_message(self, msg) -> PipeMessage:
        if not isinstance(msg, dict):
            raise InvalidPipeMsgContentError(f"Expected dict, got {type(msg).__name__}")

        if "print_args" not in msg:
            raise InvalidPipeMsgContentError("Message missing 'print_args' key")

        if not isinstance(msg["print_args"], list):
            raise InvalidPipeMsgContentError("'print_args' must be a list")

        has_result = "result" in msg
        has_error = "error" in msg

        if not has_result and not has_error:
            raise InvalidPipeMsgContentError("Msg is missing 'result' or 'error' key")

        if has_result and has_error:
            raise InvalidPipeMsgContentError("Msg has both 'result' and 'error' keys")

        if has_error and not isinstance(msg["error"], dict):
            raise InvalidPipeMsgContentError("'error' must be a dict")

        return cast(PipeMessage, msg)
Repository: n8n-io/n8n
File: packages/nodes-base/nodes/Webhook/utils.ts (TypeScript)

Code before:
		outputs[outputIndex] = [outputData];
		return outputs;
	};
};

export const isIpWhitelisted = (
	whitelist: string | string[] | undefined,
	ips: string[],
	ip?: string,
) => {
	if (whitelist === undefined || whitelist === '') {
		return true;
	}

	if (!Array.isArray(whitelist)) {
		whitelist = whitelist.split(',').map((entry) => entry.trim());
	}

	const allowList = getAllowList(whitelist);

	for (const address of whitelist) {
		if (ip === address || allowList.check(ip ?? '')) {
			return true;
		}

		if (ips.some((entry) => allowList.check(entry))) {
			return true;
		}
	}

	return false;
};

const getAllowList = (whitelist: string[]) => {
	const allowList = new BlockList();

	for (const entry of whitelist) {
		try {
			allowList.addAddress(entry);
		} catch {
			// Ignore invalid entries
		}
	}

	return allowList;
};

export const checkResponseModeConfiguration = (context: IWebhookFunctions) => {
	const responseMode = context.getNodeParameter('responseMode', 'onReceived') as string;
	const connectedNodes = context.getChildNodes(context.getNode().name);


Review comment (refactor):
I think we are now unnecessarily iterating all the addresses in the `whitelist`. Since `BlockList` only allows full IPs, could we simplify this as

```ts
const allowList = getAllowList(whitelist)

if (allowList.check(ip ?? '')) {
  return true;
}

if (ips.some(ipEntry => allowList.check(ipEntry)) {
  return true;
}
```
?

Code after:
		outputs[outputIndex] = [outputData];
		return outputs;
	};
};

export const isIpWhitelisted = (
	whitelist: string | string[] | undefined,
	ips: string[],
	ip?: string,
) => {
	if (whitelist === undefined || whitelist === '') {
		return true;
	}

	if (!Array.isArray(whitelist)) {
		whitelist = whitelist.split(',').map((entry) => entry.trim());
	}

	const allowList = getAllowList(whitelist);

	if (allowList.check(ip ?? '')) {
		return true;
	}

	if (ips.some((ipEntry) => allowList.check(ipEntry))) {
		return true;
	}

	return false;
};

const getAllowList = (whitelist: string[]) => {
	const allowList = new BlockList();

	for (const entry of whitelist) {
		try {
			allowList.addAddress(entry);
		} catch {
			// Ignore invalid entries
		}
	}

	return allowList;
};

export const checkResponseModeConfiguration = (context: IWebhookFunctions) => {
	const responseMode = context.getNodeParameter('responseMode', 'onReceived') as string;
	const connectedNodes = context.getChildNodes(context.getNode().name);

	const isRespondToWebhookConnected = connectedNodes.some(
		(node) => node.type === 'n8n-nodes-base.respondToWebhook',
Repository: n8n-io/n8n
File: packages/@n8n/config/src/configs/runners.config.ts (TypeScript)

Code before:
	 * How long (in seconds) a task is allowed to take for completion, else the
	 * task will be aborted. (In internal mode, the runner will also be
	 * restarted.) Must be greater than 0.
	 *
	 * Kept high for backwards compatibility - n8n v2 will reduce this to `60`
	 */
	@Env('N8N_RUNNERS_TASK_TIMEOUT')
	taskTimeout: number = 300; // 5 minutes

	/**
	 * How long (in seconds) a task request can wait for a runner to become
	 * available before timing out. This prevents workflows from hanging
	 * indefinitely when no runners are available. Must be greater than 0.
	 */
	@Env('N8N_RUNNERS_TASK_REQUEST_TIMEOUT')
	taskRequestTimeout: number = 60;

	/** How often (in seconds) the runner must send a heartbeat to the broker, else the task will be aborted. (In internal mode, the runner will also  be restarted.) Must be greater than 0. */
	@Env('N8N_RUNNERS_HEARTBEAT_INTERVAL')
	heartbeatInterval: number = 30;

	/**
	 * How long (in seconds) to wait for running tasks to complete during shutdown
	 * before force-closing connections. Must be greater than or equal to 0.
	 */
	@Env('N8N_RUNNERS_DRAIN_TIMEOUT')
	drainTimeout: number = 30;

	/**
	 * Whether to disable all security measures in the task runner. **Discouraged for production use.**
	 * Set to `true` for compatibility with modules that rely on insecure JS features.
	 */
	@Env('N8N_RUNNERS_INSECURE_MODE')
	insecureMode: boolean = false;
}


Review comment (suggestion):
I don't think we need another timeout for this. We can use e.g. 80-90% of what graful sthudown timeout is

Code after:
	 * restarted.) Must be greater than 0.
	 *
	 * Kept high for backwards compatibility - n8n v3 will reduce this to `60`
	 */
	@Env('N8N_RUNNERS_TASK_TIMEOUT')
	taskTimeout: number = 300; // 5 minutes

	/**
	 * How long (in seconds) a task request can wait for a runner to become
	 * available before timing out. This prevents workflows from hanging
	 * indefinitely when no runners are available. Must be greater than 0.
	 */
	@Env('N8N_RUNNERS_TASK_REQUEST_TIMEOUT')
	taskRequestTimeout: number = 60;

	/** How often (in seconds) the runner must send a heartbeat to the broker, else the task will be aborted. (In internal mode, the runner will also  be restarted.) Must be greater than 0. */
	@Env('N8N_RUNNERS_HEARTBEAT_INTERVAL')
	heartbeatInterval: number = 30;

	/**
	 * Whether to disable all security measures in the task runner. **Discouraged for production use.**
	 * Set to `true` for compatibility with modules that rely on insecure JS features.
	 */
	@Env('N8N_RUNNERS_INSECURE_MODE')
	insecureMode: boolean = false;
}