fix(draft-speed): Optimize draft creation and reduce scroll / focus delays

Summary:
This diff attempts to improve the responsiveness of the app when you hit "Reply". This is achieved by being smarter about creating the draft and loading it into the draft store, and also by allowing the compose* actions to take objects instead of just IDs (resulting in a fetch of the object).

Allow Actions.composeReply,etc. to optionally be called with thread and message objects instead of IDs. This prevents a database lookup and the data is "right there."

Create DraftStoreProxy for new drafts optimistically—this allows us to hand it the draft model we just created and it doesn't have to go query for it

When we create a new Draft, immediately bind it to a LocalId. This means that when the MessageStore receives the trigger() event from the Database, it doesn't have to wait while a localId is created

When MessageStore sees a new Message come in which is on the current thread, a draft, and not in the localIds map, assume it's a new draft and shortcut fetchFromCaceh to manually add it to the items array and display. This means the user sees the...

...draft instantly.

Remove delays from focusing draft, scrolling to draft after content is ready. I actually removed these thinking it would break something, and it didn't break anything.... Maybe new Chromium handles better?

Fix specs

Test Plan: Run specs - more in progress right now

Reviewers: evan

Reviewed By: evan

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

internal_packages/message-list/lib/message-item.cjsx

@@ -153,16 +153,13 @@ class MessageItem extends React.Component
     </div>
 
   _onReply: =>
-    tId = @props.thread.id; mId = @props.message.id
+    Actions.composeReply(thread: @props.thread, message: @props.message)
-    Actions.composeReply(threadId: tId, messageId: mId) if (tId and mId)
 
   _onReplyAll: =>
-    tId = @props.thread.id; mId = @props.message.id
+    Actions.composeReplyAll(thread: @props.thread, message: @props.message)
-    Actions.composeReplyAll(threadId: tId, messageId: mId) if (tId and mId)
 
   _onForward: =>
-    tId = @props.thread.id; mId = @props.message.id
+    Actions.composeForward(thread: @props.thread, message: @props.message)
-    Actions.composeForward(threadId: tId, messageId: mId) if (tId and mId)
 
   _onReport: (issueType) =>
     {Contact, Message, DatabaseStore, NamespaceStore} = require 'nylas-exports'

internal_packages/message-list/lib/message-list.cjsx

@@ -46,12 +46,12 @@ class MessageList extends React.Component
     else
       newDraftIds = @_newDraftIds(prevState)
       newMessageIds = @_newMessageIds(prevState)
-      if newDraftIds.length > 0
+      if newMessageIds.length > 0
+        @_prepareContentForDisplay()
+      else if newDraftIds.length > 0
         @_focusDraft(@refs["composerItem-#{newDraftIds[0]}"])
         @_prepareContentForDisplay()
-      else if newMessageIds.length > 0
+      
-        @_prepareContentForDisplay()
-
   _newDraftIds: (prevState) =>
     oldDraftIds = _.map(_.filter((prevState.messages ? []), (m) -> m.draft), (m) -> m.id)
     newDraftIds = _.map(_.filter((@state.messages ? []), (m) -> m.draft), (m) -> m.id)
@@ -63,12 +63,7 @@ class MessageList extends React.Component
     return _.difference(newMessageIds, oldMessageIds) ? []
 
   _focusDraft: (draftElement) =>
-    # We need a 100ms delay so the DOM can finish painting the elements on
+    draftElement.focus()
-    # the page. The focus doesn't work for some reason while the paint is in
-    # process.
-    _.delay =>
-      draftElement.focus()
-    ,100
 
   render: =>
     if not @state.currentThread?
@@ -114,16 +109,17 @@ class MessageList extends React.Component
   # Either returns "reply" or "reply-all"
   _replyType: =>
     lastMsg = _.last(_.filter((@state.messages ? []), (m) -> not m.draft))
-    if lastMsg?.cc.length is 0 and lastMsg?.to.length is 1
+    return "reply" if lastMsg?.cc.length is 0 and lastMsg?.to.length is 1
-      return "reply"
+    return "reply-all"
-    else return "reply-all"
 
   _onClickReplyArea: =>
     return unless @state.currentThread?.id
+    lastMsg = _.last(_.filter((@state.messages ? []), (m) -> not m.draft))
+
     if @_replyType() is "reply-all"
-      Actions.composeReplyAll(threadId: @state.currentThread.id)
+      Actions.composeReplyAll(thread: @state.currentThread, message: lastMsg)
     else
-      Actions.composeReply(threadId: @state.currentThread.id)
+      Actions.composeReply(thread: @state.currentThread,  message: lastMsg)
 
   # There may be a lot of iframes to load which may take an indeterminate
   # amount of time. As long as there is more content being painted onto
@@ -155,28 +151,29 @@ class MessageList extends React.Component
     scrollIfSettled()
 
   _messageComponents: =>
-    appliedInitialFocus = false
+    appliedInitialScroll = false
+    threadParticipants = @_threadParticipants()
     components = []
 
     @state.messages?.forEach (message, idx) =>
       collapsed = !@state.messagesExpandedState[message.id]
 
-      initialFocus = not appliedInitialFocus and not collapsed and
+      initialScroll = not appliedInitialScroll and not collapsed and
                     ((message.draft) or
                      (message.unread) or
                      (idx is @state.messages.length - 1 and idx > 0))
-      appliedInitialFocus ||= initialFocus
+      appliedInitialScroll ||= initialScroll
 
       className = classNames
         "message-item-wrap": true
-        "initial-focus": initialFocus
+        "initial-scroll": initialScroll
         "unread": message.unread
         "draft": message.draft
         "collapsed": collapsed
 
       if message.draft
         components.push <InjectedComponent matching={role:"Composer"}
-                         exposedProps={mode:"inline", localId:@state.messageLocalIds[message.id], onRequestScrollTo:@_onRequestScrollToComposer, threadId:@state.currentThread.id}
+                         exposedProps={ mode:"inline", localId:@state.messageLocalIds[message.id], onRequestScrollTo:@_onRequestScrollToComposer, threadId:@state.currentThread.id }
                          ref="composerItem-#{message.id}"
                          key={@state.messageLocalIds[message.id]}
                          className={className} />
@@ -186,7 +183,7 @@ class MessageList extends React.Component
                          message={message}
                          className={className}
                          collapsed={collapsed}
-                         thread_participants={@_threadParticipants()} />
+                         thread_participants={threadParticipants} />
 
       if idx < @state.messages.length - 1
         next = @state.messages[idx + 1]
@@ -198,7 +195,7 @@ class MessageList extends React.Component
 
     components
 
-  # Some child components (like the compser) might request that we scroll
+  # Some child components (like the composer) might request that we scroll
   # to a given location. If `selectionTop` is defined that means we should
   # scroll to that absolute position.
   #
@@ -234,14 +231,12 @@ class MessageList extends React.Component
     ready: if MessageStore.itemsLoading() then false else @state?.ready ? false
 
   _prepareContentForDisplay: =>
-    _.delay =>
+    node = React.findDOMNode(@)
-      node = React.findDOMNode(@)
+    return unless node
-      return unless node
+    initialScrollNode = node.querySelector(".initial-scroll")
-      focusedMessage = node.querySelector(".initial-focus")
+    @scrollToMessage initialScrollNode, =>
-      @scrollToMessage focusedMessage, =>
+      @setState(ready: true)
-        @setState(ready: true)
+    @_cacheScrollPos()
-      @_cacheScrollPos()
-    , 100
 
   _threadParticipants: =>
     # We calculate the list of participants instead of grabbing it from

internal_packages/message-list/spec/message-list-spec.cjsx

@@ -223,27 +223,20 @@ describe "MessageList", ->
       expect(@message_list._focusDraft).toHaveBeenCalled()
       expect(@message_list._focusDraft.mostRecentCall.args[0].props.exposedProps.localId).toEqual(draftMessages[0].id)
 
-    it "doesn't focus if we're just navigating through messages", ->
-      spyOn(@message_list, "scrollToMessage")
-      @message_list.setState messages: draftMessages
-      items = TestUtils.scryRenderedComponentsWithTypeAndProps(@message_list, InjectedComponent, matching: {role:"Composer"})
-      expect(items.length).toBe 1
-      composer = items[0]
-      expect(@message_list.scrollToMessage).not.toHaveBeenCalled()
-
-
   describe "MessageList with draft", ->
     beforeEach ->
       MessageStore._items = testMessages.concat draftMessages
       MessageStore.trigger(MessageStore)
-      @message_list.setState currentThread: test_thread
+      spyOn(@message_list, "_focusDraft")
+      @message_list.setState(currentThread: test_thread)
 
     it "renders the composer", ->
       items = TestUtils.scryRenderedComponentsWithTypeAndProps(@message_list, InjectedComponent, matching: {role:"Composer"})
       expect(@message_list.state.messages.length).toBe 6
       expect(items.length).toBe 1
 
-      expect(items.length).toBe 1
+    it "doesn't focus on initial load", ->
+      expect(@message_list._focusDraft).not.toHaveBeenCalled()
 
   describe "reply type", ->
     it "prompts for a reply when there's only one participant", ->

