Skip to content

Commit a858879

Browse files
mayagbarnesmayagbarnes
committed
Fix: analytical filter UX polish and bug fixes
1 parent 6b262eb commit a858879

8 files changed

Lines changed: 311 additions & 30 deletions

File tree

‎e2e_playwright/pivot_table_filters_test.py‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -204,30 +204,36 @@ def test_top_n_apply_reduces_row_count(self, page_at_app: Page):
204204
assert _row_headers(container).count() < rows_before
205205

206206
def test_top_n_clear_restores_row_count(self, page_at_app: Page):
207-
"""Clearing the Top N filter via the header menu restores all rows."""
207+
"""Clearing the Top N filter via the header menu restores all rows.
208+
209+
Clear keeps the menu open (for immediate re-entry) and resets inputs
210+
to defaults. The table updates immediately; the user closes the menu
211+
manually (e.g. Escape) when done.
212+
"""
208213
page = page_at_app
209214
container = get_pivot(page, "test_pivot_top_n_interactive")
210215
expect(container.get_by_test_id("pivot-table")).to_be_visible(timeout=15000)
211216

212217
# Capture baseline
213218
rows_all = _row_headers(container).count()
214219

215-
# Apply top-2
220+
# Apply top-2 (closes menu)
216221
menu = _open_header_menu(page, container, "Product")
217222
menu.get_by_test_id("header-top-n-count").fill("2")
218223
menu.get_by_test_id("header-top-n-by").select_option("Revenue")
219224
menu.get_by_test_id("header-top-n-apply").evaluate("el => el.click()")
220225
expect(page.get_by_test_id("header-menu-Product")).to_be_hidden(timeout=8000)
221226
expect(_row_headers(container)).not_to_have_count(rows_all, timeout=10000)
222227

223-
# Now clear
228+
# Clear — menu stays open; table restores all rows immediately
224229
menu2 = _open_header_menu(page, container, "Product")
225-
clear_btn = menu2.get_by_test_id("header-top-n-clear")
226-
clear_btn.evaluate("el => el.click()")
227-
expect(page.get_by_test_id("header-menu-Product")).to_be_hidden(timeout=8000)
228-
230+
menu2.get_by_test_id("header-top-n-clear").evaluate("el => el.click()")
231+
expect(page.get_by_test_id("header-menu-Product")).to_be_visible(timeout=5000)
229232
expect(_row_headers(container)).to_have_count(rows_all, timeout=10000)
230233

234+
# Close menu manually
235+
_close_header_menu(page, "Product")
236+
231237

232238
class TestValueFilterInteractiveMenu:
233239
"""Value filter set via the column header menu."""
@@ -274,14 +280,19 @@ def test_value_filter_apply_reduces_rows(self, page_at_app: Page):
274280
assert _row_headers(container).count() < rows_before
275281

276282
def test_value_filter_clear_restores_rows(self, page_at_app: Page):
277-
"""Clearing the value filter via the header menu restores all product rows."""
283+
"""Clearing the value filter via the header menu restores all product rows.
284+
285+
Clear keeps the menu open (for immediate re-entry) and resets inputs
286+
to defaults. The table updates immediately; the user closes the menu
287+
manually (e.g. Escape) when done.
288+
"""
278289
page = page_at_app
279290
container = get_pivot(page, "test_pivot_value_filter_interactive")
280291
expect(container.get_by_test_id("pivot-table")).to_be_visible(timeout=15000)
281292

282293
rows_all = _row_headers(container).count()
283294

284-
# Apply filter first (same threshold as apply test: 7000)
295+
# Apply filter first (closes menu)
285296
menu = _open_header_menu(page, container, "Product")
286297
menu.get_by_test_id("header-value-filter-by").select_option("Revenue")
287298
menu.get_by_test_id("header-value-filter-op-gt").evaluate("el => el.click()")
@@ -290,9 +301,11 @@ def test_value_filter_clear_restores_rows(self, page_at_app: Page):
290301
expect(page.get_by_test_id("header-menu-Product")).to_be_hidden(timeout=8000)
291302
expect(_row_headers(container)).not_to_have_count(rows_all, timeout=10000)
292303

293-
# Clear the filter
304+
# Clear — menu stays open; table restores all rows immediately
294305
menu2 = _open_header_menu(page, container, "Product")
295306
menu2.get_by_test_id("header-value-filter-clear").evaluate("el => el.click()")
296-
expect(page.get_by_test_id("header-menu-Product")).to_be_hidden(timeout=8000)
297-
307+
expect(page.get_by_test_id("header-menu-Product")).to_be_visible(timeout=5000)
298308
expect(_row_headers(container)).to_have_count(rows_all, timeout=10000)
309+
310+
# Close menu manually
311+
_close_header_menu(page, "Product")

‎streamlit_pivot/frontend/src/PivotRoot.tsx‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -260,11 +260,10 @@ const PivotRoot: FC<PivotRootProps> = ({
260260
typedAdaptiveGrains,
261261
);
262262
} catch {
263-
// validatePivotConfigRuntime throws for two reasons:
264-
// 1. Period comparison show_values_as on a non-temporal axis — hard error,
265-
// re-throw so the caller sees a clear message.
266-
// 2. Stale analytical filter field/by refs — strip them and retry so that
267-
// an externally-restored or imported config never hard-crashes the UI.
263+
// Strip stale analytical filter refs and retry. If the original error was
264+
// caused by a dangling field/by reference the retry succeeds. If the error
265+
// was unrelated (e.g. period comparison on a non-temporal axis) the retry
266+
// throws again and the exception propagates naturally to the error boundary.
268267
const safe: PivotConfigV1 = { ...rawCurrentConfig };
269268
delete safe.top_n_filters;
270269
delete safe.value_filters;

‎streamlit_pivot/frontend/src/engine/types.test.ts‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,56 @@ describe("validatePivotConfigV1", () => {
327327
expect(result.filters).toEqual({ region: { include: ["US", "EU"] } });
328328
});
329329

330+
it("rejects non-array analytical filter containers", () => {
331+
expect(() =>
332+
validatePivotConfigV1({
333+
...DEFAULT_CONFIG,
334+
top_n_filters: { field: "region" },
335+
}),
336+
).toThrow("'top_n_filters' must be an array");
337+
expect(() =>
338+
validatePivotConfigV1({
339+
...DEFAULT_CONFIG,
340+
value_filters: { field: "region" },
341+
}),
342+
).toThrow("'value_filters' must be an array");
343+
});
344+
345+
it("requires value2 for between value filters", () => {
346+
expect(() =>
347+
validatePivotConfigV1({
348+
...DEFAULT_CONFIG,
349+
value_filters: [
350+
{
351+
field: "region",
352+
by: "revenue",
353+
operator: "between",
354+
value: 10,
355+
},
356+
],
357+
}),
358+
).toThrow(
359+
"'value_filters[0].value2' must be a number when operator is \"between\"",
360+
);
361+
});
362+
363+
it("rejects non-numeric value2 on value filters", () => {
364+
expect(() =>
365+
validatePivotConfigV1({
366+
...DEFAULT_CONFIG,
367+
value_filters: [
368+
{
369+
field: "region",
370+
by: "revenue",
371+
operator: "gt",
372+
value: 10,
373+
value2: "20",
374+
},
375+
],
376+
}),
377+
).toThrow("'value_filters[0].value2' must be a number");
378+
});
379+
330380
it("accepts row_layout when valid", () => {
331381
const result = validatePivotConfigV1({
332382
...DEFAULT_CONFIG,
@@ -1052,6 +1102,128 @@ describe("date hierarchy helpers", () => {
10521102
),
10531103
).not.toThrow();
10541104
});
1105+
1106+
it("throws when top_n_filters field is not in the declared row dims", () => {
1107+
const config = makeConfig({
1108+
rows: ["Region"],
1109+
columns: ["Year"],
1110+
values: ["Revenue"],
1111+
top_n_filters: [
1112+
{
1113+
field: "Category",
1114+
n: 5,
1115+
by: "Revenue",
1116+
direction: "top",
1117+
axis: "rows",
1118+
},
1119+
],
1120+
});
1121+
expect(() => validatePivotConfigRuntime(config)).toThrow(
1122+
'top_n_filters[0].field "Category" is not present in rows dimensions',
1123+
);
1124+
});
1125+
1126+
it("throws when top_n_filters by is not a declared measure", () => {
1127+
const config = makeConfig({
1128+
rows: ["Region"],
1129+
columns: ["Year"],
1130+
values: ["Revenue"],
1131+
top_n_filters: [
1132+
{
1133+
field: "Region",
1134+
n: 5,
1135+
by: "Missing",
1136+
direction: "top",
1137+
axis: "rows",
1138+
},
1139+
],
1140+
});
1141+
expect(() => validatePivotConfigRuntime(config)).toThrow(
1142+
'top_n_filters[0].by "Missing" is not a declared measure or synthetic measure',
1143+
);
1144+
});
1145+
1146+
it("throws when value_filters field is not in the declared column dims", () => {
1147+
const config = makeConfig({
1148+
rows: ["Region"],
1149+
columns: ["Year"],
1150+
values: ["Revenue"],
1151+
value_filters: [
1152+
{
1153+
field: "Category",
1154+
by: "Revenue",
1155+
operator: "gt",
1156+
value: 1000,
1157+
axis: "columns",
1158+
},
1159+
],
1160+
});
1161+
expect(() => validatePivotConfigRuntime(config)).toThrow(
1162+
'value_filters[0].field "Category" is not present in columns dimensions',
1163+
);
1164+
});
1165+
1166+
it("throws when value_filters by is not a declared measure", () => {
1167+
const config = makeConfig({
1168+
rows: ["Region"],
1169+
columns: ["Year"],
1170+
values: ["Revenue"],
1171+
value_filters: [
1172+
{ field: "Region", by: "Unknown", operator: "gte", value: 500 },
1173+
],
1174+
});
1175+
expect(() => validatePivotConfigRuntime(config)).toThrow(
1176+
'value_filters[0].by "Unknown" is not a declared measure or synthetic measure',
1177+
);
1178+
});
1179+
1180+
it("accepts valid analytical filters without throwing", () => {
1181+
const config = makeConfig({
1182+
rows: ["Region"],
1183+
columns: ["Year"],
1184+
values: ["Revenue"],
1185+
top_n_filters: [
1186+
{
1187+
field: "Region",
1188+
n: 3,
1189+
by: "Revenue",
1190+
direction: "top",
1191+
axis: "rows",
1192+
},
1193+
],
1194+
value_filters: [
1195+
{
1196+
field: "Year",
1197+
by: "Revenue",
1198+
operator: "gt",
1199+
value: 5000,
1200+
axis: "columns",
1201+
},
1202+
],
1203+
});
1204+
expect(() => validatePivotConfigRuntime(config)).not.toThrow();
1205+
});
1206+
1207+
it("accepts analytical filters that reference a synthetic measure", () => {
1208+
const config = makeConfig({
1209+
rows: ["Region"],
1210+
values: ["Revenue", "Profit"],
1211+
synthetic_measures: [
1212+
{
1213+
id: "margin",
1214+
label: "Margin",
1215+
operation: "formula" as const,
1216+
numerator: "Revenue",
1217+
denominator: "Profit",
1218+
formula: "Revenue / Profit",
1219+
},
1220+
],
1221+
top_n_filters: [
1222+
{ field: "Region", n: 5, by: "margin", direction: "top", axis: "rows" },
1223+
],
1224+
});
1225+
expect(() => validatePivotConfigRuntime(config)).not.toThrow();
1226+
});
10551227
});
10561228

10571229
describe("getAggregationForField", () => {

‎streamlit_pivot/frontend/src/engine/types.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1558,6 +1558,9 @@ export function validatePivotConfigV1(obj: unknown): PivotConfigV1 {
15581558
// top_n_filters and value_filters: structural checks only here (types, enum values).
15591559
// Semantic validation (field/by references against declared dims/measures) is done
15601560
// by validatePivotConfigRuntime() and by Python at call time.
1561+
if (o.top_n_filters !== undefined && !Array.isArray(o.top_n_filters)) {
1562+
throw new Error("'top_n_filters' must be an array");
1563+
}
15611564
if (Array.isArray(o.top_n_filters)) {
15621565
const VALID_DIRECTIONS = new Set<string>(["top", "bottom"]);
15631566
const VALID_AXES = new Set<string>(["rows", "columns"]);
@@ -1588,6 +1591,9 @@ export function validatePivotConfigV1(obj: unknown): PivotConfigV1 {
15881591
result.top_n_filters = filters as unknown as TopNFilter[];
15891592
}
15901593

1594+
if (o.value_filters !== undefined && !Array.isArray(o.value_filters)) {
1595+
throw new Error("'value_filters' must be an array");
1596+
}
15911597
if (Array.isArray(o.value_filters)) {
15921598
const VALID_OPS = new Set<string>([
15931599
"gt",
@@ -1614,6 +1620,14 @@ export function validatePivotConfigV1(obj: unknown): PivotConfigV1 {
16141620
);
16151621
if (typeof f.value !== "number")
16161622
throw new Error(`'value_filters[${i}].value' must be a number`);
1623+
if (f.operator === "between" && typeof f.value2 !== "number") {
1624+
throw new Error(
1625+
`'value_filters[${i}].value2' must be a number when operator is "between"`,
1626+
);
1627+
}
1628+
if (f.value2 !== undefined && typeof f.value2 !== "number") {
1629+
throw new Error(`'value_filters[${i}].value2' must be a number`);
1630+
}
16171631
if (f.axis !== undefined && !VALID_AXES.has(f.axis as string))
16181632
throw new Error(
16191633
`'value_filters[${i}].axis' must be "rows" or "columns"`,

‎streamlit_pivot/frontend/src/renderers/HeaderMenu.module.css‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,20 @@
214214
letter-spacing: 0.05em;
215215
color: color-mix(in srgb, var(--st-text-color) 55%, transparent);
216216
padding: 0px 8px 4px;
217-
display: block;
217+
display: flex;
218+
align-items: center;
219+
gap: 5px;
220+
}
221+
222+
/* Colored dot appended to a section label when that filter is active */
223+
.menuSectionLabel.activeSection::after {
224+
content: "";
225+
display: inline-block;
226+
width: 6px;
227+
height: 6px;
228+
border-radius: 50%;
229+
background: var(--st-primary-color);
230+
flex-shrink: 0;
218231
}
219232

220233
/* ---- Divider ---- */

0 commit comments

Comments
 (0)