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

Code before:
        readonly: true,
        preview_short: formatDataForPreview(data, false),
        preview_long: formatDataForPreview(data, true),
        name: data.name,
      };

      // name, message, stack and cause are not enumerable yet still interesting.
      value.message = dehydrate(
        data.message,
        cleaned,
        unserializable,
        path.concat(['message']),
        isPathAllowed,
        isPathAllowedCheck ? 1 : level + 1,
      );
      value.stack = dehydrate(
        data.stack,
        cleaned,
        unserializable,
        path.concat(['stack']),
        isPathAllowed,
        isPathAllowedCheck ? 1 : level + 1,
      );

      if ('cause' in data) {
        value.cause = dehydrate(
          data.cause,
          cleaned,
          unserializable,
          path.concat(['cause']),
          isPathAllowed,
          isPathAllowedCheck ? 1 : level + 1,
        );
      }

      getAllEnumerableKeys(data).forEach(key => {
        const keyAsString = key.toString();

        value[keyAsString] = dehydrate(
          data[key],
          cleaned,
          unserializable,
          path.concat([keyAsString]),
          isPathAllowed,
          isPathAllowedCheck ? 1 : level + 1,
        );
      });

      unserializable.push(path);

      return value;

Review comment (suggestion):
Will be `false` if no `cause` was actually included (`'cause' in new Error('test')`) but `true` if it was (even if undefined) (`'cause' in new Error('test', { cause: undefined })`)

Code after:
        type,
        readonly: true,
        preview_short: formatDataForPreview(data, false),
        preview_long: formatDataForPreview(data, true),
        name: data.name,
      };

      // name, message, stack and cause are not enumerable yet still interesting.
      value.message = dehydrate(
        data.message,
        cleaned,
        unserializable,
        path.concat(['message']),
        isPathAllowed,
        isPathAllowedCheck ? 1 : level + 1,
      );
      value.stack = dehydrate(
        data.stack,
        cleaned,
        unserializable,
        path.concat(['stack']),
        isPathAllowed,
        isPathAllowedCheck ? 1 : level + 1,
      );

      if ('cause' in data) {
        value.cause = dehydrate(
          data.cause,
          cleaned,
          unserializable,
          path.concat(['cause']),
          isPathAllowed,
          isPathAllowedCheck ? 1 : level + 1,
        );
      }

      getAllEnumerableKeys(data).forEach(key => {
        const keyAsString = key.toString();

        value[keyAsString] = dehydrate(
          data[key],
          cleaned,
          unserializable,
          path.concat([keyAsString]),
          isPathAllowed,
          isPathAllowedCheck ? 1 : level + 1,
        );
      });

      unserializable.push(path);
Repository: facebook/react
File: packages/react-native-renderer/src/ReactFiberConfigFabric.js (JavaScript)

Code before:
    return createChildNodeSet();
  }
}

export function appendChildToContainerChildSet(
  childSet: ChildSet,
  child: Instance | TextInstance,
): void {
  if (passChildrenWhenCloningPersistedNodes) {
    childSet.push(child.node);
  } else {
    appendChildNodeToSet(childSet, child.node);
  }
}

export function finalizeContainerChildren(
  container: Container,
  newChildren: ChildSet,
): void {
  if (!enableFabricCompleteRootInCommitPhase) {
    completeRoot(container.containerTag, newChildren);
  }
}

export function replaceContainerChildren(
  container: Container,
  newChildren: ChildSet,
): void {
  // Noop - children will be replaced in finalizeContainerChildren
  if (enableFabricCompleteRootInCommitPhase) {
    completeRoot(container.containerTag, newChildren);
  }
}

export {getClosestInstanceFromNode as getInstanceFromNode};

export function beforeActiveInstanceBlur(
  internalInstanceHandle: InternalInstanceHandle,
) {
  // noop
}

export function afterActiveInstanceBlur() {
  // noop
}

export function preparePortalMount(portalInstance: Instance): void {
  // noop
}

export function detachDeletedInstance(node: Instance): void {

Review comment (suggestion):
This should probably be renamed to commitRoot to align with React terminology. Really these should exist in both forms. Both completeRoot (which would really do layout eagerly) and commitRoot. This is the commitRoot part.

Code after:
  }
}

export function appendChildToContainerChildSet(
  childSet: ChildSet,
  child: Instance | TextInstance,
): void {
  if (passChildrenWhenCloningPersistedNodes) {
    childSet.push(child.node);
  } else {
    appendChildNodeToSet(childSet, child.node);
  }
}

export function finalizeContainerChildren(
  container: Container,
  newChildren: ChildSet,
): void {
  // Noop - children will be replaced in replaceContainerChildren
}

export function replaceContainerChildren(
  container: Container,
  newChildren: ChildSet,
): void {
  completeRoot(container.containerTag, newChildren);
}

export {getClosestInstanceFromNode as getInstanceFromNode};

export function beforeActiveInstanceBlur(
  internalInstanceHandle: InternalInstanceHandle,
) {
  // noop
}

export function afterActiveInstanceBlur() {
  // noop
}

export function preparePortalMount(portalInstance: Instance): void {
  // noop
}

export function detachDeletedInstance(node: Instance): void {
  // noop
}

export function requestPostPaintCallback(callback: (time: number) => void) {
  // noop
}
Repository: facebook/react
File: packages/react-art/src/ReactART.js (JavaScript)

Code before:
  constructor(url, width, height, left, top) {
    this._args = slice.call(arguments);
  }

  applyFill(node) {
    node.fillImage.apply(node, this._args);
  }
}

/** React Components */

class Surface extends React.Component {
  componentDidMount() {
    const {height, width} = this.props;

    this._surface = Mode.Surface(+width, +height, this._tagRef);

    this._mountNode = createContainer(
      this._surface,
      disableLegacyMode ? ConcurrentRoot : LegacyRoot,
      null,
      false,
      false,
      '',
      defaultOnUncaughtError,
      defaultOnCaughtError,
      defaultOnRecoverableError,
      defaultOnDefaultTransitionIndicator,
      null,
    );
    // We synchronously flush updates coming from above so that they commit together
    // and so that refs resolve before the parent life cycles.
    updateContainerSync(this.props.children, this._mountNode, this);
    flushSyncWork();
  }

  componentDidUpdate(prevProps, prevState) {
    const props = this.props;

    if (props.height !== prevProps.height || props.width !== prevProps.width) {
      this._surface.resize(+props.width, +props.height);
    }

    // We synchronously flush updates coming from above so that they commit together
    // and so that refs resolve before the parent life cycles.
    updateContainerSync(this.props.children, this._mountNode, this);
    flushSyncWork();

    if (this._surface.render) {
      this._surface.render();
    }

Review comment (suggestion):
These were missing in ART so any errors would've logged like `onUncaughtError is undefined` instead of the error. Probably not heavily used.

Probably should have a test for errors in ART.

Code after:
    this._args = slice.call(arguments);
  }

  applyFill(node) {
    node.fillImage.apply(node, this._args);
  }
}

/** React Components */

class Surface extends React.Component {
  componentDidMount() {
    const {height, width} = this.props;

    this._surface = Mode.Surface(+width, +height, this._tagRef);

    this._mountNode = createContainer(
      this._surface,
      disableLegacyMode ? ConcurrentRoot : LegacyRoot,
      null,
      false,
      false,
      '',
      defaultOnUncaughtError,
      defaultOnCaughtError,
      defaultOnRecoverableError,
      defaultOnDefaultTransitionIndicator,
      null,
    );
    // We synchronously flush updates coming from above so that they commit together
    // and so that refs resolve before the parent life cycles.
    updateContainerSync(this.props.children, this._mountNode, this);
    flushSyncWork();
  }

  componentDidUpdate(prevProps, prevState) {
    const props = this.props;

    if (props.height !== prevProps.height || props.width !== prevProps.width) {
      this._surface.resize(+props.width, +props.height);
    }

    // We synchronously flush updates coming from above so that they commit together
    // and so that refs resolve before the parent life cycles.
    updateContainerSync(this.props.children, this._mountNode, this);
    flushSyncWork();

    if (this._surface.render) {
      this._surface.render();
    }
  }
Repository: facebook/react
File: packages/react-devtools-extensions/webpack.config.js (JavaScript)

Code before:
if (!NODE_ENV) {
  console.error('NODE_ENV not set');
  process.exit(1);
}

const builtModulesDir = resolve(
  __dirname,
  '..',
  '..',
  'build',
  'oss-experimental',
);

const __DEV__ = NODE_ENV === 'development';

const DEVTOOLS_VERSION = getVersionString(process.env.DEVTOOLS_VERSION);

const EDITOR_URL = process.env.EDITOR_URL || null;
const LOGGING_URL = process.env.LOGGING_URL || null;

const IS_CHROME = process.env.IS_CHROME === 'true';
const IS_FIREFOX = process.env.IS_FIREFOX === 'true';
const IS_EDGE = process.env.IS_EDGE === 'true';
const IS_INTERNAL_VERSION = process.env.FEATURE_FLAG_TARGET === 'extension-fb';

const IS_INTERNAL = process.env.IS_INTERNAL === 'true';

const featureFlagTarget = process.env.FEATURE_FLAG_TARGET || 'extension-oss';

const babelOptions = {
  configFile: resolve(
    __dirname,
    '..',
    'react-devtools-shared',
    'babel.config.js',
  ),
};

module.exports = {
  mode: __DEV__ ? 'development' : 'production',
  devtool: false,
  entry: {
    background: './src/background/index.js',
    backendManager: './src/contentScripts/backendManager.js',
    fileFetcher: './src/contentScripts/fileFetcher.js',
    main: './src/main/index.js',
    panel: './src/panel.js',
    proxy: './src/contentScripts/proxy.js',
    prepareInjection: './src/contentScripts/prepareInjection.js',
    installHook: './src/contentScripts/installHook.js',
    hookSettingsInjector: './src/contentScripts/hookSettingsInjector.js',

Review comment (suggestion):
```suggestion
const IS_INTERNAL_MCP_BUILD = process.env.IS_INTERNAL_MCP_BUILD === 'true';
```

Code after:
if (!NODE_ENV) {
  console.error('NODE_ENV not set');
  process.exit(1);
}

const builtModulesDir = resolve(
  __dirname,
  '..',
  '..',
  'build',
  'oss-experimental',
);

const __DEV__ = NODE_ENV === 'development';

const DEVTOOLS_VERSION = getVersionString(process.env.DEVTOOLS_VERSION);

const EDITOR_URL = process.env.EDITOR_URL || null;
const LOGGING_URL = process.env.LOGGING_URL || null;

const IS_CHROME = process.env.IS_CHROME === 'true';
const IS_FIREFOX = process.env.IS_FIREFOX === 'true';
const IS_EDGE = process.env.IS_EDGE === 'true';
const IS_INTERNAL_VERSION = process.env.FEATURE_FLAG_TARGET === 'extension-fb';

const IS_INTERNAL_MCP_BUILD = process.env.IS_INTERNAL_MCP_BUILD === 'true';

const featureFlagTarget = process.env.FEATURE_FLAG_TARGET || 'extension-oss';

const babelOptions = {
  configFile: resolve(
    __dirname,
    '..',
    'react-devtools-shared',
    'babel.config.js',
  ),
};

module.exports = {
  mode: __DEV__ ? 'development' : 'production',
  devtool: false,
  entry: {
    background: './src/background/index.js',
    backendManager: './src/contentScripts/backendManager.js',
    fileFetcher: './src/contentScripts/fileFetcher.js',
    main: './src/main/index.js',
    panel: './src/panel.js',
    proxy: './src/contentScripts/proxy.js',
    prepareInjection: './src/contentScripts/prepareInjection.js',
    installHook: './src/contentScripts/installHook.js',
    hookSettingsInjector: './src/contentScripts/hookSettingsInjector.js',
Repository: facebook/react
File: packages/react-devtools-shared/src/backend/fiber/renderer.js (JavaScript)

Code before:
        if (!isLast) {
          treeString += '\n';
        }
      }
    } else {
      treeString = 'No component tree found.';
    }

    return treeString;
  }

  return {
    cleanup,
    clearErrorsAndWarnings,
    clearErrorsForElementID,
    clearWarningsForElementID,
    getSerializedElementValueByPath,
    deletePath,
    findHostInstancesForElementID,
    flushInitialOperations,
    getBestMatchForTrackedPath,
    getDisplayNameForElementID,
    getNearestMountedDOMNode,
    getElementIDForHostInstance,
    getInstanceAndStyle,
    ...(__IS_INTERNAL__ && {internal_only_getComponentTree}),
    getOwnersList,
    getPathForElement,
    getProfilingData,
    handleCommitFiberRoot,
    handleCommitFiberUnmount,
    handlePostCommitFiberRoot,
    hasElementWithId,
    inspectElement,
    logElementToConsole,
    getComponentStack,
    getElementAttributeByPath,
    getElementSourceFunctionById,
    onErrorOrWarning,
    overrideError,
    overrideSuspense,
    overrideValueAtPath,
    renamePath,
    renderer,
    setTraceUpdatesEnabled,
    setTrackedPath,
    startProfiling,
    stopProfiling,
    storeAsGlobal,
    updateComponentFilters,
    getEnvironmentNames,

Review comment (suggestion):
```suggestion
    ...(__IS_INTERNAL_MCP_BUILD__ && {internal_only_getComponentTree}),
```

Code after:
        for (let i = 0; i < rootInstances.length; i++) {
          const isLast = i === rootInstances.length - 1;
          buildTreeString(rootInstances[i], '', isLast);
          if (!isLast) {
            treeString += '\n';
          }
        }
      } else {
        treeString = 'No component tree found.';
      }

      return treeString;
    }

    internalMcpFunctions.__internal_only_getComponentTree =
      __internal_only_getComponentTree;
  }

  return {
    cleanup,
    clearErrorsAndWarnings,
    clearErrorsForElementID,
    clearWarningsForElementID,
    getSerializedElementValueByPath,
    deletePath,
    findHostInstancesForElementID,
    flushInitialOperations,
    getBestMatchForTrackedPath,
    getDisplayNameForElementID,
    getNearestMountedDOMNode,
    getElementIDForHostInstance,
    getInstanceAndStyle,
    getOwnersList,
    getPathForElement,
    getProfilingData,
    handleCommitFiberRoot,
    handleCommitFiberUnmount,
    handlePostCommitFiberRoot,
    hasElementWithId,
    inspectElement,
    logElementToConsole,
    getComponentStack,
    getElementAttributeByPath,
    getElementSourceFunctionById,
    onErrorOrWarning,
    overrideError,
    overrideSuspense,
    overrideValueAtPath,
    renamePath,
    renderer,
    setTraceUpdatesEnabled,
Repository: facebook/react
File: compiler/packages/react-mcp-server/src/tools/componentTree.ts (TypeScript)

Code before:
import puppeteer from 'puppeteer';

export async function parseReactComponentTree(url: string): Promise<string> {
  try {
    const browser = await puppeteer.connect({
      browserURL: 'http://127.0.0.1:9222',
      defaultViewport: null,
    });

    const pages = await browser.pages();

    let localhostPage = null;
    for (const page of pages) {
      const pageUrl = await page.url();

      if (pageUrl.startsWith(url)) {
        localhostPage = page;
        break;
      }
    }

    if (localhostPage) {
      const componentTree = await localhostPage.evaluate(() => {
        return (window as any).__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces
          .get(1)
          .getComponentTree();
      });

      return componentTree;
    } else {
      throw new Error('Localhost page not found');
    }
  } catch (error) {
    throw new Error('Failed extract component tree' + error);
  }
}


Review comment (suggestion):
```suggestion
          .__internal_only_getComponentTree();
```

Code after:
import puppeteer from 'puppeteer';

export async function parseReactComponentTree(url: string): Promise<string> {
  try {
    const browser = await puppeteer.connect({
      browserURL: 'http://127.0.0.1:9222',
      defaultViewport: null,
    });

    const pages = await browser.pages();

    let localhostPage = null;
    for (const page of pages) {
      const pageUrl = await page.url();

      if (pageUrl.startsWith(url)) {
        localhostPage = page;
        break;
      }
    }

    if (localhostPage) {
      const componentTree = await localhostPage.evaluate(() => {
        return (window as any).__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces
          .get(1)
          .__internal_only_getComponentTree();
      });

      return componentTree;
    } else {
      throw new Error(
        `Could not open the page at ${url}. Is your server running?`,
      );
    }
  } catch (error) {
    throw new Error('Failed extract component tree' + error);
  }
}
Repository: facebook/react
File: compiler/packages/react-mcp-server/src/tools/componentTree.ts (TypeScript)

Code before:
import puppeteer from 'puppeteer';

export async function parseReactComponentTree(url: string): Promise<string> {
  try {
    const browser = await puppeteer.connect({
      browserURL: 'http://127.0.0.1:9222',
      defaultViewport: null,
    });

    const pages = await browser.pages();

    let localhostPage = null;
    for (const page of pages) {
      const pageUrl = await page.url();

      if (pageUrl.startsWith(url)) {
        localhostPage = page;
        break;
      }
    }

    if (localhostPage) {
      const componentTree = await localhostPage.evaluate(() => {
        return (window as any).__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces
          .get(1)
          .getComponentTree();
      });

      return componentTree;
    } else {
      throw new Error('Localhost page not found');
    }
  } catch (error) {
    throw new Error('Failed extract component tree' + error);
  }
}


Review comment (suggestion):
`get(1)` will usually return you the Fiber renderer, basically the client-side renderer of React.

In case of RSC, there could also be another renderer. I am not sure about the order of registration, but it would probably be registered after the Fiber one.

For component tree, we probably care only about Fiber renderer, but worth keeping in mind that there could be rare cases where there are multiple renderers.

Code after:
import puppeteer from 'puppeteer';

export async function parseReactComponentTree(url: string): Promise<string> {
  try {
    const browser = await puppeteer.connect({
      browserURL: 'http://127.0.0.1:9222',
      defaultViewport: null,
    });

    const pages = await browser.pages();

    let localhostPage = null;
    for (const page of pages) {
      const pageUrl = await page.url();

      if (pageUrl.startsWith(url)) {
        localhostPage = page;
        break;
      }
    }

    if (localhostPage) {
      const componentTree = await localhostPage.evaluate(() => {
        return (window as any).__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces
          .get(1)
          .__internal_only_getComponentTree();
      });

      return componentTree;
    } else {
      throw new Error(
        `Could not open the page at ${url}. Is your server running?`,
      );
    }
  } catch (error) {
    throw new Error('Failed extract component tree' + error);
  }
}
Repository: facebook/react
File: compiler/packages/react-mcp-server/src/tools/componentTree.ts (TypeScript)

Code before:
      browserURL: 'http://127.0.0.1:9222',
      defaultViewport: null,
    });

    const pages = await browser.pages();

    let localhostPage = null;
    for (const page of pages) {
      const pageUrl = await page.url();

      if (pageUrl.startsWith(url)) {
        localhostPage = page;
        break;
      }
    }

    if (localhostPage) {
      const componentTree = await localhostPage.evaluate(() => {
        return (window as any).__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces
          .get(1)
          .getComponentTree();
      });

      return componentTree;
    } else {
      throw new Error('Localhost page not found');
    }
  } catch (error) {
    throw new Error('Failed extract component tree' + error);
  }
}


Review comment (suggestion):
```suggestion
      throw new Error(`Could not open the page at ${url}. Is your server running?`);
```

Code after:
      browserURL: 'http://127.0.0.1:9222',
      defaultViewport: null,
    });

    const pages = await browser.pages();

    let localhostPage = null;
    for (const page of pages) {
      const pageUrl = await page.url();

      if (pageUrl.startsWith(url)) {
        localhostPage = page;
        break;
      }
    }

    if (localhostPage) {
      const componentTree = await localhostPage.evaluate(() => {
        return (window as any).__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces
          .get(1)
          .__internal_only_getComponentTree();
      });

      return componentTree;
    } else {
      throw new Error(
        `Could not open the page at ${url}. Is your server running?`,
      );
    }
  } catch (error) {
    throw new Error('Failed extract component tree' + error);
  }
}
Repository: facebook/react
File: scripts/jest/setupTests.js (JavaScript)

Code before:
  // Dynamic version of @gate pragma
  global.gate = gateFnOrString => {
    const gateFn = coerceGateConditionToFunction(gateFnOrString);
    const flags = getTestFlags();
    return gateFn(flags);
  };

  // We augment JSDOM to produce a document that has a loading readyState by default
  // and can be changed. We mock it here globally so we don't have to import our special
  // mock in every file.
  jest.mock('jsdom', () => {
    return require('internal-test-utils/ReactJSDOM.js');
  });
}

// We mock createHook so that we can automatically clean it up.
let installedHook = null;
jest.mock('async_hooks', () => {
  const actual = jest.requireActual('async_hooks');
  return {
    ...actual,
    createHook(config) {
      if (installedHook) {
        installedHook.disable();
      }
      return (installedHook = actual.createHook(config));
    },
  };
});


Review comment (performance):
This reveals that we're actually installing two `async_hooks` when you import `server.node`. One in the Node build and one in the Node Web Streams build. 😬 Since it's a side-effect of requiring these.

Basically you have to deep link to make it optimized.

Code after:
  // Dynamic version of @gate pragma
  global.gate = gateFnOrString => {
    const gateFn = coerceGateConditionToFunction(gateFnOrString);
    const flags = getTestFlags();
    return gateFn(flags);
  };

  // We augment JSDOM to produce a document that has a loading readyState by default
  // and can be changed. We mock it here globally so we don't have to import our special
  // mock in every file.
  jest.mock('jsdom', () => {
    return require('internal-test-utils/ReactJSDOM.js');
  });
}

// We mock createHook so that we can automatically clean it up.
let installedHook = null;
let outgoingHook = null;
jest.mock('async_hooks', () => {
  const actual = jest.requireActual('async_hooks');
  return {
    ...actual,
    createHook(config) {
      // We unmount when there's more than two hooks installed.
      // We use two because the build of server.node actually installs two hooks.
      // One in each build.
      if (outgoingHook) {
        outgoingHook.disable();
      }
      outgoingHook = installedHook;
      return (installedHook = actual.createHook(config));
    },
  };
});