From 2a099f6b289371b61af33f3ac4a9856680240d0d Mon Sep 17 00:00:00 2001 From: huyplb Date: Fri, 24 Jul 2026 23:20:09 -0600 Subject: [PATCH] fix(sql-editor): harden result paging and simplify recent UI code Fail closed on page wrap with offset, skip wraps for non-SELECT, ignore stale page fetches after a new Run, and gate Next on pageMeta. Also tidy the recent SQL Editor helpers. Co-authored-by: Cursor --- apps/web/src/backend/api/sql-execute.test.ts | 91 +++++++++++++- apps/web/src/backend/api/sql-execute.ts | 64 +++++++--- .../web/src/backend/api/sql-page-wrap.test.ts | 13 +- apps/web/src/backend/api/sql-page-wrap.ts | 15 +++ .../src/frontend/components/ProfileMenu.tsx | 9 +- .../components/sql-editor/DataGrid.tsx | 117 ++++++++++-------- .../components/sql-editor/ResultsPanel.tsx | 70 +++++------ .../sql-editor/SqlSchemaExplorer.tsx | 42 +++---- .../sql-editor/SqlVariablesPanel.tsx | 111 ++++++++--------- .../components/sql-editor/completion.ts | 32 ++--- .../components/sql-editor/sqlEditorBridge.ts | 9 ++ .../frontend/store/sqlEditorPaging.test.ts | 11 ++ apps/web/src/frontend/store/uiStore.ts | 19 ++- .../src/frontend/store/useSqlEditorStore.ts | 68 ++++++---- 14 files changed, 413 insertions(+), 258 deletions(-) create mode 100644 apps/web/src/frontend/store/sqlEditorPaging.test.ts diff --git a/apps/web/src/backend/api/sql-execute.test.ts b/apps/web/src/backend/api/sql-execute.test.ts index 53339c5..7df899f 100644 --- a/apps/web/src/backend/api/sql-execute.test.ts +++ b/apps/web/src/backend/api/sql-execute.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { rmSync } from 'node:fs'; @@ -96,6 +96,93 @@ describe('runStatements against a real SQLite file', () => { it('applies the row cap with the truncated flag', async () => { const results = await runStatements('sqlite', { connectionString: dbPath }, ['SELECT * FROM t;'], 2); - expect(results[0]).toMatchObject({ ok: true, rowCount: 2, truncated: true }); + expect(results[0]).toMatchObject({ ok: true, rowCount: 2, truncated: true, hasNext: true }); + }); + + it('pages SELECT with OFFSET (page 1 is not page 0)', async () => { + const page0 = await runStatements( + 'sqlite', + { connectionString: dbPath }, + ['SELECT id, name FROM t ORDER BY id;'], + 1, + undefined, + 0 + ); + const page1 = await runStatements( + 'sqlite', + { connectionString: dbPath }, + ['SELECT id, name FROM t ORDER BY id;'], + 1, + undefined, + 1 + ); + expect(page0[0]).toMatchObject({ ok: true, hasNext: true }); + expect(page1[0]).toMatchObject({ ok: true }); + if (page0[0]!.ok && page1[0]!.ok) { + expect(page0[0].rows[0]).toEqual([1, 'alpha']); + expect(page1[0].rows[0]).toEqual([2, 'beta']); + } + }); + + it('runs non-SELECT unwrapped at offset 0 (no wrap attempt)', async () => { + const spy = vi.spyOn(ConnectionFactory, 'executeOnConnection').mockResolvedValueOnce([]); + try { + const results = await runStatements( + 'sqlite', + { connectionString: dbPath }, + [`UPDATE t SET name = 'x' WHERE id = 1;`], + 10 + ); + expect(results[0]).toMatchObject({ ok: true, hasNext: false, rowCount: 0 }); + expect(spy).toHaveBeenCalledTimes(1); + expect(String(spy.mock.calls[0]![2])).toMatch(/^UPDATE t/i); + expect(String(spy.mock.calls[0]![2])).not.toContain('_fox_page'); + } finally { + spy.mockRestore(); + } + }); + + it('rejects paging a non-SELECT when offset > 0', async () => { + const spy = vi.spyOn(ConnectionFactory, 'executeOnConnection'); + try { + const results = await runStatements( + 'sqlite', + { connectionString: dbPath }, + [`UPDATE t SET name = 'x' WHERE id = 1;`], + 10, + undefined, + 10 + ); + expect(results[0]).toMatchObject({ ok: false }); + if (!results[0]!.ok) expect(results[0].error).toMatch(/non-SELECT/i); + expect(spy).not.toHaveBeenCalled(); + } finally { + spy.mockRestore(); + } + }); + + it('does not fall back to unwrapped SQL when offset > 0 and wrap fails', async () => { + const spy = vi + .spyOn(ConnectionFactory, 'executeOnConnection') + .mockRejectedValueOnce(new Error('wrap boom')); + try { + const results = await runStatements( + 'sqlite', + { connectionString: dbPath }, + ['SELECT id FROM t ORDER BY id;'], + 1, + undefined, + 1 + ); + expect(results[0]).toMatchObject({ ok: false }); + if (!results[0]!.ok) { + expect(results[0].error).toMatch(/Paging failed/i); + expect(results[0].error).toMatch(/wrap boom/); + } + expect(spy).toHaveBeenCalledTimes(1); + expect(String(spy.mock.calls[0]![2])).toContain('_fox_page'); + } finally { + spy.mockRestore(); + } }); }); diff --git a/apps/web/src/backend/api/sql-execute.ts b/apps/web/src/backend/api/sql-execute.ts index 245485a..22189d9 100644 --- a/apps/web/src/backend/api/sql-execute.ts +++ b/apps/web/src/backend/api/sql-execute.ts @@ -1,5 +1,5 @@ import { ConnectionFactory, getAdapter, type ConnectionOptions } from '@foxschema/core'; -import { trimPageProbe, wrapSqlForPage } from './sql-page-wrap'; +import { isPageableStatement, trimPageProbe, wrapSqlForPage } from './sql-page-wrap'; /** * Helpers behind POST /api/sql/execute (SQL Editor). One request = one @@ -105,6 +105,39 @@ export async function runStatements( const results: StatementResult[] = []; for (const sql of statements) { const started = Date.now(); + const pushErr = (message: string) => { + results.push({ ok: false, error: message, durationMs: Date.now() - started }); + }; + const pushUnwrappedOk = async () => { + const raw = await ConnectionFactory.executeOnConnection>( + dialect, + connection, + sql + ); + const shaped = shapeRows(raw, maxRows); + results.push({ + ok: true, + ...shaped, + hasNext: false, + durationMs: Date.now() - started, + }); + }; + + // Non-SELECT/DDL/utility: never wrap (avoids a failed subquery attempt). + // Paging requires a wrap — fail closed when offset > 0. + if (!isPageableStatement(sql)) { + if (offset > 0) { + pushErr('Cannot page a non-SELECT statement'); + continue; + } + try { + await pushUnwrappedOk(); + } catch (error: unknown) { + pushErr(error instanceof Error ? error.message : String(error)); + } + continue; + } + try { const paged = wrapSqlForPage(sql, dialect, offset, maxRows); const raw = await ConnectionFactory.executeOnConnection>( @@ -124,28 +157,19 @@ export async function runStatements( durationMs: Date.now() - started, }); } catch (error: unknown) { - // Fallback: run unwrapped (e.g. non-SELECT / dialect wrap rejection). + const wrapMsg = error instanceof Error ? error.message : String(error); + // Fail closed for Next/Prev: unwrapped SQL ignores OFFSET and would + // re-show page 0 while the UI advances pageIndex. + if (offset > 0) { + pushErr(`Paging failed: ${wrapMsg}`); + continue; + } + // offset === 0 only: dialect rejected the wrap (rare) — try raw SQL. try { - const raw = await ConnectionFactory.executeOnConnection>( - dialect, - connection, - sql - ); - const shaped = shapeRows(raw, maxRows); - results.push({ - ok: true, - ...shaped, - hasNext: false, - durationMs: Date.now() - started, - }); + await pushUnwrappedOk(); } catch (inner: unknown) { const message = inner instanceof Error ? inner.message : String(inner); - const wrapMsg = error instanceof Error ? error.message : String(error); - results.push({ - ok: false, - error: offset > 0 ? `${message} (page wrap: ${wrapMsg})` : message, - durationMs: Date.now() - started, - }); + pushErr(`${message} (page wrap: ${wrapMsg})`); } } } diff --git a/apps/web/src/backend/api/sql-page-wrap.test.ts b/apps/web/src/backend/api/sql-page-wrap.test.ts index 8ea5781..81cf9ce 100644 --- a/apps/web/src/backend/api/sql-page-wrap.test.ts +++ b/apps/web/src/backend/api/sql-page-wrap.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { trimPageProbe, wrapSqlForPage } from './sql-page-wrap'; +import { isPageableStatement, trimPageProbe, wrapSqlForPage } from './sql-page-wrap'; describe('sql-page-wrap', () => { it('wraps postgres-style with LIMIT/OFFSET and +1 probe', () => { @@ -26,4 +26,15 @@ describe('sql-page-wrap', () => { expect(t.hasNext).toBe(true); expect(t.truncated).toBe(true); }); + + it('isPageableStatement accepts SELECT / WITH…SELECT / VALUES only', () => { + expect(isPageableStatement('SELECT 1')).toBe(true); + expect(isPageableStatement('WITH c AS (SELECT 1 AS n) SELECT * FROM c')).toBe(true); + expect(isPageableStatement('VALUES (1), (2)')).toBe(true); + expect(isPageableStatement('INSERT INTO t VALUES (1)')).toBe(false); + expect(isPageableStatement('UPDATE t SET a = 1')).toBe(false); + expect(isPageableStatement('SET search_path TO public')).toBe(false); + expect(isPageableStatement('EXPLAIN SELECT 1')).toBe(false); + expect(isPageableStatement('WITH c AS (SELECT 1) INSERT INTO t SELECT * FROM c')).toBe(false); + }); }); diff --git a/apps/web/src/backend/api/sql-page-wrap.ts b/apps/web/src/backend/api/sql-page-wrap.ts index 1df1c03..95c34f0 100644 --- a/apps/web/src/backend/api/sql-page-wrap.ts +++ b/apps/web/src/backend/api/sql-page-wrap.ts @@ -3,12 +3,27 @@ * Fetches `limit + 1` rows so the caller can detect `hasNext` without a COUNT. */ +import { statementVerb } from '@foxschema/core'; + +/** Verbs that can appear as a subquery in `SELECT * FROM (…)` for paging. */ +const PAGEABLE_VERBS = new Set(['select', 'values']); + export function clampOffset(v: unknown): number { const n = typeof v === 'number' ? Math.floor(v) : Number.NaN; if (!Number.isFinite(n) || n < 0) return 0; return Math.min(n, 1_000_000); } +/** + * True when the statement is safe to wrap for OFFSET/LIMIT paging + * (SELECT / VALUES, including `WITH … AS (…) SELECT …`). + * Writes, DDL, SET, SHOW, EXPLAIN, CALL, etc. are not pageable. + */ +export function isPageableStatement(sql: string): boolean { + const verb = statementVerb(sql); + return verb !== null && PAGEABLE_VERBS.has(verb); +} + /** * Best-effort page wrap. Dialects without OFFSET still get a subquery + LIMIT * when offset is 0; non-zero offset uses the closest dialect syntax. diff --git a/apps/web/src/frontend/components/ProfileMenu.tsx b/apps/web/src/frontend/components/ProfileMenu.tsx index 17651a3..a50b675 100644 --- a/apps/web/src/frontend/components/ProfileMenu.tsx +++ b/apps/web/src/frontend/components/ProfileMenu.tsx @@ -46,12 +46,11 @@ export const ProfileMenu: React.FC = () => { return; } placeMenu(); - const onReposition = () => placeMenu(); - window.addEventListener('resize', onReposition); - window.addEventListener('scroll', onReposition, true); + window.addEventListener('resize', placeMenu); + window.addEventListener('scroll', placeMenu, true); return () => { - window.removeEventListener('resize', onReposition); - window.removeEventListener('scroll', onReposition, true); + window.removeEventListener('resize', placeMenu); + window.removeEventListener('scroll', placeMenu, true); }; }, [open]); diff --git a/apps/web/src/frontend/components/sql-editor/DataGrid.tsx b/apps/web/src/frontend/components/sql-editor/DataGrid.tsx index 3fc1fc6..81a65b8 100644 --- a/apps/web/src/frontend/components/sql-editor/DataGrid.tsx +++ b/apps/web/src/frontend/components/sql-editor/DataGrid.tsx @@ -157,6 +157,59 @@ function fitWidthFor(colName: string, sampleValues: unknown[]): number { return Math.min(COL_FIT_MAX_PX, Math.max(COL_MIN_PX, Math.round(raw))); } +function savePromptTitle(mode: 'scalar' | 'list' | 'table'): string { + if (mode === 'scalar') return 'Save cell as variable'; + if (mode === 'list') return 'Save column as list'; + return 'Save result as table'; +} + +function GridToolbar({ + label, + refreshing, + onRefresh, + onExport, +}: { + label?: string; + refreshing?: boolean; + onRefresh?: () => void; + onExport?: () => void; +}): React.ReactElement { + return ( +
+ {label && ( +
+ {label} +
+ )} + {onRefresh && ( + + )} + {onExport && ( + + )} +
+ ); +} + /** * Result grid — virtualized rows, column-level type colors (no per-cell regex). * Right-click a cell or column header to save as a SQL Editor variable. @@ -231,7 +284,8 @@ export const DataGrid: React.FC<{ const colKey = sourceColumns.join('\0'); const colKinds = useMemo( () => computeColKinds(sourceColumns, sourceRows), - [colKey, sourceRows.length, result.ok && result.ok ? result.rowCount : 0] + // eslint-disable-next-line react-hooks/exhaustive-deps -- colKey captures column identity; rowCount covers data refresh + [colKey, sourceRows.length, result.ok ? result.rowCount : 0] ); useEffect(() => { @@ -385,25 +439,7 @@ export const DataGrid: React.FC<{ if (!result.ok) { return (
-
- {label && ( -
- {label} -
- )} - {onRefresh && ( - - )} -
+
{result.error} @@ -433,35 +469,12 @@ export const DataGrid: React.FC<{ return (
-
- {label && ( -
- {label} -
- )} - {onRefresh && ( - - )} - {sourceColumns.length > 0 && ( - - )} -
+ 0 ? exportOrdered : undefined} + />
e.stopPropagation()} >
- {savePrompt.mode === 'scalar' - ? 'Save cell as variable' - : savePrompt.mode === 'list' - ? 'Save column as list' - : 'Save result as table'} + {savePromptTitle(savePrompt.mode)}
void): void { + const onUp = () => { + window.removeEventListener('mousemove', onMove); + window.removeEventListener('mouseup', onUp); + document.body.style.cursor = ''; + document.body.style.userSelect = ''; + }; + document.body.style.cursor = cursor; + document.body.style.userSelect = 'none'; + window.addEventListener('mousemove', onMove); + window.addEventListener('mouseup', onUp); +} + const statementLabel = (sql: string, index: number): string => { const compact = sql.replace(/\s+/g, ' ').trim(); return `Query ${index + 1} · ${compact.length > 48 ? compact.slice(0, 48) + '…' : compact}`; @@ -115,7 +129,7 @@ const ResizablePaneRow: React.FC<{ e.stopPropagation(); const startX = e.clientX; const startW = widths[index] ?? PANE_DEFAULT_PX; - const onMove = (ev: MouseEvent) => { + bindAxisDrag('col-resize', (ev) => { const next = Math.max(PANE_MIN_PX, startW + (ev.clientX - startX)); setWidths((prev) => { if (prev[index] === next) return prev; @@ -123,17 +137,7 @@ const ResizablePaneRow: React.FC<{ copy[index] = next; return copy; }); - }; - const onUp = () => { - window.removeEventListener('mousemove', onMove); - window.removeEventListener('mouseup', onUp); - document.body.style.cursor = ''; - document.body.style.userSelect = ''; - }; - document.body.style.cursor = 'col-resize'; - document.body.style.userSelect = 'none'; - window.addEventListener('mousemove', onMove); - window.addEventListener('mouseup', onUp); + }); }, [widths]); const startRowHeightResize = useCallback( @@ -141,25 +145,17 @@ const ResizablePaneRow: React.FC<{ e.preventDefault(); const startY = e.clientY; const startH = rowHeight; - const onMove = (ev: MouseEvent) => { + bindAxisDrag('row-resize', (ev) => { setRowHeight(Math.max(PANE_MIN_H_PX, startH + (ev.clientY - startY))); - }; - const onUp = () => { - window.removeEventListener('mousemove', onMove); - window.removeEventListener('mouseup', onUp); - document.body.style.cursor = ''; - document.body.style.userSelect = ''; - }; - document.body.style.cursor = 'row-resize'; - document.body.style.userSelect = 'none'; - window.addEventListener('mousemove', onMove); - window.addEventListener('mouseup', onUp); + }); }, [rowHeight] ); if (items.length === 0) return null; + const syncScroll = items.filter((x) => x.kind === 'grid').length > 1; + return (
onRefresh(item.connectionId) : undefined} - syncScrollRow={items.filter((x) => x.kind === 'grid').length > 1 ? syncRow : null} - onSyncScrollRow={ - items.filter((x) => x.kind === 'grid').length > 1 ? setSyncRow : undefined - } - pageIndex={page?.pageIndex ?? 0} + syncScrollRow={syncScroll ? syncRow : null} + onSyncScrollRow={syncScroll ? setSyncRow : undefined} + pageIndex={pageIndex} pageSize={page?.pageSize} - hasPrevPage={(page?.pageIndex ?? 0) > 0} - hasNextPage={ - page?.hasNext ?? - (item.result.ok && item.result.truncated) - } - pageLoading={page?.loading} + hasPrevPage={!refreshing && Boolean(page) && pageIndex > 0} + hasNextPage={!refreshing && Boolean(page?.hasNext)} + pageLoading={Boolean(refreshing || page?.loading)} onPrevPage={ - onPage + onPage && page && !refreshing ? () => onPage({ connectionId: item.connectionId, statementIndex: item.statementIndex, - pageIndex: Math.max(0, (page?.pageIndex ?? 0) - 1), + pageIndex: Math.max(0, pageIndex - 1), }) : undefined } onNextPage={ - onPage + onPage && page && !refreshing ? () => onPage({ connectionId: item.connectionId, statementIndex: item.statementIndex, - pageIndex: (page?.pageIndex ?? 0) + 1, + pageIndex: pageIndex + 1, }) : undefined } diff --git a/apps/web/src/frontend/components/sql-editor/SqlSchemaExplorer.tsx b/apps/web/src/frontend/components/sql-editor/SqlSchemaExplorer.tsx index cb22d5b..a07497d 100644 --- a/apps/web/src/frontend/components/sql-editor/SqlSchemaExplorer.tsx +++ b/apps/web/src/frontend/components/sql-editor/SqlSchemaExplorer.tsx @@ -4,7 +4,7 @@ import { useSyncStore } from '../../store/useSyncStore'; import { useSqlEditorStore } from '../../store/useSqlEditorStore'; import { effectiveConnectionIds } from '../../store/sqlEditorTabLogic'; import { TYPE_META } from '../SchemaTreePanel'; -import { insertAtCursor } from './sqlEditorBridge'; +import { filterCallParameters, insertAtCursor } from './sqlEditorBridge'; import type { DbObjectType, TableSchema } from '../../lib/types'; /** Categories shown in the SQL Editor schema browser (order = display order). */ @@ -189,17 +189,21 @@ const ObjectNode: React.FC<{ const meta = TYPE_META[table.objectType] ?? TYPE_META.TABLE; const insertName = quoteIfNeeded(table.name, dialect); const isRoutine = table.objectType === 'PROCEDURE' || table.objectType === 'FUNCTION'; - const params = isRoutine ? callParameters(table.parameters ?? []) : []; + const params = isRoutine ? filterCallParameters(table.parameters ?? []) : []; const columns = !isRoutine ? (table.columns ?? []).map((c) => ({ name: c.name, detail: c.type })) : []; - const insertRoutine = () => { - if (!isRoutine) { - insertAtCursor(`${insertName} `); + const insertObject = () => { + if (isRoutine) { + insertAtCursor(routineInsertText(insertName, table.objectType, params)); return; } - insertAtCursor(routineInsertText(insertName, table.objectType, params)); + insertAtCursor(`${insertName} `); + }; + + const insertIdent = (name: string) => { + insertAtCursor(`${quoteIfNeeded(name, dialect)} `); }; return ( @@ -220,7 +224,7 @@ const ObjectNode: React.FC<{ ? `Insert ${table.name}(${params.map((p) => `${p.mode} ${p.name}`).join(', ')})` : `Insert ${table.name}` } - onClick={insertRoutine} + onClick={insertObject} className="flex-1 flex items-center gap-1.5 min-w-0 text-left text-[12px] font-semibold text-slate-200 hover:text-cyan-300 py-0.5 truncate" > {meta.icon} @@ -232,6 +236,9 @@ const ObjectNode: React.FC<{ )}
+ {open && isRoutine && params.length === 0 && ( +

No parameters

+ )} {open && isRoutine && params.length > 0 && (
    {params.map((p, i) => ( @@ -239,7 +246,7 @@ const ObjectNode: React.FC<{