Skip to content

feat(ui): move buttons to canvas for better performance#26268

Merged
chirag-madlani merged 17 commits intomainfrom
fix-pipeline-btn
Mar 7, 2026
Merged

feat(ui): move buttons to canvas for better performance#26268
chirag-madlani merged 17 commits intomainfrom
fix-pipeline-btn

Conversation

@chirag-madlani
Copy link
Collaborator

This pull request introduces a new CanvasButtonPopover component to improve the user experience when interacting with pipeline buttons on lineage edges, and refactors the edge interaction logic to support this feature. It also removes the old PipelineEdgeButtons component and updates related styles and tests. The changes are grouped into three main themes: new feature implementation, refactoring and cleanup, and style improvements.

New Feature: CanvasButtonPopover

  • Added a new CanvasButtonPopover component that displays detailed information about a pipeline when a user hovers over a pipeline button on a lineage edge. This popover includes the pipeline's fully qualified name, type, and status, and is positioned absolutely based on the viewport. [1] [2]
  • Introduced comprehensive unit tests for CanvasButtonPopover, covering rendering, props, mouse interactions, and status color display.

Refactoring and Cleanup

  • Refactored CanvasEdgeRenderer.component.tsx to integrate the new popover:
    • Added logic to track hovered buttons and edges, manage popover state, and handle mouse events via a new useCanvasMouseEvents hook.
    • Removed the old PipelineEdgeButtons component and its references, simplifying the rendering logic. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]
  • Updated the import of getAbsolutePosition to use a shared utility, ensuring consistency across components. [1] [2]
  • Updated the useCanvasEdgeRenderer hook to expose new button-related methods and state for use in the renderer.

Style Improvements

  • Improved the StyledIconButton component by increasing the padding and standardizing icon size for better visual consistency.

These changes collectively modernize the edge interaction experience, improve code maintainability, and lay the groundwork for further enhancements to lineage visualization.

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

