From b4f5079dc0cc89ce1448785898ab72b358399c95 Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 18 Aug 2026 16:05:19 +0000 Subject: [PATCH] refactor(cli): remove superseded settings dialog helpers [settings-utils-generation-a] (#9379) The settings utility module carried two generations of dialog API. The dialog was rebuilt on one set of helpers and the earlier set was never removed, leaving thirteen exported functions whose only consumer was their own unit test. One was a character-for-character duplicate of a helper the dialog does use; another wrapped a wrapper. Removes those thirteen with the tests that existed only to cover them, plus the type import they alone needed. Helpers that a naive search also flags are kept: one is still called by live code, and one is named by an in-flight design document as an API it intends to reuse. Behaviour is unchanged. Candidate settings-utils-generation-a from the find-simplifications sweep. Co-authored-by: Claude Opus 5 --- packages/cli/src/utils/settingsUtils.test.ts | 340 +------------------ packages/cli/src/utils/settingsUtils.ts | 178 ---------- 2 files changed, 1 insertion(+), 517 deletions(-) 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).