Skip to content

feat(hub-projects): generate and apply datatable migrations on project publish/install#9977

Draft
diegoimbert wants to merge 127 commits into
tl/workspace-to-hubfrom
di/hub-projects-datatable
Draft

feat(hub-projects): generate and apply datatable migrations on project publish/install#9977
diegoimbert wants to merge 127 commits into
tl/workspace-to-hubfrom
di/hub-projects-datatable

Conversation

@diegoimbert

Copy link
Copy Markdown
Contributor

Summary

Integrates datatable migrations into the Hub projects feature. When a project is published to the Hub, its scripts/flows/raw apps are scanned for datatable assets (datatable_name/table_name[/column_name]), and a best-effort CREATE TABLE migration is generated per referenced data table from the source workspace's live schema. On import, the shipped migrations can be run against the target workspace.

Stacked PR. Base is tl/workspace-to-hub. The diff also contains the datatable-migrations work (its own branch, change-68b704f7 / diego/git-890-datatable-migrations) because that branch isn't in the base. The integration-specific commits are 198cd33762, af3f87ee65, a4d64a9b08.

Changes

Detection & generation (publish side)

  • projectMigrations.ts: detect datatable assets across a project's selected scripts/flows/raw apps and generate a CREATE TABLE migration per data table from the source workspace's live schema.
  • Close the generated table set over foreign keys: transitively pull a referenced table's FK targets into the migration (ordered by FK dependency), and drop any FK whose target still isn't in the set so the SQL always runs.
  • New "Data table migrations" section in DeployToHub — the publisher can edit/toggle the generated migrations in the bundle drawer.
  • Extract the pure SQL-generation out of DatatableSchemaDiff.svelte into datatableSchemaSql.ts so plain .ts modules can import it.

Import side

  • projects/install: run/skip modal for the shipped migrations. When the target data table opted into migrations they're applied as recorded migrations (datatable_migrations + _wm_migrations); otherwise as a one-off preview job.
  • Missing target data tables are surfaced and skipped via a confirmation.

Backend

  • POST /hub/migrations proxy in hub_publish.rs forwarding to the Hub (consistent with the existing thin-proxy pattern).

Fix

  • ConfirmationModal gains an optional zIndexClass prop (default z-[9999], backward-compatible); the DB-manager out-of-order confirm uses z-[10000] so it stacks above the DB table editor's still-open preview modal.

Test plan

  • projectMigrations.test.ts — asset detection, migration generation, FK closure ordering
  • Publish a project whose script/flow/raw app references main/tableA and main/tableB → confirm one migration per table is generated with correct columns and FK-ordered CREATE TABLEs
  • A table with an FK to a table not referenced by the project → confirm the FK target is pulled in, or the FK is dropped if unresolvable
  • Import the project into a workspace where the target data table opted into migrations → migrations recorded in datatable_migrations + _wm_migrations
  • Import where the data table did NOT opt in → migrations run as a one-off preview job
  • Import where the target data table is missing → missing-datatable confirmation, migration skipped
  • On a migrations-enabled data table, create a table via the DB editor → the out-of-order confirm stacks above the preview modal

Screenshots

Deferred. Exercising the publish→install datatable-migration flow requires the full backend + Hub running; screenshots to be added once verified against a running environment.

🤖 Generated with Claude Code

diegoimbert and others added 10 commits July 6, 2026 14:16
The merge command upserted datatable_migration definitions into the target
workspace and reported the item as successfully deployed, but never ran the
migrations. For forked datatables backed by separate databases, this left the
target schema unchanged until someone manually ran `wmill datatable migrate up`,
while the CLI reported a successful merge.

Collect the datatable migrations deployed (not deleted) into the target and,
after the deploy loop, offer to run them via the existing offerToRunNewMigrations
helper — the same post-deploy run prompt the push/sync path uses (interactive
only; `--yes`/non-TTY skip the mutating run, matching push behavior). Export
parseDatatableMigrationDeployPath so the merge path can parse the deployed items.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n lock

