Skip to content

redesign(desktop): rebuild the subagent settings page on the settings route idiom - #2008

Merged
Astro-Han merged 8 commits into
mainfrom
redesign/subagent-settings-astryx-idiom
Aug 3, 2026
Merged

redesign(desktop): rebuild the subagent settings page on the settings route idiom#2008
Astro-Han merged 8 commits into
mainfrom
redesign/subagent-settings-astryx-idiom

Conversation

@Astro-Han

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

Copy link
Copy Markdown
Contributor

Summary

#1999 shipped 设置 · 子 Agent as dense rows over a 560px Dialog holding eight fields behind an inner scrollbar, plus three hand-written .subagentPreset* CSS rules. A modal exists to interrupt the current task for something short and immediately decidable; naming a capability, writing the guidance the main agent selects on, and picking a connection/model/thinking route is none of those. Astryx says the same — "if the content grows beyond what fits, consider a full page instead" — and the providers panel next door already answers this exact shape with a route level, so this page follows it instead of inventing a second answer.

Structure — the editor becomes the page's second level (list ── editor), reusing settingsRouteLevel and the back-header Toolbar. RouteHeader, private to ProvidersPanel, moves to settings-route-header.tsx as the one way back from any settings sub-level, so the shape is not written twice.

Zero page CSS — all three rules are gone, each a restatement of something the kit or Astryx already draws: the flex-wrap action cluster is the row end slot, the oklch-tinted callout is Banner status="warning", the flex-end button row is SettingsActions.

Fewer elements

  • A row carries the name, the guidance, and a badge only when the preset cannot be selected — a list where every row says 可用 says nothing. Route, id, and capability boundary are the editor's answer, one level in.
  • The list group loses its title and lede, which restated the page heading verbatim. The 64-preset ceiling speaks only once it is hit.
  • 立即启用 leaves the editor: whether the main agent may select a preset is the list row's switch, which was already answering it.
  • A new preset's subagent_id starts empty (typing the name fills it) instead of pre-deriving subagent; a saved one reads as a settled row value, not a disabled input.
  • Delete moves out of the row into its own trailing section, so nothing quiet sits beside the destructive action.

Presentation only: availability, validation, id immutability, and the settings.update({ subagents }) contract are unchanged.

Refs #1999. This is the first of three planned steps; the delegation runtime surface (workbar Subagents tab, sidebar convergence) is #1457 and lands separately.

Verification

typecheck · lint · format:check · check-dead-css · check-a11y/check-copy/check-console · 1379 desktop unit tests · Storybook smoke (67 manifest checks, 75 catalog renders) · settings.spec.ts + providers.spec.ts e2e (5 passed).

The e2e journey now walks the route level instead of a dialog. Stories gain SubagentEditorExisting, covering the three states the create form cannot show: the settled read-only subagent_id, the implementation capability warning, and the delete section.

@Astro-Han
Astro-Han force-pushed the redesign/subagent-settings-astryx-idiom branch from 4c602af to 746fcf4 Compare August 3, 2026 12:34
Astro-Han added a commit that referenced this pull request Aug 3, 2026
…e what the rebuild dropped

Three-way review of #2008 (codex + two agents) against the providers panel it
copies. The rebuild took that panel's route-level shell without its route
controller, and traded away two capabilities to look simpler.

Route controller, at the owner:
- focus follows the level. The level carried `tabIndex={-1}` and a comment
  claiming it takes focus on landing, but nothing ever called `focus()`:
  clicking the chevron unmounted the button that had it, so a keyboard user
  restarted from `document.body` on every move. The Dialog this replaced got
  this for free. Entering focuses the level (not the back button — an
  IconButton pops its tooltip on focus), returning goes back to the row the
  user left from, falling back to the add button when that row is gone, and
  first render does not pull focus off the settings nav at all.
- an edit route whose preset vanished used to render as a BLANK CREATE FORM
  that appended a second preset on save, because "no presetId" and "presetId
  that no longer resolves" were the same `null`. The route is now a
  discriminated union and an unsatisfiable edit derives back to the list, as
  ProvidersPanel:194 already spells out.

Restored:
- a preset can again be created disabled. Dropping the editor switch left a
  real window where a just-saved preset is selectable before the user can
  reach the list row, and the reviewers were right that two controls over one
  state are two entrances, not two authorities. The switch is back on create
  only; for an existing preset the header states the disabled fact via the
  badge slot providers already uses for its default connection.
- `SubagentEditor` story. It was deleted under a rule written in this same PR,
  worded widely enough to cover a whole second route level — the wrong
  direction. `play` does run in the browser, so that story was the editor's
  only automated render; without it the entire `preset === null` branch and
  every degraded-route state rendered nowhere.

Corrections to what this PR asserted:
- FIDELITY.md claimed the smoke script renders every story at three viewports.
  It renders the seven manifest surfaces that way and every other story once,
  at 1280 light (`catalogJobs`). The rule is rewritten around what actually
  renders, and the "play does not execute" section is corrected to what is
  true: it runs, nothing collects its assertions.
- the id read-only row hand-rolled `<code class="settingsReadOnlyValue">`,
  missing `data-mono`, so a 128-char id rendered as body type in a 320px
  right-anchored box. It uses the kit's `SettingRow mono` path.
- the list section header lost its title, leaving an empty header with a
  divider under it and dropping the only count under a 64-preset ceiling.
  Title and count are back, and the ceiling reads from MAX_SUBAGENT_PRESETS
  rather than a hardcoded 64 in copy.
- the empty state showed two identical add buttons; the section action now
  yields to the EmptyState CTA.
- three imports left dead in ProvidersPanel by the RouteHeader extraction
  (biome has no noUnusedImports, tsc no noUnusedLocals — CI cannot see them).
- `settingsSubagentsPage` was a class with no rule, on a page whose header
  says it owns no CSS; the save/cancel row was a title-less SettingsSection
  rendering neither header nor divider around one HStack; the danger section
  repeated the confirm dialog's sentence verbatim.

Tests, for the regressions that shipped green:
- e2e seeds the preset DISABLED, so saving an unrelated field proves
  `enabled` survives — the single line holding up the switch removal.
- a create journey: id derives from the name, create-disabled persists as
  false, and the list row's switch enables it.
- focus assertions on both level changes and on arrival.
- `nextSubagentDraftForName` extracted and unit-tested, including the
  `idWasEdited` takeover the UI wiring had no coverage for.
- availability now asserts `tone` with `kind`; a broken route quietly turning
  green was a pure-data regression with no other home.
- the story fixture gains an enabled preset whose connection is gone, so the
  destructive badge renders somewhere (the disabled one short-circuits first).

Verified: typecheck, lint, format:check, check-dead-css, check-story-
annotations, check-a11y/copy/console, 1382 unit tests, Storybook smoke, and
settings + providers e2e (6 passed).
Astro-Han added a commit that referenced this pull request Aug 3, 2026
…e what the rebuild dropped

Three-way review of #2008 (codex + two agents) against the providers panel it
copies. The rebuild took that panel's route-level shell without its route
controller, and traded away two capabilities to look simpler.

Route controller, at the owner:
- focus follows the level. The level carried `tabIndex={-1}` and a comment
  claiming it takes focus on landing, but nothing ever called `focus()`:
  clicking the chevron unmounted the button that had it, so a keyboard user
  restarted from `document.body` on every move. The Dialog this replaced got
  this for free. Entering focuses the level (not the back button — an
  IconButton pops its tooltip on focus), returning goes back to the row the
  user left from, falling back to the add button when that row is gone, and
  first render does not pull focus off the settings nav at all.
- an edit route whose preset vanished used to render as a BLANK CREATE FORM
  that appended a second preset on save, because "no presetId" and "presetId
  that no longer resolves" were the same `null`. The route is now a
  discriminated union and an unsatisfiable edit derives back to the list, as
  ProvidersPanel:194 already spells out.

