From 6659707d4d00341ba0c56016fe62c74373831e17 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 10:27:52 -0600 Subject: [PATCH 1/7] fix(lokee): make revert actually reach the database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing an e2e test for the revert flow proved it had never once landed. The plan looked correct and the driver rejected it: 500 POST /lokee/databases/…/revert {"error":"near \".\": syntax error"} CREATE INDEX IDX_CUSTOMERS_EMAIL ON main.customers (email); Two defects in one statement. **SQLite cannot take a qualified table in CREATE INDEX.** The schema belongs on the index name — `CREATE INDEX main.idx ON customers(email)` — while every other dialect qualifies the table, which is why the shared generator does. SQLite gets a `createIndexStatement` hook that moves the qualifier across. **The generator used compare's match key as an identifier.** CLAUDE.md opens with "The compare key is not an identifier", and this spread the source IndexInfo then overwrote `name` with the uppercased key, emitting IDX_CUSTOMERS_EMAIL for idx_customers_email. SQLite folds case so it survived there; on a case-sensitive target it creates a differently named index and the next compare reads a rename that never happened. The cause was a type: `IndexDiff.source` omitted `name`, so the generator could not reach the identifier compare had been passing all along. Widened it — optional, not required, because this package is published and a required field would break consumers constructing these objects. Both are pinned by unit tests, since the e2e suite is not in the CI gate. Alongside the fix, the History toolbar work this proved out: - The version pickers and the capture credential were two connection-shaped controls on two rows, reading as "which of these databases am I looking at?". They are one database — recorded and live — so capture moved onto the pickers' row and defaults to the saved connection matching the history database. - The graph no longer follows the pickers. Choosing Version 1 as Original used to hide every version between the sides, so the history overview changed as a side effect of choosing what to compare. The checkboxes are the only filter now. - Execute was dead on the Blueprint tab because the data-loss acknowledgement lives on Migration SQL. A risk chip now rides beside the button, and a blocked button carries the reader to the decision instead of greying out. HistoryCompareBar renders in TopToolbar while the fetching lives in LokeeWeaveView, so it asks for work by bumping a counter in the store. Both watchers compare against the value seen at mount: the store outlives the component, and replaying the last request would re-snapshot the database on every visit. Also repairs the existing e2e suite, which CI does not run and which the shared SchemaBlueprint change had broken: the summary is `N versions` rather than `Total Versions: N`, and the inspector's sections carry `blueprint-*` ids — with indexes now rendering where the old panel stored but never showed them. Verified: revert e2e 3/3 (reads the SQLite file, not the UI), history e2e 6/6, 1613 unit tests, tsc clean, eslint 0 errors. Co-Authored-By: Claude Opus 5 --- apps/e2e/src/pages/LokeeHistoryPage.ts | 87 ++++++++- apps/e2e/src/tests/schema-history.test.ts | 9 +- apps/e2e/src/tests/schema-revert.test.ts | 169 ++++++++++++++++++ .../lokee-weave/HistoryCompareBar.tsx | 68 ++++++- .../components/lokee-weave/LokeeWeavePage.tsx | 23 +-- .../lokee-weave/LokeeWeaveView.test.tsx | 31 +++- .../components/lokee-weave/LokeeWeaveView.tsx | 162 +++++++++-------- .../lokee-weave/VersionCompareModal.tsx | 88 ++++++--- .../src/frontend/store/lokeeHistoryStore.ts | 23 +++ .../src/interfaces/diff.types.interface.ts | 13 +- .../src/modules/sql-generator.module.test.ts | 47 +++++ .../sql/src/modules/sql-generator.module.ts | 10 +- .../sqlLite/sqlite.sql-dialect.test.ts | 41 +++++ .../providers/sqlLite/sqlite.sql-dialect.ts | 18 ++ 14 files changed, 648 insertions(+), 141 deletions(-) create mode 100644 apps/e2e/src/tests/schema-revert.test.ts create mode 100644 packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts diff --git a/apps/e2e/src/pages/LokeeHistoryPage.ts b/apps/e2e/src/pages/LokeeHistoryPage.ts index 1af4980d..4131e32b 100644 --- a/apps/e2e/src/pages/LokeeHistoryPage.ts +++ b/apps/e2e/src/pages/LokeeHistoryPage.ts @@ -55,7 +55,7 @@ export class LokeeHistoryPage { (expected) => { const el = document.querySelector('[data-testid="lokee-summary"]'); const text = el?.textContent ?? ''; - return new RegExp(`Total Versions:\\s*${expected}\\b`, 'i').test(text); + return new RegExp(`\\b${expected}\\s+versions?\\b`, 'i').test(text); }, n, { timeout: timeoutMs } @@ -121,10 +121,21 @@ export class LokeeHistoryPage { } /** Sections render only once loaded, so callers must await the inspector first. */ - async inspectorHasSection(section: 'growth' | 'source' | 'columns' | 'indexes'): Promise { + async inspectorHasSection(section: 'growth' | 'source' | 'history'): Promise { return this.page.locator(`[data-testid="lokee-inspector-${section}"]`).isVisible(); } + /** + * Columns / indexes / keys / triggers are `SchemaBlueprint` now — the same + * component Compare Schema renders — so they carry `blueprint-*` ids wherever + * they appear rather than an inspector-specific one. + */ + async blueprintHasSection( + section: 'summary' | 'columns' | 'primary-key' | 'indexes' | 'foreign-keys' | 'triggers' + ): Promise { + return this.page.locator(`[data-testid="blueprint-${section}"]`).isVisible(); + } + async inspectorText(): Promise { return (await this.page.locator('[data-testid="lokee-object-inspector"]').innerText()) ?? ''; } @@ -145,7 +156,7 @@ export class LokeeHistoryPage { async versionCount(): Promise { const text = await this.summaryText(); - const match = text.match(/Total Versions:\s*(\d+)/i); + const match = text.match(/(\d+)\s+versions?\b/i); return match ? Number(match[1]) : 0; } @@ -158,4 +169,74 @@ export class LokeeHistoryPage { await box.waitFor({ state: 'visible', timeout: 10_000 }); if (!(await box.isChecked())) await box.check(); } + + // ── Compare versions → revert ──────────────────────────────────────────── + + /** Pick the Original side by its visible label (e.g. "Version 1"). */ + async selectOriginalVersion(label: string): Promise { + const select = this.page.locator('[data-testid="lokee-original-version"]'); + await select.waitFor({ state: 'visible', timeout: 20_000 }); + const option = select.locator('option', { hasText: label }); + await option.waitFor({ state: 'attached', timeout: 20_000 }); + const value = await option.getAttribute('value'); + if (!value) throw new Error(`No Original version option matching ${label}`); + await select.selectOption(value); + } + + async openCompareModal(): Promise { + await clickWhen(this.page, '[data-testid="lokee-compare-versions-btn"]'); + await this.page.waitForSelector('[data-testid="lokee-version-compare"][data-state="ready"]', { + timeout: 30_000, + }); + } + + async compareTab(tab: 'DIFF' | 'DDL_DIFF' | 'SQL'): Promise { + await clickWhen(this.page, `[data-testid="lokee-cmp-tab-${tab}"]`); + } + + async migrationSqlText(): Promise { + await this.compareTab('SQL'); + const pane = this.page.locator('[data-testid="lokee-cmp-ddl"]'); + await pane.waitFor({ state: 'visible', timeout: 20_000 }); + return pane.innerText(); + } + + /** + * Apply the revert. A lossy plan parks the button on "Review data loss…", + * which navigates to Migration SQL rather than running — acknowledge there, + * then press it again. A safe plan runs on the first press. + */ + async executeRevert(): Promise { + const run = this.page.locator('[data-testid="lokee-cmp-run-revert"]'); + await run.waitFor({ state: 'visible', timeout: 20_000 }); + await this.page.waitForFunction( + () => { + const el = document.querySelector('[data-testid="lokee-cmp-run-revert"]'); + return el instanceof HTMLButtonElement && !el.disabled; + }, + { timeout: 30_000 } + ); + if ((await run.innerText()).includes('Review data loss')) { + await run.click(); + const ack = this.page.locator('[data-testid="lokee-cmp-confirm-lossy"]'); + await ack.waitFor({ state: 'visible', timeout: 10_000 }); + await ack.check(); + } + await run.click(); + } + + /** Visible toast text, so a failed revert reports the driver's reason. */ + async toastText(): Promise { + const toasts = this.page.locator('[data-testid="app-toast"]'); + const count = await toasts.count(); + const parts: string[] = []; + for (let i = 0; i < count; i++) { + parts.push((await toasts.nth(i).innerText().catch(() => '')) ?? ''); + } + return parts.join(' | '); + } + + async compareModalOpen(): Promise { + return this.page.locator('[data-testid="lokee-version-compare"]').isVisible(); + } } diff --git a/apps/e2e/src/tests/schema-history.test.ts b/apps/e2e/src/tests/schema-history.test.ts index a82a9bb9..6282bae1 100644 --- a/apps/e2e/src/tests/schema-history.test.ts +++ b/apps/e2e/src/tests/schema-history.test.ts @@ -110,9 +110,12 @@ describe.skipIf(!ready)('Schema Sync · History (SQLite)', () => { await history.clickObjectNamed('customers'); const text = await history.inspectorText(); expect(text).toMatch(/email/i); - expect(await driver.locator('[data-testid="lokee-inspector-columns"]').isVisible()).toBe(true); - expect(await driver.locator('[data-testid="lokee-inspector-indexes"]').count()).toBe(0); - expect(await driver.locator('[data-testid="lokee-inspector-triggers"]').isVisible()).toBe(true); + // The blueprint is Compare Schema's own component now, so the sections + // carry `blueprint-*` ids — and indexes, which the old bespoke inspector + // stored but never surfaced, are among them. + expect(await history.blueprintHasSection('columns')).toBe(true); + expect(await history.blueprintHasSection('indexes')).toBe(true); + expect(await history.blueprintHasSection('triggers')).toBe(true); expect(text).toMatch(/trg_customers_audit/i); expect(await driver.locator('[data-testid="lokee-inspector-script-diff"]').isVisible()).toBe(true); }); diff --git a/apps/e2e/src/tests/schema-revert.test.ts b/apps/e2e/src/tests/schema-revert.test.ts new file mode 100644 index 00000000..80e105be --- /dev/null +++ b/apps/e2e/src/tests/schema-revert.test.ts @@ -0,0 +1,169 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Schema Sync → History → Compare versions → revert, against a local SQLite file. + * + * This is the one flow whose correctness cannot be argued from unit tests: the + * revert plan is built from stored objects, but whether it *lands* depends on + * the generator, the driver and the live schema agreeing. Everything up to the + * button was already covered; this drives the button and then reads the file on + * disk to prove the schema actually moved. + * + * Requires `npm run dev` and `sqlite3` on PATH. Skips when sqlite3 is missing. + */ +import { describe, it, beforeAll, afterAll, expect } from 'vitest'; +import { execFileSync, execSync } from 'node:child_process'; +import { mkdirSync, rmSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import type { Page } from 'playwright'; +import { buildDriver, quitDriver } from '../helpers/driver.js'; +import { AppPage } from '../pages/AppPage.js'; +import { SqlEditorPage } from '../pages/SqlEditorPage.js'; +import { LokeeHistoryPage } from '../pages/LokeeHistoryPage.js'; + +const RUN = Date.now().toString(36); +const DIR = `/tmp/foxschema-e2e-schema-revert-${RUN}`; +const DB = join(DIR, 'revert.db'); +const NAME = `E2E Revert ${RUN}`; + +function hasSqlite3(): boolean { + try { + execSync('which sqlite3', { stdio: 'ignore' }); + return true; + } catch { + return false; + } +} + +function sqlite(input: string): string { + return execFileSync('sqlite3', [DB], { input, encoding: 'utf8' }); +} + +function schemaText(): string { + return execFileSync('sqlite3', [DB, '.schema'], { encoding: 'utf8' }); +} + +const ready = hasSqlite3(); + +describe.skipIf(!ready)('Schema Sync · History revert (SQLite)', () => { + let driver: Page; + let app: AppPage; + let sql: SqlEditorPage; + let history: LokeeHistoryPage; + + beforeAll(async () => { + rmSync(DIR, { recursive: true, force: true }); + mkdirSync(DIR, { recursive: true }); + sqlite(` +CREATE TABLE customers ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + email TEXT +); +CREATE INDEX idx_customers_email ON customers(email); +INSERT INTO customers (id, name, email) VALUES (1, 'Ada', 'ada@example.com'); +`); + expect(existsSync(DB)).toBe(true); + + driver = await buildDriver(); + app = new AppPage(driver); + sql = new SqlEditorPage(driver); + history = new LokeeHistoryPage(driver); + + await app.open(); + await sql.resetPersistedEditorState(); + await driver.reload(); + await driver.waitForSelector('[data-testid="toolbar"]'); + + await sql.addSqliteCredential(NAME, DB); + await driver.locator('[data-testid="view-sync-btn"]').click(); + await driver.waitForSelector('[data-testid="sync-pane-switcher"]'); + await history.selectSavedTargetByName(NAME); + }, 120_000); + + afterAll(async () => { + if (driver) await quitDriver(driver); + rmSync(DIR, { recursive: true, force: true }); + }); + + it('records the baseline, then a version with the index dropped', async () => { + await history.snapshotTarget(); + + // The change to undo. Dropping an index is the cleanest case to assert on: + // reverting it is a pure CREATE, so the plan is safe rather than lossy and + // the file tells us plainly whether it landed. + sqlite('DROP INDEX idx_customers_email;\n'); + expect(schemaText()).not.toMatch(/idx_customers_email/i); + + await history.snapshotTarget(); + await history.openHistoryPane(); + await history.selectHistoryDatabaseContaining(RUN); + await history.waitForGraph(); + await expect.poll(async () => history.versionCount(), { timeout: 30_000 }).toBe(2); + }, 120_000); + + it('shows the reverse DDL before anything is applied', async () => { + await history.selectOriginalVersion('Version 1'); + await history.openCompareModal(); + + // The tree is the shared SchemaDiffTree; the blueprint is SchemaBlueprint. + expect(await driver.locator('[data-testid="lokee-cmp-summary"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="schema-blueprint"]').isVisible()).toBe(true); + + const migration = await history.migrationSqlText(); + expect(migration, migration).toMatch(/CREATE INDEX/i); + expect(migration, migration).toMatch(/idx_customers_email/i); + + // Nothing has run yet — the point of comparing before deciding. + expect(schemaText()).not.toMatch(/idx_customers_email/i); + }, 120_000); + + it('applies the revert and records it as a new version', async () => { + // Record what the button actually sent. A silent no-op and a rejected + // request look identical from the file system, and toasts expire before a + // 30s poll finishes — so capture the response rather than infer it. + const calls: string[] = []; + driver.on('response', (res) => { + const url = res.url(); + if (url.includes('/lokee/') && url.includes('/revert') && res.request().method() === 'POST') { + calls.push(`${res.status()} ${url}`); + void res + .text() + .then((body) => calls.push(`body: ${body.slice(0, 300)}`)) + .catch(() => undefined); + } + }); + + await history.executeRevert(); + + // The file is the source of truth: the index is back. If it is not, the + // toast carries the driver's reason — without it this failure is just + // "nothing happened", which is the least actionable thing a test can say. + try { + await expect + .poll(() => schemaText(), { timeout: 30_000 }) + .toMatch(/idx_customers_email/i); + } catch (error) { + throw new Error( + `Revert did not reach the database.\nPOSTs: ${calls.join(' :: ') || '(none)'}\nToast: ${await history.toastText()}\nSchema: ${schemaText()}`, + { cause: error } + ); + } + + // A revert is itself a migration, so history gains a version for it rather + // than rewriting the one it reverted to. + await driver.waitForSelector('[data-testid="lokee-version-compare"]', { + state: 'detached', + timeout: 30_000, + }); + await expect + .poll(async () => history.versionCount(), { timeout: 30_000 }) + .toBeGreaterThanOrEqual(3); + + // The row survived: reverting an index must not rebuild the table. + const rows = sqlite('SELECT count(*) FROM customers;\n').trim(); + expect(rows).toBe('1'); + }, 180_000); +}); diff --git a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx index 85fb5e59..d06832b4 100644 --- a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx +++ b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx @@ -7,8 +7,10 @@ * are versions of one captured database instead of two live connections. */ import React, { useMemo } from 'react'; -import { ArrowLeftRight, ArrowRight } from 'lucide-react'; +import { ArrowLeftRight, ArrowRight, Camera, Loader2, RefreshCw } from 'lucide-react'; import { useLokeeHistoryStore } from '../../store/lokeeHistoryStore'; +import { useSyncStore } from '../../store/useSyncStore'; +import { SQL_ICON_STROKE } from '../sql-editor/sqlIconStyle'; import { historyVersionLabel, lokeeDatabaseLabel, @@ -26,6 +28,12 @@ export function HistoryCompareBar(): React.ReactElement { const setOriginalVersionId = useLokeeHistoryStore((s) => s.setOriginalVersionId); const setTargetVersionId = useLokeeHistoryStore((s) => s.setTargetVersionId); const swapSides = useLokeeHistoryStore((s) => s.swapSides); + const connections = useSyncStore((s) => s.connections); + const captureConnectionId = useLokeeHistoryStore((s) => s.captureConnectionId); + const setCaptureConnectionId = useLokeeHistoryStore((s) => s.setCaptureConnectionId); + const capturing = useLokeeHistoryStore((s) => s.capturing); + const requestCapture = useLokeeHistoryStore((s) => s.requestCapture); + const requestRefresh = useLokeeHistoryStore((s) => s.requestRefresh); const newestFirst = useMemo(() => sortVersionsNewestFirst(versions), [versions]); const resolved = useMemo( @@ -37,8 +45,8 @@ export function HistoryCompareBar(): React.ReactElement { Boolean(resolved.original && resolved.target && resolved.original.id === resolved.target.id); return ( -
-
+
+
Original @@ -99,7 +107,7 @@ export function HistoryCompareBar(): React.ReactElement {
-
+
Target @@ -126,6 +134,58 @@ export function HistoryCompareBar(): React.ReactElement { ))}
+ + {/* Capture lives on this row too. It used to sit on a second bar with its + own credential picker, which read as a *third* connection control next + to the two above it — three pickers for two ideas. */} +
+
+
+ Capture +
+ +
+
+ + +
+
); } diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx index 371267cf..3ee66e11 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx @@ -58,8 +58,6 @@ export interface LokeeWeavePageProps { ) => Promise; /** Shorter header when shown inside Schema Sync. */ embedded?: boolean; - /** Original + Target version ids from the Compare-style History bar. */ - compareVersionIds?: readonly string[]; } const FILTERABLE_TYPES: LokeeObjectType[] = [ @@ -187,15 +185,14 @@ export const LokeeWeavePage: React.FC = ({ onSelectObject, onSaveVersionMeta, embedded = false, - compareVersionIds, }) => { - const [filters, setFilters] = useState(() => { - const base = freshFilters(); - if (compareVersionIds && compareVersionIds.length > 0) { - return { ...base, versionIds: new Set(compareVersionIds) }; - } - return base; - }); + // Deliberately independent of the Original/Target pickers. Driving this + // filter from them hid every version between the two sides — pick "Version 1" + // as Original and Version 2 silently vanished from the graph — so the history + // overview changed under the reader as a side effect of choosing what to + // compare. The pickers choose the diff; the graph shows the history; the + // checkboxes below are the only thing that filters it. + const [filters, setFilters] = useState(freshFilters); const [locked, setLocked] = useState(true); const [selectedVersionId, setSelectedVersionId] = useState(null); const [editName, setEditName] = useState(''); @@ -207,12 +204,6 @@ export const LokeeWeavePage: React.FC = ({ [dto.versions, selectedVersionId] ); - const compareKey = (compareVersionIds ?? []).join('|'); - useEffect(() => { - if (!compareKey) return; - setFilters((f) => ({ ...f, versionIds: new Set(compareKey.split('|')) })); - }, [compareKey]); - useEffect(() => { if (!selectedVersion) { setEditName(''); diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx index 8a5d5013..6d184870 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx @@ -9,7 +9,7 @@ */ import React from 'react'; import { describe, expect, it, vi, beforeEach } from 'vitest'; -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; import type { VersionGraphDTO } from './graphTypes'; const listLokeeDatabases = vi.fn(); @@ -108,7 +108,8 @@ describe('LokeeWeaveView', () => { await waitFor(() => expect(screen.getByTestId('graph')).toBeTruthy()); // The subtitle must name the database, not the saved connection. expect(screen.getByTestId('graph').textContent).toContain('[postgres] localhost/foxdb.public'); - expect(screen.getByTestId('lokee-weave-chrome')).toBeTruthy(); + // The chrome row is gone — Refresh and Capture moved into HistoryCompareBar. + expect(screen.queryByTestId('lokee-weave-chrome')).toBeNull(); }); it('shows an empty state rather than an empty canvas', async () => { @@ -188,7 +189,10 @@ describe('LokeeWeaveView', () => { expect(useLokeeHistoryStore.getState().versions.map((v) => v.id)).toEqual(['v2', 'v1']); }); - it('captures a schema from the chosen credential', async () => { + it('captures when the toolbar bar asks for it', async () => { + // The credential picker and Capture button moved to HistoryCompareBar, which + // renders in TopToolbar. It asks by bumping the store counter, so that is + // the seam to test here rather than a button this component no longer owns. listLokeeDatabases.mockResolvedValueOnce([]).mockResolvedValue([DB]); loadVersionGraph.mockResolvedValue({ ...DTO, truncatedObjects: false }); captureSchema.mockResolvedValue({ @@ -202,12 +206,12 @@ describe('LokeeWeaveView', () => { }); render(); + await waitFor(() => expect(listLokeeDatabases).toHaveBeenCalled()); - await waitFor(() => expect(screen.getByTestId('lokee-capture-btn')).toBeTruthy()); - fireEvent.change(screen.getByTestId('lokee-capture-connection'), { - target: { value: 'c1' }, + act(() => { + useLokeeHistoryStore.getState().setCaptureConnectionId('c1'); + useLokeeHistoryStore.getState().requestCapture(); }); - fireEvent.click(screen.getByTestId('lokee-capture-btn')); await waitFor(() => expect(captureSchema).toHaveBeenCalledWith( @@ -218,4 +222,17 @@ describe('LokeeWeaveView', () => { expect(toast).toHaveBeenCalledWith(expect.objectContaining({ title: 'Captured v1' })) ); }); + + it('does not capture or refetch on mount — the counters start at zero', async () => { + // A naive effect on the request counters would fire once per mount, so a + // visit to History would silently snapshot the database. + listLokeeDatabases.mockResolvedValue([DB]); + loadVersionGraph.mockResolvedValue({ ...DTO, truncatedObjects: false }); + + render(); + await waitFor(() => expect(screen.getByTestId('graph')).toBeTruthy()); + + expect(captureSchema).not.toHaveBeenCalled(); + expect(loadVersionGraph).toHaveBeenCalledTimes(1); + }); }); diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx index 4fe07a03..e4728763 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx @@ -9,7 +9,7 @@ * load, or be empty is handled here, so the graph itself remains trivially * testable with a fixture and has no idea a network exists. */ -import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { ArrowRight, Camera, GitBranch, Loader2, RefreshCw, TriangleAlert } from 'lucide-react'; import { LokeeWeavePage } from './LokeeWeavePage'; import { VersionCompareModal } from './VersionCompareModal'; @@ -73,12 +73,18 @@ export function LokeeWeaveView({ const setStoreDatabases = useLokeeHistoryStore((s) => s.setDatabases); const setStoreVersions = useLokeeHistoryStore((s) => s.setVersions); const originalVersionId = useLokeeHistoryStore((s) => s.originalVersionId); + const captureConnectionId = useLokeeHistoryStore((s) => s.captureConnectionId); + const setCaptureConnectionId = useLokeeHistoryStore((s) => s.setCaptureConnectionId); + const capturing = useLokeeHistoryStore((s) => s.capturing); + const setCapturing = useLokeeHistoryStore((s) => s.setCapturing); + const captureRequest = useLokeeHistoryStore((s) => s.captureRequest); + const refreshRequest = useLokeeHistoryStore((s) => s.refreshRequest); const targetVersionId = useLokeeHistoryStore((s) => s.targetVersionId); const [databases, setDatabases] = useState([]); - const [captureConnectionId, setCaptureConnectionId] = useState(''); + const [dto, setDto] = useState(EMPTY_DTO); const [loading, setLoading] = useState(true); - const [capturing, setCapturing] = useState(false); + const [error, setError] = useState(null); const [selectedObject, setSelectedObject] = useState(null); // Bumped to re-run the effect; a plain refetch() would race the in-flight one. @@ -135,11 +141,39 @@ export function LokeeWeaveView({ return [...new Set(ids)]; }, [dto.versions, originalVersionId, targetVersionId]); + /** + * Default the capture credential to the saved connection that *is* the history + * database being viewed. Two connection-shaped controls sitting side by side + * with unrelated values is what made the bar read as "which of these two + * databases am I looking at?" — when they are the same database, one recorded + * and one live. Compare's Target is the fallback, as before. + */ + const activeDatabase = useMemo( + () => databases.find((d) => d.id === activeId), + [databases, activeId] + ); + const matchingCredentialId = useMemo(() => { + if (!activeDatabase) return undefined; + const same = (a?: string | null, b?: string | null) => + (a ?? '').toLowerCase() === (b ?? '').toLowerCase(); + return connections.find( + (c) => + same(c.dialect, activeDatabase.dialect) && + same(c.host, activeDatabase.host) && + same(c.database, activeDatabase.database) + )?.id; + }, [connections, activeDatabase]); + useEffect(() => { - if (!captureConnectionId && selectedTargetConnectionId) { - setCaptureConnectionId(selectedTargetConnectionId); - } - }, [captureConnectionId, selectedTargetConnectionId]); + if (captureConnectionId) return; + const next = matchingCredentialId ?? selectedTargetConnectionId; + if (next) setCaptureConnectionId(next); + }, [ + captureConnectionId, + matchingCredentialId, + selectedTargetConnectionId, + setCaptureConnectionId, + ]); useEffect(() => { let cancelled = false; @@ -192,6 +226,16 @@ export function LokeeWeaveView({ const refresh = useCallback(() => setReloadToken((n) => n + 1), []); + // HistoryCompareBar renders in TopToolbar, so it asks for work by bumping a + // counter rather than holding a callback. 0 is the initial value — acting on + // it would refetch on every mount. + const seenRefreshRequest = useRef(refreshRequest); + useEffect(() => { + if (refreshRequest === seenRefreshRequest.current) return; + seenRefreshRequest.current = refreshRequest; + refresh(); + }, [refreshRequest, refresh]); + const saveVersionMeta = useCallback( async (versionId: string, patch: { name: string; description: string }) => { if (!activeId) return; @@ -266,74 +310,33 @@ export function LokeeWeaveView({ } finally { setCapturing(false); } - }, [captureConnectionId, capturing, connections, refresh, bumpLokeeEpoch, setStoreDatabaseId]); + }, [ + captureConnectionId, + capturing, + connections, + refresh, + bumpLokeeEpoch, + setStoreDatabaseId, + setCapturing, + ]); + + // Fire on the counter alone, through a ref. Depending on `runCapture` here + // is an infinite loop: it reads `capturing`, and it also *sets* it, so every + // toggle gives the callback a new identity, re-runs this effect while the + // counter is still non-zero, and captures again. Caught by a hanging test. + // + // The baseline is whatever the counter held at mount, not zero: the store + // outlives this component, so leaving History after a capture and coming + // back would otherwise re-fire that request and snapshot the database again. + const runCaptureRef = useRef(runCapture); + runCaptureRef.current = runCapture; + const seenCaptureRequest = useRef(captureRequest); + useEffect(() => { + if (captureRequest === seenCaptureRequest.current) return; + seenCaptureRequest.current = captureRequest; + void runCaptureRef.current(); + }, [captureRequest]); - const chrome = ( -
- {!embedded && ( - - )} - -
- - -
-
- ); @@ -366,8 +369,7 @@ export function LokeeWeaveView({ if (loading) { return (
- {chrome} -
+
Loading schema history…
@@ -378,8 +380,7 @@ export function LokeeWeaveView({ if (error) { return (
- {chrome} -
+
Could not load schema history
{error}
@@ -399,8 +400,7 @@ export function LokeeWeaveView({ if (!activeId || dto.versions.length === 0) { return (
- {chrome} -
+
No schema history yet
@@ -416,7 +416,6 @@ export function LokeeWeaveView({ return (
- {chrome} {compareBar} {dto.truncatedObjects && (
@@ -430,7 +429,6 @@ export function LokeeWeaveView({ dto={dto} subtitle={subtitle} embedded={embedded} - compareVersionIds={compareVersionIds} onSelectObject={handleSelectObject} onSaveVersionMeta={saveVersionMeta} /> diff --git a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx index 25e63e16..739a96e4 100644 --- a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx +++ b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx @@ -188,6 +188,27 @@ export function VersionCompareModal({ if (data) void loadPlan(); }, [data, loadPlan]); + /** + * Why Execute cannot run yet — the empty string means it can. + * + * The button lives in the toolbar and the data-loss acknowledgement lives on + * the Migration SQL tab, so a reader on the Blueprint tab saw a greyed-out + * button with no stated reason. Comparing before deciding is the whole point + * of this dialog; the decision has to be reachable from wherever you are. + */ + const blockedReason = useMemo(() => { + if (!captureConnectionId) return 'Choose a credential in the bar above to run this.'; + if (!plan) return planning ? 'Still planning…' : 'No plan yet.'; + if (plan.alreadyAtTarget) return 'The live schema already matches Original.'; + if (plan.statements.length === 0) return 'Nothing to apply.'; + if (plan.reversal.risk === 'blocked') { + return 'Blocked — this cannot be applied without losing data the schema cannot restore.'; + } + if (plan.reversal.risk === 'lossy' && !confirmLossy) return 'ACK_LOSSY'; + return ''; + }, [captureConnectionId, plan, planning, confirmLossy]); + const needsLossyAck = blockedReason === 'ACK_LOSSY'; + const runRevert = useCallback(async () => { if (!captureConnectionId || !plan) return; setRunning(true); @@ -340,29 +361,50 @@ export function VersionCompareModal({ testIdPrefix="lokee-cmp" size="compact" /> - +
+ {/* Risk travels with the button, not just with the tab + that happens to show the statements. */} + {plan && plan.statements.length > 0 && ( + + {riskStyle(plan.reversal.risk).label} + {plan.reversal.lossyCount > 0 ? ` · ${plan.reversal.lossyCount}` : ''} + + )} + +
diff --git a/apps/web/src/frontend/store/lokeeHistoryStore.ts b/apps/web/src/frontend/store/lokeeHistoryStore.ts index 7fdb8852..b0dc750b 100644 --- a/apps/web/src/frontend/store/lokeeHistoryStore.ts +++ b/apps/web/src/frontend/store/lokeeHistoryStore.ts @@ -25,6 +25,21 @@ interface LokeeHistoryState { setDatabases: (rows: LokeeDatabase[]) => void; setVersions: (rows: HistoryVersionOption[]) => void; swapSides: () => void; + + /** + * Capture belongs to the same toolbar row as the pickers, but the fetching + * and the graph live in LokeeWeaveView. Rather than passing callbacks + * through the store, the bar bumps a counter and the view watches it — the + * store stays plain data, which is what keeps it testable. + */ + captureConnectionId: string; + setCaptureConnectionId: (id: string) => void; + capturing: boolean; + setCapturing: (busy: boolean) => void; + captureRequest: number; + requestCapture: () => void; + refreshRequest: number; + requestRefresh: () => void; } export const useLokeeHistoryStore = create((set, get) => ({ @@ -41,6 +56,14 @@ export const useLokeeHistoryStore = create((set, get) => ({ setTargetVersionId: (targetVersionId) => set({ targetVersionId }), setDatabases: (databases) => set({ databases }), setVersions: (versions) => set({ versions }), + captureConnectionId: '', + setCaptureConnectionId: (captureConnectionId) => set({ captureConnectionId }), + capturing: false, + setCapturing: (capturing) => set({ capturing }), + captureRequest: 0, + requestCapture: () => set({ captureRequest: get().captureRequest + 1 }), + refreshRequest: 0, + requestRefresh: () => set({ refreshRequest: get().refreshRequest + 1 }), swapSides: () => { const next = swapHistoryCompare(get().versions, { originalVersionId: get().originalVersionId, diff --git a/packages/sql/src/interfaces/diff.types.interface.ts b/packages/sql/src/interfaces/diff.types.interface.ts index 28e2b2fa..8950ab4b 100644 --- a/packages/sql/src/interfaces/diff.types.interface.ts +++ b/packages/sql/src/interfaces/diff.types.interface.ts @@ -10,10 +10,19 @@ export interface ColumnDiff { } export interface IndexDiff { + /** Uppercased compare-key match name — NOT a real identifier, see source.name. */ name: string; status: 'ADDED' | 'REMOVED' | 'MODIFIED' | 'UNCHANGED'; - source?: { columns: string[]; unique: boolean; constraint?: boolean }; - target?: { columns: string[]; unique: boolean; constraint?: boolean }; + /** + * `name` here is the index's own identifier in its native casing, which is + * what DDL must use. Compare has always passed the whole IndexInfo through; + * only this declaration hid the field, so generators reached for the + * uppercased key instead and emitted IDX_CUSTOMERS_EMAIL for + * idx_customers_email. Optional rather than required because this package is + * published — every producer inside the repo sets it. + */ + source?: { name?: string; columns: string[]; unique: boolean; constraint?: boolean }; + target?: { name?: string; columns: string[]; unique: boolean; constraint?: boolean }; /** * True when this ADDED/REMOVED pair is only an index rename: same columns + * uniqueness as an unmatched index on the other side. Does not mark the table diff --git a/packages/sql/src/modules/sql-generator.module.test.ts b/packages/sql/src/modules/sql-generator.module.test.ts index c087afed..d3be2dc0 100644 --- a/packages/sql/src/modules/sql-generator.module.test.ts +++ b/packages/sql/src/modules/sql-generator.module.test.ts @@ -1109,3 +1109,50 @@ describe('SqlGeneratorModule foreign key hardening', () => { ); }); }); + +describe('CREATE INDEX identifier', () => { + it('uses the index its own name, not compare\'s uppercased match key', () => { + // CLAUDE.md: "The compare key is not an identifier." The generator spread + // the source IndexInfo then overwrote `name` with the key, so a revert + // emitted IDX_CUSTOMERS_EMAIL for idx_customers_email — a different index + // on any dialect that does not fold case, and a phantom rename next compare. + const gen = new SqlGeneratorModule(); + const steps = gen.generateMigrationPlan( + [ + { + tableName: 'CUSTOMERS', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [], + foreignKeyDiffs: [], + indexDiffs: [ + { + name: 'IDX_CUSTOMERS_EMAIL', + status: 'ADDED', + source: { name: 'idx_customers_email', columns: ['email'], unique: false }, + }, + ], + sourceTable: { + name: 'customers', + objectType: 'TABLE', + columns: [], + indices: [], + foreignKeys: [], + }, + targetTable: { + name: 'customers', + objectType: 'TABLE', + columns: [], + indices: [], + foreignKeys: [], + }, + }, + ], + 'postgres' + ); + const sql = steps.flatMap((s) => s.statements).join('\n'); + expect(sql).toMatch(/idx_customers_email/); + expect(sql).not.toMatch(/IDX_CUSTOMERS_EMAIL/); + }); +}); + diff --git a/packages/sql/src/modules/sql-generator.module.ts b/packages/sql/src/modules/sql-generator.module.ts index 3cb2c9a2..8c28779b 100644 --- a/packages/sql/src/modules/sql-generator.module.ts +++ b/packages/sql/src/modules/sql-generator.module.ts @@ -679,7 +679,15 @@ export class SqlGeneratorModule { if (!srcIdx) continue; // Bare index name (its schema follows the qualified table) — a qualified // index name is a syntax error in Postgres/MySQL/SQL Server. - statements.push(this.createIndexSql({ ...srcIdx, name: this.bareName(idx.name) }, tableName, dialect)); + // + // `idx.name` is compare's uppercased match key, not an identifier, so + // it would create IDX_CUSTOMERS_EMAIL where the source has + // idx_customers_email — a different index on any dialect that does not + // fold case, and a phantom rename on the next compare. The source's own + // name is the identifier; the key is only the fallback for a source + // that somehow carries none. + const indexName = this.bareName(srcIdx.name || idx.name); + statements.push(this.createIndexSql({ ...srcIdx, name: indexName }, tableName, dialect)); } // Add new / recreate modified FK constraints (after all column changes are done). diff --git a/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts new file mode 100644 index 00000000..4f75138e --- /dev/null +++ b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts @@ -0,0 +1,41 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, expect, it } from 'vitest'; +import { sqliteSqlDialect } from './sqlite.sql-dialect.js'; + +describe('sqlite CREATE INDEX', () => { + it('puts the schema on the index, never on the table', () => { + // `CREATE INDEX idx ON main.customers(email)` is `near ".": syntax error` + // in SQLite — the qualifier belongs on the index name. Found by driving a + // revert end to end; the plan looked right and the driver rejected it. + const sql = sqliteSqlDialect.createIndexStatement!( + { name: 'idx_customers_email', columns: ['email'], unique: false }, + 'main.customers' + ); + expect(sql).toBe( + 'CREATE INDEX IF NOT EXISTS main.idx_customers_email ON customers (email);' + ); + expect(sql).not.toMatch(/ON\s+main\./); + }); + + it('handles an unqualified table and a unique index', () => { + expect( + sqliteSqlDialect.createIndexStatement!( + { name: 'idx_u', columns: ['a', 'b'], unique: true }, + 'customers' + ) + ).toBe('CREATE UNIQUE INDEX IF NOT EXISTS idx_u ON customers (a, b);'); + }); + + it('carries a partial-index filter through', () => { + expect( + sqliteSqlDialect.createIndexStatement!( + { name: 'idx_active', columns: ['id'], unique: false, filter: 'active = 1' }, + 'main.t' + ) + ).toBe('CREATE INDEX IF NOT EXISTS main.idx_active ON t (id) WHERE active = 1;'); + }); +}); diff --git a/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts index 487fb2be..061f38a2 100644 --- a/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts +++ b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts @@ -1,4 +1,5 @@ import type { SqlDialect, ColumnSpec } from '../../modules/sql-dialect.interface.js'; +import type { IndexInfo } from '../../interfaces/schema.interface.js'; import { makeDialectTypeFns, plain, sized, decimalAs } from '../../modules/type-mapping.js'; const types = makeDialectTypeFns({ @@ -97,6 +98,23 @@ export const sqliteSqlDialect: SqlDialect = { return `DROP INDEX IF EXISTS ${indexName};`; }, + /** + * SQLite takes the schema on the *index* name, never on the table: + * `CREATE INDEX main.idx ON customers(email)` is valid, while + * `CREATE INDEX idx ON main.customers(email)` is `near ".": syntax error`. + * Every other dialect qualifies the table, so the shared generator does too — + * this hook moves the qualifier across. + */ + createIndexStatement(index: IndexInfo, qualifiedTable: string): string { + const dot = qualifiedTable.lastIndexOf('.'); + const schema = dot < 0 ? '' : qualifiedTable.slice(0, dot); + const table = dot < 0 ? qualifiedTable : qualifiedTable.slice(dot + 1); + const name = schema ? `${schema}.${index.name}` : index.name; + const unique = index.unique ? ' UNIQUE' : ''; + const where = index.filter?.trim() ? ` WHERE ${index.filter.trim()}` : ''; + return `CREATE${unique} INDEX IF NOT EXISTS ${name} ON ${table} (${index.columns.join(', ')})${where};`; + }, + dropTriggerStatement(triggerName: string, _qualifiedTable: string): string { return `DROP TRIGGER IF EXISTS ${triggerName};`; }, From 1187d3962653c62bcb125c75d5f3d90541f88c68 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 10:34:28 -0600 Subject: [PATCH 2/7] fix(test): drop the unused fireEvent import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Left behind when the capture test moved from clicking a button to bumping the store counter — the button it used to click now lives in HistoryCompareBar. Co-Authored-By: Claude Opus 5 --- .../src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx index 6d184870..e0a5dc34 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx @@ -9,7 +9,7 @@ */ import React from 'react'; import { describe, expect, it, vi, beforeEach } from 'vitest'; -import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, render, screen, waitFor } from '@testing-library/react'; import type { VersionGraphDTO } from './graphTypes'; const listLokeeDatabases = vi.fn(); From bcdece9751303b248e293c517612f95f0f7dd761 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 17:49:34 -0600 Subject: [PATCH 3/7] feat(history): Browse pane, revert provenance, and a no-SQL change report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Browse was a mode hiding inside Compare, reachable only from a button on one of Compare's two connection cards. It is its own pane now — Compare | Browse | History — with its own one-connection bar, type filters beside the tree they filter, and a card naming the database being read. Compare keeps its own bar untouched. A revert recorded `source: 'revert'` and nothing else, so history could say an undo happened but never which version was restored. Migration 15 adds `revert_from_version_id` / `revert_to_version_id`; the graph node now reads "↩ reverted to v1". The compare dialog gained a Markdown change report — deliberately no SQL, for the reviewer or the ticket rather than the person running the migration — and its Execute button now names its own blocker instead of showing a dead "(0)". Also: the History Compare button moved into the Target card (the pair is finished being chosen there), the minimap is themed so it stops rendering as a grey slab over a light canvas, and the deploy row's chips no longer wrap. Co-Authored-By: Claude Opus 5 --- apps/e2e/src/pages/LokeeHistoryPage.ts | 11 ++ apps/e2e/src/tests/schema-browse.test.ts | 140 ++++++++++++++++ apps/e2e/src/tests/schema-revert.test.ts | 37 +++++ apps/web/src/backend/api/routes.ts | 21 ++- apps/web/src/backend/database/schema.ts | 18 ++ .../modules/lokee-weave.module.test.ts | 55 ++++++ .../src/backend/modules/lokee-weave.module.ts | 21 ++- .../web/src/frontend/components/BrowseBar.tsx | 96 +++++++++++ .../frontend/components/ObjectDetailPanel.tsx | 37 ++++- .../frontend/components/SchemaTreePanel.tsx | 78 ++++++++- .../src/frontend/components/TopToolbar.tsx | 37 ++--- .../lokee-weave/HistoryCompareBar.tsx | 21 ++- .../components/lokee-weave/LokeeWeavePage.tsx | 25 ++- .../components/lokee-weave/LokeeWeaveView.tsx | 35 ++-- .../lokee-weave/VersionCompareModal.tsx | 116 +++++++++++-- .../components/lokee-weave/buildGraph.ts | Bin 8804 -> 8854 bytes .../components/lokee-weave/graphTypes.ts | 2 + .../frontend/components/lokee-weave/nodes.tsx | 12 ++ .../src/frontend/lib/migrationReport.test.ts | 129 +++++++++++++++ apps/web/src/frontend/lib/migrationReport.ts | 156 ++++++++++++++++++ .../src/frontend/store/lokeeHistoryStore.ts | 5 + apps/web/src/frontend/store/uiStore.ts | 13 +- apps/web/src/frontend/store/useSyncStore.ts | 7 + apps/web/src/shared/lokee-wire.ts | 11 ++ 24 files changed, 990 insertions(+), 93 deletions(-) create mode 100644 apps/e2e/src/tests/schema-browse.test.ts create mode 100644 apps/web/src/frontend/components/BrowseBar.tsx create mode 100644 apps/web/src/frontend/lib/migrationReport.test.ts create mode 100644 apps/web/src/frontend/lib/migrationReport.ts diff --git a/apps/e2e/src/pages/LokeeHistoryPage.ts b/apps/e2e/src/pages/LokeeHistoryPage.ts index 4131e32b..f5106883 100644 --- a/apps/e2e/src/pages/LokeeHistoryPage.ts +++ b/apps/e2e/src/pages/LokeeHistoryPage.ts @@ -236,6 +236,17 @@ export class LokeeHistoryPage { return parts.join(' | '); } + /** "↩ reverted to vN" labels on the version nodes, newest first. */ + async revertedToLabels(): Promise { + const marks = this.page.locator('[data-testid^="rf-version-revert-"]'); + const count = await marks.count(); + const out: string[] = []; + for (let i = 0; i < count; i++) { + out.push((await marks.nth(i).innerText().catch(() => '')) ?? ''); + } + return out; + } + async compareModalOpen(): Promise { return this.page.locator('[data-testid="lokee-version-compare"]').isVisible(); } diff --git a/apps/e2e/src/tests/schema-browse.test.ts b/apps/e2e/src/tests/schema-browse.test.ts new file mode 100644 index 00000000..029cd46a --- /dev/null +++ b/apps/e2e/src/tests/schema-browse.test.ts @@ -0,0 +1,140 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Schema Sync → Browse, against a local SQLite file. + * + * Browse used to be a mode hiding inside Compare, reachable only by pressing a + * button on one of Compare's two connection cards. It is its own pane now, so + * this covers the parts that only exist there: the search box and type filters + * on the left, and the connection card on the right naming the one database + * being read. + * + * Requires `npm run dev` and `sqlite3` on PATH. Skips when sqlite3 is missing. + */ +import { describe, it, beforeAll, afterAll, expect } from 'vitest'; +import { execFileSync, execSync } from 'node:child_process'; +import { mkdirSync, rmSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import type { Page } from 'playwright'; +import { buildDriver, quitDriver } from '../helpers/driver.js'; +import { AppPage } from '../pages/AppPage.js'; +import { SqlEditorPage } from '../pages/SqlEditorPage.js'; + +const RUN = Date.now().toString(36); +const DIR = `/tmp/foxschema-e2e-schema-browse-${RUN}`; +const DB = join(DIR, 'browse.db'); +const NAME = `E2E Browse ${RUN}`; + +function hasSqlite3(): boolean { + try { + execSync('which sqlite3', { stdio: 'ignore' }); + return true; + } catch { + return false; + } +} + +const ready = hasSqlite3(); + +describe.skipIf(!ready)('Schema Sync · Browse (SQLite)', () => { + let driver: Page; + let app: AppPage; + let sql: SqlEditorPage; + + beforeAll(async () => { + rmSync(DIR, { recursive: true, force: true }); + mkdirSync(DIR, { recursive: true }); + execFileSync('sqlite3', [DB], { + input: ` +CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT NOT NULL, email TEXT); +CREATE TABLE orders (id INTEGER PRIMARY KEY, customer_id INTEGER, total REAL); +CREATE INDEX idx_customers_email ON customers(email); +CREATE VIEW v_customers AS SELECT id, name FROM customers; +`, + }); + expect(existsSync(DB)).toBe(true); + + driver = await buildDriver(); + app = new AppPage(driver); + sql = new SqlEditorPage(driver); + + await app.open(); + await sql.resetPersistedEditorState(); + await driver.reload(); + await driver.waitForSelector('[data-testid="toolbar"]'); + await sql.addSqliteCredential(NAME, DB); + await driver.locator('[data-testid="view-sync-btn"]').click(); + await driver.waitForSelector('[data-testid="sync-pane-switcher"]'); + }, 120_000); + + afterAll(async () => { + if (driver) await quitDriver(driver); + rmSync(DIR, { recursive: true, force: true }); + }); + + it('offers Browse as its own pane beside Compare and History', async () => { + expect(await driver.locator('[data-testid="sync-pane-compare-btn"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="sync-pane-browse-btn"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="sync-pane-history-btn"]').isVisible()).toBe(true); + }); + + it('says what it wants before anything is loaded', async () => { + await driver.locator('[data-testid="sync-pane-browse-btn"]').click(); + const empty = driver.locator('[data-testid="schema-tree-empty"]'); + await empty.waitFor({ state: 'visible', timeout: 15_000 }); + // Not "No Comparison Active" — there is no comparison in this pane. + expect(await empty.innerText()).toMatch(/Nothing loaded/i); + }); + + it('reads one database from its own one-connection bar', async () => { + // Browse has no Original/Target and no swap — picking the database is the + // whole interaction, and it loads on pick. + // Browse's own bar, with no Original/Target and no swap. Compare keeps its + // two-sided grid and its own Browse buttons — untouched by this pane. + expect(await driver.locator('[data-testid="browse-bar"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="source-saved-select"]').count()).toBe(0); + + const select = driver.locator('[data-testid="browse-connection-select"]'); + const option = select.locator('option', { hasText: NAME }); + await option.waitFor({ state: 'attached', timeout: 15_000 }); + await select.selectOption((await option.getAttribute('value'))!); + + await driver.waitForSelector('[data-testid="browse-type-filter"]', { timeout: 30_000 }); + + // Left: the search box and the type filters, next to the list they filter. + expect(await driver.locator('input[placeholder*="Search objects"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="browse-type-TABLE"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="browse-type-VIEW"]').isVisible()).toBe(true); + }, 120_000); + + it('narrows the tree by type', async () => { + await driver.locator('[data-testid="browse-type-VIEW"]').click(); + await expect + .poll(async () => driver.locator('[data-testid="diff-item"]').count(), { timeout: 15_000 }) + .toBe(1); + expect(await driver.locator('[data-testid="diff-item"]').innerText()).toMatch(/v_customers/i); + + // Back to everything. + await driver.locator('[data-testid="browse-type-VIEW"]').click(); + await expect + .poll(async () => driver.locator('[data-testid="diff-item"]').count(), { timeout: 15_000 }) + .toBeGreaterThan(1); + }, 120_000); + + it('names the database being browsed on the right', async () => { + // Compare names two connections in the toolbar; Browse has one, and before + // this it was nowhere on screen. + const card = driver.locator('[data-testid="browse-connection-card"]'); + if (!(await card.isVisible().catch(() => false))) { + // A row is selected by default, so clear the selection to reach the + // empty-detail state that carries the card. + await driver.locator('input[placeholder*="Search objects"]').fill('zzz-no-such-object'); + await card.waitFor({ state: 'visible', timeout: 15_000 }); + } + const text = await card.innerText(); + expect(text).toMatch(/SQLITE/i); + expect(text, text).toMatch(/browse\.db/i); + }, 120_000); +}); diff --git a/apps/e2e/src/tests/schema-revert.test.ts b/apps/e2e/src/tests/schema-revert.test.ts index 80e105be..992a8660 100644 --- a/apps/e2e/src/tests/schema-revert.test.ts +++ b/apps/e2e/src/tests/schema-revert.test.ts @@ -166,4 +166,41 @@ INSERT INTO customers (id, name, email) VALUES (1, 'Ada', 'ada@example.com'); const rows = sqlite('SELECT count(*) FROM customers;\n').trim(); expect(rows).toBe('1'); }, 180_000); + + it('reverts forward again, and logs which version each revert restored', async () => { + // Backward was proven above (v2 → v1). Forward is the same machinery with + // the sides the other way round: pick the *newer* version as Original and + // the live database moves up to it. A revert is just a migration to a + // stored state, so "forward" and "backward" must not be different code. + await history.selectHistoryDatabaseContaining(RUN); + await history.waitForGraph(); + + // v3 (the revert) restored v1, so the index is present. Reverting to v2 — + // the version that dropped it — moves forward and drops it again. + await history.selectOriginalVersion('Version 2'); + await history.openCompareModal(); + const forward = await history.migrationSqlText(); + expect(forward, forward).toMatch(/DROP INDEX/i); + + await history.executeRevert(); + await expect + .poll(() => schemaText(), { timeout: 30_000 }) + .not.toMatch(/idx_customers_email/i); + + await driver.waitForSelector('[data-testid="lokee-version-compare"]', { + state: 'detached', + timeout: 30_000, + }); + await expect + .poll(async () => history.versionCount(), { timeout: 30_000 }) + .toBeGreaterThanOrEqual(4); + + // Both reverts are legible from the graph: each node says which version it + // put back, which is the whole point of recording the provenance. + await history.waitForGraph(); + const restored = await history.revertedToLabels(); + expect(restored.join(' '), restored.join(' ')).toMatch(/reverted to v1/i); + expect(restored.join(' '), restored.join(' ')).toMatch(/reverted to v2/i); + }, 180_000); }); + diff --git a/apps/web/src/backend/api/routes.ts b/apps/web/src/backend/api/routes.ts index 46f51962..c81139c4 100644 --- a/apps/web/src/backend/api/routes.ts +++ b/apps/web/src/backend/api/routes.ts @@ -119,7 +119,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt userId: string, resolved: { dialect: string; option: ConnectionOptions; schema: string }, source: 'manual' | 'migrate' | 'revert', - migrationRunId?: string + extra?: { migrationRunId?: string; revert?: { fromVersionId: string; toVersionId: string } } ) { const { tables } = await loadScopedTables( resolved.dialect, @@ -135,7 +135,8 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt schema: resolved.schema ?? null, tables, source, - migrationRunId, + migrationRunId: extra?.migrationRunId, + revert: extra?.revert, }); } @@ -824,7 +825,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt userId, { dialect, option, schema }, 'migrate', - runId ?? undefined + { migrationRunId: runId ?? undefined } ); send({ type: 'lokee', phase: 'before', ...before }); } catch (error: unknown) { @@ -850,7 +851,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt userId, { dialect, option, schema }, 'migrate', - runId ?? undefined + { migrationRunId: runId ?? undefined } ); send({ type: 'lokee', phase: 'after', ...after }); } catch (error: unknown) { @@ -933,7 +934,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt (req as AuthedRequest).userId!, resolved, body.source === 'migrate' || body.source === 'revert' ? body.source : 'manual', - body.migrationRunId + { migrationRunId: body.migrationRunId } ); res.json(result); } catch (error: unknown) { @@ -1178,7 +1179,15 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt } try { - const capture = await captureLiveSchema(userId, { dialect, option, schema }, 'revert'); + // Record where this undo came from and where it went, so reading the + // history later answers "reverted to which version?" rather than just + // "a revert happened". + const capture = await captureLiveSchema(userId, { dialect, option, schema }, 'revert', { + revert: { + fromVersionId: plan.fromVersion.id, + toVersionId: plan.toVersion.id, + }, + }); res.json({ ok: true, capture, ...published }); } catch (error: unknown) { const message = error instanceof Error ? error.message : 'capture failed'; diff --git a/apps/web/src/backend/database/schema.ts b/apps/web/src/backend/database/schema.ts index 502421cc..a89e404f 100644 --- a/apps/web/src/backend/database/schema.ts +++ b/apps/web/src/backend/database/schema.ts @@ -373,6 +373,24 @@ const MIGRATIONS: Migration[] = [ ]; }, }, + { + id: 15, + name: 'lokee_version_revert_provenance', + statements: (d) => { + const t = types(d); + return [ + // Where a revert came from and where it went. A version recorded by a + // revert carried `source = 'revert'` and nothing else, so history could + // say a revert happened but never which version was restored — the one + // question you ask when reading back an undo. + // + // Nullable: every version captured before this, and every non-revert + // capture, legitimately has neither. + `ALTER TABLE lokee_versions ADD COLUMN revert_from_version_id ${t.id}`, + `ALTER TABLE lokee_versions ADD COLUMN revert_to_version_id ${t.id}`, + ]; + }, + }, ]; const SIGNUP_WIZARD_SHOWN_KEY = 'signup.wizard_shown'; diff --git a/apps/web/src/backend/modules/lokee-weave.module.test.ts b/apps/web/src/backend/modules/lokee-weave.module.test.ts index 7385cf35..52fcab7c 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.test.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.test.ts @@ -554,6 +554,61 @@ describe('inspectObject', () => { }); }); +describe('revert provenance', () => { + it('records which version a revert restored, and from where', async () => { + // `source: 'revert'` says an undo happened; on its own it loses the only + // thing you want when reading one back — which version was put back. + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [CUSTOMER], source: 'manual' }); + const widened = table('customer', [ + ['id', 'integer', false], + ['email', 'varchar(255)'], + ]); + const v2 = await weave.capture(USER, { ...IDENTITY, tables: [widened], source: 'migrate' }); + + // The revert itself is a capture of the restored schema, tagged with where + // it came from and where it went. + const v3 = await weave.capture(USER, { + ...IDENTITY, + tables: [CUSTOMER], + source: 'revert', + revert: { fromVersionId: v2.versionId, toVersionId: v1.versionId }, + }); + + const versions = await weave.listVersions(USER, v3.databaseId, 10); + const recorded = versions.find((v) => v.id === v3.versionId); + expect(recorded?.source).toBe('revert'); + expect(recorded?.revertFromVersionId).toBe(v2.versionId); + expect(recorded?.revertToVersionId).toBe(v1.versionId); + + // Ordinary captures carry neither, so the fields stay a positive signal. + const plain = versions.find((v) => v.id === v2.versionId); + expect(plain?.revertFromVersionId).toBeUndefined(); + expect(plain?.revertToVersionId).toBeUndefined(); + }); + + it('resolves the restored version to its number for the graph node', async () => { + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [CUSTOMER], source: 'manual' }); + const v2 = await weave.capture(USER, { + ...IDENTITY, + tables: [table('customer', [['id', 'integer', false]])], + source: 'migrate', + }); + await weave.capture(USER, { + ...IDENTITY, + tables: [CUSTOMER], + source: 'revert', + revert: { fromVersionId: v2.versionId, toVersionId: v1.versionId }, + }); + + const dto = await weave.graph(USER, v1.databaseId, 10); + const revertNode = dto.versions.find((v) => v.number === 3); + expect(revertNode?.source).toBe('revert'); + expect(revertNode?.revertedToNumber).toBe(1); + }); +}); + describe('matchDatabaseIdentity', () => { it('accepts the same identity the history was captured under', async () => { const { weave } = await freshStore(); diff --git a/apps/web/src/backend/modules/lokee-weave.module.ts b/apps/web/src/backend/modules/lokee-weave.module.ts index d33ae542..3a6157be 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.ts @@ -110,6 +110,12 @@ export interface CaptureInput extends DatabaseIdentityInput { source: CaptureSource; /** Links the version to the run that caused it — this is the attribution. */ migrationRunId?: string; + /** + * For `source: 'revert'`: the head the database was at, and the version it was + * reverted to. Without it a revert records that *an* undo happened and loses + * the only thing you want when reading one back — which version was restored. + */ + revert?: { fromVersionId: string; toVersionId: string }; } /** @@ -135,6 +141,8 @@ interface VersionRow { last_observed_at: string; display_name?: string | null; description?: string | null; + revert_from_version_id?: string | null; + revert_to_version_id?: string | null; } interface DeltaRow { @@ -590,8 +598,8 @@ export class LokeeWeaveStore { `INSERT INTO lokee_versions (id, database_id, version_number, root_hash, parent_version_id, migration_run_id, author_user_id, source, object_count, change_count, observation_count, - created_at, last_observed_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?)`, + created_at, last_observed_at, revert_from_version_id, revert_to_version_id) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?, ?, ?)`, [ versionId, databaseId, @@ -605,6 +613,8 @@ export class LokeeWeaveStore { capture.changes.length, now, now, + input.revert?.fromVersionId ?? null, + input.revert?.toVersionId ?? null, ] ); await this.writeDelta(store, versionId, capture.changes); @@ -731,6 +741,8 @@ export class LokeeWeaveStore { description: r.description?.trim() || undefined, objectCount: Number(r.object_count) || 0, changeCount: Number(r.change_count) || 0, + revertFromVersionId: r.revert_from_version_id ?? undefined, + revertToVersionId: r.revert_to_version_id ?? undefined, })); } @@ -954,6 +966,11 @@ export class LokeeWeaveStore { author: v.author, name: v.name, description: v.description, + source: v.source, + // Resolve the id to the number the reader actually sees on the graph. + revertedToNumber: v.revertToVersionId + ? versions.find((other) => other.id === v.revertToVersionId)?.number + : undefined, })), objects, totalVersions: Number(totalRow?.n) || versions.length, diff --git a/apps/web/src/frontend/components/BrowseBar.tsx b/apps/web/src/frontend/components/BrowseBar.tsx new file mode 100644 index 00000000..78a33fde --- /dev/null +++ b/apps/web/src/frontend/components/BrowseBar.tsx @@ -0,0 +1,96 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Browse's toolbar: one database, nothing else. + * + * Browse used to borrow Compare's bar — two connection cards, a swap arrow, a + * direction label — for a pane that reads a single schema and has no direction + * at all. Half those controls were meaningless here, and the "Browse" buttons + * that started the read were buried on the Original/Target cards, which is why + * nobody found them. + * + * Picking a connection loads it. That is the whole interaction. + */ +import React from 'react'; +import { Loader2, Search } from 'lucide-react'; +import { useSyncStore } from '../store/useSyncStore'; +import { getSessionPassword } from '../lib/sessionPasswords'; + +export function BrowseBar(): React.ReactElement { + const connections = useSyncStore((s) => s.connections); + const sourceConfig = useSyncStore((s) => s.sourceConfig); + const isBrowsing = useSyncStore((s) => s.isBrowsing); + const browseMode = useSyncStore((s) => s.browseMode); + const selectedObjectTypes = useSyncStore((s) => s.selectedObjectTypes); + const applySavedConnection = useSyncStore((s) => s.applySavedConnection); + const browseSchema = useSyncStore((s) => s.browseSchema); + + const selectedId = sourceConfig.connectionId ?? ''; + + /** + * Browsing reads through the `source` side because that is where the store's + * connection config lives; the user never sees a side here. A saved + * connection with no stored password can still be browsed if this session + * already unlocked it — same rule Compare uses. + */ + const onPick = (id: string) => { + if (!id) return; + const conn = connections.find((c) => c.id === id); + if (!conn) return; + const password = conn.hasPassword ? undefined : getSessionPassword(id); + applySavedConnection('source', id, password); + void browseSchema('source'); + }; + + const label = sourceConfig.option.database + ? `${sourceConfig.dialect.toUpperCase()} · ${[sourceConfig.option.host, sourceConfig.option.database] + .filter(Boolean) + .join('/')}${sourceConfig.schema ? `.${sourceConfig.schema}` : ''}` + : null; + + return ( +
+ + Database + + + + + + {label && {label}} +
+ ); +} diff --git a/apps/web/src/frontend/components/ObjectDetailPanel.tsx b/apps/web/src/frontend/components/ObjectDetailPanel.tsx index 0c706ea7..e085c6c7 100644 --- a/apps/web/src/frontend/components/ObjectDetailPanel.tsx +++ b/apps/web/src/frontend/components/ObjectDetailPanel.tsx @@ -58,6 +58,7 @@ export const ObjectDetailPanel: React.FC = () => { targetConnected, compareResult, browseMode, + browseSide, syncSelection, toggleSyncSelection, nonDestructive, @@ -199,13 +200,47 @@ export const ObjectDetailPanel: React.FC = () => { }, [selectedTable, expandedTriggers, sourceConfig.dialect, targetConfig.dialect]); if (!selectedTable) { + // In Browse the left pane filters one database's objects, so this side + // should say *which* database that is. Comparing has two connections named + // in the toolbar already; browsing has one, and it was nowhere on screen. + const browsed = browseSide === 'target' ? targetConfig : sourceConfig; return (

Select Object to View Details

- Select an object from the left browser tree to inspect columns, indices, definitions, and generated migration DDL. + Select an object from the left browser tree to inspect columns, indices, definitions, + {browseMode ? ' and its CREATE script.' : ' and generated migration DDL.'}

+ {browseMode && ( +
+
Dialect
+
{browsed.dialect.toUpperCase()}
+ {browsed.option.host && ( + <> +
Host
+
{browsed.option.host}
+ + )} + {browsed.option.database && ( + <> +
Database
+
{browsed.option.database}
+ + )} + {browsed.schema && ( + <> +
Schema
+
{browsed.schema}
+ + )} +
Objects
+
{compareResult?.tables.length ?? 0}
+
+ )}
); } diff --git a/apps/web/src/frontend/components/SchemaTreePanel.tsx b/apps/web/src/frontend/components/SchemaTreePanel.tsx index 7fa2922a..173c6b05 100644 --- a/apps/web/src/frontend/components/SchemaTreePanel.tsx +++ b/apps/web/src/frontend/components/SchemaTreePanel.tsx @@ -1,5 +1,6 @@ import React, { useState, useEffect } from 'react'; import { useSyncStore } from '../store/useSyncStore'; +import { useUiStore } from '../store/uiStore'; import { Search, Layers, Table2, Eye, FunctionSquare, SquareTerminal, Zap, Hash, Box, Users } from 'lucide-react'; import type { TableDiff } from '../lib/types'; import { SchemaDiffTree, TYPE_META, TYPE_ORDER } from './SchemaDiffTree'; @@ -16,6 +17,7 @@ const MIN_WIDTH = 280; const MAX_WIDTH = 640; export const SchemaTreePanel: React.FC = () => { + const syncPane = useUiStore((s) => s.syncPane); const { compareResult, browseMode, @@ -29,6 +31,8 @@ export const SchemaTreePanel: React.FC = () => { searchTerm, setSearchTerm, typeFilter, + toggleTypeFilter, + clearTypeFilter, syncSelection, toggleSyncSelection, setAllSyncSelection, @@ -104,12 +108,22 @@ export const SchemaTreePanel: React.FC = () => { }, [compareResult, filteredTables, selectedTable, setSelectedTable]); if (!compareResult) { + // Browse is its own pane now, so the empty state has to name the thing the + // reader is actually looking at rather than always saying "comparison". + const browsing = syncPane === 'browse'; return ( -
+
-

No Comparison Active

+

+ {browsing ? 'Nothing loaded' : 'No Comparison Active'} +

- Connect and click "Compare Schemas" to view the difference tree — or "Browse" one side to search its objects. + {browsing + ? 'Pick a connection above and click "Browse" to read its objects.' + : 'Connect and click "Compare Schemas" to view the difference tree.'}

); @@ -223,10 +237,58 @@ export const SchemaTreePanel: React.FC = () => {
+ {/* Browse's own type filter. Compare puts these pills in the top toolbar, + which is full-width; Browse has no connection grid competing for that + bar, and the reader is filtering a single database's contents, so the + control belongs beside the list it filters. Written out here rather + than shared with the toolbar's row: same data, different container and + sizing, and one component bent to satisfy both would be worse than + twenty lines that read plainly. */} + {browseMode && ( +
+ + {TYPE_ORDER.filter((type) => + compareResult.tables.some((t) => t.objectType === type) + ).map((type) => { + const count = compareResult.tables.filter((t) => t.objectType === type).length; + const active = typeFilter.includes(type); + return ( + + ); + })} +
+ )} + {/* Deployment Selection Header — hidden in browse mode (nothing to deploy) */} {!browseMode && ( -
-
diff --git a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx index d06832b4..bae14ac0 100644 --- a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx +++ b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx @@ -7,7 +7,7 @@ * are versions of one captured database instead of two live connections. */ import React, { useMemo } from 'react'; -import { ArrowLeftRight, ArrowRight, Camera, Loader2, RefreshCw } from 'lucide-react'; +import { ArrowLeftRight, ArrowRight, Camera, GitCompareArrows, Loader2, RefreshCw } from 'lucide-react'; import { useLokeeHistoryStore } from '../../store/lokeeHistoryStore'; import { useSyncStore } from '../../store/useSyncStore'; import { SQL_ICON_STROKE } from '../sql-editor/sqlIconStyle'; @@ -34,6 +34,7 @@ export function HistoryCompareBar(): React.ReactElement { const capturing = useLokeeHistoryStore((s) => s.capturing); const requestCapture = useLokeeHistoryStore((s) => s.requestCapture); const requestRefresh = useLokeeHistoryStore((s) => s.requestRefresh); + const requestCompare = useLokeeHistoryStore((s) => s.requestCompare); const newestFirst = useMemo(() => sortVersionsNewestFirst(versions), [versions]); const resolved = useMemo( @@ -114,13 +115,14 @@ export function HistoryCompareBar(): React.ReactElement {
Current database or older version
+
+ {/* The diff belongs to the pair, and the pair is finished being chosen + here — so the button that opens it sits with the second side rather + than on a strip of its own below the bar. */} + +
{/* Capture lives on this row too. It used to sit on a second bar with its diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx index 3ee66e11..bf3858a7 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx @@ -28,6 +28,7 @@ import type { LokeeObjectType } from '@foxschema/sql'; import { OBJECT_STYLES, STATUS_STYLES, objectStyle, statusStyle } from '../../lib/lokeeColors'; import { LOKEE_NODE_TYPES } from './nodes'; import { buildVersionGraph } from './buildGraph'; +import { useUiStore } from '../../store/uiStore'; import { VersionCompareModal } from './VersionCompareModal'; import { DEFAULT_LAYOUT, @@ -193,6 +194,7 @@ export const LokeeWeavePage: React.FC = ({ // compare. The pickers choose the diff; the graph shows the history; the // checkboxes below are the only thing that filters it. const [filters, setFilters] = useState(freshFilters); + const isLight = useUiStore((s) => s.resolvedMode) === 'light'; const [locked, setLocked] = useState(true); const [selectedVersionId, setSelectedVersionId] = useState(null); const [editName, setEditName] = useState(''); @@ -628,17 +630,26 @@ export const LokeeWeavePage: React.FC = ({ n.type === 'versionNode' - ? 'var(--color-violet-400)' + ? '#a78bfa' : n.type === 'deletedObjectNode' - ? 'var(--color-rose-400)' - : 'var(--color-sky-400)' + ? '#fb7185' + : '#38bdf8' } + nodeStrokeWidth={3} /> diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx index e4728763..342a79ff 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx @@ -340,29 +340,17 @@ export function LokeeWeaveView({ - // The two sides come from HistoryCompareBar in the toolbar; this is only the - // trigger that opens the diff for them, so it sits next to the graph it - // explains rather than in the bar. - const compareBar = compareVersionIds.length === 2 && ( -
- - - Original and Target are set in the bar above. - -
- ); + // HistoryCompareBar's Target card owns the Compare button now, so this view + // only has to open the modal for the pair the bar resolved. + const compareRequest = useLokeeHistoryStore((s) => s.compareRequest); + const seenCompareRequest = useRef(compareRequest); + useEffect(() => { + if (compareRequest === seenCompareRequest.current) return; + seenCompareRequest.current = compareRequest; + if (compareVersionIds.length === 2) { + setComparePair({ original: compareVersionIds[0]!, target: compareVersionIds[1]! }); + } + }, [compareRequest, compareVersionIds]); // Every hook above any early return — a rules-of-hooks crash has happened in // this codebase before. @@ -416,7 +404,6 @@ export function LokeeWeaveView({ return (
- {compareBar} {dto.truncatedObjects && (
Showing the objects that changed in this window. This schema has more objects than the diff --git a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx index 739a96e4..74189656 100644 --- a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx +++ b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx @@ -14,7 +14,7 @@ * forty columns across three tables is three rows to scan, not forty. */ import React, { useCallback, useEffect, useMemo, useState } from 'react'; -import { Loader2, Play, X } from 'lucide-react'; +import { Download, Loader2, Play, X } from 'lucide-react'; import type { TableDiff } from '@foxschema/sql'; import { compareLokeeVersions, @@ -28,6 +28,7 @@ import { getSessionPassword } from '../../lib/sessionPasswords'; import { toast } from '../../store/toastStore'; import { riskStyle } from '../../lib/lokeeColors'; import { SchemaBlueprint } from '../SchemaBlueprint'; +import { buildMigrationReport, migrationReportFilename } from '../../lib/migrationReport'; import { SchemaDiffTree, orderTablesForDisplay } from '../SchemaDiffTree'; import { DetailTabs, type DetailTab } from '../DetailTabs'; import { buildTableDdlDiffLines, DdlDiffLines } from '../SchemaDdlDiff'; @@ -196,18 +197,52 @@ export function VersionCompareModal({ * button with no stated reason. Comparing before deciding is the whole point * of this dialog; the decision has to be reachable from wherever you are. */ - const blockedReason = useMemo(() => { - if (!captureConnectionId) return 'Choose a credential in the bar above to run this.'; - if (!plan) return planning ? 'Still planning…' : 'No plan yet.'; - if (plan.alreadyAtTarget) return 'The live schema already matches Original.'; - if (plan.statements.length === 0) return 'Nothing to apply.'; + const blocked = useMemo((): { code: string; label: string; why: string } | null => { + if (!captureConnectionId) { + return { + code: 'credential', + label: 'No credential', + why: 'Choose a credential in the bar above to run this.', + }; + } + if (!plan) { + return { + code: 'planning', + label: planning ? 'Planning…' : 'No plan', + why: planning ? 'Still planning…' : 'No plan yet.', + }; + } + // Revert always moves the *live* database to whatever sits on the Original + // side; the Target picker only chooses what the diff above is showing. So + // putting the newest version on Original asks to revert to where you + // already are, and the plan is empty. "(0)" did not say that. + if (plan.alreadyAtTarget) { + return { + code: 'already', + label: 'Already at Original', + why: 'Original is the current head — put the version you want to restore on the Original side.', + }; + } + if (plan.statements.length === 0) { + return { code: 'empty', label: 'Nothing to apply', why: 'The plan is empty.' }; + } if (plan.reversal.risk === 'blocked') { - return 'Blocked — this cannot be applied without losing data the schema cannot restore.'; + return { + code: 'blocked', + label: 'Blocked', + why: 'This cannot be applied without losing data the schema cannot restore.', + }; } - if (plan.reversal.risk === 'lossy' && !confirmLossy) return 'ACK_LOSSY'; - return ''; + if (plan.reversal.risk === 'lossy' && !confirmLossy) { + return { + code: 'lossy', + label: 'Review data loss…', + why: 'This revert destroys data — review it on Migration SQL and confirm there.', + }; + } + return null; }, [captureConnectionId, plan, planning, confirmLossy]); - const needsLossyAck = blockedReason === 'ACK_LOSSY'; + const needsLossyAck = blocked?.code === 'lossy'; const runRevert = useCallback(async () => { if (!captureConnectionId || !plan) return; @@ -253,6 +288,31 @@ export function VersionCompareModal({ return buildTableDdlDiffLines(selectedDiff, dialect, dialect, (ddl) => ddl); }, [selectedDiff, data]); + /** + * Download the change report. A separate artefact from the Migration SQL tab + * on purpose: this one is for the reviewer or the ticket, so it carries no + * DDL at all. + */ + const exportReport = useCallback(() => { + if (!data) return; + const meta = { + originalLabel: versionDisplayName({ number: data.to.number, name: data.to.name }), + targetLabel: data.from + ? versionDisplayName({ number: data.from.number, name: data.from.name }) + : 'first capture', + generatedAt: new Date(), + }; + const blob = new Blob([buildMigrationReport(data.compare, meta)], { + type: 'text/markdown;charset=utf-8', + }); + const url = URL.createObjectURL(blob); + const anchor = document.createElement('a'); + anchor.href = url; + anchor.download = migrationReportFilename(meta); + anchor.click(); + URL.revokeObjectURL(url); + }, [data]); + const heading = useMemo(() => { if (!data) return 'Compare versions'; const reference = versionDisplayName({ number: data.to.number, name: data.to.name }); @@ -288,6 +348,17 @@ export function VersionCompareModal({
{heading}
+
{changed.length === 0 ? ( @@ -379,15 +462,13 @@ export function VersionCompareModal({ type="button" data-testid="lokee-cmp-run-revert" title={ - needsLossyAck - ? 'This revert destroys data — review it on Migration SQL and confirm there' - : blockedReason || - `Apply ${plan?.statements.length ?? 0} statement(s) and record a new version` + blocked?.why ?? + `Apply ${plan?.statements.length ?? 0} statement(s) and record a new version` } // A lossy plan keeps the button live so it can carry the // reader to the acknowledgement; every other blocker is a // genuine dead end and stays disabled. - disabled={running || (Boolean(blockedReason) && !needsLossyAck)} + disabled={running || (Boolean(blocked) && !needsLossyAck)} onClick={() => { if (needsLossyAck) { setTab('SQL'); @@ -400,9 +481,8 @@ export function VersionCompareModal({ {running ? 'Applying…' - : needsLossyAck - ? 'Review data loss…' - : `Execute migration (${plan?.statements.length ?? 0})`} + : (blocked?.label ?? + `Execute migration (${plan?.statements.length ?? 0})`)}
diff --git a/apps/web/src/frontend/components/lokee-weave/buildGraph.ts b/apps/web/src/frontend/components/lokee-weave/buildGraph.ts index 2efb6fe9c9c590aa5b4ca8f87068376380b4b675..abfdb122a6b566cb8d45742f7f279c0b5afa953d 100644 GIT binary patch delta 57 xcmaFjGR<{^l7xL+#k(C0FU!0krr-v@HSw_Nx3jlO46*~X` delta 12 TcmbQ{`ov{}lEmf^2`4T9A^HSa diff --git a/apps/web/src/frontend/components/lokee-weave/graphTypes.ts b/apps/web/src/frontend/components/lokee-weave/graphTypes.ts index fad940d9..50dc583e 100644 --- a/apps/web/src/frontend/components/lokee-weave/graphTypes.ts +++ b/apps/web/src/frontend/components/lokee-weave/graphTypes.ts @@ -76,6 +76,8 @@ export type VersionNodeData = { name?: string; description?: string; changeCount: number; + /** Version this one restored, when a revert produced it. */ + revertedToNumber?: number; }; /** Prefer a custom label; otherwise "Version N". */ diff --git a/apps/web/src/frontend/components/lokee-weave/nodes.tsx b/apps/web/src/frontend/components/lokee-weave/nodes.tsx index 8fb4c0cf..3c3c2fde 100644 --- a/apps/web/src/frontend/components/lokee-weave/nodes.tsx +++ b/apps/web/src/frontend/components/lokee-weave/nodes.tsx @@ -81,6 +81,18 @@ export const VersionNode = memo(({ data: d, selected }: NodeProps )} + {/* A revert reads as an ordinary version otherwise — same node, same + counts — and the one thing you want to know is which version it put + back. */} + {d.revertedToNumber != null && ( +
+ ↩ reverted to v{d.revertedToNumber} +
+ )} {d.changeCount > 0 && (
{d.changeCount} changed diff --git a/apps/web/src/frontend/lib/migrationReport.test.ts b/apps/web/src/frontend/lib/migrationReport.test.ts new file mode 100644 index 00000000..7d640dcb --- /dev/null +++ b/apps/web/src/frontend/lib/migrationReport.test.ts @@ -0,0 +1,129 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, expect, it } from 'vitest'; +import { buildMigrationReport, migrationReportFilename } from './migrationReport'; +import type { SchemaCompareResult } from './types'; + +const META = { + originalLabel: 'Version 2', + targetLabel: 'Version 3', + databaseLabel: '[postgres] localhost/foxdb.demo_b', + generatedAt: new Date('2026-08-16T12:34:56.000Z'), +}; + +const COMPARE: SchemaCompareResult = { + summary: { added: 1, removed: 0, modified: 1, unchanged: 4 }, + tables: [ + { + tableName: 'CUSTOMERS', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [ + { name: 'id', status: 'UNCHANGED', source: { type: 'integer', nullable: false } }, + { + name: 'email', + status: 'MODIFIED', + source: { type: 'varchar(255)', nullable: false }, + target: { type: 'varchar(100)', nullable: true }, + }, + { name: 'phone', status: 'ADDED', source: { type: 'varchar(20)', nullable: true } }, + { name: 'fax', status: 'REMOVED', target: { type: 'varchar(20)', nullable: true } }, + ], + indexDiffs: [ + { + name: 'IDX_EMAIL', + status: 'ADDED', + source: { name: 'idx_email', columns: ['email'], unique: false }, + }, + ], + foreignKeyDiffs: [], + triggerDiffs: [], + }, + { + tableName: 'AUDIT_LOG', + objectType: 'TABLE', + status: 'ADDED', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + triggerDiffs: [], + }, + { + tableName: 'ORDERS', + objectType: 'TABLE', + status: 'UNCHANGED', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + triggerDiffs: [], + }, + ], +}; + +describe('buildMigrationReport', () => { + const md = buildMigrationReport(COMPARE, META); + + it('never contains SQL — that is the whole point of this report', () => { + // The Migration SQL tab answers "what will run"; this answers "what + // changed", for a reader who does not read DDL. + // Keywords only. Banning `;` as well was too crude — English prose uses + // punctuation, and the first version of this failed on its own sentence. + expect(md).not.toMatch(/\b(ALTER|CREATE|DROP|SELECT|INSERT|UPDATE|DELETE)\b/i); + }); + + it('names both sides, the database and the time', () => { + expect(md).toContain('Version 2 → Version 3'); + expect(md).toContain('[postgres] localhost/foxdb.demo_b'); + expect(md).toContain('2026-08-16 12:34 UTC'); + }); + + it('summarises counts as a table', () => { + expect(md).toContain('| Added | 1 |'); + expect(md).toContain('| Changed | 1 |'); + expect(md).toContain('| Unchanged | 4 |'); + }); + + it('lists only the objects that differ', () => { + expect(md).toContain('`CUSTOMERS`'); + expect(md).toContain('`AUDIT_LOG`'); + expect(md).not.toContain('`ORDERS`'); + }); + + it('describes column changes in words, with the direction of the change', () => { + expect(md).toContain('Added column `phone` (varchar(20))'); + expect(md).toContain('Removed column `fax`'); + // Old → new, and the nullability flip stated plainly. + expect(md).toContain('varchar(100) → varchar(255)'); + expect(md).toContain('now required'); + }); + + it('uses the index own name rather than the uppercased compare key', () => { + expect(md).toContain('`idx_email`'); + expect(md).not.toContain('`IDX_EMAIL`'); + }); + + it('says so plainly when an object changed but has nothing to list', () => { + expect(md).toMatch(/No column, index or constraint changes were recorded/i); + }); + + it('reports an identical pair without inventing sections', () => { + const same = buildMigrationReport( + { summary: { added: 0, removed: 0, modified: 0, unchanged: 9 }, tables: [] }, + META + ); + expect(same).toContain('No objects differ'); + expect(same).not.toContain('## Details'); + }); +}); + +describe('migrationReportFilename', () => { + it('slugs both sides into a safe name', () => { + expect(migrationReportFilename(META)).toBe('schema-report-version-2-to-version-3.md'); + expect( + migrationReportFilename({ ...META, targetLabel: 'Current database (Version 4)' }) + ).toBe('schema-report-version-2-to-current-database-version-4.md'); + }); +}); diff --git a/apps/web/src/frontend/lib/migrationReport.ts b/apps/web/src/frontend/lib/migrationReport.ts new file mode 100644 index 00000000..c329e665 --- /dev/null +++ b/apps/web/src/frontend/lib/migrationReport.ts @@ -0,0 +1,156 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * A schema change report in Markdown, for people who do not read DDL. + * + * The Migration SQL tab already answers "what will run". This answers "what + * changed", for the reviewer, the ticket, or the change-approval board — so it + * deliberately contains **no SQL at all**. A column widening reads "email: + * varchar(100) → varchar(255)", not an ALTER statement. + * + * Pure and string-only: no DOM, no download, no store. The caller decides what + * to do with the text, and the shape of the output can be asserted directly. + */ +import type { SchemaCompareResult, TableDiff } from './types'; + +export interface MigrationReportMeta { + /** e.g. "Version 2" — the reference side. */ + originalLabel: string; + /** e.g. "Version 3" or "Current database". */ + targetLabel: string; + /** Database the history belongs to, as shown in the picker. */ + databaseLabel?: string; + /** Defaults to now; injected so tests are not clock-dependent. */ + generatedAt?: Date; +} + +const STATUS_WORD: Record = { + ADDED: 'Added', + REMOVED: 'Removed', + MODIFIED: 'Changed', + UNCHANGED: 'Unchanged', +}; + +/** `varchar(100) → varchar(255)`, or one side when the column exists once. */ +function typeChange(from?: string, to?: string): string { + if (from && to && from !== to) return `${from} → ${to}`; + return to ?? from ?? ''; +} + +/** Plain-language lines for what moved inside one object. */ +function objectDetails(diff: TableDiff): string[] { + const lines: string[] = []; + + for (const column of diff.columnDiffs) { + if (column.status === 'UNCHANGED') continue; + if (column.status === 'ADDED') { + const type = column.source?.type ?? ''; + lines.push(`- Added column \`${column.name}\`${type ? ` (${type})` : ''}`); + } else if (column.status === 'REMOVED') { + lines.push(`- Removed column \`${column.name}\``); + } else { + const change = typeChange(column.target?.type, column.source?.type); + const bits: string[] = []; + if (change.includes('→')) bits.push(change); + if (column.source && column.target && column.source.nullable !== column.target.nullable) { + bits.push(column.source.nullable ? 'now nullable' : 'now required'); + } + lines.push( + `- Changed column \`${column.name}\`${bits.length ? `: ${bits.join(', ')}` : ''}` + ); + } + } + + for (const index of diff.indexDiffs) { + if (index.status === 'UNCHANGED') continue; + const columns = (index.source ?? index.target)?.columns?.join(', ') ?? ''; + const name = index.source?.name ?? index.target?.name ?? index.name; + lines.push( + `- ${STATUS_WORD[index.status] ?? index.status} index \`${name}\`${columns ? ` on (${columns})` : ''}` + ); + } + + for (const fk of diff.foreignKeyDiffs) { + if (fk.status === 'UNCHANGED') continue; + const info = fk.source ?? fk.target; + const to = info?.referencedTable ? ` → \`${info.referencedTable}\`` : ''; + lines.push(`- ${STATUS_WORD[fk.status] ?? fk.status} foreign key \`${fk.name}\`${to}`); + } + + for (const trigger of diff.triggerDiffs ?? []) { + if (trigger.status === 'UNCHANGED') continue; + const name = trigger.source?.name ?? trigger.target?.name ?? trigger.name; + lines.push(`- ${STATUS_WORD[trigger.status] ?? trigger.status} trigger \`${name}\``); + } + + return lines; +} + +/** `# Schema change report` … in Markdown. Never contains DDL. */ +export function buildMigrationReport( + compare: SchemaCompareResult, + meta: MigrationReportMeta +): string { + const when = (meta.generatedAt ?? new Date()).toISOString().slice(0, 16).replace('T', ' '); + const changed = compare.tables.filter((t) => t.status !== 'UNCHANGED'); + + const out: string[] = [ + '# Schema change report', + '', + `**Comparing:** ${meta.originalLabel} → ${meta.targetLabel}`, + ]; + if (meta.databaseLabel) out.push(`**Database:** ${meta.databaseLabel}`); + out.push(`**Generated:** ${when} UTC`, ''); + + out.push( + '## Summary', + '', + '| Change | Objects |', + '| --- | ---: |', + `| Added | ${compare.summary.added} |`, + `| Changed | ${compare.summary.modified} |`, + `| Removed | ${compare.summary.removed} |`, + `| Unchanged | ${compare.summary.unchanged} |`, + '' + ); + + if (changed.length === 0) { + out.push('No objects differ between these two versions.', ''); + return out.join('\n'); + } + + out.push('## Objects changed', '', '| Object | Type | Change |', '| --- | --- | --- |'); + for (const table of changed) { + out.push( + `| \`${table.tableName}\` | ${table.objectType} | ${STATUS_WORD[table.status] ?? table.status} |` + ); + } + out.push(''); + + out.push('## Details', ''); + for (const table of changed) { + out.push(`### ${table.tableName}`, '', `${STATUS_WORD[table.status] ?? table.status} ${table.objectType.toLowerCase()}.`, ''); + const details = objectDetails(table); + if (details.length > 0) { + out.push(...details, ''); + } else { + // A view or routine whose body changed has no child diffs to list, and + // saying so beats an empty heading that looks like a rendering bug. + out.push('_No column, index or constraint changes were recorded for this object._', ''); + } + } + + return out.join('\n'); +} + +/** Filename-safe slug for the downloaded file. */ +export function migrationReportFilename(meta: MigrationReportMeta): string { + const slug = (text: string) => + text + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-|-$/g, ''); + return `schema-report-${slug(meta.originalLabel)}-to-${slug(meta.targetLabel)}.md`; +} diff --git a/apps/web/src/frontend/store/lokeeHistoryStore.ts b/apps/web/src/frontend/store/lokeeHistoryStore.ts index b0dc750b..c1767a02 100644 --- a/apps/web/src/frontend/store/lokeeHistoryStore.ts +++ b/apps/web/src/frontend/store/lokeeHistoryStore.ts @@ -40,6 +40,9 @@ interface LokeeHistoryState { requestCapture: () => void; refreshRequest: number; requestRefresh: () => void; + /** Bumped by the Target card's Compare button; the graph view opens the modal. */ + compareRequest: number; + requestCompare: () => void; } export const useLokeeHistoryStore = create((set, get) => ({ @@ -64,6 +67,8 @@ export const useLokeeHistoryStore = create((set, get) => ({ requestCapture: () => set({ captureRequest: get().captureRequest + 1 }), refreshRequest: 0, requestRefresh: () => set({ refreshRequest: get().refreshRequest + 1 }), + compareRequest: 0, + requestCompare: () => set({ compareRequest: get().compareRequest + 1 }), swapSides: () => { const next = swapHistoryCompare(get().versions, { originalVersionId: get().originalVersionId, diff --git a/apps/web/src/frontend/store/uiStore.ts b/apps/web/src/frontend/store/uiStore.ts index 079babfb..27793989 100644 --- a/apps/web/src/frontend/store/uiStore.ts +++ b/apps/web/src/frontend/store/uiStore.ts @@ -178,7 +178,14 @@ function applyToDocument(themeMode: ThemeMode, tone: ToneId, fontSize: FontSize, /** Top-level workspace views: schema sync (compare + history) vs the SQL Editor. */ export type ActiveView = 'sync' | 'sqlEditor'; /** Compare tree vs Lokee schema-history graph, both inside Schema Sync. */ -export type SyncPane = 'compare' | 'history'; +/** + * Browse is its own pane, not a mode hiding inside Compare. It answers a + * different question — "what is in this one database?" rather than "how do + * these two differ?" — and reaching it by pressing a button on one of Compare's + * two connection cards left the app showing a comparison workspace with no + * comparison in it. + */ +export type SyncPane = 'compare' | 'browse' | 'history'; interface UiState { themeMode: ThemeMode; @@ -189,7 +196,7 @@ interface UiState { resolvedMode: 'dark' | 'light'; /** Which workspace view is showing (persisted; purely local, not synced to server prefs). */ activeView: ActiveView; - /** Compare vs schema history, only meaningful when `activeView === 'sync'`. */ + /** Compare / browse / history, only meaningful when `activeView === 'sync'`. */ syncPane: SyncPane; /** Bumped after a Lokee capture so the history graph reloads. */ lokeeEpoch: number; @@ -230,7 +237,7 @@ function migrateUiPersist(persisted: unknown, _version: number): unknown { state.activeView = 'sync'; state.syncPane = 'history'; } - if (state.syncPane !== 'history' && state.syncPane !== 'compare') { + if (!['history', 'compare', 'browse'].includes(state.syncPane as string)) { state.syncPane = 'compare'; } if (typeof state.lokeeEpoch !== 'number') state.lokeeEpoch = 0; diff --git a/apps/web/src/frontend/store/useSyncStore.ts b/apps/web/src/frontend/store/useSyncStore.ts index 7d77694a..833e8c76 100644 --- a/apps/web/src/frontend/store/useSyncStore.ts +++ b/apps/web/src/frontend/store/useSyncStore.ts @@ -461,12 +461,19 @@ export const useSyncStore = create()( selectedTable: result.tables[0] || null, isBrowsing: false, }); + // Browse is a pane, so reading a schema takes you there. Before this the + // result landed in the Compare workspace, which then showed a comparison + // layout holding one database and no comparison. + useUiStore.getState().setSyncPane('browse'); } catch (e: any) { set({ errorMsg: e.message || `Failed to load ${side} schema`, isBrowsing: false }); } }, runSchemaComparison: async () => { + // The mirror of browseSchema: comparing leaves Browse behind, so the pane + // and the state it renders can never disagree. + useUiStore.getState().setSyncPane('compare'); set({ isComparing: true, errorMsg: null, warnings: [], compareResult: null, selectedTable: null, generatedSql: null, migrationExecuted: false, browseMode: false, browseSide: null }); try { // Re-read the available schemas so the comparison runs against current server state diff --git a/apps/web/src/shared/lokee-wire.ts b/apps/web/src/shared/lokee-wire.ts index cf9148cc..4596900b 100644 --- a/apps/web/src/shared/lokee-wire.ts +++ b/apps/web/src/shared/lokee-wire.ts @@ -88,6 +88,13 @@ export interface VersionSummary { description?: string; objectCount: number; changeCount: number; + /** + * Set only on a version a revert produced: the head the database was at, and + * the version that was restored. `source: 'revert'` says an undo happened; + * these say which one, which is the question you ask when reading it back. + */ + revertFromVersionId?: string; + revertToVersionId?: string; } export interface ObjectHistoryEntry { @@ -160,6 +167,10 @@ export interface VersionGraphVersion { /** Optional display name; falls back to `Version ${number}`. */ name?: string; description?: string; + /** How the version came to exist — a revert node is worth marking. */ + source?: CaptureSource; + /** Version number this revert restored, when this version is one. */ + revertedToNumber?: number; } export interface VersionGraphObject { From 19637b30e0953c502d5c4f4b0c8f2b7be13c360e Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 20:54:23 -0600 Subject: [PATCH 4/7] fix(lokee): scope a revert to the objects actually ticked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a revert could rewrite a database nobody asked it to touch. Executing with **zero** objects ticked sent `objectKeys: undefined`, which the backend reads as "the whole schema" — one click reverted an entire database from a dialog where nothing was selected. An empty tick set is now refused, and `objectKeys` is always sent explicitly. Since that made a whole-schema revert unreachable by accident, Select all / Clear plus an `N of M ticked` counter keep the destructive path available but deliberate. Worse, `planRevert` filtered the *risk verdicts* by the ticked keys while handing the **unfiltered** state maps to the compare that generates the SQL. The dialog said "1 object" and the migration rewrote every table. Both maps are narrowed now. Ticking a lone child also carries its `table:` container along as context — `hydrateTableSchemas` drops any group without one, so the plan came back empty — but only when that container exists on both sides, so it can never turn a one-column tick into a DROP TABLE. The existing tests asserted only on `reversal.verdicts`, which is exactly how the statement generator drifted unnoticed; the new ones assert on `statements`. Also fixes a stale `blocked` memo (missing `selectedKeys`/`changed` deps left the button saying "Tick objects to revert" after you had ticked one), and hides the Compare button rather than disabling it when both sides resolve to one version. Co-Authored-By: Claude Opus 5 --- apps/e2e/src/pages/LokeeHistoryPage.ts | 28 +++++- .../tests/schema-version-revert-edges.test.ts | 47 +++++++++- .../modules/lokee-weave.module.test.ts | 91 +++++++++++++++++++ .../src/backend/modules/lokee-weave.module.ts | 41 ++++++++- .../lokee-weave/HistoryCompareBar.tsx | 10 +- .../lokee-weave/VersionCompareModal.tsx | 44 ++++++++- 6 files changed, 249 insertions(+), 12 deletions(-) diff --git a/apps/e2e/src/pages/LokeeHistoryPage.ts b/apps/e2e/src/pages/LokeeHistoryPage.ts index 7a022357..b3d006b8 100644 --- a/apps/e2e/src/pages/LokeeHistoryPage.ts +++ b/apps/e2e/src/pages/LokeeHistoryPage.ts @@ -54,7 +54,10 @@ export class LokeeHistoryPage { await this.page.waitForFunction( (expected) => { const el = document.querySelector('[data-testid="lokee-summary"]'); - const text = el?.textContent ?? ''; + // innerText, not textContent: the summary is a row of sibling spans, so + // textContent runs them together as "…2 versions10 objects…" and the + // trailing \b never matches — `s` meets `1`, both word characters. + const text = (el as HTMLElement | null)?.innerText ?? ''; return new RegExp(`\\b${expected}\\s+versions?\\b`, 'i').test(text); }, n, @@ -309,9 +312,30 @@ export class LokeeHistoryPage { await box.click(); } + /** + * Tick every changed object. Required before Execute: an empty tick set is + * refused rather than silently widening to the whole schema, which is what + * used to revert an entire database from a dialog with nothing selected. + */ + async selectAllCompareObjects(): Promise { + await clickWhen(this.page, '[data-testid="lokee-cmp-select-all"]'); + } + async compareObjectNames(): Promise { const items = this.page.locator('[data-testid="diff-item"]'); - await items.first().waitFor({ state: 'visible', timeout: 20_000 }); + try { + await items.first().waitFor({ state: 'visible', timeout: 20_000 }); + } catch (error) { + // "no diff rows" and "the dialog says the versions are identical" look + // the same from a timeout, and they have opposite causes. + const open = await this.compareModalOpen(); + const identical = await this.compareIdenticalVisible().catch(() => false); + const summary = await this.compareSummaryText().catch(() => ''); + throw new Error( + `No objects in the compare tree. modalOpen=${open} identical=${identical} summary=${summary}`, + { cause: error } + ); + } const count = await items.count(); const names: string[] = []; for (let i = 0; i < count; i++) { diff --git a/apps/e2e/src/tests/schema-version-revert-edges.test.ts b/apps/e2e/src/tests/schema-version-revert-edges.test.ts index c57a9fd0..39f76160 100644 --- a/apps/e2e/src/tests/schema-version-revert-edges.test.ts +++ b/apps/e2e/src/tests/schema-version-revert-edges.test.ts @@ -71,7 +71,10 @@ async function boot( // ── 1. Versioning: capture, pickers, preview ──────────────────────────────── describe.skipIf(!ready)('History · versioning edge cases (SQLite)', () => { - const RUN = Date.now().toString(36); + // Suffixed per suite: all three describes evaluate in the same + // millisecond, so a bare timestamp is not unique and the history + // picker matched more than one database. + const RUN = `${Date.now().toString(36)}-edges`; const DIR = `/tmp/foxschema-e2e-version-edges-${RUN}`; const DB = join(DIR, 'versions.db'); const NAME = `E2E Versions ${RUN}`; @@ -197,7 +200,10 @@ INSERT INTO invoices (id, total) VALUES (1, 10); // ── 2. Revert: no-op ticks, scoped object, new version ─────────────────────── describe.skipIf(!ready)('History · revert scope edge cases (SQLite)', () => { - const RUN = Date.now().toString(36); + // Suffixed per suite: all three describes evaluate in the same + // millisecond, so a bare timestamp is not unique and the history + // picker matched more than one database. + const RUN = `${Date.now().toString(36)}-scope`; const DIR = `/tmp/foxschema-e2e-revert-scope-${RUN}`; const DB = join(DIR, 'scope.db'); const NAME = `E2E Revert Scope ${RUN}`; @@ -231,7 +237,19 @@ INSERT INTO invoices (id, total) VALUES (1, 10); await history.openHistoryPane(); await history.selectHistoryDatabaseContaining(RUN); await history.waitForGraph(); - await expect.poll(async () => history.versionCount(), { timeout: 30_000 }).toBe(2); + // `expect.poll` is only legal inside a test — this is a beforeAll hook, so + // wait on the page instead. The page object already owns that wait. + try { + await history.waitForVersionCount(2); + } catch (error) { + // A bare timeout here says "setup failed" and nothing else; the summary + // line names the database actually on screen, which is the difference + // between a missed capture and the wrong database being selected. + throw new Error( + `Setup never reached 2 versions.\nSummary: ${await history.summaryText()}`, + { cause: error } + ); + } }, 180_000); afterAll(async () => { @@ -305,7 +323,10 @@ INSERT INTO invoices (id, total) VALUES (1, 10); // ── 3. Revert: lossy ack, drop-table, pendulum ─────────────────────────────── describe.skipIf(!ready)('History · revert lossy and pendulum (SQLite)', () => { - const RUN = Date.now().toString(36); + // Suffixed per suite: all three describes evaluate in the same + // millisecond, so a bare timestamp is not unique and the history + // picker matched more than one database. + const RUN = `${Date.now().toString(36)}-lossy`; const DIR = `/tmp/foxschema-e2e-revert-lossy-${RUN}`; const DB = join(DIR, 'lossy.db'); const NAME = `E2E Revert Lossy ${RUN}`; @@ -347,7 +368,19 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.openHistoryPane(); await history.selectHistoryDatabaseContaining(RUN); await history.waitForGraph(); - await expect.poll(async () => history.versionCount(), { timeout: 30_000 }).toBe(2); + // `expect.poll` is only legal inside a test — this is a beforeAll hook, so + // wait on the page instead. The page object already owns that wait. + try { + await history.waitForVersionCount(2); + } catch (error) { + // A bare timeout here says "setup failed" and nothing else; the summary + // line names the database actually on screen, which is the difference + // between a missed capture and the wrong database being selected. + throw new Error( + `Setup never reached 2 versions.\nSummary: ${await history.summaryText()}`, + { cause: error } + ); + } }, 180_000); afterAll(async () => { @@ -359,6 +392,8 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.selectOriginalVersion('Version 1'); await history.selectTargetCurrent(); await history.openCompareModal(); + // Zero ticks is refused by design, so say "all of it" explicitly. + await history.selectAllCompareObjects(); const run = driver.locator('[data-testid="lokee-cmp-run-revert"]'); await run.waitFor({ state: 'visible', timeout: 20_000 }); @@ -387,6 +422,7 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.selectOriginalVersion('Version 1'); await history.selectTargetCurrent(); await history.openCompareModal(); + await history.selectAllCompareObjects(); const calls: string[] = []; driver.on('response', (res) => { @@ -424,6 +460,7 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.selectOriginalVersion('Version 2'); await history.selectTargetCurrent(); await history.openCompareModal(); + await history.selectAllCompareObjects(); await history.executeRevert(); try { diff --git a/apps/web/src/backend/modules/lokee-weave.module.test.ts b/apps/web/src/backend/modules/lokee-weave.module.test.ts index 52fcab7c..b1e20b2b 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.test.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.test.ts @@ -933,6 +933,46 @@ describe('selective revert', () => { expect(plan!.reversal.risk).toBe('lossy'); }); + it('generates DDL for the selection only, not for the whole schema', async () => { + // The verdicts above are the *classification*; these are what actually runs. + // They were filtered independently, and only the verdicts were narrowed — so + // the dialog said "1 object" while the migration rewrote every table. Assert + // on the statements, or the next divergence goes unnoticed again. + const { weave, databaseId, v1 } = await twoChanges(); + const plan = await weave.planRevert(USER, databaseId, v1.versionId, 'postgres', 'public', [ + 'table:ORDERS', + ]); + const sql = plan!.statements.join('\n'); + expect(sql).toMatch(/orders/i); + // v1's customer.email is varchar(100) and HEAD's is varchar(255): a + // whole-schema revert would narrow it here. The user did not tick it. + expect(sql).not.toMatch(/customer/i); + }); + + it('reverts the ticked column without touching the sibling table', async () => { + const { weave, databaseId, v1 } = await twoChanges(); + const plan = await weave.planRevert(USER, databaseId, v1.versionId, 'postgres', 'public', [ + 'column:CUSTOMER.EMAIL', + ]); + const sql = plan!.statements.join('\n'); + expect(sql).toMatch(/customer/i); + expect(sql).toMatch(/varchar\(100\)/i); + // ORDERS arrived in v2 and a whole-schema revert would drop it. + expect(sql).not.toMatch(/\borders\b/i); + }); + + it('will not drop a whole table because one of its columns was ticked', async () => { + // ORDERS arrived in v2, so reverting to v1 means it should not exist at all. + // Pulling its container in to satisfy hydration must not turn a one-column + // tick into a DROP TABLE — widening past the tick is the bug this whole + // group exists to prevent. Tick the table itself for that. + const { weave, databaseId, v1 } = await twoChanges(); + const plan = await weave.planRevert(USER, databaseId, v1.versionId, 'postgres', 'public', [ + 'column:ORDERS.ID', + ]); + expect(plan!.statements.join('\n')).not.toMatch(/drop table/i); + }); + it('treats an empty selection as nothing, not as everything', async () => { // A UI with no boxes ticked sends `[]`. If that were read as "no filter", // clicking Revert with nothing selected would rewrite the whole database. @@ -948,6 +988,57 @@ describe('selective revert', () => { }); }); +describe('awkward names survive a capture', () => { + // Object keys are `kind:OWNER.CHILD`, so a name holding a dot is the one that + // can corrupt the addressing scheme the whole history is built on. + const AWKWARD = table('Order Details', [ + ['order id', 'integer', false], + ['a.b', 'text'], + ]); + + it('reconstructs a table whose name and columns contain spaces', async () => { + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [AWKWARD], source: 'manual' }); + const at = await weave.objectsAtVersion(USER, v1.databaseId, v1.versionId); + + // The container is addressable and keeps its real name for DDL. + const container = at.get('table:ORDER DETAILS'); + expect(container?.body.name).toBe('Order Details'); + + const spaced = at.get('column:ORDER DETAILS.ORDER ID'); + expect(spaced?.body.name).toBe('order id'); + }); + + it('does not lose a column whose name contains a dot', async () => { + // `a.b` makes the key `column:ORDER DETAILS.A.B`, which owner-parsing splits + // at the first dot. The column must still round-trip to its own name — if + // this ever regresses, the blueprint and every revert plan lose the column. + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [AWKWARD], source: 'manual' }); + const at = await weave.objectsAtVersion(USER, v1.databaseId, v1.versionId); + + const dotted = [...at.values()].filter((o) => o.body.name === 'a.b'); + expect(dotted).toHaveLength(1); + }); + + it('plans a revert for an awkward table without inventing changes', async () => { + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [AWKWARD], source: 'manual' }); + const v2 = await weave.capture(USER, { + ...IDENTITY, + tables: [table('Order Details', [['order id', 'integer', false]])], + source: 'migrate', + }); + expect(v2.changed).toBe(true); + + const plan = await weave.planRevert(USER, v2.databaseId, v1.versionId, 'sqlite'); + // Reverting restores the dropped column, and the DDL must carry the real + // name — quoted, since it cannot be written bare. + const sql = plan!.statements.join('\n'); + expect(sql).toMatch(/"a\.b"|`a\.b`|\[a\.b]/); + }); +}); + describe('version compare (shared Compare engine)', () => { it('reads forward: what the newer version added', async () => { // Direction is the trap. CompareModule answers "what must TARGET change to diff --git a/apps/web/src/backend/modules/lokee-weave.module.ts b/apps/web/src/backend/modules/lokee-weave.module.ts index 3a6157be..3398f402 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.ts @@ -1184,6 +1184,40 @@ export class LokeeWeaveStore { return owner ? selectedOwners!.has(owner) : false; }; + /** + * Both sides narrowed to the ticked objects, so the generated DDL touches + * only them. + * + * Filtering `entries` alone classified the *risk* of the selection while + * the statements below still reverted everything: the dialog said "1 object" + * and the migration rewrote every table in the schema. + * + * Ticking a lone child needs its `table:` container carried along as + * context, because `hydrateTableSchemas` drops any group without one — a + * column with no table is not a schema, and the plan came back empty. + * That container is context only: it is added just when it exists on *both* + * sides, so it contributes the table's shape and never a CREATE/DROP of a + * table nobody ticked. A child whose container exists on one side only + * therefore reverts nothing on its own; tick the table for that. + */ + const contextOwners = selected + ? new Set( + [...new Set([...current.keys(), ...desired.keys()])] + .filter((key) => wanted(key)) + .map((key) => objectKeyOwner(key)) + ) + : null; + const isContainerFor = (key: string, owners: ReadonlySet): boolean => + !key.includes('.') && owners.has(objectKeyOwner(key)) && current.has(key) && desired.has(key); + const inSelection = ( + objects: ReadonlyMap + ): ReadonlyMap => + contextOwners + ? new Map( + [...objects].filter(([key]) => wanted(key) || isContainerFor(key, contextOwners)) + ) + : objects; + const entries: Array<{ key: string; current?: CanonicalObject; target?: CanonicalObject }> = []; for (const key of new Set([...current.keys(), ...desired.keys()])) { if (!wanted(key)) continue; @@ -1214,7 +1248,12 @@ export class LokeeWeaveStore { // then the same SQL generator the live migrate flow uses. const migration = dialectName ? migrationFromCompare( - await this.compareVersionStates(desired, current, dialectName, schemaName), + await this.compareVersionStates( + inSelection(desired), + inSelection(current), + dialectName, + schemaName + ), dialectName, { targetSchema: schemaName, sourceSchema: schemaName } ) diff --git a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx index bae14ac0..79bafbaf 100644 --- a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx +++ b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx @@ -137,18 +137,24 @@ export function HistoryCompareBar(): React.ReactElement { {/* The diff belongs to the pair, and the pair is finished being chosen here — so the button that opens it sits with the second side rather - than on a strip of its own below the bar. */} + than on a strip of its own below the bar. + + Absent rather than disabled when the two sides resolve to the same + version: with a single-version history there is no pair and never + will be until another capture lands, so a permanently dead control + is just clutter. */} + {!sameSides && newestFirst.length >= 2 && ( + )}
diff --git a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx index 74189656..54d398c4 100644 --- a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx +++ b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx @@ -226,6 +226,16 @@ export function VersionCompareModal({ if (plan.statements.length === 0) { return { code: 'empty', label: 'Nothing to apply', why: 'The plan is empty.' }; } + // An empty tick set used to send `undefined`, which the backend reads as + // "the whole schema" — so pressing Execute with nothing selected reverted + // the entire database. Selecting nothing must mean nothing. + if (changed.length > 0 && selectedKeys.length === 0) { + return { + code: 'nothing-ticked', + label: 'Tick objects to revert', + why: 'Tick the objects to revert in the tree, or use Select all.', + }; + } if (plan.reversal.risk === 'blocked') { return { code: 'blocked', @@ -241,7 +251,10 @@ export function VersionCompareModal({ }; } return null; - }, [captureConnectionId, plan, planning, confirmLossy]); + // selectedKeys and changed belong here: without them, ticking an object + // left this memo stale and the button kept saying "Tick objects to revert" + // after the user had ticked one. + }, [captureConnectionId, plan, planning, confirmLossy, selectedKeys, changed]); const needsLossyAck = blocked?.code === 'lossy'; const runRevert = useCallback(async () => { @@ -253,7 +266,9 @@ export function VersionCompareModal({ connectionId: captureConnectionId, password: getSessionPassword(captureConnectionId), confirmLossy, - objectKeys: selectedKeys.length > 0 ? selectedKeys : undefined, + // Always explicit: the guard above refuses to run with an empty tick + // set, so this never silently widens to the whole schema. + objectKeys: selectedKeys, }); toast({ tone: 'success', @@ -423,6 +438,31 @@ export function VersionCompareModal({ // gesture, one component.
+
+ + + + {selectedKeys.length} of {changed.length} ticked + +
Date: Sun, 16 Aug 2026 20:54:40 -0600 Subject: [PATCH 5/7] fix(sql): emit DDL that parses, for names and types a real catalog contains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by two new harnesses rather than by hand: a seeded generator that builds adversarial schemas and checks properties across all 14 dialects at once, and a test that hands the generated DDL to a real `node:sqlite` engine and lets it judge. String assertions only ever prove the output matches what somebody expected it to be. * **Identifiers were never quoted.** A table called `Order Details` — Northwind ships one — produced unparseable SQL on every dialect, as did a column called `order id` or `select`. Adds an optional `quoteIdentifier` dialect hook (ANSI default; backticks for MySQL/MariaDB/TiDB, brackets for SQL Server/Azure), applied only when a name cannot be written bare, so every ordinary name is byte-identical to what this generator emitted before. `ident` is idempotent, which lets ADD COLUMN and CREATE INDEX be fixed for all 14 at one call site instead of in fourteen hooks. * **`ColumnDiff.name` is the uppercased compare key**, the same trap as `tableName`, and the ALTER paths emitted it — renaming a user's `new col` to `NEW COL`. Now uses `source?.name ?? target?.name`; the field is optional because this package is published. * **`ALTER TABLE … ADD CONSTRAINT` was emitted for SQLite and ClickHouse**, which reject it outright. `dialectSupportsFk` already knew this and only the blueprint UI was reading it. FKs now inline into CREATE TABLE where the dialect allows it, and otherwise emit `-- review:` — never DDL that cannot run. * **Decimal precision was dropped.** `NUMBER(10)` / `DECIMAL(10)` tokenize their single argument as a *length*, which `shapeCanonical` ignored for decimals, so an Oracle `NUMBER(10)` column silently widened to full 38-digit precision on every migration. Fixed in the shared shaper, not per dialect. * **Redshift rendered `varchar(max)`** for TEXT and XML. That is T-SQL syntax Redshift rejects; its documented maximum is `varchar(65535)`. Co-Authored-By: Claude Opus 5 --- .../modules/generated-ddl-runs.test.ts | 217 +++++++++ .../src/interfaces/diff.types.interface.ts | 13 +- packages/sql/src/modules/schema-fuzz.test.ts | 410 ++++++++++++++++++ .../sql/src/modules/sql-dialect.interface.ts | 11 + .../sql/src/modules/sql-generator.module.ts | 178 +++++++- packages/sql/src/modules/type-mapping.ts | 9 +- .../src/providers/mysql/mysql.sql-dialect.ts | 6 + .../redshift/redshift.sql-dialect.ts | 6 +- .../sqlServer/sqlserver.sql-dialect.ts | 6 + 9 files changed, 836 insertions(+), 20 deletions(-) create mode 100644 apps/web/src/backend/modules/generated-ddl-runs.test.ts create mode 100644 packages/sql/src/modules/schema-fuzz.test.ts diff --git a/apps/web/src/backend/modules/generated-ddl-runs.test.ts b/apps/web/src/backend/modules/generated-ddl-runs.test.ts new file mode 100644 index 00000000..f8ccfdb3 --- /dev/null +++ b/apps/web/src/backend/modules/generated-ddl-runs.test.ts @@ -0,0 +1,217 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Generated DDL, executed by a real SQL engine. + * + * Every other generator test asserts on strings, which only ever proves the + * output matches what somebody expected it to be. This hands the output to + * SQLite and lets the engine judge it: if the statement will not parse, the + * test fails with the engine's own error. + * + * It lives in `apps/web` rather than `packages/sql` on purpose — the sql + * package is pure and may not import Node built-ins (`purity.test.ts` enforces + * it), and this needs `node:sqlite`. + * + * SQLite is the only engine reachable without credentials, so it is the only + * one covered here. It still catches the whole class of "the identifier was + * never quoted" bugs, which is dialect-independent. + */ +import { DatabaseSync } from 'node:sqlite'; +import { describe, expect, it } from 'vitest'; +import { CompareModule, SqlGeneratorModule } from '@foxschema/sql'; +import type { TableSchema } from '@foxschema/sql'; + +const gen = new SqlGeneratorModule(); + +/** DDL for creating `tables` from nothing, as the migration flow would emit it. */ +async function createSql(tables: TableSchema[]): Promise { + const result = await new CompareModule().compare(tables, [], { + source: 'sqlite', + target: 'sqlite', + }); + return gen.generateMigrationPlan(result.tables, 'sqlite').flatMap((step) => step.statements); +} + +/** Runs every statement, surfacing the engine's complaint with the statement. */ +function runAll(statements: string[]): void { + const db = new DatabaseSync(':memory:'); + try { + for (const statement of statements) { + try { + db.exec(statement); + } catch (err) { + throw new Error( + `SQLite rejected:\n${statement}\n\n${err instanceof Error ? err.message : String(err)}` + ); + } + } + } finally { + db.close(); + } +} + +const table = (over: Partial & { name: string }): TableSchema => ({ + objectType: 'TABLE', + columns: [], + indices: [], + foreignKeys: [], + ...over, +}); + +describe('generated DDL actually parses', () => { + it('creates an ordinary table', async () => { + // The control: if this ever fails, the harness is wrong, not the generator. + await expect( + createSql([ + table({ + name: 'customers', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'email', type: 'VARCHAR(255)', nullable: true, primaryKey: false }, + ], + }), + ]).then(runAll) + ).resolves.toBeUndefined(); + }); + + it('creates a table whose name and columns contain spaces', async () => { + // Northwind ships `Order Details`; this is not a hypothetical name. Before + // identifiers were quoted, this produced `CREATE TABLE Order Details (...)`, + // which no engine accepts. + const sql = await createSql([ + table({ + name: 'Order Details', + columns: [ + { name: 'order id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'unit price', type: 'DECIMAL(10,2)', nullable: true, primaryKey: false }, + ], + primaryKey: { name: 'pk order details', columns: ['order id'] }, + }), + ]); + runAll(sql); + }); + + it('creates a table whose columns are reserved words', async () => { + // `select` and `order` are syntax errors bare; `key` and `user` are fine in + // SQLite but reserved in MySQL, so all four are quoted for every dialect. + const sql = await createSql([ + table({ + name: 'order', + columns: [ + { name: 'select', type: 'VARCHAR(10)', nullable: true, primaryKey: false }, + { name: 'order', type: 'INTEGER', nullable: true, primaryKey: false }, + { name: 'key', type: 'INTEGER', nullable: true, primaryKey: false }, + { name: 'user', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }), + ]); + runAll(sql); + }); + + it('creates a table with punctuation and non-ASCII letters in names', async () => { + const sql = await createSql([ + table({ + name: 'naïve-table', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'café', type: 'TEXT', nullable: true, primaryKey: false }, + { name: 'a.b', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }), + ]); + runAll(sql); + }); + + it('creates a table whose name contains the quote character itself', async () => { + // The escaping case: a name holding `"` must double it, or the quoting + // that was meant to fix the statement is what breaks it. + const sql = await createSql([ + table({ + name: 'we"ird', + columns: [{ name: 'i"d', type: 'INTEGER', nullable: false, primaryKey: false }], + }), + ]); + runAll(sql); + }); + + it('indexes and foreign keys on awkward names still parse', async () => { + const sql = await createSql([ + table({ + name: 'parent table', + columns: [{ name: 'parent id', type: 'INTEGER', nullable: false, primaryKey: true }], + primaryKey: { columns: ['parent id'] }, + }), + table({ + name: 'child table', + columns: [ + { name: 'child id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'parent id', type: 'INTEGER', nullable: true, primaryKey: false }, + ], + primaryKey: { columns: ['child id'] }, + indices: [{ name: 'idx child parent', columns: ['parent id'], unique: false }], + foreignKeys: [ + { + name: 'fk child parent', + columns: ['parent id'], + referencedTable: 'parent table', + referencedColumns: ['parent id'], + }, + ], + }), + ]); + runAll(sql); + }); + + it('alters a table with awkward names', async () => { + // ADD / DROP COLUMN go through per-dialect hooks rather than the CREATE + // path, so they need their own proof. + const before = table({ + name: 'my table', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'old col', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }); + const after = table({ + name: 'my table', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'new col', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }); + + runAll(await createSql([before])); + + const diff = await new CompareModule().compare([after], [before], { + source: 'sqlite', + target: 'sqlite', + }); + const alter = gen.generateMigrationPlan(diff.tables, 'sqlite').flatMap((s) => s.statements); + expect(alter.length).toBeGreaterThan(0); + + // Same database: create the original, then apply the migration to it. + const db = new DatabaseSync(':memory:'); + try { + for (const statement of await createSql([before])) db.exec(statement); + for (const statement of alter) { + try { + db.exec(statement); + } catch (err) { + throw new Error( + `SQLite rejected:\n${statement}\n\n${err instanceof Error ? err.message : String(err)}` + ); + } + } + const columns = db + .prepare(`PRAGMA table_info("my table")`) + .all() + .map((row) => String((row as { name: unknown }).name)); + expect(columns).toContain('new col'); + expect(columns).not.toContain('old col'); + } finally { + db.close(); + } + }); +}); diff --git a/packages/sql/src/interfaces/diff.types.interface.ts b/packages/sql/src/interfaces/diff.types.interface.ts index 8950ab4b..2edb48e8 100644 --- a/packages/sql/src/interfaces/diff.types.interface.ts +++ b/packages/sql/src/interfaces/diff.types.interface.ts @@ -3,10 +3,19 @@ import { type TableSchema, type DbObjectType } from './schema-provider.interface export type DiffType = 'ADDED' | 'REMOVED' | 'MODIFIED' | 'UNCHANGED'; export interface ColumnDiff { + /** Uppercased compare-key match name — NOT a real identifier, see source.name. */ name: string; status: 'ADDED' | 'REMOVED' | 'MODIFIED' | 'UNCHANGED'; - source?: { type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; - target?: { type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; + /** + * `name` is the column's own identifier in its native casing, which is what + * DDL must use — the same trap as `IndexDiff` below. Compare has always + * passed the whole ColumnInfo through; only this declaration hid the field, + * so `ALTER TABLE … ADD "NEW COL"` was emitted for a column actually called + * `new col`. Optional rather than required because this package is + * published — every producer inside the repo sets it. + */ + source?: { name?: string; type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; + target?: { name?: string; type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; } export interface IndexDiff { diff --git a/packages/sql/src/modules/schema-fuzz.test.ts b/packages/sql/src/modules/schema-fuzz.test.ts new file mode 100644 index 00000000..f5eb91a0 --- /dev/null +++ b/packages/sql/src/modules/schema-fuzz.test.ts @@ -0,0 +1,410 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * A generated edge-case sweep across every dialect. + * + * The hand-written dialect tests cover the cases somebody thought of. This + * builds adversarial schemas instead — reserved words as identifiers, mixed + * casing, quote characters inside names, empty tables, composite keys — and + * asserts properties that must hold for **all 14 dialects at once**. Where a + * property is checked per dialect, the failure message names the dialect and + * the seed, so a red run reproduces exactly. + * + * Everything is seeded and deterministic: a fuzz test that finds a different + * bug on every CI run is a flaky test, not a fuzzer. + */ +import { describe, expect, it } from 'vitest'; +import { CompareModule } from './compare.module.js'; +import { SqlGeneratorModule } from './sql-generator.module.js'; +import { DIALECT_MAP } from './dialect-registry.js'; +import type { TableSchema, ColumnInfo } from '../interfaces/index.js'; + +const DIALECTS = Object.keys(DIALECT_MAP); +const gen = new SqlGeneratorModule(); + +/** Deterministic PRNG (mulberry32) — same seed, same schema, every run. */ +function rng(seed: number): () => number { + let a = seed >>> 0; + return () => { + a = (a + 0x6d2b79f5) >>> 0; + let t = Math.imul(a ^ (a >>> 15), 1 | a); + t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t; + return ((t ^ (t >>> 14)) >>> 0) / 4294967296; + }; +} + +const pick = (next: () => number, items: readonly T[]): T => + items[Math.floor(next() * items.length) % items.length]!; + +/** + * Identifiers chosen to be awkward on purpose. + * + * Reserved words are the common production break (a column actually called + * `order` or `key`); the casing entries exist because the compare key is + * uppercased and has been mistaken for the real identifier before. + */ +const NAMES = [ + 'customer', + 'CUSTOMER', + 'Customer', + 'order', // reserved almost everywhere + 'select', + 'group', + 'user', + 'table', + 'index', + 'from', + 'primary', + 'mixed_Case_Name', + 'trailing_underscore_', + 'a', // single character + 'x'.repeat(64), // long, but under most identifier limits +]; + +/** + * Names that have to be quoted or escaped to be legal at all. + * + * Kept separate from NAMES so a failure says which class of hostility broke: + * a reserved word is a keyword problem, a name holding the dialect's own quote + * character is an escaping problem, and the two have different fixes. + */ +const NEEDS_QUOTING = [ + 'select', // reserved: a syntax error bare, on every engine + 'order', + 'key', // legal bare in SQLite, reserved in MySQL — quoted for all + 'user', + 'with space', + 'quote"inside', + 'back`tick', + 'bracket]close', + 'semi;colon', + 'dash-name', + 'naïve', + 'UPPER lower', +]; + +const TYPES = [ + 'INTEGER', + 'BIGINT', + 'SMALLINT', + 'VARCHAR(10)', + 'VARCHAR(255)', + 'CHAR(1)', + 'TEXT', + 'DECIMAL(10,2)', + 'DATE', + 'TIMESTAMP', + 'BOOLEAN', +]; + +function column(next: () => number, name: string): ColumnInfo { + return { + name, + type: pick(next, TYPES), + nullable: next() > 0.3, + primaryKey: false, + }; +} + +/** One table with 1..6 awkward columns, sometimes a PK, index or FK. */ +function table(next: () => number, name: string): TableSchema { + const count = 1 + Math.floor(next() * 6); + const used = new Set(); + const columns: ColumnInfo[] = []; + for (let i = 0; i < count; i++) { + // Names are matched case-insensitively, so two columns differing only by + // case are the *same* column — generating both would make an invalid table. + const candidate = `${pick(next, NAMES)}_${i}`; + if (used.has(candidate.toUpperCase())) continue; + used.add(candidate.toUpperCase()); + columns.push(column(next, candidate)); + } + if (columns.length === 0) columns.push(column(next, 'id_0')); + + const schema: TableSchema = { + name, + objectType: 'TABLE', + columns, + indices: [], + foreignKeys: [], + }; + + if (next() > 0.4) { + const pk = columns[0]!; + pk.nullable = false; + pk.primaryKey = true; + schema.primaryKey = { name: `pk_${name}`, columns: [pk.name] }; + } + if (next() > 0.6 && columns.length > 1) { + schema.indices = [ + { + name: `idx_${name}_${columns[1]!.name}`, + columns: [columns[1]!.name], + unique: next() > 0.7, + }, + ]; + } + return schema; +} + +function schemaOf(seed: number): TableSchema[] { + const next = rng(seed); + const count = 1 + Math.floor(next() * 4); + const names = new Set(); + const tables: TableSchema[] = []; + for (let i = 0; i < count; i++) { + const name = `${pick(next, NAMES)}_t${i}`; + if (names.has(name.toUpperCase())) continue; + names.add(name.toUpperCase()); + tables.push(table(next, name)); + } + return tables; +} + +/** A plausible next version: add, drop, widen, retype, toggle nullability. */ +function mutate(tables: TableSchema[], seed: number): TableSchema[] { + const next = rng(seed + 977); + return tables.map((t) => { + const columns = t.columns + .filter(() => next() > 0.2) // drop some + .map((c) => (next() > 0.6 ? { ...c, type: pick(next, TYPES) } : c)) + .map((c) => (next() > 0.8 ? { ...c, nullable: !c.nullable && !c.primaryKey } : c)); + if (next() > 0.5) columns.push(column(next, `added_${Math.floor(next() * 1000)}`)); + // A table must keep at least one column to be a table at all. + return { ...t, columns: columns.length > 0 ? columns : t.columns.slice(0, 1) }; + }); +} + +const SEEDS = Array.from({ length: 40 }, (_, i) => i + 1); + +/** Every statement a migration plan would run, flattened. */ +function planSql(diffs: Awaited>, dialect: string): string[] { + return gen + .generateMigrationPlan(diffs.tables, dialect) + .flatMap((step) => step.statements); +} + +describe('generated schemas · self-compare is empty', () => { + // The most basic promise the product makes: comparing a schema to itself + // reports no work. A false positive here means the app invents a migration + // for two identical databases. + it.each(DIALECTS)('%s reports no changes against itself', async (dialect) => { + for (const seed of SEEDS) { + const tables = schemaOf(seed); + const result = await new CompareModule().compare(tables, tables, { + source: dialect, + target: dialect, + }); + const drifted = result.tables.filter((t) => t.status !== 'UNCHANGED'); + expect( + drifted.map((t) => t.tableName), + `${dialect} seed ${seed} invented a change` + ).toEqual([]); + expect(planSql(result, dialect), `${dialect} seed ${seed} emitted DDL`).toEqual([]); + } + }); +}); + +describe('generated schemas · comparing the other way round', () => { + it.each(DIALECTS)('%s mirrors added and removed when the sides swap', async (dialect) => { + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + const forward = await new CompareModule().compare(a, b, { source: dialect, target: dialect }); + const back = await new CompareModule().compare(b, a, { source: dialect, target: dialect }); + + // "What must the target change to match the source" — swapping the sides + // turns every addition into a removal. If these ever disagree the two + // directions are telling the user different stories about one change. + expect(forward.summary.added, `${dialect} seed ${seed}`).toBe(back.summary.removed); + expect(forward.summary.removed, `${dialect} seed ${seed}`).toBe(back.summary.added); + expect(forward.summary.modified, `${dialect} seed ${seed}`).toBe(back.summary.modified); + } + }); +}); + +describe('generated schemas · the compare key never reaches the DDL', () => { + // `tableName` is the uppercased match key, not an identifier. Emitting it is + // a real bug that has shipped here before (and breaks case-sensitive MySQL). + it.each(DIALECTS)('%s writes the table its own name', async (dialect) => { + for (const seed of SEEDS) { + const a = schemaOf(seed); + const lower = a.filter((t) => t.name !== t.name.toUpperCase()); + if (lower.length === 0) continue; + // Everything is new, so every table is written out in full. + const result = await new CompareModule().compare(a, [], { + source: dialect, + target: dialect, + }); + const sql = planSql(result, dialect).join('\n'); + for (const t of lower) { + expect(sql, `${dialect} seed ${seed} lost the real name of ${t.name}`).toContain(t.name); + } + } + }); +}); + +describe('generated schemas · names that cannot be written bare', () => { + // Every one of these is a legal name in a real database and none of them can + // be emitted raw. The expected wrapper comes from the dialect itself, so a + // dialect that changes its quoting style updates this test with it. + it.each(DIALECTS)('%s quotes hostile table and column names', async (dialect) => { + const quote = + DIALECT_MAP[dialect]!.quoteIdentifier ?? ((n: string) => `"${n.replace(/"/g, '""')}"`); + + for (const name of NEEDS_QUOTING) { + const result = await new CompareModule().compare( + [ + { + name, + objectType: 'TABLE', + columns: [{ name, type: 'INTEGER', nullable: true, primaryKey: false }], + indices: [], + foreignKeys: [], + }, + ], + [], + { source: dialect, target: dialect } + ); + const sql = planSql(result, dialect).join('\n'); + expect(sql, `${dialect} did not quote ${name}`).toContain(quote(name)); + } + }); +}); + +describe('generated schemas · statements are well-formed', () => { + it.each(DIALECTS)('%s emits balanced quotes and parentheses', async (dialect) => { + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + const result = await new CompareModule().compare(a, b, { source: dialect, target: dialect }); + for (const statement of planSql(result, dialect)) { + // Strip escaped string literals before counting, so a legitimate + // '' inside a literal does not read as an unbalanced quote. + const withoutLiterals = statement.replace(/''/g, '').replace(/'[^']*'/g, "''"); + const singles = (withoutLiterals.match(/'/g) ?? []).length; + expect(singles % 2, `${dialect} seed ${seed}: odd quote count in ${statement}`).toBe(0); + + const opens = (statement.match(/\(/g) ?? []).length; + const closes = (statement.match(/\)/g) ?? []).length; + expect(opens, `${dialect} seed ${seed}: unbalanced parens in ${statement}`).toBe(closes); + } + } + }); +}); + +describe('type mapping · every dialect against every other', () => { + // The migration path for a cross-dialect move is parse on the source side, + // render on the target side. These are the properties that make that safe. + const NATIVE = [ + ...TYPES, + 'VARCHAR(4000)', + 'NUMERIC(38,10)', + 'DOUBLE PRECISION', + 'TIMESTAMP(6)', + 'CHARACTER VARYING(50)', + ]; + + it.each(DIALECTS)('%s renders a usable type for anything it parses', (dialect) => { + const d = DIALECT_MAP[dialect]!; + for (const native of NATIVE) { + const rendered = d.renderType(d.parseType(native)); + // An empty or `undefined` type reaches the DDL as `col ` and fails at the + // engine — the one outcome that must never happen silently. + expect(rendered.sql?.trim(), `${dialect} rendered nothing for ${native}`).toBeTruthy(); + expect(rendered.sql, `${dialect} rendered undefined for ${native}`).not.toMatch( + /undefined|NaN|\[object/i + ); + } + }); + + it.each(DIALECTS)('%s round-trips its own rendered type unchanged', (dialect) => { + // Rendering must be a fixed point: parse(render(x)) === render(x). If it is + // not, re-comparing a migrated database reports a change that is not there, + // and the tool proposes the same migration forever. + const d = DIALECT_MAP[dialect]!; + for (const native of NATIVE) { + const once = d.renderType(d.parseType(native)).sql; + const twice = d.renderType(d.parseType(once)).sql; + expect(twice, `${dialect}: ${native} → ${once} → ${twice}`).toBe(once); + } + }); + + it('translates every type between every pair of dialects without dropping it', () => { + const losses: string[] = []; + for (const from of DIALECTS) { + for (const to of DIALECTS) { + if (from === to) continue; + for (const native of NATIVE) { + const rendered = DIALECT_MAP[to]!.renderType(DIALECT_MAP[from]!.parseType(native)); + if (!rendered.sql?.trim()) losses.push(`${from} → ${to}: ${native} rendered empty`); + } + } + } + expect(losses, losses.join('\n')).toEqual([]); + }); +}); + +describe('generated schemas · dialects cross-checked against each other', () => { + it('agree on which objects need a migration', async () => { + // The comparison is dialect-aware (type equivalence differs), but *which + // tables changed* should not: a column dropped is dropped everywhere. One + // dialect disagreeing with the other thirteen is the signal worth having. + const disagreements: string[] = []; + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + const byDialect = new Map(); + for (const dialect of DIALECTS) { + const result = await new CompareModule().compare(a, b, { + source: dialect, + target: dialect, + }); + byDialect.set( + dialect, + result.tables + .filter((t) => t.status !== 'UNCHANGED') + .map((t) => `${t.tableName}:${t.status}`) + .sort() + .join(',') + ); + } + const tally = new Map(); + for (const [dialect, shape] of byDialect) { + tally.set(shape, [...(tally.get(shape) ?? []), dialect]); + } + if (tally.size > 1) { + const groups = [...tally.entries()] + .map(([shape, names]) => `${names.join('+')} → ${shape || '(no changes)'}`) + .join(' | '); + disagreements.push(`seed ${seed}: ${groups}`); + } + } + expect(disagreements, disagreements.join('\n')).toEqual([]); + }); + + it('every dialect can express a change it reported', async () => { + // A dialect that reports MODIFIED and then emits nothing has told the user + // there is work to do and quietly refused to do it. Real limits exist + // (SQLite cannot drop arbitrary columns), so this reports rather than + // asserting a hard equality — the list is the finding. + const silent: string[] = []; + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + for (const dialect of DIALECTS) { + const result = await new CompareModule().compare(a, b, { + source: dialect, + target: dialect, + }); + const changed = result.tables.filter((t) => t.status !== 'UNCHANGED'); + if (changed.length > 0 && planSql(result, dialect).length === 0) { + silent.push(`${dialect} seed ${seed}: ${changed.length} changed, 0 statements`); + } + } + } + expect(silent, silent.join('\n')).toEqual([]); + }); +}); diff --git a/packages/sql/src/modules/sql-dialect.interface.ts b/packages/sql/src/modules/sql-dialect.interface.ts index 91619bc8..9c382c58 100644 --- a/packages/sql/src/modules/sql-dialect.interface.ts +++ b/packages/sql/src/modules/sql-dialect.interface.ts @@ -120,6 +120,17 @@ export interface SqlDialect { */ nullableTypeWrapper?(typeSql: string, nullable: boolean): string; + /** + * Wrap an identifier that cannot be written bare — a name holding a space, + * punctuation, or a non-ASCII letter, as read from the live catalog. + * + * Omit for ANSI double quotes, which is right for every dialect here except + * MySQL's default mode (backticks) and SQL Server (brackets). The generator + * calls this **only** when the name actually needs it, so ordinary names are + * emitted bare exactly as before. + */ + quoteIdentifier?(name: string): string; + /** * Full ` COLLATE ...` clause (with leading space) for a column's collation, used in * CREATE TABLE / ADD COLUMN. Default: ` COLLATE `, unquoted — correct for diff --git a/packages/sql/src/modules/sql-generator.module.ts b/packages/sql/src/modules/sql-generator.module.ts index 8c28779b..ca214d89 100644 --- a/packages/sql/src/modules/sql-generator.module.ts +++ b/packages/sql/src/modules/sql-generator.module.ts @@ -1,8 +1,9 @@ -import { type TableDiff } from '../interfaces/index.js'; +import { type TableDiff, type ColumnDiff } from '../interfaces/index.js'; import { type TableSchema, type DbObjectType } from '../interfaces/index.js'; import type { IndexInfo } from '../interfaces/index.js'; import type { SqlDialect, ColumnSpec } from './sql-dialect.interface.js'; import { resolveDialect } from './dialect-registry.js'; +import { dialectSupportsFk, type FkFeatureSupport } from './dialect-fk-support.js'; export interface MigrationStep { objectName: string; @@ -45,10 +46,119 @@ const PROCEDURAL_TYPES: ReadonlySet = new Set(['VIEW', 'FUNCTION', // unlike VIEW/TRIGGER, which are ordered after ALTER (see generateMigrationPlan). const ROUTINE_TYPES: ReadonlySet = new Set(['FUNCTION', 'PROCEDURE']); +/** + * True when a name can be written into SQL exactly as it is. + * + * Deliberately conservative: letters, digits, underscore, `$` and `#` (Oracle + * and DB2 allow the last two bare), never starting with a digit. Anything else + * — a space, a dot, punctuation, a quote character, a non-ASCII letter — has to + * be quoted or the statement is a syntax error. + */ +/** + * Words that must be quoted to be usable as an identifier. + * + * The union across the supported engines, not any single one's list: a column + * called `key` is fine in SQLite and a syntax error in MySQL, and the generator + * emits for whichever dialect it was handed. Quoting a word one engine happens + * not to reserve costs nothing — the name is quoted exactly as the catalog + * spelled it, so it still resolves to the same object. + * + * Deliberately not exhaustive. It covers the clause keywords and the ones that + * turn up as real column names; a full per-dialect list is a bigger job and + * would only add words nobody names a column after. + */ +const RESERVED_WORDS: ReadonlySet = new Set([ + 'ADD', 'ALL', 'ALTER', 'AND', 'ANY', 'AS', 'ASC', 'BETWEEN', 'BOTH', 'BY', + 'CASE', 'CHECK', 'COLUMN', 'CONSTRAINT', 'CREATE', 'CROSS', 'CURRENT', + 'CURRENT_DATE', 'CURRENT_TIME', 'CURRENT_TIMESTAMP', 'CURRENT_USER', + 'DEFAULT', 'DELETE', 'DESC', 'DISTINCT', 'DROP', 'ELSE', 'END', 'EXCEPT', + 'EXISTS', 'FALSE', 'FETCH', 'FOR', 'FOREIGN', 'FROM', 'FULL', 'GRANT', + 'GROUP', 'HAVING', 'IN', 'INDEX', 'INNER', 'INSERT', 'INTERSECT', 'INTO', + 'IS', 'JOIN', 'KEY', 'LEADING', 'LEFT', 'LIKE', 'LIMIT', 'NATURAL', 'NOT', + 'NULL', 'OFFSET', 'ON', 'OR', 'ORDER', 'OUTER', 'PRIMARY', 'REFERENCES', + 'RENAME', 'REVOKE', 'RIGHT', 'ROW', 'ROWS', 'SELECT', 'SESSION_USER', 'SET', + 'SOME', 'TABLE', 'THEN', 'TO', 'TRAILING', 'TRUE', 'UNION', 'UNIQUE', + 'UPDATE', 'USER', 'USING', 'VALUES', 'VIEW', 'WHEN', 'WHERE', 'WITH', +]); + +function isBareIdentifier(name: string): boolean { + if (!/^[A-Za-z_][A-Za-z0-9_$#]*$/.test(name)) return false; + return !RESERVED_WORDS.has(name.toUpperCase()); +} + +/** ANSI quoting, correct everywhere except MySQL's default mode. */ +function ansiQuoteIdentifier(name: string): string { + return `"${name.replace(/"/g, '""')}"`; +} + +/** + * Already wrapped — by an earlier pass here, or by the catalog that handed it + * over. Quoting it again nests the quotes and breaks the statement, and + * `bareName` genuinely is applied twice on some paths (the primary-key name + * reaches it once on the way in and once on the way out). + */ +function isQuotedIdentifier(name: string): boolean { + if (name.length < 2) return false; + const first = name[0]!; + const last = name[name.length - 1]!; + return ( + (first === '"' && last === '"') || + (first === '`' && last === '`') || + (first === '[' && last === ']') + ); +} + export class SqlGeneratorModule { /** Table keys left in an FK cycle after Kahn sort (uppercase bare names). */ private fkCycleKeys = new Set(); + /** + * How this dialect wraps an identifier that cannot be written bare. Set from + * the resolved dialect at each entry point, like `fkCycleKeys` — threading it + * through all 30-odd `qualify`/`bareName` call sites buys nothing. + */ + private quoteIdentifier: (name: string) => string = ansiQuoteIdentifier; + + /** + * Dialect name for the run in progress, for the capability matrices that are + * keyed by name rather than by strategy object (`dialectSupportsFk`). + */ + private dialectName = ''; + + /** What this dialect can do with foreign keys. Empty name ⇒ the full matrix. */ + private fkSupport(): FkFeatureSupport { + return dialectSupportsFk(this.dialectName); + } + + /** + * A name as it should appear in SQL: bare when that is legal, quoted when it + * is not. + * + * Names come from a live catalog, so they are whatever somebody actually + * created — `Order Details` (Northwind ships with it), a column called + * `order id`, a Postgres table created with quotes. Emitting those raw + * produced SQL that could not parse on any dialect. + * + * Quoting only what *needs* it keeps every ordinary name byte-identical to + * what this generator emitted before, which is why this could be fixed + * without rewriting the dialect test suites. + */ + private ident(name: string): string { + if (isBareIdentifier(name) || isQuotedIdentifier(name)) return name; + return this.quoteIdentifier(name); + } + + /** + * A column's real identifier, ready to emit. + * + * `ColumnDiff.name` is the uppercased match key, exactly like `tableName` — + * emitting it renamed `new col` to `NEW COL`. The native casing lives on + * whichever side of the diff exists. + */ + private columnIdent(col: ColumnDiff): string { + return this.ident(col.source?.name ?? col.target?.name ?? col.name); + } + /** * Source catalog definitions qualify names with the source schema (HUY.GPX_FILE); * deploying into a different schema requires rewriting those qualifiers. @@ -64,9 +174,9 @@ export class SqlGeneratorModule { .replace(new RegExp(`\\b${escaped}\\.`, 'gi'), `${tgt}.`); } - /** Drops any leading "schema." prefix from an object name. */ + /** Drops any leading "schema." prefix from an object name, and quotes if needed. */ private bareName(name: string): string { - return name.replace(/^"?[^".]+"?\./, ''); + return this.ident(name.replace(/^"?[^".]+"?\./, '')); } /** @@ -191,7 +301,7 @@ export class SqlGeneratorModule { } else { typeSql = translated.sql + dialect.identityClause(c); } - let def = `${c.name} ${typeSql}`; + let def = `${this.ident(c.name)} ${typeSql}`; // COLLATE goes right after the type on every dialect that supports a per-column // collation (Postgres/MySQL/MariaDB/SQL Server/Oracle 12.2+), before DEFAULT/NOT // NULL. Dialects that never populate collation (DB2) simply never hit this. @@ -213,7 +323,22 @@ export class SqlGeneratorModule { // MySQL names every PK constraint "PRIMARY" — emitting CONSTRAINT PRIMARY PRIMARY KEY // is redundant and confusing. Skip the CONSTRAINT clause for that reserved name. const constraintName = pkName && pkName.toUpperCase() !== 'PRIMARY' ? `CONSTRAINT ${this.bareName(pkName)} ` : ''; - lines.push(` ${constraintName}PRIMARY KEY (${pkCols.join(', ')})`); + lines.push(` ${constraintName}PRIMARY KEY (${pkCols.map((c) => this.ident(c)).join(', ')})`); + } + + // A dialect that cannot ALTER a constraint in must declare it here or lose + // it: for SQLite the CREATE TABLE is the only chance the FK ever gets. + const fk = this.fkSupport(); + if (!fk.alterAdd && fk.createInline) { + for (const key of table.foreignKeys ?? []) { + const cols = (key.columns ?? []).map((c) => this.ident(c)); + const parent = (key.referencedColumns ?? []).map((c) => this.ident(c)); + if (cols.length === 0 || parent.length !== cols.length) continue; + const named = key.name ? `CONSTRAINT ${this.bareName(key.name)} ` : ''; + lines.push( + ` ${named}FOREIGN KEY (${cols.join(', ')}) REFERENCES ${this.qualify(key.referencedTable, mapping)} (${parent.join(', ')})` + ); + } } return `CREATE TABLE ${this.qualify(table.name, mapping)} (\n${lines.join(',\n')}\n);`; @@ -224,10 +349,15 @@ export class SqlGeneratorModule { * index (SQL Server renders it as ALTER TABLE ADD CONSTRAINT). `idx.name` must be bare. */ private createIndexSql(idx: IndexInfo, qualifiedTable: string, dialect?: SqlDialect): string { - if (dialect?.createIndexStatement) return dialect.createIndexStatement(idx, qualifiedTable); - const uniqueStr = idx.unique ? ' UNIQUE' : ''; - const whereClause = idx.filter?.trim() ? ` WHERE ${idx.filter.trim()}` : ''; - return `CREATE${uniqueStr} INDEX ${idx.name} ON ${qualifiedTable} (${idx.columns.join(', ')})${whereClause};`; + // Quote the column names *before* handing them to a dialect hook: the hooks + // build their own column list, so a hook-owning dialect (SQLite, SQL Server) + // would otherwise emit `ON t (order id)` while the generic path below got + // this right. `ident` is idempotent, so quoting here is safe either way. + const quoted: IndexInfo = { ...idx, columns: idx.columns.map((c) => this.ident(c)) }; + if (dialect?.createIndexStatement) return dialect.createIndexStatement(quoted, qualifiedTable); + const uniqueStr = quoted.unique ? ' UNIQUE' : ''; + const whereClause = quoted.filter?.trim() ? ` WHERE ${quoted.filter.trim()}` : ''; + return `CREATE${uniqueStr} INDEX ${quoted.name} ON ${qualifiedTable} (${quoted.columns.join(', ')})${whereClause};`; } /** @@ -247,9 +377,19 @@ export class SqlGeneratorModule { if (columns.length === 0 || refCols.length !== columns.length) { return `-- review: skip FK ${label} — referenced columns missing or length mismatch`; } + // SQLite and ClickHouse have no ALTER TABLE ... ADD CONSTRAINT. The matrix + // already knew that and only the blueprint UI was reading it, so the + // migration emitted a statement the engine rejects outright. A note beats + // DDL that cannot run. + const fk = this.fkSupport(); + if (!fk.alterAdd) { + return `-- review: skip FK ${label} — ${fk.hint}`; + } + const cols = columns.map((c) => this.ident(c)).join(', '); + const parentCols = refCols.map((c) => this.ident(c)).join(', '); const fkBody = multiline - ? `FOREIGN KEY (${columns.join(', ')}) REFERENCES ${referencedTable} (${refCols.join(', ')})` - : `FOREIGN KEY (${columns.join(', ')}) REFERENCES ${referencedTable} (${refCols.join(', ')})`; + ? `FOREIGN KEY (${cols}) REFERENCES ${referencedTable} (${parentCols})` + : `FOREIGN KEY (${cols}) REFERENCES ${referencedTable} (${parentCols})`; if (multiline) { return `ALTER TABLE ${qualifiedTable} ADD CONSTRAINT ${constraintName} \n ${fkBody};`; } @@ -286,6 +426,8 @@ export class SqlGeneratorModule { generateObjectDdl(table: TableSchema, dialectStr = 'db2'): string { const dialect = resolveDialect(dialectStr); + this.quoteIdentifier = dialect.quoteIdentifier ?? ansiQuoteIdentifier; + this.dialectName = dialectStr; if (table.objectType === 'SEQUENCE') return this.renderCreateSequence(table, dialect); if (table.objectType === 'TYPE') return this.renderCreateType(table, dialect); if (table.objectType !== 'TABLE' && table.objectType !== 'MQT') { @@ -604,7 +746,11 @@ export class SqlGeneratorModule { } else { addTypeSql = translated.sql; } - let colDef = `${col.name} ${addTypeSql}`; + // Pre-quoted here rather than inside each dialect's hook: the hooks all + // interpolate the name they are given, so quoting at the one call site + // fixes ADD COLUMN on all 14 at once. `ident` is idempotent, so a hook + // that already quotes is unaffected. + let colDef = `${this.columnIdent(col)} ${addTypeSql}`; if (col.source.collation) colDef += dialect.columnCollateClause?.(col.source.collation) ?? ` COLLATE ${col.source.collation}`; if (!dialect.nullableTypeWrapper) { // Oracle requires DEFAULT before NOT NULL; the standard allows either order. @@ -627,7 +773,7 @@ export class SqlGeneratorModule { const colSpec = this.isCrossDialect(mapping) ? { ...col.source, type: translated.sql, defaultValue: undefined, collation: undefined } : { ...col.source, type: translated.sql }; - statements.push(...dialect.modifyColumnStatements(tableName, col.name, colSpec, col.target?.nullable)); + statements.push(...dialect.modifyColumnStatements(tableName, this.columnIdent(col), colSpec, col.target?.nullable)); // The compare flags a column as MODIFIED when only its DEFAULT differs, but // the type/nullability statements above don't carry the default — apply it @@ -644,14 +790,14 @@ export class SqlGeneratorModule { const seqForDefault = dialect.serialSequenceFromDefault?.(srcDef ?? ''); if (seqForDefault) statements.push(`CREATE SEQUENCE IF NOT EXISTS ${this.qualify(seqForDefault, mapping)};`); const requalifiedDef = srcDef !== undefined ? this.requalifyDefault(srcDef, mapping) : srcDef; - statements.push(...dialect.setDefaultStatements(tableName, col.name, requalifiedDef)); + statements.push(...dialect.setDefaultStatements(tableName, this.columnIdent(col), requalifiedDef)); } } } if (!mapping?.nonDestructive) { for (const col of obj.columnDiffs.filter((c) => c.status === 'REMOVED')) { - statements.push(dialect.dropColumnStatement(tableName, col.name)); + statements.push(dialect.dropColumnStatement(tableName, this.columnIdent(col))); } } @@ -957,6 +1103,8 @@ export class SqlGeneratorModule { contextDiffs?: TableDiff[] ): MigrationStep[] { const dialect = resolveDialect(dialectStr); + this.quoteIdentifier = dialect.quoteIdentifier ?? ansiQuoteIdentifier; + this.dialectName = dialectStr; // Pin the target dialect into the mapping so the render helpers can detect a // cross-dialect migration and translate column types accordingly. const m: SchemaMapping = { ...mapping, targetDialect: mapping?.targetDialect ?? dialectStr }; diff --git a/packages/sql/src/modules/type-mapping.ts b/packages/sql/src/modules/type-mapping.ts index e1257bdf..ce9df715 100644 --- a/packages/sql/src/modules/type-mapping.ts +++ b/packages/sql/src/modules/type-mapping.ts @@ -69,7 +69,14 @@ function shapeCanonical(base: CanonicalBase, tok: TypeToken, raw: string): Canon if (base === 'char' || base === 'varchar' || base === 'binary' || base === 'varbinary') { if (tok.length !== undefined) t.length = tok.length; } else if (base === 'decimal') { - if (tok.precision !== undefined) t.precision = tok.precision; + // A single-argument `NUMBER(10)` / `DECIMAL(10)` tokenizes as a *length*, + // because the tokenizer cannot know which types read one arg as a + // precision. For a decimal it is always a precision (SQL defines + // DECIMAL(p) as DECIMAL(p,0)), and ignoring it rendered a bare `NUMBER` — + // silently widening an Oracle NUMBER(10) column to full 38-digit precision + // on every migration. + const precision = tok.precision ?? tok.length; + if (precision !== undefined) t.precision = precision; if (tok.scale !== undefined) t.scale = tok.scale; } return t; diff --git a/packages/sql/src/providers/mysql/mysql.sql-dialect.ts b/packages/sql/src/providers/mysql/mysql.sql-dialect.ts index cfed184a..2eae6fd0 100644 --- a/packages/sql/src/providers/mysql/mysql.sql-dialect.ts +++ b/packages/sql/src/providers/mysql/mysql.sql-dialect.ts @@ -81,6 +81,12 @@ const mysqlDialect: SqlDialect = { return c.identity ? ` AUTO_INCREMENT` : ''; }, + // Backticks, not ANSI double quotes: MySQL only reads `"x"` as an identifier + // under ANSI_QUOTES, which is off by default. An embedded backtick doubles. + quoteIdentifier(name: string): string { + return `\`${name.replace(/`/g, '``')}\``; + }, + addColumnStatement(tableName: string, colDef: string): string { return `ALTER TABLE ${tableName} ADD ${colDef};`; }, diff --git a/packages/sql/src/providers/redshift/redshift.sql-dialect.ts b/packages/sql/src/providers/redshift/redshift.sql-dialect.ts index bdeb3754..22cc13fb 100644 --- a/packages/sql/src/providers/redshift/redshift.sql-dialect.ts +++ b/packages/sql/src/providers/redshift/redshift.sql-dialect.ts @@ -48,7 +48,9 @@ const types = makeDialectTypeFns({ double: plain('double precision'), char: sized('char'), varchar: sized('varchar'), - text: plain('varchar(max)'), + // Redshift has no VARCHAR(MAX) — that is T-SQL, and the server rejects it. + // 65535 bytes is the documented maximum width for a Redshift VARCHAR. + text: plain('varchar(65535)'), binary: plain('varbyte'), varbinary: plain('varbyte'), blob: plain('varbyte'), @@ -58,7 +60,7 @@ const types = makeDialectTypeFns({ timestamptz: plain('timestamptz'), uuid: plain('varchar(36)'), json: plain('super'), - xml: plain('varchar(max)'), + xml: plain('varchar(65535)'), }, }); diff --git a/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts b/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts index d219ba0f..f02fe8de 100644 --- a/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts +++ b/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts @@ -64,6 +64,12 @@ export const sqlServerSqlDialect: SqlDialect = { return c.identity ? ` IDENTITY(1,1)` : ''; }, + // Brackets are the T-SQL form and work regardless of QUOTED_IDENTIFIER, which + // ANSI double quotes do not. A closing bracket inside the name is doubled. + quoteIdentifier(name: string): string { + return `[${name.replace(/]/g, ']]')}]`; + }, + addColumnStatement(tableName: string, colDef: string): string { return `ALTER TABLE ${tableName} ADD ${colDef};`; }, From 3d8582508888aac5a2603f6a83a7da4a1d551c6b Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 20:54:54 -0600 Subject: [PATCH 6/7] fix(sql-editor): count every joined table, and no phantom CTEs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `referencedTableNames` promises the *physical* tables a statement touches, and the multi-table write warning is built on it. It got both directions wrong. `FROM orders JOIN customers` reported only `orders`: the optional-alias group matched `JOIN` as the alias of `orders` and moved `lastIndex` past it, so `customers` was never scanned. A write across two tables looked like a write across one — the warning under-reported, which is the fail-open direction. The opposite error too: `WITH recent AS (…) SELECT * FROM recent` counted `recent`, a name that exists only inside the query, so the warning counted objects that do not exist. CTE names are now excluded — at this caller only, since autocomplete legitimately wants them. Adds 44 adversarial CTE/subquery cases against the safety gates: data-modifying CTEs (`WITH x AS (DELETE …) SELECT 1` leads with the word WITH), nested CTEs, `EXPLAIN ANALYZE`, and write verbs hidden inside string literals and comments. They encode the rule that a misread must fail closed — calling a read a write costs one dialog; calling a write a read runs unreviewed DDL. The existing gates passed all of them unchanged. The first version of the CTE-name scan was a regex with adjacent optional whitespace groups, which eslint's security plugin correctly flagged as ReDoS-prone — reachable from the editor, where the input is whatever the user typed. Replaced with a single-pass scanner, pinned by a timing test. Co-Authored-By: Claude Opus 5 --- packages/sql/src/modules/cte-syntax.test.ts | 184 ++++++++++++++++++++ packages/sql/src/modules/sql-splitter.ts | 113 +++++++++++- 2 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 packages/sql/src/modules/cte-syntax.test.ts diff --git a/packages/sql/src/modules/cte-syntax.test.ts b/packages/sql/src/modules/cte-syntax.test.ts new file mode 100644 index 00000000..3efd3435 --- /dev/null +++ b/packages/sql/src/modules/cte-syntax.test.ts @@ -0,0 +1,184 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Adversarial CTE and subquery syntax, aimed at the safety gates. + * + * Every guard in the SQL editor — the write-confirmation dialog, the RBAC + * `editor.write` check, the "this UPDATE has no WHERE" warning — decides what a + * statement *is* by scanning its text. A data-modifying CTE begins with the + * word `WITH`, so a scanner that reads only the leading verb calls + * `WITH x AS (DELETE FROM accounts) SELECT 1` a read and waves it straight past + * the confirmation. These cases exist to keep that hole shut. + * + * The rule they encode: **a misread must fail closed.** Calling a read a write + * costs one extra confirmation dialog; calling a write a read runs unreviewed + * DDL against the user's database. + */ +import { describe, expect, it } from 'vitest'; +import { + isWriteStatement, + requiresWritePermission, + splitSqlStatements, + dmlLacksWhere, + referencedTableNames, +} from './sql-splitter.js'; + +describe('data-modifying CTEs are writes', () => { + const WRITES = [ + // Postgres' data-modifying CTE: the DELETE runs, the SELECT reads what it + // removed. Leading verb is WITH. + 'WITH gone AS (DELETE FROM accounts RETURNING *) SELECT * FROM gone', + 'WITH x AS (SELECT 1) INSERT INTO audit SELECT * FROM x', + 'WITH x AS (UPDATE t SET c = 1 RETURNING id) SELECT count(*) FROM x', + // Nested one level down — the write is in a CTE of a CTE. + 'WITH a AS (WITH b AS (UPDATE t SET c = 1 RETURNING *) SELECT * FROM b) SELECT * FROM a', + // Recursive CTE whose tail is the write. + 'WITH RECURSIVE t(n) AS (SELECT 1 UNION ALL SELECT n+1 FROM t WHERE n < 5) DELETE FROM u WHERE id IN (SELECT n FROM t)', + // Casing and whitespace are not a defence. + 'WiTh X aS (dElEtE fRoM t) sElEcT 1', + '\n\n with x as (\n delete from t\n )\n select 1', + // A comment before the statement must not hide the verb. + '-- harmless looking\nWITH x AS (DELETE FROM t) SELECT 1', + '/* block */ WITH x AS (TRUNCATE TABLE t) SELECT 1', + // EXPLAIN ANALYZE executes its inner statement on Postgres. + 'EXPLAIN ANALYZE WITH x AS (DELETE FROM t) SELECT 1', + // SELECT … INTO creates a table; wrapped in a CTE it still does. + 'WITH x AS (SELECT 1 AS n) SELECT * INTO backup FROM x', + ]; + + it.each(WRITES)('classifies as a write: %s', (sql) => { + expect(isWriteStatement(sql), sql).toBe(true); + }); + + it.each(WRITES)('requires write permission: %s', (sql) => { + expect(requiresWritePermission(sql), sql).toBe(true); + }); +}); + +describe('read-only CTEs and subqueries stay readable', () => { + // The other half of the contract. If ordinary analytical SQL demanded a + // confirmation every time, people would learn to click through the dialog — + // which is how the dialog stops protecting anything. + const READS = [ + 'WITH recent AS (SELECT * FROM orders WHERE created_at > now() - interval \'7 days\') SELECT count(*) FROM recent', + 'WITH RECURSIVE tree(id, parent) AS (SELECT id, parent FROM nodes WHERE parent IS NULL UNION ALL SELECT n.id, n.parent FROM nodes n JOIN tree ON n.parent = tree.id) SELECT * FROM tree', + 'SELECT * FROM (SELECT id FROM t WHERE x = 1) AS sub', + 'SELECT (SELECT max(id) FROM orders) AS newest, (SELECT count(*) FROM users) AS people', + 'WITH a AS (SELECT 1), b AS (SELECT 2) SELECT * FROM a CROSS JOIN b', + // A write verb inside a string literal is data, not a statement. + "SELECT 'DELETE FROM accounts' AS example", + "WITH x AS (SELECT '; DROP TABLE t; --' AS s) SELECT * FROM x", + // …and inside a comment. + 'SELECT 1 -- DELETE FROM accounts', + 'SELECT /* UPDATE t SET x=1 */ 1', + ]; + + it.each(READS)('classifies as a read: %s', (sql) => { + expect(isWriteStatement(sql), sql).toBe(false); + }); +}); + +describe('splitting statements that contain CTEs and subqueries', () => { + it('keeps a CTE and its tail together as one statement', () => { + const sql = 'WITH x AS (SELECT 1) SELECT * FROM x'; + expect(splitSqlStatements(sql).filter((s) => s.text.trim())).toHaveLength(1); + }); + + it('does not split on a semicolon inside a CTE string literal', () => { + // Splitting here would run `WITH x AS (SELECT '` as a statement of its own. + const sql = "WITH x AS (SELECT 'a;b' AS s) SELECT * FROM x"; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(1); + expect(parts[0]!.text).toContain("'a;b'"); + }); + + it('splits two CTE statements at the boundary between them', () => { + const sql = 'WITH a AS (SELECT 1) SELECT * FROM a;\nWITH b AS (SELECT 2) SELECT * FROM b;'; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(2); + expect(parts[1]!.text).toContain('b'); + }); + + it('treats a semicolon inside a dollar-quoted body as ordinary text', () => { + const sql = 'CREATE FUNCTION f() RETURNS int AS $$ BEGIN DELETE FROM t; RETURN 1; END $$ LANGUAGE plpgsql'; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(1); + }); + + it('classifies each statement of a mixed batch on its own merits', () => { + const sql = 'SELECT 1;\nWITH x AS (DELETE FROM t) SELECT 1;'; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(2); + expect(isWriteStatement(parts[0]!.text)).toBe(false); + expect(isWriteStatement(parts[1]!.text)).toBe(true); + }); +}); + +describe('missing-WHERE warning sees through a CTE', () => { + it('flags a CTE-wrapped DELETE with no WHERE', () => { + // The warning exists to stop `DELETE FROM accounts` emptying a table by + // accident. Wrapping it in a CTE must not silence it. + expect(dmlLacksWhere('WITH x AS (SELECT 1) DELETE FROM accounts')).toBe(true); + }); + + it('does not flag one that has a WHERE', () => { + expect(dmlLacksWhere('WITH x AS (SELECT 1) DELETE FROM accounts WHERE id = 1')).toBe(false); + }); +}); + +describe('referenced tables in CTE queries', () => { + it('names the real tables a CTE query reads', () => { + const names = referencedTableNames( + 'WITH recent AS (SELECT * FROM orders) SELECT * FROM recent JOIN customers ON true' + ).map((n) => n.toLowerCase()); + expect(names).toContain('orders'); + expect(names).toContain('customers'); + }); + + it('recognises a CTE that declares a column list', () => { + const names = referencedTableNames( + 'WITH t(a, b) AS (SELECT x, y FROM source) SELECT * FROM t' + ).map((n) => n.toLowerCase()); + expect(names).toEqual(['source']); + }); + + it('recognises MATERIALIZED and NOT MATERIALIZED fences', () => { + for (const fence of ['MATERIALIZED', 'NOT MATERIALIZED']) { + const names = referencedTableNames( + `WITH t AS ${fence} (SELECT * FROM source) SELECT * FROM t` + ).map((n) => n.toLowerCase()); + expect(names, fence).toEqual(['source']); + } + }); + + it('handles several CTEs, including one whose body holds a comma', () => { + const names = referencedTableNames( + 'WITH a AS (SELECT x, y FROM one), b AS (SELECT z FROM two) SELECT * FROM a JOIN b ON true' + ).map((n) => n.toLowerCase()); + expect(names.sort()).toEqual(['one', 'two']); + }); + + it('scans pathological input in linear time', () => { + // The regex this replaced had adjacent optional whitespace groups, which + // backtrack exponentially. Input like this is reachable from the editor, so + // a slow scan is a denial of service, not a performance nit. Generous + // bound: the point is "not exponential", not a benchmark. + const evil = `WITH ${' '.repeat(50_000)}`; + const started = Date.now(); + referencedTableNames(evil); + referencedTableNames(`WITH a AS ${'('.repeat(2_000)}`); + expect(Date.now() - started).toBeLessThan(2_000); + }); + + it('does not report a CTE alias as a table', () => { + // `recent` is a name that exists only inside the query. Reporting it as a + // table makes the multi-table write warning count phantom tables, and any + // dependency scan built on this would chase an object that never existed. + const names = referencedTableNames( + 'WITH recent AS (SELECT * FROM orders) SELECT * FROM recent' + ).map((n) => n.toLowerCase()); + expect(names).not.toContain('recent'); + }); +}); diff --git a/packages/sql/src/modules/sql-splitter.ts b/packages/sql/src/modules/sql-splitter.ts index deb49d4c..6fee4061 100644 --- a/packages/sql/src/modules/sql-splitter.ts +++ b/packages/sql/src/modules/sql-splitter.ts @@ -1330,15 +1330,116 @@ export function collectMultiTableWriteWarnings( return out; } -/** Unique bare/qualified table names from {@link extractTableAliases} values. */ +/** + * Names defined by this statement's `WITH` clause. + * + * A CTE name looks exactly like a table in `FROM recent`, but it exists only + * for the length of the query. Autocomplete still wants it (you can type + * `recent.`), so this filters at the caller rather than in + * {@link extractTableAliases}. + */ +function cteNames(sql: string): Set { + const names = new Set(); + const s = stripSqlStringsAndComments(sql); + let i = 0; + + // A hand-rolled scan rather than one regex. The pattern this replaces held + // several adjacent optional `\s*` groups, which is ambiguous enough to + // backtrack exponentially — a denial of service reachable from the SQL + // editor, where the input is whatever the user typed. This walks the string + // once instead, and reads more like the grammar it is matching. + const isSpace = (c: string | undefined): boolean => c === ' ' || c === '\t' || c === '\n' || c === '\r' || c === '\f' || c === '\v'; + const skipSpace = (): void => { + while (i < s.length && isSpace(s[i])) i++; + }; + /** Case-insensitive keyword match on a fixed-length slice, then consume it. */ + const eatWord = (word: string): boolean => { + const end = i + word.length; + if (s.slice(i, end).toLowerCase() !== word) return false; + const after = s[end]; + if (after !== undefined && /[\w$]/.test(after)) return false; + i = end; + return true; + }; + /** Consume a balanced `( … )` run, respecting nesting. */ + const skipParens = (): boolean => { + if (s[i] !== '(') return false; + let depth = 0; + for (; i < s.length; i++) { + if (s[i] === '(') depth++; + else if (s[i] === ')') { + depth--; + if (depth === 0) { + i++; + return true; + } + } + } + return false; // unbalanced — stop scanning rather than guess + }; + const readIdent = (): string | null => { + const open = s[i]; + const close = open === '"' ? '"' : open === '`' ? '`' : open === '[' ? ']' : null; + if (close) { + const end = s.indexOf(close, i + 1); + if (end < 0) return null; + const raw = s.slice(i, end + 1); + i = end + 1; + return stripIdentQuotes(raw); + } + const start = i; + while (i < s.length && /[\w$]/.test(s[i]!)) i++; + return i > start ? s.slice(start, i) : null; + }; + + skipSpace(); + if (s[i] === '(') { + i++; + skipSpace(); + } + if (!eatWord('with')) return names; + skipSpace(); + eatWord('recursive'); + + for (;;) { + skipSpace(); + const name = readIdent(); + if (!name) return names; + skipSpace(); + // Optional column list: `WITH t(a, b) AS (…)`. + if (s[i] === '(' && !skipParens()) return names; + skipSpace(); + if (!eatWord('as')) return names; + skipSpace(); + // Postgres optimisation fences. + if (eatWord('not')) skipSpace(); + if (eatWord('materialized')) skipSpace(); + if (s[i] !== '(') return names; + names.add(name.toLowerCase()); + if (!skipParens()) return names; + skipSpace(); + if (s[i] !== ',') return names; + i++; + } +} + +/** + * Unique **physical** table names from {@link extractTableAliases} values. + * + * CTE names are excluded: counting `recent` in + * `WITH recent AS (SELECT * FROM orders) SELECT * FROM recent` as a table made + * the multi-table write warning count objects that do not exist. + */ export function referencedTableNames(sql: string): string[] { const map = extractTableAliases(sql); + const ctes = cteNames(sql); const seen = new Set(); const out: string[] = []; for (const table of Object.values(map)) { const key = table.toLowerCase(); if (seen.has(key)) continue; seen.add(key); + if (ctes.has(key)) continue; out.push(table); } return out; @@ -1396,7 +1497,15 @@ export function extractTableAliases(sql: string): Record { if (!aliasRaw) continue; const alias = stripIdentQuotes(aliasRaw); if (!alias) continue; - if (ALIAS_KEYWORD_BLACKLIST.has(alias.toLowerCase())) continue; + if (ALIAS_KEYWORD_BLACKLIST.has(alias.toLowerCase())) { + // The optional alias group swallowed a keyword: in `FROM orders JOIN + // customers`, `JOIN` matched as the alias of `orders`, leaving lastIndex + // past it so `customers` was never scanned at all. Rewind to where the + // keyword starts so it can open its own match. The alias always begins + // after the match start, so this still moves forward — no loop. + re.lastIndex = m.index + m[0].length - aliasRaw.length; + continue; + } out[alias.toLowerCase()] = table; } return out; From 65555b475b0c63ff779e57d715d0ba7a7d32eb90 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 20:55:05 -0600 Subject: [PATCH 7/7] fix(history): draw the graph nodes on the minimap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The minimap rendered as an empty white box over a canvas full of nodes. React Flow measures the DOM for the canvas itself, but it only writes those measurements back to the caller's node objects through `onNodesChange` — and this graph is fully controlled without one. So from the minimap's side every node reported undefined dimensions and `MiniMap` skipped all of them (`nodeHasDimensions(userNode)`), while the canvas rendered perfectly from internal state. Measured before the fix: 14 canvas nodes, 0 minimap nodes, and a minimap SVG holding nothing but its mask path. Declaring `initialWidth`/`initialHeight` satisfies the check without pinning the rendered size, so nodes still grow to fit their content — and unlike adding `onNodesChange`, it introduces no state that could re-render in a loop. Verified in the browser: 14 of 14. Separately, this file held four literal NUL bytes as composite-key separators, which made git treat it as binary and every diff of it opaque. Written as `\x00` escapes they are the same string and the file is text again. Co-Authored-By: Claude Opus 5 --- .../components/lokee-weave/buildGraph.ts | Bin 8854 -> 10173 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/apps/web/src/frontend/components/lokee-weave/buildGraph.ts b/apps/web/src/frontend/components/lokee-weave/buildGraph.ts index abfdb122a6b566cb8d45742f7f279c0b5afa953d..fad2d6a45637d8b37656bff199425485a14b5e0b 100644 GIT binary patch delta 1367 zcmZ`(&ubGw6sFZ)0^>r6SLhKiDQM7}1Cq+lUoRpeYJk=}vZEvSVjwmYGR5u}J(M z6#s^T7a?a49zA&XEdB@n1HPHfrqSx2cA59S`M&RcZ$C6Yw?BPcnN;8=Y+)pr#u%iE z5e$EgCV-S{tu&W?un9uWW1ATN>Wdhjl%4=ca1SC~QEOuf5o~ zyPl0+hR{)pUM)pRPfE?Ctf(8Lc+*sf3h@@Qdlw|?r36Bzx zh_NA9Ps%FLm?>=`<|#^dkAW`I#pvL;B^g3MtXWPURd|Ga(oaYNi6;btW&(b^eP81B zwaQ7ef*!SUs*9Agq&Wa4V~4mv2$U3KK#~U;=% z)+!T%By~@lJDu%zYroarYVLQopEuz?92TZGfD8;EK~7ZZW8)UQ0p__Fd#_UTw;w!i zZtni4y}r<1zx_vdWqP!7@$Bin(fa!I%lSLYlUmKWUb^h9GGvWSTo7_@6E(1ozQHQ& zAd4(K6e@=lnHgv-0-}+L3eZDfTkR+F(M3!S86~Sr5M-z~EZ5xnHbs+8apGZUcI01| zMF(`1R)c<0!i7q2bAI6uMH%86 p#kdex$eXKc!rvRSsBT_d>N`<9lXrCaOd;>x)zx#;uUEdE`VBfY&L;o> delta 54 zcmdn%Kh1T+YSzuG*hTpn88#mlvtZinEq#v($kvsMVFXgW^3{x+uPCwtg(g3c7uoz# H`3y4v@YoTw