A migration run snapshots a migration's code_up from datatable_migrations and
only records its version in the data table's _wm_migrations after the job
succeeds. upsert_datatable_migration checked _wm_migrations before allowing an
edit but took no lock, so a concurrent edit could read "not applied yet",
rewrite code_up/code_down, and then the in-flight run would record the version
for the old SQL — leaving _wm_migrations pointing at SQL that was never applied
(migrate up then skips it; rollback runs a down that doesn't match).

Serialize definition rewrites and deletes with the same per-database advisory
lock the run/rollback paths use:
- Factor the connect+advisory-lock into lock_datatable_migration_runs and the
  applied-versions read into read_applied_versions_on_client.
- run_datatable_migrations now snapshots the definitions AFTER taking the lock,
  so code_up can't change between snapshot and version-record.
- upsert (when changing an existing def) and delete take the lock across the
  applied-check and the write; delete now rejects deleting an already-applied
  migration (would orphan its _wm_migrations record), symmetric with upsert.
  Both fail closed if the data table database is unreachable.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…editor preview

Creating a table on a migrations-enabled data table opened the DB table editor's
"Confirm running the following" preview modal, whose confirm triggers applyDdl,
which then asks for out-of-order confirmation. Both are ConfirmationModals with a
hardcoded z-[9999]; the out-of-order one lives in DBManagerContent (mounted before
the editor), so it rendered behind the still-open preview modal.

Add an optional zIndexClass prop to ConfirmationModal (default z-[9999],
backward-compatible) and give the DB-manager out-of-order confirm z-[10000] so it
stacks on top.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Detect datatable assets in a project's scripts/flows/raw apps when
publishing to the Hub, generate a best-effort CREATE TABLE migration per
data table from the source workspace's live schema, and let the publisher
edit/toggle them in the bundle drawer. On import, offer to run the shipped
migrations: recorded (datatable_migrations + _wm_migrations) when the
target data table opted into migrations, otherwise as a one-off preview
job. Missing target data tables are surfaced and skipped.

- backend: POST /hub/migrations proxy forwarding to the Hub
- frontend publish: projectMigrations.ts detection + generation, new
  "Data table migrations" section in DeployToHub
- frontend import: run/skip modal + missing-datatable confirmation
- extract pure SQL-gen from DatatableSchemaDiff.svelte into
  datatableSchemaSql.ts so plain .ts modules can import it

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…keys

Pull a referenced table's FK targets into the generated migration
transitively, so it creates every table it references (ordered by FK
dependency), and drop any FK whose target still isn't in the set so the
generated SQL always runs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 7, 2026

Copy link
Copy Markdown

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 232002b
Status: ✅  Deploy successful!
Preview URL: https://67039902.windmill.pages.dev
Branch Preview URL: https://di-hub-projects-datatable.windmill.pages.dev

View logs

diegoimbert and others added 6 commits July 7, 2026 12:15
Detect data table usage off the predeploy bundle preview and surface it as
a "Data table dependencies" summary right after "Resource dependencies",
mirroring how resource types and triggers are shown. The editable
migration itself stays in the bundle drawer.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a table can't be found in the schema, a data table is referenced as a
whole, or the schema can't be loaded, write a `--` comment describing the
problem into the migration instead of leaving it blank. Partial migrations
keep the CREATE TABLEs that did generate and comment the rest; comment-only
migrations stay disabled. The bundle drawer now always shows the SQL box so
those comments are visible and editable.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…migration

Replace the plain "run migrations?" confirmation with a review drawer that
previews each runnable migration, lets the user edit the SQL and toggle
which to run, before the import proceeds. When recording an imported
migration, also record a down migration (DROP TABLE of the created tables,
in reverse order) derived from the up SQL, so it can be rolled back; the
derived rollback is previewed in both the publish bundle drawer and the
import review drawer.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the plain textarea with a Monaco SQL editor split into Up/Down
tabs. The down migration is now generated once as best-effort (DROP TABLE
in reverse creation order) and is fully editable — no longer parsed back
out of the up SQL. The down is threaded through publish → Hub → import
(new project_migration.sql_down) and recorded as code_down when an imported
migration is applied.

- projectMigrations: GeneratedMigration.sql_down generated from the table set
- MigrationSqlEditor.svelte: shared Up/Down tabbed Monaco editor (re-keyed on
  regeneration since Monaco ignores external code changes)
- DeployToHub + install review drawer use it; sql_down pushed/applied
- backend: PublishMigrationBody carries sql_down

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@diegoimbert

Copy link
Copy Markdown
Contributor Author

/ai review this PR

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @diegoimbert's task in 3m 49s —— View job


Review: datatable migrations on project publish/install

Read through all 8 files and traced the generation/install logic. Overall this is well-structured and thoroughly commented — the FK-closure + prune + topological-order pipeline is sound and the test coverage on the generation side is good. A few correctness/robustness points, none blocking:

1. Up migration is not idempotent, but FK closure makes collisions likely (medium)

generateMigrationSql emits CREATE TABLE (not CREATE TABLE IF NOT EXISTS) — datatableSchemaSql.ts:172. Meanwhile expandFkClosure (projectMigrations.ts:128) transitively pulls a referenced table's FK targets into the same transaction. So if a project references orders, customers gets pulled in; on import into a workspace that already has customers (common — it's a shared parent table), the whole BEGIN;…COMMIT; fails on the first CREATE TABLE customers and none of the tables are created. The down migration already uses DROP TABLE IF EXISTS, so the asymmetry is notable. Worth considering CREATE TABLE IF NOT EXISTS (with the caveat that a name collision on a differently-shaped table then silently no-ops). At minimum the "best-effort, editable" framing should call this out. Fix this →

