From 9e9782419a53ae0af19914323587a23b5cf1212e Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Sat, 19 Sep 2026 14:43:00 -0400 Subject: [PATCH] Filter aggregates with the rest of the layout cleanLayout's job is to let a layout outlive the columns it names, and it walked every config field that carries a column name except aggregates -- which carries one as its key and, in the multi-arg form, a second as the weight. Empty in practice today, so nothing was breaking; the first explicit aggregate would have made a later column change abort the whole restore rather than lose one entry. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 7 +++---- PERSPECTIVE.md | 12 ++++++------ ui/src/views/Forecast.jsx | 11 +++++++++++ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 02509e8..e0f7d33 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -361,10 +361,9 @@ anyone else, gone on another machine. The one server-side layout was published layout with no owner. That route is gone; the column remains, migrated and read by nothing. -**Known gap:** `cleanLayout()` still does not guard `aggregates` -(PERSPECTIVE.md §5). Harmless while `save()` emits `aggregates: {}`, but a -published layout that adopts the weighted-mean pattern and then loses a column -would break the restore for everyone rather than for one browser. +`cleanLayout()` guards `aggregates` along with the axes — including the weight +column of the multi-arg form — and drops the offending *entry* rather than the +layout, since `restore()` is all-or-nothing. --- diff --git a/PERSPECTIVE.md b/PERSPECTIVE.md index a5678eb..ca8782a 100644 --- a/PERSPECTIVE.md +++ b/PERSPECTIVE.md @@ -257,15 +257,15 @@ it would retire the `if(...)`-expression workaround. It costs the d3fc charts, t exist in the current dataset (plus any `expressions`). dataflow's `cleanLayout()` is the reference implementation; a stale layout referencing a dropped column otherwise throws on restore. -- **`aggregates` needs the same guard, and doesn't currently have it** (verified 2026-08, - pf_app). Both existing `cleanLayout()` implementations filter - `columns`/`group_by`/`split_by`/`sort`/`filter` but leave `aggregates` untouched. That - is harmless *today* only because `viewer.save()` emits `aggregates: {}` until someone - sets one explicitly. The moment a layout adopts the weighted-mean pattern (§3a), a +- **`aggregates` needs the same guard.** pf_app's `cleanLayout()` has it as of 2026-09; + **dataflow's does not** and should adopt it. Filtering + `columns`/`group_by`/`split_by`/`sort`/`filter` and stopping there is harmless only + while `viewer.save()` emits `aggregates: {}`, which it does until someone sets an + aggregate explicitly. The moment a layout adopts the weighted-mean pattern (§3a), a dropped column aborts the entire restore — both `table.view()` and `viewer.restore()` throw `Could not get dtype for column 'X' as it does not exist in the schema`. An aggregate entry references a *target* column and, in the multi-arg form, a *weight* - column; both need validating, and the entry should be dropped rather than the layout. + column; both need validating, and the entry is what gets dropped, never the layout. Add this guard as part of adopting §3a, not after. ### Auto-pause deletes the view — turn it off for a static pivot diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index f75b805..ca9d015 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -43,6 +43,17 @@ function cleanLayout(cfg, validCols) { if (c.split_full) c.split_full = c.split_full.filter(ok) if (c.sort) c.sort = c.sort.filter(([col]) => ok(col)) if (c.filter) c.filter = c.filter.filter(([col]) => ok(col)) + // aggregates is keyed by column and, in the multi-arg form, names a second + // one: ["weighted mean", "units"] depends on the weight column as much as on + // the column it is set against, so both have to survive or the entry goes. + // Drop the entry, never the layout -- restore() is all-or-nothing, so one + // stale aggregate would otherwise take the whole config down with it. + if (c.aggregates) { + c.aggregates = Object.fromEntries( + Object.entries(c.aggregates).filter(([col, agg]) => + ok(col) && (!Array.isArray(agg) || agg.slice(1).every(a => typeof a !== 'string' || ok(a)))) + ) + } return c }