Repository navigation
fix: Native scrollbars in sidebar and panel views - #730
Conversation
ilyabo
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAcross ten files in ChangesScrollArea Migration and Layout Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eccb94fccb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <ScrollArea className="h-full"> | ||
| <div className="h-full p-2"> |
There was a problem hiding this comment.
When a chart is rendered in a panel, this ScrollArea adds a Radix viewport/content wrapper between the panel and the h-full chart container. That intermediate content element does not have a definite height, so the h-full chain that MosaicChartView/ResponsivePlot relies on can resolve to zero; ResponsivePlot ignores zero-height measurements, leaving fresh vgplot charts without a containerSize and rendering blank. Please keep a definite-height wrapper outside the scroll area, or force the viewport content wrapper to h-full before nesting the chart view.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/layout/src/node-renderers/grid-node-renderer/GridLayout.tsx`:
- Around line 674-675: The ScrollArea component in GridLayout.tsx already
provides a vertical scrollbar internally, so the explicit vertical ScrollBar
component with orientation="vertical" is redundant and creates duplication.
Remove the line containing the vertical ScrollBar component and keep only the
horizontal ScrollBar component with orientation="horizontal".
In `@packages/mosaic/src/charts/chart-settings/MosaicChartSpecViewerPanel.tsx`:
- Around line 44-45: Remove the explicit ScrollBar component with
orientation="vertical" from the ScrollArea since ScrollArea already renders a
vertical scrollbar by default. Delete the line containing <ScrollBar
orientation="vertical" /> and keep only the <ScrollArea> component, which will
handle the vertical scrolling functionality automatically without any redundant
scrollbar rendering.
In `@packages/mosaic/src/charts/MosaicChart.tsx`:
- Around line 60-61: Remove the redundant vertical ScrollBar component from the
ScrollArea in MosaicChart.tsx. The ScrollArea component from `@sqlrooms/ui`
already includes a default vertical scrollbar internally, so the explicit
ScrollBar with orientation="vertical" creates duplication. Delete the first
ScrollBar element and keep only the ScrollBar with orientation="horizontal" to
maintain horizontal scrolling functionality.
In `@packages/mosaic/src/dashboard/panel/MosaicDashboardPanelLayout.tsx`:
- Around line 72-73: The ScrollArea components in the MosaicDashboardPanelLayout
are displaying duplicate vertical scrollbars because explicit ScrollBar
components with vertical orientation are being added when the ScrollArea from
`@sqlrooms/ui` already provides a default vertical scrollbar. Remove the explicit
ScrollBar orientation="vertical" component from both ScrollArea usages (one in
the narrow settings branch and one in the split-view branch) to eliminate the
duplicate scrollbars while keeping the default scrollbar behavior intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9803ce46-4c17-4f4d-a468-41e97d20f52c
📒 Files selected for processing (10)
apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliDataSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliWorkspaceSidebar.tsxpackages/layout/src/node-renderers/grid-node-renderer/GridLayout.tsxpackages/mosaic/src/charts/MosaicChart.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSpecViewerPanel.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanelLayout.tsxpackages/sql-editor/src/components/SchemaExplorer.tsxpackages/sql-editor/src/components/TableStructurePanel.tsx
…crollbars # Conflicts: # apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e94d8b9dd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <ScrollArea className="min-h-0 flex-1"> | ||
| <div className="h-full p-2"> |
There was a problem hiding this comment.
When users open “View spec” in a chart settings pane, this new ScrollArea inserts Radix's viewport content wrapper between the flex-1 panel and the nested h-full editor wrapper. That intermediate wrapper has no definite height, unlike the previous flex-1 overflow-auto div, so the h-full chain for MosaicCodeMirrorEditor can resolve to auto/default height instead of filling the settings pane; the spec viewer becomes a small/collapsed editor in split or narrow panels. Add the same viewport-content h-full rule used for the chart view, or keep a definite-height wrapper outside the scroll area.
Useful? React with 👍 / 👎.
Summary
ScrollAreacontainersTesting
Summary by CodeRabbit