2. orderByFkDependency keys by bare table name while the rest of the pipeline is schema-qualified (low)

expandFkClosure / pruneSchemaForTables use schema-qualified keys (tableKey = schema.table), but orderByFkDependency (projectMigrations.ts:168) builds inSet/byName/deps off t.tableName alone and derives FK targets via .split('.').pop(). Two same-named tables in different schemas collapse in byName, so one is silently dropped from ordered and never gets a CREATE TABLE. Data tables are usually single-schema so this is an edge case, but it's an inconsistency worth aligning (key by schema.table here too).

3. Rollback can DROP pre-existing tables the import didn't create (medium)

The generated sql_down (projectMigrations.ts:297) drops every table in the ordered set, including FK-pulled-in parents (customers) that may have pre-existed in the target. A rollback of a hub-import migration would then drop a table the project never created — potential data loss. It's editable and manually triggered, but the down SQL arguably shouldn't blindly include the transitively-added tables.

4. Import button stays enabled during the migration review modal (low)

In install/+page.svelte:359, planMigrations awaits the review drawer / missing-datatable modal before setting installing = true. The Import button is only disabled on installing || done, so a second click during review kicks off a concurrent install() (second review drawer, duplicated item creation). Consider gating the button while the plan modal is open. Fix this →

5. regenerateMigrations swallows failures silently (low)

DeployToHub.svelte:645 — on error the drafts are cleared and the drawer shows "No data table usage detected", which is indistinguishable from a genuine schema-load failure. A sendUserToast on the catch would help the publisher tell the two apart.

Minor

  • PR body describes the datatable asset path as datatable_name/table_name[/column_name], but the SQL parser emits datatable/[schema.]table with the column carried separately (from_column), never in the asset path. The code (parseDatatableAssetPath) matches the parser, not the PR text — so this is just a doc nit, code is correct.
  • resolveTable (projectMigrations.ts:101): a schema-qualified ref whose schema doesn't match (e.g. wrongschema.orders) returns undefined rather than falling back to a bare-name lookup of orders. Edge case.
  • Backend hub_publish.rs proxy is consistent with the existing thin-proxy pattern (admin gate + slug validation + forward_to_hub) — no issues.
  • Nice: the reviewResolve capture-then-clear in closeMigrationReview correctly prevents the on:close-fired run=false from clobbering an explicit Run/Skip choice.

None of these are blockers; #1 and #3 are the ones most worth a decision before shipping (idempotency + rollback data-loss). I didn't run the test suite or exercise the publish→install flow (needs the full backend + Hub, as noted in the PR).
· di/hub-projects-datatable

diegoimbert and others added 5 commits July 7, 2026 14:05
…grations

The FK closure pulls a referenced table's parents into the same transaction
(e.g. `orders` drags in `customers`); those shared parents often already
exist in the target, so a plain CREATE TABLE aborted the whole migration on
the first collision. Emit CREATE TABLE IF NOT EXISTS for project migrations
(via a new opt-in flag on generateMigrationSql, leaving the schema-diff
behavior unchanged) so a pre-existing parent is skipped instead of failing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
orderByFkDependency keyed its dependency graph by bare table name (and
resolved FK targets with .split('.').pop()), so two same-named tables in
different schemas collapsed and one was dropped from the ordered set and
never created. Key by schema.table like the rest of the pipeline, resolving
FK targets through resolveTable. Also let resolveTable fall back to the bare
table name when a schema-qualified ref's schema doesn't match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The generated down migration listed DROP TABLE for every table in the FK
closure, including shared parent tables that may have pre-existed in the
target — a rollback could drop a table the project never created (data
loss). Emit all DROP statements commented out with a note, so nothing is
dropped by default; the publisher uncomments the tables this migration
actually owns.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
planMigrations awaits the review / missing-datatable modals before setting
installing = true, so the Import button stayed enabled during review and a
second click launched a concurrent install() (second review drawer,
duplicated item creation). Track a planningMigrations flag, disable the
button on it, and early-return install() if already installing or planning.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
regenerateMigrations cleared the drafts on error, showing "No data table
usage detected" — indistinguishable from a genuine schema-load failure. Add
a toast on the catch so the publisher can tell the two apart.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@diegoimbert

Copy link
Copy Markdown
Contributor Author

/ai review this PR

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @diegoimbert's task in 3m 14s —— View job


Review: datatable migrations on project publish/install

Re-reviewed the full diff against origin/tl/workspace-to-hub. The five issues from the earlier pass have all been addressed in follow-up commits (IF NOT EXISTS, schema-qualified FK ordering, commented-out down DROPs, planningMigrations import guard, regenerate-failure toast) — nice. Below are the remaining points from a fresh read; none are blockers.

