Editor: Revisions: Diff revisions only once (#19072)

* Post Revisions: Don't compute or display diffs when the amount of content is too large (#19860) * Post Revisions: Don't compute or display diffs when the amount of content is too large Diff in the selector so it is memoized & can be used in both the list item & the diff view * Improve auto selecting first revision in the list * When post is too long to diff, just show the whole thing in the viewer * Post Revisions: short-circuit the selector when the flag is not enabled * remove the is-loading class when content is "too long" * Post Revisions: Update Tests for diffing change (#19944) * Update tests for the getPostRevisionChanges selector * Don't annotate the response in the receiveSuccess test. We're diffing on select now -- not receive. * Enable the feature flag in the test environment
Automattic · Nov 17, 2017 · 41e4be8 · 41e4be8
1 parent a76c807
commit 41e4be8
Show file tree

Hide file tree

Showing 8 changed files with 228 additions and 110 deletions.
diff --git a/client/post-editor/editor-diff-viewer/index.jsx b/client/post-editor/editor-diff-viewer/index.jsx
@@ -14,7 +14,7 @@ import { get, has } from 'lodash';
 /**
  * Internal dependencies
  */
-import { getPostRevisionChanges } from 'state/selectors';
+import { getPostRevision, getPostRevisionChanges } from 'state/selectors';
 import EditorDiffChanges from './changes';
 
 class EditorDiffViewer extends PureComponent {
@@ -49,23 +49,32 @@ class EditorDiffViewer extends PureComponent {
 	}
 
 	render() {
-		const { revisionChanges } = this.props;
+		const { revision, revisionChanges } = this.props;
 		const classes = classNames( 'editor-diff-viewer', {
-			'is-loading': ! has( revisionChanges, 'title[0].value' ),
+			'is-loading': ! ( revisionChanges.tooLong || has( revisionChanges, 'title[0].value' ) ),
 		} );
 		return (
 			<div className={ classes }>
 				<h1 className="editor-diff-viewer__title">
-					<EditorDiffChanges changes={ revisionChanges.title } />
+					{ revisionChanges.tooLong ? (
+						revision.title
+					) : (
+						<EditorDiffChanges changes={ revisionChanges.title } />
+					) }
 				</h1>
 				<pre className="editor-diff-viewer__content">
-					<EditorDiffChanges changes={ revisionChanges.content } />
+					{ revisionChanges.tooLong ? (
+						revision.content
+					) : (
+						<EditorDiffChanges changes={ revisionChanges.content } />
+					) }
 				</pre>
 			</div>
 		);
 	}
 }
 
 export default connect( ( state, { siteId, postId, selectedRevisionId } ) => ( {
+	revision: getPostRevision( state, siteId, postId, selectedRevisionId, 'display' ),
 	revisionChanges: getPostRevisionChanges( state, siteId, postId, selectedRevisionId ),
 } ) )( EditorDiffViewer );
diff --git a/client/post-editor/editor-revisions-list/index.jsx b/client/post-editor/editor-revisions-list/index.jsx
@@ -27,7 +27,12 @@ class EditorRevisionsList extends PureComponent {
 		selectedRevisionId: PropTypes.number,
 	};
 
-	trySelectingTimeout = null;
+	trySelectingInterval = null;
+
+	selectRevision = revisionId => {
+		clearInterval( this.trySelectingInterval );
+		this.props.selectPostRevision( revisionId );
+	};
 
 	trySelectingFirstRevision = () => {
 		const { revisions } = this.props;
@@ -38,29 +43,26 @@ class EditorRevisionsList extends PureComponent {
 		if ( ! firstRevision.id ) {
 			return;
 		}
-		this.props.selectPostRevision( firstRevision.id );
+		this.selectRevision( firstRevision.id );
 	};
 
-	componentWillReceiveProps( { selectedRevisionId } ) {
-		if (
-			! this.trySelectingTimeout &&
-			( ! selectedRevisionId || ! this.props.selectedRevisionId )
-		) {
-			this.trySelectingTimeout = setTimeout( this.trySelectingFirstRevision, 300 );
-		}
-	}
-
 	componentDidMount() {
 		// Make sure that scroll position in the editor is not preserved.
 		window.scrollTo( 0, 0 );
 
 		KeyboardShortcuts.on( 'move-selection-up', this.selectNextRevision );
 		KeyboardShortcuts.on( 'move-selection-down', this.selectPreviousRevision );
+
+		if ( ! this.props.selectedRevisionId ) {
+			this.trySelectingInterval = setInterval( this.trySelectingFirstRevision, 500 );
+		}
 	}
 
 	componentWillUnmount() {
 		KeyboardShortcuts.off( 'move-selection-up', this.selectNextRevision );
 		KeyboardShortcuts.off( 'move-selection-down', this.selectPreviousRevision );
+
+		clearInterval( this.trySelectingInterval );
 	}
 
 	componentDidUpdate() {
@@ -95,19 +97,20 @@ class EditorRevisionsList extends PureComponent {
 
 	selectNextRevision = () => {
 		const { nextRevisionId } = this.props;
-		nextRevisionId && this.props.selectPostRevision( nextRevisionId );
+		nextRevisionId && this.selectRevision( nextRevisionId );
 	};
 
 	selectPreviousRevision = () => {
 		const { prevRevisionId } = this.props;
-		prevRevisionId && this.props.selectPostRevision( prevRevisionId );
+		prevRevisionId && this.selectRevision( prevRevisionId );
 	};
 
 	render() {
-		const { revisions, selectedRevisionId, siteId } = this.props;
+		const { postId, revisions, selectedRevisionId, siteId } = this.props;
 		const classes = classNames( 'editor-revisions-list', {
 			'is-loading': isEmpty( revisions ),
 		} );
+
 		return (
 			<div className={ classes }>
 				<EditorRevisionsListHeader numRevisions={ revisions.length } />
@@ -119,7 +122,11 @@ class EditorRevisionsList extends PureComponent {
 							} );
 							return (
 								<li className={ itemClasses } key={ revision.id }>
-									<EditorRevisionsListItem revision={ revision } siteId={ siteId } />
+									<EditorRevisionsListItem
+										postId={ postId }
+										revision={ revision }
+										siteId={ siteId }
+									/>
 								</li>
 							);
 						} ) }

diff --git a/client/post-editor/editor-revisions-list/item.jsx b/client/post-editor/editor-revisions-list/item.jsx
@@ -13,6 +13,7 @@ import { flow, get } from 'lodash';
 /**
  * Internal dependencies
  */
+import { getPostRevisionChanges } from 'state/selectors';
 import { selectPostRevision } from 'state/posts/revisions/actions';
 import { isSingleUserSite } from 'state/sites/selectors';
 import TimeSince from 'components/time-since';
@@ -23,10 +24,10 @@ class EditorRevisionsListItem extends PureComponent {
 	};
 
 	render() {
-		const { revision, isMultiUserSite, translate } = this.props;
+		const { revision, revisionChanges, isMultiUserSite, translate } = this.props;
 		const authorName = get( revision, 'author.display_name' );
-		const added = get( revision, 'changes.added' );
-		const removed = get( revision, 'changes.removed' );
+		const added = get( revisionChanges, 'summary.added' );
+		const removed = get( revisionChanges, 'summary.removed' );
 
 		return (
 			<button
@@ -81,11 +82,13 @@ class EditorRevisionsListItem extends PureComponent {
 }
 
 EditorRevisionsListItem.propTypes = {
+	postId: PropTypes.number,
 	revision: PropTypes.object.isRequired,
 	siteId: PropTypes.number.isRequired,
 
 	// connected to state
 	isMultiUserSite: PropTypes.bool.isRequired,
+	revisionChanges: PropTypes.array.isRequired,
 
 	// connected to dispatcher
 	selectPostRevision: PropTypes.func.isRequired,
@@ -97,8 +100,9 @@ EditorRevisionsListItem.propTypes = {
 export default flow(
 	localize,
 	connect(
-		( state, { siteId } ) => ( {
+		( state, { postId, revision, siteId } ) => ( {
 			isMultiUserSite: ! isSingleUserSite( state, siteId ),
+			revisionChanges: getPostRevisionChanges( state, siteId, postId, revision.id ),
 		} ),
 		{ selectPostRevision }
 	)

diff --git a/client/state/data-layer/wpcom/posts/revisions/index.js b/client/state/data-layer/wpcom/posts/revisions/index.js
@@ -4,13 +4,11 @@
  * External dependencies
  */
 
-import { flow, forEach, get, map, mapKeys, mapValues, omit, pick } from 'lodash';
+import { flow, map, mapKeys, mapValues, omit, pick } from 'lodash';
 
 /**
  * Internal dependencies
  */
-import { isEnabled } from 'config';
-import { countDiffWords, diffWords } from 'lib/text-utils';
 import { POST_REVISIONS_REQUEST } from 'state/action-types';
 import { dispatchRequest } from 'state/data-layer/wpcom-http/utils';
 import { http } from 'state/data-layer/wpcom-http/actions';
@@ -77,15 +75,6 @@ export const receiveError = ( { dispatch }, { siteId, postId }, rawError ) =>
  */
 export const receiveSuccess = ( { dispatch }, { siteId, postId }, revisions ) => {
 	const normalizedRevisions = map( revisions, normalizeRevision );
-
-	if ( isEnabled( 'post-editor/revisions' ) ) {
-		forEach( normalizedRevisions, ( revision, index ) => {
-			revision.changes = countDiffWords(
-				diffWords( get( normalizedRevisions, [ index + 1, 'content' ], '' ), revision.content )
-			);
-		} );
-	}
-
 	dispatch( receivePostRevisionsSuccess( siteId, postId ) );
 	dispatch( receivePostRevisions( siteId, postId, normalizedRevisions ) );
 };

diff --git a/client/state/data-layer/wpcom/posts/revisions/test/index.js b/client/state/data-layer/wpcom/posts/revisions/test/index.js
@@ -4,13 +4,12 @@
  * External dependencies
  */
 import { expect } from 'chai';
-import { cloneDeep, forEach, map } from 'lodash';
+import { cloneDeep, map } from 'lodash';
 import sinon from 'sinon';
 
 /**
  * Internal dependencies
  */
-import { isEnabled } from 'config';
 import { fetchPostRevisions, normalizeRevision, receiveSuccess, receiveError } from '../';
 import { http } from 'state/data-layer/wpcom-http/actions';
 import {
@@ -150,11 +149,6 @@ describe( '#receiveSuccess', () => {
 		receiveSuccess( { dispatch }, action, successfulPostRevisionsResponse );
 
 		const expectedRevisions = cloneDeep( normalizedPostRevisions );
-		if ( isEnabled( 'post-editor/revisions' ) ) {
-			forEach( expectedRevisions, revision => {
-				revision.changes = { added: 2, removed: 0 };
-			} );
-		}
 
 		expect( dispatch ).to.have.callCount( 2 );
 		expect( dispatch ).to.have.been.calledWith( receivePostRevisionsSuccess( 12345678, 10 ) );

diff --git a/client/state/selectors/get-post-revision-changes.js b/client/state/selectors/get-post-revision-changes.js
@@ -4,32 +4,58 @@
  * External dependencies
  */
 
-import { findIndex, get, isUndefined, map, omitBy } from 'lodash';
+import { findIndex, get, isUndefined, map, omitBy, reduce } from 'lodash';
 
 /**
  * Internal dependencies
  */
+import { isEnabled } from 'config';
 import createSelector from 'lib/create-selector';
-import { diffWords } from 'lib/text-utils';
+import { countDiffWords, diffWords } from 'lib/text-utils';
 import getPostRevisions from 'state/selectors/get-post-revisions';
 
+const MAX_DIFF_CONTENT_LENGTH = 20000;
+
 const diffKey = ( key, obj1, obj2 ) =>
 	map( diffWords( get( obj1, key, '' ), get( obj2, key, '' ) ), change =>
 		omitBy( change, isUndefined )
 	);
 
+const getCombinedLength = list =>
+	reduce(
+		list,
+		( sum, r ) => ( sum += get( r, 'title.length', 0 ) + get( r, 'content.length', 0 ) ),
+		0
+	);
+
 const getPostRevisionChanges = createSelector(
 	( state, siteId, postId, revisionId ) => {
+		const noChanges = { content: [], summary: {}, title: [] };
+		if ( ! isEnabled( 'post-editor/revisions' ) ) {
+			return noChanges;
+		}
+
 		const orderedRevisions = getPostRevisions( state, siteId, postId, 'display' );
 		const revisionIndex = findIndex( orderedRevisions, { id: revisionId } );
 		if ( revisionIndex === -1 ) {
-			return { content: [], title: [] };
+			return noChanges;
 		}
-		const previousRevision = orderedRevisions[ revisionIndex + 1 ];
-		const currentRevision = orderedRevisions[ revisionIndex ];
+
+		const revision = orderedRevisions[ revisionIndex ];
+		const previousRevision = get( orderedRevisions, [ revisionIndex + 1 ], {} );
+		const combinedLength = getCombinedLength( [ previousRevision, revision ] );
+
+		if ( combinedLength > MAX_DIFF_CONTENT_LENGTH ) {
+			return { ...noChanges, tooLong: true };
+		}
+		const title = diffKey( 'title', previousRevision, revision );
+		const content = diffKey( 'content', previousRevision, revision );
+		const summary = countDiffWords( title.concat( content ) );
+
 		return {
-			content: diffKey( 'content', previousRevision, currentRevision ),
-			title: diffKey( 'title', previousRevision, currentRevision ),
+			content,
+			summary,
+			title,
 		};
 	},
 	state => [ state.posts.revisions.revisions ]