fix: notify server when model is selected via EcaChatSelectModel#66
Conversation
When a model was selected via EcaChatSelectModel, the local state was updated but the server was never notified. This meant the server's last-config-notified was out of sync with the client's selected model. Send chat/selectedModelChanged to the server on model selection, matching the approach used by eca-emacs. The local state update is kept for immediate header refresh since the server's response to chat/selectedModelChanged does not include selectModel. Note: /model in chat does not update the header when the selected model matches the server's last-config-notified value. This is a server-side gap where chat-selected-model-changed should include select-model in its notify-fields-changed-only! call to keep last-config-notified in sync.
There was a problem hiding this comment.
Pull request overview
Updates the Neovim EcaChatSelectModel command so that model selection not only updates local state but also notifies the ECA server, keeping server-side variants and last-config-notified in sync.
Changes:
- Send
chat/selectedModelChangedto the server when a model is selected via:EcaChatSelectModel. - Keep the local state update to refresh UI immediately (since the server response doesn’t include
selectModel).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, function(choice) | ||
| if choice then | ||
| chat.mediator:update_selected_model(choice) | ||
| chat.mediator:send("chat/selectedModelChanged", { model = choice, variant = nil }, nil) |
There was a problem hiding this comment.
{ model = choice, variant = nil } will not actually send a variant field: in Lua, table keys with nil values are removed, so vim.json.encode will omit variant entirely. If the intent is to explicitly clear the variant on the server, encode JSON null (e.g. via vim.NIL) or omit the key intentionally and adjust the server expectation accordingly.
| chat.mediator:send("chat/selectedModelChanged", { model = choice, variant = nil }, nil) | |
| chat.mediator:send("chat/selectedModelChanged", { model = choice, variant = vim.NIL }, nil) |
| }, function(choice) | ||
| if choice then | ||
| chat.mediator:update_selected_model(choice) | ||
| chat.mediator:send("chat/selectedModelChanged", { model = choice, variant = nil }, nil) |
There was a problem hiding this comment.
This change introduces a new server notification when selecting a model, but the existing EcaChatSelectModel tests only assert local state changes. Please add/extend tests to assert the outgoing request (method name and params) so regressions in server sync are caught.
- Use vim.NIL instead of nil so variant is encoded as JSON null, matching eca-emacs behaviour - Add test asserting chat/selectedModelChanged is sent to the server with the correct method and model param
`EcaChatSelectModel` was updating local state only. The server was never notified of the model change, meaning variants weren't updated and `last-config-notified` fell out of sync.
Send `chat/selectedModelChanged` to the server on model selection, matching the approach used by eca-emacs. The local state update is kept for immediate header refresh, since the server's response to `chat/selectedModelChanged` does not include `selectModel`.
Note: `/model` in chat has a related server-side gap — `chat-selected-model-changed` does not include `select-model` in its `notify-fields-changed-only!` call, so `last-config-notified` falls out of sync when the picker is used first. This means a subsequent `/model` command may not update the client. That fix belongs in the eca server.