fix(drafts): Draft syncing completely disabled to reduce code complexity

Summary:
fix(streaming): Reconnect every 30 seconds, always

Never accept drafts via any API source

fix(attachments): Fix for changes to open API

Get rid of shouldAbort, just let tasks decide what to do in cleanup

Never let SaveDraftTask run while another SaveDraftTask for same draft is running

Never used IPC

Ignore destroy draft 404

Moving draft store proxy to draft store level

Only block on older saves

Replace SaveDraftTask with SyncbackDraftTask, do saving directly from proxy

Never sync back ;-)

Fix specs

Alter SendDraftTask so that it can send an unsaved draft

Test Plan: Run tests

Reviewers: evan

Reviewed By: evan

Differential Revision: https://review.inboxapp.com/D1245

internal_packages/composer/lib/composer-view.cjsx

@@ -2,11 +2,11 @@ React = require 'react'
 _ = require 'underscore-plus'
 
 {Actions,
+ DraftStore,
  FileUploadStore,
  ComponentRegistry} = require 'inbox-exports'
 
 FileUploads = require './file-uploads.cjsx'
-DraftStoreProxy = require './draft-store-proxy'
 ContenteditableToolbar = require './contenteditable-toolbar.cjsx'
 ContenteditableComponent = require './contenteditable-component.cjsx'
 ParticipantsTextField = require './participants-text-field.cjsx'
@@ -67,7 +67,9 @@ ComposerView = React.createClass
       @_prepareForDraft()
 
   _prepareForDraft: ->
-    @_proxy = new DraftStoreProxy(@props.localId)
+    @_proxy = DraftStore.sessionForLocalId(@props.localId)
+    if @_proxy.draft()
+      @_onDraftChanged()
       
     @unlisteners = []
     @unlisteners.push @_proxy.listen(@_onDraftChanged)
@@ -256,7 +258,6 @@ ComposerView = React.createClass
           @_sendDraft({force: true})
       return
 
-    @_proxy.changes.commit()
     Actions.sendDraft(@props.localId)
 
   _destroyDraft: ->

spec-inbox/stores/task-queue-spec.coffee

@@ -119,7 +119,6 @@ describe "TaskQueue", ->
       taskToDie = makeRemoteFailed(new TaskSubclassA())
 
       spyOn(TaskQueue, "dequeue").andCallThrough()
-      spyOn(taskToDie, "abort")
 
       TaskQueue._queue = [taskToDie, @remoteFailed]
       TaskQueue.enqueue(new KillsTaskA())
@@ -127,7 +126,6 @@ describe "TaskQueue", ->
       expect(TaskQueue._queue.length).toBe 2
       expect(TaskQueue.dequeue).toHaveBeenCalledWith(taskToDie, silent: true)
       expect(TaskQueue.dequeue.calls.length).toBe 1
-      expect(taskToDie.abort).toHaveBeenCalled()
 
   describe "dequeue", ->
     beforeEach ->
@@ -147,37 +145,11 @@ describe "TaskQueue", ->
     it "throws an error if the task isn't found", ->
       expect( -> TaskQueue.dequeue("bad")).toThrow()
 
-    it "doesn't abort unstarted tasks", ->
+    it "calls cleanup on dequeued tasks", ->
-      spyOn(@unstartedTask, "abort")
-      TaskQueue.dequeue(@unstartedTask, silent: true)
-      expect(@unstartedTask.abort).not.toHaveBeenCalled()
-
-    it "aborts local tasks in progress", ->
-      spyOn(@localStarted, "abort")
-      TaskQueue.dequeue(@localStarted, silent: true)
-      expect(@localStarted.abort).toHaveBeenCalled()
-
-    it "aborts remote tasks in progress", ->
-      spyOn(@remoteStarted, "abort")
-      TaskQueue.dequeue(@remoteStarted, silent: true)
-      expect(@remoteStarted.abort).toHaveBeenCalled()
-
-    it "calls cleanup on aborted tasks", ->
       spyOn(@remoteStarted, "cleanup")
       TaskQueue.dequeue(@remoteStarted, silent: true)
       expect(@remoteStarted.cleanup).toHaveBeenCalled()
 
-    it "aborts stalled remote tasks", ->
-      spyOn(@remoteFailed, "abort")
-      TaskQueue.dequeue(@remoteFailed, silent: true)
-      expect(@remoteFailed.abort).toHaveBeenCalled()
-
-    it "doesn't abort if it's fully done", ->
-      TaskQueue._queue.push @remoteSuccess
-      spyOn(@remoteSuccess, "abort")
-      TaskQueue.dequeue(@remoteSuccess, silent: true)
-      expect(@remoteSuccess.abort).not.toHaveBeenCalled()
-
     it "moves it from the queue", ->
       TaskQueue.dequeue(@remoteStarted, silent: true)
       expect(TaskQueue._queue.length).toBe 3

spec-inbox/tasks/file-upload-task-spec.coffee

@@ -81,18 +81,38 @@ describe "FileUploadTask", ->
         expect(options.method).toBe('POST')
         expect(options.formData.file.value).toBe("Read Stream")
 
-    it "can abort the upload with the full file path", ->
-      spyOn(@task, "_getBytesUploaded").andReturn(100)
-      waitsForPromise => @task.performRemote().then =>
-        @task.abort()
-        expect(@req.abort).toHaveBeenCalled()
-        data = _.extend uploadData,
-          state: "aborted"
-          bytesUploaded: 100
-        expect(Actions.uploadStateChanged).toHaveBeenCalledWith(data)
-
     it "notifies when the file successfully uploaded", ->
       spyOn(@task, "_completedNotification").andReturn(100)
       waitsForPromise => @task.performRemote().then =>
         file = (new File).fromJSON(fileJSON)
         expect(@task._completedNotification).toHaveBeenCalledWith(file)
+
+
+  describe "cleanup", ->
+    it "should not do anything if the request has finished", ->
+      req = jasmine.createSpyObj('req', ['abort'])
+      reqSuccess = null
+      spyOn(atom.inbox, 'makeRequest').andCallFake (reqParams) ->
+        reqSuccess = reqParams.success
+        req
+
+      @task.performRemote()
+      reqSuccess([fileJSON])
+      @task.cleanup()
+      expect(req.abort).not.toHaveBeenCalled()
+
+    it "should cancel the request if it's in flight", ->
+      req = jasmine.createSpyObj('req', ['abort'])
+      spyOn(atom.inbox, 'makeRequest').andCallFake (reqParams) -> req
+      spyOn(Actions, "uploadStateChanged")
+
+      @task.performRemote()
+      @task.cleanup()
+
+      expect(req.abort).toHaveBeenCalled()
+      data = _.extend uploadData,
+        state: "aborted"
+        bytesUploaded: 0
+      expect(Actions.uploadStateChanged).toHaveBeenCalledWith(data)
+
+

spec-inbox/tasks/send-draft-spec.coffee

@@ -1,5 +1,5 @@
 Actions = require '../../src/flux/actions'
-SaveDraftTask = require '../../src/flux/tasks/save-draft'
+SyncbackDraftTask = require '../../src/flux/tasks/syncback-draft'
 SendDraftTask = require '../../src/flux/tasks/send-draft'
 DatabaseStore = require '../../src/flux/stores/database-store'
 {generateTempId} = require '../../src/flux/models/utils'
@@ -9,7 +9,7 @@ _ = require 'underscore-plus'
 
 describe "SendDraftTask", ->
   describe "shouldWaitForTask", ->
-    it "should return any SaveDraftTasks for the same draft", ->
+    it "should return any SyncbackDraftTasks for the same draft", ->
       @draftA = new Message
         version: '1'
         id: '1233123AEDF1'
@@ -30,8 +30,8 @@ describe "SendDraftTask", ->
           name: 'Dummy'
           email: 'dummy@inboxapp.com'
 
-      @saveA = new SaveDraftTask('localid-A')
+      @saveA = new SyncbackDraftTask('localid-A')
-      @saveB = new SaveDraftTask('localid-B')
+      @saveB = new SyncbackDraftTask('localid-B')
       @sendA = new SendDraftTask('localid-A')
 
       expect(@sendA.shouldWaitForTask(@saveA)).toBe(true)
@@ -40,8 +40,8 @@ describe "SendDraftTask", ->
     beforeEach ->
       TaskQueue._queue = []
       TaskQueue._completed = []
-      @saveTask = new SaveDraftTask('localid-A')
+      @saveTask = new SyncbackDraftTask('localid-A')
-      @saveTaskB = new SaveDraftTask('localid-B')
+      @saveTaskB = new SyncbackDraftTask('localid-B')
       @sendTask = new SendDraftTask('localid-A')
       @tasks = [@saveTask, @saveTaskB, @sendTask]
 
@@ -124,13 +124,33 @@ describe "SendDraftTask", ->
           expect(options.path).toBe("/n/#{@draft.namespaceId}/send")
           expect(options.method).toBe('POST')
 
-    it "should send the draft ID and version", ->
+    describe "when the draft has been saved", ->
-      waitsForPromise =>
+      it "should send the draft ID and version", ->
-        @task.performRemote().then =>
+        waitsForPromise =>
-          expect(atom.inbox.makeRequest.calls.length).toBe(1)
+          @task.performRemote().then =>
-          options = atom.inbox.makeRequest.mostRecentCall.args[0]
+            expect(atom.inbox.makeRequest.calls.length).toBe(1)
-          expect(options.body.version).toBe(@draft.version)
+            options = atom.inbox.makeRequest.mostRecentCall.args[0]
-          expect(options.body.draft_id).toBe(@draft.id)
+            expect(options.body.version).toBe(@draft.version)
+            expect(options.body.draft_id).toBe(@draft.id)
+
+    describe "when the draft has not been saved", ->
+      beforeEach ->
+        @draft = new Message
+          id: generateTempId()
+          namespaceId: 'A12ADE'
+          subject: 'New Draft'
+          draft: true
+          to:
+            name: 'Dummy'
+            email: 'dummy@inboxapp.com'
+        @task = new SendDraftTask(@draft)
+
+      it "should send the draft JSON", ->
+        waitsForPromise =>
+          @task.performRemote().then =>
+            expect(atom.inbox.makeRequest.calls.length).toBe(1)
+            options = atom.inbox.makeRequest.mostRecentCall.args[0]
+            expect(options.body).toEqual(@draft.toJSON())
 
     it "should pass returnsModel:true so that the draft is saved to the data store when returned", ->
       waitsForPromise =>

spec-inbox/tasks/syncback-draft-spec.coffee

@@ -9,7 +9,7 @@ Contact = require '../../src/flux/models/contact'
 DatabaseStore = require '../../src/flux/stores/database-store'
 TaskQueue = require '../../src/flux/stores/task-queue'
 
-SaveDraftTask = require '../../src/flux/tasks/save-draft'
+SyncbackDraftTask = require '../../src/flux/tasks/syncback-draft'
 
 inboxError =
   message: "No draft with public id bvn4aydxuyqlbmzowh4wraysg",
@@ -33,7 +33,7 @@ testData =
 localDraft = new Message _.extend {}, testData, {id: "local-id"}
 remoteDraft = new Message _.extend {}, testData, {id: "remoteid1234"}
 
-describe "SaveDraftTask", ->
+describe "SyncbackDraftTask", ->
   beforeEach ->
     spyOn(DatabaseStore, "findByLocalId").andCallFake (klass, localId) ->
       if localId is "localDraftId" then Promise.resolve(localDraft)
@@ -46,53 +46,19 @@ describe "SaveDraftTask", ->
     spyOn(DatabaseStore, "swapModel").andCallFake ->
       Promise.resolve()
 
-  describe "performLocal", ->
-    it "rejects if it isn't constructed with a draftLocalId", ->
-      task = new SaveDraftTask
-      waitsForPromise =>
-        task.performLocal().catch (error) ->
-          expect(error.message).toBeDefined()
-
-    it "does nothing if there are no new changes", ->
-      task = new SaveDraftTask("localDraftId")
-      waitsForPromise =>
-        task.performLocal().then ->
-          expect(DatabaseStore.persistModel).not.toHaveBeenCalled()
-
-    it "persists to the Database if there are new changes", ->
-      task = new SaveDraftTask("localDraftId", body: "test body")
-      waitsForPromise =>
-        task.performLocal().then ->
-          expect(DatabaseStore.persistModel).toHaveBeenCalled()
-          newBody = DatabaseStore.persistModel.calls[0].args[0].body
-          expect(newBody).toBe "test body"
-
-    it "does nothing if no draft can be found in the db", ->
-      task = new SaveDraftTask("missingDraftId")
-      waitsForPromise =>
-        task.performLocal().then ->
-          expect(DatabaseStore.persistModel).not.toHaveBeenCalled()
-
   describe "performRemote", ->
     beforeEach ->
       spyOn(atom.inbox, 'makeRequest').andCallFake (opts) ->
         opts.success(remoteDraft.toJSON()) if opts.success
 
-    it "does nothing if localOnly is set to true", ->
-      task = new SaveDraftTask("localDraftId", {}, localOnly: true)
-      waitsForPromise =>
-        task.performRemote().then ->
-          expect(DatabaseStore.findByLocalId).not.toHaveBeenCalled()
-          expect(atom.inbox.makeRequest).not.toHaveBeenCalled()
-
     it "does nothing if no draft can be found in the db", ->
-      task = new SaveDraftTask("missingDraftId")
+      task = new SyncbackDraftTask("missingDraftId")
       waitsForPromise =>
         task.performRemote().then ->
           expect(atom.inbox.makeRequest).not.toHaveBeenCalled()
 
     it "should start an API request with the Message JSON", ->
-      task = new SaveDraftTask("localDraftId")
+      task = new SyncbackDraftTask("localDraftId")
       waitsForPromise =>
         task.performRemote().then ->
           expect(atom.inbox.makeRequest).toHaveBeenCalled()
@@ -100,7 +66,7 @@ describe "SaveDraftTask", ->
           expect(reqBody.subject).toEqual testData.subject
 
     it "should do a PUT when the draft has already been saved", ->
-      task = new SaveDraftTask("remoteDraftId")
+      task = new SyncbackDraftTask("remoteDraftId")
       waitsForPromise =>
         task.performRemote().then ->
           expect(atom.inbox.makeRequest).toHaveBeenCalled()
@@ -109,7 +75,7 @@ describe "SaveDraftTask", ->
           expect(options.method).toBe('PUT')
 
     it "should do a POST when the draft is unsaved", ->
-      task = new SaveDraftTask("localDraftId")
+      task = new SyncbackDraftTask("localDraftId")
       waitsForPromise =>
         task.performRemote().then ->
           expect(atom.inbox.makeRequest).toHaveBeenCalled()
@@ -118,7 +84,7 @@ describe "SaveDraftTask", ->
           expect(options.method).toBe('POST')
 
     it "should pass returnsModel:false so that the draft can be manually removed/added to the database, accounting for its ID change", ->
-      task = new SaveDraftTask("localDraftId")
+      task = new SyncbackDraftTask("localDraftId")
       waitsForPromise =>
         task.performRemote().then ->
           expect(atom.inbox.makeRequest).toHaveBeenCalled()
@@ -126,14 +92,14 @@ describe "SaveDraftTask", ->
           expect(options.returnsModel).toBe(false)
 
     it "should swap the ids if we got a new one from the DB", ->
-      task = new SaveDraftTask("localDraftId")
+      task = new SyncbackDraftTask("localDraftId")
       waitsForPromise =>
         task.performRemote().then ->
           expect(DatabaseStore.swapModel).toHaveBeenCalled()
           expect(DatabaseStore.persistModel).not.toHaveBeenCalled()
 
     it "should not swap the ids if we're using a persisted one", ->
-      task = new SaveDraftTask("remoteDraftId")
+      task = new SyncbackDraftTask("remoteDraftId")
       waitsForPromise =>
         task.performRemote().then ->
           expect(DatabaseStore.swapModel).not.toHaveBeenCalled()
@@ -146,7 +112,7 @@ describe "SaveDraftTask", ->
         opts.error(testError(opts)) if opts.error
 
     it "resets the id", ->
-      task = new SaveDraftTask("remoteDraftId")
+      task = new SyncbackDraftTask("remoteDraftId")
       task.onAPIError(testError({}))
       waitsFor ->
         DatabaseStore.swapModel.calls.length > 0

src/atom.coffee

@@ -757,6 +757,11 @@ class Atom extends Model
     app.emit('will-exit')
     remote.process.exit(status)
 
+  showOpenDialog: (options, callback) ->
+    parentWindow = if process.platform is 'darwin' then null else @getCurrentWindow()
+    dialog = remote.require('dialog')
+    dialog.showOpenDialog(parentWindow, options, callback)
+
   showSaveDialog: (defaultPath, callback) ->
     parentWindow = if process.platform is 'darwin' then null else @getCurrentWindow()
     dialog = remote.require('dialog')

src/browser/edgehill-application.coffee

@@ -215,9 +215,6 @@ class AtomApplication
     @on 'application:quit', =>
       @quitting = true
       app.quit()
