Warn user when note sync fails

[Usiel]

While experimenting with Simplenote and a new editor, I was testing performance with very large notes. I noticed that Simperium actually stops syncing notes of a certain size, but the user is never notified. This PR introduces an error indicator for 413 (doc too large) and other errors.

This PR is better with #3385, since this PR alone only fixes the error signalling in an active session. When the user restarts Simplenote with an usnyncable note, then the error handling isn't great without #3385.

Fix

When note failed to sync we see the warning triangle:

When user changes the note (e.g. shortens it), the pending spinner shows until we either succeed or again get an error:

Test

Get a sufficiently large random note:

head -c 1M /dev/urandom | pbcopy

• Add a new note and paste in the random note text.
• Wait for sync to fail
• Expected: Warning triangle shows
• Make a change to the note
• Expected: Pending spinner shows (triangle stays) and disappears when sync fails again
• Remove most of the text to make it sufficiently small
• Expected: Pending spinner comes up and once the sync completes the warning indicator disappears

Release

Enhancements:

• Warn user when note sync fails

[dmsnell]

@Usiel this is addressing a real problem, bravo 👏

still, I think there is plenty of room to think through the full flow of known-resectable updates and I think this PR can use a lot of work before it would be ready to merge.

syncing effortlessly is really hard though! it’s good to be paced and careful.

[dmsnell]

would be great if these tests were based on accessibility primitives instead of class names. I think the app overall suffers in this way, but just a prompt that we could be working to build new code stronger

[Usiel]

Thanks, makes sense. Should be addressed with commit 92c4018

[dmsnell]

what is the purpose of the rename here? it seems like we’re moving from a semantically-descriptive label “there are pending changes to the note for which the app has received no confirmation from the server that it has accepted them” and towards a less-clear stateful indicator: “show sync spinner, for who knows why”

[Usiel]

Hmm this is a result of the fact that the UI is now slightly more complex. Previously, we showed the spinner whenever we had pending changes (hasPendingChanges), which doesn't necessarily imply we are actively trying to sync (debounce, offline, indexing). With the changes I intended the following:

• Keep the current behavior when there's no error
• When there is an error: Only show the spinner when we are actually in the process of syncing. This indicates to the user that their actions result in a retry. Just showing the spinner based on hasPendingChanges in a (server) error state doesn't help the user because syncErrorCode != null => hasPendingChanges.

But this does introduce inconsistency, because the behavior of the spinner changes based on whether we received a server error or not. I did consider something like the following to make the UI perfectly consistent with the underlying states, but the UI feels somewhat janky as a user:

State	UI
Pending changes, online, not syncing	Non-animated spinner
Pending changes, online, syncing	Animated spinner
Pending changes, offline, not syncing	Non-animated greyed out spinner
Pending changes, offline, syncing	Non-animated greyed out spinner

(The error indicator shows based on syncErrorCode != null, no dependencies on the other states, hence I didn't include it in the table.)

---

So now I'm back at the following: When there's no error we immediately show the spinner on pending changes (observing offline restrictions ofc). When there is an error we only show the spinner when actually syncing.

I did refactor a bit though, which includes the renaming back to hasPendingChanges and fixes for the awkwardness of the retry state (replaced by syncing state).

Commit: 82038d6

[dmsnell]

doesn't necessarily imply we are actively trying to sync

the model of the app is that it’s always syncing, so there’s no need to make that explicit. obviously when the server has actively rejected an update we should stop trying to resubmit that update, but…the spinner is not about syncing.

the spinner is there only to communicate to the user that there are changes which may are potentially not synchronized with the server. this is a clue that if they close the app there’s no guarantee their updates will appear on other devices, and that if they leave the web app they might lose them.

the server-status provides more insight into whether there is a known-severed connection and syncing is actively paused.

the "pending changes" terminology is at least, from a technical standpoint, carefully chosen and independent of syncing terminology, because it doesn’t matter if the app is syncing, failing to sync, offline, or whatever — the changes are not verified by the server.

---

we could have some room to improve this messaging, and I like the ideas you are bringing here. something seems off still though with the complexity and shift towards making users think about these technical details.

there’s no guarantee that submitting a rejected update will fail again if it fails once. this makes me wonder if a UI hint without actionable information or with less actionable information could be useful.

for instance, what if, once we receive an active rejection, we turned the spinner red and updates its accessibility attributes accordingly? hovering over the spinner shows that we received an error from the server specific to that note update, and that Simplenote may not be able to sync the note in its current form.

this perspective follows the idiom of Simplenote is always syncing, adds a little more insight into why syncing may seem to be taking too long, and can fade back to normal if and when it does.

---

when exiting the web app with pending changes, the app should provide the option to download copies of the notes with pending changes.

there is plenty of room in the top toolbar, especially after all of the convenient action buttons were hidden behind more clicks. we could explore adding an indicator when notes have seen rejections and this could be a place to persist those warnings so they don’t disappear too quickly.

hovering over it could reveal a list of notes that saw rejections, a description of the rejection, and whether it was resolved. in fact, we could explain resolutions like “was resolved after 10 minutes through a remote update“ and “was resolved immediately after a sync retry” and “was resolved after 6 minutes by changing the contents”

[Usiel]

the spinner is there only to communicate to the user that there are changes which may are potentially not synchronized with the server. this is a clue that if they close the app there’s no guarantee their updates will appear on other devices, and that if they leave the web app they might lose them.

Maybe that's where the disconnect comes from. To me, the spinning arrows imply something's actively happening right now. If it doesn't disappear within a few seconds and just keeps on spinning, then I feel like something is broken as a user. That is why I like the offline indicator (non-spinning, greyed out) works for me. A spinning spinner showing when there is an error feels deceptive: nothing is happening, and nothing will happen with the current way Simplenote works.

---

for instance, what if, once we receive an active rejection, we turned the spinner red and updates its accessibility attributes accordingly? hovering over the spinner shows that we received an error from the server specific to that note update, and that Simplenote may not be able to sync the note in its current form.

I like this! Just tried it out and it feels quite natural for the user (plus it removes the extra error icon). But I do prefer it with a non-spinning spinner on error, which then again requires the sync state.

I have 2 variants right now, here are the main states for each:

• usielriedl/add-warning-for-large-notes-variant-i:
  • pending, online, no error: spinning spinner
  • pending, offline, no error: non-spinning spinner (grey)
  • pending, *, error:
    • !syncing: non-spinning spinner (red)
    • syncing: spinning spinner
• usielriedl/add-warning-for-large-notes-variant-ii:
  • pending, online, no error: spinning spinner
  • pending, offline, no error: non-spinning spinner (grey)
  • pending, *, error: spinning spinner

variant-i handles my anxiety of spinning spinners, while variant-ii follows the "always-syncing" idiom you mentioned to the letter.

---

when exiting the web app with pending changes, the app should provide the option to download copies of the notes with pending changes.

These changes sound good to me. I would also add that we can improve a lot in general with regards to errors, including intelligent retries depending on the exact error scenarios. I think shipping this PR's improvement (notify users of errors, allows users to handle 413 errors without blocking queue) does not need to depend on additional work.

[dmsnell]

the spinning arrows imply something's actively happening right now

But I do prefer it with a non-spinning spinner on error, which then again requires the sync state.

if we want to do this then we have to be really careful about confusing things more. as long as the local copy differs from the ghost, Simplenote should be actively trying to sync, meaning that something is happening right now.

it’s not even as simple as associating a sync error with the contents of a note, because remote updates could come in which, when rebasing local changes, could theoretically open up our new copy for storage.

this is the fundamental problem of interrupting the normal sync mechanisms with special-casing. the general case is self-healing, albeit with some delays and confusing UI. the special-cased system blocks syncing and can lead to data loss.

it’s actually not terrible that a customer contacts support when a note is stuck because that gives us insight into problems we might be able to fix.

for this reason I like the messaging-only approach: the server actively reported a rejection reasons and the note might not sync (thought it still might be able to). that gives someone something to try without giving them false hope or expectations.

it also seems that right now we’re mostly dealing with the note (plus its history) being too large? only dealing with that? In these cases I would expect a high rate of customer understanding. interacting with the note will likely slow down, the note will be visually huge, and a basic message should be readily understandable.

---

to this end, I still think it’s valuable to retain a message for some amount of time even when a sync error is resolved. that almost suggests that we don’t have a syncError but rather a noteSyncStatus which can hold multiple values. In fact, I think we already have something like this in there, don’t we? a last-synced time?

• successfulSync(timestamp, resolvedErrors=[])
• rejectedSync(timestampOfFirstRejection, reason)

or maybe this is just a combination of what you have with the lastest-sync-time info.

const rejection = state.syncRejections.get(noteId);
const lastSync  = state.lastSyncTimes.get(noteId);

const unresolvedRejection = rejection && (!lastSync || rejection.timestamp > lastSync.timestamp);
const resolvedRejection   = rejection && lasteSync?.timestamp > rejection.timestamp;

either way we can show the spinner as it currently operates, it’s “red” if we have an unresolved rejection, and on hover, it will show the rejection message if it’s red and show that a rejection was resolved if it’s not.

[Usiel]

if we want to do this then we have to be really careful about confusing things more. as long as the local copy differs from the ghost, Simplenote should be actively trying to sync, meaning that something is happening right now.

I don't think this is the case right now. When sync fails due to anything not handled in node-simperium's channel module, a error is emitted, but the queue will be stuck (e.g. 413 or even something possibly transient like 502). Any user changes are stuck behind the failed sent. On a WS reconnect node-simperium would retry in a sense, but errors like 413 will just end up jamming up the queue again.

Based on this review I decided to remove the hacky "unstuck-queue" bit and will try to instead contribute a fix to node-simperium instead. So that complexity is gone for now - at least from this PR (it still needs to be solved imo).

---

Here's a brief recap of the current different states, what I want to show the user any why:

Pending, online, no error

The user sees a spinning spinner, assuring them that the note is being synchronized. That may actually not really be the case due to the fact that we debounce changes. The user doesn't really need to know about that though: we spin the spinner regardless of whether data is transferred or not.

Pending, offline, no error

The spinner is disabled (grey) and does not spin.

Pending with error

As mentioned above, Simplenote doesn't really have guaranteed retries, hence we do not want to pretend that it does. When we reach the error state we show a red spinner (non-spinning). We only spin the spinner when data is actually being sent. This gives immediate feedback to the user, so they are aware when their new changes are being synced.

---

Here are some users that also (like me) seem to misunderstand the spinning spinner as a signal that something's happening:

I’m seeing spinning arrows on the notes that I’ve changed on the PC, clearly indicating that it’s trying to sync to the cloud but failing.

It does sound like there might’ve been a stuck sync state that cleared once a new change was made

after login there’s no data lost, but the arrow is still spinning.

https://forums.simplenote.com/forums/topic/cant-sync-changes-from-pc-just-spinning-arrows/

[dmsnell]

if we have an issue with node-simperium that leads to a loss of all syncing activity on error, that indeed would be a great place to address the problem. that is, if node-simperium is not doing what it is supposed to be doing (retrying after failure) then it’s good to make sure it does that. thank you for tackling that.

misunderstand the spinning spinner as a signal that something's happening:

this goes back to the idea that there could be a bug in node-simperium. it is conceptually doing things as long as the spinner is spinning. showing recent errors is a great way to help, but I think we should just be careful to communicate that nothing is happening because the error itself does not mean that there is no attempt to sync (at least, if attempts to sync stop after an error, that seems like a bug in the underlying library, hopefully an obvious mistake or failed error-handling of some sort).

---

@Usiel thank you for staying diligent on this. concurrent systems are really complicated, especially ones like Simplenote that have ironed out a number of nuance edges over the years. being patience and intentional here about communication will pay off though!

[Usiel]

Ok that makes sense. I think we were sometimes talking past each other because we had different assumptions about what node-simperium does on error. I'll tackle the proper fix in node-simperium when I have a bit less on my plate (next week 🤞). Until then I'll let this PR sit. Thank you for the reviews and discussion so far!

Ideally, I would hope node-simperium had the following behavior:

• On transient errors, retry the update
• On unrecoverable errors, emit an error, release the stuck in-flight change, and not block the queue for further updates on the same object

[dmsnell]

on a similar note, it would be semantically communicative if this didn’t indicate a stateful UI operation, but rather communicated declarative semantics about the state of the note.

syncRejectionReason: state.simperium.syncErrors.get(noteId) ?? null,

syncFailureCause: state.simperium.syncErrors.get(noteId) ?? null,

syncRejection: state.simperium.syncErrors.get(noteId) ?? null,

trying to stew on names that differentiate between expected sync failures (such as network failure or being offline) and affirmatively rejected syncs that won’t succeed on retry.

[Usiel]

As of now, this state really just represents errors we receive from the server, so to me syncErrorCode is pretty clear, as in "this is the error code we got while syncing for this note".

[dmsnell]

it probably seems pretty clear at a glance, but what if the note contents have changed since we received the sync error code, or if we received a remote update to the note? the sync error code was for a version of the note that no longer exists and the error code is out of date, unless we expect it to flash and disappear in these cases.

if we do expect it to automatically flash and disappear, that feels different than a pending-changes indicator because we kind of expected changes to enter a transient state of pending, but it would give me anxiety if I saw an error message appear and then disappear before I get a chance to read it.

[dmsnell]

can you explain this more? I expect that if someone updates the note, for example, by splitting it, then the local queue will be updated with the newer copy of the smaller note.

I guess if we are trying to avoid re-sending the note as-is then yes we want to remove it from the queue, but even at that point, as someone tries to repair the note won’t it immediately be re-added to the queue?

I wonder if there’s a way to reframe this less as performing imperative queue operations (which are so prone to concurrency issues) and more as logic determining whether to send the update. e.g. if the note contents are identical to the previous attempt and that attempt failed, then avoid sending the note.

given the possible ways the server rejects a note, we could theoretically spearhead some of this by enforcing constraints in the client, such as note size. unfortunately for note size, it will not be possible to detect all cases the server might reject based on size because that’s a complicated formula based on the sum of the compressed data for all note revisions.

[Usiel]

given the possible ways the server rejects a note, we could theoretically spearhead some of this by enforcing constraints in the client, such as note size. unfortunately for note size, it will not be possible to detect all cases the server might reject based on size because that’s a complicated formula based on the sum of the compressed data for all note revisions.

True, I don't think we can do this in a manner that doesn't yield false positives.

---

I simplified this in ead0fa2, but the meddling with the underlying queue stands unfortunately and I don't see another way to do this with Simperium. Ideally, Simperium would allow us to auto-eject changes that it deems unrecoverable.

For now, I also added ac4173e to reduce the blast radius. The bucket meddling is only executed for 413 errors, everything else will follow Simperium's normal flow.

[dmsnell]

True, I don't think we can do this in a manner that doesn't yield false positives.

It’s the other way around, no? we can definitely catch some cases, but with a high rate of false negatives. In other words, we would not be able to reject in the client the majority of cases that the server rejects, though we could compress the note contents and know for sure that the resultant document would be rejected if it’s greater than 16 MB or so. I think I said 1 MB before but it could have also become 16 MB at some point based on the backend.

It’s not a thing I recommend doing anyway, so this is moot; I just wanted to point out that in the extreme cases (e.g. for those writing books in Simplenote) if the content is big enough we could add messaging.

[Usiel]

It’s the other way around, no? we can definitely catch some cases, but with a high rate of false negatives. In other words, we would not be able to reject in the client the majority of cases that the server rejects, though we could compress the note contents and know for sure that the resultant document would be rejected if it’s greater than 16 MB or so. I think I said 1 MB before but it could have also become 16 MB at some point based on the backend.

Depends on how you define positive 😆 But yes, defined as "notes that are too large," you’re right: false negatives. Thanks for writing it out in other words.

[dmsnell]

the NB here is a bit confusing to me. is this a timeless comment or is it describing a particular journey in finding the way to add the error here?

coming into the code I would have expected to listen on the bucket, not on the channel, so I am confused why the note is mentioning that as an oddity.

[Usiel]

Sorry that was an artefact from debugging I should have seen on review. Addressed in ead0fa2

[dmsnell]

why are we passing in state if it’s available available through the closure?

[Usiel]

Yep, should be covered with the fixes for #3387 (comment)

[dmsnell]

isn’t this doing the opposite of what we want? if there’s a rejection, trying again when we know it will fail?

this is a challenge I expect with the fail/retry stateful perspective. effectively, Simplenote is always retrying by nature. once we start adding intermediate stages to this flow we complicate that nature and invite bugs.

for example, what if we receive an update from the server which shrinks the note substantially? what is the appropriate UI/UX flow for that, and how would it impact these error messages and retry mechanics? what should the new store state be for those affected notes?

[Usiel]

Yeah, thanks I removed that whole bit. This was overcomplicated. What we really care about is just "is the note currently syncing?" for UI reasons explained in #3387 (comment)

[dmsnell]

these are fairly-specific tests verifying that the basic reducer code does what it does. they aren’t particularly testing any behaviors in the app, meaning we have a high risk of passing tests when bugs appear and failing tests when legitimate and valid changes are made.

I wonder if they could be focused more around intentions and behaviors to ensure that what happens on sync leads to the expected UI

[Usiel]

Hmm I don't know, for me these are valid unit tests, they assert our expected states for different [action] inputs. I agree they do not guarantee a bug free state handling in the whole app (the state may be correctly updated, but the UI or some layer below fails to represent the state correctly), but I think that would be part of a more general E2E test suite, which would be great to have, but maybe not in this PR.

[dmsnell]

yeah they are valid; just wondering if they are useful or valuable. this one in particular, and some of the others, are manufacturing app state in a way that’s disjoint from data flow in the app and then asserting that the data is the way it was manufactured.

overall I can’t remember what the state of the app’s tests are, but I want to at least raise the question on new tests given that there’s very little work going on right now to the app, and if people do have to fight broken tests when making legitimate changes, that would be unfortunate.

so I will leave it at this and let you pick, but I always want to know:

a. does it seem likely this test is likely to catch a real defect?
b. is this test likely to reject an intentional and non-breaking change?

I want the first and don’t want the second

[Usiel]

a. does it seem likely this test is likely to catch a real defect?
b. is this test likely to reject an intentional and non-breaking change?

I want the first and don’t want the second

I agree, that is a good goal. My thinking here is that most of these tests cover the contract actions -> state and may break if someone inadvertently changes the implementation without intending to change the contract. A few of the tests may seem trivial right now, but I like them as a reminder to anyone changing the respective contract down the line: "If you change the contract, fix the tests and therefore document the contract in code" (of course, they may just decide to delete the test 😆)

[dmsnell]

is this removing the sync error on every Redux action past the initial sync error?

[Usiel]

We gate this based on the incoming action. We only clear this when the error becomes irrelevant (e.g. successful sync).

Renamed the helper and added inline comments in 4262f87 to make this more readable

[dmsnell]

I guess that the resolvedSyncNoteId() call was confusing me.

but now that I examine this, I see one more place where we’re potentially confusing the sync logic by assuming that a sync error will disappear based on a remote update.

what happened when you tried this with a sync error while receiving a remote update? what did you see in the UI?

[Usiel]

what happened when you tried this with a sync error while receiving a remote update? what did you see in the UI?

We switch from the error indicator to pending indicator. Unfortunately, there are 2 cases here, one of which is not great and I'd consider that an existing bug of sorts:

• Remote update applies correctly -> Pending indicator disappears
• Remote update does not apply -> We fail silently and retry syncing our local changes (which, in the case of my test with a 413 doc fails again)

[dmsnell]

the second case seems better. it’s the first case I’m worried about due to the potential for a flash of an error state that leaves someone anxious that they did something wrong.