fix(desktop): re-seed composer when profile default model changes externally - #50198
fix(desktop): re-seed composer when profile default model changes externally#50198DavidMetcalfe wants to merge 3 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused cross-surface sync proposal. Current main still skips a re-seed once the composer has a value (apps/desktop/src/app/session/hooks/use-model-controls.ts:49-50), but it now explicitly defines that value as sticky UI state rather than the profile default (apps/desktop/src/store/session.ts:14-18). This needs a maintainer decision before salvage.
Problems
use-model-profile-sync.ts:81-85infers that an equal value was not explicitly selected.selectModelpersists any picker choice (use-model-controls.ts:77-91), so an explicit choice equal to the old default would be overwritten on the next external default change.use-model-profile-sync.ts:88-90leaves the prior provider in place when the server returns a new model with an empty provider. The existing seed path writes an empty string provider too (use-model-controls.ts:59-65).
Suggested changes
- Resolve the sticky-composer versus externally authoritative-default policy first.
- If syncing is chosen, retain explicit-vs-auto-seeded provenance and always write the returned provider; add coverage for both cases.
Automated hermes-sweeper review.
| const composerModel = $currentModel.get() | ||
| const composerProvider = $currentProvider.get() | ||
|
|
||
| const composerFollowedBaseline = |
There was a problem hiding this comment.
Equality with the prior default is not provenance: selectModel persists an explicit picker selection even when it equals the default. A user who explicitly reselects that same model/provider will satisfy this predicate and be overwritten by a later external default change. Track whether the value was auto-seeded instead.
| if (composerFollowedBaseline && serverModel) { | ||
| setCurrentModel(serverModel) | ||
|
|
||
| if (serverProvider) { |
There was a problem hiding this comment.
Do not retain the previous provider when the server returns a non-empty model with an empty provider. This produces a model/provider pair that the server did not report; mirror refreshCurrentModel and write serverProvider unconditionally when reseeding.
|
Addressed both findings from the review. Finding 1 — explicit-pick provenanceAdded Finding 2 — empty provider writeRemoved the TestsTwo new cases: "does not overwrite a picker selection equal to the previous default" (flag=true, composer==baseline) and "writes an empty provider when the model is non-empty." All 9 existing unit tests pass. Review (Flash + GPT-OSS cross-vendor)Both accept — no BLOCKERs or SHOULD-FIXs. First pass found a persistence BLOCKER (flag was in-memory only, now fixed); second pass is clean. |
…n sync Addresses maintainer review feedback on PR NousResearch#50198: 1. Adds `$currentModelExplicitlySet` atom set by `selectModel` and cleared by `refreshCurrentModel(force=true)`. The sync hook checks this flag so a picker selection equal to the old default (explicit pick, not passive follower) survives an external default change. 2. Always writes the provider returned by the server, even when empty, matching the existing seed path in `use-model-controls.ts:69`. Two new tests cover both cases. All 9 existing unit tests pass.
Flash review BLOCKER: $currentModelExplicitlySet was in-memory only, so a page reload would forget a picker selection equal to the default. Persisted via new COMPOSER_MODEL_EXPLICITLY_SET_KEY + setCurrentModelExplicitlySet() setter pattern (matching setCurrentFastMode).
desktop-controller.tsx was retired on main (369d0ee). The hook now lives alongside useBackgroundSync in wiring.tsx, where refreshCurrentModel is already called on gateway-open — same lifecycle, same gatewayState guard.
8f98db6 to
cee536a
Compare
Summary
Fixes the user-visible symptom in #50013: the Desktop composer's model label silently desynchronises from
model.defaultin the active profile's~/.hermes/profiles/<name>/config.yaml. After this lands, a Dashboard Models page edit,hermes model,hermes config set, or another Hermes client on the same profile will reach the Desktop composer within one polling interval.Root cause
apps/desktop/src/store/session.ts:238-239hydrates$currentModelfromlocalStorage["hermes.desktop.composer.model"]at module load.apps/desktop/src/app/session/hooks/use-model-controls.ts:55-57only re-seeds from/api/model/infowhen localStorage is empty — so once the user picks anything, that pick sticks forever, even ifconfig.yamlis rewritten on the same profile by any other surface. No config-file watcher exists inapps/desktop/src(grep -r 'chokidar\|fs.watch' apps/desktop/srcreturns 0 hits).Fix
New hook
apps/desktop/src/app/session/hooks/use-model-profile-sync.ts:GET /api/model/info(same cadence as the existingCRON_POLL_INTERVAL_MSpattern indesktop-controller.tsx:147).desktop-controller.tsxnext to the existing gateway-openrefreshCurrentModel()call (line 857) so the lifecycle matches the rest of the model-sync plumbing.Why the "divergent pick" check uses the server's last-seen value, not the composer's value
The test contract pinned in
use-model-controls.test.tsxis "a user pick is sacred — never clobber a pick". A naive "compare to$currentModel" check would lose that invariant: if the user picks X (different from server default Y) and then Y changes to Z externally, you'd want X to win. By comparing the server's value to the server's last-seen value, we only act when the server changed — and we only act on the composer when the composer was still showing the old server default, which by construction means the user hasn't picked.The first-run empty-composer seed remains
refreshCurrentModel's job (called on boot and gateway open). This hook owns drift after that seed.Tests
8 new tests in
use-model-profile-sync.test.tsx:render(<Harness/>)smoke)All 5 existing
use-model-controlstests still pass. The exportedsyncProfileDefaultTickhelper is what the tests drive directly — avoids fighting fake timers around the polling machinery.Full suite: 6 pre-existing failures on
origin/main(Windows path separators, Electron boot backoff), unchanged by this PR.npx tsc -p . --noEmitclean.Scope
This is the smallest piece of #50013's two-part proposal:
The polling approach here is the smallest viable patch and the same pattern as the existing cron-jobs poll. The WebSocket push variant called out in #50013 is left for a follow-up — it needs a new gateway method, a renderer subscription path, and event-bus work in
web_server.py. Polling covers the user-visible symptom today.Reproducing the bug pre-fix
Post-fix: