perf(agent): refresh acp agents incrementally - #1579
Merged
zerob13 merged 1 commit intoApr 30, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR enhances agent change notifications by introducing optional agent ID tracking to enable incremental updates. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Agent Install/Repair Flow
participant Presenter as Config Presenter
participant Bridge as Event Bridge
participant ConfigClient
participant AgentStore as Agent Store
Agent Install/Repair Flow->>Presenter: notifyAcpAgentsChanged(agentIds)
Presenter->>Presenter: Emit CONFIG_EVENTS.AGENTS_CHANGED<br/>with { agentIds }
Presenter->>Bridge: EVENT_EMITTER (CONFIG_EVENTS.AGENTS_CHANGED)
Bridge->>ConfigClient: publishAgentsChanged(payload.agentIds)
ConfigClient->>ConfigClient: Emit onAgentsChanged event<br/>with { agentIds, ... }
ConfigClient->>AgentStore: listener callback<br/>with agentIds
alt Scoped Change (agentIds provided)
AgentStore->>ConfigClient: listAgents({ agentType: 'acp', ids: agentIds })
ConfigClient-->>AgentStore: Filtered agent list
AgentStore->>AgentStore: Merge/update agents<br/>for specific IDs
else Full Change (no agentIds)
AgentStore->>AgentStore: Full agent refresh
end
AgentStore->>AgentStore: Update store.agents<br/>Sync selectedAgentId
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple interrelated components with mixed complexity: straightforward event payload additions, a new filtering route, and moderate logic additions in the agent store for incremental synchronization. The heterogeneity of edits across the event system, routing layer, and client API demands separate reasoning per file, though no single file contains dense logic. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/renderer/stores/agentStore.test.ts (1)
1-1: ⚡ Quick winPlace this suite in a path that mirrors
stores/ui/agent.tsGiven the tested source path includes
stores/ui, consider moving this suite to a mirrored location undertest/renderer/**(for example, undertest/renderer/stores/ui/...) to stay consistent with repository test-layout rules.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/stores/agentStore.test.ts` at line 1, The test suite currently in test/renderer/stores/agentStore.test.ts should be relocated so its path mirrors the source module stores/ui/agent.ts; move the file into a corresponding test/renderer/stores/ui/ directory (e.g., test/renderer/stores/ui/agentStore.test.ts) and update any imports or test-runner references accordingly so the suite aligns with the repository convention that Vitest tests mirror source layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/stores/ui/agent.ts`:
- Around line 138-139: The error assignment uses a hard-coded English string
(error.value = `Failed to refresh ${agentType} agents: ${e}`) — replace it with
a vue-i18n key lookup from the i18n module (e.g., use the t() function from
useI18n or the project i18n helper) and pass agentType and error text as
interpolation params; also update the similar hard-coded string at the other
occurrence (lines around the second error assignment referenced) to use the same
i18n key(s) located in src/renderer/src/i18n so all user-facing messages come
from i18n instead of inline text.
In `@src/shared/contracts/routes/config.routes.ts`:
- Around line 424-426: The schema allows ids:
z.array(z.string().min(1)).optional() which accepts an empty array and causes
the Set filter to return zero agents; change the schema for the ids field
(symbol "ids") to forbid empty arrays (e.g., use Zod's nonempty/ .min(1) on the
array type) or coerce an empty array to undefined so that an absent/unscoped
request is represented as undefined instead of []—update the schema where "ids"
is defined to either z.array(z.string().min(1)).nonempty().optional() or add a
transform/refine that converts [] -> undefined to prevent accidental “return
none” behavior.
In `@test/renderer/stores/agentStore.test.ts`:
- Around line 100-103: The assertion for configClient.listAgents should also
assert the call count to ensure the scoped refresh is the only call; update the
tests around the expect(configClient.listAgents).toHaveBeenCalledWith(...) at
the shown location (and the similar one around lines 131-134) to either assert
toHaveBeenCalledTimes(1) before/after the toHaveBeenCalledWith or replace with
toHaveBeenNthCalledWith(1, { agentType: 'acp', ids: ['acp-1'] }) so the test
fails if any extra unscoped/full listAgents calls occur.
---
Nitpick comments:
In `@test/renderer/stores/agentStore.test.ts`:
- Line 1: The test suite currently in test/renderer/stores/agentStore.test.ts
should be relocated so its path mirrors the source module stores/ui/agent.ts;
move the file into a corresponding test/renderer/stores/ui/ directory (e.g.,
test/renderer/stores/ui/agentStore.test.ts) and update any imports or
test-runner references accordingly so the suite aligns with the repository
convention that Vitest tests mirror source layout.
🪄 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: 0a9a738a-9204-4bf0-b658-42f142b5e6ee
📒 Files selected for processing (9)
src/main/presenter/configPresenter/index.tssrc/main/routes/config/configRouteHandler.tssrc/main/routes/legacyTypedEventBridge.tssrc/renderer/api/ConfigClient.tssrc/renderer/src/stores/ui/agent.tssrc/shared/contracts/events/config.events.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/config.routes.tstest/renderer/stores/agentStore.test.ts
Comment on lines
+138
to
+139
| error.value = `Failed to refresh ${agentType} agents: ${e}` | ||
| } |
Contributor
There was a problem hiding this comment.
Replace hard-coded refresh error strings with i18n keys
These new error messages are user-facing and should come from vue-i18n keys instead of inline English text.
As per coding guidelines src/renderer/src/**/*.{vue,ts,tsx}: All user-facing strings must use vue-i18n keys located in src/renderer/src/i18n.
Also applies to: 158-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/stores/ui/agent.ts` around lines 138 - 139, The error
assignment uses a hard-coded English string (error.value = `Failed to refresh
${agentType} agents: ${e}`) — replace it with a vue-i18n key lookup from the
i18n module (e.g., use the t() function from useI18n or the project i18n helper)
and pass agentType and error text as interpolation params; also update the
similar hard-coded string at the other occurrence (lines around the second error
assignment referenced) to use the same i18n key(s) located in
src/renderer/src/i18n so all user-facing messages come from i18n instead of
inline text.
Comment on lines
+424
to
+426
| ids: z.array(z.string().min(1)).optional() | ||
| }) | ||
| .default({}), |
Contributor
There was a problem hiding this comment.
Guard against empty ids to avoid accidental “return none” behavior
ids: [] currently parses, and the handler-side Set filter will return zero agents. If empty means “unscoped”, this can produce accidental data drops in callers.
Suggested schema hardening
- ids: z.array(z.string().min(1)).optional()
+ ids: z.array(z.string().min(1)).min(1).optional()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/contracts/routes/config.routes.ts` around lines 424 - 426, The
schema allows ids: z.array(z.string().min(1)).optional() which accepts an empty
array and causes the Set filter to return zero agents; change the schema for the
ids field (symbol "ids") to forbid empty arrays (e.g., use Zod's nonempty/
.min(1) on the array type) or coerce an empty array to undefined so that an
absent/unscoped request is represented as undefined instead of []—update the
schema where "ids" is defined to either
z.array(z.string().min(1)).nonempty().optional() or add a transform/refine that
converts [] -> undefined to prevent accidental “return none” behavior.
Comment on lines
+100
to
+103
| expect(configClient.listAgents).toHaveBeenCalledWith({ | ||
| agentType: 'acp', | ||
| ids: ['acp-1'] | ||
| }) |
Contributor
There was a problem hiding this comment.
Strengthen scoped-refresh assertions to avoid false positives
These checks should also assert call count. toHaveBeenCalledWith(...) still passes if an extra unscoped/full listAgents call happens, which weakens the “incremental only” guarantee.
Proposed test hardening
- expect(configClient.listAgents).toHaveBeenCalledWith({
+ expect(configClient.listAgents).toHaveBeenCalledTimes(1)
+ expect(configClient.listAgents).toHaveBeenCalledWith({
agentType: 'acp',
ids: ['acp-1']
})
...
- expect(configClient.listAgents).toHaveBeenCalledWith({
+ expect(configClient.listAgents).toHaveBeenCalledTimes(1)
+ expect(configClient.listAgents).toHaveBeenCalledWith({
agentType: 'acp',
ids: ['acp-2']
})Also applies to: 131-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/renderer/stores/agentStore.test.ts` around lines 100 - 103, The
assertion for configClient.listAgents should also assert the call count to
ensure the scoped refresh is the only call; update the tests around the
expect(configClient.listAgents).toHaveBeenCalledWith(...) at the shown location
(and the similar one around lines 131-134) to either assert
toHaveBeenCalledTimes(1) before/after the toHaveBeenCalledWith or replace with
toHaveBeenNthCalledWith(1, { agentType: 'acp', ids: ['acp-1'] }) so the test
fails if any extra unscoped/full listAgents calls occur.
zerob13
added a commit
that referenced
this pull request
Apr 30, 2026
* refactor(model): derive selectable model source * refactor(model): unify selection resolver (#1576) * refactor(chat): split acp status bar state (#1578) * refactor(model): unify selection resolver * refactor(chat): extract acp status bar state * perf(agent): refresh acp agents incrementally (#1579) * fix(provider): gc removed model state (#1577) * fix: address pr review feedback --------- Co-authored-by: zerob13 <zerob13@gmail.com>
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
2 participants
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.
Summary
config.listAgentsroute so the renderer can refresh only the ACP agents that actually changedagentIdsthrough the ACP agents changed event for incremental renderer updatesTesting
Summary by CodeRabbit
New Features
Improvements