mirror of
https://github.com/supabase/supabase.git
synced 2026-10-05 09:25:06 +03:00
fix(studio): display diff for view-only notebook cell edits (#49901)
## Summary Fixed a bug in the AI Assistant notebook-update proposal preview where a `replace_cell` operation that only changed a cell's view (table ↔ chart) or chart parameters (type, x/y columns, cumulative, scale, labels) would show as a "Replaced" row but the expanded diff would appear empty. **Root cause:** The diff editor only compared the cell's SQL text; view and chart configuration were never considered, so changes to those aspects showed no diff. **Solution:** * Refactored `getCellMetadata` to return structured `NotebookCellFields` with separate `source` (database/time range) and `view` (table/chart) fields instead of a single concatenated string * Added `formatChartConfig` and `formatCellView` helpers to describe chart cells * Updated `getEntryMetadata` to diff source and view independently, showing only the fields that actually changed (e.g., "Table → Chart (bar, ...)" when only the view changed, with the unchanged database omitted) * If neither field changed, metadata is hidden entirely ## Test plan * Added test cases for: chart-view cells reporting a `view` field, view-only changes surfacing without the unchanged database, chart-parameter-only changes surfacing without the unchanged database, database-only changes surfacing without the unchanged view, and fully-unchanged replacements hiding metadata entirely * All 43 tests in the touched test file pass * `tsc --noEmit` on apps/studio shows no new type errors ## Summary by CodeRabbit * **Enhancements** * Improved AI Assistant notebook previews with clearer cell details, including source content and table or chart views. * Chart previews now show key configuration details, such as chart type and selected dimensions * Replacement previews highlight only the fields that changed and hide entries with no visible changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Notebook previews now distinguish cell content from its view, including table and chart details. * Chart previews display relevant configuration, such as chart type and axes. * Log previews include their formatted time range. * Replacement previews now show only the fields that changed. * **Bug Fixes** * Unchanged replacements are now hidden instead of displaying misleading content. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
1 parent
c6435f1cbe
commit
e7d91dbd06
2 files changed
+164
-29
No files matched your search
@@ -48,6 +48,38 @@ const agentDatabaseCell = (database_identifier?: string): AgentCell => ({
|
||||
database_identifier,
|
||||
})
|
||||
|
||||
const chartDatabaseCell = (id: string, ySeries: string[]): CellWire => ({
|
||||
_tag: 'database_cell',
|
||||
_id: id,
|
||||
sql: 'select 1',
|
||||
row_limit: 100,
|
||||
view: 'chart',
|
||||
chart: {
|
||||
type: 'bar',
|
||||
x_column: 'day',
|
||||
y_series: ySeries,
|
||||
cumulative: false,
|
||||
scale: 'linear',
|
||||
show_labels: false,
|
||||
},
|
||||
})
|
||||
|
||||
const agentChartDatabaseCell = (ySeries: string[], database_identifier?: string): AgentCell => ({
|
||||
_tag: 'database_cell',
|
||||
sql: 'select 1',
|
||||
row_limit: 100,
|
||||
database_identifier,
|
||||
view: 'chart',
|
||||
chart: {
|
||||
type: 'bar',
|
||||
x_column: 'day',
|
||||
y_series: ySeries,
|
||||
cumulative: false,
|
||||
scale: 'linear',
|
||||
show_labels: false,
|
||||
},
|
||||
})
|
||||
|
||||
const successfulDatabaseContext = (
|
||||
databases: Array<{ identifier: string; region: string }> = []
|
||||
): NotebookDatabaseContext => ({
|
||||
@@ -217,10 +249,11 @@ describe('getCellMetadata', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('labels an implicit primary database', () => {
|
||||
it('labels an implicit primary database and table view', () => {
|
||||
expect(getCellMetadata(wireDatabaseCell('cell-1'), successfulDatabaseContext())).toEqual({
|
||||
status: 'ready',
|
||||
text: 'Database: Primary',
|
||||
source: 'Database: Primary',
|
||||
view: 'Table',
|
||||
})
|
||||
})
|
||||
|
||||
@@ -232,7 +265,8 @@ describe('getCellMetadata', () => {
|
||||
)
|
||||
).toEqual({
|
||||
status: 'ready',
|
||||
text: 'Database: Replica',
|
||||
source: 'Database: Replica',
|
||||
view: 'Table',
|
||||
})
|
||||
})
|
||||
|
||||
@@ -251,13 +285,24 @@ describe('getCellMetadata', () => {
|
||||
wireDatabaseCell('cell-1', 'Signups', 'missing-database'),
|
||||
successfulDatabaseContext()
|
||||
)
|
||||
).toEqual({ status: 'ready', text: 'Database: Unknown' })
|
||||
).toEqual({ status: 'ready', source: 'Database: Unknown', view: 'Table' })
|
||||
})
|
||||
|
||||
it('labels a log cell with its formatted time range', () => {
|
||||
expect(getCellMetadata(wireLogCell('cell-1'), successfulDatabaseContext())).toEqual({
|
||||
status: 'ready',
|
||||
text: 'Time range: Last 7 days',
|
||||
source: 'Time range: Last 7 days',
|
||||
view: 'Table',
|
||||
})
|
||||
})
|
||||
|
||||
it('reports the chart summary as the view field for a chart-view database cell', () => {
|
||||
expect(
|
||||
getCellMetadata(chartDatabaseCell('cell-1', ['signups']), successfulDatabaseContext())
|
||||
).toEqual({
|
||||
status: 'ready',
|
||||
source: 'Database: Primary',
|
||||
view: 'Chart (bar, x: day, y: signups)',
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -309,7 +354,7 @@ describe('getEntryMetadata', () => {
|
||||
).toEqual({ status: 'loading' })
|
||||
})
|
||||
|
||||
it('returns a single line when replacement metadata is unchanged', () => {
|
||||
it('hides metadata entirely when a replacement changes neither the database nor the view', () => {
|
||||
expect(
|
||||
getEntryMetadata(
|
||||
{
|
||||
@@ -320,7 +365,58 @@ describe('getEntryMetadata', () => {
|
||||
},
|
||||
successfulDatabaseContext()
|
||||
)
|
||||
).toEqual({ status: 'ready', text: 'Database: Primary' })
|
||||
).toEqual({ status: 'hidden' })
|
||||
})
|
||||
|
||||
it('surfaces only the view change from table to chart, omitting the unchanged database', () => {
|
||||
expect(
|
||||
getEntryMetadata(
|
||||
{
|
||||
_tag: 'replaced',
|
||||
before: wireDatabaseCell('cell-1'),
|
||||
after: agentChartDatabaseCell(['signups']),
|
||||
operationIndex: 0,
|
||||
},
|
||||
successfulDatabaseContext()
|
||||
)
|
||||
).toEqual({
|
||||
status: 'ready',
|
||||
text: 'Table → Chart (bar, x: day, y: signups)',
|
||||
})
|
||||
})
|
||||
|
||||
it('surfaces only a chart parameter change, omitting the unchanged database', () => {
|
||||
expect(
|
||||
getEntryMetadata(
|
||||
{
|
||||
_tag: 'replaced',
|
||||
before: chartDatabaseCell('cell-1', ['signups']),
|
||||
after: agentChartDatabaseCell(['active_users']),
|
||||
operationIndex: 0,
|
||||
},
|
||||
successfulDatabaseContext()
|
||||
)
|
||||
).toEqual({
|
||||
status: 'ready',
|
||||
text: 'Chart (bar, x: day, y: signups) → Chart (bar, x: day, y: active_users)',
|
||||
})
|
||||
})
|
||||
|
||||
it('surfaces only a database change, omitting the unchanged view', () => {
|
||||
expect(
|
||||
getEntryMetadata(
|
||||
{
|
||||
_tag: 'replaced',
|
||||
before: chartDatabaseCell('cell-1', ['signups']),
|
||||
after: agentChartDatabaseCell(['signups'], 'replica-3'),
|
||||
operationIndex: 0,
|
||||
},
|
||||
successfulDatabaseContext([{ identifier: 'replica-3', region: 'us-east-1' }])
|
||||
)
|
||||
).toEqual({
|
||||
status: 'ready',
|
||||
text: 'Database: Primary → Database: Replica',
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -5,7 +5,7 @@ import type {
|
||||
NotebookCellDiffEntry,
|
||||
OperationResultCell,
|
||||
} from '@/data/content/notebooks/notebook-operations'
|
||||
import type { TimeRange } from '@/data/content/notebooks/notebook-schema'
|
||||
import type { ChartConfig, TimeRange } from '@/data/content/notebooks/notebook-schema'
|
||||
import type { Database } from '@/data/read-replicas/replicas-query'
|
||||
|
||||
type DatabaseDetails = Pick<Database, 'identifier'>
|
||||
@@ -23,6 +23,11 @@ export type NotebookDatabaseTarget =
|
||||
| { status: 'unknown' }
|
||||
| { status: 'error' }
|
||||
|
||||
export type NotebookCellFields =
|
||||
| { status: 'hidden' }
|
||||
| { status: 'loading' }
|
||||
| { status: 'ready'; source?: string; view?: string }
|
||||
|
||||
export type NotebookCellMetadata =
|
||||
| { status: 'hidden' }
|
||||
| { status: 'loading' }
|
||||
@@ -139,49 +144,83 @@ function formatDatabaseTarget(target: Exclude<NotebookDatabaseTarget, { status:
|
||||
}
|
||||
}
|
||||
|
||||
/** Metadata for a query cell's source parameters, kept separate from its SQL diff. */
|
||||
function formatChartConfig(chart: ChartConfig): string {
|
||||
const parts = [chart.type, `x: ${chart.x_column}`, `y: ${chart.y_series.join(', ')}`]
|
||||
if (chart.cumulative) parts.push('cumulative')
|
||||
if (chart.scale !== 'linear') parts.push(chart.scale)
|
||||
if (chart.show_labels) parts.push('labels')
|
||||
return parts.join(', ')
|
||||
}
|
||||
|
||||
function formatCellView(
|
||||
cell: Extract<OperationResultCell, { _tag: 'database_cell' | 'log_cell' }>
|
||||
): string {
|
||||
if ((cell.view ?? 'table') !== 'chart') return 'Table'
|
||||
return `Chart (${cell.chart !== undefined ? formatChartConfig(cell.chart) : 'unconfigured'})`
|
||||
}
|
||||
|
||||
export function getCellMetadata(
|
||||
cell: OperationResultCell,
|
||||
databaseContext: NotebookDatabaseContext
|
||||
): NotebookCellMetadata {
|
||||
): NotebookCellFields {
|
||||
switch (cell._tag) {
|
||||
case 'markdown_cell':
|
||||
return { status: 'hidden' }
|
||||
case 'database_cell': {
|
||||
const target = resolveNotebookDatabaseTarget(cell.database_identifier, databaseContext)
|
||||
return target.status === 'loading'
|
||||
? { status: 'loading' }
|
||||
: { status: 'ready', text: formatDatabaseTarget(target) }
|
||||
if (target.status === 'loading') return { status: 'loading' }
|
||||
|
||||
return { status: 'ready', source: formatDatabaseTarget(target), view: formatCellView(cell) }
|
||||
}
|
||||
case 'log_cell':
|
||||
return { status: 'ready', text: `Time range: ${formatTimeRange(cell.time_range)}` }
|
||||
return {
|
||||
status: 'ready',
|
||||
source: `Time range: ${formatTimeRange(cell.time_range)}`,
|
||||
view: formatCellView(cell),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/** Header metadata for a diff row, including a before → after pair on replacements. */
|
||||
/** Joins a cell's fields into display text, dropping the view when it's just the default table. */
|
||||
function formatCellFieldsText(fields: { source?: string; view?: string }): string {
|
||||
return [fields.source, fields.view === 'Table' ? undefined : fields.view]
|
||||
.filter((part): part is string => part !== undefined)
|
||||
.join(' · ')
|
||||
}
|
||||
|
||||
/** A field that's identical before and after carries no information about what changed, so it's dropped. */
|
||||
function diffField(before: string | undefined, after: string | undefined): string | undefined {
|
||||
if (before === after) return undefined
|
||||
return `${before ?? 'Not configured'} → ${after ?? 'Not configured'}`
|
||||
}
|
||||
|
||||
/** Header metadata for a diff row. On a replacement, only the fields that actually changed are shown. */
|
||||
export function getEntryMetadata(
|
||||
entry: NotebookCellDiffEntry,
|
||||
databaseContext: NotebookDatabaseContext
|
||||
): NotebookCellMetadata {
|
||||
if (entry._tag !== 'replaced') {
|
||||
return getCellMetadata(entry.cell, databaseContext)
|
||||
const fields = getCellMetadata(entry.cell, databaseContext)
|
||||
if (fields.status !== 'ready') return fields
|
||||
|
||||
const text = formatCellFieldsText(fields)
|
||||
return text === '' ? { status: 'hidden' } : { status: 'ready', text }
|
||||
}
|
||||
|
||||
const beforeMetadata = getCellMetadata(entry.before, databaseContext)
|
||||
const afterMetadata = getCellMetadata(entry.after, databaseContext)
|
||||
if (beforeMetadata.status === 'loading' || afterMetadata.status === 'loading') {
|
||||
return { status: 'loading' }
|
||||
}
|
||||
const before = getCellMetadata(entry.before, databaseContext)
|
||||
const after = getCellMetadata(entry.after, databaseContext)
|
||||
if (before.status === 'loading' || after.status === 'loading') return { status: 'loading' }
|
||||
|
||||
const beforeText = beforeMetadata.status === 'ready' ? beforeMetadata.text : null
|
||||
const afterText = afterMetadata.status === 'ready' ? afterMetadata.text : null
|
||||
if (beforeText === null && afterText === null) return { status: 'hidden' }
|
||||
if (beforeText === afterText) return { status: 'ready', text: afterText ?? 'No metadata' }
|
||||
const beforeSource = before.status === 'ready' ? before.source : undefined
|
||||
const afterSource = after.status === 'ready' ? after.source : undefined
|
||||
const beforeView = before.status === 'ready' ? before.view : undefined
|
||||
const afterView = after.status === 'ready' ? after.view : undefined
|
||||
|
||||
return {
|
||||
status: 'ready',
|
||||
text: `${beforeText ?? 'No metadata'} → ${afterText ?? 'No metadata'}`,
|
||||
}
|
||||
const parts = [diffField(beforeSource, afterSource), diffField(beforeView, afterView)].filter(
|
||||
(part): part is string => part !== undefined
|
||||
)
|
||||
|
||||
return parts.length > 0 ? { status: 'ready', text: parts.join(' · ') } : { status: 'hidden' }
|
||||
}
|
||||
|
||||
export type NotebookDiffSummary =
|
||||
|
||||
Reference in new issue
Block a user