Prevent duplicate separators for all context menu items

app/browser/menu.js

@@ -20,7 +20,7 @@ const siteTags = require('../../js/constants/siteTags')
 const dialog = electron.dialog
 const BrowserWindow = electron.BrowserWindow
 const {fileUrl} = require('../../js/lib/appUrlUtil')
-const menuUtil = require('./lib/menuUtil')
+const menuUtil = require('../common/lib/menuUtil')
 const getSetting = require('../../js/settings').getSetting
 const locale = require('../locale')
 const {isSiteBookmarked} = require('../../js/state/siteUtil')
@@ -329,7 +329,7 @@ const createHistorySubmenu = () => {
       }
     }
   ]
-  submenu = submenu.concat(menuUtil.createRecentlyClosedMenuItems(Immutable.fromJS(Object.keys(closedFrames).map(key => closedFrames[key]))))
+  submenu = submenu.concat(menuUtil.createRecentlyClosedTemplateItems(Immutable.fromJS(Object.keys(closedFrames).map(key => closedFrames[key]))))
 
   submenu.push(
     // TODO: recently visited
@@ -374,7 +374,7 @@ const createBookmarksSubmenu = () => {
     CommonMenu.importBrowserDataMenuItem()
   ]
 
-  const bookmarks = menuUtil.createBookmarkMenuItems(appStore.getState().get('sites'))
+  const bookmarks = menuUtil.createBookmarkTemplateItems(appStore.getState().get('sites'))
   if (bookmarks.length > 0) {
     submenu.push(CommonMenu.separatorMenuItem)
     submenu = submenu.concat(bookmarks)

app/common/lib/historyUtil.js

@@ -34,7 +34,7 @@ module.exports.groupEntriesByDay = (history, locale) => {
   const reduced = history.reduce((previousValue, currentValue, currentIndex, array) => {
     const result = currentIndex === 1 ? [] : previousValue
     if (currentIndex === 1) {
-      const firstDate = getDayString(currentValue, locale)
+      const firstDate = getDayString(previousValue, locale)
       result.push({date: firstDate, entries: [previousValue]})
     }
     const date = getDayString(currentValue, locale)

app/browser/lib/menuUtil.js → app/common/lib/menuUtil.js

@@ -69,7 +69,7 @@ module.exports.setTemplateItemChecked = (template, label, checked) => {
   return null
 }
 
-const createBookmarkMenuItems = (bookmarks, parentFolderId) => {
+const createBookmarkTemplateItems = (bookmarks, parentFolderId) => {
   const filteredBookmarks = parentFolderId
     ? bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === parentFolderId)
     : bookmarks.filter((bookmark) => !bookmark.get('parentFolderId'))
@@ -99,23 +99,26 @@ const createBookmarkMenuItems = (bookmarks, parentFolderId) => {
       const submenuItems = bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === folderId)
       payload.push({
         label: site.get('customTitle') || site.get('title'),
-        submenu: submenuItems.count() > 0 ? createBookmarkMenuItems(bookmarks, folderId) : null
+        submenu: submenuItems.count() > 0 ? createBookmarkTemplateItems(bookmarks, folderId) : null
       })
     }
   })
   return payload
 }
 
 /**
- * Add bookmarks / folders to "Bookmarks" menu
+ * Used to create bookmarks and bookmark folder entries for the "Bookmarks" menu
  *
  * @param sites The application state's Immutable sites list
  */
-module.exports.createBookmarkMenuItems = (sites) => {
-  return createBookmarkMenuItems(siteUtil.getBookmarks(sites))
+module.exports.createBookmarkTemplateItems = (sites) => {
+  return createBookmarkTemplateItems(siteUtil.getBookmarks(sites))
 }
 
-module.exports.createRecentlyClosedMenuItems = (lastClosedFrames) => {
+/**
+ * Create "recently closed" history entries for the "History" menu
+ */
+module.exports.createRecentlyClosedTemplateItems = (lastClosedFrames) => {
   const payload = []
   if (lastClosedFrames && lastClosedFrames.size > 0) {
     payload.push(
@@ -141,3 +144,40 @@ module.exports.createRecentlyClosedMenuItems = (lastClosedFrames) => {
   }
   return payload
 }
+
+const isItemValid = (currentItem, previousItem) => {
+  if (previousItem && previousItem === CommonMenu.separatorMenuItem) {
+    if (currentItem === CommonMenu.separatorMenuItem) {
+      return false
+    }
+  }
+  return currentItem && (typeof currentItem.label === 'string' || typeof currentItem.type === 'string')
+}
+
+/**
+ * Remove invalid entries from a menu template:
+ * - null or falsey entries
+ * - extra menu separators
+ * - entries which don't have a label or type
+ */
+module.exports.sanitizeTemplateItems = (template) => {
+  const result = template.reduce((previousValue, currentValue, currentIndex, array) => {
+    const result = currentIndex === 1 ? [] : previousValue
+    if (currentIndex === 1) {
+      if (isItemValid(previousValue)) {
+        result.push(previousValue)
+      }
+    }
+    const previousItem = result.length > 0
+      ? result[result.length - 1]
+      : undefined
+    if (isItemValid(currentValue, previousItem)) {
+      result.push(currentValue)
+    }
+    return result
+  })
+
+  return Array.isArray(result)
+    ? result
+    : [result]
+}