ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/react
File: packages/react-devtools-shared/src/frontend/utils/withPermissionsCheck.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 {PermissionNotGrantedError} from 'react-devtools-shared/src/errors/PermissionNotGrantedError';

type SupportedPermission = 'clipboardWrite';
type Permissions = Array<SupportedPermission>;
type PermissionsOptions = {permissions: Permissions};

// browser.permissions is not available for DevTools pages in Firefox
// https://bugzilla.mozilla.org/show_bug.cgi?id=1796933
// We are going to assume that requested permissions are not optional.
export function withPermissionsCheck<T: (...$ReadOnlyArray<empty>) => mixed>(
  options: PermissionsOptions,
  callback: T,
): T | (() => Promise<ReturnType<T>>) {
  if (!__IS_CHROME__ && !__IS_EDGE__) {
    return callback;
  } else {
    return async () => {
      const alreadyContains = await chrome.permissions.contains(options);
      if (alreadyContains) {
        return callback();
      }

      const granted = await chrome.permissions.request(options);
      if (granted) {
        return callback();
      }

      return Promise.reject(new PermissionNotGrantedError());
    };
  }
}


Review comment (security):
Do we need this check? I'd expect that `chrome.permissions.request` does not prompt if you already granted permissions. At least that's how I interpret "displaying a prompt to the user **if necessary**." (https://developer.chrome.com/docs/extensions/reference/api/permissions#method-request)


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 {PermissionNotGrantedError} from 'react-devtools-shared/src/errors/PermissionNotGrantedError';

type SupportedPermission = 'clipboardWrite';
type Permissions = Array<SupportedPermission>;
type PermissionsOptions = {permissions: Permissions};

// browser.permissions is not available for DevTools pages in Firefox
// https://bugzilla.mozilla.org/show_bug.cgi?id=1796933
// We are going to assume that requested permissions are not optional.
export function withPermissionsCheck<T: (...$ReadOnlyArray<empty>) => mixed>(
  options: PermissionsOptions,
  callback: T,
): T | (() => Promise<ReturnType<T>>) {
  if (!__IS_CHROME__ && !__IS_EDGE__) {
    return callback;
  } else {
    return async () => {
      const granted = await chrome.permissions.request(options);
      if (granted) {
        return callback();
      }

      return Promise.reject(new PermissionNotGrantedError());
    };
  }
}
Repository: facebook/react
File: packages/react-reconciler/src/__tests__/ReactOwnerStacks-test.js (JavaScript)

Code before:
      return (
        <div>
          <Baz />
        </div>
      );
    }
    function Baz() {
      stack = React.captureOwnerStack();
      return <span>hi</span>;
    }

    await act(() => {
      ReactNoop.render(
        <div>
          <Foo />
        </div>,
      );
    });

    expect(normalizeCodeLocInfo(stack)).toBe(
      '\n    in Bar (at **)' + '\n    in Foo (at **)',
    );
  });

  it('returns null outside of render', async () => {
    // Akward to gate since some builds will have `captureOwnerStack` return null in prod
    if (__DEV__ && gate('enableOwnerStacks')) {
      expect(React.captureOwnerStack()).toBe(null);

      await act(() => {
        ReactNoop.render(<div />);
      });

      expect(React.captureOwnerStack()).toBe(null);
    }
  });
});


Review comment (suggestion):
```suggestion
    // Awkward to gate since some builds will have `captureOwnerStack` return null in prod
```

Code after:
      return (
        <div>
          <Baz />
        </div>
      );
    }
    function Baz() {
      stack = React.captureOwnerStack();
      return <span>hi</span>;
    }

    await act(() => {
      ReactNoop.render(
        <div>
          <Foo />
        </div>,
      );
    });

    expect(normalizeCodeLocInfo(stack)).toBe(
      '\n    in Bar (at **)' + '\n    in Foo (at **)',
    );
  });

  it('returns null outside of render', async () => {
    // Awkward to gate since some builds will have `captureOwnerStack` return null in prod
    if (__DEV__ && gate('enableOwnerStacks')) {
      expect(React.captureOwnerStack()).toBe(null);

      await act(() => {
        ReactNoop.render(<div />);
      });

      expect(React.captureOwnerStack()).toBe(null);
    }
  });
});
Repository: facebook/react
File: packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js (JavaScript)

Code before:
          'instance. First attach the observer with observeUsing()',
      );
    }
  } else {
    this._observers.delete(observer);
    traverseFragmentInstance(this._fragmentFiber, unobserveChild, observer);
  }
};
function unobserveChild(
  child: Instance,
  observer: IntersectionObserver | ResizeObserver,
) {
  observer.unobserve(child);
  return false;
}
// $FlowFixMe[prop-missing]
FragmentInstance.prototype.getClientRects = function (
  this: FragmentInstanceType,
): ChildRectLists {
  const rects: ChildRectLists = [];
  traverseFragmentInstance(this._fragmentFiber, collectClientRects, rects);
  return rects;
};
function collectClientRects(child: Instance, rects: ChildRectLists): boolean {
  // $FlowFixMe[incompatible-call]
  rects.push(child.getClientRects());
  return false;
}

function normalizeListenerOptions(
  opts: ?EventListenerOptionsOrUseCapture,
): string {
  if (opts == null) {
    return '0';
  }

  if (typeof opts === 'boolean') {
    return `c=${opts ? '1' : '0'}`;
  }

  return `c=${opts.capture ? '1' : '0'}&o=${opts.once ? '1' : '0'}&p=${opts.passive ? '1' : '0'}`;
}

function indexOfEventListener(
  eventListeners: Array<StoredEventListener>,
  type: string,
  listener: EventListener,
  optionsOrUseCapture: void | EventListenerOptionsOrUseCapture,
): number {
  for (let i = 0; i < eventListeners.length; i++) {
    const item = eventListeners[i];

Review comment (suggestion):
```suggestion
  rects.push.apply(rects, child.getClientRects());
```

Code after:
  return false;
}
// $FlowFixMe[prop-missing]
FragmentInstance.prototype.observeUsing = function (
  this: FragmentInstanceType,
  observer: IntersectionObserver | ResizeObserver,
): void {
  if (this._observers === null) {
    this._observers = new Set();
  }
  this._observers.add(observer);
  traverseFragmentInstance(this._fragmentFiber, observeChild, observer);
};
function observeChild(
  child: Instance,
  observer: IntersectionObserver | ResizeObserver,
) {
  observer.observe(child);
  return false;
}
// $FlowFixMe[prop-missing]
FragmentInstance.prototype.unobserveUsing = function (
  this: FragmentInstanceType,
  observer: IntersectionObserver | ResizeObserver,
): void {
  if (this._observers === null || !this._observers.has(observer)) {
    if (__DEV__) {
      console.error(
        'You are calling unobserveUsing() with an observer that is not being observed with this fragment ' +
          'instance. First attach the observer with observeUsing()',
      );
    }
  } else {
    this._observers.delete(observer);
    traverseFragmentInstance(this._fragmentFiber, unobserveChild, observer);
  }
};
function unobserveChild(
  child: Instance,
  observer: IntersectionObserver | ResizeObserver,
) {
  observer.unobserve(child);
  return false;
}
// $FlowFixMe[prop-missing]
FragmentInstance.prototype.getClientRects = function (
  this: FragmentInstanceType,
): Array<DOMRect> {
  const rects: Array<DOMRect> = [];
  traverseFragmentInstance(this._fragmentFiber, collectClientRects, rects);
  return rects;
Repository: facebook/react
File: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateStaticComponents.ts (TypeScript)

Code before:
              value.tag.identifier.id,
            );
            if (location != null) {
              error.push({
                reason: `Components created during render will reset their state each time they are created. Declare components outside of render. `,
                severity: ErrorSeverity.InvalidReact,
                loc: value.tag.loc,
                description: null,
                suggestions: null,
              });
              error.push({
                reason: `The component may be created during render`,
                severity: ErrorSeverity.InvalidReact,
                loc: location,
                description: null,
                suggestions: null,
              });
            }
          }
        }
      }
    }
  }
  if (error.hasErrors()) {
    throw error;
  }
}


Review comment (suggestion):
Hmm thoughts on just logging this error instead of throwing? Most of our validations have to do with the memoization feature of the compiler, so throwing makes sense for no-ref-access-in-render, hooks-usage, and no-impure-functions-during-render. 

For validation like this pass and `ValidateNoJSXInTryStatement` though, could we just report diagnostics via the logger without bailing out? In the longer term, we can mark some validation passes as required for memo and some as needing to only run for the eslint plugin
```js
fn.env.logger?.logEvent(<file name>, err);
```

Code after:
        case 'JsxExpression': {
          if (value.tag.kind === 'Identifier') {
            const location = knownDynamicComponents.get(
              value.tag.identifier.id,
            );
            if (location != null) {
              error.push({
                reason: `Components created during render will reset their state each time they are created. Declare components outside of render. `,
                severity: ErrorSeverity.InvalidReact,
                loc: value.tag.loc,
                description: null,
                suggestions: null,
              });
              error.push({
                reason: `The component may be created during render`,
                severity: ErrorSeverity.InvalidReact,
                loc: location,
                description: null,
                suggestions: null,
              });
            }
          }
        }
      }
    }
  }
  if (error.hasErrors()) {
    return Err(error);
  } else {
    return Ok(undefined);
  }
}
Repository: facebook/react
File: compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts (TypeScript)

Code before:
      kind: 'BinaryExpression';
      operator: Exclude<t.BinaryExpression['operator'], '|>'>;
      left: Place;
      right: Place;
      loc: SourceLocation;
    }
  | NewExpression
  | CallExpression
  | MethodCall
  | {
      kind: 'UnaryExpression';
      operator: Exclude<t.UnaryExpression['operator'], 'throw' | 'delete'>;
      value: Place;
      loc: SourceLocation;
    }
  | {
      kind: 'TypeCastExpression';
      value: Place;
      typeAnnotation: t.FlowType | t.TSType;
      type: Type;
      loc: SourceLocation;
    }
  | JsxExpression
  | {
      kind: 'ObjectExpression';
      properties: Array<ObjectProperty | SpreadPattern>;
      loc: SourceLocation;
    }
  | ObjectMethod
  | ArrayExpression
  | {kind: 'JsxFragment'; children: Array<Place>; loc: SourceLocation}
  | {
      kind: 'RegExpLiteral';
      pattern: string;
      flags: string;
      loc: SourceLocation;
    }
  | {
      kind: 'MetaProperty';
      meta: string;
      property: string;
      loc: SourceLocation;
    }

  // store `object.property = value`
  | {
      kind: 'PropertyStore';
      object: Place;
      property: PropertyLiteral;
      value: Place;
      loc: SourceLocation;

Review comment (suggestion):
I tried to make it so that it wouldn't be possible to have a 'satisfies' annotation kind when the annotation was `t.FlowType`.
Hence why I opted for this slightly more complex union FYI @mofeiZ 

Code after:
      kind: 'BinaryExpression';
      operator: Exclude<t.BinaryExpression['operator'], '|>'>;
      left: Place;
      right: Place;
      loc: SourceLocation;
    }
  | NewExpression
  | CallExpression
  | MethodCall
  | {
      kind: 'UnaryExpression';
      operator: Exclude<t.UnaryExpression['operator'], 'throw' | 'delete'>;
      value: Place;
      loc: SourceLocation;
    }
  | ({
      kind: 'TypeCastExpression';
      value: Place;
      type: Type;
      loc: SourceLocation;
    } & (
      | {
          typeAnnotation: t.FlowType;
          typeAnnotationKind: 'cast';
        }
      | {
          typeAnnotation: t.TSType;
          typeAnnotationKind: 'as' | 'satisfies';
        }
    ))
  | JsxExpression
  | {
      kind: 'ObjectExpression';
      properties: Array<ObjectProperty | SpreadPattern>;
      loc: SourceLocation;
    }
  | ObjectMethod
  | ArrayExpression
  | {kind: 'JsxFragment'; children: Array<Place>; loc: SourceLocation}
  | {
      kind: 'RegExpLiteral';
      pattern: string;
      flags: string;
      loc: SourceLocation;
    }
  | {
      kind: 'MetaProperty';
      meta: string;
      property: string;
      loc: SourceLocation;
    }
Repository: facebook/react
File: packages/react-server/src/ReactFlightServer.js (JavaScript)

Code before:
        error = new Error('The render was aborted due to being postponed.');
      } else {
        error =
          reason === undefined
            ? new Error(
                'The render was aborted by the server without a reason.',
              )
            : typeof reason === 'object' &&
                reason !== null &&
                typeof reason.then === 'function'
              ? new Error(
                  'The render was aborted by the server with a promise.',
                )
              : reason;
      }
      abortListeners.forEach(callback => callback(error));
      abortListeners.clear();
    }
    if (request.destination !== null) {
      flushCompletedChunks(request, request.destination);
    }
  } catch (error) {
    logRecoverableError(request, error, null);
    fatalError(request, error);
  } finally {
    callOnAllReadyIfReady(request);
  }
}


Review comment (bug):
If we fatal error, we shouldn't also call on onAllReady. In the case of a Promise the native implementation of promises might dedupe the second one (but really that should probably be a warning) but for example the Node.js callback model doesn't do this which would lead to incorrect results for the consumer.

Additionally, if the onAllReady callback itself throws we should trigger the fatal error flow to clean up. So this should be inside the try.

Code after:
        // We aborted with a Postpone but since we're passing this to an
        // external handler, passing this object would leak it outside React.
        // We create an alternative reason for it instead.
        error = new Error('The render was aborted due to being postponed.');
      } else {
        error =
          reason === undefined
            ? new Error(
                'The render was aborted by the server without a reason.',
              )
            : typeof reason === 'object' &&
                reason !== null &&
                typeof reason.then === 'function'
              ? new Error(
                  'The render was aborted by the server with a promise.',
                )
              : reason;
      }
      abortListeners.forEach(callback => callback(error));
      abortListeners.clear();
      callOnAllReadyIfReady(request);
    }
    if (request.destination !== null) {
      flushCompletedChunks(request, request.destination);
    }
  } catch (error) {
    logRecoverableError(request, error, null);
    fatalError(request, error);
  }
}
Repository: facebook/react
File: packages/react-server/src/ReactFlightServer.js (JavaScript)

Code before:
      ) {
        // We aborted with a Postpone but since we're passing this to an
        // external handler, passing this object would leak it outside React.
        // We create an alternative reason for it instead.
        error = new Error('The render was aborted due to being postponed.');
      } else {
        error =
          reason === undefined
            ? new Error(
                'The render was aborted by the server without a reason.',
              )
            : typeof reason === 'object' &&
                reason !== null &&
                typeof reason.then === 'function'
              ? new Error(
                  'The render was aborted by the server with a promise.',
                )
              : reason;
      }
      abortListeners.forEach(callback => callback(error));
      abortListeners.clear();
    }
    if (request.destination !== null) {
      flushCompletedChunks(request, request.destination);
    }
    callOnAllReadyIfReady(request);
  } catch (error) {
    logRecoverableError(request, error, null);
    fatalError(request, error);
  }
}


Review comment (refactor):
It seems better to duplicate this right after each `clear()` so that it's associated with the mutation that might have unblocked it and if there's ever a path that doesn't do a mutation then we don't have to check it in that branch.

Code after:
        typeof reason === 'object' &&
        reason !== null &&
        (reason: any).$$typeof === REACT_POSTPONE_TYPE
      ) {
        // We aborted with a Postpone but since we're passing this to an
        // external handler, passing this object would leak it outside React.
        // We create an alternative reason for it instead.
        error = new Error('The render was aborted due to being postponed.');
      } else {
        error =
          reason === undefined
            ? new Error(
                'The render was aborted by the server without a reason.',
              )
            : typeof reason === 'object' &&
                reason !== null &&
                typeof reason.then === 'function'
              ? new Error(
                  'The render was aborted by the server with a promise.',
                )
              : reason;
      }
      abortListeners.forEach(callback => callback(error));
      abortListeners.clear();
      callOnAllReadyIfReady(request);
    }
    if (request.destination !== null) {
      flushCompletedChunks(request, request.destination);
    }
  } catch (error) {
    logRecoverableError(request, error, null);
    fatalError(request, error);
  }
}
Repository: facebook/react
File: packages/react-devtools-shared/src/hydration.js (JavaScript)

Code before:
            type: type,
            preview_short: formatDataForPreview(data, false),
            preview_long: formatDataForPreview(data, true),
            name: 'fulfilled Promise',
          };

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

          unserializable.push(path);

          return unserializableValue;
        }
        case 'rejected': {
          const unserializableValue: Unserializable = {
            unserializable: true,
            type: type,
            preview_short: formatDataForPreview(data, false),
            preview_long: formatDataForPreview(data, true),
            name: 'fulfilled Promise',
          };

          unserializableValue.reason = dehydrate(
            data.reason,
            cleaned,
            unserializable,
            path.concat(['reason']),
            isPathAllowed,
            isPathAllowedCheck ? 1 : level + 1,
          );

          unserializable.push(path);

          return unserializableValue;
        }
        default:
          cleaned.push(path);
          return {
            inspectable: false,
            preview_short: formatDataForPreview(data, false),
            preview_long: formatDataForPreview(data, true),
            name: data.toString(),
            type,
          };
      }

Review comment (suggestion):
```suggestion
            name: 'rejected Promise',
```

Code after:
            type: type,
            preview_short: formatDataForPreview(data, false),
            preview_long: formatDataForPreview(data, true),
            name: 'fulfilled Thenable',
          };

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

          unserializable.push(path);

          return unserializableValue;
        }
        case 'rejected': {
          const unserializableValue: Unserializable = {
            unserializable: true,
            type: type,
            preview_short: formatDataForPreview(data, false),
            preview_long: formatDataForPreview(data, true),
            name: 'rejected Thenable',
          };

          unserializableValue.reason = dehydrate(
            data.reason,
            cleaned,
            unserializable,
            path.concat(['reason']),
            isPathAllowed,
            isPathAllowedCheck ? 1 : level + 1,
          );

          unserializable.push(path);

          return unserializableValue;
        }
        default:
          cleaned.push(path);
          return {
            inspectable: false,
            preview_short: formatDataForPreview(data, false),
            preview_long: formatDataForPreview(data, true),
            name: data.toString(),
            type,
          };
      }
Repository: facebook/react
File: compiler/packages/react-mcp-server/src/index.ts (TypeScript)

Code before:

const server = new McpServer({
  name: 'React',
  version: '0.0.0',
});

server.tool(
  'docs',
  'Search docs from react.dev',
  {
    query: z.string(),
  },
  async ({query}) => {
    const pages = await queryAlgolia(query);
    const content = pages.map(html => {
      const $ = cheerio.load(html);
      // react.dev should always have at least one <article> with the main content
      const article = $('article').html();
      if (article != null) {
        return {
          type: 'text' as const,
          text: convert(article),
        };
      } else {
        return {
          type: 'text' as const,
          // Fallback to converting the whole page to text.
          text: convert($.html()),
        };
      }
    });

    return {
      content,
    };
  },
);

server.tool(
  'compile',
  'Compile code with React Compiler. Optionally, for debugging provide a pass name like "HIR" to see more information.',
  {
    text: z.string(),
    passName: z.enum(['HIR', 'ReactiveFunction', 'All', '@DEBUG']).optional(),
  },
  async ({text, passName}) => {
    const pipelinePasses = new Map<
      string,
      Array<PrintedCompilerPipelineValue>
    >();
    const recordPass: (

Review comment (bug):
I wonder if this is the best way to handle \<article\> not being present.

I'm assuming \<article\> not being there means the tool failed to get the correct page/page failed to load

Code after:

const server = new McpServer({
  name: 'React',
  version: '0.0.0',
});

server.tool(
  'query-react-dev-docs',
  'Search/look up official docs from react.dev',
  {
    query: z.string(),
  },
  async ({query}) => {
    try {
      const pages = await queryAlgolia(query);
      if (pages.length === 0) {
        return {
          content: [{type: 'text' as const, text: `No results`}],
        };
      }
      const content = pages.map(html => {
        const $ = cheerio.load(html);
        // react.dev should always have at least one <article> with the main content
        const article = $('article').html();
        if (article != null) {
          return {
            type: 'text' as const,
            text: convert(article),
          };
        } else {
          return {
            type: 'text' as const,
            // Fallback to converting the whole page to text.
            text: convert($.html()),
          };
        }
      });
      return {
        content,
      };
    } catch (err) {
      return {
        isError: true,
        content: [{type: 'text' as const, text: `Error: ${err.stack}`}],
      };
    }
  },
);

server.tool(
  'compile',