Repository navigation
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 WalkthroughRefactors type locations and imports across config, types, and utils. Converts configuration interfaces to type aliases, updates import paths accordingly, and adds a public re-export for configuration types. Config manager switches to named imports for path/hash utilities without changing behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/types/configTypes.ts (1)
22-35: ProviderConfig name collision with providers domain (will break the barrel).This exported
ProviderConfigconflicts withProviderConfigalready re-exported from./providers.jsviasrc/lib/types/index.ts. Withexport * from "./configTypes.js"plus existing provider exports, the barrel will export two distinct symbols with the same name, causing a compile-time duplicate export error.Fix at the barrel by exporting config types explicitly and aliasing this one to avoid collision.
Apply in
src/lib/types/index.ts:-// Configuration types -export * from "./configTypes.js"; +// Configuration types (explicit to avoid ProviderConfig name collision) +export { + NeuroLinkConfig, + PerformanceConfig, + CacheConfig, + FallbackConfig, + RetryConfig, + AnalyticsConfig, + ToolConfig, + BackupInfo, + BackupMetadata, + ConfigValidationResult, + ConfigUpdateOptions, + DEFAULT_CONFIG, + ProviderConfig as ConfigProviderConfig, // alias to avoid clash with providers' ProviderConfig +} from "./configTypes.js";
🧹 Nitpick comments (6)
src/lib/types/configTypes.ts (1)
33-34: Tighten feature literals to prevent typos.Constrain
featuresto a union instead of plainstring[].+export type ProviderFeature = "streaming" | "functionCalling" | "vision"; export type ProviderConfig = { @@ - features?: string[]; // ['streaming', 'functionCalling', 'vision'] + features?: ProviderFeature[]; // ['streaming', 'functionCalling', 'vision']src/lib/types/index.ts (1)
11-13: Redundant provider re-exports.You already
export * from "./providers.js"; the laterexport type { …, ProviderConfig } from "./providers.js";is redundant and increases confusion around the twoProviderConfigs.-export type { AISDKModel, ProviderError, ProviderConfig } from "./providers.js"; +// Already exported via wildcard above. If you need type-only, drop the wildcard instead. +// export type { AISDKModel, ProviderError, ProviderConfig } from "./providers.js";Alternatively, remove the wildcard and keep the type-only export if that was intentional for tree-shaking.
Also applies to: 46-46
src/lib/utils/toolUtils.ts (2)
22-24: Normalize boolean env parsing to handle case/whitespace.Current checks only match
"true". Accept common truthy variants to reduce operator error.+function envTrue(name: string, defaultValue = false): boolean { + const v = process.env[name]; + if (v == null) return defaultValue; + return /^(1|true|yes|on)$/i.test(v.trim()); +} @@ - return process.env.NEUROLINK_DISABLE_BUILTIN_TOOLS === "true"; + return envTrue("NEUROLINK_DISABLE_BUILTIN_TOOLS"); @@ - return process.env.NEUROLINK_DISABLE_CUSTOM_TOOLS !== "true"; + return !envTrue("NEUROLINK_DISABLE_CUSTOM_TOOLS"); @@ - return process.env.NEUROLINK_DISABLE_MCP_TOOLS !== "true"; + return !envTrue("NEUROLINK_DISABLE_MCP_TOOLS");Also applies to: 36-37, 49-50
62-70: Parse max value more strictly.
parseIntwill accept prefixes like100abc. UseNumber()and validate integer/finiteness.- const envMax = process.env.NEUROLINK_MAX_TOOLS_PER_PROVIDER; - if (envMax) { - const parsed = parseInt(envMax, 10); - if (!isNaN(parsed) && parsed > 0) { - return parsed; - } - } + const raw = process.env.NEUROLINK_MAX_TOOLS_PER_PROVIDER?.trim(); + if (raw) { + const parsed = Number(raw); + if (Number.isInteger(parsed) && parsed > 0) { + return parsed; + } + }src/lib/config/configManager.ts (2)
7-8: Use Node built-in specifiers for clarity (optional).The switch to named imports is good. Consider
node:specifiers to avoid polyfill collisions in bundlers.-import { join } from "path"; -import { createHash } from "crypto"; +import { join } from "node:path"; +import { createHash } from "node:crypto";
375-381: Make config hashing deterministic for nested objects.
JSON.stringify(config, Object.keys(config).sort())only sorts top-level keys; nested objects can change order and produce different hashes for equivalent configs.Apply:
- const configString = JSON.stringify(config, Object.keys(config).sort()); - return createHash("sha256") + const configString = this.stableStringify(config); + return createHash("sha256") .update(configString) .digest("hex") .substring(0, 8);Add this helper inside the class:
private stableStringify(value: unknown): string { return JSON.stringify(value, (_k, v) => { if (v && typeof v === "object" && !Array.isArray(v)) { const obj = v as Record<string, unknown>; return Object.keys(obj).sort().reduce((acc, key) => { (acc as Record<string, unknown>)[key] = obj[key]; return acc; }, {} as Record<string, unknown>); } return v; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/config/configManager.ts(6 hunks)src/lib/types/configTypes.ts(4 hunks)src/lib/types/index.ts(1 hunks)src/lib/utils/toolUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/types/configTypes.ts (1)
src/lib/types/index.ts (1)
ProviderConfig(46-46)
🔇 Additional comments (3)
src/lib/types/configTypes.ts (1)
163-205: DEFAULT_CONFIG looks consistent with utils.Defaults align with
toolUtilsbehavior (e.g.,maxToolsPerProvider: 100). No issues spotted.src/lib/utils/toolUtils.ts (1)
7-7: Import path update looks good.Centralizing on
../types/configTypes.jsaligns with the new layout.src/lib/config/configManager.ts (1)
111-111: Path joins updated — LGTM.Consistent
join(...)usage improves portability and readability.Also applies to: 153-153, 185-185
| } from "../types/configTypes.js"; | ||
| import { DEFAULT_CONFIG } from "../types/configTypes.js"; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify TS ESM settings for .js-suffixed imports.
Importing ../types/configTypes.js from a .ts file typically requires moduleResolution: "NodeNext" | "Bundler" and verbatimModuleSyntax: true (or equivalent). Please confirm repo settings and that the new central types re-export is in place.
🏁 Script executed:
#!/bin/bash
# Check tsconfig module + resolution
fd -a 'tsconfig*.json' | while read -r f; do
echo "== $f =="; rg -n -C2 '"module"\s*:' "$f"; rg -n -C2 '"moduleResolution"\s*:' "$f"; rg -n -C2 '"verbatimModuleSyntax"\s*:' "$f"
done
# Ensure DEFAULT_CONFIG is exported from the new location
rg -nC2 'export\s+const\s+DEFAULT_CONFIG' src/lib/types/configTypes.ts
# Ensure public re-export exists
rg -nC2 'export\s+\*\s+from\s+"\.\/configTypes\.js"' src/lib/types/index.ts src/lib/types/index.tsx 2>/dev/null || trueLength of output: 476
Add public re‐export for configTypes.js
There’s no export * from './configTypes.js' in src/lib/types/index.ts—add it to centralize your type exports.
🤖 Prompt for AI Agents
In src/lib/config/configManager.ts around lines 17-19: the project lacks a
public re-export for configTypes.js in src/lib/types/index.ts, so add export *
from './configTypes.js' to src/lib/types/index.ts to centralize and expose those
types; after adding the re-export, update any imports if necessary to import
from src/lib/types (or the package's types index) and run a build/type-check to
confirm there are no unresolved import errors.
| // Configuration types | ||
| export * from "./configTypes.js"; | ||
|
|
There was a problem hiding this comment.
Barrel currently exports duplicate ProviderConfig symbols.
- You do
export * from "./configTypes.js"(which includes aProviderConfig). - You also export
ProviderConfigfrom./providers.js.
This collides and will fail type-checking.
Resolve by explicitly exporting config types and aliasing ProviderConfig from configTypes to ConfigProviderConfig (keeping the providers’ ProviderConfig intact).
Apply:
-// Configuration types
-export * from "./configTypes.js";
+// Configuration types (explicit, with alias to avoid name clash)
+export {
+ NeuroLinkConfig,
+ PerformanceConfig,
+ CacheConfig,
+ FallbackConfig,
+ RetryConfig,
+ AnalyticsConfig,
+ ToolConfig,
+ BackupInfo,
+ BackupMetadata,
+ ConfigValidationResult,
+ ConfigUpdateOptions,
+ DEFAULT_CONFIG,
+ ProviderConfig as ConfigProviderConfig,
+} from "./configTypes.js";Also applies to: 46-46
🤖 Prompt for AI Agents
In src/lib/types/index.ts around lines 20-22 and also at line 46, the barrel
export re-exports ProviderConfig twice (once via export * from
"./configTypes.js" and again from "./providers.js"), causing a type collision;
fix by changing the wildcard re-export of configTypes to an explicit list that
re-exports all needed config types but renames ProviderConfig to
ConfigProviderConfig (i.e., import or export ProviderConfig from
./configTypes.js under the alias ConfigProviderConfig) while leaving the
providers.js export of ProviderConfig untouched so only the aliased config
version is exposed from this barrel.
💡 Verification agent
🧩 Analysis chain
Sanity check for breaking API.
If consumers imported ProviderConfig from the barrel intending the config shape, they’ll now need ConfigProviderConfig. Consider a deprecation period by also re-exporting:
export type { ProviderConfig as ProviderConfig_Deprecated_Config } from "./configTypes.js";Run to find external/internal imports relying on the config-shaped ProviderConfig from the barrel:
🏁 Script executed:
#!/bin/bash
# Identify barrel imports of ProviderConfig
rg -nP -C2 'from\s+["'\'']\.\/types\/index\.js["'\''].*\{[^}]*ProviderConfig[^}]*\}'
# Identify direct imports of config ProviderConfig (safe)
rg -nP -C2 'from\s+["'\'']\.\/types\/configTypes\.js["'\''].*\{[^}]*ProviderConfig[^}]*\}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any import or export references to ProviderConfig in .ts and .js files
rg -nP '(import|export)\s+.*\bProviderConfig\b' -g '*.ts' -g '*.js'Length of output: 853
Export a deprecated alias for the original ProviderConfig
Add the following below the wildcard export to preserve the old config‐shape type before removing it in a future release:
export type { ProviderConfig as DeprecatedProviderConfig } from "./configTypes.js";This ensures any consumers importing ProviderConfig from the barrel keep the config‐shape type and can migrate on the deprecation schedule.
🤖 Prompt for AI Agents
In src/lib/types/index.ts around lines 20 to 22, the barrel export currently
re-exports configTypes but does not provide a deprecated alias for the previous
ProviderConfig; add a named type re-export below the existing wildcard export to
preserve the old type shape for consumers, specifically add a line exporting
ProviderConfig as DeprecatedProviderConfig from "./configTypes.js" so downstream
code can continue to import the legacy name during the deprecation window.
f9160df to
5fab24c
Compare
… system - Move all configuration types from src/lib/config/types.ts to src/lib/types/configTypes.ts - Convert all 14 interfaces to types following established architecture pattern - Update imports in configManager.ts to use centralized types - Update toolUtils.ts import path for ToolConfig type - Add configTypes.ts to centralized type exports in index.ts - Remove old config/types.ts file entirely in favor of centralized approach - Fix crypto and path import issues in configManager.ts (use named imports) - Maintain full backward compatibility through proper type re-exports
5fab24c to
f0d6bef
Compare
2 participants
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
Refactor
Chores