From 5feb28eb76a142892d144a1906b1878059e97c15 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Mon, 28 Sep 2026 22:40:00 -0400 Subject: [PATCH] Keep the pivot's pf_ columns in the slice, and log the panel's expected change The ledger filtered on every column of the loaded table, while the request kept only pf_segment and pf_bucket and stripped pf_iter, pf_note, pf_op, pf_user and pf_logid. Under the default layout, grouped by pf_iter, a click on a customer's `scale` row totalled the scale rows and wrote to every row the customer has -- on GRIFFIN, $4.5M shown against $9.7M written. They now travel: pf_iter / pf_user / pf_logid compared directly, pf_note / pf_op resolved to log ids as pf_segment / pf_bucket are, a null note as IS NULL. They narrow a slice but do not make one selective on their own. Each scale also carries `expected`, the Result line as a change, and `plug`, both kept in the log's params beside `resolved` so a panel/server disagreement can be found by reading it. Nothing compares them yet. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 4 ++- lib/sql_generator.js | 43 ++++++++++++++++++++++------ routes/operations.js | 13 +++++++-- ui/src/components/OperationPanel.jsx | 22 ++++++++++++-- ui/src/views/Forecast.jsx | 25 ++++++++++++---- 5 files changed, 85 insertions(+), 22 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 593c638..effafa2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -680,7 +680,9 @@ Turning a region back into slices re-derives, per cell, the same filters Perspec **Selection highlight.** The datagrid highlights whatever sits in its own `model._selection_state.selected_areas`, and wipes that list on every mousedown — so a multi-slice selection built up over several ctrl-clicks would only ever show the last cell. `Forecast.jsx` keeps `areasRef`, a `sliceKey -> rectangles` map parallel to `slices`, and an effect pushes the full set back and redraws after every change. Deselecting anywhere (ctrl-click, the panel's ×, Clear selection) prunes the map by live slice key, so the grid and the panel can't disagree. -`pf_iter` is not a col_meta column, so it is stripped when a slice is built: two cells differing only by iter band produce the same effective slice. Duplicates are collapsed before the request — without that, `apply_mode: each` would apply the same change twice. +**The `pf_` columns the pivot shows are slice columns too.** `pf_segment`, `pf_bucket`, `pf_note` and `pf_op` resolve to log ids (`COMPUTED_SLICE_COLS`); `pf_iter`, `pf_user` and `pf_logid` are forecast-table columns compared directly (`TABLE_SLICE_COLS`). They used to be stripped — `pf_iter` on the theory that two cells differing only by iter band are the same slice — while the ledger filtered on them, so under the default layout (grouped by `pf_iter`) a click on a customer's `scale` row totalled the scale rows and wrote to every row the customer has. They narrow a slice but do not make one selective on their own: `pf_iter = scale` alone is every adjustment in the version, and `assertSelective` still wants a dimension, date, `pf_segment` or `pf_bucket`. Duplicates are collapsed before the request — without that, `apply_mode: each` would apply the same change twice. + +A scale also carries `expected` — the panel's Result line as a change (`value_incr`, `units_incr`, `price`, `basis`) — which the log keeps beside `resolved` so a disagreement between panel and server can be found by reading it. Nothing compares them yet. **A blank dimension value is a value.** Both blanks occur in this data and they are different rows — on `fc_osm_skinny_34`, `partgroup` is null on 51,964 rows diff --git a/lib/sql_generator.js b/lib/sql_generator.js index e985ec7..be4cb53 100644 --- a/lib/sql_generator.js +++ b/lib/sql_generator.js @@ -541,7 +541,25 @@ function applyTokens(sql, tokens) { // Without this they were dropped from the slice, and clicking a single bucket's // cell scaled every bucket at that dimension intersection while the panel showed // only the one clicked. -const COMPUTED_SLICE_COLS = { pf_segment: SEGMENT_EXPR, pf_bucket: BUCKET_EXPR }; +// +// pf_note and pf_op are the same kind of column -- the pivot shows them, from +// the log entry -- and used to be refused, which the client pre-empted by +// stripping them. The ledger did not strip them, so a pivot grouped by one +// totalled a narrower set of rows than the operation then wrote. +const COMPUTED_SLICE_COLS = { + pf_segment: SEGMENT_EXPR, + pf_bucket: BUCKET_EXPR, + pf_note: NOTE_EXPR, + pf_op: 'l.operation', +}; + +// pf_ columns that are real columns of the forecast table, compared directly. +// pf_iter was stripped by the client and skipped here, on the reasoning that +// two cells differing only by iter band are the same slice. They are not to the +// ledger, which filtered on it: with the default layout grouped by pf_iter, a +// click on a customer's `scale` row showed the scale rows' total and wrote to +// every row the customer has. +const TABLE_SLICE_COLS = new Set(['pf_iter', 'pf_user', 'pf_logid']); function computedSlicePredicate(col, val, versionId) { if (versionId == null) { @@ -549,12 +567,21 @@ function computedSlicePredicate(col, val, versionId) { err.status = 500; throw err; } - const vals = (Array.isArray(val) ? val : [val]).map(v => `'${esc(v)}'`).join(', '); + // a blank is a value here too -- a load has no note -- and `IN (NULL)` is + // never true, so the null arm is ORed on as buildWhere does + const list = Array.isArray(val) ? val : [val]; + const nulls = list.some(v => v === null); + const vals = list.filter(v => v !== null).map(v => `'${esc(v)}'`); + const expr = COMPUTED_SLICE_COLS[col]; + const inList = vals.length ? `(${expr}) IN (${vals.join(', ')})` : null; + const test = nulls && inList ? `((${expr}) IS NULL OR ${inList})` + : nulls ? `(${expr}) IS NULL` + : inList; return `pf_logid IN ( SELECT l.id FROM pf.log l WHERE l.version_id = ${parseInt(versionId)} - AND ${COMPUTED_SLICE_COLS[col]} IN (${vals}) + AND ${test} )`; } @@ -571,15 +598,13 @@ function buildWhere(slice, dimCols, versionId) { } // A pf_ key this does not understand is refused rather than skipped. // Skipping is how a selection silently widened: the operation ran against - // everything the dropped key would have excluded. pf_iter is the one - // exception -- the client strips it deliberately, since two cells that - // differ only by iter band are the same slice. - if (col.startsWith('pf_') && col !== 'pf_iter') { + // everything the dropped key would have excluded. + if (col.startsWith('pf_') && !TABLE_SLICE_COLS.has(col)) { const err = new Error(`Slice names ${col}, which cannot be filtered on`); err.status = 400; throw err; } - if (!allowed.has(col)) continue; + if (!allowed.has(col) && !TABLE_SLICE_COLS.has(col)) continue; // A blank dimension value is a value. Both blanks exist in this data and // they are different rows -- on fc_osm_skinny_34, partgroup is null on // 51,964 and '' on 111,696 -- so a null has to become IS NULL. It used @@ -821,6 +846,6 @@ function esc(val) { return String(val).replace(/'/g, "''"); } -module.exports = { generateSQL, grainOf, COMPUTED_SLICE_COLS, buildScopeClause, buildTerritoryClause, +module.exports = { generateSQL, grainOf, COMPUTED_SLICE_COLS, TABLE_SLICE_COLS, buildScopeClause, buildTerritoryClause, SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, LABEL_GROUP_COLS, ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET, dateGroupsOf, dimPeriodMapOf, dimPeriodJoins, applyTokens, buildWhere, buildWhereAny, buildExcludeClause, buildExcludePredicate, buildSetClause, buildFilterClause, esc }; diff --git a/routes/operations.js b/routes/operations.js index d78ef8f..d1ee846 100644 --- a/routes/operations.js +++ b/routes/operations.js @@ -1,6 +1,6 @@ const express = require('express'); const { tableFromArrays, tableToIPC } = require('apache-arrow'); -const { applyTokens, buildWhere, buildWhereAny, COMPUTED_SLICE_COLS, buildScopeClause, buildTerritoryClause, buildExcludeClause, buildExcludePredicate, buildSetClause, dateGroupsOf, dimPeriodMapOf, esc, +const { applyTokens, buildWhere, buildWhereAny, COMPUTED_SLICE_COLS, TABLE_SLICE_COLS, buildScopeClause, buildTerritoryClause, buildExcludeClause, buildExcludePredicate, buildSetClause, dateGroupsOf, dimPeriodMapOf, esc, SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET } = require('../lib/sql_generator'); const { sessionUser, sessionTerritory, requireAdmin } = require('../lib/auth'); @@ -133,7 +133,10 @@ module.exports = function(pool) { // An empty slice passes only under a scope (see normalizeSlices), which // buildScopeClause refuses to let reduce to nothing. function assertSelective(slices, ctx, scope) { - const allowed = new Set([...ctx.filterCols, ...Object.keys(COMPUTED_SLICE_COLS)]); + // pf_iter, pf_note, pf_op, pf_user and pf_logid narrow a slice once it + // is selective, but do not make it so: they were only just accepted at + // all, and `pf_iter = scale` alone is every adjustment in the version + const allowed = new Set([...ctx.filterCols, 'pf_segment', 'pf_bucket']); const scoped = Array.isArray(scope) && scope.length > 0; slices.forEach((sl, i) => { if (scoped && Object.keys(sl).length === 0) return; @@ -218,7 +221,11 @@ module.exports = function(pool) { // echo back what the caller asked for, for the audit log function pickIntent(body) { const keys = ['mode', 'target_basis', 'value_incr', 'units_incr', 'value_pct', 'units_pct', 'pct', - 'target_value', 'target_units', 'target_price', 'scope']; + 'target_value', 'target_units', 'target_price', 'scope', 'plug', + // What the panel's Result line promised, kept beside + // `resolved` so a disagreement between the two can be + // found by reading the log. Not checked -- yet. + 'expected']; const out = {}; for (const k of keys) if (body[k] !== undefined && body[k] !== null && body[k] !== '') out[k] = body[k]; return out; diff --git a/ui/src/components/OperationPanel.jsx b/ui/src/components/OperationPanel.jsx index b174325..1868751 100644 --- a/ui/src/components/OperationPanel.jsx +++ b/ui/src/components/OperationPanel.jsx @@ -583,6 +583,8 @@ function TagInput({ value, onChange, tags = [], className }) { // here -- so flipping the plug brings it straight back. Forecast.jsx builds // the request from the same function, so the panel and the write agree. // Returns the inputs that count, and whether the plug is the one choosing. +const round2 = (x) => Math.round(x * 100) / 100 + const hasNumber = (edit) => edit?.raw != null && edit.raw !== '' && isFinite(parseFloat(edit.raw)) export function effectiveScaleInputs(inputs = {}, plug = 'volume') { @@ -601,7 +603,7 @@ export function effectiveScaleInputs(inputs = {}, plug = 'volume') { let editSeq = 0 const stamp = () => ++editSeq -function ScaleLedger({ currentTotals, scaleInputs: rawInputs, setScaleInputs, scalePlug, setScalePlug, targetBasis, setTargetBasis, logMeta = {}, multi, applyMode, settingsExtra }) { +function ScaleLedger({ currentTotals, scaleInputs: rawInputs, setScaleInputs, scaleExpectedRef, scalePlug, setScalePlug, targetBasis, setTargetBasis, logMeta = {}, multi, applyMode, settingsExtra }) { // everything below reads the inputs that count; only the setters touch the raw ones const { inputs: scaleInputs, choosing: plugChoosing } = effectiveScaleInputs(rawInputs, scalePlug) const valueCol = currentTotals?.valueCol @@ -822,6 +824,20 @@ function ScaleLedger({ currentTotals, scaleInputs: rawInputs, setScaleInputs, sc } })() + // The change the Result line promises, handed to the request as `expected`. + // Measured on the same basis as the ledger, so it compares directly with + // `resolved` in the log entry. Under 'each' it is the combined figure across + // the slices, which is what the panel shows -- not a per-slice amount. + if (scaleExpectedRef) { + scaleExpectedRef.current = outcome ? { + value_incr: round2(outcome.value - (basisOf('value') ?? 0)), + units_incr: round2(outcome.units - (basisOf('units') ?? 0)), + price: outcome.price == null ? null : Math.round(outcome.price * 1e6) / 1e6, + basis: onTotal ? 'selected' : 'adjustable', + ...(perSlice ? { combined_across_slices: true } : {}), + } : null + } + // The same three readings for a measure nobody typed into, taken from where // the edit actually lands it. Scaling dollars with price holding moves qty, // and leaving that column blank until the Result row meant the number you had @@ -1464,7 +1480,7 @@ export default function OperationPanel({ applyMode, setApplyMode, currentTotals, activeOp, setActiveOp, - scaleInputs, setScaleInputs, + scaleInputs, setScaleInputs, scaleExpectedRef, scalePlug, setScalePlug, targetBasis, setTargetBasis, opTag, setOpTag, knownTags = [], logMeta = {}, @@ -1520,7 +1536,7 @@ export default function OperationPanel({ {activeOp === 'scale' && ( v.trim())) body = { ...body, note: recodeNote || undefined, set } @@ -2154,7 +2167,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio applyMode, setApplyMode, currentTotals, activeOp, setActiveOp, - scaleInputs, setScaleInputs, + scaleInputs, setScaleInputs, scaleExpectedRef, scalePlug, setScalePlug, targetBasis, setTargetBasis, opTag, setOpTag, knownTags, logMeta,