sadpandajoe commented on code in PR #45090:
URL: https://github.com/apache/superset/pull/45090#discussion_r4379746726
##########
superset/config.py:
##########
@@ -3583,7 +3586,8 @@ class ExtraAccessQueryExclusions(TypedDict, total=False):
# Resolve additional editor subjects for a resource. Also used for editorship
# checks and lockout-prevention logic.
EXTRA_EDITORS_RESOLVER: Callable[..., list[Any]] | None = None
-# Post-create hook for charts/dashboards. Receives (model, asset_type).
+# Post-create hook for charts/dashboards/canvases. Receives
Review Comment:
The admin docs section for this hook
(`docs/admin_docs/configuration/configuring-superset.mdx`, 'Reacting to chart
and dashboard creation') still describes only `"chart"` and `"dashboard"`, and
the hook runs inside the create transaction where an exception cancels the
creation. A deployment hook written to that documented contract (for example
one that reads `slice_name` or `dashboard_title` for anything that is not a
chart) now raises on every canvas create. Can the docs and `UPDATING.md`
mention the new `"canvas"` asset type?
##########
superset/canvas/definition/render.py:
##########
@@ -62,35 +77,81 @@ def render_context(
)
type_ids = {node_id: w.widget_type for node_id, w in node_types.items()}
- grids: list[tuple[list[str], int]] = [
- (definition["root"]["children"],
definition["root"]["layout"]["columns"])
+ grids: list[tuple[str, list[str], int]] = [
+ (
+ ROOT_ID,
+ definition["root"]["children"],
+ definition["root"]["layout"]["columns"],
+ )
]
grid_columns: dict[str, int] = {}
for node_id, widget in node_types.items():
columns = widget.behavior.grid_columns
if widget.behavior.container and columns is not None:
- grids.append((nodes[node_id].get("children") or [], columns))
+ grids.append((node_id, nodes[node_id].get("children") or [],
columns))
grid_columns[node_id] = columns
placements: dict[str, dict[str, int]] = {}
- for children, columns in grids:
+ stored_layouts: dict[str, dict[str, Any]] = {}
+ # Rows each grid container's children take up, for the row minimum below.
+ content_rows: dict[str, int] = {}
+ for parent, children, columns in grids:
layouts = [_grid_layout(nodes[c]["layout"]) for c in children]
rects, _ = resolve_grid(layouts, columns)
- for child, rect in zip(children, rects, strict=True):
+ for child, rect, layout in zip(children, rects, layouts, strict=True):
placements[child] = {
"col": rect.col,
"row": rect.row,
"colSpan": rect.col_span,
"rowSpan": rect.row_span,
}
+ stored_layouts[child] = layout
+ extent = max((r.row + r.row_span - 1 for r in rects), default=0)
+ chrome = node_types[parent].ui.chrome_rows if parent in node_types
else 0
+ content_rows[parent] = extent + chrome if extent else 0
Review Comment:
`content_rows` is only filled for grid containers here, but `validation.py`
also propagates it through non-grid containers (the `columns is None` branch
takes the tallest child's rows plus chrome) and `_fit` grows them to it. A
non-grid container on a grid whose child group is 8 rows tall therefore reports
no `minRowSpan`, so dragging its top edge down one row sends a span one short;
the server restores it and the bottom edge ends up a row lower, pushing the
widget below it down instead of the resize being clamped. Should the minimum be
computed bottom-up through non-grid containers the same way validation does?
##########
tests/unit_tests/canvas/render_test.py:
##########
@@ -59,3 +62,175 @@ def
test_placements_include_auto_placed_nodes_and_grid_containers() -> None:
"c": "chart",
}
assert context["gridColumns"] == {"g": 12}
+
+
+def test_layouts_carry_what_placement_is_resolved_from() -> None:
+ """
+ The stored layouts, which are not recoverable from ``placements``.
+
+ An auto-placed node has no stored ``col``/``row`` at all, and a node that
+ collided has its pushed-down ``row`` written back to its layout. A client
+ previewing a drag resolves from these, so it has to be told them: resolving
+ from the rectangles instead would make an auto-placed node look pinned, and
+ a pushed node look like it was still where the user last asked for it.
+ """
+ rules, resolver = canvas_widget_types(), FakeResolver()
+ definition = normalize_definition(
+ canvas(
+ {
+ # Auto-placed: no col/row of its own.
+ "auto": node("chart-1", layout={"colSpan": 8}),
+ # Explicit, and overlapping `auto` once it lands at row 1.
+ "fixed": node("chart-2", layout={"col": 1, "row": 1,
"colSpan": 8}),
+ },
+ children=["auto", "fixed"],
+ ),
+ rules,
+ resolver,
+ )
+
+ context = render_context(definition, rules, resolver)
+
+ # `fixed` asked for row 1 and was pushed to row 2 by `auto`.
+ assert context["placements"]["fixed"] == {
+ "col": 1,
+ "row": 2,
+ "colSpan": 8,
+ "rowSpan": 1,
+ }
+ assert context["layouts"] == {
+ "auto": {"colSpan": 8},
+ # The pushed row, not the row 1 that was asked for.
+ "fixed": {"col": 1, "row": 2, "colSpan": 8},
+ }
+
+
+def test_child_order_reports_each_grid_in_reading_order() -> None:
+ """
+ Reading order per grid, which the ``place`` operation rewrites when a drop
+ lands on an earlier sibling. A client previewing a drag resolves against
+ it, so a stale order makes the preview disagree with the write.
+ """
+ rules, resolver = canvas_widget_types(), FakeResolver()
+ definition = normalize_definition(
+ canvas(
+ {
+ "k1": node("chart-1", layout={"colSpan": 8}),
+ "g": node("group", layout={"colSpan": 24}, children=["c1",
"c2"]),
+ "c1": node("chart-2", layout={"colSpan": 6}),
+ "c2": node("chart-3", layout={"colSpan": 6}),
+ },
+ children=["k1", "g"],
+ ),
+ rules,
+ resolver,
+ )
+
+ context = render_context(definition, rules, resolver)
+
+ # The root under its own id, and every grid container under its node id.
+ assert context["childOrder"] == {
+ "root": ["k1", "g"],
+ "g": ["c1", "c2"],
+ }
+
+
+def test_layout_constraints_carry_declared_span_limits() -> None:
+ """Only the limits a widget actually declares reach the client."""
+ rules, resolver = canvas_widget_types(), FakeResolver()
+ rules["sized"] = type(
+ "SizedWidget",
+ (Widget,),
+ {
+ "widget_type": "sized",
+ "name": "Sized",
+ "controls_class": Props,
+ "behavior": WidgetBehavior(),
+ "ui": WidgetUi(default_size=(6, 4), min_col_span=4,
max_row_span=8),
+ },
+ )
+ definition = normalize_definition(
+ canvas(
+ {
+ "tile": node("sized-1", layout={"colSpan": 6}),
+ # The same widget, placed inside a grid container.
+ "g": node("group", layout={"colSpan": 24}, children=["inner"]),
+ "inner": node("sized-2", layout={"colSpan": 6}),
+ # A widget that declares nothing is left out entirely.
+ "plain": node("chart-1", layout={"colSpan": 4}),
+ },
+ children=["tile", "g", "plain"],
+ ),
+ rules,
+ resolver,
+ )
+
+ context = render_context(definition, rules, resolver)
+
+ assert context["layoutConstraints"] == {
+ "tile": {"minColSpan": 4, "maxRowSpan": 8},
+ "inner": {"minColSpan": 4, "maxRowSpan": 8},
+ }
+
+
+def test_a_container_reports_the_row_minimum_its_content_imposes() -> None:
+ """
+ Validation grows a container to fit its children and never shrinks it, so
+ the rows they occupy are a real minimum even where the widget declares
+ none. Without it a client dragging a container's top edge down computes
+ the shrink against 1, sends a span the server replaces, and the
+ container's bottom edge moves down into whatever sits below.
+ """
+ rules, resolver = canvas_widget_types(), FakeResolver()
+ definition = normalize_definition(
+ canvas(
+ {
+ "g": node("group", layout={"colSpan": 24}, children=["tall"]),
+ # Fills six rows of the group's own grid.
+ "tall": node("chart-1", layout={"colSpan": 6, "rowSpan": 6}),
+ },
+ children=["g"],
+ ),
+ rules,
+ resolver,
+ )
+
+ context = render_context(definition, rules, resolver)
+
+ # Validation already grew the group to fit, so this is also its span.
+ assert context["placements"]["g"]["rowSpan"] == 6
+ assert context["layoutConstraints"]["g"]["minRowSpan"] == 6
Review Comment:
The container-minimum test uses a group with no chrome rows, so
`content_rows[parent] = extent + chrome` could drop the `+ chrome` (or read
`chrome_rows` from the wrong node) and this assertion would still pass at 6. A
container with a non-zero `chrome_rows` is exactly where the server grows it to
content plus chrome, and the client would then clamp one row short. Could a
case use a container type with `chrome_rows=1` holding six content rows and
assert `minRowSpan == 7`?
##########
superset-frontend/src/features/canvas/useCanvasLayout.ts:
##########
@@ -0,0 +1,346 @@
+/**
+ * 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';
+
+/** 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;
+ /** 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);
+ 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, 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,
+ 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];
+ 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,
+ layoutConstraints: written.layoutConstraints,
+ });
+ })
+ .catch(async response => {
+ if (era !== generation.current) return;
+ queued.current.clear();
Review Comment:
`queued.current.clear()` on failure has no test: every rejection test in
`useCanvasLayout.test.ts` (the 409, 403, 422 and 'keeps the acknowledged
layout' cases) fails a write with an empty queue. Delete this line and they all
still pass, while a second drop made during the failing request would then be
sent by `finally` on top of a layout the user was just told was undone. Could a
test hold the first PATCH, queue a second `place`, reject the first, and assert
only one PATCH went out, the queued placement is gone from `placements`, and
`saving` is false?
--
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]