zIndex: 1000,
}}
onMouseEnter={() => {
isOverPopoverRef.current = true;
Copy link

Choose a reason for hiding this comment

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

⚠️ Bug: onMouseEnter guard prevents isOverPopoverRef from ever being set

The onMouseEnter handler on line 56 has a conditional if (isOverPopoverRef.current) that guards the assignment isOverPopoverRef.current = true. Since isOverPopoverRef is initialized to false (line 59 in CanvasEdgeRenderer.component.tsx), this condition will never pass on the first entry, meaning the ref stays false forever.

This breaks the popover hover-to-stay-open behavior: when the user moves their mouse from the canvas button into the popover, the 100ms timeout in handleMouseMove fires and calls setHoveredButtonRef.current?.(null), closing the popover before the user can interact with it. The isOverPopoverRef check that's supposed to prevent this (lines 117-121 of useCanvasMouseEvents.ts) never triggers because the ref is never set to true.

Suggested fix:

onMouseEnter={() => {
    isOverPopoverRef.current = true;
    if (hoverTimeoutRef.current) {
      clearTimeout(hoverTimeoutRef.current);
      hoverTimeoutRef.current = null;
    }
  }}

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

// Path2D.isPointInStroke requires a CanvasRenderingContext2D to resolve
// the current lineWidth and transform, so we keep one around.
const canvasButtonsRef = useRef<CanvasButtonHitData[]>([]);
const hoveredButtonRef = useRef<CanvasButton | null>(null);
Copy link

Choose a reason for hiding this comment

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

💡 Performance: hoveredButton state in draw deps causes full canvas redraw on hover

The hoveredButton state (via useState) is listed in the dependency array of the main draw/render callback (line 345 of useCanvasEdgeRenderer.ts). Every time the user hovers over or away from a button, hoveredButton changes, which triggers a full canvas redraw of all edges and buttons.

Consider using a ref for the hovered state and only redrawing the affected button area (or at minimum, debouncing the redraw) to avoid unnecessary full-canvas repaints on every mouse movement over buttons.

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

@gitar-bot
Copy link

gitar-bot bot commented Mar 6, 2026

🔍 CI failure analysis for 5364586: Current CI run (job 66083331587) shows no lineage-related failures; all failures are unrelated to this PR's changes. The previous PR-related `LineageSettings.spec.ts:284` pointer-events issue did not recur in this run.

Issue

CI failure in playwright-ci-postgresql (3, 6) — failures appear unrelated to this PR.

Current Run Failures (job 66083331587)

No lineage, canvas, or pointer-events failures were detected. The failures are:

QueryEntity.spec.ts:61 — Unrelated (vote count assertion)

Expected vote count "1" but received "0". This is a timing/infrastructure issue with the voting mechanism, not caused by this PR.

Flaky Tests (unrelated)

  • BulkImport.spec.ts:394 — timeout waiting for radio-btn-Certification.Gold
  • DataProductRename.spec.ts:221locator.fill received undefined query parameter
  • GlossaryAssets.spec.ts:313 — strict mode violation: glossary-container resolved to 8 elements
  • ImpactAnalysis.spec.ts:377page.waitForResponse: browser closed
  • GlossaryPermissions.spec.ts:365page.waitForResponse: browser closed
  • ServiceEntityPermissions.spec.ts:78edit-description locator not found

None of these involve lineage, canvas edges, pipeline buttons, or pointer events.

Previous Run Note

The previous CI run (playwright-ci-postgresql (4, 6)) had a PR-related failure in LineageSettings.spec.ts:284, where clicking pipeline-label-* elements timed out because react-flow__pane intercepted pointer events (the pipeline-label-* divs were inside a container with pointerEvents: none). That failure did not recur in this run, suggesting it may have been flaky or was resolved by a subsequent commit.

Code Review ⚠️ Changes requested 1 resolved / 5 findings

Moving buttons to canvas for performance optimization, but the onMouseEnter guard prevents popover state from updating, unused dead code in CanvasMouseEventUtils exists, hoveredButton in draw dependencies causes excessive redraws, and edge path cache isn't invalidated when dependencies change. Address these issues before merging.

⚠️ Bug: onMouseEnter guard prevents isOverPopoverRef from ever being set

📄 openmetadata-ui/src/main/resources/ui/src/components/Entity/EntityLineage/CanvasButtonPopover.component.tsx:56

The onMouseEnter handler on line 56 has a conditional if (isOverPopoverRef.current) that guards the assignment isOverPopoverRef.current = true. Since isOverPopoverRef is initialized to false (line 59 in CanvasEdgeRenderer.component.tsx), this condition will never pass on the first entry, meaning the ref stays false forever.

This breaks the popover hover-to-stay-open behavior: when the user moves their mouse from the canvas button into the popover, the 100ms timeout in handleMouseMove fires and calls setHoveredButtonRef.current?.(null), closing the popover before the user can interact with it. The isOverPopoverRef check that's supposed to prevent this (lines 117-121 of useCanvasMouseEvents.ts) never triggers because the ref is never set to true.

Suggested fix
      onMouseEnter={() => {
          isOverPopoverRef.current = true;
          if (hoverTimeoutRef.current) {
            clearTimeout(hoverTimeoutRef.current);
            hoverTimeoutRef.current = null;
          }
        }}
⚠️ Bug: Edge path cache not invalidated when dependencies change

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:107 📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:233

The edgePathCacheRef WeakMap is only cleared when isRepositioning is true (line 234). However, edge path coordinates depend on columnsInCurrentPages, nodes, and getNode — all of which can change independently of repositioning.

When columnsInCurrentPages changes (e.g., a user expands a column in a node), getEdgeCoordinates would compute different coordinates, but the WeakMap still holds stale entries keyed on the same Edge object references (ReactFlow reuses Edge objects when only surrounding context changes, not the edges themselves).

This means edges and their canvas buttons will render at incorrect positions until the user triggers a repositioning action that clears the cache.

The cache should be invalidated whenever any coordinate-affecting dependency changes — either by clearing the WeakMap when those deps change, or by incorporating a cache-busting key.

Suggested fix
  const drawAllEdges = useCallback(() => {
    const canvas = canvasRef.current;

    if (!canvas || !containerWidth || !containerHeight) {
      return;
    }

    const ctx = setupCanvas(canvas, containerWidth, containerHeight);
    ctx.clearRect(0, 0, containerWidth, containerHeight);

    // Clear cache on every full redraw so coordinate changes are picked up
    edgePathCacheRef.current = new WeakMap();

    if (isRepositioning) {
💡 Quality: CanvasMouseEventUtils.ts is unused dead code

📄 openmetadata-ui/src/main/resources/ui/src/utils/CanvasMouseEventUtils.ts:1

CanvasMouseEventUtils.ts defines createCanvasClickHandler, createCanvasMouseMoveHandler, and createCanvasMouseLeaveHandler — factory functions that duplicate the logic in useCanvasMouseEvents.ts (the hook). A grep confirms no file imports from CanvasMouseEventUtils.ts. This adds ~139 lines of dead code that will need to be maintained in parallel with the hook version, creating a divergence risk.

💡 Performance: hoveredButton state in draw deps causes full canvas redraw on hover

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:68 📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:345

The hoveredButton state (via useState) is listed in the dependency array of the main draw/render callback (line 345 of useCanvasEdgeRenderer.ts). Every time the user hovers over or away from a button, hoveredButton changes, which triggers a full canvas redraw of all edges and buttons.

Consider using a ref for the hovered state and only redrawing the affected button area (or at minimum, debouncing the redraw) to avoid unnecessary full-canvas repaints on every mouse movement over buttons.

✅ 1 resolved
Bug: CanvasButtonPopover shows broken popover for function buttons

📄 openmetadata-ui/src/main/resources/ui/src/components/Entity/EntityLineage/CanvasButtonPopover.component.tsx:44 📄 openmetadata-ui/src/main/resources/ui/src/components/Entity/EntityLineage/CanvasEdgeRenderer.component.tsx:223 📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:298
CanvasButtonPopover always renders an EntityPopOverCard using hoveredEdge.data?.edge?.pipeline data (lines 44-45). However, canvas buttons are created for both 'pipeline' and 'function' types (useCanvasEdgeRenderer.ts line 298-309). When hovering over a function-type button, pipelineData will be undefined, causing EntityPopOverCard to receive empty strings for entityFQN and entityType, resulting in a broken/empty popover appearing on hover.

The rendering condition in CanvasEdgeRenderer.component.tsx (line 223) does not filter by button type: {hoveredButton && hoveredEdge && !isEditMode && (, so the popover renders for all button types indiscriminately.

Either:

  1. Filter out function buttons from showing popover: hoveredButton?.type === 'pipeline' && hoveredEdge && !isEditMode
  2. Or render different content in the popover based on button type.
🤖 Prompt for agents
Code Review: Moving buttons to canvas for performance optimization, but the `onMouseEnter` guard prevents popover state from updating, unused dead code in CanvasMouseEventUtils exists, `hoveredButton` in draw dependencies causes excessive redraws, and edge path cache isn't invalidated when dependencies change. Address these issues before merging.

1. ⚠️ Bug: onMouseEnter guard prevents isOverPopoverRef from ever being set
   Files: openmetadata-ui/src/main/resources/ui/src/components/Entity/EntityLineage/CanvasButtonPopover.component.tsx:56

   The `onMouseEnter` handler on line 56 has a conditional `if (isOverPopoverRef.current)` that guards the assignment `isOverPopoverRef.current = true`. Since `isOverPopoverRef` is initialized to `false` (line 59 in `CanvasEdgeRenderer.component.tsx`), this condition will never pass on the first entry, meaning the ref stays `false` forever.
   
   This breaks the popover hover-to-stay-open behavior: when the user moves their mouse from the canvas button into the popover, the 100ms timeout in `handleMouseMove` fires and calls `setHoveredButtonRef.current?.(null)`, closing the popover before the user can interact with it. The `isOverPopoverRef` check that's supposed to prevent this (lines 117-121 of `useCanvasMouseEvents.ts`) never triggers because the ref is never set to `true`.

   Suggested fix:
   onMouseEnter={() => {
       isOverPopoverRef.current = true;
       if (hoverTimeoutRef.current) {
         clearTimeout(hoverTimeoutRef.current);
         hoverTimeoutRef.current = null;
       }
     }}

2. 💡 Quality: CanvasMouseEventUtils.ts is unused dead code
   Files: openmetadata-ui/src/main/resources/ui/src/utils/CanvasMouseEventUtils.ts:1

   `CanvasMouseEventUtils.ts` defines `createCanvasClickHandler`, `createCanvasMouseMoveHandler`, and `createCanvasMouseLeaveHandler` — factory functions that duplicate the logic in `useCanvasMouseEvents.ts` (the hook). A grep confirms no file imports from `CanvasMouseEventUtils.ts`. This adds ~139 lines of dead code that will need to be maintained in parallel with the hook version, creating a divergence risk.

3. 💡 Performance: `hoveredButton` state in draw deps causes full canvas redraw on hover
   Files: openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:68, openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:345

   The `hoveredButton` state (via `useState`) is listed in the dependency array of the main draw/render callback (line 345 of `useCanvasEdgeRenderer.ts`). Every time the user hovers over or away from a button, `hoveredButton` changes, which triggers a full canvas redraw of all edges and buttons. 
   
   Consider using a ref for the hovered state and only redrawing the affected button area (or at minimum, debouncing the redraw) to avoid unnecessary full-canvas repaints on every mouse movement over buttons.

4. ⚠️ Bug: Edge path cache not invalidated when dependencies change
   Files: openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:107, openmetadata-ui/src/main/resources/ui/src/hooks/useCanvasEdgeRenderer.ts:233

   The `edgePathCacheRef` WeakMap is only cleared when `isRepositioning` is true (line 234). However, edge path coordinates depend on `columnsInCurrentPages`, `nodes`, and `getNode` — all of which can change independently of repositioning.
   
   When `columnsInCurrentPages` changes (e.g., a user expands a column in a node), `getEdgeCoordinates` would compute different coordinates, but the WeakMap still holds stale entries keyed on the same Edge object references (ReactFlow reuses Edge objects when only surrounding context changes, not the edges themselves).
   
   This means edges and their canvas buttons will render at incorrect positions until the user triggers a repositioning action that clears the cache.
   
   The cache should be invalidated whenever any coordinate-affecting dependency changes — either by clearing the WeakMap when those deps change, or by incorporating a cache-busting key.

   Suggested fix:
   const drawAllEdges = useCallback(() => {
     const canvas = canvasRef.current;
   
     if (!canvas || !containerWidth || !containerHeight) {
       return;
     }
   
     const ctx = setupCanvas(canvas, containerWidth, containerHeight);
     ctx.clearRect(0, 0, containerWidth, containerHeight);
   
     // Clear cache on every full redraw so coordinate changes are picked up
     edgePathCacheRef.current = new WeakMap();
   
     if (isRepositioning) {

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 6, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants