Adding dynamic height with overflow/scrolling for PopoverNavigation #3768
Conversation
olemartinorg
added
kind/bug
📝 WalkthroughWalkthroughUpdated snapshot version and added an internal hook in PopoverNavigation.tsx that computes and applies a dynamic maxHeight with overflow for the dropdown using ResizeObserver plus window resize/scroll listeners; wired via a ref and inline style. No public API changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/features/navigation/PopoverNavigation.tsx (1)
125-125: Simplify the type annotation.
React.RefObject<HTMLDivElement | null>is redundant sinceRefObject<T>already permits.currentto benull.Apply this diff:
-function useDynamicHeight(elementRef: React.RefObject<HTMLDivElement | null>, isActive: boolean): CSSProperties { +function useDynamicHeight(elementRef: React.RefObject<HTMLDivElement>, isActive: boolean): CSSProperties {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snapshots.js(1 hunks)src/features/navigation/PopoverNavigation.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/features/navigation/PopoverNavigation.tsx
🧬 Code graph analysis (1)
src/features/navigation/PopoverNavigation.tsx (1)
src/hooks/useDeviceWidths.ts (1)
useIsMobile(21-23)
🔇 Additional comments (4)
snapshots.js (1)
370-370: LGTM!Version bump is appropriate for this release.
src/features/navigation/PopoverNavigation.tsx (3)
1-2: LGTM!The
CSSPropertiesimport is correctly added for typing the hook's return value.
31-33: LGTM!The
dropdownRefanddropdownStyleare correctly implemented to enable dynamic height management for the non-mobile dropdown.
62-71: LGTM!The
refandstyleprops are correctly wired to theDropdowncomponent, enabling the dynamic height constraint.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/navigation/PopoverNavigation.tsx (1)
93-93: Fix data attribute typo (daata-color).Prevents intended styling.
- daata-color='accent' + data-color='accent'
🧹 Nitpick comments (2)
src/features/navigation/PopoverNavigation.tsx (2)
148-155: Use passive scroll listener to reduce main-thread work.Scrolling fires frequently; use
passive: truewith capture. Keep cleanup matchingcapture.- window.addEventListener('resize', updateMaxHeight); - window.addEventListener('scroll', updateMaxHeight, true); + window.addEventListener('resize', updateMaxHeight); + window.addEventListener('scroll', updateMaxHeight, { capture: true, passive: true }); @@ - window.removeEventListener('resize', updateMaxHeight); - window.removeEventListener('scroll', updateMaxHeight, true); + window.removeEventListener('resize', updateMaxHeight); + window.removeEventListener('scroll', updateMaxHeight, { capture: true });
158-159: Optional: Only set overflow when constrained.Avoids unnecessary styling when not open/active.
- return { maxHeight: maxHeight ? `${maxHeight}px` : undefined, overflowY: 'auto' }; + return { + maxHeight: maxHeight !== undefined ? `${maxHeight}px` : undefined, + overflowY: maxHeight !== undefined ? 'auto' : undefined, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/navigation/PopoverNavigation.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/features/navigation/PopoverNavigation.tsx
🧬 Code graph analysis (1)
src/features/navigation/PopoverNavigation.tsx (1)
src/hooks/useDeviceWidths.ts (1)
useIsMobile(21-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
…ggestion from coderabbit
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/navigation/PopoverNavigation.tsx (1)
93-96: Typo in attribute name.Change
daata-colortodata-color.- daata-color='accent' + data-color='accent'
♻️ Duplicate comments (1)
src/features/navigation/PopoverNavigation.tsx (1)
141-146: Initial measurement call looks good.Calling updateMaxHeight immediately prevents initial overflow/flicker.
🧹 Nitpick comments (1)
src/features/navigation/PopoverNavigation.tsx (1)
148-155: Use passive scroll listeners and consistent options.Avoids main-thread jank on scroll and ensures symmetric add/remove.
- window.addEventListener('resize', updateMaxHeight); - window.addEventListener('scroll', updateMaxHeight, true); + const listenerOptions = { capture: true, passive: true } as const; + window.addEventListener('resize', updateMaxHeight, listenerOptions); + window.addEventListener('scroll', updateMaxHeight, listenerOptions); ... - window.removeEventListener('resize', updateMaxHeight); - window.removeEventListener('scroll', updateMaxHeight, true); + window.removeEventListener('resize', updateMaxHeight, listenerOptions); + window.removeEventListener('scroll', updateMaxHeight, listenerOptions);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/navigation/PopoverNavigation.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/features/navigation/PopoverNavigation.tsx
🧬 Code graph analysis (1)
src/features/navigation/PopoverNavigation.tsx (1)
src/hooks/useDeviceWidths.ts (1)
useIsMobile(21-23)
🔇 Additional comments (2)
src/features/navigation/PopoverNavigation.tsx (2)
31-34: Good gating and wiring of dynamic height.Using a ref and enabling the hook only when open and non‑mobile is correct.
63-71: Ref + inline style applied correctly.Passing ref to Dropdown and applying computed style is appropriate.
2 participants
Description
In tablet mode, there was no way to scroll if the navigation popover exceeded the page height. This limits the height of the container to the available screen height and adds internal scrolling.
Testing for this will arrive in #3624, as one test there has been failing because the element is not reachable.
Before:
2025-10-07_14-19-45.mp4
After:
2025-10-07_14-18-36.mp4
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Chores