Summary:
Add better messaging and logging to the console.
Depends on D4098
Test Plan: manual
Reviewers: evan, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4099
Summary:
Given that NylasAPIRequest reports all APIErrors for requests that
return 5xx, we were double reporting access token errors, which caused 2
groups in sentry and makes it difficult to track.
This commit removes the extra reporting for that error in the sync loop
Test Plan: manual
Reviewers: evan, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4098
Summary:
Given that we backoff exponentially in the sync loop when we encounter
RetryableErrors (e.g. network errors), if the app goes offline and
reaches the maximum retry backoff of 5 minutes, and if then the app comes
back online, in the worst case, the sync loop will idle for 5 minutes before trying to
sync, which is undesireable.
To mitigate this, everytime we come online we can wake all of the sync
workers. This shouldn't have any adverse effects
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4097
Summary:
This is mostly to clean up the logic here a little bit.
Specifically:
- The backoff when encountering retryable errors is now truly exponential
- When encountering a permanent error, we back off for a minute
- If we don't encounter consecutive RetryableErrors, we clear the exponential backoff. Previously, we would just clear it when sync completed without errors.
Test Plan: manual
Reviewers: khamidou, spang, evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4086
Summary:
This commit makes it so we /always/ catch errors thrown in the request
established inside `NylasLongConnection`. Specifically, it catches an
uncaught `socket hang up` error which is flooding Sentry unnecessarily
(https://sentry.io/nylas/nylas-mail-old-1030/issues/213178758/).
This error is thrown when the long connection is manually ended before
any data was received from the request (which happens most times during
search when you clear the search query).
The problem was that when we end the long connection, we remove all event listeners
from the request, and /then/ the error is thrown, at which point there is no error
handler to catch it. The solution is simply to always have an error handler attached to
the request object.
This commit also adds extra error handling that seems to have been missing.
Test Plan:
manually check that delta connection is still working, and that error
is no longer thrown when you search and then immediately clear the search
Reviewers: mark, evan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4096
Summary:
We want to listen to both `abort` and `aborted`
- `abort` happens when we manually abort a request client side, so we don't want to report this to sentry
- `aborted` happens when the server aborts the request, so we /do/ want to report in this case
Test Plan: manual
Reviewers: spang, mark, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D4073
Summary:
Previously we used the Gmail REST API to get search results. This API
returns the X-GM-MSGID which is different from the folder UID. Gmail also
offers the X-GM-RAW extension for IMAP's UID SEARCH command which allows
us to pass through the raw search query. This enables us to vastly
simplify the GmailSearchClient by having it subclass ImapSearchClient
and override a few select methods. Yay!
Test Plan: Run locally, verify that Gmail search queries still work
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4094
Summary:
Previously for IMAP we just grabbed the search text and fed it into a TEXT
query. Now we have a proper backend that generates the appropriate
search criteria according to the IMAP spec. Important to note that we
don't support 'in:' yet, which is complicated due to the way that IMAP
search is scoped to the currently selected folder.
Test Plan: Run tests, run locally and verify IMAP search still works.
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4071
Summary:
- Add an icon to threads that have reminders set
- Show the pending reminder message header in all perspectives, rather than
just the Reminders perspective
- Add the ability to clear the reminder from the message header
- Condense the message header text
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Subscribers: sdw
Differential Revision: https://phab.nylas.com/D4052
Summary:
Gleb mentioned that we need the sample size of search events to be
bigger, so let's report all search events
Test Plan: manual
Reviewers: evan, gleb, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4077
Summary:
If it's expensive for us to SELECT a folder (which happens a lot for
large gmail accounts), we should try to sync more messages so that we
don't waste a ton of time SELECT-ing during initial sync. This speeds up
initial sync quite a bit.
Test Plan: Run locally, verify batches are properly computed
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4050
Summary:
This changes the scheduler from `setImmediate` to `setTimeout`. The HUGE
difference is that Chrome's async stack traces works with `setTimeout` but
does NOT work with `setImmediate`. These two functions are adjacent to
each other in the Node event loop, so nothing should depend on the change
in ordering.
Test Plan: manual
Reviewers: mark, spang, halla, juan
Reviewed By: spang, halla
Differential Revision: https://phab.nylas.com/D4068
Summary: Bluebird changed the way you call `longStackTraces`. This uses the latest
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: spang, halla
Differential Revision: https://phab.nylas.com/D4067
Summary:
We no longer optimistically fetch messages when we load their bodies. We
always get the full message with bodies returned via the in memory delta
stream
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: spang, halla, juan
Differential Revision: https://phab.nylas.com/D4066
Summary:
We no longer need to use the `returnsModel` param since we get all of our
models through the in memory delta stream. There were a ton of places
unnecessarily passing `returnsModel: false` when it defaults to false in
the first place
Depends on D4057
Test Plan: manual
Reviewers: halla, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4065
Summary:
We no longer use the `ensureOnce` bit.
Depends on D4057
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4064
Summary:
This removes the last reference to a now unused `beforeProcessing` feature
of the Nylas API Request.
Depends on D4057
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4063
Summary:
The old NylasAPIRequest helper class used to do all sorts of run-time
requiring to attempt to notify of auth errors when 401s and 403s came up
in a request. We now move that logic to the AccountStore where it belongs.
Depends on D4057
Test Plan: Manual
Reviewers: spang, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4060
Summary:
This greatly refactors the NylasAPIRequest object to be more single
purpose, easier to read, async-ified.
It also fixes issues where long stack traces weren't being reported bcause
`reportError` was being called before the error had time to make it
through Bluebird's rejection handler (which adds back in the long stack
traces).
This also fixes an issue where ignorable errors (like 404s,
ESOCKETTIMEDOUT) were being reporting to Sentry.
This also fixes subtle conditions where the request run would throw mixed
error types (sometimes regular `Error`s, sometimes `APIError`s).
The old class used to handle logging people out on 401s. This is now moved
to the AccountStore in a different diff.
We've also deprecated the `returnsModel` param in a separate diff.
This deprecates the `PriorityUICoordinator` since we no longer need to
worry about frequently making expensive API requests to pull down data
like we used to in the old cloud-based API.
This deprecates `ensureOnce` param in a separate diff.
Test Plan:
Manually boot app. Check regular 404s don't throw. Check requests get
through. Check special ESOCKETTIMEDOUT type errors are properly handled
Reviewers: juan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4057
Summary:
Previously we were using the raw visitors that were confined to the flux
attributes directory. We're going to add more search query backends, so this
is mostly just moving things to a new, more general place.
Test Plan:
Run locally, verify parser specs still work, verify in-app search
still works.
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4053
Summary:
Isomorphic-core should be functional outside of the client environment
Depends on D4056
Test Plan: Run the test suite
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4062
Summary:
Move the base Jasmine spec runner into isomorphic-core to prevent
code duplication. Jasmine will look for the config file relative to
the directory it's being run in though, so we need to symlink the
config file into each package that will need it.
Test Plan: Run tests once the suites are integrated
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4056
Summary:
Convert the isomorphic-core specs to Jasmine 1 and symlink them into
client-app/internal_packages so they are run as part of the client
test suite.
Test Plan: Ran the suite with fdescribes in iso-core to ensure they were being called properly
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4055
Summary:
Disabling verbose logging in production builds just makes it harder to help our
users help us fix their problems.
Test Plan: run app without --dev
Reviewers: mark, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4070
Summary:
We detect when a thread is removed from the thread-list by listening to
`Actions.threadListDidUpdate`. However, we were not firing the action at
the correct moment.
Before this commit, we fired the action on the root `ThreadList`'s
`componentDidUpdate`, however did this not actually fire when the
child list actually updated/removed threads from the list.
This sort of used to work because before 396a027bcb
the root ThreadList component re-rendered all the time, so it fired the
action, but after initial sync, we would never actually report any
thread-list action metrics at all
Test Plan: manual
Reviewers: spang, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4051
The previous fix only worked by coincidence. It turns out that we don't have
the right data to associate draftClientId with ChangeMailTask on task
creation, so we have to either look up this data inside the dependency
function or make the dependency check looser.
This check gets run all the time, so we chose to make the check looser
rather than putting a db call in here and changing the interface to support
an async version of isDependentOnTask().
This patch also makes it so that the fix works properly when Undo Send
is enabled.
Fixes T7889
Because ChangeMailTask only depended on EnsureMessageInSentFolder task,
if a user sent a message & then archived it before the send completed,
the ChangeMailTask could run in between the SendDraftTask and
EnsureMessageInSentFolderTask and fail because it's
EnsureMessageInSentFolderTask that detects the new UID for the message.
Fixes T7889
Temporarily reverting this commit for 1.0.30 release because it is causing
performance issues: https://phab.nylas.com/T7910
This reverts commit 5e35d39eb2.
Summary:
This commit adds a `shouldComponentUpdate` to thread-list.cjsx so that
the thread list doesn't unnecessarily /try/ to re-render when state or
props haven't actually changed.
I noticed this because the thread list was constantly calling `render`
even though it didn't render any changes to the DOM. This was caused
because it listens to `NylasSyncStatusStore` which constantly triggers,
even though the piece of state the list is interested in rarely changes,
causing unnecessary calls to the `render` loop.
Test Plan: manual
Reviewers: halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4047
Summary:
This commit is an attempt to cleanup duplicated code and crufty code
inside IMAPConnection and IMAPBox
Specifically:
- It replaces `_createConnectionPromise` with a more aptly named (imo) `_withPreparedConnection` helper, which provides the user a node-imap connection that will correctly time out and handle `error` and `end` events. Most of these changes are just changing existing code to use the new interface.
- Adds a subtle change to how we handle `end` and `error` events on the connections. Previously, we manually called `this.end()` on `error`, but not on `end`. From what I could gather from the old comment documenting `_createConnectionPromise`, we should /also/ call `this.end()` on `end` because node-imap doesn't clean up correctly and can leave the connection hanging (taking care not to introduce a recursion loop by `end`ing on `end`). Additionally, it no longer listens to the events via `once` but via `on`, which should be okay given that the listeners get cleared at the end.
This /might/ fix some instances of sync freezing up (T7837).
Depends on D4035
Test Plan: manual -- this really needs some unit tests 😢
Reviewers: spang, halla, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4036
Summary:
Remember how we made it so imap socket timeouts would increment up to 10m if
we constantly got timeout errors? Remember how we told everyone in
https://github.com/nylas/nylas-mail/issues/3232 that we'd fixed their problems, but they weren't actually fixed?
Well, we weren't /actually/ applying the correct timeout value. 😢
Depends on D4033
Test Plan: manual
Reviewers: spang, halla, mark, evan
Reviewed By: mark, evan
Differential Revision: https://phab.nylas.com/D4035
Summary:
The way it was set up was a little smelly.
- `this._settings` were /not/ the actual settings. Now, internally we have access to `this._resolvedSettings`
- `this.resolvedSettings` was being set kind of out of nowhere (gmail-oauth-helpers needs it, hence adding a getter instead)
Test Plan: manual
Reviewers: spang, mark, evan, halla
Reviewed By: mark, evan, halla
Subscribers: khamidou
Differential Revision: https://phab.nylas.com/D4033
Summary:
This commit fixes 2 issues with signatures in the preferences:
- Creating a signature (via any of the create buttons) would create 2 signatures
- Trying to select accounts to associate with a signature in the preferences would not work (the account would not be selected)
This was a regression introduced in e638e94084 (diff-4f38fd25aa24b48a309354be643165d3R26)
3111c16 made it so we attempt to `activate` any Stores that are registered with
the StoreRegistry. When adding stores to `nylas-exports`, they are
automatically registered in the StoreRegistry. Given that the
`SignatureStore` is in `nylas-exports`, and consequently is registered in
the StoreRegistry, it would be `activate`d upon window boot.
However, we were also manually activating it inside `internal_packages/composer-signatures/lib/main.es6`.
This caused it to register listeners for every action **twice**. For
this reason, 2 signatures would be created when trying to create 1, and
an account would be immediately unselected after being selected int he
signatures dropdown in preferences.
(Other changes in this are just stylistic)
Test Plan: manual
Reviewers: spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4043
Summary: See title
Test Plan: Run locally, make sure sync still works
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3998
Summary:
I did a grep for `Message.find.*` to see if there were any other places
we could limit the attributes returned
There were.
This used to be INSANELY wasteful to return all bodies when we only needed
the ids.
Test Plan: Manually bootup app and ensure search still works
Reviewers: mark, juan, spang
Reviewed By: mark, juan, spang
Differential Revision: https://phab.nylas.com/D4042
Summary:
Fixes T7783
This unbounded query, plus a yet-to-be-diagnosed bug causing large numbers
of messages with no folderIds, led to frequent crashes of the worker
window at the end of every sync loop.
This took a fair amount to diagnose. Notes on that are here:
https://paper.dropbox.com/doc/Diagnosing-Crash-X329VTxevrqtIMESBtHNV
Huge thanks to @mark.
Test Plan: Reboot app with patch, no more crashes!
Reviewers: spang, halla, mark, juan
Reviewed By: mark, juan
Subscribers: mark
Maniphest Tasks: T7783
Differential Revision: https://phab.nylas.com/D4039
This goes against the spec, but we've seen emails which use comma-separated
References instead of space-separated references. This could cause threading
to break if the message with comma-separated references was a reply and
we hadn't yet synced the message directly in the In-Reply-To header.
Summary:
When we detect that threads in the database have changed, if they've already
been indexed, mark them as unindexed so that the indexer picks them up and
re-indexes them with the updated data
Test Plan: manual
Reviewers: evan, spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4023
Summary:
If for whatever reason we passed an error with a `source` property
we weren't accounting for in `convertIMAPError`, we would return an
`undefined` error. Bad!
Instead, just return the original error
Test Plan: manual
Reviewers: spang, halla, evan, mark
Reviewed By: evan, mark
Differential Revision: https://phab.nylas.com/D4037
Summary:
When reporting different times to mixpanel other than `actionTimeMs` via
recordPerfMetric, we also want to clip those times to a range to have
good data in mixpanel
This commit adds an extra option to recordPerfMetrics to be able to clip
data other than the default `actionTimeMs`, and uses the new option to
report search metrics
Test Plan: manual
Reviewers: spang, evan, halla, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4038
Summary:
I'm making these up based on gut feeling, so feel free to completely change
these.
Test Plan: manual
Reviewers: spang, mark, evan
Reviewed By: mark, evan
Differential Revision: https://phab.nylas.com/D4026
Summary:
Since we only persist updates to fetchedmin/fetchedmax at the end of a
batch, and a batch can contain many messages, if the sync loop is
getting interrupted often we will download the same messages over and
over again and not make much progress in downloading the message
backlog. This patch keeps a set of already downloaded messages in memory
for each batch and skips downloading UIDs we've processed in interrupted
sync loops.
Messages may still be redownloaded across app restarts.
Fixes T7798
Test Plan: manual
Reviewers: juan, mark
Reviewed By: juan, mark
Maniphest Tasks: T7798
Differential Revision: https://phab.nylas.com/D4040
Summary:
Previously we would always search all mail. Now, if the user has focused
a particular folder we will limit our search to that folder. The inbox
is an exception--it will always search all mail unless the user
explicitly uses an "in:" clause.
Test Plan: Run locally, verify that searching folders returns the correct results.
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4032
This is a temporary commit to prevent connections from being closed
while we release logic to re-establish delta connections after they are closed.
Our infrastructure should be able to support current usage of delta
connections after our fixes to offline notification and infrastructure
This reverts commit 78f67d4a76.
Given that this packages lives inside the NylasMail app, it has access
to Bluebird Promises, which already define a .each method.
Using PromiseUtils here is unecessary
Summary:
We were only detecting and classifying IMAP errors as retryable and non
retryable in a few places, but errors thrown in any of our imap
operations were not being properly passed through our `convertImapErrors` :(
This was broken, oh so broken
Also loosen condition to detect System Errors
Test Plan: manual
Reviewers: spang, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4022
Summary:
Fix errors and inconsistencies in MetricsReporter code like:
- We were modifying the passed in data object in place
- We were accessing NylasEnv without knowing we were in client env to
get the version
- We were creating an incorrect logger instance
Test Plan: manual
Reviewers: spang, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4010
Summary:
This action will report perf metrics to honeycomb /and/ mixpanel. This
commit also fixes up Analytics Store a little bit
Test Plan: manual
Reviewers: spang, halla, evan
Reviewed By: halla, evan
Subscribers: jerm
Differential Revision: https://phab.nylas.com/D4012
Summary:
Also see:
3a33b0ad64
which was hot-pushed to master in order to get Travis building.
We now have two travis files:
1. /.travis.yml
2. /packages/client-app/travis.yml
The first one is alwas in the private repo and runs `npm install && npm
run build-client`. This decrypts our keys and signs, builds, and uploads
to S3.
The second one is designed to live in our yet-to-be public mirror. It will
basically just run `npm install && npm test`.
That way the public one should just about ALWAYS pass (YAY!) except of
course when you break the tests or something in the installer!
Test Plan: Run on new https://travis-ci.com/nylas/nylas-mail-all
Reviewers: jerm, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3999
Summary: See title
Test Plan: Run locally, verify IMAP search still works
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4006
Summary: See title
Test Plan: Run locally, verify we can download raw messages
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4005
Summary: See title
Test Plan: Run locally, verify downloading files works
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4004
Summary:
We did this to see if we should clear the index, which no longer makes
sense because the index never exceeds our threshold. Additionally, none
of the other search indexes do this. This was causing us to spend a ton
of time scanning the ThreadSearch table at startup.
Test Plan: Run locally, make sure things are fine
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4007
Summary:
Previously, we would create a nodemailer SMTP transport object when the
sync worker booted up. The transport object would be passed the account
SMTP credentials at the time of object creation. If the Google auth
token later expired, we would continue to try to send mail using the
expired token, resulting in "Invalid login" failures.
This patch makes it so we refresh the transport object if the auth token
changes, and also turns on SMTP connection pooling to limit simultaneous
SMTP connections (& maybe make sending multiple messages faster).
Fixes T7891
Test Plan: manual
Reviewers: juan, halla
Reviewed By: juan, halla
Subscribers: mark
Maniphest Tasks: T7891
Differential Revision: https://phab.nylas.com/D3997
Summary:
This fixes a regression introduced in D3980 that prevented sending from working
on Office365 and generic IMAP accounts.
Fixes T7892
Test Plan: manual - we could use unit tests for this but need to set up tests for iso-core first
Reviewers: juan, halla, evan
Reviewed By: halla, evan
Maniphest Tasks: T7892
Differential Revision: https://phab.nylas.com/D4003
Summary: This was broken, oh so broken
Test Plan: manual
Reviewers: halla, spang, evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4001
Summary:
Prior to this diff it was easy for us to create too many IMAP connections (e.g.
by requesting many attachments at once), causing random failures when the
server would reject our connection attempts. This diff adds a per-Account IMAP
pooling mechanism so that we avoid these failures.
Test Plan:
Run locally with sync worker and several other clients using the
pool, verify correct behavior. Also added a few unit tests.
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3965
Summary:
The prod build was failing because it couldn't find the .babelrc.
I decided to put the babelrc in the main nylas-mail repo with the
expectation that it'll need to be in the open source version too. To DRY
up this for us building, we sylink the root one to the client-app babelrc
Also since we now dereference symlinks we don't need to do the full copy
of nylas-private-resources so we don't have two error reporters floating
around
Test Plan: npm run build-client
Reviewers: juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D3993
Summary: See title
Test Plan: Depends on D3990
Reviewers: spang, halla, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3991
Summary:
See title
Depends on D3989
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: spang, halla
Differential Revision: https://phab.nylas.com/D3990
Summary:
This commit adds new actions, `applyCategoryToThreads` and `removeCategoryFromThreads`
to be proxied and measured through the ThreadListActionsStore. These are called when
labels are added or removed via the category picker or by removing using
the label icon.
For now, we are only interested in timing actions that remove threads
from the inbox.
Test Plan: manual + reenabled unit tests for category-picker
Reviewers: spang, evan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3989
Summary:
This commit adds a new action, `moveThreadsToPerspective` to be proxied and
measured through the ThreadListActionsStore. This action is called when
a thread or group of threads is dragged and dropped into an item in the
sidebar.
For now, we are only interested in timing actions that remove threads
from the inbox
Depends on D3984
Test Plan: manual
Reviewers: halla, evan, spang
Reviewed By: evan, spang
Differential Revision: https://phab.nylas.com/D3985
Summary:
This commit adds a new action, `removeThreadsFromView` to be proxied and
measured through the ThreadListActionsStore
This action can encompass many different actions, e.g.:
- unstarring in starred view
- changing unread in unread view
- Moving to inbox from trash
- archiving a search result (which won't actually remove it from the thread-list)
However, for now, we are only interested in timing actions that remove threads
from the inbox
Depends on D3983
Test Plan: manual
Reviewers: halla, spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3984
Summary:
This commit makes so it we report perf metrics for archive actions.
To achieve this, I added a new `ThreadListActionsStore` which serves as
a proxy for thread actions, which allow us to time them.
The new store is in charge of listening to thread list actions, creating and
queueing the appropriate tasks for any given action, and timing and
reporting action times to our MetricsReporter.
This commit only times archiving actions, and subsequent diffs will time
other relevant thread list actions.
Test Plan: manual
Reviewers: halla, spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3983
Summary:
This will help us aggregate metrics by user. This also makes it so we
don't report events in dev mode
Test Plan: manual
Reviewers: spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3981
Summary:
This global module wasn't really related to performance, but rather with
timing things across different processes in the app. I believe this name
is more appropriate.
Test Plan: I can still use NylasEnv.timer (instead of NylasEnv.perf)
Reviewers: spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3972
Summary:
This ensures that the Libhoney instance is a singleton in cloud
processes.
Test Plan: manual
Reviewers: mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3969
Summary:
Renamed it from SyncMetricsReporter to MetricsReporter and moved it to
iso-core.
The new metrics reporter can now be called from any environment and will
correctly report the metrics.
Test Plan: manual
Reviewers: mark, spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3967
Summary:
In client-app/node_modules Lerna symlinks isomorphic-core to '../../'.
When we copy everything over to our tmp directory when building, we copy
over the relative symlink! Damn you lerna.
We get around this by resolving the symlinks BEFORE copying and caching
them in the installer task. Then the file copy always works.
Also, for some reason the glob {absolute} param doesn't seem to work in
the latest version. I'm manually creating an absolute path for the compile
target since it's a bit more transparent what's happening anyway.
Test Plan: `npm run build-client`
Reviewers: spang, jerm, juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D3988
Summary:
Grunt is hardcoded to use paths relative to wherever the Gruntfile is
located. Unfortunately it also expects the grunt packages to be siblings
of that gruntfile. We can get around this by changing the relative base
path, but then the cwd is different for each tasks. This is okay as long
as we use absolute paths for various files in each of our tasks. This
updates our grunt tasks to use absolute paths
Test Plan: `npm run build-client`
Reviewers: spang, halla, jerm, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3987
Summary: This error should really be retryable, and will prevent more red boxes
Test Plan: manual
Reviewers: evan, spang
Reviewed By: evan, spang
Differential Revision: https://phab.nylas.com/D3982
Summary:
We previously weren't saving the smtp settings for cloud gmail accounts,
and even though we fixed that, we still need to be able to handle the accounts
that were authed before that fix went out. This diff changes `smtpConfig()` to
always call `credentialsForProvider` instead of depending on what was saved
in the database.
Test Plan: manual
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3980
Summary:
Process reminders metadata and send the reminder email if there
haven't been any replies. Checking for replies involves checking
each of the folders in `folderImapNames` for messages that are in
reply to the messages in `messageIdHeaders`.
Test Plan: manual
Reviewers: khamidou, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3958
Summary:
The SendmailClient is in isomorphic-core, but wasn't working properly when
called from cloud-workers. This diff includes a variety of fixes to get it
working:
- Add smtp config to connection settings in cloud-core
- Don't use `PromiseUtils.promisify` for a function that needs `this` to
be bound properly
- Don't reference `NylasEnv` (That error gets caught and reported
elsewhere anyway, we don't need to report it here)
- Default `message.uploads` to `[]` to keep iterator happy
- Add Gmail environment variables to cloud-worker app
Test Plan: manual
Reviewers: juan, khamidou, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3954
Summary:
The new send-reminders cloud-worker needs different information to check for
replies now that the sync engine is local. Make sure we save all of the
required information to the metadata. The current values are:
- expiration: the time the reminder should be sent, if no new replies
- folderImapNames: the folders to check for replies
- messageIdHeaders: the Message-Id headers of messages in the thread
(we search for In-Reply-To headers that match these values, and also
use them to decide whether a message is a new reply or an old reply)
- replyTo: the message to reply to when sending the reminder email
- subject: the subject to use when sending the reminder email
Test Plan: manual
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3952
Summary:
This removes client-sync deltas from the developerbar delta list.
We ONLY show cloud deltas now.
The connection between client-sync is no longer a network delta stream,
it's a direct function call. It makes no sense to show its status.
This now shows a single dot representing the state of the cloud delta
stream.
Test Plan: Manually connect and disconnect local cloud API and see icon change
Reviewers: halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3977
Summary:
SFDC's task to upload an email to salesforce needs the stripped DOM of a
Message object to call `innerText`. The API was changed to return a string
instead of the DOM. This adds a flag to request the DOM instead of a
string.
Test Plan:
Manually assert `EnsureMessageOnSalesforceTask` properly can add the plain
text to the Salesforce Task object
Reviewers: halla, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3976
Summary:
Node's native `Error` object does NOT implement toJSON. We attempt to call
toJSON when reporting errors. This wasn't noticed until now because
bunyan's pretty logger (which only run in dev mode) started JSONifying
errors
Test Plan:
Try and API auth with a bad username with local setup. See that it throws
toJSON error. After patch, error properly serializes
Reviewers: spang, halla, jerm, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3975
Summary:
Some API Errors, like ECONNREFUSED, have no \.message.
Catch this in the error reporter
Test Plan: Manually create a non-message error and see better error message
Reviewers: spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3973
Summary:
Adding READMEs for easy and helpful browsing on GitHub.
Also add missing script and `--interpreter` flag
Test Plan: Run new launch commands
Reviewers: mark, spang, juan, halla
Reviewed By: spang, juan, halla
Differential Revision: https://phab.nylas.com/D3971
Summary:
Client-sync has the full imap folder names, but used to only pass the display
name to the application. The application needs the full imap names so that it
can pass them via metadata to cloud-workers that need to open imap boxes.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Subscribers: juan
Differential Revision: https://phab.nylas.com/D3951
Summary: atob() is a global in browser environments, but needs to be imported otherwise.
Test Plan: manual
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3950
Summary: Moved a file and forgot to update this import :-/
Test Plan: Run locally
Reviewers: juan, evan, spang
Differential Revision: https://phab.nylas.com/D3970
Summary:
If an exception has the same stack trace, by default Sentry will always group
it together in the same event. We don't want to do that for sync loop
errors---e.g. 'Invalid credentials' errors should not be grouped together with
stuff like 'Too many simultaneous connections'. Creating more unique groups
will allow us to better evaluate the effect of sync & other bugfixes.
Test Plan: writing unit test right now
Reviewers: juan, mark
Subscribers:
Differential Revision: https://phab.nylas.com/D3915
Summary: See title
Test Plan: Run locally, verify that double clicking inline images opens them
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D3963
Summary: This stuff doesn't seem to be used for anything anymore and it's cluttering up the client-app dir.
Test Plan: ran the app, also did `npm start` with no ~/.nylas-dev
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3961
Summary:
This is a set of functions which will allow isomorphic-core to detect
which environment it is running on.
This will be useful for moving the metrics reporter to iso-core
Test Plan: manual
Reviewers: mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3966
Summary:
We were doing some incorrect processing of args passed to the main
function which was causing us to think we were launching NM by passing a
file (which creates a new draft and tries to attach that file). Since
were trying to attach 'packages/client-app', this was causing an error
dialogue to appear indicating that it wasn't possible to attach a
directory.
Test Plan: Run locally, verify no dialogue
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3962
[*] update babel
[client-app] remove flow-typed
[client-app] Move build/package.json to main package.json
[client-app] remove spec_integration
[client-app] fix babel support
Add client-private-plugins package.json
[client-app] add node_modules to global path for private-plugins
Move client-sync dependencies to client-app root
fix electron rebuild
[*] moved to monorepo
Summary: App now runs in monorepo
Test Plan: npm test
Reviewers: juan, mark, khamidou, halla, spang
Differential Revision: https://phab.nylas.com/D3947
Summary:
When saving a thread, we weren't properly setting the `participants`
object. Since Sequelize has object properties under getters and setters,
doing `this.participants.push()` did nothing.
Since threads had no `participants`, the related threads widget, which
queries through there, did nothing.
Test Plan: Manually inspect DB and use the related threads widget
Reviewers: halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3938
Summary:
This is the result of auto package.json fixing by lerna. Would be nice to
commit this so you can run script/bootstrap without it making local
changes.
I didn't manually bump any versions.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3934
Summary:
We don't need to check folders that often while on battery. Check every
5 minutes rather than every 10 seconds.
Test Plan: Run locally, verify the timeout is longer while on battery
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3940
Summary: See title
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: spang
Subscribers: mark
Differential Revision: https://phab.nylas.com/D3932
Summary: They obscure the location of our logs in the dev tools.
Test Plan: Run locally, verify that logs link to proper place
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3939
Summary:
Previously, we would only refresh Google OAuth2 access tokens at the
beginning of the sync loop, and _only_ if the access token had already
expired. This meant that if an access token expired in the middle of a
sync loop iteration, the user would get prompted with the reauth red box
for their account and would have to either go through the oauth flow
again or restart the app for sync to continue.
This diff makes two changes:
1. Adds 5min of padding to the refresh window, so if a token will expire
in <5min, we'll go ahead and refresh the token. This will reduce the
possibility that an access token can expire during a sync loop
iteration.
2. Catches Invalid Credentials IMAPAuthenticationErrors for Gmail
accounts and forces a token refresh on the next sync loop.
These should prevent a user from _ever_ having to reauth their Gmail
account unless the refresh token is revoked, or we encounter some other
permanent error trying to refresh the token.
Fixes T7775 (at least some cases)
Test Plan: manual
Reviewers: khamidou, evan, juan
Reviewed By: juan
Maniphest Tasks: T7775, T7755
Differential Revision: https://phab.nylas.com/D3908
Summary:
Instead of re-implementing exponential backoff, throw the retryable
error so the sync loop handles it and backs-off
Test Plan: manual
Reviewers: spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3914
Summary:
Update Salesforce to use the new search indexer
Depends on D3911
Test Plan: Manually bootup SFDC and ensure it launches and indexes models properly
Reviewers: mark, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3913
Summary: Should help a fair bit with our redis connection pileup.
Test Plan: Tested manually.
Reviewers: mark, spang, juan, evan
Reviewed By: mark, spang, evan
Differential Revision: https://phab.nylas.com/D3916
Summary:
This commit makes it so we always continue retrying syncback tasks as long as they
error with a retryable error. There's really no reason to not continue retrying syncback
tasks after an arbitrary number of retries (especially such a low one) if we
encounter a retryable error. Before this commit, if for example we got 2 random
network errors in a row, we would just mark the task as failed even
though it would eventually succeed in subsequent attempts.
Previously, when N1 synced against the cloud api, we would indefinitely
retry a Task if we continued getting retryable errors. This ensures that
the app can work correctly offline and prevents displaying unecessary errors to
the user, and having actions bouncing back or sending messages without putting them
in the sent folder.
Additionally, this commit ensures that when cleaning up messages without a
folderImapUID we don't delete messages that are currently being added to the sent
folder. This is relevant to this commit because given that we could retry the
EnsureMessageInSentFolder task indefinitely, we might end up deleting that
message because it wont have a uid until the task succeeds.
Depends on D3898
Test Plan: manual
Reviewers: mark, spang, evan, halla
Reviewed By: spang, evan, halla
Differential Revision: https://phab.nylas.com/D3900
Summary:
Since all Nylas Mail actions are thread-based except for sending,
we never use any of these. Make `git grep` less confusing by getting rid
of them.
Test Plan: use the app
Reviewers: evan, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3910
Summary:
Previously, if you were to close the app while you had any tasks queued,
these would be marked as failed the next time you open the app, showing
an annoying error message and reverting any optimisitic actions.
However, we don't need to be so defensive about retrying tasks because
the only tasks we can't retry are the Sending tasks. All of the other
tasks like moving or changing labels are fine to retry (trying move the same
set of uids twice wont cause an error)
This commit adds an extra status to syncback requests, "NOTRETRYABLE".
Only NOTRETRYABLE requests will be marked as failed at the beginning of
the sync loop, and any INPROGRESS tasks will be marked as NEW so they
can be retried
Depends on D3896
Test Plan: manual
Reviewers: mark, evan, spang, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3898
Summary:
Now that we don't run Send tasks outside the sync loop, we don't need
that awful hack wich required passing a `runTask` callback to
`runSyncbackTask` in order to customize how to run the task.
Instead, runSyncbackTask now knows 2 ways to run a task, either via imap, or
via smtp, depending on the resource declared by task to run. So now
SyncbackTasks declare a resource type they need to run, and that will be
passed as their second argument when running.
Depends D3894
Test Plan: manual
Reviewers: mark, halla, spang, evan
Reviewed By: halla, spang, evan
Differential Revision: https://phab.nylas.com/D3896
Summary:
We had previously ripped send tasks outside the sync loop to make them run faster,
but they run fast enough inside the loop.
This commit will also fix the scenario where if you closed the app in the
middle of a send task, the task would just hang forever and never succeed or
fail (T7818); given that it was excluded from the loop, we also had to exclude it
from the cleanup step to mark any INPROGRESS tasks as failed at the beginning
of each loop, which caused send tasks in progress to never get cleaned.
Putting them back inside the loop allows us to fix this without adding more messy
logic, and it cleans up ugly duplicated code. Additionally, it will prevent more
duplicated code for upcoming diffs that will improve syncback task reliability
when the app is closed or window is restarted in the middle of a task.
Depends on D3893
Test Plan:
manually test sending, it still works, it's still fast. Restarted
window in the middle of send task, task fails.
Reviewers: mark, spang, halla, evan
Reviewed By: spang, halla, evan
Differential Revision: https://phab.nylas.com/D3894
Summary:
[cloud-api]
Based on the passed in `messageIds`, it finds any existing thread
metadata that might be under a different thread id. If it realizes
there are actually multiple threads that should be the same thread,
(due to getting a missing message link), it reconciles all of them.
[local-sync]
Return `message_ids` in `Thread.toJSON()`
See D3879 for tests
N1 Pairing: D3875
Test Plan: unit tests, local testing soon
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3880
Summary:
Instead of exposing helper functions, make this a class to hold the
shared state of the db, account, and logger required to run any syncback
inside an account sync loop.
Test Plan: manual
Reviewers: mark, spang, halla, evan
Reviewed By: spang, halla, evan
Differential Revision: https://phab.nylas.com/D3893
Summary:
See title.
I really wish we could clean up this error handling a bit better, but I don't
think its super important right now.
Test Plan: manual
Reviewers: spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3903
Summary:
I've encountered random imap errors that we mark as permanent, but that
contain try again in the error message. We should check for that
Test Plan: manual
Reviewers: spang, evan, mark
Differential Revision: https://phab.nylas.com/D3899
Summary: There's no file to unlink, we just need to drop the tables.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3878
Summary:
If you have no messages in your Gmail Inbox (Yay Inbox Zero!) and you
connect your account and do first sync, then we get an error where we try
and fetch a range from null to -1.
This was due to a logical error in the first sync fetch code.
This diff fixes this bug and renames some variables to make it clearer
what's going on
Fixes T7842
{F11176}
Test Plan:
1. Bring Gmail to Inbox Zero
2. Connect account
3. Verify first sync works
Reviewers: spang, halla, juan
Reviewed By: juan
Maniphest Tasks: T7842
Differential Revision: https://phab.nylas.com/D3889
Summary: See diff title
Test Plan: Run locally, make sure we backoff
Reviewers: juan, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3886
Summary:
Previously, when processing messages during folder sync, if the message already existed, and it belonged to a thread, we would update the message, but forget to update its thread with any changes that new
message would produce on the thread (e.g. updating the threads folders or labels).
One obvious manifestation of this was when replying to a thread: the EnsureMessageInSentFolderTask would create the new message, and then attempt to sync the sent folder to fetch the newly created message. When processing this message during sync, we would update the message but not update its thread, so the thread would not be associated to the sent folder, and it wouldn't show up in your sent items list in the UI.
Test Plan: manually verify that it works
Reviewers: evan, mark, spang
Reviewed By: mark, spang
Differential Revision: https://phab.nylas.com/D3872
Summary:
Our sentry reporter tries to fetch the nylas identity from the database,
and access properties on it. However, if you are in a state where there
is no identity available (like having logged out, or just starting the
app), and encoutnered an error that would be reported to sentry, we
would throw an error while reporting and that would crash the app
Also, fix lint errors and some really janky code
This fixes T7810
Test Plan: manual
Reviewers: halla, spang, evan
Reviewed By: spang, evan
Maniphest Tasks: T7810
Differential Revision: https://phab.nylas.com/D3867
If the first argument to our local-sync logger is an object
(this is bunyan's api, and it's how we log from isomorphic-core and cloud-* packages
in order to have structured json logs for logstash), make sure we log
the object last and the string that comes as the second argument first.
Summary: Fixes T7649
Test Plan: FML writing unit tests now
Reviewers: evan, mark, juan
Reviewed By: mark, juan
Maniphest Tasks: T7649
Differential Revision: https://phab.nylas.com/D3863
Summary:
This error ocurred, to the best of our knowledge, on iCloud accounts
that had been linked to other clients like Airmail.
On such accounts, node-imap would incorrectly parse the mailbox list
from imap, and return an `Airmail` folder which did not exist, causing
us to try to sync that nonexistent folder and error in the sync loop.
This error is amongst the most frequent we've seen in Sentry and
Support: https://sentry.io/nylas/nylas-mail/issues/213158962/events/4897450600/
The fix es detailed in the PR to node-imap: https://github.com/mscdex/node-imap/pull/594/files
This commit only points the node-imap dependency to our fork for now
Test Plan: manual and unit tests in node-imap
Reviewers: mark, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3860
Summary:
This diff (and the K2 counterpart diff) allow us to run dev-mode Nylas
Mail side-by-side with prod Nylas Mail.
There were 4 things that needed to change:
1. Use different config dir
2. Use different keychain name
3. Use different localhost port
4. Prevent Electron's `app.makeSingleInstance` from killing our app
All of these are activated through `NylasEnv.inDevMode()`.
Test Plan: Manual
Reviewers: halla, mark, spang, khamidou, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3861
Summary:
In electron, the --enable-logging flag makes it so the main browser
process logs to stdout all of the logs generated from within the renderer
processes.
Unfortunately, the main process will only log out the first argument passed to
`console.log` from within a renderer process (see https://github.com/electron/electron/issues/7061)
This commit makes it so that the local sync logger logs most of the log line in the first
argument passed to `console.log`
Test Plan: manual
Reviewers: evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3852
Summary:
When multiple accounts are syncing, it's very hard to scan the local
sync logs because it is unclear to which account the logs belong to,
and it makes debugging hard.
This commit makes it so that all logs from local-sync include the
account info, with the account email prefixed at the beginning of each
log line (this allows filtering), and color coded by account.
Test Plan: manual
Reviewers: mark, spang, khamidou, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3851
Summary:
On each sync loop, we increment the socketTimeout based on how many times we've
seen socket timeouts in a row. The max socket timeout is 10m
Test Plan: manual
Reviewers: evan, spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3843
Summary:
In most cases (and especially so on Gmail and in the inbox on generic
IMAP), messages with higher UIDs are newer---and even if they aren't the
newest possible messages in other generic IMAP folders, they are the
most recent messages that have been moved to that folder.
Our previous batching strategy unfortunately resulted in us downloading
the lowest UID in each batch first, which was especially confusing when
connecting a new account and having the first message pop up on the
screen be a message from hours or days ago.
This patch changes the batching strategy in three ways:
1. Within a batch, we process downloaded messages from highest UID to
lowest UID.
2. We download batches in order of the ones containing the highest UIDs
first.
3. We group together more UIDs within a single batch by ignoring charset
and transfer-encoding on parts and grouping only by MIME part IDs (which
is the only thing you have to pass to the IMAP FETCH command---no idea
why we included this extraneous part data before, probably just
convenience.)
Example old grouping:
batch key: '[{"id":"2","transferEncoding":"QUOTED-PRINTABLE","charset":"UTF-8","mimeType":"text/html"}]'
batch UIDs: [356416,356418,356420,356423,356432,356433,356435,356436,356437,356442,356444]
batch key: '[{"id":"2","transferEncoding":"QUOTED-PRINTABLE","charset":"Windows-1252","mimeType":"text/html"}]'
batch UIDs: [353777]
In the new strategy, all of these messages will be downloaded with the
same FETCH command, reducing IMAP round trips before message processing
begins.
Fixes T7770
Test Plan: manual - connect a new account and see that most recent message downloads first
Reviewers: mark, evan, juan
Reviewed By: juan
Maniphest Tasks: T7770
Differential Revision: https://phab.nylas.com/D3838
Summary:
`isSending` was an artifact from the cloud sync engine that was used to
double check that the sending process for a multi-send draft had been
initiated. I don't believe the intermediate steps are API calls anymore,
and we've had the relevant code commented out for awhile. Time to kill it!
I've revived the double-sending tests in N1 to ensure we still have sufficient
checks against sending the draft again while the first call is still sending.
See D3834
Test Plan: N1 unit tests
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3835
Summary:
This code is dead and has confused me grepping around the codebase before.
If for some unexpected reason we need these routes back in the future, we
can always extract them from version control. For now the routes we aren't
using are a distraction.
Test Plan: been using Nylas Mail with this local patch all week
Reviewers: evan, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3833
Summary:
We can't try to set or remove the sent label on gmail accounts because
the operation will silently fail and cause the threads to later bounce
back.
This occurred when trying to delete or archive a thread that contained a
sent message, and we incorrectly tried to overwrite or remove all of the
labels on messages, without regard for sent.
This was causing https://github.com/nylas/nylas-mail/issues/2706 and
sending and archiving to immediately bounce back.
Addresses T7757
Test Plan: unit tests
Reviewers: halla, evan, spang
Reviewed By: halla, evan, spang
Differential Revision: https://phab.nylas.com/D3829
Summary:
Previously, we attempted to immeditely retry syncback tasks when a
RetryableError was encountered. However, if the error was an
IMAPConnection error, we would keep retrying with a broken connection,
which would keep failing.
The correct way to retry is to wait for the next sync loop, since at the
beginning of each loop we ensure that we are correctly connected to
imap.
To achieve this this commit simply marks a failed task as NEW if it
encoutners a RetryableError and we haven't retried too many times. To
keep track of the number of retries, we save a new field in the `props`
field of the request.
Test Plan: manual
Reviewers: evan, halla, mark, spang
Reviewed By: spang
Subscribers: khamidou
Differential Revision: https://phab.nylas.com/D3831
Summary:
I've decided to break my snooze patches in multiple parts to make it easier to review. This diff contains all the code related to running workers in the cloud. Cloud workers all inherit from the `Worker` class which defines a bunch of useful things like error handling.
What's left to do:
- spawn workers based on the plugin type
- add monitoring (I'm going to add a simple HTTP endpoint for that)
- writing a migration for the local sync db and the prod metadata db.
Test Plan: Tested manually.
Reviewers: halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3814
Summary: Make them map the same directory structure of `src`
Test Plan: unit
Reviewers: evan, halla, spang
Reviewed By: halla, spang
Differential Revision: https://phab.nylas.com/D3826
Summary:
syncState on folders may contain arbitrarily long arrays of UIDs
(particularly, failedUIDs). If we serialize this JSON column to
edgehill.db, we can end up serializing very large objects when
persisting the local task queue. When the queue contains many tasks,
this can balloon the JSON blob to megabytes, causing the main window and
the worker window to become unresponsive.
The UI doesn't need to know about IMAP bookkeeping internals, so
serialize the sync progress instead of the sync state. This has the
advantages that (1) we don't need to worry about future keys added
to the syncState being large and (2) when we add Exchange support
we already have an abstraction for sync progress.
Test Plan: manual
Reviewers: juan, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3817
Summary:
This will prevent us from showing error messages to the user when we
can automatically recover from the error
Test Plan: manual-- throw error from syncback task, check expected results
Reviewers: evan, mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3812
Summary:
There are 2 types of IMAP errors that need to be treated as retryable. See code
comments as to why.
Test Plan: manual
Reviewers: khamidou, evan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3811
Summary: This was broken because babel could not compile the `super` keyword
Test Plan: manual
Reviewers: evan, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3804
Summary:
EnsureMessageInSentFolder also needs to sync the sent folder to fetch
the uid of the newly moved message (like `MoveThreadToFolder` does)
Test Plan: manual
Reviewers: halla, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3803
Summary:
In `FetchNewMessagesInFolder`, sometimes we haven't synced anything in the folder
we are trying to fetch new messages in. Previously this would just throw
an error, now we properly check if we have a fetchedmax, and if not just
run a normal fetch.
Also, when the target folder box was already open, we were not fetching the /latest/ box status to check the latest uidnext value, so we would skip fetching new messages when in fact there were new messages to fetch
(This can happen for example when moving a sent message to the Sent
folder before we've started syncing the Sent folder)
Test Plan: manual
Reviewers: halla, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3802
Summary: see title
Test Plan: manual, deploy to staging, check that it works
Reviewers: evan, spang, tomasz, khamidou
Reviewed By: tomasz
Differential Revision: https://phab.nylas.com/D3800
Summary:
Syncback tasks that move messages to a different folder used to leave
those messages without a uid because it was unknown at the moment, and
it would only be discovered later on in the sync loop.
This had the unwanted effect of not allowing you to perform more than 1
syncback action on the same thread back to back (e.g. undoing an archive, or
sending and archiving immediately)
This commit makes it so that `SetThreadFolderAndLabels` and
`MoveThreadToFolder` runs a new sync task to fetch the new uids for the
moved messages.
We do this via a new sync task, `FetchNewMessagesInFolder` which /only/
fetches new messages in a folder (no fetching old messages or attribute
updates). This is a first step to cleaning up the gigantic
`FetchMessagesInFolder` task into smaller parts-- but that will come in
a separate diff. For now we want to fix the immediate problem.
See D3788 and D3789 for more details
Test Plan:
manually move threads around, undo moving threads, reply on a thread
and immediately archive
Reviewers: khamidou, mark, spang, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3798
Summary:
updateMessagesFromThread requires `messages` to be an array only when
recompute is falsy
Test Plan: manual
Reviewers: halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3797
Summary:
If there were any uidvalidity changes after we've completely synced a folder, we
would completely ignore them and not attempt to sync the folder.
Also, we weren't saving the latest uidvalidity from the box to the folder
syncState as soon as we recovered, we only saved it until after fetching
messages. This meant that if the operation was interrupted before updating
syncState.uidvalidity, we would always think that we were in a state of uid invalidity
Recovering from uidvalidity was also broken because we weren't resetting the
fetched ranges, and unnecessarily setting the folderId to null, which meant that
we would never restore the uids of messages we had already fetched.
Test Plan: manual
Reviewers: evan, khamidou, mark, halla
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3796
Summary:
If we attempt to operate on a box that is no longer open, we should make
the error retryable so that we re-open the correct box and continue
syncing instead of showing the scary red box to users
Addresses T7680
Test Plan: manual
Reviewers: evan, spang, halla, mark
Reviewed By: halla, mark
Subscribers: mark
Differential Revision: https://phab.nylas.com/D3792
Summary:
Given that we perform syncback actions optimistically, i.e. we save the changes to the database before they are synced, some messages might not have an IMAP UID if they haven't been synced in the loop.
When we encounter messages without uid, we could just skip them, but this would mean that we end up moving only a subset of the messages we intended to move, or not move any at all, while thinking that we /did/ move everything, which might cause the thread to bounceback later on when we sync the messages that were missing.
The permanent fix is for syncback actions to not succeed until their changes have been synced, which is coming up in a separate diff
Test Plan: manual
Reviewers: evan, spang, khamidou, mark
Reviewed By: mark
Subscribers: mark
Differential Revision: https://phab.nylas.com/D3789
Summary:
In JavaScript, null <= null is truthy. So if you set fetchedmax but not
fetchedmin and minUID, and then highestmodseq doesn't increment, we will
incorrectly skip updates on All Mail after the first batch of Gmail
inbox UID downloads, only making further progress on sync if the user
receives a new mail.
This patch tightens isSyncComplete() to require that all variables used
in the math comparisons are set.
Test Plan: manual 😢
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3787
Summary:
Previously we just assumed that we were in the "charging" state and wouldn't
update that until the state changed. This would cause us to throttle even if
the app was opened while plugged in. Now we don't do that.
Test Plan: Run locally, verify that we no longer throttle
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3782
Summary:
Previously, when updating message attributes during folder sync, we
would fetch local db messages only by folderImapUID. This was incorrect,
because messages in different folders could have the same uid, so our
query would return messages from other folders, causing us to
incorrectly update their attributes.
This commit ensures we fetch local messages in the current folder
Test Plan: manual
Reviewers: evan, halla, mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3777
We now correctly `await` for the `setFolders` and `setLabels`
operations, and make sure that the thread is always saved.
Apparently the `thread.id` check wasn't working as expected and we were skipping a
thread save, which only became apparent when we awaited for `setFolders`
and `setLabels`. Now, we always save the thread (like we did before this
method got updated)
Summary:
When I tested if we could use labels in place of the flag, I had the
Drafts folder selected, not All Mail, and the label did not show up.
Turns out that if you have All Mail selected, all draft messages will be
correctly tagged with the Draft label.
With this patch, we should correctly sync isDraft on all supported
providers and suppress these messages from the app until if or when we
decide to implement draft sync.
Thanks Brandon Long from the Gmail team for the help in getting this right.
Test Plan: manual
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3775
Summary:
Otherwise accounts with `Sent` and an `Outbox` will both get the `sent`
role, causing inconsistencies in sync
Addresses T7682
Test Plan: manual
Reviewers: khamidou, spang
Reviewed By: khamidou, spang
Differential Revision: https://phab.nylas.com/D3773
Summary:
Now that message processing is serialized, this is unnecessary. I
was debugging something else and watched the code step through the
exact same db query twice in a row.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3764
Summary:
Now that we don't do strict validation of certificates for non-major IMAP
providers this shouldn't come up as much, but when it does we're gonna
want a better error message to help support out.
I am not 100% sure there aren't other socket errors that should be fatal,
but this was the one I could figure out by test authing against a server
with a self-signed cert and grepping around the node socket source code.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3774
Summary:
It turns out we don't ever use the /account API endpoint.
The GET /accounts endpoint was designed to query all accounts connected to
a system for old K2, but we don't use that anymore. The environment
variable protecting the endpoint isn't set anywhere.
We used to `GET /account` from the N1 AccountStore to attempt to refresh
the health of the accounts. The accompianing diff to this one makes that
obsolete. We never need to query for the account health since the sync
loop pushes it to us through `Actions.updateAccount`.
We also never `DELETE /account` because our local-sync runs a function
called `ensureK2Consistency` that compares its DB against the Nylas Mail
`AccountStore`.
This file has always been a huge source of confusion to me and a massive
red herring for anyone trying to understand how the account system work.
The accompyaning diff has more comments explaining the existing system
Depends on https://phab.nylas.com/D3770
Test Plan:
Manually boot N1. Ensure existing account works. Add a new account. Remove
an account. Open the developer tools and check that all the tabs still
work.
Lots of grepping through the code base.
Reviewers: halla, mark, juan, khamidou
Reviewed By: juan, khamidou
Differential Revision: https://phab.nylas.com/D3769
Summary:
This will solve T7579 when saving messages to the sent folder. I
attempted to clean up the references code but decided it was better left for a
new diff, so added a bunch of TODO's in this diff
Test Plan: manual
Reviewers: halla, spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3766
Summary:
Unfortunately, many IMAP hosts outside the major ones do not have
certificates issued by a certificate authority, and it is very confusing
to folks to have their account auth not work. This patch relaxes our
certificate requirements for IMAP hosts outside the major providers.
It's cool that node 6 has secure TLS settings by default!
Fixes: T7673
Test Plan: manual
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3771
Summary:
We've been syncing drafts messages but not the drafts flag in K2, making
them appear in Edgehill as regular old messages.
This commit makes K2 sync the drafts flag, and also correctly label
folders called "Drafts" with the 'drafts' role.
Because 2-way syncing of drafts is very complex and error-prone since
you need to add new drafts and delete the old ones on every update, and
we reaally don't want to do things like create multiplying draft copies
or accidentally lose a draft someone started composing elsewhere, we
simply exclude messages marked as drafts from being serialized to
Edgehill through the delta stream for now. This removes the confusing
behaviour and also sets a better stage for completing drafts sync later.
Eventually we will also want to add functionality to allow users to
select their drafts folder, but for now this code does the right thing
in many more cases.
While investigating this behaviour, I also discovered a bug we've never
seen before where Gmail isn't applying the \Draft flag to draft
messages, no matter which folder we fetch them from. :-/ This is very
unfortunate and there's no way for us to work around it other than to
fetch messages in the Drafts folder and manually apply the flag locally,
since "drafts" is not a label in Gmail, only another IMAP folder. Brandon
Long from the Gmail team says that this is because they've had
problems with clients which sync drafts, so the Gmail web client and
mobile apps do not set the \Draft flag on drafts. (I don't get how this
solves their problem, but okay.) Let's solve the issue on Gmail if it
comes up by user demand—should be relatively straightforward to
implement, but it adds sync work & complexity.
Fixes T7593
Test Plan: manual
Reviewers: halla, juan
Reviewed By: juan
Maniphest Tasks: T7593
Differential Revision: https://phab.nylas.com/D3749
Summary:
We don't want to run message processing full tilt when a user isn't plugged in.
This diff adds some detection logic that causes message processing to be
throttled/unthrottled when a user unplugs/plugs in their computer.
Test Plan: Run locally unplugged and plugged in, verify that CPU use goes up/down
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3759
Summary:
See title
Depends on D3744
Test Plan: tested locally
Reviewers: spang, evan, juan
Reviewed By: spang, evan, juan
Differential Revision: https://phab.nylas.com/D3745
