Skip to content

The chart null-category bucket has two identity collisions: it merges a null group with an empty-string group, and collides with a stored value spelling the label #4508

Description

@yinlianghui

Measured while implementing #4497 (PR #4506) and pinned there as limits; filed unassigned rather than widened into that card. Both are properties of the #4466 doctrine itself, not of the pivot extension — the pivot inherits them.

1. A null group and an empty-string group merge into one bar (pivot branch)

buildChartSeries' pivot branch keys buckets by String(xRaw ?? ''), so a stored null and a stored '' land in the same bucket and draw one bar. The bar takes its label from whichever row created the bucket, so with a null row first it renders (None) — and the drill for the segment sourced from the '' row then finds nothing:

buildChartSeries(
  [{ status: null, priority: 'Low', n: 3 },
   { status: '',   priority: 'High', n: 5 }],
  ['status', 'priority'], ['n'],
).data
// [{ status: '(None)', Low: 3, High: 5 }]   ← one bar, two sources

findChartSeriesRow(rows, ['status','priority'], ['n'], '(None)', 'Low')   // 0
findChartSeriesRow(rows, ['status','priority'], ['n'], '(None)', 'High')  // -1  ← dead

handleChartDrill returns on -1, so this is a no-op click, never a drill into the wrong records — and pre-#4497 the whole bar was invisible, so it is strictly more affordance than before, not a regression. It is still an inconsistency: a bar labelled "no value" carries a segment belonging to a group whose value is the empty string.

Widening findChartSeriesRow to match '' rows against the null label was considered and rejected in #4497: it would make a bar labelled (None) drill to a raw row whose stored value is '', i.e. filter records by a value the label does not name — trading a dead click for a wrong one.

The pivot table answers the same question differently, which is worth deciding together: buildPivot (plugin-dashboard) gives a null dimension value its own bucket id, distinct from the literal placeholder character, and labels it with an em-dash. See DatasetWidget.test.tsx, "buildPivot keeps a null COLUMN-dimension value apart from the literal placeholder".

2. A stored value spelling the label collides with the bucket (both branches)

A row whose stored category is the label string keeps its own bucket (its key is '(None)', not ''), so two bars carry the same axis text and the click resolves to the first:

buildChartSeries(
  [{ status: '(None)', priority: 'High', n: 1 },
   { status: null,     priority: 'High', n: 2 }],
  ['status', 'priority'], ['n'],
).data
// [{ status: '(None)', High: 1 }, { status: '(None)', High: 2 }]

findChartSeriesRow(rows, ['status','priority'], ['n'], '(None)', 'High')  // 0 — the literal row

This one can produce a wrong drill: clicking the null bucket's bar resolves to the literal-valued row and filters on it. It is inherited unchanged from the single-dimension branch, where #4466 (PR #4498) shipped exactly this trade, and it applies to every localized label too ((未指定) and the other nine packs).

Dormant in practice: it needs a stored first-dimension value byte-equal to the active locale's bucket label.

Why they are one issue

Both are the same missing property — the bucket has no identity distinct from the display string — and any fix touches the same two functions as a pair (buildChartSeries writes the identity, findChartSeriesRow reads it back). A sentinel bucket identity carried alongside the label, rather than the label serving as its own key, would answer both; that is a doctrine-level change to #4466's shape and wants its own ruling.

Both are pinned as measured limits (not as correct) in packages/core/src/utils/chart-series.nullCategory.test.ts, describe "findChartSeriesRow — the measured limits of the bucket label (objectui#4497)", so a future change to either has to face them.


Generated by Claude Code

Activity

  1. added theissue type on Aug 13, 2026
  2. hotlong commented on Aug 13, 2026

    @hotlong
    Contributor

    Triage: needs-user-decision — the filer explicitly asks for a ruling, and it's warranted: a real fix means giving chart buckets an identity distinct from their display label, which is a doctrine-level change to #4466's shape (shared by every chart consumer, currently pinned as "measured limits, not correct" in chart-series.nullCategory.test.ts).

    Four-axis read:

    Recommendation: approve the sentinel-identity direction (distinct bucket key from display label, matching what buildPivot's COLUMN-dimension handling already does per the filer's cross-reference) and dispatch as one card touching both functions plus their pinned tests.


    Generated by Claude Code

  3. hotlong commented on Aug 14, 2026

    @hotlong
    Contributor

    Maintainer ruling (2026-08-14, verbatim: 「同意」, approving item 5 of the six-item ruling list: 「objectui#4508 批 sentinel-identity?」): approved — sentinel bucket identity carried alongside the display label, aligning buildChartSeries (writes identity) and findChartSeriesRow (reads it) with the table branch's existing distinct-bucket-id form. One combined dispatch covering both collisions (null+'' merge; stored literal "(None)" vs the null bucket). needs-user-decision → pm:queue, target:v17; enters the v17 bug-focus Seat B (objectstack-ai/objectstack#8668).


    Generated by Claude Code

  4. self-assigned this
    on Aug 15, 2026
  5. yinlianghui commented on Aug 15, 2026

    @yinlianghui
    CollaboratorAuthor

    Claim: PM loop round 6
    Session: session_01RnQd8iMMUwXQEV1crFmQiQ
    Branch: claude/issue-4508-chart-bucket-identity
    Worktree: objectui-issue-4508
    Domain: repo:objectui
    File surface: packages/core/src/utils/chart-series* (buildChartSeries + findChartSeriesRow + the pinned-limits test) + the drill consumers that read the row back (per re-derivation; likely plugin-dashboard/plugin-charts drill path) (stop on breach; explain in the report)
    Container & model: M, mode:subagent, model: opus
    Serial constraints cleared: in-flight #4640 also touches packages/core but in the evaluator region (evalRowPredicate entry — disjoint files, mutually named); #4648 holds parity/registry surfaces, #4644 holds metadata-admin field designer, #8830 is objectstack-side — all disjoint; PR #4670 (merged) touched core/predicate-fields — work from post-merge origin/main; no open dev PR touches chart-series (checked this round).


    Generated by Claude Code

  6. yinlianghui commented on Aug 15, 2026

    @yinlianghui
    CollaboratorAuthor
    {
      "issue": 4508,
      "status": "done",
      "branch": "claude/issue-4508-chart-bucket-identity",
      "pr": "https://github.com/objectstack-ai/objectui/pull/4677",
      "premise_still_valid": true,
      "summary": "Both collisions fixed as one change, per the 2026-08-14 「同意」 ruling. `chartBucketId` (core) is the bucket identity — literally the encoder `buildPivot` already keys the pivot TABLE with (`pivotBucketId` over `pivotDimensionValue`), so the chart and table stop answering one question two ways; the pivot branch now buckets by it instead of by `String(xRaw ?? '')`, which is what makes null and '' two groups again. Where two DISTINCT buckets still paint the same axis text (the null bucket beside a record storing the literal label), the emitted row carries that identity under `CHART_BUCKET_ID_KEY` and `findChartSeriesRow` takes it back as an authoritative `options.bucketId`; it is written exactly and only there, so an ordinary chart's rows are still returned by identity and no renderer-internal key reaches the `chart` schema's `data`. Plumbing: the drill event gains `categoryId` via `ChartSegmentClickEvent`, now declared once in core instead of as three inline literals (AdvancedChartImpl / ChartRenderer / ObjectChart), read off the clicked row on the cartesian, pie and funnel paths and consumed by `DatasetWidget.handleChartDrill`. Two measurements shaped the design and are worth the PM's attention: recharts builds a pie sector's `payload` as a SPREAD COPY of the row, so the identity had to be an ordinary enumerable property (a symbol or non-enumerable one would read as 'no identity' with every pure test still green); and recharts 3's chart-level onClick sends a `MouseHandlerDataParam` with no row at all, so the cartesian arm reads its row out of our own `data` by `activeTooltipIndex`. Also flipped, as a consequence of ruling item 1: a null category no longer reads as `''` too — no producer of this lookup's `category` writes that spelling, while `''` IS a genuine empty-string group's axis text, so the tolerance was handing that group's bar another group's records.",
      "tests": "All at f3e14d59a (final commit; gate union re-run after it). `pnpm exec vitest run packages/core/ packages/plugin-charts/ packages/plugin-dashboard/` -> 'Test Files 170 passed (170) / Tests 2463 passed (2463)'. `turbo run type-check lint` for the three packages with the dependency closure built -> 'Tasks: 19 successful, 19 total', 0 lint errors; type-check confirmed executed ('> tsc --noEmit && tsc -p tsconfig.test.json'). Gate scripts all OK: changeset-fixed, changeset-no-major, changeset-presence, control-bytes, spec-symbol-derivation, phantom-dependencies, i18n-call-site-keys, i18n-en-drift, lint-coverage, type-check-coverage, skills-paths. New coverage at three layers: the transform (chart-series.nullCategory.test.ts, rewritten limits describe + a new identity-contract describe), the chart library seam (AdvancedChartImpl.bucketIdentity.test.tsx — clicks a REAL pie sector, which is the only way the spread-copy hazard is observable), and the consumer (DatasetWidget.chartBucketIdentity.test.tsx — asserts which records the drawer opens on). REVERSE VERIFICATION, directions predicted before running, split into two legs for attribution: ablating the WRITER (pivot bucket key + identity write) predicted both collisions' bucketing plus every identity-carrier assertion red and the reader-only cases green -> observed 9 failed / 39 passed, exactly the predicted set; ablating the READER (exact display matching) predicted only the empty-string cases red -> observed 4 failed / 44 passed, the predicted set. Restored via path-scoped `git checkout` of the commit sha between legs (never a stash); final numbers are from the restored tree. CROSS-PACKAGE TYPE reverse check: renaming ev.categoryId to a non-existent field in DatasetWidget yields 'error TS2339: Property categoryIdNoSuchField does not exist on type ChartSegmentClickEvent' — the error names the type from the freshly built core .d.ts, proving plugin-dashboard checked against the rebuilt declaration, not a cached one. No ablation was run against dist/ (no dogfood leg applies to this card). Consumer sweep direction: downstream — plugin-charts and plugin-dashboard are the consumers of core's changed contract, and both are in the suites above.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #4672: AdvancedChartImpl's cartesian click reads `activePayload`, a recharts 2 field that recharts 3 does not send (its MouseHandlerDataParam has activeIndex/activeTooltipIndex/activeLabel/activeDataKey only), so `series` and `value` are ALWAYS undefined — which kills the pivoted dataset drill outright, since findChartSeriesRow's pivot arm requires the series key. Independent of this PR and pre-existing; not fixed here because the correct value for activeDataKey under a shared cartesian tooltip is not pinned by existing evidence (in-place exemption condition 2 unmet). Worth PM attention as it may outrank this card in user impact.",
        "filed as #4673: buildChartSeries' pivot branch drops a null SECOND-dimension group's series entirely (the `gId !== ''` gate), while still writing its measure into the bucket — the value is in the data and on no mark. The #4466 defect one dimension over; needs its own ruling because a series key is also a row key (collision risk with measure names), so it is not mechanical."
      ]
    }

    Notes for the PM, outside the JSON:


    Generated by Claude Code


    Generated by Claude Code

  7. yinlianghui commented on Aug 15, 2026

    @yinlianghui
    CollaboratorAuthor

    ACCEPT — PR #4677 (reviewer of record; readings taken on GitHub at head f3e14d59a, which matches the gate-union HEAD in the report and PR body).

    CI on this head is converging — ready + auto-merge flips when Lint / Type Check / Test shards conclude green (idempotent timer armed). This card leaves the target:v17 board at close. Note for #4673's future dev: that card is queued behind THIS merge (same pivot branch of chart-series.ts).


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

target:v17v17 发布窗口工作集(GA 前排查 2026-08-04)

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions