Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

fix(vscode): Adding a step in Kaoto VSCode takes several seconds #1338

Merged
merged 1 commit into from
Mar 3, 2023
Merged

fix(vscode): Adding a step in Kaoto VSCode takes several seconds #1338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/components/MiniCatalog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const MiniCatalog = (props: IMiniCatalog) => {
});
}
searchInputRef.current?.focus();
}, [dsl]);
}, []);

const changeSearch = (e: string) => {
setQuery(e);
Expand Down
7 changes: 6 additions & 1 deletion src/components/Visualization.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@
right: -20px !important;
}

.reactflow-wrapper {
width: 100vw;
height: calc(100vh - 77px);
}

.stepNode {
align-items: center;
background: var(--pf-global--BackgroundColor--100);
Expand Down Expand Up @@ -51,7 +56,7 @@
left: 0;
top: 0;
}

.stepNode__Icon {
position: relative;
top: 55%;
Expand Down
37 changes: 14 additions & 23 deletions src/components/Visualization.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
import { StepsService, VisualizationService } from '@kaoto/services';
import { useIntegrationJsonStore, useNestedStepsStore, useVisualizationStore } from '@kaoto/store';
import { IStepProps, IVizStepNode } from '@kaoto/types';
import { useEffect, useMemo, useRef, useState } from 'react';
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import ReactFlow, { Background, Viewport } from 'reactflow';

const Visualization = () => {
Expand All @@ -25,8 +25,6 @@ const Visualization = () => {
zoom: 1.2,
};
const [isPanelExpanded, setIsPanelExpanded] = useState(false);
const [, setReactFlowInstance] = useState(null);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kahboom is this needed? I think that it forces a render but I don't know if there another use case for it 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

confession time--I have no idea. I think that might have been from an older version of React Flow, but I can't be sure. 😅 We can remove it, just be sure to check rendering still happens as needed with the basic actions (for example replacing from big catalog, deleting steps, editing config, etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm, I think that you're right, I checked here and it seems that it's also deprecated.

const reactFlowWrapper = useRef(null);
const [selectedStep, setSelectedStep] = useState<IStepProps>({
maxBranches: 0,
minBranches: 0,
Expand All @@ -40,7 +38,10 @@ const Visualization = () => {
const previousLayout = useRef(visualizationStore.layout);
const previousIntegrationJson = useRef(integrationJsonStore.integrationJson);
const visualizationService = new VisualizationService(integrationJsonStore, visualizationStore);
const stepsService = new StepsService(integrationJsonStore, nestedStepsStore, visualizationStore);
const stepsService = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 just curious, should useMemo() be the default way of instantiating (and caching) service instances? a "memo" for #1233

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a very interesting question 😃

From my point of view, I don't think that this is the definite way to go but a transition point.

At first, the onNodeClick function was being recreated on every render pass, together with the stepService, so as I wrapped onNodeClick function into a useCallback() hook to avoid recreating it, stepService now it's a required dependency of it, so I wrapped the service with useMemo() to recreate it only if something change.

I think that a more appropriate fix would be to have this service as a context, this way we wouldn't need to create the service in different places but just call the useContext() hook where needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

aha! makes sense, thanks 👍

() => new StepsService(integrationJsonStore, nestedStepsStore, visualizationStore),
[integrationJsonStore, nestedStepsStore, visualizationStore],
);

// initial loading of visualization steps
useEffect(() => {
Expand Down Expand Up @@ -109,7 +110,7 @@ const Visualization = () => {
* @param e
* @param node
*/
const onNodeClick = (e: any, node: IVizStepNode) => {
const onNodeClick = useCallback((e: any, node: IVizStepNode) => {
// here we check if it's a node or edge
// workaround for https://github.com/wbkd/react-flow/issues/2202
if (!e.target.classList.contains('stepNode__clickable')) return;
Expand All @@ -132,11 +133,7 @@ const Visualization = () => {
visualizationStore.setSelectedStepUuid('');
}
}
};

const onLoad = (_reactFlowInstance: any) => {
setReactFlowInstance(_reactFlowInstance);
};
}, [isPanelExpanded, selectedStep.UUID, stepsService, visualizationStore]);

/**
* Handles Step View configuration changes
Expand All @@ -152,8 +149,8 @@ const Visualization = () => {
<KaotoDrawer
isExpanded={isPanelExpanded}
data-expanded={isPanelExpanded}
isResizable={true}
dataTestId={'kaoto-right-drawer'}
isResizable
dataTestId="kaoto-right-drawer"
panelContent={
<VisualizationStepViews
step={selectedStep}
Expand All @@ -162,19 +159,14 @@ const Visualization = () => {
saveConfig={saveConfig}
/>
}
position={'right'}
id={'right-resize-panel'}
defaultSize={'500px'}
minSize={'150px'}
position="right"
id="right-resize-panel"
defaultSize="500px"
minSize="150px"
>
<div
className="reactflow-wrapper"
data-testid={'react-flow-wrapper'}
ref={reactFlowWrapper}
style={{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to the corresponding Visualization.css file

width: '100vw',
height: 'calc(100vh - 77px )',
}}
data-testid="react-flow-wrapper"
>
<ReactFlow
nodes={visualizationStore.nodes}
Expand All @@ -186,7 +178,6 @@ const Visualization = () => {
onNodeClick={onNodeClick}
onNodesChange={visualizationStore.onNodesChange}
onEdgesChange={visualizationStore.onEdgesChange}
onLoad={onLoad}
snapToGrid={true}
snapGrid={[15, 15]}
deleteKeyCode={null}
Expand Down