fix(ui): read session modes off the composer footer, not the context drawer - #1910
Merged
Conversation
…drawer The composer drawer's contract is context staged for the next send: quotes, attachments and Skills are consumed when the message goes out, and its badge counts how many such items are pending. Plan, Swarm and Graph are session-scoped modes that survive the send, so rendering them there inflated that count with things the send never consumes and gave a persistent mode the visual weight of a transient attachment. Move the three mode marks to the tail of the footer's left controls, after the model and thinking pickers, so toggling a mode never shifts those two. They are icon-only Astryx Tokens now — the label stays as the accessible name and Astryx's own remove button is the way out — and the disabled reasons keep surfacing on the wrapper's title. The ＋ menu remains the switch that turns each mode on and off; the footer mark is the resting readout plus one nearby exit. The issue names Plan and Swarm; Graph shares the same block and the same miscount, so it moves with them. Closes #1897
The three marks inherited blue / teal / cyan from the drawer Tokens they replaced. Those hues are Astryx stock palette — makaTheme.ts maps nothing onto Astryx's Token colours — and DESIGN.md is explicit that Maka blue is the single product accent and that another accent must not be introduced. Nor were the hues carrying semantics: the palette's meanings are info / success / warning / destructive, and none of them is "which mode this is". Teal and cyan were serving as a mode index, and they sit close enough that Swarm and Graph were hard to tell apart at icon size anyway. Which mode a mark is now comes from its icon. All three share the one accent, which is what the colour is for: this is an active state. Moving them out of the drawer also moved them next to ＋ and permission, where a coloured pill read as a foreign object on a row of ghost icon buttons — and "don't turn every status into a coloured pill" is on the same Don't list. They are ghost IconButtons now, the same primitive as their two neighbours, tinted with --accent over a currentColor-derived wash so a disabled mark dims both layers together. Clicking one leaves the mode; the tooltip says so, and a disabled reason replaces it and takes the button out of reach. Refs #1897
…eal locks Three-way review (subagent, Codex, k3-256k) found defects each of the other two missed, and mutation testing proved two contracts this branch claimed were not locked at all. Adjudicated and fixed at the owner: The marks had no hover or press feedback. `background:` is a shorthand, so it carried an implicit `background-image: none`, and composer.css sits in the last cascade layer — that beat the ghost button's own :hover / :active overlay gradients from `astryx-components`. Measured, not inferred: the mark resolved `background-image: none` on hover where the ＋ beside it resolved a gradient, so the one control that leaves a mode was the only dead one on the row. It is `background-color` now, and so is the voice button, which is where the pattern was copied from and which had the same bug. Leaving a mode dropped focus on `document.body`, because the click unmounts the button. Removing a Skill token 200 lines up already hands focus back to the input; the modes now do the same. An active mode with no change handler rendered a focusable, tooltipped button whose click was a silent no-op. It is gated on the handler, as the ＋ menu's row already was. The marks were 13px where every sibling ghost icon on the toolbar — ＋, permission, mic, and the same three glyphs in the ＋ menu — is 15px. On the locks, mutation testing showed two were theatre. Deleting the accent rule outright left 240 unit tests, 1339 desktop tests and the dead-CSS gate green, so the rule is now pinned in the existing CSS text-contract seam, along with a ban on the `background` shorthand for both marks. Deleting the deactivate handler was equally invisible: a static render is byte-identical with and without it, so "removal stays available" — the one thing #1897 asks for — is now one representative E2E journey covering pointer, keyboard, and the focus handoff. The remaining assertions were scoped to the thing they name rather than to the rest of the document: the streaming case reads the mark's own `<button>` and follows its `aria-describedby` to the reason, the badge matches its own text node, and the drawer slice is bounded by the next footer landmark instead of the first `</div>`. Graph joins Plan and Swarm in the position and count cases. Also drops the wrapper `<span>` — `data-mode` rides the button, as the voice button's `data-state` already does — and folds `activeModes` back into one filter, which removes the `pending` field and a whole spread-map pass. Refs #1897
Astro-Han
marked this pull request as ready for review
August 2, 2026 12:06
Astro-Han
added a commit
that referenced
this pull request
Aug 2, 2026
Opening ＋ → Skills mounted a fully transparent panel: its header, rows and status text overlapped the composer placeholder and the transcript behind it, with no bounds visible. `packages/ui/src/styles.css` built that panel's surface from `--popover`, `--popover-foreground` and `--shadow-maka-panel` — three custom properties nothing in the repo defines and none with a `var()` fallback. All three declarations dropped at computed-value time, so the panel kept its stacking and radius from the two tokens that do exist and lost its fill. (The same rule named `.maka-composer-mention-panel`, which has had no consumer since `/` moved to the Astryx trigger menu.) The panel could only lose its background because it was the one composer float that owned a surface at all. But the deeper problem is that it existed: `/` and ＋ produced the same draft chip through two different UIs — an Astryx trigger menu on one side, a hand-rolled multi-select panel on the other — so they never looked alike and the product owned a popover surface standing beside Astryx's. So ＋ now opens the `/` menu by typing the trigger for the user, and `ComposerSkillPicker` is deleted: 238 lines, every `.maka-composer-skill-panel*` and `.maka-composer-skill-trigger*` rule in two CSS files, and nine copy keys that only it read. One menu, one presentation, one contract. `useTriggerMenu` recognizes `/` only at a line start or after a space or newline, so ＋ inserts the space itself when the draft ends in a word — or in a chip, which `insertToken` anchors with U+00A0. Both characters go in one `insertText`, so the editor sees a single input event and a single undo step. The caret is moved to the end first: the menu round trip leaves a stale selection collapsed at offset 0, and measured, the slash landed in front of the draft rather than after it. Dropped with the panel: checkbox multi-select (`/` twice selects two Skills) and "select all", which contradicted the panel's own hint that leaving the selection empty lets Maka match Skills automatically. Also on this surface: staged-Skill chips rendered as `<Token color="purple">`, a hue `makaTheme.ts` maps nothing onto and one `DESIGN.md` puts on its Don't list. They draw in the neutral Token now, identified by a sparkle, as #1910 did for the mode marks. Closes #1912
Astro-Han
added a commit
that referenced
this pull request
Aug 2, 2026
…hips (#1915) * fix(ui): open the `/` Skill menu from ＋ instead of a second panel Opening ＋ → Skills mounted a fully transparent panel: its header, rows and status text overlapped the composer placeholder and the transcript behind it, with no bounds visible. `packages/ui/src/styles.css` built that panel's surface from `--popover`, `--popover-foreground` and `--shadow-maka-panel` — three custom properties nothing in the repo defines and none with a `var()` fallback. All three declarations dropped at computed-value time, so the panel kept its stacking and radius from the two tokens that do exist and lost its fill. (The same rule named `.maka-composer-mention-panel`, which has had no consumer since `/` moved to the Astryx trigger menu.) The panel could only lose its background because it was the one composer float that owned a surface at all. But the deeper problem is that it existed: `/` and ＋ produced the same draft chip through two different UIs — an Astryx trigger menu on one side, a hand-rolled multi-select panel on the other — so they never looked alike and the product owned a popover surface standing beside Astryx's. So ＋ now opens the `/` menu by typing the trigger for the user, and `ComposerSkillPicker` is deleted: 238 lines, every `.maka-composer-skill-panel*` and `.maka-composer-skill-trigger*` rule in two CSS files, and nine copy keys that only it read. One menu, one presentation, one contract. `useTriggerMenu` recognizes `/` only at a line start or after a space or newline, so ＋ inserts the space itself when the draft ends in a word — or in a chip, which `insertToken` anchors with U+00A0. Both characters go in one `insertText`, so the editor sees a single input event and a single undo step. The caret is moved to the end first: the menu round trip leaves a stale selection collapsed at offset 0, and measured, the slash landed in front of the draft rather than after it. Dropped with the panel: checkbox multi-select (`/` twice selects two Skills) and "select all", which contradicted the panel's own hint that leaving the selection empty lets Maka match Skills automatically. Also on this surface: staged-Skill chips rendered as `<Token color="purple">`, a hue `makaTheme.ts` maps nothing onto and one `DESIGN.md` puts on its Don't list. They draw in the neutral Token now, identified by a sparkle, as #1910 did for the mode marks. Closes #1912 * fix(ui): keep the ＋ Skills entry out of reach when no Skill exists The entry types `/` into the draft, so an empty catalog turned one click into three surprises: a stray slash in the user's draft, an empty "no skills available" menu, and — because that menu is a light-dismiss popover — the next click anywhere on the composer footer swallowed as a dismissal. Reported as the permission and ＋ buttons "not responding". Measured, not inferred. `elementFromPoint` at both buttons still resolved inside their own SVG, and pointerdown/mousedown/click all arrived with defaultPrevented=false, which ruled out the overlay explanation first suspected. The empty menu on the popover stack was what consumed the click. Disable the entry when the catalog is empty, and say why on the row itself. The reason uses Astryx's `description` prop rather than `aria-description`: the latter never reaches the DOM through DropdownMenuItem (the assertion caught it), and a grey row whose reason only assistive tech can read still answers nothing for the user looking at it. The panel this replaced showed the same sentence. * refactor(ui): stage Skills as inline chips in the draft A Skill chosen from `/` is now an inline `ChatComposerToken` in the input, which is Astryx's own contract for the trigger menu (`onSelect` returning a token) and already how `@` file mentions work here. It used to be a selection held beside the draft and shown in the context drawer. That made one fact — "this send invokes this Skill" — into two sources of truth, and every draft operation grew a Skill-shaped twin: per-session persistence, prompt history, blocked-send recovery and revision rollback each carried and restored the selection separately. `useComposerSkillDraft`, three `ComposerHandle` methods, `TurnRevisionDraft.previousComposerSkills` and the `skillIds` argument threaded from the composer through the shell all existed only to keep the twin in step. The chip serializes to `/skill:<id>` — the invocation grammar Runtime already parses and the TUI already submits — so the text carries it and all of that goes away. Sending is `onSend(text)`. By id rather than the scope-aware ref: the ref cannot be spelled in this grammar, ids are unique within a scan, and across the gap between choosing and sending a ref is no less stale than an id. A draft rewritten from outside (session switch, history recall, revision rollback) comes back with the token as plain text rather than as a chip — same text, same send, and the same degradation `@` chips already have. * fix(ui): stop the composer's `/` menu at a readable width `useTriggerMenu` gives its popover a 180px floor and no ceiling, so the menu sizes to its widest row — and our rows carry a second line (`<id> · <description>` for a Skill, the full path for a file) that never wraps. A Skill whose description is a sentence stretched the menu past the composer it is anchored to and on toward the window edge. Cap it at 420px, the width the model switcher on the same footer already chose, so the two floats over that footer agree. The row's two lines already carry `overflow: hidden` + `text-overflow: ellipsis` and were only waiting for a constraint to act on. `.astryx-trigger-menu` is the component's documented theming hook, not an internal class. The `user-only` E2E fixture now carries a sentence-length description, which is what the new journey measures: menu narrower than the composer, and the second line overflowing its own box rather than widening it. * fix(ui): give the `/` menu's second line to the description The row read `<id> · <description>` on one non-wrapping line, so the id spent a dozen characters of the only line that tells two Skills apart — on a string nobody types here, since the menu is how you avoid typing it. The id stays searchable and is still what the chip serializes to; it just leaves the row. Kept visible rather than moved to a tooltip: every menu item in Astryx — dropdown, radio, checkbox, submenu — carries its explanation as a `description` line, and none carries one on hover. A `/` menu is driven with ↑↓, so a hover-only description would be invisible to the way it is used. * refactor(core): keep one copy of the /skill: token grammar Runtime and the TUI each carried a byte-identical `SKILL_INVOCATION_TOKEN_SOURCE`, which held only because nobody edited one of them. The renderer needs the same grammar to render a draft, and adding a third copy is not an option, so the string moves to core — which Runtime, the TUI and the UI package all already depend on — and both existing sites import it. * fix(ui): redraw staged Skill chips after a draft is restored `ChatComposerInput` rebuilds the editor from the string on every external value change, which is right for text and lossy for tokens: the chip spans go and the draft comes back as the `/skill:<id>` text they serialize to. Upstream declares a `deserialize` hook for exactly this and never calls it (facebook/astryx #4655). The draft still sends the Skill, but the user is looking at an internal id where they left a chip, and the path is the ordinary one: any session that ever staged a Skill hits it on the way back. Staging a Skill as a chip is only worth doing if the chip is still there afterwards. So redraw them. Nothing is recovered that was not already in the string — the draft stays the single source of truth and only its rendering is restored, which is what keeps this deletable in one piece when upstream deserializes. Gated three ways: only for a write that came through `textPort` (typing clears the flag, so a half-typed `/skill:` never seizes into a chip under the caret), only on the single-text-node DOM that write produces, and never mid-composition. The flag survives a render because the two inputs do not arrive together: a session switch swaps the draft immediately and the Skill catalog for the new session lands a render or two later. A token whose id is not in that catalog stays text — no chip should promise a Skill that will not resolve. * fix(ui): retry a pending chip redraw instead of consuming it `redrawSkillTokens` was called behind a `mentionSkills.length` pre-gate and its flag cleared whether or not it succeeded, so every transient bail — the catalog still in flight, an active IME composition, a DOM shape not yet settled — lost the redraw for good. Clearing the flag only on success drops the pre-gate, makes each of those a retry, and leaves the one permanent case (a token whose id is not in the catalog) exactly as it was. `applyText` now clears `caretToEndRef` alongside it. Both are armed by `textPort.setValue` and re-armed right after, but a write React bails out of — the new draft equals the old one, so no commit and no effect — left them armed until some unrelated later render, where the caret jumped to the end mid-word. * test: cut the Skill E2E back to what only a window can show Four of the eleven journeys in `composer-skill-invocation.spec.ts` were paying an Electron boot for something cheaper: - the Backspace-away-from-the-chip test guarded Maka's own `isCaretAtContentStart` until this branch deleted it; what is left is `useChatComposerTokens`, vendored; - the menu-width test read a `max-width` back out of CSS through a live window, and its `menuWidth < composerWidth` arm turned on the window size rather than on the rule. The fixture description lengthened to feed it goes back too; - the select-from-slash test's unique half is "picking produces a chip that serializes to `/skill:<id>`", which `chip-only send` already asserts and then sends; - the empty-catalog entry test is a string assertion on rendered markup, and moves to the Composer SSR test that already renders the ＋ menu. What is left grows instead: the restore journey now stages two Skills with one of them mid-draft, which is the only thing that exercises the redraw's back-to-front walk, and sends afterwards, which is the only thing that proves the U+00A0 anchors survive `composerWireText`. The ＋ journey keeps its trigger-boundary halves and loses the three `waitForTimeout(300)` sleeps: Astryx's post-dismiss click guard has no observable end, so retry the click until ＋ answers rather than sleep past it. Also gone: `PINNED_OFF_RULER`, empty since the count badge left, along with its loop and its explanation of a future exception; two `doesNotMatch` assertions on a class this branch deleted, which can no longer fail; and comments describing the `/` menu's second line as `<id> · <description>` after it became the description alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The composer drawer's contract is context staged for the next send. Quotes, attachments and Skills are consumed when the message goes out, and
drawerTokenCounttells the user how many such items are pending. Plan, Swarm and Graph are session-scoped modes that survive the send, so rendering them there inflated that count with things the send never consumes, and gave a persistent mode the visual weight of a transient attachment. Turning Plan on with nothing staged also mounted the whole drawer, growing the card by ~200px to hold one pill.The three mode marks move to the tail of the footer's left controls, after the model and thinking pickers. Placing them last is the point: a mode turning on or off never nudges those two, which it would if the marks sat before them.
Once they landed next to ＋ and permission, the colours they had inherited stopped being defensible, so the second commit fixes them too.
blue/teal/cyancame from Astryx's stock Token palette —makaTheme.tsmaps nothing onto it — whileDESIGN.mdstates that Maka blue is the single product accent and lists "don't introduce another accent" and "don't turn every status into a coloured pill" under Don't. The hues were not carrying semantics either: the palette's meanings are info / success / warning / destructive, and "which mode this is" is not among them. Teal and cyan were a mode index, and they are close enough that Swarm and Graph were hard to separate at icon size.So the marks are now ghost
IconButtons — the same primitive as the ＋ and permission buttons they sit beside — tinted with--accentover acurrentColor-derived wash, so a disabled mark dims both layers together. Which mode a mark is comes from its icon. Clicking one leaves the mode, and its tooltip says so; a disabled reason replaces that tooltip and takes the button out of reach.The ＋ menu is unchanged and remains the switch that turns each mode on and off. The footer mark is the resting readout plus one nearby exit — the same reading as the permission icon two slots to its left.
Two judgement calls worth naming:
planModeDisabledReasonand friends already disable the button and surface their reason in its tooltip, so nothing becomes actionable that shouldn't be.One correction to the issue: it says
onRemoveis the only way to leave a mode from the composer. The ＋ menu's checkbox items already toggle both directions, so moving the mark never risked stranding a user in a mode.The Skill token beside these still uses
purple, which is off-palette for the same reason. That predates this PR and is left alone here; worth its own issue.Closes #1897
Verification
Before/after and all four colour candidates were captured from the built app driven through Playwright
_electronon the fake backend, with the modes toggled through the real ＋ menu, and composed with ImageMagick. Before: Plan + Swarm stack as full pills in the drawer, pushing the card ~200px taller. After: two accent-tinted icon buttons sit afterClaude Sonnet 4.5 / 默认, card height unchanged, tooltip readingPlan 模式已启用，点击关闭.npm run lint/format:check— cleantypecheck(@maka/desktop, all four projects) — cleannode scripts/check-dead-css.mjs --check— clean@maka/uiunit — 240 pass, 0 fail@maka/desktopunit — 1339 pass, 0 failContract locks added to
composer-quiet-chrome.test.tsx:maka-composer-left-controls, at an index aftermaka-model-selection-controlslucide-list-todo,aria-label="Plan", andmaka-composer-mode-buttonastryx-tokenmaka-composer-context-drawer1conversation-localization.test.tsxkeeps its existing coverage — localized tooltips per mode, and the streaming case where the mark stays visible while its button goes inert — retargeted at the new position and primitive.No new E2E: this is a renderer-only move, and the one E2E that reads the drawer (
quote-companion) covers quotes, which still live there.Third commit: what three-way review found
A subagent, Codex and
kimi-coding/k3-256kreviewed this branch independently. Each found something the other two missed, and k3's mutation testing showed two of the contracts here were theatre. All findings were adjudicated against the code before any edit; the ones that survived:The marks had no hover or press feedback.
background:is a shorthand, so it carried an implicitbackground-image: none, andcomposer.csssits in the last cascade layer — that beat the ghost button's own:hover/:activeoverlay gradients fromastryx-components. Measured through CDP rather than inferred: the mark resolvedbackground-image: noneon hover where the ＋ beside it resolved a gradient, making the one control that leaves a mode the only dead one on the row. Nowbackground-color, and so is the voice button, which is where the pattern was copied from and which carried the same bug. k3 argued this was not a finding because it matched the voice button's existing pattern; that the same bug exists elsewhere does not make it not a bug, so both are fixed.Leaving a mode dropped focus on
document.body, because the click unmounts the button. Removing a Skill token 200 lines up already hands focus back to the input; the modes now do the same.An active mode with no change handler rendered a focusable, tooltipped button whose click was a silent no-op. Gated on the handler, as the ＋ menu's row already was.
The marks were 13px where every sibling ghost icon on this toolbar — ＋, permission, mic, and the same three glyphs inside the ＋ menu — is 15px.
Two locks did not lock anything. Mutation-verified: deleting the accent rule outright left 240 unit tests, 1339 desktop tests and the dead-CSS gate green; deleting the deactivate handler was equally invisible, because a static render is byte-identical with and without it. The accent rule is now pinned in the existing CSS text-contract seam (
chat-shell-layout-contract.test.ts) together with a ban on thebackgroundshorthand for both marks, and "removal stays available" — the one thing #1897 asks for — is covered by one representative E2E journey over pointer, keyboard and the focus handoff.The remaining assertions were scoped to the document rather than to the thing they name. The streaming case now reads the mark's own
<button>and follows itsaria-describedbyto the reason; the badge matches its own text node; the drawer slice is bounded by the next footer landmark instead of the first</div>. Graph joins Plan and Swarm in the position and count cases.Also simplified: the wrapper
<span>is gone —data-moderides the button, as the voice button'sdata-statealready does — andactiveModesfolds back into one filter, removing thependingfield and a whole spread-map pass.Pushed back on two:
aria-presseddoes not apply (the mark only switches off, it never switches on), and renaming the accessible name to "退出 Plan Mode" is unnecessary since the action is carried by the tooltip while the label says what the control is.Mutation evidence for the new locks
.maka-composer-mode-buttonrulebackground-colorback to thebackgroundshorthandmode.onDeactivate()from the clickrequestAnimationFrame(focusInput)handoffactiveFinal: lint / format:check / typecheck / dead-css clean,
@maka/ui241,@maka/desktop1340, E2E 86/86.Review focus
Stories for the four states are in
app-shell.stories.tsx(PlanModeOn,SwarmModeOn,PlanAndSwarmModeOn,ModeOnWithPendingAttachments). The last one is the interesting one: it is the split the issue is about — the badge counts the two attachments, Plan reads off the footer.