Conversation
- Add i18n support for version mismatch messages - Enhance dependency card to distinguish missing vs mismatched - Add upgrade button text for mismatched dependencies - Improve install all prompt with detailed status This change improves user experience during onboarding by clearly identifying and guiding users to fix version mismatched dependencies alongside missing ones. Related: onboarding-dependency-version-mismatch-handling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request performs a comprehensive architectural migration and feature enhancement for the Hagicode Desktop Electron application. It replaces Redux-Saga with Redux Toolkit native thunks across the renderer process, introduces region-based dependency detection, migrates to manifest-driven configuration, adds a new Settings page with debug mode support, and enhances dependency management with progress tracking and improved UI visualization. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as React Component
participant Store as Redux Store
participant Thunk as Async Thunk
participant IPC as Main Process IPC
participant API as Electron API
UI->>Store: dispatch(installFromManifest(versionId))
activate Thunk
Store->>Thunk: installFromManifest thunk executed
Thunk->>Store: getState() → pending dependencies
Thunk->>IPC: dependency:install-from-manifest
activate IPC
IPC->>API: Execute install commands
API-->>IPC: Command progress events
IPC-->>Thunk: Progress callbacks
deactivate IPC
Thunk->>Store: openInstallCommandDialog()
Thunk->>Store: dispatch progress updates
Thunk->>Store: setInstallCommandStatus('success')
Thunk->>IPC: Fetch updated dependencies
Thunk->>Store: setDependencies(updated)
Thunk->>UI: Show success toast
deactivate Thunk
Store-->>UI: State updated, re-render
sequenceDiagram
participant User as User Action
participant Settings as DebugSettings Component
participant API as Electron API
participant Store as Store/Manager
participant Dep as Dependency Check
User->>Settings: Toggle ignoreDependencyCheck
Settings->>API: setDebugMode({ignoreDependencyCheck: true})
activate API
API->>Store: Set debugMode flag in config
API-->>Settings: { success: true }
deactivate API
Settings->>Settings: Show success toast
Note over Settings,Dep: Next dependency check...
Dep->>Store: Check debugMode flag
alt Debug Mode Enabled
Dep->>Dep: Override all deps as "not installed"
Dep-->>UI: Show missing dependencies
else Debug Mode Disabled
Dep->>API: Perform actual dependency checks
API-->>UI: Show real status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 7
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 (3)
src/renderer/components/ui/language-toggle.tsx (1)
24-26:⚠️ Potential issue | 🟠 MajorIncorrect usage of
useStateas an effect.Passing a callback to
useStatesets the initial state value, it does not act as an effect. This code executes during render and the callback's return value becomes the initial state (which isundefinedhere sincesetMounted(true)returns nothing). UseuseEffectinstead.🐛 Proposed fix
- useState(() => { - setMounted(true); - }); + useEffect(() => { + setMounted(true); + }, []);Also add
useEffectto the imports:-import { useState } from 'react'; +import { useState, useEffect } from 'react';src/renderer/components/LicenseManagementPage.tsx (1)
92-94:⚠️ Potential issue | 🔴 Critical
handleRefreshdispatches a plain action instead of the thunk.Dispatching
{ type: 'license/fetch' }creates a plain action object that won't trigger thefetchLicenseasync thunk. This will silently fail to refresh the license data. Import and dispatch the thunk instead.🐛 Proposed fix
Update the import to include
fetchLicense:-import { saveLicense } from '../store/thunks/licenseThunks'; +import { saveLicense, fetchLicense } from '../store/thunks/licenseThunks';Then fix the handler:
const handleRefresh = () => { - dispatch({ type: 'license/fetch' }); + dispatch(fetchLicense()); };src/preload/index.ts (1)
173-173:⚠️ Potential issue | 🟡 MinorType mismatch:
switchViewis missing 'settings' view type.The
switchViewfunction parameter type doesn't include'settings', butApp.tsx(line 29) declares it should accept'settings'as a valid view. This inconsistency could cause runtime issues.🐛 Proposed fix
// View Management APIs - switchView: (view: 'system' | 'web' | 'dependency' | 'version' | 'license') => ipcRenderer.invoke('switch-view', view), + switchView: (view: 'system' | 'web' | 'dependency' | 'version' | 'license' | 'settings') => ipcRenderer.invoke('switch-view', view),
🤖 Fix all issues with AI agents
In
`@openspec/changes/archive/2026-02-10-dotnet-web-service-startup/specs/electron-app/spec.md`:
- Around line 5-22: The spec contradicts itself by requiring the service to be a
parent-managed child process while the Windows scenario requires spawning with
the detached option; reconcile this by updating the spec text: either (A) state
explicitly that the service must be parent-managed on all platforms and remove
the "detached" requirement from the "Dotnet command execution on Windows"
scenario (ensure the process is spawned without detached and the parent manages
lifecycle/status), or (B) allow a platform-specific ownership exception by
changing the global "child process" requirement to "process (child-managed
except on Windows where detached is permitted)" and adjust the Windows scenario
to document that lifecycle/status must be tracked via alternative means; update
the relevant lines in "Scenario: Starting embedded web service with dotnet
command" and "Scenario: Dotnet command execution on Windows" accordingly to
remove the contradiction.
In
`@openspec/changes/archive/2026-02-10-onboarding-dependency-version-mismatch-handling/specs/dependency-management/spec.md`:
- Around line 10-22: The MODIFIED delta must include the entire original
requirement block titled "Requirement: 依赖状态可视化" including all four original
scenarios ("显示已安装且版本匹配的依赖", "显示未安装的依赖", "显示检测中的加载状态", and "显示版本不匹配的依赖") before
any edits; restore any omitted scenarios into the MODIFIED section, then apply
your edits to the "显示版本不匹配的依赖" scenario (add the yellow ⚠️ icon, "版本不匹配" text,
current vs required version, and "升级/重新安装" buttons and treat as actionable in
guides) and append the two new scenarios "引导流程显示混合依赖状态" and "引导流程仅版本不匹配的依赖"
under the same MODIFIED block so the delta contains the full original block plus
your changes.
In `@package.json`:
- Line 6: The packageManager entry ("packageManager": "npm@10.9.2") is not being
enforced in CI because Corepack isn't enabled; update the CI workflow that uses
the setup-node action to enable Corepack (so the packageManager field is
honored) or change the packageManager value to match Node 22's bundled npm
version; specifically modify the CI step that calls setup-node to set cache:
'npm' and enable corepack (or run corepack enable) so the packageManager field
is enforced, or update the packageManager symbol to the Node 22 bundled npm
version for consistency.
In `@src/main/main.ts`:
- Around line 520-528: Extract the duplicated debug-mode transformation into a
single helper (e.g. function applyDebugModeIfEnabled(dependencies:
DependencyCheckResult[]): DependencyCheckResult[]) that reads
configManager.getStore().get('debugMode') and, when ignoreDependencyCheck is
true, returns dependencies.map(dep => ({ ...dep, installed: false,
versionMismatch: true })), otherwise returns the original array; then replace
the duplicated blocks in the handlers that produce a dependencies array (the
handlers for check-dependencies, version:checkDependencies,
dependency:get-missing, dependency:get-all) to call
applyDebugModeIfEnabled(dependencies) before returning so all four handlers
share the same behavior.
In `@src/main/region-detector.ts`:
- Line 9: The exported Region type ('Region') in region-detector.ts must be
changed to match manifest-reader.ts (use 'china' | 'global' rather than 'CN' |
'INTERNATIONAL') and RegionDetector (or its detect method) should return those
string literals; update any internal mapping logic that currently returns
'CN'/'INTERNATIONAL' to return 'china'/'global' instead so types align across
modules (Region type, RegionDetector.detect). Also stop classifying 'zh-TW' and
'zh-HK' as the China region — map those locales to 'global' (or the equivalent
non-China value) so region-specific package selection is correct, and update all
usages that rely on the previous values to the new literals.
In `@src/main/web-service-manager.ts`:
- Around line 117-122: The chmod call on scriptPath uses fs.chmod as
fire-and-forget causing a race where spawn may run before permissions are set;
move the chmod logic into the start() method (or otherwise ensure it runs
immediately before spawning) and await the fs.chmod(scriptPath) promise (handle
errors with log.warn) so that start() only spawns the process after permissions
are confirmed; update any existing top-level chmod call to remove the unawaited
invocation and reference scriptPath, start(), and the spawn invocation when
implementing this change.
In `@src/renderer/store/thunks/webServiceThunks.ts`:
- Around line 531-542: The current checkDependenciesAfterInstall thunk in
webServiceThunks dispatches a raw action with type
'dependency/checkAfterInstall' instead of invoking the actual thunk from
dependencyThunks.ts and duplicates the thunk name/type; fix by importing the
real thunk (the checkDependenciesAfterInstall exported from dependencyThunks.ts
or rename one to avoid collision) and dispatch that thunk
(dispatch(checkDependenciesAfterInstall({ versionId: params.versionId })))
rather than dispatching a plain action object, and remove or change the
conflicting action type string to prevent duplicate thunk/action naming.
🟡 Minor comments (21)
openspec/changes/archive/2026-02-10-dotnet-web-service-startup/tasks.md-21-41 (1)
21-41:⚠️ Potential issue | 🟡 MinorDon’t mark manual-testing items complete before verification.
Items 3.1–5.5 are checked but still say “Manual testing required.” Please leave these unchecked until the manual runs finish (and check sequentially).Based on learnings: Track implementation progress sequentially in tasks.md with checkboxes, completing one task at a time before moving to the next; Update every task in tasks.md to
- [x]after all implementation work is complete and confirmed.openspec/changes/archive/2026-02-11-migrate-package-metadata-to-manifest/tasks.md-33-37 (1)
33-37:⚠️ Potential issue | 🟡 MinorUpdate section 1.5 to reflect the thunk/listener architecture (not sagas).
The codebase has migrated to RTK thunks and listener middleware. Section 1.5 still references
dependencySaga.ts, which doesn't exist. Update the section to targetdependencyThunks.tsand listener middleware updates instead.openspec/changes/archive/2026-02-10-disable-devtools-in-production/tasks.md-200-238 (1)
200-238:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced blocks (MD040).
The commit message block and the dependency diagram block are missing languages.✅ Suggested fix
-``` +```text feat: disable DevTools in production builds - Add devTools: !app.isPackaged to BrowserWindow webPreferences - Remove temporary openDevTools() call in production - Apply change to both main window and Hagicode window This change prevents end users from accessing developer tools in production builds, improving security and user experience. Related: disable-devtools-in-production -``` +``` -``` +```text Phase 1: 代码修改 ├── Task 1.1 ─────────────┐ ├── Task 1.2 ─────────────┤ └── Task 1.3 ─────────────┤ │ ▼ Phase 2: 验证与测试 │ ├── Task 2.1 ────────────┤ ├── Task 2.2 ────────────┤ └── Task 2.3 (可选) ─────┤ │ ▼ Phase 3: 代码审查与合并 │ ├── Task 3.1 ────────────┘ └── Task 3.2 ────────────┘ -``` +```openspec/changes/archive/2026-02-11-dependency-install-progress-dialog/tasks.md-49-56 (1)
49-56:⚠️ Potential issue | 🟡 MinorUpdate Stage 1.3 tasks to reference current thunk/listener middleware architecture instead of removed sagas.
Lines 49-56 reference
src/renderer/store/sagas/dependencySaga.tswhich no longer exists. The store now usessrc/renderer/store/thunks/dependencyThunks.tswithcreateAsyncThunkandsrc/renderer/store/listenerMiddleware.tsfor async effects. Update the task list to reflect the current implementation pattern.src/renderer/store/thunks/licenseThunks.ts-54-56 (1)
54-56:⚠️ Potential issue | 🟡 MinorHardcoded Chinese strings should use i18n.
The toast messages contain hardcoded Chinese text (
"许可证已更新","许可证更新失败"), inconsistent with the app's i18n architecture. Use translation keys for proper localization support.🌐 Proposed fix - example for success toast
+import i18n from '@/i18n'; + // In saveLicense thunk: - toast.success('许可证已更新', { - description: 'License updated successfully', - }); + toast.success(i18n.t('licenseManagement.toast.saveSuccess'), { + description: i18n.t('licenseManagement.toast.saveSuccessDesc'), + });Apply similar changes to the error toasts on lines 62-64 and 72-74.
Also applies to: 62-64, 72-74
src/renderer/index.css-211-232 (1)
211-232:⚠️ Potential issue | 🟡 MinorFix alpha value notation to comply with Stylelint rules.
The static analysis tool flags the use of decimal notation (0.2, 0.3) for alpha values. Per Stylelint's
alpha-value-notationrule, percentage notation is preferred.🔧 Suggested fix
/* Custom scrollbar for dependency list */ .scrollbar-thin { scrollbar-width: thin; - scrollbar-color: oklch(from var(--muted-foreground) r g b / 0.2) transparent; + scrollbar-color: oklch(from var(--muted-foreground) r g b / 20%) transparent; } .scrollbar-thin::-webkit-scrollbar { width: 6px; } .scrollbar-thin::-webkit-scrollbar-track { background: transparent; } .scrollbar-thin::-webkit-scrollbar-thumb { - background-color: oklch(from var(--muted-foreground) r g b / 0.2); + background-color: oklch(from var(--muted-foreground) r g b / 20%); border-radius: 3px; } .scrollbar-thin::-webkit-scrollbar-thumb:hover { - background-color: oklch(from var(--muted-foreground) r g b / 0.3); + background-color: oklch(from var(--muted-foreground) r g b / 30%); }src/renderer/store/thunks/packageSourceThunks.ts-351-358 (1)
351-358:⚠️ Potential issue | 🟡 MinorPotential race condition with shared loading state in parallel dispatch.
initializePackageSourcedispatchesloadSourceConfigandloadAllSourceConfigsin parallel viaPromise.all, but both thunks independently setsetLoading(true)at start andsetLoading(false)in finally. The first thunk to complete will set loading tofalsewhile the other is still running.Consider either:
- Managing loading state directly in
initializePackageSource- Using separate loading flags for each operation
- Using a reference counter approach
🔧 Suggested fix
export const initializePackageSource = createAsyncThunk( 'packageSource/initialize', async (_, { dispatch }) => { + dispatch(setLoading(true)); + try { - await Promise.all([ - dispatch(loadSourceConfig()), - dispatch(loadAllSourceConfigs()), - ]); + // Load configs without triggering individual loading states + const [configResult, allConfigsResult] = await Promise.all([ + window.electronAPI.packageSource.getConfig(), + window.electronAPI.packageSource.getAllConfigs(), + ]); + dispatch(setCurrentConfig(configResult)); + dispatch(setAllConfigs(allConfigsResult)); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Failed to initialize package source'; + dispatch(setError(errorMessage)); + throw error; + } finally { + dispatch(setLoading(false)); + } } );openspec/changes/archive/2026-02-11-migrate-package-metadata-to-manifest/specs/package-management/spec.md-21-23 (1)
21-23:⚠️ Potential issue | 🟡 MinorMinor wording tweak for “关于” phrasing.
LanguageTool flags the prepositional phrase. Consider quoting the page name for clarity.📝 Proposed fix
-**And** 在关于页面和帮助界面显示这些信息 +**And** 在“关于”页面和帮助界面显示这些信息src/renderer/components/settings/OnboardingSettings.tsx-11-22 (1)
11-22:⚠️ Potential issue | 🟡 MinorImprove error message robustness in the catch block.
TheString(error)call at line 21 can produce"[object Object]"if a non-Error exception is caught. Use error normalization to ensure consistent, readable error messages.🛠️ Proposed fix
} catch (error) { - toast.error(t('settings.onboarding.resetError', { error: String(error) })); + const errorMessage = error instanceof Error ? error.message : String(error ?? 'Unknown error'); + toast.error(t('settings.onboarding.resetError', { error: errorMessage })); } finally {openspec/changes/archive/2026-02-10-onboarding-dependency-version-mismatch-handling/proposal.md-140-213 (1)
140-213:⚠️ Potential issue | 🟡 MinorReference PROPOSAL_DESIGN_GUIDELINES.md in the UI Design Changes section.
UI mockups and a Mermaid flow are included, but the required design-guideline reference is missing.
Based on learnings: Applies to openspec/changes/*/proposal.md : Include UI mockups, Mermaid sequence diagrams for user interaction flows, and reference PROPOSAL_DESIGN_GUIDELINES.md in proposal.md for UI changes
openspec/changes/archive/2026-02-11-dependency-install-progress-dialog/proposal.md-26-86 (1)
26-86:⚠️ Potential issue | 🟡 MinorInclude UI mockups + a Mermaid flow, and reference PROPOSAL_DESIGN_GUIDELINES.md.
This is a UI-facing component proposal, but the document doesn’t include the required mockups/sequence flow or the explicit guideline reference.
Based on learnings: Applies to openspec/changes/*/proposal.md : Include UI mockups, Mermaid sequence diagrams for user interaction flows, and reference PROPOSAL_DESIGN_GUIDELINES.md in proposal.md for UI changes
openspec/changes/archive/2026-02-10-onboarding-dependency-version-mismatch-handling/proposal.md-1-120 (1)
1-120:⚠️ Potential issue | 🟡 MinorAdd explicit “Why / What Changes / Code Flow Changes / Impact” sections.
The content is here, but the required proposal headings aren’t explicit. Please add those sections for template consistency.
Based on learnings: Applies to openspec/changes/*/proposal.md : 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-11-dependency-install-progress-dialog/proposal.md-1-107 (1)
1-107:⚠️ Potential issue | 🟡 MinorAdd explicit “Why / What Changes / Code Flow Changes / Impact” sections.
The proposal content is solid but doesn’t follow the required heading structure, which makes it harder to scan and validate.
Based on learnings: Applies to openspec/changes/*/proposal.md : 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-10-standardized-web-service-startup-scripts/tasks.md-62-65 (1)
62-65:⚠️ Potential issue | 🟡 MinorInconsistency: Validation marked complete but tests are incomplete.
Section 5 marks validation items as complete (lines 62-65), but Section 2 shows unit tests (lines 29-31) and integration tests (lines 33-36) as incomplete. The validation at line 62 ("验证所有测试通过") cannot be accurate if the tests haven't been written yet.
Consider either:
- Marking validation items as incomplete until tests are written and pass
- Adding the missing tests before marking validation complete
Based on learnings: "Update every task in tasks.md to
- [x]after all implementation work is complete and confirmed"src/renderer/components/DependencyManagementCardUnified.tsx-292-304 (1)
292-304:⚠️ Potential issue | 🟡 MinorDuplicate
fetchDependenciesdispatch in dialog callbacks.Both
onCloseandonSuccessdispatch{ type: 'dependency/fetchDependencies' }. SinceonSuccessalso callsonClose?.()indirectly via the dialog'shandleClose, this could result in double-fetching. Consider whether both callbacks need to trigger the fetch, or ifonSuccessalone is sufficient.src/renderer/components/DependencyInstallProgressDialog.tsx-112-120 (1)
112-120:⚠️ Potential issue | 🟡 MinorHardcoded Chinese strings should use i18n.
Several strings are hardcoded in Chinese instead of using the translation system:
- Line 117:
"正在验证安装结果..."- Line 126:
"安装成功并已验证"- Line 157:
"正在验证安装结果..."- Line 183:
"验证通过：依赖已成功安装"- Line 184:
"无法自动验证安装结果，请手动确认"- Line 243:
"验证中..."These should be moved to translation files for consistency with the rest of the application.
Also applies to: 126-126, 157-157, 183-184, 243-243
openspec/changes/archive/2026-02-10-settings-page-and-onboarding-reset-arg-value/proposal.md-137-168 (1)
137-168:⚠️ Potential issue | 🟡 MinorDuplicate numbering in Success Criteria section.
Lines 159 and 163 both use "5." as the item number. This appears to be a copy-paste error.
📝 Proposed fix
4. **调试模式开关功能可用** - 点击「忽略依赖检查」开关后，调试模式状态被保存 - 开启时，所有依赖项被视为未安装状态 - 关闭时，恢复正常的依赖检查逻辑 - 调试模式状态持久化，重启应用后保持 5. **国际化支持完整** - 中文和英文翻译正确显示 - 所有 UI 文本均有对应的翻译键 -5. **代码质量** +6. **代码质量** - 遵循现有代码风格和架构模式src/renderer/components/DependencyInstallProgressDialog.tsx-1-19 (1)
1-19:⚠️ Potential issue | 🟡 MinorDuplicate
useDispatchimport.
useDispatchis imported twice fromreact-reduxat Line 3 and Line 17. Remove the duplicate.🔧 Proposed fix
import { useEffect, useRef } from 'react'; import { useTranslation } from 'react-i18next'; -import { useSelector } from 'react-redux'; +import { useSelector, useDispatch } from 'react-redux'; import { Dialog, DialogContent, @@ -14,7 +14,6 @@ import { Loader2, CheckCircle2, XCircle, Terminal, AlertCircle } from 'lucide-react'; import { selectInstallCommandProgress } from '../store/slices/dependencySlice'; -import { useDispatch } from 'react-redux'; import { closeInstallCommandDialog } from '../store/slices/dependencySlice';src/main/region-detector.ts-22-23 (1)
22-23:⚠️ Potential issue | 🟡 MinorLocale classification may be incorrect for some Chinese locales.
Including
zh-TW(Taiwan) andzh-HK(Hong Kong) inCHINESE_LOCALESmaps them to the'CN'region. This affects which install commands are used from the manifest (e.g., mirror URLs). Users in these regions may:
- Not have access to China-specific mirrors
- Experience slower downloads from CN-specific sources
Consider whether these locales should map to
'INTERNATIONAL'instead, or make this configurable.src/renderer/store/thunks/dependencyThunks.ts-173-185 (1)
173-185:⚠️ Potential issue | 🟡 MinorHardcoded Chinese strings in toast messages.
Toast messages use hardcoded Chinese text (e.g.,
'依赖安装完成','安装成功'). These should use i18n keys for proper internationalization, especially since the app has locale support.Also applies to: 197-199, 209-211, 306-308, 395-397, 413-415, 485-487, 493-495, 505-507
src/main/dependency-manager.ts-548-556 (1)
548-556:⚠️ Potential issue | 🟡 MinorCommands from manifest files are executed without validation when using
shell: true.The
spawncall usesshell: trueto support command chaining, but the commands originate directly from parsed manifest files without any sanitization or validation. While manifests are loaded from installed versions stored locally (not remote/untrusted sources by default), there is no validation to prevent command injection if a manifest file were to be modified or compromised.Recommendation: Implement command validation or allowlisting before execution, or consider using
shell: falsewith explicit argument arrays instead of shell command strings.
🧹 Nitpick comments (31)
openspec/changes/archive/2026-02-10-disable-devtools-in-production/proposal.md (1)
9-95: Align proposal headings with the required template (Why / What Changes / Impact).
Consider renaming/additional headings and adding brief “UI Design Changes” / “Code Flow Changes” sections marked N/A if not applicable, to keep proposals consistent.Based on learnings: Structure proposal.md with Why, What Changes, UI Design Changes (if applicable), Code Flow Changes (if applicable), and Impact sections.
src/renderer/store/thunks/i18nThunks.ts (1)
38-57: Consider validatingsavedLanguagebefore applying it.If localStorage contains an invalid or unsupported language code (e.g., from a previous app version),
i18n.changeLanguagemay fail or behave unexpectedly. Consider validating against available languages before applying.💡 Optional: Add language validation
export const loadInitialLanguage = createAsyncThunk( 'i18n/loadInitialLanguage', async (_, { dispatch }) => { try { // Read from localStorage const savedLanguage = localStorage.getItem('appSettings.language'); - if (savedLanguage) { + if (savedLanguage && i18n.options.supportedLngs?.includes(savedLanguage)) { await i18n.changeLanguage(savedLanguage); dispatch(setCurrentLanguage(savedLanguage)); return savedLanguage; } return null; } catch (error) { console.error('Failed to load initial language:', error); throw error; } } );src/renderer/components/ui/language-toggle.tsx (1)
16-16: AddAppDispatchtyping for proper thunk dispatch support.The
useDispatch()call lacks theAppDispatchtype parameter, which may cause TypeScript errors when dispatching async thunks. For consistency withLanguageSelector.tsx, type the dispatch.♻️ Proposed fix
+import { AppDispatch } from '@/store'; + export function LanguageToggle() { const { i18n } = useTranslation('common'); - const dispatch = useDispatch(); + const dispatch = useDispatch<AppDispatch>();src/renderer/store/thunks/licenseThunks.ts (1)
49-58: Consider using.unwrap()when awaiting nested thunk dispatch.When dispatching
fetchLicense()insidesaveLicense, the await resolves to the action result object, not the payload. If you need to handle the fetched license or propagate errors, use.unwrap().💡 Optional improvement
if (result.success) { // Reload the license - await dispatch(fetchLicense()); + await dispatch(fetchLicense()).unwrap();src/renderer/store/slices/dependencySlice.ts (2)
195-201:closeInstallCommandDialogdoes not reset all state fields.When closing the dialog,
logs,status,error, andverificationPassedare not reset, which could leave stale data visible if the dialog is reopened. Consider resetting to initial state values.♻️ Proposed fix
closeInstallCommandDialog: (state) => { state.installCommandProgress.isOpen = false; state.installCommandProgress.commands = []; state.installCommandProgress.checkCommand = undefined; state.installCommandProgress.currentCommandIndex = 0; state.installCommandProgress.isExecuting = false; + state.installCommandProgress.logs = []; + state.installCommandProgress.status = 'idle'; + state.installCommandProgress.error = undefined; + state.installCommandProgress.verificationPassed = undefined; },
208-214: Review theisExecutingstate transition logic.The condition
status !== 'verifying'setsisExecuting = falsefor both terminal states (success,error) and potentially unexpected calls. This seems intentional, but consider ifisExecutingshould remaintrueduringverifyingphase or if a more explicit check for terminal states would be clearer.💡 Optional: More explicit terminal state check
setInstallCommandStatus: (state, action: PayloadAction<{ status: 'success' | 'error' | 'verifying'; error?: string }>) => { state.installCommandProgress.status = action.payload.status; - if (action.payload.status !== 'verifying') { + if (action.payload.status === 'success' || action.payload.status === 'error') { state.installCommandProgress.isExecuting = false; } state.installCommandProgress.error = action.payload.error; },openspec/changes/archive/2026-02-10-onboarding-dependency-version-mismatch-handling/tasks.md (2)
296-309: Add language specifier to fenced code block.The commit message code block lacks a language specifier, which may affect rendering in some Markdown viewers.
📝 Proposed fix
-``` +```text feat: handle dependency version mismatch in onboarding
320-346: Add language specifier to dependency diagram code block.The task dependency diagram uses a text-based format but lacks a language specifier.
📝 Proposed fix
-``` +```text Phase 1: 国际化文件更新src/renderer/components/SidebarNavigation.tsx (1)
22-27: Consider using a distinct icon for Settings to improve visual differentiation.Both the "Dashboard" (line 23) and "Settings" (line 26) navigation items use the
Settings(gear) icon. This may cause visual confusion for users. Consider using a different icon for one of them, such asCog,Sliders, orSlidersHorizontalfor the settings page.💡 Optional fix
-import { Settings, Globe as GlobeIcon, Package, FileText, ChevronLeft, ChevronRight, Users, Star, ExternalLink, Info, Key } from 'lucide-react'; +import { Settings, Globe as GlobeIcon, Package, FileText, ChevronLeft, ChevronRight, Users, Star, ExternalLink, Info, Key, SlidersHorizontal } from 'lucide-react'; const navigationItems: NavigationItem[] = [ { id: 'system', labelKey: 'sidebar.dashboard', icon: Settings }, { id: 'version', labelKey: 'sidebar.versionManagement', icon: FileText }, { id: 'license', labelKey: 'sidebar.licenseManagement', icon: Key }, - { id: 'settings', labelKey: 'sidebar.settings', icon: Settings }, + { id: 'settings', labelKey: 'sidebar.settings', icon: SlidersHorizontal }, ];src/renderer/store/thunks/onboardingThunks.ts (1)
6-11: Inconsistent action type prefix forTRIGGER_ONBOARDING_NEXT.The new constant uses
'dependency/triggerOnboardingNext'while all other action types in this file use the'onboarding/'prefix. If this action is meant to be part of the onboarding flow, consider using'onboarding/triggerNext'for consistency.If the
'dependency/'prefix is intentional because this action is dispatched from dependency-related logic, consider adding a comment to clarify this design decision.src/types/onboarding.ts (1)
66-81: Align DependencyCheckResult with the main shape to avoid drift.
Consider including the optional fields already present insrc/main/dependency-manager.tsso IPC payloads stay type-safe if you later consume them in the renderer.♻️ Suggested type alignment
export interface DependencyCheckResult { key: string; // Manifest dependency key (e.g., "dotnet", "claudeCode") name: string; type: string; installed: boolean; version?: string; requiredVersion?: string; versionMismatch?: boolean; + installCommand?: string | Record<string, unknown>; + checkCommand?: string; + downloadUrl?: string; description?: string; }openspec/changes/archive/2026-02-11-onboarding-dependency-check-detailed-display/proposal.md (1)
90-117: Consider adding language hint to ASCII diagram code block.The static analysis tool flagged the fenced code block without a language specifier. For ASCII art diagrams like this UI mockup, you can use
textorplaintextas the language identifier to silence the warning while preserving formatting:💡 Suggested change
-``` +```text ┌─────────────────────────────────────────────────────────┐ │ Package Icon │src/renderer/components/onboarding/steps/DependencyInstaller.tsx (2)
61-78: Add aria attributes to the expand/collapse toggle.
This improves screen-reader support for the details panel.♿ Proposed fix
- <button + <button onClick={() => setIsExpanded(!isExpanded)} + aria-expanded={isExpanded} + aria-controls="dependency-check-details" className="flex items-center gap-1 text-sm text-muted-foreground hover:text-foreground transition-colors" > ... - <div className="mt-4 max-h-80 overflow-y-auto space-y-2 pr-2"> + <div id="dependency-check-details" className="mt-4 max-h-80 overflow-y-auto space-y-2 pr-2">Also applies to: 101-104
104-111: Prefer a stable key over array index.
Usingindexcan cause incorrect row reuse when the list order changes.♻️ Proposed fix
- <div - key={index} + <div + key={dep.name} className="bg-background rounded-md p-3 border border-border" >src/renderer/components/settings/DebugSettings.tsx (1)
10-80: Prevent overlapping debug-mode updates while a toggle request is in flight.Rapid toggles can trigger concurrent
setDebugModecalls and lead to out-of-order UI state. Consider adding a “saving” flag to disable the switch until the request completes.🔧 Suggested change
const [ignoreDependencyCheck, setIgnoreDependencyCheck] = useState(false); const [isLoading, setIsLoading] = useState(true); + const [isSaving, setIsSaving] = useState(false); @@ const handleToggleIgnoreDependencyCheck = async (checked: boolean) => { + if (isSaving) return; + setIsSaving(true); try { const result = await window.electronAPI.setDebugMode({ ignoreDependencyCheck: checked }); if (result.success) { setIgnoreDependencyCheck(checked); toast.success(checked ? t('settings.debug.ignoreDependencyCheckEnabled') : t('settings.debug.ignoreDependencyCheckDisabled') ); } else { toast.error(t('settings.debug.toggleError', { error: result.error })); } } catch (error) { toast.error(t('settings.debug.toggleError', { error: String(error) })); + } finally { + setIsSaving(false); } }; @@ <Switch id="ignore-dependency-check" checked={ignoreDependencyCheck} onCheckedChange={handleToggleIgnoreDependencyCheck} - disabled={isLoading} + disabled={isLoading || isSaving} />src/renderer/store/index.ts (3)
58-70: Consider using proper TypeScript typing for state access.The state access uses implicit
anytyping. Consider usingRootStatefor type safety.listenerMiddleware.startListening({ predicate: (action) => action.type === 'webService/setProcessInfo', effect: (action, listenerApi) => { - const state = listenerApi.getState(); + const state = listenerApi.getState() as RootState; const currentView = state.view.currentView; - const payload = action.payload as any; + const payload = action.payload as { url?: string };
73-86: Prefer using action creators over string-based action types.Dispatching with a raw string type
'dependency/fetchDependenciesSuccess'bypasses type safety and could break if the slice action name changes.♻️ Recommended: Import and use the action creator from dependencySlice
+import { fetchDependenciesSuccess } from './slices/dependencySlice'; listenerMiddleware.startListening({ predicate: (action) => action.type === 'webService/checkDependenciesAfterInstall', effect: async (action, listenerApi) => { const payload = action.payload as { versionId: string }; const missingDeps = await window.electronAPI.getMissingDependencies(payload.versionId); if (missingDeps.length > 0) { - listenerApi.dispatch({ - type: 'dependency/fetchDependenciesSuccess', - payload: missingDeps, - }); + listenerApi.dispatch(fetchDependenciesSuccess(missingDeps)); } }, });
129-137: Polling interval lacks cleanup mechanism.The
setIntervalfor web service status polling runs indefinitely without any cleanup. While this is acceptable for a singleton store, it could cause issues during hot module replacement in development or if the store is ever recreated.♻️ Optional: Consider storing interval ID for potential cleanup
+// Store interval ID for potential cleanup +let statusPollingInterval: NodeJS.Timeout | null = null; + // Set up polling as backup for web service status -setInterval(async () => { +statusPollingInterval = setInterval(async () => { try { const status = await window.electronAPI.getWebServiceStatus(); dispatch(setProcessInfo(status)); } catch (error) { console.error('Watch web service status error:', error); } }, 5000);src/renderer/i18n/locales/en-US/pages.json (1)
8-17: Duplicate description text.Lines 10 and 12 contain identical text for
descriptionandrestartWizardDescription. Consider removing one to reduce maintenance burden."onboarding": { "title": "Onboarding", "description": "Re-run the first-time setup wizard to reconfigure data directory, download packages, install dependencies, and start the service.", "restartWizard": "Restart Wizard", - "restartWizardDescription": "Re-run the first-time setup wizard to reconfigure data directory, download packages, install dependencies, and start the service.", + "restartWizardDescription": "", "restartButton": "Restart Wizard",Or simply use
settings.onboarding.descriptionin the UI whererestartWizardDescriptionwould be used.src/renderer/store/thunks/rssFeedThunks.ts (2)
11-22: Global interface augmentation for electronAPI is fragmented.This file declares a partial
Window.electronAPIinterface. If other files also declare their own partialelectronAPIinterfaces, TypeScript will merge them, but this scattered approach can lead to maintenance issues. Consider consolidating allelectronAPItype declarations in a single.d.tsfile (e.g.,src/types/electron.d.ts).
28-55: Significant code duplication betweenfetchFeedItemsandrefreshFeed.Both thunks share nearly identical logic for loading state management, fetching lastUpdate, and error handling. Extract the common pattern into a shared helper to reduce duplication.
♻️ Proposed refactor using a shared helper
+// Helper for common RSS fetch logic +async function fetchRssItemsWithDispatch( + dispatch: any, + fetchFn: () => Promise<RSSFeedItem[]>, + errorMessage: string +): Promise<RSSFeedItem[]> { + try { + dispatch(setLoading(true)); + dispatch(clearError()); + + const items = await fetchFn(); + dispatch(setItems(items)); + + const lastUpdate = await window.electronAPI.rss.getLastUpdate(); + if (lastUpdate) { + dispatch(setLastUpdate(lastUpdate)); + } + + return items; + } catch (error) { + const msg = error instanceof Error ? error.message : errorMessage; + console.error(errorMessage, error); + dispatch(setError(msg)); + throw error; + } finally { + dispatch(setLoading(false)); + } +} + export const fetchFeedItems = createAsyncThunk( 'rssFeed/fetchItems', - async (_, { dispatch }) => { - try { - dispatch(setLoading(true)); - dispatch(clearError()); - // ... duplicate logic - } - } + async (_, { dispatch }) => fetchRssItemsWithDispatch( + dispatch, + () => window.electronAPI.rss.getFeedItems(), + 'Failed to fetch feed items' + ) );Also applies to: 61-88
src/main/manifest-reader.ts (1)
479-494: Consider reducing log verbosity for missing package keys.
getNpmPackageInfologs a warning at Line 489 when a key is not found. This could generate noise during normal operation when querying for packages that may legitimately not exist in the manifest. Consider usinglog.debuginstead oflog.warnfor the "No NPM package info found for key" message.♻️ Proposed fix
const npmInfo = packageMetadata.npm[key]; if (!npmInfo) { - log.warn('[ManifestReader] No NPM package info found for key:', key); + log.debug('[ManifestReader] No NPM package info found for key:', key); return null; }src/renderer/components/DependencyManagementCardUnified.tsx (2)
43-47: Inconsistent dispatch pattern: mix of thunks and raw action types.Line 43 correctly uses the
checkDependenciesAfterInstallthunk, but Line 46 still uses a rawdispatch({ type: 'dependency/fetchDependencies' }). For consistency with the RTK thunk migration, consider creating and using afetchDependenciesthunk.
131-135: Raw action dispatch should migrate to thunk.
handleInstallAlldispatches a raw action type'dependency/installFromManifest'. This is inconsistent with the thunk-based approach used elsewhere in this file (e.g.,installSingleDependency).src/main/web-service-manager.ts (1)
85-95: Consider simplifying the basePath IIFE.The immediately-invoked function expression for the fallback path is harder to read than a simple conditional.
♻️ Proposed simplification
private getStartupScriptPath(): string { - // Use active version path if available - const basePath = this.activeVersionPath || (() => { - const currentPlatform = this.pathManager.getCurrentPlatform(); - return this.pathManager.getInstalledPath(currentPlatform); - })(); + // Use active version path if available, otherwise fall back to installed path + let basePath = this.activeVersionPath; + if (!basePath) { + const currentPlatform = this.pathManager.getCurrentPlatform(); + basePath = this.pathManager.getInstalledPath(currentPlatform); + } // Return platform-specific script path const scriptName = process.platform === 'win32' ? 'start.bat' : 'start.sh'; return path.join(basePath, scriptName); }src/main/dependency-manager.ts (1)
543-604: Potential resource leak: duplicate 'close' event listener.The
childProcess.on('close', ...)listener is registered twice (lines 579 and 601). When the process closes normally, the first listener resolves/rejects the promise, but if a timeout occurs, the timeout callback kills the process, which also triggers the close event. This can lead to:
clearTimeoutbeing called after the timeout already fired- The promise being resolved/rejected twice (though JS ignores subsequent calls)
Additionally, consider the edge case where the process is killed by timeout but
clearTimeoutstill runs in the second listener.♻️ Suggested refactor to consolidate close handling
private async executeCommandWithRealTimeOutput( command: string, workingDirectory: string, onOutput?: (output: string, isError: boolean) => void ): Promise<boolean> { return new Promise((resolve, reject) => { log.info(`[DependencyManager] Spawning command: ${command}`); + let timeoutId: NodeJS.Timeout | null = null; + let isTimedOut = false; const childProcess = spawn(command, { cwd: workingDirectory, shell: true, stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, FORCE_COLOR: '0' }, }); // ... stdout/stderr handlers ... // Handle process completion childProcess.on('close', (code) => { + if (timeoutId) { + clearTimeout(timeoutId); + } + if (isTimedOut) { + return; // Already handled by timeout + } if (code === 0) { log.info(`[DependencyManager] Command completed successfully`); resolve(true); } else { log.error(`[DependencyManager] Command failed with exit code ${code}`); reject(new Error(`Command failed with exit code ${code}`)); } }); childProcess.on('error', (error) => { + if (timeoutId) { + clearTimeout(timeoutId); + } log.error(`[DependencyManager] Process error:`, error); reject(error); }); // Set timeout (5 minutes) - const timeout = setTimeout(() => { + timeoutId = setTimeout(() => { + isTimedOut = true; childProcess.kill(); reject(new Error('Command execution timeout')); }, 300000); - - childProcess.on('close', () => { - clearTimeout(timeout); - }); }); }src/renderer/store/thunks/dependencyThunks.ts (2)
121-126: Remove empty if block or add TODO comment.This empty if block with only a comment is a code smell. The condition is checked but no action is taken. Either remove the dead code or add a TODO explaining what should be implemented here.
♻️ Remove empty block
// Don't show confirmation dialog - the UI now has direct install buttons // Just store the dependencies in state for the UI to display - if (missingDeps.length > 0 && context === 'onboarding') { - // For onboarding, we already display dependencies with install buttons - // No need to show a separate confirmation dialog - } return missingDeps;
288-289: Arbitrary delays usingsetTimeoutare fragile.Using fixed delays (
setTimeout(..., 100)) to "wait for progress events to be processed" is a race condition waiting to happen. If the system is under load, 100ms may not be enough; if it's fast, it's unnecessary overhead.Consider using a more deterministic approach, such as:
- Waiting for a specific completion event
- Using a Promise-based event that resolves when all progress events are received
- Tracking expected vs. received events
Also applies to: 357-358, 475-476
src/renderer/store/thunks/webServiceThunks.ts (3)
144-146: AvoidgetState() as any- use proper typing.Multiple locations cast
getState()toany, losing type safety. Define and use the properRootStatetype from your store.♻️ Suggested improvement
+import type { RootState } from '../index'; -const state = getState() as any; +const state = getState() as RootState;This requires exporting
RootStatefrom your store configuration if not already done.Also applies to: 305-308, 337-340, 358-360
390-390: Use proper type fordispatchparameter.The helper function
doInstallPackageusesdispatch: any. Use the properAppDispatchtype for type safety.♻️ Suggested fix
+import type { AppDispatch } from '../index'; -async function doInstallPackage(version: string, dispatch: any) { +async function doInstallPackage(version: string, dispatch: AppDispatch) {
354-355: Arbitrary delay to wait for service stop.Using
setTimeout(..., 500)to wait for a service to stop is fragile. The actual stop time can vary. Consider polling the status or using an event-based approach to confirm the service has stopped before proceeding.
| The application MUST manage an embedded .NET-based web service (Hagicode Server) that runs locally as a child process. The web service MUST be started using the `dotnet` command to ensure cross-platform consistency. | ||
|
|
||
| #### Scenario: Starting embedded web service with dotnet command | ||
|
|
||
| **Given** the embedded web service files are present in the installation directory | ||
| **When** the user requests to start the web service | ||
| **Then** the application MUST spawn a process using `dotnet PCode.Web.dll` command | ||
| **And** this command MUST be used consistently on Windows, macOS, and Linux platforms | ||
| **And** the service MUST start within 30 seconds | ||
| **And** the application MUST display the service status as "running" | ||
|
|
||
| #### Scenario: Dotnet command execution on Windows | ||
|
|
||
| **Given** the application is running on Windows | ||
| **When** the web service needs to start | ||
| **Then** the application MUST execute `dotnet PCode.Web.dll` with appropriate working directory | ||
| **And** the process MUST be spawned with detached option for independent execution | ||
| **And** process output MUST be logged for debugging |
There was a problem hiding this comment.
Resolve the mismatch between “child process” ownership and Windows detached execution.
The spec says the service must run as a child process, but the Windows scenario mandates detached (independent execution). That conflicts with parent-managed lifecycle/status expectations and can cause orphaned processes. Please clarify whether Windows should stay parent-managed (no detached) or update the child-process requirement to allow platform-specific ownership.
🤖 Prompt for AI Agents
In
`@openspec/changes/archive/2026-02-10-dotnet-web-service-startup/specs/electron-app/spec.md`
around lines 5 - 22, The spec contradicts itself by requiring the service to be
a parent-managed child process while the Windows scenario requires spawning with
the detached option; reconcile this by updating the spec text: either (A) state
explicitly that the service must be parent-managed on all platforms and remove
the "detached" requirement from the "Dotnet command execution on Windows"
scenario (ensure the process is spawned without detached and the parent manages
lifecycle/status), or (B) allow a platform-specific ownership exception by
changing the global "child process" requirement to "process (child-managed
except on Windows where detached is permitted)" and adjust the Windows scenario
to document that lifecycle/status must be tracked via alternative means; update
the relevant lines in "Scenario: Starting embedded web service with dotnet
command" and "Scenario: Dotnet command execution on Windows" accordingly to
remove the contradiction.
| ### Requirement: 依赖状态可视化 | ||
|
|
||
| 应用 MUST 在依赖管理面板中以清晰直观的方式展示依赖状态，包括安装状态、版本信息和可用操作。 | ||
|
|
||
| #### Scenario: 显示版本不匹配的依赖 | ||
|
|
||
| **Given** 依赖已安装但版本低于要求 | ||
| **When** 渲染依赖管理面板 | ||
| **Then** 显示黄色 ⚠️ 图标 | ||
| **And** 显示"版本不匹配"状态文本 | ||
| **And** 显示当前版本和要求的版本范围 | ||
| **And** 显示"升级"或"重新安装"按钮 | ||
| **And** 在引导流程中，将版本不匹配的依赖视为需要处理的状态 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find base spec file for dependency-management
echo "=== Base spec file ==="
cat openspec/specs/dependency-management/spec.md
echo ""
echo "=== Delta spec file ==="
cat openspec/changes/archive/2026-02-10-onboarding-dependency-version-mismatch-handling/specs/dependency-management/spec.mdRepository: HagiCode-org/desktop
Length of output: 6266
Include the full original requirement block in the MODIFIED section.
The delta spec must copy all four original scenarios from the base spec, not just the modified one. Specifically, add back the missing scenarios:
- 显示已安装且版本匹配的依赖
- 显示未安装的依赖
- 显示检测中的加载状态
Then apply your modifications to "显示版本不匹配的依赖" and add your two new scenarios ("引导流程显示混合依赖状态" and "引导流程仅版本不匹配的依赖") within the MODIFIED section.
Per specification guidelines, delta specs for MODIFIED requirements must include the entire original requirement block before edits, not omit unchanged scenarios.
🤖 Prompt for AI Agents
In
`@openspec/changes/archive/2026-02-10-onboarding-dependency-version-mismatch-handling/specs/dependency-management/spec.md`
around lines 10 - 22, The MODIFIED delta must include the entire original
requirement block titled "Requirement: 依赖状态可视化" including all four original
scenarios ("显示已安装且版本匹配的依赖", "显示未安装的依赖", "显示检测中的加载状态", and "显示版本不匹配的依赖") before
any edits; restore any omitted scenarios into the MODIFIED section, then apply
your edits to the "显示版本不匹配的依赖" scenario (add the yellow ⚠️ icon, "版本不匹配" text,
current vs required version, and "升级/重新安装" buttons and treat as actionable in
guides) and append the two new scenarios "引导流程显示混合依赖状态" and "引导流程仅版本不匹配的依赖"
under the same MODIFIED block so the delta contains the full original block plus
your changes.
| "productName": "Hagicode Desktop", | ||
| "version": "0.1.0", | ||
| "type": "module", | ||
| "packageManager": "npm@10.9.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for node/npm version enforcement and corepack usage.
fd -a -t f '\.nvmrc|\.node-version|\.tool-versions|\.npmrc' .
rg -n "corepack|npm-version|node-version|packageManager" .github/workflows 2>/dev/null || trueRepository: HagiCode-org/desktop
Length of output: 385
🏁 Script executed:
#!/bin/bash
# Check NODE_VERSION definition and setup-node configuration
rg -B 5 -A 5 "NODE_VERSION" .github/workflows/build.yml | head -40
rg -B 5 -A 5 "NODE_VERSION" .github/workflows/pr-checks.yml | head -40
rg "uses: actions/setup-node" .github/workflows/*.yml -A 5 | head -50Repository: HagiCode-org/desktop
Length of output: 3038
🏁 Script executed:
#!/bin/bash
# Check for Corepack enablement and npm version enforcement
rg "enable.*corepack|corepack.*enable" .github/workflows/*.yml 2>/dev/null || echo "No Corepack enable found in workflows"
rg -n "scripts" package.json -A 20 | head -40
fd -t f 'README|CONTRIBUTING|\.md' . | xargs rg -l "npm.*version|corepack" 2>/dev/null || trueRepository: HagiCode-org/desktop
Length of output: 2479
Enable Corepack to enforce npm@10.9.2 in CI.
The packageManager field is declared but not enforced since Corepack is not enabled in CI workflows. Node 22 will use its bundled npm version (not 10.9.2), creating environment inconsistencies. Either enable Corepack in setup-node to enforce the pinned version, or align packageManager with Node 22's bundled npm version.
🤖 Prompt for AI Agents
In `@package.json` at line 6, The packageManager entry ("packageManager":
"npm@10.9.2") is not being enforced in CI because Corepack isn't enabled; update
the CI workflow that uses the setup-node action to enable Corepack (so the
packageManager field is honored) or change the packageManager value to match
Node 22's bundled npm version; specifically modify the CI step that calls
setup-node to set cache: 'npm' and enable corepack (or run corepack enable) so
the packageManager field is enforced, or update the packageManager symbol to the
Node 22 bundled npm version for consistency.
| // Check if debug mode is enabled (ignore dependency check) | ||
| const debugMode = configManager.getStore().get('debugMode') as { ignoreDependencyCheck: boolean } | undefined; | ||
| if (debugMode?.ignoreDependencyCheck) { | ||
| // Force all dependencies to appear as not installed | ||
| return dependencies.map(dep => ({ | ||
| ...dep, | ||
| installed: false, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: Debug mode check repeated in 4 IPC handlers.
The debug mode check logic that forces dependencies to appear as not installed is duplicated across:
check-dependencies(lines 520-528)version:checkDependencies(lines 687-695)dependency:get-missing(lines 1063-1073)dependency:get-all(lines 1091-1101)
Extract this into a helper function to improve maintainability and ensure consistent behavior.
♻️ Suggested helper function
/**
* Apply debug mode transformation to dependencies if enabled
*/
function applyDebugModeIfEnabled(dependencies: DependencyCheckResult[]): DependencyCheckResult[] {
const debugMode = configManager.getStore().get('debugMode') as { ignoreDependencyCheck: boolean } | undefined;
if (debugMode?.ignoreDependencyCheck) {
return dependencies.map(dep => ({
...dep,
installed: false,
versionMismatch: true,
}));
}
return dependencies;
}Then use it in each handler:
- // Check if debug mode is enabled (ignore dependency check)
- const debugMode = configManager.getStore().get('debugMode') as { ignoreDependencyCheck: boolean } | undefined;
- if (debugMode?.ignoreDependencyCheck) {
- // Force all dependencies to appear as not installed
- return dependencies.map(dep => ({
- ...dep,
- installed: false,
- }));
- }
- return dependencies;
+ return applyDebugModeIfEnabled(dependencies);Also applies to: 687-695, 1063-1073, 1091-1101
🤖 Prompt for AI Agents
In `@src/main/main.ts` around lines 520 - 528, Extract the duplicated debug-mode
transformation into a single helper (e.g. function
applyDebugModeIfEnabled(dependencies: DependencyCheckResult[]):
DependencyCheckResult[]) that reads configManager.getStore().get('debugMode')
and, when ignoreDependencyCheck is true, returns dependencies.map(dep => ({
...dep, installed: false, versionMismatch: true })), otherwise returns the
original array; then replace the duplicated blocks in the handlers that produce
a dependencies array (the handlers for check-dependencies,
version:checkDependencies, dependency:get-missing, dependency:get-all) to call
applyDebugModeIfEnabled(dependencies) before returning so all four handlers
share the same behavior.
| /** | ||
| * Region type based on user location | ||
| */ | ||
| export type Region = 'CN' | 'INTERNATIONAL'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if both files exist and read their content
echo "=== Checking region-detector.ts ==="
cat -n src/main/region-detector.ts
echo -e "\n=== Checking manifest-reader.ts around line 116 ==="
sed -n '100,130p' src/main/manifest-reader.ts | cat -nRepository: HagiCode-org/desktop
Length of output: 5975
🏁 Script executed:
# Search for all Region type definitions and imports
echo "=== Searching for Region type definitions ==="
rg -n "type Region|interface Region" --type=ts -B2 -A2
echo -e "\n=== Searching for Region imports and usages ==="
rg -n "import.*Region|from.*Region|Region" --type=ts -B1 -A1 | head -100Repository: HagiCode-org/desktop
Length of output: 6826
🏁 Script executed:
# Check if there are multiple Region type definitions
echo "=== Finding all Region type declarations ==="
rg -n "Region\s*=" --type=ts -B1 -A1Repository: HagiCode-org/desktop
Length of output: 837
Type conflict: Two incompatible Region type definitions.
This file exports Region = 'CN' | 'INTERNATIONAL' (line 9), but src/main/manifest-reader.ts (line 117) exports Region = 'china' | 'global'. The RegionDetector returns 'CN' and 'INTERNATIONAL', but the manifest parsing code expects 'china' and 'global'. This mismatch will cause type errors and runtime failures when the detected region is passed to manifest commands.
Additionally, classifying zh-TW and zh-HK as 'CN' (lines 22-23) is geopolitically problematic and functionally incorrect for region-specific package installation.
🤖 Prompt for AI Agents
In `@src/main/region-detector.ts` at line 9, The exported Region type ('Region')
in region-detector.ts must be changed to match manifest-reader.ts (use 'china' |
'global' rather than 'CN' | 'INTERNATIONAL') and RegionDetector (or its detect
method) should return those string literals; update any internal mapping logic
that currently returns 'CN'/'INTERNATIONAL' to return 'china'/'global' instead
so types align across modules (Region type, RegionDetector.detect). Also stop
classifying 'zh-TW' and 'zh-HK' as the China region — map those locales to
'global' (or the equivalent non-China value) so region-specific package
selection is correct, and update all usages that rely on the previous values to
the new literals.
| // On Unix, ensure script has execute permissions | ||
| if (process.platform !== 'win32') { | ||
| fs.chmod(scriptPath, 0o755).catch(error => { | ||
| log.warn('[WebService] Failed to set execute permissions on script:', error); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential race condition: fs.chmod is fire-and-forget.
The fs.chmod call at Lines 119-121 is not awaited, meaning the spawn could occur before the script has execute permissions set. This could cause the spawn to fail on fresh installs where the script doesn't yet have execute permissions.
🔧 Proposed fix: Make chmod synchronous or ensure it completes before spawning
Consider moving the chmod call to the start() method before spawning, and awaiting it:
private getSpawnOptions() {
const scriptPath = this.getStartupScriptPath();
const options: any = {
env: { ...process.env, ...this.config.env },
cwd: path.dirname(scriptPath),
};
- // On Unix, ensure script has execute permissions
- if (process.platform !== 'win32') {
- fs.chmod(scriptPath, 0o755).catch(error => {
- log.warn('[WebService] Failed to set execute permissions on script:', error);
- });
- }
-
// Windows: detach to run independently
// Unix: keep attached for lifecycle managementThen in start() before spawning:
+ // Ensure execute permissions on Unix before spawning
+ if (process.platform !== 'win32') {
+ try {
+ await fs.chmod(scriptPath, 0o755);
+ log.info('[WebService] Set execute permissions on script:', scriptPath);
+ } catch (error) {
+ log.warn('[WebService] Failed to set execute permissions:', error);
+ }
+ }
+
// Spawn the process
this.emitPhase(StartupPhase.Spawning, 'Starting service with startup script...');🤖 Prompt for AI Agents
In `@src/main/web-service-manager.ts` around lines 117 - 122, The chmod call on
scriptPath uses fs.chmod as fire-and-forget causing a race where spawn may run
before permissions are set; move the chmod logic into the start() method (or
otherwise ensure it runs immediately before spawning) and await the
fs.chmod(scriptPath) promise (handle errors with log.warn) so that start() only
spawns the process after permissions are confirmed; update any existing
top-level chmod call to remove the unawaited invocation and reference
scriptPath, start(), and the spawn invocation when implementing this change.
| export const checkDependenciesAfterInstall = createAsyncThunk( | ||
| 'webService/checkDependenciesAfterInstall', | ||
| async (params: { versionId: string }, { dispatch }) => { | ||
| try { | ||
| // This will trigger the dependency saga to check for missing dependencies | ||
| // We dispatch an action that the listener middleware can pick up | ||
| dispatch({ type: 'dependency/checkAfterInstall', payload: { versionId: params.versionId } }); | ||
| } catch (error) { | ||
| console.error('Failed to check dependencies after install:', error); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
Duplicate thunk action type with dependencyThunks.ts.
This checkDependenciesAfterInstall thunk uses action type 'webService/checkDependenciesAfterInstall', but dependencyThunks.ts has a thunk named checkDependenciesAfterInstall with type 'dependency/checkAfterInstall'.
More critically, line 537 dispatches { type: 'dependency/checkAfterInstall', payload: ... } which relies on listener middleware to pick it up. This is an unconventional pattern - if the actual thunk from dependencyThunks.ts should be called, import and dispatch it directly instead of relying on action type matching.
🔧 Suggested fix: Import and dispatch the actual thunk
+import { checkDependenciesAfterInstall as checkDepsThunk } from './dependencyThunks';
export const checkDependenciesAfterInstall = createAsyncThunk(
'webService/checkDependenciesAfterInstall',
async (params: { versionId: string }, { dispatch }) => {
try {
- // This will trigger the dependency saga to check for missing dependencies
- // We dispatch an action that the listener middleware can pick up
- dispatch({ type: 'dependency/checkAfterInstall', payload: { versionId: params.versionId } });
+ await dispatch(checkDepsThunk({ versionId: params.versionId, context: 'version-management' }));
} catch (error) {
console.error('Failed to check dependencies after install:', error);
}
}
);🤖 Prompt for AI Agents
In `@src/renderer/store/thunks/webServiceThunks.ts` around lines 531 - 542, The
current checkDependenciesAfterInstall thunk in webServiceThunks dispatches a raw
action with type 'dependency/checkAfterInstall' instead of invoking the actual
thunk from dependencyThunks.ts and duplicates the thunk name/type; fix by
importing the real thunk (the checkDependenciesAfterInstall exported from
dependencyThunks.ts or rename one to avoid collision) and dispatch that thunk
(dispatch(checkDependenciesAfterInstall({ versionId: params.versionId })))
rather than dispatching a plain action object, and remove or change the
conflicting action type string to prevent duplicate thunk/action naming.
Summary by CodeRabbit
New Features
Improvements