refactor: auto-resize panel to fit query plan list - #237
Conversation
|
Seeing an error when navigating to the "settings" tab: |
📝 WalkthroughWalkthroughChangesQuery plan panel resizing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
ui/src/components/QueryPlan.tsx-94-107 (1)
94-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResize the panel when the tree height changes. This effect only runs on
treeData/planId, so expanding or collapsing nodes, or any reflow that changesscrollHeight, can leave the panel at the wrong size. Use aResizeObserveror expansion callback on the content element and clean it up.🤖 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 `@ui/src/components/QueryPlan.tsx` around lines 94 - 107, Update the resizing logic in QueryPlan’s useLayoutEffect to observe treeContent height changes, not only treeData or planId updates. Use a ResizeObserver or equivalent expansion callback to recalculate and apply the capped height through topPanel.resize, and clean up the observer when the effect unmounts or dependencies change.Source: Path instructions
🤖 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 `@ui/packages/`@quent/components/src/ui/select-field.tsx:
- Line 89: Add renderOption to the SelectField props destructuring alongside the
other component props so the option mapping expression can use it safely. Add a
regression test covering both the custom renderOption path and the opt.label ??
opt.value fallback path.
In `@ui/src/components/QueryPlan.tsx`:
- Line 183: Update the top ResizablePanel in QueryPlan to pass
maxSize={MAX_TOP_PANEL_HEIGHT_PX}, ensuring user resizing cannot exceed the
300px cap while preserving the existing defaultSize and panelRef behavior.
---
Other comments:
In `@ui/src/components/QueryPlan.tsx`:
- Around line 94-107: Update the resizing logic in QueryPlan’s useLayoutEffect
to observe treeContent height changes, not only treeData or planId updates. Use
a ResizeObserver or equivalent expansion callback to recalculate and apply the
capped height through topPanel.resize, and clean up the observer when the effect
unmounts or dependencies change.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ab6cdb7e-7c64-4dd4-8d9c-648e23356272
📒 Files selected for processing (2)
ui/packages/@quent/components/src/ui/select-field.tsxui/src/components/QueryPlan.tsx
| options.map(opt => ( | ||
| <SelectItem key={opt.value} value={opt.value} className="text-xs"> | ||
| {opt.label ?? opt.value} | ||
| {renderOption ? renderOption(opt) : (opt.label ?? opt.value)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Destructure renderOption before using it.
renderOption is referenced here but is not included in the SelectField props destructuring at Lines 41-52. This causes ReferenceError: renderOption is not defined whenever options are mapped, so the fallback is never reached. Add renderOption to the destructured props and cover both renderer and fallback paths with a regression test.
Proposed fix
export const SelectField = ({
icon: Icon,
label,
options,
+ renderOption,
value,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {renderOption ? renderOption(opt) : (opt.label ?? opt.value)} | |
| export const SelectField = ({ | |
| icon: Icon, | |
| label, | |
| options, | |
| renderOption, | |
| value, |
🤖 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 `@ui/packages/`@quent/components/src/ui/select-field.tsx at line 89, Add
renderOption to the SelectField props destructuring alongside the other
component props so the option mapping expression can use it safely. Add a
regression test covering both the custom renderOption path and the opt.label ??
opt.value fallback path.
Good catch, bad cleanup from one of the recent rebases |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/src/components/QueryPlan.tsx (1)
183-183: 🎯 Functional Correctness | 🟠 MajorStill enforce the 300px cap on the
ResizablePanel.
Math.min(...)limits only the effect-driven resize. Because the top panel still has nomaxSize, users can drag it beyondMAX_TOP_PANEL_HEIGHT_PX, defeating the PR’s maximum-height objective. AddmaxSize={MAX_TOP_PANEL_HEIGHT_PX}to the topResizablePanel.🤖 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 `@ui/src/components/QueryPlan.tsx` at line 183, Add maxSize={MAX_TOP_PANEL_HEIGHT_PX} to the top ResizablePanel in QueryPlan, alongside its existing panelRef and defaultSize props, so both dragging and effect-driven resizing enforce the 300px maximum.
🤖 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.
Duplicate comments:
In `@ui/src/components/QueryPlan.tsx`:
- Line 183: Add maxSize={MAX_TOP_PANEL_HEIGHT_PX} to the top ResizablePanel in
QueryPlan, alongside its existing panelRef and defaultSize props, so both
dragging and effect-driven resizing enforce the 300px maximum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: a42c8335-5147-406c-b7eb-78531c7ee35c
📒 Files selected for processing (1)
ui/src/components/QueryPlan.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/components/QueryPlan.tsx (1)
98-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecompute the plan panel height when the Query Plan content changes.
treeData/planIdonly cover data updates; TreeView expand/collapse and switching back from Settings can leave the top panel at a stale height. Track the active tab and observetreeContentwith aResizeObserverthat disconnects in cleanup.🤖 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 `@ui/src/components/QueryPlan.tsx` around lines 98 - 107, The height effect in QueryPlan must also respond to active-tab changes and TreeView content resizing. Update the useLayoutEffect around treeContentRef/topPanelRef to include the active tab dependency and attach a ResizeObserver to treeContent that reruns the existing capped resize calculation, disconnecting the observer during cleanup.
🤖 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.
Outside diff comments:
In `@ui/src/components/QueryPlan.tsx`:
- Around line 98-107: The height effect in QueryPlan must also respond to
active-tab changes and TreeView content resizing. Update the useLayoutEffect
around treeContentRef/topPanelRef to include the active tab dependency and
attach a ResizeObserver to treeContent that reruns the existing capped resize
calculation, disconnecting the observer during cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 99c472e4-f6e2-4be2-8c7f-5eb66969cd29
📒 Files selected for processing (1)
ui/src/components/QueryPlan.tsx
|
/merge |
2 participants
Description
Detects the size of the query plan list and auto-resizes the
ResizablePanelcontainer to fit the plans. Set's a max-height of300pxso plans will start scrolling and not cause the DAG to be too small.Related Issues
Relates to #146
Screenshots
5 plans:
After hitting max height: