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

Code before:

  #enableNewAltTextWhenAddingImage = false;

  #eventAbortController = null;

  #mlManager = null;

  #switchAnnotationEditorModeAC = null;

  #switchAnnotationEditorModeTimeoutId = null;

  #getAllTextInProgress = false;

  #hiddenCopyElement = null;

  #interruptCopyCondition = false;

  #previousContainerHeight = 0;

  #resizeObserver = new ResizeObserver(this.#resizeObserverCallback.bind(this));

  #scrollModePageState = null;

  #scaleTimeoutId = null;

  #supportsPinchToZoom = false;

  #textLayerMode = TextLayerMode.ENABLE;

  /**
   * @param {PDFViewerOptions} options
   */
  constructor(options) {
    const viewerVersion =
      typeof PDFJSDev !== "undefined" ? PDFJSDev.eval("BUNDLE_VERSION") : null;
    if (version !== viewerVersion) {
      throw new Error(
        `The API version "${version}" does not match the Viewer version "${viewerVersion}".`
      );
    }
    this.container = options.container;
    this.viewer = options.viewer || options.container.firstElementChild;

    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
      if (this.container?.tagName !== "DIV" || this.viewer?.tagName !== "DIV") {
        throw new Error("Invalid `container` and/or `viewer` option.");
      }

      if (
        this.container.offsetParent &&
        getComputedStyle(this.container).position !== "absolute"

Review comment (suggestion):
```suggestion
  #supportsPinchToZoom = true;
```

Code after:

  #enableNewAltTextWhenAddingImage = false;

  #eventAbortController = null;

  #mlManager = null;

  #switchAnnotationEditorModeAC = null;

  #switchAnnotationEditorModeTimeoutId = null;

  #getAllTextInProgress = false;

  #hiddenCopyElement = null;

  #interruptCopyCondition = false;

  #previousContainerHeight = 0;

  #resizeObserver = new ResizeObserver(this.#resizeObserverCallback.bind(this));

  #scrollModePageState = null;

  #scaleTimeoutId = null;

  #supportsPinchToZoom = true;

  #textLayerMode = TextLayerMode.ENABLE;

  /**
   * @param {PDFViewerOptions} options
   */
  constructor(options) {
    const viewerVersion =
      typeof PDFJSDev !== "undefined" ? PDFJSDev.eval("BUNDLE_VERSION") : null;
    if (version !== viewerVersion) {
      throw new Error(
        `The API version "${version}" does not match the Viewer version "${viewerVersion}".`
      );
    }
    this.container = options.container;
    this.viewer = options.viewer || options.container.firstElementChild;

    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
      if (this.container?.tagName !== "DIV" || this.viewer?.tagName !== "DIV") {
        throw new Error("Invalid `container` and/or `viewer` option.");
      }

      if (
        this.container.offsetParent &&
        getComputedStyle(this.container).position !== "absolute"
Repository: mozilla/pdf.js
File: src/scripting_api/aform.js (JavaScript)

Code before:
      case 0:
        formatStr = "99999";
        break;
      case 1:
        formatStr = "99999-9999";
        break;
      case 2:
        formatStr = "999-9999";
        secondFormatStr = "(999) 999-9999";
        break;
      case 3:
        formatStr = "999-99-9999";
        break;
      default:
        throw new Error("Invalid psf in AFSpecial_Keystroke");
    }
    this.#AFSpecial_KeystrokeEx_helper(formatStr, value, false);
    if (event.rc) {
      return;
    }
    event.rc = true;
    if (secondFormatStr) {
      this.#AFSpecial_KeystrokeEx_helper(secondFormatStr, value, false);
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

    const re = /([-()]|\s)+/g;
    value = value.replaceAll(re, "");
    this.#AFSpecial_KeystrokeEx_helper(
      formatStr.replaceAll(re, ""),
      value,
      false
    );
    if (event.rc) {
      return;
    }

    event.rc = true;
    if (secondFormatStr) {
      this.#AFSpecial_KeystrokeEx_helper(
        secondFormatStr.replaceAll(re, ""),
        value,
        false
      );
      if (event.rc) {
        return;
      }
      event.rc = true;

Review comment (refactor):
Nit: Could we reduce a little bit of duplication here?
```suggestion
    const formats = secondFormatStr ? [formatStr, secondFormatStr] : [formatStr];
    for (const str of formats) {
      this.#AFSpecial_KeystrokeEx_helper(str, value, false);
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

Code after:
      case 0:
        formatStr = "99999";
        break;
      case 1:
        formatStr = "99999-9999";
        break;
      case 2:
        formatStr = "999-9999";
        secondFormatStr = "(999) 999-9999";
        break;
      case 3:
        formatStr = "999-99-9999";
        break;
      default:
        throw new Error("Invalid psf in AFSpecial_Keystroke");
    }
    const formats = secondFormatStr
      ? [formatStr, secondFormatStr]
      : [formatStr];
    for (const format of formats) {
      this.#AFSpecial_KeystrokeEx_helper(format, value, false);
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

    const re = /([-()]|\s)+/g;
    value = value.replaceAll(re, "");
    for (const format of formats) {
      this.#AFSpecial_KeystrokeEx_helper(
        format.replaceAll(re, ""),
        value,
        false
      );
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

    this.AFSpecial_KeystrokeEx(
      ((secondFormatStr && value.match(/\d/g)) || []).length > 7
        ? secondFormatStr
        : formatStr
    );
  }

  AFTime_FormatEx(cFormat) {
    this.AFDate_FormatEx(cFormat);
  }
Repository: mozilla/pdf.js
File: src/scripting_api/aform.js (JavaScript)

Code before:
        throw new Error("Invalid psf in AFSpecial_Keystroke");
    }
    this.#AFSpecial_KeystrokeEx_helper(formatStr, value, false);
    if (event.rc) {
      return;
    }
    event.rc = true;
    if (secondFormatStr) {
      this.#AFSpecial_KeystrokeEx_helper(secondFormatStr, value, false);
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

    const re = /([-()]|\s)+/g;
    value = value.replaceAll(re, "");
    this.#AFSpecial_KeystrokeEx_helper(
      formatStr.replaceAll(re, ""),
      value,
      false
    );
    if (event.rc) {
      return;
    }

    event.rc = true;
    if (secondFormatStr) {
      this.#AFSpecial_KeystrokeEx_helper(
        secondFormatStr.replaceAll(re, ""),
        value,
        false
      );
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

    const nums = (value.match(/\d/g) || []).length;
    this.AFSpecial_KeystrokeEx(nums <= 7 ? formatStr : secondFormatStr);
  }

  AFTime_FormatEx(cFormat) {
    this.AFDate_FormatEx(cFormat);
  }

  AFTime_Format(pdf) {
    if (pdf >= 0 && pdf < this._timeFormats.length) {
      this.AFDate_FormatEx(this._timeFormats[pdf]);
    }

Review comment (refactor):
Nit: Similarly here, could we reduce a little bit of duplication?
```suggestion
    for (const str of formats) {
      this.#AFSpecial_KeystrokeEx_helper(
        str.replaceAll(re, ""),
        value,
        false
      );
      if (event.rc) {
        return;
      }
      event.rc = true;
    }
```

Code after:
        throw new Error("Invalid psf in AFSpecial_Keystroke");
    }
    const formats = secondFormatStr
      ? [formatStr, secondFormatStr]
      : [formatStr];
    for (const format of formats) {
      this.#AFSpecial_KeystrokeEx_helper(format, value, false);
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

    const re = /([-()]|\s)+/g;
    value = value.replaceAll(re, "");
    for (const format of formats) {
      this.#AFSpecial_KeystrokeEx_helper(
        format.replaceAll(re, ""),
        value,
        false
      );
      if (event.rc) {
        return;
      }
      event.rc = true;
    }

    this.AFSpecial_KeystrokeEx(
      ((secondFormatStr && value.match(/\d/g)) || []).length > 7
        ? secondFormatStr
        : formatStr
    );
  }

  AFTime_FormatEx(cFormat) {
    this.AFDate_FormatEx(cFormat);
  }

  AFTime_Format(pdf) {
    if (pdf >= 0 && pdf < this._timeFormats.length) {
      this.AFDate_FormatEx(this._timeFormats[pdf]);
    }
  }

  AFTime_KeystrokeEx(cFormat) {
    this.AFDate_KeystrokeEx(cFormat);
  }

  AFTime_Keystroke(pdf) {
    if (pdf >= 0 && pdf < this._timeFormats.length) {
      this.AFDate_KeystrokeEx(this._timeFormats[pdf]);
Repository: mozilla/pdf.js
File: test/integration/stamp_editor_spec.mjs (JavaScript)

Code before:
    });
  });

  describe("Switch to edit mode by double clicking on an existing stamp annotation", () => {
    let pages;

    beforeAll(async () => {
      pages = await loadAndWait(
        "issue19239.pdf",
        getAnnotationSelector("999R")
      );
    });

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

    it("must switch to edit mode", async () => {
      await Promise.all(
        pages.map(async ([, page]) => {
          await page.waitForSelector(getAnnotationSelector("999R"));
          await scrollIntoView(page, getAnnotationSelector("999R"));

          await page.click(getAnnotationSelector("999R"), { count: 2 });

          const annotationEditorClasses = await page.evaluate(
            () => document.querySelector(".annotationEditorLayer").className
          );
          expect(annotationEditorClasses).toContain("stampEditing");
        })
      );
    });
  });
});


Review comment (bug):
I think the reason why this fails on the the bots is that it can take a bit of time before the double-click action actually leads to a class name change. In other words, the actions that take place between the double-click being initiated and the class name change in the DOM take some time, but Puppeteer doesn't know about that and immediately proceeds after the double-click action. This most likely causes us to call `page.evaluate` before the class name is actually updated.

To prevent such (intermittent) failures it's better to use `waitForFunction` since that ensures that that we actually wait until the class name is what we expect it to be. It probably works to replace `page.evaluate` and `expect` with just this instead (untested, but to illustrate the idea):

```js
await page.waitForFunction(
  () => document.querySelector(".annotationEditorLayer").className.includes("stampEditing")
);
```

This waits until the given condition is satisfied, and the test only passes if this resolves, which in turn only happens when the bugfix is in place.

Code after:
    });
  });

  describe("Switch to edit mode by double clicking on an existing stamp annotation", () => {
    const annotationSelector = getAnnotationSelector("999R");

    let pages;

    beforeAll(async () => {
      pages = await loadAndWait("issue19239.pdf", annotationSelector);
    });

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

    it("must switch to edit mode", async () => {
      await Promise.all(
        pages.map(async ([, page]) => {
          await page.waitForSelector(annotationSelector);
          await scrollIntoView(page, annotationSelector);

          await page.click(annotationSelector, { count: 2 });

          await page.waitForFunction(() =>
            document
              .querySelector(".annotationEditorLayer")
              .classList.contains("stampEditing")
          );
        })
      );
    });
  });
});
Repository: mozilla/pdf.js
File: test/integration/stamp_editor_spec.mjs (JavaScript)

Code before:
            `.page[data-page-number = "1"] .annotationEditorLayer canvas`,
            { visible: true }
          );
        })
      );
    });
  });

  describe("Switch to edit mode by double clicking on an existing stamp annotation", () => {
    let pages;

    beforeAll(async () => {
      pages = await loadAndWait(
        "issue19239.pdf",
        getAnnotationSelector("999R")
      );
    });

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

    it("must switch to edit mode", async () => {
      await Promise.all(
        pages.map(async ([, page]) => {
          await page.waitForSelector(getAnnotationSelector("999R"));
          await scrollIntoView(page, getAnnotationSelector("999R"));

          await page.click(getAnnotationSelector("999R"), { count: 2 });

          const annotationEditorClasses = await page.evaluate(
            () => document.querySelector(".annotationEditorLayer").className
          );
          expect(annotationEditorClasses).toContain("stampEditing");
        })
      );
    });
  });
});


Review comment (suggestion):
Let's define `const annotationSelector = getAnnotationSelector("999R");` once and use that instead of repeating the function call a few times. We try to use this pattern to have the selector definition at a single place, which makes the code a bit shorter/easier to read, avoids repeated lookups and makes it easier to update the test if the selector changes (or when copying and editing existing tests because the body mostly remains the same).

Code after:
            `.page[data-page-number = "1"] .annotationEditorLayer canvas`,
            { visible: true }
          );
        })
      );
    });
  });

  describe("Switch to edit mode by double clicking on an existing stamp annotation", () => {
    const annotationSelector = getAnnotationSelector("999R");

    let pages;

    beforeAll(async () => {
      pages = await loadAndWait("issue19239.pdf", annotationSelector);
    });

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

    it("must switch to edit mode", async () => {
      await Promise.all(
        pages.map(async ([, page]) => {
          await page.waitForSelector(annotationSelector);
          await scrollIntoView(page, annotationSelector);

          await page.click(annotationSelector, { count: 2 });

          await page.waitForFunction(() =>
            document
              .querySelector(".annotationEditorLayer")
              .classList.contains("stampEditing")
          );
        })
      );
    });
  });
});
Repository: mozilla/pdf.js
File: test/integration/stamp_editor_spec.mjs (JavaScript)

Code before:

  describe("Switch to edit mode by double clicking on an existing stamp annotation", () => {
    const annotationSelector = getAnnotationSelector("999R");

    let pages;

    beforeAll(async () => {
      pages = await loadAndWait("issue19239.pdf", annotationSelector);
    });

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

    it("must switch to edit mode", async () => {
      await Promise.all(
        pages.map(async ([, page]) => {
          await page.waitForSelector(annotationSelector);
          await scrollIntoView(page, annotationSelector);

          await page.click(annotationSelector, { count: 2 });

          await page.waitForFunction(() =>
            document
              .querySelector(".annotationEditorLayer")
              .className.includes("stampEditing")
          );
        })
      );
    });
  });
});


Review comment (suggestion):
Possibly a dumb question, but could we utilize `classList` rather than essentially depending on regular string matching here?
```suggestion
              .classList.contains("stampEditing")
```

Code after:

  describe("Switch to edit mode by double clicking on an existing stamp annotation", () => {
    const annotationSelector = getAnnotationSelector("999R");

    let pages;

    beforeAll(async () => {
      pages = await loadAndWait("issue19239.pdf", annotationSelector);
    });

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

    it("must switch to edit mode", async () => {
      await Promise.all(
        pages.map(async ([, page]) => {
          await page.waitForSelector(annotationSelector);
          await scrollIntoView(page, annotationSelector);

          await page.click(annotationSelector, { count: 2 });

          await page.waitForFunction(() =>
            document
              .querySelector(".annotationEditorLayer")
              .classList.contains("stampEditing")
          );
        })
      );
    });
  });
});
Repository: mozilla/pdf.js
File: src/core/jpx.js (JavaScript)

Code before:
 * 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.
 */

import { BaseException, warn } from "../shared/util.js";
import OpenJPEG from "../../external/openjpeg/openjpeg.js";
import { Stream } from "./stream.js";

class JpxError extends BaseException {
  constructor(msg) {
    super(msg, "JpxError");
  }
}

class JpxImage {
  static #buffer = null;

  static #handler = null;

  static #instantiationFailed = false;

  static #modulePromise = null;

  static #openjpegWasmUrl = "";

  static setOptions({ handler, openjpegWasmUrl }) {
    if (!this.#buffer) {
      this.#openjpegWasmUrl = openjpegWasmUrl;
      if (!openjpegWasmUrl) {
        this.#handler = handler;
      }
    }
  }

  static async #instantiateWasm(imports, successCallback) {
    try {
      if (!this.#buffer) {
        const wasmFile = "openjpeg.wasm";
        if (this.#openjpegWasmUrl) {
          const response = await fetch(`${this.#openjpegWasmUrl}/${wasmFile}`);
          this.#buffer = await response.arrayBuffer();
        } else {
          this.#buffer = await this.#handler.sendWithPromise(
            "FetchOpenjpegWasm",
            {
              filename: wasmFile,
            }
          );
        }

Review comment (suggestion):
Please keep the initial value consistent with e.g. DefaultPartialEvaluatorOptions.
```suggestion
  static #openjpegWasmUrl = null;
```

Code after:
 * 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.
 */

import { BaseException, warn } from "../shared/util.js";
import OpenJPEG from "../../external/openjpeg/openjpeg.js";
import { Stream } from "./stream.js";

class JpxError extends BaseException {
  constructor(msg) {
    super(msg, "JpxError");
  }
}

class JpxImage {
  static #buffer = null;

  static #handler = null;

  static #instantiationFailed = false;

  static #modulePromise = null;

  static #wasmUrl = null;

  static setOptions({ handler, wasmUrl }) {
    if (!this.#buffer) {
      this.#wasmUrl = wasmUrl || null;
      if (wasmUrl === null) {
        this.#handler = handler;
      }
    }
  }

  static async #instantiateWasm(imports, successCallback) {
    try {
      if (!this.#buffer) {
        if (this.#wasmUrl !== null) {
          const response = await fetch(`${this.#wasmUrl}openjpeg.wasm`);
          this.#buffer = await response.arrayBuffer();
        } else {
          this.#buffer = await this.#handler.sendWithPromise("FetchWasm", {
            filename: "openjpeg.wasm",
          });
        }
      }
      const results = await WebAssembly.instantiate(this.#buffer, imports);
      return successCallback(results.instance);
    } catch (e) {
Repository: mozilla/pdf.js
File: src/core/jpx.js (JavaScript)

Code before:
}

class JpxImage {
  static #buffer = null;

  static #handler = null;

  static #instantiationFailed = false;

  static #modulePromise = null;

  static #openjpegWasmUrl = "";

  static setOptions({ handler, openjpegWasmUrl }) {
    if (!this.#buffer) {
      this.#openjpegWasmUrl = openjpegWasmUrl;
      if (!openjpegWasmUrl) {
        this.#handler = handler;
      }
    }
  }

  static async #instantiateWasm(imports, successCallback) {
    try {
      if (!this.#buffer) {
        const wasmFile = "openjpeg.wasm";
        if (this.#openjpegWasmUrl) {
          const response = await fetch(`${this.#openjpegWasmUrl}/${wasmFile}`);
          this.#buffer = await response.arrayBuffer();
        } else {
          this.#buffer = await this.#handler.sendWithPromise(
            "FetchOpenjpegWasm",
            {
              filename: wasmFile,
            }
          );
        }
      }
      const results = await WebAssembly.instantiate(this.#buffer, imports);
      return successCallback(results.instance);
    } catch (e) {
      this.#instantiationFailed = true;
      warn(`Cannot load openjpeg.wasm ${e}`);
      return false;
    } finally {
      this.#handler = null;
      this.#openjpegWasmUrl = "";
    }
  }

  static async decode(

Review comment (suggestion):
```suggestion
    const wasmFile = "openjpeg.wasm";
    try {
      if (!this.#buffer) {
```

Code after:
}

class JpxImage {
  static #buffer = null;

  static #handler = null;

  static #instantiationFailed = false;

  static #modulePromise = null;

  static #wasmUrl = null;

  static setOptions({ handler, wasmUrl }) {
    if (!this.#buffer) {
      this.#wasmUrl = wasmUrl || null;
      if (wasmUrl === null) {
        this.#handler = handler;
      }
    }
  }

  static async #instantiateWasm(imports, successCallback) {
    try {
      if (!this.#buffer) {
        if (this.#wasmUrl !== null) {
          const response = await fetch(`${this.#wasmUrl}openjpeg.wasm`);
          this.#buffer = await response.arrayBuffer();
        } else {
          this.#buffer = await this.#handler.sendWithPromise("FetchWasm", {
            filename: "openjpeg.wasm",
          });
        }
      }
      const results = await WebAssembly.instantiate(this.#buffer, imports);
      return successCallback(results.instance);
    } catch (e) {
      this.#instantiationFailed = true;
      warn(`Cannot load openjpeg.wasm: "${e}".`);
      return false;
    } finally {
      this.#handler = null;
      this.#wasmUrl = null;
    }
  }

  static async decode(
    bytes,
    { numComponents = 4, isIndexedColormap = false, smaskInData = false } = {}
  ) {
    if (this.#instantiationFailed) {