mirror of
https://github.com/supabase/supabase.git
synced 2026-09-09 19:42:46 +08:00
Final PR of the stack. #49069, #49070, #49072 and #49074 have merged, so this now targets `master` directly. **Rebased onto latest `master`**, which includes the centralized result-rendering work (#49096). See "Conflict resolution" below. ## What's left after master's own fixes `QueryCell` was written for database cells and adapted to log cells afterwards. Master has since fixed most of it directly: `handleUpdateCell` no longer bails on a non-database cell, the cell's own binding is read via `getQuerySourceBinding`, and `database_identifier` / `time_range` propagate across a source change. What remains: - **`display` was only passed for database cells**, so the `view` field on `log_cell` stayed unreachable and a logs query could never be charted. That is the one behavioral fix left in this PR. - The per-backend branching is inline and untested. ## What changed Per-backend logic moves into `QueryCell.utils.ts`, where it is unit-tested: `changeCellSource`, `setCellSql`, `cloneQueryCell`, `getCellDisplay`, `toQueryModel`. Each narrows on the cell tag exactly once, so the SQL brand and the backend's parameters stay correlated rather than being re-derived at each call site. `cloneQueryCell` also rebuilds the chart's series array, which valtio hands over as `readonly string[]`. `NotebookEditor` renders through `isQueryCell` (#49069) rather than a tag switch, so a new backend gets picked up by classifying it in `CELL_KINDS` instead of by remembering to add a `case`. ## Conflict resolution Two rounds of master's work landed in this file set. **`QueryCell/index.tsx` (master's own rework).** `changeCellSource` **subsumes the four source-change branches** master had inline, each covered by a test: | Master's branch | Test | |---|---| | database → database (replica change) | `keeps the query when only the database changes` | | logs → logs (time-range change) | `keeps the query when only the log time range changes` | | database → logs | `carries the query text over when moving from the database to logs` | | logs → database | `carries the query text over and restores a default row limit …` | Two improvements fall out of consolidating them: - A **logs → database** move now keeps the selected replica; pinned by `applies the selected database when moving from logs to the database`. - The row-limit default is **named** rather than a hard-coded `100`. `Explorer/utils.ts` now shares `DEFAULT_CELL_ROW_LIMIT` with `createQueryCellSkeleton`, so cell creation and backend conversion can't drift. Untouched from master: `snap.updateCell`, `AddCellDropdown`, `MoveCellDropdownContent`, the `SortableSection` grip props, and `NotebookEditor`'s add-cell buttons, skeletons, `reorderCells` and `insertCellAfter`. **Centralized result rendering (#49096).** That PR moved `QueryCell/QueryResultChart.tsx` up to `Explorer/`, split `QueryResultTable` into `QueryResultError`, and added `QueryResultRenderer`. Since this PR removes `QueryChartConfig`, the type swap had to follow the move and also reach `QueryResultRenderer`, which is new and referenced the removed type. `QueryResultRenderer`, `QueryResultError` and `DataGridResults` are otherwise untouched — the empty/error-state centralization is fully preserved, and `QueryEditor` still renders through it. ## Behavior worth a second opinion `changeCellSource` **carries the query text across a backend change** and rebrands it. This is probably not what a user wants — Postgres SQL and logs SQL are separate dialects over separate schemas, so a carried-over query will usually fail to run, and the rebrand asserts a dialect the text was never written in. Keeping it for now because it destroys nothing and needs no confirmation prompt. The tradeoff is written up at the function. Worth revisiting once we know whether people switch source to port an existing query or to start a fresh one — if it's the latter, clearing the body behind a confirmation is the better answer. Results *are* dropped on a backend change, since another engine returns unrelated columns. ## Incidental `Explorer/types.ts` drops `QueryChartConfig`, which duplicated the wire schema's `ChartConfig` field for field. `chart` stays persisted alongside `view`, so switching to the table and back returns the user's chart settings rather than rebuilding them. ## Verification Typecheck, Prettier, and the lint ratchet clean. 1013 tests pass across `state/`, the Explorer surfaces, notebooks, query sources, `data/sql`, the SQL editor, and `components/ui`; 13 of them are new coverage for the extracted helpers. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved notebook cell rendering with more consistent handling of query and markdown cells. - Query cells now preserve SQL, source settings, display preferences, chart configuration, and query results when edited or switched between sources. - Added a default limit of 100 rows for applicable database queries. - **Bug Fixes** - Prevented stale query results from carrying over when changing query sources. - Improved chart configuration consistency across query results and display settings. - **Tests** - Added comprehensive coverage for query-cell updates, source transitions, SQL changes, display state, and chart data. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
168 lines
4.9 KiB
TypeScript
168 lines
4.9 KiB
TypeScript
import { untrustedSql } from '@supabase/pg-meta'
|
|
import { describe, expect, it } from 'vitest'
|
|
|
|
import {
|
|
changeCellSource,
|
|
cloneQueryCell,
|
|
DEFAULT_CELL_ROW_LIMIT,
|
|
getCellDisplay,
|
|
setCellSql,
|
|
toQueryModel,
|
|
} from './QueryCell.utils'
|
|
import { type ChartConfig, type QueryCell } from '@/data/content/notebooks/notebook-schema'
|
|
import { untrustedLogSql } from '@/data/logs/safe-analytics-sql'
|
|
|
|
const CHART: ChartConfig = {
|
|
type: 'bar',
|
|
x_column: 'day',
|
|
y_columns: ['signups'],
|
|
cumulative: false,
|
|
scale: 'linear',
|
|
show_labels: true,
|
|
}
|
|
|
|
const DATABASE_CELL: QueryCell = {
|
|
_tag: 'database_cell',
|
|
id: 'cell-1',
|
|
title: 'Signups',
|
|
view: 'chart',
|
|
chart: CHART,
|
|
unchecked_sql: untrustedSql('select * from auth.users'),
|
|
row_limit: 50,
|
|
database_identifier: 'replica-1',
|
|
}
|
|
|
|
const LOG_CELL: QueryCell = {
|
|
_tag: 'log_cell',
|
|
id: 'cell-2',
|
|
title: 'Edge errors',
|
|
view: 'table',
|
|
chart: CHART,
|
|
unchecked_sql: untrustedLogSql('select timestamp from logs'),
|
|
time_range: { _tag: 'relative_time_range', unit: 'hour', amount: 1 },
|
|
}
|
|
|
|
describe('changeCellSource', () => {
|
|
it('keeps the query when only the database changes', () => {
|
|
const next = changeCellSource(DATABASE_CELL, {
|
|
_tag: 'database',
|
|
database_identifier: 'replica-2',
|
|
})
|
|
|
|
expect(next).toEqual({ ...DATABASE_CELL, database_identifier: 'replica-2' })
|
|
})
|
|
|
|
it('keeps the query when only the log time range changes', () => {
|
|
const time_range = { _tag: 'relative_time_range', unit: 'day', amount: 7 } as const
|
|
const next = changeCellSource(LOG_CELL, { _tag: 'logs', time_range })
|
|
|
|
expect(next).toEqual({ ...LOG_CELL, time_range })
|
|
})
|
|
|
|
it('carries the query text over when moving from the database to logs', () => {
|
|
const time_range = { _tag: 'relative_time_range', unit: 'hour', amount: 1 } as const
|
|
const next = changeCellSource(DATABASE_CELL, { _tag: 'logs', time_range })
|
|
|
|
expect(next).toEqual({
|
|
_tag: 'log_cell',
|
|
id: 'cell-1',
|
|
title: 'Signups',
|
|
view: 'chart',
|
|
chart: CHART,
|
|
unchecked_sql: 'select * from auth.users',
|
|
time_range,
|
|
})
|
|
})
|
|
|
|
it('carries the query text over and restores a default row limit when moving from logs to the database', () => {
|
|
const next = changeCellSource(LOG_CELL, { _tag: 'database', database_identifier: undefined })
|
|
|
|
expect(next).toEqual({
|
|
_tag: 'database_cell',
|
|
id: 'cell-2',
|
|
title: 'Edge errors',
|
|
view: 'table',
|
|
chart: CHART,
|
|
unchecked_sql: 'select timestamp from logs',
|
|
row_limit: DEFAULT_CELL_ROW_LIMIT,
|
|
database_identifier: undefined,
|
|
})
|
|
})
|
|
|
|
it('applies the selected database when moving from logs to the database', () => {
|
|
const next = changeCellSource(LOG_CELL, {
|
|
_tag: 'database',
|
|
database_identifier: 'replica-2',
|
|
})
|
|
|
|
expect(next).toMatchObject({ _tag: 'database_cell', database_identifier: 'replica-2' })
|
|
})
|
|
|
|
it('preserves the chart across a backend change so display settings survive', () => {
|
|
const next = changeCellSource(DATABASE_CELL, {
|
|
_tag: 'logs',
|
|
time_range: { _tag: 'relative_time_range', unit: 'hour', amount: 1 },
|
|
})
|
|
|
|
expect(next.chart).toEqual(CHART)
|
|
expect(next.chart).not.toBe(DATABASE_CELL.chart)
|
|
})
|
|
})
|
|
|
|
describe('setCellSql', () => {
|
|
it('writes the text back onto a database cell without touching its source', () => {
|
|
expect(setCellSql(DATABASE_CELL, 'select 1')).toEqual({
|
|
...DATABASE_CELL,
|
|
unchecked_sql: 'select 1',
|
|
})
|
|
})
|
|
|
|
it('writes the text back onto a log cell without touching its time range', () => {
|
|
expect(setCellSql(LOG_CELL, 'select 2')).toEqual({ ...LOG_CELL, unchecked_sql: 'select 2' })
|
|
})
|
|
})
|
|
|
|
describe('cloneQueryCell', () => {
|
|
it('copies the chart series array rather than aliasing it', () => {
|
|
const clone = cloneQueryCell(DATABASE_CELL)
|
|
|
|
expect(clone).toEqual(DATABASE_CELL)
|
|
expect(clone.chart?.y_columns).not.toBe(DATABASE_CELL.chart?.y_columns)
|
|
})
|
|
})
|
|
|
|
describe('getCellDisplay', () => {
|
|
it('keeps a configured chart while the table view is selected', () => {
|
|
expect(getCellDisplay({ ...DATABASE_CELL, view: 'table' })).toEqual({
|
|
view: 'table',
|
|
chart: CHART,
|
|
})
|
|
})
|
|
|
|
it('reports no chart when a cell has never configured one', () => {
|
|
expect(getCellDisplay({ ...DATABASE_CELL, view: 'table', chart: undefined })).toEqual({
|
|
view: 'table',
|
|
chart: undefined,
|
|
})
|
|
})
|
|
})
|
|
|
|
describe('toQueryModel', () => {
|
|
it('tags a database cell with its row limit and database', () => {
|
|
expect(toQueryModel(DATABASE_CELL, 'select 3')).toEqual({
|
|
_tag: 'database',
|
|
uncheckedSql: 'select 3',
|
|
database_identifier: 'replica-1',
|
|
rowLimit: 50,
|
|
})
|
|
})
|
|
|
|
it('tags a log cell with its time range and no row limit', () => {
|
|
expect(toQueryModel(LOG_CELL, 'select 4')).toEqual({
|
|
_tag: 'logs',
|
|
uncheckedSql: 'select 4',
|
|
time_range: LOG_CELL.time_range,
|
|
})
|
|
})
|
|
})
|