diff --git a/web-common/src/features/dashboards/pivot/PivotTable.svelte b/web-common/src/features/dashboards/pivot/PivotTable.svelte index 1d4f9ad939b..bfd6b475d89 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, @@ -237,7 +259,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; @@ -340,21 +362,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; 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..92a7921acf7 --- /dev/null +++ b/web-common/src/features/dashboards/pivot/pivot-expand-keys.ts @@ -0,0 +1,41 @@ +/** + * 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. Mirrors the + * encoding in pivot-click-selection.ts. + */ + +// 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. +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..c1568d994df 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,20 @@ import { describe, expect, it } 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 { buildExpandKey } from "./pivot-expand-keys"; +import { + addExpandedDataToPivot, + getValuesForExpandedKey, +} from "./pivot-expansion"; import { type PivotDataRow, type PivotDataStoreConfig } from "./types"; -function getConfig(showTotalsRow: boolean): PivotDataStoreConfig { +function getConfig( + showTotalsRow: boolean, + rowDimensionNames = ["publisher", "campaign"], +): PivotDataStoreConfig { return { measureNames: ["impressions"], - rowDimensionNames: ["publisher", "campaign"], + rowDimensionNames, colDimensionNames: [], allMeasures: [], allDimensions: [], @@ -38,76 +45,129 @@ 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"]); }); }); diff --git a/web-common/src/features/dashboards/pivot/pivot-expansion.ts b/web-common/src/features/dashboards/pivot/pivot-expansion.ts index d1382fe31da..f0cf850f5d8 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,7 +216,6 @@ export function queryExpandedRowMeasureValues( tableData, rowDimensionNames, expandIndex, - config.pivot?.showTotalsRow !== false && numMeasures > 0, ); if ( @@ -478,35 +477,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 +525,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 +543,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..58885b2e054 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); } /** 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 3c790e926cb..fcb2ed05faf 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, @@ -583,24 +584,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); } /** @@ -653,28 +646,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/proto-state/fromProto.ts b/web-common/src/features/dashboards/proto-state/fromProto.ts index d3045c4ebd0..121c8c5cc0c 100644 --- a/web-common/src/features/dashboards/proto-state/fromProto.ts +++ b/web-common/src/features/dashboards/proto-state/fromProto.ts @@ -426,7 +426,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 6beecd0c54b..4c3c9dfe369 100644 --- a/web-common/src/features/dashboards/stores/dashboard-stores.ts +++ b/web-common/src/features/dashboards/stores/dashboard-stores.ts @@ -273,7 +273,6 @@ const metricsViewReducers = { } } - exploreState.pivot.expanded = {}; exploreState.pivot.rows = dimensions; }); }, @@ -298,7 +297,6 @@ const metricsViewReducers = { } } } - exploreState.pivot.expanded = {}; exploreState.pivot.columns = value; }); }, @@ -307,7 +305,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); @@ -339,7 +336,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 a5d79df5e0c..b779da0858c 100644 --- a/web-local/tests/explores/pivot.spec.ts +++ b/web-local/tests/explores/pivot.spec.ts @@ -700,3 +700,70 @@ 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 and sorting", 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(); + }); +});