mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-10 22:21:22 -07:00
refactor(cli): Reactive useSettingsStore hook (#14915)
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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<Settings> | 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();
|
||||
}
|
||||
|
||||
|
||||
167
packages/cli/src/ui/contexts/SettingsContext.test.tsx
Normal file
167
packages/cli/src/ui/contexts/SettingsContext.test.tsx
Normal file
@@ -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 }) => (
|
||||
<SettingsContext.Provider value={mockLoadedSettings}>
|
||||
{children}
|
||||
</SettingsContext.Provider>
|
||||
);
|
||||
|
||||
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<typeof vi.fn>
|
||||
).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(
|
||||
<ErrorBoundary onError={onError}>
|
||||
<TestHarness />
|
||||
</ErrorBoundary>,
|
||||
);
|
||||
|
||||
expect(onError).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
message: 'useSettingsStore must be used within a SettingsProvider',
|
||||
}),
|
||||
);
|
||||
|
||||
consoleSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
@@ -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<LoadedSettings | undefined>(
|
||||
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],
|
||||
);
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user