Client settings

[DOsinga]

Summary

Move away from using localStorage

[Copilot]

Pull request overview

This PR migrates UI preferences from browser localStorage to Electron's settings.json file for better persistence and integration with the desktop app architecture. The migration includes theme preferences, response styles, pricing display, session sharing configuration, and announcement tracking.

Changes:

• Adds new settings fields to the Settings interface for UI preferences (theme, responseStyle, showPricing, sessionSharing, seenAnnouncementIds)
• Implements getSetting/setSetting IPC handlers for granular settings access
• Updates all components to use window.electron.getSetting/setSetting instead of localStorage
• References a migration utility to move existing localStorage data (file not included in PR)

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
ui/desktop/src/utils/settings.ts	Adds UI preference fields to Settings interface and default values
ui/desktop/src/main.ts	Implements get-setting/set-setting IPC handlers, removes duplicate default settings definition
ui/desktop/src/preload.ts	Exposes getSetting/setSetting methods to renderer process with type safety
ui/desktop/src/renderer.tsx	Imports and calls migration function on app startup
ui/desktop/src/contexts/ThemeContext.tsx	Replaces localStorage with settings API for theme preferences
ui/desktop/src/components/settings/sessions/SessionSharingSection.tsx	Updates to use settings API instead of localStorage
ui/desktop/src/components/settings/response_styles/ResponseStylesSection.tsx	Migrates response style storage to settings API
ui/desktop/src/components/settings/app/AppSettingsSection.tsx	Updates pricing toggle to use settings API
ui/desktop/src/components/sessions/SessionHistoryView.tsx	Reads session sharing config from settings
ui/desktop/src/components/bottom_menu/CostTracker.tsx	Updates to listen for pricing changes via custom event
ui/desktop/src/components/ToolCallWithResponse.tsx	Loads response style from settings instead of localStorage
ui/desktop/src/components/AnnouncementModal.tsx	Stores seen announcements in settings instead of localStorage
ui/desktop/src/sessionLinks.ts	Reads session sharing config from settings
ui/desktop/package-lock.json	Updates peer dependency resolution for local packages

Files not reviewed (1)

• ui/desktop/package-lock.json: Language not supported

[Copilot]

The set-setting handler doesn't return a value, but the preload API expects a Promise<void>. While this might work, it's inconsistent with the save-settings handler which returns true. Consider returning a value for consistency and to match the type signature.

Suggested change

	}
	}
	return true;

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

ui/desktop/src/utils/settings.ts:92

• keyboardShortcuts is now non-optional in Settings, but getKeyboardShortcuts() still relies on !settings.keyboardShortcuts to migrate legacy globalShortcut -> shortcuts; with defaults merged on load this branch will never run, so users with only globalShortcut set will silently lose their custom shortcut.
  Either keep keyboardShortcuts optional for migration, or move the migration logic to settings-load time using the raw stored object before defaults are applied.

  spellcheckEnabled: boolean;
  externalGoosed: ExternalGoosedConfig;
  globalShortcut?: string | null;
  keyboardShortcuts: KeyboardShortcuts;

  // UI preferences (migrated from localStorage)
  theme: 'dark' | 'light';
  useSystemTheme: boolean;
  responseStyle: string;
  showPricing: boolean;
  sessionSharing: SessionSharingConfig;
  seenAnnouncementIds: string[];
}

export type SettingKey = keyof Settings;

export const defaultKeyboardShortcuts: DefaultKeyboardShortcuts = {
  focusWindow: 'CommandOrControl+Alt+G',
  quickLauncher: 'CommandOrControl+Alt+Shift+G',
  newChat: 'CommandOrControl+T',
  newChatWindow: 'CommandOrControl+N',
  openDirectory: 'CommandOrControl+O',
  settings: 'CommandOrControl+,',
  find: 'CommandOrControl+F',
  findNext: 'CommandOrControl+G',
  findPrevious: 'CommandOrControl+Shift+G',
  alwaysOnTop: 'CommandOrControl+Shift+T',
};

export const defaultSettings: Settings = {
  // Desktop app settings
  showMenuBarIcon: true,
  showDockIcon: true,
  enableWakelock: false,
  spellcheckEnabled: true,
  keyboardShortcuts: defaultKeyboardShortcuts,
  externalGoosed: {
    enabled: false,
    url: '',
    secret: '',
  },

  // UI preferences
  theme: 'light',
  useSystemTheme: true,
  responseStyle: 'concise',
  showPricing: true,
  sessionSharing: {
    enabled: false,
    baseUrl: '',
  },
  seenAnnouncementIds: [],
};

