fix(desktop): make model and thinking-level changes instant without success toasts - #3749
fix(desktop): make model and thinking-level changes instant without success toasts#3749Sun-GLiang wants to merge 18 commits into
Conversation
Generated-by: Codex
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2] Stop cannot cancel the pre-send model-setup flight
app-shell-chat-actions.ts:493-508 arms the turn before waiting for settleSessionModelSettings. Stop at that point only calls sessions.stop, which returns immediately when no Host turn exists, so the original send still proceeds after settings settle despite user cancellation.
[P2] Active-Turn follow-up bypasses the settlement barrier
app-shell.tsx:1940-1952 enqueues follow-ups directly without going through the settling actions.send path. A model mutation failing to settle still leaves the follow-up queued under the old model, violating the "latest mutation silently settles before immediate send" contract.
Checks on 3686d2aaf are test: success.
简体中文
存在预发送取消与跟随路径两项阻断。Generated-by: Codex
|
Addressed both blocking paths in ee34dd0.
Added regression coverage for local cancellation, preserved Host stop behavior, successful follow-up settlement, and failed settlement. Desktop tests pass 1496/1496; typecheck, Biome, and the production build also pass. |
Astro-Han
left a comment
There was a problem hiding this comment.
Update on ee34dd0026:
I found blocking issues that persist/drift in this head.
[P1] Follow-up can still start a new root after Stop
A follow-up submitted on an active turn waits for settings, then submitFollowUpAfterModelSettings has no cancellation identity. If the user presses Stop during the wait, preSendSettingsRef is cleared but the follow-up waiter is not cancelled — it still enqueues and the idle next_turn root starts.
[P2] Root/follow-up dispatches without re-checking surface owner
After the settings wait, the dispatch does not re-verify surface owner. Switching from surface A to B during the wait still sends with A's intent.
[P2] Thinking intent rolls back another client's new model
A stale thinking intent holding an old (model, thinking) tuple can overwrite a newer model B committed by another client.
Checks on ee34dd0026 are test: failure (slash-menu unrelated flake but still not green) — not green.
简体中文
存在跟随提交与所有者/模型回滚三项阻断。Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
|
Addressed the latest settlement/readiness findings in b1ae2a7.
Added regression coverage for all three races. Desktop tests pass 1516/1516; production build, typecheck, Biome, and diff checks also pass. |
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2] use-session-model-settings-intent.ts:118 — model equality used as ordering token
A thinking write M/high commits, another client then commits M/low and the new catalog snapshot arrives before the older IPC response. Because model is unchanged, the stale M/high is accepted and stamped at the current revision, leaving the overlay wrong if the best-effort refresh fails. Carry a causal token (host revision) through mutation result and catalog.
[P2] chat-model-switcher.tsx:231 — accessibility loses current/success state after toast removal
Fixed aria-label replaces visible label, current check is aria-hidden, no selected state, and status only announces warnings. After removing the toast, AT cannot read the current model or hear success.
[P3] app-shell.tsx:1773 — cancellation epoch tombstones accumulate
Every Stop inserts an epoch, but clearSessionRendererState never cleans it, leaving renderer-lifetime tombstones.
Checks on b1ae2a754f are test: success.
简体中文
存在两项正确性与一项可及性阻断,另有一项生命周期 P3。|
Thanks for the thorough work here — the test coverage in particular is careful. But I'd like to redirect the approach before this goes further. The seam already exists. Two of the three reported problems need no new machinery. Stacked toasts and the wrong Part of this belongs to #3347, which we have not decided to do. Settle-before-send, Stop cancelling an in-flight settings write, and gating follow-ups on settlement all address "the send may use a configuration the Host has not committed yet". That is #3347's problem, and it puts renderer-side ordering in front of Host turn admission. Please drop those here. Suggested shape — this should come out as a net deletion:
Two smaller things worth closing while you are here: 简体中文感谢你细致的工作,测试尤其扎实。但在继续之前我想调整一下方向。 这个 seam 已经存在。 三个问题里有两个不需要新机制。 toast 堆叠和 有一部分属于 #3347,而我们还没有决定要做它。 发送前 settle、Stop 取消在途的设置写入、follow-up 进队列前先 settle,解决的都是「发送时用的配置可能还没提交到 Host」——那是 #3347 的问题,而且它让渲染层排在了 Host 的 Turn 准入之前。请从本 PR 移除。 建议的形状,结果应该是净删除:
另有两处顺手可以收掉: |
|
Following up with the full reasoning behind that suggestion — it is a fairly large redirect, so it seems only fair to show the whole picture rather than just the conclusion. Two mechanisms for one factDesktop currently has two different answers to "a session setting write is in flight", and which one you get depends on which setting you touched.
Every one of these five settings goes through the same shape on the Host side — the same flowchart TB
subgraph A["useSessionSettingIntent — 144 lines, generic"]
A1["Plan mode"] --> AI["desired / committed<br/>committedAtCatalogRevision / inFlight"]
A2["Orchestration mode"] --> AI
AI --> AO["overlay: shows immediately, latest wins"]
AI --> AW["write loop: re-reads desired<br/>rolls back to committed on failure"]
AW --> AR["refreshCatalog, retire overlay by catalogRevision"]
end
subgraph B["pending registry + toast"]
B1["Model"] --> BR["keysRef: Set of sessionId"]
B2["Thinking level"] --> BR
B3["Permission mode"] --> BR2["keysRef: Set of key"]
BR --> BS["pendingSessionModelBySession"]
BR2 --> BS2["pendingPermissionModeBySession"]
BS --> BD["disabled + spinner"]
BS2 --> BD
BR --> BX["repeat click dropped"]
BW["write succeeds"] --> BT["toast.success"] --> BF["refreshSessions"]
end
What the difference looks like in timesequenceDiagram
autonumber
participant U as User
participant C as Control
participant S as settings-actions
participant R as pending registry
participant H as Runtime Host
Note over U,H: today, model / thinking / permission
U->>C: pick B
C->>S: setSessionModel(B)
S->>R: keysRef.add(sessionId)
S->>S: setPendingBySession(true)
Note over C: disabled + spinner, still shows A
U->>C: quickly pick C
C->>S: setSessionModel(C)
S-->>C: has(sessionId), returns early — C is lost
S->>H: sessions:setModel(B)
H-->>S: SessionSummary(B)
S->>S: latestAssistantModelId(messages) for the "from" label
S->>U: toast "A to B"
S->>H: refreshSessions()
S->>R: delete + setPendingBySession(false)
sequenceDiagram
autonumber
participant U as User
participant C as Control
participant I as useSessionSettingIntent
participant H as Runtime Host
Note over U,H: today, plan / orchestration
U->>C: pick B
C->>I: request(sessionId, B)
I->>C: overlay = B, shown immediately, not disabled
I->>H: write(B)
U->>C: quickly pick C
C->>I: request(sessionId, C)
I->>I: desired = C, no second write started
I->>C: overlay = C
H-->>I: committed(B)
I->>H: write(C)
H-->>I: committed(C)
I->>H: refreshCatalog()
I->>I: newer catalogRevision retires the overlay
The second diagram is the behaviour your issue asks for. It is already in the tree. What disappears if the three settings move over
Nothing new is added: For permission mode, the bypass confirmation moves to the call site — confirm first, then What we give upThe control is no longer disabled while a write is in flight. That is the point of your issue, so I think it is a gain — but it is a real behaviour change and worth stating plainly. Two smaller things in the same area
"Why can't I change this right now" is derived in three places — Neither is a blocker. They are cheap to close while the area is open, and both go away on their own once the three settings share one path. Happy to talk through any of this if you see it differently — you have been in this code more recently than I have. 简体中文补上建议背后的完整理由。这是一次幅度不小的方向调整,只给结论不太公平。 一个事实,两套机制Desktop 现在对「会话设置写入在途」有两个不同的答案,取决于你动的是哪个设置。
这五个设置在 Host 侧走的是完全相同的形状——同一个 上面两张时序图分别对应这两条路径。第二张就是你 issue 里想要的行为,它已经在仓库里了。 三个设置迁过去之后会消失的东西
不需要新增任何东西: 权限模式的 bypass 确认弹窗移到调用点——先确认,再 让出的东西写入在途时控件不再被禁用。这正是你 issue 的目的,所以我认为是收益,但它确实是一次行为变化,应该明说。 同一片区域的两个小问题
「为什么现在不能改」在三个地方各推导了一遍—— 两个都不阻塞。趁这块打开时收掉很便宜,而且三个设置合并到同一条路径后它们会自行消失。 如果你有不同看法,随时聊——这块代码你比我更熟。 |
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
071ec88 to
3fdf585
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Update on 3fdf5857a0:
[P2] Model intent fire-and-forget allows send on stale model
app-shell.tsx:3030-3032 fires modelIntent.request() without waiting, and app-shell-chat-actions.ts:349-350 send() no longer waits for settleSessionModelSettings. The Host setModel IPC can still be pending when sessions.send runs, so the turn can start with the old model A even though the UI shows B.
Fix: restore a per-session settlement barrier between model change and send, or carry expected model/version into send and retry on mismatch.
Checks on 3fdf5857a0 pending — code is NO-GO independent of CI.
简体中文
存在模型意图与发送竞态导致的错模型启动。Generated-by: Maka
23d3fb7 to
597cfe7
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
1b31027 to
064af22
Compare
Summary
useSessionSettingIntentcontrollers. Controls show the latest requested value immediately, rapid changes converge on the latest intent, terminal failures roll back to the last committed value, and newer catalog revisions retire optimistic overlays.Fixes #3745
Verification
Visual verification
Before
Rapid thinking-level changes created stacked success toasts with obsolete intermediate values.
After — instant, no spinner
Model and thinking-level controls reflect each selection immediately without a spinner, disabled flicker, or success toast.
Static final state (
GPT-5.6 Sol / Low):AI use
Tool(s) and scope: OpenAI Codex implemented the session-setting intent integration, transient-feedback cleanup, regression and accessibility coverage, and verification. A
Generated-by: OpenAI Codextrailer is present on each materially affected commit.Checklist
Does this PR entail a user-facing behavior change?