Skip to content

fix(vegalite): Waterfall axis titles: display names, no __wf_lead - #79

Merged
Chenglong-MS merged 1 commit into
microsoft:devfrom
zl190:fix/waterfall-titles
Jul 31, 2026
Merged

fix(vegalite): Waterfall axis titles: display names, no __wf_lead#79
Chenglong-MS merged 1 commit into
microsoft:devfrom
zl190:fix/waterfall-titles

Conversation

@zl190

@zl190 zl190 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • On a Vega-Lite Waterfall, field_display_names was ignored on both axes, and the internal window-transform column surfaced in the rendered x-axis title as week, __wf_lead.
  • Root cause: instantiate() rebuilds its own encoding objects and drops the titles the assembler had already resolved, leaving the shared x scale untitled, so Vega-Lite derived a title from every untitled field on that scale and concatenated them.
  • Fix carries those resolved titles into the layers, which settles the shared scale and leaves no way for an internal column to name an axis.

Context

Fixes #65, fixes #66.

#66 looks like an accidental re-post of #65: same text nine minutes apart, only the sample field name differs. Referencing both so neither is left dangling; happy to drop one if you would rather keep them separate.

Both were filed against 0.3.0. The defect still reproduces on dev @ c73d597:

x axis title y axis title
Waterfall week, __wf_lead wsu_change
Line Chart, identical input Week (Mon, JST) WSU weekly change

Changes

packages/flint-js/src/vegalite/templates/waterfall.ts (+16 / −1):

  • Derive xTitle / yTitle from ctx.resolvedEncodings, which buildVLEncodings has already populated with any field_display_names override (vegalite/assemble.ts:1021).
  • Give the shared xEnc an explicit title. This is what stops the concatenation: an explicit title on the primary channel settles the title for the whole shared scale.
  • Use yTitle on the bar layer instead of the hardcoded raw yField.

packages/flint-js/tests/waterfall-titles.test.ts (new, 4 cases).

Review focus

  1. Is the explicit-title approach the one you want? The alternative suggested in the issue, title: null on the internal bindings, blanks the axis instead: Vega-Lite merges titles across every layer on a shared scale and an explicit null anywhere in that set wins. Compiling that variant blanks the axis (see Test plan); if you would rather suppress differently, say so and I will redo it.
  2. Fallback behavior when no display names are given. The titles become the raw field names, explicitly set rather than derived. Rendered output is unchanged, but the emitted spec now carries title where it previously had none — tell me if that matters for any downstream consumer.
  3. The template-local fix vs. an assembler-level guard. See the sweep note below for why I went local.

Test plan

  • npx vitest run tests/waterfall-titles.test.ts — 4 pass. Reverting the template change makes 3 of them fail; the 4th is a forward guard that passes either way by design.
  • npm run test:js — 652 pass across 39 files, up from 648 across 38 on dev @ c73d597. The delta is this PR's new file; no existing test needed changing.
  • E2E claim, checked at the rendered axis rather than the spec: compiled the assembled spec with vl.compile and read the axis titles out of the resulting Vega spec. Before: x week, __wf_lead, y wsu_change. After: x Week (Mon, JST), y WSU weekly change.
  • Swept the whole bundled corpus for internal columns reaching an axis title: every case from every generator in packages/flint-js/src/test-data (931 instances, of which 880 assemble on the Vega-Lite backend; the other 51 are backend-specific cases that do not). On dev @ c73d597: 10 hits, all Waterfall. With this change: 0.
  • Edge matrix, ten shapes, each compiled on dev @ c73d597 and on this branch and diffed: no display names; display names; missing x; missing y; no encodings at all (the || 'Category' / || 'Amount' fallbacks); an explicit Type column; totals: 'both' with showTextLabels: true, which binds two more internal columns to the y scale; an escaped field name; column facet; row facet. All ten leak __wf_lead on dev and all ten are clean after. Nothing throws, and the fallback titles still read Category / Amount. Escaped names improve as a side effect: the x title was a\.b, __wf_lead and is now a.b.
  • Compiled the title: null alternative from the issue, to check the claim in Review focus 1: with nulls on the internal bindings the compiled Vega spec carries no x-axis title at all.
  • npx eslint src/vegalite/templates/waterfall.ts — clean.

The tests assert on the assembled Vega-Lite spec rather than compiling, because vega-lite is only an optional peer dependency of flint-js and resolves here by hoisting; I did not want to add a test-time import that is not a declared dependency. The compile-level check above was run separately.

Notes for reviewers

  • Scope. In that 880-instance sweep, Waterfall is the only template that leaks an internal column into an axis title, so this is a template-local fix rather than an assembler-level guard.

  • Adjacent defect, not fixed here. The same sweep shows Violin, ECDF and Density also drop field_display_names on one axis, via a different mechanism: a pivoted synthetic column, or a template setting its own title, which preempts the !encodingObj.title guard in buildVLEncodings. Happy to open a separate issue.

  • Cross-language parity — worth a look, and it predates this branch. packages/flint-py mirrors this template, and flint/vegalite/templates/waterfall.py has the display-name half of the same defect: x_enc (lines 59-64) carries no title, line 90 hardcodes "title": y_field.

    I left Python alone. While checking whether to mirror the fix, I found the Python template is behind the TypeScript one by more than titles: it emits a single bar layer with no connector rule and no __wf_lead at all. The 8 Waterfall fixtures under shared/test-data/ record that same older shape — 1 layer, no __wf_lead — so the stored "JS reference" already does not match what flint-js on dev emits today (2 layers, bar + rule). Those 8 tests pass because Python and the fixtures are stale in the same direction, not because the two implementations agree. For context, uv run pytest in packages/flint-py is 180 failed / 600 passed on dev @ c73d597, before this branch.

    So mirroring the title fix into Python on its own would not land cleanly: I tried it, and all 8 Waterfall fixtures go red, because Python would then emit the corrected titles while the stored reference still holds the old ones. Getting Python genuinely back in step looks like a larger job than this PR — the connector layer, then the titles, then a fixture regeneration — and it depends on a regeneration tool I could not find in the repo. Happy to take it on as a follow-up if you want it, and happy to file it as an issue instead. Flagging rather than guessing.

  • CI will be red on typecheck, from before this branch. dev @ c73d597 does not typecheck on its own: src/vegalite/templates/scatter.ts(338,28): error TS7006. Verified against a clean checkout with this branch's changes removed. Also worth knowing: tsc --noEmit aborts there before reaching tsconfig.test.json, so npm run typecheck never gets as far as typechecking the tests. I ran the test config separately and it is clean.

The Waterfall template rebuilds its own encoding objects, and it read
only `.field` off `ctx.resolvedEncodings` — discarding the titles the
assembler had already resolved from `field_display_names`. The bar layer
hardcoded the raw field name, and the shared x encoding carried no title
at all.

Vega-Lite derives an axis title from the field name of every untitled
encoding on a shared scale and concatenates them, so the untitled x also
picked up the internal connector column: the axis rendered as
"week, __wf_lead".

Carrying the resolved titles across fixes both symptoms at once. An
explicit title on the primary channel settles the whole shared scale, so
the internal columns need no treatment of their own — `title: null` on
them would blank the axis instead, since an explicit null anywhere in a
shared-scale title set wins.

Verified by compiling with vl.compile and reading the Vega axis titles,
not just the assembled spec. Swept every bundled gallery case: Waterfall
was the only template leaking an internal column into an axis title.

652 tests passing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Chenglong-MS
Chenglong-MS merged commit 778cb2d into microsoft:dev Jul 31, 2026
1 check passed
Chenglong-MS added a commit that referenced this pull request Aug 2, 2026
Brings the waterfall resolved-axis-title fix (#79).

Both branches had independently fixed the same bug — the internal
`__wf_lead` / `__wf_connector_y` helper columns leaking into the merged
waterfall axis title. dev's fix supersedes this branch's:

  - this branch pinned the x title to the raw `xField`;
  - dev pins the *resolved* `xTitle`, which falls back to `xField` but
    honours a caller-supplied label.

Resolved in favour of dev, keeping this branch's explanatory comment.

Also carried the same reasoning to the three helper layers this branch
still nulled (`__wf_connector_y`, `__wf_sum`, `__wf_center`): a
`title: null` on a primary channel resolves the merged title to nothing
and blanks the axis for every layer on the shared scale, which is the
invariant dev's new waterfall-titles suite asserts. They now carry
`yTitle`, matching the bar layer.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 175e0e03-5dda-4a02-aa14-c181660b9c99
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