Dragging to blank area of tab strip sets correct destination index

Fix #14046
Avoid a tab being moved to a new window with a frame index higher than it should have. This occurred because when dropping on the blank area of a tab strip, there is no tab being dragged over. The last tab being dragged over could have been in another window, or could be the dragged tab itself, causing a frame to be created in the new window with an index that is too high, corrupting the "frame index <-> tabId" cache. Instead, we insert the tab at the beginning, and rely on muon to provide an updated tab index (if provided with an index of -1, muon will add the tab to the end of the strip).

app/browser/reducers/tabsReducer.js

@@ -528,15 +528,21 @@ const tabsReducer = (state, action, immutableAction) => {
       if (dragData && dragData.get('type') === dragTypes.TAB) {
         const frame = dragData.get('data')
         let frameOpts = frameOptsFromFrame(frame)
+        const draggingTabId = frameOpts.get('tabId')
         const browserOpts = { positionByMouseCursor: true, checkMaximized: true }
         const tabIdForIndex = dragData.getIn(['dragOverData', 'draggingOverKey'])
-        const tabForIndex = tabState.getByTabId(state, tabIdForIndex)
+        const tabForIndex = tabIdForIndex !== draggingTabId && tabState.getByTabId(state, tabIdForIndex)
         const dropWindowId = dragData.get('dropWindowId')
-        if (dropWindowId != null && dropWindowId !== -1 && tabForIndex) {
+        let newIndex = -1
+        // Set new index for new window if last dragged-over tab is in new window.
+        // Otherwise, could be over another tab's tab strip, but most recently dragged-over a tab in another window.
+        if (dropWindowId != null && dropWindowId !== -1 && tabForIndex && tabForIndex.get('windowId') === dropWindowId) {
           const prependIndexByTabId = dragData.getIn(['dragOverData', 'draggingOverLeftHalf'])
-          frameOpts = frameOpts.set('index', tabForIndex.get('index') + (prependIndexByTabId ? 0 : 1))
+          newIndex = tabForIndex.get('index') + (prependIndexByTabId ? 0 : 1)
         }
-        tabs.moveTo(state, frame.get('tabId'), frameOpts, browserOpts, dragData.get('dropWindowId'))
+        // ensure the tab never moves window with its original index
+        frameOpts = frameOpts.set('index', newIndex)
+        tabs.moveTo(state, draggingTabId, frameOpts, browserOpts, dragData.get('dropWindowId'))
       }
       break
     }

app/browser/tabs.js

@@ -1063,12 +1063,11 @@ const api = {
   },
 
   moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => {
-
-    if (shouldDebugTabEvents) {
-      console.log(`Tab [${tabId}] tabs.moveTo(window: ${toWindowId})`)
-    }
     frameOpts = makeImmutable(frameOpts)
     browserOpts = makeImmutable(browserOpts)
+    if (shouldDebugTabEvents) {
+      console.log(`Tab [${tabId}] tabs.moveTo(window: ${toWindowId}, index: ${frameOpts.get('index')})`)
+    }
     const tab = webContentsCache.getWebContents(tabId)
     if (!tab || tab.isDestroyed()) {
       return
@@ -1125,6 +1124,7 @@ const api = {
     // invalid index? add to end of tab strip by specifying -1 index
     if (toIndex == null || toIndex === -1) {
       toIndex = -1
+      frameOpts = frameOpts.set('index', -1)
     }
     const win = getWindow(toWindowId)
     if (!win || win.isDestroyed()) {

app/renderer/reducers/frameReducer.js

@@ -84,7 +84,12 @@ const frameReducer = (state, action, immutableAction) => {
       const tabId = immutableAction.get('tabId')
       const index = frameStateUtil.getIndexByTabId(state, tabId)
       if (index === -1) {
-        console.error('frame not found for tab inserted to tab strip', tabId, state.get('frames').toJS())
+        console.error(
+          'frame not found for tab inserted to tab strip',
+          tabId,
+          (state.get('frames') || Immutable.Map()).toJS(),
+          (state.get('framesInternal') || Immutable.Map()).toJS()
+        )
         break
       }
       state = state.mergeIn(['frames', index], {

js/stores/windowStore.js

@@ -112,10 +112,16 @@ const newFrame = (state, frameOpts) => {
   }
   frameOpts = frameOpts.toJS ? frameOpts.toJS() : frameOpts
   // handle tabs.create properties
-  let insertionIndex = frameOpts.index !== undefined
+
+  // Ensure valid index
+  let insertionIndex = frameOpts.index != null
     ? frameOpts.index
     : 0
 
+  if (insertionIndex === -1) {
+    frameOpts.index = insertionIndex = 0
+  }
+
   if (frameOpts.partition) {
     frameOpts.isPrivate = frameStateUtil.isPrivatePartition(frameOpts.partition)
     if (frameStateUtil.isSessionPartition(frameOpts.partition)) {

test/unit/app/browser/reducers/tabsReducerTest.js

@@ -596,10 +596,12 @@ describe('tabsReducer unit tests', function () {
       assert.equal(args[0], state)  // State is passed in as first arg
       assert.equal(args[1], 1)  // tabId is 1 for first tab
       // frameOpts being dragged is for the first tab
-      assert.deepEqual(args[2].toJS(), { tabId: 1,
+      assert.deepEqual(args[2].toJS(), {
+        tabId: 1,
         windowId: 1,
         pinned: false,
-        active: true
+        active: true,
+        index: -1 // -1 specifies should go at end of tab strip
       })
       // Passes browser options for position by mouse cursor
       assert.deepEqual(args[3], {