From d1aa79106d057b8e299560516fc99e84dd566165 Mon Sep 17 00:00:00 2001 From: Yaroslav Boiko Date: Mon, 17 Aug 2026 17:03:33 +0200 Subject: [PATCH 1/4] [OPIK-7839] [FE] fix confusable series colors in grouped charts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Series colors are derived from the label alone: `md5(label) % TAG_VARIANTS.length`. Palette: `primary` (#6366f1) sat at ΔE 14.5 from `purple` and 22.7 from `blue` — below what anyone separates in a thin line, and the pair the customer reported. Substituted with a new `ochre` (#8c683f) entry: ΔE 44.1 to its nearest neighbour (`gray`), and it clears the thin-line contrast floor on both grounds (5.04:1 light, 3.72:1 dark). The palette's worst pair is now `gray`/`turquoise` at ΔE 35.6. Substituting rather than extending keeps the palette at ten entries, so the modulo does not shift and only labels that resolved to that slot change color — uniformly, everywhere that label appears (chart series, tag chips, feedback scores). Also stops the hardcoded metric-name map being applied when a breakdown is active: there the line names are group values, not metric names, and four of its nine keys share one violet, so a group named `cost` or `total_tokens` could silently collide with another. Guard rails in lib/colorVariants.test.ts: minimum pairwise ΔE across the palette (fails with `primary/purple ΔE=14.5` if reintroduced), a contrast floor with a documented allowlist for the pre-existing `yellow` (1.92) and `turquoise` (2.43) entries, and a golden assertion that known labels keep the colors they resolve to today, so a future palette edit cannot silently re-color unrelated labels. Identical colors remain possible when the number of series approaches the palette size — asserted explicitly as a documented limitation. That case is answered by the existing manual per-label override (OPIK-3100); reassigning automatically would break the guarantee that a label keeps one color across widgets. Reserving gray for the `Others`/`Unknown` buckets was attempted and dropped: `gray` is itself in the automatic palette, so reserving it made ~1 label in 10 render identical to `Others` — the defect this change exists to remove. Doing it properly needs a tenth distinct color, which is a design dependency tracked separately. v2 chart container only — v1 is slated for removal. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/constants/colorVariants.ts | 11 +- .../src/lib/colorVariants.test.ts | 217 ++++++++++++++++++ apps/opik-frontend/src/main.scss | 5 + apps/opik-frontend/src/ui/tag.tsx | 16 +- .../MetricChart/MetricChartContainer.tsx | 11 +- 5 files changed, 255 insertions(+), 5 deletions(-) create mode 100644 apps/opik-frontend/src/lib/colorVariants.test.ts diff --git a/apps/opik-frontend/src/constants/colorVariants.ts b/apps/opik-frontend/src/constants/colorVariants.ts index ec8f5ca08fc..b6e6dc70a74 100644 --- a/apps/opik-frontend/src/constants/colorVariants.ts +++ b/apps/opik-frontend/src/constants/colorVariants.ts @@ -14,7 +14,11 @@ export const COLOR_VARIANTS = [ ] as const; export type ColorVariant = (typeof COLOR_VARIANTS)[number]; -export type ExtendedColorVariant = ColorVariant | "primary" | "default"; +export type ExtendedColorVariant = + | ColorVariant + | "primary" + | "ochre" + | "default"; export const COLOR_VARIANTS_MAP: Record< ExtendedColorVariant, @@ -31,6 +35,7 @@ export const COLOR_VARIANTS_MAP: Record< turquoise: { css: "var(--color-turquoise)", hex: "#06b6d4" }, blue: { css: "var(--color-blue)", hex: "#3b82f6" }, primary: { css: "var(--color-primary)", hex: "#6366f1" }, + ochre: { css: "var(--color-ochre)", hex: "#8c683f" }, default: { css: "var(--color-gray)", hex: "#64748b" }, }; @@ -40,8 +45,10 @@ export const PRESET_HEX_COLORS = COLOR_VARIANTS.map( export const DEFAULT_HEX_COLOR = COLOR_VARIANTS_MAP.blue.hex; +// `primary` and `ochre` are not in COLOR_VARIANTS (they are not picker presets) but are still +// reachable as resolved colors, so their css vars must round-trip to hex for the color picker. export const CSS_VAR_TO_HEX: Record = Object.fromEntries( - [...COLOR_VARIANTS, "primary" as const].map((v) => [ + [...COLOR_VARIANTS, "primary" as const, "ochre" as const].map((v) => [ COLOR_VARIANTS_MAP[v].css, COLOR_VARIANTS_MAP[v].hex, ]), diff --git a/apps/opik-frontend/src/lib/colorVariants.test.ts b/apps/opik-frontend/src/lib/colorVariants.test.ts new file mode 100644 index 00000000000..26012ff17a7 --- /dev/null +++ b/apps/opik-frontend/src/lib/colorVariants.test.ts @@ -0,0 +1,217 @@ +import { describe, expect, it } from "vitest"; + +import { + resolveChartColorMap, + resolveColor, + resolveHexColor, +} from "@/lib/colorVariants"; +import { TAG_VARIANTS, TAG_VARIANTS_COLOR_MAP } from "@/ui/tag"; +import { COLOR_VARIANTS_MAP } from "@/constants/colorVariants"; + +/** + * Perceptual helpers live here rather than in shipped code: only these invariants need them, and + * shipping an unused utility invites a second, divergent implementation. + */ +const linearize = (c: number) => + c <= 0.04045 ? c / 12.92 : Math.pow((c + 0.055) / 1.055, 2.4); + +const linearChannels = (hex: string) => + [1, 3, 5].map((offset) => + linearize(parseInt(hex.slice(offset, offset + 2), 16) / 255), + ) as [number, number, number]; + +const toLab = (hex: string): [number, number, number] => { + const [r, g, b] = linearChannels(hex); + + const x = (r * 0.4124564 + g * 0.3575761 + b * 0.1804375) / 0.95047; + const y = r * 0.2126729 + g * 0.7151522 + b * 0.072175; + const z = (r * 0.0193339 + g * 0.119192 + b * 0.9503041) / 1.08883; + + const f = (t: number) => (t > 0.008856 ? Math.cbrt(t) : 7.787 * t + 16 / 116); + + return [116 * f(y) - 16, 500 * (f(x) - f(y)), 200 * (f(y) - f(z))]; +}; + +/** CIE76 distance. Below ~25 two thin chart lines are hard to tell apart without hovering. */ +const perceptualDistance = (a: string, b: string) => { + const [l1, a1, b1] = toLab(a); + const [l2, a2, b2] = toLab(b); + return Math.hypot(l1 - l2, a1 - a2, b1 - b2); +}; + +const relativeLuminance = (hex: string) => { + const [r, g, b] = linearChannels(hex); + return 0.2126 * r + 0.7152 * g + 0.0722 * b; +}; + +const contrastRatio = (a: string, b: string) => { + const la = relativeLuminance(a); + const lb = relativeLuminance(b); + return (Math.max(la, lb) + 0.05) / (Math.min(la, lb) + 0.05); +}; + +// The grounds the product actually paints: `--background` is 0 0% 100% in the light theme and +// 0 0% 7% in the dark one (main.scss). +const LIGHT_GROUND = "#ffffff"; +const DARK_GROUND = "#121212"; + +const autoPaletteHexes = () => + TAG_VARIANTS.map((variant) => ({ + variant: variant as string, + hex: resolveHexColor(TAG_VARIANTS_COLOR_MAP[variant!]), + })); + +describe("automatic palette invariants", () => { + it("resolves every entry to a hex value", () => { + for (const { variant, hex } of autoPaletteHexes()) { + expect(hex, `${variant} must round-trip to hex`).toMatch( + /^#[0-9a-fA-F]{6}$/, + ); + } + }); + + it("keeps every pair of entries perceptually distinguishable", () => { + // The automatic color is `hash % TAG_VARIANTS.length`, so any two labels can land on any two + // entries. Regression guard for OPIK-7839, where `primary` sat at ΔE 14.5 from `purple` and + // made distinct grouped series look identical. + const MINIMUM_DISTANCE = 30; + const palette = autoPaletteHexes(); + const tooClose: string[] = []; + + for (let i = 0; i < palette.length; i += 1) { + for (let j = i + 1; j < palette.length; j += 1) { + const distance = perceptualDistance(palette[i].hex, palette[j].hex); + if (distance < MINIMUM_DISTANCE) { + tooClose.push( + `${palette[i].variant}/${palette[j].variant} ΔE=${distance.toFixed( + 1, + )}`, + ); + } + } + } + + expect(tooClose).toEqual([]); + }); + + it("never assigns the indigo that was confusable with purple and blue", () => { + const indigo = COLOR_VARIANTS_MAP.primary.hex.toLowerCase(); + + expect( + autoPaletteHexes().map(({ hex }) => hex.toLowerCase()), + ).not.toContain(indigo); + }); + + it("keeps known labels on the colors they resolve to today", () => { + // Guards the property that makes a palette change safe to ship: the automatic color depends on + // the palette length, so adding or removing an entry silently re-colors every label in the + // product. Substituting one entry only moves labels that resolved to that slot. If this fails, + // a change shifted labels it did not intend to. + expect( + resolveChartColorMap([ + "domain-agent", + "supervisor-agent", + "planner-agent", + "retriever-agent", + ]), + ).toEqual({ + "domain-agent": "var(--color-ochre)", + "supervisor-agent": "var(--color-blue)", + "planner-agent": "var(--color-orange)", + "retriever-agent": "var(--color-yellow)", + }); + }); + + it("still lets two labels share a color once the palette is exhausted", () => { + // Documented limitation, not a defect to fix here: with ten colors, identical colors remain + // possible as the number of series approaches the palette size. The answer is the manual + // per-label override, not automatic reassignment — reassigning would break the guarantee that + // a label keeps one color across widgets. + const map = resolveChartColorMap(["id_filter_enabled", "domain-agent"]); + + expect(map["id_filter_enabled"]).toBe(map["domain-agent"]); + }); + + it("keeps entries legible as thin lines on both grounds", () => { + // `yellow` and `turquoise` predate this invariant and fail on the light ground at 1.92 and + // 2.43. They are tracked separately as a palette-contrast follow-up. The assertion below still + // blocks *new* low-contrast entries, and the allowlist must not grow. + const KNOWN_LOW_CONTRAST = ["yellow", "turquoise"]; + const MINIMUM_CONTRAST = 2.5; + const failing: string[] = []; + + for (const { variant, hex } of autoPaletteHexes()) { + if (KNOWN_LOW_CONTRAST.includes(variant)) continue; + + const light = contrastRatio(hex, LIGHT_GROUND); + const dark = contrastRatio(hex, DARK_GROUND); + if (Math.min(light, dark) < MINIMUM_CONTRAST) { + failing.push( + `${variant} light=${light.toFixed(2)} dark=${dark.toFixed(2)}`, + ); + } + } + + expect(failing).toEqual([]); + }); +}); + +describe("resolveColor priority", () => { + const label = "domain-agent"; + + it("falls back to an automatic color when no map contains the label", () => { + expect(resolveColor(label)).toBeDefined(); + }); + + it("prefers a caller-supplied color over the automatic one", () => { + const reserved = { [label]: COLOR_VARIANTS_MAP.gray.css }; + + expect(resolveColor(label, null, reserved)).toBe( + COLOR_VARIANTS_MAP.gray.css, + ); + expect(resolveColor(label, null, reserved)).not.toBe(resolveColor(label)); + }); + + it("prefers the workspace color over both the caller-supplied and the automatic one", () => { + const workspace = { [label]: "#123456" }; + const reserved = { [label]: COLOR_VARIANTS_MAP.gray.css }; + + expect(resolveColor(label, workspace, reserved)).toBe("#123456"); + }); + + it("leaves labels absent from the maps on their automatic color", () => { + const other = "supervisor-agent"; + const reserved = { [label]: COLOR_VARIANTS_MAP.gray.css }; + + expect(resolveColor(other, null, reserved)).toBe(resolveColor(other)); + }); + + it("assigns a label the same color regardless of which other labels are present", () => { + // Cross-widget consistency is a product requirement, not an incidental property: the same tag + // must keep one color across widgets that show different sets of series. + const inOneChart = resolveChartColorMap([label, "supervisor-agent"]); + const inAnother = resolveChartColorMap([label, "planner-agent", "a", "b"]); + + expect(inOneChart[label]).toBe(inAnother[label]); + expect(resolveChartColorMap([label])[label]).toBe(inOneChart[label]); + }); +}); + +describe("resolveChartColorMap", () => { + it("returns one color per label", () => { + const labels = ["alpha", "beta", "gamma"]; + const map = resolveChartColorMap(labels); + + expect(Object.keys(map).sort()).toEqual(labels.sort()); + }); + + it("pins only the labels the caller names, leaving the rest automatic", () => { + // How a chart fixes colors for specific series (metric sub-series today) without disturbing + // labels it says nothing about. + const pinned = { traces: COLOR_VARIANTS_MAP.purple.css }; + const map = resolveChartColorMap(["traces", "domain-agent"], null, pinned); + + expect(map.traces).toBe(COLOR_VARIANTS_MAP.purple.css); + expect(map["domain-agent"]).toBe(resolveColor("domain-agent")); + }); +}); diff --git a/apps/opik-frontend/src/main.scss b/apps/opik-frontend/src/main.scss index 44a6122692b..fb05f4dfbc6 100644 --- a/apps/opik-frontend/src/main.scss +++ b/apps/opik-frontend/src/main.scss @@ -205,6 +205,7 @@ --color-lime: #a3e635; --color-turquoise: #06b6d4; --color-blue: #3b82f6; + --color-ochre: #8c683f; --color-primary: #6366f1; --color-indigo: #6366f1; --color-violet: #8b5cf6; @@ -249,6 +250,8 @@ --tag-blue-text: #19426b; --tag-lavender-bg: #e5e5fe; --tag-lavender-text: #3b3b8e; + --tag-ochre-bg: #f2e6d8; + --tag-ochre-text: #5c4426; --click-blue: #262ab5; @@ -517,6 +520,8 @@ --tag-blue-text: #6eabe7; --tag-lavender-bg: #2a2a3d; --tag-lavender-text: #a8a8e0; + --tag-ochre-bg: #2f2b26; + --tag-ochre-text: #c9975c; /* Trace/span type colors - Dark Mode */ --type-trace: #945fcf; diff --git a/apps/opik-frontend/src/ui/tag.tsx b/apps/opik-frontend/src/ui/tag.tsx index cfd703de7d6..b801c1f1644 100644 --- a/apps/opik-frontend/src/ui/tag.tsx +++ b/apps/opik-frontend/src/ui/tag.tsx @@ -19,6 +19,7 @@ const tagVariants = cva("inline-block truncate rounded-sm transition-colors", { turquoise: "bg-[var(--tag-turquoise-bg)] text-[var(--tag-turquoise-text)]", blue: "bg-[var(--tag-blue-bg)] text-[var(--tag-blue-text)]", + ochre: "bg-[var(--tag-ochre-bg)] text-[var(--tag-ochre-text)]", lavender: "bg-[var(--tag-lavender-bg)] text-[var(--tag-lavender-text)]", white: "border border-gray-200 bg-white text-muted-slate dark:border-gray-600 dark:bg-gray-800 dark:text-foreground", @@ -60,11 +61,23 @@ const Tag = React.forwardRef( ); Tag.displayName = "Tag"; +/** + * Palette used to derive a color automatically from a label (see `generateTagVariant`). + * + * Two invariants are enforced by `lib/colorVariants.test.ts` and must hold for any change here: + * - every pair of entries stays perceptually distinguishable (OPIK-7839: `primary` #6366f1 sat at + * ΔE 14.5 from `purple`, which made distinct series look identical in grouped charts); + * - the list keeps its length, because the automatic color is `hash % TAG_VARIANTS.length` — + * adding or removing an entry re-maps every label in the product, while substituting one + * re-maps only the labels that resolved to that slot. + * + * `primary` is deliberately absent: it is too close to both `purple` and `blue` for chart series. + */ export const TAG_VARIANTS: Exclude< TagProps["variant"], "red" | "transparent" | "white" | "lavender" >[] = [ - "primary", + "ochre", "gray", "purple", "burgundy", @@ -94,6 +107,7 @@ export const TAG_VARIANTS_COLOR_MAP: Record< green: "var(--color-green)", turquoise: "var(--color-turquoise)", blue: "var(--color-blue)", + ochre: "var(--color-ochre)", }; export { Tag, tagVariants }; diff --git a/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx b/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx index edc5ba9eb15..2e09c6e8b68 100644 --- a/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx +++ b/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx @@ -74,7 +74,10 @@ interface MetricContainerChartProps { hideYAxis?: boolean; } -const customColorMap = { +// Fixed colors for metric sub-series (trace counts, cost, duration percentiles, token kinds). +// Only valid when no breakdown is applied — with a breakdown the line names are group values, +// not metric names, so these keys would hijack a group that happens to share a name. +const metricColorMap = { traces: COLOR_VARIANTS_MAP.purple.css, cost: COLOR_VARIANTS_MAP.purple.css, "duration.p50": COLOR_VARIANTS_MAP.turquoise.css, @@ -198,7 +201,11 @@ const MetricContainerChart = ({ ); }, [data, lines, isPending]); - const config = useChartConfig(lines, labelsMap, colorMap ?? customColorMap); + const config = useChartConfig( + lines, + labelsMap, + colorMap ?? (breakdown ? undefined : metricColorMap), + ); const labelActions = useMemo(() => { if (!getLabelAction) return undefined; From df4d2407d1e7593bbbd54c435af30e9f14f4913e Mon Sep 17 00:00:00 2001 From: Yaroslav Boiko Date: Tue, 18 Aug 2026 14:44:16 +0200 Subject: [PATCH 2/4] [OPIK-7839] [FE] design feedback: purple-dark palette entry, findable colour dot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design review on the PR asked for two things. Colour: the tenth palette entry is now `purple-dark` #491b7e from the product palette, replacing the ochre I had proposed. It keeps the property the change exists for — nearest neighbour ΔE 39 (`burgundy`, `purple`), well clear of the ΔE 14.5 pair being removed — and is better under colour-vision deficiency than ochre was (protanopia 23.2 vs 18.3). It does not clear the contrast floor on the dark ground: 1.55 against `#121212` where WCAG 1.4.11 asks 3:1, because at L* 23 it sits close to the dark theme's own background. That is recorded in the contrast test rather than hidden. Raising that test to the real 3:1 floor also exposed that `yellow` 1.92, `turquoise` 2.43, `green` 2.54 and `orange` 2.80 already fail it on the light ground, so five of ten entries are now listed as known. Half the palette failing is the finding: one palette cannot serve both themes, which is why two lightness bands are proposed in the follow-up. Colour dot: the 6px indicator was the reason customers reported the override as missing. Its pointer target is now enlarged by a transparent pseudo-element without changing how it looks, and its tooltip opens with no delay so the hint arrives the moment the dot is found. Horizontal growth is limited to 6px because in a chart legend the label sits immediately to the right and owns its own click action. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/constants/colorVariants.ts | 8 +++---- .../src/lib/colorVariants.test.ts | 24 ++++++++++++++----- apps/opik-frontend/src/main.scss | 10 ++++---- .../shared/ColorIndicator/ColorIndicator.tsx | 16 +++++++++++-- apps/opik-frontend/src/ui/tag.tsx | 9 ++++--- 5 files changed, 47 insertions(+), 20 deletions(-) diff --git a/apps/opik-frontend/src/constants/colorVariants.ts b/apps/opik-frontend/src/constants/colorVariants.ts index b6e6dc70a74..83e58d7ccdc 100644 --- a/apps/opik-frontend/src/constants/colorVariants.ts +++ b/apps/opik-frontend/src/constants/colorVariants.ts @@ -17,7 +17,7 @@ export type ColorVariant = (typeof COLOR_VARIANTS)[number]; export type ExtendedColorVariant = | ColorVariant | "primary" - | "ochre" + | "purpleDark" | "default"; export const COLOR_VARIANTS_MAP: Record< @@ -35,7 +35,7 @@ export const COLOR_VARIANTS_MAP: Record< turquoise: { css: "var(--color-turquoise)", hex: "#06b6d4" }, blue: { css: "var(--color-blue)", hex: "#3b82f6" }, primary: { css: "var(--color-primary)", hex: "#6366f1" }, - ochre: { css: "var(--color-ochre)", hex: "#8c683f" }, + purpleDark: { css: "var(--color-purple-dark)", hex: "#491b7e" }, default: { css: "var(--color-gray)", hex: "#64748b" }, }; @@ -45,10 +45,10 @@ export const PRESET_HEX_COLORS = COLOR_VARIANTS.map( export const DEFAULT_HEX_COLOR = COLOR_VARIANTS_MAP.blue.hex; -// `primary` and `ochre` are not in COLOR_VARIANTS (they are not picker presets) but are still +// `primary` and `purpleDark` are not in COLOR_VARIANTS (they are not picker presets) but are still // reachable as resolved colors, so their css vars must round-trip to hex for the color picker. export const CSS_VAR_TO_HEX: Record = Object.fromEntries( - [...COLOR_VARIANTS, "primary" as const, "ochre" as const].map((v) => [ + [...COLOR_VARIANTS, "primary" as const, "purpleDark" as const].map((v) => [ COLOR_VARIANTS_MAP[v].css, COLOR_VARIANTS_MAP[v].hex, ]), diff --git a/apps/opik-frontend/src/lib/colorVariants.test.ts b/apps/opik-frontend/src/lib/colorVariants.test.ts index 26012ff17a7..e9098c20f82 100644 --- a/apps/opik-frontend/src/lib/colorVariants.test.ts +++ b/apps/opik-frontend/src/lib/colorVariants.test.ts @@ -115,7 +115,7 @@ describe("automatic palette invariants", () => { "retriever-agent", ]), ).toEqual({ - "domain-agent": "var(--color-ochre)", + "domain-agent": "var(--color-purple-dark)", "supervisor-agent": "var(--color-blue)", "planner-agent": "var(--color-orange)", "retriever-agent": "var(--color-yellow)", @@ -133,11 +133,23 @@ describe("automatic palette invariants", () => { }); it("keeps entries legible as thin lines on both grounds", () => { - // `yellow` and `turquoise` predate this invariant and fail on the light ground at 1.92 and - // 2.43. They are tracked separately as a palette-contrast follow-up. The assertion below still - // blocks *new* low-contrast entries, and the allowlist must not grow. - const KNOWN_LOW_CONTRAST = ["yellow", "turquoise"]; - const MINIMUM_CONTRAST = 2.5; + // WCAG 1.4.11 asks for 3:1 and names "each line in a graph", so that is the floor here. + // Five of the ten entries do not meet it and are tracked as a palette-contrast follow-up: + // light ground — yellow 1.92, turquoise 2.43, green 2.54, orange 2.80 (all pre-existing) + // dark ground — purpleDark 1.55 (design-chosen; reads well on light, but is nearly the + // dark theme's own background) + // Half the palette failing is the finding, not an accident of this test: a single palette + // serving both themes cannot clear the floor on both, which is why the follow-up proposes two + // lightness bands. The assertion still blocks *new* entries from joining the list, and the + // list must not grow. + const KNOWN_LOW_CONTRAST = [ + "yellow", + "turquoise", + "green", + "orange", + "purpleDark", + ]; + const MINIMUM_CONTRAST = 3; const failing: string[] = []; for (const { variant, hex } of autoPaletteHexes()) { diff --git a/apps/opik-frontend/src/main.scss b/apps/opik-frontend/src/main.scss index fb05f4dfbc6..9a60e4bf4bd 100644 --- a/apps/opik-frontend/src/main.scss +++ b/apps/opik-frontend/src/main.scss @@ -205,7 +205,7 @@ --color-lime: #a3e635; --color-turquoise: #06b6d4; --color-blue: #3b82f6; - --color-ochre: #8c683f; + --color-purple-dark: #491b7e; --color-primary: #6366f1; --color-indigo: #6366f1; --color-violet: #8b5cf6; @@ -250,8 +250,8 @@ --tag-blue-text: #19426b; --tag-lavender-bg: #e5e5fe; --tag-lavender-text: #3b3b8e; - --tag-ochre-bg: #f2e6d8; - --tag-ochre-text: #5c4426; + --tag-purple-dark-bg: #e4d7f7; + --tag-purple-dark-text: #3d1669; --click-blue: #262ab5; @@ -520,8 +520,8 @@ --tag-blue-text: #6eabe7; --tag-lavender-bg: #2a2a3d; --tag-lavender-text: #a8a8e0; - --tag-ochre-bg: #2f2b26; - --tag-ochre-text: #c9975c; + --tag-purple-dark-bg: #2a2233; + --tag-purple-dark-text: #b98ce6; /* Trace/span type colors - Dark Mode */ --type-trace: #945fcf; diff --git a/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx index c2d5c96ef22..25b1ba83ccd 100644 --- a/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx +++ b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx @@ -22,6 +22,15 @@ const colorIndicatorVariants = cva("bg-[var(--bg-color)]", { }, }); +/** + * The dot is 6px, which is too small to find or aim at, so customers reported the colour override as + * missing entirely (OPIK_7840). A transparent pseudo-element enlarges the pointer target without + * changing how the indicator looks. Horizontal growth is kept to 6px because in a chart legend the + * dot sits immediately left of the label, and the label has its own click action to drill down. + */ +const HIT_AREA = + "before:absolute before:-inset-y-2 before:-inset-x-1.5 before:content-['']"; + type ColorIndicatorProps = VariantProps & { label: string; colorKey?: string; @@ -83,11 +92,14 @@ const ColorIndicator: React.FC = ({ return ( - + {/* No hover delay: the affordance is small, so the hint has to arrive the moment it is found. */} +
e.stopPropagation()} /> diff --git a/apps/opik-frontend/src/ui/tag.tsx b/apps/opik-frontend/src/ui/tag.tsx index b801c1f1644..309a4d01900 100644 --- a/apps/opik-frontend/src/ui/tag.tsx +++ b/apps/opik-frontend/src/ui/tag.tsx @@ -19,7 +19,8 @@ const tagVariants = cva("inline-block truncate rounded-sm transition-colors", { turquoise: "bg-[var(--tag-turquoise-bg)] text-[var(--tag-turquoise-text)]", blue: "bg-[var(--tag-blue-bg)] text-[var(--tag-blue-text)]", - ochre: "bg-[var(--tag-ochre-bg)] text-[var(--tag-ochre-text)]", + purpleDark: + "bg-[var(--tag-purple-dark-bg)] text-[var(--tag-purple-dark-text)]", lavender: "bg-[var(--tag-lavender-bg)] text-[var(--tag-lavender-text)]", white: "border border-gray-200 bg-white text-muted-slate dark:border-gray-600 dark:bg-gray-800 dark:text-foreground", @@ -72,12 +73,14 @@ Tag.displayName = "Tag"; * re-maps only the labels that resolved to that slot. * * `primary` is deliberately absent: it is too close to both `purple` and `blue` for chart series. + * The tenth entry is design-owned (`purple-dark`, from the product palette); see colorVariants.test.ts + * for the invariants any replacement must still satisfy. */ export const TAG_VARIANTS: Exclude< TagProps["variant"], "red" | "transparent" | "white" | "lavender" >[] = [ - "ochre", + "purpleDark", "gray", "purple", "burgundy", @@ -107,7 +110,7 @@ export const TAG_VARIANTS_COLOR_MAP: Record< green: "var(--color-green)", turquoise: "var(--color-turquoise)", blue: "var(--color-blue)", - ochre: "var(--color-ochre)", + purpleDark: "var(--color-purple-dark)", }; export { Tag, tagVariants }; From 24e8b8c804a2e5c326eb72462abbafa4f3faf6da Mon Sep 17 00:00:00 2001 From: Yaroslav Boiko Date: Tue, 18 Aug 2026 15:50:57 +0200 Subject: [PATCH 3/4] [OPIK-7839] [FE] address review: constant naming, boundary and interaction tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `metricColorMap` -> `METRIC_COLOR_MAP`, matching the sibling `METRIC_CHART_TYPE` and the repo convention for module-level fixed configuration. - Pin the palette length. The golden test samples four labels, so a length change that only moves other labels would have passed it, while re-colouring the whole product. - Pin the design-approved `purple-dark` hex. It is excused from the contrast assertion, so without this the value could drift to something that assertion would have rejected. - Cover the boundaries of `resolveChartColorMap` (empty list, empty-string label) and `resolveHexColor` directly (already-hex passthrough, unknown token passthrough). An empty-string group value returning no colour makes recharts fall back to black. - New `ColorIndicator.test.tsx`: clicking the indicator opens the picker, and does not fire a surrounding click handler — in a legend that handler navigates to filtered traces, so the two must not both fire. Also asserts the pointer target is larger than the 6px dot. Also restores a class order in the chart container that an earlier autofix had reordered without cause; that line is unrelated to this change. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/lib/colorVariants.test.ts | 32 +++++++++ .../ColorIndicator/ColorIndicator.test.tsx | 68 +++++++++++++++++++ .../MetricChart/MetricChartContainer.tsx | 4 +- 3 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.test.tsx diff --git a/apps/opik-frontend/src/lib/colorVariants.test.ts b/apps/opik-frontend/src/lib/colorVariants.test.ts index e9098c20f82..27bf2bf19ca 100644 --- a/apps/opik-frontend/src/lib/colorVariants.test.ts +++ b/apps/opik-frontend/src/lib/colorVariants.test.ts @@ -94,6 +94,22 @@ describe("automatic palette invariants", () => { expect(tooClose).toEqual([]); }); + it("keeps the palette at ten entries", () => { + // The automatic color is `hash % TAG_VARIANTS.length`. The golden test below samples four + // labels, which would not notice a length change that only moves labels it does not sample, + // so the length is pinned separately. Changing it re-colors every label in the product. + expect(TAG_VARIANTS).toHaveLength(10); + }); + + it("pins the design-approved hex for the entry excused from the contrast check", () => { + // `purpleDark` is skipped by the contrast assertion below, so without this the approved value + // could drift to something the check would otherwise have rejected. + expect(COLOR_VARIANTS_MAP.purpleDark.hex.toLowerCase()).toBe("#491b7e"); + expect(resolveHexColor(COLOR_VARIANTS_MAP.purpleDark.css)).toBe( + COLOR_VARIANTS_MAP.purpleDark.hex, + ); + }); + it("never assigns the indigo that was confusable with purple and blue", () => { const indigo = COLOR_VARIANTS_MAP.primary.hex.toLowerCase(); @@ -217,6 +233,22 @@ describe("resolveChartColorMap", () => { expect(Object.keys(map).sort()).toEqual(labels.sort()); }); + it("returns an empty map for an empty label list", () => { + expect(resolveChartColorMap([])).toEqual({}); + }); + + it("resolves an empty-string label to a palette color rather than undefined", () => { + // Group-by values can be empty strings; a missing color makes recharts fall back to black. + expect(resolveChartColorMap([""])[""]).toMatch(/^var\(--color-/); + }); + + it("leaves a value that is already hex untouched and passes unknown values through", () => { + expect(resolveHexColor("#123abc")).toBe("#123abc"); + expect(resolveHexColor("var(--color-not-a-real-token)")).toBe( + "var(--color-not-a-real-token)", + ); + }); + it("pins only the labels the caller names, leaving the rest automatic", () => { // How a chart fixes colors for specific series (metric sub-series today) without disturbing // labels it says nothing about. diff --git a/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.test.tsx b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.test.tsx new file mode 100644 index 00000000000..0e214814f37 --- /dev/null +++ b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.test.tsx @@ -0,0 +1,68 @@ +import { describe, expect, it, vi } from "vitest"; +import { fireEvent, render, screen } from "@testing-library/react"; + +import ColorIndicator from "./ColorIndicator"; +import { TooltipProvider } from "@/ui/tooltip"; + +// The indicator's own persistence path is not what this file is about: it covers the interaction +// contract of the nested Radix triggers, which is what customers hit when they try to recolour a +// series (OPIK_7840 — the control was reported as missing because it was hard to reach). +vi.mock("@/hooks/useUpdateColorMapping", () => ({ + default: () => ({ + updateColor: vi.fn(), + previewColor: {}, + setPreviewColor: vi.fn(), + isPending: false, + }), +})); + +const renderIndicator = (onParentClick?: () => void) => { + const utils = render( + +
+ +
+
, + ); + + // The indicator renders no text, so the trigger is found by the role Radix gives it. + const trigger = utils.container.querySelector( + "[data-state]", + ) as HTMLElement | null; + + if (!trigger) throw new Error("colour indicator trigger not rendered"); + + return { ...utils, trigger }; +}; + +describe("ColorIndicator", () => { + it("opens the colour picker when the indicator is clicked", async () => { + const { trigger } = renderIndicator(); + + fireEvent.click(trigger); + + // The hex field is the picker's only stable, user-visible landmark. + expect(await screen.findByPlaceholderText("#000000")).toBeInTheDocument(); + }); + + it("does not fire a surrounding click handler when the indicator is clicked", async () => { + // In a chart legend the label around the dot navigates to filtered traces. Opening the picker + // must not also trigger that navigation. + const onParentClick = vi.fn(); + const { trigger } = renderIndicator(onParentClick); + + fireEvent.click(trigger); + + await screen.findByPlaceholderText("#000000"); + expect(onParentClick).not.toHaveBeenCalled(); + }); + + it("gives the indicator a pointer target larger than the dot itself", () => { + // The dot is 6px. Without the enlarged target it is effectively unhittable, which is why the + // feature read as missing. The dot's own size stays unchanged. + const { trigger } = renderIndicator(); + + expect(trigger.className).toContain("size-1.5"); + expect(trigger.className).toMatch(/before:-inset-/); + }); +}); diff --git a/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx b/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx index 2e09c6e8b68..3a2fa83ec57 100644 --- a/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx +++ b/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx @@ -77,7 +77,7 @@ interface MetricContainerChartProps { // Fixed colors for metric sub-series (trace counts, cost, duration percentiles, token kinds). // Only valid when no breakdown is applied — with a breakdown the line names are group values, // not metric names, so these keys would hijack a group that happens to share a name. -const metricColorMap = { +const METRIC_COLOR_MAP = { traces: COLOR_VARIANTS_MAP.purple.css, cost: COLOR_VARIANTS_MAP.purple.css, "duration.p50": COLOR_VARIANTS_MAP.turquoise.css, @@ -204,7 +204,7 @@ const MetricContainerChart = ({ const config = useChartConfig( lines, labelsMap, - colorMap ?? (breakdown ? undefined : metricColorMap), + colorMap ?? (breakdown ? undefined : METRIC_COLOR_MAP), ); const labelActions = useMemo(() => { From e028d389591ebcd3ea590fa23f553b1d81d7b49f Mon Sep 17 00:00:00 2001 From: comet-qa-bot Date: Tue, 18 Aug 2026 14:34:58 +0000 Subject: [PATCH 4/4] [OPIK-7839] [QA] e2e: chart series colours, grouped vs ungrouped Two specs from the exploration of #7873, under a new tests/dashboards/ directory (dashboards had no e2e coverage at all). - Grouped by Tags: every group takes its tag-palette colour, the five are distinct, and the groups named after metric keys (`traces`, `cost`) are not painted the metric colour. This is the regression the PR fixes. - No grouping: the fixed metric colour map survives, on both the trace count series and the duration percentiles. That is the other side of the same `breakdown ? undefined : METRIC_COLOR_MAP` guard, which would silently drop the fixed colours product-wide if it over-fired. Supporting changes: - backend client: createDashboard / deleteDashboard / getProjectMetricSeries, behind a shared privateFetch helper for the endpoints the pinned TS SDK does not model (getProjectStats now uses it too). - seriesColorsDashboard fixture: seeds tagged traces and a dashboard carrying the three widgets, verifies via the API that the groups really exist before the browser opens, and deletes the dashboard afterwards (dashboards are outside the run-prefix sweep). - dashboard page object: reads legend swatch and SVG series colours as computed hex. - taxonomy: dashboards.configure-widget -> covered. Generated by the release QA side flow; needs review before merge. Co-Authored-By: Claude Opus 5 --- tests_end_to_end/coverage/taxonomy.yaml | 4 +- tests_end_to_end/e2e/core/backend/client.ts | 152 ++++++++++++++-- tests_end_to_end/e2e/core/backend/index.ts | 2 + tests_end_to_end/e2e/fixtures/index.ts | 11 +- .../series-colors-dashboard.fixture.ts | 171 ++++++++++++++++++ tests_end_to_end/e2e/pom/dashboard.page.ts | 139 ++++++++++++++ .../dashboards/chart-series-colors.spec.ts | 159 ++++++++++++++++ 7 files changed, 621 insertions(+), 17 deletions(-) create mode 100644 tests_end_to_end/e2e/fixtures/series-colors-dashboard.fixture.ts create mode 100644 tests_end_to_end/e2e/pom/dashboard.page.ts create mode 100644 tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts diff --git a/tests_end_to_end/coverage/taxonomy.yaml b/tests_end_to_end/coverage/taxonomy.yaml index 8cf71c3b996..5d7453ef7ee 100644 --- a/tests_end_to_end/coverage/taxonomy.yaml +++ b/tests_end_to_end/coverage/taxonomy.yaml @@ -363,7 +363,9 @@ areas: label: Dashboards nav_group: Observability routes: ["/$workspaceName/dashboards", "/dashboards/$dashboardId", "/projects/$projectId/dashboards"] + spec_dir: dashboards specs: + - dashboards/chart-series-colors.spec.ts capabilities: list-dashboards: { covered: false } create-dashboard: { covered: false } @@ -371,7 +373,7 @@ areas: edit-dashboard: { covered: false } delete-dashboard: { covered: false } add-widget: { covered: false } - configure-widget: { covered: false } + configure-widget: { covered: true, tier: t2-cuj, note: "grouped vs ungrouped series colours; the widget is configured through the API, the editor UI itself is still uncovered" } widget-filters: { covered: false } metric-date-range: { covered: false } diff --git a/tests_end_to_end/e2e/core/backend/client.ts b/tests_end_to_end/e2e/core/backend/client.ts index cecfca384c2..eb3807cbe81 100644 --- a/tests_end_to_end/e2e/core/backend/client.ts +++ b/tests_end_to_end/e2e/core/backend/client.ts @@ -97,6 +97,21 @@ export interface AnnotationQueueDetail { reviewers: AnnotationQueueReviewerRef[]; } +export interface DashboardRef { + id: string; + name: string; +} + +/** + * One series of a project-metrics response — the shape a chart widget turns + * into a line. `name` is the metric sub-series (`traces`, `duration.p50`) or, + * when the request carries a breakdown, the group value (a tag, a trace name). + */ +export interface ProjectMetricSeriesRef { + name: string; + points: Array<{ time: string; value: number | null }>; +} + export interface OptimizationRef { id: string; name: string; @@ -124,6 +139,37 @@ export function makeBackendClient(apiKey: string | null = null) { apiUrl: env.apiBaseUrl, }); + /** + * A raw call to a private REST endpoint the pinned TS SDK does not model. + * Everything the SDK *does* expose goes through `opik.api.*` — this is only + * for the gaps (dashboards, the windowed/breakdown metric requests). + */ + const privateFetch = async ( + method: 'GET' | 'POST' | 'DELETE', + path: string, + body?: unknown, + ): Promise => { + const headers: Record = { + Accept: 'application/json', + 'Comet-Workspace': env.workspace, + }; + if (body !== undefined) headers['Content-Type'] = 'application/json'; + const key = apiKey ?? env.apiKey; + if (key) headers['Authorization'] = key; + + const res = await fetch(`${env.apiBaseUrl}${path}`, { + method, + headers, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }); + if (!res.ok) { + throw new Error( + `${method} ${path} -> ${res.status}: ${(await res.text()).slice(0, 300)}`, + ); + } + return res; + }; + // Hoisted so the poll helpers (free functions) can call it without depending // on the not-yet-constructed return object. const localGetOptimization = async (id: string): Promise => { @@ -314,21 +360,7 @@ export function makeBackendClient(apiKey: string | null = null) { if (args.fromTime) params.set('from_time', args.fromTime.toISOString()); if (args.toTime) params.set('to_time', args.toTime.toISOString()); - const headers: Record = { - Accept: 'application/json', - 'Comet-Workspace': env.workspace, - }; - const key = apiKey ?? env.apiKey; - if (key) headers['Authorization'] = key; - - const res = await fetch(`${env.apiBaseUrl}/v1/private/projects/stats?${params}`, { - headers, - }); - if (!res.ok) { - throw new Error( - `GET /v1/private/projects/stats -> ${res.status}: ${(await res.text()).slice(0, 300)}`, - ); - } + const res = await privateFetch('GET', `/v1/private/projects/stats?${params}`); const body = (await res.json()) as { content?: Array<{ project_id?: string; @@ -352,6 +384,87 @@ export function makeBackendClient(apiKey: string | null = null) { })); }, + /** + * The series a project-metrics chart widget would draw — the exact call + * `useProjectMetric` makes, so a spec can check the data really groups the + * way it expects before opening a browser to look at the chart. + * + * `breakdownField` is a `BREAKDOWN_FIELD` value (`tags`, `name`, …). + * Omitting it requests the ungrouped series, where `name` is the metric + * sub-series rather than a group value. + */ + async getProjectMetricSeries(args: { + projectId: string; + metricType: string; + interval: 'HOURLY' | 'DAILY' | 'WEEKLY' | 'TOTAL'; + intervalStart: Date; + intervalEnd: Date; + breakdownField?: string; + }): Promise { + const res = await privateFetch( + 'POST', + `/v1/private/projects/${args.projectId}/metrics`, + { + metric_type: args.metricType, + interval: args.interval, + interval_start: args.intervalStart.toISOString(), + interval_end: args.intervalEnd.toISOString(), + ...(args.breakdownField ? { breakdown: { field: args.breakdownField } } : {}), + }, + ); + const body = (await res.json()) as { + results?: Array<{ name?: string; data?: Array<{ time: string; value: number | null }> }>; + }; + return (body.results ?? []).map((r) => ({ + name: String(r.name ?? ''), + points: r.data ?? [], + })); + }, + + /** + * Creates a dashboard from a full config document — the same payload the + * FE's `useDashboardCreateMutation` posts, so a widget can be seeded + * already configured instead of being assembled through the editor UI. + * + * The id only comes back in the `Location` header (the endpoint answers + * 201 with an empty body), which is what `extractIdFromLocation` reads. + */ + async createDashboard(args: { + name: string; + type: 'multi_project' | 'experiments'; + config: unknown; + description?: string; + }): Promise { + const res = await privateFetch('POST', '/v1/private/dashboards', { + name: args.name, + type: args.type, + config: args.config, + ...(args.description ? { description: args.description } : {}), + }); + const location = res.headers.get('location'); + const id = location?.split('/').pop(); + if (!id) { + throw new Error( + `POST /v1/private/dashboards returned no usable Location header: ${location}`, + ); + } + return { id, name: args.name }; + }, + + /** + * Dashboards are not swept by `global-teardown` (it only knows + * experiments, datasets and projects) and do not cascade with the project + * their widgets point at, so whatever creates one must delete it. + */ + async deleteDashboard(id: string): Promise { + try { + await privateFetch('DELETE', `/v1/private/dashboards/${id}`); + } catch (err) { + if (isNotFoundMessage(err)) return; + throw err; + } + }, + async findExperimentByName(name: string): Promise { const page = await opik.api.experiments.findExperiments({ name, size: 50 }); const content = page.content ?? []; @@ -553,6 +666,15 @@ export function makeBackendClient(apiKey: string | null = null) { }; } +/** + * The 404 check for `privateFetch`, which throws a plain Error carrying the + * status rather than the SDK's structured `statusCode` — used so a delete of + * an already-deleted entity stays a no-op, exactly as the SDK-backed deletes do. + */ +function isNotFoundMessage(err: unknown): boolean { + return err instanceof Error && / -> 404:/.test(err.message); +} + function isNotFoundError(err: unknown): boolean { return ( typeof err === 'object' && diff --git a/tests_end_to_end/e2e/core/backend/index.ts b/tests_end_to_end/e2e/core/backend/index.ts index 4b1336541d8..8241932bc32 100644 --- a/tests_end_to_end/e2e/core/backend/index.ts +++ b/tests_end_to_end/e2e/core/backend/index.ts @@ -14,5 +14,7 @@ export { type AutomationRuleRef, type AnnotationQueueDetail, type AnnotationQueueReviewerRef, + type DashboardRef, + type ProjectMetricSeriesRef, } from './client'; export { type PollFeedbackScoreOpts } from './poll-feedback-score'; diff --git a/tests_end_to_end/e2e/fixtures/index.ts b/tests_end_to_end/e2e/fixtures/index.ts index 66e1201c29b..e5858f225ad 100644 --- a/tests_end_to_end/e2e/fixtures/index.ts +++ b/tests_end_to_end/e2e/fixtures/index.ts @@ -1,4 +1,4 @@ -export { test, expect } from './filterable-traces.fixture'; +export { test, expect } from './series-colors-dashboard.fixture'; export type { ProjectFixtures } from './project.fixture'; export type { ScratchDir, ScratchDirFixtures } from './scratch-dir.fixture'; export type { @@ -50,4 +50,13 @@ export type { FilterableTraceRef, FilterableTracesFixtures, } from './filterable-traces.fixture'; +export type { + SeriesColorsDashboardRef, + SeriesColorsDashboardFixtures, +} from './series-colors-dashboard.fixture'; +export { + SERIES_COLORS_TAGS, + SERIES_COLORS_TIME_RANGE, + SERIES_COLORS_WIDGETS, +} from './series-colors-dashboard.fixture'; export type { ProjectRef } from '../core/backend'; diff --git a/tests_end_to_end/e2e/fixtures/series-colors-dashboard.fixture.ts b/tests_end_to_end/e2e/fixtures/series-colors-dashboard.fixture.ts new file mode 100644 index 00000000000..91b9a3c0fd7 --- /dev/null +++ b/tests_end_to_end/e2e/fixtures/series-colors-dashboard.fixture.ts @@ -0,0 +1,171 @@ +import { expect as baseExpect } from '@playwright/test'; +import { test as baseTest } from './filterable-traces.fixture'; +import { shouldLeaveArtifacts } from '../core/artifacts'; + +/** Widget titles, which are also how a spec addresses a widget on the page. */ +export const SERIES_COLORS_WIDGETS = { + groupedByTag: 'Traces grouped by tag', + ungrouped: 'Traces with no grouping', + duration: 'Trace duration percentiles', +} as const; + +/** + * Tag values carried by the seeded traces, one per trace, so each becomes its + * own group when the chart is grouped by Tags. + * + * `traces` and `cost` are chosen deliberately: they collide with keys of the + * chart's fixed metric colour map, which is what makes them able to detect a + * grouped chart resolving group colours through that map instead of through + * the tag palette. The rest are ordinary values that hash to distinct slots. + */ +export const SERIES_COLORS_TAGS = ['traces', 'cost', 'beta', 'zeta', 'iota'] as const; + +/** + * Ages, in days, of the traces seeded per tag. Three points spread over three + * days so a DAILY chart draws an actual line per group: recharts renders no + * line curve at all for a single-point series, only a dot. + */ +const TRACE_AGES_DAYS = [0.2, 1.2, 2.2]; + +/** Distinct durations so the p50/p90/p99 series are not the same number. */ +const TRACE_DURATIONS_SECONDS = [0.5, 2, 8]; + +/** + * The window the spec pins on the dashboard URL. Wide enough to hold every + * seeded trace, and > 3 days so the chart buckets DAILY rather than HOURLY. + */ +export const SERIES_COLORS_TIME_RANGE = 'past7days'; +const TIME_RANGE_DAYS = 7; + +export interface SeriesColorsDashboardRef { + /** The dashboard holding the three widgets, ready to open. */ + id: string; + name: string; + /** Group values present on the grouped chart, one per seeded tag. */ + groups: string[]; +} + +export interface SeriesColorsDashboardFixtures { + seriesColorsDashboard: SeriesColorsDashboardRef; +} + +/** + * A project of tagged traces plus a dashboard whose widgets read it, seeded + * entirely through the API so the browser only ever has to *look* at charts. + * + * The three widgets cover both sides of the "is a fixed metric colour map + * applied?" question on one page: one chart grouped by Tags (colours must come + * from the tag palette), and two ungrouped ones (colours must come from the + * fixed map). + * + * The widgets are configured in the created config rather than through the + * widget editor. That keeps the spec's subject the rendering, not the editor — + * and it avoids the editor's own default of switching a newly grouped widget + * to Total aggregation, which collapses every series to a single point. + */ +export const test = baseTest.extend({ + seriesColorsDashboard: async ( + { sdkClient, backendClient, project, testNamespace }, + use, + testInfo, + ) => { + for (const tag of SERIES_COLORS_TAGS) { + for (const [index, ageDays] of TRACE_AGES_DAYS.entries()) { + await sdkClient.python.createNestedTrace({ + project_name: project.name, + name: `${testNamespace}-${tag}-${index}`, + input: { query: tag }, + output: { answer: tag }, + tags: [tag], + age_days: ageDays, + duration_seconds: TRACE_DURATIONS_SECONDS[index], + spans: [], + }); + } + } + + // Prove the seed really produced one group per tag before anything opens a + // browser. Without this a chart that renders nothing — because ingestion + // had not landed, or because the grouping silently returned a single + // series — would make the UI assertions unreachable rather than failing. + const window = { + intervalStart: new Date(Date.now() - TIME_RANGE_DAYS * 24 * 60 * 60 * 1000), + intervalEnd: new Date(), + }; + await baseExpect + .poll( + async () => { + const series = await backendClient.getProjectMetricSeries({ + projectId: project.id, + metricType: 'TRACE_COUNT', + interval: 'DAILY', + breakdownField: 'tags', + ...window, + }); + return series.map((s) => s.name).sort(); + }, + { timeout: 60_000 }, + ) + .toEqual([...SERIES_COLORS_TAGS].sort()); + + const widget = (id: string, title: string, config: Record) => ({ + id, + title, + type: 'project_metrics', + config: { projectId: project.id, chartType: 'line', traceFilters: [], ...config }, + }); + + const created = await backendClient.createDashboard({ + name: `${testNamespace}-dash`, + type: 'multi_project', + config: { + version: 4, + sections: [ + { + id: 'series-colors', + title: 'Series colours', + widgets: [ + widget('w-grouped', SERIES_COLORS_WIDGETS.groupedByTag, { + metricType: 'TRACE_COUNT', + breakdown: { field: 'tags' }, + }), + widget('w-ungrouped', SERIES_COLORS_WIDGETS.ungrouped, { + metricType: 'TRACE_COUNT', + breakdown: { field: 'none' }, + }), + widget('w-duration', SERIES_COLORS_WIDGETS.duration, { + metricType: 'DURATION', + }), + ], + layout: [ + { i: 'w-grouped', x: 0, y: 0, w: 3, h: 5 }, + { i: 'w-ungrouped', x: 3, y: 0, w: 3, h: 5 }, + { i: 'w-duration', x: 0, y: 5, w: 3, h: 5 }, + ], + }, + ], + lastModified: 0, + }, + }); + + await testInfo.attach('opik.seriesColorsDashboard', { + body: JSON.stringify({ id: created.id, name: created.name }, null, 2), + contentType: 'application/json', + }); + + await use({ id: created.id, name: created.name, groups: [...SERIES_COLORS_TAGS] }); + + // The traces cascade with the project fixture's own teardown; the + // dashboard does not belong to the project and is outside the run-prefix + // sweep, so it has to be deleted here. + if (!shouldLeaveArtifacts(testInfo)) { + try { + await backendClient.deleteDashboard(created.id); + } catch (err) { + console.warn(`[seriesColorsDashboard fixture] delete warning for ${created.name}:`, err); + } + } + }, +}); + +export { expect } from './filterable-traces.fixture'; diff --git a/tests_end_to_end/e2e/pom/dashboard.page.ts b/tests_end_to_end/e2e/pom/dashboard.page.ts new file mode 100644 index 00000000000..a302815850b --- /dev/null +++ b/tests_end_to_end/e2e/pom/dashboard.page.ts @@ -0,0 +1,139 @@ +import { test, expect } from '@playwright/test'; +import type { Page, Locator } from '@playwright/test'; +import { loadEnvConfig } from '../config/env.config'; + +/** + * A workspace dashboard — the grid of widgets at + * `//dashboards/`. + * + * Everything here is about the *colours* a chart widget draws, because that is + * the part of a chart a user reads without any text: two series painted the + * same colour is a chart that is silently wrong. Two independent renderings of + * the same resolved colour are exposed, and a spec should assert both — the + * legend swatch (what the user matches a name against) and the SVG series + * stroke (what the user actually traces with their eye). They come from the + * same chart config, so agreeing is not interesting; disagreeing would be. + * + * Colours are read as *computed* values and normalised to hex rather than read + * off the `stroke`/`--bg-color` attribute. The attribute holds a CSS variable + * (`var(--color-purple)`), so asserting on it would pin the variable name and + * pass happily if the variable itself were redefined to another colour. + * + * Selector note: neither the widget card nor the legend carries a + * `data-testid`, so widgets are addressed by their (spec-controlled) title and + * legend entries by their label. `.react-grid-item` and the recharts curve + * classes are library-owned class names. A `data-testid` on the widget card + * and on the legend colour indicator would let this drop the CSS entirely. + */ +export class DashboardPage { + constructor(private readonly page: Page) {} + + /** + * Opens a dashboard with its date range pinned on the URL. The range is + * otherwise remembered in localStorage, so leaving it unset would let one + * run's choice decide another run's chart. + */ + async goto(dashboardId: string, timeRange: string): Promise { + return test.step(`Open dashboard ${dashboardId}`, async () => { + const env = loadEnvConfig(); + await this.page.goto( + `${env.baseUrl}/${env.workspace}/dashboards/${dashboardId}?dashboard_time_range=${timeRange}`, + ); + }); + } + + /** + * Waits until a widget has drawn `seriesCount` series, and asserts it drew + * exactly that many. Charts render their axes before the data arrives, so + * waiting on the chart alone would let a spec read an empty legend. + */ + async waitForChart(widgetTitle: string, seriesCount: number): Promise { + return test.step(`Wait for "${widgetTitle}" to draw ${seriesCount} series`, async () => { + const widget = this.widget(widgetTitle); + await expect(widget).toHaveCount(1); + await expect(widget.locator('.recharts-surface')).toBeVisible(); + await expect(this.legendSwatches(widgetTitle)).toHaveCount(seriesCount); + await expect(this.seriesCurves(widgetTitle)).toHaveCount(seriesCount); + }); + } + + /** The card of the widget whose header reads exactly `title`. */ + widget(title: string): Locator { + return this.page + .locator('.react-grid-item') + .filter({ has: this.page.getByText(title, { exact: true }) }); + } + + /** + * The colour of a legend entry's swatch, as `#rrggbb`. + * + * The entry is addressed by its label, anchored and exact: a substring match + * on `cost` would also match a group named `cost-usd`. + */ + async legendColor(widgetTitle: string, label: string): Promise { + return test.step(`Read the "${label}" legend colour of "${widgetTitle}"`, async () => { + const swatch = this.legendItem(widgetTitle, label).locator(LEGEND_SWATCH); + await expect(swatch).toHaveCount(1); + return toHex(await swatch.evaluate((el) => getComputedStyle(el).backgroundColor)); + }); + } + + /** + * The stroke colour of every series line the widget drew, as `#rrggbb`, in + * DOM order. + * + * Deliberately the whole set rather than a lookup per series: recharts does + * not label a rendered curve with the series it belongs to, and the property + * this is here to protect is a property of the set — that no two series share + * a colour. A per-series read would not be able to see a collision at all. + */ + async seriesColors(widgetTitle: string): Promise { + return test.step(`Read the series colours of "${widgetTitle}"`, async () => { + const strokes = await this.seriesCurves(widgetTitle).evaluateAll((els) => + els.map((el) => getComputedStyle(el).stroke), + ); + return strokes.map(toHex); + }); + } + + private legendItem(widgetTitle: string, label: string): Locator { + return this.widget(widgetTitle) + .locator(`div:has(> ${LEGEND_SWATCH})`) + .filter({ hasText: new RegExp(`^\\s*${escapeRegExp(label)}\\s*$`) }); + } + + private legendSwatches(widgetTitle: string): Locator { + return this.widget(widgetTitle).locator(LEGEND_SWATCH); + } + + /** + * A single-series chart is drawn as a filled area and a multi-series one as + * lines, so both curve classes count as "a series". + */ + private seriesCurves(widgetTitle: string): Locator { + return this.widget(widgetTitle).locator( + '.recharts-line-curve, .recharts-area-curve', + ); + } +} + +/** + * The chart legend's colour dot. It carries no testid and no text; what marks + * it out is the inline custom property the resolved series colour is written + * to, which is also what its background renders from. + */ +const LEGEND_SWATCH = 'div[style*="--bg-color"]'; + +/** `rgb(16, 185, 129)` -> `#10b981`. Anything else is returned unchanged. */ +function toHex(color: string): string { + const match = color.match(/^rgba?\((\d+),\s*(\d+),\s*(\d+)/); + if (!match) return color; + return `#${match + .slice(1, 4) + .map((v) => Number(v).toString(16).padStart(2, '0')) + .join('')}`; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts b/tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts new file mode 100644 index 00000000000..b8b668e57b4 --- /dev/null +++ b/tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts @@ -0,0 +1,159 @@ +import { + test, + expect, + SERIES_COLORS_TIME_RANGE, + SERIES_COLORS_WIDGETS, +} from '@e2e/fixtures'; +import { DashboardPage } from '@e2e/pom/dashboard.page'; + +/** + * How a project-metrics chart resolves the colour of each series. + * + * There are two sources, and which one applies depends on whether the widget + * is grouped: + * + * - **Ungrouped**, the series are the metric's own sub-series (`traces`, + * `duration.p50`, …), and each has a fixed colour so the same metric looks + * the same everywhere in the product. + * - **Grouped**, the series are *group values* — a tag, a trace name — and + * their colours come from the tag palette, `hash(label) % palette.length`, + * the same way a tag chip is coloured elsewhere. + * + * Applying the fixed map to a grouped chart is the bug these tests exist to + * catch: a group whose value happens to equal a metric key (`traces`, `cost`) + * takes that metric's colour, so several unrelated groups end up painted + * identically. Nothing errors and no value is wrong — the chart is simply + * unreadable, which is why it needs a test rather than a bug report. + * + * Both directions are asserted, because the guard can fail either way: not + * applying it where it belongs would silently drop the fixed colours from + * every ungrouped chart in the product. + * + * The expected colours are the product's palette, pinned as literal hex. That + * is the point: a spec that recomputed the hash from the palette constants + * would agree with any palette, including a broken one. + */ +const PALETTE = { + gray: '#64748b', + purple: '#8b5cf6', + purpleDark: '#491b7e', + burgundy: '#bf399e', + green: '#10b981', + turquoise: '#06b6d4', +} as const; + +/** Where each seeded tag lands in the tag palette, by its own hash. */ +const EXPECTED_GROUP_COLORS: Record = { + traces: PALETTE.green, + cost: PALETTE.purpleDark, + beta: PALETTE.purple, + zeta: PALETTE.turquoise, + iota: PALETTE.gray, +}; + +/** The fixed colours an *ungrouped* chart must keep using. */ +const METRIC_COLORS = { + traces: PALETTE.purple, + 'duration.p50': PALETTE.turquoise, + 'duration.p90': PALETTE.burgundy, + 'duration.p99': PALETTE.purple, +} as const; + +test.describe('Dashboards — chart series colours', { tag: ['@area:dashboards'] }, () => { + // Widgets are laid out on a grid; a narrow viewport stacks them and shrinks + // each chart until recharts drops the legend. + test.use({ viewport: { width: 1600, height: 1200 } }); + + test( + 'A chart grouped by tags colours every group from the tag palette, never from the fixed metric colours', + { tag: ['@t2-cuj', '@cap:dashboards.configure-widget'] }, + async ({ seriesColorsDashboard, page }) => { + const dashboard = new DashboardPage(page); + const { groupedByTag } = SERIES_COLORS_WIDGETS; + const groups = seriesColorsDashboard.groups; + + await test.step('Open the dashboard and wait for the grouped chart', async () => { + await dashboard.goto(seriesColorsDashboard.id, SERIES_COLORS_TIME_RANGE); + await dashboard.waitForChart(groupedByTag, groups.length); + }); + + await test.step('Each group takes the palette colour its own label hashes to', async () => { + for (const group of groups) { + expect(await dashboard.legendColor(groupedByTag, group), group).toBe( + EXPECTED_GROUP_COLORS[group], + ); + } + }); + + await test.step('The drawn lines carry those same colours, and no two share one', async () => { + const drawn = await dashboard.seriesColors(groupedByTag); + // Compared as sets: recharts does not say which curve is which series, + // and it is the set that has to be right — as many distinct colours as + // there are groups, and exactly the ones the legend advertises. + expect(drawn).toHaveLength(groups.length); + expect(new Set(drawn).size).toBe(groups.length); + expect([...drawn].sort()).toEqual( + groups.map((g) => EXPECTED_GROUP_COLORS[g]).sort(), + ); + }); + + await test.step('The groups named after metric keys are not painted the metric colour', async () => { + // The regression itself: with the fixed map applied, `traces` and + // `cost` would both be METRIC_COLORS.traces, and so would `beta`, + // which hashes there — three identical lines in a five-line chart. + expect(await dashboard.legendColor(groupedByTag, 'traces')).not.toBe( + METRIC_COLORS.traces, + ); + expect(await dashboard.legendColor(groupedByTag, 'cost')).not.toBe( + METRIC_COLORS.traces, + ); + }); + }, + ); + + test( + 'A chart with no grouping keeps the fixed metric colours', + { tag: ['@t2-cuj', '@cap:dashboards.configure-widget'] }, + async ({ seriesColorsDashboard, page }) => { + const dashboard = new DashboardPage(page); + const { ungrouped, duration } = SERIES_COLORS_WIDGETS; + + await test.step('Open the dashboard and wait for both ungrouped charts', async () => { + await dashboard.goto(seriesColorsDashboard.id, SERIES_COLORS_TIME_RANGE); + await dashboard.waitForChart(ungrouped, 1); + await dashboard.waitForChart(duration, 3); + }); + + await test.step('The trace count series keeps its fixed colour, not its hashed one', async () => { + expect(await dashboard.legendColor(ungrouped, 'traces')).toBe(METRIC_COLORS.traces); + expect(await dashboard.seriesColors(ungrouped)).toEqual([METRIC_COLORS.traces]); + // `traces` is seeded as a tag too, so its hashed colour is a real + // alternative here: seeing it would mean the fixed map had been + // dropped from ungrouped charts as well as from grouped ones. + expect(EXPECTED_GROUP_COLORS.traces).not.toBe(METRIC_COLORS.traces); + expect(await dashboard.legendColor(ungrouped, 'traces')).not.toBe( + EXPECTED_GROUP_COLORS.traces, + ); + }); + + await test.step('Each duration percentile keeps its own fixed colour', async () => { + for (const [series, color] of [ + ['duration.p50', METRIC_COLORS['duration.p50']], + ['duration.p90', METRIC_COLORS['duration.p90']], + ['duration.p99', METRIC_COLORS['duration.p99']], + ] as const) { + expect(await dashboard.legendColor(duration, series), series).toBe(color); + } + + const drawn = await dashboard.seriesColors(duration); + expect([...drawn].sort()).toEqual( + [ + METRIC_COLORS['duration.p50'], + METRIC_COLORS['duration.p90'], + METRIC_COLORS['duration.p99'], + ].sort(), + ); + }); + }, + ); +});