perf: Optimize the rendering logic of front-end nodes

[shaohuzhang1]

perf: Optimize the rendering logic of front-end nodes

[f2c-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[f2c-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaohuzhang1]

{
  "suggestions": [
    "Remove unnecessary `use` statements inside `renderVueComponent`. Move them to the top or appropriate scope.",
    "Consider using TypeScript interfaces and type checking to avoid runtime errors due to incorrect prop types.",
    "If `isActive()` returns true, ensure that `connect`, `targetId`, and related functions (`disconnect`) handle all necessary cases properly.",
    "Move static data like `ElementPlusIcons` outside components for better organization and performance.",
    "Add proper error handling in lifecycle hooks, such as `componentDidMount` and `componentWillUnmount`.",
    "Evaluate whether `unmount` can be merged with `unmountVueComponent`.

[
  {
    "message": "Consider replacing manual event listeners (onClick for example) with more efficient reactive state management approaches."
  },
  {
    "message": "Ensure that the HTML elements returned by your custom component correctly reflect the current state of the model."
  },
  {
    "message": "Check for potential memory leaks by monitoring instances created throughout execution time."
  },
  {
    "message": "Optimize CSS classes and styles used within the Vue component tree.")
  },
  {

[shaohuzhang1]

The provided code has several potential issues and optimizations:

Potential Issues

1. Type Safety: There are some type errors due to the lack of explicit types for variables like item, nodes, and props. TypeScript should be used to ensure these are correctly typed.

2. Error Handling: The current implementation does not handle edge cases such as empty inputs or missing dependencies.

3. Debugging Information: The code lacks comments that explain each part's purpose, which can make maintenance more difficult.

4. Optimization Overhead: Using reactive objects with large datasets might cause overhead when accessing them frequently within a loop.

5. Vue Lifecycle Concerns: Ensuring proper lifecycle hooks are used where necessary (e.g., beforeDestroy).

6. Scalability: This solution assumes a single instance of ActiveItemTeleporter, which may lead to conflicts when multiple instances exist simultaneously unless handled carefully.

Optimization Suggestions

1. Use TypeScript Annotations: Add type annotations to improve readability and maintainability.

   import {
     BaseEdgeModel,
     BaseNodeModel,
     GraphModel
   } from '@logicflow/core';
   import { defineComponent, h, reactive, isVue3, Teleport, markRaw, Fragment } from 'vue-demi';
   
   let active = false;
   const items: { [key: string]: any } = {};
   
   export function connect(
     id: string,
     component: any,
     container: HTMLDivElement,
     node: BaseNodeModel | BaseEdgeModel,
     graph: GraphModel,
     get_props?: any
   ) {
     // ...
   }
   
   export function disconnect(id: string) {
     if (active) {
       delete items[id];
     }
   }
   
   export function isActive() {
     return active;
   }
   
   export function getTeleport(): any {
     // ...

2. Handle Empty Inputs: Implement checks to ensure all necessary parameters are provided before proceeding.

3. Simplify Error Messages: Use more descriptive error messages instead of generic ones to aid debugging.

4. Refactor for Better Scopes: Consider refactoring the logic to avoid redundant operations and improve performance.

5. Use Memoization: If needed, consider using memoization techniques to reduce computational costs.

Here is an example of how you might implement some improvements based on the suggestions above:

import {
  BaseEdgeModel,
  BaseNodeModel,
  GraphModel
} from '@logicflow/core';
import { defineComponent, h, reactive, isVue3, Teleport, markRaw, Fragment } from 'vue-demi';

interface ActiveItems {
  [key: string]: any;
}

let active = false;
const items: ActiveItems = {};
const lastVisibleFlowId: Set<string> = new Set();

export function connect(
  id: string,
  component: any,
  container: HTMLDivElement,
  node: BaseNodeModel | BaseEdgeModel,
  graph: GraphModel,
  get_props?: (node: BaseNodeModel | BaseEdgeModel, graph: GraphModel) => {}
): void {
  if (!get_props) {
    get_props = ((_: BaseNodeModel | BaseEdgeModel, __: GraphModel) =>
      ({}) as Record<string, unknown>) as any; // Fallback default
  }
  
  if (!graph || !container) {
    console.error("Invalid input arguments");
    return;
  }
  
  const nodeId = `${id}-${graph.id}`;
  items[nodeId] = markRaw(defineComponent({
    render: () => (
      <Teleport to={container}>
        {/* Render the wrapped component */}
        <component {...get_props(node, graph)} />
      </Teleport>
    ),
    provide() {
      return {
        getNode: () => node,
        getGraph: () => graph
      };
    }
  }));
}

export function disconnect(id: string) {
  // Remove specific item associated with this ID per graph
  if (graph && container) {
    const nodeId = `${id}-${graph.id}`;
    if (items[nodeId]) {
      delete items[nodeId];
    }
  }
}

export function.isActive(): boolean {
  return active;
}

// Simplified version without extensive comment blocks
function processRender(children: Array<any>, propSetIds: Set<String>): JSX.Element {
  return (
    <Fragment>
      {children.filter(child => propSetIds.has(child.key))}
    </Fragment>
  );
}

/*
... rest of code remains unchanged
*/

This version includes basic validation and removes unnecessary complexity while maintaining the core functionality. Adjustments can continue depending on further needs and testing.

[shaohuzhang1]

The provided code seems functional but contains a few areas that could be improved:

1. Unused Variables: The variable customHeight is declared and assigned but never used.

2. TypeScript Usage: TypeScript should be imported at the top to ensure type safety.

3. Comments Consistency: Comments need consistency throughout the codebase for better readability.

Here's a revised version with these improvements:

// Import statements for typescript support and other modules
import { BezierEdge, BezierEdgeModel, h } from '@logicflow/core';
import { createApp, h as vh } from 'vue';

import { isActive, connect, disconnect } from './teleport';
import CustomLine from './CustomLine.vue';

function isMouseInElement(element: any, e: any) {
  const rect = element.getBoundingClientRect();
}

const DEFAULT_WIDTH = 32;
const DEFAULT_HEIGHT = 32;

class CustomEdge2 extends BezierEdge {
  private isMounted = false;
  private customLineApp?: any;
  private root?: HTMLElement | string;

  constructor() {
    super();
    this.isMounted = false;
  }

  protected renderVueComponent(root: HTMLElement | string): void | never {
    this.unmountVueComponent();

    if (root === 'string') {
      throw new Error('Root must be an HTMLElement');
    }

    if (isActive()) {
      connect(
        this.targetId(),
        CustomLine,
        root,
        this.props.model,
        (node: any, graph: any) => ({ model: node, graph }),
        (error) => console.error(`Failed to connect edge: ${error}`)
      );
    } else {
      this.customLineApp = createApp({
        render: () => vh(CustomLine, { model: this.props.model })
      });
      try {
        this.customLineApp?.mount(root);
      } catch (error) {
        console.error(`failed to mount Vue component`, error);
      }
    }
  }

  protected targetId(): string {
    return `${this.props.graphModel.flowId}:${this.props.model.id}`;
  }

  public componentWillUnmount(): void {
    if (super.componentWillUnmount) {
      super.componentWillUnmount();
    }

    if (isActive()) {
      disconnect(this.targetId());
    }

    this.unmountVueComponent();
  }

  public unmountVueComponent(): void | never {
    if (this.customLineApp) {
      try {
        this.customLineApp.unmount();
        this.customLineApp = null;
      } catch (error) {
        console.error("Failed to unmount vue component", error);
      }
    }

    // Clear content of root element if exists
    if (typeof this.root !== 'undefined' && typeof this.root?.removeChild === 'function') {
      while (this.root.firstChild != null) {
        this.root.removeChild(this.root.firstChild);
      }
    } else {
      document.getElementById("" + this.root)?.innerHTML ?? '';
    }

    return this.root;
  }

  getEdge(): any {
    const { model } = this.props;
    const id = `edge-${model.id}`;

    this.renderContainer = document.createElement('div');
    this.renderContainer.style.position = "absolute";
    this.renderContainer.style.pointerEvents = "none";

    const s = document.getElementById(id);

    let width = model.attrs.path[0] > model.attrs.path[model.attrs.path.length - 2]
      ? Math.abs(model.attrs.path[0] - model.attrs.path[model.attrs.path.length - 2]) +
        (DEFAULT_WIDTH - 6)
      : DEFAULT_WIDTH;
    const height = model.attrs.path[1] > model.attrs.path[model.attrs.path.length - 3]
      ? Math.abs(model.attrs.path[1] - model.attrs.path[model.attrs.path.length - 3]) +
        (DEFAULT_HEIGHT - 6)
      : DEFAULT_HEIGHT;

    

These changes address some common issues found in the original code and prepare it for better maintenance and scalability.