fix(ui): enable Save when editing agents without existing config entries#39306
fix(ui): enable Save when editing agents without existing config entries#39306SergioChan wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a bug where the Save button stayed disabled when editing agents that had no explicit Key points:
Confidence Score: 3/5
Last reviewed commit: 6d7007b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onModelChange: (agentId, modelId) => { | ||
| if (!configValue) { | ||
| return; | ||
| } | ||
| const list = (configValue as { agents?: { list?: unknown[] } }).agents?.list; | ||
| if (!Array.isArray(list)) { | ||
| return; | ||
| } | ||
| const index = list.findIndex( | ||
| (entry) => | ||
| entry && | ||
| typeof entry === "object" && | ||
| "id" in entry && | ||
| (entry as { id?: string }).id === agentId, | ||
| ); | ||
| if (index < 0) { | ||
| const index = ensureAgentConfigIndex(agentId); | ||
| if (index == null) { | ||
| return; | ||
| } | ||
| const basePath = ["agents", "list", index, "model"]; | ||
| if (!modelId) { | ||
| removeConfigFormValue(state, basePath); | ||
| return; |
There was a problem hiding this comment.
Bare entry created when clearing model for unconfigured agent
ensureAgentConfigIndex(agentId) is called unconditionally at the top of onModelChange, before the early-return guard for a falsy modelId. If a user triggers a "clear model" action for an agent that has no existing agents.list entry, the function will:
- Create a bare
{ id: agentId }entry in the config viaupdateConfigFormValue. - Call
removeConfigFormValueon the newly created entry'smodelpath — which is a no-op since nomodelkey exists.
The net result is an unintended { id: agentId } entry persisted in state.configForm, configFormDirty is set to true, and the Save button becomes enabled even though the user made no meaningful change. The same pattern affects onModelFallbacksChange (when normalized.length === 0 and there is no primary) and onToolsProfileChange (when profile is falsy and clearAllow is false).
Move ensureAgentConfigIndex to after the early-return checks in these handlers, so it is only invoked when an actual value is being written:
onModelChange: (agentId, modelId) => {
const basePath = ["agents", "list", 0 /* placeholder */, "model"];
if (!modelId) {
// Only try to remove if the entry already exists
const existingIndex = /* find without creating */ ...;
if (existingIndex >= 0) {
removeConfigFormValue(state, ["agents", "list", existingIndex, "model"]);
}
return;
}
const index = ensureAgentConfigIndex(agentId);
if (index == null) { return; }
// ... rest of write logic
},
Alternatively, extract the read-only lookup into a separate helper (e.g. findAgentConfigIndex) that does not create entries, and use it for the clear/remove paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-render.ts
Line: 776-784
Comment:
**Bare entry created when clearing model for unconfigured agent**
`ensureAgentConfigIndex(agentId)` is called unconditionally at the top of `onModelChange`, **before** the early-return guard for a falsy `modelId`. If a user triggers a "clear model" action for an agent that has no existing `agents.list` entry, the function will:
1. Create a bare `{ id: agentId }` entry in the config via `updateConfigFormValue`.
2. Call `removeConfigFormValue` on the newly created entry's `model` path — which is a no-op since no `model` key exists.
The net result is an unintended `{ id: agentId }` entry persisted in `state.configForm`, `configFormDirty` is set to `true`, and the **Save button becomes enabled even though the user made no meaningful change**. The same pattern affects `onModelFallbacksChange` (when `normalized.length === 0` and there is no primary) and `onToolsProfileChange` (when `profile` is falsy and `clearAllow` is false).
Move `ensureAgentConfigIndex` to after the early-return checks in these handlers, so it is only invoked when an actual value is being written:
```
onModelChange: (agentId, modelId) => {
const basePath = ["agents", "list", 0 /* placeholder */, "model"];
if (!modelId) {
// Only try to remove if the entry already exists
const existingIndex = /* find without creating */ ...;
if (existingIndex >= 0) {
removeConfigFormValue(state, ["agents", "list", existingIndex, "model"]);
}
return;
}
const index = ensureAgentConfigIndex(agentId);
if (index == null) { return; }
// ... rest of write logic
},
```
Alternatively, extract the read-only lookup into a separate helper (e.g. `findAgentConfigIndex`) that does not create entries, and use it for the clear/remove paths.
How can I resolve this? If you propose a fix, please make it concise.
ui/src/ui/app-render.ts
Outdated
| const ensureAgentConfigIndex = (agentId: string): number | null => { | ||
| const current = | ||
| state.configForm ?? (state.configSnapshot?.config as Record<string, unknown> | null) ?? {}; | ||
| const agents = | ||
| current && typeof current === "object" && !Array.isArray(current) | ||
| ? ((current as { agents?: unknown }).agents as { list?: unknown[] } | undefined) | ||
| : undefined; | ||
| const list = Array.isArray(agents?.list) ? agents.list : []; | ||
| const existingIndex = list.findIndex( | ||
| (entry) => | ||
| entry && | ||
| typeof entry === "object" && | ||
| "id" in entry && | ||
| (entry as { id?: string }).id === agentId, | ||
| ); | ||
| if (existingIndex >= 0) { | ||
| return existingIndex; | ||
| } | ||
| updateConfigFormValue(state, ["agents", "list"], [...list, { id: agentId }]); | ||
| return list.length; | ||
| }; |
There was a problem hiding this comment.
null return from ensureAgentConfigIndex is unreachable
The function always returns a non-negative integer — either existingIndex (guaranteed >= 0) or list.length (guaranteed >= 0). It never returns null. All six if (index == null) { return; } guards in the handlers are therefore dead code.
This is harmless defensiveness, but it makes the code slightly misleading: readers may wonder under what circumstances the function could return null. Consider either changing the return type to number (removing the | null) to make the contract explicit, or documenting why the null branch is kept as a future-proof safety valve.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-render.ts
Line: 170-190
Comment:
**`null` return from `ensureAgentConfigIndex` is unreachable**
The function always returns a non-negative integer — either `existingIndex` (guaranteed `>= 0`) or `list.length` (guaranteed `>= 0`). It never returns `null`. All six `if (index == null) { return; }` guards in the handlers are therefore dead code.
This is harmless defensiveness, but it makes the code slightly misleading: readers may wonder under what circumstances the function could return `null`. Consider either changing the return type to `number` (removing the `| null`) to make the contract explicit, or documenting why the null branch is kept as a future-proof safety valve.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d7007bd2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (existingIndex >= 0) { | ||
| return existingIndex; | ||
| } | ||
| updateConfigFormValue(state, ["agents", "list"], [...list, { id: agentId }]); |
There was a problem hiding this comment.
When a config has no agents.list, this write creates agents.list = [{ id: agentId }] for whichever agent is edited first. That silently changes the gateway default agent on save, because resolveDefaultAgentId picks the first list entry when no entry has default: true (src/agents/agent-scope.ts, resolveDefaultAgentId). In practice, editing a non-default agent from the Agents UI can reroute later session/command flows that rely on implicit default-agent resolution.
Useful? React with 👍 / 👎.
|
Thanks for the careful review — great catch on the clear/no-op paths. I confirmed the concern: calling ensureAgentConfigIndex() before early-return guards can create a spurious { id } entry for unconfigured agents when clearing values. I’m preparing a small follow-up commit that keeps guard checks before index creation on those handlers so we only materialize config entries when an actual persisted change exists. |
|
Pushed a follow-up commit to address this ( What changed:
Validation run locally:
Thanks again for spotting the edge case. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99afc7602a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!profile && !clearAllow) { | ||
| return; |
There was a problem hiding this comment.
The new early return if (!profile && !clearAllow) return; makes the "Inherit" action a no-op, because that button calls onToolsProfileChange(agentId, null, false); when an agent already has tools.profile set, users can no longer clear that override and fall back to global/default tool policy. Before this change, existing entries would reach removeConfigFormValue(..."profile"), so this is a functional regression that blocks a normal config workflow.
Useful? React with 👍 / 👎.
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
agents.list[]entry in config, so Save stayed disabled.ensureAgentConfigIndex()inui/src/ui/app-render.tsand routed agent edit handlers through it, auto-creating{ id: <agentId> }inagents.listbefore writing overrides.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
agents.list.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
agents.listentrySteps
agents.listitem.Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm -C /home/chen/.openclaw/workspace/tmp-openclaw exec vitest run ui/src/ui/controllers/agents.test.tsafter patch.Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
6d7007bd2.ui/src/ui/app-render.ts.agents.listentries (not expected due id-based lookup).Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.id+ one override key.