feat: Use gridstack for the layout - #43096
Conversation
dosubot
Bot
added
change:frontend
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| * is untouched by it — `availableDropSpan` still returns exactly that gap's | ||
| * own width, never wider. | ||
| */ | ||
| export const FALLBACK_COL_SPAN = Math.floor(DEFAULT_COLUMNS / 2); |
There was a problem hiding this comment.
Suggestion: FALLBACK_COL_SPAN is fixed at 12 columns, but RootGrid passes it unchanged for containers whose configured column count differs from DEFAULT_COLUMNS. An empty 12-column (or narrower) grid therefore previews and places an open-space drop at full width, defeating the new behavior intended to keep the drop width responsive to the cursor. Derive the cap from the active container's columns value, or clamp this fallback to less than the container width when appropriate. [logic error]
Severity Level: Major ⚠️
- ⚠️ Custom 12-column root grids lose responsive open-space drop sizing.
- ⚠️ Palette drops preview and land at the entire active grid width.
- ⚠️ Cursor position becomes ineffective in empty custom-column grids.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/core/dashboard/placement.ts
**Line:** 100:100
**Comment:**
*Logic Error: `FALLBACK_COL_SPAN` is fixed at 12 columns, but `RootGrid` passes it unchanged for containers whose configured column count differs from `DEFAULT_COLUMNS`. An empty 12-column (or narrower) grid therefore previews and places an open-space drop at full width, defeating the new behavior intended to keep the drop width responsive to the cursor. Derive the cap from the active container's `columns` value, or clamp this fallback to less than the container width when appropriate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The Since superset-frontend/src/core/dashboard/placement.ts |
| function cancelSelectorFor(nodeId: string): string { | ||
| return `[data-container-id]:not([data-container-id="${CSS.escape(nodeId)}"]),[data-block-remove],[data-block-resize],[data-block-header-control]`; |
There was a problem hiding this comment.
Suggestion: CSS.escape is invoked during every render, but the jsdom test environment does not provide this API or install a polyfill. Mounting RootGrid therefore throws before GridStack initialization, causing the dashboard tests (and any supported runtime without CSS.escape) to fail. Use an available escaping utility or provide the required polyfill before constructing the selector. [runtime compatibility]
Severity Level: Major ⚠️
- ❌ RootGrid jsdom tests fail during component rendering.
- ❌ Dashboard v2 fails in runtimes lacking `CSS.escape`.
- ⚠️ GridStack initialization never runs.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/core/dashboard/RootGrid.tsx
**Line:** 68:69
**Comment:**
*Runtime Compatibility: `CSS.escape` is invoked during every render, but the jsdom test environment does not provide this API or install a polyfill. Mounting `RootGrid` therefore throws before GridStack initialization, causing the dashboard tests (and any supported runtime without `CSS.escape`) to fail. Use an available escaping utility or provide the required polyfill before constructing the selector.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (placement.shrink) { | ||
| const updates: Record<string, Partial<LayoutProps>> = {}; | ||
| items.forEach(({ id, rect: itemRect }) => { | ||
| updates[id] = { | ||
| col: itemRect.x + 1, | ||
| row: itemRect.y + 1, | ||
| colSpan: itemRect.w, | ||
| rowSpan: itemRect.h, | ||
| }; | ||
| }); | ||
| updates[gesture.id] = { | ||
| col: placement.rect.x + 1, | ||
| row: placement.rect.y + 1, | ||
| colSpan: placement.rect.w, | ||
| rowSpan: placement.rect.h, | ||
| }; | ||
| updates[placement.shrink.id] = { | ||
| col: placement.shrink.rect.x + 1, | ||
| row: placement.shrink.rect.y + 1, | ||
| colSpan: placement.shrink.rect.w, | ||
| rowSpan: placement.shrink.rect.h, | ||
| }; | ||
| provider.updateLayouts(updates); |
There was a problem hiding this comment.
Suggestion: The split path combines GridStack's post-collision positions from items with a manually calculated split based on the pre-drag packed map, then writes them through updateLayouts, which does not run collision resolution. For example, GridStack can displace a sibling while the dragged item overlaps the target; this code moves the target back to its original row and shrinks it while retaining the displaced sibling's position, allowing the target and sibling to overlap in the persisted layout. Reconcile the split against the final occupancy or run the same collision resolution before committing. [logic error]
Severity Level: Major ⚠️
- ❌ Existing-block split can persist overlapping siblings.
- ⚠️ Subsequent grid rendering may reposition affected blocks.
- ⚠️ Stored layout coordinates can diverge from GridStack's final occupancy.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/core/dashboard/RootGrid.tsx
**Line:** 490:512
**Comment:**
*Logic Error: The split path combines GridStack's post-collision positions from `items` with a manually calculated split based on the pre-drag `packed` map, then writes them through `updateLayouts`, which does not run collision resolution. For example, GridStack can displace a sibling while the dragged item overlaps the target; this code moves the target back to its original row and shrinks it while retaining the displaced sibling's position, allowing the target and sibling to overlap in the persisted layout. Reconcile the split against the final occupancy or run the same collision resolution before committing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dashboard-v2 #43096 +/- ##
===============================================
Coverage ? 65.38%
===============================================
Files ? 2828
Lines ? 159215
Branches ? 36485
===============================================
Hits ? 104100
Misses ? 53138
Partials ? 1977
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
SUMMARY
Migrates the dashboard v2 prototype's root grid from
react-grid-layoutto GridStack.js, and uses the swap to add real positioning flexibility that react-grid-layout's ownisDroppablecouldn't support: a block can now be dropped or dragged to any position on the canvas — including directly onto another block — and the canvas reacts by opening exactly the room that block needs.Concretely, an author can now:
None of this was reachable with react-grid-layout's
isDroppable, whose live drop preview is driven by an internal fake-drag simulation that produced contradictory failures with no reliable way to debug them (see commit history for the earlier attempts). GridStack's lower-level imperative API replaces that with code this PR owns and can debug directly: React stays the single source of truth for layout (a "single writer" rule threaded throughuseGridStack.ts), GridStack is used purely as a renderer and gesture source, and the palette keeps its existing native HTML5 drag (GridStack's own drag-in system is pointer-based and can't see it) with our own drop-preview drawn on top instead of a library-owned placeholder.Also fixes several bugs found during manual verification of the swap (GridStack's own
auto: truesilently claiming widgets present at mount before this code got a chance to register them properly; an over-broadcancelselector that vetoed every drag inside the grid, not just presses on nested containers; a couple ofcontainerRef-as-callback casting bugs that collapsed the drop-preview's width to its own border), and moves the canvas's scroll ownership to the page-levelCanvascontainer soRootGridandCanvasno longer fight over the same scroll gesture.react-grid-layoutis removed frompackage.jsonnow that nothing references it.VIDEO
Screen.Recording.2026-08-12.at.15.55.39.mov
TESTING INSTRUCTIONS
Added both unit tests (
gridPacking.test.ts,RootGrid.test.tsx,DashboardBuilderV2/index.test.tsx) and a new Playwright E2E suite underplaywright/tests/experimental/dashboard-v2/(run withINCLUDE_EXPERIMENTAL=true npm run playwright:test), covering placement, the split gesture, drag/resize persistence, and scroll ownership. The E2E suite specifically needs a real browser — jsdom has no layout engine, so none of the bugs this PR fixes were visible to a jsdom-based test while they were live.To verify manually, on
/dashboard/v2/new/: drag palette blocks around an empty and a sparsely-filled canvas, drop one onto another's left/right half (and try dragging an existing block onto a half too), reposition/resize an existing block, and scroll a tall canvas — everything above should behave as described in the Summary.ADDITIONAL INFORMATION