chore(chat-react)!: delete the legacy sandbox-ui picker adapter - #392
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 0654d38f
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-08-06T08:50:18Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 209.6s (2 bridge agents) |
| Total | 209.6s |
💰 Value — sound
Deletes the legacy ComposerAgentControls adapter (303 lines) from /chat-react now that canonical pickers in /web-react (#390) replace it; EntryComposer renders AgentSessionControls directly, no indirection.
- What it does: Deletes
ComposerAgentControlsand its fourComposer*Selectiontypes from/chat-react, removes six sandbox-ui type re-exports that only existed to serve the adapter, and retypesEntryComposer'sagentprop from the adapter'sComposerAgentControlsProps(nestedmodel/harness/profile/effortsub-objects) to the canonical flatAgentSessionControlsPropsfrom/web-react. The playgro - Goals it achieves: Eliminates a now-redundant adapter layer over sandbox-ui's legacy picker strip — the canonical pickers (
ModelPicker/EffortPicker/AgentSessionControlsin/web-react) merged in #390 (the parent commit) already replace it. Removes the indirection:EntryComposergoes fromComposerAgentControls → sandbox-ui's AgentSessionControlsto directly rendering/web-react's canonical `AgentSessionC - Assessment: A coherent deletion following the established codebase pattern (cf. #350 removing deprecated same-origin proxy transport after all fleet migrations). The adapter was explicitly marked as deprecated/legacy before this PR; its file header, the prior
chat-react/index.tsxheader, and the priorAGENTS.mdall warned it was frozen. Zero external consumers —grepacross the full repo (excluding node - Better / existing approach: none — this is the right approach. The canonical pickers are in place, the adapter was already marked deprecated, and the codebase has a precedent for removing deprecated shared adapters once fleet migrations complete (#350). No alternative implementation exists — the canonical
AgentSessionControlsis the one and only alternative, and this PR wires directly to it. - Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge warning: opencode/kimi-for-coding/k2p7: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":"queue_timeout","admission":{"active":20,"queued":3,"maxActive":20,"maxQueue":48}}}; opencode/zai-coding-plan/glm-5.2: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":
🎯 Usefulness — error
usefulness agent produced no parseable value-audit JSON.
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":"queue_timeout","admission":{"active":20,"queued":2,"maxActive":20,"maxQueue":48}}}; opencode/kimi-for-coding/k2p7: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":
No concerns — sound change, no better or existing approach found. ✅
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 80 | 73 | 61 | 61 |
| Confidence | 95 | 95 | 95 | 95 |
| Correctness | 80 | 73 | 61 | 61 |
| Security | 80 | 73 | 61 | 61 |
| Testing | 80 | 73 | 61 | 61 |
| Architecture | 80 | 73 | 61 | 61 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 8/8 planned shots over 17 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 17 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 17 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Breaking agent prop shape on EntryComposer — no runtime guard — src/chat-react/entry-composer.tsx
The
agentprop type changed fromComposerAgentControlsPropstoAgentSessionControlsProps. This renames every field:model: {value, onChange, models}becomes flatmodel, onModelChange, models;harness: {value, onChange, available}becomes flatharness, onHarnessChange, availableHarnesses;effort: {value, onChange}becomes flateffort, onEffortChange;profileandcontextare removed entirely. Every consumer's is a compile error. The!commit marker and docs acknowledge this, but there is no runtime back-compat adapter or descriptive error to help a product that updates the package without reading the migration doc. Recommend a one-line wrapper in the first release that maps the old shape to the new for one minor cycle.
🟠 MEDIUM Chat-context trim (cli-base / non-chat models) no longer enforced by the shared composer assembly — src/chat-react/entry-composer.tsx
The deleted ComposerAgentControls defaulted context='chat': it stripped cli-base from the harness picker (harnessesForContext) and dropped non-chat-capable models (modelsForContext: image/tts/embedding/audio-in) from the model picker, with 9 tests pinning it (removed in chat-react.test.tsx). The replacement wiring
agent ? <AgentSessionControls {...agent} />renders the canonical cluster with NO such trim: HarnessPicker.options falls back to Object.keys(HARNESS_LABELS) which includes 'cli-base'→'CLI', and models pass through unfiltered (web-react/agent-session-controls.tsx:104,210). Any product migratingagentper the props mapping in docs/ui-picker-canon.md without adding availableHarnesses and pre-filtering models silently re-exposes exactly the defect the adapter prevented, contradic
🟡 LOW CODEMAP syncs a pre-existing staleness unrelated to the PR's stated change — docs/CODEMAP.md
The ./design-canvas-react row moves 129→131 and ./engine 75→77 because NODE_CACHE_PIXEL_RATIO_MAX and resolveNodeCachePixelRatio (src/design-canvas-react/export-math.ts:94,104, re-exported through engine.ts
export * from './export-math') were previously omitted from the generated map. git diff base→head over src/design-canvas-react/ is empty, so these exports predate this PR and the base CODEMAP was stale — the regeneration correctly fixes it. Not a regression; informational only. No action required.
🟡 LOW Breaking deletion is in the codemap but consumer migration is out of this shot — docs/codemap.json
The codemap accurately reflects a BREAKING CHANGE: /chat-react drops 13 public symbols (ComposerAgentControls, AgentProfile*, HarnessType, ModelInfo, Reasoning*, Composer*Selection). The file itself is correct and freshness-gated, so this is a note, not a defect in this file: a published-symbol removal shipped as
chore(chat-react)!must be paired with an import audit of the fleet (docs/ui-picker-canon.md migration mapping is updated in the same PR). Verified the codemap deletion set is byte-exact with src/chat-react/index.tsx:26-32 and types.ts (only ComposerPlanModeSelection remains).
🟡 LOW Codemap regen bundles catch-up entries outside the commit's stated scope — docs/codemap.json
The commit message scopes this to /chat-react picker-adapter deletion, but the codemap diff also adds NODE_CACHE_PIXEL_RATIO_MAX + resolveNodeCachePixelRatio (design-canvas) and the ChatMessages
chromeprop (web-react) — symbols that landed in the base commit c1b12a6 whose codemap was stale. This commit fixes the staleness (correct direction), so it is not a defect; just an FYI that the regen touched entries beyond the commit's headline scope. No action required.
🟡 LOW Codemap regeneration includes non-PR catch-up entries — docs/codemap.json
The additions to
./design-canvas-reactand./design-canvas-react/engine(NODE_CACHE_PIXEL_RATIO_MAX, resolveNodeCachePixelRatio) are catch-up for exports that already existed in source at the base commit but were missing from the stale codemap. These aren't bugs — they're correct additions — but their presence alongside the PR's intentional chat-react removals makes the diff larger than the PR's actual scope. Consider regenerating the codemap in a separate hygiene commit next time to keep PRs surgical.
🟡 LOW ComposerPlanModeSelection doc description is module-level, not interface-specific — docs/llms-full.txt
The new description 'The vocabulary the composer surfaces speak.' is the first line of the JSDoc block in src/chat-react/types.ts, which describes the whole module's vocabulary rather than what ComposerPlanModeSelection (an {enabled, setEnabled, saving?} shape) actually is. Faithfully regenerated, so not a docs-gen bug, but a reader scanning llms-full.txt gets a generic phrase where the pre-change had none. Low-impact: fix would be in the source JSDoc (move the module summary above the interface, give the interface its own @description), not in this generated file.
🟡 LOW ComposerPlanModeSelection doc description lost its explanatory detail — docs/llms-full.txt
The entry's prose is now only 'The vocabulary the composer surfaces speak', replacing what the removed types.ts comment explained (the vocabulary is re-exported from owning packages; a second spelling of HarnessType is how products disagree). The regeneration faithfully transcribed the new source comment, so this is faithful — but the resulting line is low-information for an LLM consumer reading the full API surface. Optional; not blocking.
🟡 LOW Regenerated-by-hand docs have no in-CI sync check — docs/llms-full.txt
docs/llms-full.txt is generated by an external 'agent-docs' tool (per the header) and is not regenerated or diff-checked in CI or in the local signoff gate (signoff.config.mjs runs no doc-regeneration step). This PR happens to be fully in sync — every hunk verified against source — but nothing mechanically prevents a future source change from shipping a stale surface. Low severity because the artifact is accurate now; fix is a generator + 'git diff --exit-code' step in the gate.
🟡 LOW 'Omit an unavailable capability' is stale against the new required-props contract — examples/default-workspace.md
Lines 97-99 (retained) claim 'Omit an unavailable capability and its control stays hidden', but the canonical AgentSessionControlsProps (src/web-react/agent-session-controls.tsx:142-167) makes models/model/onModelChange/harness/onHarnessChange/effort/onEffortChange ALL REQUIRED — a product cannot omit just the model control; only the whole
agentprop is optional (entry-composer.tsx:34). The capability-driven framing (lines 78-79) now holds only at whole-row granularity. The doc also omits the real per-control switch,showHarness
🟡 LOW Profile-picker placement in the modes dock contradicts modes' contract — examples/default-workspace.md
Line 94-95 suggests a product-owned profile picker live in 'the
modesdock or a settings surface', but EntryComposerProps.modes (src/chat-react/entry-composer.tsx:36-39) is documented as 'an on/off switch, not a value picker, so it sits apart from agent identity'. A named-profile picker IS a value picker, so placing it in the modes dock conflicts with the component's own seam contract. Impact: products following the doc will jam a value picker into an on/off switch slot, muddying the modes affordance. Fix: recommend 'a settings surface (or a picker beside the composer)' only, and drop the modes-dock suggestion.
🟡 LOW Demo model catalog silently defeats the harness->model coherence snap for 2 of 3 offered harnesses — playground/src/routes/WorkspaceRoute.tsx
The static models array (lines 38-57) contains only anthropic ids, but availableHarnesses offers 'codex' and 'opencode'. AgentSessionControls' onHarness uses snapModelToHarness (src/web-react/agent-session-controls.tsx:183-187), and harness/index.ts:133 documents that when no catalog id fits, snapModelToHarness returns the ORIGINAL model. So selecting Codex in the demo leaves anthropic/claude-opus-4-8 selected and the UI shows an incoherent harness+model pair with no indication, and the coherence behavior (the feature AgentSessionControls exists to demo) is unobservable for 2 of 3 harnesses. This is demo-data, not a code defect (the snap-returns-orig
🟡 LOW Initial effort value silently changed from 'auto' to 'medium' — playground/src/routes/WorkspaceRoute.tsx
The previous
useState<ReasoningLevel>('auto')becameuseState('medium'). Functionally equivalent for the canonicalEffortPicker(whoseDEFAULT_EFFORT_LEVELSare off/low/medium/high — 'auto' is not a level id and was already falling through to the index-2 'medium' fallback viaselected = levels.find(...) ?? levels[2], src/web-react/controls.tsx:402). No user-visible regression, but worth a glance if any downstream demo expected 'auto' on the wire. No fix required.
🟡 LOW Fragile textContent test assertions — src/chat-react/chat-react.test.tsx
Tests assert
document.body.textContent).toContain('GPT-4.1 Mini')and.toContain('Thinking')— broad substring matches that pass if the string appears anywhere on the page (e.g. in a different component rendered inside EntryComposer). The prior tests usedscreen.getByLabelTextandwithin(menu)scoping, which were more precise. Risk is low in this controlled fixture but makes future contributor changes fragile.
🟡 LOW No-agent-controls test checks harness picker only — src/chat-react/chat-react.test.tsx
The test asserts
document.querySelectorAll('[title="Agent backend"]')has length 0, which only proves theHarnessPickeris absent. IfAgentSessionControlsever renders withshowHarness={false}but a visibleModelPicker, this test would falsely pass. The intent is to verify the entire agent control row is absent — the simpler and more accurate check isexpect(container.querySelector('.flex.items-center.gap-1\.5') or screen.queryByText('GPT-4.1 Mini')).toBeNull()since the row renders as a flex div with gap.
🟡 LOW Test comment misattributes where harness↔model coherence is tested — src/chat-react/chat-react.test.tsx
The comment states 'picker behavior — catalog grouping, harness↔model coherence, effort levels — is tested where it is implemented (src/web-react/controls.test.tsx and src/harness)'. But controls.test.tsx (44 lines) only covers ModelPicker loading/empty/search states — it does not test coherence at all. The snap functions themselves are unit-tested in harness/index.test.ts, but the WIRING in AgentSessionControls.useCoherentHandlers (lines 173-190 of agent-session-controls.tsx: onHarness calls snapModelToHarness and fires onModelChange) has no component-level test. Impact: a regression in the coherence wiring (e.g., swapped args, missing onModelChange cal
🟡 LOW cli-base chat-surface filtering and harness-snap suppression lost their framework guard — src/chat-react/entry-composer.tsx
The old ComposerAgentControls automatically dropped cli-base from chat surfaces and suppressed the model snap on harness change (to preserve per-harness remembered picks). Both are now product-side responsibilities: a product must (a) exclude 'cli-base' from availableHarnesses on chat surfaces, and (b) store per-harness picks itself. docs/ui-picker-canon.md:72-78 documents this migration, but there is no remaining framework-level guard, lint, or test that catches a product passing cli-base on a chat surface or losing per-harness pick state. This is a deliberate design decision (domain-as-parameter), not a bug in this shot's code, but the protection layer that prevented the original defects (AGENTS.md cites 'cli-base reaching a chat composer' as defect #5) is gone. Low severity because it i
🟡 LOW Breaking removal of 9 published /chat-react exports without a version bump at head — src/chat-react/index.tsx
index.tsx removes ComposerAgentControls, ComposerAgentControlsProps, ComposerProfileSelection, ComposerModelSelection, ComposerHarnessSelection, ComposerEffortSelection and re-exported types AgentProfileCapability/AgentProfileDraft/AgentProfileOption/HarnessType/ModelInfo/ReasoningEffort/ReasoningLevel, and EntryComposerProps.agent changes from nested (ComposerAgentControlsProps) to flat (AgentSessionControlsProps) with effort/onEffortChange now required even for products with no reasoning concept. Package.json at head is still 0.45.28; AGENTS.md module-map doctrine says removal of a published export is a major. The commit's
!marker is the only guard that the release step bumps the major — every fleet product importing these symbols or passing the old agent shape breaks at typecheck unt
🟡 LOW Removed type re-exports without deprecation cycle — src/chat-react/index.tsx
Seven type re-exports removed from the barrel: ComposerAgentControlsProps, ComposerModelSelection, ComposerHarnessSelection, ComposerProfileSelection, ComposerEffortSelection (from deleted composer-agent-controls) plus AgentProfileCapability, AgentProfileDraft, AgentProfileOption, HarnessType, ModelInfo, ReasoningEffort, ReasoningLevel (from types.ts). All were available from their original packages, so consumers import them from there. No internal consumer of /chat-react uses these types. The
!commit signals breaking change, which is sufficient.
tangletools · 2026-08-06T09:40:38Z · trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 19 non-blocking findings — 0654d38f
Full multi-shot audit completed 8/8 planned shots over 17 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 17 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 17 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-08-06T09:40:38Z · immutable trace
Remove ComposerAgentControls + ComposerAgentControlsProps and the selection types that only existed to serve it (ComposerModelSelection, ComposerHarnessSelection, ComposerEffortSelection, ComposerProfileSelection), plus the legacy sandbox-ui type re-exports (HarnessType, ModelInfo, ReasoningLevel, ReasoningEffort, AgentProfile*) from /chat-react. EntryComposer keeps its controls seam: the `agent` prop is now the canonical AgentSessionControlsProps from /web-react and renders the canonical AgentSessionControls directly (its model menu IS the canonical ModelPicker). The playground workspace route and the default-workspace example migrate to the canonical flat props; named-profile picking is product-owned (not part of the canon cluster). BREAKING CHANGE: /chat-react no longer exports ComposerAgentControls, ComposerAgentControlsProps, ComposerProfileSelection, ComposerModelSelection, ComposerHarnessSelection, ComposerEffortSelection, HarnessType, ModelInfo, ReasoningLevel, ReasoningEffort, AgentProfileCapability, AgentProfileDraft, or AgentProfileOption. EntryComposer's `agent` prop takes AgentSessionControlsProps instead of ComposerAgentControlsProps. Migration mapping: docs/ui-picker-canon.md.
Review-driven sweep over the adapter-removal diff: - playground: span the demo catalog across providers so the harness→model coherence snap is observable for Codex — an anthropic-only catalog made the snap a silent no-op that kept an incoherent harness+model pair. - examples/default-workspace.md: state capability-hiding at the granularity the canon actually gives (omit `agent` for the row, `showHarness` for the backend control, reasoning-gated effort pill), and drop the modes-dock suggestion for profile pickers — a mode is an on/off switch, not a value picker, so that placement contradicted the component's own seam contract. - web-react: pin the AgentSessionControls coherence wiring with component tests (harness change snaps an incompatible model, model change snaps the harness, compatible change fires no model snap). The snap policy units already lived in /harness; the wiring between them had no guard. - chat-react: correct the test header's coverage pointers and give types.ts a module doc that describes what remains instead of the deleted adapter's vocabulary. - docs: regenerate (agent-docs).
0654d38 to
625ed7e
Compare
|
signoff PASS 625ed7e (rebased onto main with the template-pin fix) — all steps ok incl. generated projects, unit tests 2 seeds, knip. Clean tree, Node 22.23.1. |
What
Deletes the deprecated
ComposerAgentControlsadapter from/chat-reactoutright, now that the canonical pickers (ModelPicker/EffortPicker/AgentSessionControlsin@tangle-network/agent-app/web-react, #390) have merged and sandbox-ui is deleting its legacy pickers in a sibling PR.Removed:
src/chat-react/composer-agent-controls.tsx—ComposerAgentControls+ComposerAgentControlsPropsComposerModelSelection,ComposerHarnessSelection,ComposerEffortSelection,ComposerProfileSelection/chat-react(HarnessType,ModelInfo,ReasoningLevel,ReasoningEffort,AgentProfileCapability,AgentProfileDraft,AgentProfileOption) — the file's own comment said they were kept only so the adapter's props kept resolvingKept, retyped:
EntryComposergenuinely needs its controls seam, so theagentprop stays but is now the canonicalAgentSessionControlsPropsand renders/web-react'sAgentSessionControlsdirectly (no moremenuPlacement="down"override — the canonical menus open upward; no moreresponsivelayout — canonical layouts areinline/compact).Consumers migrated:
playground/src/routes/WorkspaceRoute.tsx→ canonical flat props (models/model/onModelChange,harness/onHarnessChange/availableHarnesses,effort/onEffortChange); named-profile picking dropped (not part of the canon cluster — product-owned)examples/default-workspace.mdcapability table + sample → canonical shapedocs/ui-picker-canon.md→ adapter marked removed (was "deprecated"); the props mapping stays as the migration path for products holding the old nested selection shapeAGENTS.mdpicker-canon section + default-workspace section updated to matchsrc/chat-react/chat-react.test.tsx→ adapter behavior tests deleted (that policy now lives and is tested in/web-react+/harness); the EntryComposer assembly contract tests (control row renders iffagentis supplied, attach/submit gating) kept and re-pointed at the canonical rowMigration for downstream products: props mapping in
docs/ui-picker-canon.md("Migration: sandbox-ui → agent-app canon"). The adapter's wiring products relied on — canonical-id boundary, harness-snap suppression for per-harness remembered picks, chat-context trim — becomes product-side state; the coherence grammar itself is identical in the canonical component.Note for reviewers:
docs/api/design-canvas-react*.md+docs/api/web-react.mdalso changed —pnpm docs:genpicks up pre-existing drift on main (resolveNodeCachePixelRatio/NODE_CACHE_PIXEL_RATIO_MAXare exported from main's source but missing from main's committed docs). Required for the codemap-freshness gate to pass; no source change involved.Gates
tsc --noEmit: 0 errorsvitest run src/chat-react src/web-react tests/codemap-fresh.test.ts: 28 files / 391 tests greenCI=true pnpm build-storybook: exit 0CI=true pnpm knip: exit 0pnpm docs:genrun;tests/codemap-fresh.test.tspassespnpm build(tsup): exit 0build/Releaseabsent) — pre-existing, unrelated to this change; no test imports the removed symbolsBREAKING CHANGE:
/chat-reactno longer exportsComposerAgentControls,ComposerAgentControlsProps,ComposerProfileSelection,ComposerModelSelection,ComposerHarnessSelection,ComposerEffortSelection,HarnessType,ModelInfo,ReasoningLevel,ReasoningEffort,AgentProfileCapability,AgentProfileDraft,AgentProfileOption.EntryComposer'sagentprop takesAgentSessionControlsPropsinstead ofComposerAgentControlsProps.