ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/react
File: packages/react-devtools-shared/src/bridge.js (JavaScript)

Code before:
type NativeStyleEditor_RenameAttributeParams = {
  ...ElementAndRendererID,
  oldName: string,
  newName: string,
  value: string,
};

type NativeStyleEditor_SetValueParams = {
  ...ElementAndRendererID,
  name: string,
  value: string,
};

type SavedPreferencesParams = {
  componentFilters: Array<ComponentFilter>,
};

export type BackendEvents = {
  backendInitialized: [],
  backendVersion: [string],
  bridgeProtocol: [BridgeProtocol],
  extensionBackendInitialized: [],
  fastRefreshScheduled: [],
  getSavedPreferences: [],
  inspectedElement: [InspectedElementPayload],
  isReloadAndProfileSupported: [boolean],
  operations: [Array<number>],
  ownersList: [OwnersList],
  overrideComponentFilters: [Array<ComponentFilter>],
  environmentNames: [Array<string>],
  profilingData: [ProfilingDataBackend],
  profilingStatus: [boolean],
  reloadAppForProfiling: [],
  saveToClipboard: [string],
  selectElement: [number],
  shutdown: [],
  stopInspectingHost: [boolean],
  syncSelectionFromBuiltinElementsPanel: [],
  syncSelectionToBuiltinElementsPanel: [],
  unsupportedRendererVersion: [],

  // React Native style editor plug-in.
  isNativeStyleEditorSupported: [
    {isSupported: boolean, validAttributes: ?$ReadOnlyArray<string>},
  ],
  NativeStyleEditor_styleAndLayout: [StyleAndLayoutPayload],

  hookSettings: [$ReadOnly<DevToolsHookSettings>],
};

type FrontendEvents = {

Review comment (suggestion):
Can it be `backendSupportsReloadAndProfile` or `isReloadAndProfileSupportedByBackend`?

The final version is up to you, I just want it to explicitly mention that this is about backend.

Code after:
type NativeStyleEditor_RenameAttributeParams = {
  ...ElementAndRendererID,
  oldName: string,
  newName: string,
  value: string,
};

type NativeStyleEditor_SetValueParams = {
  ...ElementAndRendererID,
  name: string,
  value: string,
};

type SavedPreferencesParams = {
  componentFilters: Array<ComponentFilter>,
};

export type BackendEvents = {
  backendInitialized: [],
  backendVersion: [string],
  bridgeProtocol: [BridgeProtocol],
  extensionBackendInitialized: [],
  fastRefreshScheduled: [],
  getSavedPreferences: [],
  inspectedElement: [InspectedElementPayload],
  isReloadAndProfileSupportedByBackend: [boolean],
  operations: [Array<number>],
  ownersList: [OwnersList],
  overrideComponentFilters: [Array<ComponentFilter>],
  environmentNames: [Array<string>],
  profilingData: [ProfilingDataBackend],
  profilingStatus: [boolean],
  reloadAppForProfiling: [],
  saveToClipboard: [string],
  selectElement: [number],
  shutdown: [],
  stopInspectingHost: [boolean],
  syncSelectionFromBuiltinElementsPanel: [],
  syncSelectionToBuiltinElementsPanel: [],
  unsupportedRendererVersion: [],

  // React Native style editor plug-in.
  isNativeStyleEditorSupported: [
    {isSupported: boolean, validAttributes: ?$ReadOnlyArray<string>},
  ],
  NativeStyleEditor_styleAndLayout: [StyleAndLayoutPayload],

  hookSettings: [$ReadOnly<DevToolsHookSettings>],
};

type FrontendEvents = {
Repository: facebook/react
File: packages/react-devtools-shared/src/backend/index.js (JavaScript)

Code before:
  hook.reactDevtoolsAgent = agent;

  const onAgentShutdown = () => {
    subs.forEach(fn => fn());
    hook.rendererInterfaces.forEach(rendererInterface => {
      rendererInterface.cleanup();
    });
    hook.reactDevtoolsAgent = null;
  };
  agent.addListener('shutdown', onAgentShutdown);
  subs.push(() => {
    agent.removeListener('shutdown', onAgentShutdown);
  });

  agent.addListener('updateHookSettings', settings => {
    hook.settings = settings;
  });

  agent.addListener('getHookSettings', () => {
    if (hook.settings != null) {
      agent.onHookSettings(hook.settings);
    }
  });

  const getIsReloadAndProfileSupported =
    hook.settings?.getIsReloadAndProfileSupported ??
    defaultGetIsReloadAndProfileSupported;

  if (getIsReloadAndProfileSupported()) {
    agent.onReloadAndProfileSupportedByHost();
  }

  return () => {
    subs.forEach(fn => fn());
  };
}

function defaultGetIsReloadAndProfileSupported() {
  // Notify the frontend if the backend supports the Storage API (e.g. localStorage).
  // If not, features like reload-and-profile will not work correctly and must be disabled.
  let isBackendStorageAPISupported = false;
  try {
    localStorage.getItem('test');
    isBackendStorageAPISupported = true;
  } catch (error) {}

  return isBackendStorageAPISupported && isSynchronousXHRSupported();
}


Review comment (suggestion):
This won't do anything, because you don't store it on the `Hook` object, and you don't need to.

Can you add boolean optional parameter to `initBackend` here?
https://github.com/facebook/react/blob/a15bbe14751287cb7ac124ff88f694d0883f3ac6/packages/react-devtools-shared/src/backend/index.js#L16-L20

Then, anywhere where you are calling `initBackend`: browser extension content script, `react-devtools-core`, `react-devtools-inline`, you will provide this argument.

For browser extension it will be `true` by default, for `react-devtools-inline` it will be `false` (we don't support it), for `react-devtools-core` this will be the return of the function `isReloadAndProfileSupported`, which is passed from the Host (React Native in our case)

Code after:
  hook.reactDevtoolsAgent = agent;

  const onAgentShutdown = () => {
    subs.forEach(fn => fn());
    hook.rendererInterfaces.forEach(rendererInterface => {
      rendererInterface.cleanup();
    });
    hook.reactDevtoolsAgent = null;
  };
  agent.addListener('shutdown', onAgentShutdown);
  subs.push(() => {
    agent.removeListener('shutdown', onAgentShutdown);
  });

  agent.addListener('updateHookSettings', settings => {
    hook.settings = settings;
  });

  agent.addListener('getHookSettings', () => {
    if (hook.settings != null) {
      agent.onHookSettings(hook.settings);
    }
  });

  if (isReloadAndProfileSupported) {
    agent.onReloadAndProfileSupportedByHost();
  }

  return () => {
    subs.forEach(fn => fn());
  };
}
Repository: facebook/react
File: packages/react-devtools-shared/src/backend/index.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 Agent from './agent';

import type {DevToolsHook, RendererID, RendererInterface} from './types';
import {isSynchronousXHRSupported} from './utils';

export type InitBackend = typeof initBackend;

export function initBackend(
  hook: DevToolsHook,
  agent: Agent,
  global: Object,
  getIsReloadAndProfileSupported: () => boolean = defaultGetIsReloadAndProfileSupported,
): () => void {
  if (hook == null) {
    // DevTools didn't get injected into this page (maybe b'c of the contentType).
    return () => {};
  }

  function registerRendererInterface(
    id: RendererID,
    rendererInterface: RendererInterface,
  ) {
    agent.registerRendererInterface(id, rendererInterface);

    // Now that the Store and the renderer interface are connected,
    // it's time to flush the pending operation codes to the frontend.
    rendererInterface.flushInitialOperations();
  }

  const subs = [
    hook.sub(
      'renderer-attached',
      ({
        id,
        rendererInterface,
      }: {
        id: number,

Review comment (suggestion):
Can this be boolean with a default value `false`? And then wrappers around it are going to pass just a boolean value, and `react-devtools-core` is going to pass `getIsReloadAndProfileSupported()`.

With a current implementation, the extension is going to use `defaultGetIsReloadAndProfileSupported`, but actually it should just receive a static `true` value.

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 Agent from './agent';

import type {DevToolsHook, RendererID, RendererInterface} from './types';

export type InitBackend = typeof initBackend;

export function initBackend(
  hook: DevToolsHook,
  agent: Agent,
  global: Object,
  isReloadAndProfileSupported: boolean,
): () => void {
  if (hook == null) {
    // DevTools didn't get injected into this page (maybe b'c of the contentType).
    return () => {};
  }

  function registerRendererInterface(
    id: RendererID,
    rendererInterface: RendererInterface,
  ) {
    agent.registerRendererInterface(id, rendererInterface);

    // Now that the Store and the renderer interface are connected,
    // it's time to flush the pending operation codes to the frontend.
    rendererInterface.flushInitialOperations();
  }

  const subs = [
    hook.sub(
      'renderer-attached',
      ({
        id,
        rendererInterface,
      }: {
        id: number,
        rendererInterface: RendererInterface,
Repository: facebook/react
File: packages/react-devtools-core/src/backend.js (JavaScript)

Code before:
  onSettingsUpdated?: (settings: $ReadOnly<DevToolsHookSettings>) => void,
  isReloadAndProfileSupported?: boolean,
  isProfiling?: boolean,
  onReloadAndProfile?: (recordChangeDescriptions: boolean) => void,
  onReloadAndProfileFlagsReset?: () => void,
};

let savedComponentFilters: Array<ComponentFilter> =
  getDefaultComponentFilters();

function debug(methodName: string, ...args: Array<mixed>) {
  if (__DEBUG__) {
    console.log(
      `%c[core/backend] %c${methodName}`,
      'color: teal; font-weight: bold;',
      'font-weight: bold;',
      ...args,
    );
  }
}

export function initialize(
  maybeSettingsOrSettingsPromise?:
    | DevToolsHookSettings
    | Promise<DevToolsHookSettings>,
  shouldStartProfilingRightNow: boolean = false,
  profilingSettings?: ProfilingSettings,
) {
  installHook(
    window,
    maybeSettingsOrSettingsPromise,
    shouldStartProfilingRightNow,
    profilingSettings,
  );
}

export function connectToDevTools(options: ?ConnectOptions) {
  const hook: ?DevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__;
  if (hook == null) {
    // DevTools didn't get injected into this page (maybe b'c of the contentType).
    return;
  }

  const {
    host = 'localhost',
    nativeStyleEditorValidAttributes,
    useHttps = false,
    port = 8097,
    websocket,
    resolveRNStyle = (null: $FlowFixMe),
    retryConnectionDelay = 2000,

Review comment (suggestion):
```suggestion
  shouldStartProfilingNow: boolean = false,
```

Code after:
  onSettingsUpdated?: (settings: $ReadOnly<DevToolsHookSettings>) => void,
  isReloadAndProfileSupported?: boolean,
  isProfiling?: boolean,
  onReloadAndProfile?: (recordChangeDescriptions: boolean) => void,
  onReloadAndProfileFlagsReset?: () => void,
};

let savedComponentFilters: Array<ComponentFilter> =
  getDefaultComponentFilters();

function debug(methodName: string, ...args: Array<mixed>) {
  if (__DEBUG__) {
    console.log(
      `%c[core/backend] %c${methodName}`,
      'color: teal; font-weight: bold;',
      'font-weight: bold;',
      ...args,
    );
  }
}

export function initialize(
  maybeSettingsOrSettingsPromise?:
    | DevToolsHookSettings
    | Promise<DevToolsHookSettings>,
  shouldStartProfilingNow: boolean = false,
  profilingSettings?: ProfilingSettings,
) {
  installHook(
    window,
    maybeSettingsOrSettingsPromise,
    shouldStartProfilingNow,
    profilingSettings,
  );
}

export function connectToDevTools(options: ?ConnectOptions) {
  const hook: ?DevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__;
  if (hook == null) {
    // DevTools didn't get injected into this page (maybe b'c of the contentType).
    return;
  }

  const {
    host = 'localhost',
    nativeStyleEditorValidAttributes,
    useHttps = false,
    port = 8097,
    websocket,
    resolveRNStyle = (null: $FlowFixMe),
    retryConnectionDelay = 2000,
Repository: facebook/react
File: packages/react-devtools-core/src/backend.js (JavaScript)

Code before:
  getDefaultComponentFilters();

function debug(methodName: string, ...args: Array<mixed>) {
  if (__DEBUG__) {
    console.log(
      `%c[core/backend] %c${methodName}`,
      'color: teal; font-weight: bold;',
      'font-weight: bold;',
      ...args,
    );
  }
}

type ReactNativeProfilingSettings = {
  recordChangeDescriptions: boolean,
};
export function initialize(
  maybeSettingsOrSettingsPromise?:
    | DevToolsHookSettings
    | Promise<DevToolsHookSettings>,
  shouldStartProfilingRightNow: boolean = false,
  profilingSettings?: ReactNativeProfilingSettings,
) {
  const mergedProfilingSettings: ProfilingSettings | void =
    typeof profilingSettings === 'object' && profilingSettings != null
      ? {...profilingSettings, recordTimeline: false}
      : undefined;

  installHook(
    window,
    maybeSettingsOrSettingsPromise,
    shouldStartProfilingRightNow,
    mergedProfilingSettings,
  );
}

export function connectToDevTools(options: ?ConnectOptions) {
  const hook: ?DevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__;
  if (hook == null) {
    // DevTools didn't get injected into this page (maybe b'c of the contentType).
    return;
  }

  const {
    host = 'localhost',
    nativeStyleEditorValidAttributes,
    useHttps = false,
    port = 8097,
    websocket,
    resolveRNStyle = (null: $FlowFixMe),
    retryConnectionDelay = 2000,

Review comment (suggestion):
Wait so does it mean we will never respect `recordTimeline: true`  pass in from `initialize`? Could we add a comment explaining this? e.g. if `store` is the only way to set this to true

Code after:
  getDefaultComponentFilters();

function debug(methodName: string, ...args: Array<mixed>) {
  if (__DEBUG__) {
    console.log(
      `%c[core/backend] %c${methodName}`,
      'color: teal; font-weight: bold;',
      'font-weight: bold;',
      ...args,
    );
  }
}

export function initialize(
  maybeSettingsOrSettingsPromise?:
    | DevToolsHookSettings
    | Promise<DevToolsHookSettings>,
  shouldStartProfilingNow: boolean = false,
  profilingSettings?: ProfilingSettings,
) {
  installHook(
    window,
    maybeSettingsOrSettingsPromise,
    shouldStartProfilingNow,
    profilingSettings,
  );
}

export function connectToDevTools(options: ?ConnectOptions) {
  const hook: ?DevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__;
  if (hook == null) {
    // DevTools didn't get injected into this page (maybe b'c of the contentType).
    return;
  }

  const {
    host = 'localhost',
    nativeStyleEditorValidAttributes,
    useHttps = false,
    port = 8097,
    websocket,
    resolveRNStyle = (null: $FlowFixMe),
    retryConnectionDelay = 2000,
    isAppActive = () => true,
    onSettingsUpdated,
    isReloadAndProfileSupported = getIsReloadAndProfileSupported(),
    isProfiling,
    onReloadAndProfile,
    onReloadAndProfileFlagsReset,
  } = options || {};
Repository: facebook/react
File: packages/react-devtools-shared/src/devtools/views/Profiler/HookChangeSummary.js (JavaScript)

Code before:
import * as React from 'react';
import {
  useContext,
  useMemo,
  useCallback,
  memo,
  useState,
  useEffect,
} from 'react';
import styles from './HookChangeSummary.css';
import ButtonIcon from '../ButtonIcon';
import {InspectedElementContext} from '../Components/InspectedElementContext';
import {StoreContext} from '../context';
import {
  HooksList,
  State,
} from 'react-devtools-shared/src/devtools/views/Profiler/types';

import {
  getAlreadyLoadedHookNames,
  getHookSourceLocationKey,
} from 'react-devtools-shared/src/hookNamesCache';
import Toggle from '../Toggle';

const hookListFormatter = new Intl.ListFormat('en', {
  style: 'long',
  type: 'conjunction',
});

const Hook = memo(({hook, hookNames}) => {
  const hookSource = hook.hookSource;
  const hookName = useMemo(() => {
    if (!hookSource || !hookNames) return null;
    const key = getHookSourceLocationKey(hookSource);
    return hookNames.get(key) || null;

Review comment (suggestion):
This file is missing Flow typings, you can trigger it by adding a comment header, similarly to other files:

```
/**
 * 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
 */
```

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,
  useMemo,
  useCallback,
  memo,
  useState,
  useEffect,
} from 'react';
import styles from './HookChangeSummary.css';
import ButtonIcon from '../ButtonIcon';
import {InspectedElementContext} from '../Components/InspectedElementContext';
import {StoreContext} from '../context';

import {
  getAlreadyLoadedHookNames,
  getHookSourceLocationKey,
} from 'react-devtools-shared/src/hookNamesCache';
import Toggle from '../Toggle';
import type {HooksNode} from 'react-debug-tools/src/ReactDebugHooks';
import type {ChangeDescription} from './types';

// $FlowFixMe: Flow doesn't know about Intl.ListFormat
const hookListFormatter = new Intl.ListFormat('en', {
  style: 'long',
  type: 'conjunction',
Repository: facebook/react
File: packages/react-devtools-shared/src/devtools/views/Profiler/HookChangeSummary.js (JavaScript)

Code before:
          className={hook.id !== null ? styles.PrimitiveHookName : styles.Name}>
          {hook.name}
          {hookName && <span className={styles.HookName}>({hookName})</span>}
        </span>
        {hook.subHooks?.map((subHook, index) => (
          <Hook
            key={`${hook.id}-${index}`}
            hook={subHook}
            hookNames={hookNames}
          />
        ))}
      </li>
    </ul>
  );
});

const shouldKeepHook = (
  hook: HooksNode,
  hooksArray: Array<number>,
): boolean => {
  if (hook.id !== null && hooksArray.includes(hook.id)) {
    return true;
  }
  return (
    hook.subHooks?.some(subHook => shouldKeepHook(subHook, hooksArray)) ?? false
  );
};

const filterHooks = (
  hook: HooksNode,
  hooksArray: Array<number>,
): HooksNode | null => {
  if (!shouldKeepHook(hook, hooksArray)) {
    return null;
  }

  if (hook.subHooks?.length > 0) {
    const filteredSubHooks = hook.subHooks
      .map(subHook => filterHooks(subHook, hooksArray))
      .filter(Boolean);

    return filteredSubHooks.length > 0
      ? {...hook, subHooks: filteredSubHooks}
      : {...hook};
  }

  return hook;
};

type Props = {|
  fiberID: number,

Review comment (suggestion):
```suggestion
  const subHooks = hook.subHooks;
  if (subHooks == null) {
    return false;
  }
  
  return subHooks.some(subHook => shouldKeepHook(subHook, hooksArray));
```

Code after:
          className={hook.id !== null ? styles.PrimitiveHookName : styles.Name}>
          {hook.name}
          {hookName && <span className={styles.HookName}>({hookName})</span>}
        </span>
        {hook.subHooks?.map((subHook, index) => (
          <Hook key={hook.id} hook={subHook} hookNames={hookNames} />
        ))}
      </li>
    </ul>
  );
});

const shouldKeepHook = (
  hook: HooksNode,
  hooksArray: Array<number>,
): boolean => {
  if (hook.id !== null && hooksArray.includes(hook.id)) {
    return true;
  }
  const subHooks = hook.subHooks;
  if (subHooks == null) {
    return false;
  }

  return subHooks.some(subHook => shouldKeepHook(subHook, hooksArray));
};

const filterHooks = (
  hook: HooksNode,
  hooksArray: Array<number>,
): HooksNode | null => {
  if (!shouldKeepHook(hook, hooksArray)) {
    return null;
  }

  const subHooks = hook.subHooks;
  if (subHooks == null) {
    return hook;
  }

  const filteredSubHooks = subHooks
    .map(subHook => filterHooks(subHook, hooksArray))
    .filter(Boolean);
  return filteredSubHooks.length > 0
    ? {...hook, subHooks: filteredSubHooks}
    : hook;
};

type Props = {|
  fiberID: number,
  hooks: $PropertyType<ChangeDescription, 'hooks'>,
Repository: facebook/react
File: packages/react-devtools-shared/src/devtools/views/Profiler/HookChangeSummary.js (JavaScript)

Code before:
};

const Hook: React.AbstractComponent<HookProps> = memo(({hook, hookNames}) => {
  const hookSource = hook.hookSource;
  const hookName = useMemo(() => {
    if (!hookSource || !hookNames) return null;
    const key = getHookSourceLocationKey(hookSource);
    return hookNames.get(key) || null;
  }, [hookSource, hookNames]);

  return (
    <ul className={styles.Hook}>
      <li>
        {hook.id !== null && (
          <span className={styles.PrimitiveHookNumber}>
            {String(hook.id + 1)}
          </span>
        )}
        <span
          className={hook.id !== null ? styles.PrimitiveHookName : styles.Name}>
          {hook.name}
          {hookName && <span className={styles.HookName}>({hookName})</span>}
        </span>
        {hook.subHooks?.map((subHook, index) => (
          <Hook
            key={`${hook.id}-${index}`}
            hook={subHook}
            hookNames={hookNames}
          />
        ))}
      </li>
    </ul>
  );
});

const shouldKeepHook = (
  hook: HooksNode,
  hooksArray: Array<number>,
): boolean => {
  if (hook.id !== null && hooksArray.includes(hook.id)) {
    return true;
  }
  return (
    hook.subHooks?.some(subHook => shouldKeepHook(subHook, hooksArray)) ?? false
  );
};

const filterHooks = (
  hook: HooksNode,
  hooksArray: Array<number>,
): HooksNode | null => {

Review comment (suggestion):
Do you really need an `index` here for constructing a key? `hook.id` and `index` actually have a correlation, because both of them depend on the fact when the hook was defined relatively to other hooks: in the end all of them are going to be stored in a linked list.

Code after:
};

const Hook: React.AbstractComponent<HookProps> = memo(({hook, hookNames}) => {
  const hookSource = hook.hookSource;
  const hookName = useMemo(() => {
    if (!hookSource || !hookNames) return null;
    const key = getHookSourceLocationKey(hookSource);
    return hookNames.get(key) || null;
  }, [hookSource, hookNames]);

  return (
    <ul className={styles.Hook}>
      <li>
        {hook.id !== null && (
          <span className={styles.PrimitiveHookNumber}>
            {String(hook.id + 1)}
          </span>
        )}
        <span
          className={hook.id !== null ? styles.PrimitiveHookName : styles.Name}>
          {hook.name}
          {hookName && <span className={styles.HookName}>({hookName})</span>}
        </span>
        {hook.subHooks?.map((subHook, index) => (
          <Hook key={hook.id} hook={subHook} hookNames={hookNames} />
        ))}
      </li>
    </ul>
  );
});

const shouldKeepHook = (
  hook: HooksNode,
  hooksArray: Array<number>,
): boolean => {
  if (hook.id !== null && hooksArray.includes(hook.id)) {
    return true;
  }
  const subHooks = hook.subHooks;
  if (subHooks == null) {
    return false;
  }

  return subHooks.some(subHook => shouldKeepHook(subHook, hooksArray));
};

const filterHooks = (
  hook: HooksNode,
  hooksArray: Array<number>,
): HooksNode | null => {
  if (!shouldKeepHook(hook, hooksArray)) {