Repository navigation
refactor(types): restore centralized type system architecture - #166
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors the type system by deleting src/lib/core/types.ts and relocating provider/generation types into src/lib/types/*. Adds new OpenAI model enums, APIVersions, and DEFAULT_MODEL_ALIASES. Updates imports across CLI, providers, utils, model registry, and tests to the new locations. Tests reflect minor AIProvider/interface and usage-shape adjustments. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/utils/conversationMemory.ts (1)
164-167: Harden against undefined AI response to avoid length access crash.result.content may be absent for some providers/errors. Use a safe default.
Apply:
- const aiResponse = result.content; + const aiResponse = + typeof result.content === "string" ? result.content : "";Also applies to: 181-183
src/lib/types/providers.ts (1)
46-51: Do not hardcode a region/account-scoped ARN for Bedrock model ID.Using an ARN with a specific region/account will break outside us-east-2 or for other accounts. Keep consistency with other Bedrock IDs (string modelId), or make ARN usage configurable.
Apply:
export enum BedrockModels { CLAUDE_3_SONNET = "anthropic.claude-3-sonnet-20240229-v1:0", CLAUDE_3_HAIKU = "anthropic.claude-3-haiku-20240307-v1:0", CLAUDE_3_5_SONNET = "anthropic.claude-3-5-sonnet-20240620-v1:0", - CLAUDE_3_7_SONNET = "arn:aws:bedrock:us-east-2:225681119357:inference-profile/us.anthropic.claude-3-7-sonnet-20250219-v1:0", + CLAUDE_3_7_SONNET = "anthropic.claude-3-7-sonnet-20250219-v1:0", }If inference profiles are required in your code paths, expose them via config rather than baking in a region/account.
🧹 Nitpick comments (6)
src/lib/types/providers.ts (2)
564-579: Model aliases are helpful; consider adding a reasoning alias mapping to O1.Optional, but improves discoverability for users seeking “reasoning” models.
export const DEFAULT_MODEL_ALIASES = { // Latest recommended models per provider LATEST_OPENAI: OpenAIModels.GPT_4O, FASTEST_OPENAI: OpenAIModels.GPT_4O_MINI, LATEST_ANTHROPIC: AnthropicModels.CLAUDE_3_5_SONNET, FASTEST_ANTHROPIC: AnthropicModels.CLAUDE_3_5_HAIKU, LATEST_GOOGLE: GoogleAIModels.GEMINI_2_5_PRO, FASTEST_GOOGLE: GoogleAIModels.GEMINI_2_5_FLASH, // Best models by use case BEST_CODING: AnthropicModels.CLAUDE_3_5_SONNET, BEST_ANALYSIS: GoogleAIModels.GEMINI_2_5_PRO, BEST_CREATIVE: AnthropicModels.CLAUDE_3_5_SONNET, BEST_VALUE: GoogleAIModels.GEMINI_2_5_FLASH, + BEST_REASONING: OpenAIModels.O1_PREVIEW, } as const;
164-165: Change ProviderName to enum value typeProviderName is currently the enum keys (BEDROCK | OPENAI | …) but callers expect the string values ("bedrock", "openai", …). Replace with the enum value type.
File: src/lib/types/providers.ts (lines 164-165)
-export type ProviderName = keyof typeof AIProviderName; +export type ProviderName = AIProviderName;src/lib/utils/providerHealth.ts (1)
934-939: Include O1 models in OpenAI common-model hints.Keeps hints aligned with supported enums and user expectations.
case AIProviderName.OPENAI: return [ OpenAIModels.GPT_4O, OpenAIModels.GPT_4O_MINI, OpenAIModels.GPT_3_5_TURBO, + OpenAIModels.O1_PREVIEW, + OpenAIModels.O1_MINI, ];src/lib/providers/azureOpenai.ts (1)
29-43: Endpoint parsing and version selection are sensible; minor enhancement suggestion.If AZURE_OPENAI_ENDPOINT is missing, the error path uses validateApiKey(createAzureEndpointConfig()). Consider a clearer utility name (e.g., validateRequiredConfig) to avoid confusion, or improve the error message to mention “endpoint,” not “API key.”
src/lib/utils/providerSetupMessages.ts (1)
11-11: Import-path migration to centralized types looks good; consider tightening the provider type to drop the cast.Optional: import AIProviderName and type the parameter so you can avoid
as keyof typeofand get better compile-time checks.import { OpenAIModels, GoogleAIModels, AnthropicModels, APIVersions, + type AIProviderName, } from "../types/providers.js"; @@ -export function getProviderSetupMessage( - provider: string, - missingVars: string[], -): string { +export function getProviderSetupMessage( + provider: AIProviderName, + missingVars: string[], +): string { @@ - const providerSetup = { + const providerSetup: Record<AIProviderName, { guide: string; envVars: string[] }> = { @@ - const setup = providerSetup[provider as keyof typeof providerSetup]; + const setup = providerSetup[provider];test/provider-edge-cases.test.ts (1)
39-41: Usage shape switched to { input, output, total } — good; minor DRY opportunity.To reduce duplication across mocks, extract a shared constant.
const MOCK_USAGE = { input: 20, output: 30, total: 50 } as const; // ...then use `usage: MOCK_USAGE` in both stream and generate for MockProvider, // and a FallbackUsage variant for FallbackProvider.Also applies to: 55-57, 95-97, 111-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/cli/loop/optionsSchema.ts(1 hunks)src/lib/core/types.ts(0 hunks)src/lib/models/modelRegistry.ts(1 hunks)src/lib/providers/azureOpenai.ts(1 hunks)src/lib/types/providers.ts(3 hunks)src/lib/utils/conversationMemory.ts(1 hunks)src/lib/utils/providerHealth.ts(1 hunks)src/lib/utils/providerSetupMessages.ts(1 hunks)test/factory-timeout.test.ts(2 hunks)test/provider-edge-cases.test.ts(5 hunks)test/provider-middleware.test.ts(1 hunks)test/providers/sagemaker.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/lib/core/types.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: YasmeenOgo
PR: juspay/neurolink#145
File: src/lib/core/types.ts:0-0
Timestamp: 2025-09-02T13:50:42.770Z
Learning: The APIVersions enum in src/lib/core/types.ts now contains comprehensive API version constants for all major AI providers: Azure OpenAI (latest, stable, legacy), OpenAI (current, beta), Google AI (current, beta), and Anthropic (current). This centralization helps avoid API version drift across the codebase.
📚 Learning: 2025-09-02T13:50:42.770Z
Learnt from: YasmeenOgo
PR: juspay/neurolink#145
File: src/lib/core/types.ts:0-0
Timestamp: 2025-09-02T13:50:42.770Z
Learning: The APIVersions enum in src/lib/core/types.ts now contains comprehensive API version constants for all major AI providers: Azure OpenAI (latest, stable, legacy), OpenAI (current, beta), Google AI (current, beta), and Anthropic (current). This centralization helps avoid API version drift across the codebase.
Applied to files:
src/lib/utils/providerHealth.tssrc/lib/providers/azureOpenai.tssrc/lib/models/modelRegistry.tssrc/lib/utils/providerSetupMessages.tssrc/lib/types/providers.ts
📚 Learning: 2025-08-19T05:02:34.555Z
Learnt from: coder-dodo
PR: juspay/neurolink#87
File: test/provider-edge-cases.test.ts:16-21
Timestamp: 2025-08-19T05:02:34.555Z
Learning: When using vi.mock() in Vitest, mocking the same import path that contains the system under test replaces the actual implementation with a mock, even if the physical file has a different extension (.ts vs .js). TypeScript's NodeNext module resolution treats .js imports as references to .ts files.
Applied to files:
test/provider-middleware.test.tstest/factory-timeout.test.ts
🧬 Code graph analysis (1)
src/lib/types/providers.ts (1)
src/lib/index.ts (1)
OpenAIModels(38-38)
🔇 Additional comments (12)
src/lib/utils/conversationMemory.ts (1)
13-15: Import path migration looks correct.Types moved to ../types/generateTypes.js are referenced consistently in this file.
src/lib/types/providers.ts (2)
62-64: O1 models addition looks good.Enums align with current naming; downstream imports compile cleanly.
132-149: Confirm API version constants against provider docs (as of 2025-09-11)src/lib/types/providers.ts:132-149 — values largely match official docs (Azure: 2025-04-01-preview, 2024-10-21; Google: v1 / v1beta; Anthropic: 2023-06-01; OpenAI: v1). Verify OPENAI_BETA — currently "v1-beta" in the enum; confirm OpenAI's exact beta token (often "v1beta" or none) and update to the precise string or remove if unused.
test/provider-middleware.test.ts (1)
3-3: Import path update to centralized types is correct.Matches the refactor; NodeNext resolution will still point to the .ts source during tests.
test/providers/sagemaker.test.ts (1)
3-3: Import path update looks good.Tests now reference AIProviderName from the centralized module.
src/lib/utils/providerHealth.ts (1)
8-14: Centralized type imports: LGTM.No behavior change; keeps provider constants in one place.
src/lib/models/modelRegistry.ts (1)
12-14: Switched DEFAULT_MODEL_ALIASES import to centralized module.Consistent with the type consolidation goal; alias build below remains correct.
src/lib/providers/azureOpenai.ts (1)
4-6: Good: consuming APIVersions from centralized types.Keeps Azure default version controlled from one place.
src/cli/loop/optionsSchema.ts (1)
1-2: Import path updates are correct and type-only usage is appropriate.Schema generation continues to reflect TextGenerationOptions accurately.
test/factory-timeout.test.ts (2)
6-6: Type import path updated to src/lib/types/providers.js — OK.This aligns the test with the restored central types.
20-25: Prefersatisfiesoverasfor the AIProvider mockMock now includes
gen— usesatisfies AIProviderso TypeScript will catch interface drift. Update in test/factory-timeout.test.ts (≈ lines 20–25):- mockPF.createProvider.mockResolvedValue({ - generate: vi.fn(), - gen: vi.fn(), - stream: vi.fn(), - setupToolExecutor: vi.fn(), - } as AIProvider); + mockPF.createProvider.mockResolvedValue({ + generate: vi.fn(), + gen: vi.fn(), + stream: vi.fn(), + setupToolExecutor: vi.fn(), + } satisfies AIProvider);Verify TypeScript supports
satisfies(TS ≥ 4.9) and confirm no lingering.jsimports tosrc/lib/core/types(e.g. runjq -r '.devDependencies.typescript // .dependencies.typescript' package.jsonornode -p "require('./package.json').devDependencies?.typescript || require('./package.json').dependencies?.typescript"andrg -n 'src/lib/core/types(\\.js)?' -g '!**/node_modules/**').test/provider-edge-cases.test.ts (1)
6-6: Imports moved to centralized types — OK; manual verification requiredAutomated repo search failed (ripgrep skipped files). Confirm locally that test/provider-edge-cases.test.ts (line ~6) imports AIProvider from '../src/lib/types/providers.js' and that no files import 'src/lib/core/types' or reference the old keys inputTokens / outputTokens / totalTokens.
76625d6 to
16c58a7
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request restores the centralized type system architecture by removing duplicate type definitions from the core module and updating imports to use the centralized types in /src/lib/types/. This fixes a problematic revert that had reintroduced duplicate types in the core module.
Key changes:
- Removed the entire
/src/lib/core/types.tsfile containing 403 lines of duplicate type definitions - Enhanced
/src/lib/types/providers.tswith new O1 models, API versions enum, and default model aliases - Updated 12 files to import types from centralized locations instead of core module
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/core/types.ts | Complete removal of duplicate type definitions (403 lines) |
| src/lib/types/providers.ts | Added O1 models, APIVersions enum, and DEFAULT_MODEL_ALIASES |
| test/providers/sagemaker.test.ts | Updated import path for AIProviderName |
| test/provider-middleware.test.ts | Updated imports for types from centralized location |
| test/provider-edge-cases.test.ts | Updated imports and fixed token usage property names |
| test/factory-timeout.test.ts | Updated import and removed hardcoded provider/model properties |
| src/lib/utils/providerSetupMessages.ts | Updated import path for provider types |
| src/lib/utils/providerHealth.ts | Updated import path for provider types |
| src/lib/utils/conversationMemory.ts | Updated import path for generation types |
| src/lib/types/streamTypes.ts | Fixed import path for AIProviderName and AnalyticsData |
| src/lib/providers/azureOpenai.ts | Updated import paths for provider types |
| src/lib/models/modelRegistry.ts | Updated import path for provider types |
| src/lib/index.ts | Updated type import paths in function signatures |
| src/cli/loop/optionsSchema.ts | Updated imports for types from centralized location |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import type { AIProviderName, AnalyticsData } from "../types/index.js"; | ||
| import type { TokenUsage } from "./analytics.js"; |
There was a problem hiding this comment.
The import path '../types/index.js' is incorrect. Based on the file structure and other imports in this PR, it should be './providers.js' for AIProviderName and './analytics.js' for AnalyticsData.
| import type { AIProviderName, AnalyticsData } from "../types/index.js"; | |
| import type { TokenUsage } from "./analytics.js"; | |
| import type { AIProviderName } from "./providers.js"; | |
| import type { AnalyticsData, TokenUsage } from "./analytics.js"; |
| input: 20, | ||
| output: 30, | ||
| total: 50, |
There was a problem hiding this comment.
The token usage property names have been changed from 'inputTokens/outputTokens/totalTokens' to 'input/output/total'. Ensure this change is consistent with the TokenUsage type definition and that all other test files and production code use the same property names.
| generate: vi.fn(), | ||
| gen: vi.fn(), | ||
| stream: vi.fn(), | ||
| setupToolExecutor: vi.fn(), |
There was a problem hiding this comment.
The mock object no longer includes 'provider' and 'model' properties. Verify that the AIProvider interface definition doesn't require these properties, or add them back if they are part of the expected interface.
Fixes problematic revert from commit c9952c0 that undid the type centralization work from commit 945fb47. This commit fully restores the clean separation of concerns between types and implementation code in src/lib/core/ module. - **REMOVED** `/src/lib/core/types.ts` (403 lines of duplicate types) - **ENHANCED** `/src/lib/types/providers.ts` with missing O1 models, `APIVersions` enum, and `DEFAULT_MODEL_ALIASES` - **UPDATED** 12 files to use centralized imports: `../core/types.js` → `../types/providers.js|generateTypes.js` - Types: Centralized in `/src/lib/types/` (definitions only) - Core: Implementation in `/src/lib/core/` (logic only) - Zero type definitions remain in core modules
16c58a7 to
06e733e
Compare
Fixes problematic revert from commit c9952c0 that undid the type centralization work from commit 945fb47. This commit fully restores the clean separation of concerns between types and implementation code in src/lib/core/ module.
REMOVED
/src/lib/core/types.ts(403 lines of duplicate types)ENHANCED
/src/lib/types/providers.tswith missing O1 models,APIVersionsenum, andDEFAULT_MODEL_ALIASESUPDATED 12 files to use centralized imports:
../core/types.js→../types/providers.js|generateTypes.jsTypes: Centralized in
/src/lib/types/(definitions only)Core: Implementation in
/src/lib/core/(logic only)Zero type definitions remain in core modules
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Deprecations
Refactor