From e8c045876e245184ad64f074b4b2021955d109f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Sun, 17 May 2026 00:50:59 +0800 Subject: [PATCH] Stabilize portable mode PR before merge The contributor PR adds useful Windows terminal and portable mode support, but it also reintroduced an older General settings zoom block and left the new native settings paths without enough regression coverage. This commit keeps the feature direction intact while removing the duplicate UI, making invalid bash paths fail at save time, and covering the portable cache and app-mode paths with focused tests. Constraint: This commit lands directly on the contributor PR branch to avoid a long review-comment loop. Rejected: Ask the contributor to rework the PR from scratch | the remaining issues are narrow and maintainable by us. Confidence: high Scope-risk: moderate Directive: Keep future portable-mode changes covered at the native boundary and the desktop store boundary. Tested: cd desktop && bun run test src/pages/TerminalSettings.test.tsx src/__tests__/generalSettings.test.tsx src/stores/settingsStore.test.ts Tested: bun test src/utils/__tests__/cachePaths.test.ts Tested: cd desktop/src-tauri && cargo test Tested: cd desktop && bun run lint Tested: cd desktop/src-tauri && cargo check Tested: bun run check:server Tested: bun run check:desktop Not-tested: Manual Windows packaged-app portable-mode smoke; to be covered before a future release. --- desktop/src-tauri/src/lib.rs | 101 ++++++++++++++------ desktop/src/pages/Settings.tsx | 27 ------ desktop/src/pages/TerminalSettings.test.tsx | 43 ++++++++- desktop/src/stores/settingsStore.test.ts | 60 ++++++++++++ src/utils/__tests__/cachePaths.test.ts | 26 +++++ src/utils/cachePaths.ts | 16 ++-- 6 files changed, 210 insertions(+), 63 deletions(-) create mode 100644 src/utils/__tests__/cachePaths.test.ts diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 3849931f..f7e94cbc 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -258,17 +258,6 @@ impl Default for AppModeConfig { } } -/// Read the persisted app-mode.json from the given config directory. -fn read_app_mode_config(config_dir: &Path) -> Option { - let path = config_dir.join(APP_MODE_FILE); - fs::read_to_string(&path).ok().and_then(|data| { - serde_json::from_str(&data).ok().or_else(|| { - eprintln!("[desktop] failed to parse app-mode.json: {}", data); - None - }) - }) -} - /// Write the persisted app-mode.json to the given config directory. fn write_app_mode_config(config_dir: &Path, config: &AppModeConfig) { let path = config_dir.join(APP_MODE_FILE); @@ -329,24 +318,25 @@ impl TerminalConfig { .unwrap_or_default() } - fn save(&self, app: &AppHandle) { - let Some(path) = terminal_config_path(app) else { return }; + fn save(&self, app: &AppHandle) -> Result<(), String> { + let Some(path) = terminal_config_path(app) else { + return Err("terminal config path is unavailable".to_string()); + }; if let Some(parent) = path.parent() { if let Err(err) = fs::create_dir_all(parent) { - eprintln!("[desktop] failed to create terminal config directory: {err}"); - return; + return Err(format!("create terminal config directory: {err}")); } } let data = match serde_json::to_string_pretty(self) { Ok(data) => data, Err(err) => { - eprintln!("[desktop] failed to serialize terminal config: {err}"); - return; + return Err(format!("serialize terminal config: {err}")); } }; if let Err(err) = fs::write(&path, data) { - eprintln!("[desktop] failed to write terminal config: {err}"); + return Err(format!("write terminal config: {err}")); } + Ok(()) } } @@ -1043,10 +1033,10 @@ fn get_terminal_bash_path(app: AppHandle) -> Option { } #[tauri::command] -fn set_terminal_bash_path(app: AppHandle, path: Option) { +fn set_terminal_bash_path(app: AppHandle, path: Option) -> Result<(), String> { let mut config = TerminalConfig::load(&app); - config.bash_path = path; - config.save(&app); + config.bash_path = normalize_terminal_bash_path(path)?; + config.save(&app) } #[tauri::command] @@ -1270,12 +1260,27 @@ fn home_dir() -> Option { .map(PathBuf::from) } -fn default_shell(custom_bash: Option<&str>) -> String { +fn normalize_terminal_bash_path(path: Option) -> Result, String> { + let Some(path) = path else { + return Ok(None); + }; + let trimmed = path.trim(); + if trimmed.is_empty() { + return Ok(None); + } + let bash_path = PathBuf::from(trimmed); + if !bash_path.is_file() { + return Err(format!("terminal bash path does not exist: {trimmed}")); + } + Ok(Some(trimmed.to_string())) +} + +fn default_shell(_custom_bash: Option<&str>) -> String { // On Windows, use configured bash path if set and valid #[cfg(target_os = "windows")] - if let Some(bash_path) = custom_bash { + if let Some(bash_path) = _custom_bash { let trimmed = bash_path.trim(); - if !trimmed.is_empty() && PathBuf::from(trimmed).exists() { + if !trimmed.is_empty() && PathBuf::from(trimmed).is_file() { return trimmed.to_string(); } } @@ -1686,9 +1691,9 @@ fn kill_windows_sidecars() { mod tests { use super::{ decode_terminal_output, default_utf8_locale, ensure_utf8_locale, - has_meaningful_intersection, is_persistable_window_state, parse_env_block, - run_notification_bridge, select_h5_dist_dir, StoredWindowState, SERVER_BIND_HOST, - SERVER_CONTROL_HOST, + has_meaningful_intersection, is_persistable_window_state, normalize_terminal_bash_path, + parse_env_block, run_notification_bridge, select_h5_dist_dir, StoredWindowState, + SERVER_BIND_HOST, SERVER_CONTROL_HOST, }; use std::{collections::HashMap, fs}; @@ -1788,6 +1793,48 @@ mod tests { assert_eq!(env.get("EMPTY").map(String::as_str), Some("")); } + #[test] + fn terminal_bash_path_normalizer_clears_blank_values() { + assert_eq!( + normalize_terminal_bash_path(Some(" ".to_string())).expect("blank path clears"), + None + ); + assert_eq!( + normalize_terminal_bash_path(None).expect("missing path clears"), + None + ); + } + + #[test] + fn terminal_bash_path_normalizer_rejects_missing_files() { + let missing = std::env::temp_dir().join(format!( + "cchh-missing-bash-{}", + std::process::id() + )); + + let error = normalize_terminal_bash_path(Some(missing.to_string_lossy().to_string())) + .expect_err("missing path should be rejected"); + + assert!(error.contains("terminal bash path does not exist")); + } + + #[test] + fn terminal_bash_path_normalizer_accepts_existing_files() { + let path = std::env::temp_dir().join(format!( + "cchh-bash-path-test-{}", + std::process::id() + )); + fs::write(&path, "").expect("write bash path fixture"); + + assert_eq!( + normalize_terminal_bash_path(Some(format!(" {} ", path.display()))) + .expect("existing file is accepted"), + Some(path.to_string_lossy().to_string()) + ); + + fs::remove_file(path).expect("remove bash path fixture"); + } + #[test] fn terminal_environment_forces_utf8_locale_when_shell_uses_c_locale() { let mut env = HashMap::from([ diff --git a/desktop/src/pages/Settings.tsx b/desktop/src/pages/Settings.tsx index b096e065..a66bc8bf 100644 --- a/desktop/src/pages/Settings.tsx +++ b/desktop/src/pages/Settings.tsx @@ -1990,33 +1990,6 @@ function GeneralSettings() { - {/* UI Zoom */} -
-

{t('settings.general.uiZoom')}

-

{t('settings.general.uiZoomDescription')}

-
-
- - {Math.round(uiZoom * 100)}% - -
-
- {Math.round(UI_ZOOM_MIN * 100)}% - setUiZoom(parseFloat(e.target.value))} - onMouseUp={(e) => e.currentTarget.blur()} - className="flex-1" - /> - {Math.round(UI_ZOOM_MAX * 100)}% -
-
-
- {/* Confirm dialog for mode switch */} { kill: vi.fn(), onOutput: vi.fn(), onExit: vi.fn(), + getBashPath: vi.fn(), + setBashPath: vi.fn(), } }) @@ -48,6 +50,8 @@ vi.mock('../api/terminal', () => ({ kill: terminalMocks.kill, onOutput: terminalMocks.onOutput, onExit: terminalMocks.onExit, + getBashPath: terminalMocks.getBashPath, + setBashPath: terminalMocks.setBashPath, }, })) @@ -55,6 +59,7 @@ import { TerminalSettings } from './TerminalSettings' describe('TerminalSettings', () => { beforeEach(() => { + vi.restoreAllMocks() useSettingsStore.setState({ locale: 'en' }) terminalMocks.available = false terminalMocks.spawn.mockReset() @@ -63,6 +68,8 @@ describe('TerminalSettings', () => { terminalMocks.kill.mockReset() terminalMocks.onOutput.mockReset() terminalMocks.onExit.mockReset() + terminalMocks.getBashPath.mockReset() + terminalMocks.setBashPath.mockReset() terminalMocks.terminalInstance.loadAddon.mockClear() terminalMocks.terminalInstance.open.mockClear() terminalMocks.terminalInstance.dispose.mockClear() @@ -73,6 +80,8 @@ describe('TerminalSettings', () => { terminalMocks.fitInstance.fit.mockClear() terminalMocks.onOutput.mockResolvedValue(vi.fn()) terminalMocks.onExit.mockResolvedValue(vi.fn()) + terminalMocks.getBashPath.mockResolvedValue(null) + terminalMocks.setBashPath.mockResolvedValue(undefined) terminalMocks.write.mockResolvedValue(undefined) terminalMocks.resize.mockResolvedValue(undefined) terminalMocks.kill.mockResolvedValue(undefined) @@ -85,6 +94,7 @@ describe('TerminalSettings', () => { observe = vi.fn() disconnect = vi.fn() }) + vi.spyOn(navigator, 'platform', 'get').mockReturnValue('MacIntel') }) it('shows a desktop-runtime empty state outside Tauri', () => { @@ -142,4 +152,35 @@ describe('TerminalSettings', () => { expect(terminalMocks.terminalInstance.write).toHaveBeenCalledWith('hello\r\n') expect(terminalMocks.terminalInstance.write).not.toHaveBeenCalledWith('ignored\r\n') }) + + it('saves a custom Windows bash path from the terminal settings panel', async () => { + vi.spyOn(navigator, 'platform', 'get').mockReturnValue('Win32') + terminalMocks.available = true + terminalMocks.getBashPath.mockResolvedValue('C:\\Program Files\\Git\\bin\\bash.exe') + + render() + + const input = await screen.findByDisplayValue('C:\\Program Files\\Git\\bin\\bash.exe') + fireEvent.change(input, { target: { value: ' C:\\Tools\\Git\\bin\\bash.exe ' } }) + fireEvent.click(screen.getByRole('button', { name: 'Save' })) + + await waitFor(() => { + expect(terminalMocks.setBashPath).toHaveBeenCalledWith('C:\\Tools\\Git\\bin\\bash.exe') + }) + expect(await screen.findByRole('button', { name: 'Saved' })).toBeInTheDocument() + }) + + it('shows an invalid path message when native bash path validation fails', async () => { + vi.spyOn(navigator, 'platform', 'get').mockReturnValue('Win32') + terminalMocks.available = true + terminalMocks.setBashPath.mockRejectedValue(new Error('terminal bash path does not exist')) + + render() + + const input = await screen.findByPlaceholderText('Bash Path') + fireEvent.change(input, { target: { value: 'C:\\missing\\bash.exe' } }) + fireEvent.click(screen.getByRole('button', { name: 'Save' })) + + expect(await screen.findByText('Path does not exist. Select a valid Bash executable.')).toBeInTheDocument() + }) }) diff --git a/desktop/src/stores/settingsStore.test.ts b/desktop/src/stores/settingsStore.test.ts index 8c45f172..c5aa7141 100644 --- a/desktop/src/stores/settingsStore.test.ts +++ b/desktop/src/stores/settingsStore.test.ts @@ -63,6 +63,66 @@ describe('settingsStore UI zoom', () => { }) }) +describe('settingsStore app mode', () => { + beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + delete (window as unknown as { __TAURI_INTERNALS__?: object }).__TAURI_INTERNALS__ + }) + + it('hydrates app mode from the native desktop command', async () => { + const invoke = vi.fn().mockResolvedValue({ + mode: 'portable', + portableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + defaultPortableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + }) + vi.doMock('@tauri-apps/api/core', () => ({ invoke })) + const tauriWindow = window as unknown as { __TAURI_INTERNALS__?: object } + tauriWindow.__TAURI_INTERNALS__ = {} + + const { useSettingsStore } = await import('./settingsStore') + + await useSettingsStore.getState().fetchAppMode() + + expect(invoke).toHaveBeenCalledWith('get_app_mode') + expect(useSettingsStore.getState().appMode).toEqual({ + mode: 'portable', + portableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + defaultPortableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + }) + }) + + it('persists app mode through the native desktop command and marks restart required', async () => { + const invoke = vi.fn().mockResolvedValue(undefined) + vi.doMock('@tauri-apps/api/core', () => ({ invoke })) + const tauriWindow = window as unknown as { __TAURI_INTERNALS__?: object } + tauriWindow.__TAURI_INTERNALS__ = {} + + const { useSettingsStore } = await import('./settingsStore') + useSettingsStore.setState({ + appMode: { + mode: 'default', + portableDir: null, + defaultPortableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + }, + appModeRequiresRestart: false, + }) + + await useSettingsStore.getState().setAppMode('portable') + + expect(invoke).toHaveBeenCalledWith('set_app_mode', { + mode: 'portable', + portableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + }) + expect(useSettingsStore.getState().appMode).toEqual({ + mode: 'portable', + portableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + defaultPortableDir: 'C:\\cc-haha\\CLAUDE_CONFIG_DIR', + }) + expect(useSettingsStore.getState().appModeRequiresRestart).toBe(true) + }) +}) + describe('settingsStore desktop notification persistence', () => { beforeEach(() => { vi.resetModules() diff --git a/src/utils/__tests__/cachePaths.test.ts b/src/utils/__tests__/cachePaths.test.ts new file mode 100644 index 00000000..2bd7af79 --- /dev/null +++ b/src/utils/__tests__/cachePaths.test.ts @@ -0,0 +1,26 @@ +import { afterEach, describe, expect, test } from 'bun:test' +import { join } from 'path' +import { tmpdir } from 'os' +import { CACHE_PATHS } from '../cachePaths.js' + +const originalClaudeConfigDir = process.env.CLAUDE_CONFIG_DIR + +afterEach(() => { + if (originalClaudeConfigDir === undefined) { + delete process.env.CLAUDE_CONFIG_DIR + } else { + process.env.CLAUDE_CONFIG_DIR = originalClaudeConfigDir + } +}) + +describe('CACHE_PATHS portable mode', () => { + test('places logs under CLAUDE_CONFIG_DIR when portable mode is active', () => { + const configDir = join(tmpdir(), 'cc-haha-portable-cache') + process.env.CLAUDE_CONFIG_DIR = configDir + + expect(CACHE_PATHS.baseLogs().startsWith(join(configDir, 'Cache'))).toBe(true) + expect(CACHE_PATHS.errors().startsWith(join(configDir, 'Cache'))).toBe(true) + expect(CACHE_PATHS.messages().startsWith(join(configDir, 'Cache'))).toBe(true) + expect(CACHE_PATHS.mcpLogs('test:server').startsWith(join(configDir, 'Cache'))).toBe(true) + }) +}) diff --git a/src/utils/cachePaths.ts b/src/utils/cachePaths.ts index e221a9e7..8162545f 100644 --- a/src/utils/cachePaths.ts +++ b/src/utils/cachePaths.ts @@ -6,10 +6,10 @@ import { djb2Hash } from './hash.js' // When CLAUDE_CONFIG_DIR is set (portable mode), place cache under it // so the install is fully self-contained. Otherwise fall back to the // system default (%LOCALAPPDATA%\claude-cli-nodejs\Cache on Windows). -const claudConfigDir = (process.env as Record).CLAUDE_CONFIG_DIR -const paths: { cache: string } = claudConfigDir - ? { cache: join(claudConfigDir, 'Cache') } - : envPaths('claude-cli') +function getCacheRoot() { + const claudeConfigDir = (process.env as Record).CLAUDE_CONFIG_DIR + return claudeConfigDir ? join(claudeConfigDir, 'Cache') : envPaths('claude-cli').cache +} // Local sanitizePath using djb2Hash — NOT the shared version from // sessionStoragePortable.ts which uses Bun.hash (wyhash) when available. @@ -29,14 +29,14 @@ function getProjectDir(cwd: string): string { } export const CACHE_PATHS = { - baseLogs: () => join(paths.cache, getProjectDir(getFsImplementation().cwd())), + baseLogs: () => join(getCacheRoot(), getProjectDir(getFsImplementation().cwd())), errors: () => - join(paths.cache, getProjectDir(getFsImplementation().cwd()), 'errors'), + join(getCacheRoot(), getProjectDir(getFsImplementation().cwd()), 'errors'), messages: () => - join(paths.cache, getProjectDir(getFsImplementation().cwd()), 'messages'), + join(getCacheRoot(), getProjectDir(getFsImplementation().cwd()), 'messages'), mcpLogs: (serverName: string) => join( - paths.cache, + getCacheRoot(), getProjectDir(getFsImplementation().cwd()), // Sanitize server name for Windows compatibility (colons are reserved for drive letters) `mcp-logs-${sanitizePath(serverName)}`,