Warn user when note sync fails

lib/note-list/note-cell.test.tsx

@@ -0,0 +1,148 @@
+import React from 'react';
+import { render } from '@testing-library/react';
+
+import { NoteCell } from './note-cell';
+import { getSyncErrorMessage } from '../utils/sync-error-message';
+import type * as T from '../types';
+
+const noteId = 'note-1' as T.EntityId;
+
+const minimalNote: T.Note = {
+  content: 'Hello world',
+  creationDate: 0,
+  deleted: false,
+  modificationDate: 0,
+  systemTags: [],
+  tags: [],
+};
+
+const baseProps = {
+  displayMode: 'comfy' as T.ListDisplayMode,
+  invalidateHeight: jest.fn(),
+  isOffline: false,
+  isOpened: false,
+  isSyncing: false,
+  lastUpdated: -Infinity,
+  note: minimalNote,
+  noteId,
+  openNote: jest.fn(),
+  pinNote: jest.fn(),
+  searchQuery: '',
+  hasPendingChanges: false,
+  style: {},
+  syncErrorCode: null as number | null,
+};
+
+const expectSpinning = (icon: HTMLElement, spinning: boolean) => {
+  expect(icon.classList.contains('is-syncing')).toBe(spinning);
+};
+
+const expectOfflineStyle = (icon: HTMLElement, offline: boolean) => {
+  expect(icon.classList.contains('is-offline')).toBe(offline);
+};
+
+const expectErrorStyle = (icon: HTMLElement, errored: boolean) => {
+  expect(icon.classList.contains('has-sync-error')).toBe(errored);
+};
+
+describe('NoteCell status icons', () => {
+  describe('pending sync icon appearance', () => {
+    it('spinning spinner shows when pending changes regardless of syncing state', () => {
+      const { getByRole } = render(
+        <NoteCell {...baseProps} hasPendingChanges />
+      );
+      const icon = getByRole('img', { name: 'Pending changes' });
+
+      expectSpinning(icon, true);
+      expectOfflineStyle(icon, false);
+      expectErrorStyle(icon, false);
+    });
+
+    it('non-spinning spinner shows when pending changes and offline', () => {
+      const { getByRole } = render(
+        <NoteCell {...baseProps} hasPendingChanges isOffline />
+      );
+      const icon = getByRole('img', {
+        name: 'Pending changes (waiting for network connection)',
+      });
+
+      expectSpinning(icon, false);
+      expectOfflineStyle(icon, true);
+      expectErrorStyle(icon, false);
+    });
+
+    it('non-spinning spinner shows when we have a sync error and we are not actively syncing', () => {
+      const { getByRole, queryByRole } = render(
+        <NoteCell {...baseProps} hasPendingChanges syncErrorCode={413} />
+      );
+      const icon = getByRole('img', {
+        name: 'Sync failed',
+        description: getSyncErrorMessage(413),
+      });
+
+      expectSpinning(icon, false);
+      expectErrorStyle(icon, true);
+      expect(queryByRole('img', { name: 'Pending changes' })).toBeNull();
+    });
+
+    it('spinning spinner shows when we are actively syncing and have a sync error', () => {
+      const { getByRole, queryByRole } = render(
+        <NoteCell
+          {...baseProps}
+          hasPendingChanges
+          isSyncing
+          syncErrorCode={413}
+        />
+      );
+      const icon = getByRole('img', { name: 'Pending changes' });
+
+      expectSpinning(icon, true);
+      expectErrorStyle(icon, false);
+      expect(queryByRole('img', { name: 'Sync failed' })).toBeNull();
+    });
+  });
+
+  describe('when the note is not pending', () => {
+    it('does not render a sync icon', () => {
+      const { queryByRole } = render(<NoteCell {...baseProps} />);
+
+      expect(queryByRole('img', { name: 'Sync failed' })).toBeNull();
+      expect(queryByRole('img', { name: 'Pending changes' })).toBeNull();
+    });
+
+    it('still renders a sync error icon when syncErrorCode is set', () => {
+      const { getByRole } = render(
+        <NoteCell {...baseProps} syncErrorCode={413} />
+      );
+
+      expect(
+        getByRole('img', {
+          name: 'Sync failed',
+          description: getSyncErrorMessage(413),
+        })
+      ).not.toBeNull();
+    });
+  });
+
+  describe('published', () => {
+    it('does not render a published icon when the note is not published', () => {
+      const { queryByRole } = render(<NoteCell {...baseProps} />);
+
+      expect(queryByRole('img', { name: 'Published note' })).toBeNull();
+    });
+
+    it('renders a published icon when publishURL is set', () => {
+      const { getByRole } = render(
+        <NoteCell
+          {...baseProps}
+          note={{
+            ...minimalNote,
+            publishURL: 'https://publish.simplenote.com/abc',
+          }}
+        />
+      );
+
+      expect(getByRole('img', { name: 'Published note' })).not.toBeNull();
+    });
+  });
+});

lib/note-list/note-cell.tsx

@@ -8,6 +8,7 @@ import SmallSyncIcon from '../icons/sync-small';
 import { decorateWith, makeFilterDecorator } from './decorators';
 import { getTerms } from '../utils/filter-notes';
 import { noteTitleAndPreview } from '../utils/note-utils';
+import { getSyncErrorMessage } from '../utils/sync-error-message';
 import { withCheckboxCharacters } from '../utils/task-transform';
 
 import actions from '../state/actions';
@@ -24,12 +25,14 @@ type OwnProps = {
 
 type StateProps = {
   displayMode: T.ListDisplayMode;
-  hasPendingChanges: boolean;
   isOffline: boolean;
   isOpened: boolean;
+  isSyncing: boolean;
   lastUpdated: number;
   note?: T.Note;
   searchQuery: string;
+  hasPendingChanges: boolean;
+  syncErrorCode: number | null;
 };
 
 type DispatchProps = {
@@ -69,16 +72,18 @@ export class NoteCell extends Component<Props> {
   render() {
     const {
       displayMode,
-      hasPendingChanges,
       isOffline,
       isOpened,
+      isSyncing,
       lastUpdated,
       noteId,
       note,
       openNote,
       pinNote,
       searchQuery,
+      hasPendingChanges,
       style,
+      syncErrorCode,
     } = this.props;
 
     if (!note) {
@@ -101,6 +106,20 @@ export class NoteCell extends Component<Props> {
       'note-list-item-pinned': isPinned,
     });
     const pinnerLabel = isPinned ? `Unpin note ${title}` : `Pin note ${title}`;
+    const hasSyncError = null !== syncErrorCode;
+    const isSyncErrorState = hasSyncError && !isSyncing;
+    const shouldShowStatusIcon = hasPendingChanges || hasSyncError;
+    const isSpinning =
+      isSyncing || (hasPendingChanges && !hasSyncError && !isOffline);
+    const pendingChangesLabel = isOffline
+      ? 'Pending changes (waiting for network connection)'
+      : 'Pending changes';
+    const statusIconLabel = isSyncErrorState
+      ? 'Sync failed'
+      : pendingChangesLabel;
+    const statusIconTooltip = isSyncErrorState
+      ? getSyncErrorMessage(syncErrorCode)
+      : undefined;
 
     const decorators = getTerms(searchQuery).map(makeFilterDecorator);
 
@@ -149,17 +168,26 @@ export class NoteCell extends Component<Props> {
             )}
           </button>
           <div className="note-list-item-status-right">
-            {hasPendingChanges && (
+            {shouldShowStatusIcon && (
               <span
+                aria-label={statusIconLabel}
                 className={classNames('note-list-item-pending-changes', {
-                  'is-offline': isOffline,
+                  'has-sync-error': isSyncErrorState,
+                  'is-offline': isOffline && !isSyncErrorState,
+                  'is-syncing': isSpinning,
                 })}
+                role="img"
+                title={statusIconTooltip}
               >
                 <SmallSyncIcon />
               </span>
             )}
             {isPublished && (
-              <span className="note-list-item-published-icon">
+              <span
+                aria-label="Published note"
+                className="note-list-item-published-icon"
+                role="img"
+              >
                 <PublishIcon />
               </span>
             )}
@@ -175,12 +203,14 @@ const mapStateToProps: S.MapState<StateProps, OwnProps> = (
   { noteId }
 ) => ({
   displayMode: state.settings.noteDisplay,
-  hasPendingChanges: selectors.noteHasPendingChanges(state, noteId),
   isOffline: state.simperium.connectionStatus === 'offline',
   isOpened: state.ui.openedNote === noteId,
+  isSyncing: state.simperium.syncingNotes.has(noteId),
   lastUpdated: state.simperium.lastRemoteUpdate.get(noteId) ?? -Infinity,
   note: state.data.notes.get(noteId),
   searchQuery: state.ui.searchQuery,
+  hasPendingChanges: selectors.noteHasPendingChanges(state, noteId),
+  syncErrorCode: state.simperium.syncErrors.get(noteId) ?? null,
 });
 
 const mapDispatchToProps: S.MapDispatch<DispatchProps> = {

lib/note-list/style.scss

@@ -210,14 +210,22 @@
   }
 
   .note-list-item-pending-changes {
-    & svg {
+    &.is-syncing svg {
       animation: 2s spin infinite linear;
     }
 
     &.is-offline svg {
       animation: none;
       fill: var(--tertiary-color);
     }
+
+    &.has-sync-error {
+      color: var(--tertiary-highlight-color);
+
+      svg {
+        animation: none;
+      }
+    }
   }
 
   .note-list-item-title {
@@ -246,7 +254,7 @@
       color: var(--primary-color);
     }
 
-    .note-list-item-pending-changes,
+    .note-list-item-pending-changes:not(.has-sync-error),
     .note-list-item-published-icon {
       color: var(--primary-color);
     }

lib/state/action-types.ts

@@ -275,6 +275,13 @@ export type NoteBucketRemove = Action<
   'NOTE_BUCKET_REMOVE',
   { noteId: T.EntityId }
 >;
+export type NoteSyncError = Action<
+  'NOTE_SYNC_ERROR',
+  {
+    noteId: T.EntityId;
+    errorCode: number;
+  }
+>;
 export type NoteBucketUpdate = Action<
   'NOTE_BUCKET_UPDATE',
   { noteId: T.EntityId; note: T.Note; isIndexing: boolean }
@@ -370,6 +377,7 @@ export type ActionType =
   | MarkdownNote
   | NoteBucketRemove
   | NoteBucketUpdate
+  | NoteSyncError
   | OpenNote
   | OpenRevision
   | OpenTag

lib/state/persistence.ts

@@ -85,6 +85,7 @@ export const loadState = (
                   ghosts: [new Map(state.cvs), new Map(state.ghosts)],
                   lastRemoteUpdate: new Map(state.lastRemoteUpdate),
                   lastSync: new Map(state.lastSync),
+                  syncErrors: new Map(state.syncErrors ?? []),
                 },
               };
 
@@ -175,6 +176,7 @@ export const saveState = (state: S.State) => {
   const ghosts = Array.from(state.simperium.ghosts[1]);
   const lastRemoteUpdate = Array.from(state.simperium.lastRemoteUpdate);
   const lastSync = Array.from(state.simperium.lastSync);
+  const syncErrors = Array.from(state.simperium.syncErrors);
 
   const data = {
     accountName: state.settings.accountName,
@@ -187,6 +189,7 @@ export const saveState = (state: S.State) => {
     ghosts,
     lastRemoteUpdate,
     lastSync,
+    syncErrors,
   };
 
   return openDB().then((db) => {

lib/state/selectors.test.ts

@@ -0,0 +1,59 @@
+import type { Ghost } from 'simperium';
+
+import { noteHasPendingChanges } from './selectors';
+import type * as S from './';
+import type * as T from '../types';
+
+const noteId = 'note-1' as T.EntityId;
+
+const localNote: T.Note = {
+  content: 'local content',
+  creationDate: 0,
+  deleted: false,
+  modificationDate: 1,
+  systemTags: [],
+  tags: [],
+};
+
+const ghostNote: T.Note = {
+  ...localNote,
+  content: 'server content',
+};
+
+const makeState = (
+  overrides: {
+    note?: T.Note;
+    ghost?: T.Note;
+  } = {}
+): S.State => {
+  const note = overrides.note ?? localNote;
+  const ghost = overrides.ghost ?? ghostNote;
+  const noteGhosts = new Map<T.EntityId, Ghost<T.Note>>();
+
+  if (ghost) {
+    noteGhosts.set(noteId, { data: ghost } as Ghost<T.Note>);
+  }
+
+  return {
+    data: {
+      notes: new Map([[noteId, note]]),
+    },
+    simperium: {
+      ghosts: [new Map(), new Map([['note', noteGhosts]])],
+    },
+  } as unknown as S.State;
+};
+
+describe('noteHasPendingChanges', () => {
+  it('returns false when local and ghost notes match', () => {
+    const state = makeState({ ghost: localNote });
+
+    expect(noteHasPendingChanges(state, noteId)).toBe(false);
+  });
+
+  it('returns true when local and ghost notes differ', () => {
+    const state = makeState();
+
+    expect(noteHasPendingChanges(state, noteId)).toBe(true);
+  });
+});