Uh oh!
There was an error while loading. Please reload this page.
fix(dashboard,report): encode an empty pivot dimension value as JSON null, not a placeholder string - #4275
Merged
Conversation
…null The pivot's bucket ids became JSON tuples in objectstack#5473 / objectstack#5665, because every delimiter tried before that assumed the data would not contain it and each assumption failed on ordinary data. The values fed into those tuples still carried one: `String(row[d] ?? '∅')` spelled an absent value as the ordinary string "∅", so it shared a bucket with a row whose value literally is that character — one bucket, later row overwriting the earlier one, the cell showing a different row's measure and drill-through following the same wrong index. An empty value now encodes as JSON `null`, which no string can spell. The normalization lives in `@object-ui/core` as `pivotDimensionValue`, not at each call site, because a caller-spelled placeholder is what survived the previous fix; `pivotBucketId` widens to `Array<string | null>` to accept it. All bucket keys move together, as the fix requires — a bucket id and the subtotal map keyed by it are built from the same expression, so changing one alone would split the headers while the subtotal map still merged. Dashboard: row id, column id, cell key, `rowTotalById`, `colTotalById`. Report: the single `bucketId` helper feeding all five. The dashboard's column id also stops being a bare string and becomes a one-element tuple through the shared encoder — the last id in the family still built by hand, and why the across axis kept carrying this collision after the row ids were fixed. Display is untouched: these placeholders only ever entered ids, never labels. Fixes#4056 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
marked this pull request as ready for review
August 11, 2026 10:36
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#4056
The last encoding in the pivot family that still relied on "the data will not contain this character". objectstack#5473 / objectstack#5665 (PR #3414) replaced the delimiter-joined bucket ids with
JSON.stringify, because every delimiter tried before it — an empty string, a plain space, a control character — assumed the data would not contain it and each assumption failed on ordinary data. The ids became JSON; the values fed into them did not:An absent dimension value became the ordinary string
"∅"(U+2205), so it shared a bucket with a row whose value literally is that character — one bucket, later row overwriting the earlier one, the cell showing a different row's measure, the overwritten row unreachable, and drill-through following the same wrong index into the wrong records. Per the card this is the assumption being removed, not a defect users hit today.An empty value now encodes as JSON
null, whichJSON.stringifyrenders as a barenullno string can spell.Where the null representation lives
In
@object-ui/core, aspivotDimensionValue(absent ⇒null, everything else ⇒ its string form) — not at each call site. A placeholder spelled by a caller is a placeholder that can collide again, which is exactly how this one survived PR #3414's convergence. This follows the routing ruling on the source thread (objectstack#5666, triage 2026-08-06):pivotBucketIdwidens toArray< string | null >accordingly. That is a widening, so existing callers passingstring[]are unaffected.Together-changed key inventory
The card's radius warning is the crux: a bucket id and the subtotal map keyed by it are built from the same expression, so changing one alone would split the headers while the subtotal map still merged — every column subtotal landing under the wrong header, the "one id, two encodings" shape PR #3414 had just converged away. Every key below moves in this PR:
DatasetWidget.buildPivotridpivotRowId(dims.map(d =String(row[d] ?? '∅')))pivotBucketId(dims.map(d =pivotDimensionValue(row[d])))DatasetWidget.buildPivotcidString(row[colDim] ?? '∅')— a bare string, not through the encoderpivotBucketId([pivotDimensionValue(row[colDim])])DatasetWidget.buildPivotcellIndexpivotCellKey(rid, cid)DatasetWidgetrowTotalByIdridridDatasetWidgetcolTotalByIdcidcidDatasetReportRenderer.bucketIdplugin-reportalready funnelled all five of its keys through one localbucketId, so it is a one-line change.plugin-dashboardhad the column axis on a second, hand-built encoding — the reason the across axis kept carrying this collision after the row ids were fixed — and it now goes through the same shared encoder as a one-element tuple. So the two files converge on the shared helper rather than being parallel-edited.Column ids are opaque lookup keys: consumed only as
Mapkeys and React keys (colTotalById.get(cc.col.id),pivotCellKey(rh.id, cc.col.id)), never parsed back into a value, never displayed, never persisted. Drill-through reads raw values (drillRawRows[index], and the report's ownkey: Row), never the id.Red-first evidence
The five pins were written and run against unmodified source first. Every one failed, for the defect's own reason:
Two of these are worth reading closely. The third is the objectstack#5473 signature verbatim — the null row displaying the other row's measure (
222where111belongs). The second shows both encodings side by side in one log: the row id'["∅"]'(JSON) against the column id'∅'(bare), which is the two-encodings state this PR removes. The report case collapsed a 4-row × 2-column cross-tab to a single row showing the last row's measure and the last subtotal.Controls
formatDimensionValuehas always produced, and a value that literally is the placeholder renders as itself. Pinned verbatim in both renderers' assertions (['—', '111']/['∅', '222']; report row labels—and∅). The placeholders only ever entered ids.$filteris{ region: 'was_null', quarter: 'q1' }, not the placeholder row's record set. Raw drill values are deliberately distinguishable so the assertion names which flat index the cell resolved to.JSON.stringify(values). This control passed before the change and still passes after.check:i18n-keysandcheck:i18n-driftboth green.Reverse verification
Reverting only the two plugin call sites (core's helper left in place, so the failure is the collision and not a compile error) — predicted RED on the collision pins, controls green:
Exactly the five new pins, and only those five. Restored, then re-verified green.
The cross-package type change got its own reverse verification, since a stale
.d.tswould report a false green in either direction. A probe compiled insideplugin-dashboardagainst the rebuilt@object-ui/core:The acceptances prove tsc read the rebuilt declaration rather than a cached one; the rejection proves the element type is enforced downstream, so a caller cannot skip the normalizer. Probe removed after measuring.
Verification
Build closure first (
--filter '< pkg >^...' build, suffix^...= the packages it depends on), then:vitest run packages/plugin-dashboard/ packages/plugin-report/ packages/core/— 118 files, 2040 tests, all passingtype-checkon@object-ui/core,@object-ui/plugin-dashboard,@object-ui/plugin-report— all Doneeslinton the six changed files — 0 errors (31 pre-existing warnings; the one added is ano-explicit-anyon a mock-call assertion, matching the identical pre-existing line in the objectstack#5473 drill test beside it)check:control-bytesgreen, plus a direct scan of the changed files for the bytes the gate does not cover@object-ui/core,@object-ui/plugin-dashboard,@object-ui/plugin-reportGenerated by Claude Code