feat(editor): Change theme only after clicking save

For consistent UX, don't commit the selected theme until user
clicks save. Otherwise the UI behaves differently depending
which input elements are touched.

packages/editor-ui/src/views/SettingsPersonalView.vue

@@ -84,12 +84,11 @@
 			<div>
 				<n8n-input-label :label="i18n.baseText('settings.personal.theme')">
 					<n8n-select
+						v-model="currentSelectedTheme"
 						:class="$style.themeSelect"
 						data-test-id="theme-select"
 						size="small"
-						:model-value="currentTheme"
 						filterable
-						@update:model-value="selectTheme"
 					>
 						<n8n-option
 							v-for="item in themeOptions"
@@ -139,11 +138,12 @@ export default defineComponent({
 	},
 	data() {
 		return {
-			hasAnyChanges: false,
+			hasAnyBasicInfoChanges: false,
 			formInputs: null as null | IFormInputs,
 			formBus: createEventBus(),
 			readyToSubmit: false,
 			mfaDocsUrl: MFA_DOCS_URL,
+			currentSelectedTheme: useUIStore().theme,
 			themeOptions: [
 				{
 					name: 'system',
@@ -160,6 +160,34 @@ export default defineComponent({
 			] as Array<{ name: ThemeOption; label: string }>,
 		};
 	},
+	computed: {
+		...mapStores(useUIStore, useUsersStore, useSettingsStore),
+		currentUser(): IUser | null {
+			return this.usersStore.currentUser;
+		},
+		isExternalAuthEnabled(): boolean {
+			const isLdapEnabled =
+				this.settingsStore.settings.enterprise.ldap && this.currentUser?.signInType === 'ldap';
+			const isSamlEnabled =
+				this.settingsStore.isSamlLoginEnabled && this.settingsStore.isDefaultAuthenticationSaml;
+			return isLdapEnabled || isSamlEnabled;
+		},
+		isPersonalSecurityEnabled(): boolean {
+			return this.usersStore.isInstanceOwner || !this.isExternalAuthEnabled;
+		},
+		mfaDisabled(): boolean {
+			return !this.usersStore.mfaEnabled;
+		},
+		isMfaFeatureEnabled(): boolean {
+			return this.settingsStore.isMfaFeatureEnabled;
+		},
+		hasAnyPersonalisationChanges(): boolean {
+			return this.currentSelectedTheme !== this.uiStore.theme;
+		},
+		hasAnyChanges() {
+			return this.hasAnyBasicInfoChanges || this.hasAnyPersonalisationChanges;
+		},
+	},
 	mounted() {
 		this.formInputs = [
 			{
@@ -201,62 +229,46 @@ export default defineComponent({
 			},
 		];
 	},
-	computed: {
-		...mapStores(useUIStore, useUsersStore, useSettingsStore),
-		currentUser(): IUser | null {
-			return this.usersStore.currentUser;
-		},
-		isExternalAuthEnabled(): boolean {
-			const isLdapEnabled =
-				this.settingsStore.settings.enterprise.ldap && this.currentUser?.signInType === 'ldap';
-			const isSamlEnabled =
-				this.settingsStore.isSamlLoginEnabled && this.settingsStore.isDefaultAuthenticationSaml;
-			return isLdapEnabled || isSamlEnabled;
-		},
-		isPersonalSecurityEnabled(): boolean {
-			return this.usersStore.isInstanceOwner || !this.isExternalAuthEnabled;
-		},
-		mfaDisabled(): boolean {
-			return !this.usersStore.mfaEnabled;
-		},
-		isMfaFeatureEnabled(): boolean {
-			return this.settingsStore.isMfaFeatureEnabled;
-		},
-		currentTheme(): ThemeOption {
-			return this.uiStore.theme;
-		},
-	},
 	methods: {
-		selectTheme(theme: ThemeOption) {
-			this.uiStore.setTheme(theme);
-		},
 		onInput() {
-			this.hasAnyChanges = true;
+			this.hasAnyBasicInfoChanges = true;
 		},
 		onReadyToSubmit(ready: boolean) {
 			this.readyToSubmit = ready;
 		},
 		async onSubmit(form: { firstName: string; lastName: string; email: string }) {
-			if (!this.hasAnyChanges || !this.usersStore.currentUserId) {
-				return;
-			}
 			try {
-				await this.usersStore.updateUser({
-					id: this.usersStore.currentUserId,
-					firstName: form.firstName,
-					lastName: form.lastName,
-					email: form.email,
-				});
+				await Promise.all([this.updateUserBasicInfo(form), this.updatePersonalisationSettings()]);
+
 				this.showToast({
 					title: this.i18n.baseText('settings.personal.personalSettingsUpdated'),
 					message: '',
 					type: 'success',
 				});
-				this.hasAnyChanges = false;
 			} catch (e) {
 				this.showError(e, this.i18n.baseText('settings.personal.personalSettingsUpdatedError'));
 			}
 		},
+		async updateUserBasicInfo(form: { firstName: string; lastName: string; email: string }) {
+			if (!this.hasAnyBasicInfoChanges || !this.usersStore.currentUserId) {
+				return;
+			}
+
+			await this.usersStore.updateUser({
+				id: this.usersStore.currentUserId,
+				firstName: form.firstName,
+				lastName: form.lastName,
+				email: form.email,
+			});
+			this.hasAnyBasicInfoChanges = false;
+		},
+		async updatePersonalisationSettings() {
+			if (!this.hasAnyPersonalisationChanges) {
+				return;
+			}
+
+			this.uiStore.setTheme(this.currentSelectedTheme);
+		},
 		onSaveClick() {
 			this.formBus.emit('submit');
 		},

packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts

@@ -6,10 +6,12 @@ import { useUsersStore } from '@/stores/users.store';
 import { createComponentRenderer } from '@/__tests__/render';
 import { setupServer } from '@/__tests__/server';
 import { ROLE } from '@/constants';
+import { useUIStore } from '@/stores/ui.store';
 
 let pinia: ReturnType<typeof createPinia>;
 let settingsStore: ReturnType<typeof useSettingsStore>;
 let usersStore: ReturnType<typeof useUsersStore>;
+let uiStore: ReturnType<typeof useUIStore>;
 let server: ReturnType<typeof setupServer>;
 
 const renderComponent = createComponentRenderer(SettingsPersonalView);
@@ -37,6 +39,7 @@ describe('SettingsPersonalView', () => {
 
 		settingsStore = useSettingsStore(pinia);
 		usersStore = useUsersStore(pinia);
+		uiStore = useUIStore(pinia);
 
 		usersStore.users[currentUser.id] = currentUser;
 		usersStore.currentUserId = currentUser.id;
@@ -56,6 +59,53 @@ describe('SettingsPersonalView', () => {
 		expect(getByTestId('change-password-link')).toBeInTheDocument();
 	});
 
+	describe('when changing theme', () => {
+		it('should disable save button when theme has not been changed', async () => {
+			const { getByTestId } = renderComponent({ pinia });
+			await waitAllPromises();
+
+			expect(getByTestId('save-settings-button')).toBeDisabled();
+		});
+
+		it('should enable save button when theme is changed', async () => {
+			const { getByTestId, getByPlaceholderText, findByText } = renderComponent({ pinia });
+			await waitAllPromises();
+
+			getByPlaceholderText('Select').click();
+			const darkThemeOption = await findByText('Dark theme');
+			darkThemeOption.click();
+
+			await waitAllPromises();
+			expect(getByTestId('save-settings-button')).toBeEnabled();
+		});
+
+		it('should not update theme after changing the selected theme', async () => {
+			const { getByPlaceholderText, findByText } = renderComponent({ pinia });
+			await waitAllPromises();
+
+			getByPlaceholderText('Select').click();
+			const darkThemeOption = await findByText('Dark theme');
+			darkThemeOption.click();
+
+			await waitAllPromises();
+			expect(uiStore.theme).toBe('system');
+		});
+
+		it('should commit the theme change after clicking save', async () => {
+			const { getByPlaceholderText, findByText, getByTestId } = renderComponent({ pinia });
+			await waitAllPromises();
+
+			getByPlaceholderText('Select').click();
+			const darkThemeOption = await findByText('Dark theme');
+			darkThemeOption.click();
+
+			await waitAllPromises();
+
+			getByTestId('save-settings-button').click();
+			expect(uiStore.theme).toBe('dark');
+		});
+	});
+
 	describe('when external auth is enabled, email and password change', () => {
 		beforeEach(() => {
 			vi.spyOn(settingsStore, 'isSamlLoginEnabled', 'get').mockReturnValue(true);