fix(web): stop a stale model-migration notice from reverting the picked model - #1974
Merged
Merged
Conversation
…ed model Picking a model in the composer appeared to do nothing on the first switch after client startup; a second click on the same model made it stick. The first explicit model update for a session is what pulls that session off disk. If its persisted model is no longer configured, restore repoints it to "auto" and broadcasts SessionModelAutoMigrated. That notice therefore lands *after* the composer already wrote the newly picked model optimistically, and the handler applied it unconditionally, reverting the selection. The second click found the session already in memory, so no restore, no notice, no revert. The backend itself switched correctly; only the store was rolled back. That is not cosmetic: syncSessionModelSelection pushes the store value before every send, so an unnoticed revert would overwrite the correct backend model with "auto" on the next turn. Apply the notice as a compare-and-swap instead: migrate only while the session still holds the model the backend migrated away from (or holds no selection). The CLI already guards this way in modes/chat/selection.rs; the desktop UI was the only consumer applying these events blindly. Background restores that genuinely need the migration still match and still apply. Also stop the selector from reporting a failed switch as a successful one: roll back the optimistic write and surface an error toast when the update rejects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Picking a model in the composer appears to do nothing on the first switch after client startup — the dropdown closes but the trigger keeps showing the old model. Clicking the same model a second time makes it stick, and every later switch works. Reproduces 100% of the time on the first attempt.
Root cause
Not a click-handling problem, and not a failed backend switch. It is a client-side ordering race.
The first explicit model update for a session is what pulls that session off disk. If its persisted
model_idis no longer configured (model deleted or disabled while the session sat on disk), restore repoints it to"auto"and broadcastsSessionModelAutoMigrated. That notice therefore lands after the composer already wrote the newly picked model optimistically, andhandleSessionModelAutoMigratedapplied it unconditionally — reverting the selection.The second click finds the session already in memory: no restore, no notice, no revert.
From a reproduction (webview.log and app.log aligned):
The backend switched correctly; only the frontend store was rolled back. That is not merely cosmetic:
syncSessionModelSelectionpushes the store value to the backend before every send, so a revert the user does not notice would overwrite the correct backend model with"auto"on the next turn.Fix
Apply the migration notice as a compare-and-swap: migrate only while the session still holds the model the backend migrated away from (or holds no selection yet). The event carries
previousModelIdalready, so no contract change is needed.The CLI has guarded this way since the event was introduced (
modes/chat/selection.rs— "Ignoring stale model migration"); the desktop web UI was the only consumer applying these events blindly. Background restores that genuinely need the migration still match and still apply, so the "your model disappeared, falling back to Auto" behavior is preserved.Also fixes a second, independent defect found in the same handler:
handleSelectModel'scatchonly logged, so a genuinely failed switch still rendered as success. It now rolls back the optimistic write and surfaces an error toast.Changed
FlowChatStore.applySessionModelAutoMigration()— CAS-guarded application of the notice.EventHandlerModule.handleSessionModelAutoMigrated— uses it; logs ignored notices at debug.ModelSelector.handleSelectModel— rollback +notificationService.erroron failure.modelSelector.switchFailedadded toen-US/zh-CN/zh-TW.Considered and rejected
Suppressing the notice backend-side when the restore is driven by an explicit model update. The desktop path restores through
session_application.ensure_session_loaded()beforeupdate_session_model_id()ever runs, so covering it would mean threading a suppression flag from the Tauri command throughsession_application→product_runtime→Coordinator→SessionManager— pushing a presentation concern down four layers of public API. The event is not untruthful ("at restore time the persisted model was indeed stale"); reconciling it against newer local state is the consumer's job, which is what the CLI already does.Testing
Fully tested at the automated level; the runtime repro was diagnosed from logs and has not been manually re-verified in a rebuilt client.
pnpm run type-check:web— cleanpnpm run lint:web— 0 errors (2 pre-existing warnings in untouched files)vitest run src/flow_chat/— 1333 passed / 166 filespnpm run i18n:contract:test(37 passed) andpnpm run i18n:audit(0 warnings)cargo check -p bitfun-core— clean (no Rust changes in this PR)New unit tests in
FlowChatStore.test.tscover: notice matches stored model (applies), session has no stored model (applies), notice is stale (ignored), unknown session (ignored).Manual check for a reviewer: fully quit the client, restart, open an older session whose bound model has since been removed, and switch models once — it should take on the first click.
Notes
AI-assisted (Claude Code). No UI layout changes, so no screenshots.