Commit 0eba1843 authored by Eduardo Sanz García's avatar Eduardo Sanz García Committed by Eduardo

Add annotation synchronisation to the notebook

This PR displays a button when there are new/deleted/updated annotations
on that the group. Clicking on the button updates the annotation thread
with the updated annotations. Currently, the button is placed by the
thread count.

If there are filters applied in the notebook the button is not shown.

To test this change make sure you update `h` to the latest version (so
it includes https://github.com/hypothesis/h/pull/6580)
parent 550eca2a
import { useEffect, useLayoutEffect, useRef, useState } from 'preact/hooks'; import { useEffect, useLayoutEffect, useRef, useState } from 'preact/hooks';
import scrollIntoView from 'scroll-into-view'; import scrollIntoView from 'scroll-into-view';
import { IconButton } from '../../shared/components/buttons';
import { ResultSizeError } from '../search-client'; import { ResultSizeError } from '../search-client';
import { withServices } from '../service-context'; import { withServices } from '../service-context';
import useRootThread from './hooks/use-root-thread';
import { useStoreProxy } from '../store/use-store'; import { useStoreProxy } from '../store/use-store';
import NotebookFilters from './NotebookFilters'; import NotebookFilters from './NotebookFilters';
import NotebookResultCount from './NotebookResultCount'; import NotebookResultCount from './NotebookResultCount';
import useRootThread from './hooks/use-root-thread';
import Panel from './Panel'; import Panel from './Panel';
import PaginatedThreadList from './PaginatedThreadList'; import PaginatedThreadList from './PaginatedThreadList';
...@@ -15,6 +16,7 @@ import PaginatedThreadList from './PaginatedThreadList'; ...@@ -15,6 +16,7 @@ import PaginatedThreadList from './PaginatedThreadList';
/** /**
* @typedef NotebookViewProps * @typedef NotebookViewProps
* @prop {Object} [loadAnnotationsService] - Injected service * @prop {Object} [loadAnnotationsService] - Injected service
* @prop {Object} [streamer] - Injected service
*/ */
/** /**
...@@ -22,7 +24,7 @@ import PaginatedThreadList from './PaginatedThreadList'; ...@@ -22,7 +24,7 @@ import PaginatedThreadList from './PaginatedThreadList';
* *
* @param {NotebookViewProps} props * @param {NotebookViewProps} props
*/ */
function NotebookView({ loadAnnotationsService }) { function NotebookView({ loadAnnotationsService, streamer }) {
const store = useStoreProxy(); const store = useStoreProxy();
const filters = store.getFilterValues(); const filters = store.getFilterValues();
...@@ -31,6 +33,7 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -31,6 +33,7 @@ function NotebookView({ loadAnnotationsService }) {
const hasAppliedFilter = store.hasAppliedFilter(); const hasAppliedFilter = store.hasAppliedFilter();
const isLoading = store.isLoading(); const isLoading = store.isLoading();
const resultCount = store.annotationResultCount(); const resultCount = store.annotationResultCount();
const pendingUpdateCount = store.pendingUpdateCount();
const rootThread = useRootThread(); const rootThread = useRootThread();
...@@ -62,6 +65,15 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -62,6 +65,15 @@ function NotebookView({ loadAnnotationsService }) {
} }
}; };
const hasFetchedProfile = store.hasFetchedProfile();
// Establish websocket connection
useEffect(() => {
if (streamer && hasFetchedProfile) {
streamer.connect({ applyUpdatesImmediately: false });
}
}, [hasFetchedProfile, streamer]);
// Load all annotations; re-load if `focusedGroup` changes // Load all annotations; re-load if `focusedGroup` changes
useEffect(() => { useEffect(() => {
// NB: In current implementation, this will only happen/load once (initial // NB: In current implementation, this will only happen/load once (initial
...@@ -87,6 +99,7 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -87,6 +99,7 @@ function NotebookView({ loadAnnotationsService }) {
sortOrder: 'desc', sortOrder: 'desc',
maxResults, maxResults,
onError: onLoadError, onError: onLoadError,
streamFilterBy: 'group',
}); });
} }
}, [loadAnnotationsService, groupId, store]); }, [loadAnnotationsService, groupId, store]);
...@@ -111,6 +124,10 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -111,6 +124,10 @@ function NotebookView({ loadAnnotationsService }) {
} }
}, [paginationPage]); }, [paginationPage]);
const tooltip = `Show ${pendingUpdateCount} new or updated ${
pendingUpdateCount > 1 ? 'annotations' : 'annotation'
}`;
return ( return (
<div className="NotebookView"> <div className="NotebookView">
<header className="NotebookView__heading" ref={threadListScrollTop}> <header className="NotebookView__heading" ref={threadListScrollTop}>
...@@ -119,7 +136,15 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -119,7 +136,15 @@ function NotebookView({ loadAnnotationsService }) {
<div className="NotebookView__filters"> <div className="NotebookView__filters">
<NotebookFilters /> <NotebookFilters />
</div> </div>
<div className="NotebookView__results"> <div className="NotebookView__results u-layout-row--align-center u-font--large">
{pendingUpdateCount > 0 && !hasAppliedFilter && (
<IconButton
icon="refresh"
onClick={() => streamer.applyPendingUpdates()}
variant="primary"
title={tooltip}
/>
)}
<NotebookResultCount <NotebookResultCount
forcedVisibleCount={forcedVisibleCount} forcedVisibleCount={forcedVisibleCount}
isFiltered={hasAppliedFilter} isFiltered={hasAppliedFilter}
...@@ -153,6 +178,6 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -153,6 +178,6 @@ function NotebookView({ loadAnnotationsService }) {
); );
} }
NotebookView.injectedProps = ['loadAnnotationsService']; NotebookView.injectedProps = ['loadAnnotationsService', 'streamer'];
export default withServices(NotebookView); export default withServices(NotebookView);
...@@ -131,7 +131,7 @@ function SidebarView({ ...@@ -131,7 +131,7 @@ function SidebarView({
const hasFetchedProfile = store.hasFetchedProfile(); const hasFetchedProfile = store.hasFetchedProfile();
useEffect(() => { useEffect(() => {
if (hasFetchedProfile && (sidebarHasOpened || isLoggedIn)) { if (hasFetchedProfile && (sidebarHasOpened || isLoggedIn)) {
streamer.connect(); streamer.connect({ applyUpdatesImmediately: false });
} }
}, [hasFetchedProfile, isLoggedIn, sidebarHasOpened, streamer]); }, [hasFetchedProfile, isLoggedIn, sidebarHasOpened, streamer]);
......
...@@ -13,6 +13,7 @@ describe('NotebookView', () => { ...@@ -13,6 +13,7 @@ describe('NotebookView', () => {
let fakeUseRootThread; let fakeUseRootThread;
let fakeScrollIntoView; let fakeScrollIntoView;
let fakeStore; let fakeStore;
let fakeStreamer;
beforeEach(() => { beforeEach(() => {
fakeLoadAnnotationsService = { fakeLoadAnnotationsService = {
...@@ -32,6 +33,13 @@ describe('NotebookView', () => { ...@@ -32,6 +33,13 @@ describe('NotebookView', () => {
isLoading: sinon.stub().returns(false), isLoading: sinon.stub().returns(false),
annotationResultCount: sinon.stub().returns(0), annotationResultCount: sinon.stub().returns(0),
setSortKey: sinon.stub(), setSortKey: sinon.stub(),
pendingUpdateCount: sinon.stub().returns(0),
hasFetchedProfile: sinon.stub().returns(true),
};
fakeStreamer = {
connect: sinon.stub(),
applyPendingUpdates: sinon.stub(),
}; };
$imports.$mock(mockImportedComponents()); $imports.$mock(mockImportedComponents());
...@@ -48,7 +56,10 @@ describe('NotebookView', () => { ...@@ -48,7 +56,10 @@ describe('NotebookView', () => {
function createComponent() { function createComponent() {
return mount( return mount(
<NotebookView loadAnnotationsService={fakeLoadAnnotationsService} /> <NotebookView
loadAnnotationsService={fakeLoadAnnotationsService}
streamer={fakeStreamer}
/>
); );
} }
...@@ -134,6 +145,38 @@ describe('NotebookView', () => { ...@@ -134,6 +145,38 @@ describe('NotebookView', () => {
assert.isTrue(wrapper.find('NotebookFilters').exists()); assert.isTrue(wrapper.find('NotebookFilters').exists());
}); });
describe('synchronization of annotations', () => {
beforeEach(() => {
fakeStore.focusedGroup.returns({ id: 'hallothere', name: 'Hallo' });
fakeStore.pendingUpdateCount.returns(3);
});
it("doesn't display button to synchronize annotations if filters are applied", () => {
fakeStore.hasAppliedFilter.returns(true);
const wrapper = createComponent();
const button = wrapper.find('IconButton[icon="refresh"]');
assert.isFalse(button.exists());
});
it('shows button to synchronize annotations if no filters are applied', () => {
const wrapper = createComponent();
const button = wrapper.find('IconButton[icon="refresh"]');
assert.isTrue(button.exists());
assert.include(button.prop('title'), 'Show 3 new or updated annotations');
});
it('synchronizes pending annotations', () => {
const wrapper = createComponent();
const button = wrapper.find('IconButton[icon="refresh"]');
assert.isTrue(button.exists());
button.prop('onClick')();
assert.called(fakeStreamer.applyPendingUpdates);
});
});
describe('pagination', () => { describe('pagination', () => {
it('passes the current pagination page to `PaginatedThreadList`', () => { it('passes the current pagination page to `PaginatedThreadList`', () => {
const wrapper = createComponent(); const wrapper = createComponent();
......
...@@ -21,6 +21,8 @@ ...@@ -21,6 +21,8 @@
* @prop {SortOrder} [sortOrder] * @prop {SortOrder} [sortOrder]
* @prop {(error: Error) => any} [onError] - Optional error handler for * @prop {(error: Error) => any} [onError] - Optional error handler for
* SearchClient. Default error handling logs errors to console. * SearchClient. Default error handling logs errors to console.
* @prop {'uri'|'group'} [streamFilterBy] - Set the websocket stream
* to filter by either URIs or groupIds.
*/ */
import { SearchClient } from '../search-client'; import { SearchClient } from '../search-client';
...@@ -44,8 +46,15 @@ export default function loadAnnotationsService( ...@@ -44,8 +46,15 @@ export default function loadAnnotationsService(
* *
* @param {LoadAnnotationOptions} options * @param {LoadAnnotationOptions} options
*/ */
function load(options) { function load({
const { groupId, onError, uris } = options; groupId,
uris,
onError,
maxResults,
sortBy,
sortOrder,
streamFilterBy = 'uri',
}) {
store.removeAnnotations(store.savedAnnotations()); store.removeAnnotations(store.savedAnnotations());
// Cancel previously running search client. // Cancel previously running search client.
...@@ -57,14 +66,24 @@ export default function loadAnnotationsService( ...@@ -57,14 +66,24 @@ export default function loadAnnotationsService(
searchClient.cancel(); searchClient.cancel();
} }
// Set the filter for the websocket stream
switch (streamFilterBy) {
case 'group':
streamFilter.resetFilter().addClause('/group', 'equals', groupId, true);
streamer.setConfig('filter', { filter: streamFilter.getFilter() });
break;
case 'uri':
default:
if (uris && uris.length > 0) { if (uris && uris.length > 0) {
streamFilter.resetFilter().addClause('/uri', 'one_of', uris); streamFilter.resetFilter().addClause('/uri', 'one_of', uris);
streamer.setConfig('filter', { filter: streamFilter.getFilter() }); streamer.setConfig('filter', { filter: streamFilter.getFilter() });
} }
break;
}
const searchOptions = { const searchOptions = {
incremental: true, incremental: true,
maxResults: options.maxResults ?? null, maxResults: maxResults ?? null,
separateReplies: false, separateReplies: false,
// Annotations are fetched in order of creation by default. This is expected // Annotations are fetched in order of creation by default. This is expected
...@@ -78,8 +97,8 @@ export default function loadAnnotationsService( ...@@ -78,8 +97,8 @@ export default function loadAnnotationsService(
// //
// If the backend would allow us to sort on document location, we could do even better. // If the backend would allow us to sort on document location, we could do even better.
sortBy: /** @type {SortBy} */ (options.sortBy ?? 'created'), sortBy: /** @type {SortBy} */ (sortBy ?? 'created'),
sortOrder: /** @type {SortOrder} */ (options.sortOrder ?? 'asc'), sortOrder: /** @type {SortOrder} */ (sortOrder ?? 'asc'),
}; };
searchClient = new SearchClient(api.search, searchOptions); searchClient = new SearchClient(api.search, searchOptions);
......
...@@ -18,6 +18,9 @@ export default function Streamer(store, auth, groups, session, settings) { ...@@ -18,6 +18,9 @@ export default function Streamer(store, auth, groups, session, settings) {
// The socket instance for this Streamer instance // The socket instance for this Streamer instance
let socket; let socket;
// Flag that controls when to apply pending updates
let updateImmediately = true;
// Client configuration messages, to be sent each time a new connection is // Client configuration messages, to be sent each time a new connection is
// established. // established.
const configMessages = {}; const configMessages = {};
...@@ -37,7 +40,7 @@ export default function Streamer(store, auth, groups, session, settings) { ...@@ -37,7 +40,7 @@ export default function Streamer(store, auth, groups, session, settings) {
break; break;
} }
if (store.route() !== 'sidebar') { if (updateImmediately) {
applyPendingUpdates(); applyPendingUpdates();
} }
} }
...@@ -186,10 +189,14 @@ export default function Streamer(store, auth, groups, session, settings) { ...@@ -186,10 +189,14 @@ export default function Streamer(store, auth, groups, session, settings) {
* *
* If the service has already connected this does nothing. * If the service has already connected this does nothing.
* *
* @return {Promise} Promise which resolves once the WebSocket connection * @param {Object} [options]
* @param {boolean} [options.applyUpdatesImmediately] - true if pending updates should be applied immediately
*
* @return {Promise<void>} Promise which resolves once the WebSocket connection
* process has started. * process has started.
*/ */
function connect() { function connect(options = {}) {
updateImmediately = options.applyUpdatesImmediately ?? true;
setUpAutoReconnect(); setUpAutoReconnect();
if (socket) { if (socket) {
return Promise.resolve(); return Promise.resolve();
......
...@@ -5,16 +5,19 @@ import loadAnnotationsService, { $imports } from '../load-annotations'; ...@@ -5,16 +5,19 @@ import loadAnnotationsService, { $imports } from '../load-annotations';
let searchClients; let searchClients;
let longRunningSearchClient = false; let longRunningSearchClient = false;
class FakeSearchClient extends EventEmitter { class FakeSearchClient extends EventEmitter {
constructor(searchFn, opts) { constructor(
searchFn,
{ incremental, separateReplies, sortBy = 'created', sortOrder = 'asc' }
) {
super(); super();
assert.ok(searchFn); assert.ok(searchFn);
searchClients.push(this); searchClients.push(this);
this.cancel = sinon.stub(); this.cancel = sinon.stub();
this.incremental = !!opts.incremental; this.incremental = !!incremental;
this.separateReplies = !!opts.separateReplies; this.separateReplies = !!separateReplies;
this.sortBy = opts.sortBy; this.sortBy = sortBy;
this.sortOrder = opts.sortOrder; this.sortOrder = sortOrder;
this.get = sinon.spy(query => { this.get = sinon.spy(query => {
if (!query.uri) { if (!query.uri) {
...@@ -326,6 +329,35 @@ describe('loadAnnotationsService', () => { ...@@ -326,6 +329,35 @@ describe('loadAnnotationsService', () => {
assert.calledWith(onError, error); assert.calledWith(onError, error);
}); });
it('configures the streamer to filter on uris (default)', () => {
const fakeAddClause = sinon.stub();
fakeStreamFilter.resetFilter.returns({ addClause: fakeAddClause });
const svc = createService();
// doesn't set the filtering if uris are undefined or []
svc.load({ groupId: fakeGroupId });
assert.notCalled(fakeAddClause);
assert.notCalled(fakeStreamer.setConfig);
svc.load({ groupId: fakeGroupId, uris: [] });
assert.notCalled(fakeAddClause);
assert.notCalled(fakeStreamer.setConfig);
svc.load({ groupId: fakeGroupId, uris: fakeUris });
assert.calledWith(fakeAddClause, '/uri', 'one_of', fakeUris);
assert.called(fakeStreamer.setConfig);
});
it('configures the streamer to filter on groups (if streamFilterBy is set to "group")', () => {
const fakeAddClause = sinon.stub();
fakeStreamFilter.resetFilter.returns({ addClause: fakeAddClause });
const svc = createService();
svc.load({ groupId: fakeGroupId, streamFilterBy: 'group' });
assert.calledWith(fakeAddClause, '/group', 'equals', fakeGroupId, true);
assert.called(fakeStreamer.setConfig);
});
}); });
describe('loadThread', () => { describe('loadThread', () => {
......
...@@ -111,7 +111,6 @@ describe('Streamer', function () { ...@@ -111,7 +111,6 @@ describe('Streamer', function () {
}), }),
receiveRealTimeUpdates: sinon.stub(), receiveRealTimeUpdates: sinon.stub(),
removeAnnotations: sinon.stub(), removeAnnotations: sinon.stub(),
route: sinon.stub().returns('sidebar'),
} }
); );
...@@ -297,12 +296,11 @@ describe('Streamer', function () { ...@@ -297,12 +296,11 @@ describe('Streamer', function () {
describe('annotation notifications', function () { describe('annotation notifications', function () {
beforeEach(function () { beforeEach(function () {
createDefaultStreamer(); createDefaultStreamer();
return activeStreamer.connect();
}); });
context('when the app is the stream', function () { context('when the app is the stream', function () {
beforeEach(function () { beforeEach(function () {
fakeStore.route.returns('stream'); return activeStreamer.connect();
}); });
it('applies updates immediately', function () { it('applies updates immediately', function () {
...@@ -324,6 +322,10 @@ describe('Streamer', function () { ...@@ -324,6 +322,10 @@ describe('Streamer', function () {
}); });
context('when the app is the sidebar', function () { context('when the app is the sidebar', function () {
beforeEach(function () {
return activeStreamer.connect({ applyUpdatesImmediately: false });
});
it('saves pending updates', function () { it('saves pending updates', function () {
fakeWebSocket.notify(fixtures.createNotification); fakeWebSocket.notify(fixtures.createNotification);
assert.calledWith(fakeStore.receiveRealTimeUpdates, { assert.calledWith(fakeStore.receiveRealTimeUpdates, {
...@@ -348,17 +350,6 @@ describe('Streamer', function () { ...@@ -348,17 +350,6 @@ describe('Streamer', function () {
assert.notCalled(fakeStore.addAnnotations); assert.notCalled(fakeStore.addAnnotations);
}); });
it('does not apply deletions immediately', function () {
const ann = fixtures.deleteNotification.payload;
fakeStore.pendingDeletions.returns({
[ann.id]: true,
});
fakeWebSocket.notify(fixtures.deleteNotification);
assert.notCalled(fakeStore.removeAnnotations);
});
}); });
}); });
......
...@@ -3,7 +3,6 @@ ...@@ -3,7 +3,6 @@
.NotebookResultCount { .NotebookResultCount {
@include layout.horizontal-rhythm; @include layout.horizontal-rhythm;
font-size: var.$font-size--large;
// Normalize line height to ensure vertical stability (no jumping around) // Normalize line height to ensure vertical stability (no jumping around)
line-height: 1; line-height: 1;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment