Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions ui/src/app/applications/components/utils.quickstart.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import * as React from 'react';
import * as renderer from 'react-test-renderer';
import {BehaviorSubject, Subscription} from 'rxjs';

import {ContextApis} from '../../shared/context';
import {AbstractApplication, Application, ApplicationTree, ResourceNode} from '../../shared/models';
import {services} from '../../shared/services';
import {renderResourceButtons} from './utils';

describe('renderResourceButtons', () => {
const resource = {
uid: 'apps/Deployment/default/guestbook',
group: 'apps',
kind: 'Deployment',
namespace: 'default',
name: 'guestbook'
} as ResourceNode;

const otherResource = {
...resource,
uid: 'apps/Deployment/default/guestbook-canary',
name: 'guestbook-canary'
} as ResourceNode;

const application = {
kind: 'Application',
metadata: {name: 'guestbook', namespace: 'argocd'},
spec: {project: 'default'},
status: {
resources: [
{
group: resource.group,
kind: resource.kind,
namespace: resource.namespace,
name: resource.name
}
]
}
} as Application;

const tree = {
nodes: [resource]
} as ApplicationTree;

const apis = {
popup: {prompt: jest.fn()},
notifications: {show: jest.fn()},
navigation: {goto: jest.fn()},
baseHref: ''
} as unknown as ContextApis;

afterEach(() => {
jest.restoreAllMocks();
});

function QuickStartWrapper(props: {resourceNode: ResourceNode; version: number; appChanged: BehaviorSubject<AbstractApplication>}) {
return <div data-version={props.version}>{renderResourceButtons(props.resourceNode, application, tree, apis, props.appChanged)}</div>;
}

it('does not reload quickstart actions on unrelated parent rerenders', () => {
const canISpy = jest.spyOn(services.accounts, 'canI').mockResolvedValue(false);
const appChanged = new BehaviorSubject<AbstractApplication>(application);
const subscription: Subscription = appChanged.subscribe(() => undefined);
let rendered: renderer.ReactTestRenderer | undefined;

try {
renderer.act(() => {
rendered = renderer.create(<QuickStartWrapper resourceNode={resource} version={0} appChanged={appChanged} />);
});

expect(canISpy).toHaveBeenCalledTimes(1);

renderer.act(() => {
rendered!.update(<QuickStartWrapper resourceNode={resource} version={1} appChanged={appChanged} />);
});

expect(canISpy).toHaveBeenCalledTimes(1);
} finally {
subscription.unsubscribe();
}
});

it('reloads quickstart actions when the resource changes', () => {
const canISpy = jest.spyOn(services.accounts, 'canI').mockResolvedValue(false);
const appChanged = new BehaviorSubject<AbstractApplication>(application);
const subscription: Subscription = appChanged.subscribe(() => undefined);
let rendered: renderer.ReactTestRenderer | undefined;

try {
renderer.act(() => {
rendered = renderer.create(<QuickStartWrapper resourceNode={resource} version={0} appChanged={appChanged} />);
});

renderer.act(() => {
rendered!.update(<QuickStartWrapper resourceNode={otherResource} version={0} appChanged={appChanged} />);
});

expect(canISpy).toHaveBeenCalledTimes(2);
} finally {
subscription.unsubscribe();
}
});

it('reloads quickstart actions when the appChanged subject identity changes', () => {
const canISpy = jest.spyOn(services.accounts, 'canI').mockResolvedValue(false);
const firstAppChanged = new BehaviorSubject<AbstractApplication>(application);
const secondAppChanged = new BehaviorSubject<AbstractApplication>(application);
const firstSubscription: Subscription = firstAppChanged.subscribe(() => undefined);
const secondSubscription: Subscription = secondAppChanged.subscribe(() => undefined);
let rendered: renderer.ReactTestRenderer | undefined;

try {
renderer.act(() => {
rendered = renderer.create(<QuickStartWrapper resourceNode={resource} version={0} appChanged={firstAppChanged} />);
});

renderer.act(() => {
rendered!.update(<QuickStartWrapper resourceNode={resource} version={0} appChanged={secondAppChanged} />);
});

expect(canISpy).toHaveBeenCalledTimes(2);
} finally {
firstSubscription.unsubscribe();
secondSubscription.unsubscribe();
}
});
});
62 changes: 53 additions & 9 deletions ui/src/app/applications/components/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -891,16 +891,49 @@ export function renderResourceActionMenu(menuItems: ActionMenuItem[]): React.Rea
);
}

export function renderResourceButtons(
resource: ResourceTreeNode,
application: appModels.AbstractApplication,
tree: appModels.AbstractApplicationTree,
apis: ContextApis,
appChanged: BehaviorSubject<appModels.AbstractApplication>
): React.ReactNode {
const menuItems: Observable<ActionMenuItem[]> = getActionItems(resource, application, tree, apis, appChanged, true);
interface ResourceButtonsProps {
resource: ResourceTreeNode;
application: appModels.AbstractApplication;
tree: appModels.AbstractApplicationTree;
apis: ContextApis;
appChanged: BehaviorSubject<appModels.AbstractApplication>;
}

interface ResourceButtonsInput {
resource: ResourceTreeNode;
application: appModels.AbstractApplication;
tree: appModels.AbstractApplicationTree;
apisId: number;
appChangedId: number;
}

const objectIds = new WeakMap<object, number>();
let nextObjectId = 1;

function getObjectId(value: object) {
let id = objectIds.get(value);
if (id === undefined) {
id = nextObjectId++;
objectIds.set(value, id);
}
return id;
}

// Keep quickstart action loading stable across unrelated PodView rerenders.
const ResourceButtons = React.memo(({resource, application, tree, apis, appChanged}: ResourceButtonsProps) => {
const input = React.useMemo(
() => ({
resource,
application,
tree,
apisId: getObjectId(apis),
appChangedId: getObjectId(appChanged)
}),
[resource, application, tree, apis, appChanged]
);

return (
<DataLoader load={() => menuItems}>
<DataLoader input={input} load={(value: ResourceButtonsInput) => getActionItems(value.resource, value.application, value.tree, apis, appChanged, true)}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, this looks good to me.
I wonder if quick start actions could still reload on unrelated watch-driven resource tree updates, since input still includes the full application and tree. Would it make sense to narrow input to only the fields needed to resolve quick start actions?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I narrowed the DataLoader input in the latest update so it no longer depends on the full application / tree objects.

It now keys off only the fields that affect quickstart resolution for this path:

  • the selected resource identity
  • whether the resource is managed
  • the derived child-resource set used by delete handling
  • whether a logs target exists
  • the application identity/project
  • stable identities for the underlying apis managers and appChanged

I also added regression coverage for the case you called out: refreshed application / tree objects that do not change the effective quickstart behavior no longer reload quickstart actions.

There was one similar issue with the recreated apis wrapper as well, and I narrowed that too. The latest push includes both fixes; checks are rerunning on it now.

{items => (
<div className='pod-view__node__quick-start-actions'>
{items.map((item, i) => (
Expand All @@ -922,6 +955,17 @@ export function renderResourceButtons(
)}
</DataLoader>
);
});
ResourceButtons.displayName = 'ResourceButtons';

export function renderResourceButtons(
resource: ResourceTreeNode,
application: appModels.AbstractApplication,
tree: appModels.AbstractApplicationTree,
apis: ContextApis,
appChanged: BehaviorSubject<appModels.AbstractApplication>
): React.ReactNode {
return <ResourceButtons resource={resource} application={application} tree={tree} apis={apis} appChanged={appChanged} />;
}

export function syncStatusMessage(app: appModels.Application) {
Expand Down
Loading