Skip to content

Commit

Permalink
refactor(core): virtualize execution rendering (#7140)
Browse files Browse the repository at this point in the history 
* refactor(core): virtualize execution rendering

* useEffect instead of useCallback
  • Loading branch information
@anotherchrisberry
anotherchrisberry authored Jun 26, 2019
1 parent 342087c commit d5425b4
Show file tree
Hide file tree
Showing 15 changed files with 317 additions and 318 deletions.
2 changes: 0 additions & 2 deletions app/scripts/modules/core/src/cloudProvider/CloudProviderRegistry.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/* tslint:disable: no-console */
import { cloneDeep, uniq, without } from 'lodash';
import { $log } from 'ngimport';

import { SETTINGS } from 'core/config/settings';

Expand Down Expand Up @@ -118,7 +117,6 @@ export class CloudProviderRegistry {
});

if (notFound) {
$log.debug(`No value configured for "${key}" in provider "${cloudProvider}"`);
return null;
}
return current;
Expand Down
4 changes: 1 addition & 3 deletions app/scripts/modules/core/src/pipeline/details/SingleExecutionDetails.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ export class SingleExecutionDetails extends React.Component<

executionService.getExecution($state.params.executionId).then(
execution => {
const stateExecution = this.state.execution || execution;
executionService.transformExecution(app, execution);
executionService.synchronizeExecution(stateExecution, execution);
if (execution.isActive && !this.executionScheduler) {
this.executionScheduler = SchedulerFactory.createScheduler(5000);
this.executionLoader = this.executionScheduler.subscribe(() => this.getExecution());
Expand All @@ -76,7 +74,7 @@ export class SingleExecutionDetails extends React.Component<
this.executionScheduler.unsubscribe();
this.executionLoader.unsubscribe();
}
this.setState({ execution: stateExecution });
this.setState({ execution });
},
() => {
this.setState({ execution: null, stateNotFound: true });
Expand Down
102 changes: 8 additions & 94 deletions app/scripts/modules/core/src/pipeline/executions/Executions.spec.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,22 +1,20 @@
import * as React from 'react';
import { ReactWrapper, mount } from 'enzyme';
import { set } from 'lodash';
import { IScope, ITimeoutService, mock, noop } from 'angular';
import { IScope, mock, noop } from 'angular';

import { Application } from 'core/application';
import { ApplicationModelBuilder } from 'core/application/applicationModel.builder';
import { INSIGHT_FILTER_STATE_MODEL } from 'core/insight/insightFilterState.model';
import { REACT_MODULE, ReactInjector } from 'core/reactShims';
import { REACT_MODULE } from 'core/reactShims';
import { OVERRIDE_REGISTRY } from 'core/overrideRegistry';
import { ScrollToService } from 'core/utils';
import * as State from 'core/state';
import { IExecutionsProps, IExecutionsState, Executions } from './Executions';

describe('<Executions/>', () => {
let component: ReactWrapper<IExecutionsProps, IExecutionsState>;
let application: Application;
let scope: IScope;
let $timeout: ITimeoutService;

function initializeApplication(data?: any) {
set(application, 'executions.activate', noop);
Expand All @@ -35,10 +33,9 @@ describe('<Executions/>', () => {

beforeEach(mock.module(INSIGHT_FILTER_STATE_MODEL, REACT_MODULE, OVERRIDE_REGISTRY));
beforeEach(
mock.inject((_$timeout_: ITimeoutService, $rootScope: IScope) => {
mock.inject(($rootScope: IScope) => {
State.initialize();
scope = $rootScope.$new();
$timeout = _$timeout_;
application = ApplicationModelBuilder.createApplicationForTests(
'app',
{ key: 'executions', lazy: true },
Expand All @@ -47,98 +44,15 @@ describe('<Executions/>', () => {
}),
);

it('should not set loading flag to false until executions and pipeline configs have been loaded', function() {
it('should not set loading flag to false until executions and pipeline configs have been loaded', done => {
initializeApplication();
expect(component.state().loading).toBe(true);
application.executions.dataUpdated();
application.pipelineConfigs.dataUpdated();
scope.$digest();
$timeout.flush();
expect(component.state().loading).toBe(false);
});

describe('auto-scrolling behavior', function() {
beforeEach(function() {
spyOn(ScrollToService, 'scrollTo');
});

it('should scroll execution into view on initialization if an execution is present in state params', function() {
ReactInjector.$stateParams.executionId = 'a';

initializeApplication({ pipelineConfigs: [], executions: [] });
scope.$digest();

expect((ScrollToService.scrollTo as any).calls.count()).toBe(1);
});

it('should NOT scroll execution into view on initialization if none present in state params', function() {
initializeApplication();
scope.$digest();

expect((ScrollToService.scrollTo as any).calls.count()).toBe(0);
});

// TODO: Figure out how to test transitions
// it('should scroll execution into view on state change success if no execution id in state params', function () {
// initializeApplication();
// scope.$digest();

// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(0);

// scope.$broadcast('$stateChangeSuccess', {name: 'executions.execution'}, {executionId: 'a'}, {name: 'executions'}, {});
// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(1);
// });

// it('should scroll execution into view on state change success if execution id changes', function () {
// initializeApplication();
// scope.$digest();

// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(0);

// scope.$broadcast('$stateChangeSuccess', {name: 'executions.execution'}, {executionId: 'a'}, {name: 'executions.execution'}, {executionId: 'b'});
// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(1);
// });

// it('should scroll into view if no params change, because the user clicked on a link somewhere else in the page', function () {
// const params = {executionId: 'a', step: 'b', stage: 'c', details: 'd'};

// initializeApplication();
// scope.$digest();

// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(0);

// scope.$broadcast('$stateChangeSuccess', {name: 'executions.execution'}, params, {name: 'executions.execution'}, params);
// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(1);
// });

// it('should NOT scroll into view if step changes', function () {
// const toParams = {executionId: 'a', step: 'b', stage: 'c', details: 'd'},
// fromParams = {executionId: 'a', step: 'c', stage: 'c', details: 'd'};

// initializeApplication();
// scope.$digest();
// scope.$broadcast('$stateChangeSuccess', {name: 'executions'}, toParams, {name: 'executions'}, fromParams);
// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(0);
// });

// it('should NOT scroll into view if stage changes', function () {
// const toParams = {executionId: 'a', step: 'b', stage: 'c', details: 'd'},
// fromParams = {executionId: 'a', step: 'b', stage: 'e', details: 'd'};

// initializeApplication();
// scope.$digest();
// scope.$broadcast('$stateChangeSuccess', {name: 'executions'}, toParams, {name: 'executions'}, fromParams);
// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(0);
// });

// it('should NOT scroll into view if detail changes', function () {
// const toParams = {executionId: 'a', step: 'b', stage: 'c', details: 'd'},
// fromParams = {executionId: 'a', step: 'b', stage: 'c', details: 'e'};

// initializeApplication();
// scope.$digest();
// scope.$broadcast('$stateChangeSuccess', {name: 'executions'}, toParams, {name: 'executions'}, fromParams);
// expect((ReactInjector.scrollToService.scrollTo as any).calls.count()).toBe(0);
// });
setTimeout(() => {
expect(component.state().loading).toBe(false);
done();
}, 100);
});
});
86 changes: 29 additions & 57 deletions app/scripts/modules/core/src/pipeline/executions/Executions.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ import { CreatePipelineButton } from 'core/pipeline/create/CreatePipelineButton'
import { IScheduler } from 'core/scheduler/SchedulerFactory';
import * as React from 'react';
import * as ReactGA from 'react-ga';
import { Transition } from '@uirouter/core';
import { get } from 'lodash';
import { $timeout, $q } from 'ngimport';
import { $q } from 'ngimport';
import { Subscription } from 'rxjs';

import { Application } from 'core/application';
Expand All @@ -20,7 +19,6 @@ import { ExecutionGroups } from './executionGroup/ExecutionGroups';
import { FilterTags, IFilterTag, ISortFilter } from 'core/filterModel';
import { Spinner } from 'core/widgets/spinners/Spinner';
import { ExecutionState } from 'core/state';
import { ScrollToService } from 'core/utils';
import { IRetryablePromise } from 'core/utils/retryablePromise';
import { SchedulerFactory } from 'core/scheduler';

Expand All @@ -38,12 +36,12 @@ export interface IExecutionsState {
sortFilter: ISortFilter;
tags: IFilterTag[];
triggeringExecution: boolean;
reloadingForFilters: boolean;
}

export class Executions extends React.Component<IExecutionsProps, IExecutionsState> {
private executionsRefreshUnsubscribe: Function;
private groupsUpdatedSubscription: Subscription;
private locationChangeUnsubscribe: Function;
private insightFilterStateModel = ReactInjector.insightFilterStateModel;
private activeRefresher: IScheduler;

Expand All @@ -59,9 +57,16 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
sortFilter: ExecutionState.filterModel.asFilterModel.sortFilter,
tags: [],
triggeringExecution: false,
reloadingForFilters: false,
};
}

private setReloadingForFilters = (reloadingForFilters: boolean) => {
if (this.state.reloadingForFilters !== reloadingForFilters) {
this.setState({ reloadingForFilters });
}
};

private clearFilters = (): void => {
ExecutionFilterService.clearFilters();
this.updateExecutionGroups(true);
Expand All @@ -74,21 +79,32 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
private updateExecutionGroups(reload?: boolean): void {
this.normalizeExecutionNames();
const { app } = this.props;
// updateExecutionGroups is debounced by 25ms, so we need to delay setting the loading flags a bit
if (reload) {
app.executions.refresh(true);
app.executions.reloadingForFilters = true;
this.setReloadingForFilters(true);
app.executions.refresh(true).then(() => {
ExecutionFilterService.updateExecutionGroups(app);
setTimeout(() => this.setReloadingForFilters(false), 50);
});
} else {
ExecutionFilterService.updateExecutionGroups(app);
this.groupsUpdated();
// updateExecutionGroups is debounced by 25ms, so we need to delay setting the loading flag a bit
$timeout(() => {
setTimeout(() => {
this.setState({ loading: false });
}, 50);
}
}

private groupsUpdated(): void {
this.setState({ tags: ExecutionState.filterModel.asFilterModel.tags });
const newTags = ExecutionState.filterModel.asFilterModel.tags;
const currentTags = this.state.tags;
const areEqual = (t1: IFilterTag, t2: IFilterTag) =>
t1.key === t2.key && t1.label === t2.label && t1.value === t2.value;
const tagsChanged =
newTags.length !== currentTags.length || newTags.some(t1 => !currentTags.some(t2 => areEqual(t1, t2)));
if (tagsChanged) {
this.setState({ tags: newTags });
}
}

private dataInitializationFailure(): void {
Expand Down Expand Up @@ -162,43 +178,6 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
ReactInjector.$state.go('.', { startManualExecution: null }, { inherit: true, location: 'replace' });
}

private scrollIntoView(delay = 200): void {
ScrollToService.scrollTo(
'#execution-' + ReactInjector.$stateParams.executionId,
'.all-execution-groups',
225,
delay,
);
}

public handleTransitionSuccess(transition: Transition): void {
const toParams = transition.params('to');
const fromParams = transition.params('from');
// if we're navigating to a different execution on the same page, scroll the new execution into view
// or, if we are navigating back to the same execution after scrolling down the page, scroll it into view
// but don't scroll it into view if we're navigating to a different stage in the same execution
let shouldScroll = false;
if (
transition.to.name.indexOf(transition.from.name) === 0 &&
toParams.application === fromParams.application &&
toParams.executionId
) {
shouldScroll = true;
if (toParams.executionId === fromParams.executionId && toParams.details) {
if (
toParams.stage !== fromParams.stage ||
toParams.step !== fromParams.step ||
toParams.details !== fromParams.details
) {
shouldScroll = false;
}
}
}
if (shouldScroll) {
this.scrollIntoView(0);
}
}

public componentDidMount(): void {
const { app } = this.props;
if (ExecutionState.filterModel.mostRecentApplication !== app.name) {
Expand All @@ -218,9 +197,6 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
});

this.groupsUpdatedSubscription = ExecutionFilterService.groupsUpdatedStream.subscribe(() => this.groupsUpdated());
this.locationChangeUnsubscribe = ReactInjector.$uiRouter.transitionService.onSuccess({}, t =>
this.handleTransitionSuccess(t),
);

this.executionsRefreshUnsubscribe = app.executions.onRefresh(
null,
Expand All @@ -240,9 +216,6 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta

$q.all([app.executions.ready(), app.pipelineConfigs.ready()]).then(() => {
this.updateExecutionGroups();
if (ReactInjector.$stateParams.executionId) {
this.scrollIntoView();
}
const nameOrIdToStart = ReactInjector.$stateParams.startManualExecution;
if (nameOrIdToStart) {
const toStart = app.pipelineConfigs.data.find((p: IPipeline) => [p.id, p.name].includes(nameOrIdToStart));
Expand All @@ -262,7 +235,6 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
app.pipelineConfigs.deactivate();
this.executionsRefreshUnsubscribe();
this.groupsUpdatedSubscription.unsubscribe();
this.locationChangeUnsubscribe();
this.activeRefresher && this.activeRefresher.unsubscribe();
this.state.poll && this.state.poll.cancel();
}
Expand Down Expand Up @@ -303,7 +275,7 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta

public render(): React.ReactElement<Executions> {
const { app } = this.props;
const { filtersExpanded, loading, sortFilter, tags, triggeringExecution } = this.state;
const { filtersExpanded, loading, sortFilter, tags, triggeringExecution, reloadingForFilters } = this.state;

const hasPipelines = !!(get(app, 'executions.data', []).length || get(app, 'pipelineConfigs.data', []).length);

Expand Down Expand Up @@ -338,7 +310,7 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
<i className="fa fa-backward" />
</Tooltip>
</a>
{!loading && <ExecutionFilters application={app} />}
{!loading && <ExecutionFilters application={app} setReloadingForFilters={this.setReloadingForFilters} />}
</div>
<div
className={`full-content ${filtersExpanded ? 'filters-expanded' : ''} ${
Expand Down Expand Up @@ -447,7 +419,7 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
<Spinner size="medium" />
</div>
)}
{app.executions.reloadingForFilters && (
{reloadingForFilters && (
<div className="text-center transition-overlay" style={{ marginLeft: '-25px' }} />
)}
{!loading && !hasPipelines && (
Expand All @@ -460,7 +432,7 @@ export class Executions extends React.Component<IExecutionsProps, IExecutionsSta
<h4>There was an error loading executions. We'll try again shortly.</h4>
</div>
)}
{!this.state.loading && hasPipelines && <ExecutionGroups application={app} />}
{!loading && hasPipelines && <ExecutionGroups application={app} />}
</div>
</div>
</div>
Expand Down
Loading

0 comments on commit d5425b4

Please sign in to comment.