sadpandajoe commented on code in PR #45090: URL: https://github.com/apache/superset/pull/45090#discussion_r4377751392
########## superset-frontend/src/features/canvas/useCanvasLayout.ts: ########## @@ -0,0 +1,322 @@ +/** + * 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, +} from './types'; + +/** 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[]>; +} + +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[]>; + /** + * 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; + /** 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, GridPlacement>()); + // 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); + // 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; + }, [revision, result.placements, result.layouts, result.childOrder]); + + // 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, GridPlacement>) => { + 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, layout]) => ({ + op: 'place', + id, + layout, + })); + // 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, Review Comment: A successful write keeps `placements`, `layouts` and `childOrder` from the PATCH result, but the result's `layoutConstraints` is dropped, so `CanvasGrid` keeps feeding `constraints={layoutConstraints}` from the last fetched definition. Say a container's child is grown from 4 to 8 rows and the user then shrinks the container before the next poll: the UI still allows the old 4-row minimum, and the server restores the 8-row span at the new row, so the bottom edge jumps and siblings are pushed. Should the acknowledged and applied state carry `layoutConstraints` too, with the grid reading them from there? ########## superset/canvas/api.py: ########## @@ -558,17 +576,33 @@ def get_definition(self, id_or_uuid: str) -> Response: description: >- Resolved grid position of every node on a grid, auto-placed nodes included + layouts: + type: object + description: >- + The stored layout of every node on a grid, which + placement is resolved from, for a client that + previews a drag before writing it widgetTypes: type: object description: The widget type of each resolvable node gridColumns: type: object description: The column count of each grid container + layoutConstraints: Review Comment: `render_context` now also returns `childOrder`, and `CanvasDefinitionResult` requires it in TypeScript, but this response docstring (and so `openapi.json`) documents `layoutConstraints` and not `childOrder`. A client generated from the spec can't type `result.childOrder`, which the drag preview depends on. Could it be added here and the spec regenerated? ########## superset-frontend/src/pages/Canvas/Canvas.test.tsx: ########## @@ -61,8 +61,11 @@ const definition = (children: string[], revision = 1) => ({ crossFilterScopes: {}, customizationScopes: {}, placements: {}, + layouts: {}, + childOrder: { root: children }, Review Comment: Every definition fixture here omits `canEdit`, so no page test ever renders the Plan mode button. If `onStarted={draft.open}` were dropped from `index.tsx`, clicking Plan mode would start a draft that the page never opens, and these tests would still pass. Could one test return `canEdit: true`, click Plan mode with a mocked token response, and assert the draft request carries that token and the draft banner appears? ########## superset-frontend/src/features/canvas/CanvasGrid.test.tsx: ########## @@ -110,10 +126,25 @@ const registrations = [ canvas.registerWidgetRenderer('test.odd', CrossSource), ]; +const layoutOf = ( + gridResult: CanvasDefinitionResult, + place = jest.fn(), +): CanvasLayout => ({ + placements: gridResult.placements, + layouts: gridResult.layouts, + childOrder: gridResult.childOrder, Review Comment: `layout.childOrder` is always the same as the definition's child order in these fixtures, so the `childOrder?.[nodeId] ?? children` read in `CanvasGrid` is never exercised. Replacing it with plain `children` would restore the stale reading order after a drop and still pass. Could a case render a definition ordered `[a, b]` with an acknowledged `childOrder.root` of `[b, a]` and assert the DOM order is `b` then `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]
