Conversation
- Remove javascript-obfuscator dependency from package.json - Remove obfuscate.js script - Update ci-build.js to remove obfuscation options - Update smoke-test.js to remove obfuscation detection - Clean up npm scripts related to obfuscation - Update build:prod to skip obfuscation step - Update openspec/project.md to reflect changes This change aligns the project with AGPL-3.0 license requirements by ensuring code transparency for the open source community. Related: remove-code-obfuscation-config
📝 WalkthroughWalkthroughThis pull request introduces major architectural enhancements to Hagico Desktop, including embedded web service lifecycle management, internationalization (i18n) support, dependency detection and installation, shadcn/ui component library integration, Electron menu bar with view switching, and npm mirror configuration for Chinese region users. Additionally, it removes code obfuscation, updates the build system, and expands the IPC surface with new process and state management capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / UI
participant Renderer as Renderer Process
participant Main as Main Process
participant WebService as Web Service
participant FileSystem as File System
User->>Renderer: Click Start Service
activate Renderer
Renderer->>Renderer: Dispatch startWebService action
Renderer->>Main: invoke('start-web-service')
deactivate Renderer
activate Main
Main->>Main: checkPortAvailable()
Main->>Main: emitPhase(CheckingPort)
Main->>Renderer: IPC emit phase change
Main->>FileSystem: loadSavedPort()
Main->>Main: emitPhase(Spawning)
Main->>Renderer: IPC emit phase change
Main->>WebService: spawn process
activate WebService
Main->>Main: emitPhase(WaitingListening)
Main->>Renderer: IPC emit phase change
Main->>Main: waitForPortListening()
Main->>Main: emitPhase(HealthCheck)
Main->>Renderer: IPC emit phase change
Main->>WebService: GET /api/health
WebService-->>Main: 200 OK
Main->>Main: emitPhase(Running)
Main->>Renderer: IPC emit phase change
Main->>FileSystem: saveLastSuccessfulPort()
deactivate WebService
deactivate Main
Renderer->>Renderer: Update UI with phase
Renderer->>User: Show Running status
sequenceDiagram
participant User as User / Settings
participant Renderer as Renderer / i18n
participant Redux as Redux Store
participant Main as Main Process
participant FileSystem as File System
User->>Renderer: Select Chinese (中文)
activate Renderer
Renderer->>Redux: dispatch(changeLanguageRequest('zh-CN'))
activate Redux
Redux->>Main: invoke('set-language', 'zh-CN')
deactivate Redux
activate Main
Main->>FileSystem: Update electron-store (appSettings.language = 'zh-CN')
Main->>Renderer: IPC emit language-changed
deactivate Main
Renderer->>Renderer: i18next.changeLanguage('zh-CN')
Renderer->>Redux: dispatch(setCurrentLanguage('zh-CN'))
activate Redux
Redux->>Renderer: Re-render with new translations
deactivate Redux
Renderer->>User: UI updates to Chinese
deactivate Renderer
sequenceDiagram
participant User as User / Package UI
participant Renderer as Renderer Process
participant Main as Main Process
participant Cache as Local Cache
participant Remote as Remote Source
User->>Renderer: Click Install Package
activate Renderer
Renderer->>Main: invoke('install-web-service-package', version)
deactivate Renderer
activate Main
Main->>Main: checkDiskSpace()
Main->>Renderer: emit package-install-progress {stage: 'downloading'}
Main->>Cache: Check if version cached
alt Version not cached
Main->>Remote: Download package
Main->>Cache: Copy to cache
end
Main->>Renderer: emit package-install-progress {stage: 'extracting'}
Main->>Main: extractPackage()
Main->>Renderer: emit package-install-progress {stage: 'verifying'}
Main->>Main: verifyInstallation()
Main->>Main: setExecutablePermissions()
Main->>FileSystem: updateMeta(packageMeta)
Main->>Renderer: emit package-install-progress {stage: 'completed'}
deactivate Main
Renderer->>Renderer: Update progress UI
Renderer->>User: Show completion alert
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci-build.js (1)
212-249:⚠️ Potential issue | 🔴 CriticalUpdate
getBuildInfo()to report final packaged artifacts instead of intermediate build files.
getBuildInfo()reads from thedist/directory, but electron-builder outputs the final installers topkg/. The build summary will display intermediate build files fromdist/rather than the actual packaged artifacts (installers) that get uploaded. Update the function to read frompkg/to accurately reflect what the workflow deploys.
🤖 Fix all issues with AI agents
In @.github/workflows/build.yml:
- Line 31: Remove all occurrences of the "working-directory: client" directive
in the workflow steps (the nine places where that exact key/value appears)
because the repository has no client/ directory; update the steps that currently
specify "working-directory: client" so they run in the repo root (no
working-directory field), leaving "cache-dependency-path: package-lock.json" and
artifact paths like "pkg/*" untouched; search for the literal
"working-directory: client" and delete that line from each affected job/step
(e.g., the build/test/install steps that reference it).
In
`@openspec/changes/archive/2026-02-04-electron-top-menu-bar-implementation/specs/top-menu-bar/spec.md`:
- Around line 13-20: Update the scenario wording so the startup default is
explicitly a fallback: change the "默认激活'系统管理'菜单项" statement in the Scenario
"应用启动时显示菜单栏" to indicate it only applies when no persisted active view exists
(e.g., "If no persisted active view, default to '系统管理'"); apply the same
clarification to the corresponding duplicated scenario block referenced at lines
90-98 to ensure consistency across the spec.
- Around line 64-85: The scenarios conflict on shortcut scope: update the spec
so CmdOrCtrl+1 and CmdOrCtrl+2 are consistently either global or view-scoped;
specifically, decide whether "使用快捷键切换到系统管理" and "使用快捷键切换到 Hagico Web" remain
global (usable from any view) or adjust those scenarios to state they only work
when the app is in 系统管理视图; also update the "快捷键冲突处理" scenario to reflect that
decision (if global, remove the line "应用菜单快捷键仅在系统管理视图中生效"; if view-scoped, add
explicit precondition to the first two scenarios and clarify priority rules
between Web view shortcuts like CmdOrCtrl+R and menu shortcuts).
In `@src/main/main.ts`:
- Around line 40-42: The BrowserWindow options currently set nodeIntegration:
false and sandbox: false; remove the sandbox: false entry (or set sandbox: true)
inside the BrowserWindow options in the createWindow block so the renderer runs
in a sandboxed process; keep contextIsolation: true and the preload script usage
(contextBridge) unchanged to ensure APIs still work while restoring renderer
isolation.
In `@src/main/package-manager.ts`:
- Around line 284-314: The switch in setExecutablePermissions declares consts
(startScript, binary, macBinary) inside cases which can leak scope across cases;
wrap each case body in its own block (e.g., case 'linux-x64': { ... break; } )
so startScript/binary and macBinary are block-scoped, keep the existing inner
try/catch for the optional binary chmod, and leave the outer catch/log as-is;
ensure each case uses its own { } and break remains inside the block.
- Around line 32-37: The constructor currently assigns a hardcoded developer
path to packageSourcePath which breaks in production; change the constructor in
the package-manager class to read the package source path from a configuration
source (e.g. process.env.PACKAGE_SOURCE_PATH or an app config/CLI arg) and fall
back to a sensible default such as path.join(app.getPath('userData'),
'packages') or a packaged resource location; update the packageSourcePath
assignment and add a basic existence check (fs.existsSync) with a clear
processLogger warning/error if the resolved path is missing so the app can fail
fast or use the fallback.
In `@src/main/web-service-manager.ts`:
- Around line 769-777: The savedPort branch wrongly calls checkPortAvailable()
which tests this.config.port instead of savedPort; update the call to pass
savedPort (e.g., this.checkPortAvailable(savedPort)) and modify the
checkPortAvailable function/method to accept an optional port parameter
(defaulting to this.config.port) and use that parameter for the availability
check so the savedPort is actually validated before assigning it to
this.config.port.
- Around line 51-59: The constructor currently calls initializeSavedPort()
without awaiting, creating a race if start() is invoked before initialization
completes; change the construction flow so initialization is awaited: implement
a static async factory method (e.g., WebServiceManager.create(config)) that
constructs the instance, awaits initializeSavedPort(), and returns the ready
instance, or alternatively modify start() to await this.initializeSavedPort()
(or an internal promise like this._initPromise) before proceeding; update call
sites to use WebServiceManager.create(...) (or rely on start() awaiting the init
promise) and remove the un-awaited call from the constructor to eliminate the
race.
In `@src/renderer/components/PackageManagementCard.tsx`:
- Around line 215-218: In PackageManagementCard, remove the hard-coded developer
path currently rendered alongside {t('packageManagement.noPackagesMessage')}
(the <code className="ml-2 px-2 py-1 bg-muted rounded text-xs"> element) and
instead render either a configurable value (e.g., a prop/state like packagesPath
or a value from app config) or omit the path entirely; update the component to
read the path from the appropriate config/state key and use that variable in
place of the literal string.
- Around line 161-175: availableVersions may be undefined; before using it in
the Select JSX (where value={selectedVersion},
onValueChange={setSelectedVersion}, disabled={isInstalling ||
availableVersions.length === 0} and mapping availableVersions to <SelectItem>),
normalize it to an empty array (e.g., const versions = availableVersions ?? [])
and use versions.length and versions.map instead; apply the same normalization
wherever availableVersions is later used (the other SelectContent/map block) so
.length and .map cannot throw.
In `@src/renderer/components/WebView.tsx`:
- Around line 15-16: The canGoBack/canGoForward state is never updated; add a
useEffect that registers webview navigation listeners on the WebView component
(use the existing webviewRef) to call setCanGoBack(webview.canGoBack()) and
setCanGoForward(webview.canGoForward()) on 'did-navigate' and
'did-navigate-in-page' (and optionally 'did-stop-loading') events, and remove
those listeners in the effect cleanup so the navigation buttons enable/disable
correctly.
- Around line 115-118: The renderer is calling require('electron') inside
handleOpenInBrowser which breaks when contextIsolation/sandbox are enabled;
remove the require usage and instead call an API exposed by the preload script
(e.g., window.myPreloadApi.openExternal or window.electronAPI.openExternal) from
handleOpenInBrowser, and add the corresponding contextBridge exposure in preload
(export a function like openExternal: (url: string) => shell.openExternal(url));
also update/declare the global window type (or use the existing preload
namespace) so TypeScript recognizes the exposed method.
🟡 Minor comments (19)
openspec/changes/archive/2026-02-04-remove-code-obfuscation-config/proposal-files/tasks.md-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorTypo in creation date.
Same as the proposal.md, the date should be "2026-02-04" to match the archive folder naming.
📝 Proposed fix
-**创建时间**: 2025-02-04 +**创建时间**: 2026-02-04openspec/changes/archive/2026-02-04-remove-code-obfuscation-config/proposal-files/tasks.md-386-398 (1)
386-398:⚠️ Potential issue | 🟡 MinorInconsistent task completion status.
The "完成检查清单" (completion checklist) at lines 390-397 shows all items as
[x](completed), but the individual implementation tasks throughout the document (e.g., lines 17, 37, 67, 85, etc.) are still marked as- [ ](unchecked). Based on learnings, tasks should be updated to- [x]after implementation work is complete and confirmed.Either mark all individual tasks as complete if implementation is done, or keep the completion checklist unchecked until tasks are verified.
openspec/changes/archive/2026-02-04-remove-code-obfuscation-config/proposal-files/proposal.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorTypo in creation date.
The creation date shows "2025-02-04" but based on the PR context and archive folder naming (
2026-02-04-remove-code-obfuscation-config), this should be "2026-02-04".📝 Proposed fix
-**创建时间**: 2025-02-04 +**创建时间**: 2026-02-04openspec/changes/archive/2026-02-04-web-service-startup-lifecycle-optimization/web-service-startup-lifecycle-optimization/specs/user-interface/spec.md-192-192 (1)
192-192:⚠️ Potential issue | 🟡 MinorRemove or rename duplicate “## ADDED Requirements” heading.
There’s already an earlier “## ADDED Requirements”; Markdown lint flags this.
✍️ Proposed heading adjustment
-## ADDED Requirements +## ADDED Requirements (continued)openspec/project.md-155-155 (1)
155-155:⚠️ Potential issue | 🟡 MinorFix minor grammar: add “地” after 清晰.
This reads more naturally as “清晰地提交信息/清晰地描述…”.
✍️ Proposed wording fix
-- **提交规范**：使用清晰的提交信息描述变更内容 +- **提交规范**：使用清晰地提交信息描述变更内容openspec/changes/archive/2026-02-04-electron-top-menu-bar-implementation/tasks.md-21-35 (1)
21-35:⚠️ Potential issue | 🟡 MinorAlign Task 1.1 completion status with the unchecked subtask and summary.
Task 1.1 is marked complete while “编写单元测试” remains unchecked, and the P0 completion stats/summary assume all P0 work is done. Please either check off the test subtask or mark Task 1.1 (and the summary counts) as in progress to keep tracking consistent and sequential. Based on learnings: “Update every task in tasks.md to- [x]after all implementation work is complete and confirmed”; “Track implementation progress sequentially in tasks.md with checkboxes, completing one task at a time before moving to the next”.Also applies to: 458-486
openspec/changes/archive/2026-02-04-web-service-port-visibility-enhancement/specs/electron-app/spec.md-118-123 (1)
118-123:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced block.
MD040 requires fenced code blocks to specify a language.🛠️ Suggested fix
-``` +```text Current: Service URL | Process ID | Uptime | Restart Count Updated: Service URL | Process ID | Uptime | Restart Count | Port</details> </blockquote></details> <details> <summary>openspec/changes/archive/2026-02-03-hagico-desktop-i18n-internationalization-support/hagico-desktop-i18n-internationalization-support/design.md-1-9 (1)</summary><blockquote> `1-9`: _⚠️ Potential issue_ | _🟡 Minor_ **Add the required design sections for structure compliance.** Please add explicit Context, Goals/Non‑Goals, Decisions, Risks/Trade‑offs, Migration Plan, and Open Questions sections (brief is fine) so the doc matches the required structure. Based on learnings: “Structure design.md with Context, Goals/Non-Goals, Decisions, Risks/Trade-offs, Migration Plan, and Open Questions sections”. <details> <summary>✅ Suggested skeleton (optional)</summary> ```diff +# Context +# Goals +# Non-Goals +# Decisions +# Risks / Trade-offs +# Migration Plan +# Open Questionssrc/renderer/components/PackageManagementCard.tsx-118-121 (1)
118-121:⚠️ Potential issue | 🟡 MinorLocalize the installed version fallback.
N/Ais hard-coded; use an i18n key to keep translations consistent.openspec/specs/dependency-management/spec.md-3-5 (1)
3-5:⚠️ Potential issue | 🟡 MinorReplace the “TBD” Purpose with the final intent.
Core specs shouldn’t ship with placeholders; define the purpose explicitly.
📝 Proposed update
-## Purpose -TBD - created by archiving change system-dependency-management-panel. Update Purpose after archive. +## Purpose +定义 Hagico Desktop 的依赖管理能力，包括检测、安装引导、状态展示与错误处理。openspec/changes/archive/2026-02-04-web-service-port-visibility-enhancement/proposal.md-9-181 (1)
9-181:⚠️ Potential issue | 🟡 MinorAdd the required proposal sections (Why / What Changes / UI Design / Code Flow / Impact).
The current structure uses Overview/Problem/Solution but omits the mandated headings. Please add those sections and map the existing content into them. As per coding guidelines: “Structure proposal.md with Why, What Changes, UI Design Changes (if applicable), Code Flow Changes (if applicable), and Impact sections”.
openspec/changes/archive/2026-02-04-system-dependency-management-panel/tasks.md-80-110 (1)
80-110:⚠️ Potential issue | 🟡 MinorTasks completion status conflicts with pending tests.
Section 9.5 says all tasks are marked complete, but Section 8 remains unchecked. Either finish and check the tests or keep 9.5 unchecked/adjust the summary.
📝 Minimal fix if tests are still pending
-- [x] 9.5 更新 `tasks.md`，将所有任务标记为完成 +- [ ] 9.5 更新 `tasks.md`，将所有任务标记为完成src/renderer/components/ui/alert.tsx-35-56 (1)
35-56:⚠️ Potential issue | 🟡 MinorFix ref element types for AlertTitle and AlertDescription.
AlertTitleusesHTMLParagraphElementbut renders<h5>(should beHTMLHeadingElement).AlertDescriptionusesHTMLParagraphElementbut renders<div>(should beHTMLDivElementfor both the ref and props generics).🛠️ Suggested fix
-const AlertTitle = React.forwardRef< - HTMLParagraphElement, - React.HTMLAttributes<HTMLHeadingElement> ->(({ className, ...props }, ref) => ( +const AlertTitle = React.forwardRef< + HTMLHeadingElement, + React.HTMLAttributes<HTMLHeadingElement> +>(({ className, ...props }, ref) => ( <h5 ref={ref} className={cn("mb-1 font-medium leading-none tracking-tight", className)} {...props} /> )) -const AlertDescription = React.forwardRef< - HTMLParagraphElement, - React.HTMLAttributes<HTMLParagraphElement> ->(({ className, ...props }, ref) => ( +const AlertDescription = React.forwardRef< + HTMLDivElement, + React.HTMLAttributes<HTMLDivElement> +>(({ className, ...props }, ref) => ( <div ref={ref} className={cn("text-sm [&_p]:leading-relaxed", className)} {...props} /> ))src/main/main.ts-353-356 (1)
353-356:⚠️ Potential issue | 🟡 MinorFragile port parsing from URL.
Parsing the port with
parseInt(status.url.split(':').pop() || '5000')is fragile:
parseIntwithout a radix parameter may produce unexpected results- URLs with paths (e.g.,
http://localhost:5000/path) would fail- Missing port in URL returns
NaN🛠️ Safer port extraction
- const port = status.url ? parseInt(status.url.split(':').pop() || '5000') : 5000; + let port = 5000; + if (status.url) { + try { + const url = new URL(status.url); + port = url.port ? parseInt(url.port, 10) : 5000; + } catch { + port = 5000; + } + }src/renderer/components/DependencyManagementCard.tsx-118-119 (1)
118-119:⚠️ Potential issue | 🟡 MinorAvoid using array index as React key.
Using
indexas the key for mapped elements can cause issues with component state and reconciliation when items are reordered, added, or removed. Use a stable unique identifier likeitem.typeinstead.🐛 Proposed fix
- {dependencies.map((item, index) => ( - <div key={index} className="bg-gray-900/50 rounded-lg p-4 border border-gray-700"> + {dependencies.map((item) => ( + <div key={item.type} className="bg-gray-900/50 rounded-lg p-4 border border-gray-700">src/main/package-manager.ts-79-82 (1)
79-82:⚠️ Potential issue | 🟡 MinorSame version regex limitation here.
🔧 Proposed fix
- const match = packageFilename.match(/^hagicode-([0-9]\.[0-9]\.[0-9](?:-[a-zA-Z0-9\.]+)?)-[a-zA-Z]+-x64\.zip$/); + const match = packageFilename.match(/^hagicode-(\d+\.\d+\.\d+(?:-[a-zA-Z0-9\.]+)?)-[a-zA-Z]+-x64\.zip$/);src/main/package-manager.ts-63-72 (1)
63-72:⚠️ Potential issue | 🟡 MinorVersion regex only matches single-digit version components.
The regex
[0-9]\.[0-9]\.[0-9]only matches versions like1.2.3but will fail for versions like10.0.0or1.23.456.🔧 Proposed fix
- const match = packageFilename.match(/^hagicode-([0-9]\.[0-9]\.[0-9](?:-[a-zA-Z0-9\.]+)?)-([a-zA-Z]+)-x64\.zip$/); + const match = packageFilename.match(/^hagicode-(\d+\.\d+\.\d+(?:-[a-zA-Z0-9\.]+)?)-([a-zA-Z]+)-x64\.zip$/);src/main/web-service-manager.ts-435-441 (1)
435-441:⚠️ Potential issue | 🟡 MinorStatus transition may conflict with exit handler.
The
closehandler sets status to'stopped'if status is'running', but theexithandler (lines 422-433) may have already set it to'error'for unexpected exits. Sinceclosefires afterexit, this could incorrectly overwrite the error state.♻️ Proposed fix
this.process.on('close', () => { log.info('[WebService] Process closed'); this.process = null; - if (this.status === 'running') { - this.status = 'stopped'; - } + // Don't override error status - exit handler already set it + // Only transition to stopped if we were in a clean stopping state + if (this.status === 'stopping') { + this.status = 'stopped'; + } });openspec/changes/archive/2026-02-03-embedded-web-service-management-packaging/embedded-web-service-management-packaging/specs/electron-app/spec.md-82-82 (1)
82-82:⚠️ Potential issue | 🟡 MinorAvoid hardcoded user-specific paths in specifications.
The path
/home/newbe36524/repos/newbe36524/pcode/Release/release-packages/is user-specific and won't work for other developers. Consider using a placeholder like{PROJECT_ROOT}/Release/release-packages/or referencing an environment variable.This same path appears again on line 196.
🧹 Nitpick comments (39)
src/renderer/components/ui/checkbox.tsx (1)
1-1: Consider removing"use client"directive in Electron renderer.The
"use client"directive is specific to React Server Components in Next.js. Since this is an Electron renderer (not a Next.js app), the directive is ignored by the bundler and serves no purpose. It appears this component was copied from a shadcn/ui template designed for Next.js.This is harmless but may cause confusion for developers unfamiliar with the codebase architecture.
🧹 Suggested removal
-"use client" - import * as React from "react"openspec/changes/archive/2026-02-03-hagico-desktop-i18n-internationalization-support/hagico-desktop-i18n-internationalization-support/tasks.md (3)
5-48: Suggest adding dependency installation task and TypeScript type safety.Phase 1 infrastructure setup is well-organized, but consider these additions:
Missing npm package installation task: Add an explicit task for installing required packages before creating configuration files:
i18next- core i18n libraryreact-i18next- React bindings (should be mentioned explicitly since this is a React app)i18next-browser-languagedetector- for automatic browser language detectionTypeScript type safety: Consider adding a task to create TypeScript types for translation keys. This provides autocomplete and compile-time checking, preventing runtime errors from mistyped translation keys. Tools like
i18next-typescriptor manually creating a types file would help.📦 Suggested additional tasks
Add after line 12:
- [ ] **安装 i18n 依赖包** - 包：`i18next`、`react-i18next`、`i18next-browser-languagedetector` - 命令：`npm install i18next react-i18next i18next-browser-languagedetector` - 验证：package.json 中包含正确的依赖版本Add after line 22:
- [ ] **创建 TypeScript 类型定义（可选但推荐）** - 文件：`src/renderer/i18n/types.ts` - 内容：为翻译键创建类型定义，提供类型安全和自动补全 - 验证：IDE 中可以自动补全翻译键
144-189: Consider adding automatic browser language detection.The language switching implementation with Redux and electron-store persistence is solid. However, consider adding automatic browser language detection for new users who haven't set a preference yet. This improves the first-run experience.
🌐 Suggested enhancement
Add a task after line 167:
- [ ] **实现自动语言检测（首次启动）** - 功能：使用 i18next-browser-languagedetector 检测浏览器语言 - 逻辑：如果 electron-store 中没有保存的语言偏好，自动检测并设置 - 回退：如果检测的语言不在支持列表中，使用默认语言 zh-CN - 验证：首次启动时自动选择合适的语言
190-232: Enhance testing strategy with i18n-specific test coverage.The testing plan is comprehensive, but consider adding more i18n-specific testing areas:
Unit testing with i18n mocking: Add guidance for testing components with i18n using
@testing-library/reactand mocked translation functions to verify correct translation keys are used.Advanced i18n features testing: Include tasks for testing:
- Pluralization rules (if applicable)
- Date/time formatting (e.g., different formats for zh-CN vs en-US)
- Number formatting (thousands separators, decimal points)
- Dynamic value interpolation in translations (e.g., "Hello, {{username}}")
🧪 Suggested additional test tasks
Add after line 205:
- [ ] **测试 i18n 组件单元测试** - 操作：使用 `@testing-library/react` 和 i18n 模拟测试组件 - 验证：组件使用正确的翻译键，测试覆盖率满足要求 - [ ] **测试高级 i18n 功能** - 测试项：变量插值、日期格式化、数字格式化、复数规则（如有） - 验证：高级功能在不同语言下正确工作src/renderer/components/settings/LanguageSelector.tsx (1)
17-17: Remove unusedi18nfrom destructuring.The
i18nvariable is destructured fromuseTranslationbut never used in the component.♻️ Suggested fix
- const { t, i18n } = useTranslation('common'); + const { t } = useTranslation('common');openspec/changes/archive/2026-02-03-i18n-internationalization-coverage-enhancement/proposal.md (1)
1-6: Proposal structure differs from documented guidelines.The proposal uses "概述 (Overview)" and "范围 (Scope)" sections instead of the expected "Why" and "What Changes" structure. While the content coverage is adequate, consider aligning future proposals with the standard template for consistency.
Based on learnings: "Structure proposal.md with Why, What Changes, UI Design Changes (if applicable), Code Flow Changes (if applicable), and Impact sections"
src/main/config.ts (1)
63-70: Consider whether exposing the raw store is necessary.The
getStore()method breaks encapsulation by providing direct access to the underlyingelectron-storeinstance. This allows consumers to bypass theConfigManagerAPI and perform uncontrolled reads/writes.If specific functionality is needed (e.g., watching for changes, atomic operations), consider adding dedicated methods instead:
// Example: Add specific methods instead of exposing the store onDidChange<K extends keyof AppConfig>( key: K, callback: (newValue: AppConfig[K], oldValue: AppConfig[K]) => void ): () => void { return this.store.onDidChange(key, callback); }If direct store access is genuinely required for edge cases, document the specific use cases to guide future maintainers.
src/renderer/components/ui/README.md (1)
281-292: Clarify Tailwind CSS v4 configuration approach.Line 283 correctly notes that the project uses Tailwind CSS v4, but Line 291 references
tailwind.config.jsfor customization. In Tailwind v4, the recommended approach is CSS-first configuration using the@themedirective in CSS files rather than JavaScript config.Consider updating the note to reflect v4's preferred configuration method:
-5. **可定制性**: 可以通过修改 `tailwind.config.js` 和 `index.css` 来自定义组件样式。 +5. **可定制性**: 可以通过修改 `index.css` 中的 `@theme` 块来自定义组件样式（Tailwind CSS v4 推荐使用 CSS 配置）。openspec/changes/archive/2026-02-04-electron-top-menu-bar-implementation/proposal.md (1)
50-77: Add language specifier to code block.The architecture diagram code block lacks a language specifier. While this is an ASCII diagram, adding a specifier improves markdown rendering consistency.
📝 Suggested fix
-``` +```text ┌─────────────────────────────────────────────────────────────┐ │ Electron Menu Bar │openspec/changes/archive/2026-02-03-embedded-web-service-management-packaging/embedded-web-service-management-packaging/proposal.md (1)
52-61: Add language specifier to code blocks.The directory structure code blocks at lines 53 and 72 lack language specifiers. Adding
textorplaintextimproves markdown rendering.📝 Suggested fix for line 53
- ``` + ```text release-packages/ hagicode-{version}-linux-x64.zip # Linux 完整包openspec/changes/archive/2026-02-03-hagico-desktop-i18n-internationalization-support/hagico-desktop-i18n-internationalization-support/proposal.md (2)
21-31: Add language specifier to ASCII mockup.The UI mockup code block should have a language specifier for consistent markdown rendering.
📝 Suggested fix
-``` +```text ┌─────────────────────────────────────────┐ │ 语言 / Language │
64-84: Add language specifier to directory structure block.📝 Suggested fix
-``` +```text src/renderer/i18n/ ├── index.ts # i18n 实例初始化和配置openspec/changes/archive/2026-02-03-i18n-internationalization-coverage-enhancement/tasks.md (2)
319-322: Add language identifier to the fenced code block.The code block at line 320 is missing a language identifier. Consider specifying a language (e.g.,
textorplaintext) for consistency and to satisfy linting requirements.💡 Suggested fix
### 格式 -``` +```text <namespace>:<category>.<item>[(.<subitem>)]</details> --- `360-369`: **Completion criteria are marked complete, but individual task items lack checkboxes.** The completion criteria section shows all items as `[x]` (complete), but the individual tasks throughout phases 1-4 (e.g., Task 1.1, Task 2.1) don't have checkbox markers to track their completion status. Based on learnings, tasks should be tracked sequentially with checkboxes (`- [ ]` / `- [x]`). Consider adding checkbox markers to each task header for consistent progress tracking, or clarify if the completion criteria section alone is intended to serve this purpose. </blockquote></details> <details> <summary>openspec/changes/archive/2026-02-04-remove-code-obfuscation-config/proposal-files/specs/build-system/spec.md (2)</summary><blockquote> `134-144`: **Minor formatting inconsistency in scenario steps.** Lines 139-143 use multiple consecutive `**Then**` statements. For consistency with BDD-style scenarios used elsewhere in this file, subsequent conditions after the first `**Then**` should use `**And**`. <details> <summary>📝 Proposed fix</summary> ```diff **Given** 开发者完成构建 **When** 执行 `npm run smoke-test` **Then** 验证 dist/ 目录结构正确 -**Then** 验证主进程文件存在且语法有效 -**Then** 验证渲染进程文件存在 -**Then** 验证预加载脚本存在 -**Then** 验证 package.json 配置正确 -**Then** 验证 electron-builder 配置有效 +**And** 验证主进程文件存在且语法有效 +**And** 验证渲染进程文件存在 +**And** 验证预加载脚本存在 +**And** 验证 package.json 配置正确 +**And** 验证 electron-builder 配置有效 **And** 所有测试通过后才能发布
146-153: Same Then/And formatting inconsistency.Similar to the previous scenario, lines 150-152 use consecutive
**Then**statements.📝 Proposed fix
**Given** 开发者提交代码或执行构建 **When** 运行质量检查脚本 **Then** TypeScript 类型检查通过 -**Then** ESLint 检查通过 (如配置) -**Then** 构建无警告或错误 +**And** ESLint 检查通过 (如配置) +**And** 构建无警告或错误 **And** 不检查代码混淆特征 (已移除此测试)openspec/changes/archive/2026-02-04-electron-top-menu-bar-implementation/design.md (2)
51-64: Missing language specifier for fenced code block.This code block showing the menu structure doesn't have a language specifier. Consider using
textorplaintextfor ASCII diagrams.📝 Proposed fix
-``` +```text 菜单结构（Windows/Linux）: ┌─────────────────────────────────────────────────────┐
430-443: Missing language specifier for file listing blocks.These fenced code blocks listing file paths should have a language specifier for consistency.
📝 Proposed fix for lines 430-443, 446-452, 455-459
-``` +```text src/main/ menu-manager.ts # 菜单管理器Apply similar changes to the other file listing blocks at lines 446 and 455.
src/renderer/components/ui/badge.tsx (1)
30-34: Consider adding forwardRef for ref forwarding.The Badge component doesn't use
React.forwardRef, which may limit its flexibility when consumers need to attach refs (e.g., for focus management, tooltips, or measurement). Other shadcn/ui components typically useforwardReffor consistency.♻️ Proposed refactor with forwardRef
-function Badge({ className, variant, ...props }: BadgeProps) { - return ( - <div className={cn(badgeVariants({ variant }), className)} {...props} /> - ) -} +const Badge = React.forwardRef<HTMLDivElement, BadgeProps>( + ({ className, variant, ...props }, ref) => { + return ( + <div + ref={ref} + className={cn(badgeVariants({ variant }), className)} + {...props} + /> + ) + } +) +Badge.displayName = "Badge"src/renderer/components/ui/button.tsx (1)
42-49: Default non-submit button type to avoid accidental submits.HTML buttons default to
submitinside forms. Consider defaulting tobuttonwhenasChildis false, while still letting callers override.♻️ Suggested change
-const Button = React.forwardRef<HTMLButtonElement, ButtonProps>( - ({ className, variant, size, asChild = false, ...props }, ref) => { +const Button = React.forwardRef<HTMLButtonElement, ButtonProps>( + ({ className, variant, size, asChild = false, type, ...props }, ref) => { const Comp = asChild ? Slot : "button" return ( <Comp className={cn(buttonVariants({ variant, size, className }))} ref={ref} + type={asChild ? undefined : type ?? "button"} {...props} /> ) } )openspec/changes/archive/2026-02-03-hagico-desktop-i18n-internationalization-support/hagico-desktop-i18n-internationalization-support/specs/internationalization/spec.md (1)
128-146: Inconsistency in default/fallback language configuration.Line 18 specifies zh-CN as the default language, but line 130 specifies en-US as the fallback language. This is valid (default ≠ fallback), but the English text at line 130 states "automatically fall back to the default language (en-US)" which is confusing since zh-CN was established as the default.
Consider clarifying: the fallback language (en-US) is distinct from the default language (zh-CN). Update the wording to avoid conflating these two concepts.
src/main/main.ts (2)
45-46: Avoid polluting the global namespace withmainWindow.Using
(global as any).mainWindowis a code smell that bypasses TypeScript's type safety and couples code through global state. Consider:
- Passing
mainWindowas a parameter to managers that need it- Using a module-level singleton pattern with proper exports
- Using IPC channels instead of direct window references
635-651: Async cleanup inbefore-quitmay not complete reliably.The pattern of calling
event.preventDefault()followed by async cleanup andapp.exit(0)can have issues:
- If cleanup throws before
finally, the error is logged butapp.exit(0)still runs- Multiple
before-quittriggers could cause race conditionsThe current implementation handles this reasonably with the
finallyblock, but consider adding a guard flag to prevent multiple cleanup attempts:♻️ Add cleanup guard
+let isQuitting = false; + app.on('before-quit', async (event) => { + if (isQuitting) return; + isQuitting = true; + event.preventDefault(); // ... rest of cleanup });src/renderer/components/SystemManagementView.tsx (1)
8-18: Movewindow.electronAPItype declaration to a shared types file.Declaring the
electronAPIinterface locally leads to:
- Potential inconsistencies if the API changes
- Duplicate declarations across components
- Maintenance burden
Consider creating a shared
src/renderer/types/electron.d.tsfile:// src/renderer/types/electron.d.ts export interface ElectronAPI { getAppVersion: () => Promise<string>; onServerStatusChange: (callback: (status: ServerStatus) => void) => () => void; // ... other methods } declare global { interface Window { electronAPI: ElectronAPI; } }Note: The
onServerStatusChangereturn type should be() => void(cleanup function) based on the usage at line 39-46, notvoid.src/renderer/components/WebView.tsx (1)
39-44: Use proper TypeScript types instead ofany.The event handlers use
anytype which bypasses TypeScript's type checking:
- Line 39:
event: anyshould be typed as the Electron webview'sDidFailLoadEvent- Line 58:
event: anyfor custom event should beCustomEvent<{ direction: string }>♻️ Add proper types
- const handleDidFailLoad = (event: any) => { + const handleDidFailLoad = (event: Electron.DidFailLoadEvent) => { if (event.isTopLevel) { // ... } }; - const handleNavigate = (event: any, direction: 'back' | 'forward' | 'refresh') => { + const handleNavigate = (event: Event) => { + const direction = (event as CustomEvent<'back' | 'forward' | 'refresh'>).detail; // ... };Also applies to: 58-72
src/main/npm-mirror-helper.ts (1)
22-46: Refine Chinese locale detection to target mainland China only.The
CHINESE_LOCALESarray includeszh-TW,zh-HK, andzh-SG. However, users in Taiwan, Hong Kong, and Singapore have reliable access to the official npm registry via Cloudflare's regional edge locations (Taipei, Hong Kong, Singapore). The Taobao mirror is designed and optimized for mainland China networks, where cross-border access to the official registry is often slow or unreliable. Taiwan, Hong Kong, and Singapore are not primary targets for mirror usage.Additionally,
app.getLocale()may return locale variants with different separators or language subtags (e.g.,zh-Hans-CN,zh_CN). Consider using a prefix match to handle these variants:♻️ Suggested improvement
-const CHINESE_LOCALES = ['zh-CN', 'zh-TW', 'zh-HK', 'zh-SG']; +// Only mainland China benefits from Taobao mirror +const CHINA_MAINLAND_LOCALE_PREFIXES = ['zh-CN', 'zh_CN', 'zh-Hans']; detectRegion(): Region { try { const locale = app.getLocale(); console.log(`[NpmMirrorHelper] System locale detected: ${locale}`); - if (CHINESE_LOCALES.includes(locale)) { + // Check if locale indicates mainland China + if (CHINA_MAINLAND_LOCALE_PREFIXES.some(prefix => locale.startsWith(prefix))) { return 'CN'; }src/renderer/App.tsx (2)
27-27: Unused translation hook.The
tfunction fromuseTranslation('common')is destructured but never used in the component's render output. Either remove it or use it for any text that should be localized.♻️ Proposed fix
function App() { - const { t } = useTranslation('common'); + useTranslation('common'); // Initialize i18n namespace for child components const dispatch = useDispatch();Or remove the import and hook entirely if not needed.
48-48: Consider extracting hardcoded fallback URL to a constant.The fallback URL
'http://localhost:36556'is hardcoded. Consider extracting this to a named constant for better maintainability and to ensure consistency if this default is used elsewhere.♻️ Proposed fix
+const DEFAULT_WEB_SERVICE_URL = 'http://localhost:36556'; + function App() { // ... return ( <div className="min-h-screen bg-gradient-to-br from-gray-900 via-gray-800 to-gray-900 text-white"> {currentView === 'system' && <SystemManagementView />} - {currentView === 'web' && <WebView src={webServiceUrl || 'http://localhost:36556'} />} + {currentView === 'web' && <WebView src={webServiceUrl || DEFAULT_WEB_SERVICE_URL} />} </div> ); }src/renderer/components/DependencyManagementCard.tsx (4)
44-84: Replaceanytype with proper interface for type safety.The helper functions
getStatusIcon,getStatusText, andgetStatusColorall acceptitem: any, which loses TypeScript's type safety benefits. Define or import a proper interface for dependency items.♻️ Proposed fix
+interface DependencyItem { + name: string; + type: DependencyType; + installed: boolean; + versionMismatch?: boolean; + version?: string; + requiredVersion?: string; + description?: string; + installCommand?: string; + downloadUrl?: string; +} + - const getStatusIcon = (item: any) => { + const getStatusIcon = (item: DependencyItem) => { // ... }; - const getStatusText = (item: any) => { + const getStatusText = (item: DependencyItem) => { // ... }; - const getStatusColor = (item: any) => { + const getStatusColor = (item: DependencyItem) => { // ... };
27-38: Consider using typed action creators instead of plain action objects.Dispatching plain action objects like
{ type: 'dependency/fetchDependencies' }bypasses TypeScript's type checking. If action creators exist in the slice, prefer using them for better type safety and refactoring support.♻️ Proposed approach
// If action creators are exported from dependencySlice: import { fetchDependencies, installDependency } from '../store/slices/dependencySlice'; // Then use: dispatch(fetchDependencies()); dispatch(installDependency(dependencyType));If these are saga-triggered actions, consider creating typed action creators for them.
156-172: Per-dependency installing state would improve UX.The
installingstate appears to be global, which disables all install buttons when any dependency is being installed. Consider tracking installation state per dependency type to allow parallel installs or at least show which specific dependency is being installed.
11-18: Consolidate Window.electronAPI type declarations.This global
Window.electronAPIdeclaration duplicates similar declarations inApp.tsx. Consider centralizing all electronAPI type definitions in a single.d.tsfile (e.g.,src/types/electron.d.ts) to avoid inconsistencies and maintenance overhead.src/main/package-manager.ts (1)
244-246:extractAllTois synchronous and may block the event loop.For large archives, the synchronous
zip.extractAllTo()call will block the main process event loop. Consider using the async extraction method or running in a worker thread.♻️ Suggested improvement using async extraction
- // Extract zip - const zip = new AdmZip(zipPath); - zip.extractAllTo(targetPath, true); + // Extract zip asynchronously to avoid blocking event loop + const zip = new AdmZip(zipPath); + await new Promise<void>((resolve, reject) => { + zip.extractAllToAsync(targetPath, true, false, (error) => { + if (error) reject(error); + else resolve(); + }); + });src/main/dependency-manager.ts (2)
1-6: Consider using electron-log for consistent logging.This file uses
console.log/console.error/console.warnwhilepackage-manager.tsuseselectron-log. For consistency and better log management in production Electron apps, consider usingelectron-log.♻️ Proposed fix
import { exec } from 'node:child_process'; import { promisify } from 'node:util'; import { NpmMirrorHelper } from './npm-mirror-helper.js'; import Store from 'electron-store'; +import log from 'electron-log'; const execAsync = promisify(exec);Then replace
console.log→log.info,console.warn→log.warn,console.error→log.errorthroughout the file.
49-56: Linux package manager fallback chain may produce confusing error output.The fallback chain
sudo apt install ... || sudo dnf install ... || sudo yum install ... || sudo pacman -S ...will execute sequentially until one succeeds. While functionally correct, failed attempts will produce error output that may confuse users even when installation ultimately succeeds via a later package manager.Consider detecting the available package manager first (similar to line 50's check command) and only running the appropriate install command, or suppress stderr from failed fallback attempts.
src/preload/index.ts (1)
7-13: Consider adding TypeScript type annotations for callbacks.The callback parameters lack type annotations, which reduces type safety. This pattern is repeated across all listener-based APIs (
onServerStatusChange,onWebServiceStatusChange,onPackageInstallProgress,onDependencyStatusChange,onViewChange).♻️ Example fix for type safety
+type ServerStatus = 'running' | 'stopped' | 'error'; +type WebServiceStatus = { status: string; pid: number | null; /* ... */ }; + const electronAPI = { // ... - onServerStatusChange: (callback) => { - const listener = (_event, status) => { + onServerStatusChange: (callback: (status: ServerStatus) => void) => { + const listener = (_event: Electron.IpcRendererEvent, status: ServerStatus) => { callback(status); }; ipcRenderer.on('server-status-changed', listener); return () => ipcRenderer.removeListener('server-status-changed', listener); },src/main/web-service-manager.ts (3)
184-190: Fragile global access for IPC communication.Using
(global as any).mainWindowis fragile and breaks encapsulation. Consider using dependency injection or an event emitter pattern.♻️ Suggested approach using callback
export class PCodeWebServiceManager { + private onPhaseChange?: (phase: StartupPhase, message?: string) => void; + - constructor(config: WebServiceConfig) { + constructor(config: WebServiceConfig, options?: { onPhaseChange?: (phase: StartupPhase, message?: string) => void }) { this.config = config; this.userDataPath = app.getPath('userData'); + this.onPhaseChange = options?.onPhaseChange; // ... } private emitPhase(phase: StartupPhase, message?: string): void { this.currentPhase = phase; - if ((global as any).mainWindow) { - (global as any).mainWindow.webContents.send('web-service-startup-phase', { - phase, - message, - timestamp: Date.now() - }); - } + this.onPhaseChange?.(phase, message); log.info('[WebService] Phase:', phase, message || ''); }
105-124: Use proper TypeScript type instead ofany.The spawn options should use the proper
SpawnOptionstype fromchild_process.♻️ Proposed fix
+import { spawn, ChildProcess, SpawnOptions } from 'child_process'; + private getSpawnOptions() { const platform = process.platform; const executablePath = this.getExecutablePath(); - const options: any = { + const options: SpawnOptions = { env: { ...process.env, ...this.config.env }, cwd: path.dirname(executablePath), + stdio: platform === 'win32' ? undefined : 'ignore', + detached: platform === 'win32', + windowsHide: platform === 'win32', }; - - if (platform === 'win32') { - options.detached = true; - options.windowsHide = true; - } else { - options.detached = false; - options.stdio = 'ignore'; - } return options; }
510-516: Unnecessary dynamic import.
spawnis already imported at the top of the file (line 1). The dynamic import here is redundant.♻️ Proposed fix
if (platform === 'win32') { // Windows: use taskkill to terminate process tree - const { spawn } = await import('child_process'); spawn('taskkill', ['/F', '/T', '/PID', pid.toString()], { stdio: 'ignore', });
| node-version: ${{ env.NODE_VERSION }} | ||
| cache: "npm" | ||
| cache-dependency-path: client/package-lock.json | ||
| cache-dependency-path: package-lock.json |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where build artifacts are output and if package-lock.json exists at expected locations
echo "=== Checking package-lock.json locations ==="
fd -t f "package-lock.json" --max-depth 3
echo -e "\n=== Checking for build/output related files and directories ==="
fd -t d "pkg|dist|build" --max-depth 2 | head -20
echo -e "\n=== Checking build script for output directory configuration ==="
fd -t f "build.sh" -x cat {} \; 2>/dev/null | head -100
echo -e "\n=== Checking package.json scripts section in client/ ==="
if [ -f "client/package.json" ]; then
cat client/package.json | grep -A 10 '"scripts"' | head -20
fi
echo -e "\n=== Checking root package.json ==="
if [ -f "package.json" ]; then
cat package.json | grep -A 10 '"scripts"' | head -20
fiRepository: HagiCode-org/desktop
Length of output: 904
🏁 Script executed:
#!/bin/bash
echo "=== Reading .github/workflows/build.yml around line 31 and 59 ==="
cat -n .github/workflows/build.yml | sed -n '25,65p'
echo -e "\n=== Checking vite configuration for output directory ==="
fd -t f "vite*.config*" -x head -50 {}
echo -e "\n=== Checking entire artifact paths section in workflow ==="
cat -n .github/workflows/build.yml | grep -A 2 -B 2 "artifact\|cache-dependency"Repository: HagiCode-org/desktop
Length of output: 5863
🏁 Script executed:
#!/bin/bash
echo "=== Checking for package-lock.json in client directory ==="
ls -la client/package-lock.json 2>&1
echo -e "\n=== Checking client/scripts/ci-build.js for artifact output path ==="
if [ -f "client/scripts/ci-build.js" ]; then
cat client/scripts/ci-build.js | head -100
else
echo "ci-build.js not found, listing client/scripts directory:"
ls -la client/scripts/
fi
echo -e "\n=== Checking if there are any existing pkg directories ==="
find . -type d -name "pkg" 2>/dev/null
echo -e "\n=== Checking full vite config path ==="
fd -t f "vite*.config*" -x dirname {}Repository: HagiCode-org/desktop
Length of output: 503
🏁 Script executed:
#!/bin/bash
echo "=== Listing directory structure ==="
ls -la | head -20
echo -e "\n=== Checking if client directory exists ==="
ls -la client 2>&1 | head -30
echo -e "\n=== Looking for all directories at root ==="
ls -d */ 2>/dev/null
echo -e "\n=== Looking for vite config files ==="
ls -la vite*.config.* 2>/dev/null
echo -e "\n=== Checking scripts directory at root ==="
ls -la scripts/ 2>/dev/null | head -20Repository: HagiCode-org/desktop
Length of output: 2003
🏁 Script executed:
#!/bin/bash
echo "=== Reading ci-build.js to understand artifact output ==="
cat scripts/ci-build.js | head -150
echo -e "\n=== Checking electron-builder config for output paths ==="
cat electron-builder.ymlRepository: HagiCode-org/desktop
Length of output: 4396
🏁 Script executed:
#!/bin/bash
echo "=== Checking git history for changes to build.yml ==="
git log --oneline -20 .github/workflows/build.yml
echo -e "\n=== Showing the actual diff for build.yml ==="
git diff HEAD~1 .github/workflows/build.yml | head -200Repository: HagiCode-org/desktop
Length of output: 414
🏁 Script executed:
#!/bin/bash
echo "=== Checking current working directory references in build.yml ==="
grep -n "working-directory:" .github/workflows/build.yml
echo -e "\n=== Verifying the structure: does client directory exist? ==="
[ -d "client" ] && echo "client directory EXISTS" || echo "client directory DOES NOT EXIST"
echo -e "\n=== Full artifact path references in workflow ==="
grep -n "path:" .github/workflows/build.yml | head -20Repository: HagiCode-org/desktop
Length of output: 1036
Remove working-directory: client — the client directory does not exist in the repository.
The workflow references a client/ directory in 9 build steps (lines 35, 43, 47, 101, 109, 113, 166, 174, 178), but this directory does not exist in the repository. The actual structure has scripts/ and src/ at the root level. These working-directory directives will cause workflow failures when attempting to change into a non-existent directory.
The cache-dependency-path: package-lock.json and artifact paths pkg/* are correct—they align with the root-level package-lock.json and the electron-builder.yml output configuration (output: pkg). Only the working-directory: client references need to be removed.
🤖 Prompt for AI Agents
In @.github/workflows/build.yml at line 31, Remove all occurrences of the
"working-directory: client" directive in the workflow steps (the nine places
where that exact key/value appears) because the repository has no client/
directory; update the steps that currently specify "working-directory: client"
so they run in the repo root (no working-directory field), leaving
"cache-dependency-path: package-lock.json" and artifact paths like "pkg/*"
untouched; search for the literal "working-directory: client" and delete that
line from each affected job/step (e.g., the build/test/install steps that
reference it).
| #### Scenario: 应用启动时显示菜单栏 | ||
|
|
||
| **Given** 应用正在启动 | ||
| **When** 主窗口创建完成 | ||
| **Then** 菜单栏在窗口顶部（Windows/Linux）或系统菜单栏（macOS）显示 | ||
| **And** 菜单项文本使用当前选择的语言（中文或英文） | ||
| **And** 默认激活"系统管理"菜单项 | ||
|
|
There was a problem hiding this comment.
Resolve startup default vs persisted view behavior.
The startup scenario forces “系统管理” as default, but the persistence requirement restores the last active view on restart. Clarify that the default applies only when no persisted view exists.
🛠️ Proposed clarification
-**And** 默认激活"系统管理"菜单项
+**And** 若存在上次保存的视图，则默认激活该视图；否则默认激活"系统管理"菜单项Also applies to: 90-98
🤖 Prompt for AI Agents
In
`@openspec/changes/archive/2026-02-04-electron-top-menu-bar-implementation/specs/top-menu-bar/spec.md`
around lines 13 - 20, Update the scenario wording so the startup default is
explicitly a fallback: change the "默认激活'系统管理'菜单项" statement in the Scenario
"应用启动时显示菜单栏" to indicate it only applies when no persisted active view exists
(e.g., "If no persisted active view, default to '系统管理'"); apply the same
clarification to the corresponding duplicated scenario block referenced at lines
90-98 to ensure consistency across the spec.
| #### Scenario: 使用快捷键切换到系统管理 | ||
|
|
||
| **Given** 用户当前在任意视图 | ||
| **When** 用户按下 `CmdOrCtrl+1` | ||
| **Then** 应用切换到系统管理视图 | ||
| **And** 焦点保持在主窗口 | ||
|
|
||
| #### Scenario: 使用快捷键切换到 Hagico Web | ||
|
|
||
| **Given** 用户当前在任意视图 | ||
| **When** 用户按下 `CmdOrCtrl+2` | ||
| **Then** 应用检查 Web 服务状态 | ||
| **And** 如果服务运行中，切换到 Web 视图 | ||
| **And** 如果服务未运行，显示启动确认对话框 | ||
|
|
||
| #### Scenario: 快捷键冲突处理 | ||
|
|
||
| **Given** 应用定义了 `CmdOrCtrl+1` 和 `CmdOrCtrl+2` 快捷键 | ||
| **When** 用户在 Web 视图中按下浏览器快捷键（如 `CmdOrCtrl+R`） | ||
| **Then** Web 视图的快捷键优先 | ||
| **And** 应用菜单快捷键仅在系统管理视图中生效 | ||
|
|
There was a problem hiding this comment.
Align shortcut scope with view-switch scenarios.
Earlier scenarios allow switching from any view, but this scenario says app menu shortcuts only work in 系统管理视图. Either keep view-switch shortcuts global or update the earlier scenario.
🛠️ Proposed clarification
-**Then** Web 视图的快捷键优先
-**And** 应用菜单快捷键仅在系统管理视图中生效
+**Then** Web 视图的常规快捷键优先
+**And** 视图切换快捷键（CmdOrCtrl+1/2）保持全局生效🤖 Prompt for AI Agents
In
`@openspec/changes/archive/2026-02-04-electron-top-menu-bar-implementation/specs/top-menu-bar/spec.md`
around lines 64 - 85, The scenarios conflict on shortcut scope: update the spec
so CmdOrCtrl+1 and CmdOrCtrl+2 are consistently either global or view-scoped;
specifically, decide whether "使用快捷键切换到系统管理" and "使用快捷键切换到 Hagico Web" remain
global (usable from any view) or adjust those scenarios to state they only work
when the app is in 系统管理视图; also update the "快捷键冲突处理" scenario to reflect that
decision (if global, remove the line "应用菜单快捷键仅在系统管理视图中生效"; if view-scoped, add
explicit precondition to the first two scenarios and clarify priority rules
between Web view shortcuts like CmdOrCtrl+R and menu shortcuts).
| nodeIntegration: false, | ||
| sandbox: true, | ||
| sandbox: false, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the main.ts file around the lines mentioned
cat -n src/main/main.ts | head -80Repository: HagiCode-org/desktop
Length of output: 3704
🏁 Script executed:
# Check if a preload script is being used
grep -n "preload" src/main/main.tsRepository: HagiCode-org/desktop
Length of output: 124
🏁 Script executed:
# Look for any comments explaining the sandbox: false setting
grep -n -B5 -A5 "sandbox.*false" src/main/main.tsRepository: HagiCode-org/desktop
Length of output: 423
🏁 Script executed:
# Search for IPC handlers to understand what functionality they provide
grep -n "ipcMain" src/main/main.ts | head -20Repository: HagiCode-org/desktop
Length of output: 1142
🏁 Script executed:
# Check if there are any renderer process files that might require direct Node access
find src -name "*.ts" -o -name "*.js" | grep -E "(render|preload)" | head -10Repository: HagiCode-org/desktop
Length of output: 395
🏁 Script executed:
# Check the preload script
cat -n src/preload/index.tsRepository: HagiCode-org/desktop
Length of output: 3768
🏁 Script executed:
# Search for any comments or documentation about the sandbox setting
grep -r "sandbox" src/main/ --include="*.ts" --include="*.js" -B2 -A2Repository: HagiCode-org/desktop
Length of output: 243
🏁 Script executed:
# Check if there are any require() calls in the renderer code that would need sandbox: false
grep -r "require\|electron.app\|process\.exit" src/renderer/ --include="*.ts" --include="*.js" | head -20Repository: HagiCode-org/desktop
Length of output: 136
Remove sandbox: false to enable renderer process isolation.
The preload script properly exposes all necessary APIs through contextBridge and IPC patterns, making the sandbox setting unnecessary. With contextIsolation: true, nodeIntegration: false, and context bridge, the renderer process has no need for direct system access. Setting sandbox: true (or removing the setting to use the default) will improve security without breaking functionality.
🤖 Prompt for AI Agents
In `@src/main/main.ts` around lines 40 - 42, The BrowserWindow options currently
set nodeIntegration: false and sandbox: false; remove the sandbox: false entry
(or set sandbox: true) inside the BrowserWindow options in the createWindow
block so the renderer runs in a sandboxed process; keep contextIsolation: true
and the preload script usage (contextBridge) unchanged to ensure APIs still work
while restoring renderer isolation.
| constructor() { | ||
| this.userDataPath = app.getPath('userData'); | ||
| // Local development path | ||
| this.packageSourcePath = '/home/newbe36524/repos/newbe36524/pcode/Release/release-packages/'; | ||
| this.currentPlatform = this.detectPlatform(); | ||
| } |
There was a problem hiding this comment.
Hardcoded local development path will break in production.
The packageSourcePath is hardcoded to a specific developer's local directory. This will fail for all other users and environments.
🔧 Proposed fix: Make the path configurable
constructor() {
this.userDataPath = app.getPath('userData');
- // Local development path
- this.packageSourcePath = '/home/newbe36524/repos/newbe36524/pcode/Release/release-packages/';
+ // Default to a path relative to userData, can be overridden via config
+ this.packageSourcePath = path.join(this.userDataPath, 'pcode-web', 'packages');
this.currentPlatform = this.detectPlatform();
}
+
+/**
+ * Set custom package source path (for development or custom deployments)
+ */
+setPackageSourcePath(sourcePath: string): void {
+ this.packageSourcePath = sourcePath;
+}🤖 Prompt for AI Agents
In `@src/main/package-manager.ts` around lines 32 - 37, The constructor currently
assigns a hardcoded developer path to packageSourcePath which breaks in
production; change the constructor in the package-manager class to read the
package source path from a configuration source (e.g.
process.env.PACKAGE_SOURCE_PATH or an app config/CLI arg) and fall back to a
sensible default such as path.join(app.getPath('userData'), 'packages') or a
packaged resource location; update the packageSourcePath assignment and add a
basic existence check (fs.existsSync) with a clear processLogger warning/error
if the resolved path is missing so the app can fail fast or use the fallback.
| if (savedPort && savedPort !== this.config.port) { | ||
| // Check if saved port is available | ||
| const available = await this.checkPortAvailable(); | ||
| if (available) { | ||
| log.info('[WebService] Using saved port:', savedPort); | ||
| this.config.port = savedPort; | ||
| } else { | ||
| log.warn('[WebService] Saved port unavailable, using default:', this.config.port); | ||
| } |
There was a problem hiding this comment.
Bug: Port availability check uses wrong port.
The code checks if the current configured port is available instead of checking if the saved port is available. This defeats the purpose of loading a saved port.
🐛 Proposed fix
const savedPort = await this.loadSavedPort();
if (savedPort && savedPort !== this.config.port) {
- // Check if saved port is available
- const available = await this.checkPortAvailable();
+ // Check if saved port is available by temporarily setting it
+ const originalPort = this.config.port;
+ this.config.port = savedPort;
+ const available = await this.checkPortAvailable();
if (available) {
log.info('[WebService] Using saved port:', savedPort);
- this.config.port = savedPort;
+ // Port already set above
} else {
+ this.config.port = originalPort;
log.warn('[WebService] Saved port unavailable, using default:', this.config.port);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (savedPort && savedPort !== this.config.port) { | |
| // Check if saved port is available | |
| const available = await this.checkPortAvailable(); | |
| if (available) { | |
| log.info('[WebService] Using saved port:', savedPort); | |
| this.config.port = savedPort; | |
| } else { | |
| log.warn('[WebService] Saved port unavailable, using default:', this.config.port); | |
| } | |
| if (savedPort && savedPort !== this.config.port) { | |
| // Check if saved port is available by temporarily setting it | |
| const originalPort = this.config.port; | |
| this.config.port = savedPort; | |
| const available = await this.checkPortAvailable(); | |
| if (available) { | |
| log.info('[WebService] Using saved port:', savedPort); | |
| // Port already set above | |
| } else { | |
| this.config.port = originalPort; | |
| log.warn('[WebService] Saved port unavailable, using default:', this.config.port); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/main/web-service-manager.ts` around lines 769 - 777, The savedPort branch
wrongly calls checkPortAvailable() which tests this.config.port instead of
savedPort; update the call to pass savedPort (e.g.,
this.checkPortAvailable(savedPort)) and modify the checkPortAvailable
function/method to accept an optional port parameter (defaulting to
this.config.port) and use that parameter for the availability check so the
savedPort is actually validated before assigning it to this.config.port.
| <Select | ||
| value={selectedVersion} | ||
| onValueChange={setSelectedVersion} | ||
| disabled={isInstalling || availableVersions.length === 0} | ||
| > | ||
| <SelectTrigger id="version-select"> | ||
| <SelectValue placeholder={t('packageManagement.versionPlaceholder')} /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {availableVersions.map((ver) => ( | ||
| <SelectItem key={ver} value={ver}> | ||
| {ver} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> |
There was a problem hiding this comment.
Normalize availableVersions before using .length/.map.
You later treat availableVersions as possibly undefined, but .length and .map above will throw if it is. Normalize to an empty array to avoid a runtime crash.
🛠️ Suggested fix
- const { packageInfo, installProgress, isInstalling, availableVersions, platform } = useSelector((state: RootState) => selectPackageManagementInfo(state));
+ const { packageInfo, installProgress, isInstalling, availableVersions, platform } = useSelector((state: RootState) => selectPackageManagementInfo(state));
+ const safeVersions = availableVersions ?? [];
...
- disabled={isInstalling || availableVersions.length === 0}
+ disabled={isInstalling || safeVersions.length === 0}
...
- {availableVersions.map((ver) => (
+ {safeVersions.map((ver) => (
<SelectItem key={ver} value={ver}>
{ver}
</SelectItem>
))}
...
- {!availableVersions || availableVersions.length === 0 ? (
+ {safeVersions.length === 0 ? (
<Alert>
...
- <div className="text-xs text-muted-foreground">
- {t('packageManagement.availableVersionsCount', { count: availableVersions.length })}
- </div>
+ <div className="text-xs text-muted-foreground">
+ {t('packageManagement.availableVersionsCount', { count: safeVersions.length })}
+ </div>
)}Also applies to: 210-224
🤖 Prompt for AI Agents
In `@src/renderer/components/PackageManagementCard.tsx` around lines 161 - 175,
availableVersions may be undefined; before using it in the Select JSX (where
value={selectedVersion}, onValueChange={setSelectedVersion},
disabled={isInstalling || availableVersions.length === 0} and mapping
availableVersions to <SelectItem>), normalize it to an empty array (e.g., const
versions = availableVersions ?? []) and use versions.length and versions.map
instead; apply the same normalization wherever availableVersions is later used
(the other SelectContent/map block) so .length and .map cannot throw.
| {t('packageManagement.noPackagesMessage')} | ||
| <code className="ml-2 px-2 py-1 bg-muted rounded text-xs"> | ||
| ~/repos/newbe36524/pcode/Release/release-packages/ | ||
| </code> |
There was a problem hiding this comment.
Remove the hard-coded local path from the user-facing message.
The developer-specific path will be wrong for end users and can leak local filesystem info. Source it from config/state or omit it.
🤖 Prompt for AI Agents
In `@src/renderer/components/PackageManagementCard.tsx` around lines 215 - 218, In
PackageManagementCard, remove the hard-coded developer path currently rendered
alongside {t('packageManagement.noPackagesMessage')} (the <code className="ml-2
px-2 py-1 bg-muted rounded text-xs"> element) and instead render either a
configurable value (e.g., a prop/state like packagesPath or a value from app
config) or omit the path entirely; update the component to read the path from
the appropriate config/state key and use that variable in place of the literal
string.
| const [canGoBack, setCanGoBack] = useState(false); | ||
| const [canGoForward, setCanGoForward] = useState(false); |
There was a problem hiding this comment.
canGoBack and canGoForward state is never updated.
The states canGoBack and canGoForward are initialized to false but never updated when navigation occurs. The navigation buttons will always remain disabled.
Add a listener to update navigation state:
🐛 Fix navigation state updates
useEffect(() => {
const webView = webViewRef.current;
if (!webView) return;
+ const updateNavigationState = () => {
+ if (webView) {
+ setCanGoBack(webView.canGoBack());
+ setCanGoForward(webView.canGoForward());
+ }
+ };
// ... existing handlers ...
+ webView.addEventListener('did-navigate', updateNavigationState);
+ webView.addEventListener('did-navigate-in-page', updateNavigationState);
return () => {
// ... existing cleanup ...
+ webView.removeEventListener('did-navigate', updateNavigationState);
+ webView.removeEventListener('did-navigate-in-page', updateNavigationState);
};
}, [canGoBack, canGoForward, t]);Also applies to: 92-92
🤖 Prompt for AI Agents
In `@src/renderer/components/WebView.tsx` around lines 15 - 16, The
canGoBack/canGoForward state is never updated; add a useEffect that registers
webview navigation listeners on the WebView component (use the existing
webviewRef) to call setCanGoBack(webview.canGoBack()) and
setCanGoForward(webview.canGoForward()) on 'did-navigate' and
'did-navigate-in-page' (and optionally 'did-stop-loading') events, and remove
those listeners in the effect cleanup so the navigation buttons enable/disable
correctly.
| const handleOpenInBrowser = () => { | ||
| const { shell } = require('electron'); | ||
| shell.openExternal(src); | ||
| }; |
There was a problem hiding this comment.
require('electron') in renderer violates context isolation.
Using require('electron') directly in the renderer won't work with contextIsolation: true and may fail with sandbox: true. The shell.openExternal should be exposed via the preload script's context bridge:
🔧 Fix via preload exposure
In preload/index.ts:
openExternal: (url: string) => shell.openExternal(url),In WebView.tsx:
const handleOpenInBrowser = () => {
- const { shell } = require('electron');
- shell.openExternal(src);
+ window.electronAPI.openExternal(src);
};🤖 Prompt for AI Agents
In `@src/renderer/components/WebView.tsx` around lines 115 - 118, The renderer is
calling require('electron') inside handleOpenInBrowser which breaks when
contextIsolation/sandbox are enabled; remove the require usage and instead call
an API exposed by the preload script (e.g., window.myPreloadApi.openExternal or
window.electronAPI.openExternal) from handleOpenInBrowser, and add the
corresponding contextBridge exposure in preload (export a function like
openExternal: (url: string) => shell.openExternal(url)); also update/declare the
global window type (or use the existing preload namespace) so TypeScript
recognizes the exposed method.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Build & Infrastructure