Skip to content

fix: Improperly aligned unfolding sub-items in context menu in data browser #2726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: alpha
Choose a base branch
from
+48 −15
Open

fix: Improperly aligned unfolding sub-items in context menu in data browser #2726

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
53f4b8c
Update ContextMenu.react.js
dblythy Apr 7, 2025
10b2a5c
Merge branch 'alpha' into fix/context-menu-position
mtrezza Apr 7, 2025
18d5c97
Update ContextMenu.react.js
dblythy Apr 9, 2025
7e920cc
Merge branch 'alpha' into fix/context-menu-position
mtrezza Apr 9, 2025
a66522f
lint
mtrezza Apr 10, 2025
848db0f
Merge branch 'alpha' into fix/context-menu-position
mtrezza Apr 13, 2025
830d089
Merge branch 'alpha' into fix/context-menu-position
mtrezza Apr 13, 2025
e220b16
Update ContextMenu.react.js
dblythy Apr 25, 2025
3a901fe
Merge branch 'alpha' into fix/context-menu-position
mtrezza Apr 25, 2025
f572a28
Merge branch 'alpha' into fix/context-menu-position
mtrezza Apr 29, 2025
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
63 changes: 48 additions & 15 deletions src/components/ContextMenu/ContextMenu.react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,40 +9,68 @@ import PropTypes from 'lib/PropTypes';
import React, { useState, useEffect, useRef } from 'react';
import styles from 'components/ContextMenu/ContextMenu.scss';

const getPositionToFitVisibleScreen = ref => {
const getPositionToFitVisibleScreen = (ref, offset = 0, mainItemCount = 0, subItemCount = 0) => {
if (ref.current) {
const elBox = ref.current.getBoundingClientRect();
const y = elBox.y + elBox.height < window.innerHeight ? 0 : 0 - elBox.y + 100;
let y = 0;

const footerHeight = 50;
const lowerLimit = window.innerHeight - footerHeight;
const upperLimit = 0;

if (elBox.bottom > lowerLimit) {
y = lowerLimit - elBox.bottom;
} else if (elBox.top < upperLimit) {
y = upperLimit - elBox.top;
}

const projectedTop = elBox.top + y + offset;
const projectedBottom = projectedTop + elBox.height;

const shouldApplyOffset = subItemCount > mainItemCount;
if (shouldApplyOffset && projectedTop >= upperLimit && projectedBottom <= lowerLimit) {
y += offset;
}

Comment on lines +12 to 34
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Guard against NaN offset & out-of-viewport jumps
getPositionToFitVisibleScreen nicely improves submenu placement, but two edge-cases can still break the computed y:

  1. When mainItemCount === 0 (top-level menu) shouldApplyOffset is always true, yet offset is often 0, so the early projectedTop/Bottom check may skip a necessary upward shift if the root element is already partially off-screen.
  2. parseInt later in the function (see below) may yield NaN, propagating into y and producing style={{ top: NaN }} which React silently drops, leaving the menu in an undefined position.

Consider calculating a safe offset and using Number.isFinite(...) guards:

-    const projectedTop = elBox.top + y + offset;
-    const projectedBottom = projectedTop + elBox.height;
-
-    const shouldApplyOffset = subItemCount > mainItemCount;
-    if (shouldApplyOffset && projectedTop >= upperLimit && projectedBottom <= lowerLimit) {
-      y += offset;
-    }
+    const projectedTop    = elBox.top + y + offset;
+    const projectedBottom = projectedTop + elBox.height;
+
+    const shouldApplyOffset = subItemCount > mainItemCount && offset !== 0;
+    if (
+      shouldApplyOffset &&
+      projectedTop    >= upperLimit &&
+      projectedBottom <= lowerLimit
+    ) {
+      y += offset;
+    }

This prevents a “0-offset no-op” and protects against accidental NaN propagation a few lines later.

🏁 Script executed:

#!/bin/bash
# Search for parseInt usage in ContextMenu.react.js with context
grep -R "parseInt" -n src/components/ContextMenu/ContextMenu.react.js -A3 -B3

Length of output: 351

🏁 Script executed:

#!/bin/bash
# Display the complete getPositionToFitVisibleScreen function for context
ast-grep --pattern $'const getPositionToFitVisibleScreen =($_$, $_$, $_$, $_$) => { $$$ }' --quiet -A 50

Length of output: 288

Guard against NaN offsets and zero-offset no-ops
We should tighten getPositionToFitVisibleScreen to handle two remaining edge‐cases:

  1. Zero-offset no-ops on top-level menus.
    When mainItemCount === 0, subItemCount > mainItemCount is always true yet offset is often 0—so we end up skipping a needed reposition for a root menu already off-screen.
  2. NaN from parseInt.
    If window.getComputedStyle(...).top is "auto" (or any non-numeric), parseInt(...,10) returns NaN, which propagates into y and yields style={{ top: NaN }}.

Apply these fixes:

--- a/src/components/ContextMenu/ContextMenu.react.js
+++ b/src/components/ContextMenu/ContextMenu.react.js
@@ –16,13 +16,18 @@ const getPositionToFitVisibleScreen = (ref, offset = 0, mainItemCount = 0, s
     const elBox = ref.current.getBoundingClientRect();
     let y = 0;

-    const footerHeight = 50;
+    const footerHeight = 50;
     const lowerLimit   = window.innerHeight - footerHeight;
     const upperLimit   = 0;

     // existing vertical-clamp logic…
@@ –30,9 +35,13 @@ const getPositionToFitVisibleScreen = (ref, offset = 0, mainItemCount = 0, s
-    const projectedTop    = elBox.top + y + offset;
-    const projectedBottom = projectedTop + elBox.height;
-
-    const shouldApplyOffset = subItemCount > mainItemCount;
+    const projectedTop    = elBox.top + y + offset;
+    const projectedBottom = projectedTop + elBox.height;
+
+    // only shift when we actually have a non-zero offset
+    const shouldApplyOffset = subItemCount > mainItemCount && offset !== 0;
     if (
       shouldApplyOffset &&
       projectedTop    >= upperLimit &&
       projectedBottom <= lowerLimit
     ) {
       y += offset;
     }
@@ -36,8 +45,13 @@ const getPositionToFitVisibleScreen = (ref, offset = 0, mainItemCount = 0, s
     if (prevEl) {
       const prevElBox   = prevEl.getBoundingClientRect();
       const prevElStyle = window.getComputedStyle(prevEl);
-      const prevElTop   = parseInt(prevElStyle.top,  10);
+      // Guard against “auto” or non-numeric styles
+      let prevElTop = parseInt(prevElStyle.top, 10);
+      if (!Number.isFinite(prevElTop)) {
+        prevElTop = 0;
+      }
 
       if (!shouldApplyOffset) {
         y = prevElTop + offset;
  • Ensures offset === 0 won’t silently bypass needed shifts on root menus
  • Wraps parseInt in a Number.isFinite guard to avoid NaN leaking into y and the rendered top

Please integrate these changes to shore up both edge-cases.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const getPositionToFitVisibleScreen = (ref, offset = 0, mainItemCount = 0, subItemCount = 0) => {
if (ref.current) {
const elBox = ref.current.getBoundingClientRect();
const y = elBox.y + elBox.height < window.innerHeight ? 0 : 0 - elBox.y + 100;
let y = 0;
const footerHeight = 50;
const lowerLimit = window.innerHeight - footerHeight;
const upperLimit = 0;
if (elBox.bottom > lowerLimit) {
y = lowerLimit - elBox.bottom;
} else if (elBox.top < upperLimit) {
y = upperLimit - elBox.top;
}
const projectedTop = elBox.top + y + offset;
const projectedBottom = projectedTop + elBox.height;
const shouldApplyOffset = subItemCount > mainItemCount;
if (shouldApplyOffset && projectedTop >= upperLimit && projectedBottom <= lowerLimit) {
y += offset;
}
const getPositionToFitVisibleScreen = (ref, offset = 0, mainItemCount = 0, subItemCount = 0) => {
if (ref.current) {
const elBox = ref.current.getBoundingClientRect();
let y = 0;
const footerHeight = 50;
const lowerLimit = window.innerHeight - footerHeight;
const upperLimit = 0;
if (elBox.bottom > lowerLimit) {
y = lowerLimit - elBox.bottom;
} else if (elBox.top < upperLimit) {
y = upperLimit - elBox.top;
}
const projectedTop = elBox.top + y + offset;
const projectedBottom = projectedTop + elBox.height;
// only shift when we actually have a non-zero offset
const shouldApplyOffset = subItemCount > mainItemCount && offset !== 0;
if (
shouldApplyOffset &&
projectedTop >= upperLimit &&
projectedBottom <= lowerLimit
) {
y += offset;
}
if (prevEl) {
const prevElBox = prevEl.getBoundingClientRect();
const prevElStyle = window.getComputedStyle(prevEl);
// Guard against “auto” or non-numeric styles
let prevElTop = parseInt(prevElStyle.top, 10);
if (!Number.isFinite(prevElTop)) {
prevElTop = 0;
}
if (!shouldApplyOffset) {
y = prevElTop + offset;
}
}
return y;
}
return 0;
};
🤖 Prompt for AI Agents (early access) 
In src/components/ContextMenu/ContextMenu.react.js around lines 12 to 34, fix getPositionToFitVisibleScreen to handle two edge cases: first, when mainItemCount is 0, ensure offset is applied even if it is zero to avoid skipping necessary repositioning for root menus; second, wrap the parseInt call used to compute y in a Number.isFinite check to prevent NaN values from propagating into y and causing invalid style top values. Adjust the logic to calculate a safe offset and only apply it when valid, ensuring the menu position stays within viewport bounds without NaN or no-op shifts.

// If there's a previous element show current next to it.
// Try on right side first, then on left if there's no place.
const prevEl = ref.current.previousSibling;
if (prevEl) {
const prevElBox = prevEl.getBoundingClientRect();
const prevElStyle = window.getComputedStyle(prevEl);
const prevElTop = parseInt(prevElStyle.top, 10);

if (!shouldApplyOffset) {
y = prevElTop + offset;
}

Comment on lines +38 to +44
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

parseInt(...).top may return NaN → submenu disappears
window.getComputedStyle(prevEl).top returns 'auto' for non-positioned elements.
parseInt('auto', 10) ⇒ NaN which then contaminates y.

-      const prevElStyle = window.getComputedStyle(prevEl);
-      const prevElTop = parseInt(prevElStyle.top, 10);
+      const prevElStyle = window.getComputedStyle(prevEl);
+      const rawTop      = prevElStyle.top;
+      const prevElTop   =
+        rawTop === 'auto' ? prevElBox.top : parseInt(rawTop, 10);

Guarding this value makes the positioning routine robust regardless of the parent element’s CSS positioning context.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents (early access) 
In src/components/ContextMenu/ContextMenu.react.js around lines 38 to 44, the code uses parseInt on prevElStyle.top which can be 'auto' for non-positioned elements, resulting in NaN and causing the submenu to disappear. Fix this by checking if prevElStyle.top is a valid numeric value before parsing; if it is 'auto' or invalid, default prevElTop to 0 or another safe fallback value to ensure y is always a valid number for positioning.

const showOnRight = prevElBox.x + prevElBox.width + elBox.width < window.innerWidth;
return {
x: showOnRight ? prevElBox.width : -elBox.width,
y,
y
};
}

return { x: 0, y };
}
};

const MenuSection = ({ level, items, path, setPath, hide }) => {
const MenuSection = ({ level, items, path, setPath, hide, parentItemCount = 0 }) => {
const sectionRef = useRef(null);
const [position, setPosition] = useState();

useEffect(() => {
const newPosition = getPositionToFitVisibleScreen(sectionRef);
const newPosition = getPositionToFitVisibleScreen(
sectionRef,
path[level] * 30,
parentItemCount,
items.length
);
newPosition && setPosition(newPosition);
}, [sectionRef]);
Comment on lines 60 to 68
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Missing dependencies in useEffect cause stale positions
useEffect only depends on sectionRef, but the calculated position also relies on path[level], items.length, and parentItemCount. When those change (e.g., the user hovers over a different root item, altering path[level]), the effect will not re-fire because the component instance is reused if the key stays stable.

-  }, [sectionRef]);
+  }, [sectionRef, path, level, items.length, parentItemCount]);

Adding the full dependency list prevents subtle mis-alignments and silences the React-hooks ESLint rule.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
const newPosition = getPositionToFitVisibleScreen(sectionRef);
const newPosition = getPositionToFitVisibleScreen(
sectionRef,
path[level] * 30,
parentItemCount,
items.length
);
newPosition && setPosition(newPosition);
}, [sectionRef]);
useEffect(() => {
const newPosition = getPositionToFitVisibleScreen(
sectionRef,
path[level] * 30,
parentItemCount,
items.length
);
newPosition && setPosition(newPosition);
}, [sectionRef, path, level, items.length, parentItemCount]);
🤖 Prompt for AI Agents (early access) 
In src/components/ContextMenu/ContextMenu.react.js around lines 60 to 68, the useEffect hook depends only on sectionRef but also uses path[level], items.length, and parentItemCount. To fix this, add path[level], items.length, and parentItemCount to the dependency array of the useEffect hook so it re-runs correctly when any of these values change, preventing stale position calculations and satisfying React-hooks ESLint rules.


const style = position
? {
left: position.x,
top: position.y + path[level] * 30,
top: position.y,
maxHeight: '80vh',
overflowY: 'scroll',
opacity: 1,
Expand Down Expand Up @@ -79,6 +107,10 @@ const MenuSection = ({ level, items, path, setPath, hide }) => {
item.callback && item.callback();
hide();
}}
onMouseEnter={() => {
const newPath = path.slice(0, level + 1);
setPath(newPath);
}}
>
{item.text}
{item.subtext && <span> - {item.subtext}</span>}
Expand All @@ -92,6 +124,8 @@ const MenuSection = ({ level, items, path, setPath, hide }) => {
const ContextMenu = ({ x, y, items }) => {
const [path, setPath] = useState([0]);
const [visible, setVisible] = useState(true);
const menuRef = useRef(null);

useEffect(() => {
setVisible(true);
}, [items]);
Expand All @@ -101,10 +135,6 @@ const ContextMenu = ({ x, y, items }) => {
setPath([0]);
};

//#region Closing menu after clicking outside it

const menuRef = useRef(null);

function handleClickOutside(event) {
if (menuRef.current && !menuRef.current.contains(event.target)) {
hide();
Expand All @@ -118,8 +148,6 @@ const ContextMenu = ({ x, y, items }) => {
};
});

//#endregion

if (!visible) {
return null;
}
Expand All @@ -142,14 +170,19 @@ const ContextMenu = ({ x, y, items }) => {
}}
>
{path.map((position, level) => {
const itemsForLevel = getItemsFromLevel(level);
const parentItemCount =
level === 0 ? items.length : getItemsFromLevel(level - 1).length;

return (
<MenuSection
key={`section-${position}-${level}`}
path={path}
setPath={setPath}
level={level}
items={getItemsFromLevel(level)}
items={itemsForLevel}
hide={hide}
parentItemCount={parentItemCount}
/>
);
})}
Expand Down
Loading