Skip to content

feat: paired histogram cells in Storybook (DRC-3390 Stage A)#1398

Merged
gcko merged 5 commits into
mainfrom
feature/drc-3390-stage-a-storybook-cells
May 29, 2026
Merged

feat: paired histogram cells in Storybook (DRC-3390 Stage A)#1398
gcko merged 5 commits into
mainfrom
feature/drc-3390-stage-a-storybook-cells

Conversation

@danyelf
Copy link
Copy Markdown
Contributor

@danyelf danyelf commented May 26, 2026

Summary

Stage A of the DRC-3390 paired-histograms 4-stage restructure. Storybook-only, no API or backend
dependency
— reviewers can clone this branch, run Storybook, and visually verify the chart cells in isolation.

Why Stage A first

Lowest-risk PR in the restructure: pure additive component-library work, no integration with SchemaView, no API plumbing, no Python. Reviewable
in parallel with Stage B (which owns the backend + API contract). Supersedes the closed #1391
(lifted verbatim minus the wrapper + hook, which move to Stage C).

Components shipped

  • PairedHistogramContinuous — quantile-bin constant-area paired histogram. Bar widths track quantile spans, heights track density, so
    bar area reads as the row-share in that bin. formatValue prop for caller-supplied label formatting.
  • PairedHistogramDiscrete — top-K paired histogram with two rendering modes (discriminated on data.mode):
    • mode: "counts" (default): paired bars sized by proportion, gap-on-absent semantics — a value with count 0 on one side simply doesn't
      render, leaving a visible empty half-slot.
    • mode: "ranks": rank-staircase for DuckDB's counts-less approx_top_k payloads. Bar height = (k − rank + 1) / k; slots are
      base-rank-ordered with current-only values appended right. Stable case reads as two matching descending staircases; order changes show as current
      bars zig-zagging against base's smooth descent. Stage B guarantees both-or-neither (no mixed mode).
    • formatValue?: (v: unknown) => string prop for caller-supplied label formatting (NULL handling, numeric abbreviation, date rendering). Stage
      C dispatches per column.type.
  • ProfileDistributionUnsupportedBanner — one-time MUI Alert for {status: "unsupported", reason} envelopes (Postgres / MySQL / SQLite / SQL
    Server pre-2022).
  • Supporting infrastructure: pairedHistogramCanvas.tsx (shared SVG frame + baseline rule + tooltip formatters), chartColors.ts (theme/palette
    helpers extracted from HistogramChart so the leaf cells don't drag Chart.js into the bundle).

CELL_WIDTH = 140, CELL_HEIGHT = 28 — sized to fit inside the existing SchemaView row (rowHeight = 35).

Test plan

  • pnpm install clean.
  • pnpm --filter @datarecce/ui type:check — no errors in new files (baseline CSS-module / useTrackLineageRender errors unchanged).
  • Vitest: 40 tests pass across the 3 new test files (PairedHistogramContinuous, PairedHistogramDiscrete, ProfileDistributionUnsupportedBanner).
  • pnpm --filter @datarecce/storybook build builds clean.
  • pnpm --filter @datarecce/storybook storybook (port 6006) — all 12 stories render.
  • Visual proofs:
    • Continuous Cell — order amount — bar widths visibly differ across bins (constant-area).
    • Continuous Degenerate range — flat checkerboard, no crash.
    • Discrete Cell — gap-on-absent — literal visual gaps where one side absent.
    • Discrete Cell — trimmed top-K marker — corner "trimmed" marker renders.
    • Storybook toolbar toggles dark theme — bar palette inverts on every story.
continuous-cell-added-column continuous-cell-order-amount--dark continuous-cell-order-amount continuous-cell-stable continuous-degenerate discrete-cell-http-status discrete-cell-signup-source discrete-cell-trimmed discrete-cell-with-gaps--dark discrete-cell-with-gaps

Ranked
image

To reproduce: pnpm install && pnpm --filter @datarecce/storybook storybook from js/, then open Visualizations / Profile Distribution in the Storybook sidebar.

danyelf added a commit that referenced this pull request May 26, 2026
10 captures from `Visualizations/Profile Distribution/*` (5 continuous +
5 discrete; 2 in dark mode via the toolbar). Linked from PR #1398 for
at-a-glance review without spinning up Storybook.

Signed-off-by: Danyel Fisher <danyel@gmail.com>
danyelf added a commit that referenced this pull request May 28, 2026
…odule

Address review findings on PR #1398:

1. Delete the duplicate `components/data/chartColors.ts` and make
   `theme/chartTheme.ts` the single source of truth. Adds
   `barLabelColor`/`secondaryTextColor` to `ChartThemeColors`, adds the
   `ChartBarColors` interface, renames `getBarColors` → `getChartBarColors`
   (drops the unused `info` field and `INFO_VAL_COLOR` constants).
2. Drop the back-compat re-export block in `HistogramChart.tsx`. All four
   internal consumers (`HistogramChart`, `TopKBarChart`, the two paired
   histograms, and the test) now import directly from `../../theme`.
   `data/index.ts`, `primitives.ts`, and `types/index.ts` no longer
   duplicate the symbols (they live in `@datarecce/ui/theme`).
3. Normalize the continuous fixtures via a new
   `continuousFromProportions(edges, baseProps, currentProps, …)` helper
   so per-bin row proportions sum to 1.0 — tooltip percentages now read
   as 100% across `continuousOrderAmount`, `continuousStable`, and
   `continuousAddedOnly` (previously they could sum to >4000%).
4. Replace the hardcoded `#0b1220cc` / `#ffffffcc` trimmed-marker chip
   background in `PairedHistogramDiscrete` with
   `${theme.tooltipBackgroundColor}cc` so the chip blends with whatever
   surface the cell ships into rather than the storybook mock.

Story boilerplate cleanup: add `toContinuousProps`, `toDiscreteProps`,
`toDiscreteRanksProps` adapters in `fixtures.ts` and use them instead
of inlining the snake→camel conversion at every call site.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
danyelf added a commit that referenced this pull request May 28, 2026
…arColors

Post-review followups on PR #1398:

- computeDiscreteSlots and computeRanksSlots now Array.isArray-check
  values / baseCounts / currentCounts (and baseRanks / currentRanks)
  before reading .length, so a malformed payload renders an empty SVG
  instead of crashing the schema row.
- Reverted the getBarColors -> getChartBarColors rename; the function
  and the INFO_VAL_COLOR / INFO_VAL_COLOR_DARK constants are restored
  to their pre-PR names so @datarecce/ui/theme has zero public surface
  change on this branch.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
…3390 Stage A)

Stage A of the DRC-3390 paired-histograms restructure: Storybook-only,
no API or backend dependency. Adds the GA-replacement cell components
for inline profile distributions in SchemaView.

Components shipped:
- PairedHistogramContinuous — quantile-bin constant-area paired histogram;
  bar width tracks the bin's quantile span, height tracks density, so bar
  area reads as the row-share of that bin.
- PairedHistogramDiscrete — top-K paired histogram with two modes
  (discriminated on data.mode): "counts" (default) renders bars sized by
  per-side proportion with gap-on-absent semantics; "ranks" renders the
  rank-staircase for DuckDB's counts-less approx_top_k payloads.
- ProfileDistributionUnsupportedBanner — one-time MUI Alert for
  {status:"unsupported", reason} envelopes (Postgres / MySQL / SQLite /
  SQL Server pre-2022).
- pairedHistogramCanvas — shared SVG frame + baseline rule + tooltip
  formatters, used by both paired-histogram cells.

Supporting refactors:
- Route HistogramChart / TopKBarChart / PairedHistogramContinuous through
  utils/formatters.ts (formatAsAbbreviatedNumber, formatIntervalMinMax)
  instead of three near-duplicate local formatters.
- Move ChartBarColors / ChartThemeColors / getChartBarColors /
  getChartThemeColors canonical location to theme/chartTheme.ts;
  primitives.ts still re-exports them so the public API surface is
  unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
@danyelf danyelf force-pushed the feature/drc-3390-stage-a-storybook-cells branch from 83e8182 to 141faaa Compare May 28, 2026 19:52
@danyelf danyelf marked this pull request as ready for review May 28, 2026 19:53
@danyelf danyelf self-assigned this May 28, 2026
danyelf and others added 3 commits May 28, 2026 13:15
The `./theme` subpath barrel re-exported `ChartThemeColors` but not its
sibling `ChartBarColors`, even though both live in `chartTheme.ts` and
both are reachable via `@datarecce/ui/primitives`. Consumers following
the inline pointer in `src/index.ts` ("canonical in @datarecce/ui/theme")
to import `ChartBarColors` hit a wall.

One-line fix in the existing re-export block.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
…1398 review

Address PR #1398 review feedback (DRC-3390 Stage A):

YAGNI (Y1-Y4):
- Drop showEndpoints/showMidpoint/showLabels/labelMaxChars props — cell
  mode is fixed at 140×28, grid mode is out of scope for Stage A.
- Drop width/height props — CELL_WIDTH and CELL_HEIGHT are constants.
- Drop ariaLabel prop — each cell now uses its hardcoded constant.
- Un-export computeContinuousLayout / computeDiscreteSlots /
  computeRanksSlots from the package barrels; they remain file-level
  exports so the colocated tests can import them.

Style (S2-S4):
- Rename pairedHistogramCanvas.tsx → PairedHistogramCanvas.tsx to match
  the PascalCase convention used by every other file in components/data/.
- Replace `${color}cc` string-concatenated alpha with a proper theme
  field `overlayBackgroundColor` on ChartThemeColors.
- Convert 12px → 0.75rem in ProfileDistributionUnsupportedBanner per
  js/CLAUDE.md (px-for-font-size lint rule).

Correctness (C1-C2):
- heightForRank: clamp to [0, chartHeight] so a Stage B contract
  violation (rank > k → negative, rank ≤ 0 → over-tall) renders a
  well-formed bar instead of a malformed SVG rect.
- computeRanksSlots: emit a console.warn alongside the defensive
  drop, so a Stage B contract violation (value absent from both
  top-Ks) surfaces in dev tools instead of vanishing silently.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
Now that `showLabels` is gone, `displayValue` on `RenderSlot` is set but
never read — the SVG body only consumes `baseH`, `currH`, and
`hoverTitle` (which already bakes in the formatted display string).
Inline `formatValue(s.value)` at the tooltip call site and drop the
field.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
@danyelf danyelf requested a review from gcko May 28, 2026 20:45
@danyelf danyelf marked this pull request as draft May 28, 2026 23:08
@danyelf
Copy link
Copy Markdown
Contributor Author

danyelf commented May 28, 2026

