From 949a7295c3b578c36c22b44e5db0c018008ceb8f Mon Sep 17 00:00:00 2001 From: Jamey H Date: Fri, 3 Nov 2023 15:00:08 -0400 Subject: [PATCH 1/2] feat(app): add download progress to robot update flows If the robot update cache files are still downloading, no longer is an error thrown. Instead, the update modal renders and a downloading stage of the update flow begins. Additionally, release files are downloaded and returned to the browser layer separately from system files, so users may see release notes as soon as they are downloaded. --- app-shell/src/http.ts | 2 +- app-shell/src/robot-update/index.ts | 34 ++-- app-shell/src/robot-update/release-files.ts | 79 +++++++--- .../AdvancedTab/UpdateRobotSoftware.tsx | 5 + .../RobotUpdateProgressModal.tsx | 69 ++------- .../RobotUpdateProgressModal.test.tsx | 17 +- .../__tests__/useRobotUpdateInfo.test.tsx | 146 +++++++++++++----- .../UpdateBuildroot/useRobotUpdateInfo.ts | 116 ++++++-------- .../redux/robot-update/__tests__/epic.test.ts | 40 +++++ .../robot-update/__tests__/reducer.test.ts | 63 ++++++++ app/src/redux/robot-update/constants.ts | 7 + app/src/redux/robot-update/epic.ts | 17 +- app/src/redux/robot-update/hooks.ts | 1 + app/src/redux/robot-update/reducer.ts | 42 ++++- app/src/redux/robot-update/types.ts | 6 +- 15 files changed, 447 insertions(+), 197 deletions(-) diff --git a/app-shell/src/http.ts b/app-shell/src/http.ts index 8efbf9dc2b2..c4ce12d4aa6 100644 --- a/app-shell/src/http.ts +++ b/app-shell/src/http.ts @@ -125,10 +125,10 @@ function createReadStreamWithSize( } readStream.once('error', handleError) - readStream.on('data', onData) function handleSuccess(): void { resolve(readStream) + readStream.removeListener('error', handleError) } function handleError(error: Error): void { diff --git a/app-shell/src/robot-update/index.ts b/app-shell/src/robot-update/index.ts index b71eab6d44b..4f4d2bc8350 100644 --- a/app-shell/src/robot-update/index.ts +++ b/app-shell/src/robot-update/index.ts @@ -137,20 +137,28 @@ export function registerRobotUpdate(dispatch: Dispatch): Dispatch { case 'robotUpdate:READ_USER_FILE': { const { systemFile } = action.payload as { systemFile: string } - readFileAndDispatchInfo(dispatch, systemFile, true) - break + return readFileAndDispatchInfo(dispatch, systemFile, true) } case 'robotUpdate:READ_SYSTEM_FILE': { const { target } = action.payload const filename = updateSet[target]?.system + if (filename == null) { - return dispatch({ - type: 'robotUpdate:UNEXPECTED_ERROR', - payload: { message: 'Robot update file not downloaded' }, - }) + if (checkingForUpdates) { + dispatch({ + type: 'robotUpdate:CHECKING_FOR_UPDATE', + payload: target, + }) + } else { + // If the file was downloaded but deleted from robot-update-cache. + dispatch({ + type: 'robotUpdate:UNEXPECTED_ERROR', + payload: { message: 'Robot update file not downloaded' }, + }) + } } else { - readFileAndDispatchInfo(dispatch, filename) + return readFileAndDispatchInfo(dispatch, filename) } } } @@ -213,7 +221,7 @@ export function checkForRobotUpdate( const handleProgress = (progress: DownloadProgress): void => { const { downloaded, size } = progress if (size !== null) { - const percentDone = Math.round(downloaded / size) * 100 + const percentDone = Math.round((downloaded / size) * 100) if (percentDone - prevPercentDone > 0) { dispatch({ type: 'robotUpdate:DOWNLOAD_PROGRESS', @@ -227,7 +235,15 @@ export function checkForRobotUpdate( const targetDownloadDir = cacheDirForMachineFiles(target) return ensureDir(targetDownloadDir) - .then(() => getReleaseFiles(urls, targetDownloadDir, handleProgress)) + .then(() => + getReleaseFiles( + urls, + targetDownloadDir, + dispatch, + target, + handleProgress + ) + ) .then(filepaths => cacheUpdateSet(filepaths, target)) .then(updateInfo => dispatch({ type: 'robotUpdate:UPDATE_INFO', payload: updateInfo }) diff --git a/app-shell/src/robot-update/release-files.ts b/app-shell/src/robot-update/release-files.ts index d2d9d6b47cc..0c84634eb59 100644 --- a/app-shell/src/robot-update/release-files.ts +++ b/app-shell/src/robot-update/release-files.ts @@ -3,12 +3,17 @@ import assert from 'assert' import path from 'path' import { promisify } from 'util' import tempy from 'tempy' -import { move, readdir, remove } from 'fs-extra' +import { move, readdir, remove, readFile } from 'fs-extra' import StreamZip from 'node-stream-zip' import getStream from 'get-stream' +import { RobotUpdateTarget } from '@opentrons/app/src/redux/robot-update/types' + import { createLogger } from '../log' import { fetchToFile } from '../http' +import { Dispatch } from '../types' +import { CURRENT_VERSION } from '../update' + import type { DownloadProgress } from '../http' import type { ReleaseSetUrls, ReleaseSetFilepaths, UserFileInfo } from './types' @@ -23,6 +28,8 @@ const outPath = (dir: string, url: string): string => export function getReleaseFiles( urls: ReleaseSetUrls, directory: string, + dispatch: Dispatch, + target: RobotUpdateTarget, onProgress: (progress: DownloadProgress) => unknown ): Promise { return readdir(directory) @@ -44,41 +51,65 @@ export function getReleaseFiles( return { system, releaseNotes } } - return downloadReleaseFiles(urls, directory, onProgress) + return Promise.all([ + downloadAndNotify(true, urls.releaseNotes, directory, dispatch, target), + downloadAndNotify( + false, + urls.system, + directory, + dispatch, + target, + onProgress + ), + ]).then(([releaseNotes, system]) => ({ releaseNotes, system })) }) } -// downloads the entire release set to a temporary directory, and once they're -// all successfully downloaded, renames the directory to `directory` +// downloads robot update files to a temporary directory, and once +// successfully downloaded, renames the directory to `directory` // TODO(mc, 2019-07-09): DRY this up if/when more than 2 files are required -export function downloadReleaseFiles( - urls: ReleaseSetUrls, +export function downloadAndNotify( + isReleaseNotesDownload: boolean, + url: ReleaseSetUrls['releaseNotes' | 'system'], directory: string, + dispatch: Dispatch, + target: RobotUpdateTarget, // `onProgress` will be called with download progress as the files are read - onProgress: (progress: DownloadProgress) => unknown -): Promise { + onProgress?: (progress: DownloadProgress) => unknown +): Promise { const tempDir: string = tempy.directory() - const tempSystemPath = outPath(tempDir, urls.system) - const tempNotesPath = outPath(tempDir, urls.releaseNotes) + const tempPath = outPath(tempDir, url) + const path = outPath(directory, tempPath) + const logMessage = isReleaseNotesDownload ? 'release notes' : 'system files' - log.debug('directory created for robot update downloads', { tempDir }) + log.debug('directory created for ' + logMessage, { tempDir }) // downloads are streamed directly to the filesystem to avoid loading them // all into memory simultaneously - const systemReq = fetchToFile(urls.system, tempSystemPath, { onProgress }) - const notesReq = fetchToFile(urls.releaseNotes, tempNotesPath) - - return Promise.all([systemReq, notesReq]).then(results => { - const [systemTemp, releaseNotesTemp] = results - const systemPath = outPath(directory, systemTemp) - const notesPath = outPath(directory, releaseNotesTemp) - - log.debug('renaming directory', { from: tempDir, to: directory }) + const req = fetchToFile(url, tempPath, { + onProgress, + }) - return move(tempDir, directory, { overwrite: true }).then(() => ({ - system: systemPath, - releaseNotes: notesPath, - })) + return req.then(() => { + return move(tempPath, path, { overwrite: true }) + .then(() => { + if (isReleaseNotesDownload) { + return readFile(path, 'utf8').then(releaseNotes => + dispatch({ + type: 'robotUpdate:UPDATE_INFO', + payload: { releaseNotes, target, version: CURRENT_VERSION }, + }) + ) + } + // This action will only have an effect if the user is actively waiting for the download to complete. + else { + return dispatch({ + type: 'robotUpdate:DOWNLOAD_DONE', + payload: target, + }) + } + }) + .then(() => path) }) } diff --git a/app/src/organisms/Devices/RobotSettings/AdvancedTab/UpdateRobotSoftware.tsx b/app/src/organisms/Devices/RobotSettings/AdvancedTab/UpdateRobotSoftware.tsx index 980d4c4f791..81087ef6e1b 100644 --- a/app/src/organisms/Devices/RobotSettings/AdvancedTab/UpdateRobotSoftware.tsx +++ b/app/src/organisms/Devices/RobotSettings/AdvancedTab/UpdateRobotSoftware.tsx @@ -55,6 +55,11 @@ export function UpdateRobotSoftware({ dispatchStartRobotUpdate(robotName, files[0].path) onUpdateStart() } + // this is to reset the state of the file picker so users can reselect the same + // system image if the upload fails + if (inputRef.current?.value != null) { + inputRef.current.value = '' + } } const handleClick: React.MouseEventHandler = () => { diff --git a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx index 136af1d856f..d951870dcd6 100644 --- a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx +++ b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx @@ -6,9 +6,7 @@ import { css } from 'styled-components' import { Flex, Icon, - Link, NewPrimaryBtn, - NewSecondaryBtn, JUSTIFY_FLEX_END, ALIGN_CENTER, COLORS, @@ -25,14 +23,13 @@ import { FOOTER_BUTTON_STYLE } from './UpdateRobotModal' import { startRobotUpdate, clearRobotUpdateSession, - getRobotSessionIsManualFile, + getRobotUpdateDownloadError, } from '../../../../redux/robot-update' -import { useDispatchStartRobotUpdate } from '../../../../redux/robot-update/hooks' import { useRobotUpdateInfo } from './useRobotUpdateInfo' import successIcon from '../../../../assets/images/icon_success.png' -import type { SetStatusBarCreateCommand } from '@opentrons/shared-data' import type { State } from '../../../../redux/types' +import type { SetStatusBarCreateCommand } from '@opentrons/shared-data/protocol' import type { RobotUpdateSession } from '../../../../redux/robot-update/types' import type { UpdateStep } from './useRobotUpdateInfo' @@ -47,10 +44,6 @@ const UPDATE_TEXT_STYLE = css` color: ${COLORS.darkGreyEnabled}; font-size: 0.8rem; ` -const TRY_RESTART_STYLE = css` - color: ${COLORS.blueEnabled}; - font-size: 0.8rem; -` const HIDDEN_CSS = css` position: fixed; clip: rect(1px 1px 1px 1px); @@ -71,19 +64,19 @@ export function RobotUpdateProgressModal({ const { t } = useTranslation('device_settings') const [showFileSelect, setShowFileSelect] = React.useState(false) const installFromFileRef = React.useRef(null) - const dispatchStartRobotUpdate = useDispatchStartRobotUpdate() - const manualFileUsedForUpdate = useSelector((state: State) => - getRobotSessionIsManualFile(state) - ) + const completeRobotUpdateHandler = (): void => { if (closeUpdateBuildroot != null) closeUpdateBuildroot() } - const reinstallUpdate = React.useCallback(() => { - dispatchStartRobotUpdate(robotName) - }, [robotName]) - const { error } = session || { error: null } - const { updateStep, progressPercent } = useRobotUpdateInfo(session) + const { updateStep, progressPercent } = useRobotUpdateInfo(robotName, session) + + let { error } = session || { error: null } + const downloadError = useSelector((state: State) => + getRobotUpdateDownloadError(state, robotName) + ) + if (error == null && downloadError != null) error = downloadError + useStatusBarAnimation(error != null) useCleanupRobotUpdateSessionOnDismount() @@ -105,7 +98,9 @@ export function RobotUpdateProgressModal({ progressPercent ) - let modalBodyText = t('installing_update') + let modalBodyText = '' + if (updateStep === 'download') modalBodyText = t('downloading_update') + if (updateStep === 'install') modalBodyText = t('installing_update') let subProgressBarText = t('do_not_turn_off') if (updateStep === 'restart') modalBodyText = t('restarting_robot') if (updateStep === 'restart' && letUserExitUpdate) { @@ -125,9 +120,6 @@ export function RobotUpdateProgressModal({ footer={ hasStoppedUpdating ? ( ) : null @@ -151,17 +143,7 @@ export function RobotUpdateProgressModal({ {letUserExitUpdate && updateStep !== 'restart' ? ( <> - {t('problem_during_update')}{' '} - setShowFileSelect(true) - } - > - {t('try_restarting_the_update')} - + {t('problem_during_update')} {t('try_restarting_the_update')} {showFileSelect && ( void - errorMessage?: string | null closeUpdateBuildroot?: () => void } function RobotUpdateProgressFooter({ - robotName, - installRobotUpdate, - errorMessage, closeUpdateBuildroot, }: RobotUpdateProgressFooterProps): JSX.Element { const { t } = useTranslation('device_settings') - const installUpdate = React.useCallback(() => { - installRobotUpdate(robotName) - }, [robotName]) return ( - {errorMessage && ( - - {t('try_again')} - - )} ( diff --git a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/RobotUpdateProgressModal.test.tsx b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/RobotUpdateProgressModal.test.tsx index 6f5c99566d3..55793ef48dc 100644 --- a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/RobotUpdateProgressModal.test.tsx +++ b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/RobotUpdateProgressModal.test.tsx @@ -8,7 +8,10 @@ import { TIME_BEFORE_ALLOWING_EXIT_MS, } from '../RobotUpdateProgressModal' import { useRobotUpdateInfo } from '../useRobotUpdateInfo' -import { getRobotSessionIsManualFile } from '../../../../../redux/robot-update' +import { + getRobotSessionIsManualFile, + getRobotUpdateDownloadError, +} from '../../../../../redux/robot-update' import { useDispatchStartRobotUpdate } from '../../../../../redux/robot-update/hooks' import type { SetStatusBarCreateCommand } from '@opentrons/shared-data' @@ -31,6 +34,9 @@ const mockGetRobotSessionIsManualFile = getRobotSessionIsManualFile as jest.Mock const mockUseDispatchStartRobotUpdate = useDispatchStartRobotUpdate as jest.MockedFunction< typeof useDispatchStartRobotUpdate > +const mockGetRobotUpdateDownloadError = getRobotUpdateDownloadError as jest.MockedFunction< + typeof getRobotUpdateDownloadError +> const render = ( props: React.ComponentProps @@ -71,12 +77,20 @@ describe('DownloadUpdateModal', () => { }) mockGetRobotSessionIsManualFile.mockReturnValue(false) mockUseDispatchStartRobotUpdate.mockReturnValue(jest.fn) + mockGetRobotUpdateDownloadError.mockReturnValue(null) }) afterEach(() => { jest.resetAllMocks() }) + it('renders robot update download errors', () => { + mockGetRobotUpdateDownloadError.mockReturnValue('test download error') + + const [{ getByText }] = render(props) + getByText('test download error') + }) + it('renders the robot name as a part of the header', () => { const [{ getByText }] = render(props) @@ -157,7 +171,6 @@ describe('DownloadUpdateModal', () => { expect(getByText('test error')).toBeInTheDocument() fireEvent.click(exitButton) - expect(getByText('Try again')).toBeInTheDocument() expect(props.closeUpdateBuildroot).toHaveBeenCalled() expect(mockUseCreateLiveCommandMutation).toBeCalledWith() diff --git a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/useRobotUpdateInfo.test.tsx b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/useRobotUpdateInfo.test.tsx index 452cfd7b8dd..f1fd45669fc 100644 --- a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/useRobotUpdateInfo.test.tsx +++ b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/__tests__/useRobotUpdateInfo.test.tsx @@ -1,14 +1,34 @@ +import * as React from 'react' import { renderHook } from '@testing-library/react-hooks' +import { createStore } from 'redux' +import { I18nextProvider } from 'react-i18next' +import { Provider } from 'react-redux' + +import { i18n } from '../../../../../i18n' import { useRobotUpdateInfo } from '../useRobotUpdateInfo' +import { getRobotUpdateDownloadProgress } from '../../../../../redux/robot-update' + +import type { Store } from 'redux' +import { State } from '../../../../../redux/types' import type { RobotUpdateSession, UpdateSessionStep, UpdateSessionStage, } from '../../../../../redux/robot-update/types' +jest.mock('../../../../../redux/robot-update') + +const mockGetRobotUpdateDownloadProgress = getRobotUpdateDownloadProgress as jest.MockedFunction< + typeof getRobotUpdateDownloadProgress +> + describe('useRobotUpdateInfo', () => { + let store: Store + let wrapper: React.FunctionComponent<{}> + + const MOCK_ROBOT_NAME = 'testRobot' const mockRobotUpdateSession: RobotUpdateSession | null = { - robotName: 'testRobot', + robotName: MOCK_ROBOT_NAME, fileInfo: { isManualFile: true, systemFile: 'testFile', version: '1.0.0' }, token: null, pathPrefix: null, @@ -18,36 +38,79 @@ describe('useRobotUpdateInfo', () => { error: null, } - it('should return initial values when session is null', () => { - const { result } = renderHook(() => useRobotUpdateInfo(null)) + beforeEach(() => { + jest.useFakeTimers() + store = createStore(jest.fn(), {}) + store.dispatch = jest.fn() + wrapper = ({ children }) => ( + + {children} + + ) + mockGetRobotUpdateDownloadProgress.mockReturnValue(50) + }) - expect(result.current.updateStep).toBe('initial') + it('should return null when session is null', () => { + const { result } = renderHook( + () => useRobotUpdateInfo(MOCK_ROBOT_NAME, null), + { wrapper } + ) + + expect(result.current.updateStep).toBe(null) expect(result.current.progressPercent).toBe(0) }) it('should return initial values when there is no session step and stage', () => { - const { result } = renderHook(session => useRobotUpdateInfo(session), { - initialProps: { - ...mockRobotUpdateSession, - step: null, - stage: null, - }, - }) + const { result } = renderHook( + session => useRobotUpdateInfo(MOCK_ROBOT_NAME, session), + { + initialProps: { + ...mockRobotUpdateSession, + step: null, + stage: null, + } as any, + wrapper, + } + ) expect(result.current.updateStep).toBe('initial') expect(result.current.progressPercent).toBe(0) }) + it('should return download updateStep and appropriate percentages when the update is downloading', () => { + const { result, rerender } = renderHook( + session => useRobotUpdateInfo(MOCK_ROBOT_NAME, session), + { + initialProps: { + ...mockRobotUpdateSession, + step: 'downloadFile', + } as any, + wrapper, + } + ) + + expect(result.current.updateStep).toBe('download') + expect(Math.round(result.current.progressPercent)).toBe(17) + + rerender({ + ...mockRobotUpdateSession, + }) + + expect(result.current.updateStep).toBe('install') + expect(result.current.progressPercent).toBe(50) + }) + it('should update updateStep and progressPercent when session is provided', () => { const { result, rerender } = renderHook( - session => useRobotUpdateInfo(session), + session => useRobotUpdateInfo(MOCK_ROBOT_NAME, session), { - initialProps: mockRobotUpdateSession, + initialProps: mockRobotUpdateSession as any, + wrapper, } ) expect(result.current.updateStep).toBe('install') - expect(Math.round(result.current.progressPercent)).toBe(67) + expect(Math.round(result.current.progressPercent)).toBe(25) rerender({ ...mockRobotUpdateSession, @@ -63,14 +126,15 @@ describe('useRobotUpdateInfo', () => { it('should return correct updateStep and progressPercent values when there is an error', () => { const { result, rerender } = renderHook( - session => useRobotUpdateInfo(session), + session => useRobotUpdateInfo(MOCK_ROBOT_NAME, session), { - initialProps: mockRobotUpdateSession, + initialProps: mockRobotUpdateSession as any, + wrapper, } ) expect(result.current.updateStep).toBe('install') - expect(Math.round(result.current.progressPercent)).toBe(67) + expect(Math.round(result.current.progressPercent)).toBe(25) rerender({ ...mockRobotUpdateSession, @@ -78,34 +142,42 @@ describe('useRobotUpdateInfo', () => { }) expect(result.current.updateStep).toBe('error') - expect(Math.round(result.current.progressPercent)).toBe(67) + expect(Math.round(result.current.progressPercent)).toBe(25) }) it('should calculate correct progressPercent when the update is not manual', () => { - const { result } = renderHook(session => useRobotUpdateInfo(session), { - initialProps: { - ...mockRobotUpdateSession, - fileInfo: { - systemFile: 'downloadPath', - version: '1.0.0', - isManualFile: false, - }, - }, - }) + const { result } = renderHook( + session => useRobotUpdateInfo(MOCK_ROBOT_NAME, session), + { + initialProps: { + ...mockRobotUpdateSession, + fileInfo: { + systemFile: 'downloadPath', + version: '1.0.0', + isManualFile: false, + }, + } as any, + wrapper, + } + ) expect(result.current.updateStep).toBe('install') - expect(Math.round(result.current.progressPercent)).toBe(67) + expect(Math.round(result.current.progressPercent)).toBe(25) }) it('should ignore progressPercent reported by a step marked as ignored', () => { - const { result } = renderHook(session => useRobotUpdateInfo(session), { - initialProps: { - ...mockRobotUpdateSession, - step: 'processFile' as UpdateSessionStep, - stage: 'awaiting-file' as UpdateSessionStage, - progress: 100, - }, - }) + const { result } = renderHook( + session => useRobotUpdateInfo(MOCK_ROBOT_NAME, session), + { + initialProps: { + ...mockRobotUpdateSession, + step: 'processFile' as UpdateSessionStep, + stage: 'awaiting-file' as UpdateSessionStage, + progress: 100, + } as any, + wrapper, + } + ) expect(result.current.updateStep).toBe('install') expect(Math.round(result.current.progressPercent)).toBe(0) diff --git a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/useRobotUpdateInfo.ts b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/useRobotUpdateInfo.ts index 7518c69c4aa..75c35746c9d 100644 --- a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/useRobotUpdateInfo.ts +++ b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/useRobotUpdateInfo.ts @@ -1,16 +1,20 @@ import * as React from 'react' +import { useSelector } from 'react-redux' + +import { getRobotUpdateDownloadProgress } from '../../../../redux/robot-update' + import type { RobotUpdateSession } from '../../../../redux/robot-update/types' +import type { State } from '../../../../redux/types' export function useRobotUpdateInfo( + robotName: string, session: RobotUpdateSession | null -): { updateStep: UpdateStep; progressPercent: number } { - const progressPercent = useFindProgressPercentFrom(session) +): { updateStep: UpdateStep | null; progressPercent: number } { + const progressPercent = useFindProgressPercentFrom(robotName, session) - const shellReportedUpdateStep = React.useMemo( - () => getShellReportedUpdateStep(session), - [session] - ) - const updateStep = useTransitionUpdateStepFrom(shellReportedUpdateStep) + const updateStep = React.useMemo(() => determineUpdateStepFrom(session), [ + session, + ]) return { updateStep, @@ -19,22 +23,35 @@ export function useRobotUpdateInfo( } function useFindProgressPercentFrom( + robotName: string, session: RobotUpdateSession | null ): number { const [progressPercent, setProgressPercent] = React.useState(0) + const hasSeenDownloadFileStep = React.useRef(false) const prevSeenUpdateStep = React.useRef(null) const prevSeenStepProgress = React.useRef(0) const currentStepWithProgress = React.useRef(-1) - if (session == null) return progressPercent + const downloadProgress = useSelector((state: State) => + getRobotUpdateDownloadProgress(state, robotName) + ) + + if (session == null) { + if (progressPercent !== 0) { + setProgressPercent(0) + prevSeenStepProgress.current = 0 + hasSeenDownloadFileStep.current = false + } + return progressPercent + } - const { + let { step: sessionStep, stage: sessionStage, progress: stepProgress, } = session - if (sessionStep === 'getToken') { + if (sessionStep == null && sessionStage == null) { if (progressPercent !== 0) { setProgressPercent(0) prevSeenStepProgress.current = 0 @@ -46,24 +63,30 @@ function useFindProgressPercentFrom( prevSeenStepProgress.current = 100 } return progressPercent - } else if ( - sessionStage === 'error' || - sessionStage === null || - stepProgress == null || - sessionStep == null - ) { + } else if (sessionStage === 'error') { return progressPercent } const stepAndStage = `${sessionStep}-${sessionStage}` - // Ignored because 0-100 is too fast to be worth recording. - const IGNORED_STEPS_AND_STAGES = ['processFile-awaiting-file'] + // Ignored because 0-100 is too fast to be worth recording or currently unsupported. + const IGNORED_STEPS_AND_STAGES = [ + 'processFile-awaiting-file', + 'uploadFile-awaiting-file', + ] + + if (sessionStep === 'downloadFile') { + hasSeenDownloadFileStep.current = true + stepProgress = downloadProgress + } + + stepProgress = stepProgress ?? 0 + // Each stepAndStage is an equal fraction of the total steps. - const TOTAL_STEPS_WITH_PROGRESS = 3 + const TOTAL_STEPS_WITH_PROGRESS = hasSeenDownloadFileStep.current ? 3 : 2 const isNewStateWithProgress = prevSeenUpdateStep.current !== stepAndStage && - stepProgress > 0 && // Accomodate for shell progress oddities. + stepProgress > 0 && // Accommodate for shell progress oddities. stepProgress < 100 // Proceed to next fraction of progress bar. @@ -76,7 +99,7 @@ function useFindProgressPercentFrom( (100 * currentStepWithProgress.current) / TOTAL_STEPS_WITH_PROGRESS prevSeenStepProgress.current = 0 prevSeenUpdateStep.current = stepAndStage - setProgressPercent(completedStepsWithProgress + stepProgress) + setProgressPercent(completedStepsWithProgress) } // Proceed with current fraction of progress bar. else if ( @@ -84,12 +107,13 @@ function useFindProgressPercentFrom( !IGNORED_STEPS_AND_STAGES.includes(stepAndStage) ) { const currentStepProgress = - progressPercent + (stepProgress - prevSeenStepProgress.current) / TOTAL_STEPS_WITH_PROGRESS + const nonBacktrackedProgressPercent = Math.max( progressPercent, - currentStepProgress + currentStepProgress + progressPercent ) + prevSeenStepProgress.current = stepProgress setProgressPercent(nonBacktrackedProgressPercent) } @@ -105,18 +129,19 @@ export type UpdateStep = | 'finished' | 'error' -function getShellReportedUpdateStep( +function determineUpdateStepFrom( session: RobotUpdateSession | null ): UpdateStep | null { if (session == null) return null const { step: sessionStep, stage: sessionStage, error } = session - // TODO(jh, 09-14-2023: add download logic to app-shell/redux/progress bar. let reportedUpdateStep: UpdateStep if (error != null) { reportedUpdateStep = 'error' } else if (sessionStep == null && sessionStage == null) { reportedUpdateStep = 'initial' + } else if (sessionStep === 'downloadFile') { + reportedUpdateStep = 'download' } else if (sessionStep === 'finished') { reportedUpdateStep = 'finished' } else if ( @@ -131,44 +156,3 @@ function getShellReportedUpdateStep( return reportedUpdateStep } - -// Shell steps have the potential to backtrack, so use guarded transitions. -function useTransitionUpdateStepFrom( - reportedUpdateStep: UpdateStep | null -): UpdateStep { - const [updateStep, setUpdateStep] = React.useState('initial') - const prevUpdateStep = React.useRef(null) - - switch (reportedUpdateStep) { - case 'initial': - if (updateStep !== 'initial') { - setUpdateStep('initial') - prevUpdateStep.current = null - } - break - case 'error': - if (updateStep !== 'error') { - setUpdateStep('error') - } - break - case 'install': - if (updateStep === 'initial' && prevUpdateStep.current == null) { - setUpdateStep('install') - prevUpdateStep.current = 'initial' - } - break - case 'restart': - if (updateStep === 'install' && prevUpdateStep.current === 'initial') { - setUpdateStep('restart') - prevUpdateStep.current = 'install' - } - break - case 'finished': - if (updateStep === 'restart' && prevUpdateStep.current === 'install') { - setUpdateStep('finished') - prevUpdateStep.current = 'restart' - } - break - } - return updateStep -} diff --git a/app/src/redux/robot-update/__tests__/epic.test.ts b/app/src/redux/robot-update/__tests__/epic.test.ts index d6ad7d46652..99e549d6b90 100644 --- a/app/src/redux/robot-update/__tests__/epic.test.ts +++ b/app/src/redux/robot-update/__tests__/epic.test.ts @@ -331,6 +331,46 @@ describe('robot update epics', () => { }) }) + describe('startUpdateAfterFileDownload', () => { + it('should start the update after file download if the robot is a flex', () => { + testScheduler.run(({ hot, cold, expectObservable }) => { + const session: ReturnType = { + stage: 'done', + step: 'downloadFile', + } as any + + getRobotUpdateRobot.mockReturnValue(brRobotFlex) + getRobotUpdateSession.mockReturnValue(session) + + const state$ = hot('-a', { a: state }) + const output$ = epics.startUpdateAfterFileDownload(null as any, state$) + + expectObservable(output$).toBe('-a', { + a: actions.readSystemRobotUpdateFile('flex'), + }) + }) + }) + + it('should start the update after file download if the robot is a ot-2', () => { + testScheduler.run(({ hot, cold, expectObservable }) => { + const session: ReturnType = { + stage: 'done', + step: 'downloadFile', + } as any + + getRobotUpdateRobot.mockReturnValue(brRobotOt2) + getRobotUpdateSession.mockReturnValue(session) + + const state$ = hot('-a', { a: state }) + const output$ = epics.startUpdateAfterFileDownload(null as any, state$) + + expectObservable(output$).toBe('-a', { + a: actions.readSystemRobotUpdateFile('ot2'), + }) + }) + }) + }) + it('retryAfterPremigrationEpic', () => { testScheduler.run(({ hot, expectObservable }) => { getRobotUpdateRobot.mockReturnValueOnce(brReadyRobot) diff --git a/app/src/redux/robot-update/__tests__/reducer.test.ts b/app/src/redux/robot-update/__tests__/reducer.test.ts index f2532910818..14d2e3d7b19 100644 --- a/app/src/redux/robot-update/__tests__/reducer.test.ts +++ b/app/src/redux/robot-update/__tests__/reducer.test.ts @@ -64,6 +64,69 @@ describe('robot update reducer', () => { }, }, }, + { + name: 'handles robotUpdate:CHECKING_FOR_UPDATE', + action: { + type: 'robotUpdate:CHECKING_FOR_UPDATE', + payload: 'ot2', + }, + initialState: { ...INITIAL_STATE, session: null }, + expected: { + ...INITIAL_STATE, + session: { + step: 'downloadFile', + stage: 'writing', + target: 'ot2', + }, + }, + }, + { + name: + 'handles robotUpdate:DOWNLOAD_DONE when the target matches the robot type', + action: { + type: 'robotUpdate:DOWNLOAD_DONE', + payload: 'ot2', + }, + initialState: { + ...INITIAL_STATE, + session: { + step: 'downloadFile', + stage: 'writing', + target: 'ot2', + }, + }, + expected: { + ...INITIAL_STATE, + session: { + step: 'downloadFile', + stage: 'done', + }, + }, + }, + { + name: + 'handles robotUpdate:DOWNLOAD_DONE when the target does not match the robot type', + action: { + type: 'robotUpdate:DOWNLOAD_DONE', + payload: 'ot2', + }, + initialState: { + ...INITIAL_STATE, + session: { + step: 'downloadFile', + stage: 'writing', + target: 'flex', + }, + }, + expected: { + ...INITIAL_STATE, + session: { + step: 'downloadFile', + stage: 'writing', + target: 'flex', + }, + }, + }, { name: 'handles robotUpdate:FILE_INFO', action: { diff --git a/app/src/redux/robot-update/constants.ts b/app/src/redux/robot-update/constants.ts index 2c0e625f892..cff9ed795ca 100644 --- a/app/src/redux/robot-update/constants.ts +++ b/app/src/redux/robot-update/constants.ts @@ -2,6 +2,7 @@ export const PREMIGRATION: 'premigration' = 'premigration' export const PREMIGRATION_RESTART: 'premigrationRestart' = 'premigrationRestart' +export const DOWNLOAD_FILE: 'downloadFile' = 'downloadFile' export const GET_TOKEN: 'getToken' = 'getToken' export const UPLOAD_FILE: 'uploadFile' = 'uploadFile' export const PROCESS_FILE: 'processFile' = 'processFile' @@ -32,6 +33,9 @@ export const REINSTALL: 'reinstall' = 'reinstall' // action types +export const ROBOTUPDATE_CHECKING_FOR_UPDATE: 'robotUpdate:CHECKING_FOR_UPDATE' = + 'robotUpdate:CHECKING_FOR_UPDATE' + export const ROBOTUPDATE_UPDATE_VERSION: 'robotUpdate:UPDATE_VERSION' = 'robotUpdate:UPDATE_VERSION' @@ -47,6 +51,9 @@ export const ROBOTUPDATE_DOWNLOAD_PROGRESS: 'robotUpdate:DOWNLOAD_PROGRESS' = export const ROBOTUPDATE_DOWNLOAD_ERROR: 'robotUpdate:DOWNLOAD_ERROR' = 'robotUpdate:DOWNLOAD_ERROR' +export const ROBOTUPDATE_DOWNLOAD_DONE: 'robotUpdate:DOWNLOAD_DONE' = + 'robotUpdate:DOWNLOAD_DONE' + export const ROBOTUPDATE_SET_UPDATE_SEEN: 'robotUpdate:SET_UPDATE_SEEN' = 'robotUpdate:SET_UPDATE_SEEN' diff --git a/app/src/redux/robot-update/epic.ts b/app/src/redux/robot-update/epic.ts index e53cea99c84..0d8a945e32e 100644 --- a/app/src/redux/robot-update/epic.ts +++ b/app/src/redux/robot-update/epic.ts @@ -62,6 +62,7 @@ import { ROBOTUPDATE_FILE_INFO, ROBOTUPDATE_CREATE_SESSION, ROBOTUPDATE_CREATE_SESSION_SUCCESS, + DOWNLOAD_FILE, } from './constants' import type { Observable } from 'rxjs' @@ -144,6 +145,19 @@ export const startUpdateEpic: Epic = (action$, state$) => }) ) +export const startUpdateAfterFileDownload: Epic = (_, state$) => { + return state$.pipe( + filter(passActiveSession({ step: DOWNLOAD_FILE, stage: DONE })), + switchMap(stateWithSession => { + const host: ViewableRobot = getRobotUpdateRobot(stateWithSession) as any + const robotModel = + host?.serverHealth?.robotModel === 'OT-3 Standard' ? 'flex' : 'ot2' + + return of(readSystemRobotUpdateFile(robotModel)) + }) + ) +} + // listen for a the active robot to come back with capabilities after premigration export const retryAfterPremigrationEpic: Epic = (_, state$) => { return state$.pipe( @@ -275,8 +289,6 @@ const passActiveSession = (props: Partial) => ( return ( robot !== null && !session?.error && - typeof session?.pathPrefix === 'string' && - typeof session?.token === 'string' && every( props, (value, key) => session?.[key as keyof RobotUpdateSession] === value @@ -440,6 +452,7 @@ export const removeMigratedRobotsEpic: Epic = (_, state$) => { export const robotUpdateEpic = combineEpics( startUpdateEpic, + startUpdateAfterFileDownload, retryAfterPremigrationEpic, startSessionAfterFileInfoEpic, createSessionEpic, diff --git a/app/src/redux/robot-update/hooks.ts b/app/src/redux/robot-update/hooks.ts index 840349ef331..9316e87ab35 100644 --- a/app/src/redux/robot-update/hooks.ts +++ b/app/src/redux/robot-update/hooks.ts @@ -7,6 +7,7 @@ type DispatchStartRobotUpdate = ( systemFile?: string | undefined ) => void +// Safely start a robot update. export function useDispatchStartRobotUpdate(): DispatchStartRobotUpdate { const dispatch = useDispatch<(a: Action) => void>() diff --git a/app/src/redux/robot-update/reducer.ts b/app/src/redux/robot-update/reducer.ts index e5286cb77b1..bb25e981b3c 100644 --- a/app/src/redux/robot-update/reducer.ts +++ b/app/src/redux/robot-update/reducer.ts @@ -21,7 +21,7 @@ export const INITIAL_STATE: RobotUpdateState = { } export const initialSession = ( - robotName: string, + robotName: string | null, session: RobotUpdateSession | null ): RobotUpdateSession => ({ robotName, @@ -64,6 +64,21 @@ export const robotUpdateReducer: Reducer = ( } } + case Constants.ROBOTUPDATE_CHECKING_FOR_UPDATE: { + const session = state.session as RobotUpdateSession + const target = action.payload + + return { + ...state, + session: { + ...session, + step: Constants.DOWNLOAD_FILE, + stage: Constants.WRITING, + target, + }, + } + } + case Constants.ROBOTUPDATE_DOWNLOAD_PROGRESS: { return { ...state, @@ -76,6 +91,24 @@ export const robotUpdateReducer: Reducer = ( } } + case Constants.ROBOTUPDATE_DOWNLOAD_DONE: { + if (!state.session) return state + + const { target, ...session } = state.session + const isThisRobotDownloadDone = + session?.step === Constants.DOWNLOAD_FILE && target === action.payload + + return isThisRobotDownloadDone + ? { + ...state, + session: { + ...session, + stage: Constants.DONE, + }, + } + : state + } + case Constants.ROBOTUPDATE_DOWNLOAD_ERROR: { return { ...state, @@ -101,7 +134,12 @@ export const robotUpdateReducer: Reducer = ( return { ...state, - session: { ...session, step: Constants.GET_TOKEN }, + session: { + ...session, + robotName: host.name, + step: Constants.GET_TOKEN, + stage: null, + }, } } diff --git a/app/src/redux/robot-update/types.ts b/app/src/redux/robot-update/types.ts index d91f774fe4b..3a8f5666b38 100644 --- a/app/src/redux/robot-update/types.ts +++ b/app/src/redux/robot-update/types.ts @@ -33,6 +33,7 @@ export type UpdateSessionStage = export type UpdateSessionStep = | 'premigration' | 'premigrationRestart' + | 'downloadFile' | 'getToken' | 'uploadFile' | 'processFile' @@ -42,7 +43,7 @@ export type UpdateSessionStep = | 'finished' export interface RobotUpdateSession { - robotName: string + robotName: string | null fileInfo: RobotUpdateFileInfo | null token: string | null pathPrefix: string | null @@ -50,6 +51,7 @@ export interface RobotUpdateSession { stage: UpdateSessionStage | null progress: number | null error: string | null + target?: RobotUpdateTarget } export interface PerTargetRobotUpdateState { @@ -172,3 +174,5 @@ export type RobotUpdateAction = | { type: 'robotUpdate:CLEAR_SESSION' } | { type: 'robotUpdate:SET_UPDATE_SEEN'; meta: { robotName: string } } | { type: 'robotUpdate:FILE_UPLOAD_PROGRESS'; payload: number } + | { type: 'robotUpdate:CHECKING_FOR_UPDATE'; payload: RobotUpdateTarget } + | { type: 'robotUpdate:DOWNLOAD_DONE'; payload: RobotUpdateTarget } From 2fcfb6e545c4760c38274ff74b929001ff99eec6 Mon Sep 17 00:00:00 2001 From: Jamey H Date: Tue, 7 Nov 2023 16:08:48 -0500 Subject: [PATCH 2/2] feedback --- app-shell/src/http.ts | 23 +++++++++++------- .../RobotUpdateProgressModal.tsx | 24 +++++++++++++++---- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/app-shell/src/http.ts b/app-shell/src/http.ts index c4ce12d4aa6..02fe50da3e1 100644 --- a/app-shell/src/http.ts +++ b/app-shell/src/http.ts @@ -90,17 +90,22 @@ export function postFile( init?: RequestInit, progress?: (progress: number) => void ): Promise { - return createReadStream(source, progress ?? null).then(readStream => { - const body = new FormData() - body.append(name, readStream) - return fetch(input, { ...init, body, method: 'POST' }) + return new Promise((resolve, reject) => { + createReadStream(source, progress ?? null, reject).then(readStream => { + return new Promise(resolve => { + const body = new FormData() + body.append(name, readStream) + resolve(fetch(input, { ...init, body, method: 'POST' })) + }).then(resolve) + }) }) } function createReadStreamWithSize( source: string, size: number, - progress: ((progress: number) => void) | null + progress: ((progress: number) => void) | null, + onError: (error: unknown) => unknown ): Promise { return new Promise((resolve, reject) => { const readStream = fs.createReadStream(source) @@ -125,6 +130,7 @@ function createReadStreamWithSize( } readStream.once('error', handleError) + readStream.once('error', onError) function handleSuccess(): void { resolve(readStream) @@ -142,12 +148,13 @@ function createReadStreamWithSize( // create a read stream, handling errors that `fetch` is unable to catch function createReadStream( source: string, - progress: ((progress: number) => void) | null + progress: ((progress: number) => void) | null, + onError: (error: unknown) => unknown ): Promise { return fsPromises .stat(source) .then(filestats => - createReadStreamWithSize(source, filestats.size, progress) + createReadStreamWithSize(source, filestats.size, progress, onError) ) - .catch(() => createReadStreamWithSize(source, Infinity, progress)) + .catch(() => createReadStreamWithSize(source, Infinity, progress, onError)) } diff --git a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx index d951870dcd6..94a31b77560 100644 --- a/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx +++ b/app/src/organisms/Devices/RobotSettings/UpdateBuildroot/RobotUpdateProgressModal.tsx @@ -99,12 +99,26 @@ export function RobotUpdateProgressModal({ ) let modalBodyText = '' - if (updateStep === 'download') modalBodyText = t('downloading_update') - if (updateStep === 'install') modalBodyText = t('installing_update') let subProgressBarText = t('do_not_turn_off') - if (updateStep === 'restart') modalBodyText = t('restarting_robot') - if (updateStep === 'restart' && letUserExitUpdate) { - subProgressBarText = t('restart_taking_too_long', { robotName }) + switch (updateStep) { + case 'initial': + case 'error': + modalBodyText = '' + break + case 'download': + modalBodyText = t('downloading_update') + break + case 'install': + modalBodyText = t('installing_update') + break + case 'restart': + modalBodyText = t('restarting_robot') + if (letUserExitUpdate) { + subProgressBarText = t('restart_taking_too_long', { robotName }) + } + break + default: + modalBodyText = t('installing_update') } return (