fix: restore table height virtualization classname (BED-9057)#3054
fix: restore table height virtualization classname (BED-9057)#3054dcairnsspecterops wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesExploreTable styling
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx`:
- Line 226: Update the ExploreTable wrapper around MemoDataTable so its height
reflects isExpanded: use the expanded available-height calculation when expanded
and retain the collapsed viewport height otherwise, or allow the parent
grid/ScrollArea to determine it. Ensure virtualization receives the correct
height in both states and add coverage for collapsed and expanded rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dc190072-a5db-4d28-b65c-c779f9394105
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx
| onColumnOrderChange={(newOrder) => { | ||
| setColumnOrder(newOrder); | ||
| }} | ||
| className='overflow-auto h-[calc(50vh-72px)]' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the table height responsive to expanded mode.
The wrapper expands from h-1/2 to h-[calc(100%-72px)], but MemoDataTable always receives h-[calc(50vh-72px)]. In expanded mode, virtualization can therefore measure only the collapsed-height viewport, leaving unused space and incorrect scrolling behavior. Make the height conditional on isExpanded or let the surrounding grid/ScrollArea provide the available height, and add coverage for both states.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx`
at line 226, Update the ExploreTable wrapper around MemoDataTable so its height
reflects isExpanded: use the expanded available-height calculation when expanded
and retain the collapsed viewport height otherwise, or allow the parent
grid/ScrollArea to determine it. Ensure virtualization receives the correct
height in both states and add coverage for collapsed and expanded rendering.
There was a problem hiding this comment.
Wow, smart bunny
There was a problem hiding this comment.
@dcairnsspecterops, happy to help! 🐇✨
dcairnsspecterops
changed the title
coderabbitai
Bot
added
the
user interface
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx`:
- Around line 225-229: Update the height-class conditions in the ExploreTable
container’s cn call so the 100vh height uses isExpanded instead of repeating
!isExpanded. Preserve the existing collapsed height condition and ensure exactly
one viewport height class applies in each state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d8072125-f782-4b5d-adcd-9b0adf6285d0
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx
| className={cn('overflow-auto', { | ||
| // TODO: why is header disappearing on bottom scroll? One extra overflow element somewhere | ||
| 'h-[calc(50vh-72px)]': !isExpanded, | ||
| 'h-[calc(100vh-72px)]': !isExpanded, | ||
| })} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the expanded-height condition.
Line 228 repeats !isExpanded; twMerge leaves collapsed mode at h-[calc(100vh-72px)], while expanded mode gets no height class. This breaks the virtualization viewport in both states. Use isExpanded on line 228.
Proposed fix
className={cn('overflow-auto', {
// TODO: why is header disappearing on bottom scroll? One extra overflow element somewhere
'h-[calc(50vh-72px)]': !isExpanded,
- 'h-[calc(100vh-72px)]': !isExpanded,
+ 'h-[calc(100vh-72px)]': isExpanded,
})}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx`
around lines 225 - 229, Update the height-class conditions in the ExploreTable
container’s cn call so the 100vh height uses isExpanded instead of repeating
!isExpanded. Preserve the existing collapsed height condition and ensure exactly
one viewport height class applies in each state.
1 participant
Description
Screen.Recording.2026-07-24.at.5.34.53.PM.mov
Describe your changes in detail
Motivation and Context
Resolves <TICKET_OR_ISSUE_NUMBER>
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit