Skip to content

refactor(composer): make the + menu one entry with one kind of row - #3198

Merged
Astro-Han merged 10 commits into
apache:mainfrom
Astro-Han:refactor/composer-plus-menu-single-rhythm
Aug 20, 2026
Merged

refactor(composer): make the + menu one entry with one kind of row#3198
Astro-Han merged 10 commits into
apache:mainfrom
Astro-Han:refactor/composer-plus-menu-single-rhythm

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

The composer's + menu offered Plan, Swarm and Graph as three DropdownMenuCheckboxItems. That was two problems, and they are not the same problem.

The rows did not share the menu's rhythm. DropdownMenuCheckboxItem never applies the menu's sm density, so its rows stood 36px against the 28px action rows above them, and its marker column pushed the mode icons 28px to the right of the action icons.

Swarm and Graph are not independent. They are the two values of one Session field, orchestrationMode, and a run carries one orchestration — so turning one on turned the other off, with nothing said about it.

What the menu is now

Plan keeps its checkbox. It is a Session field of its own (collaborationMode), a temporary collaboration excursion that Runtime ends by itself on approval or abandonment, and it really is independent of the other two: a Session can be planning with Swarm armed for afterwards, and this PR has a story for that state.

Swarm and Graph are one DropdownMenuRadioGroup — the control Astryx directs a one-of-N choice to, and the one that carries the relationship. The rows are announced as a named set, so picking one moves the set instead of silently moving a row the user is not on.

No neutral row. default is not a third way to fan a turn out, it is the absence of one, so the group holds no selection at rest — DropdownMenuRadioGroup takes value: string | undefined for exactly this. The way back is the selected row itself: a radio item reports its value on every activation, so re-activating the chosen one empties the group. A native radio does not deselect; an empty group is a state the role expresses, and the change lands on the row the user is standing on rather than on the row they left.

Density and alignment. Both selectable item types draw their own control at the row's start, so the selection mark moves to endContent as Astryx's own check indicator — what its Selector puts on a chosen option — and a rule scoped to this panel suppresses the box and the circle. Five rows at 28px, every icon on one x.

The + and permission menus also join the palette seam in astryx-mount.css. Astryx renders these panels in place rather than portaling them, so without the seam they painted from the neutral theme: the menu's own accent was near-black while the footer's mode mark beside it was Maka blue, for the same state.

Upstream ask

Let a selectable menu item choose its indicator, and the endContent + scoped-CSS pair here goes away.

Before / after

Shot live in Storybook (Product/Shell Official AppShell), same viewport, each panel cropped to its own bounds and composited at 1:1 pixel scale — the UI is never resampled.

image

How the design got here

Two things I had wrong, both corrected in review. The diff shows the destination, so the argument is worth stating.

The modes are not one choice. The first version fused Plan and orchestration into a single sessionMode prop and one radio group of four. That is wrong at the authority: Runtime ends a Plan by writing collaborationMode alone (session-manager.ts), and resolves the Plan/orchestration overlap by stripping the subagent and agent-graph tools while planning (plan-mode.ts). It never writes the two together, and nothing validates the pair — plan + swarm is a legal Session state and was reachable before this PR. Fusing them also destroyed the orchestration default the plan was written to execute with: Runtime leaves Plan by itself after approval, and Swarm never came back. So the two fields get two props, two IPC channels and two writers, and the write that leaves Plan touches nothing else.

default is not a fourth option. It has no behaviour of its own — it is how the field spells "none". So it has no row, and the group carries it as no selection.

Verification

  • Measured live in Storybook (DefaultLayout, 1440×900 at 2× DPR). Before: panel 144×180, rows 28/28/36/36/36, mode icons at x=560 against action icons at x=532. After: panel 144×167, all five rows 28px, every icon at x=532, no indicator painted, nothing checked, and the orchestration pair inside one role="group" labelled "Orchestration mode". PlanAndSwarmModeOn reads back one checked menuitemcheckbox, one checked menuitemradio, and two footer marks.
  • packages/ui/src/__tests__/composer-plus-menu.test.tsx: the divider renders only with a row above it; Plan is one menuitemcheckbox and the orchestration options are two menuitemradio in one named group; nothing is checked at rest; Plan and Swarm are checked together.
  • apps/desktop/src/main/__tests__/session-mode-ipc-main.test.ts: the Plan write carries collaborationMode alone, the orchestration write carries its own field alone, a Plan Session keeps the orchestration default it was carrying, and an unknown value is refused rather than persisted.
  • Each of those was checked against a deliberately broken version — making the Plan handler carry orchestrationMode again turns two tests red, and restoring the unconditional divider turns another red.
  • Storybook render smoke (137 stories), npm run astryx:surface-inventory, all four apps/desktop tsconfig projects, the @maka/ui suite, npm run format:check and biome lint on the touched files — clean.
  • Not run: repository-wide suites (left to CI).

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code (Opus) wrote the diff and the commit messages, and measured the result in Storybook. The problem framing, the "one entry, one kind of row" constraint, the rejection of the fused mode field, and the rejection of the neutral row are the human contributor's. Each commit carries a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Not a fast-path candidate

This changes material user-visible behaviour (the + menu the user touches on every turn) and two public contracts (Composer's props in packages/ui, and bridge-contract.d.ts), so it needs independent human review per AGENTS.md. It touches none of the protected architecture areas — Runtime Host, @maka/eval, the Harbor/Pier executor and maka eval are untouched — and it reverts cleanly as one branch, but neither of those makes it low impact. The CodeRabbit and Qodo reviews on this PR do not count as independent human review.

Review focus

The two mode fields and their two writers in app-shell.tsx: applyPlanMode carries the Plan-exit confirmation and the getPlanState gates, applyOrchestrationMode is a plain write, and neither touches the other's field. In composer.tsx, the orchestration group's onChange is where "activate the chosen row to leave it" lives.

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • Review skipped - (🔄 Check again to try again)

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c728c1f1-a992-402e-9d1c-251b6864617a

📥 Commits

Reviewing files that changed from the base of the PR and between e050cba and c759107.

📒 Files selected for processing (3)
  • apps/desktop/src/main/__tests__/session-mode-ipc-main.test.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • packages/ui/src/__tests__/composer-plus-menu.test.tsx

Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Summary

This PR replaces separate Plan, Swarm, and Graph toggles with one mutually exclusive ComposerSessionMode radio group. It adds a neutral default mode and Goal selection.

The shared session-mode mapping becomes the source of truth for UI state, slash commands, persisted fields, and IPC updates. The IPC boundary validates legal field pairs and persists both fields atomically. This removes parallel mode-update paths.

This is the smallest coherent solution identified in the current diff. The shared mapping and validation are necessary to keep UI, persistence, and IPC behavior consistent. The unified state also removes independent toggle combinations.

The PR standardizes menu density, selection indicators, palette styling, divider rendering, and radio-group accessibility labels. It removes mode-specific copy that the unified mode contract no longer needs.

The deleted PlanAndSwarmModeOn story and removed toggle-specific copy reduce maintenance without reducing behavior coverage. Focused tests cover all four modes, persisted-field mappings, invalid IPC payloads, atomic updates, and divider behavior.

Reported validation includes Storybook measurements, typechecks, formatting, linting, and affected tests. Repository-wide suites were not run. Required-check status remains unverified from direct evidence.

Complexity delta

  • Authorities: Removes duplicated mode mappings and separate IPC handlers. Adds session-mode.ts as the shared mode authority.
  • State: Replaces three independent mode states with one ComposerSessionMode.
  • Branches: Removes illegal toggle combinations. Adds legal-pair validation and Plan-exit transition handling.
  • Configuration: Replaces separate field updates with one atomic setSessionMode patch.
  • Public surface: Removes separate Composer callbacks and bridge methods. Adds ComposerSessionMode, SessionModeFields, and unified mode callbacks.
  • Tests: Adds focused mode, IPC, and menu coverage. Removes one unreachable story.

Total maintenance complexity decreases. The added shared authority, validation, and focused tests are justified by the removed parallel paths.

Potential simplification is limited. The removed story and obsolete copy are already deleted. Further deletion could weaken coverage of IPC validation, atomic persistence, or divider behavior.

Review-relevant risks

  • The Composer props and preload bridge contract change. Consumers of the removed props or methods may require updates. Material public-contract changes require independent human review under repository policy.
  • Session-mode persistence and IPC behavior change. Invalid combinations are rejected, and Plan transitions can require confirmation or stop after failure or cancellation. Material user-visible behavior changes require independent human review under repository policy.
  • The Composer UI, localization, and accessibility behavior change. Material user-visible changes require independent human review under repository policy.
  • Required-check status is unverified from direct evidence. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

The change replaces separate Plan, Swarm, and Graph state with one ComposerSessionMode. The desktop bridge updates both persisted mode fields atomically. The composer, session transitions, localized copy, stories, styles, and tests now use the unified mode contract.

Changes

Session mode unification

Layer / File(s) Summary
Session mode contract and atomic IPC
apps/desktop/src/shared/session-mode.ts, apps/desktop/src/preload/..., apps/desktop/src/main/...
Defines four session modes, validates legal field pairs, exposes setSessionMode, and persists both fields in one update. Tests cover valid modes, invalid combinations, and malformed payloads.
Composer session-mode contract and menu
packages/ui/src/composer.tsx, packages/ui/src/components.tsx, packages/ui/src/conversation-copy.ts, packages/ui/src/__tests__/*, apps/desktop/src/renderer/styles/*
Replaces independent mode controls with a four-option radio menu. Shared pending and disabled states, labels, selection indicators, and scoped menu styles support the new contract.
Desktop session-mode orchestration
apps/desktop/src/renderer/app-shell.tsx
Tracks one pending mode transition, derives the active mode from session fields, applies atomic changes, maps modes into new-chat payloads, and updates Plan, Swarm, and Graph status handling.
Mode copy and integration fixtures
apps/desktop/src/renderer/locales/shell-copy.ts, apps/desktop/stories/app-shell.stories.tsx
Updates English and Chinese state messages and changes composer stories to use sessionMode.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to c7591

The PR makes the composer modes a single four-way choice and applies both Session fields together, preventing invalid combinations and transient split states. No actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Composer
  participant AppShell
  participant PreloadBridge
  participant SessionModeIPC
  participant SessionConfiguration
  Composer->>AppShell: onSessionModeChange(mode)
  AppShell->>PreloadBridge: setSessionMode(sessionId, mode fields)
  PreloadBridge->>SessionModeIPC: sessions:setSessionMode
  SessionModeIPC->>SessionConfiguration: persist both mode fields
  SessionConfiguration-->>AppShell: updated session summary
  AppShell-->>Composer: active mode and transition state
Loading

Possibly related PRs

Suggested reviewers: m4n5ter, likun666661, hqhq1025

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Ai Use Disclosure ⚠️ Warning The PR discloses Claude Code authored the diff; five of six PR commits have Generated-by: Claude Code, but c759107 lacks the required trailer. Add a standalone Generated-by: Claude Code trailer to c759107 and ensure it survives squash or amend. See Human ownership and AI attribution in CONTRIBUTING.md.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: refactoring the Composer + menu into one consistent menu entry structure.
Description check ✅ Passed The description covers the required summary, verification, AI use, checklist, behavior change, and review focus sections with specific implementation details.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@Astro-Han
Astro-Han marked this pull request as ready for review August 18, 2026 09:21
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Unify composer session modes into one exclusive menu

🐞 Bug fix ✨ Enhancement 🕐 40+ Minutes

Grey Divider

AI Description

• Replaces independent toggles with one exclusive Default, Plan, Swarm, or Graph choice.
• Sequences field mutations to stop invalid partial transitions when Plan exit is refused.
• Unifies menu row density, indicators, palette tokens, stories, and localized copy.
Diagram

graph TD
  Select["Select session mode"] --> Radio["Composer radio group"] --> Writer["App shell writer"] --> Leaving{"Leaving Plan?"}
  Leaving -->|No| Apply["Apply target mode"] --> State["Session state"] --> Radio
  Leaving -->|Yes| Confirm{"Exit accepted?"}
  Confirm -->|Yes| Apply
  Confirm -->|No| State
Loading
High-Level Assessment

The PR's discriminated session-mode value and app-shell-owned sequencing are the appropriate approach. Retaining independent booleans with validation would preserve invalid intermediate states, while introducing a new persisted unified field would require unnecessary schema and runtime migrations when the existing two fields can be safely projected.

Files changed (8) +259 / -257

Enhancement (3) +134 / -132
composer.cssUse check indicators for plus-menu radio rows +18/-0

Use check indicators for plus-menu radio rows

• Suppresses Astryx's default radio-circle indicator only within the composer plus panel. Selected rows instead display the theme-provided check passed through 'endContent'.

apps/desktop/src/renderer/styles/composer.css

composer.tsxReplace mode checkboxes with one radio group +104/-114

Replace mode checkboxes with one radio group

• Introduces the four-value 'ComposerSessionMode' contract and replaces separate Plan, Swarm, and Graph checkbox props with one value, pending state, disabled reason, and change callback. Renders a uniform Default/Plan/Swarm/Graph radio group and limits the footer readout to the single active non-default mode.

packages/ui/src/composer.tsx

conversation-copy.tsLocalize the unified session-mode selector +12/-18

Localize the unified session-mode selector

• Adds accessible group and Default-option labels in Chinese and English. Removes obsolete enable and disable strings from the former checkbox controls.

packages/ui/src/conversation-copy.ts

Bug fix (2) +104 / -69
app-shell.tsxCoordinate exclusive session-mode transitions +102/-69

Coordinate exclusive session-mode transitions

• Replaces three new-chat mode booleans with one 'ComposerSessionMode' value and projects existing session fields into that value. Adds a single sequenced writer that stops when Plan exit fails or is refused, preventing partially applied and unsupported mode combinations.

apps/desktop/src/renderer/app-shell.tsx

astryx-mount.cssExtend Astryx palette tokens to composer menus +2/-0

Extend Astryx palette tokens to composer menus

• Adds the composer plus menu and permission-mode control to the Astryx palette seam so in-place popovers use Maka surface and accent colors.

apps/desktop/src/renderer/styles/astryx-mount.css

Refactor (2) +14 / -36
shell-copy.tsConsolidate session-mode disabled-state messages +13/-36

Consolidate session-mode disabled-state messages

• Replaces duplicated Plan, Swarm, and Graph lock messages with shared session-mode copy in Chinese and English. Mode-specific failure and status messages remain unchanged.

apps/desktop/src/renderer/locales/shell-copy.ts

components.tsxExport the composer session-mode type +1/-0

Export the composer session-mode type

• Re-exports 'ComposerSessionMode' from the UI package entry point for desktop-shell consumers.

packages/ui/src/components.tsx

Tests (1) +7 / -20
app-shell.stories.tsxUpdate stories for exclusive session modes +7/-20

Update stories for exclusive session modes

• Migrates composer fixtures and mode stories to the unified 'sessionMode' contract. Removes the unreachable combined Plan-and-Swarm story.

apps/desktop/stories/app-shell.stories.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
apps/desktop/src/renderer/app-shell.tsx (1)

2264-2277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Route slash-command mode changes through setSessionMode

When collaborationMode is plan, /swarm on and /graph on update only orchestrationMode, leaving Plan enabled. Use setSessionMode so the existing Plan-exit confirmation and invariant apply to these commands.

packages/ui/src/composer.tsx (1)

1357-1371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the @maka/ui styling ownership explicit. Composer owns the markup, but the radio-indicator override exists only in apps/desktop/src/renderer/styles/composer.css. packages/ui/src/styles.css does not contain the rule and no current host imports it. Move the rule to the shared stylesheet and import that stylesheet in each host. Otherwise, document that this Composer mode UI is desktop-only.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 65055659-dac3-4a76-a9af-540ed407cfea

📥 Commits

Reviewing files that changed from the base of the PR and between 49f8675 and edb7d40.

📒 Files selected for processing (8)
  • apps/desktop/src/renderer/app-shell.tsx
  • apps/desktop/src/renderer/locales/shell-copy.ts
  • apps/desktop/src/renderer/styles/astryx-mount.css
  • apps/desktop/src/renderer/styles/composer.css
  • apps/desktop/stories/app-shell.stories.tsx
  • packages/ui/src/components.tsx
  • packages/ui/src/composer.tsx
  • packages/ui/src/conversation-copy.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.

Comment thread apps/desktop/src/renderer/app-shell.tsx Outdated
@qodo-code-review

qodo-code-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Slash commands bypass authority ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new projection always displays Plan when both persisted fields are set, but /swarm on and
/graph on still call the orchestration-only setters, so they can create that combined state while
the menu remains on Plan and the corresponding status command reports enabled. The refactor
therefore does not enforce its one-mode invariant across an existing mode-change entry point.
Code

apps/desktop/src/renderer/app-shell.tsx[R1335-1339]

+  const activeSessionMode: ComposerSessionMode = activeId
+    ? (activeSessionForView?.collaborationMode ?? 'agent') === 'plan'
+      ? 'plan'
+      : (activeSessionForView?.orchestrationMode ?? 'default') === 'swarm'
+        ? 'swarm'
Relevance

●●● Strong

Accepted history repeatedly flags cross-entry-point invariant gaps and inconsistent persisted-state
authority.

PR-#2384
PR-#2523

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
activeSessionMode gives collaboration Plan precedence, whereas the slash status paths inspect
orchestration directly. The persistent slash handlers still invoke setSwarmMode/setGraphMode,
whose IPC calls update only orchestrationMode, while only the new setSessionMode path exits Plan
before enabling orchestration.

apps/desktop/src/renderer/app-shell.tsx[1083-1089]
apps/desktop/src/renderer/app-shell.tsx[1122-1128]
apps/desktop/src/renderer/app-shell.tsx[1160-1175]
apps/desktop/src/renderer/app-shell.tsx[1335-1342]
apps/desktop/src/renderer/app-shell.tsx[2264-2286]
apps/desktop/src/renderer/app-shell.tsx[2307-2329]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Active-session `/swarm on` and `/graph on` bypass `setSessionMode`, allowing Plan plus orchestration to remain persisted while the new one-value UI masks the orchestration mode.

## Issue Context
Consolidate duplicated authority: route persistent slash-command mode changes through `setSessionMode` instead of adding another reconciliation branch. Adjust the consolidated function to return success so existing command toasts retain their current contract; this reuses the new seam and introduces no additional state or configuration.

## Fix Focus Areas
- apps/desktop/src/renderer/app-shell.tsx[1160-1180]
- apps/desktop/src/renderer/app-shell.tsx[2264-2329]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Mode change crosses sessions ✓ Resolved 🐞 Bug ≡ Correctness
Description
setSessionMode awaits one helper and then calls the next helper, but every helper re-reads
activeIdRef.current; switching sessions during the Plan-exit confirmation can therefore disable
Plan on session A and enable Swarm/Graph on session B. The requested transition is split across two
unrelated sessions.
Code

apps/desktop/src/renderer/app-shell.tsx[1163]

+    if (current === 'plan' && !(await setPlanMode(false))) return;
Relevance

●●● Strong

PR #2523 explicitly accepts binding async work to a stable session owner before execution.

PR-#2523

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The added chain awaits setPlanMode(false) before invoking another setter, while setPlanMode,
setSwarmMode, and setGraphMode independently resolve their target from activeIdRef.current.
The Plan helper can remain suspended on toastApi.confirm, making an A→B switch reproducible; past
PR #2523 documents the same repository-specific failure pattern of awaiting before binding execution
to a stable session owner.

apps/desktop/src/renderer/app-shell.tsx[1004-1009]
apps/desktop/src/renderer/app-shell.tsx[1030-1039]
apps/desktop/src/renderer/app-shell.tsx[1069-1075]
apps/desktop/src/renderer/app-shell.tsx[1160-1175]
PR-#2523

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A composed session-mode transition can mutate two different sessions because each awaited helper resolves its target from the latest `activeIdRef.current`.

## Issue Context
Consolidate ownership rather than adding another state path: capture the transition owner once in `setSessionMode` and pass that explicit owner through every underlying mutation. The existing helpers can be adapted to accept that owner; no new public API or configuration is needed.

## Fix Focus Areas
- apps/desktop/src/renderer/app-shell.tsx[1004-1179]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Failed transition loses mode ✓ Resolved 🐞 Bug ☼ Reliability
Description
Transitions between orchestration and Plan persist the first field reset before attempting the
second write, so if the second IPC call fails the session is left in default rather than either
its prior or requested mode. For example, Swarm→Plan first disables Swarm and then a rejected
getPlanState/collaboration write permanently loses the prior selection.
Code

apps/desktop/src/renderer/app-shell.tsx[R1164-1167]

+    if (next === 'plan') {
+      if (current === 'swarm' && !(await setSwarmMode(false))) return;
+      if (current === 'graph' && !(await setGraphMode(false))) return;
+      await setPlanMode(true);
Relevance

●● Moderate

Accepted history favors failure-path fixes, but rollback semantics are a broader design choice
without close precedent.

PR-#1755
PR-#3147

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The added Plan branch awaits an orchestration reset and then separately invokes setPlanMode(true).
The first helper has already persisted and refreshed the default orchestration state before
setPlanMode can return false on getPlanState, an active-execution refusal, or either IPC/refresh
exception; the main handlers confirm these are separate updateConfiguration operations.

apps/desktop/src/renderer/app-shell.tsx[1018-1025]
apps/desktop/src/renderer/app-shell.tsx[1043-1059]
apps/desktop/src/renderer/app-shell.tsx[1083-1098]
apps/desktop/src/renderer/app-shell.tsx[1164-1168]
apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts[153-173]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A two-field mode transition commits its first mutation even when the second mutation fails, losing the session's previous mode.

## Issue Context
The two existing renderer IPC calls cannot provide atomicity, and local rollback is not sufficient because Plan exit may also abandon a proposal irreversibly. Reuse the main-process `updateConfiguration` seam to expose one combined session-mode mutation after any required confirmation; this necessarily adds one bridge method, but avoids adding renderer rollback state/branches and is the smallest authority capable of committing both persisted fields together.

## Fix Focus Areas
- apps/desktop/src/renderer/app-shell.tsx[1018-1059]
- apps/desktop/src/renderer/app-shell.tsx[1160-1180]
- apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts[153-173]
- apps/desktop/src/preload/bridge-contract.d.ts[593-594]
- apps/desktop/src/preload/preload.ts[1485-1489]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

4. Orphan divider when no leading rows ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new <DropdownMenuDivider /> before the session-mode radio group is rendered unconditionally
whenever props.onSessionModeChange is truthy, without checking whether onPickAttachments or
mentionSkills produced any row above it. If a host supplies only onSessionModeChange (e.g.
mentionSkills empty/undefined and attachments disabled), the + menu opens with a leading divider as
its first visible element.
Code

packages/ui/src/composer.tsx[R1633-1636]

+                    {props.onSessionModeChange ? (
+                      <>
+                        <DropdownMenuDivider />
+                        <DropdownMenuRadioGroup
Relevance

●●● Strong

This is a deterministic conditional-rendering defect; history accepts UI markup and separator
robustness fixes.

PR-#2951
PR-#2191

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The divider condition only checks props.onSessionModeChange, independent of whether the
attachment/skill items above rendered anything, so an orphan leading separator is reachable whenever
a caller wires session mode without the other two optional entries.

packages/ui/src/composer.tsx[1606-1659]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `DropdownMenuDivider` inserted before the session-mode radio group in the + menu is rendered whenever `props.onSessionModeChange` is truthy, regardless of whether either the attachments item (`props.onPickAttachments`) or the skills item (`props.mentionSkills`) rendered a preceding row. If both are absent, the divider becomes an orphan leading separator with nothing to divide.

## Issue Context
Currently in production wiring (`apps/desktop/src/renderer/app-shell.tsx`) `mentionSkills` is normally passed alongside `onSessionModeChange`, so the orphan case is not hit today, but the component itself does not guard against it, so any future or alternate caller (or Storybook story) that wires only session mode will show a leading divider.

## Fix Focus Areas
- packages/ui/src/composer.tsx[1606-1636]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Radio items lose enable/disable description ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The old checkbox items provided aria-description with an enable/disable hint
(copy.enablePlanMode/copy.disablePlanMode) whenever no disabled reason applied; the new
DropdownMenuRadioItems only pass aria-description={props.sessionModeDisabledReason}, which is
undefined in the common enabled case, dropping the per-item descriptive text screen readers
previously announced.
Code

packages/ui/src/composer.tsx[R1650-1654]

+                              endContent={mode.id === sessionMode ? (
+                                <SelectionMark state="checked" size="sm" />
+                              ) : undefined}
+                              aria-description={props.sessionModeDisabledReason}
+                            />
Relevance

●● Moderate

Accessibility and descriptive-copy regressions are plausible, but available composer history lacks a
close accepted precedent.

PR-#2951
PR-#2202

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Proper ARIA radio semantics (role=menuitemradio + aria-checked, conventionally set by the underlying
primitive from group value) already communicate selection state, so losing the enable/disable text
is a minor, non-blocking readability regression rather than a functional break; flagged as
informational only.

packages/ui/src/composer.tsx[1633-1656]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Each session-mode `DropdownMenuRadioItem` now only sets `aria-description` from `props.sessionModeDisabledReason`, which is undefined outside of disabled/pending states, unlike the previous per-item enable/disable description text.

## Issue Context
Radio-group semantics (checked/unchecked) convey selection, but assistive-tech users may still benefit from an explicit description of what selecting the item does, similar to the permission-mode radio menu pattern.

## Fix Focus Areas
- packages/ui/src/composer.tsx[1643-1654]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Web pages:
  +18 more
Review mode: 🧠 Deep: This changes a shared Composer API and coordinates multi-field session state across several asynchronous mode-switch paths, with many independent UI, accessibility, localization, and state-transition edit sites where subtle regressions are plausible.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread apps/desktop/src/renderer/app-shell.tsx Outdated
Comment thread apps/desktop/src/renderer/app-shell.tsx Outdated
Comment thread apps/desktop/src/renderer/app-shell.tsx Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b27bf9b5-df0d-439c-9f02-1bc3f8002fdf

📥 Commits

Reviewing files that changed from the base of the PR and between edb7d40 and 55ed865.

📒 Files selected for processing (10)
  • apps/desktop/src/main/__tests__/session-mode-ipc-main.test.ts
  • apps/desktop/src/main/__tests__/session-mode.test.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/preload/preload.ts
  • apps/desktop/src/renderer/app-shell.tsx
  • apps/desktop/src/renderer/locales/shell-copy.ts
  • apps/desktop/src/renderer/session-mode.ts
  • packages/ui/src/__tests__/composer-plus-menu.test.tsx
  • packages/ui/src/composer.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/desktop/src/renderer/app-shell.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.

Comment thread apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts Outdated
Comment thread apps/desktop/src/shared/session-mode.ts Outdated
@Astro-Han
Astro-Han force-pushed the refactor/composer-plus-menu-single-rhythm branch from 9f57be1 to e050cba Compare August 18, 2026 12:15
@Astro-Han
Astro-Han requested review from hqhq1025 and removed request for jackwener August 18, 2026 19:28
@Astro-Han
Astro-Han force-pushed the refactor/composer-plus-menu-single-rhythm branch from c05d4c7 to c759107 Compare August 19, 2026 04:30
Comment thread apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts Outdated

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

Requesting changes on head c759107.

The radio-group consolidation and atomic Desktop configuration patch are sound. One non-duplicate authority gap remains: the legal mode-pair check exists only in Desktop IPC, while Runtime Host session.create / session.configuration.update and the supported CLI can still persist plan + swarm/graph.

This changes the disposition of the resolved legacy-pair discussion. The remaining problem is not only historical normalization: new illegal pairs can still be created after this PR, and Desktop then masks them as Plan while slash-command status and mutation disagree.

Please enforce the invariant at the shared Runtime Host create/update authority and cover a non-Desktop client path. Existing-record normalization can remain separately tracked if desired.

No other P0–P2 issue was found. All GitHub checks are green; the focused Desktop build, renderer typecheck, mode/IPC tests, and Composer menu tests pass locally.

简体中文

建议在 head c759107 上 Request changes。

Radio group 收敛和 Desktop 原子 configuration patch 本身是合理的。目前还剩一个未与现有 review 重复的 authority 缺口:合法 mode pair 只在 Desktop IPC 校验,而 Runtime Host 的 session.create / session.configuration.update 以及受支持的 CLI 仍可持久化 plan + swarm/graph

这会改变此前已解决 legacy-pair 讨论的处置结论。剩余问题不只是历史记录 normalization:本 PR 合并后仍能创建新的非法 pair;Desktop 随后将其掩盖为 Plan,slash command 的状态查询和 mutation 也会互相矛盾。

请在所有 client 共用的 Runtime Host create/update authority 强制执行该 invariant,并增加一条非 Desktop client 路径的回归。已有记录的 normalization 如有需要可以继续单独跟踪。

除此之外未发现其他 P0–P2。GitHub checks 全绿;本地 focused Desktop build、renderer typecheck、mode/IPC tests 和 Composer menu tests 均通过。

@Astro-Han

Copy link
Copy Markdown
Contributor Author

Thanks for pushing on the authority question — it turned out this PR invented the invariant you asked me to enforce.

Plan and orchestration are two fields with two lifetimes that Runtime honours together, and fusing them into one choice was quietly clearing the user's Swarm default on the way into Plan. So the fusion is gone rather than pushed down to Runtime Host, and nothing is added there. Details in the thread.

Re-requesting review 🙏


感谢指出这个 authority 问题——顺着查下去发现,你要求强制的这条 invariant 是本 PR 自己发明的。Plan 和 orchestration 是两个生命周期不同的字段,Runtime 本来就同时承认它们;把它们合成一个四选一,会在进入 Plan 时悄悄清掉用户的 Swarm 默认值。所以删掉的是这个合并,而不是把它下推到 Runtime Host——Host 一行未加。细节见 thread。


AI disclosure: Claude Code (Opus) wrote the diff, the commit message and this comment. The adjudication of the finding, and the decision not to enforce at Runtime Host, are the human contributor's. Verified locally: four desktop tsconfig projects, @maka/ui typecheck, biome format and lint, both affected test files (fail-first checked), Storybook render smoke (133 stories), and the menu measured live at 28px per row with all six mode icons on one x.

@Astro-Han
Astro-Han requested a review from M4n5ter August 19, 2026 08:47
The menu mixed two Astryx item components and offered a choice the runtime
cannot honour. Plan, Swarm and Graph were three independent checkbox items,
but Swarm and Graph both write `orchestrationMode`, so turning one on
silently turned the other off — and Plan strips subagent-category tools and
the agent-graph tools (`plan-mode.ts`), which is what Swarm and Graph are
made of. The four states were always one choice.

So the modes become one radio group of four, `default` included because it
is the way out of the other three. `sessionMode` + `onSessionModeChange`
replace the three switch prop trios, and app-shell's `setSessionMode` is the
single writer that sequences the two Session fields the choice lands in.
Leaving Plan can be refused (a pending proposal asks first), so the chain
stops there rather than applying half of the change.

The rows now share one rhythm. `DropdownMenuCheckboxItem` never applies the
menu's sm density, so its rows stood 36px against the 28px action rows above
them, and its marker column pushed their icons 28px to the right. Radio rows
already had a product rule for that density; the selection mark moves to
`endContent` as Astryx's own `check` indicator — what its Selector puts on a
chosen option — and composer.css suppresses the radio circle for this panel
alone. Measured live in Storybook: six rows, 28px each, icons on one x.

The + and permission menus also join the palette seam in astryx-mount.css.
Astryx renders these panels in place rather than portaling them, so without
the seam they painted from the neutral theme: the menu's own accent was
near-black while the footer's mode mark beside it was Maka blue, for the
same state.

Generated-by: Claude Code
Review found four defects in the mode control. They are one defect: the
mode is one choice out of four, but the authority holds it as two
independently-writable fields, and the renderer computed each transition
from a value it derived by reading both.

- The write depended on the read. While a Session summary was still
  loading, the placeholder made the derived mode read `default`, so
  choosing Swarm skipped the "leave Plan" step and wrote Plan-plus-Swarm.
- The chain re-read `activeIdRef.current` per step. A Plan-exit
  confirmation is an await, so opening another Session while it was up
  finished the transition on that one instead.
- `/swarm on` and `/graph on` called the orchestration setter directly,
  which is a second path to the same fields and could persist the
  combination the menu had just been rebuilt to prevent.
- Two writes are not atomic, and nothing said which order was safe.

So `session-mode.ts` now owns the mapping, and every mode names BOTH
fields. A transition is computed from where it is going, never from where
the renderer thinks it is, and a stale read can no longer produce a
between-modes write. `applySessionMode(mode, sessionId)` is the only
caller of either setter: it takes its Session as an argument rather than
re-reading it, gates on `getPlanState` — the Host, not the projection —
and both slash commands route through it. The three per-field writers are
gone, and with them the two pending registries and six copy strings.

The order is now a stated rule: the field taking a non-neutral value is
written last, so no intermediate state is ever an illegal combination.
A test walks every starting pair through every target and fails if the
order is reversed. Two writes are still two mutations, so a failure on
the second leaves the Session in `default` with its prior mode lost and a
toast; the opposite order would fail into Plan-plus-orchestration instead.
Making that atomic means one persisted field, which is a Runtime Host
change, not a renderer one.

The mode control is also disabled while the Session summary is loading —
now a matter of not showing a mode that is not known yet, rather than of
correctness.

Generated-by: Claude Code
The divider between the action rows and the mode group rendered on
`onSessionModeChange` alone. Every prop feeding this menu is optional, so
a host that wires only the mode choice — which `packages/ui` allows and
the SSR contracts exercise — opened the menu on a rule with nothing above
it to divide. The desktop app always wires attachments, so this was a
defect in the component's contract rather than a visible one.

The condition now asks what it means: is there an action row above.

Generated-by: Claude Code
The mode is one choice held in two persisted fields, and the renderer was
writing them one at a time. A failure on the second write left the Session
in neither the mode it had nor the one that was asked for. The previous
commit could only pick which of the two failures to have — it ordered the
writes so the state between them was at least legal.

There was nothing to consolidate at the Host: `session.configuration.update`
already carries the WHOLE configuration against the Session revision, and
`updateSessionConfiguration` already merges a patch into it. The desktop
simply never exposed a bridge method that carried both mode fields, so it
sent two patches where one would do.

Now `sessions:setSessionMode` takes both fields and the pair lands in a
single revision-checked mutation. A failure changes nothing, so the
ordering rule and its property test are deleted along with the two
per-field bridge methods and IPC channels — there is no longer an
intermediate state to reason about, and no way to write one field alone.

The naming was also wrong. `collaborationMode` and `orchestrationMode` are
not two halves of one value: the orchestration field is a per-turn
DEFAULT that `turnOrchestration` overrides, which is why an AgentRun
records `orchestrationSource: 'session' | 'turn_override'`. They are two
fields of different lifetimes that the composer's four modes happen to
address together, and that is the only place they are one choice.

Generated-by: Claude Code
Review asked whether the radio-indicator override belongs in
`packages/ui/src/styles.css` rather than in the host. It does not, and the
reason is worth writing next to the workaround: all 40 `.maka-composer-*`
rules are the host's, so moving this one would make it the only composer
rule on the other side of the split.

The comment now says that, so the next reader does not have to measure it.

Generated-by: Claude Code
Each field was checked for being a valid value; the pair was not. A frame
naming `plan` with `swarm` passed both checks and persisted, which is a
Session the runtime cannot honour: Plan strips the subagent-category and
agent-graph tools that Swarm and Graph are made of.

Today the only caller builds its frame from `SESSION_MODE_FIELDS` and
cannot produce that pair. That is the argument for checking it here rather
than against it — the boundary that can persist the state is the one that
has to refuse it, instead of trusting every present and future caller.

`session-mode.ts` moves to `src/shared/`, which main, preload and renderer
already share, so the boundary and the control test the same predicate
instead of keeping two copies of one rule.

Generated-by: Claude Code
…own reason

`plusMenu` sliced the markup on the menu's marker class, but `split` returns
the whole markup when the marker is absent, so a composer that stopped
rendering the menu still satisfied the divider-absence assertion. It now
asserts the marker before slicing.

The session-mode IPC stub answered `recover()` without `failed`, which the
registration iterates. The rejection is swallowed today, so the stub would
only surface as an unrelated failure in whatever test touched it next.

Generated-by: Claude Code
Review asked for the legal mode pair to be enforced at the Runtime Host
authority, because Desktop's IPC check was the only thing preventing
`plan + swarm`. Tracing where that invariant should live found that it should
live nowhere: this PR invented it.

`collaborationMode` and `orchestrationMode` are two fields with two lifetimes.
Plan is a temporary excursion Runtime ends by itself — `finalizePlanCollaborationMode`
and `finalizePlanAbandonment` write the Session back to `agent` on approval or
abandonment and never touch orchestration (`session-manager.ts`). Orchestration
is the Session's standing default for how a turn fans out, which
`turnOrchestration` overrides per turn. Runtime resolves the overlap by
stripping the subagent-category and agent-graph tools while planning
(`plan-mode.ts`), validates the two fields independently, and rejects no
combination of them anywhere. So `plan + swarm` means "planning now, Swarm for
the execution afterwards" — a state the Session can hold.

Fusing them into one four-way choice is what made a legal state look illegal,
and it cost the user real state: entering Plan from Swarm wrote
`orchestrationMode: 'default'`, and since Runtime leaves Plan on its own after
approval, Swarm was gone for the execution the plan was written for. `main` did
not do that — the Plan switch there wrote its own field only.

So the fusion goes, along with everything built on it: `ComposerSessionMode`,
`shared/session-mode.ts` and its projection, the pair check at the IPC
boundary, and the two-field bridge method. Composer takes a Plan toggle and an
orchestration choice; app-shell has one writer per field, each a patch into
`updateConfiguration` beside `setPermissionMode`; the renderer derives no mode
at all, so there is nothing to keep in sync. Nothing is added at Runtime Host:
with the invariant gone there is no illegal pair for it to refuse, and the
`/swarm status` versus `/swarm off` disagreement the review named came from the
projection that is now deleted.

Swarm and Graph stay one radio group — one field, three values, and turning one
on silently turned the other off. That defect was real; it just was not Plan's.

The menu keeps one rhythm without pretending to be one control. A toggle and a
one-of-N choice keep their honest roles (`menuitemcheckbox`, `menuitemradio`);
composer.css puts both on the menu's sm density and suppresses both markers, so
each row is 28px with the check at its tail. The upstream ask widens from radio
items to both selectable item types.

`PlanAndSwarmModeOn` comes back. This PR deleted it as unreachable, and it is
the state the argument above is about.

Verified: `session-mode-ipc-main.test.ts` fails on the Plan write when the
handler is made to carry `orchestrationMode` again; `composer-plus-menu.test.tsx`
covers both roles and both marks. Four desktop tsconfig projects, `@maka/ui`
typecheck, biome format and lint clean.

Generated-by: Claude Code
Plan, Swarm and Graph are all things to turn on. "Neither orchestration" is
what the Session is when nothing is on, not a third thing to pick, so a
`Default` row was answering a question the user never asked — it existed only
because a radio group cannot be deselected, which is a control's limit rather
than the product's shape.

So all three become checkbox rows. Turning Swarm on sends `swarm`, turning it
off sends `default`, and because the field holds one value, naming the one
being turned on is also what turns the other off. That happens in the open
menu with both rows visible, so the exclusivity the field imposes is watched
rather than discovered.

The menu is now one kind of row throughout, which is what the PR set out to do
and could not honestly claim while a one-of-N group was in it. `orchestrationOptions`
loses its neutral entry, the radio group and its accessible-name string go, and
composer.css drops the radio-circle suppression it no longer has anything to
suppress — the density rule keeps both roles, because the permission menu
beside this one is still a real radio group.

Measured in Storybook (DefaultLayout): five rows, 28px each, every mode icon on
the same x as the action rows above, panel 144x167, nothing checked. The
PlanAndSwarmModeOn story reads back both rows checked and both footer marks.

Generated-by: Claude Code
@Astro-Han
Astro-Han force-pushed the refactor/composer-plus-menu-single-rhythm branch from 9d52187 to 4fc463c Compare August 20, 2026 00:39

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

Requesting changes on head 4fc463c.

The previous Runtime Host authority discussion is correctly resolved: Plan and orchestration are independent Session fields, and plan + swarm/graph is valid. One P2 remains in the latest UI revision: Swarm and Graph still share one three-valued field, but they are exposed as independent checkboxes, so the original silent replacement remains for assistive-technology users.

No other P0–P2 issue was found. The rebased patch series is unchanged, and all current required checks pass.

简体中文

在 head 4fc463c 上建议 Request changes。

此前 Runtime Host authority 的讨论已经正确解决:Plan 与 orchestration 是两个独立的 Session 字段,plan + swarm/graph 是合法状态。目前最新 UI revision 仍有一个 P2:Swarm 和 Graph 共享同一个三值字段,却被表达为两个独立 checkbox,导致原先的静默替换问题仍会影响辅助技术用户。

除此之外未发现其他 P0–P2。rebase 前后的 patch series 没有实质变化,当前所需检查全部通过。

Comment thread packages/ui/src/composer.tsx Outdated
Swarm and Graph are the two values of one Session field, and a run carries one
orchestration, so choosing one is choosing away from the other. Two checkbox
rows said the opposite: each announced itself as an independent boolean, and
the row that turned off was never announced at all, so a screen-reader user
turning on Swarm was not told Graph had gone. That is the silent replacement
this PR set out to remove, still present for the users least able to see it.

They are now one radio group, which is the control Astryx directs a one-of-N
choice to and the one that carries the relationship: the rows are announced as
a named set, and picking one moves the set.

No neutral row comes back with it. `default` is not a third way to fan a turn
out, it is the absence of one, so the group holds no selection at rest --
`DropdownMenuRadioGroup` takes `value: string | undefined` for exactly that.
The way back is the selected row itself: a radio item reports its value on
every activation, so re-activating the one already chosen empties the group.
A native radio does not do that, but an empty group is a state the role
expresses, and the change lands on the row the user is on rather than on the
row they left.

Nothing moves. Both selectable item types draw a control at the row's start,
so the suppression rule that already hid the checkbox box now hides the radio
circle too, and the tail check stays the mark: five rows at 28px, every icon
on x=532, panel 144x167 -- the same numbers as before this commit.

Generated-by: Claude Code
@Astro-Han
Astro-Han requested a review from M4n5ter August 20, 2026 01:51

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

LGTM. The radio-group relationship is now correct, and the remaining reactivation behavior is an intentional design choice. All current checks pass.

简体中文

LGTM。radio group 的关系已经正确,剩余的再次激活行为属于明确的设计选择。当前检查全部通过。

@Astro-Han
Astro-Han merged commit 50a8fc4 into apache:main Aug 20, 2026
11 checks passed
@Astro-Han
Astro-Han deleted the refactor/composer-plus-menu-single-rhythm branch August 20, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants