Commit 44c7e693 authored by Markus Scheidgen's avatar Markus Scheidgen
Browse files

Fixed performance and other isseus.

parent eeaef560
......@@ -11,7 +11,6 @@ import { domains } from './domains'
class About extends React.Component {
static propTypes = {
classes: PropTypes.object.isRequired,
api: PropTypes.object.isRequired,
info: PropTypes.object,
raiseError: PropTypes.func.isRequired
}
......
// trigger rebuild
import React from 'react'
import React, { useEffect, useState, useContext, useCallback } from 'react'
import PropTypes, { instanceOf } from 'prop-types'
import { compose } from 'recompose'
import classNames from 'classnames'
......@@ -17,7 +17,7 @@ import FAQIcon from '@material-ui/icons/QuestionAnswer'
import MetainfoIcon from '@material-ui/icons/Info'
import {help as searchHelp, default as SearchPage} from './search/SearchPage'
import HelpDialog from './Help'
import { ApiProvider, withApi } from './api'
import { ApiProvider, withApi, apiContext } from './api'
import { ErrorSnacks, withErrors } from './errors'
import { help as entryHelp, default as EntryPage } from './entry/EntryPage'
import About from './About'
......@@ -31,7 +31,6 @@ import {help as uploadHelp, default as UploadPage} from './uploads/UploadPage'
import ResolvePID from './entry/ResolvePID'
import DatasetPage from './DatasetPage'
import { amber } from '@material-ui/core/colors'
import KeepState from './KeepState'
import {help as userdataHelp, default as UserdataPage} from './UserdataPage'
import ResolveDOI from './dataset/ResolveDOI'
import FAQ from './FAQ'
......@@ -39,6 +38,18 @@ import EntryQuery from './entry/EntryQuery'
export const ScrollContext = React.createContext({scrollParentRef: null})
function LoadingIndicator() {
const {api} = useContext(apiContext)
const [loading, setLoading] = useState(0)
const handleOnLoading = useCallback(loading => setLoading(loading), [setLoading])
useEffect(() => {
api.onLoading(handleOnLoading)
return () => api.removeOnLoading(handleOnLoading)
}, [])
return loading ? <LinearProgress color="secondary" /> : ''
}
export class VersionMismatch extends Error {
constructor(msg) {
super(msg)
......@@ -98,7 +109,6 @@ class NavigationUnstyled extends React.Component {
classes: PropTypes.object.isRequired,
children: PropTypes.any,
location: PropTypes.object.isRequired,
loading: PropTypes.number.isRequired,
raiseError: PropTypes.func.isRequired
}
......@@ -232,7 +242,7 @@ class NavigationUnstyled extends React.Component {
}
render() {
const { classes, children, location: { pathname }, loading } = this.props
const { classes, children, location: { pathname } } = this.props
const { toolbarHelp, toolbarTitles } = this
const { showReloadSnack } = this.state
......@@ -311,7 +321,7 @@ class NavigationUnstyled extends React.Component {
icon={<MetainfoIcon/>}
/>
</MenuList>
{loading ? <LinearProgress color="secondary" /> : ''}
<LoadingIndicator />
</AppBar>
<main className={classes.content} ref={(ref) => { this.scroll.scrollParentRef = ref }}>
......
import React from 'react'
import React, { useContext, useEffect, useCallback, useRef } from 'react'
import PropTypes from 'prop-types'
import { withErrors } from './errors'
import { UploadRequest } from '@navjobs/upload'
......@@ -204,14 +204,23 @@ class Api {
}
constructor(keycloak) {
this.onStartLoading = () => null
this.onFinishLoading = () => null
this.statistics = {}
this._swaggerClient = Swagger(`${apiBase}/swagger.json`)
this.keycloak = keycloak
this.loadingHandler = []
this.loading = 0
this.onFinishLoading = () => {
this.loading--
this.loadingHandler.forEach(handler => handler(this.loading))
}
this.onStartLoading = () => {
this.loading++
this.loadingHandler.forEach(handler => handler(this.loading))
}
Api.uploadIds = 0
}
......@@ -228,6 +237,14 @@ class Api {
return upload
}
onLoading(handler) {
this.loadingHandler = [...this.loadingHandler, handler]
}
removeOnLoading(handler) {
this.loadingHandler = this.loadingHandler.filter(item => item !== handler)
}
async getUploads(state, page, perPage) {
state = state || 'all'
page = page || 1
......@@ -610,13 +627,6 @@ export class ApiProviderComponent extends React.Component {
createApi(keycloak) {
const api = new Api(keycloak)
api.onStartLoading = (name) => {
this.setState(state => ({loading: state.loading + 1}))
}
api.onFinishLoading = (name) => {
this.setState(state => ({loading: Math.max(0, state.loading - 1)}))
}
api.getInfo()
.catch(handleApiError)
.then(info => {
......@@ -634,8 +644,7 @@ export class ApiProviderComponent extends React.Component {
state = {
api: null,
info: null,
loading: 0
info: null
}
render() {
......@@ -684,16 +693,23 @@ class LoginRequiredUnstyled extends React.Component {
}
}
export function DisableOnLoading(props) {
return (
<apiContext.Consumer>
{apiContext => (
<div style={apiContext.loading ? { pointerEvents: 'none', userSelects: 'none' } : {}}>
{props.children}
</div>
)}
</apiContext.Consumer>
)
export function DisableOnLoading({children}) {
const containerRef = useRef(null)
const {api} = useContext(apiContext)
const handleLoading = useCallback((loading) => {
const enable = loading ? 'none' : ''
containerRef.current.style.pointerEvents = enable
containerRef.current.style.userSelects = enable
}, [api])
useEffect(() => {
api.onLoading(handleLoading)
return () => {
api.removeOnLoading(handleLoading)
}
}, [])
return <div ref={containerRef}>{children}</div>
}
DisableOnLoading.propTypes = {
children: PropTypes.any.isRequired
......
......@@ -18,7 +18,6 @@ class RawFiles extends React.Component {
data: PropTypes.object,
api: PropTypes.object.isRequired,
user: PropTypes.object,
loading: PropTypes.number.isRequired,
raiseError: PropTypes.func.isRequired
}
......@@ -72,7 +71,8 @@ class RawFiles extends React.Component {
fileContents: null,
shownFile: null,
files: null,
doesNotExist: false
doesNotExist: false,
loading: false
}
constructor(props) {
......@@ -103,15 +103,16 @@ class RawFiles extends React.Component {
if (files.length > 500) {
raiseError('There are more than 500 files in this entry. We can only show the first 500.')
}
this.setState({files: files})
this.setState({files: files, loading: false})
}).catch(error => {
this.setState({files: null})
this.setState({files: null, loading: false})
if (error.name === 'DoesNotExist') {
this.setState({doesNotExist: true})
} else {
raiseError(error)
}
})
this.setState({loading: true})
}
label(file) {
......@@ -185,8 +186,8 @@ class RawFiles extends React.Component {
}
render() {
const {classes, uploadId, calcId, loading, data} = this.props
const {selectedFiles, files, doesNotExist, fileContents, shownFile} = this.state
const {classes, uploadId, calcId, data} = this.props
const {selectedFiles, files, doesNotExist, fileContents, shownFile, loading} = this.state
const availableFiles = files || data.files || []
......@@ -259,7 +260,7 @@ class RawFiles extends React.Component {
label: file === shownFile ? classes.shownFile : classes.fileNameLabel}}
control={
<Checkbox
disabled={loading > 0}
disabled={loading}
checked={selectedFiles.indexOf(file) !== -1}
onChange={() => this.handleSelectFile(file)} value={file}
/>}
......
......@@ -2,7 +2,7 @@ import React from 'react'
import PropTypes from 'prop-types'
import { withStyles } from '@material-ui/core/styles'
/* eslint-disable-next-line */
import { domains } from '../domains' // TODO this causes a weird import bug
// import { domains } from '../domains' // TODO this causes a weird import bug
import ChipInput from 'material-ui-chip-input'
import Autosuggest from 'react-autosuggest'
import match from 'autosuggest-highlight/match'
......@@ -11,9 +11,7 @@ import Paper from '@material-ui/core/Paper'
import MenuItem from '@material-ui/core/MenuItem'
import { Chip, IconButton, Tooltip } from '@material-ui/core'
import { nomadPrimaryColor } from '../../config'
import { compose } from 'recompose'
import { searchContext } from './SearchContext'
import { withApi } from '../api'
import ClearIcon from '@material-ui/icons/Cancel'
function renderInput(inputProps) {
......@@ -91,8 +89,7 @@ function getSuggestionValue(suggestion) {
class SearchBar extends React.Component {
static propTypes = {
classes: PropTypes.object.isRequired,
loading: PropTypes.number
classes: PropTypes.object.isRequired
}
static styles = theme => ({
......@@ -251,16 +248,12 @@ class SearchBar extends React.Component {
}
getChips() {
const {query: values, domain} = this.context
const domainPrefix = domain.key + '.'
const {query: values} = this.context
return Object.keys(values).filter(key => values[key]).map(key => {
if (key === 'atoms') {
return `atoms=[${values[key].join(',')}]`
} else {
let quantityLabel = key
if (key.startsWith(domainPrefix)) {
quantityLabel = key.substring(domainPrefix.length)
}
return `${quantityLabel}=${values[key]}`
}
})
......@@ -269,10 +262,10 @@ class SearchBar extends React.Component {
static contextType = searchContext
render() {
const {classes, loading} = this.props
const {classes} = this.props
const {response: {pagination, statistics}, query, domain} = this.context
let helperText = <span>loading ...</span>
let helperText = ''
if (pagination && statistics) {
if (pagination.total === 0) {
helperText = <span>There are no more entries matching your criteria.</span>
......@@ -280,14 +273,14 @@ class SearchBar extends React.Component {
helperText = <span>
There {pagination.total === 1 ? 'is' : 'are'} {Object.keys(domain.searchMetrics).filter(key => statistics.total.all[key]).map(key => {
return <span key={key}>
{domain.searchMetrics[key].renderResultString(!loading ? statistics.total.all[key] : '...')}
{domain.searchMetrics[key].renderResultString(statistics.total.all[key])}
</span>
})}{Object.keys(query).length ? ' left' : ''}.
</span>
}
}
const showClearButton = query && Object.keys(query).filter(key => query[key] !== undefined).length > 1
const showClearButton = query && Object.keys(query).find(key => query[key] !== undefined)
return (
<div className={classes.root}>
......@@ -340,4 +333,4 @@ class SearchBar extends React.Component {
}
}
export default compose(withApi(false, false), withStyles(SearchBar.styles))(SearchBar)
export default withStyles(SearchBar.styles)(SearchBar)
......@@ -9,6 +9,9 @@ import { useLocation, useHistory } from 'react-router-dom'
import qs from 'qs'
import * as searchQuantities from '../../searchQuantities.json'
/**
* A custom hook that reads and writes search parameters from the current URL.
*/
const useSearchUrlQuery = () => {
const location = useLocation()
const history = useHistory()
......@@ -23,10 +26,12 @@ const useSearchUrlQuery = () => {
if (searchQuery.only_atoms && !Array.isArray(searchQuery.only_atoms)) {
searchQuery.only_atoms = [searchQuery.only_atoms]
}
const setUrlQuery = query => history.push(location.pathname + '?' + qs.stringify({
...rest,
...query
}, {indices: false}))
const setUrlQuery = query => {
history.push(location.pathname + '?' + qs.stringify({
...rest,
...objectFilter(query, key => query[key])
}, {indices: false}))
}
return [searchQuery, setUrlQuery]
}
......@@ -66,8 +71,6 @@ export default function SearchContext({initialRequest, initialQuery, query, chil
const {api} = useContext(apiContext)
const {raiseError} = useContext(errorContext)
const [urlQuery, setUrlQuery] = useSearchUrlQuery()
// React calls the children effects for the parent effect. But the parent effect is
// run with the state of the last render, which is the state before the children effects.
// If we would maintain the request in regular React state, we might execute unnecessary
......@@ -81,10 +84,6 @@ export default function SearchContext({initialRequest, initialQuery, query, chil
// lower the amount of state changes.
// The second ref keeps a hash over the last request that was send to the API.
// This is used to verify if a new request is actually necessary.
// The state requestHash is used to trigger the effect that will execute the request
// if necessary. Thus any requests are only send by effects on this context component.
// If we would send the requests in child effects, we would send unnecessary requests
// if two children change the request in the same render cycle.
const requestRef = useRef({
metric: domains.dft.defaultSearchMetric,
statistics: [],
......@@ -97,15 +96,20 @@ export default function SearchContext({initialRequest, initialQuery, query, chil
order: -1,
order_by: 'upload_time'
},
statisticsToRefresh: [],
query: {},
update: 0
})
const requestHashRef = useRef(0)
const [requestHash, setRequestHash] = useState(0)
const lastRequestHashRef = useRef(0)
// We use proper React state to maintain the last response from the API.
const [response, setResponse] = useState(emptyResponse)
const [statisticsToRefresh, setStatisticsToRefresh] = useState([]) // TODO
// We use a custom hook to read/write search parameters from the current URL.
const [urlQuery, setUrlQuery] = useSearchUrlQuery()
// We set the current query. This will be used by an effect to potentially call the
// API after rendering.
requestRef.current.query = urlQuery
// This is a callback that executes the current request in requestRef without any
// checks for necessity. It will update the response state, once the request has
......@@ -128,71 +132,52 @@ export default function SearchContext({initialRequest, initialQuery, query, chil
...requestRef.current.query,
...query
}
api.search(apiQuery, statisticsToRefresh)
api.search(apiQuery, requestRef.current.statisticsToRefresh)
.then(newResponse => {
setResponse({...emptyResponse, ...newResponse, metric: metric})
}).catch(error => {
setResponse({...emptyResponse, metric: metric})
raiseError(error)
})
}, [requestRef, setResponse, statisticsToRefresh, api])
// This callback will update the requestHash state. This will trigger an effect if
// the new hash is different from the last.
// This callback should be called after the requestRef was changed.
const onRequestChange = useCallback(
() => {
setRequestHash(hash(requestRef.current))
}, [requestRef, setRequestHash]
)
// This callback increased the update counter in requestRef therefore causes to re-run
// the request, even if no parameter have changed. This can be used by children to
// refresh the search results.
const update = useCallback(() => {
requestRef.current.update = requestRef.current.update + 1
onRequestChange()
}, [onRequestChange, requestRef])
}, [requestRef, setResponse, api])
// The following are various callbacks that can be used by children to update the
// request and implicitly trigger a search request to the API.
// request and implicitly trigger a search request to the API. The implicit triggering
// is realised that all changes to the request are accompanied by updates to the URL
// which is used to hold the whole request state. Each push to the history will rerender
// everything and therefore trigger effects.
const setRequestParameters = useCallback(
changes => {
requestRef.current.pagination = {
...requestRef.current.pagination,
...changes
}
onRequestChange()
}, [onRequestChange, requestRef])
}, [requestRef])
const setDomain = useCallback(domainKey => {
requestRef.current.domainKey = domainKey || domains.dft.key
onRequestChange()
}, [onRequestChange, requestRef])
}, [requestRef])
const setOwner = useCallback(owner => {
requestRef.current.owner = owner
onRequestChange()
}, [onRequestChange, requestRef])
}, [requestRef])
const setMetric = useCallback(metric => {
requestRef.current.metric = metric || domains.dft.defaultSearchMetric
onRequestChange()
}, [onRequestChange, requestRef])
}, [requestRef])
const setStatistics = useCallback(statistics => {
requestRef.current.statistics = [...statistics, ...defaultStatistics].filter(onlyUnique)
onRequestChange()
}, [onRequestChange, requestRef])
}, [requestRef])
const setGroups = useCallback(groups => {
requestRef.current.groups = {...groups}
onRequestChange()
}, [onRequestChange, requestRef])
}, [requestRef])
const handleStatisticsToRefreshChange = statistics => {
requestRef.current.statisticsToRefresh = [...requestRef.current.statisticsToRefresh, statistics].filter(onlyUnique)
}
const handleStatisticsToRefreshChange = statistics => setStatisticsToRefresh(
[...statisticsToRefresh, statistics].filter(onlyUnique)
)
const handleQueryChange = (changes, replace) => {
if (changes.atoms && changes.atoms.length === 0) {
changes.atoms = undefined
......@@ -206,24 +191,15 @@ export default function SearchContext({initialRequest, initialQuery, query, chil
} else {
setUrlQuery({...urlQuery, ...changes})
}
onRequestChange()
}
// We check and run (if necessary) the search request after each render
useEffect(() => {
requestRef.current.query = urlQuery
onRequestChange()
}, [urlQuery, requestRef, onRequestChange])
// We initially trigger a search request on mount.
useEffect(() => {
// In some cases, especially on mount, requestHash might not be based on the
// most current requestRef and we have to recompute the hash
const newRequestHash = hash(requestRef.current)
if (requestHashRef.current !== newRequestHash) {
requestHashRef.current = newRequestHash
if (lastRequestHashRef.current !== hash(requestRef.current)) {
runRequest()
lastRequestHashRef.current = hash(requestRef.current)
}
}, [requestHashRef, requestHash, runRequest])
})
const value = {
response: response,
......@@ -247,7 +223,7 @@ export default function SearchContext({initialRequest, initialQuery, query, chil
setOwner: setOwner,
setStatisticsToRefresh: handleStatisticsToRefreshChange,
setStatistics: setStatistics,
update: update
update: runRequest
}
return <searchContext.Provider value={value} >{children}</searchContext.Provider>
......
Supports Markdown
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