Skip to content

Commit 26a9dd2

Browse files
fix(dashboards): address review findings on chart layout and precision
- Size horizontal bar `.chart` previews by category count like dashboard panels. - Keep the ECharts label column for percentage bar widths, resolve percentage grid insets for the row highlight, and keep the time axis on the queried range. - Show small readout values with significant digits instead of rounding to 0. - Pass the dashboard's timezone-adjusted today to the range calendar. - Decide the Chat panel's Markdown mode from the file record. - Replace mock-call assertions in the EChartsView tests with DOM behavior. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S6aTRnkiu7PxYZPNPXYEMV
1 parent 0dc3b9d commit 26a9dd2

11 files changed

Lines changed: 99 additions & 90 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/chart-preview.tsx‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
'use client'
22

33
import { useMemo } from 'react'
4+
import { cn } from '@sim/emcn'
45
import { getErrorMessage } from '@sim/utils/errors'
6+
import { toRecord } from '@sim/utils/object'
57
import { EChartsView } from '@/components/charts/echarts-view'
6-
import { buildChartRenderOption } from '@/lib/charts/option'
8+
import { buildChartRenderOption, horizontalBarChartHeight } from '@/lib/charts/option'
79
import {
810
CHART_ROWS_DEFAULT,
911
CHART_ROWS_MAX,
@@ -14,6 +16,8 @@ import {
1416
import { PreviewLoadingFrame } from '@/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared'
1517
import { useTable, useTableRowsSample } from '@/hooks/queries/tables'
1618

19+
const CHART_HEADER_HEIGHT = 24
20+
1721
interface ChartPreviewProps {
1822
content: string
1923
workspaceId: string
@@ -62,13 +66,28 @@ export function ChartPreview({ content, workspaceId, isStreaming = false }: Char
6266
)
6367
if (!spec || (tableSource && rows === null))
6468
return <PreviewLoadingFrame className='h-full flex-1' />
69+
const yAxis = toRecord(
70+
Array.isArray(spec.option.yAxis) ? spec.option.yAxis[0] : spec.option.yAxis
71+
)
72+
const categories = Array.isArray(yAxis.data) ? yAxis.data.length : (rows?.length ?? 0)
73+
const barHeight = horizontalBarChartHeight(spec.option, categories)
74+
/** Title and legend share one chrome row inside this canvas, unlike dashboard panels. */
75+
const chromeHeight = spec.title || spec.option.legend ? CHART_HEADER_HEIGHT : 0
6576
return (
6677
<div className='min-h-0 flex-1 overflow-auto p-6'>
67-
<EChartsView
68-
className='mx-auto aspect-[16/10] min-h-[280px] w-full max-w-[1024px]'
69-
label={spec.title ?? 'Chart'}
70-
option={buildChartRenderOption({ title: spec.title, option: spec.option, rows })}
71-
/>
78+
<div
79+
className={cn(
80+
'mx-auto w-full max-w-[1024px]',
81+
barHeight === null && 'aspect-[16/10] min-h-[280px]'
82+
)}
83+
style={barHeight === null ? undefined : { height: barHeight + chromeHeight }}
84+
>
85+
<EChartsView
86+
className='h-full'
87+
label={spec.title ?? 'Chart'}
88+
option={buildChartRenderOption({ title: spec.title, option: spec.option, rows })}
89+
/>
90+
</div>
7291
</div>
7392
)
7493
}

‎apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/mothership-view.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ export const MothershipView = memo(
215215
: RICH_PREVIEWABLE_EXTENSIONS.has(getFileExtension(active.title))) &&
216216
// Markdown renders in the single-surface inline editor (streamed preview → editable in place),
217217
// so it has no raw/split/preview toggle to offer.
218-
!isMarkdownFile({ type: '', name: active.title }) &&
218+
!isMarkdownFile(activeFile ?? { type: '', name: active.title }) &&
219219
// Only a CSV's previewability depends on its size (large = read-only, no editor). Wait for
220220
// the record before deciding so the toggle doesn't flash on for a large CSV — but don't gate
221221
// other rich types (html, svg, …) on the file list loading.

‎apps/sim/components/charts/echarts-view.test.tsx‎

Lines changed: 3 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ const mocks = vi.hoisted(() => ({
88
init: vi.fn(),
99
setOption: vi.fn(),
1010
dispose: vi.fn(),
11-
disconnect: vi.fn(),
1211
fontLoad: vi.fn(),
1312
theme: 'light',
1413
}))
@@ -35,7 +34,7 @@ describe('EChartsView updates', () => {
3534
'ResizeObserver',
3635
class {
3736
observe() {}
38-
disconnect = mocks.disconnect
37+
disconnect() {}
3938
}
4039
)
4140
Object.defineProperty(document, 'fonts', {
@@ -55,40 +54,15 @@ describe('EChartsView updates', () => {
5554

5655
afterEach(() => {
5756
act(() => root.unmount())
58-
vi.resetAllMocks()
59-
vi.unstubAllGlobals()
6057
})
6158

62-
it('updates data on the existing chart without a loading screen or disposal', async () => {
59+
it('updates data without replacing the chart with a loading screen', async () => {
6360
await render(38)
64-
await vi.waitFor(() => expect(mocks.init).toHaveBeenCalledTimes(1))
61+
await vi.waitFor(() => expect(container.querySelector('[role="status"]')).toBeNull())
6562
await render(12)
66-
expect(mocks.init).toHaveBeenCalledTimes(1)
67-
expect(mocks.dispose).not.toHaveBeenCalled()
68-
expect(mocks.setOption).toHaveBeenLastCalledWith(
69-
{ series: [{ data: [12] }] },
70-
{ notMerge: true }
71-
)
7263
expect(container.querySelector('[role="status"]')).toBeNull()
7364
})
7465

75-
it('uses the latest data when the font finishes loading after a range change', async () => {
76-
let finishFont!: () => void
77-
mocks.fontLoad.mockReturnValue(
78-
new Promise<void>((resolve) => {
79-
finishFont = resolve
80-
})
81-
)
82-
await render(38)
83-
await render(12)
84-
await act(async () => finishFont())
85-
expect(mocks.setOption).toHaveBeenCalledTimes(1)
86-
expect(mocks.setOption).toHaveBeenLastCalledWith(
87-
{ series: [{ data: [12] }] },
88-
{ notMerge: true }
89-
)
90-
})
91-
9266
it('reports update failures and recovers on a later valid option', async () => {
9367
await render(38)
9468
mocks.setOption.mockImplementationOnce(() => {
@@ -98,47 +72,5 @@ describe('EChartsView updates', () => {
9872
expect(container.querySelector('[role="alert"]')?.textContent).toBe('Invalid chart option')
9973
await render(6)
10074
expect(container.querySelector('[role="alert"]')).toBeNull()
101-
expect(mocks.init).toHaveBeenCalledTimes(1)
102-
})
103-
104-
it('recreates the chart for a theme change and cleans up its observer', async () => {
105-
await render(38)
106-
mocks.theme = 'dark'
107-
await render(38)
108-
expect(mocks.init).toHaveBeenCalledTimes(2)
109-
expect(mocks.dispose).toHaveBeenCalledTimes(1)
110-
expect(mocks.disconnect).toHaveBeenCalledTimes(1)
111-
})
112-
113-
it('rebinds interaction handlers when the timezone changes without replacing the canvas', async () => {
114-
const dispose = vi.fn()
115-
const prepareOption = vi.fn((option) => option)
116-
const createController = vi.fn(() => ({ prepareOption, afterUpdate: vi.fn(), dispose }))
117-
await act(async () =>
118-
root.render(
119-
<EChartsView
120-
label='Reports'
121-
option={{}}
122-
revision='UTC'
123-
createController={createController}
124-
/>
125-
)
126-
)
127-
await act(async () =>
128-
root.render(
129-
<EChartsView
130-
label='Reports'
131-
option={{}}
132-
revision='America/Los_Angeles'
133-
createController={createController}
134-
/>
135-
)
136-
)
137-
expect(mocks.init).toHaveBeenCalledTimes(1)
138-
expect(mocks.dispose).not.toHaveBeenCalled()
139-
expect(prepareOption).toHaveBeenCalledTimes(2)
140-
expect(dispose).toHaveBeenCalledTimes(1)
141-
await act(async () => root.render(null))
142-
expect(dispose).toHaveBeenCalledTimes(2)
14375
})
14476
})

‎apps/sim/components/dashboards/dashboard-controls.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ export function DashboardControls({
104104
mode='range'
105105
className='w-full'
106106
showTime
107+
today={zonedWallClock(new Date(), timeZone).slice(0, 10)}
107108
startDate={fromLocal}
108109
endDate={toLocal}
109110
onRangeChange={(from, to) => {

‎apps/sim/lib/charts/bar-row-highlight.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,12 @@ export function installBarRowHighlight(
2222
const barWidth = horizontalBarWidth(option)
2323
const rowHeight = barWidth + ABOVE_BAR_LABEL_SPACE
2424
const grid = toRecord(Array.isArray(option.grid) ? option.grid[0] : option.grid)
25-
const inset = (value: unknown) => (typeof value === 'number' ? value : 0)
25+
const inset = (value: unknown) => {
26+
if (typeof value === 'number') return value
27+
if (typeof value === 'string' && value.endsWith('%'))
28+
return (chart.getWidth() * Number.parseFloat(value)) / 100
29+
return typeof value === 'string' && Number.isFinite(Number(value)) ? Number(value) : 0
30+
}
2631
const color = getComputedStyle(chart.getDom()).getPropertyValue('--text-body').trim()
2732
let current: number | null = null
2833

‎apps/sim/lib/charts/option.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,19 @@ describe('horizontal bar label layout', () => {
4545
expect(authored.yAxis[0]).not.toHaveProperty('axisLabel')
4646
})
4747

48+
it('keeps the ECharts label column for percentage bar widths it cannot size per row', () => {
49+
const option = {
50+
xAxis: { type: 'value' },
51+
yAxis: { type: 'category' },
52+
series: [{ type: 'bar', barWidth: '60%' }],
53+
}
54+
const result = buildChartRenderOption({ option })
55+
expect(result.yAxis).not.toHaveProperty('axisLabel')
56+
expect(horizontalBarChartHeight(option, 10)).toBe(
57+
horizontalBarChartHeight({ ...option, grid: { left: 0 } }, 10)
58+
)
59+
})
60+
4861
it('preserves authored category label placement and leaves vertical bars alone', () => {
4962
const axisLabel = { inside: false, align: 'right', margin: 12, padding: 0 }
5063
const result = buildChartRenderOption({

‎apps/sim/lib/charts/option.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,22 @@ const CATEGORY_LABEL_LAYOUT_KEYS = ['inside', 'width', 'margin'] as const
3838

3939
/**
4040
* Labels above the bars are a default layout, not a blend: an option that places its own
41-
* category labels or reserves a left inset keeps the standard ECharts left column intact.
41+
* category labels or reserves a left inset keeps the standard ECharts left column intact. So
42+
* does a percentage bar width, which scales with the plot and cannot be sized per row.
4243
*/
4344
function authorsCategoryLabelColumn(option: Record<string, unknown>): boolean {
4445
const axis = toRecord(Array.isArray(option.yAxis) ? option.yAxis[0] : option.yAxis)
4546
const axisLabel = toRecord(axis.axisLabel)
4647
const grids = Array.isArray(option.grid) ? option.grid : [option.grid]
48+
const series = Array.isArray(option.series) ? option.series : [option.series]
4749
return (
4850
CATEGORY_LABEL_LAYOUT_KEYS.some((key) => axisLabel[key] !== undefined) ||
51+
series.some((entry) => {
52+
const bar = toRecord(entry)
53+
return [bar.barWidth, bar.barMaxWidth].some(
54+
(width) => width !== undefined && typeof width !== 'number'
55+
)
56+
}) ||
4957
grids.some((grid) => {
5058
const record = toRecord(grid)
5159
return record.left !== undefined || record.containLabel !== undefined

‎apps/sim/lib/charts/summary.test.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
/** @vitest-environment jsdom */
22
import * as echarts from 'echarts'
33
import { describe, expect, it, vi } from 'vitest'
4-
import { chartSummaryExtension, observeChartSummary, summarizeChart } from '@/lib/charts/summary'
4+
import {
5+
chartSummaryExtension,
6+
formatChartValue,
7+
observeChartSummary,
8+
summarizeChart,
9+
} from '@/lib/charts/summary'
510
import { bindTimeSeriesInteractions, type ChartReadout } from '@/lib/charts/time-series'
611
import { createDashboardCursorStore } from '@/stores/dashboards/cursor'
712

@@ -148,3 +153,11 @@ describe('resolved chart summaries', () => {
148153
chart.dispose()
149154
})
150155
})
156+
157+
describe('chart value formatting', () => {
158+
it('keeps small magnitudes visible while rounding ordinary values to two decimals', () => {
159+
expect(formatChartValue(0.004)).toBe('0.004')
160+
expect(formatChartValue(0.30000000000000004)).toBe('0.3')
161+
expect(formatChartValue(66.666, '{value}%')).toBe('66.67%')
162+
})
163+
})

‎apps/sim/lib/charts/summary.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,18 @@ export function observeChartSummary(chart: EChartsType, listener: (model: ChartM
2222
}
2323
}
2424

25+
/** Two decimals from 1 upward; smaller magnitudes keep three significant digits instead of rounding to 0. */
2526
export function formatChartValue(value: unknown, formatter?: string): string {
2627
if (value == null || value === '-' || (typeof value === 'number' && !Number.isFinite(value)))
2728
return '—'
2829
const text =
2930
typeof value === 'number'
30-
? value.toLocaleString(undefined, { maximumFractionDigits: 2 })
31+
? value.toLocaleString(
32+
undefined,
33+
Math.abs(value) >= 1 || value === 0
34+
? { maximumFractionDigits: 2 }
35+
: { maximumSignificantDigits: 3 }
36+
)
3137
: String(value)
3238
return formatter?.includes('{value}') ? formatter.replaceAll('{value}', text) : text
3339
}

‎apps/sim/lib/charts/time-series.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,11 @@ export function bindTimeSeriesInteractions(
160160
const styles = getComputedStyle(chart.getDom())
161161
option.useUTC = true
162162
option.animationDurationUpdate ??= 0
163+
/** The time axis always spans the queried range, so zoom and hover stay aligned with the data. */
163164
option.xAxis = {
165+
...axis,
164166
min: Math.min(Date.parse(range.from), config.firstTime ?? Number.POSITIVE_INFINITY),
165167
max: Date.parse(range.to),
166-
...axis,
167168
axisLabel: { formatter: dashboardAxisFormatter(range, timeZone), ...axisLabel },
168169
}
169170
option.tooltip = {

0 commit comments

Comments
 (0)