Skip to content

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) - #543

Merged
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations
Jul 5, 2026
Merged

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5)#543
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

PR5 of #520 — the style-hook convention foundation + the smaller Base UI migrations that follow it. Four items, one commit each (each independently revertible), TDD red → green → refactor.

#22 (input canonical) was scoped into PR5 but moved to PR8 during this work — primitives/input.tsx carries a different structure (data-slot="input-control" span + inner Base UI input, ring-[3px]) than the native ui.tsxInput (single <input> + inputClasses, ring-2), and bare-field-chrome-contract.test.ts pins the native's data-maka-field-chrome + border-input + ring-2, so retiring the native is not a clean look-preserving swap. The issue plan was re-arranged (#22 → PR8); this PR opens with 1-4 only.

New primitives (packages/ui/src/primitives/)

PrimitiveWrapsdata-slot
collapsible.tsx@base-ui/react/collapsible Root/Trigger/Panelcollapsible / collapsible-trigger / collapsible-panel
tooltip.tsx@base-ui/react/tooltip Root/Trigger/Content (Portal+Positioner+Popup)tooltip / tooltip-trigger / tooltip-content
number-field.tsx@base-ui/react/number-field Root/Inputnumber-field / number-field-input

All follow the style-hook convention (item 23): data-slot on each part, Base UI's native boolean state attributes (data-panel-open, data-open, etc.) adopted as-is. The Collapsible Trigger carries a button-chrome reset (the native <summary> had no bg/border/padding, but <button> does). The Tooltip Trigger uses Base UI v1's render prop (not Radix asChild). The NumberField Input reuses maka's inputClasses (now exported from ui.tsx) so the port inputs keep their look.

Convergence

  • item 23 — style-hook convention: data-slot added to all 20 Base UI wrappers in ui.tsx (Button/Separator/Checkbox/Dialog backdrop+popup/Select Trigger+List+Popup+Group+GroupLabel+Separator+Item/FieldDescription/Label/Switch/Toggle/ToggleGroup/RadioGroup/Radio/Progress). A doc comment at the top of ui.tsx records the state-attribute decision (adopt Base UI native attribute-presence form — maka's renderer CSS has zero state-attribute selectors today, so nothing breaks), the per-component hook map, the whitelisted CSS var hooks, and the className(state) deferral.
  • item 17 — disclosure → Collapsible: the four <details>/<summary> sites (chat-view turn-thinking + reasoning-panel, permission-dialog permission-raw, tool-activity) migrate to Base UI Collapsible (all independent single sections, so Collapsible not Accordion). CSS retargeted: summary[data-slot="collapsible-trigger"], [open][data-panel-open], ::-webkit-details-marker/::marker resets dropped (button trigger has no native marker). The reasoning-panel's controlled-open moves from e.currentTarget.open off the toggle event to Collapsible's open + onOpenChange.
  • item 20 — tooltip → Base UI Tooltip: the 13 clearest icon-only-action button title= usages (app-shell-chrome-actions 7, browser-panel 4, artifact-pane collapse) migrate to Base UI Tooltip; aria-label stays on the button, TooltipContent is the visual hover hint. The artifact-pane delete button's redundant title= is dropped (it shows a visible 删除 label). The longer tail (truncation spans, SelectTrigger titles, status badges, OnboardingHero submit) are tooltip-eligible but need per-site label-vs-tooltip judgment — deferred to a follow-up.
  • item 21 — number-field: the two gateway/proxy port inputs migrate to Base UI NumberField (value: number | null, onValueChange), dropping the Number(event.currentTarget.value) hand-conversion. Proxy: 0 ↔ empty; gateway: empty → 3939 default, both preserved.

New contracts

ContractBansPins
style-hook-convention-contract.test.tsa ui.tsx wrapper forwarding to <Base*> without data-slotthe convention doc comment present
disclosure-collapsible-contract.test.ts<details>/<summary> (incl. in comments) in the 3 migrated files; missing Collapsible importprimitives/collapsible.tsx data-slot on Root/Trigger/Panel
tooltip-converge-contract.test.tsa title= JSX attribute in the 3 migrated files; missing Tooltip importprimitives/tooltip.tsx data-slot on Root/Trigger/Content
number-field-converge-contract.test.tsNumber(event.currentTarget.value) hand-conversion in the 2 port files; missing NumberField importprimitives/number-field.tsx data-slot on Root/Input

Existing tests updated

  • border-width-converge-contract: the triangle-caret allowlist selectors moved from summary::before to [data-slot="collapsible-trigger"]::before (the chevron border-width: 4px 0 4px 5px moved with the disclosure migration).
  • chat-marker-cascade-contract / chat-tool-card-cascade-contract: the turn-thinking / tool-card residue selectors + the dropped marker-reset residue.
  • app-region-hygiene-contract / search-modal-lifecycle-contract: the topbar-button className extraction + the shell-search-button regex now match the <TooltipTrigger render={<UiButton/>}> shape (the className moved from the UiButton element to the TooltipTrigger that renders it).
  • settings-network-gateway-contract: the two port-input assertions now pin the NumberField form (value={...|| null} / value={...} + onValueChange={(v) => update…({ port: v ?? default })).

Commits

  1. feat(ui): establish Base UI style-hook convention (data-slot + native state attrs) — item 23 (foundation).
  2. feat(ui): migrate disclosure sites to Base UI Collapsible — item 17.
  3. feat(ui): migrate icon-action tooltips to Base UI Tooltip — item 20.
  4. feat(ui): migrate port inputs to Base UI NumberField — item 21.

Verification

  • Tests: npm run -w @maka/desktop test — 2008/2008 pass (rebased onto main with PR2; +26 from PR2, +10 new contract assertions across 4 files). typecheck clean.
  • Screenshots vs main (turn-narrative, settings-appearance, first-run × light): tooltips are hover-only so rest state is unchanged; turn-narrative 0.09% (the disclosure commit's summary→button pixel difference), settings-appearance 0.0136 (PR4's settings-nav snap residual, untouched here), first-run 0. The port inputs (general/open-gateway settings) have no capture scenario; they keep their look via the shared inputClasses.

Out of scope / deferred

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review. Verified every claim against the code before acting; results + fixes below.

P2 — tool-activity defaultOpen (fixed, c06d50a9)

Verified. The disclosure migration set defaultOpen={isOpenByDefault(item.status)} (uncontrolled — open decided once on first render). The pre-Collapsible <details open={isOpenByDefault(status)}> re-evaluated open every render, so a card that defaulted open while pending/running no longer auto-collapsed when it settled. Real regression.

Fix: extracted a ToolActivityCard component (hooks can't live in the .map callback) with a controlled state machine: useState(isOpenByDefault(item.status)) seeds open, useEffect re-syncs it on [item.status] so a pending/running card collapses when it settles, onOpenChange={setOpen} lets the user toggle in between (the next status change re-syncs). This restores the status-following behavior and makes the user-toggle intentional rather than a React-not-rerendering side effect.

Test:disclosure-collapsible-contract now asserts tool-activity must NOT use defaultOpen, must use onOpenChange, and must re-sync via useEffect on [item.status]. (maka has no React testing-library / react-test-renderer, so the behavior is locked structurally + verified by hand; a rerender-act test is out of scope.) Will hand-test pending/running → completed/interrupted before merge.

P3 — Collapsible barrel / contract mismatch (fixed, f4dba2c2)

Verified. Barrel exported Collapsible as PrimitiveCollapsible (the old Tabs/Accordion convention), while Tooltip/NumberField export directly. The contract regex allowed @maka/ui as an import path, but the barrel didn't export Collapsible under that name — a phantom-pass path. The 3 internal sites import via ./primitives/collapsible.js (the in-package convention — alert/chat all use relative primitive paths), so the test passed for the right reason, but the @maka/ui allowance was misleading.

Fix: barrel now exports Collapsible / CollapsibleTrigger / CollapsiblePanel directly (dropped the Primitive* aliases and the CollapsiblePrimitive re-export — nothing consumes them; Tooltip/NumberField don't export their Primitive either). Internal sites keep their relative-path imports (convention); @maka/ui is now genuinely available rather than phantom.

P3 — tooltip.tsx doc showed asChild (fixed, f4dba2c2)

Verified. Line 14 usage doc showed <TooltipTrigger asChild> (Radix), but this PR uses Base UI v1's render prop. Fixed the example to render={<Button />} and noted the Trigger merges its props + children into the rendered element, so the next person doesn't copy the comment and hit the typecheck failure the migration hit.

待验证 — artifact delete title= (fixed, 28f6ed7a)

Partially verified — pushing back on the a11y part. The .maka-artifact-toolbar-destructive-label span is visually hidden but still announced (position:absolute; width:1px; clip-path:inset(50%); CSS comment: "only exists for assistive tech"). So the screen-reader path was preserved when title="删除" was dropped — no SR regression. But mouse users did lose the hover hint (native title= gone, no Base UI Tooltip added).

Fix: wrapped the delete button in a Base UI Tooltip (same shape as the artifact-pane collapse button). TooltipContent mirrors 删除/删除中… for hover; the visually-hidden span still carries the SR label. The two are complementary, not redundant.

P3 — tooltip three-layer JSX repetition (deferred)

Verified. 13 sites are highly isomorphic (<Tooltip><TooltipTrigger render={<Button variant="quiet" size="icon-sm"/>} ...props><Icon/></TooltipTrigger><TooltipContent>label</TooltipContent></Tooltip>).

Defering this one. A TooltipButton helper would save ~15-27 lines net, but the 13 sites' trigger props are heterogeneous (disabled / aria-expanded / conditional icon / conditional label / UiButton vs Button), so the helper becomes a pass-through wrapper (tooltip + render + {...triggerProps} + children) that mostly removes the <Tooltip>/<TooltipContent> wrapping — a modest win for a new abstraction layer + an app-region-hygiene regex adjustment (it extracts classNames from <TooltipTrigger render={<UiButton …/>}> blocks today). Happy to do it as a follow-up if the tooltip site count grows; for 13 sites the direct form is clearer and more flexible. Pushing back rather than adding abstraction for line-count.

待验证 — CI checks / hand-test

Local verification done:npm run -w @maka/desktop test → 2009/2009 pass. npm run typecheck clean across workspaces. Screenshots vs main: turn-narrative 0.09% (the disclosure commit's summary→button residual), settings-appearance 0.0136 (PR4's settings-nav snap, untouched here), first-run 0% — all hover-only tooltip / controlled-open changes add 0 at rest.

CI checks show 0 — will confirm CI triggers on the updated push. Will hand-test before merge: Collapsible keyboard toggle + status-following collapse, Tooltip hover/focus, NumberField empty/invalid input (proxy 0↔empty, gateway empty→3939).


Summary: 3 fix commits pushed (c06d50a9, f4dba2c2, 28f6ed7a). 1 item deferred with rationale (tooltip helper). Issue #520 re-arranged: #22 input-canonical moved to PR8 (not a clean look-preserving swap — primitives/input.tsx carries a different structure than the native ui.tsxInput, and bare-field-chrome-contract pins the native's data-maka-field-chrome + border-input + ring-2).

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
Astro-Han added a commit that referenced this pull request Jul 5, 2026
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
Astro-Han added 10 commits July 5, 2026 17:50
… state attrs) (#520 PR5 item 23)
Foundation commit for PR5 — the convention the disclosure / tooltip /
number-field / input migrations in the following commits follow.
data-slot: add data-slot="<name>" to all 20 Base UI wrappers in ui.tsx
(Button / Separator / Checkbox / DialogBackdrop / DialogPopup / Select
Trigger/List/Popup/Group/GroupLabel/Separator/Item / FieldDescription /
Label / Switch / Toggle / ToggleGroup / RadioGroup / Radio / Progress).
The ./primitives/ wrappers (accordion / alert / badge / …) already do
this; ui.tsx wrappers did not. data-slot is a stable CSS targeting hook
that survives className drift. Base UI does not emit data-slot itself, so
the wrapper owns it. No CSS targets these yet — this is forward-looking
convention, a visual no-op.
State-attribute form: adopt Base UI's native attribute-presence form
([data-active] / [data-open] / [data-checked] / [data-selected] /
[data-pressed] / [data-highlighted] / [data-disabled]), NOT the
attribute-value form ([data-active="true"]). Maka's renderer CSS has
zero state-attribute selectors today, so there is nothing to break and no
override layer to maintain. A per-component hook map (Tabs data-active /
Select data-[highlighted]/data-[selected] / Checkbox/Switch/Radio
data-[checked] / Toggle data-[pressed] / Dialog/Tooltip/Popover data-[open])
is documented in a doc comment at the top of ui.tsx, along with the
whitelisted CSS var hooks (--anchor-* / --available-* / --active-tab-*).
className(state) function form is deferred — add only when a migration in
this PR actually needs state-based classes.
style-hook-convention-contract.test.ts locks the data-slot rule: every
ui.tsx wrapper that forwards to a <Base*> component must carry data-slot,
and the convention doc comment must stay present. Hand-written native
elements (legacy Input / Textarea / Badge) are out of scope until they
retire onto a Base UI primitive (input canonical, commit 5).
Verification: 1972/1972 desktop tests pass (+4 contract). typecheck clean.
renderer build clean. Visual no-op (data-slot is an attribute; no CSS
targets it yet).
…tem 17)
Migrate the four native <details>/<summary> disclosure sites onto Base UI
Collapsible, following the style-hook convention (item 23). The code comments
at the sites already said "future Base UI Accordion path"; all four are
independent single sections (not grouped), so Collapsible (not Accordion) is
the right primitive.
Sites:
- chat-view.tsx turn-thinking (maka-turn-thinking): uncontrolled, default
closed.
- chat-view.tsx reasoning-panel (maka-reasoning-panel): controlled open state
moves from reading e.currentTarget.open off the native toggle event to
Collapsible's open + onOpenChange props (default-open behavior preserved).
- permission-dialog.tsx permission-raw (maka-permission-raw): uncontrolled.
- tool-activity.tsx tool card ([data-slot="tool"]): uncontrolled with
defaultOpen = isOpenByDefault(status); the data-slot="tool" hook overrides
the Collapsible wrapper's default data-slot="collapsible" via prop spread,
so the existing [data-slot="tool"] CSS keeps working.
New primitive: packages/ui/src/primitives/collapsible.tsx wraps Base UI
Collapsible Root/Trigger/Panel with data-slot="collapsible" /
"collapsible-trigger" / "collapsible-panel". The Trigger carries a button
chrome reset (appearance-none / bg-transparent / border-0 / p-0 / font
inherit) so the summary row's clean look is preserved — native <summary> had
no background/border/padding, but <button> does. Exported from the barrel as
PrimitiveCollapsible / PrimitiveCollapsibleTrigger / PrimitiveCollapsiblePanel.
CSS retargeting (the DOM changed: <summary> -> <button> trigger, <details
open> -> Collapsible with data-panel-open on the trigger):
- .maka-turn-thinking summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-slot="collapsible-trigger"][data-panel-open]::before
- .maka-permission-raw > summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-panel-open] variants
- .maka-reasoning-panel-header marker resets dropped (button has no native
marker); [open] .maka-reasoning-panel-chevron ->
[data-slot="collapsible-trigger"][data-panel-open] .maka-reasoning-panel-chevron
- [data-slot="tool"] > summary ::-webkit-details-marker / ::marker resets
dropped (button trigger has no native marker).
- list-style: none rules dropped (no list marker on a button).
TDD: disclosure-collapsible-contract.test.ts written first (RED: sites still
used <details>, no primitive). GREEN: primitive + 4 migrations + CSS
retarget. The contract bans <details>/<summary> (incl. in comments) in the
three migrated files and requires a Collapsible import; it also pins the
primitive's data-slot on Root/Trigger/Panel.
Existing tests updated: border-width-converge-contract TRIANGLE_CARET_SELECTORS
(the chevron ::before border-width: 4px 0 4px 5px moved from summary::before to
[data-slot="collapsible-trigger"]::before); chat-marker-cascade-contract
(.maka-turn-thinking summary -> [data-slot="collapsible-trigger"]);
chat-tool-card-cascade-contract (the marker-reset residue on
[data-slot="tool"] > summary is gone — the trigger is a button now).
Verification: 1975/1975 desktop tests pass (+4 disclosure contract; the 3
existing contracts updated). typecheck clean. Screenshots vs clean main (PR4):
turn-narrative 0.09% (the summary->button swap's tiny pixel difference — button
baseline, the CollapsiblePanel wrapper), first-run 0%. No regression.
…tem 20)
Migrate the clearest icon-only-action button tooltips off the native title=
attribute (an unstyled, delayed browser tooltip) onto Base UI Tooltip,
which gives a themed, positioned, hover+focus tooltip matching the app.
Sites (13 buttons across 3 files):
- app-shell-chrome-actions.tsx: search / sidebar / new-task / feedback /
command-palette / help / health (7).
- browser-panel.tsx: back / forward / refresh / close (4).
- artifact-pane.tsx: the collapse toggle (1); the delete button's redundant
title= is dropped instead (it already shows a visible 删除 label, so a
tooltip would be a duplicate).
New primitive: packages/ui/src/primitives/tooltip.tsx wraps Base UI Tooltip
Root / Trigger / Content with data-slot="tooltip" / "tooltip-trigger" /
"tooltip-content". TooltipContent collapses Portal + Positioner + Popup
(the DialogContent shape) with a themed look (popover bg/corner/shadow +
z-overlay). The Trigger forwards via Base UI's render prop
(<TooltipTrigger render={<Button …/>} …props>…children…</TooltipTrigger>),
which merges the Trigger's props + children into the rendered Button — Base
UI v1 uses render, not the Radix asChild prop. Exported from the barrel as
Tooltip / TooltipTrigger / TooltipContent (no ui.tsx wrapper, so direct
names). aria-label stays on the button (accessible name); the TooltipContent
is the visual hover hint.
TDD: tooltip-converge-contract.test.ts written first (RED: the 3 files still
carried native title=, no primitive). GREEN: primitive + 13 migrations. The
contract bans a title= JSX attribute in the 3 migrated files and requires
a Tooltip import; it pins the primitive's data-slot on Root/Trigger/Content.
The regex \btitle= matches JSX attributes but not JS `const title =` (the
space before =).
Scope note: the longer tail of title= usages (truncation spans like
memory-settings path + chat-model-switcher chip, SelectTrigger titles,
status-badge icons, OnboardingHero's submit button) are tooltip-eligible but
each needs a label-vs-tooltip judgment and are not swept in this commit —
deferred to a follow-up. label-prop components (SettingRow / MetricCard /
SetupHero / Section) render title as visible text and are NOT tooltips;
they stay.
Existing tests updated: app-region-hygiene-contract's
extractStaticUiButtonClassNames now also matches the
<TooltipTrigger render={<UiButton/>}> shape (the className moved from the
UiButton element to the TooltipTrigger that renders it); search-modal-
lifecycle's shell-search-button regex now matches </TooltipTrigger> (the
UiButton is the render target, self-closing).
Verification: 1979/1979 desktop tests pass (+4 tooltip contract). typecheck
clean (render prop, not asChild). renderer build clean. Screenshots vs clean
main (PR4): turn-narrative 0.09% (the disclosure commit's residual; tooltips
are hover-only so rest state is unchanged), first-run 0%.
Migrate the two gateway/proxy port inputs off the native Input +
Number(event.currentTarget.value) hand-conversion onto Base UI NumberField,
which binds value: number | null directly and parses numeric input itself.
Sites:
- general-settings-page proxy port: value={proxyDraft.port || null}
(0 renders empty, matching the old String(port || '')), onValueChange
writes v ?? 0 (empty -> 0, matching the old Number(value) || 0).
- open-gateway-settings-page gateway port: value={gatewayDraft.port}
(0 renders "0", matching String(port)), onValueChange writes v ?? 3939
(empty -> 3939, matching Number(value) || 3939). inputMode="numeric" stays
on the input.
New primitive: packages/ui/src/primitives/number-field.tsx wraps Base UI
NumberField Root / Input with data-slot="number-field" / "number-field-input".
NumberFieldInput carries the maka standalone-input look via the shared
inputClasses export (the same classes the native ui.tsx Input used), so the
port inputs keep their look; unstyled gives the bare form for any future
Field/InputGroup embedding. Exported from the barrel as NumberField /
NumberFieldInput (no ui.tsx wrapper, so direct names, like Tooltip).
To share the maka input styling without duplicating it, ui.tsx exports
inputClasses + bareFieldClasses (const -> export const); the number-field
primitive imports them. (The input-canonical commit, item 22, will move
these into primitives/input.tsx and update this import.)
TDD: number-field-converge-contract.test.ts written first (RED: the two
files still carried Number(event.currentTarget.value), no primitive). GREEN:
primitive + 2 migrations. The contract bans the hand-conversion
(Number(event.currentTarget.value)) and requires a NumberField import; it
pins the primitive's data-slot on Root/Input. The hand-convert regex is
specific (Number(event.currentTarget.value)) so a different Number() call is
not flagged.
Existing test updated: settings-network-gateway-contract's two port-input
assertions now pin the NumberField form (value={...|| null} / value={...} +
onValueChange={(v) => update...({ port: v ?? default })}) instead of the old
String(value) + onChange Number(event.currentTarget.value). The intent —
"port input renders from the local draft while persisting in the background"
— is preserved; only the binding form changed. The "port input must not
lock after each digit" doesNotMatch still passes (the NumberFieldInput is not
disabled while saving).
Verification: 1982/1982 desktop tests pass (+3 number-field contract).
typecheck clean. renderer build clean. Screenshots: the port inputs are in
general/open-gateway settings (no capture scenario), so the captured
scenarios show no change — settings-appearance 0.0136 (PR4's settings-nav
snap residual) and turn-narrative 0.0009 (the disclosure commit's residual);
commit 4 adds 0. The port inputs keep their look via the shared inputClasses.
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
After rebasing PR5 onto main, the old PR5 style-hook commit still introduced a
SelectList wrapper, but main now has the ui.tsx dead-export contract and there
is no consumer for SelectList. Keep the style-hook convention on used wrappers
only and remove the dead export instead of adding a pending allowlist entry.
Verification: style-hook/divider/disclosure contracts pass (13/13), desktop
tests pass (2019/2019), typecheck clean.
@Astro-Han
Astro-Hanforce-pushed the feat/base-ui-small-migrations branch from 4a3c541 to 92fa1e1CompareJuly 5, 2026 09:53
@Astro-Han
Astro-Han merged commit cc3fc2c into mainJul 5, 2026
@Astro-Han
Astro-Han deleted the feat/base-ui-small-migrations branch July 5, 2026 09:54
Sign up for freeto 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

@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) by Astro-Han · Pull Request #543 · apache/maka · GitHub
Skip to content

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) - #543

Merged
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations
Jul 5, 2026
Merged

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5)#543
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

PR5 of #520 — the style-hook convention foundation + the smaller Base UI migrations that follow it. Four items, one commit each (each independently revertible), TDD red → green → refactor.

#22 (input canonical) was scoped into PR5 but moved to PR8 during this work — primitives/input.tsx carries a different structure (data-slot="input-control" span + inner Base UI input, ring-[3px]) than the native ui.tsxInput (single <input> + inputClasses, ring-2), and bare-field-chrome-contract.test.ts pins the native's data-maka-field-chrome + border-input + ring-2, so retiring the native is not a clean look-preserving swap. The issue plan was re-arranged (#22 → PR8); this PR opens with 1-4 only.

New primitives (packages/ui/src/primitives/)

PrimitiveWrapsdata-slot
collapsible.tsx@base-ui/react/collapsible Root/Trigger/Panelcollapsible / collapsible-trigger / collapsible-panel
tooltip.tsx@base-ui/react/tooltip Root/Trigger/Content (Portal+Positioner+Popup)tooltip / tooltip-trigger / tooltip-content
number-field.tsx@base-ui/react/number-field Root/Inputnumber-field / number-field-input

All follow the style-hook convention (item 23): data-slot on each part, Base UI's native boolean state attributes (data-panel-open, data-open, etc.) adopted as-is. The Collapsible Trigger carries a button-chrome reset (the native <summary> had no bg/border/padding, but <button> does). The Tooltip Trigger uses Base UI v1's render prop (not Radix asChild). The NumberField Input reuses maka's inputClasses (now exported from ui.tsx) so the port inputs keep their look.

Convergence

  • item 23 — style-hook convention: data-slot added to all 20 Base UI wrappers in ui.tsx (Button/Separator/Checkbox/Dialog backdrop+popup/Select Trigger+List+Popup+Group+GroupLabel+Separator+Item/FieldDescription/Label/Switch/Toggle/ToggleGroup/RadioGroup/Radio/Progress). A doc comment at the top of ui.tsx records the state-attribute decision (adopt Base UI native attribute-presence form — maka's renderer CSS has zero state-attribute selectors today, so nothing breaks), the per-component hook map, the whitelisted CSS var hooks, and the className(state) deferral.
  • item 17 — disclosure → Collapsible: the four <details>/<summary> sites (chat-view turn-thinking + reasoning-panel, permission-dialog permission-raw, tool-activity) migrate to Base UI Collapsible (all independent single sections, so Collapsible not Accordion). CSS retargeted: summary[data-slot="collapsible-trigger"], [open][data-panel-open], ::-webkit-details-marker/::marker resets dropped (button trigger has no native marker). The reasoning-panel's controlled-open moves from e.currentTarget.open off the toggle event to Collapsible's open + onOpenChange.
  • item 20 — tooltip → Base UI Tooltip: the 13 clearest icon-only-action button title= usages (app-shell-chrome-actions 7, browser-panel 4, artifact-pane collapse) migrate to Base UI Tooltip; aria-label stays on the button, TooltipContent is the visual hover hint. The artifact-pane delete button's redundant title= is dropped (it shows a visible 删除 label). The longer tail (truncation spans, SelectTrigger titles, status badges, OnboardingHero submit) are tooltip-eligible but need per-site label-vs-tooltip judgment — deferred to a follow-up.
  • item 21 — number-field: the two gateway/proxy port inputs migrate to Base UI NumberField (value: number | null, onValueChange), dropping the Number(event.currentTarget.value) hand-conversion. Proxy: 0 ↔ empty; gateway: empty → 3939 default, both preserved.

New contracts

ContractBansPins
style-hook-convention-contract.test.tsa ui.tsx wrapper forwarding to <Base*> without data-slotthe convention doc comment present
disclosure-collapsible-contract.test.ts<details>/<summary> (incl. in comments) in the 3 migrated files; missing Collapsible importprimitives/collapsible.tsx data-slot on Root/Trigger/Panel
tooltip-converge-contract.test.tsa title= JSX attribute in the 3 migrated files; missing Tooltip importprimitives/tooltip.tsx data-slot on Root/Trigger/Content
number-field-converge-contract.test.tsNumber(event.currentTarget.value) hand-conversion in the 2 port files; missing NumberField importprimitives/number-field.tsx data-slot on Root/Input

Existing tests updated

  • border-width-converge-contract: the triangle-caret allowlist selectors moved from summary::before to [data-slot="collapsible-trigger"]::before (the chevron border-width: 4px 0 4px 5px moved with the disclosure migration).
  • chat-marker-cascade-contract / chat-tool-card-cascade-contract: the turn-thinking / tool-card residue selectors + the dropped marker-reset residue.
  • app-region-hygiene-contract / search-modal-lifecycle-contract: the topbar-button className extraction + the shell-search-button regex now match the <TooltipTrigger render={<UiButton/>}> shape (the className moved from the UiButton element to the TooltipTrigger that renders it).
  • settings-network-gateway-contract: the two port-input assertions now pin the NumberField form (value={...|| null} / value={...} + onValueChange={(v) => update…({ port: v ?? default })).

Commits

  1. feat(ui): establish Base UI style-hook convention (data-slot + native state attrs) — item 23 (foundation).
  2. feat(ui): migrate disclosure sites to Base UI Collapsible — item 17.
  3. feat(ui): migrate icon-action tooltips to Base UI Tooltip — item 20.
  4. feat(ui): migrate port inputs to Base UI NumberField — item 21.

Verification

  • Tests: npm run -w @maka/desktop test — 2008/2008 pass (rebased onto main with PR2; +26 from PR2, +10 new contract assertions across 4 files). typecheck clean.
  • Screenshots vs main (turn-narrative, settings-appearance, first-run × light): tooltips are hover-only so rest state is unchanged; turn-narrative 0.09% (the disclosure commit's summary→button pixel difference), settings-appearance 0.0136 (PR4's settings-nav snap residual, untouched here), first-run 0. The port inputs (general/open-gateway settings) have no capture scenario; they keep their look via the shared inputClasses.

Out of scope / deferred

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review. Verified every claim against the code before acting; results + fixes below.

P2 — tool-activity defaultOpen (fixed, c06d50a9)

Verified. The disclosure migration set defaultOpen={isOpenByDefault(item.status)} (uncontrolled — open decided once on first render). The pre-Collapsible <details open={isOpenByDefault(status)}> re-evaluated open every render, so a card that defaulted open while pending/running no longer auto-collapsed when it settled. Real regression.

Fix: extracted a ToolActivityCard component (hooks can't live in the .map callback) with a controlled state machine: useState(isOpenByDefault(item.status)) seeds open, useEffect re-syncs it on [item.status] so a pending/running card collapses when it settles, onOpenChange={setOpen} lets the user toggle in between (the next status change re-syncs). This restores the status-following behavior and makes the user-toggle intentional rather than a React-not-rerendering side effect.

Test:disclosure-collapsible-contract now asserts tool-activity must NOT use defaultOpen, must use onOpenChange, and must re-sync via useEffect on [item.status]. (maka has no React testing-library / react-test-renderer, so the behavior is locked structurally + verified by hand; a rerender-act test is out of scope.) Will hand-test pending/running → completed/interrupted before merge.

P3 — Collapsible barrel / contract mismatch (fixed, f4dba2c2)

Verified. Barrel exported Collapsible as PrimitiveCollapsible (the old Tabs/Accordion convention), while Tooltip/NumberField export directly. The contract regex allowed @maka/ui as an import path, but the barrel didn't export Collapsible under that name — a phantom-pass path. The 3 internal sites import via ./primitives/collapsible.js (the in-package convention — alert/chat all use relative primitive paths), so the test passed for the right reason, but the @maka/ui allowance was misleading.

Fix: barrel now exports Collapsible / CollapsibleTrigger / CollapsiblePanel directly (dropped the Primitive* aliases and the CollapsiblePrimitive re-export — nothing consumes them; Tooltip/NumberField don't export their Primitive either). Internal sites keep their relative-path imports (convention); @maka/ui is now genuinely available rather than phantom.

P3 — tooltip.tsx doc showed asChild (fixed, f4dba2c2)

Verified. Line 14 usage doc showed <TooltipTrigger asChild> (Radix), but this PR uses Base UI v1's render prop. Fixed the example to render={<Button />} and noted the Trigger merges its props + children into the rendered element, so the next person doesn't copy the comment and hit the typecheck failure the migration hit.

待验证 — artifact delete title= (fixed, 28f6ed7a)

Partially verified — pushing back on the a11y part. The .maka-artifact-toolbar-destructive-label span is visually hidden but still announced (position:absolute; width:1px; clip-path:inset(50%); CSS comment: "only exists for assistive tech"). So the screen-reader path was preserved when title="删除" was dropped — no SR regression. But mouse users did lose the hover hint (native title= gone, no Base UI Tooltip added).

Fix: wrapped the delete button in a Base UI Tooltip (same shape as the artifact-pane collapse button). TooltipContent mirrors 删除/删除中… for hover; the visually-hidden span still carries the SR label. The two are complementary, not redundant.

P3 — tooltip three-layer JSX repetition (deferred)

Verified. 13 sites are highly isomorphic (<Tooltip><TooltipTrigger render={<Button variant="quiet" size="icon-sm"/>} ...props><Icon/></TooltipTrigger><TooltipContent>label</TooltipContent></Tooltip>).

Defering this one. A TooltipButton helper would save ~15-27 lines net, but the 13 sites' trigger props are heterogeneous (disabled / aria-expanded / conditional icon / conditional label / UiButton vs Button), so the helper becomes a pass-through wrapper (tooltip + render + {...triggerProps} + children) that mostly removes the <Tooltip>/<TooltipContent> wrapping — a modest win for a new abstraction layer + an app-region-hygiene regex adjustment (it extracts classNames from <TooltipTrigger render={<UiButton …/>}> blocks today). Happy to do it as a follow-up if the tooltip site count grows; for 13 sites the direct form is clearer and more flexible. Pushing back rather than adding abstraction for line-count.

待验证 — CI checks / hand-test

Local verification done:npm run -w @maka/desktop test → 2009/2009 pass. npm run typecheck clean across workspaces. Screenshots vs main: turn-narrative 0.09% (the disclosure commit's summary→button residual), settings-appearance 0.0136 (PR4's settings-nav snap, untouched here), first-run 0% — all hover-only tooltip / controlled-open changes add 0 at rest.

CI checks show 0 — will confirm CI triggers on the updated push. Will hand-test before merge: Collapsible keyboard toggle + status-following collapse, Tooltip hover/focus, NumberField empty/invalid input (proxy 0↔empty, gateway empty→3939).


Summary: 3 fix commits pushed (c06d50a9, f4dba2c2, 28f6ed7a). 1 item deferred with rationale (tooltip helper). Issue #520 re-arranged: #22 input-canonical moved to PR8 (not a clean look-preserving swap — primitives/input.tsx carries a different structure than the native ui.tsxInput, and bare-field-chrome-contract pins the native's data-maka-field-chrome + border-input + ring-2).

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
Astro-Han added a commit that referenced this pull request Jul 5, 2026
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
Astro-Han added 10 commits July 5, 2026 17:50
… state attrs) (#520 PR5 item 23)
Foundation commit for PR5 — the convention the disclosure / tooltip /
number-field / input migrations in the following commits follow.
data-slot: add data-slot="<name>" to all 20 Base UI wrappers in ui.tsx
(Button / Separator / Checkbox / DialogBackdrop / DialogPopup / Select
Trigger/List/Popup/Group/GroupLabel/Separator/Item / FieldDescription /
Label / Switch / Toggle / ToggleGroup / RadioGroup / Radio / Progress).
The ./primitives/ wrappers (accordion / alert / badge / …) already do
this; ui.tsx wrappers did not. data-slot is a stable CSS targeting hook
that survives className drift. Base UI does not emit data-slot itself, so
the wrapper owns it. No CSS targets these yet — this is forward-looking
convention, a visual no-op.
State-attribute form: adopt Base UI's native attribute-presence form
([data-active] / [data-open] / [data-checked] / [data-selected] /
[data-pressed] / [data-highlighted] / [data-disabled]), NOT the
attribute-value form ([data-active="true"]). Maka's renderer CSS has
zero state-attribute selectors today, so there is nothing to break and no
override layer to maintain. A per-component hook map (Tabs data-active /
Select data-[highlighted]/data-[selected] / Checkbox/Switch/Radio
data-[checked] / Toggle data-[pressed] / Dialog/Tooltip/Popover data-[open])
is documented in a doc comment at the top of ui.tsx, along with the
whitelisted CSS var hooks (--anchor-* / --available-* / --active-tab-*).
className(state) function form is deferred — add only when a migration in
this PR actually needs state-based classes.
style-hook-convention-contract.test.ts locks the data-slot rule: every
ui.tsx wrapper that forwards to a <Base*> component must carry data-slot,
and the convention doc comment must stay present. Hand-written native
elements (legacy Input / Textarea / Badge) are out of scope until they
retire onto a Base UI primitive (input canonical, commit 5).
Verification: 1972/1972 desktop tests pass (+4 contract). typecheck clean.
renderer build clean. Visual no-op (data-slot is an attribute; no CSS
targets it yet).
…tem 17)
Migrate the four native <details>/<summary> disclosure sites onto Base UI
Collapsible, following the style-hook convention (item 23). The code comments
at the sites already said "future Base UI Accordion path"; all four are
independent single sections (not grouped), so Collapsible (not Accordion) is
the right primitive.
Sites:
- chat-view.tsx turn-thinking (maka-turn-thinking): uncontrolled, default
closed.
- chat-view.tsx reasoning-panel (maka-reasoning-panel): controlled open state
moves from reading e.currentTarget.open off the native toggle event to
Collapsible's open + onOpenChange props (default-open behavior preserved).
- permission-dialog.tsx permission-raw (maka-permission-raw): uncontrolled.
- tool-activity.tsx tool card ([data-slot="tool"]): uncontrolled with
defaultOpen = isOpenByDefault(status); the data-slot="tool" hook overrides
the Collapsible wrapper's default data-slot="collapsible" via prop spread,
so the existing [data-slot="tool"] CSS keeps working.
New primitive: packages/ui/src/primitives/collapsible.tsx wraps Base UI
Collapsible Root/Trigger/Panel with data-slot="collapsible" /
"collapsible-trigger" / "collapsible-panel". The Trigger carries a button
chrome reset (appearance-none / bg-transparent / border-0 / p-0 / font
inherit) so the summary row's clean look is preserved — native <summary> had
no background/border/padding, but <button> does. Exported from the barrel as
PrimitiveCollapsible / PrimitiveCollapsibleTrigger / PrimitiveCollapsiblePanel.
CSS retargeting (the DOM changed: <summary> -> <button> trigger, <details
open> -> Collapsible with data-panel-open on the trigger):
- .maka-turn-thinking summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-slot="collapsible-trigger"][data-panel-open]::before
- .maka-permission-raw > summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-panel-open] variants
- .maka-reasoning-panel-header marker resets dropped (button has no native
marker); [open] .maka-reasoning-panel-chevron ->
[data-slot="collapsible-trigger"][data-panel-open] .maka-reasoning-panel-chevron
- [data-slot="tool"] > summary ::-webkit-details-marker / ::marker resets
dropped (button trigger has no native marker).
- list-style: none rules dropped (no list marker on a button).
TDD: disclosure-collapsible-contract.test.ts written first (RED: sites still
used <details>, no primitive). GREEN: primitive + 4 migrations + CSS
retarget. The contract bans <details>/<summary> (incl. in comments) in the
three migrated files and requires a Collapsible import; it also pins the
primitive's data-slot on Root/Trigger/Panel.
Existing tests updated: border-width-converge-contract TRIANGLE_CARET_SELECTORS
(the chevron ::before border-width: 4px 0 4px 5px moved from summary::before to
[data-slot="collapsible-trigger"]::before); chat-marker-cascade-contract
(.maka-turn-thinking summary -> [data-slot="collapsible-trigger"]);
chat-tool-card-cascade-contract (the marker-reset residue on
[data-slot="tool"] > summary is gone — the trigger is a button now).
Verification: 1975/1975 desktop tests pass (+4 disclosure contract; the 3
existing contracts updated). typecheck clean. Screenshots vs clean main (PR4):
turn-narrative 0.09% (the summary->button swap's tiny pixel difference — button
baseline, the CollapsiblePanel wrapper), first-run 0%. No regression.
…tem 20)
Migrate the clearest icon-only-action button tooltips off the native title=
attribute (an unstyled, delayed browser tooltip) onto Base UI Tooltip,
which gives a themed, positioned, hover+focus tooltip matching the app.
Sites (13 buttons across 3 files):
- app-shell-chrome-actions.tsx: search / sidebar / new-task / feedback /
command-palette / help / health (7).
- browser-panel.tsx: back / forward / refresh / close (4).
- artifact-pane.tsx: the collapse toggle (1); the delete button's redundant
title= is dropped instead (it already shows a visible 删除 label, so a
tooltip would be a duplicate).
New primitive: packages/ui/src/primitives/tooltip.tsx wraps Base UI Tooltip
Root / Trigger / Content with data-slot="tooltip" / "tooltip-trigger" /
"tooltip-content". TooltipContent collapses Portal + Positioner + Popup
(the DialogContent shape) with a themed look (popover bg/corner/shadow +
z-overlay). The Trigger forwards via Base UI's render prop
(<TooltipTrigger render={<Button …/>} …props>…children…</TooltipTrigger>),
which merges the Trigger's props + children into the rendered Button — Base
UI v1 uses render, not the Radix asChild prop. Exported from the barrel as
Tooltip / TooltipTrigger / TooltipContent (no ui.tsx wrapper, so direct
names). aria-label stays on the button (accessible name); the TooltipContent
is the visual hover hint.
TDD: tooltip-converge-contract.test.ts written first (RED: the 3 files still
carried native title=, no primitive). GREEN: primitive + 13 migrations. The
contract bans a title= JSX attribute in the 3 migrated files and requires
a Tooltip import; it pins the primitive's data-slot on Root/Trigger/Content.
The regex \btitle= matches JSX attributes but not JS `const title =` (the
space before =).
Scope note: the longer tail of title= usages (truncation spans like
memory-settings path + chat-model-switcher chip, SelectTrigger titles,
status-badge icons, OnboardingHero's submit button) are tooltip-eligible but
each needs a label-vs-tooltip judgment and are not swept in this commit —
deferred to a follow-up. label-prop components (SettingRow / MetricCard /
SetupHero / Section) render title as visible text and are NOT tooltips;
they stay.
Existing tests updated: app-region-hygiene-contract's
extractStaticUiButtonClassNames now also matches the
<TooltipTrigger render={<UiButton/>}> shape (the className moved from the
UiButton element to the TooltipTrigger that renders it); search-modal-
lifecycle's shell-search-button regex now matches </TooltipTrigger> (the
UiButton is the render target, self-closing).
Verification: 1979/1979 desktop tests pass (+4 tooltip contract). typecheck
clean (render prop, not asChild). renderer build clean. Screenshots vs clean
main (PR4): turn-narrative 0.09% (the disclosure commit's residual; tooltips
are hover-only so rest state is unchanged), first-run 0%.
Migrate the two gateway/proxy port inputs off the native Input +
Number(event.currentTarget.value) hand-conversion onto Base UI NumberField,
which binds value: number | null directly and parses numeric input itself.
Sites:
- general-settings-page proxy port: value={proxyDraft.port || null}
(0 renders empty, matching the old String(port || '')), onValueChange
writes v ?? 0 (empty -> 0, matching the old Number(value) || 0).
- open-gateway-settings-page gateway port: value={gatewayDraft.port}
(0 renders "0", matching String(port)), onValueChange writes v ?? 3939
(empty -> 3939, matching Number(value) || 3939). inputMode="numeric" stays
on the input.
New primitive: packages/ui/src/primitives/number-field.tsx wraps Base UI
NumberField Root / Input with data-slot="number-field" / "number-field-input".
NumberFieldInput carries the maka standalone-input look via the shared
inputClasses export (the same classes the native ui.tsx Input used), so the
port inputs keep their look; unstyled gives the bare form for any future
Field/InputGroup embedding. Exported from the barrel as NumberField /
NumberFieldInput (no ui.tsx wrapper, so direct names, like Tooltip).
To share the maka input styling without duplicating it, ui.tsx exports
inputClasses + bareFieldClasses (const -> export const); the number-field
primitive imports them. (The input-canonical commit, item 22, will move
these into primitives/input.tsx and update this import.)
TDD: number-field-converge-contract.test.ts written first (RED: the two
files still carried Number(event.currentTarget.value), no primitive). GREEN:
primitive + 2 migrations. The contract bans the hand-conversion
(Number(event.currentTarget.value)) and requires a NumberField import; it
pins the primitive's data-slot on Root/Input. The hand-convert regex is
specific (Number(event.currentTarget.value)) so a different Number() call is
not flagged.
Existing test updated: settings-network-gateway-contract's two port-input
assertions now pin the NumberField form (value={...|| null} / value={...} +
onValueChange={(v) => update...({ port: v ?? default })}) instead of the old
String(value) + onChange Number(event.currentTarget.value). The intent —
"port input renders from the local draft while persisting in the background"
— is preserved; only the binding form changed. The "port input must not
lock after each digit" doesNotMatch still passes (the NumberFieldInput is not
disabled while saving).
Verification: 1982/1982 desktop tests pass (+3 number-field contract).
typecheck clean. renderer build clean. Screenshots: the port inputs are in
general/open-gateway settings (no capture scenario), so the captured
scenarios show no change — settings-appearance 0.0136 (PR4's settings-nav
snap residual) and turn-narrative 0.0009 (the disclosure commit's residual);
commit 4 adds 0. The port inputs keep their look via the shared inputClasses.
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
After rebasing PR5 onto main, the old PR5 style-hook commit still introduced a
SelectList wrapper, but main now has the ui.tsx dead-export contract and there
is no consumer for SelectList. Keep the style-hook convention on used wrappers
only and remove the dead export instead of adding a pending allowlist entry.
Verification: style-hook/divider/disclosure contracts pass (13/13), desktop
tests pass (2019/2019), typecheck clean.
@Astro-Han
Astro-Hanforce-pushed the feat/base-ui-small-migrations branch from 4a3c541 to 92fa1e1CompareJuly 5, 2026 09:53
@Astro-Han
Astro-Han merged commit cc3fc2c into mainJul 5, 2026
@Astro-Han
Astro-Han deleted the feat/base-ui-small-migrations branch July 5, 2026 09:54
Sign up for freeto 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

@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) by Astro-Han · Pull Request #543 · apache/maka · GitHub
Skip to content

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) - #543

Merged
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations
Jul 5, 2026
Merged

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5)#543
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

PR5 of #520 — the style-hook convention foundation + the smaller Base UI migrations that follow it. Four items, one commit each (each independently revertible), TDD red → green → refactor.

#22 (input canonical) was scoped into PR5 but moved to PR8 during this work — primitives/input.tsx carries a different structure (data-slot="input-control" span + inner Base UI input, ring-[3px]) than the native ui.tsxInput (single <input> + inputClasses, ring-2), and bare-field-chrome-contract.test.ts pins the native's data-maka-field-chrome + border-input + ring-2, so retiring the native is not a clean look-preserving swap. The issue plan was re-arranged (#22 → PR8); this PR opens with 1-4 only.

New primitives (packages/ui/src/primitives/)

PrimitiveWrapsdata-slot
collapsible.tsx@base-ui/react/collapsible Root/Trigger/Panelcollapsible / collapsible-trigger / collapsible-panel
tooltip.tsx@base-ui/react/tooltip Root/Trigger/Content (Portal+Positioner+Popup)tooltip / tooltip-trigger / tooltip-content
number-field.tsx@base-ui/react/number-field Root/Inputnumber-field / number-field-input

All follow the style-hook convention (item 23): data-slot on each part, Base UI's native boolean state attributes (data-panel-open, data-open, etc.) adopted as-is. The Collapsible Trigger carries a button-chrome reset (the native <summary> had no bg/border/padding, but <button> does). The Tooltip Trigger uses Base UI v1's render prop (not Radix asChild). The NumberField Input reuses maka's inputClasses (now exported from ui.tsx) so the port inputs keep their look.

Convergence

  • item 23 — style-hook convention: data-slot added to all 20 Base UI wrappers in ui.tsx (Button/Separator/Checkbox/Dialog backdrop+popup/Select Trigger+List+Popup+Group+GroupLabel+Separator+Item/FieldDescription/Label/Switch/Toggle/ToggleGroup/RadioGroup/Radio/Progress). A doc comment at the top of ui.tsx records the state-attribute decision (adopt Base UI native attribute-presence form — maka's renderer CSS has zero state-attribute selectors today, so nothing breaks), the per-component hook map, the whitelisted CSS var hooks, and the className(state) deferral.
  • item 17 — disclosure → Collapsible: the four <details>/<summary> sites (chat-view turn-thinking + reasoning-panel, permission-dialog permission-raw, tool-activity) migrate to Base UI Collapsible (all independent single sections, so Collapsible not Accordion). CSS retargeted: summary[data-slot="collapsible-trigger"], [open][data-panel-open], ::-webkit-details-marker/::marker resets dropped (button trigger has no native marker). The reasoning-panel's controlled-open moves from e.currentTarget.open off the toggle event to Collapsible's open + onOpenChange.
  • item 20 — tooltip → Base UI Tooltip: the 13 clearest icon-only-action button title= usages (app-shell-chrome-actions 7, browser-panel 4, artifact-pane collapse) migrate to Base UI Tooltip; aria-label stays on the button, TooltipContent is the visual hover hint. The artifact-pane delete button's redundant title= is dropped (it shows a visible 删除 label). The longer tail (truncation spans, SelectTrigger titles, status badges, OnboardingHero submit) are tooltip-eligible but need per-site label-vs-tooltip judgment — deferred to a follow-up.
  • item 21 — number-field: the two gateway/proxy port inputs migrate to Base UI NumberField (value: number | null, onValueChange), dropping the Number(event.currentTarget.value) hand-conversion. Proxy: 0 ↔ empty; gateway: empty → 3939 default, both preserved.

New contracts

ContractBansPins
style-hook-convention-contract.test.tsa ui.tsx wrapper forwarding to <Base*> without data-slotthe convention doc comment present
disclosure-collapsible-contract.test.ts<details>/<summary> (incl. in comments) in the 3 migrated files; missing Collapsible importprimitives/collapsible.tsx data-slot on Root/Trigger/Panel
tooltip-converge-contract.test.tsa title= JSX attribute in the 3 migrated files; missing Tooltip importprimitives/tooltip.tsx data-slot on Root/Trigger/Content
number-field-converge-contract.test.tsNumber(event.currentTarget.value) hand-conversion in the 2 port files; missing NumberField importprimitives/number-field.tsx data-slot on Root/Input

Existing tests updated

  • border-width-converge-contract: the triangle-caret allowlist selectors moved from summary::before to [data-slot="collapsible-trigger"]::before (the chevron border-width: 4px 0 4px 5px moved with the disclosure migration).
  • chat-marker-cascade-contract / chat-tool-card-cascade-contract: the turn-thinking / tool-card residue selectors + the dropped marker-reset residue.
  • app-region-hygiene-contract / search-modal-lifecycle-contract: the topbar-button className extraction + the shell-search-button regex now match the <TooltipTrigger render={<UiButton/>}> shape (the className moved from the UiButton element to the TooltipTrigger that renders it).
  • settings-network-gateway-contract: the two port-input assertions now pin the NumberField form (value={...|| null} / value={...} + onValueChange={(v) => update…({ port: v ?? default })).

Commits

  1. feat(ui): establish Base UI style-hook convention (data-slot + native state attrs) — item 23 (foundation).
  2. feat(ui): migrate disclosure sites to Base UI Collapsible — item 17.
  3. feat(ui): migrate icon-action tooltips to Base UI Tooltip — item 20.
  4. feat(ui): migrate port inputs to Base UI NumberField — item 21.

Verification

  • Tests: npm run -w @maka/desktop test — 2008/2008 pass (rebased onto main with PR2; +26 from PR2, +10 new contract assertions across 4 files). typecheck clean.
  • Screenshots vs main (turn-narrative, settings-appearance, first-run × light): tooltips are hover-only so rest state is unchanged; turn-narrative 0.09% (the disclosure commit's summary→button pixel difference), settings-appearance 0.0136 (PR4's settings-nav snap residual, untouched here), first-run 0. The port inputs (general/open-gateway settings) have no capture scenario; they keep their look via the shared inputClasses.

Out of scope / deferred

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review. Verified every claim against the code before acting; results + fixes below.

P2 — tool-activity defaultOpen (fixed, c06d50a9)

Verified. The disclosure migration set defaultOpen={isOpenByDefault(item.status)} (uncontrolled — open decided once on first render). The pre-Collapsible <details open={isOpenByDefault(status)}> re-evaluated open every render, so a card that defaulted open while pending/running no longer auto-collapsed when it settled. Real regression.

Fix: extracted a ToolActivityCard component (hooks can't live in the .map callback) with a controlled state machine: useState(isOpenByDefault(item.status)) seeds open, useEffect re-syncs it on [item.status] so a pending/running card collapses when it settles, onOpenChange={setOpen} lets the user toggle in between (the next status change re-syncs). This restores the status-following behavior and makes the user-toggle intentional rather than a React-not-rerendering side effect.

Test:disclosure-collapsible-contract now asserts tool-activity must NOT use defaultOpen, must use onOpenChange, and must re-sync via useEffect on [item.status]. (maka has no React testing-library / react-test-renderer, so the behavior is locked structurally + verified by hand; a rerender-act test is out of scope.) Will hand-test pending/running → completed/interrupted before merge.

P3 — Collapsible barrel / contract mismatch (fixed, f4dba2c2)

Verified. Barrel exported Collapsible as PrimitiveCollapsible (the old Tabs/Accordion convention), while Tooltip/NumberField export directly. The contract regex allowed @maka/ui as an import path, but the barrel didn't export Collapsible under that name — a phantom-pass path. The 3 internal sites import via ./primitives/collapsible.js (the in-package convention — alert/chat all use relative primitive paths), so the test passed for the right reason, but the @maka/ui allowance was misleading.

Fix: barrel now exports Collapsible / CollapsibleTrigger / CollapsiblePanel directly (dropped the Primitive* aliases and the CollapsiblePrimitive re-export — nothing consumes them; Tooltip/NumberField don't export their Primitive either). Internal sites keep their relative-path imports (convention); @maka/ui is now genuinely available rather than phantom.

P3 — tooltip.tsx doc showed asChild (fixed, f4dba2c2)

Verified. Line 14 usage doc showed <TooltipTrigger asChild> (Radix), but this PR uses Base UI v1's render prop. Fixed the example to render={<Button />} and noted the Trigger merges its props + children into the rendered element, so the next person doesn't copy the comment and hit the typecheck failure the migration hit.

待验证 — artifact delete title= (fixed, 28f6ed7a)

Partially verified — pushing back on the a11y part. The .maka-artifact-toolbar-destructive-label span is visually hidden but still announced (position:absolute; width:1px; clip-path:inset(50%); CSS comment: "only exists for assistive tech"). So the screen-reader path was preserved when title="删除" was dropped — no SR regression. But mouse users did lose the hover hint (native title= gone, no Base UI Tooltip added).

Fix: wrapped the delete button in a Base UI Tooltip (same shape as the artifact-pane collapse button). TooltipContent mirrors 删除/删除中… for hover; the visually-hidden span still carries the SR label. The two are complementary, not redundant.

P3 — tooltip three-layer JSX repetition (deferred)

Verified. 13 sites are highly isomorphic (<Tooltip><TooltipTrigger render={<Button variant="quiet" size="icon-sm"/>} ...props><Icon/></TooltipTrigger><TooltipContent>label</TooltipContent></Tooltip>).

Defering this one. A TooltipButton helper would save ~15-27 lines net, but the 13 sites' trigger props are heterogeneous (disabled / aria-expanded / conditional icon / conditional label / UiButton vs Button), so the helper becomes a pass-through wrapper (tooltip + render + {...triggerProps} + children) that mostly removes the <Tooltip>/<TooltipContent> wrapping — a modest win for a new abstraction layer + an app-region-hygiene regex adjustment (it extracts classNames from <TooltipTrigger render={<UiButton …/>}> blocks today). Happy to do it as a follow-up if the tooltip site count grows; for 13 sites the direct form is clearer and more flexible. Pushing back rather than adding abstraction for line-count.

待验证 — CI checks / hand-test

Local verification done:npm run -w @maka/desktop test → 2009/2009 pass. npm run typecheck clean across workspaces. Screenshots vs main: turn-narrative 0.09% (the disclosure commit's summary→button residual), settings-appearance 0.0136 (PR4's settings-nav snap, untouched here), first-run 0% — all hover-only tooltip / controlled-open changes add 0 at rest.

CI checks show 0 — will confirm CI triggers on the updated push. Will hand-test before merge: Collapsible keyboard toggle + status-following collapse, Tooltip hover/focus, NumberField empty/invalid input (proxy 0↔empty, gateway empty→3939).


Summary: 3 fix commits pushed (c06d50a9, f4dba2c2, 28f6ed7a). 1 item deferred with rationale (tooltip helper). Issue #520 re-arranged: #22 input-canonical moved to PR8 (not a clean look-preserving swap — primitives/input.tsx carries a different structure than the native ui.tsxInput, and bare-field-chrome-contract pins the native's data-maka-field-chrome + border-input + ring-2).

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
Astro-Han added a commit that referenced this pull request Jul 5, 2026
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
Astro-Han added 10 commits July 5, 2026 17:50
… state attrs) (#520 PR5 item 23)
Foundation commit for PR5 — the convention the disclosure / tooltip /
number-field / input migrations in the following commits follow.
data-slot: add data-slot="<name>" to all 20 Base UI wrappers in ui.tsx
(Button / Separator / Checkbox / DialogBackdrop / DialogPopup / Select
Trigger/List/Popup/Group/GroupLabel/Separator/Item / FieldDescription /
Label / Switch / Toggle / ToggleGroup / RadioGroup / Radio / Progress).
The ./primitives/ wrappers (accordion / alert / badge / …) already do
this; ui.tsx wrappers did not. data-slot is a stable CSS targeting hook
that survives className drift. Base UI does not emit data-slot itself, so
the wrapper owns it. No CSS targets these yet — this is forward-looking
convention, a visual no-op.
State-attribute form: adopt Base UI's native attribute-presence form
([data-active] / [data-open] / [data-checked] / [data-selected] /
[data-pressed] / [data-highlighted] / [data-disabled]), NOT the
attribute-value form ([data-active="true"]). Maka's renderer CSS has
zero state-attribute selectors today, so there is nothing to break and no
override layer to maintain. A per-component hook map (Tabs data-active /
Select data-[highlighted]/data-[selected] / Checkbox/Switch/Radio
data-[checked] / Toggle data-[pressed] / Dialog/Tooltip/Popover data-[open])
is documented in a doc comment at the top of ui.tsx, along with the
whitelisted CSS var hooks (--anchor-* / --available-* / --active-tab-*).
className(state) function form is deferred — add only when a migration in
this PR actually needs state-based classes.
style-hook-convention-contract.test.ts locks the data-slot rule: every
ui.tsx wrapper that forwards to a <Base*> component must carry data-slot,
and the convention doc comment must stay present. Hand-written native
elements (legacy Input / Textarea / Badge) are out of scope until they
retire onto a Base UI primitive (input canonical, commit 5).
Verification: 1972/1972 desktop tests pass (+4 contract). typecheck clean.
renderer build clean. Visual no-op (data-slot is an attribute; no CSS
targets it yet).
…tem 17)
Migrate the four native <details>/<summary> disclosure sites onto Base UI
Collapsible, following the style-hook convention (item 23). The code comments
at the sites already said "future Base UI Accordion path"; all four are
independent single sections (not grouped), so Collapsible (not Accordion) is
the right primitive.
Sites:
- chat-view.tsx turn-thinking (maka-turn-thinking): uncontrolled, default
closed.
- chat-view.tsx reasoning-panel (maka-reasoning-panel): controlled open state
moves from reading e.currentTarget.open off the native toggle event to
Collapsible's open + onOpenChange props (default-open behavior preserved).
- permission-dialog.tsx permission-raw (maka-permission-raw): uncontrolled.
- tool-activity.tsx tool card ([data-slot="tool"]): uncontrolled with
defaultOpen = isOpenByDefault(status); the data-slot="tool" hook overrides
the Collapsible wrapper's default data-slot="collapsible" via prop spread,
so the existing [data-slot="tool"] CSS keeps working.
New primitive: packages/ui/src/primitives/collapsible.tsx wraps Base UI
Collapsible Root/Trigger/Panel with data-slot="collapsible" /
"collapsible-trigger" / "collapsible-panel". The Trigger carries a button
chrome reset (appearance-none / bg-transparent / border-0 / p-0 / font
inherit) so the summary row's clean look is preserved — native <summary> had
no background/border/padding, but <button> does. Exported from the barrel as
PrimitiveCollapsible / PrimitiveCollapsibleTrigger / PrimitiveCollapsiblePanel.
CSS retargeting (the DOM changed: <summary> -> <button> trigger, <details
open> -> Collapsible with data-panel-open on the trigger):
- .maka-turn-thinking summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-slot="collapsible-trigger"][data-panel-open]::before
- .maka-permission-raw > summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-panel-open] variants
- .maka-reasoning-panel-header marker resets dropped (button has no native
marker); [open] .maka-reasoning-panel-chevron ->
[data-slot="collapsible-trigger"][data-panel-open] .maka-reasoning-panel-chevron
- [data-slot="tool"] > summary ::-webkit-details-marker / ::marker resets
dropped (button trigger has no native marker).
- list-style: none rules dropped (no list marker on a button).
TDD: disclosure-collapsible-contract.test.ts written first (RED: sites still
used <details>, no primitive). GREEN: primitive + 4 migrations + CSS
retarget. The contract bans <details>/<summary> (incl. in comments) in the
three migrated files and requires a Collapsible import; it also pins the
primitive's data-slot on Root/Trigger/Panel.
Existing tests updated: border-width-converge-contract TRIANGLE_CARET_SELECTORS
(the chevron ::before border-width: 4px 0 4px 5px moved from summary::before to
[data-slot="collapsible-trigger"]::before); chat-marker-cascade-contract
(.maka-turn-thinking summary -> [data-slot="collapsible-trigger"]);
chat-tool-card-cascade-contract (the marker-reset residue on
[data-slot="tool"] > summary is gone — the trigger is a button now).
Verification: 1975/1975 desktop tests pass (+4 disclosure contract; the 3
existing contracts updated). typecheck clean. Screenshots vs clean main (PR4):
turn-narrative 0.09% (the summary->button swap's tiny pixel difference — button
baseline, the CollapsiblePanel wrapper), first-run 0%. No regression.
…tem 20)
Migrate the clearest icon-only-action button tooltips off the native title=
attribute (an unstyled, delayed browser tooltip) onto Base UI Tooltip,
which gives a themed, positioned, hover+focus tooltip matching the app.
Sites (13 buttons across 3 files):
- app-shell-chrome-actions.tsx: search / sidebar / new-task / feedback /
command-palette / help / health (7).
- browser-panel.tsx: back / forward / refresh / close (4).
- artifact-pane.tsx: the collapse toggle (1); the delete button's redundant
title= is dropped instead (it already shows a visible 删除 label, so a
tooltip would be a duplicate).
New primitive: packages/ui/src/primitives/tooltip.tsx wraps Base UI Tooltip
Root / Trigger / Content with data-slot="tooltip" / "tooltip-trigger" /
"tooltip-content". TooltipContent collapses Portal + Positioner + Popup
(the DialogContent shape) with a themed look (popover bg/corner/shadow +
z-overlay). The Trigger forwards via Base UI's render prop
(<TooltipTrigger render={<Button …/>} …props>…children…</TooltipTrigger>),
which merges the Trigger's props + children into the rendered Button — Base
UI v1 uses render, not the Radix asChild prop. Exported from the barrel as
Tooltip / TooltipTrigger / TooltipContent (no ui.tsx wrapper, so direct
names). aria-label stays on the button (accessible name); the TooltipContent
is the visual hover hint.
TDD: tooltip-converge-contract.test.ts written first (RED: the 3 files still
carried native title=, no primitive). GREEN: primitive + 13 migrations. The
contract bans a title= JSX attribute in the 3 migrated files and requires
a Tooltip import; it pins the primitive's data-slot on Root/Trigger/Content.
The regex \btitle= matches JSX attributes but not JS `const title =` (the
space before =).
Scope note: the longer tail of title= usages (truncation spans like
memory-settings path + chat-model-switcher chip, SelectTrigger titles,
status-badge icons, OnboardingHero's submit button) are tooltip-eligible but
each needs a label-vs-tooltip judgment and are not swept in this commit —
deferred to a follow-up. label-prop components (SettingRow / MetricCard /
SetupHero / Section) render title as visible text and are NOT tooltips;
they stay.
Existing tests updated: app-region-hygiene-contract's
extractStaticUiButtonClassNames now also matches the
<TooltipTrigger render={<UiButton/>}> shape (the className moved from the
UiButton element to the TooltipTrigger that renders it); search-modal-
lifecycle's shell-search-button regex now matches </TooltipTrigger> (the
UiButton is the render target, self-closing).
Verification: 1979/1979 desktop tests pass (+4 tooltip contract). typecheck
clean (render prop, not asChild). renderer build clean. Screenshots vs clean
main (PR4): turn-narrative 0.09% (the disclosure commit's residual; tooltips
are hover-only so rest state is unchanged), first-run 0%.
Migrate the two gateway/proxy port inputs off the native Input +
Number(event.currentTarget.value) hand-conversion onto Base UI NumberField,
which binds value: number | null directly and parses numeric input itself.
Sites:
- general-settings-page proxy port: value={proxyDraft.port || null}
(0 renders empty, matching the old String(port || '')), onValueChange
writes v ?? 0 (empty -> 0, matching the old Number(value) || 0).
- open-gateway-settings-page gateway port: value={gatewayDraft.port}
(0 renders "0", matching String(port)), onValueChange writes v ?? 3939
(empty -> 3939, matching Number(value) || 3939). inputMode="numeric" stays
on the input.
New primitive: packages/ui/src/primitives/number-field.tsx wraps Base UI
NumberField Root / Input with data-slot="number-field" / "number-field-input".
NumberFieldInput carries the maka standalone-input look via the shared
inputClasses export (the same classes the native ui.tsx Input used), so the
port inputs keep their look; unstyled gives the bare form for any future
Field/InputGroup embedding. Exported from the barrel as NumberField /
NumberFieldInput (no ui.tsx wrapper, so direct names, like Tooltip).
To share the maka input styling without duplicating it, ui.tsx exports
inputClasses + bareFieldClasses (const -> export const); the number-field
primitive imports them. (The input-canonical commit, item 22, will move
these into primitives/input.tsx and update this import.)
TDD: number-field-converge-contract.test.ts written first (RED: the two
files still carried Number(event.currentTarget.value), no primitive). GREEN:
primitive + 2 migrations. The contract bans the hand-conversion
(Number(event.currentTarget.value)) and requires a NumberField import; it
pins the primitive's data-slot on Root/Input. The hand-convert regex is
specific (Number(event.currentTarget.value)) so a different Number() call is
not flagged.
Existing test updated: settings-network-gateway-contract's two port-input
assertions now pin the NumberField form (value={...|| null} / value={...} +
onValueChange={(v) => update...({ port: v ?? default })}) instead of the old
String(value) + onChange Number(event.currentTarget.value). The intent —
"port input renders from the local draft while persisting in the background"
— is preserved; only the binding form changed. The "port input must not
lock after each digit" doesNotMatch still passes (the NumberFieldInput is not
disabled while saving).
Verification: 1982/1982 desktop tests pass (+3 number-field contract).
typecheck clean. renderer build clean. Screenshots: the port inputs are in
general/open-gateway settings (no capture scenario), so the captured
scenarios show no change — settings-appearance 0.0136 (PR4's settings-nav
snap residual) and turn-narrative 0.0009 (the disclosure commit's residual);
commit 4 adds 0. The port inputs keep their look via the shared inputClasses.
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
After rebasing PR5 onto main, the old PR5 style-hook commit still introduced a
SelectList wrapper, but main now has the ui.tsx dead-export contract and there
is no consumer for SelectList. Keep the style-hook convention on used wrappers
only and remove the dead export instead of adding a pending allowlist entry.
Verification: style-hook/divider/disclosure contracts pass (13/13), desktop
tests pass (2019/2019), typecheck clean.
@Astro-Han
Astro-Hanforce-pushed the feat/base-ui-small-migrations branch from 4a3c541 to 92fa1e1CompareJuly 5, 2026 09:53
@Astro-Han
Astro-Han merged commit cc3fc2c into mainJul 5, 2026
@Astro-Han
Astro-Han deleted the feat/base-ui-small-migrations branch July 5, 2026 09:54
Sign up for freeto 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

@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) by Astro-Han · Pull Request #543 · apache/maka · GitHub
Skip to content

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) - #543

Merged
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations
Jul 5, 2026
Merged

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5)#543
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

PR5 of #520 — the style-hook convention foundation + the smaller Base UI migrations that follow it. Four items, one commit each (each independently revertible), TDD red → green → refactor.

#22 (input canonical) was scoped into PR5 but moved to PR8 during this work — primitives/input.tsx carries a different structure (data-slot="input-control" span + inner Base UI input, ring-[3px]) than the native ui.tsxInput (single <input> + inputClasses, ring-2), and bare-field-chrome-contract.test.ts pins the native's data-maka-field-chrome + border-input + ring-2, so retiring the native is not a clean look-preserving swap. The issue plan was re-arranged (#22 → PR8); this PR opens with 1-4 only.

New primitives (packages/ui/src/primitives/)

PrimitiveWrapsdata-slot
collapsible.tsx@base-ui/react/collapsible Root/Trigger/Panelcollapsible / collapsible-trigger / collapsible-panel
tooltip.tsx@base-ui/react/tooltip Root/Trigger/Content (Portal+Positioner+Popup)tooltip / tooltip-trigger / tooltip-content
number-field.tsx@base-ui/react/number-field Root/Inputnumber-field / number-field-input

All follow the style-hook convention (item 23): data-slot on each part, Base UI's native boolean state attributes (data-panel-open, data-open, etc.) adopted as-is. The Collapsible Trigger carries a button-chrome reset (the native <summary> had no bg/border/padding, but <button> does). The Tooltip Trigger uses Base UI v1's render prop (not Radix asChild). The NumberField Input reuses maka's inputClasses (now exported from ui.tsx) so the port inputs keep their look.

Convergence

  • item 23 — style-hook convention: data-slot added to all 20 Base UI wrappers in ui.tsx (Button/Separator/Checkbox/Dialog backdrop+popup/Select Trigger+List+Popup+Group+GroupLabel+Separator+Item/FieldDescription/Label/Switch/Toggle/ToggleGroup/RadioGroup/Radio/Progress). A doc comment at the top of ui.tsx records the state-attribute decision (adopt Base UI native attribute-presence form — maka's renderer CSS has zero state-attribute selectors today, so nothing breaks), the per-component hook map, the whitelisted CSS var hooks, and the className(state) deferral.
  • item 17 — disclosure → Collapsible: the four <details>/<summary> sites (chat-view turn-thinking + reasoning-panel, permission-dialog permission-raw, tool-activity) migrate to Base UI Collapsible (all independent single sections, so Collapsible not Accordion). CSS retargeted: summary[data-slot="collapsible-trigger"], [open][data-panel-open], ::-webkit-details-marker/::marker resets dropped (button trigger has no native marker). The reasoning-panel's controlled-open moves from e.currentTarget.open off the toggle event to Collapsible's open + onOpenChange.
  • item 20 — tooltip → Base UI Tooltip: the 13 clearest icon-only-action button title= usages (app-shell-chrome-actions 7, browser-panel 4, artifact-pane collapse) migrate to Base UI Tooltip; aria-label stays on the button, TooltipContent is the visual hover hint. The artifact-pane delete button's redundant title= is dropped (it shows a visible 删除 label). The longer tail (truncation spans, SelectTrigger titles, status badges, OnboardingHero submit) are tooltip-eligible but need per-site label-vs-tooltip judgment — deferred to a follow-up.
  • item 21 — number-field: the two gateway/proxy port inputs migrate to Base UI NumberField (value: number | null, onValueChange), dropping the Number(event.currentTarget.value) hand-conversion. Proxy: 0 ↔ empty; gateway: empty → 3939 default, both preserved.

New contracts

ContractBansPins
style-hook-convention-contract.test.tsa ui.tsx wrapper forwarding to <Base*> without data-slotthe convention doc comment present
disclosure-collapsible-contract.test.ts<details>/<summary> (incl. in comments) in the 3 migrated files; missing Collapsible importprimitives/collapsible.tsx data-slot on Root/Trigger/Panel
tooltip-converge-contract.test.tsa title= JSX attribute in the 3 migrated files; missing Tooltip importprimitives/tooltip.tsx data-slot on Root/Trigger/Content
number-field-converge-contract.test.tsNumber(event.currentTarget.value) hand-conversion in the 2 port files; missing NumberField importprimitives/number-field.tsx data-slot on Root/Input

Existing tests updated

  • border-width-converge-contract: the triangle-caret allowlist selectors moved from summary::before to [data-slot="collapsible-trigger"]::before (the chevron border-width: 4px 0 4px 5px moved with the disclosure migration).
  • chat-marker-cascade-contract / chat-tool-card-cascade-contract: the turn-thinking / tool-card residue selectors + the dropped marker-reset residue.
  • app-region-hygiene-contract / search-modal-lifecycle-contract: the topbar-button className extraction + the shell-search-button regex now match the <TooltipTrigger render={<UiButton/>}> shape (the className moved from the UiButton element to the TooltipTrigger that renders it).
  • settings-network-gateway-contract: the two port-input assertions now pin the NumberField form (value={...|| null} / value={...} + onValueChange={(v) => update…({ port: v ?? default })).

Commits

  1. feat(ui): establish Base UI style-hook convention (data-slot + native state attrs) — item 23 (foundation).
  2. feat(ui): migrate disclosure sites to Base UI Collapsible — item 17.
  3. feat(ui): migrate icon-action tooltips to Base UI Tooltip — item 20.
  4. feat(ui): migrate port inputs to Base UI NumberField — item 21.

Verification

  • Tests: npm run -w @maka/desktop test — 2008/2008 pass (rebased onto main with PR2; +26 from PR2, +10 new contract assertions across 4 files). typecheck clean.
  • Screenshots vs main (turn-narrative, settings-appearance, first-run × light): tooltips are hover-only so rest state is unchanged; turn-narrative 0.09% (the disclosure commit's summary→button pixel difference), settings-appearance 0.0136 (PR4's settings-nav snap residual, untouched here), first-run 0. The port inputs (general/open-gateway settings) have no capture scenario; they keep their look via the shared inputClasses.

Out of scope / deferred

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review. Verified every claim against the code before acting; results + fixes below.

P2 — tool-activity defaultOpen (fixed, c06d50a9)

Verified. The disclosure migration set defaultOpen={isOpenByDefault(item.status)} (uncontrolled — open decided once on first render). The pre-Collapsible <details open={isOpenByDefault(status)}> re-evaluated open every render, so a card that defaulted open while pending/running no longer auto-collapsed when it settled. Real regression.

Fix: extracted a ToolActivityCard component (hooks can't live in the .map callback) with a controlled state machine: useState(isOpenByDefault(item.status)) seeds open, useEffect re-syncs it on [item.status] so a pending/running card collapses when it settles, onOpenChange={setOpen} lets the user toggle in between (the next status change re-syncs). This restores the status-following behavior and makes the user-toggle intentional rather than a React-not-rerendering side effect.

Test:disclosure-collapsible-contract now asserts tool-activity must NOT use defaultOpen, must use onOpenChange, and must re-sync via useEffect on [item.status]. (maka has no React testing-library / react-test-renderer, so the behavior is locked structurally + verified by hand; a rerender-act test is out of scope.) Will hand-test pending/running → completed/interrupted before merge.

P3 — Collapsible barrel / contract mismatch (fixed, f4dba2c2)

Verified. Barrel exported Collapsible as PrimitiveCollapsible (the old Tabs/Accordion convention), while Tooltip/NumberField export directly. The contract regex allowed @maka/ui as an import path, but the barrel didn't export Collapsible under that name — a phantom-pass path. The 3 internal sites import via ./primitives/collapsible.js (the in-package convention — alert/chat all use relative primitive paths), so the test passed for the right reason, but the @maka/ui allowance was misleading.

Fix: barrel now exports Collapsible / CollapsibleTrigger / CollapsiblePanel directly (dropped the Primitive* aliases and the CollapsiblePrimitive re-export — nothing consumes them; Tooltip/NumberField don't export their Primitive either). Internal sites keep their relative-path imports (convention); @maka/ui is now genuinely available rather than phantom.

P3 — tooltip.tsx doc showed asChild (fixed, f4dba2c2)

Verified. Line 14 usage doc showed <TooltipTrigger asChild> (Radix), but this PR uses Base UI v1's render prop. Fixed the example to render={<Button />} and noted the Trigger merges its props + children into the rendered element, so the next person doesn't copy the comment and hit the typecheck failure the migration hit.

待验证 — artifact delete title= (fixed, 28f6ed7a)

Partially verified — pushing back on the a11y part. The .maka-artifact-toolbar-destructive-label span is visually hidden but still announced (position:absolute; width:1px; clip-path:inset(50%); CSS comment: "only exists for assistive tech"). So the screen-reader path was preserved when title="删除" was dropped — no SR regression. But mouse users did lose the hover hint (native title= gone, no Base UI Tooltip added).

Fix: wrapped the delete button in a Base UI Tooltip (same shape as the artifact-pane collapse button). TooltipContent mirrors 删除/删除中… for hover; the visually-hidden span still carries the SR label. The two are complementary, not redundant.

P3 — tooltip three-layer JSX repetition (deferred)

Verified. 13 sites are highly isomorphic (<Tooltip><TooltipTrigger render={<Button variant="quiet" size="icon-sm"/>} ...props><Icon/></TooltipTrigger><TooltipContent>label</TooltipContent></Tooltip>).

Defering this one. A TooltipButton helper would save ~15-27 lines net, but the 13 sites' trigger props are heterogeneous (disabled / aria-expanded / conditional icon / conditional label / UiButton vs Button), so the helper becomes a pass-through wrapper (tooltip + render + {...triggerProps} + children) that mostly removes the <Tooltip>/<TooltipContent> wrapping — a modest win for a new abstraction layer + an app-region-hygiene regex adjustment (it extracts classNames from <TooltipTrigger render={<UiButton …/>}> blocks today). Happy to do it as a follow-up if the tooltip site count grows; for 13 sites the direct form is clearer and more flexible. Pushing back rather than adding abstraction for line-count.

待验证 — CI checks / hand-test

Local verification done:npm run -w @maka/desktop test → 2009/2009 pass. npm run typecheck clean across workspaces. Screenshots vs main: turn-narrative 0.09% (the disclosure commit's summary→button residual), settings-appearance 0.0136 (PR4's settings-nav snap, untouched here), first-run 0% — all hover-only tooltip / controlled-open changes add 0 at rest.

CI checks show 0 — will confirm CI triggers on the updated push. Will hand-test before merge: Collapsible keyboard toggle + status-following collapse, Tooltip hover/focus, NumberField empty/invalid input (proxy 0↔empty, gateway empty→3939).


Summary: 3 fix commits pushed (c06d50a9, f4dba2c2, 28f6ed7a). 1 item deferred with rationale (tooltip helper). Issue #520 re-arranged: #22 input-canonical moved to PR8 (not a clean look-preserving swap — primitives/input.tsx carries a different structure than the native ui.tsxInput, and bare-field-chrome-contract pins the native's data-maka-field-chrome + border-input + ring-2).

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
Astro-Han added a commit that referenced this pull request Jul 5, 2026
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
Astro-Han added 10 commits July 5, 2026 17:50
… state attrs) (#520 PR5 item 23)
Foundation commit for PR5 — the convention the disclosure / tooltip /
number-field / input migrations in the following commits follow.
data-slot: add data-slot="<name>" to all 20 Base UI wrappers in ui.tsx
(Button / Separator / Checkbox / DialogBackdrop / DialogPopup / Select
Trigger/List/Popup/Group/GroupLabel/Separator/Item / FieldDescription /
Label / Switch / Toggle / ToggleGroup / RadioGroup / Radio / Progress).
The ./primitives/ wrappers (accordion / alert / badge / …) already do
this; ui.tsx wrappers did not. data-slot is a stable CSS targeting hook
that survives className drift. Base UI does not emit data-slot itself, so
the wrapper owns it. No CSS targets these yet — this is forward-looking
convention, a visual no-op.
State-attribute form: adopt Base UI's native attribute-presence form
([data-active] / [data-open] / [data-checked] / [data-selected] /
[data-pressed] / [data-highlighted] / [data-disabled]), NOT the
attribute-value form ([data-active="true"]). Maka's renderer CSS has
zero state-attribute selectors today, so there is nothing to break and no
override layer to maintain. A per-component hook map (Tabs data-active /
Select data-[highlighted]/data-[selected] / Checkbox/Switch/Radio
data-[checked] / Toggle data-[pressed] / Dialog/Tooltip/Popover data-[open])
is documented in a doc comment at the top of ui.tsx, along with the
whitelisted CSS var hooks (--anchor-* / --available-* / --active-tab-*).
className(state) function form is deferred — add only when a migration in
this PR actually needs state-based classes.
style-hook-convention-contract.test.ts locks the data-slot rule: every
ui.tsx wrapper that forwards to a <Base*> component must carry data-slot,
and the convention doc comment must stay present. Hand-written native
elements (legacy Input / Textarea / Badge) are out of scope until they
retire onto a Base UI primitive (input canonical, commit 5).
Verification: 1972/1972 desktop tests pass (+4 contract). typecheck clean.
renderer build clean. Visual no-op (data-slot is an attribute; no CSS
targets it yet).
…tem 17)
Migrate the four native <details>/<summary> disclosure sites onto Base UI
Collapsible, following the style-hook convention (item 23). The code comments
at the sites already said "future Base UI Accordion path"; all four are
independent single sections (not grouped), so Collapsible (not Accordion) is
the right primitive.
Sites:
- chat-view.tsx turn-thinking (maka-turn-thinking): uncontrolled, default
closed.
- chat-view.tsx reasoning-panel (maka-reasoning-panel): controlled open state
moves from reading e.currentTarget.open off the native toggle event to
Collapsible's open + onOpenChange props (default-open behavior preserved).
- permission-dialog.tsx permission-raw (maka-permission-raw): uncontrolled.
- tool-activity.tsx tool card ([data-slot="tool"]): uncontrolled with
defaultOpen = isOpenByDefault(status); the data-slot="tool" hook overrides
the Collapsible wrapper's default data-slot="collapsible" via prop spread,
so the existing [data-slot="tool"] CSS keeps working.
New primitive: packages/ui/src/primitives/collapsible.tsx wraps Base UI
Collapsible Root/Trigger/Panel with data-slot="collapsible" /
"collapsible-trigger" / "collapsible-panel". The Trigger carries a button
chrome reset (appearance-none / bg-transparent / border-0 / p-0 / font
inherit) so the summary row's clean look is preserved — native <summary> had
no background/border/padding, but <button> does. Exported from the barrel as
PrimitiveCollapsible / PrimitiveCollapsibleTrigger / PrimitiveCollapsiblePanel.
CSS retargeting (the DOM changed: <summary> -> <button> trigger, <details
open> -> Collapsible with data-panel-open on the trigger):
- .maka-turn-thinking summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-slot="collapsible-trigger"][data-panel-open]::before
- .maka-permission-raw > summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-panel-open] variants
- .maka-reasoning-panel-header marker resets dropped (button has no native
marker); [open] .maka-reasoning-panel-chevron ->
[data-slot="collapsible-trigger"][data-panel-open] .maka-reasoning-panel-chevron
- [data-slot="tool"] > summary ::-webkit-details-marker / ::marker resets
dropped (button trigger has no native marker).
- list-style: none rules dropped (no list marker on a button).
TDD: disclosure-collapsible-contract.test.ts written first (RED: sites still
used <details>, no primitive). GREEN: primitive + 4 migrations + CSS
retarget. The contract bans <details>/<summary> (incl. in comments) in the
three migrated files and requires a Collapsible import; it also pins the
primitive's data-slot on Root/Trigger/Panel.
Existing tests updated: border-width-converge-contract TRIANGLE_CARET_SELECTORS
(the chevron ::before border-width: 4px 0 4px 5px moved from summary::before to
[data-slot="collapsible-trigger"]::before); chat-marker-cascade-contract
(.maka-turn-thinking summary -> [data-slot="collapsible-trigger"]);
chat-tool-card-cascade-contract (the marker-reset residue on
[data-slot="tool"] > summary is gone — the trigger is a button now).
Verification: 1975/1975 desktop tests pass (+4 disclosure contract; the 3
existing contracts updated). typecheck clean. Screenshots vs clean main (PR4):
turn-narrative 0.09% (the summary->button swap's tiny pixel difference — button
baseline, the CollapsiblePanel wrapper), first-run 0%. No regression.
…tem 20)
Migrate the clearest icon-only-action button tooltips off the native title=
attribute (an unstyled, delayed browser tooltip) onto Base UI Tooltip,
which gives a themed, positioned, hover+focus tooltip matching the app.
Sites (13 buttons across 3 files):
- app-shell-chrome-actions.tsx: search / sidebar / new-task / feedback /
command-palette / help / health (7).
- browser-panel.tsx: back / forward / refresh / close (4).
- artifact-pane.tsx: the collapse toggle (1); the delete button's redundant
title= is dropped instead (it already shows a visible 删除 label, so a
tooltip would be a duplicate).
New primitive: packages/ui/src/primitives/tooltip.tsx wraps Base UI Tooltip
Root / Trigger / Content with data-slot="tooltip" / "tooltip-trigger" /
"tooltip-content". TooltipContent collapses Portal + Positioner + Popup
(the DialogContent shape) with a themed look (popover bg/corner/shadow +
z-overlay). The Trigger forwards via Base UI's render prop
(<TooltipTrigger render={<Button …/>} …props>…children…</TooltipTrigger>),
which merges the Trigger's props + children into the rendered Button — Base
UI v1 uses render, not the Radix asChild prop. Exported from the barrel as
Tooltip / TooltipTrigger / TooltipContent (no ui.tsx wrapper, so direct
names). aria-label stays on the button (accessible name); the TooltipContent
is the visual hover hint.
TDD: tooltip-converge-contract.test.ts written first (RED: the 3 files still
carried native title=, no primitive). GREEN: primitive + 13 migrations. The
contract bans a title= JSX attribute in the 3 migrated files and requires
a Tooltip import; it pins the primitive's data-slot on Root/Trigger/Content.
The regex \btitle= matches JSX attributes but not JS `const title =` (the
space before =).
Scope note: the longer tail of title= usages (truncation spans like
memory-settings path + chat-model-switcher chip, SelectTrigger titles,
status-badge icons, OnboardingHero's submit button) are tooltip-eligible but
each needs a label-vs-tooltip judgment and are not swept in this commit —
deferred to a follow-up. label-prop components (SettingRow / MetricCard /
SetupHero / Section) render title as visible text and are NOT tooltips;
they stay.
Existing tests updated: app-region-hygiene-contract's
extractStaticUiButtonClassNames now also matches the
<TooltipTrigger render={<UiButton/>}> shape (the className moved from the
UiButton element to the TooltipTrigger that renders it); search-modal-
lifecycle's shell-search-button regex now matches </TooltipTrigger> (the
UiButton is the render target, self-closing).
Verification: 1979/1979 desktop tests pass (+4 tooltip contract). typecheck
clean (render prop, not asChild). renderer build clean. Screenshots vs clean
main (PR4): turn-narrative 0.09% (the disclosure commit's residual; tooltips
are hover-only so rest state is unchanged), first-run 0%.
Migrate the two gateway/proxy port inputs off the native Input +
Number(event.currentTarget.value) hand-conversion onto Base UI NumberField,
which binds value: number | null directly and parses numeric input itself.
Sites:
- general-settings-page proxy port: value={proxyDraft.port || null}
(0 renders empty, matching the old String(port || '')), onValueChange
writes v ?? 0 (empty -> 0, matching the old Number(value) || 0).
- open-gateway-settings-page gateway port: value={gatewayDraft.port}
(0 renders "0", matching String(port)), onValueChange writes v ?? 3939
(empty -> 3939, matching Number(value) || 3939). inputMode="numeric" stays
on the input.
New primitive: packages/ui/src/primitives/number-field.tsx wraps Base UI
NumberField Root / Input with data-slot="number-field" / "number-field-input".
NumberFieldInput carries the maka standalone-input look via the shared
inputClasses export (the same classes the native ui.tsx Input used), so the
port inputs keep their look; unstyled gives the bare form for any future
Field/InputGroup embedding. Exported from the barrel as NumberField /
NumberFieldInput (no ui.tsx wrapper, so direct names, like Tooltip).
To share the maka input styling without duplicating it, ui.tsx exports
inputClasses + bareFieldClasses (const -> export const); the number-field
primitive imports them. (The input-canonical commit, item 22, will move
these into primitives/input.tsx and update this import.)
TDD: number-field-converge-contract.test.ts written first (RED: the two
files still carried Number(event.currentTarget.value), no primitive). GREEN:
primitive + 2 migrations. The contract bans the hand-conversion
(Number(event.currentTarget.value)) and requires a NumberField import; it
pins the primitive's data-slot on Root/Input. The hand-convert regex is
specific (Number(event.currentTarget.value)) so a different Number() call is
not flagged.
Existing test updated: settings-network-gateway-contract's two port-input
assertions now pin the NumberField form (value={...|| null} / value={...} +
onValueChange={(v) => update...({ port: v ?? default })}) instead of the old
String(value) + onChange Number(event.currentTarget.value). The intent —
"port input renders from the local draft while persisting in the background"
— is preserved; only the binding form changed. The "port input must not
lock after each digit" doesNotMatch still passes (the NumberFieldInput is not
disabled while saving).
Verification: 1982/1982 desktop tests pass (+3 number-field contract).
typecheck clean. renderer build clean. Screenshots: the port inputs are in
general/open-gateway settings (no capture scenario), so the captured
scenarios show no change — settings-appearance 0.0136 (PR4's settings-nav
snap residual) and turn-narrative 0.0009 (the disclosure commit's residual);
commit 4 adds 0. The port inputs keep their look via the shared inputClasses.
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
After rebasing PR5 onto main, the old PR5 style-hook commit still introduced a
SelectList wrapper, but main now has the ui.tsx dead-export contract and there
is no consumer for SelectList. Keep the style-hook convention on used wrappers
only and remove the dead export instead of adding a pending allowlist entry.
Verification: style-hook/divider/disclosure contracts pass (13/13), desktop
tests pass (2019/2019), typecheck clean.
@Astro-Han
Astro-Hanforce-pushed the feat/base-ui-small-migrations branch from 4a3c541 to 92fa1e1CompareJuly 5, 2026 09:53
@Astro-Han
Astro-Han merged commit cc3fc2c into mainJul 5, 2026
@Astro-Han
Astro-Han deleted the feat/base-ui-small-migrations branch July 5, 2026 09:54
Sign up for freeto 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

@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) by Astro-Han · Pull Request #543 · apache/maka · GitHub
Skip to content

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) - #543

Merged
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations
Jul 5, 2026
Merged

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5)#543
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

PR5 of #520 — the style-hook convention foundation + the smaller Base UI migrations that follow it. Four items, one commit each (each independently revertible), TDD red → green → refactor.

#22 (input canonical) was scoped into PR5 but moved to PR8 during this work — primitives/input.tsx carries a different structure (data-slot="input-control" span + inner Base UI input, ring-[3px]) than the native ui.tsxInput (single <input> + inputClasses, ring-2), and bare-field-chrome-contract.test.ts pins the native's data-maka-field-chrome + border-input + ring-2, so retiring the native is not a clean look-preserving swap. The issue plan was re-arranged (#22 → PR8); this PR opens with 1-4 only.

New primitives (packages/ui/src/primitives/)

PrimitiveWrapsdata-slot
collapsible.tsx@base-ui/react/collapsible Root/Trigger/Panelcollapsible / collapsible-trigger / collapsible-panel
tooltip.tsx@base-ui/react/tooltip Root/Trigger/Content (Portal+Positioner+Popup)tooltip / tooltip-trigger / tooltip-content
number-field.tsx@base-ui/react/number-field Root/Inputnumber-field / number-field-input

All follow the style-hook convention (item 23): data-slot on each part, Base UI's native boolean state attributes (data-panel-open, data-open, etc.) adopted as-is. The Collapsible Trigger carries a button-chrome reset (the native <summary> had no bg/border/padding, but <button> does). The Tooltip Trigger uses Base UI v1's render prop (not Radix asChild). The NumberField Input reuses maka's inputClasses (now exported from ui.tsx) so the port inputs keep their look.

Convergence

  • item 23 — style-hook convention: data-slot added to all 20 Base UI wrappers in ui.tsx (Button/Separator/Checkbox/Dialog backdrop+popup/Select Trigger+List+Popup+Group+GroupLabel+Separator+Item/FieldDescription/Label/Switch/Toggle/ToggleGroup/RadioGroup/Radio/Progress). A doc comment at the top of ui.tsx records the state-attribute decision (adopt Base UI native attribute-presence form — maka's renderer CSS has zero state-attribute selectors today, so nothing breaks), the per-component hook map, the whitelisted CSS var hooks, and the className(state) deferral.
  • item 17 — disclosure → Collapsible: the four <details>/<summary> sites (chat-view turn-thinking + reasoning-panel, permission-dialog permission-raw, tool-activity) migrate to Base UI Collapsible (all independent single sections, so Collapsible not Accordion). CSS retargeted: summary[data-slot="collapsible-trigger"], [open][data-panel-open], ::-webkit-details-marker/::marker resets dropped (button trigger has no native marker). The reasoning-panel's controlled-open moves from e.currentTarget.open off the toggle event to Collapsible's open + onOpenChange.
  • item 20 — tooltip → Base UI Tooltip: the 13 clearest icon-only-action button title= usages (app-shell-chrome-actions 7, browser-panel 4, artifact-pane collapse) migrate to Base UI Tooltip; aria-label stays on the button, TooltipContent is the visual hover hint. The artifact-pane delete button's redundant title= is dropped (it shows a visible 删除 label). The longer tail (truncation spans, SelectTrigger titles, status badges, OnboardingHero submit) are tooltip-eligible but need per-site label-vs-tooltip judgment — deferred to a follow-up.
  • item 21 — number-field: the two gateway/proxy port inputs migrate to Base UI NumberField (value: number | null, onValueChange), dropping the Number(event.currentTarget.value) hand-conversion. Proxy: 0 ↔ empty; gateway: empty → 3939 default, both preserved.

New contracts

ContractBansPins
style-hook-convention-contract.test.tsa ui.tsx wrapper forwarding to <Base*> without data-slotthe convention doc comment present
disclosure-collapsible-contract.test.ts<details>/<summary> (incl. in comments) in the 3 migrated files; missing Collapsible importprimitives/collapsible.tsx data-slot on Root/Trigger/Panel
tooltip-converge-contract.test.tsa title= JSX attribute in the 3 migrated files; missing Tooltip importprimitives/tooltip.tsx data-slot on Root/Trigger/Content
number-field-converge-contract.test.tsNumber(event.currentTarget.value) hand-conversion in the 2 port files; missing NumberField importprimitives/number-field.tsx data-slot on Root/Input

Existing tests updated

  • border-width-converge-contract: the triangle-caret allowlist selectors moved from summary::before to [data-slot="collapsible-trigger"]::before (the chevron border-width: 4px 0 4px 5px moved with the disclosure migration).
  • chat-marker-cascade-contract / chat-tool-card-cascade-contract: the turn-thinking / tool-card residue selectors + the dropped marker-reset residue.
  • app-region-hygiene-contract / search-modal-lifecycle-contract: the topbar-button className extraction + the shell-search-button regex now match the <TooltipTrigger render={<UiButton/>}> shape (the className moved from the UiButton element to the TooltipTrigger that renders it).
  • settings-network-gateway-contract: the two port-input assertions now pin the NumberField form (value={...|| null} / value={...} + onValueChange={(v) => update…({ port: v ?? default })).

Commits

  1. feat(ui): establish Base UI style-hook convention (data-slot + native state attrs) — item 23 (foundation).
  2. feat(ui): migrate disclosure sites to Base UI Collapsible — item 17.
  3. feat(ui): migrate icon-action tooltips to Base UI Tooltip — item 20.
  4. feat(ui): migrate port inputs to Base UI NumberField — item 21.

Verification

  • Tests: npm run -w @maka/desktop test — 2008/2008 pass (rebased onto main with PR2; +26 from PR2, +10 new contract assertions across 4 files). typecheck clean.
  • Screenshots vs main (turn-narrative, settings-appearance, first-run × light): tooltips are hover-only so rest state is unchanged; turn-narrative 0.09% (the disclosure commit's summary→button pixel difference), settings-appearance 0.0136 (PR4's settings-nav snap residual, untouched here), first-run 0. The port inputs (general/open-gateway settings) have no capture scenario; they keep their look via the shared inputClasses.

Out of scope / deferred

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review. Verified every claim against the code before acting; results + fixes below.

P2 — tool-activity defaultOpen (fixed, c06d50a9)

Verified. The disclosure migration set defaultOpen={isOpenByDefault(item.status)} (uncontrolled — open decided once on first render). The pre-Collapsible <details open={isOpenByDefault(status)}> re-evaluated open every render, so a card that defaulted open while pending/running no longer auto-collapsed when it settled. Real regression.

Fix: extracted a ToolActivityCard component (hooks can't live in the .map callback) with a controlled state machine: useState(isOpenByDefault(item.status)) seeds open, useEffect re-syncs it on [item.status] so a pending/running card collapses when it settles, onOpenChange={setOpen} lets the user toggle in between (the next status change re-syncs). This restores the status-following behavior and makes the user-toggle intentional rather than a React-not-rerendering side effect.

Test:disclosure-collapsible-contract now asserts tool-activity must NOT use defaultOpen, must use onOpenChange, and must re-sync via useEffect on [item.status]. (maka has no React testing-library / react-test-renderer, so the behavior is locked structurally + verified by hand; a rerender-act test is out of scope.) Will hand-test pending/running → completed/interrupted before merge.

P3 — Collapsible barrel / contract mismatch (fixed, f4dba2c2)

Verified. Barrel exported Collapsible as PrimitiveCollapsible (the old Tabs/Accordion convention), while Tooltip/NumberField export directly. The contract regex allowed @maka/ui as an import path, but the barrel didn't export Collapsible under that name — a phantom-pass path. The 3 internal sites import via ./primitives/collapsible.js (the in-package convention — alert/chat all use relative primitive paths), so the test passed for the right reason, but the @maka/ui allowance was misleading.

Fix: barrel now exports Collapsible / CollapsibleTrigger / CollapsiblePanel directly (dropped the Primitive* aliases and the CollapsiblePrimitive re-export — nothing consumes them; Tooltip/NumberField don't export their Primitive either). Internal sites keep their relative-path imports (convention); @maka/ui is now genuinely available rather than phantom.

P3 — tooltip.tsx doc showed asChild (fixed, f4dba2c2)

Verified. Line 14 usage doc showed <TooltipTrigger asChild> (Radix), but this PR uses Base UI v1's render prop. Fixed the example to render={<Button />} and noted the Trigger merges its props + children into the rendered element, so the next person doesn't copy the comment and hit the typecheck failure the migration hit.

待验证 — artifact delete title= (fixed, 28f6ed7a)

Partially verified — pushing back on the a11y part. The .maka-artifact-toolbar-destructive-label span is visually hidden but still announced (position:absolute; width:1px; clip-path:inset(50%); CSS comment: "only exists for assistive tech"). So the screen-reader path was preserved when title="删除" was dropped — no SR regression. But mouse users did lose the hover hint (native title= gone, no Base UI Tooltip added).

Fix: wrapped the delete button in a Base UI Tooltip (same shape as the artifact-pane collapse button). TooltipContent mirrors 删除/删除中… for hover; the visually-hidden span still carries the SR label. The two are complementary, not redundant.

P3 — tooltip three-layer JSX repetition (deferred)

Verified. 13 sites are highly isomorphic (<Tooltip><TooltipTrigger render={<Button variant="quiet" size="icon-sm"/>} ...props><Icon/></TooltipTrigger><TooltipContent>label</TooltipContent></Tooltip>).

Defering this one. A TooltipButton helper would save ~15-27 lines net, but the 13 sites' trigger props are heterogeneous (disabled / aria-expanded / conditional icon / conditional label / UiButton vs Button), so the helper becomes a pass-through wrapper (tooltip + render + {...triggerProps} + children) that mostly removes the <Tooltip>/<TooltipContent> wrapping — a modest win for a new abstraction layer + an app-region-hygiene regex adjustment (it extracts classNames from <TooltipTrigger render={<UiButton …/>}> blocks today). Happy to do it as a follow-up if the tooltip site count grows; for 13 sites the direct form is clearer and more flexible. Pushing back rather than adding abstraction for line-count.

待验证 — CI checks / hand-test

Local verification done:npm run -w @maka/desktop test → 2009/2009 pass. npm run typecheck clean across workspaces. Screenshots vs main: turn-narrative 0.09% (the disclosure commit's summary→button residual), settings-appearance 0.0136 (PR4's settings-nav snap, untouched here), first-run 0% — all hover-only tooltip / controlled-open changes add 0 at rest.

CI checks show 0 — will confirm CI triggers on the updated push. Will hand-test before merge: Collapsible keyboard toggle + status-following collapse, Tooltip hover/focus, NumberField empty/invalid input (proxy 0↔empty, gateway empty→3939).


Summary: 3 fix commits pushed (c06d50a9, f4dba2c2, 28f6ed7a). 1 item deferred with rationale (tooltip helper). Issue #520 re-arranged: #22 input-canonical moved to PR8 (not a clean look-preserving swap — primitives/input.tsx carries a different structure than the native ui.tsxInput, and bare-field-chrome-contract pins the native's data-maka-field-chrome + border-input + ring-2).

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
Astro-Han added a commit that referenced this pull request Jul 5, 2026
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
Astro-Han added 10 commits July 5, 2026 17:50
… state attrs) (#520 PR5 item 23)
Foundation commit for PR5 — the convention the disclosure / tooltip /
number-field / input migrations in the following commits follow.
data-slot: add data-slot="<name>" to all 20 Base UI wrappers in ui.tsx
(Button / Separator / Checkbox / DialogBackdrop / DialogPopup / Select
Trigger/List/Popup/Group/GroupLabel/Separator/Item / FieldDescription /
Label / Switch / Toggle / ToggleGroup / RadioGroup / Radio / Progress).
The ./primitives/ wrappers (accordion / alert / badge / …) already do
this; ui.tsx wrappers did not. data-slot is a stable CSS targeting hook
that survives className drift. Base UI does not emit data-slot itself, so
the wrapper owns it. No CSS targets these yet — this is forward-looking
convention, a visual no-op.
State-attribute form: adopt Base UI's native attribute-presence form
([data-active] / [data-open] / [data-checked] / [data-selected] /
[data-pressed] / [data-highlighted] / [data-disabled]), NOT the
attribute-value form ([data-active="true"]). Maka's renderer CSS has
zero state-attribute selectors today, so there is nothing to break and no
override layer to maintain. A per-component hook map (Tabs data-active /
Select data-[highlighted]/data-[selected] / Checkbox/Switch/Radio
data-[checked] / Toggle data-[pressed] / Dialog/Tooltip/Popover data-[open])
is documented in a doc comment at the top of ui.tsx, along with the
whitelisted CSS var hooks (--anchor-* / --available-* / --active-tab-*).
className(state) function form is deferred — add only when a migration in
this PR actually needs state-based classes.
style-hook-convention-contract.test.ts locks the data-slot rule: every
ui.tsx wrapper that forwards to a <Base*> component must carry data-slot,
and the convention doc comment must stay present. Hand-written native
elements (legacy Input / Textarea / Badge) are out of scope until they
retire onto a Base UI primitive (input canonical, commit 5).
Verification: 1972/1972 desktop tests pass (+4 contract). typecheck clean.
renderer build clean. Visual no-op (data-slot is an attribute; no CSS
targets it yet).
…tem 17)
Migrate the four native <details>/<summary> disclosure sites onto Base UI
Collapsible, following the style-hook convention (item 23). The code comments
at the sites already said "future Base UI Accordion path"; all four are
independent single sections (not grouped), so Collapsible (not Accordion) is
the right primitive.
Sites:
- chat-view.tsx turn-thinking (maka-turn-thinking): uncontrolled, default
closed.
- chat-view.tsx reasoning-panel (maka-reasoning-panel): controlled open state
moves from reading e.currentTarget.open off the native toggle event to
Collapsible's open + onOpenChange props (default-open behavior preserved).
- permission-dialog.tsx permission-raw (maka-permission-raw): uncontrolled.
- tool-activity.tsx tool card ([data-slot="tool"]): uncontrolled with
defaultOpen = isOpenByDefault(status); the data-slot="tool" hook overrides
the Collapsible wrapper's default data-slot="collapsible" via prop spread,
so the existing [data-slot="tool"] CSS keeps working.
New primitive: packages/ui/src/primitives/collapsible.tsx wraps Base UI
Collapsible Root/Trigger/Panel with data-slot="collapsible" /
"collapsible-trigger" / "collapsible-panel". The Trigger carries a button
chrome reset (appearance-none / bg-transparent / border-0 / p-0 / font
inherit) so the summary row's clean look is preserved — native <summary> had
no background/border/padding, but <button> does. Exported from the barrel as
PrimitiveCollapsible / PrimitiveCollapsibleTrigger / PrimitiveCollapsiblePanel.
CSS retargeting (the DOM changed: <summary> -> <button> trigger, <details
open> -> Collapsible with data-panel-open on the trigger):
- .maka-turn-thinking summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-slot="collapsible-trigger"][data-panel-open]::before
- .maka-permission-raw > summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-panel-open] variants
- .maka-reasoning-panel-header marker resets dropped (button has no native
marker); [open] .maka-reasoning-panel-chevron ->
[data-slot="collapsible-trigger"][data-panel-open] .maka-reasoning-panel-chevron
- [data-slot="tool"] > summary ::-webkit-details-marker / ::marker resets
dropped (button trigger has no native marker).
- list-style: none rules dropped (no list marker on a button).
TDD: disclosure-collapsible-contract.test.ts written first (RED: sites still
used <details>, no primitive). GREEN: primitive + 4 migrations + CSS
retarget. The contract bans <details>/<summary> (incl. in comments) in the
three migrated files and requires a Collapsible import; it also pins the
primitive's data-slot on Root/Trigger/Panel.
Existing tests updated: border-width-converge-contract TRIANGLE_CARET_SELECTORS
(the chevron ::before border-width: 4px 0 4px 5px moved from summary::before to
[data-slot="collapsible-trigger"]::before); chat-marker-cascade-contract
(.maka-turn-thinking summary -> [data-slot="collapsible-trigger"]);
chat-tool-card-cascade-contract (the marker-reset residue on
[data-slot="tool"] > summary is gone — the trigger is a button now).
Verification: 1975/1975 desktop tests pass (+4 disclosure contract; the 3
existing contracts updated). typecheck clean. Screenshots vs clean main (PR4):
turn-narrative 0.09% (the summary->button swap's tiny pixel difference — button
baseline, the CollapsiblePanel wrapper), first-run 0%. No regression.
…tem 20)
Migrate the clearest icon-only-action button tooltips off the native title=
attribute (an unstyled, delayed browser tooltip) onto Base UI Tooltip,
which gives a themed, positioned, hover+focus tooltip matching the app.
Sites (13 buttons across 3 files):
- app-shell-chrome-actions.tsx: search / sidebar / new-task / feedback /
command-palette / help / health (7).
- browser-panel.tsx: back / forward / refresh / close (4).
- artifact-pane.tsx: the collapse toggle (1); the delete button's redundant
title= is dropped instead (it already shows a visible 删除 label, so a
tooltip would be a duplicate).
New primitive: packages/ui/src/primitives/tooltip.tsx wraps Base UI Tooltip
Root / Trigger / Content with data-slot="tooltip" / "tooltip-trigger" /
"tooltip-content". TooltipContent collapses Portal + Positioner + Popup
(the DialogContent shape) with a themed look (popover bg/corner/shadow +
z-overlay). The Trigger forwards via Base UI's render prop
(<TooltipTrigger render={<Button …/>} …props>…children…</TooltipTrigger>),
which merges the Trigger's props + children into the rendered Button — Base
UI v1 uses render, not the Radix asChild prop. Exported from the barrel as
Tooltip / TooltipTrigger / TooltipContent (no ui.tsx wrapper, so direct
names). aria-label stays on the button (accessible name); the TooltipContent
is the visual hover hint.
TDD: tooltip-converge-contract.test.ts written first (RED: the 3 files still
carried native title=, no primitive). GREEN: primitive + 13 migrations. The
contract bans a title= JSX attribute in the 3 migrated files and requires
a Tooltip import; it pins the primitive's data-slot on Root/Trigger/Content.
The regex \btitle= matches JSX attributes but not JS `const title =` (the
space before =).
Scope note: the longer tail of title= usages (truncation spans like
memory-settings path + chat-model-switcher chip, SelectTrigger titles,
status-badge icons, OnboardingHero's submit button) are tooltip-eligible but
each needs a label-vs-tooltip judgment and are not swept in this commit —
deferred to a follow-up. label-prop components (SettingRow / MetricCard /
SetupHero / Section) render title as visible text and are NOT tooltips;
they stay.
Existing tests updated: app-region-hygiene-contract's
extractStaticUiButtonClassNames now also matches the
<TooltipTrigger render={<UiButton/>}> shape (the className moved from the
UiButton element to the TooltipTrigger that renders it); search-modal-
lifecycle's shell-search-button regex now matches </TooltipTrigger> (the
UiButton is the render target, self-closing).
Verification: 1979/1979 desktop tests pass (+4 tooltip contract). typecheck
clean (render prop, not asChild). renderer build clean. Screenshots vs clean
main (PR4): turn-narrative 0.09% (the disclosure commit's residual; tooltips
are hover-only so rest state is unchanged), first-run 0%.
Migrate the two gateway/proxy port inputs off the native Input +
Number(event.currentTarget.value) hand-conversion onto Base UI NumberField,
which binds value: number | null directly and parses numeric input itself.
Sites:
- general-settings-page proxy port: value={proxyDraft.port || null}
(0 renders empty, matching the old String(port || '')), onValueChange
writes v ?? 0 (empty -> 0, matching the old Number(value) || 0).
- open-gateway-settings-page gateway port: value={gatewayDraft.port}
(0 renders "0", matching String(port)), onValueChange writes v ?? 3939
(empty -> 3939, matching Number(value) || 3939). inputMode="numeric" stays
on the input.
New primitive: packages/ui/src/primitives/number-field.tsx wraps Base UI
NumberField Root / Input with data-slot="number-field" / "number-field-input".
NumberFieldInput carries the maka standalone-input look via the shared
inputClasses export (the same classes the native ui.tsx Input used), so the
port inputs keep their look; unstyled gives the bare form for any future
Field/InputGroup embedding. Exported from the barrel as NumberField /
NumberFieldInput (no ui.tsx wrapper, so direct names, like Tooltip).
To share the maka input styling without duplicating it, ui.tsx exports
inputClasses + bareFieldClasses (const -> export const); the number-field
primitive imports them. (The input-canonical commit, item 22, will move
these into primitives/input.tsx and update this import.)
TDD: number-field-converge-contract.test.ts written first (RED: the two
files still carried Number(event.currentTarget.value), no primitive). GREEN:
primitive + 2 migrations. The contract bans the hand-conversion
(Number(event.currentTarget.value)) and requires a NumberField import; it
pins the primitive's data-slot on Root/Input. The hand-convert regex is
specific (Number(event.currentTarget.value)) so a different Number() call is
not flagged.
Existing test updated: settings-network-gateway-contract's two port-input
assertions now pin the NumberField form (value={...|| null} / value={...} +
onValueChange={(v) => update...({ port: v ?? default })}) instead of the old
String(value) + onChange Number(event.currentTarget.value). The intent —
"port input renders from the local draft while persisting in the background"
— is preserved; only the binding form changed. The "port input must not
lock after each digit" doesNotMatch still passes (the NumberFieldInput is not
disabled while saving).
Verification: 1982/1982 desktop tests pass (+3 number-field contract).
typecheck clean. renderer build clean. Screenshots: the port inputs are in
general/open-gateway settings (no capture scenario), so the captured
scenarios show no change — settings-appearance 0.0136 (PR4's settings-nav
snap residual) and turn-narrative 0.0009 (the disclosure commit's residual);
commit 4 adds 0. The port inputs keep their look via the shared inputClasses.
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
After rebasing PR5 onto main, the old PR5 style-hook commit still introduced a
SelectList wrapper, but main now has the ui.tsx dead-export contract and there
is no consumer for SelectList. Keep the style-hook convention on used wrappers
only and remove the dead export instead of adding a pending allowlist entry.
Verification: style-hook/divider/disclosure contracts pass (13/13), desktop
tests pass (2019/2019), typecheck clean.
@Astro-Han
Astro-Hanforce-pushed the feat/base-ui-small-migrations branch from 4a3c541 to 92fa1e1CompareJuly 5, 2026 09:53
@Astro-Han
Astro-Han merged commit cc3fc2c into mainJul 5, 2026
@Astro-Han
Astro-Han deleted the feat/base-ui-small-migrations branch July 5, 2026 09:54
Sign up for freeto 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

@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) by Astro-Han · Pull Request #543 · apache/maka · GitHub
Skip to content

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) - #543

Merged
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations
Jul 5, 2026
Merged

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5)#543
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

PR5 of #520 — the style-hook convention foundation + the smaller Base UI migrations that follow it. Four items, one commit each (each independently revertible), TDD red → green → refactor.

#22 (input canonical) was scoped into PR5 but moved to PR8 during this work — primitives/input.tsx carries a different structure (data-slot="input-control" span + inner Base UI input, ring-[3px]) than the native ui.tsxInput (single <input> + inputClasses, ring-2), and bare-field-chrome-contract.test.ts pins the native's data-maka-field-chrome + border-input + ring-2, so retiring the native is not a clean look-preserving swap. The issue plan was re-arranged (#22 → PR8); this PR opens with 1-4 only.

New primitives (packages/ui/src/primitives/)

PrimitiveWrapsdata-slot
collapsible.tsx@base-ui/react/collapsible Root/Trigger/Panelcollapsible / collapsible-trigger / collapsible-panel
tooltip.tsx@base-ui/react/tooltip Root/Trigger/Content (Portal+Positioner+Popup)tooltip / tooltip-trigger / tooltip-content
number-field.tsx@base-ui/react/number-field Root/Inputnumber-field / number-field-input

All follow the style-hook convention (item 23): data-slot on each part, Base UI's native boolean state attributes (data-panel-open, data-open, etc.) adopted as-is. The Collapsible Trigger carries a button-chrome reset (the native <summary> had no bg/border/padding, but <button> does). The Tooltip Trigger uses Base UI v1's render prop (not Radix asChild). The NumberField Input reuses maka's inputClasses (now exported from ui.tsx) so the port inputs keep their look.

Convergence

  • item 23 — style-hook convention: data-slot added to all 20 Base UI wrappers in ui.tsx (Button/Separator/Checkbox/Dialog backdrop+popup/Select Trigger+List+Popup+Group+GroupLabel+Separator+Item/FieldDescription/Label/Switch/Toggle/ToggleGroup/RadioGroup/Radio/Progress). A doc comment at the top of ui.tsx records the state-attribute decision (adopt Base UI native attribute-presence form — maka's renderer CSS has zero state-attribute selectors today, so nothing breaks), the per-component hook map, the whitelisted CSS var hooks, and the className(state) deferral.
  • item 17 — disclosure → Collapsible: the four <details>/<summary> sites (chat-view turn-thinking + reasoning-panel, permission-dialog permission-raw, tool-activity) migrate to Base UI Collapsible (all independent single sections, so Collapsible not Accordion). CSS retargeted: summary[data-slot="collapsible-trigger"], [open][data-panel-open], ::-webkit-details-marker/::marker resets dropped (button trigger has no native marker). The reasoning-panel's controlled-open moves from e.currentTarget.open off the toggle event to Collapsible's open + onOpenChange.
  • item 20 — tooltip → Base UI Tooltip: the 13 clearest icon-only-action button title= usages (app-shell-chrome-actions 7, browser-panel 4, artifact-pane collapse) migrate to Base UI Tooltip; aria-label stays on the button, TooltipContent is the visual hover hint. The artifact-pane delete button's redundant title= is dropped (it shows a visible 删除 label). The longer tail (truncation spans, SelectTrigger titles, status badges, OnboardingHero submit) are tooltip-eligible but need per-site label-vs-tooltip judgment — deferred to a follow-up.
  • item 21 — number-field: the two gateway/proxy port inputs migrate to Base UI NumberField (value: number | null, onValueChange), dropping the Number(event.currentTarget.value) hand-conversion. Proxy: 0 ↔ empty; gateway: empty → 3939 default, both preserved.

New contracts

ContractBansPins
style-hook-convention-contract.test.tsa ui.tsx wrapper forwarding to <Base*> without data-slotthe convention doc comment present
disclosure-collapsible-contract.test.ts<details>/<summary> (incl. in comments) in the 3 migrated files; missing Collapsible importprimitives/collapsible.tsx data-slot on Root/Trigger/Panel
tooltip-converge-contract.test.tsa title= JSX attribute in the 3 migrated files; missing Tooltip importprimitives/tooltip.tsx data-slot on Root/Trigger/Content
number-field-converge-contract.test.tsNumber(event.currentTarget.value) hand-conversion in the 2 port files; missing NumberField importprimitives/number-field.tsx data-slot on Root/Input

Existing tests updated

  • border-width-converge-contract: the triangle-caret allowlist selectors moved from summary::before to [data-slot="collapsible-trigger"]::before (the chevron border-width: 4px 0 4px 5px moved with the disclosure migration).
  • chat-marker-cascade-contract / chat-tool-card-cascade-contract: the turn-thinking / tool-card residue selectors + the dropped marker-reset residue.
  • app-region-hygiene-contract / search-modal-lifecycle-contract: the topbar-button className extraction + the shell-search-button regex now match the <TooltipTrigger render={<UiButton/>}> shape (the className moved from the UiButton element to the TooltipTrigger that renders it).
  • settings-network-gateway-contract: the two port-input assertions now pin the NumberField form (value={...|| null} / value={...} + onValueChange={(v) => update…({ port: v ?? default })).

Commits

  1. feat(ui): establish Base UI style-hook convention (data-slot + native state attrs) — item 23 (foundation).
  2. feat(ui): migrate disclosure sites to Base UI Collapsible — item 17.
  3. feat(ui): migrate icon-action tooltips to Base UI Tooltip — item 20.
  4. feat(ui): migrate port inputs to Base UI NumberField — item 21.

Verification

  • Tests: npm run -w @maka/desktop test — 2008/2008 pass (rebased onto main with PR2; +26 from PR2, +10 new contract assertions across 4 files). typecheck clean.
  • Screenshots vs main (turn-narrative, settings-appearance, first-run × light): tooltips are hover-only so rest state is unchanged; turn-narrative 0.09% (the disclosure commit's summary→button pixel difference), settings-appearance 0.0136 (PR4's settings-nav snap residual, untouched here), first-run 0. The port inputs (general/open-gateway settings) have no capture scenario; they keep their look via the shared inputClasses.

Out of scope / deferred

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review. Verified every claim against the code before acting; results + fixes below.

P2 — tool-activity defaultOpen (fixed, c06d50a9)

Verified. The disclosure migration set defaultOpen={isOpenByDefault(item.status)} (uncontrolled — open decided once on first render). The pre-Collapsible <details open={isOpenByDefault(status)}> re-evaluated open every render, so a card that defaulted open while pending/running no longer auto-collapsed when it settled. Real regression.

Fix: extracted a ToolActivityCard component (hooks can't live in the .map callback) with a controlled state machine: useState(isOpenByDefault(item.status)) seeds open, useEffect re-syncs it on [item.status] so a pending/running card collapses when it settles, onOpenChange={setOpen} lets the user toggle in between (the next status change re-syncs). This restores the status-following behavior and makes the user-toggle intentional rather than a React-not-rerendering side effect.

Test:disclosure-collapsible-contract now asserts tool-activity must NOT use defaultOpen, must use onOpenChange, and must re-sync via useEffect on [item.status]. (maka has no React testing-library / react-test-renderer, so the behavior is locked structurally + verified by hand; a rerender-act test is out of scope.) Will hand-test pending/running → completed/interrupted before merge.

P3 — Collapsible barrel / contract mismatch (fixed, f4dba2c2)

Verified. Barrel exported Collapsible as PrimitiveCollapsible (the old Tabs/Accordion convention), while Tooltip/NumberField export directly. The contract regex allowed @maka/ui as an import path, but the barrel didn't export Collapsible under that name — a phantom-pass path. The 3 internal sites import via ./primitives/collapsible.js (the in-package convention — alert/chat all use relative primitive paths), so the test passed for the right reason, but the @maka/ui allowance was misleading.

Fix: barrel now exports Collapsible / CollapsibleTrigger / CollapsiblePanel directly (dropped the Primitive* aliases and the CollapsiblePrimitive re-export — nothing consumes them; Tooltip/NumberField don't export their Primitive either). Internal sites keep their relative-path imports (convention); @maka/ui is now genuinely available rather than phantom.

P3 — tooltip.tsx doc showed asChild (fixed, f4dba2c2)

Verified. Line 14 usage doc showed <TooltipTrigger asChild> (Radix), but this PR uses Base UI v1's render prop. Fixed the example to render={<Button />} and noted the Trigger merges its props + children into the rendered element, so the next person doesn't copy the comment and hit the typecheck failure the migration hit.

待验证 — artifact delete title= (fixed, 28f6ed7a)

Partially verified — pushing back on the a11y part. The .maka-artifact-toolbar-destructive-label span is visually hidden but still announced (position:absolute; width:1px; clip-path:inset(50%); CSS comment: "only exists for assistive tech"). So the screen-reader path was preserved when title="删除" was dropped — no SR regression. But mouse users did lose the hover hint (native title= gone, no Base UI Tooltip added).

Fix: wrapped the delete button in a Base UI Tooltip (same shape as the artifact-pane collapse button). TooltipContent mirrors 删除/删除中… for hover; the visually-hidden span still carries the SR label. The two are complementary, not redundant.

P3 — tooltip three-layer JSX repetition (deferred)

Verified. 13 sites are highly isomorphic (<Tooltip><TooltipTrigger render={<Button variant="quiet" size="icon-sm"/>} ...props><Icon/></TooltipTrigger><TooltipContent>label</TooltipContent></Tooltip>).

Defering this one. A TooltipButton helper would save ~15-27 lines net, but the 13 sites' trigger props are heterogeneous (disabled / aria-expanded / conditional icon / conditional label / UiButton vs Button), so the helper becomes a pass-through wrapper (tooltip + render + {...triggerProps} + children) that mostly removes the <Tooltip>/<TooltipContent> wrapping — a modest win for a new abstraction layer + an app-region-hygiene regex adjustment (it extracts classNames from <TooltipTrigger render={<UiButton …/>}> blocks today). Happy to do it as a follow-up if the tooltip site count grows; for 13 sites the direct form is clearer and more flexible. Pushing back rather than adding abstraction for line-count.

待验证 — CI checks / hand-test

Local verification done:npm run -w @maka/desktop test → 2009/2009 pass. npm run typecheck clean across workspaces. Screenshots vs main: turn-narrative 0.09% (the disclosure commit's summary→button residual), settings-appearance 0.0136 (PR4's settings-nav snap, untouched here), first-run 0% — all hover-only tooltip / controlled-open changes add 0 at rest.

CI checks show 0 — will confirm CI triggers on the updated push. Will hand-test before merge: Collapsible keyboard toggle + status-following collapse, Tooltip hover/focus, NumberField empty/invalid input (proxy 0↔empty, gateway empty→3939).


Summary: 3 fix commits pushed (c06d50a9, f4dba2c2, 28f6ed7a). 1 item deferred with rationale (tooltip helper). Issue #520 re-arranged: #22 input-canonical moved to PR8 (not a clean look-preserving swap — primitives/input.tsx carries a different structure than the native ui.tsxInput, and bare-field-chrome-contract pins the native's data-maka-field-chrome + border-input + ring-2).

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
Astro-Han added a commit that referenced this pull request Jul 5, 2026
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
Astro-Han added 10 commits July 5, 2026 17:50
… state attrs) (#520 PR5 item 23)
Foundation commit for PR5 — the convention the disclosure / tooltip /
number-field / input migrations in the following commits follow.
data-slot: add data-slot="<name>" to all 20 Base UI wrappers in ui.tsx
(Button / Separator / Checkbox / DialogBackdrop / DialogPopup / Select
Trigger/List/Popup/Group/GroupLabel/Separator/Item / FieldDescription /
Label / Switch / Toggle / ToggleGroup / RadioGroup / Radio / Progress).
The ./primitives/ wrappers (accordion / alert / badge / …) already do
this; ui.tsx wrappers did not. data-slot is a stable CSS targeting hook
that survives className drift. Base UI does not emit data-slot itself, so
the wrapper owns it. No CSS targets these yet — this is forward-looking
convention, a visual no-op.
State-attribute form: adopt Base UI's native attribute-presence form
([data-active] / [data-open] / [data-checked] / [data-selected] /
[data-pressed] / [data-highlighted] / [data-disabled]), NOT the
attribute-value form ([data-active="true"]). Maka's renderer CSS has
zero state-attribute selectors today, so there is nothing to break and no
override layer to maintain. A per-component hook map (Tabs data-active /
Select data-[highlighted]/data-[selected] / Checkbox/Switch/Radio
data-[checked] / Toggle data-[pressed] / Dialog/Tooltip/Popover data-[open])
is documented in a doc comment at the top of ui.tsx, along with the
whitelisted CSS var hooks (--anchor-* / --available-* / --active-tab-*).
className(state) function form is deferred — add only when a migration in
this PR actually needs state-based classes.
style-hook-convention-contract.test.ts locks the data-slot rule: every
ui.tsx wrapper that forwards to a <Base*> component must carry data-slot,
and the convention doc comment must stay present. Hand-written native
elements (legacy Input / Textarea / Badge) are out of scope until they
retire onto a Base UI primitive (input canonical, commit 5).
Verification: 1972/1972 desktop tests pass (+4 contract). typecheck clean.
renderer build clean. Visual no-op (data-slot is an attribute; no CSS
targets it yet).
…tem 17)
Migrate the four native <details>/<summary> disclosure sites onto Base UI
Collapsible, following the style-hook convention (item 23). The code comments
at the sites already said "future Base UI Accordion path"; all four are
independent single sections (not grouped), so Collapsible (not Accordion) is
the right primitive.
Sites:
- chat-view.tsx turn-thinking (maka-turn-thinking): uncontrolled, default
closed.
- chat-view.tsx reasoning-panel (maka-reasoning-panel): controlled open state
moves from reading e.currentTarget.open off the native toggle event to
Collapsible's open + onOpenChange props (default-open behavior preserved).
- permission-dialog.tsx permission-raw (maka-permission-raw): uncontrolled.
- tool-activity.tsx tool card ([data-slot="tool"]): uncontrolled with
defaultOpen = isOpenByDefault(status); the data-slot="tool" hook overrides
the Collapsible wrapper's default data-slot="collapsible" via prop spread,
so the existing [data-slot="tool"] CSS keeps working.
New primitive: packages/ui/src/primitives/collapsible.tsx wraps Base UI
Collapsible Root/Trigger/Panel with data-slot="collapsible" /
"collapsible-trigger" / "collapsible-panel". The Trigger carries a button
chrome reset (appearance-none / bg-transparent / border-0 / p-0 / font
inherit) so the summary row's clean look is preserved — native <summary> had
no background/border/padding, but <button> does. Exported from the barrel as
PrimitiveCollapsible / PrimitiveCollapsibleTrigger / PrimitiveCollapsiblePanel.
CSS retargeting (the DOM changed: <summary> -> <button> trigger, <details
open> -> Collapsible with data-panel-open on the trigger):
- .maka-turn-thinking summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-slot="collapsible-trigger"][data-panel-open]::before
- .maka-permission-raw > summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-panel-open] variants
- .maka-reasoning-panel-header marker resets dropped (button has no native
marker); [open] .maka-reasoning-panel-chevron ->
[data-slot="collapsible-trigger"][data-panel-open] .maka-reasoning-panel-chevron
- [data-slot="tool"] > summary ::-webkit-details-marker / ::marker resets
dropped (button trigger has no native marker).
- list-style: none rules dropped (no list marker on a button).
TDD: disclosure-collapsible-contract.test.ts written first (RED: sites still
used <details>, no primitive). GREEN: primitive + 4 migrations + CSS
retarget. The contract bans <details>/<summary> (incl. in comments) in the
three migrated files and requires a Collapsible import; it also pins the
primitive's data-slot on Root/Trigger/Panel.
Existing tests updated: border-width-converge-contract TRIANGLE_CARET_SELECTORS
(the chevron ::before border-width: 4px 0 4px 5px moved from summary::before to
[data-slot="collapsible-trigger"]::before); chat-marker-cascade-contract
(.maka-turn-thinking summary -> [data-slot="collapsible-trigger"]);
chat-tool-card-cascade-contract (the marker-reset residue on
[data-slot="tool"] > summary is gone — the trigger is a button now).
Verification: 1975/1975 desktop tests pass (+4 disclosure contract; the 3
existing contracts updated). typecheck clean. Screenshots vs clean main (PR4):
turn-narrative 0.09% (the summary->button swap's tiny pixel difference — button
baseline, the CollapsiblePanel wrapper), first-run 0%. No regression.
…tem 20)
Migrate the clearest icon-only-action button tooltips off the native title=
attribute (an unstyled, delayed browser tooltip) onto Base UI Tooltip,
which gives a themed, positioned, hover+focus tooltip matching the app.
Sites (13 buttons across 3 files):
- app-shell-chrome-actions.tsx: search / sidebar / new-task / feedback /
command-palette / help / health (7).
- browser-panel.tsx: back / forward / refresh / close (4).
- artifact-pane.tsx: the collapse toggle (1); the delete button's redundant
title= is dropped instead (it already shows a visible 删除 label, so a
tooltip would be a duplicate).
New primitive: packages/ui/src/primitives/tooltip.tsx wraps Base UI Tooltip
Root / Trigger / Content with data-slot="tooltip" / "tooltip-trigger" /
"tooltip-content". TooltipContent collapses Portal + Positioner + Popup
(the DialogContent shape) with a themed look (popover bg/corner/shadow +
z-overlay). The Trigger forwards via Base UI's render prop
(<TooltipTrigger render={<Button …/>} …props>…children…</TooltipTrigger>),
which merges the Trigger's props + children into the rendered Button — Base
UI v1 uses render, not the Radix asChild prop. Exported from the barrel as
Tooltip / TooltipTrigger / TooltipContent (no ui.tsx wrapper, so direct
names). aria-label stays on the button (accessible name); the TooltipContent
is the visual hover hint.
TDD: tooltip-converge-contract.test.ts written first (RED: the 3 files still
carried native title=, no primitive). GREEN: primitive + 13 migrations. The
contract bans a title= JSX attribute in the 3 migrated files and requires
a Tooltip import; it pins the primitive's data-slot on Root/Trigger/Content.
The regex \btitle= matches JSX attributes but not JS `const title =` (the
space before =).
Scope note: the longer tail of title= usages (truncation spans like
memory-settings path + chat-model-switcher chip, SelectTrigger titles,
status-badge icons, OnboardingHero's submit button) are tooltip-eligible but
each needs a label-vs-tooltip judgment and are not swept in this commit —
deferred to a follow-up. label-prop components (SettingRow / MetricCard /
SetupHero / Section) render title as visible text and are NOT tooltips;
they stay.
Existing tests updated: app-region-hygiene-contract's
extractStaticUiButtonClassNames now also matches the
<TooltipTrigger render={<UiButton/>}> shape (the className moved from the
UiButton element to the TooltipTrigger that renders it); search-modal-
lifecycle's shell-search-button regex now matches </TooltipTrigger> (the
UiButton is the render target, self-closing).
Verification: 1979/1979 desktop tests pass (+4 tooltip contract). typecheck
clean (render prop, not asChild). renderer build clean. Screenshots vs clean
main (PR4): turn-narrative 0.09% (the disclosure commit's residual; tooltips
are hover-only so rest state is unchanged), first-run 0%.
Migrate the two gateway/proxy port inputs off the native Input +
Number(event.currentTarget.value) hand-conversion onto Base UI NumberField,
which binds value: number | null directly and parses numeric input itself.
Sites:
- general-settings-page proxy port: value={proxyDraft.port || null}
(0 renders empty, matching the old String(port || '')), onValueChange
writes v ?? 0 (empty -> 0, matching the old Number(value) || 0).
- open-gateway-settings-page gateway port: value={gatewayDraft.port}
(0 renders "0", matching String(port)), onValueChange writes v ?? 3939
(empty -> 3939, matching Number(value) || 3939). inputMode="numeric" stays
on the input.
New primitive: packages/ui/src/primitives/number-field.tsx wraps Base UI
NumberField Root / Input with data-slot="number-field" / "number-field-input".
NumberFieldInput carries the maka standalone-input look via the shared
inputClasses export (the same classes the native ui.tsx Input used), so the
port inputs keep their look; unstyled gives the bare form for any future
Field/InputGroup embedding. Exported from the barrel as NumberField /
NumberFieldInput (no ui.tsx wrapper, so direct names, like Tooltip).
To share the maka input styling without duplicating it, ui.tsx exports
inputClasses + bareFieldClasses (const -> export const); the number-field
primitive imports them. (The input-canonical commit, item 22, will move
these into primitives/input.tsx and update this import.)
TDD: number-field-converge-contract.test.ts written first (RED: the two
files still carried Number(event.currentTarget.value), no primitive). GREEN:
primitive + 2 migrations. The contract bans the hand-conversion
(Number(event.currentTarget.value)) and requires a NumberField import; it
pins the primitive's data-slot on Root/Input. The hand-convert regex is
specific (Number(event.currentTarget.value)) so a different Number() call is
not flagged.
Existing test updated: settings-network-gateway-contract's two port-input
assertions now pin the NumberField form (value={...|| null} / value={...} +
onValueChange={(v) => update...({ port: v ?? default })}) instead of the old
String(value) + onChange Number(event.currentTarget.value). The intent —
"port input renders from the local draft while persisting in the background"
— is preserved; only the binding form changed. The "port input must not
lock after each digit" doesNotMatch still passes (the NumberFieldInput is not
disabled while saving).
Verification: 1982/1982 desktop tests pass (+3 number-field contract).
typecheck clean. renderer build clean. Screenshots: the port inputs are in
general/open-gateway settings (no capture scenario), so the captured
scenarios show no change — settings-appearance 0.0136 (PR4's settings-nav
snap residual) and turn-narrative 0.0009 (the disclosure commit's residual);
commit 4 adds 0. The port inputs keep their look via the shared inputClasses.
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
After rebasing PR5 onto main, the old PR5 style-hook commit still introduced a
SelectList wrapper, but main now has the ui.tsx dead-export contract and there
is no consumer for SelectList. Keep the style-hook convention on used wrappers
only and remove the dead export instead of adding a pending allowlist entry.
Verification: style-hook/divider/disclosure contracts pass (13/13), desktop
tests pass (2019/2019), typecheck clean.
@Astro-Han
Astro-Hanforce-pushed the feat/base-ui-small-migrations branch from 4a3c541 to 92fa1e1CompareJuly 5, 2026 09:53
@Astro-Han
Astro-Han merged commit cc3fc2c into mainJul 5, 2026
@Astro-Han
Astro-Han deleted the feat/base-ui-small-migrations branch July 5, 2026 09:54
Sign up for freeto 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

@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) by Astro-Han · Pull Request #543 · apache/maka · GitHub
Skip to content

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5) - #543

Merged
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations
Jul 5, 2026
Merged

feat(ui): style-hook convention + small Base UI migrations (Collapsible/Tooltip/NumberField) (#520 PR5)#543
Astro-Han merged 10 commits into
mainfrom
feat/base-ui-small-migrations

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

PR5 of #520 — the style-hook convention foundation + the smaller Base UI migrations that follow it. Four items, one commit each (each independently revertible), TDD red → green → refactor.

#22 (input canonical) was scoped into PR5 but moved to PR8 during this work — primitives/input.tsx carries a different structure (data-slot="input-control" span + inner Base UI input, ring-[3px]) than the native ui.tsxInput (single <input> + inputClasses, ring-2), and bare-field-chrome-contract.test.ts pins the native's data-maka-field-chrome + border-input + ring-2, so retiring the native is not a clean look-preserving swap. The issue plan was re-arranged (#22 → PR8); this PR opens with 1-4 only.

New primitives (packages/ui/src/primitives/)

PrimitiveWrapsdata-slot
collapsible.tsx@base-ui/react/collapsible Root/Trigger/Panelcollapsible / collapsible-trigger / collapsible-panel
tooltip.tsx@base-ui/react/tooltip Root/Trigger/Content (Portal+Positioner+Popup)tooltip / tooltip-trigger / tooltip-content
number-field.tsx@base-ui/react/number-field Root/Inputnumber-field / number-field-input

All follow the style-hook convention (item 23): data-slot on each part, Base UI's native boolean state attributes (data-panel-open, data-open, etc.) adopted as-is. The Collapsible Trigger carries a button-chrome reset (the native <summary> had no bg/border/padding, but <button> does). The Tooltip Trigger uses Base UI v1's render prop (not Radix asChild). The NumberField Input reuses maka's inputClasses (now exported from ui.tsx) so the port inputs keep their look.

Convergence

  • item 23 — style-hook convention: data-slot added to all 20 Base UI wrappers in ui.tsx (Button/Separator/Checkbox/Dialog backdrop+popup/Select Trigger+List+Popup+Group+GroupLabel+Separator+Item/FieldDescription/Label/Switch/Toggle/ToggleGroup/RadioGroup/Radio/Progress). A doc comment at the top of ui.tsx records the state-attribute decision (adopt Base UI native attribute-presence form — maka's renderer CSS has zero state-attribute selectors today, so nothing breaks), the per-component hook map, the whitelisted CSS var hooks, and the className(state) deferral.
  • item 17 — disclosure → Collapsible: the four <details>/<summary> sites (chat-view turn-thinking + reasoning-panel, permission-dialog permission-raw, tool-activity) migrate to Base UI Collapsible (all independent single sections, so Collapsible not Accordion). CSS retargeted: summary[data-slot="collapsible-trigger"], [open][data-panel-open], ::-webkit-details-marker/::marker resets dropped (button trigger has no native marker). The reasoning-panel's controlled-open moves from e.currentTarget.open off the toggle event to Collapsible's open + onOpenChange.
  • item 20 — tooltip → Base UI Tooltip: the 13 clearest icon-only-action button title= usages (app-shell-chrome-actions 7, browser-panel 4, artifact-pane collapse) migrate to Base UI Tooltip; aria-label stays on the button, TooltipContent is the visual hover hint. The artifact-pane delete button's redundant title= is dropped (it shows a visible 删除 label). The longer tail (truncation spans, SelectTrigger titles, status badges, OnboardingHero submit) are tooltip-eligible but need per-site label-vs-tooltip judgment — deferred to a follow-up.
  • item 21 — number-field: the two gateway/proxy port inputs migrate to Base UI NumberField (value: number | null, onValueChange), dropping the Number(event.currentTarget.value) hand-conversion. Proxy: 0 ↔ empty; gateway: empty → 3939 default, both preserved.

New contracts

ContractBansPins
style-hook-convention-contract.test.tsa ui.tsx wrapper forwarding to <Base*> without data-slotthe convention doc comment present
disclosure-collapsible-contract.test.ts<details>/<summary> (incl. in comments) in the 3 migrated files; missing Collapsible importprimitives/collapsible.tsx data-slot on Root/Trigger/Panel
tooltip-converge-contract.test.tsa title= JSX attribute in the 3 migrated files; missing Tooltip importprimitives/tooltip.tsx data-slot on Root/Trigger/Content
number-field-converge-contract.test.tsNumber(event.currentTarget.value) hand-conversion in the 2 port files; missing NumberField importprimitives/number-field.tsx data-slot on Root/Input

Existing tests updated

  • border-width-converge-contract: the triangle-caret allowlist selectors moved from summary::before to [data-slot="collapsible-trigger"]::before (the chevron border-width: 4px 0 4px 5px moved with the disclosure migration).
  • chat-marker-cascade-contract / chat-tool-card-cascade-contract: the turn-thinking / tool-card residue selectors + the dropped marker-reset residue.
  • app-region-hygiene-contract / search-modal-lifecycle-contract: the topbar-button className extraction + the shell-search-button regex now match the <TooltipTrigger render={<UiButton/>}> shape (the className moved from the UiButton element to the TooltipTrigger that renders it).
  • settings-network-gateway-contract: the two port-input assertions now pin the NumberField form (value={...|| null} / value={...} + onValueChange={(v) => update…({ port: v ?? default })).

Commits

  1. feat(ui): establish Base UI style-hook convention (data-slot + native state attrs) — item 23 (foundation).
  2. feat(ui): migrate disclosure sites to Base UI Collapsible — item 17.
  3. feat(ui): migrate icon-action tooltips to Base UI Tooltip — item 20.
  4. feat(ui): migrate port inputs to Base UI NumberField — item 21.

Verification

  • Tests: npm run -w @maka/desktop test — 2008/2008 pass (rebased onto main with PR2; +26 from PR2, +10 new contract assertions across 4 files). typecheck clean.
  • Screenshots vs main (turn-narrative, settings-appearance, first-run × light): tooltips are hover-only so rest state is unchanged; turn-narrative 0.09% (the disclosure commit's summary→button pixel difference), settings-appearance 0.0136 (PR4's settings-nav snap residual, untouched here), first-run 0. The port inputs (general/open-gateway settings) have no capture scenario; they keep their look via the shared inputClasses.

Out of scope / deferred

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
Astro-Han added a commit that referenced this pull request Jul 5, 2026
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review. Verified every claim against the code before acting; results + fixes below.

P2 — tool-activity defaultOpen (fixed, c06d50a9)

Verified. The disclosure migration set defaultOpen={isOpenByDefault(item.status)} (uncontrolled — open decided once on first render). The pre-Collapsible <details open={isOpenByDefault(status)}> re-evaluated open every render, so a card that defaulted open while pending/running no longer auto-collapsed when it settled. Real regression.

Fix: extracted a ToolActivityCard component (hooks can't live in the .map callback) with a controlled state machine: useState(isOpenByDefault(item.status)) seeds open, useEffect re-syncs it on [item.status] so a pending/running card collapses when it settles, onOpenChange={setOpen} lets the user toggle in between (the next status change re-syncs). This restores the status-following behavior and makes the user-toggle intentional rather than a React-not-rerendering side effect.

Test:disclosure-collapsible-contract now asserts tool-activity must NOT use defaultOpen, must use onOpenChange, and must re-sync via useEffect on [item.status]. (maka has no React testing-library / react-test-renderer, so the behavior is locked structurally + verified by hand; a rerender-act test is out of scope.) Will hand-test pending/running → completed/interrupted before merge.

P3 — Collapsible barrel / contract mismatch (fixed, f4dba2c2)

Verified. Barrel exported Collapsible as PrimitiveCollapsible (the old Tabs/Accordion convention), while Tooltip/NumberField export directly. The contract regex allowed @maka/ui as an import path, but the barrel didn't export Collapsible under that name — a phantom-pass path. The 3 internal sites import via ./primitives/collapsible.js (the in-package convention — alert/chat all use relative primitive paths), so the test passed for the right reason, but the @maka/ui allowance was misleading.

Fix: barrel now exports Collapsible / CollapsibleTrigger / CollapsiblePanel directly (dropped the Primitive* aliases and the CollapsiblePrimitive re-export — nothing consumes them; Tooltip/NumberField don't export their Primitive either). Internal sites keep their relative-path imports (convention); @maka/ui is now genuinely available rather than phantom.

P3 — tooltip.tsx doc showed asChild (fixed, f4dba2c2)

Verified. Line 14 usage doc showed <TooltipTrigger asChild> (Radix), but this PR uses Base UI v1's render prop. Fixed the example to render={<Button />} and noted the Trigger merges its props + children into the rendered element, so the next person doesn't copy the comment and hit the typecheck failure the migration hit.

待验证 — artifact delete title= (fixed, 28f6ed7a)

Partially verified — pushing back on the a11y part. The .maka-artifact-toolbar-destructive-label span is visually hidden but still announced (position:absolute; width:1px; clip-path:inset(50%); CSS comment: "only exists for assistive tech"). So the screen-reader path was preserved when title="删除" was dropped — no SR regression. But mouse users did lose the hover hint (native title= gone, no Base UI Tooltip added).

Fix: wrapped the delete button in a Base UI Tooltip (same shape as the artifact-pane collapse button). TooltipContent mirrors 删除/删除中… for hover; the visually-hidden span still carries the SR label. The two are complementary, not redundant.

P3 — tooltip three-layer JSX repetition (deferred)

Verified. 13 sites are highly isomorphic (<Tooltip><TooltipTrigger render={<Button variant="quiet" size="icon-sm"/>} ...props><Icon/></TooltipTrigger><TooltipContent>label</TooltipContent></Tooltip>).

Defering this one. A TooltipButton helper would save ~15-27 lines net, but the 13 sites' trigger props are heterogeneous (disabled / aria-expanded / conditional icon / conditional label / UiButton vs Button), so the helper becomes a pass-through wrapper (tooltip + render + {...triggerProps} + children) that mostly removes the <Tooltip>/<TooltipContent> wrapping — a modest win for a new abstraction layer + an app-region-hygiene regex adjustment (it extracts classNames from <TooltipTrigger render={<UiButton …/>}> blocks today). Happy to do it as a follow-up if the tooltip site count grows; for 13 sites the direct form is clearer and more flexible. Pushing back rather than adding abstraction for line-count.

待验证 — CI checks / hand-test

Local verification done:npm run -w @maka/desktop test → 2009/2009 pass. npm run typecheck clean across workspaces. Screenshots vs main: turn-narrative 0.09% (the disclosure commit's summary→button residual), settings-appearance 0.0136 (PR4's settings-nav snap, untouched here), first-run 0% — all hover-only tooltip / controlled-open changes add 0 at rest.

CI checks show 0 — will confirm CI triggers on the updated push. Will hand-test before merge: Collapsible keyboard toggle + status-following collapse, Tooltip hover/focus, NumberField empty/invalid input (proxy 0↔empty, gateway empty→3939).


Summary: 3 fix commits pushed (c06d50a9, f4dba2c2, 28f6ed7a). 1 item deferred with rationale (tooltip helper). Issue #520 re-arranged: #22 input-canonical moved to PR8 (not a clean look-preserving swap — primitives/input.tsx carries a different structure than the native ui.tsxInput, and bare-field-chrome-contract pins the native's data-maka-field-chrome + border-input + ring-2).

Astro-Han added a commit that referenced this pull request Jul 5, 2026
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
Astro-Han added a commit that referenced this pull request Jul 5, 2026
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
Astro-Han added 10 commits July 5, 2026 17:50
… state attrs) (#520 PR5 item 23)
Foundation commit for PR5 — the convention the disclosure / tooltip /
number-field / input migrations in the following commits follow.
data-slot: add data-slot="<name>" to all 20 Base UI wrappers in ui.tsx
(Button / Separator / Checkbox / DialogBackdrop / DialogPopup / Select
Trigger/List/Popup/Group/GroupLabel/Separator/Item / FieldDescription /
Label / Switch / Toggle / ToggleGroup / RadioGroup / Radio / Progress).
The ./primitives/ wrappers (accordion / alert / badge / …) already do
this; ui.tsx wrappers did not. data-slot is a stable CSS targeting hook
that survives className drift. Base UI does not emit data-slot itself, so
the wrapper owns it. No CSS targets these yet — this is forward-looking
convention, a visual no-op.
State-attribute form: adopt Base UI's native attribute-presence form
([data-active] / [data-open] / [data-checked] / [data-selected] /
[data-pressed] / [data-highlighted] / [data-disabled]), NOT the
attribute-value form ([data-active="true"]). Maka's renderer CSS has
zero state-attribute selectors today, so there is nothing to break and no
override layer to maintain. A per-component hook map (Tabs data-active /
Select data-[highlighted]/data-[selected] / Checkbox/Switch/Radio
data-[checked] / Toggle data-[pressed] / Dialog/Tooltip/Popover data-[open])
is documented in a doc comment at the top of ui.tsx, along with the
whitelisted CSS var hooks (--anchor-* / --available-* / --active-tab-*).
className(state) function form is deferred — add only when a migration in
this PR actually needs state-based classes.
style-hook-convention-contract.test.ts locks the data-slot rule: every
ui.tsx wrapper that forwards to a <Base*> component must carry data-slot,
and the convention doc comment must stay present. Hand-written native
elements (legacy Input / Textarea / Badge) are out of scope until they
retire onto a Base UI primitive (input canonical, commit 5).
Verification: 1972/1972 desktop tests pass (+4 contract). typecheck clean.
renderer build clean. Visual no-op (data-slot is an attribute; no CSS
targets it yet).
…tem 17)
Migrate the four native <details>/<summary> disclosure sites onto Base UI
Collapsible, following the style-hook convention (item 23). The code comments
at the sites already said "future Base UI Accordion path"; all four are
independent single sections (not grouped), so Collapsible (not Accordion) is
the right primitive.
Sites:
- chat-view.tsx turn-thinking (maka-turn-thinking): uncontrolled, default
closed.
- chat-view.tsx reasoning-panel (maka-reasoning-panel): controlled open state
moves from reading e.currentTarget.open off the native toggle event to
Collapsible's open + onOpenChange props (default-open behavior preserved).
- permission-dialog.tsx permission-raw (maka-permission-raw): uncontrolled.
- tool-activity.tsx tool card ([data-slot="tool"]): uncontrolled with
defaultOpen = isOpenByDefault(status); the data-slot="tool" hook overrides
the Collapsible wrapper's default data-slot="collapsible" via prop spread,
so the existing [data-slot="tool"] CSS keeps working.
New primitive: packages/ui/src/primitives/collapsible.tsx wraps Base UI
Collapsible Root/Trigger/Panel with data-slot="collapsible" /
"collapsible-trigger" / "collapsible-panel". The Trigger carries a button
chrome reset (appearance-none / bg-transparent / border-0 / p-0 / font
inherit) so the summary row's clean look is preserved — native <summary> had
no background/border/padding, but <button> does. Exported from the barrel as
PrimitiveCollapsible / PrimitiveCollapsibleTrigger / PrimitiveCollapsiblePanel.
CSS retargeting (the DOM changed: <summary> -> <button> trigger, <details
open> -> Collapsible with data-panel-open on the trigger):
- .maka-turn-thinking summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-slot="collapsible-trigger"][data-panel-open]::before
- .maka-permission-raw > summary / [open] > summary::before ->
[data-slot="collapsible-trigger"] / [data-panel-open] variants
- .maka-reasoning-panel-header marker resets dropped (button has no native
marker); [open] .maka-reasoning-panel-chevron ->
[data-slot="collapsible-trigger"][data-panel-open] .maka-reasoning-panel-chevron
- [data-slot="tool"] > summary ::-webkit-details-marker / ::marker resets
dropped (button trigger has no native marker).
- list-style: none rules dropped (no list marker on a button).
TDD: disclosure-collapsible-contract.test.ts written first (RED: sites still
used <details>, no primitive). GREEN: primitive + 4 migrations + CSS
retarget. The contract bans <details>/<summary> (incl. in comments) in the
three migrated files and requires a Collapsible import; it also pins the
primitive's data-slot on Root/Trigger/Panel.
Existing tests updated: border-width-converge-contract TRIANGLE_CARET_SELECTORS
(the chevron ::before border-width: 4px 0 4px 5px moved from summary::before to
[data-slot="collapsible-trigger"]::before); chat-marker-cascade-contract
(.maka-turn-thinking summary -> [data-slot="collapsible-trigger"]);
chat-tool-card-cascade-contract (the marker-reset residue on
[data-slot="tool"] > summary is gone — the trigger is a button now).
Verification: 1975/1975 desktop tests pass (+4 disclosure contract; the 3
existing contracts updated). typecheck clean. Screenshots vs clean main (PR4):
turn-narrative 0.09% (the summary->button swap's tiny pixel difference — button
baseline, the CollapsiblePanel wrapper), first-run 0%. No regression.
…tem 20)
Migrate the clearest icon-only-action button tooltips off the native title=
attribute (an unstyled, delayed browser tooltip) onto Base UI Tooltip,
which gives a themed, positioned, hover+focus tooltip matching the app.
Sites (13 buttons across 3 files):
- app-shell-chrome-actions.tsx: search / sidebar / new-task / feedback /
command-palette / help / health (7).
- browser-panel.tsx: back / forward / refresh / close (4).
- artifact-pane.tsx: the collapse toggle (1); the delete button's redundant
title= is dropped instead (it already shows a visible 删除 label, so a
tooltip would be a duplicate).
New primitive: packages/ui/src/primitives/tooltip.tsx wraps Base UI Tooltip
Root / Trigger / Content with data-slot="tooltip" / "tooltip-trigger" /
"tooltip-content". TooltipContent collapses Portal + Positioner + Popup
(the DialogContent shape) with a themed look (popover bg/corner/shadow +
z-overlay). The Trigger forwards via Base UI's render prop
(<TooltipTrigger render={<Button …/>} …props>…children…</TooltipTrigger>),
which merges the Trigger's props + children into the rendered Button — Base
UI v1 uses render, not the Radix asChild prop. Exported from the barrel as
Tooltip / TooltipTrigger / TooltipContent (no ui.tsx wrapper, so direct
names). aria-label stays on the button (accessible name); the TooltipContent
is the visual hover hint.
TDD: tooltip-converge-contract.test.ts written first (RED: the 3 files still
carried native title=, no primitive). GREEN: primitive + 13 migrations. The
contract bans a title= JSX attribute in the 3 migrated files and requires
a Tooltip import; it pins the primitive's data-slot on Root/Trigger/Content.
The regex \btitle= matches JSX attributes but not JS `const title =` (the
space before =).
Scope note: the longer tail of title= usages (truncation spans like
memory-settings path + chat-model-switcher chip, SelectTrigger titles,
status-badge icons, OnboardingHero's submit button) are tooltip-eligible but
each needs a label-vs-tooltip judgment and are not swept in this commit —
deferred to a follow-up. label-prop components (SettingRow / MetricCard /
SetupHero / Section) render title as visible text and are NOT tooltips;
they stay.
Existing tests updated: app-region-hygiene-contract's
extractStaticUiButtonClassNames now also matches the
<TooltipTrigger render={<UiButton/>}> shape (the className moved from the
UiButton element to the TooltipTrigger that renders it); search-modal-
lifecycle's shell-search-button regex now matches </TooltipTrigger> (the
UiButton is the render target, self-closing).
Verification: 1979/1979 desktop tests pass (+4 tooltip contract). typecheck
clean (render prop, not asChild). renderer build clean. Screenshots vs clean
main (PR4): turn-narrative 0.09% (the disclosure commit's residual; tooltips
are hover-only so rest state is unchanged), first-run 0%.
Migrate the two gateway/proxy port inputs off the native Input +
Number(event.currentTarget.value) hand-conversion onto Base UI NumberField,
which binds value: number | null directly and parses numeric input itself.
Sites:
- general-settings-page proxy port: value={proxyDraft.port || null}
(0 renders empty, matching the old String(port || '')), onValueChange
writes v ?? 0 (empty -> 0, matching the old Number(value) || 0).
- open-gateway-settings-page gateway port: value={gatewayDraft.port}
(0 renders "0", matching String(port)), onValueChange writes v ?? 3939
(empty -> 3939, matching Number(value) || 3939). inputMode="numeric" stays
on the input.
New primitive: packages/ui/src/primitives/number-field.tsx wraps Base UI
NumberField Root / Input with data-slot="number-field" / "number-field-input".
NumberFieldInput carries the maka standalone-input look via the shared
inputClasses export (the same classes the native ui.tsx Input used), so the
port inputs keep their look; unstyled gives the bare form for any future
Field/InputGroup embedding. Exported from the barrel as NumberField /
NumberFieldInput (no ui.tsx wrapper, so direct names, like Tooltip).
To share the maka input styling without duplicating it, ui.tsx exports
inputClasses + bareFieldClasses (const -> export const); the number-field
primitive imports them. (The input-canonical commit, item 22, will move
these into primitives/input.tsx and update this import.)
TDD: number-field-converge-contract.test.ts written first (RED: the two
files still carried Number(event.currentTarget.value), no primitive). GREEN:
primitive + 2 migrations. The contract bans the hand-conversion
(Number(event.currentTarget.value)) and requires a NumberField import; it
pins the primitive's data-slot on Root/Input. The hand-convert regex is
specific (Number(event.currentTarget.value)) so a different Number() call is
not flagged.
Existing test updated: settings-network-gateway-contract's two port-input
assertions now pin the NumberField form (value={...|| null} / value={...} +
onValueChange={(v) => update...({ port: v ?? default })}) instead of the old
String(value) + onChange Number(event.currentTarget.value). The intent —
"port input renders from the local draft while persisting in the background"
— is preserved; only the binding form changed. The "port input must not
lock after each digit" doesNotMatch still passes (the NumberFieldInput is not
disabled while saving).
Verification: 1982/1982 desktop tests pass (+3 number-field contract).
typecheck clean. renderer build clean. Screenshots: the port inputs are in
general/open-gateway settings (no capture scenario), so the captured
scenarios show no change — settings-appearance 0.0136 (PR4's settings-nav
snap residual) and turn-narrative 0.0009 (the disclosure commit's residual);
commit 4 adds 0. The port inputs keep their look via the shared inputClasses.
…review P2)
The disclosure migration made tool-activity's tool cards `defaultOpen={isOpenByDefault(item.status)}` (uncontrolled — open is decided once on first render). That regressed the pre-Collapsible `<details open={isOpenByDefault(status)}>` behavior, which re-evaluated open every render: a card that defaults open while pending/running no longer auto-collapsed when it settled to completed/interrupted.
Restore the status-following behavior with a controlled state machine:
- Extract the per-item card into a `ToolActivityCard` component (hooks can't
live in the `.map` callback).
- `useState(isOpenByDefault(item.status))` seeds open; `useEffect` re-syncs it
to `isOpenByDefault(item.status)` on every `[item.status]` change, so a
pending/running card collapses when it settles.
- `onOpenChange={setOpen}` lets the user still toggle the card in between;
the next status change re-syncs. (The pre-Collapsible `<details>` let the
user toggle too, but only as a side effect of React not re-rendering — the
state machine makes that intentional and predictable.)
TDD: disclosure-collapsible-contract gains an assertion that tool-activity
must NOT use `defaultOpen`, must use `onOpenChange`, and must re-sync via
`useEffect` on `[item.status]`. RED first (defaultOpen was present, no
onOpenChange, no re-sync), then GREEN. (maka has no React testing-library /
react-test-renderer, so the behavior is locked structurally + verified by
hand; the rerender-act path is out of scope.)
Verification: 2009/2009 desktop tests pass (+1 contract assertion). typecheck
clean. turn-narrative screenshot unchanged (0.09% — the disclosure commit's
summary→button residual; the card state is fixed by the scenario seed, and
controlled-open matches the pre-Collapsible behavior at that fixed state).
 review P3)
Two unrelated P3 nits from review, both one-line-ish:
1. Collapsible barrel export was aliased `as PrimitiveCollapsible` (the old
Tabs/Accordion convention), while the other new-in-PR5 primitives (Tooltip,
NumberField) export directly. The disclosure-collapsible-contract regex
allowed `@maka/ui` as an import path, but the barrel didn't actually export
`Collapsible` under that name — a phantom-pass path. Export
Collapsible/CollapsibleTrigger/CollapsiblePanel directly (drop the
`Primitive*` aliases and the `CollapsiblePrimitive` re-export — nothing
consumes them, and Tooltip/NumberField don't export their Primitive either).
The internal sites keep importing via `./primitives/collapsible.js` (the
in-package convention — alert/chat/etc. all use relative primitive paths),
so the `@maka/ui` path is now genuinely available rather than phantom.
2. tooltip.tsx usage doc showed `<TooltipTrigger asChild>` (Radix), but this
PR's convention is Base UI v1's `render` prop. Fix the example to
`render={<Button />}` and note that the Trigger merges its props + children
into the rendered element, so the next person doesn't copy the comment and
hit the same typecheck failure the migration hit.
Verification: 2009/2009 desktop tests pass. typecheck clean. No visual change
(barrel export names + a doc comment).
The tooltip migration dropped the delete button's native `title="删除"` (the
button is icon-only at rest — the visible 删除/删除中… label is visually
hidden via .maka-artifact-toolbar-destructive-label, kept only for screen
readers). The screen-reader path was preserved, but mouse users lost the
hover hint.
Wrap the delete button in a Base UI Tooltip (same shape as the artifact-pane
collapse button already migrated in this PR): TooltipTrigger renders the
destructive Button, TooltipContent mirrors the 删除/删除中… label for hover.
The visually-hidden span still carries the SR label; the two are complementary,
not redundant.
Verification: 2009/2009 desktop tests pass. typecheck clean. turn-narrative
screenshot unchanged (0.09% — the disclosure commit's residual; the delete
button's tooltip is hover-only, and the button's rest appearance is unchanged).
…ew P2)
The disclosure migration left the tool-card open-state divider pinned to the
old native selector `[&[open]>summary]`. After migrating the card to Base UI
Collapsible, the real DOM is a root with `[data-open]` and a button trigger with
`data-slot="collapsible-trigger"`, so the old selector no longer matched and
open tool-card headers could lose the body divider.
Use the Base UI / ShadCN-style ownership instead of a root-owned child selector:
- the card root exposes a named group (`group/tool`) alongside the Base UI
`[data-open]` state,
- the styled trigger/header part declares the divider with
`group-data-[open]/tool:[border-bottom:1px_solid_var(--border)]`,
- stale native-disclosure comments are removed.
TDD: chat-tool-card-cascade-contract first failed because chat.tsx still kept
`[open]>summary`; it now requires the named group + trigger-owned divider and
bans the old selector from source/comments.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds and generated CSS contains the named
group data selector.
… review P3)
Base UI Collapsible.Trigger already exposes `data-panel-open` when its panel is
open, so the previous named root group (`group/tool` +
`group-data-[open]/tool`) was more machinery than this divider needs.
Keep the Base UI / ShadCN ownership principle but make it smaller: the styled
trigger/header part now owns the divider directly via
`data-[panel-open]:[border-bottom:1px_solid_var(--border)]`, and the root drops
`group/tool`.
TDD: chat-tool-card-cascade-contract first failed because the source still used
the root group path; it now bans both the old native `[open]>summary` selector
and the root group open-state path, and requires the trigger-level
`data-[panel-open]` divider.
Verification: related contracts pass (12/12), desktop tests pass (2009/2009),
typecheck clean, renderer build succeeds, generated CSS contains the
`data-[panel-open]` utility and no `group-data-[open]/tool` / `group\/tool`
selector. turn-narrative screenshot remains at the existing 0.09% disclosure
residual.
After rebasing PR5 onto main, the old PR5 style-hook commit still introduced a
SelectList wrapper, but main now has the ui.tsx dead-export contract and there
is no consumer for SelectList. Keep the style-hook convention on used wrappers
only and remove the dead export instead of adding a pending allowlist entry.
Verification: style-hook/divider/disclosure contracts pass (13/13), desktop
tests pass (2019/2019), typecheck clean.
@Astro-Han
Astro-Hanforce-pushed the feat/base-ui-small-migrations branch from 4a3c541 to 92fa1e1CompareJuly 5, 2026 09:53
@Astro-Han
Astro-Han merged commit cc3fc2c into mainJul 5, 2026
@Astro-Han
Astro-Han deleted the feat/base-ui-small-migrations branch July 5, 2026 09:54
Sign up for freeto 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

@Astro-Han