Cross-linking from the Stage B backend (#1399) — there's a contract inconsistency here we need to settle together before either lands.

PairedHistogramContinuousData takes a single shared binEdges, but the component's premise is constant-area — "bar area reads directly as the proportion of rows in that bin," every bar an equal row-share. That equal-area property only holds when each env is binned on its own quantile edges. The moment base and current share one edge array, at most one of them is constant-area.

The backend can't fix this with a shared array. All we get back from the warehouse is approx percentiles (APPROX_PERCENTILE/approx_quantile) — inverse-CDF sample points per env, not bin counts. There's no way to measure an env's true mass in an arbitrary shared bin. The only assumption-free rendering is each env on its own quantile spans, density = (1/NUM_BINS)/span. Base and current edges genuinely don't line up (base might break at 10/20/40/60/70, current at 5/15/40/50/80) — and that's correct: two independent equal-area staircases overlaid on a shared value axis.

(Related: in Stage B as written, the shared-edge averaging makes base_density and current_density come out byte-for-byte identical — the curves can never diverge. Same root cause.)

Contract both PRs should freeze on:

  • payload carries per-env edgesbase_bin_edges + current_bin_edges (each length 12) — alongside base_density/current_density (each length 11)
  • PairedHistogramContinuousData takes both edge arrays
  • the component renders two non-aligned staircases, not aligned paired bars on one x-grid

Purely visual overlay — we're explicitly giving up bin-aligned diffing (no shared "bin 3" to subtract), which is fine for this surface.

I'll update the Stage B backend + API types (#1399) to emit per-env edges; this component, its stories, and the binEdges type need the matching rework.

The continuous cell took a single shared `binEdges`, but constant-area
rendering only holds when each env bins on its own quantile edges (see
PR #1398 discussion) — with one shared edge array at most one env is
constant-area, and the Stage B backend can't honor it without collapsing
both densities into one.

Replace `binEdges` with independent `baseBinEdges` + `currentBinEdges`.
`computeContinuousLayout` now unions both edge sets onto a shared value
axis and bars the merged edge grid: every segment lies wholly inside one
base bin and one current bin, so both densities are constant across it —
which keeps the existing agreement-zone (min) + differential (max-min)
renderer working unchanged on the skinnier segments. Constant-area is
preserved because subdividing a bin sums its sub-segments back to the
bin's area.

Fixtures/stories updated to the `base_bin_edges`/`current_bin_edges`
payload with genuinely different per-env edges so they exercise the
merged grid. The PR-discussion worked example is now a unit test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
danyelf added a commit that referenced this pull request May 28, 2026
Shared bin edges collapsed base_density and current_density to identical
arrays — averaging the two envs' quantiles onto one grid threw away the
divergence the paired histogram exists to show, and broke the equal-area
property the renderer (PR #1398) depends on.

With only approx percentiles from the warehouse (no bin counts), the
honest rendering is each env on its OWN quantile edges:
density = (1/NUM_BINS)/span. Base and current edges deliberately do not
line up; Stage C overlays the two staircases on a shared value axis.

- _build_histogram_payload now emits base_bin_edges + current_bin_edges
  via new _env_edges_and_density helper (fraction-delta / width).
- API contract: ProfileDistributionHistogramPayload carries per-env edges.
- Drop now-unused _safe_min/_safe_max.
- Tests: assert base != current for divergent inputs, equal-area per bin,
  empty-without-bounds, and the parked point-mass (tie) behaviour.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
@danyelf danyelf marked this pull request as ready for review May 28, 2026 23:48
@gcko
Copy link
Copy Markdown
Contributor

gcko commented May 29, 2026

Code Review: PR #1398

SHA 01662e58 · Verdict GO

Stage A is genuinely low-risk: two new SVG leaf cells, a shared canvas helper, a banner, and a chart-theme/​formatter consolidation. The new components are defensively written (length-mismatch guards, NaN/degenerate-range fallbacks, rank clamping) and the tests cover merged-edge geometry, gap-on-absent, both palettes, degenerate inputs, and tooltip formatting. Verified locally on 01662e58: frozen install ✓; @datarecce/ui type:check introduces 0 new errors (16 baseline CSS-module / useTrackLineageRender errors are identical on main); 75 tests pass across the 4 touched test files; Biome clean (633 files); Storybook build succeeds.

Blockers

None.

Issues

None.

Notes

  1. TopKBarChart.tsx:514formatIntervalMinMax(proportion) ?? "" is dead defensive code: formatIntervalMinMax (utils/formatters.ts:25) always returns a string, never nullish. The sibling call at :311 correctly omits the ?? "". Harmless, but the inconsistency invites a future reader to assume the function is nullable.
    Pass C.

  2. HistogramChart.tsx / TopKBarChart.tsx — the PR is described as "Storybook-only, no API or backend, pure additive," but it also swaps two production charts' local number formatters for the shared formatAsAbbreviatedNumber. This is a real (cosmetic) output change on detail/popover surfaces: e.g. a bin edge of 5 now renders 5 instead of 5.00, and 0 renders 0 instead of 0.00e+0. Percentages are unchanged (formatIntervalMinMax is behaviorally equivalent to the old formatPercentage — verified). No test asserts the new numeric format (the HistogramChart.test.tsx diff only updates imports), and the PR's visual proofs are all Storybook cells, not the affected production charts. The change is an improvement (unifies on the app's canonical formatter), but worth a spot-check of the live HistogramChart/TopKBarChart axis + tooltip rendering before merge, since it falls outside the stated scope.
    Pass F.


Checked and cleared: the getChartBarColors/getChartThemeColors/ChartBarColors/ChartThemeColors move out of HistogramChart — all in-repo consumers import from ../../theme; recce-cloud-infra only mocks the two functions (still on the main barrel), and nothing consumes the types dropped from the @datarecce/ui/types subpath. No downstream break. Also cleared: getChartBarColors correctly returns the new required info field; densityAt half-open/closed logic is safe because merged-segment midpoints never coincide with an original edge; degenerate-range fallback can't divide by zero (validity guard ensures n ≥ 1).

Copy link
Copy Markdown
Contributor

@gcko gcko left a comment

Choose a reason for hiding this comment

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

GO — 0 blockers, 0 issues, 2 non-blocking notes. Full review: #1398 (comment)

@gcko gcko merged commit 5db5044 into main May 29, 2026
7 checks passed
@gcko gcko deleted the feature/drc-3390-stage-a-storybook-cells branch May 29, 2026 01:44
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