You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2026/08/12 19:53:31 UTC
[PR] feat: Use gridstack for the layout [superset]
michael-s-molina opened a new pull request, #43096:
URL: https://github.com/apache/superset/pull/43096
### SUMMARY
Migrates the dashboard v2 prototype's root grid from `react-grid-layout` to [GridStack.js](https://gridstackjs.com/), and uses the swap to add real positioning flexibility that react-grid-layout's own `isDroppable` couldn'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:
- **Drop anywhere in open space** — not just "append to the end". A palette block dropped into a gap between two existing blocks lands in that gap, sized to fit it.
- **Split an existing block left/right** — dropping (or dragging an existing block) onto the left or right half of another block shrinks that block to the other half and inserts the new one beside it, live. The block being shrunk is the *real* block visibly resizing during the hover, not a placeholder box standing in for it — the preview is provably what you get.
- **Insert a full-width row above or below** any existing block by hovering its top/bottom edge, pushing siblings down rather than overwriting them.
- **Do all of the above by repositioning an existing block**, not just by dragging a fresh one in from the palette — dragging one block onto another's half splits it exactly the same way a palette drop would.
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 through `useGridStack.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: true` silently claiming widgets present at mount before this code got a chance to register them properly; an over-broad `cancel` selector that vetoed every drag inside the grid, not just presses on nested containers; a couple of `containerRef`-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-level `Canvas` container so `RootGrid` and `Canvas` no longer fight over the same scroll gesture.
`react-grid-layout` is removed from `package.json` now that nothing references it.
### VIDEO
https://github.com/user-attachments/assets/1778c6f2-e041-4347-913b-9443fcc8ec86
### TESTING INSTRUCTIONS
Added both unit tests (`gridPacking.test.ts`, `RootGrid.test.tsx`, `DashboardBuilderV2/index.test.tsx`) and a new Playwright E2E suite under `playwright/tests/experimental/dashboard-v2/` (run with `INCLUDE_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
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Use gridstack for the layout [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43096:
URL: https://github.com/apache/superset/pull/43096#issuecomment-5272088766
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Use gridstack for the layout [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43096:
URL: https://github.com/apache/superset/pull/43096#discussion_r3769891376
##########
superset-frontend/src/core/dashboard/RootGrid.tsx:
##########
@@ -147,276 +249,436 @@ function findContainerIdAt(
* goes through — nothing places a `RootGrid`, and nothing ever will, the same
* way nothing places the dashboard itself.
*
- * Backed by `react-grid-layout`. All position/size math — including
- * collision handling — is `react-grid-layout`'s: resizing a block never
- * shrinks a sibling, only displaces it to the next open slot (clamped to
- * `minW`/`minH: 1`), which leaves every block's own authored (hand- or
- * AI-set) span untouched. This component's job is translating between the
- * stored `col`/`row`/`colSpan`/`rowSpan` schema (which allows a child to
- * omit its position entirely, to be auto-placed) and `react-grid-layout`'s
- * own `{x, y, w, h}` — see `packChildLayout` for the auto-placement piece RGL
- * has no concept of — and committing back to the store only once a gesture
- * ends (`onDragStop`/`onResizeStop`), never on every intermediate frame.
+ * Backed by GridStack (see `useGridStack`, the only place that package is
+ * imported). All position/size math — including collision handling — is
+ * GridStack's: resizing a block never shrinks a sibling, only displaces it
+ * to the next open slot, which leaves every block's own authored (hand- or
+ * AI-set) span untouched — except a left/right *drop*, the one deliberate
+ * exception, which does shrink a sibling (see `resolveDropPlacement`).
+ * This component's job is translating between the stored
+ * `col`/`row`/`colSpan`/`rowSpan` schema (which allows a child to omit its
+ * position entirely, to be auto-placed) and GridStack's own `{x, y, w, h}`
+ * — see `packChildLayout` for the auto-placement piece GridStack has no
+ * concept of — and committing back to the store only once a gesture ends,
+ * never on every intermediate frame.
+ *
+ * The palette's own native HTML5 drag stays exactly as it always has —
+ * GridStack's own drag-in system uses a pointer-based DD engine that can't
+ * see `dataTransfer`, so this draws its own drop preview (`DropGhost`)
+ * instead of handing the gesture to GridStack at all.
*
* Dragging a block into a *different* container (reparenting, as opposed to
* repositioning within this one) is handled by hit-testing which
* `data-container-id` is under the pointer when the drag ends — deliberately
- * not something `react-grid-layout` (scoped to one grid instance) handles on
- * its own.
+ * not something GridStack (scoped to one grid instance) handles on its own.
*/
export default function RootGrid({ nodeId }: { nodeId: string }) {
useDashboardRevision();
const theme = useTheme();
const node = provider.getNode(nodeId);
+ // Ahead of the early return below, and computed off `node?.layout` rather
+ // than `node.layout`: every hook this component calls (`useGridStack`
+ // among them) has to run on every render, in the same order, whether or
+ // not `node` turns out to exist.
+ const metrics = resolveGridMetrics(node?.layout, theme);
+ const children = node?.children ?? [];
+ const packed = packChildLayout(children, metrics.columns, provider.getNode);
- const commitLayout = useCallback(
- (rglLayout: Layout) => {
- const updates: Record<string, Partial<LayoutProps>> = {};
- rglLayout.forEach(item => {
- updates[item.i] = {
- col: item.x + 1,
- row: item.y + 1,
- colSpan: item.w,
- rowSpan: item.h,
- };
- });
- debugLog('commitLayout', { containerId: nodeId, updates });
- provider.updateLayouts(updates);
- },
- [nodeId],
- );
+ // Not shown while hovering a nested container (see `handleGridDragOver`)
+ // — that container's own drop target is where the preview belongs, not
+ // here (see this component's own doc comment on the identical check the
+ // previous, react-grid-layout-backed version of this made).
+ const [ghostRect, setGhostRect] = useState<PackedRect | null>(null);
+ // The existing block's own *shrunk* half, while the cursor is hovering the
+ // left/right split band of it — `resolveDropPlacement`'s `shrink`. Not a
+ // second placeholder box: it feeds `previewPacked` (below, where
+ // `useGridStack` is called), which makes the *real* block visibly resize
+ // to this rect for as long as the hover lasts — the same way a live drag
+ // or resize elsewhere on this grid is never represented by a stand-in box
+ // either. Kept as its own piece of state (rather than folded into
+ // `ghostRect`) since committing a split still needs to know which
+ // sibling and by how much, same as it always did.
+ const [shrinkPreview, setShrinkPreview] = useState<{
+ id: string;
+ rect: PackedRect;
+ } | null>(null);
+ // A counter, not a plain boolean: this element isn't a single node, it's
+ // the grid plus every item already on it, and the pointer crossing from
+ // the container onto one of those fires a `dragleave` on the container
+ // immediately followed by a `dragenter` on the child — a plain boolean
+ // would read that as leaving entirely and flicker the ghost off for a
+ // frame. Only reaching zero really means "gone".
+ const dragOverCountRef = useRef(0);
- const handleDragStart = useCallback(
- (_layout: Layout, _oldItem: unknown, newItem: { i: string } | null) => {
- debugLog('dragStart', {
- containerId: nodeId,
- dragged: describeNode(newItem?.i),
- });
- },
- [nodeId],
- );
+ // `dragleave`/`drop` alone are not a complete story: releasing the
+ // pointer somewhere that never became a drop target at all (past every
+ // edge of the browser window, over a panel that isn't a drop target,
+ // or the drag simply being cancelled with `Escape`) fires neither one on
+ // this grid — the browser fires `dragend` on the *drag source* instead
+ // (`Palette.tsx`'s own item), which bubbles to `document` regardless of
+ // where the pointer ended up. This is the backstop for exactly that: it
+ // always fires exactly once when a drag concludes, however it concluded,
+ // so the ghost (and the enter/leave counter it depends on) can never be
+ // left stuck on screen waiting for a `dragleave` that was never coming.
+ useEffect(() => {
+ const handleDragEnd = () => {
+ dragOverCountRef.current = 0;
+ setGhostRect(null);
+ setShrinkPreview(null);
+ };
+ document.addEventListener('dragend', handleDragEnd);
+ return () => document.removeEventListener('dragend', handleDragEnd);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
- const handleResizeStart = useCallback(
- (_layout: Layout, _oldItem: unknown, newItem: { i: string } | null) => {
- debugLog('resizeStart', {
- containerId: nodeId,
- resized: describeNode(newItem?.i),
- });
- },
- [nodeId],
- );
+ // `handleGestureEnd`'s own real definition needs `containerRef` (returned
+ // by the `useGridStack` call below) and `resolvePlacementAtPoint`/`columns`
+ // (declared further down, after the `!node` early return) — but
+ // `useGridStack` needs a gesture-end callback *now*, up here, ahead of
+ // both, since every hook this component calls has to run unconditionally,
+ // before any early return. This ref is the seam: `useGridStack` is handed
+ // a stable function that only ever calls whatever this ref currently
+ // holds, and the real `handleGestureEnd`, defined later once everything
+ // it needs already exists, is written into it every render — the same
+ // pattern `useGridStack` itself already uses internally for the identical
+ // reason (see its own `onGestureEndRef`).
+ const handleGestureEndRef = useRef<
+ (items: GestureEndItem[], gesture: GestureEnd) => void
+ >(() => {});
- const handleResizeStop = useCallback(
- (rglLayout: Layout, _oldItem: unknown, newItem: { i: string } | null) => {
- debugLog('resizeStop', {
- containerId: nodeId,
- resized: describeNode(newItem?.i),
- });
- commitLayout(rglLayout);
- },
- [nodeId, commitLayout],
- );
+ // What `useGridStack` actually renders — `packed` with the split target's
+ // own entry substituted for its shrunk-to half while `shrinkPreview` is
+ // set. This is the real block visibly resizing to preview the split, not
+ // a second placeholder box drawn over it: `useGridStack`'s own sync
+ // effect diffs this against GridStack's current node and calls `update`
+ // on it exactly the way a committed resize would, then diffs right back
+ // to `packed` the moment `shrinkPreview` clears (hover moves on, or the
+ // drag ends), snapping the real block back to its actual current size.
+ // `resolveGridDropPlacement`/hit-testing below still reads the real,
+ // unmodified `packed` — resolving a placement against an already-shrunk
+ // neighbor would misread where its true edges are.
+ const previewPacked = shrinkPreview
+ ? { ...packed, [shrinkPreview.id]: shrinkPreview.rect }
+ : packed;
- const handleDragStop = useCallback(
- (
- rglLayout: Layout,
- _oldItem: unknown,
- newItem: { i: string } | null,
- _placeholder: unknown,
- event: Event,
- element: HTMLElement | null,
- ) => {
- const { clientX, clientY } = event as MouseEvent;
- const targetContainerId =
- newItem && element && clientX != null && clientY != null
- ? findContainerIdAt(clientX, clientY, element)
- : null;
-
- debugLog('dragStop', {
- containerId: nodeId,
- dragged: describeNode(newItem?.i),
- targetContainerId,
- reparenting: !!targetContainerId && targetContainerId !== nodeId,
- });
+ const { containerRef, registerItem, unregisterItem } = useGridStack({
+ metrics,
+ packed: previewPacked,
+ cancelSelector: cancelSelectorFor(nodeId),
+ onGestureEnd: (items, gesture) =>
+ handleGestureEndRef.current(items, gesture),
+ });
+
+ if (!node) return null;
+
+ const { columns } = metrics;
+
+ /**
+ * The core both `resolveGridDropPlacement` (below, a palette drag's own
+ * `DragEvent`) and the reposition-split check in `handleGestureEnd` (a
+ * `dragstop` gesture's own `clientX`/`clientY`, which carries no
+ * `currentTarget` of its own to measure) ultimately want: what does
+ * `resolveDropPlacement` say about this point, against this occupancy.
+ * The two differ only in *how* they get the point and *which* occupancy
+ * map to ask against — a reposition excludes the item being dragged from
+ * it (it is the thing about to get a new position, not a fixed point to
+ * test anyone else against); a fresh palette block was never in `packed`
+ * to begin with, so the plain, unmodified map is already correct for it.
+ */
+ const resolvePlacementAtPoint = (
+ clientX: number,
+ clientY: number,
+ containerRect: DOMRect,
+ packedForHitTest: Record<string, PackedRect> = packed,
+ ): DropPlacement => {
+ const cellGeometry = resolveCellGeometry(metrics, containerRect.width);
+ const { col, row } = cellAtPoint(
+ clientX - containerRect.left,
+ clientY - containerRect.top,
+ cellGeometry,
+ );
+ const exactCol = Math.min(columns - Number.EPSILON, Math.max(0, col));
+ const exactRow = Math.max(0, row);
+ return resolveDropPlacement(
+ packedForHitTest,
+ columns,
+ exactCol,
+ exactRow,
+ FALLBACK_ROW_SPAN,
+ FALLBACK_COL_SPAN,
+ );
+ };
+
+ /**
+ * Where a palette drag over this grid actually resolves — the single
+ * entry point `handleGridDragOver`'s own live preview and `handleGridDrop`
+ * both call with the same inputs, so what an author sees while hovering is
+ * provably what they get on release.
+ */
+ const resolveGridDropPlacement = (
+ event: ReactDragEvent<HTMLDivElement>,
+ ): DropPlacement =>
+ resolvePlacementAtPoint(
+ event.clientX,
+ event.clientY,
+ event.currentTarget.getBoundingClientRect(),
+ );
- if (targetContainerId && targetContainerId !== nodeId && newItem) {
+ /**
+ * Where a palette drag actually lands, or where an existing block's own
+ * drag/resize settles — the single sink both gestures write through,
+ * mirroring `handleGridDrop`/`handleExternalDrop`'s own "one path, so
+ * preview and outcome can't quietly diverge" reasoning.
+ *
+ * A *drag* (never a resize) can be one of two other things before it is
+ * ever just "committing wherever GridStack's own collision engine parked
+ * everything":
+ *
+ * 1. A reparent — the dragged element's own bounding-rect centre is
+ * hit-tested for a `data-container-id` other than this one. Landing on
+ * itself or one of its own descendants throws (`moveBuildingBlock`'s
+ * own guard); that's caught and falls through to the checks below,
+ * same as any other drag.
+ * 2. A split — the cursor's own final position (`gesture.clientX/clientY`,
+ * which GridStack's own `dd-draggable` copies off the underlying mouseup)
+ * is resolved through the same `resolvePlacementAtPoint` a palette
+ * drop's own preview/drop pair use, against every *other* sibling's own
+ * real, pre-drag position (the dragged item's own entry is excluded —
+ * it is the thing about to get a new position, not a fixed point to
+ * test anyone else against). Landing on a sibling's left/right split
+ * band shrinks that sibling and resizes the dragged item into the other
+ * half, in one commit — dragging an existing block onto another's half
+ * is meant to read as the identical gesture a palette drop landing
+ * there already is, not a lesser version of it that only ever pushes
+ * things down instead.
+ */
+ const handleGestureEnd = (
+ items: GestureEndItem[],
+ gesture: GestureEnd,
+ ): void => {
+ if (gesture.kind === 'drag') {
+ const rect = gesture.el.getBoundingClientRect();
+ const targetContainerId = findContainerIdAt(
+ rect.left + rect.width / 2,
+ rect.top + rect.height / 2,
+ gesture.el,
+ );
+ if (targetContainerId && targetContainerId !== nodeId) {
try {
// moveBuildingBlock itself clears col/row and clamps colSpan to
// the destination's own column count — old coordinates were only
// ever meaningful in *this* container's grid.
const destIndex =
provider.getNode(targetContainerId)?.children?.length ?? 0;
- provider.moveBuildingBlock(newItem.i, targetContainerId, destIndex);
+ provider.moveBuildingBlock(gesture.id, targetContainerId, destIndex);
return;
} catch {
// Dropped onto itself or one of its own descendants — not a valid
- // reparent target. Fall through and keep it in this container.
+ // reparent target. Fall through and commit layout instead.
}
}
- commitLayout(rglLayout);
- },
- [nodeId, commitLayout],
- );
-
- if (!node) return null;
-
- const { columns, gap, rowUnitPx } = resolveGridMetrics(node.layout, theme);
- const children = node.children ?? [];
+ const containerRect = containerRef.current?.getBoundingClientRect();
+ if (containerRect) {
+ const { [gesture.id]: _dragged, ...siblingsOnly } = packed;
+ const placement = resolvePlacementAtPoint(
+ gesture.clientX,
+ gesture.clientY,
+ containerRect,
+ siblingsOnly,
+ );
+ 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);
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Existing-block split can persist overlapping siblings.
- ⚠️ Subsequent grid rendering may reposition affected blocks.
- ⚠️ Stored layout coordinates can diverge from GridStack's final occupancy.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f10fe050d9594cadb5e86ca679ab2f70&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f10fe050d9594cadb5e86ca679ab2f70&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43096&comment_hash=d942c69e4e4e43ffd69d03ed7d7aa928e3b5ba0e1d6d22a360d2716ad96db041&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43096&comment_hash=d942c69e4e4e43ffd69d03ed7d7aa928e3b5ba0e1d6d22a360d2716ad96db041&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/dashboard/RootGrid.tsx:
##########
@@ -16,95 +16,142 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { useCallback } from 'react';
+import { useEffect, useLayoutEffect, useRef, useState } from 'react';
import type { DragEvent as ReactDragEvent } from 'react';
-// The v1-compatible flat-props API (this component's own model: a plain
-// `cols`/`rowHeight`/`margin` grid, no responsive breakpoints — see the
-// design doc's "Explicitly deferred" section) lives at this subpath in v2;
-// the package's main entry exports only the newer composable API.
-import GridLayoutLegacy, { WidthProvider } from 'react-grid-layout/legacy';
-import type { Layout, LayoutItem } from 'react-grid-layout/legacy';
-import 'react-grid-layout/css/styles.css';
+import 'gridstack/dist/gridstack.min.css';
import type { dashboard as dashboardApi } from '@apache-superset/core';
import { css, styled, useTheme } from '@apache-superset/core/theme';
import { provider, useDashboardRevision } from './store';
-import { resolveGridMetrics } from './layoutStyle';
-import { availableDropSpan, packChildLayout } from './gridPacking';
import {
+ cellAtPoint,
+ pixelRectForCell,
+ resolveCellGeometry,
+ resolveGridMetrics,
+} from './layoutStyle';
+import { packChildLayout, resolveDropPlacement } from './gridPacking';
+import type { DropPlacement, PackedRect } from './gridPacking';
+import {
+ FALLBACK_COL_SPAN,
FALLBACK_ROW_SPAN,
PALETTE_MIME,
placeBlock,
placeBlockAt,
} from './placement';
+import { useGridStack } from './useGridStack';
+import type { GestureEnd, GestureEndItem } from './useGridStack';
import BuildingBlockView from './BuildingBlockView';
type LayoutProps = dashboardApi.LayoutProps;
-const ResizableGridLayout = WidthProvider(GridLayoutLegacy);
-
-/** `react-grid-layout`'s own item id for the live drop preview — see `handleDropDragOver`. Distinct from `DashboardProvider`'s own `node_<n>` ids by construction, so it can never collide with a real node while a drag is in progress. */
-const DROPPING_ITEM_ID = '__dropping-block__';
+/**
+ * The `draggableCancel`-equivalent guard: regions a press must never start a
+ * drag from. `[data-container-id]` is a nested container (dragging one chart
+ * out of a `tabs` block must not drag the whole `tabs` block); `[data-block-
+ * remove]` is a block's own remove control; `[data-block-resize]` is a flowed
+ * block's own resize grip (`flowContent.tsx`); `[data-block-header-control]`
+ * is a type's own extra header control (e.g. `collapsible`'s toggle).
+ *
+ * `nodeId` is threaded through and excluded from the `data-container-id`
+ * clause via `:not(...)` — GridStack matches this with a plain, unbounded
+ * `e.target.closest(cancel)`, which walks every ancestor all the way to
+ * `document`, not just the ones between the press and the widget being
+ * dragged. `GridSurface` (this very grid's own outer element, below) itself
+ * carries `data-container-id={nodeId}` — the marker every *other* container
+ * on the page also carries, and the one this grid's own drop-target/defer
+ * checks need it to have — so without the exclusion, *every* press inside
+ * this grid's own boundary matches its own ancestor marker and cancels
+ * every drag unconditionally, leaving only resize (a separate code path
+ * that never consults `cancel` at all) working. A different value here
+ * (any other container actually nested inside the widget being dragged)
+ * still matches and still cancels, exactly as intended.
+ */
+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]`;
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ RootGrid jsdom tests fail during component rendering.
- ❌ Dashboard v2 fails in runtimes lacking `CSS.escape`.
- ⚠️ GridStack initialization never runs.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1890d6daa5594232b107738a6143be34&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1890d6daa5594232b107738a6143be34&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43096&comment_hash=585b5dddc80e4823257ea251eb093eed03eadd8e45848db85317b7e8541f3a7a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43096&comment_hash=585b5dddc80e4823257ea251eb093eed03eadd8e45848db85317b7e8541f3a7a&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Use gridstack for the layout [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43096:
URL: https://github.com/apache/superset/pull/43096#issuecomment-5272114916
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 3161b961b45f5dede9ad54ad2fbc03268e7f1dd6 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a7ccf3d4c0f320008537fd2 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43096--superset-docs-preview.netlify.app](https://deploy-preview-43096--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AstOLMlOKSxKTs3VzEivzS0uAcimpBTn5lZ4pQFmzRPPk5DTjFJNkgzRjIwMDAwtTY_O0FCOlWgA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43096--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Use gridstack for the layout [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43096:
URL: https://github.com/apache/superset/pull/43096#discussion_r3769885862
##########
superset-frontend/src/core/dashboard/placement.ts:
##########
@@ -85,6 +85,20 @@ const DEFAULT_ROW_SPAN: Record<string, number> = {
*/
export const FALLBACK_ROW_SPAN = 6;
+/**
+ * The widest an open-space palette drop is ever allowed to preview or land
+ * at, in columns (half of {@link DEFAULT_COLUMNS}) — `availableDropSpan`'s
+ * own `maxColSpan`. Without a cap, "open space" includes a wholly empty
+ * grid, where nothing anywhere is occupied and the free run in any row
+ * spans every column there is — so a drop there would always be full-width,
+ * regardless of where the cursor actually sits, which reads as the ghost
+ * (and the block it produces) ignoring the drag entirely rather than
+ * following it. A real gap between two existing blocks narrower than this
+ * 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);
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ 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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7644a677be844e17858aa193ef903fc1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7644a677be844e17858aa193ef903fc1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43096&comment_hash=b10470fab142edc82249ec7ff0abd5186ed34f2d6155c737d6356cdb680d37b3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43096&comment_hash=b10470fab142edc82249ec7ff0abd5186ed34f2d6155c737d6356cdb680d37b3&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Use gridstack for the layout [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43096:
URL: https://github.com/apache/superset/pull/43096#issuecomment-5272138864
<!-- Bito Reply -->
The flagged issue is correct. The `FALLBACK_COL_SPAN` constant is currently hardcoded to 12, which causes open-space drops to default to full width even in grids configured with fewer than 12 columns. To resolve this, `FALLBACK_COL_SPAN` should be derived dynamically from the active container's column count rather than being a static constant.
Since `FALLBACK_COL_SPAN` is used in multiple places, you should update its definition in `superset-frontend/src/core/dashboard/placement.ts` to be a function or a value that respects the container's configuration. If you would like me to implement this fix and check the remaining comments on this PR, please let me know.
**superset-frontend/src/core/dashboard/placement.ts**
```
// Instead of a static constant:
// export const FALLBACK_COL_SPAN = Math.floor(DEFAULT_COLUMNS / 2);
// Consider a function that accepts the container's column count:
export const getFallbackColSpan = (columns: number) => Math.floor(columns / 2);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Use gridstack for the layout [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43096:
URL: https://github.com/apache/superset/pull/43096#issuecomment-5272195529
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43096?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `86.45833%` with `39 lines` in your changes missing coverage. Please review.
:warning: Please [upload](https://docs.codecov.com/docs/codecov-uploader) report for BASE (`dashboard-v2@46be6a0`). [Learn more](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#section-missing-base-commit) about missing BASE report.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43096?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset-frontend/src/core/dashboard/RootGrid.tsx](https://app.codecov.io/gh/apache/superset/pull/43096?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fcore%2Fdashboard%2FRootGrid.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvcmUvZGFzaGJvYXJkL1Jvb3RHcmlkLnRzeA==) | 73.98% | [32 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43096?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...et-frontend/src/pages/DashboardBuilderV2/index.tsx](https://app.codecov.io/gh/apache/superset/pull/43096?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fpages%2FDashboardBuilderV2%2Findex.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3BhZ2VzL0Rhc2hib2FyZEJ1aWxkZXJWMi9pbmRleC50c3g=) | 86.48% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43096?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...perset-frontend/src/core/dashboard/useGridStack.ts](https://app.codecov.io/gh/apache/superset/pull/43096?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fcore%2Fdashboard%2FuseGridStack.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvcmUvZGFzaGJvYXJkL3VzZUdyaWRTdGFjay50cw==) | 97.14% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43096?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## dashboard-v2 #43096 +/- ##
===============================================
Coverage ? 65.38%
===============================================
Files ? 2828
Lines ? 159215
Branches ? 36485
===============================================
Hits ? 104100
Misses ? 53138
Partials ? 1977
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43096/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/43096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `71.47% <86.45%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43096?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Use gridstack for the layout [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #43096:
URL: https://github.com/apache/superset/pull/43096
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org