Improve types of annotation values in several places

 - Introduce a `SavedAnnotation` type to represent annotations which
   have definitely been saved and an associated `isSaved` function to
   test. Use this as the type for several methods which used to
   implicitly assume they were passed a saved annotation.

   Calls to `isSaved` have also been added in several places to check
   that an annotation has been saved, although this should always have
   been the case in practice.

 - Add missing annotation types to various parameters in
   `AnnotationsService`

src/sidebar/components/Annotation/Annotation.js

@@ -2,7 +2,7 @@ import { Actions } from '@hypothesis/frontend-shared';
 import classnames from 'classnames';
 
 import { useStoreProxy } from '../../store/use-store';
-import { quote } from '../../helpers/annotation-metadata';
+import { isSaved, quote } from '../../helpers/annotation-metadata';
 import { withServices } from '../../service-context';
 
 import AnnotationActionBar from './AnnotationActionBar';
@@ -14,6 +14,7 @@ import AnnotationReplyToggle from './AnnotationReplyToggle';
 
 /**
  * @typedef {import("../../../types/api").Annotation} Annotation
+ * @typedef {import("../../../types/api").SavedAnnotation} SavedAnnotation
  * @typedef {import('../../../types/api').Group} Group
  */
 
@@ -57,7 +58,11 @@ function Annotation({
   const showActions = !isSaving && !isEditing;
   const showReplyToggle = !isReply && !hasAppliedFilter && replyCount > 0;
 
-  const onReply = () => annotationsService.reply(annotation, userid);
+  const onReply = () => {
+    if (annotation && isSaved(annotation)) {
+      annotationsService.reply(annotation, userid);
+    }
+  };
 
   return (
     <article
@@ -111,7 +116,7 @@ function Annotation({
                 Saving...
               </div>
             )}
-            {annotation && showActions && (
+            {annotation && showActions && isSaved(annotation) && (
               <Actions classes="hyp-u-stretch">
                 <AnnotationActionBar
                   annotation={annotation}

src/sidebar/components/Annotation/AnnotationActionBar.js

@@ -13,13 +13,13 @@ import { useStoreProxy } from '../../store/use-store';
 import AnnotationShareControl from './AnnotationShareControl';
 
 /**
- *  @typedef {import("../../../types/api").Annotation} Annotation
+ *  @typedef {import("../../../types/api").SavedAnnotation} SavedAnnotation
  *  @typedef {import('../../../types/config').HostConfig} HostConfig
  */
 
 /**
  * @typedef AnnotationActionBarProps
- * @prop {Annotation} annotation - The annotation in question
+ * @prop {SavedAnnotation} annotation - The annotation in question
  * @prop {() => any} onReply - Callbacks for when action buttons are clicked/tapped
  * @prop {import('../../services/annotations').AnnotationsService} annotationsService
  * @prop {HostConfig} settings

src/sidebar/helpers/annotation-metadata.js

@@ -2,9 +2,12 @@
  * Utility functions for querying annotation metadata.
  */
 
-/** @typedef {import('../../types/api').Annotation} Annotation */
-/** @typedef {import('../../types/api').TextPositionSelector} TextPositionSelector */
-/** @typedef {import('../../types/api').TextQuoteSelector} TextQuoteSelector */
+/**
+ * @typedef {import('../../types/api').Annotation} Annotation
+ * @typedef {import('../../types/api').SavedAnnotation} SavedAnnotation
+ * @typedef {import('../../types/api').TextPositionSelector} TextPositionSelector
+ * @typedef {import('../../types/api').TextQuoteSelector} TextQuoteSelector
+ */
 
 /**
  * Extract document metadata from an annotation.
@@ -123,11 +126,21 @@ export function isReply(annotation) {
   return (annotation.references || []).length > 0;
 }
 
-/** Return `true` if the given annotation is new, `false` otherwise.
+/**
+ * Return true if the given annotation has been saved to the backend and assigned
+ * an ID.
  *
- * "New" means this annotation has been newly created client-side and not
- * saved to the server yet.
+ * @param {Annotation} annotation
+ * @return {annotation is SavedAnnotation}
+ */
+export function isSaved(annotation) {
+  return !!annotation.id;
+}
+
+/**
+ * Return true if an annotation has not been saved to the backend.
  *
+ * @deprecated - Use {@link isSaved} instead
  * @param {Annotation} annotation
  */
 export function isNew(annotation) {

src/sidebar/helpers/test/annotation-metadata-test.js

 import * as fixtures from '../../test/annotation-fixtures';
 import * as annotationMetadata from '../annotation-metadata';
-
-const documentMetadata = annotationMetadata.documentMetadata;
-const domainAndTitle = annotationMetadata.domainAndTitle;
+import {
+  documentMetadata,
+  domainAndTitle,
+  isSaved,
+} from '../annotation-metadata';
 
 describe('sidebar/helpers/annotation-metadata', () => {
   const fakeAnnotation = (props = {}) => {
@@ -452,6 +454,16 @@ describe('sidebar/helpers/annotation-metadata', () => {
     });
   });
 
+  describe('isSaved', () => {
+    it('returns true for saved annotations', () => {
+      assert.isTrue(isSaved(fixtures.defaultAnnotation()));
+    });
+
+    it('returns false for unsaved annotations', () => {
+      assert.isFalse(isSaved(fixtures.newAnnotation()));
+    });
+  });
+
   describe('flagCount', () => {
     const flagCount = annotationMetadata.flagCount;
 

src/sidebar/services/annotations.js

@@ -9,6 +9,7 @@ import {
 /**
  * @typedef {import('../../types/api').Annotation} Annotation
  * @typedef {import('../../types/annotator').AnnotationData} AnnotationData
+ * @typedef {import('../../types/api').SavedAnnotation} SavedAnnotation
  */
 
 /**
@@ -51,7 +52,7 @@ export class AnnotationsService {
   /**
    * Extend new annotation objects with defaults and permissions.
    *
-   * @param {AnnotationData} annotationData
+   * @param {Omit<AnnotationData, '$tag'>} annotationData
    * @param {Date} now
    * @return {Annotation}
    */
@@ -103,7 +104,7 @@ export class AnnotationsService {
    * Create a draft for it unless it's a highlight and clear other empty
    * drafts out of the way.
    *
-   * @param {object} annotationData
+   * @param {Omit<AnnotationData, '$tag'>} annotationData
    * @param {Date} now
    */
   create(annotationData, now = new Date()) {
@@ -171,6 +172,8 @@ export class AnnotationsService {
 
   /**
    * Flag an annotation for review by a moderator.
+   *
+   * @param {SavedAnnotation} annotation
    */
   async flag(annotation) {
     await this._api.annotation.flag({ id: annotation.id });
@@ -180,7 +183,7 @@ export class AnnotationsService {
   /**
    * Create a reply to `annotation` by the user `userid` and add to the store.
    *
-   * @param {object} annotation
+   * @param {SavedAnnotation} annotation
    * @param {string} userid
    */
   reply(annotation, userid) {
@@ -200,13 +203,15 @@ export class AnnotationsService {
    * Save new (or update existing) annotation. On success,
    * the annotation's `Draft` will be removed and the annotation added
    * to the store.
+   *
+   * @param {Annotation} annotation
    */
   async save(annotation) {
     let saved;
 
     const annotationWithChanges = this._applyDraftChanges(annotation);
 
-    if (metadata.isNew(annotation)) {
+    if (!metadata.isSaved(annotation)) {
       saved = this._api.annotation.create({}, annotationWithChanges);
     } else {
       saved = this._api.annotation.update(

src/sidebar/services/test/annotations-test.js

@@ -30,7 +30,7 @@ describe('AnnotationsService', () => {
     fakeMetadata = {
       isAnnotation: sinon.stub(),
       isHighlight: sinon.stub(),
-      isNew: sinon.stub(),
+      isSaved: sinon.stub(),
       isPageNote: sinon.stub(),
       isPublic: sinon.stub(),
     };
@@ -370,7 +370,7 @@ describe('AnnotationsService', () => {
 
   describe('save', () => {
     it('calls the `create` API service for new annotations', () => {
-      fakeMetadata.isNew.returns(true);
+      fakeMetadata.isSaved.returns(false);
       // Using the new-annotation fixture has no bearing on which API method
       // will get called because `isNew` is mocked, but it has representative
       // properties
@@ -384,7 +384,7 @@ describe('AnnotationsService', () => {
     });
 
     it('calls the `update` API service for pre-existing annotations', () => {
-      fakeMetadata.isNew.returns(false);
+      fakeMetadata.isSaved.returns(true);
 
       const annotation = fixtures.defaultAnnotation();
       return svc.save(annotation).then(() => {
@@ -396,7 +396,7 @@ describe('AnnotationsService', () => {
     });
 
     it('calls the relevant API service with an object that has any draft changes integrated', () => {
-      fakeMetadata.isNew.returns(true);
+      fakeMetadata.isSaved.returns(false);
       fakePrivatePermissions.returns({ read: ['foo'] });
       const annotation = fixtures.defaultAnnotation();
       annotation.text = 'not this';
@@ -424,7 +424,7 @@ describe('AnnotationsService', () => {
 
     context('successful save', () => {
       it('copies over internal app-specific keys to the annotation object', () => {
-        fakeMetadata.isNew.returns(false);
+        fakeMetadata.isSaved.returns(true);
         const annotation = fixtures.defaultAnnotation();
         annotation.$tag = 'mytag';
         annotation.$foo = 'bar';
@@ -450,7 +450,7 @@ describe('AnnotationsService', () => {
 
       it('adds the updated annotation to the store', () => {
         const annotation = fixtures.defaultAnnotation();
-        fakeMetadata.isNew.returns(false);
+        fakeMetadata.isSaved.returns(true);
         fakeApi.annotation.update.resolves(annotation);
 
         return svc.save(annotation).then(() => {
@@ -462,7 +462,7 @@ describe('AnnotationsService', () => {
     context('error on save', () => {
       it('removes the active save request from the store', () => {
         fakeApi.annotation.update.rejects();
-        fakeMetadata.isNew.returns(false);
+        fakeMetadata.isSaved.returns(true);
 
         return svc.save(fixtures.defaultAnnotation()).catch(() => {
           assert.notCalled(fakeStore.removeDraft);
@@ -472,7 +472,7 @@ describe('AnnotationsService', () => {
 
       it('does not remove the annotation draft', () => {
         fakeApi.annotation.update.rejects();
-        fakeMetadata.isNew.returns(false);
+        fakeMetadata.isSaved.returns(true);
 
         return svc.save(fixtures.defaultAnnotation()).catch(() => {
           assert.notCalled(fakeStore.removeDraft);
@@ -481,7 +481,7 @@ describe('AnnotationsService', () => {
 
       it('does not add the annotation to the store', () => {
         fakeApi.annotation.update.rejects();
-        fakeMetadata.isNew.returns(false);
+        fakeMetadata.isSaved.returns(true);
 
         return svc.save(fixtures.defaultAnnotation()).catch(() => {
           assert.notCalled(fakeStore.addAnnotations);

src/types/api.js

@@ -109,6 +109,12 @@
  * @prop {boolean} [$anchorTimeout]
  */
 
+/**
+ * An annotation which has been saved to the backend and assigned an ID.
+ *
+ * @typedef {Annotation & { id: string }} SavedAnnotation
+ */
+
 /**
  * @typedef Profile
  * @prop {string|null} userid