sadpandajoe commented on code in PR #45090: URL: https://github.com/apache/superset/pull/45090#discussion_r4231264678
########## superset-frontend/src/features/canvas/CanvasGridSurface.test.tsx: ########## @@ -0,0 +1,308 @@ +/** + * 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 { fireEvent, render, screen } from 'spec/helpers/testing-library'; +import CanvasGridSurface from './CanvasGridSurface'; +import type { GridPlacement, SpanConstraints } from './types'; + +// jsdom lays nothing out, so the grid is told how wide it is: 1000px over 24 +// columns with 16px gaps gives a 42.33px column pitch and a 56px row pitch. +const WIDTH = 1000; +const COL_PITCH = 632 / 24 + 16; +const ROW_PITCH = 56; + +const placements: Record<string, GridPlacement> = { + kpi: { col: 1, row: 1, colSpan: 6, rowSpan: 4 }, + trend: { col: 1, row: 5, colSpan: 24, rowSpan: 8 }, +}; + +const renderSurface = ({ + editable = true, + onPlace = jest.fn(), + constraints = {} as Record<string, SpanConstraints>, +} = {}) => { + render( + <CanvasGridSurface + childIds={Object.keys(placements)} + columns={24} + gap={16} + rowUnit={40} + placements={placements} + constraints={constraints} + editable={editable} + onPlace={onPlace} + renderNode={nodeId => <div>{`widget ${nodeId}`}</div>} + />, + ); + return onPlace; +}; + +/** + * jsdom's `PointerEvent` carries none of the properties a gesture needs, so + * the pointer events are built on `MouseEvent`, which does support + * coordinates and buttons, with the pointer id added on top. + */ +class TestPointerEvent extends MouseEvent { + pointerId: number; + + constructor( + type: string, + init: MouseEventInit & { pointerId?: number } = {}, + ) { + super(type, { bubbles: true, cancelable: true, ...init }); + this.pointerId = init.pointerId ?? 1; + } +} + +const pointer = ( + handle: HTMLElement, + type: 'pointerdown' | 'pointermove' | 'pointerup' | 'pointercancel', + init: MouseEventInit & { pointerId?: number } = {}, +) => fireEvent(handle, new TestPointerEvent(type, { pointerId: 1, ...init })); + +/** A whole gesture on `handle`, from grab to release. */ +const drag = (handle: HTMLElement, dx: number, dy: number) => { + pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 }); + pointer(handle, 'pointermove', { clientX: dx, clientY: dy }); + pointer(handle, 'pointerup', { clientX: dx, clientY: dy }); +}; + +const handles = (name: 'Move widget' | 'Resize widget') => + screen.getAllByRole('button', { name }); + +beforeAll(() => { + Object.defineProperty(HTMLElement.prototype, 'clientWidth', { + configurable: true, + get: () => WIDTH, + }); + // jsdom has no pointer capture; the gesture only needs it not to throw. + HTMLElement.prototype.setPointerCapture = jest.fn(); + HTMLElement.prototype.releasePointerCapture = jest.fn(); +}); + +test('dragging a widget persists the cells it was dropped on', () => { + const onPlace = renderSurface(); + + drag(handles('Move widget')[0], COL_PITCH * 4, ROW_PITCH * 2); + + expect(onPlace).toHaveBeenCalledWith('kpi', { + col: 5, + row: 3, + colSpan: 6, + rowSpan: 4, + }); +}); + +test('resizing a widget persists the new spans and leaves its origin alone', () => { + const onPlace = renderSurface(); + + drag(handles('Resize widget')[0], COL_PITCH * 3, ROW_PITCH); + + expect(onPlace).toHaveBeenCalledWith('kpi', { + col: 1, + row: 1, + colSpan: 9, + rowSpan: 5, + }); +}); + +test('a resize stops at the span limits the widget declares', () => { + const onPlace = renderSurface({ constraints: { kpi: { maxColSpan: 8 } } }); + + drag(handles('Resize widget')[0], COL_PITCH * 10, 0); + + expect(onPlace).toHaveBeenCalledWith('kpi', { + col: 1, + row: 1, + colSpan: 8, + rowSpan: 4, + }); +}); + +test('a gesture too small to reach the next cell changes nothing', () => { + const onPlace = renderSurface(); + + drag(handles('Move widget')[0], 8, 10); + + expect(onPlace).not.toHaveBeenCalled(); +}); + +test('a cancelled gesture is not persisted', () => { + const onPlace = renderSurface(); + const handle = handles('Move widget')[0]; + + pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 }); + pointer(handle, 'pointermove', { clientX: COL_PITCH * 4, clientY: 0 }); + pointer(handle, 'pointercancel'); + + expect(onPlace).not.toHaveBeenCalled(); Review Comment: This test passes even if the `pointercancel` handler is deleted: persistence only happens on `pointerup`, which is never fired here, so `onPlace` stays uncalled either way. A regression that leaves the drag state active after a cancel (stale drop preview, widget stuck offset) would go unnoticed. Could it assert that `canvas-drop-preview` is shown after the move and gone after `pointercancel`? ########## superset-frontend/src/features/canvas/useCanvasLayout.ts: ########## @@ -0,0 +1,195 @@ +/** + * 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); Review Comment: A write queued while another is in flight is sent with `max(fetched, confirmed)` as its `base_revision`, not the revision the gesture was built against. If someone else resizes widget B (revision 11) after this user drags A and queues a drag of B at revision 10, the queued B write goes out as `base_revision: 12` carrying the stale span, and the server accepts it instead of returning a 409, so the other editor's resize is silently overwritten. Should a queued gesture keep the revision it was started from (or be dropped/rebased with a conflict notice when a refetch moved that widget in between)? -- 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]
