From 50ddff07b8ee4a9f62b062e5425b426a87f3c2a6 Mon Sep 17 00:00:00 2001 From: tristantr Date: Mon, 22 Jun 2026 15:06:36 +0200 Subject: [PATCH] fix: structural project-ref rewrite and deploy-to-hub state fixes Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/windmill-api/build.rs | 4 +- backend/windmill-api/src/hub_publish.rs | 2 +- .../workspaceSettings/DeployToHub.svelte | 23 ++-- .../workspaceSettings/projectBundle.test.ts | 109 ++++++++++++++++++ .../workspaceSettings/projectBundle.ts | 109 +++++++++++++----- .../(logged)/projects/install/+page.svelte | 72 +++++++++++- .../(logged)/workspace_settings/+page.svelte | 43 ++----- 7 files changed, 284 insertions(+), 78 deletions(-) diff --git a/backend/windmill-api/build.rs b/backend/windmill-api/build.rs index 59ac12bc2b..768df207fb 100644 --- a/backend/windmill-api/build.rs +++ b/backend/windmill-api/build.rs @@ -2,8 +2,8 @@ fn main() { // rust-embed requires the embedded folder to exist at compile time. // When building without a prior frontend build, create the directory // so the derive macro doesn't panic. - let dir = - std::env::var("FRONTEND_BUILD_DIR").unwrap_or_else(|_| "../../frontend/build/".to_string()); + let dir = std::env::var("FRONTEND_BUILD_DIR") + .unwrap_or_else(|_| "../../frontend/build/".to_string()); let path = std::path::Path::new(&dir); if !path.exists() { std::fs::create_dir_all(path).ok(); diff --git a/backend/windmill-api/src/hub_publish.rs b/backend/windmill-api/src/hub_publish.rs index d1e415ce14..b653c7cca3 100644 --- a/backend/windmill-api/src/hub_publish.rs +++ b/backend/windmill-api/src/hub_publish.rs @@ -39,7 +39,7 @@ pub fn workspaced_service() -> Router { // A workspace can publish one Hub project per folder. The stable, never-mutated // link key is `workspace_id:folder_name` (folder name is the path segment and is // never renamed — only display_name changes). `:` is safe: neither workspace ids -// nor folder names (alphanumeric + underscore) contain it. +// nor folder names (alphanumeric, underscore, hyphen) contain it. #[derive(Deserialize)] struct HubScope { folder: String, diff --git a/frontend/src/lib/components/workspaceSettings/DeployToHub.svelte b/frontend/src/lib/components/workspaceSettings/DeployToHub.svelte index 0eb545ca60..189311c87d 100644 --- a/frontend/src/lib/components/workspaceSettings/DeployToHub.svelte +++ b/frontend/src/lib/components/workspaceSettings/DeployToHub.svelte @@ -163,6 +163,9 @@ let workspaceTriggers = $state([]) let triggersLoading = $state(false) let workspaceLoadSeq = 0 + // Guards the reset+reload effect so a spurious same-value workspace-store emit + // (e.g. a layout re-render) doesn't wipe state or close an open drawer. + let lastLoadedKey = '' let schedulePreviews = $state>({}) const schedulePreviewsInFlight = new Set() @@ -432,7 +435,12 @@ } $effect(() => { - if ($workspaceStore) { + const ws = $workspaceStore + const key = `${ws ?? ''}|${folderProp}` + // Only reset+reload when the workspace or folder actually changes — not on + // every store emit, which would close the record/publish drawer mid-use. + if (ws && key !== lastLoadedKey) { + lastLoadedKey = key const seq = ++workspaceLoadSeq // Wipe stale state before the parallel fetches resolve. workspaceItems = [] @@ -1076,9 +1084,8 @@ const workspace = $workspaceStore if (!workspace) return const slug = hubSlug - // Snapshot the selection and workspace sequence up-front so a workspace - // switch mid-deploy can't write stale state into the new workspace. - const deploySeq = workspaceLoadSeq + // Snapshot the selection up-front so a workspace switch mid-deploy can't + // write stale state into the new workspace. const itemsSnapshot = selectedItems.slice() const triggersSnapshot = relevantTriggers.slice() hubItemIds = {} @@ -1183,11 +1190,13 @@ } await sleep(150) - if (deploySeq !== workspaceLoadSeq) return + if ($workspaceStore !== workspace) return deploymentStatus = {} - draftItems = itemsSnapshot.map((i) => ({ ...i, rec: 'none' })) recordings = {} - phase = 'draft' + // Load authoritative state now that every item is committed on the Hub. + // Bumping the sequence cancels any rehydrate that started mid-deploy and + // would otherwise clobber draftItems with a pre-commit (empty) read. + await rehydrateFromHub(workspace, folderProp, ++workspaceLoadSeq) if (failures > 0) { sendUserToast(`Draft pushed with ${failures} failed item(s).`, true) } else { diff --git a/frontend/src/lib/components/workspaceSettings/projectBundle.test.ts b/frontend/src/lib/components/workspaceSettings/projectBundle.test.ts index 1a041e26b1..b10f84ed8f 100644 --- a/frontend/src/lib/components/workspaceSettings/projectBundle.test.ts +++ b/frontend/src/lib/components/workspaceSettings/projectBundle.test.ts @@ -3,9 +3,13 @@ import { classifyPath, extractScriptRefs, extractFlowRefs, + extractAppRefs, buildPathMap, rewriteContent, rewriteFlowValue, + rewriteAppValue, + extractRawAppRefs, + rewriteRawAppContent, buildProjectBundle, type FetchedItem, type ItemRef @@ -169,6 +173,111 @@ describe('rewriteFlowValue', () => { }) }) +// A trimmed app value: a runnable-by-path component, a hub runnable, a $res in an +// inline script, and incidental `f/...` text that must NOT be rewritten. +const appValue = () => ({ + grid: [ + { + data: { + componentInput: { + runnable: { type: 'runnableByPath', runType: 'script', path: 'u/admin/charts' } + } + } + }, + { + data: { + componentInput: { + runnable: { type: 'runnableByPath', runType: 'flow', path: 'f/shared/sync' } + } + } + }, + { + data: { + componentInput: { + runnable: { type: 'runnableByPath', runType: 'hubscript', path: 'hub/1/keep' } + } + } + } + ], + hiddenInlineScripts: [ + { name: 'h', inlineScript: { content: 'x = "$res:u/admin/pg"', language: 'deno' } } + ], + someLabel: 'see docs at f/shared/sync for details' +}) + +describe('extractAppRefs', () => { + it('extracts runnable-by-path scripts/flows and $res resources, skips hub', () => { + const refs = extractAppRefs(appValue()) + expect(refs).toContainEqual({ kind: 'script', path: 'u/admin/charts' }) + expect(refs).toContainEqual({ kind: 'flow', path: 'f/shared/sync' }) + expect(refs).toContainEqual({ kind: 'resource', path: 'u/admin/pg' }) + expect(refs.some((r) => r.path === 'hub/1/keep')).toBe(false) + }) +}) + +describe('rewriteAppValue', () => { + it('relocates runnable paths and $res, leaves hub refs and incidental text intact', () => { + const map = new Map([ + ['u/admin/charts', 'f/proj/charts'], + ['f/shared/sync', 'f/proj/sync'], + ['u/admin/pg', 'f/proj/pg'] + ]) + const value = appValue() + const out = rewriteAppValue(value, map) + expect(out.grid[0].data.componentInput.runnable.path).toBe('f/proj/charts') + expect(out.grid[1].data.componentInput.runnable.path).toBe('f/proj/sync') + expect(out.grid[2].data.componentInput.runnable.path).toBe('hub/1/keep') + expect(out.hiddenInlineScripts[0].inlineScript.content).toBe('x = "$res:f/proj/pg"') + // incidental text untouched + expect(out.someLabel).toBe('see docs at f/shared/sync for details') + // original untouched (deep clone) + expect(value.grid[0].data.componentInput.runnable.path).toBe('u/admin/charts') + }) +}) + +describe('raw app (value.raw JSON string)', () => { + const rawContent = () => + JSON.stringify({ + runnables: { + a: { type: 'path', runType: 'flow', path: 'u/admin/sync' }, + b: { type: 'path', runType: 'script', path: 'f/shared/calc' }, + c: { type: 'path', runType: 'hubscript', path: 'hub/1/keep' } + }, + files: { '/bundle.js': 'const conn = "$res:u/admin/pg"' } + }) + + it('extractRawAppRefs sees nested runnables and $res, skips hub', () => { + const refs = extractRawAppRefs(rawContent()) + expect(refs).toContainEqual({ kind: 'flow', path: 'u/admin/sync' }) + expect(refs).toContainEqual({ kind: 'script', path: 'f/shared/calc' }) + expect(refs).toContainEqual({ kind: 'resource', path: 'u/admin/pg' }) + expect(refs.some((r) => r.path === 'hub/1/keep')).toBe(false) + }) + + it('rewriteRawAppContent relocates nested runnable paths and $res', () => { + const map = new Map([ + ['u/admin/sync', 'f/proj/sync'], + ['f/shared/calc', 'f/proj/calc'], + ['u/admin/pg', 'f/proj/pg'] + ]) + const out = JSON.parse(rewriteRawAppContent(rawContent(), map)) + expect(out.runnables.a.path).toBe('f/proj/sync') + expect(out.runnables.b.path).toBe('f/proj/calc') + expect(out.runnables.c.path).toBe('hub/1/keep') + expect(out.files['/bundle.js']).toBe('const conn = "$res:f/proj/pg"') + }) + + it('falls back to $res scan on non-JSON content', () => { + expect(extractRawAppRefs('x = "$res:u/admin/pg"')).toContainEqual({ + kind: 'resource', + path: 'u/admin/pg' + }) + expect( + rewriteRawAppContent('x = "$res:u/admin/pg"', new Map([['u/admin/pg', 'f/proj/pg']])) + ).toBe('x = "$res:f/proj/pg"') + }) +}) + describe('buildProjectBundle', () => { // A flow that calls an external script which itself hardcodes a resource. const flow: FetchedItem = { diff --git a/frontend/src/lib/components/workspaceSettings/projectBundle.ts b/frontend/src/lib/components/workspaceSettings/projectBundle.ts index c364725e2f..3aad4a5036 100644 --- a/frontend/src/lib/components/workspaceSettings/projectBundle.ts +++ b/frontend/src/lib/components/workspaceSettings/projectBundle.ts @@ -1,20 +1,11 @@ -// Pure logic for the "project = folder" Hub bundle. -// -// A project is a single folder `f//...`. Bundling it means: -// 1. collect the transitive closure of what the folder references, -// 2. relocate anything referenced from OUTSIDE the folder into it -// (`u//` and `f//` -> `f//`, with -// `_2`, `_3`… on name collisions), -// 3. rewrite every reference to its new in-folder path. -// -// Hub references (`hub/...`) are external dependencies and are left untouched. -// Paths built dynamically at runtime (string concat) can't be detected and are -// out of scope by design. -// -// This module is intentionally free of API/Svelte deps so it can be unit-tested. -// The async closure orchestrator that fetches items lives in the component. +// Pure logic for the "project = folder" Hub bundle. A project is one folder +// `f//...`. Bundling: collect the transitive closure, relocate external +// refs (`u//`, `f//` -> `f//`, `_2`/`_3`… +// on collision) and rewrite them. Hub refs stay external; runtime string-concat +// paths are out of scope. No API/Svelte deps so it's unit-testable. import { getAllModules } from '$lib/components/flows/flowExplorer' +import { isRunnableByPath } from '$lib/components/apps/inputType' export type RefKind = 'resource' | 'script' | 'flow' @@ -90,18 +81,49 @@ export function extractFlowRefs(value: any): Ref[] { return out } +// Visit every object node in an app value tree (JSON-safe, no cycles). +function walkAppNodes(value: any, visit: (node: Record) => void): void { + if (value == null || typeof value !== 'object') return + if (Array.isArray(value)) { + for (const v of value) walkAppNodes(v, visit) + return + } + visit(value) + for (const k of Object.keys(value)) walkAppNodes(value[k], visit) +} + +// `runnableByPath`/`path` nodes reference a workspace runnable by path. +function runnableRef(node: Record): Ref | undefined { + if (!isRunnableByPath(node as any) || typeof node.path !== 'string') return undefined + if (node.runType === 'flow') return { kind: 'flow', path: node.path } + if (node.runType === 'script') return { kind: 'script', path: node.path } + return undefined // hubscript -> external hub, ignored +} + +// App refs: `$res:` resources anywhere in the value, plus script/flow runnables +// referenced by path in components. export function extractAppRefs(value: any): Ref[] { - return extractScriptRefs(JSON.stringify(value ?? {})) + const out: Ref[] = [] + const seen = new Set() + const add = (kind: RefKind, path: string) => { + const key = `${kind}:${path}` + if (!seen.has(key)) { + seen.add(key) + out.push({ kind, path }) + } + } + walkAppNodes(value, (node) => { + const r = runnableRef(node) + if (r) add(r.kind, r.path) + }) + for (const r of extractScriptRefs(JSON.stringify(value ?? {}))) add('resource', r.path) + return out } /** - * Build the relocation map. Paths already inside the project folder - * (`f//...`) map to themselves — their structure and subfolder depth are - * preserved. Only external paths (`u//` or `f//`) are - * relocated to `f//`; on collision the later entry gets a `_2`, - * `_3`… suffix. Internal paths are reserved first so an external one can never - * land on an occupied internal path. Input is sorted so suffix assignment is - * deterministic regardless of discovery order. + * Build the relocation map. Internal paths (`f//...`) map to themselves + * and are reserved first; external paths relocate to `f//` (`_2`/`_3`… + * on collision). Input is sorted so suffix assignment is deterministic. */ export function buildPathMap(paths: Iterable, slug: string): Map { const map = new Map() @@ -160,11 +182,38 @@ export function rewriteFlowValue(value: any, map: Map): any { return cloned } -// Round-trips through JSON since app values are opaque. -function rewriteAppValue(value: any, map: Map): any { +// Relocate `$res:` tokens (one round-trip, also produces a fresh clone) then +// runnable-by-path refs structurally. Incidental `f//` strings stay intact. +export function rewriteAppValue(value: any, map: Map): any { if (value == null) return value - const json = JSON.stringify(value) - return JSON.parse(rewriteContent(json, map)) + const cloned = JSON.parse(rewriteContent(JSON.stringify(value), map)) + walkAppNodes(cloned, (node) => { + if (runnableRef(node) && map.has(node.path)) node.path = map.get(node.path) + }) + return cloned +} + +// Raw/compiled apps store their structure as a JSON string (`{ runnables, files }`). +// Parse it so runnable-by-path refs in the runnables map are seen, reusing the +// same walk; fall back to plain `$res:` scanning if it isn't valid JSON. +export function extractRawAppRefs(content: string): Ref[] { + let parsed: any + try { + parsed = JSON.parse(content) + } catch { + return extractScriptRefs(content) + } + return extractAppRefs(parsed) +} + +export function rewriteRawAppContent(content: string, map: Map): string { + let parsed: any + try { + parsed = JSON.parse(content) + } catch { + return rewriteContent(content, map) + } + return JSON.stringify(rewriteAppValue(parsed, map)) } export type ItemKind = 'script' | 'flow' | 'app' | 'raw_app' @@ -221,7 +270,7 @@ function refsForFetched(item: FetchedItem): Ref[] { if (item.kind === 'script') return extractScriptRefs(item.content ?? '') if (item.kind === 'flow') return extractFlowRefs(item.value) if (item.kind === 'app') return extractAppRefs(item.value) - if (item.kind === 'raw_app') return extractScriptRefs(item.content ?? '') + if (item.kind === 'raw_app') return extractRawAppRefs(item.content ?? '') return [] } @@ -283,8 +332,10 @@ export async function buildProjectBundle( const items: BundledItem[] = itemPaths.map((path) => { const it = fetched.get(path)! const rewritten: BundledItem = { ...it, newPath: map.get(path) ?? path } - if (it.kind === 'script' || it.kind === 'raw_app') { + if (it.kind === 'script') { rewritten.content = rewriteContent(it.content ?? '', map) + } else if (it.kind === 'raw_app') { + rewritten.content = rewriteRawAppContent(it.content ?? '', map) } else if (it.kind === 'flow') { rewritten.value = rewriteFlowValue(it.value, map) } else if (it.kind === 'app') { diff --git a/frontend/src/routes/(root)/(logged)/projects/install/+page.svelte b/frontend/src/routes/(root)/(logged)/projects/install/+page.svelte index 2c0aca5c83..53aa6226a4 100644 --- a/frontend/src/routes/(root)/(logged)/projects/install/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/projects/install/+page.svelte @@ -13,6 +13,12 @@ FolderService } from '$lib/gen' import FolderPicker from '$lib/components/FolderPicker.svelte' + import { + rewriteAppValue, + rewriteContent, + rewriteFlowValue, + rewriteRawAppContent + } from '$lib/components/workspaceSettings/projectBundle' import { Cloud, Download, Loader2 } from 'lucide-svelte' type ExportItem = Record @@ -84,10 +90,67 @@ // Minimal non-public policy for re-created apps. const defaultPolicy = { execution_mode: 'publisher', triggerables_v2: {} } as any + // Map bundled paths `f//...` -> `f//...`. Only enumerated + // paths go in, so rewriters touch real refs, never incidental text. + function buildRetargetMap( + bundle: ProjectExport, + fromSlug: string, + folder: string + ): Map { + const map = new Map() + const prefix = `f/${fromSlug}/` + const add = (p: unknown) => { + if (typeof p === 'string' && p.startsWith(prefix)) { + map.set(p, `f/${folder}/${p.slice(prefix.length)}`) + } + } + for (const s of bundle.scripts) add(s.path) + for (const f of bundle.flows) add(f.path) + for (const a of bundle.apps) add(a.path) + for (const r of bundle.resources) add(r.path) + for (const t of bundle.triggers) { + add(t.path) + add(t.runnable_path) + } + return map + } + + // Structural retarget: rewrite each item's path and its internal refs, + // leaving Hub refs and arbitrary content untouched. function retarget(bundle: ProjectExport, fromSlug: string, folder: string): ProjectExport { if (folder === fromSlug) return bundle - const json = JSON.stringify(bundle).split(`f/${fromSlug}/`).join(`f/${folder}/`) - return JSON.parse(json) + const map = buildRetargetMap(bundle, fromSlug, folder) + const remap = (p: unknown) => (typeof p === 'string' ? (map.get(p) ?? p) : p) + return { + ...bundle, + scripts: bundle.scripts.map((s) => ({ + ...s, + path: remap(s.path), + content: rewriteContent(s.content ?? '', map) + })), + flows: bundle.flows.map((f) => ({ + ...f, + path: remap(f.path), + value: rewriteFlowValue(f.value, map) + })), + apps: bundle.apps.map((a) => ({ + ...a, + path: remap(a.path), + // Raw apps keep their structure in the `value.raw` JSON string. + value: + a.app_type === 'raw' + ? { ...a.value, raw: rewriteRawAppContent(a.value?.raw ?? '', map) } + : rewriteAppValue(a.value, map) + })), + resources: bundle.resources.map((r) => ({ ...r, path: remap(r.path) })), + triggers: bundle.triggers.map((t) => ({ + ...t, + path: remap(t.path), + runnable_path: remap(t.runnable_path), + // `$res:` refs can live in trigger args/config. + config: t.config ? JSON.parse(rewriteContent(JSON.stringify(t.config), map)) : t.config + })) + } } async function install() { @@ -269,8 +332,9 @@
- Resources are imported as empty stubs — set their values after import. Schedules are imported - disabled. + Resources are imported as empty stubs — set their values after import; a resource whose path + already exists is reported as failed (existing values are never overwritten). Only schedule + triggers are recreated (imported disabled); other trigger kinds are skipped.
diff --git a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte index 4e56fa576c..aa873f83a9 100644 --- a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte @@ -30,12 +30,10 @@ enterpriseLicense, superadmin, userStore, - userWorkspaces, usersWorkspaceStore, workspaceStore, isCriticalAlertsUIOpen } from '$lib/stores' - import { switchWorkspace } from '$lib/storeUtils' import { sendUserToast } from '$lib/toast' import { clone, emptyString, encodeState, hasUnsavedChanges } from '$lib/utils' import { downloadViaClient, shouldDownloadViaClient } from '$lib/utils/downloadFile' @@ -1544,32 +1542,11 @@ unifiedSize="md" btnClasses="mt-2" on:click={async () => { - const ws = $workspaceStore ?? '' - // Land on the parent workspace if this is a fork and the - // parent is still accessible — otherwise fall back to the - // workspace picker. - const parentId = $userWorkspaces.find((w) => w.id === ws)?.parent_workspace_id - const parentStillAccessible = !!( - parentId && $userWorkspaces.find((w) => w.id === parentId) - ) - await WorkspaceService.archiveWorkspace({ workspace: ws }) - sendUserToast(`Archived workspace ${ws}`) - if (parentStillAccessible && parentId) { - // Refresh the list so the just-archived workspace drops out before - // we land on the parent. Guarded: a refresh failure must not block - // the switch (the list reloads on next page load). - try { - usersWorkspaceStore.set(await WorkspaceService.listUserWorkspaces()) - } catch (e) { - console.error('Failed to refresh workspaces after archive', e) - } - switchWorkspace(parentId) - await goto('/') - } else { - workspaceStore.set(undefined) - usersWorkspaceStore.set(undefined) - await goto('/user/workspaces') - } + await WorkspaceService.archiveWorkspace({ workspace: $workspaceStore ?? '' }) + sendUserToast(`Archived workspace ${$workspaceStore}`) + workspaceStore.set(undefined) + usersWorkspaceStore.set(undefined) + goto('/user/workspaces') }} > Archive workspace @@ -1889,8 +1866,8 @@ export async function main( {:else if tab == 'default_app'} {#if !$enterpriseLicense} @@ -1903,11 +1880,7 @@ export async function main( before turning this feature on. {/if} - +