diff --git a/packages/cli/src/utils/settingsUtils.test.ts b/packages/cli/src/utils/settingsUtils.test.ts index 035d34e64c..dded8ac686 100644 --- a/packages/cli/src/utils/settingsUtils.test.ts +++ b/packages/cli/src/utils/settingsUtils.test.ts @@ -7,33 +7,20 @@ import { describe, it, expect } from 'vitest'; import { // Schema utilities - getSettingsByCategory, getSettingDefinition, requiresRestart, getDefaultValue, getRestartRequiredSettings, getEffectiveValue, getAllSettingKeys, - getSettingsByType, - getSettingsRequiringRestart, - isValidSettingKey, - getSettingCategory, - shouldShowInDialog, - getDialogSettingsByCategory, - getDialogSettingsByType, getDialogSettingKeys, // Business logic utilities - getSettingValue, - isSettingModified, TEST_ONLY, settingExistsInScope, setPendingSettingValue, - hasRestartRequiredSettings, getRestartRequiredFromModified, getDisplayValue, isDefaultValue, - isValueInherited, - getEffectiveDisplayValue, setNestedPropertySafe, setNestedPropertyForce, validateSettingValue, @@ -212,24 +199,6 @@ describe('SettingsUtils', () => { }); describe('Schema Utilities', () => { - describe('getSettingsByCategory', () => { - it('should group settings by category', () => { - const categories = getSettingsByCategory(); - expect(categories).toHaveProperty('Advanced'); - expect(categories).toHaveProperty('Basic'); - }); - - it('should include key property in grouped settings', () => { - const categories = getSettingsByCategory(); - - Object.entries(categories).forEach(([_category, settings]) => { - settings.forEach((setting) => { - expect(setting.key).toBeDefined(); - }); - }); - }); - }); - describe('getSettingDefinition', () => { it('should return definition for valid setting', () => { const definition = getSettingDefinition('ui.theme'); @@ -397,133 +366,6 @@ describe('SettingsUtils', () => { }); }); - describe('getSettingsByType', () => { - it('should return only boolean settings', () => { - const booleanSettings = getSettingsByType('boolean'); - expect(booleanSettings.length).toBeGreaterThan(0); - booleanSettings.forEach((setting) => { - expect(setting.type).toBe('boolean'); - }); - }); - }); - - describe('getSettingsRequiringRestart', () => { - it('should return only settings that require restart', () => { - const restartSettings = getSettingsRequiringRestart(); - expect(restartSettings.length).toBeGreaterThan(0); - restartSettings.forEach((setting) => { - expect(setting.requiresRestart).toBe(true); - }); - }); - }); - - describe('isValidSettingKey', () => { - it('should return true for valid setting keys', () => { - expect(isValidSettingKey('ui.requiresRestart')).toBe(true); - expect(isValidSettingKey('ui.accessibility.enableLoadingPhrases')).toBe( - true, - ); - }); - - it('should return false for invalid setting keys', () => { - expect(isValidSettingKey('invalidSetting')).toBe(false); - expect(isValidSettingKey('')).toBe(false); - }); - }); - - describe('getSettingCategory', () => { - it('should return correct category for valid settings', () => { - expect(getSettingCategory('ui.requiresRestart')).toBe('UI'); - expect( - getSettingCategory('ui.accessibility.enableLoadingPhrases'), - ).toBe('UI'); - }); - - it('should return undefined for invalid settings', () => { - expect(getSettingCategory('invalidSetting')).toBeUndefined(); - }); - }); - - describe('shouldShowInDialog', () => { - it('should return true for settings marked to show in dialog', () => { - expect(shouldShowInDialog('ui.requiresRestart')).toBe(true); - expect(shouldShowInDialog('general.vimMode')).toBe(true); - expect(shouldShowInDialog('ui.hideWindowTitle')).toBe(true); - }); - - it('should return false for settings marked to hide from dialog', () => { - expect(shouldShowInDialog('ui.theme')).toBe(false); - }); - - it('should return true for invalid settings (default behavior)', () => { - expect(shouldShowInDialog('invalidSetting')).toBe(true); - }); - }); - - describe('getDialogSettingsByCategory', () => { - it('should only return settings marked for dialog display', async () => { - const categories = getDialogSettingsByCategory(); - - // Should include UI settings that are marked for dialog - expect(categories['UI']).toBeDefined(); - const uiSettings = categories['UI']; - const uiKeys = uiSettings.map((s) => s.key); - expect(uiKeys).toContain('ui.requiresRestart'); - expect(uiKeys).toContain('ui.accessibility.enableLoadingPhrases'); - expect(uiKeys).not.toContain('ui.theme'); // This is now marked false - }); - - it('should not include Advanced category settings', () => { - const categories = getDialogSettingsByCategory(); - - // Advanced settings should be filtered out - expect(categories['Advanced']).toBeUndefined(); - }); - - it('should include settings with showInDialog=true', () => { - const categories = getDialogSettingsByCategory(); - - const allSettings = Object.values(categories).flat(); - const allKeys = allSettings.map((s) => s.key); - - expect(allKeys).toContain('test'); - expect(allKeys).toContain('ui.requiresRestart'); - expect(allKeys).not.toContain('ui.theme'); // Now hidden - expect(allKeys).not.toContain('general.preferredEditor'); // Now hidden - }); - }); - - describe('getDialogSettingsByType', () => { - it('should return only boolean dialog settings', () => { - const booleanSettings = getDialogSettingsByType('boolean'); - - const keys = booleanSettings.map((s) => s.key); - expect(keys).toContain('ui.requiresRestart'); - expect(keys).toContain('ui.accessibility.enableLoadingPhrases'); - expect(keys).not.toContain('privacy.usageStatisticsEnabled'); - expect(keys).not.toContain('security.auth.selectedType'); // Advanced setting - expect(keys).not.toContain('security.auth.useExternal'); // Advanced setting - }); - - it('should return only string dialog settings', () => { - const stringSettings = getDialogSettingsByType('string'); - - const keys = stringSettings.map((s) => s.key); - // Note: theme and preferredEditor are now hidden from dialog - expect(keys).not.toContain('ui.theme'); // Now marked false - expect(keys).not.toContain('general.preferredEditor'); // Now marked false - expect(keys).not.toContain('security.auth.selectedType'); // Advanced setting - - // Check that user-facing tool settings are included - expect(keys).toContain('tools.shell.pager'); - - // Check that advanced/hidden tool settings are excluded - expect(keys).not.toContain('tools.discoveryCommand'); - expect(keys).not.toContain('tools.callCommand'); - expect(keys.every((key) => !key.startsWith('advanced.'))).toBe(true); - }); - }); - describe('getDialogSettingKeys', () => { it('should return only settings marked for dialog display', () => { const dialogKeys = getDialogSettingKeys(); @@ -601,7 +443,7 @@ describe('SettingsUtils', () => { expect(existsInPending).toBe(true); // Get the value from pending settings - const valueFromPending = getSettingValue( + const valueFromPending = getEffectiveValue( key, updatedPendingSettings, {}, @@ -633,64 +475,6 @@ describe('SettingsUtils', () => { }); describe('Business Logic Utilities', () => { - describe('getSettingValue', () => { - it('should return value from settings when set', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: false }, - }); - - const value = getSettingValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(value).toBe(true); - }); - - it('should return value from merged settings when not set in current scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const value = getSettingValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(value).toBe(true); - }); - - it('should return default value for invalid setting', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({}); - - const value = getSettingValue( - 'invalidSetting', - settings, - mergedSettings, - ); - expect(value).toBe(false); // Default fallback - }); - }); - - describe('isSettingModified', () => { - it('should return true when value differs from default', () => { - expect(isSettingModified('ui.requiresRestart', true)).toBe(true); - expect( - isSettingModified('ui.accessibility.enableLoadingPhrases', true), - ).toBe(true); - }); - - it('should return false when value matches default', () => { - expect(isSettingModified('ui.requiresRestart', false)).toBe(false); - expect( - isSettingModified('ui.accessibility.enableLoadingPhrases', false), - ).toBe(false); - }); - }); - describe('settingExistsInScope', () => { it('should return true for top-level settings that exist', () => { const settings = makeMockSettings({ ui: { requiresRestart: true } }); @@ -781,26 +565,6 @@ describe('SettingsUtils', () => { }); }); - describe('hasRestartRequiredSettings', () => { - it('should return true when modified settings require restart', () => { - const modifiedSettings = new Set([ - 'advanced.autoConfigureMemory', - 'ui.requiresRestart', - ]); - expect(hasRestartRequiredSettings(modifiedSettings)).toBe(true); - }); - - it('should return false when no modified settings require restart', () => { - const modifiedSettings = new Set(['test']); - expect(hasRestartRequiredSettings(modifiedSettings)).toBe(false); - }); - - it('should return false for empty set', () => { - const modifiedSettings = new Set(); - expect(hasRestartRequiredSettings(modifiedSettings)).toBe(false); - }); - }); - describe('getRestartRequiredFromModified', () => { it('should return only settings that require restart', () => { const modifiedSettings = new Set([ @@ -1159,108 +923,6 @@ describe('SettingsUtils', () => { expect(result).toBe(false); }); }); - - describe('isValueInherited', () => { - it('should return false for top-level settings that exist in scope', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const result = isValueInherited( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(false); - }); - - it('should return true for top-level settings that do not exist in scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const result = isValueInherited( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(true); - }); - - it('should return false for nested settings that exist in scope', () => { - const settings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); - const mergedSettings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); - - const result = isValueInherited( - 'ui.accessibility.enableLoadingPhrases', - settings, - mergedSettings, - ); - expect(result).toBe(false); - }); - - it('should return true for nested settings that do not exist in scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); - - const result = isValueInherited( - 'ui.accessibility.enableLoadingPhrases', - settings, - mergedSettings, - ); - expect(result).toBe(true); - }); - }); - - describe('getEffectiveDisplayValue', () => { - it('should return value from settings when available', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: false }, - }); - - const result = getEffectiveDisplayValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(true); - }); - - it('should return value from merged settings when not in scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const result = getEffectiveDisplayValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(true); - }); - - it('should return default value for undefined values', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({}); - - const result = getEffectiveDisplayValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(false); // Default value - }); - }); }); }); diff --git a/packages/cli/src/utils/settingsUtils.ts b/packages/cli/src/utils/settingsUtils.ts index 1404afa9b0..d6013c242d 100644 --- a/packages/cli/src/utils/settingsUtils.ts +++ b/packages/cli/src/utils/settingsUtils.ts @@ -13,7 +13,6 @@ import type { import type { SettingDefinition, SettingsSchema, - SettingsType, SettingsValue, } from '../config/settingsSchema.js'; import { getSettingsSchema } from '../config/settingsSchema.js'; @@ -53,29 +52,6 @@ function clearFlattenedSchema() { _FLATTENED_SCHEMA = undefined; } -/** - * Get all settings grouped by category - */ -export function getSettingsByCategory(): Record< - string, - Array -> { - const categories: Record< - string, - Array - > = {}; - - Object.values(getFlattenedSchema()).forEach((definition) => { - const category = definition.category; - if (!categories[category]) { - categories[category] = []; - } - categories[category].push(definition); - }); - - return categories; -} - /** * Get a setting definition by key */ @@ -175,88 +151,6 @@ export function getAllSettingKeys(): string[] { return Object.keys(getFlattenedSchema()); } -/** - * Get settings by type - */ -export function getSettingsByType( - type: SettingsType, -): Array { - return Object.values(getFlattenedSchema()).filter( - (definition) => definition.type === type, - ); -} - -/** - * Get settings that require restart - */ -export function getSettingsRequiringRestart(): Array< - SettingDefinition & { - key: string; - } -> { - return Object.values(getFlattenedSchema()).filter( - (definition) => definition.requiresRestart, - ); -} - -/** - * Validate if a setting key exists in the schema - */ -export function isValidSettingKey(key: string): boolean { - return key in getFlattenedSchema(); -} - -/** - * Get the category for a setting - */ -export function getSettingCategory(key: string): string | undefined { - return getFlattenedSchema()[key]?.category; -} - -/** - * Check if a setting should be shown in the settings dialog - */ -export function shouldShowInDialog(key: string): boolean { - return getFlattenedSchema()[key]?.showInDialog ?? true; // Default to true for backward compatibility -} - -/** - * Get all settings that should be shown in the dialog, grouped by category - */ -export function getDialogSettingsByCategory(): Record< - string, - Array -> { - const categories: Record< - string, - Array - > = {}; - - Object.values(getFlattenedSchema()) - .filter((definition) => definition.showInDialog !== false) - .forEach((definition) => { - const category = definition.category; - if (!categories[category]) { - categories[category] = []; - } - categories[category].push(definition); - }); - - return categories; -} - -/** - * Get settings by type that should be shown in the dialog - */ -export function getDialogSettingsByType( - type: SettingsType, -): Array { - return Object.values(getFlattenedSchema()).filter( - (definition) => - definition.type === type && definition.showInDialog !== false, - ); -} - /** * Explicit display order for settings shown in the Settings Dialog. * Settings are ordered by importance and logical grouping: @@ -387,46 +281,6 @@ export function getDialogSettingKeys(): string[] { // BUSINESS LOGIC UTILITIES (Higher-level utilities for setting operations) // ============================================================================ -/** - * Get the current value for a setting in a specific scope - * Always returns a value (never undefined) - falls back to default if not set anywhere - */ -export function getSettingValue( - key: string, - settings: Settings, - mergedSettings: Settings, -): boolean { - const definition = getSettingDefinition(key); - if (!definition) { - return false; // Default fallback for invalid settings - } - - const value = getEffectiveValue(key, settings, mergedSettings); - // Ensure we return a boolean value, converting from the more general type - if (typeof value === 'boolean') { - return value; - } - // Fall back to default value, ensuring it's a boolean - const defaultValue = definition.default; - if (typeof defaultValue === 'boolean') { - return defaultValue; - } - return false; // Final fallback -} - -/** - * Check if a setting value is modified from its default - */ -export function isSettingModified(key: string, value: boolean): boolean { - const defaultValue = getDefaultValue(key); - // Handle type comparison properly - if (typeof defaultValue === 'boolean') { - return value !== defaultValue; - } - // If default is not a boolean, consider it modified if value is true - return value === true; -} - /** * Check if a setting exists in the original settings file for a scope */ @@ -554,15 +408,6 @@ export function setPendingSettingValueAny( return newSettings; } -/** - * Check if any modified settings require a restart - */ -export function hasRestartRequiredSettings( - modifiedSettings: Set, -): boolean { - return Array.from(modifiedSettings).some((key) => requiresRestart(key)); -} - /** * Get the restart required settings from a set of modified settings */ @@ -673,29 +518,6 @@ export function isDefaultValue(key: string, settings: Settings): boolean { return !settingExistsInScope(key, settings); } -/** - * Check if a setting value is inherited (not set at current scope) - */ -export function isValueInherited( - key: string, - settings: Settings, - _mergedSettings: Settings, -): boolean { - return !settingExistsInScope(key, settings); -} - -/** - * Get the effective value for display, considering inheritance - * Always returns a boolean value (never undefined) - */ -export function getEffectiveDisplayValue( - key: string, - settings: Settings, - mergedSettings: Settings, -): boolean { - return getSettingValue(key, settings, mergedSettings); -} - /** * Backup a settings file before modification. * Always creates a fresh backup with `.orig` suffix (overwrites any stale backup).