From 356de5145a77415c525a49a308487814effa4578 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Sat, 1 Aug 2026 00:57:32 -0700 Subject: [PATCH] =?UTF-8?q?fix(charts):=20name=20the=20slices=20=E2=80=94?= =?UTF-8?q?=20pie/donut=20legends=20lost=20their=20labels=20to=20a=20`type?= =?UTF-8?q?`=20dimension=20(#3135)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dashboard pie whose category dimension is named `type` ("按类型分布" — a staple) rendered a legend of anonymous colour dots: swatches, no text. The label came from the shadcn config helper, which read the LEGEND ITEM's own same-named prop before the data row — and recharts legend items carry a `type` of their own (the icon shape, "rect"). So `nameKey="type"` resolved to "rect", missed the config, and rendered nothing. `color` collided the same way. Read the data row first: that is what `nameKey` was pointing at all along. Two adjacent gaps, same report: - the pie/donut branch drew its legend unconditionally, so `showLegend: false` did nothing; - `DatasetWidget` never forwarded the widget's `chartConfig` at all, so the declared `showLegend` was inert metadata either way — `true` only "worked" because on is the renderer's default. Co-Authored-By: Claude Opus 5 --- .../src/AdvancedChartImpl.pieLegend.test.tsx | 99 +++++++++++++++++++ .../plugin-charts/src/AdvancedChartImpl.tsx | 12 ++- .../plugin-charts/src/ChartContainerImpl.tsx | 17 +++- .../plugin-dashboard/src/DatasetWidget.tsx | 14 ++- .../DatasetWidget.showLegend.test.tsx | 75 ++++++++++++++ 5 files changed, 206 insertions(+), 11 deletions(-) create mode 100644 packages/plugin-charts/src/AdvancedChartImpl.pieLegend.test.tsx create mode 100644 packages/plugin-dashboard/src/__tests__/DatasetWidget.showLegend.test.tsx diff --git a/packages/plugin-charts/src/AdvancedChartImpl.pieLegend.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.pieLegend.test.tsx new file mode 100644 index 0000000000..2c7a63778e --- /dev/null +++ b/packages/plugin-charts/src/AdvancedChartImpl.pieLegend.test.tsx @@ -0,0 +1,99 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Pie/donut legend (#3135) — the legend must NAME each slice's category, and it + * must obey `showLegend`. + * + * Two defects lived here. The legend resolved its label through the shadcn + * config helper, which read the legend ITEM's own `type`/`color` props before + * the data row — so a category dimension literally named `type` (a dashboard + * staple: "按类型分布") resolved to the legend ICON shape ("rect"), missed the + * config, and drew colour swatches with NO text: a pie of anonymous dots. And + * the pie branch rendered its legend unconditionally, so `showLegend: false` + * did nothing. + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, cleanup, act } from '@testing-library/react'; + +// Recharts' ResponsiveContainer measures its parent via ResizeObserver, which +// reports 0×0 under the headless DOM — so the chart never paints. Replace it +// with a fixed-size passthrough so the pie (and its legend) actually render. +vi.mock('recharts', async () => { + const actual = await vi.importActual('recharts'); + return { + ...actual, + ResponsiveContainer: ({ children }: any) => + React.cloneElement(children, { width: 320, height: 320 }), + }; +}); + +import AdvancedChartImpl from './AdvancedChartImpl'; + +afterEach(cleanup); + +// Recharts registers the pie's legend payload from a layout effect and the +// Legend re-renders off that store update, so read the legend after a tick. +const settle = () => act(async () => { await new Promise((r) => setTimeout(r, 60)); }); + +const legendText = (c: HTMLElement) => + c.querySelector('.recharts-legend-wrapper')?.textContent ?? ''; + +describe('AdvancedChartImpl — pie/donut legend', () => { + // `type` is the exact dimension name from the reported dashboard, and it + // collides with the recharts legend item's own `type` (icon shape) prop. + it.each(['type', 'color', 'urgency'])('labels each slice for a dimension named `%s`', async (dim) => { + const { container } = render( + , + ); + await settle(); + expect(legendText(container)).toContain('设备'); + expect(legendText(container)).toContain('质量'); + }); + + // Single-category data is the degenerate case the issue calls out: one full + // circle in one colour is zero information without the category's name. + it('donut: a single category is still named', async () => { + const { container } = render( + , + ); + await settle(); + expect(legendText(container)).toContain('设备'); + }); + + it('honors showLegend: false', async () => { + const { container } = render( + , + ); + await settle(); + expect(container.querySelectorAll('.recharts-legend-wrapper')).toHaveLength(0); + }); +}); diff --git a/packages/plugin-charts/src/AdvancedChartImpl.tsx b/packages/plugin-charts/src/AdvancedChartImpl.tsx index eb9207ef69..57da0a29a4 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.tsx @@ -689,11 +689,13 @@ function AdvancedChartImplInner({ return ; })} - } - /> + {legendVisible ? ( + } + /> + ) : null} ); diff --git a/packages/plugin-charts/src/ChartContainerImpl.tsx b/packages/plugin-charts/src/ChartContainerImpl.tsx index 496ac0ce84..de966c8bb0 100644 --- a/packages/plugin-charts/src/ChartContainerImpl.tsx +++ b/packages/plugin-charts/src/ChartContainerImpl.tsx @@ -401,12 +401,14 @@ function getPayloadConfigFromPayload( let configLabelKey: string = key + // The DATA ROW wins over the legend/tooltip item's own same-named prop. + // Recharts item entries carry `type` (the legend ICON shape, e.g. "rect"), + // `color`, `dataKey` and `value` of their own, so a pie whose category + // dimension is literally named `type` (`nameKey="type"`) used to resolve to + // "rect", miss the config, and render legend swatches with NO text at all — + // a pie of unlabelled colour dots (#3135). Reading the row first keys off the + // actual category ("设备"), which is what `nameKey` was pointing at. if ( - key in payload && - typeof payload[key as keyof typeof payload] === "string" - ) { - configLabelKey = payload[key as keyof typeof payload] as string - } else if ( payloadPayload && key in payloadPayload && typeof payloadPayload[key as keyof typeof payloadPayload] === "string" @@ -414,6 +416,11 @@ function getPayloadConfigFromPayload( configLabelKey = payloadPayload[ key as keyof typeof payloadPayload ] as string + } else if ( + key in payload && + typeof payload[key as keyof typeof payload] === "string" + ) { + configLabelKey = payload[key as keyof typeof payload] as string } return configLabelKey in config diff --git a/packages/plugin-dashboard/src/DatasetWidget.tsx b/packages/plugin-dashboard/src/DatasetWidget.tsx index 6156b4a9c7..15da9473cc 100644 --- a/packages/plugin-dashboard/src/DatasetWidget.tsx +++ b/packages/plugin-dashboard/src/DatasetWidget.tsx @@ -592,6 +592,18 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: }); const effectiveCategoryOrder = explicitOrder?.length ? explicitOrder : categoryOrder; + // `chartConfig.showLegend` (#3135). The widget's chart config never reached + // the renderer — this component read `options` and nothing else — so an author + // who wrote `showLegend: false` still got a legend, and one who wrote `true` + // only got one because "on" happens to be the renderer's default. Lower the + // one flag the renderer already honors; the rest of `chartConfig` stays + // unforwarded (the renderer derives axes/series from the dataset selection). + const chartConfig: Record = + widget?.chartConfig && typeof widget.chartConfig === 'object' && !Array.isArray(widget.chartConfig) + ? (widget.chartConfig as Record) + : {}; + const showLegend = typeof chartConfig.showLegend === 'boolean' ? chartConfig.showLegend : undefined; + // Map a clicked chart segment back to its dataset row, then drill through to // the underlying records — same governed path the table/pivot rows use. const handleChartDrill = (ev: { category?: string; series?: string; value?: number }) => { @@ -617,7 +629,7 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: // measurement churn, can freeze there — bars never draw until an unrelated // re-render (#2756, follow-up to #2727's ineffective settle re-mount). // Turning the tween off makes the first paint deterministic. - schema={{ type: 'chart', chartType, data: chartData, xAxisKey, series, isAnimationActive: false, ...(categoryColors ? { categoryColors } : {}), ...(effectiveCategoryOrder ? { categoryOrder: effectiveCategoryOrder } : {}) } as any} + schema={{ type: 'chart', chartType, data: chartData, xAxisKey, series, isAnimationActive: false, ...(showLegend != null ? { showLegend } : {}), ...(categoryColors ? { categoryColors } : {}), ...(effectiveCategoryOrder ? { categoryOrder: effectiveCategoryOrder } : {}) } as any} onChartClick={chartDrill} onSegmentClick={chartDrill} /> diff --git a/packages/plugin-dashboard/src/__tests__/DatasetWidget.showLegend.test.tsx b/packages/plugin-dashboard/src/__tests__/DatasetWidget.showLegend.test.tsx new file mode 100644 index 0000000000..4aa89d535d --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/DatasetWidget.showLegend.test.tsx @@ -0,0 +1,75 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3135 — a dataset widget's `chartConfig.showLegend` must reach the chart + * renderer. It never did: this component read `options` and nothing else, so + * the flag was inert metadata — `false` still drew a legend, and `true` only + * "worked" because the renderer defaults to showing one. + * + * Asserted at the source (the schema handed to the renderer), via a stubbed + * SchemaRenderer — the same seam DatasetWidget.animation uses. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, cleanup, waitFor } from '@testing-library/react'; + +let lastChartSchema: any = null; + +vi.mock('@object-ui/react', async (importOriginal) => ({ + ...(await importOriginal>()), + SchemaRenderer: (props: any) => { + lastChartSchema = props.schema; + return null; + }, +})); + +import { DatasetWidget } from '../DatasetWidget'; + +afterEach(() => { + cleanup(); + lastChartSchema = null; +}); + +const rows = [ + { type: '设备', total_count: 5 }, + { type: '质量', total_count: 3 }, +]; + +const renderWidget = (chartConfig?: Record) => { + const src = { queryDataset: vi.fn(async () => ({ rows })) }; + render( + , + ); +}; + +describe('DatasetWidget — chartConfig.showLegend (#3135)', () => { + it('forwards showLegend: true', async () => { + renderWidget({ type: 'pie', showLegend: true, showDataLabels: false }); + await waitFor(() => expect(lastChartSchema).not.toBeNull()); + expect(lastChartSchema.chartType).toBe('pie'); + expect(lastChartSchema.showLegend).toBe(true); + }); + + it('forwards showLegend: false', async () => { + renderWidget({ type: 'pie', showLegend: false }); + await waitFor(() => expect(lastChartSchema).not.toBeNull()); + expect(lastChartSchema.showLegend).toBe(false); + }); + + // No declaration → the key stays off the schema so the renderer's own default + // (legend shown) still decides. Every existing widget keeps its behaviour. + it('omits the key when the widget declares no chartConfig', async () => { + renderWidget(); + await waitFor(() => expect(lastChartSchema).not.toBeNull()); + expect('showLegend' in lastChartSchema).toBe(false); + }); +});