ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/react
File: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts (TypeScript)

Code before:
    return hasChanges;
  }

  addDerivationEntry(
    derivedVar: Place,
    sourcesIds: Set<IdentifierId>,
    typeOfValue: TypeOfValue,
  ): void {
    let newValue: DerivationMetadata = {
      place: derivedVar,
      sourcesIds: new Set(),
      typeOfValue: typeOfValue ?? 'ignored',
    };

    if (isNamedIdentifier(derivedVar)) {
      newValue.sourcesIds.add(derivedVar.identifier.id);
    }

    for (const id of sourcesIds) {
      const sourceMetadata = this.cache.get(id);

      if (sourceMetadata === undefined) {
        continue;
      }

      if (isNamedIdentifier(sourceMetadata.place)) {
        newValue.sourcesIds.add(sourceMetadata.place.identifier.id);
      } else {
        for (const sourcesSourceId of sourceMetadata.sourcesIds) {
          newValue.sourcesIds.add(sourcesSourceId);
        }
      }
    }

    this.cache.set(derivedVar.identifier.id, newValue);
  }

  private isDerivationEqual(
    a: DerivationMetadata,
    b: DerivationMetadata,
  ): boolean {
    if (a.typeOfValue !== b.typeOfValue) {
      return false;
    }
    if (a.sourcesIds.size !== b.sourcesIds.size) {
      return false;
    }
    for (const id of a.sourcesIds) {
      if (!b.sourcesIds.has(id)) {
        return false;
      }

Review comment (suggestion):
```
CompilerError.invariant(isNamedIdentifier(sourceMetadata.place), {
  reason: "Compiler internal error: expected derivation entries to always reference named variables),
```

Code after:
    const hasChanges = this.hasChanges;
    this.hasChanges = false;
    return hasChanges;
  }

  addDerivationEntry(
    derivedVar: Place,
    sourcesIds: Set<IdentifierId>,
    typeOfValue: TypeOfValue,
    isStateSource: boolean,
  ): void {
    let finalIsSource = isStateSource;
    if (!finalIsSource) {
      for (const sourceId of sourcesIds) {
        const sourceMetadata = this.cache.get(sourceId);
        if (
          sourceMetadata?.isStateSource &&
          sourceMetadata.place.identifier.name?.kind !== 'named'
        ) {
          finalIsSource = true;
          break;
        }
      }
    }

    this.cache.set(derivedVar.identifier.id, {
      place: derivedVar,
      sourcesIds: sourcesIds,
      typeOfValue: typeOfValue ?? 'ignored',
      isStateSource: finalIsSource,
    });
  }

  private isDerivationEqual(
    a: DerivationMetadata,
    b: DerivationMetadata,
  ): boolean {
    if (a.typeOfValue !== b.typeOfValue) {
      return false;
    }
    if (a.sourcesIds.size !== b.sourcesIds.size) {
      return false;
    }
    for (const id of a.sourcesIds) {
      if (!b.sourcesIds.has(id)) {
        return false;
      }
    }
    return true;
  }
}
Repository: facebook/react
File: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts (TypeScript)

Code before:

  snapshot(): boolean {
    const hasChanges = this.hasChanges;
    this.hasChanges = false;
    return hasChanges;
  }

  addDerivationEntry(
    derivedVar: Place,
    sourcesIds: Set<IdentifierId>,
    typeOfValue: TypeOfValue,
  ): void {
    let newValue: DerivationMetadata = {
      place: derivedVar,
      sourcesIds: new Set(),
      typeOfValue: typeOfValue ?? 'ignored',
    };

    if (isNamedIdentifier(derivedVar)) {
      newValue.sourcesIds.add(derivedVar.identifier.id);
    }

    for (const id of sourcesIds) {
      const sourceMetadata = this.cache.get(id);

      if (sourceMetadata === undefined) {
        continue;
      }

      if (isNamedIdentifier(sourceMetadata.place)) {
        newValue.sourcesIds.add(sourceMetadata.place.identifier.id);
      } else {
        for (const sourcesSourceId of sourceMetadata.sourcesIds) {
          newValue.sourcesIds.add(sourcesSourceId);
        }
      }
    }

    this.cache.set(derivedVar.identifier.id, newValue);
  }

  private isDerivationEqual(
    a: DerivationMetadata,
    b: DerivationMetadata,
  ): boolean {
    if (a.typeOfValue !== b.typeOfValue) {
      return false;
    }
    if (a.sourcesIds.size !== b.sourcesIds.size) {
      return false;
    }

Review comment (suggestion):
We checked if all calls to `addDerivationEntry` only add known-sourceIds, so this should be safe to remove (or assert with `CompilerError.invariant`)

Code after:
    this.hasChanges = false;
  }

  snapshot(): boolean {
    const hasChanges = this.hasChanges;
    this.hasChanges = false;
    return hasChanges;
  }

  addDerivationEntry(
    derivedVar: Place,
    sourcesIds: Set<IdentifierId>,
    typeOfValue: TypeOfValue,
    isStateSource: boolean,
  ): void {
    let finalIsSource = isStateSource;
    if (!finalIsSource) {
      for (const sourceId of sourcesIds) {
        const sourceMetadata = this.cache.get(sourceId);
        if (
          sourceMetadata?.isStateSource &&
          sourceMetadata.place.identifier.name?.kind !== 'named'
        ) {
          finalIsSource = true;
          break;
        }
      }
    }

    this.cache.set(derivedVar.identifier.id, {
      place: derivedVar,
      sourcesIds: sourcesIds,
      typeOfValue: typeOfValue ?? 'ignored',
      isStateSource: finalIsSource,
    });
  }

  private isDerivationEqual(
    a: DerivationMetadata,
    b: DerivationMetadata,
  ): boolean {
    if (a.typeOfValue !== b.typeOfValue) {
      return false;
    }
    if (a.sourcesIds.size !== b.sourcesIds.size) {
      return false;
    }
    for (const id of a.sourcesIds) {
      if (!b.sourcesIds.has(id)) {
        return false;
      }
Repository: facebook/react
File: packages/react-devtools-shared/src/backend/fiber/renderer.js (JavaScript)

Code before:

    // Compare the values to see if they changed
    if (isStatefulHook) {
      return prev.value !== next.value;
    }

    return false;
  }

  function getChangedHooksIndices(
    prevHooks: HooksTree | null,
    nextHooks: HooksTree | null,
  ): null | Array<number> {
    if (prevHooks == null || nextHooks == null) {
      return null;
    }

    const indices: Array<number> = [];
    let index = 0;

    function traverse(prevTree: HooksTree, nextTree: HooksTree): void {
      for (let i = 0; i < prevTree.length; i++) {
        const prevHook = prevTree[i];
        const nextHook = nextTree[i];

        if (prevHook?.subHooks?.length > 0 && nextHook?.subHooks?.length > 0) {
          traverse(prevHook.subHooks, nextHook.subHooks);
          continue;
        }

        if (didStatefulHookChange(prevHook, nextHook)) {
          indices.push(index);
        }

        index++;
      }
    }

    traverse(prevHooks, nextHooks);
    return indices;
  }

  function getChangedKeys(prev: any, next: any): null | Array<string> {
    if (prev == null || next == null) {
      return null;
    }

    const keys = new Set([...Object.keys(prev), ...Object.keys(next)]);
    const changedKeys = [];
    // eslint-disable-next-line no-for-of-loops/no-for-of-loops
    for (const key of keys) {

Review comment (bug):
```suggestion
        if (prevHook.subHooks?.length > 0 && nextHook.subHooks?.length > 0) {
```

Why would prevHook or nextHook be optional here, though, if the types say otherwise?

I think it is better to crash here with an error that we will fix, rather than avoiding it and potentially producing incorrect result

Code after:

    // Compare the values to see if they changed
    if (isStatefulHook) {
      return prev.value !== next.value;
    }

    return false;
  }

  function getChangedHooksIndices(
    prevHooks: HooksTree | null,
    nextHooks: HooksTree | null,
  ): null | Array<number> {
    if (prevHooks == null || nextHooks == null) {
      return null;
    }

    const indices: Array<number> = [];
    let index = 0;

    function traverse(prevTree: HooksTree, nextTree: HooksTree): void {
      for (let i = 0; i < prevTree.length; i++) {
        const prevHook = prevTree[i];
        const nextHook = nextTree[i];

        if (prevHook.subHooks.length > 0 && nextHook.subHooks.length > 0) {
          traverse(prevHook.subHooks, nextHook.subHooks);
          continue;
        }

        if (didStatefulHookChange(prevHook, nextHook)) {
          indices.push(index);
        }

        index++;
      }
    }

    traverse(prevHooks, nextHooks);
    return indices;
  }

  function getChangedKeys(prev: any, next: any): null | Array<string> {
    if (prev == null || next == null) {
      return null;
    }

    const keys = new Set([...Object.keys(prev), ...Object.keys(next)]);
    const changedKeys = [];
    // eslint-disable-next-line no-for-of-loops/no-for-of-loops
    for (const key of keys) {
Repository: facebook/react
File: packages/react-devtools-shared/src/backend/fiber/renderer.js (JavaScript)

Code before:
      return prev.value !== next.value;
    }

    return false;
  }

  function getChangedHooksIndices(
    prevHooks: HooksTree | null,
    nextHooks: HooksTree | null,
  ): null | Array<number> {
    if (prevHooks == null || nextHooks == null) {
      return null;
    }

    const indices: Array<number> = [];
    let index = 0;

    function traverse(prevTree: HooksTree, nextTree: HooksTree): void {
      for (let i = 0; i < prevTree.length; i++) {
        const prevHook = prevTree[i];
        const nextHook = nextTree[i];

        if (prevHook.subHooks?.length > 0 && nextHook.subHooks?.length > 0) {
          traverse(prevHook.subHooks, nextHook.subHooks);
          continue;
        }

        if (didStatefulHookChange(prevHook, nextHook)) {
          indices.push(index);
        }

        index++;
      }
    }

    traverse(prevHooks, nextHooks);
    return indices;
  }

  function getChangedKeys(prev: any, next: any): null | Array<string> {
    if (prev == null || next == null) {
      return null;
    }

    const keys = new Set([...Object.keys(prev), ...Object.keys(next)]);
    const changedKeys = [];
    // eslint-disable-next-line no-for-of-loops/no-for-of-loops
    for (const key of keys) {
      if (prev[key] !== next[key]) {
        changedKeys.push(key);
      }

Review comment (suggestion):
```suggestion
        if (prevHook.subHooks.length > 0 && nextHook.subHooks.length > 0) {
          traverse(prevHook.subHooks, nextHook.subHooks);
          continue;
        }
```

Sorry, I missed the fact that subHooks are also required.

Code after:
      return prev.value !== next.value;
    }

    return false;
  }

  function getChangedHooksIndices(
    prevHooks: HooksTree | null,
    nextHooks: HooksTree | null,
  ): null | Array<number> {
    if (prevHooks == null || nextHooks == null) {
      return null;
    }

    const indices: Array<number> = [];
    let index = 0;

    function traverse(prevTree: HooksTree, nextTree: HooksTree): void {
      for (let i = 0; i < prevTree.length; i++) {
        const prevHook = prevTree[i];
        const nextHook = nextTree[i];

        if (prevHook.subHooks.length > 0 && nextHook.subHooks.length > 0) {
          traverse(prevHook.subHooks, nextHook.subHooks);
          continue;
        }

        if (didStatefulHookChange(prevHook, nextHook)) {
          indices.push(index);
        }

        index++;
      }
    }

    traverse(prevHooks, nextHooks);
    return indices;
  }

  function getChangedKeys(prev: any, next: any): null | Array<string> {
    if (prev == null || next == null) {
      return null;
    }

    const keys = new Set([...Object.keys(prev), ...Object.keys(next)]);
    const changedKeys = [];
    // eslint-disable-next-line no-for-of-loops/no-for-of-loops
    for (const key of keys) {
      if (prev[key] !== next[key]) {
        changedKeys.push(key);
      }
Repository: facebook/react
File: packages/react-devtools-shared/src/devtools/views/Profiler/RecordToggle.js (JavaScript)

Code before:
 */

import * as React from 'react';
import {useContext} from 'react';
import Button from '../Button';
import ButtonIcon from '../ButtonIcon';
import {ProfilerContext} from './ProfilerContext';

import styles from './RecordToggle.css';

export type Props = {
  disabled?: boolean,
};

export default function RecordToggle({disabled}: Props): React.Node {
  const {isProfiling, startProfiling, stopProfiling} =
    useContext(ProfilerContext);

  let className = styles.InactiveRecordToggle;
  if (disabled) {
    className = styles.DisabledRecordToggle;
  } else if (isProfiling) {
    className = styles.ActiveRecordToggle;
  }

  const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0;
  const shortcut = isMac ? '⌘E' : 'Ctrl+E';
  const title = isProfiling ? `Stop profiling - ${shortcut}` : `Start profiling - ${shortcut}`;

  return (
    <Button
      className={className}
      disabled={disabled}
      onClick={isProfiling ? stopProfiling : startProfiling}
      testName="ProfilerToggleButton"
      title={title}>
      <ButtonIcon type="record" />
    </Button>
  );
}


Review comment (suggestion):
React DevTools frontend could be launched also outside of the browser environment. For example, inside Electron wrapper.

Does Electron support `navigator` global? You can launch React DevTools in Electron with `yarn start` in `react-devtools` package.

Code after:
 */

import * as React from 'react';
import {useContext} from 'react';
import Button from '../Button';
import ButtonIcon from '../ButtonIcon';
import {ProfilerContext} from './ProfilerContext';

import styles from './RecordToggle.css';

export type Props = {
  disabled?: boolean,
};

export default function RecordToggle({disabled}: Props): React.Node {
  const {isProfiling, startProfiling, stopProfiling} =
    useContext(ProfilerContext);

  let className = styles.InactiveRecordToggle;
  if (disabled) {
    className = styles.DisabledRecordToggle;
  } else if (isProfiling) {
    className = styles.ActiveRecordToggle;
  }

  const isMac =
    typeof navigator !== 'undefined' &&
    navigator.platform.toUpperCase().indexOf('MAC') >= 0;
  const shortcut = isMac ? '⌘E' : 'Ctrl+E';
  const title = `${isProfiling ? 'Stop' : 'Start'} profiling - ${shortcut}`;

  return (
    <Button
      className={className}
      disabled={disabled}
      onClick={isProfiling ? stopProfiling : startProfiling}
      testName="ProfilerToggleButton"
      title={title}>
      <ButtonIcon type="record" />
    </Button>
  );
}
Repository: facebook/react
File: packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js (JavaScript)

Code before:
  const profilerRef = useRef<HTMLDivElement | null>(null);

  const {
    didRecordCommits,
    isProcessingData,
    isProfiling,
    selectedCommitIndex,
    selectedFiberID,
    selectedTabID,
    selectTab,
    supportsProfiling,
    startProfiling,
    stopProfiling,
  } = useContext(ProfilerContext);

  const {file: timelineTraceEventData, searchInputContainerRef} =
    useContext(TimelineContext);

  const {supportsTimeline} = useContext(StoreContext);

  const isLegacyProfilerSelected = selectedTabID !== 'timeline';

  // Cmd+E to start/stop profiler recording
  useEffect(() => {
    const div = profilerRef.current;
    if (div === null) {
      return;
    }

    const ownerWindow = div.ownerDocument.defaultView;
    const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0;
    const handleKeyDown = (event: KeyboardEvent) => {
      const correctModifier = isMac ? event.metaKey : event.ctrlKey;
      if (correctModifier && event.key === 'e') {
        if (isProfiling) {
          stopProfiling();
        } else {
          startProfiling();
        }
        event.preventDefault();
        event.stopPropagation();
      }
    };

    ownerWindow.addEventListener('keydown', handleKeyDown);
    return () => {
      ownerWindow.removeEventListener('keydown', handleKeyDown);
    };
  }, [isProfiling, startProfiling, stopProfiling]);

  let view = null;

Review comment (suggestion):
`div == null`

Please learn about the difference between `== null` and `=== null`

Code after:
  const profilerRef = useRef<HTMLDivElement | null>(null);
  const isMac =
    typeof navigator !== 'undefined' &&
    navigator.platform.toUpperCase().indexOf('MAC') >= 0;

  const {
    didRecordCommits,
    isProcessingData,
    isProfiling,
    selectedCommitIndex,
    selectedFiberID,
    selectedTabID,
    selectTab,
    supportsProfiling,
    startProfiling,
    stopProfiling,
  } = useContext(ProfilerContext);

  const {file: timelineTraceEventData, searchInputContainerRef} =
    useContext(TimelineContext);

  const {supportsTimeline} = useContext(StoreContext);

  const isLegacyProfilerSelected = selectedTabID !== 'timeline';

  // Cmd+E to start/stop profiler recording
  const handleKeyDown = useEffectEvent((event: KeyboardEvent) => {
    const correctModifier = isMac ? event.metaKey : event.ctrlKey;
    if (correctModifier && event.key === 'e') {
      if (isProfiling) {
        stopProfiling();
      } else {
        startProfiling();
      }
      event.preventDefault();
      event.stopPropagation();
    }
  });

  useEffect(() => {
    const div = profilerRef.current;
    if (!div) {
      return;
    }
    const ownerWindow = div.ownerDocument.defaultView;
    ownerWindow.addEventListener('keydown', handleKeyDown);
    return () => {
      ownerWindow.removeEventListener('keydown', handleKeyDown);
    };
  }, []);
Repository: facebook/react
File: packages/react-devtools-shared/src/devtools/views/Profiler/RecordToggle.js (JavaScript)

Code before:
import * as React from 'react';
import {useContext} from 'react';
import Button from '../Button';
import ButtonIcon from '../ButtonIcon';
import {ProfilerContext} from './ProfilerContext';

import styles from './RecordToggle.css';

export type Props = {
  disabled?: boolean,
};

export default function RecordToggle({disabled}: Props): React.Node {
  const {isProfiling, startProfiling, stopProfiling} =
    useContext(ProfilerContext);

  let className = styles.InactiveRecordToggle;
  if (disabled) {
    className = styles.DisabledRecordToggle;
  } else if (isProfiling) {
    className = styles.ActiveRecordToggle;
  }

  const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0;
  const shortcut = isMac ? '⌘E' : 'Ctrl+E';
  const title = isProfiling ? `Stop profiling - ${shortcut}` : `Start profiling - ${shortcut}`;

  return (
    <Button
      className={className}
      disabled={disabled}
      onClick={isProfiling ? stopProfiling : startProfiling}
      testName="ProfilerToggleButton"
      title={title}>
      <ButtonIcon type="record" />
    </Button>
  );
}


Review comment (suggestion):
```
  const title = `${isProfiling ? 'Stop' : 'Start'} profiling - ${shortcut}`;
```

Code after:
import * as React from 'react';
import {useContext} from 'react';
import Button from '../Button';
import ButtonIcon from '../ButtonIcon';
import {ProfilerContext} from './ProfilerContext';

import styles from './RecordToggle.css';

export type Props = {
  disabled?: boolean,
};

export default function RecordToggle({disabled}: Props): React.Node {
  const {isProfiling, startProfiling, stopProfiling} =
    useContext(ProfilerContext);

  let className = styles.InactiveRecordToggle;
  if (disabled) {
    className = styles.DisabledRecordToggle;
  } else if (isProfiling) {
    className = styles.ActiveRecordToggle;
  }

  const isMac =
    typeof navigator !== 'undefined' &&
    navigator.platform.toUpperCase().indexOf('MAC') >= 0;
  const shortcut = isMac ? '⌘E' : 'Ctrl+E';
  const title = `${isProfiling ? 'Stop' : 'Start'} profiling - ${shortcut}`;

  return (
    <Button
      className={className}
      disabled={disabled}
      onClick={isProfiling ? stopProfiling : startProfiling}
      testName="ProfilerToggleButton"
      title={title}>
      <ButtonIcon type="record" />
    </Button>
  );
}
Repository: facebook/react
File: packages/react-devtools-shared/src/devtools/views/InspectedElement/InspectedElementPane.js (JavaScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

import * as React from 'react';
import {useContext} from 'react';

import portaledContent from 'react-devtools-shared/src/devtools/views/portaledContent';
import {OptionsContext} from 'react-devtools-shared/src/devtools/views/context';
import InspectedElement from 'react-devtools-shared/src/devtools/views/Components/InspectedElement';
import SettingsModal from 'react-devtools-shared/src/devtools/views/Settings/SettingsModal';
import SettingsModalContextToggle from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle';
import {SettingsModalContextController} from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContext';
import styles from './InspectedElementPane.css';

export type Props = {};

function InspectedElementPane(_: Props) {
  const {hideSettings} = useContext(OptionsContext);
  return (
    <SettingsModalContextController>
      <div className={styles.InspectedElementPane}>
        <InspectedElement
          actionButtons={!hideSettings && <SettingsModalContextToggle />}
        />
        <SettingsModal />
      </div>
    </SettingsModalContextController>
  );
}
export default (portaledContent(InspectedElementPane): component());


Review comment (suggestion):
```suggestion
function InspectedElementPane() {
```

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

import * as React from 'react';
import {useContext} from 'react';

import portaledContent from 'react-devtools-shared/src/devtools/views/portaledContent';
import {OptionsContext} from 'react-devtools-shared/src/devtools/views/context';
import InspectedElement from 'react-devtools-shared/src/devtools/views/Components/InspectedElement';
import SettingsModal from 'react-devtools-shared/src/devtools/views/Settings/SettingsModal';
import SettingsModalContextToggle from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle';
import {SettingsModalContextController} from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContext';
import styles from './InspectedElementPane.css';

function InspectedElementPane() {
  const {hideSettings} = useContext(OptionsContext);
  return (
    <SettingsModalContextController>
      <div className={styles.InspectedElementPane}>
        <InspectedElement
          actionButtons={!hideSettings && <SettingsModalContextToggle />}
        />
        <SettingsModal />
      </div>
    </SettingsModalContextController>
  );
}
export default (portaledContent(InspectedElementPane): component());