You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Sun-GLiang (via GitHub)" <gi...@apache.org> on 2026/08/25 03:05:35 UTC
[PR] fix(desktop): remove model and thinking-level success toasts [maka]
Sun-GLiang opened a new pull request, #3749:
URL: https://github.com/apache/maka/pull/3749
## Summary
- Remove success toasts after active-task model and thinking-level changes.
- Preserve localized failure toasts, permission-mode feedback, mutations, pending guards, composer-default persistence, and session refresh.
- Remove transcript/catalog dependencies and localized copy used only by the deleted success notifications.
Fixes #3745
## Verification
- Desktop focused session-settings tests: 12 passed.
- Desktop typecheck passed.
- Biome check passed for all four changed files.
- Full Desktop test suite: 1475 passed.
- Manual: switched thinking level High → Extra high → Low; the control finished at Low and no success toast appeared.
## Visual verification
### Before
Screenshot upload pending.
### After
Screenshot upload pending.
## AI use
- [ ] No generative or agentic tooling authored a material portion of this change.
- [x] Generative or agentic tooling authored a material portion of this change.
Tool(s) and scope: OpenAI Codex analyzed the existing implementation, updated the focused tests and Desktop session-settings paths, ran verification, and performed the manual UI check.
## Checklist
- [x] Tests cover the change or the PR explains why tests are not needed.
- [x] Lint, formatting, and relevant test suites pass locally.
- [x] User-facing changes update docs or the PR explains why docs are not needed.
- [x] AI use is disclosed honestly, and any materially AI-authored commit includes the required Generated-by trailer.
Does this PR entail a user-facing behavior change?
- [x] Yes, and the PR explains the change and updates relevant user-facing docs.
- [ ] No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5503764601
Thanks for the review. I checked these observations against the current head:
1. The side-chat confirmation race belongs to the Turn/configuration ordering tracked by #3347 and is explicitly outside #3745’s scope. Runtime Host currently rejects configuration transitions during an active Turn, so this cannot change the active permission boundary. I’m leaving it unchanged here.
2. The catalog-retirement concern is valid. I reproduced it under partial multi-Host coverage: the target Host’s old row can be retained while another Host advances the renderer-wide catalog revision. A robust fix requires a target-session causal revision or coverage signal across the Desktop IPC boundary, so I’ll address it in a focused follow-up with regression coverage.
3. I couldn’t locate the referenced path on head `2ff0ab249`. `app-shell-session-settings-actions.ts` is deleted by this PR, and the current model-pick path contains no `handleModelPick`, `catalogLoad`, or “Switching model…” toast. Please point me to the current path or reproduction if another surface was intended.
Therefore I’m not changing this PR for observations 1 or 3, and will track observation 2 separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks merged PR #3749:
URL: https://github.com/apache/maka/pull/3749
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5420384362
> I reviewed this head and found a blocking issue.
>
> **[P2] Model selection optimistic write races Session Send**
>
> `app-shell.tsx:3080-3087` writes model setting optimistically and returns immediately; `app-shell-chat-actions.ts:502-516` sends Session without waiting. `turn.start` may capture old model before `session.configuration.update` commits — user sees B but first turn runs with A. Shared admission only serializes enqueued ops, not cross-request ordering.
>
> Fix: restore per-Session settlement barrier or add expected-revision fence to `turn.start`; add delayed-write regression test.
>
> 简体中文
> > **Automated review notice:** This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
The race described here is a valid configuration-ordering concern, but it is explicitly out of scope for #3745 and tracked by #3347. Earlier review on this PR specifically asked to remove settle-before-send, Stop cancellation, and follow-up gating because renderer-side ordering should not own Host turn admission. The current head and its architecture test follow that direction.
Restoring the renderer settlement barrier here would reverse the agreed review direction. Unless the scope has changed, could this be treated as non-blocking and handled in #3347? If the scope has changed, I’d prefer an authoritative Host-side expected-revision/admission fence rather than reintroducing renderer-only ordering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5407362798
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 fact
Desktop currently has two different answers to "a session setting write is in flight", and which one you get depends on which setting you touched.
| | Plan / orchestration mode | Model / thinking / permission |
|---|---|---|
| Implementation | `useSessionSettingIntent` (144 lines, generic) | pending registry + React state + toast |
| Representation | one intent record | `keysRef` **and** a React state slot, kept in sync by hand |
| Rapid changes | latest intent wins | second click **silently dropped** (`if (has) return`) |
| During the write | control stays usable, shows the new value | disabled + spinner, still shows the old value |
| On success | nothing | toast |
| On failure | rolls back to the committed value | toast only, no rollback |
Every one of these five settings goes through the same shape on the Host side — the same `invokeSessionSummary` call, the same `SessionSummary` return. The divergence is entirely in the renderer.
```mermaid
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 time
```mermaid
sequenceDiagram
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)
```
```mermaid
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
- `pendingSessionModelBySession` and `pendingPermissionModeBySession` — two state slots, plus their selectors, setters, `AppShellSessionUiState` fields, and the two reads in `use-app-shell-session-ui-reads`
- `sessionModelChangeRegistry` and `permissionModeChangeRegistry` — two registry instances, four `keysRef` hand-offs, and the teardown in `app-shell-effects`
- three add/delete pairs in `app-shell-session-settings-actions.ts`, each written twice (once on the ref, once on the state slot), plus `omitSessionKey`
- the `modelChangePending` and `permissionModePending` prop chains, and `pending` / `loading` on both switchers
- the success toasts, their copy in both locales, `modelLabel` / `modelEndpointLabel`, and the `latestAssistantModelId(messages)` lookup — which also removes the entire `messages` array from the settings-action factory's dependencies, so the actions stop being rebuilt on every transcript change
Nothing new is added: `useSessionSettingIntent` is generic over the value, and the only adapter needed is `SessionSummary` to `boolean` in `write`.
For permission mode, the bypass confirmation moves to the call site — confirm first, then `request`. The hook never learns that a dialog exists, and the control keeps showing the old value until the user confirms, which is the behaviour we want anyway.
## What we give up
The 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
`pendingSessionModelChangesRef` is keyed by session and shared between the model write and the thinking-level write, so changing one blocks the other. The Host exposes them as two independent calls; the coupling is an artifact of the shared key.
"Why can't I change this right now" is derived in three places — `composer.tsx:1345`, `app-shell.tsx:1242`, `app-shell.tsx:3095` — from slightly different inputs, with two separate sets of copy. `app-shell.tsx:3091` documents a race that came from reading the persisted status instead of the live turn witness; the copy in `composer.tsx` still takes `activeSession.status` as one of its inputs.
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.
<details><summary>简体中文</summary>
补上建议背后的完整理由。这是一次幅度不小的方向调整，只给结论不太公平。
## 一个事实，两套机制
Desktop 现在对「会话设置写入在途」有两个不同的答案，取决于你动的是哪个设置。
| | Plan / 编排模式 | 模型 / 思考级别 / 权限 |
|---|---|---|
| 实现 | `useSessionSettingIntent`（144 行泛型） | pending 注册表 + React state + toast |
| 表示 | 一条 intent 记录 | `keysRef` **和** 一个 state 槽，手工保持同步 |
| 快速连改 | 最新意图胜出 | 第二次点击**被静默丢弃**（`if (has) return`） |
| 写入期间 | 控件可用，显示新值 | 禁用 + 转圈，仍显示旧值 |
| 成功时 | 无提示 | toast |
| 失败时 | 回滚到 committed 值 | 只弹 toast，不回滚 |
这五个设置在 Host 侧走的是完全相同的形状——同一个 `invokeSessionSummary`，同样返回 `SessionSummary`。分叉全部发生在渲染层。
上面两张时序图分别对应这两条路径。第二张就是你 issue 里想要的行为，它已经在仓库里了。
## 三个设置迁过去之后会消失的东西
- `pendingSessionModelBySession` 和 `pendingPermissionModeBySession` 两个 state 槽，及其 selector、setter、`AppShellSessionUiState` 字段和 `use-app-shell-session-ui-reads` 里的两处读取
- `sessionModelChangeRegistry`、`permissionModeChangeRegistry` 两个注册表实例、四处 `keysRef` 传参，以及 `app-shell-effects` 里的清理
- `app-shell-session-settings-actions.ts` 里三对 add/delete，每对都写两遍（ref 一次、state 槽一次），以及 `omitSessionKey`
- `modelChangePending` / `permissionModePending` 两条 props 链，和两个切换器上的 `pending` / `loading`
- 成功 toast、两种语言的文案、`modelLabel` / `modelEndpointLabel`，以及 `latestAssistantModelId(messages)`——这也顺带把整个 `messages` 数组从 settings-action 工厂的依赖里去掉，动作不再随转录每次变化而重建
不需要新增任何东西：`useSessionSettingIntent` 对值是泛型的，唯一需要的适配是 `write` 里把 `SessionSummary` 转成 `boolean`。
权限模式的 bypass 确认弹窗移到调用点——先确认，再 `request`。hook 完全不需要知道有弹窗，用户确认之前控件继续显示旧值，这本来也是我们想要的行为。
## 让出的东西
写入在途时控件不再被禁用。这正是你 issue 的目的，所以我认为是收益，但它确实是一次行为变化，应该明说。
## 同一片区域的两个小问题
`pendingSessionModelChangesRef` 以 session 为 key，被模型写入和思考级别写入共用，改一个会挡住另一个。Host 侧本来就是两条独立调用，这个耦合只是共用 key 的副产物。
「为什么现在不能改」在三个地方各推导了一遍——`composer.tsx:1345`、`app-shell.tsx:1242`、`app-shell.tsx:3095`——输入略有差异，还用了两套文案。`app-shell.tsx:3091` 记录过一次因为读持久化 status 而不是实时 turn witness 导致的竞态；`composer.tsx` 那份仍然把 `activeSession.status` 当作输入之一。
两个都不阻塞。趁这块打开时收掉很便宜，而且三个设置合并到同一条路径后它们会自行消失。
如果你有不同看法，随时聊——这块代码你比我更熟。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3749:
URL: https://github.com/apache/maka/pull/3749#discussion_r3860871568
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -802,6 +808,154 @@ function AppShellContent({
const activeSession = sessions.find((session) => session.id === activeId);
const activeMessageQueue = activeId ? messageQueueBySession[activeId] : undefined;
const activeDesktopSession = activeSession;
+ const modelConfigurationIntent = useSessionSettingIntent<SessionModelConfigurationIntent>({
+ catalogRevision,
+ isEqual: equalSessionModelConfigurationIntent,
+ write: async (sessionId, configuration) => {
+ const summary = await window.maka.sessions.setModelConfiguration(sessionId, {
+ ...configuration.modelTarget,
+ thinkingLevel: configuration.thinkingLevel,
+ });
+ const committed =
+ summary.llmConnectionSlug === configuration.modelTarget.llmConnectionSlug &&
+ summary.model === configuration.modelTarget.model &&
+ (summary.thinkingLevel ?? null) === configuration.thinkingLevel;
+ if (committed && configuration.changedSetting === 'model') {
+ saveComposerDefaults({ model: configuration.modelTarget });
+ }
+ return committed;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error, attempted) => {
+ if (activeIdRef.current !== sessionId) return;
+ const copy = attempted.changedSetting === 'model'
+ ? {
+ title: sessionSettingsCopy.modelFailedTitle,
+ fallback: sessionSettingsCopy.modelFallback,
+ }
+ : {
+ title: sessionSettingsCopy.thinkingFailedTitle,
+ fallback: sessionSettingsCopy.thinkingFallback,
+ };
+ showSessionError(
+ sessionId,
+ copy.title,
+ localizedShellErrorMessage(error, copy.fallback, uiLocale),
+ );
+ },
+ });
+ const permissionModeIntent = useSessionSettingIntent<PermissionMode>({
+ catalogRevision,
+ write: async (sessionId, mode) => {
+ const summary = await window.maka.sessions.setPermissionMode(sessionId, mode);
+ return summary.permissionMode === mode;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error) => {
+ if (activeIdRef.current !== sessionId) return;
+ showSessionError(
+ sessionId,
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ },
+ });
+ async function setPermissionMode(mode: PermissionMode): Promise<boolean> {
+ if (mode !== 'ask' && mode !== 'bypass') return false;
+ const owner = captureComposerImportOwner();
+ const sessionId = owner.sessionId;
+ const ownsPermissionOverlay = Boolean(
+ sessionId && Object.hasOwn(permissionModeIntent.overlayBySession, sessionId),
+ );
+ const currentMode = sessionId
+ ? ownsPermissionOverlay
+ ? permissionModeIntent.overlayBySession[sessionId]
+ : sessionsRef.current.find((session) => session.id === sessionId)?.permissionMode
+ : newTaskPermissionMode;
+ if (currentMode === mode) {
+ if (sessionId && ownsPermissionOverlay) {
+ return permissionModeIntent.request(sessionId, mode);
+ }
+ return true;
+ }
+ if (
+ mode === 'bypass' &&
+ !(await toastApi.confirm({
+ title: sessionSettingsCopy.bypassConfirmTitle,
+ description: sessionSettingsCopy.bypassConfirmDescription,
+ confirmLabel: sessionSettingsCopy.bypassConfirmLabel,
+ cancelLabel: sessionSettingsCopy.bypassCancelLabel,
+ destructive: true,
+ }))
+ ) {
+ return false;
+ }
+ if (!isComposerImportOwnerActive(owner)) return false;
+ if (sessionId) return permissionModeIntent.request(sessionId, mode);
+ try {
+ await setNewTaskPermissionMode(mode);
+ return true;
+ } catch (error) {
+ toastApi.error(
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ return false;
+ }
+ }
+ function setSessionModel(
+ sessionId: string,
+ modelTarget: SessionModelTarget,
+ ): Promise<boolean> {
+ return modelConfigurationIntent.request(
+ sessionId,
+ modelConfigurationIntentForModel(modelTarget),
+ );
+ }
+ function setSessionThinkingLevel(
Review Comment:
**P2 — changing the thinking level now writes the model back, which can undo a Host-side rebind.**
`sessions:setThinkingLevel` used to send `{ thinkingLevel }` and never touch `modelTarget`. This reads `llmConnectionSlug` / `model` out of `sessionsRef.current` and sends them along.
Failure path: Runtime rebinds the model (`app-shell-effects.ts:245` has a dedicated `reason === 'rebound'` toast), the renderer refreshes asynchronously, and if the user changes the thinking level before that refresh lands, `{ old model, new level }` is written and the rebind is silently reverted. Same for another window or the CLI changing the model.
This is new here and is not #3347 — it comes from the compound value, not from send admission. Either let `normalizeRequiredThinkingLevel` accept a missing `modelTarget`, or give thinking its own intent (same fix as the compound/independent question in the review body).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5491275078
Follow-up to my re-review — I went back to answer why the diff doubled, and two of the three blocks are not what I first assumed. Neither of these is a merge condition; the second in particular is a direction, not a request.
The joining-the-DI-architecture block is not invention: `composition/desktop-feature-services.tsx` already wires seven feature service providers, `platform/desktop/` already holds seven `create*Services`, and six of the eight have the same shape of adapter test. Session-settings is the eighth member, copied faithfully. No finding there.
**The `channels` generalization is the part worth reconsidering.** Reading all 253 lines of `packages/ui/src/session-setting-intent.ts`, there is no cross-channel coordination anywhere: every operation keys on `(channel, sessionId)`, and `channel` is only an extra map key plus an index into `options.channels`. The three places channels meet — `clear`'s loop, the catalog-revision effect's loop, and the single `overlayByChannel` state object — are all reproducible by holding N controllers and looping them.
At `d27f6fc3` the hook was 198 lines and single-channel, instantiated four times in AppShell and once in the side chat. That gave per-channel isolation *structurally*. The generalization re-implements that isolation inside one controller and then needs a 128-line test whose only case is `channels keep independent workers and overlays for the same session` — asserting a property four separate controllers had by construction. And the generality has one consumer: the desktop wrapper passes four channels, the side chat passes one.
One consequence points the wrong way for where this codebase is heading. `overlayByChannel` is a single `useState` object, so a write on any channel produces a new top-level identity and wakes every channel's readers; four controllers keep four independent states. Invisible today because all the reads sit inside AppShell — but #4316 and #4315 are moving exactly these reads below AppShell.
Dropping the generalization removes the `Values` mapped types, the nested `Map<keyof Values, Map<string, AnyIntent>>`, the `Reflect.ownKeys` initializer, the `channel` parameter threaded through `setOverlay`/`reconcile`/`request`/`isEqual`, and that test — roughly 180 lines — and costs a loop in the wrapper's `clear`. The move into `packages/ui` still stands on its own; the package already carries non-visual state hooks like `use-composer-draft` and `use-transcript-projection`.
**On the plan-mode split, I'd withdraw the way I framed it.** I said three channels reach the Host through `SessionSettingsServices` while `planMode` goes through `commitPlanMode` directly, and implied the port should grow. Having read the surroundings, that is backwards. AppShell calls `window.maka.*` 33 times across ten distinct `sessions.*` methods, so `commitPlanMode`'s three direct calls are ordinary there — the three channels that detour through a port are the unusual ones. And `commitPlanMode` is policy, not transport: it reads plan state, raises an error, confirms destructively, and branches between abandoning a proposal and writing `collaborationMode`. Putting that behind a port of three summary-returning setters would stop the port being one idea.
The more general representation is the injected closure: the port can be expressed as one, the reverse is not true, and plan mode proves it. So the lowest-entropy shape is for the wrapper to receive four `write`s of one shape — which is already the shared hook's own `channels` shape — and let the composer decide where each comes from. The split then does not need deciding; it stops existing. It also lets `session-settings-controller.test.ts` cover the plan channel, which it currently cannot fake.
The fair counter is that this is the one thing that would not match the other seven features, and that if session-settings later follows goals and module-hub below AppShell, the context port becomes load-bearing. I don't think that outweighs a four-channel boundary with one channel outside it today, but it is a real trade and it is yours to make.
Both of these are structural, and this PR has already doubled once. I would not do either here. They belong with the next round of controller-scoping work, alongside #4316 and #4315.
*AI use: audited with Claude Code (full read of the shared hook, the desktop wrapper's four channels, the seven sibling feature ports and adapters, and AppShell's direct bridge usage). I verified the pre-PR hook's arity and instantiation sites, the absence of cross-channel coordination, and that `sessions:setOrchestrationMode` validates before applying — that last one I had suspected was a missing commit check and it is not.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3749:
URL: https://github.com/apache/maka/pull/3749#discussion_r3860927383
##########
apps/desktop/src/main/__tests__/model-settings-visual-contract.test.ts:
##########
@@ -0,0 +1,162 @@
+/*
Review Comment:
**P2 — right intent, wrong mechanism.**
Pinning the agreed direction so it does not get reversed again is legitimate — that part I support. But asserting it with regexes over source text is not durable: it breaks on reformatting, and most of what it asserts (`settleSessionModelSettings`, `settingsSettlementAtSubmit`, `preSendSettingsRef`, `useSessionModelSettingsIntent`) never existed on `main`, only in this PR's reverted commits — so it guards against a past draft rather than a future regression.
A behavioral test that fails if a settlement barrier reappears, or a type that makes it unrepresentable, would hold the same line without going red on the next refactor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5477157093
Hi! @Astro-Han
Thanks for the scope review. I rebuilt this PR on top of the latest `main` and force-pushed a single focused commit (`d27f6fc37`).
Kept because they are directly required by #3745 or by bugs identified during review:
- the shared `useSessionSettingIntent` flow for latest-wins, rollback, catalog retirement, and teardown;
- compound model/thinking configuration through `sessions:setModelConfiguration`, as the later review clarified this is required to avoid stale model/thinking tuples;
- optimistic presentation only for the affected controls, while session health and execution state remain authoritative;
- the same intent mechanism for Quote Companion permission changes;
- removal of success toasts, pending spinners/registries, and the related copy;
- accessible radio semantics and current-value announcements for model/thinking selectors.
Removed from this PR:
- the Conversation feature/ports/provider architecture migration;
- catalog-diff session purge handling;
- the new-task permission write helper;
- Plan/Swarm/Graph action relocation;
- unrelated architecture/inventory work and redundant test harnesses.
The diff is now 25 files (`+759/-929`), down from 45 files (`+2809/-1123`).
Verification:
- Desktop: 1726/1726 tests passed
- UI: 276/276 tests passed
- Desktop/UI typecheck passed
- Biome passed
- Renderer architecture: 60/60 passed
- AppShell hook-scope check passed
Please review from `d27f6fc37`; the previous large implementation has been replaced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3749:
URL: https://github.com/apache/maka/pull/3749#discussion_r3860871584
##########
apps/desktop/src/main/__tests__/model-settings-visual-contract.test.ts:
##########
@@ -0,0 +1,162 @@
+/*
Review Comment:
**P2 — this file asserts against source text, and most of what it asserts never existed on `main`.**
`readFileSync` plus regexes over source. It is fragile in the ordinary way — `assert.doesNotMatch(chatActionsSource, /^\s*if \(!isShellSurfaceOwnerActive\(sendOwner\)\) return false;$/m)` asserts that one specific line of source text is absent, so any reformatting breaks it — but the larger problem is that identifiers like `settleSessionModelSettings`, `settingsSettlementAtSubmit`, `preSendSettingsRef`, and `useSessionModelSettingsIntent` are not on `main` at all. They only ever existed in the earlier commits of this PR that were asked to be reverted.
So this pins a review round-trip as a permanent test, with no coverage of product behavior. Behavior is already covered by `session-setting-intent.test.ts` (+643) and `chat-model-switcher.test.tsx` (+189). Suggest dropping the file.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -988,6 +1122,19 @@ function AppShellContent({
);
},
});
+ const previousCatalogSessionIdsRef = useRef(
+ new Set(sessions.map((session) => session.id)),
+ );
+ useEffect(() => {
+ const currentSessionIds = new Set(sessionsRef.current.map((session) => session.id));
+ for (const sessionId of removedSessionIds(
Review Comment:
**P1 — "absent from this catalog snapshot" is treated as "deleted", triggering the full teardown.**
`clearSessionRendererState` reaches `clearAppShellSessionUiStateForSession`, which loops **all** of `SESSION_UI_MAP_KEYS` — including `messageQueueBySession` (messages the user typed and queued) and `interactionBySession` (pending permission prompts). `app-shell-session-ui-state.ts:66` scopes that full clear explicitly: a full clear "(session deletion) would wipe them too".
The three existing deletion paths are all narrower, and `app-shell-effects.ts:185-193` in particular deliberately narrows to the current session via `desktopSessionHostId(activeSessionId) === event.hostId`. This effect overrides that narrowing: the same handler calls `refreshSessions()` right after, so when a Host is briefly unavailable and its sessions drop out of the list, every one of them loses its queued messages and permission queue.
That is unrecoverable loss of user-authored content on a plausible fault/reconnect path, and it is silent — hence P1 rather than P2.
What this PR actually needs here is to clear the two new intent overlays. `session-catalog-removals.test.ts` only covers the one-line pure function; nothing covers this effect's blast radius.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -802,6 +808,154 @@ function AppShellContent({
const activeSession = sessions.find((session) => session.id === activeId);
const activeMessageQueue = activeId ? messageQueueBySession[activeId] : undefined;
const activeDesktopSession = activeSession;
+ const modelConfigurationIntent = useSessionSettingIntent<SessionModelConfigurationIntent>({
+ catalogRevision,
+ isEqual: equalSessionModelConfigurationIntent,
+ write: async (sessionId, configuration) => {
+ const summary = await window.maka.sessions.setModelConfiguration(sessionId, {
+ ...configuration.modelTarget,
+ thinkingLevel: configuration.thinkingLevel,
+ });
+ const committed =
+ summary.llmConnectionSlug === configuration.modelTarget.llmConnectionSlug &&
+ summary.model === configuration.modelTarget.model &&
+ (summary.thinkingLevel ?? null) === configuration.thinkingLevel;
+ if (committed && configuration.changedSetting === 'model') {
+ saveComposerDefaults({ model: configuration.modelTarget });
+ }
+ return committed;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error, attempted) => {
+ if (activeIdRef.current !== sessionId) return;
+ const copy = attempted.changedSetting === 'model'
+ ? {
+ title: sessionSettingsCopy.modelFailedTitle,
+ fallback: sessionSettingsCopy.modelFallback,
+ }
+ : {
+ title: sessionSettingsCopy.thinkingFailedTitle,
+ fallback: sessionSettingsCopy.thinkingFallback,
+ };
+ showSessionError(
+ sessionId,
+ copy.title,
+ localizedShellErrorMessage(error, copy.fallback, uiLocale),
+ );
+ },
+ });
+ const permissionModeIntent = useSessionSettingIntent<PermissionMode>({
+ catalogRevision,
+ write: async (sessionId, mode) => {
+ const summary = await window.maka.sessions.setPermissionMode(sessionId, mode);
+ return summary.permissionMode === mode;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error) => {
+ if (activeIdRef.current !== sessionId) return;
+ showSessionError(
+ sessionId,
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ },
+ });
+ async function setPermissionMode(mode: PermissionMode): Promise<boolean> {
+ if (mode !== 'ask' && mode !== 'bypass') return false;
+ const owner = captureComposerImportOwner();
+ const sessionId = owner.sessionId;
+ const ownsPermissionOverlay = Boolean(
+ sessionId && Object.hasOwn(permissionModeIntent.overlayBySession, sessionId),
+ );
+ const currentMode = sessionId
+ ? ownsPermissionOverlay
+ ? permissionModeIntent.overlayBySession[sessionId]
+ : sessionsRef.current.find((session) => session.id === sessionId)?.permissionMode
+ : newTaskPermissionMode;
+ if (currentMode === mode) {
+ if (sessionId && ownsPermissionOverlay) {
+ return permissionModeIntent.request(sessionId, mode);
+ }
+ return true;
+ }
+ if (
+ mode === 'bypass' &&
+ !(await toastApi.confirm({
+ title: sessionSettingsCopy.bypassConfirmTitle,
+ description: sessionSettingsCopy.bypassConfirmDescription,
+ confirmLabel: sessionSettingsCopy.bypassConfirmLabel,
+ cancelLabel: sessionSettingsCopy.bypassCancelLabel,
+ destructive: true,
+ }))
+ ) {
+ return false;
+ }
+ if (!isComposerImportOwnerActive(owner)) return false;
+ if (sessionId) return permissionModeIntent.request(sessionId, mode);
+ try {
Review Comment:
**P3 — the no-session (new task) branch lost its re-entrancy guard.**
The old implementation used a `'__global_permission_mode__'` pendingKey to block re-entry. This awaits `setNewTaskPermissionMode(mode)` directly with no in-flight protection, so a quick ask -> bypass -> ask fires concurrent persistence writes with no ordering guarantee. Minor — one more click corrects it.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -802,6 +808,154 @@ function AppShellContent({
const activeSession = sessions.find((session) => session.id === activeId);
const activeMessageQueue = activeId ? messageQueueBySession[activeId] : undefined;
const activeDesktopSession = activeSession;
+ const modelConfigurationIntent = useSessionSettingIntent<SessionModelConfigurationIntent>({
+ catalogRevision,
+ isEqual: equalSessionModelConfigurationIntent,
+ write: async (sessionId, configuration) => {
+ const summary = await window.maka.sessions.setModelConfiguration(sessionId, {
+ ...configuration.modelTarget,
+ thinkingLevel: configuration.thinkingLevel,
+ });
+ const committed =
+ summary.llmConnectionSlug === configuration.modelTarget.llmConnectionSlug &&
+ summary.model === configuration.modelTarget.model &&
+ (summary.thinkingLevel ?? null) === configuration.thinkingLevel;
+ if (committed && configuration.changedSetting === 'model') {
+ saveComposerDefaults({ model: configuration.modelTarget });
+ }
+ return committed;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error, attempted) => {
+ if (activeIdRef.current !== sessionId) return;
+ const copy = attempted.changedSetting === 'model'
+ ? {
+ title: sessionSettingsCopy.modelFailedTitle,
+ fallback: sessionSettingsCopy.modelFallback,
+ }
+ : {
+ title: sessionSettingsCopy.thinkingFailedTitle,
+ fallback: sessionSettingsCopy.thinkingFallback,
+ };
+ showSessionError(
+ sessionId,
+ copy.title,
+ localizedShellErrorMessage(error, copy.fallback, uiLocale),
+ );
+ },
+ });
+ const permissionModeIntent = useSessionSettingIntent<PermissionMode>({
+ catalogRevision,
+ write: async (sessionId, mode) => {
+ const summary = await window.maka.sessions.setPermissionMode(sessionId, mode);
+ return summary.permissionMode === mode;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error) => {
+ if (activeIdRef.current !== sessionId) return;
+ showSessionError(
+ sessionId,
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ },
+ });
+ async function setPermissionMode(mode: PermissionMode): Promise<boolean> {
+ if (mode !== 'ask' && mode !== 'bypass') return false;
+ const owner = captureComposerImportOwner();
+ const sessionId = owner.sessionId;
+ const ownsPermissionOverlay = Boolean(
+ sessionId && Object.hasOwn(permissionModeIntent.overlayBySession, sessionId),
+ );
+ const currentMode = sessionId
+ ? ownsPermissionOverlay
+ ? permissionModeIntent.overlayBySession[sessionId]
+ : sessionsRef.current.find((session) => session.id === sessionId)?.permissionMode
+ : newTaskPermissionMode;
+ if (currentMode === mode) {
+ if (sessionId && ownsPermissionOverlay) {
+ return permissionModeIntent.request(sessionId, mode);
+ }
+ return true;
+ }
+ if (
+ mode === 'bypass' &&
+ !(await toastApi.confirm({
+ title: sessionSettingsCopy.bypassConfirmTitle,
+ description: sessionSettingsCopy.bypassConfirmDescription,
+ confirmLabel: sessionSettingsCopy.bypassConfirmLabel,
+ cancelLabel: sessionSettingsCopy.bypassCancelLabel,
+ destructive: true,
+ }))
+ ) {
+ return false;
+ }
+ if (!isComposerImportOwnerActive(owner)) return false;
+ if (sessionId) return permissionModeIntent.request(sessionId, mode);
+ try {
+ await setNewTaskPermissionMode(mode);
+ return true;
+ } catch (error) {
+ toastApi.error(
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ return false;
+ }
+ }
+ function setSessionModel(
+ sessionId: string,
+ modelTarget: SessionModelTarget,
+ ): Promise<boolean> {
+ return modelConfigurationIntent.request(
+ sessionId,
+ modelConfigurationIntentForModel(modelTarget),
+ );
+ }
+ function setSessionThinkingLevel(
Review Comment:
**P2 — changing the thinking level now writes the model back, which can undo a Host-side rebind.**
`sessions:setThinkingLevel` used to send `{ thinkingLevel }` and never touch `modelTarget`. This reads `llmConnectionSlug` / `model` out of `sessionsRef.current` and sends them along.
Failure path: Runtime rebinds the model (`app-shell-effects.ts:245` has a dedicated `reason === 'rebound'` toast), the renderer refreshes asynchronously, and if the user changes the thinking level before that refresh lands, `{ old model, new level }` is written and the rebind is silently reverted. Same for another window or the CLI changing the model.
This is new here and is not #3347 — it comes from the compound value, not from send admission. Either let `normalizeRequiredThinkingLevel` accept a missing `modelTarget`, or give thinking its own intent (same fix as the compound/independent question in the review body).
##########
apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts:
##########
@@ -202,6 +202,14 @@ export function registerRuntimeHostSessionCatalogIpc(
const modelTarget = normalizeExplicitModel(input);
return updateConfiguration(deps, sessionId, { modelTarget, thinkingLevel: null }, 'updated');
});
+ ipcMain.handle(
Review Comment:
**P2 — this leaves three channels where the PR set out to leave one, and one of them is dead.**
With `thinkingLevel: null` this new handler is word-for-word `sessions:setModel` four lines above: both call `updateConfiguration(deps, sessionId, { modelTarget, thinkingLevel: null }, 'updated')`.
After this PR `sessions:setModel` has zero production consumers — its only caller went away with `app-shell-session-settings-actions.ts`, and no e2e uses it. What remains is the ipcMain handler, `preload.ts:1956`, and `bridge-contract.d.ts:960`. `sessions:setThinkingLevel` is down to a single e2e caller (`slash-command-menu.spec.ts:251`).
The renderer converged; the protocol gained a channel and kept two orphans. Worth deleting `sessions:setModel` in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5422660961
On the send-ordering race: you are right, and that automated finding is non-blocking here. Renderer-side settlement was removed at my request because ordering in front of Host turn admission is the wrong place for it, and #3347 is open for exactly this. If we fence it, I agree it should be a Host-side expected-revision check on `turn.start`, not a renderer barrier — nothing to change in this PR.
Separately, my earlier review today was retracted: it asked you to split the compound model/thinking intent back apart, which would have undone the stale-tuple fix I asked for on 08-25. Sorry for the churn — the re-review reflects the corrected position.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): remove model and thinking-level success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5405887467
Addressed both blocking paths in ee34dd002.
- Stop now cancels and disarms a local pre-send settings flight before Runtime Host admission; when no local flight exists, it retains the existing `sessions.stop` behavior.
- Active-turn follow-ups now settle the latest model/thinking intent before enqueueing. A terminal settlement failure skips enqueue and leaves the composer draft available.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5407334052
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.** `apps/desktop/src/renderer/use-session-setting-intent.ts` is a 144-line generic hook that already does what the new 420-line `use-session-model-settings-intent.ts` does: optimistic overlay, latest-intent-wins coalescing, rollback to the committed value on failure, retirement against `catalogRevision`. Plan mode and orchestration mode already use it (`app-shell.tsx:928`, `:940`). Model and thinking level should join it rather than get a parallel implementation with its own version counters.
**Two of the three reported problems need no new machinery.** Stacked toasts and the wrong `from` model both disappear when the success toasts go. That `from` comes from `latestAssistantModelId(messages)` — the transcript, not the configuration before the switch — which is exactly why `A → B → C` can report `A → C`. It is also the only Desktop consumer of `latestAssistantModelId`, and the only reason the whole `messages` array is a dependency of the settings-action factory.
**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:
- route `setModel`, `setThinkingLevel` and `setPermissionMode` through the existing `useSessionSettingIntent`. For permission mode, move the bypass confirmation to the call site: the hook stays unaware of it, and the control keeps showing the old value until the user confirms;
- delete `sessionModelChangeRegistry`, `permissionModeChangeRegistry`, `pendingSessionModelBySession`, `pendingPermissionModeBySession`, the `modelChangePending` / `permissionModePending` prop chains, and `pending` / `loading` on the switchers;
- delete the success toasts and the copy, label helpers and transcript lookup that exist only for them. Failure toasts stay.
Two smaller things worth closing while you are here: `pendingSessionModelChangesRef` is keyed by session and shared between the model and thinking-level writes, so each blocks the other even though the Host exposes them as independent calls; and "why can't I change this now" is derived three times (`composer.tsx:1345`, `app-shell.tsx:1242`, `app-shell.tsx:3095`) with two separate sets of copy.
<details><summary>简体中文</summary>
感谢你细致的工作，测试尤其扎实。但在继续之前我想调整一下方向。
**这个 seam 已经存在。** `apps/desktop/src/renderer/use-session-setting-intent.ts` 是一个 144 行的泛型 hook，新增的 420 行 `use-session-model-settings-intent.ts` 做的是同一件事：乐观 overlay、最新意图胜出的合流、失败回滚到 committed 值、按 `catalogRevision` 退休。Plan 模式和编排模式已经在用它（`app-shell.tsx:928`、`:940`）。模型和思考级别应该接进去，而不是在旁边再实现一套带版本号的。
**三个问题里有两个不需要新机制。** toast 堆叠和 `from` 取错模型，删掉成功 toast 就都没了。那个 `from` 取自 `latestAssistantModelId(messages)`——转录，而不是切换前的配置，这正是 `A → B → C` 会报成 `A → C` 的原因。它也是 Desktop 侧 `latestAssistantModelId` 的唯一消费者，以及整个 `messages` 数组成为 settings-action 工厂依赖的唯一原因。
**有一部分属于 #3347，而我们还没有决定要做它。** 发送前 settle、Stop 取消在途的设置写入、follow-up 进队列前先 settle，解决的都是「发送时用的配置可能还没提交到 Host」——那是 #3347 的问题，而且它让渲染层排在了 Host 的 Turn 准入之前。请从本 PR 移除。
**建议的形状**，结果应该是净删除：
- `setModel`、`setThinkingLevel`、`setPermissionMode` 全部走既有的 `useSessionSettingIntent`。权限模式的 bypass 确认弹窗提到调用点：hook 不需要知道它存在，用户确认之前控件继续显示旧值；
- 删除 `sessionModelChangeRegistry`、`permissionModeChangeRegistry`、`pendingSessionModelBySession`、`pendingPermissionModeBySession`、`modelChangePending` / `permissionModePending` 两条 props 链，以及切换器上的 `pending` / `loading`；
- 删除成功 toast，以及只为它们存在的文案、标签辅助函数和转录取源。失败 toast 保留。
另有两处顺手可以收掉：`pendingSessionModelChangesRef` 以 session 为 key，被模型写入和思考级别写入共用，于是两者互相阻塞，而 Host 侧本来就是两条独立调用；另外「为什么现在不能改」被推导了三遍（`composer.tsx:1345`、`app-shell.tsx:1242`、`app-shell.tsx:3095`），用了两套不同的文案。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5503771836
@me2seeks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5503881851
@me2seeks I believe this PR is ready to merge. Observation 2 is a real but non-blocking edge case involving partial multi-Host catalog refreshes. I’m currently working on the fix and will open a separate follow-up PR for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): remove model and thinking-level success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5407012551
Addressed the latest settlement/readiness findings in b1ae2a754.
- Existing-session sends now capture the active settings settlement synchronously at send entry, before transcript or readiness awaits. A mutation failure remains observable after its intent is retired, so the send is skipped and the draft stays intact.
- Readiness now distinguishes an authoritative model overlay from the absence of a model override. A model adopted from a thinking-mutation response is checked before the first readiness probe, even when there was no local model selection.
- Mutation response fencing now uses a per-session catalog generation and only treats a response as stale when the session moved to a different model context. A same-model success is accepted even if an older catalog snapshot committed first, so it is not retried and cannot be negated by a later retry failure.
Added regression coverage for all three races. Desktop tests pass 1516/1516; production build, typecheck, Biome, and diff checks also pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on code in PR #3749:
URL: https://github.com/apache/maka/pull/3749#discussion_r3861400545
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -988,6 +1122,19 @@ function AppShellContent({
);
},
});
+ const previousCatalogSessionIdsRef = useRef(
+ new Set(sessions.map((session) => session.id)),
+ );
+ useEffect(() => {
+ const currentSessionIds = new Set(sessionsRef.current.map((session) => session.id));
+ for (const sessionId of removedSessionIds(
Review Comment:
Addressed in e7386e523. Removed the catalog-diff effect, session-catalog-removals.ts, and its pure-function test, so catalog absence no longer invokes clearSessionRendererState. Cleanup now follows explicit deletion facts: every deleted event clears that Session while selection and messages reset only when it is active; purge retains pre-write revision families and clears them only after removal or post-rejection verification confirms deletion. Added app-shell-deleted-session-state.test.ts and expanded session-navigation-session-purge.test.ts.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -802,6 +808,154 @@ function AppShellContent({
const activeSession = sessions.find((session) => session.id === activeId);
const activeMessageQueue = activeId ? messageQueueBySession[activeId] : undefined;
const activeDesktopSession = activeSession;
+ const modelConfigurationIntent = useSessionSettingIntent<SessionModelConfigurationIntent>({
+ catalogRevision,
+ isEqual: equalSessionModelConfigurationIntent,
+ write: async (sessionId, configuration) => {
+ const summary = await window.maka.sessions.setModelConfiguration(sessionId, {
+ ...configuration.modelTarget,
+ thinkingLevel: configuration.thinkingLevel,
+ });
+ const committed =
+ summary.llmConnectionSlug === configuration.modelTarget.llmConnectionSlug &&
+ summary.model === configuration.modelTarget.model &&
+ (summary.thinkingLevel ?? null) === configuration.thinkingLevel;
+ if (committed && configuration.changedSetting === 'model') {
+ saveComposerDefaults({ model: configuration.modelTarget });
+ }
+ return committed;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error, attempted) => {
+ if (activeIdRef.current !== sessionId) return;
+ const copy = attempted.changedSetting === 'model'
+ ? {
+ title: sessionSettingsCopy.modelFailedTitle,
+ fallback: sessionSettingsCopy.modelFallback,
+ }
+ : {
+ title: sessionSettingsCopy.thinkingFailedTitle,
+ fallback: sessionSettingsCopy.thinkingFallback,
+ };
+ showSessionError(
+ sessionId,
+ copy.title,
+ localizedShellErrorMessage(error, copy.fallback, uiLocale),
+ );
+ },
+ });
+ const permissionModeIntent = useSessionSettingIntent<PermissionMode>({
+ catalogRevision,
+ write: async (sessionId, mode) => {
+ const summary = await window.maka.sessions.setPermissionMode(sessionId, mode);
+ return summary.permissionMode === mode;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error) => {
+ if (activeIdRef.current !== sessionId) return;
+ showSessionError(
+ sessionId,
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ },
+ });
+ async function setPermissionMode(mode: PermissionMode): Promise<boolean> {
+ if (mode !== 'ask' && mode !== 'bypass') return false;
+ const owner = captureComposerImportOwner();
+ const sessionId = owner.sessionId;
+ const ownsPermissionOverlay = Boolean(
+ sessionId && Object.hasOwn(permissionModeIntent.overlayBySession, sessionId),
+ );
+ const currentMode = sessionId
+ ? ownsPermissionOverlay
+ ? permissionModeIntent.overlayBySession[sessionId]
+ : sessionsRef.current.find((session) => session.id === sessionId)?.permissionMode
+ : newTaskPermissionMode;
+ if (currentMode === mode) {
+ if (sessionId && ownsPermissionOverlay) {
+ return permissionModeIntent.request(sessionId, mode);
+ }
+ return true;
+ }
+ if (
+ mode === 'bypass' &&
+ !(await toastApi.confirm({
+ title: sessionSettingsCopy.bypassConfirmTitle,
+ description: sessionSettingsCopy.bypassConfirmDescription,
+ confirmLabel: sessionSettingsCopy.bypassConfirmLabel,
+ cancelLabel: sessionSettingsCopy.bypassCancelLabel,
+ destructive: true,
+ }))
+ ) {
+ return false;
+ }
+ if (!isComposerImportOwnerActive(owner)) return false;
+ if (sessionId) return permissionModeIntent.request(sessionId, mode);
+ try {
Review Comment:
Addressed in e7386e523. The no-session branch now uses persistNewTaskPermissionMode with a synchronous in-flight guard and guaranteed release in finally. Behavior tests cover rejecting a concurrent second persistence write and allowing retry after a failed write.
##########
apps/desktop/src/main/__tests__/model-settings-visual-contract.test.ts:
##########
@@ -0,0 +1,162 @@
+/*
Review Comment:
Addressed in e7386e523. Deleted model-settings-visual-contract.test.ts and its source-text regex contracts. The maintained guarantees now live at behavior and type boundaries: session-setting-intent, session-model-configuration-intent, session-mode-ipc-main, chat-model-switcher, and TypeScript checks for removed props and bridge members. No test pins identifiers from reverted PR drafts.
##########
apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts:
##########
@@ -202,6 +202,14 @@ export function registerRuntimeHostSessionCatalogIpc(
const modelTarget = normalizeExplicitModel(input);
return updateConfiguration(deps, sessionId, { modelTarget, thinkingLevel: null }, 'updated');
});
+ ipcMain.handle(
Review Comment:
Addressed in e7386e523. Removed the legacy sessions:setModel main handler, preload method, and bridge contract member. sessions:setModelConfiguration remains the compound production path. session-mode-ipc-main.test.ts now asserts that the compound channel exists and the model-only channel does not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5423340392
Addressed the review summary in e7386e523. Side chat permission changes now use the same useSessionSettingIntent<PermissionMode> seam: the selected mode is projected immediately, rapid changes coalesce latest-wins, and a stale first response cannot replace the newer overlay. Removed permissionModePending and the newly added permissionChanging copy. The compound modelConfigurationIntent and sessions:setModelConfiguration remain intact. Local verification passes: Desktop 1532/1532, Desktop typecheck, production build, full-repo Biome, and diff checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3749:
URL: https://github.com/apache/maka/pull/3749#discussion_r3860927369
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -988,6 +1122,19 @@ function AppShellContent({
);
},
});
+ const previousCatalogSessionIdsRef = useRef(
+ new Set(sessions.map((session) => session.id)),
+ );
+ useEffect(() => {
+ const currentSessionIds = new Set(sessionsRef.current.map((session) => session.id));
+ for (const sessionId of removedSessionIds(
Review Comment:
**P2 — "absent from this catalog snapshot" runs the deletion-semantics teardown.**
`clearSessionRendererState` reaches `clearAppShellSessionUiStateForSession`, which loops **all** of `SESSION_UI_MAP_KEYS` — including `messageQueueBySession` (messages the user typed and queued) and `interactionBySession` (pending permission prompts). `app-shell-session-ui-state.ts:66` scopes that full clear explicitly: it is what "(session deletion) would wipe them too" refers to.
I checked the obvious way this could go wrong and it does not: `createSessionListRefresher` is fail-closed — on a failed `listSessions` it keeps `currentSessions()` and never calls `commitSessions` — so a transient Host failure cannot produce a short snapshot. That is why this is P2 and not higher.
What is left is that absence and deletion are being treated as the same fact by the widest teardown available, while the three existing deletion paths are all narrower — `app-shell-effects.ts:185-193` in particular deliberately narrows to the current session via `desktopSessionHostId(activeSessionId) === event.hostId`, and this effect overrides that narrowing.
Two things would settle it: which non-deletion conditions can make a *successful* `listSessions` omit a live session (archive, host removal, any scoped query?), and whether clearing just the two new intent overlays is sufficient here — which is all this PR actually needs. `session-catalog-removals.test.ts` covers the one-line pure function but nothing covers this effect's blast radius.
##########
apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts:
##########
@@ -202,6 +202,14 @@ export function registerRuntimeHostSessionCatalogIpc(
const modelTarget = normalizeExplicitModel(input);
return updateConfiguration(deps, sessionId, { modelTarget, thinkingLevel: null }, 'updated');
});
+ ipcMain.handle(
Review Comment:
**P2 — `sessions:setModel` is now dead and should go with this PR.**
With `thinkingLevel: null` this new handler is word-for-word the `sessions:setModel` handler four lines above: both call `updateConfiguration(deps, sessionId, { modelTarget, thinkingLevel: null }, 'updated')`.
After this PR `sessions:setModel` has zero production consumers — its only caller went away with `app-shell-session-settings-actions.ts`, and no e2e uses it. What remains is the ipcMain handler, `preload.ts:1956`, and `bridge-contract.d.ts:960`. `sessions:setThinkingLevel` is down to one e2e caller (`slash-command-menu.spec.ts:251`).
Since the point of the PR is collapsing two mechanisms into one, leaving two orphaned channels behind undercuts it. Deleting `sessions:setModel` here is a clean net removal.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -802,6 +808,154 @@ function AppShellContent({
const activeSession = sessions.find((session) => session.id === activeId);
const activeMessageQueue = activeId ? messageQueueBySession[activeId] : undefined;
const activeDesktopSession = activeSession;
+ const modelConfigurationIntent = useSessionSettingIntent<SessionModelConfigurationIntent>({
+ catalogRevision,
+ isEqual: equalSessionModelConfigurationIntent,
+ write: async (sessionId, configuration) => {
+ const summary = await window.maka.sessions.setModelConfiguration(sessionId, {
+ ...configuration.modelTarget,
+ thinkingLevel: configuration.thinkingLevel,
+ });
+ const committed =
+ summary.llmConnectionSlug === configuration.modelTarget.llmConnectionSlug &&
+ summary.model === configuration.modelTarget.model &&
+ (summary.thinkingLevel ?? null) === configuration.thinkingLevel;
+ if (committed && configuration.changedSetting === 'model') {
+ saveComposerDefaults({ model: configuration.modelTarget });
+ }
+ return committed;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error, attempted) => {
+ if (activeIdRef.current !== sessionId) return;
+ const copy = attempted.changedSetting === 'model'
+ ? {
+ title: sessionSettingsCopy.modelFailedTitle,
+ fallback: sessionSettingsCopy.modelFallback,
+ }
+ : {
+ title: sessionSettingsCopy.thinkingFailedTitle,
+ fallback: sessionSettingsCopy.thinkingFallback,
+ };
+ showSessionError(
+ sessionId,
+ copy.title,
+ localizedShellErrorMessage(error, copy.fallback, uiLocale),
+ );
+ },
+ });
+ const permissionModeIntent = useSessionSettingIntent<PermissionMode>({
+ catalogRevision,
+ write: async (sessionId, mode) => {
+ const summary = await window.maka.sessions.setPermissionMode(sessionId, mode);
+ return summary.permissionMode === mode;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error) => {
+ if (activeIdRef.current !== sessionId) return;
+ showSessionError(
+ sessionId,
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ },
+ });
+ async function setPermissionMode(mode: PermissionMode): Promise<boolean> {
+ if (mode !== 'ask' && mode !== 'bypass') return false;
+ const owner = captureComposerImportOwner();
+ const sessionId = owner.sessionId;
+ const ownsPermissionOverlay = Boolean(
+ sessionId && Object.hasOwn(permissionModeIntent.overlayBySession, sessionId),
+ );
+ const currentMode = sessionId
+ ? ownsPermissionOverlay
+ ? permissionModeIntent.overlayBySession[sessionId]
+ : sessionsRef.current.find((session) => session.id === sessionId)?.permissionMode
+ : newTaskPermissionMode;
+ if (currentMode === mode) {
+ if (sessionId && ownsPermissionOverlay) {
+ return permissionModeIntent.request(sessionId, mode);
+ }
+ return true;
+ }
+ if (
+ mode === 'bypass' &&
+ !(await toastApi.confirm({
+ title: sessionSettingsCopy.bypassConfirmTitle,
+ description: sessionSettingsCopy.bypassConfirmDescription,
+ confirmLabel: sessionSettingsCopy.bypassConfirmLabel,
+ cancelLabel: sessionSettingsCopy.bypassCancelLabel,
+ destructive: true,
+ }))
+ ) {
+ return false;
+ }
+ if (!isComposerImportOwnerActive(owner)) return false;
+ if (sessionId) return permissionModeIntent.request(sessionId, mode);
+ try {
Review Comment:
**P3 — the no-session (new task) branch lost its re-entrancy guard.**
The old implementation used a `'__global_permission_mode__'` pendingKey to block re-entry. This awaits `setNewTaskPermissionMode(mode)` directly with no in-flight protection, so a quick ask -> bypass -> ask fires concurrent persistence writes with no ordering guarantee. Minor — one more click corrects it.
##########
apps/desktop/src/main/__tests__/model-settings-visual-contract.test.ts:
##########
@@ -0,0 +1,162 @@
+/*
Review Comment:
**P2 — this asserts against source text, and most of what it asserts never existed on `main`.**
`readFileSync` plus regexes over source. Fragile in the ordinary way — `assert.doesNotMatch(chatActionsSource, /^\s*if \(!isShellSurfaceOwnerActive\(sendOwner\)\) return false;$/m)` asserts one specific line of source text is absent, so reformatting breaks it — but the larger problem is that `settleSessionModelSettings`, `settingsSettlementAtSubmit`, `preSendSettingsRef`, and `useSessionModelSettingsIntent` are not on `main` at all. They existed only in the earlier commits of this PR that were reverted.
So it pins a review round-trip as a permanent test with no coverage of product behavior, and it will go red on the next refactor for reasons unrelated to correctness. Behavior is already covered by `session-setting-intent.test.ts` (+643) and `chat-model-switcher.test.tsx` (+189). Suggest dropping the file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5478331519
CI update on exact head `d27f6fc3`: the `test` job failed in `Check renderer architecture`.
The checker’s 60 fixture tests pass; the repository check then rejects the PR’s renderer debt growth relative to base `29d02dc2`. The failures form one architectural cluster:
- session-setting ownership moved back into `app-shell.tsx`, increasing bridge, dependency, hook, import, and token debt;
- side chat now imports the legacy root `use-session-setting-intent.ts`, creating a forbidden feature-to-legacy dependency;
- `session-model-configuration-intent.ts` is a new unclassified renderer-root source;
- the legacy `use-session-setting-intent.ts` also increases its stateful-hook debt.
Updating `renderer-architecture.json` does not admit these changes because the ledger is a monotonic ceiling compared against the base revision.
I will keep the reviewed behavior intact — the compound `modelConfigurationIntent`, the single `sessions:setModelConfiguration` production path, and convergence with side chat. The follow-up will refactor ownership and call sites so AppShell debt does not grow, remove the feature-to-legacy dependency, and place the shared intent mechanism behind a legal seam. It will not split the compound model/thinking controller or restore the old pending/toast mechanisms.
I’ll request another review after the architecture check and targeted regression tests are green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5477222815
Merging after the CI pass!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5477222829
Merging after the CI pass!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5478125366
Thanks — I was ready to merge once CI passed, but the exact-head `test` is failing the Renderer architecture check. Please fix the failure and rerun CI; once the required check is green, we can merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5470089827
Thanks for continuing to converge this toward the existing `useSessionSettingIntent` seam. The core direction is right: remove the success-toast and pending-registry machinery, show the latest intent immediately, and retain failure rollback.
Before another review round, could you rebase onto current `main` and slim the cumulative diff back to the #3745 obligation? The current branch is conflicting and still carries `+2016/-942` across 35 files. It includes about 1,355 lines of test additions, a 643-line session-intent test file, and deleted-session, purge, and quote-companion changes that need to be separated from or explicitly justified by the setting-feedback goal.
This issue should ideally land as a net simplification around the existing intent owner. Once the refreshed diff contains only that end state and the necessary behavioral coverage, we should be able to review and merge it quickly.
Please push back where an apparently adjacent path is required to preserve the same setting-intent invariant.
AI-assisted review disclosure: Codex assisted with the cumulative-diff inventory; Astro-Han reviewed the accepted direction and owns this recommendation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3749:
URL: https://github.com/apache/maka/pull/3749#issuecomment-5489245164
Hi @Astro-Han ,
I’ve pushed the latest updates, resolved the merge conflicts with `apache/main`, and verified that CI is now green.
Since these commits were added after the previous approval, could you please take another look at the updated diff and re-approve if everything looks good? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): make model and thinking-level changes instant without success toasts [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3749:
URL: https://github.com/apache/maka/pull/3749#discussion_r3860871560
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -988,6 +1122,19 @@ function AppShellContent({
);
},
});
+ const previousCatalogSessionIdsRef = useRef(
+ new Set(sessions.map((session) => session.id)),
+ );
+ useEffect(() => {
+ const currentSessionIds = new Set(sessionsRef.current.map((session) => session.id));
+ for (const sessionId of removedSessionIds(
Review Comment:
**P1 — "absent from this catalog snapshot" is treated as "deleted", triggering the full teardown.**
`clearSessionRendererState` reaches `clearAppShellSessionUiStateForSession`, which loops **all** of `SESSION_UI_MAP_KEYS` — including `messageQueueBySession` (messages the user typed and queued) and `interactionBySession` (pending permission prompts). `app-shell-session-ui-state.ts:66` scopes that full clear explicitly: a full clear "(session deletion) would wipe them too".
The three existing deletion paths are all narrower, and `app-shell-effects.ts:185-193` in particular deliberately narrows to the current session via `desktopSessionHostId(activeSessionId) === event.hostId`. This effect overrides that narrowing: the same handler calls `refreshSessions()` right after, so when a Host is briefly unavailable and its sessions drop out of the list, every one of them loses its queued messages and permission queue.
That is unrecoverable loss of user-authored content on a plausible fault/reconnect path, and it is silent — hence P1 rather than P2.
What this PR actually needs here is to clear the two new intent overlays. `session-catalog-removals.test.ts` only covers the one-line pure function; nothing covers this effect's blast radius.
##########
apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts:
##########
@@ -202,6 +202,14 @@ export function registerRuntimeHostSessionCatalogIpc(
const modelTarget = normalizeExplicitModel(input);
return updateConfiguration(deps, sessionId, { modelTarget, thinkingLevel: null }, 'updated');
});
+ ipcMain.handle(
Review Comment:
**P2 — this leaves three channels where the PR set out to leave one, and one of them is dead.**
With `thinkingLevel: null` this new handler is word-for-word `sessions:setModel` four lines above: both call `updateConfiguration(deps, sessionId, { modelTarget, thinkingLevel: null }, 'updated')`.
After this PR `sessions:setModel` has zero production consumers — its only caller went away with `app-shell-session-settings-actions.ts`, and no e2e uses it. What remains is the ipcMain handler, `preload.ts:1956`, and `bridge-contract.d.ts:960`. `sessions:setThinkingLevel` is down to a single e2e caller (`slash-command-menu.spec.ts:251`).
The renderer converged; the protocol gained a channel and kept two orphans. Worth deleting `sessions:setModel` in this PR.
##########
apps/desktop/src/main/__tests__/model-settings-visual-contract.test.ts:
##########
@@ -0,0 +1,162 @@
+/*
Review Comment:
**P2 — this file asserts against source text, and most of what it asserts never existed on `main`.**
`readFileSync` plus regexes over source. It is fragile in the ordinary way — `assert.doesNotMatch(chatActionsSource, /^\s*if \(!isShellSurfaceOwnerActive\(sendOwner\)\) return false;$/m)` asserts that one specific line of source text is absent, so any reformatting breaks it — but the larger problem is that identifiers like `settleSessionModelSettings`, `settingsSettlementAtSubmit`, `preSendSettingsRef`, and `useSessionModelSettingsIntent` are not on `main` at all. They only ever existed in the earlier commits of this PR that were asked to be reverted.
So this pins a review round-trip as a permanent test, with no coverage of product behavior. Behavior is already covered by `session-setting-intent.test.ts` (+643) and `chat-model-switcher.test.tsx` (+189). Suggest dropping the file.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -802,6 +808,154 @@ function AppShellContent({
const activeSession = sessions.find((session) => session.id === activeId);
const activeMessageQueue = activeId ? messageQueueBySession[activeId] : undefined;
const activeDesktopSession = activeSession;
+ const modelConfigurationIntent = useSessionSettingIntent<SessionModelConfigurationIntent>({
+ catalogRevision,
+ isEqual: equalSessionModelConfigurationIntent,
+ write: async (sessionId, configuration) => {
+ const summary = await window.maka.sessions.setModelConfiguration(sessionId, {
+ ...configuration.modelTarget,
+ thinkingLevel: configuration.thinkingLevel,
+ });
+ const committed =
+ summary.llmConnectionSlug === configuration.modelTarget.llmConnectionSlug &&
+ summary.model === configuration.modelTarget.model &&
+ (summary.thinkingLevel ?? null) === configuration.thinkingLevel;
+ if (committed && configuration.changedSetting === 'model') {
+ saveComposerDefaults({ model: configuration.modelTarget });
+ }
+ return committed;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error, attempted) => {
+ if (activeIdRef.current !== sessionId) return;
+ const copy = attempted.changedSetting === 'model'
+ ? {
+ title: sessionSettingsCopy.modelFailedTitle,
+ fallback: sessionSettingsCopy.modelFallback,
+ }
+ : {
+ title: sessionSettingsCopy.thinkingFailedTitle,
+ fallback: sessionSettingsCopy.thinkingFallback,
+ };
+ showSessionError(
+ sessionId,
+ copy.title,
+ localizedShellErrorMessage(error, copy.fallback, uiLocale),
+ );
+ },
+ });
+ const permissionModeIntent = useSessionSettingIntent<PermissionMode>({
+ catalogRevision,
+ write: async (sessionId, mode) => {
+ const summary = await window.maka.sessions.setPermissionMode(sessionId, mode);
+ return summary.permissionMode === mode;
+ },
+ refreshCatalog: refreshSessions,
+ onWriteError: (sessionId, error) => {
+ if (activeIdRef.current !== sessionId) return;
+ showSessionError(
+ sessionId,
+ sessionSettingsCopy.permissionFailedTitle,
+ localizedShellErrorMessage(
+ error,
+ sessionSettingsCopy.permissionFallback,
+ uiLocale,
+ ),
+ );
+ },
+ });
+ async function setPermissionMode(mode: PermissionMode): Promise<boolean> {
+ if (mode !== 'ask' && mode !== 'bypass') return false;
+ const owner = captureComposerImportOwner();
+ const sessionId = owner.sessionId;
+ const ownsPermissionOverlay = Boolean(
+ sessionId && Object.hasOwn(permissionModeIntent.overlayBySession, sessionId),
+ );
+ const currentMode = sessionId
+ ? ownsPermissionOverlay
+ ? permissionModeIntent.overlayBySession[sessionId]
+ : sessionsRef.current.find((session) => session.id === sessionId)?.permissionMode
+ : newTaskPermissionMode;
+ if (currentMode === mode) {
+ if (sessionId && ownsPermissionOverlay) {
+ return permissionModeIntent.request(sessionId, mode);
+ }
+ return true;
+ }
+ if (
+ mode === 'bypass' &&
+ !(await toastApi.confirm({
+ title: sessionSettingsCopy.bypassConfirmTitle,
+ description: sessionSettingsCopy.bypassConfirmDescription,
+ confirmLabel: sessionSettingsCopy.bypassConfirmLabel,
+ cancelLabel: sessionSettingsCopy.bypassCancelLabel,
+ destructive: true,
+ }))
+ ) {
+ return false;
+ }
+ if (!isComposerImportOwnerActive(owner)) return false;
+ if (sessionId) return permissionModeIntent.request(sessionId, mode);
+ try {
Review Comment:
**P3 — the no-session (new task) branch lost its re-entrancy guard.**
The old implementation used a `'__global_permission_mode__'` pendingKey to block re-entry. This awaits `setNewTaskPermissionMode(mode)` directly with no in-flight protection, so a quick ask -> bypass -> ask fires concurrent persistence writes with no ordering guarantee. Minor — one more click corrects it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org