From 6659707d4d00341ba0c56016fe62c74373831e17 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 10:27:52 -0600 Subject: [PATCH 1/2] 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/2] 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();