Restored:
- a preset can again be created disabled. Dropping the editor switch left a
  real window where a just-saved preset is selectable before the user can
  reach the list row, and the reviewers were right that two controls over one
  state are two entrances, not two authorities. The switch is back on create
  only; for an existing preset the header states the disabled fact via the
  badge slot providers already uses for its default connection.
- `SubagentEditor` story. It was deleted under a rule written in this same PR,
  worded widely enough to cover a whole second route level — the wrong
  direction. `play` does run in the browser, so that story was the editor's
  only automated render; without it the entire `preset === null` branch and
  every degraded-route state rendered nowhere.

Corrections to what this PR asserted:
- FIDELITY.md claimed the smoke script renders every story at three viewports.
  It renders the seven manifest surfaces that way and every other story once,
  at 1280 light (`catalogJobs`). The rule is rewritten around what actually
  renders, and the "play does not execute" section is corrected to what is
  true: it runs, nothing collects its assertions.
- the id read-only row hand-rolled `<code class="settingsReadOnlyValue">`,
  missing `data-mono`, so a 128-char id rendered as body type in a 320px
  right-anchored box. It uses the kit's `SettingRow mono` path.
- the list section header lost its title, leaving an empty header with a
  divider under it and dropping the only count under a 64-preset ceiling.
  Title and count are back, and the ceiling reads from MAX_SUBAGENT_PRESETS
  rather than a hardcoded 64 in copy.
- the empty state showed two identical add buttons; the section action now
  yields to the EmptyState CTA.
- three imports left dead in ProvidersPanel by the RouteHeader extraction
  (biome has no noUnusedImports, tsc no noUnusedLocals — CI cannot see them).
- `settingsSubagentsPage` was a class with no rule, on a page whose header
  says it owns no CSS; the save/cancel row was a title-less SettingsSection
  rendering neither header nor divider around one HStack; the danger section
  repeated the confirm dialog's sentence verbatim.

Tests, for the regressions that shipped green:
- e2e seeds the preset DISABLED, so saving an unrelated field proves
  `enabled` survives — the single line holding up the switch removal.
- a create journey: id derives from the name, create-disabled persists as
  false, and the list row's switch enables it.
- focus assertions on both level changes and on arrival.
- `nextSubagentDraftForName` extracted and unit-tested, including the
  `idWasEdited` takeover the UI wiring had no coverage for.
- availability now asserts `tone` with `kind`; a broken route quietly turning
  green was a pure-data regression with no other home.
- the story fixture gains an enabled preset whose connection is gone, so the
  destructive badge renders somewhere (the disabled one short-circuits first).

Verified: typecheck, lint, format:check, check-dead-css, check-story-
annotations, check-a11y/copy/console, 1382 unit tests, Storybook smoke, and
settings + providers e2e (6 passed).
@Astro-Han
Astro-Han force-pushed the redesign/subagent-settings-astryx-idiom branch from 1c8b09d to 3d9e8dd Compare August 3, 2026 14:24
… route idiom

