Clean multi provider feature#473
Conversation
- Remove unused dropdown components (ImprovedModelDropdown, ImprovedProviderDropdown) - Consolidate AddProviderModal in agents folder - Fix provider router imports in main.py - Remove misplaced routes.generated.ts from Python directory - Keep PROVIDER_INTEGRATION_CLEAN.md in providers_clean directory
The apiRequest function already adds /api prefix, so the service should only specify the path after /api
- Enable RLS on all provider tables (model_config, api_keys, model_usage) - Create explicit policies for service_role only access - Add SECURITY DEFINER to functions with explicit search_path - Revoke all access from anon and authenticated roles - Add comprehensive security documentation - Ensure only service key can access provider configuration
- Remove Usage Summary section showing monthly cost, requests, and tokens - Remove related imports (DollarSign, BarChart3, Button, ChevronUp) - Remove monthlyUsage state variable and usage data loading - Simplify page to focus on agent configuration only
|
Warning Rate limit exceeded@zaksnet has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a provider- and agent-centric system across frontend and backend: new Service Registry, Agents management UI (pages, components, model selection, provider configuration), TanStack Query hooks and optimistic mutations, a clean provider backend API with Supabase repositories/DI, an SQL migration for provider/embedding schemas, and related UI/refactor changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant FE as Frontend (App)
participant SR as ServiceRegistryProvider
participant AGP as AgentsPage
participant CPS as cleanProviderService
participant BE as Providers API (FastAPI)
U->>FE: Navigate to /agents
FE->>SR: Initialize & refresh services
SR->>BE: GET /api/providers/services/registry
BE-->>SR: List of ServiceInfo
SR-->>FE: agents/backendServices
FE->>AGP: Render AgentsPage
AGP->>CPS: GET available models & configs (via agentApi)
CPS-->>AGP: models + configs
AGP-->>U: Show agents and model selection
U->>AGP: Add API key
AGP->>BE: POST /api/providers/api-keys
BE-->>AGP: success
AGP->>BE: POST /api/providers/models/sync (background)
AGP-->>U: Updated providers/models
sequenceDiagram
autonumber
participant FE as Frontend (Chat)
participant AG as Agents Server
participant PI as Provider Integration
participant TR as Usage Tracker
FE->>AG: createSession + send/stream message
AG->>PI: invoke model (if provider integration)
PI-->>AG: model response (+usage)
AG->>TR: track usage
AG-->>FE: stream response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
Increased wait time from 0.1s to 0.15s to account for overhead when running 4 tasks with concurrency limit of 2 and 0.05s per task
- Add ProviderIntegration support to agent server - Automatic fallback to legacy credential system if integration unavailable - Agents now use models configured via the UI provider settings - Added usage tracking when provider integration is active - New /agents/model-config endpoint to view current model configuration - Backward compatible - works with or without provider integration
- Created MainServerProviderIntegration for main server services - Updated llm_provider_service to use provider integration when available - Main server now initializes provider integration on startup - Embedding service now uses models configured via UI - LLM services use models configured via UI - Automatic fallback to legacy credential service - All services maintain backward compatibility
- Updated contextual_embedding_service to use get_llm_model from provider integration - Updated source_management_service to use async functions with provider integration - Updated code_storage_service to use provider integration for LLM operations - Fixed all callers to use await for newly async functions - Removed direct OpenAI client instantiation from all services - All LLM/embedding operations now go through the provider integration layer
- Move all LLM client operations inside the async with block - Ensures client is available when making API calls - Fixes IndentationError on server startup
- Created status bar that displays current models for all services - Added /api/providers/active-models endpoint to fetch real-time configuration - Shows provider colors, API key status, and default configurations - Auto-refreshes every 30 seconds - Always visible at top of application
- Reduced padding and font sizes for compact design - Removed provider legend to save space - Adjusted main content area padding to account for bar height - Made all states (loading, error, active) consistent height - Bar is now approximately 32-40px tall instead of 80-100px
|
Up this this [Fix timing issue in test_concurrency_limit test](def85d6), there was no integration and no breaking changes. The commits after that are the important ones and the ones you would probably want to review. |
- Display total tokens used today with smart formatting (K/M) - Show today's cost and estimated monthly cost - Added Hash and Coins icons for visual clarity - Integrated usage tracking from UsageTracker service - Format currency with appropriate precision - Updates automatically with 30-second refresh
- Delete agent_server_integration.py and main_server_integration.py - These integration modules are being replaced with a simpler provider management system - Removes unnecessary complexity and singleton patterns
- Add provider_manager.py: Simplified provider management without singletons - Add embedding_models.py: Centralized embedding model configurations - Add 002_add_daily_costs_function.sql: Database migration for cost tracking - Provides cleaner API for provider and model management across services
- Remove dependency on provider integration layer in llm_provider_service.py - Simplify provider client creation with direct credential service calls - Add enhanced provider support methods in credential_service.py - Eliminate unnecessary abstraction layers for cleaner code flow
- Extract _process_embeddings_with_client helper for code reuse - Add ProviderManager import for future integration - Simplify embedding batch processing logic - Clean up __init__.py exports
- Update source_management_service.py to use simplified provider calls - Modify storage services (code & document) for new provider integration - Update crawling services to work with refactored provider system - Remove references to old integration layer - Improve error handling and logging
- Simplify provider_routes.py API endpoints - Update model_config.py with new provider configurations - Update openrouter_models.json with latest model listings - Remove integration layer dependencies from provider API
- Remove integration layer initialization from main.py - Update knowledge_api.py to use new provider system - Clean up utils/__init__.py removing unused imports - Simplify server startup and dependency management
- Refactor agent.ts: Replace llm_primary/secondary with specific service agents - Add new service types: task_agent, contextual_embedding, source_summary, code_summary - Update ModelStatusBar.tsx and AgentCard.tsx for new agent types - Update cleanProvider types and service for streamlined provider management - Improve UI consistency with new provider architecture
|
This is almost ready for review @coleam00 @Wirasm. I just want a better solution for the embeddings, some polishing and a few bug fixes. You can test it to get a quick feeling of it, tell me if its something you interested or not and then maybe @Wirasm or me can work on updating the UI to fit the rest of the project and move files from the feature folder into their respective folders |
|
Looking really good @zaksnet! |
- Updated docker-compose.yml to include profiles for backend and frontend services. - Added .vscode/settings.json for spell checking configuration. - Updated package-lock.json to reflect dependency version upgrades. - Refactored ModelStatusBar and ModelSelectionModal components for improved model handling. - Enhanced CleanProviderService for better service status retrieval and error handling. - Introduced new repository and unit of work patterns for cleaner data access in the provider system. - Refactored services to utilize the new repository pattern, improving maintainability and testability.
- Added _load_cache_ignore_age method to load cached models regardless of age. - Improved error handling in fetch logic to fallback on stale cache if API call fails. - Updated openrouter_models.json with new model "moonshotai/kimi-k2-0905" and adjusted existing models. - Refactored ModelConfigService to remove default models and improve error handling. - Introduced ServiceRegistryService for managing service registrations and usage tracking. - Enhanced ModelSyncService with better logging and error handling during model synchronization. - Added comprehensive validation and discovery methods in ServiceRegistryService.
…ng a new record in archon_settings
…nt and enhance query invalidation in useAgentMutations
…ttings and ProviderSettings components - Standardized import statements and formatting in CodeExtractionSettings.tsx - Enhanced readability by restructuring JSX elements and improving indentation - Updated event handlers to use arrow functions consistently - Refactored ProviderSettings.tsx for better code clarity and consistency in function definitions - Ensured consistent use of parentheses in arrow functions across both files
…date related methods
…and normalize document handling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
archon-ui-main/src/components/DisconnectScreenOverlay.tsx (1)
23-23: Fix memory leak with setTimeout cleanup.The
setTimeoutinonMouseLeavecreates a timer that won't be cleaned up if the component unmounts or if the mouse re-enters before the timeout completes. This can lead to memory leaks and unexpected state updates.- onMouseLeave={() => setTimeout(() => setShowControls(false), 3000)} + onMouseLeave={() => { + const timeoutId = setTimeout(() => setShowControls(false), 3000); + return () => clearTimeout(timeoutId); + }}Actually, for proper cleanup, you'll need to use
useEffectanduseRef:import React, { useState, useRef, useEffect } from "react"; // Inside the component: const timeoutRef = useRef<NodeJS.Timeout>(); const handleMouseLeave = () => { if (timeoutRef.current) { clearTimeout(timeoutRef.current); } timeoutRef.current = setTimeout(() => setShowControls(false), 3000); }; const handleMouseEnter = () => { if (timeoutRef.current) { clearTimeout(timeoutRef.current); } setShowControls(true); }; useEffect(() => { return () => { if (timeoutRef.current) { clearTimeout(timeoutRef.current); } }; }, []); // Then use: onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave}archon-ui-main/src/features/projects/components/ProjectCard.tsx (1)
31-55: Make the clickable card keyboard-accessible and announce selection state.Add tab focus, Enter/Space activation, and an explicit selected state. Also add a visible focus ring for non-mouse users.
- <motion.div - role="listitem" - onClick={() => onSelect(project)} - className={cn( + <motion.div + role="listitem" + tabIndex={0} + aria-selected={isSelected} + aria-label={`Open project ${project.title}`} + onKeyDown={(e) => { + if (e.currentTarget !== e.target) return; + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + onSelect(project); + } + }} + onClick={() => onSelect(project)} + className={cn( "relative rounded-xl backdrop-blur-md w-72 min-h-[180px] cursor-pointer overflow-visible group flex flex-col", - "transition-all duration-300", + "transition-all duration-300 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-purple-400/50",archon-ui-main/src/components/knowledge-base/GroupedKnowledgeItemCard.tsx (1)
177-185: Guard against empty groups to avoid runtime crashes.activeItem is accessed unconditionally; groupedItem.items can be empty, leading to undefined.metadata errors.
Apply:
export const GroupedKnowledgeItemCard = ({ groupedItem, onDelete, onUpdate, onRefresh, }: GroupedKnowledgeItemCardProps) => { + if (!groupedItem?.items || groupedItem.items.length === 0) { + // Optionally render a placeholder/skeleton here + return null; + } const [showDeleteConfirm, setShowDeleteConfirm] = useState(false);Also applies to: 191-193
archon-ui-main/src/components/settings/ButtonPlayground.tsx (1)
78-88: Type error: CornerRadius initialization with empty object.
const newRadius: CornerRadius = {} violates required fields; TypeScript will error. Also this logic is more complex than necessary.Simplify linked updates without constructing partial objects:
- // Update all linked corners - const newRadius: CornerRadius = {}; - Object.keys(linked).forEach((key) => { - if (linked[key as keyof CornerRadius]) { - newRadius[key as keyof CornerRadius] = value; - } else { - newRadius[key as keyof CornerRadius] = - layer1Radius[key as keyof CornerRadius]; - } - }); - setRadius(newRadius); + // Update all linked corners + setRadius(() => ({ + topLeft: value, + topRight: value, + bottomRight: value, + bottomLeft: value, + }));And for layer2:
- const newRadius: CornerRadius = {}; - Object.keys(linked).forEach((key) => { - if (linked[key as keyof CornerRadius]) { - newRadius[key as keyof CornerRadius] = value; - } else { - newRadius[key as keyof CornerRadius] = - layer2Radius[key as keyof CornerRadius]; - } - }); - setRadius(newRadius); + setRadius(() => ({ + topLeft: value, + topRight: value, + bottomRight: value, + bottomLeft: value, + }));Also applies to: 94-104
archon-ui-main/src/components/agent-chat/ArchonChatPanel.tsx (1)
147-181: Reset document body styles on cleanup/unmount during drag.If the component unmounts mid-drag,
cursoranduserSelectremain altered.Apply:
// Clean up event listeners return () => { document.removeEventListener("mousemove", handleMouseMove); document.removeEventListener("mouseup", handleMouseUp); + document.body.style.cursor = "default"; + document.body.style.userSelect = "auto"; };archon-ui-main/src/components/settings/CodeExtractionSettings.tsx (1)
24-25: Strongly type setCodeExtractionSettings (avoid any).Use a proper SetStateAction type to prevent shape drift and catch mistakes at compile time.
- setCodeExtractionSettings: (settings: any) => void; + setCodeExtractionSettings: ( + settings: React.SetStateAction<CodeExtractionSettingsProps["codeExtractionSettings"]> + ) => void;
♻️ Duplicate comments (18)
archon-ui-main/src/components/knowledge-base/GroupedKnowledgeItemCard.tsx (1)
2-14: Lucide Box alias fix looks good (resolves prior import error).Importing Box as BoxIcon is the correct fix for lucide-react. No further action needed.
archon-ui-main/src/components/knowledge-base/KnowledgeTable.tsx (1)
74-101: Resolved: guarded timestamp parsing prevents toISOString crash.Nice fix adding Date.parse + fallback; this addresses the earlier crash risk.
python/src/providers_clean/api/routes/get_services_registry.py (1)
13-17: Category validation via Enum looks good.Enum + Query description resolves earlier input-validation feedback.
python/src/agents/server.py (2)
128-178: Runtime mode + safe fallback: nice recovery path.Setting app.state.runtime_mode and clearing provider_integration on failure addresses prior misreporting/fallback issues.
314-334: Stream endpoint: pass deps and ensure JSON-safe payload.Matches run_agent behavior and prevents non-serializable objects from breaking the stream.
@app.get("/agents/stream/{agent_type}") async def stream_agent(agent_type: str, prompt: str): @@ async def generate() -> AsyncGenerator[str, None]: - # For now, just run the agent normally and yield the result - # In the future, we can implement true streaming - agent = app.state.agents[agent_type] - result = await agent.run(prompt, {}) - yield json.dumps({"result": result}) + # For now, just run the agent normally and yield the result + agent = app.state.agents[agent_type] + deps = { + "context": {}, + "options": {}, + "mcp_endpoint": os.getenv("MCP_SERVICE_URL", "http://archon-mcp:8051"), + } + result = await agent.run(prompt, deps) + safe = result if isinstance(result, (str, int, float, bool, dict, list, type(None))) else str(result) + yield json.dumps({"result": safe})archon-ui-main/src/features/agents/components/common/hooks/useOptimisticUpdate.ts (1)
31-35: Remove or underscore unused parameter.
initialState is not used; either drop it (breaking change) or underscore it to avoid lint noise.-export const useOptimisticUpdate = <T>( - initialState: T, +export const useOptimisticUpdate = <T>( + _unusedInitialState: T, setState: (state: T) => void, options: OptimisticUpdateOptions<T> = {} ): OptimisticUpdateResult<T> => {archon-ui-main/src/features/agents/utils/optimisticUpdates.ts (1)
99-107: Nice fix for animatingItems cleanup.
Timeout-based deletion prevents unbounded Set growth after repeated operations.Also applies to: 116-124
archon-ui-main/src/contexts/ServiceRegistryContext.tsx (1)
116-127: Good fix: legacy agent configs now scoped to agents only.This addresses prior reports where all services were returned.
archon-ui-main/src/features/agents/components/common/ui-primitives/GradientCard.tsx (1)
48-60: Make clickable card accessible (keyboard + focus).Add Enter/Space activation, tabbability, role fallback to “button”, and aria-pressed when interactive. Also allow “button” in valid roles.
const validAriaRoles = [ - "region", + "region", "article", "section", "main", "complementary", "navigation", "banner", "contentinfo", + "button", ]; - const effectiveRole = - role && validAriaRoles.includes(role) ? role : undefined; + const effectiveRole = + role && validAriaRoles.includes(role) ? role : onClick ? "button" : undefined; return ( <div className={cn(getCardClasses(isActive, isHoverable, size), className)} style={getCardStyle(effectiveTheme)} onClick={onClick} role={effectiveRole} aria-labelledby={ariaLabelledby} aria-describedby={ariaDescribedby} + aria-pressed={onClick ? isActive : undefined} + tabIndex={onClick ? 0 : undefined} + onKeyDown={(e) => { + if (!onClick) return; + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + onClick(); + } + }} >Also applies to: 61-69
archon-ui-main/src/features/agents/components/agent-management/AgentCard.tsx (2)
71-72: Good fix: timer ref typed for browser compatibilityUsing ReturnType avoids NodeJS.Timeout issues in the browser.
186-189: Good: isActive coerced to strict booleanPrevents object | boolean unions and matches state naming guidelines.
archon-ui-main/src/features/agents/services/agentService.ts (1)
6-8: Good: imports moved to top and types tightened.This addresses the earlier feedback: using ProviderType/ServiceType and importing cleanProviderService before use.
archon-ui-main/src/services/credentialsService.ts (3)
225-238: Good: setCredential preserves types via JSON body.This aligns with the typed JSON guidance and fixes earlier stringification issues.
94-148: Resilient coercion helpers applied correctly.Adoption of toBool/toInt/toFloat fixes prior coercion pitfalls (booleans and zero values).
Also applies to: 279-351
241-276: Send typed JSON bodies; stop stringifying into query params.Posting values via URLSearchParams coerces booleans/numbers to strings and conflicts with setCredential’s JSON body. Unify on JSON: { value }.
Apply:
async updateRagSettings(settings: Partial<RagSettings>): Promise<boolean> { try { - // Send individual requests for each setting since there's no bulk endpoint - const updatePromises = Object.entries(settings).map( - async ([key, value]) => { - const url = new URL(`${this.baseUrl}/api/app-settings/${key}`); - url.searchParams.append("value", String(value)); - - const response = await fetch(url.toString(), { - method: "POST", - headers: { - "Content-Type": "application/json", - }, - }); - - if (!response.ok) { - const errorText = await response.text(); - console.error( - `Failed to update setting ${key}: ${response.status} ${response.statusText}`, - errorText - ); - throw new Error(`Failed to update ${key}: ${response.status}`); - } - - return response.json(); - } - ); + // Send individual requests for each setting since there's no bulk endpoint + const updatePromises = Object.entries(settings).map(async ([key, value]) => { + const response = await fetch(`${this.baseUrl}/api/app-settings/${key}`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ value }), + }); + if (!response.ok) { + const errorText = await response.text(); + console.error( + `Failed to update setting ${key}: ${response.status} ${response.statusText}`, + errorText, + ); + throw new Error(`Failed to update ${key}: ${response.status}`); + } + return response.json(); + });archon-ui-main/src/services/serviceRegistryService.ts (2)
158-165: Resolved: path-segment encoding for getService.encodeURIComponent(serviceName) fixes traversal/broken-route issues. Good catch.
187-199: Resolved: encoding + typed apiRequest in deprecateService.Both URL-encoding and generic typing are now correct.
archon-ui-main/src/types/cleanProvider.ts (1)
47-54: Security: remove encrypted_key from frontend types; expose metadata only.Even encrypted, key material shouldn’t reach the client. Use is_set/masked_last4 instead.
export interface APIKeyConfig { provider: ProviderType; - encrypted_key: string; is_active: boolean; base_url?: string; + // server-computed metadata (no key material on client) + is_set?: boolean; + masked_last4?: string; created_at?: string; updated_at?: string; }
…d simplify key storage logic with upsert
…uctured logging and custom exceptions
… safety and clarity
…g in APIKeysSection
…AddProvider mutation
… configuration details
- Resolved conflicts by keeping the clean provider system implementation - Maintained migration notices and deprecation warnings for old credential system - Preserved the new provider_clean architecture
|
@Wirasm @leex279 @coleam00 This is ready for review. CodeRabbit complains more than my GF and most of the reports are not important/relevant!Please let me know if this PR is desirable and then i will finish the rest of the remaining TODO tasks and squash the commits. PS: I understand it is impossible for anyone to review this PR, i suggest cloning testing it manually and then if you think its merge-able we could merge it and deal with any leftover issues. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
python/src/providers_clean/infrastructure/dependencies.py (1)
175-179: Good: UoW is no longer cached; fresh instance per access.This resolves cross-request leakage and aligns with prior feedback.
archon-ui-main/src/components/settings/CodeExtractionSettings.tsx (1)
229-231: Resolved: 0 preserved for MAX_PROSE_RATIO (no more 0→0.15).Using parseFloatOrDefault fixes the earlier falsy-coalescing bug. Nice.
archon-ui-main/src/components/settings/APIKeysSection.tsx (1)
10-10: Prefer non-null arrays for state; drop nullable providers.This aligns with the project’s state conventions and simplifies render checks. Prior feedback already suggested this.
- const [providers, setProviders] = useState<ProviderType[] | null>(null); + const [providers, setProviders] = useState<ProviderType[]>([]); @@ - {isLoadingProviders ? ( + {isLoadingProviders ? ( <span className="text-xs text-gray-500">Loading…</span> - ) : providers && providers.length === 0 ? ( + ) : providers.length === 0 ? ( @@ - {providers && providers.length > 0 && ( + {providers.length > 0 && (Also applies to: 85-108, 109-133
archon-ui-main/src/types/cleanProvider.ts (1)
46-54: Security: don’t expose encrypted_key to the frontend.Even encrypted, returning key material to the client increases risk. Replace with metadata fields.
export interface APIKeyConfig { provider: ProviderType; - encrypted_key: string; is_active: boolean; base_url?: string; + is_set?: boolean; // server-computed: whether a key exists + masked_last4?: string; // optional display-only created_at?: string; updated_at?: string; }Please also ensure server responses never include key material. See verification snippet below to locate any remaining usages.
🧹 Nitpick comments (25)
python/src/providers_clean/infrastructure/dependencies.py (5)
11-23: Remove unused repository imports to keep Ruff/Mypy clean.
IAvailableModelsRepositoryandSupabaseAvailableModelsRepositoryare imported but unused in this module.Apply:
-from ..core.interfaces.repositories import ( - IModelConfigRepository, - IApiKeyRepository, - IUsageRepository, - IAvailableModelsRepository -) +from ..core.interfaces.repositories import ( + IModelConfigRepository, + IApiKeyRepository, + IUsageRepository, +)-from .repositories.supabase import ( - SupabaseModelConfigRepository, - SupabaseApiKeyRepository, - SupabaseUsageRepository, - SupabaseAvailableModelsRepository -) +from .repositories.supabase import ( + SupabaseModelConfigRepository, + SupabaseApiKeyRepository, + SupabaseUsageRepository, +)
61-77: Catch specific exceptions when validating ENCRYPTION_KEY and chain the cause.Broadened
except Exceptionhides root causes and violates the guidelines to use specific exception types.- try: - return Fernet(encryption_key.encode()) - except Exception: + try: + return Fernet(encryption_key.encode()) + except (ValueError, TypeError) as exc: raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=( "Invalid ENCRYPTION_KEY format. Provide a valid base64-encoded 32-byte key." ), - ) + ) from exc
186-209: Add precise return types without introducing import cycles.Use forward references/TYPE_CHECKING to satisfy Mypy while keeping runtime imports local.
Add to imports:
-from typing import Optional +from typing import Optional, TYPE_CHECKINGAdd after imports:
if TYPE_CHECKING: from ..services.model_sync_service import ModelSyncService from ..services.service_registry_service import ServiceRegistryServiceAnnotate return types:
-def get_model_sync_service(uow: IUnitOfWork = Depends(get_unit_of_work)): +def get_model_sync_service(uow: IUnitOfWork = Depends(get_unit_of_work)) -> "ModelSyncService": @@ -def get_service_registry_service(uow: IUnitOfWork = Depends(get_unit_of_work)): +def get_service_registry_service(uow: IUnitOfWork = Depends(get_unit_of_work)) -> "ServiceRegistryService":
48-78: Plan for key rotation (versioned keys or KMS).Single static
ENCRYPTION_KEYmeans rotations break decryption of existing data. Consider storing akey_idwith ciphertext and supporting multiple active keys, or delegating to a KMS.
26-46: Service key handling: ensure least privilege and protection.If the service runs on public infrastructure, restrict exposure of
SUPABASE_SERVICE_KEY(service role). Keep it server-only, avoid logging it, and consider scoping operations to RPCs with RLS instead of broad table access where possible.archon-ui-main/src/features/agents/hooks/useAgentMutations.ts (7)
44-47: Also cancel/invalidate the per-service config queryPrevents stale reads when components use agentKeys.config(serviceId).
// Cancel any outgoing refetches await queryClient.cancelQueries({ queryKey: agentKeys.configs() }); + await queryClient.cancelQueries({ + queryKey: agentKeys.config(serviceId), + }); @@ // Ensure cache consistency with server state queryClient.invalidateQueries({ queryKey: agentKeys.configs() }); + queryClient.invalidateQueries({ + queryKey: agentKeys.config(variables.serviceId), + }); @@ - setTimeout(() => { - queryClient.invalidateQueries({ queryKey: agentKeys.configs() }); - }, 1000); + setTimeout(() => { + queryClient.invalidateQueries({ queryKey: agentKeys.configs() }); + // Invalidate specific key as well to refresh detail views + // Note: harmless no-op if no pending variables + // (ignored by TanStack if not in cache) + }, 1000);Also applies to: 92-99
148-151: Don’t show a success toast before the mutation resolvesToast success in onSuccess; show an “adding…” info toast in onMutate if desired.
- // Show immediate success feedback - showToast(`${provider} added successfully`, "success"); + // Optional: progress feedback + showToast(`Adding ${provider}…`, "info"); @@ - onSuccess: (_data, _variables) => { + onSuccess: (_data, variables) => { + showToast(`${variables.provider} added successfully`, "success"); // Invalidate related queries to ensure server state consistency queryClient.invalidateQueries({ queryKey: providerKeys.apiKeys() }); queryClient.invalidateQueries({ queryKey: modelKeys.available() }); queryClient.invalidateQueries({ queryKey: providerKeys.metadata() }); queryClient.invalidateQueries({ queryKey: providerKeys.list() }); @@ - const event = new CustomEvent("agentConfigUpdated", { + const event = new CustomEvent("agentConfigUpdated", { detail: { type: "provider_added", + provider: variables.provider, timestamp: new Date().toISOString(), }, }); - window.dispatchEvent(event); + if (typeof window !== "undefined") { + window.dispatchEvent(event); + } },Also applies to: 174-190
82-91: Guard window.dispatchEvent for SSR/testsAvoids ReferenceError when window is undefined in non-browser contexts.
- window.dispatchEvent(event); + if (typeof window !== "undefined") { + window.dispatchEvent(event); + }Also applies to: 181-189, 280-287
117-124: Standardize backoff with exponential retryAlign mutations with an exponential backoff (with cap). Improves resilience per guidelines.
retry: (failureCount, error) => { // Don't retry validation errors if (error instanceof Error && error.message.includes("API key")) return false; - return failureCount < 1; // Retry once for network errors + return failureCount < 2; // Retry twice for transient issues }, - retryDelay: 1000, + retryDelay: (i) => Math.min(1000 * 2 ** i, 8000),- retry: (failureCount, _error) => { - return failureCount < 1; // Retry once for network errors - }, - retryDelay: 1000, + retry: (failureCount) => failureCount < 2, + retryDelay: (i) => Math.min(1000 * 2 ** i, 8000),return useMutation({ mutationFn: async ({ provider }: ProviderOperation) => { return agentApi.testApiKey(provider); }, + retry: (failureCount) => failureCount < 2, + retryDelay: (i) => Math.min(1000 * 2 ** i, 8000),Also applies to: 204-208, 299-303
223-231: Tighten cache typings (avoid any)Use minimal structural types to reduce accidental shape drift in optimistic updates.
- queryClient.setQueryData( - modelKeys.available(), - (old: any[] | undefined) => { + queryClient.setQueryData( + modelKeys.available(), + (old: Array<{ provider: string }> | undefined) => { @@ - queryClient.setQueryData( - providerKeys.metadata(), - (old: Record<string, any> | undefined) => { + queryClient.setQueryData( + providerKeys.metadata(), + (old: Record<string, { configured: boolean; status: string; lastTested?: string }> | undefined) => { @@ - queryClient.setQueryData( - providerKeys.metadata(), - (old: Record<string, any> | undefined) => { + queryClient.setQueryData( + providerKeys.metadata(), + (old: Record<string, { configured: boolean; status: string; lastTested?: string }> | undefined) => { @@ - queryClient.setQueryData( - providerKeys.metadata(), - (old: Record<string, any> | undefined) => { + queryClient.setQueryData( + providerKeys.metadata(), + (old: Record<string, { configured: boolean; status: string; lastTested?: string }> | undefined) => {Also applies to: 232-247, 317-331, 359-379
357-386: Invalidate metadata after test to ensure server-aligned stateAdd a follow-up invalidation to reconcile any fields not patched by the optimistic update.
onSuccess: (data, variables) => { @@ if (data.configured && data.status === "active") { showToast(`${variables.provider} connection successful`, "success"); } else { showToast(`${variables.provider} connection failed`, "error"); } }, + onSettled: () => { + // Refresh from server; inexpensive and prevents drift + setTimeout(() => { + queryClient.invalidateQueries({ queryKey: providerKeys.metadata() }); + }, 500); + },
270-279: Add optimistic removal of provider from API keys cacheRight now metadata flips to
not_configuredbut the API keys list still includes the removed provider until cache invalidation, which can confuse users. Inarchon-ui-main/src/features/agents/hooks/useAgentMutations.ts(inside theremoveProvidermutation’sonMutateblock), after removing models add:+ // Optimistically remove provider from active list + queryClient.setQueryData( + providerKeys.apiKeys(), + (old: string[] | undefined) => + old ? old.filter((p) => p !== provider) : old, + );Note:
ModelStatusBar.tsx(lines 227–231) listens for theagentConfigUpdatedevent to refresh its view—verify this integration behaves as intended.archon-ui-main/src/components/settings/CodeExtractionSettings.tsx (11)
34-43: Good fix on NaN handling; consider adding a shared clamp helper.Parsers look solid and resolve the earlier 0-as-falsy bug. Add a small clamp to reuse across numeric fields below.
const parseIntOrDefault = (value: string, defaultValue: number) => { const parsed = parseInt(value, 10); return Number.isNaN(parsed) ? defaultValue : parsed; }; + + // Keep typed and reusable for all numeric inputs + const clamp = (n: number, min: number, max: number) => Math.min(max, Math.max(min, n));
47-57: Name loading state isSaving; surface error detail in toast.Aligns with repo naming conventions and gives users actionable feedback.
- setSaving(true); + setIsSaving(true); await credentialsService.updateCodeExtractionSettings( codeExtractionSettings ); showToast("Code extraction settings saved successfully!", "success"); } catch (err) { - console.error("Failed to save code extraction settings:", err); - showToast("Failed to save settings", "error"); + console.error("Failed to save code extraction settings:", err); + const message = err instanceof Error ? err.message : String(err); + showToast(`Failed to save settings: ${message}`, "error"); } finally { - setSaving(false); + setIsSaving(false); }Outside this hunk, update the state declaration:
const [isSaving, setIsSaving] = useState(false);
66-85: Verify Button API (icon/accentColor/size) vs. the new primitives.If this file should use the new primitives (features/ui/primitives/button.tsx), replace custom icon/size handling with the built-in loading prop and valid size tokens.
Example with primitives:
<Button variant="outline" size="default" loading={isSaving} onClick={handleSave} className="whitespace-nowrap"> {isSaving ? "Saving..." : "Save Settings"} </Button>If ../ui/Button is intentionally different, ignore. Please confirm which Button system is canonical here.
93-107: Clamp minimum length to the advertised range.Prevents out-of-range saves and stabilizes UX.
- MIN_CODE_BLOCK_LENGTH: parseIntOrDefault(e.target.value, 250), + MIN_CODE_BLOCK_LENGTH: clamp(parseIntOrDefault(e.target.value, 250), 50, 2000),
108-122: Clamp maximum length to the advertised range.- MAX_CODE_BLOCK_LENGTH: parseIntOrDefault(e.target.value, 5000), + MAX_CODE_BLOCK_LENGTH: clamp(parseIntOrDefault(e.target.value, 5000), 1000, 20000),
223-237: Clamp ratio to 0–1 to match UI hint.- MAX_PROSE_RATIO: parseFloatOrDefault(e.target.value, 0.15), + MAX_PROSE_RATIO: clamp(parseFloatOrDefault(e.target.value, 0.15), 0, 1),
241-252: Clamp indicator count to 1–10.- MIN_CODE_INDICATORS: parseIntOrDefault(e.target.value, 3), + MIN_CODE_INDICATORS: clamp(parseIntOrDefault(e.target.value, 3), 1, 10),
256-267: Clamp context window to 100–5000.- CONTEXT_WINDOW_SIZE: parseIntOrDefault(e.target.value, 1000), + CONTEXT_WINDOW_SIZE: clamp(parseIntOrDefault(e.target.value, 1000), 100, 5000),
271-285: Clamp worker count to 1–10.- CODE_EXTRACTION_MAX_WORKERS: parseIntOrDefault( - e.target.value, - 3 - ), + CODE_EXTRACTION_MAX_WORKERS: clamp(parseIntOrDefault(e.target.value, 3), 1, 10),
293-295: Copy nit: “percentage” vs “ratio”.The UI uses a 0–1 range; call it a ratio to avoid confusion.
- <strong>Max Prose Ratio:</strong> Maximum percentage of prose - indicators allowed (0-1) + <strong>Max Prose Ratio:</strong> Maximum ratio of prose + indicators allowed (0–1)
307-308: Confirm scope of “Max Workers” text.If workers apply to extraction broadly (not only summaries), tweak the copy to avoid narrowing. Otherwise ignore.
archon-ui-main/src/components/settings/APIKeysSection.tsx (2)
14-14: Prevent double submit; add saving state and disable Save Key.Avoid concurrent saves and reflect progress.
const [isBootstrapping, setIsBootstrapping] = useState(false); + const [isSavingKey, setIsSavingKey] = useState(false); @@ const handleSetProviderKey = async () => { if (!selectedProvider || !providerKey) { showToast("Select a provider and enter an API key", "error"); return; } try { + setIsSavingKey(true); await cleanProviderService.setApiKey( selectedProvider, providerKey ); showToast( `API key saved for ${selectedProvider}. Syncing models...`, "success" ); setProviderKey(""); // Refresh providers list after saving key await loadProviders(); } catch (err: unknown) { - console.error("Failed to set provider key", err); + console.error("Failed to set provider key", { provider: selectedProvider, err }); showToast("Failed to set provider key", "error"); } finally { + setIsSavingKey(false); } }; @@ - <Button onClick={handleSetProviderKey}>Save Key</Button> + <Button + onClick={handleSetProviderKey} + disabled={isSavingKey || !providerKey || !selectedProvider} + > + {isSavingKey ? ( + <> + <Loader2 className="w-4 h-4 mr-2 animate-spin" /> + Saving… + </> + ) : ( + "Save Key" + )} + </Button>Also applies to: 53-74, 130-131
1-1: Make loadProviders stable; remove eslint-disable by using useCallback.Keeps deps correct and avoids ignoring lint rules.
-import { useState, useEffect } from "react"; +import { useState, useEffect, useCallback } from "react"; @@ - useEffect(() => { - loadProviders(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + useEffect(() => { + loadProviders(); + }, [loadProviders]); @@ - const loadProviders = async () => { + const loadProviders = useCallback(async () => { try { setIsLoadingProviders(true); const list = await cleanProviderService.getProviders(); setProviders(list); - if (list.length > 0 && !selectedProvider) setSelectedProvider(list[0]); + if (list.length > 0) { + setSelectedProvider((prev) => prev ?? list[0]); + } } catch (err: unknown) { const status = (err as any)?.status ?? (err as any)?.response?.status; if (status === 404) { // empty providers table setProviders([]); } else { console.error("loadProviders failed", { err }); showToast("Failed to load providers. Please try again.", "error"); setProviders([]); } } finally { setIsLoadingProviders(false); } - }; + }, [showToast]);Also applies to: 18-22, 24-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
archon-ui-main/src/components/settings/APIKeysSection.tsx(1 hunks)archon-ui-main/src/components/settings/CodeExtractionSettings.tsx(5 hunks)archon-ui-main/src/features/agents/hooks/useAgentMutations.ts(1 hunks)archon-ui-main/src/types/cleanProvider.ts(1 hunks)python/src/providers_clean/api/routes/delete_api_key.py(1 hunks)python/src/providers_clean/infrastructure/dependencies.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- python/src/providers_clean/api/routes/delete_api_key.py
🧰 Additional context used
📓 Path-based instructions (8)
archon-ui-main/src/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint (standard React rules) for legacy UI code in /src/components
Files:
archon-ui-main/src/components/settings/APIKeysSection.tsxarchon-ui-main/src/components/settings/CodeExtractionSettings.tsx
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use state naming conventions: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
archon-ui-main/src/**/*.{ts,tsx}: WebSocket event failures should be logged and not crash serving other clients
External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Never return null to signal failure in async/data flows; throw errors with details
Use polling (HTTP) with provided hooks (usePolling, useDatabaseMutation, useProjectMutation); Socket.IO is removed
State naming: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
Persist theme choice in localStorage and respect Tailwind dark mode classes across components
Files:
archon-ui-main/src/components/settings/APIKeysSection.tsxarchon-ui-main/src/features/agents/hooks/useAgentMutations.tsarchon-ui-main/src/components/settings/CodeExtractionSettings.tsxarchon-ui-main/src/types/cleanProvider.ts
archon-ui-main/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Legacy components live under src/components and may be used by pages; add related tests under archon-ui-main/test
Files:
archon-ui-main/src/components/settings/APIKeysSection.tsxarchon-ui-main/src/components/settings/CodeExtractionSettings.tsx
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid comment keywords like LEGACY, CHANGED, REMOVED; write comments that document current functionality only
Files:
archon-ui-main/src/components/settings/APIKeysSection.tsxarchon-ui-main/src/features/agents/hooks/useAgentMutations.tsarchon-ui-main/src/components/settings/CodeExtractionSettings.tsxarchon-ui-main/src/types/cleanProvider.ts
archon-ui-main/src/features/**/{hooks,services}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TanStack Query for all data fetching (no prop drilling), with query key factory pattern and smart polling via useSmartPolling
Files:
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Implement optimistic updates with rollback in TanStack Query mutations
Use Biome formatting and lint rules in /src/features with 120-character lines, double quotes, and trailing commas
Implement HTTP polling patterns: 1–2s for active ops, 5–10s for background, smart pausing on tab inactivity, use ETag caching where applicable
archon-ui-main/src/features/**/*.{ts,tsx}: Use Radix UI primitives for UI in /features; place primitives under src/features/ui/primitives
Use TanStack Query for all data fetching in /features (no manual fetch chains in components)
Apply Tron-inspired glassmorphism styling using Tailwind classes in features UI
Use vertical slice structure: each feature owns its components/hooks/types/services; nest sub-features by domain
Run and address Biome recommended lint rules in features
No prop drilling; use TanStack Query and context/hooks for state/data flow
Files:
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts
archon-ui-main/src/features/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Adhere to Biome settings in /features: 2-space formatting, 80-char line width, import sorting/grouping
Files:
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts
python/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/**/*.py: Fail fast on service startup failures, missing configuration, database connection failures, authentication/authorization failures, critical dependencies unavailable, and invalid/corrupting data
Never accept or store corrupted data; on operation failure skip the item entirely rather than writing placeholders (e.g., zero embeddings)
For batch/background operations, continue processing but log each failure with details; track both successes and failures
Use specific exception types (avoid bare Exception), include context/IDs/URLs in messages, preserve full stack traces with logging (exc_info=True), and never return None/null to indicate failure—raise with details
Use database task status values directly: todo, doing, review, done
Target Python 3.12 style with 120-character line length; use Ruff for linting and Mypy for type checking
python/src/**/*.py: Target Python 3.12 and keep lines within 120 characters
Use Ruff for linting (errors, warnings, unused imports, code style) and keep code Ruff-clean
Use Mypy for type checking; maintain type-safe code across backend
Files:
python/src/providers_clean/infrastructure/dependencies.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: zaksnet
PR: coleam00/Archon#473
File: archon-ui-main/src/components/agent-chat/ArchonChatPanel.tsx:26-31
Timestamp: 2025-09-10T11:35:20.630Z
Learning: When reviewing large refactoring PRs like "Clean multi provider feature", focus feedback on the core objectives (provider management, API keys, model configuration, service registry) rather than unrelated UI improvements in components that were only incidentally modified during the refactor.
📚 Learning: 2025-09-06T20:04:08.111Z
Learnt from: CR
PR: coleam00/Archon#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T20:04:08.111Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : Use state naming conventions: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
Applied to files:
archon-ui-main/src/components/settings/APIKeysSection.tsx
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : State naming: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
Applied to files:
archon-ui-main/src/components/settings/APIKeysSection.tsx
📚 Learning: 2025-09-10T11:35:20.630Z
Learnt from: zaksnet
PR: coleam00/Archon#473
File: archon-ui-main/src/components/agent-chat/ArchonChatPanel.tsx:26-31
Timestamp: 2025-09-10T11:35:20.630Z
Learning: When reviewing large refactoring PRs like "Clean multi provider feature", focus feedback on the core objectives (provider management, API keys, model configuration, service registry) rather than unrelated UI improvements in components that were only incidentally modified during the refactor.
Applied to files:
archon-ui-main/src/components/settings/APIKeysSection.tsx
📚 Learning: 2025-09-06T20:04:08.111Z
Learnt from: CR
PR: coleam00/Archon#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T20:04:08.111Z
Learning: Applies to archon-ui-main/src/features/**/*.{ts,tsx} : Implement optimistic updates with rollback in TanStack Query mutations
Applied to files:
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts
📚 Learning: 2025-09-06T20:04:08.111Z
Learnt from: CR
PR: coleam00/Archon#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T20:04:08.111Z
Learning: Applies to archon-ui-main/src/features/**/{hooks,services}/**/*.{ts,tsx} : Use TanStack Query for all data fetching (no prop drilling), with query key factory pattern and smart polling via useSmartPolling
Applied to files:
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/features/**/*.{ts,tsx} : Use TanStack Query for all data fetching in /features (no manual fetch chains in components)
Applied to files:
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : Use polling (HTTP) with provided hooks (usePolling, useDatabaseMutation, useProjectMutation); Socket.IO is removed
Applied to files:
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/features/**/*.{ts,tsx,json} : Adhere to Biome settings in /features: 2-space formatting, 80-char line width, import sorting/grouping
Applied to files:
archon-ui-main/src/components/settings/CodeExtractionSettings.tsx
📚 Learning: 2025-09-06T20:04:08.111Z
Learnt from: CR
PR: coleam00/Archon#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T20:04:08.111Z
Learning: Applies to archon-ui-main/src/features/**/*.{ts,tsx} : Use Biome formatting and lint rules in /src/features with 120-character lines, double quotes, and trailing commas
Applied to files:
archon-ui-main/src/components/settings/CodeExtractionSettings.tsx
🧬 Code graph analysis (5)
archon-ui-main/src/components/settings/APIKeysSection.tsx (1)
archon-ui-main/src/services/cleanProviderService.ts (2)
ProviderType(465-465)cleanProviderService(457-457)
archon-ui-main/src/features/agents/hooks/useAgentMutations.ts (3)
archon-ui-main/src/features/agents/types/index.ts (3)
AgentConfigUpdate(19-22)ModelConfig(12-12)ProviderOperation(24-28)archon-ui-main/src/features/agents/services/agentService.ts (1)
agentApi(11-37)archon-ui-main/src/features/agents/utils/queryKeys.ts (3)
agentKeys(6-11)providerKeys(18-23)modelKeys(13-16)
python/src/providers_clean/infrastructure/dependencies.py (10)
python/src/providers_clean/core/interfaces/unit_of_work.py (1)
IUnitOfWork(15-43)python/src/providers_clean/core/interfaces/repositories.py (4)
IModelConfigRepository(14-75)IApiKeyRepository(78-160)IUsageRepository(163-244)IAvailableModelsRepository(247-370)python/src/providers_clean/infrastructure/unit_of_work/supabase_uow.py (1)
SupabaseUnitOfWork(16-100)python/src/providers_clean/infrastructure/repositories/supabase/model_config_repository.py (1)
SupabaseModelConfigRepository(9-175)python/src/providers_clean/infrastructure/repositories/supabase/api_key_repository.py (1)
SupabaseApiKeyRepository(20-192)python/src/providers_clean/infrastructure/repositories/supabase/usage_repository.py (1)
SupabaseUsageRepository(10-231)python/src/providers_clean/infrastructure/repositories/supabase/available_models_repository.py (1)
SupabaseAvailableModelsRepository(9-356)python/src/providers_clean/services/model_sync_service.py (1)
ModelSyncService(14-574)python/src/providers_clean/api/deps.py (1)
get_service_registry_service(30-32)python/src/providers_clean/services/service_registry_service.py (1)
ServiceRegistryService(60-597)
archon-ui-main/src/components/settings/CodeExtractionSettings.tsx (3)
archon-ui-main/src/services/credentialsService.ts (1)
credentialsService(405-405)archon-ui-main/src/features/ui/primitives/button.tsx (1)
Button(11-115)archon-ui-main/src/features/ui/primitives/input.tsx (1)
Input(8-29)
archon-ui-main/src/types/cleanProvider.ts (1)
python/src/providers_clean/api/schemas.py (3)
AvailableModel(25-42)ServiceStatus(45-53)ProviderMetadata(65-75)
🔇 Additional comments (4)
archon-ui-main/src/components/settings/CodeExtractionSettings.tsx (2)
126-170: Detection toggles read clean and consistent.
171-215: Filtering toggles LGTM.archon-ui-main/src/types/cleanProvider.ts (2)
79-88: ServiceStatus matches backend schema.Fields align with python/src/providers_clean/api/schemas.py (service_name, model_string, provider, model, api_key_configured, temperature, max_tokens).
46-54: No remaining frontend exposures ofencrypted_key
Only defined in theAPIKeyConfigtype (cleanProvider.ts) and no usages or console logs of any API key patterns were found.
| } catch (err: unknown) { | ||
| // If 404, providers table is empty | ||
| setProviders([]); | ||
| } finally { | ||
| setLoading(false); | ||
| setIsLoadingProviders(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Load errors are swallowed; surface non-404 failures with context.
Right now all errors collapse to “empty list,” masking outages and violating the logging/UX guidelines.
} catch (err: unknown) {
- // If 404, providers table is empty
- setProviders([]);
+ const status = (err as any)?.status ?? (err as any)?.response?.status;
+ if (status === 404) {
+ // empty providers table
+ setProviders([]);
+ } else {
+ console.error("loadProviders failed", { err });
+ showToast("Failed to load providers. Please try again.", "error");
+ setProviders([]);
+ }
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err: unknown) { | |
| // If 404, providers table is empty | |
| setProviders([]); | |
| } finally { | |
| setLoading(false); | |
| setIsLoadingProviders(false); | |
| } | |
| } catch (err: unknown) { | |
| const status = (err as any)?.status ?? (err as any)?.response?.status; | |
| if (status === 404) { | |
| // empty providers table | |
| setProviders([]); | |
| } else { | |
| console.error("loadProviders failed", { err }); | |
| showToast("Failed to load providers. Please try again.", "error"); | |
| setProviders([]); | |
| } | |
| } finally { | |
| setIsLoadingProviders(false); | |
| } |
| {providers && providers.length > 0 ? ( | ||
| <div className="grid gap-2"> | ||
| {providers.map((provider) => ( | ||
| <div | ||
| key={provider} | ||
| className="flex items-center justify-between p-3 bg-gray-50 dark:bg-gray-800/50 rounded-md border border-gray-200 dark:border-gray-700" | ||
| > | ||
| <div className="flex items-center gap-3"> | ||
| <div className="w-8 h-8 rounded-full bg-gradient-to-br from-blue-500 to-purple-600 flex items-center justify-center"> | ||
| <span className="text-white text-xs font-bold uppercase"> | ||
| {provider.charAt(0)} | ||
| </span> | ||
| </div> | ||
| <div> | ||
| <span className="text-sm font-medium capitalize text-gray-900 dark:text-white"> | ||
| {provider} | ||
| </span> | ||
| <p className="text-xs text-gray-500 dark:text-gray-400"> | ||
| API key configured | ||
| </p> | ||
| </div> | ||
| </div> | ||
| <div className="flex items-center gap-2"> | ||
| <div className="flex items-center gap-1"> | ||
| <div className="w-2 h-2 bg-green-500 rounded-full animate-pulse"></div> | ||
| <span className="text-xs text-green-600 dark:text-green-400 font-medium"> | ||
| Active | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t claim “API key configured” or “Active” without backing data.
This UI unconditionally shows that each provider is configured and active, which is misleading and may confuse users after the breaking change (keys must be re-added).
Short-term fix (adjust copy until real status is wired):
- <p className="text-xs text-gray-500 dark:text-gray-400">
- API key configured
- </p>
+ <p className="text-xs text-gray-500 dark:text-gray-400">
+ Status unknown — add a key above to configure
+ </p>
@@
- <div className="flex items-center gap-2">
- <div className="flex items-center gap-1">
- <div className="w-2 h-2 bg-green-500 rounded-full animate-pulse"></div>
- <span className="text-xs text-green-600 dark:text-green-400 font-medium">
- Active
- </span>
- </div>
- </div>
+ {/* TODO: render real status/health once available */}Preferred fix: fetch/display real per-provider status (configured/health) from cleanProviderService (e.g., include is_set/health in the response) and conditionally render. Do you want me to draft the service+UI wiring?
🏁 Script executed:
#!/bin/bash
# Verify that provider entries unconditionally render "API key configured" and "Active" statuses
rg -n -C5 "providers.map" archon-ui-main/src/components/settings/APIKeysSection.tsxLength of output: 1140
🏁 Script executed:
#!/bin/bash
# Confirm the static rendering of "API key configured"
rg -n -C3 "API key configured" archon-ui-main/src/components/settings/APIKeysSection.tsxLength of output: 293
🏁 Script executed:
#!/bin/bash
# Search for usage of cleanProviderService to see if real status is wired
rg -n -C3 "cleanProviderService" -g "*.tsx"Length of output: 3984
Replace static “API key configured” and “Active” labels in APIKeysSection.tsx
- In
archon-ui-main/src/components/settings/APIKeysSection.tsx(lines 163–166), change
<p className="text-xs …">API key configured</p>
to
<p className="text-xs …">Status unknown – add a key above to configure</p>. - Remove (or comment out) the static “Active” badge block immediately following that snippet.
- Long-term: extend or update
cleanProviderService.getProviders()to return per-provideris_setandhealth, then conditionally render real configured/active states.
🤖 Prompt for AI Agents
In archon-ui-main/src/components/settings/APIKeysSection.tsx around lines 146 to
176, replace the hardcoded paragraph text "API key configured" with "Status
unknown – add a key above to configure" and remove (or comment out) the static
"Active" badge block that follows the provider info; keep the surrounding markup
intact so the provider list still renders, and leave a TODO comment to later
update cleanProviderService.getProviders() to supply per-provider is_set and
health for conditional rendering.
| const providersWithoutApiKey = ["ollama"]; | ||
|
|
||
| const requiresApiKey = (provider: string): boolean => { | ||
| return !providersWithoutApiKey.includes(provider); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize provider check and require baseUrl for no-key providers
- Make provider check case-insensitive and O(1) with a Set.
- Enforce baseUrl for providers that don’t need an API key (e.g., ollama). Prevents accepting incomplete configs.
-const providersWithoutApiKey = ["ollama"];
-
-const requiresApiKey = (provider: string): boolean => {
- return !providersWithoutApiKey.includes(provider);
-};
+const NO_KEY_PROVIDERS = new Set<string>(["ollama"]);
+const requiresApiKey = (provider: string): boolean => {
+ return !NO_KEY_PROVIDERS.has(provider.toLowerCase());
+};
@@
mutationFn: async ({ provider, apiKey, baseUrl }: ProviderOperation) => {
if (requiresApiKey(provider) && !apiKey)
throw new Error("API key is required");
- return agentApi.setApiKey(provider, apiKey || "", baseUrl);
+ if (!requiresApiKey(provider) && !baseUrl) {
+ throw new Error("Base URL is required for local providers");
+ }
+ return agentApi.setApiKey(provider, apiKey ?? "", baseUrl);
},Also applies to: 111-116
| if (!old) | ||
| return { [provider]: { configured: true, status: "testing" } }; | ||
| return { | ||
| ...old, | ||
| [provider]: { | ||
| ...old[provider], | ||
| status: "testing", | ||
| lastTested: new Date().toISOString(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t mark provider configured during “testing” optimistic state
Setting configured: true pre-success can mislead the UI. Default to false (or preserve previous value).
- if (!old)
- return { [provider]: { configured: true, status: "testing" } };
+ if (!old) {
+ return {
+ [provider]: {
+ configured: false,
+ status: "testing",
+ lastTested: new Date().toISOString(),
+ },
+ };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!old) | |
| return { [provider]: { configured: true, status: "testing" } }; | |
| return { | |
| ...old, | |
| [provider]: { | |
| ...old[provider], | |
| status: "testing", | |
| lastTested: new Date().toISOString(), | |
| if (!old) { | |
| return { | |
| [provider]: { | |
| configured: false, | |
| status: "testing", | |
| lastTested: new Date().toISOString(), | |
| }, | |
| }; | |
| } | |
| return { | |
| ...old, | |
| [provider]: { | |
| ...old[provider], | |
| status: "testing", | |
| lastTested: new Date().toISOString(), | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In archon-ui-main/src/features/agents/hooks/useAgentMutations.ts around lines
320 to 327, the optimistic update marks provider as configured: true during the
"testing" state which can mislead the UI; change the optimistic update to
preserve the previous configured value when old exists, and default to
configured: false only when there is no prior entry, while still setting status:
"testing" and updating lastTested to the current ISO timestamp.
| // Provider metadata from backend | ||
| export interface ProviderMetadata { | ||
| provider: string; | ||
| model_count: number; | ||
| max_context_length: number; | ||
| min_input_cost: number; | ||
| max_input_cost: number; | ||
| has_free_models: boolean; | ||
| supports_vision: boolean; | ||
| supports_tools: boolean; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align ProviderMetadata nullability and add last_sync to match backend.
Python schema exposes Optional[float] for min/max costs and a last_sync field.
export interface ProviderMetadata {
provider: string;
model_count: number;
max_context_length: number;
- min_input_cost: number;
- max_input_cost: number;
+ min_input_cost: number | null;
+ max_input_cost: number | null;
has_free_models: boolean;
supports_vision: boolean;
supports_tools: boolean;
+ last_sync?: string;
top_models?: Array<{
model: string;
context_length: number;
input_cost: number;
output_cost: number;
}>;
}Also applies to: 121-129
🤖 Prompt for AI Agents
In archon-ui-main/src/types/cleanProvider.ts around lines 112-121 (and also
apply the same change to 121-129), the TypeScript ProviderMetadata interface
currently types min_input_cost and max_input_cost as non-nullable numbers and
omits last_sync; update the interface so min_input_cost and max_input_cost are
nullable (number | null or undefined if your codebase prefers) to match Python's
Optional[float], and add a last_sync field (typed as number | null or string |
null depending on backend format—use number | null if backend returns a
timestamp) marked optional/nullable to mirror the backend schema.
| @lru_cache() | ||
| def get_supabase_client() -> Client: | ||
| """Get or create Supabase client instance. | ||
|
|
||
| Returns: | ||
| Supabase client | ||
|
|
||
| Raises: | ||
| HTTPException: If database configuration is missing | ||
| """ | ||
| url = os.environ.get("SUPABASE_URL") | ||
| key = os.environ.get("SUPABASE_SERVICE_KEY") | ||
|
|
||
| if not url or not key: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Database configuration missing. Please set SUPABASE_URL and SUPABASE_SERVICE_KEY environment variables." | ||
| ) | ||
|
|
||
| return create_client(url, key) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail fast on startup: pre-initialize critical dependencies.
Per guidelines, surface misconfiguration at startup, not first request. Add a small startup validator.
Diff to import FastAPI:
-from fastapi import Depends, HTTPException, status
+from fastapi import Depends, HTTPException, status, FastAPIAdd after the last line of the file:
def register_startup_validation(app: FastAPI) -> None:
@app.on_event("startup")
async def _validate_di() -> None:
# Force initialization; raise immediately if misconfigured
get_supabase_client()
get_encryption_cipher()🤖 Prompt for AI Agents
In python/src/providers_clean/infrastructure/dependencies.py around lines 26 to
46, startup should fail fast on missing critical dependencies; add a startup
validator that pre-initializes these dependencies so misconfiguration surfaces
at app startup rather than on first request. Import FastAPI at the top of the
file and add a function register_startup_validation(app: FastAPI) that registers
an @app.on_event("startup") coroutine which calls get_supabase_client() and
get_encryption_cipher() to force initialization and raise immediately if
misconfigured.
| def reset(self): | ||
| """Reset all cached dependencies.""" | ||
| self._supabase_client = None | ||
| self._cipher = None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset should also clear @lru_cache to avoid stale clients/keys after rotations.
DependencyContainer.reset() nulls fields but get_supabase_client/get_encryption_cipher remain cached; rotations won’t take effect.
def reset(self):
"""Reset all cached dependencies."""
self._supabase_client = None
self._cipher = None
+ # Ensure DI functions pick up new env/config
+ try:
+ get_supabase_client.cache_clear()
+ get_encryption_cipher.cache_clear()
+ except AttributeError:
+ pass🤖 Prompt for AI Agents
In python/src/providers_clean/infrastructure/dependencies.py around lines 180 to
184, the reset() method only nulls instance fields but does not invalidate the
@lru_cache on the factory functions, so rotated clients/keys remain stale;
update reset() to also call cache clear on the cached functions (e.g.
get_supabase_client.cache_clear() and get_encryption_cipher.cache_clear()) in
addition to setting self._supabase_client and self._cipher to None so subsequent
calls recreate fresh instances from new credentials.
…or provider management
|
Hi @zaksnet, thanks for the info. its named Ollama, but it includes other providers as well. Can you please take a look and check yourself? |
|
Looking a bit deeper I think it colides with a lot more of the other PRs we do at the moment for KnowledgeBase etc. as well. This PR is very very big. Think this would be needed to split into smaller PRs. But first we need to see how it compare to the ollama-integration. |
|
No worries @leex279, i did check olama integration and although i could not find anything related to supporting multiple providers (i assume its still under work), it's definitely a more reviewable and clean PR. When i tried to merge olama integration there were not too many conflicts, but as you suggested my pr is very big and although most of the changes are under I will close this for now to avoid confusion and we can look at it once |
…leam00#473) Add test coverage for factory singleton behavior and edge cases in get(), adopt(), create(), and destroy() methods identified during code review (PR coleam00#274). Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…leam00#473) Add test coverage for factory singleton behavior and edge cases in get(), adopt(), create(), and destroy() methods identified during code review (PR coleam00#274). Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Status:
TODO:
Breaking changes:
Summary by CodeRabbit
New Features
Improvements
Chores