UI: Avoid empty state being shown before data is fetched

When opening the builds page there is a short moment when an empty state
pattern is shown before the data is actually fetched. This happens for
the build itself ("Build not found") and for the tab contents like logs,
task summary and console information ("This build does not provide any
..."). The same applies for the buildset page on initial load.

Interestingly, this moment is extremely short in chrome (so you can't
even see it without actually pausing the state transitions with the
redux developer tools), but in firefox it's much longer so you usually
see it when opening the page.

To avoid this from happen, this change does two things.
1. It splits the build, manifest, output, hosts and errorIds into
   separate fields of the state. This allows us to fetch as much in
   parallel as possible without always waiting for the actual build to
   be present. To simplify the usage in the Build page react component,
   we use the mapStateToProps() function to combine all those values
   into a single build object.

2. Unfortunately, this is not enough as some information like the logs
   rely on the manifest entries and the build.log_url to be present. To
   still distinguish between a "unknown" value (e.g. the build or
   manifest wasn't fetched yet or is currently fetching but we don't
   know if it will be found) and a "non-existing" value (e.g. the build
   or manifest could not be found after fetching), we use the difference
   between undefined and null. In JavaScript tongue, every variable is
   undefined that hasn't assigned a value yet (though we could also
   assign undefined explicitly). The null value on the other hand is
   used to say "I define this variable and explicitly set it to an empty
   value".

   Thus, we explictly set every entity we didn't found to null in the
   reducer functions and check for (undefined || isFetching) instead to
   show the spinner animation. This way, the page always starts with the
   fetching animation and only switches to showing the result or an
   empty state depending on if the entity could be found or not.

Change-Id: I800dfe71128bb00638a51cea0d9be9be623bd923
This commit is contained in:
Felix Edel 2020-10-15 09:40:19 +02:00
parent 072bf45ff8
commit 7e131757a5
No known key found for this signature in database
GPG Key ID: E95717A102DD3030
7 changed files with 144 additions and 85 deletions

View File

@ -34,7 +34,7 @@ export const BUILD_OUTPUT_NOT_AVAILABLE = 'BUILD_OUTPUT_NOT_AVAILABLE'
export const BUILD_MANIFEST_REQUEST = 'BUILD_MANIFEST_FETCH_REQUEST'
export const BUILD_MANIFEST_SUCCESS = 'BUILD_MANIFEST_FETCH_SUCCESS'
export const BUILD_MANIFEST_FAIL = 'BUILD_MANIFEST_FETCH_FAIL'
export const BUILD_MANIFEST_NOT_AVAILBLE = 'BUILD_MANIFEST_NOT_AVAILABLE'
export const BUILD_MANIFEST_NOT_AVAILABLE = 'BUILD_MANIFEST_NOT_AVAILABLE'
export const requestBuild = () => ({
type: BUILD_FETCH_REQUEST
@ -47,10 +47,11 @@ export const receiveBuild = (buildId, build) => ({
receivedAt: Date.now()
})
const failedBuild = (error, url) => {
const failedBuild = (buildId, error, url) => {
error.url = url
return {
type: BUILD_FETCH_FAIL,
buildId,
error
}
}
@ -210,10 +211,11 @@ export const receiveBuildOutput = (buildId, output) => {
}
}
const failedBuildOutput = (error, url) => {
const failedBuildOutput = (buildId, error, url) => {
error.url = url
return {
type: BUILD_OUTPUT_FAIL,
buildId,
error
}
}
@ -245,20 +247,27 @@ export const receiveBuildManifest = (buildId, manifest) => {
}
}
const failedBuildManifest = (error, url) => {
const failedBuildManifest = (buildId, error, url) => {
error.url = url
return {
type: BUILD_MANIFEST_FAIL,
buildId,
error
}
}
function buildOutputNotAvailable() {
return { type: BUILD_OUTPUT_NOT_AVAILABLE }
function buildOutputNotAvailable(buildId) {
return {
type: BUILD_OUTPUT_NOT_AVAILABLE,
buildId: buildId,
}
}
function buildManifestNotAvailable() {
return { type: BUILD_MANIFEST_NOT_AVAILBLE }
function buildManifestNotAvailable(buildId) {
return {
type: BUILD_MANIFEST_NOT_AVAILABLE,
buildId: buildId,
}
}
export function fetchBuild(tenant, buildId, state) {
@ -276,7 +285,7 @@ export function fetchBuild(tenant, buildId, state) {
const response = await API.fetchBuild(tenant.apiPrefix, buildId)
dispatch(receiveBuild(buildId, response.data))
} catch (error) {
dispatch(failedBuild(error, tenant.apiPrefix))
dispatch(failedBuild(buildId, error, tenant.apiPrefix))
// Raise the error again, so fetchBuildAllInfo() doesn't call the
// remaining fetch methods.
throw error
@ -286,18 +295,21 @@ export function fetchBuild(tenant, buildId, state) {
function fetchBuildOutput(buildId, state) {
return async function (dispatch) {
// In case the value is already set in our local state, directly resolve the
// promise. A null value means that the output could not be found for this
// build id.
if (state.build.outputs[buildId] !== undefined) {
return Promise.resolve()
}
// As this function is only called after fetchBuild() we can assume that
// the build is in the state. Otherwise an error would have been thrown and
// this function wouldn't be called.
const build = state.build.builds[buildId]
if (build.output) {
return Promise.resolve()
}
if (!build.log_url) {
// Don't treat a missing log URL as failure as we don't want to show a
// toast for that. The UI already informs about the missing log URL in
// multiple places.
return dispatch(buildOutputNotAvailable())
return dispatch(buildOutputNotAvailable(buildId))
}
const url = build.log_url.substr(0, build.log_url.lastIndexOf('/') + 1)
dispatch(requestBuildOutput())
@ -306,7 +318,7 @@ function fetchBuildOutput(buildId, state) {
dispatch(receiveBuildOutput(buildId, response.data))
} catch (error) {
if (!error.request) {
dispatch(failedBuildOutput(error, url))
dispatch(failedBuildOutput(buildId, error, url))
// Raise the error again, so fetchBuildAllInfo() doesn't call the
// remaining fetch methods.
throw error
@ -316,7 +328,7 @@ function fetchBuildOutput(buildId, state) {
const response = await Axios.get(url + 'job-output.json')
dispatch(receiveBuildOutput(buildId, response.data))
} catch (error) {
dispatch(failedBuildOutput(error, url))
dispatch(failedBuildOutput(buildId, error, url))
// Raise the error again, so fetchBuildAllInfo() doesn't call the
// remaining fetch methods.
throw error
@ -327,13 +339,16 @@ function fetchBuildOutput(buildId, state) {
export function fetchBuildManifest(buildId, state) {
return async function(dispatch) {
// In case the value is already set in our local state, directly resolve the
// promise. A null value means that the manifest could not be found for this
// build id.
if (state.build.manifests[buildId] !== undefined) {
return Promise.resolve()
}
// As this function is only called after fetchBuild() we can assume that
// the build is in the state. Otherwise an error would have been thrown and
// this function wouldn't be called.
const build = state.build.builds[buildId]
if (build.manifest) {
return Promise.resolve()
}
dispatch(requestBuildManifest())
for (let artifact of build.artifacts) {
if (
@ -345,7 +360,7 @@ export function fetchBuildManifest(buildId, state) {
const response = await Axios.get(artifact.url)
return dispatch(receiveBuildManifest(buildId, response.data))
} catch(error) {
dispatch(failedBuildManifest(error, artifact.url))
dispatch(failedBuildManifest(buildId, error, artifact.url))
// Raise the error again, so fetchBuildAllInfo() doesn't call
// fetchLogFile which needs an existing manifest file.
throw error
@ -354,7 +369,7 @@ export function fetchBuildManifest(buildId, state) {
}
// Don't treat a missing manifest file as failure as we don't want to show a
// toast for that.
dispatch(buildManifestNotAvailable())
dispatch(buildManifestNotAvailable(buildId))
}
}
@ -363,19 +378,18 @@ export function fetchBuildAllInfo(tenant, buildId, logfileName) {
// this is the common use case we have when loading the build info.
return async function (dispatch, getState) {
try {
// Wait for the build info to be available and provide the current status
// to the fetchBuildOutput and fetchBuildManifest so they can get the log
// url from the fetched build.
// Wait for the build to be available as fetchBuildOutput and
// fetchBuildManifest require information from the build object.
await dispatch(fetchBuild(tenant, buildId, getState()))
dispatch(fetchBuildOutput(buildId, getState()))
// Wait for the manifest info to be available as this is needed in case
// we also download a logfile.
await dispatch(fetchBuildManifest(buildId, getState()))
dispatch(fetchBuildOutput(buildId, getState()))
if (logfileName) {
dispatch(fetchLogfile(buildId, logfileName, getState()))
}
} catch (error) {
dispatch(failedBuild(error, tenant.apiPrefix))
dispatch(failedBuild(buildId, error, tenant.apiPrefix))
}
}
}
@ -391,8 +405,9 @@ export const receiveBuildset = (buildsetId, buildset) => ({
receivedAt: Date.now()
})
const failedBuildset = error => ({
const failedBuildset = (buildsetId, error) => ({
type: BUILDSET_FETCH_FAIL,
buildsetId,
error
})
@ -403,7 +418,7 @@ export function fetchBuildset(tenant, buildsetId) {
const response = await API.fetchBuildset(tenant.apiPrefix, buildsetId)
dispatch(receiveBuildset(buildsetId, response.data))
} catch (error) {
dispatch(failedBuildset(error))
dispatch(failedBuildset(buildsetId, error))
}
}
}

View File

@ -80,13 +80,19 @@ const failedLogfile = (error, url) => {
}
export function fetchLogfile(buildId, file, state) {
return async function(dispatch) {
const build = state.build.builds[buildId]
return async function (dispatch) {
// Don't do anything if the logfile is already part of our local state
if (buildId in state.logfile.files && file in state.logfile.files[buildId]) {
if (
buildId in state.logfile.files &&
file in state.logfile.files[buildId]
) {
return Promise.resolve()
}
const item = build.manifest.index['/' + file]
// Since this method is only called after fetchBuild() and fetchManifest(),
// we can assume both are there.
const build = state.build.builds[buildId]
const manifest = state.build.manifests[buildId]
const item = manifest.index['/' + file]
if (!item) {
return dispatch(

View File

@ -138,10 +138,13 @@ class BuildPage extends React.Component {
// Get the logfile from react-routers URL parameters
const logfileName = this.props.match.params.file
if (!build && isFetching) {
// In case the build is not available yet (before the fetching started) or
// is currently fetching.
if (build === undefined || isFetching) {
return <Fetching />
}
// The build is null, meaning it couldn't be found.
if (!build) {
return (
<EmptyPage
@ -158,7 +161,7 @@ class BuildPage extends React.Component {
)
const resultsTabContent =
!build.hosts && isFetchingOutput ? (
build.hosts === undefined || isFetchingOutput ? (
<Fetching />
) : build.hosts ? (
<BuildOutput output={build.hosts} />
@ -183,7 +186,7 @@ class BuildPage extends React.Component {
)
let logsTabContent = null
if (!build.manifest && isFetchingManifest) {
if (build.manifest === undefined || isFetchingManifest) {
logsTabContent = <Fetching />
} else if (logfileName) {
logsTabContent = (
@ -213,7 +216,7 @@ class BuildPage extends React.Component {
}
const consoleTabContent =
!build.output && isFetchingOutput ? (
build.output === undefined || isFetchingOutput ? (
<Fetching />
) : build.output ? (
<Console
@ -307,15 +310,28 @@ class BuildPage extends React.Component {
function mapStateToProps(state, ownProps) {
const buildId = ownProps.match.params.buildId
const build =
buildId && Object.keys(state.build.builds).length > 0
? state.build.builds[buildId]
: null
// JavaScript will return undefined in case the key is missing in the
// dict/object.
const buildFromState = state.build.builds[buildId]
// Only copy the build if it's a valid object. In case it is null or undefined
// directly assign the value. The cloning is necessary as we mutate the build
// in the next step when adding the manifest, output, ... information.
const build = buildFromState ? { ...buildFromState } : buildFromState
// If the build is available, extend it with more information. All those
// values will be undefined if they are not part of the dict/object.
if (build) {
build.manifest = state.build.manifests[buildId]
build.output = state.build.outputs[buildId]
build.hosts = state.build.hosts[buildId]
build.errorIds = state.build.errorIds[buildId]
}
const logfileName = ownProps.match.params.file
const logfile =
logfileName && Object.keys(state.logfile.files).length > 0
buildId in state.logfile.files
? state.logfile.files[buildId][logfileName]
: null
: undefined
return {
build,
logfile,

View File

@ -66,7 +66,7 @@ class BuildsetPage extends React.Component {
const { buildset, isFetching, tenant } = this.props
// Initial page load
if (!buildset && isFetching) {
if (buildset === undefined || isFetching) {
return <Fetching />
}
@ -131,10 +131,9 @@ class BuildsetPage extends React.Component {
function mapStateToProps(state, ownProps) {
const buildsetId = ownProps.match.params.buildsetId
const buildset =
buildsetId && Object.keys(state.build.buildsets).length > 0
? state.build.buildsets[buildsetId]
: null
// This will be undefined if the data is not available yet or null if no
// buildset could be fetched for the given id.
const buildset = state.build.buildsets[buildsetId]
return {
buildset,
tenant: state.tenant,

View File

@ -22,9 +22,11 @@ import {
BUILD_OUTPUT_FAIL,
BUILD_OUTPUT_REQUEST,
BUILD_OUTPUT_SUCCESS,
BUILD_OUTPUT_NOT_AVAILABLE,
BUILD_MANIFEST_FAIL,
BUILD_MANIFEST_REQUEST,
BUILD_MANIFEST_SUCCESS,
BUILD_MANIFEST_NOT_AVAILABLE,
} from '../actions/build'
import initialState from './initialState'
@ -47,38 +49,51 @@ export default (state = initialState.build, action) => {
isFetching: false,
}
case BUILD_FETCH_FAIL:
return {
...state,
builds: { ...state.builds, [action.buildId]: null },
isFetching: false,
}
case BUILDSET_FETCH_FAIL:
return { ...state, isFetching: false }
return {
...state,
buildsets: { ...state.buildsets, [action.buildsetId]: null },
isFetching: false,
}
case BUILD_OUTPUT_REQUEST:
return { ...state, isFetchingOutput: true }
case BUILD_OUTPUT_SUCCESS: {
const buildsWithOutput = {
...state.builds,
[action.buildId]: {
...state.builds[action.buildId],
errorIds: action.errorIds,
hosts: action.hosts,
output: action.output,
},
case BUILD_OUTPUT_SUCCESS:
return {
...state,
outputs: { ...state.outputs, [action.buildId]: action.output },
errorIds: { ...state.errorIds, [action.buildId]: action.errorIds },
hosts: { ...state.hosts, [action.buildId]: action.hosts },
isFetchingOutput: false,
}
return { ...state, builds: buildsWithOutput, isFetchingOutput: false }
}
case BUILD_OUTPUT_FAIL:
return { ...state, isFetchingOutput: false }
case BUILD_OUTPUT_NOT_AVAILABLE:
return {
...state,
outputs: { ...state.outputs, [action.buildId]: null },
errorIds: { ...state.errorIds, [action.buildId]: null },
hosts: { ...state.hosts, [action.buildId]: null },
isFetchingOutput: false,
}
case BUILD_MANIFEST_REQUEST:
return { ...state, isFetchingManifest: true }
case BUILD_MANIFEST_SUCCESS: {
const buildsWithManifest = {
...state.builds,
[action.buildId]: {
...state.builds[action.buildId],
manifest: action.manifest,
},
case BUILD_MANIFEST_SUCCESS:
return {
...state,
manifests: { ...state.manifests, [action.buildId]: action.manifest },
isFetchingManifest: false,
}
return { ...state, builds: buildsWithManifest, isFetchingManifest: false }
}
case BUILD_MANIFEST_FAIL:
return { ...state, isFetchingManifest: false }
case BUILD_MANIFEST_NOT_AVAILABLE:
return {
...state,
manifests: { ...state.manifests, [action.buildId]: null },
isFetchingManifest: false,
}
default:
return state
}

View File

@ -2,6 +2,16 @@ export default {
build: {
builds: {},
buildsets: {},
// Store outputs, manifest, hosts and errorIds separately from the build.
// This allows us to fetch everything in parallel and we don't have to wait
// until the build is available. We also don't have to update the actual
// build object with new information everytime.
// To simplify the usage we can map everything into a single build object
// in the mapStateToProps() function in the build page.
outputs: {},
manifests: {},
hosts: {},
errorIds: {},
isFetching: false,
isFetchingOutput: false,
isFetchingManifest: false,

View File

@ -18,7 +18,7 @@ it('should fetch a build', () => {
expect(fetchedBuild).toEqual(build)
})
it('should fetch output and update the build in the store', () => {
it('should fetch an output', () => {
const store = createStore(rootReducer, initialState)
const build = {
uuid: '1234',
@ -93,14 +93,13 @@ it('should fetch output and update the build in the store', () => {
},
]
// Fetch the build
store.dispatch(buildActions.receiveBuild(build.uuid, build))
// Fetch the output
store.dispatch(buildActions.receiveBuildOutput(build.uuid, output))
const newState = store.getState()
expect(Object.keys(newState.build.builds).length).toEqual(1)
expect(Object.keys(newState.build.outputs).length).toEqual(1)
expect(Object.keys(newState.build.hosts).length).toEqual(1)
expect(Object.keys(newState.build.errorIds).length).toEqual(1)
const expectedHosts = {
localhost: {
@ -125,13 +124,15 @@ it('should fetch output and update the build in the store', () => {
},
}
const fetchedBuild = newState.build.builds[build.uuid]
expect(fetchedBuild.errorIds).toEqual(new Set())
expect(fetchedBuild.hosts).toEqual(expectedHosts)
expect(fetchedBuild.output).toEqual(output)
const fetchedOutput = newState.build.outputs[build.uuid]
const fetchedHosts = newState.build.hosts[build.uuid]
const fetchedErrorIds = newState.build.errorIds[build.uuid]
expect(fetchedOutput).toEqual(output)
expect(fetchedHosts).toEqual(expectedHosts)
expect(fetchedErrorIds).toEqual(new Set())
})
it('should fetch manifest and update the build in the store', () => {
it('should fetch a manifest file', () => {
const store = createStore(rootReducer, initialState)
const build = {
uuid: '1234',
@ -184,14 +185,11 @@ it('should fetch manifest and update the build in the store', () => {
],
}
// Fetch the build
store.dispatch(buildActions.receiveBuild(build.uuid, build))
// Fetch the manifest
store.dispatch(buildActions.receiveBuildManifest(build.uuid, manifest))
const newState = store.getState()
expect(Object.keys(newState.build.builds).length).toEqual(1)
expect(Object.keys(newState.build.manifests).length).toEqual(1)
const expectedManifestIndex = {
'/zuul-info/host-info.ubuntu-bionic.yaml': {
@ -231,8 +229,8 @@ it('should fetch manifest and update the build in the store', () => {
},
}
const fetchedBuild = newState.build.builds[build.uuid]
expect(fetchedBuild.manifest).toEqual({
const fetchedManifest = newState.build.manifests[build.uuid]
expect(fetchedManifest).toEqual({
index: expectedManifestIndex,
tree: manifest.tree,
})