#1999 shipped 设置 · 子 Agent as a list of dense rows over a 560px Dialog
holding eight fields behind an inner scrollbar, plus three hand-written
`.subagentPreset*` CSS rules. A modal exists to interrupt the current task
for something short and immediately decidable; naming a capability, writing
the guidance the main agent selects on, and picking a connection/model/
thinking route is none of those — Astryx says the same ("if the content
grows beyond what fits, consider a full page instead"), and the providers
panel next door already answers this exact shape with a route level.

- the editor becomes the page's second level (list ── editor), reusing
  `settingsRouteLevel` + the back-header Toolbar rather than a Dialog;
- `RouteHeader`, private to ProvidersPanel, moves to `settings-route-header`
  as the one way back from a settings sub-level, so the shape is not
  written twice;
- the page drops all three CSS rules: the flex-wrap action cluster is the
  row end slot, the oklch-tinted callout is `Banner status="warning"`, and
  the flex-end button row is a plain action row;
- rows carry the name, the guidance, and a badge ONLY when the preset
  cannot be selected — a list where every row says 可用 says nothing. The
  route, the id, and the capability boundary are the editor's answer;
- the list group loses its title and lede, which restated the page heading
  verbatim; the preset ceiling speaks only once it is hit;
- 立即启用 leaves the editor: whether the main agent may select a preset is
  the list row's switch, which was already answering it;
- a new preset's subagent_id starts empty (the name fills it) instead of
  pre-deriving `subagent`, and a saved one reads as a settled row value
  rather than a disabled input;
- delete moves out of the row into its own trailing section, so nothing
  quiet sits beside the destructive action.

Presentation only: availability, validation, id immutability, and the
`settings.update({ subagents })` contract are unchanged.

Verified: typecheck, lint, format:check, check-dead-css, check-a11y/copy/
console, 1379 desktop unit tests, Storybook smoke (75 renders), and
settings + providers e2e. Story gains SubagentEditorExisting for the
read-only id, the implementation warning, and the delete section.
…er subagent level

Two follow-ups from reviewing the rebuilt page in Storybook.

The settings story harness wrapped `SettingsSurface` in `height: '100%'`.
`SettingsSurface` is a `Layout height="fill"`, which needs a bounded
ancestor before it hands its content pane a scroll box; under Storybook's
fullscreen body a percentage height resolves against an auto-height parent,
so every page taller than the frame stretched the whole surface instead of
scrolling inside it. 权限与能力 reached 1942px in a 720px frame with no way
down, and 记忆 and the subagent editor were stuck the same way — a
pre-existing hole that only shows on a page long enough to need it.
`100dvh` restores the scroll box: the editor's content pane now measures
582/1005 with `overflow-y: auto`, and the delete section is reachable.

The subagent page also arrived with four stories where the settings
convention is one per page plus a variant for a state the page cannot
otherwise show. The narrow variant had nothing to say now that the row's
trailing cluster is a switch and a chevron, and the create form is the
edit form minus three elements, so both are gone: 子 Agent keeps its list
story and one editor story, on the preset that exercises the most of it.

Verified: typecheck, lint, Storybook smoke (67 manifest checks, 73 catalog
renders across three viewports), and a scroll probe on the editor level.
…l in e2e

The editor story was the list story plus one click — same bridge, same
render, one `play`. A reviewer reaches the same screen by clicking the row,
so 子 Agent keeps one story, as the settings convention has it.

What the story was covering, the e2e now states outright: the level carries
the settled subagent_id and the delete action, the two things the list row
deliberately does not show. That path already ran in a real Electron window;
it just was not asserting the level's shape.

Verified: typecheck, lint, Storybook smoke (72 catalog renders), settings
e2e (3 passed).
The subagent page arrived with four stories for one surface. FIDELITY.md
is where the Product/* story conventions live, so the rule goes there
rather than into a second document beside it: a surface gets one story,
and earns another only for a state that story cannot reach.
…e what the rebuild dropped

Three-way review of #2008 (codex + two agents) against the providers panel it
copies. The rebuild took that panel's route-level shell without its route
controller, and traded away two capabilities to look simpler.

Route controller, at the owner:
- focus follows the level. The level carried `tabIndex={-1}` and a comment
  claiming it takes focus on landing, but nothing ever called `focus()`:
  clicking the chevron unmounted the button that had it, so a keyboard user
  restarted from `document.body` on every move. The Dialog this replaced got
  this for free. Entering focuses the level (not the back button — an
  IconButton pops its tooltip on focus), returning goes back to the row the
  user left from, falling back to the add button when that row is gone, and
  first render does not pull focus off the settings nav at all.
- an edit route whose preset vanished used to render as a BLANK CREATE FORM
  that appended a second preset on save, because "no presetId" and "presetId
  that no longer resolves" were the same `null`. The route is now a
  discriminated union and an unsatisfiable edit derives back to the list, as
  ProvidersPanel:194 already spells out.

Restored:
- a preset can again be created disabled. Dropping the editor switch left a
  real window where a just-saved preset is selectable before the user can
  reach the list row, and the reviewers were right that two controls over one
  state are two entrances, not two authorities. The switch is back on create
  only; for an existing preset the header states the disabled fact via the
  badge slot providers already uses for its default connection.
- `SubagentEditor` story. It was deleted under a rule written in this same PR,
  worded widely enough to cover a whole second route level — the wrong
  direction. `play` does run in the browser, so that story was the editor's
  only automated render; without it the entire `preset === null` branch and
  every degraded-route state rendered nowhere.

Corrections to what this PR asserted:
- FIDELITY.md claimed the smoke script renders every story at three viewports.
  It renders the seven manifest surfaces that way and every other story once,
  at 1280 light (`catalogJobs`). The rule is rewritten around what actually
  renders, and the "play does not execute" section is corrected to what is
  true: it runs, nothing collects its assertions.
- the id read-only row hand-rolled `<code class="settingsReadOnlyValue">`,
  missing `data-mono`, so a 128-char id rendered as body type in a 320px
  right-anchored box. It uses the kit's `SettingRow mono` path.
- the list section header lost its title, leaving an empty header with a
  divider under it and dropping the only count under a 64-preset ceiling.
  Title and count are back, and the ceiling reads from MAX_SUBAGENT_PRESETS
  rather than a hardcoded 64 in copy.
- the empty state showed two identical add buttons; the section action now
  yields to the EmptyState CTA.
- three imports left dead in ProvidersPanel by the RouteHeader extraction
  (biome has no noUnusedImports, tsc no noUnusedLocals — CI cannot see them).
- `settingsSubagentsPage` was a class with no rule, on a page whose header
  says it owns no CSS; the save/cancel row was a title-less SettingsSection
  rendering neither header nor divider around one HStack; the danger section
  repeated the confirm dialog's sentence verbatim.

Tests, for the regressions that shipped green:
- e2e seeds the preset DISABLED, so saving an unrelated field proves
  `enabled` survives — the single line holding up the switch removal.
- a create journey: id derives from the name, create-disabled persists as
  false, and the list row's switch enables it.
- focus assertions on both level changes and on arrival.
- `nextSubagentDraftForName` extracted and unit-tested, including the
  `idWasEdited` takeover the UI wiring had no coverage for.
- availability now asserts `tone` with `kind`; a broken route quietly turning
  green was a pure-data regression with no other home.
- the story fixture gains an enabled preset whose connection is gone, so the
  destructive badge renders somewhere (the disabled one short-circuits first).

Verified: typecheck, lint, format:check, check-dead-css, check-story-
annotations, check-a11y/copy/console, 1382 unit tests, Storybook smoke, and
settings + providers e2e (6 passed).
…s to

Second review round on this PR found the page disagreeing with three owners
at once, so this fixes them at those owners rather than at the symptoms.

The store: `normalizeSubagentSettings` DROPS a preset it dislikes instead of
rejecting the write. A name past 128 characters therefore resolved the promise,
returned the user to the list, and deleted the row they had just edited — with
no error anywhere. The limits are now exported from core and enforced where the
fields change, and `persist` reads the result back instead of trusting a
resolved promise, which also covers a preset created into a list that filled up
elsewhere. Conversely the store makes `description` optional and the list has a
fallback line for it, so requiring it here made a legal preset uneditable until
the user wrote prose; that requirement is gone.

The id: an existing preset's id now comes from the preset at submit time, not
from the draft. `idWasEdited` was the only thing standing between a rename and
a silently re-keyed preset, and the branch it guarded does not even render the
field — the flag disarmed a trap that no longer exists.

The route controller: `useSettingsRouteFocus` is now shared with ProvidersPanel,
which had the same 48 lines of arrive-vs-navigate, rAF, return-focus and
fallback logic written out separately. A focus rule is the one duplication that
cannot be seen on screen when the two copies drift. The unsatisfiable-edit-route
rule moves out with it into `resolveSubagentRoute`, where it can be tested — its
regression (a vanished preset rendering the create branch, so saving appended a
second preset) had no coverage at all.

Also: `enabled` had three authorities (a list switch, a create-only switch, and
a read-only badge on the level that owns deletion) — the editor now carries it
in both branches and the badge is gone; the route's two choices get two errors
instead of one aimed at the connection; `contract` was a required prop with
zero consumers; `.settingsRouteLevel` moves out of the models stylesheet to the
shared one, since it now has two owners.

Tests: the rename that could re-key a preset, the delete flow in both
directions (cancel keeps it; confirm returns to the list and lands focus on the
fallback), and arrival focus asserted positively on the settings nav item.
FIDELITY.md said a story whose `play` throws "still ships green" and "never
could" fail. It is the opposite: `storybook-visual-smoke.mjs` subscribes to
`playFunctionThrewException` and `unhandledErrorsWhilePlaying` and collects the
console error too, so a throwing play function fails the smoke run and CI —
verified by adding a probe story that throws unconditionally and watching the
run exit 1. The script's own header says the same thing from the other side:
that pass exists BECAUSE a build alone would let such a story through.

What `play` genuinely cannot do is report: with no test addon a failure names
the story, not the assertion, the state, or the expectation — and reaching it
costs a Storybook build. That is the reason to put contracts elsewhere, and it
is what the section now says.

The viewport claim was overstated in the same paragraph: manifest surfaces
declare their own viewports and colour schemes, with a written opt-out for each
one skipped, rather than all rendering at three widths across light and dark.

The subagent editor story's annotation claimed it "exercises" four states it
only renders; the e2e journeys are what pin them.
…y drifts

Third review round. The hook landed with eight options, three of which the
subagent page degenerated (a `focusSelectors` that ignores its argument, no
`routeKey`, no `isReady`) — and the total came out longer than the duplication
it removed. It now shares only the mechanism that drifts invisibly: skip the
first render, one animation frame, `preventScroll`, cancel on the way out. Which
element a level focuses goes back to the page, because a wrong target puts the
ring somewhere you can see. Its `resolveTarget` is also read through a ref
written in an effect rather than during render.

Deleting the effect that committed the derived route back into state: it was the
same rule as `resolveSubagentRoute` said twice, it contradicted ProvidersPanel —
which answers the identical vanished-target case by deriving alone — and it had
a window where a click landing between commit and effect would be overwritten.

The three claims the diff made that the code did not support:

- Three e2e comments said the editor no longer carries an enable switch for an
  existing preset. It carries one in both branches; that was this PR's own last
  round. The tests are fine, the reasons written on them were not.
- "An IconButton opens its tooltip on focus, so focusing one on arrival would
  pop a tooltip at every mouse user" is false in both files that said it:
  Astryx's `handleFocusIn` returns early unless `:focus-visible` matches, and
  says so in a comment. Focusing the region is still right; the reason is that
  a named region is what gets announced.
- The unit test explained itself with a bug that only existed in this PR's own
  intermediate commit, which no reader after merge can see. It now states the
  present-tense structure: `preset: null` IS the create branch.

Coverage: breaking any of the three providers focus branches left CI green, so
the catalog landing, the detail region landing, and the return-to-row are now
asserted there — each verified to fail when its branch is broken. The create
journey also takes the id over before renaming, which is the half of the
derivation contract the component never exercised.

Also: an existing preset's id is no longer derived at the call site either, not
just ignored at submit; the back affordance is disabled mid-save, where leaving
discarded a draft the failed write could not give back; a thinking level whose
row is off screen is no longer submitted; the id limit is interpolated from
`SUBAGENT_PRESET_ID_MAX_CHARS` instead of spelled out in two locales; name and
description truncate the way the store measures them, after trimming; and the
providers detail is a named region too, since it now shares this header.
@Astro-Han
Astro-Han force-pushed the redesign/subagent-settings-astryx-idiom branch from 3d9e8dd to 86b02b5 Compare August 3, 2026 15:02
@Astro-Han
Astro-Han marked this pull request as ready for review August 3, 2026 15:06
@Astro-Han
Astro-Han merged commit df6cc8c into main Aug 3, 2026
11 checks passed
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.

1 participant