sadpandajoe commented on code in PR #45090: URL: https://github.com/apache/superset/pull/45090#discussion_r4248315109
########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,435 @@ +/** + * 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]; Review Comment: `targetOf` applies the pointer delta to the latest `placements[nodeId]` instead of the placement captured at pointer-down. If widget A's PATCH resolves mid-gesture and pushes widget B to a different row while B is being dragged, B jumps and the drop persists relative to the new row rather than where the user grabbed it. Should the gesture snapshot its starting placement? ########## superset-frontend/src/pages/Canvas/index.tsx: ########## @@ -101,10 +117,93 @@ const notFound = ( /> ); +interface CanvasBodyProps { + canvasId: number; + result: CanvasDefinitionResult; + reload: () => void; + values: ScopeValues; + onValueChange: ( + kind: canvasApi.ScopeKind, + nodeId: string, + value: unknown, + ) => void; + refreshKeys: Record<string, number>; + /** Set while a draft is open; layout writes then go to the draft. */ + draftToken?: string; + /** Leaves the draft, returning to the published canvas. */ + onDraftClosed: () => void; + /** True when the canvas holds no widgets yet. */ + isEmpty: boolean; +} + +/** + * The canvas itself, once its definition has loaded. Drag and resize persist + * from here; a write that doesn't land says so and leaves the layout where the + * server still has it. + */ +function CanvasBody({ + canvasId, + result, + reload, + values, + onValueChange, + refreshKeys, + draftToken, + onDraftClosed, + isEmpty, +}: CanvasBodyProps) { + const layout = useCanvasLayout(canvasId, result, reload, draftToken); + // Changes are made in a plan; the published canvas is read-only even for + // someone who may edit it. The single place this policy is decided. + const editable = result.canEdit && Boolean(draftToken); + return ( + <> + {draftToken && ( + <DraftBanner + token={draftToken} + // The revision the layout writes have reached, not the one the + // definition was fetched at: a commit must name the draft's current + // revision, and dragging moves it on well before the next poll. + revision={layout.revision} Review Comment: Publish stays enabled while a drag or resize PATCH is in flight or queued, and `layout.revision` only reflects acknowledged writes. If someone drops a widget and clicks Publish before that PATCH lands, the commit is accepted at the older revision, the draft is deleted, and the late PATCH fails with its error unmounted along with the draft UI - so the visible move is silently lost. Should the banner be told about pending writes and hold Publish until the queue drains? ########## superset-frontend/src/features/canvas/useCanvasLayout.test.ts: ########## @@ -0,0 +1,362 @@ +/** + * 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 { act, renderHook, waitFor } from '@testing-library/react'; +import fetchMock from 'fetch-mock'; +import { useCanvasLayout } from './useCanvasLayout'; +import type { CanvasDefinitionResult, GridPlacement } from './types'; + +const endpoint = 'glob:*/api/v1/canvas/7/definition'; + +const KPI: GridPlacement = { col: 1, row: 1, colSpan: 6, rowSpan: 4 }; +const TREND: GridPlacement = { col: 1, row: 5, colSpan: 24, rowSpan: 8 }; +const MOVED: GridPlacement = { col: 7, row: 1, colSpan: 6, rowSpan: 4 }; +const QUEUED: GridPlacement = { col: 1, row: 14, colSpan: 24, rowSpan: 8 }; + +const resultAt = ( + revision: number, + canEdit = true, + placements: Record<string, GridPlacement> = { kpi: KPI, trend: TREND }, +) => + ({ + revision, + canEdit, + placements, + }) as CanvasDefinitionResult; + +beforeEach(() => { + fetchMock.removeRoutes(); + fetchMock.clearHistory(); +}); + +test('placements come straight from the definition before any edit', () => { + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + expect(result.current.placements).toEqual({ kpi: KPI, trend: TREND }); + expect(result.current.error).toBeUndefined(); +}); + +test('a placement shows at once and is sent as a place op', async () => { + fetchMock.patch(endpoint, { + result: { revision: 4, ops: [], placements: { kpi: MOVED, trend: TREND } }, + }); + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + act(() => result.current.place('kpi', MOVED)); + + // Shown before the server has answered. + expect(result.current.placements.kpi).toEqual(MOVED); + + await waitFor(() => expect(fetchMock.callHistory.calls()).toHaveLength(1)); + const [call] = fetchMock.callHistory.calls(); + expect(call.options.method?.toUpperCase()).toBe('PATCH'); + expect(JSON.parse(call.options.body as string)).toEqual({ + base_revision: 3, + ops: [{ op: 'place', id: 'kpi', layout: MOVED }], + }); +}); + +test("the server's resolved placements replace the optimistic ones", async () => { + // The server pushed the trend down to make room, which the client never + // guesses for itself. + const pushed: GridPlacement = { col: 1, row: 9, colSpan: 24, rowSpan: 8 }; + fetchMock.patch(endpoint, { + result: { revision: 4, ops: [], placements: { kpi: MOVED, trend: pushed } }, + }); + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + act(() => result.current.place('kpi', MOVED)); + + await waitFor(() => expect(result.current.placements.trend).toEqual(pushed)); +}); + +test('a refetch that catches up takes over from the pending view', async () => { + fetchMock.patch(endpoint, { + result: { revision: 4, ops: [], placements: { kpi: MOVED, trend: TREND } }, + }); + const { result, rerender } = renderHook( + ({ revision }: { revision: number }) => + useCanvasLayout(7, resultAt(revision), jest.fn()), + { initialProps: { revision: 3 } }, + ); + + act(() => result.current.place('kpi', MOVED)); + await waitFor(() => expect(result.current.placements.kpi).toEqual(MOVED)); + + // The realtime nudge's refetch arrives at the revision the write produced, + // so the definition is authoritative again. + rerender({ revision: 4 }); + + expect(result.current.placements).toEqual({ kpi: KPI, trend: TREND }); +}); + +test('a failed write snaps back and explains itself', async () => { + fetchMock.patch(endpoint, 422); + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + act(() => result.current.place('kpi', MOVED)); + expect(result.current.placements.kpi).toEqual(MOVED); + + await waitFor(() => expect(result.current.error).toBeDefined()); + expect(result.current.placements.kpi).toEqual(KPI); + + act(() => result.current.dismissError()); + expect(result.current.error).toBeUndefined(); +}); + +test('a conflicting write reloads the canvas and says so', async () => { + fetchMock.patch(endpoint, 409); + const reload = jest.fn(); + const { result } = renderHook(() => useCanvasLayout(7, resultAt(3), reload)); + + act(() => result.current.place('kpi', MOVED)); + + await waitFor(() => expect(reload).toHaveBeenCalled()); + expect(result.current.error).toContain('changed while you were editing'); + expect(result.current.placements.kpi).toEqual(KPI); +}); + +test('a refused write names permission as the reason', async () => { + fetchMock.patch(endpoint, 403); + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + act(() => result.current.place('kpi', MOVED)); + + await waitFor(() => + expect(result.current.error).toContain('do not have permission'), + ); +}); + +test('a user without edit permission writes nothing', () => { + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3, false), jest.fn()), + ); + + act(() => result.current.place('kpi', MOVED)); + + expect(fetchMock.callHistory.calls()).toHaveLength(0); + expect(result.current.placements.kpi).toEqual(KPI); +}); + +test('a second drag is based on the revision the first write returned', async () => { + // The regression this guards: basing every write on the definition's + // revision, which only advances when a realtime nudge triggers a refetch. + // Without one, the second drag collided with the user's own first drag. + fetchMock.patch(endpoint, { + result: { revision: 4, ops: [], placements: { kpi: MOVED, trend: TREND } }, + }); + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + act(() => result.current.place('kpi', MOVED)); + await waitFor(() => expect(fetchMock.callHistory.calls()).toHaveLength(1)); + + const again: GridPlacement = { col: 13, row: 1, colSpan: 6, rowSpan: 4 }; + act(() => result.current.place('kpi', again)); + await waitFor(() => expect(fetchMock.callHistory.calls()).toHaveLength(2)); + + const bases = fetchMock.callHistory + .calls() + .map(call => JSON.parse(call.options.body as string).base_revision); + // 3 is the loaded definition; 4 is what the first write returned. + expect(bases).toEqual([3, 4]); +}); + +test('drags made while a write is in flight are coalesced, not conflicted', async () => { + fetchMock.patch(endpoint, { + result: { revision: 4, ops: [], placements: { kpi: MOVED, trend: TREND } }, + }); + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + const second: GridPlacement = { col: 13, row: 1, colSpan: 6, rowSpan: 4 }; + const third: GridPlacement = { col: 19, row: 1, colSpan: 6, rowSpan: 4 }; + act(() => { + // Three gestures before the first response can land. + result.current.place('kpi', MOVED); + result.current.place('kpi', second); + result.current.place('trend', third); + }); + + // The last intent per node is shown straight away. + expect(result.current.placements.kpi).toEqual(second); + expect(result.current.placements.trend).toEqual(third); + + await waitFor(() => expect(fetchMock.callHistory.calls()).toHaveLength(2)); + const [first, follow] = fetchMock.callHistory + .calls() + .map(call => JSON.parse(call.options.body as string)); + expect(first.ops).toEqual([{ op: 'place', id: 'kpi', layout: MOVED }]); + // The queued gestures go out together, on the confirmed revision. + expect(follow.base_revision).toBe(4); + expect(follow.ops).toEqual([ + { op: 'place', id: 'kpi', layout: second }, + { op: 'place', id: 'trend', layout: third }, + ]); + expect(result.current.error).toBeUndefined(); +}); + +test('a gesture queued behind a write is not undone by that write landing', async () => { + // The first write resolves; the follow-up is held open so the window + // between them is observable. Inside it the queued gesture must still show. + let releaseFollowUp: () => void = () => {}; + const followUp = new Promise<Response>(resolve => { + releaseFollowUp = () => + resolve( + new Response( + JSON.stringify({ + result: { + revision: 5, + ops: [], + placements: { kpi: MOVED, trend: QUEUED }, + }, + }), + { headers: { 'Content-Type': 'application/json' } }, + ), + ); + }); + fetchMock + .patch( + endpoint, + { + result: { + revision: 4, + ops: [], + placements: { kpi: MOVED, trend: TREND }, + }, + }, + { repeat: 1 }, + ) + .patch(endpoint, () => followUp); + + const { result } = renderHook(() => + useCanvasLayout(7, resultAt(3), jest.fn()), + ); + + act(() => { + result.current.place('kpi', MOVED); + // Queued: the first request is already in flight. + result.current.place('trend', QUEUED); + }); + + // Both requests have gone out, so the first one's response has landed. + await waitFor(() => expect(fetchMock.callHistory.calls()).toHaveLength(2)); + + // The regression: the first response's placements have trend at TREND, and + // overwriting with them would snap the widget back mid-flight. + expect(result.current.placements.trend).toEqual(QUEUED); + expect(result.current.placements.kpi).toEqual(MOVED); + + releaseFollowUp(); + await waitFor(() => expect(result.current.placements.trend).toEqual(QUEUED)); Review Comment: This final assertion passes even if the follow-up response is ignored: `trend` is already `QUEUED` from the optimistic overlay, and the follow-up mock returns the same placement. A regression that never consumes the follow-up acknowledgement (leaving `confirmed` stale so later writes conflict) would go unnoticed. Could this await the revision advancing to 5 and have the follow-up return a server-adjusted placement for `trend` to prove it was consumed? -- 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]
