From b8008695dbedc8d2e03de2d97c697a87e15c4fc9 Mon Sep 17 00:00:00 2001 From: Pyush Sinha Date: Wed, 11 Feb 2026 15:40:27 -0800 Subject: [PATCH] refactor(cli): Reactive useSettingsStore hook (#14915) --- packages/cli/src/config/settings.test.ts | 44 +++++ packages/cli/src/config/settings.ts | 48 +++++ .../src/ui/contexts/SettingsContext.test.tsx | 167 ++++++++++++++++++ .../cli/src/ui/contexts/SettingsContext.tsx | 70 +++++++- 4 files changed, 326 insertions(+), 3 deletions(-) create mode 100644 packages/cli/src/ui/contexts/SettingsContext.test.tsx diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 721458952f..e88c9104dd 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -2546,6 +2546,50 @@ describe('Settings Loading and Merging', () => { }); }); + describe('Reactivity & Snapshots', () => { + let loadedSettings: LoadedSettings; + + beforeEach(() => { + const emptySettingsFile: SettingsFile = { + path: '/mock/path', + settings: {}, + originalSettings: {}, + }; + + loadedSettings = new LoadedSettings( + { ...emptySettingsFile, path: getSystemSettingsPath() }, + { ...emptySettingsFile, path: getSystemDefaultsPath() }, + { ...emptySettingsFile, path: USER_SETTINGS_PATH }, + { ...emptySettingsFile, path: MOCK_WORKSPACE_SETTINGS_PATH }, + true, // isTrusted + [], + ); + }); + + it('getSnapshot() should return stable reference if no changes occur', () => { + const snap1 = loadedSettings.getSnapshot(); + const snap2 = loadedSettings.getSnapshot(); + expect(snap1).toBe(snap2); + }); + + it('setValue() should create a new snapshot reference and emit event', () => { + const oldSnapshot = loadedSettings.getSnapshot(); + const oldUserRef = oldSnapshot.user.settings; + + loadedSettings.setValue(SettingScope.User, 'ui.theme', 'high-contrast'); + + const newSnapshot = loadedSettings.getSnapshot(); + + expect(newSnapshot).not.toBe(oldSnapshot); + expect(newSnapshot.user.settings).not.toBe(oldUserRef); + expect(newSnapshot.user.settings.ui?.theme).toBe('high-contrast'); + + expect(newSnapshot.system.settings).not.toBe(oldSnapshot.system.settings); + + expect(mockCoreEvents.emitSettingsChanged).toHaveBeenCalled(); + }); + }); + describe('Security and Sandbox', () => { let originalArgv: string[]; let originalEnv: NodeJS.ProcessEnv; diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 8e9ff7380f..b2b526a010 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -10,6 +10,7 @@ import { platform } from 'node:os'; import * as dotenv from 'dotenv'; import process from 'node:process'; import { + CoreEvent, FatalConfigError, GEMINI_DIR, getErrorMessage, @@ -284,6 +285,20 @@ export function createTestMergedSettings( ) as MergedSettings; } +/** + * An immutable snapshot of settings state. + * Used with useSyncExternalStore for reactive updates. + */ +export interface LoadedSettingsSnapshot { + system: SettingsFile; + systemDefaults: SettingsFile; + user: SettingsFile; + workspace: SettingsFile; + isTrusted: boolean; + errors: SettingsError[]; + merged: MergedSettings; +} + export class LoadedSettings { constructor( system: SettingsFile, @@ -303,6 +318,7 @@ export class LoadedSettings { : this.createEmptyWorkspace(workspace); this.errors = errors; this._merged = this.computeMergedSettings(); + this._snapshot = this.computeSnapshot(); } readonly system: SettingsFile; @@ -314,6 +330,7 @@ export class LoadedSettings { private _workspaceFile: SettingsFile; private _merged: MergedSettings; + private _snapshot: LoadedSettingsSnapshot; private _remoteAdminSettings: Partial | undefined; get merged(): MergedSettings { @@ -368,6 +385,36 @@ export class LoadedSettings { return merged; } + private computeSnapshot(): LoadedSettingsSnapshot { + const cloneSettingsFile = (file: SettingsFile): SettingsFile => ({ + path: file.path, + rawJson: file.rawJson, + settings: structuredClone(file.settings), + originalSettings: structuredClone(file.originalSettings), + }); + return { + system: cloneSettingsFile(this.system), + systemDefaults: cloneSettingsFile(this.systemDefaults), + user: cloneSettingsFile(this.user), + workspace: cloneSettingsFile(this.workspace), + isTrusted: this.isTrusted, + errors: [...this.errors], + merged: structuredClone(this._merged), + }; + } + + // Passing this along with getSnapshot to useSyncExternalStore allows for idiomatic reactivity on settings changes + // React will pass a listener fn into this subscribe fn + // that listener fn will perform an object identity check on the snapshot and trigger a React re render if the snapshot has changed + subscribe(listener: () => void): () => void { + coreEvents.on(CoreEvent.SettingsChanged, listener); + return () => coreEvents.off(CoreEvent.SettingsChanged, listener); + } + + getSnapshot(): LoadedSettingsSnapshot { + return this._snapshot; + } + forScope(scope: LoadableSettingScope): SettingsFile { switch (scope) { case SettingScope.User: @@ -409,6 +456,7 @@ export class LoadedSettings { } this._merged = this.computeMergedSettings(); + this._snapshot = this.computeSnapshot(); coreEvents.emitSettingsChanged(); } diff --git a/packages/cli/src/ui/contexts/SettingsContext.test.tsx b/packages/cli/src/ui/contexts/SettingsContext.test.tsx new file mode 100644 index 0000000000..3124108f90 --- /dev/null +++ b/packages/cli/src/ui/contexts/SettingsContext.test.tsx @@ -0,0 +1,167 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type React from 'react'; +import { Component, type ReactNode } from 'react'; +import { renderHook, render } from '../../test-utils/render.js'; +import { act } from 'react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { SettingsContext, useSettingsStore } from './SettingsContext.js'; +import { + type LoadedSettings, + SettingScope, + type LoadedSettingsSnapshot, + type SettingsFile, + createTestMergedSettings, +} from '../../config/settings.js'; + +const createMockSettingsFile = (path: string): SettingsFile => ({ + path, + settings: {}, + originalSettings: {}, +}); + +const mockSnapshot: LoadedSettingsSnapshot = { + system: createMockSettingsFile('/system'), + systemDefaults: createMockSettingsFile('/defaults'), + user: createMockSettingsFile('/user'), + workspace: createMockSettingsFile('/workspace'), + isTrusted: true, + errors: [], + merged: createTestMergedSettings({ + ui: { theme: 'default-theme' }, + }), +}; + +class ErrorBoundary extends Component< + { children: ReactNode; onError: (error: Error) => void }, + { hasError: boolean } +> { + constructor(props: { children: ReactNode; onError: (error: Error) => void }) { + super(props); + this.state = { hasError: false }; + } + + static getDerivedStateFromError(_error: Error) { + return { hasError: true }; + } + + override componentDidCatch(error: Error) { + this.props.onError(error); + } + + override render() { + if (this.state.hasError) { + return null; + } + return this.props.children; + } +} + +const TestHarness = () => { + useSettingsStore(); + return null; +}; + +describe('SettingsContext', () => { + let mockLoadedSettings: LoadedSettings; + let listeners: Array<() => void> = []; + + beforeEach(() => { + listeners = []; + + mockLoadedSettings = { + subscribe: vi.fn((listener: () => void) => { + listeners.push(listener); + return () => { + listeners = listeners.filter((l) => l !== listener); + }; + }), + getSnapshot: vi.fn(() => mockSnapshot), + setValue: vi.fn(), + } as unknown as LoadedSettings; + }); + + const wrapper = ({ children }: { children: React.ReactNode }) => ( + + {children} + + ); + + it('should provide the correct initial state', () => { + const { result } = renderHook(() => useSettingsStore(), { wrapper }); + + expect(result.current.settings.merged).toEqual(mockSnapshot.merged); + expect(result.current.settings.isTrusted).toBe(true); + }); + + it('should allow accessing settings for a specific scope', () => { + const { result } = renderHook(() => useSettingsStore(), { wrapper }); + + const userSettings = result.current.settings.forScope(SettingScope.User); + expect(userSettings).toBe(mockSnapshot.user); + + const workspaceSettings = result.current.settings.forScope( + SettingScope.Workspace, + ); + expect(workspaceSettings).toBe(mockSnapshot.workspace); + }); + + it('should trigger re-renders when settings change (external event)', () => { + const { result } = renderHook(() => useSettingsStore(), { wrapper }); + + expect(result.current.settings.merged.ui?.theme).toBe('default-theme'); + + const newSnapshot = { + ...mockSnapshot, + merged: { ui: { theme: 'new-theme' } }, + }; + ( + mockLoadedSettings.getSnapshot as ReturnType + ).mockReturnValue(newSnapshot); + + // Trigger the listeners (simulate coreEvents emission) + act(() => { + listeners.forEach((l) => l()); + }); + + expect(result.current.settings.merged.ui?.theme).toBe('new-theme'); + }); + + it('should call store.setValue when setSetting is called', () => { + const { result } = renderHook(() => useSettingsStore(), { wrapper }); + + act(() => { + result.current.setSetting(SettingScope.User, 'ui.theme', 'dark'); + }); + + expect(mockLoadedSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'ui.theme', + 'dark', + ); + }); + + it('should throw error if used outside provider', () => { + const onError = vi.fn(); + // Suppress console.error (React logs error boundary info) + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + render( + + + , + ); + + expect(onError).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'useSettingsStore must be used within a SettingsProvider', + }), + ); + + consoleSpy.mockRestore(); + }); +}); diff --git a/packages/cli/src/ui/contexts/SettingsContext.tsx b/packages/cli/src/ui/contexts/SettingsContext.tsx index 144e1a2859..2c5ae37dfd 100644 --- a/packages/cli/src/ui/contexts/SettingsContext.tsx +++ b/packages/cli/src/ui/contexts/SettingsContext.tsx @@ -4,17 +4,81 @@ * SPDX-License-Identifier: Apache-2.0 */ -import React, { useContext } from 'react'; -import type { LoadedSettings } from '../../config/settings.js'; +import React, { useContext, useMemo, useSyncExternalStore } from 'react'; +import type { + LoadableSettingScope, + LoadedSettings, + LoadedSettingsSnapshot, + SettingsFile, +} from '../../config/settings.js'; +import { SettingScope } from '../../config/settings.js'; export const SettingsContext = React.createContext( undefined, ); -export const useSettings = () => { +export const useSettings = (): LoadedSettings => { const context = useContext(SettingsContext); if (context === undefined) { throw new Error('useSettings must be used within a SettingsProvider'); } return context; }; + +export interface SettingsState extends LoadedSettingsSnapshot { + forScope: (scope: LoadableSettingScope) => SettingsFile; +} + +export interface SettingsStoreValue { + settings: SettingsState; + setSetting: ( + scope: LoadableSettingScope, + key: string, + value: unknown, + ) => void; +} + +// Components that call this hook will re render when a settings change event is emitted +export const useSettingsStore = (): SettingsStoreValue => { + const store = useContext(SettingsContext); + if (store === undefined) { + throw new Error('useSettingsStore must be used within a SettingsProvider'); + } + + // React passes a listener fn into the subscribe function + // When the listener runs, it re renders the component if the snapshot changed + const snapshot = useSyncExternalStore( + (listener) => store.subscribe(listener), + () => store.getSnapshot(), + ); + + const settings: SettingsState = useMemo( + () => ({ + ...snapshot, + forScope: (scope: LoadableSettingScope) => { + switch (scope) { + case SettingScope.User: + return snapshot.user; + case SettingScope.Workspace: + return snapshot.workspace; + case SettingScope.System: + return snapshot.system; + case SettingScope.SystemDefaults: + return snapshot.systemDefaults; + default: + throw new Error(`Invalid scope: ${scope}`); + } + }, + }), + [snapshot], + ); + + return useMemo( + () => ({ + settings, + setSetting: (scope: LoadableSettingScope, key: string, value: unknown) => + store.setValue(scope, key, value), + }), + [settings, store], + ); +};