Summary:
build-client, aka packaging, aka bundling is now separate from uploading.
This is both to compartmentalize our tasks a bit more and so we can add a
non-grunt windows task inbetween packaging and uploading.
No more heavily-overloaded PUBLISH_BUILD flag.
Added SIGN_BUILD flag instead.
No more TRAVIS and TRAVIS_PULL_REQUEST flag.
Test Plan: Manual
Reviewers: halla, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4208
Summary:
This removes specs from the build production version.
One of the major issues with the windows build was that one of the package
specs included a .node compiled file that the codesigner attempte to sign,
but couldn't find for some reason.
In testing, removing the specs from the build prod version fixed this.
While we could just remove the one offending test, I think we should just
remove the whole suite from the build version. That'd save in the number
of files we have to ship, save download time, save build time, and have
less chance for some symlinked issue failling a build in the future.
Test Plan: Manual
Reviewers: khamidou, halla, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4206
Summary:
It always drives me crazy when folks use "setup" (no space) as a verb,
because the verb version has a space in it.
Also remove the word "Setup" from the displayName for custom IMAP accounts
because it doesn't make sense for how we use this field, e.g.
a8f36b88d9/packages/client-app/internal_packages/onboarding/lib/page-account-onboarding-success.jsx (L27)
will display "Sucessfully connected to IMAP / SMTP Setup!" with the old
wording when you successfully auth a custom IMAP account.
Test Plan: manual
Reviewers: evan, juan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4199
Summary:
Dunno if I'm the only one having this problem, but I spent at least an
hour over the last two days wondering why hapi just sits there and hangs
in the Joi validation step when I copy-paste calls to /auth from the
developer bar to my terminal. It turns out that if you fail to send the
correct Content-Type when sending a JSON payload, Joi just hangs
forever. (WTF!)
Test Plan: manual
Reviewers: halla, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4204
Summary:
Previously, n1 cloud auth errors were stuffed into an account's
syncState. The problem with that is that the sync loop manages that
state, and as long as the sync loop is running it will set that state to
running. However, it might be the case that the sync loop is running but
we can't connect to n1Cloud, so even though we would set the `syncState`
to `n1_cloud_auth_failed`, the sync loop would just set it back to
'running', and the user wouldn't see the error notification indicating
that it can't connect to n1Cloud
This commit makes it so we keep tracj of the auth failure state for
n1Cloud in a separate field, and makes sure that the error notification
component shows that error.
Test Plan: manual
Reviewers: mark, spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D4172
Summary:
On rare occasions, the api error would not have a body, and we would try
to access the message prop on the body, which would in turn throw an
error.
This commit prevents this error, and adds better messaging to the sentry
report
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4194
Summary:
This commit adds a new perf metric for inline composer times.
Additionally, it consolidates the timer logic for all other types of draft
creation (mailto links, dropping a file in the app dock icon, etc).
Test Plan: manual
Reviewers: halla, evan, mark
Reviewed By: evan, mark
Differential Revision: https://phab.nylas.com/D4186
Summary:
Before this commit, we would call .close() onError and when the
connection closed. We also closed the connection onError, even though
NylasLongConnection had also closed it, so we ended up calling close a
bunch of times. This would cause us to set a bunch of timeouts to retry
unecessarilly.
This commit makes it so we ensure there's only one retry timeout and
consolidates the logic that calls .close() so we don't call it so many
times unnecessarily
Test Plan: manual
Reviewers: spang, mark, evan
Reviewed By: mark, evan
Differential Revision: https://phab.nylas.com/D4166
Summary:
This commit consolidates the `DeltaConnectionStatusStore` and the
`DeltaConnectionStore` which kept track of very similar state and made
sense to be the same store (as per feedback in D4118#77647)
Given that this state needs to be available app-wide for plugins to
query the status of delta connections, `internal_packages/deltas` was
removed (given that it only activated that store), in favor of having the
unified store inside `src/flux/stores` and available via `nylas-exports`
The `deltas` package also contained some contacts-ranking code, which is
no longer in use until we restore that fetaure, so I created a
`internal_packages/contact-rankings` which contains this unused code for
now.
Test Plan:
manually open, close, end delta connections, verify that I'm getting
correct results. unit tests to come
Reviewers: halla, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4145
Summary:
This DRY's up code that use these status codes
Depends on D4130
Test Plan: manual
Reviewers: spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4140
Summary:
When reporting error inside `IdentityStore._fetchIdentity`, we would
pass null as the `extra` parameter for `reportError`. This would cause
us to try to assign something to null and throw an error while reporting
the error.
(Even though `extra` has a default value of `{}` (see
declaration of `NylasEnv.reportError`), passing null will be interpreted
as a valid arg and it wont get assigned the default value)
Thankfully, the second error /is/ reported to sentry: https://sentry.io/nylas/nylas-mail/issues/230589717/
Test Plan: manual
Reviewers: mark, halla, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4150
Summary:
This is not a bug. Maaaybe we want to report this to MixPanel for individual
user debugging (how do we do that?), but it doesn't make sense to report this
error to Sentry.
Example:
https://sentry.io/nylas/nylas-mail/issues/230530818/
Test Plan: yolo
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4130
Summary:
If you have the console open and are looking at the Source panel or
something, this will constantly bring you back to the Console panel. Won't
do that anymore with this chec
Test Plan: manual
Reviewers: spang, mark, halla, juan
Reviewed By: mark, halla, juan
Differential Revision: https://phab.nylas.com/D4162
Summary:
Errors weren't getting reported because of a circular reference causing
JSON.stringify to fail.
We can't add the whole API `response` object onto the error object,
because sometimes (like when the delta connection errors with a 401), the
`response` object is circular. We only use it to the extract the status
code anyway and it's a huge object. This diff excludes them from the
APIError objects.
We also have code to recover just in case JSON.stringify errors for some
other weird reason. It'll still attempt to report the error prefixed with
`Recovered Error`
Finally, we used to spit only the `error.stack` to the console. This meant
what we saw in the console didn't properly include the error messages
being incredibly not useful.
It's better to console.error the whole error object since that'll more
nicely display on the inspector console
Test Plan: manual
Reviewers: juan, spang, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4154
Summary:
Previously if we got back a huge number of unknown UIDs from IMAP search
we would try to sync all of them at once. This could lead to hanging the sync
loop trying to download tons of messages. This diff limits the UIDs we're
willing to sync per task to 500 and splits each task up into chunks of 25
messages so that we don't try to download all of them at once. If we need
to sync more than 500 uids then at the end of the syncback task it will
queue another task to run the next time the sync loop rolls back around.
Test Plan:
Run locally, verify that we gracefully handle various situations
including cancelling during the syncback task, cancelling between syncback
tasks, huge numbers of results, etc
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4142
Summary:
This commit bumps the version of sequelize to point to the latest
version in our fork. This version bump also includes latest updates from
upstream sequelize since the last time we bumped the version.
Notably, it includes a critical fix that will prevent the sync
loop from getting stuck on rare occasions.
Specifically, sequelize's `Query.prototype.run` could in some cases return a
Promise that would /never/ resolve or reject, effectively halting the
execution of any code that was waiting for that promise. This was due to
a lack of error handling inside a the query's `afterExecute` function; if an
error was thrown there, the enclosing Promise would never reject, and
the error would just remain uncaught. An example of such an error that
could cause this scenario is: T7742 - https://sentry.io/nylas/nylas-mail/issues/230016155/
Sync getting halted in this way can produce a variety of user facing
bugs like not being able to send or it taking an absurd amount of time
(hours), tasks never finishing or taking an absurd amount of time to
complete, new mail not arriving, among others
The commit that fixes this is: 0deeda9e1a
The full set of changes introduced by this version bump are: https://github.com/nylas/sequelize/compare/nylas-3.30.0...nylas-3.40.0
**Note:**
It is important to note that sequelize might still halt the sync loop if
there are other places in the code that can return Promises that never
resolve or reject under certain circumstances. I can consistently reproduce this
scenario when an error is thrown inside `afterExecute`, and I've seen it
happen in the wild, but I've also ran into this type of sync loop halting (db
promises never resolve) without any sequelize errors being thrown, which
suggests that there are other places in the sequelize code that might end
up returning a Promise that will halt sync.
Unfortunately, we don't have a good way to detect and report when this
happens yet, but we are adding one in upcoming diffs in order have data
on how many people are running into this, and/or if this patch completely
fixes the issue. Otherwise, we'd need to audit sequelize's code.
Should resolve T7837 and T7767
Test Plan: manual
Reviewers: spang, mark, halla, khamidou, evan
Reviewed By: evan
Subscribers: mg, tomasz
Maniphest Tasks: T7767, T7837
Differential Revision: https://phab.nylas.com/D4152
Summary:
This commit splits apart the `AccountDeltaConnection` class, which was
in charge of listening to both cloud /and/ local deltas by way of an
artificial interface, `DeltaStreamingInMemoryConnection`.
Splitting this into 2 modules with separate responsibilities will hopefully
make this code easier to reason about and reduce some cruft and unnecessary
indirection.
Specifically, this commit makes it so:
- `DeltaConnectionManager` is only in charge of starting and ending `DeltaStreamingConnection`s, which are solely in charge of listening to deltas from the cloud api
- `LocalSyncDeltaEmitter` no longer unnecessarily emits events for the `deltas` package to listen to but rather directly processes and saves those deltas from the K2 db to edgehill.db
- `LocalSyncDeltaEmitter` is also in charge of keeping track of the latest received cursor, under its own JSONBlob key in edgehill.db. This migrates localSync cursors saved under the old key.
- `LocalSyncDeltaEmitter` is now instantiated and managed from within the `SyncProcessManager` as opposed to the `SyncWorker`. Apart from removing extra state from the `SyncWorker`, this removes dependencies on the client-app environment from the sync-worker.
- `DeltaStreamingInMemoryConnection` and `AccountDeltaConnection` are now gone
(Sorry for the big diff! This one was a little hard to split up without landing something broken)
Depends on D4121
Test Plan: manual + unit tests planned in upcoming diff
Reviewers: halla, mark, evan, spang
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4122
Summary:
This is to make it available to all plugins in the app
Depends on D4120
Test Plan: manual
Reviewers: halla, mark, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4121
Summary:
This commit completely refactors `DeltaStreamingConnection`, notably
introducing the following changes:
- Right now, `AccountDeltaConnection` establishes both delta connections to the cloud api and to the `client-sync` database (K2). This class is meant to disapper in favor of splitting into two different classes meant for syncing with the n1Cloud api and the local database. Specifically, `DeltaStreamingConnection`'s only responsibility is now to connect to the n1Cloud API and establish an http streaming connection for metadata deltas, etc. This class no longer unecessarily inherits from `NylasLongConnection`, which removes a lot of unecessary callback indirection.
- The statuses of the n1Cloud delta streaming connections are now stored in as JSONBlobs in edgehill.db under new keys. This commit ensures that the data is correctly migrated from the old key (`NylasSyncWorker:<account_id>`).
- The `DeltaStreamingConnection` now correctly retries when closed or errors. This logic previously existed, but was removed during the K2 transition: https://github.com/nylas/nylas-mail/blob/n1-pro/internal_packages/worker-sync/lib/nylas-sync-worker.coffee#L67
- Delta connection retries now backoff using the `ExponentialBackoffScheduler`
- Attempt to restore the delta connection whenever the app comes back online
Depends on D4119
Test Plan: manual + planned unit tests in upcoming diff
Reviewers: halla, mark, evan, spang
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4120
Summary:
For legacy reasons, NylasLongConnection did not take the `onResults`
callback as an option, but rather it was a method you had to call on the
connection, which took the callback, and which you had to call after
instantiating the connection. This was an annoying and clunky interface.
This commit makes it so NylasLongConnection takes an `onResults` option
as one would expect it to, and updates any references to the old
interface
Depends on D4118
Test Plan: manual
Reviewers: halla, mark, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4119
Summary:
Turns out, NylasSyncStatusStore doesn't need to concern itself with the
status of delta connections.
This is legacy cruft from the days when this store used to provide the
online status of the app based on the delta connections. Now we have an
OnlineStatusStore, so we don't need this at all
Instead of stuffing that state in that store, this commit adds a small
DeltaConnectionStatusStore to provide an easy way to listen to changes
on the status of delta connections. The only part of the app concerned
with that state is the DeveloperBarStore in order to render the green
circles for delta connections in the worker window.
Depends on D4117
Test Plan: manual
Reviewers: mark, halla, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4118
Summary:
We have a bunch of different endpoints and it'd be helpful to know which
endpoints are failing, not just which status codes are being thrown. e.g. while
trying to reproduce send failures, I've been able to cause /auth to fail with a
504, but I couldn't tell which endpoint was failing with the 504 until I added
this extra differentiator.
Test Plan: manual
Reviewers: mark, juan
Reviewed By: mark, juan
Differential Revision: https://phab.nylas.com/D4128
Summary:
Keeping a sourceMapCache uses a great deal of memory. Don't do it if we
don't have to.
Test Plan: Run locally, verify that we no longer generate a sourceMapCache
Reviewers: evan, juan, spang
Reviewed By: evan, juan, spang
Differential Revision: https://phab.nylas.com/D3964
Summary:
We never want to ship the last release we'll ever ship.
This can happen if the new version's autoupdater is broken.
To test this we upload to a version n+1 higher than the current one.
Test Plan: manual
Reviewers: juan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4125
Summary:
When searching using IMAP/Gmail commands we sometimes get back UIDs for
messages that we have yet to sync. Previously we would just ignore these
results, which would decrease the quality search results for quite some
time during initial sync. This diff enables us to eagerly sync the unknown
messages we get back from the provider by creating a syncback task which
interrupts the sync loop and runs a sync task for the unknown UIDs.
Test Plan: Run locally, verify that we sync unknown messages
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4101
Summary:
This commit adds a new option to the `reportError` interface to allow
to rate limit specific kinds of errors that are known to be reported a
lot if they fall in a loop for example.
For now, it only allows to impose a fixed reporting rate per hour for a specific
error, and will not report any errors past that rate before an hour has
passed.
Depends on D4099
Test Plan:
manually check that it reports and does not report errors based on the rate
limit data passed
Reviewers: khamidou, mark, evan, halla, spang
Reviewed By: evan, halla, spang
Differential Revision: https://phab.nylas.com/D4100
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
