Skip to content

refactor(ui): unify the composer's Skill surfaces on / and inline chips - #1915

Merged
Astro-Han merged 9 commits into
mainfrom
fix/composer-skill-astryx-surface
Aug 2, 2026
Merged

refactor(ui): unify the composer's Skill surfaces on / and inline chips#1915
Astro-Han merged 9 commits into
mainfrom
fix/composer-skill-astryx-surface

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Aug 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

The composer had two Skill surfaces and neither was Astryx's. This PR reduces them to one, and puts the staged Skill where it belongs.

1 — the reported defect. ＋ → Skills mounted a fully transparent panel: header, rows and status text overlapped the composer placeholder and the transcript behind it. packages/ui/src/styles.css built that surface from --popover, --popover-foreground and --shadow-maka-panel — three custom properties nothing in the repo defines and none with a var() fallback, so all three dropped at computed-value time and the panel kept its stacking and radius but lost its fill.

It could only lose its background because it was the one composer float that owned a surface at all. The deeper problem is that it existed: / and ＋ produced the same result through two different UIs. ＋ now opens the / menu by typing the trigger for the user, and ComposerSkillPicker is deleted — 238 lines, every .maka-composer-skill-panel* / .maka-composer-skill-trigger* rule, and nine copy keys only it read.

useTriggerMenu recognizes / only at a line start or after whitespace, 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 one input event and one undo step.

2 — the entry writes into the draft, so it must not be reachable with nothing to write. With no Skill installed, choosing it left a stray / in the draft and opened an empty menu whose light dismiss then swallowed the user's next click on the footer — reported as "the ＋ and permission buttons stop responding". It is disabled now, with the reason on screen rather than as a silent grey row.

3 — a staged Skill is a chip in the draft, not a selection beside it. This is Astryx's own contract for the trigger menu (onSelect returning a ChatComposerToken) and already how @ file mentions work here.

The drawer chip 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 existed only to keep the twin in step. The chip serializes to /skill:<id> — the grammar Runtime already parses and the TUI already submits — so the text carries it and all of that goes away. Sending is onSend(text).

4 — the chip has to survive the draft coming back. ChatComposerInput rebuilds the editor from the string on every external value change, which drops inline token spans; upstream declares a deserialize hook for exactly this and never calls it (facebook/astryx#4655, filed here). Left alone, any session that ever staged a Skill shows a raw /skill:<id> on the way back. redrawSkillTokens re-inserts the chips from that text — nothing is recovered that the string did not already hold, so the draft stays the single source of truth and the whole thing deletes in one piece once upstream deserializes. The /skill: grammar moves to @maka/core in the process, replacing two byte-identical copies with one.

Closes #1912

Verification

  • apps/desktop E2E, full suite: 87 passed — one fewer Skill journey than main, not four more. What only a window can show stayed (＋ inserting its own space before the trigger, a draft restored blurred coming back as chips and sending correctly); a max-width read back out of CSS, a Backspace path that is now vendored, and a rendered-markup string assertion moved to cheaper checks or left with the code they guarded.
  • packages/ui 235, packages/core 688, packages/runtime 2687, maka-agent (CLI) 466, apps/desktop main 1364. composer-skill-picker.test.tsx and use-composer-skill-draft.ts deleted with what they covered.
  • Reviewed by Codex and by an independent agent against the @astryxdesign/core source; both found the redraw's algorithm correct and both landed on the same defect, fixed in bacf822: the redraw flag was consumed on a failed attempt, so a catalog still in flight lost the chips for good.
  • npm run lint, npm run format:check, npm run typecheck (core/ui/runtime/cli/storage/desktop), check-dead-css / check-a11y / check-copy / check-console: clean.

Breaking change

Composer's onSend takes one argument. ComposerHandle loses getSkills, setSkills and setSkillDraft; E2eFixtureState.composerSkills is gone (fold the Skill into composerText). The skillIds field on the sessions.send IPC command stays — Runtime still accepts it and headless callers still produce it — but the renderer no longer does.

Review focus

  • Multi-select is gone. The panel's checkboxes and "select all" leave with it; / twice still stages two Skills, one chip each.
  • The token carries the id, not the scope-aware ref. The ref cannot be spelled in this grammar (project:.maka/skills:writer parses as project), ids are unique within a scan, and across the gap between choosing and sending a ref is no less stale than an id — only differently stale.
  • /swarm and /graph are now order-dependent with a staged Skill. parseSwarmCommand requires /swarm to be the first token, so picking the Skill first produces /skill:x /swarm … and sends as an ordinary message. Appending the Skill after the command still works. The honest consequence of "the text is the draft", but worth naming.
  • The skillIds transport survives with no producer. Preload, the IPC guard, sessions-ipc-main and Runtime still carry it; after this PR nothing in the renderer, UI, CLI or headless writes it. Removing it crosses preload/main/runtime and belongs in its own PR.
  • redrawSkillTokens leans on two upstream behaviors: ensureCaretInside preserves a non-collapsed selection, and insertToken does not emit a change event. Both verified against @astryxdesign/core source. It runs only for writes that came through textPort, only on the single-text-node DOM those writes produce, and never mid-composition; a token whose id is not in the live catalog stays text.

@Astro-Han
fix(ui): open the / Skill menu from ＋ instead of a second panel
40a5951 
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
Astro-Han force-pushed the fix/composer-skill-astryx-surface branch from ddc617a to 40a5951 Compare August 2, 2026 13:12
@Astro-Han Astro-Han changed the title fix(ui): move the composer Skill panel onto an Astryx popover fix(ui): open the / Skill menu from ＋ instead of a second panel Aug 2, 2026
@Astro-Han
fix(ui): keep the ＋ Skills entry out of reach when no Skill exists
fc27a80 
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.
@Astro-Han
refactor(ui): stage Skills as inline chips in the draft
13d3e74 
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.
@Astro-Han Astro-Han changed the title fix(ui): open the / Skill menu from ＋ instead of a second panel refactor(ui): unify the composer's Skill surfaces on / and inline chips Aug 2, 2026
@Astro-Han
fix(ui): stop the composer's / menu at a readable width
c2ace5a 
`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.
@Astro-Han
fix(ui): give the / menu's second line to the description
8d200b5 
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.
@Astro-Han
refactor(core): keep one copy of the /skill: token grammar
df20310 
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.
@Astro-Han
fix(ui): redraw staged Skill chips after a draft is restored
1039b94 
`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.
@Astro-Han
fix(ui): retry a pending chip redraw instead of consuming it
bacf822 
`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.
@Astro-Han
test: cut the Skill E2E back to what only a window can show
71c9dab 
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.
@Astro-Han
Astro-Han force-pushed the fix/composer-skill-astryx-surface branch from 739401e to 71c9dab Compare August 2, 2026 16:04
@Astro-Han
Astro-Han marked this pull request as ready for review August 2, 2026 16:08
@Astro-Han
Astro-Han merged commit d158504 into main Aug 2, 2026
5 checks passed
This was referenced Aug 3, 2026
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(ui): bring the composer Skill surface onto Astryx primitives and the product palette

1 participant

@Astro-Han