Conversation
f3f6991 to
c4e99e9
Compare
Competing fix for the same Desktop new-chat model-inheritance bug as #55635 (filed minutes later). Same goal, different mechanism: this PR adds a |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Fixes desktop model selection for plain new chats to use profile default instead of stale composer overrides. Adds draftModelOverridePending state.
✅ Looks Good
- Clean implementation: draftModelOverridePending flag tracks deliberate user picks
- Good test coverage with updated tests for new behavior
- Well-scoped fix for desktop model selection
- Follows existing pattern for model controls
Reviewed by Hermes Agent
c4e99e9 to
65c10af
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused tests and narrow Desktop implementation. The proposed behavior conflicts with an explicit current Desktop contract.
Problems
apps/desktop/src/app/session/hooks/use-session-actions/index.ts:172-183gates the model override, while currentmaindeliberately sends the sticky composer model on each newsession.create(apps/desktop/src/app/session/hooks/use-session-actions/index.ts:208-225).- Commit
cb6b4127e795e55bdd7ae4fe35a0ff3cd9f53736introduced that behavior specifically so picker choices follow acrossCmd/Ctrl+Nand restarts. The user guide states the same contract atwebsite/docs/user-guide/desktop.md:61.
Suggested changes
- Please obtain a maintainer product-direction decision before salvaging this behavior change. If that direction changes, update the documented sticky-picker contract and its Desktop/backend tests together.
Automated hermes-sweeper review.
| // default (that lives in Settings → Model). | ||
| const uiModel = $currentModel.get().trim() | ||
| const uiProvider = $currentProvider.get().trim() | ||
| const useDraftModelOverride = $draftModelOverridePending.get() |
There was a problem hiding this comment.
This gate reverses the current sticky-composer contract: current main deliberately includes the displayed picker model on every new session.create so selections follow across Cmd/Ctrl+N. That behavior was introduced in cb6b4127e795 and is documented at website/docs/user-guide/desktop.md:61; please obtain a maintainer decision before changing it.
|
Closing because current |
What does this PR do?
Fixes Desktop/TUI new-chat model inheritance so a plain new conversation uses the active profile's
model.defaultinstead of accidentally carrying over the previous session's runtime model.The desktop footer can mirror the active session's model. Before this change,
session.createalways sent that mirrored model as a per-session override, so a brand-new chat could ignore the selected profile's configured default. This change only sends a model override when the user explicitly picked a model while no runtime session was active; that draft override is consumed once.Related Issue
Fixes #54906
Type of Change
Changes Made
apps/desktop/src/store/session.ts$draftModelOverridePendingflag for explicit no-session model picks.apps/desktop/src/app/session/hooks/use-model-controls.tsapps/desktop/src/app/session/hooks/use-session-actions/index.tsmodel/provideronsession.createonly when a pending draft model override exists.apps/desktop/src/app/session/hooks/use-model-controls.test.tsxapps/desktop/src/app/session/hooks/use-session-actions.test.tsxHow to Test
cd apps/desktop npm run test:ui -- src/app/session/hooks/use-model-controls.test.tsx src/app/session/hooks/use-session-actions.test.tsxcd apps/desktop npx eslint src/store/session.ts src/app/session/hooks/use-model-controls.ts src/app/session/hooks/use-session-actions/index.ts src/app/session/hooks/use-model-controls.test.tsx src/app/session/hooks/use-session-actions.test.tsxcd apps/desktop npm run typecheckChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
hermes --toolsets skills -q "Use the X skill to do Y"Screenshots / Logs