feat: windows install acp agent helper #1115
Conversation
WalkthroughAdds pre-initialization external dependency checks for ACP agents (e.g., Git Bash), exposes clipboard read API and paste action in the ACP terminal, adjusts system-install npm environment handling, introduces a dependency dialog UI, and updates i18n strings across locales. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (AcpTerminalDialog)
participant Preload as Preload API
participant Main as Main Process (acpInitHelper)
participant UI as Renderer (AcpDependencyDialog)
Renderer->>Main: Request initialize ACP agent
Main->>Main: checkRequiredDependencies()
alt Missing dependencies
Main->>Renderer: external-deps-required (IPC)
Renderer->>Renderer: handleExternalDepsRequired()
Renderer->>UI: emit dependencies-required (open dialog)
UI->>UI: show dependency list / copy commands
else All dependencies OK
Main->>Renderer: initialization proceeds
Renderer->>Renderer: terminal session active
end
rect rgba(200,220,240,0.6)
Note over Renderer,Preload: Paste flow
Renderer->>Preload: readClipboardText()
Preload->>Renderer: clipboard text
Renderer->>Main: send pasted text to terminal input (IPC)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/main/presenter/configPresenter/index.ts (1)
1047-1072: Graceful handling of missing dependencies in builtin initThe new
const result = await initializeBuiltinAgent(...); if (result === null) returncorrectly turns the “missing dependencies” case into a no-op from the caller’s perspective, relying on the helper to notify the renderer. This avoids throwing while keeping the happy path unchanged.If you like, you could inline the check to avoid the temporary variable, but that’s purely stylistic.
src/renderer/settings/components/AcpSettings.vue (1)
260-270: Dependency dialog wiring and event flow look solid; consider reusing shared typeThe new flow is coherent:
AcpTerminalDialogemitsdependencies-requiredwhen the main process reports missing deps.handleDependenciesRequiredstores the payload, closes the terminal dialog, and opensAcpDependencyDialog.AcpDependencyDialogreceivesmissingDependenciesplusopenas props and is controlled viaupdate:open.This should give a clean UX transition from the terminal to the dependency explanation.
The only concern is type duplication:
missingDependenciesinlines the dependency shape locally. Since the PR already introduces a sharedExternalDependencytype, it would be safer to reuse that to avoid drift if fields change later, e.g.:import type { ExternalDependency } from '@/shared/…' // adjust path const missingDependencies = ref<ExternalDependency[]>([]) const handleDependenciesRequired = (dependencies: ExternalDependency[]) => { console.log('[AcpSettings] Dependencies required:', dependencies) missingDependencies.value = dependencies dependencyDialogOpen.value = true terminalDialogOpen.value = false }Also applies to: 306-343, 707-712
src/renderer/settings/components/AcpDependencyDialog.vue (3)
13-15: Prefer stable keys for dependency list renderingUsing
:key="index"works but is fragile if the list ever reorders or items are inserted/removed. You already have a natural key (dep.name, or\${dep.name}-${dep.platform?.join(',')}``) that would be more stable and reduce unnecessary re-renders.- v-for="(dep, index) in dependencies" - :key="index" + v-for="dep in dependencies" + :key="dep.name"
104-117: DeduplicateExternalDependencytype across main and rendererThe
ExternalDependencyinterface is duplicated (here, inAcpTerminalDialog.vue, and inacpInitHelper.ts). That increases the chance of fields drifting over time (e.g., new properties added in one place only). Consider moving this type to a shared module (e.g., in@sharedor a small TS file imported by both main and renderer) and reusing it everywhere.- interface ExternalDependency { - name: string - description: string - ... - } +// import type { ExternalDependency } from '@shared/acpDependencies'
131-160: Provide user feedback when clipboard APIs are unavailable
copyToClipboardlogs a warning when neitherwindow.api.copyTextnornavigator.clipboardexist, but the user gets no visual feedback. It would be more user-friendly to show the same destructive toast used in the error path so they know copy didn’t work.} else { - console.warn('[AcpDependencyDialog] Clipboard API not available') + console.warn('[AcpDependencyDialog] Clipboard API not available') + toast({ + title: t('settings.acp.dependency.copyFailed'), + variant: 'destructive' + }) }src/main/presenter/configPresenter/acpInitHelper.ts (1)
18-51: CentralizeExternalDependencytype and constant for cross-process consistencyThis backend
ExternalDependencyinterface andEXTERNAL_DEPENDENCIESshape are mirrored in at least two renderer components. To avoid drift between what main sends and what the renderer expects, it would be better to define the type (and possibly the base metadata) in a shared module (e.g.,@shared/acpDependencies) and import it here and in the renderer, instead of redefining the interface multiple times.- interface ExternalDependency { ... } - const EXTERNAL_DEPENDENCIES: ExternalDependency[] = [...] + // import type { ExternalDependency } from '@shared/acpDependencies' + // import { EXTERNAL_DEPENDENCIES } from '@shared/acpDependencies'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
build/nsis-installer.nsh(1 hunks)src/main/lib/runtimeHelper.ts(1 hunks)src/main/presenter/configPresenter/acpInitHelper.ts(6 hunks)src/main/presenter/configPresenter/index.ts(1 hunks)src/preload/index.d.ts(1 hunks)src/preload/index.ts(1 hunks)src/renderer/settings/components/AcpDependencyDialog.vue(1 hunks)src/renderer/settings/components/AcpSettings.vue(4 hunks)src/renderer/settings/components/AcpTerminalDialog.vue(7 hunks)src/renderer/src/i18n/en-US/settings.json(1 hunks)src/renderer/src/i18n/fa-IR/settings.json(1 hunks)src/renderer/src/i18n/fr-FR/settings.json(2 hunks)src/renderer/src/i18n/ja-JP/settings.json(1 hunks)src/renderer/src/i18n/ko-KR/settings.json(1 hunks)src/renderer/src/i18n/pt-BR/settings.json(1 hunks)src/renderer/src/i18n/ru-RU/settings.json(1 hunks)src/renderer/src/i18n/zh-CN/settings.json(1 hunks)src/renderer/src/i18n/zh-HK/settings.json(2 hunks)src/renderer/src/i18n/zh-TW/settings.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/lib/runtimeHelper.ts (1)
test/mocks/electron.ts (1)
app(2-10)
src/main/presenter/configPresenter/index.ts (2)
src/main/presenter/configPresenter/acpInitHelper.ts (2)
initializeBuiltinAgent(177-232)initializeBuiltinAgent(625-641)src/shared/types/presenters/legacy.presenters.d.ts (1)
AcpBuiltinAgentId(678-678)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-windows (x64)
🔇 Additional comments (21)
src/preload/index.ts (1)
25-27: LGTM! Clean clipboard read implementation.The
readClipboardTextmethod follows the established pattern and correctly exposes Electron's clipboard API to the renderer process.src/renderer/src/i18n/zh-CN/settings.json (1)
875-892: LGTM! Localization additions support new features.The new translation keys for clipboard paste functionality and dependency notifications are properly structured and align with the PR's feature set.
build/nsis-installer.nsh (1)
133-133: LGTM! Formatting-only change.This whitespace removal has no functional impact on the installer behavior.
src/renderer/src/i18n/zh-TW/settings.json (1)
892-909: LGTM! Consistent localization for Traditional Chinese.The translation additions for dependency handling and paste functionality are properly structured and align with other locale files.
src/main/lib/runtimeHelper.ts (2)
323-345: LGTM! Appropriate Windows system directory detection.The method correctly identifies when the app is installed in a system directory on Windows by checking for "program files" in the normalized path. The logging is helpful for troubleshooting.
347-359: LGTM! Correct npm user prefix path for Windows.The method returns the standard Windows user npm prefix location (
%APPDATA%\npm), which is used when the app needs to install npm packages without system-level permissions.src/renderer/src/i18n/fr-FR/settings.json (2)
895-895: Minor punctuation improvement.The colon formatting in the exit error message is now more consistent.
907-918: LGTM! French localization for new features.The translation additions for clipboard paste and dependency notifications are properly structured.
src/renderer/src/i18n/fa-IR/settings.json (1)
906-918: LGTM! Persian localization additions.The translation keys for clipboard paste and dependency handling are properly added with appropriate RTL Persian text.
src/renderer/src/i18n/ja-JP/settings.json (1)
906-918: LGTM! Japanese localization complete.The translation additions for clipboard paste and dependency notifications are properly structured and align with other locale files.
src/preload/index.d.ts (1)
7-14: Clipboard API typing looks consistent
readClipboardText(): stringmatches the synchronous Electron clipboard API and is consistent with otherWindow.apimethods here.src/renderer/src/i18n/en-US/settings.json (1)
875-892: New terminal/dependency strings are clear and aligned with behaviorThe
terminal.paste/terminal.pasteErrorlabels and theacp.dependencyblock read naturally and match the intended UX for missing dependencies and clipboard paste.src/renderer/src/i18n/ru-RU/settings.json (1)
893-918: RU terminal/dependency translations look correctThe new
terminal.waiting/paste/pasteErrorstrings and thedependencyblock are idiomatic Russian and accurately mirror the English meanings, so the new UI should render fine for this locale.src/renderer/src/i18n/zh-HK/settings.json (1)
899-917: ZH-HK terminal/dependency strings align with new ACP UI
terminal.paste/pasteErrorand thedependencylabels (“已復製到剪貼板”, “缺少外部依賴”, etc.) are clear and consistent with the English source, so the new dialogs should display correctly in zh-HK.src/main/presenter/configPresenter/acpInitHelper.ts (3)
83-143: Dependency check flow for builtin agents looks solidThe multi-step
checkExternalDependency(direct command, known paths, thenwhere/which) and platform gating incheckRequiredDependenciesgive a good balance of robustness and safety. Returningtruewhenplatformdoesn’t match is a nice way to avoid spurious failures on unsupported OSes, and the 5-second timeouts should keep startup responsive with the small, fixed dependency list.
194-209: Short-circuiting builtin initialization on missing deps is a good patternBlocking
initializeBuiltinAgentwhenmissingDeps.length > 0and emitting anexternal-deps-requiredpayload to the renderer gives a clean contract: either you get a live PTY or you get a structured dependency list. ThePromise<IPty | null>return type reflects this nicely, and avoiding a thrown error here should make the UI flow much smoother.
549-577: Code is correct;getUserNpmPrefix()returns the proper npm bin directoryVerification confirms that
getUserNpmPrefix()correctly returnsC:\Users\...\AppData\Roaming\npm, which is both the npm global prefix and the directory containing globally installed binaries on Windows. The code properly uses this value for bothnpm_config_prefix/NPM_CONFIG_PREFIXand PATH prepending. No changes needed.src/renderer/settings/components/AcpTerminalDialog.vue (4)
12-29: Status badge + paste button integration looks goodThe new header layout with a compact status pill and a dedicated paste button is straightforward, and
:disabled="!isRunning"on the paste action avoids sending input when no process is alive. The staticstatusBadgeClasscomputed is fine for now and leaves room for future styling based on status.
283-299: External dependency notification wiring is clean and decoupled
handleExternalDepsRequiredsimply forwardsmissingDepsto the parent viadependencies-requiredand then closes the dialog, which keeps the terminal component focused on I/O while letting settings/UI decide how to present the dependency dialog. The defensive early return whenmissingDepsis empty is also a nice touch.
301-326: IPC listener setup/teardown covers newexternal-deps-requiredchannelAdding and removing the
external-deps-requiredlistener alongside the existingacp-init:*listeners keeps the lifecycle consistent and avoids dangling handlers after the dialog closes or unmounts. This should prevent duplicate events across multiple openings.
261-281: I found the implementation. Now let me verify Electron'sclipboard.readText()API to confirm whether it's synchronous or asynchronous.Remove suggestion to add
await—readClipboardTextis synchronousElectron's
clipboard.readText()returns a string directly, not a Promise. The documentation shows usage asconst text = clipboard.readText()without await. The preload wrapper insrc/preload/index.ts(lines 25–27) returnsclipboard.readText()synchronously, and the type definition (src/preload/index.d.ts, line 9) declaresreadClipboardText(): string, confirming synchronous behavior. The current code inAcpTerminalDialog.vueline 268 is correct and requires no changes.Likely an incorrect or invalid review comment.
| const handleOpenUpdate = (val: boolean) => { | ||
| if (!val) { | ||
| handleManualClose() | ||
| } | ||
| } | ||
| const handleManualClose = () => { | ||
| // Kill process if running | ||
| if (isRunning.value) { | ||
| if (window.electron) { | ||
| window.electron.ipcRenderer.send('acp-terminal:kill') | ||
| // Kill process if running | ||
| if (isRunning.value) { | ||
| if (window.electron) { | ||
| window.electron.ipcRenderer.send('acp-terminal:kill') | ||
| } | ||
| } | ||
| emit('update:open', false) | ||
| emit('close') | ||
| } else { | ||
| emit('update:open', val) | ||
| } |
There was a problem hiding this comment.
Ensure backend process is also killed on programmatic close, not just user-triggered
handleOpenUpdate sends acp-terminal:kill only when the dialog is closed via the Dialog component’s own update:open event. If the parent sets open to false programmatically while isRunning is still true, the watcher will clean up the terminal and listeners but the backend process may continue running. Consider mirroring the kill behavior in the watcher when open transitions to false so all close paths terminate the PTY.
watch(
() => props.open,
async (open) => {
if (open) {
...
} else {
console.log('[AcpTerminal] Dialog closing, cleaning up')
+ if (isRunning.value && window.electron) {
+ window.electron.ipcRenderer.send('acp-terminal:kill')
+ }
removeIpcListeners()
cleanupTerminal()
}
}
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/renderer/settings/components/AcpTerminalDialog.vue around lines 113–125,
the current logic sends 'acp-terminal:kill' only when the Dialog emits
update:open (user close) but not when the parent programmatically sets open to
false; update the watcher that reacts to the open prop so that when open
transitions to false it also checks if isRunning.value is true and, if
window.electron is available, sends
window.electron.ipcRenderer.send('acp-terminal:kill') before performing the
existing listener/terminal cleanup to ensure the backend PTY is terminated on
all close paths.
Summary by CodeRabbit
New Features
UI/UX Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.