From 35361dcbe29d1080c9881c9d85907286a27c2768 Mon Sep 17 00:00:00 2001 From: William Durand Date: Wed, 24 Apr 2019 14:24:42 +0200 Subject: [PATCH] Prevent infinite loop by aborting fetch version --- src/pages/Compare/index.spec.tsx | 49 +++++++++----------------------- src/pages/Compare/index.tsx | 35 ++++++++++++----------- src/reducers/versions.tsx | 2 +- 3 files changed, 33 insertions(+), 53 deletions(-) diff --git a/src/pages/Compare/index.spec.tsx b/src/pages/Compare/index.spec.tsx index 4cee99efc..9a6dcd25a 100644 --- a/src/pages/Compare/index.spec.tsx +++ b/src/pages/Compare/index.spec.tsx @@ -510,45 +510,22 @@ describe(__filename, () => { }); }); - // This could happen when a keyboard navigation updates the selected path. - it('does not dispatch viewVersionFile() on update if the query string contains a `path` that is different than `version.selectedPath`', () => { - const { dispatchSpy, root } = loadDiffAndRender(); - - const path = 'a/different/file.js'; - const history = createFakeHistory({ - location: createFakeLocation({ - search: queryString.stringify({ path }), - }), - }); - - root.setProps({ history }); - - expect(dispatchSpy).not.toHaveBeenCalled(); - }); - - it('does not dispatch viewVersionFile() on update when `path` is equal to the selected path', () => { - const { dispatchSpy, root, version } = loadDiffAndRender(); - - const history = createFakeHistory({ - location: createFakeLocation({ - search: queryString.stringify({ path: version.file.selected_file }), - }), - }); - - root.setProps({ history }); - - expect(dispatchSpy).not.toHaveBeenCalled(); - }); + it('does not dispatch anything on mount when an API error has occured', () => { + const addonId = 9999; + const baseVersionId = 1; + const headVersionId = baseVersionId + 1; - it('does not dispatch viewVersionFile() on update when the query string does not contain a `path`', () => { - const { dispatchSpy, root } = loadDiffAndRender(); + const store = configureStore(); + store.dispatch(versionsActions.beginFetchDiff()); + // Simulate an API error. + store.dispatch(versionsActions.abortFetchDiff()); + const dispatch = spyOn(store, 'dispatch'); - const history = createFakeHistory({ - location: createFakeLocation({ search: '' }), + render({ + ...getRouteParams({ addonId, baseVersionId, headVersionId }), + store, }); - root.setProps({ history }); - - expect(dispatchSpy).not.toHaveBeenCalled(); + expect(dispatch).not.toHaveBeenCalled(); }); }); diff --git a/src/pages/Compare/index.tsx b/src/pages/Compare/index.tsx index 468fbd3c3..4ed87beed 100644 --- a/src/pages/Compare/index.tsx +++ b/src/pages/Compare/index.tsx @@ -34,7 +34,6 @@ type PropsFromRouter = { type PropsFromState = { addonId: number; compareInfo: CompareInfo | null | void; - isLoading: boolean; version: Version | void | null; }; @@ -58,7 +57,14 @@ export class CompareBase extends React.Component { } loadData(prevProps?: Props) { - const { _fetchDiff, dispatch, history, match, version } = this.props; + const { + _fetchDiff, + compareInfo, + dispatch, + history, + match, + version, + } = this.props; const { addonId, baseVersionId, headVersionId, lang } = match.params; const oldVersionId = parseInt(baseVersionId, 10); @@ -75,12 +81,12 @@ export class CompareBase extends React.Component { const path = getPathFromQueryString(history); - if (version === null) { + if (compareInfo === null) { // An error has occured when fetching the version. return; } - if (version === undefined) { + if (compareInfo === undefined) { dispatch( _fetchDiff({ addonId: parseInt(addonId, 10), @@ -92,14 +98,13 @@ export class CompareBase extends React.Component { return; } - if (!prevProps) { + if (!prevProps || !version) { return; } if ( - (prevProps && - (prevProps.version && - version.selectedPath !== prevProps.version.selectedPath)) || + (prevProps.version && + version.selectedPath !== prevProps.version.selectedPath) || addonId !== prevProps.match.params.addonId || baseVersionId !== prevProps.match.params.baseVersionId || headVersionId !== prevProps.match.params.headVersionId @@ -115,9 +120,7 @@ export class CompareBase extends React.Component { } } - // When selecting a new file to view, we do not want to preserve the hash in - // the URL (this hash highlights a specific line of code). - viewVersionFile = (path: string, { preserveHash = false } = {}) => { + viewVersionFile = (path: string) => { const { _viewVersionFile, dispatch, match } = this.props; const { headVersionId } = match.params; @@ -125,15 +128,17 @@ export class CompareBase extends React.Component { _viewVersionFile({ selectedPath: path, versionId: parseInt(headVersionId, 10), - preserveHash, + // When selecting a new file to view, we do not want to preserve the + // hash in the URL (this hash highlights a specific line of code). + preserveHash: false, }), ); }; renderLoadingMessageOrError(message: string) { - const { isLoading } = this.props; + const { compareInfo } = this.props; - if (!isLoading) { + if (compareInfo === null) { return (

{gettext('Ooops, an error has occured.')} @@ -190,7 +195,6 @@ const mapStateToProps = ( const headVersionId = parseInt(match.params.headVersionId, 10); const { compareInfo } = state.versions; - const isLoading = compareInfo === undefined; // The Compare API returns the version info of the head/newest version. const version = getVersionInfo(state.versions, headVersionId); @@ -198,7 +202,6 @@ const mapStateToProps = ( return { addonId, compareInfo, - isLoading, version, }; }; diff --git a/src/reducers/versions.tsx b/src/reducers/versions.tsx index 854cc7921..0a54c3aef 100644 --- a/src/reducers/versions.tsx +++ b/src/reducers/versions.tsx @@ -615,8 +615,8 @@ export const fetchDiff = ({ }); if (isErrorResponse(response)) { - dispatch(errorsActions.addError({ error: response.error })); dispatch(actions.abortFetchDiff()); + dispatch(errorsActions.addError({ error: response.error })); } else { dispatch(actions.loadVersionInfo({ version: response })); dispatch(