ronantakizawa/codereview-bench huggingface dataset:
Repository: jiahaog/nativefier
File: src/helpers/helpers.ts (TypeScript)

Code before:
tmp.setGracefulCleanup(); // cleanup temp dirs even when an uncaught exception occurs

const now = new Date();
const TMP_TIME = `${now.getHours()}-${now.getMinutes()}-${now.getSeconds()}`;

type DownloadResult = {
  data: Buffer;
  ext: string;
};

export function isOSX(): boolean {
  return os.platform() === 'darwin';
}

export function isWindows(): boolean {
  return os.platform() === 'win32';
}

export function isWindowsAdmin(): boolean {
  if (process.platform !== 'win32') {
    return false;
  }

  return spawnSync('fltmc').status === 0;
}

/**
 * Create a temp directory with a debug-friendly name, and return its path.
 * Will be automatically deleted on exit.
 */
export function getTempDir(prefix: string, mode?: number): string {
  return tmp.dirSync({
    mode,
    unsafeCleanup: true, // recursively remove tmp dir on exit, even if not empty.
    prefix: `nativefier-${TMP_TIME}-${prefix}-`,
  }).name;
}

export async function copyFileOrDir(
  sourceFileOrDir: string,
  dest: string,
): Promise<void> {
  return new Promise((resolve, reject) => {
    ncp(sourceFileOrDir, dest, (error: any) => {
      if (error) {
        reject(error);
      }
      resolve();
    });
  });
}

Review comment (bug):
```suggestion
  // https://stackoverflow.com/questions/4051883/batch-script-how-to-check-for-admin-rights
  // https://stackoverflow.com/questions/57009374/check-admin-or-non-admin-users-in-nodejs-or-javascript
  return spawnSync('fltmc').status === 0;
```

Code after:
tmp.setGracefulCleanup(); // cleanup temp dirs even when an uncaught exception occurs

const now = new Date();
const TMP_TIME = `${now.getHours()}-${now.getMinutes()}-${now.getSeconds()}`;

type DownloadResult = {
  data: Buffer;
  ext: string;
};

export function isOSX(): boolean {
  return os.platform() === 'darwin';
}

export function isWindows(): boolean {
  return os.platform() === 'win32';
}

export function isWindowsAdmin(): boolean {
  if (process.platform !== 'win32') {
    return false;
  }

  // https://stackoverflow.com/questions/4051883/batch-script-how-to-check-for-admin-rights
  // https://stackoverflow.com/questions/57009374/check-admin-or-non-admin-users-in-nodejs-or-javascript
  return spawnSync('fltmc').status === 0;
}

/**
 * Create a temp directory with a debug-friendly name, and return its path.
 * Will be automatically deleted on exit.
 */
export function getTempDir(prefix: string, mode?: number): string {
  return tmp.dirSync({
    mode,
    unsafeCleanup: true, // recursively remove tmp dir on exit, even if not empty.
    prefix: `nativefier-${TMP_TIME}-${prefix}-`,
  }).name;
}

export async function copyFileOrDir(
  sourceFileOrDir: string,
  dest: string,
): Promise<void> {
  return new Promise((resolve, reject) => {
    ncp(sourceFileOrDir, dest, (error: any) => {
      if (error) {
        reject(error);
      }
      resolve();
    });
Repository: jiahaog/nativefier
File: app/src/components/menu.ts (TypeScript)

Code before:
        },
        {
          type: 'separator',
        },
        {
          label: 'Quit',
          accelerator: 'Cmd+Q',
          click: appQuit,
        },
      ],
    };
    (windowMenu.submenu as MenuItemConstructorOptions[]).push(
      {
        type: 'separator',
      },
      {
        label: 'Bring All to Front',
        role: 'front',
      },
    );
    menuTemplate = [electronMenu, editMenu, viewMenu, windowMenu, helpMenu];
  } else {
    menuTemplate = [editMenu, viewMenu, windowMenu, helpMenu];
  }

  const bookmarkConfigPath = path.join(__dirname, '..', 'bookmarks.json');
  if (fs.existsSync(bookmarkConfigPath)) {
    const bookmarksConfig = JSON.parse(
      fs.readFileSync(bookmarkConfigPath, 'utf-8'),
    );
    const bookmarksMenu: MenuItemConstructorOptions = {
      label: bookmarksConfig['menuLabel'],
      submenu: bookmarksConfig['bookmarks'].map((el) => {
        if (el['type'] === 'link') {
          let accelerator = null;
          if ('shortcut' in el) {
            accelerator = el['shortcut'];
          }
          return {
            label: el['title'],
            click: () => {
              gotoUrl(el['url']);
            },
            accelerator: accelerator,
          };
        } else if (el['type'] === 'separator') {
          return {
            type: 'separator',
          };
        }
      }),

Review comment (suggestion):
I would personally suggest putting a `try` block around this so if there is an issue creating the menu, the app could still proceed without it. Not sure if @ronjouch would agree though.

Code after:
        {
          type: 'separator',
        },
        {
          label: 'Hide App',
          accelerator: 'Cmd+H',
          role: 'hide',
        },
        {
          label: 'Hide Others',
          accelerator: 'Cmd+Shift+H',
          role: 'hideOthers',
        },
        {
          label: 'Show All',
          role: 'unhide',
        },
        {
          type: 'separator',
        },
        {
          label: 'Quit',
          accelerator: 'Cmd+Q',
          click: appQuit,
        },
      ],
    };
    (windowMenu.submenu as MenuItemConstructorOptions[]).push(
      {
        type: 'separator',
      },
      {
        label: 'Bring All to Front',
        role: 'front',
      },
    );
    menuTemplate = [electronMenu, editMenu, viewMenu, windowMenu, helpMenu];
  } else {
    menuTemplate = [editMenu, viewMenu, windowMenu, helpMenu];
  }

  try {
    const bookmarkConfigPath = path.join(__dirname, '..', 'bookmarks.json');
    if (fs.existsSync(bookmarkConfigPath)) {
      const bookmarksMenuConfig: BookmarksMenuConfig = JSON.parse(
        fs.readFileSync(bookmarkConfigPath, 'utf-8'),
      );
      const bookmarksMenu: MenuItemConstructorOptions = {
        label: bookmarksMenuConfig.menuLabel,
        submenu: bookmarksMenuConfig.bookmarks.map((bookmark) => {
          if (bookmark.type === 'link') {
Repository: jiahaog/nativefier
File: app/src/components/menu.ts (TypeScript)

Code before:
import { Menu, clipboard, shell, MenuItemConstructorOptions } from 'electron';
import * as fs from 'fs';
import path from 'path';

export function createMenu({
  nativefierVersion,
  appQuit,
  zoomIn,
  zoomOut,
  zoomReset,
  zoomBuildTimeValue,
  goBack,
  goForward,
  getCurrentUrl,
  gotoUrl,
  clearAppData,
  disableDevTools,
}): void {
  const zoomResetLabel =
    zoomBuildTimeValue === 1.0
      ? 'Reset Zoom'
      : `Reset Zoom (to ${zoomBuildTimeValue * 100}%, set at build time)`;

  const editMenu: MenuItemConstructorOptions = {
    label: '&Edit',
    submenu: [
      {
        label: 'Undo',

Review comment (suggestion):
Nit about imports ordering: 1. node stdlib, 2. deps, 3. internal, separated by newlines. So,

```suggestion
import * as fs from 'fs';
import path from 'path';

import { Menu, clipboard, shell, MenuItemConstructorOptions } from 'electron';
```

Code after:
import * as fs from 'fs';
import path from 'path';

import { Menu, clipboard, shell, MenuItemConstructorOptions } from 'electron';

type BookmarksLink = {
  type: 'link';
  title: string;
  url: string;
  shortcut?: string;
};
type BookmarksSeparator = {
  type: 'separator';
};
type BookmarkConfig = BookmarksLink | BookmarksSeparator;
type BookmarksMenuConfig = {
  menuLabel: string;
  bookmarks: BookmarkConfig[];
};

export function createMenu({
  nativefierVersion,
  appQuit,
  zoomIn,
  zoomOut,
  zoomReset,
  zoomBuildTimeValue,
  goBack,
Repository: jiahaog/nativefier
File: app/src/components/menu.ts (TypeScript)

Code before:
              throw Error('Bookmark URL "' + bookmark.url + '"is invalid.');
            }
            let accelerator = null;
            if ('shortcut' in bookmark) {
              accelerator = bookmark.shortcut;
            }
            return {
              label: bookmark.title,
              click: () => {
                gotoUrl(bookmark.url);
              },
              accelerator: accelerator,
            };
          } else if (bookmark.type === 'separator') {
            return {
              type: 'separator',
            };
          } else {
            throw Error(
              'A bookmarks menu entry has an invalid type; type must be one of "link", "separator".',
            );
          }
        }),
      };
      menuTemplate.splice(menuTemplate.length - 2, 0, bookmarksMenu);
    }
  } catch (err) {
    console.warn(
      'Failed to load & parse bookmarks configuration JSON file.',
      err,
    );
  }

  const menu = Menu.buildFromTemplate(menuTemplate);
  Menu.setApplicationMenu(menu);
}


Review comment (suggestion):
```suggestion
      // Insert custom bookmarks menu between menus "View" and "Window"
      menuTemplate.splice(menuTemplate.length - 2, 0, bookmarksMenu);
```

Code after:
              throw Error('Bookmark URL "' + bookmark.url + '"is invalid.');
            }
            let accelerator = null;
            if ('shortcut' in bookmark) {
              accelerator = bookmark.shortcut;
            }
            return {
              label: bookmark.title,
              click: () => {
                gotoUrl(bookmark.url);
              },
              accelerator: accelerator,
            };
          } else if (bookmark.type === 'separator') {
            return {
              type: 'separator',
            };
          } else {
            throw Error(
              'A bookmarks menu entry has an invalid type; type must be one of "link", "separator".',
            );
          }
        }),
      };
      // Insert custom bookmarks menu between menus "View" and "Window"
      menuTemplate.splice(menuTemplate.length - 2, 0, bookmarksMenu);
    }
  } catch (err) {
    console.warn(
      'Failed to load & parse bookmarks configuration JSON file.',
      err,
    );
  }

  const menu = Menu.buildFromTemplate(menuTemplate);
  Menu.setApplicationMenu(menu);
}
Repository: jiahaog/nativefier
File: src/build/buildIcon.ts (TypeScript)

Code before:
    } catch (error) {
      log.warn('Failed to convert icon to .png, skipping.', error);
      return;
    }
  }

  if (iconIsIcns(options.packager.icon)) {
    log.debug(
      'Building for macOS and icon is already a .icns, no conversion needed',
    );
  }

  if (!isOSX()) {
    log.warn(
      'Skipping icon conversion to .icns, conversion is only supported on macOS',
    );
    return;
  }

  try {
    if (!iconIsIcns(options.packager.icon)) {
      const iconPath = convertToIcns(options.packager.icon);
      options.packager.icon = iconPath;
    }
    if (options.nativefier.tray) {
      convertToTrayIcon(options.packager.icon);
    }
  } catch (error) {
    log.warn('Failed to convert icon to .icns, skipping.', error);
    options.packager.icon = undefined;
  }
  return;
}


Review comment (suggestion):
My only concern here is for the following scenario:

- Building a mac app
- With a tray icon
- On a non-Mac build machine
- (And may even be passing a `.icns` file to avoid errors)

Will now cause a recoverable error and no icon (which seems like it was the behavior before anyway). Since there is no degradation in functionality, this is fine, but I think we should probably update the `--tray` entry of `api.md` to document this known limitation.

Code after:
    } catch (error) {
      log.warn('Failed to convert icon to .png, skipping.', error);
      return;
    }
  }

  if (iconIsIcns(options.packager.icon)) {
    log.debug(
      'Building for macOS and icon is already a .icns, no conversion needed',
    );
  }

  if (!isOSX()) {
    log.warn(
      'Skipping icon conversion to .icns, conversion is only supported on macOS',
    );
    return;
  }

  try {
    if (!iconIsIcns(options.packager.icon)) {
      const iconPath = convertToIcns(options.packager.icon);
      options.packager.icon = iconPath;
    }
    if (options.nativefier.tray) {
      convertToTrayIcon(options.packager.icon);
    }
  } catch (error) {
    log.warn('Failed to convert icon to .icns, skipping.', error);
    options.packager.icon = undefined;
  }
}
Repository: jiahaog/nativefier
File: app/src/main.ts (TypeScript)

Code before:
      uploadToServer: true,
    });
  });
}

// quit if singleInstance mode and there's already another instance running
const shouldQuit = appArgs.singleInstance && !app.requestSingleInstanceLock();
if (shouldQuit) {
  app.quit();
} else {
  app.on('second-instance', () => {
    if (mainWindow) {
      if (!mainWindow.isVisible()) {
        // try
        mainWindow.show();
      }
      if (mainWindow.isMinimized()) {
        // minimized
        mainWindow.restore();
      }
      mainWindow.focus();
    }
  });

  if (appArgs.widevine !== undefined) {
    // @ts-ignore
    app.on('widevine-ready', (version, lastVersion) => {
      console.log('widevine-ready', version, lastVersion);
      ready();
    });

    // @ts-ignore
    app.on('widevine-update-pending', (currentVersion, pendingVersion) => {
      console.log(
        `Widevine ${currentVersion} is ready to be upgraded to ${pendingVersion}`,
      );
    });

    // @ts-ignore
    app.on('widevine-error', (error) => {
      console.error('WIDEVINE ERROR: ', error);
    });
  } else {
    app.on('ready', () => {
      console.log('ready');
      ready();
    });
  }
}

function ready(): void {

Review comment (suggestion):
Here and below: I like to comment why `@ts-ignore` is used when it is. Pls inline comment each of these. TSC is fine with it. For example,

```suggestion
    // @ts-ignore because blah blah blah. Alternative is to foo, but we prefer bar.
```

Code after:

app.on('quit', (event, exitCode) => {
  log.debug('quit', event, exitCode);
});

if (appArgs.crashReporter) {
  app.on('will-finish-launching', () => {
    crashReporter.start({
      companyName: appArgs.companyName || '',
      productName: appArgs.name,
      submitURL: appArgs.crashReporter,
      uploadToServer: true,
    });
  });
}

// quit if singleInstance mode and there's already another instance running
const shouldQuit = appArgs.singleInstance && !app.requestSingleInstanceLock();
if (shouldQuit) {
  app.quit();
} else {
  app.on('second-instance', () => {
    if (mainWindow) {
      if (!mainWindow.isVisible()) {
        // try
        mainWindow.show();
      }
      if (mainWindow.isMinimized()) {
        // minimized
        mainWindow.restore();
      }
      mainWindow.focus();
    }
  });

  if (appArgs.widevine !== undefined) {
    // @ts-ignore This event only appear on the widevine version of electron, which we'd see at runtime
    app.on('widevine-ready', (version, lastVersion) => {
      console.log('widevine-ready', version, lastVersion);
      onReady();
    });

    // @ts-ignore This event only appear on the widevine version of electron, which we'd see at runtime
    app.on('widevine-update-pending', (currentVersion, pendingVersion) => {
      console.log(
        `Widevine ${currentVersion} is ready to be upgraded to ${pendingVersion}`,
      );
    });

    // @ts-ignore This event only appear on the widevine version of electron, which we'd see at runtime
    app.on('widevine-error', (error) => {
Repository: jiahaog/nativefier
File: app/src/components/menu.ts (TypeScript)

Code before:
      },
      {
        label: 'Clear App Data',
        click: (item: MenuItem, focusedWindow: BrowserWindow) => {
          log.debug('Clear App Data.click', {
            item,
            focusedWindow,
            mainWindow,
          });
          if (!focusedWindow) {
            focusedWindow = mainWindow;
          }
          clearAppData(focusedWindow).catch((err) =>
            log.error('clearAppData ERROR', err),
          );
        },
      },
    ],
  };

  const viewMenu: MenuItemConstructorOptions = {
    label: '&View',
    submenu: [
      {
        label: 'Back',
        accelerator: isOSX() ? 'CmdOrAlt +Left' : 'Alt+Left',
        click: goBack,
      },
      {
        label: 'BackAdditionalShortcut',
        visible: false,
        acceleratorWorksWhenHidden: true,
        accelerator: 'CmdOrCtrl+[', // What old versions of Nativefier used, kept for backwards compat
        click: goBack,
      },
      {
        label: 'Forward',
        accelerator: isOSX() ? 'Cmd+Right' : 'Alt+Right',
        click: goForward,
      },
      {
        label: 'ForwardAdditionalShortcut',
        visible: false,
        acceleratorWorksWhenHidden: true,
        accelerator: 'CmdOrCtrl+]', // What old versions of Nativefier used, kept for backwards compat
        click: goForward,
      },
      {
        label: 'Reload',
        role: 'reload',
      },

Review comment (suggestion):
```suggestion
        accelerator: isOSX() ? 'CmdOrAlt+Left' : 'Alt+Left',
```

Code after:
      },
      {
        label: 'Clear App Data',
        click: (item: MenuItem, focusedWindow: BrowserWindow) => {
          log.debug('Clear App Data.click', {
            item,
            focusedWindow,
            mainWindow,
          });
          if (!focusedWindow) {
            focusedWindow = mainWindow;
          }
          clearAppData(focusedWindow).catch((err) =>
            log.error('clearAppData ERROR', err),
          );
        },
      },
    ],
  };

  const viewMenu: MenuItemConstructorOptions = {
    label: '&View',
    submenu: [
      {
        label: 'Back',
        accelerator: isOSX() ? 'CmdOrAlt+Left' : 'Alt+Left',
        click: goBack,
      },
      {
        label: 'BackAdditionalShortcut',
        visible: false,
        acceleratorWorksWhenHidden: true,
        accelerator: 'CmdOrCtrl+[', // What old versions of Nativefier used, kept for backwards compat
        click: goBack,
      },
      {
        label: 'Forward',
        accelerator: isOSX() ? 'Cmd+Right' : 'Alt+Right',
        click: goForward,
      },
      {
        label: 'ForwardAdditionalShortcut',
        visible: false,
        acceleratorWorksWhenHidden: true,
        accelerator: 'CmdOrCtrl+]', // What old versions of Nativefier used, kept for backwards compat
        click: goForward,
      },
      {
        label: 'Reload',
        role: 'reload',
      },
Repository: jiahaog/nativefier
File: src/constants.ts (TypeScript)

Code before:
import * as path from 'path';

export const DEFAULT_APP_NAME = 'APP';

// Update both DEFAULT_ELECTRON_VERSION and DEFAULT_CHROME_VERSION together,
// and update app / package.json / devDeps / electron to value of DEFAULT_ELECTRON_VERSION
export const DEFAULT_ELECTRON_VERSION = '13.1.7';
export const DEFAULT_CHROME_VERSION = '91.0.4472.124';

// Update each of these periodically
// https://product-details.mozilla.org/1.0/firefox_versions.json
export const DEFAULT_FIREFOX_VERSION = '89.0.2';

// https://en.wikipedia.org/wiki/Safari_version_history
export const DEFAULT_SAFARI_VERSION = {
  majorVersion: 14,
  version: '14.1.1',
  webkitVersion: '611.2.7.1.4',
};

export const ELECTRON_MAJOR_VERSION = parseInt(
  DEFAULT_ELECTRON_VERSION.split('.')[0],
  10,
);
export const PLACEHOLDER_APP_DIR = path.join(__dirname, './../', 'app');


Review comment (suggestion):
```suggestion
export const DEFAULT_FIREFOX_VERSION = '90.0';
```

Code after:
import * as path from 'path';

export const DEFAULT_APP_NAME = 'APP';

// Update both DEFAULT_ELECTRON_VERSION and DEFAULT_CHROME_VERSION together,
// and update app / package.json / devDeps / electron to value of DEFAULT_ELECTRON_VERSION
export const DEFAULT_ELECTRON_VERSION = '13.1.7';
export const DEFAULT_CHROME_VERSION = '91.0.4472.124';

// Update each of these periodically
// https://product-details.mozilla.org/1.0/firefox_versions.json
export const DEFAULT_FIREFOX_VERSION = '90.0';

// https://en.wikipedia.org/wiki/Safari_version_history
export const DEFAULT_SAFARI_VERSION = {
  majorVersion: 14,
  version: '14.1.1',
  webkitVersion: '611.2.7.1.4',
};

export const ELECTRON_MAJOR_VERSION = parseInt(
  DEFAULT_ELECTRON_VERSION.split('.')[0],
  10,
);
export const PLACEHOLDER_APP_DIR = path.join(__dirname, './../', 'app');
Repository: jiahaog/nativefier
File: app/src/helpers/helpers.ts (TypeScript)

Code before:
  url: string,
  options?: OpenExternalOptions,
): Promise<void> {
  log.debug('openExternal', { url, options });
  return shell.openExternal(url, options);
}

export function removeUserAgentSpecifics(
  userAgentFallback: string,
  appName: string,
  appVersion: string,
): string {
  // Electron userAgentFallback is the user agent used if none is specified when creating a window.
  // For our purposes, it's useful because its format is similar enough to a real Chrome's user agent to not need
  // to infer the userAgent. userAgentFallback normally looks like this:
  // Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) app-nativefier-804458/1.0.0 Chrome/89.0.4389.128 Electron/12.0.7 Safari/537.36
  // We just need to strip out the appName/1.0.0 and Electron/electronVersion
  return userAgentFallback
    .replace(`Electron/${process.versions.electron} `, '')
    .replace(`${appName}/${appVersion} `, ' ');
}

// Removes formatting from a text
export function getPlainTextWithoutFormatting(text: string): string {
  return text.trim().replace(/\n/g, ' ').replace(/\s+/g, ' ');
}


Review comment (suggestion):
Uuuuuh, unless I'm missing something, this is not what's commonly understood as "copy as plain text", as implemented in popular text-processing programs. Copy as plain text means *stripping rich text* (a.k.a. HTML in a web context), so that e.g. ***bold italic*** text get pasted without the boldness/italicness.

If you rework this PR and are looking for to find a way to do this, it feels like there must be a way to do it with pure web/Electron APIs. Please don't use an extra dep (see [HACKING.md / 4. Avoid adding npm dependencies](https://github.com/nativefier/nativefier/blob/master/HACKING.md))

Code after:
  url: string,
  options?: OpenExternalOptions,
): Promise<void> {
  log.debug('openExternal', { url, options });
  return shell.openExternal(url, options);
}

export function removeUserAgentSpecifics(
  userAgentFallback: string,
  appName: string,
  appVersion: string,
): string {
  // Electron userAgentFallback is the user agent used if none is specified when creating a window.
  // For our purposes, it's useful because its format is similar enough to a real Chrome's user agent to not need
  // to infer the userAgent. userAgentFallback normally looks like this:
  // Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) app-nativefier-804458/1.0.0 Chrome/89.0.4389.128 Electron/12.0.7 Safari/537.36
  // We just need to strip out the appName/1.0.0 and Electron/electronVersion
  return userAgentFallback
    .replace(`Electron/${process.versions.electron} `, '')
    .replace(`${appName}/${appVersion} `, ' ');
}

/** Removes extra spaces from a text */
export function cleanupPlainText(text: string): string {
  return text.trim().replace(/\s+/g, ' ');
}