fix: Improperly aligned unfolding sub-items in context menu in data browser #2726
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
Uffizzi Ephemeral Environment
|
|
I tried out all the positions to see which ones work. It's painful that we have to spend time on fixing something that should work OOTB. I hope we get to #2460 soon. OKtop-right short: OK 
top-right long: OK 
bottom-right long: OK 
bottom-left long: OK 
top-left short: OK 
top-left long: OK 
NOKbottom-right short: NOK - subitem should align with top border; I guess the logic could be, if the count of sub-items <= count of main items, then align with the top border, as we know there is enough space towards the bottom. 
bottom-left short: NOK 
|
mtrezza
changed the title
Uffizzi Ephemeral Environment
|
|
The issue seems to be a different one; I've opened #2743 with high priority, as it's currently blocking the other PRs. |
Uffizzi Ephemeral Environment
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
📝 WalkthroughWalkthroughThe changes enhance the context menu component by refining how submenu positioning is calculated and rendered. The positioning logic in Changes
Assessment against linked issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/ContextMenu/ContextMenu.react.js (1)
110-113: Minor: avoid unnecessary state churn on leaf hover
On a leaf item,newPathis identical topath.slice(0, level + 1)which is already the current value. CallingsetPathredundantly triggers a re-render.- onMouseEnter={() => { - const newPath = path.slice(0, level + 1); - setPath(newPath); - }} + onMouseEnter={() => { + /* no-op when hovering the same leaf */ + if (path.length !== level + 1) { + setPath(path.slice(0, level + 1)); + } + }}This is a tiny optimisation but helps keep React DevTools noise down.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ContextMenu/ContextMenu.react.js(4 hunks)
🧰 Additional context used
🪛 ESLint
src/components/ContextMenu/ContextMenu.react.js
[error] 56-56: 'MenuSection' is assigned a value but never used.
(no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (1)
src/components/ContextMenu/ContextMenu.react.js (1)
124-150: 👍 Nice refactor aroundmenuRefand outside-click handling
MovingmenuRefto a singleuseRefand de-duplicating the outside-click logic makes the component easier to reason about.
| const prevElStyle = window.getComputedStyle(prevEl); | ||
| const prevElTop = parseInt(prevElStyle.top, 10); | ||
|
|
||
| if (!shouldApplyOffset) { | ||
| y = prevElTop + offset; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| useEffect(() => { | ||
| const newPosition = getPositionToFitVisibleScreen(sectionRef); | ||
| const newPosition = getPositionToFitVisibleScreen( | ||
| sectionRef, | ||
| path[level] * 30, | ||
| parentItemCount, | ||
| items.length | ||
| ); | ||
| newPosition && setPosition(newPosition); | ||
| }, [sectionRef]); |
There was a problem hiding this comment.
🛠️ 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.
| 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 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 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:
- When
mainItemCount === 0(top-level menu)shouldApplyOffsetis alwaystrue, yetoffsetis often0, so the earlyprojectedTop/Bottomcheck may skip a necessary upward shift if the root element is already partially off-screen. parseIntlater in the function (see below) may yieldNaN, propagating intoyand producingstyle={{ 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 -B3Length of output: 351
🏁 Script executed:
#!/bin/bash
# Display the complete getPositionToFitVisibleScreen function for context
ast-grep --pattern $'const getPositionToFitVisibleScreen =($_$, $_$, $_$, $_$) => { $$$ }' --quiet -A 50Length of output: 288
Guard against NaN offsets and zero-offset no-ops
We should tighten getPositionToFitVisibleScreen to handle two remaining edge‐cases:
- Zero-offset no-ops on top-level menus.
WhenmainItemCount === 0,subItemCount > mainItemCountis always true yetoffsetis often 0—so we end up skipping a needed reposition for a root menu already off-screen. - NaN from
parseInt.
Ifwindow.getComputedStyle(...).topis"auto"(or any non-numeric),parseInt(...,10)returnsNaN, which propagates intoyand yieldsstyle={{ 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 === 0won’t silently bypass needed shifts on root menus - Wraps
parseIntin aNumber.isFiniteguard to avoidNaNleaking intoyand the renderedtop
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.
| 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.
New Pull Request Checklist
Issue Description
Closes: #2635
Approach
TODOs before merging
Summary by CodeRabbit
New Features
Bug Fixes