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) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Uffizzi Ephemeral Environment
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/ContextMenu/ContextMenu.react.js (1)
115-139:⚠️ Potential issueBetter ref management and event handler organization
Moving the
menuRefto component level and organizing the click handler inside the effect hook improves code organization and maintainability.- if (!visible) return null; + if (!visible) { return null; }Consider adding a cleanup of event listeners when the menu hides:
const hide = () => { setVisible(false); setPath([0]); + // When we hide the menu, ensure listeners are properly cleaned up + document.removeEventListener('mousedown', handleClickOutside); }; useEffect(() => { const handleClickOutside = event => { if (menuRef.current && !menuRef.current.contains(event.target)) { hide(); } }; document.addEventListener('mousedown', handleClickOutside); return () => { document.removeEventListener('mousedown', handleClickOutside); }; - }, []); + }, [visible]); // Re-add listener when visibility changes
🧹 Nitpick comments (1)
src/components/ContextMenu/ContextMenu.react.js (1)
76-81: Fix style indentationThe style object has inconsistent indentation.
const style = position ? { - transform: `translate(${position.x}px, ${position.y}px)`, - maxHeight: '80vh', - overflowY: 'auto', - opacity: 1, - position: 'absolute', + transform: `translate(${position.x}px, ${position.y}px)`, + maxHeight: '80vh', + overflowY: 'auto', + opacity: 1, + position: 'absolute', } : {};🧰 Tools
🪛 GitHub Check: Lint
[failure] 81-81:
Expected indentation of 4 spaces but found 6
[failure] 80-80:
Expected indentation of 6 spaces but found 8
[failure] 79-79:
Expected indentation of 6 spaces but found 8
[failure] 78-78:
Expected indentation of 6 spaces but found 8
[failure] 77-77:
Expected indentation of 6 spaces but found 8
[failure] 76-76:
Expected indentation of 6 spaces but found 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ContextMenu/ContextMenu.react.js(5 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
src/components/ContextMenu/ContextMenu.react.js
[failure] 81-81:
Expected indentation of 4 spaces but found 6
[failure] 80-80:
Expected indentation of 6 spaces but found 8
[failure] 79-79:
Expected indentation of 6 spaces but found 8
[failure] 78-78:
Expected indentation of 6 spaces but found 8
[failure] 77-77:
Expected indentation of 6 spaces but found 8
[failure] 76-76:
Expected indentation of 6 spaces but found 8
[failure] 19-19:
Expected { after 'if' condition
[failure] 141-141:
Expected { after 'if' condition
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (3)
src/components/ContextMenu/ContextMenu.react.js (3)
87-105: Improved hover handling and item interactionThe new hover handler correctly updates the path state for all menu items, allowing for consistent submenu navigation. The disabled item check is also a good defensive programming practice.
145-147: Fixed path traversal logicThe improved path traversal correctly handles undefined items with the optional chaining operator.
157-171: Optimized MenuSection rendering with improved contextPrecomputing
itemsForLevelandparentItemCountfor each level provides better context for theMenuSectioncomponent, which helps with the alignment issues mentioned in the PR objectives.
|
@dblythy could you take a look at the review feedback? |
Uffizzi Ephemeral Environment
|
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/ContextMenu/ContextMenu.react.js (2)
74-74: Add missing dependencies to useEffectThe useEffect hook is missing critical dependencies that could cause stale positioning calculations when the menu state changes.
This issue was previously identified but hasn't been addressed. The effect depends on
path[level],parentItemCount, anditems.lengthbut only includes an empty dependency array.- }, []); + }, [path, level, parentItemCount, items.length]);
32-45: Guard against NaN from parseInt and improve offset logicThe positioning logic has two potential issues that could cause submenus to disappear or misalign:
parseInton computed styles can returnNaNwhen the value is 'auto'- Zero-offset handling may skip necessary repositioning
Previous reviews identified these issues but they remain unaddressed:
if (prevEl) { const prevElBox = prevEl.getBoundingClientRect(); + const prevElStyle = window.getComputedStyle(prevEl); + const rawTop = prevElStyle.top; + const prevElTop = rawTop === 'auto' ? 0 : parseInt(rawTop, 10); + const showOnRight = prevElBox.x + prevElBox.width + elBox.width < window.innerWidth; let proposedTop = shouldApplyOffset - ? prevElBox.top + offset + ? prevElTop + offset : prevElBox.top;
🧹 Nitpick comments (2)
src/components/ContextMenu/ContextMenu.react.js (2)
89-106: Improve hover behavior and disabled item handlingThe unified hover behavior is a good improvement, but the implementation could be more robust.
The changes successfully unify the hover behavior for all menu items and improve disabled item handling. However, consider this minor optimization:
const handleHover = () => { + if (item.disabled) return; const newPath = path.slice(0, level + 1); newPath.push(index); setPath(newPath); };This prevents unnecessary path updates when hovering over disabled items.
13-54: Consider adding viewport overflow protection for tall menusThe positioning function handles basic viewport clamping but doesn't address extremely tall menus that might exceed the available height.
For better UX with very tall submenus, consider detecting when the menu height exceeds available space:
+ // Check if menu is too tall for viewport + const availableHeight = lowerLimit - upperLimit; + if (menuHeight > availableHeight) { + // Let CSS handle scrolling via maxHeight in MenuSection + console.warn('Menu exceeds viewport height, relying on CSS scrolling'); + }The existing
maxHeight: '80vh'andoverflowY: 'auto'in MenuSection already handle this case, so this is just for awareness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ContextMenu/ContextMenu.react.js(5 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
src/components/ContextMenu/ContextMenu.react.js
[failure] 83-83:
Expected indentation of 4 spaces but found 6
[failure] 82-82:
Expected indentation of 6 spaces but found 8
[failure] 81-81:
Expected indentation of 6 spaces but found 8
[failure] 80-80:
Expected indentation of 6 spaces but found 8
[failure] 79-79:
Expected indentation of 6 spaces but found 8
[failure] 78-78:
Expected indentation of 6 spaces but found 8
[failure] 143-143:
Expected { after 'if' condition
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (2)
src/components/ContextMenu/ContextMenu.react.js (2)
159-175: Excellent improvement to parent item count calculationThe addition of
parentItemCountcalculation and passing it toMenuSectioncomponents directly addresses the alignment issues mentioned in the PR objectives.This implementation correctly:
- Calculates parent item counts for each menu level
- Passes the count to MenuSection for proper positioning
- Uses stable keys for MenuSection components
The logic handles both root level (
level === 0) and nested levels appropriately.
29-29:❓ Verification inconclusive
Verify offset application logic for edge cases
The
shouldApplyOffsetlogic may not handle all scenarios correctly, particularly whenmainItemCount === 0butoffsetis also 0.The current logic
mainItemCount === 0 || subItemCount > mainItemCountmay cause issues when the root menu has zero offset. This could skip necessary repositioning for off-screen root menus.Consider this improvement:
- const shouldApplyOffset = mainItemCount === 0 || subItemCount > mainItemCount; + const shouldApplyOffset = (mainItemCount === 0 && offset !== 0) || subItemCount > mainItemCount;Let me verify how this function is being called to understand the offset patterns:
🏁 Script executed:
#!/bin/bash # Check how getPositionToFitVisibleScreen is called with different parameters rg -A 3 -B 1 "getPositionToFitVisibleScreen\(" src/Length of output: 455
Here’s some context I need to verify before updating the comment—this will show where
getPositionToFitVisibleScreencomes from and howshouldApplyOffsetis used in the file:
🏁 Script executed:
#!/bin/bash # Show import of the positioning helper rg -n "getPositionToFitVisibleScreen" -R src/components/ContextMenu/ContextMenu.react.js # Show surrounding lines for shouldApplyOffset definition rg -n -C5 "const shouldApplyOffset" -R src/components/ContextMenu/ContextMenu.react.jsLength of output: 987
I’ll try again with the correct
rgsyntax to pull the import and the surrounding context:
🏁 Script executed:
#!/bin/bash # Show import of the positioning helper rg -n "getPositionToFitVisibleScreen" src/components/ContextMenu/ContextMenu.react.js # Show surrounding lines for shouldApplyOffset definition rg -n -C5 "const shouldApplyOffset" src/components/ContextMenu/ContextMenu.react.jsLength of output: 737
Let’s pull the function signature and how
offsetis used insidegetPositionToFitVisibleScreen:
🏁 Script executed:
#!/bin/bash # Show the first 40 lines of ContextMenu.react.js (function signature + setup) sed -n '1,40p' src/components/ContextMenu/ContextMenu.react.js # Search for all uses of `offset` in this file with context rg -n -C3 "offset" src/components/ContextMenu/ContextMenu.react.jsLength of output: 1921
Double-check root-menu offset logic
The suggested change
- const shouldApplyOffset = mainItemCount === 0 || subItemCount > mainItemCount; + const shouldApplyOffset = (mainItemCount === 0 && offset !== 0) || subItemCount > mainItemCount;is effectively a no-op when
offset === 0(both branches useprevElBox.top). Before applying this tweak, please verify in off-screen root-menu scenarios that the clamping and repositioning behave as intended.• File:
src/components/ContextMenu/ContextMenu.react.js, line 29
# [7.2.0-alpha.4](7.2.0-alpha.3...7.2.0-alpha.4) (2025-05-24) ### Bug Fixes * Improperly aligned unfolding sub-items in context menu in data browser ([#2726](#2726)) ([3fed292](3fed292))
|
🎉 This change has been released in version 7.2.0-alpha.4 |
# [7.2.0](7.1.0...7.2.0) (2025-06-01) ### Bug Fixes * Data browser not scrolling to top when changing filter while cell selected ([#2821](#2821)) ([c2527dc](c2527dc)) * Data browser table shows loading indicator when info panel is loading ([#2782](#2782)) ([da57e5e](da57e5e)) * Improperly aligned unfolding sub-items in context menu in data browser ([#2726](#2726)) ([3fed292](3fed292)) * Notifications fade out erratically when executing a script on large number of rows ([#2822](#2822)) ([3891381](3891381)) * Pagination does not reset to page 1 when clicking on class or filter ([#2798](#2798)) ([29d1447](29d1447)) * Saving new filter in data browser overwrites filters added in other dashboard instances ([#2769](#2769)) ([46bc154](46bc154)) * Selecting a saved filter in data browser may highlight a different filter ([#2783](#2783)) ([4c6e853](4c6e853)) ### Features * Add confirmation dialog before saving a Cloud Config parameter that has been modified since editing it ([#2770](#2770)) ([adb9b5c](adb9b5c)) * Add custom CSS styling for info panel items ([#2788](#2788)) ([f031e5d](f031e5d)) * Add relative date filter in data browser for date constraints relative to when the query is run ([#2736](#2736)) ([d9dfd69](d9dfd69)) * Add script execution on parallel batches with option `script.executionBatchSize` ([#2828](#2828)) ([cee8b8d](cee8b8d)) * Keyboard Enter key can be used to select item in data browser filter dialog field dropdown ([#2771](#2771)) ([dc14710](dc14710))
|
🎉 This change has been released in version 7.2.0 |
New Pull Request Checklist
Issue Description
Closes: #2635
Approach
TODOs before merging
Summary by CodeRabbit
New Features
Bug Fixes