Commit 50595672 authored by Lyza Danger Gardner's avatar Lyza Danger Gardner Committed by Lyza Gardner

Emit and display `ResultSizeError` on too-many-annotations

Emit a `ResultSizeError` when encountering too many annotations to load.
Update `NotebookView` to display a message when this happens.
parent f936fffa
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 { ResultSizeError } from '../search-client';
import { withServices } from '../service-context'; import { withServices } from '../service-context';
import useRootThread from './hooks/use-root-thread'; import useRootThread from './hooks/use-root-thread';
import { useStoreProxy } from '../store/use-store'; import { useStoreProxy } from '../store/use-store';
...@@ -8,6 +9,7 @@ import { useStoreProxy } from '../store/use-store'; ...@@ -8,6 +9,7 @@ import { useStoreProxy } from '../store/use-store';
import NotebookFilters from './NotebookFilters'; import NotebookFilters from './NotebookFilters';
import NotebookResultCount from './NotebookResultCount'; import NotebookResultCount from './NotebookResultCount';
import Panel from './Panel';
import PaginatedThreadList from './PaginatedThreadList'; import PaginatedThreadList from './PaginatedThreadList';
/** /**
...@@ -46,11 +48,22 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -46,11 +48,22 @@ function NotebookView({ loadAnnotationsService }) {
const lastPaginationPage = useRef(1); const lastPaginationPage = useRef(1);
const [paginationPage, setPaginationPage] = useState(1); const [paginationPage, setPaginationPage] = useState(1);
// Load all annotations; re-load if `focusedGroup` changes const [hasTooManyAnnotationsError, setHasTooManyAnnotationsError] = useState(
useEffect(() => { false
);
// Load all annotations in the group, unless there are more than 5000 // Load all annotations in the group, unless there are more than 5000
// of them: this is a performance safety valve. // of them: this is a performance safety valve.
const maxResults = 5000;
const onLoadError = error => {
if (error instanceof ResultSizeError) {
setHasTooManyAnnotationsError(true);
}
};
// Load all annotations; re-load if `focusedGroup` changes
useEffect(() => {
// NB: In current implementation, this will only happen/load once (initial // NB: In current implementation, this will only happen/load once (initial
// annotation fetch on application startup), as there is no mechanism // annotation fetch on application startup), as there is no mechanism
// within the Notebook to change the `focusedGroup`. If the focused group // within the Notebook to change the `focusedGroup`. If the focused group
...@@ -60,8 +73,6 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -60,8 +73,6 @@ function NotebookView({ loadAnnotationsService }) {
if (groupId) { if (groupId) {
loadAnnotationsService.load({ loadAnnotationsService.load({
groupId, groupId,
maxResults: 5000,
// Load annotations in reverse-chronological order because that is how // Load annotations in reverse-chronological order because that is how
// threads are sorted in the notebook view. By aligning the fetch // threads are sorted in the notebook view. By aligning the fetch
// order with the thread display order we reduce the changes in visible // order with the thread display order we reduce the changes in visible
...@@ -74,6 +85,8 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -74,6 +85,8 @@ function NotebookView({ loadAnnotationsService }) {
// the top-level threads. // the top-level threads.
sortBy: 'updated', sortBy: 'updated',
sortOrder: 'desc', sortOrder: 'desc',
maxResults,
onError: onLoadError,
}); });
} }
}, [loadAnnotationsService, groupId, store]); }, [loadAnnotationsService, groupId, store]);
...@@ -115,6 +128,20 @@ function NotebookView({ loadAnnotationsService }) { ...@@ -115,6 +128,20 @@ function NotebookView({ loadAnnotationsService }) {
/> />
</div> </div>
<div className="NotebookView__items"> <div className="NotebookView__items">
{hasTooManyAnnotationsError && (
<div className="NotebookView__messages">
<Panel title="Too many results to show">
This preview of the Notebook can show{' '}
<strong>up to {maxResults} results</strong> at a time (there are{' '}
{resultCount} to show here).{' '}
<a href="mailto:support@hypothes.is?subject=Hypothesis%20Notebook&body=Please%20notify%20me%20when%20the%20Hypothesis%20Notebook%20is%20updated%20to%20support%20more%20than%205000%20annotations">
Contact us
</a>{' '}
if you would like to be notified when support for more annotations
is available.
</Panel>
</div>
)}
<PaginatedThreadList <PaginatedThreadList
currentPage={paginationPage} currentPage={paginationPage}
isLoading={isLoading} isLoading={isLoading}
......
...@@ -3,6 +3,7 @@ import { act } from 'preact/test-utils'; ...@@ -3,6 +3,7 @@ import { act } from 'preact/test-utils';
import mockImportedComponents from '../../../test-util/mock-imported-components'; import mockImportedComponents from '../../../test-util/mock-imported-components';
import { ResultSizeError } from '../../search-client';
import NotebookView, { $imports } from '../NotebookView'; import NotebookView, { $imports } from '../NotebookView';
describe('NotebookView', () => { describe('NotebookView', () => {
...@@ -60,6 +61,7 @@ describe('NotebookView', () => { ...@@ -60,6 +61,7 @@ describe('NotebookView', () => {
maxResults: 5000, maxResults: 5000,
sortBy: 'updated', sortBy: 'updated',
sortOrder: 'desc', sortOrder: 'desc',
onError: sinon.match.func,
}) })
); );
assert.calledWith(fakeStore.setSortKey, 'Newest'); assert.calledWith(fakeStore.setSortKey, 'Newest');
...@@ -78,6 +80,7 @@ describe('NotebookView', () => { ...@@ -78,6 +80,7 @@ describe('NotebookView', () => {
maxResults: 5000, maxResults: 5000,
sortBy: 'updated', sortBy: 'updated',
sortOrder: 'desc', sortOrder: 'desc',
onError: sinon.match.func,
}) })
); );
}); });
...@@ -91,6 +94,20 @@ describe('NotebookView', () => { ...@@ -91,6 +94,20 @@ describe('NotebookView', () => {
assert.notCalled(fakeLoadAnnotationsService.load); assert.notCalled(fakeLoadAnnotationsService.load);
}); });
it('shows a message if too many annotations to load', () => {
// Simulate the loading service emitting an error indicating
// too many annotations to load
fakeLoadAnnotationsService.load.callsFake(options => {
options.onError(new ResultSizeError(5000));
});
fakeStore.focusedGroup.returns({ id: 'hallothere', name: 'Hallo' });
const wrapper = createComponent();
const message = wrapper.find('.NotebookView__messages');
assert.include(message.text(), 'up to 5000 results at a time');
assert.isTrue(message.exists());
});
it('renders the current group name', () => { it('renders the current group name', () => {
fakeStore.focusedGroup.returns({ id: 'hallothere', name: 'Hallo' }); fakeStore.focusedGroup.returns({ id: 'hallothere', name: 'Hallo' });
const wrapper = createComponent(); const wrapper = createComponent();
......
...@@ -4,13 +4,27 @@ import { TinyEmitter } from 'tiny-emitter'; ...@@ -4,13 +4,27 @@ import { TinyEmitter } from 'tiny-emitter';
* @typedef {import('../types/api').Annotation} Annotation * @typedef {import('../types/api').Annotation} Annotation
* @typedef {import('../types/api').SearchQuery} SearchQuery * @typedef {import('../types/api').SearchQuery} SearchQuery
* @typedef {import('../types/api').SearchResult} SearchResult * @typedef {import('../types/api').SearchResult} SearchResult
*
*/ */
/**
* Indicates that there are more annotations matching the current API
* search request than the interface can currently handle displaying
* (Notebook).
*/
export class ResultSizeError extends Error {
/**
* @param {number} limit
*/
constructor(limit) {
super(`Results size exceeds ${limit}`);
}
}
/** /**
* @typedef {'created'|'updated'} SortOrder * @typedef {'created'|'updated'} SortOrder
* @typedef {'asc'|'desc'} SortBy * @typedef {'asc'|'desc'} SortBy
*/ */
/** /**
* Default callback used to get the page size for iterating through annotations. * Default callback used to get the page size for iterating through annotations.
* *
...@@ -31,7 +45,7 @@ function defaultPageSize(index) { ...@@ -31,7 +45,7 @@ function defaultPageSize(index) {
* *
* [1] https://h.readthedocs.io/en/latest/api-reference/#tag/annotations/paths/~1search/get * [1] https://h.readthedocs.io/en/latest/api-reference/#tag/annotations/paths/~1search/get
*/ */
export default class SearchClient extends TinyEmitter { export class SearchClient extends TinyEmitter {
/** /**
* @param {(query: SearchQuery) => Promise<SearchResult>} searchFn - Function for querying the search API * @param {(query: SearchQuery) => Promise<SearchResult>} searchFn - Function for querying the search API
* @param {Object} options * @param {Object} options
...@@ -116,6 +130,12 @@ export default class SearchClient extends TinyEmitter { ...@@ -116,6 +130,12 @@ export default class SearchClient extends TinyEmitter {
return; return;
} }
if (this._resultCount === null) {
// Emit the result count (total) on first encountering it
this._resultCount = results.total;
this.emit('resultCount', this._resultCount);
}
// For now, abort loading of annotations if `maxResults` is set and the // For now, abort loading of annotations if `maxResults` is set and the
// number of annotations in the results set exceeds that value. // number of annotations in the results set exceeds that value.
// //
...@@ -126,23 +146,14 @@ export default class SearchClient extends TinyEmitter { ...@@ -126,23 +146,14 @@ export default class SearchClient extends TinyEmitter {
// //
// This change has no effect on loading annotations in the SidebarView, // This change has no effect on loading annotations in the SidebarView,
// where the `maxResults` option is not used. // where the `maxResults` option is not used.
//
// TODO: Implement pagination
if (this._maxResults && results.total > this._maxResults) { if (this._maxResults && results.total > this._maxResults) {
this.emit( this.emit('error', new ResultSizeError(this._maxResults));
'error',
new Error('Results size exceeds maximum allowed annotations')
);
this.emit('end'); this.emit('end');
return; return;
} }
const page = results.rows.concat(results.replies || []); const page = results.rows.concat(results.replies || []);
if (this._resultCount === null) {
// Emit the result count (total) on first encountering it
this._resultCount = results.total;
this.emit('resultCount', this._resultCount);
}
if (this._incremental) { if (this._incremental) {
this.emit('results', page); this.emit('results', page);
} else { } else {
......
import SearchClient from '../search-client'; import { ResultSizeError, SearchClient } from '../search-client';
function awaitEvent(emitter, event) { function awaitEvent(emitter, event) {
return new Promise(function (resolve) { return new Promise(function (resolve) {
...@@ -218,10 +218,7 @@ describe('SearchClient', () => { ...@@ -218,10 +218,7 @@ describe('SearchClient', () => {
await awaitEvent(client, 'end'); await awaitEvent(client, 'end');
assert.calledOnce(onError); assert.calledOnce(onError);
assert.equal( assert.instanceOf(onError.getCall(0).args[0], ResultSizeError);
onError.getCall(0).args[0].message,
'Results size exceeds maximum allowed annotations'
);
}); });
it('does not emit an error if results size is <= `maxResults`', async () => { it('does not emit an error if results size is <= `maxResults`', async () => {
......
...@@ -52,5 +52,9 @@ ...@@ -52,5 +52,9 @@
justify-self: end; justify-self: end;
align-self: flex-end; align-self: flex-end;
} }
&__messages {
padding: 1em 0;
}
} }
} }
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