Skip to content

fix(desktop): reseed new chats from profile defaults - #67207

Closed
deniqlab wants to merge 1 commit into
NousResearch:mainfrom
deniqlab:fix/65300-new-chat-model-default-v2
Closed

fix(desktop): reseed new chats from profile defaults#67207
deniqlab wants to merge 1 commit into
NousResearch:mainfrom
deniqlab:fix/65300-new-chat-model-default-v2

Conversation

@deniqlab

Copy link
Copy Markdown
Contributor

Refs #65300\n\n## Summary\n- Force reseed the composer model/provider when a fresh new-chat draft becomes ready.\n- Keep live-session model selection sticky for active runtimes.\n- Add focused tests for fresh-draft reseeding and model-control refresh behavior.\n\n## Verification\n- vitest run --project ui src/app/session/hooks/use-model-controls.test.tsx src/app/contrib/hooks/use-background-sync.test.tsx\n- git diff --check\n- npm run typecheck (blocked by unrelated missing module/type errors in this worktree environment)

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #65326 and #48946. This adds a boot-time fresh-draft reseed as well as the ready-draft trigger, so it has broader lifecycle coverage rather than being an identical patch.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real Desktop precedence bug. The current fresh-draft path calls a non-forced refresh at apps/desktop/src/app/contrib/hooks/use-background-sync.ts:129-136, while refreshCurrentModel() preserves persisted manual state at apps/desktop/src/app/session/hooks/use-model-controls.ts:61-63.

Problems

  • The new refreshCurrentModel(true) call can overwrite a newer picker selection. force bypasses the manual-state guards before and after getGlobalModelInfo() (apps/desktop/src/app/session/hooks/use-model-controls.ts:61-80), so a manual no-session pick made while the request is pending is replaced when it resolves. This conflicts with the Desktop stale-result invariant in apps/desktop/AGENTS.md:68-70.

Suggested changes

  • Guard the forced result with a selection/draft generation (or an equivalent initial-state snapshot) and add a deferred-request regression test proving a post-start manual pick wins.

Automated hermes-sweeper review.

useEffect(() => {
if (gatewayState === 'open' && !activeSessionId && freshDraftReady) {
void refreshCurrentModel()
void refreshCurrentModel(true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

force bypasses both manual-selection checks in use-model-controls.ts:61,67; because the global-model lookup is async, a picker selection made after this call starts can be overwritten by its later result. Please tag this refresh with a draft/selection generation (and add a deferred-result test) so newer manual intent wins.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 19, 2026
@OutThisLife

Copy link
Copy Markdown
Collaborator

Carried this forward in #67291 (your commit preserved via Co-authored-by) with @teknium1's requested race guard added.

The gap teknium1 flagged is real: refreshCurrentModel(true) bypasses both manual-state guards, so a picker selection made while the reseed's async getGlobalModelInfo() is in flight gets clobbered by the resolved default (violates AGENTS "guard against the past").

Fix in #67291: a monotonic manual-pick token in session.tsselectModel bumps it; the reseed captures it before the await and bails if it moved while pending. force still overrides a pre-existing manual pick (new chat resets to default), but a newer pick made mid-reseed wins. Added the deferred-result regression test teknium1 asked for.

Green: full vitest --project ui (1530), typecheck, lint, build. Superseding here; will close once #67291 merges. Thanks @deniqlab — solid catch on the precedence bug.

@OutThisLife

Copy link
Copy Markdown
Collaborator

Thanks @deniqlab — and good instinct on the precedence question. After weighing the UX, we're going to keep the sticky-last-pick behavior rather than reseed new chats to the profile default.

The reasoning: switching between existing chats is session-scoped and unaffected, so this only changes the new-chat (Cmd+N) boundary — and resetting a deliberate manual pick there is worse for the "I switched models on purpose, keep it" workflow than seeding from the default is worth. Sticky-last-choice is what most editors/browsers do.

The genuine bug behind #65300 / #58498 — a removed or default-derived model (owl-alpha) lingering and overriding config.yaml — is already fixed on main by #65896: default- and legacy-derived composer state re-syncs from config, while an explicit manual pick sticks. So the real breakage is handled without the reset.

Closing this (and my rebase #67291) as intended-behavior. If a manually picked model later gets removed and 404s on new chats, that's a narrow separate edge worth a "fall back to default when the pick is no longer available" guard — happy to take that if it comes up. Appreciate the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants