feat(web): add Pi provider editor panel in Settings wizard#1623
feat(web): add Pi provider editor panel in Settings wizard#1623coleam00 wants to merge 3 commits into
Conversation
Closes #1607 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
🔍 Comprehensive PR ReviewPR: #1623 SummaryClean, 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: ✅
No auto-fixes required. All findings are optional/deferred. 🟡 Medium Issues (Consider Fixing)Pi panel rendering is untested at the component level📍 The Pi branch of View detailsMitigating factors: No component tests exist anywhere in
Recommendation: Create a follow-up issue. RTL setup is out of scope for this XS PR. 🟢 Low IssuesView 2 low-priority suggestions1. Test name understates narrowed assertion scope📍 The test 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)📍 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
📋 Suggested Follow-up Issues
Next Steps
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>
Fix ReportApplied 2 fixes from the automated review (commit 8d96eca). Fixes Applied1. Rename test to accurately describe narrowed assertion scope The test body was updated in this PR to assert only that - test('includes built-in providers', async () => {
+ test('claude and codex are flagged builtIn', async () => {2. Remove stale "Phase 2" label from generic provider fallback 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
Validation
|
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>
|
@$coleam00 related to #1433 — overlapping area or partial fix. |
|
@$coleam00 related to #1326 — overlapping area or partial fix. |
|
@$coleam00 related to #1607 — overlapping area or partial fix. |
|
@$coleam00 related to #1433 — overlapping area or partial fix. |
|
@$coleam00 related to #1326 — overlapping area or partial fix. |
|
@$coleam00 related to #1607 — overlapping area or partial fix. |
Summary
GET /api/providers, accepted byPATCH /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.AssistantConfigSection(SettingsPage.tsx) with a model text input and format helper text. Fixed a semantically wrong test assertion inapi.providers.test.tsthat passed only because Pi wasn't registered in test setup.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
updateProviderSettings('pi', ...)Label Snapshot
risk: lowsize: XSweb,testsweb:settings,server:providers-testChange Metadata
featurewebLinked Issue
Validation Evidence (required)
check:bundledcheck:bundled-skillbun run type-checkbun run lintbun run format:checkbun run testClaudeProvider > constructor > throws when running as rootfailure ondev, unrelated)bun run validaterun completed without errorsSecurity Impact (required)
Compatibility / Migration
Human Verification (required)
What was personally validated beyond CI:
google/gemini-2.5-pro; nested-slash formatopenrouter/qwen/qwen3-coderaccepted without client-side rejection; Claude and Codex editor panels unaffected; generic fallback still present for unknown providersupdateProviderSettings('pi', ...)correctly callsPATCH /api/config/assistantsSide Effects / Blast Radius (required)
packages/web/src/routes/SettingsPage.tsx); providers test (packages/server/src/routes/api.providers.test.ts)Rollback Plan (required)
git revert HEAD— removes the Pi branch from SettingsPage.tsx and restores the original test assertion; users fall back to the generic placeholderRisks and Mitigations
(providerSettings.model as string | undefined)assertionend of codex if-block, before generic fallback comment) rather than hard-coded line numbers