Summary:
Previously we stored message bodies uncompressed inside two different
databases. This was bad for a few reasons:
* Duplicate data in multiple places is an obvious waste of disk space
* Uncompressed data also made the disk footprint bigger than it might
otherwise be
* Storing these large message bodies in the database made the db file
larger than it otherwise could have been, increasing the size of
tables and slowing down queries.
This diff adds support for storing message bodies outside of the
database in compressed flat files. It changes the use of the body column
in the K2 database and the MessageBody table in the edgehill database to
contain a blob of JSON that contains a path to the file on disk. We
use the new format in an incremental fashion without having to perform an
actual database migration by first checking if the body matches our expected
JSON format and treating it appropriately if it doesn't. Both databases refer
to the same file on disk, thus deduplicating the messages bodies. We also
transparently support gzipping the message bodies stored on disk when we
read from/write to the files. The real world space savings depends on the
compressibility of the messages, but we've seen up to ~60% improvement in disk
space usage for certain inboxes. Typical savings are closer to 20%. Also, by
storing messages as separate files on disk we can potentially integrate with
Spotlight search at some point in the future.
Test Plan:
Run locally, make sure that upgrade to this doesn't hose things,
look at size of DB, read emails
Reviewers: evan, spang, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4422
Summary:
We weren't passing benchmarkMode through in the default window options,
so the worker window (or any other spawned window) would think it wasn't in
benchmark mode.
Test Plan:
Run locally, verify that the worker window thinks it's in
benchmark mode
Reviewers: halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4427
Summary:
Given that this error is unrecoverable it shouldn't be retryable.
Additionally, this commit improves error handling for this error in a
few ways:
- Don't delete all databases, just edgehill.db which is the one we know is corrupted. (Use `handleDatabaseUnrecoverableError` instead of `Actions.resetEmailCache`)
- Message the user about the database being reset so the app doesn't restart without notice, and make sure that this message is only displayed once by moving it to the main process
Test Plan: manual
Reviewers: mark, spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D4433
Summary:
This commit addressess issue T8135, which prevents the app from
starting.
This was happening because when 3111c166 landed, we added database
access from the main (backend) electron process to be able to read the
identity now stored in the database: https://github.com/nylas/nylas-mail/commit/3111c166#diff-1efa26fa0ae1603366b2c0033d971028R44
However, we omitted to add any error handling, so if the database failed to open
due to a database malformed error (which it does:
https://sentry.io/nylas/nylas-mail/?query=is%3Aunresolved+release%3A2.0.14+malformed&statsPeriod=14d), the app will just fail to start,
given that this happens during the initialization of the main process.
Additionally, the fact that we had no error handling increased the error
reports for malformed errors given that we would never handle them, so
every-time we opened the app we would report the same error
This commit adds the same error handling we have in the DatabaseStore
and moves the code around so it's available both in the main and
renderer processes.
After this commit, if the database fails to open during main process
initialization, due to malformed errors or others, we will correctly
inform the user that the database is corrupted, rebuild it, and restart
the app.
Test Plan:
manually throw errors during setup, verify that we handle them
correctly
Reviewers: mark, spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D4431
Summary:
Which was arguably already a code smell being inside the
DatabaseStore
Test Plan: manual
Reviewers: mark, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4430
Summary:
It seems that sometimes we get events that don't have `event.detail.reason`.
This commits tries to make our best guess, or report info about the event shape
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4420
Summary:
This changes the `name` option we pass to `electron-windows-installer`,
which makes it so that the app is installed under
`AppData\Local\NylasMail` instead of `AppData\Local\Nylas` where the
old N1 binary lives
Test Plan: manually tested the builds from `ci-test-4` branch on the surface machine
Reviewers: mark, spang, khamidou, halla
Reviewed By: khamidou, halla
Differential Revision: https://phab.nylas.com/D4419
Summary:
This makes grepping the codebase really painful. We don't need these
docs here
Test Plan: manual
Reviewers: mark halla spang evan
Subscribers:
Summary:
We listen for unhandled rejections on both the `process` and the `window`. From
my understanding, both are necessary because some errors might escape
one or the other. However, most errors get caught by both handlers, so
we don't want to double report them to sentry (or the console).
To prevent this, this commit debounces the unhandled rejection handler
for a very short time so that the same error only gets reported once.
Additionally, it rate limits reports to sentry based on the stack trace
Test Plan: manual
Reviewers: mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4413
Summary:
Previously, we were listening for unhanlded rejections on both the
`window` and the `process`. However, the window handler didn't receive
an error as a parameter, but rather a window event. When trying to
report this event and sending it over IPC we would get an ipc parse
error.
This commit makes it so we correctly report the error instead of the
window event.
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4412
Summary:
We were reporting 1m+ of these events, heavily contributing towards our
quota
Test Plan: manual
Reviewers: halla, mark
Reviewed By: halla, mark
Differential Revision: https://phab.nylas.com/D4406
Summary:
Add a sequentialId check in isDependentOnTask() so that the first
SyncbackMetadataTask won't get blocked on other SyncbackMetadataTasks
in the queue for the same pluginId.
Addresses T8112
Test Plan: Manual
Reviewers: spang, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4404
Summary:
Put the IMAP option back on the provider selection page within
onboarding. Also remove the 'hidden' property on account types since I
don't think we'll be using it for any other providers, and remove the
'Add Custom IMAP Account' from the application menu.
Addresses T8097
Test Plan: Manual
Reviewers: spang, mark, juan
Reviewed By: mark, juan
Differential Revision: https://phab.nylas.com/D4402
Summary:
Previously, every time our NylasLongConnection changed its status to
`connected`, we would reset our backoff scheduler, assuming that
every time our status changed to `connected` it meant that we would not
receive any errors.
However, if our backend is down or overloaded (or returning 401s like in
our recent outage), we would do the following:
1. Establish connection, change status to `connected`
2. Reset backoff scheduler
3. Receive error
4. Close connection
5. Schedule next retry using backoff scheduler. Delay will be 1s or less, because scheduler was reset in 2.
6. Retry after delay,
7. Repeat from 1., ad infinitum
This caused us to consistantly hammer our servers and render our
exponential backoff useless.
This commit makes it so we conly reset the backoff scheduler when we
actually receive deltas, and increments the max backoff to 10 minutes
Test Plan:
run against local n1Cloud, manually return 401s from the api, verify that we
didn't backoff at all before this diff, verify that we backoff correctly after
this diff
Reviewers: mark, spang, halla
Reviewed By: mark, spang, halla
Differential Revision: https://phab.nylas.com/D4401
Summary:
There are some settings that apply to dev and prod modes that we don't want
to use while benchmarking. E.g. the folder where we store messages,
whether we generate long stack traces in our bluebird promises, etc.
This diff adds a benchmark mode so that we can change these settings to
something that works better for benchmarking.
Test Plan: Run locally, verify it works
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4374
Summary:
Previously, SyncbackMetadataTasks for the same plugin did not depend on
each other, so they could try to modify the same piece of metadata
concurrently, which would likely produce version conflict errors.
This was happening with send reminders because we were queuing 2 of
these tasks back to back upn send success. Adding this dependency fixes
the version conflict errors when setting the metadata
Test Plan: manual
Reviewers: evan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4393
Summary:
This was annoying because we wouldn't fetch the identity when running `local`
env which produced all sorts of errors
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4380
Summary:
Previously, the generic IMAP auth screen presented one security option to
users: "Require SSL". This was ambiguous and difficult to translate into
the correct security options behind the scenes, causing confusion and problems
connecting some accounts.
This patch does the following:
* Separates security settings for IMAP and SMTP, as these different protocols
may also require different SSL/TLS settings
* Reworks the generic IMAP auth page to allow specifying security settings
with higher fidelity. We looked at various different email apps and decided
that the best solution to this problem was to allow more detailed
specification of security settings and to ease the burden of more options
by having sane defaults that work correctly in the majority of cases.
This new screen allows users to pick from "SSL / TLS", "STARTTLS", or "none"
for the security settings for a protocol, and also to instruct us that
they're OK with us using known insecure SSL settings to connect to their
server by checking a checkbox.
We default to port 993 / SSL/TLS for IMAP and port 587 / STARTTLS for SMTP.
These are the most common settings for providers these days and will work
for most folks.
* Significantly tightens our default security. Now that we can allow folks to
opt-in to bad security, by default we should protect folks as best we can.
* Removes some now-unnecessary jank like specifying the SSLv3 "cipher"
in some custom SMTP configs. I don't think this was actually necessary
as SSLv3 is a protocol and not a valid cipher, but these custom
configs may have been necessary because of how the ssl_required flag was
linked between IMAP and SMTP before (and thus to specify different
settings for SMTP you'd have to override the SMTP config).
* Removes hard-coding of Gmail & Office365 settings in several
locations. (This was a major headache while working on the patch.)
This depends on version 2.0.1 of imap-provider-settings, which has major
breaking changes from version 1.0. See commit for more info:
9851054f91
Among other things, I did a serious audit of the settings in this file and
"upgraded" a few servers which weren't using the SSL-enabled ports for their
provider to the secure ones. Hurray for nmap and openssl.
Test Plan: manual
Reviewers: evan, mark, juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D4316
Summary:
Make sure SyncActivity.getLastSyncActivityForAccount returns a sensible
default to prevent runtime errors when the SyncProcessManager uses it
Test Plan: manual
Reviewers: evan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4366
Summary:
The unapply transformation was incorrectly passing the props parameter
to buildAnchorTag() as a string, even though it expects an object.
buildAnchorTag() goes on to stringify this parameter, which causes extra
surrounding quotations and escape slashes in front of all the other
quotations. The unapply transform is applied on several re-renders,
which causes the number of escape characters to unboundedly increase.
Firstly, this was causing inline images to not appear properly in the
draft, and secondly, the large number of escape characters was making
the draft body large enough to make the app unresponsive.
This generally wasn't an issue because the unapply transformation is
only applied when there has been an apply transformation, and this is
triggered by the SyncbackDraftTask. This task was previously only queued
upon send, in which case the user never re-opens the draft. Now,
enabling send-later on a draft will queue the task, and these issues
appear if the user re-opens the draft for editing.
This diff makes it so that the unapply transform passes in the props as
an object, like the buildAnchorTag function expects.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D4348
Summary:
Remove packages from the disabled package list when they have been
migrated to be enabled by default.
Also, the last migration would not have worked properly anyways because
the daily channel was already on 1.0.56, and we check for greater-than
rather than equal-to. Bump that version to match the next update.
Test Plan: manual
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D4335
Summary:
Previously, is you signed out of your NylasID, you would not pick up new
updates because we never updated the autoupdater url to use the new id.
Test Plan: manual
Reviewers: mark, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4346
Summary:
Sometimes, when logging out of your NylasID and restarting the app, we
would continue making some requests from the worker window that required a
NylasID. This would make the app enter a restart loop and become
completely unresponsive because when we made a request without a
NylasID, we would force the user to log out and restart the app, and
then we would again make the requests without the id, ad infinitum.
To fix this, we make sure we have a NylasID before making any requests
that require it
Test Plan: manual
Reviewers: halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4344
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/D4328
Summary:
Before this patch, the IdentityStore would initialize in our empty hot
window. However, hot windows don't receive any `action-bridge-message`s,
which include DB updates. Since the hot window loads first, it was with a
stale verison of the Identity. The main window fetches a fresh identity,
but that fresh update failed to get to new composers because the hot
window wasn't listening to changes to the DB.
This makes it such that the IdentityStore properly boots up when the
window props change.
Test Plan: manual
Reviewers: halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4327
Summary:
This adds feature limit modals (graphics pending) for reminders and send
later
Test Plan: manual
Reviewers: juan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4322
Summary: This shaves off ~150ms trying to issue this query
Test Plan: manual
Reviewers: evan, spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4317
Summary:
Bodies that already exist on K2 shouldn't be overridden.
See the comment for how this caused open/link tracking to fail in certain
cases when a lot of messages were being sent
Test Plan: manual
Reviewers: halla, mark, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4319
Summary: It was really slow, and we no longer need it.
Test Plan: Run locally, verify that we no longer do that super slow query
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4312
Summary:
Removes some VERY long running `ANLAYZE` queries. Was taking up to 50
seconds on my 9GB database on every boot
https://sqlite.org/pragma.html#pragma_optimize
Test Plan:
I tested to make sure the app still quits quickly. It does. The SQLite
docs also say this should be fast.
Reviewers: halla, spang, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4314
Summary:
This improves window open times by about 70ms
We would spend a very long time in the backend browser process building a
new hot window when we didn't need to do that until much later
Test Plan: manual
Reviewers: halla, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4311
Summary:
This prevents us from hitting the endpoint before the database for that
account is initialized and it's actually syncing.
Before this commit, we would throw errors upon adding an account
Test Plan: manually add, remove accounts, run benchmark script
Reviewers: mark, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4305
Summary:
Creates a new `DatabaseStore.write` method alongside
`DatabaseStore.inTransaction` that doesn't always wrap trivial queries in
a transaction. SQLite does this automatically.
Test Plan: manual
Reviewers: spang, mark, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4303
Summary: Renamed in prep for next few diffs
Test Plan: manual
Reviewers: juan, mark, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4302
Summary:
Use the wonderful `debug` library instead of all our random flags.
You can now do things like: `DEBUG="app:*,sync:*" npm start` to print out
everything.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4297
Summary: See title
Test Plan: Run locally, verify that attachments are returned
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4293
Summary:
Every persistModel would trigger a large number of downstream updates.
These weren't necessary for thread indexing and causing a lot of
unnecessary DB thrashing. This adds a `silent` flag to `persistModel` and
its ilk that just does the write.
significantly improve performance, and also contribute to T8046
Test Plan: manual
Reviewers: spang, mark, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4295
Summary:
This adds better logging to the DB
You can use `ENABLE_SEQUELIZE_DEBUG_LOGGING=true` and
`ENABLE_RXDB_DEBUG_LOGGING=true` to spit out the raw queries of both DBs.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4294
Summary:
We would mark modified threads for reindexing, the thread search indexer
would reindex them, which would trigger a notification that the thread
had been modified, so we would mark the thread for reindexing, ...
Now we keep track in memory of which threads we've marked for reindexing
so we avoid re-marking them when the update notification arrives later.
Test Plan:
Run locally, verify same threads aren't getting continuously
reindexed and that the in-memory set doesn't grow unboundedly
Reviewers: spang, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D4289
Summary:
This commit makes a few changes to how we log database queries:
- We log queries in 2 different places now:
- When `DatabaseStore._executeLocally` takes longer than 100ms. This might be due to the actual db operation taking long, or it might be due to our retry logic, or potentially other factors.
- When the actual raw database operation takes more than 100ms.
- When raw database queries take more than 100ms, we log out the query plan if it is a `SELECT` query and we are in dev mode.
- If `DEBUG_TO_LOG` is true, we always log all queries (including background queries)
Test Plan: manual
Reviewers: mark, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4288
Summary:
- Rename `databaseAgent` usage to `background`, in the code and in the logs, which I
believe is less confusing. Often times found myself asking what was this agent thing in
the logs.
- Make messaging more explicit
Test Plan: manual
Reviewers: evan, mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4285
Summary:
This commit makes it so that when we retry database queries after
encountering a db locked error, we back off for at most 500ms, instead
of the current max of 3s. This meant that a single query could
potentially take 3+ seconds and block the transaction queue for that long.
Test Plan: manual
Reviewers: evan, mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4283
Summary:
After introducing retry logic, we would always defer db queries to the
next tick even if there was no retry delay present.
This would cause almost all db queries take more than 100ms even if the
actual db operation was really fast. This affected app performance
negatively
Test Plan: manual
Reviewers: evan, halla, mark
Reviewed By: halla, mark
Differential Revision: https://phab.nylas.com/D4282
Summary:
The problem actually was that we weren't applying the transform for Gmail
messages sent to 1 recipient
Test Plan: manual
Reviewers: halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4291
Summary:
We weren't stripping link and open items from messages we sent properly.
While we stripped it from the first outgoing message, when the sync loop
came back around it would have them back in. This adds it in the message
processor to extra ensure that ANYTHING that comes from us (or one of our
aliases) gets the open/link tracking stripped
Test Plan: New tests
Reviewers: juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D4287
Summary:
Prior to this diff, the "in:" search query syntax didn't work for IMAP.
This diff implements "in:" by changing the IMAP search backend to take
folder context into account and emit the appropriate queries for each
folder. Queries that include "in:foo" will replace the corresponding AST
nodes with 'ALL' or 'NOT ALL' depending on whether or not the current folder
is "foo". We also now filter which folders we search based on which
folders are referenced in the query.
Test Plan: Run locally, verify that in: works quickly
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4284
This commit is the same as 7d409f5.
This commit was accidentally reverted in 95ec679, because I (juan) was being
dumb. So I'm re-committing here.
Summary:
We weren't doing that, so we would have accurate search index info for
threads when they were, e.g., moved to another folder.
Test Plan: Run locally
Reviewers: spang, evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D4260
Summary:
By default, the 'local'/'development' environment assumes that all services
are running locally. This makes sense.
However, if you're not making changes to auxiliary services such as
billing.nylas.com and hence do not have them running locally, firing up
Nylas Mail with the 'local' env will not work. It's a bit of a pain to
get a local billing service running locally, especially if you're not
making any changes to that service. To ease development, change the
default billing URL during development to staging, and allow manually
overriding the billing server URL to point to localhost in the case
that you're making changes to the billing service also.
To override the billing URL for local development, run like this:
npm run cloud -- --env billing_local
BILLING_URL=http://billing.lvh.me:5555 npm start
This is both a problem that I've run into during development as well as
(my hunch) why Karim accidentally landed a global override of the
billing server URL.
Test Plan: manual
Reviewers: evan, khamidou, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4202
Summary:
This prevented the /health endpoint from being available in the
local-api
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4280
Summary:
This patch will prevent users from being able to connect accounts which sync
mail but fail to send.
This commit includes a couple pieces:
* Adds a call to nodemailer's `verify()` function in the /auth endpoint
* Adds Error object conversion for SMTP errors. Since we don't implement our
own connection object or connection pool for SMTP, we simply wrap the couple
places we call functions from nodemailer that connect to SMTP, namely
SendmailClient's _send() and the new verify() call in /auth.
* Moves RetryableError to the 'errors' module since it's now a base class for
retryable IMAP //and// SMTP errors.
* Moves the main `smtpConfig()` logic which used to live on the Account model
into AuthHelpers so it can be shared between the Account model and the verify
code.
* Converts a few different places to use `import` syntax instead of
`require` syntax for module imports. Apologies for not splitting this out
into a separate diff—would have been a fair amount of work and looks not too
difficult to skim over in the context of the rest of the patch.
* Fixing a bug in a previous commit where erroring sends would crash because of
using `this._transporter.options` instead of `this._transporter.transporter.options`
Test Plan: manual
Reviewers: evan, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4200
Summary:
This diff will give us insight into how people use Nylas Mail on their
computers with respect to how mobile they are (i.e. whether they use NM
on battery or plugged in).
Test Plan: Run locally
Reviewers: evan, spang, juan
Reviewed By: juan
Subscribers: gleb
Differential Revision: https://phab.nylas.com/D4267
Summary:
Prior to Nylas Mail, the Nylas Cloud API provided an endpoint that
returned rankings for contacts which it computed based on how frequently
and how recently a user sent mail to a recipient. This diff reimplements
that functionality in Nylas Mail. This should improve contact
auto-complete when composing emails to frequently contacted recipients.
Test Plan: Run locally, verify that frequent contacts are suggested earlier
Reviewers: spang, evan, juan
Reviewed By: evan, juan
Maniphest Tasks: T7948
Differential Revision: https://phab.nylas.com/D4253
Summary: See title. Part of T7681.
Test Plan: ran the specs
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4264
Summary:
Periodically ping client-sync's /health endpoint and store the latest
sync activity. If we get an ECONNREFUSED error, the worker window is
unavailable. Report the last known activity and restart the worker
window. Part of T7681.
Test Plan: manual, specs
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4263
Summary:
We still don't know exactly what scenarios cause us to get 'database is
locked' errors, but generically this means that we have multiple
processes trying to access the database at the same time. In an attempt
to handle this gracefully, this diff makes it so that we retry the
queries in these cases. Theoretically, the database should free up once
the other process is done using it, and the erroring process just needs
to wait its turn. We still throw an error after 5 retries, so if there's
a larger issue, we'll still be able to tell in Sentry.
Addresses T7992
Test Plan:
I opened a transaction in the worker window and then tried to
do the same in the main window. If I didn't release the transaction in
the worker window, the main window eventually errored. If I did release
the transaction, the main window continued creating its own transaction.
Reviewers: mark, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D4254
Summary:
We weren't doing that, so we would have accurate search index info for
threads when they were, e.g., moved to another folder.
Test Plan: Run locally
Reviewers: spang, evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D4260
Summary:
Before this commit, it was impossible to remove inline images via the x
button
Test Plan: manual
Reviewers: mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4257
Summary:
I had a bit of downtime this morning so I decided to look into how to store Electron crash data. Electron, like Chromium, stores crash data in an arcane file format named minidump.
There's a bunch of services you can use to store files formatted in this format, like Mozilla's Socorro or zcbenz's own mini-breakpad-server but they're all pretty hard to install. Luckily, it turns out there's a Ruby gem to process minidump files and send them to Sentry! Seeing that, I whipped up a quick sinatra service and hosted it on Heroku – you can take a look at the source code here if you're curious: https://github.com/khamidou/electron-breakpad-sentry
This diff adds the client-side code we need to start sending crash reports.
Test Plan: Tested manually.
Reviewers: juan, evan
Reviewed By: juan, evan
Maniphest Tasks: T7926
Differential Revision: https://phab.nylas.com/D4259
Summary:
We'd like to get an idea of how long the client app is having to wait
when requesting a file. Waiting can cause things like inline image
attachments to appear very slow to load.
Test Plan: Run locally
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4255
Summary:
The original name seems like it's initiating the download, when really
it's just returning the data of an already in-progress/completed
download.
Test Plan: Manual, specs
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4251
Summary:
We need to download the files and then treat them as uploads. Rather
than using an actual Upload object, which would require another data
transfer, we create an object with all the necessary Upload-like
properties and point it to the downloaded file.
Addresses part of T7960
Test Plan: Manual, some specs
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4249
Summary: Using `await` instead of `advanceClock()` fixes all the things!
Test Plan: Ran the spec file
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4248
Summary:
This fixes T7995.
Previously, attempting to log out from your NylasID would just
automatically sign you back in with the same NylasID because the webview
session was preserved.
Now, instead of relaunching the windows, we restart the app to clear the
webview session
Test Plan: manual
Reviewers: halla, evan
Reviewed By: evan
Maniphest Tasks: T7995
Differential Revision: https://phab.nylas.com/D4224
Summary:
Previously, the logic for deciding wether to report an APIError lived
inside NylasAPIRequest and depended on an array of ignorable
statusCodes. However, when handling APIErrors, NylasAPIRequest would
convert special error codes (for offline errors) into a statusCode of 0,
which would automatically be ignored.
Given that the delta streaming connection didn't convert status codes to
0, it wouldn't actually ignore the error and end up reporting it,
flooding sentry.
This commit makes it so that that logic lives inside APIError, and
anyone who handles APIErrors can check wether to report them or not,
including the delta streaming connection
Test Plan: manual :(
Reviewers: spang, evan, halla
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4235
Summary:
Currently, when we auth an account for the first time in Nylas Mail (or
we blow away the database), the app is going to request transactions
since cursor `null` from the /delta/streaming endpoint and from the local-sync
delta observable, instead of requesting transactions since cursor `0`
This is due to a subtle bug with the use of default values when
destructuring an object. Our coded did the following:
```
const {cursor = 0} = this._state
```
Which at a glance seems correct. However, this will only work as
expected if `this._state` has the following shape:
```
{cursor: undefined}
```
And unfortunately, our `this._state` looked like this when authing an
account for the first time:
```
{cursor: null}
```
Which would make `cursor === null` instead of `0`.
This is because when using default values, null is considered an
intentional argument/value, as opposed to not passing any argument/value
(which will mean that the argument is undefined).
This was a regression introduced in d60a23c and 8bc2ec5
Test Plan: manual, will add regression test in upcoming diff
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4243
