Summary:
Before trying to sync a folder, check if we actually need to do so. This will prevent us from doing unnecessary work that slows down the sync loop (like performing SELECT commands)
We will perform a folder sync if any of the following are true
- The folder hasn't been completely synced
- There are new messages (using imap STATUS command)
- There are attribute changes indicated via highestmodseq (using imap STATUS command)
- If server doesn't support highestmodseq, it has passed enough time since we last ran an attribute scan on the folder.
Addresses T7513
Test Plan: manual
Reviewers: evan, halla, spang
Reviewed By: halla, spang
Differential Revision: https://phab.nylas.com/D3675
Summary:
Currently, our mail sync strategy of expanding UID ranges from UIDNEXT
backwards until a UID of 1 implicitly assumes that every UID corresponds to an
actual message. This assumption is incorrect, and results in several
significant bugs regarding sync status.
This patch fixes issue 1:
Since UIDs are persistent and, so long as the UIDVALIDITY is valid, ascend
monotonically upward, every time you move a message to a new folder you "lose"
UIDs lower down in the range. In my work Inbox, where I get a lot of mail,
archive all the time, and generally have only a small number of threads in the
mailbox, the smallest UID is over 100k. This means that, after all my inbox
messages are synced, the sync loop will continue attempting to download
nonexistent old messages in this mailbox for hundreds of sync iterations, and
will not mark the mailbox as fully synced until fetchmin reaches 1, regardless
of the fact that there are no actually messages being pulled down.
This patch needs a small associated patch to N1 to update how sync status is
calculated (coming soon).
The next patch in this series will deal with gaps in the UIDspace that slow
down syncing of a folder.
Test Plan: manual
Reviewers: halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3677
Summary:
We want to do this in order to prevent send tasks from blocking the sync loop given that they can take a very long time to run. This is especially true when sending emails with large attachments to multiple recipients.
There is no real way to make sending in these cases faster, but we can prevent it from blocking the sync loop at least, especially because sending is mostly I/O bound.
This is a bit messy actually, but should be fixed when we properly implement a sync scheduler
Also added a limit to the total size of attachments you can upload to try to prevent weird EPIPE errors when sending.
See: D3670.
Also moved and renamed stuff a little
Test Plan: manual
Reviewers: halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3669
Summary: Allows us to reset accounts in local-sync too
Test Plan: manual
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3672
Summary:
I happened to be testing between Jan 2017 and Dec 2016, so I
missed this logic flaw. Boo.
Test Plan: tested locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3667
Summary: We did this for gmail, but not for other providers.
Test Plan: tested locally
Reviewers: juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3665
Summary: While working on separating send out of the sync loop, I realized sync tasks could use some cleanup to be more consistent with how we implemented syncback tasks. I reorganized and renamed things a little bit. This will also help us move in the direction of the scheduler implementation under which everything is a task.
Test Plan: manual
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3660
Summary:
Only updated within month precision. We can use this to show how
far back a folder has been synced.
Test Plan: tested locally
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3662
Summary:
Fixes https://phab.nylas.com/T7435
The old deepScan (now `scanForAttributeChanges`) and shallowScan (now
`fetchLatestAttributeChanges`) had some fatal flaws.
If you deep scanned it would attempt to load the message attributes of all
messages ever and cause very bad memory leaks.
Also, if you left a mailbox running for a long time, there was a query
that would eventually run `Message.findAll` and, even though it was just
returning the headers, would still run insanely expensive operations
This fixes (and renames) these issues.
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3657
Summary:
This swaps out our generic IMAP threading mechanism to use the threading
headers on the message instead of the prior way of grouping by subject
and then differentiating based on participants, as that design was
somewhat driven by what we could accomplish easily given legacy data
schema decisions and has serious caveats, such as different threads between
the same people with the same subject being misthreaded together. With K2, we
have free reign to change the data format, so we can do it right.
The algorithm is super simple:
- Define "references" as the union of the Message-Id, In-Reply-To, and
References headers on a message, filtered for valid RFC2822 Message-IDs
- On message sync, if any element of the new message's references
matches any element of an existing message's references, thread them
together
In order to accomplish this, we need to store References in a way that
allows each element to be indexed for fast lookup. That meant either
using the sqlite JSON1 extension + expression-based indices, or creating
a new table. I chose the latter as a time-tested and simple solution,
since we don't need the flexibility of JSON here.
Test Plan: manual - unit tests coming
Reviewers: khamidou, evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3651
Summary:
Previously we would unconditionally issue a SELECT when openBox was
called. Now we check if the currently open box is the one we want first and
return immediately if it is, avoiding the unnecessary SELECT (which can be
quite expensive on large folders like INBOX). We were also calling closeBox
after iterating all the messages in a thread to mark them as read/unread.
This was unnecessary and was causing extra SELECTs to be issued. Now we don't!
This diff is a 5x speedup over the old behavior when marking lots of
threads in the same folder as read all at once.
Test Plan: Run locally, measure perf with log statements
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3654
Summary:
We were returning the wrong type in the case that we got no messages
back from the Gmail search API.
Test Plan: Run locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3646
Summary:
Add a new dedicated imap connection to listen for any updates or new mail on the inbox.
Previously, we wouldn't be able to receive new mail events on the inbox during the sync loop
because other mailboxes would be open while we sync them. This would cause big
delays in receiving new mail, especially if you have a lot of folders
Test Plan: manual
Reviewers: spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3650
There are plenty of valid use cases for sending subject-only emails,
and we also want to still download the headers and create message
objects if e.g. the email consists of only an event invitation or
an attachment.
Summary:
This commit passes down the `socketTimeout` option to node-imap. However, just
passing this option doesn't seem to work reliably, so this commit manually implements
the socketTimeout option for our IMAPConnection.
How it works is that basically every operation is wrapped with a timeout by
augmenting the `createConnectionPromise` construct that already existed.
Test Plan:
Locally, tested by sleeping computer and turning off wifi. The
connection will successfully error and be restarted. It will reconnect when the
network is available again
Reviewers: khamidou, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3642
Summary:
We may find it useful at some point to be able to tell which messages in
a user's mailbox were sent using N1/the REST API vs Nylas Mail.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3631
Summary:
Sometimes things go very wrong when trying to syncback an action. For example, the worker window could crash, preventing us from marking the current task as failed. What's worse, another sync iteration could try to run the task which crashed, thinking it a new one. To prevent this, this diff adds a fourth syncback task state, `INPROGRESS`.
New syncback tasks are marked as `INPROGRESS` before being executed. When they complete we mark them as `SUCCEEDED/FAILED`. Stray `INPROGRESS` tasks are automatically marked as `FAILED` at the beginning of every sync iteration, to make sure we don't retry them again.
Test Plan: Tested manually.
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3635
A bug allowed multiple sync loop processes to start. This could lead to
double sending and the sync loop appearing as thouogh it couldn't be
interrupted
Summary:
Well-behaved MUAs don't do this, but it is totally legal and we should
handle it.
The Python sync engine handles this properly also:
7db949fec9/sync-engine/inbox/util/addr.py (L26-L47)
(Found this out while scratching my head over the output format of
mimelib.parseHeaders---turns out it's an array for a reason:
> let to = mimelib.parseHeaders(`To: Christine Spang <spang@nylas.com>
... To: Foo Bar <foo@example.com>`);
undefined
> to
{ to:
[ 'Christine Spang <spang@nylas.com>',
'Foo Bar <foo@example.com>' ] }
)
Test Plan: unit test included
Reviewers: halla, jackie, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3625
Summary: Update `_getNewSyncbackTasks` to return any send tasks first, and then others
Test Plan: locally
Reviewers: halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D3627
Summary:
See title. Got rid of that syncback-worker class which was kind of useless and
made things harder. My b.
Test Plan: locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3624
Summary:
We have another function called extractContacts, in its own file,
extract-contacts.js, which is used to create Contact objects. This has
confused me a number of times and also leads to grep collisions.
This patch also makes the snippet unit tests pass again after a recent
API change.
Test Plan: included unit tests, manual
Reviewers: halla, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3623
Summary:
Message bodies, drafts aside, are immutable, and we set the snippet on
new messages manually in parseFromImap()---meaning this hook, if
invoked, is likely to replace the snippet with a broken version computed
with this old implementation. If we need a hook in the future (e.g. for
updating drafts), it should use the snippet function from
message-factory.
Test Plan: n/a
Reviewers: juan, halla
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3622
1. It could be expensive to delete many messages at the exact moment
when the folder is deleted
2. The folder delete could actually just be a rename, and if we
deleted all the messages, we would have to re-process them all
3. We already do a clean-up check for orphaned messages at the end
of the sync loop, where we already know if the folder was
actually deleted or just renamed
Summary:
Delete associated children when a parent is deleted to prevent foreign
key constraint errors. Also make sure any child hooks are run.
Test Plan: tested locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3620
Summary: N1 uses this to show the little attachment icon in the thread list.
Test Plan: tested locally
Reviewers: evan, juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3617
Summary:
This commit introduces interruptible sync operations. Now, the `SyncWorker`, `FetchFolderList` operation and `FetchMessagesInFolder` operation can be interrupted at several points during their execution. This improves the performance of SyncbackTasks, which now run almost immediately.
To achieve this, this commit adds an Interruptible abstraction, which is an object that can run functions and interrupt them at points marked by the function. For more info on how this works, see the docs on the Interruptible class.
This commit also splits up the SyncWorker a little bit to make it smaller, byadding a SyncbackTaskWorker.
Depends on D3613
Test Plan: Manual
Reviewers: spang, mark, jackie, khamidou, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3612
Since we weren't giving Node IMAP the proper format for the changedsince
flag, and since node IMAP never warned of improper schemas, we weren't
properly requesting the correct range. This would cause us to request the
metadata attributes of EVERY message in the mailbox and attempt to store
them in a hash. This would eventually lead to a memory leak and take down
the worker window, which caused other subtle issues like sends failing
when the worker window dropped task half way through their perform remotes
and never re-sent the deltas notifying of their success or failure.
This was only triggered when new highestmodseq numbers fired on the remote
server, which would be triggered by the underlying mailbox getting folders
or labels changed on messages
We separately filter out contacts without email addresses before
committing to the contacts table in the database for autocomplete (in
isContactMeaningful()), and if we filter out these already we can end up
excluding legitimate elements of the headers. For example, the Clutter
feature of Office 365 sends emails with a From: header like this:
From: Microsoft Outlook
Fixes: T7413
Summary:
Extract files for inline attachments and store their content id
Fixes T7414
Test Plan: tested locally
Reviewers: evan, spang
Reviewed By: spang
Maniphest Tasks: T7414
Differential Revision: https://phab.nylas.com/D3609
Summary:
Fixes T7398
We were create unnecessary and duplicate indices for the IDs of all of
our objects and increasing db write overhead.
We were not creating the correct reverse index for our join tables.
The search API'd db is already in scope of the accountId, this is an
unnecessary constraint on the query
Test Plan: manual
Reviewers: spang, juan
Reviewed By: juan
Maniphest Tasks: T7398
Differential Revision: https://phab.nylas.com/D3606
Summary:
Headers can be quite big, so we might as well download and store only
the ones that we care about. This patch also makes it so we stop
downloading MIME structures twice per message.
While it's possible that we _may_ want to make more headers accessible
later, we don't currently make the generic pile of headers accessible to
N1 or N1 plugins in any way, so doing that would end up requiring
changes to the sync code regardless. I think it's worth optimizing the
base experience rather than trying to predict what we may want in the
future. Plus, it seems more likely that we'll want to build future
extensibility using thread metadata, rather than message headers.
On inboxapptest1@fastmail.fm, this patch decreases the size of the
generated sqlite file for a fully synced mailbox by 35%.
Test Plan: manual
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3611
Summary:
It's possible to have multiple inline HTML parts in a message, or even
a multipart/alternative part that contains text and HTML, followed by a
plaintext signature. Previously, if there was more than one text part in
an email, we would pick the _last_ text/html or text/plain part that we
found, and treat that as the entire message body. This works most of the
time, but fails to display the full message body in some edge cases.
This patch fixes that by resolving multipart/alternative subparts to a
single part in the mimepart fetch stage, and then treating each desired
mime part separately when parsing the message, concatenating them if
there are multiple.
This makes K2's handling of multipart MIME message text better,
bug-wise, than the Python sync engine's, which has been mangling some
rare messages forever. (Example from my email: every email from the MIT
EECS Jobs List has never displayed the mailing list signature in N1.)
Note that this patch also removes our tentative support for PGP
encrypted messages. I'd rather add that back in later when I've dug up
some real example messages to test on, rather than leaving it in in its
current not-really-tested and probably not-really-working state, since
it makes it harder to make sure that the rest of the logic isn't broken.
Test Plan: manual for now - added examples of this to my growing list of regression tests to add to the message parser unit tests once I fix them
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3600
Summary:
We sync messages in the same order as the Python sync engine (newest to
oldest, generally), so we should be able to just use the same threading
algorithm. While we may still want to take into account References /
In-Reply-To at some point, this is a big step up from the current
thread-matching-only.
Test Plan: manual --- could pretty easily port the unit tests from the python codebase if we wanted
Reviewers: khamidou, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3604
This reverts commit 0b3e3d2f39.
Interrupting sync by closing connection causes errors downstream when
`syncNow` is called elsewhere. Instead of interrupting by closing the
connection, we will post a patch to interrupt the sync loop properly
Summary:
Sync operations are mostly bound by I/O and the imap connection.
What we believe that is mostly affecting cpu and battery life is that node’s event
loop is being hosed with cpu intensive message processing operations.
To alleviate this, we do a few things:
- Restore a global message processing queue to process messages serially and meter cpu usage (message processing continues to be a fire and forget call from within sync operations)
- Move actual cpu intensive work to the message processing queue, i.e. `MessageFactory.parseFromImap`
- Keep track of message processing queue length, and skip sync operations if queue is too big to prevent massive memory consumption
This commit also renames the package from new-message-processor to
message-processor, given that now it processes both new and existing
messages, and we like to minimize confusion.
Test Plan: manual
Reviewers: spang, khamidou, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3602
Summary:
- Ensure delete deltas make it through to N1
- Don't fail if we can't find a category that needs to be deleted
Test Plan: local
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3593
Summary:
- Adds `File` objects onto the `Message`s so N1 sees attachments
- Ensures `File` is eagerly loaded for all messages
- Base 64 streams attachments through the local /download endpoint
- ExtractFile only uses disposition type attachment when extracting
attachments
- Makes sure we save existing messages when processing them
Test Plan: manual :(
Reviewers: juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3595
Summary:
We were previously not taking into account the 'Content-Disposition'
MIME header, which differentiates between parts intended for display
('inline') and parts that are instead transferred files ('attachment').
See the RFC for more details:
https://www.ietf.org/rfc/rfc2183.txt
Fixes: T7367
Test Plan: unit test coming soon---have the test data and going to fix all message parsing test cases at once
Reviewers: juan, jackie, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3585
Summary:
Labels don't get added via passing in a labels attribute to
create(). We need to call addLabels() instead.
Test Plan: Tested locally
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3581
Summary:
refactor multi-send
This diff started off by fixing sending with attachments.
The issue is that our `FileUploadStore` listened for
`Actions.sendDraftSuccess` as its signal to remove the files from the
.nylas temp directory. Unfortunately, the old MultiSend tasks, after
delivery of the message, would try and put the base message in the sent
folder. Since we already deleted the file from our local temp dir,
creating the base message for the sent folder would fail.
This exposed a much bigger issue which is that we don't consistently
distinguish between "delivery" of a message and any post-processing we do
(like filling the sent folder). This was leading to a variety of other
subtle issues.
For example, N1 assumes that if the SendMessage task fails, then we pop
the draft back up and ask the user to try again. Unfortunately, since we
were combining "delivery" and "post processing" it was possible for the
message to actually deliver, but fail when stuffing the sent folder, or
fail due to some other random bug. This would cause the user to send the
message twice.
To help us ensure we never "deliver" twice and handle errors more
intuitively, I separated out the two concepts.
Now there are "send" set of tasks and endpoints, and a
"EnsureMessageInSentFolder" set of tasks and endpoint (the latter used to
be ambiguously known as ReconcileMultiSend, whatever that meant)
The logic for send hasn't changed. This is mostly a renaming and moving
files around.
Test Plan: manual :(
Reviewers: jackie, juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D3577
Contrary to what you might think, a message can have both an empty From: header
and multiple From: headers / multiple addresses in a From header. In that case,
we must save all of them and let the client decide how to display.
Fixes: T7370
Summary: I've found a pretty annoying bug --- N1 would stop syncing all accounts after the Internet connection dropped. It seems that deep inside node-imap or NodeJS itself, connections aren't timing out the right way. To work around this, this diff unilaterally restarts the sync every `nextSyncIn` milliseconds.
Test Plan: Tested manually by cutting internet access and checking that K2 recovered.
Reviewers: evan, juan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3573
The 'encoding' library transparently upgrades to using iconv instead of
iconv-lite, if available. This allows us to support more encodings in
emails, such as ISO-2022-JP.
Fixes: T7358
Summary:
If you sent an email to yourself it would not show up in your inbox. This
is because sent messages would never get a lastMessageReceived timestamp.
Since we order the inbox by lastMessageReceived, setting that to null to
on sent mail would mean it never shows up in the thread list.
Also fixed an assertion bug in SFDC that requires transactions to return a
promise.
Finally added extra debug interfaces that will show more info if the delta
stream detects an inconsistency
Test Plan: manual
Reviewers: juan, halla, jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3552
Summary: We were creating duplicate `Message` objects because the formatting for the date was different between `buildForSend` and `parseFromImap`. Now, we create the initial hash using the same format that `buildmail` uses to ensure that we generate the same IDs.
Test Plan: Tested locally.
Reviewers: evan, juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3559
Summary:
Because of the way we were attempting to parse contacts from
From/To/Cc/Bcc headers by converting them to JSON with a regex, we were
erroneously breaking contacts that contained commas in quoted names into
multiple contacts. This could result in things like parsing multiple
addresses for the From: header, incorrectly!
To resolve the problem, replace our homegrown logic with mimelib's
seemingly excellent parseAddresses(), which handles this and a myriad of
other cases correctly.
Fixes: T7370
Test Plan: unit tests included
Reviewers: mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3565
This doesn't do anything with sqlite, and just generates the following
warning in the logs:
>> WARNING: SQLite does not support TEXT with options. Plain `TEXT` will be used instead.
>> Check: https://www.sqlite.org/datatype3.html
Will fix these once I've finished up the current slew of bugfixes I'm
working on---kind of a pain to ensure they're passing in all
intermediate states.
Summary:
Similar to the fix in D3555, concurrent message processing may cause insert
races for folder and label thread associations. If the row is already present,
we can simply do nothing.
Test Plan: manual for now
Reviewers: jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3557
Summary:
When the Date: header is not present, use the INTERNALDATE from the IMAP server
instead.
Test Plan: manual for now - will add a regression test for this though
Reviewers: juan, jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3556
Summary:
Because of JavaScript's asynchronous nature, it is possible that we will
be processing several downloaded messages concurrently. This can lead to
calling extractContacts() in an interleaved fashion, which it was not
designed to handle. It looks up which contacts are already in the
database and then performs inserts or updates accordingly, assuming
nothing has changed in the contacts table in between---which is not
true! If several messages have similar contacts, an insert race can
cause one of the inserts to throw an unhandled exception.
We fix this by simply catching the unique constraint error, and falling
back to an update instead. (There's not really a better way to deal
with write races other than to enforce that we process contacts from
messages serially, as transactions are of no help here.)
This commit also removes extractContacts()'s return value, which is not
currently used and I found confusing.
Test Plan: manual
Reviewers: juan, evan, mark, jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3555
Summary:
This was leading us to put funny things like 'Nylas !' in some snippets that used
tags like <i> and <b> for text formatting. This is probs a teeny little bit slower
than the previous version since it invokes a callback on a lot more nodes, but we
can't really fix this issue without knowledge of the preceding tag name.
Test Plan: unit test included!!
Reviewers: evan, jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3553
Summary:
I have quite a few emails in my mailbox that have both multiple Reply-To
addresses. This is perfectly OK by the spec.
Fixes: T7369
Test Plan:
regression test coming - making a list and planning to update all the tests once I've hammered out the current crop of fixes I've identified
I also tested and made sure that N1 does the right thing in this case...
multiple Reply-To addresses are displayed correctly, and when you hit "Reply" a
new draft is started with both in the To: field. Makes sense given this is
something the Python sync engine supported too.
Reviewers: jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3558
Using node-imap's parseHeader function to parse headers was resulting in
a huge number of message parse failures on Office365 accounts, because
the results contained unicode control character 9 and we'd then feed that
string to JSON.parse when extracting contacts, which would throw an
exception.
Using mimelib's header parsing function eliminates these errors.
Summary:
This replaces the API delta stream with a direct in-memory one
Addresses T7300
Test Plan: manual
Reviewers: jackie, halla, juan
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3548
Summary:
Associated N1 Diff: D3545
This commit ensures that authh notifications are showed when the
underlying sync worker fails and are cleared when an account is
successfully reconnected
To achieve this, we manually keep track and update syncStates where
appropriate via `Actions.updateAccount`, given that we have access to
N1's version of the account directly from local-sync.
Initially I was considering account delta stream to the cloud-api and the local-api, but that
just complicated things more than it helped.
This commit also fixes a bug with refreshing the gmail token in which we
we were only attempting a token refresh upon restarting the app
This addresses: T7346, T7305, T7335
Test Plan: Manual
Reviewers: halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3544
Summary:
We were seeing JS blocking in snippet extraction of up to 2k ms. This
is because we were walking the entire DOM of a message and extracting
all text, regardless of message size---and using our own homegrown
DOM walker function.
To remedy this, use the standard TreeWalker from the Chrome browser
APIs (which in benchmarks looks 2-4x faster) and also exit out of
the DOM walking process once we've accumulated enough text to create
a snippet. Informal eyeballing of timing metrics for this function suggests
the new implementation is something like 10-100x faster for some messages.
As a bonus, we get to delete some code and end up with a cleaner
implementation!
Test Plan: old unit tests yaay
Reviewers: juan
Reviewed By: juan
Subscribers: evan
Differential Revision: https://phab.nylas.com/D3543
Summary:
Save metadata correctly by reassigning an object to value.
Since account IDs are different between N1 and N1 Cloud, use just the message ID, which should be unique.
Test Plan: Tested locally.
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3524
Summary:
This is a refactor of the auth APIs to use async/await. Gmail Auth is
pretty confusing and I wanted to make it cleaner to read and easier to
use. This is also part of the general API upgrade to modern ES6
This also fixes the Gmail auth error we saw at showcase
Test Plan: manual
Reviewers: halla, jackie, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3535
Summary:
See https://github.com/mscdex/node-imap/issues/585 for details.
This issue was causing us constantly run the sync loop without pauses,
i.e. every next sync loop was scheduled immediately.
Currently, when we receive a new `'mail'`event, we trigger a new sync loop. Previously, when this happened while a sync loop was already in progress we would just ignore the event. However, my recent patch keeps track of how many times we tried to start a sync loop while one was already in progress. If the number of times this happens is > 0, it will schedule the next sync loop immediately (as opposed to waiting a constant amount seconds before the next loop).
The problem is that this new logic is making the sync worker always schedule the next sync loop immediately (without pausing for a few seconds). This is due to the following chain of events (assume we are just syncing `all` and `trash` folders):
This commit is a temporary workaround to this problem.
Test Plan: manual
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3537
Summary:
Associated N1 Diff: D3530
This commit converts multi-send from a 3 step process into a 2 step
process
The first step creates the base message and sends a message per
recipient, each with its customized message body for tracking.
The second step reconciles all sent messages, specifically removing any
sent messages created by gmail, and saving the correct message to the
sent folder
This commit also ensures that we run the send tasks immediately by
ensuring we restart the sync loop if its already running
Test Plan: Manual
Reviewers: evan, jackie, halla
Reviewed By: jackie, halla
Differential Revision: https://phab.nylas.com/D3529
Summary:
This is a small patch but it's pretty complex, because of the numbers of moving parts. Gmail has two types of tokens, access and refresh tokens. Access tokens have a limited shelf life of one hour. After that they expire and you need to use your refresh token to get a new one.
We've decided to do the access token generation on the server, because we don't feel comfortable giving our users both our Google client id and secret. To do that, I've added an endpoint, `/gmail/auth/refresh` which returns a valid access token as well as an expiration date for the token.
The only place where we handle token expiration is in the sync workers. Before trying opening a new connection we check if our access token is expired. If yes, we get a new one from the API. If there's an issue doing this, we notify N1 using `NylasAPIHelpers.handleAuthenticationFailure`.
There's a second patch for N1 with tiny related fixes.
Test Plan: Tested manually. Will need to test more in the real world.
Reviewers: evan, jackie, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3522
Summary:
- Don't fail if there's no subject, just set it to `(no subject)`
- Support "BINARY" content-transfer-encoding. (This really means
that there is no encoding, so it's simple to add support for it)
Test Plan: tested locally
Reviewers: jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3528
Summary:
Add internal state to the sync worker to allow for it to be interrupted
and restarted.
The concept in this commit is that if we've tried to trigger a sync enough times
while its already in progress, bail and start over.
Usually, we manually trigger sync loops when we queue a new SyncbackTasks,
so that the newly queued task gets executed. This is necessary because the only
way to run SyncbackTasks is via the sync loop, for consistency and simplicity
reasons.
For example, we might run into a case where we queue a SendMessage task,
and we want it to be executed ASAP, but if we're in the middle of a
syncing a mailbox with a ton of folders, we wont get to the SendMessage
task after some considerable time.
Specifically this commit makes it so:
- If the number of sync attempts while in progress is > 0, make sure we schedule the next sync immediately
- If we reach a threshold of sync attempts while in progress, interrupt sync and restart
Test Plan: todo :(
Reviewers: mark, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3520
Summary:
We were doing all of this fancy filtering and sorting to determine which
tasks to run, but in the end, we were just running the whole unsorted
list (using the wrong variable)
Also extracted getting the list of tasks into its own function, for
easier unit tests (it should really have some)
And, also wanted to make sure other people looked at this code, since I
believe no one has before.
Test Plan: todo
Reviewers: mark, khamidou, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3516
Summary: See description at T7327
Test Plan: Manual, but this should have unit tests
Reviewers: mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3513
Summary:
Move sendmail-client and errors to isomorphic-core, given that they will
probably be used by cloud-workers (plugin backends) and cloud-api
Depends on D3510
Test Plan: Manual
Reviewers: halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3512
Summary:
Associated N1 diff: D3511
Convert send endpoints to use syncback tasks for consistency with how we
perform other imap operations, but primarily:
- So that it triggers a sync loop immediately and we pick up changes quickly
- To keep track of various send operations as a single unit (e.g. sending + saving to sent folder or deleting from sent)
This commit also fixes SyncbackRequest error handling and processing in
N1-- previously we were saving error fields to the syncbackRequests with
a format that didn't match N1's API error and which wasn't properly
serializable. (Also rename HTTPError to APIError)
Test Plan: Todo/Manual
Reviewers: jackie, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3510
Summary:
I've found this useful for generating test cases and am tired
of adding and removing this code!
Test Plan: inspect output of /tmp/k2-parse-output
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3518
Summary: This was understandably causing some messages to fail to display correctly.
Test Plan: unit tests are already broken for message parsing -- will fix in follow up diff
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3517
Summary:
This diff solves a few separate issues from T7313, T7316, T7282, and it refactors
the send code a little bit.
Initially, the problem that led to this diff was generating message ids that
wouldn't collide (which was causing errors in the message-processor). Collisions
in ids were being caused by messages that contained the exact same participants,
subject and date (most likely due bots or scripts sending emails in quick
succession)
To prevent collisions this commit adds the `message-id` header as part of the
database message id, and ensures that we set it correctly before sending, and
that it remains consistent through send, multi-send, and the sync loop.
During the refactor and review, I removed some code that assumed that we were
syncing drafts (which we aren't), and also fixes a few other known and
unknown issues around sending, message creation, and tracking, like assigning
the correct date header (we were previously assigning the draft creation date
from within N1), fixing the tracking regex, among other smaller bugs/typos.
Will address inline TODOs in a separate diff
Test Plan: TODO!!! I will add tests in another diff
Reviewers: evan, halla, jackie, khamidou
Reviewed By: halla, jackie
Differential Revision: https://phab.nylas.com/D3507
Summary:
We forgot to refactor `publicId` to just `id`, which was breaking contact
deltas in N1.
Test Plan: Run locally
Reviewers: khamidou, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3508
Summary: See title
Test Plan: Run locally
Reviewers: juan, evan
Reviewed By: juan, evan
Maniphest Tasks: T7281
Differential Revision: https://phab.nylas.com/D3498
Summary:
Allow other fields to be passed in with participants, so that Contact
objects can be passed in as JSON. Also fix an error check.
Test Plan: tested locally
Reviewers: jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3504
Summary:
This fixes multiple issues, including snippets telling you you
ought to look at the HTML as well as cruft like HTML entities
and CSS in snippets.
Test Plan: unit tests included o.O
Reviewers: juan
Reviewed By: juan
Subscribers: evan
Differential Revision: https://phab.nylas.com/D3500
Summary: See title
Test Plan: Ran it locally
Reviewers: khamidou, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3496
Summary:
1) Send the custom body, rather than the generic body
2) Extract contacts correctly so that the saved sent message has all the
participants, rather than just the last one
Test Plan: Tested locally
Reviewers: jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3499
Summary:
This commit fixes the following bugs in message parsing:
- we were unilaterally decoding MIME bodies as UTF-8; instead, decode according
to the charset data in the mimepart header
- '7bit' content-transfer-encoding means us-ascii, NOT utf-7
- only interpret valid content-transfer-encodings (previously we were trying
to treat various charsets as transfer-encodings)
- clearer naming: s/values/parsedMessage/
- unify snippet cleanup between plaintext & stripped HTML (merging
whitespace etc.)
Test Plan: units tests coming
Reviewers: juan
Differential Revision: https://phab.nylas.com/D3491
We were getting sql unique constraint violation errors for ids because
we were attempting to create objects with the same id within the same
transaction.
This commit ensures that only attempt to write a contact with the same
id once, and that we check for all exsiting contacts before hand.
For file, we ensure that we don't attempt to write 2 files with the same
id more than once
We were using the stripped version of label/folder names for the id
hash, e.g. [Gmail]/Drafts would be Drafts.
However, we can't do this because it might collide with other names. e.g. if the
user created a Drafts label, it would end up colliding with [Gmail]/Drafts
Minor lint fix
Summary:
This commit also fixes snippets for HTML-only messages to strip out HTML
tags, and makes us preserve whitespace for plaintext emails by
displaying them in <pre class="nylas-plaintext"> tags, and makes us log
messages that fail to parse at all to a tempdir.
The only issue I found with using <pre> tags for plaintext email was
that some lines may trigger scrolling, so there is an associated commit
(D3484) that changes the CSS for <pre class="nylas-plaintext"> to wrap
lines.
In the future, we can add regression tests to this test suite whenever
we fix parsing bugs.
Test Plan: unit tests included
Reviewers: bengotow
Reviewed By: bengotow
Differential Revision: https://phab.nylas.com/D3483
