Feat/browse pane and revert report - #265
Closed
huyplb wants to merge 15 commits into
Closed
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…port 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 <noreply@anthropic.com>
…revert-report # Conflicts: # apps/e2e/src/pages/LokeeHistoryPage.ts # apps/e2e/src/tests/schema-revert.test.ts # apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx # apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx # apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx # apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx # apps/web/src/frontend/store/lokeeHistoryStore.ts
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 <noreply@anthropic.com>
…ntains 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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
The SQLite oracle needs no credentials, so it runs everywhere — but it cannot tell you whether the *per-dialect* quoting is right. Backticks for MySQL, brackets for T-SQL, double quotes elsewhere: only the servers know, and getting it wrong is how a migration dies halfway through a customer's database. Runs the same hostile schemas — spaces, reserved words, punctuation, non-ASCII, an index and an FK over awkward names — against Postgres, MySQL, MariaDB, SQL Server, CockroachDB and YugabyteDB from `docker compose`. 30 cases, all green, with the identifier-quoting fix in place. Gated behind FOX_IT_DB=1 so the default run and CI stay DB-free; unreachable engines skip individually so a partial stack still tells you something. Each case asserts it generated statements before executing them, so an empty plan cannot pass vacuously, and every table it creates is dropped afterwards. Verified the harness can actually fail: the unquoted form of the same statement is rejected by the live server with `syntax error at or near "Order"`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ithfully Creating a table exercises one code path; ADD/DROP/MODIFY COLUMN go through the per-dialect hooks, which is where the dialects diverge most. Adds an ALTER case over awkward names to every live engine, and a DB2 target for when that container is up. The ALTER case failed on Postgres and YugabyteDB at first, and the failure was this harness, not the product — worth writing down because it looks so much like a real bug. Postgres's dependent-view hooks stash view definitions in a `CREATE TEMP TABLE … ON COMMIT DROP` and read them back several statements later, so the plan only holds together when it runs the way MigrationModule runs it: one unpooled connection, one transaction. A connection per statement loses the temp table with the session; a transaction per statement drops it at the first commit. Both report `relation "_fs_vdep_…" does not exist`. `runPlan` now mirrors MigrationModule exactly, so what the test proves is what the product actually does. 49 cases green across Postgres, MySQL, MariaDB, SQL Server, CockroachDB and YugabyteDB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DB2 leaves a table in *reorg-pending* after `ALTER TABLE … DROP COLUMN` (and
some type changes). `SELECT` still works — every INSERT/UPDATE/DELETE, and
every index or key rebuild, fails with **SQL0668N reason code 7** until REORG
runs. So the migration reported success and handed back a table nobody could
write to, and because reads kept working it could be a long time before anyone
connected the two.
Adds a `postColumnChangeStatements` hook, emitted after the column changes and
*before* the keys and indexes are rebuilt — those rebuilds are blocked by the
same pending state. DB2 implements it as
`CALL SYSPROC.ADMIN_CMD('REORG TABLE …')`; bare `REORG TABLE` is a CLP command,
not SQL, and cannot be sent over a client connection. No other dialect
implements the hook, and a unit test holds them to that.
Two harness faults had to be fixed before this bug could even be seen, both
worth recording because each produced a confident green:
* The liveness probe was `SELECT 1`, which DB2 rejects (SQL0104N — it wants a
FROM clause). DB2 was therefore marked unreachable and every DB2 case
returned early, reported as **passing** in 0ms while touching nothing.
Targets now carry their own probe, and an unreachable engine calls
`ctx.skip()` so it can never again be mistaken for a pass.
* The post-migration check was a `SELECT`. Reads are exactly what reorg-pending
still allows, so it went green against a table the user could no longer
write to. It is an INSERT/DELETE now.
Verified both directions against DB2 11.5: without the REORG the live suite
fails with SQL0668N on the write-back; with it, all 49 cases pass across
Postgres, MySQL, MariaDB, SQL Server, CockroachDB, YugabyteDB and DB2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…, drifting defaults Ran the shipped DEMO_A/DEMO_B samples through the actual app path against DB2 11.5 — provider introspection, CompareModule, generated plan applied to a throwaway schema, then re-introspected and re-compared. Three bugs, each of which a string test could not have seen. **Every DB2 comparison carried six phantom ROLE objects.** `SYSCAT.ROLES` was read unfiltered, so DB2's own built-in roles (SYSDEBUG, SYSGEOADM, SYSTS_*) were reported as user objects in every schema. They cannot be recreated — DB2 reserves the SYS prefix and answers `CREATE ROLE SYSDEBUG` with SQL0707N — so the migration also contained eleven statements that can never succeed. In this database *every* role is a system role, so the whole ROLE section was noise. **Sessions never worked on DB2 at all.** The query selected `SESSION_DB_PARTITION_NUM`, which `MON_GET_CONNECTION` does not expose: DB2 answered SQL0206N and the utility failed outright. It is `CURRENT SERVER` now, which is what the `database_name` column claims to be — a partition number was the wrong value for that slot regardless. **Migrations to DB2 never converged.** DB2 rejects adding a NOT NULL column to a populated table without a default (SQL0193N), so the dialect appends `WITH DEFAULT` — correct, and documented. But that leaves the column holding a default (`''`, `0`) the source never declared, so re-comparing straight after a *successful* migration still reported the column as changed and proposed the same work again, for ever. A new `afterAddColumnStatements` hook drops the implicit default once the rows are backfilled; verified on the server that DROP DEFAULT is accepted immediately after the ADD, needs no REORG between, and leaves the catalog default NULL — matching the source exactly. End state on the samples: 30/30 statements execute, and re-comparing after the migration reports **no differences at all**. All five utilities (pool, sessions, system, sizes, index-fragmentation) run against the live server, and DEMO_A is byte-for-byte untouched by the run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…self back
Drove the whole product loop against real DB2 for the first time: seed a scratch
schema with MigrationModule, capture v1 with Lokee, migrate DEMO_A into it,
capture v2, diff the versions, plan a revert, execute it, and check the database
actually went back.
It did not. The revert reported success and changed nothing.
Table drops are ordered before the ALTERs, and dropping a parent table takes its
inbound foreign keys with it — so `ALTER TABLE … DROP FOREIGN KEY` ran against a
constraint DB2 had already removed and raised SQL0204N. DB2 has transactional
DDL, so that one statement rolled the *entire* revert back. Everything else in
the plan was correct; none of it survived.
The dialect already knows this shape: its DROP TABLE/VIEW go through a SQL PL
`CONTINUE HANDLER FOR SQLSTATE '42704'` because DB2 has no DROP IF EXISTS. The
FK drop simply never got the same treatment, even though the generic fallback it
overrides says `DROP CONSTRAINT IF EXISTS` for exactly this reason. It is
wrapped now.
Verified end to end on DB2 11.5: after the revert the live schema matches v1
exactly, and the new version's root hash equals v1's — content-addressed proof
that what came back is identical, not merely similar.
One note for whoever writes the next harness: `MigrationModule.execute` reports
failure through its **event stream** (`{type:'done', success:false,
rolledBack:true}`), not by throwing. My first pass wrapped it in try/catch, saw
no exception, and cheerfully reported a rolled-back revert as applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Routine bodies are the least portable thing in SQL and nothing here exercised them: the DB2 samples contain no routines at all (its seed's compound blocks are only drop-cleanup), so DEMO_A/DEMO_B never tested a single one. Adds the round trip, which is the only version of this test worth running: create a function and a procedure with native DDL, read them back through the provider, ask the generator to recreate them in a *second* schema, and execute that. Asserting "the object exists" would pass on a definition that is empty, missing its terminator, or has the source schema baked into it; executing the regenerated DDL somewhere else catches all three. Covers Postgres, MySQL, SQL Server and DB2 — each with its own body, since there is no portable one. Engines without a spec skip visibly rather than reporting a green they did not earn. All four pass: the captured definition is faithful enough to rebuild the routine elsewhere. MySQL needs the second *database* (a MySQL schema is a database), which the demo user has no rights to create, so that one target carries admin credentials — an environment limit, not a product one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…se buttons `browseSchema`, `isBrowsing`, `dialectOptions` and the `PROVIDER_SETTINGS` import stopped being referenced when Browse became its own pane and the buttons came out of Compare. Dead either way, but they read as if Compare still has a browse path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_53ffef38-cf2a-407c-9bc1-c133f263ab11) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Changes affect live migration/revert DDL and Lokee capture on customer databases across many dialects, though the PR adds substantial automated coverage. UI and metadata changes are lower risk than the generator and revert-scope fixes.
Overview
Browse is now a third Schema Sync pane (with Compare and History), not a button on a connection card. A dedicated BrowseBar picks one database and loads it; the tree gets type filters and empty states that say “Nothing loaded,” and the detail panel can show which database is being read.
Lokee revert records from/to version ids on capture (metadata migration 15) and shows “↩ reverted to vN” on graph nodes. Revert planning now narrows generated DDL to ticked objects (empty selection is a no-op, not whole-schema). The compare modal adds Select all, explicit
objectKeys, a Markdown change report download, and clearer blocked-execute reasons.SQL generation quotes awkward identifiers and uses real column names in DDL; DB2 gets REORG after column drops, implicit-default cleanup after NOT NULL adds, tolerant FK drops, and Sessions query fix; DB2 roles skip built-in
SYS%roles. CTE/subquery classification and table-reference scanning are hardened for editor safety gates.Tests: new E2E for browse and forward revert; expanded revert edge cases;
generated-ddl-runs(SQLite) and optionalFOX_IT_DBlive engine runs; schema fuzz across dialects; lokee-weave provenance and scoped-revert unit tests.Reviewed by Cursor Bugbot for commit 416de6d. Bugbot is set up for automated code reviews on this repo. Configure here.