Repository navigation
fix(observable-plot): avoid implicit scatter stacking - #522
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dffbf465d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasRequiredGroup = (mode !== 'normalize' && mode !== 'center') || Boolean(enc.color?.field); | ||
| if (enabled && hasRequiredGroup && explicitChannel && explicitlyStackableMarks.has(markType)) { |
There was a problem hiding this comment.
Preserve normalized stacks when color is absent
When defaultAggregated is false and a bar or area chart has repeated values for a positional category, selecting normalize or center without a color field now makes this condition reject stacking. The subsequent suppressImplicitStack call converts the measure to y1: 0, y2: field (or the X equivalent), so the records overlap from zero instead of forming the requested normalized/centered stack. A color channel is not required for Plot.stackX/stackY to stack raw records by the opposing positional channel; this gate needs to distinguish the pre-aggregated singleton case from raw data rather than using color as the requirement.
Useful? React with 👍 / 👎.
Summary
stackproperty is absentnone,zero,normalize, andcentersemantics without passing Observable Plot's unsupportedoffset: "zero"Reproduction and root cause
A minimal point spec with quantitative X/Y and nominal color reproduced the bug locally. Graphic Walker's
stack/zeromodes intentionally omit the Vega-Litestackproperty, but the old Observable Plot converter treatedundefinedas enabled because it only checked that the value was notnullorfalse. The failing regression test proved that the point mark was sent throughPlot.stackYeven though Vega-Lite emitted no stack transform.The converter now resolves stacking from the mark type, quantitative non-binned channels, and explicit stack value:
Plot.stackX2/stackY2for the upper endpointstackX/stackYVega-Lite and Observable Plot parity
The implementation was checked against installed
vega-lite@5.16.3and@observablehq/plot@0.6.16behavior, including stackable/default-stack marks, X-before-Y precedence,stackX2/stackY2, implicit-stack suppression, and Plot rejectingoffset: "zero".A separate runtime comparison used repeated categories (
A: 1 + 2,B: 4 + 5) across bar/area, raw/aggregate inputs, and absent/zero/truemodes (12 cases). Vega runtime and Plot both produced category endpointsA = 3,B = 9; explicit Plot stacking was channel-for-channel identical to each mark's implicit stack, so the change does not double-accumulate ungrouped bar/area values.Validation
@kanaries/graphic-walkerJest: 33 suites, 363 tests, 4 snapshots passednode_modules/.bin/tsc -p packages/graphic-walker/tsconfig.json --noEmityarn workspace @kanaries/graphic-walker buildgit diff --checkIndependent review
The final exact diff was reviewed in a fresh, read-only Claude Code session using explicit model
claude-opus-5:da84ef5b-5e27-43bb-84b3-d3b3535dff14canonicalModel:claude-opus-5APPROVEwith no blocking findingsAn earlier independent Opus 5 code review identified the no-color normalize/center regression; that finding was reproduced with four failing tests, fixed, fully revalidated, and then submitted to the fresh final delta review above.
Commit:
9dffbf465d05e0b81db05de33844a2a2b097043bFixes #470