ronantakizawa/codereview-bench huggingface dataset:
Repository: mozilla/pdf.js
File: src/core/jbig2_ccittFax_wasm.js (JavaScript)

Code before:
      const promises = [promise];
      if (this.#useWasm) {
        promises.push(
          JBig2({
            warn,
            instantiateWasm: this.#instantiateWasm.bind(this, resolve),
          })
        );
      } else {
        resolve(null);
      }
      this.#modulePromise = Promise.race(promises);
    }
    const module = await this.#modulePromise;
    if (!module) {
      throw new JBig2Error("JBig2 failed to initialize");
    }
    let ptr, globalsPtr;

    try {
      const size = bytes.length;
      ptr = module._malloc(size);
      module.writeArrayToMemory(bytes, ptr);

      if (CCITOptions) {
        module._ccit_decode(
          ptr,
          size,
          width,
          height,
          CCITOptions.K,
          CCITOptions.EndOfLine,
          CCITOptions.ByteAlign,
          CCITOptions.BlackIs1,
          CCITOptions.Columns,
          CCITOptions.Rows
        );
      } else {
        const globalsSize = globals ? globals.length : 0;
        if (globalsSize > 0) {
          globalsPtr = module._malloc(globalsSize);
          module.writeArrayToMemory(globals, globalsPtr);
        }
        module._jbig2_decode(ptr, size, width, height, globalsPtr, globalsSize);
      }
      if (!module.imageData) {
        throw new JBig2Error("Unknown error");
      }
      const { imageData } = module;
      module.imageData = null;


Review comment (suggestion):
s/_ccit_decode/_ccitt_decode (this should probably also be changed in the occurrences from https://github.com/mozilla/pdf.js.jbig2/commit/824d912a548a09a5ec4f8a0afabc7787a3126c4c)

Code after:
      const promises = [promise];
      if (this.#useWasm) {
        promises.push(
          JBig2({
            warn,
            instantiateWasm: this.#instantiateWasm.bind(this, resolve),
          })
        );
      } else {
        resolve(null);
      }
      this.#modulePromise = Promise.race(promises);
    }
    const module = await this.#modulePromise;
    if (!module) {
      throw new JBig2Error("JBig2 failed to initialize");
    }
    let ptr, globalsPtr;

    try {
      const size = bytes.length;
      ptr = module._malloc(size);
      module.writeArrayToMemory(bytes, ptr);

      if (CCITTOptions) {
        module._ccitt_decode(
          ptr,
          size,
          width,
          height,
          CCITTOptions.K,
          CCITTOptions.EndOfLine ? 1 : 0,
          CCITTOptions.EncodedByteAlign ? 1 : 0,
          CCITTOptions.BlackIs1 ? 1 : 0,
          CCITTOptions.Columns,
          CCITTOptions.Rows
        );
      } else {
        const globalsSize = globals ? globals.length : 0;
        if (globalsSize > 0) {
          globalsPtr = module._malloc(globalsSize);
          module.writeArrayToMemory(globals, globalsPtr);
        }
        module._jbig2_decode(ptr, size, width, height, globalsPtr, globalsSize);
      }
      if (!module.imageData) {
        throw new JBig2Error("Unknown error");
      }
      const { imageData } = module;
      module.imageData = null;
Repository: mozilla/pdf.js
File: web/pdf_thumbnail_view.js (JavaScript)

Code before:
    }
    ctx.drawImage(
      reducedImage,
      0,
      0,
      reducedWidth,
      reducedHeight,
      0,
      0,
      canvas.width,
      canvas.height
    );
    return canvas;
  }

  get #pageL10nArgs() {
    return JSON.stringify({ page: this.pageLabel ?? this.id });
  }

  /**
   * @param {string|null} label
   */
  setPageLabel(label) {
    this.pageLabel = typeof label === "string" ? label : null;
    this.image.setAttribute("data-l10n-args", this.#pageL10nArgs);
    if (this.checkbox) {
      this.checkbox.setAttribute("data-l10n-args", this.#pageL10nArgs);
    }
  }
}

export { PDFThumbnailView };


Review comment (suggestion):
We could probably replace this `if` by using `this.checkbox?.setAttribute` below.

Code after:
    }
    ctx.drawImage(
      reducedImage,
      0,
      0,
      reducedWidth,
      reducedHeight,
      0,
      0,
      canvas.width,
      canvas.height
    );
    return canvas;
  }

  get #pageL10nArgs() {
    return JSON.stringify({ page: this.pageLabel ?? this.id });
  }

  /**
   * @param {string|null} label
   */
  setPageLabel(label) {
    this.pageLabel = typeof label === "string" ? label : null;
    this.image.setAttribute("data-l10n-args", this.#pageL10nArgs);
    this.checkbox?.setAttribute("data-l10n-args", this.#pageL10nArgs);
  }
}

export { PDFThumbnailView };
Repository: mozilla/send
File: server/portal_server.js (JavaScript)

Code before:
app.post('/delete/:id', (req, res) => {
  const id = req.params.id;

  if (!validateID(id)) {
    res.sendStatus(404);
    return;
  }

  const delete_token = req.body.delete_token;

  if (!delete_token) {
    res.sendStatus(404);
  }

  storage
    .delete(id, delete_token)
    .then(err => {
      if (!err) {
        log.info('Deleted:', id);
        res.sendStatus(200);
      }
    })
    .catch(err => res.sendStatus(404));
});

app.post('/upload/:id', (req, res, next) => {
  if (!validateIV(req.params.id)) {
    res.sendStatus(404);
    return;
  }
  const meta = JSON.parse(req.header('X-File-Metadata'));
  log.info('meta', meta);
  req.pipe(req.busboy);

  req.busboy.on('file', (fieldname, file, filename) => {
    log.info('Uploading:', req.params.id);

    storage.set(req.params.id, file, filename, meta).then(([delete_token, new_id]) => {
      const protocol = conf.env === 'production' ? 'https' : req.protocol;
      const url = `${protocol}://${req.get('host')}/download/${new_id}/`;
      res.json({
        url,
        delete: delete_token
      });
    });
  });
});

app.get('/__lbheartbeat__', (req, res) => {
  res.sendStatus(200);
});

Review comment (suggestion):
We no longer need the `:id` parameter in the upload url. `new_id` should be created at this level instead of in storage.js. It'd also be better to create `meta.delete` up here too. Ideally storage.js should only be concerned with saving and fetching the data from persistent storage and leaving the logic of what the data is and how it's created to something else (this function).

Code after:

app.post('/delete/:id', (req, res) => {
  const id = req.params.id;

  if (!validateID(id)) {
    res.sendStatus(404);
    return;
  }

  const delete_token = req.body.delete_token;

  if (!delete_token) {
    res.sendStatus(404);
  }

  storage
    .delete(id, delete_token)
    .then(err => {
      if (!err) {
        log.info('Deleted:', id);
        res.sendStatus(200);
      }
    })
    .catch(err => res.sendStatus(404));
});

app.post('/upload', (req, res, next) => {
  const newId = crypto.randomBytes(5).toString('hex');
  const meta = JSON.parse(req.header('X-File-Metadata'));
  meta.delete = crypto.randomBytes(10).toString('hex');
  log.info('meta', meta);
  req.pipe(req.busboy);

  req.busboy.on('file', (fieldname, file, filename) => {
    log.info('Uploading:', newId);

    storage.set(newId, file, filename, meta).then(() => {
      const protocol = conf.env === 'production' ? 'https' : req.protocol;
      const url = `${protocol}://${req.get('host')}/download/${newId}/`;
      res.json({
        url,
        delete: meta.delete,
        id: newId
      });
    });
  });
});

app.get('/__lbheartbeat__', (req, res) => {
  res.sendStatus(200);
});
Repository: mozilla/send
File: server/server.js (JavaScript)

Code before:
  storage
    .delete(id, delete_token)
    .then(err => {
      if (!err) {
        log.info('Deleted:', id);
        res.sendStatus(200);
      }
    })
    .catch(err => res.sendStatus(404));
});

app.post('/upload', (req, res, next) => {
  const newId = crypto.randomBytes(5).toString('hex');
  let meta;

  try {
    meta = JSON.parse(req.header('X-File-Metadata'));
  } catch (err) {
    res.sendStatus(400);
    return;
  }

  if (
    !meta.hasOwnProperty('aad') ||
    !meta.hasOwnProperty('id') ||
    !meta.hasOwnProperty('filename' ||
    !validateIV(meta.id))
  ) {
    res.sendStatus(404);
    return;
  }

  meta.delete = crypto.randomBytes(10).toString('hex');
  log.info('meta', meta);
  req.pipe(req.busboy);

  req.busboy.on('file', (fieldname, file, filename) => {
    log.info('Uploading:', newId);

    storage.set(newId, file, filename, meta).then(() => {
      const protocol = conf.env === 'production' ? 'https' : req.protocol;
      const url = `${protocol}://${req.get('host')}/download/${newId}/`;
      res.json({
        url,
        delete: meta.delete,
        id: newId
      });
    });
  });
});


Review comment (bug):
I think... this is wrong.

```
!meta.hasOwnProperty('filename') ||
!validateIV(meta.id)
```


Code after:
    res.sendStatus(404);
    return;
  }

  storage
    .delete(id, delete_token)
    .then(err => {
      if (!err) {
        log.info('Deleted:', id);
        res.sendStatus(200);
      }
    })
    .catch(err => res.sendStatus(404));
});

app.post('/upload', (req, res, next) => {
  const newId = crypto.randomBytes(5).toString('hex');
  let meta;

  try {
    meta = JSON.parse(req.header('X-File-Metadata'));
  } catch (err) {
    res.sendStatus(400);
    return;
  }

  if (
    !meta.hasOwnProperty('aad') ||
    !meta.hasOwnProperty('id') ||
    !meta.hasOwnProperty('filename') ||
    !validateIV(meta.id)
  ) {
    res.sendStatus(404);
    return;
  }

  meta.delete = crypto.randomBytes(10).toString('hex');
  log.info('meta', meta);
  req.pipe(req.busboy);

  req.busboy.on('file', (fieldname, file, filename) => {
    log.info('Uploading:', newId);

    storage.set(newId, file, filename, meta).then(() => {
      const protocol = conf.env === 'production' ? 'https' : req.protocol;
      const url = `${protocol}://${req.get('host')}/download/${newId}/`;
      res.json({
        url,
        delete: meta.delete,
        id: newId
      });
Repository: mozilla/send
File: server/server.js (JavaScript)

Code before:
      scriptSrc: ["'self'"],
      styleSrc: ["'self'", 'https://code.cdn.mozilla.net'],
      fontSrc: ["'self'", 'https://code.cdn.mozilla.net'],
      formAction: ["'none'"],
      frameAncestors: ["'none'"],
      objectSrc: ["'none'"],
      reportUri: '/__cspreport__'
    }
  })
);
app.use(
  busboy({
    limits: {
      fileSize: conf.max_file_size
    }
  })
);
app.use(bodyParser.json());
app.use(express.static(STATIC_PATH));

app.get('/', (req, res) => {
  res.render('index');
});

app.get('/unsupported/:reason', (req, res) => {
  const outdated = req.params.reason === 'outdated'? true : false;
  res.render('unsupported', {
    outdated: outdated
  });
});

app.get('/legal', (req, res) => {
  res.render('legal');
});

app.get('/jsconfig.js', (req, res) => {
  res.set('Content-Type', 'application/javascript');
  res.render('jsconfig', {
    trackerId: conf.analytics_id,
    dsn: conf.sentry_id,
    maxFileSize: conf.max_file_size,
    expireSeconds: conf.expire_seconds,
    layout: false
  });
});

app.get('/exists/:id', (req, res) => {
  const id = req.params.id;
  if (!validateID(id)) {
    res.sendStatus(404);
    return;

Review comment (suggestion):
I think the `? true : false` sorcery is unnecessary here since `req.params.reason === 'outdated'` should be Boolean-enough(tm).

Code after:
      scriptSrc: ["'self'"],
      styleSrc: ["'self'", 'https://code.cdn.mozilla.net'],
      fontSrc: ["'self'", 'https://code.cdn.mozilla.net'],
      formAction: ["'none'"],
      frameAncestors: ["'none'"],
      objectSrc: ["'none'"],
      reportUri: '/__cspreport__'
    }
  })
);
app.use(
  busboy({
    limits: {
      fileSize: conf.max_file_size
    }
  })
);
app.use(bodyParser.json());
app.use(express.static(STATIC_PATH));

app.get('/', (req, res) => {
  res.render('index');
});

app.get('/unsupported/:reason', (req, res) => {
  const outdated = req.params.reason === 'outdated';
  res.render('unsupported', {
    outdated: outdated
  });
});

app.get('/legal', (req, res) => {
  res.render('legal');
});

app.get('/jsconfig.js', (req, res) => {
  res.set('Content-Type', 'application/javascript');
  res.render('jsconfig', {
    trackerId: conf.analytics_id,
    dsn: conf.sentry_id,
    maxFileSize: conf.max_file_size,
    expireSeconds: conf.expire_seconds,
    layout: false
  });
});

app.get('/exists/:id', (req, res) => {
  const id = req.params.id;
  if (!validateID(id)) {
    res.sendStatus(404);
    return;
Repository: mozilla/send
File: frontend/src/fileSender.js (JavaScript)

Code before:
  constructor(file) {
    super();
    this.file = file;
    this.iv = window.crypto.getRandomValues(new Uint8Array(12));
    this.uploadXHR = new XMLHttpRequest();
  }

  static delete(fileId, token) {
    return new Promise((resolve, reject) => {
      if (!fileId || !token) {
        return reject();
      }
      const xhr = new XMLHttpRequest();
      xhr.open('post', '/delete/' + fileId, true);
      xhr.setRequestHeader('Content-Type', 'application/json');

      xhr.onreadystatechange = () => {
        if (xhr.readyState === XMLHttpRequest.DONE) {
          resolve();
        }
      };

      xhr.send(JSON.stringify({ delete_token: token }));
    });
  }

  cancel() {
    this.uploadXHR.abort();
  }

  upload() {
    const self = this;
    self.emit('loading', true);
    return Promise.all([
      window.crypto.subtle
        .generateKey(
          {
            name: 'AES-GCM',
            length: 128
          },
          true,
          ['encrypt', 'decrypt']
        )
        .catch(err =>
          // eslint-disable-next-line no-console
          console.error('There was an error generating a crypto key')
        ),
      new Promise((resolve, reject) => {
        const reader = new FileReader();
        reader.readAsArrayBuffer(this.file);
        reader.onload = function(event) {

Review comment (suggestion):
This felt weird to kill, but wasn't really "adding" anything to the user experience.
Not sure if we should be doing some metrics logging here instead of just vanilla `console.log()`.

Code after:
  constructor(file) {
    super();
    this.file = file;
    this.iv = window.crypto.getRandomValues(new Uint8Array(12));
    this.uploadXHR = new XMLHttpRequest();
  }

  static delete(fileId, token) {
    return new Promise((resolve, reject) => {
      if (!fileId || !token) {
        return reject();
      }
      const xhr = new XMLHttpRequest();
      xhr.open('post', '/delete/' + fileId, true);
      xhr.setRequestHeader('Content-Type', 'application/json');

      xhr.onreadystatechange = () => {
        if (xhr.readyState === XMLHttpRequest.DONE) {
          resolve();
        }
      };

      xhr.send(JSON.stringify({ delete_token: token }));
    });
  }

  cancel() {
    this.uploadXHR.abort();
  }

  upload() {
    const self = this;
    self.emit('loading', true);
    return Promise.all([
      window.crypto.subtle
        .generateKey(
          {
            name: 'AES-GCM',
            length: 128
          },
          true,
          ['encrypt', 'decrypt']
        ),
      new Promise((resolve, reject) => {
        const reader = new FileReader();
        reader.readAsArrayBuffer(this.file);
        reader.onload = function(event) {
          self.emit('loading', false);
          self.emit('hashing', true);
          const plaintext = new Uint8Array(this.result);
          window.crypto.subtle.digest('SHA-256', plaintext).then(hash => {
Repository: mozilla/send
File: frontend/src/utils.js (JavaScript)

Code before:
  switch (href) {
    case 'https://www.mozilla.org/':
      return 'mozilla';
    case 'https://www.mozilla.org/about/legal':
      return 'legal';
    case 'https://testpilot.firefox.com/about':
      return 'about';
    case 'https://testpilot.firefox.com/privacy':
      return 'privacy';
    case 'https://testpilot.firefox.com/terms':
      return 'terms';
    case 'https://www.mozilla.org/privacy/websites/#cookies':
      return 'cookies';
    case 'https://github.com/mozilla/send':
      return 'github';
    case 'https://twitter.com/FxTestPilot':
      return 'twitter';
    case 'https://www.mozilla.org/firefox/new/?scene=2':
      return 'download-firefox';
    default:
      return 'other';
  }
}

function isFile(id) {
  return ![
    'referrer',
    'totalDownloads',
    'totalUploads',
    'testpilot_ga__cid'
  ].includes(id);
}

function sendEvent() {
  return window.analytics.sendEvent
    .apply(window.analytics, arguments)
    .catch(() => 0);
}

const ONE_DAY_IN_MS = 86400000;

module.exports = {
  arrayToHex,
  hexToArray,
  notify,
  gcmCompliant,
  findMetric,
  isFile,
  sendEvent,
  ONE_DAY_IN_MS
};

Review comment (security):
micro nit, but you could just pass the `i` flag to make it case insensitive so you wouldnt need the extra "A-F" character range. 🤷‍♀️ 

```js
return /^[0-9a-f]{10}$/i.test(id);
```

But, yeah, well, that's just, like, my opinion, man.

Code after:
  switch (href) {
    case 'https://www.mozilla.org/':
      return 'mozilla';
    case 'https://www.mozilla.org/about/legal':
      return 'legal';
    case 'https://testpilot.firefox.com/about':
      return 'about';
    case 'https://testpilot.firefox.com/privacy':
      return 'privacy';
    case 'https://testpilot.firefox.com/terms':
      return 'terms';
    case 'https://www.mozilla.org/privacy/websites/#cookies':
      return 'cookies';
    case 'https://github.com/mozilla/send':
      return 'github';
    case 'https://twitter.com/FxTestPilot':
      return 'twitter';
    case 'https://www.mozilla.org/firefox/new/?scene=2':
      return 'download-firefox';
    default:
      return 'other';
  }
}

function isFile(id) {
  return /^[0-9a-fA-F]{10}$/.test(id);
}

function sendEvent() {
  return window.analytics.sendEvent
    .apply(window.analytics, arguments)
    .catch(() => 0);
}

const ONE_DAY_IN_MS = 86400000;

module.exports = {
  arrayToHex,
  hexToArray,
  notify,
  gcmCompliant,
  findMetric,
  isFile,
  sendEvent,
  ONE_DAY_IN_MS
};
Repository: mozilla/send
File: server/server.js (JavaScript)

Code before:

const log = mozlog('send.server');

const STATIC_PATH = path.join(__dirname, '../public');

const app = express();

function allLangs() {
  return fs
    .readdirSync(path.join(STATIC_PATH, 'locales'))
    .map(function(f) {
      return f.split('.')[0];
    })
    .join(',');
}

function prodLangs() {
  return require('../package.json').availableLanguages.join(',');
}

const availableLanguages = conf.l10n_dev ? allLangs() : prodLangs();

const envURL = (env) => {
  switch (env) {
    case 'test':
      return 'https://send.stage.mozaws.net';
    case 'development':
      return 'https://testpilot.dev.mozaws.net';
  }
  return 'https://send.firefox.com';
}

if (conf.env === 'development') {
  const webpack = require('webpack');
  const webpackDevMiddleware = require('webpack-dev-middleware');
  const config = require('../webpack.config.js');
  config.devtool = 'inline-source-map';
  const compiler = webpack(config);
  app.use(
    webpackDevMiddleware(compiler, {
      publicPath: config.output.publicPath
    })
  );
}

app.engine(
  'handlebars',
  exphbs({
    defaultLayout: 'main',
    partialsDir: 'views/partials/',
    helpers: {

Review comment (suggestion):
I'm not convinced this is a thing.
I have a feeling that stage would use an env of "production". Not sure what [if anything] uses the "test" NODE_ENV, unless Circle-CI somehow sets it to that, but I strangely kind of doubt it.

I don't know how to tell the detect whether we're running on stage vs prod on the server.

Code after:

const log = mozlog('send.server');

const STATIC_PATH = path.join(__dirname, '../public');

const app = express();

function allLangs() {
  return fs
    .readdirSync(path.join(STATIC_PATH, 'locales'))
    .map(function(f) {
      return f.split('.')[0];
    })
    .join(',');
}

function prodLangs() {
  return require('../package.json').availableLanguages.join(',');
}

const availableLanguages = conf.l10n_dev ? allLangs() : prodLangs();

if (conf.env === 'development') {
  const webpack = require('webpack');
  const webpackDevMiddleware = require('webpack-dev-middleware');
  const config = require('../webpack.config.js');
  config.devtool = 'inline-source-map';
  const compiler = webpack(config);
  app.use(
    webpackDevMiddleware(compiler, {
      publicPath: config.output.publicPath
    })
  );
}

app.engine(
  'handlebars',
  exphbs({
    defaultLayout: 'main',
    partialsDir: 'views/partials/',
    helpers: {
      availableLanguages,
      l10nDev: conf.l10n_dev,
      baseUrl: conf.base_url,
      title: 'Firefox Send',
      description: 'Encrypt and send files with a link that automatically expires to ensure your important documents don’t stay online forever.'
    }
  })
);
app.set('view engine', 'handlebars');
Repository: mozilla/send
File: frontend/src/storage.js (JavaScript)

Code before:
          // obviously you're not a golfer
          this.engine.removeItem(k);
        }
      }
    }
    return fs.sort((file1, file2) => {
      const creationDate1 = new Date(file1.creationDate);
      const creationDate2 = new Date(file2.creationDate);
      return creationDate1 - creationDate2;
    });
  }

  get numFiles() {
    let length = 0;
    for (let i = 0; i < this.engine.length; i++) {
      const k = this.engine.key(i);
      if (isFile(k)) {
        length += 1;
      }
    }
    return length;
  }

  getFileById(id) {
    try {
    return JSON.parse(this.engine.getItem(id));
  }
  catch (e) { return null }
  }

  remove(property) {
    this.engine.removeItem(property);
  }

  addFile(id, file) {
    this.engine.setItem(id, JSON.stringify(file));
  }
}


Review comment (bug):
Indenting here looks v wrong. If I had to guess, it's because we've disabled indenting checks in ESLint since that's handled automatically by **prettier**. 

One solution may be to have some auto-format+lint code on a Git hook. It'd force **prettier** formatting on each commit, which may cause 1-2 noisy commits w/ unrelated formatting fixes at first, but then probably 👌 after that initial hiccup.


Code after:
          // obviously you're not a golfer
          this.engine.removeItem(k);
        }
      }
    }
    return fs.sort((file1, file2) => {
      const creationDate1 = new Date(file1.creationDate);
      const creationDate2 = new Date(file2.creationDate);
      return creationDate1 - creationDate2;
    });
  }

  get numFiles() {
    let length = 0;
    for (let i = 0; i < this.engine.length; i++) {
      const k = this.engine.key(i);
      if (isFile(k)) {
        length += 1;
      }
    }
    return length;
  }

  getFileById(id) {
    try {
      return JSON.parse(this.engine.getItem(id));
    } catch (e) {
      return null;
    }
  }

  remove(property) {
    this.engine.removeItem(property);
  }

  addFile(id, file) {
    this.engine.setItem(id, JSON.stringify(file));
  }
}