From 7738b904bfbcff761f06854d86979a385c695354 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Thu, 17 Sep 2026 22:44:07 -0400 Subject: [PATCH] Delete the client-side ordering expressions With the prefix stored in pf.log.label there is nothing left to compute, so all of it goes: ORDER_EXPR_NAMES, buildOrderExpression and its ExprTK printable-ASCII-per-byte guard, SYNTHETIC_SEGMENTS and its 98/99 ordinals, syncOrderExpressions with its two self-issued fetches, the dbgOrder tracing, and the three places it had to be re-applied because restore() replaces `expressions` wholesale. What remains is a list of the names it used to manage, stripped by cleanLayout so a layout saved under the old scheme does not keep ordering by a rule nothing updates. The strip goes before the axis filter: dropping them from `expressions` is what makes the existing ok() reject them on every axis, which restore() requires -- an expression the pivot is using cannot vanish from underneath it. Net 250 lines out. The Baseline page's reorder buttons no longer feed anything and go next. Co-Authored-By: Claude Opus 5 (1M context) --- ui/src/components/BridgeView.jsx | 7 +- ui/src/views/Forecast.jsx | 237 +++---------------------------- 2 files changed, 26 insertions(+), 218 deletions(-) diff --git a/ui/src/components/BridgeView.jsx b/ui/src/components/BridgeView.jsx index 7802673..c4781ae 100644 --- a/ui/src/components/BridgeView.jsx +++ b/ui/src/components/BridgeView.jsx @@ -241,10 +241,9 @@ export default function BridgeView({ } } else { let filter = [] - // Expression columns have to come with the filter that uses them. The - // pivot's filter can name pf_bucket_ord or pf_segment_ord, which exist - // only as expressions — a view built without them cannot resolve the - // column and fails outright. + // Expression columns have to come with the filter that uses them: a + // filter can name a column that exists only as an expression, and a view + // built without it cannot resolve the column and fails outright. let expressions = {} if (scope === 'filtered' && viewerRef?.current) { const cfg = await viewerRef.current.save() diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index b85d8fc..390cafa 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -18,7 +18,13 @@ const LAYOUTS_KEY = (vid) => `pf_layouts_v${vid}` // named layout list function cleanLayout(cfg, validCols) { if (!cfg) return cfg const c = { ...cfg } - const exprNames = new Set(Object.keys(cfg.expressions || {})) + // Dead expressions go before the axis filter, not after: dropping them from + // `expressions` is what makes `ok()` reject them everywhere else. + if (DEAD_ORDER_EXPRS.some(n => c.expressions?.[n] !== undefined)) { + c.expressions = { ...c.expressions } + for (const name of DEAD_ORDER_EXPRS) delete c.expressions[name] + } + const exprNames = new Set(Object.keys(c.expressions || {})) const ok = (col) => validCols.has(col) || exprNames.has(col) if (c.columns) c.columns = c.columns.filter(col => col == null || ok(col)) if (c.group_by) c.group_by = c.group_by.filter(ok) @@ -30,105 +36,16 @@ function cleanLayout(cfg, validCols) { return c } -// Ordering for the pf_bucket and pf_segment column groups, as Perspective -// expression columns. +// Expression columns this view used to manage, back when the pf_bucket and +// pf_segment ordering prefix was computed in the pivot rather than stored in +// pf.log.label. // -// Perspective orders column groups by the value string, and SortDir's `col asc` / -// `col desc` only reverses that — so Prior Year → Plan → Actual → Forecast is -// expressible as neither, being alphabetical in neither direction. The order has -// to be part of the value, as a "01 · " prefix. -// -// Done as expressions rather than in SQL. The prefix is a pivot-ordering concern, -// and computing it server-side put it in every other reader too: the change log -// and the bridge's basis list would both show "04 · Forecast". As expressions it -// stays in the pivot, lives in ViewConfig so it travels with saved layouts, and -// reordering takes effect immediately instead of needing a reload. -// -// Syntax per rust/perspective-client/src/rust/config/expressions.rs: columns in -// double quotes, string literals in single, if/else with braces. The ordered -// label is emitted as a literal rather than built with concat() — fewer moving -// parts, and the mapping is known here anyway. -// pf_ prefixed like every other column the server synthesises, so they sort -// beside pf_bucket and pf_segment in the column list and cannot be mistaken for -// source data. "Bucket" and "Segment" were the first choice and are too close to -// the real thing -- segment_new is an actual column on this source -- and an -// expression named after an existing column would shadow or reject rather than -// merely confuse. -const ORDER_EXPR_NAMES = { bucket: 'pf_bucket_ord', segment: 'pf_segment_ord' } - -// Removed from any config that still carries them. They are no longer managed -// names, so without this they would sit in saved layouts forever, ordering by a -// rule nothing updates. -const LEGACY_ORDER_EXPR_NAMES = ['Bucket', 'Segment'] - -// ASCII only, and " - " specifically because the source data already reads -// "07 - Dec". A middle dot was the first choice and ExprTK rejects it: its string -// scanner tests each *byte* with -// -// is_valid_string_char(c) = isprint((unsigned char) c) || is_whitespace(c) -// -// and "·" is U+00B7, two bytes 0xC2 0xB7 in UTF-8, of which isprint(0xC2) is -// false in the C locale. The literal fails at that byte and the parser reports -// from the start of it — "Invalid string token: 01". -const ORDER_SEP = ' - ' - -function orderedLabel(ord, label) { - return `${String(ord).padStart(2, '0')}${ORDER_SEP}${label}` -} - -// Same byte rule applies to the label being matched, so a bucket named with -// anything outside printable ASCII cannot appear in an expression at all. Such a -// label is left unordered rather than emitted into an expression that will not -// parse and take the whole column with it. -function isExprSafe(v) { - return /^[\x20-\x7e]*$/.test(String(v)) -} - -function sqlSafe(v) { - return String(v).replace(/'/g, "''") -} - -// Anything unlisted falls through to the raw column.\n// pairs: [[rawValue, ordinal], ...] or [[rawValue, ordinal, displayAs], ...] where -// the third element renames the value as well as ordering it. -function buildOrderExpression(sourceCol, pairs, extra = []) { - const cases = [...pairs, ...extra] - .filter(([label, ord]) => label && ord > 0 && isExprSafe(label)) - .sort((a, b) => a[1] - b[1]) - .map(([label, ord, displayAs]) => - `if ("${sourceCol}" == '${sqlSafe(label)}') { '${sqlSafe(orderedLabel(ord, displayAs || label))}' }`) - if (cases.length === 0) return null - return `${cases.join('\nelse ')}\nelse { "${sourceCol}" }` -} - -// The server's synthetic segment labels. Unordered they sort *first*, not last: -// '(' is 0x28 and digits begin at 0x30, so '(adjustment)' precedes '01 - ...'. -// They get high ordinals instead, and lose the parentheses — those marked the -// value as not-a-real-segment, which the ordinal now does. -const SYNTHETIC_SEGMENTS = [ - ['(unlabeled load)', 98, 'Unlabeled'], - ['(adjustment)', 99, 'Adjustments'], -] - -// Build both expressions from the version's bucket_order and the log's seq values. -function buildOrderExpressions(bucketOrder, logMeta) { - const out = {} - - const buckets = (Array.isArray(bucketOrder) ? bucketOrder : []) - .map((label, i) => [label, i + 1]) - const bucketExpr = buildOrderExpression('pf_bucket', buckets) - if (bucketExpr) out[ORDER_EXPR_NAMES.bucket] = bucketExpr - - // Segments are ordered by pf.log.seq, keyed on the label the pivot shows — - // which is the tag if there is one, else the note, the same precedence the - // server uses to build pf_segment. - const segs = Object.values(logMeta || {}) - .filter(m => m.seq != null && ['baseline', 'reference'].includes(m.operation)) - .map(m => [(m.tag || m.note || '').trim(), m.seq]) - const segExpr = buildOrderExpression('pf_segment', segs, SYNTHETIC_SEGMENTS) - if (segExpr) out[ORDER_EXPR_NAMES.segment] = segExpr - - return out -} +// Stripped by cleanLayout, never created: a layout saved under that scheme still +// names them, and without this they would sit there forever, ordering by a rule +// nothing updates. They have to leave the axes at the same time as the +// expressions themselves -- restore() rejects a config whose group_by or sort +// names a column that no longer exists. +const DEAD_ORDER_EXPRS = ['pf_bucket_ord', 'pf_segment_ord', 'Bucket', 'Segment'] export default function Forecast({ sources = [], sourceId, versions = [], versionId, refreshSources }) { const { dark } = useTheme() @@ -653,11 +570,8 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio }, [sourceId, versionId]) useEffect(() => { refreshLogMeta(versionId) }, [versionId]) - // Reordering on the Baseline page takes effect here without a reload: the - // expressions are rebuilt and the pivot re-renders. - // Re-run when the ordering could have changed on the Baseline page. logMeta is - // the signal, not the source: the function reads the ordering itself. - useEffect(() => { if (tableRef.current) syncOrderExpressions() }, [logMeta, versionId]) + // Relabelling on the Baseline page needs a reload to show here: the label is + // part of the aggregated row, so the pivot cannot re-derive it in place. useEffect(() => { refreshTags(sourceId) }, [sourceId]) // Stream an Arrow IPC endpoint into one buffer, reporting download progress. @@ -804,19 +718,12 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio adoptSplit(cfg.split_full?.length ? cfg.split_full : cfg.split_by, (cfg.split_by || []).length) } - // auto-persist viewer state (formatting, columns, etc.) to the last-used cache - // After the layout, not before: restoring a saved config replaces - // `expressions` wholesale, so expressions applied earlier were being wiped - // by the very next line and the ordering columns never appeared in the - // column list. + // Its own try: the restore above has one, but this sits outside it, and a + // throw here would abort the rest of initViewer silently. try { - await syncOrderExpressions() await ensureRowLabelWidth() } catch (err) { - // Its own try covers only restore(); save() and the builders sit outside - // it, and a throw there would abort the rest of initViewer silently. - console.error('[pf-order] threw', err) - flash(`Column ordering failed: ${err.message || err}`, 'error') + console.error('[pf-layout] threw', err) } if (viewer._pspUpdate) viewer.removeEventListener('perspective-config-update', viewer._pspUpdate) @@ -938,100 +845,6 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio // log's seq values. Merged into the live config rather than replacing // expressions, so anything the user defined themselves survives. // - // Removing an ordering removes its column: leaving a stale Bucket expression - // behind would keep ordering by an order that no longer exists. - // Silent unless localStorage.pf_debug is set, like the rest of the tracing. - // Kept rather than removed: it took several rounds to work out that this ran - // before its inputs existed, and it will be wanted again. - function dbgOrder(msg, extra) { - try { if (!localStorage.getItem('pf_debug')) return } catch { return } - if (extra === undefined) console.log(`[pf-order] ${msg}`) - else console.log(`[pf-order] ${msg}`, extra) - } - - // Fetches its own inputs rather than reading the `versions` prop and the - // `logMeta` state. - // - // Those are populated asynchronously, and this runs from initViewer — which - // finishes well before them on a large load. It was therefore called with - // versionFound: false and logMetaCount: 0 every time, could never build - // anything, and the effect meant to re-run it once the data arrived never - // fired. Two small queries are worth more than the right timing. - async function syncOrderExpressions() { - const viewer = viewerRef.current - if (!viewer) return - - let bucketOrder = null - let entries = [] - try { - const [vers, log] = await Promise.all([ - fetch(`/api/sources/${sourceId}/versions`).then(r => r.ok ? r.json() : []), - fetch(`/api/versions/${versionId}/log`).then(r => r.ok ? r.json() : []), - ]) - bucketOrder = (Array.isArray(vers) ? vers : []) - .find(v => String(v.id) === String(versionId))?.bucket_order ?? null - entries = Array.isArray(log) ? log : [] - } catch (err) { - console.error('[pf-order] could not read the ordering', err) - return - } - - const byId = Object.fromEntries(entries.map(e => [e.id, e])) - const wanted = buildOrderExpressions(bucketOrder, byId) - dbgOrder('inputs', { - bucket_order: bucketOrder, - seqs: entries.filter(e => e.seq != null).map(e => `${e.tag || e.note}=${e.seq}`), - wanted: Object.keys(wanted), - }) - - const { table: _t, ...cfg } = await viewer.save() - dbgOrder('saved config read', { existing: Object.keys(cfg.expressions || {}) }) - const current = cfg.expressions || {} - const managed = [...Object.values(ORDER_EXPR_NAMES), ...LEGACY_ORDER_EXPR_NAMES] - - const next = { ...current } - for (const name of managed) delete next[name] - Object.assign(next, wanted) - - const same = JSON.stringify(next) === JSON.stringify(current) - if (same) { - // Nothing to do is a legitimate outcome, but "nothing was ordered" and - // "the ordering is already applied" look identical from the outside. - if (Object.keys(wanted).length === 0) { - dbgOrder('no ordering to apply', { - bucket_order: version?.bucket_order, - seqs: Object.entries(logMeta || {}) - .filter(([, m]) => m.seq != null) - .map(([id, m]) => `${id}:${m.tag || m.note}=${m.seq}`), - }) - } - else dbgOrder('exit: already applied', Object.keys(next)) - return - } - dbgOrder('applying', Object.keys(next)) - - // An expression the pivot is using cannot simply vanish; drop it from the - // axes first or restore() rejects the config. - const stillValid = (col) => next[col] !== undefined || !managed.includes(col) - try { - await viewer.restore({ - ...cfg, - expressions: next, - columns: (cfg.columns || []).filter(c => c == null || stillValid(c)), - group_by: (cfg.group_by || []).filter(stillValid), - split_by: (cfg.split_by || []).filter(stillValid), - sort: (cfg.sort || []).filter(([c]) => stillValid(c)), - }) - dbgOrder('applied', Object.keys(next)) - } catch (err) { - // Surfaced rather than logged: this failing silently is what made the - // missing Bucket and Segment columns so hard to pin down -- the columns - // simply were not there, with nothing to say why. - console.error('[syncOrderExpressions]', err, { wanted, next }) - flash(`Column ordering failed: ${err.message || err}`, 'error') - } - } - // Size every column to its contents. // // Values fit on their own: draw() calls regular_table.resetAutoSize(), which @@ -1378,15 +1191,11 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio if (cfg.group_by_depth != null) setExpandDepth(cfg.group_by_depth - 1) else if (cfg.expand_depth != null) await applyDepth(cfg.expand_depth) - // restore() replaces `expressions` wholesale, so loading a layout drops the - // ordering columns — and a layout saved before they existed has none to put - // back. Re-applied after every restore that comes from a saved config, which - // is the general form of the fix initViewer already needed. - await syncOrderExpressions() await ensureRowLabelWidth() setActiveLayoutId(layout.id) - // After the sync, so the persisted copy carries the expressions too. + // The persisted copy is taken after the restore, so it carries whatever + // cleanLayout dropped on the way in rather than the stale original. const merged = await captureConfig() await persistLayout(versionId, merged || cfg) }