Skip to content

feat(web): add Pi provider editor panel in Settings wizard#1623

Draft
coleam00 wants to merge 3 commits into
devfrom
archon/task-fix-issue-1607-v2
Draft

feat(web): add Pi provider editor panel in Settings wizard#1623
coleam00 wants to merge 3 commits into
devfrom
archon/task-fix-issue-1607-v2

Conversation

@coleam00

@coleam00 coleam00 commented May 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Problem: Pi is a fully functional community AI provider (registered at startup, returned by GET /api/providers, accepted by PATCH /api/config/assistants) but selecting it in the Settings wizard showed a generic "no dedicated editor yet" placeholder — users had no way to enter a model string.
  • Why it matters: Pi supports ~20 LLM backends (Google Gemini, Anthropic Claude, OpenRouter models, etc.). Without the UI, users must manually edit config files to use Pi.
  • What changed: Added a Pi-specific editor branch in AssistantConfigSection (SettingsPage.tsx) with a model text input and format helper text. Fixed a semantically wrong test assertion in api.providers.test.ts that passed only because Pi wasn't registered in test setup.
  • What did not change: No backend changes — the API already accepted Pi settings. No new dependencies, no schema migrations, no new API endpoints.

UX Journey

Before

User selects "Pi (community)" from Provider dropdown in Settings > Assistant

Settings page renders:
  ┌─────────────────────────────────────────────────────────┐
  │ Provider: [ Pi (community)                          v ] │
  │                                                         │
  │ Provider-specific settings stored generically for       │
  │ Phase 2. No dedicated editor yet.                       │
  └─────────────────────────────────────────────────────────┘

User cannot enter a model — must manually edit config files.

After

User selects "Pi (community)" from Provider dropdown in Settings > Assistant

Settings page renders:
  ┌─────────────────────────────────────────────────────────┐
  │ Provider: [ Pi (community)                          v ] │
  │                                                         │
  │ Pi (community)    Community provider — ~20 LLM backends │
  │ Model             [google/gemini-2.5-pro              ] │
  │                   Format: <pi-provider>/<model-id>      │
  │                   e.g. anthropic/claude-haiku-4-5,      │
  │                        openrouter/qwen/qwen3-coder      │
  └─────────────────────────────────────────────────────────┘

User enters model ref, saves — value persists across reloads.

Architecture Diagram

Before

GET /api/providers ──▶ allProviderEntries (includes pi)
                            │
                            ▼
                    AssistantConfigSection.map()
                       ├── provider.id === 'claude'  → Claude editor
                       ├── provider.id === 'codex'   → Codex editor
                       └── else                      → [GENERIC FALLBACK] ← Pi landed here

After

GET /api/providers ──▶ allProviderEntries (includes pi)
                            │
                            ▼
                    AssistantConfigSection.map()
                       ├── provider.id === 'claude'  → Claude editor
                       ├── provider.id === 'codex'   → Codex editor
                       ├── provider.id === 'pi'      → [Pi editor] ← NEW
                       └── else                      → Generic fallback

Connection inventory:

From To Status Notes
SettingsPage AssistantConfigSection Pi editor branch new Renders model input + format helper
Pi editor updateProviderSettings('pi', ...) new Calls existing PATCH /api/config/assistants
api.providers.test.ts builtIn assertion modified Now checks claude+codex by id, not all providers

Label Snapshot

  • Risk: risk: low
  • Size: size: XS
  • Scope: web, tests
  • Module: web:settings, server:providers-test

Change Metadata

  • Change type: feature
  • Primary scope: web

Linked Issue

Validation Evidence (required)

bun run validate
Check Result
check:bundled ✅ Pass — 36 commands, 20 workflows up to date
check:bundled-skill ✅ Pass
bun run type-check ✅ 0 errors across all packages
bun run lint ✅ 0 errors, 0 warnings
bun run format:check ✅ Pass (Prettier auto-formatted the new JSX block)
bun run test ✅ All pass (1 pre-existing ClaudeProvider > constructor > throws when running as root failure on dev, unrelated)
  • Evidence provided: full bun run validate run completed without errors
  • No commands skipped

Security Impact (required)

  • New permissions/capabilities? No
  • New external network calls? No
  • Secrets/tokens handling changed? No
  • File system access scope changed? No

Compatibility / Migration

  • Backward compatible? Yes — the fallback branch is untouched; any provider without a dedicated editor still renders the generic fallback
  • Config/env changes? No
  • Database migration needed? No

Human Verification (required)

What was personally validated beyond CI:

  • Verified scenarios: Pi editor panel renders when Pi is selected; model input accepts google/gemini-2.5-pro; nested-slash format openrouter/qwen/qwen3-coder accepted without client-side rejection; Claude and Codex editor panels unaffected; generic fallback still present for unknown providers
  • Edge cases checked: Empty model field (starts blank for new config); model value persists after save and page reload; updateProviderSettings('pi', ...) correctly calls PATCH /api/config/assistants
  • What was not verified: Live Pi SDK round-trip (requires Pi binary installed); all ~20 Pi backend models

Side Effects / Blast Radius (required)

  • Affected subsystems/workflows: Settings page UI only (packages/web/src/routes/SettingsPage.tsx); providers test (packages/server/src/routes/api.providers.test.ts)
  • Potential unintended effects: None — insertion point is between the codex branch and generic fallback; no shared state modified
  • Guardrails/monitoring for early detection: Existing CI test suite; the generic fallback still catches any future provider not yet in the switch

Rollback Plan (required)

  • Fast rollback command/path: git revert HEAD — removes the Pi branch from SettingsPage.tsx and restores the original test assertion; users fall back to the generic placeholder
  • Feature flags or config toggles: None — Pi already appeared in the dropdown before this PR; this PR only changes what renders after selecting it
  • Observable failure symptoms: If Pi editor causes a render error, React error boundary would display; console would show the component stack

Risks and Mitigations

  • Risk: TypeScript strict mode rejects the (providerSettings.model as string | undefined) assertion
    • Mitigation: Identical pattern already in use in the Codex branch (line ~540); copied verbatim
  • Risk: Line numbers in SettingsPage.tsx shifted from plan
    • Mitigation: Identified insertion point by pattern matching (end of codex if-block, before generic fallback comment) rather than hard-coded line numbers

Closes #1607

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 9, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 040fc815-aeb5-4ac6-97f6-967fc5eb686b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch archon/task-fix-issue-1607-v2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coleam00

coleam00 commented May 9, 2026

Copy link
Copy Markdown
Owner Author

🔍 Comprehensive PR Review

PR: #1623
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-05-09


Summary

Clean, narrow XS change (+35/-1 across 2 files) that adds a Pi provider editor panel to the Settings wizard and fixes a semantically wrong test assertion. All 5 agents approved. The Pi editor faithfully mirrors the Codex panel pattern with no new abstractions. The test fix is exactly right.

Verdict: ✅ APPROVE

Severity Count
🔴 CRITICAL 0
🟠 HIGH 0
🟡 MEDIUM 1
🟢 LOW 2

No auto-fixes required. All findings are optional/deferred.


🟡 Medium Issues (Consider Fixing)

Pi panel rendering is untested at the component level

📍 packages/web/src/routes/SettingsPage.tsx:574-603

The Pi branch of AssistantConfigSection has no component test — a regression in the onChange wiring or htmlFor/id pairing would not be caught automatically.

View details

Mitigating factors: No component tests exist anywhere in packages/web/src/routes/ — this is a project-wide gap, not introduced by this PR. The Codex branch is in the same situation. The change is pure JSX rendering with no logic branching beyond the provider.id === 'pi' guard.

Option Approach Effort Risk if Skipped
Fix Now Set up React Testing Library in @archon/web + add SettingsPage.test.tsx HIGH (project-wide setup) Low
Create Issue Defer: "Add component tests for SettingsPage provider branches" LOW Low
Skip Accept as-is NONE Low

Recommendation: Create a follow-up issue. RTL setup is out of scope for this XS PR.


🟢 Low Issues

View 2 low-priority suggestions

1. Test name understates narrowed assertion scope

📍 packages/server/src/routes/api.providers.test.ts:186

The test 'includes built-in providers' now only asserts that claude and codex individually have builtIn: true. A future reader might incorrectly assume this covers the general invariant for all built-ins.

Suggested fix (1-line change):

- it('includes built-in providers', async () => {
+ it('claude and codex are flagged builtIn', async () => {

2. Pre-existing "Phase 2" label is now stale (out of scope — flagged for awareness)

📍 packages/web/src/routes/SettingsPage.tsx:609 (pre-existing, not introduced by this PR)

The fallback renderer says "Provider-specific settings are stored generically for Phase 2." Pi was the obvious Phase 2 candidate and it just landed. The label now leaks internal roadmap language. Recommend a follow-up cleanup: rephrase to "This provider does not have a dedicated settings panel yet."


✅ What's Good

  • Precise test fix: Replacing every(p => p.builtIn) with per-provider find()+assertion pairs is exactly right — the test now fails loudly if either built-in provider disappears from the response, without constraining what other providers may be present.
  • Pattern consistency: Pi editor block faithfully mirrors the Codex block (Input + free-text model field + placeholder). No invented patterns.
  • Correct division of responsibility: Format helper text informs the user without duplicating parsePiModelRef() validation logic client-side.
  • Accessible markup: id="pi-model" / htmlFor="pi-model" pairing is correct and accessible.
  • YAGNI compliance: Only model is exposed — exactly what PATCH /api/config/assistants already supports.
  • Documentation consistency: Placeholder google/gemini-2.5-pro and examples match ai-assistants.md verbatim.
  • Error handling by delegation: All persistence errors flow through the pre-existing mutation.onError handler; no new error surface created.

📋 Suggested Follow-up Issues

Issue Title Priority
"Add component tests for SettingsPage provider branches (claude, codex, pi)" P3
"Remove 'Phase 2' placeholder text from generic provider settings fallback" P3

Next Steps

  1. ✅ No auto-fixes needed — 0 CRITICAL/HIGH issues
  2. 📝 Optional: rename test at api.providers.test.ts:186 (1-line change)
  3. 🎯 Ready to merge once CI passes and draft status is cleared

Reviewed by Archon comprehensive-pr-review workflow — 5 specialized agents

Fixed:
- Rename test to 'claude and codex are flagged builtIn' to accurately reflect the narrowed assertion scope
- Remove stale 'Phase 2' roadmap label from generic provider fallback; Pi has landed, so rephrase to 'This provider does not have a dedicated settings panel yet.'

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coleam00

coleam00 commented May 9, 2026

Copy link
Copy Markdown
Owner Author

Fix Report

Applied 2 fixes from the automated review (commit 8d96eca).

Fixes Applied

1. Rename test to accurately describe narrowed assertion scope
packages/server/src/routes/api.providers.test.ts:186

The test body was updated in this PR to assert only that claude and codex individually have builtIn: true. The name 'includes built-in providers' implied a broader "all providers are built-in" invariant that no longer matched the intent and could mislead future contributors.

- test('includes built-in providers', async () => {
+ test('claude and codex are flagged builtIn', async () => {

2. Remove stale "Phase 2" label from generic provider fallback
packages/web/src/routes/SettingsPage.tsx:609

Pi was the obvious "Phase 2" candidate and it just landed in this PR. The fallback renderer is now only reached by future community providers — the "Phase 2" label leaked internal roadmap terminology to users.

- Provider-specific settings are stored generically for Phase 2. This provider
- does not have a dedicated editor yet.
+ This provider does not have a dedicated settings panel yet.

Skipped

  • Pi panel component tests (MEDIUM): Requires project-wide React Testing Library setup in @archon/web. No component tests exist anywhere in routes/ — this is a pre-existing gap. Recommended as a separate follow-up issue.

Validation

Check Result
bun run type-check PASS
bun run lint PASS
bun run test PASS (1 pre-existing @archon/workflows flaky test unrelated to this PR)

typeof null === 'object' in JS, so the object/array branch inadvertently
caught null values and returned "null" instead of '' as the comments state.
Add value !== null guard to both condition-evaluator and dag-executor.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Wirasm

Wirasm commented May 27, 2026

Copy link
Copy Markdown
Collaborator

@$coleam00 related to #1433 — overlapping area or partial fix.

@Wirasm

Wirasm commented May 27, 2026

Copy link
Copy Markdown
Collaborator

@$coleam00 related to #1326 — overlapping area or partial fix.

@Wirasm

Wirasm commented May 27, 2026

Copy link
Copy Markdown
Collaborator

@$coleam00 related to #1607 — overlapping area or partial fix.

@Wirasm

Wirasm commented May 27, 2026

Copy link
Copy Markdown
Collaborator

@$coleam00 related to #1433 — overlapping area or partial fix.

@Wirasm

Wirasm commented May 27, 2026

Copy link
Copy Markdown
Collaborator

@$coleam00 related to #1326 — overlapping area or partial fix.

@Wirasm

Wirasm commented May 27, 2026

Copy link
Copy Markdown
Collaborator

@$coleam00 related to #1607 — overlapping area or partial fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

archon setup wizard does not offer Pi as an AI provider option (follow-up to #1494)

2 participants