ronantakizawa/codereview-bench huggingface dataset:
Repository: mozilla/pdf.js
File: web/comment_manager.js (JavaScript)

Code before:
    this.#setPosition(x, y, /* isInitial */ true);

    await this.#overlayManager.open(this.#dialog);
  }

  async #save() {
    this.#currentEditor.comment = this.#textInput.value;
    this.#finish();
  }

  get _dialogWidth() {
    const dialog = this.#dialog;
    const { style } = dialog;
    style.opacity = "0";
    style.display = "block";
    const width = dialog.getBoundingClientRect().width;
    style.opacity = style.display = "";
    return shadow(this, "_dialogWidth", width);
  }

  #lightenColor(color) {
    if (!color) {
      return null; // No color provided.
    }
    const [r, g, b] = getRGB(color);
    return changeLightness(r, g, b, l => (1 + Math.sqrt(l)) / 2);
  }

  #setText(text) {
    const textView = this.#textView;
    for (const line of text.split("\n")) {
      const span = document.createElement("span");
      span.textContent = line;
      textView.append(span, document.createElement("br"));
    }
  }

  #setPosition(x, y, isInitial = false) {
    this.#dialogX = x;
    this.#dialogY = y;
    const { style } = this.#dialog;
    style.left = `${x}px`;
    style.top = isInitial
      ? `calc(${y}px + var(--editor-toolbar-vert-offset))`
      : `${y}px`;
  }

  #edit() {
    const textInput = this.#textInput;
    const textView = this.#textView;
    if (textView.childElementCount > 0) {

Review comment (suggestion):
The `l` parameter can be removed here since it's the default value now.

Code after:
    this.#setPosition(x, y, /* isInitial */ true);

    await this.#overlayManager.open(this.#dialog);
  }

  async #save() {
    this.#currentEditor.comment = this.#textInput.value;
    this.#finish();
  }

  get _dialogWidth() {
    const dialog = this.#dialog;
    const { style } = dialog;
    style.opacity = "0";
    style.display = "block";
    const width = dialog.getBoundingClientRect().width;
    style.opacity = style.display = "";
    return shadow(this, "_dialogWidth", width);
  }

  #lightenColor(color) {
    if (!color) {
      return null; // No color provided.
    }
    const [r, g, b] = getRGB(color);
    return changeLightness(r, g, b);
  }

  #setText(text) {
    const textView = this.#textView;
    for (const line of text.split("\n")) {
      const span = document.createElement("span");
      span.textContent = line;
      textView.append(span, document.createElement("br"));
    }
  }

  #setPosition(x, y, isInitial = false) {
    this.#dialogX = x;
    this.#dialogY = y;
    const { style } = this.#dialog;
    style.left = `${x}px`;
    style.top = isInitial
      ? `calc(${y}px + var(--editor-toolbar-vert-offset))`
      : `${y}px`;
  }

  #edit() {
    const textInput = this.#textInput;
    const textView = this.#textView;
    if (textView.childElementCount > 0) {
Repository: mozilla/pdf.js
File: src/display/editor/tools.js (JavaScript)

Code before:

  static #pointerType = null;

  static #pointerIds = null;

  static #moveTimestamp = NaN;

  static initializeAndAddPointerId(pointerId) {
    (CurrentPointer.#pointerIds ||= new Set()).add(pointerId);
  }

  static setPointer(pointerType, pointerId) {
    CurrentPointer.#pointerId ||= pointerId;
    CurrentPointer.#pointerType ??= pointerType;
  }

  static setTimeStamp(timeStamp) {
    CurrentPointer.#moveTimestamp = timeStamp;
  }

  static isSamePointerId(pointerId) {
    if (CurrentPointer.#pointerId === pointerId) {
      return true;
    }

    CurrentPointer.#pointerIds?.delete(pointerId);
    return false;
  }

  static isPointerType() {
    return CurrentPointer.#pointerType !== null;
  }

  static isSamePointerType(pointerType) {
    return CurrentPointer.#pointerType === pointerType;
  }

  static isInitializedAndDifferentPointerType(pointerType) {
    return (
      CurrentPointer.isPointerType() &&
      !CurrentPointer.isSamePointerType(pointerType)
    );
  }

  static isSameTimeStamp(timeStamp) {
    return CurrentPointer.#moveTimestamp === timeStamp;
  }

  static isUsingMultiplePointers() {
    return CurrentPointer.#pointerIds?.size >= 1;
  }

Review comment (suggestion):
It's strange to delete something in a function which is supposed to be a simple checker.
The `delete` is because originally the pointer was up or cancel which means that this pointer is no more active, consequently I'm not sure it makes sense to use this function here:
https://github.com/mozilla/pdf.js/pull/20213/files#diff-02919db67aef2f378464ba4c97e5f59e1e114498a147113025cd4163287f25f4R802

Code after:

  // The pointerId  and pointerIds are used to keep track of
  // the pointers with a same type (e.g. two fingers).
  static #pointerId = NaN;

  static #pointerIds = null;

  // Track the timestamp to know if the touchmove event is used.
  static #moveTimestamp = NaN;

  // The pointerType is used to know if we are using a mouse, a pen or a touch.
  static #pointerType = null;

  static initializeAndAddPointerId(pointerId) {
    // Store pointer ids. For example, the user is using a second finger.
    (CurrentPointers.#pointerIds ||= new Set()).add(pointerId);
  }

  static setPointer(pointerType, pointerId) {
    CurrentPointers.#pointerId ||= pointerId;
    CurrentPointers.#pointerType ??= pointerType;
  }

  static setTimeStamp(timeStamp) {
    CurrentPointers.#moveTimestamp = timeStamp;
  }

  static isSamePointerId(pointerId) {
    return CurrentPointers.#pointerId === pointerId;
  }

  // Check if it's the same pointer id, otherwise remove it from the set.
  static isSamePointerIdOrRemove(pointerId) {
    if (CurrentPointers.#pointerId === pointerId) {
      return true;
    }

    CurrentPointers.#pointerIds?.delete(pointerId);
    return false;
  }

  static isSamePointerType(pointerType) {
    return CurrentPointers.#pointerType === pointerType;
  }

  static isInitializedAndDifferentPointerType(pointerType) {
    return (
      CurrentPointers.#pointerType !== null &&
      !CurrentPointers.isSamePointerType(pointerType)
    );
  }
Repository: mozilla/pdf.js
File: src/display/editor/tools.js (JavaScript)

Code before:
  AnnotationEditorParamsType,
  AnnotationEditorPrefix,
  AnnotationEditorType,
  FeatureTest,
  getUuid,
  shadow,
  Util,
  warn,
} from "../../shared/util.js";
import {
  fetchData,
  getColorValues,
  getRGB,
  PixelsPerInch,
  stopEvent,
} from "../display_utils.js";
import { FloatingToolbar } from "./toolbar.js";

function bindEvents(obj, element, names) {
  for (const name of names) {
    element.addEventListener(name, obj[name].bind(obj));
  }
}

/**
 * Class to store current pointer used by the editor.
 */
class CurrentPointer {
  // To manage the pointer events.
  static #pointerId = NaN;

  static #pointerType = null;

  static #pointerIds = null;

  static #moveTimestamp = NaN;

  static initializeAndAddPointerId(pointerId) {
    (CurrentPointer.#pointerIds ||= new Set()).add(pointerId);
  }

  static setPointer(pointerType, pointerId) {
    CurrentPointer.#pointerId ||= pointerId;
    CurrentPointer.#pointerType ??= pointerType;
  }

  static setTimeStamp(timeStamp) {
    CurrentPointer.#moveTimestamp = timeStamp;
  }

  static isSamePointerId(pointerId) {

Review comment (suggestion):
It isn't exactly correct: the idea is more about handling multiple pointers (fingers, pen, mouse, ...)

Code after:
  AnnotationEditorParamsType,
  AnnotationEditorPrefix,
  AnnotationEditorType,
  FeatureTest,
  getUuid,
  shadow,
  Util,
  warn,
} from "../../shared/util.js";
import {
  fetchData,
  getColorValues,
  getRGB,
  PixelsPerInch,
  stopEvent,
} from "../display_utils.js";
import { FloatingToolbar } from "./toolbar.js";

function bindEvents(obj, element, names) {
  for (const name of names) {
    element.addEventListener(name, obj[name].bind(obj));
  }
}

/**
 * Class to store current pointers used by the editor to be able to handle
 * multiple pointers (e.g. two fingers, a pen, a mouse, ...).
 */
class CurrentPointers {
  // To manage the pointer events.

  // The pointerId  and pointerIds are used to keep track of
  // the pointers with a same type (e.g. two fingers).
  static #pointerId = NaN;

  static #pointerIds = null;

  // Track the timestamp to know if the touchmove event is used.
  static #moveTimestamp = NaN;

  // The pointerType is used to know if we are using a mouse, a pen or a touch.
  static #pointerType = null;

  static initializeAndAddPointerId(pointerId) {
    // Store pointer ids. For example, the user is using a second finger.
    (CurrentPointers.#pointerIds ||= new Set()).add(pointerId);
  }

  static setPointer(pointerType, pointerId) {
    CurrentPointers.#pointerId ||= pointerId;
    CurrentPointers.#pointerType ??= pointerType;
Repository: mozilla/pdf.js
File: src/display/editor/tools.js (JavaScript)

Code before:
    if (this.#isEnabled) {
      layer.enable();
    } else {
      layer.disable();
    }
  }

  /**
   * Remove a layer.
   * @param {AnnotationEditorLayer} layer
   */
  removeLayer(layer) {
    this.#allLayers.delete(layer.pageIndex);
  }

  /**
   * Change the editor mode (None, FreeText, Ink, ...)
   * @param {number} mode
   * @param {string|null} editId
   * @param {boolean} [isFromKeyboard] - true if the mode change is due to a
   *   keyboard action.
   * @param {boolean} [mustEnterInEditMode] - true if the editor must enter in
   *   edit mode.
   * @param {boolean} [editComment] - true if the mode change is due to a
   *   comment edit.
   * @param {boolean} [isFromEvent] - true if the mode change is due to an event
   *   (e.g. toolbar button clicked).
   */
  async updateMode(
    mode,
    editId = null,
    isFromKeyboard = false,
    mustEnterInEditMode = false,
    editComment = false,
    isFromEvent = false
  ) {
    if (this.#mode === mode) {
      return;
    }

    if (this.#updateModeCapability) {
      await this.#updateModeCapability.promise;
      if (!this.#updateModeCapability) {
        // This ui manager has been destroyed.
        return;
      }
    }

    this.#updateModeCapability = Promise.withResolvers();
    this.#currentDrawingSession?.commitOrRemove();


Review comment (bug):
Sorry but this change should be in an other patch.
I've the feeling you're fixing a bug and if it's the case we should have an integration test for it.

Code after:

    if (hasChanged) {
      this._eventBus.dispatch("annotationeditorstateschanged", {
        source: this,
        details: Object.assign(this.#previousStates, details),
      });
      // We could listen on our own event but it sounds like a bit weird and
      // it's a way to simpler to handle that stuff here instead of having to
      // add something in every place where an editor can be unselected.
      if (
        this.#mode === AnnotationEditorType.HIGHLIGHT &&
        details.hasSelectedEditor === false
      ) {
        this.#dispatchUpdateUI([
          [AnnotationEditorParamsType.HIGHLIGHT_FREE, true],
        ]);
      }
    }
  }

  #dispatchUpdateUI(details) {
    this._eventBus.dispatch("annotationeditorparamschanged", {
      source: this,
      details,
    });
  }

  /**
   * Set the editing state.
   * It can be useful to temporarily disable it when the user is editing a
   * FreeText annotation.
   * @param {boolean} isEditing
   */
  setEditingState(isEditing) {
    if (isEditing) {
      this.#addFocusManager();
      this.#addCopyPasteListeners();
      this.#dispatchUpdateStates({
        isEditing: this.#mode !== AnnotationEditorType.NONE,
        isEmpty: this.#isEmpty(),
        hasSomethingToUndo: this.#commandManager.hasSomethingToUndo(),
        hasSomethingToRedo: this.#commandManager.hasSomethingToRedo(),
        hasSelectedEditor: false,
      });
    } else {
      this.#removeFocusManager();
      this.#removeCopyPasteListeners();
      this.#dispatchUpdateStates({
        isEditing: false,
      });
      this.disableUserSelect(false);
Repository: mozilla/pdf.js
File: src/display/editor/tools.js (JavaScript)

Code before:
  /**
   * Add a new layer for a page which will contains the editors.
   * @param {AnnotationEditorLayer} layer
   */
  addLayer(layer) {
    this.#allLayers.set(layer.pageIndex, layer);
    if (this.#isEnabled) {
      layer.enable();
    } else {
      layer.disable();
    }
  }

  /**
   * Remove a layer.
   * @param {AnnotationEditorLayer} layer
   */
  removeLayer(layer) {
    this.#allLayers.delete(layer.pageIndex);
  }

  /**
   * Change the editor mode (None, FreeText, Ink, ...)
   * @param {number} mode
   * @param {boolean} [isFromUser] - true if the mode change is due to a
   *   user action.
   * @param {string|null} editId
   * @param {boolean} [isFromKeyboard] - true if the mode change is due to a
   *   keyboard action.
   * @param {boolean} [mustEnterInEditMode] - true if the editor must enter in
   *   edit mode.
   * @param {boolean} [editComment] - true if the mode change is due to a
   *   comment edit.
   */
  async updateMode(
    mode,
    isFromUser = false,
    editId = null,
    isFromKeyboard = false,
    mustEnterInEditMode = false,
    editComment = false
  ) {
    if (this.#mode === mode) {
      return;
    }

    if (this.#updateModeCapability) {
      await this.#updateModeCapability.promise;
      if (!this.#updateModeCapability) {
        // This ui manager has been destroyed.
        return;

Review comment (suggestion):
Required arguments should be listed before optional ones, so please swap `isFromUser` with `editId` here and in the function definition below.

Code after:
  /**
   * Add a new layer for a page which will contains the editors.
   * @param {AnnotationEditorLayer} layer
   */
  addLayer(layer) {
    this.#allLayers.set(layer.pageIndex, layer);
    if (this.#isEnabled) {
      layer.enable();
    } else {
      layer.disable();
    }
  }

  /**
   * Remove a layer.
   * @param {AnnotationEditorLayer} layer
   */
  removeLayer(layer) {
    this.#allLayers.delete(layer.pageIndex);
  }

  /**
   * Change the editor mode (None, FreeText, Ink, ...)
   * @param {number} mode
   * @param {string|null} editId
   * @param {boolean} [isFromUser] - true if the mode change is due to a
   *   user action.
   * @param {boolean} [isFromKeyboard] - true if the mode change is due to a
   *   keyboard action.
   * @param {boolean} [mustEnterInEditMode] - true if the editor must enter in
   *   edit mode.
   * @param {boolean} [editComment] - true if the mode change is due to a
   *   comment edit.
   */
  async updateMode(
    mode,
    editId = null,
    isFromUser = false,
    isFromKeyboard = false,
    mustEnterInEditMode = false,
    editComment = false
  ) {
    if (this.#mode === mode) {
      return;
    }

    if (this.#updateModeCapability) {
      await this.#updateModeCapability.promise;
      if (!this.#updateModeCapability) {
        // This ui manager has been destroyed.
        return;
Repository: mozilla/pdf.js
File: test/integration/comment_spec.mjs (JavaScript)

Code before:
import {
  awaitPromise,
  closePages,
  createPromise,
  dragAndDrop,
  getEditorSelector,
  getRect,
  getSpanRectFromText,
  loadAndWait,
  scrollIntoView,
  selectEditor,
  switchToEditor,
  waitAndClick,
  waitForSerialized,
} from "./test_utils.mjs";

const switchToHighlight = switchToEditor.bind(null, "Highlight");
const switchToStamp = switchToEditor.bind(null, "Stamp");
const switchToComment = switchToEditor.bind(null, "Comment");

describe("Comment", () => {
  describe("Comment edit dialog must be visible in ltr", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait(
        "bug1989304.pdf",
        ".annotationEditorLayer",
        "page-width",
        null,
        { enableComment: true }
      );
    });

    afterEach(async () => {
      await closePages(pages);
    });

    it("must set the comment dialog in the viewport (LTR)", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await switchToHighlight(page);

          await scrollIntoView(page, ".textLayer span:last-of-type");
          const rect = await getSpanRectFromText(page, 1, "...");
          const x = rect.x + rect.width / 2;
          const y = rect.y + rect.height / 2;
          // Here and elsewhere, we add a small delay between press and release
          // to make sure that a pointerup event is triggered after
          // selectionchange.
          // It works with a value of 1ms, but we use 100ms to be sure.

Review comment (bug):
Aren't we potentially waiting for the wrong selector here, given that where it's used we seem to be interested in editor selector 9 as being the highlight editor?

It may also be useful to use a helper like this in more places.

Code after:
import {
  awaitPromise,
  closePages,
  createPromise,
  dragAndDrop,
  getEditorSelector,
  getRect,
  getSpanRectFromText,
  loadAndWait,
  scrollIntoView,
  selectEditor,
  switchToEditor,
  waitAndClick,
  waitForSerialized,
} from "./test_utils.mjs";

const switchToHighlight = switchToEditor.bind(null, "Highlight");
const switchToStamp = switchToEditor.bind(null, "Stamp");
const switchToComment = switchToEditor.bind(null, "Comment");

const highlightSpan = async (page, pageIndex, text) => {
  const rect = await getSpanRectFromText(page, pageIndex, text);
  const x = rect.x + rect.width / 2;
  const y = rect.y + rect.height / 2;
  await page.mouse.click(x, y, { count: 2, delay: 100 });
  await page.waitForSelector(getEditorSelector(0));
};

describe("Comment", () => {
  describe("Comment edit dialog must be visible in ltr", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait(
        "bug1989304.pdf",
        ".annotationEditorLayer",
        "page-width",
        null,
        { enableComment: true }
      );
    });

    afterEach(async () => {
      await closePages(pages);
    });

    it("must set the comment dialog in the viewport (LTR)", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await switchToHighlight(page);
Repository: mozilla/pdf.js
File: src/core/struct_tree.js (JavaScript)

Code before:
const MAX_DEPTH = 40;

const StructElementType = {
  PAGE_CONTENT: 1,
  STREAM_CONTENT: 2,
  OBJECT: 3,
  ANNOTATION: 4,
  ELEMENT: 5,
};

class StructTreeRoot {
  constructor(xref, rootDict, rootRef) {
    this.xref = xref;
    this.dict = rootDict;
    this.ref = rootRef instanceof Ref ? rootRef : null;
    this.roleMap = new Map();
    this.structParentIds = null;
  }

  init() {
    this.readRoleMap();
  }

  #addIdToPage(pageRef, id, type) {
    if (!(pageRef instanceof Ref) || id < 0) {
      return;
    }
    this.structParentIds ||= new RefSetCache();
    let ids = this.structParentIds.get(pageRef);
    if (!ids) {
      ids = [];
      this.structParentIds.put(pageRef, ids);
    }
    ids.push([id, type]);
  }

  addAnnotationIdToPage(pageRef, id) {
    this.#addIdToPage(pageRef, id, StructElementType.ANNOTATION);
  }

  readRoleMap() {
    const roleMapDict = this.dict.get("RoleMap");
    if (!(roleMapDict instanceof Dict)) {
      return;
    }
    for (const [key, value] of roleMapDict) {
      if (value instanceof Name) {
        this.roleMap.set(key, value.name);
      }
    }
  }

Review comment (refactor):
Is it possible to simplify this using `for (const ref of obj) {` so we don't need a loop iterator variable and indexing on the line below?

Code after:
const MAX_DEPTH = 40;

const StructElementType = {
  PAGE_CONTENT: 1,
  STREAM_CONTENT: 2,
  OBJECT: 3,
  ANNOTATION: 4,
  ELEMENT: 5,
};

class StructTreeRoot {
  constructor(xref, rootDict, rootRef) {
    this.xref = xref;
    this.dict = rootDict;
    this.ref = rootRef instanceof Ref ? rootRef : null;
    this.roleMap = new Map();
    this.structParentIds = null;
    this.kidRefToPosition = undefined;
  }

  getKidPosition(kidRef) {
    if (this.kidRefToPosition === undefined) {
      const obj = this.dict.get("K");
      if (Array.isArray(obj)) {
        const map = (this.kidRefToPosition = new Map());
        for (let i = 0, ii = obj.length; i < ii; i++) {
          const ref = obj[i];
          if (ref) {
            map.set(ref.toString(), i);
          }
        }
      } else if (obj instanceof Dict) {
        this.kidRefToPosition = new Map([[obj.objId, 0]]);
      } else if (!obj) {
        this.kidRefToPosition = new Map();
      } else {
        this.kidRefToPosition = null;
      }
    }
    return this.kidRefToPosition
      ? (this.kidRefToPosition.get(kidRef) ?? NaN)
      : -1;
  }

  init() {
    this.readRoleMap();
  }

  #addIdToPage(pageRef, id, type) {
    if (!(pageRef instanceof Ref) || id < 0) {
      return;
Repository: mozilla/pdf.js
File: src/core/intersector.js (JavaScript)

Code before:
/* Copyright 2025 Mozilla Foundation
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

class SingleIntersector {
  #annotation;

  _minX = Infinity;

  _minY = Infinity;

  _maxX = -Infinity;

  _maxY = -Infinity;

  #quadPoints = null;

  #text = [];

  #extraChars = [];

  #lastIntersectingQuadIndex = -1;

  #canTakeExtraChars = false;

  constructor(annotation) {
    this.#annotation = annotation;
    const quadPoints = annotation.data.quadPoints;
    if (!quadPoints) {
      // If there are no quad points, we use the rectangle to determine the
      // bounds of the annotation.
      [this._minX, this._minY, this._maxX, this._maxY] = annotation.data.rect;
      return;

Review comment (suggestion):
If this is used outside of the class now it might be better to make the fields public instead to indicate that, or alternatively make a getter for them if you want to avoid them being changed from the outside.

Code after:
/* Copyright 2025 Mozilla Foundation
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

class SingleIntersector {
  #annotation;

  minX = Infinity;

  minY = Infinity;

  maxX = -Infinity;

  maxY = -Infinity;

  #quadPoints = null;

  #text = [];

  #extraChars = [];

  #lastIntersectingQuadIndex = -1;

  #canTakeExtraChars = false;

  constructor(annotation) {
    this.#annotation = annotation;
    const quadPoints = annotation.data.quadPoints;
    if (!quadPoints) {
      // If there are no quad points, we use the rectangle to determine the
      // bounds of the annotation.
      [this.minX, this.minY, this.maxX, this.maxY] = annotation.data.rect;
      return;