From 9f0ffcbb1b8ac4a787a23ae362264a20b12ce6ac Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Tue, 20 Oct 2026 11:40:17 +0530 Subject: [PATCH] fix: keep `if` conditions when moving a canvas item across containers Dragging a canvas item into another tab, out of a tab group, or into the top-level rows goes through `moveItemAcrossContainers`, which spliced the raw YAML item into the destination without reading the `if` conditions of the source tab group, tab or row. The moved item lost the visibility it inherited and became visible to every viewer; in embeds and public URLs restricted to the canvas, its metrics view also became queryable. Fold the source container's conditions that the destination doesn't already apply into the item's own `if`, mirroring what the same-container move and `deleteTab` already do. --- .../features/canvas/stores/tab-edit.spec.ts | 120 ++++++++++++++++++ .../src/features/canvas/stores/tab-edit.ts | 39 ++++++ 2 files changed, 159 insertions(+) diff --git a/web-common/src/features/canvas/stores/tab-edit.spec.ts b/web-common/src/features/canvas/stores/tab-edit.spec.ts index f31b02c209f..efbe00f2a13 100644 --- a/web-common/src/features/canvas/stores/tab-edit.spec.ts +++ b/web-common/src/features/canvas/stores/tab-edit.spec.ts @@ -470,6 +470,126 @@ rows: ]); }); + // A tab group whose Finance tab, one of its rows and one of its items all carry an `if`. + const CONDITIONED_TABS = `type: canvas +rows: + - items: + - component: outside + - if: '{{ has "staff" .user.groups }}' + tabs: + - label: Overview + rows: + - items: + - component: kpi_overview + - label: Finance + if: '{{ has "finance" .user.groups }}' + rows: + - if: "'{{ .user.domain }}' = 'acme.com'" + items: + - component: kpi_finance + if: '{{ .user.admin }}' +`; + + it("moveItemAcrossContainers keeps the visibility of an item moved to another tab", () => { + const doc = parseDocument(CONDITIONED_TABS); + // Finance's row 0 col 0 joins Overview's row 0, to the right of kpi_overview. + const ok = moveItemAcrossContainers( + doc, + { rowsPath: ["rows", 1, "tabs", 1, "rows"], row: 0, col: 0 }, + { rowsPath: ["rows", 1, "tabs", 0, "rows"], row: 0, col: 1 }, + ); + expect(ok).toBe(true); + + const tabs = doc.toJSON().rows[1].tabs; + expect(tabs[1].rows).toEqual([]); + // The group's condition still applies in Overview, so only the tab's, the row's and + // the item's own conditions travel with the item. + expect(tabs[0].rows[0].items).toEqual([ + { component: "kpi_overview" }, + { + component: "kpi_finance", + if: `({{ has "finance" .user.groups }}) AND ('{{ .user.domain }}' = 'acme.com') AND ({{ .user.admin }})`, + }, + ]); + }); + + it("moveItemAcrossContainers keeps the visibility of an item moved out of a tab group", () => { + const doc = parseDocument(CONDITIONED_TABS); + // Finance's item becomes a new top-level row after the tab group. + const ok = moveItemAcrossContainers( + doc, + { rowsPath: ["rows", 1, "tabs", 1, "rows"], row: 0, col: 0 }, + { rowsPath: ["rows"], col: null }, + ); + expect(ok).toBe(true); + + const json = doc.toJSON(); + expect(json.rows).toHaveLength(3); + // Nothing applies to a top-level row, so the group's condition travels too. + expect(json.rows[2]).toEqual({ + items: [ + { + component: "kpi_finance", + if: `({{ has "staff" .user.groups }}) AND ({{ has "finance" .user.groups }}) AND ('{{ .user.domain }}' = 'acme.com') AND ({{ .user.admin }})`, + }, + ], + }); + }); + + it("moveItemAcrossContainers lets an item moved into a tab inherit the tab's visibility", () => { + const doc = parseDocument(CONDITIONED_TABS); + // The unconditioned top-level item joins Finance's conditioned row. + const ok = moveItemAcrossContainers( + doc, + { rowsPath: ["rows"], row: 0, col: 0 }, + { rowsPath: ["rows", 1, "tabs", 1, "rows"], row: 0, col: 0 }, + ); + expect(ok).toBe(true); + + const json = doc.toJSON(); + // The source row is gone, so the group is now the only top-level row. + expect(json.rows).toHaveLength(1); + const finance = json.rows[0].tabs[1]; + expect(finance.if).toBe('{{ has "finance" .user.groups }}'); + expect(finance.rows[0].if).toBe("'{{ .user.domain }}' = 'acme.com'"); + // The item had no conditions of its own and gains none; the destination's apply to it. + expect(finance.rows[0].items[0]).toEqual({ component: "outside" }); + }); + + it("moveItemAcrossContainers doesn't repeat a condition the destination already applies", () => { + const doc = parseDocument(`type: canvas +rows: + - tabs: + - label: A + if: '{{ .user.admin }}' + rows: + - if: "'{{ .user.domain }}' = 'acme.com'" + items: + - component: a + - if: '{{ has "finance" .user.groups }}' + tabs: + - label: B + rows: + - if: "'{{ .user.domain }}' = 'acme.com'" + items: + - component: b +`); + // Tab A's item joins tab B's row, which has the same row condition. + const ok = moveItemAcrossContainers( + doc, + { rowsPath: ["rows", 0, "tabs", 0, "rows"], row: 0, col: 0 }, + { rowsPath: ["rows", 1, "tabs", 0, "rows"], row: 0, col: 1 }, + ); + expect(ok).toBe(true); + + const json = doc.toJSON(); + expect(json.rows[0].tabs[0].rows).toEqual([]); + expect(json.rows[1].tabs[0].rows[0].items).toEqual([ + { component: "b" }, + { component: "a", if: "{{ .user.admin }}" }, + ]); + }); + it("tabHasContent reflects whether a tab has rows", () => { const doc = parseDocument(`type: canvas rows: diff --git a/web-common/src/features/canvas/stores/tab-edit.ts b/web-common/src/features/canvas/stores/tab-edit.ts index 72c55a91c04..84a2666c2f4 100644 --- a/web-common/src/features/canvas/stores/tab-edit.ts +++ b/web-common/src/features/canvas/stores/tab-edit.ts @@ -29,6 +29,10 @@ const MAX_ITEMS_PER_ROW = 4; * room, the item joins that row at that column (e.g. dropping to the left of a widget); * - otherwise it becomes a new row inserted at `dest.row` (or appended). * + * The item keeps the visibility it had at the source: the `if` conditions of the source + * group, tab and row that the destination doesn't apply too are folded into the item's own + * `if`, so content hidden from some viewers doesn't become visible to them (as in deleteTab). + * * Node references for both containers are resolved up front, so removing the source row * never invalidates the destination even when the source sits above a destination tab group. * Returns true if the move was applied. @@ -60,6 +64,25 @@ export function moveItemAcrossContainers( isSeq(destRowItems) && destRowItems.items.length < MAX_ITEMS_PER_ROW; + // Carry the source conditions the destination doesn't already apply. + const destConditions = new Set( + [ + ...containerConditions(doc, dest.rowsPath), + joinExistingRow && isMap(destRowNode) ? destRowNode.get("if") : undefined, + ] + .map((c) => combineConditions(c)) + .filter((c) => c !== undefined), + ); + const carried = [ + ...containerConditions(doc, source.rowsPath), + sourceRow.get("if"), + ] + .map((c) => combineConditions(c)) + .filter((c) => c !== undefined && !destConditions.has(c)); + if (carried.length > 0 && isMap(itemNode)) { + itemNode.set("if", combineConditions(...carried, itemNode.get("if"))); + } + // Remove from the source (and drop the row if it is now empty). sourceItems.items.splice(source.col, 1); if (sourceItems.items.length === 0) { @@ -78,6 +101,22 @@ export function moveItemAcrossContainers( return true; } +/** + * The `if` conditions that apply to every row of a rows container: those of the enclosing + * tab group and tab for a tab's rows (`["rows", group, "tabs", tab, "rows"]`), none for the + * top-level rows (`["rows"]`). + */ +function containerConditions( + doc: Document, + rowsPath: (string | number)[], +): unknown[] { + if (rowsPath.length < 5) return []; + return [ + doc.getIn([...rowsPath.slice(0, 2), "if"]), + doc.getIn([...rowsPath.slice(0, 4), "if"]), + ]; +} + /** Number of top-level entries in the rows sequence. */ function rowCount(doc: Document): number { const rows = doc.get("rows");