fix: resolve engineering practice violations (Rules 6, 7, 8)#1044
fix: resolve engineering practice violations (Rules 6, 7, 8)#1044murdore wants to merge 1 commit into
Conversation
Closes juspay#1014 - Rule 6: formatProviderError must return, never throw - Changed throw to return in src/lib/providers/litellm.ts:556 Closes juspay#1015 - Rule 8: No 'Types' suffix in type filenames - Renamed fileTypes.ts -> fileExtensions.ts - Renamed mimeTypes.ts -> mimeConstants.ts - Updated barrel imports in config/index.ts Closes juspay#1016 - Rule 7: Zero interface, always use type - Converted 78 interface declarations to type aliases across: - docs-site/ (17 in 8 files) - scripts/ (34 in 16 files) - tools/ (11 in 4 files) - neurolink-demo/ (10 in 5 files) - landing/src/ (6 in 4 files) - test/ + vite.config.ts (3 in 2 files) - Used intersection types (&) instead of extends clauses All changes are non-breaking: type aliases are structurally equivalent to interfaces, filename renames preserve exports, and the throw->return fix corrects a bug per Rule 6.
|
Someone is attempting to deploy a commit to the Sachin Sharma's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR modernizes the TypeScript type system across auxiliary code to achieve CLAUDE.md Rule 7 compliance, fixes a critical error-handling violation in LiteLLM embeddings (Rule 6), and updates module re-exports following file renames (Rule 8). The bulk of the work converts ~80 ChangesType System Modernization and Rule Compliance
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/lib/processors/config/index.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'. src/lib/providers/litellm.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'. 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Mechanical refactor to fix engineering practice violations: converts interface declarations to type aliases across the codebase, renames type-related filenames, and changes a throw to return in the LiteLLM provider's error formatting path.
Changes:
- Converted 78
interfacedeclarations totypealiases across 40 files. - Renamed
fileTypes.ts→fileExtensions.tsandmimeTypes.ts→mimeConstants.ts, updating the barrel re-exports inconfig/index.ts. - Changed
throw this.formatProviderError(...)toreturn this.formatProviderError(...)insrc/lib/providers/litellm.ts.
Reviewed changes
Copilot reviewed 40 out of 42 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/providers/litellm.ts | Replaces throw with return on formatProviderError call. |
| src/lib/processors/config/index.ts | Updates barrel imports for renamed mime/file modules. |
| vite.config.ts | Converts VitestConfig interface to type with intersection. |
| tools/testing/performanceMonitor.ts | Interface→type conversions for benchmark/result/results. |
| tools/testing/adaptiveTestRunner.ts | Interface→type conversions for runner config/results. |
| tools/content/videoCleanup.ts | Interface→type conversions for video info/analysis/results. |
| tools/automation/projectOrganizer.ts | Interface→type conversions for organizer types. |
| scripts/testing/executeAllTestsParallel.ts | Interface→type conversions for test case/result. |
| scripts/security-check.ts | Interface→type conversions for security issue/finding/results. |
| scripts/sdkComprehensiveTest.ts | Interface→type for SdkTestResult. |
| scripts/quickVerification.ts | Interface→type for VerifyEntry. |
| scripts/quality-metrics.ts | Interface→type conversions for all metrics types. |
| scripts/phase6ValidationCleanup.ts | Interface→type conversions for options/results. |
| scripts/modelServer.ts | Interface→type for ModelData/ModelConfig. |
| scripts/examples/demo-without-keys.ts | Interface→type conversions for mock types. |
| scripts/env-validation.ts | Interface→type conversions for issue/pattern/provider. |
| scripts/criticalTest.ts | Interface→type for TestResult. |
| scripts/createCliOverviewVideo.ts | Interface→type for CLICommand. |
| scripts/correctedSdkTest.ts | Interface→type for SdkTestResult. |
| scripts/correctedFunctionalityTest.ts | Interface→type for TestResult. |
| scripts/coreFunctionalityTest.ts | Interface→type for TestEntry. |
| scripts/comprehensiveTest.ts | Interface→type for LogEntry. |
| scripts/benchmarkThreeProviders.ts | Interface→type conversions for benchmark types. |
| neurolink-demo/server.ts | Interface→type for SessionMessage/Session. |
| neurolink-demo/mcp-workflow.ts | Interface→type for command/workflow results. |
| neurolink-demo/mcp-helpers.ts | Interface→type conversions for MCP types. |
| neurolink-demo/mcp-examples.ts | Interface→type for CommandResult. |
| neurolink-demo/create-comprehensive-demo-videos.ts | Interface→type for DemoAction. |
| landing/src/routes/api/og/templates.ts | Interface→type for OGParams. |
| landing/src/lib/stores/canvasState.ts | Interface→type for SectionCanvasConfig. |
| landing/src/lib/lsystem.ts | Interface→type conversions for LBranch/LSystemOptions/NeuronPath. |
| landing/src/lib/actions/reveal.ts | Interface→type for RevealOptions. |
| docs-site/src/theme/hooks/useNewDocs.ts | Interface→type for DocMetadata/NewDocsData. |
| docs-site/src/hooks/useLocalSearch.ts | Interface→type for SearchDocument/UseLocalSearchOptions. |
| docs-site/src/hooks/useAlgoliaSearch.ts | Interface→type for SearchResult/options/return. |
| docs-site/scripts/validate-frontmatter.ts | Interface→type conversions for validation types. |
| docs-site/scripts/sync-docs.ts | Interface→type for TransformResult/FileInfo. |
| docs-site/scripts/gitChangeDetector.ts | Interface→type for GitChangeInfo. |
| docs-site/scripts/create-version.ts | Interface→type for PackageJson. |
| docs-site/scripts/build-llms-txt.ts | Interface→type for PriorityRule/DocFile/ProviderInfo. |
Comments suppressed due to low confidence (3)
src/lib/providers/litellm.ts:1
- Changing
throwtoreturnhere is likely incorrect and changes runtime behavior in a breaking way.formatProviderErrorreturns anErrorobject (its name and surroundingthrow this.formatProviderError(...)usages elsewhere imply this), so returning it from a function whose normal return type is the successful operation result will produce anErrorinstance as a value rather than propagating the failure. Downstream code that previously relied on the thrown exception will now silently receive anErrorobject as if it were a valid result, causing incorrect behavior or hard-to-diagnose runtime failures. If the rule requires thatformatProviderErroritself never throw, the fix should be applied insideformatProviderError(make it return anError) while the call site keepsthrow this.formatProviderError(...). Please revert this line tothrowor refactor the call site to throw the returned error (e.g.throw this.formatProviderError(...)).
src/lib/processors/config/index.ts:1 - Renaming
mimeTypes.tstomimeConstants.tsto avoid theTypessuffix is reasonable, butmimeConstantsis less descriptive than the original (the file exports MIME type groupings, not generic constants). Consider a more domain-accurate name such asmimeGroups.tsormimeCategories.tsto preserve clarity while still complying with the no-Types-suffix rule. Optional.
tools/automation/projectOrganizer.ts:1 Categorizationpreviously usedinterfacewith an index signature[key: string]: string[]alongside named properties. Withinterface, TypeScript validates each named property's type against the index signature at declaration time; converting to atypealias preserves the same structural shape but the index signature still requires every named property to be assignable tostring[], which they are here, so this specific case is fine. However, note that for interfaces elsewhere that were extended (rather than just declared), intersection (&) is not always semantically equivalent — particularly with method overloads and declaration merging. Please audit any conversion where the originalinterfacewas being merged or extended externally; mechanicalinterface → typeconversion can silently break such cases.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 (1)
src/lib/providers/litellm.ts (1)
984-989:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix LiteLLM embedMany error path: throw
formatProviderError(...)instead of returning itIn
src/lib/providers/litellm.tscallEmbeddings()(around lines 977-990), the!res.okbranch doesreturn this.formatProviderError(new Error(...)).callEmbeddings()is typed to returnPromise<number[][]>, whileformatProviderError()returns anErrorobject—this breaks the return type contract and also preventssrc/lib/server/routes/agentRoutes.tsfrom hitting itscatchblock (it only catches thrown errors).Proposed fix
if (!res.ok) { const bodyText = await res.text().catch(() => ""); const parsed = bodyText ? (JSON.parse(bodyText) as { error?: { message?: string }; }) : undefined; - return this.formatProviderError( + throw this.formatProviderError( new Error( parsed?.error?.message || `LiteLLM ${operation} failed with status ${res.status}`, ), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/providers/litellm.ts` around lines 984 - 989, In callEmbeddings(), the error branch currently does "return this.formatProviderError(...)" which returns an Error object and violates the Promise<number[][]> contract and prevents upstream catch handlers; replace that return with "throw this.formatProviderError(...)" so the formatted Error is thrown (not returned) and will be caught by callers; refer to the callEmbeddings function and the formatProviderError method when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/providers/litellm.ts`:
- Around line 984-989: In callEmbeddings(), the error branch currently does
"return this.formatProviderError(...)" which returns an Error object and
violates the Promise<number[][]> contract and prevents upstream catch handlers;
replace that return with "throw this.formatProviderError(...)" so the formatted
Error is thrown (not returned) and will be caught by callers; refer to the
callEmbeddings function and the formatProviderError method when making this
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b12fd10f-6d43-4d68-8eb2-db4fa2a0f7b5
📒 Files selected for processing (42)
docs-site/scripts/build-llms-txt.tsdocs-site/scripts/create-version.tsdocs-site/scripts/gitChangeDetector.tsdocs-site/scripts/sync-docs.tsdocs-site/scripts/validate-frontmatter.tsdocs-site/src/hooks/useAlgoliaSearch.tsdocs-site/src/hooks/useLocalSearch.tsdocs-site/src/theme/hooks/useNewDocs.tslanding/src/lib/actions/reveal.tslanding/src/lib/lsystem.tslanding/src/lib/stores/canvasState.tslanding/src/routes/api/og/templates.tsneurolink-demo/create-comprehensive-demo-videos.tsneurolink-demo/mcp-examples.tsneurolink-demo/mcp-helpers.tsneurolink-demo/mcp-workflow.tsneurolink-demo/server.tsscripts/benchmarkThreeProviders.tsscripts/comprehensiveTest.tsscripts/coreFunctionalityTest.tsscripts/correctedFunctionalityTest.tsscripts/correctedSdkTest.tsscripts/createCliOverviewVideo.tsscripts/criticalTest.tsscripts/env-validation.tsscripts/examples/demo-without-keys.tsscripts/modelServer.tsscripts/phase6ValidationCleanup.tsscripts/quality-metrics.tsscripts/quickVerification.tsscripts/sdkComprehensiveTest.tsscripts/security-check.tsscripts/testing/executeAllTestsParallel.tssrc/lib/processors/config/fileExtensions.tssrc/lib/processors/config/index.tssrc/lib/processors/config/mimeConstants.tssrc/lib/providers/litellm.tstools/automation/projectOrganizer.tstools/content/videoCleanup.tstools/testing/adaptiveTestRunner.tstools/testing/performanceMonitor.tsvite.config.ts
Summary
Fixes all engineering practice violations identified during audit against CLAUDE.md critical rules. Rebased onto latest
releasebranch.Issues Resolved
Closes [CRITICAL] formatProviderError throws instead of returning (Rule 6 violation) #1014 — [CRITICAL] Rule 6:
formatProviderErrormust return, never throwthrow→returninsrc/lib/providers/litellm.tsCloses [MEDIUM] Files with "Types" suffix violate naming convention (Rule 8) #1015 — [MEDIUM] Rule 8: No "Types" suffix in type filenames
fileTypes.ts→fileExtensions.tsmimeTypes.ts→mimeConstants.tsconfig/index.tsCloses [LOW] 80 interface declarations should be converted to types (Rule 7) #1016 — [LOW] Rule 7: Zero
interface, always usetypetypealiases across 40 files&) instead ofextendsclausesVerification
interfacedeclarationstypealiases structurally equivalent tointerfacethrow→returnfix corrects a bug per Rule 6release— no merge conflictsFiles Changed: 42
src/lib/providers/litellm.tsinterface→typeconversionsSummary by CodeRabbit
Refactor
Bug Fixes