export function getKeyboardShortcuts(settings: Settings): KeyboardShortcuts {
  if (!settings.keyboardShortcuts && settings.globalShortcut !== undefined) {
    const focusShortcut = settings.globalShortcut;
    let launcherShortcut: string | null = null;

[Copilot]

window.electron.getSetting('showPricing').then(setShowPricing) has no .catch(...); if the IPC call rejects, this becomes an unhandled promise rejection in the renderer.
Handle errors (e.g. catch + keep the default true) so a corrupted settings file doesn't break the settings UI.

Suggested change

	window.electron.getSetting('showPricing').then(setShowPricing);
	window.electron
	.getSetting('showPricing')
	.then((value) => {
	if (typeof value === 'boolean') {
	setShowPricing(value);
	}
	})
	.catch(() => {
	// Keep default `true` if the setting cannot be loaded
	});

[Copilot]

getSettings() does a shallow merge ({ ...defaultSettings, ...stored }), which will drop nested defaults when a stored nested object is partial (e.g. externalGoosed.secret or individual keyboardShortcuts keys), leading to undefined fields at runtime.
Consider normalizing with a per-nested-key merge (or schema validation) so nested objects are merged with their defaults instead of replaced wholesale.

Suggested change

	const stored = JSON.parse(data);
	// Merge with defaults to ensure all keys exist
	return { ...defaultSettings, ...stored };
	const stored = JSON.parse(data) as Partial<Settings>;
	// Merge with defaults to ensure all keys exist, including nested objects
	const merged: Settings = {
	...defaultSettings,
	...stored,
	externalGoosed: {
	...defaultSettings.externalGoosed,
	...(stored.externalGoosed ?? {}),
	},
	keyboardShortcuts: {
	...defaultSettings.keyboardShortcuts,
	...(stored.keyboardShortcuts ?? {}),
	},
	};
	return merged;

[Copilot]

The set-setting IPC handler writes (settings as any)[key] = value without runtime validation of key; a compromised renderer could pass __proto__ / constructor and trigger prototype pollution or mutate unexpected fields.
Add a runtime whitelist check against known SettingKey values (and explicitly block prototype keys) before applying the update.

[Copilot]

parseLocalStorageValue() treats any non-"true" value as false for boolean settings (e.g. useSystemTheme, showPricing), so a corrupted or unexpected localStorage value will be migrated into settings as false instead of being ignored.
Prefer returning null unless the raw value is exactly "true" or "false" (and optionally clear the bad localStorage key) to avoid silently changing user preferences during migration.

Suggested change

	return (rawValue === 'true') as unknown as Settings[K];
	case 'responseStyle':
	return rawValue as Settings[K];
	case 'showPricing':
	return (rawValue === 'true') as unknown as Settings[K];
	if (rawValue === 'true') {
	return true as unknown as Settings[K];
	}
	if (rawValue === 'false') {
	return false as unknown as Settings[K];
	}
	return null;
	case 'responseStyle':
	return rawValue as Settings[K];
	case 'showPricing':
	if (rawValue === 'true') {
	return true as unknown as Settings[K];
	}
	if (rawValue === 'false') {
	return false as unknown as Settings[K];
	}
	return null;

[Copilot]

The getSetting('sessionSharing') call in the effect isn't error-handled; if the IPC invoke rejects (e.g. settings file parse error) this can produce an unhandled promise rejection and leave the UI in an inconsistent state.
Wrap the async load in a try/catch (or add .catch(...)) and fall back to the current in-memory defaults on failure.

Suggested change

	window.electron.getSetting('sessionSharing').then((config) => {
	setSessionSharingConfig(config);
	});
	window.electron
	.getSetting('sessionSharing')
	.then((config) => {
	setSessionSharingConfig(config);
	})
	.catch((error) => {
	// Fall back to current in-memory defaults on failure.
	console.error('Failed to load session sharing settings, using defaults', error);
	});

[zanesq]

Thanks for tackling this! Tested locally and looks good. Only piece of feedback from goose worth noting is:

Prototype Pollution Protection is Good, But (settings as any)[key] = value is Risky
The validSettingKeys allowlist in main.ts is a good security measure. However, the value parameter is completely unvalidated — a renderer compromise could write arbitrary data structures into any allowed key. For example, writing a massive blob to seenAnnouncementIds or a non-object to keyboardShortcuts.

Recommendation: Consider adding basic type validation per key (e.g., typeof value === 'boolean' for boolean keys, Array.isArray(value) for array keys). Not critical for v1, but worth a TODO.

[DOsinga]

Thanks for testing, @zanesq

Thanks for tackling this! Tested locally and looks good. Only piece of feedback from goose worth noting is:
Prototype Pollution Protection is Good, But (settings as any)[key] = value is Risky
The validSettingKeys allowlist in main.ts is a good security measure. However, the value parameter is completely unvalidated — a renderer compromise could write arbitrary data structures into any allowed key. For example, writing a massive blob to seenAnnouncementIds or a non-object to keyboardShortcuts.
Recommendation: Consider adding basic type validation per key (e.g., typeof value === 'boolean' for boolean keys, Array.isArray(value) for array keys). Not critical for v1, but worth a TODO.

yeah, I don't know - we're not protecting against an compromised renderer here. even checking the keys seems not all that useful since typescript should protect us. but that is something somebody might override easier, I think.

if you can think of a way to this with introspection rather than explicit, I'd be up for it