spec-nylas/stores/draft-store-spec.coffee

@@ -86,6 +86,7 @@ describe "DraftStore", ->
         return Promise.resolve(fakeMessage2) if query._klass is Message
         return Promise.reject(new Error('Not Stubbed'))
       spyOn(DatabaseStore, 'persistModel')
+      spyOn(DatabaseStore, 'bindToLocalId')
 
     describe "onComposeReply", ->
       beforeEach ->
@@ -235,6 +236,22 @@ describe "DraftStore", ->
         , (model) ->
           expect(model.constructor).toBe(Message)
 
+      it "should assign and save a local Id for the new message", ->
+        @_callNewMessageWithContext {threadId: fakeThread.id}
+        , (thread, message) ->
+          {}
+        , (model) ->
+          expect(DatabaseStore.bindToLocalId).toHaveBeenCalled()
+
+      it "should setup a draft session for the draftLocalId, so that a subsequent request for the session's draft resolves immediately.", ->
+        @_callNewMessageWithContext {threadId: fakeThread.id}
+        , (thread, message) ->
+          {}
+        , (model) ->
+          [draft, localId] = DatabaseStore.bindToLocalId.mostRecentCall.args
+          session = DraftStore.sessionForLocalId(localId).value()
+          expect(session.draft()).toBe(draft)
+
       it "should set the subject of the new message automatically", ->
         @_callNewMessageWithContext {threadId: fakeThread.id}
         , (thread, message) ->
@@ -250,6 +267,38 @@ describe "DraftStore", ->
         , (model) ->
           expect(model.subject).toEqual("Fwd: Fake subject")
 
+      describe "context", ->
+        it "should accept `thread` or look up a thread when given `threadId`", ->
+          @_callNewMessageWithContext {thread: fakeThread}
+          , (thread, message) ->
+            expect(thread).toBe(fakeThread)
+            expect(DatabaseStore.find).not.toHaveBeenCalled()
+            {}
+          , (model) ->
+
+          @_callNewMessageWithContext {threadId: fakeThread.id}
+          , (thread, message) ->
+            expect(thread).toBe(fakeThread)
+            expect(DatabaseStore.find).toHaveBeenCalled()
+            {}
+          , (model) ->
+
+        it "should accept `message` or look up a message when given `messageId`", ->
+          @_callNewMessageWithContext {thread: fakeThread, message: fakeMessage1}
+          , (thread, message) ->
+            expect(message).toBe(fakeMessage1)
+            expect(DatabaseStore.find).not.toHaveBeenCalled()
+            {}
+          , (model) ->
+
+          @_callNewMessageWithContext {thread: fakeThread, messageId: fakeMessage1.id}
+          , (thread, message) ->
+            expect(message).toBe(fakeMessage1)
+            expect(DatabaseStore.find).toHaveBeenCalled()
+            {}
+          , (model) ->
+
+
       describe "when a reply-to message is provided by the attributesCallback", ->
         it "should include quoted text in the new message", ->
           @_callNewMessageWithContext {threadId: fakeThread.id}

src/flux/stores/database-store.coffee

@@ -227,7 +227,7 @@ class DatabaseStore
     set = (change) =>
       clearTimeout(@_changeFireTimer) if @_changeFireTimer
       @_changeAccumulated = change
-      @_changeFireTimer = setTimeout(flush, 50)
+      @_changeFireTimer = setTimeout(flush, 20)
 
     concat = (change) =>
       @_changeAccumulated.objects.push(change.objects...)
@@ -490,7 +490,7 @@ class DatabaseStore
   #
   # Returns a {Promise} that resolves with the localId assigned to the model.
   #
-  bindToLocalId: (model, localId) =>
+  bindToLocalId: (model, localId = null) =>
     return Promise.reject(new Error("You must provide a model to bindToLocalId")) unless model
 
     new Promise (resolve, reject) =>
@@ -501,6 +501,8 @@ class DatabaseStore
           localId = generateTempId()
 
       link = new LocalLink({id: localId, objectId: model.id})
+      @_localIdLookupCache[model.id] = localId
+
       @persistModel(link).then ->
         resolve(localId)
       .catch(reject)
@@ -523,10 +525,7 @@ class DatabaseStore
           @_localIdLookupCache[model.id] = link.id
           resolve(link.id)
         else
-          @bindToLocalId(model).then (localId) =>
+          @bindToLocalId(model).then(resolve).catch(reject)
-            @_localIdLookupCache[model.id] = localId
-            resolve(localId)
-          .catch(reject)
 
   # Heavy Lifting
 

src/flux/stores/draft-store-proxy.coffee

@@ -74,19 +74,24 @@ class DraftStoreProxy
   @include Publisher
   @include Listener
 
-  constructor: (@draftLocalId) ->
+  constructor: (@draftLocalId, draft = null) ->
     DraftStore = require './draft-store'
 
     @listenTo DraftStore, @_onDraftChanged
     @listenTo Actions.didSwapModel, @_onDraftSwapped
 
-    @_draft = false
-    @_draftPromise = null
     @changes = new DraftChangeSet @draftLocalId, =>
       if !@_draft
         throw new Error("DraftChangeSet was modified before the draft was prepared.")
       @trigger()
 
+    if draft
+      @_draft = draft
+      @_draftPromise = Promise.resolve(@)
+    else
+      @_draft = false
+      @_draftPromise = null
+
     @prepare().catch (error) ->
       console.error(error)
       console.error(error.stack)

src/flux/stores/draft-store.coffee

@@ -18,7 +18,7 @@ Actions = require '../actions'
 
 TaskQueue = require './task-queue'
 
-{subjectWithPrefix} = require '../models/utils'
+{subjectWithPrefix, generateTempId} = require '../models/utils'
 
 {Listener, Publisher} = require '../modules/reflux-coffee'
 CoffeeHelpers = require '../coffee-helpers'
@@ -205,18 +205,30 @@ class DraftStore
     @_newMessageWithContext context, (thread, message) ->
       forwardMessage: message
 
-  _newMessageWithContext: ({threadId, messageId}, attributesCallback) =>
+  _newMessageWithContext: ({thread, threadId, message, messageId}, attributesCallback) =>
     return unless NamespaceStore.current()
 
+    # We accept all kinds of context. You can pass actual thread and message objects,
+    # or you can pass Ids and we'll look them up. Passing the object is preferable,
+    # and in most cases "the data is right there" anyway. Lookups add extra latency
+    # that feels bad.
     queries = {}
-    queries.thread = DatabaseStore.find(Thread, threadId)
+
-    if messageId?
+    if thread?
+      throw new Error("newMessageWithContext: `thread` present, expected a Model. Maybe you wanted to pass `threadId`?") unless thread instanceof Thread
+      queries.thread = thread
+    else
+      queries.thread = DatabaseStore.find(Thread, threadId)
+
+    if message?
+      throw new Error("newMessageWithContext: `message` present, expected a Model. Maybe you wanted to pass `messageId`?") unless message instanceof Message
+      queries.message = message
+    else if messageId?
       queries.message = DatabaseStore.find(Message, messageId)
+      queries.message.include(Message.attributes.body)
     else
       queries.message = DatabaseStore.findBy(Message, {threadId: threadId}).order(Message.attributes.date.descending()).limit(1)
-
+      queries.message.include(Message.attributes.body)
-    # Make sure message body is included
-    queries.message.include(Message.attributes.body)
 
     # Waits for the query promises to resolve and then resolve with a hash
     # of their resolved values. *swoon*
@@ -283,6 +295,15 @@ class DraftStore
         threadId: thread.id
         namespaceId: thread.namespaceId
 
+      # Normally we'd allow the DatabaseStore to create a localId, wait for it to
+      # commit a LocalLink and resolve, etc. but it's faster to create one now.
+      draftLocalId = generateTempId()
+
+      # Optimistically create a draft session and hand it the draft so that it
+      # doesn't need to do a query for it a second from now when the composer wants it.
+      @_draftSessions[draftLocalId] = new DraftStoreProxy(draftLocalId, draft)
+
+      DatabaseStore.bindToLocalId(draft, draftLocalId)
       DatabaseStore.persistModel(draft)
 
   # Eventually we'll want a nicer solution for inline attachments

src/flux/stores/message-store.coffee

@@ -59,6 +59,17 @@ MessageStore = Reflux.createStore
       if inDisplayedThread
         @_fetchFromCache()
 
+        # Are we most likely adding a new draft? If the item is a draft and we don't
+        # have it's local Id, optimistically add it to the set, resort, and trigger.
+        # Note: this can avoid 100msec+ of delay from "Reply" => composer onscreen,
+        item = change.objects[0]
+        if change.objects.length is 1 and item.draft is true and @_itemsLocalIds[item.id] is null
+          DatabaseStore.localIdForModel(item).then (localId) =>
+            @_itemsLocalIds[item.id] = localId
+            @_items.push(item)
+            @_items = @_sortItemsForDisplay(@_items)
+            @trigger()
+
     if change.objectClass is Thread.name
       updatedThread = _.find change.objects, (obj) => obj.id is @_thread.id
       if updatedThread