diff --git a/Cargo.lock b/Cargo.lock index 2643e23..53cf21e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2386,6 +2386,7 @@ dependencies = [ "log", "serde", "serde_json", + "windows-sys 0.61.2", "winreg 0.52.0", ] diff --git a/core/Cargo.toml b/core/Cargo.toml index 8370a58..872688b 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -13,3 +13,4 @@ log = "0.4" winreg = "0.52" dirs = "5" chrono = "0.4" +windows-sys = { version = "0.61.2", features = ["Win32_System_Environment", "Win32_UI_WindowsAndMessaging", "Win32_Foundation"] } diff --git a/core/src/system.rs b/core/src/system.rs index a5da4af..aef4e12 100644 --- a/core/src/system.rs +++ b/core/src/system.rs @@ -1,3 +1,7 @@ +use windows_sys::Win32::System::Environment::ExpandEnvironmentStringsW; +use windows_sys::Win32::UI::WindowsAndMessaging::{ + SendMessageTimeoutW, HWND_BROADCAST, SMTO_ABORTIFHUNG, WM_SETTINGCHANGE, +}; use winreg::enums::*; use winreg::RegKey; @@ -26,6 +30,7 @@ pub fn validate_path(path: &str) -> bool { } /// 展开路径中的环境变量（如 %JAVA_HOME%\bin → C:\Program Files\Java\jdk-17\bin） +/// 包含 % 的路径（环境变量路径）无法展开，返回原始路径 pub fn expand_env_vars(path: &str) -> String { if !path.contains('%') { return path.to_string(); @@ -46,7 +51,7 @@ pub fn expand_env_vars(path: &str) -> String { // SAFETY: buffer 容量为 required（API 返回的精确大小），wide_path 以 null 结尾， // 且两个指针指向不同的内存区域，不存在重叠 - let mut buffer: Vec = vec![0; required as usize]; + let mut buffer = vec![0_u16; required as usize]; let result = unsafe { ExpandEnvironmentStringsW(wide_path.as_ptr(), buffer.as_mut_ptr(), required) }; @@ -73,10 +78,6 @@ fn decode_utf16_preserving(v: &[u16]) -> String { /// 广播环境变量更改通知（WM_SETTINGCHANGE） /// 广播 `WM_SETTINGCHANGE` 通知系统环境变量已变更 pub fn broadcast_env_change() { - const HWND_BROADCAST: isize = 0xFFFF; - const WM_SETTINGCHANGE: u32 = 0x001A; - const SMTO_ABORTIFHUNG: u32 = 0x0002; - // SAFETY: env_str 是以 null 结尾的 UTF-16 字符串，所有指针和常量均遵循 Win32 API 约定 let env_str: Vec = "Environment\0".encode_utf16().collect(); @@ -84,7 +85,7 @@ pub fn broadcast_env_change() { // lpdwResult 为 null 表示不需要返回值，其他参数均为常量 let result = unsafe { SendMessageTimeoutW( - HWND_BROADCAST, + HWND_BROADCAST as _, WM_SETTINGCHANGE, 0, env_str.as_ptr() as isize, @@ -101,24 +102,6 @@ pub fn broadcast_env_change() { } } -// ── 外部 FFI 声明 ── - -extern "system" { - /// https://learn.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-expandenvironmentstringsw - fn ExpandEnvironmentStringsW(lpSrc: *const u16, lpDst: *mut u16, nSize: u32) -> u32; - - /// https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-sendmessagetimeoutw - fn SendMessageTimeoutW( - hWnd: isize, - Msg: u32, - wParam: usize, - lParam: isize, - fuFlags: u32, - uTimeout: u32, - lpdwResult: *mut usize, - ) -> isize; -} - #[cfg(test)] mod tests { use super::*; diff --git a/docs/code-and-architecture-review.md b/docs/code-and-architecture-review.md new file mode 100644 index 0000000..c7ebe53 --- /dev/null +++ b/docs/code-and-architecture-review.md @@ -0,0 +1,69 @@ +# PathEditor v5.0 代码与架构审查报告 + +## 1. 项目概览 + +PathEditor v5.0 是一个功能完善的 Windows 系统环境变量 (PATH) 编辑器，支持 GUI 与 CLI 双模式。 +技术栈选型现代化且合理： +- **后端 / 核心逻辑**：Rust (Cargo Workspace) +- **GUI 框架**：Tauri 2.x +- **前端**：React 19 + TypeScript + Zustand + +整体项目结构清晰，职责划分明确，严格遵循了前后端分离与核心逻辑无平台依赖的设计原则。 + +## 2. 架构设计审查 + +### 2.1 Cargo Workspace 三层架构 +项目采用了经典的 Cargo Workspace 模式，分为三层： +- `core`: 纯 Rust 库 crate，包含所有的核心业务逻辑（注册表读写、备份、配置文件管理、路径验证与清理等）。该层**完全不依赖** Tauri 或 CLI 库，极大地提高了代码的复用性和可测试性。 +- `gui`: Tauri 桌面应用。仅作为薄包装层（Thin Wrapper），通过 `#[tauri::command]` 将 `core` 的功能暴露为 IPC 接口供前端调用。 +- `cli`: 命令行工具层。依赖 `core` 和 `clap` 库，直接提供命令行交互能力。 + +**审查结论**：架构设计非常优秀。核心逻辑解耦彻底，无论是 GUI 还是 CLI 都能复用同一套安全、经过测试的核心代码。 + +### 2.2 IPC 通信与状态同步 +前端与 Rust 后端通过 Tauri IPC 进行通信。 +- 所有的错误处理均通过 `Result` 返回，前端通过 `Promise` 捕获并处理，用户体验良好。 +- 针对非事务性的双写操作（如同时保存系统和用户 PATH），前端 `app-store.ts` 中使用了 `Promise.allSettled`。当发生部分成功（Partial Success）时，能正确捕获并重新加载注册表状态，避免了前端内存状态与后端注册表状态的漂移（State Drift）。 + +## 3. 后端代码审查 (Rust) + +### 3.1 核心逻辑 (`core`) +- **安全性与健壮性**： + - 在 `registry.rs` 中，严格检查了路径字符串的 Null 字节，以及 32767 个字符的 Windows 注册表长度上限，防止缓冲区溢出或写入失败。 + - 使用了安全的 `winreg` 库进行注册表操作。 +- **FFI 调用**： + - 在 `system.rs` 中调用 Windows API（如 `ExpandEnvironmentStringsW` 和 `SendMessageTimeoutW`）时，对 `unsafe` 代码块进行了详尽的 SAFETY 注释。 + - 能够妥善处理 UTF-16 编码和解码，保留非法码点避免丢失路径信息，细节处理非常到位。 + +### 3.2 命令行工具 (`cli`) +- **原子性与并发安全**： + - 在 CLI 的 `verify_and_save` 逻辑中，写入前会重新读取注册表并与原始状态对比。如果不一致，则拒绝写入并报错退出。这有效地防止了并发情况下的配置覆盖问题。 +- **用户体验**： + - 命令设计符合直觉，支持 `--dry-run` 预览以及 JSON 格式输出，方便与其他脚本集成。 + +## 4. 前端代码审查 (React + TypeScript) + +### 4.1 状态管理 (`app-store.ts`) +- 使用 `Zustand` 进行全局状态管理，状态树设计合理，避免了 React Context 可能带来的不必要重渲染。 +- 实现了完善的 `UndoRedoManager`，将每一步操作抽象为 `OperationType`，支持撤销/重做功能，这对于编辑器类应用来说是核心体验的加分项。 +- `isSaving` 状态守卫有效防止了用户双击保存按钮引发的并发竞争。 + +### 4.2 UI 与逻辑分离 +- 业务逻辑抽象到 `src/core` 目录下（如 `path-manager.ts`, `validation.ts`），UI 组件仅负责渲染和事件绑定。 +- `useAppActions.ts` 钩子巧妙地将组件层与 Store 状态操作解耦，使得组件代码极其整洁。 + +## 5. 改进建议 (Recommendations) + +虽然当前代码质量已经很高，但仍有以下几个方面可以进一步优化： + +1. **Rust FFI 维护性**： + 当前 `system.rs` 中手动声明了 `extern "system"` 函数。建议引入 `windows-rs` 或 `windows-sys` 库，这能提供微软官方维护的安全的 API 绑定，减少手动编写 FFI 签名带来的维护成本和潜在错误。 +2. **GUI 保存的并发安全 (Race Condition)**： + CLI 已经实现了保存前的二次状态比对（`verify_and_save`），但在 `gui/src/commands/registry.rs` 中，直接调用了 `save_system_paths`。如果在用户打开 GUI 修改期间，另一个进程修改了注册表，GUI 保存时可能会覆盖该修改。建议在 GUI 的 IPC 保存接口中，也引入类似 CLI 的版本校验（例如传入 `expected_original_paths` 进行比对）。 +3. **前端单元测试覆盖**： + 核心逻辑如 `undo-redo.ts` 和 `path-manager.ts` 纯函数特性明显，建议在 `tests/unit/` 下增加对这些文件的边界用例测试，确保复杂编辑操作下状态不崩溃。 +4. **长列表性能**： + 如果 PATH 环境变量条目非常多（虽然实际场景中一般在 100 条以内），React 渲染完整列表可能会有微小延迟。当前规模下无影响，但若未来考虑显示大量工具链路径扫描结果，可引入虚拟列表（Virtual List）。 + +## 总结 +PathEditor v5.0 的代码库是一个优秀的 Rust + Tauri + React 实践范例。它具有清晰的三层架构、严格的类型和边界检查、以及良好的错误处理机制，整体架构稳健且易于长期维护。 diff --git a/gui/src/commands/registry.rs b/gui/src/commands/registry.rs index b38c93e..0e7001d 100644 --- a/gui/src/commands/registry.rs +++ b/gui/src/commands/registry.rs @@ -9,10 +9,22 @@ pub fn load_user_paths() -> Result, String> { registry::load_user_paths() } #[tauri::command] -pub fn save_system_paths(paths: Vec) -> Result<(), String> { +pub fn save_system_paths(paths: Vec, original: Option>) -> Result<(), String> { + if let Some(orig) = original { + let current = registry::load_system_paths()?; + if current != orig { + return Err("注册表已被其他进程修改，请重新加载后重试".to_string()); + } + } registry::save_system_paths(paths) } #[tauri::command] -pub fn save_user_paths(paths: Vec) -> Result<(), String> { +pub fn save_user_paths(paths: Vec, original: Option>) -> Result<(), String> { + if let Some(orig) = original { + let current = registry::load_user_paths()?; + if current != orig { + return Err("注册表已被其他进程修改，请重新加载后重试".to_string()); + } + } registry::save_user_paths(paths) } diff --git a/package-lock.json b/package-lock.json index 42a046b..bc50bcf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,14 +1,15 @@ { "name": "patheditor", - "version": "5.0.0", + "version": "5.1.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "patheditor", - "version": "5.0.0", + "version": "5.1.0", "dependencies": { "@tailwindcss/vite": "^4.3.0", + "@tanstack/react-virtual": "^3.13.26", "@tauri-apps/api": "^2.11.0", "@tauri-apps/plugin-dialog": "^2.7.1", "i18next": "^26.2.0", @@ -1346,6 +1347,33 @@ "vite": "^5.2.0 || ^6 || ^7 || ^8" } }, + "node_modules/@tanstack/react-virtual": { + "version": "3.13.26", + "resolved": "https://registry.npmmirror.com/@tanstack/react-virtual/-/react-virtual-3.13.26.tgz", + "integrity": "sha512-DosdgjOxCLahkn0o+ilmZYwEjo1glfMGuRT/j3PQ18yr5XqA8N/BCaL9IJ3B5TRl+nnzyK2IOFgAILwzN3a9xQ==", + "license": "MIT", + "dependencies": { + "@tanstack/virtual-core": "3.16.0" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/tannerlinsley" + }, + "peerDependencies": { + "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", + "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" + } + }, + "node_modules/@tanstack/virtual-core": { + "version": "3.16.0", + "resolved": "https://registry.npmmirror.com/@tanstack/virtual-core/-/virtual-core-3.16.0.tgz", + "integrity": "sha512-Er2N7q3WOiH6y2JLxsxNX+u2/sLqSsL0bxFgDjuiPiA7vKhZRm+IzcS17vRee3GNXr64UsesA5CAp9yTiIYw9A==", + "license": "MIT", + "funding": { + "type": "github", + "url": "https://github.com/sponsors/tannerlinsley" + } + }, "node_modules/@tauri-apps/api": { "version": "2.11.0", "resolved": "https://registry.npmmirror.com/@tauri-apps/api/-/api-2.11.0.tgz", diff --git a/package.json b/package.json index 933827a..6238a70 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ }, "dependencies": { "@tailwindcss/vite": "^4.3.0", + "@tanstack/react-virtual": "^3.13.26", "@tauri-apps/api": "^2.11.0", "@tauri-apps/plugin-dialog": "^2.7.1", "i18next": "^26.2.0", diff --git a/src/components/path-list/MergePreview.tsx b/src/components/path-list/MergePreview.tsx index d57d32a..af7b47c 100644 --- a/src/components/path-list/MergePreview.tsx +++ b/src/components/path-list/MergePreview.tsx @@ -1,7 +1,8 @@ -import { useMemo } from 'react'; +import { useMemo, useRef } from 'react'; import { useAppStore } from '@/store/app-store'; import { useTranslation } from 'react-i18next'; import type { PathEntry } from '@/core/path-entry'; +import { useVirtualizer } from '@tanstack/react-virtual'; export function MergePreview() { const sysPaths = useAppStore((s) => s.sysPaths); @@ -33,47 +34,62 @@ export function MergePreview() { return merged.filter((r) => r.path.toLowerCase().includes(q)); }, [sysPaths, userPaths, searchQuery, t]); - return ( -
- - - - - - - - - - {allPaths.map(({ path, enabled, source, displayIndex }, rowIdx) => { - const textColor = enabled ? 'var(--app-fg)' : '#6b7280'; - const textDecoration = enabled ? 'none' : 'line-through'; - const opacity = enabled ? 1 : 0.6; + const parentRef = useRef(null); - return ( - parentRef.current, + estimateSize: () => 28, // 预估行高 28px + initialRect: { width: 800, height: 600 }, + }); + + return ( +
+
+
#
+
{t('dialog.pathLabel')}
+
{t('merge.source')}
+
+
+ {rowVirtualizer.getVirtualItems().map((virtualRow) => { + const rowIdx = virtualRow.index; + const { path, enabled, source, displayIndex } = allPaths[rowIdx]; + const textColor = enabled ? 'var(--app-fg)' : '#6b7280'; + const textDecoration = enabled ? 'none' : 'line-through'; + const opacity = enabled ? 1 : 0.6; + + return ( +
+
{rowIdx + 1}
+
-
- - - - ); - })} - -
#{t('dialog.pathLabel')}{t('merge.source')}
{rowIdx + 1} - {path} - {source}
+ {path} +
+
{source}
+ + ); + })} + ); } diff --git a/src/components/path-list/PathTable.tsx b/src/components/path-list/PathTable.tsx index 74a0125..c73cebf 100644 --- a/src/components/path-list/PathTable.tsx +++ b/src/components/path-list/PathTable.tsx @@ -1,9 +1,10 @@ -import { useMemo, useCallback } from 'react'; +import { useMemo, useCallback, useRef } from 'react'; import { useTranslation } from 'react-i18next'; import { useAppStore } from '@/store/app-store'; import { TargetType } from '@/core/undo-redo'; import { usePathValidation } from '@/hooks/use-path-validation'; import type { ValidationState } from '@/hooks/use-path-validation'; +import { useVirtualizer } from '@tanstack/react-virtual'; interface PathTableProps { tabId: 'system' | 'user'; @@ -80,76 +81,91 @@ export function PathTable({ tabId }: PathTableProps) { [isActive, paths], ); + const parentRef = useRef(null); + + const rowVirtualizer = useVirtualizer({ + count: filtered.length, + getScrollElement: () => parentRef.current, + estimateSize: () => 28, // 预估行高 28px + initialRect: { width: 800, height: 600 }, + }); + return ( -
- - - - - - - - - - {filtered.map(({ path, index, enabled }, rowIdx) => { - const v = validations[rowIdx]; - const isSelected = selectedIndices.includes(index); - let textColor = 'var(--app-fg)'; - if (v.state === 'invalid') textColor = '#dc3545'; - else if (v.isDuplicate) textColor = '#fd7e14'; - else if (v.state === 'unknown') textColor = 'var(--app-fg)'; +
+
+
#
+
+
{t('table.path')}
+
+
+ {rowVirtualizer.getVirtualItems().map((virtualRow) => { + const rowIdx = virtualRow.index; + const { path, index, enabled } = filtered[rowIdx]; + const v = validations[rowIdx]; + const isSelected = selectedIndices.includes(index); + let textColor = 'var(--app-fg)'; + if (v.state === 'invalid') textColor = '#dc3545'; + else if (v.isDuplicate) textColor = '#fd7e14'; + else if (v.state === 'unknown') textColor = 'var(--app-fg)'; - let textDecoration = 'none'; - let opacity = 1; - if (!enabled) { - textColor = '#6b7280'; - textDecoration = 'line-through'; - opacity = 0.6; - } + let textDecoration = 'none'; + let opacity = 1; + if (!enabled) { + textColor = '#6b7280'; + textDecoration = 'line-through'; + opacity = 0.6; + } - return ( -
handleClick(index, e)} - onDoubleClick={() => handleDoubleClick(index)} - className="cursor-pointer select-none" - style={{ - backgroundColor: isSelected - ? 'var(--app-select-row)' - : rowIdx % 2 === 0 - ? 'var(--app-list-bg)' - : 'var(--app-list-alt)', - }} + return ( +
handleClick(index, e)} + onDoubleClick={() => handleDoubleClick(index)} + className="cursor-pointer select-none flex items-center absolute top-0 left-0 w-full" + style={{ + height: `${virtualRow.size}px`, + transform: `translateY(${virtualRow.start}px)`, + backgroundColor: isSelected + ? 'var(--app-select-row)' + : rowIdx % 2 === 0 + ? 'var(--app-list-bg)' + : 'var(--app-list-alt)', + }} + > +
+ {index + 1} +
+
+ { + const target = tabId === 'system' ? TargetType.SYSTEM : TargetType.USER; + useAppStore.getState().togglePath(index, target); + }} + className="cursor-pointer" + /> +
+
-
- - - - ); - })} - -
#{t('table.path')}
- {index + 1} - - { - const target = tabId === 'system' ? TargetType.SYSTEM : TargetType.USER; - useAppStore.getState().togglePath(index, target); - }} - className="cursor-pointer" - /> - - {path} -
+ {path} +
+ + ); + })} + ); } diff --git a/src/store/app-store.ts b/src/store/app-store.ts index 3e029b9..e39907a 100644 --- a/src/store/app-store.ts +++ b/src/store/app-store.ts @@ -352,9 +352,12 @@ export const useAppStore = create((set, get) => { await invoke('backup_registry', { customDir: null }) .catch(() => { backupFailed = true; }); + const origSys = state._savedSys.filter(e => e.enabled).map(e => e.path); + const origUser = state._savedUser.filter(e => e.enabled).map(e => e.path); + const [sysResult, userResult] = await Promise.allSettled([ - invoke('save_system_paths', { paths: sysPaths }), - invoke('save_user_paths', { paths: userPaths }), + invoke('save_system_paths', { paths: sysPaths, original: origSys }), + invoke('save_user_paths', { paths: userPaths, original: origUser }), ]); const sysOk = sysResult.status === 'fulfilled'; diff --git a/tests/unit/merge-preview.test.tsx b/tests/unit/merge-preview.test.tsx index 4277a20..596f2eb 100644 --- a/tests/unit/merge-preview.test.tsx +++ b/tests/unit/merge-preview.test.tsx @@ -18,6 +18,21 @@ vi.mock('@/store/app-store', () => ({ }), })); +vi.mock('@tanstack/react-virtual', () => ({ + useVirtualizer: (options: any) => ({ + getVirtualItems: () => { + // return an array of objects to mock virtual items + return Array.from({ length: options.count }).map((_, index) => ({ + index, + start: index * 28, + size: 28, + key: `mock-key-${index}`, + })); + }, + getTotalSize: () => options.count * 28, + }), +})); + vi.mock('@/i18n', () => ({ default: { t: vi.fn((key: string) => key) }, })); diff --git a/tests/unit/path-manager.test.ts b/tests/unit/path-manager.test.ts index 02b6333..cd601a7 100644 --- a/tests/unit/path-manager.test.ts +++ b/tests/unit/path-manager.test.ts @@ -47,12 +47,25 @@ describe('pathClean', () => { expect(removed.length).toBe(1); }); + it('保留第一个出现的 enabled 状态', () => { + const [kept, removed] = pathClean([pe('C:\\Valid', false), pe('C:\\Valid', true)], alwaysValid); + expect(kept.length).toBe(1); + expect(kept[0].enabled).toBe(false); // 第一个状态 + expect(removed.length).toBe(1); + }); + it('全部有效无变化', () => { const [kept, removed] = pathClean([pe('C:\\a'), pe('D:\\b')], alwaysValid); expect(kept.map(e => e.path)).toEqual(['C:\\a', 'D:\\b']); expect(removed.length).toBe(0); }); + it('空数组处理', () => { + const [kept, removed] = pathClean([], alwaysValid); + expect(kept.length).toBe(0); + expect(removed.length).toBe(0); + }); + it('全部无效全部移除', () => { const [kept, removed] = pathClean([pe('C:\\Invalid1'), pe('C:\\Invalid2')], validateFn); expect(kept.length).toBe(0); diff --git a/tests/unit/undo-redo.test.ts b/tests/unit/undo-redo.test.ts index d61134b..6d3848e 100644 --- a/tests/unit/undo-redo.test.ts +++ b/tests/unit/undo-redo.test.ts @@ -125,6 +125,26 @@ describe('UndoRedoManager', () => { expect(mgr.canRedo()).toBe(false); }); + it('空历史栈的撤销与重做', () => { + expect(mgr.undo(sys, user)).toBeNull(); + expect(mgr.redo(sys, user)).toBeNull(); + }); + + it('超出栈底/栈顶的安全处理', () => { + mgr.push(makeRecord(OperationType.ADD, TargetType.SYSTEM, 2, 1, [], [pe('C:\\NewPath')])); + sys.push(pe('C:\\NewPath')); + + // undo一次 + mgr.undo(sys, user); + // 再次undo，此时应到达底部返回null + expect(mgr.undo(sys, user)).toBeNull(); + + // redo一次 + mgr.redo(sys, user); + // 再次redo，应到达顶部返回null + expect(mgr.redo(sys, user)).toBeNull(); + }); + it('超出最大历史容量时移除最旧记录', () => { const small = new UndoRedoManager(3); for (let i = 0; i < 5; i++) {