[codex] Reapply active LLM profile after save#961
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Reapply active LLM profile after save
🟡 Acceptable — The fix is correct and well-motivated. A few readability and test-coverage points worth addressing.
What the PR does
The root cause is clearly stated: conversation start reads , not the profile row directly. Saving an active profile was updating the profile store but not syncing those values back into . The fix: after saving any active profile, re-run the activation mutation (which copies LLM config into ) and route it through so React Query cache invalidation stays consistent.
This is correct and the scope is appropriately minimal.
Issues
[IMPROVEMENT OPPORTUNITIES]
** condition is harder to read than it needs to be**
The expression:
The clause is effectively unreachable in edit mode (because renaming to an existing profile name, including the active profile's name, is blocked by the guard). It is only meaningful in create mode when is /, where already produces — making the second clause a redundant repeat of the first.
This simpler form communicates the intent without the puzzle:
[TESTING GAPS]
Missing test: create-mode, is null/undefined, matches the active profile
The new test suite covers three cases (save active without rename, rename active profile, inactive profile), but not the fourth reachable path: creating a new profile (e.g. after deleting the active one) where is and equals the stale . The function's behaviour for this case is currently untested.
No runtime evidence in PR description
For a behavioural bug fix that changes when is synced (affecting every conversation started after saving), the description should include an section with a short before/after demonstration — e.g. steps to reproduce the original bug and a screen recording or console log showing the corrected sync. Tests prove the decision logic; they do not prove the end-to-end conversation-start behaviour.
What's good
- Extracting as a pure, exported function is the right call — it's directly testable and the three covered cases are meaningful.
- Switching from to is the correct approach. Going through the mutation hook keeps React Query's cache consistent rather than bypassing it with a raw service call, and this is exactly what the existing rename path should have been doing.
- The rename-test update (mock instead of ) more accurately reflects the real code path.
- added to deps is consistent with how is already treated, so no new instability is introduced.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW — The change adds one extra mutation call on the happy path when saving an active profile. Worst case is a spurious extra activation request; no data loss or breaking API changes. Error handling is already covered by the surrounding try/catch.
VERDICT:
✅ Worth merging with the test gap and evidence section addressed. The core logic is sound.
KEY INSIGHT:
The broadened condition (save active profile → always re-apply, not just on rename) is the right fix, but extract the redundant branch so the function's semantics are self-evident.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a file to your branch (or edit it if one already exists) with the
/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
Was this review helpful? React with 👍 or 👎 to give feedback.
Simplify the active-profile reapply predicate and cover the recreate-active-profile path from review feedback. Co-authored-by: openhands <openhands@all-hands.dev>
|
PR Artifacts Cleaned Up The |
|
CI is green on |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Reapply active LLM profile after save
✅ Looks good to merge — the root cause is correctly diagnosed, the fix is minimal and correctly scoped, and the follow-up commit (45f66a0) addressed both concerns raised in the previous review round.
Current state
shouldReapplyProfileAfterSave is now expressed as three clean guard clauses:
if (!activeProfileName) return false;
if (originalName) return activeProfileName === originalName; // edit mode
return activeProfileName === savedName; // create modeAll four reachable branches are covered by unit tests: save-without-rename, rename, create-with-active-name, and inactive profile. The function is exported and directly testable, which is the right design.
Mutation routing through activateProfile.mutateAsync instead of a raw ProfilesService.activateProfile call is correct — it keeps React Query cache invalidation consistent with every other activation in the component. The renamed-profile test now correctly mocks mockActivateMutateAsync rather than the service, accurately reflecting the real code path.
Minor observations (non-blocking)
-
Untested guard: The
if (!activeProfileName) return falsebranch is not directly tested. A single case —{ activeProfileName: null, originalName: 'x', savedName: 'x' } → false— would complete branch coverage. Low priority given the simplicity of the guard. -
Error UX after partial failure: If
activateProfile.mutateAsyncthrows after the profile save already succeeded, the surroundingcatchwill surface a generic save-error toast even though the profile data was persisted. This is a pre-existing pattern (out of scope here) but worth noting for a future polish pass.
Summary
The fix broadens the re-activation condition from "rename of active profile" to "any save of the active profile", routes it through the mutation hook for cache consistency, extracts the predicate as a pure exported function with good test coverage, and includes visual evidence. Ready to merge.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
📸 Snapshot Test Report✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
4 participants
Summary
agent_settings.llm.SettingsServicecache invalidation stay consistent.Root Cause
Conversation start reads
agent_settings.llm, not the profile row directly. Saving an active profile updated the profile record, but did not re-activate it unless it was renamed, so new conversations could still use stale encrypted LLM settings.Validation
npm test -- __tests__/components/settings/llm-profiles/llm-settings-local-view.test.tsxnpx vitest run __tests__/components/settings/llm-profiles/llm-settings-local-view.test.tsxnpm run typechecknpm run buildgit diff --checkVisual Evidence
The recording compares
mainagainst this PR using a controlled local agent-server API: before the fix, saving the active profile updates the profile row whileagent_settings.llmstays stale; after the fix, the active profile is reapplied and the settings used for conversation start match the saved profile.AI disclosure: this visual-evidence update was added by an AI agent (OpenHands) on behalf of the user.
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a561e92591d7d306a58b85aedf9af5b11df30ad03ePull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-61e9259Run
All tags pushed for this build
About Multi-Architecture Support
sha-61e9259) is a multi-arch manifest supporting both amd64 and arm64sha-61e9259-amd64) are also available if needed