feat: Frontend — input picker in NewAgentPanel + preview dialog enrichment#2127
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe New Agent flow now fetches bundle previews, renders inline MCP and secret configuration fields, submits nonblank values during creation, and displays bundle details in preview dialogs. Shared template substitution, preview types, mock endpoints, unit tests, and Playwright coverage were added. ChangesBundle configuration flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant NewAgentPanel
participant ArchetypePreviewAPI
participant FleetAPI
Operator->>NewAgentPanel: select Product Manager archetype
NewAgentPanel->>ArchetypePreviewAPI: request archetype preview
ArchetypePreviewAPI-->>NewAgentPanel: return MCP inputs and secrets
Operator->>NewAgentPanel: enter configuration values
NewAgentPanel->>FleetAPI: create agent with inputs and secrets
FleetAPI-->>NewAgentPanel: return creation result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/e2e/fleet.spec.ts`:
- Around line 78-91: Update the test flow around the GitHub token and Brave API
key fields to fill the declared “Brave API key” secret with a non-empty value
before creating the agent. Change the posted payload assertion to verify that
secrets contains the expected Brave secret entry, while preserving the existing
github_token input assertion.
In `@apps/web/src/lib/archetypeConfig.ts`:
- Around line 85-90: Preserve raw submitted values in the field-routing loop of
archetypeConfig.ts: use value.trim() only to detect blank input, then pass the
untrimmed value to secrets or inputs. Update archetypeConfig.test.ts to expect
surrounding spaces to remain unchanged and add coverage confirming
whitespace-only values are omitted.
In `@apps/web/src/lib/mcpTemplate.ts`:
- Around line 9-10: Update the key lookup in the sub substitution function to
only use values defined as own properties of values, returning an empty string
for inherited or absent keys such as constructor. Add a regression test covering
prototype-inherited placeholders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: beb8437a-ca03-4e68-b5db-42730ca7fd52
📒 Files selected for processing (13)
apps/web/e2e/fixtures.mjsapps/web/e2e/fleet.spec.tsapps/web/e2e/mock-server.mjsapps/web/src/app/McpCatalogDialog.tsxapps/web/src/app/theme.cssapps/web/src/lib/api.tsapps/web/src/lib/archetypeConfig.test.tsapps/web/src/lib/archetypeConfig.tsapps/web/src/lib/mcpTemplate.test.tsapps/web/src/lib/mcpTemplate.tsapps/web/src/lib/types.tsapps/web/src/settings/NewAgentPanel.tsxapps/web/src/setup/ArchetypePreviewDialog.tsx
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head bbfd5abfe2a3 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
Overall risk: Low. This PR introduces a single new file (archetypeConfig.ts, 109 lines) with two utility functions. The sole confirmed finding is a minor naming-convention inconsistency between adjacent boolean predicates — no correctness, security, or behavioral defects surfaced.
Fix-first item: Rename configMissingRequired to hasConfigMissingRequired (or isConfigMissingRequired) to match the hasConfigFields predicate three lines above it. A one-line, zero-risk change.
Panel disagreement: None. Only one finding came through the structural pass, and the verifier confirmed it cleanly.
What verification changed: The verifier adjusted the line numbers (≈66/72 vs. the claimed 68/73) but confirmed the naming inconsistency is real. The PROTO.md rule itself was not independently fetched, but the inconsistency is a plain fact in the diff.
Coverage gap: A 109-line new file touching production code produced only a single minor finding from the structural pass — no LLM finders appear to have contributed. The remaining ~100 lines (including archetypeConfigFields, collectConfigDefaults, and their internal logic) went effectively unreviewed for correctness, edge cases, or type safety. Treat the silence as a coverage gap, not a clean bill of health.
[
{
"file": "apps/web/src/lib/archetypeConfig.ts",
"line": 73,
"severity": "minor",
"category": "conventions",
"claim": "Boolean predicate `configMissingRequired` lacks an `is`/`has` prefix, inconsistent with the adjacent `hasConfigFields` (line 68) in the same module, violating the PROTO.md rule to match surrounding naming within a file.",
"evidence": "Line 68: `export function hasConfigFields(...)` — uses `has` prefix. Line 73: `export function configMissingRequired(...)` — predicate with no prefix, three lines later. A reader scanning the exports cannot tell at a glance that both are boolean predicates.",
"verdict": "confirmed",
"note": "Verified in the diff: hasConfigFields (≈line 66) and configMissingRequired (≈line 72) are adjacent boolean predicates in the same new-file diff — the former uses `has`, the latter has no prefix. Exact line numbers differ by 1-2 from the claim but the naming inconsistency is real."
}
]
…cate prefix parity with hasConfigFields QA panel finding on #2127 (confirmed, adjudicated fix-before-merge): adjacent boolean predicates in the new archetypeConfig module should share the is/has prefix convention. Zero-reference rename (both symbols new in this PR). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hh8gD6749kPXoBK9Wsd1FA
|
WARN adjudication (protoEngineer's brain): fix-before-merge — zero-reference rename, cheaper now than as debt. Applied: |
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 9c622219dc44 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
This PR introduces a new archetypeConfig.ts module for MCP config field handling — additive, single-file, low overall risk. The one confirmed finding (minor correctness) is that fieldId drops the server discriminator from its Pick type, so two MCP servers in a bundle with same-named input keys (e.g., both declare "token") collide in the form values map — the operator can't supply distinct values per server. Fix fieldId first by including server in both the Pick type parameter and the template string. No panel disagreements (single-finder report). Verification was clean and confirmed the defect against source. Gap noted by the verifier: even with fieldId fixed, splitConfigValues uses a flat inputs[f.key] map that would still overwrite same-key values from different servers — the true root cause may span both functions.
[
{
"file": "apps/web/src/lib/archetypeConfig.ts",
"line": 30,
"severity": "minor",
"category": "correctness",
"claim": "fieldId uses only origin+key for disambiguation but omits server, so two MCP servers in the same bundle that share an input key name (e.g., both have a 'token' input) collide in the values map, making it impossible for the operator to set different values for different servers.",
"evidence": "Line 29-31: `export function fieldId(f: Pick<ConfigField, \"origin\" | \"key\">): string { return `${f.origin}:${f.key}`; }` — server is excluded from the Pick type and unused. But ConfigField.server is populated (line 47: `server: item.name`) and two servers in a bundle could reasonably have an input with the same key. The test suite covers input-vs-secret collisions (archetypeConfig.test.ts line 127) but not MCP-vs-MCP collisions.",
"source": "correctness-finder",
"verdict": "confirmed",
"note": "Code matches: fieldId Pick type excludes server, ConfigField.server is defined & populated, test only covers input-vs-secret collision (origin prefix), no MCP-vs-MCP collision test exists. Also note: splitConfigValues uses flat inputs[f.key] map, so even with fieldId fixed, two servers with same key would still overwrite in the output — root cause is partly in the flat inputs design."
}
]
|
WARN adjudication, round 2 (protoEngineer's brain): merge-then-followup — the collision finding is real but zero real-world bundles trigger it, and the true fix is an end-to-end design change spanning the flat |
…rtifacts, fleet inputs/secrets (#2130) Bump 0.106.0 → 0.107.0 and roll the changelog for the release. Highlights: - Versioned file artifacts + save_file_artifact (ADR 0092 D2, #2126) - Document-generation stack baked into the desktop bundle (ADR 0092 D1, #2123) - Fleet agents accept operator inputs + secrets at create time (#2121/#2122/#2125/#2127) - Deterministic persona-drift detection (#2116) - Chat tab context menu Close Others/Left/Right (#2112) - Fixes: archetype-catalog bundling (#2115), PyPI publishes on tag push (#2113) Backfilled [Unreleased] entries for the feature PRs that lacked them, rolled to [0.107.0], scaffolded + polished the marketing changelog. uv.lock: project version line only (surgical — avoids uv 0.11.13-vs-pinned-0.11.29 marker drift). Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Ships the final UI slice of operator-supplied bundle setup (#2041): the archetype preview and the new-agent picker now surface and collect the MCP inputs + secrets a bundle archetype will ask for.
fillTemplateextracted fromMcpCatalogDialog.tsxintoapps/web/src/lib/mcpTemplate.tsas a shared utility; the dialog now imports it.ArchetypePreviewDialogreads the enrichedmcp+secretsoff the preview peek and renders them read-only below the members/skills section —MCP servers: GitHub (needs token), …andSecrets: GitHub token, …. Because the dialog is shared, the setup wizard gets the same display for free.NewAgentPanelgains an inline, collapsible Configure step between archetype pick and name. When the picked bundle has MCP inputs or declared secrets, a form appears (non-secret →Input, secret → maskedSecretInput, required marked*). Filled values are split intoinputs(MCP${…}map) andsecrets([{key,value}]) and passed toapi.createAgent()alongside name/bundle/soul; blanks are dropped so the backend's env-only fallback still applies. Skipping (collapse) or a no-inputs archetype sends nothing — backward compatible. Creation is never hard-blocked; unfilled required fields show a soft hint.archetypeConfig.ts) carry the logic and the unit coverage; newmcpTemplate+archetypeConfigvitest suites and two Playwright specs (Configure form collects the GitHub token into the create payload; preview dialog lists the servers/secrets) exercise the behavior, with the mock server + fixtures extended to serve the enriched preview.New/changed files:
lib/mcpTemplate.ts,lib/archetypeConfig.ts(+ tests),lib/types.ts,lib/api.ts,settings/NewAgentPanel.tsx,setup/ArchetypePreviewDialog.tsx,app/McpCatalogDialog.tsx,app/theme.css, and e2e fixtures/mock/spec.Refs #2041 (slice 3 of 3)
Summary by CodeRabbit
New Features
Bug Fixes