Conversation
Contributor
📝 WalkthroughWalkthroughAdds an ACP Registry agent uninstall workflow: orchestration entrypoint, service-level uninstall and path-safe filesystem cleanup, repository state reset, renderer uninstall UI with confirmation, i18n/type additions, and tests covering repository, service, and UI behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Renderer)
participant UI as AcpSettings UI
participant CP as ConfigPresenter
participant LS as AcpLaunchSpecService
participant AR as AgentRepository
participant FS as File System
participant DB as SQLite DB
User->>UI: Click uninstall button
UI->>UI: Show AlertDialog confirmation
User->>UI: Confirm uninstall
UI->>CP: uninstallAcpRegistryAgent(agentId)
CP->>LS: uninstallRegistryAgent(agent, installState)
LS->>LS: Resolve paths & validate with isPathWithinRoot
alt installDir within managed root
LS->>FS: Remove installDir (recursive)
FS-->>LS: Deleted
LS->>FS: Prune empty agents/<agentId> dir
FS-->>LS: Pruned
else unsafe installDir
LS-->>CP: throw Unsafe ACP install directory for uninstall
end
LS-->>CP: cleanup complete
CP->>AR: clearRegistryAcpAgentInstallation(agentId, not_installed)
AR->>DB: SELECT agent row
DB-->>AR: Agent record
AR->>DB: UPDATE enabled=0, state_json.installState=not_installed
DB-->>AR: Updated
AR-->>CP: state reset
CP->>CP: handleAcpAgentsMutated([agentId])
CP-->>UI: notify completion
UI->>UI: Reload ACP data and show success toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types/i18n.d.ts (1)
1998-2003:⚠️ Potential issue | 🟡 MinorAdd missing
registryUninstallDescriptionin ACP i18n type contract.
acp.registryUninstallDescriptionis present in locale JSON but absent inDefineLocaleMessage.acp, so typed locale coverage is incomplete.💡 Proposed fix
registryInstallAction: string registryUninstallAction: string registryUninstallConfirm: string + registryUninstallDescription: string registryInstallTitle: string registryInstallDescription: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/i18n.d.ts` around lines 1998 - 2003, The ACP i18n type is missing the registryUninstallDescription property, causing the typed locale contract (DefineLocaleMessage.acp) to not match the locale JSON; add a new property named registryUninstallDescription: string to the ACP section of the DefineLocaleMessage type (or the interface/type where registryUninstallAction, registryUninstallConfirm, registryInstallTitle, etc. are declared) so the TypeScript type covers acp.registryUninstallDescription used in the locales.
🧹 Nitpick comments (2)
test/main/presenter/agentRepository.test.ts (1)
1-2: Mirror the source path in the test tree.This suite exercises
src/main/presenter/agentRepository/index.ts, so it should live undertest/main/presenter/agentRepository/index.test.tsinstead of flattening the directory here. That keeps source and test navigation one-to-one.As per coding guidelines, "Vitest test suites should mirror the source structure under
test/main/**andtest/renderer/**"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/agentRepository.test.ts` around lines 1 - 2, Move this test to mirror the source directory: rename and relocate test/main/presenter/agentRepository.test.ts to test/main/presenter/agentRepository/index.test.ts so the test tree mirrors src/main/presenter/agentRepository/index.ts; update any relative imports in the moved file as needed (the test references AgentRepository) so they still resolve from the new location.test/renderer/components/AcpSettings.test.ts (1)
1-4: Mirror the component path in the renderer test tree.The source under test is
src/renderer/settings/components/AcpSettings.vue, so this suite should live undertest/renderer/settings/components/AcpSettings.test.ts. Right now thesettings/segment is dropped.As per coding guidelines, "Vitest test suites should mirror the source structure under
test/main/**andtest/renderer/**"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/AcpSettings.test.ts` around lines 1 - 4, The test file for AcpSettings.vue is placed under test/renderer/components but must mirror the source tree (the source component is settings/components/AcpSettings.vue); move or rename the test so it lives under the renderer test tree that includes the settings segment (i.e., test/renderer/settings/components) and update any imports or test runner references accordingly so the test file remains named AcpSettings.test.ts and still imports the AcpSettings component correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/acp-agent-uninstall/plan.md`:
- Line 14: Update the plan text that currently mentions using `window.confirm`
to instead describe the AlertDialog-based confirmation flow; replace the phrase
"window.confirm" with a description like "AlertDialog-based confirmation
(AlertDialog component/modal)" and note that it uses the AlertDialog UI flow for
lightweight confirmation so the document matches the implemented `AlertDialog`
component behavior.
In `@src/renderer/settings/components/AcpSettings.vue`:
- Around line 548-550: The copy shown by
t('settings.acp.registryUninstallDescription') is inaccurate for non-binary
installs; update the UI to use distribution-neutral wording or choose the
message based on the agent's distribution type: inspect where the template
renders t('settings.acp.registryUninstallDescription') in AcpSettings.vue and
replace it with either a new generic key (e.g.,
settings.acp.registryUninstallGeneric) or a conditional that picks keys like
settings.acp.registryUninstallBinary vs settings.acp.registryUninstallNpx based
on the agent/distribution property, and ensure the logic aligns with
ConfigPresenter.uninstallAcpRegistryAgent() and
AcpLaunchSpecService.uninstallRegistryAgent() behavior so the displayed text
matches what is actually removed.
- Around line 924-931: The catch block in uninstallRegistryAgent is using the
save-failed toast key (t('settings.acp.saveFailed')) which is incorrect for an
uninstall operation; update the call to handleError to pass
t('settings.acp.registryUninstallFailed') instead, and add the new
settings.acp.registryUninstallFailed string to the locale bundles next to other
uninstall strings so translations exist; locate the uninstallRegistryAgent
function and the catch that calls handleError(error, undefined, t(...)) and the
toast usage to make this change.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Around line 1273-1275: Add the missing i18n type for the key
"registryUninstallDescription" by updating the i18n type declarations (in
src/types/i18n.d.ts) to include registryUninstallDescription: string on the
shared locale keys/interface (the same interface where registryUninstallAction
and registryUninstallConfirm are declared) so all locales including ja-JP are
properly typed for this key.
---
Outside diff comments:
In `@src/types/i18n.d.ts`:
- Around line 1998-2003: The ACP i18n type is missing the
registryUninstallDescription property, causing the typed locale contract
(DefineLocaleMessage.acp) to not match the locale JSON; add a new property named
registryUninstallDescription: string to the ACP section of the
DefineLocaleMessage type (or the interface/type where registryUninstallAction,
registryUninstallConfirm, registryInstallTitle, etc. are declared) so the
TypeScript type covers acp.registryUninstallDescription used in the locales.
---
Nitpick comments:
In `@test/main/presenter/agentRepository.test.ts`:
- Around line 1-2: Move this test to mirror the source directory: rename and
relocate test/main/presenter/agentRepository.test.ts to
test/main/presenter/agentRepository/index.test.ts so the test tree mirrors
src/main/presenter/agentRepository/index.ts; update any relative imports in the
moved file as needed (the test references AgentRepository) so they still resolve
from the new location.
In `@test/renderer/components/AcpSettings.test.ts`:
- Around line 1-4: The test file for AcpSettings.vue is placed under
test/renderer/components but must mirror the source tree (the source component
is settings/components/AcpSettings.vue); move or rename the test so it lives
under the renderer test tree that includes the settings segment (i.e.,
test/renderer/settings/components) and update any imports or test runner
references accordingly so the test file remains named AcpSettings.test.ts and
still imports the AcpSettings component correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39d30c0b-bb2d-4b47-93ca-a2f8e083a1da
📒 Files selected for processing (24)
docs/specs/acp-agent-uninstall/plan.mddocs/specs/acp-agent-uninstall/spec.mdsrc/main/presenter/agentRepository/index.tssrc/main/presenter/configPresenter/acpLaunchSpecService.tssrc/main/presenter/configPresenter/index.tssrc/renderer/settings/components/AcpSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/ui/agent.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/types/i18n.d.tstest/main/presenter/agentRepository.test.tstest/main/presenter/configPresenter/acpLaunchSpecService.test.tstest/renderer/components/AcpSettings.test.ts
Comment on lines
+548
to
+550
| <AlertDialogDescription> | ||
| {{ t('settings.acp.registryUninstallDescription') }} | ||
| </AlertDialogDescription> |
Contributor
There was a problem hiding this comment.
Use distribution-neutral uninstall copy.
ConfigPresenter.uninstallAcpRegistryAgent() clears install state for every registry agent, but AcpLaunchSpecService.uninstallRegistryAgent() only removes files for binary distributions. For installed npx/uvx agents this description is inaccurate, because there may be nothing on disk to remove. Please make the copy generic or branch it on distribution type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/settings/components/AcpSettings.vue` around lines 548 - 550, The
copy shown by t('settings.acp.registryUninstallDescription') is inaccurate for
non-binary installs; update the UI to use distribution-neutral wording or choose
the message based on the agent's distribution type: inspect where the template
renders t('settings.acp.registryUninstallDescription') in AcpSettings.vue and
replace it with either a new generic key (e.g.,
settings.acp.registryUninstallGeneric) or a conditional that picks keys like
settings.acp.registryUninstallBinary vs settings.acp.registryUninstallNpx based
on the agent/distribution property, and ensure the logic aligns with
ConfigPresenter.uninstallAcpRegistryAgent() and
AcpLaunchSpecService.uninstallRegistryAgent() behavior so the displayed text
matches what is actually removed.
Comment on lines
+924
to
+931
| const uninstallRegistryAgent = async (agent: AcpRegistryAgent) => { | ||
| setAgentPending(agent.id, true) | ||
| try { | ||
| await configPresenter.uninstallAcpRegistryAgent(agent.id) | ||
| await loadAcpData() | ||
| toast({ title: t('settings.acp.deleteSuccess') }) | ||
| } catch (error) { | ||
| handleError(error, undefined, t('settings.acp.saveFailed')) |
Contributor
There was a problem hiding this comment.
Don’t reuse the save-failed toast for uninstall errors.
If uninstall fails, this surfaces settings.acp.saveFailed, so the toast talks about saving while the user is performing a delete. Please switch this to an uninstall/delete-specific key and add that key to the locale bundles.
Suggested change
- handleError(error, undefined, t('settings.acp.saveFailed'))
+ handleError(error, undefined, t('settings.acp.registryUninstallFailed'))Add settings.acp.registryUninstallFailed alongside the other uninstall strings in the locale files.
📝 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.
Suggested change
| const uninstallRegistryAgent = async (agent: AcpRegistryAgent) => { | |
| setAgentPending(agent.id, true) | |
| try { | |
| await configPresenter.uninstallAcpRegistryAgent(agent.id) | |
| await loadAcpData() | |
| toast({ title: t('settings.acp.deleteSuccess') }) | |
| } catch (error) { | |
| handleError(error, undefined, t('settings.acp.saveFailed')) | |
| const uninstallRegistryAgent = async (agent: AcpRegistryAgent) => { | |
| setAgentPending(agent.id, true) | |
| try { | |
| await configPresenter.uninstallAcpRegistryAgent(agent.id) | |
| await loadAcpData() | |
| toast({ title: t('settings.acp.deleteSuccess') }) | |
| } catch (error) { | |
| handleError(error, undefined, t('settings.acp.registryUninstallFailed')) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/settings/components/AcpSettings.vue` around lines 924 - 931, The
catch block in uninstallRegistryAgent is using the save-failed toast key
(t('settings.acp.saveFailed')) which is incorrect for an uninstall operation;
update the call to handleError to pass t('settings.acp.registryUninstallFailed')
instead, and add the new settings.acp.registryUninstallFailed string to the
locale bundles next to other uninstall strings so translations exist; locate the
uninstallRegistryAgent function and the catch that calls handleError(error,
undefined, t(...)) and the toast usage to make this change.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/settings/components/AcpSettings.vue (1)
951-960: Clear agent reference after uninstall to prevent stale state.The agent reference is cleared only after
uninstallRegistryAgentcompletes. If the uninstall fails,uninstallDialog.agentremains set even though the dialog is closed. Consider clearing it in afinallyblock or immediately after closing the dialog.♻️ Suggested improvement
const confirmRegistryAgentUninstallAction = async () => { const agent = uninstallDialog.agent if (!agent) { return } uninstallDialog.open = false - await uninstallRegistryAgent(agent) uninstallDialog.agent = null + await uninstallRegistryAgent(agent) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/AcpSettings.vue` around lines 951 - 960, The handler confirmRegistryAgentUninstallAction currently clears uninstallDialog.agent only after awaiting uninstallRegistryAgent, which can leave a stale agent if the uninstall fails; change it to clear the reference regardless by moving uninstallDialog.agent = null into a finally block (or set it immediately after closing the dialog before awaiting) so that uninstallDialog.agent is always reset even if uninstallRegistryAgent throws; update the function confirmRegistryAgentUninstallAction to reference uninstallDialog.agent, call uninstallRegistryAgent(agent), and ensure uninstallDialog.agent = null runs in finally (or immediately after uninstallDialog.open = false).docs/specs/acp-agent-uninstall/plan.md (1)
19-21: Optional: Vary sentence structure in test section.Static analysis flagged three successive sentences starting with "Cover". Consider minor rewording for readability.
📝 Optional rewording
## Tests -- Cover binary uninstall cleanup and safe path handling in `AcpLaunchSpecService`. -- Cover repository/state reset for registry agents. -- Cover renderer uninstall CTA wiring in `AcpSettings.vue`. +- Binary uninstall cleanup and safe path handling in `AcpLaunchSpecService`. +- Repository/state reset for registry agents. +- Renderer uninstall CTA wiring in `AcpSettings.vue`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/acp-agent-uninstall/plan.md` around lines 19 - 21, The three successive bullet points beginning with "Cover" reduce readability; rephrase one or two to vary sentence structure — e.g., change the first to "Ensure binary uninstall cleanup and safe path handling are implemented in AcpLaunchSpecService", the second to "Reset repository/state for registry agents where applicable", and the third to "Wire the renderer uninstall CTA in AcpSettings.vue". Update the lines referencing AcpLaunchSpecService and AcpSettings.vue accordingly to use the new varied phrasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/specs/acp-agent-uninstall/plan.md`:
- Around line 19-21: The three successive bullet points beginning with "Cover"
reduce readability; rephrase one or two to vary sentence structure — e.g.,
change the first to "Ensure binary uninstall cleanup and safe path handling are
implemented in AcpLaunchSpecService", the second to "Reset repository/state for
registry agents where applicable", and the third to "Wire the renderer uninstall
CTA in AcpSettings.vue". Update the lines referencing AcpLaunchSpecService and
AcpSettings.vue accordingly to use the new varied phrasing.
In `@src/renderer/settings/components/AcpSettings.vue`:
- Around line 951-960: The handler confirmRegistryAgentUninstallAction currently
clears uninstallDialog.agent only after awaiting uninstallRegistryAgent, which
can leave a stale agent if the uninstall fails; change it to clear the reference
regardless by moving uninstallDialog.agent = null into a finally block (or set
it immediately after closing the dialog before awaiting) so that
uninstallDialog.agent is always reset even if uninstallRegistryAgent throws;
update the function confirmRegistryAgentUninstallAction to reference
uninstallDialog.agent, call uninstallRegistryAgent(agent), and ensure
uninstallDialog.agent = null runs in finally (or immediately after
uninstallDialog.open = false).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bce1f7ae-d180-43be-a1b3-da24f166e7d5
📒 Files selected for processing (15)
docs/specs/acp-agent-uninstall/plan.mdsrc/renderer/settings/components/AcpSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/types/i18n.d.ts
✅ Files skipped from review due to trivial changes (11)
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/en-US/settings.json
- src/renderer/src/i18n/ru-RU/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/he-IL/settings.json
- src/renderer/src/i18n/zh-CN/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/zh-HK/settings.json
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/i18n/pt-BR/settings.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixed #1430
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation