diff --git a/web/src/lib/components/charts/BarChart.render.svelte.test.ts b/web/src/lib/components/charts/BarChart.render.svelte.test.ts index 033331f65..71d50f833 100644 --- a/web/src/lib/components/charts/BarChart.render.svelte.test.ts +++ b/web/src/lib/components/charts/BarChart.render.svelte.test.ts @@ -4,11 +4,15 @@ // Nothing in the suite asserted anything about `BarChart`'s output. Its only // consumers are the insights page and its print route; no vitest suite rendered // either, and the one e2e spec that visits `/insights` -// (page-title-survives-pane-and-nav.spec.ts:88) SPA-navigates there and checks -// `document.title` alone — on a fresh fixture workspace the chart's `hasData` -// is false, so the layers never even mount. So `Web` went green on a major bump -// that rewrote the library's context API out from under the three chart layers. -// The green was the enumeration (nothing looked), not the property (the charts +// (page-title-survives-pane-and-nav.spec.ts:88) asserts the URL pathname and +// `document.title` — nothing about chart output. +// +// A title assertion cannot catch this class of break, which was measured rather +// than reasoned: serving the unmigrated v11 build in a real browser, the +// insights page still loaded with its title intact and threw only `pageerror`s +// while rendering zero chart elements. So `Web` went green on a major bump that +// rewrote the library's context API out from under the three chart layers. The +// green was the enumeration (nothing looked), not the property (the charts // work). // // This test renders the chart end-to-end through the real `` — no @@ -22,11 +26,15 @@ // under test. import { afterEach, beforeAll, describe, expect, it } from 'vitest'; import { render, cleanup } from '@testing-library/svelte'; +import { tick } from 'svelte'; import BarChart from './BarChart.svelte'; const BOX_WIDTH = 600; const BOX_HEIGHT = 240; +const TOOLTIP_WIDTH = 80; +// BarChart's own padding literal; bandCenter offsets by its left edge. +const PADDING_LEFT = 36; beforeAll(() => { // LayerCake measures its container through `bind:clientWidth` / @@ -40,8 +48,15 @@ beforeAll(() => { // binding not to throw. Object.defineProperty(HTMLElement.prototype, 'clientWidth', { configurable: true, - get() { - return BOX_WIDTH; + get(this: HTMLElement) { + // The tooltip reports a NARROW width on purpose. BarChart clamps the + // tooltip's left edge into [w/2, canvasWidth - w/2]; if the tooltip + // measured the full canvas width that interval would collapse to a + // single point and every position assertion below would hold no matter + // what the layer computed. Measured: with both at 600 the hover leg + // could not tell `bandCenter` apart from one with `k.padding.left` + // dropped. + return this.classList?.contains('tooltip') ? TOOLTIP_WIDTH : BOX_WIDTH; }, }); Object.defineProperty(HTMLElement.prototype, 'clientHeight', { @@ -174,4 +189,40 @@ describe('BarChart renders through LayerCake', () => { ); expect(Math.max(...ticks)).toBeGreaterThanOrEqual(12); }); + + it('drives the hover tooltip from context read at event time', async () => { + // `bandCenter` and `bandSummary` (Bars.svelte) are the only context reads + // in the unit that happen OUTSIDE a tracking context — they run in a + // pointer handler and want the value at event time rather than a + // dependency. Review round 1 judged that correct by design; this leg is + // what makes it checked rather than argued. It is also the only coverage + // of `k.padding` anywhere in this file. + const { container } = renderChart(); + + const hits = container.querySelectorAll('g.bars rect.hit'); + expect(hits.length).toBe(DATA.length); + + // The accessible per-band summary is built from k.x plus the series values. + expect(hits[1].querySelector('title')?.textContent).toBe( + 'Tue — Created 3, Completed 7' + ); + + hits[1].dispatchEvent(new PointerEvent('pointerenter', { bubbles: true })); + await tick(); + + const tooltip = container.querySelector('.tooltip') as HTMLElement | null; + expect(tooltip).not.toBeNull(); + expect(tooltip?.textContent).toContain('Tue'); + expect(tooltip?.textContent).toContain('7'); + + // bandCenter = k.padding.left + k.xGet(d) + bandwidth/2. Both operands are + // readable off the hit-rect itself (its x IS k.xGet(d), its width IS the + // bandwidth), so this asserts the actual relation rather than a magic + // pixel — and it fails if the k.padding.left term is dropped. + const hitX = Number(hits[1].getAttribute('x')); + const hitW = Number(hits[1].getAttribute('width')); + const left = Number.parseFloat(tooltip!.style.left); + expect(Number.isFinite(left)).toBe(true); + expect(left).toBeCloseTo(PADDING_LEFT + hitX + hitW / 2, 5); + }); }); diff --git a/web/src/lib/components/charts/theme.ts b/web/src/lib/components/charts/theme.ts index 627ea1f43..a72212c8b 100644 --- a/web/src/lib/components/charts/theme.ts +++ b/web/src/lib/components/charts/theme.ts @@ -58,15 +58,17 @@ export interface ResolvedSeries { * started reading it. * * Layers reach it as `getLayerCakeContext() as unknown as LayerCakeContext`. - * The hop through `unknown` is required, not laziness. The package types every - * scale as the loose `Scale` (`{ (value: any): any, [key: string]: any }`), - * which DECLARES none of the methods the shapes above require, and an index - * signature does not satisfy a required named member. So TS refuses the direct - * cast — verbatim: "Conversion of type 'LayerCakeContext' to - * type 'LayerCakeContext' may be a mistake because neither type sufficiently - * overlaps with the other ... Property 'ticks' is missing in type 'Scale' but - * required in type '{ (value: unknown): number; ticks: ... }'". The package - * casts its own context through `unknown` for the same reason. + * The hop through `unknown` is required, not laziness: the package types every + * scale as `{ (value: any): any, [key: string]: any }`, and an index signature + * does not satisfy a required named property — so `yScale` below, which + * requires `ticks`, is judged non-comparable and TS2352 rejects the direct cast. + * `xScale` is NOT what blocks it: `bandwidth` is optional there, so nothing it + * requires is missing. Measured while narrowing this interface — when both + * scales still required `domain`, the compiler named `xScale` first; dropping it + * moved the error to `yScale`. The package also casts its own context through + * `unknown`, but for its own reasons (it gives two: conditional `ScaleFor` types + * that don't compare across different `S`, and an object that starts empty and + * is filled by `defineProperties`) — not this one. * * What the hop gives up is only that one comparison, against a type that * asserts nothing. What this interface exists to check is the layer bodies, and @@ -83,7 +85,14 @@ export interface LayerCakeContext { (value: unknown): number; bandwidth?: () => number; }; - /** Linear scale: called for a position, and asked for `ticks()` by AxisY. */ + /** + * Linear scale: called for a position, and asked for `ticks()` by AxisY. + * `ticks` is REQUIRED, which is only safe because `BarChart` hardcodes + * `scaleLinear()` and never exposes the y scale as a prop — a band scale has + * no `ticks`. If the y scale ever becomes caller-supplied, this narrowing + * turns into a lie the cast would hide, and AxisY needs the same `typeof` + * probe the bandwidth reads use. + */ yScale: { (value: unknown): number; ticks: (count?: number) => number[];