Merge pull request #9949 from brave/issue-9948

don’t return from reducer without state
brave · Jul 13, 2017 · d3f96bb · d3f96bb
2 parents 692b2b5 + 6957927
commit d3f96bb
Show file tree

Hide file tree

Showing 3 changed files with 148 additions and 15 deletions.
diff --git a/app/browser/menu.js b/app/browser/menu.js
@@ -359,12 +359,14 @@ const updateRecentlyClosedMenuItems = (state) => {
 
   // Update in-memory menu template (Windows)
   const oldTemplate = state.getIn(['menu', 'template'])
-  const historyMenuKey = oldTemplate.findKey(value =>
-    value.get('label') === locale.translation('history')
-  )
-  const newSubmenu = createHistorySubmenu()
-  const newTemplate = oldTemplate.setIn([historyMenuKey, 'submenu'], newSubmenu)
-  appActions.setMenubarTemplate(newTemplate)
+  if (oldTemplate) {
+    const historyMenuKey = oldTemplate.findKey(value =>
+      value.get('label') === locale.translation('history')
+    )
+    const newSubmenu = createHistorySubmenu()
+    const newTemplate = oldTemplate.setIn([historyMenuKey, 'submenu'], newSubmenu)
+    appActions.setMenubarTemplate(newTemplate)
+  }
 }
 
 const isCurrentLocationBookmarked = (state) => {
@@ -644,7 +646,7 @@ const doAction = (state, action) => {
     case windowConstants.WINDOW_UNDO_CLOSED_FRAME:
       {
         if (!lastClosedUrl) {
-          return
+          break
         }
         closedFrames = closedFrames.delete(lastClosedUrl)
         const nextLastFrame = closedFrames.last()
@@ -662,17 +664,20 @@ const doAction = (state, action) => {
     case appConstants.APP_TAB_CLOSE_REQUESTED:
       {
         action = makeImmutable(action)
-        const tab = getByTabId(state, action.get('tabId'))
-        const frame = tab && tab.get('frame')
-        if (tab && !tab.get('incognito') && frame && frameStateUtil.isValidClosedFrame(frame)) {
-          lastClosedUrl = tab.get('url')
-          closedFrames = closedFrames.set(tab.get('url'), tab.get('frame'))
-          updateRecentlyClosedMenuItems(state)
+        const tabId = action.get('tabId')
+        if (tabId) {
+          const tab = getByTabId(state, tabId)
+          const frame = tab && tab.get('frame')
+          if (tab && !tab.get('incognito') && frame && frameStateUtil.isValidClosedFrame(frame)) {
+            lastClosedUrl = tab.get('url')
+            closedFrames = closedFrames.set(tab.get('url'), tab.get('frame'))
+            updateRecentlyClosedMenuItems(state)
+          }
         }
         break
       }
     case appConstants.APP_APPLY_SITE_RECORDS:
-      if (action.records.find((record) => record.objectData === 'bookmark')) {
+      if (action.records && action.records.find((record) => record.objectData === 'bookmark')) {
         createMenu(state)
       }
       break
@@ -682,7 +687,7 @@ const doAction = (state, action) => {
       {
         if (action.tag === siteTags.BOOKMARK || action.tag === siteTags.BOOKMARK_FOLDER) {
           createMenu(state)
-        } else if (action.siteDetail.constructor === Immutable.List && action.tag === undefined) {
+        } else if (action.siteDetail && action.siteDetail.constructor === Immutable.List && action.tag === undefined) {
           let shouldRebuild = false
           action.siteDetail.forEach((site) => {
             const tag = site.getIn(['tags', 0])

diff --git a/test/unit/app/browser/menuTest.js b/test/unit/app/browser/menuTest.js
@@ -0,0 +1,122 @@
+/* global describe, it, before, after */
+const mockery = require('mockery')
+const Immutable = require('immutable')
+const assert = require('assert')
+const appConstants = require('../../../../js/constants/appConstants')
+const windowConstants = require('../../../../js/constants/windowConstants')
+const fakeElectron = require('../../lib/fakeElectron')
+const fakeAdBlock = require('../../lib/fakeAdBlock')
+
+require('../../braveUnit')
+
+describe('menu reducer unit tests', function () {
+  let menuReducer
+  before(function () {
+    mockery.enable({
+      warnOnReplace: false,
+      warnOnUnregistered: false,
+      useCleanCache: true
+    })
+
+    const fakeLocale = {
+      translation: (token) => { return token }
+    }
+
+    const fakeMenuUtil = {
+      createRecentlyClosedTemplateItems: (closedFrames) => {
+        return []
+      },
+      updateRecentlyClosedMenuItems: (menu, closedFrames) => {},
+      createBookmarkTemplateItems: (sites) => {
+        return []
+      },
+      getMenuItem: (menu, label) => {
+        return {
+          click: () => {}
+        }
+      }
+    }
+
+    mockery.registerMock('electron', fakeElectron)
+    mockery.registerMock('ad-block', fakeAdBlock)
+    mockery.registerMock('../../js/l10n', fakeLocale)
+    mockery.registerMock('../common/lib/menuUtil', fakeMenuUtil)
+    menuReducer = require('../../../../app/browser/menu')
+  })
+
+  after(function () {
+    mockery.disable()
+  })
+
+  const assertNoStateChange = (actionType) => {
+    const input = Immutable.fromJS({keyGoesHere: 'valueGoesHere'})
+    const output = menuReducer(input, {actionType})
+    assert.deepEqual(input, output)
+  }
+
+  describe('APP_SET_STATE', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(appConstants.APP_SET_STATE)
+    })
+  })
+
+  describe('WINDOW_SET_FOCUSED_FRAME', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(windowConstants.WINDOW_SET_FOCUSED_FRAME)
+    })
+  })
+
+  describe('APP_CHANGE_SETTING', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(appConstants.APP_CHANGE_SETTING)
+    })
+  })
+
+  describe('WINDOW_UNDO_CLOSED_FRAME', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(windowConstants.WINDOW_UNDO_CLOSED_FRAME)
+    })
+  })
+
+  describe('WINDOW_CLEAR_CLOSED_FRAMES', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(windowConstants.WINDOW_CLEAR_CLOSED_FRAMES)
+    })
+  })
+
+  describe('APP_TAB_CLOSE_REQUESTED', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(appConstants.APP_TAB_CLOSE_REQUESTED)
+    })
+  })
+
+  describe('APP_APPLY_SITE_RECORDS', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(appConstants.APP_APPLY_SITE_RECORDS)
+    })
+  })
+
+  describe('APP_ADD_SITE', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(appConstants.APP_ADD_SITE)
+    })
+  })
+
+  describe('APP_REMOVE_SITE', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(appConstants.APP_REMOVE_SITE)
+    })
+  })
+
+  describe('APP_ON_CLEAR_BROWSING_DATA', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(appConstants.APP_ON_CLEAR_BROWSING_DATA)
+    })
+  })
+
+  describe('WINDOW_CLICK_MENUBAR_SUBMENU', function () {
+    it('state is returned unchanged', function () {
+      assertNoStateChange(windowConstants.WINDOW_CLICK_MENUBAR_SUBMENU)
+    })
+  })
+})
diff --git a/test/unit/lib/fakeElectron.js b/test/unit/lib/fakeElectron.js
@@ -63,6 +63,12 @@ const fakeElectron = {
   dialog: {
     showOpenDialog: function () { }
   },
+  Menu: {
+    setApplicationMenu: (template) => {},
+    buildFromTemplate: (template) => {
+      return require('./fakeElectronMenu')
+    }
+  },
   shell: {
     openExternal: function () {
     },