sadpandajoe commented on code in PR #45090: URL: https://github.com/apache/superset/pull/45090#discussion_r4404742995
########## superset-frontend/src/features/canvas/useCanvasLayout.ts: ########## @@ -0,0 +1,421 @@ +/** + * 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 { DRAFT_TOKEN_HEADER } from './useCanvasDefinition'; +import type { + ApplyOperationsResult, + CanvasDefinitionResult, + GridLayout, + GridPlacement, + SpanConstraints, +} from './types'; + +/** + * A pending write for one node. + * + * A drag within a grid replaces the node's layout; a drag into another grid + * reparents it as well, which the server takes as a different operation. + * Either way the node ends up somewhere, so both queue and coalesce the same. + */ +export type LayoutWrite = + | { kind: 'place'; layout: GridPlacement } + | { kind: 'move'; parent: string; index: number; layout: GridPlacement }; + +const asOperation = (id: string, write: LayoutWrite) => + write.kind === 'place' + ? { op: 'place', id, layout: write.layout } + : { + op: 'move', + id, + parent: write.parent, + index: write.index, + layout: write.layout, + }; + +/** The newest layout this client knows of, and the revision it belongs to. */ +interface AppliedLayout { + revision: number; + placements: Record<string, GridPlacement>; + /** The stored layouts behind `placements`; see `CanvasLayout.layouts`. */ + layouts: Record<string, GridLayout>; + /** Reading order per grid; see `CanvasLayout.childOrder`. */ + childOrder: Record<string, string[]>; + /** Span limits per node; see `CanvasLayout.layoutConstraints`. */ + layoutConstraints: Record<string, SpanConstraints>; +} + +export interface CanvasLayout { + /** Placements to render: the newest local ones, else the server's. */ + placements: Record<string, GridPlacement>; + /** + * The stored layouts behind those placements, which is what the server + * resolves from. Kept in step with every write rather than read from the + * fetched definition: a write can change a node's stored layout without + * being asked to, since a collided sibling is pushed down and its new `row` + * written back. A drag preview resolving from the fetched layouts would + * then disagree with the server until the next refetch. + */ + layouts: Record<string, GridLayout>; + /** + * Each grid's children in reading order, kept in step with every write for + * the same reason as `layouts`: a `place` reorders the parent's children so + * the dropped widget wins the spot it landed on. A preview resolving against + * the fetched order would put the neighbours somewhere the server does not, + * and they would jump on release. + */ + childOrder: Record<string, string[]>; + /** + * Each node's span limits, kept in step with every write for the same + * reason as `layouts` and `childOrder`. A container's row minimum comes + * from the rows its children occupy, so it moves whenever they do: a child + * grown from four rows to eight raises its container's minimum, and a + * client still clamping to four would send a span the server replaces. + */ + layoutConstraints: Record<string, SpanConstraints>; + /** + * The newest revision the server is known to hold — the one the last write + * returned, else the one the definition was fetched at. Never an optimistic + * guess, so it is safe to send back as the revision a request is based on + * (a draft commit, say, which requires an exact match). + */ + revision: number; + /** + * Whether a layout write is in flight or queued, so `revision` is about to + * move. A caller that sends `revision` to the server — publishing a draft — + * must wait for this to clear, or it names a revision that is already stale + * and loses the writes that had not landed. + */ + saving: boolean; + /** Persist a node's new placement. A no-op when the user can't edit. */ + place: (nodeId: string, placement: GridPlacement) => void; + /** + * Exchange two widgets' places, in one request. A drop landing squarely on + * one widget takes its slot and gives up its own. + */ + swap: ( + nodeId: string, + placement: GridPlacement, + partnerId: string, + partnerPlacement: GridPlacement, + ) => void; + /** + * Persist a node moving into another container, at `index` in its reading + * order and `placement` in its grid. A no-op when the user can't edit. + */ + move: ( + nodeId: string, + parent: string, + index: number, + 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, + draftToken?: string, +): CanvasLayout { + const [applied, setApplied] = useState<AppliedLayout>(); + const [error, setError] = useState<string>(); + // Mirrors `writing`/`queued` as state, since callers render against it. + const [saving, setSaving] = useState(false); + 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, LayoutWrite>()); + // The definition's placements, for an optimistic view that has no newer + // local layout to build on. + const fetchedPlacements = useRef(result.placements); + const fetchedLayouts = useRef(result.layouts); + const fetchedOrder = useRef(result.childOrder); + const fetchedConstraints = useRef(result.layoutConstraints); + // The last layout the server acknowledged, without any optimistic overlay. + // A failed write falls back to this rather than to the fetched definition, + // which may be several acknowledged drags behind. + const acknowledged = useRef<AppliedLayout>(); + + useEffect(() => { + fetched.current = revision; + fetchedPlacements.current = result.placements; + fetchedLayouts.current = result.layouts; + fetchedOrder.current = result.childOrder; + fetchedConstraints.current = result.layoutConstraints; + }, [ + revision, + result.placements, + result.layouts, + result.childOrder, + result.layoutConstraints, + ]); + + // Which surface this state belongs to. A request already in flight when the + // surface changes still returns, and its response has to be dropped rather + // than written over the new surface's. + const generation = useRef(0); + + useEffect(() => { + // A different canvas, or switching between a draft and the published + // canvas, shares none of this state -- a draft counts its own revisions + // from zero, so a revision confirmed on one surface means nothing on the + // other. + generation.current += 1; + confirmed.current = undefined; + acknowledged.current = undefined; + writing.current = false; + queued.current.clear(); + setApplied(undefined); + setError(undefined); + setSaving(false); + }, [canvasId, draftToken]); + + const send = useCallback( + (ops: Map<string, LayoutWrite>) => { + writing.current = true; + setSaving(true); + // The surface this write belongs to, as of now. Everything below runs + // after the response, by which time the user may have published or + // discarded the plan, or navigated to another canvas. + const era = generation.current; + const base = Math.max(fetched.current, confirmed.current ?? 0); + const placeOps = [...ops].map(([id, write]) => asOperation(id, write)); + // While a draft is open the layout belongs to the draft, not the + // published canvas: same operations, but its own route, its own + // revision field, and the token that authorizes it. + const target = draftToken + ? { + endpoint: '/api/v1/canvas/draft', + headers: { [DRAFT_TOKEN_HEADER]: draftToken }, + jsonPayload: { revision: base, ops: placeOps }, + } + : { + endpoint: `/api/v1/canvas/${canvasId}/definition`, + jsonPayload: { base_revision: base, ops: placeOps }, + }; + SupersetClient.request({ ...target, method: 'PATCH' }) + .then(({ json }) => { + // A response that outlived its surface. Letting it through would put + // the old surface's revision in `confirmed`, and the new surface's + // next write would be based on a revision it never had -- a draft + // counts from zero, so that is a 409 or the wrong layout. + if (era !== generation.current) return; + const written = json?.result as ApplyOperationsResult | undefined; + if (!written) return; + confirmed.current = written.revision; + acknowledged.current = { + revision: written.revision, + placements: written.placements, + layouts: written.layouts, + childOrder: written.childOrder, + layoutConstraints: written.layoutConstraints, + }; + // Gestures queued while this write was in flight are newer than the + // placements it resolved, so they stay on top -- otherwise the + // widget would snap back until the follow-up request returned. The + // view is tagged a revision ahead, which is what the follow-up will + // produce, so a refetch of this revision doesn't drop them either. + const waiting = [...queued.current].map( + ([id, write]) => [id, write.layout] as const, + ); + setApplied({ + revision: written.revision + (waiting.length > 0 ? 1 : 0), + placements: + waiting.length > 0 + ? { ...written.placements, ...Object.fromEntries(waiting) } + : written.placements, + layouts: + waiting.length > 0 + ? { ...written.layouts, ...Object.fromEntries(waiting) } + : written.layouts, + // Not merged with the queued gestures: how they affect reading + // order is the server's to decide, and the follow-up returns it. + childOrder: written.childOrder, Review Comment: A successful write keeps `childOrder` and `layoutConstraints` from the PATCH result but drops the filter, cross-filter and customization scopes it returns alongside them (`**result.scopes` in the API). After a drag that reparents a chart from group A into group B, the chart renders in B while `CanvasGrid` still resolves its scopes from the fetched definition, so it keeps querying with A's filter until the next refetch, and with `CANVAS_DRAFT_POLL_INTERVAL=0` there is none. Should the acknowledged layout carry the returned scope maps too, so the tree and its scopes advance together? ########## superset-frontend/src/features/canvas/CanvasGrid.test.tsx: ########## @@ -190,3 +232,400 @@ test('setting a cross-filter does nothing while cross-filters are off', () => { expect(onValueChange).not.toHaveBeenCalled(); }); + +test('a canvas that is not editable gets no drag or resize handles', () => { + renderGrid(); + + expect( + screen.queryByRole('button', { name: 'Move widget' }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole('button', { name: /^Resize widget from/ }), + ).not.toBeInTheDocument(); +}); + +test('an editable canvas gets a handle per placed widget', () => { + renderGrid(emptyScopeValues(), jest.fn(), result, jest.fn(), true); + + // Every node with a resolved placement, on the root grid and inside the + // nested group, is draggable and resizable. + expect(screen.getAllByRole('button', { name: 'Move widget' })).toHaveLength( + Object.keys(result.placements).length, + ); + // Four corners each. + expect( + screen.getAllByRole('button', { name: /^Resize widget from/ }), + ).toHaveLength(Object.keys(result.placements).length * 4); +}); + +/** + * jsdom carries none of the properties a gesture needs on its own + * `PointerEvent`, so these are built on `MouseEvent` with the pointer id added + * -- the same shape `CanvasGridSurface.test.tsx` uses. + */ +class TestPointerEvent extends MouseEvent { + pointerId: number; + + constructor(type: string, init: MouseEventInit = {}) { + super(type, { bubbles: true, cancelable: true, ...init }); + this.pointerId = 1; + } +} + +const dragRight = (handle: HTMLElement, dx: number) => + [ + new TestPointerEvent('pointerdown', { button: 0, clientX: 0, clientY: 0 }), + new TestPointerEvent('pointermove', { clientX: dx, clientY: 0 }), + new TestPointerEvent('pointerup', { clientX: dx, clientY: 0 }), + ].forEach(event => fireEvent(handle, event)); + +beforeAll(() => { + // jsdom lays nothing out, so every grid is told it is 1000px wide. + Object.defineProperty(HTMLElement.prototype, 'clientWidth', { + configurable: true, + get: () => 1000, + }); + // jsdom has no pointer capture; the gesture only needs it not to throw. + HTMLElement.prototype.setPointerCapture = jest.fn(); + HTMLElement.prototype.releasePointerCapture = jest.fn(); +}); + +test('a nested grid resizes against its own column count, not the root', () => { + // 'c1' sits in group 'g', which declares 12 columns against the root's 24. + // Handing the root count to the nested surface would let 'c1' grow to 24, + // and would halve the column pitch so the same drag reported twice the + // movement. + const place = jest.fn(); + renderGrid(emptyScopeValues(), jest.fn(), result, place, true); + + const group = screen.getByRole('region', { name: 'group' }); + dragRight(group.querySelector('[data-corner="se"]') as HTMLElement, 2000); + + expect(place).toHaveBeenCalledWith('c1', { + col: 1, + row: 1, + colSpan: 12, + rowSpan: 1, + }); +}); + +test("a nested resize honours the widget's declared minimum span", () => { + // 'c1' declares minColSpan 2 and starts at 6 of the group's 12 columns. + // Dragging its corner well past the left edge must stop at 2, not 1: this + // is what fails if `layoutConstraints` never reaches the nested surface. + const place = jest.fn(); + renderGrid(emptyScopeValues(), jest.fn(), result, place, true); + + const group = screen.getByRole('region', { name: 'group' }); + // The bottom-right corner, dragged left: the span shrinks while the origin + // stays put, so it is the minimum that stops it. + dragRight(group.querySelector('[data-corner="se"]') as HTMLElement, -2000); + + expect(place).toHaveBeenCalledWith('c1', { + col: 1, + row: 1, + colSpan: 2, + rowSpan: 1, + }); +}); + +test('a press inside a group drags the widget, not the group', () => { + // A deferred body press does not stop propagation -- a click has to reach + // the widget -- so it bubbles to the nested grid and to the root one. Both + // would start a gesture, the outer one would win the pointer, and the + // group would move instead of the chart. + const place = jest.fn(); + renderGrid(emptyScopeValues(), jest.fn(), result, place, true); + + const group = screen.getByRole('region', { name: 'group' }); + const inner = group.querySelector('.canvas-interactive') as HTMLElement; + + // Downwards: the group is full width, so a sideways drag cannot move it and + // would never commit, hiding the very thing this guards. + [ + new TestPointerEvent('pointerdown', { button: 0, clientX: 0, clientY: 0 }), + new TestPointerEvent('pointermove', { clientX: 0, clientY: 112 }), + new TestPointerEvent('pointerup', { clientX: 0, clientY: 112 }), + ].forEach(event => fireEvent(inner, event)); + + expect(place).toHaveBeenCalledTimes(1); + expect(place.mock.calls[0][0]).toBe('c1'); +}); + +test("the acknowledged reading order wins over the definition's", () => { + // A `place` reorders the parent's children server-side, and the fetched + // definition keeps the old order until the next poll. Reading it from the + // definition would restore the stale order right after a drop. + render( + <CanvasGrid + canvasId={7} + result={result} + values={emptyScopeValues()} + onValueChange={jest.fn()} + layout={layoutWith(result, { + // The definition's root order starts 'f', 'g'; this swaps them. + childOrder: { ...result.childOrder, root: ['g', 'f', 'lost'] }, + })} + editable={false} + />, + ); + + // Direct children of the root grid only: a nested grid's own children sit + // inside their container and would interleave with them. + const rendered = Array.from( + document.querySelectorAll( + '[data-test="canvas-grid"] > [data-test="canvas-grid-surface"] > [data-node-id]', + ), + ).map(node => node.getAttribute('data-node-id')); + + expect(rendered).toEqual(['g', 'f', 'lost']); +}); + +test('the acknowledged span limits are what a resize clamps to', () => { + // A container's row minimum follows the rows its children occupy, so a + // write moves it. Clamping to the definition's limits would let the user + // shrink a container past a minimum the server has already raised. + const place = jest.fn(); + render( + <CanvasGrid + canvasId={7} + result={result} + values={emptyScopeValues()} + onValueChange={jest.fn()} + layout={layoutWith(result, { + place, + // 'c1' spans 6 of the group's 12 columns; this forbids going below 5. + layoutConstraints: { c1: { minColSpan: 5 } }, + })} + editable + />, + ); + + const group = screen.getByRole('region', { name: 'group' }); + dragRight(group.querySelector('[data-corner="se"]') as HTMLElement, -2000); + + expect(place).toHaveBeenCalledWith('c1', { + col: 1, + row: 1, + colSpan: 5, + rowSpan: 1, + }); +}); + +test('a widget dragged out of a group moves into the grid it was dropped on', () => { + // A drag only ever sent `place`, which changes a widget's layout inside the + // container it already lives in -- so dropping a widget below its group + // just grew the group. The drop now resolves which grid is under the + // pointer and, when that is a different one, reparents the widget instead. + const place = jest.fn(); + const move = jest.fn(); + const { container } = render( + <CanvasGrid + canvasId={7} + result={result} + values={emptyScopeValues()} + onValueChange={jest.fn()} + layout={layoutWith(result, { place, move })} + editable + />, + ); + + // Two grids: the root, and the one inside the group. + const grids = container.querySelectorAll('[data-test="canvas-grid-surface"]'); + expect(grids).toHaveLength(2); + const [root, inner] = Array.from(grids) as HTMLElement[]; + + // jsdom lays nothing out, so say where each grid is. The root fills the + // page; the group's grid is the band across its middle. + const box = (el: HTMLElement, top: number, bottom: number) => + Object.defineProperty(el, 'getBoundingClientRect', { + configurable: true, + value: () => ({ + left: 0, + top, + right: 1000, + bottom, + width: 1000, + height: bottom - top, + x: 0, + y: top, + toJSON: () => ({}), + }), + }); + box(root, 0, 800); + box(inner, 200, 400); + + // 'c1' lives in the group. Drag it out, down into the root grid. + const group = screen.getByRole('region', { name: 'group' }); + const handle = group.querySelector( + '[data-test="canvas-drag-handle"]', + ) as HTMLElement; + + fireEvent( + handle, + new TestPointerEvent('pointerdown', { + button: 0, + clientX: 100, + clientY: 300, + }), + ); + // Well below the group's band, so the root grid is under the pointer. + fireEvent( + handle, + new TestPointerEvent('pointermove', { clientX: 100, clientY: 700 }), + ); + fireEvent( + handle, + new TestPointerEvent('pointerup', { clientX: 100, clientY: 700 }), + ); + + // Reparented, not merely re-laid-out. + expect(place).not.toHaveBeenCalled(); + expect(move).toHaveBeenCalledTimes(1); + const [nodeId, parent, index, placement] = move.mock.calls[0]; + expect(nodeId).toBe('c1'); + expect(parent).toBe('root'); + expect(typeof index).toBe('number'); + // Landed in the root's 24 columns, not the group's 12. + expect(placement.colSpan).toBeLessThanOrEqual(24); Review Comment: The cross-grid assertions are loose enough to pass with the wrong wiring: `typeof index` is `'number'` and `colSpan <= 24`, so passing the destination's columns where the source's are expected (keeping the 12-column span) or always inserting at index 0 would still be green. For the 12-column group to 24-column root drag at `clientY: 700`, can this assert the exact `placement` and `index`? -- 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]
