Conversation
…x menu Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Astro-Han Can you take a look？ |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed the current head. It replaces the native composer model and thinking controls with one Astryx menu and adds a Fast toggle backed by the per-model override update path. I found one P2 in the new Fast write path; see the inline comment. Please add a regression with a pending first save, a second toggle, and a switch to another Host before the queued write resumes.
I built core and ran four focused core/UI test files from emitted JS: 50/50 passed. The local UI TypeScript build failed on component-contract errors involving settledText, autoScroll, menuAnchorRef, and trailingAction; menuAnchorRef is present on the PR base, but I did not establish the cause of the complete failure set. Fresh-main merge-tree and diff check are clean. The current head has a successful label check but no hosted test result, so the gate is not green. I did not run Electron interaction or the full suite. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| if (!model) return Promise.resolve(); | ||
| const key = `${model.connectionId}\u0000${model.model}`; | ||
| const task = tailRef.current.then(async () => { | ||
| const connection = latest.current.connections.find( |
There was a problem hiding this comment.
[P2] Keep queued Fast writes bound to the Host selected at click time. The click captures model, but the deferred task looks up latest.current.connections here and later passes latest.current.host to connections.update. Reachable ordering: on session A, click Fast on and then off while the first update is pending; switch to session B on another Host before the second task starts. AppShell replaces connections with the B snapshot (app-shell.tsx:523-526). If the A connection is absent, this branch returns successfully without saving off or reporting an error. If identities overlap, the write instead targets B. Capture the Host and connection context for each queued intent, or explicitly reject a stale intent rather than silently completing it.
See the image for the result. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 5d366e845db4bf96646a5b63caf66cdbd48d7e2d. I found no substantiated P0–P3 issue in the reviewed paths.
The follow-up fix captures the model, connection snapshot, Host reference, and callbacks when Fast is clicked, before the queued task runs (use-composer-model-options.ts:58-85). This removes the previously reported path where a queued Fast write could follow the composer to a different Host. I also checked the model/effort/Fast menu, the service-tier override gate and optimistic update path, and the new copy. There is no schema or migration change.
Node 24 build:test passed after applying the repository's dependency patches; 36 focused core/UI/Desktop tests, focused Biome, ASF headers, and git diff --check passed. The branch merges cleanly with main 03237142. There are no hosted checks on this head. I did not run an Electron end-to-end session-switch race, so the click-time Host behavior is supported by code inspection, not a dynamic regression test. This is a review, not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
There was a problem hiding this comment.
Correction to my earlier review on this same commit: I missed the repository architecture check. npm run check:app-shell-hooks fails on the new hook call in AppShellContent; the hosted test check is red for the same reason. This is one P2 merge blocker, so my prior “no P0–P3” conclusion is withdrawn. The click-time Host binding assessment and other reported focused checks remain unchanged. This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| const composerOptionsHost = activeId && activeSession?.profileId && activeSession.runtimeHostId | ||
| ? { profileId: activeSession.profileId, hostId: activeSession.runtimeHostId } | ||
| : newTaskHost; | ||
| const composerModelOptions = useComposerModelOptions({ |
There was a problem hiding this comment.
[P2] The new useComposerModelOptions call fails npm run check:app-shell-hooks: the shell hook inventory rejects a new hook in AppShellContent without a feature-provider move or an explicitly justified inventory entry (per #4109). The current-head hosted test check is red, so this cannot merge as-is.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 0c93898818d18c3d3cec65ef288b3d55d083af35. I found no new substantiated P0–P3 issue in the reviewed paths.
The previous P2 architecture-check failure is fixed: the Fast write hook now sits behind useShellChatModel, uses the conversation services port, and is no longer called directly from AppShellContent. npm run check:app-shell-hooks passes. The click-time model, connection and Host binding from the prior fix remains in place. The connection update emits connection_list_changed, which the shell handles by refreshing projections. I also checked the recovery-picker test adaptation and the Astryx inventory update. No schema or migration change.
Node 24 build:test, 40 focused core/UI/Desktop tests, focused Biome, ASF headers, git diff --check, and the app-shell hook check passed. The branch merges cleanly with main ae71ab31. No hosted checks are reported on this head yet. I did not run a real Electron session-switch race or a full packaged Desktop test; these remain validation gaps. This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 0c93898818d18c3d3cec65ef288b3d55d083af35.
P2: the Fast toggle can get permanently stuck after an outside edit (use-composer-model-options.ts:73-75, inline). When the stored override differs from the composer's last saved value, the hook assumes its snapshot is stale and sends that last save as expected. The same difference appears when something else really changed the override, for example the context window or Fast in Settings, or another window. The Host rejects the write because expected doesn't match, and rememberedRef only updates on success. Every later toggle then fails with "Model parameters changed" until reload. I reproduced this by running the compiled hook against a fake Host that does the same expected check.
P3s (inline):
- The only new test covers
modelOverrideForServiceTier; the new hook and Fast row have none. - A new-task Fast write can target a different Host than the one whose connections the menu shows.
- The menu isn't keyed by session, so an in-flight pick carries over briefly.
- The model list lost search and grouping (a product question).
Session and Host pinning at click time, the Host's rejection of outdated writes, keeping the other override fields, failure handling, and the architecture boundaries all look correct.
Checks run, all passing: check:app-shell-hooks, check:asf-headers, check:renderer-architecture, check:locale-hygiene, git diff --check, build:test, a desktop renderer type-check, and targeted tests (23/23). These ran on Node 22, not the pinned 24.18.1. I didn't run the full npm test or exercise real-app keyboard navigation.
Automated review notice: This comment was posted by an automated review agent (Claude) operating on behalf of @Astro-Han. It is not an independent human review and does not replace one.
| if (!connection) throw new Error(`Connection is no longer available: ${model.slug}`); | ||
| const stored = modelOverride(connection, model.model) ?? null; | ||
| const remembered = rememberedRef.current?.key === key ? rememberedRef.current.override : undefined; | ||
| const expected = remembered !== undefined && JSON.stringify(stored) !== JSON.stringify(remembered) |
There was a problem hiding this comment.
P2: stored !== remembered is treated as "snapshot is stale", but it's also what a real outside edit looks like (for example, changing this model's context window or Fast in Settings after toggling Fast here). In that case we keep sending our old save as expected, the Host CAS rejects it, and since rememberedRef only updates on success, every later toggle fails with "Model parameters changed" until reload. I reproduced this with the compiled hook. Could we remember {before, after} and prefer after only while stored still equals before, and also drop rememberedRef on failure? A hook test for "outside edit after a composer save" would pin this down.
| }} | ||
| > | ||
| {choice?.supportsFast && props.onFastChange ? ( | ||
| <DropdownMenuCheckboxItem |
There was a problem hiding this comment.
P3: Nothing tests this Fast row, the "Model default" → undefined mapping in the effort submenu, or useComposerModelOptions. The only new test is for modelOverrideForServiceTier. Could we add a render test that opens the menu with a supportsFast choice and checks the checkbox calls onFastChange, plus a hook test for serialized writes and the stale/outside-edit expected case?
| ? (activeSession.profileId && activeSession.runtimeHostId | ||
| ? { profileId: activeSession.profileId, hostId: activeSession.runtimeHostId } | ||
| : undefined) | ||
| : (options.executorTarget |
There was a problem hiding this comment.
P3: For new tasks, the connection list comes from taskEntry.selectors.selectedHost (app-shell newTaskHost), but this Host comes from executorTarget, which is undefined when the selected Host has no resolvable project. Then preload falls back to the active runtime Host, not the one whose connections we're showing. Could this use the same Host that feeds newTaskConnections, or hide Fast when it's unknown?
| ) | ||
| : undefined); | ||
| const renderComposerOptions = (): ReactNode => ( | ||
| <ComposerOptionsMenu |
There was a problem hiding this comment.
P3: The old ChatModelSwitcher selector was keyed by session id, so a session switch reset its pending pick and closed it. This menu isn't keyed, so an in-flight model or Fast pick from session A shows on session B's trigger until it settles, and re-picking that value in B is a no-op during that window. Would key={props.activeSession?.id ?? 'new-task'} restore the old behavior?
| }} | ||
| /> | ||
| ) : null} | ||
| <DropdownMenuRadioGroup |
There was a problem hiding this comment.
P3: The previous session picker had search and per-connection groups. This submenu is a flat radio list with neither. Is that intended for users with many enabled models? If Astryx DropdownMenu has typeahead, that may be enough; otherwise it may be worth keeping search for long lists.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head's Fast-override conflict handling, click-time Host/connection capture, and Session-keyed menu state. I did not find a substantiated P0–P3 issue in the paths reviewed. The prior stale expected problem is addressed: an external override edit becomes the baseline, while a queued pick can use the immediately preceding saved value when the connection list has not refreshed.
Node 24 build:test, the six focused hook/menu tests, app-shell hook check, ASF headers, focused Biome, diff-check, and a merge-tree against current main pass locally. This head currently has no hosted checks; I did not run a real Electron multi-window/session race. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 0d96261e9b8608947100b3232c50d22cc0c26fd0.
The earlier P2 is fixed. With the compiled hook and a fake Host running the same expected check, later Fast toggles now succeed after an outside edit. The new hook tests fail on the old code, so they catch it. The session-keyed menu (composer.tsx:1879) and hiding Fast when the Host is unknown also address the earlier P3s.
P2 (inline, use-composer-model-options.ts:83): an outside edit that restores the pre-save value makes the next toggle a silent no-op. Sequence: turn Fast on in the composer, turn it off in Settings, then turn it on in the composer again.
storednow equalsremembered.before, so the hook assumes the list hasn't refreshed and usesremembered.after(on) asexpected.- The new value (on) equals
expected, so the write is skipped at:87. Nothing reaches the Host and no error is shown. rememberedRefis only cleared on failure, so every later click does the same until reload.
I reproduced this in both directions with the compiled hook. The previous code had the same hole, so it's the part of the original bug the fix doesn't close rather than a new regression. Suggested fix: drop the remembered entry once the connection list shows remembered.after, and add a test for this sequence.
P3 (inline): the fix commit also adds SOURCE-STRUCTURE.zh-CN.md, a 95-line repository-structure document that looks unrelated to this PR and may have been committed by accident.
Checks run:
check:app-shell-hooks,check:asf-headers,check:renderer-architecture: passgit diff --check: clean- Biome on the changed files: clean
- 29 targeted tests after
build:test: pass (Node 22, not 24)
Automated review notice: This comment was posted by an automated review agent (Claude) operating on behalf of @Astro-Han. It is not an independent human review and does not replace one.
| if (!connection) throw new Error(`Connection is no longer available: ${model.slug}`); | ||
| const stored = modelOverride(connection, model.model) ?? null; | ||
| const remembered = rememberedRef.current?.key === key ? rememberedRef.current : undefined; | ||
| const expected = remembered && normalizedOverride(stored) === normalizedOverride(remembered.before) |
There was a problem hiding this comment.
P2: stored === remembered.before is also what an outside edit that restores the pre-save value looks like. Example: Fast on here, then off in Settings, then on here again. In that case expected becomes remembered.after (on), the new value equals it, and the :87 early return skips the write. There's no Host call and no error, and it repeats until reload because rememberedRef is only cleared on failure. Could the remembered entry be dropped once the connection list shows remembered.after, with a test for this on → outside-off → on sequence?
| under the License. | ||
| --> | ||
|
|
||
| # Maka 源码结构 |
There was a problem hiding this comment.
P3: this 95-line repository-structure document seems unrelated to the composer options change. Was it committed by accident?
There was a problem hiding this comment.
Error submitted—this is to help me understand the structure of Maka.
…-menu Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # apps/desktop/src/renderer/application/contracts/conversation-copy.ts # docs/astryx-surface-file-inventory.md
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 565f8f0a3864194fff210dc83657f5b364c68df9 (incremental from 0d96261e). Thanks for the follow-up.
- The earlier P2 is fixed: the remembered Fast save is dropped once the connection list shows the saved value, and the two new tests fail against the previous hook and pass here. The reverse sequence (on → composer off → Settings on → composer off) also works. The stray
SOURCE-STRUCTURE.zh-CN.mdis gone. - No regressions found for two quick toggles, a toggle queued across a Host switch, or a model switch while a save is pending.
Three P3s (inline for the first two):
- The remembered save is only cleared while the composer is still on the same model/Host, so switching Session/model before the refresh lands (then restoring the value in Settings), or a refresh that merges with an outside revert, still leaves the next toggle a silent no-op until reload.
- After a quick on→off, the post-save cleanup can drop the memory early; a stale refresh then briefly shows Fast on and the next click surfaces a "Model parameters changed" toast (previously a correct no-op). Narrow timing window; self-heals on the next refresh.
- The new-task Fast wiring in
use-shell-chat-model.tsis not reachable today: with no active Session the shell always suppliesexecutorPicker, so the composer takes the existing picker branch rather than the new menu.
Checks run locally: ASF headers, git diff --check, merge-tree vs main, app-shell hooks, renderer architecture, Astryx surface and Windows inventories, locale hygiene, UI and Desktop typecheck, Biome on changed files; targeted Desktop 9/9, UI 28/28, core 14/14. Not run: Electron, full suite.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| if (!model || !host || writeKey(host, model) !== key) return undefined; | ||
| const connection = options.connections.find( | ||
| (candidate) => candidate.connectionId === model.connectionId && candidate.slug === model.slug, | ||
| ); |
There was a problem hiding this comment.
P3: this only clears the remembered save while the composer is still on the same model and Host. If the user switches Session/model before the refresh lands and Settings then restores the original value, or the refresh merges with an outside revert and never shows the saved value, the next toggle for that model is still a silent no-op until reload. Keying the memory by connection+model and clearing it whenever the list shows any value for that key would cover both.
| }); | ||
| rememberedRef.current = { key, before: expected, after: saved }; | ||
| // The refresh may have landed before the save resolved. | ||
| forgetIfShown(); |
There was a problem hiding this comment.
P3 (narrow): after a quick on→off, this post-save cleanup can drop the memory before a stale refresh arrives; if that refresh briefly shows Fast on and the user clicks off, the Host rejects the write with a "Model parameters changed" toast, where the previous code did a correct no-op. Self-heals on the next refresh.
github-actions
Bot
added
effort/XL
|
Thanks for sticking with this, and for the quick fixes on the Fast toggle. The menu itself looks good: it's built entirely from Astryx The goal from #5787 is for the new menu to replace the existing composer pickers rather than sit next to them. At this head it only appears in an open native Session (and expanded WorkHub). The home / new-task composer still renders Could you:
Happy to take another look once these are sorted out. Thanks again! 中文版感谢一直跟进，也感谢很快修好了 Fast 开关的问题。菜单本身做得不错：完全用 Astryx 的 #5787 的目标是让新菜单替换现有的输入框选择器，而不是和它们并存。在当前版本里，它只在已打开的普通会话（以及 WorkHub 展开态）中出现；首页/新建任务的输入框仍然是 能否请你：
这几点处理好之后，我会尽快再看一遍。再次感谢！ |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 565f8f0, including the Fast-toggle follow-up and its merge from main. The stray repository document is removed, and the new same-model tests pass. I independently traced the two remaining Fast-state edge cases already called out inline in review 5361270924, so I am not duplicating line comments:
- P3:
shownOverriderequires the currently selected Host/model to match the remembered key (apps/desktop/src/renderer/features/conversation/controller/use-composer-model-options.ts:55-60). If the user switches away before the list shows the saved value, then Settings restores the old value, the remembered entry survives and the next Fast click can be silently skipped at lines 114-120. - P3: after a quick on→off, post-save
forgetIfShown()at lines 128-130 can clear the remembered entry while the list still shows the old off value. A late on refresh then makes a subsequent off click use a staleexpectedand fail with the model-parameters-changed toast. The existing tests do not exercise this order.
The new-task menu path also remains gated by an executorPicker supplied when there is no active task (apps/desktop/src/renderer/features/conversation/model/executor-composer.ts:34-38, packages/ui/src/composer.tsx:2515), as the same-head review notes. I found no additional substantiated finding in the increment.
Node 24 npm ci, npm run build:test, Desktop typecheck, and the 5 focused Fast-toggle tests passed; git diff --check is clean. Fresh main 0aa2707 still conflicts in docs/astryx-surface-file-inventory.md. The current hosted test rerun is in progress; its prior attempt failed a WorkHub E2E model-button assertion, whose cause I have not isolated. This head is not ready for merge. I did not run packaged Electron, the full suite, or a live Settings-to-composer interaction.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current increment. This revision removes the composer Fast toggle rather than changing its save/refresh timing: the menu now exposes model and thinking effort only (packages/ui/src/composer-options-menu.tsx:50), the composer callback and Desktop override-write controller are removed, and ChatModelChoice no longer projects Fast state (packages/core/src/chat-model-choice.ts:38). The Settings provider capability editor remains the place to configure the Fast service tier. The two Fast-memory findings on the previous head no longer apply to this composer path. I found no new substantiated P0-P3 issue in the inspected increment; this is a scope change, not a validation of the former toggle behavior.
Node 24 core/UI builds and 15 focused tests pass. This head has no hosted checks, and merging with current main conflicts in docs/astryx-surface-file-inventory.md; it is not merge-ready. I did not run a real Desktop/Host session, the complete test suite, or a visual menu check.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
…-menu Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # docs/astryx-surface-file-inventory.md
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current merge commit and effective nine-file PR diff against its merged main parent. The only manual merge resolution is the Astryx inventory total (docs/astryx-surface-file-inventory.md:9); the paths file includes the new composer options menu, and the inventory checker confirms 316 files. The Composer Fast control remains removed from this menu while the model/effort options and recovery path remain. I found no substantiated new P0–P3 issue in this increment.
Node 24 UI build and seven focused picker/recovery tests pass. A fresh-main merge-tree and diff check are clean. This head has no hosted checks, so I cannot treat the repository gate as green or recommend merge yet. I did not run packaged Desktop, real Host interactions, visual checks, or the full test suite.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current three-file increment. The Composer options trigger now places the switch action and selected model in Astryx’s label (packages/ui/src/composer-options-menu.tsx:134-143), matching the accessible name when visible children are present. The focused test checks that name, and the WorkHub E2E assertion now expects the menu popover rather than a listbox. I found no substantiated P0–P3 issue in the inspected change.
Node 24 UI build and seven focused picker/recovery tests pass; fresh-main merge-tree and diff check are clean. This head still has no hosted checks, so the repository gate is not green. I did not run the WorkHub Electron E2E, visual accessibility check, or full suite.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
This branch has not been deployed
3 participants
Summary
The composer footer currently shows the model and the thinking level as
separate selectors. For native Maka sessions, this PR replaces them with a
single Astryx
DropdownMenu:DropdownMenuSubMenu+DropdownMenuRadioGroup) for thethinking level, including "Model default".
prompt-cache warning stays as the first row when the session has history.
service tier (
supportsCustomFastServiceTier). It writesmodelOverrides[model].serviceTierthrough the existing connection updatepath, using the same optimistic
expectedcheck as Settings.(when not default) and Fast (when on) in muted text, for example
Claude Opus 5.5 1M Medium Fast.The composer model-picker recovery handle (
openModelPicker) now opens thismenu.
The menu is built only from existing Astryx components. Where Astryx lacks
something, the gap is listed under Remaining work rather than worked around.
The one stopgap:
DropdownMenuSubMenuhas noendContent, so the row's currentvalue is rendered inside its
label.Refs #5787
Remaining work (draft)
The replacement is intentionally partial. The old selectors are still used in
these cases:
ExecutorModelPicker+ its thinkingselector) still use the executor popover.
DropdownMenuonly supportspopoverin compound mode.ChatModelSwitcher,NewChatModelPicker,ThinkingLevelSelectorand
ModelChipStaticonce every case above uses the new menu.Astryx capabilities needed first (to be raised before building our own):
endContent/valueonDropdownMenuSubMenu.SelectorhashasSearch).DropdownMenuSubMenuin Electron (it is gated on(hover: hover)with a 150 ms delay).Verification
packages/ui:node --test dist/__tests__/composer-model-picker-recovery.test.js dist/__tests__/executor-model-picker.test.js→ 22 pass, 0 failpackages/core:node --test dist/__tests__/model-thinking.test.js dist/__tests__/llm-connections.test.js→ 28 pass, 0 failapps/desktop:tsc -p tsconfig.renderer.json --noEmit→ cleanpackages/corefiles; the pre-commit hooks (ASFheader audit, protocol epoch guard) pass.
Not run: the full repository test suite, repository-wide lint, and a manual
check in the Electron app (no screenshot attached yet).
AI use
Tool(s) and scope: Cursor agent (Claude). It drafted the unified menu component,
the Fast override helper and the desktop wiring, and updated the tests. I
reviewed and directed the design, including removing the context-window
setting and the custom hover workaround so that only Astryx components are
used.
Checklist
Does this PR entail a change in behavior?