fix: skip row virtualization for small data grids - #10146
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDataGridView now selects virtualised or static row rendering from visible column count, row count, and an optional field override. PgReactTableStyled positions static rows relatively. ChangesConditional Grid Virtualisation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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 `@web/pgadmin/static/js/SchemaView/DataGridView/grid.jsx`:
- Around line 166-210: The static-row path in DataGridView still relies on the
virtualizer for index-based scrolling, which can misplace the viewport when rows
are auto-height. Update the scroll handling in DataGridHeader and
DataGridFormHeader so `scrollToIndex()` is only used when `virtualise` is
enabled; in the non-virtualized branch, use a DOM-based scroll target instead.
Keep the fix aligned with the existing `virtualise`, `virtualizer`, and
`GridRow` flow so the behavior matches the rendered mode.
🪄 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: CHILL
Plan: Pro
Run ID: d14ff283-30a4-4d36-a005-65c9ad66811e
📒 Files selected for processing (2)
web/pgadmin/static/js/SchemaView/DataGridView/grid.jsxweb/pgadmin/static/js/components/PgReactTableStyled.jsx
There was a problem hiding this comment.
Pull request overview
This PR improves UI responsiveness in SchemaView dialogs by disabling row virtualization for “small” editable data grids, preventing expensive re-measure/layout work when tabs are hidden/shown (e.g., Table Properties → Columns).
Changes:
- Adds an adaptive
virtualiseThreshold(optionally overridable viafield.virtualiseThreshold) based on visible column count, and only virtualizes whenrows.lengthexceeds that threshold. - Splits table-body rendering: virtualized mode uses the existing virtualizer path; non-virtualized mode renders rows in normal document flow with a static-row class.
- Updates shared table row styling so static rows are positioned normally (non-absolute) while preserving existing virtualized-row layout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| web/pgadmin/static/js/SchemaView/DataGridView/grid.jsx | Introduces adaptive virtualization thresholding and a non-virtualized rendering branch for small grids. |
| web/pgadmin/static/js/components/PgReactTableStyled.jsx | Adds styling for pgrt-row--static to opt rows out of the absolute-position virtualized layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Validate const VIRTUALISE_CELL_BUDGET = 700;
const DEFAULT_VIRTUALISE_THRESHOLD = 100;
const visibleColCount = table.getVisibleLeafColumns().length;
const virtualiseThreshold =
field.virtualiseThreshold ??
(visibleColCount > 0
? Math.min(400, Math.max(25, Math.round(VIRTUALISE_CELL_BUDGET / visibleColCount)))
: DEFAULT_VIRTUALISE_THRESHOLD);
const virtualise = rows.length > virtualiseThreshold;
Suggest guarding it: const rawThreshold = field.virtualiseThreshold;
const virtualiseThreshold =
(typeof rawThreshold === 'number' && rawThreshold > 0)
? rawThreshold
: (visibleColCount > 0
? Math.min(400, Math.max(25, Math.round(VIRTUALISE_CELL_BUDGET / visibleColCount)))
: DEFAULT_VIRTUALISE_THRESHOLD);That way an invalid/omitted Separately (non-blocking, FYI): |
Description
Switching back to a dialog tab that contains an editable data grid (e.g. Table Properties → Columns) was noticeably slow — the grid appeared to "reload" its rows on every tab switch. This PR skips row virtualization for small grids so hide/show becomes a pure CSS toggle, eliminating the jank while preserving virtualization for large grids.
Root Cause
DataGridViewunconditionally virtualized rows. InSchemaViewdialogs, inactive tabs stay mounted but hidden via the HTMLhiddenattribute (display:none) incomponents/TabPanel.jsx:27, so a hidden tab's scroll viewport measures 0 and the virtual window collapses.On switching back, the viewport height goes 0 → real, the virtualizer's
ResizeObserverfires, and every rendered row is re-measured through a synchronousgetBoundingClientRect()via a freshmeasureElementref callback (SchemaView/DataGridView/grid.jsx:143,:177). This forced-layout settle loop is visible lag on grids with complex cell editors, and at small row counts virtualization provides no benefit — it is pure overhead.Fix
SchemaView/DataGridView/grid.jsx:125-134— compute an adaptivevirtualiseThreshold(overridable viafield.virtualiseThreshold), scaled by visible column count asmin(400, max(25, round(700 / cols))), defaulting to 100 when column count is unknown; virtualize only whenrows.length > virtualiseThreshold.SchemaView/DataGridView/grid.jsx:166-210— branch the table body: large grids keep the virtualizer (measureElement+ absolute positioning +translateY); small grids render every row in normal document flow withclassName='pgrt-row--static', nomeasureElement.components/PgReactTableStyled.jsx:94-96— add&.pgrt-row--static { position: relative }so opted-out rows escape the virtualizer's absolute layout and stack in normal flow, making tab hide/show a pure CSS toggle.Test Plan
npx eslint -c .eslintrc.json both changed files — clean, no errorsFixes : #10143
Summary by CodeRabbit