fix(web): remember the thinking level per model#1838
Conversation
🦋 Changeset detectedLatest commit: 1837681 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ab7898416
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (id === undefined || id === prevId) return; | ||
| const model = modelById(id); | ||
| if (model === undefined) return; | ||
| rawState.thinking = thinkingLevelForModel(model); |
There was a problem hiding this comment.
When a user queues a follow-up in a session using model A and opens a session using model B before A finishes, this watcher replaces the sole rawState.thinking value with B's preference. The background completion path then drains A's queue through finishPromptLocal(sid) → submitPromptInternal(sid), but useWorkspaceState.ts:1488-1494 combines A's model with that global value, so the original cross-model leak can recur and may submit an undeclared effort. Resolve thinking from the submitted session's model (or retain it with the queued prompt) rather than relying on the active-session value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Avoid persisting derived defaults during model switches
When switching to a model with no saved preference, nextThinking is the catalog default, but this call routes it through applyThinkingLevel, which unconditionally writes it to the per-model storage map. Consequently, if the catalog later changes that model's default while the old value remains supported, the old derived value is treated as an explicit user choice and overrides the new default; the same write also occurs before a failed switch is rolled back. Update the in-memory level without persisting here, and reserve storage writes for explicit thinking-picker actions.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b7c1e86c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // which tracks whatever session the user is looking at now: a queue | ||
| // drain for a background session would otherwise submit the level of | ||
| // the session the user switched to since enqueueing. | ||
| thinking: modelProvider.thinkingLevelForModelId(model) ?? rawState.thinking, |
There was a problem hiding this comment.
When openSideChatOn(parent, initialPrompt) is awaiting startBtw, the user can switch sessions and the new active-model watcher changes rawState.thinking; the later sendSideChatPromptOn call still combines the captured parent's model with that active session's level at useSideChat.ts:206-215. This defeats the explicit-parent race protection and can send an unsupported effort that the provider silently drops. Use thinkingLevelForModelId(model) for the BTW submission as done here for the other submitPrompt paths.
Useful? React with 👍 / 👎.
| if (id === undefined || id === prevId) return; | ||
| const model = modelById(id); | ||
| if (model === undefined) return; | ||
| rawState.thinking = thinkingLevelForModel(model); |
There was a problem hiding this comment.
When an existing session's model has a stored level that differs from that session's agent profile, this watcher updates only the picker state. activateSkill at useModelProviderState.ts:351-382 sends only the skill name and arguments, so the daemon runs with the session's stale profile effort even though the UI displays the restored per-model level. Ensure skill activation first applies the resolved level to the target session profile, as the new-session skill path already does.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b76ffb9a1a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function thinkingLevelForModel(model: AppModel): ThinkingLevel { | ||
| const stored = loadThinkingForModel(model.id); | ||
| if (stored !== undefined && levelDeclaredBy(model, stored)) return stored; | ||
| return defaultThinkingLevelFor(model); |
There was a problem hiding this comment.
When localStorage is unavailable or setItem throws—for example because of a storage policy or quota—the safe storage helpers swallow the failure, so setThinking updates only rawState.thinking. This resolver then rereads an empty map and returns the catalog default; all prompt, steer, BTW, and skill paths now use that result, causing the UI to show the user's selected level while the daemon receives a different default. Keep the per-model choices in memory as the runtime source of truth and use localStorage only for hydration and best-effort persistence.
Useful? React with 👍 / 👎.
| function thinkingLevelForModel(model: AppModel): ThinkingLevel { | ||
| const stored = loadThinkingForModel(model.id); | ||
| if (stored !== undefined && levelDeclaredBy(model, stored)) return stored; | ||
| return defaultThinkingLevelFor(model); |
There was a problem hiding this comment.
When another tab changes the thinking level for the same model, this resolver sees the new localStorage map on every submission, but rawState.thinking remains unchanged because the facade only installs a storage listener for unread state. Consequently, one tab can continue highlighting low while silently submitting max after the other tab changes it. Either handle storage events for the thinking key and re-resolve the displayed state, or avoid using an unseen cross-tab value for runtime submission.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70f8582f85
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function loadThinkingMap(): Record<string, ThinkingLevel> { | ||
| const parsed = safeGetJson<unknown>(THINKING_STORAGE_KEY); | ||
| if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return {}; |
There was a problem hiding this comment.
On the first launch after upgrading, every existing installation has a plain-string value such as "low" under kimi-web.thinking; safeGetJson cannot parse that value, so this returns an empty map and permanently discards the user's explicit preference. The catalog default is then displayed and submitted instead, changing behavior until the user manually selects the level again. Preserve the legacy value and associate it with the active model once the catalog/model ID is available before rewriting the key in map format.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bebfb18b96
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const thinkingPicks: Record<string, ThinkingLevel> = loadThinkingMap(); | ||
| const legacyThinkingPick: ThinkingLevel | undefined = | ||
| Object.keys(thinkingPicks).length === 0 ? loadLegacyThinkingValue() : undefined; |
There was a problem hiding this comment.
For an upgraded user with a legacy raw value such as low, the first explicit pick for any model rewrites this storage key as a JSON map containing only that model. On the next reload, this condition sees a nonempty map and drops legacyThinkingPick, so every other model loses the promised legacy fallback. Persist the legacy value alongside per-model overrides, or migrate it before replacing the raw value, so a per-model entry can override it without deleting it for all other models.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9a8ba18bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // legacy fallback) over the stored map, so concurrent tabs' entries survive | ||
| // and the legacy value is rewritten in map format rather than dropped. | ||
| const map = loadThinkingMap(); | ||
| for (const [id, pick] of Object.entries(thinkingPicks)) map[id] = pick; |
There was a problem hiding this comment.
When two tabs hydrate an existing multi-model map, changing model A in one tab and then model B in the other causes the second write to copy its entire stale thinkingPicks snapshot over the freshly loaded map, reverting model A's new preference. Persist only the changed modelId entry (plus the legacy fallback when migration requires it), as the delta-based saveUnread pattern does, so edits for different models survive across tabs and reloads.
Useful? React with 👍 / 👎.
Persist kimi-web.thinking as a JSON map of model id to level instead of a single global value, and resolve the active level against the model's catalog (stored pick when still declared, else the model default) at loadModels, setModel, and on active-model changes via a watcher. Fixes the empty, unresponsive thinking picker shown for a model that does not declare a previously stored level (e.g. a max-only model with a stale global 'low').
submitPromptInternal and the steer path read the single active-session rawState.thinking, so a queue drain for a background session submitted the level of whichever session the user had switched to since enqueueing — the same cross-model leak on the submit path. Thinking now joins model and the per-session modes in being resolved from the prompt's own session model (its stored pick when declared, else the catalog default), falling back to the active value only when the model has left the catalog.
setModel routed the resolved level through applyThinkingLevel, which writes per-model storage unconditionally — a switch to a model with no saved pick stored the catalog default as if it were an explicit choice, pinning the user to it across later default changes, and the rollback path did the same write for a switch that never happened. Model switches now update the in-memory level only; storage writes stay with setThinking, the explicit picker path.
sendSideChatPromptOn combined the captured parent's model with the active-session level, so a session switch during the startBtw await sent the BTW first turn at the wrong model's effort — resolve it from the parent's own model, falling back to the active value off-catalog, same as the other submit paths. activateSkill carries no thinking either, so the daemon ran skills at the session profile effort, which can predate the per-model restore the picker now shows. Persist the resolved level to the session profile first, mirroring the new-session skill path; that path itself now resolves against the new session's model instead of the raw active value.
The resolver re-read localStorage on every submission, letting storage — not the displayed state — decide what the daemon receives: with storage unavailable (policy/quota) an explicit pick reached the UI while every submit path fell back to the catalog default, and a pick made in another tab silently changed what this tab submits mid-session. Per-model picks now live in an in-memory map hydrated from localStorage at startup; explicit picks update it first and persist best-effort (read-modify-write merge, so concurrent tabs' entries still survive). localStorage is only hydration plus persistence — another tab's pick can no longer alter this tab's runtime level.
Pre-map installs stored a single global level as a raw string; the map parser dropped it, silently resetting the user's explicit preference to the catalog default on upgrade. The legacy value is now carried as a fallback for models without their own entry — validated against each model's catalog at resolution, so effort models keep the user's pick while a max-only model still falls through to its default and can never be trapped by it.
The first explicit pick after an upgrade rewrote the raw legacy value into a map containing only that one model, so the next reload saw a nonempty map and dropped the legacy fallback for every other model. The migrated value now lives inside the map under a '*' key that no real model id can collide with: per-model entries override it, and rewrites persist it alongside them instead of deleting it.
a9a8ba1 to
0b844e7
Compare
Overlaying the whole in-memory map on write could revert a newer pick made in another tab for a model this tab still held a stale copy of. Write the changed entry alone (delta-style, like saveUnread), carrying only the migrated legacy '*' fallback along so it survives the first rewrite into map format.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 818bfb5f74
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await persistSessionProfile( | ||
| { thinking: thinkingLevelForModelId(skillModel) ?? rawState.thinking }, | ||
| sid, | ||
| ); | ||
| await getKimiWebApi().activateSkill(sid, skillName, args); |
There was a problem hiding this comment.
When the /profile request fails but the skill activation endpoint remains available, this still starts the skill with the session's stale thinking effort. The injected persistSessionProfile in useKimiWebClient.ts catches every update/refresh error and resolves its promise, so awaiting it here does not enforce the ordering claimed by the comment and execution continues to activateSkill. Have profile persistence return a success result or reject for this call, and do not activate the skill after a failed thinking update.
Useful? React with 👍 / 👎.
persistSessionProfile surfaces failures itself and resolves, so awaiting it never blocked a following activation: a failed /profile write still launched the skill at the session's stale effort. It now resolves a success flag; both activation paths (existing session and new-session draft) gate on it and skip activating when the persist fails, without reporting a second, synthetic error.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94383b31e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const persisted = await persistSessionProfile( | ||
| { thinking: thinkingLevelForModelId(skillModel) ?? rawState.thinking }, | ||
| sid, | ||
| ); |
There was a problem hiding this comment.
When a skill is launched from the empty composer, startSessionAndActivateSkill already awaits persistSessionProfile with the resolved thinking level before calling activateSkill (useWorkspaceState.ts:1207-1220). This unconditional persist repeats the profile update and status refresh; if that redundant second update transiently fails, the sentinel path cancels the skill even though the prerequisite profile was successfully applied. Let the new-session path skip this second persist or consolidate profile persistence inside a single activation call.
Useful? React with 👍 / 👎.
startSessionAndActivateSkill persisted the resolved thinking and then activateSkill persisted it again unconditionally — a redundant profile update and status refresh whose transient failure would false-veto an activation whose prerequisite profile was already applied. Thinking is now written by activateSkill alone (the single, gated writer); the draft patch carries only model, plan/swarm and permission.
|
@codex review |
oxlint --type-aware (only-throw-error) rejects throwing a Symbol; the identity-based sentinel works the same as a shared Error instance.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2175508baa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const map = loadThinkingMap(); | ||
| const legacy = thinkingPicks[LEGACY_THINKING_PICK_KEY]; | ||
| if (legacy !== undefined) map[LEGACY_THINKING_PICK_KEY] = legacy; | ||
| map[modelId] = level; | ||
| safeSetJson(THINKING_STORAGE_KEY, map); |
There was a problem hiding this comment.
When two tabs pick levels for different models at nearly the same time, both can call loadThinkingMap() before either calls safeSetJson(), after which the last writer replaces the first tab's entry. Individual localStorage operations are synchronous, but this read-modify-write sequence is not atomic, so overlaying only the changed entry does not provide the claimed concurrent-tab preservation. Store models under separate keys or add a synchronization/version-and-retry mechanism.
Useful? React with 👍 / 👎.
| const skillModel = rawState.sessions.find((s) => s.id === sid)?.model; | ||
| const persisted = await persistSessionProfile( | ||
| { thinking: thinkingLevelForModelId(skillModel) ?? rawState.thinking }, | ||
| sid, |
There was a problem hiding this comment.
When activateSkill targets an explicit session whose cached model is empty—a state the client already handles for daemon profile echoes—thinkingLevelForModelId(skillModel) returns undefined and the fallback uses rawState.thinking, which belongs to the currently active view. In the new-session skill flow, a session switch during the preceding profile request can therefore persist another model's effort onto the target session if the status refresh leaves its cached model empty. Resolve session.model || rawState.defaultModel here, as the prompt and side-chat paths do, before selecting the thinking level.
Useful? React with 👍 / 👎.
…kills session.model can be '' transiently (daemon profile echo), so activateSkill fell back to the raw active-view level; in the new-session flow a concurrent switch could persist another model's effort onto the target session. Normalize '' through the configured default_model first, same as the prompt/BTW/steer paths.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Related Issue
No linked issue — the problem is explained below.
Problem
kimi-web persisted the thinking level as a single global localStorage value (
kimi-web.thinking), shown and submitted verbatim without validation against the active model's catalog. When the active model does not declare the stored level — e.g. a max-only always-on model while alowpicked for another model is stored — the composer highlights no segment, and the single-segment control is read-only, leaving an empty thinking picker with no way to switch.What changed
useKimiWebClient.ts):kimi-web.thinkingnow holds a JSON map of model id → level. The runtime source of truth is an in-memory map hydrated from localStorage at startup; explicit picks update it first and persist best-effort with a whole-map read-modify-write merge (same pattern assaveUnread), so a pick takes effect even when storage is unavailable, concurrent tabs don't clobber each other's entries, and another tab's pick can't silently change what this tab displays or submits mid-session. A legacy plain-string value (the pre-map global pick) is migrated into the map under a'*'key — a fallback for models that declare it, which per-model entries override but never delete, even across the first rewrite of the key into map format — while a max-only model still can't be trapped by it.useModelProviderState.ts): newthinkingLevelForModel()— use the model's stored pick when the model still declares it, else fall back to the model's catalog default. Applied at all three moments the active model becomes known or changes:loadModels()(always re-resolved, not only when unset),setModel()(a switch now restores the target model's own stored pick instead of unconditionally pinning the default; switches update the in-memory level only — storage writes stay reserved for explicit picker picks viasetThinking), and a watcher oncurrentModelId()covering session switches, snapshot adoption and late catalog arrival.applyThinkingLevelpersists explicit picks under the current model id.modelThinking.ts: newlevelDeclaredBy()helper;thinkingLevelForModelSwitchgains an optionalstoredLevelparameter.useWorkspaceState.ts):submitPromptInternaland the steer path now resolve the thinking level from the prompt's OWN session model (newthinkingLevelForModelId) instead of the single active-sessionrawState.thinking— a queue drain for a background session no longer submits the level of the session the user has since switched to (the same cross-model leak on the submit path; found in review). The BTW side-chat path (useSideChat.ts) resolves from the captured parent's model too (a session switch during thestartBtwawait no longer leaks the new view's level), and skill activation (activateSkill) first persists the resolved level to the target session profile — mirroring the new-session skill path, which itself now resolves against the new session's model — so skills can't run at a stale profile effort;persistSessionProfilenow resolves a success flag (it surfaces failures itself and never rejects), and both activation paths abort instead of launching at the stale effort when the persist fails.modelThinking.test.ts,workspace-state.test.ts): updated for the new behavior and extended with cases for undeclared stored level → default fallback, session-switch re-resolution, per-model persistence, switch restoring the target model's stored pick, and queue-drain resolving the level from the drained session's own model (plus catalog-miss fallback), and model switches not persisting derived defaults (incl. the rollback path), BTW resolving from the parent model across a session switch, skill activation persisting the resolved level before activation, and the legacy pre-map pick falling back per declaring model (incl. per-model override). kimi-web suite: 632 passed;vue-tscclean; oxlint 0 errors (no new warnings).Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.