Skip to content

fix: DH-21820: Sanitize pivot builder config, skip missing columns#1394

Open
vbabich wants to merge 7 commits into
deephaven:mainfrom
vbabich:vlad-DH-21820
Open

fix: DH-21820: Sanitize pivot builder config, skip missing columns#1394
vbabich wants to merge 7 commits into
deephaven:mainfrom
vbabich:vlad-DH-21820

Conversation

@vbabich

@vbabich vbabich commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes DH-21820: a saved pivot/rollup config that references columns which no
longer exist rendered a broken, empty grid on restart (no error, no
notification). We now sanitize stale column references before they reach the
host/service, keep the raw config so the user can still see and fix them.

What changed

  • Rollup path — sanitize groupingColumns and rollup.aggregations
    before rollupConfig is set (drop missing columns and operations invalid
    for their column type). If all grouping columns are dropped, fall back to a
    flat source instead of an empty rollup.
  • Totals (Aggregate Values) path — sanitize totals.operationMap before
    getTotalsTable() (existence + operation/type validity). Diff the
    sanitized value against what was applied to avoid RPC churn/flicker.
  • Pivot path — extend the existing aggregation sanitization to also filter
    rowKeys/columnKeys. When all keys are stale, revert to a flat table if no
    aggregations survive, or keep a flat summary if they do.
  • Notification — new PIVOT_BUILDER_STALE_COLUMNS event + stale references are logged (log.warn) and shown struck-through in the sidebar.
  • Hydration fix — config is applied during model hydration, before any
    listener can attach, so the middleware also reads a synchronous
    staleColumnReport snapshot off the proxy once the model is available.
  • Raw config (storedRollup/storedTotals/lastIntent) is always preserved;
    only the value sent to the host/service is sanitized.

Testing

  • Full pivot-builder JS suite: 12 suites, 139 tests passing, 0 TS errors.
  • New/extended: findStaleColumnRefs.test.ts, pivotBuilderModelApply.test.ts
    (incl. the hydration/no-listener repro), buildPivotAggregationsMap.test.ts.
  • Verified live: rollup-grouping, rollup-aggregations, and plain-totals failure
    modes (drop a referenced column, restart) now sanitize + notify instead of
    rendering broken.

@vbabich
vbabich marked this pull request as ready for review July 23, 2026 20:16
@vbabich
vbabich requested a review from Copilot July 23, 2026 20:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Sanitizes stale pivot, rollup, and totals column references to prevent broken grids after schema changes.

Changes:

  • Adds schema-aware sanitization and fallback behavior.
  • Reports stale configurations through events and toast notifications.
  • Marks stale sidebar entries and expands test coverage.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
usePivotBuilderMiddlewareCore.tsx Displays stale-column notifications.
PivotConfigSection.tsx Identifies stale card entries.
pivotConfig/rows/rowParts.tsx Adds stale label styling support.
pivotConfig/rows/rowParts.test.tsx Tests stale labels.
pivotConfig/rows/ColumnRow.tsx Propagates stale state.
pivotConfig/rows/ColumnRow.test.tsx Tests stale column rows.
pivotConfig/rows/aggregateRows.tsx Marks missing aggregate columns.
pivotConfig/rows/aggregateRows.test.tsx Tests aggregate stale states.
pivotBuilderModelApply.test.ts Tests sanitization, fallback, and notifications.
pivotBuilderModel.ts Implements sanitization and stale reporting.
PivotBuilder.scss Styles stale column names.
findStaleColumnRefs.test.ts Tests stale-reference detection and sanitizers.
CreatePivotPage.tsx Uses a null-prototype column map.
buildPivotAggregationsMap.test.ts Tests sanitized pivot requests.
DH-21820-pivot-builder-stale-column-notifications.md Documents the design and implementation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugins/pivot-builder/src/js/src/pivotBuilderModel.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

plugins/pivot-builder/src/js/src/pivotBuilderModel.ts:1384

  • Use the applied pivot state here rather than the raw intent. With UI re-derivation, config.pivot/lastIntent.pivot can be null while effective.pivot has set current; a later UI state that derives totals or flat then skips this clear and leaves the old pivot model active. Checking current correctly reflects whether a pivot must be removed.
    if (lastIntent.pivot != null) {

Comment thread plugins/pivot-builder/src/js/src/pivotBuilderModel.ts
Comment thread plugins/pivot-builder/src/js/src/makePivotModelTransform.ts Outdated
Comment thread plugins/pivot-builder/src/js/src/CreatePivotPage.tsx Outdated
Comment thread plugins/pivot-builder/src/js/src/usePivotBuilderMiddlewareCore.tsx
Comment thread plugins/pivot-builder/src/js/src/pivotConfig/rows/rowParts.tsx
@vbabich
vbabich requested a review from mofojed July 24, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants