mirror of
https://github.com/supabase/supabase.git
synced 2026-10-06 01:45:10 +03:00
## I have read the [CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md) file. YES ## What kind of change does this PR introduce? Bug fix. Hardening ahead of any decision to enable session replay. ### What's inside - ~47 lines of logic: positional series keys, the X-key collision guard, and the rewiring of rows, cumulative results and axis width ([QueryResultChart.tsx](https://github.com/supabase/supabase/pull/50270/changes#diff-dae994728613498678bcc06a3ea5176b36708e06b531cfc7eddae3e588fff528)) - ~123 lines of tests, one case per chart type, cumulative setting and edge case ([QueryResultChart.test.tsx](https://github.com/supabase/supabase/pull/50270/changes#diff-6e1c92ad725bac0324db4d079f3d0cd061b8f6e2aecdc21bcdc5969c90dc3c59)) ## What is the current behavior? Session replay is disabled in every environment, and no recordings exist. This is about what a recording *would* contain if it were ever switched on: charting a query result would put the customer's own column names into it. `ChartContainer` writes every chart config key into a `<style>` element as `--color-<key>`. rrweb records `<style>` text verbatim: ```js u = "STYLE" === parentTagName || void 0 h = "SCRIPT" === parentTagName || void 0 !u && !h && o && r && (o = maskTextFn ? maskTextFn(o, parent) : o.replace(/\S/g, "*")) ``` The `!u` guard means `maskTextFn` never sees stylesheet text. It is a text node, so `maskAttributeFn` does not see it either. CSS written into the DOM is a channel no masking hook reaches. `QueryResultChart` keyed its config by the column names picked for the Y axis, which come from the customer's own SQL results. With recording enabled, a query charting a column named `customer_email` would produce `--color-customer_email` in the captured DOM. Linear [GROWTH-1229](https://linear.app/supabase/issue/GROWTH-1229). #48818 masks the attribute channel. This channel is text, so that PR does not reach it. ## What is the new behavior? Y series are keyed by position (`series_0`, `series_1`), so no customer string reaches the config keys. The column name still goes through as `config[key].label`, which `chart.tsx` renders into the tooltip and legend as text. Text nodes outside `<style>` are masked by `maskReplayText`, so the name is safe there and the chart stays readable. The X column keeps its own name. Only config keys reach the `<style>` and the X column is never one, so renaming it would buy nothing, and it would cost the `timestamp` handling: `ChartBar` and `ChartLine` branch on `xKey === 'timestamp'` to format tooltip dates and render the date-range footer. Keeping the original name needs one guard, since an X column literally named `series_0` would share a row key with the first Y series. `xKeyFor` appends underscores until the two are distinct. ## Additional context ### Testing Eleven tests. They assert the rendered `<style>` contains `--color-series_0` and does not contain the column name, across both chart types and both cumulative settings, plus the two-series case, the `timestamp` column and the collision guard. Three are verified to catch the defect they cover: the style-key assertion fails against the unfixed component, the `timestamp` assertion fails when the X key is pinned to a constant, and the `xKeyFor` cases fail when the guard's body is removed. `vitest run components/interfaces/Explorer` passes: 170 tests across 16 files. The tooltip and legend path isn't asserted, because recharts doesn't render either one at the 0x0 size jsdom gives the container. That the label is what renders there comes from reading `chart.tsx`. It is unverified at runtime. ### Remaining exposure Every other `ChartContainer` caller passes literal keys (`--color-error`, `--color-ok_count`), and `UnifiedLogs` filters a static config by a fixed level set, so this was the only caller feeding it customer strings. `chart-bar.tsx:119-124` and `chart-line.tsx` still fall back to building a config from whatever `dataKeys` they're given, so a future caller can reintroduce this without touching `QueryResultChart`. A general guard would have to live in `ChartContainer` or in the replay config. CSSOM writes (`insertRule`, `replace`, `replaceSync`, adopted stylesheets) also emit CSS outside both masking hooks. This PR does not close that path. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved query result charts to preserve the source column name used for the X-axis. - Prevented X-axis names from conflicting with generated series identifiers. - Ensured bar and line charts consistently bind data to the correct X-axis and series. - Preserved timestamp-based chart behavior, including the date-range footer. - Chart series styling now uses stable positional identifiers, ensuring colors remain correctly assigned across configured series. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
124 lines
4.3 KiB
TypeScript
124 lines
4.3 KiB
TypeScript
import { describe, expect, it } from 'vitest'
|
|
|
|
import { QueryResultChart, xKeyFor } from './QueryResultChart'
|
|
import { customRender } from '@/tests/lib/custom-render'
|
|
|
|
const CUSTOMER_COLUMN = 'customer_email_address'
|
|
|
|
const chart = {
|
|
type: 'bar' as const,
|
|
x_column: 'day',
|
|
y_series: [CUSTOMER_COLUMN],
|
|
cumulative: false,
|
|
show_labels: true,
|
|
scale: 'linear' as const,
|
|
}
|
|
|
|
const result = {
|
|
rows: [
|
|
{ day: '2026-01-01', [CUSTOMER_COLUMN]: 4 },
|
|
{ day: '2026-01-02', [CUSTOMER_COLUMN]: 7 },
|
|
],
|
|
}
|
|
|
|
const styleTextOf = (container: HTMLElement) =>
|
|
Array.from(container.querySelectorAll('style'))
|
|
.map((style) => style.textContent ?? '')
|
|
.join('\n')
|
|
|
|
/**
|
|
* `ChartContainer` writes every config key into a `<style>` element as `--color-<key>`,
|
|
* and rrweb records `<style>` text without masking it, so a config keyed by column name
|
|
* would put customer column names into a recording.
|
|
*/
|
|
describe('QueryResultChart column names in CSS', () => {
|
|
it.each([
|
|
['bar', false],
|
|
['line', false],
|
|
['bar', true],
|
|
['line', true],
|
|
])('keys the style by position for type=%s cumulative=%s', (type, cumulative) => {
|
|
const { container } = customRender(
|
|
<QueryResultChart
|
|
chart={{ ...chart, type: type as 'bar' | 'line', cumulative }}
|
|
result={result as never}
|
|
/>
|
|
)
|
|
|
|
const css = styleTextOf(container)
|
|
expect(css).toContain('--color-series_0')
|
|
expect(css).not.toContain(CUSTOMER_COLUMN)
|
|
})
|
|
|
|
it('emits one distinct series key per selected column', () => {
|
|
const twoSeries = { ...chart, y_series: [CUSTOMER_COLUMN, 'customer_plan'] }
|
|
const rows = result.rows.map((row) => ({ ...row, customer_plan: 1 }))
|
|
const { container } = customRender(
|
|
<QueryResultChart chart={twoSeries} result={{ rows } as never} />
|
|
)
|
|
|
|
// ChartStyle emits one block per theme, so count distinct keys rather than matches.
|
|
const seriesKeys = new Set(styleTextOf(container).match(/--color-series_\d+/g))
|
|
expect(seriesKeys.size).toBe(twoSeries.y_series.length)
|
|
expect(styleTextOf(container)).not.toContain('customer_plan')
|
|
})
|
|
|
|
it('leaves the X column name alone, which ChartBar and ChartLine check for "timestamp"', () => {
|
|
const timestampChart = { ...chart, x_column: 'timestamp' }
|
|
const rows = [
|
|
{ timestamp: '2026-01-01T00:00:00Z', [CUSTOMER_COLUMN]: 4 },
|
|
{ timestamp: '2026-01-02T00:00:00Z', [CUSTOMER_COLUMN]: 7 },
|
|
]
|
|
const { container } = customRender(
|
|
<QueryResultChart chart={timestampChart} result={{ rows } as never} />
|
|
)
|
|
|
|
// The date-range footer only renders when xKey is literally 'timestamp'.
|
|
expect(container.textContent).toContain('2026')
|
|
expect(styleTextOf(container)).not.toContain(CUSTOMER_COLUMN)
|
|
})
|
|
|
|
it('renders with an X column named like a series key', () => {
|
|
const colliding = { ...chart, x_column: 'series_0' }
|
|
const rows = [
|
|
{ series_0: 'a', [CUSTOMER_COLUMN]: 4 },
|
|
{ series_0: 'b', [CUSTOMER_COLUMN]: 7 },
|
|
]
|
|
const { container } = customRender(
|
|
<QueryResultChart chart={colliding} result={{ rows } as never} />
|
|
)
|
|
|
|
expect(styleTextOf(container)).not.toContain(CUSTOMER_COLUMN)
|
|
})
|
|
})
|
|
|
|
/**
|
|
* Asserted directly rather than through a render. jsdom draws no bars or ticks at the
|
|
* 0x0 size it gives the chart container, so a rendered collision is not observable and
|
|
* the same assertions pass with the guard removed.
|
|
*/
|
|
describe('xKeyFor', () => {
|
|
it('moves the X key off a colliding series key', () => {
|
|
expect(xKeyFor('series_0', ['series_0'])).toBe('series_0_')
|
|
})
|
|
|
|
it('keeps appending until the X key is distinct', () => {
|
|
expect(xKeyFor('series_0', ['series_0', 'series_0_'])).toBe('series_0__')
|
|
})
|
|
|
|
it('leaves a non-colliding X column alone', () => {
|
|
expect(xKeyFor('timestamp', ['series_0', 'series_1'])).toBe('timestamp')
|
|
expect(xKeyFor('day', [])).toBe('day')
|
|
})
|
|
|
|
it('cannot produce a key that collides again, since series keys carry no trailing underscore', () => {
|
|
const seriesKeys = ['series_0', 'series_1', 'series_2']
|
|
const result = xKeyFor('series_1', seriesKeys)
|
|
expect(seriesKeys).not.toContain(result)
|
|
})
|
|
})
|
|
|
|
// The column name still reaches the chart as `config[key].label`, which `chart.tsx`
|
|
// renders into the tooltip and legend as text. That path is not asserted here because
|
|
// recharts does not render either one at the 0x0 size jsdom gives the container.
|