From 0ff65550f71cdda05cb620b992094358da41eb3a Mon Sep 17 00:00:00 2001 From: Guilhem Date: Wed, 18 Feb 2026 21:12:20 +0000 Subject: [PATCH] improve instance settings drawer UX (#8002) * fix(frontend): prevent false dirty state in instance settings on load Co-Authored-By: Claude Opus 4.6 * fix(frontend): handle undefined python version in select binding Co-Authored-By: Claude Opus 4.6 * refactor(frontend): extract SaveButton component and improve drawer header UX Co-Authored-By: Claude Opus 4.6 * refactor(frontend): replace inline diff with diff drawer and simplify save flow Save now saves immediately instead of requiring a two-step confirm flow. Diff view opens in a separate drawer with split/unified toggle instead of replacing the form content inline. Co-Authored-By: Claude Opus 4.6 * fix(frontend): preserve dirty state when toggling YAML mode in instance settings syncFormToYaml() was setting yamlCodeInitial to the current modified YAML, causing hasUnsavedChanges to become false when entering YAML mode with pending form changes. Build yamlCodeInitial from initialValues instead. Co-Authored-By: Claude Opus 4.6 * fix(frontend): clear dirty state after saving in YAML mode Co-Authored-By: Claude Opus 4.5 * reduce save button timeout * feat(frontend): add review changes button to unsaved changes confirmation modal Co-Authored-By: Claude Opus 4.5 * fix(frontend): address code review issues from PR #8002 Remove unnecessary IIFE wrappers in handleSave/handleSaveAndCloseDiff, fix stale on:close reference on diff drawer, clip SaveButton overlay with overflow-hidden, make DiffEditor respond reactively to inlineDiff prop instead of using {#key} destroy/recreate, and revert normalizeValue object check to original simpler behavior. Co-Authored-By: Claude Opus 4.6 * fix(frontend): remove tab-switch confirmation modal in full settings mode In full mode, the save button saves all settings across all categories, so switching tabs cannot lose unsaved changes. Remove the per-category dirty check, confirmation modal, and unused ConfirmationModal import. Co-Authored-By: Claude Opus 4.6 * fix(frontend): prevent SMTP toggles from creating false dirty state Use getter/setter bind:checked so Toggle reads undefined as false without writing it back to the store. This prevents visiting the SMTP tab from mutating smtp_settings and triggering a false unsaved diff. Co-Authored-By: Claude Opus 4.6 * fix(frontend): prevent OTEL toggles from creating false dirty state Same fix as SMTP toggles: use getter/setter bind:checked so Toggle reads undefined as false without writing it back to the store. Co-Authored-By: Claude Opus 4.6 * refactor(frontend): use recursive normalizeValue for dirty state instead of per-component fixes Co-Authored-By: Claude Opus 4.5 * refactor(frontend): replace save button with always-visible review changes button Co-Authored-By: Claude Opus 4.5 * fix(frontend): address PR review comments on DiffEditor and SaveButton Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.6 --- frontend/src/lib/components/DiffEditor.svelte | 12 +- .../src/lib/components/InstanceSetting.svelte | 6 +- .../lib/components/InstanceSettings.svelte | 104 ++++++++------- frontend/src/lib/components/SaveButton.svelte | 99 ++++++++++++++ .../lib/components/SuperadminSettings.svelte | 126 +++++++++--------- .../components/SuperadminSettingsInner.svelte | 10 +- .../src/lib/components/instanceSettings.ts | 2 +- .../workspaceSettings/SettingsFooter.svelte | 89 ++----------- .../(user)/instance_settings/+page.svelte | 44 +----- 9 files changed, 251 insertions(+), 241 deletions(-) create mode 100644 frontend/src/lib/components/SaveButton.svelte diff --git a/frontend/src/lib/components/DiffEditor.svelte b/frontend/src/lib/components/DiffEditor.svelte index 4015a7a9ac..277cc66bda 100644 --- a/frontend/src/lib/components/DiffEditor.svelte +++ b/frontend/src/lib/components/DiffEditor.svelte @@ -169,12 +169,6 @@ open = false } - function onWidthChange(editorWidth: number) { - diffEditor?.updateOptions({ - renderSideBySide: inlineDiff ? false : editorWidth >= SIDE_BY_SIDE_MIN_WIDTH - }) - } - $effect(() => { if (open && diffDivEl) { loadDiffEditor() @@ -182,7 +176,11 @@ }) $effect(() => { - onWidthChange(editorWidth) + if (diffEditor) { + diffEditor.updateOptions({ + renderSideBySide: inlineDiff ? false : editorWidth >= SIDE_BY_SIDE_MIN_WIDTH + }) + } }) onMount(() => { diff --git a/frontend/src/lib/components/InstanceSetting.svelte b/frontend/src/lib/components/InstanceSetting.svelte index b493469139..1780bc1d80 100644 --- a/frontend/src/lib/components/InstanceSetting.svelte +++ b/frontend/src/lib/components/InstanceSetting.svelte @@ -208,7 +208,9 @@ ee_only={setting.ee_only} settingKey={setting.key} > - + $values[setting.key] ?? 'default', (v) => ($values[setting.key] = v)} + > {#snippet children({ item: toggleButton })} {#each setting.select_items ?? [] as item} diff --git a/frontend/src/lib/components/InstanceSettings.svelte b/frontend/src/lib/components/InstanceSettings.svelte index 4753c532b4..9c8e809676 100644 --- a/frontend/src/lib/components/InstanceSettings.svelte +++ b/frontend/src/lib/components/InstanceSettings.svelte @@ -30,7 +30,6 @@ onNavigateToTab?: (category: string) => void quickSetup?: boolean yamlMode?: boolean - diffMode?: boolean hasUnsavedChanges?: boolean } @@ -42,7 +41,6 @@ onNavigateToTab, quickSetup = false, yamlMode = $bindable(false), - diffMode = $bindable(false), hasUnsavedChanges = $bindable(false) }: Props = $props() @@ -86,7 +84,8 @@ function applyFormDefaults(vals: Record): void { for (const [key, defaultVal] of Object.entries(formDefaults)) { if (vals[key] == undefined) { - vals[key] = typeof defaultVal === 'object' ? JSON.parse(JSON.stringify(defaultVal)) : defaultVal + vals[key] = + typeof defaultVal === 'object' ? JSON.parse(JSON.stringify(defaultVal)) : defaultVal } } } @@ -111,6 +110,19 @@ } applyFormDefaults(nvalues) + // Apply select/select_python defaults so initialValues matches what InstanceSetting's $effect does + for (const category of settingsKeys) { + for (const s of settings[category]) { + if ( + (s.fieldType === 'select' || s.fieldType === 'select_python') && + nvalues[s.key] == undefined && + s.defaultValue + ) { + nvalues[s.key] = s.defaultValue() + } + } + } + // Snapshot initialValues before applying the base_url fallback so that // the dirty-check detects the unsaved default and enables the Save button. initialValues = JSON.parse(JSON.stringify(nvalues)) @@ -201,6 +213,10 @@ initialRequirePreexistingUserForOauth = requirePreexistingUserForOauth baseUrlIsFallback = false + if (yamlMode) { + yamlCodeInitial = yamlCode + } + if (licenseKeySet) { setLicense() } @@ -333,11 +349,20 @@ if (value === false) return undefined if (typeof value === 'string' && value.trim() === '') return undefined if (Array.isArray(value) && value.length === 0) return undefined - if (typeof value === 'object' && Object.keys(value).length === 0) return undefined + if (typeof value === 'object' && !Array.isArray(value)) { + // Recursively normalize: if all values in the object normalize to undefined, + // the object itself is effectively empty (e.g. {smtp_tls_implicit: false} ≡ {}) + const hasNonEmpty = Object.values(value).some((v) => normalizeValue(v) !== undefined) + if (!hasNonEmpty) return undefined + } // Key-specific defaults: these values are equivalent to "not set" if (key === 'secret_backend') { - if (typeof value === 'object' && value?.type === 'Database' && Object.keys(value).length === 1) { + if ( + typeof value === 'object' && + value?.type === 'Database' && + Object.keys(value).length === 1 + ) { return undefined } } @@ -658,7 +683,12 @@ normalize: true, mask: !showSensitive }) - yamlCodeInitial = yamlCode + yamlCodeInitial = buildSettingsYaml( + initialValues, + initialOauths, + initialRequirePreexistingUserForOauth, + { normalize: true, mask: !showSensitive } + ) yamlEditor?.setCode(yamlCode) yamlError = '' } @@ -797,40 +827,25 @@
- {#if diffMode} -
- {#await import('$lib/components/DiffEditor.svelte')} - - {:then Module} - {@const diff = buildFullDiff()} - +

+ Use this YAML to manage instance settings as code. + Learn more +

+ +
+ handleShowSensitiveToggle(e.detail)} + options={{ right: 'Show sensitive values' }} + size="xs" /> - {/await} -
- {:else if yamlMode} -

- Use this YAML to manage instance settings as code. - Learn more -

- -
- handleShowSensitiveToggle(e.detail)} - options={{ right: 'Show sensitive values' }} - size="xs" - /> +
{#await import('$lib/components/SimpleEditor.svelte')} @@ -887,11 +902,7 @@ link="https://www.windmill.dev/docs/advanced/imports" /> {#if !$enterpriseLicense} - + {/if} {:else if category == 'Alerts'} {/if} {#if quickSetup && category === 'Core' && setting.key === 'base_url'} @@ -1076,4 +1089,5 @@ /> {/if} {/snippet} +
diff --git a/frontend/src/lib/components/SaveButton.svelte b/frontend/src/lib/components/SaveButton.svelte new file mode 100644 index 0000000000..7c89c2e196 --- /dev/null +++ b/frontend/src/lib/components/SaveButton.svelte @@ -0,0 +1,99 @@ + + +
+ + + {#if saveStatus === 'success'} +
+ +
+ {:else if saveStatus === 'error'} +
+ +
+ {/if} +
diff --git a/frontend/src/lib/components/SuperadminSettings.svelte b/frontend/src/lib/components/SuperadminSettings.svelte index 1600de6e0a..f9b446b04e 100644 --- a/frontend/src/lib/components/SuperadminSettings.svelte +++ b/frontend/src/lib/components/SuperadminSettings.svelte @@ -5,7 +5,9 @@ import MeltTooltip from './meltComponents/Tooltip.svelte' import Toggle from './Toggle.svelte' import ConfirmationModal from './common/confirmationModal/ConfirmationModal.svelte' - import { X, FileDiff, Save, Loader2 } from 'lucide-svelte' + import SaveButton from './SaveButton.svelte' + import { X, FileDiff, Loader2 } from 'lucide-svelte' + import { fade } from 'svelte/transition' import { SettingsService } from '$lib/gen' import { isCloudHosted } from '$lib/cloud' @@ -16,14 +18,14 @@ let { disableChatOffset = false }: Props = $props() let drawer: Drawer | undefined = $state() + let diffDrawer: Drawer | undefined = $state() let innerComponent: SuperadminSettingsInner | undefined = $state() let uptodateVersion: string | undefined = $state(undefined) let yamlMode = $state(false) - let diffMode = $state(false) let hasUnsavedChanges = $state(false) - let pendingSave = $state(false) - let isSaving = $state(false) let showCloseConfirmModal = $state(false) + let diffData: { original: string; modified: string } = $state({ original: '', modified: '' }) + let inlineDiff = $state(false) async function loadUptodate() { try { @@ -65,39 +67,24 @@ bypassCloseCheck = false } - async function handleSave() { - if (!pendingSave) { - if (!innerComponent?.syncBeforeDiff()) return - diffMode = true - pendingSave = true - return - } - isSaving = true - try { - await innerComponent?.saveSettings() - diffMode = false - pendingSave = false - } catch (e) { - console.error('Save failed:', e) - } finally { - isSaving = false - } + async function handleSave(): Promise { + if (!innerComponent?.syncBeforeDiff()) throw new Error('YAML sync failed') + await innerComponent?.saveSettings() + } + + async function handleSaveAndCloseDiff(): Promise { + await handleSave() + diffDrawer?.closeDrawer() } function handleDiscard() { innerComponent?.discardAll() - diffMode = false - pendingSave = false } - function handleShowDiff() { - if (!diffMode) { - if (!innerComponent?.syncBeforeDiff()) return - } - diffMode = !diffMode - if (!diffMode) { - pendingSave = false - } + function handleReviewChanges() { + if (!innerComponent?.syncBeforeDiff()) return + diffData = innerComponent?.buildFullDiff() ?? { original: '', modified: '' } + diffDrawer?.openDrawer() } @@ -124,41 +111,32 @@ {/snippet} {#snippet actions()}
+ {#if hasUnsavedChanges} +
+ +
+ {/if} - -
{/snippet} + + diffDrawer?.closeDrawer()}> + {#snippet actions()} + + + {/snippet} + +
+ {#await import('$lib/components/DiffEditor.svelte')} + + {:then Module} + + {/await} +
+ + + {#if showCloseConfirmModal} { innerComponent?.discardAll() showCloseConfirmModal = false - diffMode = false - pendingSave = false closeDrawer() }} > @@ -192,14 +196,14 @@ You have unsaved changes. Are you sure you want to discard them and close?
diff --git a/frontend/src/lib/components/SuperadminSettingsInner.svelte b/frontend/src/lib/components/SuperadminSettingsInner.svelte index 22883567bb..4d6b0e33f1 100644 --- a/frontend/src/lib/components/SuperadminSettingsInner.svelte +++ b/frontend/src/lib/components/SuperadminSettingsInner.svelte @@ -46,7 +46,6 @@ closeDrawer, showHeaderInfo = true, yamlMode = $bindable(false), - diffMode = $bindable(false), hasUnsavedChanges = $bindable(false) } = $props() @@ -142,6 +141,9 @@ return instanceSettings?.syncBeforeDiff() ?? true } + export function buildFullDiff(): { original: string; modified: string } { + return instanceSettings?.buildFullDiff() ?? { original: '', modified: '' } + } // --- Settings search --- const searchableItems = buildSearchableSettingItems() @@ -203,7 +205,7 @@ {/if} {/if}
- {#if !yamlMode && !diffMode} + {#if !yamlMode}
@@ -231,7 +233,7 @@
- {#if tab === 'users' && !yamlMode && !diffMode} + {#if tab === 'users' && !yamlMode}
{#if !automateUsernameCreation && !isCloudHosted()}
@@ -507,7 +509,6 @@ bind:this={instanceSettings} hideTabs bind:yamlMode - bind:diffMode bind:hasUnsavedChanges tab={instanceSettingsCategory} {authSubTab} @@ -543,4 +544,3 @@ Are you sure you want to remove {deleteUserEmail}?
- diff --git a/frontend/src/lib/components/instanceSettings.ts b/frontend/src/lib/components/instanceSettings.ts index 287d7aa9a2..b336b11fca 100644 --- a/frontend/src/lib/components/instanceSettings.ts +++ b/frontend/src/lib/components/instanceSettings.ts @@ -181,7 +181,7 @@ export const settings: Record = { ], Jobs: [ { - label: 'Job Isolation', + label: 'Job isolation', key: 'job_isolation', fieldType: 'select', description: diff --git a/frontend/src/lib/components/workspaceSettings/SettingsFooter.svelte b/frontend/src/lib/components/workspaceSettings/SettingsFooter.svelte index afff676149..bebd117dee 100644 --- a/frontend/src/lib/components/workspaceSettings/SettingsFooter.svelte +++ b/frontend/src/lib/components/workspaceSettings/SettingsFooter.svelte @@ -1,7 +1,8 @@
Discard changes
{/if} -
- - - - {#if saveStatus === 'success'} -
- -
- {:else if saveStatus === 'error'} -
- -
- {/if} -
+
diff --git a/frontend/src/routes/(root)/(logged)/user/(user)/instance_settings/+page.svelte b/frontend/src/routes/(root)/(logged)/user/(user)/instance_settings/+page.svelte index 772480d0de..07b821fe04 100644 --- a/frontend/src/routes/(root)/(logged)/user/(user)/instance_settings/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/user/(user)/instance_settings/+page.svelte @@ -5,7 +5,6 @@ import InstanceSettings from '$lib/components/InstanceSettings.svelte' import { Alert, Button } from '$lib/components/common' import SidebarNavigation from '$lib/components/common/sidebar/SidebarNavigation.svelte' - import ConfirmationModal from '$lib/components/common/confirmationModal/ConfirmationModal.svelte' import { setupNavigationGroups, tabToCategoryMap, @@ -25,6 +24,7 @@ import Toggle from '$lib/components/Toggle.svelte' import SettingsPageHeader from '$lib/components/settings/SettingsPageHeader.svelte' import SettingCard from '$lib/components/instanceSettings/SettingCard.svelte' + import ConfirmationModal from '$lib/components/common/confirmationModal/ConfirmationModal.svelte' const settingsSteps = [ { id: 'Core', label: 'Core' }, @@ -148,19 +148,9 @@ let authSubTab: 'sso' | 'oauth' | 'scim' = $derived(tabToAuthSubTab[fullTab] ?? 'sso') let yamlMode = $state(false) - // --- Unsaved changes detection (full mode) --- - let pendingTab: string | undefined = $state(undefined) - let showUnsavedChangesModal = $state(false) - function handleNavigate(newTab: string) { if (newTab === fullTab) return - const currentCategory = tabToCategoryMap[fullTab] - if (currentCategory && instanceSettings?.isDirty(currentCategory)) { - pendingTab = newTab - showUnsavedChangesModal = true - } else { - fullTab = newTab - } + fullTab = newTab } // --- Settings search (full mode) --- @@ -558,33 +548,6 @@
-{#if showUnsavedChangesModal} - { - showUnsavedChangesModal = false - pendingTab = undefined - }} - on:confirmed={() => { - if (pendingTab !== undefined) { - const currentCategory = tabToCategoryMap[fullTab] - if (currentCategory) { - instanceSettings?.discardCategory(currentCategory) - } - fullTab = pendingTab - } - showUnsavedChangesModal = false - pendingTab = undefined - }} - > -
- You have unsaved changes. Are you sure you want to discard them? -
- -{/if} - {#if showLicenseKeyWarning}
- You are running the Enterprise Edition image but have not entered a license key. A valid license key is required to use EE features. Are you sure you want to continue without one? + You are running the Enterprise Edition image but have not entered a license key. A valid + license key is required to use EE features. Are you sure you want to continue without one?