feat: unify agents & BrowserOS AI (one picker, merged settings)#1065
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 5m 10s —— View job Code ReviewTasks
OverviewThis is a well-executed refactor that delivers on its goals: the dead-end "no agents" empty state is gone, agent management moves into settings, and adapter visibility is now a single-source-of-truth. The new utility modules are small, well-named, and properly tested. Overall the PR is in good shape — the findings below are mostly minor. Findings1. Dead ternary branches in
|
✅ Tests passed — 1056/1061
|
Greptile SummaryThis PR unifies the standalone Agents page and BrowserOS AI settings into a single "AI & Agents" master-detail settings pane, and replaces the home composer's agent-only picker with a merged LLM provider + agent picker backed by the sidepanel's existing
Confidence Score: 3/5Safe to merge for navigation and UI changes; agent creation in the new settings pane can produce incorrect data when the user switches between adapter sections without completing a create flow. The home composer refactor and redirect wiring are clean and well-tested. The main concern is in AISettingsPage: AdapterAgentsPane shares a React instance across adapter sections (no key prop), so form state from a prior adapter can bleed into a new agent creation on a different adapter — a user who manually changed a model string and then switched panes would silently submit the wrong model to the backend. packages/browseros-agent/apps/agent/entrypoints/app/ai-settings/AISettingsPage.tsx — the AdapterAgentsPane rendering needs a key prop to prevent stale state across adapter switches. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Home["/home (AgentCommandHome)"] --> Picker["Unified Provider Picker\n(LLM providers + Agents)"]
Picker -->|"LLM selected"| PersistTarget["persistSidepanelChatTargetSelection\nsetDefaultProvider"]
PersistTarget --> HomeChat["/home/chat"]
Picker -->|"Agent selected"| PendingMsg["setPendingInitialMessage"]
PendingMsg --> AgentConvo["/home/agents/:id"]
LegacyAgents["/agents"] -->|redirect| AISettings["/settings/ai?section=claude"]
LegacyAgentId["/agents/:agentId"] -->|redirect| AgentConvo
AISettings --> MasterDetail["AI & Agents Master-Detail"]
MasterDetail -->|"section=browseros"| BrowserOsPane["BrowserOsAiPane\n(LLM providers)"]
MasterDetail -->|"section=claude"| ClaudePane["AdapterAgentsPane(claude)"]
MasterDetail -->|"section=codex"| CodexPane["AdapterAgentsPane(codex)"]
MasterDetail -.->|"hidden"| Hermes["Hermes (filtered by adapter-visibility.ts)"]
style Hermes stroke-dasharray: 5 5
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/apps/agent/entrypoints/app/agent-command/AgentCommandHome.tsx:33
**Hardcoded `?section=claude` may not link to the right pane**
`MANAGE_AGENTS_PATH` always points at the Claude adapter pane. If the user has no Claude adapter (or if Claude is not listed as a visible adapter), `resolveAiSettingsSection` will fall back to the BrowserOS AI pane — hiding all agents rather than surfacing the active adapter's management UI. Consider deriving the first visible adapter's ID at runtime (e.g., from `adapters`) rather than hardcoding `'claude'`.
### Issue 2 of 2
packages/browseros-agent/apps/agent/entrypoints/app/ai-settings/AISettingsPage.tsx:86-90
**Stale form fields after switching adapter panes**
`AdapterAgentsPane` lacks a reconciliation identity prop, so React reuses the same instance when the user navigates between adapters. The `modelId` and `reasoningEffort` states carry over, and the seeding effect — which guards with `(current) => current || default` — will not overwrite truthy leftover values. A user who customised a model in one adapter pane and then switched would silently create an agent in the second pane with an incompatible model string. Adding a `key` prop set to `activeSection` forces a fresh mount on every section transition.
Reviews (1): Last reviewed commit: "chore: self-review fixes — persist chat-..." | Re-trigger Greptile |
Greptile SummaryThis PR unifies BrowserOS AI and agent management into a single settings page (
Confidence Score: 3/5Safe to merge after fixing the adapter state leakage — creating an agent from the wrong adapter pane would silently use a stale model ID from a previously visited adapter. The master-detail settings shell has no packages/browseros-agent/apps/agent/entrypoints/app/ai-settings/AISettingsPage.tsx — missing Important Files Changed
|
|
Thanks Greptile (@greptileai) / Claude (@claude) — addressed in 0fb0577. Accepted
Declined
Gates: |
|
Claude finished Nikhil (@shadowfax92)'s task in 1m 15s —— View job Claude finished reviewing Nikhil (@shadowfax92)'s follow-up fixes — Tasks
Verification ResultsAll four accepted fixes are correctly implemented. [P1] Stale adapter state —
ACP misroute guard — Dynamic One Open Item (not in scope for this PR)The Looks good to merge — the P1 state-leak and all other accepted findings are correctly resolved. |
|
All four fixes verified in
The declined LGTM — good to merge. |
* fix: version-gate Claude Code/Codex agents to BrowserOS 0.46.0.0 The unified picker (#1065) surfaced the agent-harness adapters (Claude Code, Codex) on every BrowserOS version the auto-updating extension runs on, but the harness/ACP backend only ships in BrowserOS 0.46.0.0+. On older builds the agents appeared but could not run. Add Feature.AGENT_HARNESS_SUPPORT (minBrowserOSVersion 0.46.0.0) and gate the two data hooks that feed every surface: useAgentAdapters (AI & Agents settings tabs) and useHarnessAgents (picker ACP targets). When unsupported, both disable their query and return empty — so the home and sidepanel pickers, settings tabs, and recent-agents dock all hide the agents, with no requests to harness endpoints that may not exist. * fix: address review — guard error field and loading through capabilities init - loading now folds in useCapabilities().isLoading so consumers don't see a premature 'no agents' empty state during the capabilities-init window on supported BrowserOS (Greptile P2, both hooks). - error is now gated by agentsSupported, so urlError from useAgentServerUrl can't leak an error banner on older builds where harness endpoints are absent — older builds see nothing, per intent (Claude review). - Note why checkFeatureSupport is exported (tests only).
Summary
?section=-driven page: BrowserOS AI (the LLM-providers UI) plus one pane per visible adapter (Claude/Codex) that owns instance create/delete. Hermes is hidden in the UI (kept in the backend/types)./agentsroute, its page, and the "Agents" sidebar item are gone; agent management lives in settings, conversations under/home/agents. Old links redirect (/agents→/settings/ai?section=claude,/agents/:id→/home/agents/:id).Design
Lift the sidepanel's proven pattern (
Provider { kind: 'llm' | 'acp' }, kind-based routing) up to the home composer and reorganize settings — no new runtime. LLM picks route to the existing/home/chat; agent picks to/home/agents/:id. A singlelib/chat/adapter-visibilityhelper hides Hermes at thebuildSidepanelChatTargetschokepoint, so every picker/list inherits it. The home→/home/chathandoff persists the shared chat-target selection (the value that surface actually resolves from, preferred over the global default).entrypoints/app/agents/*shared libs (data hooks,NewAgentDialog,AgentList) are reused by the new settings panes; only the page-level files were deleted.Test plan
bun run typecheck— passesbun test apps/agent— 81 unit tests pass (new: adapter-visibility, ai-settings-sections, home-compose routing)/settings/ailists BrowserOS AI / Claude / Codex (no Hermes);?section=claudedeep-links; create + delete an instance from a pane/homewith zero agents shows the composer with BrowserOS selected (no empty-state card)/home+ send → lands in/home/chaton that provider; pick an agent → lands in/home/agents/:id/agentsand/agents/:idredirect correctly; no "Agents" sidebar item