fix(selection): Fix issue where selecting offscreen thread (null) could break selection

Summary:
- ModelViewSelection shouldn't break if you accidentally call it with null

- ModelViewSelection methods should throw if you pass non-items, need to do this to narrow down other Sentry errors

Test Plan: Run new specs

Reviewers: evan

Reviewed By: evan

Differential Revision: https://phab.nylas.com/D1589

spec-nylas/database-view-spec.coffee

@@ -63,11 +63,11 @@ describe "DatabaseView", ->
       @view = new DatabaseView(Message, config)
       @view._pages =
         0:
-          items: [{id: 'a'}, {id: 'b'}, {id: 'c'}]
+          items: [new Thread(id: 'a'), new Thread(id: 'b'), new Thread(id: 'c')]
           metadata: {'a': 'a-metadata', 'b': 'b-metadata', 'c': 'c-metadata'}
           loaded: true
         1:
-          items: [{id: 'd'}, {id: 'e'}, {id: 'f'}]
+          items: [new Thread(id: 'd'), new Thread(id: 'e'), new Thread(id: 'f')]
           metadata: {'d': 'd-metadata', 'e': 'e-metadata', 'f': 'f-metadata'}
           loaded: true
       @view._count = 1
@@ -229,7 +229,7 @@ describe "DatabaseView", ->
         @view._pages = {}
         for i in [0..9]
           @view._pages[i] =
-            items: [{id: 'a'}, {id: 'b'}, {id: 'c'}]
+            items: [new Thread(id: 'a'), new Thread(id: 'b'), new Thread(id: 'c')]
             metadata: {'a': 'a-metadata', 'b': 'b-metadata', 'c': 'c-metadata'}
             loaded: true
 
@@ -274,7 +274,7 @@ describe "DatabaseView", ->
       beforeEach ->
         @view.retrievePage(0)
         @completeQuery = =>
-          @items = [{id: 'model-a'}, {id: 'model-b'}, {id: 'model-c'}]
+          @items = [new Thread(id: 'model-a'), new Thread(id: 'model-b'), new Thread(id: 'model-c')]
           @queries[0].resolve(@items)
           @queries = []
         spyOn(@view, 'loaded').andCallFake -> true

spec-nylas/model-view-selection-spec.coffee

@@ -50,6 +50,27 @@ describe "ModelViewSelection", ->
       @selection.clear()
       expect(@trigger).toHaveBeenCalled()
 
+  describe "remove", ->
+    beforeEach ->
+      @selection.set([@items[2], @items[4]])
+
+    it "should do nothing if called without a valid item", ->
+      @selection.remove(null)
+      @selection.remove(undefined)
+      @selection.remove(false)
+      expect(@selection.ids()).toEqual(['2', '4'])
+
+    it "should remove the item from the set", ->
+      @selection.remove(@items[2])
+      expect(@selection.ids()).toEqual(['4'])
+
+    it "should throw an exception if the item passed is not a model", ->
+      expect( => @selection.remove('hi')).toThrow()
+
+    it "should trigger", ->
+      @selection.remove()
+      expect(@trigger).toHaveBeenCalled()
+
   describe "updateModelReferences", ->
     it "should replace items in the selection with the matching provided items, if present", ->
       @selection.set([@items[2], @items[4], @items[7]])
@@ -64,6 +85,15 @@ describe "ModelViewSelection", ->
     beforeEach ->
       @selection.set([@items[2]])
 
+    it "should do nothing if called without a valid item", ->
+      @selection.toggle(null)
+      @selection.toggle(undefined)
+      @selection.toggle(false)
+      expect(@selection.ids()).toEqual(['2'])
+
+    it "should throw an exception if the item passed is not a model", ->
+      expect( => @selection.toggle('hi')).toThrow()
+
     it "should select the item if it is not selected", ->
       @selection.toggle(@items[3])
       expect(@selection.ids()).toEqual(['2', '3'])
@@ -82,6 +112,15 @@ describe "ModelViewSelection", ->
       @selection.expandTo(@items[2])
       expect(@selection.ids()).toEqual(['2'])
 
+    it "should do nothing if called without a valid item", ->
+      @selection.expandTo(null)
+      @selection.expandTo(undefined)
+      @selection.expandTo(false)
+      expect(@selection.ids()).toEqual([])
+
+    it "should throw an exception if the item passed is not a model", ->
+      expect( => @selection.expandTo('hi')).toThrow()
+
     it "should select all items from the last selected item to the provided item", ->
       @selection.set([@items[2], @items[5]])
       @selection.expandTo(@items[8])

src/components/injected-component.cjsx

@@ -88,11 +88,13 @@ class InjectedComponent extends React.Component
 
   focus: =>
     # Not forwarding event - just a method call
-    @refs.inner.focus() if @refs.inner.focus?
+    # Note that our inner may not be populated, and it may not have a focus method
+    @refs.inner.focus() if @refs.inner?.focus?
 
   blur: =>
     # Not forwarding an event - just a method call
-    @refs.inner.blur() if @refs.inner.blur?
+    # Note that our inner may not be populated, and it may not have a blur method
+    @refs.inner.blur() if @refs.inner?.blur?
 
   _getStateFromStores: (props) =>
     props ?= @props

src/flux/stores/model-view-selection.coffee

@@ -23,18 +23,25 @@ class ModelViewSelection
   set: (items) ->
     @_items = []
     for item in items
-      throw new Error("selectItems must be called with Models") unless item instanceof Model
+      throw new Error("set must be called with Models") unless item instanceof Model
       @_items.push(item)
     @trigger(@)
 
   updateModelReferences: (items = []) ->
     for newer in items
+      unless newer instanceof Model
+        console.error(JSON.stringify(newer))
+        throw new Error("updateModelReferences must be called with Models")
+
       for existing, idx in @_items
         if existing.id is newer.id
           @_items[idx] = newer
           break
 
   toggle: (item) ->
+    return unless item
+    throw new Error("toggle must be called with a Model") unless item instanceof Model
+
     without = _.reject @_items, (t) -> t.id is item.id
     if without.length < @_items.length
       @_items = without
@@ -43,12 +50,18 @@ class ModelViewSelection
     @trigger(@)
 
   remove: (item) ->
+    return unless item
+    throw new Error("remove must be called with a Model") unless item instanceof Model
+
     without = _.reject @_items, (t) -> t.id is item.id
     if without.length < @_items.length
       @_items = without
       @trigger(@)
 
   expandTo: (item) ->
+    return unless item
+    throw new Error("expandTo must be called with a Model") unless item instanceof Model
+
     if @_items.length is 0
       @_items.push(item)
     else