fix: restore table height virtualization classname (BED-9057) - #3054
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesExploreTable styling
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
jvacca-specterops
left a comment
jvacca-specterops
left a comment
There was a problem hiding this comment.
Thanks for catching this!
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 225: Move the `overflow-auto h-full` classes from the `className` prop on
the table to `TableProps.tableContainerClassName`. Keep the table’s own styling
separate, and ensure `disableDefaultOverflowAuto` remains configured so the
virtualization viewport stays within the outer `overflow-hidden` container.
🪄 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: 3e5043e2-6f7a-4194-a4e1-e1180e16b944
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx
2 participants
Description
It looks like we lost a fixed height and overflow-scroll somewhere along the way, and DataTable virtualization depends on these properties. This PR restores those CSS properties and removes the custom scrollbars, to be restored in a later ticket (https://specterops.atlassian.net/browse/BED-9074)
Screen.Recording.2026-07-24.at.5.34.53.PM.mov
Describe your changes in detail
Motivation and Context
Resolves BED-9057
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 of changes