codeant-ai-for-open-source[bot] commented on code in PR #45090: URL: https://github.com/apache/superset/pull/45090#discussion_r4230593380
########## superset-frontend/src/features/canvas/useCanvasLayout.ts: ########## @@ -0,0 +1,187 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useCallback, useEffect, useRef, useState } from 'react'; +import { getClientErrorObject, SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import type { + ApplyOperationsResult, + CanvasDefinitionResult, + GridPlacement, +} from './types'; + +/** The newest layout this client knows of, and the revision it belongs to. */ +interface AppliedLayout { + revision: number; + placements: Record<string, GridPlacement>; +} + +export interface CanvasLayout { + /** Placements to render: the newest local ones, else the server's. */ + placements: Record<string, GridPlacement>; + /** Persist a node's new placement. A no-op when the user can't edit. */ + place: (nodeId: string, placement: GridPlacement) => void; + /** Why the last write failed, for a dismissable notice. */ + error?: string; + dismissError: () => void; +} + +/** + * Persists drag and resize as `place` operations. + * + * The moved widget appears where it was dropped straight away, then takes the + * placements the server resolved β which may differ, since the server pushes + * overlapping widgets down. + * + * Each write is based on the newest revision the server has *confirmed*, taken + * from the previous write's response rather than from the definition. The + * definition's revision only advances when a refetch lands, and that refetch + * is driven by a realtime nudge; without one (no realtime service, a dropped + * socket) it would stay put and every drag after the first would collide with + * the user's own previous drag. + * + * A write while another is still in flight is queued rather than sent with a + * revision the server hasn't reached yet, so dragging quickly coalesces into + * one follow-up request instead of conflicting. + */ +export function useCanvasLayout( + canvasId: number, + result: CanvasDefinitionResult, + reload: () => void, +): CanvasLayout { + const [applied, setApplied] = useState<AppliedLayout>(); + const [error, setError] = useState<string>(); + const { revision, canEdit } = result; + + // Last revision the server acknowledged. Never an optimistic guess, so a + // write is never based on a revision the server hasn't reached. + const confirmed = useRef<number>(); + // The definition's own revision, for the write that hasn't had a response + // yet (first drag after a load or a refetch). + const fetched = useRef(revision); + const writing = useRef(false); + // Newest intent per node while a write is in flight; sent as one batch. + const queued = useRef(new Map<string, GridPlacement>()); + // The definition's placements, for an optimistic view that has no newer + // local layout to build on. + const fetchedPlacements = useRef(result.placements); + + useEffect(() => { + fetched.current = revision; + fetchedPlacements.current = result.placements; + }, [revision, result.placements]); + + useEffect(() => { + // A different canvas shares none of this state. + confirmed.current = undefined; + writing.current = false; + queued.current.clear(); + setApplied(undefined); + setError(undefined); + }, [canvasId]); + + const send = useCallback( + (ops: Map<string, GridPlacement>) => { + writing.current = true; + const base = Math.max(fetched.current, confirmed.current ?? 0); + SupersetClient.request({ + endpoint: `/api/v1/canvas/${canvasId}/definition`, + method: 'PATCH', + jsonPayload: { + base_revision: base, + ops: [...ops].map(([id, layout]) => ({ op: 'place', id, layout })), + }, + }) + .then(({ json }) => { + const written = json?.result as ApplyOperationsResult | undefined; + if (written) { + confirmed.current = written.revision; + setApplied({ + revision: written.revision, + placements: written.placements, + }); Review Comment: **Suggestion:** A queued drag's optimistic placement is overwritten by the first PATCH response, so the widget snaps back until the follow-up request completes. **Assessment:** π `Major` Β· π `Occurrence: Sometimes` Β· π·οΈ `Logic error` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=71a667d4bbfa4ff59448ce593ffb2b86&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=71a667d4bbfa4ff59448ce593ffb2b86&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/features/canvas/useCanvasLayout.ts **Line:** 115:118 **Comment:** *Logic Error: A queued drag's optimistic placement is overwritten by the first PATCH response, so the widget snaps back until the follow-up request completes. 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%2F45090&comment_hash=052c9610530db955fc521739ede20f920b584f0d438758a8544a45f1e92bf4c4&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45090&comment_hash=052c9610530db955fc521739ede20f920b584f0d438758a8544a45f1e92bf4c4&reaction=dislike'>π</a> ########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,420 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview One grid of a canvas, with drag and resize built in. + * + * There is no edit mode: a user who may edit gets a drag grip and a resize + * corner on each widget, on the canvas as they view it. A user who may not + * gets the same grid with no handles. Dragging never covers the widget's own + * surface, so a filter or chart underneath stays clickable. + * + * While a gesture is live the widget follows the pointer and a dashed outline + * marks the cells it would take; releasing persists exactly those cells. + */ + +import { + PointerEvent as ReactPointerEvent, + KeyboardEvent as ReactKeyboardEvent, + ReactNode, + useCallback, + useEffect, + useRef, + useState, +} from 'react'; +import { css, styled } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core/translation'; +import { Icons } from '@superset-ui/core/components'; +import { + GridMetrics, + gridDelta, + movedPlacement, + NO_DELTA, + overlappedSiblings, + resizedPlacement, + samePlacement, +} from './gridGeometry'; +import type { GridPlacement, SpanConstraints } from './types'; + +type GestureKind = 'move' | 'resize'; + +interface Gesture { + nodeId: string; + kind: GestureKind; + pointerId: number; + startX: number; + startY: number; + dx: number; + dy: number; +} + +const Grid = styled.div<GridMetrics>` + display: grid; + grid-template-columns: repeat(${({ columns }) => columns}, minmax(0, 1fr)); + grid-auto-rows: ${({ rowUnit }) => rowUnit}px; + gap: ${({ gap }) => gap}px; +`; + +const GridItem = styled.div<{ + placement?: GridPlacement; + dragging?: boolean; + offset?: { x: number; y: number }; +}>` + ${({ placement, dragging, offset }) => css` + min-width: 0; + min-height: 0; + position: relative; + ${ + placement && + css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + ` + } + ${ + dragging && + offset && + css` + /* Follow the pointer without reflowing the grid under it. */ + transform: translate(${offset.x}px, ${offset.y}px); + z-index: 2; + opacity: 0.85; + ` + } + `} +`; + +/** + * Where the widget being dragged would land. Turns red once it covers a + * sibling, so the user sees an overlap before releasing rather than being + * surprised when the server pushes a widget down to resolve it. + */ +const DropPreview = styled.div<{ + placement: GridPlacement; + colliding?: boolean; +}>` + ${({ theme, placement, colliding }) => css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + border: 2px dashed ${colliding ? theme.colorError : theme.colorPrimary}; + border-radius: ${theme.borderRadius}px; + background: ${colliding ? theme.colorErrorBg : theme.colorPrimaryBg}; + pointer-events: none; + z-index: 1; + `} +`; + +/** + * Wraps a widget so its handles can sit on top of it. The handles stay hidden + * until the pointer is over the widget or a handle has focus, so a canvas at + * rest looks the same whether or not the user may edit it. + */ +const Interactive = styled.div` + ${({ theme }) => css` + height: 100%; + position: relative; + + /* Direct children only: hovering a container must not reveal the + handles of every widget nested inside it. */ + & > .canvas-handle { + opacity: 0; + transition: opacity ${theme.motionDurationMid}; + } + + &:hover > .canvas-handle, + & > .canvas-handle:focus-visible { + opacity: 1; + } + `} +`; + +const Grip = styled.button` + ${({ theme }) => css` + position: absolute; + top: ${theme.sizeUnit}px; + right: ${theme.sizeUnit}px; + z-index: 3; + display: flex; + align-items: center; + justify-content: center; + padding: ${theme.sizeUnit / 2}px; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.borderRadiusSM}px; + background: ${theme.colorBgElevated}; + color: ${theme.colorTextSecondary}; + cursor: grab; + touch-action: none; + + &:active { + cursor: grabbing; + } + `} +`; + +const ResizeCorner = styled.button` + ${({ theme }) => css` + position: absolute; + right: 0; + bottom: 0; + z-index: 3; + width: ${theme.sizeUnit * 4}px; + height: ${theme.sizeUnit * 4}px; + padding: 0; + border: none; + background: transparent; + cursor: nwse-resize; + touch-action: none; + + /* A two-line corner, the conventional resize affordance. */ + &::after { + content: ''; + position: absolute; + right: ${theme.sizeUnit / 2}px; + bottom: ${theme.sizeUnit / 2}px; + width: ${theme.sizeUnit * 2}px; + height: ${theme.sizeUnit * 2}px; + border-right: 2px solid ${theme.colorTextTertiary}; + border-bottom: 2px solid ${theme.colorTextTertiary}; + } + `} +`; + +/** The grid's content width, kept current as the viewport changes. */ +function useElementWidth(): [(element: HTMLDivElement | null) => void, number] { + const [element, setElement] = useState<HTMLDivElement | null>(null); + const [width, setWidth] = useState(0); + + useEffect(() => { + if (!element) return undefined; + const measure = () => setWidth(element.clientWidth); + measure(); + if (typeof ResizeObserver === 'undefined') return undefined; + const observer = new ResizeObserver(measure); + observer.observe(element); + return () => observer.disconnect(); + }, [element]); + + return [setElement, width]; +} + +export interface CanvasGridSurfaceProps { + childIds: string[]; + columns: number; + gap: number; + rowUnit: number; + placements: Record<string, GridPlacement>; + constraints: Record<string, SpanConstraints>; + /** Whether the user may drag and resize these widgets. */ + editable: boolean; + onPlace: (nodeId: string, placement: GridPlacement) => void; + renderNode: (nodeId: string) => ReactNode; +} + +export default function CanvasGridSurface({ + childIds, + columns, + gap, + rowUnit, + placements, + constraints, + editable, + onPlace, + renderNode, +}: CanvasGridSurfaceProps) { + const [gridRef, width] = useElementWidth(); + const [gesture, setGesture] = useState<Gesture>(); + // The handlers read the gesture directly, so committing never runs as a + // side effect inside a state updater. + const live = useRef<Gesture>(); + + const update = useCallback((next: Gesture | undefined) => { + live.current = next; + setGesture(next); + }, []); + + /** Where `nodeId` would land given a pointer or keyboard delta. */ + const targetOf = useCallback( + (nodeId: string, kind: GestureKind, dx: number, dy: number) => { + const placement = placements[nodeId]; + if (!placement) return undefined; + const metrics: GridMetrics = { columns, gap, rowUnit }; + const delta = width > 0 ? gridDelta(dx, dy, width, metrics) : NO_DELTA; + return kind === 'move' + ? movedPlacement(placement, delta, columns) + : resizedPlacement(placement, delta, columns, constraints[nodeId]); + }, + [placements, width, columns, gap, rowUnit, constraints], + ); + + const commit = useCallback( + (nodeId: string, next: GridPlacement | undefined) => { + const placement = placements[nodeId]; + if (next && placement && !samePlacement(next, placement)) { + onPlace(nodeId, next); + } + }, + [onPlace, placements], + ); + + const begin = + (nodeId: string, kind: GestureKind) => + (event: ReactPointerEvent<HTMLButtonElement>) => { + if (event.button !== 0) return; + event.preventDefault(); + event.stopPropagation(); + event.currentTarget.setPointerCapture(event.pointerId); + update({ + nodeId, + kind, + pointerId: event.pointerId, + startX: event.clientX, + startY: event.clientY, + dx: 0, + dy: 0, + }); + }; + + const onPointerMove = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update({ + ...current, + dx: event.clientX - current.startX, + dy: event.clientY - current.startY, + }); + }; + + const onPointerUp = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update(undefined); + commit( + current.nodeId, + targetOf(current.nodeId, current.kind, current.dx, current.dy), + ); + }; + + const onPointerCancel = () => update(undefined); Review Comment: **Suggestion:** If two pointers are active, a cancel from one clears the other pointerβs gesture because this handler ignores `pointerId`; the other gesture then cannot be committed. **Assessment:** π `Major` Β· π `Occurrence: Rarely` Β· π·οΈ `Race condition` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=678305b4cb304fedab41489b3407a2cd&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=678305b4cb304fedab41489b3407a2cd&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/features/canvas/CanvasGridSurface.tsx **Line:** 313:313 **Comment:** *Race Condition: If two pointers are active, a cancel from one clears the other pointerβs gesture because this handler ignores `pointerId`; the other gesture then cannot be committed. 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%2F45090&comment_hash=61c5a609353106ee7c831fee87321dc9b2d36c32b00d691e3efc60372bf8865b&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45090&comment_hash=61c5a609353106ee7c831fee87321dc9b2d36c32b00d691e3efc60372bf8865b&reaction=dislike'>π</a> ########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,420 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview One grid of a canvas, with drag and resize built in. + * + * There is no edit mode: a user who may edit gets a drag grip and a resize + * corner on each widget, on the canvas as they view it. A user who may not + * gets the same grid with no handles. Dragging never covers the widget's own + * surface, so a filter or chart underneath stays clickable. + * + * While a gesture is live the widget follows the pointer and a dashed outline + * marks the cells it would take; releasing persists exactly those cells. + */ + +import { + PointerEvent as ReactPointerEvent, + KeyboardEvent as ReactKeyboardEvent, + ReactNode, + useCallback, + useEffect, + useRef, + useState, +} from 'react'; +import { css, styled } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core/translation'; +import { Icons } from '@superset-ui/core/components'; +import { + GridMetrics, + gridDelta, + movedPlacement, + NO_DELTA, + overlappedSiblings, + resizedPlacement, + samePlacement, +} from './gridGeometry'; +import type { GridPlacement, SpanConstraints } from './types'; + +type GestureKind = 'move' | 'resize'; + +interface Gesture { + nodeId: string; + kind: GestureKind; + pointerId: number; + startX: number; + startY: number; + dx: number; + dy: number; +} + +const Grid = styled.div<GridMetrics>` + display: grid; + grid-template-columns: repeat(${({ columns }) => columns}, minmax(0, 1fr)); + grid-auto-rows: ${({ rowUnit }) => rowUnit}px; + gap: ${({ gap }) => gap}px; +`; + +const GridItem = styled.div<{ + placement?: GridPlacement; + dragging?: boolean; + offset?: { x: number; y: number }; +}>` + ${({ placement, dragging, offset }) => css` + min-width: 0; + min-height: 0; + position: relative; + ${ + placement && + css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + ` + } + ${ + dragging && + offset && + css` + /* Follow the pointer without reflowing the grid under it. */ + transform: translate(${offset.x}px, ${offset.y}px); + z-index: 2; + opacity: 0.85; + ` + } + `} +`; + +/** + * Where the widget being dragged would land. Turns red once it covers a + * sibling, so the user sees an overlap before releasing rather than being + * surprised when the server pushes a widget down to resolve it. + */ +const DropPreview = styled.div<{ + placement: GridPlacement; + colliding?: boolean; +}>` + ${({ theme, placement, colliding }) => css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + border: 2px dashed ${colliding ? theme.colorError : theme.colorPrimary}; + border-radius: ${theme.borderRadius}px; + background: ${colliding ? theme.colorErrorBg : theme.colorPrimaryBg}; + pointer-events: none; + z-index: 1; + `} +`; + +/** + * Wraps a widget so its handles can sit on top of it. The handles stay hidden + * until the pointer is over the widget or a handle has focus, so a canvas at + * rest looks the same whether or not the user may edit it. + */ +const Interactive = styled.div` + ${({ theme }) => css` + height: 100%; + position: relative; + + /* Direct children only: hovering a container must not reveal the + handles of every widget nested inside it. */ + & > .canvas-handle { + opacity: 0; + transition: opacity ${theme.motionDurationMid}; + } + + &:hover > .canvas-handle, + & > .canvas-handle:focus-visible { + opacity: 1; + } + `} +`; + +const Grip = styled.button` + ${({ theme }) => css` + position: absolute; + top: ${theme.sizeUnit}px; + right: ${theme.sizeUnit}px; + z-index: 3; + display: flex; + align-items: center; + justify-content: center; + padding: ${theme.sizeUnit / 2}px; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.borderRadiusSM}px; + background: ${theme.colorBgElevated}; + color: ${theme.colorTextSecondary}; + cursor: grab; + touch-action: none; + + &:active { + cursor: grabbing; + } + `} +`; + +const ResizeCorner = styled.button` + ${({ theme }) => css` + position: absolute; + right: 0; + bottom: 0; + z-index: 3; + width: ${theme.sizeUnit * 4}px; + height: ${theme.sizeUnit * 4}px; + padding: 0; + border: none; + background: transparent; + cursor: nwse-resize; + touch-action: none; + + /* A two-line corner, the conventional resize affordance. */ + &::after { + content: ''; + position: absolute; + right: ${theme.sizeUnit / 2}px; + bottom: ${theme.sizeUnit / 2}px; + width: ${theme.sizeUnit * 2}px; + height: ${theme.sizeUnit * 2}px; + border-right: 2px solid ${theme.colorTextTertiary}; + border-bottom: 2px solid ${theme.colorTextTertiary}; + } + `} +`; + +/** The grid's content width, kept current as the viewport changes. */ +function useElementWidth(): [(element: HTMLDivElement | null) => void, number] { + const [element, setElement] = useState<HTMLDivElement | null>(null); + const [width, setWidth] = useState(0); + + useEffect(() => { + if (!element) return undefined; + const measure = () => setWidth(element.clientWidth); + measure(); + if (typeof ResizeObserver === 'undefined') return undefined; + const observer = new ResizeObserver(measure); + observer.observe(element); + return () => observer.disconnect(); + }, [element]); + + return [setElement, width]; +} + +export interface CanvasGridSurfaceProps { + childIds: string[]; + columns: number; + gap: number; + rowUnit: number; + placements: Record<string, GridPlacement>; + constraints: Record<string, SpanConstraints>; + /** Whether the user may drag and resize these widgets. */ + editable: boolean; + onPlace: (nodeId: string, placement: GridPlacement) => void; + renderNode: (nodeId: string) => ReactNode; +} + +export default function CanvasGridSurface({ + childIds, + columns, + gap, + rowUnit, + placements, + constraints, + editable, + onPlace, + renderNode, +}: CanvasGridSurfaceProps) { + const [gridRef, width] = useElementWidth(); + const [gesture, setGesture] = useState<Gesture>(); + // The handlers read the gesture directly, so committing never runs as a + // side effect inside a state updater. + const live = useRef<Gesture>(); + + const update = useCallback((next: Gesture | undefined) => { + live.current = next; + setGesture(next); + }, []); + + /** Where `nodeId` would land given a pointer or keyboard delta. */ + const targetOf = useCallback( + (nodeId: string, kind: GestureKind, dx: number, dy: number) => { + const placement = placements[nodeId]; + if (!placement) return undefined; + const metrics: GridMetrics = { columns, gap, rowUnit }; + const delta = width > 0 ? gridDelta(dx, dy, width, metrics) : NO_DELTA; + return kind === 'move' + ? movedPlacement(placement, delta, columns) + : resizedPlacement(placement, delta, columns, constraints[nodeId]); + }, + [placements, width, columns, gap, rowUnit, constraints], + ); + + const commit = useCallback( + (nodeId: string, next: GridPlacement | undefined) => { + const placement = placements[nodeId]; + if (next && placement && !samePlacement(next, placement)) { + onPlace(nodeId, next); + } + }, + [onPlace, placements], + ); + + const begin = + (nodeId: string, kind: GestureKind) => + (event: ReactPointerEvent<HTMLButtonElement>) => { + if (event.button !== 0) return; + event.preventDefault(); + event.stopPropagation(); + event.currentTarget.setPointerCapture(event.pointerId); + update({ + nodeId, + kind, + pointerId: event.pointerId, + startX: event.clientX, + startY: event.clientY, + dx: 0, + dy: 0, + }); + }; + + const onPointerMove = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update({ + ...current, + dx: event.clientX - current.startX, + dy: event.clientY - current.startY, + }); + }; + + const onPointerUp = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update(undefined); + commit( + current.nodeId, + targetOf(current.nodeId, current.kind, current.dx, current.dy), + ); + }; + + const onPointerCancel = () => update(undefined); + + /** + * Arrow keys do what dragging does, one cell at a time, so the layout is + * reachable without a pointer. Shift resizes instead of moving. + */ + const onKeyDown = + (nodeId: string, kind: GestureKind) => + (event: ReactKeyboardEvent<HTMLButtonElement>) => { + const steps: Record<string, [number, number]> = { + ArrowLeft: [-1, 0], + ArrowRight: [1, 0], + ArrowUp: [0, -1], + ArrowDown: [0, 1], + }; + const step = steps[event.key]; + if (!step) return; + event.preventDefault(); + const placement = placements[nodeId]; + if (!placement) return; + const delta = { cols: step[0], rows: step[1] }; + commit( + nodeId, + kind === 'move' + ? movedPlacement(placement, delta, columns) + : resizedPlacement(placement, delta, columns, constraints[nodeId]), Review Comment: **Suggestion:** Shift is not checked, so Shift+arrow on the move handle still moves the widget instead of resizing it, contrary to the documented keyboard behavior. **Assessment:** π `Major` Β· π `Occurrence: Sometimes` Β· π·οΈ `Incorrect condition logic` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c924673013c64e7188cf7bf507236b5d&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=c924673013c64e7188cf7bf507236b5d&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/features/canvas/CanvasGridSurface.tsx **Line:** 336:338 **Comment:** *Incorrect Condition Logic: Shift is not checked, so Shift+arrow on the move handle still moves the widget instead of resizing it, contrary to the documented keyboard behavior. 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%2F45090&comment_hash=74fedabd2a1b336cda97f1a9ac76d14ab5ce643aa034c9036e74fdf3b04c3767&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45090&comment_hash=74fedabd2a1b336cda97f1a9ac76d14ab5ce643aa034c9036e74fdf3b04c3767&reaction=dislike'>π</a> ########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,420 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview One grid of a canvas, with drag and resize built in. + * + * There is no edit mode: a user who may edit gets a drag grip and a resize + * corner on each widget, on the canvas as they view it. A user who may not + * gets the same grid with no handles. Dragging never covers the widget's own + * surface, so a filter or chart underneath stays clickable. + * + * While a gesture is live the widget follows the pointer and a dashed outline + * marks the cells it would take; releasing persists exactly those cells. + */ + +import { + PointerEvent as ReactPointerEvent, + KeyboardEvent as ReactKeyboardEvent, + ReactNode, + useCallback, + useEffect, + useRef, + useState, +} from 'react'; +import { css, styled } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core/translation'; +import { Icons } from '@superset-ui/core/components'; +import { + GridMetrics, + gridDelta, + movedPlacement, + NO_DELTA, + overlappedSiblings, + resizedPlacement, + samePlacement, +} from './gridGeometry'; +import type { GridPlacement, SpanConstraints } from './types'; + +type GestureKind = 'move' | 'resize'; + +interface Gesture { + nodeId: string; + kind: GestureKind; + pointerId: number; + startX: number; + startY: number; + dx: number; + dy: number; +} + +const Grid = styled.div<GridMetrics>` + display: grid; + grid-template-columns: repeat(${({ columns }) => columns}, minmax(0, 1fr)); + grid-auto-rows: ${({ rowUnit }) => rowUnit}px; + gap: ${({ gap }) => gap}px; +`; + +const GridItem = styled.div<{ + placement?: GridPlacement; + dragging?: boolean; + offset?: { x: number; y: number }; +}>` + ${({ placement, dragging, offset }) => css` + min-width: 0; + min-height: 0; + position: relative; + ${ + placement && + css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + ` + } + ${ + dragging && + offset && + css` + /* Follow the pointer without reflowing the grid under it. */ + transform: translate(${offset.x}px, ${offset.y}px); + z-index: 2; + opacity: 0.85; + ` + } + `} +`; + +/** + * Where the widget being dragged would land. Turns red once it covers a + * sibling, so the user sees an overlap before releasing rather than being + * surprised when the server pushes a widget down to resolve it. + */ +const DropPreview = styled.div<{ + placement: GridPlacement; + colliding?: boolean; +}>` + ${({ theme, placement, colliding }) => css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + border: 2px dashed ${colliding ? theme.colorError : theme.colorPrimary}; + border-radius: ${theme.borderRadius}px; + background: ${colliding ? theme.colorErrorBg : theme.colorPrimaryBg}; + pointer-events: none; + z-index: 1; + `} +`; + +/** + * Wraps a widget so its handles can sit on top of it. The handles stay hidden + * until the pointer is over the widget or a handle has focus, so a canvas at + * rest looks the same whether or not the user may edit it. + */ +const Interactive = styled.div` + ${({ theme }) => css` + height: 100%; + position: relative; + + /* Direct children only: hovering a container must not reveal the + handles of every widget nested inside it. */ + & > .canvas-handle { + opacity: 0; + transition: opacity ${theme.motionDurationMid}; + } + + &:hover > .canvas-handle, + & > .canvas-handle:focus-visible { + opacity: 1; + } + `} +`; + +const Grip = styled.button` + ${({ theme }) => css` + position: absolute; + top: ${theme.sizeUnit}px; + right: ${theme.sizeUnit}px; + z-index: 3; + display: flex; + align-items: center; + justify-content: center; + padding: ${theme.sizeUnit / 2}px; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.borderRadiusSM}px; + background: ${theme.colorBgElevated}; + color: ${theme.colorTextSecondary}; + cursor: grab; + touch-action: none; + + &:active { + cursor: grabbing; + } + `} +`; + +const ResizeCorner = styled.button` + ${({ theme }) => css` + position: absolute; + right: 0; + bottom: 0; + z-index: 3; + width: ${theme.sizeUnit * 4}px; + height: ${theme.sizeUnit * 4}px; + padding: 0; + border: none; + background: transparent; + cursor: nwse-resize; + touch-action: none; + + /* A two-line corner, the conventional resize affordance. */ + &::after { + content: ''; + position: absolute; + right: ${theme.sizeUnit / 2}px; + bottom: ${theme.sizeUnit / 2}px; + width: ${theme.sizeUnit * 2}px; + height: ${theme.sizeUnit * 2}px; + border-right: 2px solid ${theme.colorTextTertiary}; + border-bottom: 2px solid ${theme.colorTextTertiary}; + } + `} +`; + +/** The grid's content width, kept current as the viewport changes. */ +function useElementWidth(): [(element: HTMLDivElement | null) => void, number] { + const [element, setElement] = useState<HTMLDivElement | null>(null); + const [width, setWidth] = useState(0); + + useEffect(() => { + if (!element) return undefined; + const measure = () => setWidth(element.clientWidth); + measure(); + if (typeof ResizeObserver === 'undefined') return undefined; + const observer = new ResizeObserver(measure); + observer.observe(element); + return () => observer.disconnect(); + }, [element]); + + return [setElement, width]; +} + +export interface CanvasGridSurfaceProps { + childIds: string[]; + columns: number; + gap: number; + rowUnit: number; + placements: Record<string, GridPlacement>; + constraints: Record<string, SpanConstraints>; + /** Whether the user may drag and resize these widgets. */ + editable: boolean; + onPlace: (nodeId: string, placement: GridPlacement) => void; + renderNode: (nodeId: string) => ReactNode; +} + +export default function CanvasGridSurface({ + childIds, + columns, + gap, + rowUnit, + placements, + constraints, + editable, + onPlace, + renderNode, +}: CanvasGridSurfaceProps) { + const [gridRef, width] = useElementWidth(); + const [gesture, setGesture] = useState<Gesture>(); + // The handlers read the gesture directly, so committing never runs as a + // side effect inside a state updater. + const live = useRef<Gesture>(); + + const update = useCallback((next: Gesture | undefined) => { + live.current = next; + setGesture(next); + }, []); + + /** Where `nodeId` would land given a pointer or keyboard delta. */ + const targetOf = useCallback( + (nodeId: string, kind: GestureKind, dx: number, dy: number) => { + const placement = placements[nodeId]; + if (!placement) return undefined; + const metrics: GridMetrics = { columns, gap, rowUnit }; + const delta = width > 0 ? gridDelta(dx, dy, width, metrics) : NO_DELTA; + return kind === 'move' + ? movedPlacement(placement, delta, columns) + : resizedPlacement(placement, delta, columns, constraints[nodeId]); + }, + [placements, width, columns, gap, rowUnit, constraints], + ); + + const commit = useCallback( + (nodeId: string, next: GridPlacement | undefined) => { + const placement = placements[nodeId]; + if (next && placement && !samePlacement(next, placement)) { + onPlace(nodeId, next); + } + }, + [onPlace, placements], + ); + + const begin = + (nodeId: string, kind: GestureKind) => + (event: ReactPointerEvent<HTMLButtonElement>) => { + if (event.button !== 0) return; + event.preventDefault(); + event.stopPropagation(); + event.currentTarget.setPointerCapture(event.pointerId); + update({ + nodeId, + kind, + pointerId: event.pointerId, + startX: event.clientX, + startY: event.clientY, + dx: 0, + dy: 0, + }); + }; + + const onPointerMove = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update({ + ...current, + dx: event.clientX - current.startX, + dy: event.clientY - current.startY, + }); + }; + + const onPointerUp = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update(undefined); + commit( + current.nodeId, + targetOf(current.nodeId, current.kind, current.dx, current.dy), + ); Review Comment: **Suggestion:** If the pointer moves after its last move event but before release, the final position is ignored, so the widget can drop short of where the user released it. **Assessment:** π `Major` Β· π `Occurrence: Rarely` Β· π·οΈ `Logic error` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=18c07bf95a8b43a6be2a831698a53daa&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=18c07bf95a8b43a6be2a831698a53daa&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/features/canvas/CanvasGridSurface.tsx **Line:** 306:310 **Comment:** *Logic Error: If the pointer moves after its last move event but before release, the final position is ignored, so the widget can drop short of where the user released it. 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%2F45090&comment_hash=0be2b0a4c570a69839c90bddff548615c95a2c3944d89d8c570981ef78f513dd&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45090&comment_hash=0be2b0a4c570a69839c90bddff548615c95a2c3944d89d8c570981ef78f513dd&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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