-    @on 'application:open-file-to-window', -> @promptForPath({type: 'file', to_window: true})
-    @on 'application:open-dev', -> @promptForPath(devMode: true)
-    @on 'application:open-safe', -> @promptForPath(safeMode: true)
     @on 'application:inspect', ({x,y, atomWindow}) ->
       atomWindow ?= @focusedWindow()
       atomWindow?.browserWindow.inspectElement(x, y)

src/flux/actions.coffee

@@ -1,4 +1,3 @@
-ipc = require 'ipc'
 Reflux = require 'reflux'
 
 # These actions are rebroadcast through the ActionBridge to all
@@ -46,9 +45,6 @@ windowActions = [
   # Fired when a dialog is opened and a file is selected
   "clearDeveloperConsole",
 
-  "openPathsSelected",
-  "savePathSelected",
-
   # Actions for Selection State
   "selectNamespaceId",
   "selectThreadId",
@@ -87,7 +83,7 @@ windowActions = [
   # Some file actions only need to be processed in their current window
   "attachFile",
   "abortUpload",
-  "persistUploadedFile", # This touches the DB, should only be in main window
+  "attachFileComplete",
   "removeFile",
   "fetchAndOpenFile",
   "fetchAndSaveFile",
@@ -106,10 +102,4 @@ Actions.windowActions = windowActions
 Actions.mainWindowActions = mainWindowActions
 Actions.globalActions = globalActions
 
-ipc.on "paths-to-open", (pathsToOpen=[]) ->
-  Actions.openPathsSelected(pathsToOpen)
-
-ipc.on "save-file-selected", (savePath) ->
-  Actions.savePathSelected(savePath)
-
 module.exports = Actions

src/flux/inbox-api.coffee

@@ -193,11 +193,11 @@ class InboxAPI
       Thread = require './models/thread'
       return @_shouldAcceptModelIfNewer(Thread, model)
 
-    # For some reason, we occasionally get a delta with:
+    # For the time being, we never accept drafts from the server. This single
-    # delta.object = 'message', delta.attributes.object = 'draft'
+    # change ensures that all drafts in the system are authored locally. To
+    # revert, change back to use _shouldAcceptModelIfNewer
     if classname is "draft" or model?.object is "draft"
-      Message = require './models/message'
+      return Promise.reject()
-      return @_shouldAcceptModelIfNewer(Message, model)
 
     Promise.resolve()
 

src/flux/inbox-long-connection.coffee

@@ -18,7 +18,7 @@ class InboxLongConnection
     @_emitter = new Emitter
     @_state = 'idle'
     @_req = null
-    @_reqPingInterval = null
+    @_reqForceReconnectInterval = null
     @_buffer = null
 
     @_deltas = []
@@ -86,9 +86,11 @@ class InboxLongConnection
     @_buffer = bufferJSONs[bufferJSONs.length - 1]
 
   start: ->
-    throw (new Error 'Cannot start polling without auth token.') unless @_inbox.APIToken
+    return if @_state is InboxLongConnection.State.Ended
     return if @_req
 
+    throw (new Error 'Cannot start polling without auth token.') unless @_inbox.APIToken
+
     console.log("Long Polling Connection: Starting....")
     @withCursor (cursor) =>
       return if @state is InboxLongConnection.State.Ended
@@ -122,18 +124,22 @@ class InboxLongConnection
       req.write("1")
 
       @_req = req
-      @_reqPingInterval = setInterval ->
-        req.write("1")
-      ,250
 
-  retry: ->
+      # Currently we have trouble identifying when the connection has closed.
+      # Instead of trying to fix that, just reconnect every 30 seconds.
+      @_reqForceReconnectInterval = setInterval =>
+        @retry(true)
+      ,30000
+
+  retry: (immediate = false) ->
     return if @_state is InboxLongConnection.State.Ended
     @setState(InboxLongConnection.State.Retrying)
-
     @cleanup()
+
+    startDelay = if immediate then 0 else 10000
     setTimeout =>
       @start()
-    , 10000
+    , startDelay
 
   end: ->
     console.log("Long Polling Connection: Closed.")
@@ -141,8 +147,8 @@ class InboxLongConnection
     @cleanup()
 
   cleanup: ->
-    clearInterval(@_reqPingInterval) if @_reqPingInterval
+    clearInterval(@_reqForceReconnectInterval) if @_reqForceReconnectInterval
-    @_reqPingInterval = null
+    @_reqForceReconnectInterval = null
     if @_req
       @_req.end()
       @_req.abort()

src/flux/models/utils.coffee

@@ -16,7 +16,7 @@ utils =
     SalesforceContact = require './salesforce-contact'
     SalesforceTask = require './salesforce-task'
 
-    SaveDraftTask = require '../tasks/save-draft'
+    SyncbackDraftTask = require '../tasks/syncback-draft'
     SendDraftTask = require '../tasks/send-draft'
     DestroyDraftTask = require '../tasks/destroy-draft'
     AddRemoveTagsTask = require '../tasks/add-remove-tags'
@@ -43,7 +43,7 @@ utils =
       'MarkMessageReadTask': MarkMessageReadTask
       'AddRemoveTagsTask': AddRemoveTagsTask
       'SendDraftTask': SendDraftTask
-      'SaveDraftTask': SaveDraftTask
+      'SyncbackDraftTask': SyncbackDraftTask
       'DestroyDraftTask': DestroyDraftTask
       'FileUploadTask': FileUploadTask
     }

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

@@ -1,4 +1,5 @@
-{Message, Actions,DraftStore} = require 'inbox-exports'
+Message = require '../models/message'
+Actions = require '../actions'
 EventEmitter = require('events').EventEmitter
 _ = require 'underscore-plus'
 
@@ -15,7 +16,11 @@ _ = require 'underscore-plus'
 #
 class DraftChangeSet
   constructor: (@localId, @_onChange) ->
+    @reset()
+ 
+  reset: ->
     @_pending = {}
+    clearTimeout(@_timer) if @_timer
     @_timer = null
 
   add: (changes, immediate) =>
@@ -28,9 +33,12 @@ class DraftChangeSet
       @_timer = setTimeout(@commit, 5000)
 
   commit: =>
-    @_pending.localId = @localId
+    return unless Object.keys(@_pending).length > 0
-    if Object.keys(@_pending).length > 1
+
-      Actions.saveDraft(@_pending)
+    DatabaseStore = require './database-store'
+    DatabaseStore.findByLocalId(Message, @localId).then (draft) =>
+      draft = @applyToModel(draft)
+      DatabaseStore.persistModel(draft)
       @_pending = {}
 
   applyToModel: (model) =>
@@ -51,30 +59,44 @@ module.exports =
 class DraftStoreProxy
 
   constructor: (@draftLocalId) ->
+    DraftStore = require './draft-store'
+
     @unlisteners = []
     @unlisteners.push DraftStore.listen(@_onDraftChanged, @)
     @unlisteners.push Actions.didSwapModel.listen(@_onDraftSwapped, @)
+
     @_emitter = new EventEmitter()
     @_draft = false
-    @_reloadDraft()
+    @_draftPromise = null
-
     @changes = new DraftChangeSet @draftLocalId, =>
       @_emitter.emit('trigger')
+    @prepare()
 
   draft: ->
     @changes.applyToModel(@_draft)
     @_draft
   
+  prepare: ->
+    @_draftPromise ?= new Promise (resolve, reject) =>
+      DatabaseStore = require './database-store'
+      DatabaseStore.findByLocalId(Message, @draftLocalId).then (draft) =>
+        @_draft = draft
+        @_emitter.emit('trigger')
+        resolve(@)
+      .catch(reject)
+    @_draftPromise
+
   listen: (callback, bindContext) ->
     eventHandler = (args) ->
       callback.apply(bindContext, args)
     @_emitter.addListener('trigger', eventHandler)
     return =>
       @_emitter.removeListener('trigger', eventHandler)
-      if @_emitter.listeners('trigger').length == 0
+
-        # Unlink ourselves from the stores/actions we were listening to
+  cleanup: ->
-        # so that we can be garbage collected
+    # Unlink ourselves from the stores/actions we were listening to
-        unlisten() for unlisten in @unlisteners
+    # so that we can be garbage collected
+    unlisten() for unlisten in @unlisteners
 
   _onDraftChanged: (change) ->
     # We don't accept changes unless our draft object is loaded
@@ -93,12 +115,3 @@ class DraftStoreProxy
     if change.oldModel.id is @_draft.id
       @_draft = change.newModel
       @_emitter.emit('trigger')
-
-  _reloadDraft: ->
-    promise = DraftStore.findByLocalId(@draftLocalId)
-    promise.catch (err) ->
-      console.log(err)
-    promise.then (draft) =>
-      @_draft = draft
-      @_emitter.emit('trigger')
-

src/flux/stores/draft-store.coffee

@@ -2,10 +2,10 @@ _ = require 'underscore-plus'
 moment = require 'moment'
 
 Reflux = require 'reflux'
+DraftStoreProxy = require './draft-store-proxy'
 DatabaseStore = require './database-store'
 NamespaceStore = require './namespace-store'
 
-SaveDraftTask = require '../tasks/save-draft'
 SendDraftTask = require '../tasks/send-draft'
 DestroyDraftTask = require '../tasks/destroy-draft'
 
@@ -21,6 +21,7 @@ Actions = require '../actions'
 #
 # Remember that a "Draft" is actually just a "Message" with draft: true.
 #
+
 module.exports =
 DraftStore = Reflux.createStore
   init: ->
@@ -32,18 +33,19 @@ DraftStore = Reflux.createStore
     @listenTo Actions.composePopoutDraft, @_onComposePopoutDraft
     @listenTo Actions.composeNewBlankDraft, @_onComposeNewBlankDraft
 
-    @listenTo Actions.saveDraft, @_onSaveDraft
     @listenTo Actions.sendDraft, @_onSendDraft
     @listenTo Actions.destroyDraft, @_onDestroyDraft
 
     @listenTo Actions.removeFile, @_onRemoveFile
-    @listenTo Actions.persistUploadedFile, @_onFileUploaded
+    @listenTo Actions.attachFileComplete, @_onAttachFileComplete
+    @_draftSessions = {}
 
   ######### PUBLIC #######################################################
 
   # Returns a promise
-  findByLocalId: (localId) ->
+  sessionForLocalId: (localId) ->
-    DatabaseStore.findByLocalId(Message, localId)
+    @_draftSessions[localId] ?= new DraftStoreProxy(localId)
+    @_draftSessions[localId]
 
   ########### PRIVATE ####################################################
 
@@ -127,55 +129,30 @@ DraftStore = Reflux.createStore
     atom.displayComposer(draftLocalId)
 
   _onDestroyDraft: (draftLocalId) ->
+    # Immediately reset any pending changes so no saves occur
+    @_draftSessions[draftLocalId]?.changes.reset()
+    delete @_draftSessions[draftLocalId]
+
+    # Queue the task to destroy the draft
     Actions.queueTask(new DestroyDraftTask(draftLocalId))
     atom.close() if atom.state.mode is "composer"
 
-  _onSaveDraft: (paramsWithLocalId) ->
-    params = _.clone(paramsWithLocalId)
-    draftLocalId = params.localId
-
-    if (not draftLocalId?) then throw new Error("Must call saveDraft with a localId")
-    delete params.localId
-
-    if _.size(params) > 0
-      task = new SaveDraftTask(draftLocalId, params)
-      Actions.queueTask(task)
-
   _onSendDraft: (draftLocalId) ->
-    Actions.queueTask(new SendDraftTask(draftLocalId))
+    # Immediately save any pending changes so we don't save after sending
-    atom.close() if atom.state.mode is "composer"
+    save = @_draftSessions[draftLocalId]?.changes.commit() ? Promise.resolve()
+    save.then ->
+      # Queue the task to send the draft
+      Actions.queueTask(new SendDraftTask(draftLocalId))
+      atom.close() if atom.state.mode is "composer"
 
-  _findDraft: (draftLocalId) ->
+  _onAttachFileComplete: ({file, messageLocalId}) ->
-    new Promise (resolve, reject) ->
+    @sessionForLocalId(messageLocalId).prepare().then (proxy) ->
-      DatabaseStore.findByLocalId(Message, draftLocalId)
+      files = proxy.draft().files ? []
-      .then (draft) ->
+      files.push(file)
-        if not draft? then reject("Can't find draft with id #{draftLocalId}")
+      proxy.changes.add({files}, true)
-        else resolve(draft)
-      .catch (error) -> reject(error)
-
-  # Receives:
-  #   file: - A `File` object
-  #   uploadData:
-  #     messageLocalId
-  #     filePath
-  #     fileSize
-  #     fileName
-  #     bytesUploaded
-  #     state - one of "started" "progress" "completed" "aborted" "failed"
-  _onFileUploaded: ({file, uploadData}) ->
-    @_findDraft(uploadData.messageLocalId)
-    .then (draft) ->
-      draft.files ?= []
-      draft.files.push(file)
-      DatabaseStore.persistModel(draft)
-      Actions.queueTask(new SaveDraftTask(uploadData.messageLocalId))
-    .catch (error) -> console.error(error, error.stack)
 
   _onRemoveFile: ({file, messageLocalId}) ->
-    @_findDraft(messageLocalId)
+    @sessionForLocalId(messageLocalId).prepare().then (proxy) ->
-    .then (draft) ->
+      files = proxy.draft().files ? []
-      draft.files ?= []
+      files = _.reject files, (f) -> f.id is file.id
-      draft.files = _.reject draft.files, (f) -> f.id is file.id
+      proxy.changes.add({files}, true)
-      DatabaseStore.persistModel(draft)
-      Actions.queueTask(new SaveDraftTask(uploadData.messageLocalId))
-    .catch (error) -> console.error(error, error.stack)

src/flux/stores/file-upload-store.coffee

@@ -35,17 +35,14 @@ FileUploadStore = Reflux.createStore
   _onAttachFile: ({messageLocalId}) ->
     @_verifyId(messageLocalId)
 
-    unlistenOpen = Actions.openPathsSelected.listen (pathsToOpen=[]) ->
+    # When the dialog closes, it triggers `Actions.pathsToOpen`
-      unlistenOpen?()
+    atom.showOpenDialog {properties: ['openFile', 'multiSelections']}, (pathsToOpen) ->
       pathsToOpen = [pathsToOpen] if _.isString(pathsToOpen)
       for path in pathsToOpen
         # When this task runs, we expect to hear `uploadStateChanged`
         # Actions.
         Actions.queueTask(new FileUploadTask(path, messageLocalId))
 
-    # When the dialog closes, it triggers `Actions.pathsToOpen`
-    ipc.send('command', 'application:open-file-to-window')
-
   # Receives:
   #   uploadData:
   #     messageLocalId - The localId of the message (draft) we're uploading to

src/flux/stores/task-queue.coffee

@@ -55,7 +55,6 @@ TaskQueue = Reflux.createStore
       throw new Error("You must queue a `Task` object")
 
     @_initializeTask(task)
-
     @_dequeueObsoleteTasks(task)
     @_queue.push(task)
     @_update() if not silent
@@ -63,10 +62,7 @@ TaskQueue = Reflux.createStore
   dequeue: (taskOrId={}, {silent}={}) ->
     task = @_parseArgs(taskOrId)
 
-    task.abort() if @_shouldAbort(task)
-
     task.queueState.isProcessing = false
-
     task.cleanup()
 
     @_queue.splice(@_queue.indexOf(task), 1)
@@ -116,14 +112,15 @@ TaskQueue = Reflux.createStore
     @_processQueue()
 
   _dequeueObsoleteTasks: (task) ->
-    for otherTask in @_queue
+    for otherTask in @_queue by -1
-      if otherTask? and task.shouldDequeueOtherTask(otherTask)
+      # Do not interrupt tasks which are currently processing
+      continue if otherTask.queueState.isProcessing
+      # Do not remove ourselves from the queue
+      continue if otherTask is task
+      # Dequeue tasks which our new task indicates it makes obsolete
+      if task.shouldDequeueOtherTask(otherTask)
         @dequeue(otherTask, silent: true)
 
-  _shouldAbort: (task) ->
-    task.queueState.isProcessing or
-    (task.queueState.performedLocal and not task.queueState.performedRemote)
-
   _taskIsBlocked: (task) ->
     _.any @_queue, (otherTask) ->
       task.shouldWaitForTask(otherTask) and task isnt otherTask

src/flux/tasks/destroy-draft.coffee

@@ -3,7 +3,7 @@ Message = require '../models/message'
 DatabaseStore = require '../stores/database-store'
 Actions = require '../actions'
 
-SaveDraftTask = require './save-draft'
+SyncbackDraftTask = require './syncback-draft'
 SendDraftTask = require './send-draft'
 FileUploadTask = require './file-upload-task'
 
@@ -12,12 +12,12 @@ class DestroyDraftTask extends Task
   constructor: (@draftLocalId) -> super
 
   shouldDequeueOtherTask: (other) ->
-    (other instanceof SaveDraftTask and other.draftLocalId is @draftLocalId) or
+    (other instanceof SyncbackDraftTask and other.draftLocalId is @draftLocalId) or
     (other instanceof SendDraftTask and other.draftLocalId is @draftLocalId) or
     (other instanceof FileUploadTask and other.draftLocalId is @draftLocalId)
 
   shouldWaitForTask: (other) ->
-    (other instanceof SaveDraftTask and other.draftLocalId is @draftLocalId)
+    (other instanceof SyncbackDraftTask and other.draftLocalId is @draftLocalId)
 
   performLocal: ->
     new Promise (resolve, reject) =>
@@ -25,8 +25,8 @@ class DestroyDraftTask extends Task
         return reject(new Error("Attempt to call DestroyDraftTask.performLocal without @draftLocalId"))
 
       DatabaseStore.findByLocalId(Message, @draftLocalId).then (draft) =>
-        DatabaseStore.unpersistModel(draft).then(resolve)
         @draft = draft
+        DatabaseStore.unpersistModel(draft).then(resolve)
 
   performRemote: ->
     new Promise (resolve, reject) =>
@@ -47,7 +47,7 @@ class DestroyDraftTask extends Task
 
   onAPIError: (apiError) ->
     inboxMsg = apiError.body?.message ? ""
-    if inboxMsg.indexOf("No draft found") >= 0
+    if apiError.statusCode is 404
       # Draft has already been deleted, this is not really an error
       return true
     else if inboxMsg.indexOf("is not a draft") >= 0

src/flux/tasks/file-upload-task.coffee

@@ -11,8 +11,8 @@ DatabaseStore = require '../stores/database-store'
 class FileUploadTask extends Task
 
   constructor: (@filePath, @messageLocalId) ->
-    @progress = null # The progress checking timer.
     super
+    @progress = null # The progress checking timer.
 
   performLocal: ->
     return Promise.reject(new Error("Must pass an absolute path to upload")) unless @filePath?.length
@@ -30,50 +30,57 @@ class FileUploadTask extends Task
         json: false
         returnsModel: true
         formData: @_formData()
-        success: (json) => @_onUploadSuccess(json, resolve)
         error: reject
+        success: (json) =>
+          # The Inbox API returns the file json wrapped in an array
+          file = (new File).fromJSON(json[0])
+          Actions.uploadStateChanged @_uploadData("completed")
+          @_completedNotification(file)
+
+          clearInterval(@progress)
+          @req = null
+          resolve()
 
       @progress = setInterval =>
         Actions.uploadStateChanged(@_uploadData("progress"))
       , 250
 
-  abort: ->
+  cleanup: ->
-    @req?.abort()
+    super
-    clearInterval(@progress)
-    Actions.uploadStateChanged(@_uploadData("aborted"))
 
-    setTimeout =>
+    # If the request is still in progress, notify observers that
-      Actions.fileAborted(@_uploadData("aborted"))
+    # we've failed.
-    , 1000 # To see the aborted state for a little bit
+    if @req
+      @req.abort()
+      clearInterval(@progress)
+      Actions.uploadStateChanged(@_uploadData("aborted"))
+      setTimeout =>
+        # To see the aborted state for a little bit
+        Actions.fileAborted(@_uploadData("aborted"))
+      , 1000
+
+  onAPIError: (apiError) ->
+    @_rollbackLocal()
+
+  onOtherError: (otherError) ->
+    @_rollbackLocal()
+
+  onTimeoutError: ->
+    # Do nothing. It could take a while.
     Promise.resolve()
 
-  onAPIError: (apiError) -> @_rollbackLocal()
-  onOtherError: (otherError) -> @_rollbackLocal()
-
-  onTimeoutError: -> Promise.resolve() # Do nothing. It could take a while.
-
   onOfflineError: (offlineError) ->
     msg = "You can't upload a file while you're offline."
     @_rollbackLocal(msg)
 
   _rollbackLocal: (msg) ->
     clearInterval(@progress)
+    @req = null
+
     msg ?= "There was a problem uploading this file. Please try again later."
     Actions.postNotification({message: msg, type: "error"})
     Actions.uploadStateChanged @_uploadData("failed")
 
-  _onUploadSuccess: (json, taskCallback) ->
-    clearInterval(@progress)
-
-    # The Inbox API returns the file json wrapped in an array
-    file = (new File).fromJSON(json[0])
-
-    Actions.uploadStateChanged @_uploadData("completed")
-
-    @_completedNotification(file)
-
-    taskCallback()
-
   # The `persistUploadFile` action affects the Database and should only be
   # heard in the main window.
   #
@@ -81,11 +88,8 @@ class FileUploadTask extends Task
   # composers) that the file has finished uploading.
   _completedNotification: (file) ->
     setTimeout =>
-      Actions.persistUploadedFile
+      Actions.attachFileComplete({file, @messageLocalId})
-        file: file
+      Actions.fileUploaded(uploadData: @_uploadData("completed"))
-        uploadData: @_uploadData("completed")
-      Actions.fileUploaded
-        uploadData: @_uploadData("completed")
     , 1000 # To see the success state for a little bit
 
   _formData: ->

src/flux/tasks/send-draft.coffee

@@ -4,7 +4,7 @@ Actions = require '../actions'
 DatabaseStore = require '../stores/database-store'
 Message = require '../models/message'
 Task = require './task'
-SaveDraftTask = require './save-draft'
+SyncbackDraftTask = require './syncback-draft'
 
 module.exports =
 class SendDraftTask extends Task
@@ -15,7 +15,7 @@ class SendDraftTask extends Task
     other instanceof SendDraftTask and other.draftLocalId is @draftLocalId
 
   shouldWaitForTask: (other) ->
-    other instanceof SaveDraftTask and other.draftLocalId is @draftLocalId
+    other instanceof SyncbackDraftTask and other.draftLocalId is @draftLocalId
 
   performLocal: ->
     # When we send drafts, we don't update anything in the app until
@@ -31,15 +31,19 @@ class SendDraftTask extends Task
       # recent draft version
       DatabaseStore.findByLocalId(Message, @draftLocalId).then (draft) ->
         # The draft may have been deleted by another task. Nothing we can do.
-        return reject(new Error("We couldn't find the saved draft. Please try again in a couple seconds")) unless draft
+        return reject(new Error("We couldn't find the saved draft.")) unless draft
-        return reject(new Error("Cannot send draft that is not saved!")) unless draft.isSaved()
+
+        if draft.isSaved()
+          body =
+            draft_id: draft.id
+            version: draft.version
+        else
+          body = draft.toJSON()
 
         atom.inbox.makeRequest
           path: "/n/#{draft.namespaceId}/send"
           method: 'POST'
-          body:
+          body: body
-            draft_id: draft.id
-            version: draft.version
           returnsModel: true
           success: ->
             atom.playSound('mail_sent.ogg')
@@ -49,15 +53,15 @@ class SendDraftTask extends Task
       .catch(reject)
 
   onAPIError: ->
-    msg = "Our server is having problems. Your messages has NOT been sent"
+    msg = "Our server is having problems. Your message has not been sent."
     @notifyErrorMessage(msg)
 
   onOtherError: ->
-    msg = "We had a serious issue while sending. Your messages has NOT been sent"
+    msg = "We had a serious issue while sending. Your message has not been sent."
     @notifyErrorMessage(msg)
 
   onTimeoutError: ->
-    msg = "The server is taking an abnormally long time to respond. Your messages has NOT been sent"
+    msg = "The server is taking an abnormally long time to respond. Your message has not been sent."
     @notifyErrorMessage(msg)
 
   onOfflineError: ->

src/flux/tasks/syncback-draft.coffee

@@ -9,48 +9,33 @@ Message = require '../models/message'
 
 FileUploadTask = require './file-upload-task'
 
+# MutateDraftTask
+
 module.exports =
-class SaveDraftTask extends Task
+class SyncbackDraftTask extends Task
 
-  constructor: (@draftLocalId, @changes={}, {@localOnly}={}) ->
+  constructor: (@draftLocalId) ->
-    @_saveAttempts = 0
-    @queuedAt = Date.now()
     super
+    @_saveAttempts = 0
 
-  # We also don't want to cancel any tasks that have a later timestamp
-  # creation than us. It's possible, because of retries, that tasks could
-  # get re-pushed onto the queue out of order.
   shouldDequeueOtherTask: (other) ->
-    other instanceof SaveDraftTask and
+    other instanceof SyncbackDraftTask and other.draftLocalId is @draftLocalId and other.creationDate < @creationDate
-    other.draftLocalId is @draftLocalId and
-    other.queuedAt < @queuedAt # other is an older task.
 
   shouldWaitForTask: (other) ->
-    other instanceof FileUploadTask and other.draftLocalId is @draftLocalId
+    other instanceof SyncbackDraftTask and other.draftLocalId is @draftLocalId and other.creationDate < @creationDate
 
-  performLocal: -> new Promise (resolve, reject) =>
+ performLocal: ->
-    if not @draftLocalId?
+  # SyncbackDraftTask does not do anything locally. You should persist your changes
-      errMsg = "Attempt to call FileUploadTask.performLocal without @draftLocalId"
+  # to the local database directly or using a DraftStoreProxy, and then queue a
-      return reject(new Error(errMsg))
+  # SyncbackDraftTask to send those changes to the server.
-
+  if not @draftLocalId?
-    DatabaseStore.findByLocalId(Message, @draftLocalId).then (draft) =>
+    errMsg = "Attempt to call FileUploadTask.performLocal without @draftLocalId"
-      if not draft?
+    Promise.reject(new Error(errMsg))
-        # This can happen if a save draft task is queued after it has been
+  else
-        # destroyed. Nothing we can really do about it, so ignore this.
+    Promise.resolve()
-        resolve()
-      else if _.size(@changes) is 0
-        resolve()
-      else
-        updatedDraft = @_applyChangesToDraft(draft, @changes)
-        DatabaseStore.persistModel(updatedDraft).then(resolve)
-    .catch(reject)
 
   performRemote: ->
-    if @localOnly then return Promise.resolve()
-
     new Promise (resolve, reject) =>
-      # Fetch the latest draft data to make sure we make the request with the most
-      # recent draft version
       DatabaseStore.findByLocalId(Message, @draftLocalId).then (draft) =>
         # The draft may have been deleted by another task. Nothing we can do.
         return resolve() unless draft
@@ -74,11 +59,11 @@ class SaveDraftTask extends Task
           body: body
           returnsModel: false
           success: (json) =>
-            newDraft = (new Message).fromJSON(json)
+            if json.id != initialId
-            if newDraft.id != initialId
+              newDraft = (new Message).fromJSON(json)
               DatabaseStore.swapModel(oldModel: draft, newModel: newDraft, localId: @draftLocalId).then(resolve)
             else
-              DatabaseStore.persistModel(newDraft).then(resolve)
+              DatabaseStore.persistModel(draft).then(resolve)
           error: reject
 
   onAPIError: (apiError) ->
@@ -139,7 +124,3 @@ class SaveDraftTask extends Task
 
     @notifyErrorMessage(msg)
 
-  _applyChangesToDraft: (draft, changes={}) ->
-    for key, definition of draft.attributes()
-      draft[key] = changes[key] if changes[key]?
-    return draft

src/flux/tasks/task.coffee

@@ -39,7 +39,9 @@ Actions = require '../actions'
 
 class Task
   ## These are commonly overridden ##
-  constructor: -> @id = generateTempId()
+  constructor: ->
+    @id = generateTempId()
+    @creationDate = new Date()
 
   performLocal: -> Promise.resolve()