diff --git a/src/sidebar/components/NotebookView.js b/src/sidebar/components/NotebookView.js index 6e6953859ee..b2913b88194 100644 --- a/src/sidebar/components/NotebookView.js +++ b/src/sidebar/components/NotebookView.js @@ -81,6 +81,7 @@ function NotebookView({ loadAnnotationsService, streamer }) { store.setSortKey('Newest'); if (groupId) { loadAnnotationsService.load({ + frameId: null, groupId, // Load annotations in reverse-chronological order because that is how // threads are sorted in the notebook view. By aligning the fetch diff --git a/src/sidebar/components/SidebarView.js b/src/sidebar/components/SidebarView.js index 18a2eaa51a8..92e1962dbbe 100644 --- a/src/sidebar/components/SidebarView.js +++ b/src/sidebar/components/SidebarView.js @@ -51,7 +51,7 @@ function SidebarView({ ? tabForAnnotation(linkedAnnotation) : 'annotation'; - const searchUris = store.searchUris(); + const searchURIMap = store.searchURIMap(); const sidebarHasOpened = store.hasSidebarOpened(); const userId = store.profile().userid; @@ -105,13 +105,16 @@ function SidebarView({ } prevGroupId.current = focusedGroupId; } - if (focusedGroupId && searchUris.length) { - loadAnnotationsService.load({ - groupId: focusedGroupId, - uris: searchUris, - }); + if (focusedGroupId && searchURIMap.size) { + for (let [frameId, uris] of searchURIMap) { + loadAnnotationsService.load({ + frameId, + groupId: focusedGroupId, + uris, + }); + } } - }, [store, loadAnnotationsService, focusedGroupId, userId, searchUris]); + }, [store, loadAnnotationsService, focusedGroupId, userId, searchURIMap]); // When a `linkedAnnotationAnchorTag` becomes available, scroll to it // and focus it diff --git a/src/sidebar/components/StreamView.js b/src/sidebar/components/StreamView.js index 936e3133c45..901ccb76f0a 100644 --- a/src/sidebar/components/StreamView.js +++ b/src/sidebar/components/StreamView.js @@ -42,7 +42,10 @@ function StreamView({ api, toastMessenger }) { try { store.annotationFetchStarted(); const results = await api.search(queryParams); - store.addAnnotations([...results.rows, ...results.replies]); + store.addAnnotations(null /* frameId */, [ + ...results.rows, + ...results.replies, + ]); } finally { store.annotationFetchFinished(); } diff --git a/src/sidebar/services/annotations.js b/src/sidebar/services/annotations.js index 53fe70fa8e8..6fb8e109245 100644 --- a/src/sidebar/services/annotations.js +++ b/src/sidebar/services/annotations.js @@ -52,11 +52,12 @@ export class AnnotationsService { /** * Extend new annotation objects with defaults and permissions. * + * @param {string|null} frameId * @param {Omit} annotationData * @param {Date} now * @return {Annotation} */ - _initialize(annotationData, now = new Date()) { + _initialize(frameId, annotationData, now = new Date()) { const defaultPrivacy = this._store.getDefault('annotationPrivacy'); const groupid = this._store.focusedGroupId(); const profile = this._store.profile(); @@ -76,6 +77,9 @@ export class AnnotationsService { /** @type {Annotation} */ const annotation = Object.assign( { + $frameId: frameId, + $tag, + created: now.toISOString(), group: groupid, permissions: defaultPermissions(userid, groupid, defaultPrivacy), @@ -84,7 +88,6 @@ export class AnnotationsService { updated: now.toISOString(), user: userid, user_info: userInfo, - $tag, hidden: false, links: {}, document: { title: '' }, @@ -104,13 +107,14 @@ export class AnnotationsService { * Create a draft for it unless it's a highlight and clear other empty * drafts out of the way. * + * @param {string|null} frameId * @param {Omit} annotationData * @param {Date} now */ - create(annotationData, now = new Date()) { - const annotation = this._initialize(annotationData, now); + create(frameId, annotationData, now = new Date()) { + const annotation = this._initialize(frameId, annotationData, now); - this._store.addAnnotations([annotation]); + this._store.addAnnotations(frameId, [annotation]); // Remove other drafts that are in the way, and their annotations (if new) this._store.deleteNewAndEmptyDrafts(); @@ -159,7 +163,7 @@ export class AnnotationsService { target: [], uri: topLevelFrame.uri, }; - this.create(pageNoteAnnotation); + this.create(topLevelFrame.id, pageNoteAnnotation); } /** @@ -196,7 +200,7 @@ export class AnnotationsService { target: [{ source: annotation.target[0].source }], uri: annotation.uri, }; - this.create(replyAnnotation); + this.create(annotation.$frameId, replyAnnotation); } /** @@ -238,7 +242,7 @@ export class AnnotationsService { this._store.removeDraft(annotation); // Add (or, in effect, update) the annotation to the store's collection - this._store.addAnnotations([savedAnnotation]); + this._store.addAnnotations(annotation.$frameId, [savedAnnotation]); return savedAnnotation; } } diff --git a/src/sidebar/services/frame-sync.js b/src/sidebar/services/frame-sync.js index 36e38e89499..86c58b886c7 100644 --- a/src/sidebar/services/frame-sync.js +++ b/src/sidebar/services/frame-sync.js @@ -297,7 +297,7 @@ export class FrameSyncService { this._hostRPC.call('showHighlights'); // Create the new annotation in the sidebar. - this._annotationsService.create(annot); + this._annotationsService.create(frameIdentifier, annot); } ); diff --git a/src/sidebar/services/groups.js b/src/sidebar/services/groups.js index 9d50b031fcf..f9f24a0d405 100644 --- a/src/sidebar/services/groups.js +++ b/src/sidebar/services/groups.js @@ -455,8 +455,8 @@ export class GroupsService { .filter(ann => !isReply(ann)) .map(ann => ({ ...ann, group: newGroupId })); - if (updatedAnnotations.length) { - this._store.addAnnotations(updatedAnnotations); + for (let ann of updatedAnnotations) { + this._store.addAnnotations(ann.$frameId, [ann]); } // Persist this group as the last focused group default diff --git a/src/sidebar/services/load-annotations.js b/src/sidebar/services/load-annotations.js index b9d62399368..262484eb7bc 100644 --- a/src/sidebar/services/load-annotations.js +++ b/src/sidebar/services/load-annotations.js @@ -8,6 +8,7 @@ import { SearchClient } from '../search-client'; /** * @typedef LoadAnnotationOptions + * @prop {string|null} frameId * @prop {string} groupId * @prop {string[]} [uris] * @prop {number} [maxResults] - If number of annotations in search results @@ -47,8 +48,8 @@ export class LoadAnnotationsService { this._streamer = streamer; this._streamFilter = streamFilter; - /** @type {SearchClient|null} */ - this._searchClient = null; + /** @type {Map} */ + this._searchClients = new Map(); } /** @@ -60,6 +61,7 @@ export class LoadAnnotationsService { * @param {LoadAnnotationOptions} options */ load({ + frameId, groupId, uris, onError, @@ -68,18 +70,24 @@ export class LoadAnnotationsService { sortOrder, streamFilterBy = 'uri', }) { - this._store.removeAnnotations(this._store.savedAnnotations()); + const currentAnnotations = this._store + .savedAnnotations() + .filter(a => a.$frameId === frameId); + this._store.removeAnnotations(currentAnnotations); // Cancel previously running search client. // // This will emit the "end" event for the existing client and trigger cleanup // associated with that client (eg. resetting the count of in-flight // annotation fetches). - if (this._searchClient) { - this._searchClient.cancel(); + const prevSearchClient = this._searchClients.get(frameId); + if (prevSearchClient) { + prevSearchClient.cancel(); } // Set the filter for the websocket stream + // + // TODO: How will multiple frames be handled here? Multiple WebSocket clients? switch (streamFilterBy) { case 'group': this._streamFilter @@ -120,19 +128,21 @@ export class LoadAnnotationsService { sortOrder, }; - this._searchClient = new SearchClient(this._api.search, searchOptions); + const searchClient = new SearchClient(this._api.search, searchOptions); + this._searchClients.set(frameId, searchClient); - this._searchClient.on('resultCount', resultCount => { + searchClient.on('resultCount', resultCount => { + // TODO - Pass frame ID here. this._store.setAnnotationResultCount(resultCount); }); - this._searchClient.on('results', results => { + searchClient.on('results', results => { if (results.length) { - this._store.addAnnotations(results); + this._store.addAnnotations(frameId, results); } }); - this._searchClient.on('error', error => { + searchClient.on('error', error => { if (typeof onError === 'function') { onError(error); } else { @@ -140,13 +150,14 @@ export class LoadAnnotationsService { } }); - this._searchClient.on('end', () => { + searchClient.on('end', () => { // Remove client as it's no longer active. - this._searchClient = null; + this._searchClients.delete(frameId); if (uris && uris.length > 0) { this._store.frames().forEach(frame => { if (uris.indexOf(frame.uri) >= 0) { + // TODO - Replace `frame.uri` with frame ID here? this._store.updateFrameAnnotationFetchStatus(frame.uri, true); } }); @@ -155,7 +166,7 @@ export class LoadAnnotationsService { }); this._store.annotationFetchStarted(); - this._searchClient.get({ group: groupId, uri: uris }); + searchClient.get({ group: groupId, uri: uris }); } /** @@ -192,7 +203,7 @@ export class LoadAnnotationsService { } const threadAnnotations = [annotation, ...replySearchResult.rows]; - this._store.addAnnotations(threadAnnotations); + this._store.addAnnotations(null /* frameId */, threadAnnotations); // If we've been successful in retrieving a thread, with a top-level annotation, // configure the connection to the real-time update service to send us diff --git a/src/sidebar/services/streamer.js b/src/sidebar/services/streamer.js index c8089fdce87..abc980c3929 100644 --- a/src/sidebar/services/streamer.js +++ b/src/sidebar/services/streamer.js @@ -71,6 +71,7 @@ export class StreamerService { applyPendingUpdates() { const updates = Object.values(this._store.pendingUpdates()); if (updates.length) { + // TODO - Handle multiple frames this._store.addAnnotations(updates); } @@ -169,6 +170,9 @@ export class StreamerService { * @param {object} configMessage */ setConfig(key, configMessage) { + // TODO - Figure out how to handle multiple frames. Do we create a separate + // connection for each frame or make changes on the backend so we can figure + // out which annotation goes with which frame? this._configMessages[key] = configMessage; if (this._socket?.isConnected()) { this._socket.send(configMessage); diff --git a/src/sidebar/store/modules/annotations.js b/src/sidebar/store/modules/annotations.js index 72c5b5b8311..87f9cece080 100644 --- a/src/sidebar/store/modules/annotations.js +++ b/src/sidebar/store/modules/annotations.js @@ -70,18 +70,16 @@ function findByTag(annotations, tag) { } /** - * Set custom private fields on an annotation object about to be added to the - * store's collection of `annotations`. + * Initialize the local ("$"-prefixed) fields of an annotation. * - * `annotation` may either be new (unsaved) or a persisted annotation retrieved - * from the service. - * - * @param {Omit} annotation + * @param {Annotation} annotation - New (unsaved) annotation or one retrieved from + * the backend. This annotation may not have all local fields set. * @param {string} tag - The `$tag` value that should be used for this * if it doesn't have a `$tag` already - * @return {Annotation} - annotation with local (`$*`) fields set + * @param {string} frameId + * @return {Annotation} - Annotation with all local fields set */ -function initializeAnnotation(annotation, tag) { +function initializeAnnotation(annotation, tag, frameId) { let orphan = annotation.$orphan; if (!annotation.id) { @@ -89,12 +87,14 @@ function initializeAnnotation(annotation, tag) { orphan = false; } - return Object.assign({}, annotation, { - // Flag indicating whether waiting for the annotation to anchor timed out. + return { + ...annotation, + $anchorTimeout: false, + $frameId: frameId, $tag: annotation.$tag || tag, $orphan: orphan, - }); + }; } const initialState = { @@ -155,7 +155,7 @@ const reducers = { updatedTags[existing.$tag] = true; } } else { - added.push(initializeAnnotation(annot, 't' + nextTag)); + added.push(initializeAnnotation(annot, 't' + nextTag, action.frameId)); ++nextTag; } }); @@ -267,9 +267,10 @@ const actions = util.actionTypes(reducers); /** * Add these `annotations` to the current collection of annotations in the store. * + * @param {string|null} frameId * @param {Annotation[]} annotations - Array of annotation objects to add. */ -function addAnnotations(annotations) { +function addAnnotations(frameId, annotations) { return function (dispatch, getState) { const added = annotations.filter(annot => { return ( @@ -279,6 +280,7 @@ function addAnnotations(annotations) { dispatch({ type: actions.ADD_ANNOTATIONS, + frameId, annotations, currentAnnotationCount: getState().annotations.annotations.length, }); diff --git a/src/sidebar/store/modules/frames.js b/src/sidebar/store/modules/frames.js index 31abf935fb6..d3202e770e5 100644 --- a/src/sidebar/store/modules/frames.js +++ b/src/sidebar/store/modules/frames.js @@ -1,8 +1,4 @@ -import { - createSelector, - createSelectorCreator, - defaultMemoize, -} from 'reselect'; +import { createSelector } from 'reselect'; import shallowEqual from 'shallowequal'; import * as util from '../util'; @@ -113,9 +109,9 @@ const mainFrame = createSelector( ); /** - * @param {Frame} frame + * @param {Pick} frame */ -function searchUrisForFrame(frame) { +function searchURIsForFrame(frame) { let uris = [frame.uri]; if (frame.metadata && frame.metadata.documentFingerprint) { @@ -135,24 +131,42 @@ function searchUrisForFrame(frame) { return uris; } -// "selector creator" that uses `shallowEqual` instead of `===` for memoization -const createShallowEqualSelector = createSelectorCreator( - defaultMemoize, - shallowEqual -); +/** + * Return true if two arrays have the same entries in the same order. + * + * @template T + * @param {T[]} a + * @param {T[]} b + */ +function entriesShallowEqual(a, b) { + return ( + a.length === b.length && a.every((item, i) => shallowEqual(item, b[i])) + ); +} /** - * Memoized selector will return the same array (of URIs) reference unless the - * values of the array change (are not shallow-equal). + * Return a map of frame ID to search URIs. + * + * The returned map's identity will change only if the set of frames or + * URIs for those frames changes. */ -const searchUris = createShallowEqualSelector( +const searchURIMap = createSelector( + // Extract only fields of frames which may affect search URIs. /** @param {State} frames */ - frames => - frames.reduce( - (uris, frame) => uris.concat(searchUrisForFrame(frame)), - /** @type {string[]} */ ([]) - ), - uris => uris + frames => frames.map(f => ({ id: f.id, uri: f.uri, metadata: f.metadata })), + frames => { + /** @type {Map} */ + const uriMap = new Map(); + for (let frame of frames) { + uriMap.set(frame.id, searchURIsForFrame(frame)); + } + return uriMap; + }, + { + memoizeOptions: { + equalityCheck: entriesShallowEqual, + }, + } ); export const framesModule = createStoreModule(initialState, { @@ -168,6 +182,6 @@ export const framesModule = createStoreModule(initialState, { selectors: { frames, mainFrame, - searchUris, + searchURIMap, }, }); diff --git a/src/types/api.js b/src/types/api.js index 7f188e0902e..14a42c0b491 100644 --- a/src/types/api.js +++ b/src/types/api.js @@ -79,6 +79,9 @@ * @prop {string} $tag - A locally-generated unique identifier for annotations. * This is set for all annotations, whether they have been saved to the backend * or not. + * @prop {string|null} $frameId - Local identifier for the frame that this annotation + * is associated with. TODO - Decide what to do if the same annotation + * is returned for multiple frames. * @prop {string[]} [references] * @prop {string} created * @prop {boolean} [flagged]