1. The FK ALTER TABLE … ADD CONSTRAINT is not idempotent, so the IF NOT EXISTS fix is only half-complete (medium)

generateMigrationSql (datatableSchemaSql.ts:176-183) now emits CREATE TABLE IF NOT EXISTS, but the foreign keys are still added unconditionally via ALTER TABLE … ADD CONSTRAINT fk_… FOREIGN KEY …, and renderForeignKey (dbQueriesUtils.ts:45-54) emits a named constraint. FK closure pulls a shared parent (customers) into the transaction; if that parent already exists in the target and itself carries an in-set FK, CREATE TABLE IF NOT EXISTS customers correctly no-ops, but the following ALTER TABLE customers ADD CONSTRAINT fk_… runs against the pre-existing table and fails on the duplicate constraint — aborting the whole BEGIN;…COMMIT;. That's the exact "one pre-existing parent kills the transaction" failure the IF NOT EXISTS commit set out to prevent, just narrowed to FK-bearing parents (and any re-run). Worth either guarding the FK add (e.g. DO $$ … IF NOT EXISTS (…pg_constraint…) $$) or at least noting in the SQL comment that FK re-adds aren't idempotent. Fix this →

2. Re-importing the same project can collide on the recorded migration name (low)

applyOneMigration (install/+page.svelte:340) records with a stable name hub_import_${slug}. On a migrations-enabled target, importing the same project twice (across page loads — the button self-disables only within one session) issues two createDatatableMigration calls with the same name. Depending on the backend's uniqueness handling in datatable_migrations this either errors or double-records. A per-import suffix (or a check for an existing entry) would make re-import safe.

3. The missing-datatable modal can't actually cancel the import (low)

planMigrations (install/+page.svelte:308) awaits missingDatatableModal.ask(...) but ignores the boolean. ask() resolves false on cancel / true on confirm and never rejects, so both the "Import without them" button and dismissing the modal proceed with the import. If the intent is to let the user back out when a data table is missing, wire the returned boolean to abort (if (!ok) throw / early-return); otherwise the modal is purely informational and the cancel affordance is misleading.

Minor / nits

  • Migration sync ships disabled + comment-only drafts. DeployToHub.svelte:1291 posts all migrationDrafts (including enabled:false and comment-only stubs), relying on the Hub to prune empty-SQL entries. Not a bug given the import side re-filters on enabled && sql.trim(), but the publish payload carries dead weight and couples correctness to the Hub's pruning.
  • orderByFkDependency cycle fallback. The topo-sort silently falls back to partial/input order on a cycle (data tables rarely have FK cycles, so fine) — worth a one-line log or comment if you ever expect cycles, since the emitted SQL would then fail at apply time rather than generation time.
  • Backend publish_migrations proxy is consistent with the sibling handlers (admin gate + validate_project_slug + source_key + forward_to_hub) — looks correct.
  • generateMigrationSql's new options?.ifNotExists param is backward-compatible; the existing DatatableSchemaDiff callers pass nothing and keep plain CREATE TABLE. Good.
  • Test coverage on the generation pipeline (FK closure, ordering, pruning, dangling-FK drop, comment stubs) is solid. Gap: no test exercises the applyOneMigration recorded-vs-preview branch or planMigrations missing/runnable split — those are the install-side paths most likely to regress.

Overall this is well-structured and the commentary is unusually good at recording the why. #1 is the one I'd resolve before shipping (the idempotency story is incomplete for FK-bearing pulled-in parents); #2/#3 are cheap robustness wins. I didn't run the test suite or exercise the publish→install flow (needs full backend + Hub).

· di/hub-projects-datatable

diegoimbert and others added 4 commits July 7, 2026 14:29
planMigrations awaited missingDatatableModal.ask() but ignored its boolean,
so cancelling the "some data tables are missing" warning still proceeded
with the import — the cancel affordance did nothing. Show the warning first
and abort the whole import when the user cancels (planMigrations returns
null; install() early-returns), so they can create the data table(s) and
re-run. Confirming still imports without the missing migrations.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Low-code apps don't carry a persisted asset list, but the DB-table
component declares its data table and table explicitly: a `oneOf` `type`
config with `selected === 'datatable'` holding `datatable://<name>` and the
table. Walk the app value for those configs so an app that reads a data
table is picked up by the Data table dependencies detection.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Full-code (raw) apps explicitly declare the data tables/tables they use in
value.data.tables (refs like main/customers or main/schema:table), which the
"Data table dependencies" detection missed — it only looked at inline-script
assets. Read the declaration via extractDataConfig/parseDataTableRef. The
bundler previously dropped value.data (kept only files + runnables); include
it so detection sees it and the imported app keeps its declaration, and pass
it through on import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant