diff --git a/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.spec.ts b/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.spec.ts index 75ccf300cf1..b624c7734ff 100644 --- a/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.spec.ts +++ b/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.spec.ts @@ -23,6 +23,7 @@ import { dimKeyFromDimValues, dimKeyFromRow, } from "../../../dashboards/pivot/pivot-click-selection"; +import { buildExpandKey } from "../../../dashboards/pivot/pivot-expand-keys"; import { createPivotClickToFilter } from "./pivot-click-to-filter"; import { ExpressionFilterManager } from "@rilldata/web-common/features/dashboards/filters/ExpressionFilterManager.svelte.ts"; import type { MetricsViewsProvider } from "@rilldata/web-common/features/metrics-views/providers/MetricsViewsProvider.svelte.ts"; @@ -64,6 +65,11 @@ function dk(dims: Record, order: string[]): string { return dimKeyFromDimValues(dims, order); } +/** TanStack row id as PivotTable's getRowId builds it: the values from the root to the row. */ +function rowId(...values: (string | null)[]): string { + return buildExpandKey(values); +} + /** A real filter manager over {@link PIVOT_METRICS_INIT}, torn down after the test. */ function createFilterManager() { const { value, destroy } = createInEffectRoot(() => { @@ -247,10 +253,15 @@ describe("flat table: single-cell-per-row", () => { it("replaces existing cell in the same row", () => { const { result } = setup(config, data); - result.handleCellClickToFilter("0", "country", false, data[0]); + result.handleCellClickToFilter( + rowId("US", "NYC"), + "country", + false, + data[0], + ); expect(sel(result).isCellSelected(dkRow0, "country")).toBe(true); - result.handleCellClickToFilter("0", "city", false, data[0]); + result.handleCellClickToFilter(rowId("US", "NYC"), "city", false, data[0]); expect(sel(result).isCellSelected(dkRow0, "country")).toBe(false); expect(sel(result).isCellSelected(dkRow0, "city")).toBe(true); expect(sel(result).cellSelections.size).toBe(1); @@ -261,8 +272,18 @@ describe("flat table: single-cell-per-row", () => { it("deselects by re-clicking the same cell", () => { const { result } = setup(config, data); - result.handleCellClickToFilter("0", "country", false, data[0]); - result.handleCellClickToFilter("0", "country", false, data[0]); + result.handleCellClickToFilter( + rowId("US", "NYC"), + "country", + false, + data[0], + ); + result.handleCellClickToFilter( + rowId("US", "NYC"), + "country", + false, + data[0], + ); expect(sel(result).cellSelections.size).toBe(0); result.destroy(); @@ -271,8 +292,18 @@ describe("flat table: single-cell-per-row", () => { it("allows selections across different rows", () => { const { result } = setup(config, data); - result.handleCellClickToFilter("0", "country", false, data[0]); - result.handleCellClickToFilter("1", "country", false, data[1]); + result.handleCellClickToFilter( + rowId("US", "NYC"), + "country", + false, + data[0], + ); + result.handleCellClickToFilter( + rowId("UK", "London"), + "country", + false, + data[1], + ); expect(sel(result).isCellSelected(dkRow0, "country")).toBe(true); expect(sel(result).isCellSelected(dkRow1, "country")).toBe(true); @@ -299,8 +330,13 @@ describe("nested table: multi-select", () => { it("allows multiple cells in the same row", () => { const { result } = setup(config, data); - result.handleCellClickToFilter("1", "revenue", false, data[0]); - result.handleCellClickToFilter("1", "other_measure", false, data[0]); + result.handleCellClickToFilter(rowId("US"), "revenue", false, data[0]); + result.handleCellClickToFilter( + rowId("US"), + "other_measure", + false, + data[0], + ); expect(sel(result).isCellSelected(dkRow0, "revenue")).toBe(true); expect(sel(result).isCellSelected(dkRow0, "other_measure")).toBe(true); @@ -341,7 +377,12 @@ describe("nested table: cross-parent selection isolation", () => { it("does NOT select X under B when clicking X under A", () => { const { result } = setup(config, data); - result.handleCellClickToFilter("1.0", "revenue", false, innerRowXUnderA); + result.handleCellClickToFilter( + rowId("A", "X"), + "revenue", + false, + innerRowXUnderA, + ); expect( sel(result).isCellSelected( @@ -363,7 +404,12 @@ describe("nested table: cross-parent selection isolation", () => { it("does NOT select row header X under B when clicking X under A", () => { const { result } = setup(config, data); - result.handleCellClickToFilter("1.0", "outer", true, innerRowXUnderA); + result.handleCellClickToFilter( + rowId("A", "X"), + "outer", + true, + innerRowXUnderA, + ); expect( sel(result).isRowHeaderSelected(dk({ outer: "A", inner: "X" }, dims)), @@ -391,7 +437,7 @@ describe("null dimension values", () => { it("selects a cell with null dimension value", () => { const { result, fm } = setup(config, data); - result.handleCellClickToFilter("0", "total", false, data[0]); + result.handleCellClickToFilter(rowId(null), "total", false, data[0]); expect(sel(result).isCellSelected(dkNull, "total")).toBe(true); expect(selectedValues(fm, "country")).toEqual([null]); @@ -401,8 +447,8 @@ describe("null dimension values", () => { it("deselects a cell with null dimension value", () => { const { result, fm } = setup(config, data); - result.handleCellClickToFilter("0", "total", false, data[0]); - result.handleCellClickToFilter("0", "total", false, data[0]); + result.handleCellClickToFilter(rowId(null), "total", false, data[0]); + result.handleCellClickToFilter(rowId(null), "total", false, data[0]); expect(sel(result).cellSelections.size).toBe(0); expect(selectedValues(fm, "country")).toEqual([]); @@ -443,7 +489,7 @@ describe("selection survives sorting", () => { ); const usDk = dimKeyFromRow(dataBefore[0], ["country"]); - result.handleCellClickToFilter("0", "total", false, dataBefore[0]); + result.handleCellClickToFilter(rowId("US"), "total", false, dataBefore[0]); expect(sel(result).isCellSelected(usDk, "total")).toBe(true); // Simulate sort: UK now first @@ -630,15 +676,15 @@ describe("deselect retains shared column filters", () => { const { result, fm } = setup(config, data, columnDimensionAxes); - result.handleCellClickToFilter("1", colId, false, data[0]); - result.handleCellClickToFilter("2", colId, false, data[1]); + result.handleCellClickToFilter(rowId("New York"), colId, false, data[0]); + result.handleCellClickToFilter(rowId("Bronx"), colId, false, data[1]); const dkNY = dimKeyFromRow(data[0], ["borough"]); const dkBronx = dimKeyFromRow(data[1], ["borough"]); expect(sel(result).isCellSelected(dkNY, colId)).toBe(true); expect(sel(result).isCellSelected(dkBronx, colId)).toBe(true); - result.handleCellClickToFilter("2", colId, false, data[1]); + result.handleCellClickToFilter(rowId("Bronx"), colId, false, data[1]); expect(sel(result).isCellSelected(dkBronx, colId)).toBe(false); expect(sel(result).isCellSelected(dkNY, colId)).toBe(true); @@ -679,7 +725,7 @@ describe("unrelated global filters are left alone", () => { it("does not re-add global filter values when selecting a cell", () => { const { result, fm } = setup(flatConfig, flatData, {}, globalFilter); - result.handleCellClickToFilter("0", "revenue", false, flatData[0]); + result.handleCellClickToFilter(rowId("US"), "revenue", false, flatData[0]); expect(selectedValues(fm, "country")).toEqual(["US"]); expectGlobalFilterIntact(fm); @@ -695,7 +741,7 @@ describe("unrelated global filters are left alone", () => { globalFilter, ); - result.handleCellClickToFilter("0", "revenue", false, flatData[0]); + result.handleCellClickToFilter(rowId("US"), "revenue", false, flatData[0]); expect([...get(selfFilteredDimensions)]).toEqual(["country"]); @@ -705,8 +751,8 @@ describe("unrelated global filters are left alone", () => { it("keeps global filters when deselecting a cell", () => { const { result, fm } = setup(flatConfig, flatData, {}, globalFilter); - result.handleCellClickToFilter("0", "revenue", false, flatData[0]); - result.handleCellClickToFilter("0", "revenue", false, flatData[0]); + result.handleCellClickToFilter(rowId("US"), "revenue", false, flatData[0]); + result.handleCellClickToFilter(rowId("US"), "revenue", false, flatData[0]); expect(sel(result).cellSelections.size).toBe(0); expect(selectedValues(fm, "country")).toEqual([]); @@ -725,11 +771,11 @@ describe("unrelated global filters are left alone", () => { const { result, fm } = setup(nestedConfig, nestedData, {}, globalFilter); - result.handleCellClickToFilter("1", "country", true, nestedData[0]); + result.handleCellClickToFilter(rowId("US"), "country", true, nestedData[0]); expect(sel(result).rowHeaderSelections.size).toBe(1); expect(selectedValues(fm, "country")).toEqual(["US"]); - result.handleCellClickToFilter("1", "country", true, nestedData[0]); + result.handleCellClickToFilter(rowId("US"), "country", true, nestedData[0]); expect(sel(result).rowHeaderSelections.size).toBe(0); expect(selectedValues(fm, "country")).toEqual([]); @@ -797,14 +843,14 @@ describe("unrelated global filters are left alone", () => { const { result, fm } = setup(nestedConfig, nestedData, {}, globalFilter); result.handleCellClickToFilter( - "1.0", + rowId("Zoom", "US-East"), "revenue", false, nestedData[0].subRows![0], ); expect(selectedValues(fm, "inner")).toEqual(["US-East"]); - result.handleCellClickToFilter("1", "outer", true, nestedData[0]); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, nestedData[0]); // The evicted child cell's inner value is dropped, the clicked row header's // outer value takes its place, and the global filter is untouched. @@ -847,11 +893,16 @@ describe("header/cell mutual exclusivity", () => { const dkChild = dk({ outer: "Zoom", inner: "US-East" }, dims); const dkZoom = dk({ outer: "Zoom" }, dims); - result.handleCellClickToFilter("1.0", "revenue", false, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "revenue", + false, + childUSEast, + ); expect(sel(result).isCellSelected(dkChild, "revenue")).toBe(true); expect(selectedValues(fm, "inner")).toEqual(["US-East"]); - result.handleCellClickToFilter("1", "outer", true, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, parentZoom); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(true); expect(sel(result).isCellSelected(dkChild, "revenue")).toBe(false); @@ -865,10 +916,15 @@ describe("header/cell mutual exclusivity", () => { const dkZoom = dk({ outer: "Zoom" }, dims); const dkChild = dk({ outer: "Zoom", inner: "US-East" }, dims); - result.handleCellClickToFilter("1", "outer", true, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, parentZoom); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(true); - result.handleCellClickToFilter("1.0", "revenue", false, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "revenue", + false, + childUSEast, + ); expect(sel(result).isCellSelected(dkChild, "revenue")).toBe(true); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(false); @@ -879,8 +935,13 @@ describe("header/cell mutual exclusivity", () => { it("different lineage coexists: header + cell under different parent", () => { const { result } = setupNested(); - result.handleCellClickToFilter("1", "outer", true, parentZoom); - result.handleCellClickToFilter("2.0", "revenue", false, childUSWest); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, parentZoom); + result.handleCellClickToFilter( + rowId("Airtable", "US-West"), + "revenue", + false, + childUSWest, + ); expect(sel(result).isRowHeaderSelected(dk({ outer: "Zoom" }, dims))).toBe( true, @@ -901,12 +962,17 @@ describe("header/cell mutual exclusivity", () => { const dkChild = dk({ outer: "Zoom", inner: "US-East" }, dims); // Select child row header first - result.handleCellClickToFilter("1.0", "inner", true, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "inner", + true, + childUSEast, + ); expect(sel(result).isRowHeaderSelected(dkChild)).toBe(true); expect(selectedValues(fm, "inner")).toEqual(["US-East"]); // Click parent row header — child must be evicted - result.handleCellClickToFilter("1", "outer", true, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, parentZoom); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(true); expect(sel(result).isRowHeaderSelected(dkChild)).toBe(false); @@ -923,11 +989,16 @@ describe("header/cell mutual exclusivity", () => { const dkChild = dk({ outer: "Zoom", inner: "US-East" }, dims); // Select parent first - result.handleCellClickToFilter("1", "outer", true, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, parentZoom); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(true); // Click child row header — parent must be evicted - result.handleCellClickToFilter("1.0", "inner", true, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "inner", + true, + childUSEast, + ); expect(sel(result).isRowHeaderSelected(dkChild)).toBe(true); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(false); @@ -942,11 +1013,16 @@ describe("header/cell mutual exclusivity", () => { const dkAirtableChild = dk({ outer: "Airtable", inner: "US-West" }, dims); // Select a child under a different parent first - result.handleCellClickToFilter("2.0", "inner", true, childUSWest); + result.handleCellClickToFilter( + rowId("Airtable", "US-West"), + "inner", + true, + childUSWest, + ); expect(sel(result).isRowHeaderSelected(dkAirtableChild)).toBe(true); // Click Zoom parent — Airtable's child header is a different lineage, must coexist - result.handleCellClickToFilter("1", "outer", true, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, parentZoom); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(true); expect(sel(result).isRowHeaderSelected(dkAirtableChild)).toBe(true); @@ -961,12 +1037,17 @@ describe("header/cell mutual exclusivity", () => { const dkZoom = dk({ outer: "Zoom" }, dims); // Select a child row header first - result.handleCellClickToFilter("1.0", "inner", true, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "inner", + true, + childUSEast, + ); expect(sel(result).isRowHeaderSelected(dkChildHeader)).toBe(true); expect(selectedValues(fm, "inner")).toEqual(["US-East"]); // Click parent row's measure cell — child row header must be evicted - result.handleCellClickToFilter("1", "revenue", false, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "revenue", false, parentZoom); expect(sel(result).isCellSelected(dkZoom, "revenue")).toBe(true); expect(sel(result).isRowHeaderSelected(dkChildHeader)).toBe(false); @@ -982,11 +1063,16 @@ describe("header/cell mutual exclusivity", () => { const dkChild = dk({ outer: "Zoom", inner: "US-East" }, dims); // Select parent row header first - result.handleCellClickToFilter("1", "outer", true, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "outer", true, parentZoom); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(true); // Click child row's measure cell — parent row header must be evicted - result.handleCellClickToFilter("1.0", "revenue", false, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "revenue", + false, + childUSEast, + ); expect(sel(result).isCellSelected(dkChild, "revenue")).toBe(true); expect(sel(result).isRowHeaderSelected(dkZoom)).toBe(false); @@ -1001,12 +1087,17 @@ describe("header/cell mutual exclusivity", () => { const dkZoom = dk({ outer: "Zoom" }, dims); // Select a child measure cell first - result.handleCellClickToFilter("1.0", "revenue", false, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "revenue", + false, + childUSEast, + ); expect(sel(result).isCellSelected(dkChild, "revenue")).toBe(true); expect(selectedValues(fm, "inner")).toEqual(["US-East"]); // Click the parent row's measure cell — child cell must be evicted - result.handleCellClickToFilter("1", "revenue", false, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "revenue", false, parentZoom); expect(sel(result).isCellSelected(dkZoom, "revenue")).toBe(true); expect(sel(result).isCellSelected(dkChild, "revenue")).toBe(false); @@ -1029,9 +1120,14 @@ describe("header/cell mutual exclusivity", () => { }; nestedData[0].subRows = [...dkChildExpanded, childUSWestUnderZoom]; - result.handleCellClickToFilter("1.0", "revenue", false, childUSEast); result.handleCellClickToFilter( - "1.1", + rowId("Zoom", "US-East"), + "revenue", + false, + childUSEast, + ); + result.handleCellClickToFilter( + rowId("Zoom", "US-West"), "revenue", false, childUSWestUnderZoom, @@ -1039,7 +1135,7 @@ describe("header/cell mutual exclusivity", () => { expect(sel(result).cellSelections.size).toBe(2); // Click parent row cell — both children evicted - result.handleCellClickToFilter("1", "revenue", false, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "revenue", false, parentZoom); expect( sel(result).isCellSelected(dk({ outer: "Zoom" }, dims), "revenue"), @@ -1058,11 +1154,16 @@ describe("header/cell mutual exclusivity", () => { const dkChild = dk({ outer: "Zoom", inner: "US-East" }, dims); // Select parent row cell first - result.handleCellClickToFilter("1", "revenue", false, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "revenue", false, parentZoom); expect(sel(result).isCellSelected(dkZoom, "revenue")).toBe(true); // Click child row cell — parent cell must be evicted - result.handleCellClickToFilter("1.0", "revenue", false, childUSEast); + result.handleCellClickToFilter( + rowId("Zoom", "US-East"), + "revenue", + false, + childUSEast, + ); expect(sel(result).isCellSelected(dkChild, "revenue")).toBe(true); expect(sel(result).isCellSelected(dkZoom, "revenue")).toBe(false); @@ -1076,12 +1177,17 @@ describe("header/cell mutual exclusivity", () => { const dkAirtableChild = dk({ outer: "Airtable", inner: "US-West" }, dims); // Select a cell under Airtable parent - result.handleCellClickToFilter("2.0", "revenue", false, childUSWest); + result.handleCellClickToFilter( + rowId("Airtable", "US-West"), + "revenue", + false, + childUSWest, + ); expect(sel(result).isCellSelected(dkAirtableChild, "revenue")).toBe(true); // Click Zoom parent row cell — Airtable's child cell is a different // lineage and must coexist - result.handleCellClickToFilter("1", "revenue", false, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "revenue", false, parentZoom); expect( sel(result).isCellSelected(dk({ outer: "Zoom" }, dims), "revenue"), @@ -1098,8 +1204,13 @@ describe("header/cell mutual exclusivity", () => { // Two cells in the same parent row, different columns: same dimValues, // not in a strict subset/superset relationship — both must coexist. - result.handleCellClickToFilter("1", "revenue", false, parentZoom); - result.handleCellClickToFilter("1", "other_measure", false, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "revenue", false, parentZoom); + result.handleCellClickToFilter( + rowId("Zoom"), + "other_measure", + false, + parentZoom, + ); expect(sel(result).isCellSelected(dkZoom, "revenue")).toBe(true); expect(sel(result).isCellSelected(dkZoom, "other_measure")).toBe(true); @@ -1112,13 +1223,23 @@ describe("header/cell mutual exclusivity", () => { const { result, fm } = setupNested(); const dkZoom = dk({ outer: "Zoom" }, dims); - result.handleCellClickToFilter("1", "revenue", false, parentZoom); - result.handleCellClickToFilter("1", "other_measure", false, parentZoom); + result.handleCellClickToFilter(rowId("Zoom"), "revenue", false, parentZoom); + result.handleCellClickToFilter( + rowId("Zoom"), + "other_measure", + false, + parentZoom, + ); expect(selectedValues(fm, "outer")).toEqual(["Zoom"]); // Deselecting one of the two cells removes no dimension value, since the // remaining cell in the same row still needs outer=Zoom. - result.handleCellClickToFilter("1", "other_measure", false, parentZoom); + result.handleCellClickToFilter( + rowId("Zoom"), + "other_measure", + false, + parentZoom, + ); expect(sel(result).isCellSelected(dkZoom, "revenue")).toBe(true); expect(sel(result).isCellSelected(dkZoom, "other_measure")).toBe(false); @@ -1176,20 +1297,25 @@ describe("header/cell mutual exclusivity", () => { const dkZoom = dk({ outer: "Zoom" }, colDims); result.handleCellClickToFilter( - "1.0", + rowId("Zoom", "US-East"), childEastColId, false, childEastCol, ); result.handleCellClickToFilter( - "1.1", + rowId("Zoom", "US-West"), childWestColId, false, childWestCol, ); expect(sel(result).cellSelections.size).toBe(2); - result.handleCellClickToFilter("1", totalsColId, false, parentZoomCol); + result.handleCellClickToFilter( + rowId("Zoom"), + totalsColId, + false, + parentZoomCol, + ); expect(sel(result).isCellSelected(dkZoom, totalsColId)).toBe(true); expect(sel(result).isCellSelected(dkChildEast, childEastColId)).toBe( @@ -1211,13 +1337,13 @@ describe("header/cell mutual exclusivity", () => { // Children sit at different quarters result.handleCellClickToFilter( - "1.0", + rowId("Zoom", "US-East"), childEastColId, false, childEastCol, ); result.handleCellClickToFilter( - "1.1", + rowId("Zoom", "US-West"), childWestColId, false, childWestCol, @@ -1226,7 +1352,12 @@ describe("header/cell mutual exclusivity", () => { // Click parent's Q1-total cell — both children must be evicted even // though one is at Q2. - result.handleCellClickToFilter("1", q1TotalColId, false, parentZoomCol); + result.handleCellClickToFilter( + rowId("Zoom"), + q1TotalColId, + false, + parentZoomCol, + ); expect(sel(result).isCellSelected(dkZoom, q1TotalColId)).toBe(true); expect(sel(result).isCellSelected(dkChildEast, childEastColId)).toBe( @@ -1247,13 +1378,13 @@ describe("header/cell mutual exclusivity", () => { const dkZoom = dk({ outer: "Zoom" }, colDims); result.handleCellClickToFilter( - "1.0", + rowId("Zoom", "US-East"), childEastColId, false, childEastCol, ); result.handleCellClickToFilter( - "1.1", + rowId("Zoom", "US-West"), childWestColId, false, childWestCol, @@ -1262,7 +1393,12 @@ describe("header/cell mutual exclusivity", () => { // Click parent's Q1×Prod leaf cell — both children must be evicted // regardless of which column they sit in. - result.handleCellClickToFilter("1", q1ProdColId, false, parentZoomCol); + result.handleCellClickToFilter( + rowId("Zoom"), + q1ProdColId, + false, + parentZoomCol, + ); expect(sel(result).isCellSelected(dkZoom, q1ProdColId)).toBe(true); expect(sel(result).isCellSelected(dkChildEast, childEastColId)).toBe( @@ -1299,7 +1435,12 @@ describe("header/cell mutual exclusivity", () => { ); const dkUS = dimKeyFromRow(nestedFlatData[0], ["country"]); - result.handleCellClickToFilter("1", naColId, false, nestedFlatData[0]); + result.handleCellClickToFilter( + rowId("US"), + naColId, + false, + nestedFlatData[0], + ); expect(sel(result).isCellSelected(dkUS, naColId)).toBe(true); expect(selectedValues(fm, "country")).toEqual(["US"]); @@ -1319,7 +1460,12 @@ describe("header/cell mutual exclusivity", () => { result.handleColumnHeaderClick({ region: "NA" }); expect(sel(result).isColumnHeaderSelected({ region: "NA" })).toBe(true); - result.handleCellClickToFilter("1", naColId, false, nestedFlatData[0]); + result.handleCellClickToFilter( + rowId("US"), + naColId, + false, + nestedFlatData[0], + ); expect(sel(result).isCellSelected(dkUS, naColId)).toBe(true); expect(sel(result).isColumnHeaderSelected({ region: "NA" })).toBe(false); @@ -1333,7 +1479,12 @@ describe("header/cell mutual exclusivity", () => { result.handleColumnHeaderClick({ region: "NA" }); // header is NA // cell is in EU column, so they do NOT overlap - result.handleCellClickToFilter("1", euColId, false, nestedFlatData[0]); + result.handleCellClickToFilter( + rowId("US"), + euColId, + false, + nestedFlatData[0], + ); expect(sel(result).isColumnHeaderSelected({ region: "NA" })).toBe(true); expect(sel(result).isCellSelected(dkUS, euColId)).toBe(true); diff --git a/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.ts b/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.ts index 7a669aac397..2f17b22f029 100644 --- a/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.ts +++ b/web-common/src/features/canvas/components/pivot/pivot-click-to-filter.ts @@ -20,6 +20,7 @@ import { dimKeyFromDimValues, dimKeyFromRow, } from "@rilldata/web-common/features/dashboards/pivot/pivot-click-selection"; +import { parentExpandKey } from "@rilldata/web-common/features/dashboards/pivot/pivot-expand-keys"; import { type ExtractedFilter, type PivotRowSelectionState, @@ -614,7 +615,7 @@ export function createPivotClickToFilter( const $clickSelection = get(clickSelectionStore); // In nested mode, row data stores all values under rowDimensions[0], - // so we must use positional rowId navigation to get correct dim→value pairs. + // so we must resolve the value-based rowId to get correct dim→value pairs. const isNested = !$config.isFlat; const dimValues = isNested ? Object.fromEntries( @@ -627,7 +628,7 @@ export function createPivotClickToFilter( // For nested child rows (depth > 0), build dimKey from the fully-resolved // dimValues (which include parent dimension values); dimKeyFromRow only // sees rowDimensions[0] and would produce identical keys across parents. - const isNestedChild = isNested && rowId.includes("."); + const isNestedChild = isNested && parentExpandKey(rowId) !== ""; const dk = isNestedChild ? dimKeyFromDimValues(dimValues, $config.rowDimensionNames) : dimKeyFromRow(rowData, $config.rowDimensionNames); diff --git a/web-common/src/features/dashboards/pivot/FlatTable.svelte b/web-common/src/features/dashboards/pivot/FlatTable.svelte index e4386dcdc5c..cd67966c12f 100644 --- a/web-common/src/features/dashboards/pivot/FlatTable.svelte +++ b/web-common/src/features/dashboards/pivot/FlatTable.svelte @@ -28,6 +28,7 @@ type PivotClickSelectionState, dimKeyFromRow, } from "./pivot-click-selection"; + import { PIVOT_TOTALS_ROW_ID } from "./pivot-expand-keys"; import type { PivotRowSelectionState } from "./pivot-row-selection"; import type { CellFormatter } from "./pivot-conditional-formatting"; import PivotHeaderLabel from "./PivotHeaderLabel.svelte"; @@ -76,9 +77,10 @@ $: headers = headerGroups[0].headers; - // The totals row is always tanstack row "0" (see PivotTable.svelte). When - // pinned to the bottom it is skipped in the virtualized body and rendered - // once more in a sticky , so no row ids or index math change. + // The totals row is always the first tanstack row, with id + // PIVOT_TOTALS_ROW_ID (see PivotTable.svelte). When pinned to the bottom it + // is skipped in the virtualized body and rendered once more in a sticky + // , so no row ids or index math change. $: totalsRowAtBottom = !!totalsRow && totalsRowPosition === "bottom"; // Initialize column lengths if not already set @@ -291,7 +293,7 @@ {@const rowId = rows[rowIndex].id} {@const rowData = rows[rowIndex].original} {@const dk = dimKeyFromRow(rowData, config?.rowDimensionNames ?? [])} - {@const isTotalsRow = !!totalsRow && rowId === "0"} + {@const isTotalsRow = !!totalsRow && rowId === PIVOT_TOTALS_ROW_ID} {@const isSelected = rowSelectionState?.isRowSelected(rowData) ?? false} {@const hasClickedCell = clickSelection?.hasSelectedCellInRow(dk) ?? false} {@const effectiveDimIdx = computeEffectiveDimIdx( diff --git a/web-common/src/features/dashboards/pivot/NestedTable.svelte b/web-common/src/features/dashboards/pivot/NestedTable.svelte index 8cfad08bfa4..84908cc046a 100644 --- a/web-common/src/features/dashboards/pivot/NestedTable.svelte +++ b/web-common/src/features/dashboards/pivot/NestedTable.svelte @@ -29,6 +29,7 @@ getNestedRowDimensionWidthKey, COLUMN_WIDTH_CONSTANTS as WIDTHS, } from "./pivot-column-width-utils"; + import { PIVOT_TOTALS_ROW_ID } from "./pivot-expand-keys"; import type { PivotRowSelectionState } from "./pivot-row-selection"; import { computeAncestorRowIds, @@ -120,9 +121,10 @@ $: hasExpandableRows = rowDimensions.length > 1; $: hasMeasures = measures.length > 0; - // The totals row is always tanstack row "0" (see PivotTable.svelte). When - // pinned to the bottom it is skipped in the virtualized body and rendered - // once more in a sticky , so no row ids or index math change. + // The totals row is always the first tanstack row, with id + // PIVOT_TOTALS_ROW_ID (see PivotTable.svelte). When pinned to the bottom it + // is skipped in the virtualized body and rendered once more in a sticky + // , so no row ids or index math change. $: totalsRowAtBottom = !!totalsRow && totalsRowPosition === "bottom"; $: rowDimensionNames = rowDimensions.map((d) => d.name); $: rowDimensionLabel = getRowNestedLabel(rowDimensions); @@ -631,7 +633,7 @@ rows[rowIndex].depth > 0 ? nestedDimKeyFromRow(rows[rowIndex], rowDimensionNames) : dimKeyFromRow(rowData, rowDimensionNames)} - {@const isTotalsRow = !!totalsRow && rowId === "0"} + {@const isTotalsRow = !!totalsRow && rowId === PIVOT_TOTALS_ROW_ID} {@const filterSelected = rowSelectionState?.isRowSelected( rowData, diff --git a/web-common/src/features/dashboards/pivot/PivotTable.svelte b/web-common/src/features/dashboards/pivot/PivotTable.svelte index 6fdb8d471f2..6b62acdfcfa 100644 --- a/web-common/src/features/dashboards/pivot/PivotTable.svelte +++ b/web-common/src/features/dashboards/pivot/PivotTable.svelte @@ -18,6 +18,13 @@ isShowMoreRow, splitPivotChips, } from "@rilldata/web-common/features/dashboards/pivot/pivot-utils"; + import { + buildExpandKey, + childExpandKey, + encodeExpandKeyValue, + parentExpandKey, + PIVOT_TOTALS_ROW_ID, + } from "@rilldata/web-common/features/dashboards/pivot/pivot-expand-keys"; import { copyToClipboard } from "@rilldata/web-common/lib/actions/copy-to-clipboard"; import { createVirtualizer, @@ -89,15 +96,30 @@ export let clickSelection: PivotClickSelectionState | undefined = undefined; const options: Readable> = derived( - [pivotDataStore, pivotState], - ([pivotData, state]) => { + [pivotDataStore, pivotState, config], + ([pivotData, state, cfg]) => { let tableData = [...pivotData.data]; if (pivotData.totalsRowData) { tableData = [pivotData.totalsRowData, ...pivotData.data]; } + const totalsRowData = pivotData.totalsRowData; + const rowDims = cfg.rowDimensionNames; return { data: tableData, columns: pivotData.columnDef, + // Value-based, hierarchical row ids (parent id + this row's value) so + // expansion survives sorting, adding a field, and data refreshes. + getRowId: (row, _index, parent) => { + if (totalsRowData && row === totalsRowData) + return PIVOT_TOTALS_ROW_ID; + if (cfg.isFlat) { + return buildExpandKey(rowDims.map((dim) => row[dim])); + } + const anchor = rowDims[0]; + return parent + ? childExpandKey(parent.id, row[anchor]) + : encodeExpandKeyValue(row[anchor]); + }, state: { expanded: state.expanded, sorting: state.sorting, @@ -240,7 +262,7 @@ if (needsDomains) { for (const row of flatRows) { // Always skip the prepended grand-totals row. - if (hasTotalsRow && row.id === "0") continue; + if (hasTotalsRow && row.id === PIVOT_TOTALS_ROW_ID) continue; const target = row.subRows.length > 0 ? parentValues : leafValues; for (const cell of row.getAllCells()) { const meta = cell.column.columnDef.meta; @@ -364,21 +386,20 @@ if (!nextLimit) return; - // Check if this is the outermost dimension or a nested dimension - // Outermost dimension has rowId like "0", "1", etc. (no dots) - // Nested dimensions have rowId like "0.1", "0.1.2", etc. - const isOutermostDimension = !rowId.includes("."); + // The outermost "Show more" row sits at the top level (its parent key + // is the root ""); a nested one has a real parent node whose child + // limit we raise. + const parentKey = parentExpandKey(rowId); - if (isOutermostDimension) { + if (parentKey === "") { // Handle outermost dimension "Show more" click if (setPivotOutermostRowLimit) { setPivotOutermostRowLimit(nextLimit); } } else { // Handle nested dimension "Show more" click - const expandIndex = rowId.split(".").slice(0, -1).join("."); - if (expandIndex && setPivotRowLimitForExpanded) { - setPivotRowLimitForExpanded(expandIndex, nextLimit); + if (setPivotRowLimitForExpanded) { + setPivotRowLimitForExpanded(parentKey, nextLimit); } } return; @@ -391,7 +412,7 @@ if (row.getCanExpand()) row.getToggleExpandedHandler()(); } else { // Skip totals row for filtering - const isTotalsRow = totalsRow && rowId === "0"; + const isTotalsRow = totalsRow && rowId === PIVOT_TOTALS_ROW_ID; if (isTotalsRow && onCellClickToFilter) { return; } diff --git a/web-common/src/features/dashboards/pivot/pivot-expand-keys.spec.ts b/web-common/src/features/dashboards/pivot/pivot-expand-keys.spec.ts new file mode 100644 index 00000000000..1c07709d1e6 --- /dev/null +++ b/web-common/src/features/dashboards/pivot/pivot-expand-keys.spec.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "vitest"; +import { + buildExpandKey, + childExpandKey, + expandKeyDepth, + expandKeySegments, + parentExpandKey, +} from "./pivot-expand-keys"; + +describe("pivot-expand-keys", () => { + it("round-trips a value path through build/segments", () => { + const values = ["Galatea-Stories", "facebook", "Jul 2026"]; + const key = buildExpandKey(values); + expect(expandKeySegments(key)).toEqual(values); + expect(expandKeyDepth(key)).toBe(3); + }); + + it("keeps values that contain dots or spaces intact", () => { + const values = ["a.b.c", "x y z"]; + const key = buildExpandKey(values); + expect(expandKeySegments(key)).toEqual(values); + expect(expandKeyDepth(key)).toBe(2); + }); + + it("encodes null distinctly from a genuinely absent level", () => { + const withNull = buildExpandKey(["app", null]); + const shallow = buildExpandKey(["app"]); + expect(withNull).not.toBe(shallow); + expect(expandKeyDepth(withNull)).toBe(2); + expect(expandKeyDepth(shallow)).toBe(1); + }); + + it("computes the parent by stripping the deepest segment", () => { + const key = buildExpandKey(["app", "src", "month"]); + const parent = parentExpandKey(key); + expect(expandKeySegments(parent)).toEqual(["app", "src"]); + expect(parentExpandKey(buildExpandKey(["app"]))).toBe(""); + expect(parentExpandKey("")).toBe(""); + }); + + it("childExpandKey extends a parent and is inverse of parentExpandKey", () => { + const parent = buildExpandKey(["app", "src"]); + const child = childExpandKey(parent, "month"); + expect(expandKeySegments(child)).toEqual(["app", "src", "month"]); + expect(parentExpandKey(child)).toBe(parent); + expect(childExpandKey("", "app")).toBe(buildExpandKey(["app"])); + }); + + it("treats the empty root key as depth 0", () => { + expect(expandKeyDepth("")).toBe(0); + expect(expandKeySegments("")).toEqual([]); + }); +}); diff --git a/web-common/src/features/dashboards/pivot/pivot-expand-keys.ts b/web-common/src/features/dashboards/pivot/pivot-expand-keys.ts new file mode 100644 index 00000000000..8809267a65b --- /dev/null +++ b/web-common/src/features/dashboards/pivot/pivot-expand-keys.ts @@ -0,0 +1,44 @@ +/** + * Value-based keys for pivot row expansion. A row is keyed by the dimension + * values from the root to it, NUL-joined and hierarchical, so the key stays + * stable across sorting, adding a field, and data refreshes. Uses the same + * separator and null sentinel as the dimKeys in pivot-click-selection.ts, + * except that undefined also maps to the sentinel here. + */ + +// NUL separator, since dimension values won't contain it. +export const EXPAND_KEY_SEP = "\0"; + +// Sentinel for null values, so a null at depth N differs from an absent level. +// A literal "" string value encodes the same way, which only matters if +// both appear under the same parent; the dimKeys make the same trade-off. +const NULL_SENTINEL = ""; + +// The leading separator can't occur in a real id, so this can't collide. +export const PIVOT_TOTALS_ROW_ID = EXPAND_KEY_SEP + "totals"; + +export function encodeExpandKeyValue(value: unknown): string { + return value === null || value === undefined ? NULL_SENTINEL : String(value); +} + +export function buildExpandKey(values: unknown[]): string { + return values.map(encodeExpandKeyValue).join(EXPAND_KEY_SEP); +} + +export function expandKeySegments(key: string): string[] { + return key === "" ? [] : key.split(EXPAND_KEY_SEP); +} + +export function expandKeyDepth(key: string): number { + return expandKeySegments(key).length; +} + +export function parentExpandKey(key: string): string { + const i = key.lastIndexOf(EXPAND_KEY_SEP); + return i === -1 ? "" : key.slice(0, i); +} + +export function childExpandKey(parentKey: string, value: unknown): string { + const segment = encodeExpandKeyValue(value); + return parentKey === "" ? segment : parentKey + EXPAND_KEY_SEP + segment; +} diff --git a/web-common/src/features/dashboards/pivot/pivot-expansion.spec.ts b/web-common/src/features/dashboards/pivot/pivot-expansion.spec.ts index 74eee7c7385..e938a859c24 100644 --- a/web-common/src/features/dashboards/pivot/pivot-expansion.spec.ts +++ b/web-common/src/features/dashboards/pivot/pivot-expansion.spec.ts @@ -1,13 +1,32 @@ -import { describe, expect, it } from "vitest"; +import { readable } from "svelte/store"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { LOADING_CELL } from "@rilldata/web-common/features/dashboards/pivot/pivot-constants"; import { createAndExpression } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; -import { addExpandedDataToPivot } from "./pivot-expansion"; -import { type PivotDataRow, type PivotDataStoreConfig } from "./types"; +import { buildExpandKey } from "./pivot-expand-keys"; +import { + addExpandedDataToPivot, + getValuesForExpandedKey, + queryExpandedRowMeasureValues, +} from "./pivot-expansion"; +import { getAxisForDimensions } from "./pivot-queries"; +import { + type PivotDashboardContext, + type PivotDataRow, + type PivotDataStoreConfig, +} from "./types"; -function getConfig(showTotalsRow: boolean): PivotDataStoreConfig { +vi.mock("./pivot-queries", async (importOriginal) => ({ + ...(await importOriginal()), + getAxisForDimensions: vi.fn(() => readable({ isFetching: true })), +})); + +function getConfig( + showTotalsRow: boolean, + rowDimensionNames = ["publisher", "campaign"], +): PivotDataStoreConfig { return { measureNames: ["impressions"], - rowDimensionNames: ["publisher", "campaign"], + rowDimensionNames, colDimensionNames: [], allMeasures: [], allDimensions: [], @@ -38,76 +57,168 @@ function getConfig(showTotalsRow: boolean): PivotDataStoreConfig { } as unknown as PivotDataStoreConfig; } -describe("pivot expansion", () => { - it("adds expanded rows at the correct index when totals row is hidden", () => { +describe("pivot expansion (value-based keys)", () => { + it("fills the node matched by dimension value, regardless of totals row", () => { + for (const showTotals of [false, true]) { + const tableData: PivotDataRow[] = [ + { publisher: "A", subRows: [{ publisher: LOADING_CELL }] }, + { publisher: "B", subRows: [{ publisher: LOADING_CELL }] }, + ]; + + addExpandedDataToPivot( + getConfig(showTotals), + tableData, + ["publisher", "campaign"], + {}, + [ + { + isFetching: false, + expandIndex: buildExpandKey(["B"]), + rowDimensionValues: ["B"], + totals: [{ campaign: "campaign-1", impressions: 10 }], + data: [], + }, + ], + ); + + // Row "A" is untouched; row "B" (matched by value, not position) is filled. + expect(tableData[0].subRows?.[0]?.publisher).toBe(LOADING_CELL); + expect(tableData[1].subRows?.[0]).toMatchObject({ + publisher: "campaign-1", + campaign: "campaign-1", + impressions: 10, + }); + } + }); + + it("resolves a nested value path to the correct deep node", () => { const tableData: PivotDataRow[] = [ { publisher: "A", - subRows: [{ publisher: LOADING_CELL }], - }, - { - publisher: "B", - subRows: [{ publisher: LOADING_CELL }], + subRows: [ + { publisher: "camp1", subRows: [{ publisher: LOADING_CELL }] }, + ], }, ]; addExpandedDataToPivot( - getConfig(false), + getConfig(true, ["publisher", "campaign", "adgroup"]), tableData, - ["publisher", "campaign"], + ["publisher", "campaign", "adgroup"], {}, [ { isFetching: false, - expandIndex: "1", - rowDimensionValues: ["B"], - totals: [{ campaign: "campaign-1", impressions: 10 }], + expandIndex: buildExpandKey(["A", "camp1"]), + rowDimensionValues: ["A", "camp1"], + totals: [{ adgroup: "ad-1", impressions: 5 }], data: [], }, ], ); - expect(tableData[0].subRows?.[0]?.publisher).toBe(LOADING_CELL); - expect(tableData[1].subRows?.[0]).toMatchObject({ - publisher: "campaign-1", - campaign: "campaign-1", - impressions: 10, + expect(tableData[0].subRows?.[0]?.subRows?.[0]).toMatchObject({ + publisher: "ad-1", + adgroup: "ad-1", + impressions: 5, }); }); - it("keeps the totals row offset when totals row is visible", () => { + it("does nothing when the value path matches no row", () => { const tableData: PivotDataRow[] = [ - { - publisher: "A", - subRows: [{ publisher: LOADING_CELL }], - }, - { - publisher: "B", - subRows: [{ publisher: LOADING_CELL }], - }, + { publisher: "A", subRows: [{ publisher: LOADING_CELL }] }, ]; - addExpandedDataToPivot( - getConfig(true), + getConfig(false), tableData, ["publisher", "campaign"], {}, [ { isFetching: false, - expandIndex: "2", - rowDimensionValues: ["B"], - totals: [{ campaign: "campaign-1", impressions: 10 }], + expandIndex: buildExpandKey(["does-not-exist"]), + rowDimensionValues: ["does-not-exist"], + totals: [{ campaign: "c", impressions: 1 }], data: [], }, ], ); - expect(tableData[0].subRows?.[0]?.publisher).toBe(LOADING_CELL); - expect(tableData[1].subRows?.[0]).toMatchObject({ - publisher: "campaign-1", - campaign: "campaign-1", - impressions: 10, - }); + }); +}); + +describe("getValuesForExpandedKey", () => { + const tableData: PivotDataRow[] = [ + { + publisher: "A", + subRows: [{ publisher: "camp1" }, { publisher: "camp2" }], + }, + { publisher: "B", subRows: [{ publisher: "camp3" }] }, + ]; + + it("returns the actual values along the matched path", () => { + expect( + getValuesForExpandedKey( + tableData, + ["publisher", "campaign"], + buildExpandKey(["B"]), + ), + ).toEqual(["B"]); + expect( + getValuesForExpandedKey( + tableData, + ["publisher", "campaign"], + buildExpandKey(["A", "camp2"]), + ), + ).toEqual(["A", "camp2"]); + }); + + it("stops at the deepest resolvable segment", () => { + expect( + getValuesForExpandedKey( + tableData, + ["publisher", "campaign"], + buildExpandKey(["A", "missing"]), + ), + ).toEqual(["A"]); + }); +}); + +describe("queryExpandedRowMeasureValues", () => { + const rowDimensionNames = ["publisher", "domain", "campaign"]; + // Rows are [publisher, domain, campaign], so A's children are domains. + const tableData: PivotDataRow[] = [ + { publisher: "A", subRows: [{ publisher: "example.com" }] }, + ]; + + function query(expandedKey: string) { + const config = getConfig(false, rowDimensionNames); + config.pivot.expanded = { [expandedKey]: true }; + return queryExpandedRowMeasureValues( + {} as PivotDashboardContext, + config, + tableData, + {}, + {}, + ); + } + + beforeEach(() => { + vi.mocked(getAxisForDimensions).mockClear(); + }); + + it("queries the next dimension for a fully resolved key", () => { + query(buildExpandKey(["A"])); + expect(getAxisForDimensions).toHaveBeenCalledTimes(1); + expect(vi.mocked(getAxisForDimensions).mock.calls[0][2]).toEqual([ + "domain", + ]); + }); + + it("does not query for a key that only partially resolves", () => { + // Expanded as [publisher, campaign] before domain was inserted: "camp1" + // no longer matches a row at depth 1, so only "A" resolves. + query(buildExpandKey(["A", "camp1"])); + expect(getAxisForDimensions).not.toHaveBeenCalled(); }); }); diff --git a/web-common/src/features/dashboards/pivot/pivot-expansion.ts b/web-common/src/features/dashboards/pivot/pivot-expansion.ts index d1382fe31da..66352771251 100644 --- a/web-common/src/features/dashboards/pivot/pivot-expansion.ts +++ b/web-common/src/features/dashboards/pivot/pivot-expansion.ts @@ -3,6 +3,11 @@ import { MAX_ROW_EXPANSION_LIMIT, SHOW_MORE_BUTTON, } from "@rilldata/web-common/features/dashboards/pivot/pivot-constants"; +import { + encodeExpandKeyValue, + expandKeyDepth, + expandKeySegments, +} from "@rilldata/web-common/features/dashboards/pivot/pivot-expand-keys"; import { mergeFilters } from "@rilldata/web-common/features/dashboards/pivot/pivot-merge-filters"; import { createAndExpression, @@ -59,26 +64,21 @@ export function getValuesForExpandedKey( tableData: PivotDataRow[], rowDimensions: string[], key: string, - hasTotalsRow = true, ): string[] { - const indices = key.split(".").map((index) => parseInt(index, 10)); - - if (hasTotalsRow) { - // The first row is always the totals row for the expanded context with measures - indices[0] = indices[0] - 1; - } - - // Retrieve the value from the nested array - let currentValue: PivotDataRow[] | undefined = tableData; + // Each row stores its own value under rowDimensions[0] at every depth, so + // match key segments against that to resolve a key to the actual values. + const anchor = rowDimensions[0]; const dimensionValues: string[] = []; + let currentRows: PivotDataRow[] | undefined = tableData; - indices.forEach((index, i) => { - if (!currentValue?.[index]) { - return; - } - dimensionValues.push(currentValue[index]?.[rowDimensions[i]] as string); - currentValue = currentValue[index]?.subRows; - }); + for (const segment of expandKeySegments(key)) { + const node = currentRows?.find( + (row) => encodeExpandKeyValue(row[anchor]) === segment, + ); + if (!node) break; + dimensionValues.push(node[anchor] as string); + currentRows = node.subRows; + } return dimensionValues; } @@ -201,7 +201,7 @@ export function queryExpandedRowMeasureValues( } return derived( Object.keys(expanded)?.map((expandIndex) => { - const nestLevel = expandIndex?.split(".")?.length; + const nestLevel = expandKeyDepth(expandIndex); if (nestLevel >= rowDimensionNames.length) return readable({ @@ -216,12 +216,14 @@ export function queryExpandedRowMeasureValues( tableData, rowDimensionNames, expandIndex, - config.pivot?.showTotalsRow !== false && numMeasures > 0, ); + // A key that resolves only partway is either still loading or stale + // (e.g. a dimension was inserted above it), so don't query with a + // partial filter. if ( !anchorDimension || - !values.length || + values.length !== nestLevel || values.some((v) => v === undefined || v === LOADING_CELL) ) return readable({ @@ -478,35 +480,21 @@ export function addExpandedDataToPivot( const rowValues = expandedRowData.rowDimensionValues; if (rowValues.length === 0) return; - const indices = expandedRowData.expandIndex - .split(".") - .map((index) => parseInt(index, 10)); - - if ( - config.pivot?.showTotalsRow !== false && - config.measureNames.length > 0 - ) { - // The first row is always the totals row for the expanded context with measures - indices[0] = indices[0] - 1; - } + const segments = expandKeySegments(expandedRowData.expandIndex); - let parent: PivotDataRow[] = pivotData; // Keep a reference to the parent array - let lastIdx = 0; - - // Traverse the data array to the right position - for (let i = 0; i < indices.length; i++) { - if (!parent[indices[i]]) break; - if (i < indices.length - 1) { - const subRows = parent[indices[i]].subRows; - if (!subRows) break; - parent = subRows; - } - lastIdx = indices[i]; + let node: PivotDataRow | undefined; + let currentRows: PivotDataRow[] | undefined = pivotData; + for (const segment of segments) { + node = currentRows?.find( + (row) => encodeExpandKeyValue(row[rowDimensions[0]]) === segment, + ); + if (!node) break; + currentRows = node.subRows; } - // Update the specific array at the position - if (parent[lastIdx] && parent[lastIdx].subRows) { - const anchorDimension = rowDimensions[indices.length]; + // Update the node's subRows in place + if (node && node.subRows) { + const anchorDimension = rowDimensions[segments.length]; let skeletonSubTable: PivotDataRow[] = [ { [anchorDimension]: LOADING_CELL }, @@ -540,7 +528,7 @@ export function addExpandedDataToPivot( * is greater than number of nest levels expanded except * for the last level */ - if (numRowDimensions - 1 > indices.length) { + if (numRowDimensions - 1 > segments.length) { newRow.subRows = [{ [rowDimensions[0]]: LOADING_CELL }]; } return newRow; @@ -558,7 +546,7 @@ export function addExpandedDataToPivot( } as PivotDataRow); } - parent[lastIdx].subRows = mappedSubRows; + node.subRows = mappedSubRows; } }); return pivotData; diff --git a/web-common/src/features/dashboards/pivot/pivot-filter-scope.spec.ts b/web-common/src/features/dashboards/pivot/pivot-filter-scope.spec.ts index 60265ebf0fc..6cdf991fe55 100644 --- a/web-common/src/features/dashboards/pivot/pivot-filter-scope.spec.ts +++ b/web-common/src/features/dashboards/pivot/pivot-filter-scope.spec.ts @@ -17,6 +17,7 @@ import { import type { V1Expression } from "@rilldata/web-common/runtime-client"; import { describe, expect, it } from "vitest"; import { buildFinalPivotStateDetails } from "./pivot-data-assembly"; +import { buildExpandKey } from "./pivot-expand-keys"; import { getFiltersForColumnHeader, getFiltersForRowData, @@ -76,7 +77,11 @@ describe("pivot filter builders exclude the global where filter", () => { }); it("getFiltersForRowHeader", () => { - const result = getFiltersForRowHeader(makeConfig(), "0", FLAT_DATA); + const result = getFiltersForRowHeader( + makeConfig(), + buildExpandKey(["US"]), + FLAT_DATA, + ); expect(identsIn(result.filters)).toEqual(["country"]); }); @@ -92,7 +97,7 @@ describe("pivot filter builders exclude the global where filter", () => { it("getFiltersForCell on a flat table", () => { const result = getFiltersForCell( makeConfig(), - "0", + buildExpandKey(["US"]), "revenue", {}, FLAT_DATA, @@ -110,7 +115,7 @@ describe("pivot filter builders exclude the global where filter", () => { const result = getFiltersForCell( config, - "0", + buildExpandKey(["US"]), "c0v0m0", { region: ["NA", "EU"] }, FLAT_DATA, @@ -143,7 +148,7 @@ describe("rows viewer cell filters include the global where filter", () => { it("merges config.whereFilter into activeCellFilters", () => { const config = makeConfig({ pivot: { - activeCell: { rowId: "0", columnId: "revenue" }, + activeCell: { rowId: buildExpandKey(["US"]), columnId: "revenue" }, rowPage: 1, showTotalsRow: false, sorting: [], diff --git a/web-common/src/features/dashboards/pivot/pivot-row-selection.ts b/web-common/src/features/dashboards/pivot/pivot-row-selection.ts index 3717d98d1db..93a6cbbc36a 100644 --- a/web-common/src/features/dashboards/pivot/pivot-row-selection.ts +++ b/web-common/src/features/dashboards/pivot/pivot-row-selection.ts @@ -39,17 +39,10 @@ function getRawRowValues( rowId: string, tableData: PivotDataRow[], ): string[] { - const { rowDimensionNames, measureNames, isFlat } = config; - const hasTotalsRow = - config.pivot?.showTotalsRow !== false && measureNames.length > 0; + const { rowDimensionNames, isFlat } = config; return isFlat - ? getValuesForFlatTable(tableData, rowDimensionNames, rowId, hasTotalsRow) - : getValuesForExpandedKey( - tableData, - rowDimensionNames, - rowId, - hasTotalsRow, - ); + ? getValuesForFlatTable(tableData, rowDimensionNames, rowId) + : getValuesForExpandedKey(tableData, rowDimensionNames, rowId); } /** @@ -77,7 +70,7 @@ export function getDimensionValuesForRow( /** * Like getDimensionValuesForRow but reads values directly from a PivotDataRow - * instead of using positional rowId indexing. Stable across sorting. + * instead of resolving a rowId against the table data. */ export function getDimensionValuesFromRowData( config: PivotDataStoreConfig, diff --git a/web-common/src/features/dashboards/pivot/pivot-selection-indices.spec.ts b/web-common/src/features/dashboards/pivot/pivot-selection-indices.spec.ts index 1f85c9733bc..411afd5b118 100644 --- a/web-common/src/features/dashboards/pivot/pivot-selection-indices.spec.ts +++ b/web-common/src/features/dashboards/pivot/pivot-selection-indices.spec.ts @@ -5,6 +5,7 @@ import { columnHeaderKey, nestedDimKeyFromRow, } from "./pivot-click-selection"; +import { buildExpandKey } from "./pivot-expand-keys"; import { computeAncestorRowIds, computeCellSelectedColDimGroupIndices, @@ -296,10 +297,20 @@ describe("computeAncestorRowIds", () => { const rowDimensionNames = ["A", "B", "C"]; // Tree: A expanded, B expanded. Visible rows: aRow, bRow, c1Row, c2Row. - const aRow = makeRow("1", 0, "a_val", []); - const bRow = makeRow("1.0", 1, "b_val", [aRow]); - const c1Row = makeRow("1.0.0", 2, "c1_val", [aRow, bRow]); - const c2Row = makeRow("1.0.1", 2, "c2_val", [aRow, bRow]); + const aRow = makeRow(buildExpandKey(["a_val"]), 0, "a_val", []); + const bRow = makeRow(buildExpandKey(["a_val", "b_val"]), 1, "b_val", [aRow]); + const c1Row = makeRow( + buildExpandKey(["a_val", "b_val", "c1_val"]), + 2, + "c1_val", + [aRow, bRow], + ); + const c2Row = makeRow( + buildExpandKey(["a_val", "b_val", "c2_val"]), + 2, + "c2_val", + [aRow, bRow], + ); const allRows = [aRow, bRow, c1Row, c2Row]; it("B header clicked with C rows visible: B's own id is NOT in ancestor set", () => { @@ -322,9 +333,9 @@ describe("computeAncestorRowIds", () => { const ids = computeAncestorRowIds(selection, allRows, rowDimensionNames); // A's rowId "1" should be in the set (A is an ancestor of B). - expect(ids.has("1")).toBe(true); + expect(ids.has(buildExpandKey(["a_val"]))).toBe(true); // B's own rowId "1.0" must NOT be in the set — B is the clicked row. - expect(ids.has("1.0")).toBe(false); + expect(ids.has(buildExpandKey(["a_val", "b_val"]))).toBe(false); }); it("B header clicked and a C row has a null leaf value: keys do not collide", () => { @@ -332,7 +343,12 @@ describe("computeAncestorRowIds", () => { // landmark is null for a given city+agency pair). The dimKey for // such a child row must remain distinct from its B parent's dk so // B's own selection does not bleed into a null-valued descendant. - const c1RowNullLeaf = makeRow("1.0.0", 2, null, [aRow, bRow]); + const c1RowNullLeaf = makeRow( + buildExpandKey(["a_val", "b_val", "c1_null"]), + 2, + null, + [aRow, bRow], + ); const rows = [aRow, bRow, c1RowNullLeaf, c2Row]; const dkB = ["a_val", "b_val"].join("\0"); @@ -354,8 +370,8 @@ describe("computeAncestorRowIds", () => { const ids = computeAncestorRowIds(selection, rows, rowDimensionNames); - expect(ids.has("1")).toBe(true); - expect(ids.has("1.0")).toBe(false); + expect(ids.has(buildExpandKey(["a_val"]))).toBe(true); + expect(ids.has(buildExpandKey(["a_val", "b_val"]))).toBe(false); }); it("A (depth-0) header clicked: a depth-1 child with null value is not selected", () => { diff --git a/web-common/src/features/dashboards/pivot/pivot-selection-indices.ts b/web-common/src/features/dashboards/pivot/pivot-selection-indices.ts index a9fa8c6fab4..71552c25e4f 100644 --- a/web-common/src/features/dashboards/pivot/pivot-selection-indices.ts +++ b/web-common/src/features/dashboards/pivot/pivot-selection-indices.ts @@ -8,6 +8,7 @@ import type { HeaderGroup, Row } from "tanstack-table-8-svelte-5"; import type { PivotClickSelectionState } from "./pivot-click-selection"; import { dimKeyFromRow, nestedDimKeyFromRow } from "./pivot-click-selection"; +import { parentExpandKey } from "./pivot-expand-keys"; import type { PivotDataRow } from "./types"; function selectedColumnHeaderFilters( @@ -215,10 +216,10 @@ export function computeAncestorRowIds( const selectedDepth = selectedDepthByDk.get(dk); if (selectedDepth === undefined) continue; if (row.depth !== selectedDepth) continue; - let id = row.id; - while (id.includes(".")) { - id = id.substring(0, id.lastIndexOf(".")); + let id = parentExpandKey(row.id); + while (id !== "") { ancestorIds.add(id); + id = parentExpandKey(id); } } return ancestorIds; diff --git a/web-common/src/features/dashboards/pivot/pivot-utils.ts b/web-common/src/features/dashboards/pivot/pivot-utils.ts index 7be4af5a963..437c64fe8f5 100644 --- a/web-common/src/features/dashboards/pivot/pivot-utils.ts +++ b/web-common/src/features/dashboards/pivot/pivot-utils.ts @@ -2,6 +2,7 @@ import { itemsInTag, type TagIndex, } from "@rilldata/web-common/components/menu/tag-utils"; +import { buildExpandKey } from "@rilldata/web-common/features/dashboards/pivot/pivot-expand-keys"; import { getValuesForExpandedKey } from "@rilldata/web-common/features/dashboards/pivot/pivot-expansion"; import { createAndExpression, @@ -603,24 +604,16 @@ export function getValuesForFlatTable( tableData: PivotDataRow[], rowDimensions: string[], rowId: string, - hasTotalsRow: boolean, ): string[] { - let index = parseInt(rowId, 10); - const dimensionValues: string[] = []; - - if (hasTotalsRow) index = index - 1; - - const row = tableData?.[index]; - if (!row) return dimensionValues; - - // For flat tables, collect all dimension values in order - rowDimensions.forEach((dim) => { - if (dim in row) { - dimensionValues.push(row[dim] as string); - } - }); + const row = tableData?.find( + (candidate) => + buildExpandKey(rowDimensions.map((dim) => candidate[dim])) === rowId, + ); + if (!row) return []; - return dimensionValues; + return rowDimensions + .filter((dim) => dim in row) + .map((dim) => row[dim] as string); } /** @@ -636,7 +629,7 @@ export function getValuesForFlatTable( * merge config.whereFilter themselves. * * Every public getFiltersFor* function delegates here after extracting its - * dimension entries from whichever data source it uses (positional rowId, + * dimension entries from whichever data source it uses (value-based rowId, * direct rowData, column header path, etc.). */ export function buildPivotFilter( @@ -673,28 +666,16 @@ export function getFiltersForCell( tableData: PivotDataRow[], upToDimensionIndex?: number, ): PivotFilter { - const { rowDimensionNames, measureNames, isFlat } = config; - const hasTotalsRow = - config.pivot?.showTotalsRow !== false && measureNames.length > 0; + const { rowDimensionNames, isFlat } = config; let values: string[]; if (isFlat) { - values = getValuesForFlatTable( - tableData, - rowDimensionNames, - rowId, - hasTotalsRow, - ); + values = getValuesForFlatTable(tableData, rowDimensionNames, rowId); if (upToDimensionIndex !== undefined && upToDimensionIndex >= 0) { values = values.slice(0, upToDimensionIndex + 1); } } else { - values = getValuesForExpandedKey( - tableData, - rowDimensionNames, - rowId, - hasTotalsRow, - ); + values = getValuesForExpandedKey(tableData, rowDimensionNames, rowId); } const rowEntries = values.map((value, index) => ({ diff --git a/web-common/src/features/dashboards/pivot/types.ts b/web-common/src/features/dashboards/pivot/types.ts index 1385180dcba..18eb539215c 100644 --- a/web-common/src/features/dashboards/pivot/types.ts +++ b/web-common/src/features/dashboards/pivot/types.ts @@ -65,7 +65,7 @@ export interface PivotState { totalsRowPosition?: PivotTotalsRowPosition; rowLimit?: number; outermostRowLimit?: number; // Local limit for outermost dimension only - nestedRowLimits?: Record; // Local per-row limits keyed by expand index (e.g., "0.1.2") + nestedRowLimits?: Record; // Local per-row limits keyed by value-based row id (see pivot-expand-keys.ts) // Per-measure conditional formatting, keyed by measure name. Measures not // present here render without any cell formatting. measureFormatting?: Record; diff --git a/web-common/src/features/dashboards/proto-state/fromProto.ts b/web-common/src/features/dashboards/proto-state/fromProto.ts index f98d8ecf9c4..c28f26871ff 100644 --- a/web-common/src/features/dashboards/proto-state/fromProto.ts +++ b/web-common/src/features/dashboards/proto-state/fromProto.ts @@ -460,7 +460,8 @@ function fromPivotProto( ...colDimensions, ...dashboard.pivotColumnMeasures.map(mapMeasure), ], - expanded: dashboard.pivotExpanded, + // Expanded rows are runtime-only, not restored from saved dashboard state. + expanded: {}, sorting: dashboard.pivotSort ?? [], columnPage: dashboard.pivotColumnPage ?? 1, rowPage: 1, diff --git a/web-common/src/features/dashboards/stores/dashboard-stores.ts b/web-common/src/features/dashboards/stores/dashboard-stores.ts index 20351c49099..018130c9f9e 100644 --- a/web-common/src/features/dashboards/stores/dashboard-stores.ts +++ b/web-common/src/features/dashboards/stores/dashboard-stores.ts @@ -346,7 +346,6 @@ const metricsViewReducers = { } } - exploreState.pivot.expanded = {}; exploreState.pivot.rows = dimensions; }); }, @@ -371,7 +370,6 @@ const metricsViewReducers = { } } } - exploreState.pivot.expanded = {}; exploreState.pivot.columns = value; }); }, @@ -380,7 +378,6 @@ const metricsViewReducers = { updateMetricsExplorerByName(name, (exploreState) => { exploreState.pivot.rowPage = 1; exploreState.pivot.activeCell = null; - exploreState.pivot.expanded = {}; if (value.type === PivotChipType.Measure) { exploreState.pivot.columns.push(value); @@ -412,7 +409,6 @@ const metricsViewReducers = { ...exploreState.pivot, sorting, rowPage: 1, - expanded: {}, activeCell: null, }; }); diff --git a/web-local/tests/explores/pivot.spec.ts b/web-local/tests/explores/pivot.spec.ts index 188cedc5997..c12cea087d2 100644 --- a/web-local/tests/explores/pivot.spec.ts +++ b/web-local/tests/explores/pivot.spec.ts @@ -742,3 +742,83 @@ test.describe("pivot run through", () => { await validateTableContents(page, "table", expectSortedDeltaCol, 4); }); }); + +test.describe("pivot expansion persistence", () => { + test.use({ project: "AdBids" }); + + // Expanded rows are keyed by dimension values, so they survive config + // changes that keep the same rows. + // https://github.com/rilldata/rill/issues/9781 + test("expanded rows survive adding a measure, sorting, and a refresh", async ({ + page, + }) => { + test.setTimeout(45_000); + const watcher = new ResourceWatcher(page); + + await gotoNavEntry(page, "/metrics/AdBids_metrics.yaml"); + await page.getByRole("button", { name: "switch to code editor" }).click(); + await watcher.updateAndWaitForDashboard(pivotDashboard); + await gotoNavEntry(page, "/dashboards/AdBids_metrics_explore.yaml"); + await page.getByRole("button", { name: "Preview" }).click(); + await page.getByRole("link", { name: "Pivot", exact: true }).click(); + + const rowZone = page.locator(".dnd-zone.horizontal").nth(0); + const columnZone = page.locator(".dnd-zone.horizontal").nth(1); + const totalRecords = page.getByLabel("Total records pivot chip", { + exact: true, + }); + const timeMonth = page.getByLabel("Time pivot chip", { exact: true }); + const addRowField = page + .getByRole("button", { name: "Add filter button" }) + .nth(1); + const addColumnField = page + .getByRole("button", { name: "Add filter button" }) + .nth(2); + + // Rows = Time (month) > Publisher, columns = Total records. + await dragPivotChip(page, totalRecords, columnZone); + await dragPivotChip(page, timeMonth, rowZone); + await addRowField.click(); + await clickMenuButton(page, "Publisher"); + await expect(page.locator(".status.running")).toHaveCount(0); + + // Expand a month; a nested publisher row becomes visible. + await page.locator("td").filter({ hasText: "Jan" }).first().click(); + await expect(page.locator(".status.running")).toHaveCount(0); + await expect( + page.locator("td").filter({ hasText: "Facebook" }).first(), + ).toBeVisible(); + + // The nested row must stay visible after adding a measure. + await addColumnField.click(); + await clickMenuButton(page, "Sum of Bid Price"); + await expect(page.locator(".status.running")).toHaveCount(0); + await expect( + page.locator("td").filter({ hasText: "Facebook" }).first(), + ).toBeVisible(); + + // And it must stay visible after sorting by a measure. + await page + .locator(".header-cell") + .filter({ hasText: "Total records" }) + .first() + .click(); + await expect(page.locator(".status.running")).toHaveCount(0); + await expect( + page.locator("td").filter({ hasText: "Facebook" }).first(), + ).toBeVisible(); + + // And after a data refresh: a wider time range re-queries every row but + // still includes January. + await interactWithTimeRangeMenu(page, async () => { + await page.getByRole("menuitem", { name: "Last 12 Months" }).click(); + }); + await expect(page.getByLabel("Select time range")).toContainText( + "Last 12 Months", + ); + await expect(page.locator(".status.running")).toHaveCount(0); + await expect( + page.locator("td").filter({ hasText: "Facebook" }).first(), + ).toBeVisible(); + }); +});